fix(scripts): a test file is source — the Bun-only rule stops skipping it - #382
Merged
Conversation
…g it `checkNodeImports` opened with `if (isTestPath(file.path)) continue` at both of its walks, from the day it landed. So the SCANNER read every test file and the RATCHET dropped every finding: **404 unexplained `node:` imports across 164 test files**, under a green `bun run node-imports`, while the pin table said 146. The hole was ASSERTED AS CORRECT by a test named *"a test file is a test — its imports are the harness, not the shipped surface"*, which is how it survived. `CLAUDE.md`'s non-negotiable exempts nothing, and it already records this exact mechanism happening once before: "`checkErrorFixes` skips test files, so the rule was prose there and 422 sites accumulated under a green gate". `storage` is the proof it mattered — review flagged two of its test files on #364 and `storage` had no row in the pin table at all. A FIXTURE IS EXEMPT, and both carriers are. `maskLiterals` blanks string contents and comment text alike while preserving every offset, so a match survives it exactly when the process would really evaluate the import. The old line-prefix test (`//` or `*` at the start) missed a specifier inside a template literal — `packages/cli/src/templates/` emits app source the CLI writes and never runs — and `async-context-guard.test.ts:106` explains its shape by quoting it. Two rows FELL as the rest rose: `cli` lost 4 template sites, `scripts` lost 25 fixtures. Swept 545 -> 209. Nine sites were CONVERTED rather than annotated — `Bun.file(p).exists()`, `Bun.file(p).text()` and `Bun.write()` (which creates intermediate directories) retired seven whole `node:fs` imports. The rest carry the sentence, because Bun 1.4 has no `tmpdir()`, no `mkdtemp`, no recursive remove and **no path API at all**: `Object.keys(Bun)` has `file`, `write`, `Glob`, `pathToFileURL`, `fileURLToPath`, and nothing that joins a path or makes a directory. This is the one time a number in `node-import-pins.ts` may rise, and it rose because the rule started reading files it was always written to read. `scripts/node-imports.test.ts` holds the 2026-08-26 ceiling and refuses a raise past it. Fixes #365 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
On-demand reviews are free for the next 25 days. After that, they cost $0.25 per reviewed file. Or wait 27 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 76 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (139)
Comment |
Contributor
|
Looks good — CI is green, this is a patch-level bug fix to the node-imports rule, and no risk signals apply. Ready for merge when you're ready. Feel free to tag with 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
Contributor
Author
|
@coderabbitai review |
|
Contributor
Author
|
Merging without a CodeRabbit review, and recording why. Its included-review allowance is exhausted (76 attempts over 7 days → 1 review/hour), and both an automatic pass and an explicit What stands in its place:
Nothing here is unreviewable-and-unverified; it is reviewed-by-machine-gate and unreviewed-by-CodeRabbit, which is a different claim and the accurate one. |
|
Tip For best results, initiate chat on the files or code changes.
The CodeRabbit review did not run because of the documented rate limit. The PR has independent verification from the CI checks, The statement correctly distinguishes verified changes from an executed CodeRabbit review. You are interacting with an AI system. |
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.
Closes #365. The test-file half of the sweep whose scanner-correctness half merged as #381.
The rule read every test file and threw the answer away
checkNodeImportsopened withif (isTestPath(file.path)) continueat both of its walks, from the day it landed. The scanner worked fine; the ratchet dropped its findings. Measured: 404 unexplainednode:imports across 164 test files, under a greenbun run node-imports, whilenode-import-pins.tssaid 146.The hole was asserted as correct by a test named "a test file is a test — its imports are the harness, not the shipped surface". That is how it survived a year of edits: the rule that would have caught it had a unit test saying it should not.
CLAUDE.md's non-negotiable exempts nothing — and it already records this exact mechanism happening once before:storageis the proof it mattered in practice: review flagged two of its test files on #364 for exactly this, andstoragehad no row in the pin table at all.A fixture is exempt, and both carriers are
maskLiteralsblanks string contents and comment text, preserving every offset — so a match survives it exactly when the process would really evaluate the import. It is the maskrender-modes,frozen-records,secret-compareandproto-indexalready read, so no second tokenizer enters the tree.The old test was a line prefix (
//or*at the start), which misses:packages/cli/src/templates/emits app source the CLI writes and never runsasync-context-guard.test.ts:106explains its shape by quoting itTwo rows therefore fell as the rest rose:
clilost 4 template sites,scriptslost 25 fixtures. Both are the mask, not a sweep.Swept 545 → 209
Nine sites converted rather than annotated —
Bun.file(p).exists(),Bun.file(p).text()andBun.write()(which creates intermediate directories, so it ismkdir -ptoo) retired seven wholenode:fsimports.The rest carry the sentence, because Bun 1.4 genuinely has no replacement: no
tmpdir(), nomkdtemp, no recursive remove, and no path API at all.Object.keys(Bun)hasfile,write,Glob,pathToFileURLandfileURLToPath— nothing that joins a path or makes a directory.node:pathis 0-of-127 convertible today, which is the honest reasonclistill holds 95 andscripts28: those are shipped source, and both packages' test-file sites are now entirely answered (336 of them).cliscriptsThe one time a pin may rise
It rose because the rule started reading files it was always written to read — not because anything regressed.
scripts/node-imports.test.tsholds the 2026-08-26 ceiling and refuses a raise past it, so the ratchet only falls from here.Verified
bun run verify— 14 of 20 passed, 6 skipped (drift, contract-diff, budgets, seo, i18n, policy), the documented root baselinebun run scripts/reference-app-gate.ts— every pin holds:examples/dummy18/20 (2 pinned),dummy/social-media-clone18/20 (2 pinned)bun run node-imports --jsonre-derives the table;CLAUDE.mdandwiki/Error-Codes.mdare updated to say what it now measures, and to say--jsonis what to read🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.