Repository navigation
Adopt rtk-ai/rtk#2965: hooks - don't rewrite pipe-feeding segments whose filter changes shape - #16
Merged
Merged
Conversation
cargo build re-resolved the lock after the rtk-ai#2717 adoption; the upstream lock hunk omitted windows-sys from rtk's dependency list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…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>
Amendment to the adopted rtk-ai#2965: is_pipe_incompatible used its own env-prefix heuristic (skip words containing =), which missed sudo, env, and quoted env values, and did not normalize absolute binary paths -- so sudo grep ... | wc -l and /usr/bin/grep ... | wc -l were still rewritten and miscounted downstream. Reuse the same strip_disabled_prefix (ENV_PREFIX) + basename normalization the rewrite path applies, so skip detection and rewriting can never disagree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #13.
Adopts rtk-ai/rtk#2965 (cherry-pick, authorship preserved) plus one fork-authored amendment and one lockfile chore.
Commits
9d678b1chore: complete Cargo.lock entry for windows-sys dependency — housekeeping from the fix(core): decode process output using Windows console code page rtk-ai/rtk#2717 adoption (PR Adopt rtk-ai/rtk#2717: core - decode process output using Windows console code page #15): the upstream lock hunk omittedwindows-sysfrom rtk's dependency list;cargo buildre-resolved it.e95207dfix(hooks): don't rewrite pipe-feeding segments whose filter changes output shape — cherry-pick of upstreambc0f568, author XU Pengfei (@xuing). Generalizes the find/fd pipe-skip (Trouble running xargs as second bash command. rtk-ai/rtk#439) into aPIPE_INCOMPATIBLElist (find, fd, grep, rg, ls, head, tail, cat) +is_pipe_incompatible()helper, with 6 new tests. Byte-for-byte faithful:git diff pr-2965 HEAD -- src/discover/registry.rswas empty at cherry-pick time.e1e37fdfix(hooks): make pipe-skip normalization match the rewrite path — amendment: the helper's env-prefix heuristic missedsudo/env/quoted env values and absolute binary paths, sosudo grep … | wc -land/usr/bin/grep … | wc -lwere still rewritten (found by pre-PR review, confirmed empirically). Now reuses the samestrip_disabled_prefix(ENV_PREFIX) + basename normalization asrewrite_segment, so skip detection and rewriting can never disagree. +4 tests.Adoption checklist
upstream/developis already an ancestor of forkdeveloprewrite_compound; grep/ls/head/tail/cat/rg pipe-feeders were still rewrittencargo fmt --allclean,cargo clippy --all-targetszero warnings,cargo test --all2466 passed / 0 failedRepro (real output, this machine)
Before (develop @ 337bdda):
wc -lreports 25 for 50 real lines —rtk readelides half the lines. (The installed hook even rewrote one of this session's own verification pipelines mid-investigation, live-demonstrating the corruption.)After (this branch):
Review
Two-axis review (standards + spec) run pre-PR.
&&/;segment isolation and quoted-|safety verified in code. One confirmed generalization gap (env-prefix parity claim not actually met forsudo/env/quoted values) — fixed by the amendment.strip_disabled_prefix) is resolved by the amendment, which deletes the duplicate heuristic. Known ceiling: transparent prefixes (e.g. builtin wrapper prefixes the rewrite path strips recursively) are not mirrored in the skip check — same trade-off as upstream, noted for a future ticket if it bites.🤖 Generated with Claude Code