diff --git a/src/mcp/dispatch-telemetry.ts b/src/mcp/dispatch-telemetry.ts index 70960bcce..de8402e73 100644 --- a/src/mcp/dispatch-telemetry.ts +++ b/src/mcp/dispatch-telemetry.ts @@ -80,8 +80,11 @@ function describe(toolName: string): { category: string; excluded: boolean } { * Wrap one tool handler with dispatch telemetry. * * `ok` follows the CALLER-VISIBLE outcome: a handler that reports failure by returning an error - * envelope did not succeed, even though it never threw. That matches what the HTTP-level telemetry - * has always recorded (`response.status < 400`) so the two views of the same call agree. + * envelope did not succeed, even though it never threw. The HTTP-level telemetry in src/mcp/server.ts + * used to derive its own view from `response.status < 400` alone, which never agreed with this + * `ok` for a refused `tools/call` -- `enableJsonResponse: true` means a refusal is still HTTP 200. + * `handleMcpRequest` now reads the same JSON-RPC body this wrapper's caller produced (`result.isError` + * / a top-level `error`) before falling back to the status, so the two views agree there too. */ export function instrumentToolDispatch( toolName: string, diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 59162aa15..ad273907b 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -691,12 +691,17 @@ export async function handleMcpRequest(c: AppContext): Promise { const server = mcp.createServer(); try { const response = await createMcpHandler(server, { route: "/mcp", enableJsonResponse: true })(c.req.raw, c.env, executionCtx); + const statusOk = response.status < 400; + // #10035: `enableJsonResponse: true` above means a refused `tools/call` is still HTTP 200 -- the + // failure lives in the JSON-RPC body (`result.isError`), not the status line. Only a `tools/call` + // carries that body shape, so every other request keeps the status-derived outcome unchanged. + const ok = usageMetadata.rpcMethod === "tools/call" ? await resolveMcpToolCallOk(response, statusOk) : statusOk; if (typeof usageMetadata.toolName === "string") { - executionCtx.waitUntil(recordMcpToolTelemetry(c.env, usageMetadata.toolName, response.status < 400, Date.now() - startedAt)); + executionCtx.waitUntil(recordMcpToolTelemetry(c.env, usageMetadata.toolName, ok, Date.now() - startedAt)); } // #10175: PostHog's canonical protocol-level events. Only on a request that actually succeeded, so // a rejected handshake never inflates the session/client counts. - if (response.status < 400) { + if (statusOk) { if (usageMetadata.rpcMethod === "initialize") { recordMcpInitialize(c.env, defer, readInitializeHandshake(envelope), analyticsContext); } else if (usageMetadata.rpcMethod === "tools/list") { @@ -712,7 +717,7 @@ export async function handleMcpRequest(c: AppContext): Promise { route: "/mcp", actor: identity.actor, sessionId: identity.kind === "session" ? identity.session.id : undefined, - outcome: response.status >= 400 ? "error" : "success", + outcome: ok ? "success" : "error", latencyMs: Date.now() - startedAt, clientName: telemetry.clientName, clientVersion: telemetry.clientVersion, @@ -793,6 +798,29 @@ async function readMcpRequestEnvelope(request: Request): Promise { + const body = await response.clone().json().catch(() => null); + if (!body || typeof body !== "object") return statusOk; + const envelope = body as McpToolCallResponseEnvelope; + if (envelope.error) return false; + if (envelope.result?.isError === true) return false; + return statusOk; +} + function describeMcpUsageRequest( envelope: McpRequestEnvelope | null, method: string, diff --git a/test/integration/api.test.ts b/test/integration/api.test.ts index 3ce8c3530..81ba9765e 100644 --- a/test/integration/api.test.ts +++ b/test/integration/api.test.ts @@ -5843,6 +5843,45 @@ describe("api routes", () => { expect(JSON.stringify(mcpUsageEvents)).not.toMatch(/oktofeesh1|\/Users|github_pat|ghp_|source code|wallet|hotkey|raw trust/i); }, 15_000); + it("records a refused MCP tool call as a telemetry failure, not a success (#10035)", async () => { + const app = createApp(); + const env = createTestEnv(); + const { token: mcpSessionToken } = await createSessionForGitHubUser(env, { login: "jsonbored", id: 12345 }); + + const refusedToolCall = await app.request( + "/mcp", + { + method: "POST", + headers: { ...mcpHeaders(env), authorization: `Bearer ${mcpSessionToken}` }, + body: JSON.stringify({ + jsonrpc: "2.0", + id: "wrong-login-10035", + method: "tools/call", + params: { name: "loopover_get_decision_pack", arguments: { login: "someone-else" } }, + }), + }, + env, + ); + // enableJsonResponse means a refused tool call is still HTTP 200 -- the failure lives in the + // JSON-RPC body, not the status line. + expect(refusedToolCall.status).toBe(200); + await expect(mcpJson(refusedToolCall)).resolves.toMatchObject({ + result: { isError: true, content: [expect.objectContaining({ text: expect.stringContaining("authenticated GitHub login") })] }, + }); + + const usageEvents = await listProductUsageEvents(env, { limit: 20 }); + expect(usageEvents).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + surface: "mcp", + eventName: "mcp_tool_called", + outcome: "error", + metadata: expect.objectContaining({ toolName: "loopover_get_decision_pack", rpcMethod: "tools/call" }), + }), + ]), + ); + }); + it("gates the MCP contributor profile and redacts miner financial fields", async () => { const app = createApp(); const env = createTestEnv({ ADMIN_GITHUB_LOGINS: "oktofeesh1,other" }); diff --git a/test/unit/mcp-dispatch-telemetry.test.ts b/test/unit/mcp-dispatch-telemetry.test.ts index 90563cf5d..f0f946429 100644 --- a/test/unit/mcp-dispatch-telemetry.test.ts +++ b/test/unit/mcp-dispatch-telemetry.test.ts @@ -28,6 +28,7 @@ import { } from "@loopover/contract"; import { FORBIDDEN_CONTENT } from "../../scripts/forbidden-content"; import { instrumentToolDispatch, NOOP_DISPATCH_SINK, type DispatchTelemetrySink } from "../../src/mcp/dispatch-telemetry"; +import { resolveMcpToolCallOk } from "../../src/mcp/server"; const call: McpToolCallTelemetry = { tool: "loopover_get_repo_context", category: "maintainer", surface: "remote", ok: true, durationMs: 12 }; @@ -453,3 +454,32 @@ describe("PostHog canonical MCP analytics contract (#10175)", () => { expect(buildMcpToolsListProperties([]).$mcp_listed_tool_names).toEqual([]); }); }); + +describe("resolveMcpToolCallOk (#10035)", () => { + it("reports failure for a tools/call response whose result carries isError", async () => { + const response = new Response(JSON.stringify({ jsonrpc: "2.0", id: 1, result: { isError: true, content: [] } }), { status: 200 }); + await expect(resolveMcpToolCallOk(response, true)).resolves.toBe(false); + }); + + it("reports failure for a tools/call response with a top-level JSON-RPC error", async () => { + const response = new Response(JSON.stringify({ jsonrpc: "2.0", id: 1, error: { code: -32602, message: "bad params" } }), { status: 200 }); + await expect(resolveMcpToolCallOk(response, true)).resolves.toBe(false); + }); + + it("reports success for a normal tool result", async () => { + const response = new Response(JSON.stringify({ jsonrpc: "2.0", id: 1, result: { structuredContent: {} } }), { status: 200 }); + await expect(resolveMcpToolCallOk(response, true)).resolves.toBe(true); + }); + + it("falls back to the status-derived outcome when the body does not parse as JSON, rather than throwing", async () => { + const response = new Response("not-json", { status: 200 }); + await expect(resolveMcpToolCallOk(response, true)).resolves.toBe(true); + await expect(resolveMcpToolCallOk(response.clone(), false)).resolves.toBe(false); + }); + + it("leaves the response body available for the caller after reading it for telemetry", async () => { + const response = new Response(JSON.stringify({ jsonrpc: "2.0", id: 1, result: { isError: true, content: [] } }), { status: 200 }); + await resolveMcpToolCallOk(response, true); + await expect(response.json()).resolves.toMatchObject({ result: { isError: true } }); + }); +});