shell: exit with the positive errno when a builtin's output write fails by robobun · Pull Request #40702 · oven-sh/bun

robobun · 2026-08-28T05:14:16Z

Problem

A builtin whose output write fails reports a garbage exit code. echo hi > /dev/full and which sh > /dev/full exit 65508 (ShellError: Failed with exit code 65508). export > /dev/full and cd /nonexistent 2> /dev/full exit 1 and drop the errno.
echo.rs:116 and which.rs:182 cast SystemError.errno to the u16 exit code. That field holds the negated errno (fill_system_error_common, src/sys/Error.rs:371), so ENOSPC (-28) wraps to 65508. export.rs:85 and cd.rs:127 hard-code 1.

Fix

The four builtins read the errno through SystemError::get_errno(), which undoes the negation. cat and rm already do this, and the Zig builtins did too (getErrno()). cd keeps exit 1 when the stderr message is written without error.
mv reads a failed rename's errno through bun_sys::Error::get_errno() instead of the raw field. That field is already positive, so the value does not change on POSIX.
Excluded on purpose: pwd, basename, dirname, seq and yes keep exit 1 on a failed write. They did that before the rewrite too. shell: exit 1 and report a failed output write in every builtin #40033 proposes one status for every builtin.
Verified: test/js/bun/shell/bunshell.test.ts (new test, stock bun reports 65508/65508/1/1). Also the rest of bunshell.test.ts, commands/{echo,which,mv}.test.ts, epipe.test.ts, shell-pipe-read-fault.test.ts, shell-write-fault.test.ts.

Background

A builtin that writes to a real fd (a file redirect, or the process stdout) goes through IOWriter, the shell's per-fd write queue. The result arrives later in the builtin's on_io_writer_chunk as an Option<bun_sys::SystemError>.
bun_sys::SystemError is the JS-facing error shape. Its errno is stored negated to match Node's err.errno. get_errno() returns the positive E value.
bun_sys::Error is the syscall-level error. Its errno is the positive u16.

Notes

Values from the repro with /dev/full as the redirect target (Linux, ENOSPC = 28):

command	1.4.1	now
`echo hi > /dev/full`	65508	28
`which sh > /dev/full`	65508	28
`export FOO=bar; export > /dev/full`	1	28
`cd /nonexistent 2> /dev/full`	1	28
`cd /nonexistent` (stderr ok)	1	1
`mv nosuch dst`	2	2
`pwd > /dev/full`	1	1
`ls / > /dev/full`	0	0

On Windows SystemError.errno is the libuv code (UV_ENOSPC = -4075), so the raw cast gave a different garbage value there. get_errno() canonicalizes it to the E discriminant, so the exit code is 28 on every platform.

The test is Linux only because /dev/full does not exist on macOS and Windows.

The excluded sites, with the pre-rewrite Zig behavior (commit 23427dbc12^, src/shell/builtin/*.zig):

pwd.zig, basename.zig, dirname.zig, seq.zig, yes.zig: onIOWriterChunk returned done(1) on a write error. The Rust port (pwd.rs:72, basename.rs:62, dirname.rs:64, seq.rs:209, yes.rs:166) does the same.
ls.zig: onIOWriterChunk dropped the error. ls.rs:182 does the same, so ls / > /dev/full exits 0.
echo.zig, which.zig, export.zig, cd.zig: onIOWriterChunk returned done(e.getErrno()). The Rust port changed these four, and this PR restores them.

#40033 and #32278 are open PRs that change the same sites to exit 1 (coreutils style) for every failed write. This PR keeps the errno as the exit code, which is what the other builtins report and what the Zig implementation did.

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

echo and which cast SystemError.errno to the exit code. That field holds the negated errno, so ENOSPC (28) came out as 65508. export and cd ignored the write error and exited 1. All four now read the errno through SystemError::get_errno(), like cat and rm. mv reads the errno of a failed rename through bun_sys::Error::get_errno() for the same reason, which does not change its value on POSIX.

robobun · 2026-08-28T05:14:24Z

Reproduced on bun 1.4.1 with /dev/full as the redirect target:

echo hi > /dev/full                  -> 65508 (expected 28)
which sh > /dev/full                 -> 65508 (expected 28)
export FOO=bar; export > /dev/full   -> 1     (expected 28)
cd /nonexistent 2> /dev/full         -> 1     (expected 28)

With this branch all four exit 28. Test: test/js/bun/shell/bunshell.test.ts ("builtins exit with the errno of a failed output write").

CI: the shell tests pass on every lane. The two red jobs in the latest run are unrelated to this change and are reported separately: test/js/web/url/url.test.ts on darwin x64 (an IDNA table assertion, also red on main) and test/cli/run/run_command.test.ts on ubuntu 25.04 aarch64 (bun run exits with SIGABRT instead of SIGINT after a Ctrl+C). Ready for review.

Superseded by #32278, which fixes the same sites with exit 1 instead of the positive errno. See the closing comment below for the reasoning.

coderabbitai · 2026-08-28T05:14:24Z

Warning

Review limit reached

Run on-demand review

_{On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file.}

Or wait 3 minutes for your next included review.

View limit details

Limit details: You’ve used all 5 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 97c87c47-0728-4a65-856e-de83f426ee06

📥 Commits

Reviewing files that changed from the base of the PR and between a92d84e and 8cef172.

📒 Files selected for processing (6)

src/runtime/shell/builtin/cd.rs
src/runtime/shell/builtin/echo.rs
src/runtime/shell/builtin/export.rs
src/runtime/shell/builtin/mv.rs
src/runtime/shell/builtin/which.rs
test/js/bun/shell/bunshell.test.ts

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

robobun · 2026-08-28T05:14:36Z

^{Updated 11:29 PM PT - Aug 27th, 2026}

❌ @robobun, your commit 8cef172 has 2 failures in Build #107374 (All Failures):

test/js/web/url/url.test.ts - code 1: TypeError: Invalid URL on 🍎 any x64
test/cli/run/run_command.test.ts - pid 6501 sigabrt on 🐧 25.04 aarch64

🧪 To try this PR locally:

bunx bun-pr 40702

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

bun-40702 --bun

robobun · 2026-08-28T05:28:09Z

On the review note about pwd, basename, dirname, seq and yes: those five are excluded on purpose. They exited 1 on a failed write before the Rust rewrite too, so they are not part of the regression this PR fixes. echo, which, export and cd reported the errno in Zig, and the port changed that. The PR body now lists the excluded sites with the pre-rewrite behavior of each.

claude

LGTM — thanks for adding the exclusion rationale for pwd/basename/dirname/seq/yes; restoring only the four that regressed in the Zig→Rust port is a coherent scope.

Checked that SystemError::get_errno() and bun_sys::Error::get_errno() both return the positive E discriminant (Windows libuv codes canonicalized), so the as ExitCode cast is well-defined everywhere it's used. The cd fallback of 1 when the stderr write succeeds preserves the prior "chdir failed" contract. Test is Linux-gated for /dev/full and asserts against the pre-fix 65508/1 values, so it fails on stock bun.

Extended reasoning...

Overview

Five one-line changes in shell builtins (cd, echo, export, mv, which) swap raw e.errno field reads or hardcoded 1 for e.get_errno() as ExitCode in the write-failure branch of on_io_writer_chunk, matching what cat/rm already do and what the pre-rewrite Zig implementation did. One Linux-only test in bunshell.test.ts redirects each to /dev/full and asserts all exit 28 (ENOSPC).

Security risks

None. This only changes the numeric exit code reported after a write to stdout/stderr fails; no new input parsing, no allocation, no privilege or filesystem operations.

Level of scrutiny

Low. The change is mechanical and follows the established cat.rs/rm.rs pattern. I verified both get_errno() implementations (src/sys/Error.rs:165 for bun_sys::Error, src/sys/lib.rs:53 for SystemError) return the positive E discriminant with a checked constructor, so the as ExitCode cast cannot wrap negative on any platform. The mv.rs site operates on bun_sys::Error (already-positive errno), so behavior is unchanged there — it's a consistency edit.

Other factors

My earlier inline comment asked why five sibling builtins with the same hardcoded-1 shape were excluded. The PR description now names all five and explains they exited 1 in the original Zig too — this PR only restores the four that regressed during the port, deferring the broader "one status for every builtin" question to the referenced open PR. That satisfies REVIEW.md's "if a site is intentionally excluded, say so in the PR." No code changed since my prior review (the second commit is an empty CI retrigger). No CODEOWNERS cover these paths. The test uses a single .toEqual on an object per test/CLAUDE.md and would report {echo: 65508, which: 65508, export: 1, cd: 1} on the unfixed build.

robobun · 2026-08-28T19:23:14Z

Closing in favor of #32278, which fixes the same exit code cast in echo, which and mv, and the same class of site in cat, rm, CondExpr.rs and the Cmd.rs output relay. That PR uses exit 1 for a failing builtin.

Why exit 1 and not the positive errno:

bash, dash and zsh exit 1 when a builtin's write fails (echo hi > /dev/full prints bash: echo: write error: No space left on device and sets $? to 1). coreutils mv, cat and rm exit 1 on an operational error. Bun Shell is a bash-like shell whose builtins stand in for those programs, so a script that branches on $? sees the same values.
An errno is not a portable exit code. The number differs by platform, and values above 125 collide with the shell's own 126, 127 and 128+n.
The pre-Rust Zig builtins did not agree with each other, so there is no single old behavior to restore: echo, which, export, cd and cat returned the errno, basename, dirname, seq, yes and mkdir returned 1, pwd and ls ignored the write error. With this PR, cd /nonexistent would also exit 1 or 28 depending on where stderr points.

The export > /dev/full and cd /nonexistent 2> /dev/full cases from the test here are carried over to #32278 as exit 1 assertions.

robobun assigned dylan-conway Aug 28, 2026

github-actions Bot added the claude label Aug 28, 2026

claude Bot reviewed Aug 28, 2026

View reviewed changes

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

ci: retrigger

8cef172

claude Bot reviewed Aug 28, 2026

View reviewed changes

This was referenced Aug 28, 2026

shell: exit 1 and report a failed output write in every builtin #40033

Open

shell: export rejects invalid identifiers #40710

Open

shell: exit 1 on builtin failure instead of raw errno; fix cd ENOENT message #32278

Open

robobun closed this Aug 28, 2026

Navigation Menu

Repository navigation

shell: exit with the positive errno when a builtin's output write fails - #40702

shell: exit with the positive errno when a builtin's output write fails#40702
robobun wants to merge 2 commits into
mainfrom
farm/1e0de1e9/shell-builtin-write-error-exit-code

robobun commented Aug 28, 2026 •

edited

Loading

Uh oh!

robobun commented Aug 28, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading

Review limit reached

Uh oh!

robobun commented Aug 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

robobun commented Aug 28, 2026

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Problem

Fix

Background

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Review limit reached

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

robobun commented Aug 28, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Aug 28, 2026 •

edited

Loading

robobun commented Aug 28, 2026 •

edited

Loading

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading

robobun commented Aug 28, 2026 •

edited

Loading