fix(query): render every edge between visited nodes, not just traversal edges
`graphify query` returned the right nodes but dropped real edges between them. Both traversals 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 reports. `_bfs` marks every seed visited before the loop starts, so an edge between two seeds could never be recorded — the reported symptom, where both endpoints render and the edge between them does not. It drops ordinary cross-edges and hub-adjacent edges for the same reason. `_dfs` needed the same completion for a narrower case. It appends on push rather than on visit, so it already captured seed-to-seed and cross-edges; an exhaustive search over every connected four- and five-node graph found no miss. Its one gap is an edge between two non-seed hubs, where the hub guard means neither endpoint is ever expanded and so neither records it. The new `_complete_induced_edges` runs at the end of both. It scans only edges incident to the visited set, so cost tracks the subgraph rather than the whole graph. A visited hub is rescanned in full, which is unavoidable: a hub-to-hub edge is exactly the case `_dfs` misses. Placing the pass inside the traversals rather than at the call site binds it to the context-filtered `traversal_graph` by construction, so a relation the user filtered out cannot reappear. Dedup keys on the ordered pair for directed graphs and the unordered pair otherwise: on a DiGraph `u->v` and `v->u` are distinct edges (mutual recursion, circular imports) and collapsing them would drop a real one. Parallel edges on a multigraph collapse to one entry, matching the renderer, which already shows only the first. Self-loops are skipped, since no traversal ever recorded one and surfacing recursion edges is a separate output change. Traversal edges keep their discovery order; completions are appended after. Closes #2323
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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]
|
||||
Reference in New Issue
Block a user