mirror of
https://github.com/safishamsi/graphify.git
synced 2026-10-20 10:51:24 +00:00
fix(mcp): preserve parallel edges through graph.json round-trip and MCP output
Fixes #4500
Signed-off-by: Raphael Malikian <rtmalikian@gmail.com>
(cherry picked from commit b30705dd3f)
This commit is contained in:
1 parent
e2d2fddffb
commit
3ab2cbabb4
6 files changed
+286
-63
No files matched your search
@@ -862,6 +862,27 @@ def edge_datas(G: nx.Graph, u: str, v: str) -> list[dict]:
|
||||
return [raw]
|
||||
|
||||
|
||||
def has_parallel_links(links: "list[dict]", directed: bool = True) -> bool:
|
||||
"""True when any two link dicts share the same endpoints.
|
||||
|
||||
Used to keep the serialized ``multigraph`` flag consistent with the links
|
||||
actually stored (#4500): a simple-graph write-back can restore parallel
|
||||
links from ``original_links`` while ``node_link_data`` derived
|
||||
``multigraph: false`` from the in-memory graph, and readers that honour
|
||||
the flag then collapse the parallel edges on load.
|
||||
"""
|
||||
seen: set = set()
|
||||
for link in links or []:
|
||||
if not isinstance(link, dict):
|
||||
continue
|
||||
endpoints = (link.get("source"), link.get("target"))
|
||||
key = endpoints if directed else frozenset(endpoints)
|
||||
if key in seen:
|
||||
return True
|
||||
seen.add(key)
|
||||
return False
|
||||
|
||||
|
||||
def dedupe_nodes(nodes: list[dict]) -> list[dict]:
|
||||
"""Collapse nodes sharing an ``id``, last-writer-wins on attributes.
|
||||
|
||||
|
||||
+6
-1
@@ -15,7 +15,7 @@ import networkx as nx
|
||||
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 edge_data, has_parallel_links
|
||||
from graphify.paths import (
|
||||
os_replace_with_fallback,
|
||||
stem_filename_budget,
|
||||
@@ -404,6 +404,11 @@ def to_json(G: nx.Graph, communities: dict[int, list[str]], output_path: str, *,
|
||||
if isinstance(link, dict)
|
||||
and link.get("source") in node_ids and link.get("target") in node_ids
|
||||
]
|
||||
# The flag above was derived from the in-memory simple graph, but the
|
||||
# restored links may hold parallel edges (#3999); keep the serialized
|
||||
# metadata consistent with the links or readers collapse them (#4500).
|
||||
if has_parallel_links(data["links"], directed=bool(data.get("directed", True))):
|
||||
data["multigraph"] = True
|
||||
for link in data["links"]:
|
||||
if "confidence_score" not in link:
|
||||
conf = link.get("confidence", "EXTRACTED")
|
||||
|
||||
+84
-62
@@ -14,7 +14,7 @@ from typing import NamedTuple
|
||||
import networkx as nx
|
||||
from networkx.readwrite import json_graph
|
||||
from graphify.security import sanitize_label, check_graph_file_size_cap, _CONTROL_CHAR_RE
|
||||
from graphify.build import edge_data, edge_datas
|
||||
from graphify.build import edge_datas, has_parallel_links
|
||||
from graphify.paths import default_graph_json as _default_graph_json
|
||||
|
||||
try:
|
||||
@@ -61,6 +61,13 @@ def _read_graph(graph_path: str) -> nx.Graph:
|
||||
# not mistake the override for graph truth.
|
||||
_logical_directed = bool(data.get("directed", False))
|
||||
data = {**data, "directed": True}
|
||||
# Repair files whose links hold parallel edges while the serialized flag says
|
||||
# otherwise (#4500): older writers restored original_links verbatim while
|
||||
# node_link_data derived "multigraph": false from the simple graph. Forced the
|
||||
# same way "directed" is forced above, so the load keeps every relationship
|
||||
# instead of collapsing parallel edges.
|
||||
if has_parallel_links(data.get("links"), directed=True):
|
||||
data = {**data, "multigraph": True}
|
||||
try:
|
||||
from graphify.build import graph_has_legacy_ids as _legacy
|
||||
if _legacy(data.get("nodes", [])):
|
||||
@@ -1028,8 +1035,9 @@ def _complete_induced_edges(G: nx.Graph, visited: set[str], edges_seen: list[tup
|
||||
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`).
|
||||
one. On a multigraph the traversal still records one entry per pair; the
|
||||
renderer expands it into one line per parallel edge (`_subgraph_to_text`,
|
||||
#4500).
|
||||
|
||||
Traversal edges keep their discovery order; completions are appended after.
|
||||
"""
|
||||
@@ -1200,39 +1208,42 @@ def _subgraph_to_text(G: nx.Graph, nodes: set[str], edges: list[tuple], token_bu
|
||||
lines.append(line)
|
||||
for u, v in edges:
|
||||
if u in nodes and v in nodes:
|
||||
raw = G[u][v]
|
||||
d = next(iter(raw.values()), {}) if isinstance(G, (nx.MultiGraph, nx.MultiDiGraph)) else raw
|
||||
# (u, v) is BFS/DFS visit order, not necessarily the true edge
|
||||
# direction: on an undirected graph G.neighbors() walks callers
|
||||
# and callees alike, so a caller->callee edge renders backwards
|
||||
# whenever the callee is visited first. _src/_tgt (stashed on the
|
||||
# edge data by the `query` CLI loader) carry the real direction;
|
||||
# fall back to (u, v) for graphs/edges that don't set them.
|
||||
src = d.get("_src", u)
|
||||
tgt = d.get("_tgt", v)
|
||||
# Guard against a stray/dangling _src/_tgt (hand-edited or adversarial
|
||||
# graph.json): only trust them when they name exactly this edge's
|
||||
# endpoints, else fall back to (u, v). Without this, G.nodes[src]
|
||||
# would KeyError on an unknown id (#2080 review).
|
||||
if {src, tgt} != {u, v}:
|
||||
src, tgt = u, v
|
||||
context = d.get("context")
|
||||
context_suffix = f" context={sanitize_label(str(context))}" if context else ""
|
||||
# The relation SITE (call/import/reference line in the source's
|
||||
# file), not a def line — so "who calls X" cites a clickable call
|
||||
# location, not the caller's def (#BUG1).
|
||||
_loc = str(d.get("source_location") or "")
|
||||
at_suffix = (
|
||||
f" at={sanitize_label(str(d.get('source_file') or ''))}:{sanitize_label(_loc)}"
|
||||
if _loc else ""
|
||||
)
|
||||
line = (
|
||||
f"EDGE {sanitize_label(G.nodes[src].get('label', src))} "
|
||||
f"--{sanitize_label(str(d.get('relation', '')))} "
|
||||
f"[{sanitize_label(str(d.get('confidence', '')))}{context_suffix}]--> "
|
||||
f"{sanitize_label(G.nodes[tgt].get('label', tgt))}{at_suffix}"
|
||||
)
|
||||
lines.append(line)
|
||||
# One EDGE line per relationship: on a multigraph a pair can
|
||||
# carry several edges (e.g. contains + calls), each with its
|
||||
# own metadata (#4500). The traversal records one entry per
|
||||
# pair; expanding here keeps every relationship visible.
|
||||
for d in edge_datas(G, u, v):
|
||||
# (u, v) is BFS/DFS visit order, not necessarily the true edge
|
||||
# direction: on an undirected graph G.neighbors() walks callers
|
||||
# and callees alike, so a caller->callee edge renders backwards
|
||||
# whenever the callee is visited first. _src/_tgt (stashed on the
|
||||
# edge data by the `query` CLI loader) carry the real direction;
|
||||
# fall back to (u, v) for graphs/edges that don't set them.
|
||||
src = d.get("_src", u)
|
||||
tgt = d.get("_tgt", v)
|
||||
# Guard against a stray/dangling _src/_tgt (hand-edited or adversarial
|
||||
# graph.json): only trust them when they name exactly this edge's
|
||||
# endpoints, else fall back to (u, v). Without this, G.nodes[src]
|
||||
# would KeyError on an unknown id (#2080 review).
|
||||
if {src, tgt} != {u, v}:
|
||||
src, tgt = u, v
|
||||
context = d.get("context")
|
||||
context_suffix = f" context={sanitize_label(str(context))}" if context else ""
|
||||
# The relation SITE (call/import/reference line in the source's
|
||||
# file), not a def line — so "who calls X" cites a clickable call
|
||||
# location, not the caller's def (#BUG1).
|
||||
_loc = str(d.get("source_location") or "")
|
||||
at_suffix = (
|
||||
f" at={sanitize_label(str(d.get('source_file') or ''))}:{sanitize_label(_loc)}"
|
||||
if _loc else ""
|
||||
)
|
||||
line = (
|
||||
f"EDGE {sanitize_label(G.nodes[src].get('label', src))} "
|
||||
f"--{sanitize_label(str(d.get('relation', '')))} "
|
||||
f"[{sanitize_label(str(d.get('confidence', '')))}{context_suffix}]--> "
|
||||
f"{sanitize_label(G.nodes[tgt].get('label', tgt))}{at_suffix}"
|
||||
)
|
||||
lines.append(line)
|
||||
output = "\n".join(lines)
|
||||
if len(output) > char_budget:
|
||||
cut_at = output[:char_budget].rfind("\n")
|
||||
@@ -2079,6 +2090,42 @@ def _community_header(cid: int, community_name) -> str:
|
||||
return base
|
||||
|
||||
|
||||
def _neighbor_lines(G: nx.Graph, nid: str, rel_filter: str = "") -> list[str]:
|
||||
"""One output line per relationship incident to ``nid``.
|
||||
|
||||
Parallel edges between the same pair each render as their own line with
|
||||
their own metadata (#4500); ``edge_data()`` would return only the first.
|
||||
"""
|
||||
def _edge_at(d: dict) -> str:
|
||||
# Edge location = the relation SITE (call/import line) in the source
|
||||
# node's file, not a def line (#BUG1).
|
||||
loc = str(d.get("source_location") or "")
|
||||
return (
|
||||
f" at={sanitize_label(str(d.get('source_file') or ''))}:{sanitize_label(loc)}"
|
||||
if loc else ""
|
||||
)
|
||||
lines: list[str] = []
|
||||
for nb in G.successors(nid):
|
||||
for d in edge_datas(G, nid, nb):
|
||||
rel = d.get("relation", "")
|
||||
if rel_filter and rel_filter not in rel.lower():
|
||||
continue
|
||||
lines.append(
|
||||
f" --> {sanitize_label(G.nodes[nb].get('label', nb))} "
|
||||
f"[{sanitize_label(str(rel))}] [{sanitize_label(str(d.get('confidence', '')))}]{_edge_at(d)}"
|
||||
)
|
||||
for nb in G.predecessors(nid):
|
||||
for d in edge_datas(G, nb, nid):
|
||||
rel = d.get("relation", "")
|
||||
if rel_filter and rel_filter not in rel.lower():
|
||||
continue
|
||||
lines.append(
|
||||
f" <-- {sanitize_label(G.nodes[nb].get('label', nb))} "
|
||||
f"[{sanitize_label(str(rel))}] [{sanitize_label(str(d.get('confidence', '')))}]{_edge_at(d)}"
|
||||
)
|
||||
return lines
|
||||
|
||||
|
||||
def _build_server(graph_path: str):
|
||||
"""Build the configured low-level MCP Server (shared by every transport).
|
||||
|
||||
@@ -2380,32 +2427,7 @@ def _build_server(graph_path: str):
|
||||
if err:
|
||||
return err
|
||||
lines = [f"Neighbors of {sanitize_label(G.nodes[nid].get('label', nid))}:"]
|
||||
def _edge_at(d: dict) -> str:
|
||||
# Edge location = the relation SITE (call/import line) in the source
|
||||
# node's file, not a def line (#BUG1).
|
||||
loc = str(d.get("source_location") or "")
|
||||
return (
|
||||
f" at={sanitize_label(str(d.get('source_file') or ''))}:{sanitize_label(loc)}"
|
||||
if loc else ""
|
||||
)
|
||||
for nb in G.successors(nid):
|
||||
d = edge_data(G, nid, nb)
|
||||
rel = d.get("relation", "")
|
||||
if rel_filter and rel_filter not in rel.lower():
|
||||
continue
|
||||
lines.append(
|
||||
f" --> {sanitize_label(G.nodes[nb].get('label', nb))} "
|
||||
f"[{sanitize_label(str(rel))}] [{sanitize_label(str(d.get('confidence', '')))}]{_edge_at(d)}"
|
||||
)
|
||||
for nb in G.predecessors(nid):
|
||||
d = edge_data(G, nb, nid)
|
||||
rel = d.get("relation", "")
|
||||
if rel_filter and rel_filter not in rel.lower():
|
||||
continue
|
||||
lines.append(
|
||||
f" <-- {sanitize_label(G.nodes[nb].get('label', nb))} "
|
||||
f"[{sanitize_label(str(rel))}] [{sanitize_label(str(d.get('confidence', '')))}]{_edge_at(d)}"
|
||||
)
|
||||
lines.extend(_neighbor_lines(G, nid, rel_filter))
|
||||
budget = int(arguments.get("token_budget", 2000))
|
||||
return _cut_lines_to_budget(
|
||||
lines, budget, "Narrow with relation_filter or use get_node for a specific symbol"
|
||||
|
||||
@@ -2081,3 +2081,34 @@ def test_method_ghost_ambiguous_same_name_in_one_file_is_not_merged_3705():
|
||||
assert not G.has_edge("doc", "svc_a_save_state")
|
||||
assert not G.has_edge("doc", "svc_b_save_state")
|
||||
|
||||
|
||||
|
||||
def test_has_parallel_links_detects_repeated_pairs():
|
||||
from graphify.build import has_parallel_links
|
||||
links = [
|
||||
{"source": "a", "target": "b", "relation": "contains"},
|
||||
{"source": "a", "target": "b", "relation": "calls"},
|
||||
{"source": "b", "target": "c", "relation": "calls"},
|
||||
]
|
||||
assert has_parallel_links(links) is True
|
||||
|
||||
|
||||
def test_has_parallel_links_false_without_repeats():
|
||||
from graphify.build import has_parallel_links
|
||||
links = [
|
||||
{"source": "a", "target": "b", "relation": "calls"},
|
||||
{"source": "b", "target": "c", "relation": "calls"},
|
||||
]
|
||||
assert has_parallel_links(links) is False
|
||||
assert has_parallel_links([]) is False
|
||||
assert has_parallel_links(None) is False
|
||||
|
||||
|
||||
def test_has_parallel_links_antiparallel_is_not_parallel_when_directed():
|
||||
from graphify.build import has_parallel_links
|
||||
links = [
|
||||
{"source": "a", "target": "b", "relation": "calls"},
|
||||
{"source": "b", "target": "a", "relation": "calls"},
|
||||
]
|
||||
assert has_parallel_links(links, directed=True) is False
|
||||
assert has_parallel_links(links, directed=False) is True
|
||||
@@ -1196,3 +1196,61 @@ def test_cluster_only_preserves_parallel_edges_across_an_id_remap(tmp_path):
|
||||
("claude_claude_alpha", "pkg_beta", "mentions"),
|
||||
("claude_claude_alpha", "pkg_beta", "references"),
|
||||
], pairs
|
||||
|
||||
|
||||
def test_to_json_declares_multigraph_when_original_links_have_parallel_edges(tmp_path):
|
||||
"""#4500: to_json restored original_links (which may hold parallel edges,
|
||||
#3999) while node_link_data derived "multigraph": false from the simple
|
||||
in-memory graph. The flag must match the links actually written."""
|
||||
from graphify.build import build_from_json
|
||||
from graphify.cluster import cluster
|
||||
from graphify.export import to_json
|
||||
|
||||
extraction = {
|
||||
"directed": True,
|
||||
"nodes": [
|
||||
{"id": "a", "label": "App()", "file_type": "code", "source_file": "src/App.tsx"},
|
||||
{"id": "b", "label": "applySeed()", "file_type": "code", "source_file": "src/App.tsx"},
|
||||
],
|
||||
"links": [
|
||||
{"source": "a", "target": "b", "relation": "contains", "confidence": "EXTRACTED",
|
||||
"source_file": "src/App.tsx", "source_location": "80"},
|
||||
{"source": "a", "target": "b", "relation": "calls", "confidence": "EXTRACTED",
|
||||
"source_file": "src/App.tsx", "source_location": "163"},
|
||||
],
|
||||
}
|
||||
G = build_from_json(extraction, directed=True)
|
||||
communities = cluster(G)
|
||||
out = tmp_path / "graph.json"
|
||||
assert to_json(G, communities, str(out), original_links=extraction["links"])
|
||||
|
||||
data = json.loads(out.read_text())
|
||||
assert data["multigraph"] is True
|
||||
pairs = sorted((l["source"], l["target"], l["relation"]) for l in data["links"])
|
||||
assert pairs == [("a", "b", "calls"), ("a", "b", "contains")], pairs
|
||||
|
||||
|
||||
def test_to_json_keeps_multigraph_false_without_parallel_edges(tmp_path):
|
||||
"""#4500: the flag repair must not flip graphs that have no parallel edges."""
|
||||
from graphify.build import build_from_json
|
||||
from graphify.cluster import cluster
|
||||
from graphify.export import to_json
|
||||
|
||||
extraction = {
|
||||
"directed": True,
|
||||
"nodes": [
|
||||
{"id": "a", "label": "a", "file_type": "code", "source_file": "a.py"},
|
||||
{"id": "b", "label": "b", "file_type": "code", "source_file": "a.py"},
|
||||
],
|
||||
"links": [
|
||||
{"source": "a", "target": "b", "relation": "calls", "confidence": "EXTRACTED"},
|
||||
],
|
||||
}
|
||||
G = build_from_json(extraction, directed=True)
|
||||
communities = cluster(G)
|
||||
out = tmp_path / "graph.json"
|
||||
assert to_json(G, communities, str(out), original_links=extraction["links"])
|
||||
|
||||
data = json.loads(out.read_text())
|
||||
assert data["multigraph"] is False
|
||||
assert len(data["links"]) == 1
|
||||
@@ -34,6 +34,7 @@ from graphify.serve import (
|
||||
_subgraph_to_text,
|
||||
_cut_lines_to_budget,
|
||||
_load_graph,
|
||||
_neighbor_lines,
|
||||
_community_header,
|
||||
_search_tokens,
|
||||
_shortest_path_text,
|
||||
@@ -1905,3 +1906,88 @@ def test_dotted_python_symbol_same_file_tie_is_refused_by_path_endpoint():
|
||||
G.add_node("m", label=".run()", source_file="pkg/mod.py", source_location="L9")
|
||||
nid, _, err = _resolve_path_endpoint(G, "pkg.mod.run")
|
||||
assert nid is None and err.startswith("Ambiguous")
|
||||
# --- parallel-edge preservation (#4500) ---
|
||||
|
||||
def _parallel_edge_graph_json(tmp_path):
|
||||
"""A graph.json like the #4500 reporter's: parallel links stored while the
|
||||
serialized flag says multigraph:false."""
|
||||
p = tmp_path / "graph.json"
|
||||
p.write_text(json.dumps({
|
||||
"directed": True, "multigraph": False, "graph": {},
|
||||
"nodes": [
|
||||
{"id": "src_app_app", "label": "App()"},
|
||||
{"id": "src_app_app_applyseed", "label": "applySeed()"},
|
||||
],
|
||||
"links": [
|
||||
{"source": "src_app_app", "target": "src_app_app_applyseed",
|
||||
"relation": "contains", "confidence": "EXTRACTED",
|
||||
"source_file": "src/App.tsx", "source_location": "80"},
|
||||
{"source": "src_app_app", "target": "src_app_app_applyseed",
|
||||
"relation": "calls", "confidence": "EXTRACTED",
|
||||
"source_file": "src/App.tsx", "source_location": "163"},
|
||||
],
|
||||
}))
|
||||
return str(p)
|
||||
|
||||
|
||||
def test_load_graph_repairs_multigraph_false_with_parallel_links(tmp_path):
|
||||
G = _load_graph(_parallel_edge_graph_json(tmp_path))
|
||||
assert G.is_multigraph()
|
||||
# graph_stats reports G.number_of_edges(): the reporter's 134 stored links
|
||||
# became 131 loaded edges; here both must survive.
|
||||
assert G.number_of_edges() == 2
|
||||
got = sorted(
|
||||
d.get("relation")
|
||||
for u, v, d in G.edges(data=True)
|
||||
if (u, v) == ("src_app_app", "src_app_app_applyseed")
|
||||
)
|
||||
assert got == ["calls", "contains"], got
|
||||
|
||||
|
||||
def test_load_graph_keeps_simple_graph_simple(tmp_path):
|
||||
p = tmp_path / "graph.json"
|
||||
p.write_text(json.dumps({
|
||||
"directed": True, "multigraph": False, "graph": {},
|
||||
"nodes": [{"id": "a", "label": "a"}, {"id": "b", "label": "b"}],
|
||||
"links": [{"source": "a", "target": "b", "relation": "calls", "confidence": "EXTRACTED"}],
|
||||
}))
|
||||
G = _load_graph(str(p))
|
||||
assert not G.is_multigraph()
|
||||
assert G.number_of_edges() == 1
|
||||
|
||||
|
||||
def test_neighbor_lines_shows_every_parallel_relationship(tmp_path):
|
||||
G = _load_graph(_parallel_edge_graph_json(tmp_path))
|
||||
lines = _neighbor_lines(G, "src_app_app")
|
||||
assert len(lines) == 2, lines
|
||||
assert any("[contains]" in l and "App.tsx:80" in l for l in lines), lines
|
||||
assert any("[calls]" in l and "App.tsx:163" in l for l in lines), lines
|
||||
# each relationship keeps its own metadata: no combined/merged edge
|
||||
assert len({l for l in lines}) == 2
|
||||
|
||||
|
||||
def test_neighbor_lines_relation_filter_applies_per_edge(tmp_path):
|
||||
G = _load_graph(_parallel_edge_graph_json(tmp_path))
|
||||
lines = _neighbor_lines(G, "src_app_app", rel_filter="calls")
|
||||
assert len(lines) == 1, lines
|
||||
assert "[calls]" in lines[0]
|
||||
|
||||
|
||||
def test_subgraph_to_text_renders_every_parallel_edge(tmp_path):
|
||||
G = _load_graph(_parallel_edge_graph_json(tmp_path))
|
||||
text = _subgraph_to_text(
|
||||
G,
|
||||
{"src_app_app", "src_app_app_applyseed"},
|
||||
[("src_app_app", "src_app_app_applyseed")],
|
||||
)
|
||||
edge_lines = [l for l in text.splitlines() if l.startswith("EDGE ")]
|
||||
assert len(edge_lines) == 2, edge_lines
|
||||
assert any("--contains " in l and "App.tsx:80" in l for l in edge_lines), edge_lines
|
||||
assert any("--calls " in l and "App.tsx:163" in l for l in edge_lines), edge_lines
|
||||
|
||||
|
||||
def test_query_graph_text_surfaces_parallel_relationships(tmp_path):
|
||||
G = _load_graph(_parallel_edge_graph_json(tmp_path))
|
||||
text = _query_graph_text(G, "applySeed", token_budget=2000)
|
||||
assert "--contains " in text, text
|
||||
assert "--calls " in text, text
|
||||
Reference in new issue
Block a user