chore: three small cleanups flagged in the v2.2.0 merge review

clients/web/.npmignore

@@ -8,11 +8,15 @@
 # load. (The other clients don't hit this because none of them ship a nested
 # .gitignore.)
 #
-# Crucially this file does NOT list `build` or `dist`, so both the prod runner
-# (build/) and the SPA (dist/) are packed. The root "files" allowlist already
-# restricts publishing to those two directories, so everything else in
-# clients/web (src, configs, node_modules, coverage, storybook-static) stays out
-# regardless — the entries below are just belt-and-suspenders.
+# Crucially this file does NOT list `build`, `dist`, or `static`, so all three
+# are packed: the prod runner (build/), the SPA (dist/), and the MCP Apps
+# sandbox proxy page (static/sandbox_proxy.html — a committed source file, read
+# from disk at runtime by server/sandbox-controller.ts as
+# `<runner dir>/../static/sandbox_proxy.html`, so it must ship at exactly that
+# path; #1859). The root "files" allowlist names those three directories and
+# nothing else under clients/web, so the rest (src, configs, node_modules,
+# coverage, storybook-static) stays out regardless — the entries below are just
+# belt-and-suspenders.
 node_modules
 coverage
 storybook-static

clients/web/src/test/core/react/useServers.test.tsx

@@ -875,7 +875,9 @@ describe("useServers", () => {
         fetchFn: async (input, init) => {
           const url = input instanceof Request ? input.url : String(input);
           if (url.endsWith("/api/servers/events")) {
-            return { ok: true, body: null } as unknown as Response;
+            // A real Response constructed from `null` has a null `.body`,
+            // so the guard is exercised through the actual Response API.
+            return new Response(null, { status: 200 });
           }
           return h.fetchFn(url, init);
         },
@@ -894,12 +896,14 @@ describe("useServers", () => {
         fetchFn: async (input, init) => {
           const url = input instanceof Request ? input.url : String(input);
           if (url.endsWith("/api/servers/events")) {
-            const body = {
-              getReader: () => ({
-                read: () => Promise.reject(new Error("stream broke")),
-              }),
-            };
-            return { ok: true, body } as unknown as Response;
+            // A real stream whose first pull throws — `reader.read()` then
+            // rejects exactly as a broken network body would, with no cast.
+            const body = new ReadableStream<Uint8Array>({
+              pull() {
+                throw new Error("stream broke");
+              },
+            });
+            return new Response(body, { status: 200 });
           }
           return h.fetchFn(url, init);
         },
@@ -928,22 +932,18 @@ describe("useServers", () => {
         fetchFn: async (input, init) => {
           const url = input instanceof Request ? input.url : String(input);
           if (url.endsWith("/api/servers/events")) {
-            const body = {
-              getReader: () => ({
-                read: async () => {
-                  reads += 1;
-                  if (reads === 1) {
-                    // Two frames in one chunk → one background refresh.
-                    return {
-                      done: false,
-                      value: encoder.encode("event: change\n\n\n\n"),
-                    };
-                  }
-                  return { done: true, value: undefined };
-                },
-              }),
-            };
-            return { ok: true, body } as unknown as Response;
+            const body = new ReadableStream<Uint8Array>({
+              pull(controller) {
+                reads += 1;
+                if (reads === 1) {
+                  // Two frames in one chunk → one background refresh.
+                  controller.enqueue(encoder.encode("event: change\n\n\n\n"));
+                  return;
+                }
+                controller.close();
+              },
+            });
+            return new Response(body, { status: 200 });
           }
           return h.fetchFn(url, init);
         },
@@ -1138,22 +1138,24 @@ describe("useServers", () => {
     const fetchFn: typeof fetch = async (input, init) => {
       const url = input instanceof Request ? input.url : String(input);
       if (url.endsWith("/api/servers/events")) {
-        const body = {
-          getReader: () => ({
-            read: async () => {
-              reads += 1;
-              // Priming comment only — no `event:` / `data:` line.
-              if (reads === 1) {
-                return { done: false, value: encoder.encode(":\n\n") };
-              }
-              // Hold the stream open so the loop can't end and let a
-              // teardown-time settle hide a queued refresh.
-              await secondRead;
-              return { done: true, value: undefined };
-            },
-          }),
-        };
-        return { ok: true, body } as unknown as Response;
+        // `pull` runs once per read, so the counting and the blocking
+        // second read work the same way they would on a hand-rolled reader
+        // double — while staying type-checked against the Response API.
+        const body = new ReadableStream<Uint8Array>({
+          async pull(controller) {
+            reads += 1;
+            // Priming comment only — no `event:` / `data:` line.
+            if (reads === 1) {
+              controller.enqueue(encoder.encode(":\n\n"));
+              return;
+            }
+            // Hold the stream open so the loop can't end and let a
+            // teardown-time settle hide a queued refresh.
+            await secondRead;
+            controller.close();
+          },
+        });
+        return new Response(body, { status: 200 });
       }
       if (url.endsWith("/api/servers")) listGets += 1;
       return h.fetchFn(url, init);

scripts/lib/announced-child.mjs

@@ -0,0 +1,85 @@
+/**
+ * Spawn a child process and wait for it to announce readiness on its output.
+ *
+ * Extracted from `scripts/smoke-web-app.mjs` so the failure path can be tested
+ * (#2000). A smoke script only ever exercises its own happy path: the real MCP
+ * test server always announces, so nothing in `npm run ci` proved that a child
+ * which stays alive *through* the readiness timeout is still reachable by the
+ * caller's teardown. That is precisely the case that used to orphan a live
+ * server holding its port, and it is invisible to the smoke itself.
+ *
+ * The ownership rule this encodes: the child is handed to `onSpawn` the moment
+ * it exists, **before** the readiness wait, so every throw path below leaves
+ * the caller holding a stoppable handle. Returning it only on success is what
+ * made the timeout leak — `spawnError` and `exited` were never affected (the
+ * child is already gone there), but they cost nothing to cover the same way.
+ *
+ * Both stdio channels are piped and scanned: a child that announces with
+ * `console.error` is missed entirely by a stdout-only scan, which then times
+ * out with an empty diagnostic. Piping both also keeps the child's noise out of
+ * the caller's output while still making it available in the failure message.
+ */
+
+import { spawn } from "node:child_process";
+import { setTimeout as delay } from "node:timers/promises";
+
+/**
+ * @param {object} opts
+ * @param {string} opts.command       Executable to spawn.
+ * @param {string[]} opts.args        Arguments for it.
+ * @param {string} [opts.cwd]         Working directory.
+ * @param {RegExp} opts.pattern       Matched against the accumulated output;
+ *                                    the first match ends the wait.
+ * @param {(child: import("node:child_process").ChildProcess) => void} opts.onSpawn
+ *   Called synchronously with the child immediately after spawn, before any
+ *   waiting. This is the caller's teardown handle.
+ * @param {string} opts.what          Noun used in error messages ("MCP test server").
+ * @param {number} [opts.timeoutMs]   Readiness budget (default 30s).
+ * @param {number} [opts.pollMs]      Poll interval (default 250ms).
+ * @returns {Promise<{ child: import("node:child_process").ChildProcess, match: RegExpMatchArray }>}
+ */
+export async function startAnnouncedChild({
+  command,
+  args,
+  cwd,
+  pattern,
+  onSpawn,
+  what,
+  timeoutMs = 30_000,
+  pollMs = 250,
+}) {
+  const child = spawn(command, args, {
+    cwd,
+    stdio: ["ignore", "pipe", "pipe"],
+  });
+  // Publish before waiting — this is the whole point of the helper.
+  onSpawn(child);
+
+  let out = "";
+  child.stdout.on("data", (d) => (out += d));
+  child.stderr.on("data", (d) => (out += d));
+  let exited = false;
+  let spawnError = null;
+  // A spawn failure (e.g. an unbuilt/renamed entry) emits `error`, NOT `exit` —
+  // and with no `error` listener Node throws it uncaught, replacing the caller's
+  // diagnostic with a raw stack. `close` is listened to alongside `exit` for the
+  // same reason: it fires in cases `exit` does not, so a child that dies without
+  // an exit event can't leave the poll below spinning for the full budget.
+  child.on("error", (err) => (spawnError = err));
+  child.on("exit", () => (exited = true));
+  child.on("close", () => (exited = true));
+
+  const attempts = Math.max(1, Math.ceil(timeoutMs / pollMs));
+  for (let attempt = 0; attempt < attempts; attempt++) {
+    const match = out.match(pattern);
+    if (match) return { child, match };
+    if (spawnError) {
+      throw new Error(`could not spawn the ${what}: ${spawnError.message}`);
+    }
+    if (exited) throw new Error(`${what} exited early:\n${out}`);
+    await delay(pollMs);
+  }
+  throw new Error(
+    `${what} did not start within ${Math.round(timeoutMs / 1000)}s:\n${out}`,
+  );
+}

scripts/lib/announced-child.test.mjs

@@ -0,0 +1,128 @@
+/**
+ * Tests for the spawn/readiness ownership helper (#2000).
+ *
+ * The invariant under test is the one a smoke script can never check itself:
+ * a child that stays alive *through* the readiness timeout must still be
+ * reachable by the caller's teardown. `smoke:web:app`'s happy path always gets
+ * the announcement, so a regression here would be silent in `npm run ci` and
+ * would surface only as a stray process holding a port on a later run.
+ *
+ * Every case drives a real `node -e` child rather than a spy, so the assertion
+ * is about a real process being killable, not about a mock's call log.
+ */
+
+import test from "node:test";
+import assert from "node:assert/strict";
+import { startAnnouncedChild } from "./announced-child.mjs";
+
+/** Wait for a real child to exit (or resolve immediately if it already has). */
+function waitForExit(child) {
+  if (child.exitCode !== null || child.signalCode !== null)
+    return Promise.resolve();
+  return new Promise((resolve) => child.once("close", resolve));
+}
+
+test("hands the child to onSpawn before waiting, and returns the match", async () => {
+  let seen = null;
+  const { child, match } = await startAnnouncedChild({
+    command: process.execPath,
+    args: ["-e", 'console.error("listening at http://127.0.0.1:9999")'],
+    pattern: /listening at (http:\/\/\S+)/i,
+    onSpawn: (c) => (seen = c),
+    what: "probe",
+    timeoutMs: 10_000,
+    pollMs: 25,
+  });
+  assert.equal(
+    seen,
+    child,
+    "onSpawn received the same child that was returned",
+  );
+  assert.equal(match[1], "http://127.0.0.1:9999");
+  child.kill("SIGKILL");
+  await waitForExit(child);
+});
+
+test("scans stdout as well as stderr", async () => {
+  const { child, match } = await startAnnouncedChild({
+    command: process.execPath,
+    args: ["-e", 'console.log("ready on 4242")'],
+    pattern: /ready on (\d+)/,
+    onSpawn: () => {},
+    what: "probe",
+    timeoutMs: 10_000,
+    pollMs: 25,
+  });
+  assert.equal(match[1], "4242");
+  child.kill("SIGKILL");
+  await waitForExit(child);
+});
+
+test("a non-announcing child is still reachable and killable after the timeout", async () => {
+  // The regression: a child that is alive but never announces. The throw must
+  // not be the only thing that happens — the caller must already hold the
+  // handle, or `process.exit(1)` leaves this running.
+  let published = null;
+  await assert.rejects(
+    startAnnouncedChild({
+      // Sleeps well past the budget and prints nothing matching.
+      command: process.execPath,
+      args: ["-e", "setTimeout(() => {}, 60000)"],
+      pattern: /never going to match/,
+      onSpawn: (c) => (published = c),
+      what: "probe",
+      timeoutMs: 300,
+      pollMs: 25,
+    }),
+    /probe did not start within/,
+  );
+
+  assert.ok(published, "the child was published before the readiness wait");
+  assert.equal(
+    published.exitCode,
+    null,
+    "and it is still alive, as in the bug",
+  );
+
+  published.kill("SIGKILL");
+  await waitForExit(published);
+  assert.ok(
+    published.exitCode !== null || published.signalCode !== null,
+    "teardown could actually stop it",
+  );
+});
+
+test("reports a child that exits before announcing, with its output", async () => {
+  let published = null;
+  await assert.rejects(
+    startAnnouncedChild({
+      command: process.execPath,
+      args: ["-e", 'console.error("boom: bad config"); process.exit(3)'],
+      pattern: /never going to match/,
+      onSpawn: (c) => (published = c),
+      what: "probe",
+      timeoutMs: 10_000,
+      pollMs: 25,
+    }),
+    (err) =>
+      /probe exited early/.test(err.message) &&
+      /boom: bad config/.test(err.message),
+  );
+  assert.ok(published, "published even on the early-exit path");
+  await waitForExit(published);
+});
+
+test("reports a spawn failure rather than throwing it uncaught", async () => {
+  await assert.rejects(
+    startAnnouncedChild({
+      command: "definitely-not-an-executable-2000",
+      args: [],
+      pattern: /never/,
+      onSpawn: () => {},
+      what: "probe",
+      timeoutMs: 10_000,
+      pollMs: 25,
+    }),
+    /could not spawn the probe:/,
+  );
+});