From deede1c475bedf9a1a74247e18fe9b433697b917 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 23 May 2026 20:08:24 +0000 Subject: [PATCH 1/2] fix: serve MCP at /mcp and /mcp/ directly without a 307 redirect Strict OAuth MCP clients (Claude.ai web / Cowork) POST to the exact advertised resource URL (now /mcp, no trailing slash) and do not follow redirects. The MCP app was mounted at /mcp with the endpoint at "/", so /mcp returned a 307 to /mcp/ that the client wouldn't follow, leaving the authenticated session unestablished ("Authorization with the MCP server failed") even though discovery and token exchange succeeded. Build the FastMCP endpoint at path="/mcp", add an explicit /mcp/ alias route, and mount the app at the root (registered LAST so it doesn't shadow the other routes). Both /mcp and /mcp/ now resolve directly with auth enforced. Add a regression test asserting both variants return 200, and document the mounting invariant. https://claude.ai/code/session_01U3EtN3puoZRq2t7nedcnHY --- CLAUDE.md | 2 ++ app/main.py | 19 ++++++++++++++++--- docs/DEVELOPER_GUIDE.md | 6 ++++++ tests/test_logging.py | 6 +++++- tests/test_main.py | 20 ++++++++++++++++++++ 5 files changed, 49 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b4aafef..9a40dd8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,6 +28,8 @@ Phase 1 (MVP) = static bearer-token auth. Phase 2 adds OAuth 2.1 + PKCE + DCR en - **One `Memory` instance per process.** REST and MCP both share a single `mem0.Memory` (lazily built, `@lru_cache`) so the LLM/embedder clients aren't duplicated. This is the whole reason for the single-process, dual-protocol design — don't split them. - **FastMCP lifespan must be passed to FastAPI's constructor.** FastMCP is mounted into FastAPI via `mcp.http_app(...)`; its lifespan has to be wired into the FastAPI `lifespan` or the first MCP request fails with `Task group is not initialized`. See PRD §8.2. - **`stateless_http=True`** is required on `mcp.http_app()` — non-negotiable when running uvicorn with `--workers > 1`, or you get session-not-found errors. +- **MCP is mounted at the root (`app.mount("/", mcp_app)`) and must be registered LAST.** The endpoint is built at `path="/mcp"` with an extra `/mcp/` alias route so both `/mcp` and `/mcp/` serve directly (no 307) — strict OAuth clients POST to the exact resource URL and won't follow a redirect. The root mount is a catch-all, so every other route must be registered before it. +- **The OAuth protected-resource `resource` is the canonical URI without a trailing slash** (`/mcp`). MCP clients canonicalize away the trailing slash and reject auth on a mismatch. - **`MEM0_EMBED_DIMS` must match the embedder's real output dimension** (3-small=1536, 3-large=3072). A mismatch causes *silent* search failures, not errors. Changing embedding models requires dropping and recreating the Qdrant collection. - **FastMCP = the PrefectHQ `fastmcp` PyPI package**, imported `from fastmcp import FastMCP`. It is NOT the older `mcp.server.fastmcp` module. - **Same `MEM0_API_KEY` protects both** the REST endpoints (`require_bearer` dependency) and the MCP endpoint (`StaticTokenVerifier` in Phase 1). diff --git a/app/main.py b/app/main.py index 3861dd3..bddbdab 100644 --- a/app/main.py +++ b/app/main.py @@ -7,6 +7,7 @@ from fastapi import FastAPI, Request, Response from fastapi.responses import JSONResponse from prometheus_client import CONTENT_TYPE_LATEST, generate_latest +from starlette.routing import Route from app.config import get_settings from app.logging_setup import configure_logging @@ -21,7 +22,15 @@ mcp = build_mcp() # stateless_http=True is required to avoid session-not-found errors with >1 worker. -mcp_app = mcp.http_app(path="/", stateless_http=True, transport="streamable-http") +# The endpoint is served at /mcp (not /mcp/): mounting at the root below, plus the +# /mcp/ alias route added here, lets BOTH /mcp and /mcp/ resolve directly without a +# 307 redirect. Strict MCP clients (Claude.ai web / Cowork) POST to the exact +# advertised resource URL and don't follow the redirect, so a redirect breaks them. +mcp_app = mcp.http_app(path="/mcp", stateless_http=True, transport="streamable-http") +_mcp_route = next(r for r in mcp_app.router.routes if getattr(r, "path", None) == "/mcp") +mcp_app.router.routes.append( + Route("/mcp/", _mcp_route.endpoint, methods=list(_mcp_route.methods)) +) @asynccontextmanager @@ -74,8 +83,6 @@ async def log_requests(request: Request, call_next): oauth_store.init_db() app.include_router(oauth_router) -app.mount("/mcp", mcp_app) - @app.get("/metrics") def metrics() -> Response: @@ -101,3 +108,9 @@ async def healthz() -> JSONResponse: return JSONResponse( content={"ok": True, "version": app.version, "qdrant": "reachable"} ) + + +# Mounted at the root LAST so the specific routes above (/api/v1, /oauth, +# /.well-known, /metrics, /healthz) take precedence; the MCP app only owns +# /mcp and /mcp/ and 404s everything else. +app.mount("/", mcp_app) diff --git a/docs/DEVELOPER_GUIDE.md b/docs/DEVELOPER_GUIDE.md index d55b0b5..8781552 100644 --- a/docs/DEVELOPER_GUIDE.md +++ b/docs/DEVELOPER_GUIDE.md @@ -38,6 +38,12 @@ also enumerated in `CLAUDE.md`.) raises `Task group is not initialized`. 3. **`stateless_http=True` on `mcp.http_app()`** is required because the container runs uvicorn with `--workers 2`. Stateful sessions would produce session-not-found errors across workers. +3a. **The MCP app is mounted at the root (`app.mount("/", mcp_app)`), registered LAST**, with the + FastMCP endpoint built at `path="/mcp"` plus an explicit `/mcp/` alias route. This serves both + `/mcp` and `/mcp/` directly (no 307 redirect) — strict clients like Claude.ai web POST to the + exact resource URL and won't follow a redirect. Because the root mount is a catch-all, every + other route (`/api/v1`, `/oauth`, `/.well-known`, `/metrics`, `/healthz`) MUST be registered + before it or it will be shadowed. 4. **`MEM0_EMBED_DIMS` must equal the embedder's real output dimension.** A mismatch produces *silent* empty searches, not an exception. Changing the embedding model requires dropping and recreating the Qdrant collection. diff --git a/tests/test_logging.py b/tests/test_logging.py index 063d2cf..be84752 100644 --- a/tests/test_logging.py +++ b/tests/test_logging.py @@ -25,11 +25,15 @@ def test_request_is_logged_without_token(app_instance): def test_request_logged_as_500_when_handler_raises(app_instance): import structlog + from fastapi.routing import APIRoute - @app_instance.get("/boom-test") async def _boom(): raise RuntimeError("kaboom") + # Insert at the front: the app mounts the MCP sub-app at "/" last, which would + # otherwise shadow a route appended after it. + app_instance.router.routes.insert(0, APIRoute("/boom-test", _boom, methods=["GET"])) + client = TestClient(app_instance, raise_server_exceptions=False) with capture_logs() as logs: client.get("/boom-test") diff --git a/tests/test_main.py b/tests/test_main.py index 87ecb39..6561ab1 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1,6 +1,26 @@ from fastapi.testclient import TestClient +def test_mcp_endpoint_served_at_both_slash_variants(app_instance, auth_header): + # Claude.ai web / Cowork POST to the exact resource URL and do not follow + # redirects, so both /mcp and /mcp/ must resolve directly (no 307). + headers = {**auth_header, "Accept": "application/json, text/event-stream"} + body = { + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2025-06-18", + "capabilities": {}, + "clientInfo": {"name": "t", "version": "1"}, + }, + } + with TestClient(app_instance) as client: + for path in ("/mcp", "/mcp/"): + resp = client.post(path, json=body, headers=headers, follow_redirects=False) + assert resp.status_code == 200, (path, resp.status_code) + + def test_oauth_routes_not_mounted_when_disabled(app_instance): # The test env sets no OAUTH_SIGNING_KEY, so OAuth is disabled and its # routes must not be exposed. A regression here would leak unconfigured From 49d290b1932801b59a0460db1807c9a97dc29f19 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 23 May 2026 20:14:22 +0000 Subject: [PATCH 2/2] refactor: harden /mcp alias lookup and preserve MCP metric label Address review feedback on the root-mount change: - raise a clear error if FastMCP's /mcp route isn't found, instead of an opaque StopIteration at import time - the root mount leaves no matched route at the middleware level, so MCP requests were bucketing under "__unmatched__"; derive a stable "/mcp" label (both slash variants) and keep real fallthrough 404s unmatched - renumber the developer-guide invariant list (3a -> 4) so it renders https://claude.ai/code/session_01U3EtN3puoZRq2t7nedcnHY --- app/main.py | 18 ++++++++++++++++-- docs/DEVELOPER_GUIDE.md | 8 ++++---- tests/test_metrics.py | 14 ++++++++++++++ 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/app/main.py b/app/main.py index bddbdab..c72ceb8 100644 --- a/app/main.py +++ b/app/main.py @@ -27,7 +27,14 @@ # 307 redirect. Strict MCP clients (Claude.ai web / Cowork) POST to the exact # advertised resource URL and don't follow the redirect, so a redirect breaks them. mcp_app = mcp.http_app(path="/mcp", stateless_http=True, transport="streamable-http") -_mcp_route = next(r for r in mcp_app.router.routes if getattr(r, "path", None) == "/mcp") +_mcp_route = next( + (r for r in mcp_app.router.routes if getattr(r, "path", None) == "/mcp"), None +) +if _mcp_route is None: + raise RuntimeError( + "FastMCP did not register the expected /mcp route; cannot add the /mcp/ alias. " + "Check the fastmcp version and the http_app(path=...) argument." + ) mcp_app.router.routes.append( Route("/mcp/", _mcp_route.endpoint, methods=list(_mcp_route.methods)) ) @@ -61,7 +68,14 @@ async def log_requests(request: Request, call_next): # keep label cardinality bounded. Unmatched (404) requests have no route, # so bucket them under a fixed label instead of the arbitrary raw path. route = request.scope.get("route") - metric_path = getattr(route, "path", None) or "__unmatched__" + metric_path = getattr(route, "path", None) + if not metric_path: + # Requests served by the root-mounted MCP app have no route at this + # outer level. Bucket the two MCP path variants under a single stable + # label; anything else that fell through is genuinely unmatched. + metric_path = ( + "/mcp" if request.url.path.rstrip("/") == "/mcp" else "__unmatched__" + ) observe_request(request.method, metric_path, status, elapsed) _log.info( "request", diff --git a/docs/DEVELOPER_GUIDE.md b/docs/DEVELOPER_GUIDE.md index 8781552..9e9e82b 100644 --- a/docs/DEVELOPER_GUIDE.md +++ b/docs/DEVELOPER_GUIDE.md @@ -38,18 +38,18 @@ also enumerated in `CLAUDE.md`.) raises `Task group is not initialized`. 3. **`stateless_http=True` on `mcp.http_app()`** is required because the container runs uvicorn with `--workers 2`. Stateful sessions would produce session-not-found errors across workers. -3a. **The MCP app is mounted at the root (`app.mount("/", mcp_app)`), registered LAST**, with the +4. **The MCP app is mounted at the root (`app.mount("/", mcp_app)`), registered LAST**, with the FastMCP endpoint built at `path="/mcp"` plus an explicit `/mcp/` alias route. This serves both `/mcp` and `/mcp/` directly (no 307 redirect) — strict clients like Claude.ai web POST to the exact resource URL and won't follow a redirect. Because the root mount is a catch-all, every other route (`/api/v1`, `/oauth`, `/.well-known`, `/metrics`, `/healthz`) MUST be registered before it or it will be shadowed. -4. **`MEM0_EMBED_DIMS` must equal the embedder's real output dimension.** A mismatch produces +5. **`MEM0_EMBED_DIMS` must equal the embedder's real output dimension.** A mismatch produces *silent* empty searches, not an exception. Changing the embedding model requires dropping and recreating the Qdrant collection. -5. **FastMCP is the PrefectHQ `fastmcp` PyPI package** (`from fastmcp import FastMCP`), **not** the +6. **FastMCP is the PrefectHQ `fastmcp` PyPI package** (`from fastmcp import FastMCP`), **not** the older `mcp.server.fastmcp` module. -6. **The same `MEM0_API_KEY` protects both protocols** — `require_bearer` for REST and the token +7. **The same `MEM0_API_KEY` protects both protocols** — `require_bearer` for REST and the token verifier for MCP. Keep them in sync. ## Project layout diff --git a/tests/test_metrics.py b/tests/test_metrics.py index b28cb0a..2d2e059 100644 --- a/tests/test_metrics.py +++ b/tests/test_metrics.py @@ -17,3 +17,17 @@ def test_metrics_endpoint_reports_requests(app_instance): # The matched route template is used as the path label, not the raw id. assert 'path="/api/v1/memories/{memory_id}"' in body assert "abc123" not in body + + +def test_mcp_requests_get_stable_metric_label(app_instance): + # The MCP app is mounted at the root, so its requests have no matched route + # at the middleware level. They must still get a stable "/mcp" label (both + # slash variants), while genuine fallthrough 404s bucket under __unmatched__. + with TestClient(app_instance) as client: + headers = {"Accept": "application/json, text/event-stream"} + client.post("/mcp", json={}, headers=headers, follow_redirects=False) + client.post("/mcp/", json={}, headers=headers, follow_redirects=False) + client.get("/totally-unknown-path") + body = client.get("/metrics").text + assert 'path="/mcp"' in body + assert 'path="__unmatched__"' in body