From 866d503b4808b07e57ba11a1670a2866fc92ac29 Mon Sep 17 00:00:00 2001 From: safishamsi Date: Wed, 22 Jul 2026 16:11:15 +0100 Subject: [PATCH] fix(dedup): order-independent gap-fill, source_file gate, _origin/None handling (follow-up to #2102) --- graphify/dedup.py | 43 ++++++++++++++++++++++------- tests/test_dedup.py | 66 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 10 deletions(-) diff --git a/graphify/dedup.py b/graphify/dedup.py index f4de6958..0e1341e7 100644 --- a/graphify/dedup.py +++ b/graphify/dedup.py @@ -253,14 +253,28 @@ def _same_source_entity(survivor: dict, duplicate: dict) -> bool: """ keep_file = survivor.get("source_file") or "" lose_file = duplicate.get("source_file") or "" - return keep_file == lose_file + # Require a non-empty source_file: two provenance-less records ("" == "") + # are NOT proof of the same symbol (#1178), and merging their attributes + # would be a cross-pollination bug in the opposite direction (#2091 review). + return bool(keep_file) and keep_file == lose_file def _merge_missing_attributes(survivor: dict, duplicate: dict) -> dict: - """Keep survivor values while retaining non-conflicting duplicate data.""" + """Fill the survivor's absent/None attributes from a same-source duplicate, + without overriding values the survivor already has (#2091).""" merged = dict(survivor) for key, value in duplicate.items(): - merged.setdefault(key, value) + # Never inherit a provenance tag from a dropped record: a false + # _origin="ast" on an LLM survivor is read as an authority signal by the + # ghost-merge (#2068) and watch deletion logic (#2091 review). + if key == "_origin": + continue + if value is None: + continue + # Treat an explicit None on the survivor as absent — the codebase emits + # `source_location: None`, and that is exactly the attribute #2091 loses. + if merged.get(key) is None: + merged[key] = value return merged @@ -353,17 +367,26 @@ def deduplicate_entities( # Smallest-ranked node wins; the min over a total order is independent # of the order nodes arrive in, so the survivor no longer depends on # chunk ordering (#1851). - seen_ids[nid] = ( - _merge_missing_attributes(node, incumbent) - if _same_source_entity(node, incumbent) - else node - ) + seen_ids[nid] = node dropped[nid].append(incumbent) else: - if _same_source_entity(incumbent, node): - seen_ids[nid] = _merge_missing_attributes(incumbent, node) dropped[nid].append(node) + # Gap-fill each survivor from its SAME-SOURCE losers, applied in deterministic + # _collision_rank order (best loser first). Merging here — not incrementally in + # the loop above — keeps the merged attributes independent of chunk arrival + # order with 3+ colliding records, preserving the #1851 order-independence + # contract (#2091 review). + for nid, losers in dropped.items(): + survivor = seen_ids[nid] + same_source = sorted( + (l for l in losers if _same_source_entity(survivor, l)), + key=_collision_rank, + ) + for loser in same_source: + survivor = _merge_missing_attributes(survivor, loser) + seen_ids[nid] = survivor + for nid, losers in dropped.items(): _report_id_collision(nid, seen_ids[nid], losers) diff --git a/tests/test_dedup.py b/tests/test_dedup.py index 7195226a..ab38b0f4 100644 --- a/tests/test_dedup.py +++ b/tests/test_dedup.py @@ -626,3 +626,69 @@ def test_defines_id_helper(): # A path that is merely a string-prefix of the ID's path does not define it. assert not _defines_id({"id": "agents_foo", "source_file": "agent/foo.md"}) assert not _defines_id({"id": "docs_intro_foo", "source_file": ""}) + + +# ── #2091 review: attribute-merge correctness (fixes A-D) ───────────────────── + +def test_dedup_gapfill_is_order_independent_with_multiple_losers(): + """(fix A) With 3+ same-ID same-source records, the merged attributes must not + depend on arrival order — the best loser by collision rank supplies each + missing key deterministically, preserving the #1851 order-independence.""" + import itertools + base = [ + {"id": "f", "label": "f", "file_type": "code", "source_file": "m.py", + "source_location": "L1"}, # shortest label -> survivor + {"id": "f", "label": "f helper beta", "file_type": "code", + "source_file": "m.py", "summary": "BETA"}, + {"id": "f", "label": "f helper alpha", "file_type": "code", + "source_file": "m.py", "summary": "ALPHA"}, + ] + seen = set() + for perm in itertools.permutations(base): + nodes, _ = deduplicate_entities([dict(n) for n in perm], [], communities={}) + assert len(nodes) == 1 + seen.add(nodes[0].get("summary")) + assert len(seen) == 1, f"merged summary depends on arrival order: {seen}" + + +def test_dedup_no_attribute_merge_when_source_file_missing(): + """(fix B) Two provenance-less records sharing an ID must NOT cross-pollinate + attributes — '' == '' is not proof of the same symbol (#1178).""" + nodes = [ + {"id": "c", "label": "c", "file_type": "concept", "summary": "A"}, + {"id": "c", "label": "c", "file_type": "concept", "notes": "B"}, + ] + result, _ = deduplicate_entities([dict(n) for n in nodes], [], communities={}) + assert len(result) == 1 + surv = result[0] + assert not ("summary" in surv and "notes" in surv), ( + "provenance-less same-id records must not merge attributes" + ) + + +def test_dedup_survivor_does_not_inherit_false_origin_ast(): + """(fix C) An LLM survivor must not inherit _origin='ast' from a dropped + same-source AST record — a false authority tag is read by ghost-merge/watch.""" + nodes = [ + {"id": "x", "label": "run", "file_type": "code", "source_file": "m.py", + "source_location": "L9"}, # shorter label -> survivor (LLM) + {"id": "x", "label": "run() [ast]", "file_type": "code", "source_file": "m.py", + "source_location": "L2", "_origin": "ast"}, # loser carries _origin=ast + ] + result, _ = deduplicate_entities([dict(n) for n in nodes], [], communities={}) + assert len(result) == 1 + assert result[0].get("_origin") != "ast", "survivor must not inherit a false _origin=ast" + + +def test_dedup_fills_explicit_none_attribute(): + """(fix D) An explicit source_location=None on the survivor is treated as + absent and filled from a same-source record that has a real line (#2091).""" + nodes = [ + {"id": "y", "label": "y", "file_type": "code", "source_file": "m.py", + "source_location": None}, # survivor, explicit None + {"id": "y", "label": "y helper", "file_type": "code", "source_file": "m.py", + "source_location": "L7"}, + ] + result, _ = deduplicate_entities([dict(n) for n in nodes], [], communities={}) + assert len(result) == 1 + assert result[0].get("source_location") == "L7", "explicit-None must be filled from the loser"