feat(git): filter large git show blob dumps with recovery and Latin-1 decoding

[breisnerlopez]

Summary

• feat(git): git show <rev>:<path> blob dumps (previously passed through at 0% savings) are now filtered — large text blobs are capped to an 8 KiB preview with a tail -n +N recovery pointer (the full blob is tee'd); small blobs, tree listings, binary, and blobs too large to recover intact pass through unchanged.
• Encoding-aware capture: blobs are captured as raw bytes and decoded encoding-aware — ISO-8859 / Latin-1 files (e.g. Oracle PL/SQL .pck) are transcoded losslessly instead of being corrupted into U+FFFD by lossy UTF-8, which also unblocks their compression. Ambiguous single-byte encodings (CP1252) and binary pass through raw. Measured on a real 77 KB Latin-1 blob: 0% → 89% savings with the É→� corruption removed. (Same class of fix as rtk curl http://....tar.gz | tar -xzf fails due rtk truncation, eg "(153 more lines, 94292 bytes total)" #1087.)

Test plan

• cargo fmt --all --check && cargo clippy --all-targets && cargo test — clean; blob/decode tests green on current develop.
• Unit + fixture tests cover decode (UTF-8 / Latin-1 / CP1252 / binary), byte-cap windowing, recovery-offset math, and edge cases (tree passthrough, binary passthrough, UTF-8 boundary no-panic, giant single line, exceeds-recoverable passthrough, no-trailing-newline).
• Manual testing:
  • git show HEAD:src/main.rs → ~120 KB → 8 KB, tail -n +N recovery reconstructs byte-for-byte
  • git show HEAD:<latin1.pck> → 89% savings, no �, recovery byte-identical to iconv -f ISO-8859-1
  • git show HEAD:<dir> (tree) and HEAD:<binary> → passthrough unchanged

Scope note: this is the feat(git) part of the earlier bundled PR #3064, split into a focused PR (per the single-focus rule) and rebased onto current develop. The independent fix(tee) recovery-slug fix from that bundle is submitted separately; the subjective advisory-hints refactor was dropped. #3064 will be closed in favor of these focused PRs.

[breisnerlopez]

Split out of the old #3064 and rebased clean onto develop: filters large git show <rev>:<path> blob dumps (~269 KB → ~8 KB) with encoding-aware Latin-1 capture, so binary-ish blobs still compress instead of flooding the context. Green + mergeable — would love a review when you get a chance, @KuSh 🙏

[breisnerlopez]

Gentle nudge — open ~10 days, CI green and CLA signed. @aeppling @KuSh, a review whenever you get a chance would be much appreciated.

This is the highest-value of my open PRs technically: it filters large git show <rev>:<path> blob dumps (~269 KB → ~8 KB) with encoding-aware Latin-1 capture, so even binary-ish blobs (e.g. an Oracle .pck that was passing through at ~0%) still compress well. It was split cleanly out of the old #3064 and rebased onto develop as a single-focus change. No rush, and glad to adjust anything. Thanks!

[KuSh]

Follow-up from review — none of these block merge, flagging for awareness / potential fast-follow.

1. decode_output's Latin-1 fallback can silently mis-decode EUC-JP / EUC-KR as garbage text (not just fail loudly)

The ambiguous-encoding guard only checks for bytes in 0x80–0x9F:

if bytes.iter().any(|&b| (0x80..=0x9F).contains(&b)) {
    Decoded::Binary
} else {
    Decoded::Latin1(bytes.iter().map(|&b| b as char).collect())
}

That correctly catches Shift-JIS/GBK/Big5 (lead bytes overlap 0x80–0x9F), but EUC-JP and EUC-KR use only the 0xA1–0xFE range for both lead and trail bytes, entirely outside that check. Such content falls through to the Latin1 branch and gets transcoded byte-for-byte into U+00A1–U+00FE — no panic, no U+FFFD, just wrong text that looks like a successful decode. That's arguably worse than the old from_utf8_lossy behavior, which at least surfaced visible � as a signal something was off.

Not a regression for the case this PR targets (ISO-8859-1 European source), but worth a follow-up — e.g. sniffing a UTF-16/UTF-32 BOM explicitly, and/or treating a high density of bytes clustered in 0xA1–0xFE as another "ambiguous, don't guess" signal alongside the existing 0x80–0x9F check.

2. Ambiguous CP1252 detection is conservative in the other direction

decode_output's Latin-1 branch only fires when no byte in 0x80–0x9F appears anywhere in the blob. Since invalid-UTF-8 content can contain such a byte incidentally (not necessarily a genuine CP1252 smart-quote), an otherwise-clean Latin-1 blob with one stray 0x80–0x9F byte anywhere falls back to Binary (full unfiltered passthrough) rather than being transcoded+windowed. Safe, but reduces the hit rate on real-world CP1252-tainted files. Given the stated "fidelity over guessing" philosophy this seems like an intentional tradeoff — just flagging the savings impact.

3. Multiple blob args always pass through whole

blob_args.len() == 1 is required for windowing; anything else (e.g. git show HEAD:a.txt HEAD:b.txt) passes through unwindowed even if both blobs would individually fit the budget. Documented and safe (avoids cutting the second blob mid-stream), just noting it as a known gap in case multi-blob invocations turn out to be common enough to be worth handling later.

4. blob_truncation's signature is dense with same-typed params

fn blob_truncation(raw: &str, budget: usize, max_recoverable: usize) -> Option<(&str, usize, usize)>

Three usize-shaped params/return values with no type-level distinction — a future caller could transpose budget/max_recoverable without a compiler error. Not urgent for a single well-tested private call site, but worth keeping in mind if this function grows more callers.

[KuSh]

One more follow-up, on CaptureBytes vs CaptureResult duplication in src/core/stream.rs:

CaptureBytes mirrors CaptureResult field-for-field (stdout/stderr/exit_code + a success() method), and exec_capture_bytes duplicates the cmd.stdin(Stdio::null()); cmd.output()... plumbing already in exec_capture — the only difference is String::from_utf8_lossy(...).into_owned() vs returning the raw Vec<u8> unchanged.

Considered making CaptureResult<T = String> generic instead of introducing CaptureBytes, but CaptureResult is referenced by type name across every *_cmd.rs filter that calls exec_capture, so genericizing it increases blast radius for a change whose actual duplication lives in the function bodies, not the struct shape (success() doesn't touch stdout/stderr at all; combined() is String-specific and would need to move to an impl CaptureResult<String> block).

Narrower alternative: keep CaptureResult/CaptureBytes as two distinct structs (matches the existing pattern and avoids a generic type threaded through the whole codebase for one byte-vs-text split), but factor the shared cmd.stdin(...); cmd.output() + exit-code logic into one private raw-capture helper that exec_capture/exec_capture_stdin decode from (lossy UTF-8) and that exec_capture_bytes returns as-is. That removes the actual repeated Command::output() boilerplate without changing any public API shape.

Not a blocker — just a low-risk follow-up to consider if exec_capture_bytes gets more call sites later.

[breisnerlopez]

Thanks for the thorough read, @KuSh — these are all fair, and I agree with the "fidelity over guessing" framing. Quick pass on each:

1. EUC-JP / EUC-KR silent mis-decode — agreed, this is the one that's an actual correctness gap rather than a savings tradeoff: 0xA1–0xFE sits entirely outside the 0x80–0x9F ambiguity guard, so it transcodes to wrong text with no U+FFFD signal. I'd like to fix it — treat a high density of bytes clustered in 0xA1–0xFE as another "ambiguous, don't guess → Binary" signal alongside the existing check, plus an explicit UTF-16/UTF-32 BOM sniff. Happy to fold it into this PR, or keep it as a focused fast-follow once this merges — your call on which keeps review easier.
2. Conservative CP1252 fallback (one stray 0x80–0x9F → Binary) — intentional, exactly the fidelity-over-hit-rate tradeoff you describe. Fine to leave as-is; the BOM/density work in (1) would also let this relax a bit without guessing.
3. Multi-blob passthrough — intentional and documented: windowing only the single-blob case avoids cutting a later blob mid-stream. Easy to extend to "window each blob that individually fits the budget" if multi-blob git show turns out common; I've not seen it in practice, so I left it as the safe gap.
4. blob_truncation's three usizes — agreed, a budget/max_recoverable transposition would compile silently. Cheap to guard with lightweight newtypes; I'll tidy that up.
5. CaptureBytes vs CaptureResult — I like your narrower alternative over genericizing CaptureResult (which would thread a type param through every *_cmd.rs): keep the two structs, factor the shared cmd.stdin(null); cmd.output() + exit-code plumbing into one private raw-capture helper that exec_capture decodes (lossy UTF-8) and exec_capture_bytes returns as-is. Removes the real duplication without touching any public shape.

I'll bundle (1), (4) and (5) — since none block merge, I'm glad to do them as a follow-up right after this lands, or push them here now if you'd prefer to review them together. Just say which.

Unrelated, only if it helps your batching: my other single-focus PRs off develop are green and ready whenever there's bandwidth — #3274 (just pushed a fix for your review there), #3266, #3258, #3259, #3275. No rush on any. Thanks again!

[KuSh]

Thanks for the thorough read, @KuSh — these are all fair, and I agree with the "fidelity over guessing" framing. Quick pass on each:

1. **EUC-JP / EUC-KR silent mis-decode** — agreed, this is the one that's an actual correctness gap rather than a savings tradeoff: `0xA1–0xFE` sits entirely outside the `0x80–0x9F` ambiguity guard, so it transcodes to wrong text with no `U+FFFD` signal. I'd like to fix it — treat a high density of bytes clustered in `0xA1–0xFE` as another "ambiguous, don't guess → `Binary`" signal alongside the existing check, plus an explicit UTF-16/UTF-32 BOM sniff. Happy to fold it into this PR, or keep it as a focused fast-follow once this merges — your call on which keeps review easier.

2. **Conservative CP1252 fallback** (one stray `0x80–0x9F` → `Binary`) — intentional, exactly the fidelity-over-hit-rate tradeoff you describe. Fine to leave as-is; the BOM/density work in (1) would also let this relax a bit without guessing.

Take a look at commit 57872f8 that just landed, you may find something useful there (also, encoding_rs has Encoding::for_bom() for free) ;)

I'll try to have a look at your other work as soon as I get time!

[breisnerlopez]

Rebased onto current develop (clean — the only collision was a keep-both test block in stream.rs), and used your pointers to harden decode_output. Thanks for the 57872f8 nudge — encoding_rs being a first-class, all-platform dep now made this much cleaner. New commit on top of the original:

1. EUC-JP/KR gap (the real correctness issue you flagged). Added looks_multibyte_cjk: a dense run of adjacent 0xA1–0xFE pairs is a legacy multi-byte CJK encoding (EUC-JP/KR, GBK/Big5), not Latin-1, so it returns Binary (raw passthrough) instead of transcoding to silent mojibake. Adjacency is what keeps it safe — real Latin-1 prose has essentially no adjacent high-byte pairs, so the .pck fixture and sparse accents stay Latin1. Documented limit: this targets substantially-CJK blobs (the whole-file case where every char mis-decodes); a few CJK bytes sprinkled into mostly-ASCII text stay below the density floor and fall back to Latin-1. Density alone can't tell that from a Latin-1 accent without a charset auto-detector, which encoding_rs doesn't ship — so I don't guess.

2. BOM handling via Encoding::for_bom (your hint): honor UTF-8/UTF-16 BOMs and decode them correctly rather than passing them through raw.

3. UTF-32 guard. encoding_rs has no UTF-32, and the UTF-32LE BOM (FF FE 00 00) shares its first two bytes with UTF-16LE — for_bom would decode the UTF-32 body as garbage. Rejected explicitly up front as Binary.

4. BOM sanity-check. A binary blob that merely starts with a UTF-16 BOM decodes without error into control-char garbage; text_looks_binary sends the common (NUL/control-dense) case back to raw passthrough. A clean BMP-text decode is kept (honoring a BOM is standard). Documented limit: catching a binary that decodes to clean text would need a full charset detector — disproportionate, since a bare FF FE is almost always genuine UTF-16.

New tests cover EUC-JP/KR → Binary, UTF-16/UTF-8 BOM decode, UTF-32 BOM → Binary, BOM-prefixed binary → Binary, and the two documented low-density limits. fmt/clippy/full suite green (2635).

On your other non-blocking nits (4 blob_truncation usize newtypes, 5 shared raw-capture helper): happy to fold those in here or as a fast-follow — your call on what keeps review easier.

[KuSh]

On the two open "your call" nits from before — please just fold both into this PR now, they're small:

• blob_truncation's budget/max_recoverable newtypes (or even a doc-comment + debug_assert!(budget <= max_recoverable) if newtypes feel heavy for one call site).
• The shared raw-capture helper for exec_capture/exec_capture_bytes in stream.rs.

Both are self-contained and low-risk enough that a fast-follow PR is more overhead than doing it here.

[KuSh]

Re-reviewed against the latest commit (b19d9b06b), after the CJK/BOM hardening. That part looks solid — nice use of for_bom and the adjacency-based CJK density check is a good call (sparse accents vs. whole-file CJK is exactly the right signal). Two correctness bypasses below block merge; the rest are non-blocking but worth a look.

[breisnerlopez]

Pushed a follow-up (f4b883a) addressing both blocking items and folding the two nits.

Both bypasses had the same root you pointed at — blob detection sat beside the other gates instead of ahead of them — so I collapsed routing into a single show_route() that decides the blob case first. --stat/--numstat/--pretty/--format with a <rev>:<blob> target now take the byte-safe path; I verified git emits the file byte-identically for --stat/--numstat/--shortstat/--pretty/--format, --pretty=format: with a colon in the value, and index/stage blobs (:file, :0:file) — all sha256-identical to a plain dump.

For the -- gate, blob_show_targets() now splits at the first -- and scans only the rev side. Wiring that surfaced a latent bug: run_show never actually received the -- — clap's trailing_var_arg strips it and, unlike run_diff/run_checkout, show wasn't calling restore_double_dash. Added it, which makes the split live and also fixes a mis-route of git show <rev> -- <path:with:colon> (was being dumped as a blob instead of a commit-diff).

Nits:

• Budget/MaxRecoverable newtypes on blob_truncation — an ordering assert doesn't fit here, since a recovery cap smaller than the budget is a valid config (tee.max_file_size), so the newtype is the right guard against transposition.
• Shared capture_raw() behind capture()/exec_capture_bytes(); the raw path now also emits the process terminated by signal N diagnostic it was dropping.
• looks_binary() samples head+tail, so a clean-headed blob with a binary tail past 8 KiB (no NUL) routes to passthrough instead of Latin-1 mojibake.
• Added a blob-windowing token-savings test, and a note on the tree -prefix false positive.

fmt / clippy --all-targets -D warnings / suite all green.

[KuSh]

Thanks for the follow-up work — I went through f4b883a line by line and re-verified each of my earlier points against real git 2.53. Five of the eight threads are genuinely closed and I've resolved them: the two blocking routing bugs (I confirmed live that --stat/--numstat/--shortstat/--pretty/--format on a blob target are sha256-identical to a plain dump, and that a trailing -- <path> still dumps the blob), the head-only binary scan, the tree false-positive note, and the signal diagnostic on the raw capture path. The Budget/MaxRecoverable newtypes and the shared capture_raw() helper both landed as discussed. fmt, clippy --all-targets -D warnings and cargo test --all are clean on my side too.

Three earlier threads I've left open, and a fresh pass on the new code turned up six issues — one of which I think is a genuine regression against develop. Requesting changes for that one; the rest are calibration, not blockers.

Still open from before

• git.rs:487 / git.rs:524 — the cross-file pointers (see core::tee::write_tee_file for the same fix, Mirrors the binary path in cloud/curl_cmd.rs) are unchanged. The why for max_recoverable moved from the blob_truncation doc to the MaxRecoverable newtype doc, which is an improvement, but the comments I flagged still read as "go look over there" rather than explaining themselves.
• git.rs:279 — stderr is still dropped on success in both the StatOrFormat and Blob branches. Pre-existing and minor, just not addressed.
• git.rs:2615 — test_blob_windowing_token_savings generates 600 formatted lines rather than using a real fixture, and that isn't cosmetic here: I measured tests/fixtures/git/blob_large.txt (the real fixture this PR ships) at 24.9% savings — 10 765 B → 8 165 B head. The synthetic input passes only because it's ~40 KB. A fixture large enough to clear the 60% floor would make the assertion mean something; as written it mostly asserts that 40 KB is bigger than 8 KiB.

New this pass — six inline comments below. #1 is the one I'd like fixed before merge: the whole-buffer Latin-1 fallback corrupts predominantly-UTF-8 files worse than the develop behaviour it replaces, on exactly the mixed-encoding legacy files this PR is aimed at. #2 and #4 are also worth a look since both change output for inputs that previously passed through untouched.

None of this takes away from the core of the PR — the routing rework is clean, the line math is right (including the no-trailing-newline case), the is_char_boundary walk-back is panic-safe, and I confirmed end-to-end recovery is byte-identical: head -122 out && tail -n +123 tee.log reproduces git show HEAD:big.txt exactly.

[breisnerlopez]

Thanks for the thorough pass. Follow-up in 3e07c40 — the blocking #1 plus two of the non-blockers, and an honest disposition on the rest.

Blocking #1 — whole-buffer Latin-1 on a single bad byte (stream.rs)

Fixed. Added utf8_multibyte_scan(bytes) -> (valid_mb, invalid) that counts well-formed ≥2-byte UTF-8 sequences vs bytes that fail to decode. In the Err(_) arm, before the C1 and CJK guards, when valid_mb > invalid the buffer is a UTF-8 file with sparse corruption, so it decodes via from_utf8_lossy (keep the text, replace only the bad bytes — develop's behavior) instead of Latin-1-transcoding the whole thing. It runs before the C1 guard on purpose: a valid UTF-8 continuation can fall in 0x80–0x9F (ć = C4 87), which the guard would otherwise dump as Binary.

Genuine Latin-1 has ~no well-formed multibyte runs (each accent is a lone high byte that fails as a lead), so valid_mb ≈ 0 there and this never steals the Latin-1 path.

I ran an adversarial review on this (it's a data/correctness change), executing against the real function:

• Natural CJK stays Binary — multi-KB real prose: EUC-JP 15 KB (vmb 3120 / inv 8080), GBK 9 KB (1320 / 6440), Big5 8.4 KB (600 / 4720), EUC-KR 7.3 KB (800 / 4640). Firing the new arm needs >~67% of non-ASCII chars to encode as valid 2-byte UTF-8; natural CJK measures 7–18% (4–9× margin). New test test_decode_output_large_dense_euc_jp_blob_is_binary (6 KB) locks this in.
• Latin-1 preserved — ISO-8859-1 (es/fr/de) and ISO-8859-15 (€) → Latin1, no U+FFFD.
• é case fixed — test_decode_output_mostly_utf8_with_bad_byte_stays_utf8 asserts UTF-8 out with no é.
• No panics (fuzzed utf8_multibyte_scan incl. truncated multibyte tails).

One honest caveat: a synthetic blob that cherry-picks only the Big5/GBK codepoints whose bytes happen to form valid UTF-8 (≈100% in the C2–DF/80–BF band, vs 7–18% natural) does escape to lossy where develop gave Binary — reproducible, but absent from real text, and reversible (the git blob is untouched; re-running without rtk recovers it). The clean way to close it is if looks_multibyte_cjk(bytes) { return Binary } before the new arm — but that would demote a genuine UTF-8 CJK file with sparse corruption from readable-lossy back to Binary passthrough, so I left the order prioritizing the common case. Happy to add the guard if you'd rather kill the corner.

#4 magic pathspecs (git.rs:385) — fixed

is_blob_show_arg now excludes :(…) and :!… (pathspecs, never blobs). Tests added for :(exclude)b.txt, :(top,glob)*.rs, :!b.txt. The -S 'a:b' option-value colon still trips it — disambiguating that needs git's per-flag arg grammar, so I documented it as the pre-existing false positive rather than guess.

git.rs:2615 synthetic test — fixed

Now windows a real large blob, src/main.rs (~120 KB — the exact git show HEAD:<src> case), giving 91.7% measured savings, well clear of the floor. (Went with main.rs over Cargo.lock to avoid coupling the assert to a churny generated file.)

Remaining non-blockers — disposition

• stream.rs:662 (BOM dropped) — acknowledged; for a preview the BOM is invisible noise, but it is a silent output change vs develop. I can re-prepend U+FEFF on the blob path if you want byte-fidelity — say the word.
• git.rs:480 (max_recoverable = 1 MB inverts windowing for >1 MB blobs) — this is the recovery-integrity tradeoff: the tee file is capped at max_file_size, so windowing a blob larger than it would promise a tail the tee can't hold. I'd rather not silently window-without-full-recovery; the right fix is a larger blob-specific recovery budget, which I'm happy to do as a separate PR.
• git.rs:308 (metrics overstated by transcode), git.rs:516 (orphan recovery file when never_worse discards), git.rs:487/524 (cross-file comment pointers), git.rs:279 (stderr dropped on success) — all noted; low-risk, no behavior impact on the filtered output. I can fold the two comment-doc ones and the orphan-file cleanup into this branch in a quick follow-up, or bundle everything as a post-merge fast-follow — your call.

fmt / clippy --all-targets -D warnings / full suite (2646) all green.

[KuSh]

Re-reviewed at 3e07c40 in a clean worktree. Gate is green: cargo fmt --check, cargo clippy --all-targets (zero warnings) and cargo test --all all pass.

Verified fixed — resolved

• The blocking whole-buffer Latin-1 re-decode. Confirmed with a before/after build of both heads rather than from the diff: at f4b883a a predominantly-UTF-8 blob came out as C3 83 C2 A9 (é); at 3e07c40 it is byte-identical C3 A9 (é). I also pushed the new valid_mb > invalid heuristic past the single repeated-kanji test that ships with it — real prose in EUC-JP, Shift_JIS, EUC-KR, GBK, GB18030 and Big5 all still route to Binary with a 3–8x margin, and French/German/Spanish ISO-8859-1 and CP1252 score valid_mb = 0, so genuine Latin-1 cannot be stolen by the new arm. The fix is sound.
• The synthetic savings fixture. Reproduced the number independently: src/main.rs gives exactly 91.7%, against 24.9% for the shipped blob_large.txt. Good call on the swap.

Still requesting changes

The is_blob_show_arg work is the problem. The partial fix landed :(, :!, and an inline comment waving off the option-value case as "a known (pre-existing) false positive" — but that reasoning does not survive the routing change this PR makes.

On develop the misclassification was genuinely harmless, because wants_blob_show ended in a plain print!("{}", result.stdout) — full passthrough. This PR sends the same misclassified args to ShowRoute::Blob, which windows at 8 KiB. The false positive is pre-existing; silently truncating a commit diff because of it is not. Two live instances below, one of them the :^ synonym the partial fix missed.

Also outstanding from the previous round and untouched, on their existing threads: the stripped UTF-8 BOM, the 1 MB max_recoverable cap that no-ops the feature on the largest blobs, metrics tracked against the transcoded string, and the tee file written before never_worse may discard it.

Everything below is reproduced against 3e07c40, not read off the diff.

[breisnerlopez]

Rebased onto develop and addressed the review. Summary of this push:

Both blockers

• :^ exclude synonym: is_blob_show_arg now excludes :^ alongside :!/:(/:/, so git show <commit> ':^src' routes CommitDiff (compacted) instead of a raw blob dump. Verified: :^src → 8685 B, same route as :!src.
• Separated-value flag operand: replaced blob_show_targets with show_positionals (walks args via git's own flag/value grammar, skipping the operand of -S/-G/-L/-O) + first-positional-only classification. git show -S 'a:b' <commit> is no longer misread as a blob and truncated.

Non-blockers: BOM preserved (decode only when windowing), tee-independent recovery hint git show '<rev>:<path>' | tail -n +N (dropped the 1 MB max_recoverable cap so the filter also applies to the giant lockfiles it targets), savings tracked against result.stdout.len() (raw bytes), blob stderr via decode_process_output, committed test fixture instead of include_str!(main.rs), doc comments state the rationale directly.

One thing I want to flag for your call — the is_terminal gate.

I'd added it to honor "truncation must not change a piped consumer's bytes", but on measuring it, it defeats the feature's purpose. rtk is invoked from the agent hook with stdout piped, so is_terminal() is false there and the blob path passed everything through unwindowed — 0 savings in exactly the scenario the filter exists for. Reproduced against a 9186 B blob (budget 8192):

path	bytes
rtk git show HEAD:file piped (with gate)	9186 (full, no savings)
git log piped	1006852 → 2965
git show HEAD (diff) piped	132084 → 30783

Only the blob path self-disabled in a pipe; diff/log/status all compact regardless. The gate is also not a sound safety signal: per hooks/init.rs, CI agents hand rtk a pseudo-TTY (is_terminal() returns true with no human present), so it neither reliably saves in real pipes nor reliably protects a > file.

So I removed the gate and aligned with diff/log: window large text blobs regardless of TTY, and rely on the | tail -n +N recovery hint for exact reconstruction — the same tradeoff git log | grep already makes. Small (≤ budget) and binary blobs still pass through byte-identically. After the change, a piped git show HEAD:file windows to 8265 B + hint.

If you still want pipe byte-exactness as a hard rule I'm happy to restore the gate and instead scope the PR's savings claims to interactive TTY — but flagging that it makes the feature inert for piped agents. Your call.

fmt/clippy (-D warnings)/test --all (3141+ tests) all green.

[KuSh]

Round 3, verified at 275f2450 in a dedicated worktree against 3e07c40 as the reference point. Gate is green — cargo fmt --check, cargo clippy --all-targets (0 warnings), cargo test --all (3141) — and still green merged with the latest develop, which has moved 12 commits past your merge-base (3154 tests, clippy clean).

Everything from round 2 is genuinely fixed

I ran each repro rather than reading the diff:

• -S 'url:1' and :^sub now take the commit-diff route — confirmed by the route marker ([full diff: rtk git diff --no-compact]), not by byte count, since a smaller output could equally have meant a blob truncation.
• BOM preserved byte-identically (ef bb bf).
• The 1 MB cap is gone: a 1.38 MB blob now windows, where it previously passed through whole.
• The reworked recovery hint reconstructs byte-exactly, and shell_single_quote handles ', $ and spaces correctly.
• decode_process_output on both stderr paths; track_bytes measures the bytes git actually wrote.

Dropping the tee for a self-describing git show <rev>:<path> | tail -n +N hint is a genuine improvement — it removed two of my previous findings structurally rather than patching them. Nice.

Two corrections I owe you

1. My 60% floor claim was wrong. I told you blob_large.txt's 24.9% was "far under the repo's 60% release-blocker floor". It is not. CONTRIBUTING.md:76 and :268 say 20%+, and docs/contributing/TECHNICAL.md:369/401/408 says the same — the sample there is literally assert!(savings >= 20.0). Only .claude/rules/cli-testing.md still says 60%, and it is stale on that point. 24.9% always cleared the real floor.

That one wrong finding cost you two rounds of fixture churn and pushed you off a real committed fixture onto 110 KB of synthetic filler — which the still-valid part of that same rules file lists as an anti-pattern ("use real command output, not synthetic data"). Sorry for the runaround. See the inline note on the test.

2. My is_terminal suggestion was wrong, and your revert is right. I verified it: a command run the way the agent hook runs it reports stdout isatty: False, so gating on a TTY would have made blob windowing inert in its primary scenario. Worth noting your commit message cites hooks/init.rs for this, but that comment guards stdin().is_terminal() for an interactive prompt — it is not evidence about stdout. The argument stands on its own without it. For what it is worth, the reason nobody hit this before is that every existing stdout().is_terminal() in the tree gates presentation only (--line-buffered, colour, curl formatting); this would have been the first gating content preservation. That distinction is worth a line in the comment, because the next reader will ask.

Still requesting changes

The classification bug class is now in its third round: round 1 :(exclude), round 2 :^ and -S 'url:1', and now two more instances. Rather than filing them one at a time I fuzzed it — 732 generated git show invocations, differential against real git:

• 3 misroutes — commit diff truncated into the 8 KiB blob window (all clustered shorts: -pwG, -wpG, -wG … -I)
• 15 silent savings losses — a ~23 KB commit diff passed through completely uncompacted: no error, no marker, zero savings
• 0 broken recovery hints among correctly-routed blobs

That is 18/732 ≈ 2.5% of realistic invocations. The second bucket is the one that worries me, because it is invisible — the filter simply does nothing and nothing says so. The useful negative result: the fuzzer found no new classes; every hit traces to the two root causes below.

Details and the suggested shape are inline. Full findings list, ranked, with the two blocking ones first.

[breisnerlopez]

Thanks for the depth of the round-3 review — and for the two corrections, both taken:

• 20% floor: reverted the fixture churn. Deleted the 110 KB synthetic filler and put test_blob_windowing_token_savings back on the real blob_large.txt (24.9% clears the true 20% floor); fixed the stale comment.
• is_terminal rationale: the comment no longer cites hooks/init.rs (that guard is on stdin()). It now states the real argument — the hook always pipes stdout (isatty: False) — and notes this would have been the first content-preserving is_terminal gate, vs. the presentation-only ones elsewhere.

The classification class — closed at the root, with the fuzzer as the proof

You were right that an allowlist mirroring git's grammar keeps drifting. Two changes:

1. cat-file probe per candidate. run_show now probes every rev:path positional with git cat-file -t (built via git_cmd(global_args)); only an exact blob windows, and it takes the byte-safe raw path whenever any positional resolves to a blob, so a routing miss can't send a blob through the lossy commit-diff decode. Removed the raw.starts_with("tree ") content-sniff (subsumed). TODO(after #3681).
2. Cluster-aware operand skip. The two new instances you'd expect — --ignore-matching-lines and the short-flag clusters (-pS, -wG, -pI, -pwG, -wpG) — were a selection gap: the probe rejects the mis-selected x:y, but without skipping the cluster's operand the real blob never got probed (silent loss for UTF-8, and mojibake for Latin-1 via the commit-diff path — I reproduced both). flag_token_consumes_next now walks a short cluster's tail and consumes its value inline (-Sfoo) or as the next token (-wG x), deriving its char set from the existing value-flag table (single source of truth). Not a hand-rolled tokenizer — a TODO(after #3681) marks it for ValueSpec replacement.

Windowing gate: positionals.len()==1 && blob_objects.len()==1 — so git show HEAD HEAD:blob and A:x B:y pass through byte-identically rather than truncating a concatenated stream.

Differential fuzzer, committed (tests/git_show_blob_differential_test.rs): hermetic temp repo, fixed-seed SplitMix64, 600 invocations diffed against git cat-file -t — misroutes=0, silent_losses=0, fidelity_breaks=0. I extended build_invocation to cross the colon-flag clusters with real UTF-8 and Latin-1 blobs (the cross your round-3 note showed was the unsampled gap); the extended fuzzer fails on the pre-fix commit (2/3), which is what earns the "no other classes hiding" claim.

Fidelity invariant (round-3 blocker)

Windowing now happens only when from_utf8(&stdout) is Ok — i.e. the shown head is a byte-exact prefix and | tail -n +N reconstructs exactly. Lossy-UTF-8, Latin-1, UTF-16/BOM, and binary all take emit_raw_bytes_passthrough byte-identically. That orphaned decode_output and its Latin-1/CJK/BOM helpers (used nowhere else), so I deleted them (~420 lines) rather than leave dead code; the Latin-1 fixture test now asserts passthrough, documenting why exact recovery is impossible once transcoded.

Other round-3 items

• Hint global args: git <globals> show '<arg>' | tail — runs from outside the repo (-C, -c, --git-dir, --work-tree), tested.
• Content-transforming flags: --textconv/--filters/--ext-diff force passthrough (the hint can't reproduce a transformed dump). Trailing -- <pathspec> likewise (git ignores it for a blob, but passthrough is airtight).
• Tracking: saved = input - output is signed now; a blob that emits more than git records the honest negative instead of clamping to 0%. Reader casts guarded with .max(0).

Honest residual

A long value-flag in space form that isn't in the table (--ignore-matching-lines a:b HEAD:big) leaves a:b as a phantom positional → the combo passes through byte-identically (no savings, but no corruption and no misroute). The short/-I/cluster forms window correctly; this asymmetry is the conservative side and is fuzzer-covered as byte-identical. #3681's ValueSpec factorization is the place that closes it for good.

Rebased onto the current develop (0 conflicts). fmt / clippy (-D warnings) / test --all all green.

[KuSh]

Round 6 (landing) — APPROVED, follow-ups filed

Claim: git show <rev>:<path> on a large blob is windowed to an 8 KiB head with an exact recovery hint, instead of dumping the whole file.
Scope: accept. (Derived and frozen now — the earlier rounds predate the current template.)
Ran: 851 unique git show invocations diffed against real git (UTF-8 / Latin-1 / UTF-16+BOM / binary blobs crossed with colon-bearing flag values and short-flag clusters); recovery round-trip; -C from outside the repo; gate on the head. Savings 24.9% vs the 20% floor (CONTRIBUTING.md).

Blocks merge (0)

Both round-3 blockers are closed, verified by execution rather than by reading the diff.

Classification class. My own fuzzer, written independently of the one you committed, on 851 invocations: misroutes 0, fidelity breaks 0, bad/inexact hints 0, silent savings losses 0. Round 3 was 3 and 15. The two instances that round found are gone:

--ignore-matching-lines a:b   raw 22838 -> rtk 5736   (commit-diff route, compacted)
-pS / -pwG / -wpG / -pI       no blob window, no truncation

The cat-file probe is the right call — it decides by asking git rather than mirroring its grammar, which is what kept drifting.

Fidelity invariant. "Window only when from_utf8 is Ok" holds exactly:

big_lossy.txt  25990 B  byte-identical   (was ef bf bd corruption at round 3)
u16.txt        56580 B  byte-identical   (was transcoded, BOM stripped)
latin1.txt     25089 B  byte-identical
bin.dat        20480 B  byte-identical
utf8_big.txt   32289 -> 8230 B  windowed, head is a byte-exact prefix, `| tail -n +103` reconstructs exactly

Deleting the now-orphaned decode_output and its helpers rather than leaving ~420 lines of dead code was the right instinct.

Also verified: the hint carries global args (git '-C' '<path>' show '<arg>' | tail -n +N runs from outside the repo); tracking records signed savings (input - output as i64, no clamp); the savings test is back on the real blob_large.txt at 24.9% against a >= 20.0 assert with the synthetic filler deleted; the is_terminal comment now states the real argument and the content-vs-presentation distinction. Gate green on the head: fmt, clippy -D warnings 0, 3459 tests. PR sits exactly on the develop tip, 0 commits behind.

Filed as follow-ups

• Value-flag table gaps — #3954 — --notes, --conflict-marker-size, --submodule take a separated value but are not in the table, so the operand becomes a phantom positional and the real blob is never windowed. I confirmed your disclosed asymmetry, and that it is benign in both directions:

  --ignore-matching-lines a:b HEAD:big.txt   32289 -> 32289  byte-identical (no savings)
  --ws-error-highlight all HEAD:big.txt      32289 ->  8225  windows correctly
  

  Savings gap, never corruption — the conservative side, exactly as you described. Worth knowing this is the same class already filed three times against rtk grep's VALUE_FLAGS_SHORT (#3882, #3881, #2253, #3663), where it produces wrong answers rather than merely lost savings; the cat-file probe is what keeps the git instance benign. Adjacent but distinct from #3910, which is about output shape routing rather than operand consumption. Both close via #3681's ValueSpec, which your TODO(after #3681) already points at.
• Trailing newline on empty output — recorded as a comment on #3924 rather than a new issue, since that one already frames this exact byte-exactness question (and #3303 / #3659 are the same class). println! at the stat/format branch emits a lone \n where git emits nothing. Not a behaviour change you introduced: the idiom is unchanged from develop, and it only became reachable because the probe correctly stopped treating HEAD:x as a blob.

Checked and correct — no need to re-verify

cat-file probe built via git_cmd(global_args); the positionals.len()==1 && blob_objects.len()==1 windowing gate; recovery offset arithmetic and shell_single_quote (verified against ', $, spaces in round 3, unchanged); content-transforming flags (--textconv/--filters/--ext-diff) forced to passthrough; your committed differential fuzzer (3 tests, passes, and I confirmed the extended version is what earns the "no other classes" claim).

Hypotheses built and dropped

• 96 "fidelity breaks" from my first fuzzer run — every one was the 1-byte trailing newline above, not a fidelity break. My detector was too strict; re-run with it excluded gives 0.
• Unaccented French in the new FEATURES.md warning — the file has zero accented characters across 1447 lines, so you matched its existing convention. Not a defect.

Next

Nothing — approving. Two follow-up issues filed and linked above; both are #3681's territory or cosmetic. Thanks for the depth on this one: the cat-file probe and the committed differential fuzzer between them turn a class that produced findings in three consecutive rounds into one with a standing regression test.

Rounds: 6 (landing). Threads: 0 open, 6 resolved this round.

Dismissing my own approval: CI fails on all three runners at 06bae5a with a regression my round-3 finding introduced. Details in the comment below. Paused pending a maintainer decision on the telemetry endpoint.

[KuSh]

Approval dismissed — on hold, and this one is my fault

CI fails on all three runners at 06bae5ab, deterministically and with the same assertion:

core::telemetry::tests::test_enriched_stats_returns_valid_data
thread panicked at src/core/telemetry.rs:589:9:
assertion failed: (0.0..=100.0).contains(&stats.avg_savings_per_command)

ubuntu, macos and windows, identical. I approved on a green local cargo test --all (3459 passing) and that was premature — see the last section.

Root cause: my round-3 finding

You implemented exactly what I asked for at tracking.rs:523:

- let saved = input_tokens.saturating_sub(output_tokens);
+ let saved = input_tokens as i64 - output_tokens as i64;

You then clamped the aggregate token-count readers with .max(0) and deliberately let per-command savings_pct keep the honest negative — which is the right reading of the finding. But avg_savings_per_command() at tracking.rs:1333 does an unclamped AVG(savings_pct):

SELECT COALESCE(AVG(avg_sav), 0.0) FROM (
    SELECT rtk_cmd, AVG(savings_pct) as avg_sav
    FROM commands WHERE input_tokens > 0 GROUP BY rtk_cmd)

and telemetry.rs:589 asserts that result is within 0.0..=100.0. Once any row carries a negative savings_pct, the average can go below zero and the assertion trips.

Reproduced deterministically:

$ RTK_DB_PATH=<empty.db> cargo test --bin rtk test_enriched_stats_returns_valid_data
test result: ok. 1 passed

# seed one row with savings_pct = -50.0
$ RTK_DB_PATH=<seeded.db> cargo test --bin rtk test_enriched_stats_returns_valid_data
test result: FAILED. 0 passed; 1 failed

It stayed invisible in my local gate because that test reads the real user DB through Tracker::new(), and mine happened to hold no negative rows. CI records commands as the suite runs, so on a clean machine it is deterministic. A test whose outcome depends on the developer's own database is worth a separate look — it will hide the next regression of this shape too.

Please hold — the fix is a design call, not a patch

Two ways to close it, and they disagree about what the metric means:

1. Clamp avg_savings_per_command to 0, consistent with the four sibling readers you already clamped. But it re-hides exactly the signal the finding asked to surface, one level up.
2. Widen the assertion to allow negatives. The metric is genuinely signed now, and a negative average is meaningful — filters enlarging output on average is a real thing worth seeing.

I lean towards 2, since the honest-negative behaviour was the point. What stops me committing to it: avg_savings_per_command ships in the telemetry payload, and docs/TELEMETRY.md:66 documents it as a plain average with no stated range. Whether the receiving endpoint tolerates a negative there is not something I can determine from the repo.

So this is paused on the maintainer confirming what the telemetry endpoint accepts for that field. Once that is settled the fix is a couple of lines either way, and nothing else in the PR is affected — the blob-show work all still verifies (851-invocation differential run clean, byte-fidelity invariant holding, gate otherwise green).

Sorry for the churn: the finding was right, the approval was not. I should have grepped every reader of savings_pct the moment the field became signed, rather than trusting a local run of an environment-dependent test.

[breisnerlopez]

Pushed a fix for the CI failure that dismissed the approval.

Root cause. This PR's signed record() change (9da3270) lets a net-regressing filter store a negative savings_pct, so both overall_savings_pct and avg_savings_per_command are now legitimately negative — but two telemetry tests still asserted (0.0..=100.0) against the real user DB (Tracker::new()), panicking deterministically on any machine with a net-negative row. That's why test_enriched_stats_returns_valid_data failed on all three runners; its twin test_get_stats_returns_tuple (same assertion on overall_savings_pct, same real-DB pattern) was one row away from the same fate.

Fix. Instead of clamping the readers, I widened both assertions to the real invariant — a saving never exceeds 100% — and dropped the stale 0 floor. This keeps the honest signed savings from round 3 (already locked in for overall_savings_pct by test_record_reflects_worsening_not_zero), consistent across both fields, instead of re-hiding a regressing filter behind 0%. Added a hermetic in-memory test pinning that a worsening command yields a negative aggregate.

I went widen-not-clamp because the payload already sends negative overall_savings_pct to the collector today, so this PR's own design already answers the endpoint-tolerance question. If your collector actually rejects negatives, the alternative is a one-line clamp on both readers instead — your call, since only you can see the endpoint side.

Re-requesting review. fmt + clippy -D warnings + the full unit-test binary are green locally.

(You also flagged that these telemetry tests read the developer's real DB — I kept that out of scope here, but happy to make them fully hermetic in a follow-up if you'd like.)

[KuSh]

Round 7 (landing) — APPROVED

Claim: git show <rev>:<path> on a large blob is windowed to an 8 KiB head with an exact recovery hint, instead of dumping the whole file.
Scope: accept (frozen).
CI @96a8ecf1: all 11 green — test on ubuntu, macos and windows, the three that failed at 06bae5ab.
Merged with the latest develop (75 commits ahead of the merge-base): merges clean, clippy -D warnings 0, 3565 tests pass.

The blocker is closed

1ba6f1a6 took the option the maintainer has now confirmed: the telemetry endpoint already accepts negative values, so widening the assertions rather than clamping the metric was right.

It also went further than the round-6 comment asked. I only named test_enriched_stats_returns_valid_data; you found its twin test_get_stats_returns_tuple, bounded both at the real invariant (<= 100.0) rather than restoring an arbitrary floor, documented the signed contract on avg_savings_per_command, and added a hermetic Tracker::new_in_memory() test. I checked that last one pins what it claims instead of trusting it — re-clamping avg_savings_per_command to 0 makes test_avg_savings_per_command_reflects_negative fail. Being hermetic, it also sidesteps the "reads the developer's own DB" problem that let the original regression through.

Fixup pushed: 96a8ecf1

One arm of the same class was still open, and it is mine, not yours — my round-3 finding is what made saved_tokens signed, and I should have swept every reader when I asked for it.

Seeding a DB with a net-negative row, test_get_stats_returns_tuple still panicked at assert!(saved_24h >= 0) — three lines above the assertion you widened:

thread '...test_get_stats_returns_tuple' panicked at src/core/telemetry.rs:572:9:
assertion failed: saved_24h >= 0

tokens_saved_24h and total_tokens_saved both return i64 from an unclamped SUM(saved_tokens), deliberately — unlike the as usize readers that take .max(0) for their unsigned API. So both sums go honestly negative.

Worth recording why the fix drops the assertions rather than replacing them: my first attempt asserted saved_24h <= saved_total, which is false. With a positive row inside the 24h window and a larger negative one outside it, saved_24h (90) exceeds saved_total (-110). They are unbounded signed sums with no ordering between them, so the test stops binding them via let (cmds, top, pct, ..) and a comment records the reason.

Verified before pushing: both telemetry tests pass against a net-negative DB, fmt ok, clippy 0, 3460 tests, and the blob-show differential suite still green.

Checked and correct — no need to re-verify

The blob-show work is untouched by the last two commits (telemetry/tracking only) and still verifies: cat-file probe classification, the byte-fidelity invariant (window only when from_utf8 is Ok), exact | tail -n +N recovery including global args, and your committed differential fuzzer (3 tests green).

Hypotheses built and dropped

• saved_24h <= saved_total as a replacement invariant — disproved by construction before it shipped; see above.

Your questions

• "Pushed a fix for the CI failure that dismissed the approval." → Confirmed fixed, plus the one remaining arm folded into 96a8ecf1.

Next

Nothing — approving. Follow-ups already filed: #3954 (value-flag table gaps, closes via #3681) and a note on #3924 (trailing newline on empty output). Both are out of this PR's scope.

Seven rounds is far more than this should have taken, and at least two of them were my doing — the 60% floor that sent you through two fixture churns, and the signed-savings finding whose readers I failed to sweep. Thank you for the patience, and for consistently fixing the class rather than the instance I happened to name.

Rounds: 7 (landing). Threads: 0 open, 0 resolved this round.