From 143c5042b5c5904049887b0c6213480953b2688e Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 16:51:13 +0100 Subject: [PATCH 1/6] fix(agent-core): disambiguate MCP tools with colliding names across servers Instead of dropping the second server's tool when two MCP servers expose tools with the same name, append a numeric suffix (__2, __3) so both tools remain accessible. This fixes #575. Enhancements: - Track collision count across registrations for incrementing suffixes - Updated collision error message to mention disambiguation - Both original and disambiguated tools are callable independently Tests: - Cross-server collision disambiguation with suffix assignment - Multiple collisions get incrementing suffixes - Both tools callable independently via loopTools - Collision results contain disambiguation details --- packages/agent-core/src/agent/tool/index.ts | 16 +- .../test/mcp/tool-manager-mcp.test.ts | 139 +++++++++++++++++- 2 files changed, 145 insertions(+), 10 deletions(-) diff --git a/packages/agent-core/src/agent/tool/index.ts b/packages/agent-core/src/agent/tool/index.ts index 33679f88c..e651b3ca3 100644 --- a/packages/agent-core/src/agent/tool/index.ts +++ b/packages/agent-core/src/agent/tool/index.ts @@ -39,6 +39,8 @@ 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(); protected readonly store: Partial = {}; private mcpToolStatusUnsubscribe: (() => void) | undefined; @@ -145,7 +147,7 @@ export class ToolManager { const seenInThisCall = new Map(); for (const tool of tools) { if (enabledTools !== undefined && !enabledTools.has(tool.name)) continue; - const qualified = qualifyMcpToolName(serverName, tool.name); + let qualified = qualifyMcpToolName(serverName, tool.name); const firstInThisCall = seenInThisCall.get(qualified); if (firstInThisCall !== undefined) { collisions.push({ @@ -157,12 +159,16 @@ export class ToolManager { } const existingEntry = this.mcpTools.get(qualified); if (existingEntry !== undefined) { + // Cross-server collision: disambiguate by appending a numeric suffix + // so both tools remain accessible. + const count = (this.mcpCollisionCount.get(qualified) ?? 1) + 1; + this.mcpCollisionCount.set(qualified, count); + qualified = `${qualified}__${String(count)}`; collisions.push({ qualified, toolName: tool.name, collidesWith: { kind: 'other_server', serverName: existingEntry.serverName }, }); - continue; } seenInThisCall.set(qualified, tool.name); const wrapped: ExecutableTool = { @@ -286,8 +292,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 +302,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..12dea56d5 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,132 @@ 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 alphaExec!.resolveExecution({}).execute({ + 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 betaExec!.resolveExecution({}).execute({ + 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('does not write set_active_tools records when registering an MCP server', async () => { const calls: unknown[] = []; const tm = new ToolManager(fakeAgent(calls)); From a5c580ddeb8532b22b574b55a2fd07a73b5f119c Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 17:25:12 +0100 Subject: [PATCH 2/6] fix(agent-core): fix typecheck error in MCP collision test and add changeset - Use executeTool helper instead of direct resolveExecution().execute() to handle Promise return type - Add changeset for agent-core and kimi-code patch bumps --- .changeset/fix-mcp-tool-name-collision.md | 7 +++++++ packages/agent-core/test/mcp/tool-manager-mcp.test.ts | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 .changeset/fix-mcp-tool-name-collision.md 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/test/mcp/tool-manager-mcp.test.ts b/packages/agent-core/test/mcp/tool-manager-mcp.test.ts index 12dea56d5..a4e48d4ae 100644 --- a/packages/agent-core/test/mcp/tool-manager-mcp.test.ts +++ b/packages/agent-core/test/mcp/tool-manager-mcp.test.ts @@ -285,7 +285,7 @@ describe('ToolManager MCP integration', () => { // Call the original tool. const alphaExec = tm.loopTools.find((t) => t.name === 'mcp__alpha_beta__query'); expect(alphaExec).toBeDefined(); - const alphaResult = await alphaExec!.resolveExecution({}).execute({ + const alphaResult = await executeTool(alphaExec!, { turnId: '0', toolCallId: 'c1', args: {}, @@ -296,7 +296,7 @@ describe('ToolManager MCP integration', () => { // Call the disambiguated tool. const betaExec = tm.loopTools.find((t) => t.name === 'mcp__alpha_beta__query__2'); expect(betaExec).toBeDefined(); - const betaResult = await betaExec!.resolveExecution({}).execute({ + const betaResult = await executeTool(betaExec!, { turnId: '0', toolCallId: 'c2', args: {}, From 1e1c2379f5afdca0ee9b97710c0eb0cf3d902471 Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 19:44:16 +0100 Subject: [PATCH 3/6] fix(agent-core): truncate disambiguated MCP tool names exceeding MAX_QUALIFIED_LENGTH When a disambiguated name (base + __N suffix) would exceed the 64-char limit enforced by LLM providers, truncate the base name to fit. This prevents API errors from providers that reject tool names over 64 chars. Also added test verifying the length constraint is respected. --- packages/agent-core/src/agent/tool/index.ts | 7 +++- .../test/mcp/tool-manager-mcp.test.ts | 38 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/packages/agent-core/src/agent/tool/index.ts b/packages/agent-core/src/agent/tool/index.ts index e651b3ca3..e8dbcad1c 100644 --- a/packages/agent-core/src/agent/tool/index.ts +++ b/packages/agent-core/src/agent/tool/index.ts @@ -163,7 +163,12 @@ export class ToolManager { // so both tools remain accessible. const count = (this.mcpCollisionCount.get(qualified) ?? 1) + 1; this.mcpCollisionCount.set(qualified, count); - qualified = `${qualified}__${String(count)}`; + // Truncate the base name if the suffix would exceed the max length. + const suffix = `__${String(count)}`; + const maxBase = 64 - suffix.length; + qualified = qualified.length > maxBase + ? `${qualified.slice(0, maxBase)}${suffix}` + : `${qualified}${suffix}`; collisions.push({ qualified, toolName: tool.name, 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 a4e48d4ae..94dd65c41 100644 --- a/packages/agent-core/test/mcp/tool-manager-mcp.test.ts +++ b/packages/agent-core/test/mcp/tool-manager-mcp.test.ts @@ -348,6 +348,44 @@ describe('ToolManager MCP integration', () => { 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('does not write set_active_tools records when registering an MCP server', async () => { const calls: unknown[] = []; const tm = new ToolManager(fakeAgent(calls)); From cfc73e2a22f1fec8646526e1ba06fab123609508 Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 19:53:33 +0100 Subject: [PATCH 4/6] fix(agent-core): probe for uniqueness after truncation in MCP tool disambiguation When truncating a disambiguated name to fit MAX_QUALIFIED_LENGTH, the resulting name might already be registered by another tool. Now probes by incrementing the suffix count until a unique name is found. Fixes P2 from Codex review: two long colliding names could truncate to the same prefix+suffix, causing the latter to silently overwrite the former. --- packages/agent-core/src/agent/tool/index.ts | 21 +++++--- .../test/mcp/tool-manager-mcp.test.ts | 53 +++++++++++++++++++ 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/packages/agent-core/src/agent/tool/index.ts b/packages/agent-core/src/agent/tool/index.ts index e8dbcad1c..751dd985b 100644 --- a/packages/agent-core/src/agent/tool/index.ts +++ b/packages/agent-core/src/agent/tool/index.ts @@ -161,14 +161,21 @@ export class ToolManager { if (existingEntry !== undefined) { // Cross-server collision: disambiguate by appending a numeric suffix // so both tools remain accessible. - const count = (this.mcpCollisionCount.get(qualified) ?? 1) + 1; + 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); - // Truncate the base name if the suffix would exceed the max length. - const suffix = `__${String(count)}`; - const maxBase = 64 - suffix.length; - qualified = qualified.length > maxBase - ? `${qualified.slice(0, maxBase)}${suffix}` - : `${qualified}${suffix}`; + qualified = disambiguated; collisions.push({ qualified, toolName: tool.name, 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 94dd65c41..d4c2556bf 100644 --- a/packages/agent-core/test/mcp/tool-manager-mcp.test.ts +++ b/packages/agent-core/test/mcp/tool-manager-mcp.test.ts @@ -386,6 +386,59 @@ describe('ToolManager MCP integration', () => { 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__*']); + + // Manually register a tool with a name that would be the truncated+__2 form. + // Base name: "mcp__x__" + "a".repeat(52) = 63 chars. Truncated to 62 + "__2" = 64. + const preRegisterName = 'mcp__x__' + 'a'.repeat(52) + '__2'; + tm.mcpTools.set(preRegisterName, { + tool: { name: preRegisterName, description: 'pre-registered', parameters: {} }, + serverName: 'pre', + }); + + // Now register two servers whose colliding tool would truncate to the same name. + // The disambiguation should probe and find __3 instead of __2. + const baseName = 'mcp__x__' + 'a'.repeat(52); + const firstClient: MCPClient = { + async listTools() { + return [{ name: 'tool', description: 'first', inputSchema: { type: 'object', properties: {} } }]; + }, + async callTool() { return { content: [], isError: false }; }, + }; + const secondClient: MCPClient = { + async listTools() { + return [{ name: 'tool', description: 'second', inputSchema: { type: 'object', properties: {} } }]; + }, + async callTool() { return { content: [], isError: false }; }, + }; + + // Force the base qualified name to match by using server/tool names that produce it. + // "x" server, "tool" tool → "mcp__x__tool" (too short). Instead, register directly. + // Simpler: just verify the probing works by registering the same base twice. + tm.mcpTools.clear(); + tm.mcpToolsByServer.clear(); + // First server registers "mcp__x__tool" (short name, no truncation needed). + tm.mcpTools.set('mcp__x__tool', { + tool: { name: 'mcp__x__tool', description: 'first', parameters: {} }, + serverName: 's1', + }); + tm.mcpToolsByServer.set('s1', ['mcp__x__tool']); + + // Pre-register "mcp__x__tool__2" to force probing to __3. + tm.mcpTools.set('mcp__x__tool__2', { + tool: { name: 'mcp__x__tool__2', description: 'existing', parameters: {} }, + serverName: 'pre', + }); + + // Now trigger a collision by registering a server that collides on "mcp__x__tool". + // Since "mcp__x__tool__2" is taken, the probe should skip to __3. + const r2 = tm.registerMcpServer('x', secondClient, await discoverTools(secondClient)); + // The disambiguation should have probed past __2 and used __3. + expect(r2.registered).toContain('mcp__x__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)); From 71bbcbbf5188e569dd2a7e50e252dcdfae0b6d5c Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 21:37:57 +0100 Subject: [PATCH 5/6] fix(agent-core): stabilize MCP tool names across server reconnections When a server re-registers after a reconnect, reset the collision count for its base tool names so the disambiguated suffix numbers stay stable. This prevents saved approvals, pending tool references, and model context from becoming stale after normal reconnect flows. Also fixes typecheck error by rewriting the probing test to use only public API (no direct access to protected mcpTools map). --- packages/agent-core/src/agent/tool/index.ts | 15 +++++ .../test/mcp/tool-manager-mcp.test.ts | 60 ++++++------------- 2 files changed, 33 insertions(+), 42 deletions(-) diff --git a/packages/agent-core/src/agent/tool/index.ts b/packages/agent-core/src/agent/tool/index.ts index 751dd985b..ab620d1e6 100644 --- a/packages/agent-core/src/agent/tool/index.ts +++ b/packages/agent-core/src/agent/tool/index.ts @@ -41,6 +41,8 @@ export class ToolManager { 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; @@ -143,6 +145,7 @@ 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) { @@ -158,6 +161,7 @@ export class ToolManager { continue; } const existingEntry = this.mcpTools.get(qualified); + const baseName = qualified; if (existingEntry !== undefined) { // Cross-server collision: disambiguate by appending a numeric suffix // so both tools remain accessible. @@ -206,8 +210,10 @@ export class ToolManager { }; this.mcpTools.set(qualified, { tool: wrapped, serverName }); qualifiedNames.push(qualified); + baseNames.push(baseName); } this.mcpToolsByServer.set(serverName, qualifiedNames); + this.mcpServerToolBases.set(serverName, baseNames); return { registered: qualifiedNames, collisions }; } @@ -218,6 +224,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; } 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 d4c2556bf..3aab77468 100644 --- a/packages/agent-core/test/mcp/tool-manager-mcp.test.ts +++ b/packages/agent-core/test/mcp/tool-manager-mcp.test.ts @@ -390,53 +390,29 @@ describe('ToolManager MCP integration', () => { const tm = new ToolManager(fakeAgent()); tm.setActiveTools(['mcp__*']); - // Manually register a tool with a name that would be the truncated+__2 form. - // Base name: "mcp__x__" + "a".repeat(52) = 63 chars. Truncated to 62 + "__2" = 64. - const preRegisterName = 'mcp__x__' + 'a'.repeat(52) + '__2'; - tm.mcpTools.set(preRegisterName, { - tool: { name: preRegisterName, description: 'pre-registered', parameters: {} }, - serverName: 'pre', - }); - - // Now register two servers whose colliding tool would truncate to the same name. - // The disambiguation should probe and find __3 instead of __2. - const baseName = 'mcp__x__' + 'a'.repeat(52); - const firstClient: MCPClient = { - async listTools() { - return [{ name: 'tool', description: 'first', inputSchema: { type: 'object', properties: {} } }]; - }, - async callTool() { return { content: [], isError: false }; }, - }; - const secondClient: MCPClient = { + // 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: 'second', inputSchema: { type: 'object', properties: {} } }]; + return [{ name: 'tool', description: desc, inputSchema: { type: 'object', properties: {} } }]; }, - async callTool() { return { content: [], isError: false }; }, - }; - - // Force the base qualified name to match by using server/tool names that produce it. - // "x" server, "tool" tool → "mcp__x__tool" (too short). Instead, register directly. - // Simpler: just verify the probing works by registering the same base twice. - tm.mcpTools.clear(); - tm.mcpToolsByServer.clear(); - // First server registers "mcp__x__tool" (short name, no truncation needed). - tm.mcpTools.set('mcp__x__tool', { - tool: { name: 'mcp__x__tool', description: 'first', parameters: {} }, - serverName: 's1', + async callTool() { return { content: [{ type: 'text', text: desc }], isError: false }; }, }); - tm.mcpToolsByServer.set('s1', ['mcp__x__tool']); - // Pre-register "mcp__x__tool__2" to force probing to __3. - tm.mcpTools.set('mcp__x__tool__2', { - tool: { name: 'mcp__x__tool__2', description: 'existing', parameters: {} }, - serverName: 'pre', - }); + tm.registerMcpServer('srv a', makeClient('first'), await discoverTools(makeClient('first'))); + const r2 = tm.registerMcpServer('srv__a', makeClient('second'), await discoverTools(makeClient('second'))); - // Now trigger a collision by registering a server that collides on "mcp__x__tool". - // Since "mcp__x__tool__2" is taken, the probe should skip to __3. - const r2 = tm.registerMcpServer('x', secondClient, await discoverTools(secondClient)); - // The disambiguation should have probed past __2 and used __3. - expect(r2.registered).toContain('mcp__x__tool__3'); + // 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 () => { From c159bf212714a7faaff0433cb04795bd10d7829e Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 21:59:09 +0100 Subject: [PATCH 6/6] fix(agent-core): track same-server duplicates by base name before disambiguation When a server has same-server tool name duplicates AND the base name collides with another server, the duplicate check must use the base name (before suffix), not the disambiguated name. Otherwise the second same-server duplicate slips through with a different suffix instead of being reported as a same-server collision. --- packages/agent-core/src/agent/tool/index.ts | 24 +++++++++++---------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/agent-core/src/agent/tool/index.ts b/packages/agent-core/src/agent/tool/index.ts index ab620d1e6..9b7544c34 100644 --- a/packages/agent-core/src/agent/tool/index.ts +++ b/packages/agent-core/src/agent/tool/index.ts @@ -150,7 +150,9 @@ export class ToolManager { const seenInThisCall = new Map(); for (const tool of tools) { if (enabledTools !== undefined && !enabledTools.has(tool.name)) continue; - let qualified = qualifyMcpToolName(serverName, tool.name); + 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({ @@ -160,8 +162,9 @@ export class ToolManager { }); continue; } + seenInThisCall.set(qualified, tool.name); const existingEntry = this.mcpTools.get(qualified); - const baseName = qualified; + let finalName = qualified; if (existingEntry !== undefined) { // Cross-server collision: disambiguate by appending a numeric suffix // so both tools remain accessible. @@ -179,21 +182,20 @@ export class ToolManager { } while (this.mcpTools.has(disambiguated)); count--; this.mcpCollisionCount.set(qualified, count); - qualified = disambiguated; + finalName = disambiguated; collisions.push({ - qualified, + qualified: finalName, toolName: tool.name, collidesWith: { kind: 'other_server', serverName: existingEntry.serverName }, }); } - 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 @@ -203,14 +205,14 @@ 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); - baseNames.push(baseName); + this.mcpTools.set(finalName, { tool: wrapped, serverName }); + qualifiedNames.push(finalName); + baseNames.push(qualified); } this.mcpToolsByServer.set(serverName, qualifiedNames); this.mcpServerToolBases.set(serverName, baseNames);