From 0553dc0d4910e4e1559e0624d6d8f224146b6b7b Mon Sep 17 00:00:00 2001 From: Bennet Bo Fenner Date: Wed, 2 Jul 2025 22:32:07 +0200 Subject: [PATCH] agent: Fix issue with duplicated tool names from MCP servers (#33811) Closes #33792 Follow up to #33237 - Turns out my fix for this was not correct Release Notes: - agent: Fixed an issue where tools would not work when two MCP servers provided a tool with the same name --- crates/agent/src/agent_profile.rs | 18 +- crates/agent/src/thread.rs | 233 +---------- crates/agent/src/thread_store.rs | 24 +- crates/agent_ui/src/tool_compatibility.rs | 4 +- crates/assistant_tool/Cargo.toml | 1 + crates/assistant_tool/src/tool_working_set.rs | 379 +++++++++++++++++- 6 files changed, 386 insertions(+), 273 deletions(-) diff --git a/crates/agent/src/agent_profile.rs b/crates/agent/src/agent_profile.rs index 2c3b457dc2..a89857e71a 100644 --- a/crates/agent/src/agent_profile.rs +++ b/crates/agent/src/agent_profile.rs @@ -1,7 +1,7 @@ use std::sync::Arc; use agent_settings::{AgentProfileId, AgentProfileSettings, AgentSettings}; -use assistant_tool::{Tool, ToolSource, ToolWorkingSet}; +use assistant_tool::{Tool, ToolSource, ToolWorkingSet, UniqueToolName}; use collections::IndexMap; use convert_case::{Case, Casing}; use fs::Fs; @@ -72,7 +72,7 @@ impl AgentProfile { &self.id } - pub fn enabled_tools(&self, cx: &App) -> Vec> { + pub fn enabled_tools(&self, cx: &App) -> Vec<(UniqueToolName, Arc)> { let Some(settings) = AgentSettings::get_global(cx).profiles.get(&self.id) else { return Vec::new(); }; @@ -81,7 +81,7 @@ impl AgentProfile { .read(cx) .tools(cx) .into_iter() - .filter(|tool| Self::is_enabled(settings, tool.source(), tool.name())) + .filter(|(_, tool)| Self::is_enabled(settings, tool.source(), tool.name())) .collect() } @@ -137,7 +137,7 @@ mod tests { let mut enabled_tools = cx .read(|cx| profile.enabled_tools(cx)) .into_iter() - .map(|tool| tool.name()) + .map(|(_, tool)| tool.name()) .collect::>(); enabled_tools.sort(); @@ -174,7 +174,7 @@ mod tests { let mut enabled_tools = cx .read(|cx| profile.enabled_tools(cx)) .into_iter() - .map(|tool| tool.name()) + .map(|(_, tool)| tool.name()) .collect::>(); enabled_tools.sort(); @@ -207,7 +207,7 @@ mod tests { let mut enabled_tools = cx .read(|cx| profile.enabled_tools(cx)) .into_iter() - .map(|tool| tool.name()) + .map(|(_, tool)| tool.name()) .collect::>(); enabled_tools.sort(); @@ -267,10 +267,10 @@ mod tests { } fn default_tool_set(cx: &mut TestAppContext) -> Entity { - cx.new(|_| { + cx.new(|cx| { let mut tool_set = ToolWorkingSet::default(); - tool_set.insert(Arc::new(FakeTool::new("enabled_mcp_tool", "mcp"))); - tool_set.insert(Arc::new(FakeTool::new("disabled_mcp_tool", "mcp"))); + tool_set.insert(Arc::new(FakeTool::new("enabled_mcp_tool", "mcp")), cx); + tool_set.insert(Arc::new(FakeTool::new("disabled_mcp_tool", "mcp")), cx); tool_set }) } diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 028dabbd91..815b9e86ea 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -13,7 +13,7 @@ use anyhow::{Result, anyhow}; use assistant_tool::{ActionLog, AnyToolCard, Tool, ToolWorkingSet}; use chrono::{DateTime, Utc}; use client::{ModelRequestUsage, RequestUsage}; -use collections::{HashMap, HashSet}; +use collections::HashMap; use feature_flags::{self, FeatureFlagAppExt}; use futures::{FutureExt, StreamExt as _, future::Shared}; use git::repository::DiffType; @@ -960,13 +960,14 @@ impl Thread { model: Arc, ) -> Vec { if model.supports_tools() { - resolve_tool_name_conflicts(self.profile.enabled_tools(cx).as_slice()) + self.profile + .enabled_tools(cx) .into_iter() .filter_map(|(name, tool)| { // Skip tools that cannot be supported let input_schema = tool.input_schema(model.tool_input_format()).ok()?; Some(LanguageModelRequestTool { - name, + name: name.into(), description: tool.description(), input_schema, }) @@ -2386,7 +2387,7 @@ impl Thread { let tool_list = available_tools .iter() - .map(|tool| format!("- {}: {}", tool.name(), tool.description())) + .map(|(name, tool)| format!("- {}: {}", name, tool.description())) .collect::>() .join("\n"); @@ -2606,7 +2607,7 @@ impl Thread { .profile .enabled_tools(cx) .iter() - .map(|tool| tool.name()) + .map(|(name, _)| name.clone().into()) .collect(); self.message_feedback.insert(message_id, feedback); @@ -3144,85 +3145,6 @@ struct PendingCompletion { _task: Task<()>, } -/// Resolves tool name conflicts by ensuring all tool names are unique. -/// -/// When multiple tools have the same name, this function applies the following rules: -/// 1. Native tools always keep their original name -/// 2. Context server tools get prefixed with their server ID and an underscore -/// 3. All tool names are truncated to MAX_TOOL_NAME_LENGTH (64 characters) -/// 4. If conflicts still exist after prefixing, the conflicting tools are filtered out -/// -/// Note: This function assumes that built-in tools occur before MCP tools in the tools list. -fn resolve_tool_name_conflicts(tools: &[Arc]) -> Vec<(String, Arc)> { - fn resolve_tool_name(tool: &Arc) -> String { - let mut tool_name = tool.name(); - tool_name.truncate(MAX_TOOL_NAME_LENGTH); - tool_name - } - - const MAX_TOOL_NAME_LENGTH: usize = 64; - - let mut duplicated_tool_names = HashSet::default(); - let mut seen_tool_names = HashSet::default(); - for tool in tools { - let tool_name = resolve_tool_name(tool); - if seen_tool_names.contains(&tool_name) { - debug_assert!( - tool.source() != assistant_tool::ToolSource::Native, - "There are two built-in tools with the same name: {}", - tool_name - ); - duplicated_tool_names.insert(tool_name); - } else { - seen_tool_names.insert(tool_name); - } - } - - if duplicated_tool_names.is_empty() { - return tools - .into_iter() - .map(|tool| (resolve_tool_name(tool), tool.clone())) - .collect(); - } - - tools - .into_iter() - .filter_map(|tool| { - let mut tool_name = resolve_tool_name(tool); - if !duplicated_tool_names.contains(&tool_name) { - return Some((tool_name, tool.clone())); - } - match tool.source() { - assistant_tool::ToolSource::Native => { - // Built-in tools always keep their original name - Some((tool_name, tool.clone())) - } - assistant_tool::ToolSource::ContextServer { id } => { - // Context server tools are prefixed with the context server ID, and truncated if necessary - tool_name.insert(0, '_'); - if tool_name.len() + id.len() > MAX_TOOL_NAME_LENGTH { - let len = MAX_TOOL_NAME_LENGTH - tool_name.len(); - let mut id = id.to_string(); - id.truncate(len); - tool_name.insert_str(0, &id); - } else { - tool_name.insert_str(0, &id); - } - - tool_name.truncate(MAX_TOOL_NAME_LENGTH); - - if seen_tool_names.contains(&tool_name) { - log::error!("Cannot resolve tool name conflict for tool {}", tool.name()); - None - } else { - Some((tool_name, tool.clone())) - } - } - } - }) - .collect() -} - #[cfg(test)] mod tests { use super::*; @@ -3238,7 +3160,6 @@ mod tests { use futures::future::BoxFuture; use futures::stream::BoxStream; use gpui::TestAppContext; - use icons::IconName; use language_model::fake_provider::{FakeLanguageModel, FakeLanguageModelProvider}; use language_model::{ LanguageModelCompletionError, LanguageModelName, LanguageModelProviderId, @@ -3883,148 +3804,6 @@ fn main() {{ }); } - #[gpui::test] - fn test_resolve_tool_name_conflicts() { - use assistant_tool::{Tool, ToolSource}; - - assert_resolve_tool_name_conflicts( - vec![ - TestTool::new("tool1", ToolSource::Native), - TestTool::new("tool2", ToolSource::Native), - TestTool::new("tool3", ToolSource::ContextServer { id: "mcp-1".into() }), - ], - vec!["tool1", "tool2", "tool3"], - ); - - assert_resolve_tool_name_conflicts( - vec![ - TestTool::new("tool1", ToolSource::Native), - TestTool::new("tool2", ToolSource::Native), - TestTool::new("tool3", ToolSource::ContextServer { id: "mcp-1".into() }), - TestTool::new("tool3", ToolSource::ContextServer { id: "mcp-2".into() }), - ], - vec!["tool1", "tool2", "mcp-1_tool3", "mcp-2_tool3"], - ); - - assert_resolve_tool_name_conflicts( - vec![ - TestTool::new("tool1", ToolSource::Native), - TestTool::new("tool2", ToolSource::Native), - TestTool::new("tool3", ToolSource::Native), - TestTool::new("tool3", ToolSource::ContextServer { id: "mcp-1".into() }), - TestTool::new("tool3", ToolSource::ContextServer { id: "mcp-2".into() }), - ], - vec!["tool1", "tool2", "tool3", "mcp-1_tool3", "mcp-2_tool3"], - ); - - // Test that tool with very long name is always truncated - assert_resolve_tool_name_conflicts( - vec![TestTool::new( - "tool-with-more-then-64-characters-blah-blah-blah-blah-blah-blah-blah-blah", - ToolSource::Native, - )], - vec!["tool-with-more-then-64-characters-blah-blah-blah-blah-blah-blah-"], - ); - - // Test deduplication of tools with very long names, in this case the mcp server name should be truncated - assert_resolve_tool_name_conflicts( - vec![ - TestTool::new("tool-with-very-very-very-long-name", ToolSource::Native), - TestTool::new( - "tool-with-very-very-very-long-name", - ToolSource::ContextServer { - id: "mcp-with-very-very-very-long-name".into(), - }, - ), - ], - vec![ - "tool-with-very-very-very-long-name", - "mcp-with-very-very-very-long-_tool-with-very-very-very-long-name", - ], - ); - - fn assert_resolve_tool_name_conflicts( - tools: Vec, - expected: Vec>, - ) { - let tools: Vec> = tools - .into_iter() - .map(|t| Arc::new(t) as Arc) - .collect(); - let tools = resolve_tool_name_conflicts(&tools); - assert_eq!(tools.len(), expected.len()); - for (i, expected_name) in expected.into_iter().enumerate() { - let expected_name = expected_name.into(); - let actual_name = &tools[i].0; - assert_eq!( - actual_name, &expected_name, - "Expected '{}' got '{}' at index {}", - expected_name, actual_name, i - ); - } - } - - struct TestTool { - name: String, - source: ToolSource, - } - - impl TestTool { - fn new(name: impl Into, source: ToolSource) -> Self { - Self { - name: name.into(), - source, - } - } - } - - impl Tool for TestTool { - fn name(&self) -> String { - self.name.clone() - } - - fn icon(&self) -> IconName { - IconName::Ai - } - - fn may_perform_edits(&self) -> bool { - false - } - - fn needs_confirmation(&self, _input: &serde_json::Value, _cx: &App) -> bool { - true - } - - fn source(&self) -> ToolSource { - self.source.clone() - } - - fn description(&self) -> String { - "Test tool".to_string() - } - - fn ui_text(&self, _input: &serde_json::Value) -> String { - "Test tool".to_string() - } - - fn run( - self: Arc, - _input: serde_json::Value, - _request: Arc, - _project: Entity, - _action_log: Entity, - _model: Arc, - _window: Option, - _cx: &mut App, - ) -> assistant_tool::ToolResult { - assistant_tool::ToolResult { - output: Task::ready(Err(anyhow::anyhow!("No content"))), - card: None, - } - } - } - } - // Helper to create a model that returns errors enum TestError { Overloaded, diff --git a/crates/agent/src/thread_store.rs b/crates/agent/src/thread_store.rs index 516151e9ff..0347156cd4 100644 --- a/crates/agent/src/thread_store.rs +++ b/crates/agent/src/thread_store.rs @@ -6,7 +6,7 @@ use crate::{ }; use agent_settings::{AgentProfileId, CompletionMode}; use anyhow::{Context as _, Result, anyhow}; -use assistant_tool::{ToolId, ToolWorkingSet}; +use assistant_tool::{Tool, ToolId, ToolWorkingSet}; use chrono::{DateTime, Utc}; use collections::HashMap; use context_server::ContextServerId; @@ -537,8 +537,8 @@ impl ThreadStore { } ContextServerStatus::Stopped | ContextServerStatus::Error(_) => { if let Some(tool_ids) = self.context_server_tool_ids.remove(server_id) { - tool_working_set.update(cx, |tool_working_set, _| { - tool_working_set.remove(&tool_ids); + tool_working_set.update(cx, |tool_working_set, cx| { + tool_working_set.remove(&tool_ids, cx); }); } } @@ -569,19 +569,17 @@ impl ThreadStore { .log_err() { let tool_ids = tool_working_set - .update(cx, |tool_working_set, _| { - response - .tools - .into_iter() - .map(|tool| { - log::info!("registering context server tool: {:?}", tool.name); - tool_working_set.insert(Arc::new(ContextServerTool::new( + .update(cx, |tool_working_set, cx| { + tool_working_set.extend( + response.tools.into_iter().map(|tool| { + Arc::new(ContextServerTool::new( context_server_store.clone(), server.id(), tool, - ))) - }) - .collect::>() + )) as Arc + }), + cx, + ) }) .log_err(); diff --git a/crates/agent_ui/src/tool_compatibility.rs b/crates/agent_ui/src/tool_compatibility.rs index 936612e556..d4e1da5bb0 100644 --- a/crates/agent_ui/src/tool_compatibility.rs +++ b/crates/agent_ui/src/tool_compatibility.rs @@ -42,8 +42,8 @@ impl IncompatibleToolsState { .profile() .enabled_tools(cx) .iter() - .filter(|tool| tool.input_schema(model.tool_input_format()).is_err()) - .cloned() + .filter(|(_, tool)| tool.input_schema(model.tool_input_format()).is_err()) + .map(|(_, tool)| tool.clone()) .collect() }) } diff --git a/crates/assistant_tool/Cargo.toml b/crates/assistant_tool/Cargo.toml index a8df1131c6..5a54e86eac 100644 --- a/crates/assistant_tool/Cargo.toml +++ b/crates/assistant_tool/Cargo.toml @@ -22,6 +22,7 @@ gpui.workspace = true icons.workspace = true language.workspace = true language_model.workspace = true +log.workspace = true parking_lot.workspace = true project.workspace = true regex.workspace = true diff --git a/crates/assistant_tool/src/tool_working_set.rs b/crates/assistant_tool/src/tool_working_set.rs index c72c52ba7a..9a6ec49914 100644 --- a/crates/assistant_tool/src/tool_working_set.rs +++ b/crates/assistant_tool/src/tool_working_set.rs @@ -1,18 +1,52 @@ -use std::sync::Arc; - -use collections::{HashMap, IndexMap}; -use gpui::App; +use std::{borrow::Borrow, sync::Arc}; use crate::{Tool, ToolRegistry, ToolSource}; +use collections::{HashMap, HashSet, IndexMap}; +use gpui::{App, SharedString}; +use util::debug_panic; #[derive(Copy, Clone, PartialEq, Eq, Hash, Default)] pub struct ToolId(usize); +/// A unique identifier for a tool within a working set. +#[derive(Clone, PartialEq, Eq, Hash, Default)] +pub struct UniqueToolName(SharedString); + +impl Borrow for UniqueToolName { + fn borrow(&self) -> &str { + &self.0 + } +} + +impl From for UniqueToolName { + fn from(value: String) -> Self { + UniqueToolName(SharedString::new(value)) + } +} + +impl Into for UniqueToolName { + fn into(self) -> String { + self.0.into() + } +} + +impl std::fmt::Debug for UniqueToolName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) + } +} + +impl std::fmt::Display for UniqueToolName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0.as_ref()) + } +} + /// A working set of tools for use in one instance of the Assistant Panel. #[derive(Default)] pub struct ToolWorkingSet { context_server_tools_by_id: HashMap>, - context_server_tools_by_name: HashMap>, + context_server_tools_by_name: HashMap>, next_tool_id: ToolId, } @@ -24,16 +58,20 @@ impl ToolWorkingSet { .or_else(|| ToolRegistry::global(cx).tool(name)) } - pub fn tools(&self, cx: &App) -> Vec> { - let mut tools = ToolRegistry::global(cx).tools(); - tools.extend(self.context_server_tools_by_id.values().cloned()); + pub fn tools(&self, cx: &App) -> Vec<(UniqueToolName, Arc)> { + let mut tools = ToolRegistry::global(cx) + .tools() + .into_iter() + .map(|tool| (UniqueToolName(tool.name().into()), tool)) + .collect::>(); + tools.extend(self.context_server_tools_by_name.clone()); tools } pub fn tools_by_source(&self, cx: &App) -> IndexMap>> { let mut tools_by_source = IndexMap::default(); - for tool in self.tools(cx) { + for (_, tool) in self.tools(cx) { tools_by_source .entry(tool.source()) .or_insert_with(Vec::new) @@ -49,27 +87,324 @@ impl ToolWorkingSet { tools_by_source } - pub fn insert(&mut self, tool: Arc) -> ToolId { + pub fn insert(&mut self, tool: Arc, cx: &App) -> ToolId { + let tool_id = self.register_tool(tool); + self.tools_changed(cx); + tool_id + } + + pub fn extend(&mut self, tools: impl Iterator>, cx: &App) -> Vec { + let ids = tools.map(|tool| self.register_tool(tool)).collect(); + self.tools_changed(cx); + ids + } + + pub fn remove(&mut self, tool_ids_to_remove: &[ToolId], cx: &App) { + self.context_server_tools_by_id + .retain(|id, _| !tool_ids_to_remove.contains(id)); + self.tools_changed(cx); + } + + fn register_tool(&mut self, tool: Arc) -> ToolId { let tool_id = self.next_tool_id; self.next_tool_id.0 += 1; self.context_server_tools_by_id .insert(tool_id, tool.clone()); - self.tools_changed(); tool_id } - pub fn remove(&mut self, tool_ids_to_remove: &[ToolId]) { - self.context_server_tools_by_id - .retain(|id, _| !tool_ids_to_remove.contains(id)); - self.tools_changed(); - } - - fn tools_changed(&mut self) { - self.context_server_tools_by_name.clear(); - self.context_server_tools_by_name.extend( - self.context_server_tools_by_id + fn tools_changed(&mut self, cx: &App) { + self.context_server_tools_by_name = resolve_context_server_tool_name_conflicts( + &self + .context_server_tools_by_id .values() - .map(|tool| (tool.name(), tool.clone())), + .cloned() + .collect::>(), + &ToolRegistry::global(cx).tools(), ); } } + +fn resolve_context_server_tool_name_conflicts( + context_server_tools: &[Arc], + native_tools: &[Arc], +) -> HashMap> { + fn resolve_tool_name(tool: &Arc) -> String { + let mut tool_name = tool.name(); + tool_name.truncate(MAX_TOOL_NAME_LENGTH); + tool_name + } + + const MAX_TOOL_NAME_LENGTH: usize = 64; + + let mut duplicated_tool_names = HashSet::default(); + let mut seen_tool_names = HashSet::default(); + seen_tool_names.extend(native_tools.iter().map(|tool| tool.name())); + for tool in context_server_tools { + let tool_name = resolve_tool_name(tool); + if seen_tool_names.contains(&tool_name) { + debug_assert!( + tool.source() != ToolSource::Native, + "Expected MCP tool but got a native tool: {}", + tool_name + ); + duplicated_tool_names.insert(tool_name); + } else { + seen_tool_names.insert(tool_name); + } + } + + if duplicated_tool_names.is_empty() { + return context_server_tools + .into_iter() + .map(|tool| (resolve_tool_name(tool).into(), tool.clone())) + .collect(); + } + + context_server_tools + .into_iter() + .filter_map(|tool| { + let mut tool_name = resolve_tool_name(tool); + if !duplicated_tool_names.contains(&tool_name) { + return Some((tool_name.into(), tool.clone())); + } + match tool.source() { + ToolSource::Native => { + debug_panic!("Expected MCP tool but got a native tool: {}", tool_name); + // Built-in tools always keep their original name + Some((tool_name.into(), tool.clone())) + } + ToolSource::ContextServer { id } => { + // Context server tools are prefixed with the context server ID, and truncated if necessary + tool_name.insert(0, '_'); + if tool_name.len() + id.len() > MAX_TOOL_NAME_LENGTH { + let len = MAX_TOOL_NAME_LENGTH - tool_name.len(); + let mut id = id.to_string(); + id.truncate(len); + tool_name.insert_str(0, &id); + } else { + tool_name.insert_str(0, &id); + } + + tool_name.truncate(MAX_TOOL_NAME_LENGTH); + + if seen_tool_names.contains(&tool_name) { + log::error!("Cannot resolve tool name conflict for tool {}", tool.name()); + None + } else { + Some((tool_name.into(), tool.clone())) + } + } + } + }) + .collect() +} +#[cfg(test)] +mod tests { + use gpui::{AnyWindowHandle, Entity, Task, TestAppContext}; + use language_model::{LanguageModel, LanguageModelRequest}; + use project::Project; + + use crate::{ActionLog, ToolResult}; + + use super::*; + + #[gpui::test] + fn test_unique_tool_names(cx: &mut TestAppContext) { + fn assert_tool( + tool_working_set: &ToolWorkingSet, + unique_name: &str, + expected_name: &str, + expected_source: ToolSource, + cx: &App, + ) { + let tool = tool_working_set.tool(unique_name, cx).unwrap(); + assert_eq!(tool.name(), expected_name); + assert_eq!(tool.source(), expected_source); + } + + let tool_registry = cx.update(ToolRegistry::default_global); + tool_registry.register_tool(TestTool::new("tool1", ToolSource::Native)); + tool_registry.register_tool(TestTool::new("tool2", ToolSource::Native)); + + let mut tool_working_set = ToolWorkingSet::default(); + cx.update(|cx| { + tool_working_set.extend( + vec![ + Arc::new(TestTool::new( + "tool2", + ToolSource::ContextServer { id: "mcp-1".into() }, + )) as Arc, + Arc::new(TestTool::new( + "tool2", + ToolSource::ContextServer { id: "mcp-2".into() }, + )) as Arc, + ] + .into_iter(), + cx, + ); + }); + + cx.update(|cx| { + assert_tool(&tool_working_set, "tool1", "tool1", ToolSource::Native, cx); + assert_tool(&tool_working_set, "tool2", "tool2", ToolSource::Native, cx); + assert_tool( + &tool_working_set, + "mcp-1_tool2", + "tool2", + ToolSource::ContextServer { id: "mcp-1".into() }, + cx, + ); + assert_tool( + &tool_working_set, + "mcp-2_tool2", + "tool2", + ToolSource::ContextServer { id: "mcp-2".into() }, + cx, + ); + }) + } + + #[gpui::test] + fn test_resolve_context_server_tool_name_conflicts() { + assert_resolve_context_server_tool_name_conflicts( + vec![ + TestTool::new("tool1", ToolSource::Native), + TestTool::new("tool2", ToolSource::Native), + ], + vec![TestTool::new( + "tool3", + ToolSource::ContextServer { id: "mcp-1".into() }, + )], + vec!["tool3"], + ); + + assert_resolve_context_server_tool_name_conflicts( + vec![ + TestTool::new("tool1", ToolSource::Native), + TestTool::new("tool2", ToolSource::Native), + ], + vec![ + TestTool::new("tool3", ToolSource::ContextServer { id: "mcp-1".into() }), + TestTool::new("tool3", ToolSource::ContextServer { id: "mcp-2".into() }), + ], + vec!["mcp-1_tool3", "mcp-2_tool3"], + ); + + assert_resolve_context_server_tool_name_conflicts( + vec![ + TestTool::new("tool1", ToolSource::Native), + TestTool::new("tool2", ToolSource::Native), + TestTool::new("tool3", ToolSource::Native), + ], + vec![ + TestTool::new("tool3", ToolSource::ContextServer { id: "mcp-1".into() }), + TestTool::new("tool3", ToolSource::ContextServer { id: "mcp-2".into() }), + ], + vec!["mcp-1_tool3", "mcp-2_tool3"], + ); + + // Test deduplication of tools with very long names, in this case the mcp server name should be truncated + assert_resolve_context_server_tool_name_conflicts( + vec![TestTool::new( + "tool-with-very-very-very-long-name", + ToolSource::Native, + )], + vec![TestTool::new( + "tool-with-very-very-very-long-name", + ToolSource::ContextServer { + id: "mcp-with-very-very-very-long-name".into(), + }, + )], + vec!["mcp-with-very-very-very-long-_tool-with-very-very-very-long-name"], + ); + + fn assert_resolve_context_server_tool_name_conflicts( + builtin_tools: Vec, + context_server_tools: Vec, + expected: Vec<&'static str>, + ) { + let context_server_tools: Vec> = context_server_tools + .into_iter() + .map(|t| Arc::new(t) as Arc) + .collect(); + let builtin_tools: Vec> = builtin_tools + .into_iter() + .map(|t| Arc::new(t) as Arc) + .collect(); + let tools = + resolve_context_server_tool_name_conflicts(&context_server_tools, &builtin_tools); + assert_eq!(tools.len(), expected.len()); + for (i, (name, _)) in tools.into_iter().enumerate() { + assert_eq!( + name.0.as_ref(), + expected[i], + "Expected '{}' got '{}' at index {}", + expected[i], + name, + i + ); + } + } + } + + struct TestTool { + name: String, + source: ToolSource, + } + + impl TestTool { + fn new(name: impl Into, source: ToolSource) -> Self { + Self { + name: name.into(), + source, + } + } + } + + impl Tool for TestTool { + fn name(&self) -> String { + self.name.clone() + } + + fn icon(&self) -> icons::IconName { + icons::IconName::Ai + } + + fn may_perform_edits(&self) -> bool { + false + } + + fn needs_confirmation(&self, _input: &serde_json::Value, _cx: &App) -> bool { + true + } + + fn source(&self) -> ToolSource { + self.source.clone() + } + + fn description(&self) -> String { + "Test tool".to_string() + } + + fn ui_text(&self, _input: &serde_json::Value) -> String { + "Test tool".to_string() + } + + fn run( + self: Arc, + _input: serde_json::Value, + _request: Arc, + _project: Entity, + _action_log: Entity, + _model: Arc, + _window: Option, + _cx: &mut App, + ) -> ToolResult { + ToolResult { + output: Task::ready(Err(anyhow::anyhow!("No content"))), + card: None, + } + } + } +}