Skip to content

shell: implement seq -w and report -f as unsupported - #37926

Open
robobun wants to merge 4 commits into
mainfrom
farm/bb54df59/shell-seq-fixed-width
Open

shell: implement seq -w and report -f as unsupported#37926
robobun wants to merge 4 commits into
mainfrom
farm/bb54df59/shell-seq-fixed-width

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • The seq builtin's usage string advertises -w and -f format (src/runtime/shell/Builtin.rs:193), but neither does anything useful.
  • -w / --fixed-width is consumed by the flag loop and then ignored (src/runtime/shell/builtin/seq.rs:83, the arm only did idx += 1), so seq -w 8 11 prints 8 9 10 11 where BSD and GNU seq print 08 09 10 11. The original Zig builtin stored a fixed_width bool it never read, so this has been the case since the builtin was added.
  • -f is not parsed at all, so it falls through to the operand parser and seq -f %g 1 3 fails with seq: invalid argument, which points at the wrong thing.

Fix

  • -w sets fixed_width; each operand's decimal places as written are tracked in decimals (decimal_places: 0.25 -> 2, 2.5e-1 -> 2, 1.5e1 -> 0); do_ prints every value with {current:0width$.decimals$}, where width is the wider of start and end rendered at that precision (FixedWidth::new). Output without -w is unchanged (seq 1 0.5 3 still prints 1 1.5 2 2.5 3).
  • Why this layout: every value of start + k*incr has at most as many decimals as the most precise operand, so rendering at that precision prints each value exactly and gives all of them the same fractional part; every value lies between start and end, and at a fixed precision a value never renders wider than the bound it lies inside of, so the wider bound is the width of the widest value. Rust's 0 flag pads after the sign, so seq -w -10 5 10 gives -10 -05 000 005 010, which is what GNU seq prints; precision from the operands as written and width from the bounds is also how both BSD and GNU seq define -w (the only visible difference: GNU keeps leading zeros typed in an operand, seq -w 008 11 pads to 3 there and to 2 here, as on BSD).
  • decimals is capped at 1074 when printing (MAX_FIXED_WIDTH_DECIMALS): no float has more fractional digits than that, and core::fmt panics on a precision or width above u16::MAX, which an operand like 1e-70000 (parses to 0, 70000 implied decimals) would otherwise reach.
  • -f, -fFORMAT and --format go through Builtin::fail_parse with ParseError::Unsupported, so they fail with seq: unsupported option, please open a GitHub issue -- -f. This is how cat, cp, touch and mkdir already treat the flags their usage strings list but they do not implement; a printf-style -f (%e/%g/%a with flags, width and precision) is a feature of its own and is not added here. The usage string is left as is, like theirs.
  • Verified with test/js/bun/shell/commands/seq.test.ts: 23 new cases (padding up and down, one operand, width taken from an unreached end bound, negative values, decimals from the increment, exponent spellings, -s/-t, both output paths (captured stdout and a > redirect), the precision cap, and the four -f spellings) fail on the released build and pass with this change (58/58).
  • test/js/bun/shell/pipeline_stack.test.ts, test/js/bun/shell/shell-seq-condexpr.test.ts and test/internal/source-lints/byte-search.test.ts still pass; cargo clippy -p bun_runtime and cargo fmt are clean.
  • Overlaps with open seq PRs: shell: make the seq builtin count in f64 at the operands' decimal precision #37923 adds the same decimals field and decimal_places helper (copied here as is, so whichever lands second only has to drop its duplicate; FixedWidth::new takes the unscaled operands, which that PR still has at that point), and shell: accept -- end-of-options delimiter in builtins #33995 adds a -- arm next to the new -f arm. Both are mechanical to resolve in either order.

Background

  • seq in Bun.$ is always the builtin; it follows the BSD variant (its usage string is BSD's, seq 5 1 counts down, -t exists, -s is emitted after every value). Without -w it prints each value in its shortest round-trip form, so fractional sequences have values of varying width (1, 1.5); -w in both BSD and GNU seq switches to a fixed number of decimals and zero-pads, which is the only way every line can have the same width.
  • Builtin::fail_parse / ParseError::Unsupported (src/runtime/shell/Builtin.rs, interpreter.rs) is the shared path that renders <cmd>: unsupported option, please open a GitHub issue -- <flag> and exits 1; seq has a hand-rolled flag loop (options take arguments and operands may be negative numbers), so it calls that path directly instead of going through parse_flags.
  • The builtin currently counts in f32; -w only changes how values are rendered, so it is independent of that (and of shell: make the seq builtin count in f64 at the operands' decimal precision #37923, which changes it).
Before / after
$ bun -e 'console.log(JSON.stringify(await Bun.$`seq -w 8 11`.text())); const r = await Bun.$`seq -f %g 1 3`.nothrow().quiet(); console.log(r.exitCode, r.stderr.toString())'

# bun 1.4.0
"8\n9\n10\n11\n"
1 seq: invalid argument

# this branch
"08\n09\n10\n11\n"
1 seq: unsupported option, please open a GitHub issue -- -f

More -w output from this branch, each identical to GNU coreutils 9.7 (seq 11 8 excepted, GNU prints nothing there):

seq -w 10             01 02 03 04 05 06 07 08 09 10
seq -w 11 8           11 10 09 08
seq -w 1 4 10         01 05 09
seq -w -1 1           -1 00 01
seq -w -10 5 10       -10 -05 000 005 010
seq -w 1 0.5 3        1.0 1.5 2.0 2.5 3.0
seq -w 0 0.25 1       0.00 0.25 0.50 0.75 1.00
seq -w -0.5 0.25 0.5  -0.50 -0.25 00.00 00.25 00.50
seq -w 1 -0.5 -1      01.0 00.5 00.0 -0.5 -1.0
seq -w -0.0 1         -0.0 01.0
seq -w 1.5e1 16       15 16

The seq builtin parsed -w/--fixed-width and then ignored it, and did not
parse -f at all, so a format string was rejected as an invalid number.
Both flags are in its usage string.

-w now prints every value with the most decimals any operand was written
with, zero-padded (after the sign) to the wider of the two bounds, as BSD
and GNU seq do. -f/--format fails with the shell's standard unsupported
option message, as the other builtins do for flags their usage string
lists but they do not implement.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The seq builtin now supports fixed-width zero-padded output, infers decimal precision from operands, rejects unsupported format options, and handles decimal and exponent notation with bounded precision.

Changes

seq formatting behavior

Layer / File(s) Summary
Option parsing and precision state
src/runtime/shell/builtin/seq.rs
The builtin initializes formatting state, supports -w and --fixed-width, rejects -f and --format, and records operand decimal places.
Fixed-width layout and rendering
src/runtime/shell/builtin/seq.rs
The implementation handles exponent notation, caps extreme precision, calculates endpoint width, and renders zero-padded values when enabled.
Formatting behavior tests
test/js/bun/shell/commands/seq.test.ts
Tests cover decimal output, unsupported options, integer and fractional padding, signs, separators, terminators, captured and file output, and precision limits.

Possibly related PRs

  • oven-sh/bun#37923: This PR extends the same seq operand precision and f64 parsing work with fixed-width formatting.

Suggested reviewers: jarred-sumner

Mergeability Score: 🟠 High · up to 5b071

Fixed-width sequences can buffer an unexpectedly large amount of output before writing it, allowing crafted inputs to exhaust memory and terminate the shell. The implementation should stream output or enforce a recoverable size limit before merge.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: implementing seq fixed-width output and reporting format options as unsupported.
Description check ✅ Passed The description explains the problem, implementation, verification steps, test coverage, and known CI context, although it does not use the template headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:46 PM PT - Aug 12th, 2026

@robobun, your commit 5b07156 has 2 failures in Build #93787 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37926

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

bun-37926 --bun

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on bun 1.4.0 and a current debug build of main with

bun -e 'console.log(JSON.stringify(await Bun.$`seq -w 8 11`.text())); const r = await Bun.$`seq -f %g 1 3`.nothrow().quiet(); console.log(r.exitCode, r.stderr.toString())'

which prints "8\n9\n10\n11\n" and 1 seq: invalid argument. Fix and tests are in this PR; the new cases in test/js/bun/shell/commands/seq.test.ts fail 23 on the released build and pass with the change.

CI: the diff is green on every lane that ran (build 93665: 176 of 181 jobs passed, seq.test.ts included; the only failure was the freebsd aarch64 build lane, and the 4 darwin jobs never left the queue). Each build of this branch has lost a different one to four build lanes to vendor tarball downloads from github.com failing in the dependency step (Failed to download after 5 attempts, mimalloc / c-ares / libuv / lol-html / WebKit), which is hitting other PRs today as well and is unrelated to this change; that has been reported separately. Review threads are all addressed and resolved. Ready for a maintainer.

@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 — self-contained implementation of seq -w plus a proper unsupported-option message for -f, with thorough test coverage.

What was reviewed

  • -w width/precision derivation in FixedWidth::new and decimal_places: bounds-based width, operand-based precision, and the 1074-decimal cap guarding core::fmt's u16::MAX panic — all check out against the stated invariants.
  • The non--w output path is byte-for-byte unchanged; decimals is tracked unconditionally but only consumed under fixed_width.
  • -f routing through Builtin::fail_parse/ParseError::Unsupported matches how other builtins report unimplemented advertised flags; --format=foo falls through to the operand parser like --separator=foo already does, so behavior is consistent.
  • Verified the referenced helpers (bun_core::fmt::count, parse_decimal, strings::index_of_any, unsupported_flag) exist with matching signatures.
Extended reasoning...

Overview

This PR implements the -w / --fixed-width flag for the seq shell builtin (previously parsed and silently ignored) and routes -f / --format through the shared ParseError::Unsupported path so users get an actionable message instead of invalid argument. Changes are confined to src/runtime/shell/builtin/seq.rs (net +~100 lines: two new struct fields, a FixedWidth helper, a decimal_places parser, and a new arm in the flag loop) and 22 new test cases in test/js/bun/shell/commands/seq.test.ts.

Security risks

None. This is output formatting for a shell builtin that operates on already-validated finite f32 operands. The one adversarial-input concern — a user-written exponent like 1e-70000 requesting a precision above core::fmt's u16::MAX panic threshold — is explicitly handled by the MAX_FIXED_WIDTH_DECIMALS cap and covered by a test. decimal_places only runs on strings parse_f32 has already accepted, and its arithmetic is done in i64 and clamped before the u32 cast, so there is no overflow path. No unsafe, no allocation ownership changes, no FFI.

Level of scrutiny

Low-to-moderate. The change is additive to a small, isolated builtin; the non--w code path is unchanged. The -w semantics are derived from BSD/GNU seq and the PR description walks through why width-from-bounds and precision-from-operands is correct — I checked the reasoning holds (every printed value lies in [min(start,end), max(start,end)], and at fixed precision no value renders wider than the wider bound). The -f change replaces one error message with a more accurate one and follows the existing pattern used by cat/cp/touch/mkdir.

Other factors

Test coverage is unusually thorough for a builtin flag: integers up/down, single operand, negative bounds, sign-after-zero-padding, fractional increments, exponent-notation operands (both directions), interaction with -s/-t, captured stdout, and the precision-cap edge case. All referenced in-tree helpers (bun_core::fmt::count, bun_core::fmt::parse_decimal, bun_core::strings::index_of_any/index_of_char_usize, interpreter::unsupported_flag, Builtin::fail_parse) exist with the signatures used. The PR description already flags the mechanical overlap with #37923 and #33995. No prior human review comments to address.

Comment thread src/runtime/shell/builtin/seq.rs Outdated
Comment thread src/runtime/shell/builtin/seq.rs Outdated
Comment thread src/runtime/shell/builtin/seq.rs Outdated
Comment thread src/runtime/shell/builtin/seq.rs
Comment thread src/runtime/shell/builtin/seq.rs Outdated
Comment thread src/runtime/shell/builtin/seq.rs Outdated
Comment thread src/runtime/shell/builtin/seq.rs
Comment thread src/runtime/shell/builtin/seq.rs
Comment thread src/runtime/shell/builtin/seq.rs
Comment thread src/runtime/shell/builtin/seq.rs
Comment thread src/runtime/shell/builtin/seq.rs
Comment thread src/runtime/shell/builtin/seq.rs

@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 — self-contained seq builtin change, well-covered by tests.

What was reviewed:

  • -w implementation: FixedWidth::new computes width from max(len(start), len(end)) at fixed precision; the non--w path is unchanged.
  • decimal_places: only runs on operands parse_f32 already accepted, and the result is clamped to MAX_FIXED_WIDTH_DECIMALS (1074) before reaching core::fmt, so the u16::MAX precision panic is unreachable (covered by the 1e-70000 test).
  • -f handling: starts_with(b"-f") cannot collide with negative operands or the other short flags; routes through the shared fail_parse / ParseError::Unsupported path like cat/cp/touch.
  • Comment-cop threads were addressed in 804573d and are all resolved.
Extended reasoning...

Overview

This PR touches exactly two files: src/runtime/shell/builtin/seq.rs (implementing -w and reporting -f as unsupported) and test/js/bun/shell/commands/seq.test.ts (23 new test cases). The -w flag was previously consumed and ignored; it now zero-pads output to a fixed width derived from the wider of the two bounds rendered at the maximum decimal precision seen across all operands. -f/--format now goes through the shared Builtin::fail_parse + ParseError::Unsupported path instead of falling through to "invalid argument".

Security risks

None. This is output formatting for a shell builtin. No untrusted length is used to size an allocation without a bound: decimals is capped at 1074 before being passed as a core::fmt precision, which both prevents the documented u16::MAX panic and bounds per-value output size. No new unsafe, no FFI, no syscalls added.

Level of scrutiny

Low-to-medium. The change is confined to one builtin's flag parsing and output rendering; the pre-existing loop, state machine, and I/O paths are unchanged. The new code follows the file's established patterns (Self::state_mut via the BuiltinState trait, bun_core::fmt/bun_core::strings helpers, Builtin::fail_parse for unsupported flags). I verified that the referenced helpers (bun_core::fmt::count, parse_decimal, index_of_any, index_of_char_usize, unsupported_flag, fail_parse) exist with the expected signatures.

Other factors

Test coverage is thorough for a feature this size: up/down/single-operand, negative values, sign placement, decimals from the increment, exponent spellings, interaction with -s/-t, both output paths (captured stdout and file redirect), the precision cap, and all four -f spellings. The comment-cop bot flagged verbose comments; the author trimmed them in 804573d and each thread is marked resolved with a stated rationale for what was kept. No CODEOWNERS entry covers src/runtime/shell/. The one CI failure (FreeBSD aarch64 build-bun) is in scripts/build/ci.ts infrastructure, not this change. The PR notes overlap with #37923 and #33995 and states both are mechanically resolvable in either merge order.

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

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 `@src/runtime/shell/builtin/seq.rs`:
- Around line 214-221: Bound output accumulation in the fixed-width formatting
path around fixed_width and out so untrusted width/decimal inputs cannot grow
the buffer without limit before I/O. Enforce a recoverable maximum before
appending each rendered value, or flush bounded chunks incrementally, while
preserving seq output formatting and progression behavior.
🪄 Autofix

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: f65b6f44-33ef-4fdc-8957-813bec0bfbcb

📥 Commits

Reviewing files that changed from the base of the PR and between 315136d and 5b07156.

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

Comment thread src/runtime/shell/builtin/seq.rs

@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 — self-contained implementation of seq -w plus routing -f through the shared ParseError::Unsupported path.

What was reviewed:

  • -w width/precision derivation: decimal_places handles ., e±N, and clamps; FixedWidth::new caps at 1074 so core::fmt can't panic on width/precision > u16::MAX.
  • Verified arg.starts_with(b"-f") cannot shadow a valid negative-float operand, and parse_decimal::<i32> accepts a leading + so 1e+3 exponents parse.
  • Non--w output path is unchanged; 23 new tests cover both output paths, signs, exponent spellings, flag combinations, and the precision cap.
Extended reasoning...

Overview

Two files: src/runtime/shell/builtin/seq.rs gains a working -w/--fixed-width (new fixed_width: bool and decimals: u32 fields, a FixedWidth layout struct, and a decimal_places helper) and a -f/--format arm that returns Builtin::fail_parse(.., ParseError::Unsupported(..)). test/js/bun/shell/commands/seq.test.ts adds a seq -w describe block plus -f cases (23 new tests).

Security risks

None. This is output formatting in a shell builtin; no filesystem, network, or privilege boundaries are touched. The one user-controlled size input (implied decimal places from operand text) is clamped to 1074 before reaching core::fmt, and decimal_places clamps its intermediate to u32::MAX in i64 before casting.

Level of scrutiny

Low-to-medium. The change is confined to one builtin's rendering path, uses existing in-tree helpers (bun_core::fmt::count, parse_decimal, strings::index_of_any, Builtin::fail_parse/unsupported_flag), and leaves the non--w branch byte-identical. I traced operand reassignment for the 1/2/3-operand forms and confirmed FixedWidth::new reads the final start/end; checked that parse_int (behind parse_decimal) accepts a leading + so 1e+3 doesn't fall into the unwrap_or(0) branch; and confirmed -f's starts_with(b"-f") cannot collide with any operand parse_f32 would accept.

Other factors

Test coverage is thorough per REVIEW.md's variant-matrix guidance: up/down, single operand, unreached end bound, negative bounds and sign placement, fractional increments, exponent spellings, -s/-t interaction, captured stdout vs. file redirect, and the 1074-decimal cap. The comment-cop bot flagged verbose comments on an earlier revision; the author trimmed them in 804573d and all threads are resolved. No human reviewer has requested changes and there is no CODEOWNERS entry for src/runtime/shell/.

@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Heads-up from #39265, which makes seq's -s/-t arms accept the --separator=X / --terminator=X spellings: the --format arm added here matches the exact token only, so seq --format=%g 3 would still fall through to the operand parser and report seq: invalid argument rather than the unsupported-option message. Checking arg.starts_with(b"--format=") alongside arg == b"--format" (mirroring how -f is already matched by prefix) would cover it. The two PRs touch different lines, so they should rebase over each other cleanly either way.

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