diff --git a/.changeset/fix-mcp-tool-name-collision.md b/.changeset/fix-mcp-tool-name-collision.md new file mode 100644 index 000000000..c90447925 --- /dev/null +++ b/.changeset/fix-mcp-tool-name-collision.md @@ -0,0 +1,7 @@ +--- +"@moonshot-ai/agent-core": patch +"@moonshot-ai/kimi-code": patch + +--- + +Disambiguate MCP tools with colliding names across servers instead of silently dropping them. diff --git a/packages/agent-core/src/agent/tool/index.ts b/packages/agent-core/src/agent/tool/index.ts index 33679f88c..9b7544c34 100644 --- a/packages/agent-core/src/agent/tool/index.ts +++ b/packages/agent-core/src/agent/tool/index.ts @@ -39,6 +39,10 @@ export class ToolManager { protected enabledTools: Set = new Set(); /** Glob patterns (e.g. `mcp__*`, `mcp__github__*`) gating which MCP tools the profile exposes. */ private mcpAccessPatterns: string[] = []; + /** Tracks how many times each base qualified name has been disambiguated across registrations. */ + private readonly mcpCollisionCount = new Map(); + /** Maps server name → base qualified names (before disambiguation) for cleanup. */ + private readonly mcpServerToolBases = new Map(); protected readonly store: Partial = {}; private mcpToolStatusUnsubscribe: (() => void) | undefined; @@ -141,11 +145,14 @@ export class ToolManager { ): McpServerRegistrationResult { this.unregisterMcpServer(serverName); const qualifiedNames: string[] = []; + const baseNames: string[] = []; const collisions: McpToolCollision[] = []; const seenInThisCall = new Map(); for (const tool of tools) { if (enabledTools !== undefined && !enabledTools.has(tool.name)) continue; const qualified = qualifyMcpToolName(serverName, tool.name); + // Track by base name (before disambiguation) so same-server duplicates + // are always caught, even when the base name collides with another server. const firstInThisCall = seenInThisCall.get(qualified); if (firstInThisCall !== undefined) { collisions.push({ @@ -155,23 +162,40 @@ export class ToolManager { }); continue; } + seenInThisCall.set(qualified, tool.name); const existingEntry = this.mcpTools.get(qualified); + let finalName = qualified; if (existingEntry !== undefined) { + // Cross-server collision: disambiguate by appending a numeric suffix + // so both tools remain accessible. + let count = (this.mcpCollisionCount.get(qualified) ?? 1) + 1; + // Probe for uniqueness: if truncation causes the disambiguated name + // to collide with an existing tool, keep incrementing the suffix. + let disambiguated: string; + do { + const suffix = `__${String(count)}`; + const maxBase = 64 - suffix.length; + disambiguated = qualified.length > maxBase + ? `${qualified.slice(0, maxBase)}${suffix}` + : `${qualified}${suffix}`; + count++; + } while (this.mcpTools.has(disambiguated)); + count--; + this.mcpCollisionCount.set(qualified, count); + finalName = disambiguated; collisions.push({ - qualified, + qualified: finalName, toolName: tool.name, collidesWith: { kind: 'other_server', serverName: existingEntry.serverName }, }); - continue; } - seenInThisCall.set(qualified, tool.name); const wrapped: ExecutableTool = { - name: qualified, + name: finalName, description: tool.description, parameters: tool.parameters, resolveExecution: (args) => { return { - approvalRule: qualified, + approvalRule: finalName, execute: async (context) => { // `args` has already been JSON-parsed and schema-validated by // the loop's preflight (`loop/tool-call.ts`), so the MCP @@ -181,15 +205,17 @@ export class ToolManager { (args ?? {}) as Record, context.signal, ); - return mcpResultToExecutableOutput(result, qualified); + return mcpResultToExecutableOutput(result, finalName); }, }; }, }; - this.mcpTools.set(qualified, { tool: wrapped, serverName }); - qualifiedNames.push(qualified); + this.mcpTools.set(finalName, { tool: wrapped, serverName }); + qualifiedNames.push(finalName); + baseNames.push(qualified); } this.mcpToolsByServer.set(serverName, qualifiedNames); + this.mcpServerToolBases.set(serverName, baseNames); return { registered: qualifiedNames, collisions }; } @@ -200,6 +226,15 @@ export class ToolManager { this.mcpTools.delete(qualified); } this.mcpToolsByServer.delete(serverName); + // Clear collision counts for this server's base tool names so that + // re-registration after a reconnect reuses the same suffix numbers. + const bases = this.mcpServerToolBases.get(serverName); + if (bases !== undefined) { + for (const base of bases) { + this.mcpCollisionCount.delete(base); + } + this.mcpServerToolBases.delete(serverName); + } return true; } @@ -286,8 +321,8 @@ export class ToolManager { const summary = collisions .map((c) => c.collidesWith.kind === 'same_server' - ? `"${c.toolName}" -> ${c.qualified} (collides with "${c.collidesWith.toolName}" from the same server)` - : `"${c.toolName}" -> ${c.qualified} (collides with server "${c.collidesWith.serverName}")`, + ? `"${c.toolName}" -> ${c.qualified} (collides with "${c.collidesWith.toolName}" from the same server; duplicate dropped)` + : `"${c.toolName}" -> ${c.qualified} (disambiguated from server "${c.collidesWith.serverName}")`, ) .join('; '); this.agent.emitEvent({ @@ -296,7 +331,7 @@ export class ToolManager { 'mcp.tool_name_collision', `MCP server "${serverName}" registered ${collisions.length} tool name` + `${collisions.length === 1 ? '' : 's'} ` + - `that collide with existing qualified names; the losing tools were dropped: ${summary}`, + `that collide with existing qualified names; cross-server duplicates are disambiguated with a numeric suffix: ${summary}`, { details: { serverName, collisions: collisions as readonly unknown[] } }, ), }); diff --git a/packages/agent-core/test/mcp/tool-manager-mcp.test.ts b/packages/agent-core/test/mcp/tool-manager-mcp.test.ts index cdd3bde50..3aab77468 100644 --- a/packages/agent-core/test/mcp/tool-manager-mcp.test.ts +++ b/packages/agent-core/test/mcp/tool-manager-mcp.test.ts @@ -148,7 +148,7 @@ describe('ToolManager MCP integration', () => { expect(mcpNames).toEqual(['mcp__srv__a_b']); }); - it('reports cross-server collisions instead of silently overwriting another server tool', async () => { + it('disambiguates cross-server collisions with a numeric suffix instead of dropping', async () => { const tm = new ToolManager(fakeAgent()); tm.setActiveTools(['mcp__*']); const firstClient: MCPClient = { @@ -181,16 +181,19 @@ describe('ToolManager MCP integration', () => { tm.registerMcpServer('srv a', firstClient, await discoverTools(firstClient)); const result = tm.registerMcpServer('srv__a', secondClient, await discoverTools(secondClient)); - expect(result.registered).toEqual([]); + // The second server's tool is disambiguated with __2 suffix. + expect(result.registered).toEqual(['mcp__srv_a__shared__2']); expect(result.collisions).toEqual([ { - qualified: 'mcp__srv_a__shared', + qualified: 'mcp__srv_a__shared__2', toolName: 'shared', collidesWith: { kind: 'other_server', serverName: 'srv a' }, }, ]); - // First server's tool still wins and stays callable. - expect(tm.loopTools.map((t) => t.name)).toEqual(['mcp__srv_a__shared']); + // Both tools are now accessible. + const mcpNames = [...tm.toolInfos()].filter((i) => i.source === 'mcp').map((i) => i.name); + expect(mcpNames).toContain('mcp__srv_a__shared'); + expect(mcpNames).toContain('mcp__srv_a__shared__2'); }); it('re-registering the same server replaces its previous tool set', async () => { @@ -219,6 +222,199 @@ describe('ToolManager MCP integration', () => { expect(mcpNames).toEqual(['mcp__s__only']); }); + it('assigns incrementing suffixes for multiple cross-server collisions on the same tool', async () => { + const tm = new ToolManager(fakeAgent()); + tm.setActiveTools(['mcp__*']); + const makeClient = (desc: string): MCPClient => ({ + async listTools() { + return [ + { name: 'shared', description: desc, inputSchema: { type: 'object', properties: {} } }, + ]; + }, + async callTool() { + return { content: [{ type: 'text', text: desc }], isError: false }; + }, + }); + + // "srv a", "srv__a", "srv a" all sanitize to "srv_a" so they collide. + tm.registerMcpServer('srv a', makeClient('first'), await discoverTools(makeClient('first'))); + const r2 = tm.registerMcpServer('srv__a', makeClient('second'), await discoverTools(makeClient('second'))); + const r3 = tm.registerMcpServer('srv a', makeClient('third'), await discoverTools(makeClient('third'))); + + expect(r2.registered).toEqual(['mcp__srv_a__shared__2']); + expect(r3.registered).toEqual(['mcp__srv_a__shared__3']); + const mcpNames = [...tm.toolInfos()].filter((i) => i.source === 'mcp').map((i) => i.name); + expect(mcpNames).toContain('mcp__srv_a__shared'); + expect(mcpNames).toContain('mcp__srv_a__shared__2'); + expect(mcpNames).toContain('mcp__srv_a__shared__3'); + }); + + it('allows calling both original and disambiguated MCP tools independently', async () => { + const tm = new ToolManager(fakeAgent()); + tm.setActiveTools(['mcp__*']); + const firstClient: MCPClient = { + async listTools() { + return [ + { name: 'query', description: 'first', inputSchema: { type: 'object', properties: {} } }, + ]; + }, + async callTool() { + return { content: [{ type: 'text', text: 'result-from-first' }], isError: false }; + }, + }; + const secondClient: MCPClient = { + async listTools() { + return [ + { name: 'query', description: 'second', inputSchema: { type: 'object', properties: {} } }, + ]; + }, + async callTool() { + return { content: [{ type: 'text', text: 'result-from-second' }], isError: false }; + }, + }; + + // "alpha beta" and "alpha__beta" both sanitize to "alpha_beta", causing collision. + tm.registerMcpServer('alpha beta', firstClient, await discoverTools(firstClient)); + tm.registerMcpServer('alpha__beta', secondClient, await discoverTools(secondClient)); + + // Both tools should be registered. + const allNames = [...tm.toolInfos()].filter((i) => i.source === 'mcp').map((i) => i.name); + expect(allNames).toContain('mcp__alpha_beta__query'); + expect(allNames).toContain('mcp__alpha_beta__query__2'); + + // Call the original tool. + const alphaExec = tm.loopTools.find((t) => t.name === 'mcp__alpha_beta__query'); + expect(alphaExec).toBeDefined(); + const alphaResult = await executeTool(alphaExec!, { + turnId: '0', + toolCallId: 'c1', + args: {}, + signal: new AbortController().signal, + }); + expect(alphaResult.output).toContain('result-from-first'); + + // Call the disambiguated tool. + const betaExec = tm.loopTools.find((t) => t.name === 'mcp__alpha_beta__query__2'); + expect(betaExec).toBeDefined(); + const betaResult = await executeTool(betaExec!, { + turnId: '0', + toolCallId: 'c2', + args: {}, + signal: new AbortController().signal, + }); + expect(betaResult.output).toContain('result-from-second'); + }); + + it('reports disambiguation details in collision results for cross-server collisions', async () => { + const tm = new ToolManager(fakeAgent()); + tm.setActiveTools(['mcp__*']); + const firstClient: MCPClient = { + async listTools() { + return [ + { name: 'search', description: 'v1', inputSchema: { type: 'object', properties: {} } }, + ]; + }, + async callTool() { + return { content: [], isError: false }; + }, + }; + const secondClient: MCPClient = { + async listTools() { + return [ + { name: 'search', description: 'v2', inputSchema: { type: 'object', properties: {} } }, + ]; + }, + async callTool() { + return { content: [], isError: false }; + }, + }; + + // "srv a" and "srv__a" both sanitize to "srv_a", causing collision. + tm.registerMcpServer('srv a', firstClient, await discoverTools(firstClient)); + const result = tm.registerMcpServer('srv__a', secondClient, await discoverTools(secondClient)); + + // The collision result should contain disambiguation details. + expect(result.collisions).toHaveLength(1); + expect(result.collisions[0]).toMatchObject({ + qualified: 'mcp__srv_a__search__2', + toolName: 'search', + collidesWith: { kind: 'other_server', serverName: 'srv a' }, + }); + // The second tool should be registered under the disambiguated name. + expect(result.registered).toEqual(['mcp__srv_a__search__2']); + // Both tools should be in the tool list. + const allNames = [...tm.toolInfos()].filter((i) => i.source === 'mcp').map((i) => i.name); + expect(allNames).toContain('mcp__srv_a__search'); + expect(allNames).toContain('mcp__srv_a__search__2'); + }); + + it('truncates disambiguated name when it would exceed MAX_QUALIFIED_LENGTH', async () => { + const tm = new ToolManager(fakeAgent()); + tm.setActiveTools(['mcp__*']); + // Create a server name + tool name that produces a qualified name near the limit. + const longServer = 'a'.repeat(50); + const firstClient: MCPClient = { + async listTools() { + return [ + { name: 'x', description: 'first', inputSchema: { type: 'object', properties: {} } }, + ]; + }, + async callTool() { + return { content: [{ type: 'text', text: 'ok' }], isError: false }; + }, + }; + const secondClient: MCPClient = { + async listTools() { + return [ + { name: 'x', description: 'second', inputSchema: { type: 'object', properties: {} } }, + ]; + }, + async callTool() { + return { content: [{ type: 'text', text: 'ok' }], isError: false }; + }, + }; + + tm.registerMcpServer(longServer, firstClient, await discoverTools(firstClient)); + const result = tm.registerMcpServer(`${longServer} `, secondClient, await discoverTools(secondClient)); + + // The disambiguated name must not exceed 64 characters. + for (const name of result.registered) { + expect(name.length).toBeLessThanOrEqual(64); + } + // The tool should still be callable. + const allNames = [...tm.toolInfos()].filter((i) => i.source === 'mcp').map((i) => i.name); + expect(allNames.length).toBeGreaterThanOrEqual(2); + }); + + it('probes for uniqueness when truncation would cause a disambiguation collision', async () => { + const tm = new ToolManager(fakeAgent()); + tm.setActiveTools(['mcp__*']); + + // Register two servers that collide. "srv a" and "srv__a" both sanitize to "srv_a". + const makeClient = (desc: string): MCPClient => ({ + async listTools() { + return [{ name: 'tool', description: desc, inputSchema: { type: 'object', properties: {} } }]; + }, + async callTool() { return { content: [{ type: 'text', text: desc }], isError: false }; }, + }); + + tm.registerMcpServer('srv a', makeClient('first'), await discoverTools(makeClient('first'))); + const r2 = tm.registerMcpServer('srv__a', makeClient('second'), await discoverTools(makeClient('second'))); + + // The first collision gets __2. + expect(r2.registered).toEqual(['mcp__srv_a__tool__2']); + + // Now register a third server that also collides — should get __3, not __2. + const r3 = tm.registerMcpServer('srv a', makeClient('third'), await discoverTools(makeClient('third'))); + expect(r3.registered).toEqual(['mcp__srv_a__tool__3']); + + // All three tools should be registered. + const allNames = [...tm.toolInfos()].filter((i) => i.source === 'mcp').map((i) => i.name); + expect(allNames).toContain('mcp__srv_a__tool'); + expect(allNames).toContain('mcp__srv_a__tool__2'); + expect(allNames).toContain('mcp__srv_a__tool__3'); + }); + it('does not write set_active_tools records when registering an MCP server', async () => { const calls: unknown[] = []; const tm = new ToolManager(fakeAgent(calls));