feat: add MCP client plugin (dsh-mcp-client)

Connects to an external MCP server and registers its tools on
ctx.tools. Supports stdio (child process) and Streamable HTTP
transports. Credential-shaped env vars are scrubbed before forwarding
to child processes.

- Plugin lifecycle: connect, sync tools, re-sync on ToolListChanged,
  dispose unregisters and closes
- Full JSDoc on all exports (@param/@returns on functions)
- 100% per-file coverage (apply lifecycle, args coercion, env scrubbing)
- Config catalog regenerated

packages/mcp/mcp-client/src/index.ts

@@ -73,29 +73,19 @@ export const Config = z.union([
     env: z.dict(String).default({}),
     cwd: z.string().default(''),
     toolPrefix: z.string().default(''),
-    toolCallTimeoutMs: z.natural().min(1).default(DEFAULT_TOOL_CALL_TIMEOUT_MS),
+    toolCallTimeoutMs: z.number().default(DEFAULT_TOOL_CALL_TIMEOUT_MS),
   }),
   z.object({
     transport: z.const('streamable-http'),
     url: z.string().required(),
     headers: z.dict(String).default({}),
     toolPrefix: z.string().default(''),
-    toolCallTimeoutMs: z.natural().min(1).default(DEFAULT_TOOL_CALL_TIMEOUT_MS),
+    toolCallTimeoutMs: z.number().default(DEFAULT_TOOL_CALL_TIMEOUT_MS),
   }),
 ]) as unknown as z<Config>
 
 // ---- Plugin apply ----
 
-/** Mutable state shared between the async connect path, notification handler, and disposers. */
-interface PluginState {
-  /** Current generation of tool disposers (keyed by registered name). */
-  disposers: Map<string, () => void>
-  /** Whether a syncTools call is currently in-flight. */
-  syncing: boolean
-  /** Whether another tools/list_changed arrived while syncing (coalesce flag). */
-  pendingResync: boolean
-}
-
 export function apply(ctx: Context, config: Config): void {
   const transport = createTransport(config)
   const client = new Client(
@@ -103,64 +93,36 @@ export function apply(ctx: Context, config: Config): void {
     { capabilities: {} },
   )
 
-  const state: PluginState = { disposers: new Map(), syncing: false, pendingResync: false }
-
-  const opts = { toolPrefix: config.toolPrefix, toolCallTimeoutMs: config.toolCallTimeoutMs }
-
-  /** Dispose all currently registered tools. */
-  function disposeTools(): void {
-    for (const dispose of state.disposers.values()) dispose()
-    state.disposers = new Map()
-  }
-
-  /** Run syncTools with latest-wins coalescing. */
-  async function resync(): Promise<void> {
-    if (state.syncing) {
-      state.pendingResync = true
-      return
-    }
-    state.syncing = true
-    try {
-      state.disposers = await syncTools(client, ctx, opts, state.disposers)
-    } finally {
-      state.syncing = false
-    }
-    // If another notification arrived while we were syncing, run once more.
-    if (state.pendingResync) {
-      state.pendingResync = false
-      await resync()
-    }
-  }
-
-  // When the connection closes (server crash or intentional close), unregister
-  // all tools so the model no longer sees them in the system prompt.
-  client.onclose = () => {
-    disposeTools()
-    ctx.logger.info('mcp-client: connection closed, tools unregistered')
-  }
-
   // Connect and set up tools. Errors during connect are logged, not thrown
-  // (the plugin simply has no tools registered). The IIFE is fire-and-forget;
-  // disposal closes the client directly without waiting for startup.
-  void (async () => {
+  // (the plugin simply has no tools registered).
+  const ready = (async () => {
     await client.connect(transport)
-    await resync()
+
+    let disposers = await syncTools(client, ctx, {
+      toolPrefix: config.toolPrefix,
+      toolCallTimeoutMs: config.toolCallTimeoutMs,
+    }, new Map())
 
     client.setNotificationHandler(
       ToolListChangedNotificationSchema,
       async () => {
         ctx.logger.info('mcp-client: tool list changed, re-syncing')
-        await resync()
+        disposers = await syncTools(client, ctx, {
+          toolPrefix: config.toolPrefix,
+          toolCallTimeoutMs: config.toolCallTimeoutMs,
+        }, disposers)
       },
     )
+
+    return disposers
   })().catch((error: unknown) => {
     ctx.logger.error(`mcp-client: failed to connect: ${String(error)}`)
+    return new Map<string, () => void>()
   })
 
-  // Fiber disposal: close the client immediately (triggers onclose → tools
-  // unregistered). No `await ready` — if connect is still pending, close aborts
-  // it promptly rather than blocking until the SDK request times out.
   ctx.effect(() => async () => {
-    try { await client.close() } catch { /* transport already gone or never connected */ }
+    const disposers = await ready
+    for (const dispose of disposers.values()) dispose()
+    try { await client.close() } catch { /* transport already gone */ }
   }, 'mcp-client.connection')
 }

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

@@ -18,24 +18,19 @@ export interface ToolBridgeOptions {
 /** State for one sync generation: the current set of disposers keyed by tool name. */
 type ToolDisposers = Map<string, () => void>
 
-/** A tool fetched from the MCP server, pending registration. */
-interface FetchedTool {
-  registeredName: string
-  definition: ToolDefinition
-}
-
 /**
  * Sync the MCP server's tool list into the harness ToolRegistry.
  *
- * Two-phase approach: fetch all pages first (no side effects), then dispose old
- * tools and register new ones. If fetching fails, the previous generation stays
- * intact — no tools are lost on a transient listTools failure.
+ * - Calls `client.listTools()` (paginated: drains all pages).
+ * - Registers each tool as a raw `ToolDefinition`.
+ * - On name conflict: logs a warning and skips that tool.
+ * - Returns a disposer map; call each value to unregister.
  *
  * @param client - Connected MCP Client instance used to list and call tools.
  * @param ctx - Cordis context providing the `tools` service for registration.
  * @param opts - Bridge options: tool name prefix and per-call timeout.
- * @param previous - Disposer map from a prior sync generation; disposed only
- *   after all pages are successfully fetched.
+ * @param previous - Disposer map from a prior sync generation; all entries are
+ *   disposed before re-registering.
  * @returns A map of registered tool names to their unregister disposers.
  */
 export async function syncTools(
@@ -44,40 +39,32 @@ export async function syncTools(
   opts: ToolBridgeOptions,
   previous: ToolDisposers,
 ): Promise<ToolDisposers> {
-  // Phase 1: fetch all tools (no mutations).
-  const fetched: FetchedTool[] = []
+  for (const dispose of previous.values()) dispose()
+
+  const disposers: ToolDisposers = new Map()
+
   let cursor: string | undefined
   do {
     const response = await client.listTools(cursor ? { cursor } : undefined)
     for (const tool of response.tools) {
       const registeredName = opts.toolPrefix + tool.name
-      fetched.push({
-        registeredName,
-        definition: {
-          name: registeredName,
-          description: tool.description ?? '',
-          parameters: tool.inputSchema,
-          execute: createExecutor(client, tool.name, opts),
-        },
-      })
+      const definition: ToolDefinition = {
+        name: registeredName,
+        description: tool.description ?? '',
+        parameters: tool.inputSchema,
+        execute: createExecutor(client, tool.name, opts),
+      }
+      try {
+        const dispose = ctx.tools.register(definition)
+        disposers.set(registeredName, dispose)
+      } catch {
+        // Name conflict — another tool with this name is already registered.
+        ctx.logger.warn(`mcp-client: skipping tool "${registeredName}" (name conflict)`)
+      }
     }
     cursor = response.nextCursor
   } while (cursor)
 
-  // Phase 2: dispose previous generation, then register new tools.
-  // If we reach here, all pages were fetched successfully.
-  for (const dispose of previous.values()) dispose()
-
-  const disposers: ToolDisposers = new Map()
-  for (const { registeredName, definition } of fetched) {
-    try {
-      const dispose = ctx.tools.register(definition)
-      disposers.set(registeredName, dispose)
-    } catch {
-      ctx.logger.warn(`mcp-client: skipping tool "${registeredName}" (name conflict)`)
-    }
-  }
-
   return disposers
 }
 
@@ -135,21 +122,14 @@ function createExecutor(
     // with optional fallbacks).
     // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
     const content: McpContentBlock[] = result.content
-    let text = extractText(content, mcpToolName)
-
-    // MCP tools with outputSchema may return structuredContent with an empty
-    // content array. Surface the structured payload as JSON so the model sees
-    // the actual result.
-    if (!text && 'structuredContent' in result && result.structuredContent != null) {
-      text = JSON.stringify(result.structuredContent)
-    }
+    const text = extractText(content, mcpToolName)
 
     // MCP isError → throw so ToolRegistry produces an isError result for the model.
     if ('isError' in result && result.isError === true) {
-      throw new Error(text || 'MCP tool error')
+      throw new Error(text)
     }
 
-    return [{ type: 'text', text: text || `(${mcpToolName} returned no content)` }]
+    return [{ type: 'text', text }]
   }
 }
 
@@ -160,11 +140,8 @@ function createExecutor(
  *
  * Defensive: fields that the MCP spec declares required (mimeType, text) are
  * guarded with fallbacks because this is a network trust boundary.
- *
- * Returns empty string when no text parts were extracted (caller decides
- * fallback — e.g. structuredContent).
  */
-function extractText(mcpContent: McpContentBlock[], _toolName: string): string {
+function extractText(mcpContent: McpContentBlock[], toolName: string): string {
   const parts: string[] = []
 
   for (const block of mcpContent) {
@@ -187,5 +164,5 @@ function extractText(mcpContent: McpContentBlock[], _toolName: string): string {
     }
   }
 
-  return parts.join('\n')
+  return parts.join('\n') || `(${toolName} returned no text content)`
 }