From 2b1db6bc6ec018c8c7eb6f795c8527e9605034ad Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 12 Oct 2026 23:06:19 -0400 Subject: [PATCH 1/2] chore(scripts): guard the file set each coverage run measures (#2709) The per-file coverage gate only judges files that reach coverage-summary.json, and nothing checked that the report holds the files it should. Under vitest 5 web's report collapsed from 418 files to 11 and the gate stayed green (#2557). verify:coverage-scope resolves each gated client's coverage include/exclude through Vitest, expands them against the file system with Node's own glob (never Vitest's matcher), and fails naming every declared file missing from the report. It runs as the last step of `npm run coverage`, so local:gate and CI's coverage job both enforce it. A coverage client it does not list fails deny-by-default. On v2/main every client's report matched its declaration except web's pendingReauth.ts, a type-only module v8 has no row for; it joins web's existing pure-type coverage excludes. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .claude/skills/pre-push-gate/SKILL.md | 19 ++ .github/workflows/main.yml | 4 + AGENTS.md | 2 +- clients/web/vite.config.ts | 3 + docs/quality-gate.md | 3 +- package.json | 3 +- scripts/verify-coverage-scope.mjs | 338 +++++++++++++++++++++++++ scripts/verify-coverage-scope.test.mjs | 319 +++++++++++++++++++++++ 8 files changed, 688 insertions(+), 3 deletions(-) create mode 100644 scripts/verify-coverage-scope.mjs create mode 100644 scripts/verify-coverage-scope.test.mjs diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 0ba5fc0a9..ca679038f 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -178,6 +178,25 @@ lowering the gate. See `/testing` for the acceptable reasons. Also check the file is inside a gated directory — the web coverage `include` is a whitelist, so a module placed outside it falls out of the gate silently. +### `verify:coverage-scope` + +The last step of `coverage` (#2709). A client's `coverage-summary.json` is +missing files its own coverage `include`/`exclude` declares, so those files +were never judged by the per-file gate. The failure names the client and each +missing file. Two causes: + +- **The run's glob matching changed** — typically a Vitest bump. The fix is + the client's `include` patterns (an absolute `path.join(dirname, …)` pattern + means the same thing under every matcher), never an entry that excuses the + files. +- **A file is in scope but the run legitimately drops it.** State it in + `KNOWN_OMISSIONS` in `scripts/verify-coverage-scope.mjs`, with the reason — + or, better, add it to the client's coverage `exclude`, which says the same + thing where the gate reads it. + +"no `coverage-summary.json`" means the client's coverage did not run (or ran +without the `json-summary` reporter). Run `npm run coverage`. + ### `smoke:web*` ⚠️ **An orphaned prod web server from a previous run fakes a rejection.** It diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 52a0c8ec5..a05fbcf93 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -217,6 +217,10 @@ jobs: # This also covers the web integration project: web's `test:coverage` # runs `--project=unit --project=integration --coverage`, so there is no # separate integration step anywhere in this workflow. + # + # `coverage` ends with `verify:coverage-scope` (#2709), which fails if + # any client's report measured fewer files than its coverage `include` + # declares — the gate otherwise passes on whatever subset it was handed. run: npm run coverage # Publish the single `@modelcontextprotocol/inspector` package to npm on a diff --git a/AGENTS.md b/AGENTS.md index 8d7c0031f..aad77a190 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -422,7 +422,7 @@ diagnose a failing gate — is the `testing` skill. These are the rules. - **Root tooling**: a `scripts/*.mjs` helper with pure logic gets a sibling `*.test.mjs`. Keep that exact filename — `node --test` silently _skips_ a file its glob misses and still exits 0. - **Render Ink components through the TUI's own `render`** (`clients/tui/__tests__/helpers/renderTui.tsx`), never `ink-testing-library`'s directly. It is the same function with every frame ANSI-stripped, which is what keeps an assertion on styled text from depending on the ambient environment: Ink writes styling _inside_ the styled run, so `Info` reaches the frame buffer with escapes between `I` and `nfo` and `toContain("Info")` fails. It only bites where chalk emits color — a developer whose shell exports `FORCE_COLOR` — so CI is green on a suite that is broken for them (#2207). A test that genuinely needs the raw bytes reads `stdout.lastFrame()` off the returned instance. - **Render React components through `renderWithMantine`** (`src/test/renderWithMantine.tsx`); do not hand-roll a bare `MantineProvider`, which skips the project theme and the helper's options and drifts from every other test. Pass the `colorScheme` option to exercise a forced scheme rather than hand-rolling `defaultColorScheme`. Use `renderWithMantineTransitions` **only** when a test must assert mid-flight transition state, and read the long comment on the helper before changing anything about it. -- **The web coverage `include` is a whitelist.** It names `components`/`hooks`/`theme`/`lib`/`utils`/`server` plus the browser-consumed `core/*` runtime, so a module placed **outside** those directories falls out of the gate entirely, silently. Place new modules inside a gated directory. The documented exceptions — `src/App.tsx` and the `src/main.tsx` / `src/index.ts` bootstraps — are called out in a comment on the `include` array itself. +- **The web coverage `include` is a whitelist.** It names `components`/`hooks`/`theme`/`lib`/`utils`/`server` plus the browser-consumed `core/*` runtime, so a module placed **outside** those directories falls out of the gate entirely, silently. Place new modules inside a gated directory. The converse — the run measuring **fewer** files than the `include` declares, as web's did under vitest 5 (418 → 11, still green) — is not silent: `npm run verify:coverage-scope`, the last step of `coverage`, fails naming each missing file (#2709). Fix that by fixing the globs, never by excusing the files. The documented exceptions — `src/App.tsx` and the `src/main.tsx` / `src/index.ts` bootstraps — are called out in a comment on the `include` array itself. ### Test-gate timeouts are chosen values, stated once diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index c079c57f4..3d22adb3d 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -252,6 +252,9 @@ export default defineConfig(({ command }) => { path.join(repoRoot, "core/mcp/remote/types.ts"), path.join(repoRoot, "core/mcp/import/types.ts"), "clients/web/server/types.ts", + // Type declarations only, so v8 reports no row for it at all — named + // here so `verify:coverage-scope` (#2709) is not left expecting one. + "src/utils/pendingReauth.ts", // .d.ts files are declaration-only. path.join(repoRoot, "**/*.d.ts"), // inspectorClientEventTarget.ts is types + a single empty-body class diff --git a/docs/quality-gate.md b/docs/quality-gate.md index 75b7f1c58..198e71ba0 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -35,7 +35,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 seven durable guards first — `verify:install-fresh` (every install's `node_modules` matches its own lockfile, so a checkout that pulled a dependency bump without re-running `npm install` fails in under a second naming it, instead of minutes later as a behavioral test — #2494), `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), `verify:action-pins` (every action a credentialed workflow job runs is SHA-pinned — #2484) — 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`). **`maxWorkers` is deliberately unset**, here and in every Vitest project ([#2336](https://github.com/modelcontextprotocol/inspector/issues/2336), PR [#2345](https://github.com/modelcontextprotocol/inspector/pull/2345)): the forks-pool projects inherit Vitest's `max(availableParallelism() - 1, 1)` — 7 on the eight-logical-core machine this team works on — and keeping it is a choice with a measurement, recorded beside `TIMEOUTS` in `vitest.shared.mts`: at the leased baseline, arms interleaved over three rounds, `maxWorkers: 4` cost +14% wall on web `unit` and +39% on `coverage:web` for zero fewer failures (0 across all 18 runs). See [Multi-agent testing](#multi-agent-testing-the-gate-lease) for why a cap was the wrong lever for the contention it was proposed against. | +| `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`). **`maxWorkers` is deliberately unset**, here and in every Vitest project ([#2336](https://github.com/modelcontextprotocol/inspector/issues/2336), PR [#2345](https://github.com/modelcontextprotocol/inspector/pull/2345)): the forks-pool projects inherit Vitest's `max(availableParallelism() - 1, 1)` — 7 on the eight-logical-core machine this team works on — and keeping it is a choice with a measurement, recorded beside `TIMEOUTS` in `vitest.shared.mts`: at the leased baseline, arms interleaved over three rounds, `maxWorkers: 4` cost +14% wall on web `unit` and +39% on `coverage:web` for zero fewer failures (0 across all 18 runs). See [Multi-agent testing](#multi-agent-testing-the-gate-lease) for why a cap was the wrong lever for the contention it was proposed against. Ends with `verify:coverage-scope` (below), which checks that each report measured the file set its client declares. | | `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. The Playwright budgets the browser smokes wait under are **named once**, in `scripts/lib/browser-timeouts.mjs` (`BROWSER_TIMEOUTS`: `ui` 30s, `roundTrip` 45s, `nested` 15s, `bestEffort` 5s — [#2323](https://github.com/modelcontextprotocol/inspector/issues/2323)); since [#2333](https://github.com/modelcontextprotocol/inspector/issues/2333) (PR [#2349](https://github.com/modelcontextprotocol/inspector/pull/2349)) the shared flow helpers `deep-link-connect.mjs` and `mcp-app-flow.mjs` default to those constants instead of carrying literals of their own, and the two helper budgets that stay put are **stated at the site with the measurement behind them**: `announced-child.mjs`'s `DEFAULTS` (a 30s readiness ceiling on a 250ms poll — boot-to-announce measured 135ms median through it at load average 15, so a passing start pays one poll interval, not the budget) and `pty.mjs`'s `SCRIPT_PROBE_TIMEOUT_MS` (5s for the `script --version` probe, measured at 1.3ms median). That probe's expiry is now a **failure, not a skip**: a timed-out `spawnSync` reports in the same `error` slot as `ENOENT`, and reading both as "no `script(1)` on PATH" had turned `smoke:tui` into an exit-0 skip on exactly the loaded machine the gate is meant to be believable on. | | `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. | | `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run local:gate` and of GitHub CI. | @@ -45,6 +45,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run verify:skills:cli` | Runs `claude plugin validate` — the **authoritative** skill schema — as a guaranteed step, in `local:gate` and in CI. `verify:skills` reimplements the parse and *skips* when the CLI is absent, which is right for `validate` (fast, offline, no Claude Code required) but adds up to "never runs". This one resolves the CLI instead: an installed one **only when it matches the pin exactly**, otherwise the pinned package via `npx -y @anthropic-ai/claude-code@…`. Exact, not a floor: accepting a newer local CLI would validate against a different schema than CI's, so the same `local:gate` could disagree across machines — which is the failure a pin exists to prevent. Needs no authentication. Not in `validate` — it can reach the network. | | `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. `gate-lease.test.mjs` ([#2339](https://github.com/modelcontextprotocol/inspector/issues/2339), PR [#2344](https://github.com/modelcontextprotocol/inspector/pull/2344)) covers the gate's lease wrapper the way `announced-child` and `render-smoke` are covered — against a **real** `proper-lockfile` lock and real child processes: two runs serialize; queued waiters take the lease in arrival order even when a later one polls faster, a live ticket ahead holds the line while a dead one is pruned, and an unwritable queue still takes the lease out of turn ([#2473](https://github.com/modelcontextprotocol/inspector/issues/2473)); a stale lock is taken over while one that cannot be removed is waited on; the give-up names the holder; the bypass variable and an unusable lease directory each degrade to an unleased run; and a real `SIGTERM` to the wrapper stops the grandchild, releases the lease and exits 143. Every case uses a throwaway lease directory, so the suite can neither queue behind nor block a developer's actual gate, and an `after` hook removes them ([#2346](https://github.com/modelcontextprotocol/inspector/issues/2346), PR [#2351](https://github.com/modelcontextprotocol/inspector/pull/2351): 14 directories left behind per run → 0). `workflow-gate.test.mjs` also pins `local:gate` to being *exactly* the wrapper over `local:gate:stages` — a stage placed beside the wrapper would run outside the lease — and every assertion about the gate's shape reads the stages script directly. 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:coverage-scope` | The coverage analog of the two guards above ([#2709](https://github.com/modelcontextprotocol/inspector/issues/2709)), for the file set the per-file gate judges. For each client `npm run coverage` gates, it resolves the coverage `include`/`exclude` through Vitest, expands them against the file system with Node's own `fs.globSync` / `path.matchesGlob` — never Vitest's matcher — and **fails** listing every declared file missing from that client's `coverage-summary.json`. Under vitest 5 web's report fell from 418 files to 11 while every remaining file cleared 90, so the gate went green on 3% of its scope ([#2557](https://github.com/modelcontextprotocol/inspector/issues/2557)); this is what turns that red. No count to maintain; deliberate omissions are stated in its `KNOWN_OMISSIONS`; a coverage client it does not list fails deny-by-default. Reads coverage output, so it runs as the last step of `coverage` (locally in `local:gate`, and in CI's `coverage` job), not 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 verify:action-pins` | Guards the credentialed-job pinning rule ([#2484](https://github.com/modelcontextprotocol/inspector/issues/2484)). Actions stay on moving major tags by default (#2235), but a tag is mutable, so whoever can move one replaces the code running next to a credential. For every job under `.github/workflows/` that holds `id-token: write` / `packages: write` / `write-all` (its own `permissions:` or the workflow's), is handed any secret other than `GITHUB_TOKEN`, or uploads an artifact such a job downloads and `needs`, it fails on any `uses:` that is not a 40-hex commit SHA followed by an exact `# vX.Y.Z` comment. The comment is what keeps the pin in the monthly `dependency-refresh` sweep, which ranks a SHA pin by it to the patch. Offline, so it cannot check that the SHA matches the comment — resolve both from one tag lookup. | diff --git a/package.json b/package.json index 08c666215..5b521ac3e 100644 --- a/package.json +++ b/package.json @@ -80,6 +80,7 @@ "verify:install-fresh": "node scripts/verify-install-fresh.mjs", "verify:test-timeouts": "node scripts/verify-test-timeouts.mjs", "verify:action-pins": "node scripts/verify-action-pins.mjs", + "verify:coverage-scope": "node scripts/verify-coverage-scope.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 vitest.setup.shared.mts eslint.config.js --max-warnings 0", @@ -95,7 +96,7 @@ "validate:tui": "cd clients/tui && npm run validate", "validate:web": "cd clients/web && npm run validate", "validate:launcher": "cd clients/launcher && npm run validate", - "coverage": "npm run coverage:web && npm run coverage:cli && npm run coverage:mcpdo && npm run coverage:tui && npm run coverage:launcher", + "coverage": "npm run coverage:web && npm run coverage:cli && npm run coverage:mcpdo && npm run coverage:tui && npm run coverage:launcher && npm run verify:coverage-scope", "coverage:cli": "cd clients/cli && npm run test:coverage", "coverage:mcpdo": "cd clients/mcpdo && npm run test:coverage", "coverage:tui": "cd clients/tui && npm run test:coverage", diff --git a/scripts/verify-coverage-scope.mjs b/scripts/verify-coverage-scope.mjs new file mode 100644 index 000000000..4cd88db1a --- /dev/null +++ b/scripts/verify-coverage-scope.mjs @@ -0,0 +1,338 @@ +/** + * Guards the file set each client's coverage run measures (#2709). + * + * The per-file ≥90 gate only judges the files that reach + * `coverage/coverage-summary.json`. Nothing checked that the report holds the + * files it should, so when a client's coverage `include` globs stop matching, + * the gate shrinks silently and stays green. That happened during the #2557 + * dependency refresh: under vitest 5, whose fix for "coverage include/exclude + * globs too eager" changed how `clients/web`'s mixed relative/absolute + * patterns resolve, web's report fell from 418 files to 11 — every remaining + * file cleared 90, every test passed, and only a by-hand comparison of the + * summary file counts caught it. It is #1301's class (core/ files silently + * leaving the gate), reached through a tooling upgrade instead of a path move. + * + * **The check compares the report against an independent expansion of the + * client's own declaration.** For each client, Vitest resolves the config — + * so the patterns read here are the ones the run actually used, hard-coded + * default exclusions included — but the patterns are then expanded against + * the file system with Node's own `fs.globSync` / `path.matchesGlob`, never + * with Vitest's matcher. Asking the tool under test which files it meant would + * agree with it by construction, which is exactly the blind spot this closes. + * There is no file count to maintain: a new file under an included directory + * is expected the moment it exists. + * + * **How a declared pattern is read** — the declaration, not any one Vitest + * version's matcher, is the reference: + * + * • an absolute pattern means itself; + * • a relative pattern resolves against the client directory, unless it + * starts with the client's own repo-relative path (`clients/web/server/**` + * in web's list), which can only mean a repo-root-relative path; + * • an exclude pattern removes a file when it matches the file's absolute, + * client-relative or repo-relative path. That is deliberately lenient: an + * over-broad exclude can only make the guard expect fewer files, never + * report a file as missing that the run was right to drop. + * + * Every client gated by `npm run coverage` is checked, and a client with a + * `test:coverage` script that this guard does not list fails loudly rather + * than going unchecked — the same deny-by-default as `verify:test-timeouts`. + * + * It reads coverage output, so it runs at the end of the root `coverage` + * script (and with it in `local:gate` and the CI `coverage` job), not in + * `validate`. + * + * ⚠️ Observed to FAIL before it was trusted: with web's summary reduced to + * the `clients/web/server/**` rows a vitest 5 run produced, it names every + * web component, hook, lib, util and core/ file that dropped out. + */ + +import { + existsSync, + globSync, + readFileSync, + readdirSync, + statSync, +} from "node:fs"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +const scriptsDir = path.dirname(fileURLToPath(import.meta.url)); +const repoRoot = path.resolve(scriptsDir, ".."); + +/** + * Every client whose coverage run `npm run coverage` gates, and the config + * file that declares its coverage scope. + */ +export const CLIENTS = [ + { name: "web", config: "vite.config.ts" }, + { name: "cli", config: "vitest.config.ts" }, + { name: "mcpdo", config: "vitest.config.ts" }, + { name: "tui", config: "vitest.config.ts" }, + { name: "launcher", config: "vitest.config.ts" }, +]; + +/** + * Files a client's declaration matches that its coverage run legitimately + * omits, keyed by client, as repo-relative paths. Each entry must say why. + * Empty today: on `v2/main` every client's report holds exactly what its + * declaration expands to. + * + * @type {Record>} + */ +export const KNOWN_OMISSIONS = {}; + +/** @param {string} p */ +const slash = (p) => p.split(path.sep).join("/"); + +/** + * Expand one client's coverage `include`/`exclude` against the file system. + * + * @param {{ + * include: readonly string[], + * exclude: readonly string[], + * clientDir: string, + * repoRoot: string, + * }} scope absolute `clientDir` and `repoRoot` + * @returns {string[]} sorted absolute, forward-slashed file paths + */ +export function expandCoverageScope({ include, exclude, clientDir, repoRoot }) { + const clientPrefix = `${slash(path.relative(repoRoot, clientDir))}/`; + const files = new Set(); + for (const raw of include) { + const pattern = slash(raw); + let matches; + if (path.isAbsolute(raw)) { + matches = globSync(pattern); + } else { + const base = pattern.startsWith(clientPrefix) ? repoRoot : clientDir; + matches = globSync(pattern, { cwd: base }).map((f) => + path.resolve(base, f), + ); + } + for (const f of matches) files.add(slash(path.resolve(f))); + } + const excludes = exclude.map(slash); + return ( + [...files] + .filter((abs) => { + const candidates = [ + abs, + slash(path.relative(clientDir, abs)), + slash(path.relative(repoRoot, abs)), + ]; + return !excludes.some((p) => + candidates.some((c) => path.matchesGlob(c, p)), + ); + }) + // `globSync` returns directories too; a coverage report holds files. + .filter((abs) => statSync(abs).isFile()) + .sort() + ); +} + +/** + * Compare the expected file set with the files a coverage summary reports. + * + * @param {{ + * client: string, + * expected: readonly string[], + * reported: readonly string[], + * omissions?: Record, + * repoRoot: string, + * }} input absolute, forward-slashed paths + * @returns {string[]} failure messages; empty when the scope holds + */ +export function diffCoverageScope({ + client, + expected, + reported, + omissions = {}, + repoRoot, +}) { + const failures = []; + if (expected.length === 0) { + failures.push( + `${client}: its coverage include expands to no files at all — the ` + + `declaration no longer names anything on disk`, + ); + return failures; + } + const have = new Set(reported.map(slash)); + const rel = (abs) => slash(path.relative(repoRoot, abs)); + const missing = expected.filter( + (f) => !have.has(f) && !(rel(f) in omissions), + ); + if (missing.length > 0) { + failures.push( + `${client}: ${missing.length} of ${expected.length} file(s) its coverage ` + + `include declares are missing from coverage-summary.json (the report ` + + `holds ${have.size}) — the run measured a narrower scope than it ` + + `declares, so those files are not under the per-file gate:\n` + + missing.map((f) => ` ${rel(f)}`).join("\n"), + ); + } + const expectedRel = new Set(expected.map(rel)); + for (const omitted of Object.keys(omissions)) { + if (!expectedRel.has(omitted)) { + failures.push( + `${client}: KNOWN_OMISSIONS names ${omitted}, which its declaration ` + + `no longer matches — a stale entry hides nothing and should be removed`, + ); + } + } + return failures; +} + +/** + * Clients on disk with a `test:coverage` script that `CLIENTS` does not list, + * and listed clients with no such script. + * + * @param {string[]} discovered client names with a `test:coverage` script + * @param {readonly {name: string}[]} [configured] + * @returns {string[]} + */ +export function checkClientRoster(discovered, configured = CLIENTS) { + const known = new Set(configured.map((c) => c.name)); + const failures = []; + for (const name of discovered) { + if (!known.has(name)) { + failures.push( + `clients/${name} has a test:coverage script this guard does not ` + + `check — add it to CLIENTS rather than leaving its scope unguarded`, + ); + } + } + for (const name of known) { + if (!discovered.includes(name)) { + failures.push( + `CLIENTS names ${name}, which has no test:coverage script — a stale ` + + `row silently stops checking anything`, + ); + } + } + return failures; +} + +/** + * @param {string} root + * @returns {string[]} + */ +function discoverCoverageClients(root) { + const clientsDir = path.join(root, "clients"); + return readdirSync(clientsDir, { withFileTypes: true }) + .filter((d) => d.isDirectory()) + .map((d) => d.name) + .filter((name) => { + const manifest = path.join(clientsDir, name, "package.json"); + if (!existsSync(manifest)) return false; + const scripts = JSON.parse(readFileSync(manifest, "utf8")).scripts ?? {}; + return typeof scripts["test:coverage"] === "string"; + }) + .sort(); +} + +/** + * Resolve a client's coverage options through Vitest's own config resolver, + * so the patterns checked are the ones the run used. + * + * @param {string} clientDir + * @param {string} configFile + * @returns {Promise<{include: string[], exclude: string[]}>} + */ +async function resolveCoverageOptions(clientDir, configFile) { + const { createVitest } = await import("vitest/node"); + const vitest = await createVitest("test", { + watch: false, + run: true, + root: clientDir, + config: path.join(clientDir, configFile), + }); + try { + const { include, exclude } = vitest.config.coverage; + return { include: include ?? [], exclude: exclude ?? [] }; + } finally { + await vitest.close(); + } +} + +/** + * @param {string} summaryPath + * @returns {string[] | null} reported absolute paths, or null when absent + */ +function readReportedFiles(summaryPath) { + if (!existsSync(summaryPath)) return null; + const summary = JSON.parse(readFileSync(summaryPath, "utf8")); + return Object.keys(summary).filter((k) => k !== "total"); +} + +/** + * @param {{ + * root?: string, + * clients?: readonly {name: string, config: string}[], + * omissions?: Record>, + * resolveOptions?: typeof resolveCoverageOptions, + * log?: (line: string) => void, + * }} [deps] + * @returns {Promise} failures + */ +export async function run({ + root = repoRoot, + clients = CLIENTS, + omissions = KNOWN_OMISSIONS, + resolveOptions = resolveCoverageOptions, + log = (line) => console.log(line), +} = {}) { + const failures = checkClientRoster(discoverCoverageClients(root), clients); + for (const { name, config } of clients) { + const clientDir = path.join(root, "clients", name); + const summaryPath = path.join( + clientDir, + "coverage", + "coverage-summary.json", + ); + const reported = readReportedFiles(summaryPath); + if (reported === null) { + failures.push( + `${name}: no ${slash(path.relative(root, summaryPath))} — run its ` + + `coverage first (\`npm run coverage\`); this guard reads its output`, + ); + continue; + } + const { include, exclude } = await resolveOptions(clientDir, config); + const expected = expandCoverageScope({ + include, + exclude, + clientDir, + repoRoot: root, + }); + const clientFailures = diffCoverageScope({ + client: name, + expected, + reported, + omissions: omissions[name], + repoRoot: root, + }); + failures.push(...clientFailures); + if (clientFailures.length === 0) { + log(` ${name}: ${expected.length} declared file(s), all measured`); + } + } + return failures; +} + +if ( + process.argv[1] && + import.meta.url === pathToFileURL(process.argv[1]).href +) { + console.log( + "verify:coverage-scope — coverage reports hold their declared scope", + ); + const failures = await run(); + if (failures.length > 0) { + console.error(`\n✗ ${failures.length} coverage-scope failure(s):\n`); + for (const f of failures) console.error(` - ${f}\n`); + process.exit(1); + } + console.log("✓ every client's coverage report holds its declared file set"); +} diff --git a/scripts/verify-coverage-scope.test.mjs b/scripts/verify-coverage-scope.test.mjs new file mode 100644 index 000000000..4475db236 --- /dev/null +++ b/scripts/verify-coverage-scope.test.mjs @@ -0,0 +1,319 @@ +// Unit tests for verify-coverage-scope.mjs (#2709): how a declared coverage +// scope is expanded, how a report is compared against it, and the client +// roster's deny-by-default. `run()` is driven over a throwaway repo with an +// injected config resolver, so the vitest-5 shape that motivated the guard — +// a report collapsed to `clients/web/server/**` — is reproduced without +// installing vitest 5. Run via `npm run test:scripts`. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { + CLIENTS, + checkClientRoster, + diffCoverageScope, + expandCoverageScope, + run, +} from "./verify-coverage-scope.mjs"; + +const slash = (p) => p.split(path.sep).join("/"); + +/** + * A throwaway repo holding the given files (repo-relative), each with a stub + * body. realpath'd because macOS `tmpdir()` is a symlink and the guard + * compares resolved paths. + * + * @param {string[]} files + */ +function makeRepo(files) { + const root = realpathSync(mkdtempSync(path.join(tmpdir(), "cov-scope-"))); + for (const f of files) { + const abs = path.join(root, f); + mkdirSync(path.dirname(abs), { recursive: true }); + writeFileSync(abs, "export {};\n"); + } + return root; +} + +const abs = (root, ...rel) => rel.map((r) => slash(path.join(root, r))); + +test("a relative include resolves against the client directory", (t) => { + const root = makeRepo([ + "clients/web/src/components/A.tsx", + "clients/web/src/components/b/B.ts", + "src/components/NotThisOne.tsx", + ]); + t.after(() => rmSync(root, { recursive: true, force: true })); + const expected = expandCoverageScope({ + include: ["src/components/**/*.{ts,tsx}"], + exclude: [], + clientDir: path.join(root, "clients/web"), + repoRoot: root, + }); + assert.deepEqual( + expected, + abs( + root, + "clients/web/src/components/A.tsx", + "clients/web/src/components/b/B.ts", + ), + ); +}); + +test("an include starting with the client's own repo path resolves against the repo root", (t) => { + const root = makeRepo(["clients/web/server/s.ts"]); + t.after(() => rmSync(root, { recursive: true, force: true })); + const expected = expandCoverageScope({ + include: ["clients/web/server/**/*.{ts,tsx}"], + exclude: [], + clientDir: path.join(root, "clients/web"), + repoRoot: root, + }); + assert.deepEqual(expected, abs(root, "clients/web/server/s.ts")); +}); + +test("an absolute include means itself", (t) => { + const root = makeRepo(["core/json/a.ts", "core/json/deep/b.ts"]); + t.after(() => rmSync(root, { recursive: true, force: true })); + const expected = expandCoverageScope({ + include: [path.join(root, "core/json/**/*.{ts,tsx}")], + exclude: [], + clientDir: path.join(root, "clients/web"), + repoRoot: root, + }); + assert.deepEqual( + expected, + abs(root, "core/json/a.ts", "core/json/deep/b.ts"), + ); +}); + +test("an exclude matches absolute, client-relative or repo-relative paths", (t) => { + const root = makeRepo([ + "clients/web/src/lib/keep.ts", + "clients/web/src/lib/keep.test.ts", + "clients/web/src/lib/index.ts", + "clients/web/server/plugin.ts", + "clients/web/server/kept.ts", + "core/mcp/types.ts", + "core/mcp/client.ts", + ]); + t.after(() => rmSync(root, { recursive: true, force: true })); + const expected = expandCoverageScope({ + include: [ + "src/lib/**/*.ts", + "clients/web/server/**/*.ts", + path.join(root, "core/mcp/**/*.ts"), + ], + exclude: [ + "**/*.test.ts", // glob over any form + "src/lib/index.ts", // client-relative + "clients/web/server/plugin.ts", // repo-relative + path.join(root, "core/mcp/types.ts"), // absolute + ], + clientDir: path.join(root, "clients/web"), + repoRoot: root, + }); + assert.deepEqual( + expected, + abs( + root, + "clients/web/server/kept.ts", + "clients/web/src/lib/keep.ts", + "core/mcp/client.ts", + ), + ); +}); + +test("directories a glob matches are not expected files", (t) => { + const root = makeRepo(["clients/tui/src/dir.ts/inner.ts"]); + t.after(() => rmSync(root, { recursive: true, force: true })); + const expected = expandCoverageScope({ + include: ["src/**/*.ts"], + exclude: [], + clientDir: path.join(root, "clients/tui"), + repoRoot: root, + }); + assert.deepEqual(expected, abs(root, "clients/tui/src/dir.ts/inner.ts")); +}); + +test("a report holding the declared set passes", () => { + const expected = ["/r/clients/cli/src/a.ts", "/r/clients/cli/src/b.ts"]; + assert.deepEqual( + diffCoverageScope({ + client: "cli", + expected, + reported: [...expected].reverse(), + repoRoot: "/r", + }), + [], + ); +}); + +test("a narrower report fails, naming the client and each missing file", () => { + const failures = diffCoverageScope({ + client: "web", + expected: [ + "/r/clients/web/server/s.ts", + "/r/clients/web/src/components/A.tsx", + "/r/core/json/a.ts", + ], + reported: ["/r/clients/web/server/s.ts"], + repoRoot: "/r", + }); + assert.equal(failures.length, 1); + assert.match(failures[0], /^web: 2 of 3 file\(s\)/); + assert.match(failures[0], /report holds 1/); + assert.match(failures[0], /\n {4}clients\/web\/src\/components\/A\.tsx\n/); + assert.match(failures[0], /\n {4}core\/json\/a\.ts$/); +}); + +test("an include that expands to nothing fails rather than passing vacuously", () => { + const failures = diffCoverageScope({ + client: "tui", + expected: [], + reported: [], + repoRoot: "/r", + }); + assert.equal(failures.length, 1); + assert.match(failures[0], /expands to no files at all/); +}); + +test("a stated omission is not reported missing, and a stale one is flagged", () => { + const ok = diffCoverageScope({ + client: "web", + expected: ["/r/clients/web/src/a.ts", "/r/clients/web/src/b.ts"], + reported: ["/r/clients/web/src/a.ts"], + omissions: { "clients/web/src/b.ts": "types only" }, + repoRoot: "/r", + }); + assert.deepEqual(ok, []); + const stale = diffCoverageScope({ + client: "web", + expected: ["/r/clients/web/src/a.ts"], + reported: ["/r/clients/web/src/a.ts"], + omissions: { "clients/web/src/gone.ts": "types only" }, + repoRoot: "/r", + }); + assert.equal(stale.length, 1); + assert.match(stale[0], /KNOWN_OMISSIONS names clients\/web\/src\/gone\.ts/); +}); + +test("the roster fails on an unlisted coverage client and on a stale row", () => { + const listed = CLIENTS.map((c) => c.name); + assert.deepEqual(checkClientRoster([...listed].sort()), []); + const unlisted = checkClientRoster([...listed, "newclient"]); + assert.equal(unlisted.length, 1); + assert.match(unlisted[0], /clients\/newclient has a test:coverage script/); + const stale = checkClientRoster(listed.filter((n) => n !== "tui")); + assert.equal(stale.length, 1); + assert.match(stale[0], /CLIENTS names tui/); +}); + +test("the shipped roster is the five clients `npm run coverage` gates", () => { + assert.deepEqual(CLIENTS.map((c) => c.name).sort(), [ + "cli", + "launcher", + "mcpdo", + "tui", + "web", + ]); +}); + +/** + * A fixture repo with one coverage client, `web`, whose declaration matches + * three files — two outside `clients/web/server/**`. + */ +function makeWebFixture() { + const root = makeRepo([ + "clients/web/server/s.ts", + "clients/web/src/components/A.tsx", + "core/json/a.ts", + ]); + writeFileSync( + path.join(root, "clients/web/package.json"), + JSON.stringify({ scripts: { "test:coverage": "vitest run --coverage" } }), + ); + const resolveOptions = async () => ({ + include: [ + "src/components/**/*.{ts,tsx}", + "clients/web/server/**/*.{ts,tsx}", + path.join(root, "core/json/**/*.{ts,tsx}"), + ], + exclude: ["**/node_modules/**"], + }); + /** @param {string[]} rel */ + const writeSummary = (rel) => { + const summary = { total: {} }; + for (const r of rel) summary[path.join(root, r)] = {}; + mkdirSync(path.join(root, "clients/web/coverage"), { recursive: true }); + writeFileSync( + path.join(root, "clients/web/coverage/coverage-summary.json"), + JSON.stringify(summary), + ); + }; + return { root, resolveOptions, writeSummary }; +} + +const webOnly = [{ name: "web", config: "vite.config.ts" }]; + +test("run(): a complete report passes", async (t) => { + const { root, resolveOptions, writeSummary } = makeWebFixture(); + t.after(() => rmSync(root, { recursive: true, force: true })); + writeSummary([ + "clients/web/server/s.ts", + "clients/web/src/components/A.tsx", + "core/json/a.ts", + ]); + const lines = []; + const failures = await run({ + root, + clients: webOnly, + omissions: {}, + resolveOptions, + log: (l) => lines.push(l), + }); + assert.deepEqual(failures, []); + assert.deepEqual(lines, [" web: 3 declared file(s), all measured"]); +}); + +test("run(): the vitest-5 collapse to clients/web/server/** fails (#2557)", async (t) => { + const { root, resolveOptions, writeSummary } = makeWebFixture(); + t.after(() => rmSync(root, { recursive: true, force: true })); + writeSummary(["clients/web/server/s.ts"]); + const failures = await run({ + root, + clients: webOnly, + omissions: {}, + resolveOptions, + log: () => {}, + }); + assert.equal(failures.length, 1); + assert.match(failures[0], /^web: 2 of 3 file\(s\)/); + assert.match(failures[0], /clients\/web\/src\/components\/A\.tsx/); + assert.match(failures[0], /core\/json\/a\.ts/); +}); + +test("run(): a missing summary fails rather than being skipped", async (t) => { + const { root, resolveOptions } = makeWebFixture(); + t.after(() => rmSync(root, { recursive: true, force: true })); + const failures = await run({ + root, + clients: webOnly, + omissions: {}, + resolveOptions, + log: () => {}, + }); + assert.equal(failures.length, 1); + assert.match( + failures[0], + /^web: no clients\/web\/coverage\/coverage-summary\.json/, + ); +}); From a305dcaf5d990c8d73075703bccffab5c6b8dc0b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 12 Oct 2026 23:18:40 -0400 Subject: [PATCH 2/2] fix(scripts): require a reason on coverage-scope omissions; flag redundant ones (#2709) Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- scripts/verify-coverage-scope.mjs | 19 ++++++++++++++-- scripts/verify-coverage-scope.test.mjs | 31 ++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/scripts/verify-coverage-scope.mjs b/scripts/verify-coverage-scope.mjs index 4cd88db1a..ec119f3d5 100644 --- a/scripts/verify-coverage-scope.mjs +++ b/scripts/verify-coverage-scope.mjs @@ -160,8 +160,12 @@ export function diffCoverageScope({ } const have = new Set(reported.map(slash)); const rel = (abs) => slash(path.relative(repoRoot, abs)); + // An omission is honored only with a stated reason; an entry without one + // would weaken the gate as silently as the glob drift this guard exists for. + const isJustified = (r) => + typeof omissions[r] === "string" && omissions[r].trim() !== ""; const missing = expected.filter( - (f) => !have.has(f) && !(rel(f) in omissions), + (f) => !have.has(f) && !(rel(f) in omissions && isJustified(rel(f))), ); if (missing.length > 0) { failures.push( @@ -173,12 +177,23 @@ export function diffCoverageScope({ ); } const expectedRel = new Set(expected.map(rel)); + const reportedRel = new Set([...have].map(rel)); for (const omitted of Object.keys(omissions)) { - if (!expectedRel.has(omitted)) { + if (!isJustified(omitted)) { + failures.push( + `${client}: KNOWN_OMISSIONS names ${omitted} without a reason — ` + + `state why the run may omit it, or remove the entry`, + ); + } else if (!expectedRel.has(omitted)) { failures.push( `${client}: KNOWN_OMISSIONS names ${omitted}, which its declaration ` + `no longer matches — a stale entry hides nothing and should be removed`, ); + } else if (reportedRel.has(omitted)) { + failures.push( + `${client}: KNOWN_OMISSIONS names ${omitted}, which the report now ` + + `measures — the exemption is stale and should be removed`, + ); } } return failures; diff --git a/scripts/verify-coverage-scope.test.mjs b/scripts/verify-coverage-scope.test.mjs index 4475db236..4f4f97e52 100644 --- a/scripts/verify-coverage-scope.test.mjs +++ b/scripts/verify-coverage-scope.test.mjs @@ -206,6 +206,37 @@ test("a stated omission is not reported missing, and a stale one is flagged", () assert.match(stale[0], /KNOWN_OMISSIONS names clients\/web\/src\/gone\.ts/); }); +test("an omission without a reason is not honored and is itself a failure", () => { + for (const reason of ["", " ", undefined]) { + const failures = diffCoverageScope({ + client: "web", + expected: ["/r/clients/web/src/a.ts", "/r/clients/web/src/b.ts"], + reported: ["/r/clients/web/src/a.ts"], + omissions: { "clients/web/src/b.ts": reason }, + repoRoot: "/r", + }); + assert.equal(failures.length, 2, `reason ${JSON.stringify(reason)}`); + assert.match(failures[0], /^web: 1 of 2 file\(s\)/); + assert.match(failures[0], /clients\/web\/src\/b\.ts/); + assert.match( + failures[1], + /names clients\/web\/src\/b\.ts without a reason/, + ); + } +}); + +test("an omission for a file the report now measures is flagged as stale", () => { + const failures = diffCoverageScope({ + client: "web", + expected: ["/r/clients/web/src/a.ts"], + reported: ["/r/clients/web/src/a.ts"], + omissions: { "clients/web/src/a.ts": "types only" }, + repoRoot: "/r", + }); + assert.equal(failures.length, 1); + assert.match(failures[0], /which the report now measures/); +}); + test("the roster fails on an unlisted coverage client and on a stale row", () => { const listed = CLIENTS.map((c) => c.name); assert.deepEqual(checkClientRoster([...listed].sort()), []);