diff --git a/src/openhuman/agent/harness/session/builder.rs b/src/openhuman/agent/harness/session/builder.rs index 6a441accfa..96890d5e50 100644 --- a/src/openhuman/agent/harness/session/builder.rs +++ b/src/openhuman/agent/harness/session/builder.rs @@ -39,7 +39,7 @@ use std::sync::Arc; /// list — initial build, post-composio refresh, scope-filter change — /// so the request the provider sees is always name-unique regardless /// of which path produced it. -pub(super) fn dedup_visible_tool_specs(specs: Vec) -> Vec { +pub(crate) fn dedup_visible_tool_specs(specs: Vec) -> Vec { let mut seen: std::collections::HashSet = std::collections::HashSet::new(); let mut deduped: Vec = Vec::with_capacity(specs.len()); let mut dropped: Vec = Vec::new(); diff --git a/src/openhuman/agent/harness/session/mod.rs b/src/openhuman/agent/harness/session/mod.rs index 7e69c3ac73..16d9ec3c55 100644 --- a/src/openhuman/agent/harness/session/mod.rs +++ b/src/openhuman/agent/harness/session/mod.rs @@ -33,3 +33,8 @@ pub use migration::{migrate_session_layout_if_needed, MigrationOutcome}; mod tests; pub use types::{Agent, AgentBuilder}; + +// Re-export the duplicate-tool-spec guard for sibling harness modules +// (`tool_loop`, `subagent_runner`) so all three provider call sites +// share one tested implementation. +pub(crate) use builder::dedup_visible_tool_specs; diff --git a/src/openhuman/agent/harness/subagent_runner/ops.rs b/src/openhuman/agent/harness/subagent_runner/ops.rs index 7be0d0d72a..defd75260f 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops.rs @@ -824,20 +824,37 @@ async fn run_typed_mode( None }; - let mut filtered_specs: Vec = allowed_indices - .iter() - .map(|&i| parent.all_tool_specs[i].clone()) - .collect(); + // Build provider-visible tool schemas in EXECUTION-PRECEDENCE order: + // `dynamic_tools` (extra_tools at runtime) before parent specs, because + // the inner loop's name lookup (see end of this fn) resolves + // `extra_tools` first and only falls back to `parent_tools`. Aligning + // the dedup order with the runtime lookup order guarantees the schema + // the model sees and the tool that actually executes describe the same + // behaviour. (CodeRabbit review on PR #2446.) + let mut filtered_specs: Vec = dynamic_tools.iter().map(|t| t.spec()).collect(); + filtered_specs.extend( + allowed_indices + .iter() + .map(|&i| parent.all_tool_specs[i].clone()), + ); let mut allowed_names: HashSet = allowed_indices .iter() .map(|&i| parent.all_tools[i].name().to_string()) .collect(); - // Append dynamic tool specs / names so they're discoverable by the - // provider (native tool-calling) and by the inner loop's allowlist. + // 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 { - filtered_specs.push(tool.spec()); allowed_names.insert(tool.name().to_string()); } + // Dedup by name: first occurrence wins. Dynamic Composio action tools + // can share a name with an inherited parent-registry spec when the + // agent's AllowedAll scope includes a same-named skill tool. Some + // providers (Anthropic, OpenHuman cloud after the uniqueness-enforcement + // rollout) 400 on duplicate tool names — see TAURI-RUST-4. Because + // `filtered_specs` is in execution order (dynamic first), the kept + // schema matches what the runtime will actually dispatch. + let filtered_specs = + crate::openhuman::agent::harness::session::dedup_visible_tool_specs(filtered_specs); tracing::debug!( agent_id = %definition.id, diff --git a/src/openhuman/agent/harness/tool_loop.rs b/src/openhuman/agent/harness/tool_loop.rs index 015ad5d118..13f7f68af0 100644 --- a/src/openhuman/agent/harness/tool_loop.rs +++ b/src/openhuman/agent/harness/tool_loop.rs @@ -133,12 +133,20 @@ pub(crate) async fn run_tool_call_loop( } }; - let tool_specs: Vec = tools_registry + // Filter to visible tools, then dedup by name before sending to the + // provider. Registry tools may collide with per-turn synthesised + // extra_tools (e.g. an `ArchetypeDelegationTool` whose + // `delegate_name = "research"` shadowing a same-named skill). Some + // providers (Anthropic, OpenHuman cloud after the uniqueness-enforcement + // rollout) 400 on duplicate tool names — see TAURI-RUST-4. + let filtered_specs: Vec = tools_registry .iter() .chain(extra_tools.iter()) .filter(|tool| is_visible(tool.name())) .map(|tool| tool.spec()) .collect(); + let tool_specs = + crate::openhuman::agent::harness::session::dedup_visible_tool_specs(filtered_specs); let use_native_tools = provider.supports_native_tools() && !tool_specs.is_empty(); log::debug!( diff --git a/src/openhuman/agent/harness/tool_loop_tests.rs b/src/openhuman/agent/harness/tool_loop_tests.rs index 684d4e69da..0324472527 100644 --- a/src/openhuman/agent/harness/tool_loop_tests.rs +++ b/src/openhuman/agent/harness/tool_loop_tests.rs @@ -886,3 +886,128 @@ async fn run_tool_call_loop_applies_per_tool_max_result_size_cap() { tool_results.content.len() ); } + +// ── TAURI-RUST-4 regression guard ──────────────────────────────────── +// +// Some providers (Anthropic, OpenHuman cloud after the uniqueness- +// enforcement rollout) reject chat requests whose `tools` list contains +// two specs with the same `name` — HTTP 400 "Tool names must be unique." +// `run_tool_call_loop` chains the persistent `tools_registry` with the +// per-turn synthesised `extra_tools`; if any name collides across the +// two lists, both would have made it to the provider before the fix. +// +// This test wires a capturing provider, builds a colliding tool list +// (one `EchoTool` in the registry + a second `EchoTool` clone in +// `extra_tools`), and asserts the names the provider sees contain +// `"echo"` exactly once. + +/// Provider that records the tool-spec names of every `chat()` request +/// it sees, then returns the next scripted response. +struct CapturingProvider { + /// One entry per `chat()` call — the tool-name list extracted from + /// `ChatRequest.tools`. `None` if `tools` was `None`. + captured: Mutex>>>, + responses: Mutex>>, + native_tools: bool, +} + +#[async_trait] +impl Provider for CapturingProvider { + async fn chat_with_system( + &self, + _system_prompt: Option<&str>, + _message: &str, + _model: &str, + _temperature: f64, + ) -> Result { + Ok("fallback".into()) + } + + async fn chat( + &self, + request: ChatRequest<'_>, + _model: &str, + _temperature: f64, + ) -> Result { + let names = request + .tools + .map(|specs| specs.iter().map(|s| s.name.clone()).collect::>()); + self.captured.lock().push(names); + let mut guard = self.responses.lock(); + guard.remove(0) + } + + fn capabilities(&self) -> ProviderCapabilities { + ProviderCapabilities { + native_tool_calling: self.native_tools, + vision: false, + ..ProviderCapabilities::default() + } + } +} + +#[tokio::test] +async fn run_tool_call_loop_dedups_duplicate_tool_names_before_provider_call() { + // Provider returns a single final text response — no tool calls — + // so the loop terminates after exactly one `chat()` invocation, + // and the captured tool list reflects what the fix is supposed to + // guard against (no duplicate names reaching the wire). + let provider = CapturingProvider { + captured: Mutex::new(Vec::new()), + responses: Mutex::new(vec![Ok(ChatResponse { + text: Some("done".into()), + tool_calls: vec![], + usage: None, + })]), + // Native tool-calling on: only when the provider supports native + // tools does `run_tool_call_loop` populate `ChatRequest.tools`. + native_tools: true, + }; + + // Registry has `EchoTool` (name = "echo"). `extra_tools` adds a + // second tool also named "echo" — the exact collision pattern from + // the bug report (a synthesised delegation tool whose + // `delegate_name` shadows a same-named skill tool). + let registry: Vec> = vec![Box::new(EchoTool)]; + let extra: Vec> = vec![Box::new(EchoTool)]; + + let mut history = vec![ChatMessage::user("hi")]; + let result = run_tool_call_loop( + &provider, + &mut history, + ®istry, + "test-provider", + "model", + 0.0, + true, + None, + "channel", + &crate::openhuman::config::MultimodalConfig::default(), + 2, + None, + None, + &extra, + None, + None, + ) + .await + .expect("loop should succeed with deduplicated tool list"); + assert_eq!(result, "done"); + + let captured = provider.captured.lock(); + assert_eq!( + captured.len(), + 1, + "exactly one chat() call expected for a final-only response" + ); + let names = captured[0] + .as_ref() + .expect("native_tools=true should populate ChatRequest.tools"); + let echo_count = names.iter().filter(|n| n.as_str() == "echo").count(); + assert_eq!( + echo_count, 1, + "duplicate tool names must be dropped before the provider call \ + (TAURI-RUST-4) — got names={:?}", + names + ); +}