Skip to content

feat: support redirects in pipe sequence commands#177

Merged
bartlomieju merged 1 commit into
mainfrom
feat/redirects-in-pipe-sequence
Jun 1, 2026
Merged

feat: support redirects in pipe sequence commands#177
bartlomieju merged 1 commit into
mainfrom
feat/redirects-in-pipe-sequence

Conversation

@bartlomieju
Copy link
Copy Markdown
Member

The parser previously rejected redirects on any command in a pipe
sequence (e.g. cmd 2>&1 | tee log), even though the AST's Command
node already carries a per-command redirect and the executor's
execute_command applies it before dispatching to execute_simple_command
/ execute_subshell. Drop the parser bailout in parse_pipeline_inner
so common bash idioms like cmd 2>&1 | reader and cmd > file | reader
parse and execute.

The existing negative parser test that asserted the error message is
replaced with two positive parser tests covering echo 1 1> stdout.txt | cat and cmd 2>&1 | tee log. Two integration tests exercise the
runtime behavior end-to-end: that the left side's stdout redirect makes
the right side receive an empty pipe, and that 2>&1 | correctly
funnels both stdout and stderr into the pipe.

Fixes denoland/deno#28808.

The parser previously rejected redirects on any command in a pipe
sequence (e.g. `cmd 2>&1 | tee log`), even though the AST already
carries a per-command `redirect` and the executor's `execute_command`
applies it before dispatching to `execute_simple_command` /
`execute_subshell`. Drop the parser bailout so common bash idioms like
`cmd 2>&1 | reader` and `cmd > file | reader` parse and execute.

Fixes denoland/deno#28808.
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 29, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Member Author

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — clean, correct, and well-tested. I traced the executor to confirm the central claim, and the semantics match bash exactly.

The fix is sound. The parser bailout was overly conservative; the executor already handles per-command redirects inside a pipe sequence:

  • execute_pipe_sequence assigns each command's stdout to the pipe writer, then calls execute_command.
  • execute_command resolves the per-command redirect after the pipe stdout is assigned, then reassigns the fds.

This ordering is exactly bash's model (pipe redirection first, then the command's own redirects), so both cases come out right:

  • echo 1 > output.txt | cat — pipe sets echo's stdout to the pipe, then > output.txt overrides fd 1 to the file; the pipe writer drops, so cat gets EOF (len=0). ✓
  • ... 2>&1 | readerresolve_redirect_pipe for IoFile::Fd(1) returns the stdout writer, which is the pipe, so fd 2 → pipe; both streams funnel through (o\ne). ✓

Tests improved. The negative parser test is replaced by two positive parser tests (full AST) plus two deterministic end-to-end integration tests covering the redirect-vs-pipe ordering.

Minor: input/middle-command redirects (a | cmd < input | reader) are now also permitted — correct bash behavior and the executor handles it, though not covered by a test. Non-blocking.

One gate note: CI shows "no checks found" — worth confirming the workflows actually run before merge.

@bartlomieju this is ready to merge (pending CI).

@bartlomieju bartlomieju merged commit 8f6c55a into main Jun 1, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support redirects in pipe sequences in deno.json tasks

2 participants