test(smoke): exercise the real teardown path, not just removeSafe

Copilot was right that the previous test proved only that `removeSafe` can
delete a directory — it never drove `stop()`, so deleting the cleanup call
would have left the unit suite AND all three smokes green. That is exactly the
regression these tests exist to catch, and they did not catch it.

Extract the teardown into an exported `teardownWebServer({ child, catalogDir,
label })`, which `stop()` now delegates to, and drive it against a stand-in
child process (a real `node -e setInterval` that stays alive until signalled)
rather than a mock. Three cases:

  - it removes the catalog dir it was given (asserted on the dir itself, not a
    spy, so the assertion cannot drift from the behavior);
  - it awaits the child's exit before removing — the #1801 race in one
    assertion, since losing the await is otherwise invisible;
  - an already-dead child is not an error, which is the failure path a smoke
    takes when it calls fail() after the launcher has crashed.

Mutation-checked: with the `removeSafe` call deleted, 2 of the 3 now fail
(previously: 0). AGENTS.md updated to match.

Signed-off-by is now on every commit (#1978 DCO); the requirement itself is
undocumented and tracked in #1979.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>

AGENTS.md

@@ -659,7 +659,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab
 - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below.
 - `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. It shares the spawn/readiness/teardown helper (`scripts/lib/prod-web-server.mjs`) with **`smoke:web:browser` and `smoke:web:app`**, so the three can't drift.
 
-  **Every web smoke runs against a throwaway catalog (#1977).** The helper mints a temp dir per run and passes it as `MCP_CATALOG_PATH`; without it the web backend falls back to the developer's real `~/.mcp-inspector/mcp.json`, which made these smokes both destructive and non-deterministic — `smoke:web:app`'s deep link persists a `deep-link` server row, so a *second* run found it already on disk, raced hydration, and drew a spurious (swallowed, non-fatal) 409 that was really just residue from the previous run. CI never saw it: a fresh `HOME` per run made every CI run look like a first run. This matches `smoke:cli` / `smoke:tui`, which have always driven a temp `--catalog`. Only the **catalog** is redirected — other per-user state under `~/.mcp-inspector` (OAuth tokens, `storage/`) stays shared, because isolating it means redirecting `HOME` wholesale, which would also move the npx and Playwright caches these smokes depend on. Teardown uses **both** halves of `scripts/lib/child-cleanup.mjs` (`stopChild` to await the child's exit, then `removeSafe` to delete the dir) — a bare `kill()` only *delivers* the signal, so removing synchronously re-enters the #1801 ENOTEMPTY race. That makes `stop()` **async**, so every caller must `await` it (and a caller's own `fail()`/`shutdown()` becomes async in turn, or execution runs past the intended exit). The isolation contract's pure halves — `createTempCatalog` and `buildWebServerEnv` — are unit-tested in `scripts/lib/prod-web-server.test.mjs` via `test:scripts`, since the smokes exit immediately after teardown and so cannot detect a regression that silently reshared the catalog.
+  **Every web smoke runs against a throwaway catalog (#1977).** The helper mints a temp dir per run and passes it as `MCP_CATALOG_PATH`; without it the web backend falls back to the developer's real `~/.mcp-inspector/mcp.json`, which made these smokes both destructive and non-deterministic — `smoke:web:app`'s deep link persists a `deep-link` server row, so a *second* run found it already on disk, raced hydration, and drew a spurious (swallowed, non-fatal) 409 that was really just residue from the previous run. CI never saw it: a fresh `HOME` per run made every CI run look like a first run. This matches `smoke:cli` / `smoke:tui`, which have always driven a temp `--catalog`. Only the **catalog** is redirected — other per-user state under `~/.mcp-inspector` (OAuth tokens, `storage/`) stays shared, because isolating it means redirecting `HOME` wholesale, which would also move the npx and Playwright caches these smokes depend on. Teardown uses **both** halves of `scripts/lib/child-cleanup.mjs` (`stopChild` to await the child's exit, then `removeSafe` to delete the dir) — a bare `kill()` only *delivers* the signal, so removing synchronously re-enters the #1801 ENOTEMPTY race. That makes `stop()` **async**, so every caller must `await` it (and a caller's own `fail()`/`shutdown()` becomes async in turn, or execution runs past the intended exit). The isolation contract is unit-tested in `scripts/lib/prod-web-server.test.mjs` via `test:scripts`, since the smokes exit immediately after teardown and so cannot detect a regression that silently reshared the catalog or stopped cleaning up: `createTempCatalog` and `buildWebServerEnv` cover *which* catalog the server gets, and `teardownWebServer` — extracted from `stop()` for exactly this reason — is driven against a stand-in child process so the teardown asserts on the real directory rather than a spy.
 - `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a _class_ (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first _call_ into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A _synchronous_ such throw fires `pageerror`; its _async_ twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never _called_ ships a harmless `{}` and is invisible here by design. Every _other_ console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed).
 - `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=<token>&openApp=…&appArgs=…&autoOpen=<token>`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout.
 - **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's _browser-externalization warning_ (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the _called-at-init_ case (which `smoke:web:browser` also catches, but later/at runtime) **and** the _imported-but-never-called_ case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin _records_ the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — **and** to the browser (`client`) environment (`applyToEnvironment`), so a future SSR/node environment built from this config isn't failed for a legitimate `node:*` import; the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about.

scripts/lib/prod-web-server.mjs

@@ -78,6 +78,36 @@ export function buildWebServerEnv({
   };
 }
 
+/**
+ * Terminate a spawned web server, then remove its catalog dir.
+ *
+ * The two halves are the documented pair in `child-cleanup.mjs`, and both are
+ * needed. `stopChild` closes the #1801 race on the normal path (a bare `kill()`
+ * only *delivers* the signal, so a synchronous remove can hit ENOTEMPTY when the
+ * server writes the catalog on its way out); `removeSafe` then makes the
+ * residual case harmless, warning instead of throwing so a leftover temp dir can
+ * never turn a passing smoke red.
+ *
+ * Exported separately from `startProdWebServer` so the teardown *contract* is
+ * testable against a stand-in child, without booting a real launcher. Testing it
+ * matters more than it looks: deleting the `removeSafe` call would leave both the
+ * unit suite and all three smokes green (the smokes exit immediately after
+ * teardown), which is precisely the regression the tests exist to catch.
+ *
+ * @param {object} opts
+ * @param {import("node:child_process").ChildProcess} opts.child
+ * @param {string} opts.catalogDir
+ * @param {string} [opts.label]
+ */
+export async function teardownWebServer({
+  child,
+  catalogDir,
+  label = "smoke:web",
+}) {
+  await stopChild(child, { label, what: "prod web server" });
+  removeSafe(catalogDir, { label });
+}
+
 /**
  * Spawn `mcp-inspector --web` (prod, no `--dev`) against the built
  * `clients/web/dist` and return handles for readiness + teardown.
@@ -194,18 +224,7 @@ export function startProdWebServer({ host, port, token, label = "smoke:web" }) {
     catalogPath,
     waitForReady,
     whenChildExits,
-    /**
-     * Terminate the server, then remove its catalog dir. **Await this** — the
-     * two halves are the documented pair in `child-cleanup.mjs`, and both are
-     * needed. `stopChild` closes the #1801 race on the normal path (a bare
-     * `kill()` only *delivers* the signal, so a synchronous remove can hit
-     * ENOTEMPTY when the server writes the catalog on its way out); `removeSafe`
-     * then makes the residual case harmless, warning instead of throwing so a
-     * leftover temp dir can never turn a passing smoke red.
-     */
-    stop: async () => {
-      await stopChild(child, { label, what: "prod web server" });
-      removeSafe(catalogDir, { label });
-    },
+    /** Terminate the server, then remove its catalog dir. **Await this.** */
+    stop: () => teardownWebServer({ child, catalogDir, label }),
   };
 }

scripts/lib/prod-web-server.test.mjs

@@ -15,11 +15,17 @@
 
 import { test } from "node:test";
 import assert from "node:assert/strict";
-import { existsSync, mkdirSync, writeFileSync } from "node:fs";
+import { spawn } from "node:child_process";
+import { once } from "node:events";
+import { existsSync, writeFileSync } from "node:fs";
 import { dirname } from "node:path";
 
-import { buildWebServerEnv, createTempCatalog } from "./prod-web-server.mjs";
-import { removeSafe } from "./child-cleanup.mjs";
+import {
+  buildWebServerEnv,
+  createTempCatalog,
+  teardownWebServer,
+} from "./prod-web-server.mjs";
+import { hasExited, removeSafe } from "./child-cleanup.mjs";
 
 const BASE = {
   host: "127.0.0.1",
@@ -53,11 +59,55 @@ test("createTempCatalog: does not create the catalog file itself", () => {
   }
 });
 
-test("createTempCatalog: its dir is removable by removeSafe", () => {
+/**
+ * A stand-in for the web server: a real child process that ignores nothing and
+ * simply stays alive until signalled, so `teardownWebServer` exercises its true
+ * SIGTERM→exit path rather than a mock's idea of one.
+ */
+function spawnIdleChild() {
+  return spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], {
+    stdio: "ignore",
+  });
+}
+
+test("teardownWebServer: removes the catalog dir it was given", async () => {
+  // The regression this guards: dropping the `removeSafe` call from the teardown
+  // would leave every smoke green, since they exit right after teardown and so
+  // never observe the leak. Assert on the dir itself, not on a spy.
+  const child = spawnIdleChild();
   const { dir, path } = createTempCatalog();
-  mkdirSync(dirname(path), { recursive: true });
   writeFileSync(path, "{}");
-  assert.equal(removeSafe(dir), true);
+  assert.ok(existsSync(dir));
+
+  await teardownWebServer({ child, catalogDir: dir, label: "test" });
+
+  assert.equal(existsSync(dir), false, "teardown must remove the catalog dir");
+});
+
+test("teardownWebServer: waits for the child to exit before removing", async () => {
+  // The #1801 race in one assertion: a bare kill() only *delivers* SIGTERM, so a
+  // teardown that removed synchronously could unlink the dir while the server was
+  // still writing to it. If teardown resolves with the child still alive, the
+  // await on stopChild has been lost.
+  const child = spawnIdleChild();
+  const { dir } = createTempCatalog();
+  assert.equal(hasExited(child), false, "child should start alive");
+
+  await teardownWebServer({ child, catalogDir: dir, label: "test" });
+
+  assert.ok(hasExited(child), "teardown must await the child's exit");
+});
+
+test("teardownWebServer: an already-dead child is not an error", async () => {
+  // The failure path: a smoke calls fail() after the launcher already crashed.
+  // Teardown still has to clean up rather than hang on an exit that never comes.
+  const child = spawnIdleChild();
+  child.kill("SIGKILL");
+  await once(child, "exit");
+  const { dir } = createTempCatalog();
+
+  await teardownWebServer({ child, catalogDir: dir, label: "test" });
+
   assert.equal(existsSync(dir), false);
 });