fix: explain no longer resolves an ambiguous name to an arbitrary file
`_find_node` ranks matches but never reports that a tie was broken, so callers taking `matches[0]` present one arbitrary file as the answer. Two workspaces that each define `MetricsPort` put both nodes in the same `exact` tier, separated only by `G.nodes()` iteration order — reorder the graph and the same query answers with a different file, equally confidently. `affected` already declines this via `resolve_seed()` returning None, and `path` warns when the top two scores are within 10%. Only `explain` (and the MCP `get_neighbors` tool, which shares the matcher) was silent. Split `_find_node` into `_find_node_tiers` (logic unchanged, tiers exposed) plus a flattening wrapper, and add `find_node_ambiguity()`, which returns rivals only when the winning tier spans multiple source files. Matches within one file (a file node plus its members) stay ordinary precedence and resolve as before. `_disambiguate_file_node_labels` (#2032) already relabels colliding *file* nodes; this covers the symbol case it does not reach.
This commit is contained in:
0bLoM
safishamsi
+9
-1
@@ -1289,7 +1289,7 @@ def dispatch_command(cmd: str) -> None:
|
||||
if len(sys.argv) < 3:
|
||||
print('Usage: graphify explain "<node>" [--graph path]', file=sys.stderr)
|
||||
sys.exit(1)
|
||||
from graphify.serve import _find_node
|
||||
from graphify.serve import _find_node, find_node_ambiguity
|
||||
from networkx.readwrite import json_graph
|
||||
|
||||
label = sys.argv[2]
|
||||
@@ -1316,6 +1316,14 @@ def dispatch_command(cmd: str) -> None:
|
||||
if not matches:
|
||||
print(f"No node matching '{label}' found.")
|
||||
sys.exit(0)
|
||||
rivals = find_node_ambiguity(G, label)
|
||||
if rivals:
|
||||
print(f"Ambiguous: '{label}' matches {len(rivals)} nodes in different files.")
|
||||
for rival in rivals:
|
||||
print(f" {G.nodes[rival].get('source_file') or rival}")
|
||||
print(f" id: {rival}")
|
||||
print("Retry with the repo-relative path or the full node id.")
|
||||
sys.exit(1)
|
||||
nid = matches[0]
|
||||
d = G.nodes[nid]
|
||||
print(f"Node: {d.get('label', nid)}")
|
||||
|
||||
+56
-5
@@ -1067,12 +1067,15 @@ def _query_graph_text(
|
||||
return header + _subgraph_to_text(traversal_graph, nodes, edges, token_budget, seeds=start_nodes)
|
||||
|
||||
|
||||
def _find_node(G: nx.Graph, label: str) -> list[str]:
|
||||
"""Return node IDs whose label or ID matches the search term (diacritic-insensitive).
|
||||
def _find_node_tiers(
|
||||
G: nx.Graph, label: str
|
||||
) -> tuple[list[str], list[str], list[str], list[str]]:
|
||||
"""Return match tiers in precedence order: (source_exact, exact, prefix, substring).
|
||||
|
||||
Results are ordered by precedence: exact source-file path match first, then
|
||||
exact (label/ID) match, then prefix match, then substring match. Node-ID exact
|
||||
matches are grouped with label exact matches.
|
||||
Split out of `_find_node` so callers that must not guess between equally-good
|
||||
matches can inspect the winning tier alone. `_find_node` flattens these, and
|
||||
its consumers take `[0]` — which resolves by graph-iteration order when one
|
||||
tier holds several nodes from different files. See `find_node_ambiguity`.
|
||||
"""
|
||||
term = " ".join(_search_tokens(label))
|
||||
if not term:
|
||||
@@ -1134,9 +1137,47 @@ def _find_node(G: nx.Graph, label: str) -> list[str]:
|
||||
if len(preferred) == 1:
|
||||
source_exact = preferred + [nid for nid in source_exact if nid != preferred[0]]
|
||||
|
||||
return source_exact, exact, prefix, substring
|
||||
|
||||
|
||||
def _find_node(G: nx.Graph, label: str) -> list[str]:
|
||||
"""Return node IDs whose label or ID matches the search term (diacritic-insensitive).
|
||||
|
||||
Results are ordered by precedence: exact source-file path match first, then
|
||||
exact (label/ID) match, then prefix match, then substring match. Node-ID exact
|
||||
matches are grouped with label exact matches.
|
||||
"""
|
||||
source_exact, exact, prefix, substring = _find_node_tiers(G, label)
|
||||
return source_exact + exact + prefix + substring
|
||||
|
||||
|
||||
def find_node_ambiguity(G: nx.Graph, label: str) -> list[str]:
|
||||
"""Return rival candidates when the winning match tier spans several source files.
|
||||
|
||||
`_find_node` ranks matches but never reports that a tie was broken, so callers
|
||||
taking `[0]` present one arbitrary file as the answer. Two workspaces that each
|
||||
define `MetricsPort` put both nodes in the same `exact` tier, separated only by
|
||||
`G.nodes()` iteration order — reorder the graph and the same query answers with
|
||||
a different file, equally confidently.
|
||||
|
||||
Returns one representative node id per distinct source file when the winning
|
||||
tier is split that way, else `[]`. Several matches *within one file* (a file
|
||||
node plus its members) are ordinary precedence, not ambiguity, and return `[]`.
|
||||
|
||||
`_disambiguate_file_node_labels` (#2032) already relabels colliding *file*
|
||||
nodes; this covers the symbol case it does not reach.
|
||||
"""
|
||||
for tier in _find_node_tiers(G, label):
|
||||
if not tier:
|
||||
continue
|
||||
by_source: dict[str, str] = {}
|
||||
for nid in tier:
|
||||
source = str(G.nodes[nid].get("source_file") or "")
|
||||
by_source.setdefault(source, nid)
|
||||
return list(by_source.values()) if len(by_source) > 1 else []
|
||||
return []
|
||||
|
||||
|
||||
def _filter_blank_stdin() -> None:
|
||||
"""Filter blank lines from stdin before MCP reads it.
|
||||
|
||||
@@ -1448,6 +1489,16 @@ def _build_server(graph_path: str):
|
||||
matches = _find_node(G, label)
|
||||
if not matches:
|
||||
return f"No node matching '{label}' found."
|
||||
rivals = find_node_ambiguity(G, label)
|
||||
if rivals:
|
||||
listing = "\n".join(
|
||||
f" {G.nodes[r].get('source_file') or r}\n id: {r}" for r in rivals
|
||||
)
|
||||
return (
|
||||
f"Ambiguous: '{label}' matches {len(rivals)} nodes in different files.\n"
|
||||
f"{listing}\n"
|
||||
"Retry with the repo-relative path or the full node id."
|
||||
)
|
||||
nid = matches[0]
|
||||
lines = [f"Neighbors of {sanitize_label(G.nodes[nid].get('label', nid))}:"]
|
||||
def _edge_at(d: dict) -> str:
|
||||
|
||||
@@ -236,3 +236,87 @@ def test_explain_grouping_boundary_at_exactly_21_vs_20_connections(monkeypatch,
|
||||
out20 = _run(monkeypatch, p20, "hub", capsys)
|
||||
assert "Grouped by file:" not in out20
|
||||
assert "more" not in out20
|
||||
|
||||
|
||||
# --- ambiguous label across files (#explain-ambiguity) -----------------------
|
||||
|
||||
|
||||
def _write_ambiguous_graph(tmp_path, *, reverse: bool = False):
|
||||
"""Two DIFFERENT symbols that happen to share a label, in different files.
|
||||
|
||||
This is the monorepo shape: each workspace defines its own `MetricsPort`.
|
||||
Both land in `_find_node`'s `exact` tier, separated only by iteration order.
|
||||
"""
|
||||
nodes = [
|
||||
{"id": "chat_metrics_port", "label": "MetricsPort",
|
||||
"source_file": "services/chat/src/application/ports/metrics.port.ts",
|
||||
"community": 0},
|
||||
{"id": "scraping_metrics_port", "label": "MetricsPort",
|
||||
"source_file": "services/scraping/src/application/ports/metrics.port.ts",
|
||||
"community": 0},
|
||||
]
|
||||
graph_data = {
|
||||
"directed": False, "multigraph": False, "graph": {},
|
||||
"nodes": list(reversed(nodes)) if reverse else nodes,
|
||||
"links": [],
|
||||
}
|
||||
p = tmp_path / ("graph_rev.json" if reverse else "graph.json")
|
||||
p.write_text(json.dumps(graph_data))
|
||||
return p
|
||||
|
||||
|
||||
def _run_expect_exit(monkeypatch, graph_path, label, capsys):
|
||||
monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None)
|
||||
monkeypatch.setattr(mainmod.sys, "argv",
|
||||
["graphify", "explain", label, "--graph", str(graph_path)])
|
||||
try:
|
||||
mainmod.main()
|
||||
except SystemExit as exc:
|
||||
return capsys.readouterr().out, exc.code
|
||||
return capsys.readouterr().out, None
|
||||
|
||||
|
||||
def test_explain_ambiguous_label_lists_every_candidate(monkeypatch, tmp_path, capsys):
|
||||
p = _write_ambiguous_graph(tmp_path)
|
||||
out, code = _run_expect_exit(monkeypatch, p, "MetricsPort", capsys)
|
||||
assert "Ambiguous" in out
|
||||
assert "services/chat/src/application/ports/metrics.port.ts" in out
|
||||
assert "services/scraping/src/application/ports/metrics.port.ts" in out
|
||||
assert code == 1
|
||||
# It must not present one file as the answer.
|
||||
assert "Node: MetricsPort\n ID:" not in out
|
||||
|
||||
|
||||
def test_explain_ambiguous_answer_does_not_depend_on_node_order(
|
||||
monkeypatch, tmp_path, capsys
|
||||
):
|
||||
"""The bug: reversing node order flipped which file was reported as fact."""
|
||||
forward, _ = _run_expect_exit(
|
||||
monkeypatch, _write_ambiguous_graph(tmp_path), "MetricsPort", capsys)
|
||||
reverse, _ = _run_expect_exit(
|
||||
monkeypatch, _write_ambiguous_graph(tmp_path, reverse=True), "MetricsPort", capsys)
|
||||
assert "Ambiguous" in forward and "Ambiguous" in reverse
|
||||
# Same candidate set either way, regardless of iteration order.
|
||||
assert sorted(l.strip() for l in forward.splitlines() if "metrics.port.ts" in l) == \
|
||||
sorted(l.strip() for l in reverse.splitlines() if "metrics.port.ts" in l)
|
||||
|
||||
|
||||
def test_explain_matches_within_one_file_are_not_ambiguous(monkeypatch, tmp_path, capsys):
|
||||
"""A file node plus its members is ordinary precedence, not a tie."""
|
||||
source_file = "services/chat/src/application/ports/metrics.port.ts"
|
||||
graph_data = {
|
||||
"directed": False, "multigraph": False, "graph": {},
|
||||
"nodes": [
|
||||
{"id": "file_node", "label": "metrics.port.ts",
|
||||
"source_file": source_file, "source_location": "L1", "community": 0},
|
||||
{"id": "member", "label": "MetricsPort",
|
||||
"source_file": source_file, "source_location": "L4", "community": 0},
|
||||
],
|
||||
"links": [{"source": "file_node", "target": "member",
|
||||
"relation": "contains", "confidence": "EXTRACTED"}],
|
||||
}
|
||||
p = tmp_path / "graph.json"
|
||||
p.write_text(json.dumps(graph_data))
|
||||
out = _run(monkeypatch, p, "MetricsPort", capsys)
|
||||
assert "Ambiguous" not in out
|
||||
assert "Node: MetricsPort" in out
|
||||
|
||||
Reference in New Issue
Block a user