From 25096153a7b983656d72c0f4e192222d912e7e7c Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Fri, 3 Jul 2026 22:33:46 +0000 Subject: [PATCH] wip(agent): #4452 partial implementation (agent interrupted by spend limit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Uncommitted implementation salvaged from the parity workflow worktree after the subagent was killed by the monthly spend limit before it could verify, commit, and open a PR. NOT yet build-verified — needs a cargo check pass and test run before merge. Preserved so the work is not lost. Ref: tinyhumansai/openhuman#4452 Claude-Session: https://claude.ai/code/session_019j5TLsRLHsM3kqAYFyH4hR --- src/openhuman/agent/harness/agent_graph.rs | 4 +- src/openhuman/agent/harness/graph.rs | 11 +- .../agent/harness/session/turn/graph.rs | 5 +- .../harness/subagent_runner/ops/graph.rs | 107 +++++++++++- .../harness/subagent_runner/ops/runner.rs | 19 ++- src/openhuman/tinyagents/mod.rs | 79 ++++++++- src/openhuman/tinyagents/tests.rs | 152 +++++++++++++++++- 7 files changed, 350 insertions(+), 27 deletions(-) diff --git a/src/openhuman/agent/harness/agent_graph.rs b/src/openhuman/agent/harness/agent_graph.rs index 98147643dd..5cc633042e 100644 --- a/src/openhuman/agent/harness/agent_graph.rs +++ b/src/openhuman/agent/harness/agent_graph.rs @@ -46,7 +46,9 @@ pub struct AgentTurnRequest { pub parent_tools: Arc>>, pub dynamic_tools: Vec>, pub specs: Vec, - pub allowed_names: HashSet, + /// 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>, pub max_iterations: usize, pub run_queue: Option>, pub on_progress: Option>, diff --git a/src/openhuman/agent/harness/graph.rs b/src/openhuman/agent/harness/graph.rs index 48727e09ed..9ed7c225be 100644 --- a/src/openhuman/agent/harness/graph.rs +++ b/src/openhuman/agent/harness/graph.rs @@ -55,10 +55,13 @@ pub(crate) async fn run_channel_turn_via_graph( ) -> Result { 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> = 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 diff --git a/src/openhuman/agent/harness/session/turn/graph.rs b/src/openhuman/agent/harness/session/turn/graph.rs index fd2e5fde68..8d39bd27f5 100644 --- a/src/openhuman/agent/harness/session/turn/graph.rs +++ b/src/openhuman/agent/harness/session/turn/graph.rs @@ -85,7 +85,10 @@ pub(crate) async fn run_chat_turn_graph(graph: ChatTurnGraph) -> Result>>, dynamic_tools: Vec>, specs: Vec, - allowed_names: HashSet, + // 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>, max_iterations: usize, run_queue: Option>, on_progress: Option>, @@ -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 { + 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 { + Ok(String::new()) + } + async fn chat( + &self, + _r: crate::openhuman::inference::provider::ChatRequest<'_>, + _model: &str, + _t: f64, + ) -> anyhow::Result { + 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>> = Arc::new(vec![Box::new(ExplodingShellTool)]); + // The deny-all allowlist: `Some(empty)` (a `tools = []` scope resolves here). + let allowed: Option> = 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, diff --git a/src/openhuman/agent/harness/subagent_runner/ops/runner.rs b/src/openhuman/agent/harness/subagent_runner/ops/runner.rs index 90ba65e31c..711b89f39c 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops/runner.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops/runner.rs @@ -687,15 +687,28 @@ async fn run_typed_mode( .iter() .map(|&i| parent.all_tool_specs[i].clone()), ); - let mut allowed_names: HashSet = allowed_indices + let mut allowed_name_set: HashSet = 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> = 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); @@ -703,7 +716,7 @@ async fn run_typed_mode( 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" diff --git a/src/openhuman/tinyagents/mod.rs b/src/openhuman/tinyagents/mod.rs index 474bf8d697..71c876ecb5 100644 --- a/src/openhuman/tinyagents/mod.rs +++ b/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, tool_sets: Vec>>>, - allowed: HashSet, + allowed: Option>, max_iterations: usize, on_progress: Option>, subagent_scope: Option, @@ -901,6 +911,22 @@ struct AssembledTurnHarness { prompt_cache_guard: Arc, } +/// 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>>>, - allowed: HashSet, + allowed: Option>, max_iterations: usize, on_progress: Option>, subagent_scope: Option, @@ -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 = HashSet::new(); let candidate_names: Vec = tool_sets .iter() @@ -1122,8 +1159,26 @@ fn assemble_turn_harness( }) .collect(); let mut registered: HashSet = 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); } diff --git a/src/openhuman/tinyagents/tests.rs b/src/openhuman/tinyagents/tests.rs index 3760604556..aaaa8552d1 100644 --- a/src/openhuman/tinyagents/tests.rs +++ b/src/openhuman/tinyagents/tests.rs @@ -165,7 +165,7 @@ async fn streaming_path_forwards_text_deltas_and_cost() { 0.0, history, vec![registry], - std::collections::HashSet::new(), + None, // allowed=None → register all visible tools (top-level turn) 4, Some(tx), None, @@ -267,7 +267,7 @@ async fn pre_queued_steer_message_is_injected_into_the_request() { 0.0, vec![ChatMessage::user("investigate the bug")], vec![registry], - std::collections::HashSet::new(), + None, // allowed=None → register all visible tools (top-level turn) 4, None, None, @@ -364,7 +364,7 @@ async fn concurrent_shared_turns_each_get_a_distinct_result() { 0.0, vec![ChatMessage::user("task one")], vec![registry.clone()], - std::collections::HashSet::new(), + None, // allowed=None → register all visible tools (top-level turn) 4, None, None, @@ -384,7 +384,7 @@ async fn concurrent_shared_turns_each_get_a_distinct_result() { 0.0, vec![ChatMessage::user("task two")], vec![registry], - std::collections::HashSet::new(), + None, // allowed=None → register all visible tools (top-level turn) 4, None, None, @@ -436,7 +436,7 @@ fn adapter_inventory_registers_model_tools_and_middleware() { "mock-model", 0.0, tool_sets, - HashSet::new(), + None, // allowed=None → register all visible tools (top-level turn) 4, None, // on_progress: fire-and-forget None, // subagent_scope: top-level turn @@ -560,7 +560,7 @@ fn adapter_inventory_gates_context_middleware_on_window() { "mock-model", 0.0, tool_sets, - HashSet::new(), + None, // allowed=None → register all visible tools (top-level turn) 4, None, None, @@ -636,7 +636,7 @@ async fn unobserved_turn_reports_aggregate_usage_for_the_cost_fallback() { 0.0, vec![ChatMessage::user("hello")], Vec::new(), - HashSet::new(), + None, // allowed=None → register all visible tools (top-level turn) 3, None, // on_progress: unobserved — no bridge, cost fallback branch runs None, @@ -671,3 +671,141 @@ fn record_unobserved_turn_usage_gates_on_observed_tokens() { assert!(record_unobserved_turn_usage("m", 0, 3, 0, 0.0)); assert!(record_unobserved_turn_usage("m", 10, 3, 2, 0.5)); } + +// ───────────────────────────────────────────────────────────────────────────── +// Registration allowlist predicate (issue #4452) +// ───────────────────────────────────────────────────────────────────────────── + +/// A no-op tool with a caller-chosen name, so tests can synthesise a parent +/// surface (`shell`, `spawn_subagent`, …) without pulling in real tools. +struct NamedNoopTool(&'static str); + +#[async_trait] +impl Tool for NamedNoopTool { + fn name(&self) -> &str { + self.0 + } + fn description(&self) -> &str { + "noop" + } + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({ "type": "object", "properties": {} }) + } + async fn execute(&self, _args: serde_json::Value) -> anyhow::Result { + Ok(ToolResult::success("ok")) + } +} + +/// Assemble a harness over a fixed three-tool parent surface +/// (`echo`, `shell`, `spawn_subagent`) with the given `allowed` filter and +/// optional sub-agent scope, and return the set of registered tool names. +fn registered_tool_names( + allowed: Option>, + subagent: bool, +) -> std::collections::HashSet { + let provider: Arc = Arc::new(EchoThenDone { + calls: AtomicUsize::new(0), + }); + let tool_sets: Vec>>> = vec![Arc::new(vec![ + Box::new(EchoTool) as Box, + Box::new(NamedNoopTool("shell")) as Box, + Box::new(NamedNoopTool("spawn_subagent")) as Box, + ])]; + let subagent_scope = subagent.then(|| SubagentScope { + agent_id: "summarizer".to_string(), + task_id: "t1".to_string(), + extended_policy: false, + }); + let assembled = assemble_turn_harness( + provider, + "mock-model", + 0.0, + tool_sets, + allowed, + 4, + None, + subagent_scope, + None, + &[], + None, + TurnContextMiddleware::defaults(), + None, + None, + false, + ); + assembled.harness.tools().names().into_iter().collect() +} + +/// `None` = "no filter supplied" → every visible tool is registered (top-level +/// chat/channel turn whose visibility set is the full surface). +#[test] +fn allowed_none_registers_all_visible_tools() { + let names = registered_tool_names(None, false); + assert!(names.contains("echo"), "saw {names:?}"); + assert!(names.contains("shell"), "saw {names:?}"); + assert!(names.contains("spawn_subagent"), "saw {names:?}"); +} + +/// `Some(empty)` on a sub-agent turn = deny-all → NO tools registered. This is +/// the #4452 fix: a `tools = []` / zero-match sub-agent must not inherit the +/// parent's shell/spawn surface. +#[test] +fn allowed_some_empty_registers_no_tools() { + let names = registered_tool_names(Some(HashSet::new()), true); + assert!( + names.is_empty(), + "empty allowlist must register zero tools, saw {names:?}" + ); +} + +/// `Some(named)` registers exactly the named tools and nothing else. +#[test] +fn allowed_some_named_registers_only_named() { + let mut allow = HashSet::new(); + allow.insert("echo".to_string()); + let names = registered_tool_names(Some(allow), true); + assert!(names.contains("echo"), "saw {names:?}"); + assert!(!names.contains("shell"), "saw {names:?}"); + assert!(!names.contains("spawn_subagent"), "saw {names:?}"); + assert_eq!(names.len(), 1, "saw {names:?}"); +} + +/// A `Named` list whose entries are absent from the parent surface resolves to +/// zero registrations (fail-closed) rather than falling through to "all". +#[test] +fn allowed_named_but_unresolvable_registers_nothing() { + let mut allow = HashSet::new(); + allow.insert("does_not_exist".to_string()); + let names = registered_tool_names(Some(allow), true); + assert!( + names.is_empty(), + "unresolvable names must register zero tools, saw {names:?}" + ); +} + +/// The never-spawn invariant is re-asserted at registration time: even if the +/// allowlist explicitly names `spawn_subagent`, a sub-agent turn must never +/// register it (backstops the runner's index-level strip). +#[test] +fn subagent_never_registers_spawn_tools_even_if_allowlisted() { + let mut allow = HashSet::new(); + allow.insert("echo".to_string()); + allow.insert("spawn_subagent".to_string()); + let names = registered_tool_names(Some(allow), true); + assert!(names.contains("echo"), "saw {names:?}"); + assert!( + !names.contains("spawn_subagent"), + "sub-agent must never register spawn tools, saw {names:?}" + ); +} + +/// A top-level (non-sub-agent) turn keeps spawn tools — the strip is scoped to +/// sub-agent turns only, so the parent orchestrator can still delegate. +#[test] +fn top_level_turn_keeps_spawn_tools() { + let names = registered_tool_names(None, false); + assert!( + names.contains("spawn_subagent"), + "top-level turn keeps spawn tools, saw {names:?}" + ); +}