fix(scripts): eliminate repository-linter test race - #824
Conversation
…shes mid-walk `lint-no-hardcoded-runners.mjs` walks the packages tree with a recursive `readdir`. The repo's own script self-tests create and delete probe package directories in that same tree while other suites run, and vitest ran those suites in parallel — so the walk could enumerate a probe directory and find it gone before recursing into it. Unhandled, that rejects with ENOENT and Node exits 1: the same code the linter uses for "found a hardcoded npx". CI then failed naming nothing, on a tree the identical script had just passed one step earlier, and a re-run went green. The script already draws this distinction for a missing target — "Exit 2 means the linter could not run" — but only at the top level, not one frame down. Observed on three branches before this one (30316404472, 30317589094), which is why it reads as a flake rather than a defect. Two changes: - `walk` treats ENOENT as "nothing here to lint" and skips. A directory that no longer exists cannot contain a violation, and crashing with the violation exit code is the actual bug. - `scripts/vitest.config.mjs` sets `fileParallelism: false`. Several of these suites mutate the real repo tree, so cross-file parallelism is unsound here regardless of this particular pair. Eleven small files; it costs a second. Verified by reproducing the race locally — churning a probe directory while running the linter failed 6/30 before, 0/40 after. One wrinkle worth recording: the first draft of these comments named the probe directories with their `packages/` prefix, which the sibling `lint-no-dead-package-paths` linter correctly flagged as references to directories that do not exist. The names are now written bare.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
auxesis
left a comment
auxesis
left a comment
There was a problem hiding this comment.
Verdict
Small, focused PR: it fixes a real filesystem race (a directory that vanishes mid-walk now exits 0 instead of a raw ENOENT crash that read as exit 1 == "found a hardcoded npx"), plus serializes the scripts suite so the tests stop recreating that race. The change itself looks correct. The one gap both reviews agree on is that neither new contract is pinned by a test — the ENOENT arm of walk() and the load-bearing fileParallelism: false can both regress silently and stay green. Two inline comments below.
Review stats
| Source | Raw | Survived |
|---|---|---|
| claude (claude-opus-4-8) [test-gap] | 1 | 1 |
| codex (gpt-5.5) [test-gap] | 2 | 2 |
- Cross-model overlap: 1 kept finding (the untested ENOENT walk arm) was corroborated by 2 models (claude + codex). The
fileParallelismguard is single-source (codex). - No overflow: 2 kept findings, under the 8-comment cap.
The two changes are non-published tooling (scripts/), so a changeset is not required — no customer-facing package surface changes and no skill is affected.
| try { | ||
| entries = await readdir(dir, { withFileTypes: true }) | ||
| } catch (err) { | ||
| if (err?.code === 'ENOENT') return |
There was a problem hiding this comment.
[2 models: claude, codex] The new ENOENT arm in walk() — the whole point of this PR — has no test. Both arms are new and uncovered: ENOENT -> silently skip, any other code -> rethrow. A future change that turns "vanished directory means nothing to lint" back into an exit-1 crash would still pass the existing suite. The sibling suite already pins the analogous "linter could not run" contract (lint-no-dead-package-paths.test.mjs:162, exit 2 when git is unavailable), so the precedent exists.
Since walk isn't exported, the subprocess style this file already uses fits — copy the script into a probe with readdir stubbed to throw ENOENT and assert exit 0 / OK:
it('skips a directory that vanished mid-walk (ENOENT)', () => {
const probe = resolve(fileURLToPath(import.meta.url), '../../walk-enoent-probe.mjs')
const dir = mkdtempSync(join(tmpdir(), 'lint-hardcoded-runners-gone-'))
const src = readFileSync(SCRIPT, 'utf8').replace(
"import { readdir } from 'node:fs/promises'",
"async function readdir() { const err = new Error('gone'); err.code = 'ENOENT'; throw err }",
)
try {
writeFileSync(probe, src)
const r = runScript(probe, dir)
expect(r.exitCode).toBe(0)
expect(r.output).toContain('OK')
} finally {
rmSync(probe, { force: true })
rmSync(dir, { recursive: true, force: true })
}
})Passes with the fix; fails (or throws a raw ENOENT stack) if the try/catch is reverted. Ideally add a second case stubbing a non-ENOENT code (e.g. EACCES) to pin the rethrow arm too — otherwise it can silently rot into a swallow.
| test: { | ||
| include: ['scripts/__tests__/**/*.test.mjs'], | ||
| pool: 'forks', | ||
| fileParallelism: false, |
There was a problem hiding this comment.
[single-source: codex] fileParallelism: false is load-bearing but only defended by a comment. Deleting or renaming it silently re-opens the cross-suite filesystem race this PR fixes, and nothing catches it. A one-line guard makes the intent enforceable:
import { describe, expect, it } from 'vitest'
import scriptsVitestConfig from '../vitest.config.mjs'
describe('scripts vitest config', () => {
it('keeps script test files serialized', () => {
expect(scriptsVitestConfig.test.fileParallelism).toBe(false)
})
})Passes on the current config, fails if script tests are allowed back into parallel.
Review Feedback AddressedBoth unresolved test-gap findings were addressed in follow-up PR #827. Files modified:
Commit: Validation passed: 134 scripts tests, Biome, and a zero-finding CodeRabbit review. |
2 participants
Summary
Root cause
The deleted-package-shell regression test introduced in 0344e27 creates and removes a probe directory beneath packages. Vitest was running that file concurrently with the hardcoded-runner tests, whose default invocation recursively scans packages.
The scanner could enumerate the probe and then attempt to read it after the other test removed it. The unhandled ENOENT made Node exit 1, which is also the linter's violation exit code, producing a flaky CI failure despite a clean tree.
This surfaced in PR #823 as run 30409513874 / job 90442300248. The fix was previously bundled into #812; this PR extracts it so it can land independently on remove-v2.
Verification
No changeset: internal repository test infrastructure only.