Skip to content

fix(serve): prefer exact label match in MCP shortest_path / query_graph#879

Open
josephmcknight-bot wants to merge 2 commits into
safishamsi:mainfrom
josephmcknight-bot:feature/exact-label-match
Open

fix(serve): prefer exact label match in MCP shortest_path / query_graph#879
josephmcknight-bot wants to merge 2 commits into
safishamsi:mainfrom
josephmcknight-bot:feature/exact-label-match

Conversation

@josephmcknight-bot
Copy link
Copy Markdown

Why

Today an MCP call like:

shortest_path('Repo content audit - 2026-05-15', X)

silently coerces the source to the audit() function node (a high-degree substring hit) instead of the literal audit document the caller meant. No warning is emitted; the path summary just shows an unrelated walk through code. Operator-impacting - cost ~1 hour of misdiagnosis during a real audit before get_node was used to pin the actual node ID. Any downstream agent using shortest_path to verify architectural coupling is exposed to the same silent-coercion failure.

What changed

Three changes in graphify/serve.py (file-line references against main HEAD; in published graphifyy==0.7.15 the lines are 235 / 54 / 451 respectively):

  1. _find_node exact-then-substring (serve.py:96 here). Returns exact label matches first, falls back to substring only when no exact hit exists. Mirrors how _tool_get_node already orders its matches. Strips trailing () so a query for audit still matches the function-style label audit().

  2. _score_nodes exact-match bonus (serve.py:34 here). Applies a +10 bonus for exact label match. The bonus dwarfs the substring (+1) and source-file (+0.5) weights, so the literal audit node strictly outranks audit_log, audit_trail, and audit_runner substring matches.

  3. _tool_shortest_path echoes resolved labels (serve.py:266 here). Output now starts with:

    Resolving 'audit' -> node 'audit'
    Resolving 'cluster' -> node 'cluster'
    Shortest path (...)
    

    so any remaining substring coercion is visible to the caller instead of disappearing into the path summary.

Refactor

The shortest_path closure body is extracted into a new _build_shortest_path_handler(G) factory so the closure can be unit-tested without spinning up an MCP stdio server. serve() invokes the factory at the same point the inline definition used to live; runtime behaviour is identical.

Tests

6 new cases added to tests/test_serve.py (23 serve tests total, all green):

  • test_find_node_prefers_exact_match_over_substring - literal audit ranks ahead of audit_log / audit().
  • test_find_node_falls_back_to_substring_when_no_exact_match - regression guard for the fallback path.
  • test_score_nodes_exact_match_outranks_substring - asserts strict (not tied) lead.
  • test_score_nodes_exact_match_handles_function_suffix - audit matches audit().
  • test_resolve_label_returns_label_for_resolved_node - helper, including fallback to ID.
  • test_tool_shortest_path_echoes_resolved_labels - asserts the new Resolving ... -> node ... prefix.

Full suite: 258 passed / 41 pre-existing failures (all ModuleNotFoundError on optional [mcp,pdf,watch] extras when the environment lacks graspologic/pypdf/etc; identical to the main baseline before this PR).

Out of scope / non-goals

  • _tool_get_node already orders exact-then-substring correctly. Not changed.
  • No new MCP tools added.
  • No MCP tool schemas changed. Existing clients work unchanged - the output format change in shortest_path is additive (two prefix lines), and the resolved labels are now correct rather than coerced.

Notes

  • TDD: failing tests committed before the implementation (commit f04c912); implementation in commit e7cbfb9.
  • The +10 exact-match bonus is conservative; a single substring match already scores at most ~1.5 (label + source_file). Tunable if maintainers prefer a different weight.

…esolved-label echo

Three new behaviors covered (all currently failing):

- _find_node('audit') must rank the literal 'audit' node ahead of
  substring matches like 'audit_log' / 'audit()'. Falls back to
  substring only when no exact hit exists.
- _score_nodes must apply an exact-match bonus so the literal label
  strictly outranks substring matches, including function-suffix
  labels like 'audit()'.
- _tool_shortest_path must echo resolved labels (Resolving 'X' ->
  node 'Y') so silent substring coercion is visible to callers.
  Surfaced via a new _build_shortest_path_handler factory so the
  closure is testable without spinning up an MCP stdio server.
Today an MCP call like:

    shortest_path('Repo content audit - 2026-05-15', X)

silently coerces the source to the 'audit()' function node (a
high-degree substring hit) instead of the literal audit document the
caller meant. No warning. Operator-impacting - cost ~1 hour of
misdiagnosis during a recent audit before get_node was used to pin
the actual node ID.

Three changes in graphify/serve.py:

1. _find_node now returns exact label matches first, falling back to
   substring only when no exact hit exists. Mirrors how _tool_get_node
   already orders matches. Strips trailing '()' so a query for 'audit'
   still matches the function-style label 'audit()'.

2. _score_nodes applies a +10 bonus for exact label match. The bonus
   dwarfs the substring (+1) and source-file (+0.5) weights, so the
   literal 'audit' node strictly outranks 'audit_log', 'audit_trail',
   and 'audit_runner' substring matches.

3. _tool_shortest_path echoes resolved labels:

       Resolving 'audit' -> node 'audit'
       Resolving 'cluster' -> node 'cluster'
       Shortest path (...)

   so any remaining substring coercion is visible to the caller
   instead of disappearing into the path summary.

Refactor: the shortest_path closure body is extracted to
_build_shortest_path_handler(G) so it can be unit-tested without
spinning up an MCP stdio server. serve() invokes the factory at the
same place the inline definition used to live; behaviour is identical.

Tests: 6 new cases in tests/test_serve.py (23 serve tests total,
all green). Full suite: 258 passed / 41 pre-existing failures (all
missing-optional-deps; identical to baseline). No MCP tool schemas
change - existing clients work unchanged.

Out of scope: get_node already orders exact-then-substring correctly
and is unchanged. No new MCP tools added.
Copy link
Copy Markdown
Owner

@safishamsi safishamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Unfortunately this PR was built against a stale baseline and would regress several improvements that landed on main since the branch was cut.

Specifically, main already has:

  • 3-tier priority scoring (1000/100/1 weights for exact/prefix/substring matches) — the scoring in this PR would replace that with a simpler scheme that produces worse ranking
  • Diacritic-insensitive normalization — this PR's normalization step doesn't include it, so accented characters would fall back to lower quality matches
  • Ambiguity warnings — main surfaces uncertain matches to the user; this PR silently drops them

Rather than closing, I'd suggest rebasing onto current v8 (or main) and only keeping the parts of this PR that add new behavior not already present. Happy to point out exactly which hunks to keep if that would help — just let us know.

@safishamsi
Copy link
Copy Markdown
Owner

Thanks for this fix - the exact-match bonus and test structure are excellent. However this PR is based on main and conflicts with v8's current _find_node and _score_nodes which already have: three-tier (exact/prefix/substring) ordering, diacritic stripping via _strip_diacritics, and IDF-weighted scoring. Could you rebase onto v8 and:\n1. Preserve the prefix tier and diacritic normalization\n2. Layer your exact-match bonus (+10) on top of the IDF score\n3. Keep the _build_shortest_path_handler(G) refactor and all six tests\n\nAlso note that #994 (now merged) added a _search_tokens helper that strips punctuation in _find_node — worth preserving that too. The core fix is definitely wanted, just needs reconciliation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants