test(build): sweep every tsc program for file-level @ts-nocheck, not one directory - #386
Merged
Conversation
…*` stops at Config discovery descended into dot-directories, which is the one place tsc's `**` provably never goes and the one place this repo's tooling parks throwaway projects. A `/review-cycle` reviewer's probe tsconfig under `.review-cycle-<id>/` joined the roster and reddened the suite; an unparseable one threw out of the describe body and took collection for the whole file down. Skip dot-prefixed directories as a rule instead of growing the denylist, which also makes `.git` and `.vscode-test` redundant in SKIP_DIRS. Pinned by a temp-dir fixture rather than by the comment. Four further gaps closed in the same file: - `sweepProject`'s `root` was optional. A root that does not contain the config makes `repoRelative` reject every file, so the sweep returns empty while reporting success — "this project is clean" and "this sweep saw nothing" become one value. Required now, explicit at the single repo call site. - The anti-vacuity floor was a union floor of 300, which `test/webview` clears alone with 323 files; six of seven programs could shrink to their anchor and stay green. Floored per project at roughly half each measured count, reported as the list of offenders so the diff names the program that shrank. - The 32 directive fixtures asserted one at a time, so a regression printed `expected false to be true` for one of 24 prefixes, several of them invisible characters. Filter to the offenders instead, the idiom the file already uses. - The per-project filter rebound `swept`, shadowing the union of the same name that the sibling tests read, on the line where the test's own comment draws that exact distinction. Discovery is now run once and shared: the roster, the compile cross-check and the sweep assert against each other, so they must be talking about one set.
…t measures
This guard's recurring defect is prose asserting coverage the code does not
have. Review found ten instances in its own comments; each is corrected against
a measurement rather than reworded.
- The file header still scoped the tripwire to "this directory" while the sweep
is repo-wide, and `test/build/tsconfig.json` said the opposite. Rewritten to
state the real scope and its limit: it answers for the files a program
CONTAINS, and 103 tracked sources sit in no program at all.
- The `startsWith` counter-example did not hold as written: with the trailing
separator the comment itself spelled, `"/repo-backup/a.ts"` correctly fails
the prefix test. It is the slash-less form that matches.
- The transitive-reach example named `src/shared`, which
`test/extension/tsconfig.unit.json` includes explicitly. The files reached
only through imports are `src/extension/**` and `src/markdown/table/model.ts`.
- Four comments deferred their evidence to `.claude/plans/` documents, which
are gitignored and unreachable from a `quoll` checkout. The costliest was the
sole explanation of a load-bearing invariant (do not `realpathSync` one side
of the root comparison); all four now state the reason inline.
- Two internal pointers aimed the wrong way: the roster assertion is below, not
above, and "the read layer below" survived the layer's deletion.
- "Would build all seven twice" undercounted: the describe holds four
assertions, so re-deriving per `it` is four full sweeps, ~6.4 s.
- "Seven configs in the repo's life" contradicted the neighbouring measured
"eight configs", and "roughly once a year" was guessed — git records two
additions in the repo's two months. The cadence claim is dropped rather than
re-guessed.
- A line-number citation into `node_modules/typescript/lib/typescript.d.ts`
under a caret range would drift on any patch bump; the load-bearing half
("is public") needs no line number.
- The `ts.sys.readFile` branch now records why its message names no cause:
missing, is-a-directory and chmod-000 all measure as `undefined`.
- "No cast anywhere" is scoped to the route it describes, since the file carries
three casts elsewhere.
`mergeSweeps` declared three Sets and filled them with three structurally identical nested loops. One set construction per output field says the same thing; `files` stays a Set because callers use `.size` and `.has`. Move `type Sweep` above the comment block, which describes `mergeSweeps` rather than the type it was attached to. Also pin why the encoding fixtures' `bytesOf`/`concat` helpers are not the redundant wrappers they resemble: with this repo's @types/node a `Buffer` is a `Uint8Array<ArrayBufferLike>` and does not assign to the `Uint8Array<ArrayBuffer>` that `writeFileSync` and `utf16be` speak in, so folding them into `Buffer.from`/`Buffer.concat` costs four TS2345s under `pnpm compile` — while vitest, being transpile-only, still reports every test green. Measured, not assumed.
Three comment claims in the repo-wide @ts-nocheck guard asserted more than the code delivers. - test/build/tsconfig.json said dist/out/coverage are "the same places tsc's own `**` will not look, so no program can live there". Measured with the guard's own API: an `include: ["**/*.ts"]` with no `exclude` returns dist/f.ts, out/f.ts and coverage/f.ts, and skips only node_modules and dot-directories. Those three are excluded by SKIP_DIRS, a repo-curated denylist, so the header now attributes each exclusion to its real authority and states the reach plainly: `**`-glob discovery plus the programs named by `compile`/`compile:webview`. A dot-rooted project invoked directly is out of reach by design, since the dot-directory skip is what keeps throwaway probe tsconfigs out of the roster. - "Each floor is roughly half its measured count" is false for test/build, whose floor is 12 of 18 (two-thirds). Re-measured all seven: the other six do sit at ~0.5. - mergeSweeps returns `files` as a Set while optOuts/declarations are sorted arrays; the reason (Set is only queried by .has()/.size, the arrays are compared with toEqual) was stated nowhere. Comments only — no assertion or behaviour changes.
The fixture planted a `// @ts-nocheck` under `.scratch/` and asserted only `discoverProjects`, so the directive was inert — deleting it changed no outcome — while the comment narrated it as pinning the second half of the failure (a stray project is also swept, so its files reach `optOuts`). Sweep what discovery returned, through the same `mergeSweeps` composition the suite body uses, and assert `optOuts` is empty. The planted directive is now load-bearing. Non-vacuity, measured: renaming the fixture's `.scratch` to `scratch` turns the new assertion red with `["scratch/b.ts"]` against `[]`. Deleting the dot-directory skip in `findConfigFiles` does NOT work as a check — measured, it takes collection down to `0 test` on `.vscode-test`'s bundled VS Code (TS5083), which is the cycle-1 failure that skip exists to close. Both measurements are recorded at the assertion.
The comment recorded a one-step recipe — rename the fixture's `.scratch` to `scratch` — and claimed it reddens the sweep assertion with `["scratch/b.ts"]`. Measured: the rename reddens the DISCOVERY assertion above it first, so the sweep assertion is never reached and the recipe proves nothing about the line it annotates. Reaching it takes two edits, the rename plus widening the discovery expectation. Recorded as measured, with what the one-step version actually produces, since a recipe that cannot be followed is the same defect class this file exists to catch.
…to pointer The header kept re-deriving, in free-form English, mechanics that `findConfigFiles`, `SKIP_DIRS` and the compile cross-check already state once each. Three review cycles of that re-derivation shipped three sets of wrong claims, the latest being a regression introduced by the previous correction: it sorted `node_modules` into the same repo-curated denylist as `dist`/`out`/`coverage`, though measured, an `include: ["**/*.ts"]` with `exclude: []` returns files from those three and nothing from `node_modules` — which only enters when named explicitly, exactly like a dot-directory. So the header now names the two gates by function in one sentence, states the one gap they leave (a dot-rooted project invoked outside `pnpm compile`), and defers every boundary detail to the code that enforces it. The two remedies it used to suggest are dropped rather than reworded: the cross-check asserts `referenced ⊆ discovered` over a `discovered` set `findConfigFiles` can never populate with a dot-rooted config, so wiring one into `compile` — or widening what feeds `referenced` — only turns that assertion permanently red. The authority it defers to now carries the distinction: the `SKIP_DIRS` comment splits the two classes and records the measurement for each, including why `node_modules` stays on the list anyway (this walk is a plain `readdirSync` recursion, not a tsc glob). References are by assertion and symbol name, never line number.
The SKIP_DIRS comment said dropping `node_modules` would let the walk descend into 30 vendored tsconfigs. Measured: all 30 sit under `node_modules/.pnpm/`, so the dot-directory skip above already catches them and dropping the entry discovers zero extra configs. The entry still earns its place — a hoisted `node_modules/<pkg>/tsconfig.json` carries no dot segment — but that is a reason about layouts we do not control, not a measurement of this checkout, and the comment now says which it is.
The comment now claims `dist`/`out`/`coverage` are repo curation that tsc's own `**` would happily reach, while `node_modules` sits with the dot-directories `**` refuses. Both are claims about TypeScript, and unpinned claims about TypeScript are exactly what this branch got wrong sixteen times across three review cycles — every one a sentence no assertion could contradict. Assert them instead, through the guard's own config reader: a bare `**` with `exclude: []` returns the three and neither refused name, and naming either one explicitly brings it in. A release that moves a name between the classes now goes red. `exclude: []` is load-bearing in the fixture — tsc's default exclude already names `node_modules`, so without the override the default would be doing the work the `**` token is under test for.
The comment credited only the dot-skip, and then justified keeping the `node_modules` entry with "a hoisted `node_modules/<pkg>/tsconfig.json` carries no dot segment, and nothing else would stop the walk". Measured with this walker: pnpm's hoisted top level is symlinks, and `entry.isDirectory()` reports a symlink as false, so the walk does not enter them either — two independent mechanisms, not one, and the second is the non-obvious one. Dropping the entry still finds zero extra configs. It earns its place for npm and yarn, which materialise real package directories where neither mechanism applies.
… claim about it The fixture added last commit claimed `exclude: []` was load-bearing because tsc's default exclude already names `node_modules`. Measured through the fixture's own reader: dropping the key leaves all three results identical — the absent-`exclude` default is only `[outDir, declarationDir]`, and the refusal under test lives in the `**` token itself, beside the dot-segment refusal. The override stays as insulation against that default widening, and now says so. Two further gaps closed: - The `node_modules` entry in SKIP_DIRS pinned nothing: pnpm hides packages behind `.pnpm/` and reaches them by symlink, so deleting the entry reddens nothing on this checkout. A fixture builds the hoisted layout npm and yarn produce — a real `node_modules/pkg/` with no dot segment — where the entry is the only thing stopping a `readdirSync` recursion. Removing the entry now goes red there. - The tsconfig header's two-gate sentence omitted the walk's third boundary, its filename pattern, which made the "what escapes both" clause an undercount. It now names all three and says what escapes accordingly.
…ot on a throw The vendored config it plants carried `files: []`. Removing the `node_modules` entry from SKIP_DIRS did turn the test red, but via TS18002 thrown out of `readProject` before the assertion ran — so the fixture was pinning the error path, not discovery. If that throw ever stops being fatal the test goes quietly green and the entry it exists to pin is unpinned again, in the same file that records the entry as inert on this checkout. Plant a config that parses instead; the failure is now the assertion itself.
The taxonomy listed three ways a program can be invisible to `findConfigFiles` — dot-directory, `SKIP_DIRS` name, filename pattern — and omitted the fourth: the walk tests `entry.isDirectory()`, which reports false for a directory symlink, so a config reachable only through one is never seen. Measured. The omission was conspicuous because the sibling `SKIP_DIRS` comment, added in the same series, documents that exact mechanism doing load-bearing work on this checkout, where pnpm reaches every package through a link.
The comment justified using a parseable config by claiming the alternative "would go quietly green the day that throw stops being fatal". Measured: with the throw removed, the unparseable config still surfaces in discovery and the old fixture still goes red at the assertion. Silence needs a narrower change — discovery catching and dropping configs it cannot parse. The reason to prefer a parseable config stands either way, since the old form pinned `readProject`'s diagnostics while reading as though it pinned the walk; only the size of the future risk was overstated, which is the defect class this file exists to stop.
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.
Why
test/build/no-file-level-ts-nocheck.test.tsasked the TypeScript API the right question — "is this file opted out?" — but only ever asked it abouttest/build. The failure mode was never directory-scoped: one file-level@ts-nocheckinsrc/sharedswitches that file off in every program that includes it, andpnpm compileplus CI stay green throughout.test/build/tsconfig.json's ownincludealready reaches../../src/shared/**/*.ts, so the guard was blind to files its own project compiles.What changed
The guard now hands the two questions it used to answer itself back to tsc:
discoverProjects()finds everytsconfig*.jsonon disk and treats the ones no other configextendsas the programs (today: 7). Each is built withts.createProgramand swept viaprogram.getSourceFiles(). That is a superset ofpnpm compile's 5 projects, and it catches files reached only transitively by import, which anincludeglob never would.readAsTypeScriptWould(BOM strip + UTF-16LE/BE decode + odd-byte trim) is deleted. The compiler host's own read is used instead, so BOM/UTF-16/shebang handling can no longer drift from tsc.Both hand-rolled layers are gone rather than widened. They are what produced the four silent gaps found in PR #382's review cycle.
Keeping the guard honest in both directions:
package.jsonparses thetsc -ptargets out ofcompile/compile:webview(6 of them) so a config wired into CI but misclassified by discovery goes red even if someone "fixed" the roster to match the broken output.skipLibCheckleaves a.d.tsunchecked with no directive at all;src/shared/quoll-perf-flag.d.tsis the single allowlisted exemption.test/markdownandtest/sharedremain unswept — they are in no tsconfig, so they appear in no program. That is the separate, already-documented hazard, not a gap in this sweep.Divergence from the TODO entry's suggested implementation
The entry proposed lifting the read helper into a shared module and shelling out to
tsc -p <config> --listFilesOnly. Neither is used, deliberately: with aProgramin hand the read layer is dead, not shared, and the in-process API returns the same file list in ~1.7 s without spawning seven compilers. The Done-when — which is the acceptance contract — is met either way.Acceptance evidence
Done-when clause 3, run by hand: plant
// @ts-nocheckat the top ofsrc/shared/protocol.ts—Restored,
git status --porcelainempty, guard green again:The encoding fixtures were separately proven non-vacuous by swapping the sweep for a naive
readFileSync(..., "utf8")reader: the BOM+shebang, UTF-16LE, UTF-16BE and UTF-16BE-odd-byte cases all go red, while the plain-UTF-8 control stays green.Gate
pnpm compilepnpm test:unitpnpm test:browserpnpm linttest/markdown/validate-for-write-incremental.test.ts)git status --porcelainTest-only plus a doc header; no runtime code is touched, so steps 3–4 of the local install gate do not apply.