paths: compute the common path of 9+ inputs with the same loop as fewer - #38732
paths: compute the common path of 9+ inputs with the same loop as fewer#38732robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 9:08 PM PT - Aug 17th, 2026
❌ @robobun, your commit 9419775 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38732That installs a local version of the PR into your bun-38732 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — deletes the broken 9+ branch and lets the working loop handle every count.
What was reviewed:
- Traced both functions: the surviving loop is byte-identical to the old 2..=8 arm, and
nql_at_index_dynalready takes a runtime count, so 2-8 inputs (includingrelative()'s 2-input hot path) are unchanged. - Windows volume check hoisted out of the match: was duplicated verbatim in both arms; now runs once before
min_length, which is read-only so the reorder is behavior-preserving. - Checked the empty-input case:
min_lengthbecomes 0, loop doesn't execute,index == 0returns the separator — same as the old 2..=8 answer, no moreindex -= 1underflow. - Tests cover the fix at three levels (Rust unit tests over 2..=20 inputs and edge cases;
itBundledfor both API/CLI backends; a spawned CLI test for the panic).
Extended reasoning...
Overview
The PR fixes get_if_exists_longest_common_path and longest_common_path in src/paths/resolve_path.rs. Both functions previously had a match input.len() with a correct 2..=8 arm and a broken _ (9+) arm. The 9+ arm compared input[0] against each other input pairwise and only checked for a separator at the byte where each pair diverged, so it almost always missed the last common separator; it also executed index -= 1 when min_length == 0, panicking on an empty input. The fix deletes the 9+ arm entirely and lets the 2..=8 loop (which already takes the count as a runtime argument via nql_at_index_dyn) run for every n >= 2. The Windows volume-root check, which was duplicated verbatim in both arms of longest_common_path_generic, is hoisted out into a single loop over &input[1..]. The 0/1-input early returns are moved before the min_length scan, which is a pure reorder of read-only code.
Security risks
None. This is byte-string prefix computation over paths already in memory; no filesystem, network, allocation, or unsafe code is touched. The change strictly removes a panic reachable from CLI arguments (an empty entry point among 9+), which is a robustness improvement.
Level of scrutiny
Moderate. longest_common_path is called by relative(), which is on many hot paths, but the 2-input case that relative() exercises runs the exact same loop body as before — the only difference is that the Windows volume check now happens before the min_length scan instead of after, and both are pure reads. The 3-8 input cases are likewise unchanged. Only the 9+ case changes behavior, and it changes from wrong-or-panicking to matching what 2-8 already produced. I verified nql_at_index_dyn and nql_at_index_case_insensitive_dyn iterate 1..string_count with no upper bound assumption.
Other factors
Test coverage is thorough: four new Rust unit tests exercise 2 through 20 inputs, a directory that is itself an input, unrelated inputs, and empty inputs (both one-empty-among-many and all-empty), each asserting both function variants; bundler_naming.test.ts adds an itBundled case with 9 entry points for both api and cli backends checking the flat output layout via readdirSync; cli.test.ts spawns bun build with 8 real entries plus an empty string and asserts the ModuleNotFound error and exit code 1 instead of the panic. The PR description documents that three of the four unit tests fail against the old implementation, and that the bundler and node:path suites still pass. The diff is a net deletion of ~90 lines of duplicated logic. No CODEOWNERS entries cover these files, and there are no outstanding reviewer comments.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the previous nit (undrained stdout pipe in the cli.test.ts helper) was addressed in bbf618a; this run found nothing further.
What was reviewed:
- Both
get_if_exists_longest_common_path_genericandlongest_common_path_generic: the deleted 9+ arm is replaced by the existing 2..=8 loop, which already takesstring_countas a runtime argument vianql_at_index_dyn; 0/1 early-returns and the post-loop tail are unchanged. - Hoisting the Windows volume check before the
min_lengthscan inlongest_common_path_generic— the two are independent, so the 2-input path used byrelative()is behaviorally identical. - Callers in build_command.rs:406/833 and JSBundler.rs:902 — both already handle the
None/separator answers, so no downstream signature or behavior change beyond the fix itself.
Extended reasoning...
Overview
The PR deletes the broken _ (9+) match arm in get_if_exists_longest_common_path_generic and longest_common_path_generic in src/paths/resolve_path.rs. That arm compared input[0] against each other input pairwise and only recorded a separator at the divergence point, so it returned None / a one-byte prefix for practically any 9+ input set, and underflowed index -= 1 when the shortest input was empty. The fix routes all counts ≥2 through the loop that already handled 2..=8 (which calls nql_at_index_dyn(string_count, ...) with a runtime count). The Windows volume check in longest_common_path_generic is hoisted above the match into a single loop over inputs. Test coverage is added at three levels: Rust unit tests in the same file (2..=20 inputs, directory-as-input, unrelated inputs, empty input), an itBundled test for 9 entry points via both API and CLI backends in bundler_naming.test.ts, and a subprocess crash-regression test in cli.test.ts comparing 2-entry vs 9-entry error output.
Security risks
None. This is pure path-string prefix computation with no filesystem I/O, no user-controlled allocation sizing, and no unsafe blocks touched. The fix removes a user-reachable panic (integer underflow on index -= 1 with index == 0), which is strictly a hardening improvement.
Level of scrutiny
Medium. longest_common_path feeds relative(), which is on hot paths, but the 2-input case that relative() exercises is byte-for-byte identical to before except that the Windows volume check now precedes the min-length scan — both are independent reads of input, so the reorder is inert. The 9+ path was demonstrably broken, so any behavior change there is the fix, not a regression risk. The three call sites (build_command.rs:406, build_command.rs:833, JSBundler.rs:902) all pass entry-point/output-path lists and already handle the fallback answers (.unwrap_or(b"."), or a separator prefix).
Other factors
- My previous review's only finding (the
stdout: "pipe"left undrained in the new cli.test.ts helper) was addressed in bbf618a by switching tostdout: "ignore", matching the two sibling tests above it. - The comment-cop bot's note about a paragraph-long comment was addressed in b541864.
- Test coverage is thorough: the Rust unit tests assert the same answer across 2–20 inputs and specifically cover the empty-input underflow; the integration tests cover both
Bun.build()and the CLI; the cli.test.ts test compares against the 2-entry baseline so it holds on all platforms without hardcoding platform-specific error text. - No CODEOWNER'd files touched.
|
CI status for the current head (9419775, rebased over #35644): build 100350 passed 178 of 179 jobs. The one failed shard is alpine x64, and the only thing failing in it is test/cli/install/bun-audit.test.ts (a crash in a JSC GC helper thread during an install test, after which the test file's local registry was gone), which this change does not touch; the same source diff passed that lane on every earlier build, and the previous head passed all 179 jobs on build 99804. It has been reported separately. The remaining annotations are tests that failed in a parallel batch and passed on retry. The new tests in bundler_naming.test.ts and cli.test.ts passed on every lane, alongside the tests #35644 added to cli.test.ts. |
bbf618a to
3f34818
Compare
There was a problem hiding this comment.
LGTM — deletes the broken 9+ arm and reuses the already-correct 2..=8 loop for all counts.
What was reviewed:
- Traced both functions: the surviving loop is byte-identical to the previous 2..=8 arm;
nql_at_index_dynalready took runtimen, so nothing about it depended on the ≤8 bound. - Checked the reordering in
longest_common_path: the Windows volume check now runs before themin_lengthscan instead of after, but only after the 0/1 early returns, soinput[0]/input[1..]are always valid and the 2-input path used byrelative()is behaviorally unchanged. - Confirmed the prior undrained-stdout nit was addressed (
stdout: "ignore"), and the comment-cop flag was resolved.
Extended reasoning...
Overview
The PR fixes get_if_exists_longest_common_path and longest_common_path in src/paths/resolve_path.rs. Both previously matched on input.len() with a correct 2..=8 arm and a broken _ (9+) arm: the 9+ arm compared inputs pairwise, only checked for a separator at the divergence byte (so last_common_separator was never set on the first mismatch), and executed index -= 1 with index == 0 when the shortest input was empty, panicking. The fix deletes the 9+ arm entirely and hoists the 0/1 early returns and the Windows volume check out of the match, so the existing 2..=8 loop now handles every count ≥ 2. The nql_at_index_fn assignment is simplified from wrapping closures to bare function pointers. Four Rust unit tests, an itBundled test pair (api/cli), and a CLI subprocess test are added.
Security risks
None. This is pure path-string prefix computation over already-collected entry-point strings; no parsing of untrusted external data, no filesystem or network I/O in the changed code. The panic being removed was a user-reachable abort (DoS-class), so the fix strictly reduces risk.
Level of scrutiny
Moderate. longest_common_path feeds relative(), which is on hot paths, so I checked that the 2-input case is unchanged: the loop body is identical, and the only reorder (volume check before min_length scan) is two independent O(n) passes with no data dependency. The bundler's implicit-root derivation is user-visible but low-risk — the fix makes 9+ entries match the documented and already-correct ≤8 behavior rather than introducing new semantics.
Other factors
Test coverage is thorough at three levels: Rust unit tests sweep 2–20 inputs plus the specific edge cases (directory-as-input, unrelated inputs, empty input, all-empty), itBundled covers both the JS API and CLI backends, and the CLI test compares the 9-entry error against the 2-entry error so it holds across platforms without hardcoding message text. All prior review feedback (comment-cop, my undrained-stdout nit) is resolved and confirmed in the current diff. CI on Linux and Windows is green apart from an unrelated pre-existing flake.
3f34818 to
d247ece
Compare
There was a problem hiding this comment.
LGTM — the broken 9+ arm is deleted and the existing 2..=8 loop (which already takes a runtime count via nql_at_index_dyn) now handles all counts ≥ 2.
What was reviewed:
- Confirmed the 2-input path used by
relative()is behavior-identical; only the Windows volume check moved above themin_lengthscan, which it doesn't depend on. - Checked
nql_at_index_dyn/nql_at_index_case_insensitive_dynloop1..string_countand are safe for arbitrary counts; the 0/1 early returns moved but produce the same values. - Prior feedback (undrained stdout pipe, long comment) is addressed and resolved in the current diff; the new tests cover both functions across 2–20 inputs, empty input, and both bundler backends.
Extended reasoning...
Overview
This PR fixes get_if_exists_longest_common_path and longest_common_path in src/paths/resolve_path.rs, which previously had two separate match arms for input counts: a correct 2..=8 arm and a broken _ (9+) arm. The 9+ arm compared input[0] against each other input pairwise, only checked for a separator at the divergence point (so last_common_separator was almost always unset at first mismatch), and executed index -= 1 when index == 0 for empty inputs — a wrapping underflow / debug panic. The fix deletes the broken arm entirely and lets the working loop handle all counts ≥ 2, since nql_at_index_dyn already takes the count as a runtime argument. The Windows volume check in longest_common_path is hoisted out of the match into a single loop over &input[1..]. The eta-expanded fn-pointer closures are simplified to bare function names. Net effect on the source is deletion of ~60 lines of broken code.
Tests added: four Rust unit tests in the same file (2–20 inputs give the same answer, directory-as-input, unrelated inputs match the 2-input answer, empty input among many), an itBundled test in bundler_naming.test.ts covering both API and CLI backends with 9 entry points, and a CLI test in cli.test.ts comparing 2-entry vs 9-entry behavior with an empty entry point.
Security risks
None. This is pure byte-slice comparison over paths already held in memory; no unsafe blocks are touched, no allocation, no external input parsing beyond what the callers already do.
Level of scrutiny
Medium. The functions feed the bundler's implicit-root computation and path.relative(), so a regression would be visible, but the change is a strict simplification: the surviving code path is exactly the one that has been producing correct answers for 2–8 inputs. I traced the 0/1/2-input cases before and after — the early returns moved above the min_length loop but yield identical values, and the 2-input path (the hot one via relative()) is byte-for-byte the same loop body. The Windows volume check moving before min_length is safe because it reads only input[0..] and returns early on mismatch.
Other factors
All prior review feedback is resolved: the comment-cop flag on a long test comment was addressed in b541864, and my earlier inline comment about the undrained stdout pipe was addressed in bbf618a (stdout: "ignore" now matches the two sibling tests). The author reports CI passed on all lanes that ran (Linux x64/aarch64/musl/ASAN, Windows x64/aarch64, darwin aarch64), with the two darwin 14 lanes expiring in queue due to saturation but passing on the previous identical diff. The PR description demonstrates each new test fails against the old implementation. This is a well-scoped, well-tested deletion of broken code.
d247ece to
03a36e4
Compare
There was a problem hiding this comment.
LGTM — the broken 9+ arm is deleted and the already-correct 2..=8 loop now handles every count; prior feedback (stdout pipe, comment-cop) is addressed.
What was reviewed:
- Traced that the surviving loop is byte-identical to the old 2..=8 arm, so
relative()'s 2-input path is unchanged (the Windows volume check moving before the min-length scan is order-independent). - Confirmed the only external callers are
build_command.rsandJSBundler.rs(implicit root derivation), both of which want the fixed behavior. - Checked the new tests: Rust unit tests cover 2–20 inputs / directory-as-input / unrelated / empty; the cli.test.ts empty-entry test compares 9-entry against 2-entry output so it holds cross-platform; the itBundled tests exercise both API and CLI backends.
Extended reasoning...
Overview
Removes the divergent 9+-input arm from get_if_exists_longest_common_path_generic and longest_common_path_generic in src/paths/resolve_path.rs. The old _ arm compared input[0] against each other input pairwise, only checked for a separator at the divergence byte (so last_common_separator stayed unset), and executed index -= 1 on index == 0 when the shortest input was empty — an underflow panic. The fix hoists the 0/1 early returns, then runs the existing 2..=8 loop (which already takes a runtime n via nql_at_index_dyn) for every count ≥2. The Windows volume check in longest_common_path_generic becomes a plain for over &input[1..]. The closure-to-fn-pointer wrapping (|n,i,inp| f(n,i,inp) → f) is a clippy cleanup. Adds 4 Rust unit tests, 2 itBundled tests (api+cli), and 1 CLI subprocess test.
Security risks
None. This is safe Rust operating on byte slices of entry-point path strings with bounds-checked indexing; the change only removes an underflow site and unifies control flow. No untrusted-input parsing beyond what already existed for ≤8 inputs.
Level of scrutiny
Moderate. longest_common_path is on relative()'s hot path with exactly 2 inputs, so behavior preservation there matters. I verified the surviving loop body is unchanged from the old 2..=8 arm; the only reordering is the Windows volume check moving before the (independent) min-length scan. For 9+ inputs the behavior does change — that is the fix — and the only callers (build_command.rs:406/833, JSBundler.rs:902) want the corrected implicit-root answer.
Other factors
- Both earlier review findings are resolved: the paragraph-long unit-test comment was removed (b541864) and the undrained
stdout: "pipe"becamestdout: "ignore"(bbf618a); the current diff reflects both. - CI on the current head passed 178/179 jobs; the one failure is an unrelated
test/bake/deinitialization.test.tsflake also seen on other builds from the same hour. - Test design follows REVIEW.md: the empty-entry CLI test compares the 9-entry run against the 2-entry run rather than snapshotting a platform-specific error string, and asserts the 2-entry stderr is non-empty so the equality can't be vacuously satisfied. The Rust unit tests likewise assert 9-input answers equal 2-input answers rather than hard-coding what "nothing in common" returns.
…as fewer longest_common_path and get_if_exists_longest_common_path switched to a separate loop past 8 inputs. That loop never recorded separators, so get_if_exists returned None at the first differing byte (the implicit bundler root of 9 or more entry points fell back to the cwd), and it stepped index below zero when one of the inputs was empty. Both functions now run the 2..=8 loop for any count.
03a36e4 to
9419775
Compare
There was a problem hiding this comment.
LGTM — deletes the broken 9+ arm and reuses the already-correct loop for all input counts.
What was reviewed:
- Verified the surviving loop is byte-identical to the previous
2..=8arm;nql_at_index_dynalready takes a runtime count so no upper bound applies. - Checked the Windows volume-check hoist in
longest_common_path_generic— pure reordering, no dependency onmin_length, and the 2-inputrelative()path is unchanged. - Confirmed the earlier
stdout: "pipe"nit was addressed (now"ignore"); new tests follow harness conventions and cover both backends plus Rust unit tests for 2..20 inputs, empty input, and the directory-as-input edge.
Extended reasoning...
Overview
This PR fixes two bugs in src/paths/resolve_path.rs's get_if_exists_longest_common_path_generic and longest_common_path_generic: with 9+ inputs the old _ match arm produced wrong answers (didn't track last_common_separator per-byte) and panicked on an empty input via index -= 1 at index == 0. The fix deletes that arm entirely and lets the existing 2..=8 loop handle all counts ≥2 — it already takes the count as a runtime argument. The Windows volume-ID check is hoisted out of the match into a single loop, and the nql_at_index_fn closure wrappers become direct fn references. Tests are added at three levels: Rust unit tests (cargo test -p bun_paths), itBundled integration tests for 9 entry points via both API and CLI backends, and a CLI subprocess test for the empty-entry-point crash.
Security risks
None. This is pure byte-slice comparison over path strings the caller already holds; no allocation, no syscalls, no user-facing parsing changed. The removed code was the only path that could underflow.
Level of scrutiny
Medium-low. The change is a code deletion that unifies two implementations onto the one that has been producing correct answers for the heavily-exercised 2-input relative() case and all builds with ≤8 entry points. The only non-deletion changes are (a) moving the 0/1 early returns above the min-length scan and (b) hoisting the Windows volume check — both are pure reorderings of independent reads with no data dependency between them. I traced the 2-input path (used by relative()) through the new code and it is behaviorally identical.
Other factors
- My previous inline nit (undrained
stdout: "pipe") was addressed in bbf618a; the test now usesstdout: "ignore"matching its siblings. - The comment-cop note about a paragraph-long comment was addressed in b541864.
- CI passed on all lanes per the author's report; the one Windows x64 failure is an unrelated
test/bake/deinitialization.test.tsflake also seen on other builds from the same hour. - The PR description demonstrates the fix was verified against the released build (tests fail there, pass with the change) and lists the additional test files that were run.
- Test coverage is thorough: the Rust unit tests assert equality between the 9-input answer and the 2-input answer for the edge cases (unrelated paths, empty input, all-empty), which directly pins the invariant the PR title states.
|
Sentry BUN-4QDT (bun 1.4.0, Windows, 1.4.0 includes #35644. That change sets Repro on the released build (1.4.0 canary, Linux): for i in 1 2 3 4 5 6 7 8; do echo "export const a$i = $i;" > file$i.ts; done
printf PNG > img.png
bun build --no-bundle --outdir=out file1.ts file2.ts file3.ts file4.ts file5.ts file6.ts file7.ts file8.ts img.png
# panic: index out of bounds: the len is 10 but the index is 18446744073709551615The same command with 7 I ran this branch's The copied entry itself is a separate problem that this PR does not claim: the summary prints an empty name, the file is written next to the source, and on Windows |
2 participants
Problem
bun buildandBun.build()derive the implicit root from the entry points withresolve_path::get_if_exists_longest_common_path. With 9 or more entry points it returnsNonefor practically any input, so the root falls back to the cwd: 8 files inpages/build toout/page0.js, the same build with 9 files writesout/pages/page0.js. docs/bundler/index.mdx (therootoption) documents the flat layout.bun build --outdir=out e0.ts ... e7.ts ""dies withpanic: index out of bounds: the len is 5 but the index is 18446744073709551615(debug build:attempt to subtract with overflowat src/paths/resolve_path.rs:282) instead oferror: ModuleNotFound resolving "" (entry point), which is what the same command with 2 entry points prints.bun build --no-bundle --outdir=dirwith 9+ entry points crashed the same way throughlongest_common_path(src/runtime/cli/build_command.rs:833), because every transform-only output had an emptydest_path. bun build --no-bundle: write output files when --outdir is set #35644 gives those outputs names (and fixes theENOENT: failed to write file '""'seen with 1 to 8 entries); this PR fixes the function itself, which any caller with an empty or 9th input still hits.input.len(). The2..=8arm runs one loop that compares every input at each byte and records the last separator it passed. The_arm (9+) comparesinput[0]against each other input in turn and only checks for a separator at the byte where the two diverge, so at the first mismatchlast_common_separatoris still unset (get_if_existsreturnsNone;longest_common_pathreturns a one byte prefix), and when the shortest input is empty it executesindex -= 1withindex == 0. The Zig version had the same two arms; this is not a port regression.Fix
nql_at_index_dyn), so it now runs for every count of 2 or more; the Windows volume check inlongest_common_pathbecomes one loop over the inputs./as the root directory.relative()is unchanged except that the volume check now runs before the length scan.naming/ImplicitOutbaseManyEntryPoints/{api,cli}: 9 entry points inpages/land directly inout/. On the released build both fail withBundle was not written to disk: .../out/page0.js.an empty entry point is reported the same way past 8 entry points: runsbun build --no-bundle --outdirwith 1 + 1 and with 8 + 1 entry points and expects the same first stderr line and exit code, so it holds on every platform. On the released build the 9 entry run exits 134 with the crash banner instead. It uses--no-bundlebecause the bundler's error exit still has the teardown race bundler: join in-flight pool tasks before tearing the bundle down #37480 fixes; the first version of this test went through the bundler and hit that race as a segfault on one CI lane.cargo test -p bun_paths): same answer for 2 through 20 inputs, a directory that is itself one of the inputs, unrelated inputs, an empty input. Against the previous implementation three of the four fail (None,".",attempt to subtract with overflow).cargo clippy -p bun_paths.--no-bundle with --outdirtests and the tests above all pass on the rebased tree.Background
--root/rootis not given. It is derived from the entry point strings before anything is resolved (src/runtime/cli/build_command.rs:406, src/runtime/api/JSBundler.rs:902), which is why an unresolvable entry point reaches this code at all.get_if_exists_longest_common_pathreturns the directory prefix shared by all of its inputs, orNonewhen they diverge before the first directory boundary (the callers then fall back to.).longest_common_pathis the same scan but always returns something: the separator when nothing is shared.relative()calls the latter with two inputs, so the 2-input loop is the heavily exercised one.no test proof · iteration 13 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/cli.test.ts