Skip to content

Commit a6b19ce

Browse files
fix(agent-harness): dedup visible tool specs in all provider-call paths (tinyhumansai#2446)
1 parent b0facb5 commit a6b19ce

5 files changed

Lines changed: 164 additions & 9 deletions

File tree

src/openhuman/agent/harness/session/builder.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ use std::sync::Arc;
3939
/// list — initial build, post-composio refresh, scope-filter change —
4040
/// so the request the provider sees is always name-unique regardless
4141
/// of which path produced it.
42-
pub(super) fn dedup_visible_tool_specs(specs: Vec<ToolSpec>) -> Vec<ToolSpec> {
42+
pub(crate) fn dedup_visible_tool_specs(specs: Vec<ToolSpec>) -> Vec<ToolSpec> {
4343
let mut seen: std::collections::HashSet<String> = std::collections::HashSet::new();
4444
let mut deduped: Vec<ToolSpec> = Vec::with_capacity(specs.len());
4545
let mut dropped: Vec<String> = Vec::new();

src/openhuman/agent/harness/session/mod.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,3 +33,8 @@ pub use migration::{migrate_session_layout_if_needed, MigrationOutcome};
3333
mod tests;
3434

3535
pub use types::{Agent, AgentBuilder};
36+
37+
// Re-export the duplicate-tool-spec guard for sibling harness modules
38+
// (`tool_loop`, `subagent_runner`) so all three provider call sites
39+
// share one tested implementation.
40+
pub(crate) use builder::dedup_visible_tool_specs;

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

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -862,20 +862,37 @@ async fn run_typed_mode(
862862
None
863863
};
864864

865-
let mut filtered_specs: Vec<ToolSpec> = allowed_indices
866-
.iter()
867-
.map(|&i| parent.all_tool_specs[i].clone())
868-
.collect();
865+
// Build provider-visible tool schemas in EXECUTION-PRECEDENCE order:
866+
// `dynamic_tools` (extra_tools at runtime) before parent specs, because
867+
// the inner loop's name lookup (see end of this fn) resolves
868+
// `extra_tools` first and only falls back to `parent_tools`. Aligning
869+
// the dedup order with the runtime lookup order guarantees the schema
870+
// the model sees and the tool that actually executes describe the same
871+
// behaviour. (CodeRabbit review on PR #2446.)
872+
let mut filtered_specs: Vec<ToolSpec> = dynamic_tools.iter().map(|t| t.spec()).collect();
873+
filtered_specs.extend(
874+
allowed_indices
875+
.iter()
876+
.map(|&i| parent.all_tool_specs[i].clone()),
877+
);
869878
let mut allowed_names: HashSet<String> = allowed_indices
870879
.iter()
871880
.map(|&i| parent.all_tools[i].name().to_string())
872881
.collect();
873-
// Append dynamic tool specs / names so they're discoverable by the
874-
// provider (native tool-calling) and by the inner loop's allowlist.
882+
// Dynamic tool names must also be in the allowlist so the inner loop
883+
// accepts model tool_calls that reference them.
875884
for tool in &dynamic_tools {
876-
filtered_specs.push(tool.spec());
877885
allowed_names.insert(tool.name().to_string());
878886
}
887+
// Dedup by name: first occurrence wins. Dynamic Composio action tools
888+
// can share a name with an inherited parent-registry spec when the
889+
// agent's AllowedAll scope includes a same-named skill tool. Some
890+
// providers (Anthropic, OpenHuman cloud after the uniqueness-enforcement
891+
// rollout) 400 on duplicate tool names — see TAURI-RUST-4. Because
892+
// `filtered_specs` is in execution order (dynamic first), the kept
893+
// schema matches what the runtime will actually dispatch.
894+
let filtered_specs =
895+
crate::openhuman::agent::harness::session::dedup_visible_tool_specs(filtered_specs);
879896

880897
// Dedup by tool name before the specs reach the provider (see
881898
// `dedup_tool_specs_by_name` for why duplicates appear here).

src/openhuman/agent/harness/tool_loop.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,12 +133,20 @@ pub(crate) async fn run_tool_call_loop(
133133
}
134134
};
135135

136-
let tool_specs: Vec<crate::openhuman::tools::ToolSpec> = tools_registry
136+
// Filter to visible tools, then dedup by name before sending to the
137+
// provider. Registry tools may collide with per-turn synthesised
138+
// extra_tools (e.g. an `ArchetypeDelegationTool` whose
139+
// `delegate_name = "research"` shadowing a same-named skill). Some
140+
// providers (Anthropic, OpenHuman cloud after the uniqueness-enforcement
141+
// rollout) 400 on duplicate tool names — see TAURI-RUST-4.
142+
let filtered_specs: Vec<crate::openhuman::tools::ToolSpec> = tools_registry
137143
.iter()
138144
.chain(extra_tools.iter())
139145
.filter(|tool| is_visible(tool.name()))
140146
.map(|tool| tool.spec())
141147
.collect();
148+
let tool_specs =
149+
crate::openhuman::agent::harness::session::dedup_visible_tool_specs(filtered_specs);
142150
let use_native_tools = provider.supports_native_tools() && !tool_specs.is_empty();
143151

144152
log::debug!(

src/openhuman/agent/harness/tool_loop_tests.rs

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -886,3 +886,128 @@ async fn run_tool_call_loop_applies_per_tool_max_result_size_cap() {
886886
tool_results.content.len()
887887
);
888888
}
889+
890+
// ── TAURI-RUST-4 regression guard ────────────────────────────────────
891+
//
892+
// Some providers (Anthropic, OpenHuman cloud after the uniqueness-
893+
// enforcement rollout) reject chat requests whose `tools` list contains
894+
// two specs with the same `name` — HTTP 400 "Tool names must be unique."
895+
// `run_tool_call_loop` chains the persistent `tools_registry` with the
896+
// per-turn synthesised `extra_tools`; if any name collides across the
897+
// two lists, both would have made it to the provider before the fix.
898+
//
899+
// This test wires a capturing provider, builds a colliding tool list
900+
// (one `EchoTool` in the registry + a second `EchoTool` clone in
901+
// `extra_tools`), and asserts the names the provider sees contain
902+
// `"echo"` exactly once.
903+
904+
/// Provider that records the tool-spec names of every `chat()` request
905+
/// it sees, then returns the next scripted response.
906+
struct CapturingProvider {
907+
/// One entry per `chat()` call — the tool-name list extracted from
908+
/// `ChatRequest.tools`. `None` if `tools` was `None`.
909+
captured: Mutex<Vec<Option<Vec<String>>>>,
910+
responses: Mutex<Vec<anyhow::Result<ChatResponse>>>,
911+
native_tools: bool,
912+
}
913+
914+
#[async_trait]
915+
impl Provider for CapturingProvider {
916+
async fn chat_with_system(
917+
&self,
918+
_system_prompt: Option<&str>,
919+
_message: &str,
920+
_model: &str,
921+
_temperature: f64,
922+
) -> Result<String> {
923+
Ok("fallback".into())
924+
}
925+
926+
async fn chat(
927+
&self,
928+
request: ChatRequest<'_>,
929+
_model: &str,
930+
_temperature: f64,
931+
) -> Result<ChatResponse> {
932+
let names = request
933+
.tools
934+
.map(|specs| specs.iter().map(|s| s.name.clone()).collect::<Vec<_>>());
935+
self.captured.lock().push(names);
936+
let mut guard = self.responses.lock();
937+
guard.remove(0)
938+
}
939+
940+
fn capabilities(&self) -> ProviderCapabilities {
941+
ProviderCapabilities {
942+
native_tool_calling: self.native_tools,
943+
vision: false,
944+
..ProviderCapabilities::default()
945+
}
946+
}
947+
}
948+
949+
#[tokio::test]
950+
async fn run_tool_call_loop_dedups_duplicate_tool_names_before_provider_call() {
951+
// Provider returns a single final text response — no tool calls —
952+
// so the loop terminates after exactly one `chat()` invocation,
953+
// and the captured tool list reflects what the fix is supposed to
954+
// guard against (no duplicate names reaching the wire).
955+
let provider = CapturingProvider {
956+
captured: Mutex::new(Vec::new()),
957+
responses: Mutex::new(vec![Ok(ChatResponse {
958+
text: Some("done".into()),
959+
tool_calls: vec![],
960+
usage: None,
961+
})]),
962+
// Native tool-calling on: only when the provider supports native
963+
// tools does `run_tool_call_loop` populate `ChatRequest.tools`.
964+
native_tools: true,
965+
};
966+
967+
// Registry has `EchoTool` (name = "echo"). `extra_tools` adds a
968+
// second tool also named "echo" — the exact collision pattern from
969+
// the bug report (a synthesised delegation tool whose
970+
// `delegate_name` shadows a same-named skill tool).
971+
let registry: Vec<Box<dyn Tool>> = vec![Box::new(EchoTool)];
972+
let extra: Vec<Box<dyn Tool>> = vec![Box::new(EchoTool)];
973+
974+
let mut history = vec![ChatMessage::user("hi")];
975+
let result = run_tool_call_loop(
976+
&provider,
977+
&mut history,
978+
&registry,
979+
"test-provider",
980+
"model",
981+
0.0,
982+
true,
983+
None,
984+
"channel",
985+
&crate::openhuman::config::MultimodalConfig::default(),
986+
2,
987+
None,
988+
None,
989+
&extra,
990+
None,
991+
None,
992+
)
993+
.await
994+
.expect("loop should succeed with deduplicated tool list");
995+
assert_eq!(result, "done");
996+
997+
let captured = provider.captured.lock();
998+
assert_eq!(
999+
captured.len(),
1000+
1,
1001+
"exactly one chat() call expected for a final-only response"
1002+
);
1003+
let names = captured[0]
1004+
.as_ref()
1005+
.expect("native_tools=true should populate ChatRequest.tools");
1006+
let echo_count = names.iter().filter(|n| n.as_str() == "echo").count();
1007+
assert_eq!(
1008+
echo_count, 1,
1009+
"duplicate tool names must be dropped before the provider call \
1010+
(TAURI-RUST-4) — got names={:?}",
1011+
names
1012+
);
1013+
}

0 commit comments

Comments
 (0)