From 9cade95fd00995579d47d41e385e93fbd1627c58 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 12:48:53 -0400 Subject: [PATCH] test(scripts): kill the probe child on every path; document the helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings from round 2, both correct. 1. The regression suite killed its child only after the assertions. If `assert.rejects` or either assertion failed, execution never reached the kill, leaving a 60-second child attached to the runner — a suite guarding a process leak would leak one itself, precisely on the run where the regression had come back. Teardown is now registered from `onSpawn` via `t.after`, so it runs on every path out of every case. That is the helper's own ownership rule applied to the tests; killing an already-exited child is a no-op, so the early-exit cases pay nothing for it. 2. Document the new shared helper, per the maintenance rules. AGENTS.md's scripts/lib tree entry now carries announced-child.mjs and what its contract buys, and the smoke:web:app paragraph records why the spawn/readiness step moved out of the smoke (same reason teardownWebServer left prod-web-server's stop()). README's test:scripts row names the new suite alongside resolve-node-bin's. Not mirrored into .github/copilot-instructions.md: that file is a distillation of the rules a reviewer cites against a diff, and deliberately excludes the project-structure tree and tooling inventory. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- AGENTS.md | 10 ++++-- README.md | 2 +- scripts/lib/announced-child.test.mjs | 53 +++++++++++++++++----------- 3 files changed, 41 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d68142c1..a63b8d0d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -185,7 +185,13 @@ v2/main/ │ # pack-and-verify.mjs, and lib/ shared helpers │ # (tsc-program.mjs is the `tsc --listFilesOnly` │ # measurement both coverage guards read a program -│ # through — #1965; resolve-node-bin.mjs resolves a +│ # through — #1965; announced-child.mjs owns the +│ # spawn-and-wait-for-a-readiness-line step, publishing +│ # the child via `onSpawn` BEFORE the wait so the +│ # caller's teardown reaches it on every throw path — +│ # the readiness timeout included, which is what used +│ # to orphan a live server holding its port — #2000; +│ # resolve-node-bin.mjs resolves a │ # package's bin through its own package.json, so the │ # verify/smoke scripts spawn it with `process.execPath` │ # rather than the `npx` `.cmd` shim Node refuses to @@ -773,7 +779,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab **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. +- `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`. Both mechanics now live in `scripts/lib/announced-child.mjs` rather than in the smoke, so the failure path is testable: this smoke's happy path always receives the announcement, so nothing it could assert would prove that a child alive *through* the 30s timeout is still reachable by `shutdown()` — the case that orphaned a live server (#2000). The helper publishes the child via `onSpawn` before waiting, and `scripts/lib/announced-child.test.mjs` drives real `node -e` children (not spies) to assert it is published, still alive when the throw lands, and actually killable. Same reason `teardownWebServer` was extracted from `prod-web-server.mjs`'s `stop()`. **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. - `smoke:web:elicit` (`scripts/smoke-web-elicitation.mjs`, #1854) is the app-rendered **elicitation** counterpart of `smoke:web:app`: same prod `--web` server and the same deep-link connect, but it then calls `app_choose_option` from the Tools tab, waits for `[data-testid="app-elicitation"][data-app-elicitation-status="ready"]`, clicks a choice **inside the sandboxed app** (two `frameLocator` hops — the trusted sandbox proxy, then the untrusted app), and asserts the app's standard `ElicitResult` comes back in the *tool result*, i.e. that it reached the server rather than merely the host. It then repeats against `app-elicitation-native-http.json` — the same tool and app on a server that never advertised the nested MCP Apps `elicitation` capability — and asserts the **native** elicitation dialog takes it and no app modal is rendered. That second half is the more valuable one: the failure this feature can produce is not "the app doesn't render" but "an app renders when it should not have been offered one", which strands every user of a server that never opted in. Set `SMOKE_SCREENSHOT_DIR` to capture PNGs of the three states (used for PR proof); unset, it asserts only. Two mechanics worth knowing: the main-view tabs are a Mantine `SegmentedControl`, so there is no `role="tab"` — the clickable element is the sibling `label[for$="-Tools"]`; and the prompt string also appears in the (hidden) Protocol-tab payload, so the fallback assertion is scoped to the dialog rather than a bare text lookup. - **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/README.md b/README.md index 672d86a6..e8800272 100644 --- a/README.md +++ b/README.md @@ -368,7 +368,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus three headless-Chromium smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus `scripts/lib/resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | +| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | diff --git a/scripts/lib/announced-child.test.mjs b/scripts/lib/announced-child.test.mjs index f8ff8847..a47f6977 100644 --- a/scripts/lib/announced-child.test.mjs +++ b/scripts/lib/announced-child.test.mjs @@ -22,43 +22,55 @@ function waitForExit(child) { return new Promise((resolve) => child.once("close", resolve)); } -test("hands the child to onSpawn before waiting, and returns the match", async () => { +/** + * Build an `onSpawn` that registers teardown the moment the child exists, so it + * is killed on every path out of the test — a failed assertion included. + * + * This is the helper's own ownership rule applied to the suite: killing only on + * the success path would leak a 60-second child into the runner precisely when + * the regression being guarded has come back, which is the one run where a leak + * matters most. Killing an already-exited child is a no-op, so the early-exit + * cases pay nothing for it. + */ +function killOnTeardown(t, capture = () => {}) { + return (child) => { + capture(child); + t.after(async () => { + child.kill("SIGKILL"); + await waitForExit(child); + }); + }; +} + +test("hands the child to onSpawn before waiting, and returns the match", async (t) => { 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), + onSpawn: killOnTeardown(t, (c) => (seen = c)), what: "probe", timeoutMs: 10_000, pollMs: 25, }); - assert.equal( - seen, - child, - "onSpawn received the same child that was returned", - ); + assert.equal(seen, child, "onSpawn got the 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({ +test("scans stdout as well as stderr", async (t) => { + const { match } = await startAnnouncedChild({ command: process.execPath, args: ["-e", 'console.log("ready on 4242")'], pattern: /ready on (\d+)/, - onSpawn: () => {}, + onSpawn: killOnTeardown(t), 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 () => { +test("a non-announcing child is still reachable and killable after the timeout", async (t) => { // 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. @@ -69,7 +81,7 @@ test("a non-announcing child is still reachable and killable after the timeout", command: process.execPath, args: ["-e", "setTimeout(() => {}, 60000)"], pattern: /never going to match/, - onSpawn: (c) => (published = c), + onSpawn: killOnTeardown(t, (c) => (published = c)), what: "probe", timeoutMs: 300, pollMs: 25, @@ -92,14 +104,14 @@ test("a non-announcing child is still reachable and killable after the timeout", ); }); -test("reports a child that exits before announcing, with its output", async () => { +test("reports a child that exits before announcing, with its output", async (t) => { 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), + onSpawn: killOnTeardown(t, (c) => (published = c)), what: "probe", timeoutMs: 10_000, pollMs: 25, @@ -109,16 +121,15 @@ test("reports a child that exits before announcing, with its output", async () = /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 () => { +test("reports a spawn failure rather than throwing it uncaught", async (t) => { await assert.rejects( startAnnouncedChild({ command: "definitely-not-an-executable-2000", args: [], pattern: /never/, - onSpawn: () => {}, + onSpawn: killOnTeardown(t), what: "probe", timeoutMs: 10_000, pollMs: 25,