From a3ba93b859ee3768fa45ecb4edeabb5203e40eb5 Mon Sep 17 00:00:00 2001 From: abhay-codes07 Date: Tue, 8 Sep 2026 23:48:09 +0530 Subject: [PATCH] fix(dedup): pick the richer duplicate as survivor and keep the losers' fields (#3372) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JJbLfztSxm2tH5cJwBbe9q --- graphify/dedup.py | 62 +++++++++++++++-- tests/test_dedup_survivor_richness.py | 98 +++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 4 deletions(-) create mode 100644 tests/test_dedup_survivor_richness.py diff --git a/graphify/dedup.py b/graphify/dedup.py index f6d193597..a5ed6dbfc 100644 --- a/graphify/dedup.py +++ b/graphify/dedup.py @@ -850,6 +850,8 @@ def deduplicate_entities( n["id"]: (i, n) for i, n in enumerate(unique_nodes) } + # Survivors enriched with their losers' fields, substituted at the end. + enriched_by_id: dict[str, dict] = {} for root, members in components.items(): if len(members) == 1: continue @@ -861,6 +863,17 @@ def deduplicate_entities( ] winner = _pick_winner(group_nodes) if group_nodes else {"id": root} winner_id = winner["id"] + # Even with the right survivor, dropping the losers wholesale loses + # whatever fields only they carried. Fill the survivor's absent + # fields from each loser — the same never-override merge the + # same-source collision path already applies (#2091/#3372); loser + # order follows unique_nodes order, so the fill is deterministic. + merged = winner + for node in group_nodes: + if node["id"] != winner_id: + merged = _merge_missing_attributes(merged, node) + if merged != winner: + enriched_by_id[winner_id] = merged for member in members: if member != winner_id: remap[member] = winner_id @@ -892,7 +905,9 @@ def deduplicate_entities( if hyperedges: _remap_hyperedge_members(hyperedges, remap) - deduped_nodes = [n for n in unique_nodes if n["id"] not in remap] + deduped_nodes = [ + enriched_by_id.get(n["id"], n) for n in unique_nodes if n["id"] not in remap + ] deduped_edges = [] for edge in edges: e = dict(edge) @@ -916,14 +931,53 @@ def deduplicate_entities( return deduped_nodes, deduped_edges +# Keys every node carries (or that describe placement rather than content). +# They say nothing about which duplicate is the better-established record, so +# the richness score below ignores them. +_RICHNESS_IGNORED_KEYS = frozenset({ + "id", "label", "norm_label", "file_type", "source_file", "source_location", +}) + + +def _content_richness(n: dict) -> int: + """How much actual content a node carries, for survivor selection (#3372). + + Counts populated fields beyond the identity/placement baseline, weighting + ``attributes`` by entry count and ``_merged_from`` by its history length — + a node that already absorbed prior merges is the established record. + """ + score = 0 + for key, value in n.items(): + if key in _RICHNESS_IGNORED_KEYS: + continue + if value is None or value == "" or value == [] or value == {}: + continue + score += 1 + if key == "attributes" and isinstance(value, dict): + score += len(value) + elif key == "_merged_from" and isinstance(value, list): + score += len(value) + return score + + def _pick_winner(nodes: list[dict]) -> dict: - """Pick the canonical survivor: prefer no chunk suffix, then shorter ID.""" + """Pick the canonical survivor: no chunk suffix, then richer content, + then shorter ID. + + ID length used to be the primary signal after the chunk-suffix check, + which made a passing one-line mention on a shallow page (short id, one + fewer path segment) beat the dedicated, enriched page for the same entity + — every time the pattern occurred, the established node's content was + discarded (#3372). Content richness now decides first; ID shape only + breaks ties between equally-rich candidates, preserving the old + deterministic ordering there. + """ if not nodes: raise ValueError("Cannot pick winner from empty list") - def _score(n: dict) -> tuple[int, int]: + def _score(n: dict) -> tuple[int, int, int]: has_suffix = bool(_CHUNK_SUFFIX.search(n["id"])) - return (1 if has_suffix else 0, len(n["id"])) + return (1 if has_suffix else 0, -_content_richness(n), len(n["id"])) return min(nodes, key=_score) diff --git a/tests/test_dedup_survivor_richness.py b/tests/test_dedup_survivor_richness.py new file mode 100644 index 000000000..a8a54ce18 --- /dev/null +++ b/tests/test_dedup_survivor_richness.py @@ -0,0 +1,98 @@ +"""Dedup survivor selection prefers content over ID shape (#3372). + +`_pick_winner` scored purely on chunk-suffix + ID length, so a passing +one-line mention on a shallow page (short id) beat the dedicated, enriched +page for the same entity, and the losers were dropped wholesale — the +established node's attributes, description, confidence and merge history +were discarded every time the pattern occurred. Richness now decides first +(ID shape breaks ties), and the survivor is back-filled with any fields only +a loser carried. +""" + +from graphify.dedup import _pick_winner, deduplicate_entities + + +def _rich(**over): + node = { + "id": "topics_networking_widget_x_widget_x", + "label": "Widget X", + "file_type": "concept", + "source_file": "topics/networking/widget-x.md", + "source_location": "L1", + "description": "The flagship telemetry relay used by every edge deployment.", + "attributes": {"protocol": "mqtt", "version": "3.2", "owner": "platform"}, + "confidence": "EXTRACTED", + "_merged_from": ["widget_x_old"], + } + node.update(over) + return node + + +def _shallow(**over): + node = { + "id": "sources_notes_widget_x", + "label": "Widget X", + "file_type": "concept", + "source_file": "sources/notes.md", + "source_location": "L14", + } + node.update(over) + return node + + +def test_richer_node_survives_despite_longer_id(): + """The issue's exact shape: dedicated nested page vs shallow mention.""" + dn, _ = deduplicate_entities([_rich(), _shallow()], [], communities={}) + labels = [n for n in dn if n["label"] == "Widget X"] + assert len(labels) == 1 + surv = labels[0] + assert surv["id"] == "topics_networking_widget_x_widget_x" + assert surv["attributes"] == {"protocol": "mqtt", "version": "3.2", + "owner": "platform"} + assert surv["_merged_from"] == ["widget_x_old"] + + +def test_loser_only_fields_are_folded_into_the_survivor(): + """Even the right winner must not lose what only a loser carried.""" + rich = _rich() + shallow = _shallow(summary="Mentioned during the Q3 review.") + dn, _ = deduplicate_entities([rich, shallow], [], communities={}) + surv = next(n for n in dn if n["label"] == "Widget X") + assert surv["id"] == rich["id"] + assert surv.get("summary") == "Mentioned during the Q3 review." + # Never-override: the survivor's own fields stay its own. + assert surv["description"].startswith("The flagship") + + +def test_equally_bare_nodes_keep_the_shorter_id_tiebreak(): + """Old deterministic ordering survives for content-equal candidates.""" + a = _shallow(id="a_widget_x", source_file="a/notes.md") + b = _shallow(id="longer_b_widget_x", source_file="b/notes.md") + assert _pick_winner([a, b])["id"] == "a_widget_x" + assert _pick_winner([b, a])["id"] == "a_widget_x" + + +def test_chunk_suffix_still_dominates_richness(): + """A chunk-suffixed id never beats a clean one, however rich.""" + chunked = _rich(id="topics_widget_x_c3") + clean = _shallow() + assert _pick_winner([chunked, clean])["id"] == "sources_notes_widget_x" + + +def test_richness_counts_content_not_placement(): + from graphify.dedup import _content_richness + + assert _content_richness(_shallow()) == 0 + assert _content_richness(_rich()) > _content_richness( + _rich(attributes={"protocol": "mqtt"})) + + +def test_edges_rewire_to_the_rich_survivor(): + edges = [{"source": "sources_notes_widget_x", "target": "other", + "relation": "references", "source_file": "sources/notes.md"}] + dn, de = deduplicate_entities( + [_rich(), _shallow(), {"id": "other", "label": "Other page", + "file_type": "concept", + "source_file": "docs/other.md"}], + edges, communities={}) + assert de[0]["source"] == "topics_networking_widget_x_widget_x"