Repository navigation
Conversation
A test file that the walk finds through a symbolic link has its real path in the module graph, so the lookup of the walk path found no node and the test was never selected. The scan now reports the source index of each entry point by position, and the lookup by path is gone. The scan never keeps the paths of links: git names a file by its real path.
|
Updated 3:10 AM PT - Oct 27th, 2026
✅ @robobun, your commit ac5beadfe921360c01fa6809ad033d4638f6dae4 passed in 🧪 To try this PR locally: bunx bun-pr 46583That installs a local version of the PR into your bun-46583 --bun |
|
Status: ready for review. Reproduced on a release build of main (5c6d4e2). In a repository where |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Collect entry-point source indexes src/bundler/bundle_v2.rs |
The CLI graph scan accepts one output slot per entry point. Scan-specific enqueueing records indexes for resolved entries, including entries already in the graph. |
Map changed-test entries to graph nodes src/runtime/cli/test/ChangedFilesFilter.rs |
The filter uses the scan’s per-entry-point results instead of mapping paths after the scan. For paths that share a graph source, it prefers the graph-path match or otherwise the lexically smallest path. Changed test paths remain selected. |
Test linked-path selection test/cli/test/test-changed.test.ts |
Tests cover duplicate arguments, linked files and directories, unresolved paths, path variants, shard selection, and watch-mode reruns after an imported file changes. |
Suggested reviewers: jarred-sumner, dylan-conway, alii
Priority:
Merge Risk: 
c9889
-
No actionable merge-blocking issue was established; the change is ready for normal checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: binding test files to module-graph entries by position instead of path for test --changed.Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description follows the repository template with both required sections, "What does this PR do?" (Problem, Fix, Background, Downsides) and "How did you verify your code works?" (test results, repr… Full details: Description check
-
Explanation
The description follows the repository template with both required sections, "What does this PR do?" (Problem, Fix, Background, Downsides) and "How did you verify your code works?" (test results, repro, measurements, suites run). It states the cause, the fix, the verification, and the known limits. The only gap is that the verification relies on a platform-specific test file whose Windows-only cases are deferred to CI, which the bot evidence flags as "no test proof" on this machine. That is disclosed in the description, so the description is complete.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I checked the SCAN=false path of the split entry loop against the old one: every continue became return Ok(None), ? propagation and is_entry_point: true are unchanged, and both bun build callers of enqueue_entry_points_normal are untouched, so bun build/Bun.build behavior is preserved. The scan's duplicate-file fallback keys by the same path.text that enqueue_entry_item checked before its early return, and the scan error path still drains the pool. I also looked at whether the watch test's " across " needle could match the first run's summary; it slices from before, so it cannot.
Extended reasoning...
The change splits the bundler's entry-point enqueue loop into a reserve step plus a const-generic per-entry helper, adds a positional out-slice variant for the --changed scan, and rewires ChangedFilesFilter.rs to bind test files to graph nodes by position instead of by path, with a table-driven symlink suite and a watch case added to test-changed.test.ts. It touches no security-sensitive surface (no auth, injection, or data exposure), only CLI test selection and the bundler's entry-point setup. The findings posted inline (a behavior change for files found twice under --parallel/--isolate, a dead preserve_symlinks line, and the watch test's timeout) are what keep this from an approve; the refactor itself traced as behavior-preserving for the non-scan callers.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/cli/test/ChangedFilesFilter.rs— Monorepo users runningbun test --changed --parallel(or--isolate) from a root that reaches a shared test directory both directly and through a link now get that test file run twice; on the base branch it ran once. The scan's SCAN fallback at src/bundler/bundle_v2.rs:3650-3655 returns the same source index for the link-path slot, so both slots pass theaffectedcheck at src/runtime/cli/test/ChangedFilesFilter.rs:295-296 and both copies are kept. Fix: dedupe the kept list by source index (or real path) before returning so a file selected under two names runs once; keeping it is a user-visible regression for--changed --parallel, where runtime and failure counts double.Why this was flagged
Trigger: a checkout where
bun testwalks a test file under its real path and again through a directory link (the PR's own row at test/cli/test/test-changed.test.ts:528-538), run with--changed --parallelor--changed --isolate. On the base,slot_to_sourcewas filled bypath_to_index.get(tf)(old ChangedFilesFilter.rs:249-251), which missed the link-path slot, so only the real-path slot was kept and the file ran once. Nowenqueue_entry_point_normal::<true>at src/bundler/bundle_v2.rs:3650-3655 looks the duplicate up inpath_to_source_index_mapand writes the same index into the second slot; ChangedFilesFilter.rs:295-296 keeps both. With--parallel, test_command runs each kept path in its own worker, so the same tests execute twice and any failure is reported twice. The dismissal relied on the author's statement that plainbun testalready lists the file twice; that is a different command, and for the--changedpopulation this is a strict change from the base. Remedy: collapse kept slots that share a source index before returningtest_files[0..write].Verification: Triggered by
bun test --changed --parallel(or--isolate) where the walk reaches the same test file both directly and through a link. The SCAN fallback at src/bundler/bundle_v2.rs:3650-3655 gives the link-path slot the same source index as the real-path slot, sokeepat src/runtime/cli/test/ChangedFilesFilter.rs:292-301 is true for both slots and both run; on the base only the real-path slot ran.
A test file that the walk finds under two paths is one module for the run, unless the run keeps the paths of links. The scan marks the later entry points of a file, and only the first one is selected through the graph. The log of the scan is in the CLI arena, so a message for a test file that does not resolve was never freed.
|
Thanks for the review. What changed in 94c2831:
The same push fixes the failure on the ASAN lane: the log of the scan kept a message for a test file that does not resolve (685 bytes under LeakSanitizer, on main too). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the SCAN = false instantiation of the split entry loop in src/bundler/bundle_v2.rs against the old enqueue_entry_points_normal body: the plugin, FileMap, and resolve-failure early exits land on the same continue points and the same enqueue_entry_item calls, so bun build / Bun.build behavior is unchanged. I also traced the new duplicate keep rule in ChangedFilesFilter.rs through link-first and real-path-first discovery order and with --preserve-symlinks on and off, and the zip in enqueue_entry_points_for_scan (lengths only ever match at its single caller) — none of those turned up a problem.
Extended reasoning...
The change splits the bundler's shared entry-point loop into a const-generic per-entry function used by both bun build and the --changed scan, adds a positional ScannedEntryPoint out-slice, and rewrites the keep decision in ChangedFilesFilter.rs plus a large symlink test matrix. It touches no security-sensitive surface (no auth, injection, or untrusted parsing), though it does touch the hot bun build entry loop. The one surviving finding is a pre-existing leak on the scan-failure fallback path; the dead preserve_symlinks = None line flagged last round is gone from HEAD. Because the refactor reaches a shared bundler path and the new keep rule changes which of two paths to one file is selected, a human look is still worthwhile.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/cli/test/ChangedFilesFilter.rs— nit, pre-existing: maintainers still get a leaked scan log and transpiler on the fallback path, even though the success path now frees both. When scan_module_graph_from_cli returns Err at src/runtime/cli/test/ChangedFilesFilter.rs:186, the function returns at :194 without touching scan_transpiler. The log messages that BundleV2::init or Transpiler::init added before the failure stay allocated for the life of the process. Fix: release the scan's heap-owned state on every exit of filter after the arena allocations at :137-150, calling scan_transpiler.deinit() and log clear_and_free() in the Err arm too, so the new release at :320-323 has no unpaired sibling path.Why this was flagged
Trigger:
bun test --changedwhere the scan fails after the transpiler is built, for exampleBundleV2::initfailing or the scan log already holding an error so src/bundler/bundle_v2.rs:4655 returnsErr(crate::Error::BuildFailed).filterreaches the Err arm at…Verification: pre-existing. Trigger:
bun test --changedwhenBundleV2::scan_module_graph_from_clireturns Err. The Err arm at src/runtime/cli/test/ChangedFilesFilter.rs:180-194 never callsscan_transpiler.deinit()norlog_mut().clear_and_free(); both releases exist only on the success path at :313 and :315. The base branch has the identical Err arm, so merging makes nothing worse.
… of discovery A file that several test paths name was selected under the path that the walk found first. The walk follows the order of a directory listing, and `--shard` sorts the selected paths, so one tree and one change gave two shard splits on two checkouts. main does not depend on that order. The graph now selects such a file under the path that the graph has for it, as on main, or else under the smallest of its test paths. A path that git names is selected by that name, as before. The `--preserve-symlinks` exception is gone: after the scan, the run loads one module for the two paths.
|
ca917e2 reworks one rule of this PR, and this comment also answers the second review. The fallback path (the finding of the second review). It is real, it is on main, and this PR does not change it.
A file found under two paths (987cf47 had this wrong). 987cf47 selected such a file under the path that the walk found first. Below its root the walk follows the order of the directory listing, and
The PR body has the new numbers (no link: 132 instructions less per test file, +1,792 bytes) and the limits. |
…exMap::get` `get_path(path)` is `get(path.text)`. The other lookups in bundle_v2.rs that have a resolved path call `get(path.text)`, and this one now does too.
|
c988981 is a one-token change. It does not change behavior. The lookup of a duplicate entry point called
|
|
Merged main (8a0e49e) into this branch.
|
Problem
bun test --changedskips a test file thatbun testfinds through a symbolic link. It prints--changed: 1 changed file, but no test files are affectedand exits 0.filter(ChangedFilesFilter.rs:251on main) matches test files to the module graph by path.bun testnames the file through the link, the graph by its real path.Fix
scan_module_graph_from_clireports the graph node of each entry point by position. The path lookup is gone.filterfrees the scan's log, which has a message for each unresolved test file.test/cli/test/test-changed.test.ts(20 new cases, 13 fail on main). Self-reviewed twice: 62 of 70 concerns addressed (Notes).Background
--changedgives all test files to the bundler as entry points, scans their imports, then walks backward from each changed file.filter(keeps the path compare), real paths in test discovery (filters match the link name, positional paths have no discovery entry).Downsides
bun build+0). Binary: +1,792 bytes.--changedrun can fail.--preserve-symlinks(see Notes).Notes
Source of the report. An internal fuzz run of
bun test --changed. No user reported it and there is no issue.Repro.
Forms that main skips and this PR selects (each is a case in the test file):
[test] rootin bunfig.toml, two selected files with--parallel../l/t.test.ts,./l, an absolute path, and an absolute path through a link above the repository (no link in the repository).--changed --watch: the restart after a change to the import of a linked test../A.test.tsfora.test.tson a volume that ignores case.Mechanism.
enqueue_entry_itemalready returned the source index of each entry point, andenqueue_entry_points_normaldropped it. The body of that loop is nowenqueue_entry_point_normal::<SCAN>.bun buildandBun.buildcall it withSCAN = false. The scan calls it withSCAN = true, which also finds the index when an earlier entry point is the same file, and marks that entry point as a duplicate. An entry point that does not resolve keepsNone, so the positions cannot shift.A file with several test paths. A walk can find one file through a link and under its real path, or through two links. Only when the scan marks a duplicate,
filterfixes one path for each file (kept_paths): the test path that is equal to the path in the graph, or else the smallest test path bystrings::order, the order that--shardsorts by. The graph selects the file under that path only. A path that git names is selected by its name, as before. Where main selected a file, this PR selects the same path. A path that the command line names twice is selected twice, as on main.The second version of this PR kept the path that the walk found first. Below its root the walk follows the order of the directory listing, and
--shardsorts the selected paths. With a linkato a directoryzbesidem.test.ts, that version ranz/limit.test.tsin shard 1/2 where main runsm.test.ts. The documentation says that the shards "cover each file exactly once with no coordination" (docs/test/parallel.mdx). That version also selected./t.test.ts ./t.test.tsonce (1/2) where main selects it twice.The log of the scan.
Transpiler::resolve_entry_pointadds a message to the log for an entry point that does not resolve. The log of the scan is in the CLI arena, so nothing dropped it: LeakSanitizer reports 685 bytes in 2 allocations, on main too (bun test --changed ./a.test.ts ./package.json/nope.test.ts).filternow callsclear_and_freeon it.--preserve-symlinks. The scan never has the flag (src/bundler/options.rs:1759), and the run reads the real paths that the scan left in the directory cache of the resolver. So after the scan the run loads one module for a file found under two paths. The second version of this PR selected both paths under the flag: it printedrunning 2/3 test filesand thenRan 1 test across 2 files. That exception is removed. The selection under the flag is now the selection without it, as on main. #46007 makes the bundler read the flag and #44835 makes the run forget what the scan cached. What the flag selects is a decision for after those two.Measurements. Release builds of 5c6d4e2, the merge-base at that time, and of this PR at ca917e2. The later merge of main (8a0e49e) changed two calls in the entry loop (#46834) and nothing in
filter. The numbers are not measured again after it. gdb 16.3 (catch syscall, breakpoints, and aperf_event_openinstruction counter on the main thread). strace, perf, valgrind and bloaty are not installed in the build container.access, +0newfstatat, +0open, noreadlink.openatandgetdents64move by 1 between runs of the same binary.filter, without the scan: +0. Onemi_zallocbecame onemi_malloc(the vector with one slot for each test file).filterafter the scan 1,087 to 942. The entry loop of the scan: +14. Net: 132 less.filterafter the scan: 2,838 to 2,628 instructions.bun buildwith 16 entry points: 12,330 to 12,330 instructions for each entry point in the entry loop.diff -rof the two output directories: identical.sizetext: 107,635,854 to 107,637,646 (+1,792).filter8,239 to 8,357 bytes,kept_paths736 bytes.scan_module_graph_from_cli843 to 1,811 bytes, because it now holds the entry loop.enqueue_entry_itemandappend_pathsdo not change.Limits, not fixed here.
lstatwhere the changed set is built, and a decision on what a changed directory link selects.t.test.ts -> real/x.bak). The run parses it as a test. The scan reads it with thefileloader and sees no import.bun testfails andbun test --changedprints "no test files are affected", on main and with this PR. The loader of the scan is the subject of test --changed: follow bare specifiers that resolve to local files #44836.--preserve-symlinks: see above.--tsconfig-overridewith a file that does not exist) prints--changed: failed to build module graph (BuildFailed); running all tests. On that path main keeps the scan transpiler and its log (LeakSanitizer: about 33 KB in 921 allocations) and the 8 threads of the scan's pool (14 threads during the run, 6 without). This PR does not change that path. bundler: make the enqueue functions unable to fail and check everygraph.astappend #39855 has the teardown of the pool for the error exits ofscan_module_graph_from_cli, and bundler: remove unsafe from bundle_v2, ParseTask, LinkerContext and the bundler thread pool #40438 addsDrop for BundleV2.--parallelthe file header shows the path that discovery found. A serial run shows the real path. This PR does not change that.git ls-files --otherslists only the subtree of the working directory (ChangedFilesFilter.rs:542), so an untracked file outside it is not in the changed set.pathsalias underpackages = externalreturns before the real-path step of the resolver (resolver.rs:1199-1232). A change to a file that a test reaches only through an alias that crosses a link selects nothing, andbun build --packages externalbundles that file twice. test --changed: follow bare specifiers that resolve to local files #44836 changes this path.substor mapped drive can make git's spelling of the root differ from the graph's.paths, package.jsonimports, a bunfigpreloador.env.testselects nothing. test --changed: follow bare specifiers that resolve to local files #44836 asks the same question.The same skip in other commands, not changed here.
bun lint --changedandbun lint --stagedskip a changed file that the run reaches through a link to a directory. In the repro above, withdebugger;added toshared-tests/limits.ts:bun lintinpackages/areports it, andbun lint --changed=HEADprints "No changed files to lint" and exits 0.bun build --appwith two route files that are links to one file writes the module of another file for 7 of 8 routes and exits 0. The two bake entry loops (enqueue_entry_points_dev_server,enqueue_entry_points_bake_production) drop the index that the scan now keeps.Other open PRs on these lines. #46007 (the bundler reads
preserve_symlinks), #44835 (the run forgets what the scan cached), #44836 (bare specifiers that resolve to local files), #46262 (HTML imports and every copy of a changed file; its notes list the linked test file as a gap with no PR), #45938 (barrel mode off for the scan), #44487 (a directory link back to a directory of the walk), #43361 (a test file listed once when two path arguments select it: the case "a test file named twice is selected twice" then becomes 1/1), #41030 (one committed repository for each case of this test file: its copy needsverbatimSymlinksfor the link cases), #39855 and #40438 (teardown of a bundle). #45248 deletesPathToSourceIndexMap::get_path, so the lookup of a duplicate entry point callsget(path.text): it compiles before and after that PR.Windows. 13 of the 18 link cases need only a link to a directory. They run on Windows with a junction that
.gitignorehides from git. The 5 cases with a link to a file do not run there.vitest, the model for
--changed(pristine installs, node 26.3.0): 4.1.9 selects a linked test when one of its imports changes and skips it when the test file changes. 5.0.5 skips both.Self-review. Two rounds, 35 concerns each.
preserve_symlinksfor the scan (no effect until CLI: fixes for bun run, flags, bunfig.toml, .env, --filter, bun init and completions #46007), a seeded generator of link layouts, a case for a Windows working directory behind a junction, a case for a volume that ignores case, tracking issues for the limits, and the order of landing relative to test --changed: follow bare specifiers that resolve to local files #44836.--shardshows it. The--preserve-symlinksexception and its case are gone. A path named twice is selected twice again. Two links with no real path select the smallest path. The link cases pin the status line, the files that ran and the exit code in one assertion, and make the repository with three awaited git processes. A commit that released the scan when it fails is not in this PR: bundler: make the enqueue functions unable to fail and check everygraph.astappend #39855 has the teardown, and the check of that commit (a count of pool threads) failed on the fixed build under load. The limits, the two other commands and the other open PRs are listed above.--parallelor--isolate, where main runs it once.--changedPRs could share.Suites run. Debug build:
test/cli/test/test-changed.test.ts(41 pass). Release build of this PR: test-changed (41 pass), test-shard (29 pass), parallel (46 pass, 1 fail: "--parallel --dots prints one status character per test" also fails on the release build of the merge-base here, 4 of 4 runs). Release-asan build with LeakSanitizer: test-changed (41 pass). Earlier, on the first commits of this PR, which have the samebundle_v2.rs:test/bundler/cli, bundler_files, bundler_html, bundler_plugin, bundler_edgecase, bun-build-api,cargo clippyfor bun_bundler and bun_runtime, andcargo checkfor x86_64-pc-windows-msvc and aarch64-apple-darwin.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/test/test-changed.test.ts