shell(cat): map Step to a Yield in one place

[robobun]

Problem

• Cat::on_io_writer_chunk and Cat::on_io_reader_done (src/runtime/shell/builtin/cat.rs:303 and :397 before this change) both end with the same three-arm match turning a Step into a Yield. A change to one copy would miss the other.
• This is the same_match_twice:src/runtime/shell/builtin/cat.rs entry in mordant-baseline.toml.

Fix

• Add Step::run(self, interp, cmd) -> Yield holding the one mapping; both callbacks now end with step.run(interp, cmd).
• No behavior change: the arms are the ones that were there (Suspend -> Yield::suspended(), Done(code) -> Builtin::done, Next -> Cat::next), only the location moved. Step is private to cat.rs, so nothing else is affected.
• Remove the now-fixed entry from mordant-baseline.toml.
• Add a test to test/js/bun/shell/bunshell.test.ts ("builtin cat finishes from its reader and writer completions") that runs the builtin with captured output, with stdout on a file, and on a missing file with stderr captured and on a file. Instrumenting Step::run locally showed these go through on_io_reader_done -> Done(0), on_io_writer_chunk -> Suspend then on_io_reader_done -> Done(0), and on_io_writer_chunk -> Done(1). It is a characterization test for the refactor: it passes before and after this change (USE_SYSTEM_BUN=1 and bun bd), it does not fail without it.
• The missing-file assertions accept the message with or without a directory prefix: on Windows the file branch of shell_openat leaves the resolved absolute path in the error, so the builtin prints cat: C:\...\missing.txt: ... where POSIX prints cat: missing.txt: ... (seen on both Windows lanes of the first run of this test). That inconsistency is pre-existing and filed separately rather than fixed here.
• Verified:
  • bun bd test test/js/bun/shell/bunshell.test.ts: 424 pass, 0 fail. Note that on POSIX the builtin is only used when BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS is set (Kind::DISABLED_ON_POSIX), so on Linux only the new test and the flagged case in yield.test.ts run cat.rs; the rest of the shell suite exercises it on the Windows lanes, where the builtin is always on.
  • bun bd test on file-io, yield, shell-write-fault, shell-pipe-read-fault, exec, assignments-in-pipeline, bunshell-instance: all pass.
  • bun run rust:mordant with this diff: no findings, target/mordant/over-baseline.txt not written. As a control, the same command with cat.rs reverted and the baseline entry still removed reports the cat.rs:397 finding as 1 over baseline in bun_runtime.
• The Next arm is not reachable from the new test on POSIX: reading a regular file through the builtin currently fails there (epoll rejects regular files), which is what shell(cat): read regular files synchronously in the builtin on POSIX #35337 fixes. shell(cat): read regular files synchronously in the builtin on POSIX #35337 and shell(cat): finish after a read error instead of cancelling the queued output and hanging #37743 also edit the tail of these two callbacks, so whichever of those and this lands later needs a one-line rebase in cat.rs.

Background

• The cat builtin is a small state machine driven by IOReader/IOWriter callbacks. Each callback mutates CatState under a &mut borrow and produces a Step (suspend, finish with an exit code, or move to the next file), then acts on it after the borrow is released; Step::run is that second half.
• mordant is the advisory Rust lint pack run by bun run rust:mordant; mordant-baseline.toml holds per-(lint, file) counts of pre-existing findings, and a fixed finding's entry is deleted so the ratchet tightens.

Local leak.test.ts note

bun bd test test/js/bun/shell/leak.test.ts in this (debug + ASAN) environment times out the 500-iteration memleak_* cases at their 100s limit, including ones that never run cat (memleak_change_cwd, memleak_redirect_file, memleak_ls, memleak_Blob_*); the 100-iteration cases and the 1000-iteration fdleak_* cases pass. The timeouts are machine speed, not this change (and on Linux those tests run the system cat anyway, see above).

---

no test proof · iteration 2 · 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

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 53 minutes

Limit details: You’ve used all 1 included review currently available under your plan. You completed 74 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour.

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: 11ba4aa5-6004-475c-affd-6c8cf08ee75a

📥 Commits

Reviewing files that changed from the base of the PR and between 486af28 and 9e2f40b.

📒 Files selected for processing (3)

• mordant-baseline.toml
• src/runtime/shell/builtin/cat.rs
• test/js/bun/shell/bunshell.test.ts

---

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

[robobun]

^{Updated 1:06 PM PT - Aug 15th, 2026}

✅ @robobun, your commit 13ce39758a71a12ac4dea1d1e235a89ffe307023 passed in Build #98586! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 39147

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

bun-39147 --bun

[claude]

LGTM — pure dedup of two identical Step → Yield match blocks into Step::run.

What was reviewed:

• Confirmed the three arms in Step::run are byte-identical to both removed copies (Self::next was Cat::next in the impl Cat block).
• Step is pub(crate) but not referenced outside cat.rs, so the new method has no other consumers.
• mordant-baseline.toml entry removal matches the resolved same_match_twice finding.

Extended reasoning...

Overview

This PR deduplicates two identical three-arm match blocks in src/runtime/shell/builtin/cat.rs that each mapped a Step to a Yield at the tail of on_io_writer_chunk and on_io_reader_done. The mapping is moved into a new Step::run(self, interp, cmd) -> Yield and both call sites now invoke it. The corresponding same_match_twice:src/runtime/shell/builtin/cat.rs entry is removed from mordant-baseline.toml.

Security risks

None. This is an internal refactor of a shell builtin's control-flow tail; no input parsing, no allocation, no FFI, no lifetime changes. The moved arms call the same three functions with the same arguments.

Level of scrutiny

Low. The change is mechanical: the new method body is character-for-character the same as both removed blocks (modulo Self::next → Cat::next, which resolve to the same function since the original blocks lived in impl Cat). Step is file-local (grep confirms no external references), so adding a method to it cannot affect anything else. The baseline-file edit is a single line deletion matching the fixed lint.

Other factors

• No CODEOWNERS entries cover the touched paths.
• The PR description reports the shell test suite (bunshell, yield, shell-write-fault, shell-pipe-read-fault, etc.) passing on the debug build, and a control run showing mordant re-flags the finding when cat.rs is reverted — so the baseline removal is verified live, not assumed.
• This is exactly the shape the repo review guide asks for ("the second time a multi-line block appears in your diff, extract a named helper"), applied to pre-existing code flagged by the mordant ratchet.

[claude]

LGTM — the Windows path concern from my earlier review is now handled by the expect.stringMatching(/^cat: (.*[\\/])?missing\.txt: .../) matcher in 011b659.

What was reviewed:

• Step::run is a byte-for-byte extraction of the two identical match arms; Step is file-private so nothing else is affected.
• The error-message text on Windows also comes from coreutils_error_map (ENOENT → "No such file or directory"), so the regex tail matches there too.
• Test follows harness conventions (tempDir, bunEnv spread, concurrent pipe drain, stderr asserted before exitCode).

Extended reasoning...

Overview

This PR extracts a duplicated three-arm match Step -> Yield block from Cat::on_io_writer_chunk and Cat::on_io_reader_done into a single Step::run(self, interp, cmd) method. Both call sites now end with step.run(interp, cmd). The corresponding same_match_twice:src/runtime/shell/builtin/cat.rs entry is removed from mordant-baseline.toml, and a characterization test is added to bunshell.test.ts exercising the builtin cat's reader/writer completion paths.

Security risks

None. No user-facing surface changes, no new inputs parsed, no allocation or FFI changes. Pure code motion within a file-private enum.

Level of scrutiny

Low. The Rust change is a mechanical extraction: the three arms (Suspend -> Yield::suspended(), Done(code) -> Builtin::done, Next -> Cat::next) are identical to what was inlined before, just relocated into an impl Step block. Step is pub(crate) and only referenced within cat.rs, so there are no external consumers to audit. The baseline-file edit is a one-line deletion whose correctness the author verified with bun run rust:mordant (including a control run).

Other factors

My previous review flagged that the new test's exact-string assertion on cat: missing.txt: No such file or directory would fail on Windows because shell_openat's file-open branch tags the error with the resolved absolute path there. The author addressed this in commit 011b659 by switching both missing-file assertions to expect.stringMatching(/^cat: (.*[\\/])?missing\.txt: No such file or directory\n$/), which accepts an optional path prefix ending in either separator. I confirmed the message text itself is platform-stable: task_error_to_string pulls it from bun_sys::coreutils_error_map, a static table where ENOENT maps to "No such file or directory" on all platforms (there's a debug_assert for exactly that in coreutils_error_map.rs:25).

The test otherwise follows repo conventions: tempDir with using, {...bunEnv, ...}, Bun.spawn with concurrent Promise.all drain of stdout/stderr/exited, stderr asserted before exitCode, and the asymmetric matcher used correctly inside toEqual. The PR description is transparent that this is a characterization test (passes before and after), which is appropriate for a no-behavior-change refactor.

[claude]

LGTM — the Windows path issue from my previous review is fixed via expect.stringMatching, and the thread is resolved.

What was reviewed:

• Step::run is a byte-for-byte extraction of the two identical match blocks; Self::next → Cat::next is the same call.
• The regex ^cat: (.*[\\/])?missing\.txt: ... accepts both the bare POSIX path and the Windows absolute path the author confirmed on both Windows lanes.
• Test follows harness conventions (tempDir, bunEnv spread, concurrent pipe drain, exitCode asserted last); asymmetric matchers inside toEqual work in Bun's Jest runner.

Extended reasoning...

Overview

This PR deduplicates two identical three-arm match Step { ... } blocks in src/runtime/shell/builtin/cat.rs into a single Step::run(self, interp, cmd) -> Yield method, removes the corresponding same_match_twice entry from mordant-baseline.toml, and adds a characterization test to test/js/bun/shell/bunshell.test.ts covering the reader/writer completion paths of the builtin cat.

Change since my last review

I previously flagged that the missing-file assertions would fail on Windows because shell_openat's Windows file-open branch tags the error with the resolved absolute path. The author confirmed this on both Windows CI lanes and updated the two assertions to expect.stringMatching(/^cat: (.*[\\/])?missing\.txt: No such file or directory\n$/), which accepts an optional directory prefix ending in either separator. The underlying shell_openat inconsistency is filed separately — reasonable scoping for a lint-cleanup PR. The review thread is marked resolved.

Security risks

None. No user-facing behavior change, no new input parsing, no allocation or FFI changes — just moving three match arms into a method on a file-private enum.

Level of scrutiny

Low. The Rust change is mechanically verifiable as behavior-preserving: the extracted arms are identical to both originals (Self::next in the original context is Cat::next). Step is private to cat.rs. The baseline removal matches the fix. The test is additive and hermetic (temp dir, spawned subprocess, no network).

Other factors

The PR description is transparent that the test is a characterization test (passes before and after, including with USE_SYSTEM_BUN=1) — appropriate for a no-behavior-change refactor. Test follows repo conventions: {...bunEnv, ...}, tempDir with using, Promise.all on stdout/stderr/exited, stderr asserted before exitCode. The author verified bun run rust:mordant is clean and ran the shell test suite. The one open concern from my prior review has been addressed and confirmed against actual Windows CI output.

[claude]

LGTM — the Windows path-prefix issue from my earlier comment is addressed by the expect.stringMatching regex, and CI is green on all lanes including both Windows targets.

What was reviewed:

• Step::run is a byte-identical extraction of the two duplicated match arms (Self::next → Cat::next is the same call); Step is file-private so nothing else can observe the move.
• The mordant baseline entry removal matches the fixed lint; author verified the ratchet with a control run.
• New test follows harness conventions (tempDir, bunEnv spread, concurrent pipe drain, stderr asserted before exitCode); the missing-file regex is anchored and handles both POSIX relative and Windows absolute-path prefixes.

Extended reasoning...

Overview

This PR deduplicates two identical three-arm match blocks in src/runtime/shell/builtin/cat.rs (at the tails of on_io_writer_chunk and on_io_reader_done) into a single Step::run(self, interp, cmd) -> Yield method, removes the corresponding same_match_twice entry from mordant-baseline.toml, and adds a characterization test to test/js/bun/shell/bunshell.test.ts covering the reader/writer completion paths of the builtin cat.

Security risks

None. This is an internal extract-method refactor in the shell builtin state machine with no new inputs, no parsing changes, and no boundary crossings. The test spawns a child bun process in a temp dir with no network or privileged operations.

Level of scrutiny

Low. The Rust change is a pure mechanical extraction: the three match arms (Suspend → Yield::suspended(), Done(code) → Builtin::done, Next → Cat::next) are moved verbatim into impl Step, and both call sites now delegate to it. Step is pub(crate) and only used within cat.rs, so there are no other consumers. The only thing that could go wrong is a typo in the extraction, and a line-by-line comparison shows none (Self::next in the original context resolved to Cat::next, which is what the new helper calls explicitly).

Other factors

• I previously flagged that the test's exact-string missing-file assertions would fail on Windows because shell_openat's file branch doesn't re-tag the error path. The author confirmed this on both Windows CI lanes and switched to expect.stringMatching(/^cat: (.*[\\/])?missing\.txt: No such file or directory\n$/), which accepts either form. That thread is resolved and the underlying shell_openat inconsistency is filed separately.
• CI (Build #98586) passed on all platforms after the fix.
• The PR description is transparent that the test is a characterization test (passes with and without the change) — appropriate for a no-behavior-change refactor, and the author verified via local instrumentation that the four cases actually route through the code paths named.
• Test follows repo conventions: tempDir from harness, {...bunEnv, ...} spread, Promise.all on stdout/stderr/exited, stderr asserted before exitCode, port: 0 not applicable.