Skip to content

shell(touch): report --date=, --reference= and --time= as unsupported options - #39227

Open
robobun wants to merge 2 commits into
mainfrom
farm/c369ba43/touch-long-opt-value-spelling
Open

shell(touch): report --date=, --reference= and --time= as unsupported options#39227
robobun wants to merge 2 commits into
mainfrom
farm/c369ba43/touch-long-opt-value-spelling

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • In the Bun shell, touch --date=x f, touch --reference=f g and touch --time=atime f fail with touch: illegal option -- date=x (reference=f, time=atime), as if the option did not exist. The bare spellings of the same options (touch --date x f) already fail with touch: unsupported option, please open a GitHub issue -- --date.
  • parse_long in src/runtime/shell/builtin/touch.rs compares the whole argument against --date, --reference and --time, so --date=x matches nothing, parse_long returns None, and the shared parser (parse_one_flag in src/runtime/shell/interpreter.rs) falls through to short option parsing of -date=x, whose first byte - is reported as an illegal option.
  • Exit code 1 is already right; only the stderr text is wrong. The same comparison was in the Zig version, so this is not a port regression. Found by reading the option table, not from a report.

Fix

Background

  • The shell builtins touch, mkdir, cat and cp parse their flags through the FlagParser trait in src/runtime/shell/interpreter.rs. For an argument starting with --, parse_one_flag first offers the whole argument to the builtin's parse_long; if that returns None it treats the bytes after the first - as a cluster of short flags, which is where the illegal option -- date=x text comes from.
  • A builtin returns ParseFlagResult::Unsupported(name) for an option it recognizes but does not implement; Builtin::fail_parse formats name (a literal chosen by the builtin, not the argv entry) into the unsupported option, please open a GitHub issue -- <name> line and exits 1. IllegalOption produces the illegal option -- <bytes> line, also exit 1.
  • touch is a shell builtin on every platform (only cat and cp defer to the system binary on POSIX), so the test needs no platform gating.

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.
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 6:52 PM PT - Aug 15th, 2026

@robobun, your commit b10a1b4c34d811d7cb7a65c120c0721852cbc3d5 passed in Build #98949! 🎉


🧪   To try this PR locally:

bunx bun-pr 39227

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

bun-39227 --bun

@coderabbitai

coderabbitai Bot commented Aug 15, 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: 22 minutes

Limit details: You’ve used all 5 included reviews currently available under your plan.

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: b2ebbe64-4014-4d48-90fe-15eec08e9fdc

📥 Commits

Reviewing files that changed from the base of the PR and between a42889a and b10a1b4.

📒 Files selected for processing (2)
  • src/runtime/shell/builtin/touch.rs
  • test/js/bun/shell/commands/touch.test.ts

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

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced and fixed.

  • Reproduced on the released bun 1.4.0 with test/js/bun/shell/commands/touch.test.ts: the --date=@0, --date=, --reference=other and --time=atime cases fail with touch: illegal option -- date=@0 (etc.) instead of the unsupported option message.
  • With this branch's debug build all 16 cases in that file pass, as do test/js/bun/shell/exec.test.ts and the exit codes block of test/js/bun/shell/bunshell.test.ts.
  • Stacked on shell(touch): name --time in its unsupported option message #39219 (first commit); the second commit is this PR's change. The PR body's Scope bullet covers why the split lives in touch.rs for now rather than in the shared parser.

@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, well-scoped error-message fix with a table test covering every arm.

What was reviewed:

  • parse_long now splits on = before matching value-taking options; split_once_char returns None when no = is present so bare --date still matches.
  • --no-create is checked before the split, so --no-create=1 still falls through to illegal option — pinned by the new test.
  • Test follows sibling test/js/bun/shell/commands/*.test.ts conventions (TestBuilder, describe.concurrent, ensureTempDir, exit-code + stderr assertions).
Extended reasoning...

Overview

This PR touches src/runtime/shell/builtin/touch.rs (the parse_long implementation of the FlagParser trait) and adds test/js/bun/shell/commands/touch.test.ts. The change makes touch --date=x, --reference=x and --time=x report as unsupported options (matching the existing behavior for the space-separated --date x spelling) instead of falling through to short-flag parsing and reporting illegal option -- date=x. It also carries forward the stacked #39219 fix where --time previously reported --reference=FILE due to a copy-paste. Only stderr text changes; exit code was already 1.

Security risks

None. This is pure error-message classification in a shell builtin's flag parser. No user input reaches a syscall differently than before — every path through parse_long still returns Unsupported or None, both of which cause the command to exit 1 before any filesystem operation. split_once_char is a bounds-safe in-tree helper over &[u8].

Level of scrutiny

Low. The diff is ~10 lines of Rust changing which literal string an already-failing command prints, plus a new test file. No control-flow, memory, or lifetime changes. The --no-create exact-match is deliberately kept before the split (it takes no value in GNU touch), and the test pins that --no-create=1 remains an illegal option. I confirmed bun_core::strings::split_once_char exists and returns Option<(&[u8], &[u8])>, so .map_or(flag, ...) correctly preserves the no-= case. I also confirmed parse_one_flag in interpreter.rs only calls parse_long for args with the -- prefix, so the split is always operating on a well-formed long option.

Other factors

The test file mirrors the structure of sibling files in test/js/bun/shell/commands/ (TestBuilder, .ensureTempDir(), .exitCode(), .doesNotExist(), describe.concurrent). The table covers every long and short option arm plus the =VALUE variants and the empty-value --date= edge, and a negative case for --no-create=1. The PR description explains the stacking on #39219 and verifies the test fails on released Bun and passes on the branch. No outstanding reviewer comments.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. shell(touch): name --time in its unsupported option message #39219 - Makes the identical fix to the same --time arm of parse_long in src/runtime/shell/builtin/touch.rs and adds the same new test/js/bun/shell/commands/touch.test.ts, making it a strict subset of this PR.

🤖 Generated with Claude Code

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Not a duplicate of #39219: this PR is stacked on it. The first commit here is #39219's commit (the --time arm's name), which is why that PR shows up as a subset. The second commit is the change this PR is for: --date=X, --reference=X and --time=X were reported as illegal options instead of as the unsupported --date / --reference / --time, which #39219 does not touch (it only changes the literal the bare --time arm reports). Once #39219 lands this rebases down to the second commit.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Heads up: the --no-create=1 test here pins touch: illegal option -- no-create=1. That text comes from the off-by-one in parse_short (it slices the cluster at 1 + i, one past the rejected byte), which #39230 fixes; after that change an unknown --long option is reported as touch: illegal option -- -, like rm/ls/mv already do, so whichever of the two lands second will fail this assertion. Matching on the prefix (/^touch: illegal option -- /) plus the exit code and doesNotExist("file") would keep this test valid either way.

@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Heads up: #39223 has been reworked to handle --name=value in the shared parse_one_flag (interpreter.rs), so touch --date=x, --reference=f and --time=atime get the unsupported message there without any change to touch's parse_long. If that lands, the touch.rs part of this PR becomes unnecessary; the table test here is still useful. One difference to be aware of: with the dispatcher-level change --no-create=1 is reported as unsupported --no-create too (the option is unimplemented in any spelling), so the --no-create=1 is still illegal case here would disagree with it.

@robobun
robobun force-pushed the farm/c369ba43/touch-long-opt-value-spelling branch from d3fd1ea to 9fa2108 Compare August 16, 2026 01:22
Comment thread src/runtime/shell/builtin/touch.rs Outdated
…pported options they spell

touch's long option table matched --date, --reference and --time only as
bare words. The --option=VALUE spelling of the same options fell through
to short option parsing and was reported as an illegal option
("illegal option -- date=x"), as if the option did not exist. Split the
argument at its first '=' and match the name, so both spellings are
reported as the same unsupported option. --no-create takes no value, so
it only matches without one and --no-create=x stays an illegal option.
@robobun
robobun force-pushed the farm/c369ba43/touch-long-opt-value-spelling branch from 9fa2108 to b10a1b4 Compare August 16, 2026 01:26

@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, correct error-message fix with a table test that pins each case.

What was reviewed:

  • parse_long now splits on the first = via bun_core::strings::split_once_char and matches the name; --no-create's value.is_none() guard keeps --no-create=1 falling through to illegal — matches GNU touch.
  • The stacked --time arm fix (was reporting --reference=FILE) is included.
  • Test uses the shell TestBuilder harness with ensureTempDir/doesNotExist/exit-code checks; .stderr(callback) is a supported overload; the --no-create=1 case pins only the illegal option -- prefix so it stays green regardless of #39230's ordering.
Extended reasoning...

Overview

Two files: src/runtime/shell/builtin/touch.rs (the parse_long body only) and a new test/js/bun/shell/commands/touch.test.ts. The Rust change splits the incoming long-option argument at its first = into (name, value) and matches on name, so --date=x, --reference=x and --time=x are recognized as the same unsupported options as their bare spellings instead of falling through to the short-flag path and being reported as illegal. The --time arm's copy-paste literal (--reference=FILE) is fixed to --time (the stacked #39219 commit). --no-create gains a value.is_none() match guard so --no-create=VALUE continues to fall through as illegal, matching GNU touch.

Security risks

None. This affects only which of two error messages is printed on an already-failing path (exit code was already 1). No new syscalls, no memory management, no user-controlled data reaching allocation or filesystem paths.

Level of scrutiny

Low. The change is a ~15-line rewrite of a static match table in an error-message path of a shell builtin. I verified bun_core::strings::split_once_char returns (before, after) at the first delimiter or None, so --date (no =) still matches, --date= yields ("--date", "") and matches, and --no-create=1 yields Some so the guard rejects it and it falls through to _ => None → short-flag illegal. No other callers of parse_long are affected (this is touch's own FlagParser impl).

Other factors

The new test file follows the existing test/js/bun/shell/commands/*.test.ts conventions (createTestBuilder, ensureTempDir, .quiet(), .exitCode(1), .doesNotExist("file")), covers all 15 known-but-unsupported spellings plus the negative --no-create=1 case, and asserts exact stderr for each. The .stderr(callback) overload used for the illegal-option case exists in test_builder.ts. The comment-cop bot's feedback about a paragraph-long comment was addressed in b10a1b4 — the current code has a one-line comment and expresses the takes-no-value rule as a match guard. The remaining timeline notes are cross-PR merge-order coordination (#39223, #39230), not correctness issues with this diff, and the test was already loosened to survive #39230. No CODEOWNERS entry for shell paths.

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