[robobun]

Problem

• The shell's which builtin prints two different lines for an arg it cannot resolve, depending on how stdout is collected:
  • .quiet(), .text(), $(which ...), which ... > ${buf}: which: nope not found
  • a tty, | cmd, > file: nope not found
• Cause: src/runtime/shell/builtin/which.rs had two implementations of its output. When stdout is a captured buffer, start resolved every arg in a loop and formatted the line with fmt_error_arena(.., Some(Kind::Which), ..), which adds the prefix. When stdout is an fd, a per-arg state machine (State::MultiArgs, next, arg_complete) formatted the line with enqueue_fmt(.., None, ..). The two had disagreed since the builtin was added; which.test.ts on main papers over it by stripping an optional which: with a regex.
• Found by reading the code (while shell: return which's PATH and cwd lookup as a struct instead of a tuple #39131 touched the same file); there is no user report.

Fix

• which now renders its whole output once. Which::render builds one buffer with a line per arg (the resolved path, or <arg> not found) and the exit code; start either enqueues that buffer on the IOWriter or appends it to the captured buffer. This is the shape pwd, basename and dirname already use, and with one formatting site the two output modes cannot drift again. The only state left is the exit code to report when the write completes, so Which moves from the boxed: to the inline: row of the builtin table in Builtin.rs.
• Deleted: State::{Idle, OneArg, MultiArgs}, next, arg_complete, the arg helper and both formatter call sites. SearchEnv from shell: return which's PATH and cwd lookup as a struct instead of a tuple #39131 is unchanged and is now loaded once per command in both modes (the fd mode used to reload it per arg). which.rs goes from 227 lines to 101.
• The unprefixed line is the one kept: it is what bunshell.test.ts "which" has asserted since the builtin was added, it is zsh's wording for its builtin which, and in this codebase the "<builtin>: " prefix (Kind::as_str) is the convention for stderr diagnostics, while this line is regular stdout output.
• Unchanged on purpose: the no-arg case still prints a lone newline and exits 1, the exit code is still 1 if any arg was not found, and a failed write still reports err.errno as the exit code exactly as before (the write-error exit convention is shell: exit 1 on builtin failure instead of raw errno; fix cd ENOENT message #32278's subject).
• Second visible change: lines are assembled from the raw bytes of the path or arg. Before, both modes went through bstr::BStr's Display, which replaces invalid UTF-8 with U+FFFD; the other builtins write their stdout byte for byte and so did this one before the Rust port.
• Verified with test/js/bun/shell/commands/which.test.ts: a describe block asserts the same line for every buffer-backed mode (.quiet(), .text(), command substitution, > ${Buffer}) and for a file redirect (IOWriter mode), plus the all-not-found and no-arg cases; a second block feeds a 0xff byte in through $(cat arg.bin) and compares hex output in both modes, which pins the raw-bytes change (the unfixed build produces ef bf bd in both modes). The long-path test no longer strips an optional prefix, and the comment in shell: return which's PATH and cwd lookup as a struct instead of a tuple #39131's streamed-stdout test is updated since there is no longer a one-write-per-arg path. 8 of 14 cases fail on the unfixed build; all 14 pass with the fix.
• Also green against the debug build: bunshell.test.ts (423 pass), pipeline_stack.test.ts, exec.test.ts, the rest of test/js/bun/shell/commands/, and test/internal/source-lints/.

Background

• Shell builtins have two ways to write. When stdout is an fd (tty, pipe, file redirect), needs_io() returns a safeguard and the builtin enqueues bytes on an IOWriter, which copies them and later calls the builtin's on_io_writer_chunk; when stdout is a captured buffer (.quiet(), command substitution, a > ${buf} redirect), the builtin appends synchronously with write_no_io and finishes on the spot. Small builtins build their output first and then pick one of the two writes; until now which instead had a separate implementation per write.
• Builtin::fmt_error_arena(.., Option<Kind>, ..) and BuiltinIO::enqueue_fmt(.., Option<Kind>, ..) prepend "<builtin name>: " when given a Kind; builtins use that for their error messages on stderr.
• The shell_builtins! table in Builtin.rs stores inline: builtins' state directly in the per-command enum and boxed: ones behind a Box, to keep the common node small; Which is now two bytes of state.

Repro on 1.4.0

import { $ } from "bun";
const quiet = await $`which ls nope_zz`.nothrow().quiet();
console.log(JSON.stringify(quiet.stdout.toString()));
// "/usr/bin/ls\nwhich: nope_zz not found\n"
const piped = await $`which ls nope_zz | cat`.nothrow().quiet();
console.log(JSON.stringify(piped.stdout.toString()));
// "/usr/bin/ls\nnope_zz not found\n"

Earlier version of this PR

The first revision kept both output paths and only routed them through a shared line-building helper (changing Some(Kind::Which) to the unprefixed form). Self-review pointed out that the two paths are themselves the mechanism of the bug and that the one-buffer shape used by the neighbouring builtins is both smaller and removes the drift structurally, and #39131 landed on main in the meantime, so the branch was rebased and the src side replaced with the rewrite described above. The tests from that revision are kept and extended with the byte-level cases.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

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: 68dd6dd8-7221-42b0-9acc-92c590d09605

📥 Commits

Reviewing files that changed from the base of the PR and between e1a4ba7 and a8fbd4c.

📒 Files selected for processing (3)

• src/runtime/shell/Builtin.rs
• src/runtime/shell/builtin/which.rs
• test/js/bun/shell/commands/which.test.ts

---

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

[robobun]

^{Updated 4:05 PM PT - Aug 15th, 2026}

❌ @robobun, your commit a8fbd4c has 2 failures in Build #98746 (All Failures):

• test/js/web/fetch/blob.test.ts - code 1 on 🐧 13 x64-asan
• test/js/sql/postgres-prepared-pipeline-reorder.test.ts - code 1: error: Failed to start service postgres_plain (via coordinator): Docker is not available. Please … on 🐧 3.23 x64

---

🧪   To try this PR locally:

bunx bun-pr 39163

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

bun-39163 --bun

[robobun]

Status

• Reproduced on 1.4.0 (and the debug build of main): $\which ls nope`.quiet()captureswhich: nope not found, while which ls nope | catandwhich ls nope > fileproducenope not found`.
• Fix (a8fbd4c, rebased on main after shell: return which's PATH and cwd lookup as a struct instead of a tuple #39131): which renders its output into one buffer and writes it through whichever stdout mechanism applies, replacing the separate captured-mode loop and streaming-mode state machine whose formatting had drifted. The line is <arg> not found in every mode.
• Proof: test/js/bun/shell/commands/which.test.ts, 8 of 14 cases fail without the fix (including the two byte-level cases that pin the raw-bytes output) and all 14 pass with it. bunshell.test.ts, pipeline_stack.test.ts, exec.test.ts and the rest of commands/ pass against the debug build.
• CI on a8fbd4c (build 98746): 175 of 179 jobs green and which.test.ts passes on every lane that ran; the two darwin aarch64 test shards never got an agent before the build was canceled (the previous revision's tests were green there in build 98577). The two red jobs are test/js/web/fetch/blob.test.ts (file-backed slice streaming, also failing on main) and test/js/sql/postgres-prepared-pipeline-reorder.test.ts (Docker unavailable on the alpine agent); neither involves the shell, and both are reported separately. Ready for review.

[claude]

LGTM — small, well-scoped consistency fix with thorough test coverage.

What was reviewed:

• Verified Which::line preserves the semantics of both collapsed branches: had_not_found |= !found matches the old per-branch assignments, waiting_write is still set on both outcomes, and ChildPtr::new (a pure const fn) is safely moved into the enqueue branch.
• Confirmed BuiltinIO::enqueue(child, &[u8], safeguard) matches the new call site, and the unprefixed <arg> not found form is what bunshell.test.ts:1187 already asserts for the streaming path.
• Checked that switching from format_args!("{}", bstr::BStr::new(...)) to raw bytes only affects non-UTF-8 paths/args, where writing raw bytes is the correct shell behavior anyway.
• Tests cover both the buffer-backed and fd-backed paths with exact-string assertions, use test.concurrent/tempDir/bunEnv per harness conventions, and the existing long-path test's prefix-stripping workaround is removed rather than left behind.

Extended reasoning...

Overview

This PR fixes an output inconsistency in the shell's which builtin: the not-found line was which: <arg> not found when stdout was a captured buffer (.quiet(), .text(), command substitution, > ${buf}) but <arg> not found when stdout was an fd (tty, pipe, file redirect). The cause was two independent formatting sites — the sync path in start() passed Some(Kind::Which) to fmt_error_arena (adding the prefix), the streaming path in next() passed None.

The fix extracts a single Which::line(path_env, cwd, arg) -> (Vec<u8>, bool) helper that both paths call, and both write exactly those bytes. The unprefixed form is kept, which matches the existing bunshell.test.ts assertion (line 1187), zsh's builtin which, and the repo convention that the <builtin>: prefix is for stderr diagnostics — this line is stdout output.

Net diff: ~70 lines removed, ~30 added in which.rs; 6 new test cases covering every stdout collection mode plus the existing long-path test tightened to no longer strip an optional prefix.

Security risks

None. This is output formatting for a shell builtin; no parsing of untrusted input, no auth/crypto/permissions, no new syscalls.

Level of scrutiny

Low-to-medium. The change is a mechanical dedup of two near-identical code paths into one helper, in a leaf builtin with no cross-module reach. I verified each piece of the collapsed control flow against the original:

• had_not_found |= !found in both start() and next() reproduces the old = true on the not-found branch and no-op on the found branch.
• waiting_write = true is now set unconditionally in next() — previously set in both match arms, so identical.
• ChildPtr::new is a pure const fn (IOWriter.rs:57), so moving it inside the needs_io() branch has no side effect.
• BuiltinIO::enqueue (Builtin.rs:406) takes &[u8] and the safeguard, matching the new call replacing enqueue_fmt.
• The now-unused Kind import is removed.

The only semantic change beyond dropping the prefix is that resolved paths and args are now written as raw bytes rather than through bstr::BStr's Display impl. That only differs for non-UTF-8 byte sequences, and for a shell path-printing builtin, raw bytes is the correct behavior.

Other factors

• Tests follow harness conventions: test.concurrent for independent cases, tempDir with using, .env(bunEnv), exact toBe assertions on stdout before exit code. The Bun.which(bunExe()) reference pattern is the same one bunshell.test.ts:1184-1187 already relies on.
• The PR description reports 6/9 new cases fail on the unfixed build and all pass with the fix, plus bunshell.test.ts -t which, pipeline_stack.test.ts, exec.test.ts, and source-lints all pass.
• No CODEOWNERS entry covers src/runtime/shell/.
• No prior human review comments or outstanding requests on the PR.

[claude]

LGTM — the doc-comment nit from the earlier pass is resolved in e2ef46f (both comments are gone), and no new issues.

What was reviewed:

• Both write paths (start() sync-buffer, next() streaming) now go through Self::line; control flow is preserved — had_not_found/waiting_write still updated, arg_complete still called on the no-io fallback.
• BuiltinIO::enqueue copies its &[u8] into the IOWriter's buffer (extend_from_slice), so passing the local line: Vec<u8> is sound.
• Tests cover the capture-mode matrix (.quiet(), .text(), $(), > ${Buffer}, > file) plus all-not-found; the existing long-path test drops its which: workaround.

Extended reasoning...

Overview

Two-file change: src/runtime/shell/builtin/which.rs collapses the two duplicated formatting paths (sync captured-buffer in start(), streaming fd in next()) into one Which::line(arg, resolved) -> Vec<u8> helper that both paths write verbatim, and test/js/bun/shell/commands/which.test.ts adds a describe block asserting identical bytes across every stdout collection mode. Net: ~90 lines removed from the Rust, ~45 lines of tests added, and the unused Kind import dropped.

Deltas since the last review

My previous inline comment flagged two redundant doc-comment clauses. In e2ef46f the author dropped the doc comments entirely (commit message: "take the resolve result in line() and drop the doc comments") and simplified line() to take the already-computed Option<Vec<u8>> instead of re-resolving. The current file has no module-doc addition and no doc on line(), so the nit is fully addressed. The comment-cop bot threads are marked resolved.

Security risks

None. This is stdout formatting for a shell builtin; no parsing of untrusted input beyond what bun_which::which already handled, no new syscalls, no auth/crypto surface.

Level of scrutiny

Low-to-medium. It is a small refactor of a single builtin's output path with a well-defined behavioral contract (zsh's <arg> not found). The one nontrivial thing to verify was that swapping enqueue_fmt for enqueue with a local Vec<u8> is lifetime-safe — IOWriter::enqueue does s.buf.extend_from_slice(buf) before returning, so the caller's buffer is copied and can drop immediately. Control flow is byte-for-byte preserved: had_not_found |= resolved.is_none() replaces the per-arm = true, waiting_write is still set before enqueue, and the no-io fallback in next() still calls arg_complete.

Other factors

The test additions follow harness conventions (test.concurrent, tempDir, bunEnv, exit-code asserted last) and cover the variant matrix REVIEW.md asks for. The existing long-path test was tightened rather than left with a lenient regex strip. The PR description documents that 6/9 new cases fail on the unfixed build and that bunshell.test.ts -t which still passes, so the choice of unprefixed output is consistent with pre-existing streaming-path assertions.

[claude]

LGTM — the collapsed render() in a8fbd4c is simpler than the two-path version I commented on earlier, and the doc-comment nit is moot now.

What was reviewed:

• which.rs: single render() builds the whole output; both fd and captured-buffer paths write those bytes verbatim, so the format cannot drift. on_io_writer_chunk now just reads back the stored exit_code.
• Builtin.rs: Which moved boxed→inline — the struct is now one ExitCode, so no Impl/Node size concern.
• Tests: cover every capture mode plus a file redirect (fd path), no-args, all-not-found, and non-UTF-8 arg bytes on both paths; the long-path test no longer strips the prefix. Checked that Bun.which(bunExe()) and the builtin agree on an absolute-path arg.

Extended reasoning...

Overview

The shell which builtin printed which: <arg> not found on the captured-buffer stdout path and <arg> not found on the fd path, because the two paths formatted the line separately and only one passed Some(Kind::Which) to fmt_error_arena. The fix collapses the ~175-line state machine in src/runtime/shell/builtin/which.rs into a ~70-line implementation: render() resolves every arg and returns (Vec<u8>, ExitCode); start() either enqueues that buffer once on the IOWriter or writes it via write_no_io. Which shrinks to { exit_code: ExitCode } and moves from the boxed to the inline group in shell_builtins!. Nine new tests in which.test.ts pin the exact bytes across .quiet(), .text(), command substitution, > ${Buffer}, > file, and non-UTF-8 args.

Security risks

None. No new unsafe blocks, no untrusted-input parsing beyond what bun_which::which already did, no new syscalls or fd handling. The output buffer is bounded by argc × PATH_MAX.

Level of scrutiny

Moderate — this rewrites a builtin's control flow, not just the format string. I checked: (1) BuiltinIO::enqueue copies its &[u8] argument (other callers pass locals the same way), so dropping out after enqueue is fine; (2) render() holds only shared borrows via Builtin::of/Builtin::shell and arg_bytes's lifetime is decoupled from &self per its doc, so no aliasing with the interior-mutability accessors; (3) moving Which to inline is safe since it's now one ExitCode — smaller than several existing inline variants; (4) the fd path now writes one chunk instead of one per arg, which matches pwd/basename/dirname/echo and is fine for which's bounded output. The PR description's "control flow is unchanged" line is stale relative to a8fbd4c, but the head commit's own message documents the single-buffer change.

Other factors

My earlier inline nit (redundant doc-comment clauses) targeted an intermediate revision and is resolved — those comments and the line() helper they described no longer exist. All comment-cop threads are resolved. CI on e2ef46f was 178/179 green with the one failure unrelated (bake dev-server on win2019); a8fbd4c is a pure simplification on top of that. Tests follow harness conventions (tempDir, bunEnv, test.concurrent, no sleeps, no network) and assert exact bytes including the exit code.

[robobun]

Closing: superseded by #43055. That PR moves the not-found line to stderr and formats it in one place, so the captured and the streamed text are equal there too. The raw-bytes change from this PR (no U+FFFD for invalid UTF-8 in the output) is not in #43055 and can be filed on its own. If the maintainers prefer the line on stdout, reopen this PR and close #43055.