subagent: hand a subagent its parent's built-in tools, and no others
`build_config` spawned a subagent with `--dangerously-skip-permissions` and no `--tools` at all, so it got claude's entire built-in set — `SendMessage` and `ListAgents` (message peers, or the operator, as its parent), `Task*` including `TaskStop`, which takes an *agent* id and so reaches clean outside the run, `Cron*`, `RemoteTrigger` and `EnterWorktree`/`ExitWorktree`. None of that is part of "do this bounded task in this directory", and none of it is something the parent agent itself can do: the harness has always passed `--tools`. Pass the same one. The value comes from `hive_sh4re::permissions::builtin_tools_arg()` — literally the function the harness resolves its own session with — so the subagent's set is the parent's set, `HIVE_TOOL_GROUPS` and all. That inheritance is the requirement, not an implementation detail: a hardcoded subagent list would hand `WebFetch`/`WebSearch` to the subagent of an agent without the `web_tools` group, which is a privilege escalation, and would drift from the parent's list the first time anyone added a tool to either. `--tools` is the real gate: it holds under `--dangerously-skip-permissions`, unlike `--allowedTools`, which only auto-approves prompts. It does not filter MCP tools, so the `goal_reached`/`need_help` signal surface is deliberately unnamed in it and survives on `--strict-mcp-config` alone. `build_config`'s doc comment claimed `strict_mcp_config` was *the* safety property and that a subagent got "nothing implicit and nothing more". That was false for built-ins, and is what hid this gap for as long as it did; it now says which flag covers which half and that neither substitutes for the other. An empty `--tools` value parses as *unset* and grants more than omitting the flag, so an empty resolution can only be a bug — `build_config` asserts against it and a test pins the non-emptiness alongside the subset-of-parent property. Refs #4416
This commit is contained in:
parent
8b01dbeef1
commit
d6c8cd5a6f
4 changed files with 156 additions and 16 deletions
1
Cargo.lock
generated
1
Cargo.lock
generated
|
|
@ -1995,6 +1995,7 @@ dependencies = [
|
||||||
"clap",
|
"clap",
|
||||||
"hive-agent-sock",
|
"hive-agent-sock",
|
||||||
"hive-claude",
|
"hive-claude",
|
||||||
|
"hive-sh4re",
|
||||||
"hive-sock-client",
|
"hive-sock-client",
|
||||||
"hive-types",
|
"hive-types",
|
||||||
"libc",
|
"libc",
|
||||||
|
|
|
||||||
|
|
@ -218,13 +218,50 @@ agent that needs a stable or non-default port.
|
||||||
Own systemd unit, defined alongside the other per-agent MCP daemons in
|
Own systemd unit, defined alongside the other per-agent MCP daemons in
|
||||||
`nix/agent-modules/mcp.nix`.
|
`nix/agent-modules/mcp.nix`.
|
||||||
|
|
||||||
## MCP servers available to a subagent
|
## The tool surface a subagent gets
|
||||||
|
|
||||||
|
Two flags, each covering one half, and neither covering the other:
|
||||||
|
`--tools` governs claude's built-in tools, `--strict-mcp-config` governs
|
||||||
|
the MCP ones. Dropping either brings that half back in full; in
|
||||||
|
particular `--tools` does **not** filter `mcp__*` tools.
|
||||||
|
|
||||||
|
### Built-in tools (`--tools`)
|
||||||
|
|
||||||
|
**A subagent gets exactly the built-ins its parent agent has** — the same
|
||||||
|
list, resolved by the same function
|
||||||
|
(`hive_sh4re::permissions::builtin_tools_arg`) from the same
|
||||||
|
`HIVE_TOOL_GROUPS`: `Edit`, `Glob`, `Grep`, `Read`, `Skill`,
|
||||||
|
`Write`, plus `WebFetch`/`WebSearch` for an agent granted the `web_tools`
|
||||||
|
tool group and not otherwise. See
|
||||||
|
[the harness's own allowlist](../turn-loop/mcp.md#tool-allowlist-hive_sh4repermissionsallowed_builtin_tools)
|
||||||
|
for what that list contains and why.
|
||||||
|
|
||||||
|
Inheriting rather than listing is the point: a hardcoded subagent list
|
||||||
|
would hand web egress to the subagent of an agent that isn't allowed web
|
||||||
|
egress, and would diverge from the parent's on the first tool anyone adds
|
||||||
|
to either.
|
||||||
|
|
||||||
|
Everything else in claude's built-in set is absent, in particular the
|
||||||
|
tools that let a session act outside the run it was started for: peer and
|
||||||
|
operator messaging, nested agents (including the stop verb, which takes
|
||||||
|
an _agent_ id rather than a session), schedule and webhook creation, and
|
||||||
|
worktree switching. Before this flag was passed, a subagent reached all of
|
||||||
|
them — `--dangerously-skip-permissions` had removed the only thing that
|
||||||
|
would have asked, and `--allowedTools` would not have helped: it approves
|
||||||
|
prompts in advance rather than restricting anything.
|
||||||
|
|
||||||
|
One trap worth knowing before editing any of this: an empty `--tools`
|
||||||
|
value parses as _unset_ and grants **more** than omitting the flag, so
|
||||||
|
there is no way to spell "no built-in tools" — the daemon asserts rather
|
||||||
|
than emitting one.
|
||||||
|
|
||||||
|
### MCP servers (`--strict-mcp-config`)
|
||||||
|
|
||||||
A subagent runs with `--strict-mcp-config` and, by default, exactly one
|
A subagent runs with `--strict-mcp-config` and, by default, exactly one
|
||||||
MCP server: the two-tool `subagent_control` route above. It otherwise
|
MCP server: the two-tool `subagent_control` route above — not the
|
||||||
falls back to claude's own native tools (`Bash`, `WebFetch`, etc.), not
|
parent's `mcp__bash__*` / `mcp__hyperhive__*` surface. Those two signal
|
||||||
the parent's `mcp__bash__*` / `mcp__hyperhive__*` surface. Nothing
|
tools are deliberately unnamed in `--tools`, which doesn't govern them;
|
||||||
implicit reaches it: the built-in
|
they survive on this flag alone. Nothing implicit reaches it: the built-in
|
||||||
hyperhive surface (todos/messaging) isn't an `extraMcpServers` entry at
|
hyperhive surface (todos/messaging) isn't an `extraMcpServers` entry at
|
||||||
all, and the automatically injected `bash`/`subagent` entries default to excluded
|
all, and the automatically injected `bash`/`subagent` entries default to excluded
|
||||||
too (a subagent can't spawn hive-bash tasks or its own nested subagents
|
too (a subagent can't spawn hive-bash tasks or its own nested subagents
|
||||||
|
|
|
||||||
|
|
@ -13,6 +13,10 @@ axum.workspace = true
|
||||||
clap.workspace = true
|
clap.workspace = true
|
||||||
hive-agent-sock.workspace = true
|
hive-agent-sock.workspace = true
|
||||||
hive-claude.workspace = true
|
hive-claude.workspace = true
|
||||||
|
# `permissions::builtin_tools_arg` — the same `--tools` resolution the parent
|
||||||
|
# harness spawns its own claude with, so a subagent's built-in surface is its
|
||||||
|
# parent's rather than a second list that drifts. See `session::build_config`.
|
||||||
|
hive-sh4re.workspace = true
|
||||||
hive-sock-client.workspace = true
|
hive-sock-client.workspace = true
|
||||||
hive-types.workspace = true
|
hive-types.workspace = true
|
||||||
libc.workspace = true
|
libc.workspace = true
|
||||||
|
|
|
||||||
|
|
@ -834,18 +834,18 @@ fn subagent_otel_attrs(name: &str) -> String {
|
||||||
/// (`high` on most models) — a deliberate hive policy for subagent work
|
/// (`high` on most models) — a deliberate hive policy for subagent work
|
||||||
/// specifically, not a reflection of Anthropic's own recommendation, on the
|
/// specifically, not a reflection of Anthropic's own recommendation, on the
|
||||||
/// same "cheaper than you" cost-consciousness the `base:claude-subagents`
|
/// same "cheaper than you" cost-consciousness the `base:claude-subagents`
|
||||||
/// skill already asks of `model`. `prompt_file`, when
|
/// skill already asks of `model`. `prompt_file`, when given, becomes
|
||||||
/// given, becomes `--append-system-prompt-file` — the subagent's task
|
/// `--append-system-prompt-file` — the subagent's task instructions.
|
||||||
/// instructions. `dir`, when given, becomes `Config::cwd` (e.g. a worktree
|
/// `dir`, when given, becomes `Config::cwd` (e.g. a worktree
|
||||||
/// the caller already prepared); `None` inherits this daemon's own working
|
/// the caller already prepared); `None` inherits this daemon's own working
|
||||||
/// directory, same as before this field existed. Always
|
/// directory, same as before this field existed. Always
|
||||||
/// `--dangerously-skip-permissions --strict-mcp-config` — the safety
|
/// `--dangerously-skip-permissions --strict-mcp-config --tools` — two gates
|
||||||
/// property is `strict_mcp_config: true` with no ambient MCP discovery, not
|
/// covering one half each, neither substituting for the other:
|
||||||
/// an unconditional absence of `--mcp-config`: a subagent gets exactly the
|
/// `strict_mcp_config` over **MCP** tools (no ambient discovery, so exactly
|
||||||
/// `hyperhive.extraMcpServers` entries an operator has explicitly opted in
|
/// what [`crate::mcp_config::build`] renders), `--tools` over **built-in**
|
||||||
/// via `availableToSubagents = true` (`crate::mcp_config::build`), plus this
|
/// ones, via [`hive_sh4re::permissions::builtin_tools_arg`] — the parent
|
||||||
/// daemon's own two-tool signal surface when `signal_url` is given — nothing
|
/// agent's own resolved set, never wider. `--tools` does not filter
|
||||||
/// implicit and nothing more.
|
/// `mcp__*`, so the signal tools are unnamed in it (`docs/tools/subagent.md`).
|
||||||
///
|
///
|
||||||
/// `signal_url` is what makes `goal_reached`/`need_help` callable at all: a
|
/// `signal_url` is what makes `goal_reached`/`need_help` callable at all: a
|
||||||
/// subagent reaches them over the same streamable-http listener its parent
|
/// subagent reaches them over the same streamable-http listener its parent
|
||||||
|
|
@ -865,7 +865,21 @@ fn build_config(
|
||||||
dir: Option<&str>,
|
dir: Option<&str>,
|
||||||
signal_url: Option<&str>,
|
signal_url: Option<&str>,
|
||||||
) -> Config {
|
) -> Config {
|
||||||
let mut extra_args = vec!["--dangerously-skip-permissions".to_owned()];
|
let tools = hive_sh4re::permissions::builtin_tools_arg();
|
||||||
|
// An empty `--tools` value parses as *unset* and yields MORE tools than
|
||||||
|
// omitting the flag, so there is no way to spell "no built-ins" — an
|
||||||
|
// empty resolution is a bug, and failing here is louder than silently
|
||||||
|
// spawning an unrestricted subagent.
|
||||||
|
assert!(
|
||||||
|
!tools.is_empty(),
|
||||||
|
"resolved an empty --tools value — that parses as unset and would \
|
||||||
|
grant the subagent every built-in claude has"
|
||||||
|
);
|
||||||
|
let mut extra_args = vec![
|
||||||
|
"--dangerously-skip-permissions".to_owned(),
|
||||||
|
"--tools".to_owned(),
|
||||||
|
tools,
|
||||||
|
];
|
||||||
if let Some(path) = prompt_file {
|
if let Some(path) = prompt_file {
|
||||||
extra_args.push("--append-system-prompt-file".to_owned());
|
extra_args.push("--append-system-prompt-file".to_owned());
|
||||||
extra_args.push(path.to_owned());
|
extra_args.push(path.to_owned());
|
||||||
|
|
@ -1968,6 +1982,90 @@ mod tests {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The `--tools` value as it reaches the spawned argv.
|
||||||
|
fn spawned_tools(config: &Config) -> &str {
|
||||||
|
let flag = config
|
||||||
|
.extra_args
|
||||||
|
.iter()
|
||||||
|
.position(|a| a == "--tools")
|
||||||
|
.expect("--tools is passed on every spawn");
|
||||||
|
config
|
||||||
|
.extra_args
|
||||||
|
.get(flag + 1)
|
||||||
|
.expect("--tools carries a value")
|
||||||
|
}
|
||||||
|
|
||||||
|
/// `--tools` reaches the argv on every caller shape, carrying a non-empty
|
||||||
|
/// value. Non-empty is the load-bearing half: an empty `--tools` parses as
|
||||||
|
/// *unset* and hands the subagent **more** built-ins than omitting the
|
||||||
|
/// flag would, so "we passed the flag" is not on its own the restriction.
|
||||||
|
/// Checked for each shape because a flag that appears on only some spawn
|
||||||
|
/// paths is no restriction at all. `signal_url` stays `None` throughout —
|
||||||
|
/// it only steers `mcp_config`, which `--tools` does not govern, and
|
||||||
|
/// passing it would make this test need the container's injected
|
||||||
|
/// `HYPERHIVE_HARNESS_DIR`.
|
||||||
|
#[test]
|
||||||
|
fn build_config_always_passes_a_non_empty_tools_list() {
|
||||||
|
for config in [
|
||||||
|
build_config("n", None, None, None, None, None),
|
||||||
|
build_config("n", None, None, Some("/tmp/p.md"), None, None),
|
||||||
|
build_config("n", None, None, None, Some("/tmp/wt"), None),
|
||||||
|
] {
|
||||||
|
let tools = spawned_tools(&config);
|
||||||
|
assert!(!tools.is_empty(), "--tools must never be empty");
|
||||||
|
assert!(tools.split(',').all(|t| !t.trim().is_empty()));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// A subagent's built-ins are a subset of its parent's resolved set —
|
||||||
|
/// the property that makes this safe to ship, since a subagent that can
|
||||||
|
/// reach a tool its parent cannot is a privilege escalation dressed as a
|
||||||
|
/// convenience. Both sides are read from
|
||||||
|
/// `hive_sh4re::permissions`, deliberately: it is the parent harness's
|
||||||
|
/// own resolver, so this compares against what the parent actually gets
|
||||||
|
/// rather than against a restatement of it that could drift.
|
||||||
|
#[test]
|
||||||
|
fn a_subagent_gets_no_builtin_its_parent_lacks() {
|
||||||
|
let parent = hive_sh4re::permissions::builtin_tools_for(
|
||||||
|
&hive_sh4re::permissions::effective_tool_groups(),
|
||||||
|
);
|
||||||
|
let config = build_config("n", None, None, None, None, None);
|
||||||
|
for tool in spawned_tools(&config).split(',') {
|
||||||
|
assert!(
|
||||||
|
parent.contains(&tool),
|
||||||
|
"subagent got {tool}, which the parent's resolved set does not have"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Named individually rather than inferred from "the list is short": the
|
||||||
|
/// tools this daemon used to hand a subagent that reach outside the run
|
||||||
|
/// it was started for. Adding one back has to be a deliberate edit here.
|
||||||
|
/// They are absent because the parent has never had them, which is the
|
||||||
|
/// point — this pins the consequence, not a second allow-list.
|
||||||
|
#[test]
|
||||||
|
fn no_spawned_tool_escapes_the_session() {
|
||||||
|
let config = build_config("n", None, None, None, None, None);
|
||||||
|
let tools: Vec<&str> = spawned_tools(&config).split(',').collect();
|
||||||
|
for escaping in [
|
||||||
|
"SendMessage",
|
||||||
|
"ListAgents",
|
||||||
|
"Task",
|
||||||
|
"TaskStop",
|
||||||
|
"TaskOutput",
|
||||||
|
"CronCreate",
|
||||||
|
"CronDelete",
|
||||||
|
"RemoteTrigger",
|
||||||
|
"EnterWorktree",
|
||||||
|
"ExitWorktree",
|
||||||
|
] {
|
||||||
|
assert!(
|
||||||
|
!tools.contains(&escaping),
|
||||||
|
"{escaping} must not be in a subagent's --tools"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn resolve_dir_remembers_an_explicit_dir_and_falls_back_to_it_when_omitted() {
|
fn resolve_dir_remembers_an_explicit_dir_and_falls_back_to_it_when_omitted() {
|
||||||
let state = State::new(PathBuf::from("/dev/null"), signal_url());
|
let state = State::new(PathBuf::from("/dev/null"), signal_url());
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue