From 594305ce19765086472c6ef95c09c3f2a70ce37c Mon Sep 17 00:00:00 2001 From: gengruilin Date: Tue, 8 Sep 2026 23:33:23 +0800 Subject: [PATCH] fix(mcp): reject repeated tool discovery cursors Refs: https://github.com/deepseek-ai/deepseek-harness/discussions/3660 --- .../2026-07-07-mcp-client-plugin.i18n.yaml | 4 +- .../feature/2026-07-07-mcp-client-plugin.md | 4 +- .../2026-07-07-mcp-client-plugin.zh.md | 4 +- .../expected/mcp-pagination/stderr-cause.txt | 1 + .../tests/mcp-pagination.expected.e2e.ts | 30 ++++++++++++ packages/mcp/mcp-client/README.i18n.yaml | 4 +- packages/mcp/mcp-client/README.md | 1 + packages/mcp/mcp-client/README.zh.md | 1 + packages/mcp/mcp-client/src/tools.ts | 13 ++++- packages/mcp/mcp-client/tests/apply.spec.ts | 47 +++++++++++++++++++ .../tests/fixtures/repeated-cursor-server.ts | 18 +++++++ .../tests/fixtures/repeated-cursor.patch.yml | 17 +++++++ .../mcp/mcp-client/tests/mcp-client.spec.ts | 44 +++++++++++++++++ 13 files changed, 180 insertions(+), 8 deletions(-) create mode 100644 apps/cli/tests/profiles/headless/tests/expected/mcp-pagination/stderr-cause.txt create mode 100644 apps/cli/tests/profiles/headless/tests/mcp-pagination.expected.e2e.ts create mode 100644 packages/mcp/mcp-client/tests/fixtures/repeated-cursor-server.ts create mode 100644 packages/mcp/mcp-client/tests/fixtures/repeated-cursor.patch.yml diff --git a/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.i18n.yaml b/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.i18n.yaml index 67227acc29..c5435129be 100644 --- a/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.i18n.yaml +++ b/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write .agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.md -2026-07-07-mcp-client-plugin.md: 1c451e14ea2a7c494454956ce9bb09d97d4e1e1d -2026-07-07-mcp-client-plugin.zh.md: 78a086d6c218a254e850777fe4024973e8848e17 +2026-07-07-mcp-client-plugin.md: 91c1bf428973eae7935e9c2a14dda7028a734313 +2026-07-07-mcp-client-plugin.zh.md: c0c2dbae5b854734240f8035edfc8d0b5a33408f diff --git a/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.md b/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.md index 1c451e14ea..91c1bf4289 100644 --- a/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.md +++ b/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.md @@ -96,12 +96,14 @@ Every MCP tool has two names: This server-qualified shape is the de-facto standard among multi-server agent clients — every surveyed end-user product qualifies MCP tools by server ([Claude Code](https://code.claude.com/docs/en/agent-sdk/mcp#tool-naming-convention) `mcp__github__list_issues`, [Codex](https://openai.com/index/unrolling-the-codex-agent-loop/) `mcp__weather__get-forecast`, [Gemini CLI](https://geminicli.com/docs/tools/mcp-server/#3-tool-naming-and-namespaces), [VS Code](https://github.com/microsoft/vscode/blob/ab9ec62c6a61e429a9abd612ff220c3f4834c9ea/src/vs/workbench/contrib/mcp/common/mcpServer.ts#L217-L260), [Cline](https://github.com/cline/cline/blob/52fdbb1d72f7324a28142a7ba7678d4b53c902f4/sdk/packages/core/src/extensions/mcp/name-transform.ts#L20-L35), [Roo Code](https://github.com/RooCodeInc/Roo-Code/blob/b867ec9145750d0ae1ff7f02d35406e9bf2a0b16/src/utils/mcp-name.ts#L117-L140), [Goose](https://github.com/block/goose/blob/b3a012cbdde854b0fe14f95b1c48543bf6517c0a/crates/goose/src/agents/extension_manager.rs#L1391-L1441), [OpenCode](https://github.com/anomalyco/opencode/blob/d199b1bff90282a4f9cd6251b5fc7b16875a52f6/packages/opencode/src/mcp/catalog.ts#L117-L120)); the exact `mcp____` spelling follows Claude Code and Codex. The `mcp__` marker keeps MCP registrations out of the native tools' namespace and gives permission/telemetry rules a stable shape (`mcp__*`, `mcp__github__*`). -1. On connect: drain `client.listTools()` pagination, derive every tool's `publicName`, then register each as a raw `ToolDefinition` via `ctx.tools.register()`. The MCP JSON Schema and description pass through unchanged (no `defineTool` DSL conversion); only the model-facing `name` is replaced. +1. On connect: drain uncached `tools/list` pagination, derive every tool's `publicName`, then register each as a raw `ToolDefinition` via `ctx.tools.register()`. The MCP JSON Schema and description pass through unchanged (no `defineTool` DSL conversion); only the model-facing `name` is replaced. 2. Listen for `notifications/tools/list_changed` → re-run the same sync (dispose previous generation, register new). Deterministic names mean unchanged tools keep their names across re-syncs. 3. The executor closes over `rawName`; the public name is never sent to the server and never parsed to recover the raw name. 4. No `presentCall`/`presentResult` — UI consumers use the provider-neutral generic-card fallback. 5. Tools are transparent in the system prompt — no "[via MCP]" annotation beyond the name itself. +Each synchronization rejects a repeated non-empty continuation cursor before requesting another page, retaining the previous tool generation. Empty pages cannot establish progress through tool-name uniqueness, so cursor history also detects cycles spanning several pages ([reported failure](https://github.com/deepseek-ai/deepseek-harness/discussions/3660)). Cursor history belongs to one synchronization: a later update may reuse the same cursors. Focused bridge and lifecycle tests cover cycle rejection, retained callable tools, strict startup failure, and notification recovery. This detects repeated cursors; it does not bound a server that continually returns distinct cursors. + ### Public name normalization MCP allows tool names up to 128 characters including `.`; the DeepSeek function-name contract allows `[A-Za-z0-9_-]` and at most 64. Public names are normalized deterministically: invalid characters become `_`, and when replacement or truncation changed the name, a 12-hex-char SHA-256 hash of the `(serverName, rawName)` identity is appended so distinct MCP identities can never collapse into the same public name: diff --git a/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.zh.md b/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.zh.md index 78a086d6c2..c0c2dbae5b 100644 --- a/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.zh.md +++ b/.agents/notes/implemented/feature/2026-07-07-mcp-client-plugin.zh.md @@ -96,12 +96,14 @@ type Config = StdioConfig | StreamableHttpConfig 这种按服务器限定的形式是多服务器 agent 客户端事实上的标准——所有被调研的终端用户产品都按服务器限定 MCP 工具名（[Claude Code](https://code.claude.com/docs/en/agent-sdk/mcp#tool-naming-convention) `mcp__github__list_issues`、[Codex](https://openai.com/index/unrolling-the-codex-agent-loop/) `mcp__weather__get-forecast`、[Gemini CLI](https://geminicli.com/docs/tools/mcp-server/#3-tool-naming-and-namespaces)、[VS Code](https://github.com/microsoft/vscode/blob/ab9ec62c6a61e429a9abd612ff220c3f4834c9ea/src/vs/workbench/contrib/mcp/common/mcpServer.ts#L217-L260)、[Cline](https://github.com/cline/cline/blob/52fdbb1d72f7324a28142a7ba7678d4b53c902f4/sdk/packages/core/src/extensions/mcp/name-transform.ts#L20-L35)、[Roo Code](https://github.com/RooCodeInc/Roo-Code/blob/b867ec9145750d0ae1ff7f02d35406e9bf2a0b16/src/utils/mcp-name.ts#L117-L140)、[Goose](https://github.com/block/goose/blob/b3a012cbdde854b0fe14f95b1c48543bf6517c0a/crates/goose/src/agents/extension_manager.rs#L1391-L1441)、[OpenCode](https://github.com/anomalyco/opencode/blob/d199b1bff90282a4f9cd6251b5fc7b16875a52f6/packages/opencode/src/mcp/catalog.ts#L117-L120)）；`mcp____` 的拼写方式与 Claude Code 和 Codex 一致。`mcp__` 前缀将 MCP 注册与原生工具的命名空间隔离，并为权限/遥测规则提供稳定的匹配模式（`mcp__*`、`mcp__github__*`）。 -1. 连接时：遍历 `client.listTools()` 的分页结果，推导每个工具的 `publicName`，然后通过 `ctx.tools.register()` 将其注册为原始 `ToolDefinition`。MCP 的 JSON Schema 和描述原样透传（不做 `defineTool` DSL 转换）；仅替换模型可见的 `name`。 +1. 连接时：遍历未缓存的 `tools/list` 分页结果，推导每个工具的 `publicName`，然后通过 `ctx.tools.register()` 将其注册为原始 `ToolDefinition`。MCP 的 JSON Schema 和描述原样透传（不做 `defineTool` DSL 转换）；仅替换模型可见的 `name`。 2. 监听 `notifications/tools/list_changed` → 重新执行同步（dispose 上一代、注册新一代）。确定性命名意味着未变化的工具在重新同步后保持原名。 3. 执行器闭包持有 `rawName`；公开名称永远不发送给服务器，也永远不被解析以还原原始名称。 4. 无 `presentCall`/`presentResult`——UI 消费方使用提供方无关的通用卡片兜底。 5. 工具在系统提示词中是透明的——除名称本身外不附加「[via MCP]」标注。 +每次同步在请求下一页前拒绝重复的非空续传游标，并保留上一代工具。空页无法通过工具名唯一性证明分页在前进，因此游标记录还会检测跨越多页的循环（[问题报告](https://github.com/deepseek-ai/deepseek-harness/discussions/3660)）。游标记录只属于一次同步：后续更新可以复用相同游标。定向桥接与生命周期测试覆盖循环拒绝、保留可调用工具、严格启动失败及通知恢复。此机制检测重复游标；它不限制持续返回不同游标的服务器。 + ### 公开名称规范化 MCP 允许工具名最长 128 字符且可包含 `.`；DeepSeek 的函数名约定允许 `[A-Za-z0-9_-]` 且最多 64 字符。公开名称按确定性规则规范化：非法字符替换为 `_`，当替换或截断改变了名称时，追加 `(serverName, rawName)` 标识的 12 位十六进制 SHA-256 hash，确保不同的 MCP 标识永远不会坍缩为同一个公开名称： diff --git a/apps/cli/tests/profiles/headless/tests/expected/mcp-pagination/stderr-cause.txt b/apps/cli/tests/profiles/headless/tests/expected/mcp-pagination/stderr-cause.txt new file mode 100644 index 0000000000..19a6f28bec --- /dev/null +++ b/apps/cli/tests/profiles/headless/tests/expected/mcp-pagination/stderr-cause.txt @@ -0,0 +1 @@ +Error: mcp-client(pagination-cycle): server repeated a tools/list continuation cursor — invalid tool list diff --git a/apps/cli/tests/profiles/headless/tests/mcp-pagination.expected.e2e.ts b/apps/cli/tests/profiles/headless/tests/mcp-pagination.expected.e2e.ts new file mode 100644 index 0000000000..e9e08bb302 --- /dev/null +++ b/apps/cli/tests/profiles/headless/tests/mcp-pagination.expected.e2e.ts @@ -0,0 +1,30 @@ +/** Startup diagnostics through the shipped headless profile and real MCP stdio transport. */ + +import { fileURLToPath } from 'node:url' +import { expect, it } from 'vitest' +import { LOADER_SMOKE_TEST_TIMEOUT_MS, runLoaderSmoke } from '@deepseek-ai/dsh-loader-smoke' + +const fixtureRoot = new URL('../../../../../../packages/mcp/mcp-client/tests/fixtures/', import.meta.url) +const configPath = fileURLToPath(new URL('repeated-cursor.patch.yml', fixtureRoot)) +const expectedPath = fileURLToPath(new URL('./expected/mcp-pagination/stderr-cause.txt', import.meta.url)) + +it('reports a repeated MCP discovery cursor and exits before starting a turn', async () => { + const { stdout, stderr } = await runLoaderSmoke({ + label: 'MCP discovery pagination cycle', + tempDirPrefix: 'dsh-mcp-pagination-', + binScript: fileURLToPath(new URL('../../../../src/bin.ts', import.meta.url)), + libBinScript: fileURLToPath(new URL('../../../../lib/bin.js', import.meta.url)), + configPath, + binArgs: ['--profile', 'headless', '--patch', configPath, 'unreachable task'], + tsconfigPath: fileURLToPath(new URL('../../../../../../tsconfig.json', import.meta.url)), + expectedExitCode: 1, + env: { + DSH_MCP_PAGINATION_FIXTURE: fileURLToPath(new URL('repeated-cursor-server.ts', fixtureRoot)), + DSH_TELEMETRY_DISABLED: '1', + }, + }) + expect(stdout).toBe('') + expect(stderr).toContain('initial connection or tool synchronization failed') + const cause = stderr.split('\n').find(line => line.startsWith('Error: mcp-client(pagination-cycle):')) + await expect(`${cause}\n`).toMatchFileSnapshot(expectedPath) +}, LOADER_SMOKE_TEST_TIMEOUT_MS) diff --git a/packages/mcp/mcp-client/README.i18n.yaml b/packages/mcp/mcp-client/README.i18n.yaml index 370f6a1e7f..a3f68e19e5 100644 --- a/packages/mcp/mcp-client/README.i18n.yaml +++ b/packages/mcp/mcp-client/README.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write packages/mcp/mcp-client/README.md -README.md: 112f477556d1b8ee2788c9b96251b6969c617f62 -README.zh.md: e642c7e130510cd717342a2e879c26396c77d55d +README.md: de646256888f1c271ad94a4c863338dfb02551e6 +README.zh.md: 2c71060ec309e823ff3456207881e27ab9aca553 diff --git a/packages/mcp/mcp-client/README.md b/packages/mcp/mcp-client/README.md index 112f477556..de64625688 100644 --- a/packages/mcp/mcp-client/README.md +++ b/packages/mcp/mcp-client/README.md @@ -76,6 +76,7 @@ The model sees each tool under a stable server-qualified name: `mcp____` namespace — the partial generation is @@ -149,6 +149,7 @@ export async function syncTools( ): Promise { // Phase 1: fetch and build the next generation without touching the registry. const definitions = new Map() + const seenCursors = new Set() let cursor: string | undefined do { const response = await listToolsUncached(client, cursor) @@ -172,6 +173,14 @@ export async function syncTools( )) } cursor = response.nextCursor + if (cursor) { + if (seenCursors.has(cursor)) { + throw new Error( + `mcp-client(${opts.serverName}): server repeated a tools/list continuation cursor — invalid tool list`, + ) + } + seenCursors.add(cursor) + } } while (cursor) // Phase 2: swap generations. diff --git a/packages/mcp/mcp-client/tests/apply.spec.ts b/packages/mcp/mcp-client/tests/apply.spec.ts index 1b1716e51b..5dee651d80 100644 --- a/packages/mcp/mcp-client/tests/apply.spec.ts +++ b/packages/mcp/mcp-client/tests/apply.spec.ts @@ -303,6 +303,28 @@ describe('apply (plugin lifecycle)', () => { expect(mockClose).toHaveBeenCalled() }) + it('rejects strict startup on a repeated discovery cursor and closes the client', async () => { + mockListTools + .mockResolvedValueOnce({ tools: [], nextCursor: 'cursor1' }) + .mockResolvedValueOnce({ tools: [], nextCursor: 'cursor1' }) + .mockRejectedValue(new Error('pagination continued after the repeated cursor')) + try { + await expect(apply(ctx, { + ...stdioConfig, + failOnStartupError: true, + reconnect: { enabled: false }, + })).rejects.toMatchObject({ + message: 'mcp-client(srv): initial connection or tool synchronization failed', + cause: new Error('mcp-client(srv): server repeated a tools/list continuation cursor — invalid tool list'), + }) + expect(mockListTools).toHaveBeenCalledTimes(2) + expect(mockClose).toHaveBeenCalledTimes(1) + expect(ctx.tools.schemas()).toEqual([]) + } finally { + await ctx.fiber.dispose() + } + }) + it('preserves strict startup registration when list_changed arrives before connect resolves', async () => { ctx.tools.register({ name: 'mcp__srv__remote', @@ -358,6 +380,31 @@ describe('apply (plugin lifecycle)', () => { expect(ctx.tools.get('mcp__srv__remote')).toBeDefined() }) + it('continues notification synchronization after rejecting a pagination cycle', async () => { + try { + await apply(ctx, stdioConfig) + const handler = mockSetNotificationHandler.mock.calls[0]![1] as () => Promise + mockListTools + .mockResolvedValueOnce({ tools: [], nextCursor: 'cursor1' }) + .mockResolvedValueOnce({ tools: [], nextCursor: 'cursor1' }) + .mockRejectedValue(new Error('pagination continued after the repeated cursor')) + + await handler() + expect(mockListTools).toHaveBeenCalledTimes(3) + expect(ctx.tools.get('mcp__srv__remote')).toBeDefined() + + mockListTools + .mockResolvedValueOnce({ tools: [], nextCursor: 'cursor1' }) + .mockResolvedValueOnce({ tools: [{ name: 'updated', inputSchema: { type: 'object' } }], nextCursor: undefined }) + await handler() + expect(ctx.tools.get('mcp__srv__remote')).toBeUndefined() + expect(ctx.tools.get('mcp__srv__updated')).toBeDefined() + } finally { + await ctx.fiber.dispose() + } + expect(mockClose).toHaveBeenCalledTimes(1) + }) + it('effect disposer unregisters the CURRENT generation and closes client', async () => { // Load through ctx.plugin so ONLY the plugin's fiber is disposed — the // registry must survive to observe the unregistration. diff --git a/packages/mcp/mcp-client/tests/fixtures/repeated-cursor-server.ts b/packages/mcp/mcp-client/tests/fixtures/repeated-cursor-server.ts new file mode 100644 index 0000000000..d7c71f6394 --- /dev/null +++ b/packages/mcp/mcp-client/tests/fixtures/repeated-cursor-server.ts @@ -0,0 +1,18 @@ +/** MCP wire fixture returning a repeated discovery cursor over empty pages. */ + +import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js' +import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js' +import { ListToolsRequestSchema } from '@modelcontextprotocol/sdk/types.js' + +const server = new McpServer( + { name: 'repeated-cursor', version: '1.0.0' }, + { capabilities: { tools: {} } }, +) +let requests = 0 +server.server.setRequestHandler(ListToolsRequestSchema, () => { + // Bound the broken-client path without relying on a test timeout to kill it. + if (++requests > 2) throw new Error('pagination continued after the repeated cursor') + return { tools: [], nextCursor: 'same-cursor' } +}) + +await server.connect(new StdioServerTransport()) diff --git a/packages/mcp/mcp-client/tests/fixtures/repeated-cursor.patch.yml b/packages/mcp/mcp-client/tests/fixtures/repeated-cursor.patch.yml new file mode 100644 index 0000000000..1a4d033b5b --- /dev/null +++ b/packages/mcp/mcp-client/tests/fixtures/repeated-cursor.patch.yml @@ -0,0 +1,17 @@ +- id: headless-startup + disabled: true + +- id: headless-runner + disabled: true + +- insert: + - id: mcp-pagination-cycle + name: '@deepseek-ai/dsh-mcp-client' + config: + transport: stdio + serverName: pagination-cycle + command: !!js process.execPath + args: !!js '[process.env.DSH_MCP_PAGINATION_FIXTURE]' + failOnStartupError: true + reconnect: + enabled: false diff --git a/packages/mcp/mcp-client/tests/mcp-client.spec.ts b/packages/mcp/mcp-client/tests/mcp-client.spec.ts index 605a31ece1..929331b98c 100644 --- a/packages/mcp/mcp-client/tests/mcp-client.spec.ts +++ b/packages/mcp/mcp-client/tests/mcp-client.spec.ts @@ -309,6 +309,50 @@ describe('syncTools', () => { expect(ctx.tools.get('mcp__srv__page2')).toBeDefined() }) + it.each([ + ['immediate', ['cursor1', 'cursor1']], + ['multi-page', ['cursor1', 'cursor2', 'cursor1']], + ])('rejects a pagination cycle through empty pages (%s)', async (_kind, cursors) => { + const client = createMockClient([]) + client.listTools.mockRejectedValue(new Error('pagination continued after the repeated cursor')) + for (const nextCursor of cursors) { + client.listTools.mockResolvedValueOnce({ tools: [], nextCursor }) + } + + await expect(syncTools(client as never, ctx, defaultOpts, new Map())) + .rejects.toThrow('mcp-client(srv): server repeated a tools/list continuation cursor — invalid tool list') + expect(client.listTools).toHaveBeenCalledTimes(cursors.length) + expect(ctx.tools.schemas()).toEqual([]) + }) + + it('keeps callable tools after a pagination cycle and accepts a later complete list', async () => { + const client = createMockClient([{ name: 'stable', inputSchema: { type: 'object' } }]) + const previous = await syncTools(client as never, ctx, defaultOpts, new Map()) + const stable = ctx.tools.get('mcp__srv__stable') + client.listTools + .mockResolvedValueOnce({ tools: [{ name: 'partial', inputSchema: { type: 'object' } }], nextCursor: 'cursor1' }) + .mockResolvedValueOnce({ tools: [], nextCursor: 'cursor1' }) + .mockRejectedValue(new Error('pagination continued after the repeated cursor')) + + await expect(syncTools(client as never, ctx, defaultOpts, previous)).rejects.toThrow(/repeated.*cursor/) + expect(client.listTools).toHaveBeenCalledTimes(3) + expect(ctx.tools.get('mcp__srv__stable')).toBe(stable) + expect(ctx.tools.get('mcp__srv__partial')).toBeUndefined() + const result = await ctx.tools.execute({ + signal: testToolSignal, callId: ToolCallId('pagination-retained'), name: 'mcp__srv__stable', arguments: {}, + }) + expect(result.isError).toBe(false) + expect(result.content).toEqual([{ type: 'text', text: 'ok' }]) + + client.listTools + .mockResolvedValueOnce({ tools: [], nextCursor: 'cursor1' }) + .mockResolvedValueOnce({ tools: [{ name: 'recovered', inputSchema: { type: 'object' } }], nextCursor: undefined }) + const recovered = await syncTools(client as never, ctx, defaultOpts, previous) + expect([...recovered.keys()]).toEqual(['mcp__srv__recovered']) + expect(ctx.tools.get('mcp__srv__stable')).toBeUndefined() + expect(client.listTools).toHaveBeenLastCalledWith({ cursor: 'cursor1' }) + }) + it('owns output validation independently of the SDK per-page cache', async () => { const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair() serverTransport.onmessage = (message) => {