shell: report the spawned command's exit code when argv comes from a sole $(...)

[robobun]

Problem

• In Bun Shell, $(echo ./exit7.sh) reports exit code 0 when the script exits 7, and $(echo /bin/true; exit 5) reports 5 instead of 0. bash reports 7 and 0.
• $(echo ./exit7.sh) &> out.txt never resolves.
• Only a command that is a single $(...) word expanding to an external command is affected. Builtins overwrite the exit code and are fine.
• Cause: the substitution's status was stored in the field that also means "the process has already reported", so the real status was ignored, and with nothing piped back to the shell nothing ever finished the command.

Fix

• The substitution's status gets its own field, read only when the expansion produced no command name (POSIX 2.9.1). $(exit 3) still reports 3.
• Property to check: the exit-code field stays unset until a builtin or the subprocess reports, which is what the finish and process-exit paths assume.
• Verification: new shell tests. Without the fix the external-command cases report 0, the &> case times out, and the failing-substitution case reports 5. The builtin and empty-expansion cases already passed.

Background

• Command substitution: $(cmd) runs cmd and splices its stdout into the command line as words, so $(echo ./x.sh) becomes the command ./x.sh.
• Each simple command in Bun Shell's Rust interpreter is a Cmd state: it expands its words one at a time, then runs a builtin or spawns a subprocess.
• A spawned command is finished once its exit code is recorded and its pipes back to the shell are closed. A file redirect removes the pipe, so only the process exit notification can finish it.
• The exit-code field doubles as an "already reported" flag: the process-exit handler leaves it alone when set, so a stdout/stderr read error wins over the process status.

Original description

Repro

import { $ } from "bun";
// exit7.sh: #!/bin/sh
//           exit 7
console.log((await $`$(echo ./exit7.sh)`.nothrow()).exitCode);         // 0, expected 7
console.log((await $`$(echo ./exit7.sh) x`.nothrow()).exitCode);       // 7 (two words, works)
console.log((await $`$(echo /bin/true; exit 5)`.nothrow()).exitCode);  // 5, expected 0
await $`$(echo ./exit7.sh) &> out.txt`.nothrow();                      // never resolves

bash reports 7, 7, 0 for the first three ($(echo ./exit7.sh); echo $? prints 7): once a substitution expands to a command name, the status of the simple command is the status of the command that ran. The substitution's own status only counts when there is no command name at all (POSIX 2.9.1), which is $(exit 3) reporting 3 today.

Only the combination "the whole command is one $(...) word" and "it expands to an external command" is affected. Builtins are fine because Builtin::done overwrites the exit code unconditionally.

Cause

Cmd::child_done (src/runtime/shell/states/Cmd.rs) stashed the substitution's status in Cmd::exit_code when the sole argv atom is a $(...), so that the empty-argv branch of transition_to_exec could return it. When argv turned out to be non-empty the stash was left in place, and exit_code.is_some() is what the subprocess path uses to mean "the exec has already reported":

• ShellSubprocess::on_process_exit skips Cmd::on_exit when exit_code is already set (so that a stdout/stderr read error wins over the process status), so the real status was never recorded and the stale value was reported.
• Cmd::has_finished only checks exit_code.is_some() plus the piped stdio being closed, so with stdout/stderr captured the command "finished" as soon as the pipes closed, and with nothing piped (&> file, or a file redirect on both streams) nothing ever finished it, so the promise hung.

Fix

Keep the substitution's status in its own Cmd field and read it only in the empty-argv branch. exit_code now stays None until the builtin or subprocess actually reports, which is the invariant has_finished and on_process_exit rely on. The empty-argv behavior ($(exit 3) reports 3) is unchanged.

Tests

New cases in the cmd subst expanding to the whole command block of test/js/bun/shell/bunshell.test.ts. Against the current release the first two report 0 instead of 7, the third times out, and the fourth reports 5 instead of 0; the last two already pass and pin the builtin and empty-argv paths.

• $(echo bun exit7.js) with stdout/stderr captured: 7
• the same with > out.txt (stderr still piped): 7
• the same with &> out.txt (nothing piped): 7, and the command completes
• $(echo bun ok.js; exit 5): 0
• $(echo false): 1
• $(echo; exit 3): 3

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 8 minutes

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: 97553908-f1fa-4b72-b400-72f16721b5d4

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and efa8531.

📒 Files selected for processing (2)

• src/runtime/shell/states/Cmd.rs
• test/js/bun/shell/bunshell.test.ts

---

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

[robobun]

Status: reproduced on the 1.4.0 release and on a debug build of main (Linux) with the new cases in test/js/bun/shell/bunshell.test.ts. Without the fix, external command and external command with stdout redirected report 0 instead of 7, external command with stdout and stderr redirected times out, failing substitution does not override the command's exit code reports 5, and external command's exit code drives || never runs the || branch. All seven pass with the fix, along with the rest of test/js/bun/shell/bunshell.test.ts.

CI: the first build (d0063ea) passed every lane that ran, including all Windows lanes and macOS 14; the only non-passing jobs were the macOS 26 arm64 test lane expiring without an agent. Later pushes only add a test case (eb21596) and shorten comments (efa8531).

[claude]

LGTM — clean root-cause fix that separates the substitution's stashed status from the exec's, restoring the exit_code.is_none() invariant that has_finished and on_process_exit depend on.

What was reviewed:

• Verified on_process_exit gates on cmd.exit_code.is_none() (subproc.rs:987), so leaving exit_code unset until the exec reports is the correct fix for both the wrong-status and the hang cases.
• Confirmed the new slice pattern [Simple(CmdSubst(_))] on name_and_args: &[Atom] is equivalent to the old len() == 1 && matches!(...) guard; the dropped new_idx == 1 check was redundant since a one-element argv only expands once.
• Checked the empty-argv path still returns the substitution's status via cmd_subst_exit_code (default 0), and that redirects don't affect the match since redirect_file is separate from name_and_args.
• Tests cover piped / partially redirected / fully redirected (the hang case), the inverse (failing subst + succeeding command), and pin the builtin and empty-argv paths.

Extended reasoning...

Overview

The PR fixes a bug in Bun's shell where a command whose entire argv comes from a single $(...) substitution reported the substitution's exit code instead of the spawned command's, and could hang when both stdout/stderr were redirected to files. The change touches src/runtime/shell/states/Cmd.rs (adds a cmd_subst_exit_code: ExitCode field, moves the stash from exit_code to that field, and reads it only in the empty-argv branch of transition_to_exec) and adds six TestBuilder cases in test/js/bun/shell/bunshell.test.ts.

Security risks

None. This is internal shell state-machine bookkeeping — no untrusted input parsing, no new unsafe blocks, no FFI or memory-ownership changes, no filesystem/network surface.

Level of scrutiny

Moderate. The shell interpreter state machine is intricate, but the change is narrowly scoped: one new plain u8 field on Cmd, one assignment retargeted, one read site changed from exit_code.unwrap_or(0) to cmd_subst_exit_code. The refactor from an index-and-length check to a slice pattern is a straight simplification. I traced the invariant the PR description claims (ShellSubprocess::on_process_exit skips on_exit when exit_code is already Some, and has_finished keys off exit_code.is_some()) directly in the source and it holds, so the mechanism is proven rather than correlated.

Other factors

The tests are well-constructed per the repo's review standards: they cover the variant matrix (captured / one stream redirected / both redirected — the hang case), the inverse case where the substitution fails but the expanded command succeeds, and pin the two paths that already worked (builtin via false, empty expansion via $(echo; exit 3)) so a future regression there is caught. They use the existing TestBuilder / BUN / .file() harness conventions from the surrounding file. The PR description explicitly documents which cases fail on the current release, satisfying the fails-for-the-right-reason requirement.

[robobun]

^{Updated 3:06 PM PT - Aug 12th, 2026}

❌ @robobun, your commit efa8531 has 4 failures in Build #93454 (All Failures):

• test/js/third_party/grpc-js/test-tonic.test.ts - code 1: error: FILE_ENDED on 🐧 3.23 x64
• test/bake/deinitialization.test.ts - pid 2708 segmentation fault at address 0x0 on 🪟 2019 x64
• step failed outside runner - exit 56 on 🐧 x64-musl - verify-baseline
• Failed to create agent for 🪟 x64 - verify-baseline

---

🧪   To try this PR locally:

bunx bun-pr 37808

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

bun-37808 --bun

[claude]

LGTM — focused fix that restores the exit_code.is_none() invariant has_finished and on_process_exit depend on.

What was reviewed:

• Confirmed ShellSubprocess::on_process_exit gates Cmd::on_exit on exit_code.is_none() (subproc.rs:987), so the old stash did suppress the real status.
• Verified name_and_args is &[Atom], so the if let [CmdSubst] slice pattern is equivalent to the old len() == 1 && matches!(...) check (the dropped new_idx == 1 was redundant given a single element).
• Checked cmd_subst_exit_code is only read on the empty-argv path and defaults to 0, so non-substitution empty commands and FOO=bar-only commands are unchanged.
• Tests cover piped / stdout-redirected / both-redirected (the hang), failing-subst-successful-cmd, || chaining, builtin, and the POSIX empty-expansion case.

Extended reasoning...

Overview

The PR fixes exit-code reporting for shell commands whose entire argv comes from a single $(...) substitution. Previously, Cmd::child_done stashed the substitution's own exit code into Cmd::exit_code so the empty-argv branch of transition_to_exec could return it (POSIX 2.9.1). But exit_code.is_some() is also the signal has_finished() and ShellSubprocess::on_process_exit() use to mean "the exec has already reported", so when the substitution did expand to an external command, the real process exit code was never recorded (or, with both stdout/stderr redirected to a file, nothing ever completed the command and the promise hung). The fix moves the stash into a new dedicated cmd_subst_exit_code field, read only in the empty-argv branch, leaving exit_code None until the builtin or subprocess actually reports.

Two files change: src/runtime/shell/states/Cmd.rs (+8/−23, mostly deleting a verbose comment block and simplifying to a slice pattern) and test/js/bun/shell/bunshell.test.ts (+44 lines, seven new TestBuilder cases in a new describe block).

Security risks

None. This is exit-code bookkeeping in the shell interpreter's Cmd state machine — no parsing of untrusted input, no new syscalls, no memory-ownership changes. The added struct field is a plain ExitCode (u16) with private visibility.

Level of scrutiny

Medium. The shell subprocess path involves careful state-machine sequencing and raw-pointer callbacks, but this change doesn't touch any of that machinery — it only moves where a stashed value lives and where it's read. I verified the invariant the PR description claims (on_process_exit at subproc.rs:987 checks cmd.exit_code.is_none() before calling on_exit), confirmed name_and_args is a &'arena [Atom] slice so the new if let [ast::Atom::Simple(ast::SimpleAtom::CmdSubst(_))] pattern is semantically identical to the old length+matches check, and checked that the dropped new_idx == 1 guard was redundant (with exactly one argv atom, ExpandingArgs completes exactly once). The new field defaults to 0 and is only read on the empty-argv path, so ordinary commands, assign-only commands, and non-substitution empty argv all still return 0 as before.

Other factors

The tests cover the full variant matrix the review guide asks for: the three stdio configurations (both piped, stdout-only redirected, both redirected — the hang case), the inverse case (failing substitution expanding to a successful command → 0), || chaining, a builtin (false), and the POSIX empty-expansion case ($(echo; exit 3) → 3) that pins the behaviour the fix must preserve. The author confirmed all seven fail on release/main without the fix and pass with it, and that the rest of bunshell.test.ts still passes. The comment-cop bot's feedback about long comments was addressed in efa8531 and the threads are resolved. The CI failures on the latest build (bake deinitialization segfault on Windows 2019, musl verify-baseline exit 56, agent-creation failure) are unrelated infrastructure/pre-existing issues — none touch the shell.