fix(serve): address rounds 5-7 review items — build, security, correctness (T2.8 #4514)

Build breakers (Critical):
- events.ts: add missing /** JSDoc opener for DaemonMcpServerAddedData
- events.ts: add missing `: undefined` arm in followup_suggestion ternary
- events.ts: close isFollowupSuggestionData function body (missing ); })

Security (Critical):
- acpAgent: strip authProviderType, includeTools, excludeTools, cwd from
  runtime-added server configs (prevents SSRF via cloud creds leak and
  arbitrary cwd spawn)
- mcp-client-manager: reject servers in excludedMcpServers blocklist
- acpAgent: add Array.isArray guard to config validation

Correctness:
- mcp-client-manager: identity-check on pooledConnections.delete in remove
  (prevents concurrent add+remove race deleting NEW pool entry)
- mcp-client-manager: add client.disconnect() in catch block for
  standalone path (prevents transport/process leak)
- mcp-client-manager: add consecutiveFailures, isReconnecting,
  dropRefusalEntry cleanup in removeRuntimeMcpServer
- mcp-client-manager: emit mcp-client-update on spawn failure cleanup
- mcp-client-manager: extract exitCode from error when available
- mcp-client-manager: fix replaced=true → false for same-fingerprint
  idempotent re-add (no transport was torn down)
- server.ts: whitelist error fields in sendBridgeError responses
  (prevent unbounded internal ACP data spread)
- bridge.ts: remove dead try/catch in addRuntimeMcpServer (all branches
  just re-threw)
- bridge.ts: add try/catch to removeRuntimeMcpServer for error mapping
- bridge.ts: narrow AddOk.transport to literal union type

SDK / DX:
- DaemonClient: add timeoutMs param to addRuntimeMcpServer (default 330s,
  matching restartMcpServer — prevents 30s SDK timeout vs 5min bridge)
- mcp-client-manager: add debugLogger.info at method entry

Docs:
- qwen-serve.md: clarify replaced:true vs replaced:false semantics

Author: doudouOUC

docs/users/qwen-serve.md

@@ -543,7 +543,7 @@ Response (200) — success:
 }
 ```
 
-- `replaced: true` — a runtime entry with the same name already existed and was replaced (old connection torn down, new one established).
+- `replaced: true` — a runtime entry with the same name already existed and the config fingerprint differs; old connection torn down, new one established. When the fingerprint matches (idempotent re-add), `replaced` is `false`.
 - `shadowedSettings: true` — a settings-defined server with the same name exists; the runtime entry now shadows it. The settings entry is untouched and re-emerges if the runtime entry is later removed.
 - `toolCount` — number of tools discovered on the newly connected server.
 

packages/acp-bridge/src/bridge.ts

@@ -3616,7 +3616,7 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge {
       }
       type AddOk = {
         name: string;
-        transport: string;
+        transport: 'stdio' | 'sse' | 'http' | 'tcp' | 'sdk';
         replaced: boolean;
         shadowedSettings: boolean;
         toolCount: number;
@@ -3627,37 +3627,17 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge {
         skipped: true;
         reason: 'budget_warning_only';
       };
-      let response: AddOk | AddSkip;
-      try {
-        response = (await Promise.race([
-          withTimeout(
-            info.connection.extMethod(
-              SERVE_CONTROL_EXT_METHODS.workspaceMcpRuntimeAdd,
-              { name, config, originatorClientId },
-            ),
-            MCP_RESTART_TIMEOUT_MS,
+      const response = (await Promise.race([
+        withTimeout(
+          info.connection.extMethod(
             SERVE_CONTROL_EXT_METHODS.workspaceMcpRuntimeAdd,
+            { name, config, originatorClientId },
           ),
-          getChannelClosedReject(info),
-        ])) as AddOk | AddSkip;
-      } catch (err) {
-        // Re-instantiate structured ACP error payloads into typed bridge
-        // errors so `sendBridgeError` maps them to stable HTTP codes.
-        const data = (err as { data?: unknown })?.data;
-        if (data && typeof data === 'object') {
-          const kind = (data as { errorKind?: unknown }).errorKind;
-          if (kind === 'mcp_budget_would_exceed') {
-            throw err;
-          }
-          if (kind === 'mcp_server_spawn_failed') {
-            throw err;
-          }
-          if (kind === 'invalid_config') {
-            throw err;
-          }
-        }
-        throw err;
-      }
+          MCP_RESTART_TIMEOUT_MS,
+          SERVE_CONTROL_EXT_METHODS.workspaceMcpRuntimeAdd,
+        ),
+        getChannelClosedReject(info),
+      ])) as AddOk | AddSkip;
       // Emit event on success (non-skip)
       const addSkipped = (response as { skipped?: boolean }).skipped === true;
       if (!addSkipped) {
@@ -3696,17 +3676,29 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge {
         originatorClientId: string;
       };
       type RemoveSkip = { name: string; skipped: true; reason: 'not_present' };
-      const response = (await Promise.race([
-        withTimeout(
-          info.connection.extMethod(
+      let response: RemoveOk | RemoveSkip;
+      try {
+        response = (await Promise.race([
+          withTimeout(
+            info.connection.extMethod(
+              SERVE_CONTROL_EXT_METHODS.workspaceMcpRuntimeRemove,
+              { name, originatorClientId },
+            ),
+            MCP_RESTART_TIMEOUT_MS,
             SERVE_CONTROL_EXT_METHODS.workspaceMcpRuntimeRemove,
-            { name, originatorClientId },
           ),
-          MCP_RESTART_TIMEOUT_MS,
-          SERVE_CONTROL_EXT_METHODS.workspaceMcpRuntimeRemove,
-        ),
-        getChannelClosedReject(info),
-      ])) as RemoveOk | RemoveSkip;
+          getChannelClosedReject(info),
+        ])) as RemoveOk | RemoveSkip;
+      } catch (err) {
+        const data = (err as { data?: unknown })?.data;
+        if (data && typeof data === 'object') {
+          const kind = (data as { errorKind?: unknown }).errorKind;
+          if (kind === 'acp_channel_unavailable') {
+            throw err;
+          }
+        }
+        throw err;
+      }
       // Emit event on success (non-skip)
       const removeSkipped =
         (response as { skipped?: boolean }).skipped === true;

packages/cli/src/acp-integration/acpAgent.ts

@@ -2504,7 +2504,7 @@ class QwenAgent implements Agent {
             'Invalid or missing name',
           );
         }
-        if (!config || typeof config !== 'object') {
+        if (!config || typeof config !== 'object' || Array.isArray(config)) {
           throw RequestError.invalidParams(
             undefined,
             'Invalid or missing config',
@@ -2528,11 +2528,17 @@ class QwenAgent implements Agent {
         }
         try {
           // Strip security-sensitive fields — runtime-added servers must
-          // not bypass permission gates via trust:true from HTTP body
-          const { trust: _stripped, ...safeConfig } = config as Record<
-            string,
-            unknown
-          >;
+          // not bypass permission gates via trust:true, leak cloud creds
+          // via authProviderType, manipulate tool filtering, or spawn in
+          // arbitrary directories
+          const {
+            trust: _trust,
+            authProviderType: _auth,
+            includeTools: _inc,
+            excludeTools: _exc,
+            cwd: _cwd,
+            ...safeConfig
+          } = config as Record<string, unknown>;
           const result = await manager.addRuntimeMcpServer(
             name,
             safeConfig as MCPServerConfig,

packages/cli/src/serve/server.ts

@@ -3620,10 +3620,11 @@ function sendBridgeErrorImpl(
     if (data && typeof data === 'object') {
       const kind = (data as { errorKind?: unknown }).errorKind;
       if (kind === 'mcp_budget_would_exceed') {
+        const d = data as { serverName?: string };
         res.status(409).json({
           error: errorMessage(err),
           code: 'mcp_budget_would_exceed',
-          ...(data as object),
+          serverName: d.serverName,
         });
         return;
       }
@@ -3646,18 +3647,19 @@ function sendBridgeErrorImpl(
         return;
       }
       if (kind === 'invalid_config') {
+        const d = data as { serverName?: string; reason?: string };
         res.status(400).json({
           error: errorMessage(err),
           code: 'invalid_config',
-          ...(data as object),
+          serverName: d.serverName,
+          reason: d.reason,
         });
         return;
       }
       if (kind === 'acp_channel_unavailable') {
         res.status(503).json({
           error: errorMessage(err),
           code: 'acp_channel_unavailable',
-          ...(data as object),
         });
         return;
       }

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

@@ -3373,11 +3373,11 @@ describe('McpClientManager — addRuntimeMcpServer / removeRuntimeMcpServer (T2.
       'client-4',
     );
 
-    // pool.acquire should NOT have been re-called (idempotent replace)
+    // pool.acquire should NOT have been re-called (idempotent no-op)
     expect(acquireSpy).not.toHaveBeenCalled();
     expect(result).toMatchObject({
       name: 'dup-srv',
-      replaced: true,
+      replaced: false,
       toolCount: 3,
     });
   });

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

@@ -2649,6 +2649,18 @@ export class McpClientManager {
     config: MCPServerConfig,
     originatorClientId: string,
   ): Promise<AddRuntimeMcpServerResult> {
+    // Reject explicitly excluded servers
+    if (this.cliConfig.isMcpServerDisabled(name)) {
+      throw new InvalidMcpConfigError(
+        name,
+        `server '${name}' is in excludedMcpServers and cannot be added at runtime`,
+      );
+    }
+
+    debugLogger.info(
+      `addRuntimeMcpServer: ${name} (transport=${mcpTransportOf(config)}, client=${originatorClientId})`,
+    );
+
     // Validate config minimally: must have at least one transport field
     const transport = mcpTransportOf(config);
     if (transport === 'unknown') {
@@ -2668,14 +2680,13 @@ export class McpClientManager {
     const newConnId = connectionIdOf(name, config);
     const existingConn = this.pooledConnections.get(name);
     if (existingConn && existingConn.id === newConnId) {
-      // Same fingerprint — just update the Config overlay (in case
-      // non-transport fields like trust/includeTools changed).
+      // Same fingerprint — no transport churn, just update Config overlay
       this.cliConfig.addRuntimeMcpServer(name, config);
       const toolCount = existingConn.toolsSnapshot.length;
       return {
         name,
         transport,
-        replaced: true,
+        replaced: false,
         shadowedSettings,
         toolCount,
         originatorClientId,
@@ -2812,14 +2823,28 @@ export class McpClientManager {
         this.releaseSlotName(name);
       }
       // Clean up any partial state (including tools from partial discover)
+      this.toolRegistry.removeMcpToolsByServer(name);
       this.pooledConnections.delete(name);
+      const failedClient = this.clients.get(name);
+      if (failedClient) {
+        try {
+          await failedClient.disconnect();
+        } catch {
+          /* best effort */
+        }
+      }
       this.clients.delete(name);
-      this.toolRegistry.removeMcpToolsByServer(name);
       this.stopHealthCheck(name);
+      this.eventEmitter?.emit('mcp-client-update', this.clients);
 
       const message = err instanceof Error ? err.message : String(err);
       const isTimeout = message.includes('timed out');
+      const exitCode =
+        err instanceof Error && 'exitCode' in err
+          ? (err as { exitCode?: number }).exitCode
+          : undefined;
       throw new McpServerSpawnFailedError(name, {
+        exitCode,
         stderr: message,
         timeout: isTimeout,
       });
@@ -2860,15 +2885,17 @@ export class McpClientManager {
     const settingsServers = this.cliConfig.getSettingsMcpServers() ?? {};
     const wasShadowingSettings = name in settingsServers;
 
-    // Release pool connection
+    // Release pool connection (identity-check prevents race with concurrent add)
     const poolConn = this.pooledConnections.get(name);
     if (poolConn) {
       try {
         poolConn.release();
       } catch {
         /* best effort */
       }
-      this.pooledConnections.delete(name);
+      if (this.pooledConnections.get(name) === poolConn) {
+        this.pooledConnections.delete(name);
+      }
     }
 
     // Disconnect standalone client
@@ -2883,10 +2910,13 @@ export class McpClientManager {
       this.eventEmitter?.emit('mcp-client-update', this.clients);
     }
 
-    // Cleanup: tool registry, status, health check (mirrors removeServer)
+    // Cleanup: tool registry, status, health check, diagnostics (mirrors removeServer)
     this.toolRegistry.removeMcpToolsByServer(name);
     removeMCPServerStatus(name);
     this.stopHealthCheck(name);
+    this.consecutiveFailures.delete(name);
+    this.isReconnecting.delete(name);
+    this.dropRefusalEntry(name);
 
     // Release budget slot
     const budget = this.pool?.getBudget();

packages/sdk-typescript/src/daemon/DaemonClient.ts

@@ -1204,7 +1204,7 @@ export class DaemonClient {
    */
   async addRuntimeMcpServer(
     request: DaemonRuntimeMcpAddRequest,
-    opts?: { clientId?: string },
+    opts?: { clientId?: string; timeoutMs?: number },
   ): Promise<DaemonRuntimeMcpAddResult> {
     return await this.fetchWithTimeout(
       `${this.baseUrl}/workspace/mcp/servers`,
@@ -1222,6 +1222,7 @@ export class DaemonClient {
         }
         return (await res.json()) as DaemonRuntimeMcpAddResult;
       },
+      opts?.timeoutMs ?? MCP_RESTART_DEFAULT_TIMEOUT_MS,
     );
   }
 

packages/sdk-typescript/src/daemon/events.ts

@@ -630,6 +630,7 @@ export interface DaemonFollowupSuggestionData {
   [key: string]: unknown;
 }
 
+/**
  * T2.8 (#4514). Fired when `POST /workspace/mcp/servers` succeeds,
  * including both fresh additions and replace-on-existing-name. The
  * event fans out to every active session SSE bus.
@@ -1319,6 +1320,7 @@ export function asKnownDaemonEvent(
     case 'followup_suggestion':
       return isFollowupSuggestionData(event.data)
         ? (event as DaemonFollowupSuggestionEvent)
+        : undefined;
     case 'mcp_server_added':
       return isMcpServerAddedData(event.data)
         ? (event as DaemonMcpServerAddedEvent)
@@ -2382,6 +2384,9 @@ function isFollowupSuggestionData(
     isNonEmptyString(value['sessionId']) &&
     isNonEmptyString(value['suggestion']) &&
     isNonEmptyString(value['promptId'])
+  );
+}
+
 function isMcpServerAddedData(
   value: unknown,
 ): value is DaemonMcpServerAddedData {