cli: make several unhelpful error messages actionable#32470
Conversation
- 'bun install --linker=<bad>' now echoes the rejected value - 'bun patch' / 'bun patch --commit' / 'bun patch-commit' with no positional now show a one-line usage example and point at --help - 'bun build --format=<bad>' now echoes the rejected value - 'bun build --loader <bad>' and '--define <bad>' (no ':' / '=') now name which flag rejected the token, echo the token, and show an example of the expected shape - 'bun run --filter' now names which workspace package.json it failed to read instead of a bare 'Failed to read package.json'
|
Updated 4:35 PM PT - Jun 17th, 2026
❌ @robobun, your commit 77625c7 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32470That installs a local version of the PR into your bun-32470 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughImproves CLI error messages across ChangesCLI Error Message Improvements
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/filter-workspace.test.ts`:
- Around line 607-618: The test is not validating the command success or the
actual execution of the valid workspace package. Modify the test to capture the
exit code from the Bun.spawn call (currently proc.exited is awaited but ignored)
and add assertions that verify both stdout output and exit code. First check
that stdout contains expected output from the valid workspace package execution,
then assert that the exit code is 0 to confirm the command succeeded. This
ensures the test validates that the valid package actually ran successfully, not
just that error warnings were logged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2bb01511-9184-4955-bad0-d89c3313aebf
📒 Files selected for processing (8)
src/install/PackageManager/CommandLineArguments.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/colon_list_type.rssrc/runtime/cli/filter_run.rstest/bundler/cli.test.tstest/cli/install/bun-patch.test.tstest/cli/install/isolated-install.test.tstest/cli/run/filter-workspace.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
CI status: the diff itself is green. Build 63227 finished with 259 lanes passed; none of the 4 changed test files appear in any failure. The red is:
None of these touch |
There was a problem hiding this comment.
LGTM — error-message-only changes with regression tests; my earlier nit on the filter-workspace test was addressed in 77625c7.
Extended reasoning...
Overview
This PR rewords seven CLI error/warning messages across bun install, bun build, bun patch, and bun run --filter so they echo the offending value and/or show a usage example. Source changes touch four Rust files (CommandLineArguments.rs, Arguments.rs, colon_list_type.rs, filter_run.rs); the rest is eight new regression tests plus one tightened existing assertion across four test files. Every source edit is confined to an error/warning code path that already terminated the process — no happy-path control flow changes.
The only non-string-literal change is in colon_list_type.rs, where the missing-separator case now prints and Global::exit(1) directly inside load() instead of returning Err(InvalidSeparator) for resolve() to catch and do the same thing. I verified load() has exactly one caller (resolve(), line 86) and that the two ColonListValue implementors are precisely Loader (IS_LOADER = true, → --loader message) and &'static [u8] (→ --define message), so the branch is exhaustive and the observable behavior is unchanged apart from the message text.
Security risks
None. The changes only format user-supplied CLI argument strings into error output via the existing bun_core::fmt::quote / bstr::BStr helpers (same pattern as the neighboring --registry, --cpu, --os errors in the same file). No parsing, no filesystem, no auth, no network.
Level of scrutiny
Low. These are user-facing error-string improvements on already-failing code paths, each backed by a test that asserts the new message content and a non-zero exit code. The PR description documents that all 8 tests fail under USE_SYSTEM_BUN=1, confirming they pin the new behavior. None of the touched paths are covered by CODEOWNERS.
Other factors
- The bug-hunting system found no issues.
- My earlier inline nit (and CodeRabbit's identical one) about asserting
stdout/exitCodein the newfilter-workspacetest was addressed in commit 77625c7 and both threads are resolved. - The CI failures reported by robobun are infrastructure-only (musl build agents and "Failed to create agent" on Windows runners), not test failures related to this diff.
- The new error strings follow the surrounding house style (
Output::err_generic+fmt::quote,<d>/<cyan>/<r>markup,bun_core::note!for the help hint) used elsewhere in the same files.
1 participant
What
A handful of CLI error messages that gave the user nothing to act on now echo the offending value and/or show what a correct invocation looks like. Same spirit as #32465.
Before / After
bun install --linker=isoalted(typo)bun patch(no argument)bun patch --commit/bun patch-commit(no argument)bun build --format=commonjsbun build --loader text(missing:)bun build --define FOO(missing=)bun run --filter='*' scriptwith a workspace package whosepackage.jsonis unreadable/malformedWhy these matter
--linker/--format: every sibling flag in the same file already echoes the bad value; these two were the odd ones out. A user who typosisoaltedor writescommonjsinstead ofcjscurrently gets no pointer at what they typed.--loader/--define: the old message didn't even say which flag tripped. Both flags go through the same helper, sobun build --loader foo --define BARgave an identical message for either mistake.bun patch: "pkg" is jargon and there was no hint at what to pass.--filter: in a monorepo with many packages, "Failed to read package.json" doesn't tell you which one.Verification
New tests in each of the relevant existing files:
All 8 fail under
USE_SYSTEM_BUN=1(they assert on content the old messages never had).One existing assertion in
filter-workspace.test.tsthat matched the old generic warning text has been tightened to also assert the path is present.