Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/openhuman/agent/harness/session/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ToolSpec>) -> Vec<ToolSpec> {
pub(crate) fn dedup_visible_tool_specs(specs: Vec<ToolSpec>) -> Vec<ToolSpec> {
let mut seen: std::collections::HashSet<String> = std::collections::HashSet::new();
let mut deduped: Vec<ToolSpec> = Vec::with_capacity(specs.len());
let mut dropped: Vec<String> = Vec::new();
Expand Down
5 changes: 5 additions & 0 deletions src/openhuman/agent/harness/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
31 changes: 24 additions & 7 deletions src/openhuman/agent/harness/subagent_runner/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -824,20 +824,37 @@ async fn run_typed_mode(
None
};

let mut filtered_specs: Vec<ToolSpec> = 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<ToolSpec> = 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<String> = 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,
Expand Down
10 changes: 9 additions & 1 deletion src/openhuman/agent/harness/tool_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,20 @@ pub(crate) async fn run_tool_call_loop(
}
};

let tool_specs: Vec<crate::openhuman::tools::ToolSpec> = 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<crate::openhuman::tools::ToolSpec> = 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!(
Expand Down
125 changes: 125 additions & 0 deletions src/openhuman/agent/harness/tool_loop_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<Option<Vec<String>>>>,
responses: Mutex<Vec<anyhow::Result<ChatResponse>>>,
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<String> {
Ok("fallback".into())
}

async fn chat(
&self,
request: ChatRequest<'_>,
_model: &str,
_temperature: f64,
) -> Result<ChatResponse> {
let names = request
.tools
.map(|specs| specs.iter().map(|s| s.name.clone()).collect::<Vec<_>>());
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<Box<dyn Tool>> = vec![Box::new(EchoTool)];
let extra: Vec<Box<dyn Tool>> = vec![Box::new(EchoTool)];

let mut history = vec![ChatMessage::user("hi")];
let result = run_tool_call_loop(
&provider,
&mut history,
&registry,
"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
);
}
Loading