Repository navigation
Conversation
|
Updated 4:07 AM PT - Jul 29th, 2026
❌ @robobun, your commit fb59879 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35352That installs a local version of the PR into your bun-35352 --bun |
WalkthroughChangesSpawn path redirection
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
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/spawn/spawn.test.ts`:
- Around line 89-100: Update the spawnSync test cases around the shown
truncation test and the additionally referenced cases to capture each
invocation’s result and assert a successful exitCode, including relevant
diagnostics when available. Ensure every spawned child’s completion status is
validated rather than only checking the produced file contents.
🪄 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: a82c0b35-fe4d-485b-91b8-0acde3900abc
📒 Files selected for processing (3)
src/spawn/process.rssrc/spawn_sys/spawn_process.rstest/js/bun/spawn/spawn.test.ts
There was a problem hiding this comment.
LGTM — adds O_TRUNC to the stdout/stderr path-redirect open on both POSIX and Windows, matching Bun.write, shell >, and fs.openSync(path, 'w').
What was reviewed:
- Both spawn paths gate on index (
i != 0/fd_i != 0) so stdin staysO_RDONLYuntouched; extra stdio slots (index ≥ 3) keepO_RDWR|O_CREATwithout truncation on both platforms. - The
/dev/nullopen (Ignore/Ipc) is a separate arm and unaffected. - Existing "Bun.file() works as stdin and stdout" test still holds (previously passed only because "hello!" and "wrong!" were the same length).
Extended reasoning...
Overview
Two-line native change: src/spawn_sys/spawn_process.rs (POSIX posix_spawn file-actions) and src/spawn/process.rs (Windows uv_fs_open) each add O_TRUNC to the open flags for Stdio::Path when the slot is stdout or stderr. Three new tests in test/js/bun/spawn/spawn.test.ts (spawnSync stdout, spawn stdout, spawn stderr) pre-fill the target with a longer string and assert exact contents plus exitCode === 0.
Security risks
None. The change adds a standard open flag to an existing O_WRONLY|O_CREAT open of a caller-supplied path. No new syscalls, no new user-controlled data, no memory management changes.
Level of scrutiny
Low-to-moderate. The mechanical change is trivial (one flag OR'd in, gated on index), and both platform arms were updated symmetrically. The only judgment call is that this is a user-visible default change — but the prior behavior (overwrite from offset 0 without truncating, leaving a stale tail) is not a semantic anyone would deliberately rely on; it's the classic missing-O_TRUNC bug. Every comparable Bun/Node/shell surface truncates. Users wanting append can already pass an fd opened with O_APPEND.
Other factors
- Verified stdin path is unaffected (
i == 0→trunc = 0, andflagis alreadyO_RDONLYthere soO_TRUNCwould be meaningless anyway). - Verified the extra-fds loop on both platforms was left untouched (
O_RDWR|O_CREAT, no truncation) — consistent with the PR description and the existing "Bun.file(path) at index >= 3 is readable in the child" test. - CodeRabbit's minor feedback (assert exitCode) was addressed in c05c61a and the thread is resolved.
- Tests follow harness conventions (
bunExe(),bunEnv,tmpdirSync(), content assertion before exitCode).
|
Status: closed, superseded by #46665. That PR applies the same truncating open to |
Passing Bun.file(path) as stdout or stderr to Bun.spawn/spawnSync opened the destination with O_WRONLY|O_CREAT but without O_TRUNC. If the file already existed with longer content than the child wrote, the stale tail was left in place, silently corrupting the output. This matches the truncate-on-write behaviour of Bun.write(path, ...), the shell's > redirect, and fs.openSync(path, 'w'). stdin and stdio[i>=3] are unchanged: stdin is opened read-only, and extra slots are O_RDWR with the documented use case of reading in the child.
c05c61a to
187d7a9
Compare
|
Rebased onto main @ 59242d6 (includes e532ad9). Re-verified locally: All three truncation tests fail on the released bun and pass on the debug build. |
There was a problem hiding this comment.
LGTM — adds O_TRUNC to the stdout/stderr path-redirect open on both the posix_spawn and Windows libuv codepaths, matching > shell semantics and Bun.write.
What was reviewed:
- Confirmed
i == 0/fd_i == 0guards keep stdinO_RDONLYwithoutO_TRUNCon both platforms; extra stdio slots (index ≥ 3) stayO_RDWR|O_CREATuntouched. - The
/dev/nullopen forIgnore/Ipcis a separate arm and unchanged. - New tests pre-fill with a longer string, assert exact content then
exitCode === 0, and follow the file's existingtmpdirSync/gcTickconventions. CI red is pre-existing on main and unrelated to this codepath.
Extended reasoning...
Overview
Two-line change on each of the two spawn backends: when stdout/stderr is a Bun.file(path), add O_TRUNC (POSIX) / uv::O::TRUNC (Windows libuv) to the open flags so the destination file is truncated before the child writes. Stdin (i == 0) is explicitly excluded and stays O_RDONLY|O_CREAT. Extra stdio slots at index ≥ 3 use a separate loop that opens O_RDWR|O_CREAT and is deliberately left untouched (the established use case there is reading in the child, covered by an existing passing test). Three new tests cover spawn stdout, spawn stderr, and spawnSync stdout.
Security risks
None. Adding O_TRUNC to a O_WRONLY|O_CREAT open does not change what the parent can access; it only zeroes the file length at open time. No new path handling, no permission changes, no untrusted input parsing.
Level of scrutiny
Low-to-medium. The mechanical change is tiny and obviously correct (O_WRONLY|O_CREAT|O_TRUNC is the canonical "write to file" mode used by fopen("w"), shell >, and Bun.write). The only judgment call is whether truncation is the right default — and leaving stale bytes from a previous run in a redirected output file is clearly a bug, not a feature. Every comparable Bun surface (Bun.write, Bun.$, fs.openSync('w')) already truncates.
Other factors
- Both platform backends fixed symmetrically; verified the Windows
flagcomputation mirrors POSIX (fd_i == 0 → RDONLY, elseWRONLY), so thefd_i == 0guard is correct there too. - CodeRabbit's feedback (assert exit codes) was addressed; thread resolved.
- CI failures are pre-existing on main at the base commit and hit the
stdout: "pipe"path, not theBun.file(path)arm this PR touches. - Tests follow the surrounding file's conventions (
tmpdirSync,gcTick, content-then-exitCode assertion order) rather than introducing new patterns.
There was a problem hiding this comment.
No bugs found. The two-line native change (OR'ing O_TRUNC into the open flags for stdout/stderr on both the posix_spawn and libuv paths) is mechanically correct and well-tested, but it changes the user-observable semantics of Bun.spawn({ stdout: Bun.file(path) }) on existing files. Worth a maintainer confirming the intent — in particular the deliberate choice to leave extra stdio slots (index ≥ 3) as O_RDWR|O_CREAT without truncation.
What was reviewed
- POSIX path:
O::TRUNCadded only fori != 0in the 0..3 stdio loop; stdin staysO_RDONLY, extra_fds loop untouched (stillO_RDWR|O_CREAT). - Windows path: same guard on
fd_i != 0in the 0..3 loop; extra_fdsPatharm at line ~2025 unchanged, matching POSIX. - Tests cover spawn/spawnSync × stdout/stderr, assert exact content and
exitCode === 0; existing stdin and index-≥-3 read tests still pass per author.
Extended reasoning...
Overview
Adds O_TRUNC when opening a path-backed Bun.file() as stdout or stderr in Bun.spawn/spawnSync. Two one-line changes: src/spawn_sys/spawn_process.rs (posix_spawn actions) and src/spawn/process.rs (Windows uv_fs_open). Three new tests in test/js/bun/spawn/spawn.test.ts pre-fill a file with a longer string and assert the exact short child output afterward.
Security risks
None. The change only adds a truncate flag to a file the caller explicitly named as an output destination. No new inputs, no path handling changes, no privilege changes.
Level of scrutiny
Medium. The native diff is trivially correct — i == 0 / fd_i == 0 correctly identifies stdin inside the 0..3 loop on both platforms, and O_TRUNC on an O_RDONLY open would be a no-op anyway. However, this is a user-visible behavioral change to a public Bun API. The old behavior (overwrite-from-start without truncate) was clearly a bug and the PR makes a solid case for consistency with Bun.write, Bun.$, and shell >, but there's no linked issue and the author made a judgment call to leave index-≥-3 Bun.file(path) slots untruncated (O_RDWR|O_CREAT) on the grounds that the established use case is reading. That's reasonable, but it's the kind of API-surface decision a maintainer should confirm rather than have land silently.
Other factors
CodeRabbit's one nit (assert exitCode) was addressed. CI passed the three new tests on all lanes; remaining red is documented as pre-existing on main and unrelated to the Bun.file(path) code path. The existing Bun.file() works as stdin and stdout test already implied replacement semantics (pre-fills with "wrong!" and expects "hello!") but happened to use equal-length strings, so it never caught this.
|
Closing: #46665 supersedes this PR.
A design review of this PR picked the shape that #46665 already builds. Reopen this PR if the two-line change is preferred. |
What does this PR do?
Bun.spawn({ stdout: Bun.file(path) })(andspawnSync, andstderr:) opened the destination withO_WRONLY|O_CREATbut withoutO_TRUNC. If the file already existed and the child wrote fewer bytes than were already there, the stale tail of the old content was left in place.Every other "write these bytes to this path" surface already truncates:
Bun.write(path, data),Bun.$\cmd > ${path}`, andfs.openSync(path, 'w')`. This brings the spawn redirect in line.How did you verify your code works?
New tests in
test/js/bun/spawn/spawn.test.tsforspawnstdout/stderr andspawnSyncstdout. Each pre-fills the destination with a longer string and asserts the exact short output afterwards.Existing
Bun.file() works as stdinandBun.file(path) at index >= 3 is readable in the childstill pass: stdin staysO_RDONLYand extra stdio slots stayO_RDWR|O_CREAT(no truncation), since the established use case there is reading from the file in the child.Fix applied on both the posix_spawn path (
src/spawn_sys/spawn_process.rs) and the Windows libuv path (src/spawn/process.rs).no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/spawn/spawn.test.ts