Repository navigation
Conversation
`2>&1` / `1>&2` are complete on their own; the following word is an argument of the command. `parse_redirect` was only checking `duplicate_out` after `parse_atom()` had already consumed the next word, so `echo a 2>&1 b` opened a file named `b` and wrote "a\n" into it instead of printing "a b". Check `duplicate_out` before reading a file operand, and interleave argument and redirect parsing in `parse_simple_cmd` so the word that follows the dup stays in `name_and_args`.
|
Warning Review limit reached
Next review available in: 1 minute 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)
Comment |
|
Reproduced with: bun -e 'await Bun.$`${{raw:"echo a 2>&1 b"}}`.quiet().then(r => console.log(JSON.stringify(r.stdout.toString())))'On 1.4.0-canary this prints |
|
Updated 3:48 AM PT - Jul 21st, 2026
✅ @robobun, your commit b6ee4df4d4c22dfe6cbabaabf39dd48a184a6070 passed in 🧪 To try this PR locally: bunx bun-pr 34904That installs a local version of the PR into your bun-34904 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted parser fix with parse-level and behavioral coverage.
What was reviewed:
parse_redirect: moving theduplicate_out()early-exit before the operand read is correct; the lexer encodes the target fd in the flags so no file operand exists.parse_simple_cmdloop: verifiedcheck()is a peek andParsedRedirect::default()matches the old no-redirect path, so commands without redirects andecho a 2>&1(bare) parse identically; the second-redirect guard turns a confusing leftover-token error into the existing "Multiple redirects" message.- Confirmed the parse test's
{stdout: true, duplicate_out: true}for2>&1matches the lexer (which flips STDERR→STDOUT), and the subshellparse_redirectcaller is unaffected since subshell redirects already error as unsupported.
Extended reasoning...
Overview
Two changes to src/shell_parser/parse.rs:
parse_redirectnow checksredirect.duplicate_out()before trying to consume a file operand, so2>&1/1>&2never eat the following word.parse_simple_cmdrestructures its arg-collection into a loop that interleavesparse_atomand (at most one)parse_redirect, so a word after the redirect returns toname_and_argsinstead of being left as a stray token for the caller.
Tests added in parse.test.ts (AST shape for echo a 2>&1 b, echo a 1>&2 b, and unchanged bare echo a 2>&1) and file-io.test.ts (end-to-end: correct stream, correct output, .doesNotExist("b") negative contract).
Security risks
None. Pure parser control flow over already-tokenized input; no allocation-size arithmetic, no FFI, no filesystem writes in the parser path. The fix removes an unintended file write.
Level of scrutiny
Medium. The parse_simple_cmd loop is a real control-flow change affecting all commands with redirects, but it is ~15 lines, the no-redirect path is provably identical (ParsedRedirect::default() = empty flags + None, same as the old parse_redirect() fallthrough when has_redirect was false), and the single-redirect path only differs by continuing to collect args afterward — which is the bash-correct behavior. The other parse_redirect caller (subshell, line 1236) is guarded by an "unsupported" error on any non-empty redirect flags, so no behavior change there.
Other factors
- I cross-checked the test's expected flags against the lexer at lines 3354-3372:
2>&1producesSTDOUT|DUPLICATE_OUT(STDERR is removed), matchingredirect({ stdout: true, duplicate_out: true }). The doc comment onDUPLICATE_OUT(lines 589-590) is inverted, but that's pre-existing and not touched here. .stderr("")in the new TestBuilder cases follows the established in-process shell-buffer convention used ~95× acrosstest/js/bun/shell/.- The overlap with #34901 is called out in the description; the loop shape is compatible.
- PR reports full
parse.test.ts,lex.test.ts,file-io.test.ts, andbunshell.test.tspassing.
…T comments The stored bit is the *target* fd: `2>&1` emits stdout=true and `1>&2` emits stderr=true. The doc comment on the flag and a matching annotation in Builtin.rs had them swapped.
There was a problem hiding this comment.
LGTM — focused parser fix with matching parse-level and behavioral tests.
What was reviewed:
- Verified against the lexer (parse.rs:3354-3369) that
2>&1→stdout+duplicate_outand1>&2→stderr+duplicate_out, so the corrected comments in Builtin.rs / parse.rs and theinit_redirectionsbranch semantics all line up. - Traced the new interleaved loop in
parse_simple_cmd:check()peeks,parse_redirect()consumes; a trailing word after a dup redirect lands back inname_and_args, and a secondRedirecttoken hits the new multi-redirect error. - Checked the other
parse_redirectcaller (subshell, parse.rs:1236) — the earlyduplicate_out()return is correct there too.ParsedRedirectderivesDefault, so the no-redirect path is unchanged. - Tests cover both directions, the bare
2>&1case,.doesNotExist("b"), and the multi-redirect error message.
Extended reasoning...
Overview
Fixes a shell-parser bug where 2>&1 / 1>&2 incorrectly consumed the following word as a redirect file target. Two logic changes in src/shell_parser/parse.rs: (1) parse_redirect now checks redirect.duplicate_out() before attempting to parse a file operand instead of only in the parse_atom() == None fallback, and (2) parse_simple_cmd interleaves argument and redirect parsing so words after a redirect stay in name_and_args. Two comment-only hunks in Builtin.rs and the RedirectFlags doc fix inverted flag descriptions. Tests added in parse.test.ts (AST shape) and file-io.test.ts (end-to-end via TestBuilder).
Security risks
None. This is grammar handling in the shell parser; no auth, crypto, path traversal, or untrusted-length arithmetic is touched. The change removes an unintended file-creation side effect.
Level of scrutiny
Moderate — the shell parser is user-facing runtime code, but the diff is ~25 lines of localized control-flow change with a clearly identified root cause and a mechanical fix (move an existing guard earlier; loop instead of sequence). I cross-checked the comment corrections and test expectations against the lexer's actual flag assignment at parse.rs:3354-3369, which confirms 2>&1 produces STDOUT|DUPLICATE_OUT (STDERR removed) — the old comments were indeed inverted and the init_redirections branch in Builtin.rs (if redirect.stdout() { stderr = stdout.dup_ref() }) does the right thing.
Other factors
- The interleaved-loop shape is acknowledged in the PR as overlapping #34901 for the
> file wordcase; I greppedtest/js/bun/shellfor existing tests exercising a word after a file redirect and found none, so no silent behavior change to existing coverage. ParsedRedirecthas#[derive(Default)](parse.rs:2152), so the no-redirect path yields the same empty flags/None as before.- The subshell caller of
parse_redirectat parse.rs:1236 also benefits from the earlyduplicate_out()return —(cmd) 2>&1 wordwon't grabwordeither (though the subshell path doesn't loop back for more args, that's pre-existing and out of scope here). - The new "Multiple redirects" error is a strict UX improvement over the previous
expected a command or assignment but got: "Redirect"and is covered by a test. - PR description states full
parse.test.ts,lex.test.ts,file-io.test.ts, andbunshell.test.tspass; robobun reproduced the fix on canary vs. this branch.
|
Closing: this is covered by #34901. That PR has the same parser change, and it now carries the tests and the corrected |
…OUT comments `2>&1` sets STDOUT with DUPLICATE_OUT and `1>&2` sets STDERR with DUPLICATE_OUT. The comments on the flag and in the builtin redirect setup said the reverse. parse_simple_cmd now reports a second redirection, so the note in parse_redirect that asked for that check is removed. Tests: the fd-dup forms create no file named by the next word and do not overwrite an existing one, `1>&2` and a trailing `2>&1` keep their AST shape, `echo a 2>&1 b > f` reports the multiple-redirect error, and the tail of an interpolated array after `>` stays in argv.
1 participant
Repro
1>&2had the mirrored problem:echo a 1>&2 bcreated an empty fileband wroteato stdout instead of writinga bto stderr.Cause
parse_redirectinsrc/shell_parser/parse.rsunconditionally read a file operand after consuming aRedirecttoken. Theduplicate_out()check was only reached whenparse_atom()returnedNone(i.e. the redirect was the last token of the command), so forecho a 2>&1 bit grabbedband stored it asredirect_file.Fix
Check
redirect.duplicate_out()before attempting to parse an operand; fd-dup redirects carry their target fd in the token itself and never take a file.parse_simple_cmdalso now interleaves argument and redirect parsing, so thebthat follows the dup stays inname_and_argsinstead of starting a new statement. That loop is the same shape as #34901 (which fixes the> filevariant of the same interleaving problem), so whichever lands first the other is a trivial merge; theparse_redirectchange here is the new piece.A second redirect after the dup (e.g.
echo a 2>&1 b > f) now reports the existing "Multiple redirects are not supported yet" parse error, which previously surfaced as the less helpfulexpected a command or assignment but got: "Redirect".Verification
test/js/bun/shell/parse.test.ts:echo a 2>&1 bandecho a 1>&2 bparse as one command withname_and_args: [echo, a, b]andredirect_file: null; bareecho a 2>&1is unchanged.test/js/bun/shell/file-io.test.ts: both forms printa bto the expected stream and no filebis created.Both fail on the released binary (parse sees
redirect_file: b; behavioral sees empty stdout and a file on disk). Fullparse.test.ts,lex.test.ts,file-io.test.ts, andbunshell.test.ts(414 tests) pass on this branch.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The shell parser's
parse_redirectunconditionally calledparse_atom()to read a file operand after any redirect token, so for fd-duplication forms like2>&1that take no operand, the following word was wrongly consumed as the redirect target instead of remaining a command argument. The fix checksredirect.duplicate_out()immediately after extracting the redirect flags and returns early with no file operand, leaving subsequent words to be parsed as arguments. It also reports the existing "multiple redirects are not supported" error when a dup redirect appears alongside another redire…