Repository navigation
fix(diff): keep +/- markers at column 0 in condense_unified_diff - #121
Merged
Merged
Conversation
The stdin diff path (git diff | rtk diff -) emitted changed lines two-space indented, pushing their +/- markers to column 2. Anchored greps over the output (grep -E '^[+-]') matched nothing — a silent false negative for any script or CI gate built on diff conventions. Same class as the compact_diff fix in #118 (upstream rtk-ai#3646), same design rule applied: change lines at column 0; the [file] header and overflow line are unchanged and never start with +/-. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review response. Three findings, all reproduced before fixing.
1. The multi-file flush site was untested. condense_unified_diff flushes a
file's changes at two places — once per `+++` for the preceding file, once
after the loop for the last one. The regression test used a single-file
diff, which only ever reaches the second. Mutation-proved: reverting the
in-loop site alone left the entire suite green, so every file but the last
would still emit indented markers with nothing to catch it. The test now
uses a two-file diff and fails against that mutant.
2. Change lines whose content starts with `---`/`+++` were dropped outright
and excluded from the (+N -M) counter. Only `--- a/f` and `+++ b/f` (with
the trailing space) are headers, and those are already consumed by the
branch above. Markdown rules and front-matter, `++i`, SQL `-- comment` all
hit this. Same silent-loss class this PR set out to close, four lines below
the edit, so the invariant was not actually delivered:
before: [file] README.md (+1 -1) <- ---/+++ lines gone, count wrong
after: [file] README.md (+2 -2) <- all four lines, column 0
3. The new test asserted no line starts with a space. The ` ... +N more`
trailer is legitimately indented, so that held only for diffs of ten or
fewer changes and would have become a false failure with a misleading
message. Scoped to change lines.
Also drops the per-line String clone for `result.append(&mut changes)` (the
repo's anti-pattern table calls out cloning in the hot path), and the
redundant `: Vec<String>` — inference comes from `changes.push(line.to_string())`,
not from the removed `format!`, which takes any Display and types nothing.
Gate: fmt / clippy --all-targets / test --all all exit 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 2 review response. The previous commit's guard clauses were dead code: deleting both left the whole suite green. Anything starting with `--- ` or `+++ ` is consumed by the outer `if` before reaching them, so the earlier fix only rescued the bare `---`/`+++` forms and the comment it added claimed a `-- comment` case it did not actually cover. Two live silent-loss paths remained, both reproduced against the built binary: $ printf '...--- Legacy bootstrap\n--- DO NOT RUN\n- email TEXT,\n' | rtk diff - before: [file] m.sql (+0 -1) <- two lines gone, counter wrong after: [file] m.sql (+0 -3) <- all three, column 0 $ printf '...+++ new section marker\n-- removed a feature\n' | rtk diff - before: [file] new section marker (+0 -1) <- file renamed to user content after: [file] c.md (+1 -1) `---`/`+++` cannot be classified by prefix: `--- a/f` is a header, and a removed line whose content is `-- note` is `--- note` on the wire. Position settles it for free — a header only ever appears outside a hunk. Track that with one bool, so inside a hunk every +/- line is content with no exceptions, and the two tautological guards disappear. Mutation-proved: removing either `!in_hunk` gate kills its test. Hits SQL/Lua/Haskell/Ada comments, mail signature delimiters, Markdown rules and front-matter fences, and `++i`. Also rewords the trailer comment from the previous commit, which called the `... +N more` line "legitimately indented" — that trailer reports hidden lines when nothing is ever truncated. Stating the mechanical fact instead, so the comment does not bless it. Gate: fmt / clippy --all-targets / test --all all exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 3 review response, and a regression I introduced in the previous commit.
Keying the hunk close on `diff --git` stranded every producer that spells the
separator differently. `in_hunk` stayed stuck true past the first hunk, so
files 2..N had their `---`/`+++` headers classified as content: one merged
[file] entry under the wrong name, an inflated counter, header lines injected
into the change list. Worse than the merge base, which got these right.
`git show --cc` on a two-file merge — git's own output:
base: [file] f1.txt (+1 -1) head: [file] f1.txt (+3 -3)
- MAIN1 - MAIN1
++MERGED1 ++MERGED1
[file] f2.txt (+1 -1) --- a/f2.txt
- MAIN2 +++ b/f2.txt
++MERGED2 - MAIN2
++MERGED2
Same collapse on `diff -ru` and svn `Index:`.
What closes a hunk is the format invariant, not a list of separators: every
line inside a hunk carries a ` `, `+`, `-` or `\` marker, so any other line
has left it. That covers `diff --git`, `diff --cc`, `diff -ru`, `diff -r`,
`Index:` and anything else without needing an entry per tool. Empty stays
neutral so a context line stripped of its trailing space does not end the hunk
early.
All three producers now match the base's file count, with markers at column 0.
Mutation-proved: deleting the reset fails the new boundary test. It survived
mutation before — the previous tests all used `any(|l| l == want)` on lines
that were still emitted, just under the wrong file, so nothing pinned the
[file] count across a boundary.
Gate: fmt / clippy --all-targets / test --all all exit 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kylehgc
marked this pull request as draft
August 29, 2026 23:35
…ow trailer
Round 4 review response, and the fix for a second regression of my own.
Closing hunks on a marker set broke the most canonical input there is: a plain
POSIX `diff -u` patch has no separator line at all between files, and the next
file's `--- a/bar.c` is itself a valid hunk-body marker. So files 2..N were
folded into the first, headers leaked into the change list, counters inflated:
base: [file] foo.c (+1 -1) head: [file] foo.c (+3 -3)
[file] bar.c (+1 -1) --- a/bar.c
+++ b/bar.c <- bar.c entry gone
That was the third separator-shaped heuristic in a row, each one moving the
breakage rather than removing it. The format already answers the question:
`@@ -a,b +c,d @@` declares exactly how many old and new lines follow. Count
them down and the hunk ends where the format says it ends — no list of
producers to keep extending. Combined diffs (`@@@`) carry a second parent
column the budget does not model, so they keep the marker rule as a fallback.
Verified against base on every producer that has broken so far — plain
`diff -u`, `diff --git`, `diff --cc`, `diff -ru`, svn `Index:` — all now match
base's file count, with markers at column 0 and `---`/`+++` content preserved.
Also gates +/- counting on being inside a hunk. `git format-patch` puts a bare
`---` before the diffstat and a `-- ` signature before the git version, both
outside any hunk; base counted the signature as a removed line. On a real
two-patch mbox: base (+83 -25), head (+83 -24) — the extra removal was the
signature.
Drops the `... +N more` trailer. Nothing is truncated — the loop emits every
change line — so it announced withheld lines that were already on screen, and
an agent reading it re-runs the raw command. Its test asserted "only 10 shown",
a display cap removed long ago; rewritten to pin what is actually true.
Adds the token-savings test cli-testing.md requires, on a real captured diff
fixture. Measured 8.6%: this filter strips headers and emits every change line
verbatim by design, so it is nowhere near the 60% floor that truncating filters
meet. The assertion is a regression floor plus a fidelity check that no change
line is dropped. See the PR for why the floor does not fit this filter.
Fixes a fixture that could not occur in reality: multiple_files had no `@@` at
all, so it locked in counting outside a hunk.
Mutation-proved, all three previously-surviving guards now killed:
budget close -> plain_patch_multi_file, mbox_separators
is_empty guard -> blank_line_does_not_end_hunk
`\` marker -> no_newline_marker_stays_in_hunk
Gate: fmt / clippy --all-targets / test --all all exit 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 5 review response. The line budget was trusted absolutely, so a patch
whose declared counts do not match its body lost content silently — the exact
outcome this function's own contract forbids, and worse than the merge base.
Both directions were reachable and both are real. Hand-maintained queues
(debian/patches, Nix, Homebrew), LKML posts, model-generated patches and
diffs truncated mid-stream all carry stale counts; `git apply --recount` and
patch(1)'s recount tolerance exist because of it.
under-declared body outruns the budget -> trailing changes dropped, counter
adjusted to match, no warning
over-declared budget never closes -> on a plain patch, where it is the only
close, the next file's headers are swallowed as content and
that file disappears
Guessing a better rule is what the previous four rounds did, each trading one
failure mode for another. This does not guess: a mismatch is detected and
answered with the fallback rust-patterns.md mandates for a filter that cannot
do its job — return the input untouched. Worst case is no compression. There
is no case where a line is lost.
Detection is exact, from the two places a mismatch can surface: budget still
owing when the next `@@` opens or at EOF, and a marked line appearing outside
every hunk. The two `format-patch` artifacts that legitimately sit outside a
hunk, the bare `---` before the diffstat and the `-- ` version delimiter, are
excluded by value.
Verified the fallback stays out of the way of real input: `git diff`,
`git log -p`, `format-patch` mbox, `show --cc`, `diff -ru`, svn `Index:`,
plain `diff -u` and the captured fixture all still compress, none falls back.
Also from round 5:
- `@@ -0,0 +0,0 @@` left the hunk open one line too long, eating the next
file's `--- ` header as a removal. The close is evaluated at the top of the
loop now, before the line is classified.
- Dropped the `@@@` early-return in parse_hunk_counts: it was dead code, since
`strip_prefix("@@ ")` already rejects `@@@`. The comment said otherwise,
which would have misled anyone loosening that prefix.
- Savings floor raised 5.0 -> 8.0 (measured 8.6%); a 40% relative regression
used to pass.
- Removed a comment defending the `... +N more` trailer this branch deletes.
- Corrected four fixtures that declared counts their bodies never matched.
They were untestable shapes: real producers always emit accurate counts, and
with mismatch detection those fixtures now correctly return raw.
Mutation-proved, all three round-5 survivors killed:
marker-rule close -> combined_diff_multi_file (first test of the @@@ path)
`\` arm no-op -> no_newline_marker_stays_in_hunk
in_hunk gate on + -> added_line_past_budget_passes_through
Gate: fmt / clippy --all-targets / test --all all exit 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reverts the classifier exploration (5bcee1c..1f64783 plus the guard half of 6375941) back to the b008c53 behavior, keeping only what six adversarial review rounds never faulted: - both flush sites emit change lines at column 0, moved with result.append(&mut changes) instead of a per-line clone - the two-file regression test that pins BOTH flush sites (mutation-proved: reverting either site alone fails it) - the anti-indent assertion scoped to change lines - the Vec<String> annotation stays gone; its stated justification was wrong (inference comes from changes.push(line.to_string()), not the format!) The classifier attempts each traded one failure mode for another — separator rules broke --cc/-ru/Index:, then plain diff -u; the trusted line budget dropped content on stale counts; the mismatch fallback false-fired on 38% of real git format-patch output (unindented commit-message bullets). Critical findings rose round over round instead of falling. That work is preserved in this branch's history and distilled into claudedocs/diff-classifier-review-2026-08-29.md as the design brief for its own ticket; classification is a real parsing problem and deserves one. Gate: fmt / clippy --all-targets / test --all all exit 0. Verified against the base binary: 0 of 12 real format-patch outputs fall back (the fallback no longer exists), column-0 markers hold on multi-file diffs. Co-Authored-By: Claude Opus 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 #120. Original fix — upstream PR at rtk-ai#3687 (companion to #118 / upstream rtk-ai#3646, which fixed the same class in
compact_diff).Bug
The stdin diff path (
git diff | rtk diff -,run_stdin→condense_unified_diff) emitted changed lines asformat!(" {}", c). The+/-markers landed at column 2, so anchored greps (grep -E '^[+-]') over the output matched nothing — a silent false negative for scripts and CI gates built on diff conventions.Fix
Emit change lines at column 0 by moving them with
result.append(&mut changes)— they already carry their markers, and the move also drops a per-lineStringclone flagged by the repo's anti-pattern table.[file] name (+N -M)headers and the... +N moreoverflow line are unchanged; neither starts with+/-when produced by real git output, so anchored greps see the change lines.Verification
printf '<unified diff>' | rtk diff - | grep -cE '^[+-]'→ 0. After: 4 on a two-file diff, all markers at column 0.+++for the preceding file, once after the loop for the last). A single-file fixture only reaches the second — mutation testing showed reverting the first site alone left the entire suite green. The two-file fixture kills both mutants." +"/" -"), because the... +N moretrailer is legitimately indented and a blanket check goes false past ten changes.cargo fmt --all && cargo clippy --all-targets && cargo test --all— all exit 0.Review history, in the open
Six rounds of adversarial review ran against this branch (uncontaminated runs of the repo's code-reviewer prompt plus mutation testing and base-vs-head A/B on real producers). None of them faulted the fix above. They did surface that the underlying line classifier has deeper problems —
--- notecontent lines dropped,+++ textrenaming files, and more — and commits5bcee1c..1f64783on this branch explored a rewrite. Each attempt traded one failure mode for another (the last one false-fell-back on 38% of realgit format-patchoutput), so that work is reverted in1a11e13and preserved two ways:claudedocs/diff-classifier-review-2026-08-29.md(added here) distills all six rounds into reproducers and a recommended approach, as the design brief for a dedicated ticket.Correction to an earlier revision of this description: it claimed a
Vec<String>annotation replaced inference lost with the removedformat!. That was wrong — inference comes fromchanges.push(line.to_string())— and the annotation is gone.Deliberately not fixed (all pre-existing, documented in the design brief)
--- note/+++ notecontent-line loss;+++ /dev/nullnaming on deletions; binary/rename-only files emitting nothing;--colorinput producing empty output; non-UTF-8 stdin hard-erroring with no fallback; the... +N moretrailer firing when nothing was truncated. Each is reproducible from the brief; none is made worse by this change.