diff --git a/Cargo.lock b/Cargo.lock index 29c3f839..1e0f2b33 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1972,6 +1972,7 @@ dependencies = [ "serde", "serde_json", "strum", + "tracing", ] [[package]] @@ -1994,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..05aedc96 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -218,13 +218,55 @@ 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 rule worth knowing before editing any of this: **the daemon never +emits an empty `--tools` value**, and asserts rather than doing so. Not +because of what an empty value does — that's exactly the point. Our own +measurement and the installed `claude --help` disagree about whether +`--tools ""` means "no tools" or reads as the flag being absent, and the +answer belongs to whichever claude release is installed rather than to +this repo. Refusing the case is correct under either reading, so the code +never has to know which holds. Nothing here wants a subagent with no +built-in tools anyway. + +### 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/docs/turn-loop/mcp.md b/docs/turn-loop/mcp.md index eb54fc57..c565a322 100644 --- a/docs/turn-loop/mcp.md +++ b/docs/turn-loop/mcp.md @@ -203,7 +203,12 @@ than deriving from SSE events. runs the body, logs the result. Pre-/post-log only — the inbox status hint lives in the wake prompt + UI header, not here. -## Tool allowlist (`mcp_config::ALLOWED_BUILTIN_TOOLS`) +## Tool allowlist (`hive_sh4re::permissions::ALLOWED_BUILTIN_TOOLS`) + +The built-in list and the `--tools` value it resolves to live in +`hive-sh4re` alongside `ToolGroup`, not in the harness, because the +subagent daemon spawns its own `claude` and must resolve the same set — +see [`docs/tools/subagent.md`](../tools/subagent.md). - Allowed built-ins: `Edit`, `Glob`, `Grep`, `Read`, `Skill`, `Write`. `Skill` is what makes an installed plugin's `SKILL.md` invokable — diff --git a/hive-agent/src/mcp_config.rs b/hive-agent/src/mcp_config.rs index e13a4585..0818c7ba 100644 --- a/hive-agent/src/mcp_config.rs +++ b/hive-agent/src/mcp_config.rs @@ -18,27 +18,15 @@ pub const SERVER_NAME: &str = "hyperhive"; /// so `127.0.0.1:` is per-container-private (no cross-agent collision). pub const DEFAULT_MCP_HTTP_PORT: u16 = 8790; -/// Built-in claude tools always present in every session. Anything not -/// in this list (or added by `extra_builtin_tools`) literally doesn't -/// exist in the session. Web egress (`WebFetch`/`WebSearch`) are -/// tool-group-gated (`web_tools`) — off by default. Nested agents -/// (`Task`/`Agent`) are intentionally omitted. `Bash` is disallowed — shell -/// execution goes through `mcp__bash__run` (background tasks -/// with structured output via `hive-bash-mcp`) instead of a raw interactive shell. `TodoWrite` -/// is omitted because the todo list lives in claude's in-process session -/// state and silently evaporates on /compact or session reset — agents -/// should plan in /state notes instead. `Skill` is included so installed -/// plugin skills (`base@hyperhive` and friends) are actually invokable — -/// without it here, every `SKILL.md`'s `description` frontmatter is inert: -/// the model never sees the tool that triggers a skill, no matter how well -/// it matches the task at hand. -pub const ALLOWED_BUILTIN_TOOLS: &[&str] = &["Edit", "Glob", "Grep", "Read", "Skill", "Write"]; - -/// Env var written by the meta renderer with a comma-separated list of -/// `hive_sh4re::permissions::ToolGroup` `snake_case` names (e.g. `"messaging,inbox,meta"`). -/// When present, the harness expands the groups into per-tool allow entries -/// instead of using the hardcoded flavor default. See `docs/process/conventions.md::Tool groups`. -const TOOL_GROUPS_ENV: &str = "HIVE_TOOL_GROUPS"; +/// The built-in tool surface — the base list, the group-gated additions, the +/// `HIVE_TOOL_GROUPS` parse, and the `--tools` value they resolve to — lives +/// 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. +pub use hive_sh4re::permissions::{builtin_tools_arg, effective_tool_groups}; /// `HIVE_CAPABILITIES` env var injected by `meta::render_flake` when the /// operator grants capabilities to this agent. Comma-separated @@ -73,40 +61,6 @@ fn allowed_capability_tools() -> Vec { tools } -/// Resolve the active tool groups for a harness session. -/// -/// Reads `HIVE_TOOL_GROUPS` from the environment first. Each comma-separated -/// token is matched (case-insensitive) against the `ToolGroup` serde names -/// (`messaging`, `meta`, `inbox`, `lifecycle`, `approvals`, `scheduling`, -/// `diagnostics`, `execution`). Unrecognised tokens are logged and skipped. -/// Falls back to `AGENT_DEFAULT` when the env var is absent or empty. -fn effective_tool_groups() -> Vec { - let raw = match std::env::var(TOOL_GROUPS_ENV) { - Ok(v) if !v.trim().is_empty() => v, - _ => return hive_sh4re::permissions::ToolGroup::AGENT_DEFAULT.to_vec(), - }; - let mut groups = Vec::new(); - for token in raw.split(',') { - let t = token.trim().to_ascii_lowercase(); - // Parse via serde_json (the canonical deserialization path). - if let Ok(g) = serde_json::from_value::( - serde_json::Value::String(t.clone()), - ) { - groups.push(g); - } else { - tracing::warn!(token = %t, "{TOOL_GROUPS_ENV}: unknown tool group, skipping"); - } - } - if groups.is_empty() { - tracing::warn!( - "{TOOL_GROUPS_ENV} set but contained no recognised groups; \ - falling back to AGENT_DEFAULT" - ); - return hive_sh4re::permissions::ToolGroup::AGENT_DEFAULT.to_vec(); - } - groups -} - /// Tool group an extra (out-of-process) MCP server is gated behind, if any. /// /// Most `hyperhive.extraMcpServers` entries are ungated — available whenever @@ -200,19 +154,12 @@ pub fn allowed_mcp_tools(groups: &[hive_sh4re::permissions::ToolGroup]) -> Vec String { let groups = effective_tool_groups(); - // Base built-ins always present. - let mut all: Vec = ALLOWED_BUILTIN_TOOLS - .iter() - .map(|s| (*s).to_owned()) + // The same built-ins `--tools` makes exist this session, so a tool that + // exists is never one claude has to prompt about. + let mut all: Vec = hive_sh4re::permissions::builtin_tools_for(&groups) + .into_iter() + .map(ToOwned::to_owned) .collect(); - // Extra built-ins gated by tool groups (e.g. WebFetch/WebSearch via web_tools). - for group in &groups { - for tool in group.builtin_tools() { - if !all.iter().any(|t| t == *tool) { - all.push((*tool).to_owned()); - } - } - } all.extend(allowed_mcp_tools(&groups)); // Capability-gated MCP tools: added to --allowedTools when HIVE_CAPABILITIES // includes the corresponding capability. hive-c0re performs a second @@ -224,23 +171,6 @@ pub fn allowed_tools_arg() -> String { all.join(",") } -/// Built-in tools list for `--tools` (which built-ins exist in this -/// session). Base set plus any group-gated built-ins (e.g. -/// `WebFetch`/`WebSearch` when the `web_tools` group is active). -#[must_use] -pub fn builtin_tools_arg() -> String { - let groups = effective_tool_groups(); - let mut tools: Vec<&str> = ALLOWED_BUILTIN_TOOLS.to_vec(); - for group in &groups { - for t in group.builtin_tools() { - if !tools.contains(t) { - tools.push(t); - } - } - } - tools.join(",") -} - /// Render the MCP config blob claude reads from `--mcp-config `. /// The built-in `hyperhive` surface is an HTTP entry pointing at the /// persistent `hive-mcp-http` daemon (see [`DEFAULT_MCP_HTTP_PORT`]); there diff --git a/hive-sh4re/Cargo.toml b/hive-sh4re/Cargo.toml index f7a8ae73..2efaebd9 100644 --- a/hive-sh4re/Cargo.toml +++ b/hive-sh4re/Cargo.toml @@ -14,6 +14,10 @@ hive-types.workspace = true schemars.workspace = true serde.workspace = true strum.workspace = true +# Facade only, for the one warn in `permissions::ToolGroup::parse_list`: an +# unknown tool-group name is skipped rather than fatal, so the log line is the +# only trace it leaves. +tracing.workspace = true [dev-dependencies] serde_json.workspace = true diff --git a/hive-sh4re/src/permissions.rs b/hive-sh4re/src/permissions.rs index b7f8d8eb..f47de653 100644 --- a/hive-sh4re/src/permissions.rs +++ b/hive-sh4re/src/permissions.rs @@ -3,16 +3,108 @@ //! per-agent config (`tool-groups.json` / `capabilities.json`), injected //! into the container as env vars, and read by the harness to decide //! which MCP tools claude actually sees. +//! +//! It also resolves the *built-in* claude tool list ([`builtin_tools_arg`]): +//! that set is group-gated too (`web_tools`), so it belongs next to the +//! 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. + +use std::str::FromStr; use serde::{Deserialize, Serialize}; +/// Env var the meta renderer writes with a comma-separated list of +/// [`ToolGroup`] `snake_case` names (e.g. `"messaging,inbox,meta"`). Absent +/// or unparseable means [`ToolGroup::AGENT_DEFAULT`]. See +/// `docs/process/conventions.md::Tool groups`. +pub const TOOL_GROUPS_ENV: &str = "HIVE_TOOL_GROUPS"; + +/// Built-in claude tools present in every session regardless of tool +/// groups. Anything not in this list (and not added by a group's +/// [`ToolGroup::builtin_tools`]) literally doesn't exist in the session — +/// this is the `--tools` list, which holds even under +/// `--dangerously-skip-permissions`. +/// +/// Web egress (`WebFetch`/`WebSearch`) is group-gated (`web_tools`) and so +/// lives there, off by default. Nested agents (`Task`/`Agent`) are +/// intentionally omitted. `Bash` is disallowed — shell execution goes +/// through `mcp__bash__run` (background tasks with structured output via +/// `hive-bash-mcp`) instead of a raw interactive shell. `TodoWrite` is +/// omitted because the todo list lives in claude's in-process session state +/// and silently evaporates on /compact or session reset — agents should +/// plan in /state notes instead. `Skill` is included so installed plugin +/// skills (`base@hyperhive` and friends) are actually invokable — without +/// it here, every `SKILL.md`'s `description` frontmatter is inert: the +/// model never sees the tool that triggers a skill, no matter how well it +/// matches the task at hand. +pub const ALLOWED_BUILTIN_TOOLS: &[&str] = &["Edit", "Glob", "Grep", "Read", "Skill", "Write"]; + +/// The tool groups in effect for this process, from [`TOOL_GROUPS_ENV`]. +/// +/// Absent, blank, or naming nothing recognisable all fall back to +/// [`ToolGroup::AGENT_DEFAULT`] — the var is written per agent by the meta +/// renderer, so "unset" is an ordinary state, not an error. +#[must_use] +pub fn effective_tool_groups() -> Vec { + ToolGroup::parse_list(&std::env::var(TOOL_GROUPS_ENV).unwrap_or_default()) +} + +/// The built-in claude tools `groups` resolve to: [`ALLOWED_BUILTIN_TOOLS`] +/// plus each group's own built-ins, de-duplicated, base set first. +#[must_use] +pub fn builtin_tools_for(groups: &[ToolGroup]) -> Vec<&'static str> { + let mut tools: Vec<&'static str> = ALLOWED_BUILTIN_TOOLS.to_vec(); + for group in groups { + for tool in group.builtin_tools() { + if !tools.contains(tool) { + tools.push(tool); + } + } + } + tools +} + +/// The value for claude's `--tools` flag: which built-in tools exist in this +/// session at all. +/// +/// ⚠️ **Never emit this as an empty string** — not because of what an empty +/// value does, but because we don't know. Our own measurement and the +/// installed `claude --help` disagree about whether `--tools ""` means "no +/// tools" or parses as the flag being unset, and the answer is a property of +/// whichever claude release is installed, not of this code. So an empty +/// value is refused rather than relied on: the rule holds under either +/// reading, and no caller has to know which one is true today. +/// [`ALLOWED_BUILTIN_TOOLS`] is non-empty, which keeps the case from +/// arising in the first place. +/// +/// ⚠️ This governs built-ins only. `mcp__*` tools are not filtered by +/// `--tools` at all; they are governed by `--strict-mcp-config` plus +/// whatever the caller renders into `--mcp-config`. +#[must_use] +pub fn builtin_tools_arg() -> String { + 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 /// config) and expands it to the matching tool names for `--allowedTools`. /// When the env var is absent the harness falls back to `AGENT_DEFAULT`. /// See `docs/process/conventions.md::Tool groups`. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, strum::IntoStaticStr)] +#[derive( + Debug, + Clone, + Copy, + PartialEq, + Eq, + Hash, + Serialize, + Deserialize, + strum::IntoStaticStr, + strum::EnumString, +)] #[serde(rename_all = "snake_case")] #[strum(serialize_all = "snake_case")] pub enum ToolGroup { @@ -50,6 +142,42 @@ pub enum ToolGroup { } impl ToolGroup { + /// Parse a comma-separated [`TOOL_GROUPS_ENV`] value. Tokens are trimmed + /// and lowercased; an unrecognised one is warned about and skipped + /// rather than failing the whole list, so one stale name in an agent's + /// config can't take its whole tool surface away. + /// + /// Returns [`AGENT_DEFAULT`](Self::AGENT_DEFAULT) when `raw` is blank or + /// names nothing recognisable — the same fallback either way, because + /// "the operator granted no groups" and "the var is missing" are not + /// distinguishable here and neither should mean "no tools". + #[must_use] + pub fn parse_list(raw: &str) -> Vec { + let mut groups = Vec::new(); + for token in raw.split(',') { + let token = token.trim().to_ascii_lowercase(); + if token.is_empty() { + continue; + } + match Self::from_str(&token) { + Ok(group) => groups.push(group), + Err(_) => { + tracing::warn!(token = %token, "{TOOL_GROUPS_ENV}: unknown tool group, skipping"); + } + } + } + if groups.is_empty() { + if !raw.trim().is_empty() { + tracing::warn!( + "{TOOL_GROUPS_ENV} set but contained no recognised groups; \ + falling back to AGENT_DEFAULT" + ); + } + return Self::AGENT_DEFAULT.to_vec(); + } + groups + } + /// The MCP tool names (without the `mcp__hyperhive__` prefix) in this group. /// Returns `&[]` for `WebTools` — it enables Claude built-in tools, /// not MCP tools; see `builtin_tools()`. @@ -245,3 +373,95 @@ impl Capability { } } } + +#[cfg(test)] +mod tests { + use super::*; + + /// The env-var spellings the meta renderer actually writes must round-trip + /// back to groups. `strum::EnumString` and serde's `rename_all` are two + /// independent derives over the same names; this pins them together, since + /// the renderer writes what serde would produce and the parse is strum's. + #[test] + fn parse_list_accepts_the_snake_case_names_the_renderer_writes() { + assert_eq!( + ToolGroup::parse_list("messaging, Meta ,web_tools"), + vec![ToolGroup::Messaging, ToolGroup::Meta, ToolGroup::WebTools] + ); + for group in ToolGroup::ALL { + let name: &'static str = (*group).into(); + assert_eq!(ToolGroup::parse_list(name), vec![*group], "{name}"); + } + } + + /// Blank, absent and all-garbage are the same state as far as the tool + /// surface is concerned: the default groups, never an empty list. + #[test] + fn parse_list_falls_back_to_the_agent_default_rather_than_nothing() { + for raw in ["", " ", ",,", "not_a_group,also_not"] { + assert_eq!( + ToolGroup::parse_list(raw), + ToolGroup::AGENT_DEFAULT.to_vec(), + "{raw:?}" + ); + } + } + + /// Adding groups only ever adds built-ins — the base set is in every + /// resolution, and no group can remove one. Both `--tools` callers (the + /// harness and the subagent daemon) rely on this to reason about "the + /// parent's set" without re-deriving it. + #[test] + fn builtin_tools_are_the_base_set_plus_whatever_the_groups_add() { + assert_eq!( + builtin_tools_for(&[]), + ALLOWED_BUILTIN_TOOLS.to_vec(), + "no groups resolves to exactly the base set" + ); + assert_eq!( + builtin_tools_for(ToolGroup::AGENT_DEFAULT), + ALLOWED_BUILTIN_TOOLS.to_vec(), + "the default groups add no built-ins" + ); + for group in ToolGroup::ALL { + let resolved = builtin_tools_for(&[*group]); + for base in ALLOWED_BUILTIN_TOOLS { + assert!(resolved.contains(base), "{group:?} dropped {base}"); + } + } + } + + /// `web_tools` is the one group that widens the built-in set, and it is + /// opt-in. A subagent inherits this resolution rather than a list of its + /// own precisely so it cannot get web egress from a parent without the + /// group. + #[test] + fn web_egress_is_present_only_with_the_web_tools_group() { + let with = builtin_tools_for(&[ToolGroup::WebTools]); + assert!(with.contains(&"WebFetch") && with.contains(&"WebSearch")); + let without = builtin_tools_for(ToolGroup::MANAGER_DEFAULT); + assert!(!without.contains(&"WebFetch") && !without.contains(&"WebSearch")); + } + + /// No resolution may produce an empty `--tools` value. What an empty one + /// means is disputed and release-dependent (see [`builtin_tools_arg`]), + /// so this pins the rule that makes the question moot rather than any + /// answer to it. + #[test] + fn no_group_combination_resolves_to_an_empty_tools_arg() { + assert!(!ALLOWED_BUILTIN_TOOLS.is_empty()); + assert!(!builtin_tools_for(&[]).is_empty()); + assert!(!builtin_tools_for(ToolGroup::ALL).is_empty()); + assert!(!builtin_tools_arg().is_empty()); + } + + /// Built-ins are not MCP tools and the two lists must not be mixed: a + /// `mcp__*` name in `--tools` would be silently inert, since `--tools` + /// does not filter the MCP surface at all. + #[test] + fn the_builtin_list_names_no_mcp_tools() { + for tool in builtin_tools_for(ToolGroup::ALL) { + assert!(!tool.contains("__"), "{tool} looks like an MCP tool name"); + } + } +} 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..4759c24e 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,22 @@ 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(); + // 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 + // "a subagent with no built-ins" anyway, so an empty resolution is a bug + // either way, and failing on it is safe under every reading. + assert!( + !tools.is_empty(), + "resolved an empty --tools value — a subagent is never spawned on one, \ + since what an empty value grants is release-dependent" + ); + 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 +1983,100 @@ 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 matters separately from "the flag is there": what an + /// empty value grants is disputed and release-dependent, so we never ship + /// one and never depend on the answer. 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. + /// + /// ⚠️ **Scope: this catches a code divergence, not an environment one.** + /// Both sides resolve in one process off one `HIVE_TOOL_GROUPS`, so it + /// cannot see the two real units disagreeing about that var — which they + /// did until `nix/agent-modules/mcp.nix` forwarded it onto this daemon. + /// 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( + &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. They are absent because the parent has never had + /// them — this pins the consequence, not a second allow-list. + /// + /// ⚠️ **A canary, not a guarantee.** The names are hardcoded, so this + /// only fails for a tool someone thought to list here; the *guarantee* + /// is the subset property above, which holds for tools nobody has + /// imagined yet. Do not read a pass as "no escaping tool is reachable" — + /// read it as "none of these ten came back". + #[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()); diff --git a/nix/agent-modules/mcp.nix b/nix/agent-modules/mcp.nix index f65ba05f..3c96ab36 100644 --- a/nix/agent-modules/mcp.nix +++ b/nix/agent-modules/mcp.nix @@ -378,6 +378,16 @@ in # HIVE_AGENT_SOCKET (agent-service.nix). HIVE_AGENT_SOCKET = "/run/hive-agent/${userName}/agent.sock"; RUST_LOG = "info"; + # This daemon resolves each subagent's `--tools` from the same + # tool groups the harness resolves its own session from, so that a + # subagent's built-in tools are its parent agent's and never wider. + # The meta renderer writes this var onto the harness unit alone, so + # forward it from there rather than re-deriving it: without it the + # daemon falls back to the default groups and an agent granted + # `web_tools` would spawn subagents that quietly lack web access. + # `null` when the agent has no groups declared, which systemd drops + # — the same "absent" the harness itself would see. + HIVE_TOOL_GROUPS = config.systemd.services.hive-agent.environment.HIVE_TOOL_GROUPS or null; # HYPERHIVE_HARNESS_DIR / HYPERHIVE_STATE_DIR: see # `hive-bash-daemon`'s own comment above — same global injection, # same reasoning.