From 5ce0a357b4a527888281b3ac2994d39613e64718 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 15 Sep 2026 18:47:27 +0200 Subject: [PATCH] subagent: grant claude's Bash when the agent holds the execution group A subagent inherited the parent's built-in tool list, which correctly has no `Bash` -- the agent reaches a shell through the `bash` MCP server, not the built-in. Subagents get no such server, so the intersection was empty and they could not run a command at all: no commits, no pushes, no gates. Add `subagent_builtin_tools_for`/`_arg`, which reuse the shared resolver and append `Bash` only when `Execution` -- the group that gates the `bash` MCP server -- is present. Only the subagent spawn path calls them, so the harness's own `--tools`/`--allowedTools` are unchanged. The capability transfers; the mechanism does not. Refs #4422 --- docs/tools/subagent.md | 30 +++++++++---- hive-agent/src/mcp_config.rs | 8 ++-- hive-sh4re/src/permissions.rs | 77 +++++++++++++++++++++++++++++++- hive-subagent-mcp/src/session.rs | 39 ++++++++-------- 4 files changed, 122 insertions(+), 32 deletions(-) diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index 05aedc96..ea0a07ea 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -227,19 +227,31 @@ 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 +**A subagent gets exactly the built-ins its parent agent has, plus one +deliberate exception** — the base resolution +(`hive_sh4re::permissions::subagent_builtin_tools_arg`, wrapping the +harness's own `builtin_tools_for`) 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. +The exception: **a subagent gets Claude's built-in `Bash` iff its parent +holds the `execution` tool group** — the group that already grants the +parent shell execution, via the out-of-process `bash` MCP server +(`mcp__bash__run`/`status`/`kill`). A subagent has no MCP server of its +own by default (see [MCP servers](#mcp-servers---strict-mcp-config) +below), so there is no `mcp__bash__*` for it to inherit that capability +through; it gets Claude's own tool instead. The rule is the capability +transfers, not the mechanism — the parent itself never gains built-in +`Bash` no matter which groups it holds, only a subagent spawned by an +`execution`-holding parent does. + 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. +would hand web egress (or `Bash`) to the subagent of an agent that isn't +allowed it, 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 diff --git a/hive-agent/src/mcp_config.rs b/hive-agent/src/mcp_config.rs index 0818c7ba..faf62fe0 100644 --- a/hive-agent/src/mcp_config.rs +++ b/hive-agent/src/mcp_config.rs @@ -23,9 +23,11 @@ pub const DEFAULT_MCP_HTTP_PORT: u16 = 8790; /// in `hive_sh4re::permissions`, next to `ToolGroup` itself. Re-exported here /// because this module is where the rest of the claude launch config is /// assembled, and because the subagent daemon (`hive-subagent-mcp`) spawns -/// its own `claude` from the same resolution: `hive-agent` is a binary-only -/// crate with no lib target, so a shared home was the only way for both -/// spawners to read one list rather than two that drift. +/// its own `claude` from the same base resolution (plus its own `Bash` +/// exception — see `hive_sh4re::permissions::subagent_builtin_tools_arg`): +/// `hive-agent` is a binary-only crate with no lib target, so a shared home +/// was the only way for both spawners to read one list rather than two that +/// drift. pub use hive_sh4re::permissions::{builtin_tools_arg, effective_tool_groups}; /// `HIVE_CAPABILITIES` env var injected by `meta::render_flake` when the diff --git a/hive-sh4re/src/permissions.rs b/hive-sh4re/src/permissions.rs index f47de653..a8197398 100644 --- a/hive-sh4re/src/permissions.rs +++ b/hive-sh4re/src/permissions.rs @@ -9,7 +9,11 @@ //! groups rather than in any one consumer. Both processes that spawn a //! `claude` — the harness itself and the subagent daemon — call the same //! function, so a subagent can never be handed a built-in its parent does -//! not have. +//! not have — with one deliberate exception: [`subagent_builtin_tools_arg`] +//! additionally grants a **subagent** Claude's own `Bash` when the parent +//! holds [`ToolGroup::Execution`]. The parent never gets built-in `Bash` +//! itself (it has the out-of-process `bash` MCP server instead); the +//! capability transfers to a subagent, not the mechanism. use std::str::FromStr; @@ -87,6 +91,40 @@ pub fn builtin_tools_arg() -> String { builtin_tools_for(&effective_tool_groups()).join(",") } +/// [`builtin_tools_for`] plus Claude's built-in `Bash`, granted iff `groups` +/// contains [`ToolGroup::Execution`] — the **subagent**-only `--tools` +/// resolution. +/// +/// `Execution` already grants shell execution to the parent agent, via the +/// out-of-process `bash` MCP server (`mcp__bash__run`/`status`/`kill` — +/// see the variant's own doc comment). A subagent has no MCP server at all +/// by default (`docs/tools/subagent.md`), so there is no `mcp__bash__*` for +/// it to inherit that capability through; it gets Claude's own `Bash` +/// instead. The capability transfers from the parent, the mechanism it +/// arrives by does not have to match. +/// +/// 🩸 Deliberately **not** folded into [`builtin_tools_for`]/[`ToolGroup::builtin_tools`]: +/// those are shared with the harness's own `--tools` (`hive-agent`'s +/// `mcp_config::allowed_tools_arg`), and the main agent must never gain +/// built-in `Bash` no matter which groups it holds — only a *subagent* +/// spawned by an `Execution`-holding parent does. +#[must_use] +pub fn subagent_builtin_tools_for(groups: &[ToolGroup]) -> Vec<&'static str> { + let mut tools = builtin_tools_for(groups); + if groups.contains(&ToolGroup::Execution) { + tools.push("Bash"); + } + tools +} + +/// The value for a **subagent's** `--tools` flag: [`subagent_builtin_tools_for`] +/// resolved against [`effective_tool_groups`]. See that function for why this +/// differs from [`builtin_tools_arg`]. +#[must_use] +pub fn subagent_builtin_tools_arg() -> String { + subagent_builtin_tools_for(&effective_tool_groups()).join(",") +} + /// Named group of MCP tools an agent may be granted. The harness reads /// `HIVE_TOOL_GROUPS` from the environment (a comma-separated list of /// `snake_case` group names written by the meta renderer from per-agent @@ -464,4 +502,41 @@ mod tests { assert!(!tool.contains("__"), "{tool} looks like an MCP tool name"); } } + + /// A subagent whose parent holds `Execution` gets Claude's own `Bash` — + /// the parent already has shell execution (via the out-of-process + /// `bash` MCP server), and a subagent has no MCP server of its own to + /// inherit that through, so it gets Claude's built-in tool instead. + #[test] + fn subagent_gets_bash_when_parent_has_execution() { + let tools = subagent_builtin_tools_for(&[ToolGroup::Execution]); + assert!(tools.contains(&"Bash")); + } + + /// 🎯 The negative case that matters: without `Execution`, a subagent + /// must not get `Bash` either. Without this, a later refactor that + /// grants it unconditionally would go unnoticed. + #[test] + fn subagent_gets_no_bash_without_execution() { + for groups in [&[][..], &[ToolGroup::Messaging, ToolGroup::WebTools][..]] { + let tools = subagent_builtin_tools_for(groups); + assert!(!tools.contains(&"Bash"), "{groups:?} must not grant Bash"); + } + } + + /// The parent (harness) resolver is untouched: `builtin_tools_for` / + /// `builtin_tools_arg` — what `hive-agent`'s own `--tools` and + /// `--allowedTools` are built from — never produce `Bash`, with or + /// without `Execution`. Only the subagent-specific resolver above adds + /// it; the operator's ruling was explicit that the main agent does not. + #[test] + fn parent_resolver_never_gains_bash() { + for groups in [&[][..], &[ToolGroup::Execution][..], ToolGroup::ALL] { + let tools = builtin_tools_for(groups); + assert!( + !tools.contains(&"Bash"), + "{groups:?} must not grant Bash to the parent" + ); + } + } } diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index 4759c24e..37d48b90 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -836,16 +836,15 @@ fn subagent_otel_attrs(name: &str) -> String { /// 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 -/// 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 --tools` — two gates -/// covering one half each, neither substituting for the other: +/// `dir`, when given, becomes `Config::cwd` (e.g. a worktree the caller +/// already prepared); `None` inherits this daemon's own working directory. +/// Always `--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`). +/// ones, via [`hive_sh4re::permissions::subagent_builtin_tools_arg`], which +/// documents its one `Bash` exception. `--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 +864,7 @@ fn build_config( dir: Option<&str>, signal_url: Option<&str>, ) -> Config { - let tools = hive_sh4re::permissions::builtin_tools_arg(); + let tools = hive_sh4re::permissions::subagent_builtin_tools_arg(); // Refuse to spawn on an empty `--tools` rather than reason about what it // would do: what an empty value means is disputed and depends on the // installed claude release (see `builtin_tools_arg`). Nothing here wants @@ -2017,13 +2016,15 @@ mod tests { } } - /// 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. + /// A subagent's built-ins are a subset of `subagent_builtin_tools_for` — + /// the parent's own resolved set, plus `Bash` iff the parent holds + /// `Execution` (see that function's doc comment). This is the property + /// that makes shipping the daemon safe: a subagent reaching a tool + /// outside that resolution is a privilege escalation dressed as a + /// convenience. Read from `hive_sh4re::permissions`, deliberately: it is + /// the parent harness's own resolver, so this compares against what the + /// parent actually licenses rather than against a restatement of it that + /// could drift. /// /// ⚠️ **Scope: this catches a code divergence, not an environment one.** /// Both sides resolve in one process off one `HIVE_TOOL_GROUPS`, so it @@ -2032,14 +2033,14 @@ mod tests { /// Nothing in a unit test reaches that; it is a deployment property. #[test] fn a_subagent_gets_no_builtin_its_parent_lacks() { - let parent = hive_sh4re::permissions::builtin_tools_for( + let allowed = hive_sh4re::permissions::subagent_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" + allowed.contains(&tool), + "subagent got {tool}, which the parent's resolved set does not license" ); } }