diff --git a/README.md b/README.md index ddca719f..7f73f4ea 100644 --- a/README.md +++ b/README.md @@ -490,7 +490,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | `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:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. 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 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. Two more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. 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. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. 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` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook. A strict superset of GitHub CI — `smoke:web:firefox` (and `smoke:tui`) run only here. | diff --git a/scripts/lib/render-smoke.mjs b/scripts/lib/render-smoke.mjs index e61c3f02..f156f123 100644 --- a/scripts/lib/render-smoke.mjs +++ b/scripts/lib/render-smoke.mjs @@ -246,6 +246,15 @@ export function runRenderSmoke({ // " — quoting an output tail that visibly contains the marker. // First paint has happened; its deadline no longer has a question to ask. clearTimeout(renderTimer); + // `exit` can beat the last of the child's buffered output, and `done` + // then keeps reading until `close` — so this can run *after* the verdict + // is settled. Recording `markerAt` is still right (the deferred message + // below reads it, and a marker that only surfaced in the drain was still + // painted). Arming a timer is not: `done` is a no-op once settled, so it + // could not change the result, but it holds the event loop open for the + // whole survival window — which under `node --test` is a suite that + // hangs on for two seconds per case for no reason. + if (settled) return; // First paint is not the verdict — it only starts the clock on the // question this smoke actually asks. surviveTimer = setTimeout(() => { @@ -267,14 +276,25 @@ export function runRenderSmoke({ child.on("exit", (code, signal) => { childExited = true; + const exitAt = since(); if (settled) return; const how = signal ? `signal ${signal}` : `code ${code}`; + // Thunked: rendered at `finish()`, i.e. after `close`, so `markerAt` is + // whatever the FULL output showed — a marker still sitting in the pipe + // when `exit` fired is accounted for by the time this is read. + // + // Deliberately makes no claim about the order in which this process + // *observed* the two. `exit` and the last `data` event race, so a + // "N ms later" phrased off observation times would flip wording (and + // sign) run to run on the same child. The times are reported as the + // observations they are and the verdict does not rest on them. done(1, () => markerAt === null ? `child exited (${how}) before rendering "${marker}"\n${outputTail(output)}` - : `child rendered "${marker}" at ${markerAt}ms and then exited (${how}) ` + - `${since() - markerAt}ms later, inside the ${surviveMs}ms survival window — ` + - `it painted one frame, it did not run\n${outputTail(output)}`, + : `child rendered "${marker}" and then exited (${how}) inside the ` + + `${surviveMs}ms survival window — it painted one frame, it did not run ` + + `(first paint seen at ${markerAt}ms, exit at ${exitAt}ms)` + + `\n${outputTail(output)}`, ); }); diff --git a/scripts/lib/render-smoke.test.mjs b/scripts/lib/render-smoke.test.mjs index d631c62a..e3b12f1b 100644 --- a/scripts/lib/render-smoke.test.mjs +++ b/scripts/lib/render-smoke.test.mjs @@ -36,12 +36,26 @@ test("fails when the child paints the marker and then exits non-zero", async () const r = await runRenderSmoke({ ...opts, ...paintThenExit(1) }); assert.equal(r.code, 1, `expected failure, got: ${r.message}`); assert.match(r.message, /code 1/); - assert.match(r.message, /survival window/); + assert.match(r.message, /it painted one frame, it did not run/); // The diagnostic must not read as a crash *before* first paint — that was a // different defect, and conflating them sends the reader to the wrong place. assert.doesNotMatch(r.message, /before rendering/); }); +// `exit` races the child's last `data` event, so this stub can be observed +// either way round on the same machine. Both orders must produce the same +// verdict AND the same wording — a message phrased off observation order would +// flip run to run, which is how a stable failure gets written off as flake. +// Repeated because one pass proves nothing about a race. +test("the verdict does not depend on whether exit beats the final output", async () => { + for (let i = 0; i < 12; i++) { + const r = await runRenderSmoke({ ...opts, ...paintThenExit(1) }); + assert.equal(r.code, 1, `run ${i}: expected failure, got: ${r.message}`); + assert.match(r.message, /it painted one frame, it did not run/, `run ${i}`); + assert.doesNotMatch(r.message, /before rendering/, `run ${i}`); + } +}); + // Exiting 0 right after painting is the same failure: the app did not run. It // is called out separately because `script(1)` under busybox reports 0 for a // crashed child, so the verdict cannot rest on the exit code. @@ -159,10 +173,12 @@ test("a zero survival window falls back rather than passing instantly", async () surviveMs: Number(""), }); assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + // The window named in the message is the normalized one, not the 0 passed in. assert.match( r.message, new RegExp(`${DEFAULTS.surviveMs}ms survival window`), ); + assert.doesNotMatch(r.message, /\b0ms survival window/); }); test("outputTail drops a leading partial line but keeps short output whole", () => {