Repository navigation
Conversation
A file in excludeFiles never runs in the --parallel batch, so the annotation scan can't observe it flaking there; "zero flakes" for it means nothing. The generator now reads the existing table before overwriting it and keeps those entries. An exclusion leaves the list only when the file is deleted, its dir drops out of dirs, or someone removes the entry by hand (at which point the next regen has real batch data for it). Also re-adds inspect-error-leak.test.js and sourcetextmodule-leak.test.ts to excludeFiles; both measure in-process RSS and were dropped by #36585 for this reason. computeAllowlist is now a pure export so the carry-forward rule is covered by test/internal/parallel-allowlist.test.ts without hitting the Buildkite API.
|
Status: diff touches CI on build 88675: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe update extracts pure allowlist computation, preserves prior exclusions, isolates CLI execution, adds focused tests, and updates the generated parallel test exclusion list. ChangesParallel allowlist update
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/update-parallel-allowlist.mjs`:
- Around line 165-168: Update the previousExcludes loading in the allowlist
regeneration flow to treat only ENOENT as an empty initial allowlist; for
malformed JSON, permission errors, or other read failures, emit an actionable
error and stop before writing output. Validate that parsed excludeFiles is an
array of strings, preserve the existing file unchanged on any invalid input, and
add a regression test covering corrupted JSON.
- Around line 75-85: Validate the parsed CLI values in the entrypoint before any
build scanning or network access: require builds to be a strict, finite, safe
integer of at least 50, and fast-ms to be a strict, finite, safe non-negative
integer (including 0 as valid). Reject empty or partially parsed strings rather
than relying on parseInt, and report each invalid value with its constraint and
correction guidance.
In `@test/internal/parallel-allowlist.test.ts`:
- Around line 75-86: Update the test "stress-named and docker-prefixed files
never qualify" to assert that the computed dirs result does not contain
"js/sql", ensuring the Docker-prefix production guard is exercised in addition
to the existing excludeFiles assertions.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47de4f7b-78a9-45bd-8f5b-165a393bb51d
📒 Files selected for processing (3)
scripts/update-parallel-allowlist.mjstest/internal/parallel-allowlist.test.tstest/parallel-allowlist.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/update-parallel-allowlist.mjs`:
- Around line 167-173: Update the previous-exclusions loading logic around
previousExcludes to validate every excludeFiles entry is a string before
constructing the Set. On invalid entries, fail with an error containing the
output path, rejected value or index, expected string-array shape, and repair
action; add a regression test covering a non-string entry and ensure valid
exclusions remain preserved.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 171f6a66-fbea-4a71-8e86-75c871a6c85a
📒 Files selected for processing (2)
scripts/update-parallel-allowlist.mjstest/internal/parallel-allowlist.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — all three nits from my earlier pass are addressed (fail-loud on unreadable previous allowlist, dirs assertion in the docker test, realpathSync on the entry-point guard).
What was reviewed:
computeAllowlistextraction is behavior-preserving vs. the inline original; only new logic is thepreviousExcludes.has(file)carry-forward, which is the point of the PR and is unit-tested.- Previous-allowlist read now matches the sibling
durationsread: ENOENT → empty set, everything else exits before writing. - Confirmed
process.argv[1]underbun testis the test file's absolute path, sorealpathSyncon it succeeds and the CLI block stays skipped on import. - The two re-added
excludeFilesentries are in listed dirs and exist on disk (covered by the existing shape test).
Extended reasoning...
Overview
This PR touches CI tooling only: scripts/update-parallel-allowlist.mjs (the Buildkite-driven regenerator for test/parallel-allowlist.json), its unit test at test/internal/parallel-allowlist.test.ts, and two entries re-added to test/parallel-allowlist.json. No src/ changes; nothing shipped to users.
The functional change is one line — if (previousExcludes.has(file)) return false inside isGood() — plus the plumbing to read the previous excludeFiles before overwriting. The rest is a refactor that hoists the decision logic into an exported pure function and gates the CLI body behind an entry-point check so the test file can import it without triggering the token check / network scan.
Security risks
None. The script already read BUILDKITE_API_TOKEN and hit the Buildkite API; that code moved verbatim inside the guard. No new external inputs, no new writes outside test/parallel-allowlist.json, no shell interpolation.
Level of scrutiny
Low. This is internal maintenance tooling whose output is a checked-in JSON file that goes through PR review before landing, and the two allowlist edits are strictly conservative (excluding a file from the parallel batch is always safe — it still runs serially). A regression here manifests as a wrong regen diff in a future PR, not as a user-visible bug or CI break.
Other factors
- All prior review threads (mine and CodeRabbit's) are resolved. The one declined CodeRabbit suggestion (per-entry string validation of
excludeFiles) is reasonably declined: the checked-in file is validated per-entry bytest/internal/parallel-allowlist.test.tson every CI run, and a non-string inpreviousExcludescannot match a real file path so cannot drop a real exclusion. - I verified that under
bun test,process.argv[1]is the test file's absolute path, sorealpathSync(process.argv[1])in 25d2902 succeeds and does not match the script's__filename; theprocess.argv[1] &&prefix guards the undefined case. - The refactored
computeAllowlistreproduces the original per-dir threshold, sorting, andexcludeFilesemission exactly;totalDirsreplaces the removedbyDir.sizereference in the final log line. - New tests cover carry-forward, hand-removal re-evaluation, dir-below-threshold interaction, stress/docker exclusion (now asserting on
dirs), and import-without-CLI.
There was a problem hiding this comment.
LGTM — all three nits from the previous pass are addressed (ENOENT-only catch, realpathSync guard, dirs assertion in the docker test). The one remaining inline comment is test hygiene on a placeholder assertion and not merge-blocking.
Extended reasoning...
Overview
Internal CI tooling only — no src/ changes. Three files:
scripts/update-parallel-allowlist.mjs: extracts the existing decision logic into a pure exportedcomputeAllowlist({...}), adds apreviousExcludes.has(file)check so exclusions carry forward across regens, and moves the CLI body verbatim into arealpathSync(process.argv[1]) === __filenameguard so the test file can import it. ThepreviousExcludesread fails loud on anything but ENOENT.test/internal/parallel-allowlist.test.ts: adds five unit tests forcomputeAllowlist, including a direct test of the carry-forward rule (the PR's actual fix).test/parallel-allowlist.json: manually re-adds two RSS-leak tests that #36585 dropped via the ping-pong bug this PR fixes.
Security risks
None. Maintainer-run script that reads Buildkite annotations and writes a checked-in JSON consumed only by the CI test runner. No user-facing surface.
Level of scrutiny
Low. This is test-infrastructure tooling whose output goes through PR review before commit. The refactor is a mechanical extraction — I diffed the moved CLI body against the original and the only semantic change is the new previousExcludes input, which is exactly the fix. The two re-added excludeFiles entries exist on disk and their parent dirs are already listed, so the existing "excludeFiles are real files inside listed dirs" test covers them. _meta.stats.excluded is now stale (160 vs 162 actual), but nothing reads it and the next regen corrects it.
Other factors
All prior review feedback (mine and CodeRabbit's) is resolved in 1515e8a and 25d2902. The one nit posted this run — the vacuous typeof computeAllowlist === "function" assertion — is real but does not gate correctness: the top-level import at line 4 already provides the load-bearing coverage for "import doesn't run the CLI", and the carry-forward rule itself is properly covered by the first test in the describe block.
|
Updated 2:11 AM PT - Aug 4th, 2026
✅ @robobun, your commit 7911386035df2cf69db8b38906102c868fb8b5d3 passed in 🧪 To try this PR locally: bunx bun-pr 36868That installs a local version of the PR into your bun-36868 --bun |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/internal/parallel-allowlist.test.ts`:
- Around line 92-97: Update the Bun.spawn invocation in the parallel allowlist
test to pipe stdout, then consume proc.stdout concurrently with proc.stderr and
proc.exited. Assert that the collected stdout is empty before asserting the
child exitCode, preserving the existing stderr and exit-status checks.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6741c68-dd2c-4ca0-a34c-c68fc0f0b3a6
📒 Files selected for processing (1)
test/internal/parallel-allowlist.test.ts
…n't drop the dir A carried exclude has no batch data in the scan window; letting it count against DIR_MIN_FRACTION can push a marginal dir below threshold, which drops the dir and all its carried excludes, recreating the ping-pong one level up. js/node/vm is at margin=0 (7 files, 2 carried) on the table this PR writes.
There was a problem hiding this comment.
LGTM — internal CI tooling change with all prior review feedback addressed.
What was reviewed:
computeAllowlistcarry-forward logic and the fresh-files-only threshold fix from 7911386 — carried excludes no longer count against dir eligibility, and thefresh.length === 0edge falls through to0 < max(1, 0)and drops as before.- The
realpathSync(argv[1]) === __filenameguard:argv[1]is checked for truthiness first, and the subprocess test inbun -emode won't match__filenameeven ifBUILDKITE_API_TOKENis inherited viabunEnv. - Previous-allowlist read: ENOENT is the only silent path; malformed JSON / non-array
excludeFilesexits loudly. - Two re-added
excludeFilesentries pass the existing "real files inside listed dirs" invariant test.
Extended reasoning...
Overview
This PR touches three files, none of which ship in the Bun runtime: scripts/update-parallel-allowlist.mjs (a maintainer-run script that scans BuildKite annotations and regenerates test/parallel-allowlist.json), test/internal/parallel-allowlist.test.ts (unit tests for that script and shape checks on the checked-in JSON), and test/parallel-allowlist.json (two entries re-added). The functional change is: (a) extract the decision logic into a pure exported computeAllowlist so it can be unit-tested without hitting BuildKite, (b) carry forward the previous excludeFiles set so files that never ran in the batch aren't spuriously re-admitted on zero evidence, (c) compute the 2/3 dir threshold over fresh (non-carried) files only so carry-forward can't push a marginal dir out and recreate the ping-pong one level up, and (d) guard the CLI body behind realpathSync(argv[1]) === __filename so importing the module for tests doesn't hit the network.
Security risks
None. This is an internal maintenance script invoked by a maintainer or a scheduled BuildKite pipeline; its output is a checked-in JSON file that goes through PR review. It reads a BuildKite API token from the environment (pre-existing behavior, unchanged), makes read-only API calls, and writes a local file. No user-facing surface, no auth/crypto/permissions.
Level of scrutiny
Low-to-medium. This is CI plumbing whose worst-case failure mode is "some tests flake in the parallel batch and get re-excluded on the next regen" — not runtime breakage. The output file lands via a reviewed PR, so a bad regen is caught before it affects CI. That said, I gave it four rounds of inline review on earlier commits (bare catch {} on the previous-allowlist read, symlinked-checkout argv[1] mismatch, a vacuous test assertion, and the dir-level ping-pong from carried excludes counting against the threshold); all four were addressed in follow-up commits and every thread is resolved.
Other factors
- No CODEOWNERS entry covers
scripts/,test/internal/, ortest/parallel-allowlist.json. - 8 unit tests pass on the debug build per the PR verification block; the new tests exercise the carry-forward rule, the fresh-threshold rule, the hand-removal path, dir-drop, stress/docker exclusion, and the import-doesn't-run-CLI guard via a spawned subprocess with piped and drained stdout/stderr.
- The one candidate the finders raised this run (bunEnv passes
BUILDKITE_API_TOKENthrough to the subprocess test) was verified as a non-issue: inbun -emodeprocess.argv[1]cannot equal the script's realpath, so the CLI block is unreachable regardless of the token. - The bulk of the diff is indentation from wrapping the pre-existing CLI body in the
if (argv[1] === __filename)block; the actual logic delta is small and now covered by pure-function unit tests it previously lacked.
|
Heads up: #39373 landed a different fix for the same oscillation, a persistent |
|
Closing as superseded by #39373.
|
Problem
scripts/update-parallel-allowlist.mjsdecides a file is safe for thebun test --parallelbatch when it has zero flaky/failed annotations in the scanned builds. But a file already inexcludeFilesnever runs in the parallel batch, so it never produces a parallel-batch annotation. On the next regen the generator sees zero flakes and drops the exclusion; the file runs in the batch, flakes, and the regen after that re-adds it. Ping-pong.Observed: #36585 (30032c6) regenerated from builds 85133-86444.
js/bun/util/inspect-error-leak.test.jswas inexcludeFiles(via #36175) throughout that window and had zero annotations, so the regen dropped it; it flaked on almost every build from 86456 onward:42 entries were dropped this way (dir still listed, file no longer excluded). Every RSS /
heapStatstest currently inexcludeFilesis exposed to the same thing on a future regen whose window post-dates its last batch flake.Cause
isGood(file)only has negative evidence (flake annotations). It has no positive evidence that a previously-excluded file ran in the batch without flaking, so "zero flakes for an excluded file" cannot be distinguished from "this file is batch-safe".Fix
Read the existing
excludeFilesbefore overwriting and carry those entries forward. An exclusion leaves the list only when the file is deleted, its dir drops out ofdirs, or someone removes the entry by hand; the next regen then has real batch data for the file and re-excludes only if it flakes again. The_meta.ruletext is updated to say so.The decision logic is now a pure
computeAllowlist({files, flaky, durations, previousExcludes, ...})export; the CLI body is guarded byprocess.argv[1] === __filenamesotest/internal/parallel-allowlist.test.tscan unit-test the carry-forward rule without hitting the Buildkite API.Also re-adds the two in-process-RSS tests that #36585 dropped and are currently running in the batch:
js/bun/util/inspect-error-leak.test.js(supersedes ci: check parallel-denylist.txt against the allowlist; denylist three more in-process leak tests #36867)js/node/vm/sourcetextmodule-leak.test.tsThe other 40 dropped entries are left alone; they have been running in the batch since 86456, so the next regen (with this fix applied) has real data for them.
Verification
node scripts/update-parallel-allowlist.mjsstill runs as before (enters the CLI block, reads the previous table, proceeds to the build scan).Runner predicate with the updated table:
[stamp-90s] gate passed · iteration 0 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 0
evidence per changed file