modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

refactor: assert retry and asyncUtilTimeout at runtime, delete the source scanner

Browse source 
Round 8 of the review asked for an AST. That was the right diagnosis of the
wrong problem: the question was never which parser to use, it was why a
test-timeout guard was parsing JavaScript at all.

It got there one reasonable step at a time. The guard enforces two rules no
Vitest config can report — a per-test `retry`, and Testing Library's
`asyncUtilTimeout` — so it read them out of source text, and the review found a
new valid spelling it missed in five consecutive rounds: a trailing comment,
nested parens in `it.each([makeCase()])`, a member-access prefix, the shorthand
`{ retry }`, an aliased import, a non-literal title. Each fix was correct and
each made it more parser-shaped, until it carried eight helpers doing quote
tracking and bracket balancing.

Both rules are observable at runtime, exactly:

- `vitest.setup.shared.mts` is loaded by all six projects and reads
  `ctx.task.retry` — the value Vitest resolved for the test. That covers
  strictly more than the scan did: a per-test option, a `describe` option
  inherited by children, a project-level setting, and a `--retry` flag typed at
  a command line, which the scan could never see since it only read npm scripts.
- `clients/web/src/test/asyncUtilTimeout.test.ts` asserts
  `getConfig().asyncUtilTimeout`, the value this project's own waitFors read.

Neither has a spelling to anticipate, so neither can be outrun by a new one.
Both were observed firing before being trusted.

The guard keeps only what it can answer by asking Vitest: resolved budgets,
project-level retry, config discovery, and that each project still loads the
setup file — the one thing about the runtime assertion a config CAN report.
868 lines to 361, every scanner helper gone, and 33 of 48 unit tests deleted
because they pinned edge cases of a thing that no longer exists.

storybookSetup.ts goes with it. At the library's own value its configure() call
was a no-op, and the assertion is what pins the number.

AGENTS.md gains the general rule: a rule about what the code DOES is asserted at
runtime, not read out of the source — reach for a verify:* guard when the
question is about configuration, which a tool can be asked to resolve.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-12 15:58:19 -04:00
commit 69e6097b68
17 changed files with 228 additions and 799 deletions
Download patch file Download diff file Expand all files Collapse all files

6
.claude/skills/pre-push-gate/SKILL.md
View file

@ -89,9 +89,9 @@ than fixing it.
### `verify:test-timeouts`
A Vitest project resolves to a wall-clock budget nobody stated, a web setup
file stopped configuring Testing Library's `asyncUtilTimeout`, or someone added
a `retry`. The shared values live in `vitest.shared.mts` (`TIMEOUTS` /
A Vitest project resolves to a wall-clock budget nobody stated, or stopped
loading `vitest.setup.shared.mts`. (A `retry` itself fails at **runtime**, from
that setup file, with a message naming the test — not here.) The shared values live in `vitest.shared.mts` (`TIMEOUTS` /
`INTEGRATION_TIMEOUTS`) and every project spreads one of them — so **raise a
budget there**, not with a per-suite `}, 30_000)` argument, which only moves the
one site and leaves every future file on the default. A per-suite raise is

15
AGENTS.md
View file

@ -423,6 +423,16 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race.
then it says so: real cross-process lock contention (`file-lock.test.ts`), a
full interactive OAuth round trip (`oauth-interactive.test.ts`). Name the
constant and state the reason at the site.
- **A rule about what the code _does_ is asserted at runtime, not read out of
the source.** Both of this section's runtime checks
(`vitest.setup.shared.mts`, `clients/web/src/test/asyncUtilTimeout.test.ts`)
began as source scanning in #2334, and the review found a new valid
JavaScript spelling the scanner missed in five consecutive rounds — each fix
correct, each making it more parser-shaped, until it carried eight helpers
doing quote tracking and bracket balancing. Asking the runtime needs no
spelling to be anticipated and covers cases the scan could not reach at all.
Reach for `verify:*` when the question is about **configuration**, which a
tool can be asked to resolve; assert at runtime when it is about behavior.
- **A budget is stated, which is not the same as raised.** Testing Library's
`asyncUtilTimeout` is pinned at its own default in both web setup files
because raising it was *measured worse*: a wait that is meant to expire —
@ -443,7 +453,10 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race.
test.
- **`retry` stays unset.** A retry turns a load-induced red into a silent green
on the only pre-push gate this repo has, and hides a real race behind a second
attempt. The guard enforces this.
attempt. `vitest.setup.shared.mts` asserts it **at runtime**, in every
project, reading the value Vitest resolved for the test — so a per-test
option, a `describe` option, a project setting and a `--retry` flag all land
on the same check.
- **The Playwright locator budgets the web smokes use are named constants in
`scripts/lib/browser-timeouts.mjs`** — raise one there, not at a call site.
The *remaining* smoke-helper budgets and CI's missing `timeout-minutes` are

8
clients/cli/vitest.config.ts
View file

@ -1,7 +1,11 @@
import path from "node:path";
import { fileURLToPath } from "node:url";
import { defineConfig } from "vitest/config";
import { TIMEOUTS, vitestSharedPaths } from "../../vitest.shared.mts";
import {
NO_RETRY_SETUP,
TIMEOUTS,
vitestSharedPaths,
} from "../../vitest.shared.mts";
const dirname = path.dirname(fileURLToPath(import.meta.url));
const { projectResolve } = vitestSharedPaths(dirname);
@ -12,7 +16,7 @@ export default defineConfig({
globals: false,
environment: "node",
include: ["__tests__/**/*.test.ts"],
setupFiles: ["__tests__/helpers/mock-open-url.ts"],
setupFiles: ["__tests__/helpers/mock-open-url.ts", NO_RETRY_SETUP],
// Shared budgets (#2323). `testTimeout` was already 15000 here by hand;
// the hook and teardown budgets were Vitest's defaults until now.
...TIMEOUTS,

3
clients/launcher/vitest.config.ts
View file

@ -1,11 +1,12 @@
import { defineConfig } from "vitest/config";
import { TIMEOUTS } from "../../vitest.shared.mts";
import { NO_RETRY_SETUP, TIMEOUTS } from "../../vitest.shared.mts";
export default defineConfig({
test: {
globals: false,
environment: "node",
include: ["__tests__/**/*.test.ts"],
setupFiles: [NO_RETRY_SETUP],
// Shared budgets (#2323). This project holds a single test file, which is
// exactly why it is pinned rather than left on the defaults: it is cheap to
// include and expensive to remember when a second file arrives. This import

8
clients/tui/vitest.config.ts
View file

@ -1,7 +1,11 @@
import path from "node:path";
import { fileURLToPath } from "node:url";
import { defineConfig } from "vitest/config";
import { TIMEOUTS, vitestSharedPaths } from "../../vitest.shared.mts";
import {
NO_RETRY_SETUP,
TIMEOUTS,
vitestSharedPaths,
} from "../../vitest.shared.mts";
const dirname = path.dirname(fileURLToPath(import.meta.url));
const { projectResolve } = vitestSharedPaths(dirname);
@ -12,7 +16,7 @@ export default defineConfig({
globals: false,
environment: "node",
include: ["__tests__/**/*.test.{ts,tsx}"],
setupFiles: ["./__tests__/setup.ts"],
setupFiles: ["./__tests__/setup.ts", NO_RETRY_SETUP],
// Shared budgets (#2323). Ink renders through React, so every assertion
// here waits on a React commit under v8 instrumentation — the shape behind
// the poll-budget raises in #1742 and #1942.

43
clients/web/src/test/asyncUtilTimeout.test.ts Normal file
View file

@ -0,0 +1,43 @@
import { describe, it, expect } from "vitest";
import { getConfig } from "@testing-library/react";
/**
* Pins Testing Library's `asyncUtilTimeout` for the `unit` project (#2323).
*
* That value governs every `waitFor` and `findBy*` here — 788 call sites — and
* is the binding bound on any async assertion, being tighter than the project's
* own per-test budget. So it is a number this repo states rather than inherits,
* and a Testing Library upgrade that changed its default would fail here rather
* than silently move all 788.
*
* ⚠️ The value is the library's own default, and that is a MEASURED decision,
* not a shrug. #2323 proposed 5000, on the reasoning that a contended
* happy-dom render is the worst case. Measured on the machine this team uses,
* 5000 made the suite worse: three arms of three full unit runs each, same
* worktree, comparable load —
*
* • unmodified `v2/main`: 3/3 green
* • the raise to 5000: 1, 4 and 0 files timing out at 15000ms
* • the raise reverted: 3/3 green
*
* A different unrelated set failed each time and every one passed in about a
* second in isolation, which is the shape of CPU starvation rather than of a
* slow assertion. The mechanism: `asyncUtilTimeout` is not only a ceiling — any
* wait that is *meant* to expire spends the whole budget on the happy path, so
* a 5x raise is a 5x cost on exactly those tests, which saturates the worker
* pool and starves tests that were never slow. The tests that assert absence
* that way are the real defect and are #2335; revisit this when they are gone.
*
* ⚠️ This asserts the EFFECTIVE value at runtime rather than checking that
* `setup.ts` contains a `configure()` call. An earlier revision of #2334 did
* the latter by scanning source text, and the review found five ways to satisfy
* the scan without configuring anything — a commented-out call, a call on
* another object, a non-literal value, and so on. `getConfig()` is what the
* `waitFor`s in this project actually read, so there is nothing left to spell
* around.
*/
describe("Testing Library configuration (unit project)", () => {
it("uses the asyncUtilTimeout this repo states, not whatever the library ships", () => {
expect(getConfig().asyncUtilTimeout).toBe(1000);
});
});

35
clients/web/src/test/storybookSetup.ts
View file

@ -1,35 +0,0 @@
/**
* Setup file for the `storybook` Vitest project (#2323).
*
* It exists for one call. Storybook's instrumented Testing Library carries its
* own `asyncUtilTimeout`, and it governs the 80 `waitFor` / `findBy*` sites
* across the story files. It is the *tighter* of the two bounds this project
* has — the per-test ceiling is 15000 (#2292) — so it is stated here rather
* than inherited, which also pins it against a Storybook upgrade changing the
* default under all 80 sites at once.
*
* The value matches the unit project's, and for the reason recorded in
* `setup.ts`: raising it was measured and rejected. A wait that is *meant* to
* expire spends its whole budget on the happy path, so a raise is a
* proportional cost on exactly those tests.
*
* ⚠️ Two things about this file that are deliberate and easy to undo:
*
* 1. **It must not live in `.storybook/`, and must not call
* `setProjectAnnotations`.** `@storybook/addon-vitest` provisions the
* preview annotations itself, and skips doing so when it finds a setup file
* that is *both* inside `configDir` and contains that call (#1898) — so the
* old `.storybook/vitest.setup.ts` was opting out of the automatic path.
* Either half of the condition is enough to stay on it; this file misses
* both. `src/test/PreviewAnnotations.stories.tsx` asserts the provisioning
* directly, so a regression here fails a test rather than silently rendering
* every story without the Mantine decorator.
* 2. **The import is `storybook/test`, not `@testing-library/*`.** Storybook
* instruments its own copy so the interactions panel can trace each step;
* configuring a different copy would change nothing that a play function
* actually calls.
*/
import { configure } from "storybook/test";
configure({ asyncUtilTimeout: 1000 });

8
clients/web/tsconfig.test.json
View file

@ -41,7 +41,13 @@
"src/test/**/*.ts",
"src/test/**/*.tsx",
"../../core/**/__tests__/**/*.ts",
"../../core/**/__tests__/**/*.tsx"
"../../core/**/__tests__/**/*.tsx",
// The shared no-retry setup every Vitest project loads (#2323). It is
// referenced by the configs as a resolved PATH rather than imported, so no
// project's tsconfig picks it up transitively the way `vitest.shared.mts`
// is picked up through `vitest.config.ts` — it needs naming, or it lands in
// no tsconfig project at all and `verify:typecheck-coverage` fails.
"../../vitest.setup.shared.mts"
],
"exclude": []
}

19
clients/web/vite.config.ts
View file

@ -16,6 +16,7 @@ import { buildWebServerConfigFromEnv } from "./server/web-server-config";
import { createBrowserExternalizedBuiltinGate } from "./server/browser-externalized-builtin-gate";
import {
INTEGRATION_TIMEOUTS,
NO_RETRY_SETUP,
TIMEOUTS,
vitestSharedPaths,
} from "../../vitest.shared.mts";
@ -318,7 +319,10 @@ export default defineConfig(({ command }) => {
include: ["clients/web/src/**/*.test.{ts,tsx}"],
// Integration tests run in the integration project below (node env).
exclude: [integrationGlob],
setupFiles: [path.join(dirname, "src/test/setup.ts")],
setupFiles: [
path.join(dirname, "src/test/setup.ts"),
NO_RETRY_SETUP,
],
// Pin after-hooks to LIFO (reverse registration). This is Vitest 4's
// own default (`resolved.sequence.hooks ??= "stack"` — the CLI
// help-text's "parallel" is stale), so this line documents intent and
@ -357,6 +361,7 @@ export default defineConfig(({ command }) => {
// can transform core/ modules and run tests against the source.
root: repoRoot,
include: [integrationGlob],
setupFiles: [NO_RETRY_SETUP],
// Integration tests spawn real HTTP/stdio servers via test-servers/,
// bind sockets, run e2e OAuth flows, and exercise filesystem-backed
// storage. 30s matches the v1.5 core/vitest.config.ts. Now stated
@ -402,11 +407,13 @@ export default defineConfig(({ command }) => {
// now follow it rather than restate it. The hook and teardown
// budgets come along with it, having been left on the defaults.
...TIMEOUTS,
// One `configure({ asyncUtilTimeout })` call, and deliberately not
// in `.storybook/` — see the file's own header for why that
// location and the absence of `setProjectAnnotations` are what keep
// the automatic preview-annotation provisioning below intact.
setupFiles: [path.join(dirname, "src/test/storybookSetup.ts")],
// Carries the shared no-retry assertion and nothing else.
// ⚠️ It is deliberately NOT in `.storybook/` and declares no
// annotations: `@storybook/addon-vitest` skips its automatic
// preview-annotation provisioning only for a setup file that is
// both inside `configDir` and calls `setProjectAnnotations`, and
// this one is neither (#1898).
setupFiles: [NO_RETRY_SETUP],
browser: {
enabled: true,
headless: true,

4
docs/quality-gate.md
View file

@ -29,7 +29,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor
| Script | What it does |
| ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `npm run validate` | Runs the five durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:skills` (every `.claude/skills` manifest parses and declares its invocation mode; the **model-invoked** ones also carry eval cases — the name-only skills intentionally have none), `verify:typecheck-coverage` (every tracked TS file lands in a tsconfig project), `verify:dep-lockstep` (two tiers: no dependency reaching one `tsc` program from two installs skews across them, and — since #2226 — no **declared** package holds two different top-level versions across installs at all), `verify:test-timeouts` (every Vitest project resolves to a budget somebody chose, both web projects configure Testing Library's `asyncUtilTimeout`, and `retry` stays unset in every project, test and script) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate — including the `core/react/**` React-hooks block added by #2192, whose plugin is a **root** devDependency because a client-local copy is unreachable from the root: Node resolution walks up, not down), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. |
| `npm run validate` | Runs the five durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:skills` (every `.claude/skills` manifest parses and declares its invocation mode; the **model-invoked** ones also carry eval cases — the name-only skills intentionally have none), `verify:typecheck-coverage` (every tracked TS file lands in a tsconfig project), `verify:dep-lockstep` (two tiers: no dependency reaching one `tsc` program from two installs skews across them, and — since #2226 — no **declared** package holds two different top-level versions across installs at all), `verify:test-timeouts` (every Vitest project resolves to a budget somebody chose, both web projects configure Testing Library's `asyncUtilTimeout`, and every project loads the runtime no-retry setup) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate — including the `core/react/**` React-hooks block added by #2192, whose plugin is a **root** devDependency because a client-local copy is unreachable from the root: Node resolution walks up, not down), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. |
| `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). |
| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web — the TUI one runs under a **pseudoterminal** and asserts the Ink app is still running two seconds after its first frame, not merely that it painted one, [#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)), plus three headless-browser 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. Those three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). A fourth, **`smoke:web:tabs`**, drives the core tabs against one server on one browser launch — Tools runs a tool and asserts its `structuredContent` section, Resources reads a resource with templates listed, Prompts fetches one ([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)) — asserting the `data-*` contract in [the web README](../clients/web/README.md#core-tab-automation-contract) rather than visible copy. It is **Chromium-only**: those tabs are ordinary React and Mantine, so unlike the sandbox they are not engine-sensitive. |
| `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 local:gate` and of GitHub CI. |
@ -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, #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** — enforced in all three places Vitest accepts one, since the decision is about the behavior rather than about a config key: the resolved project, an individual `it`/`describe` options object (the parameterized `it.each` form included), and a `--retry` flag in an npm script. A human typing `--retry` in their own shell is the one thing no committed check can see, and the guard says so rather than overclaiming. 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: #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. A **seventh project** with no row, or a config file it does not discover (it knows all twelve filenames Vitest accepts, not just the two this repo uses), is an error rather than a silent skip. It also checks that every project loads `vitest.setup.shared.mts`. ⚠️ **Everything it reads comes from a resolved project, never from source text** — the two rules no config can report are asserted at runtime instead: `retry` by `vitest.setup.shared.mts` (which reads the value Vitest resolved, so a per-test option, a `describe` option, a project setting and a `--retry` flag are one check) and Testing Library's `asyncUtilTimeout` by `clients/web/src/test/asyncUtilTimeout.test.ts` (which reads what the project's own `waitFor`s use). Both began as source scanning in #2334 and the review found a new valid spelling missed in five consecutive rounds, so the boundary is deliberate: if a rule cannot be answered by asking the tool, assert it at runtime rather than reading the source for it. Observed **red** against the pre-#2323 config before it was trusted. Runs in `validate`; its 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). |

2
eslint.config.js
View file

@ -101,6 +101,7 @@ export default defineConfig([
files: [
"test-servers/src/**/*.{ts,tsx,mts,cts}",
"vitest.shared.mts",
"vitest.setup.shared.mts",
"eslint.config.js",
],
extends: [js.configs.recommended, tseslint.configs.recommended],
@ -121,6 +122,7 @@ export default defineConfig([
"core/**/*.{ts,tsx}",
"test-servers/src/**/*.{ts,tsx,mts,cts}",
"vitest.shared.mts",
"vitest.setup.shared.mts",
],
...typeAware,
},

6
package.json
View file

@ -52,13 +52,13 @@
"verify:test-timeouts": "node scripts/verify-test-timeouts.mjs",
"validate:core": "npm run format:check:core && npm run format:check:scripts && npm run format:check:shared && npm run lint:core && npm run lint:shared",
"lint:core": "eslint \"core/**/*.{ts,tsx}\" --max-warnings 0",
"lint:shared": "eslint \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts eslint.config.js --max-warnings 0",
"lint:shared": "eslint \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts vitest.setup.shared.mts eslint.config.js --max-warnings 0",
"format:core": "prettier --write \"core/**/*.{ts,tsx}\"",
"format:check:core": "prettier --check \"core/**/*.{ts,tsx}\"",
"format:scripts": "prettier --write \"scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"",
"format:check:scripts": "prettier --check \"scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"",
"format:shared": "prettier --write \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts eslint.config.js",
"format:check:shared": "prettier --check \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts eslint.config.js",
"format:shared": "prettier --write \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts vitest.setup.shared.mts eslint.config.js",
"format:check:shared": "prettier --check \"test-servers/src/**/*.{ts,tsx,mts,cts}\" vitest.shared.mts vitest.setup.shared.mts eslint.config.js",
"format": "npm run format:core && npm run format:scripts && npm run format:shared && cd clients/web && npm run format && cd ../cli && npm run format && cd ../tui && npm run format && cd ../launcher && npm run format",
"validate:cli": "cd clients/cli && npm run validate",
"validate:tui": "cd clients/tui && npm run validate",

513
scripts/verify-test-timeouts.mjs
View file

@ -1,6 +1,6 @@
/**
* Guards the wall-clock budgets every Vitest project in this repo runs under
* (#2323), and the two decisions that came with them.
* (#2323), and the decision that no project retries.
*
* The class it encodes against: a budget nobody chose. Three of the six
* projects ran on Vitest's own `testTimeout: 5000` and five on its
@ -11,48 +11,51 @@
* 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, #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).
* class, 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 upholds.
*
* Three properties, which is what makes this worth a guard rather than a
* comment:
* **Everything here is read from a resolved Vitest project, never from source
* text.** That is a deliberate and hard-won boundary. An earlier revision of
* #2334 also enforced the two rules that no config can report — a per-test
* `retry`, and Testing Library's `asyncUtilTimeout` — by scanning source, and
* the review found a new valid JavaScript spelling it missed in five
* consecutive rounds. Each fix was correct and each made the scanner more
* parser-shaped, until it carried eight helpers doing quote tracking and
* bracket balancing: a bad parser inside a linter. Both rules are now asserted
* where they are unambiguous —
*
* • `retry`, at runtime, by `vitest.setup.shared.mts`, which every project
* loads and which reads the value Vitest actually resolved for the test;
* • `asyncUtilTimeout`, at runtime, by
* `clients/web/src/test/asyncUtilTimeout.test.ts`, which reads the value
* the project's own `waitFor`s use.
*
* Both are strictly stronger than the scan was, because neither has to
* anticipate a spelling. **If a future rule here cannot be answered by asking
* the tool, assert it at runtime — do not read the source for it.**
*
* Three properties of what remains:
*
* 1. **Resolved, not declared.** It asks Vitest to resolve each project and
* reads the number a test actually gets. Asserting that a key is absent from
* some config block would pass just as happily on a config that had stopped
* being loaded at all.
* 2. **Unknown projects fail loudly.** A seventh project added without a row
* here is exactly the drift this exists to prevent, so it is an error rather
* than something the table silently skips.
* 3. **`retry` must stay unset.** A retry converts a load-induced red into a
* silent green on the only pre-push gate this repo has, and would re-open
* #1596 by hiding a real race behind a second attempt. That was a deliberate
* "do not raise" decision, so it is enforced rather than remembered — and
* enforced in all three places Vitest accepts one, since the decision is
* about the behavior rather than about a config key: the resolved project,
* an individual `it`/`describe` options object, and a `--retry` flag in an
* npm script (Copilot). What stays outside its reach is a human typing
* `--retry` into their own shell, which no committed check can see.
*
* It also asserts the Testing Library half, which no Vitest config can see:
* `asyncUtilTimeout` governs every `waitFor` / `findBy*` in the web projects
* and is the binding constraint on an async assertion, since it is tighter than
* any per-test budget here. What is enforced is that each web project *states*
* it — raising it was measured and rejected, for the reason recorded in
* `clients/web/src/test/setup.ts`.
* 2. **Unknown projects fail loudly.** A seventh project with no row here, or a
* config file this guard does not discover, is exactly the drift it exists to
* prevent — so both are errors rather than silent skips.
* 3. **Every project loads the no-retry setup.** The runtime assertion only
* binds a project that actually loads it, so that wiring is checked here
* rather than assumed.
*
* ⚠️ Observed to FAIL against the unfixed config before it was trusted: on
* `origin/v2/main` it reports `unit`, `tui` and `launcher` at
* `testTimeout: 5000`, five of the six projects at `hookTimeout: 10000`, and
* both `asyncUtilTimeout` assertions unmet.
* `testTimeout: 5000` and five of the six projects at `hookTimeout: 10000`.
*/
import { execFileSync } from "node:child_process";
import { existsSync, readdirSync, readFileSync } from "node:fs";
import { existsSync, readdirSync } from "node:fs";
import { dirname, join, relative, resolve } from "node:path";
import { fileURLToPath } from "node:url";
@ -107,38 +110,6 @@ export const CONFIG_ROOTS = Object.freeze([
{ root: "clients/launcher", projects: ["launcher"] },
]);
/**
* Files that must configure Testing Library's `asyncUtilTimeout`, with the
* import each has to configure it through. Storybook instruments its own copy
* of Testing Library so its interactions panel can trace each step, so
* configuring `@testing-library/*` there would configure a copy no play
* function calls.
*/
export const ASYNC_UTIL_SITES = Object.freeze([
{
file: "clients/web/src/test/setup.ts",
from: "@testing-library/react",
project: "unit",
},
{
file: "clients/web/src/test/storybookSetup.ts",
from: "storybook/test",
project: "storybook",
},
]);
/**
* The value those sites must configure — stated here for the same reason the
* Vitest budgets are: a guard that accepts any number at all would pass on
* `asyncUtilTimeout: 1` (Copilot).
*
* It is Testing Library's own default, and deliberately so: what this guard
* enforces is that the value is *stated*, not that it is large. Raising it was
* measured and rejected — see the long comment in
* `clients/web/src/test/setup.ts`.
*/
export const EXPECTED_ASYNC_UTIL_TIMEOUT = 1_000;
/**
* Directory every Vitest config in this repo lives one level under. Discovery
* (below) walks it rather than trusting a hand-written list.
@ -199,358 +170,33 @@ export function checkProject(name, config, expected = EXPECTED_PROJECTS) {
}
/**
* Strip comments so a disabled call cannot satisfy a check.
* Does this project load the setup file that asserts no test retries?
*
* 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).
*
* 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) {
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;
}
/**
* Does this source configure `asyncUtilTimeout` at the expected value, through
* the right import?
*
* Deliberately a source check and not a runtime one: the value only exists
* inside a running test environment, and a `configure` call reached by no
* project would satisfy a runtime probe of this module just as well. What
* closes that second gap is `checkSetupFilesWiring` below, which asks the
* resolved project whether it actually loads the file.
*
* @param {string} source
* @param {string} from module the `configure` must come from
* @param {number} [expected] the value it must configure
* @returns {string[]}
*/
export function checkAsyncUtilSource(
source,
from,
expected = EXPECTED_ASYNC_UTIL_TIMEOUT,
) {
const code = stripComments(source);
const failures = [];
// Bind-then-check, not two independent regexes. Checking "imports from X" and
// "calls configure()" separately passes on a split import — `cleanup` from
// `@testing-library/react` and `configure` from `@testing-library/dom` — which
// is a configuration applied to a copy no test calls, and green by inspection
// (Copilot). So find the name `configure` is bound to *in the import from the
// required module*, and require the call to use that name.
const importRe = new RegExp(
`import\\s*\\{([^}]*)\\}\\s*from\\s*["']${from.replace(/[.*+?^$()|[\]\\]/g, "\\$&").replace(/\//g, "\\/")}["']`,
);
const imported = importRe.exec(code);
if (!imported) {
failures.push(`does not import from "${from}"`);
return failures;
}
// `configure`, or `configure as somethingElse`.
const binding = imported[1]
.split(",")
.map((spec) => spec.trim())
.map((spec) => /^configure(?:\s+as\s+([A-Za-z_$][\w$]*))?$/.exec(spec))
.find(Boolean);
if (!binding) {
failures.push(`does not import configure from "${from}"`);
return failures;
}
const name = binding[1] ?? "configure";
// EVERY call, not the first. A setup file holding the expected call followed
// by a second one is the shape that matters: the later call wins at runtime,
// so inspecting only the first would approve a file whose effective timeout
// is something else entirely (Copilot). Requiring all of them to agree is
// stricter than checking the last and gives a clearer message than "the
// effective value is X" would.
// ⚠️ `(?<![.\\w$])` and not `\\b`: a word boundary succeeds straight after a
// dot, so `other.configure({ … })` matched the imported name and satisfied the
// check while the imported binding was never called at all (Copilot). Excluded
// are a member access and any identifier this name is merely the tail of.
const calls = [];
const nameRe = new RegExp(`(?<![.\\w$])${name}\\s*\\(`, "g");
let m;
while ((m = nameRe.exec(code)) !== null) {
const open = m.index + m[0].length - 1;
const close = matchBracket(code, open);
if (close === -1) break;
calls.push(code.slice(open + 1, close));
nameRe.lastIndex = close;
}
if (calls.length === 0) {
failures.push(`does not call ${name}()`);
return failures;
}
const configured = calls
.map((args) => /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(args))
.filter(Boolean)
.map((match) => match[1]);
if (configured.length === 0) {
failures.push(`calls ${name}() without an asyncUtilTimeout`);
return failures;
}
for (const value of configured) {
if (Number(value.replace(/_/g, "")) !== expected) {
failures.push(
`configures asyncUtilTimeout as ${value}, expected ${expected}`,
);
}
}
return failures;
}
/**
* Is each declared site actually loaded by the project it claims to configure?
*
* Without this the guard reads two files and reports both projects configured
* while `setupFiles` had been deleted from `vite.config.ts` and Testing Library
* had silently gone back to its 1000ms default (Copilot). Vitest resolves
* `setupFiles` to absolute paths, so compare by suffix against the declared
* repo-relative path.
* The runtime assertion in `vitest.setup.shared.mts` binds only a project that
* actually loads it, so a dropped `setupFiles` entry would silently un-enforce
* the rule for that project while everything still looked configured. This is
* the one thing about that assertion a resolved config *can* answer, so it is
* answered here rather than assumed.
*
* @param {string} name project name as this guard knows it
* @param {unknown} setupFiles the project's resolved `setupFiles`
* @param {readonly {file: string, project: string}[]} [sites]
* @returns {string[]}
*/
export function checkSetupFilesWiring(
name,
setupFiles,
sites = ASYNC_UTIL_SITES,
) {
const site = sites.find((s) => s.project === name);
if (!site) return [];
export function checkNoRetrySetupLoaded(name, setupFiles) {
const loaded = Array.isArray(setupFiles) ? setupFiles : [];
const wanted = site.file.split("/").join("/");
const found = loaded.some(
(f) => typeof f === "string" && f.split("\\").join("/").endsWith(wanted),
(f) =>
typeof f === "string" &&
f.split("\\").join("/").endsWith("vitest.setup.shared.mts"),
);
return found
? []
: [
`project "${name}" does not load ${site.file} as a setupFile, so its ` +
`asyncUtilTimeout never takes effect`,
`project "${name}" does not load vitest.setup.shared.mts, so nothing ` +
`stops a test in it from declaring a retry (#2323)`,
];
}
/**
* Skip whitespace from `i`.
*
* @param {string} code
* @param {number} i
* @returns {number}
*/
function skipWs(code, i) {
while (i < code.length && /\s/.test(code[i])) i += 1;
return i;
}
/**
* Index just past the string or template literal starting at `i`.
*
* @param {string} code
* @param {number} i index of the opening quote
* @returns {number} index after the closing quote, or `code.length` if unclosed
*/
function skipString(code, i) {
const quote = code[i];
let j = i + 1;
while (j < code.length) {
if (code[j] === "\\") {
j += 2;
continue;
}
if (code[j] === quote) return j + 1;
j += 1;
}
return code.length;
}
/**
* Index of the bracket closing the one at `i`, honoring nesting and skipping
* string literals so a bracket inside a string cannot unbalance the count.
*
* This is what a regex cannot do, and the reason this scan is structural: a
* character class like `[^()]*` cannot consume `it.each([makeCase()])` or
* `it.skipIf(() => isWindows())`, so every such suite was a blind spot
* (Copilot).
*
* @param {string} code
* @param {number} i index of the opening bracket
* @returns {number} index of the matching close, or -1
*/
function matchBracket(code, i) {
const open = code[i];
const close = open === "(" ? ")" : "}";
let depth = 0;
let j = i;
while (j < code.length) {
const c = code[j];
if (c === '"' || c === "'" || c === "`") {
j = skipString(code, j);
continue;
}
if (c === open) depth += 1;
else if (c === close) {
depth -= 1;
if (depth === 0) return j;
}
j += 1;
}
return -1;
}
/** Test-definition heads a `retry` option can be attached to. */
const TEST_HEAD = /\b(?:it|test|describe|suite|bench)(?:\.\w+)*/g;
/**
* A `retry` declared on an individual test or suite, or passed on a command
* line — the two places a project-level check cannot see.
*
* The "no retry" decision is about the behavior, not about one config key, so a
* guard that only reads `project.config.retry` leaves `it("…", { retry: 2 })`
* and `--retry=2` in an npm script wide open (Copilot). Both are scanned here.
*
* Structural rather than a single regex, for two reasons learned one round
* apart. The `retry` must sit in an **options object that follows the test
* name**, which is the only position Vitest reads it from — that is what keeps
* a `retry` field in fixture data, a variable named `retry`, or a mocked API's
* option out of it, and this repo has several. And the chain may carry an
* argument of its own — `it.each([makeCase()])`, `it.skipIf(() => isWin())`,
* `it.each\`table\`` — which a character-class regex cannot step over once it
* contains nested parentheses, silently skipping every such suite.
*
* @param {string} source
* @returns {string[]} the matched declarations, empty when there are none
*/
export function findTestLevelRetries(source) {
const code = stripComments(source);
const found = [];
TEST_HEAD.lastIndex = 0;
let head;
while ((head = TEST_HEAD.exec(code)) !== null) {
const decl = readRetryOption(code, head.index + head[0].length);
if (decl) found.push(decl);
}
return found;
}
/**
* Read a `retry` out of the options object of the test call starting at `i`.
*
* @param {string} code comment-stripped source
* @param {number} i index just past the `it`/`describe`/… chain
* @returns {string | null}
*/
function readRetryOption(code, i) {
let j = skipWs(code, i);
// `it.each\`table\`` puts a tagged template between the chain and the call.
if (code[j] === "`") j = skipWs(code, skipString(code, j));
if (code[j] !== "(") return null;
// The first `(` is either the call itself or the chain's own argument
// (`it.each([...])`, `it.skipIf(…)`). It is the call when a string literal —
// the test name — comes first; otherwise step over it, balanced, and the real
// call is the next `(`.
let open = j;
let name = skipWs(code, open + 1);
if (!/["'`]/.test(code[name] ?? "")) {
const close = matchBracket(code, open);
if (close === -1) return null;
open = skipWs(code, close + 1);
if (code[open] !== "(") return null;
name = skipWs(code, open + 1);
if (!/["'`]/.test(code[name] ?? "")) return null;
}
let k = skipWs(code, skipString(code, name));
if (code[k] !== ",") return null;
k = skipWs(code, k + 1);
if (code[k] !== "{") return null;
const end = matchBracket(code, k);
if (end === -1) return null;
const options = code.slice(k + 1, end);
// Two forms, because JavaScript has two. `retry: 2` and the shorthand
// `{ retry }`, which is the same declaration with the value bound above and
// which a colon-requiring pattern misses entirely (Copilot).
const explicit = /(?:^|[\s,{])retry\s*:\s*([^,}\s]+)/.exec(options);
if (explicit) return `retry: ${explicit[1]}`;
const shorthand = /(?:^|[\s,{])retry\s*(?:,|$)/.test(options);
return shorthand ? "retry (shorthand)" : null;
}
/**
* A `retry` flag passed to vitest from an npm script.
*
* @param {Record<string, unknown>} scripts
* @returns {string[]} `"<name>: <script>"` for each offender
*/
export function findScriptRetries(scripts) {
return Object.entries(scripts ?? {})
.filter(
([, cmd]) => typeof cmd === "string" && /(^|\s)--retry(=|\s|$)/.test(cmd),
)
.map(([name, cmd]) => `${name}: ${String(cmd)}`);
}
/**
* Every Vitest config under `clients/`, as repo-relative directories.
*
@ -679,7 +325,7 @@ async function main() {
checked += 1;
failures.push(
...checkProject(name, project.config).map((f) => `${root}: ${f}`),
...checkSetupFilesWiring(name, project.config.setupFiles).map(
...checkNoRetrySetupLoaded(name, project.config.setupFiles).map(
(f) => `${root}: ${f}`,
),
);
@ -691,58 +337,6 @@ async function main() {
}
}
// `retry`, in the two places a resolved project cannot show it. Tracked files
// only — an untracked scratch test is not something this repo ships.
const testFiles = execFileSync(
"git",
["ls-files", "*.test.ts", "*.test.tsx", "*.test.mts", "*.stories.tsx"],
{ cwd: repoRoot, encoding: "utf-8" },
)
.split("\n")
.filter(Boolean);
for (const file of testFiles) {
const found = findTestLevelRetries(
readFileSync(resolve(repoRoot, file), "utf-8"),
);
for (const decl of found) {
failures.push(
`${file} declares \`${decl}\` on a test or suite — a retry turns a ` +
`load-induced red into a silent green on the only pre-push gate here (#1596)`,
);
}
}
const manifests = execFileSync(
"git",
["ls-files", "package.json", "*/package.json", "*/*/package.json"],
{ cwd: repoRoot, encoding: "utf-8" },
)
.split("\n")
.filter(Boolean);
for (const file of manifests) {
const { scripts } = JSON.parse(
readFileSync(resolve(repoRoot, file), "utf-8"),
);
for (const offender of findScriptRetries(scripts)) {
failures.push(`${file} passes --retry from a script — ${offender}`);
}
}
for (const { file, from, project } of ASYNC_UTIL_SITES) {
const abs = resolve(repoRoot, file);
if (!existsSync(abs)) {
failures.push(
`${file} is missing — the "${project}" project has no asyncUtilTimeout`,
);
continue;
}
failures.push(
...checkAsyncUtilSource(readFileSync(abs, "utf-8"), from).map(
(f) => `${file} (${project} project) ${f}`,
),
);
}
if (failures.length > 0) {
console.error("verify:test-timeouts FAILED\n");
for (const f of failures) console.error(` - ${f}`);
@ -750,18 +344,17 @@ async function main() {
"\nEvery test-gate budget must be a value someone chose, sized for a machine\n" +
"running three or four concurrent worktree gates (#2323). The shared values live\n" +
"in `vitest.shared.mts` (TIMEOUTS / INTEGRATION_TIMEOUTS) and every project\n" +
"spreads one of them; Testing Library's own asyncUtilTimeout is configured in\n" +
"each web project's setup file. Raising a budget is a decision to state there,\n" +
"not a per-suite argument to add — and `retry` stays unset.",
"spreads one of them. Raising a budget is a decision to state there, not a\n" +
"per-suite argument to add.\n\n" +
"`retry` itself is asserted at RUNTIME by vitest.setup.shared.mts, which every\n" +
"project loads — this only checks that each project still loads it.",
);
process.exit(1);
}
console.log(
`verify:test-timeouts OK — ${checked} Vitest projects on stated budgets, ` +
`no retry in any project, test or script (${testFiles.length} test files, ` +
`${manifests.length} manifests), ` +
`${ASYNC_UTIL_SITES.length} asyncUtilTimeout sites configured.`,
`no project-level retry, and every one loading the runtime no-retry setup.`,
);
}

301
scripts/verify-test-timeouts.test.mjs
View file

@ -7,6 +7,12 @@
* the two failure modes a green repo can never produce on its own: an unknown
* project, and a `retry` someone added.
*
* ⚠️ This suite used to be twice this size, pinning the edge cases of a source
* scanner that no longer exists — a commented-out call, a nested paren in a
* chain argument, an aliased import. Those rules are asserted at runtime now
* (`vitest.setup.shared.mts`, `clients/web/src/test/asyncUtilTimeout.test.ts`),
* where there is no spelling to anticipate and so nothing to pin.
*
* ⚠️ Keep the filename exactly `verify-test-timeouts.test.mjs`. `node --test`
* silently SKIPS a file its glob misses and still exits 0, so a typo here reads
* as a passing suite.
@ -16,21 +22,15 @@ import test from "node:test";
import assert from "node:assert/strict";
import {
ASYNC_UTIL_SITES,
CONFIG_ROOTS,
EXPECTED_ASYNC_UTIL_TIMEOUT,
EXPECTED_INTEGRATION_TIMEOUTS,
EXPECTED_PROJECTS,
EXPECTED_TIMEOUTS,
checkAsyncUtilSource,
checkConfigRootCoverage,
checkNoRetrySetupLoaded,
checkProject,
checkSetupFilesWiring,
discoverConfigRoots,
findScriptRetries,
findTestLevelRetries,
identifyProject,
stripComments,
VITEST_CONFIG_FILENAMES,
} from "./verify-test-timeouts.mjs";
@ -117,175 +117,6 @@ test("an unrecognized project name is not silently matched", () => {
assert.equal(identifyProject({ name: "" }, ["unit", "storybook"]), undefined);
});
test("a setup file that configures asyncUtilTimeout passes", () => {
const source = `import { cleanup, configure } from "@testing-library/react";
configure({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(source, "@testing-library/react"), []);
});
test("configure() without the timeout is caught", () => {
const source = `import { configure } from "storybook/test";
configure({ testIdAttribute: "data-testid" });`;
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
"calls configure() without an asyncUtilTimeout",
]);
});
test("a split import does not satisfy the check", () => {
// The hole this closes: checking "imports from X" and "calls configure()"
// independently passes when `configure` comes from a *different* Testing
// Library copy than the one the tests use, which is a configuration nothing
// reads — and green by inspection (Copilot).
const source = `import { cleanup } from "@testing-library/react";
import { configure } from "@testing-library/dom";
configure({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(source, "@testing-library/react"), [
'does not import configure from "@testing-library/react"',
]);
});
test("configure imported under an alias is followed to its call", () => {
const source = `import { configure as cfg } from "storybook/test";
cfg({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []);
const unused = `import { configure as cfg } from "storybook/test";
configure({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(unused, "storybook/test"), [
"does not call cfg()",
]);
});
test("the timeout is read from the configure call, not from anywhere in the file", () => {
// A matching number elsewhere in the module must not stand in for the
// argument actually passed.
const source = `import { configure } from "storybook/test";
const asyncUtilTimeout = 1000;
configure({ testIdAttribute: "data-testid" });`;
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
"calls configure() without an asyncUtilTimeout",
]);
});
test("the import is checked, not just the call", () => {
// Storybook instruments its own Testing Library copy, so configuring
// `@testing-library/*` from a story setup configures a copy no play function
// ever calls — green by inspection, dead in practice.
const source = `import { configure } from "@testing-library/dom";
configure({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
'does not import from "storybook/test"',
]);
});
test("a setup file with no configure() at all is caught", () => {
// Reported as the missing import and nothing else: once the module is not
// imported there is no binding to look for a call to, so listing a second
// failure would be guesswork about which of the two the author meant.
assert.deepEqual(
checkAsyncUtilSource("export const nothing = 1;\n", "storybook/test"),
['does not import from "storybook/test"'],
);
});
test("both web projects have an asyncUtilTimeout site", () => {
assert.deepEqual(ASYNC_UTIL_SITES.map((s) => s.project).sort(), [
"storybook",
"unit",
]);
});
test("a commented-out configure() does not satisfy the check", () => {
// The most likely way this stops being configured is someone disabling it in
// place, and a raw-source scan would read that as configured (Copilot).
const lineComment = `import { configure } from "storybook/test";
// configure({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(lineComment, "storybook/test"), [
"does not call configure()",
]);
const blockComment = `import { configure } from "storybook/test";
/* configure({ asyncUtilTimeout: 1000 }); */`;
assert.deepEqual(checkAsyncUtilSource(blockComment, "storybook/test"), [
"does not call configure()",
]);
});
test("the configured value must be the expected one, not merely present", () => {
const source = `import { configure } from "storybook/test";
configure({ asyncUtilTimeout: 9000 });`;
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
`configures asyncUtilTimeout as 9000, expected ${EXPECTED_ASYNC_UTIL_TIMEOUT}`,
]);
});
test("a numeric separator in the configured value is read, not rejected", () => {
const source = `import { configure } from "storybook/test";
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
configure({ asyncUtilTimeout: 1000 });`;
const code = stripComments(source);
assert.match(code, /configure\(\{ asyncUtilTimeout: 1000 \}\)/);
assert.doesNotMatch(code, /header/);
});
test("a project that does not load its declared setup file is caught", () => {
// Reading the file proves it configures the timeout; only this proves the
// project ever loads it. Deleting the setupFiles entry used to leave the
// guard reporting both projects configured (Copilot).
const failures = checkSetupFilesWiring("unit", []);
assert.equal(failures.length, 1);
assert.match(failures[0], /does not load .*setup\.ts as a setupFile/);
});
test("a setup file loaded under an absolute path is recognized", () => {
assert.deepEqual(
checkSetupFilesWiring("unit", [
"/repo/clients/web/src/test/setup.ts",
"/repo/other.ts",
]),
[],
);
});
test("a project with no declared site is not required to load one", () => {
assert.deepEqual(checkSetupFilesWiring("cli", []), []);
assert.deepEqual(checkSetupFilesWiring("cli", undefined), []);
});
test("a Vitest config this guard does not check is an error", () => {
const failures = checkConfigRootCoverage([
"clients/web",
@ -315,67 +146,6 @@ test("discovery finds exactly the configs this guard is set up to check", () =>
assert.deepEqual(checkConfigRootCoverage(discoverConfigRoots()), []);
});
test("a retry on an individual test or suite is caught", () => {
// The project-level check cannot see these, and the decision is about the
// behavior rather than about one config key (Copilot).
for (const src of [
`it("x", { retry: 2 }, async () => {});`,
`test("x", { retry: 1 }, () => {});`,
`describe("x", { retry: 3 }, () => {});`,
`it.each([1])("x %i", { retry: 2 }, () => {});`,
'describe.each([[1, 2]])("x", { retry: 1 }, () => {});',
// Nested parentheses in the chain's own argument. A character class cannot
// step over either of these, so both were blind spots (Copilot).
'it.each([makeCase()])("x", { retry: 2 }, () => {});',
'it.skipIf(() => isWindows())("x", { retry: 2 }, () => {});',
'it.each`a|b`("x", { retry: 3 }, () => {});',
`it('single quoted', { timeout: 100, retry: 2 }, () => {});`,
]) {
assert.equal(findTestLevelRetries(src).length, 1, src);
}
});
test("a retry that is not a test option is not a false positive", () => {
// All four shapes exist in this repo. A bare /\bretry\b/ scan would fail on
// every one of them, and a guard that cries wolf gets disabled.
for (const src of [
`const fixture = { retry: 2 };`,
`expect(client.retry).toBe(2);`,
`it("x", async () => { await withRetries({ retry: 2 }); });`,
`vi.mock("x", () => ({ retry: 2 }));`,
`it("has retry: 2 in the name", () => {});`,
`const o = { noRetry: 2 };`,
// Shorthand for a DIFFERENT property must not match the shorthand form.
`it("x", { retryCount }, () => {});`,
]) {
assert.deepEqual(findTestLevelRetries(src), [], src);
}
});
test("a retry hidden in a comment is not reported", () => {
assert.deepEqual(
findTestLevelRetries(`// it("x", { retry: 2 }, () => {});`),
[],
);
});
test("a --retry flag in an npm script is caught", () => {
assert.deepEqual(findScriptRetries({ test: "vitest run --retry=2" }), [
"test: vitest run --retry=2",
]);
assert.equal(findScriptRetries({ test: "vitest run --retry 2" }).length, 1);
});
test("a script that merely contains the word retry is not a false positive", () => {
assert.deepEqual(
findScriptRetries({ test: "vitest run retry-helper.ts" }),
[],
);
assert.deepEqual(findScriptRetries({ test: "vitest run --no-retry-x" }), []);
assert.deepEqual(findScriptRetries({}), []);
assert.deepEqual(findScriptRetries(undefined), []);
});
test("discovery knows every filename Vitest loads a config from", () => {
// Deny-by-default only holds if discovery sees every config Vitest would; a
// `vitest.config.mts` this list did not name would be invisible (Copilot).
@ -396,51 +166,20 @@ test("discovery knows every filename Vitest loads a config from", () => {
);
});
test("every configure() call is inspected, not just the first", () => {
// The later call wins at runtime, so approving a file on its first call would
// approve one whose effective timeout is something else entirely (Copilot).
const overridden = `import { configure } from "storybook/test";
configure({ asyncUtilTimeout: 1000 });
configure({ asyncUtilTimeout: 5000 });`;
assert.deepEqual(checkAsyncUtilSource(overridden, "storybook/test"), [
"configures asyncUtilTimeout as 5000, expected 1000",
]);
});
test("a project that does not load the no-retry setup is caught", () => {
// The runtime assertion binds only a project that loads it, so a dropped
// setupFiles entry would un-enforce the rule while everything still looked
// configured. This is the one thing about that assertion a resolved config
// can answer.
const failures = checkNoRetrySetupLoaded("unit", []);
assert.equal(failures.length, 1);
assert.match(failures[0], /does not load vitest\.setup\.shared\.mts/);
test("a configure() call that sets something else does not hide the real one", () => {
const source = `import { configure } from "storybook/test";
configure({ testIdAttribute: "data-testid" });
configure({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []);
});
test("the shorthand property form is caught", () => {
// `{ retry }` is the same declaration as `{ retry: 2 }` with the value bound
// above, and a colon-requiring pattern misses it entirely (Copilot).
assert.deepEqual(
findTestLevelRetries('const retry = 2; it("x", { retry }, () => {});'),
["retry (shorthand)"],
);
assert.deepEqual(
findTestLevelRetries('it("x", { timeout: 100, retry }, () => {});'),
["retry (shorthand)"],
checkNoRetrySetupLoaded("unit", [
"/repo/clients/web/src/test/setup.ts",
"/repo/vitest.setup.shared.mts",
]),
[],
);
});
test("a member call is not mistaken for the imported binding", () => {
// `\\b` succeeds straight after a dot, so `other.configure(…)` satisfied the
// check while the imported binding was never called at all (Copilot).
const source = `import { configure } from "storybook/test";
other.configure({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
"does not call configure()",
]);
});
test("a longer identifier ending in the binding name is not a call to it", () => {
const source = `import { configure } from "storybook/test";
reconfigure({ asyncUtilTimeout: 1000 });`;
assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [
"does not call configure()",
]);
});

3
tsconfig.lint.json
View file

@ -30,6 +30,7 @@
"test-servers/src/**/*.tsx",
"test-servers/src/**/*.mts",
"test-servers/src/**/*.cts",
"vitest.shared.mts"
"vitest.shared.mts",
"vitest.setup.shared.mts"
]
}

41
vitest.setup.shared.mts Normal file
View file

@ -0,0 +1,41 @@
/**
* The one assertion every Vitest project in this repo runs before each test.
*
* **`retry` must stay unset** (#2323). A retry converts a load-induced red into
* a silent green on the only pre-push gate this repo has, and would re-open
* #1596 by hiding a real race behind a second attempt.
*
* ⚠️ This is a *runtime* check, and that is the whole point. An earlier
* revision of #2334 enforced the same rule by scanning source text for
* `it("…", { retry: 2 })`, and the review found a new valid JavaScript spelling
* it missed in five consecutive rounds — a trailing comment, nested parens in
* `it.each([makeCase()])`, a member-access prefix, the shorthand `{ retry }`, an
* aliased import, a non-literal title. Each fix was correct and each made the
* scanner more parser-shaped, until it had eight helpers doing quote tracking
* and bracket balancing. That is a parser, badly, in a linter.
*
* `ctx.task.retry` is the value Vitest actually resolved for this test, so it
* needs no spelling to be anticipated and cannot be outrun by a new one. It also
* covers strictly more than the scan ever did: a per-test option, a `describe`
* option inherited by its children, a project-level `retry`, and a `--retry`
* flag on the command line all arrive here as the same number.
*
* The general lesson, worth keeping: when a rule is about what the code *does*,
* ask the runtime; reading the source is a guess that has to be right about
* every way the language can express the same thing.
*/
import { beforeEach } from "vitest";
beforeEach((ctx) => {
const { retry } = ctx.task;
if (typeof retry === "number" && retry > 0) {
throw new Error(
`"${ctx.task.name}" declares retry: ${retry}. Retries are not used in ` +
`this repo (#2323): a retry turns a load-induced red into a silent ` +
`green on the only pre-push gate there is, which is how a real race ` +
`ships (#1596). Fix the test — fake timers, an awaited condition — or ` +
`raise the shared budget in vitest.shared.mts if it is genuinely slow.`,
);
}
});

10
vitest.shared.mts
View file

@ -37,6 +37,16 @@ import { fileURLToPath } from "node:url";
* converts a load-induced red into a silent green on the only pre-push gate
* this repo has. `scripts/verify-test-timeouts.mjs` enforces both halves.
*/
/**
* Absolute path to the setup file every project loads, which asserts at runtime
* that no test declares a `retry` (#2323). Exported from here so the six
* projects name one path rather than six copies of a relative one.
*/
export const NO_RETRY_SETUP = path.resolve(
path.dirname(fileURLToPath(import.meta.url)),
"vitest.setup.shared.mts",
);
export const TIMEOUTS = Object.freeze({
/**
* 3x Vitest's default. Covers the measured load; past this a genuinely hung