fix(core): keep uniquely named MCP tools when names collide (#15757)

* fix(core): keep MCP tools when a server returns duplicate names

@google/genai aborts the whole tools/list conversion on a single
duplicated declaration, so discovery discarded every tool of the server
and the caller reported "No prompts, tools, or resources found".

Keep the first declaration of each duplicated name in the discovery
adapter, log the skipped names, and let the rest of the server register.

* fix(core): drop every declaration of a duplicated MCP tool name

Two tools that share a name can still be unrelated commands, so advertising
one of them shows the model a schema `tools/call` need not execute. Count
declarations over the complete paginated listing and skip every declaration of
a colliding name, keeping uniquely named tools and reporting the group size.

The traversal now runs inside the adapter call, which also resets the
bookkeeping on a retry and terminates on a repeated cursor instead of letting
the SDK pager spin on pages filtered down to nothing.

* fix(core): bound MCP tool pagination and end the chain on any falsy cursor

A server that mints a fresh cursor per call never repeats one, so the
duplicate filter's walk had no exit; cap it at the 100-tool ceiling the
genai pager used. An empty-string cursor is the pager's end-of-chain
signal, so treat it as terminal instead of re-fetching the last page as
duplicates. Warn when a repeated cursor truncates the listing, and keep an
all-collision server from being reported as an empty one.

---------

Co-authored-by: 肖洪进 <[email protected]>
Co-authored-by: Shaojin Wen <[email protected]>

packages/core/src/tools/mcp-client.test.ts

@@ -58,6 +58,7 @@ import {
   isEnabled,
   listMcpPrompts,
   listMcpResources,
+  MCP_MAX_TOOL_LIST_PAGES,
   MCPServerStatus,
   McpClient,
   mcpServerRequiresOAuth,
@@ -162,6 +163,55 @@ function mockToolDecls(...decls: Array<string | Obj>) {
   } as unknown as GenAiLib.CallableTool);
 }
 
+/**
+ * Makes `mcpToTool().tool()` convert the adapter the way @google/genai does:
+ * follow `nextCursor` until it is falsy, then reject a duplicated name. The
+ * page cap turns an adapter that never terminates into a failure instead of a
+ * hang, and makes the adapter's terminal page (`nextCursor: undefined`)
+ * load-bearing — a live cursor here re-enters the adapter and duplicates every
+ * declaration.
+ */
+function mockGenAiDiscovery() {
+  const maxPages = 20;
+  const convert = (tools: Array<{ name: string; description?: string }>) => {
+    const names = new Set<string>();
+    for (const mcpTool of tools) {
+      if (names.has(mcpTool.name)) {
+        throw new Error(
+          `Duplicate function name ${mcpTool.name} found in MCP tools. Please ensure function names are unique.`,
+        );
+      }
+      names.add(mcpTool.name);
+    }
+    return {
+      functionDeclarations: tools.map((mcpTool) => ({
+        name: mcpTool.name,
+        description: mcpTool.description,
+      })),
+    };
+  };
+  return vi.mocked(GenAiLib.mcpToTool).mockImplementation(
+    ((client: { listTools: ClientLib.Client['listTools'] }) =>
+      ({
+        tool: async () => {
+          const tools: Array<{ name: string; description?: string }> = [];
+          let cursor: string | undefined;
+          for (let page = 0; page < maxPages; page++) {
+            const { tools: pageTools, nextCursor } = await client.listTools(
+              cursor === undefined ? undefined : { cursor },
+            );
+            tools.push(...pageTools);
+            if (!nextCursor) return convert(tools);
+            cursor = nextCursor;
+          }
+          throw new Error(
+            `listTools did not terminate after ${maxPages} pages`,
+          );
+        },
+      }) as unknown) as typeof GenAiLib.mcpToTool,
+  );
+}
+
 /** An McpClient (stdio `test-command` by default), not in debug mode. */
 function newClient(
   name: string,
@@ -1427,6 +1477,219 @@ lOTTGqPpwFUbw2EMOOpFYuIyzGMIpUNMBjE2gvJiqFQ=
       expect(tools[0].alwaysLoad).toBe(true);
     });
 
+    it('drops every declaration of a duplicated name instead of discarding the server', async () => {
+      const listed = (name: string, description: string) => ({
+        name,
+        description,
+        inputSchema: { type: 'object', properties: {} },
+      });
+      const mockedClient = {
+        listTools: vi.fn().mockResolvedValue({
+          tools: [
+            listed('create', 'create group A'),
+            listed('create', 'create group B'),
+            listed('other', 'other tool'),
+          ],
+        }),
+      } as unknown as ClientLib.Client;
+      // Mirror @google/genai: one duplicated declaration fails the whole
+      // conversion, so the discovery adapter must never hand it a duplicate.
+      mockGenAiDiscovery();
+
+      const tools = await discoverTools(
+        'unity',
+        { command: 'test-command' },
+        mockedClient,
+        cfgWithResources(),
+      );
+
+      // No single command sits behind `tools/call` for a duplicated name, so
+      // none of its declarations may be offered.
+      expect(tools.map((tool) => tool.serverToolName)).toEqual(['other']);
+      expect(mockDebugLogger.warn).toHaveBeenCalledWith(
+        expect.stringContaining("'create' (2 conflicting declarations)"),
+      );
+    });
+
+    it('counts duplicates across pages and requests each cursor once', async () => {
+      const listed = (name: string, description: string) => ({
+        name,
+        description,
+        inputSchema: { type: 'object', properties: {} },
+      });
+      const listTools = vi.fn(async (params?: { cursor?: string }) =>
+        params?.cursor === 'c2'
+          ? {
+              tools: [
+                listed('create', 'create group B'),
+                listed('other', 'other tool'),
+              ],
+            }
+          : {
+              tools: [listed('create', 'create group A')],
+              nextCursor: 'c2',
+            },
+      );
+      mockGenAiDiscovery();
+
+      const tools = await discoverTools(
+        'unity',
+        { command: 'test-command' },
+        { listTools } as unknown as ClientLib.Client,
+        cfgWithResources(),
+      );
+
+      expect(tools.map((tool) => tool.serverToolName)).toEqual(['other']);
+      expect(listTools).toHaveBeenNthCalledWith(1, undefined);
+      expect(listTools).toHaveBeenNthCalledWith(2, { cursor: 'c2' });
+    });
+
+    it('restarts the duplicate bookkeeping when a transient failure retries listing', async () => {
+      const listed = (name: string, description: string) => ({
+        name,
+        description,
+        inputSchema: { type: 'object', properties: {} },
+      });
+      const listTools = vi
+        .fn()
+        .mockResolvedValueOnce({
+          tools: [listed('a', 'a tool')],
+          nextCursor: 'p2',
+        })
+        .mockRejectedValueOnce(
+          Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }),
+        )
+        .mockResolvedValueOnce({
+          tools: [listed('a', 'a tool')],
+          nextCursor: 'p2',
+        })
+        .mockResolvedValueOnce({ tools: [listed('b', 'b tool')] });
+      mockGenAiDiscovery();
+
+      const tools = await discoverTools(
+        'unity',
+        { command: 'test-command' },
+        { listTools } as unknown as ClientLib.Client,
+        cfgWithResources(),
+      );
+
+      // The retry re-reads page 1; its names must not be treated as duplicates
+      // of the failed attempt's.
+      expect(tools.map((tool) => tool.serverToolName)).toEqual(['a', 'b']);
+      expect(listTools).toHaveBeenCalledTimes(4);
+    });
+
+    it('stops paging when a server repeats a cursor', async () => {
+      const listed = (name: string, description: string) => ({
+        name,
+        description,
+        inputSchema: { type: 'object', properties: {} },
+      });
+      const listTools = vi.fn().mockResolvedValue({
+        tools: [listed('a', 'a tool')],
+        nextCursor: 'same',
+      });
+      mockGenAiDiscovery();
+
+      const tools = await discoverTools(
+        'unity',
+        { command: 'test-command' },
+        { listTools } as unknown as ClientLib.Client,
+        cfgWithResources(),
+      );
+
+      // Two declarations of the same name, both dropped; the important part is
+      // that discovery returned at all.
+      expect(tools).toEqual([]);
+      expect(listTools).toHaveBeenCalledTimes(2);
+      // A truncated listing must not look like a complete one.
+      expect(mockDebugLogger.warn).toHaveBeenCalledWith(
+        expect.stringContaining("repeated cursor 'same'"),
+      );
+    });
+
+    it('treats an empty nextCursor as the end of the chain', async () => {
+      const listed = (name: string, description: string) => ({
+        name,
+        description,
+        inputSchema: { type: 'object', properties: {} },
+      });
+      // ListToolsResultSchema accepts `nextCursor: ''`; the pager this walk
+      // replaced stopped on any falsy cursor.
+      const listTools = vi.fn().mockResolvedValue({
+        tools: [listed('a', 'a tool')],
+        nextCursor: '',
+      });
+      mockGenAiDiscovery();
+
+      const tools = await discoverTools(
+        'unity',
+        { command: 'test-command' },
+        { listTools } as unknown as ClientLib.Client,
+        cfgWithResources(),
+      );
+
+      expect(tools.map((tool) => tool.serverToolName)).toEqual(['a']);
+      expect(listTools).toHaveBeenCalledTimes(1);
+    });
+
+    it('bounds the page walk when a server keeps minting new cursors', async () => {
+      const listed = (name: string, description: string) => ({
+        name,
+        description,
+        inputSchema: { type: 'object', properties: {} },
+      });
+      let call = 0;
+      const listTools = vi.fn(async () => {
+        call++;
+        return {
+          tools: [listed(`tool-${call}`, 'a tool')],
+          nextCursor: `c${call}`,
+        };
+      });
+      mockGenAiDiscovery();
+
+      const tools = await discoverTools(
+        'unity',
+        { command: 'test-command' },
+        { listTools } as unknown as ClientLib.Client,
+        cfgWithResources(),
+      );
+
+      // Without the cap this walk never terminates.
+      expect(listTools).toHaveBeenCalledTimes(MCP_MAX_TOOL_LIST_PAGES);
+      expect(tools).toHaveLength(MCP_MAX_TOOL_LIST_PAGES);
+      expect(mockDebugLogger.warn).toHaveBeenCalledWith(
+        expect.stringContaining(`after ${MCP_MAX_TOOL_LIST_PAGES} pages`),
+      );
+    });
+
+    it('reports an all-collision server as discovered instead of empty', async () => {
+      const listed = (name: string, description: string) => ({
+        name,
+        description,
+        inputSchema: { type: 'object', properties: {} },
+      });
+      // Every advertised name collides, so nothing survives the conversion.
+      promptServer([], {
+        listTools: vi.fn().mockResolvedValue({
+          tools: [listed('create', 'create A'), listed('create', 'create B')],
+        }),
+      });
+      mockGenAiDiscovery();
+
+      const client = await connectedClient('all-collision-server');
+      const snapshot = await client.discoverAndReturn(cfgWithResources());
+
+      expect(snapshot.tools).toEqual([]);
+      // Not "No prompts, tools, or resources found" — the server stays
+      // connected, and the collision warning is what tells the user why.
+      expect(client.getStatus()).toBe(MCPServerStatus.CONNECTED);
+      expect(mockDebugLogger.warn).toHaveBeenCalledWith(
+        expect.stringContaining("'create' (2 conflicting declarations)"),
+      );
+    });
+
     it('preserves App-only tools for the separate App registry', async () => {
       const appTool = (
         name: string,

packages/core/src/tools/mcp-client.ts

@@ -106,6 +106,12 @@ export const MCP_VERSION_NEGOTIATION_FALLBACK_HEADROOM_MS =
   MCP_VERSION_NEGOTIATION_PROBE_TIMEOUT_MS;
 export const MCP_APPS_EXTENSION_ID = 'io.modelcontextprotocol/ui';
 export const MCP_APP_RESOURCE_MIME_TYPE = 'text/html;profile=mcp-app';
+/**
+ * Upper bound on `tools/list` round-trips for one server. The @google/genai
+ * pager this walk replaced stopped after 100 tools, so a 100-page ceiling is
+ * never stricter for a server that returns at least one tool per page.
+ */
+export const MCP_MAX_TOOL_LIST_PAGES = 100;
 
 const debugLogger = createDebugLogger('MCP');
 const AUTOMATIC_MCP_OAUTH_TIMEOUT_MS = 60_000;
@@ -1661,22 +1667,83 @@ async function discoverToolsWithMetadata(
     const listedTools: Array<
       Awaited<ReturnType<Client['listTools']>>['tools'][number]
     > = [];
+    let collidedToolNames: string[] = [];
     let didListTools = false;
-    const listTools = async (params?: { cursor?: string }) => {
-      // Clients without getProtocolEra (tests and the genai adapter) keep
-      // the typed helper. Modern sessions use it only when `tools` is
-      // declared; otherwise the v2 helper returns [] without a wire request.
-      const result =
-        typeof mcpClient.getProtocolEra !== 'function' ||
-        canUseModernTypedHelper(mcpClient, 'tools')
-          ? await mcpClient.listTools(params)
-          : await mcpClient.request(
-              { method: 'tools/list', params: params ?? {} },
-              ListToolsResultSchema,
-            );
+    // Clients without getProtocolEra (tests and the genai adapter) keep the
+    // typed helper. Modern sessions use it only when `tools` is declared;
+    // otherwise the v2 helper returns [] without a wire request.
+    const requestTools = async (params?: { cursor?: string }) =>
+      typeof mcpClient.getProtocolEra !== 'function' ||
+      canUseModernTypedHelper(mcpClient, 'tools')
+        ? await mcpClient.listTools(params)
+        : await mcpClient.request(
+            { method: 'tools/list', params: params ?? {} },
+            ListToolsResultSchema,
+          );
+    // Walk the whole cursor chain inside one call: the duplicate counts have to
+    // span every page, and a retry (which restarts at the first page) must
+    // start from empty bookkeeping. @google/genai is then handed a single
+    // terminal page — its pager terminates by counting yielded tools, so a page
+    // filtered down to nothing would leave it issuing tools/list forever when a
+    // server repeats a cursor.
+    const listTools = async () => {
+      const tools: typeof listedTools = [];
+      const declarationCounts = new Map<string, number>();
+      const seenCursors = new Set<string>();
+      let pages = 0;
+      let cursor: string | undefined;
+      do {
+        if (pages >= MCP_MAX_TOOL_LIST_PAGES) {
+          // A server that mints a fresh cursor per call never repeats one and
+          // never terminates the chain, so bound the walk itself.
+          debugLogger.warn(
+            `MCP server '${mcpServerName}' stopped paginating tools/list ` +
+              `after ${MCP_MAX_TOOL_LIST_PAGES} pages.`,
+          );
+          break;
+        }
+        pages++;
+        const result = await requestTools(
+          cursor === undefined ? undefined : { cursor },
+        );
+        for (const tool of result.tools) {
+          tools.push(tool);
+          declarationCounts.set(
+            tool.name,
+            (declarationCounts.get(tool.name) ?? 0) + 1,
+          );
+        }
+        const next = result.nextCursor;
+        // @google/genai's pager treats a falsy cursor as the end of the chain,
+        // so `''` is terminal here too — otherwise the last page is fetched
+        // twice and every tool on it is counted as a duplicate.
+        if (!next) break;
+        // A server that ignores the cursor would otherwise loop here forever.
+        if (seenCursors.has(next)) {
+          debugLogger.warn(
+            `MCP server '${mcpServerName}' repeated cursor '${next}' while ` +
+              `listing tools; stopping after ${tools.length} declarations.`,
+          );
+          break;
+        }
+        seenCursors.add(next);
+        cursor = next;
+      } while (cursor !== undefined);
       didListTools = true;
-      listedTools.push(...result.tools);
-      return result;
+      listedTools.length = 0;
+      listedTools.push(...tools);
+      collidedToolNames = [...declarationCounts]
+        .filter(([, count]) => count > 1)
+        .map(
+          ([name, count]) => `'${name}' (${count} conflicting declarations)`,
+        );
+      // A duplicated name has no single command behind `tools/call`, so no
+      // declaration of it can be advertised safely: drop the whole group
+      // rather than electing an arbitrary one (#15741).
+      return {
+        tools: tools.filter((tool) => declarationCounts.get(tool.name) === 1),
+        nextCursor: undefined,
+      };
     };
     // @google/genai still types this structural adapter against SDK v1. Its
     // discovery path only calls listTools(); execution stays on the v2 client.
@@ -1696,6 +1763,14 @@ async function discoverToolsWithMetadata(
       return { tools: [], hadVisibilityFilteredTools: false };
     }
 
+    if (collidedToolNames.length > 0) {
+      debugLogger.warn(
+        `MCP server '${mcpServerName}' returned duplicate tool names ` +
+          `${collidedToolNames.join(', ')}. Skipping every declaration of ` +
+          `each duplicated name.`,
+      );
+    }
+
     // Fetch raw tool list from MCP client to get annotations (readOnlyHint, etc.)
     // that are not preserved by mcpToTool's functionDeclarations conversion.
     const annotationsMap = new Map<string, McpToolAnnotations>();
@@ -1789,7 +1864,14 @@ async function discoverToolsWithMetadata(
         );
       }
     }
-    return { tools: discoveredTools, hadVisibilityFilteredTools };
+    // A server whose every declaration collided is not an empty server: the
+    // caller must not report it as "found nothing" and mark it DISCONNECTED,
+    // since the warning above already named the colliding tools.
+    return {
+      tools: discoveredTools,
+      hadVisibilityFilteredTools:
+        hadVisibilityFilteredTools || collidedToolNames.length > 0,
+    };
   } catch (error) {
     if (!isMethodNotFound(error)) {
       debugLogger.error(