Skip to content

Commit 1bfa9b9

Browse files
cliffhallclaude
andcommitted
test(scripts): make test:scripts self-guarding; fill parser test gaps
Responds to the round-30 @claude review of #1799 (mutation testing killed 5/5; findings are about the new test gate + suite completeness): - Finding 1: `test:scripts` lacked the two properties the guard enforces for every tsc pass. verify-typecheck-coverage now asserts, in phase 1, that `test:scripts` is reachable from the root `validate` (via the already-imported rootReachesScript) AND that its tracked `scripts/**/*.{test,spec}.*` file set is non-empty and every file is matched by the command's glob (a new exported `globToRegExp`). So dropping it from validate, or renaming a test to `*.spec.mjs`/`*.test.mts` (which node --test silently skips), now hard-fails. Verified both. - Nit 2: extracted a pure `parseTsconfigReferences(raw)` from tsconfigReferences and table-tested the JSONC tolerance (block/line comments, trailing comma, no references, malformed, no path) — the r17/r26 rules had no cover. - Nit 3: added `-b`/`--build`/bare-`tsc -b` and explicit `--project` cases to typecheckProjects's test so its title matches its table. - Nit 4: rewrote the rootRunsClientValidate test to put `cmd` in a script reached from validate, and added the reachability cases (an orphan cd-validate must NOT count; validate:tui indirection must) — the r5 property. - Nit 5: added test:scripts to AGENTS's validate ordering + the README table. 15 guard tests now. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
1 parent 0a415d1 commit 1bfa9b9

5 files changed

Lines changed: 123 additions & 4 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i
229229
### Mandatory pre-push gate
230230
- ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/` tooling (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. Every **client** format glob uses the uniform extension set `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` (#1792) so a new-extension file can't slip the gate; `core/` stays `{ts,tsx}` and the shared surface `{ts,tsx,mts,cts}` (their surfaces can't hold the other extensions), and `npm run verify:format-coverage` (the first step of `validate`, #1792) is the backstop — it fails if any tracked source file is left uncovered by a `format:check` glob regardless of which glob was expected to catch it. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it.
231231
- **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression).
232-
- ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then **`verify:typecheck-coverage`** (the #1791 guard — asserts every tracked `.ts`/`.tsx`/`.mts`/`.cts` in each gated Node client, plus the non-client first-party TS like `core/` and `test-servers/src`, lands in a tsconfig project), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies).
232+
- ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then **`verify:typecheck-coverage`** (the #1791 guard — asserts every tracked `.ts`/`.tsx`/`.mts`/`.cts` in each gated Node client, plus the non-client first-party TS like `core/` and `test-servers/src`, lands in a tsconfig project), then **`test:scripts`** (the guard's own parser unit tests, `node --test`), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies).
233233
- **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}"`, the root build/verify tooling — #1778) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.{ts,tsx,mts,cts}`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump.
234234
- **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib *resolution* options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`.
235235
- **The `__tests__` dirs are typechecked too (#1791).** The src-only `tsconfig.json` excludes `**/*.test.*`, so each of cli, tui, and launcher carries a **`tsconfig.test.json`** — extending the build config, `noEmit`, including `__tests__/**/*` (only the tests root the project; tsc pulls in the `src` they import, and the src-only config already validates all of `src` without the test-only aliases) and adding the test-only path aliases that resolve what vitest resolves via `vitest.shared.mts`. The alias set differs per client: **cli's is the widest** (`@modelcontextprotocol/inspector-test-server` → `test-servers/src`, the `@inspector/core/*` deep paths, express/vitest — cli is the only one importing the test-server package); **tui's** carries only the `@inspector/core/*` + react/vitest redirects; **launcher's** has **no** `paths` at all — it's a plain `rootDir: "."` sibling of the build config (whose `rootDir: ./src` is what rejects the tests). Each client's `typecheck` script runs **both** projects (`tsc -p tsconfig.json && tsc -p tsconfig.test.json`) so running it standalone means the same thing everywhere (launcher's `build` also `tsc`s `src`, but `typecheck` doesn't rely on that). cli additionally carries `@types/express` (devDep) so the transitively-aliased test-server source typechecks, mirroring `clients/web` (cli's `tsconfig.test.json` also names `test-servers/src/server-composable.ts` explicitly — a bin entry the barrel doesn't import, so nothing else gives it a tsc pass). The client **config files** are typechecked too: cli's/tui's (`vitest.config.ts`, `tsup.config.ts`, tui `dev.ts`) are folded into each src `tsconfig.json`'s `include`; launcher's `vitest.config.ts` goes in its `tsconfig.test.json` instead (again the `rootDir: ./src` reason). Note the gate checks mock **implementations and return types** (typing a `vi.fn<T>()` against a real signature keeps its `mockResolvedValue`/impl in sync) but **not** `toHaveBeenCalledWith(...)` arguments — vitest types those to accept anything regardless of the mock's type parameter. **`npm run verify:typecheck-coverage`** (`scripts/verify-typecheck-coverage.mjs`, run as the second step of `validate` right after `verify:format-coverage`) is the durable guard for this invariant: it runs each client's `typecheck` projects with `tsc --listFilesOnly`, unions them, and fails on any tracked `.ts`/`.tsx`/`.mts`/`.cts` that lands in no project — for every gated Node client, which it discovers from disk (each `clients/*` is enrolled through its `typecheck` script's projects, or — for a `tsc -b` client like `clients/web` with no `typecheck` script — through its `tsconfig.json` `references`), so a new client is covered without editing the guard — the typecheck analog of `verify:format-coverage`, since a project only reaches the files its `include` names plus their transitive imports, so a new top-level file (launcher especially, whose build `rootDir: ./src` rejects package-root files) can otherwise fall out silently. Like its sibling it also asserts the gate is *wired* (each client's typecheck pass is reachable from its `validate` — its `typecheck` script for cli/tui/launcher, or a real `tsc -b` for web — and the root chain runs each client's `validate`), so it can't stay green while measuring a pass nothing invokes. Beyond the clients it also covers, **deny-by-default**, the first-party TS no client owns — everything tracked outside `clients/*` (`test-servers/src/**`, the root `vitest.shared.mts`, **all of `core/`**, and any new top-level TS location) must land in the *global* union of client projects (cli aliases the test-server source; web's enrolled projects include `core/`). So a `core` `*.tsx` web's `include` doesn't reach, or an unimported `test-servers/src` bin entry, can't ship uncompiled-but-unchecked. The one "listed but unchecked" tier the guard structurally can't see — a per-file `// @ts-nocheck` — is owned by a different gate: `@typescript-eslint/ban-ts-comment` rejects it across every surface (`lint:core`, `lint:shared`, and each client's `eslint .`). The guard's own pure parsers (`scripts/lib/npm-scripts.mjs` + the exported helpers of `verify-typecheck-coverage.mjs`, whose execution is behind a `main()` so importing it for tests doesn't run it) are **unit-tested** — `npm run test:scripts` (node's built-in `node --test`, in `validate`; the root has no vitest harness by design) runs table-driven cases, one per rule the guard's parsers encode.

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ Each client self-validates from its own folder; the root scripts chain them. The
146146
| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus a headless-Chromium 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). |
147147
| `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 ci`. |
148148
| `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. |
149+
| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs` + the exported helpers of `verify-typecheck-coverage.mjs`), one case per rule they encode. Runs in `validate`. |
149150
| `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`. |
150151
| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. |
151152
| `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). |

‎scripts/lib/npm-scripts.test.mjs‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,9 @@ test("reachableScripts: a `prevalidate`-hosted typecheck is reachable (r10)", ()
6060
});
6161

6262
test("rootRunsClientValidate: forms that count vs. don't", () => {
63+
// `cmd` lives in `vt`, reached from `validate` via `npm run vt`.
6364
const runs = (cmd) =>
64-
rootRunsClientValidate({ validate: cmd, x: cmd }, "clients/tui");
65+
rootRunsClientValidate({ validate: "npm run vt", vt: cmd }, "clients/tui");
6566
// Counts:
6667
assert.ok(runs("cd clients/tui && npm run validate"), "plain cd");
6768
assert.ok(runs("cd ./clients/tui && npm run validate"), "leading ./ (r15)");
@@ -87,6 +88,32 @@ test("rootRunsClientValidate: forms that count vs. don't", () => {
8788
assert.ok(!runs("cd clients/tui && npm run build"), "no validate");
8889
});
8990

91+
test("rootRunsClientValidate: only counts a reachable script (r5)", () => {
92+
// A cd-validate call in a script nothing reachable from `validate` runs must
93+
// NOT count — that's the whole reason the reachability restriction exists.
94+
assert.ok(
95+
!rootRunsClientValidate(
96+
{
97+
validate: "npm run something-else",
98+
orphan: "cd clients/tui && npm run validate",
99+
},
100+
"clients/tui",
101+
),
102+
"orphan script isn't reachable from validate",
103+
);
104+
// Reached via one hop of indirection counts.
105+
assert.ok(
106+
rootRunsClientValidate(
107+
{
108+
validate: "npm run validate:tui",
109+
"validate:tui": "cd clients/tui && npm run validate",
110+
},
111+
"clients/tui",
112+
),
113+
"reached via validate:tui",
114+
);
115+
});
116+
90117
test("rootReachesScript: sibling-guard vouching", () => {
91118
const scripts = {
92119
validate: "npm run verify:format-coverage && npm run validate:web",

‎scripts/verify-typecheck-coverage.mjs‎

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,14 +79,30 @@ export const isTsc = (t) => /(?:^|[\\/])tsc(?:\.(?:cmd|exe|ps1))?$/.test(t);
7979
// `typecheck`-script path and the reference (`tsc -b`) path.
8080
export const isDisablingFlag = (t) => /^--(noCheck|listFilesOnly)$/i.test(t);
8181

82+
/** A glob (with `**`/`*`) anchored to a RegExp over POSIX paths. Used to check a
83+
* tracked test file is matched by the `test:scripts` command's glob. */
84+
export function globToRegExp(glob) {
85+
let re = "";
86+
for (let i = 0; i < glob.length; i++) {
87+
const c = glob[i];
88+
if (c === "*") {
89+
if (glob[i + 1] === "*") {
90+
re += glob[i + 2] === "/" ? "(?:.*/)?" : ".*";
91+
i += glob[i + 2] === "/" ? 2 : 1;
92+
} else re += "[^/]*";
93+
} else if (".+^${}()|[]\\".includes(c)) re += "\\" + c;
94+
else re += c;
95+
}
96+
return new RegExp("^" + re + "$");
97+
}
98+
8299
/**
83100
* The `references` paths declared in the tsconfig at repo-relative `tsconfigRel`
84101
* (a `tsc -b` solution config), or `[]` if it has none / isn't readable. Paths
85102
* are as written (relative to that tsconfig's own directory).
86103
*/
87-
export function tsconfigReferences(tsconfigRel) {
104+
export function parseTsconfigReferences(raw) {
88105
try {
89-
const raw = readFileSync(path.join(repoRoot, tsconfigRel), "utf8");
90106
// Tolerate JSONC — block AND line comments + trailing commas (tsconfig
91107
// allows all; block comments are in fact the style of every other tsconfig
92108
// here). Block comments are stripped first so a `//` inside one doesn't
@@ -106,6 +122,16 @@ export function tsconfigReferences(tsconfigRel) {
106122
}
107123
}
108124

125+
export function tsconfigReferences(tsconfigRel) {
126+
try {
127+
return parseTsconfigReferences(
128+
readFileSync(path.join(repoRoot, tsconfigRel), "utf8"),
129+
);
130+
} catch {
131+
return []; // unreadable file (e.g. a directory / missing path)
132+
}
133+
}
134+
109135
/**
110136
* The `references` in a client's root `tsconfig.json`. Non-empty for a `tsc -b`
111137
* client (like `clients/web`, which has no `typecheck` script) — this guard
@@ -485,6 +511,36 @@ export function main() {
485511
);
486512
}
487513

514+
// Vouch for `test:scripts` (this guard's OWN parser tests) the same way the
515+
// guard vouches for a `tsc` pass: it must be run from `validate`, and its file
516+
// set must be non-empty and fully covered by the command's glob — so a rename
517+
// to `*.spec.mjs` / `*.test.mts` (which `node --test` silently skips, still
518+
// exiting 0) can't quietly shrink the suite to nothing.
519+
if (!rootReachesScript(rootPkg.scripts, "test:scripts")) {
520+
integrity.push(
521+
"the root `validate` no longer runs `test:scripts` — the guard's own parser tests run nowhere; restore it.",
522+
);
523+
} else {
524+
const testGlobs = tokenize(rootPkg.scripts["test:scripts"] ?? "").filter(
525+
(t) => !t.startsWith("-") && t !== "node",
526+
);
527+
const testFiles = execFileSync("git", ["ls-files", "scripts"], {
528+
cwd: repoRoot,
529+
encoding: "utf8",
530+
})
531+
.split("\n")
532+
.filter((f) => /\.(test|spec)\.[^/]+$/.test(f));
533+
if (testFiles.length === 0)
534+
integrity.push(
535+
"no `scripts/**/*.test.*` files are tracked — the guard's parser tests are gone.",
536+
);
537+
for (const f of testFiles)
538+
if (!testGlobs.some((g) => globToRegExp(g).test(f)))
539+
integrity.push(
540+
`${f}: not matched by the \`test:scripts\` glob — \`node --test\` won't run it (a rename to a form it skips). Rename it back to \`*.test.mjs\`.`,
541+
);
542+
}
543+
488544
for (const clientDir of CLIENTS) {
489545
checkingProjects.set(clientDir, []);
490546
if (!rootRunsClientValidate(rootPkg.scripts, clientDir)) {

0 commit comments

Comments
 (0)