feat(server): merge recce mcp-server into recce server (DRC-3228)#1305
feat(server): merge recce mcp-server into recce server (DRC-3228)#1305kentwelcome wants to merge 17 commits intomainfrom
recce mcp-server into recce server (DRC-3228)#1305Conversation
Single source of truth for MCP server construction so transport adapters can share the tool-registration code path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
Move the stdio transport runner out of RecceMCPServer into a dedicated mcp_transport module. The class method becomes a delegating shim until all callers migrate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
…gger logger.exception already captures the active exception; the f-string interpolation duplicates the message in log output and defeats lazy formatting. Code-review feedback on Task 2. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
run_mcp_sse_legacy preserves the old /sse + /messages + /health surface for the deprecated recce mcp-server --sse path. RecceMCPServer.run_sse is kept as a delegating shim until callers migrate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
- Add return type annotation Starlette to _build_legacy_sse_app via TYPE_CHECKING import. - Comment the SseServerTransport endpoint to flag the deliberate /messages/ path (old run_sse used / which was non-standard). - Strengthen route-presence test to pin the /messages mount path rather than relying on the 'nameless route' heuristic. Code-review feedback on Task 3. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
Mounts both Streamable HTTP (at /mcp) and legacy SSE (at /mcp/sse + /mcp/messages) on an existing FastAPI app. Returns the StreamableHTTPSessionManager so the caller drives its lifecycle from the FastAPI lifespan. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
- Pin both transports (Streamable HTTP catch-all + legacy SSE/messages)
in the route-presence test so a regression that drops or reorders
Mount('/') is caught at unit-test time.
- Annotate app: FastAPI in attach_mcp_to_fastapi via TYPE_CHECKING.
Code-review feedback on Task 4.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Kent Huang <kent@infuseai.io>
Single event recording MCP availability, active transports, and the command that booted the server. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
The MCP server is built off-thread after context loads, then mounted at /mcp via attach_mcp_to_fastapi. If the build fails, REST keeps serving and /mcp* returns a structured 503 JSON-RPC envelope. attach_mcp_to_fastapi now inserts the Mount at app.routes[0] so the runtime mount shadows the module-import-time 503 fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
- Wrap log_mcp_startup in its own try/except so a telemetry failure does not populate mcp_startup_error (which would incorrectly flip a healthy MCP session into an error state on subsequent requests). - Add id: null to JSON-RPC error responses for spec conformance. - Add test for the loading-state 503 (-32002) branch that exercises _mcp_fallback directly to avoid lifespan startup races. Code-review feedback on Task 5. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
Defaults to enabled. Explicit CLI flag overrides the env var. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
Body shrinks to: deprecation warning -> build mcp.Server via the shared builder -> dispatch to run_mcp_stdio or run_mcp_sse_legacy. Behavior preserved exactly for both transports. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
One happy-path test confirms MCP mount at app.routes[0] shadows the /mcp fallback handler and both endpoints respond. One failure-path test confirms a thrown attach error leaves REST functional while /mcp returns the 503 fallback envelope. An autouse fixture snapshots and restores app.routes after each test to prevent the injected marker route from leaking across tests. In-process TestClient approach replaces the planned subprocess-based test — faster, less flaky, covers the same code paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
StreamableHTTPSessionManager.run() uses anyio cancel scopes which are task-affine — they must be entered and exited in the same task. The previous AsyncExitStack-based approach entered the scope inside background_load (one task) and exited it in the outer lifespan (different task), causing RuntimeError: 'Attempted to exit cancel scope in a different task'. Restructure so mcp_lifecycle is its own task that: 1. Waits for context load via ctx_ready_event 2. Builds and mounts MCP 3. Enters session_manager.run() and holds it via mcp_shutdown_event 4. Exits cleanly when the lifespan signals shutdown Behavior preserved: REST survives MCP failures, --no-mcp / RECCE_DISABLE_MCP still disables, /mcp fallback routes unchanged, telemetry still fires. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
There was a problem hiding this comment.
Pull request overview
This PR merges the MCP server into the existing recce server process so a single invocation hosts the REST/UI endpoints and the MCP endpoints, while keeping recce mcp-server as a deprecated shim.
Changes:
- Adds MCP transport adapters (
stdio, legacy SSE, and FastAPI mount) and wires MCP startup/shutdown into the FastAPI lifespan. - Adds
/mcp*fallback routing for disabled/loading/failed MCP states and introduces--mcp/--no-mcp+RECCE_DISABLE_MCP=1. - Adds a suite of tests covering lifespan integration, transport wiring, CLI behavior, and deprecation behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
recce/server.py |
Adds MCP lifecycle task in lifespan and /mcp* fallback handler before SPA mount. |
recce/mcp_transport.py |
Introduces shared transport wiring for stdio, legacy SSE, and FastAPI-mounted MCP. |
recce/mcp_server.py |
Refactors to delegate transport logic and adds build_mcp_server() builder. |
recce/cli.py |
Adds --mcp/--no-mcp flag + env var resolution; deprecates mcp-server command into a shim. |
recce/event/__init__.py |
Adds log_mcp_startup telemetry helper. |
tests/test_server_mcp_lifespan.py |
Tests MCP-related AppState fields and fallback behaviors (404/503 envelopes). |
tests/test_merged_server_e2e.py |
Tests REST + MCP coexistence and failure isolation in merged server. |
tests/test_mcp_transport.py |
Verifies transport adapter wiring and route ordering. |
tests/test_mcp_server.py |
Adds unit tests for build_mcp_server(). |
tests/test_event_mcp_startup.py |
Tests MCP startup telemetry payload. |
tests/test_cli.py |
Updates MCP CLI tests to the new build/transport structure. |
tests/test_cli_server_mcp_flag.py |
Tests --mcp/--no-mcp and RECCE_DISABLE_MCP precedence + help text. |
tests/test_cli_mcp_server_deprecation.py |
Tests deprecated recce mcp-server warning and dispatch behavior. |
| load_kwargs = {k: v for k, v in kwargs.items() if k != "state_loader"} | ||
| context = load_context(**load_kwargs) |
There was a problem hiding this comment.
mcp_server builds a state_loader (cloud mode or state_file) but then calls load_context(**load_kwargs) without passing that state_loader. RecceContext.load() relies on state_loader to import state and set context.state_loader, so this likely breaks recce mcp-server <state_file> / --cloud behavior (state won’t be loaded; tools that read context.state_loader will see None). Pass state_loader into load_context (and avoid filtering it out) so context loads the state consistently with the rest of the CLI/server paths.
| load_kwargs = {k: v for k, v in kwargs.items() if k != "state_loader"} | |
| context = load_context(**load_kwargs) | |
| load_kwargs = dict(kwargs) | |
| context = load_context(state_loader=state_loader, **load_kwargs) |
There was a problem hiding this comment.
Good catch — confirmed bug. Fixed in 4903ea1.
recce mcp-server <state_file> and --cloud were silently broken because load_context was called without state_loader, so RecceContext.load() (recce/core.py:33) never imported state. The shim now mirrors the pattern used by setup_server in recce/server.py:
context = load_context(**kwargs, state_loader=state_loader)The previous load_kwargs = {k: v for k, v in kwargs.items() if k != "state_loader"} filter was leftover dead code from before this refactor — also removed in the same commit.
| if state.mcp_startup_error: | ||
| return JSONResponse( | ||
| status_code=503, | ||
| content={ | ||
| "jsonrpc": "2.0", | ||
| "id": None, | ||
| "error": { | ||
| "code": -32603, | ||
| "message": ("MCP server failed to start. Check server logs. " f"Error: {state.mcp_startup_error}"), | ||
| }, |
There was a problem hiding this comment.
The MCP startup-failure response includes the raw exception string (state.mcp_startup_error) in the JSON-RPC error message. This can leak internal details (paths, config values, dependency info) to remote clients. Consider returning a generic message (and optionally an error type/code) while keeping full details only in server logs, similar to how /api/health avoids exposing the full exception.
There was a problem hiding this comment.
Agreed — fixed in 4903ea1.
The /mcp 503 response no longer echoes state.mcp_startup_error. The new message is:
MCP server failed to start. Check server logs for details.
The full exception (with paths/config) stays in the server logs only, written via logger.exception("[MCP] Failed to build/mount MCP server; REST will continue") in the mcp_lifecycle task. Also added a unit-test assertion (tests/test_server_mcp_lifespan.py) that the raw error string is NOT in the response body, so a future regression that re-leaks it gets caught at unit-test time.
| async def handle_sse_request(request: Request): | ||
| client_info = f"{request.client.host}:{request.client.port}" if request.client else "unknown" | ||
| logger.info(f"[MCP HTTP] SSE connection established from {client_info}") | ||
| try: | ||
| async with sse.connect_sse(request.scope, request.receive, request._send) as streams: | ||
| await rmcp.server.run(streams[0], streams[1], rmcp.server.create_initialization_options()) | ||
| finally: | ||
| logger.info(f"[MCP HTTP] SSE connection closed from {client_info}") | ||
| return Response() |
There was a problem hiding this comment.
handle_sse_request relies on request._send, which is a private Starlette Request attribute and not part of the public API. This is brittle across Starlette/FastAPI versions. Prefer implementing the SSE endpoint as a raw ASGI callable that receives (scope, receive, send) (or otherwise obtain send via supported APIs) so connect_sse(...) doesn’t depend on a private attribute.
There was a problem hiding this comment.
Agreed — fixed in 4903ea1.
Both SSE handlers (this one in _build_legacy_sse_app and the merged-server one in attach_mcp_to_fastapi) now use raw ASGI signatures mounted directly via Mount("/sse", app=handle_sse_asgi). The handler accepts (scope, receive, send) and forwards them straight into sse.connect_sse(...) — no Request._send access, no private API.
Method filtering is preserved by an explicit check inside the handler (returns 405 for non-GET).
| async def handle_sse_request(request: Request) -> Response: | ||
| client_info = f"{request.client.host}:{request.client.port}" if request.client else "unknown" | ||
| logger.info(f"[MCP HTTP] SSE connection established from {client_info}") | ||
| try: | ||
| async with sse.connect_sse(request.scope, request.receive, request._send) as streams: | ||
| await rmcp.server.run(streams[0], streams[1], rmcp.server.create_initialization_options()) | ||
| finally: | ||
| logger.info(f"[MCP HTTP] SSE connection closed from {client_info}") | ||
| return Response() |
There was a problem hiding this comment.
Same issue as above: this SSE handler uses request._send (private Starlette API). To reduce breakage risk during dependency upgrades, consider rewriting the route endpoint to accept (scope, receive, send) and pass the real send callable into connect_sse(...) without touching private Request fields.
There was a problem hiding this comment.
Same fix applied here in 4903ea1 — attach_mcp_to_fastapi now mounts an ASGI handler at /sse instead of using a Starlette Route that touches request._send. See the reply above on the legacy SSE handler for the pattern.
Code reviewFound 1 issue:
Lines 2414 to 2419 in 9fc8dc7 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
- recce/cli.py: `recce mcp-server <state_file>` and `--cloud` were
silently broken — load_context was called without state_loader, so
RecceContext.load() never imported state from disk/cloud. Pass
state_loader through and drop the now-redundant kwargs filter.
- recce/server.py: don't echo state.mcp_startup_error verbatim in the
/mcp 503 response body (paths, config values, dependency info can
leak to remote clients). Keep full details in server logs only.
- recce/mcp_transport.py: replace request._send (private Starlette
attribute) with raw ASGI handlers mounted directly. Both the merged
server and the legacy SSE app now use Mount("/sse", app=...) with
an ASGI-signature handler that forwards scope/receive/send into
SseServerTransport.connect_sse — no private API access.
- tests: tighten the startup-failure assertion so a future regression
that re-leaks the exception string is caught at unit-test time.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Kent Huang <kent@infuseai.io>
|
Follow-up to my earlier comment about the dead context = load_context(**kwargs, state_loader=state_loader)The filter was leftover scaffolding from the pre-refactor code path that injected |
The dbtlatest tox env pulls a newer Click that removed the mix_stderr parameter (it now always separates stderr). Detect support at import time and pass mix_stderr only when the parameter exists, so the same test file passes under dbt1.6-1.9 (Click 8.1) and dbtlatest (Click 8.2+). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
Codecov Report❌ Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Adds 12 focused tests covering branches missed by the smoke tests: - run_mcp_stdio: state export with non-None message, with state_file set, and swallowed exceptions on shutdown. - _build_legacy_sse_app: ASGI handler rejects non-GET (405) and ignores non-http scope; /health endpoint returns 200; lifespan shutdown exports state and swallows export failures. - attach_mcp_to_fastapi: merged-server SSE handler rejects non-GET and ignores non-http scope; Streamable HTTP catch-all forwards into session_manager.handle_request (verified via AsyncMock on the returned manager). - run_mcp_sse_legacy: confirms uvicorn.Server.serve is invoked. Lifts mcp_transport.py patch coverage from 48% to 85%. The remaining 15% is the SSE protocol body (sse.connect_sse + server.run) which requires a real MCP handshake — already covered by tests/test_mcp_e2e. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kent Huang <kent@infuseai.io>
PR checklist
What type of PR is this?
feat— merge two CLI commands into one, with deprecation shim for back-compat.What this PR does / why we need it:
Refactors the Recce CLI so a single
recce serverinvocation hosts both the HTTP API/UI and the MCP endpoint for agents. Previously these required two separate commands (recce serverandrecce mcp-server), forcing developers to choose one transport at a time and adding friction for dev-session integration with Claude.Now,
recce serverexposes:GET /+/api/*— the existing web UI and REST API (unchanged)POST/GET /mcp— Streamable HTTP (modern MCP transport)GET /mcp/sse+POST /mcp/messages/— legacy SSE transport (during deprecation window)Architecture:
recce/mcp_transport.pymodule hosts three transport adapters:run_mcp_stdio,run_mcp_sse_legacy,attach_mcp_to_fastapi.recce/mcp_server.pyexposesbuild_mcp_server(ctx, ...)as the single source of truth for MCP server construction — all transports consume it.recce/server.pylifespan builds the MCP server off-thread after context loads, mounts it atapp.routes[0]to shadow the 503 fallback, and holds the session-manager open in a dedicated task (required by anyio cancel-scope task-affinity).recce/cli.pyadds--mcp/--no-mcpflag plusRECCE_DISABLE_MCP=1env var torecce server; convertsrecce mcp-serverinto a deprecating thin shim that still dispatches to the shared transport adapters.recce/event/__init__.pyaddslog_mcp_startuptelemetry helper.Error handling:
/mcp*returns JSON-RPC-32603503 while the UI keeps serving./mcp*returns JSON-RPC-32002503.--no-mcp/RECCE_DISABLE_MCP=1returns 404 at/mcp*.log_mcp_startup) failures are isolated so they don't flip a healthy MCP session into an error state.Which issue(s) this PR fixes:
DRC-3228
Special notes for your reviewer:
recce serveralso changes the legacy SSE message-channel path to/mcp/messages/(spec-compliant MCP), so agent configs pointing at the oldrecce mcp-server --ssesurface need to update their SSE endpoint fromhttp://host:port/sse→http://host:port/mcp/sse(or migrate to Streamable HTTP at/mcp).recce mcp-servercommand still preserves its original paths (/sse,/messages,/health) for back-compat until it's removed in a future release.RecceMCPServer.runandRecceMCPServer.run_ssemethods are kept as delegating shims for one release — removing them is tracked as a deferrable follow-up.test_mcp_transport.py,test_server_mcp_lifespan.py,test_merged_server_e2e.py,test_event_mcp_startup.py,test_cli_server_mcp_flag.py,test_cli_mcp_server_deprecation.py). All 137 MCP-related tests pass.TestClient-based test for robustness and speed — covers the same code paths.Does this PR introduce a user-facing change?:
```
recce server now hosts both the HTTP API/UI and an MCP endpoint in a single process.
MCP is available by default at /mcp (Streamable HTTP) and /mcp/sse (legacy SSE).
Disable with --no-mcp or RECCE_DISABLE_MCP=1.
recce mcp-server is now deprecated — it still works (with a warning) but will be
removed in a future release. Migrate to recce server.
action required: If you previously used recce mcp-server --sse, update your agent
config to point at recce server's /mcp/sse path (or Streamable HTTP at /mcp).
```
🤖 Generated with Claude Code