From 2898612962b98395519d46bd9c1d7abd658bcbbe Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Mon, 13 Apr 2026 19:13:15 +0530 Subject: [PATCH] feat(agent): plumb per-agent tool scoping through bus + tool loop (#525, #526) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the parameter plumbing for agent-aware tool filtering without changing any runtime behaviour. Every existing call site continues to pass `None` / empty extras, so the LLM still sees the full unfiltered registry — the actual routing logic that populates these fields lands in commit 4b (dispatch.rs onboarding-flag → target_agent_id). Why two parameters and not one filter: Tools in this codebase are `Box` — owned trait objects with no Clone impl, stored in a shared `Arc>>`. We can't cheaply build a per-turn filtered subset of the global registry, and we can't mutate the Arc to remove entries. Two new parameters work around this without touching the global registry's lifetime model: * `visible_tool_names: Option<&HashSet>` — whitelist filter applied at the iteration site inside `run_tool_call_loop`. When `Some(set)`, only tools whose `name()` is in the set contribute to the function-calling schema and are eligible for execution; every other tool in the combined registry is hidden from the model and rejected if the model emits a call for it. `None` preserves the legacy "everything visible" behaviour. * `extra_tools: &[Box]` — per-turn synthesised tools spliced alongside `tools_registry`. The dispatch path will use this to surface delegation tools (`research`, `delegate_gmail`, …) that are built fresh each turn from the active agent's `subagents` field and the current Composio integration list — tools that don't exist in the global startup-time registry because they depend on per-user runtime state. Empty slice for agents that don't delegate. Inside the loop, `tool_specs` is built from `tools_registry.iter().chain(extra_tools.iter()).filter(is_visible)`, and the tool-execution lookup uses the same chain + filter so the function-calling schema and the execution surface stay in sync. Files touched in this commit: src/openhuman/agent/harness/tool_loop.rs - Add `visible_tool_names` and `extra_tools` parameters to `run_tool_call_loop`. Build `tool_specs` from chained iteration with the visibility filter applied. Replace the `find_tool` call at the execution site with an inline chain+filter lookup so hallucinated calls to filtered-out tools surface as "unknown tool" errors. Drop the now-unused `find_tool` import. - Update the legacy `agent_turn` wrapper to pass `None, &[]`, preserving its existing unfiltered behaviour. - Update all 9 in-file test sites to pass `None, &[]`. src/openhuman/agent/harness/tests.rs - Update all 3 `run_tool_call_loop` test sites to pass `None, &[]`. src/openhuman/agent/bus.rs - Add `target_agent_id: Option`, `visible_tool_names: Option>`, and `extra_tools: Vec>` fields to `AgentTurnRequest`, with rustdoc explaining each. - Destructure the new fields in the `agent.run_turn` handler; thread `visible_tool_names.as_ref()` and `&extra_tools` through to `run_tool_call_loop`. Augment the dispatch trace with target_agent / extra_tool_count / visible_tool_count / filter_active so production logs show whether scoping is active. - Update the in-test `test_request()` helper to populate the new fields with safe defaults. src/openhuman/agent/triage/evaluator.rs - Update the triage `AgentTurnRequest` initializer to set `target_agent_id = Some("trigger_triage")` (for tracing) with `visible_tool_names: None` + `extra_tools: Vec::new()` because the classifier intentionally runs against an empty registry and emits a structured JSON decision rather than calling tools. src/openhuman/channels/runtime/dispatch.rs - Update the channel-message `AgentTurnRequest` initializer to set the three new fields to safe defaults (`None` / `None` / empty vec). Commit 4b will replace these with the real onboarding-flag based routing. src/openhuman/tools/impl/agent/mod.rs - Bug fix: `dispatch_subagent` previously took `_skill_filter: Option<&str>` but discarded the value, hardcoding `SubagentRunOptions::skill_filter_override = None`. That meant `SkillDelegationTool::execute()` synthesising `dispatch_subagent("skills_agent", ..., Some("gmail"))` never actually narrowed `skills_agent`'s tool list — so even with the orchestrator's view scoped, the spawned `skills_agent` subagent would still see the full Composio catalog. Drop the underscore, propagate `skill_filter` into `skill_filter_override`, and add a tracing log line to make this path observable. This is the downstream half of the #526 leak that commit 3's orchestrator- side scoping alone wouldn't have caught. Tests: 8/8 `tool_loop` tests pass, 3/3 harness `tests.rs` cases pass, 323/324 agent module tests pass overall (the one failure is the same pre-existing Windows-path bug in `self_healing::tool_maker_prompt_ includes_command` that fails identically on the upstream baseline). No existing test expectations were changed. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/openhuman/agent/bus.rs | 40 +++++++++ src/openhuman/agent/harness/tests.rs | 6 ++ src/openhuman/agent/harness/tool_loop.rs | 95 ++++++++++++++++++++-- src/openhuman/agent/triage/evaluator.rs | 8 ++ src/openhuman/channels/runtime/dispatch.rs | 7 ++ src/openhuman/tools/impl/agent/mod.rs | 14 +++- 6 files changed, 160 insertions(+), 10 deletions(-) diff --git a/src/openhuman/agent/bus.rs b/src/openhuman/agent/bus.rs index 4c0fa6bf1..cf8a279c4 100644 --- a/src/openhuman/agent/bus.rs +++ b/src/openhuman/agent/bus.rs @@ -13,6 +13,7 @@ //! See [`crate::openhuman::channels::runtime::dispatch`] for the primary //! caller. +use std::collections::HashSet; use std::sync::Arc; use tokio::sync::mpsc; @@ -84,6 +85,33 @@ pub struct AgentTurnRequest { /// chunks here so channel providers can update "draft" messages in /// real time. `None` disables streaming for this turn. pub on_delta: Option>, + + // ── Per-agent scoping (issues #525 / #526) ──────────────────────── + /// Identifier of the agent definition this turn represents (e.g. + /// `"orchestrator"`, `"welcome"`). Used for structured tracing and + /// downstream bookkeeping; the actual filtering is driven by + /// [`Self::visible_tool_names`] and [`Self::extra_tools`] below. + /// `None` preserves the legacy "generic unfiltered turn" behaviour. + pub target_agent_id: Option, + + /// Whitelist of tool names visible to the LLM this turn. When + /// `Some(set)`, the bus handler filters both the function-calling + /// schema and the tool-execution lookup to names in the set. + /// Pre-built on the dispatch side from the target agent's + /// definition (its `[tools] named` list unioned with the names of + /// any per-turn synthesised delegation tools). `None` means no + /// filter — every tool in `tools_registry` plus `extra_tools` is + /// visible. + pub visible_tool_names: Option>, + + /// Per-turn synthesised tools to splice alongside `tools_registry`. + /// The dispatch path uses this to carry `ArchetypeDelegationTool` / + /// `SkillDelegationTool` instances built fresh each turn from the + /// active agent's `subagents` field and the current Composio + /// integrations — tools that don't exist in the global startup + /// registry because they depend on per-user runtime state. + /// Empty vec for agents that don't delegate. + pub extra_tools: Vec>, } /// Final response from an agentic turn. @@ -114,14 +142,21 @@ pub fn register_agent_handlers() { multimodal, max_tool_iterations, on_delta, + target_agent_id, + visible_tool_names, + extra_tools, } = req; tracing::debug!( channel = %channel_name, + target_agent = target_agent_id.as_deref().unwrap_or(""), provider = %provider_name, model = %model, history_len = history.len(), tool_count = tools_registry.len(), + extra_tool_count = extra_tools.len(), + visible_tool_count = visible_tool_names.as_ref().map(|s| s.len()).unwrap_or(0), + filter_active = visible_tool_names.is_some(), streaming = on_delta.is_some(), "[agent::bus] dispatching {AGENT_RUN_TURN_METHOD}" ); @@ -143,6 +178,8 @@ pub fn register_agent_handlers() { &multimodal, max_tool_iterations, on_delta, + visible_tool_names.as_ref(), + &extra_tools, ) .await .map_err(|e| e.to_string())?; @@ -286,6 +323,9 @@ mod tests { multimodal: MultimodalConfig::default(), max_tool_iterations: 1, on_delta: None, + target_agent_id: None, + visible_tool_names: None, + extra_tools: Vec::new(), } } diff --git a/src/openhuman/agent/harness/tests.rs b/src/openhuman/agent/harness/tests.rs index 8aead7732..85cba5128 100644 --- a/src/openhuman/agent/harness/tests.rs +++ b/src/openhuman/agent/harness/tests.rs @@ -124,6 +124,8 @@ async fn run_tool_call_loop_returns_structured_error_for_non_vision_provider() { &crate::openhuman::config::MultimodalConfig::default(), 3, None, + None, + &[], ) .await .expect_err("provider without vision support should fail"); @@ -165,6 +167,8 @@ async fn run_tool_call_loop_rejects_oversized_image_payload() { &multimodal, 3, None, + None, + &[], ) .await .expect_err("oversized payload must fail"); @@ -200,6 +204,8 @@ async fn run_tool_call_loop_accepts_valid_multimodal_request_flow() { &crate::openhuman::config::MultimodalConfig::default(), 3, None, + None, + &[], ) .await .expect("valid multimodal payload should pass"); diff --git a/src/openhuman/agent/harness/tool_loop.rs b/src/openhuman/agent/harness/tool_loop.rs index cc53c7a72..324e55b80 100644 --- a/src/openhuman/agent/harness/tool_loop.rs +++ b/src/openhuman/agent/harness/tool_loop.rs @@ -4,12 +4,13 @@ use crate::openhuman::providers::{ChatMessage, ChatRequest, Provider, ProviderCa use crate::openhuman::tools::traits::ToolScope; use crate::openhuman::tools::Tool; use anyhow::Result; +use std::collections::HashSet; use std::fmt::Write as _; use std::io::Write as _; use super::credentials::scrub_credentials; use super::parse::{ - build_native_assistant_history, find_tool, parse_structured_tool_calls, parse_tool_calls, + build_native_assistant_history, parse_structured_tool_calls, parse_tool_calls, }; use crate::openhuman::context::guard::{ContextCheckResult, ContextGuard}; @@ -23,6 +24,11 @@ pub(crate) const DEFAULT_MAX_TOOL_ITERATIONS: usize = 10; /// Execute a single turn of the agent loop: send messages, parse tool calls, /// execute tools, and loop until the LLM produces a final text response. /// When `silent` is true, suppresses stdout (for channel use). +/// +/// This is a thin wrapper around [`run_tool_call_loop`] with the per-agent +/// filter and extra-tool plumbing disabled — i.e. the LLM sees the entire +/// `tools_registry` unchanged. Used by legacy call sites and harness tests +/// that don't need agent-aware scoping. #[allow(clippy::too_many_arguments)] pub(crate) async fn agent_turn( provider: &dyn Provider, @@ -48,12 +54,40 @@ pub(crate) async fn agent_turn( multimodal_config, max_tool_iterations, None, + None, + &[], ) .await } /// Execute a single turn of the agent loop: send messages, parse tool calls, /// execute tools, and loop until the LLM produces a final text response. +/// +/// # Per-agent tool scoping +/// +/// The last two parameters support per-agent tool filtering without +/// requiring callers to build a filtered copy of the (non-`Clone`able) +/// tool registry: +/// +/// * `visible_tool_names` — optional whitelist of tool names that are +/// allowed to reach the LLM. When `Some(set)`, only tools whose +/// `name()` is present in the set contribute to the function-calling +/// schema and are eligible for execution; every other tool in the +/// registry is hidden from the model and rejected if the model +/// somehow emits a call for it. When `None`, no filtering is applied +/// and every tool in the combined registry is visible (the legacy +/// behaviour used by CLI/REPL and harness tests). +/// +/// * `extra_tools` — per-turn synthesised tools to splice alongside the +/// persistent `tools_registry`. The agent-dispatch path uses this to +/// surface delegation tools (`research`, `delegate_gmail`, …) that +/// are synthesised fresh per turn from the active agent's +/// `subagents` field and the current Composio integration list, and +/// therefore are not registered in the global startup-time registry. +/// +/// The combined tool list seen by the LLM this turn is +/// `tools_registry.iter().chain(extra_tools.iter())`, further narrowed +/// by `visible_tool_names` when supplied. #[allow(clippy::too_many_arguments)] pub(crate) async fn run_tool_call_loop( provider: &dyn Provider, @@ -68,6 +102,8 @@ pub(crate) async fn run_tool_call_loop( multimodal_config: &crate::openhuman::config::MultimodalConfig, max_tool_iterations: usize, on_delta: Option>, + visible_tool_names: Option<&HashSet>, + extra_tools: &[Box], ) -> Result { let max_iterations = if max_tool_iterations == 0 { DEFAULT_MAX_TOOL_ITERATIONS @@ -75,16 +111,34 @@ pub(crate) async fn run_tool_call_loop( max_tool_iterations }; - let tool_specs: Vec = - tools_registry.iter().map(|tool| tool.spec()).collect(); + // Is a given tool name visible to the model this turn? `None` + // means no filter (legacy behaviour = everything visible). + let is_visible = |name: &str| -> bool { + match visible_tool_names { + Some(set) => set.contains(name), + None => true, + } + }; + + let tool_specs: Vec = tools_registry + .iter() + .chain(extra_tools.iter()) + .filter(|tool| is_visible(tool.name())) + .map(|tool| tool.spec()) + .collect(); let use_native_tools = provider.supports_native_tools() && !tool_specs.is_empty(); log::debug!( - "[tool-loop] Registry has {} tool(s): [{}]", + "[tool-loop] Registry has {} tool(s), extra {} tool(s), filter={} — {} visible in schema: [{}]", tools_registry.len(), - tools_registry + extra_tools.len(), + visible_tool_names + .map(|s| format!("whitelist({})", s.len())) + .unwrap_or_else(|| "none".to_string()), + tool_specs.len(), + tool_specs .iter() - .map(|t| t.name()) + .map(|s| s.name.as_str()) .collect::>() .join(", ") ); @@ -289,7 +343,16 @@ pub(crate) async fn run_tool_call_loop( } } - let tool_opt = find_tool(tools_registry, &call.name); + // Look up the tool by name in the combined registry + extras, + // subject to the visibility whitelist. If the model hallucinated + // a filtered-out tool name we treat it as unknown — the error + // path below produces a structured error message the LLM can + // correct in the next iteration. + let tool_opt: Option<&dyn Tool> = tools_registry + .iter() + .chain(extra_tools.iter()) + .find(|t| t.name() == call.name && is_visible(t.name())) + .map(|b| b.as_ref()); tracing::debug!( iteration, tool = call.name.as_str(), @@ -563,6 +626,8 @@ mod tests { &crate::openhuman::config::MultimodalConfig::default(), 1, None, + None, + &[], ) .await .expect_err("vision markers should be rejected"); @@ -597,6 +662,8 @@ mod tests { &crate::openhuman::config::MultimodalConfig::default(), 1, Some(tx), + None, + &[], ) .await .expect("final text should succeed"); @@ -646,6 +713,8 @@ mod tests { &crate::openhuman::config::MultimodalConfig::default(), 2, None, + None, + &[], ) .await .expect("loop should recover after denial"); @@ -698,6 +767,8 @@ mod tests { &crate::openhuman::config::MultimodalConfig::default(), 2, None, + None, + &[], ) .await .expect("native tool flow should succeed"); @@ -753,6 +824,8 @@ mod tests { &crate::openhuman::config::MultimodalConfig::default(), 2, None, + None, + &[], ) .await .expect("non-cli channels should auto-approve supervised tools"); @@ -801,6 +874,8 @@ mod tests { &crate::openhuman::config::MultimodalConfig::default(), 0, None, + None, + &[], ) .await .expect("default iteration fallback should still succeed"); @@ -853,6 +928,8 @@ mod tests { &crate::openhuman::config::MultimodalConfig::default(), 2, None, + None, + &[], ) .await .expect("loop should recover after tool errors"); @@ -889,6 +966,8 @@ mod tests { &crate::openhuman::config::MultimodalConfig::default(), 1, None, + None, + &[], ) .await .expect_err("provider error path should fail"); @@ -918,6 +997,8 @@ mod tests { &crate::openhuman::config::MultimodalConfig::default(), 1, None, + None, + &[], ) .await .expect_err("loop should stop after configured iterations"); diff --git a/src/openhuman/agent/triage/evaluator.rs b/src/openhuman/agent/triage/evaluator.rs index 259cc05ed..d390ff10b 100644 --- a/src/openhuman/agent/triage/evaluator.rs +++ b/src/openhuman/agent/triage/evaluator.rs @@ -229,6 +229,14 @@ pub async fn run_triage_with_resolved( // cap is only a safety net. max_tool_iterations: 1, on_delta: None, + // The triage classifier runs against an empty tools registry + // by design and emits a structured JSON decision rather than + // calling tools — record the agent identity for tracing but + // leave the visible-tool filter unset so the legacy unfiltered + // behaviour is preserved. + target_agent_id: Some("trigger_triage".to_string()), + visible_tool_names: None, + extra_tools: Vec::new(), }; tracing::debug!( provider = %provider_name, diff --git a/src/openhuman/channels/runtime/dispatch.rs b/src/openhuman/channels/runtime/dispatch.rs index 540cfc776..54cceade4 100644 --- a/src/openhuman/channels/runtime/dispatch.rs +++ b/src/openhuman/channels/runtime/dispatch.rs @@ -389,6 +389,13 @@ pub(crate) async fn process_channel_message( multimodal: ctx.multimodal.clone(), max_tool_iterations: ctx.max_tool_iterations, on_delta: delta_tx, + // Per-agent scoping fields are populated in commit 4b (the + // dispatch routing logic for #525). For now, leave them at + // their defaults so this commit ships zero behaviour change — + // every channel turn still sees the full unfiltered registry. + target_agent_id: None, + visible_tool_names: None, + extra_tools: Vec::new(), }; tracing::debug!( channel = %msg.channel, diff --git a/src/openhuman/tools/impl/agent/mod.rs b/src/openhuman/tools/impl/agent/mod.rs index 1f0354d8f..ffea55e18 100644 --- a/src/openhuman/tools/impl/agent/mod.rs +++ b/src/openhuman/tools/impl/agent/mod.rs @@ -15,7 +15,7 @@ pub(crate) async fn dispatch_subagent( agent_id: &str, tool_name: &str, prompt: &str, - _skill_filter: Option<&str>, + skill_filter: Option<&str>, ) -> anyhow::Result { let registry = match AgentDefinitionRegistry::global() { Some(reg) => reg, @@ -51,14 +51,22 @@ pub(crate) async fn dispatch_subagent( }); log::info!( - "[agent] delegating to {} via {} prompt_chars={}", + "[agent] delegating to {} via {} (skill_filter={}) prompt_chars={}", agent_id, tool_name, + skill_filter.unwrap_or(""), prompt.chars().count() ); + // Propagate the per-call skill filter into the subagent runner so + // that `SkillDelegationTool`s can narrow `skills_agent` to a single + // Composio toolkit (e.g. `delegate_gmail` → skills_agent + + // skill_filter="gmail"). Previously this argument was hardcoded to + // `None`, which meant the toolkit pre-selection never reached the + // subagent and skills_agent always saw the full Composio catalog — + // the downstream half of the #526 leak. let options = SubagentRunOptions { - skill_filter_override: None, + skill_filter_override: skill_filter.map(str::to_string), category_filter_override: None, context: None, task_id: Some(task_id.clone()),