Conversation
The --time arm of touch's long option table reported the option as --reference=FILE, copied from the arm above it, so `touch --time` pointed users at an option they had not passed. Report it as --time, like the other arms report the option that was given. Adds a table test pinning the message for every unsupported touch option.
|
Updated 4:07 PM PT - Aug 15th, 2026
✅ @robobun, your commit 74c75d53d49677998494ea85714857e85989a67e passed in 🧪 To try this PR locally: bunx bun-pr 39219That installs a local version of the PR into your bun-39219 --bun |
|
Status: reproduced on bun 1.4.0 ( |
|
Warning Review limit reached
Next review available in: 32 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-literal copy-paste fix in the --time match arm.
What was reviewed:
- Confirmed
fail_parse→fmt_error_arenaproduces exactlytouch: unsupported option, please open a GitHub issue -- <opt>\n, matching the test's stderr assertion. - New test file follows sibling conventions in
test/js/bun/shell/commands/(TestBuilder,$.nothrow(),describe.concurrent);.ensureTempDir()and.doesNotExist()exist in test_builder.ts. - Checked the other
parse_long/parse_shortarms for the same copy-paste pattern — none share the bug; the test now pins all 11.
Extended reasoning...
Overview
The PR changes a single string literal in src/runtime/shell/builtin/touch.rs: the b"--time" match arm of parse_long was passing b"--reference=FILE" to unsupported_flag (an obvious copy-paste from the arm directly above), so touch --time x reported the wrong option name. The fix changes it to b"--time". A new test file test/js/bun/shell/commands/touch.test.ts adds a table-driven test pinning stdout/stderr/exit-code and .doesNotExist("file") for all 11 unsupported touch options.
Security risks
None. This changes only a diagnostic string literal in an error path; no parsing, path handling, or syscall behavior is affected.
Level of scrutiny
Low. The Rust change is a one-token literal correction whose intent is self-evident from the surrounding match arms. I verified the exact error format against Builtin::fail_parse / fmt_error_arena ({kind}: unsupported option, please open a GitHub issue -- {opt}\n) — the test's expected stderr matches byte-for-byte. The test file mirrors sibling files (seq.test.ts, rm.test.ts, cp.test.ts) in structure: createTestBuilder, $.nothrow(), .ensureTempDir(), .runAsTest().
Other factors
The test covers the whole class (all 4 long + 7 short unsupported flags), not just the one repro, and asserts the operand file is not created — meeting the "cover the variant matrix" and "strongest invariant" review guidance. The PR description confirms the test fails on released bun 1.4.0 and passes on this branch. touch is registered unconditionally in the builtin kind table (Builtin.rs:191), so no platform gating is needed. No prior reviews or outstanding comments on the PR.
This branch has not been deployed
1 participant
Problem
touch --time xin the Bun shell fails withtouch: unsupported option, please open a GitHub issue -- --reference=FILE, naming an option the user did not pass.--timearm ofparse_longinsrc/runtime/shell/builtin/touch.rs:360passes the--reference=FILEliteral copied from the--referencearm above it. The same line was in the Zig version, so this is not a port regression.Fix
--timearm now reports--time.--no-create,--date,-a, ...), and--date, the arm most like--time(both take a value in GNU touch), is reported bare, so--timefollows it.--referencestill reports--reference=FILE: it names the option that was passed, so it is left alone here; shell(touch): report --date=, --reference= and --time= as unsupported options #39227, which is stacked on this PR and handles the--option=VALUEspellings, is where that suffix would be reconsidered.mkdir -m(trailing space) in shell: report --name=value spellings of unsupported builtin options as unsupported #39223,cp -p(reported as-P) in shell(cp): accept-r/--recursive/--verboseand fix clustered short flags #35616 / shell(cp): parse every short flag in a -Rv/-vR cluster #35705, and theillegal optionpayload of the_arms of cat/touch/mkdir/cp in shell: name the rejected flag in the builtins' illegal option errors #39230. Deriving the reported name from argv in the sharedFlagParser, which would retire all of these literals at once, touches the functions those PRs are editing, so it is a follow-up for after they land; it would delete this line along with the others and keep this PR's test.test/js/bun/shell/commands/touch.test.ts, a table pinning stdout, stderr and exit code for all 11 unsupported touch options (4 long, 7 short) and checking the operand is not created. On the releasedbun(1.4.0) only the--timecase fails; on this branch all 11 pass. shell(mkdir, touch): report operands longer than the path buffers instead of aborting #38379 and shell: fail an empty operand with ENOENT instead of acting on the cwd #38002 also create this file (different tests); whichever lands later combines the describe blocks.test/js/bun/shell/exec.test.ts(--helptable) and theexit codesblock oftest/js/bun/shell/bunshell.test.tsagainst the debug build; both still pass.Background
touch,mkdir,catandcpparse their flags through the sharedFlagParsertrait insrc/runtime/shell/interpreter.rs. A builtin returnsParseFlagResult::Unsupported(name)for an option it recognizes but does not implement, andBuiltin::fail_parseformatsnameinto theunsupported option, please open a GitHub issue -- <name>line, so the text after--is whatever literal the builtin chose to pass, not the argv entry.touchis a shell builtin on every platform (unlikecatandcp, which defer to the system binary on POSIX), so the test needs no platform gating.