Skip to content

fix(hooks): don't rewrite pipe-feeding segments whose filter changes output shape - #2965

Closed
xuing wants to merge 1 commit into
rtk-ai:developfrom
xuing:fix/pipe-aware-rewrites
Closed

xuing wants to merge 1 commit into
rtk-ai:developfrom
xuing:fix/pipe-aware-rewrites

Conversation

@xuing

@xuing xuing commented Jul 12, 2026

Copy link
Copy Markdown

Summary

  • find/fd already skip the pipe-left rewrite because rtk's output shape breaks xargs (Trouble running xargs as second bash command. #439). The same reasoning applies to every filter that reshapes output — this generalizes the special case into a PIPE_INCOMPATIBLE list: find, fd, grep, rg, ls, head, tail, cat
  • Downstream programs stop receiving rtk's summary headers, capped match lists, elision markers and size columns: grep -n X f | wc -l counted 28 for 60 real matches; $(ls -t | head -1) captured a decorated name
  • Commands without a pipe keep their rewrites, so the token savings on the common (agent-reads-it) path are untouched

Fixes #2962

Implementation notes

  • The inline find/fd check in rewrite_compound becomes one is_pipe_incompatible(seg) helper + a const list — net behavior change is which names are in the list
  • The helper skips leading VAR=value assignments, matching rewrite_segment's own env-prefix handling (LC_ALL=C grep … | sort is correctly recognized)
  • Only the segment feeding the pipe is skipped: grep … | wc -l && git status still rewrites git status

Test plan

  • test_rewrite_grep_pipe_skipped — the wc -l miscount case is not rewritten
  • test_rewrite_ls_pipe_skipped, test_rewrite_head_pipe_skipped — same for the other shape-changing filters
  • test_rewrite_grep_env_prefix_pipe_skipped — env prefix doesn't hide the command
  • test_rewrite_grep_no_pipe_still_rewritten — no pipe, still rtk grep (savings preserved)
  • test_rewrite_pipe_skip_only_affects_pipeline_segment — && segments after the pipeline still rewrite
  • Existing test_rewrite_find_pipe_skipped / test_rewrite_find_no_pipe_still_rewritten / test_rewrite_pipe_first_only unchanged and passing
  • End-to-end via rtk hook claude: grep … | wc -l and ls -t | head -1 now pass through; grep -rn TODO src still rewrites
  • cargo fmt --check && cargo clippy --all-targets && cargo test — 2441 passed, 0 failed, no new warnings

…output shape

The pipe-left rewrite fed rtk's summarized formats into downstream
programs: 'grep -n X f | wc -l' counted rtk grep's header and elision
lines (measured 28 for 60 real matches), '$(ls -t | head -1)' captured a
size-decorated name, and head's rtk-read mapping leaked '[N more lines]'
markers to whatever parsed the file. find/fd already had this exact
carve-out (rtk-ai#439) — generalize it to a PIPE_INCOMPATIBLE list covering the
other shape-changing rewrites (grep, rg, ls, head, tail, cat). Segments
without a pipe, and non-pipeline segments of the same compound command,
keep their rewrites.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 12, 2026 07:25
@CLAassistant

CLAassistant commented Jul 12, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the hook command rewriter to avoid rewriting the pipe-feeding segment when the rewritten command would change the output shape in a way that breaks downstream pipe consumers (e.g., grep -n … | wc -l, ls -t | head -1). It generalizes the existing find/fd carve-out into a shared PIPE_INCOMPATIBLE list and adds regression tests for several representative commands.

Changes:

  • Introduces PIPE_INCOMPATIBLE and an is_pipe_incompatible(seg) helper (with env-assignment prefix skipping) to decide when the pipe-left rewrite must be skipped.
  • Replaces the prior inline find/fd special-case in rewrite_compound with the helper.
  • Adds unit tests covering pipe-skipped behavior for grep, ls, head, env-prefixed grep, plus a test ensuring post-pipeline && segments still rewrite.

@KuSh

KuSh commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this, and sorry it sat — the diagnosis in #2962 was right, and the find/fd carve-out really was too narrow.

It's since been fixed upstream of this branch, which is why the PR now conflicts. #3128 (fix(rewrite): only rewrite safe final pipeline stages) and #3343 (feat(rewrite): rewrite in pipe when consumer is safe) replaced the inline seg.starts_with("find ") check this PR generalizes with a consumer-aware model: a per-rule PipelineSafety (producer_safe/final_safe) plus a SAFE_PIPE_CONSUMERS list. The producer is rewritten only when every downstream consumer is a display-only tail (cat, head, non-following tail). So the conflict isn't mechanical — the code this patches no longer exists.

I ran all six of the tests added here against develop (d0c29851). Four already pass:

case this PR expects develop
grep -n match sixty.txt | wc -l None None ✅
LC_ALL=C grep -n match f.txt | sort None None ✅
grep -rn TODO src rtk grep … rtk grep … ✅
grep -n x f | wc -l && git status … && rtk git status … && rtk git status ✅
ls -t | head -1 None rtk ls -t | head -1
head -20 src/main.rs | grep use None head -20 src/main.rs | rtk grep use

The last row is develop being deliberately better than the PIPE_INCOMPATIBLE approach: it keeps the producer raw and rewrites the final grep stage, so the savings this PR would have discarded are preserved. The ls -t | head -1 row is a real gap, but a narrower one than this PR's model — I've filed it separately as #4026, because the fix belongs in #3343's producer gating rather than in a blanket skip list.

Worth saying explicitly: the ls case in #2962 was $(ls -t | head -1), and every command-substitution form is still correctly left alone on develop. #2962 stays closed.

Recommending we close this one as superseded by #3128/#3343 — the analysis in #2962 is what drove that work. Thanks for filing both.

@KuSh KuSh closed this Sep 13, 2026
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.

Pipe-left rewrites feed rtk's summarized output into downstream programs (grep | wc -l counts 28 for 60 matches)

4 participants