From 2ca565ac1ad21d6647939943bfd7b1d186f6e0ee Mon Sep 17 00:00:00 2001 From: 0bLoM Date: Wed, 29 Jul 2026 14:19:12 +0000 Subject: [PATCH] fix: explain no longer resolves an ambiguous name to an arbitrary file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_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. --- graphify/cli.py | 10 ++++- graphify/serve.py | 61 +++++++++++++++++++++++++--- tests/test_explain_cli.py | 84 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 6 deletions(-) diff --git a/graphify/cli.py b/graphify/cli.py index dc9ed08..932aebd 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -1289,7 +1289,7 @@ def dispatch_command(cmd: str) -> None: if len(sys.argv) < 3: print('Usage: graphify explain "" [--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)}") diff --git a/graphify/serve.py b/graphify/serve.py index f785456..1a44c78 100644 --- a/graphify/serve.py +++ b/graphify/serve.py @@ -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: diff --git a/tests/test_explain_cli.py b/tests/test_explain_cli.py index 94ba22b..60b3e62 100644 --- a/tests/test_explain_cli.py +++ b/tests/test_explain_cli.py @@ -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