From ad55266fba31c28d3b4d5f79c6b60b7650b717c1 Mon Sep 17 00:00:00 2001 From: bchan84 Date: Mon, 13 Jul 2026 21:11:36 +0800 Subject: [PATCH] fix(dedup): let the defining file win an ID collision, and warn only about real loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Node IDs are _, so a doc that merely references an entity mints the entity's own ID and collides with the entity's node by construction. The pre-dedup pass kept whichever arrived first, so chunk order decided whether an entity kept its own attributes or a passing cross-reference's, and the warning that fired (#1504) told the user a same-name-different-directory clash had lost their data — while the one drop that really is lossy, two labels for one ID from the same file, was silent because the warning was gated on source_file differing. The survivor is now the node whose source_file is the file its ID encodes (any trailing slice of the path, so absolute, repo-relative, and pre-#1504 bare-stem IDs all resolve), falling back to first-seen. Reporting follows what the drop actually costs: a cross-reference folding into the node it references loses nothing (edges are keyed by ID and rewire to the survivor) and is silent; a same-file relabel notes the discarded label; two files that both encode the ID are distinct entities and still WARNING, now stating the ID-scheme cause rather than a filename clash. Refs #1851 --- graphify/dedup.py | 102 +++++++++++++++++++++++++++++++++++++------- tests/test_dedup.py | 84 +++++++++++++++++++++++++++++++++++- 2 files changed, 169 insertions(+), 17 deletions(-) diff --git a/graphify/dedup.py b/graphify/dedup.py index e0ddd2e3b..2851cb06a 100644 --- a/graphify/dedup.py +++ b/graphify/dedup.py @@ -187,6 +187,76 @@ def _is_code(node: dict) -> bool: return node.get("file_type") == "code" +# ── ID collisions ───────────────────────────────────────────────────────────── + +_ID_SEGMENT = re.compile(r"[^a-z0-9]+") +_EXTENSION = re.compile(r"\.[^./]+$") + + +def _id_prefixes(source_file: str) -> set[str]: + """The ID prefixes a node extracted from ``source_file`` may legitimately mint. + + An ID is ``_``, where the path is the extension-stripped source + path, each segment slugified and joined with ``_``. Every trailing slice of the + path counts as a prefix: the stored path may be absolute or repo-relative, and + graphs built under the pre-#1504 scheme keyed off the bare filename stem. + """ + stem = _EXTENSION.sub("", source_file.replace("\\", "/")) + segments = [s for s in (_ID_SEGMENT.sub("_", p.casefold()).strip("_") + for p in stem.split("/")) if s] + return {"_".join(segments[i:]) for i in range(len(segments))} + + +def _defines_id(node: dict) -> bool: + """True when the node's own source_file is the file its ID encodes. + + A doc that *references* an entity mints the ID of the entity's own file, not one + derived from the doc's path — so the referencing node collides with the defining + node by construction. This separates the two: the definer owns the ID. + """ + nid = node.get("id") or "" + source_file = node.get("source_file") or "" + if not nid or not source_file: + return False + return any(nid.startswith(f"{prefix}_") for prefix in _id_prefixes(source_file)) + + +def _report_id_collision(nid: str, survivor: dict, losers: list[dict]) -> None: + """Report an ID collision in proportion to what dropping the loser actually costs. + + Cross-reference to a defining node: same entity, edges are keyed by ID and rewire + to the survivor — nothing is lost, so say nothing. Same file, different labels: the + extractor emitted two labels for one entity and one is discarded — note it. Two + files that both encode this ID: they are distinct entities and one is genuinely + lost — warn, and point at the extraction split that keeps them apart (#1504). + """ + keep_file = survivor.get("source_file") or "" + keep_label = survivor.get("label") or "" + for loser in losers: + lose_file = loser.get("source_file") or "" + lose_label = loser.get("label") or "" + if lose_file == keep_file: + if _norm(lose_label) != _norm(keep_label): + print( + f"[graphify] note: node '{nid}' was extracted twice from " + f"'{keep_file}' under different labels — keeping '{keep_label}', " + f"dropping '{lose_label}'.", + file=sys.stderr, + ) + elif _defines_id(survivor) and not _defines_id(loser): + continue # the loser only references the entity the survivor defines + else: + print( + f"[graphify] WARNING: node '{nid}' is minted by two different files — " + f"keeping '{keep_label}' from '{keep_file}', dropping '{lose_label}' " + f"from '{lose_file}'. An ID is derived from the source path plus the " + f"entity name, so this one does not identify a single entity and the " + f"dropped node is lost. To keep them distinct, run 'graphify extract' " + f"per subfolder and merge with 'graphify merge-graphs'.", + file=sys.stderr, + ) + + # ── main entry point ────────────────────────────────────────────────────────── def deduplicate_entities( @@ -220,29 +290,29 @@ def deduplicate_entities( if len(nodes) <= 1: return nodes, edges - # Pre-deduplicate: keep first occurrence of each id. - # Warn when two nodes share an ID but originate from different source files — - # this indicates a cross-chunk ID collision (#1504) where silent data loss occurs. + # Pre-deduplicate: one node per ID. The survivor is the node that *defines* the + # ID (its source_file is the file the ID encodes), not merely the first seen — + # otherwise chunk order decides whether an entity keeps its own attributes or a + # passing cross-reference's. Warnings are then emitted for what is actually lost + # (#1504); a same-entity merge costs nothing and stays quiet. seen_ids: dict[str, dict] = {} + dropped: dict[str, list[dict]] = defaultdict(list) for node in nodes: nid = node.get("id", "") if not nid: continue - if nid not in seen_ids: + incumbent = seen_ids.get(nid) + if incumbent is None: seen_ids[nid] = node + elif _defines_id(node) and not _defines_id(incumbent): + seen_ids[nid] = node + dropped[nid].append(incumbent) else: - existing_sf = seen_ids[nid].get("source_file") or "" - new_sf = node.get("source_file") or "" - if existing_sf != new_sf: - print( - f"[graphify] WARNING: node '{nid}' from '{new_sf}' collides with " - f"node from '{existing_sf}' — the second node will be dropped. " - f"This is a cross-chunk ID collision caused by two files with the " - f"same name in different directories. To avoid data loss, run " - f"'graphify extract' per subfolder and merge with " - f"'graphify merge-graphs'.", - file=sys.stderr, - ) + dropped[nid].append(node) + + for nid, losers in dropped.items(): + _report_id_collision(nid, seen_ids[nid], losers) + unique_nodes = list(seen_ids.values()) if len(unique_nodes) <= 1: diff --git a/tests/test_dedup.py b/tests/test_dedup.py index 267388e57..1cfe84239 100644 --- a/tests/test_dedup.py +++ b/tests/test_dedup.py @@ -1,7 +1,7 @@ """Tests for graphify/dedup.py entity deduplication pipeline.""" from __future__ import annotations import pytest -from graphify.dedup import deduplicate_entities, _entropy, _shingles +from graphify.dedup import deduplicate_entities, _defines_id, _entropy, _shingles # ── entropy gate ───────────────────────────────────────────────────────────── @@ -404,3 +404,85 @@ def test_same_id_same_source_file_no_warning(capsys): assert len(result_nodes) == 1 captured = capsys.readouterr() assert "WARNING" not in captured.err + + +# ── ID collisions: definition vs cross-reference ────────────────────────────── + +# The defining node and a doc that merely mentions the entity. Both mint the ID +# encoded from the *defining* file's path, so they collide by construction. +_DEFINING = {"id": "agents_make_batch_fixtures_make_batch_fixtures", + "label": "make-batch-fixtures agent", "file_type": "concept", + "source_file": "agents/make-batch-fixtures.md"} +_REFERENCING = {"id": "agents_make_batch_fixtures_make_batch_fixtures", + "label": "make-batch-fixtures", "file_type": "concept", + "source_file": "available/diagnose-issue/SKILL.md"} + + +@pytest.mark.parametrize("nodes", [ + [_DEFINING, _REFERENCING], + [_REFERENCING, _DEFINING], +], ids=["definition-first", "reference-first"]) +def test_defining_file_wins_over_referencing_file(nodes, capsys): + """The node whose source_file is the file its ID encodes survives, whichever + chunk order the nodes arrive in — the survivor must not depend on it.""" + result_nodes, _ = deduplicate_entities(list(nodes), [], communities={}) + + assert len(result_nodes) == 1 + assert result_nodes[0]["source_file"] == "agents/make-batch-fixtures.md" + assert result_nodes[0]["label"] == "make-batch-fixtures agent" + + +def test_reference_collision_is_silent(capsys): + """A cross-reference collapsing into the entity it references loses nothing — + edges are keyed by ID and rewire to the survivor — so it must not be reported.""" + edges = _make_edges("agents_make_batch_fixtures_make_batch_fixtures", "other") + result_nodes, result_edges = deduplicate_entities( + [_DEFINING, _REFERENCING], edges, communities={}) + + assert len(result_nodes) == 1 + assert len(result_edges) == 1 + captured = capsys.readouterr() + assert "WARNING" not in captured.err + assert "note:" not in captured.err + + +def test_absolute_source_path_still_defines_id(capsys): + """source_file is absolute in some pipelines and repo-relative in others; the + defining file is recognised either way.""" + absolute = dict(_DEFINING, source_file="/home/u/proj/agents/make-batch-fixtures.md") + result_nodes, _ = deduplicate_entities([_REFERENCING, absolute], [], communities={}) + + assert len(result_nodes) == 1 + assert result_nodes[0]["label"] == "make-batch-fixtures agent" + assert "WARNING" not in capsys.readouterr().err + + +def test_same_file_relabel_is_noted(capsys): + """Two labels for one ID from one file: the loser's label is discarded, which is + the one drop that used to be silent. It is a note, not a collision warning.""" + nodes = [ + {"id": "agents_make_batch_fixtures_make_batch_fixtures", + "label": "make-batch-fixtures agent", "file_type": "concept", + "source_file": "agents/make-batch-fixtures.md"}, + {"id": "agents_make_batch_fixtures_make_batch_fixtures", + "label": "make-batch-fixtures helper agent", "file_type": "concept", + "source_file": "agents/make-batch-fixtures.md"}, + ] + result_nodes, _ = deduplicate_entities(nodes, [], communities={}) + + assert len(result_nodes) == 1 + captured = capsys.readouterr() + assert "note:" in captured.err + assert "make-batch-fixtures helper agent" in captured.err + assert "WARNING" not in captured.err + + +def test_defines_id_helper(): + assert _defines_id(_DEFINING) + assert not _defines_id(_REFERENCING) + # Pre-#1504 IDs keyed off the bare filename stem. + assert _defines_id({"id": "readme_booking_service", + "source_file": "module-a/README.md"}) + # 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": ""})