From f05917071ca77e6182af151717da971ba52ec362 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 22 May 2026 00:44:38 +0800 Subject: [PATCH 1/6] =?UTF-8?q?refactor(core):=20F2=20PR=20A=20R9=20?= =?UTF-8?q?=E2=80=94=20McpClientManager=20options-object=20ctor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R9 (filed as F2 follow-up from #4336 review): 7 positional ctor args collapse to (config, toolRegistry, options?: McpClientManagerOptions). The trailing 5 (eventEmitter, sendSdkMcpMessage, healthConfig, budgetConfig, pool) become named fields on `McpClientManagerOptions`. Test factory `mkManager(overrides?)` introduced at the top of `mcp-client-manager.test.ts` so each of the prior 80 inline constructions becomes a single line naming only the field(s) the test overrides; the 4 `undefined` sentinels each test threaded through to reach the trailing `pool` arg are gone. Net: 113 LOC removed (test) + 35 LOC added (src exposes interface + mkManager factory + tool-registry call site update). Behavior unchanged — same field assignments, same downgrade-enforce-without- budget breadcrumb, same budget event wiring. Filed bucket: F2 perf / cleanup PR A (R9 + W11 + W12 + R10/R23 T7), see issue #4175 item 7 "F2 post-merge cleanup PRs". This is the first of the 4 fixes in PR A; W11/W12/R10 follow as separate commits. Test sweep: 84/84 mcp-client-manager.test.ts pass; typecheck clean. --- .../core/src/tools/mcp-client-manager.test.ts | 813 ++++++++---------- packages/core/src/tools/mcp-client-manager.ts | 35 +- packages/core/src/tools/tool-registry.ts | 24 +- 3 files changed, 384 insertions(+), 488 deletions(-) diff --git a/packages/core/src/tools/mcp-client-manager.test.ts b/packages/core/src/tools/mcp-client-manager.test.ts index 9220bb3b853..c86ac31abc0 100644 --- a/packages/core/src/tools/mcp-client-manager.test.ts +++ b/packages/core/src/tools/mcp-client-manager.test.ts @@ -5,7 +5,10 @@ */ import { afterEach, describe, expect, it, vi } from 'vitest'; -import { McpClientManager } from './mcp-client-manager.js'; +import { + McpClientManager, + type McpClientManagerOptions, +} from './mcp-client-manager.js'; import { McpClient } from './mcp-client.js'; import type { ToolRegistry } from './tool-registry.js'; import type { Config } from '../config/config.js'; @@ -22,6 +25,37 @@ vi.mock('./mcp-client.js', async () => { }; }); +/** + * F2 (#4175 commit 6 review fix — wenshao R9 / PR A): test factory + * for `McpClientManager`. Pre-fix the 80 construction sites in this + * file each repeated a 7-positional call with 4 `undefined` sentinels + * to reach the trailing `pool` arg. With the options-object ctor + + * this factory, each site names only the fields it overrides; default + * `mockConfig` + `{} as ToolRegistry` cover the no-arg case. + */ +function mkManager( + overrides: { + config?: Config; + toolRegistry?: ToolRegistry; + options?: McpClientManagerOptions; + } = {}, +): McpClientManager { + const config = + overrides.config ?? + ({ + isTrustedFolder: () => true, + getMcpServers: () => ({}), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => ({}), + getWorkspaceContext: () => ({}), + getDebugMode: () => false, + getSessionId: () => 'sid-1', + isMcpServerDisabled: () => false, + } as unknown as Config); + const toolRegistry = overrides.toolRegistry ?? ({} as ToolRegistry); + return new McpClientManager(config, toolRegistry, overrides.options ?? {}); +} + describe('McpClientManager', () => { afterEach(() => { vi.restoreAllMocks(); @@ -59,15 +93,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverAllMcpTools(mockConfig); expect(acquireSpy).toHaveBeenCalledTimes(1); expect(acquireSpy).toHaveBeenCalledWith( @@ -125,15 +154,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Should resolve without throwing — the BudgetExhaustedError on // srvB is caught and downgraded to a debug log. await manager.discoverAllMcpTools(mockConfig); @@ -184,15 +208,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Kick off discovery; it enters in-flight (acquire awaits the gate). const discoveryPromise = manager.discoverAllMcpTools(mockConfig); @@ -247,15 +266,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Inject a rejecting in-flight promise (the only way to hit the // W108 catch block — internal per-server catches mean the natural // path always resolves). @@ -297,15 +311,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Inject a never-settling in-flight promise. ( manager as unknown as { discoveryInFlight?: Promise } @@ -354,15 +363,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); // Simulate a prior timed-out shutdown that set the sticky flag. (manager as unknown as { stopTimedOut: boolean }).stopTimedOut = true; @@ -417,15 +421,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); expect(acquireSpy).toHaveBeenCalledTimes(1); expect(McpClient).not.toHaveBeenCalled(); @@ -454,15 +453,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverMcpToolsForServer('srv', mockConfig); @@ -500,15 +494,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverAllMcpTools(mockConfig); const result = await manager.readResource('srv', 'mcp://srv/doc'); @@ -565,15 +554,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); await manager.discoverAllMcpTools(mockConfig); // Sanity: healthy fast-path still works. await expect(manager.readResource('srv', 'mcp://srv/doc')).resolves.toEqual( @@ -636,15 +620,13 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - { removeMcpToolsByServer: vi.fn() } as unknown as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + options: { pool: fakePool }, + }); await manager.discoverAllMcpTools(mockConfig); expect(releaseSpy).not.toHaveBeenCalled(); await manager.disconnectServer('srv'); @@ -687,15 +669,10 @@ describe('McpClientManager', () => { getSessionId: () => 'sid-1', isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - undefined, - undefined, - fakePool, - ); + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); const p1 = manager.discoverAllMcpTools(mockConfig); const p2 = manager.discoverAllMcpTools(mockConfig); // Both passes block on the in-flight `pool.acquire`. Pre-fix @@ -732,7 +709,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpTools(mockConfig); expect(McpClient).toHaveBeenCalledOnce(); expect(mockedMcpClient.connect).toHaveBeenCalledOnce(); @@ -757,7 +734,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpTools(mockConfig); expect(mockedMcpClient.connect).toHaveBeenCalledOnce(); expect(mockedMcpClient.discover).toHaveBeenCalledOnce(); @@ -782,7 +759,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpTools(mockConfig); expect(mockedMcpClient.connect).not.toHaveBeenCalled(); expect(mockedMcpClient.discover).not.toHaveBeenCalled(); @@ -812,7 +789,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); // First connect to create the clients await manager.discoverAllMcpTools({ isTrustedFolder: () => true, @@ -848,7 +825,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpTools({ isTrustedFolder: () => true, isMcpServerDisabled: () => false, @@ -879,7 +856,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverMcpToolsForServer( 'test-server', @@ -919,7 +896,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverMcpToolsForServer( 'test-server', @@ -979,7 +956,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverMcpToolsForServer( 'test-server', @@ -1048,18 +1025,17 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - undefined, - undefined, - { - autoReconnect: true, - checkIntervalMs: 10, - maxConsecutiveFailures: 1, - reconnectDelayMs: 10, + const manager = mkManager({ + config: mockConfig, + options: { + healthConfig: { + autoReconnect: true, + checkIntervalMs: 10, + maxConsecutiveFailures: 1, + reconnectDelayMs: 10, + }, }, - ); + }); try { await manager.discoverMcpToolsForServer( @@ -1116,7 +1092,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); const discovery = manager.discoverMcpToolsForServer( 'test-server', @@ -1165,7 +1141,7 @@ describe('McpClientManager', () => { getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverMcpToolsForServer('unknown-server', { isTrustedFolder: () => true, @@ -1203,9 +1179,12 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer: vi.fn(), - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + }); const t0 = Date.now(); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1254,7 +1233,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: (name: string) => name === 'disabled', } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1297,7 +1276,10 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: (name: string) => name === 'foo' && disabled, } as unknown as Config; - const manager = new McpClientManager(mockConfig, toolRegistryStub); + const manager = mkManager({ + config: mockConfig, + toolRegistry: toolRegistryStub, + }); // First pass: server enabled, gets connected. await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1352,7 +1334,7 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + const manager = mkManager({ config: mockConfig }); await manager.discoverAllMcpToolsIncremental(mockConfig); // Cleanup the global sink so it doesn't leak into other tests. @@ -1409,9 +1391,12 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer: vi.fn(), - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); spy.mockRestore(); @@ -1461,9 +1446,12 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer: vi.fn(), - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); spy.mockRestore(); @@ -1510,9 +1498,10 @@ describe('McpClientManager', () => { isMcpServerDisabled: () => false, } as unknown as Config; const removeMcpToolsByServer = vi.fn(); - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer, - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { removeMcpToolsByServer } as unknown as ToolRegistry, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1562,9 +1551,12 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager(mockConfig, { - removeMcpToolsByServer: vi.fn(), - } as unknown as ToolRegistry); + const manager = mkManager({ + config: mockConfig, + toolRegistry: { + removeMcpToolsByServer: vi.fn(), + } as unknown as ToolRegistry, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1625,11 +1617,10 @@ describe('McpClientManager', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( - mockConfig, - {} as ToolRegistry, - events, - ); + const manager = mkManager({ + config: mockConfig, + options: { eventEmitter: events }, + }); await manager.discoverAllMcpToolsIncremental(mockConfig); @@ -1742,14 +1733,10 @@ describe('McpClientManager — PR 14 guardrails', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(created).toHaveLength(2); // only 2 McpClient instances created const accounting = manager.getMcpClientAccounting(); @@ -1772,14 +1759,10 @@ describe('McpClientManager — PR 14 guardrails', () => { b: { command: 'node' }, c: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'warn' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'warn' } }, + }); await manager.discoverAllMcpTools(config); // warn mode: all 3 connect; reservedSlots grows past budget; no refusals. expect(created).toHaveLength(3); @@ -1797,14 +1780,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { budgetMode: 'off' }, - ); + options: { budgetConfig: { budgetMode: 'off' } }, + }); await manager.discoverAllMcpTools(config); const accounting = manager.getMcpClientAccounting(); expect(accounting.total).toBe(2); @@ -1826,14 +1805,10 @@ describe('McpClientManager — PR 14 guardrails', () => { alpha: { command: 'node' }, mike: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(created).toEqual(['zulu', 'alpha']); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual([ @@ -1849,14 +1824,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); @@ -1877,14 +1848,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); // `a` was reserved; `b` was refused. A `readResource('b', ...)` would // lazy-spawn — must throw rather than silently exceed the cap. @@ -1901,14 +1868,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().reservedSlots).toEqual(['a']); await manager.disconnectServer('a'); @@ -1920,7 +1883,7 @@ describe('McpClientManager — PR 14 guardrails', () => { process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = '7'; process.env['QWEN_SERVE_MCP_BUDGET_MODE'] = 'enforce'; const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBe(7); expect(manager.getMcpBudgetMode()).toBe('enforce'); }); @@ -1929,7 +1892,7 @@ describe('McpClientManager — PR 14 guardrails', () => { process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = '5'; // No mode env var. Resolved mode is `warn` (the safe default). const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBe(5); expect(manager.getMcpBudgetMode()).toBe('warn'); }); @@ -1937,7 +1900,7 @@ describe('McpClientManager — PR 14 guardrails', () => { it('env var fallback rejects non-positive budgets silently', async () => { process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = '-3'; const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); // Invalid values fall through to `undefined` budget + `off` mode — // no enforcement, no boot-time crash. Validation lives in the CLI // flag handler (`packages/cli/src/commands/serve.ts`). @@ -1965,14 +1928,10 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'b') as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(created.sort()).toEqual(['a', 'c']); expect(manager.getMcpClientAccounting().reservedSlots.sort()).toEqual([ @@ -1994,14 +1953,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { 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 @@ -2023,14 +1978,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); // Operator action: explicit disconnect of `b` should drop it from @@ -2059,16 +2010,13 @@ describe('McpClientManager — PR 14 guardrails', () => { getDebugMode: () => false, isMcpServerDisabled: () => false, } as unknown as Config; - const manager = new McpClientManager( + const manager = mkManager({ config, - { + toolRegistry: { removeMcpToolsByServer: () => undefined, } as unknown as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpToolsIncremental(config); expect(manager.getMcpClientAccounting().reservedSlots.sort()).toEqual([ 'a', @@ -2103,14 +2051,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { budgetMode: 'off' }, - ); + options: { budgetConfig: { budgetMode: 'off' } }, + }); await manager.discoverAllMcpTools(config); const accounting = manager.getMcpClientAccounting(); expect(accounting.total).toBe(2); @@ -2140,14 +2084,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, // will fail b: { command: 'node' }, // would be refused pre-fix }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); // `a` failed → slot freed → `b` ought to fit (budget=1, current=0 // after `a` released). But discoverAllMcpTools walks all servers @@ -2186,14 +2126,10 @@ describe('McpClientManager — PR 14 guardrails', () => { const config = configWithServers({ a: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); // No discovery yet → `a` not in clients → lazy spawn path. await expect(manager.readResource('a', 'file:///x')).rejects.toThrow( 'lazy connect boom', @@ -2216,7 +2152,7 @@ describe('McpClientManager — PR 14 guardrails', () => { // `tryReserveSlot` returns 'reserved' when `clientBudget === undefined`, // so an "enforce" daemon would let unlimited servers through. const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBeUndefined(); // Downgraded — not 'enforce' — because enforce requires a budget. expect(manager.getMcpBudgetMode()).toBe('off'); @@ -2238,7 +2174,7 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'a') as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); await expect(manager.readResource('a', 'file:///x')).rejects.toThrow( /'a' is disabled/, ); @@ -2261,14 +2197,10 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'b') as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); // Even though `b` would be budget-refused if not disabled, the // disabled gate must trip first. @@ -2300,14 +2232,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ x: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); // Server `x` not previously reserved; this call freshly reserves // then connect() throws. Pre-fix the slot leaked permanently // under enforce mode, blocking any later server in `clients.size=1`. @@ -2345,19 +2273,21 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ a: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - { removeMcpToolsByServer: () => undefined } as unknown as ToolRegistry, - undefined, - undefined, - { - autoReconnect: false, - checkIntervalMs: 100, - maxConsecutiveFailures: 1, - reconnectDelayMs: 100, + toolRegistry: { + removeMcpToolsByServer: () => undefined, + } as unknown as ToolRegistry, + options: { + healthConfig: { + autoReconnect: false, + checkIntervalMs: 100, + maxConsecutiveFailures: 1, + reconnectDelayMs: 100, + }, + budgetConfig: { clientBudget: 2, budgetMode: 'enforce' }, }, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + }); const discoveryPromise = manager.discoverAllMcpToolsIncremental(config); // Advance past the stdio default discovery timeout (30s). await vi.advanceTimersByTimeAsync(31_000); @@ -2383,14 +2313,13 @@ describe('McpClientManager — PR 14 guardrails', () => { second: { command: 'node' }, third: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - { removeMcpToolsByServer: () => undefined } as unknown as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 2, budgetMode: 'enforce' }, - ); + toolRegistry: { + removeMcpToolsByServer: () => undefined, + } as unknown as ToolRegistry, + options: { budgetConfig: { clientBudget: 2, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpToolsIncremental(config); // First two declared servers fit; third refused. Refusal-order // determinism preserved (config-declaration order) — the inner @@ -2418,14 +2347,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); // Free a slot. @@ -2450,14 +2375,10 @@ describe('McpClientManager — PR 14 guardrails', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']); // Free a slot. @@ -2485,7 +2406,7 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'a') as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); await manager.discoverMcpToolsForServer('a', config); expect(createdCount).toBe(0); }); @@ -2508,14 +2429,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ x: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverMcpToolsForServer('x', config); // Slot released on weReservedSlot+catch path AND the transport // was closed before dropping the client reference. @@ -2528,7 +2445,7 @@ describe('McpClientManager — PR 14 guardrails', () => { process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = 'abc'; try { const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBeUndefined(); // Operator-visible breadcrumb landed on stderr. const calls = writeSpy.mock.calls.map((c) => String(c[0])); @@ -2560,7 +2477,7 @@ describe('McpClientManager — PR 14 guardrails', () => { name === 'a' && disabled) as Config['isMcpServerDisabled'], }, ); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); // First connect while NOT disabled. await manager.discoverAllMcpTools(config); // Now operator disables 'a' mid-session. @@ -2594,14 +2511,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ x: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await expect(manager.readResource('x', 'file:///a')).rejects.toThrow( /mid-handshake failure/, ); @@ -2615,7 +2528,7 @@ describe('McpClientManager — PR 14 guardrails', () => { // No budget → downgrade fires try { const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpBudgetMode()).toBe('off'); const calls = writeSpy.mock.calls.map((c) => String(c[0])); expect( @@ -2650,14 +2563,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ a: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); await manager.discoverAllMcpTools(config); // Transport closed before client reference dropped + slot released. expect(disconnectCalls).toBeGreaterThanOrEqual(1); @@ -2669,7 +2578,7 @@ describe('McpClientManager — PR 14 guardrails', () => { // No budget — pre-fix this passed through with mode='warn', // reaching emitBudgetTelemetry with clientBudget=undefined. const config = configWithServers({}); - const manager = new McpClientManager(config, {} as ToolRegistry); + const manager = mkManager({ config }); expect(manager.getMcpClientBudget()).toBeUndefined(); expect(manager.getMcpBudgetMode()).toBe('off'); }); @@ -2681,15 +2590,11 @@ describe('McpClientManager — PR 14 guardrails', () => { // path's downgrade so a future caller that bypasses validation // can't silently fail-open. const config = configWithServers({}); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, // Invalid combination: enforce mode without a budget. - { budgetMode: 'enforce' }, - ); + options: { budgetConfig: { budgetMode: 'enforce' } }, + }); // Downgraded to off so tryReserveSlot doesn't masquerade as enforce. expect(manager.getMcpBudgetMode()).toBe('off'); }); @@ -2718,14 +2623,10 @@ describe('McpClientManager — PR 14 guardrails', () => { }) as unknown as McpClient, ); const config = configWithServers({ a: { command: 'node' } }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { clientBudget: 1, budgetMode: 'enforce' }, - ); + options: { budgetConfig: { clientBudget: 1, budgetMode: 'enforce' } }, + }); // First pass: a connects successfully, slot reserved. await manager.discoverAllMcpTools(config); expect(manager.getMcpClientAccounting().reservedSlots).toEqual(['a']); @@ -2803,18 +2704,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); const warnings = events.filter( (e) => (e as { kind: string }).kind === 'budget_warning', @@ -2845,18 +2744,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); expect( events.filter((e) => (e as { kind: string }).kind === 'budget_warning'), @@ -2882,18 +2779,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { const config = configWithServers({}, { getMcpServers: cfgGetter, } as Partial); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); expect( events.filter((e) => (e as { kind: string }).kind === 'budget_warning'), @@ -2943,14 +2838,15 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { budgetMode: 'off', onBudgetEvent: (e) => events.push(e) }, - ); + options: { + budgetConfig: { + budgetMode: 'off', + onBudgetEvent: (e) => events.push(e), + }, + }, + }); await manager.discoverAllMcpTools(config); expect(events).toEqual([]); }); @@ -2966,18 +2862,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { b: { httpUrl: 'http://b' }, c: { url: 'http://c' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); const batches = events.filter( (e) => (e as { kind: string }).kind === 'refused_batch', @@ -3003,18 +2897,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 5, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 5, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); expect( events.filter((e) => (e as { kind: string }).kind === 'refused_batch'), @@ -3031,18 +2923,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); // First pass fills the budget with `a`. `b` is refused — that's // the bulk refusal (length-1 batch). await manager.discoverAllMcpTools(config); @@ -3080,14 +2970,15 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { a: { command: 'node' }, b: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { budgetMode: 'off', onBudgetEvent: (e) => events.push(e) }, - ); + options: { + budgetConfig: { + budgetMode: 'off', + onBudgetEvent: (e) => events.push(e), + }, + }, + }); await manager.discoverAllMcpTools(config); // Force discovery refusal would be impossible in off mode (no // budget). Disconnect-then-rediscover also no-ops the state @@ -3110,18 +3001,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { d: { tcp: 'ws://d' }, // websocket (refused) e: { type: 'sdk', command: 'sdk' }, // sdk (refused) }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); const batches = events.filter( (e) => (e as { kind: string }).kind === 'refused_batch', @@ -3142,18 +3031,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { b: { command: 'node' }, c: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); // warn mode: no refusals, but the warning may fire (3/1 ratio crosses 0.75). expect( @@ -3172,18 +3059,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); // First crossing fired one warning. expect( @@ -3218,18 +3103,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 1, - budgetMode: 'enforce', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 1, + budgetMode: 'enforce', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpToolsIncremental(config); const batches = events.filter( (e) => (e as { kind: string }).kind === 'refused_batch', @@ -3261,18 +3144,16 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { c: { command: 'node' }, d: { command: 'node' }, }); - const manager = new McpClientManager( + const manager = mkManager({ config, - {} as ToolRegistry, - undefined, - undefined, - undefined, - { - clientBudget: 4, - budgetMode: 'warn', - onBudgetEvent: (e) => events.push(e), + options: { + budgetConfig: { + clientBudget: 4, + budgetMode: 'warn', + onBudgetEvent: (e) => events.push(e), + }, }, - ); + }); await manager.discoverAllMcpTools(config); expect( events.filter((e) => (e as { kind: string }).kind === 'budget_warning'), diff --git a/packages/core/src/tools/mcp-client-manager.ts b/packages/core/src/tools/mcp-client-manager.ts index 25f231027d1..53a4dff7836 100644 --- a/packages/core/src/tools/mcp-client-manager.ts +++ b/packages/core/src/tools/mcp-client-manager.ts @@ -343,6 +343,26 @@ function readBudgetFromEnv(): McpBudgetConfig { return { clientBudget, budgetMode }; } +/** + * F2 (#4175 commit 6 review fix — wenshao R9 / PR A): options bag for + * `McpClientManager` construction, replacing the prior 5 trailing + * positional parameters (`eventEmitter`, `sendSdkMcpMessage`, + * `healthConfig`, `budgetConfig`, `pool`). Pre-fix every test site + * threaded 4 explicit `undefined`s to reach the trailing `pool` arg — + * the fixed positions also blocked future option additions without + * re-ordering. The options-object form lets each caller name only the + * fields it cares about and keeps the constructor signature stable + * across future additions (e.g. when the W8 health-monitor wire-up + * lands a new `reconnectStrategy` knob). + */ +export interface McpClientManagerOptions { + eventEmitter?: EventEmitter; + sendSdkMcpMessage?: SendSdkMcpMessage; + healthConfig?: Partial; + budgetConfig?: McpBudgetConfig; + pool?: import('./mcp-transport-pool.js').McpTransportPool; +} + /** * Manages the lifecycle of multiple MCP clients, including local child processes. * This class is responsible for starting, stopping, and discovering tools from @@ -513,19 +533,16 @@ export class McpClientManager { constructor( config: Config, toolRegistry: ToolRegistry, - eventEmitter?: EventEmitter, - sendSdkMcpMessage?: SendSdkMcpMessage, - healthConfig?: Partial, - budgetConfig?: McpBudgetConfig, - pool?: import('./mcp-transport-pool.js').McpTransportPool, + options: McpClientManagerOptions = {}, ) { this.cliConfig = config; this.toolRegistry = toolRegistry; - this.pool = pool; + this.pool = options.pool; - this.eventEmitter = eventEmitter; - this.sendSdkMcpMessage = sendSdkMcpMessage; - this.healthConfig = { ...DEFAULT_HEALTH_CONFIG, ...healthConfig }; + this.eventEmitter = options.eventEmitter; + this.sendSdkMcpMessage = options.sendSdkMcpMessage; + this.healthConfig = { ...DEFAULT_HEALTH_CONFIG, ...options.healthConfig }; + const budgetConfig = options.budgetConfig; // Tests inject `budgetConfig` directly; production reads env vars // set by `qwen serve --mcp-client-budget=N --mcp-budget-mode=X` diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index a9dcb637b95..ab79c8b04c8 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -196,21 +196,19 @@ export class ToolRegistry { sendSdkMcpMessage?: SendSdkMcpMessage, ) { this.config = config; - this.mcpClientManager = new McpClientManager( - this.config, - this, + // F2 (#4175 commit 6 review fix — wenshao R9 / PR A): options-bag + // ctor; previously 7 positional args with `undefined, undefined` + // sentinels for `healthConfig` / `budgetConfig`. `pool` is + // forwarded from Config (set by daemon-mode QwenAgent in + // `newSessionConfig`); when undefined the manager keeps its pre-F2 + // per-session spawn behavior, when defined non-SDK MCP discovery + // goes through `pool.acquire` so N sessions in the same workspace + // share one transport per unique server config. + this.mcpClientManager = new McpClientManager(this.config, this, { eventEmitter, sendSdkMcpMessage, - // F2 (#4175 commit 4): forward the workspace-shared MCP transport - // pool from Config (set by daemon-mode QwenAgent in - // newSessionConfig). When undefined, McpClientManager keeps its - // pre-F2 per-session spawn behavior. When defined, non-SDK MCP - // discovery goes through pool.acquire so N sessions in the same - // workspace share one transport per unique server config. - undefined, // healthConfig: keep manager defaults - undefined, // budgetConfig: keep manager defaults - this.config.getMcpTransportPool(), - ); + pool: this.config.getMcpTransportPool(), + }); } /** From 20d2f1b90d3c8daf24492ae9b836c7bf9cd8747a Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 22 May 2026 00:47:40 +0800 Subject: [PATCH 2/6] =?UTF-8?q?refactor(core):=20F2=20PR=20A=20W11=20?= =?UTF-8?q?=E2=80=94=20extract=20attachPooledSession=20+=20rollbackReserva?= =?UTF-8?q?tionOnSpawnFailure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W11 (filed as F2 follow-up from #4336 review): two private helpers on `McpTransportPool` to eliminate inline duplication in `acquire()`: - `attachPooledSession(entry, id, serverName, cfg, sessionId, toolReg, promptReg)`: builds `SessionMcpView` + `entry.attach` with the standard pool release callback. Used by both the fast-path attach (existing entry) and the post-spawn attach (after `await inFlight`). NOT used by `createUnpooledConnection` — its release callback runs `entry.forceShutdown('manual')` + `indexDetach` directly (no pool refcount accounting since unpooled entries are per-session). - `rollbackReservationOnSpawnFailure(reservationResult, serverName)`: R24 T17 contract — only release the budget slot if THIS acquire actually reserved a new slot (`'reserved'`); `'already_held'` skips because the sibling owns it. Used by both the unpooled catch and the pooled spawn-in-flight catch. Race-window invariants (W10 / W77 / W90 / W111 / W125 / R24 T17) stay at the call sites because they describe the SURROUNDING ordering, not the helpers themselves. Helpers are documented to defer those decisions back to callers. Behavior unchanged. Filed bucket: F2 perf cleanup PR A (R9 done / W11 this commit / W12 + R10 to follow). Test sweep: 28/28 mcp-transport-pool.test.ts pass; typecheck clean. --- packages/core/src/tools/mcp-transport-pool.ts | 153 ++++++++++++------ 1 file changed, 108 insertions(+), 45 deletions(-) diff --git a/packages/core/src/tools/mcp-transport-pool.ts b/packages/core/src/tools/mcp-transport-pool.ts index a4cdb4f6b95..e1d671cdd74 100644 --- a/packages/core/src/tools/mcp-transport-pool.ts +++ b/packages/core/src/tools/mcp-transport-pool.ts @@ -237,13 +237,6 @@ export class McpTransportPool { // session caller. Also handles the leftover stale entry case // (any pre-existing zombie not yet evicted by `onClosed`). if (existing && !existing.isTerminated()) { - const view = new SessionMcpView( - sessionToolRegistry, - sessionPromptRegistry, - sessionId, - serverName, - cfg, - ); // F2 (#4175 commit 6 review fix — wenshao W10): index update // happens AFTER `attach` succeeds. Pre-fix the order was // reversed; an `attach` rejection (e.g., entry transitioned @@ -251,11 +244,20 @@ export class McpTransportPool { // `attach` call) left a stale `sessionToEntries[sessionId]` // mapping with no matching `entry.refs.has(sessionId)` — // `releaseSession` would later iterate the stale id and call - // `entry.detach` on a non-attached session. + // `entry.detach` on a non-attached session. W11/PR A: + // `attachPooledSession` is the shared view+attach helper; + // call-site ordering (indexAttach AFTER attach, terminal-state + // self-heal in catch) stays here, not in the helper. try { - const conn = existing.attach(sessionId, view, { - release: () => this.release(id, sessionId), - }); + const conn = this.attachPooledSession( + existing, + id, + serverName, + cfg, + sessionId, + sessionToolRegistry, + sessionPromptRegistry, + ); this.indexAttach(sessionId, id); return conn; } catch (err) { @@ -355,16 +357,11 @@ export class McpTransportPool { sessionPromptRegistry, ); } catch (err) { - // R24 T17: only release if THIS acquire actually reserved a - // new slot. `'already_held'` means the sibling holds it; not - // ours to release. - if ( - this.opts.budget !== undefined && - reservationResult === 'reserved' && - !this.hasNameSibling(serverName) - ) { - this.opts.budget.release(serverName); - } + // R24 T17 (codified by W11/PR A as a shared helper): only + // release if THIS acquire actually reserved a new slot. + // `'already_held'` means the sibling holds it; not ours to + // release. + this.rollbackReservationOnSpawnFailure(reservationResult, serverName); throw err; } } @@ -397,20 +394,14 @@ export class McpTransportPool { // spawn failure (V21-4) so a transient connect failure // doesn't leak the slot until daemon restart. // - // R24 T17: only release if THIS acquire actually reserved a - // new slot (`reservationResult === 'reserved'`). When - // `tryReserve` returned `'already_held'`, a same-name - // sibling held the slot; this acquire reserved nothing, so - // a release here would phantom-decrement the budget counter - // if the sibling were concurrently evicted between - // `tryReserve` and this catch. - if ( - this.opts.budget !== undefined && - reservationResult === 'reserved' && - !this.hasNameSibling(serverName) - ) { - this.opts.budget.release(serverName); - } + // R24 T17 contract (codified as `rollbackReservationOnSpawnFailure` + // helper in W11/PR A): only release if THIS acquire actually + // reserved a new slot (`reservationResult === 'reserved'`). + // `'already_held'` means a sibling holds the slot — phantom- + // releasing here would decrement the counter if the sibling + // were concurrently evicted between `tryReserve` and this + // catch. + this.rollbackReservationOnSpawnFailure(reservationResult, serverName); throw err; }); this.spawnInFlight.set(id, inFlight); @@ -455,17 +446,16 @@ export class McpTransportPool { ); } - const view = new SessionMcpView( - sessionToolRegistry, - sessionPromptRegistry, - sessionId, - serverName, - cfg, - ); try { - const conn = entry.attach(sessionId, view, { - release: () => this.release(id, sessionId), - }); + const conn = this.attachPooledSession( + entry, + id, + serverName, + cfg, + sessionId, + sessionToolRegistry, + sessionPromptRegistry, + ); // F2 (#4175 commit 6 review fix — qwen-latest W111): re-index // AFTER attach succeeds. Pre-fix the early `indexAttach` at the // top of this branch was enough on the unpooled path (W77) @@ -831,6 +821,79 @@ export class McpTransportPool { // ---------- internals ---------- + /** + * F2 (#4175 commit 6 review fix — wenshao W11 / PR A): shared + * view+attach helper for the two POOLED `acquire()` branches (the + * fast-path for an existing entry, and the post-spawn attach after + * `await inFlight`). Pre-fix both branches inlined the same 3-step + * pattern (build view → entry.attach → return) with identical + * release-callback wiring; PR A's stated cleanup goal is to dedupe + * without losing the per-call-site race-window invariant comments + * that explain WHY each branch's surrounding ordering is what it is. + * + * NOT used by `createUnpooledConnection` — the unpooled release + * callback runs `entry.forceShutdown('manual')` directly (no pool + * refcount accounting since unpooled entries are per-session) and + * also calls `indexDetach` from the release callback itself. + * + * Caller is responsible for: + * - Terminal-state pre-check (`!entry.isTerminated()`) + race- + * window self-heal (`evictEntry` on the W125 catch path). + * - Reverse-index ordering (early `indexAttach` BEFORE await on + * the post-spawn branch per W90; AFTER attach on the fast-path + * per W10; W111 re-indexAttach AFTER attach on post-spawn). + * The race-window comments live at the call sites because they + * describe the surrounding ordering, not the attach itself. + */ + private attachPooledSession( + entry: PoolEntry, + id: ConnectionId, + serverName: string, + cfg: MCPServerConfig, + sessionId: string, + sessionToolRegistry: ToolRegistry, + sessionPromptRegistry: PromptRegistry, + ): PooledConnection { + const view = new SessionMcpView( + sessionToolRegistry, + sessionPromptRegistry, + sessionId, + serverName, + cfg, + ); + return entry.attach(sessionId, view, { + release: () => this.release(id, sessionId), + }); + } + + /** + * F2 (#4175 commit 6 review fix — wenshao W11 / PR A; codifies the + * R24 T17 contract): roll back THIS acquire's slot reservation on + * spawn failure. Used by both the unpooled-spawn catch and the + * pooled-spawn-in-flight catch — both decisions are identical: + * - `'reserved'` → THIS acquire newly held the slot; release + * if no sibling holds it + * - `'already_held'` → sibling holds it; never release here (the + * sibling's own onClosed / evictEntry will + * handle it). Pre-R24 the bare + * `!hasNameSibling()` check would phantom- + * release a slot this acquire never reserved + * when the sibling was concurrently evicted. + * - `undefined` → no budget configured; nothing to do. + */ + private rollbackReservationOnSpawnFailure( + reservationResult: 'reserved' | 'already_held' | undefined, + serverName: string, + ): void { + if ( + this.opts.budget !== undefined && + reservationResult === 'reserved' && + !this.hasNameSibling(serverName) + ) { + this.opts.budget.release(serverName); + } + } + /** * F2 (#4175 commit 6 review fix — wenshao R22 W125-followup A+B): * Single source of truth for evicting a pooled entry from From 6cf18f64146122f1f2cb6b501edc12b8acb6c07d Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 22 May 2026 00:49:22 +0800 Subject: [PATCH 3/6] =?UTF-8?q?refactor(core):=20F2=20PR=20A=20W12=20?= =?UTF-8?q?=E2=80=94=20SessionMcpView=20precompute=20filter=20Sets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W12 (filed as F2 follow-up from #4336 review): `applyTools` / `applyPrompts` precompute `excludeSet` + `includeSet` once per pass instead of scanning `cfg.includeTools` / `cfg.excludeTools` arrays inside every per-tool iteration. Pre-fix the per-tool predicate (`passesSessionFilter`) walked both arrays for every snapshot entry → O(M × N) per `applyTools` call. With M tools × N filter entries, typical M=5-20 / N=2-5 case finishes in microseconds either way; the win is data-structure correctness and code clarity, not perceived perf. `passesSessionFilter` / `passesSessionPromptFilter` (the array- based predicates) stay exported and unchanged for unit tests + any caller wanting to test a single name without paying Set construction. The bulk path uses two new private helpers `compileNameFilter` + `compiledFilterAccepts` whose Sets live on the `applyTools` / `applyPrompts` stack frame. Same semantics: `excludeTools` is direct-equality match (no parens strip — pre-F2 behavior preserved); `includeTools` strips the first `(...)` suffix so `toolName(args)` matches `toolName`. Filed bucket: F2 perf cleanup PR A (R9 + W11 done / W12 this commit / R10 to follow). Test sweep: 13/13 session-mcp-view.test.ts pass; typecheck clean. --- packages/core/src/tools/session-mcp-view.ts | 70 ++++++++++++++++++--- 1 file changed, 60 insertions(+), 10 deletions(-) diff --git a/packages/core/src/tools/session-mcp-view.ts b/packages/core/src/tools/session-mcp-view.ts index 9ae2311ddb1..f720b033ff4 100644 --- a/packages/core/src/tools/session-mcp-view.ts +++ b/packages/core/src/tools/session-mcp-view.ts @@ -28,6 +28,51 @@ function passesNameFilter( }); } +/** + * F2 (#4175 commit 6 review fix — wenshao W12 / PR A): precompute + * lookup `Set`s once per `applyTools` / `applyPrompts` pass so the + * per-tool predicate is O(1) instead of repeating the array scan + * inside `passesNameFilter` for every snapshot entry. Same semantics: + * `excludeTools` is direct-equality match (parens form not stripped — + * intentional pre-F2 behavior preserved); `includeTools` strips the + * first `(...)` suffix so `toolName(args)` matches `toolName`. + * + * `passesSessionFilter` / `passesSessionPromptFilter` (the array- + * based predicates exported above) stay unchanged for unit tests + * and any caller that wants to test a single name without paying + * the Set-construction cost. The Sets live on `applyTools` / + * `applyPrompts`'s stack frame. + */ +interface CompiledNameFilter { + excludeSet?: ReadonlySet; + includeSet?: ReadonlySet; +} + +function compileNameFilter( + includeTools?: readonly string[], + excludeTools?: readonly string[], +): CompiledNameFilter { + return { + excludeSet: excludeTools ? new Set(excludeTools) : undefined, + includeSet: includeTools + ? new Set( + includeTools.map((entry) => + entry.includes('(') ? entry.slice(0, entry.indexOf('(')) : entry, + ), + ) + : undefined, + }; +} + +function compiledFilterAccepts( + filter: CompiledNameFilter, + name: string, +): boolean { + if (filter.excludeSet?.has(name)) return false; + if (!filter.includeSet) return true; + return filter.includeSet.has(name); +} + /** * Decide whether a tool from a snapshot passes a session's * include/exclude filter. Exported for unit-testability and so the @@ -127,11 +172,17 @@ export class SessionMcpView { */ applyTools(snapshot: readonly DiscoveredMCPTool[]): void { this.sessionToolRegistry.removeMcpToolsByServer(this.serverName); + // W12/PR A: precompute filter Sets once per pass so the per-tool + // predicate is O(1). Pre-fix `passesSessionFilter` re-scanned the + // includeTools / excludeTools arrays inside every iteration — + // O(M tools × N filter entries) per pass. Same semantics applied. + const filter = compileNameFilter( + this.cfg.includeTools, + this.cfg.excludeTools, + ); let registered = 0; for (const tool of snapshot) { - if ( - !passesSessionFilter(tool, this.cfg.includeTools, this.cfg.excludeTools) - ) { + if (!compiledFilterAccepts(filter, tool.serverToolName)) { continue; } // V21 C7: per-session trust copy. `withTrust` returns the same @@ -176,15 +227,14 @@ export class SessionMcpView { */ applyPrompts(snapshot: readonly DiscoveredMCPPrompt[]): void { this.sessionPromptRegistry.removePromptsByServer(this.serverName); + // W12/PR A: same Set precompute as applyTools. + const filter = compileNameFilter( + this.cfg.includeTools, + this.cfg.excludeTools, + ); let registered = 0; for (const prompt of snapshot) { - if ( - !passesSessionPromptFilter( - prompt.name, - this.cfg.includeTools, - this.cfg.excludeTools, - ) - ) { + if (!compiledFilterAccepts(filter, prompt.name)) { continue; } try { From 2a41c6faee0ed3eb5fd34558489e4039c2e1d786 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 22 May 2026 00:53:44 +0800 Subject: [PATCH 4/6] =?UTF-8?q?perf(core):=20F2=20PR=20A=20R10=20/=20R23?= =?UTF-8?q?=20T7=20=E2=80=94=20pid-descendants=20ps=20snapshot=20+=20pgrep?= =?UTF-8?q?=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R10 / R23 T7 (filed as F2 follow-up from #4336 review): the Linux / macOS pid-descendant enumeration moves from per-pid `pgrep -P ` BFS (one subprocess fork per node visited) to a single `ps -A -o pid=,ppid=` snapshot followed by an in-memory tree walk over `Map`. Windows analog: single `Get-CimInstance Win32_Process | ConvertTo-Csv` snapshot of all `(ProcessId, ParentProcessId)` rows replaces per-pid `Get-CimInstance -Filter "ParentProcessId=$p"` BFS. Two motivations: 1. **Fork count**: typical `npx → tool` / `uvx → tool` wrapper trees are 2-3 levels deep with B=1-3 children per node → pre-fix BFS forked ~5-10 subprocesses per pool-shutdown call. Post-fix: exactly 1 fork regardless of tree depth. 2. **Snapshot consistency**: pre-fix BFS walked the table level by level; a child that forked between two adjacent BFS levels could be missed (we'd see the child but query its descendants AFTER the new fork). The snapshot path captures the table at one instant; new descendants forked after the snapshot are tolerated by the existing ESRCH-tolerant SIGTERM loop. Caveats: - `ps -A -o pid=,ppid=` is POSIX standard (macOS / Linux / *BSD), but BusyBox `ps` ` per-PID-per-level cost | Single `ps -eo pid,ppid` requires platform-specific column flags + tree builder; current 16s worst-case bound is acceptable | +| # | Site | Reason for declining | +| --- | --------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| W7 | Test coverage gaps (4 untested critical paths) | 1/4 added (W6 regression test); rest deferred to focused test-coverage PR after F2 series merges | +| W8 | `maxReconnectAttempts` / `reconnectStrategy` unused | Forward-compat placeholders for the deferred health-monitor-driven reconnect (design §6.6); removing + re-adding churns the public type | +| W11 | Duplicate fast-path / in-flight-path attach blocks | ✅ Done in PR A: `attachPooledSession` + `rollbackReservationOnSpawnFailure` private helpers (commit `2d546efca`) | +| W12 | `passesSessionFilter` O(M×N) per `applyTools` | ✅ Done in PR A: `applyTools` / `applyPrompts` precompute filter `Set`s once per pass; predicate becomes O(1) per tool (commit `a4a855ab3`) | +| R9 | `McpClientManager` ctor 7-positional sentinels | ✅ Done in PR A: options-object ctor + `mkManager` test factory (commit `0cb1eaa27`) | +| R10 | `pgrep -P ` per-PID-per-level cost | ✅ Done in PR A: single `ps -A -o pid=,ppid=` snapshot + in-memory BFS walk; pgrep BFS retained as fallback for BusyBox ` / `Get-CimInstance -Filter` subprocess per node) to a single process-table snapshot followed by in-memory tree walk. Two motivations: (1) one fork instead of B^D forks on the hot pool-shutdown path; (2) snapshot consistency — pre-fix BFS could miss descendants that forked between adjacent BFS levels. Per-pid path retained as fallback for BusyBox `ps` { - if (process.platform === 'win32') return listDescendantPidsWin(rootPid); - return listDescendantPidsUnix(rootPid); + if (!Number.isInteger(rootPid) || rootPid <= 0) return []; + try { + if (process.platform === 'win32') + return await listDescendantPidsWin(rootPid); + return await listDescendantPidsUnix(rootPid); + } catch { + return []; // OS reaps orphans; pool shutdown still proceeds. + } } async function listDescendantPidsUnix(root: number): Promise { - const all: number[] = []; - const queue = [root]; - while (queue.length) { - const parent = queue.shift()!; - const { stdout } = await execFile('pgrep', ['-P', String(parent)], { - timeout: 2000, - }).catch(() => ({ stdout: '' })); - const children = stdout.split('\n').map(Number).filter(Number.isFinite); - all.push(...children); - queue.push(...children); + let tree: Map | undefined; + try { + tree = await snapshotProcessTreeUnix(); // ps -A -o pid=,ppid= + } catch { + /* fall through to fallback */ } - return all; + if (tree) return walkDescendants(tree, root); // O(descendants), 1 fork + return await listDescendantPidsUnixPgrepFallback(root); // legacy BFS } -async function listDescendantPidsWin(root: number): Promise { - // PowerShell CIM query — wmic deprecated on modern Windows - const script = `Get-CimInstance Win32_Process | Where-Object { $_.ParentProcessId -eq } | Select-Object -ExpandProperty ProcessId`; - // ... recursive walk, return aggregated pids +async function snapshotProcessTreeUnix(): Promise> { + // -A: all processes (POSIX, equivalent to -e but unambiguous on BSD). + // -o pid=,ppid=: pid + ppid columns, trailing `=` suppresses headers. + const { stdout } = await execFile('ps', ['-A', '-o', 'pid=,ppid='], { + timeout: 2000, + maxBuffer: 8 * 1024 * 1024, // covers >250k-process pathological hosts + }); + const childrenByPpid = new Map(); + for (const line of stdout.split('\n')) { + const m = line.trim().match(/^(\d+)\s+(\d+)$/); + if (!m) continue; + /* parse, push into childrenByPpid */ + } + return childrenByPpid; } + +// Windows: single Get-CimInstance Win32_Process | ConvertTo-Csv snapshot +// of all (ProcessId, ParentProcessId) rows + in-memory walk; per-pid +// `Get-CimInstance -Filter "ParentProcessId=$p"` retained as fallback. ``` -Called from `PoolEntry.shutdown()` before `client.disconnect()`. Handles `npx @modelcontextprotocol/server-X`, `uvx ...`, `pnpm dlx ...` wrapper leaks. +Called from `PoolEntry.shutdown()` before `client.disconnect()`. Handles `npx @modelcontextprotocol/server-X`, `uvx ...`, `pnpm dlx ...` wrapper leaks. MAX_DESCENDANTS=256 / MAX_DEPTH=8 caps preserved. ### 6.5 Spawn failure handling diff --git a/packages/core/src/tools/pid-descendants.test.ts b/packages/core/src/tools/pid-descendants.test.ts index 1346a567575..cb21d7169c9 100644 --- a/packages/core/src/tools/pid-descendants.test.ts +++ b/packages/core/src/tools/pid-descendants.test.ts @@ -59,13 +59,19 @@ describe('pid-descendants', () => { // Cross-platform integration test: spawn a wrapper that itself // spawns a child, verify listDescendantPids finds both levels. - // Gated on POSIX availability of `pgrep` (skipped on Windows + on - // CI without pgrep). + // + // F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): + // Pre-fix gate skipped on `CI === '1'` (pgrep not always available + // on minimal CI runners). Post-fix the snapshot path uses + // `ps -A -o pid=,ppid=` (POSIX standard, available on every + // non-distroless Linux/macOS), so we keep only the Windows skip; + // the snapshot's per-pid pgrep fallback covers the rare BusyBox + // { - it('enumerates one level of children via pgrep', async () => { + it('enumerates one level of children via process-tree snapshot', async () => { // Parent process spawns a node child with `--eval` that sleeps. // Use spawn directly so we control the lifecycle. const parent = spawn('/bin/sh', [ diff --git a/packages/core/src/tools/pid-descendants.ts b/packages/core/src/tools/pid-descendants.ts index 3b028cdfb4d..cbb789f60c4 100644 --- a/packages/core/src/tools/pid-descendants.ts +++ b/packages/core/src/tools/pid-descendants.ts @@ -12,11 +12,22 @@ const debugLogger = createDebugLogger('PidDescendants'); const execFileAsync = promisify(execFile); /** - * Wall-clock budget for each individual `pgrep` / `Get-CimInstance` call. + * Wall-clock budget for each individual snapshot / per-pid query call. * Bounded so a hung process-table walk can't stall pool shutdown. */ const QUERY_TIMEOUT_MS = 2_000; +/** + * F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): cap + * for `execFile`'s internal stdout buffer on the snapshot path. Default + * is 1MB, which is enough for ~30k-process hosts (~30 bytes/line) but + * an 8MB cap covers >250k-process pathological cases without forcing + * the truncation-or-fallback branch on real machines. The cap applies + * only to the snapshot family of calls; the per-pid `pgrep -P` fallback + * has a tiny output (just the children of one pid) and uses the default. + */ +const SNAPSHOT_MAXBUFFER_BYTES = 8 * 1024 * 1024; + /** * Hard cap on recursion depth + total descendants returned. Defense * against runaway process trees (forkbomb-style) or pathological @@ -35,15 +46,28 @@ const MAX_DEPTH = 8; * `uvx ...`, `pnpm dlx ...`) that would otherwise leak when the * pool entry's primary child is killed. * + * F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): + * the implementation switched from per-pid `pgrep -P ` BFS + * (Linux/macOS) / per-pid `Get-CimInstance -Filter "ParentProcessId=$p"` + * BFS (Windows) — which forked one subprocess per node visited — to + * a single process-table snapshot followed by an in-memory tree walk. + * Two motivations: (1) ~B^D fork count → 1 fork per call, on the + * hot pool-shutdown path; (2) snapshot consistency — pre-fix BFS + * could miss descendants that forked between adjacent BFS levels. + * * Behavior: - * - Linux/macOS: `pgrep -P ` walked recursively, BFS order - * - Windows: PowerShell `Get-CimInstance Win32_Process` filtered - * by `ParentProcessId`, walked recursively - * - Either platform: graceful degradation if the tool is missing - * or the query times out — returns whatever was collected so far - * and logs a warning. Pool shutdown still proceeds; orphan - * processes will be reaped by the OS eventually (Linux init, - * Windows job objects). + * - Linux/macOS: `ps -A -o pid=,ppid=` snapshot, in-memory BFS walk + * over the parsed `Map`. + * - Windows: PowerShell `Get-CimInstance Win32_Process` → + * `ConvertTo-Csv` snapshot of all `(ProcessId, ParentProcessId)` + * rows, in-memory walk. + * - Either platform: graceful degradation if the snapshot tool is + * missing / blocked / times out — falls back to per-pid BFS + * (preserves the pre-fix code path so BusyBox `ps` { } async function listDescendantPidsUnix(root: number): Promise { + let tree: Map | undefined; + try { + tree = await snapshotProcessTreeUnix(); + } catch (err) { + debugLogger.warn( + `Unix snapshot via 'ps -A' failed (${ + err instanceof Error ? err.message : String(err) + }); falling back to per-pid pgrep BFS`, + ); + } + if (tree) { + return walkDescendants(tree, root); + } + return await listDescendantPidsUnixPgrepFallback(root); +} + +async function snapshotProcessTreeUnix(): Promise> { + // `ps -A -o pid=,ppid=` + // -A: all processes (POSIX, equivalent to -e; -A is unambiguous + // across BSD/SysV — BSD historically used -e for env display). + // -o pid=,ppid=: pid + ppid columns; trailing `=` suppresses each + // column header (POSIX standard). + // Output is " " per line, no header. + const { stdout } = await execFileAsync('ps', ['-A', '-o', 'pid=,ppid='], { + timeout: QUERY_TIMEOUT_MS, + maxBuffer: SNAPSHOT_MAXBUFFER_BYTES, + }); + const childrenByPpid = new Map(); + let parsedRows = 0; + for (const line of stdout.split('\n')) { + const m = line.trim().match(/^(\d+)\s+(\d+)$/); + if (!m) continue; + const pid = Number.parseInt(m[1], 10); + const ppid = Number.parseInt(m[2], 10); + if (!Number.isFinite(pid) || pid <= 0) continue; + if (!Number.isFinite(ppid) || ppid < 0) continue; + parsedRows += 1; + const arr = childrenByPpid.get(ppid); + if (arr) arr.push(pid); + else childrenByPpid.set(ppid, [pid]); + } + if (parsedRows === 0) { + // Snapshot tool ran but produced no parseable lines (e.g. BusyBox + // `ps` without `-o` support echoing usage). Treat as failure so + // the caller's catch falls back to per-pid pgrep. + throw new Error( + `'ps -A -o pid=,ppid=' returned no parseable rows (stdout length=${stdout.length})`, + ); + } + return childrenByPpid; +} + +async function listDescendantPidsUnixPgrepFallback( + root: number, +): Promise { const all: number[] = []; const queue: Array<{ pid: number; depth: number }> = [ { pid: root, depth: 0 }, @@ -107,6 +186,65 @@ async function listDescendantPidsUnix(root: number): Promise { } async function listDescendantPidsWin(root: number): Promise { + let tree: Map | undefined; + try { + tree = await snapshotProcessTreeWin(); + } catch (err) { + debugLogger.warn( + `Windows snapshot via Get-CimInstance failed (${ + err instanceof Error ? err.message : String(err) + }); falling back to per-pid filter BFS`, + ); + } + if (tree) { + return walkDescendants(tree, root); + } + return await listDescendantPidsWinPerPidFallback(root); +} + +async function snapshotProcessTreeWin(): Promise> { + // Single-shot CIM query for ALL processes' (ProcessId, + // ParentProcessId), CSV-formatted for stable parsing. + // F2 (#4175 commit 5 review fix — wenshao R5): no integer + // interpolation into the script; this query takes no parameters + // (we filter in-memory after the snapshot returns). + const script = + 'Get-CimInstance -ClassName Win32_Process ' + + '| Select-Object ProcessId,ParentProcessId ' + + '| ConvertTo-Csv -NoTypeInformation'; + const { stdout } = await execFileAsync( + 'powershell.exe', + ['-NoProfile', '-NonInteractive', '-Command', script], + { timeout: QUERY_TIMEOUT_MS, maxBuffer: SNAPSHOT_MAXBUFFER_BYTES }, + ); + const childrenByPpid = new Map(); + let parsedRows = 0; + // CSV: first line is header `"ProcessId","ParentProcessId"`, + // subsequent lines are `"",""`. + const lines = stdout.split(/\r?\n/); + for (let i = 1; i < lines.length; i++) { + const m = lines[i].match(/^"(\d+)","(\d+)"$/); + if (!m) continue; + const pid = Number.parseInt(m[1], 10); + const ppid = Number.parseInt(m[2], 10); + if (!Number.isFinite(pid) || pid <= 0) continue; + if (!Number.isFinite(ppid) || ppid < 0) continue; + parsedRows += 1; + const arr = childrenByPpid.get(ppid); + if (arr) arr.push(pid); + else childrenByPpid.set(ppid, [pid]); + } + if (parsedRows === 0) { + throw new Error( + `Get-CimInstance snapshot returned no parseable rows (stdout length=${stdout.length})`, + ); + } + return childrenByPpid; +} + +async function listDescendantPidsWinPerPidFallback( + root: number, +): Promise { const all: number[] = []; const queue: Array<{ pid: number; depth: number }> = [ { pid: root, depth: 0 }, @@ -161,6 +299,32 @@ async function listDescendantPidsWin(root: number): Promise { return all; } +/** + * F2 (#4175 commit 6 review fix — wenshao R10 / R23 T7 / PR A): + * shared in-memory BFS over a snapshot tree. Replaces both + * platforms' per-node subprocess forks once the snapshot has been + * obtained. Same MAX_DESCENDANTS / MAX_DEPTH caps as the legacy + * fallback path. Returns BFS order — children before grandchildren. + */ +function walkDescendants(tree: Map, root: number): number[] { + const all: number[] = []; + const queue: Array<{ pid: number; depth: number }> = [ + { pid: root, depth: 0 }, + ]; + while (queue.length && all.length < MAX_DESCENDANTS) { + const { pid, depth } = queue.shift()!; + if (depth >= MAX_DEPTH) continue; + const children = tree.get(pid); + if (!children) continue; + for (const child of children) { + if (all.length >= MAX_DESCENDANTS) break; + all.push(child); + queue.push({ pid: child, depth: depth + 1 }); + } + } + return all; +} + /** * Send SIGTERM to a list of pids, tolerating per-pid failures * (already exited, permission denied, etc.). On Windows, Node's From ced5d62b00b4df425c43f761d1d33ad5d21d595e Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 22 May 2026 22:15:35 +0800 Subject: [PATCH 5/6] =?UTF-8?q?refactor(core):=20F2=20PR=20A=20R2=20?= =?UTF-8?q?=E2=80=94=20wenshao=20followup=20(visited=20set=20+=20dedup=20p?= =?UTF-8?q?redicate)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Suggestions from wenshao's first PR #4411 review pass (07:15Z), both small and worth folding before merge: PR-A-R2 #1 (pid-descendants.ts:309 — walkDescendants visited set): `walkDescendants`'s BFS lacked a `visited` set. If the snapshot captures a PID-reuse cycle — rare but possible on busy hosts with rapid pid churn between `ps -A`'s start and parse, where Linux wraparound can show a freed pid in a different parent's children list creating an A→B / B→A cycle — pre-fix BFS would revisit nodes and fill the MAX_DESCENDANTS=256 quota with duplicate entries, starving legitimate descendants. Pre-PR-A the per-pid `pgrep` BFS had the same theoretical issue but was less exposed (each `pgrep -P pid` call returns only DIRECT children; snapshot captures the whole tree at once, making cycles instantly visible). Fix: 3-LOC `Set` add. `root` seeded into `visited` so a malformed snapshot listing root as a descendant of its own child doesn't re-enqueue root either. PR-A-R2 #2 (session-mcp-view.ts:117 — predicate dedup): After W12, the exported `passesSessionFilter` / `passesSessionPromptFilter` still called `passesNameFilter` (the pre-W12 array-based implementation), while `applyTools` / `applyPrompts` used `compiledFilterAccepts(compileNameFilter(...))`. Two parallel implementations of the same predicate — future change to one without the other would silently diverge: - the exported function's tests (passesSessionFilter unit tests) would still pass - the production filter path in applyTools/applyPrompts would behave differently Reviewer also noted `passesSessionPromptFilter` had zero callers in production code or tests after W12 — `applyPrompts` no longer references it. Kept the export rather than deleting it (matches the `passesSessionFilter` shape for symmetry + the F3 audit-path comment block earmarks both as the replay predicates), but routed both through `compiledFilterAccepts(compileNameFilter(...))` so there is a single source of truth. Set construction is per-call for these exports (negligible for unit-test / one-off probes); the bulk paths in `applyTools` / `applyPrompts` still construct ONE filter per pass via the original W12 code path. `passesNameFilter` (the standalone array-based helper) deleted — its only callers were the two exports, which now use the compiled path. Public-API surface unchanged: the two exported functions keep their signatures and semantics. Test sweep: 19/19 pid-descendants + session-mcp-view tests pass; typecheck + ESLint clean. Continues commit chain: f05917071 (R9) → 20d2f1b90 (W11) → 6cf18f641 (W12) → 2a41c6fae (R10) → this (R2 followups). --- packages/core/src/tools/pid-descendants.ts | 17 ++++++ packages/core/src/tools/session-mcp-view.ts | 63 +++++++++++---------- 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/packages/core/src/tools/pid-descendants.ts b/packages/core/src/tools/pid-descendants.ts index cbb789f60c4..dae89f79364 100644 --- a/packages/core/src/tools/pid-descendants.ts +++ b/packages/core/src/tools/pid-descendants.ts @@ -305,9 +305,24 @@ async function listDescendantPidsWinPerPidFallback( * platforms' per-node subprocess forks once the snapshot has been * obtained. Same MAX_DESCENDANTS / MAX_DEPTH caps as the legacy * fallback path. Returns BFS order — children before grandchildren. + * + * F2 (#4175 commit 6 review fix — wenshao PR-A-R2 #1): `visited` + * set prevents BFS revisits when the snapshot captures a PID-reuse + * cycle (rare but possible on busy hosts with rapid pid churn + * between snapshot start and parse — Linux pid wraparound can make + * `ps -A` show a freed pid in a different parent's children list, + * producing an A→B / B→A cycle). Pre-fix the cycle would fill the + * MAX_DESCENDANTS=256 quota with duplicate entries and starve + * legitimate descendants. The per-pid `pgrep` BFS fallback had the + * same theoretical issue but was less exposed because each + * `pgrep -P pid` call only returns DIRECT children; the snapshot + * captures the whole tree at once. `root` is seeded into `visited` + * so a malformed snapshot listing root as a descendant of one of + * its own children doesn't re-enqueue root. */ function walkDescendants(tree: Map, root: number): number[] { const all: number[] = []; + const visited = new Set([root]); const queue: Array<{ pid: number; depth: number }> = [ { pid: root, depth: 0 }, ]; @@ -317,6 +332,8 @@ function walkDescendants(tree: Map, root: number): number[] { const children = tree.get(pid); if (!children) continue; for (const child of children) { + if (visited.has(child)) continue; + visited.add(child); if (all.length >= MAX_DESCENDANTS) break; all.push(child); queue.push({ pid: child, depth: depth + 1 }); diff --git a/packages/core/src/tools/session-mcp-view.ts b/packages/core/src/tools/session-mcp-view.ts index f720b033ff4..c6b37535c22 100644 --- a/packages/core/src/tools/session-mcp-view.ts +++ b/packages/core/src/tools/session-mcp-view.ts @@ -13,35 +13,26 @@ import type { ToolRegistry } from './tool-registry.js'; const debugLogger = createDebugLogger('McpPool:View'); -function passesNameFilter( - name: string, - includeTools?: readonly string[], - excludeTools?: readonly string[], -): boolean { - if (excludeTools?.includes(name)) return false; - if (!includeTools) return true; - return includeTools.some((entry) => { - const stripped = entry.includes('(') - ? entry.slice(0, entry.indexOf('(')) - : entry; - return stripped === name; - }); -} - /** - * F2 (#4175 commit 6 review fix — wenshao W12 / PR A): precompute - * lookup `Set`s once per `applyTools` / `applyPrompts` pass so the - * per-tool predicate is O(1) instead of repeating the array scan - * inside `passesNameFilter` for every snapshot entry. Same semantics: - * `excludeTools` is direct-equality match (parens form not stripped — - * intentional pre-F2 behavior preserved); `includeTools` strips the - * first `(...)` suffix so `toolName(args)` matches `toolName`. + * F2 (#4175 commit 6 review fix — wenshao W12 / PR A; PR-A-R2 #2 + * folded the exports to delegate here): precompute lookup `Set`s + * once per `applyTools` / `applyPrompts` pass so the per-tool + * predicate is O(1) instead of repeating an array scan for every + * snapshot entry. Same semantics: `excludeTools` is direct-equality + * match (parens form not stripped — intentional pre-F2 behavior + * preserved); `includeTools` strips the first `(...)` suffix so + * `toolName(args)` matches `toolName`. * - * `passesSessionFilter` / `passesSessionPromptFilter` (the array- - * based predicates exported above) stay unchanged for unit tests - * and any caller that wants to test a single name without paying - * the Set-construction cost. The Sets live on `applyTools` / - * `applyPrompts`'s stack frame. + * PR-A-R2 #2: `passesSessionFilter` / `passesSessionPromptFilter` + * (exported below for unit-testability) now route THROUGH + * `compiledFilterAccepts(compileNameFilter(...))` so there is a + * single source of truth for the predicate. Pre-fix the exports + * called a separate `passesNameFilter` array-based implementation + * with the same semantics, creating a drift risk where a future + * change to one impl wouldn't be caught by tests of the other. + * The Set construction is per-call for these exports (cheap for + * tests / one-off probes); the bulk paths in + * `applyTools`/`applyPrompts` still construct ONE filter per pass. */ interface CompiledNameFilter { excludeSet?: ReadonlySet; @@ -91,13 +82,21 @@ function compiledFilterAccepts( * support, intentionally matching the existing pre-F2 behavior so * operators don't see semantic divergence between the two filter * lists when migrating sessions through pool mode. + * + * PR-A-R2 #2: routes through `compiledFilterAccepts(compileNameFilter(...))` + * so the bulk-path predicate and the exported per-name predicate + * share one implementation. Set construction is paid per call here + * (negligible for unit tests / one-off audit-path probes). */ export function passesSessionFilter( tool: DiscoveredMCPTool, includeTools?: readonly string[], excludeTools?: readonly string[], ): boolean { - return passesNameFilter(tool.serverToolName, includeTools, excludeTools); + return compiledFilterAccepts( + compileNameFilter(includeTools, excludeTools), + tool.serverToolName, + ); } /** @@ -113,13 +112,19 @@ export function passesSessionFilter( * parens form `excludeTools: ['toolName(args)']` which only matches * tools (the parens-stripping in `passesSessionFilter` matches * `toolName` in the include list, not the exclude list). + * + * PR-A-R2 #2: same delegation to the compiled path as + * `passesSessionFilter`. */ export function passesSessionPromptFilter( promptName: string, includeTools?: readonly string[], excludeTools?: readonly string[], ): boolean { - return passesNameFilter(promptName, includeTools, excludeTools); + return compiledFilterAccepts( + compileNameFilter(includeTools, excludeTools), + promptName, + ); } /** From 5dec00ff98e7dcbf4ef445e7dcd10448b9dc0b09 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 22 May 2026 22:44:00 +0800 Subject: [PATCH 6/6] =?UTF-8?q?fix(core):=20F2=20PR=20A=20R3=20T3=20?= =?UTF-8?q?=E2=80=94=20Windows=20CSV=20delimiter=20locale=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ConvertTo-Csv -NoTypeInformation` honors the system locale's list separator on PowerShell 5.1. On German / French / Dutch / Italian / ... locales the separator is `;` not `,`, so the regex `^"(\d+)","(\d+)"$` in `snapshotProcessTreeWin` never matched → `parsedRows === 0` → snapshot threw → fell back to the per-pid CIM filter path with ~0.5-1s extra PowerShell startup latency per descendant on every pool shutdown. Fix: 1-LOC `-Delimiter ","` on `ConvertTo-Csv`. Forces comma regardless of locale or PowerShell version. PowerShell 7+ defaults to comma already; 5.1 (the Windows-bundled version most users have without explicit upgrade) honored locale. The explicit delimiter makes both consistent. Skipped wenshao's companion Suggestion T4 (test coverage for walkDescendants MAX_DESCENDANTS / MAX_DEPTH caps) as F2 hardening follow-up — the caps are simple 2-line guards exercisable by inspection; ~50 LOC of mock infrastructure isn't commensurate with the regression risk on currently-stable defensive code, and (per the issue #4175 follow-up bucket) we keep dedicated test-coverage work out of perf-cleanup PRs. Continues commit chain: f05917071 (R9) → 20d2f1b90 (W11) → 6cf18f641 (W12) → 2a41c6fae (R10) → ced5d62b0 (R2) → this (R3 T3). Test sweep: 6/6 pid-descendants tests pass; typecheck + ESLint clean. --- packages/core/src/tools/pid-descendants.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/core/src/tools/pid-descendants.ts b/packages/core/src/tools/pid-descendants.ts index dae89f79364..363e5683e37 100644 --- a/packages/core/src/tools/pid-descendants.ts +++ b/packages/core/src/tools/pid-descendants.ts @@ -208,10 +208,20 @@ async function snapshotProcessTreeWin(): Promise> { // F2 (#4175 commit 5 review fix — wenshao R5): no integer // interpolation into the script; this query takes no parameters // (we filter in-memory after the snapshot returns). + // + // F2 (#4175 commit 6 review fix — wenshao PR-A-R3 T3): explicit + // `-Delimiter ","` on `ConvertTo-Csv`. Pre-fix PowerShell 5.1 + // honored the system locale's list separator (semicolon on + // German / French / Dutch / etc.), so the regex + // `^"(\d+)","(\d+)"$` below never matched on those locales → + // snapshot threw → fell back to the slower per-pid CIM path + // (~0.5-1s extra PowerShell startup latency per descendant on + // every shutdown). Forcing comma normalizes the output across + // locales / PS versions. const script = 'Get-CimInstance -ClassName Win32_Process ' + '| Select-Object ProcessId,ParentProcessId ' + - '| ConvertTo-Csv -NoTypeInformation'; + '| ConvertTo-Csv -NoTypeInformation -Delimiter ","'; const { stdout } = await execFileAsync( 'powershell.exe', ['-NoProfile', '-NonInteractive', '-Command', script],