diff --git a/graphify/build.py b/graphify/build.py index bb03fe1f5..339aa01c1 100644 --- a/graphify/build.py +++ b/graphify/build.py @@ -40,6 +40,301 @@ # 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 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(canonical_member_ref(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 +405,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 +449,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 +458,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 +1608,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 +1630,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 +1648,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 +1729,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 +2355,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 +2473,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 622ba6cff..94d214c4f 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,55 @@ 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. 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 bool(dropped_ids & set(h.get("nodes") or [])) + return any( + member_in_id_space(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 6642f57d5..797e52b0b 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 4816f103b..1f719da33 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,90 @@ def _report_id_collision(nid: str, survivor: dict, losers: list[dict]) -> None: # ── main entry point ────────────────────────────────────────────────────────── +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 + 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. + + 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: dict[int, 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[id(he)] = seen + return originals + + +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; + 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, raws: dict) -> object: + """The node id member *key* should carry, preferring its own raw form.""" + own = raws.get(key) + if own is not None and own in raw_ids: + return own + return raw_by_coerced.get(key) + + 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")), raws) + restored.append(dict(m, id=raw) if raw is not None else m) + else: + raw = resolved(m, raws) + 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 +563,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 +648,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 +740,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 +955,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 +971,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 +992,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 69136befc..652d14bae 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 09cac2d84..6b089df69 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 fbcbbc011..c79992c3d 100644 --- a/graphify/watch.py +++ b/graphify/watch.py @@ -714,6 +714,51 @@ 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 _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 +1015,36 @@ 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. + # + # 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(member, member_id_space) + for member in members + ): continue preserved_hyperedges.append(edge) @@ -1775,10 +1843,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 b376b173b..7d956d3d7 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 2e341c3b8..f8d8291c7 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 f01a1cd29..ed67ade68 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 d5b4d1047..f7d54d620 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 446eb5ef7..983954103 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,179 @@ 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}" + ) + + + +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" + ) diff --git a/tests/test_export.py b/tests/test_export.py index d957b8795..48258244b 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 4c9fb445f..0675f7b7a 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 678bfa5d0..76571a321 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 6f0e9a8ef..fd638bbb2 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 a5208095a..461ac1f36 100644 --- a/tests/test_hypergraph.py +++ b/tests/test_hypergraph.py @@ -7,7 +7,15 @@ 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, + member_in_id_space, + node_id_set, +) from graphify.export import attach_hyperedges, to_json from graphify.report import generate @@ -72,13 +80,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 +119,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 +137,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 +148,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 +177,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 +189,361 @@ 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 + + +# --------------------------------------------------------------------------- +# 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 # --------------------------------------------------------------------------- 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 +556,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 +596,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 +607,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 +616,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 +631,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 +639,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 +668,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 +679,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 +704,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 +722,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 643e29e0a..2a1fbe81f 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 f6f1d6c46..673605d74 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 de13bd245..a146cda22 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 a189446b6..8ccfa6e1c 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,10 +753,51 @@ 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") + + +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", @@ -771,11 +819,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 +844,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 +3108,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."""