Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughParser now accepts subshells with redirections. Subshell runtime defers script start, applies/redacts redirections (open targets, wire/duplicate streams, ambiguous target checks), and always processes expansions on child completion. Tests updated and new regression tests added for subshell redirection behavior. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/11124.test.ts`:
- Around line 1-4: Add a single-line URL comment linking to the GitHub issue at
the top of the test file (above the existing imports such as the line "import {
$ } from \"bun\";") so the regression test has traceability to the issue (e.g. a
comment containing the issue URL for `#11124`).
🪄 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: 9dd48406-10f1-4061-9c67-2f737303f376
📒 Files selected for processing (4)
src/shell/shell.zigsrc/shell/states/Subshell.zigtest/js/bun/shell/bunshell.test.tstest/regression/issue/11124.test.ts
💤 Files with no reviewable changes (1)
- src/shell/shell.zig
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shell/states/Subshell.zig`:
- Around line 157-158: The code currently hard-codes is_pollable based on
bun.Environment.isPosix, which misclassifies FIFOs/TTYs; instead use the
.pollable value returned by openForWritingImpl(). Update Subshell.zig to read
the openForWritingImpl() result (the variable holding the returned struct from
openForWritingImpl) and set is_pollable = result.pollable (and remove any use of
bun.Environment.isPosix for this purpose); apply the same change in the other
occurrences noted (the blocks around lines 163-183 and 222-225) so all branches
use the openForWritingImpl().pollable value rather than a platform constant.
- Around line 238-240: The .jsbuf branch in Subshell.zig currently fails all JS
buffer redirections by calling writeFailingError; update Subshell.zig to handle
.jsbuf the same way as Cmd.zig does (reuse the blob/response wiring used in
src/shell/states/Cmd.zig for JS buffer targets) or alternatively reintroduce
explicit rejection for .jsbuf only until parity exists; specifically, locate the
.jsbuf arm in Subshell.zig and either forward the redirect to the same handling
logic used by Cmd.zig (the blob/response path that accepts Bun.file and Blob
targets) or change the branch to return a clear rejection consistent with the
parser behavior by calling writeFailingError for only .jsbuf while leaving other
redirects intact.
🪄 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: 9979944c-45aa-4384-ba11-c242d5f0f96a
📒 Files selected for processing (1)
src/shell/states/Subshell.zig
There was a problem hiding this comment.
LGTM — all previously identified bugs have been addressed.
Extended reasoning...
Overview
This PR removes the parser guard that rejected subshell redirections, fixes the Subshell state machine (start/next/childDone), and adds applyRedirections() to wire file-based redirects before executing the inner Script. Four files are touched: the parser, Subshell.zig, and two test files.
Bugs Identified and Fixed
My prior review surfaced four issues; all were resolved before this approval:
- Dead code in the outer
else if (redirect_flags.duplicate_out)branch (needed guard fix in transitionToExec) — fixed in 0f59789 - Ambiguous redirect error missing
atsubshell`` context — fixed in 0f59789 - exit_code not set to 1 before writeFailingError() — fixed in cbedb0b
- Dead
duplicate_outarm inside the.atommatch — cleaned up in cbedb0b
Remaining Pre-existing Issue
The is_pollable constant (always false on POSIX) ignoring the dynamic pollable out-parameter from openForWritingImpl() is a pre-existing pattern copied verbatim from Builtin.zig. It only affects edge cases (FIFOs, /dev/fd/N, TTYs) and the same fix should be applied to both files in a follow-up.
Security Risks
No security-sensitive code paths are touched. The change opens file descriptors via the existing ShellSyscall.openat abstraction already used throughout the shell, with the same permission/flag handling.
CI
The azure-service-bus SIGILL failure on x64-asan is unrelated to shell redirection changes and predates this PR.
Test Coverage
Regression tests directly exercise the four primary cases: overwrite, multi-command, append, and variable-expanded path.
cbedb0b to
98feb72
Compare
There was a problem hiding this comment.
No issues found on this revision, but this is a ~300-line shell-interpreter feature (new state-machine transitions, FD lifecycle, IOWriter/IOReader wiring, unsafe pointer derefs in apply_redirections), so it warrants a human pass before merge.
Extended reasoning...
Overview
This PR enables (cmd) > file redirections on subshells by removing the parser guard in src/shell_parser/parse.rs and implementing the runtime support in src/runtime/shell/states/Subshell.rs (~300 net new lines): a reworked next() state machine with an ExpandingRedirect state, a new transition_to_exec() / apply_redirections() pair that opens the redirect target via shell_openat / open_for_writing_impl and rewires self.io.{stdin,stdout,stderr}, a write_failing_error() helper, and Expansion-child handling in child_done(). Tests: one updated assertion in bunshell.test.ts and four new regression tests in test/regression/issue/11124.test.ts.
Security risks
The redirect target is a user-controlled path opened relative to the shell's cwd_fd with 0o666 perms — identical semantics to the existing Cmd/Builtin redirect paths, so no new attack surface beyond what plain echo > file already exposes. No auth, crypto, or privilege boundaries are touched. The unsafe { &*me.node } derefs rely on the documented AST-arena-outlives-state invariant already used throughout the interpreter.
Level of scrutiny
This is non-trivial runtime logic in the shell interpreter: it adds FD lifecycle management (open → wrap in IOWriter/IOReader → hand to child Script), new state transitions, and several error paths (ambiguous redirect, open failure, jsbuf rejection, expansion failure). It has gone through several review rounds — earlier iterations had real bugs (unreachable fd-dup branch, exit-code-0 on redirect failure, stray test file in cwd) that were all fixed — and the current revision looks correct to me. But the combination of size, unsafe code, and FD/IO ownership semantics puts it outside the "simple/mechanical" bar for auto-approval.
Other factors
All prior inline comments (mine and CodeRabbit's) are resolved; the only change since my last review is the requested .ensureTempDir() addition. The known is_pollable hardcoding and lack of .jsbuf support are intentionally deferred to match existing Builtin.rs behaviour. Regression tests cover overwrite, append, multi-command, and var-expanded paths. The bug-hunting system found nothing on this revision.
e82be10 to
fa9b618
Compare
d782c5e to
0f17fe9
Compare
0f17fe9 to
3092127
Compare
3092127 to
25931d6
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and I didn't find any new issues, but this is ~350 lines of new shell-interpreter state-machine code (fd opening, IO rewiring, env buffer aliasing for the Pipe fd-dup case) so it's worth a human pass before merging.
Extended reasoning...
Overview
This PR enables (cmd) > file / >> / < / 2>&1 / 1>&2 redirections on subshells in Bun's shell. It removes a one-line parser guard in src/shell_parser/parse.rs and adds ~350 net lines to src/runtime/shell/states/Subshell.rs: a new ExpandingRedirect state, transition_to_exec(), apply_redirections() (opens files via shell_openat/open_for_writing_impl, wires IOReader/IOWriter, aliases env capture buffers for the OutKind::Pipe fd-dup case), write_failing_error(), and child_done() handling for the Expansion child. Eight new tests are added to the describe("subshell") block in bunshell.test.ts, covering >, >>, variable-expanded path, 2>&1, 1>&2, fd-dup inside command substitution, and <.
Security risks
None identified. Redirect targets are opened relative to the shell's cwd_fd via the same shell_openat/open_for_writing_impl helpers already used by Builtin and Cmd — no new path-resolution or privilege surface. The Bufio::Borrowed aliasing writes a raw pointer into the subshell's own duped env (which the Subshell exclusively owns until deinit), so lifetime is bounded by the existing ownership model.
Level of scrutiny
Medium-high. This is not a config tweak or mechanical change — it's a feature implementation in the shell interpreter's core state machine, with fd ownership, async error-write paths, cross-platform branches (Windows libuv fd ownership), and a subtle buffer-aliasing fix for the captured-pipe case that depends on ShellExecEnv internals. The PR has been through eight rounds of bot review (dead-code reachability, exit-code propagation, Pipe-case fd-dup semantics, test hygiene, stdin coverage) and the author addressed each one, but the cumulative complexity and the fact that no human has reviewed it yet argue for a human pass.
Other factors
The PR was originally written in Zig and re-ported to Rust after the Rust rewrite landed on main, which adds translation risk. Test coverage is now reasonable across all redirect operators. The is_pollable hardcoding and JsBuf rejection are documented as intentional parity with Builtin.rs and deferred to follow-ups. The bug-hunting system found no issues on the current revision.
There was a problem hiding this comment.
All prior feedback has been addressed and the bug hunt found nothing new, but ~300 lines of new interpreter state-machine + fd/IO-rewiring logic in the shell runtime is substantial enough that a maintainer should take a final look.
Extended reasoning...
Overview
This PR enables (cmd) > file / (cmd) 2>&1 style redirections on subshells in Bun's shell. It removes a 6-line parser guard in src/shell_parser/parse.rs and adds ~300 lines to src/runtime/shell/states/Subshell.rs: a new ExpandingRedirect state, apply_redirections() (opens redirect targets via shell_openat/open_for_writing_impl and rewires self.io with IOReader/IOWriter), write_failing_error(), and an expanded child_done() that handles the new Expansion child. Eight new tests were added to bunshell.test.ts covering >, >>, variable-expanded paths, 2>&1, 1>&2, fd-dup inside $(...), and <.
Security risks
The new code opens files relative to the shell's cwd_fd using paths derived from shell expansion — the same mechanism already used by Cmd/Builtin redirections, so no new attack surface is introduced beyond what plain echo > file already exposes. No auth, crypto, or permission logic is touched.
Level of scrutiny
This is core shell-interpreter runtime: a hand-rolled state machine with manual fd lifecycle management, raw-pointer env access, and OutKind/Bufio aliasing for the captured-pipe case. Across earlier review rounds I flagged (and the author fixed) an unreachable fd-dup branch, a wrong exit code on redirect failure, dead code inside the Atom arm, a semantic no-op for 2>&1 when stdout is OutKind::Pipe, test cwd hygiene, and a stdin coverage gap — the number of iterations needed to converge is itself a signal that this code is subtle. It's a feature addition, not a mechanical change, and it sits on a path that real package install scripts will hit.
Other factors
All inline comments are resolved; the only CI failure (bunx.test.ts) is unrelated. Test coverage for the new branches is now reasonable. The known limitations (hard-coded is_pollable, JsBuf rejection) are intentional and documented as follow-ups consistent with pre-existing Builtin behavior. I'm deferring rather than approving solely on the size/complexity criterion — there's nothing specific left for the author to change.
Parser no longer rejects subshells with redirections. The Subshell state
machine now expands the redirect target, applies the redirection to its
IO (opening files via ShellSyscall.openat / openForWritingImpl and
rewiring stdin/stdout/stderr), then spawns the inner Script with the
modified IO. fd-dup redirects (`2>&1`, `1>&2`) are handled without
opening a file.
Mirrors the Cmd/Builtin redirect pattern:
- ambiguous-redirect detection
- error messages with context label "at `subshell`"
- writeFailingError helper that sets exit_code=1 and enqueues via the
Subshell IOWriter tag
- duplicate_out handled in the no-target path; inside .atom only the
stdout/stderr flags are honored (duplicate_out never co-occurs by
parser invariant)
JS buffer (`> ${Bun.file(...)}`) redirects for subshells remain
rejected at runtime with a clear error, since the Subshell IO type
does not carry blob/array-buffer slots.
Closes #11124
Subshell redirections never worked before this change, so per test/CLAUDE.md this is a feature, not a regression - the tests belong in the module test file, not test/regression/issue/.
OutKind::Pipe is a unit variant resolved positionally downstream, so cloning stdout into stderr was a no-op in quiet mode and command substitution. Alias the subshell env's capture buffer (Bufio::Borrowed) so (cmd) 2>&1 and (cmd) 1>&2 route output to the right buffer.
215440a to
28be165
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Mirror Cmd's bounds check: a redirect target that expands to more than one word (brace/glob split) leaves redirection_file empty so the ambiguous-redirect error fires instead of opening a concatenated filename. Add tests for stderr-to-file, combined redirect, ambiguous redirect, and stdin open failure.
There was a problem hiding this comment.
Re-reviewed after a191df7 — no new issues found. Both prior findings are addressed: child_done now checks out.bounds.is_empty() before taking out.buf (matching Cmd.rs), and tests were added for 2>, &>, brace-expansion ambiguous-redirect, and the < missing open-failure path. Given ~350 lines of new state-machine + fd/IOWriter lifecycle code in the shell interpreter, a human pass is still worthwhile.
The one open comment-cop nag at Subshell.rs:437 targets the 2-line "Zero words or >1 word..." comment — that's copied verbatim from the sibling in Cmd.rs and reads as a false positive.
Extended reasoning...
Overview
PR removes the parser guard that rejected (cmd) > file and implements subshell redirections in the runtime: Subshell gains an ExpandingRedirect state that runs an Expansion child on the redirect atom, then apply_redirections() opens the target via shell_openat/open_for_writing_impl and rewires self.io with IOReader/IOWriter. fd-dup forms (2>&1/1>&2) rewire in place and alias the env capture buffer when the target is OutKind::Pipe. Errors go through write_failing_error() → WaitWriteErr → the re-added WriterTag::Subshell dispatch. ~350 net new lines across Subshell.rs and IOWriter.rs, plus 12 new tests in bunshell.test.ts.
What changed since my last review (11:08 UTC)
Commit a191df7 landed both fixes I asked for:
- The 🔴 multi-word-target bug:
child_donenow doeslet mut buf = if out.bounds.is_empty() { out.buf } else { Vec::new() }, so(echo hi) > {a,b}.txtnow hits the ambiguous-redirect path instead of creatinga.txtb.txt. A test assertsstderr == "bun: ambiguous redirect: atsubshell\n"withexitCode(1). - The 🟡 coverage gap: tests added for
2> err.txt(stderr-to-file arm),&> both.txt(clone-then-move Arc path), and(cat) < missing.txt(stdin open failure →write_failing_error→ exit 1). TheWaitWriteErr/on_io_writer_chunkpath is now exercised by the two error-path tests.
Security risks
None identified. Redirect targets are opened relative to the shell's cwd_fd via the same shell_openat/open_for_writing_impl helpers Cmd/Builtin already use; no new path-parsing logic. JS-object redirect targets are explicitly rejected with a clear error.
Level of scrutiny
Medium-high. This is user-facing runtime behavior in native code with fd ownership (IOReader/IOWriter init, Arc clone-vs-move for the stdout+stderr case), a new state-machine transition, and a re-added WriterTag dispatch arm. It closely mirrors the established Builtin::init_redirections pattern, which reduces risk, but the interaction between OutKind::Pipe positional resolution and the Bufio::Borrowed aliasing (the fix for the earlier fd-dup bug) is subtle enough that a maintainer familiar with the shell interpreter should sign off.
Other factors
- The PR has been through several review rounds; every substantive finding (fd-dup Pipe aliasing, stdin coverage, multi-word bounds check, error-path coverage) was addressed with code + tests.
- The lone unresolved inline comment is a comment-cop bot flag on a 2-line explanatory comment that is byte-identical to the one in Cmd.rs — the author previously (correctly) declined identical nags on this file as false positives.
- Not approving because the change is neither small nor mechanical; it's new native feature code in the shell interpreter.
This branch has not been deployed
1 participant
Problem
Subshell redirections like
(echo hello) > file.txtwere rejected at parse time with:This affected real packages like
ttf2woff2that use subshell redirections in install scripts.Root Cause
The shell parser (
src/shell_parser/parse.rs,parse_compound_cmd) explicitly rejected subshells with non-emptyredirect_flags, even though the AST already carriesredirectandredirect_flagsonast::Subshell. The runtimeSubshellstate (src/runtime/shell/states/Subshell.rs) spawned its innerScriptdirectly and never consumed those fields (the code carried a TODO for exactly this).Fix
Subshellstate machine:Idlenow expands the redirect target via anExpansionchild (ExpandingRedirectstate) before transitioning to execapply_redirections(): opens the redirect target (viashell_openat/open_for_writing_impl) and rewiresself.iostdin/stdout/stderr withIOReader/IOWriter, mirroringBuiltin::init_redirections; fd-dup forms (2>&1,1>&2) rewire without opening a filewrite_failing_error(): setsexit_code = 1, enqueues the message on the Subshell stderr writer (theWriterTag::Subshelldispatch already existed), so redirect failures propagate non-zero exitchild_done()collects the expanded path from theExpansionchild and re-entersnext()JS-object redirects (
(cmd) > ${Bun.file(...)}) remain rejected at runtime with a clear error: the SubshellIOtype has no blob/buffer slots (that path is Cmd-specific viaspawn_args.stdio).This PR was originally implemented in Zig and re-ported to Rust after the Rust rewrite landed on main.
Verification
New tests in
test/js/bun/shell/bunshell.test.ts(subshell describe block): stdout redirect, append redirect, variable-expanded path, plus the previously-disabled "redirection on subshell" ported case now asserts real output. All fail on the released bun (parser rejects) and pass on this branch.Closes #11124
Rebase notes (post shell-API refactor on main)
Main refactored the shell interpreter while this PR was open; the rebase resolved:
Base::newlost itsStateKindarg;Expansion::initlost itsio/ExpansionOptsargs — call sites updatedWriterTag::Subshelland itson_io_writer_chunkdispatch arm had been dropped fromIOWriter.rs(nothing used them on main); re-added for the Subshell failing-error pathast::Subshell.redirect/.redirect_flagsbecamepub(crate)inbun_shell_parser; madepubagain since the runtime now consumes themRedirectFlags::isEmptywas renamedis_emptypollablehandling for the redirectIOWriter(honors the value computed byopen_for_writing_implon POSIX), replacing the old hard-coded platform constantno test proof · iteration 17 · 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