explain: add path::Symbol resolution and fix the ambiguity retry hint - #3491
explain: add path::Symbol resolution and fix the ambiguity retry hint#3491ayushcodes10 wants to merge 3 commits into
Conversation
A bare symbol name that exists in multiple files is ambiguous, and the only suggested retry (a bare repo relative path) resolves to the file node itself, never the symbol, so it could not actually disambiguate anything. This adds a path::Symbol form that restricts the label match to nodes defined in that file, wired into _find_node_tiers ahead of the ordinary tiers so it is available to explain, get_node and get_neighbors alike. Fixes Graphify-Labs#3485. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
The prior retry suggestion, a bare repo relative path, resolves to the file node rather than the symbol, so following it never actually disambiguates anything. Now that path::symbol exists, point both the explain CLI and the shared MCP resolver at it, using a generic placeholder rather than one of the rival paths so the message stays the same regardless of node iteration order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Formal verification. 2 change(s) alter behavior, breaking input(s) attached.
Behavior changes: \_find\_node\_tiers changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_find\_node\_tiers behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"\(lambda \_g: \(\_g\.add\_nodes\_from\(\[\('a', \{'label': 'A', 'kind': 'fn'\}\), \('b', \{'label': 'B'\}\)\]\), \_g\.add\_edges\_from\(\[\('a', 'b', \{'weight': 2, 'kind': 'calls'\}\)\]\), \_g\)\[\-1\]\)\(\_\_import\_\_\('networkx'\)\.Graph\(\)\)","label":"'\)'"\}, the old code produced \[\] but the new code produces \(\[\], \[\], \[\], \[\]\). Paste that input straight into a regression test.
Behavior changes: \_resolve\_single\_node changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_resolve\_single\_node behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"\(lambda \_g: \(\_g\.add\_nodes\_from\(\[\('a', \{'label': 'A', 'kind': 'fn'\}\), \('b', \{'label': 'B'\}\)\]\), \_g\.add\_edges\_from\(\[\('a', 'b', \{'weight': 2, 'kind': 'calls'\}\)\]\), \_g\)\[\-1\]\)\(\_\_import\_\_\('networkx'\)\.Graph\(\)\)","label":"'\)'"\}, the old code produced raises ValueError but the new code produces \(None, "No node matching '\)' found\."\). Paste that input straight into a regression test.
Graphify review — findings
Adds a path::Symbol query form to node resolution: _find_node_tiers now intercepts labels containing ::, resolves them via _resolve_path_scoped_symbol against both source file and label/id, and returns the result as a source_exact match so a same-named symbol in a specific file is reachable without its opaque id. When the path or symbol part matches nothing, it falls through to ordinary matching, so genuine :: labels (Rust modules, C++ namespaces) still resolve. Updates the ambiguity retry hint in both the CLI and _resolve_single_node to suggest <path>::symbol instead of a bare repo-relative path, which resolved to the file node rather than the symbol.
Worth a look
- '::'-containing label that is a legitimate symbol name may be captured by path-scoping and return empty, changing resolution —
graphify/serve.py:1405· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Rust/C++ '::' label no longer resolves when path_part matches a file —
graphify/serve.py:1407· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 625 functions depend on the 218 functions this change touches.
Health — this change adds coupling hotspots:
- new:
_query_graph_text()— 25 callers, 10 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
_score_query()— 15 callers, 6 callees - new:
_query_terms()— 20 callers, 3 callees - new:
run_benchmark()— 16 callers, 3 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_load_graph()— 14 callers, 3 callees - new:
_build_server()— 2 callers, 16 callees - …and 11 more — each is listed as a finding
Verification — 625 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 581 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
31 of 268 test file(s) selected (12%) via static blast radius.
tests/test_affected_cli.py— impacttests/test_agents_platform.py— impacttests/test_benchmark.py— impacttests/test_benchmark_raw_graph.py— impacttests/test_codebuddy.py— impacttests/test_devin.py— impacttests/test_explain_cli.py— impact, changed-testtests/test_extract_cli.py— impacttests/test_file_label_disambiguation.py— impacttests/test_global_add_tag_inference.py— impacttests/test_god_nodes_cli.py— impacttests/test_hollow_chunks_arm_shrink_guard.py— impacttests/test_hook_guard_token_match.py— impacttests/test_hook_out_of_project_paths.py— impacttests/test_hook_strict.py— impacttests/test_incomplete_build_guard.py— impacttests/test_install.py— impacttests/test_install_references.py— impacttests/test_merge_chunks_validation.py— impacttests/test_multigraph_diagnostics.py— impacttests/test_no_dedup_flag.py— impacttests/test_partial_cache.py— impacttests/test_path_cli.py— impacttests/test_query_cli.py— impacttests/test_query_induced_edges.py— impacttests/test_query_mcp_direction.py— impacttests/test_query_names_its_graph.py— impacttests/test_serve.py— impacttests/test_serve_http.py— impacttests/test_stale_prune.py— impacttests/test_unverified_semantic_shrink.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Behavior changes: \_find\_node\_tiers changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_find\_node\_tiers behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"\(lambda \_g: \(\_g\.add\_nodes\_from\(\[\('a', \{'label': 'A', 'kind': 'fn'\}\), \('b', \{'label': 'B'\}\)\]\), \_g\.add\_edges\_from\(\[\('a', 'b', \{'weight': 2, 'kind': 'calls'\}\)\]\), \_g\)\[\-1\]\)\(\_\_import\_\_\('networkx'\)\.Graph\(\)\)","label":"'\)'"\}, the old code produced \[\] but the new code produces \(\[\], \[\], \[\], \[\]\). Paste that input straight into a regression test.
Behavior changes: \_resolve\_single\_node changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_resolve\_single\_node behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"\(lambda \_g: \(\_g\.add\_nodes\_from\(\[\('a', \{'label': 'A', 'kind': 'fn'\}\), \('b', \{'label': 'B'\}\)\]\), \_g\.add\_edges\_from\(\[\('a', 'b', \{'weight': 2, 'kind': 'calls'\}\)\]\), \_g\)\[\-1\]\)\(\_\_import\_\_\('networkx'\)\.Graph\(\)\)","label":"'\)'"\}, the old code produced raises ValueError but the new code produces \(None, "No node matching '\)' found\."\). Paste that input straight into a regression test.
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 19 more finding(s) on lines outside this diff (see the check run).
A native Rust or C++ label containing "::" could be hijacked by the new path::Symbol resolution when the substring before "::" happens to match some unrelated file elsewhere in the graph, for example an extensionless file literally named the same as a Rust module. The query would then resolve to a node in that unrelated file instead of the node the literal label actually names. Path scoped resolution now only runs when the raw, unsplit label does not already have a literal exact match among real node labels, since a genuine label essentially never equals a whole path plus symbol string verbatim. Found by the graphify review bot on PR 3491. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
|
Thanks for the review. Went through both advisory findings and the formal verification section. Both "Worth a look" findings were real — reproduced the collision directly: a native The formal verification section's two "behavior changes" are intentional, not regressions — both are the same underlying fix: |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. 2 change(s) alter behavior, breaking input(s) attached.
Behavior changes: \_find\_node\_tiers changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_find\_node\_tiers behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"\(lambda \_g: \(\_g\.add\_nodes\_from\(\[\('a', \{'label': 'A', 'kind': 'fn'\}\), \('b', \{'label': 'B'\}\)\]\), \_g\.add\_edges\_from\(\[\('a', 'b', \{'weight': 2, 'kind': 'calls'\}\)\]\), \_g\)\[\-1\]\)\(\_\_import\_\_\('networkx'\)\.Graph\(\)\)","label":"'\)'"\}, the old code produced \[\] but the new code produces \(\[\], \[\], \[\], \[\]\). Paste that input straight into a regression test.
Behavior changes: \_resolve\_single\_node changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_resolve\_single\_node behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"\(lambda \_g: \(\_g\.add\_nodes\_from\(\[\('a', \{'label': 'A', 'kind': 'fn'\}\), \('b', \{'label': 'B'\}\)\]\), \_g\.add\_edges\_from\(\[\('a', 'b', \{'weight': 2, 'kind': 'calls'\}\)\]\), \_g\)\[\-1\]\)\(\_\_import\_\_\('networkx'\)\.Graph\(\)\)","label":"'\)'"\}, the old code produced raises ValueError but the new code produces \(None, "No node matching '\)' found\."\). Paste that input straight into a regression test.
Graphify review — findings
Adds a path::Symbol query form to node lookup so an ambiguous symbol name can be pinned to a specific file without knowing its opaque node id; _find_node_tiers splits on ::, matches nodes whose source_file and label both satisfy the two parts via _resolve_path_scoped_symbol, and returns them in the source_exact tier so existing callers are unchanged. Guards against hijacking native :: labels (Rust modules, C++ namespaces) by first checking whether the raw string literally matches a node's own label via _label_has_literal_exact_match, and falls through to ordinary matching when the path-scoped resolution finds nothing. Updates the ambiguity messages in both the CLI and _resolve_single_node to suggest the new path::symbol retry form instead of the bare repo-relative path that resolved to the file node.
Worth a look
- rstrip('()') strips all trailing parens/chars, not a trailing '()' pair —
graphify/serve.py:1372· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 629 functions depend on the 222 functions this change touches.
Health — this change adds coupling hotspots:
- new:
_query_graph_text()— 25 callers, 10 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
_score_query()— 15 callers, 6 callees - new:
_query_terms()— 20 callers, 3 callees - new:
run_benchmark()— 16 callers, 3 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_load_graph()— 14 callers, 3 callees - new:
_build_server()— 2 callers, 16 callees - …and 11 more — each is listed as a finding
Verification — 629 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 585 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
31 of 268 test file(s) selected (12%) via static blast radius.
tests/test_affected_cli.py— impacttests/test_agents_platform.py— impacttests/test_benchmark.py— impacttests/test_benchmark_raw_graph.py— impacttests/test_codebuddy.py— impacttests/test_devin.py— impacttests/test_explain_cli.py— impact, changed-testtests/test_extract_cli.py— impacttests/test_file_label_disambiguation.py— impacttests/test_global_add_tag_inference.py— impacttests/test_god_nodes_cli.py— impacttests/test_hollow_chunks_arm_shrink_guard.py— impacttests/test_hook_guard_token_match.py— impacttests/test_hook_out_of_project_paths.py— impacttests/test_hook_strict.py— impacttests/test_incomplete_build_guard.py— impacttests/test_install.py— impacttests/test_install_references.py— impacttests/test_merge_chunks_validation.py— impacttests/test_multigraph_diagnostics.py— impacttests/test_no_dedup_flag.py— impacttests/test_partial_cache.py— impacttests/test_path_cli.py— impacttests/test_query_cli.py— impacttests/test_query_induced_edges.py— impacttests/test_query_mcp_direction.py— impacttests/test_query_names_its_graph.py— impacttests/test_serve.py— impacttests/test_serve_http.py— impacttests/test_stale_prune.py— impacttests/test_unverified_semantic_shrink.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Behavior changes: \_find\_node\_tiers changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_find\_node\_tiers behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"\(lambda \_g: \(\_g\.add\_nodes\_from\(\[\('a', \{'label': 'A', 'kind': 'fn'\}\), \('b', \{'label': 'B'\}\)\]\), \_g\.add\_edges\_from\(\[\('a', 'b', \{'weight': 2, 'kind': 'calls'\}\)\]\), \_g\)\[\-1\]\)\(\_\_import\_\_\('networkx'\)\.Graph\(\)\)","label":"'\)'"\}, the old code produced \[\] but the new code produces \(\[\], \[\], \[\], \[\]\). Paste that input straight into a regression test.
Behavior changes: \_resolve\_single\_node changes behavior, here is the input that shows it.
The verifier found a concrete input on which \_resolve\_single\_node behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"\(lambda \_g: \(\_g\.add\_nodes\_from\(\[\('a', \{'label': 'A', 'kind': 'fn'\}\), \('b', \{'label': 'B'\}\)\]\), \_g\.add\_edges\_from\(\[\('a', 'b', \{'weight': 2, 'kind': 'calls'\}\)\]\), \_g\)\[\-1\]\)\(\_\_import\_\_\('networkx'\)\.Graph\(\)\)","label":"'\)'"\}, the old code produced raises ValueError but the new code produces \(None, "No node matching '\)' found\."\). Paste that input straight into a regression test.
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 19 more finding(s) on lines outside this diff (see the check run).
|
Re the |
|
Shipped in v0.9.59 — landed on |
2 participants
Summary
Fixes #3485.
explain's ambiguity message told callers to retry with "the repo relative path", but a bare path resolves to the FILE node, not the symbol, and there was no query form that scoped a symbol to a specific file.path::Symbolquery form, resolved in_find_node_tiersbefore the ordinary tiers, soexplainand the MCP toolsget_node/get_neighbors(which share the same resolver) can all disambiguate a symbol by file.explainCLI and the shared MCP resolver to recommendpath::symbolinstead of the old, misleading suggestion, using a generic<path>::symbolplaceholder so the message stays identical regardless of node iteration order (there is an existing test guarding exactly that property).Test plan
tests/test_explain_cli.py: the exact ambiguous scenario from the issue resolves to the right node either way, a symbol not present in the given file falls through to "no match" cleanly, and a genuineFoo::Barstyle label (Rust/C++ conventions) still resolves via ordinary matching when nothing before::matches a file path.python3 -m pytest -q— 5423 passed, only the pre-existing unrelated failures (test_ollama_retry_cap.pymissingopenaiin this env, one flaky timing assertion intest_ts_import_type_arguments.py).python3 -m tools.skillgen --check— OK.🤖 Generated with Claude Code
https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh