diff --git a/packages/mcp-core/src/mcp-guards/mcp-notification-ack.spec.ts b/packages/mcp-core/src/mcp-guards/mcp-notification-ack.spec.ts new file mode 100644 index 000000000..0a63cdbdd --- /dev/null +++ b/packages/mcp-core/src/mcp-guards/mcp-notification-ack.spec.ts @@ -0,0 +1,87 @@ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; + +/** + * Every JSON-RPC notification is acknowledged, never answered MethodNotFound. + * + * A JSON-RPC 2.0 notification (a message with no `id`) gets no response body, + * and MCP's HTTP transport accepts one with an empty 2xx. MCP clients send + * `notifications/cancelled` whenever they abandon a request, and may send + * `notifications/progress` or `notifications/roots/list_changed`. `handleMcp` + * used to acknowledge only `notifications/initialized`, so every other + * notification fell through to the unknown-method branch: HTTP 400, JSON-RPC + * -32601, and `error.type=MethodNotFound` on the request span. + * + * Measured 2026-10-12→19 in production: 749 `notifications/cancelled` requests + * answered 400 `MethodNotFound`, 10.7% of all MCP requests — an error rate the + * clients did nothing to earn. + * + * Scans the Deno handler, which vitest cannot import — the same approach as + * `mcp-method-guard-ordering.spec.ts` and `mcp-authz-status.spec.ts`. + */ + +const here = path.dirname(fileURLToPath(import.meta.url)); +const handler = readFileSync( + path.resolve(here, '../../../../supabase/functions/mcp/mcp-handler.ts'), + 'utf8', +); + +const ACK = "startsWith('notifications/')"; +const FALLTHROUGH = 'MethodNotFound: ${method}'; + +/** The acknowledgement branch: from the prefix test to the end of its block. */ +function ackBlock(): string { + const start = handler.indexOf(ACK); + expect(start, `no \`${ACK}\` branch in handleMcp`).toBeGreaterThan(-1); + const end = handler.indexOf('\n }', start); + expect(end).toBeGreaterThan(start); + return handler.slice(start, end); +} + +describe('handleMcp acknowledges every JSON-RPC notification', () => { + it('acknowledges by the notifications/ PREFIX, not one exact method name', () => { + // An exact match on `notifications/initialized` is the defect: it leaves + // `notifications/cancelled` and every other notification to the fallthrough. + expect(handler).toMatch(/method\??\.startsWith\('notifications\/'\)/); + expect(handler).not.toMatch(/method === 'notifications\/initialized'/); + }); + + it('guards the prefix test with typeof, so a non-string method cannot throw', () => { + // `method` is unchecked JSON. Optional chaining only skips null/undefined, + // so `method?.startsWith(...)` throws a TypeError for `42` or `{}` and turns + // a client error into a 500. The CLI stdio server guards the same check. + expect(handler).toMatch(/typeof method === 'string' && method\.startsWith\('notifications\/'\)/); + }); + + it('answers a notification with an empty 2xx, not a JSON-RPC body', () => { + expect(ackBlock()).toMatch(/return new Response\(null, \{ status: 204 \}\);/); + }); + + it('does not mark the span as an error for a notification', () => { + // A notification is handled correctly by being acknowledged. Recording + // `error.type` here would keep inflating the MCP error rate. + const block = ackBlock(); + expect(block).not.toMatch(/clientError\(/); + expect(block).not.toMatch(/\.error\(/); + expect(block).not.toMatch(/jsonrpcError\(/); + }); + + it('is decided before the unknown-method fallthrough, so notifications/cancelled cannot reach it', () => { + const ackIdx = handler.indexOf(ACK); + const fallthroughIdx = handler.indexOf(FALLTHROUGH); + expect(ackIdx).toBeGreaterThan(-1); + expect(fallthroughIdx, 'MethodNotFound fallthrough not found').toBeGreaterThan(-1); + expect(ackIdx).toBeLessThan(fallthroughIdx); + }); + + it('still answers a non-notification unknown method with MethodNotFound (-32601)', () => { + // `resources/list`, `server/discover` and friends are requests the server + // does not implement — they must keep their error. + const fallthroughIdx = handler.indexOf(FALLTHROUGH); + expect(fallthroughIdx).toBeGreaterThan(-1); + const tail = handler.slice(fallthroughIdx); + expect(tail).toMatch(/return jsonrpcError\(id, -32601, `Method not found: \$\{method\}`\);/); + }); +}); diff --git a/scripts/smoke/smoke-mcp-tools.mjs b/scripts/smoke/smoke-mcp-tools.mjs index eb1e24081..4de924d9e 100644 --- a/scripts/smoke/smoke-mcp-tools.mjs +++ b/scripts/smoke/smoke-mcp-tools.mjs @@ -457,6 +457,20 @@ check('an unknown method is -32601', async () => { eq(json.error?.code, -32601, 'error code'); }); +check('a notification other than initialized is acknowledged with an empty 2xx', async () => { + // A JSON-RPC notification carries no `id` and gets no response body. Clients + // send `notifications/cancelled` whenever they abandon a request; it used to + // fall through to the unknown-method branch and answer 400 / -32601. + const { status, json } = await rpc({ + jsonrpc: '2.0', + method: 'notifications/cancelled', + params: { requestId: rpcId, reason: 'smoke' }, + }); + ok(status >= 200 && status < 300, `http status: expected 2xx, got ${status}`); + // `rpc` keeps an unparseable body under `_unparsed`, so an empty body is ''. + eq(json?._unparsed, '', 'response body'); +}); + check('an unknown tool is -32601', async () => { const r = await call('memory.frobnicate'); eq(r.error?.code, -32601, 'error code'); diff --git a/supabase/functions/mcp/mcp-handler.ts b/supabase/functions/mcp/mcp-handler.ts index 90430f229..7a4c21ef6 100644 --- a/supabase/functions/mcp/mcp-handler.ts +++ b/supabase/functions/mcp/mcp-handler.ts @@ -123,7 +123,16 @@ export async function handleMcp(req: Request, auth: AuthContext, span: Span, ada }); } - if (method === 'notifications/initialized') { + // Every JSON-RPC notification (no `id`, no response body) is acknowledged — + // `notifications/initialized`, and the `notifications/cancelled` a client + // sends whenever it abandons a request. Matching only `initialized` sent + // every other one to the MethodNotFound fallthrough below (HTTP 400). Not a + // span error: the request was handled correctly. 204 is what `initialized` + // has always answered; pinned by `mcp-notification-ack.spec.ts`. + // `method` is unchecked JSON: the `typeof` guard keeps a non-string value + // (`42`, `{}`) on the MethodNotFound path instead of throwing a TypeError + // (500), the same guard the CLI stdio server uses. + if (typeof method === 'string' && method.startsWith('notifications/')) { return new Response(null, { status: 204 }); }