diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d818561b..06aba0844 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ Full release notes with details on each version: [GitHub Releases](https://github.com/safishamsi/graphify/releases) +## Unreleased + +- Fix: `--graph=PATH` is now honored by `query`/`path`/`explain`, not just `affected`. The three surfaces parsed only the space-separated `--graph PATH` form, so the `=` form was silently dropped and the user queried the default graph with no warning. All four surfaces now share one option parser. +- Fix: a valueless `--graph` on `query`/`path`/`explain`/`affected` now exits 2 with an actionable message. A trailing flag was silently ignored by all four commands; the empty `--graph=` form was ignored by three and reached a less-useful file-type error in `affected`. +- Fix: `graphify watch` / `update` rebuilds now pass the project root through to the graph builder, so absolute `source_file` paths from semantic fragments are relativized the same way `graphify build` does instead of persisting machine-absolute paths (#932). +- Fix: pruning a stale source file now also drops hyperedges that reference a removed node, not just hyperedges owned by the stale file — a hyperedge can no longer outlive its members. +- Fix: community detection now repairs a non-numeric, NaN, infinite, or negative edge `weight` to 1.0 before it reaches Leiden/Louvain, matching the normalization the graph builder already applies; hand-edited or LLM-produced graph.json files loaded from disk bypassed that path. + ## 0.9.53 (2026-08-30) - Fix: a batch of cross-language inheritance-edge corrections (thanks @Synvoya): JavaScript `class X extends Y` now emits an `inherits` edge (#1790); PHP interfaces, enums, and traits are captured as class-like nodes with their heritage (#1791); Scala `trait` declarations become class-like nodes (#1792) and qualified `extends`/`with` bases resolve to the tail type (#1794); a qualified Kotlin supertype resolves to its tail type instead of the package head (#1793); a C# interface extending an interface is classified as `inherits`, not `implements` (#1817); and a Go interface type-set constraint no longer emits a spurious `embeds` edge (#1818). diff --git a/graphify/build.py b/graphify/build.py index bb03fe1f5..036f4544b 100644 --- a/graphify/build.py +++ b/graphify/build.py @@ -2148,3 +2148,150 @@ def prune_repo_from_graph(G: nx.Graph, repo_tag: str) -> int: to_remove = [n for n, d in G.nodes(data=True) if d.get("repo") == repo_tag] G.remove_nodes_from(to_remove) return len(to_remove) + + +def load_graph_json( + path: Path, + *, + preserve_type: bool = False, + directed: bool = False, + preserve_direction: bool = False, +) -> nx.Graph: + """Load persisted node-link JSON, optionally preserving its graph type. + + Shared by merge-graphs and the global graph. Applies the graph-file size + cap, normalizes the legacy ``edges`` key to ``links`` (#738). By default + directed/multi inputs are coerced to a simple Graph for established + callers; type-preserving composition uses ``preserve_type``. + + directed=True loads the stored source/target order into a directed graph. + Persisted simple graphs say ``"directed": false`` even though their edge + order is meaningful (export restores it from _src/_tgt and pops the + attrs), so an undirected round-trip re-emits endpoints by node insertion + order and silently flips caller/callee — the #760 failure mode. Callers + that re-serialize a composed graph must load members directed. + + preserve_direction=True keeps the graph undirected but stashes the stored + endpoints on each edge as ``_src``/``_tgt`` first, so direction survives + the round-trip for callers that must compose into an undirected graph and + cannot switch type (#2261, merge-graphs). Mirrors export.py's marker + convention; use ``directed`` instead when the caller can hold a DiGraph. + """ + from networkx.readwrite import json_graph as _jg + from .security import check_graph_file_size_cap + + try: + check_graph_file_size_cap(path) + data = json.loads(path.read_text(encoding="utf-8")) + if not isinstance(data, dict): + raise ValueError("expected a mapping at the top level") + + nodes = data.get("nodes") + if not isinstance(nodes, list): + raise ValueError("'nodes' must be a list") + for i, node in enumerate(nodes): + if not isinstance(node, dict): + raise ValueError(f"nodes[{i}] must be a mapping") + if "id" not in node: + raise ValueError(f"nodes[{i}] is missing required 'id'") + try: + hash(node["id"]) + except TypeError as exc: + raise ValueError(f"nodes[{i}].id must be hashable") from exc + + links_key = "links" if "links" in data else "edges" if "edges" in data else "" + if not links_key: + raise ValueError("expected a 'links' or legacy 'edges' list") + links = data[links_key] + if not isinstance(links, list): + raise ValueError(f"'{links_key}' must be a list") + for i, link in enumerate(links): + if not isinstance(link, dict): + raise ValueError(f"{links_key}[{i}] must be a mapping") + for endpoint in ("source", "target"): + if endpoint not in link: + raise ValueError( + f"{links_key}[{i}] is missing required '{endpoint}'" + ) + try: + hash(link[endpoint]) + except TypeError as exc: + raise ValueError( + f"{links_key}[{i}].{endpoint} must be hashable" + ) from exc + + if links_key == "edges": + data = dict(data, links=links) + if preserve_direction: + # Keep in-file markers when present (#2309): unconditionally + # overwriting them with source/target would clobber the true + # direction of a link persisted in flipped endpoint order. + data = dict( + data, + links=[ + { + **link, + "_src": link.get("_src", link.get("source")), + "_tgt": link.get("_tgt", link.get("target")), + } + for link in links + ], + ) + if directed: + data = dict(data, directed=True) + try: + G = _jg.node_link_graph(data, edges="links") + except TypeError: + G = _jg.node_link_graph(data) + # node_link_graph restores only the nested `graph.hyperedges` slot; a + # graph.json whose hyperedges live only at the top level (the other + # half of to_json's dual-slot shape, #2485) would silently lose them + # here. Fall back to the top-level key (#2484). + if "hyperedges" not in G.graph and isinstance(data.get("hyperedges"), list): + G.graph["hyperedges"] = data["hyperedges"] + except ( + OSError, + ValueError, + KeyError, + TypeError, + AttributeError, + nx.NetworkXException, + ) as exc: + raise ValueError(f"cannot load graph {path}: {exc}") from exc + + simple_type = nx.DiGraph if directed else nx.Graph + if not preserve_type and type(G) is not simple_type: + G = simple_type(G) + return G + + +def merge_prefixed_into(G: nx.Graph, prefixed: nx.Graph) -> int: + """Merge a repo_tag::-prefixed graph into G in-place. Returns nodes added. + + External-library nodes (no ``source_file``) are deduplicated by label + against G's existing externals, with incident edges rewired onto the + shared node instead of dropped — the one place cross-repo identity is + established. Self-loops introduced by the rewiring are skipped. + """ + external_labels = { + d.get("label", ""): n + for n, d in G.nodes(data=True) + if not d.get("source_file") and d.get("label") + } + # Map each deduplicated external onto the existing node so that edges + # incident to it can be rewired instead of dropped. + remap = {} + for node, data in prefixed.nodes(data=True): + if not data.get("source_file") and data.get("label") in external_labels: + remap[node] = external_labels[data["label"]] + + for node, data in prefixed.nodes(data=True): + if node not in remap: + G.add_node(node, **data) + for u, v, data in prefixed.edges(data=True): + u = remap.get(u, u) + v = remap.get(v, v) + if u != v: # don't introduce self-loops via remapping + G.add_edge(u, v, **data) + + return prefixed.number_of_nodes() - len(remap) diff --git a/graphify/cli.py b/graphify/cli.py index 6642f57d5..d9a829d69 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -85,6 +85,54 @@ def _default_graph_path() -> str: return str(Path(_GRAPHIFY_OUT) / "graph.json") +def _parse_graph_option(args: list[str]) -> "tuple[str, bool, list[str]]": + """Strip the --graph selection out of ``args``. + + Returns ``(graph_path, graph_given, remaining)``. ``remaining`` keeps every + other token in order so each command can run its own flag loop over it + (``--budget``/``--context``, ``--depth``/``--relation``, + ``--directed``/``--undirected``). + + query/path/explain/affected each grew their own copy of this parsing in two + different styles, and only ``affected`` ever handled the ``--graph=PATH`` + form — the others silently dropped the token, so an explicitly selected + graph was ignored and the user queried the default graph with no warning. + A trailing valueless ``--graph`` was likewise silently dropped by all four + commands. The empty ``--graph=`` form was ignored by three commands and + reached a less-useful file-type error in ``affected``. Both now exit 2 at + parse time. One parser keeps the four surfaces honest. + + ``graph_given`` is unused by the callers today but is part of the contract: + a later ``--cluster`` option needs it for its mutual-exclusion check. + """ + graph_path = _default_graph_path() + graph_given = False + remaining: list[str] = [] + i = 0 + while i < len(args): + arg = args[i] + value: "str | None" = None + if arg == "--graph": + if i + 1 < len(args): + value = args[i + 1] + i += 2 + else: + i += 1 + elif arg.startswith("--graph="): + value = arg.split("=", 1)[1] + i += 1 + else: + remaining.append(arg) + i += 1 + continue + if not value: + print("error: --graph requires a path", file=sys.stderr) + sys.exit(2) + graph_path = value + graph_given = True + return graph_path, graph_given, remaining + + def _stamped_manifest_files( files_by_type: dict[str, list[str]], sem_result: dict, @@ -580,6 +628,51 @@ def _zero_node_stamped_semantic_sources( return healed +def _filter_payload_sources(data: dict, stale: set) -> int: + """Drop nodes/edges/hyperedges owned by ``stale`` source spellings from a + raw graph payload IN MEMORY, mutating ``data``. Both serialized hyperedge + slots are filtered. Returns nodes removed. + + Exact string matching against ``source_file`` — callers pass spellings the + graph itself uses (or every plausible spelling of a path). + """ + links_key = "links" if "links" in data else "edges" + nodes = [n for n in data.get("nodes", []) if isinstance(n, dict)] + kept_nodes = [n for n in nodes if n.get("source_file") not in stale] + removed_ids = { + n.get("id") for n in nodes if n.get("source_file") in stale + } + n_removed = len(nodes) - len(kept_nodes) + data["nodes"] = kept_nodes + data[links_key] = [ + e for e in data.get(links_key, []) + if isinstance(e, dict) + and e.get("source_file") not in stale + and e.get("source") not in removed_ids + and e.get("target") not in removed_ids + ] + + def _kept_hyperedges(items: list) -> list: + return [ + h for h in items + if isinstance(h, dict) + and h.get("source_file") not in stale + and not ( + isinstance(h.get("nodes"), list) + and any(member in removed_ids for member in h["nodes"]) + ) + ] + + if "hyperedges" in data: + data["hyperedges"] = _kept_hyperedges(data.get("hyperedges", [])) + graph_meta = data.get("graph") + if isinstance(graph_meta, dict) and "hyperedges" in graph_meta: + graph_meta["hyperedges"] = _kept_hyperedges( + graph_meta.get("hyperedges", []) + ) + return n_removed + + 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. @@ -597,33 +690,29 @@ def _prune_graph_json_sources(graph_path: Path, stale_sources: list[str]) -> int return 0 if not isinstance(data, dict): return 0 - stale = set(stale_sources) links_key = "links" if "links" in data else "edges" - nodes = [n for n in data.get("nodes", []) if isinstance(n, dict)] - kept_nodes = [n for n in nodes if n.get("source_file") not in stale] - removed_ids = { - n.get("id") for n in nodes if n.get("source_file") in stale - } - n_removed = len(nodes) - len(kept_nodes) - kept_edges = [ - e for e in data.get(links_key, []) - if isinstance(e, dict) - and e.get("source_file") not in stale - 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", [])) + n_edges_before = len(data.get(links_key, [])) + n_hyper_before = len(data.get("hyperedges", [])) + graph_meta = data.get("graph") + n_nested_hyper_before = ( + len(graph_meta.get("hyperedges", [])) + if isinstance(graph_meta, dict) + else 0 + ) + n_removed = _filter_payload_sources(data, set(stale_sources)) + graph_meta = data.get("graph") + n_nested_hyper_after = ( + len(graph_meta.get("hyperedges", [])) + if isinstance(graph_meta, dict) + else 0 + ) + if ( + n_removed == 0 + and len(data.get(links_key, [])) == n_edges_before + and len(data.get("hyperedges", [])) == n_hyper_before + and n_nested_hyper_after == n_nested_hyper_before ): return 0 - data["nodes"] = kept_nodes - data[links_key] = kept_edges - if "hyperedges" in data: - data["hyperedges"] = kept_hyper from graphify.export import backup_if_protected as _backup _backup(graph_path.parent) from graphify.paths import write_json_atomic @@ -1211,9 +1300,8 @@ def dispatch_command(cmd: str) -> None: question = sys.argv[2] use_dfs = "--dfs" in sys.argv budget = 2000 - graph_path = _default_graph_path() context_filters: list[str] = [] - args = sys.argv[3:] + graph_path, _graph_given, args = _parse_graph_option(sys.argv[3:]) i = 0 while i < len(args): if args[i] == "--budget" and i + 1 < len(args): @@ -1236,9 +1324,6 @@ def dispatch_command(cmd: str) -> None: elif args[i].startswith("--context="): context_filters.append(args[i].split("=", 1)[1]) i += 1 - elif args[i] == "--graph" and i + 1 < len(args): - graph_path = args[i + 1] - i += 2 else: i += 1 gp = Path(graph_path).resolve() @@ -1326,19 +1411,12 @@ def dispatch_command(cmd: str) -> None: sys.exit(1) from graphify.affected import DEFAULT_AFFECTED_RELATIONS, format_affected, load_graph query = sys.argv[2] - graph_path = _default_graph_path() depth = 2 relations: list[str] = [] - args = sys.argv[3:] + graph_path, _graph_given, args = _parse_graph_option(sys.argv[3:]) i = 0 while i < len(args): - if args[i] == "--graph" and i + 1 < len(args): - graph_path = args[i + 1] - i += 2 - elif args[i].startswith("--graph="): - graph_path = args[i].split("=", 1)[1] - i += 1 - elif args[i] == "--depth" and i + 1 < len(args): + if args[i] == "--depth" and i + 1 < len(args): try: depth = int(args[i + 1]) except ValueError: @@ -1545,13 +1623,10 @@ def dispatch_command(cmd: str) -> None: source_label = sys.argv[2] target_label = sys.argv[3] - graph_path = _default_graph_path() - args = sys.argv[4:] + graph_path, _graph_given, args = _parse_graph_option(sys.argv[4:]) direction_flag = None - for i, a in enumerate(args): - if a == "--graph" and i + 1 < len(args): - graph_path = args[i + 1] - elif a == "--directed": + for a in args: + if a == "--directed": if direction_flag == "undirected": print( "error: --directed and --undirected are mutually exclusive", @@ -1706,11 +1781,7 @@ def dispatch_command(cmd: str) -> None: from networkx.readwrite import json_graph label = sys.argv[2] - graph_path = _default_graph_path() - args = sys.argv[3:] - for i, a in enumerate(args): - if a == "--graph" and i + 1 < len(args): - graph_path = args[i + 1] + graph_path, _graph_given, _rest = _parse_graph_option(sys.argv[3:]) gp = Path(graph_path).resolve() if not gp.exists(): print(f"error: graph file not found: {gp}", file=sys.stderr) @@ -2606,45 +2677,29 @@ def _load_graph(p: str): sys.exit(1) import networkx as _nx from networkx.readwrite import json_graph as _jg - from graphify.build import prefix_graph_for_global as _prefix, distinct_repo_tags as _repo_tags + from graphify.build import ( + prefix_graph_for_global as _prefix, + distinct_repo_tags as _repo_tags, + load_graph_json as _load_graph, + ) graphs = [] for gp in graph_paths: if not gp.exists(): print(f"error: not found: {gp}", file=sys.stderr) sys.exit(1) - _enforce_graph_size_cap_or_exit(gp) - data = json.loads(gp.read_text(encoding="utf-8")) - # Normalize edges/links key before loading — graphify writes "links" - # via node_link_data but older runs may have used "edges" (#738). - if "links" not in data and "edges" in data: - data = dict(data, links=data["edges"]) - # Preserve stored edge direction across undirected node_link_graph (#2261). - # Mirrors cli.py's query pattern and export.py's _src/_tgt restoration. - # Keep in-file markers when present (#2309): unconditionally - # overwriting them with source/target would clobber the true - # direction of a link persisted in flipped endpoint order. - data = dict( - data, - links=[ - { - **link, - "_src": link.get("_src", link.get("source")), - "_tgt": link.get("_tgt", link.get("target")), - } - for link in data.get("links", []) - ], - ) + # load_graph_json enforces the size cap, normalizes the legacy + # "edges" key (#738), and coerces directed/multi inputs to a plain + # undirected Graph so nx.compose never sees mixed types (#1606). + # preserve_direction stashes the stored endpoints as _src/_tgt so + # the undirected round-trip can't flip caller/callee (#2261), + # keeping in-file markers when present (#2309) — the merged graph + # stays a plain Graph, as compose requires. Top-level-only + # hyperedges are restored onto G.graph there too (#2484/#2485). try: - G = _jg.node_link_graph(data, edges="links") - except TypeError: - G = _jg.node_link_graph(data) - # node_link_graph restores only the nested `graph.hyperedges` slot; - # a graph.json whose hyperedges live only at the top level (the - # other half of to_json's dual-slot shape, #2485) would silently - # lose them here. Fall back to the top-level key (#2484). - if "hyperedges" not in G.graph and isinstance(data.get("hyperedges"), list): - G.graph["hyperedges"] = data["hyperedges"] - graphs.append(G) + graphs.append(_load_graph(gp, preserve_direction=True)) + except ValueError as exc: + print(f"error: {exc}", file=sys.stderr) + sys.exit(1) # nx.compose requires all graphs to be the same type. When input graphs # come from different sources (e.g. an AST-only run vs a full LLM run) one # may be a MultiGraph and another a Graph. Normalise everything to Graph diff --git a/graphify/cluster.py b/graphify/cluster.py index 34952a44a..a2dcdeee3 100644 --- a/graphify/cluster.py +++ b/graphify/cluster.py @@ -4,6 +4,7 @@ import inspect import io import json +import math import sys import networkx as nx @@ -127,7 +128,21 @@ def _partition(G: nx.Graph, resolution: float = 1.0) -> dict[str, int]: ), ) for src, tgt, attrs in edge_rows: - stable.add_edge(src, tgt, **attrs) + # The partitioners consume `weight` unvalidated: a NaN/inf, negative, + # or non-numeric value in a hand-edited or LLM-produced graph.json + # reaches Leiden/Louvain as-is. Repair to 1.0 — the same contract + # build_from_json enforces when it normalizes edge attrs, but graphs + # loaded from disk bypass that path. + weight = attrs.get("weight", 1.0) + try: + weight = float(weight) + if not math.isfinite(weight) or weight < 0: + raise ValueError("edge weight must be finite and non-negative") + except (TypeError, ValueError): + weight = 1.0 + projected = dict(attrs) + projected["weight"] = weight + stable.add_edge(src, tgt, **projected) native_result = _native_leiden(stable, resolution) if native_result is not None: diff --git a/graphify/global_graph.py b/graphify/global_graph.py index eddd0c92a..8f8f7582c 100644 --- a/graphify/global_graph.py +++ b/graphify/global_graph.py @@ -48,15 +48,8 @@ def _save_manifest(manifest: dict) -> None: def _load_global_graph() -> nx.Graph: if _GLOBAL_GRAPH.exists(): - from graphify.security import check_graph_file_size_cap - check_graph_file_size_cap(_GLOBAL_GRAPH) - data = json.loads(_GLOBAL_GRAPH.read_text(encoding="utf-8")) - if "links" not in data and "edges" in data: - data = dict(data, links=data["edges"]) - try: - return _jg.node_link_graph(data, edges="links") - except TypeError: - return _jg.node_link_graph(data) + from graphify.build import load_graph_json + return load_graph_json(_GLOBAL_GRAPH, preserve_type=True) return nx.Graph() @@ -82,7 +75,12 @@ def global_add(source_path: Path, repo_tag: str) -> dict: Returns a summary dict with keys: repo_tag, nodes_added, nodes_removed, skipped. Skipped=True means the source graph hasn't changed since last add. """ - from graphify.build import prefix_graph_for_global, prune_repo_from_graph + from graphify.build import ( + load_graph_json, + merge_prefixed_into, + prefix_graph_for_global, + prune_repo_from_graph, + ) if not source_path.exists(): raise FileNotFoundError(f"graph not found: {source_path}") @@ -102,48 +100,15 @@ def global_add(source_path: Path, repo_tag: str) -> dict: if existing.get("source_hash") == src_hash: return {"repo_tag": repo_tag, "nodes_added": 0, "nodes_removed": 0, "skipped": True} - # Load source graph - from graphify.security import check_graph_file_size_cap - check_graph_file_size_cap(source_path) - data = json.loads(source_path.read_text(encoding="utf-8")) - if "links" not in data and "edges" in data: - data = dict(data, links=data["edges"]) - try: - src_G = _jg.node_link_graph(data, edges="links") - except TypeError: - src_G = _jg.node_link_graph(data) - - # Prefix IDs for cross-project isolation + # Load source graph, prefix IDs for cross-project isolation + src_G = load_graph_json(source_path, preserve_type=True) prefixed = prefix_graph_for_global(src_G, repo_tag) - # Load global graph and prune stale nodes for this repo + # Load global graph, prune stale nodes for this repo, merge with + # external-library dedup-by-label (shared helper in build.py). G = _load_global_graph() removed = prune_repo_from_graph(G, repo_tag) - - # Merge external-library nodes (no source_file) by label to avoid duplication - external_labels = { - d.get("label", ""): n - for n, d in G.nodes(data=True) - if not d.get("source_file") and d.get("label") - } - # Map each deduplicated external onto the existing global node so that - # edges incident to it can be rewired instead of dropped. - remap = {} - for node, data in prefixed.nodes(data=True): - if not data.get("source_file") and data.get("label") in external_labels: - remap[node] = external_labels[data["label"]] - - # Compose: add prefixed nodes (except deduplicated externals) into global graph - for node, data in prefixed.nodes(data=True): - if node not in remap: - G.add_node(node, **data) - for u, v, data in prefixed.edges(data=True): - u = remap.get(u, u) - v = remap.get(v, v) - if u != v: # don't introduce self-loops via remapping - G.add_edge(u, v, **data) - - added = prefixed.number_of_nodes() - len(remap) + added = merge_prefixed_into(G, prefixed) _save_global_graph(G) manifest["repos"][repo_tag] = { diff --git a/graphify/watch.py b/graphify/watch.py index fbcbbc011..09416fd69 100644 --- a/graphify/watch.py +++ b/graphify/watch.py @@ -1870,8 +1870,16 @@ def _failed(f: str) -> bool: # Inherit the existing graph's directed flag (#2342) so `graphify # update` can't silently downgrade a directed graph to undirected - - # build_from_json defaults to directed=False otherwise. - G = build_from_json(result, directed=bool((existing_graph_data or {}).get("directed", False))) + # build_from_json defaults to directed=False otherwise. Pass the + # project root so absolute source_file paths from semantic fragments + # are relativized the same way `graphify build` does (#932) — without + # it a watch rebuild writes machine-absolute paths that break sharing + # and path-based selectors. + G = build_from_json( + result, + directed=bool((existing_graph_data or {}).get("directed", False)), + root=project_root, + ) candidate_topology = _topology_from_graph(G) if existing_graph_data: try: diff --git a/tests/test_cluster.py b/tests/test_cluster.py index fa4d0a265..08dfd68d6 100644 --- a/tests/test_cluster.py +++ b/tests/test_cluster.py @@ -1,9 +1,11 @@ import json +import math import sys import networkx as nx +import pytest from pathlib import Path from graphify.build import build_from_json -from graphify.cluster import cluster, cohesion_score, remap_communities_to_previous, score_all +from graphify.cluster import _partition, cluster, cohesion_score, remap_communities_to_previous, score_all FIXTURES = Path(__file__).parent / "fixtures" @@ -177,3 +179,53 @@ def build(order, flip): assert _grouping(_partition(forward, 1.0)) == _grouping(_partition(flipped, 1.0)), ( "partition drifted with edge-endpoint orientation / insertion order" ) + + +@pytest.mark.parametrize( + "weight", [math.nan, math.inf, -math.inf, -1.0, "abc", None, [1]] +) +def test_partition_repairs_invalid_edge_weights(monkeypatch, weight): + """The partitioners consume `weight` unvalidated: a non-numeric, NaN, + infinite, or negative value in a hand-edited or LLM-produced graph.json + reached Leiden/Louvain as-is. _partition now repairs each to 1.0 — the + same contract build_from_json enforces on edge attrs, which graphs loaded + from disk bypass.""" + graph = nx.Graph() + graph.add_edge("a", "b", weight=weight) + captured = {} + + import graphify.cluster as cl + # Both Leiden paths must be out of the way for the Louvain fallback to run: + # _native_leiden (graspologic_native, tried first) and the graspologic wrapper. + monkeypatch.setattr(cl, "_native_leiden", lambda *a, **k: None) + monkeypatch.setitem(sys.modules, "graspologic.partition", None) + + def fake_louvain(projected, **_kwargs): + captured["weight"] = projected["a"]["b"]["weight"] + return [{"a", "b"}] + + monkeypatch.setattr(nx.community, "louvain_communities", fake_louvain) + + assert _partition(graph) == {"a": 0, "b": 0} + assert captured["weight"] == 1.0, f"weight {weight!r} was not repaired" + + +def test_partition_keeps_valid_edge_weights(monkeypatch): + graph = nx.Graph() + graph.add_edge("a", "b", weight=2.5) + captured = {} + + import graphify.cluster as cl + # Both Leiden paths must be out of the way for the Louvain fallback to run: + # _native_leiden (graspologic_native, tried first) and the graspologic wrapper. + monkeypatch.setattr(cl, "_native_leiden", lambda *a, **k: None) + monkeypatch.setitem(sys.modules, "graspologic.partition", None) + + def fake_louvain(projected, **_kwargs): + captured["weight"] = projected["a"]["b"]["weight"] + return [{"a", "b"}] + + monkeypatch.setattr(nx.community, "louvain_communities", fake_louvain) + + _partition(graph) + assert captured["weight"] == 2.5, "a valid weight must pass through unchanged" diff --git a/tests/test_extract_code_only_cli.py b/tests/test_extract_code_only_cli.py index 2776b2aa4..3cc09cb74 100644 --- a/tests/test_extract_code_only_cli.py +++ b/tests/test_extract_code_only_cli.py @@ -431,3 +431,67 @@ def test_code_only_force_rescan_re_resolves_tsconfig_paths_and_preserves_semanti assert "doc_arch" in nodes2, ( "existing semantic nodes must survive --code-only --force (#2923/#3125)" ) + + +def test_filter_payload_sources_drops_hyperedges_with_removed_members(): + """A hyperedge must not outlive its members: pruning a stale source used + to drop only hyperedges whose own source_file was stale, leaving danglers + that referenced removed node ids.""" + from graphify.cli import _filter_payload_sources + + payload = { + "nodes": [ + {"id": "removed", "source_file": "stale.py"}, + {"id": "kept", "source_file": "live.py"}, + ], + "links": [], + "hyperedges": [ + { + "id": "dangling", + "nodes": ["removed", "kept"], + "source_file": "live.py", + }, + { + "id": "owned-by-stale", + "nodes": ["kept"], + "source_file": "stale.py", + }, + {"id": "kept", "nodes": ["kept"], "source_file": "live.py"}, + "malformed", + ], + } + payload["graph"] = {"hyperedges": list(payload["hyperedges"])} + + assert _filter_payload_sources(payload, {"stale.py"}) == 1 + assert payload["nodes"] == [{"id": "kept", "source_file": "live.py"}] + expected = [{"id": "kept", "nodes": ["kept"], "source_file": "live.py"}] + assert payload["hyperedges"] == expected + assert payload["graph"]["hyperedges"] == expected + + +def test_prune_graph_json_sources_writes_nested_only_hyperedge_removal(tmp_path): + """A nested-only hyperedge change must bypass the no-change early return.""" + from graphify.cli import _prune_graph_json_sources + + graph_path = tmp_path / "graph.json" + payload = { + "nodes": [{"id": "kept", "source_file": "live.py"}], + "links": [], + "graph": { + "hyperedges": [ + { + "id": "stale", + "nodes": ["kept"], + "source_file": "stale.py", + } + ], + }, + } + graph_path.write_text( + json.dumps(payload), + encoding="utf-8", + ) + + assert _prune_graph_json_sources(graph_path, ["stale.py"]) == 0 + payload = json.loads(graph_path.read_text(encoding="utf-8")) + assert payload["graph"]["hyperedges"] == [] diff --git a/tests/test_graph_option_cli.py b/tests/test_graph_option_cli.py new file mode 100644 index 000000000..a326ddf30 --- /dev/null +++ b/tests/test_graph_option_cli.py @@ -0,0 +1,120 @@ +"""--graph option parsing across query / affected / path / explain. + +The four commands each grew their own copy of the option loop, in two different +parse styles, and only `affected` ever handled the `--graph=PATH` form — the +others silently dropped the token, so an explicitly selected graph was ignored +and the user queried the default graph with no warning. A trailing valueless +`--graph` was silently dropped by all four, same bug class. One shared +pre-pass (`cli._parse_graph_option`) keeps the four surfaces honest. + +The `--graph PATH` cases are characterization: they passed before the shared +parser and must keep passing after it. The `--graph=PATH` and valueless cases +pin the fix. +""" +from __future__ import annotations + +import json + +import networkx as nx +import pytest +from networkx.readwrite import json_graph + +import graphify.__main__ as mainmod + + +def _write_graph(tmp_path): + """A graph distinctive enough that output proves WHICH graph was loaded: + the default graphify-out/graph.json does not exist in tmp cwd, so loading + the explicit graph is the only way these labels can appear.""" + graph = nx.DiGraph() + graph.add_node("alpha", label="AlphaFn", source_file="alpha.py", source_location="L1") + graph.add_node("beta", label="BetaFn", source_file="beta.py", source_location="L2") + graph.add_edge("alpha", "beta", relation="calls", context="call", confidence="EXTRACTED") + graph_path = tmp_path / "somewhere" / "graph.json" + graph_path.parent.mkdir() + graph_path.write_text( + json.dumps(json_graph.node_link_data(graph, edges="links")), encoding="utf-8" + ) + return graph_path + + +# argv builders: (name, positionals-before-options) +_COMMANDS = { + "query": ["query", "AlphaFn"], + "affected": ["affected", "BetaFn"], + "path": ["path", "AlphaFn", "BetaFn"], + "explain": ["explain", "AlphaFn"], +} + +# A string that only appears when the explicit graph actually loaded. +_PROOF = { + "query": "AlphaFn", + "affected": "AlphaFn", # affected BetaFn reports the caller AlphaFn + "path": "AlphaFn", + "explain": "AlphaFn", +} + + +def _run(monkeypatch, tmp_path, argv): + monkeypatch.chdir(tmp_path) # default graphify-out/graph.json cannot exist + monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) + monkeypatch.setattr(mainmod.sys, "argv", ["graphify", *argv]) + mainmod.main() + + +@pytest.mark.parametrize("command", sorted(_COMMANDS)) +def test_graph_space_form_honored(command, monkeypatch, tmp_path, capsys): + """Characterization: the space-separated form worked before the shared + parser and must keep working after it.""" + graph_path = _write_graph(tmp_path) + _run(monkeypatch, tmp_path, [*_COMMANDS[command], "--graph", str(graph_path)]) + out = capsys.readouterr().out + assert _PROOF[command] in out + + +@pytest.mark.parametrize("command", sorted(_COMMANDS)) +def test_graph_equals_form_honored(command, monkeypatch, tmp_path, capsys): + """The `=` form was silently swallowed by query/path/explain (only + affected parsed it), leaving the user on the default graph unannounced.""" + graph_path = _write_graph(tmp_path) + _run(monkeypatch, tmp_path, [*_COMMANDS[command], f"--graph={graph_path}"]) + out = capsys.readouterr().out + assert _PROOF[command] in out + + +@pytest.mark.parametrize("command", sorted(_COMMANDS)) +def test_trailing_valueless_graph_errors(command, monkeypatch, tmp_path, capsys): + """A trailing `--graph` with no value used to be silently dropped — the + same silent-selection-loss class as the `=` form. Now exit 2 with an + actionable message.""" + _write_graph(tmp_path) + with pytest.raises(SystemExit) as excinfo: + _run(monkeypatch, tmp_path, [*_COMMANDS[command], "--graph"]) + assert excinfo.value.code == 2 + assert "--graph requires a path" in capsys.readouterr().err + + +@pytest.mark.parametrize("command", sorted(_COMMANDS)) +def test_graph_equals_empty_errors(command, monkeypatch, tmp_path, capsys): + """`--graph=` (empty value) gets the same rejection as a valueless + `--graph`. An empty path resolves to the cwd — a directory — which only + two of the four commands guard with a .json suffix check; path/explain + would crash reading it. Rejecting at parse time keeps all four uniform.""" + _write_graph(tmp_path) + with pytest.raises(SystemExit) as excinfo: + _run(monkeypatch, tmp_path, [*_COMMANDS[command], "--graph="]) + assert excinfo.value.code == 2 + assert "--graph requires a path" in capsys.readouterr().err + + +def test_own_flags_still_parsed_after_pre_pass(monkeypatch, tmp_path, capsys): + """The pre-pass strips only graph tokens; each command's own flag loop + still sees its flags (query --budget here as the representative).""" + graph_path = _write_graph(tmp_path) + _run( + monkeypatch, + tmp_path, + ["query", "AlphaFn", "--budget", "50", f"--graph={graph_path}"], + ) + out = capsys.readouterr().out + assert "AlphaFn" in out diff --git a/tests/test_load_graph_json.py b/tests/test_load_graph_json.py new file mode 100644 index 000000000..88241ee6f --- /dev/null +++ b/tests/test_load_graph_json.py @@ -0,0 +1,227 @@ +"""Direct tests for build.load_graph_json — the shared node-link loader. + +merge-graphs, and the global graph each grew their own copy of "load node-link +JSON, normalize the legacy key, stash direction markers", and each copy carried +a different subset of the #738/#2261/#2309/#2484 fixes. The shared loader is +covered transitively by those callers' tests; these pin its own contract — +especially the validation branches, which reject a malformed graph.json with a +ValueError instead of letting it propagate a confusing NetworkX error. +""" +import json + +import networkx as nx +import pytest + +from graphify.build import load_graph_json, merge_prefixed_into + + +def _write(tmp_path, payload): + p = tmp_path / "graph.json" + p.write_text(json.dumps(payload), encoding="utf-8") + return p + + +def _minimal(**overrides): + data = { + "directed": False, + "multigraph": False, + "graph": {}, + "nodes": [{"id": "a"}, {"id": "b"}], + "links": [{"source": "a", "target": "b", "relation": "calls"}], + } + data.update(overrides) + return data + + +# --- validation branches: every malformed shape becomes a ValueError --------- + +@pytest.mark.parametrize( + "payload", + [ + ["not", "a", "mapping"], + _minimal(nodes={"id": "a"}), + _minimal(nodes=[["not-a-dict"]]), + _minimal(nodes=[{"label": "no id"}]), + _minimal(nodes=[{"id": ["un", "hashable"]}]), + {"nodes": [{"id": "a"}]}, # neither links nor edges + _minimal(links={"source": "a"}), + _minimal(links=["not-a-dict"]), + _minimal(links=[{"target": "b"}]), # missing source + _minimal(links=[{"source": "a"}]), # missing target + _minimal(links=[{"source": ["un", "hashable"], "target": "b"}]), + ], + ids=[ + "top-level-not-mapping", + "nodes-not-list", + "node-not-mapping", + "node-missing-id", + "node-id-unhashable", + "no-links-or-edges", + "links-not-list", + "link-not-mapping", + "link-missing-source", + "link-missing-target", + "link-endpoint-unhashable", + ], +) +def test_malformed_graph_raises_value_error(tmp_path, payload): + path = _write(tmp_path, payload) + with pytest.raises(ValueError, match=r"cannot load graph"): + load_graph_json(path) + + +def test_missing_file_raises_value_error(tmp_path): + with pytest.raises(ValueError, match=r"cannot load graph"): + load_graph_json(tmp_path / "absent.json") + + +def test_invalid_json_raises_value_error(tmp_path): + p = tmp_path / "graph.json" + p.write_text("{not json", encoding="utf-8") + with pytest.raises(ValueError, match=r"cannot load graph"): + load_graph_json(p) + + +# --- happy paths -------------------------------------------------------------- + +def test_loads_canonical_links_spelling(tmp_path): + G = load_graph_json(_write(tmp_path, _minimal())) + assert set(G.nodes()) == {"a", "b"} + assert G.has_edge("a", "b") + + +def test_loads_legacy_edges_spelling(tmp_path): + """#738: older runs persisted the list under "edges" instead of "links".""" + data = _minimal() + data["edges"] = data.pop("links") + G = load_graph_json(_write(tmp_path, data)) + assert G.has_edge("a", "b") + assert G["a"]["b"]["relation"] == "calls" + + +def test_directed_returns_digraph_in_stored_order(tmp_path): + """#760: an undirected round-trip re-emits endpoints by node insertion + order; directed=True must preserve the stored caller→callee arc.""" + G = load_graph_json(_write(tmp_path, _minimal()), directed=True) + assert isinstance(G, nx.DiGraph) + assert G.has_edge("a", "b") + assert not G.has_edge("b", "a") + + +def test_preserve_direction_stashes_src_tgt_markers(tmp_path): + """#2261: the graph stays undirected, but each edge carries the stored + endpoints so a re-serialization can restore the true direction.""" + G = load_graph_json(_write(tmp_path, _minimal()), preserve_direction=True) + assert not G.is_directed() + d = G["a"]["b"] + assert d["_src"] == "a" + assert d["_tgt"] == "b" + + +def test_preserve_direction_keeps_existing_markers(tmp_path): + """#2309: a link persisted in flipped endpoint order carries its truth in + pre-existing _src/_tgt markers; the loader must not overwrite them.""" + data = _minimal( + links=[{"source": "b", "target": "a", "relation": "calls", + "_src": "a", "_tgt": "b"}], + ) + G = load_graph_json(_write(tmp_path, data), preserve_direction=True) + d = G["a"]["b"] + assert d["_src"] == "a", "pre-existing marker was clobbered by the arc tail" + assert d["_tgt"] == "b" + + +def test_top_level_hyperedges_restored(tmp_path): + """#2484: node_link_graph only restores the nested graph.hyperedges slot; a + file whose hyperedges live only at the top level must not lose them.""" + data = _minimal(hyperedges=[{"id": "h1", "nodes": ["a", "b"]}]) + G = load_graph_json(_write(tmp_path, data)) + assert G.graph.get("hyperedges") == [{"id": "h1", "nodes": ["a", "b"]}] + + +def test_nested_hyperedges_not_overwritten_by_top_level(tmp_path): + data = _minimal( + graph={"hyperedges": [{"id": "nested"}]}, + hyperedges=[{"id": "top-level"}], + ) + G = load_graph_json(_write(tmp_path, data)) + assert G.graph["hyperedges"] == [{"id": "nested"}] + + +def test_default_coerces_stored_digraph_to_simple_graph(tmp_path): + """Established callers (merge-graphs) compose into nx.Graph; a directed or + multi input must be coerced so nx.compose never sees mixed types (#1606).""" + G = load_graph_json(_write(tmp_path, _minimal(directed=True))) + assert type(G) is nx.Graph + + +def test_preserve_type_keeps_stored_digraph(tmp_path): + G = load_graph_json( + _write(tmp_path, _minimal(directed=True)), preserve_type=True + ) + assert isinstance(G, nx.DiGraph) + + +@pytest.mark.parametrize( + ("directed", "expected_type"), + [(False, nx.MultiGraph), (True, nx.MultiDiGraph)], +) +def test_preserve_type_keeps_keyed_parallel_edges(tmp_path, directed, expected_type): + data = _minimal( + multigraph=True, + links=[ + {"source": "a", "target": "b", "key": "calls", "relation": "calls"}, + { + "source": "a", + "target": "b", + "key": "references", + "relation": "references", + }, + ], + ) + G = load_graph_json( + _write(tmp_path, data), preserve_type=True, directed=directed + ) + assert type(G) is expected_type + assert set(G["a"]["b"]) == {"calls", "references"} + + +def test_size_cap_enforced(tmp_path, monkeypatch): + monkeypatch.setenv("GRAPHIFY_MAX_GRAPH_BYTES", "10") + path = _write(tmp_path, _minimal()) + with pytest.raises(ValueError, match=r"cannot load graph"): + load_graph_json(path) + + +# --- merge_prefixed_into ------------------------------------------------------- + +def _prefixed(tag: str, *, external_label: str = "requests") -> nx.Graph: + G = nx.Graph() + G.add_node(f"{tag}::app", label="app", source_file="app.py", repo=tag) + G.add_node(f"{tag}::ext_requests", label=external_label, repo=tag) # external: no source_file + G.add_edge(f"{tag}::app", f"{tag}::ext_requests", relation="imports") + return G + + +def test_merge_prefixed_into_dedups_externals_by_label(): + G = nx.Graph() + added_one = merge_prefixed_into(G, _prefixed("one")) + added_two = merge_prefixed_into(G, _prefixed("two")) + assert added_one == 2 + assert added_two == 1, "the shared external must dedup onto the existing node" + externals = [n for n, d in G.nodes(data=True) if not d.get("source_file")] + assert len(externals) == 1 + # Both repos' import edges were rewired onto the shared external. + assert G.degree(externals[0]) == 2 + + +def test_merge_prefixed_into_skips_self_loops_from_remap(): + G = nx.Graph() + G.add_node("ext", label="requests") # existing sourceless external + prefixed = nx.Graph() + prefixed.add_node("one::a", label="requests") # dedups onto "ext" + prefixed.add_node("one::b", label="requests2") + prefixed.add_edge("one::a", "one::b", relation="uses") + prefixed.add_edge("one::a", "one::a", relation="self") + merge_prefixed_into(G, prefixed) + assert not G.has_edge("ext", "ext"), "remapping must not introduce self-loops" diff --git a/tests/test_watch.py b/tests/test_watch.py index a189446b6..58b238973 100644 --- a/tests/test_watch.py +++ b/tests/test_watch.py @@ -4203,3 +4203,29 @@ def test_markdown_reconcile_does_not_suffix_match_top_level_target(tmp_path): assert _rebuild_code(corpus, no_cluster=True, acquire_lock=False) is True links = json.loads(graph_path.read_text(encoding="utf-8"))["links"] assert not any(edge.get("relation") == "references" for edge in links) + + +def test_rebuild_code_passes_project_root_to_builder(tmp_path, monkeypatch): + """#932: the update-path build must receive the project root so absolute + source_file paths from semantic fragments are relativized the same way + `graphify build` does. Without it a watch rebuild writes machine-absolute + paths that break sharing and path-based selectors.""" + import graphify.build as buildmod + from graphify.watch import _rebuild_code + + corpus = tmp_path / "corpus" + corpus.mkdir() + (corpus / "app.py").write_text("def run(): pass\n", encoding="utf-8") + + captured = {} + real_build_from_json = buildmod.build_from_json + + def capturing(extraction, **kwargs): + captured.update(kwargs) + return real_build_from_json(extraction, **kwargs) + + monkeypatch.setattr(buildmod, "build_from_json", capturing) + + assert _rebuild_code(corpus, acquire_lock=False) is True + assert captured.get("root") is not None, "build_from_json was called without root" + assert Path(captured["root"]).resolve() == corpus.resolve()