diff --git a/AGENTS.md b/AGENTS.md index 7a864308..5f9e44bc 100644 --- a/AGENTS.md +++ b/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=&openApp=…&appArgs=…&autoOpen=`). 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. diff --git a/scripts/lib/prod-web-server.mjs b/scripts/lib/prod-web-server.mjs index 708933ee..44986c2c 100644 --- a/scripts/lib/prod-web-server.mjs +++ b/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 }), }; } diff --git a/scripts/lib/prod-web-server.test.mjs b/scripts/lib/prod-web-server.test.mjs index bcf65f25..d3f46671 100644 --- a/scripts/lib/prod-web-server.test.mjs +++ b/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); });