diff --git a/graphify/serve.py b/graphify/serve.py index 1a44c781d..5684b01eb 100644 --- a/graphify/serve.py +++ b/graphify/serve.py @@ -819,6 +819,56 @@ def _filter_graph_by_context(G: nx.Graph, context_filters: list[str] | None) -> return H +def _complete_induced_edges(G: nx.Graph, visited: set[str], edges_seen: list[tuple]) -> None: + """Append edges between visited nodes that the traversal never recorded (#2323). + + Both traversals only record an edge that *discovers* an unvisited neighbour, + so what they return is a traversal tree, not the induced subgraph over the + nodes they return. `_bfs` marks every seed visited up front, so an edge + between two seeds can never be recorded — the reported symptom, where both + endpoints render and the edge between them does not. It drops ordinary + cross-edges for the same reason. `_dfs` appends on push rather than on + visit, so it already captured those; its one gap is an edge between two + non-seed hubs, since neither endpoint is ever expanded. + + Scans only edges incident to `visited`, so cost tracks the subgraph rather + than the whole graph, bounded by O(2E) overall. A visited hub is rescanned + in full even though the traversal deliberately did not expand it — that is + unavoidable, since a hub-to-hub edge is exactly the case `_dfs` misses. + `G` here is the context-filtered `traversal_graph` (see + `_query_graph_text`), so a filtered-out relation cannot reappear. + + Self-loops are skipped. A recursive function legitimately carries one, but + neither traversal has ever recorded one (`n` is always already visited when + its own self-loop is examined), and surfacing them is a separate output + change from the missing edges reported here. + + Dedup keys on the ordered pair for directed graphs and the unordered pair + otherwise: on a DiGraph `u->v` and `v->u` are genuinely distinct edges + (mutual recursion, circular imports), and collapsing them would drop a real + one. On a multigraph parallel edges collapse to one entry, matching the + renderer, which already shows only the first (`_subgraph_to_text`). + + Traversal edges keep their discovery order; completions are appended after. + """ + directed = G.is_directed() + + def _key(u: str, v: str): + return (u, v) if directed else frozenset((u, v)) + + seen = {_key(u, v) for u, v in edges_seen} + # sorted() so the appended order can't shift run-to-run with CPython's + # per-process string-hash seed, the same reason the renderer sorts (#1753). + for u, v in G.edges(sorted(visited)): + if u == v or v not in visited: + continue + key = _key(u, v) + if key in seen: + continue + seen.add(key) + edges_seen.append((u, v)) + + def _bfs(G: nx.Graph, start_nodes: list[str], depth: int) -> tuple[set[str], list[tuple]]: # Compute hub threshold: nodes above this degree are not expanded as transit. # p99 of degree distribution, floored at 50 to avoid over-blocking small graphs. @@ -846,6 +896,7 @@ def _bfs(G: nx.Graph, start_nodes: list[str], depth: int) -> tuple[set[str], lis edges_seen.append((n, neighbor)) visited.update(next_frontier) frontier = next_frontier + _complete_induced_edges(G, visited, edges_seen) return visited, edges_seen @@ -872,6 +923,7 @@ def _dfs(G: nx.Graph, start_nodes: list[str], depth: int) -> tuple[set[str], lis if neighbor not in visited: stack.append((neighbor, d + 1)) edges_seen.append((node, neighbor)) + _complete_induced_edges(G, visited, edges_seen) return visited, edges_seen diff --git a/tests/test_query_induced_edges.py b/tests/test_query_induced_edges.py new file mode 100644 index 000000000..bd02b6a8d --- /dev/null +++ b/tests/test_query_induced_edges.py @@ -0,0 +1,249 @@ +"""`graphify query` must render every edge between visited nodes (#2323). + +`_bfs`/`_dfs` recorded an edge only when it discovered an *unvisited* neighbour, +so the result was a traversal tree rather than the induced subgraph over the +node set the query returns. The most visible casualty is an edge between two +seed nodes: both endpoints render, the edge between them does not. +""" +from __future__ import annotations + +import json + +import networkx as nx +from networkx.readwrite import json_graph + +import graphify.__main__ as mainmod +from graphify.serve import _bfs, _dfs, _filter_graph_by_context, _query_graph_text + +# Hub suppression only kicks in at degree >= 50 (serve.py `hub_threshold`). +_HUB_PADDING = 60 + + +def _add(G, *names): + for n in names: + G.add_node(n, label=n, source_file=f"{n}.py", source_location="L1", community=0) + + +def _link(G, a, b, relation="calls", context="call"): + G.add_edge(a, b, relation=relation, confidence="EXTRACTED", context=context) + + +def _pairs(edges): + return {frozenset(e) for e in edges} + + +def _induced(G, visited): + return {frozenset((u, v)) for u, v in G.edges() if u in visited and v in visited} + + +# --- the reported case ------------------------------------------------------- + + +def test_bfs_records_edge_between_two_seeds(): + """The reporter's repro: both endpoints are seeds, so neither discovers the other.""" + G = nx.Graph() + _add(G, "checkout", "discounted_total") + _link(G, "checkout", "discounted_total") + + visited, edges = _bfs(G, ["checkout", "discounted_total"], depth=1) + + assert visited == {"checkout", "discounted_total"} + assert _pairs(edges) == {frozenset(("checkout", "discounted_total"))} + + +def test_bfs_records_cross_edge_in_a_triangle(): + """n2-n3 closes the triangle but discovers nothing, so it was dropped.""" + G = nx.Graph() + _add(G, "n1", "n2", "n3") + _link(G, "n1", "n2") + _link(G, "n1", "n3") + _link(G, "n2", "n3") + + visited, edges = _bfs(G, ["n1"], depth=2) + + assert visited == {"n1", "n2", "n3"} + assert _pairs(edges) == _induced(G, visited) + + +def test_bfs_records_edge_between_two_visited_hubs(): + """A hub is visited but never expanded, so hub-to-hub edges vanished.""" + G = nx.Graph() + _add(G, "seed", "hub_a", "hub_b") + _link(G, "seed", "hub_a") + _link(G, "seed", "hub_b") + _link(G, "hub_a", "hub_b") + for i in range(_HUB_PADDING): + _add(G, f"a{i}", f"b{i}") + _link(G, "hub_a", f"a{i}") + _link(G, "hub_b", f"b{i}") + + visited, edges = _bfs(G, ["seed"], depth=1) + + assert visited == {"seed", "hub_a", "hub_b"} + assert frozenset(("hub_a", "hub_b")) in _pairs(edges) + + +def test_dfs_records_edge_between_two_visited_hubs(): + """DFS's only induced-edge gap: neither endpoint expands, so neither records it. + + DFS appends on push rather than on visit, so unlike `_bfs` it already + captured seed-to-seed and ordinary cross-edges. Two mutually adjacent + non-seed hubs are the case it could not reach. + """ + G = nx.Graph() + _add(G, "seed", "hub_a", "hub_b") + _link(G, "seed", "hub_a") + _link(G, "seed", "hub_b") + _link(G, "hub_a", "hub_b") + for i in range(_HUB_PADDING): + _add(G, f"a{i}", f"b{i}") + _link(G, "hub_a", f"a{i}") + _link(G, "hub_b", f"b{i}") + + visited, edges = _dfs(G, ["seed"], depth=1) + + assert visited == {"seed", "hub_a", "hub_b"} + assert frozenset(("hub_a", "hub_b")) in _pairs(edges) + + +# --- invariants the completion pass must not break --------------------------- + + +def test_traversal_edges_keep_discovery_order_and_come_first(): + G = nx.Graph() + _add(G, "n1", "n2", "n3") + _link(G, "n1", "n2") + _link(G, "n1", "n3") + _link(G, "n2", "n3") + + _, edges = _bfs(G, ["n1"], depth=2) + + assert edges[:2] == [("n1", "n2"), ("n1", "n3")] + + +def test_no_duplicate_edges_are_returned(): + G = nx.Graph() + _add(G, "n1", "n2", "n3", "n4") + for a, b in [("n1", "n2"), ("n1", "n3"), ("n2", "n3"), ("n2", "n4"), ("n3", "n4")]: + _link(G, a, b) + + for traverse in (_bfs, _dfs): + _, edges = traverse(G, ["n1"], depth=3) + assert len(edges) == len(_pairs(edges)), traverse.__name__ + + +def test_completion_respects_the_context_filter(): + """The completion pass must scan the filtered graph, never the raw one. + + Scanning raw `G` would resurrect the `import` edge the user explicitly + filtered out, which is worse than the bug being fixed. The two relations + need distinct node pairs: on a plain Graph a second `add_edge` on the same + pair overwrites the first, which would leave nothing to filter and make + this assertion vacuous. + """ + G = nx.Graph() + _add(G, "n1", "n2", "n3") + _link(G, "n1", "n2", relation="calls", context="call") + _link(G, "n1", "n3", relation="imports", context="import") + _link(G, "n2", "n3", relation="imports", context="import") + + filtered = _filter_graph_by_context(G, ["call"]) + _, edges = _bfs(filtered, ["n1", "n2", "n3"], depth=1) + + assert _pairs(edges) == {frozenset(("n1", "n2"))}, "an import edge came back" + + +def test_self_loops_are_not_introduced(): + """A recursive function carries a self-loop; the traversal never rendered one. + + Surfacing them would be an output change beyond the missing edges reported + in #2323, so the completion pass skips them. + """ + G = nx.Graph() + _add(G, "recurse", "caller") + _link(G, "recurse", "recurse") + _link(G, "caller", "recurse") + + _, edges = _bfs(G, ["caller", "recurse"], depth=1) + + assert _pairs(edges) == {frozenset(("caller", "recurse"))} + + +def test_directed_graph_keeps_both_directions_of_a_mutual_edge(): + """u->v and v->u are distinct on a DiGraph (mutual recursion, circular imports). + + The MCP `query_graph` path forces `directed: True`, so unordered dedup here + would silently drop a real edge. + """ + G = nx.DiGraph() + _add(G, "ping", "pong") + _link(G, "ping", "pong") + _link(G, "pong", "ping") + + _, edges = _bfs(G, ["ping", "pong"], depth=1) + + assert set(edges) == {("ping", "pong"), ("pong", "ping")} + + +def test_directed_graph_renders_the_seed_to_seed_edge(): + """Mirrors the MCP path, which loads every graph with directed=True.""" + G = nx.DiGraph() + _add(G, "checkout", "discounted_total") + _link(G, "checkout", "discounted_total") + + text = _query_graph_text(G, "checkout discounted_total", depth=1) + + assert "EDGE checkout --calls" in text + assert "discounted_total" in text + + +# --- end to end through the real CLI ---------------------------------------- + + +def _write_two_seed_graph(tmp_path): + """The reporter's shape: one `calls` edge whose endpoints are both seeds.""" + G = nx.Graph() + G.add_node( + "app.py::checkout", + label="checkout", + source_file="app.py", + source_location="L4", + community=0, + ) + G.add_node( + "pricing.py::discounted_total", + label="discounted_total", + source_file="pricing.py", + source_location="L1", + community=0, + ) + G.add_edge( + "app.py::checkout", + "pricing.py::discounted_total", + relation="calls", + confidence="EXTRACTED", + context="call", + source_file="app.py", + source_location="L5", + ) + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps(json_graph.node_link_data(G, edges="links"))) + return graph_path + + +def test_query_cli_renders_the_edge_between_two_seeds(monkeypatch, tmp_path, capsys): + graph_path = _write_two_seed_graph(tmp_path) + monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) + monkeypatch.setattr( + mainmod.sys, + "argv", + ["graphify", "query", "checkout discounted_total", "--graph", str(graph_path)], + ) + + mainmod.main() + out = capsys.readouterr().out + + assert "NODE checkout" in out + assert "NODE discounted_total" in out + assert "EDGE checkout --calls" in out + assert "discounted_total" in out.split("EDGE checkout --calls", 1)[1]