Skip to content

cli: name --target and echo the rejected value when it is invalid - #34315

Open
robobun wants to merge 1 commit into
mainfrom
claude/c036e5d8/fix-invalid-target-error-message
Open

cli: name --target and echo the rejected value when it is invalid#34315
robobun wants to merge 1 commit into
mainfrom
claude/c036e5d8/fix-invalid-target-error-message

Conversation

@robobun

@robobun robobun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

What

bun build --target=<invalid> lost both the flag name and the rejected value in its error message, and the --bytecode / --app / --server-components conflict errors leaked Rust Debug formatting (Received: Browser instead of Received: browser).

Repro

$ bun build e.ts --target=nodejs --outfile x.js
error: Invalid Argument ''

$ bun build e.ts --bytecode --target=browser --outdir o
error: target must be 'bun' when bytecode is true. Received: Browser

Cause

invalid_target() in src/runtime/cli/mod.rs took the rejected value as _target but never read it, reporting through Diagnostic::report with Error::InvalidArgument. Because --target itself parsed fine (only its value was rejected), the Diagnostic carried no arg name, so the renderer emitted an empty ''. This regressed in #33909 which collapsed the former InvalidTarget variant into the generic InvalidArgument path.

Separately, three conflict errors in Arguments.rs formatted bun_ast::Target with {:?} (Browser, Node, ...) instead of the user-facing spelling.

Fix

  • invalid_target() now prints Invalid value for --target: "<value>". Must be 'browser', 'node', 'macro', or 'bun'. via Output::err_generic + bun::fmt::quote, matching the existing --format error. The now-unused diag parameter is dropped (and with it from parse_build_command_options).
  • The --bytecode, --app, and --server-components errors echo the user's --target value verbatim instead of the Debug repr of the enum.

After

$ bun build e.ts --target=nodejs --outfile x.js
error: Invalid value for --target: "nodejs". Must be 'browser', 'node', 'macro', or 'bun'.

$ bun build e.ts --bytecode --target=browser --outdir o
error: target must be 'bun' when bytecode is true. Received: browser

Tests

Added to the existing CLI argument error messages block in test/bundler/cli.test.ts:

  • test.each over nodejs / web / deno / empty string for the invalid-target message
  • --bytecode --target=browser for the lowercase spelling

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/cli.test.ts

invalid_target() discarded its value argument and reported through the
generic Diagnostic path with no arg name set, producing
"error: Invalid Argument ''". The message now names --target, quotes
the rejected value, and lists the valid choices, matching the --format
error.

Also stop Debug-formatting bun_ast::Target in the --bytecode / --app /
--server-components conflict errors (which printed "Browser" instead
of "browser"); echo back the user's --target value instead.
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1ae2e684-4843-49ea-8f4a-19535a59aa0f

📥 Commits

Reviewing files that changed from the base of the PR and between 4bbe075 and 75b63a9.

📒 Files selected for processing (3)
  • src/runtime/cli/Arguments.rs
  • src/runtime/cli/mod.rs
  • test/bundler/cli.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 2:24 AM PT - Jul 16th, 2026

@robobun, your commit 75b63a9 has 3 failures in Build #73717 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34315

That installs a local version of the PR into your bun-34315 executable, so you can run:

bun-34315 --bun

@robobun

robobun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

test/bundler/cli.test.ts (including the 5 new tests) passes on all lanes. Fail-before verified with USE_SYSTEM_BUN=1.

CI build #73717 has three unrelated red tests, all of which are also failing on other branches and already being tracked separately:

  • test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js (pre-existing JSC assertion on x64-asan)
  • test/js/node/test/parallel/test-net-connect-memleak.js (pre-existing GC flake on alpine)
  • test/cli/run/require-cache.test.ts (RSS threshold 74 MB vs 64 MB on darwin 26 aarch64; main #73716 had a build failure on that lane so no baseline)

None of these touch CLI argument parsing; this diff only changes error-message text on --target error paths.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — small, self-contained CLI error-message fix.

Checked that invalid_target and parse_build_command_options have only the one call site each, so the dropped diag param is fully accounted for. The --server-components case re-reads args.option(b"--target") because the raw byte slice is out of scope there (only the enum is bound); unwrap_or_default is defensive but the non-server-side branch is only reachable when --target was passed. New tests mirror the existing --format test in the same describe block.

Extended reasoning...

Overview

This PR fixes two cosmetic CLI error-message regressions in bun build:

  • invalid_target() in src/runtime/cli/mod.rs was discarding the rejected value and reporting an empty Invalid Argument ''. It now prints Invalid value for --target: "<value>". Must be 'browser', 'node', 'macro', or 'bun'. via Output::err_generic + bun::fmt::quote, matching the existing --format error path. The now-unused diag parameter is dropped from the signature and from parse_build_command_options.
  • Three conflict errors in Arguments.rs (--bytecode, --app, --server-components vs non-bun target) were formatting the enum with {:?} (Browser) instead of echoing the user's spelling (browser). They now print BStr::new(target) (or re-read args.option(b"--target") for the server-components block where the raw slice is out of scope).

Tests are added to the existing CLI argument error messages block in test/bundler/cli.test.ts.

Security risks

None. This changes only error-message text on already-failing CLI paths that immediately Global::exit(1) / Global::crash(). No parsing, validation, or control-flow semantics change.

Level of scrutiny

Low. The diff is ~30 lines of Rust touching cold error paths and ~40 lines of test. I verified via grep that invalid_target has exactly one call site (Arguments.rs:1996) and parse_build_command_options has exactly one call site (Arguments.rs:1348), both updated. The --server-components block re-reads args.option(b"--target") rather than using a local because at that point only the api::Target enum is bound (if let Some(target) = opts.target), not the raw byte slice; .unwrap_or_default() yields an empty string if --target wasn't passed, but the non-server-side branch is only reachable via browser/node which require the flag, so this is defensive rather than load-bearing.

Other factors

  • The new error message follows the exact pattern of the existing --format error (bun::fmt::quote + list of accepted values), which is already tested in the same describe block.
  • The new tests use tempDir, await using, drain stdout/stderr/exited concurrently, and assert content before exit code — matching harness conventions and the sibling --format test.
  • The bug-hunting system found no issues.
  • No prior review comments to address; only bot noise on the timeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant