Repository navigation
fix(mcp): acknowledge every JSON-RPC notification instead of answering 400 - #706
Conversation
…g 400 handleMcp acknowledged only `notifications/initialized` (204); every other notification fell through to the unknown-method branch and answered HTTP 400 / -32601 with error.type=MethodNotFound on the request span. MCP clients send `notifications/cancelled` whenever they abandon a request, so ~11% of production MCP requests were recorded as errors. Acknowledge any `notifications/*` method with the same empty 204, without marking the span. Non-notification unknown methods still get -32601. Pinned by mcp-notification-ack.spec.ts (source scan) and a live check in smoke-mcp-tools.mjs. Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com>
✅ No issues foundNo code changes since Checked: 0 of 1 dependent file traced Progress: open review threads 1 → 0 → 0 across the last 3 reviews What this change reaches — 1 changed export · 1 dependent file · 0 checked · 1 not checkedflowchart LR
subgraph pr["Changed in this PR"]
s1["handleMcp<br/>body changed"]:::changed
end
s1c1["supabase/functions/mcp/index.ts<br/>? not checked"]:::unknown
s1 -.-> s1c1
classDef changed fill:#eef2ff,stroke:#6366f1,color:#1e1b4b
classDef ok fill:#e7f6ec,stroke:#16a34a,color:#14532d
classDef partial fill:#fef9c3,stroke:#ca8a04,color:#713f12
classDef unknown fill:#f3f4f6,stroke:#9ca3af,stroke-dasharray:4 3,color:#374151
classDef bad fill:#fde8e8,stroke:#dc2626,color:#7f1d1d
classDef warn fill:#fff4e5,stroke:#d97706,color:#78350f
classDef imports fill:#f8fafc,stroke:#64748b,stroke-dasharray:4 3,color:#1e293b
Review details
Found Quality — produced 0 → posted inline 0 · cleared 0 · carried forward 0 · deferred 0 · below-bar 0 Run incremental · 0 lines in delta · tier deep · depth checkout Nothing to report — standards (skipped), optimality (skipped), measurability (skipped), severity, 0 files skipped.
|
…ethod Addresses the pr-reviewer bot's comment: #706 (comment) Refs: #706 Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com>
|
The dashboard preview for this PR — redeployed on each push that changes the web app.
Preview always points at this PR's latest commit; Deployment is the immutable build of |
Summary
The hosted MCP endpoint answers every JSON-RPC notification except
notifications/initializedwith HTTP 400 / JSON-RPC-32601 Method not found. MCP clients sendnotifications/cancelledwhenever they abandon a request, so every MCP user sees spurious 400s, and about 11% of LoreKit's production MCP requests are counted as errors. This change acknowledges everynotifications/*method with the same empty 204 thatnotifications/initializedalready gets, and does not mark the span as an error.Evidence (production telemetry)
mcpSERVER spans witherror.type=MethodNotFound,deployment.environment.name=production: 11.15% now (780.4 of 7,001 spans over 7d, evaluated at 2026-10-19T06:01:39Z) vs 1.77% over the 21-day baseline 2026-09-21..10-12 (929 spans). The baseline undercounts, becauseerror.typewas only introduced on 2026-10-03 (fix(edge-otel): bounded DB span names, parameterised db.query.text, http.route + error.type #682). A count over the baseline window that does not depend onerror.typefound 2,729notifications/cancelled400s (5.2% of MCP requests), so the problem is ongoing, not new. In 2026-10-12T06:01:39Z..2026-10-19T06:01:39Z, 749notifications/cancelledrequests got HTTP 400MethodNotFound: notifications/cancelled(10.7% of MCP requests).c108e0d0554a5fc882c991c4b34bedbf, span46f9d528d9b2a1ae: anotifications/cancelledrequest answered 400 after 244 ms and 3 DB child calls (auth, plan and rate-limit preamble).Root cause
handleMcpacknowledges only one notification, by exact name:lorekit/supabase/functions/mcp/mcp-handler.ts
Lines 126 to 128 in fed98c8
Every other notification (
notifications/cancelled,notifications/progress,notifications/roots/list_changed, …) matches none of the handled methods, so it reaches the unknown-method fallthrough. That fallthrough callsspan.clientError("MethodNotFound: …")(which recordserror.type=MethodNotFound) and returns a 400 JSON-RPC error body:lorekit/supabase/functions/mcp/mcp-handler.ts
Lines 589 to 591 in fed98c8
Under JSON-RPC 2.0, a notification (a message without an
id) never gets a response body. MCP's HTTP transport accepts one with an empty 2xx. The CLI's local stdio MCP server never had this defect, because it already acknowledges by prefix:lorekit/packages/cli/src/commands/mcp-server.mjs
Lines 433 to 436 in fed98c8
Change
supabase/functions/mcp/mcp-handler.ts: the exactmethod === 'notifications/initialized'check is nowtypeof method === 'string' && method.startsWith('notifications/'). Thetypeofguard (added in review, the same guard the CLI stdio server uses) keeps a non-stringmethodsuch as42on the-32601path instead of throwing a TypeError (500). It keeps the same position, beforetools/listandtools/calland well before the fallthrough, and it still returnsnew Response(null, { status: 204 })without callingclientError/error. The span keepsmcp.method(already set above the branch), so notifications stay visible in telemetry, but they no longer carryerror.type.resources/list,server/discover,resources/templates/list, …) still reach the fallthrough and still get-32601.2024-11-05. 204 is what clients already receive fornotifications/initializedfrom this endpoint on every session, so keeping 204 means no behaviour change for the one notification that already worked. It also keeps the diff to one condition, and keeps the existingmcp/index.tscomment ("notifications/initializedanswers 204") true without touching that file. Both statuses are an empty 2xx, which is what a notification sender expects. Moving every notification to 202 together can be a separate, deliberate change.supabase/functions/mcp/index.ts(a notification still pays the auth + plan + rate-limit preamble, asinitializeddoes today), and the CLI stdio server, which is already correct.llms.txtdescribes how notifications are answered.docs/mcp-tools.mdlists-32601as "Unknown method or tool name", which stays accurate.Test
packages/mcp-core/src/mcp-guards/mcp-notification-ack.spec.ts(runs in CI'scheckjob vianx test mcp-core; vitest cannot import the Deno handler, which is whymcp-method-guard-ordering.spec.ts/mcp-authz-status.spec.tsare also source scans). It asserts thathandleMcpacknowledges by thenotifications/prefix and not by the exactinitializedname, that the prefix test istypeof-guarded, that the branch answers an empty 204, that it calls none ofclientError(/.error(/jsonrpcError(, that it comes before theMethodNotFoundfallthrough (sonotifications/cancelledcannot reach it), and that the fallthrough still returns-32601.mcp-handler.ts(the original five tests):pnpm exec vitest run --root packages/mcp-core src/mcp-guards/mcp-notification-ack.spec.tsgave 4 failed / 1 passed (the passing one is the "non-notification still-32601" guard).typeofguard: it fails with the guard removed and passes with it. All ofsrc/mcp-guards/passes, and the fullnx test mcp-corepasses (86 files, 1991 passed, 1 skipped).deno test): a Deno test imported the realhandleMcp, called it with a fake span / service auth / hosted adapter, and checkednotifications/{initialized,cancelled,progress,roots/list_changed}andresources/list,server/discover,resources/templates/list. Before the fix: 4 passed / 3 failed (cancelled, progress and roots/list_changed got 400). After: 7/7 passed. Each notification gets an empty-body 204 with no span-error call; each unknown request still gets 400 /-32601with exactly oneclientError("MethodNotFound: …").scripts/smoke/smoke-mcp-tools.mjs: a new conformance check sendsnotifications/cancelledto the hosted endpoint and asserts a 2xx with an empty body. Only CI confirms this one. It runs indeploy.yml's "Smoke — hosted MCP tool surface against preview" step after merge (and on a manual preview redeploy of a PR). It is not part ofci.yml's Integration job, whose MCP smoke (smoke-mcp-stdio.mjs) goes through the CLI stdio server, and that server answers notifications locally without forwarding them. Locally:node --checkpasses, andpackages/cli/test/cli-smoke.test.mjs(the static smoke-coverage guard) passes 42/42.node scripts/ci/deno-check-functions.mjs --node-modules-dir=nonereports 0 errors formcp(baseline 0).nx typecheck mcp-coreandnx lint mcp-corepass (0 errors; warnings are pre-existing and none are in the new file). eslint onmcp-handler.tsis clean. supabasenode --testpasses 10/10 and evalsnode --testpasses 250/250.nx affectedalso listsweb/web-e2e(through the project graph only, since no file there changed). Their typecheck/test was not run locally because the sandbox ran out of memory, so CI'scheckjob is the confirmation there.How we will know it worked
Re-run the PromQL above (unit: % of production
mcpSERVER spans witherror.type=MethodNotFound, 7d window). Expected direction: down, from 11.15% to roughly the residual non-notification unknown methods (server/discover~17,resources/list~7,resources/templates/list~7 per week, about 0.4%). The weekly loop will re-measure on or after 2026-10-26, once this has been deployed to production for a full 7-day window.Opened by the weekly Agent0 telemetry-driven improvement automation. It is merged automatically when the review loop converges with an approving verdict and CI is green (and, for a web UI change, when the ui-verify test passes on this PR's Vercel preview), unless it carries the needs-human-merge label.