diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 1a70f21a..1d3d9443 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -30,10 +30,17 @@ review, which is the argument for not making a fourth. `npm run local:gate` prints each stage as it starts, so the running command is the other reliable answer. -It is a **strict superset** of GitHub CI (which additionally runs `npm install`, -and runs `coverage` as a parallel job). So the direction that matters holds: -**passing `local:gate` locally means CI's gates will pass.** The reverse does -not. +It runs **every check** GitHub CI runs (which additionally runs `npm install`, +and runs `coverage` as a parallel job), plus two local-only steps. So the +direction that matters holds: **passing `local:gate` locally means CI's gates +will pass.** The reverse does not. + +One difference in *invocations*, not checks: CI runs each client's unit suite +twice — bare inside `validate`, instrumented inside `coverage` — on two +parallel runners, while the gate runs it **once**, instrumented (#2341). The +gate's first stage is `local:validate`, which is `validate` minus each client's +`test` leg; `npm run validate` itself is unchanged. The reasoning is in +[`AGENTS.md`](../../../AGENTS.md#mandatory-pre-push-gate). ⚠️ **There is no `npm run ci`.** The gate was renamed to `local:gate` (#2146) precisely because `npm ci` is a built-in that clean-installs from the lockfile diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 96efe0c1..9c73b861 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -49,7 +49,9 @@ jobs: # (#2159), which consumes nothing this job produces — every client's # `test:coverage` builds whatever it needs itself. Unit tests run in # both jobs (fast here, instrumented there); now that the two run in - # parallel that duplication costs no wall clock at all. + # parallel that duplication costs no wall clock at all. (The local gate, + # which runs serially, calls `local:validate` instead and runs them once + # — #2341. Keep `validate` here: this job IS the fast inner loop.) # A future optimization could split `coverage` into per-client parallel # jobs, but that's a larger restructure and deliberately out of scope. run: npm run validate @@ -109,9 +111,10 @@ jobs: - name: Run cross-client smokes # NOTE: this workflow runs GitHub CI's tier only. The LOCAL pre-push - # gate is `npm run local:gate`, and it is a strict superset — do not add - # it, a `local:*` script, `smoke:web:firefox`/`smoke:web:webkit`, - # `smoke:web:engine`, or a non-Chromium `SMOKE_BROWSER` to any workflow. + # gate is `npm run local:gate`, and it runs every check here plus the + # local-only ones — do not add it, a `local:*` script, + # `smoke:web:firefox`/`smoke:web:webkit`, `smoke:web:engine`, or a + # non-Chromium `SMOKE_BROWSER` to any workflow. # `npm run test:scripts` fails if you do (scripts/lib/workflow-gate.mjs, # #2146). `npm run smoke` and `smoke:web:chromium` belong here and are # not affected. The canonical CI-vs-local table is in the root README. diff --git a/AGENTS.md b/AGENTS.md index 4a0b5238..81c1d6cf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -473,7 +473,8 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race. ## Mandatory pre-push gate - **ALWAYS run `npm run format` before committing.** The **root** `format` auto-fixes `core/`, the root `scripts/` tooling, the root shared surface, and every client's scope in one shot. `validate` runs the non-fixing `format:check` and will fail in CI on any unformatted file, so run the auto-fixer first rather than letting `format:check` catch it. -- **`npm run local:gate` is the mandatory pre-push command.** It is a **strict superset** of `.github/workflows/main.yml`, so passing it locally means CI's gates will pass. Expect several minutes. +- **`npm run local:gate` is the mandatory pre-push command.** It runs **every check** `.github/workflows/main.yml` runs, plus two local-only steps, so passing it locally means CI's gates will pass. Expect several minutes. +- **The gate runs each client's test suite once, instrumented; CI runs it twice (#2341).** CI's `build` job runs the bare `test` inside `validate` and its parallel `coverage` job runs `test:coverage`, which costs CI no wall clock. Serially in one process the bare pass was ~80s of a ~370s gate, re-running exactly the files the coverage pass runs a few minutes later — so the gate calls **`local:validate`**, which is `validate` minus each client's `test` leg (every client's `validate` is `check && test`, and `local:validate` runs the `check` half). "Every check" is therefore a claim about checks, not invocations, and it holds because `@vitest/coverage-v8` collects coverage from V8's own profiler (`Profiler.takePreciseCoverage`) and rewrites no source: a test file sees identical code either way, and the instrumented run is only *slower*, which makes it the stricter of the two for the failure class this repo actually sees (a correct test cut off under load). A test that passed only *because* it ran slower would be a race — #1596's class, a defect wherever it surfaces — and CI's bare pass still runs it. **`npm run validate` is unchanged**, because CI runs it directly and it is the inner-loop check; `verify:*` guards that ask "is this reachable from `validate`" are unaffected. `local:validate` lives in the `local:` namespace so the workflow guard keeps it out of CI by construction. - **`npm run validate` is the fast inner-loop check and is NOT an acceptable substitute.** It runs `test`, not `test:coverage`, so it does **zero** coverage gating, no smokes, and no Storybook tests. Skipping the gate is how a push passes every fast local check and still fails CI. - There is deliberately **no `npm run ci`** — that name collided with the `npm ci` built-in, which clean-installs from the lockfile and does not run this script. - What each stage covers, and why two of them are local-only, is [`docs/quality-gate.md`](./docs/quality-gate.md); how to diagnose a failing stage is the `pre-push-gate` skill. diff --git a/README.md b/README.md index 6490c07c..1b813e38 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ Each client self-validates from its own folder; the root scripts chain them. The ```bash npm run validate # fast inner loop: format:check + lint + typecheck + build + unit tests npm run coverage # the per-file ≥90% gate (lines/statements/functions/branches) -npm run local:gate # MANDATORY before pushing — a strict superset of GitHub CI +npm run local:gate # MANDATORY before pushing — every GitHub CI check, plus two local-only ones ``` `npm run local:gate` chains every check below, plus the smokes and the Storybook tests. [Testing and the quality gate](./docs/quality-gate.md) owns the stage list and says what each one covers and why two are local-only; [`AGENTS.md`](./AGENTS.md) holds the testing rules themselves. diff --git a/clients/cli/README.md b/clients/cli/README.md index 4e2336fe..2e1edcbe 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -472,16 +472,19 @@ While the Web Client provides a rich visual interface, the CLI is designed for: Like the other clients, the CLI self-validates from its own folder: ```bash -npm run validate # format:check && lint && typecheck && test (fast; no coverage gate) +npm run check # format:check && lint && typecheck (no tests) +npm run validate # check && test (fast; no coverage gate) npm test # build test-servers + binary, then run all tests npm run test:coverage # build + tests under the per-file ≥90 coverage gate ``` The CLI's `test` / `test:coverage` **build the binary first** (out-of-process -`e2e.test.ts` spawns it). `validate` is `format:check && lint && typecheck && test` -with no separate `build` step (`pretest` builds). Repo-root `validate:cli` -delegates here; the coverage gate is `npm run coverage` / `coverage:cli` (also in -`npm run local:gate`), matching AGENTS.md. +`e2e.test.ts` spawns it). `check` is `format:check && lint && typecheck` and +`validate` is `check && test`, with no separate `build` step (`pretest` builds). +Repo-root `validate:cli` delegates to `validate`, and the root `local:validate` +— the first stage of `npm run local:gate` — runs `check` instead, so the gate +runs the suite once, under `coverage` / `coverage:cli` (#2341), matching +AGENTS.md. Tests run the CLI **in-process** (importing `runCli()`) so `src/` is measured under coverage, with a thin out-of-process spawn layer for the real binary. See diff --git a/clients/cli/package.json b/clients/cli/package.json index 11ef4342..5bcf04cd 100644 --- a/clients/cli/package.json +++ b/clients/cli/package.json @@ -17,7 +17,8 @@ "scripts": { "build": "tsup", "typecheck": "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.test.json", - "validate": "npm run format:check && npm run lint && npm run typecheck && npm run test", + "check": "npm run format:check && npm run lint && npm run typecheck", + "validate": "npm run check && npm run test", "test": "vitest run", "test:watch": "vitest", "test:coverage": "npm run test-servers:build && npm run build && vitest run --coverage", diff --git a/clients/launcher/README.md b/clients/launcher/README.md index 056acbc8..e69cf0cb 100644 --- a/clients/launcher/README.md +++ b/clients/launcher/README.md @@ -110,7 +110,8 @@ stale fixture reports a product failure rather than a staleness one. Like the web client, the launcher self-validates from its own folder: ```bash -npm run validate # format:check && lint && build && test:coverage +npm run check # format:check && lint && typecheck && build (no tests) +npm run validate # check && test ``` This has **no** dependency on the other clients being built — it only checks the @@ -118,7 +119,9 @@ launcher's own source. `eslint.config.js` is a Node-only flat config (the web client's React/Storybook plugins stripped out), and the per-file coverage gate covers `parse-launcher-argv.ts` (the pure arg-parsing logic); `src/index.ts` is excluded as binary bootstrap and is instead exercised by the smokes above. The -repo-root `validate:launcher` simply delegates here (`cd clients/launcher && npm run validate`). +repo-root `validate:launcher` simply delegates here (`cd clients/launcher && npm run validate`), +and the root `local:validate` — the first stage of `npm run local:gate` — runs +`check` instead, so the gate runs the suite once, under `coverage:launcher` (#2341). ## Publishing diff --git a/clients/launcher/package.json b/clients/launcher/package.json index ceb07a06..4ea7560f 100644 --- a/clients/launcher/package.json +++ b/clients/launcher/package.json @@ -20,6 +20,7 @@ "format:check": "prettier --check src __tests__ scripts \"*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}\"", "test": "vitest run", "test:coverage": "vitest run --coverage", - "validate": "npm run format:check && npm run lint && npm run typecheck && npm run build && npm run test" + "check": "npm run format:check && npm run lint && npm run typecheck && npm run build", + "validate": "npm run check && npm run test" } } diff --git a/clients/tui/README.md b/clients/tui/README.md index 91be95e2..17fdb0c9 100644 --- a/clients/tui/README.md +++ b/clients/tui/README.md @@ -93,12 +93,15 @@ The TUI provides terminal-native tabs and panes for interacting with your MCP se Like the other clients, the TUI self-validates from its own folder: ```bash -npm run validate # format:check && lint && build && test:coverage +npm run check # format:check && lint && typecheck && build (no tests) +npm run validate # check && test npm test # run all tests npm run test:coverage # run tests under the per-file coverage gate ``` -The repo-root `validate:tui` just delegates here. `eslint.config.js` registers +The repo-root `validate:tui` just delegates here; the root `local:validate` +(the first stage of `npm run local:gate`) runs `check` instead, so the gate +runs the suite once, under `coverage:tui` (#2341). `eslint.config.js` registers `react-hooks` for the classic rules only (rules-of-hooks + exhaustive-deps); the stricter react-hooks@7 rules are not enforced on the interim component surface (#1501). diff --git a/clients/tui/package.json b/clients/tui/package.json index 60fa4a95..42f8571e 100644 --- a/clients/tui/package.json +++ b/clients/tui/package.json @@ -19,7 +19,8 @@ "dev": "vite-node --config vitest.config.ts dev.ts", "build": "tsup", "typecheck": "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.test.json", - "validate": "npm run format:check && npm run lint && npm run typecheck && npm run build && npm run test", + "check": "npm run format:check && npm run lint && npm run typecheck && npm run build", + "validate": "npm run check && npm run test", "test": "vitest run", "test:coverage": "vitest run --coverage", "lint": "eslint . --max-warnings 0", diff --git a/clients/web/package.json b/clients/web/package.json index aa4df909..421c8b99 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -15,7 +15,8 @@ "build:client": "vite build", "build:runner": "tsup --config tsup.runner.config.ts", "build:storybook": "storybook build", - "validate": "npm run format:check && npm run lint && npm run build && npm run test", + "check": "npm run format:check && npm run lint && npm run build", + "validate": "npm run check && npm run test", "pretest": "npm run test-servers:build", "test": "vitest run --project=unit", "test:watch": "vitest --project=unit", diff --git a/docs/quality-gate.md b/docs/quality-gate.md index ff073b8b..b06cadc2 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -12,15 +12,17 @@ Each client self-validates from its own folder; the root scripts chain them. The | Tier | How it runs | What it covers | | --- | --- | --- | | **GitHub CI** (`.github/workflows/main.yml`) | Automatically, on every push | `npm install`, then `validate`, `verify:skills:cli`, `verify:build-gate`, `verify:bundle-externals`, `smoke` (which includes `smoke:web:chromium`), `test:storybook` — plus `coverage` in a parallel job ([#2159](https://github.com/modelcontextprotocol/inspector/issues/2159)) | -| **The local gate** (`npm run local:gate`) | By hand, before you push | Every check above (the install is yours to run), **plus** the Firefox engine pass (`smoke:web:firefox`), and `smoke:tui` for real rather than self-skipped | +| **The local gate** (`npm run local:gate`) | By hand, before you push | Every check above (the install is yours to run; `local:validate` stands in for `validate`, see below), **plus** the Firefox engine pass (`smoke:web:firefox`), and `smoke:tui` for real rather than self-skipped | -The local gate is a **strict superset**, not a mirror. Two of its steps have no GitHub CI counterpart, each for its own reason: +The local gate runs **every check** CI runs, and is not a mirror. Two of its steps have no GitHub CI counterpart, each for its own reason: | Local-only step | Why it is local-only | | --- | --- | | `smoke:web:firefox` | Trialled as a CI job and removed ([#2086](https://github.com/modelcontextprotocol/inspector/issues/2086)): across a dozen runs it never once disagreed with Chromium, and `playwright install --with-deps` carries a real flake surface. Kept in front of a human about to push instead. `smoke:web:webkit` is worse still — it fails two of the three smokes for reasons nobody has identified — so it is in neither tier. See [Supported browsers](#supported-browsers). | | `smoke:tui` | The Ink TUI needs a real TTY. It _is_ invoked in CI via `npm run smoke` and self-skips there on `process.env.CI`, so it needs no guarding — it handles itself. | +And one CI *invocation* the gate deliberately does not repeat: CI runs each client's unit suite **twice** — bare inside `validate` in the `build` job, instrumented inside `coverage` in the parallel job — at no wall-clock cost, because the two jobs run on separate runners. Serially in one process that second pass was ~80s of a ~370s gate, re-running exactly the test files the coverage pass runs a few minutes later ([#2341](https://github.com/modelcontextprotocol/inspector/issues/2341)). So the gate's first stage is **`local:validate`**: the same guards and `validate:core`, then each client's `check` (`format:check` + `lint` + `typecheck` + `build`) instead of its `validate` (`check` + `test`). The suites still run once, under `coverage`. That subsumes the bare pass because `@vitest/coverage-v8` reads V8's own precise-coverage profiler and rewrites nothing — the code under test is byte-identical, and the instrumented run is only slower, which is the stricter direction for the failures this repo sees. `npm run validate` and both CI jobs are unchanged. + So the direction that matters holds: **passing `npm run local:gate` means CI's gates will pass.** The reverse does not. **The gate is deliberately not named `ci`** ([#2146](https://github.com/modelcontextprotocol/inspector/issues/2146)). That name collided with `npm ci` — a built-in that clean-installs from the lockfile, does **not** run this script, and is one keystroke away — and it invited the cross-engine passes into GitHub CI by looking like the thing a workflow ought to call. There is **no back-compat alias**: `npm run ci` now fails with npm's missing-script error, which prints the available scripts. @@ -41,7 +43,8 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `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: #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 local:gate` | **Mandatory pre-push command.** `local:validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. Every GitHub CI check plus two local-only ones — 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 local:validate` | The gate's first stage: `validate` with each client's `test` leg removed (#2341) — the same `validate:guards` and `validate:core`, then every client's `check` (`format:check` + `lint` + `typecheck` + `build`; each client's `validate` is `check && test`). The suites run once, under `coverage`, instead of bare here and instrumented there. In the `local:` namespace so the workflow guard keeps it out of CI, where the bare pass is free. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package, `format:scripts` for the root `scripts/` tooling, and `format:shared` / `lint:shared` for the root "shared" surface (`test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`). Run `npm run format` before committing — the root `format` fixes `core/`, the root `scripts/`, the shared surface, and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file. diff --git a/package.json b/package.json index e9d07c22..f7042afe 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,8 @@ "build:tui": "cd clients/tui && npm run build", "build:web": "cd clients/web && npm run build", "build:launcher": "cd clients/launcher && npm run build", - "local:gate": "npm run validate && npm run verify:skills:cli && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run local:storybook", + "local:gate": "npm run local:validate && npm run verify:skills:cli && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run local:storybook", + "local:validate": "npm run validate:guards && npm run validate:core && cd clients/web && npm run check && cd ../cli && npm run check && cd ../tui && npm run check && cd ../launcher && npm run check", "local:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", "verify:build-gate": "node scripts/verify-build-gate.mjs", "verify:bundle-externals": "node scripts/verify-bundle-externals.mjs", @@ -46,7 +47,8 @@ "verify:skills": "node scripts/verify-skills.mjs", "verify:skills:cli": "node scripts/verify-skills-cli.mjs", "test:scripts": "node --test \"scripts/**/*.test.mjs\"", - "validate": "npm run verify:format-coverage && npm run verify:skills && npm run verify:typecheck-coverage && npm run verify:dep-lockstep && npm run verify:test-timeouts && npm run test:scripts && npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", + "validate": "npm run validate:guards && npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", + "validate:guards": "npm run verify:format-coverage && npm run verify:skills && npm run verify:typecheck-coverage && npm run verify:dep-lockstep && npm run verify:test-timeouts && npm run test:scripts", "verify:format-coverage": "node scripts/verify-format-coverage.mjs", "verify:dep-lockstep": "node scripts/verify-dep-lockstep.mjs", "verify:test-timeouts": "node scripts/verify-test-timeouts.mjs", diff --git a/scripts/lib/workflow-gate.mjs b/scripts/lib/workflow-gate.mjs index 1f3e175c..9a531b11 100644 --- a/scripts/lib/workflow-gate.mjs +++ b/scripts/lib/workflow-gate.mjs @@ -7,9 +7,11 @@ * two verify gates, `smoke` and the Storybook tests. `npm run smoke` covers * `smoke:launcher`, `smoke:cli`, `smoke:tui`, `smoke:web` and * `smoke:web:chromium` — all of which BELONG there. - * - **`npm run local:gate`**, the pre-push gate, is a strict superset: it adds - * the **Firefox** engine pass, and `smoke:tui` really runs there rather than - * self-skipping. WebKit is on demand and belongs to neither tier. + * - **`npm run local:gate`**, the pre-push gate, runs every check CI runs and + * adds the **Firefox** engine pass, and `smoke:tui` really runs there rather + * than self-skipping. WebKit is on demand and belongs to neither tier. It + * runs each client's unit suite once (instrumented, under `coverage`) via + * `local:validate`, where CI's two parallel jobs run it twice (#2341). * * The narrow thing that must never reach CI is a **non-Chromium engine pass**. * `smoke:web:firefox` was placed in the local gate on purpose (#2086, diff --git a/scripts/lib/workflow-gate.test.mjs b/scripts/lib/workflow-gate.test.mjs index 483f3089..cfcf84a3 100644 --- a/scripts/lib/workflow-gate.test.mjs +++ b/scripts/lib/workflow-gate.test.mjs @@ -23,6 +23,7 @@ import { findWorkflowViolations, formatWorkflowViolations, } from "./workflow-gate.mjs"; +import { reachableScripts } from "./npm-scripts.mjs"; const repoRoot = join(import.meta.dirname, "..", ".."); const workflowDir = join(repoRoot, ".github", "workflows"); @@ -609,6 +610,106 @@ describe("the gate's name", () => { assert.match(scripts["local:gate"], /local:storybook/); }); + describe("runs each client's test suite once, not twice (#2341)", () => { + // CI runs each client's unit suite bare (inside `validate`) and again + // instrumented (inside `coverage`) on two PARALLEL runners, so the second + // pass is free there. The gate runs serially, where the bare pass was ~80s + // of a ~370s run re-executing exactly the files `coverage` runs a few + // minutes later. So the gate's first stage is `local:validate` — `validate` + // minus every client's `test` leg — and these pin the three properties + // that keep it honest: the gate no longer reaches a client's bare `test`, + // it still reaches every non-test check `validate` reaches, and `validate` + // itself (CI's inner loop) is untouched. + const clients = ["web", "cli", "tui", "launcher"]; + const clientScripts = Object.fromEntries( + clients.map((c) => [ + c, + JSON.parse( + readFileSync(join(repoRoot, "clients", c, "package.json"), "utf8"), + ).scripts, + ]), + ); + + it("starts from `local:validate`, and never from `validate`", () => { + assert.match(scripts["local:gate"], /^npm run local:validate && /); + assert.doesNotMatch(scripts["local:gate"], /\brun validate(?=$|[\s&;])/); + // `local:validate` must not reach a client's `validate` either — that is + // the one door through which the bare `test` leg would come back. + const reached = reachableScripts(scripts, "local:validate"); + for (const c of clients) + assert.ok( + !reached.has(`validate:${c}`), + `local:validate must not reach validate:${c}`, + ); + assert.match( + scripts["local:validate"], + /\brun check(?=$|[\s&;])/, + "local:validate must run each client's `check`", + ); + for (const c of clients) + assert.match( + scripts["local:validate"], + new RegExp(`(?:cd (?:\\.\\./|clients/)${c}|--prefix clients/${c})`), + `local:validate must visit clients/${c}`, + ); + }); + + it("shares the guards and `validate:core` with `validate`", () => { + // One list of durable guards, reached by both entry points — so a guard + // added to `validate:guards` lands in CI and in the gate together. + const gate = reachableScripts(scripts, "local:validate"); + const inner = reachableScripts(scripts, "validate"); + for (const name of ["validate:guards", "validate:core"]) { + assert.ok(gate.has(name), `local:validate must reach ${name}`); + assert.ok(inner.has(name), `validate must reach ${name}`); + } + // Everything `validate` reaches at the root that is not a client + // delegation is reached by the gate too. + for (const name of inner) + if ( + name !== "validate" && + !/^validate:(web|cli|tui|launcher)$/.test(name) + ) + assert.ok(gate.has(name), `local:validate must reach ${name}`); + }); + + it("each client's `validate` is `check` plus `test`, and `check` runs no tests", () => { + for (const c of clients) { + const s = clientScripts[c]; + assert.equal( + s.validate, + "npm run check && npm run test", + `clients/${c} validate`, + ); + const fromCheck = reachableScripts(s, "check"); + for (const name of fromCheck) + assert.doesNotMatch( + name, + /^(pre|post)?test(:|$)/, + `clients/${c} check must not reach ${name}`, + ); + assert.ok( + !/\bvitest\b/.test([...fromCheck].map((n) => s[n]).join(" ")), + `clients/${c} check must not invoke vitest`, + ); + // And `check` is the whole of `validate` minus the test leg: what + // `validate` reaches is exactly itself, plus what `check` reaches, plus + // what `test` reaches (its `pretest` build preambles included). + assert.deepEqual( + [...reachableScripts(s, "validate")].sort(), + [ + ...new Set([ + "validate", + ...fromCheck, + ...reachableScripts(s, "test"), + ]), + ].sort(), + `clients/${c} validate must reach exactly check + test`, + ); + } + }); + }); + it("has no `ci` alias, not even a deprecated one", () => { // The rename is only worth anything if the old name is GONE (#2146): an // alias preserves exactly the association being scrubbed, and leaves