test(cli,tui): typecheck the __tests__ dirs (#1791)#1799
Conversation
The cli/tui `typecheck` scripts only checked `src` (the src-only tsconfig excludes `**/*.test.*`), leaving the `__tests__` dirs — which do the most `as` casting — with no tsc pass. Add a `tsconfig.test.json` per client that includes `__tests__` with the test-exclusions dropped and the test-only path aliases (`@modelcontextprotocol/inspector-test-server`, the `@inspector/core/*` deep paths, express/vitest) resolving what vitest resolves, then fold `tsc -p tsconfig.test.json` into each `typecheck`. Fixes the 76 errors it surfaced: - cli: a wrong `@modelcontextprotocol/inspector-core/...` module specifier (should be `@inspector/core/...`; only a type import so vitest never caught it), `r.headers` optional-access guards, and partial OAuth mocks replaced with typed `makeFakeCliOAuthClient()` / `makeFakeServerSettings()` factories instead of `as` casts. Adds `@types/express` (devDep) so the transitively aliased test-server source typechecks, mirroring clients/web. - tui: rest-param signatures on the App OAuth spies (fixes the spread-arg and narrow-return errors), a widened callback param (`iss?`), a `makeFakeServer- Settings()` factory for the tuiOAuth tests, required OAuth-metadata/client fields in AuthTab, and a loosened `entry()` message param for the deliberately-malformed wire messages HistoryTab's defensive branches render. Gate coverage only; no runtime behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
@claude review |
|
Claude finished @cliffhall's task in 3m 30s —— View job Review:
|
…, nits Responds to the @claude review of #1799: - App.test.tsx: type the OAuth spies against the real InspectorClient method signatures (`vi.fn<InspectorClient["…"]>()`) and forward `Parameters<…>` tuples from the FakeClient wrappers, so `toHaveBeenCalledWith(...)` on the six OAuth-lifecycle spies stays argument-checked under the new gate rather than accepting anything via `unknown[]`. `getOAuthState` regains its wide `OAuthConnectionState | undefined` return. - Typecheck the client config files too: add `vitest.config.ts` / `tsup.config.ts` (and tui `dev.ts`) to each src `tsconfig.json` include — they were the one first-party surface still getting no tsc pass. - Trim the redundant `src/**/*` from each `tsconfig.test.json` include (tsc pulls in the src the tests import; the src-only config already checks it). - Rename the tui settings helper to `oauth-test-fakes.ts` to match cli's, and soften both factories' docstrings to stop naming a loader they don't mirror field-for-field. - Fix a stale `@modelcontextprotocol/inspector-core/...` specifier in a createRemoteTransport JSDoc example (the same wrong package name the gate caught in the cli tests, surviving in a comment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Thanks — all addressed in 373f8e9. Responses point by point: Main pushback — Sub-nit — Nit 1 — duplicated helper under two names. Renamed the tui helper to Nit 2 — docstring overstates parity. Both docstrings no longer name Nit 3 — Nit 4 — double Nit 5 — stderr now swallowed / predicate narrowed. Left as-is per your read — the Follow-up — config files untypechecked. Handled here rather than filed: Follow-up — stale JSDoc specifier. Fixed
|
|
@claude review |
|
Claude finished @cliffhall's task in 7m 10s —— View job Review:
|
| Check | Result |
|---|---|
clients/cli npm run typecheck (both projects) |
clean |
clients/tui npm run typecheck (both projects) |
clean |
Injected const __probe: number = "nope" into cli.test.ts + HistoryTab.test.tsx |
both caught (TS2322) — the gate bites |
clients/cli npm test |
25 files / 301 tests pass |
clients/tui npm test |
25 files / 282 tests pass |
format:check + lint, both clients |
clean |
The one substantive finding: toHaveBeenCalledWith is not argument-type-checked
The PR body says the reworked spies mean "toHaveBeenCalledWith(...) on all six lifecycle spies (incl. L1014 completeOAuthFlow, L1136 handleAuthChallenge) is now argument-checked", and clients/tui/__tests__/App.test.tsx:51-53 asserts the same in a comment. That isn't true under vitest 4.1.9, and it wasn't true before either — so the round‑1 concern ("under (...a: unknown[]) those accept literally any arguments") was describing a property those assertions never had.
I patched line 1141 to obviously-wrong arguments and re-ran the new gate:
expect(h.clientSpies.handleAuthChallenge).toHaveBeenCalledWith(123, 456);$ npx tsc --noEmit -p tsconfig.test.json
$ echo $? # 0 — no error
Isolating it against a directly-declared mock confirms it's a vitest typing property, not something about the vi.hoisted indirection:
const m = vi.fn<(a: string, b: number) => Promise<string>>();
m.mockResolvedValue(42); // TS2345 ✅ checked
await m(1, "wrong"); // TS2345 ✅ checked
expect(m).toHaveBeenCalledWith(123, 456); // no error ❌ not checkedSo the typed spies do buy something real — mockResolvedValue / mockImplementation payloads and direct calls through the Parameters<…> wrappers are now checked, which is how the stale { kind: "satisfied" }-literal problem gets fixed — but they buy nothing for the toHaveBeenCalledWith assertions. Keep the change; just fix the description, because a comment claiming compiler enforcement that doesn't exist is worse than no comment. Suggested rewording for App.test.tsx:50-56: the spies are typed against the real InspectorClient signatures so the mock implementations and return types stay in sync with the client (note that toHaveBeenCalledWith args are not type-checked by vitest — those assertions are runtime-only).
Nits
1. authenticate is now the one spy that lies about its return type — App.test.tsx:58-60. Every other spy is vi.fn<InspectorClient["…"]>; this one is () => Promise<string | undefined> while the real method returns Promise<URL | undefined>. The justification in the comment is about arguments ("it takes no arguments, so there is nothing to arg-check"), which doesn't explain the return divergence. It's harmless today — the only consumer is core/auth/node/runner-interactive-oauth.ts:115, which does nothing but authUrl !== undefined — but a fake returning a string where production returns a URL is precisely the drift this gate exists to catch, and vi.fn<InspectorClient["authenticate"]>(async () => new URL("https://auth.example/start")) costs one line. (Test refs at 602 / 1003 / 1053 would need the same one-word change.)
2. makeFakeCliOAuthClient's members are untyped vi.fn() — clients/cli/__tests__/helpers/oauth-test-fakes.ts:23-32. The docstring bills these as "typed mock factories", but the factory only enforces the shape (all CliOAuthClient keys present); each member is vi.fn(), so an override with a wrong signature is accepted. connect: vi.fn<CliOAuthClient["connect"]>() etc. would give the cli fakes the same implementation/return checking the tui spies now get.
3. entry()'s message?: unknown widens further than needed — clients/tui/__tests__/HistoryTab.test.tsx:36-37. Two malformed fixtures motivate it, but the widening also un-checks message on the ~15 well-formed entries in the file, so a methd: typo in a valid fixture would now sail through the gate this PR is adding. Casting at the two malformed call sites instead would keep the rest checked. Minor, and the added justification comment for the pre-existing as unknown as return cast satisfies the AGENTS.md double-cast rule either way.
4. "express" in types is redundant — clients/cli/tsconfig.test.json:9. I removed it and the project still typechecks clean: the paths entry is what makes the transitively-aliased test-server source resolve express; the types entry only pulls @types/express globals into every test file. It mirrors clients/web/tsconfig.test.json so consistency is a fine reason to keep it — just noting it's cargo, not load-bearing.
5. Round‑1 nits all confirmed addressed. Filenames now match (oauth-test-fakes.ts both sides), docstrings no longer name storedFieldsToInspectorSettings, getOAuthState is InspectorClient["getOAuthState"], src/**/* is out of both test-project includes (I confirmed all cli/tui test files live under __tests__, so ["__tests__/**/*"] is complete coverage), the config files are in each src project's include, and the stale JSDoc specifier is fixed.
Follow-up worth an issue
clients/launcher/__tests__ still has no typecheck — the same hole, in the one client this PR doesn't touch. clients/launcher/tsconfig.json is include: ["src/**/*"] with rootDir: "./src", and build is a plain tsc, so __tests__/parse-launcher-argv.test.ts gets no tsc pass. It can't just be added to the existing include (the rootDir would reject it), so it needs the same tsconfig.test.json + a typecheck script folded into validate. I probed it with a throwaway config: it already typechecks clean, so this is a zero-fix, gate-only change — cheap to land and it makes the "every client's __tests__ is typechecked" invariant actually complete.
· branch v2/1791-typecheck-tests
…ck launcher Responds to the round-2 @claude review of #1799: - Correct the toHaveBeenCalledWith claim: vitest doesn't type-check `toHaveBeenCalledWith(...)` arguments against the mock's signature, so the App.test.tsx comment no longer claims it does — the typed spies buy checked implementations/return payloads (the stale-literal fix), not arg-checking. - authenticate spy now uses the real `InspectorClient["authenticate"]` signature (returns a `URL`), so it no longer diverges from production; the one mockResolvedValue is updated to a URL. - makeFakeCliOAuthClient members are typed against the real CliOAuthClient signatures (not bare vi.fn()), so a wrong-signature override is rejected. - HistoryTab: revert the `entry()` `message: unknown` widening (it un-checked the ~15 well-formed fixtures) and instead scope a named `MALFORMED` cast to the two intentionally-malformed messages. - Typecheck the launcher `__tests__` too (the same gate hole in the one client the PR hadn't touched): new clients/launcher/tsconfig.test.json + a typecheck script folded into validate. Completes the "every client's __tests__ is typechecked" invariant. - Doc the new test-typecheck + config-file typecheck in AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Thanks for the careful re-verification — the Substantive — Nit 1 — Nit 2 — Nit 3 — Nit 4 — Follow-up — launcher
|
|
@claude review |
|
Claude finished @cliffhall's task in 6m 10s —— View job Review:
|
| Check | Result |
|---|---|
clients/cli npm run typecheck (both projects) |
clean |
clients/tui npm run typecheck (both projects) |
clean |
clients/launcher npm run typecheck (new) |
clean |
Probe: type error appended to launcher/__tests__/parse-launcher-argv.test.ts |
caught — TS2322 (the new launcher gate bites) |
Probe: handleAuthChallenge.mockResolvedValue({ kind: "totally-bogus" }) |
caught — TS2322: not assignable to '"failed" | "satisfied" | "interactive" | "step_up_confirm"' (nit 1's typed spies bite) |
Probe: makeFakeCliOAuthClient({ connect: vi.fn<() => number>() }) |
caught — TS2322: 'Mock<() => number>' is not assignable to '() => Promise<void>' (nit 2's typed fakes bite) |
Probe: methd: typo in a well-formed HistoryTab fixture |
caught — TS2353 (nit 3's re-narrowing works as claimed) |
cli / tui / launcher npm test |
301 / 282 / 5 pass |
format:check + lint, all three clients |
clean |
The round-2 correction is accurately reflected: the App.test.tsx:51-56 comment now scopes the benefit to implementations/return payloads and explicitly flags toHaveBeenCalledWith as runtime-only, and the same caveat is carried into AGENTS.md. Good — that's the durable half.
Nits
1. The launcher's vitest.config.ts is in neither project — the config-file half of the round-2 follow-up didn't reach it. clients/launcher/tsconfig.json is include: ["src/**/*"] and tsconfig.test.json is include: ["__tests__/**/*"]; I confirmed with --listFiles that vitest.config.ts appears in 0 files for both. It can't join the src project for the same rootDir: "./src" reason the tests can't — so it belongs in the new tsconfig.test.json. That matters because AGENTS.md:233 now asserts the config files "are folded into each src tsconfig.json's include, so they're typechecked as well", which is true for cli (2/2 reached) and tui (3/3 reached) but not launcher. I probed adding "vitest.config.ts" to clients/launcher/tsconfig.test.json's include: typechecks clean, zero source fixes — same shape as the launcher __tests__ gap you just closed in this commit.
2. callTool is the one un-typed member of clientSpies, which the block comment above it now contradicts. clients/tui/__tests__/App.test.tsx:51 says "Each spy is typed against the real InspectorClient method signature", but line 63 is a bare callTool: vi.fn(). This is the same comment-vs-reality class round 2 was about, just inverted. And unlike authenticate, it isn't hard to fix — I probed the two-line change and it's clean:
callTool: vi.fn<InspectorClient["callTool"]>(), // L63
callTool = (...a: Parameters<InspectorClient["callTool"]>) => // L148
clientSpies.callTool(...a);$ npx tsc --noEmit -p tsconfig.test.json # exit 0, no other edits needed
Worth doing rather than weakening the comment: callTool is configured with mockResolvedValue({ result: … }) payloads in this file, which is precisely the return-type drift the typed spies exist to catch.
3. clearOAuthTokens has a type that promises more than the mock delivers — App.test.tsx:61. vi.fn<InspectorClient["clearOAuthTokens"]>() with no implementation returns undefined at runtime while its declared type is Promise<void>. Harmless today (clients/tui/src/App.tsx:890 only awaits it, and await undefined is fine), but adding a .catch() on the production side would TypeError inside the test rather than fail an assertion. vi.fn<…>(async () => {}) — matching how completeOAuthFlow is written two lines below — closes it.
4. The new AGENTS.md paragraph describes cli's alias set as if it were all three. "…adding the test-only path aliases that resolve what vitest resolves via vitest.shared.mts (@modelcontextprotocol/inspector-test-server → test-servers/src, the @inspector/core/* deep paths, express/vitest)" — tui's config has no test-server/express alias, and launcher's has no paths at all (it's a rootDir sibling, which its own file comment states correctly). Doc-accuracy only; a "(cli's set is the widest; the launcher's is a plain rootDir sibling with no aliases)" clause would cover it.
5. Verified-addressed from round 2, for the record. toHaveBeenCalledWith claim corrected in both the code comment and AGENTS.md; authenticate is now vi.fn<InspectorClient["authenticate"]>(async () => new URL(…)) with the beforeEach mockResolvedValue switched to a URL (and I confirmed all three getOAuthState/clearOAuthTokens/authenticate are genuinely zero-arg in core/mcp/inspectorClient.ts:4625/4882/4913, so the () => wrappers are exact, not lossy); makeFakeCliOAuthClient members are individually typed; the MALFORMED(...) helper scopes the double cast to its two real call sites with a justification comment satisfying the AGENTS.md as unknown as rule; launcher tsconfig.test.json + typecheck folded into validate. The makeFakeServerSettings docstring's "0 = SDK default" is accurate — it matches the loader's own comment at core/mcp/serverList.ts:216.
Nothing here blocks. The invariant this PR set out to establish — every client's __tests__ gets a tsc pass — is real and I confirmed it bites in all three clients; nit 1 is the last small hole in its sibling invariant for config files.
· branch v2/1791-typecheck-tests
…d, docs Responds to the round-3 @claude review of #1799 (all non-blocking): - launcher: add `vitest.config.ts` to tsconfig.test.json's include (it was in neither project — the build config's rootDir: ./src rejects it). Now typechecked. - App.test.tsx: type `callTool` against `InspectorClient["callTool"]` (+ the Parameters<…> wrapper) so its `mockResolvedValue` payloads are checked, and give `clearOAuthTokens` an `async () => {}` impl so the mock actually returns the `Promise<void>` its type promises. - AGENTS.md: correct the per-client alias-set description (cli's is widest, tui's is core+react only, launcher's has no paths) and note launcher's config file lives in its test project, not the src project. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
All four addressed in f520947: Nit 1 — launcher Nit 2 — Nit 3 — Nit 4 — AGENTS.md described cli's alias set as if all three shared it. Rewritten to spell out the per-client difference (cli's is the widest with the test-server/express aliases; tui's is core + react/vitest only; launcher's has no
|
|
@claude review |
|
Claude finished @cliffhall's task in 6m 34s —— View job Review:
|
| Check | Result |
|---|---|
clients/cli npm run typecheck (both projects) |
clean |
clients/tui npm run typecheck (both projects) |
clean |
clients/launcher npm run typecheck |
clean |
--listFiles sweep: every tracked .ts/.tsx in cli/tui/launcher vs. the union of both projects |
0 unreached ✅ |
Same sweep on clients/web (out of scope, for reference) |
only vitest.shims.d.ts (an ambient shim — not a real gap) |
Probe: type error in launcher/vitest.config.ts |
caught — TS2322 (nit 1's fix bites) |
Probe: callTool.mockResolvedValue({ bogus: 1 }) |
caught — TS2353: 'bogus' does not exist in type 'ToolCallInvocation' (nit 2's fix bites) |
cli / tui / launcher npm test |
301 / 282 / 5 pass |
format:check + lint, all three clients |
clean |
root validate:core, verify:format-coverage |
clean (915 files gated) |
Nits
1. AGENTS.md still claims a uniform validate ordering that launcher doesn't match. The paragraph this PR edits says "validate:web/validate:tui/validate:launcher are the uniform format:check && lint && (typecheck &&) build && test". Actual:
tui: format:check && lint && typecheck && build && test ✅ matches
launcher: format:check && lint && build && typecheck && test ❌ typecheck after build
Functionally identical, but this PR is the one that added launcher's typecheck and rewrote that sentence, so it's the moment to line them up. Moving typecheck ahead of build in clients/launcher/package.json is the one-token fix and matches tui.
2. clients/launcher's typecheck checks less than the identically-named script in cli/tui. It's tsc --noEmit -p tsconfig.test.json only. The rationale in AGENTS.md is sound within validate (build is a plain tsc over src), but a developer running npm run typecheck directly in clients/launcher — the obvious inner-loop move, and exactly what cli/tui train you to do — silently gets no src pass. Making it self-contained costs nothing: I ran npx tsc --noEmit -p tsconfig.json in launcher and it exits 0, so "typecheck": "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.test.json" is a zero-fix change that makes all three clients' typecheck mean the same thing.
3. makeFakeCliOAuthClient's typing stops at the defaults — it doesn't reach the overrides, which is what each test actually drives. The factory's members are now individually typed (good, and I confirmed a typed wrong mockResolvedValue is rejected: TS2345 'string' is not assignable to 'boolean'). But overrides: Partial<CliOAuthClient> accepts a bare vi.fn(), whose type is Mock<(...args: any[]) => any> and therefore assignable to anything. All nine call sites (cliOAuth.test.ts:603, 632, 659, 684, 702, 724, 753, 776, 1101) pass bare vi.fn()s. Verified:
makeFakeCliOAuthClient({
connect,
checkAuthChallengeSatisfied: vi.fn().mockResolvedValue("definitely-not-a-boolean"),
})$ npx tsc --noEmit -p tsconfig.test.json # exit 0 — not caught
So the exact drift the factory is billed as catching still gets through on the members tests configure. Not a regression (it's strictly better than the pre-PR partial object literals) and not worth churning nine call sites over — but the round-3 exchange implied stronger coverage than exists, so it's worth a one-line note on the factory's comment rather than leaving a reader to assume overrides are checked.
4. Round-3's stated motivation for typing callTool doesn't match the file. That review said callTool "is configured with mockResolvedValue({ result: … }) payloads in this file". It isn't — the only configurations are three mockRejectedValues (App.test.tsx:1100, 1128, 1262). The change is still correct and I confirmed it now bites, but it's future-proofing plus the Parameters<…> wrapper, not the closing of an existing hole. Nothing to do; noting so the record's accurate.
5. Round-3 items, all confirmed addressed. launcher vitest.config.ts is in the test project (verified via --listFiles) and a type error in it now fails; callTool is vi.fn<InspectorClient["callTool"]>() with the Parameters<…> wrapper; clearOAuthTokens has an async () => {} impl so it actually returns the Promise<void> its type promises; the AGENTS.md alias-set paragraph now correctly distinguishes cli's (widest) / tui's (core + react/vitest) / launcher's (no paths, a rootDir sibling) and notes launcher's config file lives in the test project. I also re-confirmed cli's and tui's config files are reached by their src projects (dev.ts, vitest.config.ts, tsup.config.ts — 3/3 and 2/2).
Follow-up worth an issue (out of scope)
The typecheck-coverage invariant has no durable guard. Round 3 found launcher/__tests__ unreached; round 4 found launcher/vitest.config.ts unreached. Both were caught by hand-running tsc --listFiles and diffing — the same question verify:format-coverage (#1792) was built to answer for prettier, and the same failure mode it was built to prevent. I ran the sweep and it's complete today, but nothing stops the next new top-level .ts from silently falling out (launcher is especially exposed: its rootDir: "./src" actively rejects anything at the package root, so a new config file there is unreached by default). A verify:typecheck-coverage sibling — enumerate tracked .ts/.tsx per client, union each project's --listFiles, fail on the difference — would make this PR's invariant self-enforcing instead of convention-enforced. I'd file it against the same #1791/#1792 cluster.
The invariant this PR set out to establish is real, complete, and I verified it bites in all three clients. Nothing here blocks.
· branch v2/1791-typecheck-tests
…oc nits Responds to the round-4 @claude review of #1799 (all non-blocking): - Add `verify:typecheck-coverage` (scripts/verify-typecheck-coverage.mjs), the typecheck-coverage analog of verify:format-coverage and the durable guard for the invariant this PR establishes: for each Node client it runs the tsconfig projects its `typecheck` names with `tsc --listFiles`, unions them, and fails on any tracked .ts/.tsx that lands in no project. This closes the class the review kept surfacing by hand (launcher/__tests__ in round 3, launcher/vitest.config.ts in round 4) — a new top-level file can no longer go untypechecked silently. Wired into `npm run ci` and the GitHub workflow. - launcher: reorder `validate` to run `typecheck` before `build` (matches tui and the documented order), and make `typecheck` self-contained — it now runs both `tsconfig.json` and `tsconfig.test.json`, so running it standalone means the same thing it does in cli/tui. - makeFakeCliOAuthClient: note on the comment that the typing covers the defaults, not `overrides` (a bare vi.fn() override is Mock<(...args)=>any>). - Docs: README table + AGENTS.md updated for the new guard and the corrected launcher typecheck/validate ordering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Thanks — all four addressed, and I built the follow-up guard here rather than filing it. In 2d770a5: Follow-up — no durable guard for the typecheck-coverage invariant. Built it: Nit 1 — launcher Nit 2 — launcher Nit 3 — factory typing stops at the defaults, not the overrides. Added a note to Nit 4 — round-3's
|
|
@claude review |
|
Claude finished @cliffhall's task in 7m 3s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 112 files / 3 clients, 23s |
Probe: tracked clients/launcher/probe-uncovered.ts at the package root |
caught — exit 1, file named ✅ |
Probe: type error injected into launcher/src/index.ts |
guard still reports OK — the documented error-swallowing works, a broken project doesn't mask a coverage gap ✅ |
Probe: npm run typecheck removed from launcher's validate |
guard still reports OK ❌ — finding 1 |
clients/launcher typecheck + test |
clean / 5 pass |
clients/tui typecheck + test |
clean / 282 pass |
clients/cli typecheck + lint |
clean |
npm run format:check:scripts |
clean |
npx prettier --check package.json |
warn — finding 2 |
1. The guard doesn't inherit verify:format-coverage's reachability check — so it can pass while nothing is typechecked
verify-format-coverage.mjs goes out of its way to only trust a glob from a script validate actually reaches, and says why (scripts/verify-format-coverage.mjs:63-68):
"a
prettier --checkscript that nothing invokes fromvalidatedoesn't gate anything, and counting its globs would let the gate be silently unwired"
…plus a second check (clientsUnreachedFromRoot, L150-172) that the root chain still invokes each client's validate. The new guard has neither: typecheckProjects() reads pkg.scripts.typecheck directly (scripts/verify-typecheck-coverage.mjs:43-51) and runs it itself, never asking whether anything runs it. Verified:
-"validate": "... && npm run typecheck && npm run build && npm run test"
+"validate": "... && npm run build && npm run test"$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — OK: all 112 tracked source files across 3 clients get a tsc pass. # exit 0
So after that one-line refactor, cli/tui/launcher get zero tsc pass in validate or CI and both coverage guards stay green — the guard is measuring a script that no longer gates anything. It's the same "gate silently stops gating" hole, one layer up, and the fix is small: reuse the reachableScripts(scripts, "validate") idea (assert typecheck is reachable from each client's validate) and the root-chain check (validate:<client> still present). Given the PR bills this as "the typecheck-coverage analog of verify:format-coverage", matching that property is what makes the analogy hold.
2. The root package.json edit is misindented, and no gate catches it
package.json:43 — the new entry is 2-space indented while every sibling is 4:
"verify:build-gate": "node scripts/verify-build-gate.mjs",
"verify:typecheck-coverage": "node scripts/verify-typecheck-coverage.mjs",
"validate": "npm run verify:format-coverage && ...",npx prettier --check package.json flags it, but nothing in the repo runs that: every format glob is scoped to source extensions ({ts,tsx,mts,cts,js,jsx,mjs,cjs}), so .json is outside format:check:scripts and outside verify:format-coverage's SOURCE_EXTENSIONS. A PR whose subject is "make the gate reach the files it claims to" is a slightly ironic place for it. One-token fix; whether to also widen the format globs to manifests is a separate call (I'd leave it).
Nits
3. --listFilesOnly is 4× faster with a byte-identical file list. The guard currently runs a full type check per project purely to harvest the file list — work validate already did moments earlier. tsc --listFilesOnly skips checking:
$ time npx tsc --noEmit -p tsconfig.test.json --listFiles # 5.7s (current)
$ time npx tsc -p tsconfig.test.json --listFilesOnly # 1.5s
$ diff <(sort a) <(sort b) # identical
That takes the whole guard from ~23s to ~6s, which also makes it cheap enough to move into validate alongside verify:format-coverage rather than living only in ci (its sibling runs in the fast loop; this one doesn't). The catch at L67-72 can stay as belt-and-braces, but its comment about type errors would need a tweak since --listFilesOnly doesn't type-check.
4. npx tsc without --no-install. verify-build-gate.mjs:240 uses spawnSync("npx", ["--no-install", "vite", "build"]) precisely so a missing local binary fails fast instead of npx reaching for the registry. Same reasoning applies here — worth matching for consistency.
5. projectFiles's JSDoc overstates what it filters. scripts/verify-typecheck-coverage.mjs:53-57 says the result is "restricted to those under clientDir", but the body (L77-79) only drops paths outside the repo root and node_modules — so the aliased core/ and test-servers/ files stay in the set. Harmless (the set is only ever queried with client-relative paths) but the sentence describes a filter that isn't there.
6. CLIENTS is a hardcoded list (L35). A future Node client under clients/ is silently unguarded until someone remembers this file. verify-format-coverage.mjs has the same shape (MANIFESTS), so it's consistent — but both could be derived from the root validate chain, which would also give finding 1 for free.
7. Round-4 items, all confirmed addressed. launcher validate is now format:check && lint && typecheck && build && test (matches tui and the doc); launcher typecheck runs both projects and is clean standalone; the makeFakeCliOAuthClient comment now states plainly that the typing covers the defaults and not overrides (a bare vi.fn() is Mock<(...args) => any>); README table + AGENTS.md updated for the new guard and the corrected ordering. The round-3 callTool motivation point needed no code change, as noted.
The core invariant this PR set out to establish — every tracked .ts/.tsx in cli, tui, and launcher gets a tsc pass — is real, and it now has a guard that I confirmed fails when it's violated. Finding 1 is about the guard's own durability, not the invariant.
· branch v2/1791-typecheck-tests
…alidate Responds to the round-5 @claude review of #1799: - Finding 1 (the pre-merge one): the guard now ports verify:format-coverage's reachability checks — it asserts each client's `typecheck` is reachable from its own `validate` and that the root chain runs each client's `validate`, so it can't stay green while measuring a typecheck nothing invokes. Verified: stripping `npm run typecheck` from launcher's validate now fails the guard. - Nit 3: switch `tsc --listFiles` → `tsc --listFilesOnly` (identical file list, ~4× faster: whole guard 23s → ~4s). Now cheap enough for the inner loop, so it moves into `validate` (right after verify:format-coverage) instead of a standalone ci step — matching its sibling. Removed the separate ci-chain entry and GitHub workflow step (validate covers it). - Nit 4: `npx --no-install tsc` so a missing local tsc fails fast instead of reaching for the registry (matches verify-build-gate.mjs). - Nit 5: corrected projectFiles' JSDoc to describe the filter that's actually there (drops repo-external + node_modules paths; core/ + test-servers/ stay but are harmless). - Finding 2: fixed the 2-space misindent on the new root package.json line (prettier --write). Docs (README/AGENTS) updated for the validate placement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
All addressed in 718c830: Finding 1 — guard could pass while nothing is typechecked. Ported both reachability checks from Nit 3 — Nit 4 — Nit 5 — Nit 6 — hardcoded Finding 2 — misindented root
|
|
@claude review |
|
Claude finished @cliffhall's task in 6m 10s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 112 files / 3 clients, 8.0s |
Probe: strip npm run typecheck && from launcher's validate |
caught — "typecheck is not reachable from its validate — the typecheck it measures gates nothing" ✅ |
Probe: drop validate:tui from the root chain |
caught — "the root validate chain no longer runs cd clients/tui && npm run validate" ✅ (the second check fires independently) |
Probe: tracked clients/launcher/probe-uncovered.ts at the package root |
caught — exit 1, file named ✅ |
Probe: drop -p tsconfig.test.json from tui's typecheck |
caught — all 29 tui test files listed ✅ |
Probe: delete clients/launcher/tsconfig.test.json |
caught, but misattributed — see nit 3 |
cli / tui / launcher typecheck (both projects each) |
clean |
cli / tui / launcher npm test |
301 / 282 / 5 pass |
format:check + lint, all three clients |
clean |
npm run verify:format-coverage |
OK — 916 files |
npx prettier --check package.json |
clean (finding 2 fixed) |
Round-5 items confirmed: reachability ported and both halves work; --listFilesOnly in place with the catch-block comment corrected to config-errors; npx --no-install tsc; projectFiles JSDoc now matches the body; root package.json reindented; README/AGENTS updated for the validate placement and the removed standalone ci/workflow step.
Nits
1. The guard reads pkg.scripts.typecheck literally, so a legitimate refactor fails it with a wrong diagnosis. typecheckProjects() (scripts/verify-typecheck-coverage.mjs:73-78) regexes -p <file> out of one script string. Its sibling deliberately harvests globs from every script reachable from validate (verify-format-coverage.mjs:91-111) precisely so a chained/delegating script still counts. Verified the divergence:
"typecheck": "npm run typecheck:src && npm run typecheck:test",
"typecheck:src": "tsc --noEmit -p tsconfig.json",
"typecheck:test": "tsc --noEmit -p tsconfig.test.json",verify:typecheck-coverage — 1 issue(s): tracked source files that get no `tsc` pass:
clients/tui: its `typecheck` script names no `-p <project>` — nothing is typechecked.
Fail-safe, so not a correctness bug — but the message asserts something false (both projects are typechecked) and the remediation text below it points at tsconfig include, which is the wrong fix. Collecting -p from reachableScripts(scripts, "typecheck") instead of the single string costs two lines and makes the two guards behave the same way. Same block: only -p is matched, not --project.
2. reachableScripts is now duplicated verbatim across the two verify scripts — verify-typecheck-coverage.mjs:57-70 vs verify-format-coverage.mjs:70-83 (identical body, near-identical docstring), and clientsUnreachedFromRoot / wiringFailures share the same cd <dir> && npm run validate matcher. scripts/lib/ already exists (prod-web-server.mjs) as precedent, and the stated reason for it there — "so the two can't drift" — applies exactly. A scripts/lib/npm-scripts.mjs exporting reachableScripts + rootRunsClientValidate would deduplicate the wiring logic both guards now depend on.
3. A broken tsconfig degrades into a misleading coverage report. projectFiles's catch (:95-101) keeps stdout and discards err.stderr entirely. I deleted clients/launcher/tsconfig.test.json and got:
verify:typecheck-coverage — 1 issue(s): tracked source files that get no `tsc` pass:
clients/launcher/__tests__/parse-launcher-argv.test.ts — in no tsconfig project
clients/launcher/vitest.config.ts — in no tsconfig project
Directionally right (those files genuinely lost their pass) but it never says why — the actual error TS5083: Cannot read file …tsconfig.test.json went to a discarded buffer. Since this guard now runs second in validate, before any client's own typecheck has had a chance to report the config error itself, it's the first thing a developer sees. Echoing err.stderr as a console.warn diagnostic when the exec fails would make it self-explaining.
4. The CI workflow's Validate step name and comment are now stale. .github/workflows/main.yml:31-37 — "Validate (format, lint, build, fast tests)" / "Each client self-validates: format:check + lint + build + test". As of this PR validate also runs verify:format-coverage, verify:typecheck-coverage, validate:core, and per-client typecheck. Pre-existing drift that this PR widens (it's the commit that both added launcher's typecheck and folded the new guard into validate), and it's the step a reader consults to learn what CI enforces. Comment-only; I can't edit .github/workflows/ myself.
5. isRequiredSource's .d.ts exclusion is currently speculative — :47-48. There are zero tracked .d.ts under cli/tui/launcher today (git ls-files confirms), so the carve-out only pre-empts a web-style vitest.shims.d.ts. Fine to keep — just noting the comment reads as if it were guarding an existing case.
The invariant is real, the guard now enforces both halves of it (coverage and wiring), and I confirmed each half fails when violated. The nits are about the guard's own maintainability, not the gate it protects.
· branch v2/1791-typecheck-tests
…agnostics Responds to the round-6 @claude review of #1799 (all guard-ergonomics nits): - Nit 2: extract the shared reachability logic (`reachableScripts`, `rootRunsClientValidate`) into scripts/lib/npm-scripts.mjs and consume it from BOTH verify-format-coverage.mjs and verify-typecheck-coverage.mjs, so the "is this gate actually run?" wiring can't drift between them (same rationale as scripts/lib/prod-web-server.mjs). - Nit 1: typecheckProjects now harvests `-p`/`--project` from every script reachable from `typecheck` (not the single string), so a delegating `typecheck` (`npm run typecheck:src && …`) is handled instead of failing with a false "names no -p project" diagnosis. Also accepts `--project`. - Nit 3: on a tsc config error, echo the `error TS…` diagnostic (scanning both streams — tsc prints config errors to stdout) so a broken tsconfig is self-explaining rather than surfacing only as a misleading "file in no project" report. - Nit 4: refreshed the CI workflow's stale "Validate" step name/comment (it now runs the coverage guards + validate:core + per-client typecheck). - Nit 5: reworded the `.d.ts` carve-out comment to read as pre-emptive (there are none under cli/tui/launcher today). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
All six addressed in 464ae40: Nit 2 — Nit 1 — literal Nit 3 — broken tsconfig gave a misleading report. The catch now echoes the Nit 4 — stale CI Nit 5 — Nit 6 — hardcoded
|
|
@claude review |
|
Claude finished @cliffhall's task in 7m 12s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 112 files / 3 clients, 8.3s |
npm run verify:format-coverage |
OK — 917 files |
Probe: strip npm run typecheck && from launcher's validate |
caught — "typecheck is not reachable from its validate" ✅ (survives the lib extraction) |
Probe: delegating typecheck → typecheck:src && typecheck:test, one using --project |
OK, exit 0 ✅ — round-6 nit 1 fixed; the old code failed this with a false "names no -p" |
Probe: delete clients/launcher/tsconfig.test.json |
caught, and now self-explaining — echoes error TS5058: The specified path does not exist: 'tsconfig.test.json'. above the report ✅ (nit 3 fixed) |
Probe: tracked clients/launcher/probe.mts with a type error |
not caught ❌ — finding 1 |
cli / tui / launcher typecheck (both projects each) |
clean |
cli / tui / launcher npm test |
301 / 282 / 5 pass |
lint + format:check:scripts, all clients |
clean |
1. The guard requires only .ts/.tsx — a client-level .mts/.cts is silently unguarded
trackedSourceFiles asks git for *.ts *.tsx (scripts/verify-typecheck-coverage.mjs:121) and isRequiredSource re-filters to the same two (:52-53). Its sibling enumerates ["ts","tsx","mts","cts","js",…] (verify-format-coverage.mjs:33-38), so the two guards disagree about what "a source file" is. Verified:
$ printf 'export const x: number = "not a number";\n' > clients/launcher/probe.mts
$ git add -N clients/launcher/probe.mts
$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — OK: all 112 tracked source files across 3 clients get a tsc pass. # exit 0
The file is tracked, is TypeScript, gets no tsc pass, and the count doesn't even move (112 → 112 — it isn't merely unreported, it's outside the required set).
This isn't hypothetical for this repo: .mts is already the idiom for shared config here (vitest.shared.mts at the root), and #1792 deliberately put mts/cts in every client format glob for exactly this reason. The concrete path is a rename: clients/tui/vitest.config.ts → .mts drops it from the tsconfig include (which names the filename literally) and out of the guard's required set — the same two-part hole rounds 3 and 4 each found by hand, now reachable via a one-line rename. Worth noting the current setup is otherwise solid here: I confirmed vitest.shared.mts is typechecked today, transitively, because tui's src project includes vitest.config.ts which imports it.
Fix is two tokens and provably zero-risk — there are no tracked .mts/.cts under cli/tui/launcher today (git ls-files confirms), so the required set is unchanged and the guard stays green.
Nits
2. rootReachedCommands is exported but nothing imports it — scripts/lib/npm-scripts.mjs:30. Both guards import only reachableScripts and rootRunsClientValidate; the third export is used solely by its own module neighbour on L43. Nothing lints scripts/**/*.mjs (the scripts/ gate is prettier-only, per AGENTS.md), so no unused-export warning will ever surface it. Drop the export keyword, or leave it as a deliberate part of the lib's API — either is fine, just noting it's currently dead surface on a brand-new file.
3. The guard can silently measure a different tsc than the client's own typecheck uses. projectFiles runs npx --no-install tsc with cwd = the client dir (:83-87), and npx resolves up the tree. I removed clients/launcher/node_modules/.bin/tsc and the guard still reported OK — it found an ancestor's tsc rather than failing. Harmless today (root and all clients are on typescript 5.9.3), and --no-install correctly prevents a registry fetch, so this is a footnote rather than a fix request; it just means "the guard passed" doesn't strictly imply "the client's own toolchain resolves".
4. callTool is the one spy left with a type it doesn't honour at runtime — clients/tui/__tests__/App.test.tsx:63. vi.fn<InspectorClient["callTool"]>() with no implementation returns undefined while declaring Promise<ToolCallInvocation> — structurally the same thing round 3 flagged on clearOAuthTokens, which got async () => {} in round 4. Unlike that case the fix isn't cheap (a default would mean constructing a whole ToolCallInvocation), and the three configured cases are all mockRejectedValue, so I'd leave it. Flagging only so the asymmetry is a recorded decision rather than an oversight.
5. Round-6 items, all confirmed addressed. reachableScripts + rootRunsClientValidate extracted to scripts/lib/npm-scripts.mjs with both guards importing them (and the format guard's clientsUnreachedFromRoot reduced to a one-line delegation — same predicate, verified by re-running it); typecheckProjects harvests -p/--project across reachableScripts(scripts, "typecheck") and I confirmed a delegating script now passes; the broken-tsconfig path echoes the error TS… diagnostic from both streams; the CI Validate step name and comment now list the guards, validate:core, and per-client typecheck; the .d.ts comment is reworded as pre-emptive; CLIENTS kept explicit as agreed. (Sidebar on the --project=<file> form: I checked, tsc rejects it outright — error TS5023 — so the regex not matching it is correct, not a gap.)
The invariant, its wiring check, and now its diagnostics all hold up under probing. Finding 1 is about the guard's own definition of "source file" drifting from its sibling's — the last asymmetry between the two, and a no-op to close.
· branch v2/1791-typecheck-tests
Responds to the round-7 @claude review of #1799: - Finding 1: verify-typecheck-coverage now requires a tsc pass for tracked `.mts`/`.cts` (not just `.ts`/`.tsx`), matching verify-format-coverage's extension set — `.mts` is already the shared-config idiom here, so a client `vitest.config.ts` → `.mts` rename can no longer drop out of both the tsconfig include and the guard's required set. No-op on today's tree (no client `.mts`/`.cts`); confirmed a tracked `.mts` with a type error is now caught. Ambient `.d.{ts,mts,cts}` stay excluded. - Nit 2: drop the unused `export` on `rootReachedCommands` (internal to the lib; nothing imports it, and scripts/ isn't eslint-gated to catch dead exports). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Both addressed in 73bd043: Finding 1 — Nit 2 — dead Nit 3 (ancestor
|
|
Addressed in d700c08: Finding 1 — Nit 2 — Nit 3 — "shared source" message for non-shared paths / string-sniffing. The non-client miss is now the general "in no client's tsconfig project"; the Nit 4 — success line. Now "(N clients + non-client)".
|
|
@claude review |
|
Claude finished @cliffhall's task in 7m 37s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 127 files (3 clients + non-client), 10.2s, both exemptions named |
npm run verify:format-coverage |
OK — 917 files |
Probe: core/mcp/probe-widget.tsx |
caught ✅ — round-22 finding 1 fixed (was green) |
Probe: core/mcp/__tests__/probe.test.ts |
false failure ❌ — web's tsc -b reports TS2322 on it — finding 1 |
Probe: core/mcp/__tests__/probe-widget.tsx |
false failure ❌ — same, web catches it — finding 1 |
Probe: second EXEMPT_ROOTS entry (specification) |
announced in the OK line, not applied — finding 2 |
Probe: test-servers/src/server-orphan-bin.ts |
caught, with the test-servers clause ✅ (round-22 nit 3 fixed) |
Probe: tools/probe.ts (new top-level location) |
caught, without the test-servers clause ✅ (deny-by-default holds) |
cli / tui / launcher npm run typecheck (both projects each) |
clean |
tui / launcher npm test |
282 / 5 pass |
1. isExemptCoreFile mirrors only web's app project — a core/**/__tests__/*.test.ts or *.tsx is checked by tsc -b but flagged anyway
The comment at scripts/verify-typecheck-coverage.mjs:292-296 says the predicate models what web includes:
A
core/file web's app project (clients/web/tsconfig.app.json) includes:../../core/**/*.tsand NOT the excluded co-located*.test.ts/*.test.tsx.
Accurate for that one project — but tsc -b builds four, and clients/web/tsconfig.test.json:40-41 adds:
"include": [ …, "../../core/**/__tests__/**/*.ts", "../../core/**/__tests__/**/*.tsx" ]So web's real core/ coverage is the union, and the guard's exemption is a strict subset of it. Both shapes verified end to end:
$ printf 'export const bad: number = "definitely not a number";\n' > core/mcp/__tests__/probe.test.ts && git add -N …
$ (cd clients/web && npx --no-install tsc -b)
../../core/mcp/__tests__/probe.test.ts(1,14): error TS2322: Type 'string' is not assignable to type 'number'. # web DOES check it
$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — 1 tracked source file(s) get no `tsc` pass:
core/mcp/__tests__/probe.test.ts — in no client's tsconfig project
…
For a co-located test, instead move it to `__tests__/` — adding it to the src `include` would make the build emit it.
Same for core/mcp/__tests__/probe-widget.tsx (web reports it; guard flags it). Note the remediation compounds it — the advice is "move it to __tests__/" for a file already in __tests__/, and the alternative it offers (add it to a client tsconfig.test.json) would make a cli/tui project typecheck a core test it has no business owning.
Fail-safe, so no gate hole — but it's a false failure on the one core/ test layout web explicitly supports, and that layout is live: core/mcp/__tests__/fakeInspectorClient.ts already sits there (it's exempt only by the .ts branch, incidentally). A sibling foo.test.ts beside it is the obvious next file. Zero-count fix — git ls-files finds 0 tracked core/**/__tests__/*.test.* and 0 core/**/__tests__/*.tsx today:
const isExemptCoreFile = (f) => {
if (!f.startsWith("core/")) return false;
// web's tsconfig.test.json: ../../core/**/__tests__/**/*.{ts,tsx}
if (f.includes("/__tests__/")) return /\.tsx?$/.test(f);
// web's tsconfig.app.json: ../../core/**/*.ts minus co-located *.test.ts(x)
return /\.ts$/.test(f) && !/\.test\.tsx?$/.test(f);
};2. EXEMPT_ROOTS no longer drives anything — it announces exemptions the filter doesn't apply
Round 22 promoted EXEMPT_ROOTS to a Map (dir → reason) to surface the reason at runtime, and simultaneously replaced the filter it fed (EXEMPT_ROOTS.some(...)) with the hardcoded isExemptCoreFile. The Map's keys are now read by nothing — only its values, for the OK line. So it presents as the exemption registry ("Roots this guard defers to another gate") while being pure documentation. Verified:
const EXEMPT_ROOTS = new Map([
["specification", "prose only, no TS"], // added
["core", …],
]);verify:typecheck-coverage — 1 tracked source file(s) get no `tsc` pass:
specification/probe/x.ts — in no client's tsconfig project # announced-but-not-exempt
Fail-safe (a phantom exemption doesn't create a hole), and low-likelihood — but it's now two definitions of "what core/ means", and unlike EXEMPT, which round 17 gave a presence assertion precisely so "the exemption can't outlive its subject", this one can't usefully get that check because it isn't the subject of anything. Simplest resolution is to drop it to a const CORE_EXEMPT_REASON = "…" string next to isExemptCoreFile (one definition, same OK line), or make the predicate genuinely dispatch off the Map.
Nits
3. Two docs still describe the pre-round-21/22 scope. README.md:149 says the guard covers "any new non-client, non-core/ location" — after round 22 a core/*.tsx, core/*.mts, or co-located core test is required, so core/ is no longer wholesale excluded. AGENTS.md:235's closing sentence still describes the non-client half as just "test-servers/src/** and the root vitest.shared.mts", predating round 21's deny-by-default widening (which is what makes tools/probe.ts fail). Understating, as in earlier rounds — but the README table is the one-stop scope description and these are the two rounds that changed it.
4. nonClientMisses is populated but only ever tested with startsWith("test-servers/"). Correct and it's the fix I asked for last round (no more string-sniffing the composed message) — noting that the array is otherwise unused, so if a third remediation clause ever lands it should dispatch off the same array rather than re-scanning failures.
5. Round-22 items, all four confirmed. isExemptCoreFile narrows the exemption and a core/*.tsx is now flagged (finding 1 is about the other edge of the same predicate, not a regression in this fix); EXEMPT_ROOTS's reason is surfaced in the OK line alongside the client exemption (see finding 2 for its side effect); the non-client miss message is the general "in no client's tsconfig project" and the test-servers clause is conditional on an actual test-servers/ path — I confirmed it fires for test-servers/src/server-orphan-bin.ts and stays quiet for tools/probe.ts; the success line reads "(3 clients + non-client)". Everything from the earlier rounds still holds under re-probing: enroll-or-exempt enumeration, the anchored wiring match on both dir and script tail, pre/post hook following, --noCheck rejection in command and config forms, basename tsc matching, the shared quote-aware tokenizer, .mts/.cts in the required set, and the **/*.test.* excludes keeping a compiled test out of the launcher tarball.
6. Test-side read, unchanged verdict. Read the cli/tui diffs against the merge base once more (16 files, +325/−95): the makeFake* factories with their honest caveats (overrides unchecked; Partial admits explicit undefined), the uniform Parameters<InspectorClient[…]> wrappers and typed spies with the accurate "toHaveBeenCalledWith is runtime-only" note, entry() fully gate-checked with MALFORMED's single scoped cast as the file's only assertion, the vi.spyOn(process.stderr) refactor, the r.headers?.[…] guards, the AuthTab fixture's newly-required OAuth fields, and the @inspector/core/... specifier fix that started all of this. Nothing to change.
Twenty-three rounds in. The invariant — every tracked .ts/.tsx/.mts/.cts in each gated Node client, plus the non-client first-party TS, gets a real tsc pass — is real and guarded on four axes (coverage, wiring, non-inertness, enrollment), each of which I've confirmed bites. Finding 1 is the round-22 predicate landing one project short of the gate it mirrors; it's fail-safe, zero-count today, and equally fine as a follow-up. Ship it.
· branch v2/1791-typecheck-tests
Responds to the round-23 @claude review of #1799: - Finding 1: round 22's `isExemptCoreFile` mirrored only web's app project, but web's `tsc -b` also builds `tsconfig.test.json`, which includes `core/**/__tests__/**/*.{ts,tsx}`. So a `core/**/__tests__/*.test.ts` (a live layout — `core/mcp/__tests__/fakeInspectorClient.ts` already sits there) was flagged though web checks it. Now mirrors the UNION: under `__tests__/` → exempt `.ts`/`.tsx`; otherwise `.ts` minus co-located `*.test.ts(x)`. Verified a core `__tests__` test now passes while a core `.tsx` outside `__tests__` stays required. - Finding 2: `EXEMPT_ROOTS` was a Map whose keys nothing read (round 22 replaced its filter with hardcoded `isExemptCoreFile`) — a phantom registry. Collapsed to a single `CORE_EXEMPT_REASON` const used in the OK line. - Nit 3: README/AGENTS scope descriptions updated (core is no longer wholesale excluded; the non-client set is deny-by-default). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Both findings + nit addressed in 9004f8d: Finding 1 — Finding 2 — Nit 3 — stale scope in docs. README no longer says "non- Nit 4 (
|
|
@claude review |
|
Claude finished @cliffhall's task in 11m 15s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 127 files (3 clients + non-client), 11.7s, both exemptions named |
npm run verify:format-coverage |
OK — 917 files |
Probe: core/mcp/__tests__/probe-a.**test.ts** |
guard 0 flags, web tsc -b 1 error ✅ (round-23 finding 1 fixed; was a false failure) |
Probe: core/mcp/__tests__/probe-b.**tsx** |
guard 0 flags, web 1 error ✅ |
Probe: core/mcp/probe-c.**tsx** (outside __tests__) |
guard 1 flag, web 0 ✅ (still required, correctly) |
Probe: core/mcp/__tests__/probe-d.**mts** |
guard 1 flag, web 0 ✅ |
Probe: core/mcp/probe-e.**test.ts** (co-located) |
guard 1 flag, web 0 ✅ |
Probe: top-level core/__tests__/x.test.ts (zero-depth **/) |
web does check it → exemption correct ✅ |
Probe: clients/web/playwright.config.ts with a type error |
passed every gate ❌ — finding 1 |
Sweep: 625 tracked clients/web TS vs. union of its 4 projects |
625/625 covered — finding 1's fix is zero-fix |
cli / tui / launcher npm run typecheck (both projects each) |
clean |
tui / launcher npm test |
282 / 5 pass |
format:check:scripts |
clean |
The round-23 matrix is worth stating plainly: guard-exempt ≡ web-covered on all five core/ shapes, and the zero-depth **/ case (core/__tests__/… with no intervening dir) resolves in the direction that keeps them equal. That's the tightest this predicate has been.
1. The clients/web exemption is whole-tree — the exact asymmetry rounds 22–23 just removed from core/, and here it's fully silent
EXEMPT (scripts/verify-typecheck-coverage.mjs:60-62) exempts the entire clients/web tree, deferring to tsc -b. But tsc -b reaches only what its four referenced projects include: src + ../../core/**/*.ts (app), vite.config.ts + tsup.runner.config.ts + server/**/*.ts (node), .storybook (storybook), the test globs (test). A new file at web's package root is in none of them:
$ printf 'export const bad: number = "definitely not a number";\n' > clients/web/playwright.config.ts
$ git add -N clients/web/playwright.config.ts
| gate | result |
|---|---|
verify:typecheck-coverage |
OK — 127 / 3 (count unchanged) |
clients/web npx tsc -b |
exit 0 |
clients/web eslint . |
exit 0 |
verify:format-coverage |
OK — 918 (counts it, prettier only) |
Note the last row: unlike the core/*.mts case from round 22, the sibling guard passes too — web's format:check glob already covers top-level *.{ts,…}. So this is the fully silent version of the shape, with no loud-but-misdirecting fallback. And clients/web has no typecheck script (its build is tsc -b && vite build), so there's no per-client pass to fall back on either.
The reason this is more than symmetry: it's the one exemption that is enrollable. The JSDoc says the solution-style -b limitation is "unreachable today … web, the only tsc -b client, is out of scope" — but web is out of scope because of that limitation, so the sentence is self-sealing. The workaround is already proven: read references out of clients/web/tsconfig.json and run each with -p … --listFilesOnly (which is exactly what projectFiles already does). I measured the union that produces against the tracked set:
tracked clients/web TS (minus ambient .d.ts): 625
tracked but in NO web project: 0
So enrolling web via reference expansion is a no-op on the current tree and closes the playwright.config.ts shape. Same predicate-narrowing move as round 22/23, one directory over.
Nits
2. The core/ exemption lost the presence assertion its predecessor had. Round 17 added a stale-key check for EXEMPT (:119-128) for the stated reason "the exemption outliving its subject". Round 22's EXEMPT_ROOTS Map inherited that framing; round 23 collapsed it to a plain CORE_EXEMPT_REASON string appended unconditionally to the OK line (:461-464). Nothing asserts core/ exists. The coverage consequence is still fail-closed as the comment claims (a renamed core/ surfaces as required, not vanishing — that's the subtractive property, and it holds), so this is narrower than the SHARED_ROOTS case: the output can claim an exemption for a directory that isn't there. Two lines (existsSync(path.join(repoRoot, "core")) → integrity failure) restores parity with the client list.
3. A core/ miss gets client-shaped remediation. My core/mcp/probe-c.tsx probe printed:
core/mcp/probe-c.tsx — in no client's tsconfig project
Add the file to a client's `tsconfig.json` / `tsconfig.test.json` `include` …
The correct fix is to widen clients/web/tsconfig.app.json's include from ../../core/**/*.ts to also cover .tsx — naming a core/ file in a cli/tui test project would be the wrong home for it. Same class as round 13's nit 3 and round 22's nit 3, both of which you fixed by conditioning the line; nonClientMisses is already the right array to dispatch off (round 22's nit 4 anticipated exactly this).
4. Both doc scope sentences predate round 23's __tests__ carve-out by one commit. README.md:149 lists the required core shapes as "a core *.tsx/*.mts or co-located *.test.ts" — but as of 9004f8d2 a core/**/__tests__/*.tsx is exempt (I verified web checks it), so the unqualified *.tsx overstates. Conversely AGENTS.md:235 describes the exempt set as core/**/*.ts + core/**/__tests__/**, and that second glob reads as covering a core/**/__tests__/*.mts, which is required (verified flagged). Both are one clause; understating/overstating in opposite directions from the same commit.
5. Round-23 items, both confirmed. isExemptCoreFile mirrors the union of web's app and test projects and the five-shape matrix above shows it's exact, including the zero-depth **/ case (core/__tests__/x.test.ts — web does check it, so exempting it is right). The phantom EXEMPT_ROOTS registry is gone, collapsed to a single CORE_EXEMPT_REASON with one reader (see nit 2 for its one side effect). Everything from earlier rounds still holds under re-probing: enroll-or-exempt client enumeration, the anchored wiring match on both dir and script tail, pre/post hook following, --noCheck rejection in command and config forms, basename tsc matching, the shared quote-aware tokenizer, .mts/.cts in the required set, and the **/*.test.* excludes keeping a compiled test out of the launcher tarball.
6. Test-side read, unchanged verdict. Re-read the cli/tui diffs against the merge base: the makeFake* factories with their honest caveats (overrides unchecked; Partial admits explicit undefined), the uniform Parameters<InspectorClient[…]> wrappers and typed spies with the accurate "toHaveBeenCalledWith is runtime-only" note, entry() fully gate-checked with MALFORMED's single scoped cast as the file's only assertion, the vi.spyOn(process.stderr) refactor, the r.headers?.[…] guards, the AuthTab fixture's newly-required OAuth fields, and the @inspector/core/... specifier fix that started all of this. Nothing to change.
Twenty-four rounds in. The invariant — every tracked .ts/.tsx/.mts/.cts in each gated Node client, plus the first-party TS no client owns, gets a real tsc pass — is real and guarded on four axes (coverage, wiring, non-inertness, enrollment), each of which I've confirmed bites. Finding 1 is the last whole-tree exemption whose scope exceeds the gate it defers to; it's zero-fix today and equally fine as a follow-up in the #1791/#1792 cluster. Ship it.
· branch v2/1791-typecheck-tests
Responds to the round-24 @claude review of #1799: - Finding 1: `clients/web` was whole-tree EXEMPT, but web's `tsc -b` only reaches what its four referenced projects include — a file at web's package root (e.g. `playwright.config.ts`) got no tsc pass and passed EVERY gate, fully silently. This was the last whole-tree exemption wider than the gate it defers to. Now web is ENROLLED: a client with no `typecheck` script but a `tsconfig.json` `references` array is measured through those reference projects (`tsc -p <ref> --listFilesOnly`), and is wired iff its `validate` runs `tsc -b` (`validateRunsTscBuild`). EXEMPT is now the empty escape hatch. Verified: a web-root `playwright.config.ts` with a type error is now flagged, and dropping `tsc -b` from web's build hard-fails the integrity phase. Zero-fix on the tree — the union of web's 4 projects covers all 625 tracked web files. - Nit 2: assert `core/` exists (the OK line unconditionally states its exemption) — parity with the EXEMPT stale-key check. - Nit 3: a `core/` miss now gets core-specific remediation (widen web's `tsconfig.app.json` include, not a cli/tui project). - Nit 4: README/AGENTS scope descriptions corrected — web is enrolled (not exempt), and the exact core-exempt shape (app `*.ts` minus co-located tests + test `__tests__/**/*.{ts,tsx}`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Finding 1 closed by enrolling Web was
Nit 2 — Nit 3 — a Nit 4 — README/AGENTS corrected: web is enrolled (not exempt), and the exact core-exempt shape (app project's
|
|
@claude review |
|
Claude finished @cliffhall's task in 17m 42s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 752 files (4 clients + non-client), 20s |
npm run verify:format-coverage |
OK — 917 files |
Probe: clients/web/playwright.config.ts with a type error |
caught — "in no tsconfig project" ✅ (round-24 finding 1 fixed; passed every gate last round) |
Probe: drop tsc -b from web's build |
caught — "no typecheck script and its validate never runs tsc -b" ✅ |
Probe: isExemptCoreFile disabled entirely |
still green — 752 → 894 files, exit 0 — finding 1 |
Probe: web's app include narrowed off core/, + unimported core/mcp/orphan-probe.ts |
web tsc -b exit 0, guard green ❌ — finding 1 |
Probe: web build → tsc -b --noCheck |
green ❌ — finding 2 (and tsc -b --noCheck verified to genuinely disable checking) |
Probe: /* */ comment in web's tsconfig.json |
false enrollment failure — nit 3 |
cli / tui / launcher npm run typecheck (both projects each) |
clean |
tui npm test |
282 pass |
format:check:scripts |
clean |
1. Enrolling web made the core/ exemption both redundant and fail-open — it's now a hand-model of something the guard measures directly
Rounds 22–23 were spent making isExemptCoreFile (scripts/verify-typecheck-coverage.mjs:341-345) an exact mirror of what web's tsc -b covers. Round 24 then enrolled web and measures those same four projects with --listFilesOnly — so the mirror is no longer needed, and the guard now has two disagreeing sources of truth about core/. First that it's redundant:
$ # isExemptCoreFile → false for everything
$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — OK: all 894 tracked source files (4 clients + non-client) get a tsc pass … # exit 0
752 → 894: all 142 core/ files become required and all 142 are already covered by web's measured projects. Zero-fix.
And with the exemption in place, the model can go stale in the silent direction — the exact failure the round-21/22 comment says the subtractive design prevents. Narrowing web's app include from ["src", "../../core/**/*.ts"] to ["src"], plus an unimported core/ file with a real error:
$ printf 'export const orphanBad: number = "definitely not a number";\n' > core/mcp/orphan-probe.ts && git add -N …
$ (cd clients/web && npx --no-install tsc -b) ; echo $?
0 # web no longer checks it
$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — OK: all 752 tracked source files (4 clients + non-client) get a tsc pass
(core/ exempt: typechecked by web's `tsc -b` (`core/**/*.ts` + `core/**/__tests__/**`)). # exit 0
The guard defers to a claim about web's include that is no longer true, while holding the measurement that would have caught it. Deleting isExemptCoreFile + CORE_EXEMPT_REASON and letting trackedNonClientSource() require every core/ file to land in the global union removes ~15 lines, removes the drift axis, and removes the round-24 nit-2 presence assertion (:402-406) as a thing that needs to exist.
2. The new reference-client path never applies the non-inertness check — tsc -b --noCheck gates nothing and the guard stays green
validateRunsTscBuild (:105-119) asks only "does some validate-reachable segment invoke tsc with -b?". It ignores every other flag, and projectDisablesChecking (which the reference branch does call, :441-449) reads only the config. So the command-flag form — the one round 10 closed for the typecheck-script path (:228-230) — is unguarded here. First, the flag really does disable checking with -b:
$ printf 'export const bad: number = "nope";\n' > clients/web/src/probe-bad.ts
$ (cd clients/web && npx --no-install tsc -b --noCheck) ; echo $?
0
$ (cd clients/web && npx --no-install tsc -b --force)
src/probe-bad.ts(1,14): error TS2322: Type 'string' is not assignable to type 'number'.
Then, with web's build set to tsc -b --noCheck && vite build && …:
$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — OK: all 752 tracked source files (4 clients + non-client) get a tsc pass … # exit 0
Web's entire 625-file tree — and, while finding 1 stands, core/ by deferral — is checked by nothing, and the guard vouches for it. Same class as round 10's finding 1, in the branch added last commit. typecheckProjects already has the predicate (/^--(noCheck|listFilesOnly)$/i); validateRunsTscBuild needs to reject a segment carrying it and report the same integrity failure.
Nits
3. A /* */ comment in a reference client's tsconfig.json produces a false enrollment failure — and block comments are the prevailing style in this repo. clientTsconfigReferences (:86-102) strips only // lines and trailing commas, so a block comment throws and the catch returns [] — which reads as "has no references":
verify:typecheck-coverage — 1 gate-integrity issue(s): …
clients/web: declares no `typecheck` script, has no `tsconfig.json` `references`, and isn't in the EXEMPT set …
…while tsc -b --dry in the same tree is perfectly happy. The JSDoc names the //-only support as deliberate, but this isn't an exotic form: every other tsconfig in the repo already uses /* */ — clients/{cli,tui,launcher}/tsconfig.json, all four of clients/web/tsconfig.{app,node,storybook,test}.json. clients/web/tsconfig.json is the only one without, so it's one comment away from this. Stripping /\/\*[\s\S]*?\*\//g before the line-comment pass covers it; note also the line-comment regex would corrupt a // inside a string value (an https:// in extends).
4. EXEMPT is now an empty Map, which makes three paths dead — including the round-17 assertion that justified it. EXEMPT.has(dir) (:148), the stale-key loop (:173-182, added in round 17 precisely so "the exemption can't outlive its subject"), and the exemptNote spread (:553) can never do anything. The exemptNote ? … : "." ternary (:559) is also dead now — core/ exempt: … is appended unconditionally, so the string is never empty. Keeping EXEMPT as a documented escape hatch is reasonable; if finding 1 lands, exemptNote collapses to nothing and this whole block goes with it.
5. Both scope docs still describe the wiring assertion in typecheck-script terms only. README.md:149 ("asserts the gate is wired (each typecheck is reachable from its validate…)") and AGENTS.md:235 (same phrasing) — for web there is no typecheck script; the assertion is "its validate runs tsc -b", which is what probe D exercises. Both sentences correctly describe the enrollment half added in round 24; it's only the wiring clause that lagged. One clause each.
6. validateRunsTscBuild doesn't tie the tsc -b it finds to the tsconfig whose references were read. A build of tsc -b tsconfig.other.json satisfies the check while building a different graph than the one clientTsconfigReferences measured — so the guard would report coverage for projects nothing builds. Contrived, and fail-open only in that narrow shape; recording it because the reference path's two halves (which config supplies the projects, which command proves they're built) are otherwise unconnected.
7. Round-24 items, all four confirmed. clients/web is enrolled through its tsconfig.json references and I verified both directions: a package-root playwright.config.ts with a type error is now flagged (it passed literally every gate in the repo last round, including verify:format-coverage), and dropping tsc -b from web's build hard-fails the integrity phase. The union of web's four reference projects covers its tree with no new failures — the guard reports 752 tracked source files (4 clients + non-client), up from 127. EXEMPT is the empty escape hatch (see nit 4), core/'s existence is asserted, a core/ miss gets core-specific remediation naming tsconfig.app.json rather than a cli/tui project, and README/AGENTS describe web as enrolled. Everything from earlier rounds still holds under re-probing: enroll-or-exempt enumeration, the anchored wiring match on both dir and script tail, pre/post hook following, --noCheck rejection for the typecheck-script path in both command and config forms, basename tsc matching, the shared quote-aware tokenizer, .mts/.cts in the required set, and the **/*.test.* excludes keeping a compiled test out of the launcher tarball.
8. Test-side read, unchanged verdict. The actual subject of the PR remains clean: the makeFake* factories with their honest caveats (overrides unchecked; Partial admits explicit undefined), the uniform Parameters<InspectorClient[…]> wrappers and typed spies with the accurate "toHaveBeenCalledWith is runtime-only" note, entry() fully gate-checked with MALFORMED's single scoped cast as the file's only assertion, the vi.spyOn(process.stderr) refactor, the r.headers?.[…] guards, the AuthTab fixture's newly-required OAuth fields, and the @inspector/core/... specifier fix that started all of this. Nothing to change.
Twenty-five rounds in. The invariant — every tracked .ts/.tsx/.mts/.cts in every client, plus the first-party TS no client owns, gets a real tsc pass — is real and guarded on four axes (coverage, wiring, non-inertness, enrollment), each of which I've confirmed bites. Both findings are in the reference-client path added last commit: finding 1 removes code and a drift axis for free, finding 2 extends an existing predicate one branch further. Neither is a hole in the PR's actual subject. Ship it — here or as a follow-up in the #1791/#1792 cluster.
· branch v2/1791-typecheck-tests
…b --noCheck Responds to the round-25 @claude review of #1799: - Finding 1: enrolling web (round 24) made the hand-maintained `core/` exemption both redundant and fail-open — web's measured `tsc -b` projects already cover core, and the mirror could go stale (narrowing web's include let an unimported core error pass). Deleted `isExemptCoreFile`, `CORE_EXEMPT_REASON`, the core presence assertion, and the exemptNote core clause. `core/` is now required like everything else and covered by measurement (127→894 files; a `core/*.tsx` web doesn't reach is now caught by measurement, not a hand-model). Kept the core-specific remediation clause. - Finding 2: the reference (`tsc -b`) path skipped the non-inertness check — a `tsc -b --noCheck` build gated nothing while the guard stayed green. Turned `validateRunsTscBuild` into `tscBuildStatus` (ok/neutered/none); a neutered `tsc -b` now hard-fails, matching the typecheck-script path. Factored the disabling-flag predicate (`isDisablingFlag`) so both paths share it. - Nit 3: `clientTsconfigReferences` now strips block comments too (every other tsconfig in the repo uses `/* */`). - Nit 5: README/AGENTS wiring clause covers web's `tsc -b` (not just the `typecheck` script), and core is described as measured, not exempt. Nit 4 resolves itself: EXEMPT is empty and the exemptNote/ternary now collapse cleanly. Nit 6 (tsc -b bound to a specific tsconfig) left as recorded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Both findings + nits in a6a82ba: Finding 1 — the Finding 2 — the reference path skipped the non-inertness check. Nit 3 — block comments in a reference client's tsconfig.json. Nit 5 — wiring docs. README/AGENTS now describe the wiring assertion as "each client's typecheck pass — its Nit 4 resolves itself (EXEMPT empty → exemptNote/ternary collapse cleanly). Nit 6 (tie the
|
|
@claude review |
|
Claude finished @cliffhall's task in 8m 5s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 894 files (4 clients + non-client), 20s |
npm run verify:format-coverage |
OK — 917 files |
Probe A: web build → tsc -b --noCheck |
caught — "carries --noCheck/--listFilesOnly … gated by nothing" ✅ (round-25 finding 2 fixed; was green) |
Probe B: narrow web app include off core/ + unimported core/mcp/orphan-probe.ts |
caught — 5 files incl. the probe ✅ (round-25 finding 1 fixed; was green at 752) |
Probe C: /* */ comment in clients/web/tsconfig.json |
no enrollment failure ✅ (round-25 nit 3 fixed) |
Probe D: clients/web declares "typecheck": "tsc -b" |
665 false failures ❌ — finding 1 |
Probe E: core/mcp/__tests__/probe.mts |
correctly required, but app-project remediation — nit 2 |
Probe F: which web project covers core/ |
app 142/142, test 137, node 34, storybook 28 |
Probe G: does an incremental tsc -b report a fresh core/ error? |
yes — warm build exit 0, then TS2322 after editing core/logging/index.ts ✅ |
cli / tui / launcher npm run typecheck (both projects each) |
clean ×3 |
tui / launcher npm test |
282 / 5 pass |
format:check:scripts |
clean |
Probe B is worth calling out: last round that exact edit left both web's tsc -b and the guard green, because the guard deferred to a hand-written model of web's include. Deleting the model and measuring instead is what makes it bite. And probe G closes the loop that core/'s coverage now rests on — tsc -b being incremental doesn't cause it to skip a freshly-broken project.
1. Enrolling web put it in scope, but the -b solution-config limitation still says it isn't — and the refactor that trips it is a natural one
typecheckProjects's JSDoc (scripts/verify-typecheck-coverage.mjs:221-224) still carries the round-13 framing:
Two limitations, both unreachable today (cli/tui/launcher use plain
-p--noEmitpasses; web, the onlytsc -bclient, is out of scope): a solution-style-bconfig ("files": []+references) is run here as-p … --listFilesOnly, which lists nothing …
Web was out of scope when that was written. Round 24 enrolled it — but only via the references branch, which the code takes solely because web declares no typecheck script. Adding one flips it to the typecheck-script branch, where the harvested project is web's solution config, and tsc -p tsconfig.json --listFilesOnly on {"files": [], "references": […]} lists nothing:
"typecheck": "tsc -b",
"validate": "… && npm run typecheck && npm run build && npm run test"$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — 665 tracked source file(s) get no `tsc` pass:
clients/web/.storybook/main.ts — in no tsconfig project
clients/web/server/browser-externalized-builtin-gate.ts — in no tsconfig project
… # 625 web files + 40 non-client (core/, test-servers/)
Fail-safe, so no gate hole — but it's a false failure on the uniformity move (all three other clients have a typecheck; round 24's own review probed adding one to web), and the blast radius now includes core/, which since round 25 depends on web's projects. The two halves of the reference support are also asymmetric in a way that invites it: the branch is selected by "has no typecheck script" rather than "is a solution config".
Cheap fix, and the machinery already exists: when a harvested project resolves to a solution config (empty file list, or a parsed references array), expand it through clientTsconfigReferences and measure those — exactly what the reference branch does today. Zero-fix on the current tree (web declares no typecheck). At minimum, the JSDoc's "web … is out of scope" clause needs to go, since it's the sentence that justifies leaving the limitation unhandled.
Nits
2. The core/ remediation names the app project for a file whose home is the test project. Probe E — a core/mcp/__tests__/probe.mts (correctly required, since web's test project globs only {ts,tsx} there) prints:
core/mcp/__tests__/probe.mts — in no client's tsconfig project
…
For a `core/` file (a `*.tsx`/`*.mts` or co-located test web's `tsc -b` doesn't reach),
widen `clients/web/tsconfig.app.json`'s `include` …
But core/**/__tests__/** is clients/web/tsconfig.test.json's territory (:40-41); widening the app project's include to reach a core test would pull test files into the app build. Same class as the round-13/22 remediation-accuracy nits, both of which you fixed by conditioning the line — here, f.includes("/__tests__/") picks the right project name.
3. EXEMPT is now an empty Map and three paths are unreachable. EXEMPT.has(dir) (:165), the stale-key loop (:190-199, added in round 17), and the exemptNote spread (:540). Round 25 nit 4 flagged this and you noted it "resolves itself" — it does, cleanly: the exemptNote ? … : "." ternary is now genuinely live (I confirmed the OK line ends with a bare .). Recording that keeping EXEMPT as a documented escape hatch is a fine call, and that the round-17 stale-key assertion is now dormant rather than deleted, so it comes back for free if the hatch is ever used.
4. AGENTS.md:232's one-line summary still describes the client-only scope. "asserts every tracked .ts/.tsx/.mts/.cts in each gated Node client lands in a tsconfig project" — the detailed bullet at :235 was correctly widened this commit ("all of core/", deny-by-default), and README's table row too, but the summary sentence a reader hits first still omits the non-client half. Understating, and one clause.
5. Round-25 items, both confirmed. isExemptCoreFile / CORE_EXEMPT_REASON / the presence assertion / the exemptNote clause are all gone, and core/ is now measured — probe B is the proof: narrowing web's app include while adding an unimported core/ file, which left both web and the guard green last round, now fails with the file named. The -15 lines and the removed drift axis are exactly as claimed. tscBuildStatus distinguishes ok/neutered/none with isDisablingFlag shared by both paths, and a tsc -b --noCheck build now hard-fails; probe G additionally confirms the plain incremental tsc -b this all defers to does report a fresh core/ error rather than skipping it. Block comments in a reference client's tsconfig.json no longer false-fail enrollment. Docs updated in both places. Everything from earlier rounds still holds under re-probing.
6. Test-side read, unchanged verdict. The actual subject of the PR: the makeFake* factories with their honest caveats (overrides unchecked; Partial admits explicit undefined), the uniform Parameters<InspectorClient[…]> wrappers and typed spies with the accurate "toHaveBeenCalledWith is runtime-only" note, entry() fully gate-checked with MALFORMED's single scoped cast as the file's only assertion, the vi.spyOn(process.stderr) refactor, the r.headers?.[…] guards, the AuthTab fixture's newly-required OAuth fields, and the @inspector/core/... specifier fix that started all of this. Nothing to change.
Twenty-six rounds in. The invariant — every tracked .ts/.tsx/.mts/.cts in every client, plus all the first-party TS no client owns, gets a real tsc pass — is real and guarded on four axes (coverage, wiring, non-inertness, enrollment), each of which I've confirmed bites. Round 25 is a net deletion that removed the last hand-maintained model in favor of measurement, which is the right direction. Finding 1 is a stale justification plus the false failure it leaves unguarded — fail-safe, zero-fix today, and equally fine as a follow-up in the #1791/#1792 cluster. Ship it.
· branch v2/1791-typecheck-tests
Responds to the round-26 @claude review of #1799: - Finding 1: the reference (`tsc -b`) path was selected only by "no typecheck script", so adding `typecheck: "tsc -b"` to web (the natural uniformity move) flipped it to the typecheck-script path, where the harvested solution config lists nothing → 665 false failures. Fixed at the measurement layer: projectFiles now expands a solution config (`{"files":[], "references":[…]}` — lists nothing under --listFilesOnly) to its references and measures those recursively, so a `tsc -b` project is measured no matter which path harvests it. Generalized the references reader (tsconfigReferences(path)). Verified web with `typecheck: "tsc -b"` now passes. Dropped the stale "web … is out of scope" JSDoc clause. - Nit 2: a `core/**/__tests__/**` miss now points at `tsconfig.test.json` (its actual owner), not `tsconfig.app.json`. - Nit 4: AGENTS.md's one-line summary now mentions the non-client (`core/`, `test-servers/src`) coverage, matching the detailed bullet. Nit 3 (EXEMPT empty, dormant stale-key check) left as the intended escape hatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Finding 1 + nits in be8ca3b: Finding 1 — a solution config on the typecheck-script path lists nothing (665 false failures if web adds Nit 2 — core Nit 4 — AGENTS summary. The one-line summary now names the non-client ( Nit 3 (empty
|
|
@claude review |
|
Claude finished @cliffhall's task in 7m 6s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 894 files (4 clients + non-client), 20s |
npm run verify:format-coverage |
OK — 917 files |
Probe A: clients/web declares "typecheck": "tsc -b" |
OK, exit 0 ✅ — round-26 finding 1 fixed (was 665 false failures) |
Probe B: same, + "noCheck": true in tsconfig.app.json |
green ❌ — finding 1 (and tsc -b --force exits 0 on a real TS2322) |
Probe C: same noCheck, reference path (no typecheck script) |
caught — 3 refs named ✅ (the paths disagree) |
Probe D: core/mcp/__tests__/probe.mts |
caught, remediation now names tsconfig.test.json ✅ (round-26 nit 2 fixed) |
Probe E: web src error, plain tsc -b --force |
TS2322 reported ✅ (probe B's exit 0 really was the noCheck) |
Probe F: "typecheck": "tsc -b --noCheck" |
caught — "lists files without type-checking them" ✅ (command form still guarded) |
cli / tui / launcher npm run typecheck (both projects each) |
clean ×3 |
tui / launcher npm test |
282 / 5 pass |
format:check:scripts |
clean |
1. The solution-config expansion fixed coverage on the typecheck-script path but not non-inertness — a noCheck in a referenced project is invisible there
The reference path checks each ref individually (scripts/verify-typecheck-coverage.mjs:453-461). The typecheck-script path runs projectDisablesChecking on the harvested project only (:477-485) — which, for a tsc -b client, is the solution config, whose own compilerOptions are empty. Round 26 taught projectFiles to expand the solution's references for measurement, but the integrity phase never follows them. Both directions verified on the same tree, with only clients/web/package.json differing:
// clients/web/tsconfig.app.json → "compilerOptions": { "noCheck": true, … }$ printf 'export const bad: number = "nope";\n' > clients/web/src/probe-bad.ts
$ (cd clients/web && npx --no-install tsc -b --force) ; echo $?
0 # checking is genuinely off (without noCheck: TS2322)
# path 1 — "typecheck": "tsc -b" (solution harvested from the script)
$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — OK: all 894 tracked source files (4 clients + non-client) get a tsc pass. # exit 0
# path 2 — no typecheck script (the dedicated reference path), same tsconfigs
$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — 3 gate-integrity issue(s): …
clients/web: reference `./tsconfig.app.json` sets `noCheck` in its tsconfig — …
clients/web: reference `./tsconfig.storybook.json` sets `noCheck` in its tsconfig — …
clients/web: reference `./tsconfig.test.json` sets `noCheck` in its tsconfig — … # exit 1
On path 1 web's whole 625-file tree — plus core/, which since round 25 depends on web's projects — is checked by nothing and the guard vouches for it. That's the round-10 hole ("the gate runs but checks nothing") in the branch round 26 just made reachable: adding typecheck: "tsc -b" is precisely the uniformity move round 26's finding was about, so the two changes open and close halves of the same door.
Zero-fix today (web declares no typecheck), fail-open rather than fail-safe. The machinery already exists — the same reference expansion projectFiles does, applied in the integrity loop: when a harvested project's --listFilesOnly set is empty and it has references, run projectDisablesChecking over those instead of (or as well as) the solution.
Nits
2. A nested solution referenced in directory form isn't expanded. tsconfigReferences (:89-109) readFileSyncs the reference path as a file, but TS accepts a directory ({ "path": "./packages/a" } — the form the TS handbook uses) and resolves tsconfig.json inside it. I confirmed readFileSync on a directory throws EISDIR → the catch returns []. Harmless at the first level (tsc -p ./packages/a --listFilesOnly resolves the directory fine, so coverage is measured), so this only bites when a directory-form reference points at a project that is itself a solution — its refs are then never reached and its files report as uncovered. Doubly contrived, fail-safe, and the fix is one line (append /tsconfig.json when the path doesn't end in .json) if you'd rather not carry it as a boundary.
3. covered.size === 0 is a sufficient signal for "solution config", not a necessary one — and it's also what a failed tsc produces. The new block (:336-348) and its JSDoc both frame the empty set as "a solution config lists nothing". A project whose tsc invocation died on a config error (already warned two lines up) also lands here and then gets read as a tsconfig for references — harmless, since a broken config yields [] too, but the comment reads as if the branch were solution-specific when it's really "nothing was listed, try references". One clause.
4. The script header still describes the reference path as the only tsc -b route. :3-6 — "every clients/*, enrolled via its typecheck script's projects (cli, tui, launcher) or, for a tsc -b client with no typecheck script (clients/web), via its tsconfig.json references". Accurate for enrollment, but after round 26 a tsc -b solution is measured through its references on either path — which is the whole point of the commit, and the typecheckProjects JSDoc now says so correctly. Same one-line lag class as round 8's doc sweep; the AGENTS/README sentences are fine as written since they describe enrollment.
5. Round-26 items, all three confirmed. Probe A is the headline: clients/web with typecheck: "tsc -b" now reports OK where it produced 665 false failures last round, and I verified the expansion is what does it (web's solution lists nothing under --listFilesOnly; its four refs supply the 625 files). tsconfigReferences is generalized and clientTsconfigReferences is a thin wrapper — the recursion's seen set correctly terminates a self- or diamond-referencing graph, and because every level accumulates into the same covered, dedup doesn't lose files. The core/ remediation now names tsconfig.test.json for a core/**/__tests__/** miss and tsconfig.app.json otherwise (probe D). AGENTS.md:232's summary mentions the non-client coverage, matching the detailed bullet at :235 and README's table row. The stale "web … is out of scope" JSDoc clause is gone. Everything from earlier rounds still holds under re-probing: enroll-or-exempt enumeration, the anchored wiring match on both dir and script tail, pre/post hook following, --noCheck rejection in the command form on both paths (probe F) and the config form on the reference path (probe C), basename tsc matching, the shared quote-aware tokenizer, .mts/.cts in the required set, and the **/*.test.* excludes keeping a compiled test out of the launcher tarball.
6. Test-side read, unchanged verdict. The PR's actual subject is stable at 18 files vs. the merge base and I re-read it: the makeFake* factories with their honest caveats (overrides unchecked; Partial admits explicit undefined), the uniform Parameters<InspectorClient[…]> wrappers and typed spies with the accurate "toHaveBeenCalledWith is runtime-only" note, entry() fully gate-checked with MALFORMED's single scoped cast as the file's only assertion, the vi.spyOn(process.stderr) refactor, the r.headers?.[…] guards, the AuthTab fixture's newly-required OAuth fields, and the @inspector/core/... specifier fix that started all of this. Nothing to change.
Twenty-seven rounds in. The invariant — every tracked .ts/.tsx/.mts/.cts in every client, plus all the first-party TS no client owns, gets a real tsc pass — is real and guarded on four axes (coverage, wiring, non-inertness, enrollment), each of which I've confirmed bites. Finding 1 is the one axis where the two enrollment paths still diverge; it's unreachable on today's tree and equally fine as a follow-up in the #1791/#1792 cluster. Ship it.
· branch v2/1791-typecheck-tests
…aths Responds to the round-27 @claude review of #1799: - Finding 1: round 26 made COVERAGE follow a solution config's references, but the non-inertness (`noCheck`) check on the typecheck-script path still only inspected the harvested solution (empty compilerOptions) — so a `noCheck` in a REFERENCED project was invisible there (caught only on the reference path). Factored a shared `resolveLeafProjects` (a solution → its real leaf projects, cached rawProjectFiles) used by BOTH coverage (`projectFiles`) and a shared `checkingLeaves` disable-check, so a `noCheck` in a referenced project is now caught on either path. Verified: `typecheck: "tsc -b"` + `noCheck` in tsconfig.app.json now hard-fails (was green). Caching also cut the guard from ~20s to ~11s. - Nit 2: directory-form references (`{ "path": "./packages/a" }`) now resolve to `<dir>/tsconfig.json`, so a nested-solution dir reference is expanded. - Nit 3: reworded the empty-listFiles comment (it's "solution config OR errored config", both of which the reference expansion handles). - Nit 4: script header notes a `tsc -b` solution is reduced to its references on either enrollment path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Finding 1 + nits in 82db89e: Finding 1 — non-inertness didn't follow references on the typecheck-script path. Factored Nit 2 — directory-form references. Nit 3 — the empty- Nit 4 — script header. Now notes a
|
|
@claude review |
|
Claude encountered an error after 6m 15s —— View job Review:
|
|
The previous review run errored out (infra, not the diff). @claude review |
|
Claude finished @cliffhall's task in 17m 43s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 894 files (4 clients + non-client), 21s |
npm run verify:format-coverage |
OK — 917 files |
Probe A: clients/web typecheck: "tsc -b" + noCheck in tsconfig.app.json |
caught — 3 per-leaf lines ✅ round-27 finding 1 fixed (was green) |
Probe B: same noCheck, reference path (no typecheck script) |
caught — same 3 lines ✅ (the two paths now agree) |
Probe C: -p . (directory-form project) |
OK ✅ — nit-2's dir handling covers this idiom too |
Probe D: core/mcp/probe-widget.tsx |
caught ✅ (round-22/25 still bites) |
Probe E: core/mcp/__tests__/probe.test.ts |
required and covered — 895 files, web tsc -b reports it ✅ |
Probe F: unimported test-servers/src/server-orphan.ts |
caught ✅ |
Probe G: clients/web/playwright.config.ts |
caught ✅ (round-24 still bites) |
Probe H: // @ts-nocheck in a gated test file |
guard green, client typecheck exit 0 — but eslint fails ✅ finding 3 |
Probe I: same, in core/, test-servers/src, launcher/__tests__, cli/__tests__ |
eslint fails all four ✅ |
Probe J: hybrid solution (own files + references) |
not expanded — but tsc itself rejects the shape (TS6306/TS6310) → unreachable |
| Probe K: duplicate leaf from two harvested projects | duplicate integrity line — nit 1 |
Probe L: does web's tsc -b really check all four projects? |
4/4 — a fresh error in src / server / .storybook / src/test each reported ✅ |
cli / tui / launcher npm run typecheck (both projects each) |
clean ×3 |
tui / launcher npm test |
282 / 5 pass |
format:check:scripts |
clean |
Probe A is the one that matters: last round that exact manifest reported "OK — 894 files" while web's entire tree plus core/ was checked by nothing. It now hard-fails, and probe B shows the reference path produces the identical diagnosis — which is precisely what resolveLeafProjects + a shared checkingLeaves was for.
Probe E deserves a note too: core/**/__tests__/*.test.ts is now required by the guard and covered by measurement (895/895, web reports the error). Rounds 22–23 spent two iterations hand-tuning a predicate to make those two sets agree; deleting the predicate in favor of measurement made them agree by construction.
1. checkingLeaves dedups the leaf list but not the failure list, so one noCheck can be reported twice
scripts/verify-typecheck-coverage.mjs:391-402 guards the checking array with !checking.includes(leaf), but projectDisablesChecking is called — and integrity.pushed — on every visit. resolveLeafProjects's seen is per top-level project (a fresh default arg per projects entry), so a leaf reachable from two harvested projects is visited twice. Verified:
"typecheck": "tsc --noEmit -p tsconfig.json && tsc --noEmit -p tsconfig.json" // + noCheck in that tsconfigverify:typecheck-coverage — 2 gate-integrity issue(s): …
clients/tui: `tsconfig.json` sets `noCheck` in its tsconfig — …
clients/tui: `tsconfig.json` sets `noCheck` in its tsconfig — … ← same line twice
Cosmetic (the count in the header is inflated, the advice is right), and the realistic trigger is two solutions sharing a referenced project rather than the literal duplicate above. Moving the checking.includes check to the top of the loop body — skip an already-visited leaf entirely, rather than only skipping the push — fixes both the duplicate line and the redundant tsc --showConfig spawn.
Nits
2. The commit's perf claim doesn't reproduce here — and the remaining lever is the other tsc call. The message says caching cut the guard from ~20s to ~11s; I measure 20.7s / 21.1s on two runs. The cache is doing real work — I instrumented it and got 30 --listFilesOnly lookups → 10 actual spawns. But there are also 10 tsc --showConfig spawns from projectDisablesChecking (:276-287), which is the one tsc call round 27 left uncached, and it's now roughly half the wall clock. Today there are exactly 10 distinct leaves and 10 calls, so a cache wouldn't help on the current tree (nit 1's duplicate is the only case it would) — the honest framing is just that ~11s isn't reproducible on a CI-class runner, so don't lean on it.
3. Completeness result: the last "listed but unchecked" tier is // @ts-nocheck, and it's covered — by eslint, not by this guard. The guard now rejects the flag form (--noCheck), the config form (compilerOptions.noCheck), and — as of this commit — either of those in a referenced project. The tier below that is per-file, and it's invisible to a tsc pass by construction:
$ # `// @ts-nocheck` + a real TS2322 prepended to clients/tui/__tests__/HistoryTab.test.tsx
$ (cd clients/tui && npx tsc --noEmit -p tsconfig.test.json) ; echo $? # 0
$ node scripts/verify-typecheck-coverage.mjs # OK — 894 files
That would matter, because AGENTS.md forbids exactly this ("NEVER suppress error types … as a way of satisfying the linter or compiler") — except @typescript-eslint/ban-ts-comment fires on it, and I confirmed it fires in every surface this PR widened the typecheck gate to reach: cli __tests__, tui __tests__, launcher __tests__, core/ via lint:core, and test-servers/src via lint:shared. There are also zero @ts-nocheck in the repo today. So this is a closed tier, not an open one — worth recording because it's the natural next question after this commit, and the answer is "a different gate owns it."
4. Two boundaries on the reference machinery, both verified unreachable. (a) resolveLeafProjects stops at a project that lists files, so a hybrid solution (own files plus references) wouldn't have its refs expanded — but tsc rejects that shape in this repo (TS6306: Referenced project … must have setting "composite": true, TS6310), so it can't ship. (b) The whole per-ref -p … --listFilesOnly measurement depends on web's refs being non-composite: nothing declares composite: true, and files: [] at the solution root makes each reference a standalone build root, which is why probe L sees all four projects genuinely checked and why --listFilesOnly lists real sources rather than a referenced project's .d.ts output. A future migration to real composite references would change that (fail-safe — false failures), so it's worth knowing the arrangement is load-bearing rather than incidental.
5. Round-27 items, all four confirmed. resolveLeafProjects + the shared checkingLeaves make non-inertness follow references on both paths — probes A and B give byte-identical diagnoses from the two enrollment routes, where last round only the reference path caught it. Directory-form references resolve to <dir>/tsconfig.json, which also makes the plausible tsc -p . form work (probe C). The empty---listFilesOnly comment now says "solution config or errored config", which matches the behavior (a broken config yields no references either). The script header notes the reduction happens on either path. The rawProjectFiles cache is correct — an empty Set is truthy so a legitimately-empty listing is cached rather than re-spawned, and the recursion's seen terminates a self- or diamond-referencing graph while every level accumulates into the same covered, so dedup can't lose files.
6. Test-side read, unchanged verdict. The PR's actual subject: the makeFake* factories with their honest caveats (defaults typed against the real signatures, overrides explicitly not signature-checked, Partial admits an explicit undefined), the uniform Parameters<InspectorClient[…]> wrappers and typed spies with the accurate "toHaveBeenCalledWith is runtime-only" note, entry() fully gate-checked with MALFORMED's single scoped cast as the file's only assertion, the vi.spyOn(process.stderr) refactor, the r.headers?.[…] guards, the AuthTab fixture's newly-required OAuth fields, and the @inspector/core/... specifier fix that started all of this. The three tsconfig.test.json files all inherit or set noEmit, and each includes only what its sibling src project can't reach. Nothing to change.
Twenty-eight rounds in. The invariant — every tracked .ts/.tsx/.mts/.cts in every client, plus all the first-party TS no client owns, gets a real tsc pass — is real and guarded on four axes (coverage, wiring, non-inertness, enrollment), each of which I've confirmed bites, and the one tier the guard structurally can't reach is owned by eslint. Nothing here blocks; nit 1 is a two-line message fix. Ship it.
· branch v2/1791-typecheck-tests
Responds to the round-28 @claude review of #1799 (which found no new gate hole): - Finding 1: `checkingLeaves` dedup'd the `checking` array but ran projectDisablesChecking + integrity.push on every visit, so a leaf reachable from two harvested projects produced a duplicate `noCheck` line (and a redundant `tsc --showConfig`). Now tracks visited leaves in a Set and skips an already-seen leaf entirely. Verified a duplicate harvested project yields one line, not two. - Nit 3 (record-completeness): noted in AGENTS that the one tier the guard structurally can't see — a per-file `// @ts-nocheck` — is owned by `@typescript-eslint/ban-ts-comment` across lint:core/lint:shared/each client. Nit 2 (the ~11s perf claim isn't reproducible on a CI-class runner) and nit 4 (hybrid-solution / composite boundaries, both tsc-rejected → unreachable) left as recorded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
|
Thanks — glad round 28 turned up no new gate hole, and the Finding 1 — duplicate Nit 3 — record completeness. Added a sentence to AGENTS noting the per-file Nit 2 (perf) — fair; ~11s was my machine, ~20s on a CI-class runner. Not leaning on it (the honest framing is the
|
|
@claude review |
|
Claude finished @cliffhall's task in 9m 4s —— View job Review:
|
| Check | Result |
|---|---|
npm install (root cascade, fresh checkout) |
ok, all four clients |
npm run verify:typecheck-coverage |
OK — 894 files (4 clients + non-client), 16.0s |
npm run verify:format-coverage |
OK — 917 files |
Probe: duplicate harvested project + noCheck in it |
one integrity line, not two ✅ (round-28 finding 1 fixed) |
Probe: tracked scripts/probe-ts.ts |
caught — but with unsatisfiable remediation, see nit 2 |
Probe: import() the guard for testing |
0 exports, self-executes (printed its OK line on import) — finding 1 |
Probe: import() scripts/lib/npm-scripts.mjs |
4 pure exports, importable today — finding 1 |
cli / tui / launcher npm run typecheck (both projects each) |
exit 0 ×3 |
cli / tui / launcher npm test |
301 / 282 / 5 pass |
Merged config check: does any tsconfig.test.json emit? |
noEmit: true in all three ✅ |
npx spawn count (PATH shim) |
20 — 10 --listFilesOnly + 10 --showConfig — nit 3 |
1. The guard that now gates the whole repo's typechecking has zero tests — and 29 rounds of hand-probing is what has been standing in for them
scripts/verify-typecheck-coverage.mjs (616 lines) + scripts/lib/npm-scripts.mjs (89) are the largest piece of new logic in this PR, and nothing tests either. That isn't an oversight in the usual sense — scripts/ is structurally outside every gate the repo has:
$ git ls-files | grep '^scripts/' # 12 files, all .mjs
$ grep -rl 'verify-typecheck-coverage\|npm-scripts.mjs' --include='*.test.*' .
(nothing)
scripts/ is prettier-gated only (format:check:scripts) — AGENTS.md says so explicitly, since the root has no eslint config for .mjs — and no client's test:coverage include reaches it. So the ≥90%-per-file gate this repo enforces everywhere else doesn't apply here at all.
What makes that worth raising now rather than as a general observation: the review thread is the empirical case. Across rounds 5–27 the guard shipped, by my count of this thread, nine false passes (r5 unwired gate, r10 --noCheck, r14 fail-open CLIENTS derivation, r15 renamed typecheck, r16 prefix-sibling dir match, r20 run validate matching validate:fast, r21 stale SHARED_ROOTS, r25 tsc -b --noCheck, r27 noCheck in a referenced project) and six false failures (r11, r13, r17, r18, r23, r26). Every one was the behavior of a pure function on a specific input string or config graph. And r18's was a regression introduced by r17's own fix — tokens.includes("tsc") losing the path-invoked node_modules/.bin/tsc that the previous \btsc\b matched. That is precisely the failure mode a table-driven test prevents, and the only thing that caught it was someone thinking to re-probe an input the previous round hadn't.
The split in effort required is worth knowing, because half of it is free:
scripts/lib/npm-scripts.mjsis testable right now, no refactor. It exports four pure functions, and the three interesting ones each encode a rule that took multiple rounds to get right:tokenize(quoting — r17),reachableScripts(pre/posthooks — r10),rootRunsClientValidate(dir anchoring — r16;validate:fasttail anchoring — r20;--prefix— r19;./normalization — r15; quote stripping on both halves — r18). A ~30-case table over(rootScripts, clientDir) → booleanpins all six of those at once.verify-typecheck-coverage.mjsneeds a small refactor first. I verified it exports nothing and runs on import —await import(...)printedverify:typecheck-coverage — OK: all 894 …. SotypecheckProjects,tscBuildStatus,tsconfigReferences,resolveLeafProjects,isTsc,isDisablingFlag,isRequiredSourceare all unreachable by a test. Exporting them and moving the module-initnodeClients()call + theprocess.exittail behind amain()invoked only when run directly would make the parsers testable without touching their logic.
The honest complication, which is why this is a follow-up and not a change request: scripts/ has no test harness. The root has no vitest devDependency, no vitest.config.ts, and no test script — deliberately, per AGENTS.md ("The repo root has no aggregate test script — each client self-validates"). So this needs a decision, not just a file: a minimal root vitest project for scripts/ (and a coverage-chain entry to keep it honest), or accept the gap and say so in AGENTS.md next to the format:check:scripts note. I'd file it in the #1791/#1792 cluster; the npm-scripts.mjs half is small enough to be most of the value.
Nits
2. A scripts/*.ts is required by the guard but cannot satisfy it — the remediation names a home that would be wrong. Deny-by-default (round 21) requires every non-client tracked TS to land in the global union of client projects. scripts/ is the one first-party location where no client project can legitimately reach. Verified:
$ printf 'export const bad: number = "nope";\n' > scripts/probe-ts.ts && git add -N scripts/probe-ts.ts
$ node scripts/verify-typecheck-coverage.mjs
verify:typecheck-coverage — 1 tracked source file(s) get no `tsc` pass:
scripts/probe-ts.ts — in no client's tsconfig project
Add the file to a client's `tsconfig.json` / `tsconfig.test.json` `include` … # not a real option
Fail-closed and loud, so no hole — and it doesn't fire today because the dir is uniformly .mjs, which is a reasonable local convention rather than a deviation this PR introduced. But it does mean the guard now makes that convention effectively mandatory (a scripts/*.ts cannot be made green without pointing a client project at root tooling), which sits a little awkwardly against AGENTS.md's "Use TypeScript for all new code". One clause in the failure text — for root tooling under scripts/, keep it .mjs, or give it its own tsconfig project and enroll it here — would say which of the two the repo means. Same remediation-accuracy class as rounds 13/22/23.
3. Half the guard's cost is npx process startup — 20 spawns, ~3.8s of the ~16s. The PATH-shim count is exactly 10 --listFilesOnly + 10 --showConfig (one pair per distinct leaf, so the rawFilesCache is doing its job). Measured overhead per spawn in clients/tui:
npx --no-install tsc --version ×3 → 0.729s (~0.24s each)
./node_modules/.bin/tsc --version ×3 → 0.164s (~0.055s each)
≈0.19s × 20 ≈ 3.8s of pure npx resolution, in a guard that now runs first in the inner-loop validate. --no-install correctly prevents a registry fetch (matching verify-build-gate.mjs), but that script makes one spawn where this makes twenty. Resolving the binary once and calling it directly — createRequire based at the client's package.json, the pattern smoke-web-browser.mjs already uses for playwright, then execFileSync(process.execPath, [tscJs, …]) — keeps the "must be locally installed" property while dropping the per-call cost. Worth noting round 27's ~11s claim was already flagged as not reproducible; this is where most of the difference lives.
4. Round-28 items, both confirmed. checkingLeaves tracks visited leaves in a Set and continues at the top, so a leaf reachable from two harvested projects yields one integrity line and one --showConfig (I re-ran the duplicate-project probe: 1 gate-integrity issue(s), previously 2). The change is strictly an improvement over the old else if (!checking.includes(leaf)) — the non-disabling dedup behavior is unchanged and only the integrity.push/spawn path got deduped. AGENTS.md now records that the per-file // @ts-nocheck tier is owned by @typescript-eslint/ban-ts-comment across lint:core, lint:shared, and each client's eslint . — accurate, and it's the right thing to have written down, since it's the one tier no tsc pass can see.
5. Config-shape check on the PR's own scaffolding, done fresh. All three tsconfig.test.jsons resolve to noEmit: true in their merged config (not just via the --noEmit on the typecheck command line), so nothing can emit even if invoked bare — I checked with --showConfig rather than reading the files. Each include names only what its sibling src project can't reach (__tests__/**/*, plus launcher's vitest.config.ts and cli's explicit server-composable.ts), and each test config's paths correctly re-lists the base's entries, since tsconfig replaces paths wholesale rather than merging it — that's the kind of thing that fails silently, and both cli's and tui's get it right with a comment saying why. The **/*.test.* excludes are uniform across all three src configs.
6. Test-side read, unchanged verdict. The PR's actual subject: makeFakeCliOAuthClient/makeFakeServerSettings with defaults typed against the real signatures and honest caveats on what the typing does not reach (overrides, and Partial admitting an explicit undefined); the uniform Parameters<InspectorClient[…]> wrappers and typed spies with the accurate "toHaveBeenCalledWith is runtime-only" note; entry() fully gate-checked with MALFORMED's single scoped cast as the file's only assertion; the vi.spyOn(process.stderr) refactor; the r.headers?.[…] guards; the AuthTab fixture's newly-required OAuth fields; and the @inspector/core/... specifier fix that started all of this. One thing I checked specifically because it's the kind of substitution that can quietly change what a test exercises: makeFakeServerSettings() replacing {} at ~15 call sites does not default enterpriseManaged (it's an override-only field), so the isStandardOAuthStepUp / isStepUpConfirmation assertions still exercise the same branches. Nothing to change.
Twenty-nine rounds in. The invariant — every tracked .ts/.tsx/.mts/.cts in every client, plus all the first-party TS no client owns, gets a real tsc pass — is real and guarded on four axes (coverage, wiring, non-inertness, enrollment), and I've confirmed each bites. Two consecutive rounds have now found no new hole in it, which is the signal to stop probing. Finding 1 is what should replace the probing: the guard has earned a test suite, and the shared lib's half of it is nearly free. Ship this, file that.
· branch v2/1791-typecheck-tests
1 participant
Closes #1791
Summary
The cli/tui
typecheckscripts (tsc --noEmit -p tsconfig.json)includeonlysrcandexclude**/*.test.*, so the__tests__dirs — which do the mostascasting — got notscpass. This adds atsconfig.test.jsonper client (mirroringclients/web/tsconfig.test.json) that includes__tests__with the test-exclusions dropped and the test-only path aliases resolving what vitest resolves viavitest.shared.mts, then foldstsc -p tsconfig.test.jsoninto eachtypecheck.Fixing the surfaced errors turned up a real latent bug: a
@modelcontextprotocol/inspector-core/mcp/index.jsimport specifier in two cli test files that should be@inspector/core/.... It only ever appeared in aimport type, so vitest (which strips types) never resolved it — exactly the class of mistake this gate is meant to catch.Changes
Scaffolding
clients/cli/tsconfig.test.json,clients/tui/tsconfig.test.json— extend the src-only config, add__tests__, drop the**/*.test.*exclusions, and add the test-only aliases (@modelcontextprotocol/inspector-test-server,@inspector/core/*deep paths, express/vitest). Wired into each client'stypecheck(sovalidatecovers it).clients/cligains@types/express(devDep) so the transitively-aliased test-server source typechecks, the same wayclients/webalready does.cli fixes
@modelcontextprotocol/inspector-core/...→@inspector/core/...specifier (fixtures.ts,cli.test.ts).r.headers?.[…]optional-access guards for theRecordedRequest.headers?field.makeFakeCliOAuthClient()/makeFakeServerSettings()(__tests__/helpers/oauth-test-fakes.ts) replace partial OAuth-client / settings mocks — per the AGENTS.mdas-cast policy — and theoauth-interactivestderr monkeypatch is refactored tovi.spyOn.tui fixes
(...a: unknown[]) => …generic signatures on the App OAuth spies (fixes the spread-argTS2556s and lets a testmockResolvedValueanyAuthChallengeOutcomevariant rather than only the impl's"satisfied"literal), and a widened callback param (iss?).makeFakeServerSettings()factory (__tests__/helpers/server-settings.ts) for the tuiOAuth tests.client.hasClientSecret+ authorization-server-metadata fields in AuthTab.entry()helper'smessageparam tounknown(it already casts its return) for the deliberately-malformed wire messages HistoryTab's defensive branches render.Gate coverage only; no runtime behavior change.
Testing
npm run cipasses (validate → coverage → verify:build-gate → smoke → Storybook).🤖 Generated with Claude Code
https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5