diff --git a/Cargo.lock b/Cargo.lock index de1729f7..1e0f2b33 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1995,6 +1995,7 @@ dependencies = [ "clap", "hive-agent-sock", "hive-claude", + "hive-sh4re", "hive-sock-client", "hive-types", "libc", diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index f797904c..c2039d1b 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -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 `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 -MCP server: the two-tool `subagent_control` route above. It otherwise -falls back to claude's own native tools (`Bash`, `WebFetch`, etc.), not -the parent's `mcp__bash__*` / `mcp__hyperhive__*` surface. Nothing -implicit reaches it: the built-in +MCP server: the two-tool `subagent_control` route above — not the +parent's `mcp__bash__*` / `mcp__hyperhive__*` surface. Those two signal +tools are deliberately unnamed in `--tools`, which doesn't govern them; +they survive on this flag alone. Nothing implicit reaches it: the built-in hyperhive surface (todos/messaging) isn't an `extraMcpServers` entry at 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 diff --git a/hive-subagent-mcp/Cargo.toml b/hive-subagent-mcp/Cargo.toml index f7223e86..e1719919 100644 --- a/hive-subagent-mcp/Cargo.toml +++ b/hive-subagent-mcp/Cargo.toml @@ -13,6 +13,10 @@ axum.workspace = true clap.workspace = true hive-agent-sock.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-types.workspace = true libc.workspace = true diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index 48230c58..622e659f 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -834,18 +834,18 @@ fn subagent_otel_attrs(name: &str) -> String { /// (`high` on most models) — a deliberate hive policy for subagent work /// specifically, not a reflection of Anthropic's own recommendation, on the /// same "cheaper than you" cost-consciousness the `base:claude-subagents` -/// skill already asks of `model`. `prompt_file`, when -/// given, becomes `--append-system-prompt-file` — the subagent's task -/// instructions. `dir`, when given, becomes `Config::cwd` (e.g. a worktree +/// skill already asks of `model`. `prompt_file`, when given, becomes +/// `--append-system-prompt-file` — the subagent's task instructions. +/// `dir`, when given, becomes `Config::cwd` (e.g. a worktree /// the caller already prepared); `None` inherits this daemon's own working /// directory, same as before this field existed. Always -/// `--dangerously-skip-permissions --strict-mcp-config` — the safety -/// property is `strict_mcp_config: true` with no ambient MCP discovery, not -/// an unconditional absence of `--mcp-config`: a subagent gets exactly the -/// `hyperhive.extraMcpServers` entries an operator has explicitly opted in -/// via `availableToSubagents = true` (`crate::mcp_config::build`), plus this -/// daemon's own two-tool signal surface when `signal_url` is given — nothing -/// implicit and nothing more. +/// `--dangerously-skip-permissions --strict-mcp-config --tools` — two gates +/// covering one half each, neither substituting for the other: +/// `strict_mcp_config` over **MCP** tools (no ambient discovery, so exactly +/// what [`crate::mcp_config::build`] renders), `--tools` over **built-in** +/// ones, via [`hive_sh4re::permissions::builtin_tools_arg`] — the parent +/// agent's own resolved set, never wider. `--tools` does not filter +/// `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 /// subagent reaches them over the same streamable-http listener its parent @@ -865,7 +865,21 @@ fn build_config( dir: Option<&str>, signal_url: Option<&str>, ) -> 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 { extra_args.push("--append-system-prompt-file".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] fn resolve_dir_remembers_an_explicit_dir_and_falls_back_to_it_when_omitted() { let state = State::new(PathBuf::from("/dev/null"), signal_url());