shell: report --name=value spellings of unsupported builtin options as unsupported

[robobun]

Problem

• The shell builtins that share parse_flags (mkdir, touch, cat, cp) reject the options they do not implement with unsupported option, please open a GitHub issue -- <option>, but only when the option is spelled without a value. mkdir --mode=755 d prints mkdir: illegal option -- mode=755, and touch --date=x f, --reference=f, --time=atime likewise print illegal option, as if the option did not exist; mkdir --mode 755 d and touch --date x f already print the unsupported message.
• Cause: parse_one_flag (src/runtime/shell/interpreter.rs:2384) offers the whole --name=value token to the builtin's parse_long, which compares it against its option names, matches nothing, and returns None; the dispatcher then reparses the token as the short cluster -name=value, whose first byte is rejected as an illegal option. Every builtin with a value-taking unsupported option (mkdir --mode, touch --date/--reference/--time) has the gap, because the = is never handled anywhere.
• Separately, mkdir -m 755 d prints -- -m with a trailing space: the literal in mkdir's parse_short is b"-m " (src/runtime/shell/builtin/mkdir.rs:448). Every other unsupported_flag literal in the builtins is clean.
• Exit code is 1 in all of these cases before and after; only the stderr text changes. Both bugs were carried over from the Zig implementation, so this is not a regression.

Fix

• parse_one_flag splits a -- token at its first = and offers the name to parse_long. A rejection (Unsupported, IllegalOption) is returned whether or not a value was given; an acceptance is returned only when no value was given. With a value, an accepted name falls through to the short-cluster path and is rejected exactly as today (mkdir --parents=1 stays an illegal option), and so does an unknown name (--bogus=1). The fixing lines are the split_once_char and the if !has_value guard; the other builtins' parse_long implementations are unchanged.
• Why this is right: --name=value and --name value are the two spellings getopt_long accepts for the same option, so both must classify the same way, and the builtin's answer about the name ("I do not implement this") holds whatever follows the =. Handling the = in the dispatcher fixes all four options at once and means a builtin only ever matches option names; no long option any of these builtins accepts takes a value, so the dispatcher can also keep rejecting values on accepted options without asking the builtin. One consequence is deliberate: an unsupported option is reported as unsupported even in a spelling the real tool would reject (touch --no-create=1 now says unsupported --no-create, previously illegal), since the option is unimplemented either way; this is pinned in the test.
• mkdir's -m literal loses its trailing space.
• Unchanged, and checked by hand with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1: cat and cp have no long options, so every --x and --x=y they get still falls through byte for byte as before; mkdir --help/touch --help (pinned in exec.test.ts) and --=x are unaffected.
• Verified by the new options a builtin rejects block in test/js/bun/shell/bunshell.test.ts: 14 cases covering -m 755, -m755, -pm 755, --mode 755, --mode=755, --mode=, -p --mode=755, touch --date x, --date=x, --no-create=1, plus --modes, --bogus=1 and --parents=1 staying illegal and --parents still working; each also checks nothing was created. 8 of the 14 fail on the unfixed build (the 7 whose message changes plus --no-create=1), all pass with the fix. The rest of bunshell.test.ts (437 tests) and exec.test.ts pass with the fix.
• The test lives in bunshell.test.ts rather than a new commands/mkdir.test.ts because it covers the shared parser, and because shell: fail an empty operand with ENOENT instead of acting on the cwd #38002, shell(mkdir, touch): report operands longer than the path buffers instead of aborting #38379 and shell(mkdir): accept --verbose instead of the misspelling --vebose #39221 each already add that file.
• Not changed here, on purpose: the names touch reports for --reference/--time (--reference=FILE; shell(touch): name --time in its unsupported option message #39219 fixes --time), cp reporting -p as -P (shell(cp): accept -r/--recursive/--verbose and fix clustered short flags #35616/shell(cp): parse every short flag in a -Rv/-vR cluster #35705), and which byte the illegal-option message names (shell: name the rejected flag in the builtins' illegal option errors #39230). shell(touch): report --date=, --reference= and --time= as unsupported options #39227 fixes the touch spellings inside touch's own parse_long; with this change that becomes unnecessary, noted there.

Background

• Option parsing for these builtins: parse_flags in interpreter.rs walks argv until the first non-option and calls parse_one_flag per token, which offers --x tokens to the builtin's FlagParser::parse_long and otherwise iterates the token's bytes through parse_short. A None from parse_long means "not one of my long options" and falls into the byte loop, where the token's second - is what gets rejected; that fall-through is what produced the illegal option text here and is what this change still relies on for the cases it leaves unchanged.
• ParseFlagResult::Unsupported carries a static option name chosen by the builtin, which Builtin::fail_parse prints after unsupported option, please open a GitHub issue -- ; IllegalOption produces illegal option -- <bytes>. Both exit 1, so the only user-visible difference between them is whether the message says the option is known.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/shell/bunshell.test.ts

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 1 minute

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: e0ad9f06-2f5e-4696-87fe-1b39abddc415

📥 Commits

Reviewing files that changed from the base of the PR and between 619a88d and bfdcc92.

📒 Files selected for processing (3)

• src/runtime/shell/builtin/mkdir.rs
• src/runtime/shell/interpreter.rs
• test/js/bun/shell/bunshell.test.ts

---

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

[robobun]

Reproduced on bun 1.4.0 and on main: mkdir -m 755 d prints unsupported option ... -- -m (trailing space), and mkdir --mode=755 d / touch --date=x f print illegal option -- mode=755 / date=x while the --mode 755 / --date x spellings print the unsupported message.

Reworked after self-review: the =value handling now lives in the shared parse_one_flag in interpreter.rs (one fix for mkdir and touch) instead of mkdir's own parse_long; mkdir.rs only loses the trailing space.

Test: the options a builtin rejects block in test/js/bun/shell/bunshell.test.ts (8 of its 14 cases fail on the unfixed build, all pass with the fix). The rest of bunshell.test.ts and exec.test.ts pass.

CI (build 98922, finished): 178 of 179 jobs passed. The one red job is debian 13 x64-asan, failing on test/js/web/fetch/blob.test.ts (a file-backed Response slice streaming the whole file), which is already failing on main and unrelated to this change; it has been reported separately. Not retriggering, since that failure would recur. Ready for review.

[claude]

LGTM — two byte-string literal fixes in an error-only path, with a thorough new test file.

What was reviewed:

• starts_with(b"--mode=") is precise: --mode=755 and --mode= now hit Unsupported, while --modes still falls through to IllegalOption via parse_one_flag.
• b"-m" now matches every other unsupported_flag literal in cat/touch/cp; no other builtin has a trailing space.
• Test file follows sibling commands/*.test.ts conventions (module-level $.nothrow(), tempDir, test.each); array interpolation ${words} spreads as separate argv words (verified against bunshell.test.ts:117); each concurrent case gets its own tempdir and asserts nothing was created.

Extended reasoning...

Overview

Two-line change in src/runtime/shell/builtin/mkdir.rs fixing error-message text for the unimplemented -m/--mode option: adding || flag.starts_with(b"--mode=") to parse_long so --mode=MODE is reported as unsupported instead of falling through to short-flag parsing as an illegal option, and dropping a stray trailing space from the b"-m " literal in parse_short. A new test/js/bun/shell/commands/mkdir.test.ts covers seven spellings (-m 755, -m755, -pm 755, --mode 755, --mode=755, --mode=, -p --mode=755).

Security risks

None. This is error-message-only code on a rejected-option path; exit codes are unchanged (still 1) and no filesystem operation runs.

Level of scrutiny

Low. The change touches two static byte-string literals in a builtin's flag parser. I traced parse_one_flag in interpreter.rs to confirm the described fall-through mechanism (long-flag None → reparse as short cluster → _ arm → IllegalOption with mode=755), and confirmed the starts_with predicate cannot over-match (--modes fails both the equality and the prefix check because it lacks the =). The -m literal now matches all 20+ other unsupported_flag call sites in cat/touch/cp/mkdir, none of which have trailing whitespace.

Other factors

The test file follows existing test/js/bun/shell/commands/ conventions: module-level $.nothrow() (used by 10 sibling files), using tempDir(...) per case so describe.concurrent is safe, test.each for the variant matrix, and asserts the full {stdout, stderr, exitCode} object plus readdirSync(cwd) to prove nothing was created. Array interpolation ${words} in the shell template spreads elements as separate argv words (same behavior exercised at bunshell.test.ts:117). The PR description explains why the parallel touch gap is intentionally out of scope (conflicts with in-flight #39219) and flags the file-add overlap with three other open PRs — a merge-order note for maintainers, not a correctness concern for this change.

[github-actions]

This PR may be a duplicate of:

1. shell(mkdir): accept --verbose instead of the misspelling --vebose #39221 - Patches the same Opts::parse_long match chain in src/runtime/shell/builtin/mkdir.rs to fix mkdir long-option handling, and adds the same brand-new test/js/bun/shell/commands/mkdir.test.ts with an identical test scaffold.

🤖 Generated with Claude Code

[robobun]

Not a duplicate of #39221. That PR fixes a different bug in the same parser: it changes the --vebose arm so --verbose is accepted, and its tests cover the verbose output. This PR changes the --mode arm and the -m arm (lines #39221 does not touch), and its tests cover the mode option's error message. The two mkdir.rs hunks merge cleanly; only the new test file needs its describe blocks combined when the second one lands, as noted in the description.

[robobun]

^{Updated 6:43 PM PT - Aug 15th, 2026}

❌ @robobun, your commit bfdcc92 has 1 failures in Build #98922 (All Failures):

• test/js/web/fetch/blob.test.ts - code 1 on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 39223

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

bun-39223 --bun

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/runtime/shell/interpreter.rs:2392-2401 — The 4-line comment here explains why it is OK to discard an accepting parse_long result and fall through to short-flag parsing to get the rejection — that is exactly the "paragraph-long comment justifying a workaround" CLAUDE.md rule #13 forbids (and comment-cop already flagged this line). Have this arm return ParseFlagResult::IllegalOption(std::ptr::from_ref(&flag[2..])) directly instead of {}; then the rejection is explicit, the side-effect-then-reject-elsewhere indirection is gone, and the comment can drop to one line or nothing. (The sibling comment-cop flag on line 2349 — the two-line trait doc-comment — looks like a false positive.)

  Extended reasoning...

  What the comment is justifying

  The Some((name, _)) arm at interpreter.rs:2396-2401 handles --name=value. When parse_long(name) returns Unsupported/IllegalOption, that result is returned — good. But when it returns ContinueParsing or Done (i.e. the builtin accepted the option), the result is discarded and control falls through to let small_flags = &flag[1..] so the whole token is re-parsed as a short-flag cluster and rejected there. The 4-line comment at 2392-2395 exists to explain why that indirection is acceptable: "No long option a builtin accepts takes a value, so … on an accepted option the token falls through and is rejected as an illegal option below."

  CLAUDE.md rule #13 is explicit: "If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code." REVIEW.md ("Only comment what the code cannot say. One line.") says the same. The repo's comment-cop bot has already flagged line 2395 with the bare rule text; this comment adds the concrete fix.

  Step-by-step: mkdir --parents=1 d

  1. flag = b"--parents=1"; split_once_char yields name = b"--parents".
  2. Opts::parse_long(b"--parents") runs, sets self.parents = true, and returns Some(ContinueParsing).
  3. Line 2400 matches Some(ContinueParsing) → {} (result discarded).
  4. Fall through to small_flags = b"-parents=1"; the first byte '-' hits the wildcard in parse_short → IllegalOption(b"parents=1").
  5. parse_flags returns Err, Mkdir::start calls Builtin::fail_parse, exits 1 with mkdir: illegal option -- parents=1.

  So the accepting side effect (parents = true) already ran, and rejection then happens via an unrelated mechanism (short-flag parsing tripping on a leading -). The test at bunshell.test.ts:3266 only asserts the message prefix (expect.stringMatching(/^mkdir: illegal option -- /)), which is consistent with relying on whatever the fall-through happens to produce rather than a deliberate message.

  Why it is not a functional bug

  Exit code and message are identical to pre-PR behavior (before, parse_long saw the whole b"--parents=1" token, matched nothing, and fell through the same way). The mutated opts are discarded because parse_flags returns Err before any filesystem work — the new test proves created: []. So nothing user-observable regresses; this is a code-structure issue.

  Fix

  Make the arm reject explicitly instead of falling through:

  Some((name, _)) => match opts.parse_long(name) {
      Some(r @ (ParseFlagResult::Unsupported(_) | ParseFlagResult::IllegalOption(_))) => return r,
      Some(ParseFlagResult::ContinueParsing | ParseFlagResult::Done) | None => {
          return ParseFlagResult::IllegalOption(std::ptr::from_ref(&flag[2..]));
      }
  },

  Now --parents=1 is rejected right here (message becomes illegal option -- parents=1, same as today), the code says what it does, and the comment collapses to nothing (or one line: "no accepted long option takes a value"). The side-effect-before-rejection still technically occurs, but it is no longer being papered over by routing through an unrelated parser.

  Line 2349

  The comment-cop bot also flagged the updated FlagParser::parse_long doc-comment at line 2349. That is a normal 2-line trait doc string ("Handle a --long flag, given without any =value. Return None to fall through to short parsing."), not a workaround justification — it looks like a false positive and this finding does not cover it.

[robobun]

On the suggestion to return IllegalOption directly from the value-present arm instead of falling through (the review looked at f3faba1; bfdcc92 already collapsed this to one parse_long call with a match guard and a one-line comment):

The fall-through is kept on purpose. Every other --x token the builtin does not accept (--bogus, --bogus=1, --help) is rejected by the same short-cluster path, and the byte that path names is exactly what #39230 is changing (to -, as BSD getopt prints it). Returning &flag[2..] from this one arm would make --parents=1 print parents=1 while --bogus=1 prints whatever the cluster path prints, in either landing order; routing it through the common path keeps all rejected long options consistent with each other before and after #39230, and keeps this PR's behavior change limited to the unsupported message. The test pins that case with a prefix match for the same reason.

The parse_long side effect on the rejected token is the same under both shapes, and it is discarded: every caller returns from fail_parse on Err without reading the opts (cat.rs:57, cp.rs:80, mkdir.rs:61, touch.rs:55).

[claude]

LGTM — the description now matches the diff (my earlier note is addressed), and the shared parse_one_flag change traces correctly through all four FlagParser impls.

What was reviewed:

• Traced the new split_once_char + match-guard path against every parse_long (mkdir/touch/cat/cp): cat/cp return None so are byte-for-byte unchanged; touch only returns Unsupported so --date=x etc. now hit the right arm.
• Checked the --parents=1 case: parse_long sets opts.parents before the fall-through, but parse_flags then returns Err and mkdir goes to fail_parse without touching the filesystem — the test's created: [] pins this.
• Confirmed bun_core::strings::split_once_char is the right helper per the byte-search lint rules, and file-level $.nothrow() means the failing-command tests read exitCode rather than throwing.

Extended reasoning...

Overview

Two Rust changes and one test block. src/runtime/shell/interpreter.rs teaches the shared parse_one_flag dispatcher to split --name=value at the first = before offering the name to the builtin's parse_long, so that mkdir --mode=755 and touch --date=x classify the same as their space-separated spellings ("unsupported option" instead of "illegal option"). src/runtime/shell/builtin/mkdir.rs drops a stray trailing space from the -m unsupported-option literal. test/js/bun/shell/bunshell.test.ts gains a 14-case describe.concurrent block asserting exact stderr, exit code 1, and that nothing was created on disk.

Security risks

None. This only changes which of two error-message templates is printed for rejected shell-builtin options; exit code is 1 before and after, and no new filesystem or exec path is reached.

Level of scrutiny

Low-to-medium. The dispatcher is shared across four builtins, so I read every FlagParser::parse_long implementation to confirm the new match arms cover their return shapes: cat and cp always return None (unchanged fall-through), touch only ever returns Unsupported(_) (now returned regardless of =value), and mkdir returns Unsupported for --mode and ContinueParsing for --parents/--vebose. The one subtlety — --parents=1 mutates opts.parents inside parse_long before the guard sends it to the illegal-option fall-through — is harmless because parse_flags then returns Err and mkdir's start goes straight to fail_parse without scheduling any ShellMkdirTask; the test asserts created: [] for exactly this input. No parse_long returns Done, so the if !has_value guard's only live payload is ContinueParsing.

Other factors

My previous inline note (description pointed at a non-existent commands/mkdir.test.ts) has been addressed — the description and robobun follow-up now correctly reference bunshell.test.ts and the 14-case block. The three comment-cop warnings were resolved in bfdcc92 (the in-code comment is now one line stating the trait contract). split_once_char is the mandated bun_core::strings helper, satisfying the byte-search lint. Tests use tempDir + using, assert a combined {stdout, stderr, exitCode, created} object, and rely on the file-level $.nothrow() so failing commands return rather than throw.