Conversation
|
Warning Review limit reached
Next review available in: 12 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 (2)
Comment |
|
Updated 1:33 AM PT - Aug 11th, 2026
✅ @robobun, your commit 9bc500a8a3b57eb8ced0242064f8679b3261ef00 passed in 🧪 To try this PR locally: bunx bun-pr 37389That installs a local version of the PR into your bun-37389 --bun |
|
Status: reproduced with bun 1.4.0 on both entry points. Fix and tests are in this PR. With |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped fix that turns a silent fallthrough into the same InvalidTarget error the incomplete-version case already returns.
What was reviewed:
- The
v1./v0.arm restructure:!parsed.validnow returnsInvalidTargetinstead of falling out of the arm withthis.versionleft at its default; thelet ... elsereplaces the threeunwrap()s with no behavior change on the valid path. - Error routing:
CompileTarget::frommapsInvalidTargeton any input containingvto the "complete version number" message, andcompile_target_from_slicethrowsUnknown compile target: <full string>— both match what the new tests assert. - Tests cover both entry points (Bun.build and CLI) across the malformed shapes plus a control that names the running version so no lane downloads; the
readdirSynccheck confirms nothing is written on the CLI reject path.
Extended reasoning...
Overview
The PR fixes a silent-failure bug in CompileTarget::try_from (src/options_types/compile_target.rs): when a --target token starts with v1./v0. but Version::parse returns valid = false (e.g. v1.2.3.4.5, v1..2, v0.a), the old code had no else branch and fell out of the arm without a continue, so the token was dropped and this.version kept its default (the running bun's version). The build then succeeded with the wrong version and exit code 0. The fix returns ParseError::InvalidTarget on !parsed.valid, matching what the incomplete-version case (major/minor/patch is None) already did. The arm is also restructured to use let (Some, Some, Some) = ... else in place of three unwrap()s, which is a pure refactor on the valid path.
Tests are added in test/bundler/bun-build-compile.test.ts: a test.each over 8 malformed/incomplete targets against both Bun.build (throws Unknown compile target: ...) and the CLI (exits 1 with the "complete version number" message, writes nothing), plus a control that compiles for bun-<host>-v<running version> to confirm the happy path still works without a download. An unrelated pre-existing test that compiles three binaries gets a 60s timeout to match its siblings under debug+ASAN.
Security risks
None. This is input validation on a CLI/API option string; the change strictly tightens validation (previously-ignored malformed tokens now error). No untrusted-data parsing, no memory management, no network or filesystem effects beyond what the existing code already did.
Level of scrutiny
Low-to-medium. The Rust change is ~20 lines with a net +2; the only behavioral delta is one new early-return on a path that was previously a silent no-op. I traced the error through both consumers: CompileTarget::from (CLI) hits the contains(input, b"v") branch and prints the version hint; compile_target_from_slice (Bun.build) throws with the full input string including the bun- prefix, which matches the test's .toThrow(Unknown compile target: ${target}). The UnsupportedTarget re-scan in from() still skips v1./v0. tokens, which is consistent because those now always resolve in try_from (either as a version or as InvalidTarget).
Other factors
- The tests follow harness conventions:
tempDirwithusing,bunEnv/bunExe, concurrent pipe drain viaPromise.all, exit-code asserted last,test.eachfor the matrix. - The control test uses the host platform + running version so
is_default()is true andself_exe_path()is used — no network on any CI lane. - The
currentPlatformTargetcomputation was hoisted from an existing test to module scope; the original test now reuses it, no behavior change there. - The PR description explicitly verified the new tests fail without the src change and pass with it.
- No prior human reviews or outstanding comments on the PR.
|
Heads up on the |
### What
`test/bundler/bun-build-compile.test.ts` "compile with relative outfile
paths" ran three `Bun.build({ compile: { outfile } })` calls inside one
test. Every compile copies and rewrites the whole bun binary, which is
about 1 GB with a debug+ASAN build and takes roughly 2.5s there (every
single-compile test in the file reports 2.4s to 2.9s), so the test
needed 7s or more of a 5s default per-test budget. On the unmodified
file, `bun bd test test/bundler/bun-build-compile.test.ts` at
9fcdea8:
```
(fail) Bun.build compile > compile with relative outfile paths [5523.70ms]
^ this test timed out after 5000ms.
(pass) Bun.build compile > compile with embedded resources uses correct module prefix [5227.91ms]
```
The failure is intermittent (the session that reported it saw it in
about 1 run in 3, with the passing runs taking 7.0s to 7.2s). The
compile step runs on the JS thread inside the bundle completion task
(`do_compilation` is called from `JSBundleCompletionTask::on_complete`),
so the timeout can only be observed between compiles: when the first two
compiles add up to more than 5s the test fails after the second one,
otherwise the third compile starts and the test finishes late but
passes. The third compile of a timed-out run also keeps going and slows
down the next test, which is why "embedded resources" shows 5.2s above
and 2.5s once this is fixed.
This only affects the 5s default used by a plain `bun test` / `bun bd
test` run; the CI runner passes a much larger `--timeout`, and with a
release binary each compile takes about 0.13s.
### Fix
Turn the three cases into a `test.each`, so every test does exactly one
compile, the same shape as the rest of the file. The per-test timeout is
left at the default rather than raised: the workload per test is what
was out of proportion.
While rewriting the assertion, the three `toContain` / `toEndWith`
checks on the reported path become an exact comparison against the
outfile that was passed in (`.exe` appended on Windows; with no `outdir`
the artifact path is the outfile verbatim), and each case also checks
that the executable exists at that path, which is what the nested
`output/nested/` and `a/b/c/d/` cases are there to prove. Nothing else
in the file changes.
### Verification
`bun bd test test/bundler/bun-build-compile.test.ts` (debug+ASAN), two
runs, 14 pass / 1 skip each, about 40s per run:
```
(pass) Bun.build compile > compile writes the executable to outfile output/nested/app1 [2567.03ms]
(pass) Bun.build compile > compile writes the executable to outfile app2 [2576.66ms]
(pass) Bun.build compile > compile writes the executable to outfile a/b/c/d/app3 [2590.81ms]
(pass) Bun.build compile > compile with embedded resources uses correct module prefix [2501.16ms]
```
The same file also passes with a release bun on Linux
(`USE_SYSTEM_BUN=1`) and on Windows Server 2019 x64 (release bun 1.4.0,
where the exact path assertion exercises the `.exe` branch).
Related: #37389 raises this test's timeout to 60s as a side change of an
unrelated fix; whichever lands second drops or rebases a one-line hunk.
#37397 fixes the neighbouring "embedded resources" test in the same file
and does not overlap with this hunk.
CompileTarget::try_from only handled the case where the v1.x / v0.x token parsed as a valid version. When Semver rejected the token (bun-linux-x64-v1.2.3.4.5, -v1..2, -v0.a, -v1.2.3.) the branch fell through without setting anything or returning an error, so the token was dropped and the build compiled for the running version of bun. Version::parse is also lenient about what it accepts: it turns trailing text into a tag and reads an overflowing component as 0, so -v1.2.3rc1 selected 1.2.3 and -v1.99999999999999999999.7 selected 1.0.7. Compare the parsed version against the token instead, so the token has to name exactly the version that will be downloaded. The version failure gets its own ParseError variant so the CLI message no longer depends on substring matching of the whole target string, which reported "musl libc only exists on linux" for bun-musl-v1.2.3.4.5. Bun.build keeps throwing "Unknown compile target". Tests run both entry points in child processes with the download pointed at a local 404 server, so a token that gets past the parser fails offline with a message naming the parsed target. That also covers the well formed case: -v1.2.3 reaches the download step as v1.2.3.
d40ddc3 to
89a441f
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
LGTM — focused parser tightening with a clear root cause and thorough offline tests.
What was reviewed:
parse_exact_versionround-trip check covers all the malformed shapes (extra components, trailing text, overflow, unparseable) with one rule; validvX.Y.Zstill passes.- Removing the
contains(input, b"v")heuristic from theInvalidTargetarm is safe: the version arm now always consumesv1./v0.tokens, and the remainingInvalidTargetproducers (musl/android on non-linux, wasm) are still matched by their own substring checks. compile_target_from_sliceinoptions_jsc.rsignores the error variant, so theBun.buildmessage is unchanged ("Unknown compile target: ...") as the tests assert.- Tests are hermetic (local 404 server + isolated cache dir), cover CLI and API, include a positive control, and follow harness conventions.
Extended reasoning...
Overview
Two files: src/options_types/compile_target.rs tightens the --target version-token parser and adds a ParseError::InvalidVersion variant with its own CLI message; test/bundler/bun-build-compile.test.ts adds a describe("version token in the target") block with 21 test cases across CLI and Bun.build entry points. The existing "compile with current platform target string" test is refactored to share the hoisted currentPlatformTarget constant.
Security risks
None. This is CLI/API argument validation for bun build --compile. The change strictly rejects more inputs than before; nothing new is accepted. No memory management, no untrusted parsing beyond what Version::parse already did, no network or filesystem changes.
Level of scrutiny
Low-to-medium. The parser is a one-shot CLI-arg path (not hot), the format! round-trip is a simple and robust way to enforce exact X.Y.Z, and the change is a strict tightening — the failure mode is a loud error where there was previously silent wrong behavior. I traced the two other consumers of ParseError: CompileTarget::from (CLI, updated in this PR) and compile_target_from_slice in bundler_jsc/options_jsc.rs (JS API, uses let Ok(...) else so the new variant maps to the existing "Unknown compile target" message without changes). I also confirmed the removed contains(input, b"v") heuristic is dead: after this change, InvalidTarget is only returned for libc-on-non-linux (input contains "musl"/"android") or wasm (input contains "wasm"), each of which has its own message branch, so nothing falls through to the generic fallback that previously needed the v check.
Other factors
Tests follow the repo's conventions closely: tempDir/bunEnv/bunExe, await using on spawns, concurrent stdout/stderr/exited drain, test.each for the matrix, port: 0 local server with afterAll cleanup, and a positive control that proves a valid v1.2.3 still reaches the download step with the version intact. The comment-cop bot flagged verbose comments twice; both were addressed in b1a14eb and 9bc500a (helper extracted, doc comment cut to one line), and those threads are resolved. The PR description documents that the new tests fail on main and pass with the fix.
This branch has not been deployed
1 participant
Repro
The version token is not a version, but the build succeeds and
outis built from the running bun (1.4.0 here), with no download.Bun.build({ compile: { target: "bun-linux-x64-v1.2.3.4.5" } })does the same.-v1.2.3.,-v1..2and-v0.atake the same path. Two more shapes get through differently:-v1.2.3rc1downloads 1.2.3 and-v1.99999999999999999999.7downloads 1.0.7. By contrast-v1.2and-v1.are already rejected ("Please pass a complete version number to --target"). Reproduced with bun 1.4.0.Cause
CompileTarget::try_from(src/options_types/compile_target.rs) walks the-separated tokens; every arm either records the token or returns an error, except thev1./v0.arm. It callsVersion::parseand only handlesversion.valid: when Semver rejects the token there is no else branch, so the loop moves on to the next token andthis.versionkeeps its default, the running version. The Zig original had the same shape.Version::parseis also lenient by design (it is the package manager's parser): trailing text becomes a tag and a component that overflows is read as 0 (parse_version_numberdoesunwrap_or(ZERO)), withvalidstill set. So checkingvalidalone would still let-v1.2.3rc1and the overflowing token select a version the user did not write.Fix
The arm now requires the token to print back as itself: after taking major/minor/patch out of the parse result (a missing component is rejected as before),
format!("{major}.{minor}.{patch}")has to equal the text after thev. Anything else isParseError::InvalidVersion. That covers the dropped tokens, trailing text and overflow with one rule, and it is the rule that matters for this token: the download URL and cache name are built from exactly those three numbers, so the token has to name that version and nothing else.Why reject rather than keep the lenient readings: this token exists to pin which bun the executable is built from. Building from a different one (the running bun, 1.2.3 instead of 1.2.3rc1, 1.0.7 instead of the written version) while exiting 0 is the one outcome
--targetis there to prevent.-v1.2.3rc1-style tokens were never meaningful for this flag: pre-release bun builds are not published under these versions, and anything with a-in it is split into separate tokens before this arm sees it.InvalidVersionis its own variant becauseCompileTarget::fromused to pick the CLI message by substring matching the whole target string, checking formusl/androidbeforev; with the version arm now rejecting more inputs,bun-musl-v1.2.3.4.5would have been reported as "musl libc only exists on linux".from()prints the existing version message for the new variant and no longer needs thevheuristic; the musl / android / wasm messages for the remainingInvalidTargetproducers are unchanged.Bun.build(compile_target_from_slice) ignores the variant and keeps throwingUnknown compile target: ....The
UnsupportedTargettoken scan infrom()still treatsv1./v0.tokens as known, which stays correct: the version arm now always consumes them, either as a version or asInvalidVersion.Verification
New tests in
test/bundler/bun-build-compile.test.ts("version token in the target"). Both entry points run in a child process whose download is pointed at a localBun.servethat returns 404 (BUN_COMPILE_TARGET_TARBALL_URL, plusBUN_INSTALL_CACHE_DIRset to an empty directory), so every case runs offline and a target that gets past the parser fails immediately with an error naming the parsed target.bun-<host platform>-v1.2.3reaches the download step asv1.2.3on both entry points ("Target platform 'bun-linux-x64-v1.2.3' is not available for download",Bun.buildwiththrow: falsereturnssuccess: falsewith that log), which checks that the version actually lands in the target rather than just that the token is accepted.bun-v1.2.3.4.5, the trailing-text and overflow tokens,bun-musl-v1.2.3.4.5(CLI reports the version message, not the musl one), and the two incomplete shapes that were already rejected. The CLI cases check the message, an empty stdout, exit code 1 and that nothing was written; theBun.buildcases check the exact thrown message.With
src/options_types/compile_target.rsat main, the 8 newly rejected targets fail on both entry points (16 failures; the five dropped shapes build successfully, the other three hit the local server); the control and the two incomplete shapes pass either way. With the change, the whole file passes underbun bd test.#37385 (a
glibctoken) edits the same token loop and the same test file; whichever lands second has a small textual rebase, the two changes do not interact otherwise.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-compile.test.ts