diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index c933445a1d..082300968f 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -607,7 +607,7 @@ export const REES_ANALYZERS = [ network: "Calls the GitHub API for each changed file's content at headSha. Requires headSha and token forwarding for private repos.", notes: - "Complements `duplication` (which flags NEW duplication introduced) with the reverse, before/after signal. Per-file only in this version: a duplicate pair split across two different files is not detected. Uses a greedy (not globally optimal) old-to-new block assignment, which can rarely under-report a resolved pair as still-present in multi-candidate scenarios -- an acknowledged v1 heuristic limit, not a correctness/data-integrity issue.", + "Complements `duplication` (which flags NEW duplication introduced) with the reverse, before/after signal. Per-file only in this version: a duplicate pair split across two different files is not detected. Uses a maximum-bipartite-matching (augmenting-path) old-to-new block assignment, so asymmetric multi-candidate scenarios are resolved optimally rather than greedily.", }, }, { diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index e43379ee99..3c35820fad 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -691,7 +691,7 @@ "looksAt": "Each changed file's pre-PR content (reconstructed from its patch) compared against its own post-PR content, using the same chunk-normalization + suffix-automaton matcher as the `duplication` analyzer.", "reports": "The file and the pre-PR locations of the resolved duplicate pair, plus the matched line count. Never file contents.", "network": "Calls the GitHub API for each changed file's content at headSha. Requires headSha and token forwarding for private repos.", - "notes": "Complements `duplication` (which flags NEW duplication introduced) with the reverse, before/after signal. Per-file only in this version: a duplicate pair split across two different files is not detected. Uses a greedy (not globally optimal) old-to-new block assignment, which can rarely under-report a resolved pair as still-present in multi-candidate scenarios -- an acknowledged v1 heuristic limit, not a correctness/data-integrity issue." + "notes": "Complements `duplication` (which flags NEW duplication introduced) with the reverse, before/after signal. Per-file only in this version: a duplicate pair split across two different files is not detected. Uses a maximum-bipartite-matching (augmenting-path) old-to-new block assignment, so asymmetric multi-candidate scenarios are resolved optimally rather than greedily." } }, { diff --git a/review-enrichment/src/analyzers/duplication-delta.ts b/review-enrichment/src/analyzers/duplication-delta.ts index ba776406ed..36acb37cd7 100644 --- a/review-enrichment/src/analyzers/duplication-delta.ts +++ b/review-enrichment/src/analyzers/duplication-delta.ts @@ -9,13 +9,13 @@ // duplicate" is identical between the add-detector and this remove-detector — never a second, differently-tuned // similarity algorithm running side by side with the first. // -// Scope (v1): PER FILE only. For each changed file, find pairs of near-identical blocks that existed in its -// RECONSTRUCTED OLD content, then GREEDILY ASSIGN each old block (in file order) to an unclaimed matching NEW -// block. That assignment step matters: if OLD had two near-identical copies of some text and NEW keeps exactly -// one, a naive "does this old block's text exist ANYWHERE in NEW" check would say BOTH old copies "survive" (the -// single remaining occurrence matches either query) and the resolved-duplication signal would never fire. Greedy -// assignment lets only as many old blocks "survive" as there are still-distinct matching occurrences in NEW; any -// old block left unclaimed — but that WAS part of an old duplicate pair — is the resolved-duplication finding. +// Scope: PER FILE only. For each changed file, find pairs of near-identical blocks that existed in its +// RECONSTRUCTED OLD content, then assign each old block to an unclaimed matching NEW block via a maximum +// bipartite matching (#4812) over the old/new candidate graph — never a naive per-block "does this old block's +// text exist ANYWHERE in NEW" check, which would say BOTH old copies of a since-deduped block "survive" (the +// single remaining occurrence matches either query) and the resolved-duplication signal would never fire. The +// matching lets only as many old blocks "survive" as there are still-distinct matching occurrences in NEW; any +// old block left unmatched — but that WAS part of an old duplicate pair — is the resolved-duplication finding. // Cross-file duplication removal (the twin lived in a DIFFERENT file, changed or not) is NOT detected — // deliberately out of scope for this version (see PR description for the follow-up), not silently approximated. // @@ -132,44 +132,72 @@ export function findInternalDuplicatePairs( return pairs; } -/** Greedily assign each OLD block (in file order) to an UNCLAIMED matching NEW block (>= MIN_RUN significant - * lines), each NEW block usable by at most one OLD block. This is what lets duplicate COUNT reductions show up: - * if OLD had N near-identical copies of some text and NEW retains only M (M < N), exactly M of the N old blocks - * claim a surviving NEW occurrence and the remaining (N - M) do not — instead of every old copy independently - * matching the SAME still-present text and all appearing to "survive". Returns a parallel boolean array over - * `oldBlocks`. An aborted signal stops early; every OLD block not yet processed is left `false` ("not confirmed - * to survive") rather than risk reporting a stale/partial comparison as conclusive. +/** Assign each OLD block to an UNCLAIMED matching NEW block (>= MIN_RUN significant lines) via a true MAXIMUM + * bipartite matching — Kuhn's algorithm, an O(V*E) augmenting-path search over the old-block/new-index + * candidate graph — each NEW block usable by at most one OLD block. This is what lets duplicate COUNT + * reductions show up: if OLD had N near-identical copies of some text and NEW retains only M (M < N), exactly M + * of the N old blocks claim a surviving NEW occurrence and the remaining (N - M) do not — instead of every old + * copy independently matching the SAME still-present text and all appearing to "survive". Returns a parallel + * boolean array over `oldBlocks`. * - * KNOWN v1 LIMITATION: this is a greedy, order-dependent assignment, not a globally optimal bipartite matching. - * In a multi-candidate scenario where old blocks match NEW occurrences asymmetrically (e.g. old block A matches - * BOTH remaining new occurrences but old block B matches only one of them), first-come-first-claimed can let A - * grab the occurrence B needed, leaving B unmatched even though a different (optimal) assignment would have - * paired both. The practical effect is a false "resolved" report for a duplicate pair that is, in fact, still - * present — never a crash or a data-integrity issue, since this is an ADVISORY-ONLY signal (see epic #4737's - * design constraints) that never gates anything. A true maximum-bipartite-matching algorithm (e.g. an - * augmenting-path search) would close this gap; tracked as a separate, non-urgent follow-up rather than - * attempted here. */ + * A maximum matching (rather than a greedy, order-dependent one) is required because old-to-new candidacy can be + * asymmetric: if old block A matches BOTH of two remaining NEW occurrences but old block B matches only ONE of + * them, a first-come-first-claimed walk can let A grab the occurrence B needed, under-reporting a pair that is, + * in fact, still present (#4812, closing a known v1 heuristic gap). Kuhn's algorithm avoids this by re-routing + * an already-matched old block onto a different NEW occurrence it also fits, whenever doing so frees up an + * augmenting path for the old block currently being placed — so every old block that COULD be matched, IS + * matched. + * + * Two-phase, so the abort-signal contract stays exactly what it was: phase 1 builds the full old×new adjacency + * matrix (the only part that calls `longestSharedRun`, i.e. the only part that can be slow or need aborting); + * phase 2 runs Kuhn's search purely over that already-known, in-memory boolean matrix (no further comparison + * work, so nothing left to abort). An aborted signal — checked before starting, and again before/after every + * `longestSharedRun` call in phase 1 — discards the whole in-progress matrix and returns all-`false` rather than + * run the matching over incomplete candidacy data, which could silently under-report a survivor exactly the way + * the old greedy version could. */ export function assignSurvivors( oldBlocks: NormBlock[], newIndices: MatchIndex[], signal: AbortSignal | undefined, ): boolean[] { - const claimed = new Array(newIndices.length).fill(false); const survived = new Array(oldBlocks.length).fill(false); + if (signal?.aborted) return survived; + + // Phase 1: the full candidacy matrix, `adjacency[i][n]` true iff old block i shares a >= MIN_RUN run with new + // index n. Any abort here discards everything (returns all-`false`) — a partially built matrix under-counts + // real edges, and matching over it would silently reintroduce the old greedy bug in a new shape. + const adjacency: boolean[][] = []; for (let i = 0; i < oldBlocks.length; i += 1) { if (signal?.aborted) return survived; + const row = new Array(newIndices.length).fill(false); for (let n = 0; n < newIndices.length; n += 1) { - if (claimed[n]) continue; if (signal?.aborted) return survived; const run = longestSharedRun(oldBlocks[i]!, newIndices[n]!, signal); if (run?.status === "aborted") return survived; - if (run?.status === "matched") { - claimed[n] = true; - survived[i] = true; - break; + if (run?.status === "matched") row[n] = true; + } + adjacency.push(row); + } + + // Phase 2: Kuhn's algorithm. `matchOf[n]` is the OLD block index currently claiming NEW index `n` (-1 = free). + // `visited` is scoped to one top-level old block's augmenting search so a NEW index already tried (and + // rejected) THIS search is never revisited, but is fair game for the NEXT old block's own search. + const matchOf = new Array(newIndices.length).fill(-1); + const tryAugment = (i: number, visited: boolean[]): boolean => { + for (let n = 0; n < newIndices.length; n += 1) { + if (!adjacency[i]![n] || visited[n]) continue; + visited[n] = true; + if (matchOf[n] === -1 || tryAugment(matchOf[n]!, visited)) { + matchOf[n] = i; + return true; } } + return false; + }; + for (let i = 0; i < oldBlocks.length; i += 1) { + survived[i] = tryAugment(i, new Array(newIndices.length).fill(false)); } + return survived; } diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index f5770e99bc..694144ab38 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -544,7 +544,7 @@ export const ANALYZER_DESCRIPTORS = [ network: "Calls the GitHub API for each changed file's content at headSha. Requires headSha and token forwarding for private repos.", notes: - "Complements `duplication` (which flags NEW duplication introduced) with the reverse, before/after signal. Per-file only in this version: a duplicate pair split across two different files is not detected. Uses a greedy (not globally optimal) old-to-new block assignment, which can rarely under-report a resolved pair as still-present in multi-candidate scenarios -- an acknowledged v1 heuristic limit, not a correctness/data-integrity issue.", + "Complements `duplication` (which flags NEW duplication introduced) with the reverse, before/after signal. Per-file only in this version: a duplicate pair split across two different files is not detected. Uses a maximum-bipartite-matching (augmenting-path) old-to-new block assignment, so asymmetric multi-candidate scenarios are resolved optimally rather than greedily.", }, render: (findings, helpers) => { if (!findings.length) return []; diff --git a/review-enrichment/test/duplication-delta.test.ts b/review-enrichment/test/duplication-delta.test.ts index bd8dd2289e..687ffa1fe7 100644 --- a/review-enrichment/test/duplication-delta.test.ts +++ b/review-enrichment/test/duplication-delta.test.ts @@ -28,6 +28,20 @@ const DUP_BLOCK = [ const HEADER = "const fileHeaderMarkerForFixture = 'v1'"; const TRAILER = "const fileTrailerMarkerForFixture = 'end'"; +// A second run of exactly MIN_RUN significant lines, sharing NO line text at all with DUP_BLOCK — used to build +// asymmetric-candidacy fixtures for assignSurvivors (#4812): a block combining DUP_BLOCK + OTHER_BLOCK content +// matches a NEW occurrence of EITHER one, while a block containing only one of them matches just that one. +const OTHER_BLOCK = [ + "const totalStakeWeightForValidator = validatorStats.baseWeight * validatorStats.uptimeFactor", + "const boundedStakeWeightValue = Math.min(validatorStats.maxWeight, Math.max(0, totalStakeWeightForValidator))", + "const consensusBonusApplied = validatorStats.consensusBonus * validatorStats.agreementFactor", + "const slashPenaltyApplied = validatorStats.slashCount * validatorStats.slashWeight", + "const settledStakeAmount = boundedStakeWeightValue + consensusBonusApplied - slashPenaltyApplied", + "const finalStakeRounded = Math.round(settledStakeAmount * 1000000) / 1000000", + "const safeFinalStake = Number.isFinite(finalStakeRounded) ? finalStakeRounded : 0", + "const persistedStakeForValidator = safeFinalStake", +]; + // Build a synthetic (patch, oldContent, newContent) trio for a PURE CONTIGUOUS DELETION: oldLines = prefixLines + // removedLines + suffixLines; newLines = prefixLines + suffixLines. reconstructOldContent only ever reads the // hunk header's `+` (new-file) start number — never the `-` side — so a single-hunk, all-removed-lines patch @@ -204,6 +218,120 @@ test("assignSurvivors: an already-aborted signal leaves every block unconfirmed assert.deepEqual(assignSurvivors(oldBlocks, newIndices, AbortSignal.abort()), [false]); }); +// ── assignSurvivors: maximum-matching regression (#4812) ──────────────────── + +test("assignSurvivors: asymmetric candidacy (#4812) — old A matches BOTH remaining new occurrences, old B matches only ONE — a maximum matching lets BOTH survive", () => { + // This is the exact scenario #4812 describes. Old block A's content is DUP_BLOCK immediately followed by + // OTHER_BLOCK, so it independently shares a full MIN_RUN run with EACH new occurrence; old block B's content is + // only DUP_BLOCK, so it shares a run with ONLY the DUP_BLOCK new occurrence. + // + // A first-come-first-claimed (greedy) walk processes A first, finds N1 (DUP_BLOCK) already matches on its FIRST + // candidate check, and claims it immediately without ever considering that A could equally take N2 instead -- + // leaving B, which has no other candidate, permanently unmatched. Verified empirically against the actual + // pre-fix greedy implementation: it returns [true, false] for this exact fixture, not [true, true]. + // + // A maximum matching finds the better assignment (A -> N2, B -> N1, via one augmenting-path re-route) and + // reports both as surviving -- closing the false "resolved duplication" gap #4812 tracks. + const oldA = { norm: [...DUP_BLOCK, ...OTHER_BLOCK], lineNos: Array.from({ length: 16 }, (_, i) => i + 1) }; + const oldB = { norm: DUP_BLOCK, lineNos: [50, 51, 52, 53, 54, 55, 56, 57] }; + const newN1 = buildMatchIndex({ norm: DUP_BLOCK, lineNos: [3, 4, 5, 6, 7, 8, 9, 10] }); + const newN2 = buildMatchIndex({ norm: OTHER_BLOCK, lineNos: [20, 21, 22, 23, 24, 25, 26, 27] }); + assert.deepEqual(assignSurvivors([oldA, oldB], [newN1, newN2], undefined), [true, true]); +}); + +test("assignSurvivors: an odd number (3) of old blocks against two new occurrences — the augmenting search re-routes to match two, the un-matchable third correctly does not survive", () => { + // Extends the asymmetric fixture with a THIRD old block (C) that, like B, matches ONLY the DUP_BLOCK new + // occurrence. Only 2 new occurrences exist, so at most 2 of the 3 old blocks can ever survive -- exercises the + // augmenting-path search's OWN re-route failing (C tries to displace B from N1, recurses into B's search, which + // exhausts every new index and returns false) in addition to the re-route that DOES succeed (B displacing A). + const oldA = { norm: [...DUP_BLOCK, ...OTHER_BLOCK], lineNos: Array.from({ length: 16 }, (_, i) => i + 1) }; + const oldB = { norm: DUP_BLOCK, lineNos: [50, 51, 52, 53, 54, 55, 56, 57] }; + const oldC = { norm: DUP_BLOCK, lineNos: [60, 61, 62, 63, 64, 65, 66, 67] }; + const newN1 = buildMatchIndex({ norm: DUP_BLOCK, lineNos: [3, 4, 5, 6, 7, 8, 9, 10] }); + const newN2 = buildMatchIndex({ norm: OTHER_BLOCK, lineNos: [20, 21, 22, 23, 24, 25, 26, 27] }); + assert.deepEqual(assignSurvivors([oldA, oldB, oldC], [newN1, newN2], undefined), [true, true, false]); +}); + +test("assignSurvivors: an odd number (3) of identical old blocks against a single new occurrence — exactly one survives", () => { + const oldBlocks = [ + { norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }, + { norm: DUP_BLOCK, lineNos: [20, 21, 22, 23, 24, 25, 26, 27] }, + { norm: DUP_BLOCK, lineNos: [40, 41, 42, 43, 44, 45, 46, 47] }, + ]; + const newIndices = [buildMatchIndex({ norm: DUP_BLOCK, lineNos: [3, 4, 5, 6, 7, 8, 9, 10] })]; + assert.deepEqual(assignSurvivors(oldBlocks, newIndices, undefined), [true, false, false]); +}); + +test("assignSurvivors: an odd number (3) of new occurrences, most non-matching (decoys) — same result as if the decoys were absent", () => { + const oldBlocks = [ + { norm: DUP_BLOCK, lineNos: [3, 4, 5, 6, 7, 8, 9, 10] }, + { norm: DUP_BLOCK, lineNos: [12, 13, 14, 15, 16, 17, 18, 19] }, + ]; + const newIndices = [ + buildMatchIndex({ norm: OTHER_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }), // decoy: does not match either old block + buildMatchIndex({ norm: DUP_BLOCK, lineNos: [20, 21, 22, 23, 24, 25, 26, 27] }), // the one real match + buildMatchIndex({ norm: ["a totally unrelated short filler line here"], lineNos: [40] }), // decoy: too short to match + ]; + assert.deepEqual(assignSurvivors(oldBlocks, newIndices, undefined), [true, false]); +}); + +test("assignSurvivors: a signal that aborts on the OUTER per-old-block checkpoint during phase 1 discards the whole result, including an already-confirmed row", () => { + // Read #5 lands on the outer per-old-block abort check for i=1 -- AFTER old block 0's adjacency row (which DOES + // match) has already been fully computed (reads #1-4: top-level check, i=0's outer check, i=0/n=0's inner + // check, then longestSharedRun's own a=0 poll). Even though old block 0 would clearly survive, a mid-build + // abort must discard everything computed so far in phase 1 rather than let phase 2 match over an incomplete + // adjacency matrix -- which could silently under-report a survivor exactly like the old greedy bug. + let reads = 0; + const fakeSignal = { + get aborted() { + reads += 1; + return reads === 5; + }, + }; + const oldBlocks = [ + { norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }, + { norm: DUP_BLOCK, lineNos: [20, 21, 22, 23, 24, 25, 26, 27] }, + ]; + const newIndices = [buildMatchIndex({ norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] })]; + assert.deepEqual(assignSurvivors(oldBlocks, newIndices, fakeSignal), [false, false]); +}); + +test("assignSurvivors: a signal that aborts on the INNER per-new-index checkpoint during phase 1 discards the whole result", () => { + // Read #5 lands on the inner per-new-index abort check for n=1, after n=0 was already compared for the only old + // block (reads #1-4: top-level, outer i=0, inner n=0, longestSharedRun's a=0 poll for n=0). Exercises the INNER + // loop's own checkpoint, a distinct line from the outer per-old-block one covered by the prior test. + let reads = 0; + const fakeSignal = { + get aborted() { + reads += 1; + return reads === 5; + }, + }; + const oldBlocks = [{ norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }]; + const newIndices = [ + buildMatchIndex({ norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }), + buildMatchIndex({ norm: OTHER_BLOCK, lineNos: [20, 21, 22, 23, 24, 25, 26, 27] }), + ]; + assert.deepEqual(assignSurvivors(oldBlocks, newIndices, fakeSignal), [false]); +}); + +test("assignSurvivors: a signal that flips aborted INSIDE a longestSharedRun call itself (not either loop's own checkpoint) still discards the whole result", () => { + // Read #4 lands on longestSharedRun's own internal poll (its first line, a=0) for the only old/new pair -- + // AFTER assignSurvivors' own pre-checks (top-level, outer i=0, inner n=0) already saw "not aborted" (reads + // #1-3). Exercises the `run?.status === "aborted"` branch specifically, distinct from either loop's own + // checkpoint covered by the two prior tests. + let reads = 0; + const fakeSignal = { + get aborted() { + reads += 1; + return reads === 4; + }, + }; + const oldBlocks = [{ norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }]; + const newIndices = [buildMatchIndex({ norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] })]; + assert.deepEqual(assignSurvivors(oldBlocks, newIndices, fakeSignal), [false]); +}); + // ── scanDuplicationDelta: fail-safe guards ────────────────────────────────── test("scanDuplicationDelta: fails safe with no githubToken", async () => {