[theCodeDrift]

Closes #694

What

platform-binary.test.ts created packages/cli/src/node_modules/.bin to exercise the resolver's node_modules/.bin tier, while api-deprecated-paths.test.ts walked packages/cli/src with a recursive readdir. Running in parallel, the walk could list src/node_modules and then throw ENOENT descending into it after the other test removed it.

Both sides are fixed:

• platform-binary.test.ts no longer writes under src/. resolvePlatformBinary locates the tier relative to its own module (dirname(fileURLToPath(import.meta.url))/../node_modules/.bin). The test now relocates that module path to <tmp>/rules/platform-binary.ts via a passthrough vi.mock("node:url") (every other fileURLToPath call is untouched), plants the fake binary at <tmp>/node_modules/.bin/sg, and asserts the resolver finds it with source: "node_modules/.bin". The resolver's own path computation is still what is under test, and production code is unchanged.
• api-deprecated-paths.test.ts walks by hand instead of readdir({ recursive: true }), prunes node_modules and generated/ before descending, and skips a directory or file that vanishes between being listed and being read.

Verification

• pnpm build, pnpm typecheck, pnpm lint: clean.
• pnpm exec vitest run test/platform-binary.test.ts test/api-deprecated-paths.test.ts 20 times in a row: 20/20 pass, and src/node_modules never appears.
• Stress: ran api-deprecated-paths.test.ts 10 times while a background loop repeatedly created and removed src/node_modules/x/y/a.ts: 10/10 pass.
• Full pnpm test: 162 files passed, 1 skipped (3163 tests passed, 11 skipped).

Changeset

None. This is a test-only change and ships nothing to consumers.

Note

Commits are unsigned by owner decision (GPG key unavailable).

[theCodeDrift]

@claude /review

[github-actions]

Claude finished @theCodeDrift's task in 1m 34s —— View job

---

Review mode: incremental — read 0 prior review thread(s) before reviewing.

The only prior content on this PR is the @claude /review trigger comment itself — no prior reviews, review threads, or other top-level comments. This is the first review of this PR; everything below is assessed fresh against the full diff.

• Fetch PR diff and metadata
• Read changed files in full (platform-binary.ts, platform-binary.test.ts, api-deprecated-paths.test.ts)
• Review correctness, security, performance, test adequacy, clarity
• Post inline comments for findings
• Post final summary

Assessment

Both fixes are correct and well-targeted at the race described in #694.

platform-binary.test.ts: The vi.mock("node:url", ...) passthrough is scoped correctly — it only redirects fileURLToPath when relocation.modulePath is set (only during the one test that needs it) and only for paths matching rules/platform-binary.ts. I traced resolvePlatformBinary's own computation (dirname(fileURLToPath(import.meta.url))/../node_modules/.bin in packages/cli/src/rules/platform-binary.ts:242-247) against the relocated path (<root>/rules/platform-binary.ts → dirname = <root>/rules → .. = <root> → <root>/node_modules/.bin), which matches the test's localBin exactly. relocation.modulePath is reset in the finally block, and nothing under src/ is written anymore — the module path is faked instead, which is the right fix since it keeps the resolver's real path arithmetic under test without touching the source tree.

api-deprecated-paths.test.ts: The hand-rolled walk prunes node_modules and generated by directory name before recursing, which closes the actual race (previously readdir({ recursive: true }) could list src/node_modules and then fail descending into it after it was concurrently removed by the other test). isMissing/readSource correctly narrow on ENOENT only and tolerate both a vanished directory (returns []) and a vanished file (returns undefined, skipped by the caller). I checked packages/cli/src for any fixture actually named node_modules that this pruning might wrongly skip — there is none, so no behavior change for real source files. The generated/ pruning (by exact directory name, matched at any depth via recursion) is equivalent to the prior relative(...).split(sep).includes("generated") filter.

Minor, non-blocking observations:

• The manual walk now issues one readdir per directory sequentially (via await in a for loop) instead of a single recursive call — slightly more syscalls than before, but this is test-only code and the difference is immaterial for this repo's source tree size.
• The exact race (a directory/file vanishing during the walk) isn't exercised by an automated test in this PR — it's validated per the PR description via a manual stress loop, which is reasonable since deterministically reproducing the race in CI would require its own synchronization scaffolding. Worth knowing that regression coverage for this specific scenario relies on manual verification, not an assertion in the suite.

No correctness, security, or clarity issues found. Production code (platform-binary.ts) is untouched, consistent with the PR's claim.
(branch)

[theCodeDrift]

Re: @github-actions — "Claude finished @theCodeDrift's task in 1m 34s — Review mode: incremental…"
#695 (comment)

Both observations are non-blocking and need no change: the per-directory readdir walk is test-only and cheap at this tree size, and the vanished-mid-walk race is not deterministically reproducible in CI without synchronization scaffolding that would cost more than it guards, so the manual stress loop stays the verification.

— AI Coding Agent