fix(test): resolve repo reads from the module, not the process cwd - #3592
Conversation
`src/config/cicd-coverage-workflow.test.ts` read repo files by cwd-relative path in a module-level await. Test files are separate isolates sharing one process under --parallel, and src/testing/cwd.ts chdirs that shared process, so the read resolved against another test's directory and threw. Because the await was at module scope the throw was an uncaught module error, which failed the whole shard and both jobs that depend on it. Resolve from import.meta.url and read inside each test, then add a lint ratchet so the pattern cannot come back. Fixes the same latent bug in scripts/build/generated-artifact-checks.test.ts.
📝 WalkthroughWalkthroughThe change adds a Babel-based audit for cwd-relative test reads, updates affected tests to use module-relative or per-test reads, adds a callback-read baseline, and runs the audit through CI and verification tasks. Changescwd-relative test-read enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AuditCLI
participant TestFileDiscovery
participant findCwdRelativeReads
participant BaselineComparison
AuditCLI->>TestFileDiscovery: discover configured test files
TestFileDiscovery->>findCwdRelativeReads: parse each file and collect reads
findCwdRelativeReads-->>AuditCLI: return scoped read findings
AuditCLI->>BaselineComparison: compare callback counts with baseline
BaselineComparison-->>AuditCLI: return regressions and improvements
AuditCLI-->>AuditCLI: report findings and set failure status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a4f7a4872
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lint/audit-cwd-relative-test-reads.ts`:
- Around line 61-65: Replace delimiter-depth classification in the audit with
syntax-aware execution-scope detection: preserve reads in top-level
object-literal initializers, handle multiline calls correctly, and exclude reads
inside inline Deno.test callbacks. Update
scripts/lint/audit-cwd-relative-test-reads.ts at lines 61-65 and 94-109, and
first add focused failing regressions for multiline reads, object-literal reads,
and inline test callbacks in scripts/lint/audit-cwd-relative-test-reads.test.ts
lines 5-81.
In `@src/config/cicd-coverage-workflow.test.ts`:
- Around line 30-32: Convert the test in cicd-coverage-workflow.test.ts to
runtime-neutral BDD style by importing and using describe() and it() from
`#veryfront/testing/bdd.ts` instead of Deno.test. Replace Deno.readTextFile in
readRepoFile with the project’s runtime-neutral file-reading utility, while
preserving the existing workflow assertions and helper behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9391a3f7-a171-489b-9ec9-669002966901
📒 Files selected for processing (5)
deno.jsonscripts/build/generated-artifact-checks.test.tsscripts/lint/audit-cwd-relative-test-reads.test.tsscripts/lint/audit-cwd-relative-test-reads.tssrc/config/cicd-coverage-workflow.test.ts
Rework the audit from review feedback on #3592. The line-and-brace heuristic enforced a weaker property than its header claimed, in three separate ways: - It only looked at module scope. The chdir race does not care where the read sits — a sibling isolate can hold `withCwd` while this file's `it(...)` callback runs — so moving a read into a test body only changes how the failure is reported. - `Deno.readTextFile(\n "deno.json",\n)` is ordinary `deno fmt` output and the path never shares a line with the callee, so a future module-scope offender walked straight through the new CI gate. - Delimiter depth is not execution scope: a read in a top-level object initializer sits at depth > 0 but still runs at module eval, and an inline `Deno.test("x", () => { read })` was flagged because the match ran before the line's depth was updated. Execution scope is a syntactic property, so read it off the syntax: `@babel/parser`, already used by `scripts/codemods/`, with a walk that tracks whether a call is inside a function body. Two tiers now: - MODULE SCOPE — zero allowed, always. These are uncaught module errors that kill the shard and its dependent jobs. Still 0. - CALLBACK SCOPE — 95 pre-existing reads across 21 files, frozen in `cwd-relative-test-reads-baseline.json` as a per-file count that may only shrink, following the sanitizer, skipped-test, and module-boundary ratchets. Counts, not just the file set, so an already-listed file cannot quietly grow another. The audit also watches `#veryfront/platform/compat/fs.ts`, matched by import binding, so the runtime-neutral reader that `src` tests are pushed towards is not a hole. `src/config/cicd-coverage-workflow.test.ts` moves to describe/it and that same compat reader. It mentioned the runtime global, and `tests/node/run-tests.mjs` drops any src file that does — so every assertion in it ran on one runtime while the other two reported a silent pass. It now runs under all three, with a guard test asserting the property the runner filters on.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lint/audit-cwd-relative-test-reads.ts`:
- Around line 300-308: Classify direct IIFEs using their enclosing scope instead
of always marking function nodes as callback scope. In
scripts/lint/audit-cwd-relative-test-reads.ts lines 300-308, update the
traversal around the function-node handling and CallExpression.callee
relationship while preserving callback classification for deferred callbacks. In
scripts/lint/audit-cwd-relative-test-reads.test.ts lines 70-77, first add a
focused regression asserting that a cwd-relative read inside a direct IIFE is
reported with "module" scope.
- Around line 411-418: Update the repoRoot initialization in the scan flow to
use the existing fromFileUrl adapter with the module-relative URL instead of
URL.pathname. Keep repoRoot as a filesystem-compatible path for collectTestFiles
and the subsequent file slicing and Deno.readTextFile calls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c2f53a6-601a-4a74-b2f6-2eb83f065e81
📒 Files selected for processing (4)
scripts/lint/audit-cwd-relative-test-reads.test.tsscripts/lint/audit-cwd-relative-test-reads.tsscripts/lint/cwd-relative-test-reads-baseline.jsonsrc/config/cicd-coverage-workflow.test.ts
…ortably
Two more from review.
A direct IIFE is a function node but its body runs during module evaluation,
so `(async () => { await Deno.readTextFile("deno.json") })()` at the top of a
test file is an uncaught module error — a shard-killer. The walk filed it
under callback scope, which would have let the baseline tier absorb it. A
function node that is the callee of its own call no longer opens a callback
boundary; a callback the IIFE merely receives still does.
`new URL("../../", import.meta.url).pathname` keeps the URL's leading slash
and percent encoding, so a Windows checkout would scan `/C:/...` and find
nothing. Use `fromFileUrl`, and normalise the reported path to posix
separators so the committed baseline's keys match on every platform.
`lint:cwd-relative-test-reads` failed on the new case: it raised `tests/docs/guide-content.test.ts` from 29 to 30 cwd-relative reads inside test callbacks, and that ratchet may only shrink. The surrounding cases in this file are the grandfathered 29. Rather than join them, resolve the new read from `import.meta.url` — the fix the audit header prescribes, and the one #3592 applied. Test files are separate isolates sharing one process under `--parallel` and `src/testing/cwd.ts` chdirs that process, so a cwd-relative read is correct only until an unrelated file lands beside it in the same shard. Baseline is untouched: 0 at module scope, 95 in callbacks across 21 files, exactly as before.
Symptom
coverage shard N/8failed three times in the merge queue on PRs that touch none of this code (#3580, #3589), each time takingtests (unit)andcoverage gatedown with it as dependents — three red checks from one failure, with no useful message:Root cause
src/config/cicd-coverage-workflow.test.tsread three repo files by cwd-relative path in a module-levelawait. Both halves matter:--parallel, andsrc/testing/cwd.tscallsDeno.chdiron that shared process — its own header says it "mutates state shared by every test in the process". It restores in afinally, but a restore only closes the window afterwards; a reader executing inside the window resolves against another test's directory. Which files share a process is decided byselectShardFiles(index % 8over the sorted file list), so adding any test file anywhere reshuffles the pairings. This test was correct only until something landed beside it.awaitthat throws is an uncaught module error: Deno fails the whole file rather than one test, so the shard dies and both dependent jobs fail.Fix
import.meta.urlinstead of the process cwd, which removes the dependency rather than trying to enforce chdir discipline across every test in a process (--parallelmakes that impossible anyway).scripts/lint/audit-cwd-relative-test-reads.ts, wired intolint:ci/verify, so the class cannot return.The audit found a second offender,
scripts/build/generated-artifact-checks.test.ts, fixed here the same way.The audit, after review
The first version of the audit matched a regex per source line and tracked delimiter depth. Review (thanks @chatgpt-codex-connector, @coderabbitai) showed that enforced a materially weaker property than its header claimed, in three separate ways. All three are now covered by regressions that were red first.
It only looked at module scope. The chdir race does not care where in a file the read sits — a sibling isolate can hold
withCwdwhile this file'sit(...)callback is running. Moving a read into a test body changes how the failure is reported, not whether it can happen. Codex's cited example was real:src/oauth/providers/common.test.tsreadscli/templates/integrations/slack/connector.jsoninside a callback and the old audit ignored it by construction.Multiline calls were invisible.
Deno.readTextFile(\n "deno.json",\n)is ordinarydeno fmtoutput for longer calls; the path never shares a line with the callee, so a future module-scope offender walked straight through the newly wired CI gate.Delimiter depth is not execution scope. A read in a top-level object initializer sits at depth > 0 but still runs at module eval, and an inline
Deno.test("x", () => { read })was flagged because the match ran before the line's depth was updated.Execution scope is a syntactic property, so it is now read off the syntax:
@babel/parser, already vendored and used byscripts/codemods/, with a walk that tracks whether a call sits inside a function body. The whole repo parses in ~1.2s.A second review round caught two more, both red-first as well:
(async () => { await Deno.readTextFile("deno.json") })()at the top of a file is an uncaught module error, but the walk filed it under callback scope — the baseline tier would have absorbed a shard-killer. A function node that is the callee of its own call no longer opens a callback boundary; a callback it merely receives still does, pinned by its own test.URL.pathnameis not a portable path. On Windows it yields/C:/..., so the scan would have found nothing and the audit would have exited 0 on a repo it never looked at — a ratchet failing open. NowfromFileUrl, plustoRepoRelativenormalising baseline keys to posix separators so the committed baseline matches on every platform.Two tiers
scripts/lint/cwd-relative-test-reads-baseline.jsonand may only shrink. This follows the sanitizer, skipped-test, and module-boundary ratchets: growth fails with "do not raise the baseline", shrinkage passes and tells you to regenerate withdeno task lint:cwd-relative-test-reads -- --print-baseline.The baseline records a per-file count, not just the set of files: adding a second racy read to an already-listed file fails. Gating membership alone would let the existing offenders accumulate freely.
The audit also watches
readTextFile& co. from#veryfront/platform/compat/fs.ts, matched by import binding rather than by name.srctests are pushed towards that module (they avoid theDeno.*global), so leaving it unwatched would open a hole exactly where the guidance sends people.The config test, after review
src/config/cicd-coverage-workflow.test.tsnow usesdescribe()/it()from#veryfront/testing/bdd.tsand reads through#veryfront/platform/compat/fs.ts+fromFileUrl, which is the patternsrc/platform/compat/std/async.test.tsalready uses.This was not cosmetic.
tests/node/run-tests.mjsdrops any file undersrc/whose source mentions the runtime global, and the Bun runner shares that list — so every assertion in this file ran on exactly one of the three runtimes while the other two reported a silent pass. Verified with the runner's own predicate:truebefore,falseafter, and the file now runs green undernode --import ./tests/node/resolver.mjs --test.A guard test asserts the property the runner filters on, so the gap cannot reopen unnoticed.
Verification
0 vs 1,0 vs 1, and1 vs 0respectively.["callback"]where they should report["module"], and the baseline-key normalisation test failed beforetoRepoRelativeexisted.0 at module scope, 95 baselined.fmt:check,lint,lint:style,lint:module-boundaries,lint:dependency-boundaries,lint:sanitizer-baseline,lint:skipped-tests,lint:ban-test-onlyall clean;deno run --frozenneeds no lockfile change.Honest limitation: I could not reproduce the chdir race locally — two test files is not enough to hit the window, which is consistent with it appearing only under an 8-way shard of the full suite. The evidence for the mechanism is the CI failure text, the
chdirinsrc/testing/cwd.ts, and this having been the only test in the repo reading repo files cwd-relative at module scope. The fix does not depend on that diagnosis being exactly right: resolving fromimport.meta.urlis immune to any cause of a differing cwd.Known scope limit of the audit: a function declared at module scope and called at module scope is classified as callback scope. That under-reports rather than over-reports, and the callback tier is baselined, so it cannot grow silently either way.
Found while unblocking the developer-experience dogfood PR batch.