fix(mcp): derive the remote request-level ok from the tool result, not HTTP status (#10231)

enableJsonResponse means a refused tools/call is still HTTP 200, so the legacy
mcp_tool_call event and the product-usage outcome -- both keyed off
response.status -- reported every refusal as a success even though the
dispatch chokepoint's own usage_event already recorded it as a failure.
handleMcpRequest now reads the JSON-RPC body the same way it already reads the
request, falling back to the status on any parse failure, and only for
tools/call so every other request keeps its current behavior.

Co-authored-by: bitfathers94 <237535319+bitfathers94@users.noreply.github.com>

Author: bitfathers94

src/mcp/dispatch-telemetry.ts

@@ -87,8 +87,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<TArgs extends unknown[], TResult extends ToolResultLike>(
   toolName: string,

src/mcp/server.ts

@@ -691,12 +691,17 @@ export async function handleMcpRequest(c: AppContext): Promise<Response> {
   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<Response> {
       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<McpRequestEnvel
   return body && typeof body === "object" ? (body as McpRequestEnvelope) : null;
 }
 
+/** The JSON-RPC response fields that reveal a `tools/call`'s CALLER-VISIBLE outcome: a top-level `error`
+ *  (the request itself was rejected) or a `result.isError` envelope (the tool answered no). Structural and
+ *  permissive like {@link McpRequestEnvelope}, for the same reason -- this is the MCP SDK's own response,
+ *  not a contract this module owns. */
+type McpToolCallResponseEnvelope = { error?: unknown; result?: { isError?: unknown } };
+
+/**
+ * Derive a `tools/call` response's `ok` from its JSON-RPC body rather than the HTTP status (#10035):
+ * `enableJsonResponse: true` above means a refused tool call is still a 200, so `statusOk` alone reports a
+ * clean sheet for every refusal. Reads the response the same way {@link readMcpRequestEnvelope} reads the
+ * request -- clone before consuming, so the caller's own response body is untouched -- and falls back to
+ * `statusOk` on any parse failure rather than throwing: telemetry must never turn a working call into a
+ * failed one (src/mcp/dispatch-telemetry.ts's own guarantee).
+ */
+export async function resolveMcpToolCallOk(response: Response, statusOk: boolean): Promise<boolean> {
+  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,

test/integration/api.test.ts

@@ -5844,6 +5844,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" });

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 };
 
@@ -529,3 +530,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 } });
+  });
+});