fix(core): refresh MCP session metadata without reconnecting

docs/design/mcp-session-metadata-hot-reload.md

@@ -0,0 +1,50 @@
+# MCP session metadata hot reload
+
+## Problem
+
+MCP transport identity deliberately excludes `trust`, `includeTools`, `excludeTools`, and `alwaysLoadTools` so daemon sessions with different policies can share one healthy transport. Settings reconciliation previously treated a matching transport fingerprint as a complete no-op. The attached session view therefore kept stale tool and prompt registrations. The same omission existed in same-fingerprint runtime replacement.
+
+Unpooled connections exposed a different mismatch: their public lifecycle id (`name::unpooled-N`) was compared with the desired transport fingerprint (`name::fingerprint`). A healthy unpooled connection was consequently replaced on every reconciliation pass.
+
+## Invariants
+
+1. Transport-affecting changes reconnect; metadata-only changes do not.
+2. Pooled and unpooled handles expose both a unique lifecycle id and the transport identity captured when the transport was created.
+3. Session metadata is projected from the canonical discovery snapshot without mutating it. Sessions sharing a transport may independently choose filters, trust, and eager loading.
+4. Equivalent metadata does not churn registries.
+5. Runtime replacement and settings reconciliation use the same refresh behavior.
+
+## Metadata identity
+
+The session metadata key has these canonical rules:
+
+| Setting           | Canonical behavior                                                                                                                                                              |
+| ----------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `trust`           | Missing, `false`, and `true` remain distinct.                                                                                                                                   |
+| `alwaysLoadTools` | Only `true` enables eager loading; missing and `false` are equivalent.                                                                                                          |
+| `includeTools`    | Presence is significant: missing allows all tools and `[]` allows none. Order and duplicates are ignored, and `foo(args)` is equivalent to `foo`, matching discovery filtering. |
+| `excludeTools`    | Missing and `[]` are equivalent. Order and duplicates are ignored; names remain exact matches.                                                                                  |
+
+The key is captured by each session view instead of recomputed from its previous config reference. This detects a settings object mutated in place. Transport identity is likewise captured by each pool entry so mutating the caller-owned config cannot make an old transport appear current.
+
+## Refresh flow
+
+During pooled reconciliation, each held connection is compared through its captured transport identity. A mismatch releases and reacquires the connection. A match calls the existing handle's metadata refresh method. The handle updates only its attached `SessionMcpView`; when the canonical metadata key changes, the view replays the entry's current tool, prompt, and resource snapshots. Equivalent settings return without registry changes.
+
+The runtime add-or-replace path performs the same handle refresh after updating the runtime overlay when the transport identity matches.
+
+`alwaysLoadTools` is projected alongside trust by cloning the discovered tool for the session. The canonical snapshot retains its original value and remains safe to share. Legacy single-session discovery includes the same canonical metadata key in its connected-config identity, so changes reconnect and rediscover there.
+
+## Lifecycle behavior
+
+Refreshing a released handle or a terminal entry fails closed. A live entry can accept the new view config while a restart is in progress; the normal restart snapshot fan-out then applies that config to the refreshed discovery result. Refresh never calls `acquire` again for the same session, avoiding handle replacement and reference-count ambiguity.
+
+## Compatibility and scope
+
+No user setting, MCP wire message, transport fingerprint, or daemon API shape changes. Existing sessions gain immediate metadata updates, and unpooled transports stop reconnecting on metadata-only or equivalent settings changes. Transport changes, disabled servers, approval changes, and explicit restarts retain their existing teardown behavior.
+
+Atomic filesystem configuration reloads, MCP server-driven list-change behavior, and new metadata fields are out of scope.
+
+## Verification
+
+Unit and integration coverage exercises pooled refresh, unpooled identity, same-fingerprint runtime replacement, legacy reconnect keys, equivalent filter normalization, absent-versus-empty include lists, in-place config mutation, per-session `alwaysLoadTools`, and shared-snapshot isolation. A real local stdio MCP harness additionally verifies transport reuse, metadata projection, an actual tool call, and zero churn for canonical-equivalent settings.

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

@@ -591,6 +591,141 @@ describe('McpClientManager', () => {
     expect(McpClient).not.toHaveBeenCalled();
   });
 
+  it('refreshes metadata on a retained unpooled connection without transport churn', async () => {
+    let serverConfig = {
+      command: 'node',
+      includeTools: ['first'],
+    } as MCPServerConfig;
+    const transportId = connectionIdOf('srv', serverConfig);
+    const release = vi.fn();
+    const updateConfig = vi.fn();
+    const connection = {
+      release,
+      updateConfig,
+      on: vi.fn(),
+      off: vi.fn(),
+      id: 'srv::unpooled-0',
+      transportId,
+      serverName: 'srv',
+      entryIndex: 0,
+      toolsSnapshot: [],
+      promptsSnapshot: [],
+      resourcesSnapshot: [],
+    };
+    const acquire = vi.fn().mockResolvedValue(connection);
+    const fakePool = {
+      acquire,
+      releaseSession: vi.fn(),
+      getBudget: vi.fn().mockReturnValue(undefined),
+    } as unknown as import('./mcp-transport-pool.js').McpTransportPool;
+    const mockConfig = {
+      isTrustedFolder: () => true,
+      getMcpServers: () => ({ srv: serverConfig }),
+      getMcpServerCommand: () => undefined,
+      getTargetDir: () => '/session/worktree',
+      getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }),
+      getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }),
+      getWorkspaceContext: () => ({}),
+      getDebugMode: () => false,
+      getSessionId: () => 'sid-1',
+      isMcpServerDisabled: () => false,
+    } as unknown as Config;
+    const manager = mkManager({
+      config: mockConfig,
+      options: { pool: fakePool },
+    });
+
+    await manager.discoverAllMcpTools(mockConfig);
+    serverConfig = {
+      command: 'node',
+      includeTools: ['second'],
+      trust: true,
+      alwaysLoadTools: true,
+    } as MCPServerConfig;
+    await manager.discoverAllMcpTools(mockConfig);
+
+    expect(acquire).toHaveBeenCalledTimes(1);
+    expect(release).not.toHaveBeenCalled();
+    expect(updateConfig).toHaveBeenCalledOnce();
+    expect(updateConfig).toHaveBeenCalledWith(serverConfig);
+
+    serverConfig = { command: 'different-node' } as MCPServerConfig;
+    await manager.discoverAllMcpTools(mockConfig);
+    expect(acquire).toHaveBeenCalledTimes(2);
+    expect(release).toHaveBeenCalledOnce();
+  });
+
+  it('isolates retained connection metadata refresh failures between servers', async () => {
+    let serverConfigs = {
+      srvA: { command: 'node', includeTools: ['first-a'] } as MCPServerConfig,
+      srvB: { command: 'node', includeTools: ['first-b'] } as MCPServerConfig,
+    };
+    const updateA = vi.fn();
+    const updateB = vi.fn();
+    const connections = {
+      srvA: {
+        release: vi.fn(),
+        updateConfig: updateA,
+        on: vi.fn(),
+        off: vi.fn(),
+        id: 'srvA::unpooled-0',
+        transportId: connectionIdOf('srvA', serverConfigs.srvA),
+        serverName: 'srvA',
+        entryIndex: 0,
+      },
+      srvB: {
+        release: vi.fn(),
+        updateConfig: updateB,
+        on: vi.fn(),
+        off: vi.fn(),
+        id: 'srvB::unpooled-0',
+        transportId: connectionIdOf('srvB', serverConfigs.srvB),
+        serverName: 'srvB',
+        entryIndex: 0,
+      },
+    };
+    const acquire = vi.fn((name: 'srvA' | 'srvB') =>
+      Promise.resolve(connections[name]),
+    );
+    const fakePool = {
+      acquire,
+      releaseSession: vi.fn(),
+      getBudget: vi.fn().mockReturnValue(undefined),
+    } as unknown as import('./mcp-transport-pool.js').McpTransportPool;
+    const mockConfig = {
+      isTrustedFolder: () => true,
+      getMcpServers: () => serverConfigs,
+      getMcpServerCommand: () => undefined,
+      getTargetDir: () => '/session/worktree',
+      getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }),
+      getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }),
+      getWorkspaceContext: () => ({}),
+      getDebugMode: () => false,
+      getSessionId: () => 'sid-1',
+      isMcpServerDisabled: () => false,
+    } as unknown as Config;
+    const manager = mkManager({
+      config: mockConfig,
+      options: { pool: fakePool },
+    });
+    await manager.discoverAllMcpTools(mockConfig);
+
+    serverConfigs = {
+      srvA: { command: 'node', includeTools: ['second-a'] } as MCPServerConfig,
+      srvB: { command: 'node', includeTools: ['second-b'] } as MCPServerConfig,
+    };
+    updateA.mockImplementationOnce(() => {
+      throw new Error('refresh A failed');
+    });
+
+    await expect(manager.discoverAllMcpTools(mockConfig)).resolves.toBe(
+      undefined,
+    );
+    expect(updateA).toHaveBeenCalledWith(serverConfigs.srvA);
+    expect(updateB).toHaveBeenCalledWith(serverConfigs.srvB);
+    expect(acquire).toHaveBeenCalledTimes(2);
+  });
+
   it('routes single-server discovery through the pool when injected', async () => {
     const acquireSpy = vi.fn().mockResolvedValue({
       release: vi.fn(),
@@ -1824,6 +1959,92 @@ describe('McpClientManager', () => {
     expect(mockedMcpClient.connect).toHaveBeenCalledTimes(2);
   });
 
+  it('normalizes duplicate filters but reconnects when alwaysLoadTools changes', async () => {
+    const { MCPServerStatus } = await import('./mcp-client.js');
+    const mockedMcpClient = {
+      connect: vi.fn().mockResolvedValue(undefined),
+      discover: vi.fn().mockResolvedValue(undefined),
+      disconnect: vi.fn().mockResolvedValue(undefined),
+      getStatus: vi.fn().mockReturnValue(MCPServerStatus.CONNECTED),
+    };
+    vi.mocked(McpClient).mockReturnValue(
+      mockedMcpClient as unknown as McpClient,
+    );
+
+    let serverConfig = {
+      command: 'node',
+      includeTools: ['alpha(args)', 'alpha(args)', 'beta'],
+      alwaysLoadTools: false,
+    } as MCPServerConfig;
+    const mockConfig = {
+      isTrustedFolder: () => true,
+      getMcpServers: () => ({ foo: serverConfig }),
+      getMcpServerCommand: () => undefined,
+      getTargetDir: () => '/session/worktree',
+      getPromptRegistry: () =>
+        ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry,
+      getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }),
+      getWorkspaceContext: () => ({}) as WorkspaceContext,
+      getDebugMode: () => false,
+      isMcpServerDisabled: () => false,
+    } as unknown as Config;
+    const manager = mkManager({ config: mockConfig });
+
+    await manager.discoverAllMcpToolsIncremental(mockConfig);
+    serverConfig = {
+      command: 'node',
+      includeTools: ['beta', 'alpha'],
+      alwaysLoadTools: false,
+    } as MCPServerConfig;
+    await manager.discoverAllMcpToolsIncremental(mockConfig);
+    expect(mockedMcpClient.disconnect).not.toHaveBeenCalled();
+
+    serverConfig = { ...serverConfig, alwaysLoadTools: true };
+    await manager.discoverAllMcpToolsIncremental(mockConfig);
+    expect(mockedMcpClient.disconnect).toHaveBeenCalledTimes(1);
+    expect(mockedMcpClient.connect).toHaveBeenCalledTimes(2);
+  });
+
+  it('reconnects legacy discovery when includeTools changes from absent to empty', async () => {
+    const { MCPServerStatus } = await import('./mcp-client.js');
+    const mockedMcpClient = {
+      connect: vi.fn().mockResolvedValue(undefined),
+      discover: vi.fn().mockResolvedValue(undefined),
+      disconnect: vi.fn().mockResolvedValue(undefined),
+      getStatus: vi.fn().mockReturnValue(MCPServerStatus.CONNECTED),
+    };
+    vi.mocked(McpClient).mockReturnValue(
+      mockedMcpClient as unknown as McpClient,
+    );
+
+    const settings: { includeTools?: string[] } = {};
+    const mockConfig = {
+      isTrustedFolder: () => true,
+      getMcpServers: () => ({
+        foo: {
+          command: 'node',
+          includeTools: settings.includeTools,
+        } as MCPServerConfig,
+      }),
+      getMcpServerCommand: () => undefined,
+      getTargetDir: () => '/session/worktree',
+      getPromptRegistry: () =>
+        ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry,
+      getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }),
+      getWorkspaceContext: () => ({}) as WorkspaceContext,
+      getDebugMode: () => false,
+      isMcpServerDisabled: () => false,
+    } as unknown as Config;
+    const manager = mkManager({ config: mockConfig });
+
+    await manager.discoverAllMcpToolsIncremental(mockConfig);
+    settings.includeTools = [];
+    await manager.discoverAllMcpToolsIncremental(mockConfig);
+
+    expect(mockedMcpClient.disconnect).toHaveBeenCalledTimes(1);
+    expect(mockedMcpClient.connect).toHaveBeenCalledTimes(2);
+  });
+
   it('reconnects a server first connected via the bulk path when its config later changes', async () => {
     // Regression: the bulk `discoverAllMcpTools` path (reached via legacy
     // blocking boot + extension reload) used to connect WITHOUT recording a
@@ -4192,7 +4413,7 @@ describe('McpClientManager — addRuntimeMcpServer / removeRuntimeMcpServer (T2.
     expect(fakePool.acquire).not.toHaveBeenCalled();
   });
 
-  it('case 4: replace same name + same fingerprint → replaced=true, pool.acquire NOT re-called', async () => {
+  it('case 4: same-fingerprint runtime replace refreshes metadata without re-acquiring', async () => {
     const serverConfig = {
       command: 'echo',
       args: ['hi'],
@@ -4202,10 +4423,16 @@ describe('McpClientManager — addRuntimeMcpServer / removeRuntimeMcpServer (T2.
     const realId = connectionIdOf('dup-srv', serverConfig);
 
     const releaseSpyConn1 = vi.fn();
+    const updateConfig = vi.fn();
     const conn1 = {
       release: releaseSpyConn1,
+      updateConfig,
       on: vi.fn(),
-      id: realId,
+      // Distinct lifecycle id: if the same-fingerprint comparison below
+      // regressed from `transportId` back to `id`, the replace would tear
+      // down and re-acquire the transport, and this test would catch it.
+      id: 'dup-srv::unpooled-0',
+      transportId: realId,
       serverName: 'dup-srv',
       entryIndex: 0,
       toolsSnapshot: [
@@ -4223,26 +4450,56 @@ describe('McpClientManager — addRuntimeMcpServer / removeRuntimeMcpServer (T2.
     } as unknown as import('./mcp-transport-pool.js').McpTransportPool;
 
     const config = mkRuntimeConfig();
-    const manager = mkManager({ config, options: { pool: fakePool } });
+    // The refresh re-filters the session; the reported count must be the
+    // session-visible one, not the unfiltered snapshot size.
+    const sessionTools = [{ name: 'tool-b' }];
+    const toolRegistry = {
+      removeMcpToolsByServer: vi.fn(),
+      getToolsByServer: vi.fn().mockReturnValue(sessionTools),
+    } as unknown as ToolRegistry;
+    const manager = mkManager({
+      config,
+      toolRegistry,
+      options: { pool: fakePool },
+    });
 
     // First add
     await manager.addRuntimeMcpServer('dup-srv', serverConfig, 'client-4');
     expect(acquireSpy).toHaveBeenCalledTimes(1);
 
-    // Second add with SAME config (same fingerprint)
+    // Second add changes only per-session metadata, so the transport
+    // fingerprint remains identical while the session view must refresh.
     acquireSpy.mockClear();
+    const updatedConfig = {
+      ...serverConfig,
+      includeTools: ['tool-b'],
+      trust: true,
+      alwaysLoadTools: true,
+    } as MCPServerConfig;
     const result = await manager.addRuntimeMcpServer(
       'dup-srv',
-      serverConfig,
+      updatedConfig,
       'client-4',
     );
 
-    // pool.acquire should NOT have been re-called (idempotent no-op)
+    // The existing handle refreshes in place; the transport is not reacquired.
     expect(acquireSpy).not.toHaveBeenCalled();
+    expect(updateConfig).toHaveBeenCalledOnce();
+    expect(updateConfig).toHaveBeenCalledWith(updatedConfig);
+    // The overlay write persists the refresh across reconciliations, and it
+    // lands AFTER the refresh so a throwing refresh cannot persist config
+    // the session view never received.
+    const addRuntimeSpy = config.addRuntimeMcpServer as ReturnType<
+      typeof vi.fn
+    >;
+    expect(addRuntimeSpy).toHaveBeenLastCalledWith('dup-srv', updatedConfig);
+    expect(updateConfig.mock.invocationCallOrder[0]).toBeLessThan(
+      addRuntimeSpy.mock.invocationCallOrder.at(-1)!,
+    );
     expect(result).toMatchObject({
       name: 'dup-srv',
       replaced: false,
-      toolCount: 3,
+      toolCount: 1,
     });
   });