fix(agent-core): disambiguate MCP tools with colliding names across s…

.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.

packages/agent-core/src/agent/tool/index.ts

@@ -39,6 +39,10 @@ export class ToolManager {
   protected enabledTools: Set<string> = 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<string, number>();
+  /** Maps server name → base qualified names (before disambiguation) for cleanup. */
+  private readonly mcpServerToolBases = new Map<string, string[]>();
   protected readonly store: Partial<ToolStoreData> = {};
   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<string, string>();
     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<string, unknown>,
                 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[] } },
       ),
     });

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));