test(build): type-check test/build as a 5th tsconfig project - #378
Merged
Conversation
test/build/ sat in no tsconfig: the root config excludes "test" wholesale and test/extension/tsconfig.unit.json includes only the protocol drift guard, so the whole directory was transpile-only under vitest. Every type-level assertion there was permanently vacuous — proof already on disk in the shape of a @ts-expect-error that could never fire. Add test/build/tsconfig.json (Node + ESM compiler view, no DOM lib) and append it to the compile chain. Clearing the four pre-existing errors it surfaces: - theme-palettes.test.ts had its @ts-expect-error above a Biome-wrapped multi-line import. TS7016 is reported at the module specifier, so the directive covered the wrong line and produced both an unused-directive error and the live error it meant to suppress. A namespace import keeps the specifier on the directive's line no matter how many bindings the file destructures. - notice-covers-bundled-deps.test.ts was missing the directive entirely on its untyped esbuild.config.mjs import, and Object.values() over the resulting any widens each entry to unknown — annotated at the boundary with the esbuild.BuildOptions[] shape the suite actually requires. Non-vacuity measured by deleting each new pin and watching the new project go red. Five suites in the directory still carry a file-level @ts-nocheck for their untyped .mjs import, which switches the whole file off; swapping those to the line-scoped directive costs 31 annotations and is tracked separately to keep this change to one purpose.
Adding test/build/ as a tsc project invalidated several nearby comments that asserted the opposite, and the review surfaced one annotation that reads as a checked pin but is not. - notice-covers-bundled-deps.test.ts: the `esbuild.BuildOptions[]` annotation looked like a compile-time pin on createBuildConfigs, but contextual typing feeds the target type back into Object.values<T>'s inference, so `any` satisfies it trivially. Spelled as an `as` cast instead, which is honest about being an unverified assertion. It is still load-bearing: without it the call is unknown[] (TS2345). - quoll-perf-flag.d.ts: "All four tsc programs" is now five. - publish-workflow-sbom-config.test.ts: the comment said its Record<SharedStep, RegExp> exhaustiveness check "never executes" because test/build/ is in no tsconfig. It executes now — this file carries no @ts-nocheck. The runtime assertion stays as a backstop, with the reason stated. - doc-sync / stale-todo-markers / todo-hygiene / preview-server-theme: all four justified their @ts-nocheck with "tsc does not include test/build/", which is no longer true of the directory. - tsconfig.json: replaced the hand-maintained five-filename list (also duplicated in CLAUDE.md) with a git grep the reader can run, and fixed the rootDir rationale — rootDir plays no part in module resolution, only emit layout and the TS6059 containment check. Measured: dropping the "Generate SBOM (SPDX)" entry yields TS2741 at the LOAD_BEARING literal; removing the cast yields TS2345.
… rationale The perf-flag comment named a literal program count that this PR itself had to bump, and it was under-counting anyway (six programs include src/shared, not the five in the compile chain). State the property instead of the number. test/webview/tsconfig.json claimed rootDir is why its cross-layer imports resolve — the same false mechanism this PR corrected in the new config, which now sits next to it contradicting it. rootDir governs emit layout and the TS6059 containment check only.
Cycle 2 of the review found that cycle 1's corrections had introduced two false statements of their own. The perf-flag comment had been broadened to "every tsc program in the repo includes src/shared/**". There are seven programs, not the five the compile script names: test/extension/tsconfig.json is the E2E emit config, rootDir ".", and pulls in no src/ file at all. Scope the claim to the programs that compile a QUOLL_PERF reference, name the exception, and point at `git ls-files '*tsconfig*.json'` as the way to check it -- reading the compile script misses the configs no script chains, which is how three separate passes over this PR reached the same wrong answer. test/build/tsconfig.json still justified omitting the @ts-nocheck roster by citing a duplicate list in CLAUDE.md that cycle 1 had itself deleted. Comment-only: there is no behaviour here to pin, which is why the tests that guard these facts land in a separate commit.
Every type-level guarantee in this repo holds only because `compile` runs tsc
over the project that owns it. Drop a project from that chain -- a reformat, a
bad merge, a "tidy the scripts" pass -- and build, test and CI all stay green
while every assertion that project gated goes permanently vacuous. That silent
revert is the failure mode test/build/tsconfig.json was added to close, and
nothing pinned the wiring itself until now.
Extracts the -p arguments rather than substring matching: `toContain("tsc -p
./")` is satisfied by any of the four nested project paths, so the root pin
could never have gone red.
Also pins the exception named by src/shared/quoll-perf-flag.d.ts -- the E2E
program compiles no src/ file. Widening that include silently falsifies the
comment; now it fails here instead.
Verified red:
compile chain: expected [ './', ...(3) ] to deeply equal [ './', ...(4) ]
E2E exception: expected [ '../../src/shared/**/*.ts' ] to deeply equal []
The bundle-step assertion had the exact hole this file warns about for `tsc -p ./`: "pnpm compile" is a prefix of "pnpm compile:webview", which `build` also runs, so toContain() stayed green after the compile step was deleted outright. Split on && and compare whole steps, and pin the ordering against the esbuild step rather than implying it in the test name. Verified red by removing `pnpm compile &&` from `build`: AssertionError: expected [ 'pnpm compile:webview', ...(1) ] to include 'pnpm compile'
…nfigs as JSONC Cycle 3 found the tripwire too loose to catch the failures it was written for. Extracting `-p` arguments never saw what sat between the steps, so three mutations left it green while the gate was dead: `;` instead of `&&` (in sh the exit status is the last command's, demoting the first four projects to non-gates), `|| true` on a step, and an `echo` in front of one. Pin the whole script by exact equality instead -- strictly stronger and simpler than the regex it replaces. The ordering test pinned `pnpm compile` but not `pnpm compile:webview`, which is the only tsc pass over src/webview and gates the same dist/webview/ output; moving it behind esbuild stayed green. Pin both gates, and guard the bundle step's index so a missing step fails closed. tsconfig reads now tolerate JSONC. Only whole-line `//` comments are dropped, so a `//` inside a string literal survives; test/extension/tsconfig.json is the only comment-free config of the eight, making a future doc comment likely -- and JSON.parse would have turned that into a SyntaxError, breaking `pnpm test` on a documentation edit. Also corrects two false claims in test/build/tsconfig.json: the roster is named in LEARNING.md as well as TODO.md, and the unanchored grep returns eight files, not five -- three are prose about the directive, including two suites that say they carry none. `git grep -l '^// @ts-nocheck' test/build` returns exactly the five that do. Verified red: dropping a project, `&&`->`;`, `|| true`, moving compile:webview behind the bundle step, and leaking src/ into the E2E include. Verified green: adding a `//` comment to that config.
…ot redundant Cycle-3 simplify pass. Test 1's assertion was widened from a project roster to a whole-script pin, but its name still described only half of what fails it. The comment on `toContain` guards a live trap: a missing gate makes indexOf return -1, and -1 satisfies toBeLessThan, so the ordering assertion alone would pass silently. That line reads as redundant and is the one a future simplify pass would delete. Also reflows two comment blocks in test/build/tsconfig.json that the previous edit left broken mid-phrase.
Codex and Fable were consulted independently on whether to fix these or stop; both returned FIX_NOW on all three, and both judged text-pinning the right approach rather than a treadmill -- only three scripts sit upstream of the guarantee, so pinning all three is a fixed point with no surface left to expand into. compile:webview's body was never pinned. `build` calling it means nothing if the script stops type-checking: "echo skip" or a trailing `|| true` left every other assertion green while the only tsc pass over src/webview disappeared. Pin the body exactly, as `compile` already is. The gate-order check split on `&&` and compared indices, which cannot see shell short-circuiting: `pnpm compile && pnpm compile:webview && false || node esbuild…` put both gates before the bundle segment while `||` ran the bundler on failure. Assert the required prefix instead -- it models the real property (the bundler is reachable only through two successful gates) and replaces the split/indexOf logic rather than adding to it. The E2E include filter matched "src/" as a substring, missing a bare-directory include spelled "../../src". Match `src` as a path segment. Verified by 12 mutations: all four previously-green holes now red, all seven earlier holes still red, and two negative controls still green -- an added bundler flag and a JSONC comment, confirming the pins did not widen into false tripwires.
… fails `expect(script.startsWith(prefix)).toBe(true)` reports only "expected false to be true", which tells whoever trips this tripwire nothing. Compare the sliced prefix instead, so the failure prints the injected text -- under the short-circuit mutation the `&& false ||` is visible in the diff itself. Rename the test from "runs both type-check gates ahead of the bundle step" to "reaches the bundle step only through both type-check gates": the old name described the ordering check this commit's predecessor deleted, and ordering is the weaker property that let `&& false || node esbuild…` through in the first place.
…tail Codex and Fable independently found the prefix pin leaves the script's tail free: `… --production || node esbuild…` bundles ungated when a gate fails, and `… --production || true` makes `pnpm build` exit 0 on a failed type-check. The rename shipped in the previous commit therefore claimed more than the assertion delivered. Both proposed constraining the tail -- a grammar, or rejecting `[&|;\n]`. Taken instead: pin `build` exactly, as `compile` already is. Two reasons. The stated rationale for exempting it -- "the bundle step's trailing flags are expected to move for ordinary reasons" -- was never measured and is false: `build` has not changed since the initial commit. And a forbidden-character list is a denylist, while this file's entire history is denylists missing a case; that one would still admit `>`, `$(…)`, backticks and `#`. An exact match has no such gap. The cost is deliberate and now documented: changing `build` requires updating this string, the same contract `compile` carries. Verified by 13 mutations: both reported bypasses red, the three metacharacters a denylist would have missed (`>`, `$(…)`, `#`) red, all cycle-4 regressions still red, and a JSONC comment still green.
…ssed away Rewriting the gate test for exact equality shortened "pinning `compile` alone left the webview half free to drift behind esbuild while this test stayed green" to "BOTH gates matter", dropping the measured episode that explains why the second gate is pinned at all. Restored as a trailing clause, so the paragraph does not grow. Compressing a load-bearing fact into a shorter sentence is how three of this file's four vacuities were introduced; doing it while fixing the fourth was not the intended irony.
…local "compile:webview is the only tsc pass over src/webview" is false: test/webview and test/webview-browser both include ../../src/webview/**/*.ts, so `compile` already compiles that source twice. What is unique is the ambient view -- compile:webview is the only pass using `types: ["vscode-webview"]` with no node, which is what catches webview code reaching for a Node global. The history claim was overstated for the same reason and is narrowed to the check that actually drifted. Also inlines `pkg.scripts.compile`, matching the other two script pins.
The adversarial review measured the boundary rather than assuming it, so write it down where a reader will hit it: the chain is pinned, the contents of each link are not, the test's own execution path is not, and dist/ is still reachable outside the gate by design. A tripwire that is trusted for more than it checks is how the original bug survived -- test/build/ sat outside every tsconfig for months while its @ts-expect-error pins read as enforcement. The per-project content gap is filed as a follow-up; it needs a design, not another assertion.
Cycle 7 found the same claim corrected in one place and left standing 24 lines
below ("the ONLY tsc pass over src/webview"), the correction ITSELF still
overclaiming, and the new boundary note granting protection it does not have.
The gate-2 rationale is now measured rather than reasoned: a `process.cwd()`
probe in src/webview/shell.ts reddens compile:webview AND test/webview-browser
but not test/webview, so the browser program already subsumes what
compile:webview checks. The gate is pinned as defence in depth against an
include-shrink nothing pins -- not because it uniquely catches Node globals,
which was the cited mechanism and is false.
The boundary note claimed the E2E include check was "the one exception" to
shrink/noCheck/strict; it is not an exception to that list at all -- it fires
when that config GAINS a src path. And CI runs `pnpm test:unit`, never the
composite `pnpm test`.
Every one of the five comment defects across this review has the same shape: an
unmeasured explanation of WHY something matters, wrapped around an assertion
that was correct. These edits remove mechanism claims rather than adding more.
Cosmetic only. One comment line ran to 90 columns against the file's 81-column maximum, and "measured" appeared twice in the same paragraph -- the blanket lead-in and, three lines later, the scoped attribution naming which three claims were measured. Kept the scoped one. No assertion, test name or fact claim touched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
test/build/was in no tsconfig — the root config excludes"test"wholesale andtest/extension/tsconfig.unit.jsonincludes only the protocol drift guard — so the whole directory was transpile-only under vitest and every type-level assertion in it was permanently vacuous. This addstest/build/tsconfig.jsonas a 5th project in thepnpm compilechain and clears the four errors that surfaces.Changes
test/build/tsconfig.json(new) — Node + ESM compiler view (no DOM lib, novscode-webviewtypes),includestest/build/**/*.tsplussrc/sharedfor the cross-layer protocol import.noEmitinherited: this is a CI gate, not a build step.package.json— appendedtsc -p ./test/build/tsconfig.jsontocompile.theme-palettes.test.ts— its@ts-expect-errorsat above a Biome-wrapped multi-line import. TS7016 is reported at the module specifier, not theimportkeyword, so the directive covered the wrong line and produced bothTS2578: Unused '@ts-expect-error' directiveand the live TS7016 it meant to suppress. Swapped to a namespace import, which keeps the specifier on the directive's line however many bindings the file destructures below.notice-covers-bundled-deps.test.ts— added the missing directive on the untypedesbuild.config.mjsimport, and annotatedconfigsasesbuild.BuildOptions[]:Object.values()over ananywidens each entry tounknown, notany.Related
test/build/is in NO tsconfig — its type-level assertions are permanently vacuous" (type-analyzer finding on PR ci: rehearse the release SBOM pipeline on every PR #354).doc-sync,preview-server-theme,stale-todo-markers,todo-hygiene,verify-sbom-scope) still carry a file-level@ts-nocheckfor their untyped.mjsimport, which switches the whole file off — so the new project buys them nothing yet. Measured cost of swapping them to the line-scoped directive is 31 annotations across three files, kept out of this PR to hold it to one purpose. Documented in the tsconfig comment and in CLAUDE.md so the config is not misread as proof that every file under it is checked.Test Plan
pnpm compilegreen with the new project as the 5thtsc -pinvocation.@ts-expect-errorintheme-palettes.test.ts→TS7016; dropping theesbuild.BuildOptions[]annotation →TS2345. Both go red only under the new project.pnpm test:unit— 265 files, 5070 tests pass (the namespace-import rewrite is a runtime change totheme-palettes.test.ts).pnpm build,pnpm package(incl. the vsix audit),pnpm lintall exit 0.