shell: accept -- end-of-options delimiter in builtins - #33995
Conversation
POSIX Utility Syntax Guideline 10 says the first `--` argument ends
option parsing and everything after it is an operand, even when it
starts with `-`. Bun Shell's builtins rejected it as an illegal
option, so `rm -- "$f"` (the standard shellcheck-recommended
hardening) failed unconditionally:
rm -- a -> rm: illegal option -- - (file not removed)
mv -- a b -> mv: illegal option -- - (nothing moved)
mkdir -- d -> mkdir: illegal option --
touch -- t -> touch: illegal option --
ls -- -> ls: illegal option -- -
seq -- 2 -> seq: invalid argument
Add a `--` arm to the shared flag parser (touch/mkdir/cat/cp) and to
each builtin with its own parser (rm/mv/ls/seq/cd/basename/dirname/
which/yes) so operands after `--` are passed through untouched.
|
Updated 10:04 PM PT - Jul 11th, 2026
❌ @robobun, your commit 695a6ed has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33995That installs a local version of the PR into your bun-33995 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 29 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 (1)
WalkthroughChanges
Double-dash option delimiter support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 3
🤖 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 `@src/runtime/shell/builtin/basename.rs`:
- Around line 25-30: Extract the duplicated leading “--” end-of-options
validation from the basename flow and the equivalent checks in dirname, yes,
which, and cd into a shared helper on Builtin. Update each builtin to call that
helper while preserving its existing invalid-usage failure behavior and
argument-start semantics.
In `@src/runtime/shell/builtin/cd.rs`:
- Around line 26-37: Update the argument handling in the cd builtin around the
skip calculation and first_arg processing so the previous-directory shortcut is
only used when the single argument is “-” without a preceding “--”. Preserve
“--” as an option terminator, allowing cd -- - to resolve a directory literally
named “-”.
In `@test/js/bun/shell/commands/double-dash.test.ts`:
- Around line 139-176: Extend the affected `cd`, `basename`, `dirname`, and
`which` test blocks to cover both ordinary and dash-prefixed operands, including
cases such as `cd -- -dir`, `basename -- -name`, `dirname -- -name`, and `which
-- -name`; create local fixtures where required and assert the expected output
and exit status for each variant.
🪄 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: 7b1f6eac-0b39-4d86-b7f5-bfcc19675f8d
📒 Files selected for processing (11)
src/runtime/shell/builtin/basename.rssrc/runtime/shell/builtin/cd.rssrc/runtime/shell/builtin/dirname.rssrc/runtime/shell/builtin/ls.rssrc/runtime/shell/builtin/mv.rssrc/runtime/shell/builtin/rm.rssrc/runtime/shell/builtin/seq.rssrc/runtime/shell/builtin/which.rssrc/runtime/shell/builtin/yes.rssrc/runtime/shell/interpreter.rstest/js/bun/shell/commands/double-dash.test.ts
Also cover dash-prefixed operands for cd/basename/dirname/which.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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/js/bun/shell/commands/double-dash.test.ts`:
- Around line 218-229: Strengthen the export tests in the describe("export")
block so they distinguish the fixed behavior from the old implementation: after
exporting FOO=bar, invoke export -- and assert the output includes the known
FOO=bar assignment while still excluding --=. Update the existing command
assertions without changing unrelated coverage.
🪄 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: 60756978-bdb0-4791-8a09-871fb1d4b471
📒 Files selected for processing (10)
src/runtime/shell/Builtin.rssrc/runtime/shell/builtin/basename.rssrc/runtime/shell/builtin/cd.rssrc/runtime/shell/builtin/dirname.rssrc/runtime/shell/builtin/exit.rssrc/runtime/shell/builtin/export.rssrc/runtime/shell/builtin/pwd.rssrc/runtime/shell/builtin/which.rssrc/runtime/shell/builtin/yes.rstest/js/bun/shell/commands/double-dash.test.ts
There was a problem hiding this comment.
No bugs found, but this touches argument parsing across 15 shell-builtin files including rm and mv, with a control-flow refactor in mv::parse_opts — worth a human glance.
Checked: operand_start() returns 0/1 correctly and each of the 8 no-parser builtins iterates start..argc with the right empty-operand fallback (usage vs. print-all vs. no-op).
Checked: the mv.rs loop refactor (Done => break + post-loop validation) preserves pre-PR behavior for mv a b, mv -f a b, and all-flags-no-operands.
Checked: rm's args_start = idx + is_end_of_options and the new >= argc usage guard; rm -f -- a and rm -- route correctly.
Checked: seq's -- arm doesn't break negative-number operands (seq -1 3 still parses -1 as positional).
Extended reasoning...
Overview
Adds POSIX -- end-of-options delimiter support to Bun Shell builtins, fixing #23851. Introduces Builtin::operand_start() (returns 1 if argv[0] is --, else 0) and routes 8 no-flag-parser builtins (basename, cd, dirname, exit, export, pwd, which, yes) through it. Adds a flag == b"--" arm to the shared parse_flags in interpreter.rs (covers touch/mkdir/cat/cp) and to each builtin with its own parser (ls, mv, rm, seq). Ships a 228-line test file with 36 cases covering plain operands, dash-prefixed operands, and bare -- for every affected builtin.
Security risks
None identified. This is option-parsing for in-process shell builtins; no auth, crypto, or network surface. The change actually improves safety by enabling the standard rm -- "$f" hardening idiom. I checked that the rm change cannot cause -- itself to be treated as a delete target, and that the root-path guard (args_start..argc loop) still runs over the correct operand range.
Level of scrutiny
Medium. Each individual change is small and follows one of two consistent patterns, and the test coverage is thorough. But this is 14 Rust files in the shell runtime, two of which (rm, mv) mutate the filesystem — a parsing regression there could delete or move the wrong files. The mv::parse_opts change is a genuine control-flow refactor (moving operand-count validation from inside the Done match arm to after the loop), not a pure addition. I traced it and it's behavior-preserving, but that's the kind of change a maintainer should eyeball.
Other factors
All prior review feedback is addressed and resolved: CodeRabbit's dedup suggestion (→ operand_start()), dash-prefixed-operand test coverage, the export -- test-strength fix, and my earlier nit about pwd/exit/export. The cd -- - → OLDPWD behavior was correctly kept (matches bash). echo is intentionally excluded per POSIX. CI build #72031 is in progress.
|
CI build #72031: the new
Ready for review. |
|
Heads-up for the rebase: #39225 moves rm's no-operands decision to the start of the |
1 participant
What
Bun Shell's file builtins reject the POSIX
--end-of-options delimiter.rm -- "$f"is the standard hardening idiom (shellcheck recommends it for every operand that might start with-), and under Bun Shell it fails unconditionally:Same for
mv -- a b,mkdir -- d,touch -- t,ls --,seq -- 2: all exit 1 withillegal optionand do nothing. Inside&&/||chains the exit 1 silently takes the wrong branch.Cause
Each builtin's flag loop treats any argument starting with
-as an option cluster with no terminator arm, so--falls through to the unknown-option error.cat/cpappeared to accept it only because they fall through to the system binary on POSIX.Fix
Add a
flag == b"--"arm to the shared flag parser ininterpreter.rs(coverstouch,mkdir,cat,cp) and to each builtin that has its own parser (rm,mv,ls,seq). Builtins with no flag parser (cd,basename,dirname,which,yes,pwd,exit,export) skip a leading--via a newBuiltin::operand_start()helper.--is consumed and everything after it is treated as an operand, matching POSIX Utility Syntax Guideline 10 and coreutils/bash.echois intentionally left alone (POSIX saysechodoes not recognise--).Verification
test/js/bun/shell/commands/double-dash.test.ts(36 cases) covers each builtin with plain operands, dash-prefixed operands, and bare--. All fail on the released build, all pass with this change.Fixes #23851
[review] gate passed · iteration 0 · 15 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The option-parsing loops in Bun Shell's builtins treated any argument starting with
-as a flag with no special case for--, so the POSIX end-of-options delimiter was rejected as an illegal option and the command aborted before touching its operands. The fix adds a--arm to each affected builtin's flag loop that stops option parsing and treats all remaining arguments as operands, matching the behavior already present in cat, cd, and which. This lets the standard hardening idiomcmd -- "$f"work and allows operands that begin with-to be passed safely.