Fix attribution and performance bugs in hunk-level significance matching - #140
Merged
Conversation
… crediting
consumeAndShift() decided whether to keep a tracked line's AI attribution
at the whole-hunk level: if a hunk's overall old-vs-new text was similar
enough ("insignificant"), it credited *every* new line in that hunk's
range to the tasklet that owned the one tracked line inside it — not just
the specific line that actually matched.
That's fine for the common case (a hunk that's just the tracked line
itself, tweaked). But git diff has no move detection, so a restructuring
can bundle a wide, mostly-unrelated span of lines into one hunk that still
scores as "similar overall" (most of the text is still present, just
reshuffled or interleaved with something else nearby). When that happens,
lines the tracked tasklet never touched — anything else that happened to
land in the same hunk — were getting mislabeled as AI-generated.
Fixed by matching each tracked line to the specific new line its own
content actually corresponds to (exact match first, then best BLEU-similar
candidate), instead of crediting the whole hunk indiscriminately. If
nothing in the hunk resembles it anymore, the line's attribution is
dropped rather than guessed at.
Verified with a real git-based repro: an AI-written line and an adjacent,
never-AI-attributed comment edited in the same commit (no blank line
between them, so git bundles both into one hunk). Before the fix, both
lines were credited to the AI tasklet; after, only the one it actually
wrote is.
…al parent Code review finding on this PR: extractSnapshot() decided whether a snapshot was diffing against a prior AI edit by checking isAiChange(chain[index - 1]) — the array-adjacent entry. That's only a valid stand-in for "this snapshot's actual parent" on a simple linear chain. getTracyChain() does a BFS over (possibly multiple) parents to support squash-merged chains (see its doc comment), so on a chain with a merge point, array-adjacent entries can be siblings from different branches rather than parent/child. Concretely: a branch A snapshot whose real parent is the base (non-AI) commit could end up array-adjacent to an unrelated branch B AI snapshot after the BFS-then-reverse ordering. The significance check would then wrongly treat branch A's edit as AI-to-AI and skip filtering, letting a trivial edit (that should have been filtered as a User->AI change) get attributed. The diff base itself (diffFromTree) was already resolved correctly via snapshot.parentHash — only the "is this AI-authored" check was using the wrong reference. Fixed by resolving the real parent from snapshot.parentHash and looking it up in the chain by hash, instead of assuming array adjacency. Multi-parent snapshots (the merge commit itself) keep the old array-adjacent fallback, since diffFromTree already silently falls back to it too (getCommitTree rejects the space-joined multi-parent string), so the two stay consistent. Verified with a real git-based repro: a squash-merged two-branch chain where branch A's tiny edit and branch B's substantial edit land array- adjacent to each other. Before the fix, branch A's trivial edit was wrongly attributed; after, only branch B's real edit is.
EsmeYi
changed the title
Code review finding on this PR: findMatchingNewLineIndex matched each tracked old line independently via findIndex, which always resolves to the FIRST matching new line. If an insignificant hunk contains two duplicate AI-attributed old lines (common: `}`, `return;`, identical log statements), both independent lookups landed on the same new position. The Set-based survivor collection then silently dropped the second one — and if the two duplicates belonged to different tasklets (consumeAndShift is called separately per tasklet's own Change), the later call's line could overwrite the earlier tasklet's legitimate claim entirely. Replaced the independent per-line lookup with alignHunkLines(), which aligns a hunk's old and new lines one-to-one and in order for the whole hunk at once: exact (whitespace-insensitive) matches are aligned via LCS, which respects both order and duplicate multiplicity, so two `}` lines in the old text land on two different `}` lines in the new text rather than both on the first one. Anything left over falls back to the best remaining (not-yet-used) BLEU-similar candidate. The alignment is a pure function of the hunk's own content, memoized per hunk within a single consumeAndShift call — and since it doesn't depend on which specific old line is being queried, two separate calls against the same hunk (e.g. for two different tasklets) independently arrive at the same consistent mapping. Verified with two real git-based repros: two duplicate AI-written lines from the SAME tasklet bundled into one hunk (before: the second one vanished; after: both survive at distinct positions), and two duplicate lines from DIFFERENT tasklets bundled together (before: one tasklet's line was silently absorbed into the other's; after: each keeps its own).
Code review finding on this PR: alignHunkLines() unconditionally built a full (n+1)x(m+1) LCS table to align a hunk's old and new lines. A reformat, reorder, or bulk rename across a large AI-generated file can easily put thousands of lines into a single hunk (whitespace is stripped before the significance check, so a pure reindent stays "insignificant" and lands right in this path) — at that scale the DP table means tens to hundreds of millions of number slots for one hunk alone. Confirmed empirically: at 8,000 lines the old table pushed peak memory to ~620MB for a single hunk, growing quadratically from there. The prior implementation (independent per-line findIndex lookups) was linear, so this was a real regression, not a pre-existing cost. Replaced the LCS table with an O(n+m) hash-based pass for exact (whitespace-insensitive) matches: group new-line indices by content, then consume each group in order as old lines are walked. This still gives each duplicate line (`}`, `return;`, identical log statements) its own distinct occurrence — the property the LCS table existed for — without building a quadratic table. It's not strictly optimal the way LCS is (a pathological case with duplicates AND genuine reordering could pick a less-intuitive pairing), but it never produces a collision or an incorrect cross-tasklet overwrite, which is what actually mattered. The BLEU fuzzy-match fallback stays inherently O(unmatched x unmatched), so it's now bounded by FUZZY_MATCH_SEARCH_CAP (200,000 candidate pairs): past that, leftover lines are left unmatched rather than guessed at, consistent with this function's existing "drop rather than guess" philosophy. In practice this pass has little to do anyway once exact matching (which alone handles ordinary reformatting/reindentation) runs first. Verified all six previously-added repro scenarios (from this PR and the one before it) still produce identical results. Added a performance regression test: a 5,000-line single-hunk reindent now resolves in ~0.5s with ~0MB heap growth; the same scenario against the old LCS implementation took ~195MB of heap growth, confirming the test catches the regression this fix addresses.
Code review finding on this PR: the exact-match pass in alignHunkLines() (added to fix the O(n*m) LCS table) grouped new-line indices by content into buckets, then consumed each bucket with Array.prototype.shift(). shift() shifts every remaining element down by one index, so it's O(k) per call — and a hunk with many identical lines (`}`, blank lines, templated log statements) means the SAME bucket gets shifted repeatedly, for a total cost of O(k^2) on that bucket alone. The previous performance test used lines with unique content (each line embeds its own index), so every bucket had exactly one entry and never exercised this path. Confirmed empirically with an isolated microbenchmark of shift() alone: 30k elements ~50ms, 60k ~219ms, 120k ~901ms — a clean quadratic curve. Reproduced in the full pipeline with a 300,000-line single-hunk, all-identical-content reindent: ~11.7s pre-fix vs ~6.1s after. Fixed by tracking a read cursor per content bucket instead of mutating the array — each consumption becomes an O(1) map lookup plus array index, with no shifting. Every previously-passing scenario (six repro tests across this PR and the one before it) still produces identical results. Added a dedicated large-hunk-with-repeated-content performance test, separate from the existing large-unique-content one, since that one doesn't build large buckets and can't catch this specific cost.
Code review finding on this PR: the BLEU fuzzy-match fallback gated the ENTIRE pass on the total unmatched-old x unmatched-new product against a fixed cap (FUZZY_MATCH_SEARCH_CAP = 200,000). Past that cap, the whole fallback was skipped rather than searching less — so a hunk with enough purely-unmatched lines (e.g. a uniform field rename applied throughout a large AI-generated block, ~450 lines on each side with none exact- matching post-rename) lost attribution for the ENTIRE hunk, not just the lines a bounded search couldn't resolve. This is exactly the case the fallback exists to handle, so the cap turned a precision trade-off into a functional regression: confirmed empirically with a 500-line uniform rename (500*500 = 250,000, over the cap) — only the 2 untouched lines (def/return) kept attribution; the 500 renamed lines were dropped entirely. Replaced the total-size gate with a fixed-size search window per old line: a restructuring or in-place rename rarely moves a line far from its proportional position in the hunk, so each unmatched old line searches outward from its expected position (estimated by linear scaling across the hunk) instead of scanning every remaining candidate. This bounds cost per line regardless of hunk size — no all-or-nothing cliff — while still giving every old line a real, local search. The window alone reintroduced a different cost: always scanning the full window even after finding an obviously-correct match at offset 0 (the common case for an in-place rename) made a 5,000-line rename take ~67s. Added an early exit: once a match already above the significance threshold has gone unbeaten for 20 consecutive offsets, stop searching. Brought the same case down to ~3.5s while keeping identical results. Verified: the 500-line rename now correctly attributes all 502 lines (previously only 2). Re-verified all six repro scenarios from this PR and #139 still produce identical results, and the two existing performance tests (large unique-content and large repeated-content hunks) are unaffected. Added a committed regression test for this exact scenario.
EsmeYi
changed the title
Code review finding on this PR: the fixed-size proportional estimate assumes a hunk's line-count change is spread evenly across it. When it's actually concentrated at one end — e.g. a block of lines inserted right before a large renamed section — an old line near that end gets an estimate off by roughly the insertion size. Combined with the early exit, if something else in the hunk happens to be near-duplicate content (templated/generated code often is), the search could settle on that wrong occurrence before ever reaching the real match, which sits further out but is still within the window. Not a missed match — a wrong one. Two changes: 1. Estimate the expected position by interpolating between the nearest exact-match anchors on either side of the gap (already found by the earlier exact-match pass) instead of a single global proportional scale. An anchor's position is exactly correct by construction, so this corrects for insertions/deletions concentrated anywhere in the hunk, not just ones spread evenly. Falls back to the hunk's own boundaries when there are no anchors nearby (a hunk with no exact matches at all, which is exactly the case this whole fallback exists for). 2. Scale the early-exit patience by the local line-count drift for that gap (nextAnchorNew - prevAnchorNew vs nextAnchorOld - prevAnchorOld), so the search can't settle before at least reaching the position the gap's own count change implies — and prefer the later candidate on an exact score tie, so that when a nearby wrong candidate and the position accounting for the drift score identically, the one that reflects the actual shift wins instead of whichever was found first. Verified with a repro that isolates the mechanism: 50 lines inserted right before a 500-line renamed block, reusing indices 0..49 so the inserted lines become byte-identical to the first 50 lines of the real (shifted) block — the sharpest version of "templated lines similar enough to collide". Before this fix, all 50 of those lines were pulled onto the inserted duplicate instead of the real block; after, none are. Re-verified all seven prior repro scenarios and all three performance tests in this PR still produce identical results.
…ontent Code review finding on this PR: the previous commit's tie-break (prefer the farther-explored candidate on an exact score tie, so a real match found after accounting for drift wins over an earlier, wrong one) treated every tie as "progress" and reset the patience counter. A hunk where every old line is identical to every other — a real pattern: the same templated statement repeated verbatim thousands of times — and gets uniformly renamed means every old line's fuzzy search sees thousands of candidates that all score identically. Patience never accumulates, so every single line scans the full search window regardless of the patience setting. Confirmed empirically: ~26s for a 5,000-line uniform rename where every line is identical, versus ~0.4-3.5s for the earlier tests that used unique-per-line content and never actually exercised a long run of ties. Fixed by separating "does this become the new pick" from "does this count as progress": a strict score improvement does both (updates the pick, resets patience). An exact tie still updates the pick — preserving the drift-direction tie-break from the previous commit — but does NOT reset patience, so a long run of tied candidates no longer defeats the exit. Brought the 5,000-line uniform-rename case down to ~2s (from ~26s) while keeping correctness: re-verified the previous commit's drift/tie-break repro (50 lines inserted before a 500-line rename, duplicate indices) still has zero misattribution, and all other repro and performance scenarios in this PR are unaffected. Added a dedicated regression test for this exact case.
Code review finding on this PR: always preferring the farther-explored candidate on an exact score tie was only correct when the gap actually had a size change to account for. When it didn't (no insertion/deletion — oldCount === newCount for the gap, an in-place rename with no lines added or removed), the estimate is already exactly right, and "prefer farther" has no justification — it just walks every tied pick outward toward the edge of the search window for no reason. Concretely: 1,000 identical lines "record = load(user_id=value)" uniformly renamed to userId — old line i's correct position is new line i, but tie-breaking toward "farthest explored" pushed every pick to roughly i + patience, losing the earliest lines entirely (repro showed the lowest attributed line at 4 instead of 1, and 991/1002 lines instead of 1002). Fixed by making the tie-break target the position implied by the gap's own SIGNED line-count change (positive for a net insertion, negative for a deletion, zero when nothing shifted) instead of unconditionally preferring "farther". On a tie, the candidate whose offset from the estimate is closest to that signed drift wins — closest to the estimate itself when nothing shifted (fixing this bug), and still closest to the drift-corrected position when something did (preserving the previous commit's fix for the insertion case). Ties still don't count as progress for the patience counter, so the earlier fix for long tie-runs stalling the search is unaffected. Verified: the no-drift uniform-rename case now attributes the full, unshifted range [1, 1002] with no loss. Re-verified the drift/tie-break insertion repro from two commits ago still has zero misattribution, and all other repro and performance scenarios in this PR are unaffected. Strengthened the existing large-uniform-rename test to check the attributed line range (not just the count), since a count-only check is exactly what let this regression through undetected.
Code review finding on this PR (confirmed as a real, mirror-image regression of the previous commit's own fix): biasing ties toward signedLocalDrift is a single number for the WHOLE gap — it has no way to know WHERE within the gap an insertion or deletion actually happened. The previous commit's own repro put the extra lines BEFORE the AI's block, so biasing toward "+drift" happened to be correct. Mirrored the same construction with the extra lines AFTER the block instead (50 lines, identical to the AI's own content, appended right after it): the AI's 1,000 lines need no shift at all, but the same "+drift" bias walked lines near the end of the block into the appended, non-AI region, since that region is exactly as textually similar to the AI's own lines as the AI's own lines are to each other. Confirmed empirically: 50 lines pulled into the appended tail with the code as of the previous commit. Root cause traced further than the tie-break alone: the anchor- interpolated ESTIMATE itself already assumes a gap's size change is spread proportionally across it, which is also wrong when concentrated at one end — for old lines near that end, no tie-break fix alone could undo an estimate that already overshoots. Fixed by deciding, per old line, whether reaching for the gap's drift is warranted at all — based on how many OTHER old lines share that exact text. A rare old line (occurs only a handful of times) is what a real "moved/renamed block vs. one stray duplicate elsewhere" situation looks like (the previous commit's repro: exactly one decoy per rare index), and both the estimate and the tie-break target account for the full drift for those. Text that instead repeats many times over (uniform/templated content) gets an estimate assuming NO shift (1:1 against the nearest anchor) and ties prefer whichever candidate is closest to THAT — there's no way to tell, from content alone, which of many identical occurrences is this line's own, and assuming a shift risks walking into a same- looking but unrelated region. Verified against all three relevant scenarios: the new append-after repro (0 lines pulled into the tail, previously 50), the original insert-before repro from two commits ago (0 misattribution, unchanged), and the no-drift uniform-rename case (still the full, unshifted [1, 1002] range). Re-verified all seven correctness repro scenarios and all five performance tests in this PR still produce identical results.
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
1 participant
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.
Summary
Ten related fixes to
extractSnapshot()/consumeAndShift()'s significance-filtering and line-matching, all about attribution ending up on the wrong line/tasklet (or the process falling over, or a whole hunk losing attribution) rather than just being imprecise.1–9 (see commit history for full detail on each)
10. Drift bias walking uniform content into an unrelated appended tail
Caught in review as a mirror-image regression of fix #9: biasing ties toward
signedLocalDriftis a single number for the WHOLE gap — it has no way to know WHERE within the gap an insertion/deletion actually happened. Fix #7's own repro put the extra lines BEFORE the AI's block, so biasing toward "+drift" happened to be correct there. Mirroring that construction with the extra lines AFTER the block instead (50 lines identical to the AI's own content, appended right after it) showed the AI's 1,000 lines needed no shift at all, but the same bias still walked lines near the end of the block into the appended, non-AI region — confirmed empirically: 50 lines pulled into the tail.Traced the root cause further than the tie-break alone: the anchor-interpolated estimate itself already assumes a gap's size change is spread proportionally across it, which is also wrong when concentrated at one end.
Fixed by deciding, per old line, whether reaching for the gap's drift is warranted at all — based on how many OTHER old lines share that exact text. A rare line (occurs only a handful of times) is what a real "moved block vs. one stray duplicate elsewhere" situation looks like, and both the estimate and the tie-break account for the full drift there. Text that repeats many times over (uniform/templated content) gets an estimate assuming NO shift, with ties preferring whichever candidate is closest to that instead — there's no way to tell, from content alone, which of many identical occurrences is this line's own.
Verified against all three relevant scenarios (new append-after repro, original insert-before repro, no-drift uniform-rename case) — all correct. Also extracted
bleuSimilarity()out ofcomputeHunkSignificance()inutils.tsso the hunk-level and line-level comparisons share one implementation.Test plan
npm run compile/npm run lint/npm run test:unit(53 tests) cleangit diff --checkclean (no whitespace issues)