From 597f011e666be045e7e34722e757ea92eccf436d Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sun, 17 May 2026 23:31:14 +0800 Subject: [PATCH] fixup(serve): address PR 14 review (#4247) findings 1-7 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Codex + Copilot review feedback on #4247. Seven functional and forward-compat fixes; (8) `tcp` transport mapper vs createTransport deferred pending @wenshao direction (separate core/protocol decision). 1. **Single-server rediscovery bypass** — add `tryReserveSlot` at the top of `discoverMcpToolsForServerInternal`. Pre-fix a server refused at startup could be brought online later via `/mcp reconnect ` and exceed the cap in enforce mode. 2. **Empty `budgets[]` when mode=off** — early `return []` in `buildBudgetCells` when mode is `off`. Protocol docs / SDK types promise empty array; pre-fix emitted a synthetic noisy cell. 3. **runQwenServe validation + env leakage** — mirror CLI budget validation in `runQwenServe` (the embedded entry point); explicitly delete `QWEN_SERVE_MCP_*` env vars when options are undefined so multiple daemons in one process don't leak prior budget config to subsequent ACP children. 4. **Disabled-vs-refused precedence + stale refusal log** — config-disable wins over budget refusal in the per-server cell; `removeServer` + `disconnectServer` drop the entry from `lastRefusedServerNames` so operator action immediately clears the budget tag. 5. **Incremental remove-before-reserve ordering** — process config-removed servers FIRST in `discoverAllMcpToolsIncremental` so freed slots are visible to subsequent `tryReserveSlot` calls. Pre-fix scenario {a,b}→{a,c} with budget=2 wasted a slot. 6. **`scope` forward-compat type widening** — `'workspace' | (string & {})` on both `ServeMcpBudgetStatusCell` and `DaemonMcpBudgetStatusCell` so SDK consumers don't break when PR 23 adds `scope: 'pool'` per the documented no-schema-bump contract. 7. **Test comment alignment** — fix "With budget=1" comment to match `clientBudget: 2` code. Plus 4 new core regression tests covering #1/#2/#4/#5, and 4 new serve tests covering #3 (boot rejection + env cleanup). 237/237 pass across the affected files (36 core mcp-client-manager + 50 acpAgent + 151 serve). --- packages/cli/src/acp-integration/acpAgent.ts | 24 ++- packages/cli/src/serve/runQwenServe.ts | 31 ++++ packages/cli/src/serve/server.test.ts | 69 +++++++++ packages/cli/src/serve/status.ts | 14 +- .../core/src/tools/mcp-client-manager.test.ts | 141 +++++++++++++++++- packages/core/src/tools/mcp-client-manager.ts | 75 ++++++++-- packages/sdk-typescript/src/daemon/types.ts | 10 +- 7 files changed, 342 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 95df1246a7..8f42e48af6 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -691,13 +691,23 @@ class QwenAgent implements Agent { const disabled = config.isMcpServerDisabled(name); const rawStatus = getMCPServerStatus(name); const refusedByBudget = refusedSet.has(name); + // PR 14 fix (review #4247): config-disable takes precedence + // over budget-refusal. `lastRefusedServerNames` is a + // per-discovery-pass snapshot; if an operator runs + // `/mcp disable ` against a server that was refused + // last pass, the entry stays in the refused list until the + // next discovery pass clears it (`McpClientManager.removeServer` + // now drops the entry too — see sibling fix). Either way, + // a `disabled` cell should NEVER show `budget_exhausted` — + // the operator's deliberate disable wins. + const effectivelyRefused = refusedByBudget && !disabled; const out: ServeWorkspaceMcpServerStatus = { kind: 'mcp_server', // Refused-by-budget shadows the raw status: the rawStatus // is `DISCONNECTED` (we never tried to connect), but the // operator-facing severity is `error` with an explanatory // errorKind rather than the generic disconnected `error`. - status: refusedByBudget + status: effectivelyRefused ? 'error' : this.mcpCellStatus(rawStatus, disabled), name, @@ -705,7 +715,7 @@ class QwenAgent implements Agent { transport: this.mcpTransport(server), disabled, }; - if (refusedByBudget) { + if (effectivelyRefused) { out.errorKind = 'budget_exhausted'; out.disabledReason = 'budget'; out.hint = @@ -772,6 +782,16 @@ class QwenAgent implements Agent { mode: ServeMcpBudgetMode, refusedCount: number, ): ServeMcpBudgetStatusCell[] { + // PR 14 fix (review #4247): when no `--mcp-client-budget` is + // configured the manager resolves to `mode: 'off'`. The protocol + // docs and SDK type comments promise `budgets: []` for that case; + // a synthetic `mcp_budget` cell carrying nothing actionable was + // (a) protocol-noncompliant, (b) clutter — clients iterating + // `budgets[]` to render rows would draw an "ok" budget row for + // uncapped workspaces. Always return empty so the top-level + // `budgetMode: 'off'` field is the sole signal that guardrails + // are inactive. + if (mode === 'off') return []; let status: ServeStatus = 'ok'; let errorKind: string | undefined; let hint: string | undefined; diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index ecfa5404c9..d5d2520920 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -185,11 +185,42 @@ export async function runQwenServe( // `process.env` via `{...process.env}` at spawn time. Standalone // `qwen` invocations (no daemon) leave the env untouched, so // direct CLI usage retains the historical no-enforcement default. + // + // PR 14 fix (review #4247): `runQwenServe` is exported and other + // validations (`requireAuth` no-token, `maxConnections` NaN, + // `--workspace` checks) live here, so embedded callers expect + // boot-time rejection of invalid inputs. The yargs CLI handler + // duplicates these checks for fast-fail UX, but `runQwenServe` is + // the source of truth. Also explicitly DELETE (vs leaving stale) + // the env vars when the caller omits the option, so an embedded + // process that starts multiple daemons in sequence doesn't leak + // a prior budget/mode into the next ACP child. + if (opts.mcpClientBudget !== undefined) { + if ( + !Number.isFinite(opts.mcpClientBudget) || + !Number.isInteger(opts.mcpClientBudget) || + opts.mcpClientBudget <= 0 + ) { + throw new TypeError( + `Invalid mcpClientBudget: ${opts.mcpClientBudget}. Must be a positive integer.`, + ); + } + } + if (opts.mcpBudgetMode === 'enforce' && opts.mcpClientBudget === undefined) { + throw new Error( + 'mcpBudgetMode="enforce" requires a positive mcpClientBudget. ' + + 'Pass mcpClientBudget=N, or set mcpBudgetMode to "warn" or "off".', + ); + } if (opts.mcpClientBudget !== undefined) { process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = String(opts.mcpClientBudget); + } else { + delete process.env['QWEN_SERVE_MCP_CLIENT_BUDGET']; } if (opts.mcpBudgetMode !== undefined) { process.env['QWEN_SERVE_MCP_BUDGET_MODE'] = opts.mcpBudgetMode; + } else { + delete process.env['QWEN_SERVE_MCP_BUDGET_MODE']; } const bridge = diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 5730ebacd7..ad2d89b390 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -2661,6 +2661,75 @@ describe('runQwenServe', () => { ).rejects.toThrow(/--require-auth/); }); + // PR 14 fix (review #4247): runQwenServe is the documented embedded + // entry point, so budget validation must live here, not just in the + // yargs CLI handler. Embedded callers (other tools wrapping the + // daemon, deps.bridge test injection) silently produced an uncapped + // child pre-fix despite requesting enforce. + it('rejects non-positive mcpClientBudget (#4175 PR 14)', async () => { + await expect( + runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + mcpClientBudget: 0, + }), + ).rejects.toThrow(/mcpClientBudget/); + await expect( + runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + mcpClientBudget: -5, + }), + ).rejects.toThrow(/mcpClientBudget/); + }); + + it('rejects mcpBudgetMode=enforce without a budget (#4175 PR 14)', async () => { + await expect( + runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + mcpBudgetMode: 'enforce', + }), + ).rejects.toThrow(/enforce.*requires.*mcpClientBudget/); + }); + + it('clears mcp env vars when caller omits the options (no leakage across daemons)', async () => { + process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = '99'; + process.env['QWEN_SERVE_MCP_BUDGET_MODE'] = 'enforce'; + try { + handle = await runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + // No mcpClientBudget / mcpBudgetMode — must wipe stale env + // from the previous daemon in the same process. + }); + expect(process.env['QWEN_SERVE_MCP_CLIENT_BUDGET']).toBeUndefined(); + expect(process.env['QWEN_SERVE_MCP_BUDGET_MODE']).toBeUndefined(); + } finally { + delete process.env['QWEN_SERVE_MCP_CLIENT_BUDGET']; + delete process.env['QWEN_SERVE_MCP_BUDGET_MODE']; + } + }); + + it('writes mcp env vars when caller provides the options', async () => { + handle = await runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + mcpClientBudget: 10, + mcpBudgetMode: 'warn', + }); + expect(process.env['QWEN_SERVE_MCP_CLIENT_BUDGET']).toBe('10'); + expect(process.env['QWEN_SERVE_MCP_BUDGET_MODE']).toBe('warn'); + // Cleanup so the next test starts clean. + delete process.env['QWEN_SERVE_MCP_CLIENT_BUDGET']; + delete process.env['QWEN_SERVE_MCP_BUDGET_MODE']; + }); + it('starts with --require-auth + token on loopback', async () => { handle = await runQwenServe({ hostname: '127.0.0.1', diff --git a/packages/cli/src/serve/status.ts b/packages/cli/src/serve/status.ts index 04a7c1028b..b71e861241 100644 --- a/packages/cli/src/serve/status.ts +++ b/packages/cli/src/serve/status.ts @@ -83,13 +83,21 @@ export type ServeMcpBudgetMode = 'enforce' | 'warn' | 'off'; */ export interface ServeMcpBudgetStatusCell extends ServeStatusCell { kind: 'mcp_budget'; - /** Identifies which accounting scope this cell describes. */ - scope: 'workspace'; + /** + * Identifies which accounting scope this cell describes. Today only + * `'workspace'` is emitted; future PRs (e.g. Wave 5 PR 23 shared + * pool) will add `'pool'`. The `string & {}` widening keeps IDE + * autocomplete + literal narrowing for known scopes while allowing + * unknown scopes through without a compile-time break — the + * protocol contract is "consumers MUST tolerate additional scope + * values, drop don't fail." See `qwen-serve-protocol.md` forward-compat note. + */ + scope: 'workspace' | (string & {}); /** Live (CONNECTED) MCP client count at snapshot time. */ liveCount: number; /** Configured cap (positive integer). Absent only when mode is `off`. */ budget?: number; - /** Active enforcement mode. `off` cells SHOULD still be reported with status `ok`. */ + /** Active enforcement mode. `off` mode produces no cell — `budgets: []`. */ mode: ServeMcpBudgetMode; /** Servers refused during the most recent discovery pass. */ refusedCount: number; diff --git a/packages/core/src/tools/mcp-client-manager.test.ts b/packages/core/src/tools/mcp-client-manager.test.ts index 90c9ffcd58..d47728cea5 100644 --- a/packages/core/src/tools/mcp-client-manager.test.ts +++ b/packages/core/src/tools/mcp-client-manager.test.ts @@ -1239,8 +1239,9 @@ describe('McpClientManager — PR 14 guardrails', () => { created.push(name); return makeConnectedMcpClientMock() as unknown as McpClient; }); - // `b` is disabled — must not even attempt to reserve. With budget=1, - // `a` and `c` should both succeed (b is invisible to the gate). + // `b` is disabled — must not even attempt to reserve. With budget=2, + // `a` and `c` should both succeed (b is invisible to the gate, so it + // doesn't consume a slot; the cap is enough for the remaining two). const config = configWithServers( { a: { command: 'node' }, @@ -1268,4 +1269,140 @@ describe('McpClientManager — PR 14 guardrails', () => { ]); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual([]); }); + + // PR 14 fix (review #4247): regression tests for the four bypass / + // ordering / staleness bugs the Codex + Copilot reviews caught. + it('single-server rediscovery respects the budget gate (review #1)', async () => { + const created: string[] = []; + vi.mocked(McpClient).mockImplementation((name: string) => { + created.push(name); + return makeConnectedMcpClientMock() as unknown as McpClient; + }); + const config = configWithServers({ + a: { command: 'node' }, + b: { command: 'node' }, + }); + const manager = new McpClientManager( + config, + {} as ToolRegistry, + undefined, + undefined, + undefined, + { clientBudget: 1, budgetMode: 'enforce' }, + ); + await manager.discoverAllMcpTools(config); + // `b` was refused at startup. A manual `/mcp reconnect b` (which goes + // through `discoverMcpToolsForServer` → `...Internal`) would have + // pre-fix bypassed the gate and exceeded the cap. Now it must stay + // refused. + expect(created).toEqual(['a']); + expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); + await manager.discoverMcpToolsForServer('b', config); + expect(created).toEqual(['a']); // no new McpClient created + expect(manager.getMcpClientAccounting().reservedSlots).toEqual(['a']); + expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); + }); + + it('disconnectServer-then-disable drops refusal tag (review #4)', async () => { + vi.mocked(McpClient).mockImplementation( + () => makeConnectedMcpClientMock() as unknown as McpClient, + ); + const config = configWithServers({ + a: { command: 'node' }, + b: { command: 'node' }, + }); + const manager = new McpClientManager( + config, + {} as ToolRegistry, + undefined, + undefined, + undefined, + { clientBudget: 1, budgetMode: 'enforce' }, + ); + await manager.discoverAllMcpTools(config); + expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); + // Operator action: explicit disconnect of `b` should drop it from + // the refusal log so a snapshot doesn't keep tagging the now- + // operator-disabled server with `budget_exhausted`. + await manager.disconnectServer('b'); + expect(manager.getMcpClientAccounting().refusedServerNames).toEqual([]); + }); + + it('incremental discovery frees removed slots BEFORE reserving new ones (review #5)', async () => { + let inflight = 0; + vi.mocked(McpClient).mockImplementation( + () => makeConnectedMcpClientMock() as unknown as McpClient, + ); + inflight = 0; + const mcpServers: Record = { + a: { command: 'node' }, + b: { command: 'node' }, + }; + const config = { + isTrustedFolder: () => true, + getMcpServers: () => mcpServers, + getMcpServerCommand: () => undefined, + getPromptRegistry: () => ({}) as PromptRegistry, + getWorkspaceContext: () => ({}) as WorkspaceContext, + getDebugMode: () => false, + isMcpServerDisabled: () => false, + } as unknown as Config; + const manager = new McpClientManager( + config, + { + removeMcpToolsByServer: () => undefined, + } as unknown as ToolRegistry, + undefined, + undefined, + undefined, + { clientBudget: 2, budgetMode: 'enforce' }, + ); + await manager.discoverAllMcpToolsIncremental(config); + expect(manager.getMcpClientAccounting().reservedSlots.sort()).toEqual([ + 'a', + 'b', + ]); + + // Swap b → c (still budget=2). Pre-fix order: `c` refused because + // `b`'s slot was only freed after the new-server loop. Post-fix: + // `b` removed first → reservedSlots={a} → `c` reserved. + delete mcpServers['b']; + mcpServers['c'] = { command: 'node' }; + await manager.discoverAllMcpToolsIncremental(config); + expect(manager.getMcpClientAccounting().reservedSlots.sort()).toEqual([ + 'a', + 'c', + ]); + expect(manager.getMcpClientAccounting().refusedServerNames).toEqual([]); + void inflight; + }); + + it('buildBudgetCells deferred to acpAgent — manager off-mode returns no budget bookkeeping (review #2)', async () => { + // Sibling check: when `mode === 'off'` the manager doesn't reserve + // anything and the snapshot has empty `reservedSlots` + zero + // `refusedServerNames`. The empty-`budgets[]` assertion lives in + // the serve route test (`server.test.ts`) because the cell is + // built by `acpAgent.buildBudgetCells`. This test just pins the + // manager-side invariant: off-mode is pure observability. + vi.mocked(McpClient).mockImplementation( + () => makeConnectedMcpClientMock() as unknown as McpClient, + ); + const config = configWithServers({ + a: { command: 'node' }, + b: { command: 'node' }, + }); + const manager = new McpClientManager( + config, + {} as ToolRegistry, + undefined, + undefined, + undefined, + { budgetMode: 'off' }, + ); + await manager.discoverAllMcpTools(config); + const accounting = manager.getMcpClientAccounting(); + expect(accounting.total).toBe(2); + expect(accounting.reservedSlots).toEqual([]); + expect(accounting.refusedServerNames).toEqual([]); + }); }); diff --git a/packages/core/src/tools/mcp-client-manager.ts b/packages/core/src/tools/mcp-client-manager.ts index 610a986cf2..1e3c08cf15 100644 --- a/packages/core/src/tools/mcp-client-manager.ts +++ b/packages/core/src/tools/mcp-client-manager.ts @@ -447,6 +447,28 @@ export class McpClientManager { return; } + // PR 14 fix (review #4247): single-server rediscovery (reachable from + // `/mcp reconnect ` and `ToolRegistry.discoverToolsForServer`) + // previously bypassed the budget gate, so a server refused at startup + // could be brought online later under `enforce` mode and exceed the + // cap. True reconnect against a held slot returns `'already_held'` + // and falls through unchanged; only a fresh attempt against a server + // without a reservation can be refused. Best-effort semantics — log + // the refusal and return without creating an `McpClient`; the caller + // observes the absence via `getStatus()` like any other discovery + // failure. + const reservation = this.tryReserveSlot(serverName); + if (reservation === 'refused') { + if (!this.lastRefusedServerNames.includes(serverName)) { + this.lastRefusedServerNames.push(serverName); + } + process.stderr.write( + `qwen serve: MCP server '${serverName}' refused (budget exhausted, ` + + `budget=${this.clientBudget}, mode=enforce)\n`, + ); + return; + } + this.stopHealthCheck(serverName); // Ensure we don't leak an existing connection for this server. @@ -552,15 +574,23 @@ export class McpClientManager { this.consecutiveFailures.delete(serverName); this.isReconnecting.delete(serverName); this.serverDiscoveryPromises.delete(serverName); - // PR 14: explicit operator-driven disconnect releases the - // budget slot. The internal reconnect path - // (`discoverMcpToolsForServerInternal`) calls - // `existingClient.disconnect()` directly, NOT this public - // method, so reconnect doesn't release. - this.reservedSlots.delete(serverName); this.eventEmitter?.emit('mcp-client-update', this.clients); } } + // PR 14: explicit operator-driven disconnect releases the budget + // slot AND drops the entry from the per-pass refusal log. Outside + // the `if (client)` guard because a budget-refused server has NO + // `McpClient` instance — but operator intent ("stop tracking this + // server") still demands the records be cleared so a subsequent + // snapshot doesn't keep tagging it as `budget_exhausted`. The + // internal reconnect path (`discoverMcpToolsForServerInternal`) + // calls `existingClient.disconnect()` directly, NOT this public + // method, so reconnect still doesn't release the slot. + this.reservedSlots.delete(serverName); + const refusedIdx = this.lastRefusedServerNames.indexOf(serverName); + if (refusedIdx >= 0) { + this.lastRefusedServerNames.splice(refusedIdx, 1); + } } getDiscoveryState(): MCPDiscoveryState { @@ -755,6 +785,22 @@ export class McpClientManager { const currentServerNames = new Set(this.clients.keys()); const newServerNames = new Set(Object.keys(servers)); + // PR 14 fix (review #4247): process removals BEFORE the new-server + // reservation pass so freed slots are visible to `tryReserveSlot`. + // Scenario: budget=2, currently `{a, b}` reserved, new config + // `{a, c}`. Pre-fix order refused `c` because `b`'s slot was only + // freed after the new-server loop. Now `b` is removed first → + // reservedSlots={a} → `c` reservation succeeds. Disabled-mid-session + // removals stay inline (below) because they also release slots + // via `removeServer`'s `reservedSlots.delete` — same call, just + // reached from a different branch. + for (const name of currentServerNames) { + if (!newServerNames.has(name)) { + // Server was removed from configuration + await this.removeServer(name); + } + } + // Check for new servers or configuration changes for (const [name] of Object.entries(servers)) { // Mirror `discoverAllMcpTools` (line ~102): users who explicitly @@ -801,14 +847,6 @@ export class McpClientManager { // the old and new config, which is not implemented here } - // Find removed servers - for (const name of currentServerNames) { - if (!newServerNames.has(name)) { - // Server was removed from configuration - await this.removeServer(name); - } - } - // Update only the servers that need it. Each per-server discover is // wrapped in a discovery-only timeout (stdio default 30s, remote 5s, // per-server override via `discoveryTimeoutMs`). Tool-call timeout is @@ -1043,6 +1081,15 @@ export class McpClientManager { // be running", so it must not block a different server from taking // its place on the next discovery pass. this.reservedSlots.delete(serverName); + // PR 14 fix (review #4247): also drop the entry from the per-pass + // refusal log so a snapshot taken between discoveries doesn't + // stale-tag the (now-disabled or now-removed) server as + // `disabledReason: 'budget'`. Operator action wins over the + // last-pass startup refusal record. + const refusedIdx = this.lastRefusedServerNames.indexOf(serverName); + if (refusedIdx >= 0) { + this.lastRefusedServerNames.splice(refusedIdx, 1); + } // Remove tools for this server from registry this.toolRegistry.removeMcpToolsByServer(serverName); diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index f8448e0424..408afda036 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -233,7 +233,15 @@ export type DaemonMcpBudgetMode = 'enforce' | 'warn' | 'off'; */ export interface DaemonMcpBudgetStatusCell extends DaemonStatusCell { kind: 'mcp_budget'; - scope: 'workspace'; + /** + * Today only `'workspace'` is emitted; future PRs (e.g. Wave 5 + * PR 23 shared MCP pool) will add `'pool'` without a schema bump. + * The `string & {}` widening keeps IDE autocomplete + literal + * narrowing for known scopes while allowing unknown scopes through + * — the protocol contract is "consumers MUST tolerate additional + * scope values, drop don't fail." See `qwen-serve-protocol.md`. + */ + scope: 'workspace' | (string & {}); liveCount: number; /** Configured cap. Absent when mode is `off`. */ budget?: number;