From 8b01dbeef1298500370ba9256e2949ff1410f2e7 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 15 Sep 2026 16:43:06 +0200 Subject: [PATCH 1/4] permissions: give the built-in tool list one home, next to ToolGroup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `--tools` list a harness session gets is not a constant: the base set plus whatever the agent's `HIVE_TOOL_GROUPS` add (today, `web_tools` → `WebFetch`/`WebSearch`). That resolution lived in `hive-agent`'s `mcp_config`, which is fine while the harness is the only thing that spawns a `claude` — and it is not: `hive-subagent-mcp` spawns one too. `hive-agent` is binary-only (no `src/lib.rs`, no lib target), so nothing can depend on it to reach `builtin_tools_arg`. The alternative to a shared home is a second list in the subagent daemon, which diverges on the first tool anyone adds to either — and diverging upward is a subagent holding a built-in its parent does not have. So move the base list, the `HIVE_TOOL_GROUPS` parse and the resolution into `hive_sh4re::permissions`, beside `ToolGroup` — whose `builtin_tools()` was already half of the answer. `hive-agent` re-exports them, so `mcp_config::builtin_tools_arg()` still reads the same at the call site, and `allowed_tools_arg` now derives its built-in half from the same function rather than repeating the merge loop. Behaviour is unchanged. The parse is `strum::EnumString` rather than a `serde_json::from_value` round-trip through a `Value::String`: same `snake_case` names (a test pins the two derives against each other), without `hive-sh4re` needing `serde_json` outside its dev-dependencies. It is now a pure function of its input, so the fallbacks are testable without mutating the environment — which under edition 2024 is `unsafe` and racy across a test binary's threads. Refs #4416 --- Cargo.lock | 1 + docs/turn-loop/mcp.md | 7 +- hive-agent/src/mcp_config.rs | 98 +++------------ hive-sh4re/Cargo.toml | 4 + hive-sh4re/src/permissions.rs | 216 +++++++++++++++++++++++++++++++++- 5 files changed, 240 insertions(+), 86 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 29c3f839..de1729f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1972,6 +1972,7 @@ dependencies = [ "serde", "serde_json", "strum", + "tracing", ] [[package]] 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..4ea8f320 100644 --- a/hive-sh4re/src/permissions.rs +++ b/hive-sh4re/src/permissions.rs @@ -3,16 +3,104 @@ //! 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.** An empty `--tools` value parses +/// as *unset* and yields **more** tools than omitting the flag, so there is +/// no way to spell "no built-in tools" — an empty list is a fail-loudly bug, +/// not a lockdown. [`ALLOWED_BUILTIN_TOOLS`] is non-empty, which is what +/// keeps that from arising. +/// +/// ⚠️ 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 +138,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 +369,93 @@ 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")); + } + + /// An empty `--tools` value parses as *unset* and grants more than + /// omitting the flag, so no resolution may ever produce one. + #[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"); + } + } +} From d6c8cd5a6f73658764a49a49075818177182463e Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 15 Sep 2026 16:46:38 +0200 Subject: [PATCH 2/4] subagent: hand a subagent its parent's built-in tools, and no others MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- Cargo.lock | 1 + docs/tools/subagent.md | 47 ++++++++++-- hive-subagent-mcp/Cargo.toml | 4 ++ hive-subagent-mcp/src/session.rs | 120 ++++++++++++++++++++++++++++--- 4 files changed, 156 insertions(+), 16 deletions(-) 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()); From 4121ccf1f3d975d83e739ef1c279f6c074e2abdb Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 15 Sep 2026 16:58:11 +0200 Subject: [PATCH 3/4] subagent: let the daemon see the tool groups it resolves --tools from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build_config` now resolves a subagent's `--tools` from `HIVE_TOOL_GROUPS`, the same var the harness resolves its own session from — but the meta renderer writes that var onto the `hive-agent` unit alone (`systemd.services.${service}.environment`), and the subagent daemon is a separate unit. It would therefore have resolved the default groups no matter what the agent was actually granted. That direction is safe — the default groups add no built-ins, so the resolution is a subset of the parent's either way, never a superset — but it isn't what the code says it does: an agent granted `web_tools` would spawn subagents silently without `WebFetch`/`WebSearch`, and the "same set as the parent" property would be true only for agents whose groups happen not to matter. Forward the var onto the daemon's unit, read off the harness unit rather than re-derived, so there is one place it is decided. Absent stays absent: `or null`, which systemd drops from the unit, leaving the daemon the same fallback the harness would take. Refs #4416 --- nix/agent-modules/mcp.nix | 10 ++++++++++ 1 file changed, 10 insertions(+) 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. From 9cd30a58ba34cbd2f8b3595e5ed6eb20672414b6 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 15 Sep 2026 17:17:09 +0200 Subject: [PATCH 4/4] subagent: say why we refuse an empty --tools, not what one would do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comments and docs around the empty-`--tools` assert stated a mechanism: that an empty value parses as *unset* and therefore grants MORE built-ins than omitting the flag. That claim came from a measurement, and the installed `claude --help` (2.1.268) says the opposite in as many words — `Use "" to disable all tools`. One of the two is wrong and this repo is not the place that settles it. The rule the code enforces is right under both readings, which is precisely why it should not be argued from either: we never emit an empty `--tools`, because what an empty one means is release-dependent and nothing here wants a subagent with no built-in tools regardless. So the prose now states the rule and the ambiguity, and asserts nothing about `""` in either direction — replacing the claim with its opposite would be the same mistake sourced differently. No behaviour change: the assert, the resolution and every test are untouched. Two test comments also gained the limit they were quietly missing. `no_spawned_tool_escapes_the_session` hardcodes ten names, so it is a canary for tools someone thought to list, not a guarantee — the guarantee is the subset property, which covers tools nobody has invented yet. And `a_subagent_gets_no_builtin_its_parent_lacks` resolves both sides in one process off one env var, so it catches a code divergence but not the two real systemd units disagreeing about `HIVE_TOOL_GROUPS` — which is what they did until the previous commit, and is not a thing a unit test can reach. Refs #4416 --- docs/tools/subagent.md | 13 ++++++--- hive-sh4re/src/permissions.rs | 20 +++++++++----- hive-subagent-mcp/src/session.rs | 45 ++++++++++++++++++++------------ 3 files changed, 50 insertions(+), 28 deletions(-) diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index c2039d1b..05aedc96 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -250,10 +250,15 @@ 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. +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`) diff --git a/hive-sh4re/src/permissions.rs b/hive-sh4re/src/permissions.rs index 4ea8f320..f47de653 100644 --- a/hive-sh4re/src/permissions.rs +++ b/hive-sh4re/src/permissions.rs @@ -69,11 +69,15 @@ pub fn builtin_tools_for(groups: &[ToolGroup]) -> Vec<&'static str> { /// The value for claude's `--tools` flag: which built-in tools exist in this /// session at all. /// -/// ⚠️ **Never emit this as an empty string.** An empty `--tools` value parses -/// as *unset* and yields **more** tools than omitting the flag, so there is -/// no way to spell "no built-in tools" — an empty list is a fail-loudly bug, -/// not a lockdown. [`ALLOWED_BUILTIN_TOOLS`] is non-empty, which is what -/// keeps that from arising. +/// ⚠️ **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 @@ -439,8 +443,10 @@ mod tests { assert!(!without.contains(&"WebFetch") && !without.contains(&"WebSearch")); } - /// An empty `--tools` value parses as *unset* and grants more than - /// omitting the flag, so no resolution may ever produce one. + /// 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()); diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index 622e659f..4759c24e 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -866,14 +866,15 @@ fn build_config( signal_url: Option<&str>, ) -> Config { 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. + // 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 — that parses as unset and would \ - grant the subagent every built-in claude has" + "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(), @@ -1996,14 +1997,13 @@ mod tests { } /// `--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`. + /// 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 [ @@ -2024,6 +2024,12 @@ mod tests { /// `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( @@ -2040,9 +2046,14 @@ mod tests { /// 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. + /// 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);