app: find_references and list_imports MCP tools (#87) - #94
Conversation
Add find_references and list_imports as thin FastMCP tool registrations backed only by app/service.py builders. find_references_payload is unchanged; list_imports_payload gains an additive target/direction extension (imports|imported_by) with deterministic pre-DB structured validation. Extends the MCP e2e fixture, smoke checks, docstrings, and docs for the new tools and the "what tests cover symbol X" composition pattern.
IceRhymers
left a comment
IceRhymers
left a comment
There was a problem hiding this comment.
Review verdict: APPROVE (posted as a comment — GitHub blocks the PR author's own account from a formal Approve)
Note: this account (`IceRhymers`) authored the PR, so `gh pr review --approve` returns "Can not approve your own pull request." This is an APPROVE verdict with zero blocking/non-blocking findings; it is recorded as a comment only because of the self-approval restriction, not because anything needs changing. A formal green check requires a different reviewer account.
This is a separate reviewer pass from the implementation/pre-PR context. I re-verified the committed diff (HEAD `a1fe6f4`) against the Fable-approved binding plan's D1a–D8 decisions, AC1–AC4 mapping, and the non-negotiable constraints — not just trusting the prior review.
Gates re-run fresh on this branch
- `uv run ruff check .` → All checks passed
- `uv run mypy app indexer webui` → Success: no issues found in 35 source files
- `uv run pytest -m "unit or observability"` → 1049 passed, 231 deselected
(Integration/e2e need Postgres and were not re-run in this pass; I relied on the PR body's 177-passed / 25-25 MCP e2e report, but independently confirmed the `test_reference_tools_streamable_http` assertions are consistent with the seed and the resolver's branch/candidate scoping.)
Non-negotiable constraints — all 10 honored (verified via `git diff --stat 548c1a1..HEAD`)
No parser/grammar change · no migration · no `INDEX_SEMANTICS_VERSION` bump · no `webui/` change · no `app/search/references.py` resolver SQL change · `find_references_payload` byte-unchanged (only `_list_imports_error_payload` + `list_imports_payload` changed in service.py) · additive-only · deterministic ordering · branch behavior byte-identical (threaded as a resolver param) · `json.dumps` only via `_dispatch`.
Design decisions spot-checked against source
- D1a `find_references` thin wrapper over the unchanged builder; introduces no `repo_known` key (Critic note 5) — e2e asserts its absence.
- D1b `list_imports_payload` signature-compatible additive extension; existing positional callers still bind; `imported_by` routes `target_name=target, edge_kind="import", repo=None`, index-served by `ix_reference_edges_target_name` (confirmed against the read-only `resolve_references` signature) — no resolver change.
- Validation PRE-DB, structured payloads (never raises), no-DB-touch proven via `_FailingEngine`; `query` pinned to `""` (Critic note 3); `DEFAULT_ROW_LIMIT` imported from `app.query.compiler` (Critic note 1).
- D4 four additive None-safe `_signals` keys (test-pinned).
- D6 `validate_references_payload` shape-only/zero-sites-accepting yet rejects malformed envelopes (8 negatives); live leg adds a `find_references` call.
Prior optional-hardening nitpick
The untested unknown-repo + `imported_by`-narrowing combo is behavior-correct: `resolve_references` short-circuits `repo_id is None` → `repo_known=False`, and the builder surfaces `result.repo_known`. Optional hardening, not a defect.
Zero blocking or non-blocking issues. AC1–AC4 and the epic-#82 ACs covered; `direction=imported_by` shipped as planned. LGTM.
ba85a2c
into
integration/knowledge-graph-reference-edges
1 participant
Part of #82 / umbrella #89. Closes #87.
What & why
Exposes the reference-edge graph (shipped in #83–#86) to MCP agents:
find_referencesandlist_importstools on the existing FastMCP app (app/main.py), backed only byapp/service.pybuilders.Design
find_references(symbol, ctx, limit=200, branch=None)— thin tool wrapper over the unchangedservice.find_references_payloadbuilder (corpus-wide,edge_kind="call"fixed). Docstring states candidate-set (grep, not LSP) semantics explicitly.list_imports(ctx, repo=None, target=None, direction="imports", branch=None, limit=DEFAULT_ROW_LIMIT)— additive, signature-compatible extension ofservice.list_imports_payloadaddingtarget/direction. Both directions from the original issue sketch ship:direction="imports"(default):reporequired,targetoptional narrowing.direction="imported_by":targetrequired (index-served viaix_reference_edges_target_name, no resolver SQL change),repooptional narrowing.unsupported_direction/missing_repo/missing_target) lives in the service-layer builder — never raises, so future non-MCP callers (e.g. webui: graph tools parity and documentation #88 webui) inherit it for free.find_references+ client-side path filter, no new primitive — documented in the tool docstring, README, and the reference-edges runbook, and proven end-to-end._signals()keys:repo_known,unsupported_direction,missing_repo,missing_target.validate_references_payloadshape predicate (zero-sites-accepting, still rejects malformed envelopes) + a livefind_referencesleg inscripts/smoke.py --enable-mcp.Full binding design rationale: Fable Critic-approved plan (Planner→Architect→Critic consensus,
APPROVE2026-07-23).Explicit scope deferrals (recorded on issue #87)
find_references(repo?, kind?)filters — would force arepo_knownkey ontofind_references_payload, whichtests/unit/test_service.py:588currently pins absent. Left for a deliberate follow-up that breaks that pin as an additive contract change.Non-negotiable constraints honored
No parser/grammar change · no migration · no
INDEX_SEMANTICS_VERSIONbump · nowebui/change · noapp/search/references.py(resolver) change ·find_references_payloadbyte-unchanged · additive-only on existing payloads · deterministic ordering · byte-identical branch behavior ·json.dumpsonly via_dispatch.Acceptance-criteria mapping
app/main.py, backed only byapp/service.pybuilders) → thin tool wrappers, validation lives in the builder; wiring tests intests/unit/test_main.py; e2elist_tools()registration check.tests/path-filter case.validate_references_payload(unit-tested, 8 negatives) + livefind_referencesleg.direction=imported_by→ shipped (not deferred).Test plan
Fresh runs on this branch:
uv run ruff check .→ all checks passeduv run ruff format --check .→ clean (102 files)uv run mypy app indexer webui→ Success: no issues found in 35 source filesuv run pytest -m "unit or observability"→ 1049 passed, 231 deselecteduv run pytest -m "integration or e2e"(local Postgres,pgvector/pgvector:pg16) → 177 passed, 0 failures/errors in any file touched by this PR (tests/integration/test_service.py,tests/integration/test_mcp_server.py→ 25/25 passed). The 7 failures / 41 errors elsewhere are pre-existing Lakebase-operator environment gaps in unrelated files (test_migrations.py,test_reconcile.py,test_semantic_rrf.py,test_store_chunk_writer.py,test_webui_semantic.py,test_commit_search.py), verified identical on the base branch viagit stash.Review
Independent
code-reviewerpass against the binding plan and this diff: APPROVE, zero blocking/non-blocking findings (one optional-hardening nitpick, addressed as informational only — an untested unknown-repo +imported_bynarrowing combination that is otherwise plan-consistent and correctly handled by the existing resolver code path).Not merging this PR — leaving open for review per the epic #82 / umbrella #89 workflow.