Conversation
|
Warning Review limit reached
Next review available in: 4 minutes 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 11:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 06fbc4b has some failures in 🧪 To try this PR locally: bunx bun-pr 38749That installs a local version of the PR into your bun-38749 --bun |
|
Status: fixed, tests in place, ready for review. CI is green on every job that ran; the remaining red is a CI capacity problem, not this change. Reproduced on the current release with The three new tests in CI for 06fbc4b (build 97342): 177 of 179 jobs passed, including the new tests on every Linux lane; the entries in the failure list are unrelated tests that passed on retry. The other two jobs are the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — swaps an unchecked path-buffer join for the checked variant so an over-long --destination reports archive destination name too long instead of panicking.
What was reviewed:
join_abs_string_buf_checkedsemantics inresolve_path.rs— returnsNoneonly when the normalized result overflows, so<5000 bytes>/..still succeeds (covered by the third test).- The new
err_genericcall's 2-tuple matches the two-placeholder format string and mirrors the sibling--filenameoverflow branch;without_trailing_slashonpack_destinationkeeps the message stable for the trailing-/--dry-runcase. - Tests:
tempDir+await using, pipes drained concurrently, exit-code asserted last,test.concurrent, Windows skip justified — all follow the file's existing patterns.
Extended reasoning...
Overview
One-line functional change in src/runtime/cli/pack_command.rs: tarball_destination() now calls resolve_path::join_abs_string_buf_checked instead of the unchecked join_abs_string_buf, and the new None arm emits the same archive destination name too long error the function already produces when the directory fits but the tarball filename does not, then Global::crash()s (exit 1). Three new tests in test/cli/install/bun-pack.test.ts cover the error (with and without --dry-run) and the normalization-into-range case.
Security risks
None. The change replaces an unchecked slice write on user-controlled input with a bounds-checked one — strictly a hardening. No new inputs, no new I/O, no allocation on the happy path (the checked helper only heap-allocates a scratch buffer when the raw input already exceeds buf.len()).
Level of scrutiny
Low. This is a targeted crash fix on a CLI validation path. I confirmed against src/paths/resolve_path.rs:1640 that the checked variant has an identical signature to the unchecked one modulo Option<>, normalizes before deciding (so .. collapses correctly), and is already used the same way at ~8 other call sites. The new error branch reuses the exact Output::err_generic + Global::crash() pattern from the three neighbouring error paths in the same function, and the format-string arity matches the FmtTuple argument.
Other factors
Tests follow the harness conventions from REVIEW.md: tempDir with await using for cleanup, Buffer.alloc(5000, "d") instead of .repeat(), concurrent pipe draining via Promise.all, exit code asserted after stderr, test.concurrent for independent subprocess spawns, and a commented skipIf(isWindows) explaining why the overflow is unreachable there. The tempDir-as-cwd pattern is already established elsewhere in this file. The PR description notes overlap with #38739/#38743, but this change is self-contained and whichever lands second is a trivial rebase.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The overlap with #38739 is intentional and called out in the description. #38739 moves this join to the checked variant as part of changing what relative |
tarball_destination() joined the user-supplied --destination into a PathBuffer with the unchecked join_abs_string_buf, which panics with a slice bounds error once the joined directory itself is longer than the buffer (PATH_MAX). Use join_abs_string_buf_checked and report the existing "archive destination name too long" error when it does not fit.
edd1fa3 to
ab13275
Compare
There was a problem hiding this comment.
LGTM — swaps the unchecked path join for the checked variant and reports the existing "archive destination name too long" error instead of panicking.
What was reviewed:
join_abs_string_buf_checkedsemantics inresolve_path.rs— returnsNoneon overflow, still succeeds when..normalizes the path back into range.let-elsedivergence viaGlobal::crash() -> !, and the 2-tupleFmtTuplearg correctly filling both{}in the error template.- Tests: concurrent subprocess spawns drain pipes via
Promise.all, assert stderr before exit code, useBuffer.allocper harness conventions, and cover both call sites (normal +--dry-run) plus the normalizes-to-fit positive case; Windows skip is justified in the comment.
Extended reasoning...
Overview
The PR touches one function in src/runtime/cli/pack_command.rs (tarball_destination) and adds three tests to test/cli/install/bun-pack.test.ts. The Rust change replaces resolve_path::join_abs_string_buf with resolve_path::join_abs_string_buf_checked and handles the None case with the same Output::err_generic + Global::crash() pattern the sibling --filename branch and the downstream write! failure already use. Nothing else in the function changes: the trimmed/full length pair and the subsequent cursor write are reached only when the checked join returns Some, so the existing narrow-window error at the write! still fires for directories that fit but leave no room for the tarball name.
Security risks
None new. This is validation of user-supplied CLI input (--destination) against a fixed-size PathBuffer before writing into it, replacing a slice-index panic (already bounds-checked in Rust, so no memory corruption — just an ugly crash report) with a proper error message and exit 1. No new code paths accept or forward untrusted data.
Level of scrutiny
Low-to-medium. This is a focused crash-to-error conversion in a CLI flag handler. The checked helper is documented for exactly this use ("user-controlled parts") and is already used elsewhere in the codebase. I verified Global::crash() is -> ! so the let-else diverges, that FmtTuple is implemented for 2-tuples of Display values so the (BStr, TarballNameFormatter) argument correctly fills both {} placeholders, and that join_abs_string_buf_checked normalizes before length-checking (so <5000 bytes>/.. still succeeds, which the third test verifies).
Other factors
The tests follow the repo's conventions closely: test.concurrent for independent subprocess tests, await using + tempDir, Buffer.alloc(n, fill) instead of repeat, pipes drained concurrently, stderr asserted before exit code, negative assertion that no tarball is written, and a commented skipIf(isWindows) with the reason (Windows' path buffer exceeds max command-line length). The await using dir = tempDir(...) + pack(dir, ...) pattern is already used elsewhere in this file. The one CI failure (test-http-chunk-problem.js) is unrelated to bun pm pack and fails on main per the author's follow-up. The overlap with #38739 is acknowledged in the description and is a trivial rebase either way.
…-destination with it Several tests need "one byte more than bun's path buffer"; give the platform table one home in harness.ts instead of a literal per test.
There was a problem hiding this comment.
LGTM — targeted crash fix that swaps the unchecked path join for the checked variant and reports the existing "archive destination name too long" error instead of panicking.
What was reviewed:
- Verified
join_abs_string_buf_checkedreturnsOptionon overflow and normalizes first, so the<long>/..positive test is sound. - Confirmed the harness
MAX_PATH_BYTESvalues matchsrc/bun_core/util.rs(Windows 32767*3+1, Linux/Android 4096, else 1024). - Checked the sibling
--filenamebranch already length-checks, so both entry points totarball_destinationnow cover the overflow. - Tests follow repo conventions:
Buffer.alloc(N, fill), concurrent pipe drain, exact error assertion, per-testtempDir, Windows skip with a stated reason.
Extended reasoning...
Overview
The PR fixes a panic in bun pm pack --destination=<path longer than MAX_PATH_BYTES>, which currently aborts with range end index N out of range for slice of length 4095 (exit 134). The fix is a one-function change in src/runtime/cli/pack_command.rs::tarball_destination(): the unchecked resolve_path::join_abs_string_buf becomes join_abs_string_buf_checked, and the new None arm reports the same archive destination name too long error the function already emits when the directory fits but the appended filename does not, then Global::crash()s (exit 1). Three subprocess tests are added to test/cli/install/bun-pack.test.ts (normal pack, --dry-run with a trailing slash to exercise without_trailing_slash, and an over-long path that normalizes back into range via /..), and MAX_PATH_BYTES is exported from test/harness.ts to size the test input.
Security risks
None. The change is defensive: it converts a user-reachable slice-index panic on an over-long CLI argument into a clean error and exit(1). No new I/O paths, no allocation-sizing changes, and join_abs_string_buf_checked decides on the normalized result, so it does not reject legitimate inputs that would have fit.
Level of scrutiny
Low-to-medium. This is a mechanical swap to the checked variant of an existing helper, mirroring how the --filename branch of the same function already length-checks its input. The error path uses the same Output::err_generic + Global::crash() pattern as the four other error sites in tarball_destination(). I verified that err_generic accepts a FmtTuple and that the test asserts the exact rendered string, so the 2-tuple form (vs the sibling's format_args!) is not just compiling but producing the expected output. The harness constant matches src/bun_core/util.rs platform-for-platform, and the test's Windows skip is justified (path buffer ≈ 96 KiB there, larger than any command line).
Other factors
- The tests follow REVIEW.md conventions closely:
Buffer.alloc(N, fill).toString()instead of.repeat(),Promise.allfor concurrent pipe drain,test.concurrentwith per-testtempDir(no shared state via the file-levelpackageDir), exit code asserted last, exact error string asserted, and a negative check that no tarball was written. - The only CI failure on the first push was
test-http-chunk-problem.js, an unrelated main-branch break the author rebased past; the pack tests passed on every lane. - The overlap with #38739 is documented in the description and the author's follow-up comment; this PR is the crash-fix subset with tests, so it can land independently.
- No prior review comments to address; no CODEOWNERS constraints on these files.
… pack output (#40959) ### Problem - `test/cli/install/bun-pack.test.ts` takes 10.7s on debian 13 x64-asan in the serial phase (build 108487). Its 80 tests run one at a time, each with one to five `bun pm pack` spawns. - The assertions are loose: the harness `pack()` helper only checks that stderr lacks `error:`, `warning:`, `failed` and `panic:`, tarballs are checked with `toMatchObject`, and the `--filename="out/foo.tgz"` error case accepts any outcome. ### Fix - Each test builds its tree with `tempDir` instead of the shared `beforeEach` directory. The describes are `describe.concurrent`, the top-level tests `test.concurrent`. - A local `runPack()` returns stdout and stderr, raw and normalized with `normalizeBunSnapshot`. The normalized stdout masks the shasum, the integrity and the packed size, which depend on the compressor. - Every test asserts that `err` is `""` (or the exact `$ script` echo), the exact stdout, the exit code, and the full entry list with `toEqual`. Error cases assert the exact message and that nothing was written. - Verified: local debug+ASAN build, 80 tests in 20.6s and 21.8s before, 83 tests in 6.9s, 6.9s and 7.0s after. `--rerun-each=3` passes 249 of 249. CI debian 13 x64-asan: 10.7s before, 3.0s after (build 108529). ### Background - `describe.concurrent` runs a group's async tests up to `--max-concurrency` at a time (20, or 5 in ASAN builds). Groups and top-level `test.concurrent` tests overlap, so a shared module-level directory is not safe. - `toMatchInlineSnapshot` works in concurrent tests, but one call site cannot hold different values across `test.each` rows. The tables compare a line array instead. <details><summary>Notes</summary> - Test count 80 to 83: `--gzip` is split into three rejected-level cases and one level 0 vs level 9 case, and the `--filename="out/foo.tgz"` error row is its own test. No test was removed or skipped. - `readTarball` from `bun:internal-for-testing` parses a tarball into its entries, shasum and integrity. - Lines that use `expect.stringMatching` instead of an exact value: the package.json size and the unpacked size in the tables whose rows change package.json (scoped names, `workspace:` specs, `bundledDependencies` spelling), and in the two lifecycle tests whose scripts embed `bunExe()`, so the size depends on the path of the bun binary. On the darwin CI agent that path pushes package.json past 512 bytes and the size prints as `0.58KB`, so those two matchers accept any size format (build 108529 caught the `NNNB`-only version). - The exact output records some current behavior as-is: the name `//` writes `-1.1.1.tgz` but prints `//-1.1.1.tgz`; the name `@//` fails with `failed to open tarball file destination: ".../-/-1.1.1.tgz"` (the old test only asserted a non-zero exit); transitive scoped bundled deps print without their scope (`bundled dep3` for `@scoped/dep3`); `--dry-run` prints the on-disk package.json size while a real pack prints the re-serialized size; empty files print as `0KB`. None of these is changed here. - `bun install` still runs once per `workspace:` lockfile case (7 runs). They are workspace-only and contact no registry. The `bundledDependencies` tests already built `node_modules` on disk. - The release binary runs the file in 0.19s locally. Under ASAN each spawned pack still costs 150 to 400ms, so what remains is CPU bound: about 85 debug `bun pm pack` runs, 5 at a time. - Open PRs that add cases to this file (#36266, #38715, #36699, #38813, #38721, #38739, #38835, #38720, #38749, #38784, #38707, #38716) need a rebase onto the new shape: a `tempDir` tree plus `runPack(dir)`. - CI durations before, build 108487 serial phase: 10.7s debian 13 x64-asan, 2.0s windows 11 aarch64, 1.5 to 1.7s alpine, about 1s on the other release lanes. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-pack.test.ts <!-- robobun:evidence:end -->
1 participant
Problem
bun pm pack --destination=<value longer than the path buffer>aborts with a crash report instead of an error:panic: range end index 5014 out of range for slice of length 4095(exit 134). Same with--dry-run.tarball_destination()insrc/runtime/cli/pack_command.rsjoins the raw--destinationvalue into aPathBufferwithresolve_path::join_abs_string_buf, which is unchecked and indexes past the buffer when the joined directory does not fit.archive destination name too longerror only fires in the narrow window where the directory fits but/<name>-<version>.tgzdoes not (thewrite!after the join is bounds-checked). On Linux with a 15-byte cwd that is a destination of 4060 to 4081 bytes; from 4082 up it panics. macOS has a 1024-byte buffer, so the panic starts much lower there.Fix
resolve_path::join_abs_string_buf_checked, which returnsNoneinstead of overflowing, and report the samearchive destination name too long: "<destination>/<name>-<version>.tgz"error (exit 1) in that case, the way the--filenamebranch already handles an over-long--filename.--destination=<over-long>/..still packs into the package directory, as it would for a short path, instead of being rejected on raw length.write!check, and thedest_buf[dir_end]access inpack()is only reached after that check, so every length now ends in one of the two errors or a packed tarball.test/harness.tsgainsMAX_PATH_BYTES(the platform table behindPathBuffer); the new tests build their destination from it. pack: fill every placeholder in multi-argument error messages #38743 and publish: report ENAMETOOLONG for a tarball path that does not fit the path buffer #38753 each carry a private copy of the same table and can switch to this one.test/cli/install/bun-pack.test.ts(--destination longer than the path buffer): the error for a normal pack and for--dry-run(the two call sites an over-long--destinationcan reach;bun publishdoes not accept the flag), plus the/..case above. The three tests fail on the current release (panic) and pass with this change; the whole file (79 tests) passes. They are skipped on Windows, whose path buffer is larger than any command line.Relationship to the other open
tarball_destinationPRs--destination/--filenameagainst the invoking directory, a behavior change) rewrites the whole function and contains this PR'sNonearm verbatim; once this lands, its rebase drops that duplicate and its overflow bullet. pack: fill every placeholder in multi-argument error messages #38743 only touches the argument lists of the neighbouring error messages. Neither has a test for this overflow.bun publish <over-long tarball path>), fixed inpublish_command.rs.Background
PathBufferis a fixed[u8; MAX_PATH_BYTES]scratch buffer (4096 bytes on Linux, 1024 on macOS, about 96 KiB on Windows) that path helpers write into.join_abs_string_buf(cwd, buf, parts)resolvespartsagainstcwd(likepath.resolve) and writes the normalized result intobuf, assuming it fits.join_abs_string_buf_checkedis the variant documented for user-controlled parts: it normalizes into a temporary buffer when the input is long and returnsNoneif the normalized result is longer thanbuf.Output::err_generic+Global::crash()is how this file reports fatal user errors;Global::crash()isexit(1), not an abort.Repro
Earlier revision
The first push sized the destination with a literal 5000 bytes and had no harness change; the test was then moved onto the shared
MAX_PATH_BYTESconstant. Thepack_command.rshunk is unchanged since the first push.