wip(agent): subagent empty tool allowlist fail-open (Option<HashSet> allowlist) (tinyhumansai#4452)

src/openhuman/agent/harness/agent_graph.rs

@@ -46,7 +46,9 @@ pub struct AgentTurnRequest {
     pub parent_tools: Arc<Vec<Box<dyn Tool>>>,
     pub dynamic_tools: Vec<Box<dyn Tool>>,
     pub specs: Vec<ToolSpec>,
-    pub allowed_names: HashSet<String>,
+    /// Fail-closed callable allowlist (issue #4452): `Some(empty)` = deny all,
+    /// `Some(names)` = exactly those tools. Sub-agent turns never pass `None`.
+    pub allowed_names: Option<HashSet<String>>,
     pub max_iterations: usize,
     pub run_queue: Option<Arc<RunQueue>>,
     pub on_progress: Option<Sender<AgentProgress>>,

src/openhuman/agent/harness/graph.rs

@@ -55,10 +55,13 @@ pub(crate) async fn run_channel_turn_via_graph(
 ) -> Result<String> {
     let extra_arc = Arc::new(extra_tools);
 
-    // The callable set is the visibility whitelist (empty = every tool visible
-    // across the registry + per-turn extras). The runner advertises each via its
-    // own `spec()`, deduped by name (extras shadow the registry).
-    let allowed = visible_tool_names.cloned().unwrap_or_default();
+    // The callable set is the visibility whitelist. Top-level channel/CLI turn:
+    // `None` (or an empty set) means "no filter — every tool visible across the
+    // registry + per-turn extras is callable", mapped to `None` for the seam
+    // (issue #4452). A non-empty set is a real whitelist → `Some(..)`. The runner
+    // advertises each callable via its own `spec()`, deduped by name (extras
+    // shadow the registry).
+    let allowed: Option<HashSet<String>> = visible_tool_names.filter(|s| !s.is_empty()).cloned();
 
     // Capture native-tool support before `provider` is moved into the runner: the
     // durable history append below serializes this turn's typed suffix with the

src/openhuman/agent/harness/session/turn/graph.rs

@@ -85,7 +85,10 @@ pub(crate) async fn run_chat_turn_graph(graph: ChatTurnGraph) -> Result<Tinyagen
         graph.temperature,
         graph.messages,
         vec![graph.tools],
-        graph.visible_tool_names,
+        // Top-level chat turn: an empty visibility set means "no filter — every
+        // visible tool is callable", so map it to `None` (issue #4452). A
+        // non-empty set is a real whitelist and passes through as `Some(..)`.
+        Some(graph.visible_tool_names).filter(|s| !s.is_empty()),
         graph.max_iterations,
         // Mirror the harness event stream onto this session's progress sink.
         graph.on_progress,

src/openhuman/agent/harness/subagent_runner/ops/graph.rs

@@ -136,7 +136,11 @@ pub(super) async fn run_subagent_via_graph(
     parent_tools: Arc<Vec<Box<dyn Tool>>>,
     dynamic_tools: Vec<Box<dyn Tool>>,
     specs: Vec<ToolSpec>,
-    allowed_names: HashSet<String>,
+    // Fail-closed callable allowlist (issue #4452). Sub-agent turns always pass
+    // `Some(..)`: `Some(empty)` denies all tools (a `tools = []` / zero-match
+    // agent), `Some(names)` registers exactly those. Never `None` on this path —
+    // `None` is reserved for top-level turns that mean "no filter, all tools".
+    allowed_names: Option<HashSet<String>>,
     max_iterations: usize,
     run_queue: Option<Arc<crate::openhuman::agent::harness::run_queue::RunQueue>>,
     on_progress: Option<tokio::sync::mpsc::Sender<AgentProgress>>,
@@ -668,7 +672,7 @@ mod tests {
             parent_tools,
             vec![],
             vec![],
-            allowed,
+            Some(allowed),
             10,
             None,
             None,
@@ -697,6 +701,99 @@ mod tests {
         assert!(history.iter().any(|m| m.content.contains("echoed:hi")));
     }
 
+    /// A tool that panics if it is ever executed — a canary for "no tool should
+    /// have been registered/called on this turn".
+    struct ExplodingShellTool;
+    #[async_trait]
+    impl Tool for ExplodingShellTool {
+        fn name(&self) -> &str {
+            "shell"
+        }
+        fn description(&self) -> &str {
+            "shell"
+        }
+        fn parameters_schema(&self) -> serde_json::Value {
+            serde_json::json!({"type": "object"})
+        }
+        async fn execute(&self, _args: serde_json::Value) -> anyhow::Result<ToolResult> {
+            panic!("shell tool must never be reachable from a tools=[] sub-agent");
+        }
+    }
+
+    /// A provider that answers in one shot with plain text (no tool calls).
+    struct TextOnlyProvider;
+    #[async_trait]
+    impl Provider for TextOnlyProvider {
+        async fn chat_with_system(
+            &self,
+            _s: Option<&str>,
+            _m: &str,
+            _model: &str,
+            _t: f64,
+        ) -> anyhow::Result<String> {
+            Ok(String::new())
+        }
+        async fn chat(
+            &self,
+            _r: crate::openhuman::inference::provider::ChatRequest<'_>,
+            _model: &str,
+            _t: f64,
+        ) -> anyhow::Result<ChatResponse> {
+            Ok(ChatResponse {
+                text: Some("summary complete".to_string()),
+                ..Default::default()
+            })
+        }
+        fn supports_native_tools(&self) -> bool {
+            true
+        }
+    }
+
+    /// #4452 acceptance: a `tools = []` sub-agent (allowlist `Some(empty)`) with a
+    /// parent surface that includes `shell` must register ZERO tools — it must not
+    /// inherit the parent's shell — yet still complete a text-only turn.
+    #[tokio::test]
+    async fn tools_empty_subagent_has_no_shell_but_completes_text_turn() {
+        let provider = Arc::new(TextOnlyProvider);
+        // Parent surface has a shell tool that panics if reached.
+        let parent_tools: Arc<Vec<Box<dyn Tool>>> = Arc::new(vec![Box::new(ExplodingShellTool)]);
+        // The deny-all allowlist: `Some(empty)` (a `tools = []` scope resolves here).
+        let allowed: Option<HashSet<String>> = Some(HashSet::new());
+        let mut history = vec![ChatMessage::user("summarize this untrusted content")];
+
+        let (output, iterations, _usage, early_exit, hit_cap) = run_subagent_via_graph(
+            provider,
+            "mock-model",
+            0.0,
+            &mut history,
+            parent_tools,
+            vec![],
+            vec![],
+            allowed,
+            10,
+            None,
+            None,
+            "summarizer",
+            "task-1",
+            false,
+            None,
+            std::env::temp_dir(),
+            None,
+            1024,
+            false,
+            "root-session__tools_empty",
+            "mock-channel",
+            None,
+        )
+        .await
+        .expect("tools=[] sub-agent still completes a text-only turn");
+
+        assert_eq!(output, "summary complete");
+        assert_eq!(iterations, 1, "one model call, no tool round");
+        assert!(early_exit.is_none());
+        assert!(!hit_cap);
+    }
+
     /// A provider that streams visible text + reasoning through the request's
     /// delta sender, exercising the child-progress bridge end to end.
     struct ThinkingStreamProvider;
@@ -756,7 +853,7 @@ mod tests {
             parent_tools,
             vec![],
             vec![],
-            HashSet::new(),
+            Some(HashSet::new()),
             4,
             None,
             Some(tx),
@@ -902,7 +999,7 @@ mod tests {
             parent_tools,
             vec![],
             vec![],
-            allowed,
+            Some(allowed),
             10,
             None,
             None,
@@ -1008,7 +1105,7 @@ mod tests {
             parent_tools,
             vec![],
             vec![],
-            allowed,
+            Some(allowed),
             2,
             None,
             None,

src/openhuman/agent/harness/subagent_runner/ops/runner.rs

@@ -687,23 +687,36 @@ async fn run_typed_mode(
             .iter()
             .map(|&i| parent.all_tool_specs[i].clone()),
     );
-    let mut allowed_names: HashSet<String> = allowed_indices
+    let mut allowed_name_set: HashSet<String> = allowed_indices
         .iter()
         .map(|&i| parent.all_tools[i].name().to_string())
         .collect();
     // Dynamic tool names must also be in the allowlist so the inner loop
     // accepts model tool_calls that reference them.
     for tool in &dynamic_tools {
-        allowed_names.insert(tool.name().to_string());
+        allowed_name_set.insert(tool.name().to_string());
     }
+    // Fail-closed allowlist (issue #4452): a sub-agent's callable set is ALWAYS
+    // `Some(..)`. An empty set (a `tools = []` scope, a zero-match `skill_filter`,
+    // or a `Named` list whose entries are absent from the parent surface) means
+    // deny-all — it must NOT fall through the seam's `None` "no filter → all
+    // tools" branch and inherit the parent's shell / file-write / spawn surface.
+    if allowed_name_set.is_empty() {
+        tracing::warn!(
+            agent_id = %definition.id,
+            "[subagent] tool allowlist resolved empty — registering no tools"
+        );
+    }
+    let allowed_names: Option<HashSet<String>> = Some(allowed_name_set);
+    let allowed_tool_count = allowed_names.as_ref().map_or(0, |s| s.len());
     let filtered_specs =
         crate::openhuman::agent::harness::session::dedup_visible_tool_specs(filtered_specs);
     let filtered_specs = dedup_tool_specs_by_name(&definition.id, filtered_specs);
 
     tracing::debug!(
         agent_id = %definition.id,
         model = %model,
-        tool_count = allowed_names.len(),
+        tool_count = allowed_tool_count,
         max_iterations = definition.effective_max_iterations(),
         iteration_policy = ?definition.iteration_policy,
         "[subagent_runner:typed] resolved configuration"

src/openhuman/tinyagents/mod.rs

@@ -353,8 +353,18 @@ pub(crate) async fn run_turn_via_tinyagents(
 /// advertised spec so the same `Arc`-shared tools the legacy loop runs are
 /// reused without cloning.
 ///
-/// `allowed` is the callable tool-name whitelist (empty = every tool visible in
-/// `tool_sets`); each callable tool is advertised via its own `spec()`.
+/// `allowed` is the callable tool-name whitelist, fail-closed on `Some`:
+/// * `None` — no filter supplied → register every tool visible in `tool_sets`
+///   (top-level chat/channel turns whose visibility set is the full surface).
+/// * `Some(set)` — register only the named tools; **`Some(empty)` registers
+///   NONE** (deny-all). This distinction is the fix for #4452: a sub-agent with
+///   `tools = []` (or a zero-match `skill_filter`) must NOT silently inherit the
+///   parent's full tool surface (shell / file-write / spawn). Sub-agent turns
+///   therefore always pass `Some(..)`; only top-level turns pass `None`.
+///
+/// Regardless of `allowed`, when `subagent_scope` is `Some` the spawn/delegate
+/// meta-tools are stripped at registration time — the "sub-agents never spawn"
+/// invariant is re-asserted here, not just upstream in the runner's index filter.
 ///
 /// When `on_progress` is `Some`, the run streams (`invoke_streaming_in_context`)
 /// and a [`OpenhumanEventBridge`] mirrors the harness event stream onto
@@ -377,7 +387,7 @@ pub(crate) async fn run_turn_via_tinyagents_shared(
     temperature: f64,
     history: Vec<ChatMessage>,
     tool_sets: Vec<Arc<Vec<Box<dyn crate::openhuman::tools::Tool>>>>,
-    allowed: HashSet<String>,
+    allowed: Option<HashSet<String>>,
     max_iterations: usize,
     on_progress: Option<Sender<AgentProgress>>,
     subagent_scope: Option<SubagentScope>,
@@ -901,6 +911,22 @@ struct AssembledTurnHarness {
     prompt_cache_guard: Arc<PromptCacheGuardMiddleware>,
 }
 
+/// Spawn/delegate meta-tools that a sub-agent turn must never be able to
+/// register, regardless of its resolved allowlist (issue #4452, defense in
+/// depth). Kept in lockstep with the canonical
+/// [`crate::openhuman::agent::harness::subagent_runner`] index-level strip
+/// (`is_subagent_spawn_tool` + the explicit `spawn_worker_thread` retain); this
+/// is the registration-time backstop for the shared seam, which also runs for
+/// custom-graph sub-agents that bypass that filter. If the delegation-tool
+/// naming scheme changes, update both together.
+fn is_subagent_never_register_tool(name: &str) -> bool {
+    name == "spawn_subagent"
+        || name == "spawn_worker_thread"
+        || name == "use_tinyplace"
+        || name == "agent_prepare_context"
+        || name.starts_with("delegate_")
+}
+
 /// Assemble the turn harness for [`run_turn_via_tinyagents_shared`]: register
 /// the provider model, every shared tool, and the full middleware stack in the
 /// intended order. Split out of the runner so the adapter inventory is directly
@@ -912,7 +938,7 @@ fn assemble_turn_harness(
     model: &str,
     temperature: f64,
     tool_sets: Vec<Arc<Vec<Box<dyn crate::openhuman::tools::Tool>>>>,
-    allowed: HashSet<String>,
+    allowed: Option<HashSet<String>>,
     max_iterations: usize,
     on_progress: Option<Sender<AgentProgress>>,
     subagent_scope: Option<SubagentScope>,
@@ -1109,7 +1135,18 @@ fn assemble_turn_harness(
         .map(|h| EarlyExitHook::new(h.clone()));
 
     // Register one adapter per unique callable tool name found across the shared
-    // sets (newest set wins on a name clash; `allowed` empty = all visible).
+    // sets (newest set wins on a name clash). The allowlist is fail-closed on
+    // `Some` (issue #4452):
+    // * `allowed == None` → no filter supplied → every visible tool is callable
+    //   (top-level chat/channel turn whose visibility set is the full surface);
+    // * `allowed == Some(set)` → only names in `set` are callable — and
+    //   `Some(empty)` therefore registers NOTHING (deny-all). A sub-agent with
+    //   `tools = []` / a zero-match `skill_filter` lands here and must NOT
+    //   inherit the parent's shell / file-write / spawn tools.
+    let is_subagent_turn = subagent_scope.is_some();
+    if is_subagent_turn && allowed.as_ref().is_some_and(|a| a.is_empty()) {
+        tracing::warn!("[subagent] tool allowlist resolved empty — registering no tools");
+    }
     let mut seen_candidates: HashSet<String> = HashSet::new();
     let candidate_names: Vec<String> = tool_sets
         .iter()
@@ -1122,8 +1159,26 @@ fn assemble_turn_harness(
         })
         .collect();
     let mut registered: HashSet<String> = HashSet::new();
+    let mut spawn_stripped_at_registration = 0usize;
     for name in candidate_names.iter().map(String::as_str) {
-        if !registered.contains(name) && (allowed.is_empty() || allowed.contains(name)) {
+        let allowed_ok = match &allowed {
+            None => true,
+            Some(set) => set.contains(name),
+        };
+        // Re-assert the never-spawn invariant at registration time (#4452): a
+        // sub-agent turn must never register a spawn/delegate meta-tool no matter
+        // what the allowlist contains, backstopping the runner's index filter. We
+        // only count (and warn about) a strip when the allowlist would otherwise
+        // have let the tool through — so the diagnostic means "an allowlist tried
+        // to register a spawn tool onto a sub-agent", not just "a spawn tool was
+        // visible in the parent set".
+        if is_subagent_turn && is_subagent_never_register_tool(name) {
+            if allowed_ok {
+                spawn_stripped_at_registration += 1;
+            }
+            continue;
+        }
+        if !registered.contains(name) && allowed_ok {
             if let Some(mut adapter) = SharedToolAdapter::for_name(tool_sets.clone(), name) {
                 if early_exit_set.contains(name) {
                     if let Some(hook) = &early_exit_hook {
@@ -1137,7 +1192,19 @@ fn assemble_turn_harness(
             }
         }
     }
+    if spawn_stripped_at_registration > 0 {
+        tracing::warn!(
+            stripped = spawn_stripped_at_registration,
+            "[subagent] stripped spawn/delegate tools at registration (never-spawn invariant)"
+        );
+    }
     let tool_count = registered.len();
+    tracing::debug!(
+        subagent = is_subagent_turn,
+        allowlist = ?allowed.as_ref().map(|a| a.len()),
+        registered = tool_count,
+        "[tinyagents] tool registration resolved"
+    );
     for report in all_graph_topologies() {
         let _ = capability_registry.register_descriptor(ComponentKind::Graph, report.name);
     }