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);