diff --git a/CHANGELOG.md b/CHANGELOG.md index e9cd1d4e..bf8ef753 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to `yoagent` are documented here. The format loosely follows [Keep a Changelog](https://keepachangelog.com/), and the project adheres to [Semantic Versioning](https://semver.org/). +## Unreleased + +### yoagent + +- **`McpToolAdapter::with_prefix` / `from_client_with_prefix` now name the tool** `__` (as `OpenApiConfig::with_name_prefix` does for OpenAPI tools). The prefix was stored but never used, so two servers offering a tool of the same name, or a server tool named like a built-in, still gave the agent duplicate tool names, which a provider rejects. The server is still called with its own tool name. The MCP guide now shows how (the `with_mcp_server_*` helpers add no prefix). Reported by the yobuild session. + ## 0.25.6 (2026-10-16) - **DeepSeek is priced** (after `prices::enable_bundled()`): `deepseek-flash`, `deepseek-v4-pro` and the legacy `deepseek-v4-flash` (billed as Flash), from DeepSeek's pricing page. **At the peak rate:** DeepSeek bills half of it off-peak (all hours but 01:00–04:00 and 06:00–10:00 UTC on weekdays), and a `CostConfig` has no time of day, so costs are an upper bound — off-peak they read up to twice the bill, and a `Budget` stops early rather than late. Install the off-peak rates as an override to count those instead (`docs/concepts/pricing.md`). `ModelConfig::deepseek(..)` for those ids now has a `cost`; `Budget::for_model` works for them. The price audit checks the rates as exactly twice models.dev's off-peak ones, and models.dev data (`enable_live`, a fetch or a price cache written by an earlier version) never brings DeepSeek's off-peak rates back over the bundled peak ones. `examples/llm_compaction_live.rs` reads DeepSeek's rates from the snapshot instead of its own literals. diff --git a/docs/concepts/tools.md b/docs/concepts/tools.md index e9b781e2..78b0bfb0 100644 --- a/docs/concepts/tools.md +++ b/docs/concepts/tools.md @@ -514,7 +514,10 @@ The contract: one survives: the agent's own tools (including the injected `shared_state` tool) win, then earlier sources in installation order, then the earlier tool within one source. Each dropped duplicate is logged with - `tracing::warn!`; the run is never refused. + `tracing::warn!`; the run is never refused. (The agent's own tools — + `with_tools`, MCP adapters included — are not de-duplicated: give clashing + MCP or OpenAPI tools a prefix with `McpToolAdapter::with_prefix` or + `OpenApiConfig::with_name_prefix`.) - **Failures.** A source that panics — while building its future or while it runs — is contained, logged with the panic message, and contributes no tools to that run. diff --git a/docs/guides/mcp.md b/docs/guides/mcp.md index ba1a2a96..7ffceeb0 100644 --- a/docs/guides/mcp.md +++ b/docs/guides/mcp.md @@ -170,6 +170,24 @@ When you call `with_mcp_server_stdio()` or `with_mcp_server_http()`, yoagent: MCP tools appear alongside built-in tools. The LLM sees them with their original names, descriptions, and JSON Schema parameters — it can call them just like any other tool. +**Two servers with the same tool name.** The agent's tools are not de-duplicated, and a provider rejects two tools with one name — so two servers that both offer `search`, or a server tool named like a built-in (`bash`, `read_file`), break the request. Give such a server's tools a prefix by wrapping them yourself (the `with_mcp_server_*` helpers add none): + +```rust +use std::sync::Arc; +use tokio::sync::Mutex; +use yoagent::mcp::{McpClient, McpToolAdapter}; +use yoagent::AgentTool; + +let docs = Arc::new(Mutex::new(McpClient::connect_stdio("docs-server", &[], None).await?)); +let mut tools = yoagent::tools::default_tools(); +for tool in McpToolAdapter::from_client_with_prefix(docs, "docs").await? { + // The model sees `docs__search`; the server is still called with `search`. + tools.push(Box::new(tool) as Box); +} +// `with_tools` replaces the agent's list: pass every tool in one call. +let agent = agent.with_tools(tools); +``` + Each call ends when the run is cancelled. Over **stdio** it is also bounded at five minutes (`McpToolAdapter::DEFAULT_CALL_TIMEOUT`), counted from when the call is sent, not while it waits behind another call to the same server. Over @@ -213,6 +231,8 @@ let agent = Agent::from_config(ModelConfig::anthropic("claude-sonnet-5", "Claude // Agent now has both built-in coding tools AND MCP database tools ``` +If a server's tools are named like the built-ins (`bash`, `read_file`, `search`, …), prefix them (see above). + ## Using the MCP Client Directly For lower-level control, use `McpClient` directly: diff --git a/src/mcp/tool_adapter.rs b/src/mcp/tool_adapter.rs index a33b49f2..df394699 100644 --- a/src/mcp/tool_adapter.rs +++ b/src/mcp/tool_adapter.rs @@ -11,8 +11,10 @@ use tokio::sync::Mutex; pub struct McpToolAdapter { client: Arc>, tool: McpToolInfo, - /// Prefix to avoid name collisions (e.g., "server_name__tool_name"). - prefix: Option, + /// The name the agent and the model see: the server's tool name, or + /// `__` with [`with_prefix`](Self::with_prefix). Calls to + /// the server always use the server's own name. + name: String, call_timeout: Option, } @@ -28,8 +30,8 @@ impl McpToolAdapter { pub fn new(client: Arc>, tool: McpToolInfo) -> Self { Self { client, + name: tool.name.clone(), tool, - prefix: None, call_timeout: None, } } @@ -42,9 +44,16 @@ impl McpToolAdapter { self } - /// Create with a name prefix for disambiguation. + /// Name the tool `__` for the agent and the model (as + /// `OpenApiConfig::with_name_prefix` does for OpenAPI tools), so two + /// servers offering a tool of the same name, or a server tool named like + /// a built-in, do not collide. The server is still called with its own + /// tool name. A second call replaces the prefix. Provider limits on tool + /// names apply to the result: keep the prefix to letters, digits, `_` and + /// `-`, and short (several providers cap names at 64 characters); neither + /// the prefix nor the server's name is checked here. pub fn with_prefix(mut self, prefix: impl Into) -> Self { - self.prefix = Some(prefix.into()); + self.name = format!("{}__{}", prefix.into(), self.tool.name); self } @@ -63,7 +72,8 @@ impl McpToolAdapter { .collect()) } - /// Create adapters with a name prefix for all tools from an MCP client. + /// Create adapters for all tools from an MCP client, each named + /// `__` ([`with_prefix`](Self::with_prefix)). pub async fn from_client_with_prefix( client: Arc>, prefix: impl Into, @@ -88,13 +98,13 @@ impl McpToolAdapter { #[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] impl AgentTool for McpToolAdapter { fn name(&self) -> &str { - // Return the tool name; prefix is applied in label for display. - // We use the raw name so MCP server recognizes it in call_tool. - &self.tool.name + // Prefixed when a prefix was given; `execute` calls the server with + // the server's own name (`self.tool.name`). + &self.name } fn label(&self) -> &str { - &self.tool.name + &self.name } fn description(&self) -> &str { @@ -336,4 +346,99 @@ mod tests { assert_eq!(adapters[0].name(), "tool_a"); assert_eq!(adapters[1].name(), "tool_b"); } + + /// Records each request and answers from a queue. + struct RecordingTransport { + sent: Arc>>, + responses: std::sync::Mutex>, + } + + #[async_trait] + impl McpTransport for RecordingTransport { + async fn send(&self, request: JsonRpcRequest) -> Result { + self.sent.lock().unwrap().push(request); + let mut responses = self.responses.lock().unwrap(); + if responses.is_empty() { + Err(McpError::ConnectionClosed) + } else { + Ok(responses.remove(0)) + } + } + + async fn close(&self) -> Result<(), McpError> { + Ok(()) + } + } + + #[tokio::test] + async fn a_prefix_names_the_tool_and_the_server_still_gets_its_own_name() { + // Two servers offering a tool of the same name. + let mut adapters = Vec::new(); + let mut sent = Vec::new(); + for server in ["docs", "web"] { + let log = Arc::new(std::sync::Mutex::new(Vec::new())); + let transport = RecordingTransport { + sent: log.clone(), + responses: std::sync::Mutex::new(vec![ + ok_response( + 1, + serde_json::json!({ "tools": [{ "name": "search", "inputSchema": {"type": "object"} }] }), + ), + ok_response( + 2, + serde_json::json!({ "content": [{"type": "text", "text": server}], "isError": false }), + ), + ]), + }; + let client = McpClient::from_transport(Box::new(transport)) + .with_call_timeout(Some(Duration::from_secs(7))); + adapters.extend( + McpToolAdapter::from_client_with_prefix(Arc::new(Mutex::new(client)), server) + .await + .unwrap(), + ); + sent.push(log); + } + let names: Vec<&str> = adapters.iter().map(|a| a.name()).collect(); + assert_eq!(names, ["docs__search", "web__search"]); + // The client's call timeout is carried over, prefix or not. + assert!(adapters + .iter() + .all(|a| a.call_timeout == Some(Duration::from_secs(7)))); + assert_eq!(adapters[1].label(), "web__search"); + + let ctx = ToolContext::new("tc-1", "web__search"); + let result = adapters[1] + .execute(serde_json::json!({"q": "x"}), ctx) + .await + .unwrap(); + assert!(matches!(&result.content[0], Content::Text { text } if text == "web")); + // The server was called with its own tool name, not the prefixed one. + let calls = sent[1].lock().unwrap(); + let call = calls + .iter() + .find(|r| r.method == "tools/call") + .expect("a tools/call"); + assert_eq!(call.params.as_ref().unwrap()["name"], "search"); + // ...and only the server it belongs to. + assert!(!sent[0] + .lock() + .unwrap() + .iter() + .any(|r| r.method == "tools/call")); + + // Without a prefix, the server's name. + let tool = McpToolInfo { + name: "search".into(), + description: None, + input_schema: serde_json::Value::Null, + }; + let client = Arc::new(Mutex::new(McpClient::from_transport(Box::new( + MockTransport::new(vec![]), + )))); + let plain = McpToolAdapter::new(client, tool); + assert_eq!(plain.name(), "search"); + // A second prefix replaces the first; prefixes never stack. + assert_eq!(plain.with_prefix("a").with_prefix("b").name(), "b__search"); + } } diff --git a/tests/mcp_prefix_agent_test.rs b/tests/mcp_prefix_agent_test.rs new file mode 100644 index 00000000..45c2f108 --- /dev/null +++ b/tests/mcp_prefix_agent_test.rs @@ -0,0 +1,112 @@ +//! Two MCP servers offering a tool of the same name, prefixed, in one agent: +//! the model sees two distinct names, and a call reaches only its server, +//! under the server's own tool name. + +use std::sync::{Arc, Mutex as StdMutex}; +use tokio::sync::Mutex; +use yoagent::mcp::types::{JsonRpcRequest, JsonRpcResponse}; +use yoagent::mcp::{McpClient, McpError, McpToolAdapter, McpTransport}; +use yoagent::provider::mock::{MockResponse, MockToolCall}; +use yoagent::provider::{MockProvider, ModelConfig}; +use yoagent::*; + +/// Records each request; answers `tools/list` with one `search` tool and +/// `tools/call` with this server's name. +struct Server { + name: &'static str, + sent: Arc>>, +} + +#[async_trait::async_trait] +impl McpTransport for Server { + async fn send(&self, request: JsonRpcRequest) -> Result { + let result = match request.method.as_str() { + "tools/list" => serde_json::json!({ + "tools": [{ "name": "search", "inputSchema": { "type": "object" } }] + }), + "tools/call" => serde_json::json!({ + "content": [{ "type": "text", "text": self.name }], "isError": false + }), + other => panic!("unexpected {other}"), + }; + let id = request.id; + self.sent.lock().unwrap().push(request); + Ok(JsonRpcResponse { + jsonrpc: "2.0".into(), + id: Some(id), + result: Some(result), + error: None, + }) + } + + async fn close(&self) -> Result<(), McpError> { + Ok(()) + } +} + +#[tokio::test] +async fn two_prefixed_servers_in_one_agent() { + let mut tools: Vec> = Vec::new(); + let mut logs = Vec::new(); + for name in ["docs", "web"] { + let sent = Arc::new(StdMutex::new(Vec::new())); + let client = McpClient::from_transport(Box::new(Server { + name, + sent: sent.clone(), + })); + for tool in McpToolAdapter::from_client_with_prefix(Arc::new(Mutex::new(client)), name) + .await + .unwrap() + { + tools.push(Box::new(tool)); + } + logs.push(sent); + } + let names: Vec<&str> = tools.iter().map(|t| t.name()).collect(); + assert_eq!(names, ["docs__search", "web__search"]); + + let provider = MockProvider::new(vec![ + MockResponse::ToolCalls(vec![MockToolCall { + name: "web__search".into(), + arguments: serde_json::json!({ "q": "x" }), + provider_metadata: None, + }]), + MockResponse::Text("done".into()), + ]); + let mut agent = Agent::from_provider(provider, ModelConfig::mock()).with_tools(tools); + let mut rx = agent.prompt("search the web").await; + let mut started = Vec::new(); + while let Some(event) = rx.recv().await { + if let AgentEvent::ToolExecutionStart { tool_name, .. } = event { + started.push(tool_name); + } + } + agent.finish().await; + assert_eq!(started, ["web__search"]); + + // The result came from the web server... + let result = agent + .messages() + .iter() + .find_map(|m| match m { + AgentMessage::Llm(Message::ToolResult { content, .. }) => match content.first() { + Some(Content::Text { text }) => Some(text.clone()), + _ => None, + }, + _ => None, + }) + .expect("a tool result"); + assert_eq!(result, "web"); + // ...which was called with its own tool name; the docs server never was. + let calls = |i: usize| { + logs[i] + .lock() + .unwrap() + .iter() + .filter(|r| r.method == "tools/call") + .map(|r| r.params.as_ref().unwrap()["name"].clone()) + .collect::>() + }; + assert_eq!(calls(1), [serde_json::json!("search")]); + assert!(calls(0).is_empty()); +}