From 8fbc8e0a14e570442cca9d1e40c5488a66f719fb Mon Sep 17 00:00:00 2001 From: Eduardo Garcia-Prieto Date: Thu, 3 Sep 2026 15:57:52 +1000 Subject: [PATCH 1/3] fix: enforce the 3-member hyperedge invariant at every persistence boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #3297. A hyperedge models a group relationship, so it needs at least three distinct, resolvable members; a pair belongs in the ordinary edge set. Valid input could degrade to a pair once unresolved members were pruned, and then persist in the semantic cache and in graph.json. Fixing it properly meant settling the SHAPE problem underneath first. Member alias keys (members/node_ids), object members {"id": "a"}, duplicate ids, numeric ids equivalent to their string form, and unusable ids (None, "", booleans) all have to be resolved before a count can mean anything. canonical_hyperedge is the shared gate: alias fold, member coercion, dedupe, malformed-shape rejection, cardinality. Membership sits above it in gate_hyperedges and gate_hyperedges_against_graph, because it needs the id-space bridge, and one invariant must not have two membership rules. The audit in the issue found twelve independent writers. Review found a thirteenth (the git merge-driver) and a fourteenth site applies the gate on the READ side, so a pre-gate cache entry cannot replay forever. The id-space rule, which accounts for most of this diff. Member refs are coerced so they can be compared at all, which means every id set they are compared against must be coerced too, and whatever survives must be handed back in the id space the caller is about to persist. Twelve places compared member refs against node ids and each resolved slightly differently; some dropped valid groups, some kept invalid ones. There is now one definition: _member_keys yields the two lookup keys in priority order, _id_map bridges the spaces, resolve_member_ref returns the raw id for callers writing members back out, member_in_id_space answers yes/no for callers that only decide. Duplicate-attribution decisions are made in the exact space before aliases are added, and a member's own raw form wins over the map's choice, so a member is never rebound from the node it named to one that merely collides with it. Pre-existing defects this uncovered, each demonstrated against v8 (33362d9): - semantic_cleanup filtered members with an exact test, so a ref the graph builder heals was removed and the group persisted as a PAIR — issue #3297 itself, reached by a different trigger. - prefix_graph_for_global left such a ref unprefixed while every node gained the repo:: prefix, writing a dangling group after a cross-repo merge. - watch's reconciliation evicted a group whose members resolve. - both cache dangling prunes failed to prune a group naming a node deliberately never written. - watch's raw --no-cluster writer, to_json, the raw CLI block and the exclusion-only prune had no gate at all. Defects introduced by this change and found in review, listed because they are the argument for the structural note below rather than something to hide: the gate returning coerced members into a raw id space (four writers); the cache's skipped-node prune breaking once members were coerced; dedup's consolidated exit coercing members where the old early return had skipped them; the id map being rebuilt per candidate, O(nodes x hyperedges) on a merged corpus; a second weaker membership rule left reachable only from tests; a non-dict graph value raising AttributeError out of the prune; and two collision cases where distinct ids share a coerced or normalized key. Structurally: thirteen write boundaries plus one read boundary share a gate, which is not the same as a single persistence funnel. Two of the fourteen were found by reviewers rather than by the audit. A funnel is the durable fix and is larger than this change. Known shortcomings are documented in the pull request rather than papered over: a dangling-member group still leaves a doc manifest-stamped and the #2927 heal cannot clear it; membership is deliberately not validated before stamping or caching, with measurements showing why; and a no-change --no-cluster run exits before any gate, so a pre-upgrade pair already on disk is not migrated. Verification: 5,319 passed, 56 skipped. Ruff clean over graphify/ and tests/. CodeRabbit CLI reports no findings over the full diff. Docstring coverage 100% across the 190 functions touched. Six local test_ollama failures are environmental (an exported OPENAI_API_KEY with the openai package absent) and reproduce identically on an untouched checkout; CI does not set that variable and passes on 3.10 and 3.12. Reviewed across many rounds by CodeRabbit, Codex and GitHub Copilot on egarcia74/graphify#3 and #6. Co-Authored-By: Claude Opus 5 --- graphify/build.py | 412 ++++++++++++++- graphify/cache.py | 143 ++++- graphify/cli.py | 156 +++++- graphify/dedup.py | 179 ++++++- graphify/export.py | 71 ++- graphify/semantic_cleanup.py | 36 +- graphify/watch.py | 87 ++- tests/test_build.py | 100 +++- .../test_build_merge_hyperedges_and_prune.py | 108 +++- tests/test_cache.py | 346 +++++++++++- tests/test_carried_hyperedge_remap.py | 7 +- tests/test_dedup_remaps_hyperedges.py | 173 +++++- tests/test_export.py | 7 +- tests/test_extract_cli.py | 500 +++++++++++++++++- tests/test_hyperedge_member_shapes.py | 33 +- tests/test_hyperedge_roundtrip.py | 22 +- tests/test_hypergraph.py | 365 ++++++++++++- tests/test_merge_graphs_cli.py | 84 ++- tests/test_non_string_node_ids.py | 456 +++++++++++++++- tests/test_semantic_cleanup.py | 18 +- tests/test_watch.py | 53 +- 21 files changed, 3157 insertions(+), 199 deletions(-) diff --git a/graphify/build.py b/graphify/build.py index bb03fe1f54..75c74f478e 100644 --- a/graphify/build.py +++ b/graphify/build.py @@ -40,6 +40,273 @@ # legacy items that predate the _origin marker (#2334). _AST_LOC_RE = re.compile(r"^L\d") +# Hyperedges model group relationships. Pairwise relationships belong in the +# ordinary edge set, so a hyperedge is meaningful only with 3+ members. +MIN_HYPEREDGE_MEMBERS = 3 + + +def _has_minimum_hyperedge_members(he: object) -> bool: + """Return whether *he* has enough canonical members to form a group.""" + return ( + isinstance(he, dict) + and isinstance(he.get("nodes"), list) + and len(he["nodes"]) >= MIN_HYPEREDGE_MEMBERS + ) + + +def canonical_hyperedge(he: object) -> "dict | None": + """Return a canonical copy of hyperedge *he*, or None when it is not a group. + + Settles the *shape* every persistence boundary needs before a member count + can mean anything. Callers hand this raw producer output or a reloaded + graph.json, where three things go wrong: + + - a ``members``/``node_ids`` alias (#1561) carries no ``nodes`` key at all, + and would be read as "no members" rather than folded; + - a member listed twice, or two members an id remap collapsed onto one, + inflates a pair into an apparent group; + - a member object (``{"id": "a"}``) is not comparable to a node id (#2486). + + ``_normalize_hyperedge_members`` settles all three on a shallow copy, so the + caller's dict is untouched — the same contract ``cache._normalized`` keeps + for ``source_file``, and it matters because downstream steps (``_partial`` + marker stripping, manifest stamping) still read the original shape. + + Returns the canonical copy iff it has ``MIN_HYPEREDGE_MEMBERS`` distinct + usable members, else None. A pairwise relationship belongs in the ordinary + edge set. + + Membership is deliberately NOT checked here. Filtering members to a node set + needs the id-space bridge in :func:`_id_map` — comparing in a normalized + space and returning the container's own ids — so it lives in + :func:`gate_hyperedges` and :func:`gate_hyperedges_against_graph`. Doing it + both here and there would be two membership rules for one invariant, and + the weaker one drifting out of step is how several of this gate's own + defects arose. Boundaries that have no node set (the semantic cache, the + pre-manifest-stamp gate) call this directly. + """ + if not isinstance(he, dict): + return None + he = dict(he) + _normalize_hyperedge_members(he) + # Normalization only ASSIGNS `nodes` when `nodes` or an alias was already a + # list, so a malformed value (absent, None, a bare string, a dict) is still + # sitting there. Reject it: a later membership filter would iterate a string + # into characters or a dict into keys and fabricate a well-formed group out + # of junk. Same guard the per-site gates this helper replaced all carried. + if not isinstance(he.get("nodes"), list): + return None + return he if _has_minimum_hyperedge_members(he) else None + + +_MISSING = object() + + +def _member_keys(member: object): + """Yield the keys a canonical member ref may be found under, best first. + + The single definition of "what does this member name". Exactly two keys, in + priority order: the member as canonicalized (``_coerce_id`` has already run, + so ``7`` is ``"7"``), then its ``_normalize_id`` form, which is how + ``build_from_json`` heals a ref that drifted in casing or punctuation. + + Every membership decision in the feature goes through this — the writers' + gate, the cross-repo prefixer, the semantic-fragment filter, watch's + reconciliation and the cache's skipped-node prune. They compared member refs + against id sets independently before, and each one that resolved slightly + differently either dropped a valid group or kept an invalid one. + """ + if not _hashable(member): + return + yield member + if isinstance(member, str): + norm = _normalize_id(member) + if norm != member: + yield norm + + +def resolve_member_ref(member: object, raw_by_coerced: dict, default: object = None): + """Resolve *member* to the raw id its container carries, or *default*. + + Pair with :func:`_id_map`, whose keys are exactly what :func:`_member_keys` + looks for. Returns the raw id so a caller writing members back out keeps + them in the id space it is about to persist. + """ + for key in _member_keys(member): + if key in raw_by_coerced: + return raw_by_coerced[key] + return default + + +def member_in_id_space(member: object, ids: object) -> bool: + """Whether *member* names anything in *ids*, under the shared lookup order. + + For the callers that need a yes/no rather than the resolved id — watch's + whole-group reconciliation drop and the cache's skipped-node prune. Build + *ids* with :func:`node_id_set`, which carries the same keys. + """ + return any(key in ids for key in _member_keys(member)) + + + +def _id_map(ids: object, normalized: bool = True) -> dict: + """Map every id in *ids* to itself, keyed by both id spaces members use. + + The one place the id spaces are bridged. Member refs are coerced by + ``_normalize_hyperedge_members``, so membership has to be tested in the + coerced space — but whatever survives has to be handed back in the ids the + caller is actually persisting, or the written file names members no node + has. Compare in a normalized space, return raw. + + Two layers of key, in priority order: + + 1. the coerced id (``_coerce_id``), so ``7`` and ``"7"`` are one node; + 2. the ``_normalize_id`` form, so a member that drifted in casing or + punctuation (``Foo-Bar`` for node ``foo_bar``) still resolves. This + mirrors the ``norm_to_id`` healing ``build_from_json`` applies on the + clustered path — without it the raw writers, which never reach + ``build_from_json``, drop a group that is perfectly resolvable. + + Exact keys always win: layer 2 only fills gaps, so an id present verbatim is + never redirected to a different node that merely normalizes the same way. + + ``normalized=False`` omits layer 2 entirely. Callers that compare two id + sets against each other need it: with aliases present, a node ``foo_bar`` + in one set and a *distinct* node ``Foo-Bar`` in the other share the key + ``foo_bar``, so set arithmetic conflates two different nodes. Decide such + questions in the exact space, then add aliases for lookup. + + First writer wins within a layer, except that an id already equal to its + coerced form is preferred: with both ``7`` and ``"7"`` present they are two + distinct nodes collapsing to one key, and the exact match is the honest + answer. ``None`` and unhashable ids are skipped, per :func:`node_id_set`. + """ + out: dict = {} + aliases: dict = {} + for raw in ids or (): + if raw is None or not _hashable(raw): + continue + key = _coerce_id(raw) + if key not in out or (raw == key and out[key] != key): + out[key] = raw + if normalized and isinstance(key, str): + norm = _normalize_id(key) + if norm != key: + aliases.setdefault(norm, raw) + for norm, raw in aliases.items(): + out.setdefault(norm, raw) + return out + + +def node_id_map(nodes: object, normalized: bool = True) -> dict: + """:func:`_id_map` over node *records* — the shape writers hold node lists in. + + A non-dict entry and an id-less node contribute nothing; only ``n["id"]`` + is an id here. ``_id_map`` alone cannot tell the two shapes apart, and + guessing wrong would read a malformed list entry as a node id. + """ + return _id_map( + (n["id"] for n in (nodes or ()) if isinstance(n, dict) and "id" in n), + normalized=normalized, + ) + + +def node_id_set(nodes: object, normalized: bool = True) -> set: + """Collect the ids from node records *nodes* that a member could name. + + Ids are coerced with :func:`_coerce_id`, the same normalization member refs + receive, so the two sides are compared in one space. Without it a numeric + node id — a supported input that #2326 heals — stays ``7`` here while its + member ref becomes ``"7"``, `"7" in {7}` is False, and every member of an + otherwise valid group is dropped. + + An id-less node contributes nothing. An unhashable id is skipped rather than + added: a persisted ``list``/``dict`` id is deliberately tolerated by the + build path for the validator to report, and putting one in a set raises; + ``None`` is skipped for the same reason it is not a usable member — it would + let a null member count towards the minimum. + + Use this where only membership is asked. Where survivors are written back + out next to the nodes, gate through :func:`gate_hyperedges` instead, which + keeps the ids in the caller's own space. + """ + return set(node_id_map(nodes, normalized=normalized)) + + +def _gate(hyperedges: object, raw_by_coerced: "dict | None") -> "tuple[list[dict], int]": + """Canonicalize *hyperedges* against a prebuilt id map, survivors first. + + Takes the map rather than the container so a caller gating many groups walks + its nodes once. Rebuilding per candidate is what turns a linear gate into + O(nodes x hyperedges), which on a merged corpus of thousands of groups is + the whole cost of the operation. + + Resolution goes through the map instead of a plain ``in`` test, because the + map carries the normalized fallback layer (see :func:`_id_map`) and because + the surviving members must be the container's own ids. Members are deduped + again afterwards: two refs that resolve onto one node are one member, or a + pair would pass as a group. + + Pass None for the map where no node set exists — the semantic cache stores + per-file fragments, and the pre-manifest-stamp gate runs before the final + node set is known — in which case only shape and distinct cardinality are + checked and members stay in their coerced form. + """ + incoming = list(hyperedges or ()) + kept: list[dict] = [] + for he in incoming: + # No node set here: shape, coercion and dedupe only. Membership is + # applied below so it can resolve through the map's fallback layer. + candidate = canonical_hyperedge(he) + if candidate is None: + continue + if raw_by_coerced is not None: + resolved: list = [] + seen: set = set() + for m in candidate["nodes"]: + raw = resolve_member_ref(m, raw_by_coerced, _MISSING) + if raw is _MISSING or raw in seen: + continue + seen.add(raw) + resolved.append(raw) + candidate["nodes"] = resolved + if not _has_minimum_hyperedge_members(candidate): + continue + kept.append(candidate) + return kept, len(incoming) - len(kept) + + +def gate_hyperedges( + hyperedges: object, nodes: object = None, +) -> "tuple[list[dict], int]": + """Canonicalize *hyperedges*, returning the survivors and how many were cut. + + The shared shape of every writer's gate. Pass the node *records* about to be + persisted to filter members by membership; survivors come back in those + records' own id space, because the raw ``--no-cluster`` writers persist the + records unchanged. Pass None where no node set exists. + + Returns the count rather than logging, because the callers word their own + messages: the raw writer, the exclusion-only prune and the pre-stamp gate + each say something different about what the drop means. + """ + return _gate(hyperedges, node_id_map(nodes) if nodes is not None else None) + + +def gate_hyperedges_against_graph( + hyperedges: object, G: object, +) -> "tuple[list[dict], int]": + """Gate *hyperedges* against *G*'s nodes, in *G*'s own id space. + + :func:`gate_hyperedges` for callers whose container is a graph rather than a + node list — iterating an ``nx.Graph`` yields the ids themselves. The id + space matters just as much here: ``node_link_data`` writes ``{"id": 7}``, + and a member left as ``"7"`` is a dangling reference in the written file. + + Gate whole lists, not one candidate at a time — the map is built per call. + """ + return _gate(hyperedges, _id_map(G or ())) + def _is_ast_tier(item: dict) -> bool: """AST vs semantic tier. _origin wins when present; unstamped legacy items @@ -110,6 +377,32 @@ def _is_ast_tier(item: dict) -> bool: _HE_MEMBER_ALIASES = ("members", "node_ids") +def _is_usable_member_ref(value: object) -> bool: + """Whether *value* could name a graph node, so may count towards cardinality. + + One rule for both member shapes. The bare and object branches of + ``_coerce_hyperedge_member_refs`` each had their own copy and drifted twice — + the object branch rejected ``{"id": None}`` while a bare ``None`` was kept, + then the reverse once booleans were rejected in the bare branch only. Each + time, an unusable ref padded the count: the semantic cache has no node set to + filter members against, so a two-real-member group with a junk third member + was cached and stamped as valid, then dropped on replay by the graph-backed + revalidation — a cache hit yielding nothing for a file marked as covered. + + Unhashable values cannot be compared against a node set at all. ``None`` and + ``""`` name nothing. Booleans are excluded for the reason ``_coerce_id`` + already refuses to str-coerce them: ``True`` is not a number the model meant + as an id. A genuine numeric id survives — ``_coerce_id`` turns 7 into "7" and + 0 into "0" — and the explicit ``bool`` test keeps ``0``/``1`` out of that + case despite ``bool`` subclassing ``int``. + """ + return ( + _hashable(value) + and value not in (None, "") + and not isinstance(value, bool) + ) + + def _coerce_hyperedge_member_refs(he: dict, members: list) -> list: """Coerce a hyperedge member list to hashable scalar ids, deduped in order. @@ -128,7 +421,7 @@ def _coerce_hyperedge_member_refs(he: dict, members: list) -> list: for ref in members: if isinstance(ref, dict): inner = _coerce_id(ref.get("id")) - if inner in (None, "") or not _hashable(inner): + if not _is_usable_member_ref(inner): print( f"[graphify] WARNING: hyperedge " f"'{he.get('id', '?')}' has a member object with no usable " @@ -137,7 +430,13 @@ def _coerce_hyperedge_member_refs(he: dict, members: list) -> list: ) continue ref = inner - elif not _hashable(ref): + elif not _is_usable_member_ref(ref := _coerce_id(ref)): + # Same rule AND the same coercion as the object branch above. The + # coercion is what makes the dedupe below agree with replay: the + # build path str-coerces numeric members via _coerce_non_string_ids, + # so `7` and `"7"` are one node id. Keyed on the raw Python value + # they counted as two, letting `[7, "7", "b"]` pass the cache gate as + # a group and then collapse to a pair and be dropped on replay. print( f"[graphify] WARNING: hyperedge " f"'{he.get('id', '?')}' has an unusable member reference " @@ -1281,6 +1580,21 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat # verbatim — never leaks an absolute path from a semantic subagent (#1418). kept_hyperedges = [] for he in hyperedges: + # Reject a shape this boundary cannot validate, rather than letting + # it fall past the member check below into G.graph["hyperedges"]. + # Aliases were folded at the top of this function, so a non-list + # `nodes` here is genuinely malformed — and it would otherwise be + # read back by every consumer of the graph's metadata (report, wiki, + # the html exporter, watch's topology compare), each of which + # assumes the canonical shape. + if not isinstance(he, dict) or not isinstance(he.get("nodes"), list): + print( + f"[graphify] WARNING: dropping hyperedge " + f"{he.get('id', '?') if isinstance(he, dict) else he!r} — its " + f"member list is malformed (not a list).", + file=sys.stderr, + ) + continue if isinstance(he, dict) and he.get("source_file"): he["source_file"] = _norm_source_file(he["source_file"], _root) # Validate members against the built node set (#1916): a hyperedge @@ -1288,12 +1602,17 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat # G.graph["hyperedges"] verbatim and reach graph.json dangling, # even from a live (non-cache) extraction. Mirror the pairwise-edge # handling above: remap mismatched ids via normalization first, - # then drop members that still don't resolve; drop the hyperedge - # itself when no valid member remains (single-member hyperedges - # are legal in this codebase, e.g. a per-file flow, so we prune - # rather than require two survivors). + # then drop members that still don't resolve. If pruning leaves a + # pair (or singleton), the relationship belongs in ordinary edges, + # not the hyperedge set, so drop the hyperedge as a whole. if isinstance(he, dict) and isinstance(he.get("nodes"), list): + # Count DISTINCT members: the dedupe _normalize_hyperedge_members + # did above is undone by three later steps that can map two ids + # onto one — the semantic re-key, the doc-twin fold, and the + # norm_to_id remap right here (a ghost onto its AST twin, or + # `Foo` onto `foo`). Three positions naming two nodes is a pair. valid_members = [] + seen_members: set = set() for m in he["nodes"]: try: hash(m) @@ -1301,12 +1620,14 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat continue if m not in node_set and isinstance(m, str): m = norm_to_id.get(_normalize_id(m), m) - if m in node_set: + if m in node_set and m not in seen_members: + seen_members.add(m) valid_members.append(m) - if not valid_members: + if len(valid_members) < MIN_HYPEREDGE_MEMBERS: print( f"[graphify] WARNING: dropping hyperedge " - f"{he.get('id', '?')!r} — none of its members " + f"{he.get('id', '?')!r} — fewer than " + f"{MIN_HYPEREDGE_MEMBERS} members from " f"{he.get('nodes')!r} match built nodes.", file=sys.stderr, ) @@ -1380,6 +1701,14 @@ def build( for n in combined["nodes"]: if isinstance(n, dict): _fold_node_aliases(n) + # Canonicalize hyperedge members before dedup for the same reason (#1561): + # dedup rewires members onto survivors via _remap_hyperedge_members, which + # can only read the canonical `nodes` list, so an alias-keyed (`members` / + # `node_ids`) or duplicate-member hyperedge would be passed through + # un-rewired. Idempotent — build_from_json's own fold below then finds + # the aliases already gone and stays silent. + for he in combined["hyperedges"]: + _normalize_hyperedge_members(he) combined["nodes"], combined["edges"] = deduplicate_entities( combined["nodes"], combined["edges"], communities={}, dedup_llm_backend=dedup_llm_backend, root=root, @@ -1998,6 +2327,22 @@ def _prune_match(sf: "str | None") -> bool: file=sys.stderr, ) + # Final gate on hyperedge cardinality — whether or not a prune ran. build() + # validated members when it assembled the graph; the deleted-source prune + # above may have removed nodes since, and a carried group must not reach + # graph.json degraded to a pair or holding a dangling member after an + # incremental update. With no prune nothing removes nodes between build() + # and here, so on that path this is a defensive gate, not a live repair. + # Guarded on the key being present: build_from_json only sets + # G.graph["hyperedges"] when hyperedge metadata flowed through (an explicit + # [] marks a full wipeout, #2485), and to_json keys its "file already holds + # hyperedges but the graph carries none" warning on the key being ABSENT. + # Manufacturing an empty list here would silence that diagnostic. + if "hyperedges" in G.graph: + G.graph["hyperedges"], _ = gate_hyperedges_against_graph( + G.graph.get("hyperedges", []), G, + ) + # Safety check: refuse to SILENTLY drop nodes (#479, reworked in #2497). # The old count comparison ran against the post-replace `existing_nodes`, # which had already lost the re-extracted sources' old nodes — so it could @@ -2100,14 +2445,55 @@ def prefix_graph_for_global( hyperedges = H.graph.get("hyperedges") if isinstance(hyperedges, list): rewritten = [] + # Built once per graph, not per hyperedge: a semantic graph can carry + # thousands of groups, and rebuilding this map inside the loop makes + # prefixing O(nodes x hyperedges). + # Keyed through _id_map, so a member resolves here exactly as it does at + # the gate: coerced form first, then the _normalize_id form. Keyed on + # the coerced spelling alone, a member that drifted in casing or + # punctuation stayed unprefixed while its node became `repo::foo_bar`, + # and attach_hyperedges then dropped a group the gate calls valid — + # two halves of one invariant disagreeing. + _old_by_key = _id_map(relabel) + _coerced_relabel = { + key: relabel[old] for key, old in _old_by_key.items() if old in relabel + } for he in hyperedges: if isinstance(he, dict): he = dict(he) + # Canonicalize BEFORE mapping through relabel (#1561): only a + # canonical `nodes` list of scalar ids can be prefixed. An + # alias-keyed (`members`/`node_ids`) or object-member group + # would otherwise keep its unprefixed ids while every node gains + # the `repo::` prefix, and the attach boundary downstream then + # discards the whole group for having no member backed by a node. + _normalize_hyperedge_members(he) if isinstance(he.get("nodes"), list): - he["nodes"] = [ - relabel.get(m, m) if _hashable(m) else m - for m in he["nodes"] - ] + # Look the member up in the COERCED id space. Normalization + # above turns a numeric member into "7" while `relabel` is + # keyed by the raw node id `7`, so a raw lookup misses and + # the member stays unprefixed while its node becomes + # `repo::7` — after which attach_hyperedges drops the group + # for having no member backed by a node. + _prefixed: list = [] + _seen: set = set() + for m in he["nodes"]: + if not _hashable(m): + _prefixed.append(m) + continue + new_m = resolve_member_ref(m, _coerced_relabel, _MISSING) + if new_m is _MISSING: + # No node of this graph: leave it for the attach + # boundary to drop, as before. + new_m = m + # Deduped after resolution: an exact and a drifted ref to + # one node prefix to the same id, and counting both would + # let a pair reach the attach boundary as a group. + if new_m in _seen: + continue + _seen.add(new_m) + _prefixed.append(new_m) + he["nodes"] = _prefixed if he.get("id"): he["id"] = f"{repo_tag}::{he['id']}" rewritten.append(he) diff --git a/graphify/cache.py b/graphify/cache.py index 622ba6cff9..9e45fc2756 100644 --- a/graphify/cache.py +++ b/graphify/cache.py @@ -12,6 +12,15 @@ from collections.abc import Callable, Iterable from pathlib import Path +from graphify.build import ( + _coerce_id, + _normalize_id, + canonical_hyperedge, + gate_hyperedges as _gate_hyperedges, + member_in_id_space, + node_id_set, +) + # Output directory name — override with GRAPHIFY_OUT env var for worktrees or # shared-output setups. Accepts a relative name ("graphify-out-feature") or an # absolute path ("/shared/graphify-out"). Single source of truth in graphify.paths @@ -1282,9 +1291,21 @@ def check_semantic_cache( result = load_cached(p, root, kind=kind, cache_root=cache_root, prompt=prompt, prompt_file=prompt_file) if result is not None: + # Canonicalize on READ as well as on write. An entry written before + # the cardinality gate can hold nothing but a two-member group: the + # raw list is non-empty so the zero-output rule at save time passes, + # the file replays as a hit, the gates downstream then drop the pair, + # and the file is never freshly extracted — repeating every run. + # Writing cannot heal an entry that is only ever read. Cardinality + # and shape need no node set, so they can be judged here; membership + # cannot and stays with the graph-backed gates. + _hes, _ = _gate_hyperedges(result.get("hyperedges")) + if not result.get("nodes") and not _hes: + uncached.append(fpath) + continue cached_nodes.extend(result.get("nodes", [])) cached_edges.extend(result.get("edges", [])) - cached_hyperedges.extend(result.get("hyperedges", [])) + cached_hyperedges.extend(_hes) else: uncached.append(fpath) @@ -1472,6 +1493,11 @@ def _normalized(item: dict) -> dict: if src: by_file[src]["edges"].append(e) for h in (hyperedges or []): + # No node set here — the cache stores fragments, not a built graph — so + # this checks shape and cardinality only. + h = canonical_hyperedge(h) + if h is None: + continue h = _normalized(h) src = h.get("source_file", "") if src: @@ -1522,35 +1548,61 @@ def group_skipped(fpath: str) -> bool: # member (whole-hyperedge drop, mirroring #1895) — references one. Gated # on allowed_source_files so unscoped callers stay byte-identical. if allowed_paths is not None: - skipped_ids: set = set() - written_ids: set = set() + # Coerced, via node_id_set: canonical_hyperedge has already coerced the + # member refs, so a raw set would leave `"7" in {7}` False and a group + # naming a node from a skipped source would be cached and dangle on + # every replay. node_id_set applies the same id-less/unhashable skips + # this loop did by hand. + # + # Built EXACT first, because the duplicate-attribution subtraction below + # is set arithmetic between two id sets: with normalized aliases present, + # a skipped `foo_bar` and a distinct written `Foo-Bar` share the key + # `foo_bar`, and subtracting would delete the skipped node outright. + skipped_exact: set = set() + written_exact: set = set() for fpath, result in by_file.items(): - target = skipped_ids if group_skipped(fpath) else written_ids - for n in result["nodes"]: - nid = n.get("id") - if nid is None: - continue - try: - hash(nid) - except TypeError: - continue - target.add(nid) + target = skipped_exact if group_skipped(fpath) else written_exact + target |= node_id_set(result["nodes"], normalized=False) # A duplicate-attribution node (defined in a skipped AND a written # group) still reaches the cache — don't over-prune references to it. - skipped_ids -= written_ids + # Decided in the exact space, then aliased for lookup; an alias is not + # added when a written node already owns that exact id. + skipped_exact -= written_exact + skipped_ids = _with_lookup_aliases(skipped_exact, written_exact) if skipped_ids: def edge_dangles(e: dict) -> bool: + """Whether *e* references a node from a skipped source group. + + Endpoints are coerced on the way in for the same reason the id + sets are: edge endpoints reach the cache raw, so a numeric one + would stop matching its own node the moment the set is coerced. + """ try: - return e.get("source") in skipped_ids or e.get("target") in skipped_ids + return ( + _coerce_id(e.get("source")) in skipped_ids + or _coerce_id(e.get("target")) in skipped_ids + ) except TypeError: # Non-hashable endpoint from an untrusted result; leave it # to build-time validation rather than fail the save. return False def hyperedge_dangles(h: dict) -> bool: + """Whether *h* names a node from a skipped source group. + + Members go through the shared lookup order rather than a raw + set intersection: a member that drifted in casing or + punctuation resolves everywhere else in the feature, so an + exact intersection missed it and the group was cached with a + reference to a node deliberately not written — under-pruning, + and it reappears if another layer later supplies that id. + """ try: - return bool(skipped_ids & set(h.get("nodes") or [])) + return any( + member_in_id_space(m, skipped_ids) + for m in (h.get("nodes") or []) + ) except TypeError: return False @@ -1615,6 +1667,13 @@ def hyperedge_dangles(h: dict) -> bool: ) if is_partial: result = {**result, "partial": True} + # Canonicalize BEFORE filtering: the union above may carry a legacy + # alias-keyed entry written before the cache normalized members. + result["hyperedges"] = [ + c + for h in result.get("hyperedges", []) + if (c := canonical_hyperedge(h)) is not None + ] # A semantic extraction with zero nodes and zero hyperedges is not a valid # standalone extraction (#2927): edge-only or empty results must not be # cached, so that subsequent runs can re-dispatch and retry the file (#933/#1666). @@ -1638,6 +1697,26 @@ def hyperedge_dangles(h: dict) -> bool: return saved +def _with_lookup_aliases(exact_ids: set, veto_ids: set) -> set: + """Add normalized lookup aliases to *exact_ids*, skipping *veto_ids* owners. + + The two dangling prunes decide duplicate attribution by subtracting one + exact id set from another, then have to look members up by the same + two-key order the rest of the feature uses. Expanding before the + subtraction conflates a node with a distinct one that normalizes the same + way; expanding after is safe, except that an alias must not be added when a + node in *veto_ids* owns that id exactly — otherwise a member naming the + vetoed node resolves onto the pruned one. + """ + out = set(exact_ids) + for key in exact_ids: + if isinstance(key, str): + alias = _normalize_id(key) + if alias != key and alias not in veto_ids: + out.add(alias) + return out + + def scope_semantic_result( result: dict, root: Path = Path("."), @@ -1706,29 +1785,53 @@ def _hashable(value) -> bool: if bucket == "nodes" and item.get("id") is not None: nid = item["id"] if _hashable(nid): - dropped_ids.add(nid) + # Exact only here, for the same reason as the skipped + # prune above: the subtraction below is set arithmetic + # between two id sets, and a shared normalized alias + # would conflate two distinct nodes. + dropped_ids |= node_id_set([item], normalized=False) continue if bucket == "nodes" and item.get("id") is not None and _hashable(item["id"]): - kept_ids.add(item["id"]) + kept_ids |= node_id_set([item], normalized=False) kept.append(item) result[bucket] = kept # A duplicate-attribution node (defined in a dropped AND a kept group) # survives the filter — don't prune references to it. dropped_ids -= kept_ids + dropped_ids = _with_lookup_aliases(dropped_ids, kept_ids) if dropped_ids: def edge_dangles(e: dict) -> bool: + """Whether *e* references a node dropped as out of scope. + + Endpoints are coerced on the way in, because the id sets above are: + a raw numeric endpoint would stop matching its own node otherwise. + """ try: - return e.get("source") in dropped_ids or e.get("target") in dropped_ids + return ( + _coerce_id(e.get("source")) in dropped_ids + or _coerce_id(e.get("target")) in dropped_ids + ) except TypeError: # Non-hashable endpoint from an untrusted result; leave it # to build-time validation rather than fail here. return False def hyperedge_dangles(h: dict) -> bool: + """Whether *h* names a node dropped as out of scope. + + Through the shared lookup order rather than a raw set + intersection, for the same reason as the skipped-node prune above: + an exact intersection misses a member that resolves everywhere + else, leaving the group holding a reference to a node this + function has just removed. + """ try: - return bool(dropped_ids & set(h.get("nodes") or [])) + return any( + member_in_id_space(_coerce_id(m), dropped_ids) + for m in (h.get("nodes") or []) + ) except TypeError: return False diff --git a/graphify/cli.py b/graphify/cli.py index 6642f57d52..797e52b0b4 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -580,6 +580,23 @@ def _zero_node_stamped_semantic_sources( return healed +def _gate_hyperedges(hyperedges: object, nodes: object = None) -> "tuple[list, int]": + """Canonicalize *hyperedges*, returning the survivors and the drop count. + + A thin module-level seam over :func:`graphify.build.gate_hyperedges`, for two + reasons. It keeps the gate's fan-out off ``dispatch_command`` — which is + already thousands of lines and every writer in it would otherwise add its + own import alias — and it keeps the ``graphify.build`` import function-local, + since ``cli`` deliberately imports only ``graphify.paths`` at module scope so + ``graphify install`` works before networkx is present. + + Pass the node list about to be written to filter members by membership; omit + it where no node set exists yet (the pre-manifest-stamp gate). + """ + from graphify.build import gate_hyperedges as _gate + return _gate(hyperedges, nodes) + + def _prune_graph_json_sources(graph_path: Path, stale_sources: list[str]) -> int: """Drop nodes/edges/hyperedges owned by ``stale_sources`` from graph.json in place. Returns the number of nodes removed. @@ -590,6 +607,21 @@ def _prune_graph_json_sources(graph_path: Path, stale_sources: list[str]) -> int newly-excluded file's nodes survive forever (#1909). ``stale_sources`` comes from :func:`_stale_graph_sources`, i.e. the graph's own ``source_file`` spellings, so exact string matching is enough. + + Hyperedges get the same revalidation the clustered path gets from + build_merge: an entry owned by a surviving file can still name a node this + prune just removed, so members are filtered to the survivors and the group + is dropped when that leaves it under ``MIN_HYPEREDGE_MEMBERS``. Two + wrinkles worth knowing: + + - BOTH hyperedge slots are rewritten. ``to_json`` persists them top-level + and under ``graph`` (#2484), and :func:`_zero_node_stamped_semantic_sources` + unions the two when judging whether a doc's manifest stamp is honest + (#2927) — so a stale nested copy of a dropped group would keep that doc + looking covered and it would never be re-dispatched. + - The return value counts NODES only, so a hyperedge-only rewrite returns 0 + and the caller prints nothing; the stderr line below is what makes such a + rewrite attributable. """ try: data = json.loads(graph_path.read_text(encoding="utf-8")) @@ -612,18 +644,67 @@ def _prune_graph_json_sources(graph_path: Path, stale_sources: list[str]) -> int and e.get("source") not in removed_ids and e.get("target") not in removed_ids ] - kept_hyper = [ - h for h in data.get("hyperedges", []) - if isinstance(h, dict) and h.get("source_file") not in stale - ] - if n_removed == 0 and len(kept_edges) == len(data.get(links_key, [])) and ( - len(kept_hyper) == len(data.get("hyperedges", [])) + # Read whichever slot actually holds the list. A node_link_data-only writer + # emits hyperedges solely under `graph` with no top-level key (#2485, the + # shape build_from_json folds), so reading only the top level would find + # nothing to revalidate and then overwrite the nested slot with that empty + # result — destroying valid groups. The isinstance guards also absorb a + # legacy {"hyperedges": null}, which would otherwise raise TypeError out of + # this function (the try above wraps only the JSON load). + raw_hyper = data.get("hyperedges") + if not isinstance(raw_hyper, list): + _graph_md = data.get("graph") + # isinstance, not `or {}`: a legacy or hand-edited file can hold a + # non-dict `graph` value, and `.get` on a str/list/int raises + # AttributeError out of this function, aborting the whole prune. The + # nested-slot sync below already guards this way. + nested_hyper = _graph_md.get("hyperedges") if isinstance(_graph_md, dict) else None + raw_hyper = nested_hyper if isinstance(nested_hyper, list) else [] + # gate_hyperedges collects the surviving ids for us, skipping the id-less + # and malformed-id nodes this path keeps (unlike the raw --no-cluster path, + # where dedupe_nodes has already dropped them) — either would otherwise let + # a null member count or abort the prune outright. + kept_hyper, _ = _gate_hyperedges( + [h for h in raw_hyper if isinstance(h, dict) and h.get("source_file") not in stale], + kept_nodes, + ) + # The nested slot has to be part of the change test, not just the write. + # The pre-revalidation pruner filtered only the top-level list, so an + # upgraded graph.json can carry a stale nested copy of a group the top level + # already lost. Comparing the top level alone would find nothing to do and + # return early, leaving that copy for _zero_node_stamped_semantic_sources to + # keep counting as coverage (#2927). + _nested = (data.get("graph") or {}).get("hyperedges") if isinstance(data.get("graph"), dict) else None + _nested_needs_sync = isinstance(_nested, list) and _nested != kept_hyper + if ( + n_removed == 0 + and len(kept_edges) == len(data.get(links_key, [])) + and kept_hyper == raw_hyper + and not _nested_needs_sync ): return 0 + if kept_hyper != raw_hyper or _nested_needs_sync: + # Worded for a content change, not a drop count: the comparison above is + # by value, so this also fires when a member was pruned from a group that + # still has enough left, when a legacy alias-keyed entry was healed, or + # when only a stale nested copy needed reconciling. + print( + f"[graphify extract] rewrote {len(kept_hyper)} hyperedge(s) in " + f"{graph_path.name} ({len(raw_hyper) - len(kept_hyper)} dropped " + f"below the minimum surviving members).", + file=sys.stderr, + ) data["nodes"] = kept_nodes data[links_key] = kept_edges if "hyperedges" in data: data["hyperedges"] = kept_hyper + # Keep BOTH slots in step (#2484/#2485): to_json persists hyperedges + # top-level AND under `graph`, and _zero_node_stamped_semantic_sources + # unions the two when judging whether a doc's manifest stamp is honest + # (#2927). A stale nested copy of a group we just dropped would keep that + # doc looking covered, so it would never be re-dispatched. + if isinstance(data.get("graph"), dict) and "hyperedges" in data["graph"]: + data["graph"]["hyperedges"] = kept_hyper from graphify.export import backup_if_protected as _backup _backup(graph_path.parent) from graphify.paths import write_json_atomic @@ -1057,11 +1138,18 @@ def _clone_repo( def _reenter_main() -> None: + """Re-dispatch through ``__main__.main`` after rewriting ``sys.argv``.""" from graphify.__main__ import main main() def dispatch_command(cmd: str) -> None: + """Run the subcommand named *cmd*, reading its flags from ``sys.argv``. + + The single entry point every ``graphify `` invocation goes through. + Each branch parses its own arguments and exits via ``sys.exit`` rather than + returning a status, so callers get the process exit code directly. + """ if cmd == "provider": from graphify.llm import _custom_providers_path, BACKENDS import json as _json @@ -2577,6 +2665,26 @@ def _load_graph(p: str): file=sys.stderr, ) sys.exit(1) + # This writer composes two graph.json files and serializes the result + # directly — it never reaches build_from_json or to_json, so nothing + # else applies the hyperedge invariant to it. A legacy branch carrying + # a pair or a dangling group would have that metadata written straight + # back into graph.json, the same omission watch's raw writer had. + # Guarded on the key being present so an absent slot stays absent + # (#2485); note also that nx.compose takes graph attrs from one side + # only, so a group present solely on the current side is already lost + # before this point — pre-existing compose behaviour, not this gate. + if "hyperedges" in merged.graph: + from graphify.build import gate_hyperedges_against_graph as _gate_vs_graph + merged.graph["hyperedges"], _dropped_md = _gate_vs_graph( + merged.graph.get("hyperedges", []), merged, + ) + if _dropped_md: + print( + f"[graphify merge-driver] dropped {_dropped_md} hyperedge(s) " + f"that are not group relationships over the merged nodes.", + file=sys.stderr, + ) try: out_data = _jg.node_link_data(merged, edges="links") except TypeError: @@ -4055,6 +4163,25 @@ def _progress(idx: int, total: int, _result: dict) -> None: print(f"[graphify extract] Cargo: {len(cargo_result['nodes'])} nodes, " f"{len(cargo_result['edges'])} edges") + # Drop hyperedges that can never become a group BEFORE the merge and + # before manifest stamping. _stamped_manifest_files counts a hyperedge + # as output for its source file (#1920), so a doc whose only result was + # an under-cardinality group would be stamped as successfully extracted + # and then contribute nothing to graph.json, leaving the #2927 graph + # heal to notice a run later. Shape and cardinality only — membership + # needs the final node set, so it stays with build_from_json and the + # raw --no-cluster gate below. + sem_result["hyperedges"], _dropped_sem_hes = _gate_hyperedges( + sem_result.get("hyperedges") + ) + if _dropped_sem_hes: + print( + f"[graphify extract] dropped {_dropped_sem_hes} semantic " + f"hyperedge(s) that are not group relationships, before " + f"manifest stamping.", + file=sys.stderr, + ) + # Merge AST + semantic + pg_result + cargo_result. Order matters for deduplication: passing AST # first means semantic node attributes win on collision (richer labels # for symbols also referenced in docs). Hyperedges only come from the @@ -4232,6 +4359,23 @@ def _invalidate_file_manifest_for_db_graph() -> None: _e["source_file"] = ( _node_sf.get(_e.get("source")) or _node_sf.get(_e.get("target")) or "" ) + # Hyperedge parity for the raw path: it never reaches build_from_json + # / build_merge / to_json, whose gates canonicalize member lists, + # drop members with no backing node and enforce the 3-member + # minimum — and merge_raw_extraction carries hyperedges verbatim + # through replace/prune, so a deleted source leaves a cross-file + # group naming a node that is no longer here. Gate on the final + # (post-dedupe) node ids, the same set about to be written. + merged["hyperedges"], _dropped_raw_hes = _gate_hyperedges( + merged.get("hyperedges"), merged["nodes"], + ) + if _dropped_raw_hes: + print( + f"[graphify extract] dropped {_dropped_raw_hes} hyperedge(s) " + f"with fewer than the minimum surviving members from the " + f"raw graph.", + file=sys.stderr, + ) # RT-parity for the raw path: an incomplete build must not force a # partial graph over a larger complete one here either. The clustered # path gets this from to_json's #479 guard; this path never calls diff --git a/graphify/dedup.py b/graphify/dedup.py index 4816f103b6..b089490061 100644 --- a/graphify/dedup.py +++ b/graphify/dedup.py @@ -12,6 +12,12 @@ from pathlib import Path from graphify._minhash import MinHash, MinHashLSH +from graphify.build import ( + _coerce_id, + _has_minimum_hyperedge_members, + _hashable, + _is_usable_member_ref, +) from rapidfuzz.distance import DamerauLevenshtein, Jaro, JaroWinkler @@ -460,6 +466,76 @@ def _report_id_collision(nid: str, survivor: dict, losers: list[dict]) -> None: # ── main entry point ────────────────────────────────────────────────────────── +def _member_raw_forms(hyperedges: list) -> list[dict]: + """Per hyperedge, map each member's coerced key back to its original id. + + Captured BEFORE remapping, because remapping coerces members and that + discards which raw form the author actually wrote. With two distinct nodes + ``7`` and ``"7"`` present, a member ``7`` and a member ``"7"`` both become + ``"7"``, and nothing downstream can tell them apart — so restoration would + bind both to whichever node the id map prefers, silently moving one member + to a different node. + """ + originals: list[dict] = [] + for he in hyperedges or (): + seen: dict = {} + if isinstance(he, dict) and isinstance(he.get("nodes"), list): + for m in he["nodes"]: + raw = m.get("id") if isinstance(m, dict) else m + if _hashable(raw): + seen.setdefault(_coerce_id(raw), raw) + originals.append(seen) + return originals + + +def _restore_member_id_space(hyperedges: list, nodes: list, originals: list) -> None: + """Rewrite each member to the id its node record carries, in place. + + Members are coerced during remapping so they can be compared and deduped; + the node records are not. Comparing in one space and returning in another + is what puts a member like ``"7"`` next to a node ``{"id": 7}``. Object + members keep their other fields, so only their ``id`` is rewritten. + + *originals* (from :func:`_member_raw_forms`) wins whenever the member's own + raw form is itself one of the returned nodes: the id map has to pick one + node per coerced key, and preferring its choice over what the author wrote + would rebind a member from the node it named to a colliding one. The map is + the fallback, for a member whose raw form names nothing. + + A member with no matching node either way is left untouched — remapping is + not membership gating. + """ + from graphify.build import node_id_map + + raw_by_coerced = node_id_map(nodes) + if not raw_by_coerced: + return + raw_ids = { + n["id"] for n in (nodes or ()) + if isinstance(n, dict) and _hashable(n.get("id")) + } + + def resolved(key: object, index: int) -> object: + """The node id member *key* should carry, preferring its own raw form.""" + own = (originals[index] if index < len(originals) else {}).get(key) + if own is not None and own in raw_ids: + return own + return raw_by_coerced.get(key) + + for i, he in enumerate(hyperedges or ()): + if not isinstance(he, dict) or not isinstance(he.get("nodes"), list): + continue + restored: list = [] + for m in he["nodes"]: + if isinstance(m, dict): + raw = resolved(_coerce_id(m.get("id")), i) + restored.append(dict(m, id=raw) if raw is not None else m) + else: + raw = resolved(m, i) + restored.append(m if raw is None else raw) + he["nodes"] = restored + + def _remap_hyperedge_members(hyperedges: list[dict], remap: dict[str, str]) -> None: """Rewire hyperedge member ids onto dedup survivors, in place. @@ -473,31 +549,54 @@ def _remap_hyperedge_members(hyperedges: list[dict], remap: dict[str, str]) -> N behaviour dropped the loser without promoting it, which shrank the group *and* lost the participant. Order is preserved so a rebuilt graph does not churn. + + A group left with fewer than ``MIN_HYPEREDGE_MEMBERS`` distinct survivors is + dropped — a pair belongs in the ordinary edge set. Entries without a + canonical ``nodes`` list are passed through unchanged, never deleted. """ + kept: list = [] for he in hyperedges: - if not isinstance(he, dict): - continue - members = he.get("nodes") + members = he.get("nodes") if isinstance(he, dict) else None if not isinstance(members, list): + # Nothing this remap can interpret: a non-dict, a member-less dict, or + # an alias-keyed (`members`/`node_ids`) entry build_from_json has not + # canonicalized yet. Pass it through untouched — the kept-list rewrite + # below must never turn "skip" into "delete". Such an entry is NOT + # rewired onto survivors here; on the build() path that cannot bite, + # because build() normalizes hyperedges before dedup, so only direct + # deduplicate_entities(..., hyperedges=) callers reach this branch. + kept.append(he) continue seen: set = set() rewired: list = [] for m in members: - if isinstance(m, str): - new_id = remap.get(m, m) - entry = new_id - elif isinstance(m, dict): - raw = m.get("id") + if isinstance(m, dict): + # Object members keep their other fields (role, weight, ...), so + # this branch cannot delegate to canonical_hyperedge, which + # flattens them to bare ids. + raw = _coerce_id(m.get("id")) new_id = remap.get(raw, raw) if isinstance(raw, str) else raw - entry = dict(m, id=new_id) if new_id != raw else m + entry = dict(m, id=new_id) if new_id != m.get("id") else m else: - new_id, entry = None, m - if isinstance(new_id, str): - if new_id in seen: - continue - seen.add(new_id) + # Coerce first, so a numeric id becomes the "7" form every other + # member path uses and can therefore be remapped and deduped. + new_id = _coerce_id(m) + if isinstance(new_id, str): + new_id = remap.get(new_id, new_id) + entry = new_id + if not _is_usable_member_ref(new_id): + # Drop rather than append: these used to be appended untouched + # and then counted by list length, so [None, 7, False] survived + # as a three-POSITION group with no usable member in it. + continue + if new_id in seen: + continue + seen.add(new_id) rewired.append(entry) he["nodes"] = rewired + if _has_minimum_hyperedge_members(he): + kept.append(he) + hyperedges[:] = kept def deduplicate_entities( @@ -535,8 +634,43 @@ def deduplicate_entities( f"Cross-project dedup is disabled — run dedup per-repo before merging." ) + def _finish( + out_nodes: list[dict], out_edges: list[dict], remap: dict[str, str], + ) -> tuple[list[dict], list[dict]]: + """Rewire hyperedge members onto survivors, then return the pair. + + Hyperedge members are node references exactly like edge endpoints, and + must follow the survivor for the same reason. Without this the member + naming a merged-away id was simply absent from the rebuilt graph: the + group lost a participant silently, could fall under the 3-member + threshold that makes it a hyperedge at all, and left NO dangling + reference, so a referential-integrity check saw nothing wrong (#2805). + + Every return goes through here, including the short-circuits where + nothing merged: the cardinality contract must not depend on whether a + remap happened, or a direct caller keeps a pair that every other path + rejects. + + Members are then put back into the id space of the nodes being returned. + The remap pass coerces every member so it can be looked up (`7` becomes + `"7"`), but a direct caller's node records keep `7`, so returning the + coerced form would hand back a group of dangling references — the shape + #1916 removed. `build()` coerces node ids before dedup, so on that path + this is a no-op; it matters for direct + `deduplicate_entities(..., hyperedges=)` callers. + + Unresolved members are left as they are rather than dropped: this + function remaps, it does not gate membership. The writers do that. + """ + if hyperedges: + # Captured before the remap coerces the members away. + _originals = _member_raw_forms(hyperedges) + _remap_hyperedge_members(hyperedges, remap) + _restore_member_id_space(hyperedges, out_nodes, _originals) + return out_nodes, out_edges + if len(nodes) <= 1: - return nodes, edges + return _finish(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 @@ -592,7 +726,7 @@ def deduplicate_entities( unique_nodes = list(seen_ids.values()) if len(unique_nodes) <= 1: - return unique_nodes, edges + return _finish(unique_nodes, edges, {}) # ── pass 1: exact normalization ─────────────────────────────────────────── norm_to_nodes: dict[str, list[dict]] = defaultdict(list) @@ -807,7 +941,7 @@ def deduplicate_entities( # ── apply remap ─────────────────────────────────────────────────────────── if not remap: - return unique_nodes, edges + return _finish(unique_nodes, edges, {}) total = len(remap) msg = f"[graphify] Deduplicated {total} node(s)" @@ -823,15 +957,6 @@ def deduplicate_entities( msg += f" ({', '.join(parts)})" print(msg + ".", flush=True) - # Hyperedge members are node references exactly like edge endpoints, and - # must follow the survivor for the same reason. Without this the member - # naming a merged-away id was simply absent from the rebuilt graph: the - # group lost a participant silently, could fall under the 3-member threshold - # that makes it a hyperedge at all, and left NO dangling reference, so a - # referential-integrity check saw nothing wrong (#2805). - if hyperedges: - _remap_hyperedge_members(hyperedges, remap) - deduped_nodes = [n for n in unique_nodes if n["id"] not in remap] deduped_edges = [] for edge in edges: @@ -853,7 +978,7 @@ def deduplicate_entities( if e["source"] != e["target"]: deduped_edges.append(e) - return deduped_nodes, deduped_edges + return _finish(deduped_nodes, deduped_edges, remap) def _pick_winner(nodes: list[dict]) -> dict: diff --git a/graphify/export.py b/graphify/export.py index 69136befce..652d14bae4 100644 --- a/graphify/export.py +++ b/graphify/export.py @@ -15,7 +15,11 @@ from networkx.readwrite import json_graph from graphify.security import sanitize_label from graphify.analyze import _node_community_map -from graphify.build import edge_data +from graphify.build import ( + MIN_HYPEREDGE_MEMBERS, + edge_data, + gate_hyperedges_against_graph, +) from graphify.paths import stem_filename_budget from graphify.exporters.graphdb import push_to_falkordb, push_to_neo4j # noqa: E402,F401 @@ -178,8 +182,18 @@ def _yaml_str(s: str) -> str: def attach_hyperedges(G: nx.Graph, hyperedges: list) -> None: - """Store hyperedges in the graph's metadata dict.""" - existing = G.graph.get("hyperedges", []) + """Store hyperedges in the graph's metadata dict. + + merge-graphs hands persisted metadata straight to this boundary with no + build_from_json in between, so the shared gate does the alias fold, member + coercion and dedupe before filtering to nodes G actually has. + + Both lists are gated whole rather than one candidate at a time: the gate + walks G's nodes once per call to build its id map, so per-candidate gating + would cost O(nodes x hyperedges) on exactly the merged, thousands-of-groups + corpora this boundary exists for. + """ + existing, _ = gate_hyperedges_against_graph(G.graph.get("hyperedges", []), G) # Skip id-less persisted entries when seeding the dedup set (#2775): the # semantic extractor emits hyperedges with no `id` and build.py persists them # verbatim, so a prior graph.json can contain id-less hyperedges. A hard @@ -187,10 +201,11 @@ def attach_hyperedges(G: nx.Graph, hyperedges: list) -> None: # symmetric with the `.get("id")` guard the loop below already applies to the # incoming set. seen_ids = {h["id"] for h in existing if h.get("id")} - for h in hyperedges: - if h.get("id") and h["id"] not in seen_ids: - existing.append(h) - seen_ids.add(h["id"]) + incoming, _ = gate_hyperedges_against_graph(hyperedges, G) + for candidate in incoming: + if candidate.get("id") and candidate["id"] not in seen_ids: + existing.append(candidate) + seen_ids.add(candidate["id"]) G.graph["hyperedges"] = existing @@ -264,6 +279,16 @@ def existing_graph_node_count(path: "str | Path"): def to_json(G: nx.Graph, communities: dict[int, list[str]], output_path: str, *, force: bool = False, built_at_commit: str | None = None, community_labels: dict[int, str] | None = None) -> bool: + """Write *G* to ``output_path`` as graph.json, returning whether it wrote. + + The canonical persistence boundary: assigns communities, canonicalizes field + order for byte-stable diffs, gates hyperedge cardinality, and writes + atomically. + + Returns True when it wrote. Returns False without writing when the new graph + would shrink an existing one (#479) — ``force=True`` bypasses that guard and + writes anyway. + """ # Safety check: refuse to silently shrink an existing graph (#479) existing_path = Path(output_path) if not force and existing_path.exists(): @@ -395,9 +420,37 @@ def _canonical(item: dict, lead: tuple[str, ...]) -> dict: f"extraction if this is unexpected.", file=sys.stderr, ) - hyperedges = sorted(getattr(G, "graph", {}).get("hyperedges", []), key=_json_sort_key) + # Gate at the final persistence boundary too. Every internal producer + # (build_from_json, build_merge, attach_hyperedges) already canonicalizes, + # so in normal flows this drops nothing — but to_json is public API, and a + # library caller populating G.graph["hyperedges"] itself would otherwise + # write a pair, a duplicate-inflated group or a dangling member into both + # JSON slots. Filters a copy: an export must not mutate the caller's graph. + # The #2485 absent-vs-empty warning above is unaffected — it keys on the + # metadata key being missing from G.graph, which this does not touch. + _raw_hyperedges = getattr(G, "graph", {}).get("hyperedges", []) + if not isinstance(_raw_hyperedges, list): + # A direct caller can leave this as None; iterating it raised TypeError + # before the graph was written at all. + _raw_hyperedges = [] + _valid_hyperedges, _dropped_hyperedges = gate_hyperedges_against_graph(_raw_hyperedges, G) + if _dropped_hyperedges: + print( + f"[graphify] WARNING: dropping " + f"{_dropped_hyperedges} hyperedge(s) with " + f"fewer than {MIN_HYPEREDGE_MEMBERS} members backed by graph nodes " + f"while writing {output_path}.", + file=sys.stderr, + ) + hyperedges = sorted(_valid_hyperedges, key=_json_sort_key) if isinstance(data.get("graph"), dict) and "hyperedges" in data["graph"]: - data["graph"]["hyperedges"] = hyperedges + # Rebind rather than mutate in place: node_link_data hands back the + # SAME graph-attrs dict the caller's G owns (the by-reference sharing + # #2484 was diagnosed from), so assigning into it would edit their + # graph — and now that the list is filtered, that edit would silently + # drop their hyperedges. Replacing the key preserves its position, so + # the field-order/byte-stability contract is untouched. + data["graph"] = {**data["graph"], "hyperedges": hyperedges} data["hyperedges"] = hyperedges # Fallback provenance comes from the repo the graph is being written INTO # (output_path lives in /graphify-out/), never the shell's cwd — diff --git a/graphify/semantic_cleanup.py b/graphify/semantic_cleanup.py index 09cac2d847..6b089df698 100644 --- a/graphify/semantic_cleanup.py +++ b/graphify/semantic_cleanup.py @@ -14,7 +14,10 @@ import re from pathlib import Path -from .build import _normalize_hyperedge_members +from .build import ( + _normalize_hyperedge_members, + gate_hyperedges, +) # Labels longer than this many characters, or containing >= this many words, # are candidates for being sentence-like rationale text rather than entity names. @@ -185,7 +188,7 @@ def sanitize_semantic_fragment(fragment: dict) -> dict: 3. Strips nodes whose only distinguishing field is the label itself (empty id — likely LLM hallucination). 4. Filters hyperedges so they cannot reference removed or unknown node - IDs after the cleanup passes above. A hyperedge with fewer than two + IDs after the cleanup passes above. A hyperedge with fewer than three surviving members is dropped. Returns the same dict for convenience. @@ -272,27 +275,14 @@ def sanitize_semantic_fragment(fragment: dict) -> dict: keep_edges.append(e) # ---- pass 4: filter hyperedges to surviving node IDs -------------------- - surviving_ids: set[str] = {n.get("id", "") for n in keep_nodes} - surviving_ids.discard("") - keep_hyperedges: list[dict] = [] - for he in hyperedges: - if not isinstance(he, dict): - continue - # Fold alias member keys (members/node_ids) onto `nodes` (#1561) so an - # alias-keyed hyperedge isn't silently dropped below for a missing - # `nodes` list before build can canonicalize it. - _normalize_hyperedge_members(he) - he_nodes = he.get("nodes") - if not isinstance(he_nodes, list): - continue - filtered = [ref for ref in he_nodes if isinstance(ref, str) and ref in surviving_ids] - if len(filtered) < 2: - # A hyperedge needs at least two surviving members to be meaningful. - continue - if len(filtered) != len(he_nodes): - he = dict(he) - he["nodes"] = filtered - keep_hyperedges.append(he) + # Delegated to the shared writer gate rather than filtering here. This pass + # used to keep its own copy of the rule — an exact `ref in surviving_ids` + # test — which resolved fewer members than the gate and build_from_json do, + # so a member that had merely drifted in casing or punctuation was removed + # and the group dropped below the minimum. The gate folds member aliases, + # coerces, dedupes, resolves through the shared id map and enforces the + # cardinality rule, and hands members back in keep_nodes' own id space. + keep_hyperedges, _dropped = gate_hyperedges(hyperedges, keep_nodes) fragment["nodes"] = keep_nodes fragment["edges"] = keep_edges diff --git a/graphify/watch.py b/graphify/watch.py index fbcbbc011f..d1c8ecbdbe 100644 --- a/graphify/watch.py +++ b/graphify/watch.py @@ -714,6 +714,58 @@ def _keep_edge(edge: dict) -> bool: return preserved_edges +def _member_id_space(*node_lists: list) -> set: + """The key set a hyperedge member is looked up in, over several node lists. + + :func:`graphify.build.node_id_set` carries both keys + :func:`graphify.build.member_in_id_space` searches, so a numeric or drifted + member resolves here exactly as it does at every writer gate. + """ + from graphify.build import node_id_set + + space: set = set() + for nodes in node_lists: + space |= node_id_set(nodes) + return space + + +def _coerce_member(value: object) -> object: + """Canonicalize one member ref the way the shared gate does.""" + from graphify.build import _coerce_id + + return _coerce_id(value) + + +def _member_in_id_space(member: object, ids: object) -> bool: + """Whether *member* names anything in *ids*, under the shared lookup order. + + Delegates to :func:`graphify.build.member_in_id_space` so watch's + reconciliation resolves a member exactly as every writer gate does. Kept as + a module-level seam for the same reason as :func:`_gated_hyperedges`: the + ``graphify.build`` import stays function-local. + """ + from graphify.build import member_in_id_space + + return member_in_id_space(member, ids) + + +def _gated_hyperedges(hyperedges: list, nodes: list) -> list: + """Canonicalize *hyperedges* against the ids present in *nodes*. + + The gate every other persistence boundary applies, for watch's raw + ``--no-cluster`` writer — which never builds a graph and so never reaches + build_from_json's member revalidation or to_json's own gate. Skips id-less + and unhashable node ids when collecting the id set: this path keeps both + (only ``dedupe_nodes`` drops id-less ones, and a persisted malformed id is + deliberately left for the validator), and either would otherwise poison the + set or raise. + """ + from graphify.build import gate_hyperedges + + kept, _dropped = gate_hyperedges(hyperedges, nodes) + return kept + + def _reconcile_existing_graph( existing_graph: Path, result: dict, @@ -970,13 +1022,30 @@ def _ignored_now(identity: str) -> bool: edge.get("id") for edge in result.get("hyperedges", []) if edge.get("id") } preserved_hyperedges = [] + # Built once, before the loop: a watched graph can carry thousands of + # groups, and rebuilding this per hyperedge makes reconciliation + # O(nodes x hyperedges). + member_id_space = _member_id_space(result.get("nodes") or [], preserved_nodes) for edge in existing.get("hyperedges", []): members = edge.get("nodes", edge.get("members", edge.get("node_ids", []))) if edge.get("id") in new_hyperedge_ids or source_paths.is_evicted( edge, hyperedge_evicted_source_identities ): continue - if isinstance(members, list) and any(member not in all_ids for member in members): + # Membership goes through the shared lookup order, not a raw `in`: + # a member that drifted in casing or punctuation is resolvable, and + # the gate downstream keeps such a group — so a raw test here + # deleted, on an unrelated rebuild, a group the rest of the feature + # calls valid. The whole-group drop semantics are unchanged: any + # member that resolves to nothing still evicts the group. + # + # Against its own key set, not `all_ids`: that set holds the node + # ids raw for the edge-endpoint checks above, so a numeric node id + # stays `7` there while its member coerces to `"7"`. + if isinstance(members, list) and any( + not _member_in_id_space(_coerce_member(member), member_id_space) + for member in members + ): continue preserved_hyperedges.append(edge) @@ -1775,10 +1844,22 @@ def _failed(f: str) -> bool: # without it, --no-cluster + repeated `update` accumulate duplicates and edge # counts diverge across build modes (#1317). from graphify.build import dedupe_edges as _dedupe_edges, dedupe_nodes as _dedupe_nodes + _cand_nodes = _dedupe_nodes(result.get("nodes", [])) candidate_graph_data = { - **{k: v for k, v in result.items() if k not in ("edges", "nodes")}, - "nodes": _dedupe_nodes(result.get("nodes", [])), + **{k: v for k, v in result.items() if k not in ("edges", "nodes", "hyperedges")}, + "nodes": _cand_nodes, "links": _dedupe_edges(result.get("edges", [])), + # Hyperedge parity for watch's raw writer. This path never builds + # a graph, so it misses build_from_json's member revalidation and + # to_json's gate, and _reconcile_existing_graph carries an + # existing group forward on source eviction and dangling members + # alone — never cardinality. A legacy pair whose members both + # still exist therefore survived every `update --no-cluster`. + # Gate against the deduplicated candidate node ids, the set about + # to be written. + "hyperedges": _gated_hyperedges( + result.get("hyperedges") or [], _cand_nodes, + ), # Inherit the existing graph's directed flag (#2342) so # `graphify update --no-cluster` can't silently drop it - # `result` (the raw merged extraction) never carries one. diff --git a/tests/test_build.py b/tests/test_build.py index b376b173be..7d956d3d79 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -1140,6 +1140,8 @@ def _write_two_tier_graph(graph_path): "_origin": "ast"}, {"id": "auth_flow", "label": "Auth Flow", "file_type": "concept", "source_file": "docs/readme.md", "source_location": None}, + {"id": "auth_policy", "label": "Auth Policy", "file_type": "concept", + "source_file": "docs/readme.md", "source_location": None}, ], "links": [ {"source": "docs_readme", "target": "docs_readme_intro", @@ -1149,7 +1151,7 @@ def _write_two_tier_graph(graph_path): ], "hyperedges": [ {"id": "auth_group", "label": "Auth Group", - "nodes": ["docs_readme", "auth_flow"], "relation": "form", + "nodes": ["docs_readme", "auth_flow", "auth_policy"], "relation": "form", "confidence": "INFERRED", "source_file": "docs/readme.md"}, ], } @@ -1553,18 +1555,108 @@ def test_build_from_json_prunes_dangling_hyperedge_members(capsys): "nodes": [ {"id": "alpha", "label": "alpha", "file_type": "code", "source_file": "a.py"}, {"id": "beta", "label": "beta", "file_type": "code", "source_file": "a.py"}, + {"id": "gamma", "label": "gamma", "file_type": "code", "source_file": "a.py"}, ], "edges": [], "hyperedges": [ - {"id": "he_partial", "nodes": ["alpha", "beta", "ghost_member"], "source_file": "a.py"}, + { + "id": "he_partial", + "nodes": ["alpha", "beta", "gamma", "ghost_member"], + "source_file": "a.py", + }, + { + "id": "he_below_minimum", + "nodes": ["alpha", "beta", "ghost_member"], + "source_file": "a.py", + }, {"id": "he_all_ghost", "nodes": ["ghost1", "ghost2"], "source_file": "a.py"}, ], } G = build_from_json(ext) hes = {h["id"]: h for h in G.graph.get("hyperedges", [])} assert set(hes) == {"he_partial"}, "an all-dangling hyperedge must be dropped" - assert hes["he_partial"]["nodes"] == ["alpha", "beta"] - assert "he_all_ghost" in capsys.readouterr().err + assert hes["he_partial"]["nodes"] == ["alpha", "beta", "gamma"] + assert "he_below_minimum" in capsys.readouterr().err + + +def _doc(nid: str, label: str, source_file: str) -> dict: + """Build a document-tier node dict.""" + return {"id": nid, "label": label, "file_type": "document", "source_file": source_file} + + +def test_build_from_json_counts_distinct_members_after_doc_twin_fold(capsys): + """The member dedupe in _normalize_hyperedge_members runs BEFORE the doc-twin + fold (#1799) maps `` onto `_doc`. A hyperedge naming both twins + therefore ends up with the same id twice, and the minimum-cardinality check + must count DISTINCT members, not list positions: three positions with two + distinct ids is a pair, not a group.""" + ext = { + "nodes": [ + _doc("docs_guide", "Guide", "docs/guide.md"), + _doc("docs_guide_doc", "Guide (semantic)", "docs/guide.md"), + _doc("docs_other_doc", "Other", "docs/other.md"), + _doc("docs_third_doc", "Third", "docs/third.md"), + ], + "edges": [], + "hyperedges": [ + {"id": "he_twins_pair", "source_file": "docs/other.md", + "nodes": ["docs_guide", "docs_guide_doc", "docs_other_doc"]}, + {"id": "he_twins_kept", "source_file": "docs/other.md", + "nodes": ["docs_guide", "docs_guide_doc", "docs_other_doc", "docs_third_doc"]}, + ], + } + G = build_from_json(ext) + hes = {h["id"]: h for h in G.graph.get("hyperedges", [])} + assert set(hes) == {"he_twins_kept"}, "two distinct members is a pair, not a hyperedge" + assert hes["he_twins_kept"]["nodes"] == ["docs_guide_doc", "docs_other_doc", "docs_third_doc"] + assert "he_twins_pair" in capsys.readouterr().err + + +@pytest.mark.parametrize("malformed", [ + {"id": "s", "nodes": "a,b,c"}, # a string, not a list + {"id": "d", "nodes": {"a": 1}}, # a dict, not a list + {"id": "n", "label": "x"}, # no members at all + "not-a-dict", +], ids=["string-nodes", "dict-nodes", "no-nodes", "non-dict"]) +def test_build_from_json_does_not_persist_a_malformed_hyperedge(malformed, capsys): + """The member revalidation only runs for a dict with a list-valued `nodes`; + every other shape used to fall straight through to the kept list, so + G.graph["hyperedges"] could carry metadata this very boundary would reject. + Aliases are already folded by this point, so a non-list `nodes` here is + genuinely malformed and must not be persisted for report/wiki/html consumers + to read back.""" + ext = { + "nodes": [ + {"id": n, "label": n, "file_type": "code", "source_file": "a.py"} + for n in ("alpha", "beta", "gamma") + ], + "edges": [], + "hyperedges": [ + malformed, + {"id": "he_ok", "nodes": ["alpha", "beta", "gamma"], "source_file": "a.py"}, + ], + } + G = build_from_json(ext) + assert [h["id"] for h in G.graph.get("hyperedges", [])] == ["he_ok"] + + +def test_build_from_json_counts_distinct_members_after_case_remap(capsys): + """Same invariant via the other collapse path: a member that misses the node + set only by casing is remapped through norm_to_id onto the canonical id, so + `Foo` and `foo` become the same member and must be counted once.""" + ext = { + "nodes": [ + {"id": "foo", "label": "foo", "file_type": "code", "source_file": "a.py"}, + {"id": "bar", "label": "bar", "file_type": "code", "source_file": "a.py"}, + ], + "edges": [], + "hyperedges": [ + {"id": "he_cased", "nodes": ["foo", "Foo", "bar"], "source_file": "a.py"}, + ], + } + G = build_from_json(ext) + assert G.graph.get("hyperedges", []) == [] + assert "he_cased" in capsys.readouterr().err # --- foreign-absolute source_file must not leak into IDs -------------------- diff --git a/tests/test_build_merge_hyperedges_and_prune.py b/tests/test_build_merge_hyperedges_and_prune.py index 2e341c3b8e..f8d8291c7f 100644 --- a/tests/test_build_merge_hyperedges_and_prune.py +++ b/tests/test_build_merge_hyperedges_and_prune.py @@ -16,6 +16,7 @@ import os from pathlib import Path +import networkx as nx import pytest from graphify.build import build_merge, _infer_merge_root @@ -36,29 +37,39 @@ def _he_ids(G) -> set[str]: # ── #1574: hyperedge preservation ───────────────────────────────────────────── def _seed_two_file_graph(tmp_path): + """Write a two-file graph.json with per-file and cross-file hyperedges.""" root = tmp_path / "corpus" root.mkdir() graph_path = tmp_path / "graph.json" nodes = [ {"id": "a1", "label": "a1", "file_type": "document", "source_file": "a.md"}, + {"id": "a2", "label": "a2", "file_type": "document", "source_file": "a.md"}, + {"id": "a3", "label": "a3", "file_type": "document", "source_file": "a.md"}, {"id": "b1", "label": "b1", "file_type": "document", "source_file": "b.md"}, + {"id": "b2", "label": "b2", "file_type": "document", "source_file": "b.md"}, + {"id": "b3", "label": "b3", "file_type": "document", "source_file": "b.md"}, ] hyperedges = [ - {"id": "he_a", "label": "flow A", "source_file": "a.md", "nodes": ["a1"]}, - {"id": "he_b", "label": "flow B", "source_file": "b.md", "nodes": ["b1"]}, - {"id": "he_global", "label": "cross-file flow", "nodes": ["a1", "b1"]}, # no source_file + {"id": "he_a", "label": "flow A", "source_file": "a.md", "nodes": ["a1", "a2", "a3"]}, + {"id": "he_b", "label": "flow B", "source_file": "b.md", "nodes": ["b1", "b2", "b3"]}, + {"id": "he_global", "label": "cross-file flow", "nodes": ["a1", "b1", "b2"]}, ] _write_graph(graph_path, nodes, [], hyperedges) return root, graph_path def test_update_preserves_hyperedges_of_unchanged_files(tmp_path): + """#1574: an unchanged file's hyperedges must survive an incremental update.""" root, graph_path = _seed_two_file_graph(tmp_path) # Re-extract only b.md, with a fresh hyperedge for it. new_chunk = { - "nodes": [{"id": "b1", "label": "b1", "file_type": "document", "source_file": "b.md"}], + "nodes": [ + {"id": "b1", "label": "b1", "file_type": "document", "source_file": "b.md"}, + {"id": "b2", "label": "b2", "file_type": "document", "source_file": "b.md"}, + {"id": "b3", "label": "b3", "file_type": "document", "source_file": "b.md"}, + ], "edges": [], - "hyperedges": [{"id": "he_b_v2", "label": "flow B v2", "source_file": "b.md", "nodes": ["b1"]}], + "hyperedges": [{"id": "he_b_v2", "label": "flow B v2", "source_file": "b.md", "nodes": ["b1", "b2", "b3"]}], } G = build_merge([new_chunk], graph_path, dedup=False, root=root) ids = _he_ids(G) @@ -72,9 +83,13 @@ def test_update_without_root_still_preserves_hyperedges(tmp_path): """The runbook omits root; the fallback root must not break preservation.""" root, graph_path = _seed_two_file_graph(tmp_path) new_chunk = { - "nodes": [{"id": "b1", "label": "b1", "file_type": "document", "source_file": "b.md"}], + "nodes": [ + {"id": "b1", "label": "b1", "file_type": "document", "source_file": "b.md"}, + {"id": "b2", "label": "b2", "file_type": "document", "source_file": "b.md"}, + {"id": "b3", "label": "b3", "file_type": "document", "source_file": "b.md"}, + ], "edges": [], - "hyperedges": [{"id": "he_b_v2", "source_file": "b.md", "nodes": ["b1"]}], + "hyperedges": [{"id": "he_b_v2", "source_file": "b.md", "nodes": ["b1", "b2", "b3"]}], } G = build_merge([new_chunk], graph_path, dedup=False) # no root ids = _he_ids(G) @@ -83,17 +98,94 @@ def test_update_without_root_still_preserves_hyperedges(tmp_path): def test_deleted_file_hyperedges_are_pruned(tmp_path): + """A deleted file's own hyperedges go, and a cross-file group left under the minimum goes with them.""" root, graph_path = _seed_two_file_graph(tmp_path) deleted_abs = [str(root / "a.md")] G = build_merge([], graph_path, prune_sources=deleted_abs, dedup=False, root=root) ids = _he_ids(G) assert "he_a" not in ids # deleted file's hyperedge pruned assert "he_b" in ids # untouched file's hyperedge kept - assert "he_global" in ids # global hyperedge kept + assert "he_global" not in ids # deletion leaves fewer than 3 members # and its node is gone too assert "a1" not in set(G.nodes) +# ── minimum cardinality on a plain --update (no prune_sources) ─────────────── + +def _seed_single_node_graph(tmp_path, nodes): + """Write a graph.json holding just *nodes* and no hyperedges.""" + root = tmp_path / "corpus" + root.mkdir() + graph_path = tmp_path / "graph.json" + _write_graph(graph_path, nodes, [], []) + return root, graph_path + + +def test_update_without_prune_drops_hyperedge_collapsed_by_doc_twin_fold(tmp_path): + """A plain --update never reaches the prune branch, yet a new chunk can still + carry a group whose members collapse to two distinct ids once build_from_json + folds `` onto `_doc`. That pair must not be persisted.""" + root, graph_path = _seed_single_node_graph( + tmp_path, [{"id": "z1", "label": "z1", "file_type": "document", "source_file": "z.md"}]) + chunk = { + "nodes": [ + {"id": "docs_guide", "label": "Guide", "file_type": "document", "source_file": "docs/guide.md"}, + {"id": "docs_guide_doc", "label": "Guide (semantic)", "file_type": "document", + "source_file": "docs/guide.md"}, + {"id": "docs_other_doc", "label": "Other", "file_type": "document", "source_file": "docs/other.md"}, + ], + "edges": [], + "hyperedges": [{"id": "he_twins", "source_file": "docs/other.md", + "nodes": ["docs_guide", "docs_guide_doc", "docs_other_doc"]}], + } + G = build_merge([chunk], graph_path, dedup=False, root=root) + assert "he_twins" not in _he_ids(G) + + +_TWO_NODES = [ + {"id": "a1", "label": "a1", "file_type": "document", "source_file": "a.md"}, + {"id": "a2", "label": "a2", "file_type": "document", "source_file": "a.md"}, +] + + +def _fake_build_returning(hyperedges): + """Stand-in for build(): the seeded nodes (so the #479 shrink guard stays quiet) + plus whatever hyperedge metadata the test wants to smuggle past build().""" + def fake_build(*args, **kwargs): + """Stand in for build(), returning the seeded nodes plus canned hyperedge metadata.""" + G = nx.Graph() + for n in _TWO_NODES: + G.add_node(n["id"], **n) + if hyperedges is not None: + G.graph["hyperedges"] = hyperedges + return G + return fake_build + + +def test_update_without_prune_still_revalidates_hyperedges_against_final_graph(tmp_path, monkeypatch): + """The final revalidation gate must run whether or not a prune happened: a + hyperedge that leaves build() with a dangling member and fewer than three + survivors is dropped on a no-prune --update too.""" + root, graph_path = _seed_single_node_graph(tmp_path, _TWO_NODES) + monkeypatch.setattr( + "graphify.build.build", + _fake_build_returning([{"id": "he_dangling", "nodes": ["a1", "a2", "ghost"]}]), + ) + G = build_merge([], graph_path, dedup=False, root=root) + assert "he_dangling" not in _he_ids(G) + + +def test_update_without_prune_keeps_absent_hyperedge_key_absent(tmp_path, monkeypatch): + """#2485: a graph that never engaged hyperedge metadata has NO `hyperedges` key, + and to_json warns on that absence when the file on disk already holds some. + The unconditional revalidation must not manufacture an empty list and hide + that diagnostic.""" + root, graph_path = _seed_single_node_graph(tmp_path, _TWO_NODES) + monkeypatch.setattr("graphify.build.build", _fake_build_returning(None)) + G = build_merge([], graph_path, dedup=False, root=root) + assert "hyperedges" not in G.graph + + # ── #1571: root-less prune (absolute deleted paths vs relative node keys) ────── def test_prune_without_root_removes_ghost_nodes_via_grandparent_fallback(tmp_path): diff --git a/tests/test_cache.py b/tests/test_cache.py index f01a1cd295..ed67ade683 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -1341,11 +1341,19 @@ def test_save_semantic_cache_drops_hyperedges_touching_skipped_nodes(tmp_path): nodes = [ {"id": "kept", "source_file": "allowed.md"}, {"id": "kept2", "source_file": "allowed.md"}, + {"id": "kept3", "source_file": "allowed.md"}, {"id": "stray", "source_file": "outside.md"}, ] hyperedges = [ - {"id": "he_bad", "nodes": ["kept", "stray"], "source_file": "allowed.md"}, - {"id": "he_ok", "nodes": ["kept", "kept2"], "source_file": "allowed.md"}, + # Three members, so the cardinality gate keeps it and the assertion + # below is actually testing the skipped-node prune rather than passing + # because a pair was dropped first. + {"id": "he_bad", "nodes": ["kept", "kept2", "stray"], "source_file": "allowed.md"}, + { + "id": "he_ok", + "nodes": ["kept", "kept2", "kept3"], + "source_file": "allowed.md", + }, ] with pytest.warns(RuntimeWarning, match="out-of-scope source_file"): save_semantic_cache( @@ -1359,10 +1367,156 @@ def test_save_semantic_cache_drops_hyperedges_touching_skipped_nodes(tmp_path): assert {h["id"] for h in cached_hyperedges} == {"he_ok"} -def test_save_semantic_cache_unscoped_preserves_dangling_refs_verbatim(tmp_path): - """#1916 guard-rail: unscoped callers (allowed_source_files=None) must stay - byte-identical — no pruning happens even when an edge or hyperedge - references a node grouped under a ghost file.""" +def test_save_semantic_cache_prunes_numeric_skipped_ids_like_string_ones(tmp_path): + """The #1916 skipped-node prune has to compare in the coerced id space. + + `canonical_hyperedge` coerces members (`7` -> `"7"`) while `skipped_ids` was + collected raw, so `"7" in {7}` was False and a group naming a node from a + deliberately skipped source was cached anyway — dangling on every replay, + the exact shape #1916 removed. A string-id member of the same shape was + dropped correctly, which is what makes this a coercion bug and not a + disagreement about the prune's semantics. + """ + from graphify.cache import check_semantic_cache, save_semantic_cache + + allowed = tmp_path / "allowed.md" + allowed.write_text("# Allowed\n") + outside = tmp_path / "outside.md" + outside.write_text("# Outside\n") + + nodes = [ + {"id": "kept", "source_file": "allowed.md"}, + {"id": "kept2", "source_file": "allowed.md"}, + {"id": 7, "source_file": "outside.md"}, + ] + hyperedges = [ + {"id": "he_numeric", "nodes": [7, "kept", "kept2"], "source_file": "allowed.md"}, + ] + edges = [{"source": "kept", "target": 7, "source_file": "allowed.md"}] + with pytest.warns(RuntimeWarning, match="out-of-scope source_file"): + save_semantic_cache( + nodes, edges, hyperedges, root=tmp_path, + allowed_source_files=["allowed.md"], + ) + + _, cached_edges, cached_hyperedges, _ = check_semantic_cache( + [str(allowed)], root=tmp_path + ) + assert cached_hyperedges == [], ( + "a group naming a node from a skipped source must be pruned whether " + "that node's id is numeric or a string" + ) + assert cached_edges == [], ( + "the numeric endpoint must stay detectable once both sides are coerced" + ) + + +def test_scope_semantic_result_prunes_a_normalized_ref_to_a_dropped_node(tmp_path): + """`_scope_semantic_result` drops out-of-scope items and then prunes + references to the nodes it dropped. That prune intersected its raw id set + with the member spellings, so a member resolvable everywhere else in the + feature did not register and the group survived holding a reference to a + node that had just been removed. Twelfth site of one root cause, and the + second in this file: the two prunes are near-identical code.""" + from graphify.cache import scope_semantic_result + + allowed = tmp_path / "allowed.md" + allowed.write_text("# Allowed\n") + + result = { + "nodes": [ + {"id": "a", "source_file": "allowed.md"}, + {"id": "b", "source_file": "allowed.md"}, + {"id": "foo_bar", "source_file": "outside.md"}, + ], + "edges": [], + "hyperedges": [ + {"id": "grp", "nodes": ["Foo-Bar", "a", "b"], "source_file": "allowed.md"}, + ], + } + scope_semantic_result(result, root=tmp_path, allowed_source_files=["allowed.md"]) + + assert {n["id"] for n in result["nodes"]} == {"a", "b"}, "the node is dropped" + assert result["hyperedges"] == [], ( + "the group referenced the dropped node under a resolvable spelling" + ) + + +def test_save_semantic_cache_prunes_a_normalized_ref_to_a_skipped_node(tmp_path): + """The #1916 skipped-node prune intersects `skipped_ids` with the member + spellings directly, so a member the rest of the feature resolves — `Foo-Bar` + for skipped node `foo_bar` — did not register as touching a skipped node and + the group was cached. Under-pruning this time: the group replays with a + member no node backs, and reappears if another layer supplies that id.""" + from graphify.cache import save_semantic_cache + + allowed = tmp_path / "allowed.md" + allowed.write_text("# Allowed\n") + outside = tmp_path / "outside.md" + outside.write_text("# Outside\n") + + nodes = [ + {"id": "a", "source_file": "allowed.md"}, + {"id": "b", "source_file": "allowed.md"}, + {"id": "foo_bar", "source_file": "outside.md"}, + ] + hyperedges = [ + {"id": "grp", "nodes": ["Foo-Bar", "a", "b"], "source_file": "allowed.md"}, + ] + with pytest.warns(RuntimeWarning, match="out-of-scope source_file"): + save_semantic_cache( + nodes, [], hyperedges, root=tmp_path, + allowed_source_files=["allowed.md"], + ) + + from graphify.cache import check_semantic_cache + _, _, cached_hyperedges, _ = check_semantic_cache([str(allowed)], root=tmp_path) + assert cached_hyperedges == [], ( + "a group naming a skipped node must be pruned whatever spelling it uses" + ) + + +def test_save_semantic_cache_does_not_over_subtract_a_colliding_normalized_key(tmp_path): + """Duplicate attribution is subtracted between the skipped and written id + sets, and those sets now carry normalized lookup keys — so a skipped node + `foo_bar` and a *distinct* written node `Foo-Bar` share the key `foo_bar`, + the subtraction removed the skipped node entirely, and an exact member + `foo_bar` naming it was no longer detected as dangling. The subtraction has + to happen in the exact id space, before aliases are added.""" + from graphify.cache import check_semantic_cache, save_semantic_cache + + allowed = tmp_path / "allowed.md" + allowed.write_text("# Allowed\n") + outside = tmp_path / "outside.md" + outside.write_text("# Outside\n") + + nodes = [ + {"id": "Foo-Bar", "source_file": "allowed.md"}, + {"id": "a", "source_file": "allowed.md"}, + {"id": "b", "source_file": "allowed.md"}, + {"id": "foo_bar", "source_file": "outside.md"}, + ] + hyperedges = [ + # Names the SKIPPED node exactly, not the written one. + {"id": "grp", "nodes": ["foo_bar", "a", "b"], "source_file": "allowed.md"}, + ] + with pytest.warns(RuntimeWarning, match="out-of-scope source_file"): + save_semantic_cache( + nodes, [], hyperedges, root=tmp_path, + allowed_source_files=["allowed.md"], + ) + + _, _, cached_hyperedges, _ = check_semantic_cache([str(allowed)], root=tmp_path) + assert cached_hyperedges == [], ( + "the group names a skipped node exactly and must still be pruned" + ) + + +def test_save_semantic_cache_unscoped_drops_under_cardinality_hyperedges(tmp_path): + """#1916 guard-rail: unscoped callers (allowed_source_files=None) get no + dangling-reference pruning — the edge to a node grouped under a ghost file is + cached byte-identical. The hyperedge is dropped for a different reason: two + members is under the minimum cardinality, regardless of scoping.""" from graphify.cache import save_semantic_cache doc = tmp_path / "doc.md" @@ -1383,7 +1537,152 @@ def test_save_semantic_cache_unscoped_preserves_dangling_refs_verbatim(tmp_path) (cache_dir(tmp_path, "semantic") / f"{file_hash(doc, tmp_path)}.json").read_text() ) assert raw["edges"] == edges - assert raw["hyperedges"] == hyperedges + assert raw["hyperedges"] == [] + + +def test_save_semantic_cache_drops_two_member_hyperedge(tmp_path): + """Pairwise relationships must not be persisted as hyperedges.""" + from graphify.cache import load_cached, save_semantic_cache + + doc = tmp_path / "doc.md" + doc.write_text("# Doc\n") + nodes = [ + {"id": "a", "source_file": "doc.md"}, + {"id": "b", "source_file": "doc.md"}, + ] + hyperedges = [{"id": "pair", "nodes": ["a", "b"], "source_file": "doc.md"}] + + assert save_semantic_cache(nodes, [], hyperedges, root=tmp_path) == 1 + cached = load_cached(doc, root=tmp_path, kind="semantic") + assert cached is not None + assert cached["hyperedges"] == [] + + +def test_save_semantic_cache_normalizes_alias_members_before_cardinality_check(tmp_path): + """A `members`-keyed group (#1561 alias) with three members is a valid + hyperedge. The cardinality gate must read it through the same canonicalization + build applies, not drop it for lacking a `nodes` list — and the caller's dict + must be left untouched (the cache writer copies, like _normalized does).""" + from graphify.cache import load_cached, save_semantic_cache + + doc = tmp_path / "doc.md" + doc.write_text("# Doc\n") + nodes = [{"id": nid, "source_file": "doc.md"} for nid in ("a", "b", "c")] + he = {"id": "grp", "members": ["a", "b", "c"], "source_file": "doc.md"} + + assert save_semantic_cache(nodes, [], [he], root=tmp_path) == 1 + cached = load_cached(doc, root=tmp_path, kind="semantic") + assert [h["id"] for h in cached["hyperedges"]] == ["grp"] + assert cached["hyperedges"][0]["nodes"] == ["a", "b", "c"] + assert "members" not in cached["hyperedges"][0] + assert he == {"id": "grp", "members": ["a", "b", "c"], "source_file": "doc.md"} + + +def test_save_semantic_cache_counts_distinct_members(tmp_path): + """Three list positions with two distinct ids is a pair, not a group.""" + from graphify.cache import load_cached, save_semantic_cache + + doc = tmp_path / "doc.md" + doc.write_text("# Doc\n") + nodes = [{"id": nid, "source_file": "doc.md"} for nid in ("a", "b", "c")] + hyperedges = [ + {"id": "dupe_pair", "nodes": ["a", "a", "b"], "source_file": "doc.md"}, + {"id": "dupe_trio", "nodes": ["a", "a", "b", "c"], "source_file": "doc.md"}, + ] + + assert save_semantic_cache(nodes, [], hyperedges, root=tmp_path) == 1 + cached = load_cached(doc, root=tmp_path, kind="semantic") + assert {h["id"]: h["nodes"] for h in cached["hyperedges"]} == {"dupe_trio": ["a", "b", "c"]} + + +def test_save_semantic_cache_merge_existing_heals_legacy_alias_entry(tmp_path): + """A pre-existing cache entry may hold an alias-keyed hyperedge written before + the cache canonicalized members. The merge_existing union must fold it onto + `nodes` rather than delete a valid three-member group.""" + import json + + from graphify.cache import cache_dir, file_hash, load_cached, save_semantic_cache + + doc = tmp_path / "doc.md" + doc.write_text("# Doc\n") + nodes = [{"id": nid, "source_file": "doc.md"} for nid in ("a", "b", "c")] + save_semantic_cache(nodes, [], root=tmp_path, merge_existing=True) + cache = load_cached(doc, root=tmp_path, kind="semantic") + assert cache is not None + cache["hyperedges"] = [{"id": "legacy_alias", "members": ["a", "b", "c"]}] + path = cache_dir(tmp_path, "semantic") / f"{file_hash(doc, tmp_path)}.json" + path.write_text(json.dumps(cache), encoding="utf-8") + + save_semantic_cache( + [{"id": "d", "source_file": "doc.md"}], [], root=tmp_path, merge_existing=True, + ) + cached = load_cached(doc, root=tmp_path, kind="semantic") + assert [h["id"] for h in cached["hyperedges"]] == ["legacy_alias"] + assert cached["hyperedges"][0]["nodes"] == ["a", "b", "c"] + + +def test_check_semantic_cache_treats_an_undersized_legacy_entry_as_a_miss(tmp_path): + """A cache entry written before the cardinality gate can hold nothing but a + two-member group. Gating only on write cannot heal an entry that is merely + read: the raw list is non-empty, so the zero-output check passes, the file + is reported as a hit, the CLI gate then removes the pair, and the file is + never freshly extracted — repeating every run. Cardinality needs no node + set, so it can be judged on read.""" + import json + + from graphify.cache import cache_dir, check_semantic_cache, file_hash + + doc = tmp_path / "doc.md" + doc.write_text("# Doc\n") + entry = cache_dir(tmp_path, "semantic") + entry.mkdir(parents=True, exist_ok=True) + (entry / f"{file_hash(doc, tmp_path)}.json").write_text(json.dumps({ + "nodes": [], "edges": [], + "hyperedges": [{"id": "legacy_pair", "nodes": ["a", "b"]}], + }), encoding="utf-8") + + _nodes, _edges, hyperedges, uncached = check_semantic_cache([str(doc)], root=tmp_path) + assert hyperedges == [], "the pair must not be replayed as cached output" + assert [str(p) for p in uncached] == [str(doc)], ( + "an entry whose only output is unusable is a miss, so the file is re-extracted" + ) + + +def test_check_semantic_cache_keeps_a_valid_legacy_entry(tmp_path): + """The over-fix guard: an entry carrying a real group is still a hit.""" + import json + + from graphify.cache import cache_dir, check_semantic_cache, file_hash + + doc = tmp_path / "doc.md" + doc.write_text("# Doc\n") + entry = cache_dir(tmp_path, "semantic") + entry.mkdir(parents=True, exist_ok=True) + (entry / f"{file_hash(doc, tmp_path)}.json").write_text(json.dumps({ + "nodes": [], "edges": [], + "hyperedges": [{"id": "trio", "nodes": ["a", "b", "c"]}], + }), encoding="utf-8") + + _n, _e, hyperedges, uncached = check_semantic_cache([str(doc)], root=tmp_path) + assert [h["id"] for h in hyperedges] == ["trio"] + assert uncached == [] + + +def test_save_semantic_cache_does_not_cache_a_group_padded_by_a_null_member(tmp_path): + """The cache has no node set, so it cannot filter members by membership — but + `None`/`""` can never name a node in any graph. Counting them would cache a + two-real-member group that build_from_json drops on replay, leaving a cache + hit that produces no semantic data and a file that is never re-dispatched.""" + from graphify.cache import load_cached, save_semantic_cache + + doc = tmp_path / "doc.md" + doc.write_text("# Doc\n") + nodes = [{"id": nid, "source_file": "doc.md"} for nid in ("a", "b")] + hyperedges = [{"id": "padded", "nodes": ["a", "b", None], "source_file": "doc.md"}] + + assert save_semantic_cache(nodes, [], hyperedges, root=tmp_path) == 1 + cached = load_cached(doc, root=tmp_path, kind="semantic") + assert cached["hyperedges"] == [] def test_save_semantic_cache_merge_existing_prunes_only_incoming(tmp_path): @@ -1428,6 +1727,39 @@ def test_save_semantic_cache_merge_existing_prunes_only_incoming(tmp_path): assert not any("stray" in p for p in pairs) +def test_save_semantic_cache_merge_existing_heals_legacy_pair(tmp_path): + """A two-member group already on disk is pruned when the entry is next merged.""" + from graphify.cache import load_cached, save_semantic_cache + + doc = tmp_path / "doc.md" + doc.write_text("# Doc\n") + save_semantic_cache( + [{"id": "a", "source_file": "doc.md"}], + [], + root=tmp_path, + merge_existing=True, + ) + cache = load_cached(doc, root=tmp_path, kind="semantic") + assert cache is not None + cache["hyperedges"] = [{"id": "legacy_pair", "nodes": ["a", "b"]}] + + from graphify.cache import cache_dir, file_hash + import json + + path = cache_dir(tmp_path, "semantic") / f"{file_hash(doc, tmp_path)}.json" + path.write_text(json.dumps(cache), encoding="utf-8") + save_semantic_cache( + [{"id": "b", "source_file": "doc.md"}], + [], + root=tmp_path, + merge_existing=True, + ) + + healed = load_cached(doc, root=tmp_path, kind="semantic") + assert healed is not None + assert healed["hyperedges"] == [] + + # --- extraction-prompt fingerprinting (#1939) ------------------------------- diff --git a/tests/test_carried_hyperedge_remap.py b/tests/test_carried_hyperedge_remap.py index d5b4d10472..f7d54d6201 100644 --- a/tests/test_carried_hyperedge_remap.py +++ b/tests/test_carried_hyperedge_remap.py @@ -77,12 +77,17 @@ def test_edges_and_hyperedges_agree_on_the_survivor(tmp_path): def test_a_hyperedge_re_emitted_by_the_new_chunk_is_not_duplicated(tmp_path): + """The re-extracted version replaces the carried one rather than joining it.""" fresh = {"nodes": [{"id": "beta_node", "label": "Beta", "file_type": "concept", "source_file": "notes/group.md"}, {"id": "gamma_node", "label": "Gamma", "file_type": "concept", + "source_file": "notes/group.md"}, + {"id": "epsilon_node", "label": "Epsilon", "file_type": "concept", "source_file": "notes/group.md"}], "edges": [], - "hyperedges": [{**HYPEREDGE, "nodes": ["beta_node", "gamma_node"], "label": "The Group v2"}]} + "hyperedges": [{**HYPEREDGE, + "nodes": ["beta_node", "gamma_node", "epsilon_node"], + "label": "The Group v2"}]} G = build_merge([fresh], _baseline(tmp_path)) hes = [he for he in G.graph.get("hyperedges", []) if he["id"] == "the_group"] assert len(hes) == 1 diff --git a/tests/test_dedup_remaps_hyperedges.py b/tests/test_dedup_remaps_hyperedges.py index 446eb5ef7d..a1dc7a5be9 100644 --- a/tests/test_dedup_remaps_hyperedges.py +++ b/tests/test_dedup_remaps_hyperedges.py @@ -14,7 +14,7 @@ import pytest from graphify.build import build -from graphify.dedup import _remap_hyperedge_members +from graphify.dedup import _remap_hyperedge_members, deduplicate_entities def _node(nid, label): @@ -22,9 +22,10 @@ def _node(nid, label): "source_file": "notes/a.md"} -def _extraction(members): +def _extraction(members, key="nodes"): """Two nodes that normalise to the same label, so dedup merges them; the - hyperedge names the id that loses.""" + hyperedge names the id that loses. `key` lets a test spell the member list + with a legacy alias (`members` / `node_ids`) instead of canonical `nodes`.""" return { "nodes": [ _node("alpha_a", "Alpha Concept"), @@ -34,7 +35,7 @@ def _extraction(members): ], "edges": [], "hyperedges": [{"id": "the_group", "label": "The Group", - "nodes": members, "relation": "participate_in", + key: members, "relation": "participate_in", "confidence": "INFERRED", "confidence_score": 0.75, "source_file": "notes/a.md"}], } @@ -85,6 +86,15 @@ def test_an_untouched_hyperedge_is_unchanged(): assert _members(G) == ["alpha_a", "beta_node", "gamma_node"] +def test_an_alias_keyed_hyperedge_is_remapped_not_deleted(): + """A `members`-keyed group reaches dedup BEFORE build_from_json canonicalizes + it (#1561 fold runs later). It must be normalized first and then rewired like + any other hyperedge — not deleted for lacking a `nodes` list.""" + G = build([_extraction( + ["alpha_concept_long_variant_id", "beta_node", "gamma_node"], key="members")]) + assert _members(G) == ["alpha_a", "beta_node", "gamma_node"] + + # --------------------------------------------------------------------------- # _remap_hyperedge_members directly # --------------------------------------------------------------------------- @@ -93,9 +103,9 @@ def test_two_members_collapsing_onto_one_survivor_dedupe(): """They were the same entity, so one entry is right. The old code shrank the group AND lost the participant; this shrinks it because the members really were duplicates.""" - hes = [{"id": "h", "nodes": ["a_old", "a_new", "b"]}] + hes = [{"id": "h", "nodes": ["a_old", "a_new", "b", "c"]}] _remap_hyperedge_members(hes, {"a_old": "a", "a_new": "a"}) - assert hes[0]["nodes"] == ["a", "b"] + assert hes[0]["nodes"] == ["a", "b", "c"] def test_member_order_is_preserved(): @@ -105,9 +115,21 @@ def test_member_order_is_preserved(): def test_object_members_keep_their_other_fields(): - hes = [{"id": "h", "nodes": [{"id": "x_old", "role": "subject"}]}] + """Remapping an object-shaped member must preserve its non-id fields.""" + hes = [{ + "id": "h", + "nodes": [ + {"id": "x_old", "role": "subject"}, + {"id": "y", "role": "object"}, + {"id": "z", "role": "context"}, + ], + }] _remap_hyperedge_members(hes, {"x_old": "x"}) - assert hes[0]["nodes"] == [{"id": "x", "role": "subject"}] + assert hes[0]["nodes"] == [ + {"id": "x", "role": "subject"}, + {"id": "y", "role": "object"}, + {"id": "z", "role": "context"}, + ] @pytest.mark.parametrize("he", [ @@ -121,26 +143,147 @@ def test_malformed_hyperedges_do_not_raise(he): _remap_hyperedge_members([he], {"a": "b"}) +@pytest.mark.parametrize("members", [ + [None, 7, False], + [None, None, None], + ["a", None, False], +], ids=["mixed-junk", "all-null", "one-real-two-junk"]) +def test_unusable_members_do_not_make_up_the_minimum(members): + """The rewire appended every non-str, non-dict entry untouched and then + counted list length, so a direct caller kept a three-POSITION group whose + entries could not name a node at all — contradicting the cleanup this + function is supposed to guarantee on every exit path.""" + hes = [{"id": "h", "nodes": list(members)}] + _remap_hyperedge_members(hes, {}) + assert hes == [], f"none of {members!r} is a usable member id" + + +def test_a_numeric_member_is_canonicalized_while_rewiring(): + """A numeric id is usable and becomes its string form, matching the coercion + every other member path applies, so it can be remapped and deduped.""" + hes = [{"id": "h", "nodes": [7, "b", "c"]}] + _remap_hyperedge_members(hes, {"7": "seven"}) + assert hes[0]["nodes"] == ["seven", "b", "c"] + + def test_an_empty_remap_changes_nothing(): + """With nothing merged, a canonical group passes through untouched.""" hes = [{"id": "h", "nodes": ["a", "b", "c"]}] _remap_hyperedge_members(hes, {}) assert hes[0]["nodes"] == ["a", "b", "c"] +def test_an_entry_without_a_nodes_list_is_passed_through_not_deleted(): + """The remap can only rewire a canonical `nodes` list. An entry it cannot + interpret (alias-keyed here) must survive untouched for build_from_json to + heal — the kept-list rewrite must never turn "skip" into "delete".""" + hes = [{"id": "h", "members": ["a", "b", "c"]}] + _remap_hyperedge_members(hes, {"a": "z"}) + assert hes == [{"id": "h", "members": ["a", "b", "c"]}] + + def test_chained_collapse_lands_on_the_final_survivor(): """A dedup remap built from union-find is fully flattened (path-compressed), so a member of a chained component (a_old -> a_mid -> a) rewires directly to the final survivor in a single lookup, never to an intermediate.""" - hes = [{"id": "h", "nodes": ["a_old", "a_mid", "b"]}] + hes = [{"id": "h", "nodes": ["a_old", "a_mid", "b", "c"]}] # what components()/UnionFind produces: every non-winner maps to the winner _remap_hyperedge_members(hes, {"a_old": "a", "a_mid": "a"}) - assert hes[0]["nodes"] == ["a", "b"] + assert hes[0]["nodes"] == ["a", "b", "c"] -def test_a_hyperedge_collapsing_to_one_member_is_kept(): - """Sub-two-member hyperedges are kept by design (build_from_json only drops - the zero-valid-member case). Pin it so a future refactor doesn't silently - start dropping a 1-member group after a collapse.""" +def test_a_hyperedge_collapsing_to_one_member_is_dropped(): + """A deduplicated singleton is no longer a group relationship.""" hes = [{"id": "h", "nodes": ["a_old", "a_new"]}] _remap_hyperedge_members(hes, {"a_old": "a", "a_new": "a"}) - assert hes[0]["nodes"] == ["a"] # collapsed to one, still present + assert hes == [] + + +# --------------------------------------------------------------------------- +# deduplicate_entities(..., hyperedges=) must clean up on EVERY return path +# --------------------------------------------------------------------------- + +_DISTINCT_NODES = [ + _node("alpha_concept_long_variant_id", "alpha concept"), + _node("beta_node", "Beta"), + _node("gamma_node", "Gamma"), +] + + +def test_a_pair_is_dropped_even_when_dedup_merges_nothing(): + """The cardinality cleanup must not depend on whether a merge happened: with + an empty remap the early return used to skip _remap_hyperedge_members, so a + direct caller kept a two-member "group" that every other path rejects.""" + hes = [ + {"id": "pair", "nodes": ["alpha_concept_long_variant_id", "beta_node"]}, + {"id": "trio", "nodes": ["alpha_concept_long_variant_id", "beta_node", "gamma_node"]}, + ] + deduplicate_entities(list(_DISTINCT_NODES), [], communities={}, hyperedges=hes) + assert [h["id"] for h in hes] == ["trio"] + + +def test_a_pair_is_dropped_on_the_single_node_short_circuit(): + """Same contract on the other early return (nothing to dedup with one node).""" + hes = [{"id": "pair", "nodes": ["beta_node", "gamma_node"]}] + deduplicate_entities([_node("beta_node", "Beta")], [], communities={}, hyperedges=hes) + assert hes == [] + + +def test_a_pair_is_dropped_after_duplicate_id_collapse(): + """The third early return: two records sharing one id collapse to a single + `unique_nodes` entry, so the function short-circuits AFTER the initial + length check and used to skip the hyperedge pass on the way out.""" + hes = [{"id": "pair", "nodes": ["only_node", "beta_node"]}] + deduplicate_entities( + [_node("only_node", "Only"), _node("only_node", "Only Again")], + [], communities={}, hyperedges=hes, + ) + assert hes == [] + +def test_direct_dedup_keeps_members_in_the_returned_nodes_id_space(): + """A direct `deduplicate_entities(..., hyperedges=)` caller with numeric node + ids and nothing to merge got its group mutated into dangling references: + the remap pass coerces every member to `"7"` so it can be looked up, while + the returned node records still carry `7`. Whatever comes back has to name + the nodes that come back with it.""" + from graphify.dedup import deduplicate_entities + + nodes = [{"id": 7, "label": "A"}, {"id": 8, "label": "B"}, {"id": 9, "label": "C"}] + hyperedges = [{"id": "g", "nodes": [7, 8, 9]}] + out_nodes, _ = deduplicate_entities( + nodes, [], communities=None, hyperedges=hyperedges + ) + + returned_ids = {n["id"] for n in out_nodes} + assert set(hyperedges[0]["nodes"]) <= returned_ids, ( + f"members {hyperedges[0]['nodes']} must name the returned nodes " + f"{sorted(returned_ids, key=str)}" + ) + + +def test_direct_dedup_does_not_rebind_a_member_to_a_colliding_node(): + """With two distinct nodes `7` and `"7"`, a member `7` coerces to `"7"` for + lookup and the restore step then picked the exact string node — silently + rebinding the member from the node it named to a different one. Worse than a + dangling member, and a subset assertion cannot see it because both ids are + present, so this asserts the member still names the node it started on.""" + from graphify.dedup import deduplicate_entities + + nodes = [ + {"id": 7, "label": "int seven"}, + {"id": "7", "label": "str seven"}, + {"id": "b", "label": "B"}, + {"id": "c", "label": "C"}, + ] + hyperedges = [{"id": "g", "nodes": [7, "b", "c"]}] + out_nodes, _ = deduplicate_entities( + nodes, [], communities=None, hyperedges=hyperedges + ) + + by_id = {n["id"]: n["label"] for n in out_nodes} + member = hyperedges[0]["nodes"][0] + assert member == 7 and by_id.get(member) == "int seven", ( + f"member {member!r} must still name the node it named (int seven), " + f"not the colliding {by_id.get(member)!r}" + ) + diff --git a/tests/test_export.py b/tests/test_export.py index d957b87957..48258244b6 100644 --- a/tests/test_export.py +++ b/tests/test_export.py @@ -42,6 +42,7 @@ def test_to_json_nodes_have_community(): def test_to_json_sorts_graph_collections_across_insertion_order(tmp_path): + """Insertion order must not change the bytes: nodes, links and hyperedges are all sorted.""" import networkx as nx nodes = [("b", {"label": "Beta"}), ("a", {"label": "Alpha"}), ("c", {"label": "Gamma"})] @@ -49,9 +50,11 @@ def test_to_json_sorts_graph_collections_across_insertion_order(tmp_path): ("b", "c", {"relation": "uses", "_src": "b", "_tgt": "c"}), ("a", "b", {"relation": "calls", "_src": "a", "_tgt": "b"}), ] + # Three members each: to_json enforces the minimum cardinality, so a pair + # would be dropped and this test would stop exercising hyperedge sorting. hyperedges = [ - {"id": "h2", "nodes": ["b", "c"]}, - {"id": "h1", "nodes": ["a", "b"]}, + {"id": "h2", "nodes": ["b", "c", "a"]}, + {"id": "h1", "nodes": ["a", "b", "c"]}, ] def make_graph(reverse=False): diff --git a/tests/test_extract_cli.py b/tests/test_extract_cli.py index 4c9fb445f8..0675f7b7a6 100644 --- a/tests/test_extract_cli.py +++ b/tests/test_extract_cli.py @@ -330,6 +330,10 @@ def _extract(paths, **kwargs): monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) def _run(): + """Run the CLI once, tolerating a clean SystemExit. + + Returns nothing — the assertions read the files the run wrote. + """ monkeypatch.setattr(mainmod.sys, "argv", ["graphify", "extract", str(corpus), "--backend", "claude", "--no-cluster", "--out", str(out_dir)]) @@ -515,6 +519,132 @@ def _hyperedge_only(paths, **kwargs): ) +def test_under_cardinality_hyperedge_only_doc_is_not_stamped(monkeypatch, tmp_path): + """#1920 stamps a doc whose only output is a hyperedge — but only if that + hyperedge is a real group. An under-cardinality one is dropped before it + reaches graph.json, so stamping the doc would mark it successfully extracted + while contributing nothing, and only the #2927 graph heal could later notice + and re-queue it. Don't stamp it in the first place.""" + import json + + corpus = _make_corpus(tmp_path) # main.go + README.md + out_dir = tmp_path / "out" + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-test-fake-key") + + def _pair_only(paths, **kwargs): + """Return a single two-member group for README.md and nothing else.""" + on_chunk = kwargs.get("on_chunk_done") + if on_chunk: + on_chunk(0, 1, {"nodes": [], "edges": [], "hyperedges": []}) + return { + "nodes": [], + "edges": [], + "hyperedges": [{"id": "pair", "label": "Pair", "nodes": ["a", "b"], + "relation": "participate_in", "source_file": "README.md"}], + "input_tokens": 10, + "output_tokens": 5, + } + + monkeypatch.setattr("graphify.llm.extract_corpus_parallel", _pair_only) + monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) + monkeypatch.setattr( + mainmod.sys, "argv", + ["graphify", "extract", str(corpus), "--backend", "claude", + "--no-cluster", "--out", str(out_dir)], + ) + try: + mainmod.main() + except SystemExit as exc: + assert exc.code in (None, 0), f"unexpected exit code {exc.code}" + + manifest = json.loads((out_dir / "graphify-out" / "manifest.json").read_text()) + assert not manifest.get("README.md", {}).get("semantic_hash"), ( + "a doc whose only output was dropped as under-cardinality must stay " + "unstamped so the next run re-dispatches it" + ) + + +def test_dangling_member_hyperedge_only_doc_stamps_then_re_queues_from_cache( + monkeypatch, tmp_path, capsys +): + """Pins the accepted recovery path for a group that passes shape and + cardinality but whose members resolve to nothing. + + Membership cannot be checked before stamping — build_from_json resolves + members afterwards through _semantic_id_remap and norm_to_id, so gating on + raw ids would re-dispatch docs whose groups actually survive. The doc is + therefore stamped on the strength of a group the graph later drops. + + What follows is worth knowing, and is NOT the clean one-run recovery it + looks like: the #2927 heal does re-queue the doc on the next run, but the + group was already cached (the cache has no node set to reject it with), so + the re-queue is satisfied from cache, the group is dropped again and the doc + is re-stamped. The heal therefore re-fires every run without resolving. + Costs no LLM call and puts no bad data in graph.json — but it does not + self-heal either. Breaking the loop needs a design change, not a gate. + """ + import json + + corpus = _make_corpus(tmp_path) # main.go + README.md + out_dir = tmp_path / "out" + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-test-fake-key") + calls: list = [] + + def _dangling_group(paths, **kwargs): + """Return one three-member group whose members exist nowhere.""" + calls.append(1) + on_chunk = kwargs.get("on_chunk_done") + if on_chunk: + on_chunk(0, 1, {"nodes": [], "edges": [], "hyperedges": []}) + return { + "nodes": [], + "edges": [], + "hyperedges": [{"id": "ghost", "label": "G", + "nodes": ["nope_a", "nope_b", "nope_c"], + "relation": "participate_in", "source_file": "README.md"}], + "input_tokens": 1, + "output_tokens": 1, + } + + monkeypatch.setattr("graphify.llm.extract_corpus_parallel", _dangling_group) + monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) + monkeypatch.setattr( + mainmod.sys, "argv", + ["graphify", "extract", str(corpus), "--backend", "claude", + "--no-cluster", "--out", str(out_dir)], + ) + + def _run(): + """Run the CLI once, tolerating a clean SystemExit, and return its output.""" + try: + mainmod.main() + except SystemExit as exc: + assert exc.code in (None, 0), f"unexpected exit code {exc.code}" + return capsys.readouterr() + + graphify_out = out_dir / "graphify-out" + _run() + manifest = json.loads((graphify_out / "manifest.json").read_text()) + graph = json.loads((graphify_out / "graph.json").read_text()) + assert manifest.get("README.md", {}).get("semantic_hash"), ( + "the group passes shape + cardinality, so #1920 stamps the doc" + ) + assert graph["hyperedges"] == [], "the graph gate drops it: no member resolves" + assert calls == [1], "one extraction so far" + + out2 = _run() + assert "#2927" in out2.out, "the heal must notice the stamped-but-empty source" + assert calls == [1], ( + "the re-queue is served from cache, not re-extracted — no LLM call" + ) + graph2 = json.loads((graphify_out / "graph.json").read_text()) + manifest2 = json.loads((graphify_out / "manifest.json").read_text()) + assert graph2["hyperedges"] == [], "still dropped on replay" + assert manifest2.get("README.md", {}).get("semantic_hash"), ( + "and re-stamped, so the heal re-fires next run without resolving" + ) + + # --- #1894: --force and deep-mode dispatch over a warm cache ----------------- def _recording_extractor(calls): @@ -741,6 +871,7 @@ def test_missing_manifest_code_only_preserves_semantic_layer(monkeypatch, tmp_pa monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) def _sem_doc_count(g): + """Count the graph's nodes attributed to README.md.""" return sum(1 for n in g["nodes"] if n.get("source_file") == "README.md") # 1) seed a code-only graph @@ -754,11 +885,14 @@ def _sem_doc_count(g): "source_file": "README.md", "file_type": "document"}) graph["nodes"].append({"id": "doc_readme_b", "label": "Concept B", "source_file": "README.md", "file_type": "document"}) + graph["nodes"].append({"id": "doc_readme_c", "label": "Concept C", + "source_file": "README.md", "file_type": "document"}) graph.setdefault("edges", []).append( {"source": "doc_readme_a", "target": "doc_readme_b", "relation": "relates_to", "source_file": "README.md"}) graph.setdefault("hyperedges", []).append( - {"id": "h1", "label": "Shared", "nodes": ["doc_readme_a", "doc_readme_b"], + {"id": "h1", "label": "Shared", + "nodes": ["doc_readme_a", "doc_readme_b", "doc_readme_c"], "relation": "participate_in", "source_file": "README.md"}) graph_path.write_text(json.dumps(graph)) (graphify_out / ".graphify_semantic_marker").write_text( @@ -771,12 +905,20 @@ def _sem_doc_count(g): _run_extract(monkeypatch, ["graphify", "extract", str(corpus), "--code-only", "--out", str(out_dir)]) after = json.loads(graph_path.read_text()) - assert _sem_doc_count(after) >= 2, ( + # All THREE seeded nodes must survive, not merely two: the committed + # hyperedge below names every one of them, so losing `doc_readme_c` would + # take h1 under the minimum cardinality as well. + _survivors = {n["id"] for n in after["nodes"] if n.get("source_file") == "README.md"} + assert {"doc_readme_a", "doc_readme_b", "doc_readme_c"} <= _survivors, ( "committed semantic doc nodes must survive a missing-manifest " - f"--code-only rebuild (#1925); got {_sem_doc_count(after)}" + f"--code-only rebuild (#1925); got {sorted(_survivors)}" ) - assert any(h.get("id") == "h1" for h in after.get("hyperedges", [])), ( - "committed hyperedge must survive the rebuild" + _h1 = next((h for h in after.get("hyperedges", []) if h.get("id") == "h1"), None) + assert _h1 is not None, "committed hyperedge must survive the rebuild" + # Assert the membership directly rather than inferring it from the group + # having survived at all: that inference only holds while the minimum is 3. + assert set(_h1["nodes"]) == {"doc_readme_a", "doc_readme_b", "doc_readme_c"}, ( + f"h1 must keep every committed member; got {_h1['nodes']}" ) assert any("keep" in n["id"] for n in after["nodes"]), "code nodes intact" @@ -1124,6 +1266,354 @@ def test_incremental_extract_prunes_excluded_file_listed_in_manifest( assert any("keep.py" in s for s in sources) +def _read_graph(graph_path): + """Parse a written graph.json.""" + import json + return json.loads(graph_path.read_text(encoding="utf-8")) + + +def _he_by_id(graph_path): + """Map hyperedge id -> entry from a written graph.json.""" + return {h["id"]: h for h in _read_graph(graph_path).get("hyperedges", [])} + + +def test_no_cluster_gates_hyperedge_cardinality_in_the_raw_graph(monkeypatch, tmp_path): + """--no-cluster never reaches build_from_json / build_merge / to_json, whose + gates canonicalize members and enforce the minimum, so the raw path must + apply the same gate before writing graph.json (#3203 follow-up).""" + corpus = _make_corpus(tmp_path) # main.go + README.md + out_dir = tmp_path / "out" + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-test-fake-key") + + def _fake_extract(paths, **kwargs): + """Return three README-owned nodes and four hyperedges of varying validity.""" + on_chunk = kwargs.get("on_chunk_done") + if on_chunk: # else the zero-succeeded gate exits 1 + on_chunk(0, 1, {"nodes": [], "edges": [], "hyperedges": []}) + return { + "nodes": [ + {"id": nid, "label": nid, "file_type": "document", "source_file": "README.md"} + for nid in ("a", "b", "c") + ], + "edges": [], + "hyperedges": [ + {"id": "pair", "nodes": ["a", "b"], "source_file": "README.md"}, + {"id": "alias", "members": ["a", "b", "c"], "source_file": "README.md"}, + {"id": "dupes", "nodes": ["a", "a", "b"], "source_file": "README.md"}, + {"id": "dangling", "nodes": ["a", "b", "ghost"], "source_file": "README.md"}, + ], + "input_tokens": 10, + "output_tokens": 5, + } + + monkeypatch.setattr("graphify.llm.extract_corpus_parallel", _fake_extract) + monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) + monkeypatch.setattr( + mainmod.sys, "argv", + ["graphify", "extract", str(corpus), "--backend", "claude", + "--no-cluster", "--out", str(out_dir)], + ) + with pytest.raises(SystemExit) as exc: + mainmod.main() + assert exc.value.code in (None, 0) + + graph_path = out_dir / "graphify-out" / "graph.json" + hes = _he_by_id(graph_path) + assert set(hes) == {"alias"}, ( + "only the alias-keyed 3-member group is a real hyperedge: pair has 2, " + f"dupes has 2 distinct, dangling loses ghost — got {sorted(hes)}" + ) + assert hes["alias"]["nodes"] == ["a", "b", "c"] + assert "members" not in hes["alias"] + + +def test_no_cluster_incremental_drops_a_hyperedge_left_dangling_by_a_prune( + monkeypatch, tmp_path +): + """A deleted file's node is pruned by merge_raw_extraction, which carries + hyperedges verbatim — so a cross-file group owned by a surviving file keeps a + dangling member and can fall under the minimum. The raw gate must catch it.""" + project = _two_file_corpus(tmp_path) # x.py + keep.py + (project / "README.md").write_text("# Notes\nDescribes the helpers.\n") + out_dir = tmp_path / "out" + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-test-fake-key") + + def _fake_extract(paths, **kwargs): + """Return one README-owned group spanning both Python files' AST nodes.""" + on_chunk = kwargs.get("on_chunk_done") + if on_chunk: + on_chunk(0, 1, {"nodes": [], "edges": [], "hyperedges": []}) + return { + "nodes": [], + "edges": [], + # Members are the real AST ids for the two Python files. + "hyperedges": [{ + "id": "cross", "label": "Helpers", + "nodes": ["x_secret_helper", "keep_kept", "keep_still_here"], + "source_file": "README.md", + }], + "input_tokens": 10, + "output_tokens": 5, + } + + monkeypatch.setattr("graphify.llm.extract_corpus_parallel", _fake_extract) + monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) + monkeypatch.setattr( + mainmod.sys, "argv", + ["graphify", "extract", str(project), "--backend", "claude", + "--no-cluster", "--out", str(out_dir)], + ) + with pytest.raises(SystemExit) as exc: + mainmod.main() + assert exc.value.code in (None, 0) + + graph_path = out_dir / "graphify-out" / "graph.json" + hes = _he_by_id(graph_path) + assert "cross" in hes, f"precondition: the 3-member group must survive run 1, got {sorted(hes)}" + assert len(hes["cross"]["nodes"]) == 3 + + # Delete x.py: its AST node is pruned, leaving `cross` with 2 live members. + (project / "x.py").unlink() + with pytest.raises(SystemExit) as exc: + mainmod.main() + assert exc.value.code in (None, 0) + + data = _read_graph(graph_path) + assert "cross" not in {h["id"] for h in data.get("hyperedges", [])}, ( + "a group left with 2 live members after the prune must not be persisted" + ) + assert not any( + (n.get("source_file") or "").endswith("x.py") for n in data["nodes"] + ), "the deleted file's nodes must be pruned too" + + +def test_prune_graph_json_sources_revalidates_hyperedges(tmp_path): + """The exclusion-only early exit prunes graph.json in place and never runs + build_merge, so it needs the same hyperedge revalidation: a member whose node + just went away must go, and the group must die if that leaves it a pair.""" + import json + + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps({ + "nodes": [ + {"id": "gone", "source_file": "stale.py"}, + {"id": "a", "source_file": "live.py"}, + {"id": "b", "source_file": "live.py"}, + {"id": "c", "source_file": "live.py"}, + ], + "edges": [], + "hyperedges": [ + # owned by a LIVE file, so the source_file filter keeps it, but it + # names the removed node and is left with 2 members + {"id": "degraded", "nodes": ["gone", "a", "b"], "source_file": "live.py"}, + # still has 3 live members after the prune + {"id": "survivor", "nodes": ["gone", "a", "b", "c"], "source_file": "live.py"}, + ], + }), encoding="utf-8") + + assert _prune_graph_json_sources(graph_path, ["stale.py"]) == 1 + hes = {h["id"]: h for h in _read_graph(graph_path)["hyperedges"]} + assert set(hes) == {"survivor"}, f"a degraded pair must not survive, got {sorted(hes)}" + assert hes["survivor"]["nodes"] == ["a", "b", "c"] + + +def test_prune_graph_json_sources_syncs_the_nested_hyperedge_slot(tmp_path): + """to_json persists hyperedges in BOTH slots (#2484). Dropping one from the + top-level slot only would leave a stale nested copy, and + _zero_node_stamped_semantic_sources unions both when deciding whether a doc's + manifest stamp is honest (#2927) — so the dropped group would still look + present there and the doc would never be re-dispatched.""" + import json + + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_path / "graph.json" + hyperedges = [{"id": "degraded", "nodes": ["gone", "a", "b"], "source_file": "notes.md"}] + graph_path.write_text(json.dumps({ + "nodes": [ + {"id": "gone", "source_file": "stale.py"}, + {"id": "a", "source_file": "live.py"}, + {"id": "b", "source_file": "live.py"}, + ], + "edges": [], + "graph": {"hyperedges": hyperedges}, + "hyperedges": hyperedges, + }), encoding="utf-8") + + _prune_graph_json_sources(graph_path, ["stale.py"]) + + data = _read_graph(graph_path) + assert data["hyperedges"] == [] + assert data["graph"]["hyperedges"] == [], ( + "the nested slot must not keep a hyperedge the top-level slot just lost" + ) + + +def test_prune_graph_json_sources_revalidates_a_nested_only_slot(tmp_path): + """A node_link_data-only writer emits hyperedges solely under `graph`, with + no top-level key (#2485 — build_from_json folds nested onto top-level for + exactly that shape). Reading only the top-level slot would see nothing to + revalidate and then overwrite the nested slot with that empty result, + destroying a perfectly valid group.""" + import json + + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps({ + "nodes": [{"id": n, "source_file": "live.py"} for n in ("a", "b", "c")] + + [{"id": "gone", "source_file": "stale.py"}], + "links": [], + "graph": {"hyperedges": [ + {"id": "keeper", "nodes": ["a", "b", "c"], "source_file": "live.py"}, + {"id": "degraded", "nodes": ["gone", "a", "b"], "source_file": "live.py"}, + ]}, + }), encoding="utf-8") + + _prune_graph_json_sources(graph_path, ["stale.py"]) + + data = _read_graph(graph_path) + nested = {h["id"]: h for h in data["graph"]["hyperedges"]} + assert set(nested) == {"keeper"}, ( + f"the valid group must survive and only the degraded one go, got {sorted(nested)}" + ) + assert nested["keeper"]["nodes"] == ["a", "b", "c"] + assert "hyperedges" not in data, "a nested-only file must not sprout a top-level slot" + + +def test_prune_graph_json_sources_tolerates_an_unhashable_node_id(tmp_path): + """A persisted node can carry a malformed list/dict id — the build path + deliberately leaves those for the validator to report rather than dropping + them. Collecting surviving ids into a set must skip them, or the whole + exclusion-only prune aborts with TypeError on a legacy graph.json.""" + import json + + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps({ + "nodes": [ + {"id": ["malformed", "list"], "source_file": "live.py"}, + {"id": {"also": "malformed"}, "source_file": "live.py"}, + {"id": "a", "source_file": "live.py"}, + {"id": "b", "source_file": "live.py"}, + {"id": "c", "source_file": "live.py"}, + {"id": "gone", "source_file": "stale.py"}, + ], + "links": [], + "hyperedges": [{"id": "grp", "nodes": ["a", "b", "c"], "source_file": "live.py"}], + }), encoding="utf-8") + + assert _prune_graph_json_sources(graph_path, ["stale.py"]) == 1 + data = _read_graph(graph_path) + assert [h["id"] for h in data["hyperedges"]] == ["grp"], ( + "an unrelated malformed node id must not cost a valid group" + ) + # The malformed nodes belong to a live file, so the prune leaves them alone. + assert len(data["nodes"]) == 5 + + +def test_prune_graph_json_sources_reconciles_a_stale_nested_slot(tmp_path): + """The pre-revalidation pruner filtered only the top-level slot, so an + upgraded graph.json can carry a stale nested copy of a group the top level + already lost. Preferring the top level whenever it is list-shaped would find + nothing to change, return early, and leave that copy in place — where + _zero_node_stamped_semantic_sources (#2927) still unions it and keeps the + source looking covered.""" + import json + + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps({ + "nodes": [{"id": "a", "source_file": "live.py"}], + "links": [], + # The skew: nested still holds what the top level already dropped. + "graph": {"hyperedges": [ + {"id": "ghost_group", "nodes": ["x", "y", "z"], "source_file": "stale.md"}, + ]}, + "hyperedges": [], + }), encoding="utf-8") + + _prune_graph_json_sources(graph_path, ["stale.md"]) + data = _read_graph(graph_path) + assert data["graph"]["hyperedges"] == [], ( + "the stale nested copy must be reconciled, not ignored because the " + "top-level slot already looks clean" + ) + assert data["hyperedges"] == [] + + +def test_prune_graph_json_sources_leaves_a_clean_graph_untouched(tmp_path): + """Nothing to prune must mean no rewrite at all (the caller reports 0).""" + import json + + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_path / "graph.json" + payload = json.dumps({ + "nodes": [{"id": n, "source_file": "live.py"} for n in ("a", "b", "c")], + "edges": [], + "hyperedges": [{"id": "ok", "nodes": ["a", "b", "c"], "source_file": "live.py"}], + }) + graph_path.write_text(payload, encoding="utf-8") + + assert _prune_graph_json_sources(graph_path, ["stale.py"]) == 0 + assert graph_path.read_text(encoding="utf-8") == payload, "must not rewrite" + + +def test_prune_graph_json_sources_tolerates_a_null_hyperedges_slot(tmp_path): + """A legacy `"hyperedges": null` makes .get return None, so a bare `for h in` + would raise TypeError straight out of the function (its try wraps only the + JSON load).""" + import json + + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps({ + # A node that really is pruned, so the null slot is carried through the + # rewrite (and the nested-slot sync) rather than short-circuiting at the + # nothing-changed check. + "nodes": [{"id": "a", "source_file": "live.py"}, + {"id": "gone", "source_file": "stale.py"}], + "edges": [], + "hyperedges": None, + }), encoding="utf-8") + + assert _prune_graph_json_sources(graph_path, ["stale.py"]) == 1 + data = _read_graph(graph_path) + assert [n["id"] for n in data["nodes"]] == ["a"] + assert data["hyperedges"] == [], "the null slot is healed to an empty list, not left null" + + +def test_prune_graph_json_sources_does_not_count_an_id_less_node_as_a_member(tmp_path): + """This path keeps id-less nodes, so an unfiltered surviving-id set contains + None and `[null, "a", "b"]` would pass as a 3-member group.""" + import json + + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps({ + "nodes": [ + {"source_file": "live.py"}, # no id at all + {"id": "a", "source_file": "live.py"}, + {"id": "b", "source_file": "live.py"}, + {"id": "gone", "source_file": "stale.py"}, + ], + "edges": [], + "hyperedges": [{"id": "nully", "nodes": [None, "a", "b"], "source_file": "live.py"}], + }), encoding="utf-8") + + _prune_graph_json_sources(graph_path, ["stale.py"]) + assert _read_graph(graph_path)["hyperedges"] == [], ( + "a null member is not a node: this is a pair, not a group" + ) + + def test_no_cluster_incremental_prunes_newly_excluded_file( monkeypatch, tmp_path, capsys ): diff --git a/tests/test_hyperedge_member_shapes.py b/tests/test_hyperedge_member_shapes.py index 678bfa5d01..76571a321f 100644 --- a/tests/test_hyperedge_member_shapes.py +++ b/tests/test_hyperedge_member_shapes.py @@ -19,44 +19,53 @@ def _node(nid: str) -> dict: def test_dict_members_coerced_via_canonical_nodes_key(capsys): + """#2486: object members on the canonical key are coerced to ids and deduped.""" extraction = { - "nodes": [_node("a_ts"), _node("b_ts")], + "nodes": [_node("a_ts"), _node("b_ts"), _node("c_ts")], "edges": [], "hyperedges": [ # the #2486 repro shape: object members mixed with bare ids, # including a duplicate that must dedupe after coercion - {"id": "h_flow", "nodes": [{"id": "a_ts"}, "b_ts", {"id": "a_ts"}]}, + { + "id": "h_flow", + "nodes": [{"id": "a_ts"}, "b_ts", {"id": "a_ts"}, "c_ts"], + }, ], } G = build_from_json(extraction, directed=True) # must not raise - assert set(G.nodes()) == {"a_ts", "b_ts"} - assert G.graph["hyperedges"][0]["nodes"] == ["a_ts", "b_ts"] + assert set(G.nodes()) == {"a_ts", "b_ts", "c_ts"} + assert G.graph["hyperedges"][0]["nodes"] == ["a_ts", "b_ts", "c_ts"] def test_dict_members_coerced_via_members_alias(capsys): + """The same coercion applies when members arrive under the `members` alias.""" extraction = { - "nodes": [_node("a_ts"), _node("c_ts")], + "nodes": [_node("a_ts"), _node("b_ts"), _node("c_ts")], "edges": [], "hyperedges": [ - {"id": "h_alias", "members": ["a_ts", {"id": "c_ts"}]}, + {"id": "h_alias", "members": ["a_ts", "b_ts", {"id": "c_ts"}]}, ], } G = build_from_json(extraction, directed=True) (he,) = G.graph["hyperedges"] assert "members" not in he, "alias key must be folded onto nodes" - assert he["nodes"] == ["a_ts", "c_ts"] + assert he["nodes"] == ["a_ts", "b_ts", "c_ts"] def test_member_object_without_id_dropped_with_one_warning(capsys): + """A member object carrying no usable id is dropped, warning exactly once.""" extraction = { - "nodes": [_node("a_ts"), _node("b_ts")], + "nodes": [_node("a_ts"), _node("b_ts"), _node("c_ts"), _node("d_ts")], "edges": [], "hyperedges": [ - {"id": "h_partial", "nodes": [{"label": "no id here"}, "b_ts"]}, + { + "id": "h_partial", + "nodes": [{"label": "no id here"}, "b_ts", "c_ts", "d_ts"], + }, ], } G = build_from_json(extraction, directed=True) - assert G.graph["hyperedges"][0]["nodes"] == ["b_ts"] + assert G.graph["hyperedges"][0]["nodes"] == ["b_ts", "c_ts", "d_ts"] err = capsys.readouterr().err warnings = [ line for line in err.splitlines() @@ -66,14 +75,16 @@ def test_member_object_without_id_dropped_with_one_warning(capsys): def test_hyperedge_losing_all_members_is_dropped_not_fatal(capsys): + """Losing every member drops the hyperedge instead of aborting the build.""" extraction = { "nodes": [_node("a_ts")], "edges": [], "hyperedges": [ {"id": "h_empty", "nodes": [{"label": "no id"}, {"nested": True}]}, - {"id": "h_ok", "nodes": ["a_ts"]}, + {"id": "h_ok", "nodes": ["a_ts", "b_ts", "c_ts"]}, ], } + extraction["nodes"].extend([_node("b_ts"), _node("c_ts")]) G = build_from_json(extraction, directed=True) # must not raise assert [he["id"] for he in G.graph["hyperedges"]] == ["h_ok"] assert "h_empty" in capsys.readouterr().err diff --git a/tests/test_hyperedge_roundtrip.py b/tests/test_hyperedge_roundtrip.py index 6f0e9a8ef3..fd638bbb27 100644 --- a/tests/test_hyperedge_roundtrip.py +++ b/tests/test_hyperedge_roundtrip.py @@ -27,42 +27,44 @@ def _roundtrip(G, tmp_path): def test_nested_only_slot_is_read_and_reexported_to_both_slots(tmp_path): + """A nested-only input is read and re-exported to both persistence slots (#2485).""" # node_link_data-only writers emit hyperedges solely under graph attrs. extraction = { "directed": True, "multigraph": False, - "graph": {"hyperedges": [{"id": "h1", "nodes": ["a", "b"]}]}, - "nodes": [_node("a"), _node("b")], + "graph": {"hyperedges": [{"id": "h1", "nodes": ["a", "b", "c"]}]}, + "nodes": [_node("a"), _node("b"), _node("c")], "links": [], } G = build_from_json(extraction, directed=True) - assert G.graph["hyperedges"] == [{"id": "h1", "nodes": ["a", "b"]}] + assert G.graph["hyperedges"] == [{"id": "h1", "nodes": ["a", "b", "c"]}] data = _roundtrip(G, tmp_path) - assert data["hyperedges"] == [{"id": "h1", "nodes": ["a", "b"]}] + assert data["hyperedges"] == [{"id": "h1", "nodes": ["a", "b", "c"]}] assert data["graph"]["hyperedges"] == data["hyperedges"], ( "re-export must carry the set in BOTH slots" ) # Full round-trip: rebuilding from the exported file preserves the set exactly. G2 = build_from_json(json.loads(json.dumps(data)), directed=True) - assert G2.graph["hyperedges"] == [{"id": "h1", "nodes": ["a", "b"]}] + assert G2.graph["hyperedges"] == [{"id": "h1", "nodes": ["a", "b", "c"]}] def test_top_level_slot_roundtrips_unchanged(tmp_path): + """Control arm: the canonical to_json shape still round-trips.""" # Control arm: the canonical to_json shape keeps working as before. extraction = { - "nodes": [_node("a"), _node("b")], + "nodes": [_node("a"), _node("b"), _node("c")], "edges": [], - "hyperedges": [{"id": "h_top", "nodes": ["a", "b"]}], + "hyperedges": [{"id": "h_top", "nodes": ["a", "b", "c"]}], } G = build_from_json(extraction, directed=True) - assert G.graph["hyperedges"] == [{"id": "h_top", "nodes": ["a", "b"]}] + assert G.graph["hyperedges"] == [{"id": "h_top", "nodes": ["a", "b", "c"]}] data = _roundtrip(G, tmp_path) - assert data["hyperedges"] == [{"id": "h_top", "nodes": ["a", "b"]}] + assert data["hyperedges"] == [{"id": "h_top", "nodes": ["a", "b", "c"]}] assert data["graph"]["hyperedges"] == data["hyperedges"] G2 = build_from_json(json.loads(json.dumps(data)), directed=True) - assert G2.graph["hyperedges"] == [{"id": "h_top", "nodes": ["a", "b"]}] + assert G2.graph["hyperedges"] == [{"id": "h_top", "nodes": ["a", "b", "c"]}] def test_full_wipeout_emits_one_aggregate_warning(tmp_path, capsys): diff --git a/tests/test_hypergraph.py b/tests/test_hypergraph.py index a5208095a7..2b2252ef87 100644 --- a/tests/test_hypergraph.py +++ b/tests/test_hypergraph.py @@ -7,7 +7,14 @@ import networkx as nx import pytest -from graphify.build import build_from_json +from graphify.build import ( + MIN_HYPEREDGE_MEMBERS, + build_from_json, + canonical_hyperedge, + gate_hyperedges, + gate_hyperedges_against_graph, + node_id_set, +) from graphify.export import attach_hyperedges, to_json from graphify.report import generate @@ -72,13 +79,15 @@ def test_build_from_json_relativizes_hyperedge_source_file(tmp_path): extraction = { "nodes": [ {"id": "a", "label": "A", "file_type": "document", "source_file": str(abs_doc)}, + {"id": "b", "label": "B", "file_type": "document", "source_file": str(abs_doc)}, + {"id": "c", "label": "C", "file_type": "document", "source_file": str(abs_doc)}, ], "edges": [], "hyperedges": [ { "id": "arch", "label": "Architecture", - "nodes": ["a"], + "nodes": ["a", "b", "c"], "relation": "participate_in", "confidence": "INFERRED", "confidence_score": 0.75, @@ -109,13 +118,17 @@ def test_build_from_json_missing_hyperedges_key(): # --------------------------------------------------------------------------- def test_attach_hyperedges_adds_new(): + """A fresh hyperedge is stored in the graph's metadata.""" G = nx.Graph() + G.add_nodes_from(["A", "B", "C"]) attach_hyperedges(G, [{"id": "auth_flow", "label": "Auth Flow", "nodes": ["A", "B", "C"]}]) assert len(G.graph["hyperedges"]) == 1 def test_attach_hyperedges_deduplicates(): + """Attaching the same id twice must not duplicate the entry.""" G = nx.Graph() + G.add_nodes_from(["A", "B", "C"]) h = {"id": "auth_flow", "label": "Auth Flow", "nodes": ["A", "B", "C"]} attach_hyperedges(G, [h]) attach_hyperedges(G, [h]) # second call with same id should not duplicate @@ -123,7 +136,9 @@ def test_attach_hyperedges_deduplicates(): def test_attach_hyperedges_multiple_different_ids(): + """Distinct ids all land in the metadata list.""" G = nx.Graph() + G.add_nodes_from(["A", "B", "C", "D", "E", "F"]) attach_hyperedges(G, [ {"id": "flow_a", "label": "Flow A", "nodes": ["A", "B", "C"]}, {"id": "flow_b", "label": "Flow B", "nodes": ["D", "E", "F"]}, @@ -132,20 +147,26 @@ def test_attach_hyperedges_multiple_different_ids(): def test_attach_hyperedges_skips_entry_without_id(): + """An id-less incoming entry is not attached.""" G = nx.Graph() + G.add_nodes_from(["A", "B", "C"]) attach_hyperedges(G, [{"label": "No ID", "nodes": ["A", "B", "C"]}]) assert G.graph.get("hyperedges", []) == [] def test_attach_hyperedges_tolerates_id_less_persisted(): + """#2775: an id-less entry already persisted must not raise KeyError.""" # Regression for #2775: the semantic extractor emits hyperedges with no `id` # and build.py persists them verbatim, so a prior graph.json can carry id-less # hyperedges. On the next (incremental) run, attach_hyperedges read that # persisted set with a hard `h["id"]` and died with `KeyError: 'id'`, writing # nothing. Reading the persisted set must tolerate missing ids. G = nx.DiGraph() - G.graph["hyperedges"] = [{"nodes": ["a", "b"], "type": "project", "attributes": {}}] - attach_hyperedges(G, [{"id": "flow_a", "label": "Flow A", "nodes": ["A", "B"]}]) + G.add_nodes_from(["a", "b", "c", "A", "B", "C"]) + G.graph["hyperedges"] = [ + {"nodes": ["a", "b", "c"], "type": "project", "attributes": {}} + ] + attach_hyperedges(G, [{"id": "flow_a", "label": "Flow A", "nodes": ["A", "B", "C"]}]) # No crash; the id-less persisted entry is retained and the new id-bearing # incoming hyperedge is appended. assert len(G.graph["hyperedges"]) == 2 @@ -155,10 +176,11 @@ def test_attach_hyperedges_tolerates_many_id_less_persisted(): """The real corpus had 183/234 persisted hyperedges id-less: all of them must load without crashing and be retained (#2775).""" G = nx.DiGraph() + G.add_nodes_from(["a", "b", "c", "A", "B", "C"]) G.graph["hyperedges"] = [ - {"nodes": ["a", "b"], "type": "project", "attributes": {}} for _ in range(5) + {"nodes": ["a", "b", "c"], "type": "project", "attributes": {}} for _ in range(5) ] - attach_hyperedges(G, [{"id": "flow_a", "nodes": ["A", "B"]}]) + attach_hyperedges(G, [{"id": "flow_a", "nodes": ["A", "B", "C"]}]) assert len(G.graph["hyperedges"]) == 6 # 5 id-less retained + 1 appended @@ -166,16 +188,317 @@ def test_attach_hyperedges_treats_empty_id_as_id_less(): """An empty-string id is falsy, so it is treated the same as a missing id: it seeds nothing into the dedup set and does not crash.""" G = nx.DiGraph() - G.graph["hyperedges"] = [{"id": "", "nodes": ["a", "b"], "type": "project"}] - attach_hyperedges(G, [{"id": "flow_a", "nodes": ["A", "B"]}]) + G.add_nodes_from(["a", "b", "c", "A", "B", "C"]) + G.graph["hyperedges"] = [{"id": "", "nodes": ["a", "b", "c"], "type": "project"}] + attach_hyperedges(G, [{"id": "flow_a", "nodes": ["A", "B", "C"]}]) assert len(G.graph["hyperedges"]) == 2 +def test_attach_hyperedges_drops_legacy_two_member_entries(): + """A two-member group persisted by an older version is pruned on attach.""" + G = nx.Graph() + G.add_nodes_from(["a", "b", "c"]) + G.graph["hyperedges"] = [{"id": "legacy_pair", "nodes": ["a", "b"]}] + + attach_hyperedges(G, [{"id": "valid_group", "nodes": ["a", "b", "c"]}]) + + assert [he["id"] for he in G.graph["hyperedges"]] == ["valid_group"] + + +def test_attach_hyperedges_canonicalizes_members_before_validating(): + """merge-graphs feeds persisted hyperedge metadata straight through this + boundary with no build_from_json in between (#1561 alias fold never ran), so + the member gate must canonicalize first: an alias-keyed group and one with + object-shaped members are both valid three-member hyperedges, not junk to + drop. The caller's dicts are left untouched.""" + G = nx.Graph() + G.add_nodes_from(["a", "b", "c"]) + alias_shaped = {"id": "alias_group", "members": ["a", "b", "c"]} + object_shaped = {"id": "object_group", "nodes": [{"id": "a"}, "b", "c"]} + + attach_hyperedges(G, [alias_shaped, object_shaped]) + + attached = {he["id"]: he for he in G.graph["hyperedges"]} + assert set(attached) == {"alias_group", "object_group"} + assert attached["alias_group"]["nodes"] == ["a", "b", "c"] + assert "members" not in attached["alias_group"] + assert attached["object_group"]["nodes"] == ["a", "b", "c"] + assert alias_shaped == {"id": "alias_group", "members": ["a", "b", "c"]} + + +# --------------------------------------------------------------------------- +# 2b. canonical_hyperedge — the one gate every persistence boundary shares +# --------------------------------------------------------------------------- + +def test_canonical_hyperedge_folds_alias_keys(): + """A `members`/`node_ids` group is valid (#1561); the gate must read it.""" + he = {"id": "h", "members": ["a", "b", "c"]} + out = canonical_hyperedge(he) + assert out["nodes"] == ["a", "b", "c"] + assert "members" not in out + assert he == {"id": "h", "members": ["a", "b", "c"]}, "caller's dict must be untouched" + + +def test_canonical_hyperedge_counts_distinct_members(): + """Positions are not members: a repeated id must not inflate the count.""" + assert canonical_hyperedge({"id": "h", "nodes": ["a", "a", "b", "c"]})["nodes"] == ["a", "b", "c"] + assert canonical_hyperedge({"id": "h", "nodes": ["a", "a", "b"]}) is None + + +def test_canonical_hyperedge_coerces_object_members(): + """Members are tolerated as bare ids or as objects carrying one.""" + out = canonical_hyperedge({"id": "h", "nodes": [{"id": "a"}, "b", {"id": "c"}]}) + assert out["nodes"] == ["a", "b", "c"] + + +@pytest.mark.parametrize("container", [ + {"a", "b", "c"}, + nx.Graph([("a", "b"), ("b", "c")]), +]) +def test_gate_filters_members_to_the_node_set(container): + """A member with no backing node is dropped; the group dies below the minimum. + + Membership lives in the gate rather than in canonical_hyperedge, because it + needs the id-space bridge — compare normalized, return the container's own + ids. Both a plain set and an nx.Graph are valid containers.""" + kept, _ = gate_hyperedges_against_graph( + [{"id": "h", "nodes": ["a", "b", "c", "ghost"]}], container + ) + assert kept[0]["nodes"] == ["a", "b", "c"] + assert gate_hyperedges_against_graph( + [{"id": "h", "nodes": ["a", "b", "ghost"]}], container + ) == ([], 1) + + +def test_canonical_hyperedge_alone_does_not_check_membership(): + """The cache and the pre-stamp gate have no node set, so they call + canonical_hyperedge directly and get shape and cardinality only.""" + out = canonical_hyperedge({"id": "h", "nodes": ["ghost1", "ghost2", "ghost3"]}) + assert out["nodes"] == ["ghost1", "ghost2", "ghost3"] + + +@pytest.mark.parametrize("empty", [set(), nx.Graph()]) +def test_gate_treats_an_empty_node_set_as_empty_not_absent(empty): + """`set()` and `nx.Graph()` are both FALSY, so a truthiness guard anywhere on + this path would skip membership entirely and keep a group whose every member + dangles. An empty container must drop everything.""" + assert gate_hyperedges_against_graph( + [{"id": "h", "nodes": ["a", "b", "c"]}], empty + ) == ([], 1) + + +@pytest.mark.parametrize("nodes_value", [ + None, # explicit null + "a,b,c", # a string: iterating it yields characters + {"a": 1, "b": 2, "c": 3}, # a dict: iterating it yields keys +]) +def test_canonical_hyperedge_rejects_a_non_list_nodes_value(nodes_value): + """Normalization only assigns `nodes` when `nodes` or an alias is already a + list, so a malformed value survives to the membership step. Without an + explicit list guard a string or dict is *fabricated* into a well-formed + 3-member group (its characters / keys pass membership), and an absent or + null value raises. Every shape must be rejected outright.""" + assert canonical_hyperedge({"id": "h", "nodes": nodes_value}) is None + assert gate_hyperedges_against_graph( + [{"id": "h", "nodes": nodes_value}], {"a", "b", "c"} + ) == ([], 1) + + +def test_canonical_hyperedge_rejects_a_member_less_entry(): + """No `nodes` key and no alias at all — same guard as the shapes above.""" + assert canonical_hyperedge({"id": "h", "label": "x"}) is None + assert gate_hyperedges_against_graph( + [{"id": "h", "label": "x"}], {"a", "b", "c"} + ) == ([], 1) + + +@pytest.mark.parametrize("he", ["not-a-dict", None, 7, ["a", "b", "c"]]) +def test_canonical_hyperedge_rejects_a_non_dict(he): + """Anything that is not a dict is not a hyperedge.""" + assert canonical_hyperedge(he) is None + + +@pytest.mark.parametrize("junk", [None, "", True, False]) +def test_canonical_hyperedge_rejects_an_unusable_member_id_in_object_form(junk): + """The bare and object member branches must apply the SAME rule. They drifted + twice: the object branch rejected `{"id": None}` while a bare `None` was kept, + and then the reverse once booleans were added to the bare branch only.""" + assert canonical_hyperedge({"id": "h", "nodes": ["a", "b", {"id": junk}]}) is None + + +@pytest.mark.parametrize("junk", [None, "", True, False]) +def test_canonical_hyperedge_does_not_count_an_unusable_member_id(junk): + """`None` and `""` can never name a node, so they must not pad the count. + + They are hashable, so they used to survive member coercion — unlike the + equivalent object member `{"id": None}`, which was already dropped. That + asymmetry let a two-real-member group pass the cache gate (which has no node + set to filter against) and then be dropped on replay by build_from_json, + leaving a cache hit that yields no semantic data and never re-dispatches.""" + assert canonical_hyperedge({"id": "h", "nodes": ["a", "b", junk]}) is None + kept = canonical_hyperedge({"id": "h", "nodes": ["a", "b", junk, "c"]}) + assert kept["nodes"] == ["a", "b", "c"] + + +@pytest.mark.parametrize("number", [7, 0]) +def test_canonical_hyperedge_canonicalizes_a_numeric_member(number): + """A numeric id is legitimate and is canonicalized to its string form, the + same coercion `_coerce_non_string_ids` applies on the build path. `0` in + particular must survive the boolean rejection sitting next to it.""" + assert canonical_hyperedge({"id": "h", "nodes": ["a", "b", number]})["nodes"] == [ + "a", "b", str(number), + ] + + +def test_canonical_hyperedge_counts_a_numeric_and_its_string_form_once(): + """`7` and `"7"` are the same node id — `_coerce_non_string_ids` str-coerces + numeric members on the build path. Deduplicating on the raw Python value + counted them separately, so `[7, "7", "b"]` passed the cache gate as three + members and then collapsed to two on replay and was dropped: precisely the + empty-cache-hit the gate exists to prevent.""" + assert canonical_hyperedge({"id": "h", "nodes": [7, "7", "b"]}) is None + kept = canonical_hyperedge({"id": "h", "nodes": [7, "7", "b", "c"]}) + assert kept["nodes"] == ["7", "b", "c"] + + +def test_canonical_hyperedge_keeps_a_group_exactly_at_the_minimum(): + """The threshold is inclusive — MIN_HYPEREDGE_MEMBERS distinct members pass.""" + members = [f"n{i}" for i in range(MIN_HYPEREDGE_MEMBERS)] + assert canonical_hyperedge({"id": "h", "nodes": members})["nodes"] == members + assert canonical_hyperedge({"id": "h", "nodes": members[:-1]}) is None + + +def test_to_json_tolerates_non_list_hyperedge_metadata(tmp_path): + """A direct caller can leave `G.graph["hyperedges"]` as None; iterating it + raised TypeError before the graph was written at all.""" + G = nx.Graph() + G.add_nodes_from(["a", "b", "c"]) + G.graph["hyperedges"] = None + out = tmp_path / "graph.json" + assert to_json(G, {0: ["a", "b", "c"]}, str(out)) + assert json.loads(out.read_text(encoding="utf-8"))["hyperedges"] == [] + + +def test_to_json_gates_hyperedges_written_by_a_direct_caller(tmp_path): + """to_json is public API and the final persistence boundary. A library caller + that populates G.graph["hyperedges"] itself bypasses build_from_json, + build_merge and attach_hyperedges, so the minimum-cardinality invariant has + to hold here too — in both JSON slots.""" + G = nx.Graph() + G.add_nodes_from(["a", "b", "c"]) + G.graph["hyperedges"] = [ + {"id": "pair", "nodes": ["a", "b"]}, + {"id": "dupes", "nodes": ["a", "a", "b"]}, + {"id": "dangling", "nodes": ["a", "b", "ghost"]}, + {"id": "alias", "members": ["a", "b", "c"]}, + {"id": "good", "nodes": ["a", "b", "c"]}, + ] + out = tmp_path / "graph.json" + assert to_json(G, {0: ["a", "b", "c"]}, str(out)) + + data = json.loads(out.read_text(encoding="utf-8")) + assert {h["id"] for h in data["hyperedges"]} == {"alias", "good"} + assert {h["id"] for h in data["graph"]["hyperedges"]} == {"alias", "good"} + assert next(h for h in data["hyperedges"] if h["id"] == "alias")["nodes"] == ["a", "b", "c"] + # The caller's own graph must not be mutated by an export. + assert len(G.graph["hyperedges"]) == 5 + + +# --------------------------------------------------------------------------- +# 2c. node_id_set / gate_hyperedges — the reusable pair the writers share +# --------------------------------------------------------------------------- + +def test_node_id_set_skips_ids_that_cannot_be_members(): + """An id-less node contributes nothing, and an unhashable id must be skipped + rather than poisoning the set (`None` would let a null member count) or + raising (a persisted list/dict id is deliberately left for the validator).""" + nodes = [ + {"id": "a"}, {"id": "b"}, + {"label": "no id at all"}, + {"id": None}, + {"id": ["malformed", "list"]}, + {"id": {"also": "malformed"}}, + "not-a-dict", + ] + assert node_id_set(nodes) == {"a", "b"} + + +def test_gate_hyperedges_without_a_node_list_checks_shape_only(): + """The pre-stamp and cache callers have no node set: members are not checked + for membership, only the group's shape and distinct cardinality.""" + kept, dropped = gate_hyperedges([ + {"id": "ghosts", "nodes": ["nope_a", "nope_b", "nope_c"]}, + {"id": "pair", "nodes": ["a", "b"]}, + ]) + assert [h["id"] for h in kept] == ["ghosts"] + assert dropped == 1 + + +def test_gate_hyperedges_filters_against_the_given_nodes(): + """The writer callers pass the node list about to be persisted.""" + nodes = [{"id": n} for n in ("a", "b", "c")] + kept, dropped = gate_hyperedges([ + {"id": "ok", "nodes": ["a", "b", "c"]}, + {"id": "dangling", "nodes": ["a", "b", "ghost"]}, + {"id": "alias", "members": ["a", "b", "c"]}, + ], nodes) + assert [h["id"] for h in kept] == ["ok", "alias"] + assert kept[1]["nodes"] == ["a", "b", "c"], "alias folded onto nodes" + assert dropped == 1 + + +def test_gate_hyperedges_resolves_members_through_the_normalized_id_space(): + """The clustered path heals casing/punctuation drift in member refs through + `norm_to_id` in build_from_json, so a group naming `Foo-Bar` for node + `foo_bar` survives there. The raw `--no-cluster` and watch writers never + reach build_from_json, so an exact-membership filter dropped a group that + is perfectly resolvable — a group silently lost by the gate that was added + to protect it. Resolve through the same normalized space, and hand back the + ids the nodes actually carry.""" + nodes = [{"id": "foo_bar"}, {"id": "baz_qux"}, {"id": "third"}] + kept, dropped = gate_hyperedges( + [{"id": "g", "nodes": ["Foo-Bar", "Baz Qux", "THIRD"]}], nodes + ) + assert dropped == 0, "a resolvable group must not be dropped" + assert kept[0]["nodes"] == ["foo_bar", "baz_qux", "third"], ( + "members come back as the persisted node ids, not the drifted refs" + ) + + +def test_gate_hyperedges_counts_a_drifted_and_exact_ref_to_one_node_once(): + """Two refs that normalize onto the same node are one member, not two — + otherwise normalized resolution would let a pair pass as a group.""" + nodes = [{"id": "foo_bar"}, {"id": "other"}] + assert gate_hyperedges( + [{"id": "g", "nodes": ["foo_bar", "Foo-Bar", "other"]}], nodes + ) == ([], 1) + + +def test_gate_hyperedges_prefers_an_exact_match_over_a_normalized_one(): + """An exact id must never be redirected to a different node that merely + normalizes the same way.""" + nodes = [{"id": "Foo-Bar"}, {"id": "foo_bar"}, {"id": "a"}, {"id": "b"}] + kept, _ = gate_hyperedges( + [{"id": "g", "nodes": ["Foo-Bar", "a", "b"]}], nodes + ) + assert kept[0]["nodes"] == ["Foo-Bar", "a", "b"] + + +def test_gate_hyperedges_reports_the_drop_count_for_the_caller_message(): + """Callers word their own stderr line, so the count comes back rather than + being printed here — the three writers say different things.""" + assert gate_hyperedges([], None) == ([], 0) + assert gate_hyperedges(None, None) == ([], 0) + _, dropped = gate_hyperedges([{"id": "p", "nodes": ["a", "b"]}], None) + assert dropped == 1 + + # --------------------------------------------------------------------------- # 3. to_json includes hyperedges key # --------------------------------------------------------------------------- def test_to_json_includes_hyperedges(): + """to_json writes the hyperedge set into graph.json.""" G = build_from_json(SAMPLE_EXTRACTION) communities = {0: list(G.nodes())} with tempfile.NamedTemporaryFile(suffix=".json", delete=False) as f: @@ -188,6 +511,7 @@ def test_to_json_includes_hyperedges(): def test_to_json_hyperedges_empty_when_none(): + """With no hyperedges the written key is an empty list.""" extraction = {**SAMPLE_EXTRACTION, "hyperedges": []} G = build_from_json(extraction) communities = {0: list(G.nodes())} @@ -227,6 +551,8 @@ def test_hyperedges_roundtrip_via_json_file(): # --------------------------------------------------------------------------- def _make_report(G): + + """Render a report for *G* with every node in one community.""" communities = {0: list(G.nodes())} cohesion = {0: 1.0} labels = {0: "All"} @@ -236,6 +562,7 @@ def _make_report(G): def test_report_includes_hyperedges_section(): + """A non-empty hyperedge set renders a hyperedges section in the report.""" G = build_from_json(SAMPLE_EXTRACTION) report = _make_report(G) assert "## Hyperedges (group relationships)" in report @@ -244,6 +571,9 @@ def test_report_includes_hyperedges_section(): def test_report_includes_hyperedge_node_list(): + + + """The hyperedges section lists each group's member ids.""" G = build_from_json(SAMPLE_EXTRACTION) report = _make_report(G) # Node IDs should appear in the report line @@ -256,6 +586,7 @@ def test_report_includes_hyperedge_node_list(): # --------------------------------------------------------------------------- def test_report_skips_hyperedges_section_when_empty(): + """An empty hyperedge set renders no hyperedges section in the report.""" extraction = {**SAMPLE_EXTRACTION, "hyperedges": []} G = build_from_json(extraction) report = _make_report(G) @@ -263,6 +594,7 @@ def test_report_skips_hyperedges_section_when_empty(): def test_report_skips_hyperedges_section_when_key_missing(): + """An extraction with no `hyperedges` key at all gets no report section.""" extraction = {k: v for k, v in SAMPLE_EXTRACTION.items() if k != "hyperedges"} G = build_from_json(extraction) report = _make_report(G) @@ -291,6 +623,7 @@ def _alias_extraction(): def test_build_normalizes_member_aliases_to_nodes(): + """Both `members` and `node_ids` aliases fold onto the canonical `nodes` key.""" G = build_from_json(_alias_extraction()) hes = {he["id"]: he for he in G.graph["hyperedges"]} for hid in ("he_nodes", "he_members", "he_node_ids"): @@ -301,20 +634,23 @@ def test_build_normalizes_member_aliases_to_nodes(): def test_build_dedups_alias_members_preserving_order(): + """Alias members are deduped in first-seen order.""" extraction = { "nodes": [ {"id": "a", "label": "A", "file_type": "code", "source_file": "m.py"}, {"id": "b", "label": "B", "file_type": "code", "source_file": "m.py"}, + {"id": "c", "label": "C", "file_type": "code", "source_file": "m.py"}, ], "edges": [], - "hyperedges": [{"id": "h", "label": "x", "members": ["a", "a", "b"]}], + "hyperedges": [{"id": "h", "label": "x", "members": ["a", "a", "b", "c"]}], } G = build_from_json(extraction) - assert G.graph["hyperedges"][0]["nodes"] == ["a", "b"] + assert G.graph["hyperedges"][0]["nodes"] == ["a", "b", "c"] assert "members" not in G.graph["hyperedges"][0] def test_build_canonical_nodes_wins_over_alias(): + """When both are present the canonical `nodes` key wins and the alias is dropped.""" extraction = { "nodes": [ {"id": "a", "label": "A", "file_type": "code", "source_file": "m.py"}, @@ -323,12 +659,12 @@ def test_build_canonical_nodes_wins_over_alias(): ], "edges": [], "hyperedges": [ - {"id": "h", "label": "x", "nodes": ["a", "b"], "members": ["x"]}, + {"id": "h", "label": "x", "nodes": ["a", "b", "x"], "members": ["b"]}, ], } G = build_from_json(extraction) he = G.graph["hyperedges"][0] - assert he["nodes"] == ["a", "b"] # canonical untouched + assert he["nodes"] == ["a", "b", "x"] # canonical untouched assert "members" not in he # stray alias dropped @@ -341,15 +677,16 @@ def test_build_rekeys_alias_keyed_hyperedge_members(): "nodes": [ {"id": "mod_foo", "label": "foo", "file_type": "code", "source_file": "pkg/mod.py"}, {"id": "mod_bar", "label": "bar", "file_type": "code", "source_file": "pkg/mod.py"}, + {"id": "mod_baz", "label": "baz", "file_type": "code", "source_file": "pkg/mod.py"}, ], "edges": [], "hyperedges": [ - {"id": "h", "label": "x", "members": ["mod_foo", "mod_bar"]}, + {"id": "h", "label": "x", "members": ["mod_foo", "mod_bar", "mod_baz"]}, ], } G = build_from_json(extraction) he = G.graph["hyperedges"][0] - assert he["nodes"] == ["pkg_mod_foo", "pkg_mod_bar"] + assert he["nodes"] == ["pkg_mod_foo", "pkg_mod_bar", "pkg_mod_baz"] def test_build_warns_once_per_aliased_hyperedge(capsys): diff --git a/tests/test_merge_graphs_cli.py b/tests/test_merge_graphs_cli.py index 643e29e0a1..2a1fbe81f5 100644 --- a/tests/test_merge_graphs_cli.py +++ b/tests/test_merge_graphs_cli.py @@ -184,6 +184,7 @@ def _write_with_hyperedges(p: Path, node_ids: list[str], hyperedges: list[dict], def test_merge_graphs_carries_hyperedges_from_all_inputs(tmp_path): + """#2484: every input's hyperedges reach the merged output with prefixed members.""" # #2484: prefix_graph_for_global never rewrote G.graph["hyperedges"], and # nx.compose's dict.update graph-attr merge clobbered each prior input's # list, so at best the LAST graph's hyperedges survived — with stale, @@ -191,8 +192,8 @@ def test_merge_graphs_carries_hyperedges_from_all_inputs(tmp_path): # relabeled to the prefixed node ids, in BOTH persistence slots. a = tmp_path / "alpha" / "graphify-out" / "graph.json" b = tmp_path / "beta" / "graphify-out" / "graph.json" - _write_with_hyperedges(a, ["x", "y"], [{"id": "h_alpha", "nodes": ["x", "y"]}]) - _write_with_hyperedges(b, ["p", "q"], [{"id": "h_beta", "nodes": ["p", "q"]}]) + _write_with_hyperedges(a, ["x", "y", "z"], [{"id": "h_alpha", "nodes": ["x", "y", "z"]}]) + _write_with_hyperedges(b, ["p", "q", "r"], [{"id": "h_beta", "nodes": ["p", "q", "r"]}]) out = tmp_path / "merged.json" r = _run(["merge-graphs", str(a), str(b), "--out", str(out)], tmp_path) @@ -214,13 +215,16 @@ def test_merge_graphs_carries_hyperedges_from_all_inputs(tmp_path): def test_merge_graphs_hyperedges_dedup_on_shared_prefixed_id(tmp_path): + """A duplicated id within an input yields one merged entry.""" # Idempotence: a duplicated hyperedge id within an input must not produce # duplicate entries in the merged output (attach_hyperedges dedups by id). a = tmp_path / "alpha" / "graphify-out" / "graph.json" b = tmp_path / "beta" / "graphify-out" / "graph.json" - he = {"id": "h_alpha", "nodes": ["x"]} - _write_with_hyperedges(a, ["x"], [he, dict(he)]) - _write_with_hyperedges(b, ["p"], [{"id": "h_beta", "nodes": ["p"]}]) + he = {"id": "h_alpha", "nodes": ["x", "y", "z"]} + _write_with_hyperedges(a, ["x", "y", "z"], [he, dict(he)]) + _write_with_hyperedges( + b, ["p", "q", "r"], [{"id": "h_beta", "nodes": ["p", "q", "r"]}] + ) out = tmp_path / "merged.json" r = _run(["merge-graphs", str(a), str(b), "--out", str(out)], tmp_path) @@ -231,12 +235,13 @@ def test_merge_graphs_hyperedges_dedup_on_shared_prefixed_id(tmp_path): def test_merge_graphs_reads_top_level_only_hyperedges(tmp_path): + """#2485: an input whose hyperedges live only at the top level is not lost.""" # #2485 skew on the input side: node_link_graph restores only the nested # graph-attrs slot, so an input whose hyperedges live only at the top # level used to lose them entirely. a = tmp_path / "alpha" / "graphify-out" / "graph.json" b = tmp_path / "beta" / "graphify-out" / "graph.json" - _write_with_hyperedges(a, ["x"], [{"id": "h_top", "nodes": ["x"]}], + _write_with_hyperedges(a, ["x", "y", "z"], [{"id": "h_top", "nodes": ["x", "y", "z"]}], top_level_only=True) _write_with_hyperedges(b, ["p"], []) out = tmp_path / "merged.json" @@ -245,10 +250,36 @@ def test_merge_graphs_reads_top_level_only_hyperedges(tmp_path): assert r.returncode == 0, r.stderr data = json.loads(out.read_text()) assert [h["id"] for h in data["hyperedges"]] == ["alpha::h_top"] - assert data["hyperedges"][0]["nodes"] == ["alpha::x"] + assert data["hyperedges"][0]["nodes"] == ["alpha::x", "alpha::y", "alpha::z"] +def test_merge_graphs_prefixes_alias_and_object_shaped_members(tmp_path): + """A member list is prefixed only once it is canonical, so the fold has to + happen BEFORE prefixing. Otherwise an alias-keyed (`members`) or + object-shaped (`{"id": ...}`) group keeps unprefixed member ids while every + node gains a `repo::` prefix, and the attach boundary then discards the + whole group for having no member backed by a node.""" + a = tmp_path / "alpha" / "graphify-out" / "graph.json" + b = tmp_path / "beta" / "graphify-out" / "graph.json" + _write_with_hyperedges(a, ["x", "y", "z"], [ + {"id": "h_alias", "members": ["x", "y", "z"]}, + {"id": "h_objects", "nodes": [{"id": "x"}, {"id": "y"}, {"id": "z"}]}, + ]) + _write_with_hyperedges(b, ["p", "q", "r"], [{"id": "h_beta", "nodes": ["p", "q", "r"]}]) + out = tmp_path / "merged.json" + + r = _run(["merge-graphs", str(a), str(b), "--out", str(out)], tmp_path) + assert r.returncode == 0, r.stderr + data = json.loads(out.read_text()) + hes = {h["id"]: h for h in data["hyperedges"]} + assert set(hes) == {"alpha::h_alias", "alpha::h_objects", "beta::h_beta"}, ( + f"no group may be lost to its member shape; got {sorted(hes)}" + ) + assert hes["alpha::h_alias"]["nodes"] == ["alpha::x", "alpha::y", "alpha::z"] + assert hes["alpha::h_objects"]["nodes"] == ["alpha::x", "alpha::y", "alpha::z"] + + def _write_with_communities(p: Path, nodes): p.parent.mkdir(parents=True, exist_ok=True) p.write_text(json.dumps({ @@ -304,3 +335,42 @@ def test_merge_graphs_community_offset_is_byte_reproducible(tmp_path): assert _run(["merge-graphs", str(a), str(b), "--out", str(out1)], tmp_path).returncode == 0 assert _run(["merge-graphs", str(a), str(b), "--out", str(out2)], tmp_path).returncode == 0 assert out1.read_bytes() == out2.read_bytes(), "same-order merge is not byte-reproducible" + +def test_merge_driver_gates_composed_hyperedges(tmp_path): + """`graphify merge-driver` composes two graph.json files and serializes the + result directly — it never reaches build_from_json or to_json, so nothing + applied the cardinality/dangling invariant. A legacy branch carrying a + two-member pair or a dangling group had that metadata written straight back + into graph.json, which is the same omission `watch`'s raw writer had. + """ + def write(p: Path, hyperedges): + """Write a three-node graph.json carrying *hyperedges* in graph attrs.""" + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(json.dumps({ + "directed": False, "multigraph": False, + "graph": {"hyperedges": hyperedges}, + "nodes": [{"id": "a"}, {"id": "b"}, {"id": "c"}], "links": [], + })) + + base = tmp_path / "base.json" + current = tmp_path / "current.json" + other = tmp_path / "other.json" + write(base, []) + write(current, [{"id": "ok", "nodes": ["a", "b", "c"]}]) + write(other, [ + {"id": "legacy_pair", "nodes": ["a", "b"]}, + {"id": "dangler", "nodes": ["a", "b", "ghost"]}, + ]) + + r = _run(["merge-driver", str(base), str(current), str(other)], tmp_path) + assert r.returncode == 0, r.stderr + + data = json.loads(current.read_text()) + written = data.get("graph", {}).get("hyperedges", []) + node_ids = {n["id"] for n in data["nodes"]} + assert all(len(h["nodes"]) >= 3 for h in written), ( + f"a sub-minimum group was written by the merge driver: {written}" + ) + assert all(set(h["nodes"]) <= node_ids for h in written), ( + f"a dangling member was written by the merge driver: {written}" + ) diff --git a/tests/test_non_string_node_ids.py b/tests/test_non_string_node_ids.py index f6f1d6c46b..673605d743 100644 --- a/tests/test_non_string_node_ids.py +++ b/tests/test_non_string_node_ids.py @@ -69,22 +69,470 @@ def test_float_id_is_coerced_too(): def test_legacy_from_to_endpoints_are_coerced(): """dedup reads the legacy from/to aliases (#803), so they need it as well.""" ext = { - "nodes": [_node(10, "Alpha"), _node("b", "Beta")], + "nodes": [_node(10, "Alpha"), _node("b", "Beta"), _node("c", "Gamma")], "edges": [{"from": 10, "to": "b", "relation": "uses", "confidence": "EXTRACTED"}], } G = build([ext], dedup=True) assert G.has_edge("10", "b") +def test_node_id_set_coerces_numeric_ids_like_members_are(): + """#2326 heals numeric node ids to their string form, and member coercion + does the same to member refs — so the comparison set has to be built in the + same space. Keyed on raw values, `"7" in {7}` is False and every member of + an otherwise valid group is dropped.""" + from graphify.build import gate_hyperedges, node_id_set + + nodes = [{"id": 7}, {"id": 8}, {"id": 9}] + assert node_id_set(nodes) == {"7", "8", "9"} + + kept, dropped = gate_hyperedges([{"id": "g", "nodes": [7, 8, 9]}], nodes) + assert dropped == 0, "a group over numeric node ids must survive" + assert kept[0]["nodes"] == [7, 8, 9], ( + "the raw writers persist `nodes` unchanged, so surviving members have " + "to come back in the node list's own id space" + ) + + +def test_gate_hyperedges_returns_members_in_the_node_lists_own_id_space(): + """The raw `--no-cluster` writers gate against the node records they are + about to persist and write those records unchanged. Coercing only the + comparison side left the file holding nodes `[7, 8, 9]` and members + `["7", "8", "9"]` — a dangling reference, the shape #1916 removed, written + by the gate that exists to prevent it. Compare coerced, return raw.""" + from graphify.build import gate_hyperedges + from graphify.watch import _gated_hyperedges + + nodes = [{"id": 7}, {"id": 8}, {"id": 9}] + written_ids = {n["id"] for n in nodes} + + kept, _ = gate_hyperedges([{"id": "g", "nodes": [7, 8, 9]}], nodes) + assert set(kept[0]["nodes"]) <= written_ids, ( + f"members {kept[0]['nodes']} must name nodes actually written " + f"{sorted(written_ids, key=str)}" + ) + + # watch's raw writer shares the gate and writes the same node records. + members = _gated_hyperedges([{"id": "g", "nodes": [7, 8, 9]}], nodes)[0]["nodes"] + assert set(members) <= written_ids + + +def test_prune_graph_json_sources_keeps_the_files_own_node_id_space(): + """An externally produced or legacy graph.json can carry numeric node ids. + The pruner rewrites hyperedges but leaves the node records alone, so a + coerced member list would turn a valid group into a dangling one on disk.""" + import json + + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_graph_json( + nodes=[ + {"id": 7, "source_file": "a.py"}, + {"id": 8, "source_file": "a.py"}, + {"id": 9, "source_file": "a.py"}, + {"id": 99, "source_file": "gone.py"}, + ], + hyperedges=[{"id": "g", "source_file": "a.py", "nodes": [7, 8, 9, 99]}], + ) + _prune_graph_json_sources(graph_path, ["gone.py"]) + + data = json.loads(graph_path.read_text(encoding="utf-8")) + node_ids = {n["id"] for n in data["nodes"]} + members = data["hyperedges"][0]["nodes"] + assert node_ids == {7, 8, 9}, "the stale source's node is pruned" + assert set(members) <= node_ids, ( + f"members {members} must name nodes actually written " + f"{sorted(node_ids, key=str)}" + ) + + +def test_prune_graph_json_sources_tolerates_a_non_dict_graph_value(): + """A legacy or hand-edited graph.json can carry a non-dict `graph` value. + Reading the nested slot as `(data.get("graph") or {}).get(...)` raised + AttributeError straight out of the function — the try above it wraps only + the JSON load — so the whole exclusion-only prune aborted. The nested-sync + code further down already guards with isinstance; this read must too.""" + import json + + from graphify.cli import _prune_graph_json_sources + + for bad in ("oops", [1, 2], 5, True): + path = tmp_graph_json( + nodes=[{"id": "a", "source_file": "gone.py"}, + {"id": "b", "source_file": "live.py"}], + hyperedges=[], + ) + data = json.loads(path.read_text(encoding="utf-8")) + data["graph"] = bad + # The nested read is only reached when the TOP-LEVEL slot is not a list, + # so leaving `"hyperedges": []` here would skip the branch under test + # entirely and the assertion would pass for the wrong reason. + del data["hyperedges"] + path.write_text(json.dumps(data), encoding="utf-8") + + removed = _prune_graph_json_sources(path, ["gone.py"]) + assert removed == 1, f"the prune must still run with graph={bad!r}" + assert {n["id"] for n in json.loads(path.read_text())["nodes"]} == {"b"} + + +def test_gate_coerces_a_raw_set_container_like_every_other_kind(): + """A set was once trusted as already coerced, which made numeric ids behave + differently from a list or a graph: members became `"7"` and then failed + membership against `{7, 8, 9}`, dropping a valid group. Every container kind + goes through the same coercion now; this pins it so the exemption cannot + come back as an optimization.""" + from graphify.build import gate_hyperedges_against_graph + + for container in ({7, 8, 9}, frozenset({7, 8, 9}), [7, 8, 9]): + kept, dropped = gate_hyperedges_against_graph( + [{"id": "g", "nodes": [7, 8, 9]}], container + ) + assert dropped == 0, f"a valid group must survive against {container!r}" + assert kept[0]["nodes"] == [7, 8, 9], "in the container's own id space" + + +def tmp_graph_json(*, nodes, hyperedges): + """Write a minimal hand-authored graph.json and return its path.""" + import json + import tempfile + from pathlib import Path + + path = Path(tempfile.mkdtemp()) / "graph.json" + path.write_text( + json.dumps({"nodes": nodes, "edges": [], "hyperedges": hyperedges}), + encoding="utf-8", + ) + return path + + +def test_graph_container_membership_uses_the_coerced_id_space(): + """`attach_hyperedges`, `to_json` and `build_merge` pass the graph itself as + the container. Coercing only the member side left `"7" in nx.Graph([7])` + False, so every member of a valid group over numeric node ids was dropped — + the list path was fixed by node_id_set, the container path was not.""" + import json + import tempfile + from pathlib import Path + + import networkx as nx + + from graphify.build import gate_hyperedges_against_graph + from graphify.export import attach_hyperedges, to_json + + G = nx.Graph() + G.add_nodes_from([7, 8, 9]) + kept, _ = gate_hyperedges_against_graph([{"id": "g", "nodes": [7, 8, 9]}], G) + assert kept[0]["nodes"] == [7, 8, 9], "in the graph's own id space" + + H = nx.Graph() + H.add_nodes_from([7, 8, 9]) + attach_hyperedges(H, [{"id": "g", "nodes": [7, 8, 9]}]) + assert [h["id"] for h in H.graph.get("hyperedges", [])] == ["g"] + + J = nx.Graph() + J.add_nodes_from([7, 8, 9]) + J.graph["hyperedges"] = [{"id": "g", "nodes": [7, 8, 9]}] + out = Path(tempfile.mkdtemp()) / "graph.json" + to_json(J, {0: [7, 8, 9]}, str(out)) + assert [h["id"] for h in json.loads(out.read_text())["hyperedges"]] == ["g"] + + +def test_to_json_writes_members_in_the_graphs_own_id_space(): + """Coercing only the comparison side left graph.json internally inconsistent: + node_link_data writes `{"id": 7}` while the surviving member reads `"7"`, so + the written file carries a dangling member — the very shape #1916 removed. + Whatever the gate keeps has to come back out in the node ids' own space.""" + import json + import tempfile + from pathlib import Path + + import networkx as nx + + from graphify.export import to_json + + G = nx.Graph() + G.add_nodes_from([7, 8, 9]) + G.graph["hyperedges"] = [{"id": "g", "nodes": [7, 8, 9]}] + out = Path(tempfile.mkdtemp()) / "graph.json" + to_json(G, {0: [7, 8, 9]}, str(out)) + + data = json.loads(out.read_text(encoding="utf-8")) + node_ids = {n["id"] for n in data["nodes"]} + assert data["hyperedges"], "the group must survive" + members = data["hyperedges"][0]["nodes"] + assert set(members) <= node_ids, ( + f"members {members} must name nodes actually written {sorted(node_ids, key=str)}" + ) + + +def test_semantic_cleanup_keeps_a_group_over_numeric_node_ids(): + """Member refs are coerced, so the surviving-id set has to be read in the + same space or every member of a valid numeric group is filtered out. + + Members come back as the fragment's own node ids rather than the coerced + spelling: this pass now delegates to the shared gate, which returns the + container's ids so a member always names a node that is actually there. + An earlier revision of this test asserted `["7", "8", "9"]`, which was the + weaker guarantee the pass made when it filtered members itself. + """ + from graphify.semantic_cleanup import sanitize_semantic_fragment + + fragment = { + "nodes": [ + {"id": n, "label": f"N{n}", "file_type": "code", "source_file": "a.py"} + for n in (7, 8, 9) + ], + "edges": [], + "hyperedges": [{"id": "g", "nodes": [7, 8, 9]}], + } + out = sanitize_semantic_fragment(fragment) + assert [h["id"] for h in out["hyperedges"]] == ["g"] + assert out["hyperedges"][0]["nodes"] == [7, 8, 9] + assert set(out["hyperedges"][0]["nodes"]) <= {n["id"] for n in out["nodes"]} + + +def test_prefix_graph_for_global_builds_the_relabel_map_once(monkeypatch): + """The coerced relabel map must be built once per graph, not once per + hyperedge — rebuilding it inside the loop makes prefixing O(nodes x + hyperedges), and a semantic graph can carry thousands of groups.""" + import networkx as nx + + import graphify.build as buildmod + + calls = {"n": 0} + real = buildmod._coerce_id + + def counting(value): + """Count every _coerce_id call so the map rebuild is detectable.""" + calls["n"] += 1 + return real(value) + + monkeypatch.setattr(buildmod, "_coerce_id", counting) + + nodes = list(range(50)) + G = nx.Graph() + G.add_nodes_from(nodes) + G.graph["hyperedges"] = [ + {"id": f"h{i}", "nodes": [0, 1, 2]} for i in range(20) + ] + buildmod.prefix_graph_for_global(G, "repo") + + # 50 nodes + 20 groups x 3 members = 110 if the map is built once; rebuilding + # it per hyperedge costs 50 x 20 = 1000 extra coercions on its own. + assert calls["n"] < 500, ( + f"_coerce_id called {calls['n']} times — the relabel map is being " + f"rebuilt per hyperedge" + ) + + +def test_attach_hyperedges_builds_the_graph_id_map_once(monkeypatch): + """Same defect class as the prefix_graph_for_global map above, in the other + direction: gating one candidate at a time rebuilt the graph's coerced id map + for every hyperedge, so merge-graphs went from linear to O(nodes x groups) + on exactly the thousands-of-groups corpora this gate was added for.""" + import networkx as nx + + import graphify.build as buildmod + from graphify.export import attach_hyperedges + + calls = {"n": 0} + real = buildmod._coerce_id + + def counting(value): + """Count every _coerce_id call so a per-candidate rebuild is visible.""" + calls["n"] += 1 + return real(value) + + monkeypatch.setattr(buildmod, "_coerce_id", counting) + + G = nx.Graph() + G.add_nodes_from(range(50)) + attach_hyperedges(G, [{"id": f"h{i}", "nodes": [0, 1, 2]} for i in range(20)]) + + assert [h["id"] for h in G.graph["hyperedges"]] == [f"h{i}" for i in range(20)] + # 50 nodes + 20 groups x 3 members = 110 with the map built once; per + # candidate it costs 50 x 20 = 1000 extra coercions on its own. + assert calls["n"] < 500, ( + f"_coerce_id called {calls['n']} times — the graph id map is being " + f"rebuilt per hyperedge" + ) + + +def test_semantic_cleanup_resolves_normalized_members(): + """`sanitize_semantic_fragment` filtered members with an exact `in` test, so + a member the gate and build_from_json both resolve was removed and the group + dropped below the minimum. Every membership decision in the feature has to + use one resolution rule.""" + from graphify.semantic_cleanup import sanitize_semantic_fragment + + fragment = { + "nodes": [ + {"id": n, "label": n, "file_type": "code", "source_file": "a.py"} + for n in ("foo_bar", "b", "c") + ], + "edges": [], + "hyperedges": [{"id": "g", "nodes": ["Foo-Bar", "b", "c"]}], + } + out = sanitize_semantic_fragment(fragment) + assert [h["id"] for h in out["hyperedges"]] == ["g"] + assert out["hyperedges"][0]["nodes"] == ["foo_bar", "b", "c"] + + +def test_watch_reconcile_keeps_a_group_with_normalized_members(tmp_path): + """`_reconcile_existing_graph` drops a preserved group when ANY member is + absent, using a raw `in` test upstream of the gate — so an unrelated watch + rebuild deleted a group the gate resolves and keeps. The whole-group drop + semantics stay; only the resolution changes.""" + import json + + from graphify.watch import _reconcile_existing_graph + + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps({ + "nodes": [ + {"id": n, "source_file": "keep.py", "_origin": "ast", + "source_location": "L1"} + for n in ("foo_bar", "baz_qux", "third") + ], + "edges": [], + "hyperedges": [{"id": "g", "nodes": ["Foo-Bar", "Baz Qux", "THIRD"], + "source_file": "keep.py"}], + }), encoding="utf-8") + + merged, _ = _reconcile_existing_graph( + graph_path, + {"nodes": [], "edges": [], "hyperedges": []}, + out=tmp_path, + project_root=tmp_path, + watch_root=tmp_path, + code_files=[tmp_path / "keep.py"], + extract_targets=[], + full_rebuild=False, + deleted_paths=set(), + deleted_source_identities=set(), + ) + assert [h["id"] for h in merged.get("hyperedges", [])] == ["g"], ( + "a group whose members resolve must survive reconciliation" + ) + + +def test_watch_reconcile_keeps_a_group_over_numeric_node_ids(tmp_path): + """Routing watch's membership check through the shared lookup order is not + enough on its own: `all_ids` holds the node ids raw, so a numeric node id + stays `7` while its member coerces to `"7"` and the group is evicted. The + member-side set has to be built in the shared key space. `all_ids` itself + stays raw, because the edge-endpoint checks above compare raw endpoints.""" + import json + + from graphify.watch import _reconcile_existing_graph + + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps({ + "nodes": [ + {"id": n, "source_file": "keep.py", "_origin": "ast", + "source_location": "L1"} + for n in (7, 8, 9) + ], + "edges": [], + "hyperedges": [{"id": "g", "nodes": [7, 8, 9], "source_file": "keep.py"}], + }), encoding="utf-8") + + merged, _ = _reconcile_existing_graph( + graph_path, + {"nodes": [], "edges": [], "hyperedges": []}, + out=tmp_path, + project_root=tmp_path, + watch_root=tmp_path, + code_files=[tmp_path / "keep.py"], + extract_targets=[], + full_rebuild=False, + deleted_paths=set(), + deleted_source_identities=set(), + ) + assert [h["id"] for h in merged.get("hyperedges", [])] == ["g"] + + +def test_prefix_graph_for_global_prefixes_normalized_members(): + """The gate resolves a member that drifted in casing or punctuation, so the + cross-repo relabel has to resolve it the same way. Keyed on the coerced + spelling only, `Foo-Bar` stayed unprefixed while its node became + `repo::foo_bar`, and `attach_hyperedges` then dropped the whole group — a + valid group lost to two halves of one invariant disagreeing.""" + import networkx as nx + + from graphify.build import prefix_graph_for_global + from graphify.export import attach_hyperedges + + G = nx.Graph() + G.add_nodes_from(["foo_bar", "baz_qux", "third"]) + G.graph["hyperedges"] = [{"id": "g", "nodes": ["Foo-Bar", "Baz Qux", "THIRD"]}] + + H = prefix_graph_for_global(G, "repo") + assert H.graph["hyperedges"][0]["nodes"] == [ + "repo::foo_bar", "repo::baz_qux", "repo::third", + ] + + merged = nx.Graph() + merged.add_nodes_from(H.nodes) + attach_hyperedges(merged, [dict(H.graph["hyperedges"][0])]) + assert [h["id"] for h in merged.graph.get("hyperedges", [])] == ["repo::g"] + + +def test_prefix_graph_for_global_counts_two_refs_to_one_node_once(): + """Normalized resolution must not let a pair through: an exact and a drifted + ref to the same node prefix to one member, so the group is under the + minimum and the attach boundary drops it.""" + import networkx as nx + + from graphify.build import prefix_graph_for_global + from graphify.export import attach_hyperedges + + G = nx.Graph() + G.add_nodes_from(["foo_bar", "other"]) + G.graph["hyperedges"] = [{"id": "g", "nodes": ["foo_bar", "Foo-Bar", "other"]}] + + H = prefix_graph_for_global(G, "repo") + assert H.graph["hyperedges"][0]["nodes"] == ["repo::foo_bar", "repo::other"] + + merged = nx.Graph() + merged.add_nodes_from(H.nodes) + attach_hyperedges(merged, [dict(H.graph["hyperedges"][0])]) + assert merged.graph.get("hyperedges", []) == [] + + +def test_prefix_graph_for_global_prefixes_numeric_members(): + """`merge-graphs` relabels node `7` to `repo::7`, and member normalization + turns the member into `"7"` — so the relabel lookup must be keyed in the + coerced space too, or the member stays unprefixed and the attach boundary + drops the group for having no member backed by a node.""" + import networkx as nx + + from graphify.build import prefix_graph_for_global + from graphify.export import attach_hyperedges + + G = nx.Graph() + G.add_nodes_from([7, 8, 9]) + G.graph["hyperedges"] = [{"id": "g", "nodes": [7, 8, 9]}] + + H = prefix_graph_for_global(G, "repo") + assert H.graph["hyperedges"][0]["nodes"] == ["repo::7", "repo::8", "repo::9"] + + merged = nx.Graph() + merged.add_nodes_from(H.nodes) + attach_hyperedges(merged, [dict(H.graph["hyperedges"][0])]) + assert [h["id"] for h in merged.graph.get("hyperedges", [])] == ["repo::g"] + + def test_hyperedge_members_are_coerced_with_their_nodes(): + """#2326: a numeric member is str-coerced alongside its node id.""" ext = { - "nodes": [_node(10, "Alpha"), _node("b", "Beta")], + "nodes": [_node(10, "Alpha"), _node("b", "Beta"), _node("c", "Gamma")], "edges": [], - "hyperedges": [{"id": "he1", "label": "grp", "nodes": [10, "b"]}], + "hyperedges": [{"id": "he1", "label": "grp", "nodes": [10, "b", "c"]}], } G = build([ext], dedup=True) members = G.graph["hyperedges"][0]["nodes"] - assert members == ["10", "b"] + assert members == ["10", "b", "c"] def test_build_from_json_coerces_on_the_direct_entry(): diff --git a/tests/test_semantic_cleanup.py b/tests/test_semantic_cleanup.py index de13bd2452..a146cda22a 100644 --- a/tests/test_semantic_cleanup.py +++ b/tests/test_semantic_cleanup.py @@ -240,6 +240,7 @@ def test_sanitize_filters_hyperedges_after_node_removal(): "nodes": [ {"id": "real_node", "label": "Real", "file_type": "code"}, {"id": "other", "label": "Other", "file_type": "code"}, + {"id": "third", "label": "Third", "file_type": "code"}, {"id": "garbage", "label": "junk", "file_type": "rationale"}, ], "edges": [], @@ -247,7 +248,7 @@ def test_sanitize_filters_hyperedges_after_node_removal(): { "id": "group_a", "label": "Group A", - "nodes": ["garbage", "real_node", "other"], + "nodes": ["garbage", "real_node", "other", "third"], "relation": "participate_in", }, { @@ -260,11 +261,11 @@ def test_sanitize_filters_hyperedges_after_node_removal(): } out = sc.sanitize_semantic_fragment(fragment) he_ids = {he["id"] for he in out["hyperedges"]} - # group_a survives with garbage filtered out + # group_a survives with three valid members after garbage is filtered out assert "group_a" in he_ids group_a = next(he for he in out["hyperedges"] if he["id"] == "group_a") assert "garbage" not in group_a["nodes"] - assert set(group_a["nodes"]) == {"real_node", "other"} + assert set(group_a["nodes"]) == {"real_node", "other", "third"} # group_b had only 1 surviving member → dropped assert "group_b" not in he_ids @@ -349,23 +350,28 @@ def test_sanitize_rationale_only_propagates_through_rationale_for_edges(): def test_sanitize_keeps_members_keyed_hyperedge(capsys): - """#1561: a `members`-keyed hyperedge with >=2 surviving members must be + """#1561: a `members`-keyed hyperedge with >=3 surviving members must be KEPT (normalized to `nodes`), not silently dropped before build.""" fragment = { "nodes": [ {"id": "real_a", "label": "A", "file_type": "code"}, {"id": "real_b", "label": "B", "file_type": "code"}, + {"id": "real_c", "label": "C", "file_type": "code"}, ], "edges": [], "hyperedges": [ - {"id": "grp", "label": "Group", "members": ["real_a", "real_b"]}, + { + "id": "grp", + "label": "Group", + "members": ["real_a", "real_b", "real_c"], + }, ], } out = sc.sanitize_semantic_fragment(fragment) assert len(out["hyperedges"]) == 1 he = out["hyperedges"][0] assert he["id"] == "grp" - assert he["nodes"] == ["real_a", "real_b"] + assert he["nodes"] == ["real_a", "real_b", "real_c"] assert "members" not in he diff --git a/tests/test_watch.py b/tests/test_watch.py index a189446b6e..53409b5d23 100644 --- a/tests/test_watch.py +++ b/tests/test_watch.py @@ -733,10 +733,17 @@ def test_rebuild_code_evicts_nodes_from_deleted_files(tmp_path): def _add_unrelated_semantic_pair(graph_path): + """Seed an unrelated semantic node pair, their link, and a group over them. + + The group names a third concept as well: a hyperedge needs three distinct + members to be one, so a two-member fixture would simply be gated away and + the survival assertions would pass vacuously. + """ data = json.loads(graph_path.read_text(encoding="utf-8")) data["nodes"].extend([ {"id": "docs_topic", "label": "DocsTopic", "file_type": "concept"}, {"id": "shared_concept", "label": "SharedConcept", "file_type": "concept"}, + {"id": "third_concept", "label": "ThirdConcept", "file_type": "concept"}, ]) data["links"].append({ "source": "docs_topic", @@ -746,7 +753,7 @@ def _add_unrelated_semantic_pair(graph_path): data["hyperedges"] = [{ "id": "semantic_context", "label": "Semantic context", - "nodes": ["docs_topic", "shared_concept"], + "nodes": ["docs_topic", "shared_concept", "third_concept"], }] graph_path.write_text(json.dumps(data), encoding="utf-8") @@ -771,11 +778,13 @@ def test_rebuild_code_preserves_hyperedges_for_rebuilt_surviving_source( assert _rebuild_code(corpus, no_cluster=True, acquire_lock=False) is True graph_path = corpus / "graphify-out" / "graph.json" data = json.loads(graph_path.read_text(encoding="utf-8")) - assert {"doc", "doc_design"} <= {node["id"] for node in data["nodes"]} + assert {"doc", "doc_design", "doc_flow"} <= {node["id"] for node in data["nodes"]} + # Three surviving members: a pair is not a hyperedge and would be gated away, + # making the preservation assertion below vacuous. data["hyperedges"] = [{ "id": "doc_flow_group", "label": "Doc flow group", - "nodes": ["doc", "doc_design"], + "nodes": ["doc", "doc_design", "doc_flow"], "relation": "implements", "confidence": "EXTRACTED", "confidence_score": 1.0, @@ -794,7 +803,7 @@ def test_rebuild_code_preserves_hyperedges_for_rebuilt_surviving_source( assert after["hyperedges"] == [{ "id": "doc_flow_group", "label": "Doc flow group", - "nodes": ["doc", "doc_design"], + "nodes": ["doc", "doc_design", "doc_flow"], "relation": "implements", "confidence": "EXTRACTED", "confidence_score": 1.0, @@ -3058,6 +3067,42 @@ def test_rebuild_code_inherits_directed_flag_no_cluster(tmp_path): ) +def test_rebuild_code_drops_legacy_pair_no_cluster(tmp_path): + """`graphify update --no-cluster` goes through watch, whose raw writer copies + `result`'s hyperedges straight into the candidate JSON. _reconcile_existing_graph + only evicts by source and dangling members, so a legacy two-member group whose + nodes are both still present was carried through every rebuild — unlike the + clustered watch path and the CLI raw path, which both gate cardinality.""" + from graphify.watch import _rebuild_code + + corpus = tmp_path / "corpus" + corpus.mkdir() + (corpus / "a.py").write_text("def f(): pass\n\ndef h(): pass\n", encoding="utf-8") + assert _rebuild_code(corpus, no_cluster=True, acquire_lock=False) is True + + graph_path = corpus / "graphify-out" / "graph.json" + data = json.loads(graph_path.read_text(encoding="utf-8")) + ids = [n["id"] for n in data["nodes"]][:2] + assert len(ids) == 2, f"need two real nodes to name; got {ids}" + # A pair whose members both still exist: nothing evicts it, nothing dangles. + data["hyperedges"] = [{"id": "legacy_pair", "nodes": ids, "source_file": "a.py"}] + graph_path.write_text(json.dumps(data), encoding="utf-8") + + n_before = len(json.loads(graph_path.read_text(encoding="utf-8"))["nodes"]) + (corpus / "a.py").write_text( + "def f(): pass\n\ndef h(): pass\n\ndef g(): pass\n", encoding="utf-8") + assert _rebuild_code(corpus, no_cluster=True, acquire_lock=False) is True + + after = json.loads(graph_path.read_text(encoding="utf-8")) + assert len(after["nodes"]) > n_before, ( + "guard: graph.json must actually have been rewritten, or the assertion " + "below is vacuous" + ) + assert [h["id"] for h in after.get("hyperedges", [])] == [], ( + "a two-member group must not survive a no-cluster update rebuild" + ) + + def test_rebuild_code_keeps_undirected_graph_undirected(tmp_path): """An existing undirected graph (no directed key, the on-disk default) must not be spuriously flipped to directed=True by an update rebuild.""" From 3d184c48c1c9e7508e6bd7fe70afaa25d3dc391d Mon Sep 17 00:00:00 2001 From: Eduardo Garcia-Prieto Date: Thu, 3 Sep 2026 20:24:55 +1000 Subject: [PATCH 2/3] fix(dedup): key the member raw-form capture by entry, not by position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_finish` captures each hyperedge's raw member forms BEFORE the remap, so a member can be restored onto the node it actually named rather than whichever node the id map prefers for its coerced key. That capture was indexed by position — and `_remap_hyperedge_members`, which runs in between, drops undersized groups and rewrites the list in place with `hyperedges[:] = kept`. After the first drop every surviving group was therefore restored through a DIFFERENT group's capture. With distinct nodes `7` and `"7"` present, a group naming the string node was resolved through a dropped group's `"7" -> 7` entry and silently rebound to the int node — reintroducing exactly the failure the capture exists to prevent, and the worse half of it: not a dangling member, a wrong one that looks correct. Keyed by `id(he)` instead. The entries are mutated in place, so a survivor keeps its identity across the remap; a dropped entry leaves a key nothing ever looks up, because the map is only queried for an entry still in the list. Reachable from the public `deduplicate_entities(..., hyperedges=)` API, which the new test drives; it needs one group that falls under the minimum ahead of one that survives. Co-Authored-By: Claude Opus 5 --- graphify/dedup.py | 32 +++++++++++++++++++-------- tests/test_dedup_remaps_hyperedges.py | 32 +++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/graphify/dedup.py b/graphify/dedup.py index b089490061..1f719da33c 100644 --- a/graphify/dedup.py +++ b/graphify/dedup.py @@ -466,7 +466,7 @@ def _report_id_collision(nid: str, survivor: dict, losers: list[dict]) -> None: # ── main entry point ────────────────────────────────────────────────────────── -def _member_raw_forms(hyperedges: list) -> list[dict]: +def _member_raw_forms(hyperedges: list) -> dict[int, dict]: """Per hyperedge, map each member's coerced key back to its original id. Captured BEFORE remapping, because remapping coerces members and that @@ -475,8 +475,17 @@ def _member_raw_forms(hyperedges: list) -> list[dict]: ``"7"``, and nothing downstream can tell them apart — so restoration would bind both to whichever node the id map prefers, silently moving one member to a different node. + + Keyed by the identity of the entry each capture came from, NOT by position: + :func:`_remap_hyperedge_members` runs in between, and it drops undersized + groups and compacts the list in place. Read back positionally, every group + after the first drop was handed a DIFFERENT group's raw forms — which + reintroduces the exact silent rebind this capture exists to prevent. The + entries are mutated in place, so a surviving group keeps its identity; a + dropped one leaves a key nothing ever looks up, because the map is only + ever queried for an entry still in the list. """ - originals: list[dict] = [] + originals: dict[int, dict] = {} for he in hyperedges or (): seen: dict = {} if isinstance(he, dict) and isinstance(he.get("nodes"), list): @@ -484,11 +493,13 @@ def _member_raw_forms(hyperedges: list) -> list[dict]: raw = m.get("id") if isinstance(m, dict) else m if _hashable(raw): seen.setdefault(_coerce_id(raw), raw) - originals.append(seen) + originals[id(he)] = seen return originals -def _restore_member_id_space(hyperedges: list, nodes: list, originals: list) -> None: +def _restore_member_id_space( + hyperedges: list, nodes: list, originals: dict[int, dict] +) -> None: """Rewrite each member to the id its node record carries, in place. Members are coerced during remapping so they can be compared and deduped; @@ -515,23 +526,26 @@ def _restore_member_id_space(hyperedges: list, nodes: list, originals: list) -> if isinstance(n, dict) and _hashable(n.get("id")) } - def resolved(key: object, index: int) -> object: + def resolved(key: object, raws: dict) -> object: """The node id member *key* should carry, preferring its own raw form.""" - own = (originals[index] if index < len(originals) else {}).get(key) + own = raws.get(key) if own is not None and own in raw_ids: return own return raw_by_coerced.get(key) - for i, he in enumerate(hyperedges or ()): + for he in hyperedges or (): if not isinstance(he, dict) or not isinstance(he.get("nodes"), list): continue + # By identity, not position: the remap between capture and restore + # compacted the list (see _member_raw_forms). + raws = originals.get(id(he)) or {} restored: list = [] for m in he["nodes"]: if isinstance(m, dict): - raw = resolved(_coerce_id(m.get("id")), i) + raw = resolved(_coerce_id(m.get("id")), raws) restored.append(dict(m, id=raw) if raw is not None else m) else: - raw = resolved(m, i) + raw = resolved(m, raws) restored.append(m if raw is None else raw) he["nodes"] = restored diff --git a/tests/test_dedup_remaps_hyperedges.py b/tests/test_dedup_remaps_hyperedges.py index a1dc7a5be9..9839541033 100644 --- a/tests/test_dedup_remaps_hyperedges.py +++ b/tests/test_dedup_remaps_hyperedges.py @@ -287,3 +287,35 @@ def test_direct_dedup_does_not_rebind_a_member_to_a_colliding_node(): f"not the colliding {by_id.get(member)!r}" ) + + +def test_a_dropped_group_does_not_shift_the_raw_form_capture(): + """Dropping an undersized group must not misalign the raw-form capture. + + The capture that protects a member from being rebound onto a colliding node + is taken per hyperedge BEFORE the remap; the remap then drops undersized + groups and compacts the list in place. Read back by position, every + surviving group after the first drop was handed the raw forms of a + DIFFERENT group — so with distinct nodes `7` and `"7"` present, `trio`'s + member `"7"` was resolved through `pair`'s capture and silently rebound to + the int node it never named. That is #3297's failure mode: not a dangling + member, a wrong one. + """ + hes = [ + {"id": "pair", "nodes": [7, "delta_node"]}, + {"id": "trio", "nodes": ["7", "beta_node", "gamma_node"]}, + ] + deduplicate_entities( + [ + _node(7, "Int Seven"), + _node("7", "Str Seven"), + _node("beta_node", "Beta"), + _node("gamma_node", "Gamma"), + _node("delta_node", "Delta"), + ], + [], communities={}, hyperedges=hes, + ) + assert [h["id"] for h in hes] == ["trio"] + assert hes[0]["nodes"] == ["7", "beta_node", "gamma_node"], ( + "the member must keep naming the string node it was written against" + ) From 6716de8ba1c456a058d1714814117008822222a5 Mon Sep 17 00:00:00 2001 From: Eduardo Garcia-Prieto Date: Thu, 3 Sep 2026 20:25:08 +1000 Subject: [PATCH 3/3] fix: canonicalize a member ref inside member_in_id_space, not per caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `member_in_id_space` is the yes/no half of the shared gate, and its three callers read members straight off a persisted group that has NOT been through `_normalize_hyperedge_members` — watch's reconciliation drop reads `edge["nodes"]` verbatim out of graph.json. So both member shapes the feature tolerates arrived raw, and `_member_keys` assumed a caller had already canonicalized: - an object-shaped member (`{"id": "a"}`) is unhashable, so it named nothing and watch DELETED, on an unrelated rebuild, a group every one of whose members is alive. `_coerce_hyperedge_member_refs` exists precisely because backends emit that shape, so this is a live loss of valid content; - an uncoerced numeric member missed the `_coerce_id`-coerced key `node_id_set` holds, so the cache's skipped-node prune left a group referencing a node deliberately not written. `canonical_member_ref` unwraps an object member to its `id` and coerces, and `member_in_id_space` applies it to its own argument. An id-less object collapses to `None`, which resolves to nothing — canonicalizing must not invent a member. It is idempotent, so the two callers that pre-coerced are unaffected; both now pass the member as emitted, and watch's `_coerce_member` seam is gone with its only caller's need for it. The watch test drives the real `_rebuild_code` path end to end: the group survives, and the writers' gate flattens its members to bare ids on the way out, so the persisted shape stays canonical. Co-Authored-By: Claude Opus 5 --- graphify/build.py | 30 ++++++++++++++++++++++++++- graphify/cache.py | 6 ++++-- graphify/watch.py | 15 +++++++------- tests/test_hypergraph.py | 45 ++++++++++++++++++++++++++++++++++++++++ tests/test_watch.py | 41 ++++++++++++++++++++++++++++++++++++ 5 files changed, 126 insertions(+), 11 deletions(-) diff --git a/graphify/build.py b/graphify/build.py index 75c74f478e..339aa01c11 100644 --- a/graphify/build.py +++ b/graphify/build.py @@ -138,14 +138,42 @@ def resolve_member_ref(member: object, raw_by_coerced: dict, default: object = N return default +def canonical_member_ref(member: object) -> object: + """One member ref in the canonical scalar space, whatever shape it arrives in. + + An object-shaped member (``{"id": "a"}``) names its ``id``; a numeric ref is + coerced by :func:`_coerce_id`. :func:`_coerce_hyperedge_member_refs` applies + this same rule to a whole member list, with a WARNING and a drop for an + unusable id. This is the single-ref form, for the predicates that answer a + yes/no about a member taken straight off a persisted group that nothing has + normalized yet. + + An id-less object collapses to ``None``, which :func:`_member_keys` then + resolves to nothing — canonicalizing must not invent a member. + """ + if isinstance(member, dict): + member = member.get("id") + return _coerce_id(member) + + def member_in_id_space(member: object, ids: object) -> bool: """Whether *member* names anything in *ids*, under the shared lookup order. For the callers that need a yes/no rather than the resolved id — watch's whole-group reconciliation drop and the cache's skipped-node prune. Build *ids* with :func:`node_id_set`, which carries the same keys. + + *member* is canonicalized here rather than by each caller, because these + callers read members off a group that has NOT been through + ``_normalize_hyperedge_members``: watch's drop reads ``edge["nodes"]`` + verbatim out of the persisted graph. Both shapes the feature tolerates + therefore arrived raw, and an unwrapped object member is unhashable, so it + named nothing and watch deleted a group every one of whose members is alive + — while an uncoerced numeric member missed the coerced key + :func:`node_id_set` holds, leaving the cache's prune under-pruning. Callers + that pre-coerce are unaffected: :func:`canonical_member_ref` is idempotent. """ - return any(key in ids for key in _member_keys(member)) + return any(key in ids for key in _member_keys(canonical_member_ref(member))) diff --git a/graphify/cache.py b/graphify/cache.py index 9e45fc2756..94d214c4fd 100644 --- a/graphify/cache.py +++ b/graphify/cache.py @@ -1825,11 +1825,13 @@ def hyperedge_dangles(h: dict) -> bool: intersection, for the same reason as the skipped-node prune above: an exact intersection misses a member that resolves everywhere else, leaving the group holding a reference to a node this - function has just removed. + function has just removed. Members go in as they were emitted — + ``member_in_id_space`` canonicalizes the ref, so the numeric and + object shapes resolve here as they do at every writer gate. """ try: return any( - member_in_id_space(_coerce_id(m), dropped_ids) + member_in_id_space(m, dropped_ids) for m in (h.get("nodes") or []) ) except TypeError: diff --git a/graphify/watch.py b/graphify/watch.py index d1c8ecbdbe..c79992c3dc 100644 --- a/graphify/watch.py +++ b/graphify/watch.py @@ -729,13 +729,6 @@ def _member_id_space(*node_lists: list) -> set: return space -def _coerce_member(value: object) -> object: - """Canonicalize one member ref the way the shared gate does.""" - from graphify.build import _coerce_id - - return _coerce_id(value) - - def _member_in_id_space(member: object, ids: object) -> bool: """Whether *member* names anything in *ids*, under the shared lookup order. @@ -1039,11 +1032,17 @@ def _ignored_now(identity: str) -> bool: # calls valid. The whole-group drop semantics are unchanged: any # member that resolves to nothing still evicts the group. # + # Members go in as persisted, in whatever shape they were written: + # `member_in_id_space` canonicalizes the ref itself, so an + # object-shaped `{"id": "a"}` names `a` here as it does everywhere + # else. Coercing only the numeric case on the way in left the object + # form unhashable, naming nothing, and evicted a live group. + # # Against its own key set, not `all_ids`: that set holds the node # ids raw for the edge-endpoint checks above, so a numeric node id # stays `7` there while its member coerces to `"7"`. if isinstance(members, list) and any( - not _member_in_id_space(_coerce_member(member), member_id_space) + not _member_in_id_space(member, member_id_space) for member in members ): continue diff --git a/tests/test_hypergraph.py b/tests/test_hypergraph.py index 2b2252ef87..461ac1f36e 100644 --- a/tests/test_hypergraph.py +++ b/tests/test_hypergraph.py @@ -13,6 +13,7 @@ canonical_hyperedge, gate_hyperedges, gate_hyperedges_against_graph, + member_in_id_space, node_id_set, ) from graphify.export import attach_hyperedges, to_json @@ -493,6 +494,50 @@ def test_gate_hyperedges_reports_the_drop_count_for_the_caller_message(): assert dropped == 1 +# --------------------------------------------------------------------------- +# 2d. member_in_id_space — the yes/no predicate canonicalizes its own argument +# --------------------------------------------------------------------------- + +MEMBER_SPACE = node_id_set([{"id": "foo_bar"}, {"id": 7}]) + + +@pytest.mark.parametrize( + "member", + ["foo_bar", {"id": "foo_bar"}, "Foo-Bar", {"id": "Foo-Bar"}], + ids=["bare", "object", "drifted-bare", "drifted-object"], +) +def test_member_in_id_space_resolves_a_string_member_in_either_shape(member): + """An object-shaped member names its `id`, exactly as it does everywhere else. + + `_normalize_hyperedge_members` flattens `{"id": "a"}` to `"a"`, but the + yes/no callers test members straight off a persisted group that has not + been through it — watch's reconciliation drop reads + `edge["nodes"]` verbatim. An unwrapped dict is unhashable, so it named + nothing, and watch DELETED a group whose members are all alive. + """ + assert member_in_id_space(member, MEMBER_SPACE) is True + + +@pytest.mark.parametrize("member", [7, "7", {"id": 7}, {"id": "7"}]) +def test_member_in_id_space_coerces_a_numeric_member(member): + """`node_id_set` keys are `_coerce_id`-coerced, so the member side has to be + too. The cache's skipped-node prune passes members raw, so a numeric one + missed the node it named and the group was cached referencing a node + deliberately not written.""" + assert member_in_id_space(member, MEMBER_SPACE) is True + + +@pytest.mark.parametrize( + "member", + ["ghost", {"id": "ghost"}, {"id": None}, {"label": "no id"}, {}, None, + ["unhashable"]], +) +def test_member_in_id_space_rejects_what_names_nothing(member): + """Canonicalizing must not invent a resolution: a member with no usable id, + in either shape, still names nothing.""" + assert member_in_id_space(member, MEMBER_SPACE) is False + + # --------------------------------------------------------------------------- # 3. to_json includes hyperedges key # --------------------------------------------------------------------------- diff --git a/tests/test_watch.py b/tests/test_watch.py index 53409b5d23..8ccfa6e1c4 100644 --- a/tests/test_watch.py +++ b/tests/test_watch.py @@ -758,6 +758,47 @@ def _add_unrelated_semantic_pair(graph_path): graph_path.write_text(json.dumps(data), encoding="utf-8") +def test_rebuild_code_preserves_a_hyperedge_whose_members_are_object_shaped( + tmp_path, +): + """An object-shaped member names its `id`; reconciliation must resolve it. + + Members come in both shapes the rest of the feature tolerates — a bare id + or an object carrying one (`_coerce_hyperedge_member_refs` exists precisely + because backends emit `{"id": "a"}`). Watch's whole-group drop tested the + member ref without unwrapping it, so an unhashable dict resolved to + nothing, "any member names nothing" fired, and a group whose members are + all alive was DELETED on an unrelated rebuild. + """ + from graphify.watch import _rebuild_code + + corpus = tmp_path / "corpus" + corpus.mkdir() + (corpus / "doc.md").write_text( + "# Design\n\n## Flow\n\nDetails.\n", encoding="utf-8" + ) + + assert _rebuild_code(corpus, no_cluster=True, acquire_lock=False) is True + graph_path = corpus / "graphify-out" / "graph.json" + data = json.loads(graph_path.read_text(encoding="utf-8")) + members = ["doc", "doc_design", "doc_flow"] + assert set(members) <= {node["id"] for node in data["nodes"]} + data["hyperedges"] = [{ + "id": "doc_flow_group", + "label": "Doc flow group", + "nodes": [{"id": mid} for mid in members], + "source_file": "doc.md", + }] + graph_path.write_text(json.dumps(data), encoding="utf-8") + + assert _rebuild_code(corpus, no_cluster=True, acquire_lock=False) is True + + after = json.loads(graph_path.read_text(encoding="utf-8")) + kept = {he["id"]: he for he in after.get("hyperedges", [])} + assert "doc_flow_group" in kept, "every member is alive; the group must survive" + assert kept["doc_flow_group"]["nodes"] == members + + @pytest.mark.parametrize( "changed_paths", [None, [Path("doc.md")]],