Skip to content

fix(rees): replace greedy duplication-delta assignment with maximum matching - #4961

Merged
JSONbored merged 1 commit into
mainfrom
fix/4812-duplication-delta-max-matching
Jul 11, 2026
Merged

fix(rees): replace greedy duplication-delta assignment with maximum matching#4961
JSONbored merged 1 commit into
mainfrom
fix/4812-duplication-delta-max-matching

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Replaces assignSurvivors's greedy, order-dependent old-to-new block assignment in the duplicationDelta REES analyzer with a true maximum bipartite matching (Kuhn's algorithm, an O(V·E) augmenting-path search over the old/new candidate graph).
  • Closes a documented v1 accuracy gap: in an asymmetric multi-candidate scenario (old block A matches both of two remaining new occurrences, old block B matches only one of them), the greedy first-come-first-claimed walk could let A grab the occurrence B needed, under-reporting a still-present duplicate pair as "resolved". The maximum matching re-routes an already-matched old block to a different new occurrence it also fits whenever that frees an augmenting path, so every old block that could be matched, is.
  • Preserves the exact function signature and the existing abort-signal fail-safe contract (an aborted signal discards everything computed so far rather than reporting a partial/stale result as conclusive), restructured into two phases: phase 1 builds the full old×new adjacency matrix (the only part that calls longestSharedRun, so the only part that can be aborted); phase 2 runs the augmenting-path search purely over that already-known, in-memory matrix.
  • Removes the "greedy (not globally optimal)" v1-limitation caveat from docs.notes in all three analyzer-registration sites now that the gap is closed — registry.ts (the source of truth), and analyzer-metadata.json + apps/gittensory-ui/src/lib/rees-analyzers.ts, both regenerated via npm run metadata (not hand-edited, since both are machine-generated from registry.ts).

Closes #4812

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes (5 files: the analyzer, its test file, and its 3 doc-registration sites).
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (Closes #4812).

Validation

Ran the full local gate synchronously, in this order, all green:

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage — 702 files / 13920 tests passed (root suite; review-enrichment/src itself is outside Codecov's src/** patch-coverage scope, so I additionally hand-verified 100% branch coverage of the new assignSurvivors code via Node's --experimental-test-coverage / raw V8 coverage output — the only uncovered branches in the file are two pre-existing, unrelated lines in scanDuplicationDelta that I did not touch)
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • New/changed behavior has tests for new branches and fallback paths — see Notes below.

All of the above ran as part of one full npm run test:ci pass (green end to end), plus these REES-specific commands the change also requires:

  • npm run rees:test — review-enrichment's own full suite (build + sourcemap validation + generate-analyzer-metadata.mjs --check + node --test): 1315/1315 passing after rebasing onto latest main.
  • npx tsx scripts/check-engine-parity.ts — clean, no drift.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized — n/a, no PR/check-output rendering text changed (only an internal analyzer doc string).
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests — n/a, no such surface touched.
  • API/OpenAPI/MCP behavior is updated and tested where needed — n/a, docs.notes is descriptive text, not an API contract.
  • UI changes use live API data or real empty/error/loading states — n/a, no UI behavior change.
  • UI Evidence — n/a, not a visible UI/frontend/docs/extension change (only a machine-generated data file's docs.notes string changed); this repo's screenshot-table gate is also presently unconfigured/disabled regardless of path.
  • Public docs/changelogs are updated where needed — n/a, no changelog edit.

Notes

  • Algorithm: Kuhn's algorithm (augmenting-path maximum bipartite matching), O(V·E), computed in two phases so the abort-signal contract stays exactly what it was — see the updated doc comment on assignSurvivors in review-enrichment/src/analyzers/duplication-delta.ts for the full rationale.
  • Regression test: reproduces the exact scenario Upgrade duplicationDelta's greedy block assignment to optimal bipartite matching #4812 describes (old block A's content matches both remaining new occurrences, old block B's content matches only one) and asserts both old blocks are now correctly reported as surviving. Verified empirically against the actual pre-fix compiled greedy implementation that this exact fixture returns [true, false] there (B incorrectly reported as still-duplicated), not the optimal [true, true].
  • Additional edge cases added: odd old/new candidate counts (3-vs-2 with re-routing, 3-vs-1, 2-vs-3-with-decoys), plus three distinct abort-signal-checkpoint tests (the outer per-old-block loop, the inner per-new-index loop, and a trip inside longestSharedRun itself) — full branch coverage of the new code.
  • Zero behavior change: the full pre-existing duplication-delta.test.ts suite (31 tests) passes unchanged.

…atching

assignSurvivors used a first-come-first-claimed greedy walk to pair old
duplicate blocks with surviving new occurrences. In an asymmetric scenario
where one old block matches multiple new occurrences but a sibling old
block matches only one of them, the greedy walk could let the first grab
the occurrence the second needed, under-reporting a still-present
duplicate pair as resolved.

Replaces it with Kuhn's algorithm (an O(V*E) augmenting-path search) over
the old/new candidate graph, computed in two phases so the existing
abort-signal contract is unchanged: phase 1 builds the full adjacency
matrix (the only part that calls longestSharedRun, so the only part that
can be aborted), phase 2 runs the augmenting-path search purely over that
already-known matrix. An abort during phase 1 discards everything rather
than matching over incomplete candidacy data.

Adds a regression test reproducing the exact asymmetric scenario (#4812),
plus odd-count and phase-1-abort edge cases, and removes the "greedy (not
globally optimal)" v1-limitation caveat from the analyzer's docs.notes in
all three registration sites now that the gap is closed.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 5db4b9c Commit Preview URL

Branch Preview URL
Jul 11 2026, 03:45 AM

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-11 03:51:02 UTC

5 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This replaces the greedy, order-dependent old→new block assignment in duplicationDelta with a correct Kuhn's-algorithm maximum bipartite matching, split into a phase-1 adjacency-build (the only part touching longestSharedRun/abort) and a phase-2 pure in-memory augmenting-path search. The two-phase split correctly preserves the existing abort-signal contract (any abort during phase 1 discards the whole matrix and returns all-false), and the matching logic itself (matchOf/visited/tryAugment) is a standard, correctly-implemented Kuhn's algorithm. Docs are updated consistently across all three registration sites (registry.ts as source of truth, plus the two generated mirrors), and the new tests directly reproduce the #4812 asymmetric-candidacy scenario against the actual old greedy behavior. One real behavioral tradeoff: phase 1 now always computes the full old×new adjacency row for every old block (no early-exit on first match, no per-row 'claimed' skip), so it invokes longestSharedRun strictly more than the old greedy version did on files with many candidates — worth flagging but not disqualifying given the correctness gain and per-file (not repo-wide) scope.

Nits — 6 non-blocking
  • review-enrichment/src/analyzers/duplication-delta.ts: phase 1 now computes the full old×new adjacency matrix unconditionally (no early break once a match is found, no 'claimed' skip), so longestSharedRun is called strictly more often than the old greedy version on files with many duplicate candidates — worth a comment noting this is an intentional accuracy-for-cost tradeoff.
  • review-enrichment/src/analyzers/duplication-delta.ts: tryAugment recurses per re-route in the augmenting-path search; fine at per-file block counts but worth a one-line note on the practical bound (recursion depth ≤ newIndices.length) so it doesn't look unbounded on first read.
  • review-enrichment/test/duplication-delta.test.ts: the three abort-checkpoint tests assert on an exact `aborted` getter read-count (reads === 4/5), which is brittle to any future reordering of checks inside assignSurvivors/longestSharedRun — consider asserting the observable behavior (result + which longestSharedRun calls happened) instead of the literal call count.
  • review-enrichment/src/analyzers/duplication-delta.ts:146 references issue Upgrade duplicationDelta's greedy block assignment to optimal bipartite matching #4812 only in a comment; fine as-is, but if this pattern repeats consider a shared convention for citing tracking issues in code comments vs. just the PR/commit history.
  • Consider short-circuiting a row in phase 1 once MIN_RUN candidacy is confirmed against the current known-matched set is unnecessary for correctness (matching needs the full row anyway), so this is fine to leave as-is — just document the cost tradeoff mentioned above.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #4812
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 312 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 312 issue(s).
Gate result ✅ Passing No configured blocker found.
Linked issue satisfaction

Addressed
The diff replaces the greedy assignSurvivors walk with Kuhn's augmenting-path maximum bipartite matching, adds a regression test reproducing the exact asymmetric A/B scenario asserting both survive, preserves the abort-signal fail-safe via a two-phase design (with dedicated abort tests), and removes the v1-limitation caveat from all three doc-registration sites (registry.ts, rees-analyzers.ts, ana

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, JavaScript, MDX, Shell, Solidity
  • Official Gittensor activity: 48 PR(s), 312 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop before / after /
/ mobile before / (mobile) after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 11, 2026
@JSONbored
JSONbored merged commit bf6fa2b into main Jul 11, 2026
13 checks passed
@JSONbored
JSONbored deleted the fix/4812-duplication-delta-max-matching branch July 11, 2026 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade duplicationDelta's greedy block assignment to optimal bipartite matching

1 participant