Repository navigation
fix(rewrite): only rewrite safe final pipeline stages - #3128
Conversation
| value: "||".into(), | ||
| offset: start, | ||
| }); | ||
| } else if chars.peek() == Some(&'&') { |
There was a problem hiding this comment.
Supporting |& is necessary for this pr. Bash defines both | and |& as pipeline operators, but RTK previously interpreted |& as | followed by background &. This could produce an incorrect pipeline boundary and an unsafe rewrite. This PR recognizes |& atomically so RTK can distinguish it from a normal | pipeline and conservatively leave the complete stderr pipeline raw.
There was a problem hiding this comment.
Shouldn't we handle 2>&1 the same?
pszymkowiak
left a comment
There was a problem hiding this comment.
Super PR 🙌 I tested a lot of cases (30-stage pipes, |&, for/if, heredocs, permission deny/ask/allow) and nothing seems broken.
One small optimization left: statements separated by a bare newline only get the last one's final stage rewritten — \n isn't treated as a clause boundary like ;, so earlier lines' grep/wc/rg finals are missed. Not a correctness issue, just coverage:
git log | grep fix | wc -l # ← not rewritten
gh pr list | grep OPEN | wc -l # ← only this one
|
Hey @TaKO8Ki Thanks for addressing this important fix. search.rs steam stdinUse streaming for stdin grep before rewriting final stages, currently buffer-to-EOF delivers 0 bytes on a killed pipe (raw grep: 24KB) and tail -f | rtk grep never outputs. Not out of scope to modify search.rs, it is pre-requisite so all this work together. Open to other ideas repro: |& operatorsNIce catch on this, but instead of discarding all command can't we treat it as a pipe once final stage stream? Could apply same rule as for | keep rewrite for display tailWe could keep rewriting producers before display-only tails (head/cat/tail without -f), the agent is still the reader there, so it's safe. cargo test 2>&1 | tail -50 -> rtk cargo test 2>&1 | tail -50
git log --oneline | head -20 -> rtk git log --oneline | head -20drop wc from the safe finalsrtk wc -l is byte-identical to wc -l (zero savings) and bare rtk wc reshapes the output. Suggest keeping the list to grep/rg and growing it one measured entry at a time. |
| pub pattern: &'static str, | ||
| pub rtk_cmd: &'static str, | ||
| /// Whether this command may be rewritten as the final pipeline stage. | ||
| pub pipeline_final_safe: bool, |
There was a problem hiding this comment.
nitpick: would be great to have default to avoid having to rewrite every instances each time we add an entry
#3171
Summary
grep,rg, orwcstage.|&pipelines raw.|&atomically while continuing to check both sides during permission analysis.Behavior
Automatic rewriting previously could rewrite a pipeline producer, changing the bytes consumed by downstream commands.
This PR changes pipeline rewriting so that:
grep,rg, andwcare initially eligible.grepandrgusing-f/--fileremain raw because those forms are not yet parsed safely.|&pipelines remain raw.&&,||, and;operators can still be rewritten.|&is tokenized atomically while permission checks continue to inspect both sides.For example:
Follow-ups
To keep this PR focused and reviewable, I have a plan to address the following in separate PRs.
rtk_cmdstring comparisons and centralized command-specific checks with structured rule-level safety policies such asNever,Always, andConditional.Test plan
cargo fmt --all --checkcargo clippy --all-targets -- -D warningscargo test --allcargo test --test pipeline_stdin_testrtk <command>output inspected