diff --git a/docs/quality-gate.md b/docs/quality-gate.md index d5309172..94eb2112 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -40,7 +40,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `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. `workflow-gate.test.mjs` is a different shape again ([#2146](https://github.com/modelcontextprotocol/inspector/issues/2146)): besides the table of parser cases it runs the parser over the repo's real `.github/workflows/**`, so a workflow that invokes the local-only gate or a non-Chromium engine pass fails here rather than in a CI run nobody expected to be red. Five more arrive with the skills (#2163). `skill-manifest.test.mjs` pins the frontmatter contract one case per rule — most importantly that a fence which is not a whole line is not a terminator, and that malformed YAML is an error rather than an empty description. `verify-skills.main.test.mjs` drives that guard's `main()` over fixture directories, the sibling guards' `*.main.test.mjs` pattern, so it cannot stop enforcing while the parser tests stay green. `skill-eval.test.mjs` covers the trigger eval's stream parsing and its run classification, which an eval run's happy path never reaches and which would otherwise report a plausible hit rate for a CLI that failed to run. `verify-skills-cli.test.mjs` covers which validator gets run — a local CLI only at the exact pin — and the orchestration around it, neither of which an ordinary run on a working machine exercises. `claude-cli.test.mjs` covers the one place that decides how the CLI is spawned — the Windows `.cmd` shim needs a shell, and its arguments then need quoting — with the platform **injected**, since a win32-only bug that no runner here executes is exactly the kind that ships. 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*. A **second tier** (#2226) runs alongside it, asking the weaker but broader question the `AGENTS.md` rule actually states: does a package this repo *declares* anywhere resolve to two versions across our installs at all? Its candidate set is every name in any install's `dependencies`/`devDependencies`/`optionalDependencies` — unioned across the root and all four clients, so a copy declared by only one of them still counts — that **more than one install holds a top-level copy of** (17 packages today). Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range to govern, not ours. Neither tier subsumes the other — the program tier sees a copy no manifest names (`@modelcontextprotocol/sdk`, arriving through another package's `.d.ts`), while the declared tier sees a **transitive** copy no program loads (cli's `@types/node`, hoisted via `@types/express` — the case that motivated it), two **clients** disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `eslint`/`typescript`/`vitest` that never enter a program. Same deny-by-default and same within-a-major rule, against its own `TOLERATED_DECLARED_SKEW` — also empty. Two limits: it reads **lockfiles**, so an uncommitted hand-installed copy is invisible, and it compares only declared names, so a purely transitive package no manifest names stays the first tier's business. Runs in `validate`. -| `npm run verify:test-timeouts` | Guards the wall-clock budgets the test gates run under (#2323). The class it encodes against is a budget **nobody chose**: three of the six Vitest projects ran on Vitest's own `testTimeout: 5000` and five on its `hookTimeout`/`teardownTimeout: 10000`, sized for an idle machine rather than the one this team works on — three or four concurrent agent sessions in separate worktrees, each free to run a full `local:gate`, on eight logical cores. A correct, deterministic test cut off by such a budget fails a gate its diff did not break, which is the same "channel nobody trusts" failure [Lint has no warning tier](../AGENTS.md#lint-has-no-warning-tier) describes from the other direction; #2292, #2278, #2250, #1942 and #1742 were each an instance, found one site at a time. It asks **Vitest itself** to resolve each of the four configs and reads the number a test actually gets, rather than checking that a key is absent from a config block — which would pass just as happily on a config that had stopped being loaded. Three properties beyond the numbers: a **seventh project** with no row is an error rather than a silent skip, since an unstated budget is precisely the drift being prevented; **`retry` must stay unset**, because a retry turns a load-induced red into a silent green on the only pre-push gate here and would re-open #1596; and both web projects must `configure({ asyncUtilTimeout })`, the Testing Library half that no Vitest config can see — it governs 788 `waitFor`/`findBy*` sites and is the binding bound on any async assertion, being tighter than any per-test budget here. What is enforced there is that the value is **stated**, not that it is large: raising it to 5000 was tried and measured worse (three arms of three full unit runs — unmodified `v2/main` 3/3 green, the raise 1/4/0 files timing out, the raise reverted 3/3 green), because a wait that is *meant* to expire spends its whole budget on the happy path, so a 5x raise is a 5x cost on exactly those tests and starves the worker pool. The reasoning is recorded at the call site. Observed **red** against the pre-#2323 config before it was trusted. Runs in `validate`; its pure decision logic has a sibling `verify-test-timeouts.test.mjs` under `test:scripts`. +| `npm run verify:test-timeouts` | Guards the wall-clock budgets the test gates run under (#2323). The class it encodes against is a budget **nobody chose**: three of the six Vitest projects ran on Vitest's own `testTimeout: 5000` and five on its `hookTimeout`/`teardownTimeout: 10000`, sized for an idle machine rather than the one this team works on — three or four concurrent agent sessions in separate worktrees, each free to run a full `local:gate`, on eight logical cores. A correct, deterministic test cut off by such a budget fails a gate its diff did not break, which is the same "channel nobody trusts" failure [Lint has no warning tier](../AGENTS.md#lint-has-no-warning-tier) describes from the other direction; #2292, #1942 and #1742 were each an instance, found one site at a time. Deliberately **not** cited here: #2278 (a missing condition wait around a geometry read) and #2250 (a real race in a test's own timing) were fixed by making the test wait for the right thing, and presenting a race fix as evidence for a larger ceiling would argue against #1596. It asks **Vitest itself** to resolve each of the four configs and reads the number a test actually gets, rather than checking that a key is absent from a config block — which would pass just as happily on a config that had stopped being loaded. Three properties beyond the numbers: a **seventh project** with no row is an error rather than a silent skip, since an unstated budget is precisely the drift being prevented; **`retry` must stay unset**, because a retry turns a load-induced red into a silent green on the only pre-push gate here and would re-open #1596; and both web projects must `configure({ asyncUtilTimeout })`, the Testing Library half that no Vitest config can see — it governs 788 `waitFor`/`findBy*` sites and is the binding bound on any async assertion, being tighter than any per-test budget here. What is enforced there is that the value is **stated**, not that it is large: raising it to 5000 was tried and measured worse (three arms of three full unit runs — unmodified `v2/main` 3/3 green, the raise 1/4/0 files timing out, the raise reverted 3/3 green), because a wait that is *meant* to expire spends its whole budget on the happy path, so a 5x raise is a 5x cost on exactly those tests and starves the worker pool. The reasoning is recorded at the call site. Observed **red** against the pre-#2323 config before it was trusted. Runs in `validate`; its pure decision logic has a sibling `verify-test-timeouts.test.mjs` under `test:scripts`. | `npm run local:gate` | **Mandatory pre-push command.** `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. A strict superset of GitHub CI — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | diff --git a/scripts/lib/browser-timeouts.mjs b/scripts/lib/browser-timeouts.mjs index ed3a8bb1..37de15a6 100644 --- a/scripts/lib/browser-timeouts.mjs +++ b/scripts/lib/browser-timeouts.mjs @@ -10,16 +10,23 @@ * has, and for the same reason: a budget nobody can find is a budget nobody * revisits. * - * These are generous on purpose and cost a passing run nothing. Playwright - * budgets are ceilings on a poll, not sleeps: every one of them returns the - * instant its locator condition holds. What they have to absorb is a smoke - * running against a cold `dist/` build on a machine already carrying three - * other agent sessions' gates. + * Three of the four are ceilings on a poll rather than sleeps — `ui`, + * `roundTrip` and `nested` each return the instant their locator condition + * holds, so a passing smoke pays only the time the app actually took. What they + * absorb is a smoke running against a cold `dist/` build on a machine already + * carrying other agent sessions' gates. * - * ⚠️ Raising one of these does NOT make a smoke wait longer for a server that - * is never coming: the launcher-death race in `smoke-web-browser.mjs` and the - * `waitForStage` diagnostics in the other two are what report that, and they - * report it with a cause rather than a timeout. + * ⚠️ `bestEffort` is the exception and is not free: its expiry is caught and + * ignored, so a smoke that never reaches network idle spends the full budget on + * an otherwise **passing** run. That is why it is the smallest of the four, and + * why it should stay that way. + * + * ⚠️ Raising any of these does delay the report when the thing waited on never + * arrives — a larger budget is a longer wait before the failure. What the + * launcher-death race in `smoke-web-browser.mjs` and the `waitForStage` + * diagnostics in the other two buy is a better *eventual* message, naming a + * cause instead of a locator; they do not make the wait shorter. Weigh both + * when changing a value here. */ export const BROWSER_TIMEOUTS = Object.freeze({ diff --git a/scripts/verify-test-timeouts.mjs b/scripts/verify-test-timeouts.mjs index edafe525..770cf7eb 100644 --- a/scripts/verify-test-timeouts.mjs +++ b/scripts/verify-test-timeouts.mjs @@ -10,8 +10,13 @@ * eight logical cores). A correct, deterministic test cut off mid-flight by * such a budget fails a gate its diff did not break, which trains people to * re-run rather than read — the same argument AGENTS.md's "Lint has no warning - * tier" makes from the other direction. #2292, #2278, #2250, #1942 and #1742 - * were each that class, found and fixed one site at a time. + * tier" makes from the other direction. #2292, #1942 and #1742 were each that + * class — a budget or poll ceiling nobody chose — found and fixed one site at a + * time. ⚠️ Not every timing failure in this repo's history belongs here: #2278 + * was a missing condition wait around a geometry read and #2250 a genuine race + * in a test's own timing, both fixed by making the test wait for the right + * thing. Citing those as evidence for a larger ceiling would argue against + * #1596, which this guard is meant to uphold rather than erode (Copilot). * * Three properties, which is what makes this worth a guard rather than a * comment: @@ -172,22 +177,67 @@ export function checkProject(name, config, expected = EXPECTED_PROJECTS) { } /** - * Strip `//` and block comments so a commented-out call cannot satisfy a check. + * Strip comments so a disabled call cannot satisfy a check. * * Comment-aware rather than exact, deliberately: a naive scan of the raw source * would accept a `// configure({ asyncUtilTimeout: 1000 })` left behind by * someone disabling it, which is the most likely way this stops being - * configured (Copilot). String literals are not parsed out — a `configure(` in - * a string would still be accepted — but these two files are fifteen lines each - * and the false-positive that matters is the commented one. + * configured (Copilot). + * + * A scanner rather than a regex, for two reasons that a regex gets wrong in + * opposite directions. It must strip a comment that **follows code** on a line + * — `const disabled = true; // configure({ … })` — which an anchored + * line-comment pattern misses entirely (Copilot); and it must NOT treat the + * `//` inside a string literal as the start of one, which an unanchored pattern + * would, silently truncating any line holding a URL. Tracking quotes is the + * only way to have both. * * @param {string} source * @returns {string} */ export function stripComments(source) { - return source - .replace(/\/\*[\s\S]*?\*\//g, "") - .replace(/(^|\n)\s*\/\/[^\n]*/g, "$1"); + let out = ""; + let quote = null; + for (let i = 0; i < source.length; i += 1) { + const c = source[i]; + const next = source[i + 1]; + if (quote) { + // A backslash escapes the next character, so a `\"` cannot close a `"`. + if (c === "\\") { + out += c + (next ?? ""); + i += 1; + continue; + } + if (c === quote) quote = null; + out += c; + continue; + } + if (c === '"' || c === "'" || c === "`") { + quote = c; + out += c; + continue; + } + if (c === "/" && next === "/") { + while (i < source.length && source[i] !== "\n") i += 1; + // Keep the newline, so line structure (and any anchored match a caller + // makes) survives the strip. + out += "\n"; + continue; + } + if (c === "/" && next === "*") { + i += 2; + while ( + i < source.length && + !(source[i] === "*" && source[i + 1] === "/") + ) { + i += 1; + } + i += 1; + continue; + } + out += c; + } + return out; } /** diff --git a/scripts/verify-test-timeouts.test.mjs b/scripts/verify-test-timeouts.test.mjs index 5fd35f4c..833edadd 100644 --- a/scripts/verify-test-timeouts.test.mjs +++ b/scripts/verify-test-timeouts.test.mjs @@ -222,6 +222,34 @@ configure({ asyncUtilTimeout: 1_000 });`; assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []); }); +test("a call disabled in a TRAILING line comment does not satisfy the check", () => { + // The anchored regex this replaced only stripped `//` at the start of a line, + // so a call commented out after code survived and the guard passed with no + // effective configuration (Copilot). + const source = `import { configure } from "storybook/test"; +const disabled = true; // configure({ asyncUtilTimeout: 1000 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [ + "does not call configure()", + ]); +}); + +test("a // inside a string literal is not treated as a comment", () => { + // The failure mode in the other direction: an unanchored strip would truncate + // any line holding a URL, which could remove the real call. + const source = `import { configure } from "storybook/test"; +const docs = "https://testing-library.com/docs"; +configure({ asyncUtilTimeout: 1000 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []); + assert.match(stripComments(source), /https:\/\/testing-library/); +}); + +test("stripComments handles escapes and mid-line block comments", () => { + assert.equal(stripComments("a /* x */ b"), "a b"); + // A backslash-escaped quote must not close the string early, or everything + // after it would be scanned as code. + assert.match(stripComments('const s = "a\\" // b";\nkeep'), /keep/); +}); + test("stripComments leaves executable code alone", () => { const source = `/** header */ import { configure } from "storybook/test"; // trailing