Skip to content

fix(resolve): don't count merges that _merge_entities skipped - #8

Open
cosmic-fire-eng wants to merge 2 commits into
johncarpenter:mainfrom
cosmic-fire-eng:fix/resolve-merge-count
Open

fix(resolve): don't count merges that _merge_entities skipped#8
cosmic-fire-eng wants to merge 2 commits into
johncarpenter:mainfrom
cosmic-fire-eng:fix/resolve-merge-count

Conversation

@cosmic-fire-eng

Copy link
Copy Markdown
Contributor

Hey John! Small follow-up to #1 — the guard that PR added has a counting side effect I missed at the time.

run_resolution does total_merges += len(merge_ids) - 1 on the line right after _merge_entities(...), unconditionally. But _merge_entities returns early when the survivor row is already gone (the stale-survivor case, resolve.py:251), so on that path nothing merges and the count still goes up. The overstated number is what _finalize_run writes into resolution_runs.merges and what comes back as {"merges": ...}, so kgmd resolve reports merges that didn't happen. It's off by len(drop_ids) per skipped cluster.

Repro is the same shape as the crash the guard fixed — three entities of one type where the middle one has two mentions, one matching each neighbour, so union-find yields clusters {1,2} and {2,3}. The first merges 2 into 1; the second names survivor 2, which is gone, and is skipped. One merge really happened; the run reports 2. The new test asserts that end to end (entity count, returned stats, and the persisted resolution_runs row) and fails on main with assert 2 == 1.

Fix is small: _merge_entities returns an int — 0 on the early return, len(drop_ids) otherwise — and the call site accumulates that instead of computing it itself. Behaviour is unchanged on every path that actually merges, since len(merge_ids) - 1 == len(drop_ids).

pytest is 136 passed locally on 3.13, ruff check . clean.

Thanks for merging #1 and shipping it in 0.2.0 — glad it was useful. Happy to adjust either the fix or the test if you'd rather shape it differently.

run_resolution adds `len(merge_ids) - 1` to total_merges right after
calling _merge_entities, whether or not the merge happened. When the
survivor row is already gone — the stale-survivor case the function
guards against by returning early — nothing is merged but the count
still goes up, so the value stored in resolution_runs.merges and
returned as {"merges": ...} overstates by len(drop_ids).

Return the number of entities actually merged away (0 on the early
return) and accumulate that at the call site. Adds a regression test
driving the two-cluster case end to end through run_resolution.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown

Greptile Summary

The PR moves resolution merge accounting into _merge_entities so stale-survivor clusters return zero instead of inflating reported and persisted statistics.

  • Changes _merge_entities to return an integer merge count.
  • Adds an end-to-end regression test for overlapping clusters whose later survivor has already been removed.
  • The accounting still counts already-removed drop entities when the survivor remains present.

Confidence Score: 4/5

The merge-count fix should be completed before merging because a reachable overlapping-cluster shape still inflates returned and persisted resolution statistics.

_merge_entities silently performs no work for already-deleted drop IDs but returns the complete requested drop count, leaving the accounting defect reachable.

Files Needing Attention: kgmd/resolve.py

Fix all with Greploop Fix All in Claude Code

Reviews (1): Last reviewed commit: "fix(resolve): don't count merges that _m..." | Re-trigger Greptile

Comment thread kgmd/resolve.py Outdated
)

conn.commit()
return len(drop_ids)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 When overlapping mention clusters revisit a deleted drop under a still-live survivor, the merge operations are no-ops but _merge_entities returns the full len(drop_ids), causing the returned and persisted merge counts to remain inflated.

Knowledge Base Used: Entity resolution

Fix in Claude Code

Mirror of the stale-survivor case, caught by the review bot on this PR.
When the survivor row is still present the merge runs, but a drop_id an
earlier cluster already deleted has nothing left to re-point or delete —
those statements are no-ops — while the return value still counted it.

Count only the drop_ids that still had a row. The attribute-merge loop
already does that SELECT, so this is the same query, not a new one. Adds
a regression test for the overlapping-cluster shape that reaches it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cosmic-fire-eng

Copy link
Copy Markdown
Contributor Author

Second commit pushed — the review bot caught a real gap in the first one, and it was right.

Same class of problem on the other side of the merge: when the survivor row is still there the merge runs, but a drop_id an earlier cluster already deleted has nothing left to re-point or delete, so those statements are no-ops and the count shouldn't include it either. Reachable with the same overlapping-cluster shape, just with the shared entity holding the highest id — clusters {2,3} then {1,3}, where the second names a live survivor and a dead drop. Reported 2 merges for 1 real one before this commit.

The attribute-merge loop already selects each drop row, so counting the ones that exist is the same query rather than a new one. Second regression test added for that shape; both new tests fail on main, 137 pass on the branch, ruff clean.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant