Repository navigation
Conversation
…tured A command substitution env owned a fresh stderr buffer when the root stderr was captured (.quiet(), .text()). Nothing read that buffer, so the stderr of every command inside $(...) was dropped. The env now owns only its stdout buffer, which is the value of the substitution. Its stderr borrows the buffer of the parent env, like a subshell or a pipeline member does. Delete ShellExecEnvKind::Normal. No caller passed it.
|
Updated 5:17 AM PT - Sep 17th, 2026
✅ @robobun, your commit ae31978b9cfc964d7292605a415736f1c37b37e0 passed in 🧪 To try this PR locally: bunx bun-pr 43063That installs a local version of the PR into your bun-43063 --bun |
|
Status How I reproduced it, on 1.4.3-canary.1+c6b7fcb5b and on import { $ } from "bun";
$.nothrow();
const r = await $`echo "[$(ls /nonexistent-dir-zz)]"`.quiet();
console.log(JSON.stringify(r.stderr.toString()));
// before: ""
// after: "ls: /nonexistent-dir-zz: No such file or directory\n"
PR: #43063 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesShell command substitution buffering
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The command-substitution buffering change has no identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — a small, well-scoped change to dupe_for_subshell that makes a cmd-subst env borrow the parent's stderr buffer instead of dropping an owned one, with tests covering the variant matrix.
What was reviewed:
- The refactored
bufio_for/stdoutsplit against the old arms: Subshell/Pipeline stdout and all stderr paths resolve to the sameBufioas before; only cmd-subst stderr underPipechanges (Owned -> Borrowed). The removedNormalvariant had no callers. - The
Bufio::Borrowedlifetime contract for cmd-subst stderr: it now points at the root env's buffer the same way subshell/pipeline children already did under.quiet(); child envs are freed inExpansion::child_donebefore the parent resumes, and the finalizer frees owned envs beforedeinit_embedded, so no new ordering is introduced. - The
.exitCode(1)ordering test relies onTestBuilder's pre-existing catch-pathdoChecks(not awaited); the expects run synchronously so a failure still surfaces as a rejected promise — same shape as the existingexitCode(1)case at line 262.
Extended reasoning...
Overview
The PR changes one function in src/runtime/shell/interpreter.rs (ShellExecEnv::dupe_for_subshell) and adds a describe("cmd subst stderr") block to test/js/bun/shell/bunshell.test.ts. Previously the OutKind::Pipe arm of the shared bufio_for closure gave a CmdSubst child an owned Vec for both stdout and stderr; only stdout was ever read (Expansion::child_done), so stderr written inside $(...) was silently dropped whenever the root stderr was Pipe (.quiet(), .text(), ShellError.stderr). The fix hoists the cmd-subst special case out of the closure so it applies to stdout only, and makes Pipe always borrow the parent's buffer. The unused ShellExecEnvKind::Normal variant and its Default derive are deleted; the three call sites (Expansion.rs, Subshell.rs, Pipeline.rs) are unchanged.
Security risks
None identified. The change does not touch input parsing, path handling, credentials, or process spawning. The one memory-safety-relevant piece is a *mut Vec<u8> borrow, discussed below; it is a reuse of an existing pattern, not a new one.
Level of scrutiny
Moderate: it is native code with raw-pointer borrowing, so I traced the old and new arms explicitly. For Subshell/Pipeline kinds, stdout still goes through bufio_for(&io.stdout, ...) and every OutKind arm produces the same Bufio as before (Fd captured -> Borrowed, Fd uncaptured / Ignore -> Owned, Pipe -> Borrowed). For CmdSubst, stdout was Owned before (since the only caller passes OutKind::Pipe) and is unconditionally Owned now. stderr for CmdSubst under Pipe is the sole behavior change. The borrowed pointer targets the root env's _buffered_stderr (via buffered_stderr() which resolves through a borrow, so borrows do not chain); the root env is embedded in the boxed Interpreter, child envs are freed in Expansion::child_done, and the finalizer path frees all owned child envs before calling deinit_embedded(true). Subshell and pipeline children under .quiet() already relied on exactly this ordering, so the fix does not introduce a new lifetime obligation. The candidate concerns from the hunt (a write into a freed root buffer while a subprocess inside $(...) is alive; memory growth from retained stderr; the io argument being ignored for CmdSubst stdout) all resolve to either pre-existing behavior shared with pipelines or to the only caller's actual inputs.
Other factors
The tests cover builtin, subprocess, command-not-found, nested substitution, assignment, subshell, pipeline, ordering with surrounding writes, 2>&1 inside the substitution (guarding that the value is unaffected), and both the .quiet() result and ShellError.stderr from .throws(true).text(). They assert exact strings on {stdout, stderr, exitCode}. The messages asserted (ls: X: No such file or directory, bun: command not found: X) match strings already asserted elsewhere in the same file. The .exitCode(1) case goes through TestBuilder.run's catch path where doChecks is not awaited; that is a pre-existing harness quirk shared with the existing exitCode(1) test at line 262, and since the expects execute before the first await inside doChecks, a failure surfaces as a rejected promise that the test runner reports. No CODEOWNERS entry covers the changed files, no third-party objections are recorded in the timeline, and the bug hunt exited on dry_streak with zero findings.
Problem
.quiet()or.text(), the Bun Shell drops the stderr of each command inside a command substitution$(...).(await $`echo "[$(ls missing)]"`.quiet()).stderris empty, and so isShellError.stderr. Without.quiet()the script prints and capturesls: missing: No such file or directory, as bash does.ShellExecEnv::dupe_for_subshell(src/runtime/shell/interpreter.rs:1911). It gave a command substitution an owned buffer for stdout and for stderr.Expansion::child_donereads only the stdout buffer.deinit_implfrees the stderr buffer unread.Fix
.quiet(), each env borrows the stderr buffer of the root env, whichresult.stderrreturns. Without.quiet()the substitution already borrowed that buffer.ShellExecEnvKind::Normalvariant. No caller passed it.test/js/bun/shell/bunshell.test.ts(quiet > cmd subst stderr, 11 cases, 10 fail without the fix). Also all oftest/js/bun/shell/. Self-review: see Notes.Background
ShellExecEnvholds the variables, the cwd, and two capture buffers for one scope of the shell.dupe_for_subshellcopies it for a subshell, a pipeline member, or a command substitution.Bufio:Owned(Vec<u8>)orBorrowed(*mut Vec<u8>). A borrowed buffer points at the buffer of an ancestor env, so the child output lands in the result of the script.OutKind::Pipemeans: append to the capture buffer of the current env..quiet()sets it for the root stdout and stderr.Notes
Left out on purpose: the stdin of a substitution (#43052).
Expansion::nextbuilds the IO of a substitution from the root IO (src/runtime/shell/states/Expansion.rs:204). Soecho hi | echo "[$(cat)]"reads the stdin of the process. bash prints[hi]. That defect is about where the IO comes from. This PR is about who owns the stderr buffer. A fix for stdin must pass the IO of the enclosing command toExpansion, and it does not touchdupe_for_subshell. This PR does not change stdin.Self-review.
VAR=$(ls missing) && echo .... In bash the assignment takes the exit status of the substitution, soechodoes not run. Bun runs it (Assigns::nextalways reports 0). The test now uses;and does not depend on that difference.match kind, so a new kind must make a decision.src/runtime/shell/states/Cmd.rs:165and:184call the capture buffer a "command-substitution aggregate buffer". That wording was already loose before this change (the same code path serves a root command under.quiet()).Why the parent buffer is the right target.
IOis built at three sites only: the root (interpreter.rs:568),Pipeline.rs:219(clones its own stderr), andExpansion.rs:204(the stderr of the root). Under.quiet()each of them isOutKind::Pipe, andbuffered_stderr()resolves a borrow to the owner, so borrows do not chain. Each env points at theVecof the root env, which lives in the boxedInterpreter. A child env is freed before its parent resumes (Expansion::child_done,Pipeline::child_done,Subshell::deinit). The finalizer path only drops boxes and does not write through a borrowed pointer.Behavior I compared, fixed build against bash.
.quiet())result.stderrbeforeecho "[$(ls missing)]"ls: missing: No such file or directoryecho "[$(sh -c 'echo err >&2')]"errecho "[$(echo hi 1>&2)]"hiecho "[$(ls missing 2>&1)]"echo "[$(ls missing 2>/dev/null)]"echo "[$(ls missing)]" 2> filelsline (bash also sends it to the stderr of the shell, not to the file)$(ls missing)with.throws(true)ShellError.stderremptylsline$(...)The same scripts without
.quiet()capture the same stderr before and after.Not a regression. The Zig
dupeForSubshellhad the same.normal, .cmd_subst => .ownedarm for both streams.Related open work. #40228 rewrites
BufioasCapturedBufand keeps the owned stderr for a command substitution. The PR that lands second needs a small rebase of this one function.What I ran. Debug build with ASAN on Linux x64.
2>&1case passes on both builds on purpose: it guards the value of the substitution.test/js/bun/shell/bunshell.test.ts. On the final run the host was under heavy load, and two cases ofstdin redirect from a zero-length bufferhit the 5 s timeout (445 pass, 2 fail). They spawn seven debug processes at once and do not use a command substitution. The same file was fully green (446 pass) on the first shape of this change, before I replaced theowns_pipeflag with thematch kindand deletedNormal.test/js/bun/shell/andtest/js/bun/shell/commands/, andtest/regression/issue/27099.test.ts. The failures there were timeouts under the debug build (shell-hangwith its 700 ms budget,shell-leak-args,memleak_*,#11816 external,shell-load) and the twolspermission tests, which also fail on the released build because the container runs as root. None of the failed tests uses a command substitution. Theshell-hangfixtures give the right exit codes when I run them directly.leak.test.tsandshell-load.test.tsran in full on the first shape only. On the final shape I ran the threeleak.test.tscases that use$(...), and they pass.$(...)under.quiet()is still alive. It reported nothing.no test proof · iteration 0 · 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