Repository navigation
Conversation
The --app arm of the --target check printed the error and then let the build continue, so the config was still evaluated and the build exited 0. Exit after printing, the same way the --bytecode arm next to it does.
|
Warning Review limit reached
Next review available in: 2 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 |
|
Status: fix pushed, waiting on CI. Reproduced on bun 1.4.0 with The new cases in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line fix that adds the missing Global::exit(1) to the --app target check, mirroring the --bytecode arm directly above it.
What was reviewed:
- Confirmed the
--bytecodearm atArguments.rs:2188-2199uses the identical print-then-Global::exit(1)pattern; the new line matches it exactly. - Tests follow harness conventions (
tempDir,bunEnv, concurrent pipe drain, exit code asserted last) and cover both rejected targets plus the--target=bunpass-through. - Verified
bunEnvsetsBUN_FEATURE_FLAG_EXPERIMENTAL_BAKEso the test runs on release builds; the stdout marker inbun.app.tsproves the build did not start.
Extended reasoning...
Overview
Single-line change in src/runtime/cli/Arguments.rs (parse_build_command_options): adds Global::exit(1) after the existing error print for bun build --app --target=<not-bun>. The --bytecode arm three lines above already does exactly this; the --app arm was written without the exit call. Accompanied by three new test cases in test/bundler/cli.test.ts.
Security risks
None. This is CLI argument validation for an experimental build mode; the change makes an already-printed error: line actually terminate the process instead of continuing to build.
Level of scrutiny
Low. The fix is mechanical — it copies the exit call from the sibling arm immediately above. Global::exit(1) is the established idiom for fatal argument errors throughout this function (per the PR description and confirmed by reading the surrounding code). The affected feature (--app / Bake) is experimental and behind a feature flag.
Other factors
- Tests are well-constructed:
test.eachovernode/browser, exact stderr assertion vianormalizeBunSnapshot, empty-stdout assertion using a config-file marker to prove the build never started, and a positive case for--target=bun. Pipes are drained concurrently, resources useusing/await using, and the tests are placed alongside the existing "CLI argument error messages" describe block. - Confirmed
bunEnvintest/harness.ts:86setsBUN_FEATURE_FLAG_EXPERIMENTAL_BAKE, so the test's comment about release-build coverage is accurate. - No prior reviewer comments to address; no outstanding concerns from the timeline.
Problem
bun build --app --target=node ./bun.app.tsprintserror: target must be 'bun' when using --app. Received: Nodeand then carries on: the config file is evaluated, the build runs, and the process exits 0 when the build succeeds.--apparm of the--targetcheck insrc/runtime/cli/Arguments.rs(parse_build_command_options) prints the message but never exits. The--bytecodearm directly above it prints the same kind of message and callsGlobal::exit(1).Arguments.zig): it was written as a copy of the--bytecodearm without itsGlobal.exit(1). The Rust port reproduced it as-is.Fix
Global::exit(1)after the error message, so--appwith a non-bun target behaves like--bytecodewith a non-bun target: one error line, exit 1, nothing built.--appbuild always produces a Bun server plus browser client bundles and never reads--target(src/runtime/bake/production.rsbuilds its own transpilers), so a different--targetis a request the build cannot honor. Printingerror:and then exiting 0 hides exactly that from scripts and CI.test/bundler/cli.test.ts("CLI argument error messages > --app --target").--target=nodeand--target=browsermust produce exactly the error line on stderr, nothing on stdout (the config file logs a marker when it is evaluated), and exit 1;--target=bunmust still start the build. The two rejected cases fail on the current release (stderr continues with the bake banner,Loading configurationand the config error, and the marker shows up on stdout) and pass with this change.detect_leaks=1, since the fix adds a new process-exit path during argument parsing.Background
bun build --appis the production build for Bun's experimental full-stack framework support (Bake). It is gated behind a feature flag that is on in canary and debug builds and can be enabled withBUN_FEATURE_FLAG_EXPERIMENTAL_BAKE=1; the test harness'sbunEnvsets that variable, which is how the test runs on release builds as well.Global::exit(1)flushes stdout/stderr and exits the process; it is the idiom used by the other fatal argument errors in this function (Global::crash()is the same thing with the exit code fixed at 1).