I2.2 — Dependency MCP Adapter for v0.4.0 MCP vertical slice

[michaelegner]

Summary

• get_service_dependencies now has a real MCP dispatch body (spec i2-mcp-vertical-slice-and-evidence-drill-down.md §19 "I2.2 — Dependency MCP Adapter"): maps MCP input to the existing I1 ServiceDependenciesRequest, calls ArchitectureIntelligenceService.get_service_dependencies exactly once, and returns its answer unchanged as structuredContent (spec §10) — no new semantics, claims, or limitations are added.
• New app/mcp/wiring.py bridges MCP tool registration (import time, before FastAPI's lifespan exists) with the real Neo4j driver/Producer built during lifespan startup, without the adapter itself opening a session, running Cypher, or importing app.graph (spec §8 boundary).
• get_evidence remains an unimplemented stub (ToolError), unchanged — its dispatch is I2.3.

Notable finding

While implementing the failure-mapping behavior (spec §16), live inspection of the installed mcp SDK (mcp.server.mcpserver.tools.base.Tool.run) showed it already sanitizes any uncaught non-ToolError tool-body exception into a generic UnexpectedToolError("Error executing tool <name>") — never interpolating the raw exception text — and separately logs the full traceback server-side. So the adapter only needs to special-case pydantic.ValidationError (the service's own documented, deliberate signal for a malformed observation-context value, e.g. a reversed window) to preserve actionable client feedback; every other exception (e.g. a Neo4j connectivity failure whose message could embed the bolt URI/host) is already safely sanitized by the SDK with no adapter code needed. A regression test (test_unexpected_service_failure_is_sanitized_not_leaked) locks this in.

Test plan

• uv run ruff check . / uv run ruff format --check .
• uv run pytest tests/unit (709 passed)
• uv run pytest tests/integration (197 passed)
• New unit tests: test_mcp_service_dependencies_adapter.py (dispatch/error-mapping against a stub service, incl. the sanitization regression test), test_mcp_read_only_boundary.py (static AST check that app/mcp imports no app.graph/neo4j session code, per spec §17 item 17)
• New integration test: test_mcp_service_dependencies_equivalence.py (real Neo4j — direct-vs-MCP structured-content equivalence for a confirmed answer and a provider-only refusal, determinism across repeated calls, revision-fence unchanged across success/refusal/validation-error paths)
• Updated test_mcp_discovery.py's stale "not yet implemented" expectation for get_service_dependencies

🤖 Generated with Claude Code

https://claude.ai/code/session_011tBVicyuqNjpdRVBudmmaJ

[Copilot]

🟡 Changes recommended

app/mcp/wiring._current_git_sha() can crash application startup in containerized/non-git environments due to unconditional git rev-parse HEAD execution.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Implements the v0.4.0 I2.2 MCP “Dependency Adapter” vertical slice by wiring MCP tool dispatch to the existing ArchitectureIntelligenceService.get_service_dependencies without altering response semantics, and adds unit/integration coverage to lock in dispatch, sanitization, and read-only behavior.

Changes:

• Add real MCP dispatch for get_service_dependencies, including pydantic.ValidationError → actionable ToolError mapping while relying on SDK sanitization for unexpected failures.
• Introduce app/mcp/wiring.py + FastAPI lifespan wiring to bridge import-time tool registration with runtime-constructed Neo4j-backed services.
• Add comprehensive unit/integration tests for adapter behavior, schema/boundary constraints, and direct-vs-MCP equivalence.

File summaries

File	Description
tests/unit/test_mcp_service_dependencies_adapter.py	Unit coverage for get_service_dependencies dispatch behavior, error mapping, and sanitization regression.
tests/unit/test_mcp_read_only_boundary.py	AST-level guard that MCP adapter modules don’t import graph repositories / open sessions / embed Cypher.
tests/unit/test_mcp_discovery.py	Updates discovery tests to reflect I2.2 behavior and adds “unconfigured wiring” sanitization coverage.
tests/integration/test_mcp_service_dependencies_equivalence.py	End-to-end equivalence and revision-fence immutability checks against real Neo4j.
app/mcp/wiring.py	Lazy runtime wiring for the MCP tool to access a real ArchitectureIntelligenceService.
app/mcp/tools.py	Real tool body for get_service_dependencies + injectable get_service for testability; get_evidence remains stub.
app/main.py	Lifespan startup now configures the MCP wiring using the app’s Neo4j driver.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[michaelegner]

Reviewed current head 3d6d62f.

Verdict: REQUEST CHANGES.

The core I2.2 design is sound: the MCP tool delegates once to ArchitectureIntelligenceService, preserves the returned structured answer, keeps graph/session access outside the adapter, and adds useful direct-vs-MCP, read-only, determinism, and failure-path coverage. CI, dependency audit, and CodeQL are green. Two blockers remain:

1. Production container startup is broken by unconditional Git execution.

   app/mcp/wiring._current_git_sha() runs git rev-parse HEAD with check=True during FastAPI lifespan startup. The repository's production Dockerfile is based on python:3.13-slim, does not install Git, and copies only selected files—not .git. Therefore the normal application image will fail during startup before MCP or any existing HTTP endpoint can be used.

   Required fix: resolve the immutable build revision from build/deployment metadata supplied to the image (for example a validated AIP_BUILD_REVISION populated by the build), with a deliberate safe local-development fallback. Add a test for the no-Git/no-.git path and preferably a container startup smoke test.

2. Catching every Pydantic ValidationError can expose internal graph/output values.

   In app/mcp/tools.py, every pydantic.ValidationError raised anywhere inside get_service_dependencies() is converted to ToolError(str(exc)). That exception type is not exclusive to malformed caller context: SnapshotRef, projected claims/entities, ServiceDependenciesData, and the final ArchitectureAnswer are also Pydantic models constructed after graph reads. A malformed internal value can therefore appear in Pydantic's input_value detail and be returned to the client, bypassing the SDK's otherwise-correct unexpected-error sanitization. The current secret-leak regression test uses RuntimeError, so it does not exercise this path.

   Required fix: distinguish the specific caller-input failure with a dedicated exception or validate it before service dispatch, and let all other ValidationError instances follow the sanitized unexpected-error path. Add a regression test using a ValidationError whose input contains a secret-like internal value and assert that it is not returned.

These fixes do not require expanding I2.2 or changing dependency semantics.

[michaelegner]

Both blockers fixed in 74b970a (rebased onto latest main):

1. Container startup crash — app/mcp/wiring._resolve_build_revision() no longer runs git rev-parse HEAD unconditionally. It now reads an explicit AIP_BUILD_REVISION env var first (validated as a 40-hex SHA when present), wired through Dockerfile's new ARG/ENV and .github/workflows/docker.yml's build-args: AIP_BUILD_REVISION=${{ github.sha }}. Only falls back to git rev-parse HEAD when the env var is absent (local dev), and that fallback itself never raises — a missing git binary or .git dir logs a warning and returns "unknown" instead of crashing. New tests/unit/test_mcp_wiring.py covers the env-var-present/malformed/absent and git-missing/no-.git cases directly (no live container build, but the exact failure mode — git unavailable — is exercised).

2. Broad ValidationError catch — app/mcp/tools.py no longer wraps the service call in except pydantic.ValidationError. It now pre-validates a supplied observation_context's values before dispatch, by calling the same build_observation_context_ref helper the service uses internally — so the service call itself is never inside a ValidationError handler at all. If a ValidationError somehow still escaped the service (constructing SnapshotRef/claims/ServiceDependenciesData/ArchitectureAnswer from graph data), it now falls through uncaught into the SDK's own generic sanitization, same as any other unexpected exception. Added test_unexpected_validation_error_from_service_is_sanitized_not_leaked — a ValidationError with a secret-like input_value raised from the (stub) service, asserting it's not echoed back — and reworked the old test into test_reversed_observation_window_is_caught_before_dispatch, which also asserts the service is never called (0 dispatches) when the adapter's own pre-check rejects the input.

Full suite (ruff check, ruff format --check, unit incl. the two new tests, integration) all still green.

[michaelegner]

Re-reviewed current head 74b970a.

Verdict: APPROVE — no remaining code findings.

Both previous blockers are resolved:

• Build revision resolution now prefers a validated AIP_BUILD_REVISION; the release-image workflow supplies the exact GitHub SHA, while a Git-less/no-.git fallback no longer crashes application startup. The relevant failure paths are regression-tested.
• Caller observation-context values are pre-validated before dispatch using the existing shared helper. The service call is no longer wrapped in a broad ValidationError handler, so internal/output validation failures follow the SDK's sanitized unexpected-error path. The new secret-bearing ValidationError regression test verifies that internal values are not returned.

The adapter still calls ArchitectureIntelligenceService exactly once for every valid request and preserves the direct answer unchanged. The read-only and semantic-equivalence boundaries remain intact.

Current-head lint/tests, dependency audit, and both CodeQL analyses are green, and GitHub reports the PR as mergeable.

One housekeeping step remains before merge: resolve the existing Copilot thread about git rev-parse; its underlying finding is fixed by this head.