fix(appkit): mcp client hardening + reload() race

Address four agentic-review findings on PR #306. All four were P1 risks
under the MCP path or the markdown agent reload flow.

1. mcp callTool joined undefined text into 'undefined' literal

   McpToolCallResult.content[].text is optional per the spec; the old
   filter only matched `type === "text"`, so an entry like
   { type: "text" } (text undefined) flowed through and
   `Array.join('\n')` emitted the literal string "undefined" — visible
   to the agent and to the user.

   Tighten the filter on both the success and error paths with a type
   predicate that narrows on `typeof c.text === "string"`. The error
   path falls back to a generic "MCP tool call failed" message when no
   text entries survive.

2. mcp sendNotification ignored HTTP error status

   `notifications/initialized` and other fire-and-forget notifications
   completed `connect()` even when the server returned 4xx/5xx — a
   silent connect followed by mysterious tool-call failures. MCP spec
   says notifications don't acknowledge, so we keep the fire-and-forget
   contract but log a warning when the server clearly rejected the
   request. Wrap the fetch in try/catch so network failures surface in
   the logs without breaking the protocol.

3. mcp response body had no size cap

   `response.text()` and `response.json()` both buffer the entire body
   into memory. A misconfigured or malicious MCP server could stream
   unbounded data to exhaust client memory.

   New `readResponseTextCapped()` helper streams the body via the
   ReadableStream API and aborts once cumulative bytes cross
   MCP_RESPONSE_BODY_LIMIT_BYTES (1 MiB — far above any normal
   `initialize` / `tools/list` / `tools/call` JSON-RPC response). Used
   on both the SSE and plain-JSON branches in sendRpc.

4. agents.reload() closed the live mcpClient mid-stream

   Tool dispatch reads `this.mcpClient` at call time, not via capture.
   The old `reload()` called `await this.mcpClient.close()` and
   nulled the field; any in-flight stream that resolved the field
   afterwards crashed with "MCP client is closed" mid-conversation.

   Drop the synchronous close. The client owns only short-lived
   fetch handles, so a reload doesn't need to tear down state — new
   endpoints append to the existing connection map via
   `connectHostedTools`, stale connections from removed configs
   become unreachable through the new agent tool indexes (small
   memory cost, no correctness hazard). The shutdown path still
   closes — that's process teardown, where in-flight streams have
   already been aborted via `abortActiveOperations`.

Tests: 4 new MCP cases (undefined-text filter on success + error
paths, generic error fallback, 1 MiB body cap), 1 reload-doesn't-close
regression, plus a connect-still-succeeds-on-4xx-notification case.
2298 tests passing across the workspace.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

Author: MarioCadenas

packages/appkit/src/connectors/mcp/client.ts

@@ -34,6 +34,52 @@ import type { McpEndpointConfig } from "./types";
 
 const logger = createLogger("connector:mcp");
 
+/**
+ * Hard cap on the size of a single MCP response body, including SSE
+ * frames bundled into one HTTP response. MCP `initialize` / `tools/list`
+ * / `tools/call` responses are JSON-RPC payloads — single-digit kilobytes
+ * in normal use. A response anywhere near this size signals either a
+ * misbehaving server or an attempt to exhaust client memory; we'd rather
+ * fail loudly than allocate unbounded buffers from a remote.
+ */
+const MCP_RESPONSE_BODY_LIMIT_BYTES = 1024 * 1024;
+
+/**
+ * Read a fetch Response body into a string with a hard size cap. Aborts
+ * and throws if the cumulative bytes read cross {@link
+ * MCP_RESPONSE_BODY_LIMIT_BYTES}, so a remote server cannot keep
+ * streaming data past the limit. Returns the empty string when the
+ * response has no readable body.
+ */
+async function readResponseTextCapped(
+  response: Response,
+  maxBytes: number,
+  contextLabel: string,
+): Promise<string> {
+  if (!response.body) return "";
+  const reader = response.body.getReader();
+  const decoder = new TextDecoder("utf-8");
+  let total = 0;
+  let out = "";
+  try {
+    while (true) {
+      const { done, value } = await reader.read();
+      if (done) break;
+      total += value.byteLength;
+      if (total > maxBytes) {
+        throw new Error(
+          `MCP ${contextLabel}: response body exceeded ${maxBytes} bytes — refusing to allocate unbounded buffer from a remote server.`,
+        );
+      }
+      out += decoder.decode(value, { stream: true });
+    }
+    out += decoder.decode();
+  } finally {
+    reader.releaseLock();
+  }
+  return out;
+}
+
 interface JsonRpcRequest {
   jsonrpc: "2.0";
   id: number;
@@ -261,18 +307,22 @@ export class AppKitMcpClient {
     );
     const result = rpcResult.result as McpToolCallResult;
 
+    // `text` is optional on `McpToolCallResult.content[]` per the MCP
+    // spec; filtering only on `type === "text"` lets `c.text` be
+    // `undefined`, which `Array.join` would render as the literal
+    // string `"undefined"` and ship to the agent. Narrow on both
+    // fields so the joined string only contains real text.
+    const textContent = (result.content ?? []).filter(
+      (c): c is { type: "text"; text: string } =>
+        c.type === "text" && typeof c.text === "string",
+    );
+
     if (result.isError) {
-      const errText = (result.content ?? [])
-        .filter((c) => c.type === "text")
-        .map((c) => c.text)
-        .join("\n");
+      const errText = textContent.map((c) => c.text).join("\n");
       throw new Error(errText || "MCP tool call failed");
     }
 
-    return (result.content ?? [])
-      .filter((c) => c.type === "text")
-      .map((c) => c.text)
-      .join("\n");
+    return textContent.map((c) => c.text).join("\n");
   }
 
   async close(): Promise<void> {
@@ -334,11 +384,20 @@ export class AppKitMcpClient {
     }
 
     const contentType = response.headers.get("content-type") ?? "";
+    // Always read the body via the capped helper so a misconfigured or
+    // malicious server can't exhaust client memory by streaming an
+    // unbounded payload. Applies to both SSE (`response.text()` would
+    // have buffered the whole stream) and plain JSON (`response.json()`
+    // does the same internally).
+    const bodyText = await readResponseTextCapped(
+      response,
+      MCP_RESPONSE_BODY_LIMIT_BYTES,
+      method,
+    );
     let json: JsonRpcResponse;
 
     if (contentType.includes("text/event-stream")) {
-      const text = await response.text();
-      const lastData = text
+      const lastData = bodyText
         .split("\n")
         .filter((line) => line.startsWith("data: "))
         .map((line) => line.slice(6))
@@ -348,7 +407,10 @@ export class AppKitMcpClient {
       }
       json = JSON.parse(lastData) as JsonRpcResponse;
     } else {
-      json = (await response.json()) as JsonRpcResponse;
+      if (bodyText.length === 0) {
+        throw new Error(`MCP response for ${method} had an empty body`);
+      }
+      json = JSON.parse(bodyText) as JsonRpcResponse;
     }
 
     if (json.error) {
@@ -380,12 +442,36 @@ export class AppKitMcpClient {
     }
 
     const fetchImpl = this.options.fetchImpl ?? fetch;
-    await fetchImpl(url, {
-      method: "POST",
-      headers,
-      body: JSON.stringify({ jsonrpc: "2.0", method }),
-      signal: AbortSignal.timeout(30_000),
-    });
+    // MCP notifications are fire-and-forget per spec — we don't throw on
+    // failure. But silently swallowing 4xx/5xx hides server-side
+    // rejections that would otherwise look like a successful connect()
+    // followed by mysterious tool-call failures. Surface the bad status
+    // via the logger so the dev sees it without breaking the protocol
+    // contract.
+    try {
+      const response = await fetchImpl(url, {
+        method: "POST",
+        headers,
+        body: JSON.stringify({ jsonrpc: "2.0", method }),
+        signal: AbortSignal.timeout(30_000),
+      });
+      if (!response.ok) {
+        logger.warn(
+          "MCP notification %s to %s returned %d %s — the server may have rejected the request, but per MCP spec notifications are fire-and-forget and the connection is considered established.",
+          method,
+          url,
+          response.status,
+          response.statusText,
+        );
+      }
+    } catch (err) {
+      logger.warn(
+        "MCP notification %s to %s failed before a response was received: %O",
+        method,
+        url,
+        err,
+      );
+    }
   }
 
   /**

packages/appkit/src/connectors/mcp/tests/client.test.ts

@@ -400,3 +400,182 @@ describe("AppKitMcpClient — caller abort signal composition", () => {
     expect(error.name).toBe("AbortError");
   });
 });
+
+describe("AppKitMcpClient — callTool result hardening", () => {
+  let authSpy: ReturnType<typeof vi.fn>;
+
+  beforeEach(() => {
+    authSpy = vi.fn(workspaceAuth);
+  });
+
+  async function connectAndCall(
+    callResult: unknown,
+  ): Promise<{ result: string }> {
+    const connectResponders = [
+      () =>
+        jsonResponse(
+          { jsonrpc: "2.0", id: 1, result: {} },
+          { "mcp-session-id": "sess-1" },
+        ),
+      () => jsonResponse({ jsonrpc: "2.0", result: null }),
+      () =>
+        jsonResponse({
+          jsonrpc: "2.0",
+          id: 3,
+          result: { tools: [{ name: "tool" }] },
+        }),
+    ];
+    const callResponder = () =>
+      jsonResponse({ jsonrpc: "2.0", id: 4, result: callResult });
+
+    const { fetchImpl } = recordingFetch([...connectResponders, callResponder]);
+    const client = new AppKitMcpClient(WORKSPACE, authSpy, workspacePolicy, {
+      fetchImpl,
+      dnsLookup: publicDnsLookup,
+    });
+    await client.connect({
+      name: "srv",
+      url: `${WORKSPACE}/api/2.0/mcp/genie/abc`,
+    });
+    const result = await client.callTool("mcp.srv.tool", {}, undefined);
+    return { result };
+  }
+
+  test("filters content entries whose text is undefined (regression: 'undefined' literal in joined output)", async () => {
+    // McpToolCallResult.content[i].text is optional. Previously the
+    // filter only checked `type === "text"`, so an entry like
+    // { type: "text" } (text undefined) flowed through, and
+    // `Array.join('\n')` emitted the literal string "undefined".
+    const { result } = await connectAndCall({
+      content: [
+        { type: "text", text: "first line" },
+        { type: "text" },
+        { type: "text", text: "second line" },
+        { type: "image", data: "..." },
+      ],
+    });
+    expect(result).toBe("first line\nsecond line");
+    expect(result).not.toContain("undefined");
+  });
+
+  test("filters undefined text on the error path too", async () => {
+    await expect(
+      connectAndCall({
+        isError: true,
+        content: [{ type: "text" }, { type: "text", text: "boom" }],
+      }),
+    ).rejects.toThrow(/^boom$/);
+  });
+
+  test("error with no text content falls back to a generic message", async () => {
+    await expect(
+      connectAndCall({
+        isError: true,
+        content: [{ type: "text" }, { type: "image", data: "..." }],
+      }),
+    ).rejects.toThrow(/MCP tool call failed/);
+  });
+});
+
+describe("AppKitMcpClient — response body size cap", () => {
+  let authSpy: ReturnType<typeof vi.fn>;
+
+  beforeEach(() => {
+    authSpy = vi.fn(workspaceAuth);
+  });
+
+  test("rejects an unbounded response body (1 MB cap)", async () => {
+    // Mimic a server streaming forever: each `read()` returns another
+    // 64 KB chunk. The capped reader must abort once it crosses the
+    // 1 MB limit rather than buffer indefinitely.
+    const oversizedBody = new ReadableStream<Uint8Array>({
+      start(controller) {
+        const chunk = new Uint8Array(64 * 1024).fill(0x41); // 'A'
+        let pushed = 0;
+        const maxChunks = 32; // 32 * 64KiB = 2 MiB, well above the 1 MiB cap
+        const id = setInterval(() => {
+          controller.enqueue(chunk);
+          pushed++;
+          if (pushed >= maxChunks) {
+            clearInterval(id);
+            controller.close();
+          }
+        }, 0);
+      },
+    });
+    const oversizedResponse = new Response(oversizedBody, {
+      status: 200,
+      headers: { "content-type": "application/json" },
+    });
+
+    const connectResponders = [
+      () =>
+        jsonResponse(
+          { jsonrpc: "2.0", id: 1, result: {} },
+          { "mcp-session-id": "sess-1" },
+        ),
+      () => jsonResponse({ jsonrpc: "2.0", result: null }),
+      () => oversizedResponse,
+    ];
+    const { fetchImpl } = recordingFetch(connectResponders);
+    const client = new AppKitMcpClient(WORKSPACE, authSpy, workspacePolicy, {
+      fetchImpl,
+      dnsLookup: publicDnsLookup,
+    });
+
+    await expect(
+      client.connect({
+        name: "evil",
+        url: `${WORKSPACE}/api/2.0/mcp/genie/abc`,
+      }),
+    ).rejects.toThrow(/exceeded 1048576 bytes/);
+  });
+});
+
+describe("AppKitMcpClient — sendNotification HTTP error surfacing", () => {
+  let authSpy: ReturnType<typeof vi.fn>;
+
+  beforeEach(() => {
+    authSpy = vi.fn(workspaceAuth);
+  });
+
+  test("connect succeeds even when notifications/initialized returns 4xx (fire-and-forget per spec)", async () => {
+    // The MCP spec says notifications are fire-and-forget. We must not
+    // throw, and connect() must return normally; the regression we're
+    // guarding is that the failure shouldn't silently appear as a clean
+    // connect from the dev's perspective (the warning log surfaces it).
+    const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
+    try {
+      const { fetchImpl } = recordingFetch([
+        () =>
+          jsonResponse(
+            { jsonrpc: "2.0", id: 1, result: {} },
+            { "mcp-session-id": "sess-1" },
+          ),
+        () =>
+          new Response("bad request", {
+            status: 400,
+            statusText: "Bad Request",
+          }),
+        () =>
+          jsonResponse({
+            jsonrpc: "2.0",
+            id: 3,
+            result: { tools: [{ name: "tool" }] },
+          }),
+      ]);
+      const client = new AppKitMcpClient(WORKSPACE, authSpy, workspacePolicy, {
+        fetchImpl,
+        dnsLookup: publicDnsLookup,
+      });
+      await expect(
+        client.connect({
+          name: "srv",
+          url: `${WORKSPACE}/api/2.0/mcp/genie/abc`,
+        }),
+      ).resolves.not.toThrow();
+    } finally {
+      warnSpy.mockRestore();
+    }
+  });
+});

packages/appkit/src/plugins/agents/agents.ts

@@ -259,10 +259,21 @@ export class AgentsPlugin extends Plugin implements ToolProvider {
    */
   async reload(): Promise<void> {
     const next = await this.buildAgentRegistry();
-    if (this.mcpClient) {
-      await this.mcpClient.close();
-      this.mcpClient = null;
-    }
+    // Deliberately NOT closing the existing mcpClient here. Tool
+    // dispatch in `dispatchToolCall` reads `this.mcpClient` at call
+    // time; closing it mid-stream throws "MCP client is closed" from
+    // the next sendRpc and kills the in-flight conversation. The
+    // client owns only short-lived `fetch` handles (no keep-alive
+    // sockets) and the connections map persists in the live instance,
+    // so dropping `this.mcpClient` would also strand in-flight tool
+    // calls that resolved the field a moment earlier. Leave the live
+    // client in place; `buildAgentRegistry` -> `connectHostedTools`
+    // adds any new endpoints to the same instance, and stale
+    // connections from a removed config become unreachable through
+    // the new agent tool indexes (small memory cost, no correctness
+    // hazard). The shutdown path still closes — that's process
+    // teardown, where in-flight streams have already been aborted via
+    // `abortActiveOperations`.
     this.agents = next.agents;
     this.defaultAgentName = next.defaultAgentName;
   }