shell: accept seq --separator=X and --terminator=X - #39265
Conversation
The seq builtin only matched the long options spelled as a separate argument (--separator X), so the --separator=X spelling fell through to operand parsing and failed with "seq: invalid argument". Take the value after the = in the same arm that handles the attached -sX / -tX forms.
|
Warning Review limit reached
Next review available in: 4 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. 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 |
|
Reproduced on bun 1.4.0 (Linux): Fix is in #39265 (this PR): the attached-value arms in |
There was a problem hiding this comment.
LGTM — small, mechanical option-parsing extension with thorough tests.
What was reviewed:
- Traced the new
strip_prefix(b"-s").or_else(...)arms: bare-s/--separatorare still consumed by the exact-match arm above, so dropping thelen > 2guard is safe;--separator,(no=) still falls through to the numeric parser and is rejected. - Checked that
bytesremains a sub-slice of the same argv entry, so theRawSliceborrow invariant documented on the struct fields is unchanged. - Verified the new tests cover the happy path, empty value (
--separator=),=inside the value, both options together, and pin the rejection of--separator,/--terminator,.
Extended reasoning...
Overview
This PR teaches the Bun shell seq builtin to accept --separator=X and --terminator=X (the --name=value spelling of a getopt_long option with a required argument), which previously fell through the option loop and failed with "invalid argument". The change touches two arms of the flag-parsing loop in src/runtime/shell/builtin/seq.rs (rewriting starts_with(b"-s") && len > 2 as strip_prefix(b"-s").or_else(|| strip_prefix(b"--separator=")), and the same for -t) and adds eight test cases to test/js/bun/shell/commands/seq.test.ts.
Security risks
None. This is option parsing for a shell builtin that emits a numeric sequence to stdout. The value is stored as a borrowed slice of argv (same as the existing -sX path) and only ever copied into the output buffer via extend_from_slice. No paths, no syscalls, no allocation sizing derived from the value.
Level of scrutiny
Low. The diff is ~10 lines of Rust that mechanically extends an existing pattern. I verified the one non-obvious point the PR description calls out: dropping the explicit len > 2 guard is safe because the exact-match arm arg == b"-s" || arg == b"--separator" immediately above still catches the bare forms, so strip_prefix(b"-s") never sees an empty remainder. --separator= (empty value) intentionally reaches the new arm and sets an empty separator, which is tested. --separator, does not match either prefix (second byte is -, not s; and no = at position 11), so it still falls through to parse_num! and fails — also pinned by a test.
Other factors
- Tests are added alongside the existing
-s/--separatorcases in the correct file, assert exact stdout/stderr/exit, and cover the variant matrix (both options, empty value,=in value, combined use, negative pins). - The
RawSlicestored inseparator/terminatoris a sub-slice of the same argv arena string as before, so the lifetime invariant documented on the struct is preserved. - No prior human or bot reviews with outstanding feedback; the bug-hunting system found nothing.
Problem
seq --separator=, 1 3andseq --terminator=X 1 3exit 1 withseq: invalid argument. The other spellings of the same options (--separator ,,-s ,,-s,,--terminator X) work.src/runtime/shell/builtin/seq.rsonly recognizes the long names by exact comparison (arg == b"--separator",arg == b"--terminator"). A--separator=,argument matches no arm, the loop breaks, and the argument is then parsed as the first numeric operand, which fails.Fix
-s,) now also strips a--separator=prefix, and the-t,arm a--terminator=prefix. Everything after the first=is the value, so--separator=sets an empty separator and--separator=a=bsetsa=b.--name=valueand--name valueare the two spellings getopt_long accepts for a long option with a required argument, and bun's seq already accepts the second one for these options;.claude/docs/landing-prs.md(API design) asks for every accepted form of an option to be handled identically, naming exactly this pair. GNUseq --separator=, 1 3accepts the=spelling too (checked locally; GNU seq has no--terminator, that one is BSD's, so the-tchange only follows the-sone). The=is required:--separator,is still rejected, as getopt_long would reject it.arg.len() > 2guard the old arm had was already implied by the exact-sarm right above it, which still consumes a bare-s(and still reports the missing argument);strip_prefix(b"-s")therefore only ever sees a non-empty remainder, so-sXbehaves as before.-s,form, so theRawSliceborrow documented on theseparator/terminatorfields is unchanged.test/js/bun/shell/commands/seq.test.ts:--separator=,,--separator=(empty),--separator=a=b,--terminator=,,--terminator=, both options together, and--separator,/--terminator,staying rejected. Six of the eight fail on bun 1.4.0 (the two rejection pins pass before and after, they pin the shape of the fix); all 40 tests in the file pass with the fix.test/js/bun/shell/pipeline_stack.test.ts,exec.test.ts(seq --helpstill reports an invalid argument) andshell-seq-condexpr.test.tsalso pass with the fix.Background
seqparses its options by hand inSeq::startbecause-s/-ttake values; the sharedparse_flagshelper the other builtins use only handles boolean flags. shell: report --name=value spellings of unsupported builtin options as unsupported #39223, which teachesparse_flagsabout--name=value, therefore does not reach seq.Seq::separator/Seq::terminatorarebun_ptr::RawSlices: non-owning slices into the command's argv, which outlives the builtin. That is why the option loop stores sub-slices of the argument instead of copying.--end-of-options delimiter in builtins #33995) change other parts of the file (operand parsing, output, the-w/-farms,--); this change is confined to the two attached-value arms. shell: implement seq -w and report -f as unsupported #37926 adds an exact-match--formatarm, so once it lands--format=Xwill want the same=handling to report the option as unsupported; noted there.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/commands/seq.test.ts