From 9cd30a58ba34cbd2f8b3595e5ed6eb20672414b6 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 15 Sep 2026 17:17:09 +0200 Subject: [PATCH] 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);