diff --git a/graphify/build.py b/graphify/build.py index f738fdfe..f72067b1 100644 --- a/graphify/build.py +++ b/graphify/build.py @@ -1306,7 +1306,7 @@ def build( _fold_node_aliases(n) combined["nodes"], combined["edges"] = deduplicate_entities( combined["nodes"], combined["edges"], communities={}, - dedup_llm_backend=dedup_llm_backend, + dedup_llm_backend=dedup_llm_backend, root=root, ) return build_from_json(combined, directed=directed, root=root) diff --git a/graphify/dedup.py b/graphify/dedup.py index e9ce0398..3683e435 100644 --- a/graphify/dedup.py +++ b/graphify/dedup.py @@ -9,6 +9,7 @@ import re import sys import unicodedata from collections import defaultdict +from pathlib import Path from graphify._minhash import MinHash, MinHashLSH from rapidfuzz.distance import Jaro, JaroWinkler @@ -225,22 +226,89 @@ def _defines_id(node: dict) -> bool: for prefix in _id_prefixes(source_file)) -def _collision_rank(node: dict) -> tuple: +# Path-segment lifecycle markers used by _collision_rank (#2532). Lower penalty +# wins. Without them, pure lexical source_file order makes ``plans/_done/…`` +# beat ``plans/in-progress/…`` because "_" < "i" in ASCII. Active-vs-archived +# marker idea by @michaelxer (#2540); matched against ROOT-RELATIVE directory +# segments only, so a checkout directory that happens to be named ``wip`` or +# ``done`` never leaks into the ranking. +_ACTIVE_PATH_SEGMENTS = frozenset({ + "in-progress", + "in_progress", + "active", + "current", + "wip", +}) +_ARCHIVED_PATH_SEGMENTS = frozenset({ + "_done", + "done", + "archive", + "archived", + "backup", + "bak", + "old", + "attic", + "graveyard", + "completed", +}) + + +def _lifecycle_penalty(rank_path: str) -> int: + """0 for active/in-progress paths, 2 for archived/done paths, 1 otherwise. + + Judged on the DIRECTORY segments of the root-relative rank path — a file + literally named ``done.md`` is not a marker. Among mixed markers the best + (lowest) score wins so an active segment is not drowned out by an unrelated + archive directory higher in the tree (#2532). + """ + segments = [s for s in rank_path.casefold().split("/") if s] + marked = [ + 0 if s in _ACTIVE_PATH_SEGMENTS else 2 + for s in segments[:-1] # directories only, never the basename + if s in _ACTIVE_PATH_SEGMENTS or s in _ARCHIVED_PATH_SEGMENTS + ] + return min(marked) if marked else 1 + + +def _rank_path(source_file: str, root: Path | None) -> str: + """The root-relative form of ``source_file`` used for collision ranking. + + Mirrors ``_source_key`` in extractors/resolution.py: with a scan root, an + absolute stored path and its repo-relative twin rank identically, and the + checkout location's own segments never participate (#2532). Without a root + (or when relativizing fails) the normalized stored path is used as-is. + """ + normalized = source_file.replace("\\", "/") + if root is not None and normalized: + try: + return Path(normalized).resolve().relative_to(root).as_posix() + except Exception: + pass + return normalized + + +def _collision_rank(node: dict, root: Path | None = None) -> tuple: """A total order for choosing the survivor of an ID collision, independent of the order the colliding nodes arrive in. The winner is the node with the SMALLEST rank. A node whose ``source_file`` defines the ID always outranks a mere reference; among equally-(non-)defining - nodes it prefers the shorter, more canonical label over a longer qualified - variant, then breaks any remaining tie lexically on label and then source_file - (so the lexically-first path wins) — fully deterministic regardless of order. + nodes an active/in-progress path outranks an archived/done one (#2532); then + it prefers the shorter, more canonical label over a longer qualified variant, + then breaks any remaining tie lexically on label and finally on the REVERSED + segments of the root-relative path. Basename-first comparison decides two + in-repo colliders by segments present in both path forms, so absolute and + repo-relative spellings of the same layout order identically — fully + deterministic regardless of arrival order (#1851) or checkout location. """ label = node.get("label") or "" + rank_path = _rank_path(node.get("source_file") or "", root) return ( not _defines_id(node), # definers (False) sort before references (True) + _lifecycle_penalty(rank_path), # active paths beat archived ones (#2532) len(label), # shorter, more canonical label first label, # lexical tiebreak - node.get("source_file") or "", # lexically-first source path wins + tuple(reversed([s for s in rank_path.split("/") if s and s != "."])), ) @@ -323,6 +391,7 @@ def deduplicate_entities( *, communities: dict[str, int], dedup_llm_backend: str | None = None, + root: str | Path | None = None, ) -> tuple[list[dict], list[dict]]: """Deduplicate near-identical entities in a knowledge graph. @@ -331,6 +400,8 @@ def deduplicate_entities( edges: list of edge dicts with {"source": str, "target": str, ...} communities: mapping of node_id -> community_id (from cluster()) dedup_llm_backend: if set, use LLM to resolve ambiguous pairs + root: scan root; ID-collision ranking judges source paths relative to + it so path form and checkout location cannot flip the survivor (#2532) Returns: (deduped_nodes, deduped_edges) with edges rewired to survivors @@ -348,6 +419,15 @@ def deduplicate_entities( if len(nodes) <= 1: return nodes, edges + # Resolve the scan root once: _collision_rank ranks each node's source_file + # relative to it, so an absolute stored path and its repo-relative twin rank + # identically and lifecycle markers in the checkout location's own segments + # cannot flip the survivor (#2532). + try: + root_resolved: Path | None = Path(root).resolve() if root else None + except Exception: + root_resolved = None + # 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 @@ -363,7 +443,7 @@ def deduplicate_entities( incumbent = seen_ids.get(nid) if incumbent is None: seen_ids[nid] = node - elif _collision_rank(node) < _collision_rank(incumbent): + elif _collision_rank(node, root_resolved) < _collision_rank(incumbent, root_resolved): # 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). @@ -381,7 +461,7 @@ def deduplicate_entities( survivor = seen_ids[nid] same_source = sorted( (l for l in losers if _same_source_entity(survivor, l)), - key=_collision_rank, + key=lambda l: _collision_rank(l, root_resolved), ) for loser in same_source: survivor = _merge_missing_attributes(survivor, loser) diff --git a/tests/test_dedup.py b/tests/test_dedup.py index e1370fc7..e66cdc93 100644 --- a/tests/test_dedup.py +++ b/tests/test_dedup.py @@ -1,7 +1,14 @@ """Tests for graphify/dedup.py entity deduplication pipeline.""" from __future__ import annotations import pytest -from graphify.dedup import deduplicate_entities, _defines_id, _entropy, _shingles +from graphify.dedup import ( + deduplicate_entities, + _collision_rank, + _defines_id, + _entropy, + _lifecycle_penalty, + _shingles, +) # ── entropy gate ───────────────────────────────────────────────────────────── @@ -628,6 +635,108 @@ def test_defines_id_helper(): assert not _defines_id({"id": "docs_intro_foo", "source_file": ""}) +# ── #2532: lifecycle-aware, environment-stable collision ranking ────────────── +# Active-vs-archived path markers adapted from @michaelxer's PR #2540, judged on +# root-relative segments with a reversed-segment final tiebreak so the survivor +# cannot flip with path form or checkout location. + +_DONE_PLAN = {"id": "plans_binding_doctrine", "label": "binding doctrine", + "file_type": "concept", + "source_file": "plans/_done/binding-doctrine.md"} +_ACTIVE_PLAN = {"id": "plans_binding_doctrine", "label": "binding doctrine", + "file_type": "concept", + "source_file": "plans/in-progress/binding-doctrine.md"} + + +def test_lifecycle_penalty_on_relative_segments(): + assert _lifecycle_penalty("plans/_done/x.md") == 2 + assert _lifecycle_penalty("plans/in-progress/x.md") == 0 + assert _lifecycle_penalty("docs/x.md") == 1 + # A FILE named after a marker is not a marker — only directory segments count. + assert _lifecycle_penalty("done.md") == 1 + assert _lifecycle_penalty("plans/done.md") == 1 + # Mixed markers: the best (most active) marked segment wins. + assert _lifecycle_penalty("archive/in-progress/x.md") == 0 + + +def test_archived_path_ranks_below_active_despite_ascii_order(): + """#2532: `plans/_done` sorts before `plans/in-progress` lexically ('_' < 'i'), + so a raw-string tiebreak picks the archived copy — assert the ASCII trap is + real, then that the rank inverts it.""" + assert _DONE_PLAN["source_file"] < _ACTIVE_PLAN["source_file"] # the trap + assert _collision_rank(_ACTIVE_PLAN) < _collision_rank(_DONE_PLAN) + + +def test_active_plan_survives_archived_copy_order_independent(): + """#2532 reported case: the in-progress copy survives, whichever order the + colliding nodes arrive in.""" + import itertools + for perm in itertools.permutations([_DONE_PLAN, _ACTIVE_PLAN]): + out, _ = deduplicate_entities([dict(n) for n in perm], [], communities={}) + assert len(out) == 1 + assert out[0]["source_file"] == "plans/in-progress/binding-doctrine.md" + + +def test_checkout_dir_lifecycle_name_does_not_mark_paths(tmp_path): + """Gap in #2540: markers must be matched on ROOT-RELATIVE segments only. An + absolute source_file stored under a checkout directory named `wip` must not + be scored active by the checkout location — the archived copy still loses.""" + import itertools + root = tmp_path / "wip" / "repo" + archived = {"id": "plans_doc", "label": "doc", "file_type": "concept", + "source_file": str(root / "plans" / "_done" / "doc.md")} + active = {"id": "plans_doc", "label": "doc", "file_type": "concept", + "source_file": "plans/roadmap/doc.md"} + for perm in itertools.permutations([archived, active]): + out, _ = deduplicate_entities([dict(n) for n in perm], [], + communities={}, root=root) + assert len(out) == 1 + assert out[0]["source_file"] == "plans/roadmap/doc.md" + + +def test_neutral_collision_survivor_is_stable_across_path_forms(tmp_path): + """#2532: with no lifecycle markers, the tiebreak must not depend on whether + either path is stored absolute or repo-relative — one root-relative file + identity survives across all four form combinations and both insertion + orders (8 runs).""" + import itertools + root = tmp_path / "repo" + rel_a, rel_b = "plans/q3/doc.md", "plans/roadmap/doc.md" + survivors = set() + for form_a in (rel_a, str(root / rel_a)): + for form_b in (rel_b, str(root / rel_b)): + a = {"id": "plans_doc", "label": "doc", "file_type": "concept", + "source_file": form_a} + b = {"id": "plans_doc", "label": "doc", "file_type": "concept", + "source_file": form_b} + for perm in itertools.permutations([a, b]): + out, _ = deduplicate_entities([dict(n) for n in perm], [], + communities={}, root=root) + assert len(out) == 1 + sf = out[0]["source_file"].replace("\\", "/") + prefix = str(root).replace("\\", "/") + "/" + survivors.add(sf.removeprefix(prefix)) + assert survivors == {"plans/q3/doc.md"}, ( + f"survivor depends on path form or order: {survivors}" + ) + + +def test_archived_definer_beats_active_reference(): + """The definer flag outranks the lifecycle penalty: a node that owns its ID + survives even from an archived folder against a live cross-reference.""" + import itertools + nid = "plans_done_spec_spec" + definer = {"id": nid, "label": "spec doc", "file_type": "concept", + "source_file": "plans/_done/spec.md"} + reference = {"id": nid, "label": "spec", "file_type": "concept", + "source_file": "plans/in-progress/roadmap.md"} + assert _defines_id(definer) and not _defines_id(reference) + for perm in itertools.permutations([definer, reference]): + out, _ = deduplicate_entities([dict(n) for n in perm], [], communities={}) + assert len(out) == 1 + assert out[0]["source_file"] == "plans/_done/spec.md" + + # ── #2091 review: attribute-merge correctness (fixes A-D) ───────────────────── def test_dedup_gapfill_is_order_independent_with_multiple_losers():