shell: fold write_no_io ENOSPC into builtin exit codes - #34699
Closed
robobun wants to merge 2 commits into
Closed
Claude / Claude Code Review
completed
Jul 19, 2026 in 18m 25s
Code review found 1 important issue
Found 3 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/shell/builtin/cat.rs:150-151 |
cat streaming path still discards ENOSPC on ArrayBuffer stdout |
| 🟡 Nit | test/js/bun/shell/bunshell.test.ts:479-485 |
Test does not cover which/export/cat — 3 of 8 fixed sites untested |
Annotations
Check failure on line 151 in src/runtime/shell/builtin/cat.rs
claude / Claude Code Review
cat streaming path still discards ENOSPC on ArrayBuffer stdout
The stdin-sync path here is fixed, but `on_io_reader_chunk` (the streaming path — hit by `cat file.txt > ${arraybuf}`, or `cat > ${arraybuf}` with a real-fd stdin) still ends with `let _ = Builtin::write_no_io(...); Yield::done()`. When the ArrayBuffer fills, ENOSPC is dropped, `on_io_reader_done` fires with `err=None`, and cat exits 0 with truncated output — the same bug this PR fixes elsewhere. cat isn't covered by the stated exclusion rationale (its primary output *is* stdout) and this path i
Check warning on line 485 in test/js/bun/shell/bunshell.test.ts
claude / Claude Code Review
Test does not cover which/export/cat — 3 of 8 fixed sites untested
The test.each table covers echo/seq/basename/dirname/pwd but not the other three sites this PR modifies: `export` (print-all path), `which` (sync loop), and `cat` (stdin-sync). `which` in particular uses bespoke `had_write_err` flag logic rather than the shared `write_no_io_exit` helper, so reverting that hunk would break no test. `export` and `which` are cheap to add as extra rows (`export` needs a big-buffer sized from the actual env; `which` needs a resolvable command like `bunExe()` so `had_
Loading