shell(exit): propagate last status on no-arg, fix negative/invalid arg codes#34903
shell(exit): propagate last status on no-arg, fix negative/invalid arg codes#34903robobun wants to merge 4 commits into
Conversation
|
Updated 6:00 AM PT - Jul 21st, 2026
❌ @robobun, your commit d8a9367 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34903That installs a local version of the PR into your bun-34903 --bun |
|
Reproduced on stock 1.4.0 with: bun -e 'import {$} from "bun"; $.nothrow(); console.log((await $`${{raw:"false; exit"}}`.quiet()).exitCode)'
# prints 0, should print 1Fix verified locally: CI (build #76822): |
WalkthroughChangesShell exit status behavior
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused POSIX-alignment fix for the exit builtin with good test coverage.
What was reviewed:
last_exit_codeonShellExecEnv: both construction sites (root init +dupe) set it, writes happen only inStmt::child_done/Binary::child_done, and duped envs inherit one-way as intended.- Async error-write path:
write_failing_errorstashes the code onBuiltin.exit_code, soon_io_writer_chunkreading it back with.unwrap_or(1)is correct. parse_decimal::<i64>handles leading-andn as u8→u16::fromgives the documented 255/212/44 wrapping.
Extended reasoning...
Overview
The PR fixes three POSIX/bash divergences in Bun shell's exit builtin: (1) no-arg exit now propagates the last command's status instead of hardcoding 0, (2) negative arguments parse as signed and wrap mod 256, and (3) the "numeric argument required" error returns 2 instead of 1. To support (1), a last_exit_code: ExitCode field is added to ShellExecEnv and updated at the two sequencing points (Stmt::child_done, Binary::child_done). Six files touched: ~25 lines of Rust across four files, plus 16 new test cases and one updated assertion in exec.test.ts.
Security risks
None. No untrusted-input parsing beyond what already existed (the arg was already parsed as an integer; only the target type changed from u64 to i64). No new syscalls, no allocation changes, no pointer/lifetime changes — the new field is a plain u16 copied by value.
Level of scrutiny
Moderate. Shell semantics are user-visible and the cmd; exit idiom is common in CI scripts, so correctness matters — but the change is small, mechanical, and well-aligned with a spec (POSIX) and reference implementation (bash). The state addition on ShellExecEnv is the same shape as the pending $? work in #33274, so the placement has already been vetted. I verified: ShellExecEnv is only constructed at the root and in dupe(), both updated; Builtin::write_failing_error stashes exit_code so the async on_io_writer_chunk path correctly recovers 2 vs 1; parse_int (behind parse_decimal) accepts a leading sign; i64 as u8 → u16::from yields the expected low-8-bit values for -1/-300/300.
Other factors
Test coverage is thorough for the change (;, ||, subshell, subprocess, newline-separated statements, explicit-arg-wins, both negative-wrap cases) and follows local conventions (TestBuilder, process.argv0 matching sibling files like rm.test.ts/ls.test.ts). The 1→2 change is reflected in the only other assertion on that message (exec.test.ts). PR body confirms USE_SYSTEM_BUN=1 fails 8 of the new cases and bunshell.test.ts passes unchanged. No outstanding reviewer comments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/js/bun/shell/commands/exit.test.ts`:
- Around line 22-30: Extend the “no argument propagates the last command’s
status” test in the exit command suite with cases covering command substitution
and pipeline-child execution. Assert the expected inherited exit codes and
retain the existing direct, compound, subshell, subprocess, statement, and
explicit-argument cases.
- Line 4: Replace the process.argv0-based executable selection in exit.test.ts
with the existing bunExe() helper for the nested -e subprocess, ensuring it
invokes the Bun binary under test.
🪄 Autofix (Beta)
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: cc802ae2-5841-4ded-b5c3-ebf83dfe4ff3
📒 Files selected for processing (6)
src/runtime/shell/builtin/exit.rssrc/runtime/shell/interpreter.rssrc/runtime/shell/states/Binary.rssrc/runtime/shell/states/Stmt.rstest/js/bun/shell/commands/exit.test.tstest/js/bun/shell/exec.test.ts
There was a problem hiding this comment.
LGTM — focused POSIX-compat fix to the exit builtin with thorough test coverage.
What was reviewed:
last_exit_codefield: plainu16onShellExecEnv, initialized at both construction sites (root env +dupe_for_subshell), written fromStmt/Binary::child_donevia the existingshell_mut()helper — no ownership/lifetime concerns.on_io_writer_chunknow reads back the code stashed byBuiltin::write_failing_error(verified at Builtin.rs:1132), so the async-stderr path returns 2 for non-numeric args as intended.i64 as u8truncation gives the documented wrap values (-1→255, -300→212, 300→44);ExitCodeisu16so::from(u8)is the standard widening impl.- All CodeRabbit threads resolved; the declined pipeline-child test case was withdrawn after verifying
dupe_for_subshellhas no per-kind branching.
Extended reasoning...
Overview
This PR fixes three bash-compat gaps in Bun shell's exit builtin: no-argument exit now propagates the last command's status (was hardcoded 0), negative arguments wrap mod 256 instead of failing to parse, and non-numeric arguments return 2 instead of 1. Runtime changes span four files (~30 lines): a new last_exit_code: u16 field on ShellExecEnv, two one-line writes in Stmt::child_done/Binary::child_done, and the reworked exit.rs builtin. Tests add 13 new cases and update two existing exit-code assertions from 1→2.
Security risks
None. The change touches no user-controlled parsing beyond what already existed (parse_decimal on the arg bytes), adds no allocations, and the new field is plain-data u16 with no ownership semantics.
Level of scrutiny
Medium-low. The shell interpreter state machine is touched, but only additively: two writes to a new field via an existing shell_mut() accessor, and a read in one builtin. No control-flow changes to Stmt/Binary. The struct initializer additions are exhaustive (Rust would fail to compile otherwise). The one intentional user-visible behavior change (exit code 1→2 for non-numeric args) is a bash-alignment fix and both existing assertions on the old value were updated in the same PR.
Other factors
- Test coverage is strong: 16 cases across statements,
&&/||, pipelines, subshells, subprocess exits, negative wraps, and explicit-arg-wins; the PR body confirms 8 fail on the current release andbunshell.test.ts(414 tests) passes unchanged. - I verified
Builtin::write_failing_errorstores the code on theBuiltinstruct so the asyncon_io_writer_chunkcompletion correctly recovers 2 vs 1 rather than the old hardcoded 1. - All review-bot threads are resolved; the one declined suggestion (extra pipeline-child inheritance test) was withdrawn by the reviewer after confirming
dupe_for_subshellhas a single unbranchedlast_exit_codecopy. - PR description explicitly coordinates with the two adjacent open PRs (#33274
$?, #33282 script-unwind) — no hidden merge conflict risk.
…g codes POSIX specifies that exit with no argument returns the status of the last command executed. Bun returned 0 unconditionally, so the standard 'cmd; exit' tail idiom always reported success to ShellPromise.exitCode. Also: - exit -1 now wraps to 255 (was: 'numeric argument required', exit 1). The argument is now parsed as a signed integer and the low 8 bits are kept, matching bash. - exit abc now returns 2 (was: 1), matching bash's 'numeric argument required' status. - exit 1 2 (too many arguments) still returns 1. Tracking for the last command's status lives on ShellExecEnv so it is scoped per subshell/command-substitution (inherited on dupe, never written back), and is updated by Stmt::child_done and Binary::child_done.
a4f73a6 to
058ab3c
Compare
There was a problem hiding this comment.
Beyond the inline nit, I checked the async stderr path: Builtin::write_failing_error stashes the code on Builtin.exit_code (Builtin.rs:1132), so on_io_writer_chunk's new .exit_code.unwrap_or(1) correctly returns 2 for the numeric-arg error even when stderr needs I/O. Also confirmed ExitCode is u16 and i64 as u8 gives the documented wrap (-300 → 212).
Extended reasoning...
The change is small and correct; the one finding is a pre-existing check-order edge that only became observable because this PR split the two error codes. Verified the non-obvious bit — that the async write-then-done path picks up the new per-call code rather than the old hardcoded 1 — by reading Builtin::write_failing_error which sets self.exit_code = Some(exit_code) before enqueueing. Not approving because the inline nit is part of the same bash-compat class this PR targets and a maintainer may want it folded in, and because the new ShellExecEnv.last_exit_code field overlaps with in-flight #33274.
bash validates the first argument before counting: `exit abc def` is 'numeric argument required' (2), not 'too many arguments' (1). The order was previously unobservable in the exit code because both paths returned 1; splitting them exposed it.
There was a problem hiding this comment.
LGTM — my earlier nit (exit abc def check order) is addressed in d8a9367.
What was reviewed:
last_exit_codefield: plainu16onShellExecEnv, initialized to 0, copied one-way indupe_for_subshell, written only inStmt/Binary::child_done— no lifetime or write-back concerns.on_io_writer_chunkreadsBuiltin.exit_code, whichBuiltin::write_failing_errorstashes before the async stderr enqueue, so the 2-vs-1 split survives the async path.i64 as u8truncation matches the -1→255 / -300→212 / 300→44 cases;parse_decimal::<i64>handles the leading sign.- Test matrix covers Stmt/Binary/pipeline/subshell/subprocess/negative/non-numeric/argc-order; fails-on-main evidence confirmed.
Extended reasoning...
Overview
Aligns the shell exit builtin with POSIX/bash: no-arg exit propagates the last command's status instead of hardcoding 0, negative arguments wrap mod 256 instead of failing to parse, "numeric argument required" now returns 2 instead of 1, and (per my earlier nit) the numeric check on arg[0] runs before the argc check. The state is a new last_exit_code: u16 on ShellExecEnv, updated by the two child_done sites and copied forward in dupe_for_subshell. ~43 net lines across 4 source files plus tests.
Security risks
None. No user-controlled allocation, no pointer/lifetime changes; the new field is a plain u16 on an existing struct. i64 as u8 truncation is well-defined in Rust and cannot panic or overflow.
Level of scrutiny
Medium — it touches shared interpreter state (ShellExecEnv), but the field is inert data with no ownership implications. I traced the read site (exit.rs → base.shell().last_exit_code), both write sites, both init sites, and confirmed Builtin::write_failing_error stashes exit_code on the Builtin before the async stderr path so on_io_writer_chunk's .exit_code.unwrap_or(1) picks up the correct 2 vs 1. The subshell/pipeline copy is one-way because dupe_for_subshell boxes a fresh env; child writes never reach the parent.
Other factors
Test coverage is thorough (16+ cases across the execution-variant matrix), the PR body includes fails-on-main / passes-on-PR evidence for both ASAN-debug and release, and all prior review threads (CodeRabbit's bunExe() and pipeline-coverage asks, my exit abc def ordering nit) are resolved. The exec.test.ts 1→2 update is the expected consequence of the error-code change. The author flagged the trivial merge with #33274 (which adds the same field for $?).
1 participant
Problem
POSIX specifies that
exitwith no argument returns the status of the last command executed. Bun returned 0 unconditionally, so the standardcmd; exittail idiom always reported success toShellPromise.exitCode, which is what CI/deploy wrappers branch on.Cause
exitwith no argument was hardcoded to0inbuiltin/exit.rs.u64, so negatives failed to parse and fell into the "numeric argument required" path.Fix
ShellExecEnv(last_exit_code), updated byStmt::child_doneandBinary::child_done. It is inherited when the env is duped for a subshell/command-substitution/pipeline child, so(exit 7); exitworks and the child never writes back.exitwith no argument readslast_exit_codeinstead of returning 0.i64and keep the low 8 bits:exit -1→ 255,exit -300→ 212,exit 300→ 44 (unchanged).Verification
test/js/bun/shell/commands/exit.test.tsgains coverage for all of the above (16 cases, 8 of which fail on the current release).bunshell.test.ts(414 tests) andexec.test.tspass unchanged apart from the expected 1→2 update forexit --help.Related: #33274 adds the same
last_exit_codefield onShellExecEnvto implement$?; whichever lands first, the other is a trivial merge. #33282 makesexitunwind the script, which is orthogonal.[review] gate passed · iteration 2 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file