diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 66db5b7c..1a70f21a 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -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 diff --git a/AGENTS.md b/AGENTS.md index 19aba0ac..4a0b5238 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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 diff --git a/clients/cli/vitest.config.ts b/clients/cli/vitest.config.ts index 92a0852c..49217c73 100644 --- a/clients/cli/vitest.config.ts +++ b/clients/cli/vitest.config.ts @@ -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, diff --git a/clients/launcher/vitest.config.ts b/clients/launcher/vitest.config.ts index f0e0b07f..4c4caacf 100644 --- a/clients/launcher/vitest.config.ts +++ b/clients/launcher/vitest.config.ts @@ -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 diff --git a/clients/tui/vitest.config.ts b/clients/tui/vitest.config.ts index 84433522..1dfbc79c 100644 --- a/clients/tui/vitest.config.ts +++ b/clients/tui/vitest.config.ts @@ -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. diff --git a/clients/web/src/test/asyncUtilTimeout.test.ts b/clients/web/src/test/asyncUtilTimeout.test.ts new file mode 100644 index 00000000..c7817328 --- /dev/null +++ b/clients/web/src/test/asyncUtilTimeout.test.ts @@ -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); + }); +}); diff --git a/clients/web/src/test/storybookSetup.ts b/clients/web/src/test/storybookSetup.ts deleted file mode 100644 index f5f31cf2..00000000 --- a/clients/web/src/test/storybookSetup.ts +++ /dev/null @@ -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 }); diff --git a/clients/web/tsconfig.test.json b/clients/web/tsconfig.test.json index e12c27bd..713badf3 100644 --- a/clients/web/tsconfig.test.json +++ b/clients/web/tsconfig.test.json @@ -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": [] } diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 1070cfcf..d6fea376 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -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, diff --git a/docs/quality-gate.md b/docs/quality-gate.md index bec8f39a..ff073b8b 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -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). | diff --git a/eslint.config.js b/eslint.config.js index df50a329..9990e974 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -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, }, diff --git a/package.json b/package.json index 9c52e81e..e9d07c22 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/scripts/verify-test-timeouts.mjs b/scripts/verify-test-timeouts.mjs index f00b83ed..59f0394a 100644 --- a/scripts/verify-test-timeouts.mjs +++ b/scripts/verify-test-timeouts.mjs @@ -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. - // ⚠️ `(? /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} scripts - * @returns {string[]} `":