shell: make exit end the script instead of just the current command by robobun · Pull Request #33282 · oven-sh/bun

robobun · 2026-07-02T21:41:49Z

Fixes #20368

What does this PR do?

Repro

import { $ } from "bun";

const r = await $`echo start; exit 5; echo never`.nothrow();
// bash: stdout "start\n", exit 5
// bun:  stdout "start\nnever\n", exit 0

exit did not stop the script, and the reported status came from whatever command happened to run last. exit mid-script is the standard early return in shell (cmd || exit 1 guards), so code ported into Bun.$ kept running past its guards, and the wrong status defeated .nothrow() exit-code checks.

Cause

The exit builtin completed only its own Cmd and returned the status to its parent, like any other builtin. Nothing told the enclosing Stmt/Script to stop. src/runtime/shell/builtin/exit.rs carried a TODO(port) about this ("bash exit should unwind the whole script... the Zig version sets a flag on the interpreter"), which #31783 reworded into "intentional divergence from bash" without implementing it.

Fix

Record the requested status on the ShellExecEnv that the exit builtin shares with its script. ShellExecEnv is duped for each subshell, command substitution, and pipeline element, which are the contexts bash forks, so the scoping falls out for free: exit ends the context that ran it and no more.

Three state nodes consume the flag:

Stmt::next refuses to run a command once it is set. Every command in a script body or an if arm is spawned as a Stmt, so this one check stops both.
Binary::next checks before its short-circuit, so exit 0 && echo hi stops even though the left side succeeded.
If::next checks because a pipeline spawns an if-clause straight into its own env, with no Stmt in between to carry the status out. A failed condition with no else arm reports a hardcoded Done(0), which would otherwise swallow it (echo hi | if exit 5; then echo t; fi).

Script needs no check of its own: it only ever spawns Stmts, which refuse. Confirmed by ablation, where removing any of the three above breaks tests and adding a fourth in Script::child_done breaks none.

Matching bash, a bad argument (exit abc, exit 1 2) also ends the script, with the status it already used.

How did you verify your code works?

Differential run against bash, 19 exit scenarios: 8/19 matched before, 17/19 after.

The two that still differ are separate missing features, not this bug:

false; exit (bare exit should reuse $?) and echo $? need $?, which does not exist in Bun's shell yet. shell: implement $?, $$, $#, and ${NAME}; reject unsupported expansions #33274 implements it; once both land, bare exit can read last_exit_code and that case closes too.
{ ...; } brace grouping is unimplemented (command not found: {) and is already marked .todo in the suite.

Before / after vs bash

                                              before             after              bash
echo start; exit 5; echo never             0 "start\nnever\n"  5 "start\n"        5 "start\n"
exit 0 && echo x                           0 "x\n"             0 ""               0 ""
false || exit 3; echo never                0 "never\n"         3 ""               3 ""
if true; then exit 7; fi; echo never       0 "never\n"         7 ""               7 ""
if exit 5; then echo t; else echo f; fi    0 "f\n"             5 ""               5 ""
exit 1; exit 2                             2 ""                1 ""               1 ""
echo a; exit; echo b                       0 "a\nb\n"          0 "a\n"            0 "a\n"
(exit 6; echo inner); echo after           0 "inner\nafter\n"  0 "after\n"        0 "after\n"
echo cs=$(exit 4; echo inner); echo after  0 "cs=inner\n..."   0 "cs=\nafter\n"   0 "cs=\nafter\n"
echo a; exit 5 | cat; echo b               0 "a\nb\n"          0 "a\nb\n"         0 "a\nb\n"

22 tests added to test/js/bun/shell/commands/exit.test.ts: the statements after exit are skipped, &&/||/if-body/if-condition all unwind, argument errors still end the script, and the negative contract that a subshell, command substitution, and pipeline element each contain their own exit. 19 of them fail on main.

test/js/bun/shell/bunshell.test.ts had the upstream effect of subshell case weakened to assert the buggy output:

# (a=2; echo $a; exit; echo not reached)
# NOTE: We actually implemented exit wrong so changing this for now until we fix it
(a=2; echo $a; exit; echo reached)

Restored to its original form, asserting 2\n1\n.

Whole test/js/bun/shell/ suite: no new failures (the pre-existing fd-leak timeouts and root-only ls permission cases are unchanged).

The exit builtin completed only its own Cmd, so the statements after it kept running and the reported status came from whatever ran last: await $`echo start; exit 5; echo never` // printed "never", exited 0 Record the requested status on the ShellExecEnv that the exit builtin shares with its script. Stmt refuses to run a command once it is set, which unwinds the script body and any enclosing if branch, and Binary checks it before its short-circuit so `exit 0 && echo hi` stops too. A subshell, command substitution, and pipeline element each dupe the env, so exit stays inside the context that ran it, as in bash. Restores the upstream form of the "effect of subshell" test, which had been weakened with a note saying exit was implemented wrong.

coderabbitai · 2026-07-02T21:41:56Z

Walkthrough

Adds an exit_requested field to ShellExecEnv, sets it from the exit builtin, and checks it in Base, Binary, If, and Stmt so exit stops only the current execution context. Tests were updated for script termination and scoped exit behavior.

Changes

Scoped exit builtin behavior

Layer / File(s)	Summary
ShellExecEnv exit_requested field and initialization `src/runtime/shell/interpreter.rs`	Adds public `exit_requested: Option<ExitCode>` field, initialized to `None` on root env creation and on subshell/pipeline duplication.
exit builtin sets exit_requested `src/runtime/shell/builtin/exit.rs`	`start` and `fail` now call a new `request_exit` helper to set `exit_requested` on the enclosing `ShellExecEnv`.
Base accessor and state short-circuit checks `src/runtime/shell/states/Base.rs`, `src/runtime/shell/states/Binary.rs`, `src/runtime/shell/states/If.rs`, `src/runtime/shell/states/Stmt.rs`	Adds `Base::exit_requested()` accessor; `Binary::next`, `If::next`, and `Stmt::next` check it and complete early via `child_done` instead of continuing evaluation.
Test coverage for scoped exit behavior `test/js/bun/shell/bunshell.test.ts`, `test/js/bun/shell/commands/exit.test.ts`	Updates subshell output expectations and adds cases covering `exit` ending the current script flow versus remaining confined to subshells, command substitutions, pipelines, and `if` bodies.

Related issues: #20368

Related PRs: None identified.

Suggested labels: shell, bug, rust

Suggested reviewers: Maintainers of src/runtime/shell

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address `#20368` by making exit terminate the script and adding tests for the reported failure cases.
Out of Scope Changes check	✅ Passed	The extra control-flow and test updates support the exit fix and its shell scoping behavior, with no obvious unrelated changes.
Title check	✅ Passed	The title clearly summarizes the main change: making shell exit stop the script instead of only the current command.
Description check	✅ Passed	The description matches the template and includes both the change summary and concrete verification details.

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

robobun · 2026-07-02T21:42:00Z

^{Updated 5:06 PM PT - Jul 2nd, 2026}

✅ @robobun, your commit 7fd550aff871e59aed0855695ce754b9e4def1de passed in Build #68090! 🎉

🧪 To try this PR locally:

bunx bun-pr 33282

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

bun-33282 --bun

github-actions · 2026-07-02T21:44:49Z

Found 1 issue this PR may fix:

Bun shell ignores 'exit' command #20368 - Bun shell ignores exit command: subsequent commands continue executing after exit, which this PR fixes by setting exit_requested on ShellExecEnv and checking it in Stmt and Binary state nodes

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #20368

🤖 Generated with Claude Code

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/runtime/shell/interpreter.rs (1)
1786-1801: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Give async jobs their own shell env
Async::init stores the parent ShellExecEnv directly, so exit inside cmd & writes the shared exit_requested flag and can terminate the foreground script. Async nodes need an owned/isolated env, or an equivalent scoped exit flag.
🤖 Prompt for 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.

In `@src/runtime/shell/interpreter.rs` around lines 1786 - 1801, The async
execution path is reusing the parent ShellExecEnv directly, so Async::init needs
to stop sharing state that should be scoped per async job. Update the
Async::init flow and related ShellExecEnv handling so cmd & gets its own owned
ShellExecEnv instance (or an equivalent isolated exit flag), and ensure
exit_requested in ShellExecEnv is not shared back to the foreground script. Use
the ShellExecEnv and Async::init symbols to locate the fix.

🤖 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/states/Stmt.rs`:
- Around line 61-66: Gate the If state transition on exit_requested just like
Stmt does, so an early exit from the condition unwinds instead of advancing into
Then/Else. Update the dispatch/advance logic in If and the related
state-handling path in If.rs so it checks
interp.as_stmt(this).base.exit_requested() before spawning either branch, and
returns interp.child_done(parent, this, code) immediately when set.

---

Outside diff comments:
In `@src/runtime/shell/interpreter.rs`:
- Around line 1786-1801: The async execution path is reusing the parent
ShellExecEnv directly, so Async::init needs to stop sharing state that should be
scoped per async job. Update the Async::init flow and related ShellExecEnv
handling so cmd & gets its own owned ShellExecEnv instance (or an equivalent
isolated exit flag), and ensure exit_requested in ShellExecEnv is not shared
back to the foreground script. Use the ShellExecEnv and Async::init symbols to
locate the fix.

🪄 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: 18b87fbb-7eb7-433a-a7ef-7471fd1aa489

📥 Commits

Reviewing files that changed from the base of the PR and between 1498d7b and 9697533.

📒 Files selected for processing (7)

src/runtime/shell/builtin/exit.rs
src/runtime/shell/interpreter.rs
src/runtime/shell/states/Base.rs
src/runtime/shell/states/Binary.rs
src/runtime/shell/states/Stmt.rs
test/js/bun/shell/bunshell.test.ts
test/js/bun/shell/commands/exit.test.ts

coderabbitai

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 `@test/js/bun/shell/commands/exit.test.ts`:
- Around line 29-39: The bare exit test in exit.test.ts only covers the
successful path, so add a separate case using the same TestBuilder.command shell
script pattern that runs a failing command like false before a bare exit and
asserts the resulting nonzero exit code. Keep the existing a bare exit on its
own line test, and add the new scenario near it so the behavior of exit
inheriting the previous command’s status is explicitly pinned.

🪄 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: 2fc9cf41-7a85-4aef-89c3-bd4799a9e947

📥 Commits

Reviewing files that changed from the base of the PR and between 9697533 and 0140375.

📒 Files selected for processing (1)

test/js/bun/shell/commands/exit.test.ts

Adds multi-statement if arms and an elif condition to prove no command in any arm runs after exit, and marks the bare-exit-reuses-$? case todo since the shell does not track the last command's status yet.

A pipeline spawns an if-clause straight into its own duped env, with no Stmt in between to report the status. When the condition fails and there is no else arm, If::next returns a hardcoded Done(0), so the status from `exit` was dropped: echo hi | if exit 5; then echo t; fi // exited 0, bash exits 5 Check exit_requested in If::next as well. Stmt's guard already stopped any command in the arms from running, so only the status was wrong.

robobun · 2026-07-02T22:43:23Z

Thanks, the If finding is a real bug and I've fixed it in 7fd550a. Details on all three points below.

`If` needs its own check (confirmed, fixed)

This was right, and my reasoning for removing the If guard was wrong. I had concluded it was redundant because Stmt::next already refuses to run anything in the arms, and an ablation backed that up. But the ablation ran against a test suite that had no if-as-pipeline-element case, so it proved nothing about that shape.

Execution suppression was indeed fine. The status was not. A pipeline spawns an If straight into its own duped env with no Stmt in between, and the Action::Done(0) arm for a failed condition with no else (If.rs:112, and the elif path at :142) hardcodes 0. Pipeline::child_done records that verbatim and frees the duped env without ever reading exit_requested:

                                                              before    bash
echo hi | if exit 5; then echo t; fi                             0        5
echo hi | if false; then echo x; elif exit 5; then echo y; fi    0        5

With an else arm it happened to work, because the else Stmt refuses and reports 5, which Action::Done(exec.last_exit_code) then passes through. That is what masked it.

Fixed by checking exit_requested at the top of If::next. Both shapes now exit 5, and both are pinned as tests in the "stays inside its execution context" group.

Re-ran the ablation with those tests present, and all three guards now earn their place:

guard removed	tests broken
`Stmt::next`	12
`Binary::next`	2 (`exit 0 && ...`, `exit 5 \|\| ...`)
`If::next`	2 (the two pipeline shapes above)

Adding a fourth guard in Script::child_done still breaks nothing, so I've left that one out.

`exit` inside `cmd &` (not reachable)

Expr::Async is never constructed: the parser rejects & before any Async node exists, so Async::init is unreachable and exit has no path to it.

src/shell_parser/parse.rs:1075
    if self.r#match(TokenTag::Ampersand) {
        self.add_error(format_args!("Background commands \"&\" are not supported yet."))?;
        return Err(ParseError::Unsupported.into());
    }

Every form, including the exact repro:

exit 5 & sleep 0.01; echo b    rejected: Background commands "&" are not supported yet.
exit 5 &                       rejected: Background commands "&" are not supported yet.
if true; then exit 5; fi &     rejected: Background commands "&" are not supported yet.
echo a& echo b                 rejected: Background commands "&" are not supported yet.

The only mentions of Expr::Async in the tree are pattern matches (memory_cost, json_fmt, spawn_expr, and a panic! arm in Async::next). You're right that the invariant as I worded it is too strong, though, so I've reworded it to say Subshell/Pipeline/command substitution rather than implying the set is closed. Worth flagging for whoever implements &: bash forks for background jobs, so Async will need to dupe the env like the others, which would scope exit_requested correctly for free. I'd rather not change unreachable code here, since no test can reach it.

Nonzero status for bare `exit`

Real gap, and the one thing from the original report I could not close. Bare exit should report the last command's status, but the shell tracks no last-exit-status at all ($? expands literally today), so false; exit exits 0 instead of 1.

Pinned as a .todo with the bash-correct expectation in 9661670:

TestBuilder.command`false; exit`
  .exitCode(1)
  .todo("the shell does not track the last command's status yet")
  .runAsTest("a bare exit reports the last command's status");

#33274 adds last_exit_code to ShellExecEnv for $?. Once that lands, bare exit can read it and this can be un-skipped.

github-actions Bot added the claude label Jul 2, 2026

[autofix.ci] apply automated fixes

9697533

coderabbitai Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread src/runtime/shell/states/Stmt.rs

test: cover the bare-exit repro from issue #20368

0140375

coderabbitai Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread test/js/bun/shell/commands/exit.test.ts

test: cover if-arm skipping and the bare-exit status gap

9661670

Adds multi-statement if arms and an elif condition to prove no command in any arm runs after exit, and marks the bare-exit-reuses-$? case todo since the shell does not track the last command's status yet.

claude Bot reviewed Jul 2, 2026

View reviewed changes

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

Comment thread src/runtime/shell/states/Stmt.rs

robobun mentioned this pull request Jul 21, 2026

shell(exit): propagate last status on no-arg, fix negative/invalid arg codes #34903

Open

Navigation Menu

Uh oh!

shell: make exit end the script instead of just the current command - #33282

shell: make exit end the script instead of just the current command#33282
robobun wants to merge 5 commits into
mainfrom
farm/a3150fda/shell-exit-ends-script

robobun commented Jul 2, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 2, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robobun commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robobun commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Repro

Cause

Fix

How did you verify your code works?

Uh oh!

coderabbitai Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

robobun commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 2, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robobun commented Jul 2, 2026

If needs its own check (confirmed, fixed)

exit inside cmd & (not reachable)

Nonzero status for bare exit

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jul 2, 2026 •

edited

Loading

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

robobun commented Jul 2, 2026 •

edited

Loading

`If` needs its own check (confirmed, fixed)

`exit` inside `cmd &` (not reachable)

Nonzero status for bare `exit`