From 002dc8d76d45e96b8cbd0e1508b168f0036c5cd8 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Fri, 22 May 2026 15:29:03 +0530 Subject: [PATCH] fix(subagent): dedup tool specs before sending to provider (#2485) Co-authored-by: sanil-23 Co-authored-by: Claude Opus 4.7 (1M context) --- .../agent/harness/subagent_runner/ops.rs | 46 ++++++++++++ .../subagent_runner/ops_dedup_tests.rs | 71 +++++++++++++++++++ 2 files changed, 117 insertions(+) create mode 100644 src/openhuman/agent/harness/subagent_runner/ops_dedup_tests.rs diff --git a/src/openhuman/agent/harness/subagent_runner/ops.rs b/src/openhuman/agent/harness/subagent_runner/ops.rs index 7be0d0d72..2353c699b 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops.rs @@ -326,6 +326,44 @@ pub async fn run_subagent( // Typed mode — narrow prompt, filtered tools, cheaper model // ───────────────────────────────────────────────────────────────────────────── +/// Deduplicate assembled tool specs by name, keeping the first occurrence. +/// +/// The sub-agent's `filtered_specs` is a `Vec` assembled from +/// `parent.all_tool_specs` indices plus dynamic tools, so a delegation tool can +/// shadow a same-named skill/integration tool (common for the wide-set +/// `tools_agent`), leaving two specs with the same name. Strict providers reject +/// such a request with `400 "Tool names must be unique."` The main-agent path +/// dedups via [`session::builder::dedup_visible_tool_specs`]; this separate +/// sub-agent assembly must do the same. +/// +/// First occurrence wins so registration-order semantics are preserved (tool +/// dispatch still resolves by name). Dropped duplicates are logged at `debug` +/// (diagnostic instrumentation, per the repo Rust logging guideline). +/// +/// Extracted as a free function so the regression suite can exercise the dedup +/// without standing up the full `run_typed_mode` plumbing. +fn dedup_tool_specs_by_name(agent_id: &str, specs: Vec) -> Vec { + let mut seen: HashSet = HashSet::with_capacity(specs.len()); + let mut deduped: Vec = Vec::with_capacity(specs.len()); + let mut dropped: Vec = Vec::new(); + for spec in specs { + if seen.insert(spec.name.clone()) { + deduped.push(spec); + } else { + dropped.push(spec.name); + } + } + if !dropped.is_empty() { + tracing::debug!( + agent_id = %agent_id, + "[subagent_runner] dropped {} duplicate tool spec(s) before sending to provider: {:?}", + dropped.len(), + dropped + ); + } + deduped +} + /// Execute a sub-agent in "Typed" mode. /// /// This mode builds a brand-new, minimized system prompt specifically for the @@ -839,6 +877,10 @@ async fn run_typed_mode( allowed_names.insert(tool.name().to_string()); } + // Dedup by tool name before the specs reach the provider (see + // `dedup_tool_specs_by_name` for why duplicates appear here). + let filtered_specs = dedup_tool_specs_by_name(&definition.id, filtered_specs); + tracing::debug!( agent_id = %definition.id, model = %model, @@ -1680,6 +1722,10 @@ pub(crate) fn user_is_signed_in_to_composio(config: &crate::openhuman::config::C #[path = "ops_tests.rs"] mod tests; +#[cfg(test)] +#[path = "ops_dedup_tests.rs"] +mod dedup_tests; + #[cfg(test)] #[path = "ops_truncation_tests.rs"] mod truncation_tests; diff --git a/src/openhuman/agent/harness/subagent_runner/ops_dedup_tests.rs b/src/openhuman/agent/harness/subagent_runner/ops_dedup_tests.rs new file mode 100644 index 000000000..4df9bc147 --- /dev/null +++ b/src/openhuman/agent/harness/subagent_runner/ops_dedup_tests.rs @@ -0,0 +1,71 @@ +//! Focused unit tests for [`super::dedup_tool_specs_by_name`]. +//! +//! Mirrors `session::builder::dedup_visible_tool_specs` coverage: the +//! sub-agent assembly path must drop same-named duplicate tool specs +//! (first occurrence wins) before they reach a strict provider that +//! 400s on `"Tool names must be unique."` + +use super::*; +use serde_json::json; + +fn spec(name: &str) -> ToolSpec { + ToolSpec { + name: name.to_string(), + description: format!("description for {name}"), + parameters: json!({}), + } +} + +#[test] +fn drops_duplicates_first_wins() { + // Real-world collision: a delegation tool (e.g. `tools_agent`) shadows a + // same-named skill/integration tool. Keep the *first* occurrence so + // registration-order semantics hold (dispatch still resolves by name). + let specs = vec![ + spec("research"), // skill + spec("plan"), + spec("research"), // delegate, dropped + spec("run_code"), + spec("plan"), // dropped + ]; + + let deduped = dedup_tool_specs_by_name("test-agent", specs); + + let names: Vec<&str> = deduped.iter().map(|s| s.name.as_str()).collect(); + assert_eq!(names, vec!["research", "plan", "run_code"]); +} + +#[test] +fn passes_through_when_no_duplicates() { + let specs = vec![spec("a"), spec("b"), spec("c")]; + let deduped = dedup_tool_specs_by_name("test-agent", specs); + let names: Vec<&str> = deduped.iter().map(|s| s.name.as_str()).collect(); + assert_eq!(names, vec!["a", "b", "c"]); +} + +#[test] +fn handles_empty_input() { + let deduped = dedup_tool_specs_by_name("test-agent", Vec::::new()); + assert!(deduped.is_empty()); +} + +#[test] +fn preserves_full_spec_content_for_kept_entries() { + // Description + parameters must survive intact — the LLM uses both for + // tool-call decisions, and the kept entry must be the *first* one. + let mut first = spec("alpha"); + first.description = "first alpha — should win".to_string(); + first.parameters = json!({"type": "object", "required": ["x"]}); + + let mut dup = spec("alpha"); + dup.description = "second alpha — should be dropped".to_string(); + + let deduped = dedup_tool_specs_by_name("test-agent", vec![first, dup]); + + assert_eq!(deduped.len(), 1); + assert_eq!(deduped[0].description, "first alpha — should win"); + assert_eq!( + deduped[0].parameters, + json!({"type": "object", "required": ["x"]}) + ); +}