feat(rewrite): rewrite in pipe when consummer is safe

[aeppling]

Follow #3128 which protect pipe consumer.
Results are valid in benchmark, we can now extend to safe consumers rewritting, like tail / head / cat which can ingest filtered data

[TaKO8Ki]

Thanks. The consumer allowlist looks reasonable, but I think producer eligibility also needs to be checked.

For example, this PR newly rewrites:

ping 127.0.0.1 | head -5
→ rtk ping 127.0.0.1 | head -5

rtk ping buffers until ping exits, so head receives no output.

It also rewrites:

grep -f patterns.txt input.txt | cat
→ rtk grep -f patterns.txt input.txt | cat

However, rtk grep does not yet parse -f safely.

These RTK limitations already exist, but this PR exposes them to pipelines that currently remain raw. Could we add producer-specific safety checks and keep these cases raw for now?

[aeppling]

Hey @TaKO8Ki

What do you think would be the correct approach here ? We could add a new field in Rules to determine whether a command is a safe pipe producer or not, but that will still need maintenance from us, and a initial cost of choosing which current commands are safe or not

I didn't found a better solution than this, open to your thoughts about it so we can figure out how to implement this

[aeppling]

@TaKO8Ki added a pipe safe producer boolean in the rules list to set up all safe producers.

@KuSh no more magic string, moved to rules list

[aeppling]

@KuSh updated :)

[aeppling]

@KuSh thanks for the review, all 5 points are addressed in 148e0d4:

• Quote-aware consumer args: consumer stages now go through shell_split (same helper as search_uses_pattern_file), so quoted or escaped flags like tail "-f" are dequoted before matching. Test added.
• Producer/final consolidation: shared rewrite_pipeline_stage handles slice, rewrite and identity filter, both callers are thin wrappers around it.
• fd-dup redirects: the guard now reuses redirect_has_file_target from the lexer, so only real file targets disqualify a consumer stage. git log | tail -5 2>&1 rewrites now.
• PipelineSafety enum: None / ProducerOnly / FinalOnly / Both replaces the two booleans (grep and rg are Both). Kept FinalOnly as you suggested, with #[allow(dead_code)] until a rule needs it. If you would rather drop the variant for now, easy change.
• Docs: src/discover/README.md and docs/contributing/TECHNICAL.md now describe producer rewriting under Tracking issue for safe pipeline rewriting #3171 instead of the always-raw claim.

[KuSh]

Round 3 — verified against 148e0d44, built and run rather than read. All five open threads are fixed; I've resolved them. No code objection remains.

What I verified

thread	evidence
quote-awareness	tail "-f", tail \-f, tail '-f', tail "--follow" all went REWRITTEN → RAW
2>&1 over-conservatism	35 redirect spellings exercised; 13 fd-dup / /dev/null forms newly rewritable, zero real-file forms slipped through (> o, >> o, &> o, >& o, >| o, 2>&1 > o, > o 2>&1, | tee o all still raw)
producer/final duplication	rewrite_pipeline_stage factors the shared body and keeps the differing wrapping per caller — right call, the two callers genuinely differ
stale doc claims	TECHNICAL.md and README.md now match the real consumer set and redirect rule
two correlated bools	84 rules migrated to PipelineSafety, zero semantic differences vs 7ed1dd63 (53 producer-safe, 2 final-safe, as before)

Two hypotheses I built and dropped, so they don't come back next round:

• >&3 with a pre-bound fd. git log | tail -5 >&3 now rewrites, so a descriptor bound by exec 3>out.txt could in principle receive filtered output. Unreachable: every same-string route stays raw (;, &&, newline, { }, ( ), same-stage), and shell fd state does not survive between tool invocations.
• tokens[i+1] reading past end_offset. It's .get(), and end_offset is the boundary token's own offset, so the lookahead lands on the Operator and falls to _ => true. Checked with &&, ;, &, ||.

Two things still needed

1. Rebase. The branch conflicts with develop since #3704's lexer consolidation landed. It's textual only — two hunks in registry.rs: the super::lexer import list, and a doc comment colliding with rewrite_pipeline_producer. The import resolves to:

use super::lexer::{
    advance_quote_state, coalesce_words, is_crlf_at, redirect_has_file_target, shell_split,
    split_on_operators, tokenize, tokenize_with_newlines, ParsedToken, PipeKind, TokenKind,
};

I resolved it locally and ran the full gate on the merged tree: cargo fmt --check clean, cargo clippy --all-targets zero warnings, 3137 tests pass / 0 fail. Worth knowing that CI's Rust gate has not run on this head — ci.yml triggers on pull_request and GitHub can't build a merge ref while the PR conflicts, so the green checks here only cover CodeQL.

2. A test that closes the class. test_rewrite_pipe_following_tail_stays_raw has gained one literal per review round — --foll after round 2, "-f" and \-f after round 3. The spellings found by review are covered; the class isn't. tail '-f' and tail "--follow" both behave correctly today and nothing pins them, so the next spelling nobody thought of lands the same way.

Could this become a property test — for each entry in unsafe_flags / unsafe_flag_chars, generate the shell-equivalent spellings (single-quoted, double-quoted, backslash-escaped, =-joined, bundled short, unambiguous long abbreviation) and assert every one stays raw? A consumer added to SAFE_PIPE_CONSUMERS later would then be covered on arrival rather than after the round that finds the hole.

One caveat so it isn't over-applied: please don't merge arg_matches_unsafe_flag with search_uses_pattern_file. They look alike and aren't. Prefix-abbreviation matching is correct for tail — --follow is its only f long option, and tail --foll does follow on coreutils 9.10 — and would be wrong for grep, where --fil is ambiguous against --files-with-matches/--files-without-match and is rejected outright. Two matchers, two input languages.

One note, not a request

The consumer-safety analysis is newline- and comment-unaware, so git log | tail -5 + newline + tee out.txt and git log | tail -5 # note both fold the trailing text into the consumer stage. I tried to invert a safety verdict with both and couldn't — newline-gluing breaks the exact c.name == head match, and a comment only adds more tokens for the flag scan to trip on, so both can only over-block. It costs a missed optimization on inputs like tail -- -f, never correctness. Mentioning it only because those spellings are natural cases for the property test above to generate.

[KuSh]

Round 5 (landing) — APPROVED

Claim: rewrite a pipeline's producer command when its rule allows it and every downstream stage is a display-only consumer (#3171).
Scope: accept (frozen at round 1).
Ran: 570 generated tail flag spellings, RTK's verdict diffed against real coreutils 9.10 behaviour; an 18-case redirect/consumer matrix across three binaries (develop d9a6dba6, previous head 148e0d44, this head); the full gate on the real merged head.

Blocks merge (0)

Empty since round 3, and nothing new. Both outstanding asks are closed:

• Rebase — done in 280d0918, and your resolution is byte-identical to the one I validated locally last round.
• Property test — pushed as the fixup below.

Fixup pushed: ceb1d175

test(rewrite): cover every shell spelling of an unsafe consumer flag — +62 lines in src/discover/registry.rs, tests only, no production code. It generates the spellings from SAFE_PIPE_CONSUMERS rather than listing them: every unambiguous getopt_long prefix in bare, double-quoted, single-quoted, backslash-escaped and =value form, plus each unsafe short flag bare, quoted, escaped and bundled. A consumer added to that table later is covered on arrival instead of after the round that finds the hole.

It earns its place — reverting either of the last two rounds' fixes makes it fail:

shell_split -> split_whitespace   FAILED on  git log | tail "--f"
starts_with -> ==                 FAILED on  git log | tail --f

Gate with it in: fmt clean, clippy zero warnings, 3241 pass / 0 fail.

Checked and correct — no need to re-verify

• Differential fuzz, 570 spellings. Cases where rtk says safe but real tail actually follows: 0. All 53 genuine follow-forms are blocked, including --f through --follow and their quoted and escaped variants. The remainder are safe (487) or over-conservative on flags tail itself rejects (30) — both harmless directions.
• The develop merge changed no verdict. This head vs 148e0d44 across all 570 spellings: 0 differences. Worth stating explicitly because the merge brought in #3704's lexer rework, which touches exactly this code.
• No regression against develop on the redirect/consumer matrix. Every difference is the intended new producer rewriting; > out.txt, >> out.txt, &> out.txt, | tee out.txt and |& all still stay raw.
• CI is real now. It could not run while the PR conflicted — the green checks then were CodeQL only. fmt, clippy, test on ubuntu/macos/windows, test presence, semgrep and Security Scan all pass on ceb1d175.

Your questions

• "Kept FinalOnly … If you would rather drop the variant for now, easy change." → Keep it. Dropping it would leave the enum unable to express a state the gating logic already handles, which is the representability problem the enum was introduced to fix in the first place. #[allow(dead_code)] is the right marker for "valid, not yet used".

Next

Nothing — approving. Thanks for the patience across five rounds; the consumer-safety analysis ended up considerably more solid than where it started.

Rounds: landing. Threads: 0 open, 14 resolved.

[pszymkowiak]

Good PR. git log | tail -5 is one of the most common shapes an agent types, and it was a complete blind spot until now, so the token win here is real rather than theoretical.

What convinced me is that the two locks are independent. The consumer side proves safety by closed enumeration - three commands allowed, everything else falls raw - so a consumer nobody adds later costs a missed optimization and never a broken command. The producer side is opt-in per rule with None as the default, which is the right way round given that rtk reads the child to completion before emitting anything. And generating the flag spellings off SAFE_PIPE_CONSUMERS instead of listing them turns "someone remembered --fol" into "every abbreviation is covered", with the companion test making sure the generated one can't pass vacuously.

Approving. Two things I want on the record, since neither is a bug but both change what users get:

1. tail -5 no longer returns the same lines. It's the last five lines of rtk's compressed output, not of the raw log. That's defensible - the agent would see the filtered output anyway if it dropped the pipe - but it's the first place rtk changes a command's result rather than only its presentation. Please put a line in the CHANGELOG saying so explicitly.

2. Early SIGPIPE termination goes away. huge-command | head -5 used to kill the producer as soon as head had its five lines; with rtk in the middle the producer runs to completion. ProducerOnly rules out the never-terminating producers, but not the merely slow ones. Worth a stopwatch on git log | head -5 in a large repo before this ships - if it's noticeable, it belongs in the docs next to point 1.

Smaller note, no action needed: is_safe_pipe_consumer looks at the head word and the flags but not the operands, so git log | tail -5 file.txt classes as safe even though tail ignores stdin entirely. Harmless in practice, just one more implicit assumption inside a function whose whole job is to be conservative.