Skip to content

feat: add v2 navigation tools (PR-V2-1)#49

Merged
HumanBean17 merged 2 commits into
masterfrom
feat/mcp-v2-tools
May 7, 2026
Merged

feat: add v2 navigation tools (PR-V2-1)#49
HumanBean17 merged 2 commits into
masterfrom
feat/mcp-v2-tools

Conversation

@HumanBean17
Copy link
Copy Markdown
Owner

Scope

Implement plans/PLAN-MCP-API-V2.md § PR-V2-1 by adding v2 navigation tools (search, find, describe, neighbors) alongside the existing v1 surface.

Plan link: plans/PLAN-MCP-API-V2.md § PR-V2-1

What Changed

  • Added new mcp_v2.py with shared NodeFilter, output DTOs, _node_kind_from_id, and search_v2/find_v2/describe_v2/neighbors_v2 handlers.
  • Registered 4 new MCP tools in server.py after v1 registrations: search, find, describe, neighbors.
  • Added README.md subsection ### v2 navigation tools (preview) right after the v1 tool reference.
  • Added tests/test_mcp_v2.py with PR-V2-1 unit coverage plus one extra regression test for required find.filter validation.
  • Added tests/test_mcp_v2_equivalence.py with the 14 prescribed v1↔v2 equivalence tests.

Semantics / Non-Goals

  • describe_v2.edge_summary remains None in this PR (PR-V2-2 scope).
  • search_v2.symbol_id consistency improvements beyond current behavior are deferred to PR-V2-2 scope.
  • _graph_meta_output per-edge-type count expansion is not included (PR-V2-2 scope).
  • v1 tool registrations remain unchanged in this PR.
  • No ontology/schema bump.

Validation

Lint

  • Not requested by plan prompt for PR-V2-1.

Tests

  • python -m pytest tests -q
  • Result: 360 passed, 4 skipped
  • New-test delta: baseline +33 new (tests/test_mcp_v2.py +19, tests/test_mcp_v2_equivalence.py +14). The extra +1 test is an intentional regression guard for missing find.filter validation.

Additional checks

  • python -m pytest tests/test_mcp_v2.py tests/test_mcp_v2_equivalence.py -q ✅ (33 passed)
  • python build_ast_graph.py --source-root tests/bank-chat-system --kuzu-path /tmp/mcp_v2_pr_graph --verbose

Sentinel checks

  • rg "trace_v2|ask_v2|impact_v2" mcp_v2.py -> no matches
  • rg "@mcp\.tool" server.py -> 27 registrations
  • rg "def (describe_v2|neighbors_v2)\(" mcp_v2.py -> both handlers present, no kind argument

Manual evidence

  • Tool count
    • rg "@mcp\.tool" server.py | wc -l
    • Observed: 27
  • No kind argument on describe/neighbors
    • rg "def (describe_v2|neighbors_v2)\(" mcp_v2.py
    • Observed: def describe_v2(id: str, graph: KuzuGraph | None = None) and def neighbors_v2(ids: str | list[str], ...)
  • End-to-end v2 chain (adapted to local fixture state: graph-first flow)
    • python -c "from mcp_v2 import find_v2, describe_v2, neighbors_v2; from kuzu_queries import KuzuGraph; g=KuzuGraph('/tmp/mcp_v2_pr_graph'); hits=find_v2('symbol', {'role':'SERVICE'}, limit=3, offset=0, graph=g); sid=hits.results[0].id; print('symbol_id:', sid); rec=describe_v2(sid, graph=g); print('kind:', rec.record.kind, 'fqn:', rec.record.fqn); edges=neighbors_v2(sid, direction='in', edge_types=['CALLS'], limit=10, offset=0, filter=None, graph=g); print('callers:', len(edges.results))"
    • Observed: printed symbol_id, kind/fqn, and caller count shape as expected.

Out of Scope Confirmed

Did not implement:

  • describe_v2.edge_summary population
  • search_v2 full symbol-id consistency sweep
  • _graph_meta_output edge-type count expansion
  • v1 registration deletion/removal
  • CLI extraction (user_rag/cli.py, pyproject.toml packaging changes)
  • ontology/version bump

Definition of Done

  • All listed deliverables for this PR are shipped.
  • Required tests pass locally with recorded command output.
  • Sentinel checks produce expected results.
  • Only in-scope files are modified.
  • PR description includes scope, validation, and manual evidence.
  • PR targets master with agreed title and branch naming.

Made with Cursor

Implement the v2 search/find/describe/neighbors surface alongside v1 with shared filter models and parity-focused tests so migration can proceed with confidence.

Co-authored-by: Cursor <cursoragent@cursor.com>
@HumanBean17
Copy link
Copy Markdown
Owner Author

Review: PR-V2-1 — feat: add v2 navigation tools

Verdict: Approved ✅

PR is on-spec for plans/PLAN-MCP-API-V2.md § PR-V2-1. All four primitives land, the four MCP tool registrations are added cleanly behind v1, no schema/ontology changes, no v1 deletions. NodeFilter has the corrected 15 keys (3 universal + 5 symbol + 3 route + 4 client) — matches the post-review-nit propose. Tests green.

Scope discipline (out-of-scope checks)

Sentinel Status
_chunk_to_symbol_id (PR-V2-2 helper) ✅ 0 matches
edge_counts (PR-V2-2 graph_meta) ✅ 0 matches
edge_summary populated (PR-V2-2) ✅ 2 hits — both are the deferred stub: field declared `dict
user_rag/cli, pyproject.toml (PR-V2-4) ✅ 0 matches; no packaging changes
ONTOLOGY_VERSION, CREATE NODE TABLE, CREATE REL TABLE, DROP TABLE ✅ 0 matches; no schema work
trace_v2, ask_v2, impact_v2 (dropped designs) ✅ 0 matches
v1 @mcp.tool deletions (PR-V2-3) ✅ 0 deletions; only 4 additions
build_ast_graph.py / kuzu_queries.py touched ✅ unchanged

git diff master...HEAD --stat: 5 files (README.md, mcp_v2.py, server.py, tests/test_mcp_v2.py, tests/test_mcp_v2_equivalence.py) — exactly the deliverables in the plan, nothing extra.

Plan compliance

# Step from plan Verified
1 New mcp_v2.py with NodeFilter, output models, four *_v2 handlers, _node_kind_from_id ✅ all present (mcp_v2.py:32-46 for filter, :109 for kind helper, :290/336/381/400 for handlers)
2 NodeFilter has 15 optional keys (3 universal + 5 symbol + 3 route + 4 client) ✅ exact match; microservice/module/source_layer universal, role/exclude_roles/annotation/capability/fqn_prefix symbol, http_method/path_prefix/framework route, client_kind/target_service/target_path_prefix/client_method client
3 direction and edge_types REQUIRED on neighbors_v2 (no defaults) ✅ both use Field(...) to enforce required at validation; covered by test_neighbors_missing_direction_rejected / test_neighbors_missing_edge_types_rejected
4 find_v2.filter REQUIRED (no default) filter: NodeFilter | dict[str, Any] positional, no default; covered by extra regression test test_find_missing_filter_rejected
5 id is kind-agnostic — internal prefix dispatch on describe_v2 _node_kind_from_id (mcp_v2.py:109) + _resolve_node_kind (:119)
6 neighbors_v2 accepts ids: str | list[str] and returns Edge objects with attrs ✅ signature line 401; test_neighbors_batch_ids_carries_origin_id exercises batch + origin-id propagation
7 Register 4 new @mcp.tool blocks after v1 in server.py ✅ at server.py:1674-1740, after the existing v1 block; pre-PR registrations untouched
8 README adds ### v2 navigation tools (preview) after v1 tool reference README.md:179-189; explicitly notes "will replace v1 in PR-V2-3"
9 18 tests in test_mcp_v2.py per plan enumeration ✅ collected count is 19 — 18 prescribed + 1 bonus regression test (test_find_missing_filter_rejected); transparent in PR description
10 14 prescribed equivalence tests in test_mcp_v2_equivalence.py ✅ all 14 present (test_eq_codebase_search is async def — easy to miss with a ^def grep, but it's there at line 64; pytest collects 14)

Tests

360 passed, 4 skipped in 82.58s

Delta vs baseline (master at 6d9571e, 327 passed): +33 new = 19 (tests/test_mcp_v2.py) + 14 (tests/test_mcp_v2_equivalence.py). Matches the PR claim of "baseline +33 new"; the +1 over the plan-prescribed 32 is the deliberate test_find_missing_filter_rejected regression guard. PR description is fully consistent with the actual collection.

Manual evidence reproduced

Rebuilt the fixture graph at /tmp/v21_check, then ran the PR's graph-first end-to-end chain:

from mcp_v2 import find_v2, describe_v2, neighbors_v2
from kuzu_queries import KuzuGraph
g = KuzuGraph('/tmp/v21_check')
hits = find_v2('symbol', {'role': 'SERVICE'}, limit=3, offset=0, graph=g)
sid = hits.results[0].id
rec = describe_v2(sid, graph=g)
edges = neighbors_v2(sid, direction='in', edge_types=['CALLS'], limit=10, offset=0, filter=None, graph=g)

Output:

find symbols: 3
symbol_id: a0af47515c78a6f764922496fc865c3ab7ba2f59
kind: symbol | fqn: com.bank.chat.assign.service.ChatManagementService | edge_summary: None
callers count: 0

find returns 3 SERVICE symbols. ✅ describe returns kind=symbol, valid fqn, edge_summary=None (correct deferral to V2-2). ✅ neighbors(in, [CALLS]) runs cleanly. The 0-callers result is fixture-truthful (the picked SERVICE has no inbound CALLS edges in bank-chat-system); the chain shape is what matters per the manual-evidence disclaimer in the cursor prompts.

Sentinel sanity: grep -cE "@mcp.tool" server.py27 (23 v1 + 4 v2) ✅.

Notes that earned my trust

  • Disciplined deferral of edge_summary. The field is declared on NodeRecord and describe_v2 returns it as None everywhere — no half-implementation, no scaffolding leakage into PR-V2-2's territory. The same goes for _chunk_to_symbol_id (deferred entirely, not stubbed).
  • Bonus regression test. test_find_missing_filter_rejected isn't on the plan but defends the "filter required" decision against accidental future regressions; flagged transparently in the PR description as +1 over baseline.
  • NodeFilter shape exactly matches the post-review-nit propose (15 keys, not 14). The agent picked up the corrected spec from the merged PR Propose: MCP API v2 — 4-tool graph navigator + ops CLI #48, not the older draft.
  • MCP tool registrations are minimal wrappers. Each @mcp.tool async function delegates to mcp_v2.<handler>_v2 via asyncio.to_thread — no logic duplication between the MCP boundary and the pure handlers, which keeps the equivalence/unit tests honest (they call handlers directly).

Observations (non-blocking)

  1. Field(...) as a function-arg default in mcp_v2.py:402-403 is unusual — Field is normally a Pydantic-model construct, and using it as a function default value works because FastMCP's @mcp.tool decorator picks it up during validation, but it's surprising for readers who call neighbors_v2 directly from Python. The two test_neighbors_missing_*_rejected tests confirm it raises ValidationError even on direct calls, so the contract is honored. Consider — in PR-V2-2 or V2-3 — adding a one-line comment near these defaults explaining "this Pydantic Field marker is intentional; both ensures MCP-boundary validation and Python-call validation". Non-blocking.
  2. describe MCP arg shadows builtin id. Plan prescribes id, the agent followed the plan, and the linter probably says nothing about decorator-arg shadowing — but worth a mental note. No action.
  3. _resolve_node_kind (mcp_v2.py:119) does a graph lookup even when the prefix would have been enough; this is correct (verifies the id actually exists) but is one extra round-trip per call. If describe/neighbors show up in latency budgets later, the prefix-only fast path is an obvious win. Non-blocking; deferred for a follow-up if profiling demands.

Plan deltas needed

None. The plan is internally consistent with what shipped; the +1 bonus test is a quality improvement, not a deviation worth re-recording.


Ready to merge. Next: PR-V2-2 (feat/mcp-v2-compose) — adds edge_summary population, _chunk_to_symbol_id helper, and _graph_meta_output.edge_counts per plans/PLAN-MCP-API-V2.md § PR-V2-2.

@HumanBean17
Copy link
Copy Markdown
Owner Author

Thanks — captured the non-blocking review notes as explicit follow-ups:

Follow-ups (deferred intentionally)

  1. neighbors_v2 required args via Field(...) defaults (mcp_v2.py)

    • Action: add a short clarifying comment near the signature that this is intentional for MCP + direct-call validation parity.
    • Target: PR-V2-2 (documentation/readability touch only; no behavior change).
  2. id parameter name on describe MCP wrapper shadows builtin

    • Action: keep as-is for now (plan/prompt contract uses id), reassess at cutover when API surface stabilizes.
    • Target: PR-V2-3 discussion checkpoint (decide whether to keep id permanently for API consistency).
  3. _resolve_node_kind extra existence round-trips

    • Action: treat as perf follow-up only if profiling shows describe/neighbors hot-path impact; otherwise keep correctness-first behavior.
    • Target: PR-V2-2 (measure while touching compose/perf-adjacent code); if needed, spin into separate perf micro-PR.

No blockers for this PR; keeping current scope unchanged.

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.

1 participant