Skip to content

fix(scripts): eliminate repository-linter test race #824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tobyhede merged 1 commit into from
Jul 29, 2026
+43 −2
Merged

fix(scripts): eliminate repository-linter test race #824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion scripts/lint-no-hardcoded-runners.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,26 @@ const TARGETS = process.argv.slice(2).length
const NPX_TOKEN = /['"`]npx\b|(?:^|[^a-zA-Z0-9_$])npx\s+[@\w-]/

async function* walk(dir) {
const entries = await readdir(dir, { withFileTypes: true })
// A directory can vanish between the parent's `readdir` and this call. The
// repo's own script self-tests create and delete probe packages under
// `packages/` (`lint-untracked-probe`, `lint-dead-shell-probe` in
// `lint-no-dead-package-paths.test.mjs`) while other suites run, so a walk of
// `packages/` can enumerate one and find it gone a moment later.
//
// Left unhandled this rejects with ENOENT, and Node exits **1** — the same
// code as "found a hardcoded npx". Every caller then reads a crash as a
// violation: CI fails naming a file that is perfectly clean, and re-running
// it passes. The missing-target case deliberately exits 2 for exactly this
// reason ("Exit 2 means the linter could not run"); this is the same
// distinction, one level down. There is nothing to lint in a directory that
// no longer exists, so skip it.
let entries
try {
entries = await readdir(dir, { withFileTypes: true })
} catch (err) {
if (err?.code === 'ENOENT') return

@auxesis auxesis Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

throw err
}
for (const entry of entries) {
const full = join(dir, entry.name)
if (entry.isDirectory()) {
Expand Down
24 changes: 23 additions & 1 deletion scripts/vitest.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,26 @@
import { defineConfig } from 'vitest/config'

// `fileParallelism: false` is load-bearing, not a performance knob.
//
// Several of these suites mutate the real repo tree while they run:
// `lint-no-dead-package-paths.test.mjs` creates and deletes probe package
// directories inside the packages tree (`lint-untracked-probe`,
// `lint-dead-shell-probe`), and `lint-no-hardcoded-runners.test.mjs` walks that
// same tree and writes an `allowlist-probe.mjs` into `scripts/`. Run in
// parallel, one suite's scratch state lands inside another's scan — which is
// how a clean tree produced an `ENOENT: scandir` on a probe directory and a CI
// failure that pointed at nothing.
//
// Note the probe names are deliberately written WITHOUT their directory prefix:
// the sibling `lint-no-dead-package-paths` linter flags that shape when the
// directory does not exist, and these only exist mid-test.
//
// These are eleven small files; serialising them costs a second and removes
// the whole class of cross-suite filesystem race.
export default defineConfig({
test: { include: ['scripts/__tests__/**/*.test.mjs'], pool: 'forks' },
test: {
include: ['scripts/__tests__/**/*.test.mjs'],
pool: 'forks',
fileParallelism: false,

@auxesis auxesis Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

},
})