mirror of
https://github.com/safishamsi/graphify.git
synced 2026-08-27 16:56:36 +00:00
fix(path): deterministic route + honest edge relation, not fabricated calls (#2074)
`graphify path` (and the MCP shortest_path tool) ran shortest_path over G.to_undirected(as_view=True), whose neighbor iteration is a hash-seeded set union, so among equal-length paths BFS returned a route that varied per process. Build a sorted, materialized undirected graph so the chosen path is canonical. The hop label also printed a relation read from an arbitrarily-collapsed parallel edge, so it could show `calls` on a pair that only carries `references`. Force multigraph on the cli path reload so parallel links survive, and render the ACTUAL stored relation(s) via edge_datas, falling back to an honest "related" when the edge has none. Serve's shared graph is left untouched (its degree feeds query-seed tie-breaks); the fix is applied locally in both path readers.
This commit is contained in:
+27
-10
@@ -1088,8 +1088,13 @@ def dispatch_command(cmd: str) -> None:
|
||||
_raw = json.loads(gp.read_text(encoding="utf-8"))
|
||||
if "links" not in _raw and "edges" in _raw:
|
||||
_raw = dict(_raw, links=_raw["edges"])
|
||||
# Force directed so the renderer can recover stored caller→callee direction.
|
||||
_raw = {**_raw, "directed": True}
|
||||
# Force directed so the renderer can recover stored caller→callee
|
||||
# direction, and multigraph so exact-pair parallel links (e.g. a
|
||||
# `references` and a `calls` edge between the same two nodes) survive load
|
||||
# instead of being silently collapsed last-writer-wins — otherwise the
|
||||
# printed relation could be one the traversed pair doesn't actually
|
||||
# carry (#2074). Local to this read; serve's shared graph is untouched.
|
||||
_raw = {**_raw, "directed": True, "multigraph": True}
|
||||
try:
|
||||
G = json_graph.node_link_graph(_raw, edges="links")
|
||||
except TypeError:
|
||||
@@ -1129,26 +1134,38 @@ def dispatch_command(cmd: str) -> None:
|
||||
f"(top score {_top:g}, runner-up {_runner:g})",
|
||||
file=sys.stderr,
|
||||
)
|
||||
# Deterministic shortest path (#2074): to_undirected(as_view=True)
|
||||
# iterates neighbors via a hash-seeded set union, so among equal-length
|
||||
# paths BFS returned an arbitrary route that varied per process. Build a
|
||||
# sorted, materialized undirected graph so neighbor order — and thus the
|
||||
# chosen path — is canonical for a given graph.json.
|
||||
_und = _nx.Graph()
|
||||
_und.add_nodes_from(sorted(G.nodes))
|
||||
_und.add_edges_from(sorted((min(u, v), max(u, v)) for u, v in G.edges()))
|
||||
try:
|
||||
path_nodes = _nx.shortest_path(G.to_undirected(as_view=True), src_nid, tgt_nid)
|
||||
path_nodes = _nx.shortest_path(_und, src_nid, tgt_nid)
|
||||
except (_nx.NetworkXNoPath, _nx.NodeNotFound):
|
||||
print(f"No path found between '{source_label}' and '{target_label}'.")
|
||||
sys.exit(0)
|
||||
hops = len(path_nodes) - 1
|
||||
segments = []
|
||||
from graphify.build import edge_data
|
||||
from graphify.build import edge_datas
|
||||
for i in range(len(path_nodes) - 1):
|
||||
u, v = path_nodes[i], path_nodes[i + 1]
|
||||
# Check which direction the stored edge points.
|
||||
# Report the ACTUAL stored relation(s) of the traversed pair and
|
||||
# direction — never a fabricated `calls` (#2074). A pair may carry
|
||||
# several parallel relations; show all, and fall back to an honest
|
||||
# "related" when the stored edge has no relation.
|
||||
if G.has_edge(u, v):
|
||||
edata = edge_data(G, u, v)
|
||||
datas = edge_datas(G, u, v)
|
||||
forward = True
|
||||
else:
|
||||
edata = edge_data(G, v, u)
|
||||
datas = edge_datas(G, v, u)
|
||||
forward = False
|
||||
rel = edata.get("relation", "")
|
||||
conf = edata.get("confidence", "")
|
||||
conf_str = f" [{conf}]" if conf else ""
|
||||
rels = sorted({d.get("relation") for d in datas if d.get("relation")})
|
||||
rel = "/".join(rels) if rels else "related"
|
||||
confs = sorted({d.get("confidence") for d in datas if d.get("confidence")})
|
||||
conf_str = f" [{'/'.join(confs)}]" if confs else ""
|
||||
if i == 0:
|
||||
segments.append(G.nodes[u].get("label", u))
|
||||
if forward:
|
||||
|
||||
+17
-8
@@ -10,7 +10,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
|
||||
from graphify.build import edge_data
|
||||
from graphify.build import edge_data, edge_datas
|
||||
from graphify.paths import default_graph_json as _default_graph_json
|
||||
|
||||
try:
|
||||
@@ -1376,8 +1376,14 @@ def _build_server(graph_path: str):
|
||||
)
|
||||
max_hops = int(arguments.get("max_hops", 8))
|
||||
try:
|
||||
# Use undirected view for path-finding (works regardless of query src/tgt order)
|
||||
path_nodes = nx.shortest_path(G.to_undirected(as_view=True), src_nid, tgt_nid)
|
||||
# Deterministic path (#2074): the hash-seeded undirected view picked an
|
||||
# arbitrary route among equal-length paths. Build a sorted, materialized
|
||||
# undirected graph so the chosen path is canonical. Serve's shared G is
|
||||
# left untouched (its degree feeds query-seed tie-breaks).
|
||||
_und = nx.Graph()
|
||||
_und.add_nodes_from(sorted(G.nodes))
|
||||
_und.add_edges_from(sorted((min(u, v), max(u, v)) for u, v in G.edges()))
|
||||
path_nodes = nx.shortest_path(_und, src_nid, tgt_nid)
|
||||
except (nx.NetworkXNoPath, nx.NodeNotFound):
|
||||
return f"No path found between '{G.nodes[src_nid].get('label', src_nid)}' and '{G.nodes[tgt_nid].get('label', tgt_nid)}'."
|
||||
hops = len(path_nodes) - 1
|
||||
@@ -1386,15 +1392,18 @@ def _build_server(graph_path: str):
|
||||
segments = []
|
||||
for i in range(len(path_nodes) - 1):
|
||||
u, v = path_nodes[i], path_nodes[i + 1]
|
||||
# Report the actual stored relation(s), never a fabricated `calls`;
|
||||
# fall back to an honest "related" when the edge has no relation (#2074).
|
||||
if G.has_edge(u, v):
|
||||
edata = edge_data(G, u, v)
|
||||
datas = edge_datas(G, u, v)
|
||||
forward = True
|
||||
else:
|
||||
edata = edge_data(G, v, u)
|
||||
datas = edge_datas(G, v, u)
|
||||
forward = False
|
||||
rel = edata.get("relation", "")
|
||||
conf = edata.get("confidence", "")
|
||||
conf_str = f" [{conf}]" if conf else ""
|
||||
rels = sorted({d.get("relation") for d in datas if d.get("relation")})
|
||||
rel = "/".join(rels) if rels else "related"
|
||||
confs = sorted({d.get("confidence") for d in datas if d.get("confidence")})
|
||||
conf_str = f" [{'/'.join(confs)}]" if confs else ""
|
||||
if i == 0:
|
||||
segments.append(G.nodes[u].get("label", u))
|
||||
if forward:
|
||||
|
||||
+90
-1
@@ -1,6 +1,10 @@
|
||||
"""Regression tests for `graphify path` arrow direction (#849)."""
|
||||
"""Regression tests for `graphify path` arrow direction (#849) and determinism +
|
||||
honest edge labels (#2074)."""
|
||||
from __future__ import annotations
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import networkx as nx
|
||||
import pytest
|
||||
from networkx.readwrite import json_graph
|
||||
@@ -103,3 +107,88 @@ def test_endpoint_falls_back_to_score_head(monkeypatch, tmp_path, capsys):
|
||||
mainmod.main()
|
||||
assert exc_info.value.code == 0
|
||||
assert "No path found" in capsys.readouterr().out
|
||||
|
||||
|
||||
# ── #2074: deterministic route + honest edge relation ────────────────────────
|
||||
|
||||
def _diamond_graph(tmp_path):
|
||||
"""Two equal-length routes A->P->B and A->Q->B — a tie the traversal must
|
||||
resolve deterministically."""
|
||||
data = {
|
||||
"directed": False, "multigraph": False, "graph": {},
|
||||
"nodes": [
|
||||
{"id": "a", "label": "Alpha", "source_file": "a.py"},
|
||||
{"id": "p", "label": "Pmid", "source_file": "p.py"},
|
||||
{"id": "q", "label": "Qmid", "source_file": "q.py"},
|
||||
{"id": "b", "label": "Beta", "source_file": "b.py"},
|
||||
],
|
||||
"links": [
|
||||
{"source": "a", "target": "p", "relation": "calls", "confidence": "EXTRACTED"},
|
||||
{"source": "p", "target": "b", "relation": "calls", "confidence": "EXTRACTED"},
|
||||
{"source": "a", "target": "q", "relation": "calls", "confidence": "EXTRACTED"},
|
||||
{"source": "q", "target": "b", "relation": "calls", "confidence": "EXTRACTED"},
|
||||
],
|
||||
}
|
||||
p = tmp_path / "graph.json"
|
||||
p.write_text(json.dumps(data))
|
||||
return p
|
||||
|
||||
|
||||
def _arrow_line(stdout: str) -> str:
|
||||
return next((l.strip() for l in stdout.splitlines() if "-->" in l or "<--" in l), "")
|
||||
|
||||
|
||||
def test_path_deterministic_across_hash_seeds(tmp_path):
|
||||
"""#2074: the same graph must yield the same route regardless of
|
||||
PYTHONHASHSEED. pytest fixes the seed per process, so run out-of-process."""
|
||||
gp = _diamond_graph(tmp_path)
|
||||
routes = set()
|
||||
for seed in ("0", "1", "2", "3", "4", "5", "6", "7"):
|
||||
env = {**os.environ, "PYTHONHASHSEED": seed}
|
||||
r = subprocess.run(
|
||||
[sys.executable, "-m", "graphify", "path", "Alpha", "Beta", "--graph", str(gp)],
|
||||
capture_output=True, text=True, env=env, cwd=str(tmp_path),
|
||||
)
|
||||
assert r.returncode == 0, r.stderr
|
||||
routes.add(_arrow_line(r.stdout))
|
||||
assert len(routes) == 1, f"non-deterministic path across hash seeds: {routes}"
|
||||
# Canonical tie-break picks the lexicographically-smaller mid node (Pmid).
|
||||
assert "Pmid" in next(iter(routes))
|
||||
|
||||
|
||||
def test_path_relation_matches_stored_edge_not_fabricated(monkeypatch, tmp_path, capsys):
|
||||
"""#2074: the printed relation must be the edge's ACTUAL stored relation,
|
||||
never a hardcoded/fabricated `calls`."""
|
||||
data = {
|
||||
"directed": False, "multigraph": False, "graph": {},
|
||||
"nodes": [
|
||||
{"id": "a", "label": "Alpha", "source_file": "a.py"},
|
||||
{"id": "b", "label": "Beta", "source_file": "b.py"},
|
||||
],
|
||||
"links": [
|
||||
{"source": "a", "target": "b", "relation": "references", "confidence": "INFERRED"},
|
||||
],
|
||||
}
|
||||
gp = tmp_path / "graph.json"
|
||||
gp.write_text(json.dumps(data))
|
||||
out = _run(monkeypatch, gp, "Alpha", "Beta", capsys)
|
||||
assert "--references [INFERRED]-->" in out
|
||||
assert "calls" not in out
|
||||
|
||||
|
||||
def test_path_relation_fallback_related_when_missing(monkeypatch, tmp_path, capsys):
|
||||
"""#2074: an edge with no stored relation prints an honest 'related', not an
|
||||
empty '---->' arrow and not a fabricated relation."""
|
||||
data = {
|
||||
"directed": False, "multigraph": False, "graph": {},
|
||||
"nodes": [
|
||||
{"id": "a", "label": "Alpha", "source_file": "a.py"},
|
||||
{"id": "b", "label": "Beta", "source_file": "b.py"},
|
||||
],
|
||||
"links": [{"source": "a", "target": "b"}],
|
||||
}
|
||||
gp = tmp_path / "graph.json"
|
||||
gp.write_text(json.dumps(data))
|
||||
out = _run(monkeypatch, gp, "Alpha", "Beta", capsys)
|
||||
assert "--related-->" in out
|
||||
assert "---->" not in out.replace("--related-->", "")
|
||||
|
||||
Reference in New Issue
Block a user