fix(mcp): shape a remote JSON-RPC error into a tool result in the gateway proxy

packages/loopover-mcp/bin/loopover-mcp.ts

@@ -170,7 +170,7 @@ import {
   projectToolDefinition,
   ListPendingActionsStdioInput,
 } from "@loopover/contract/tools";
-import { AUTONOMY_LEVELS as MAINTAIN_AUTONOMY_LEVELS, MAINTAIN_ACTION_CLASSES, PROPOSE_ACTION_CLASSES, type ToolContract } from "@loopover/contract";
+import { AUTONOMY_LEVELS as MAINTAIN_AUTONOMY_LEVELS, MAINTAIN_ACTION_CLASSES, PROPOSE_ACTION_CLASSES, resolveErrorCode, type ToolContract } from "@loopover/contract";
 import { buildBranchAnalysisPayload, collectLocalDiff, collectLocalBranchMetadata, probeLocalScorer, referenceScorePreviewExample, resolveScorePreviewCommand, resolveWorkspaceCwd, sanitizeLocalScorerStatus, setupGuidanceForLocalScorer, isTestFile } from "../lib/local-branch.js";
 import { formatTable } from "../lib/format-table.js";
 import { argsWantJson, describeCliError, reportCliFailure } from "../lib/cli-error.js";
@@ -2579,7 +2579,21 @@ function registerProxiedTool(tool: RemoteToolDescriptor): void {
         // Forwarded verbatim to the remote's own tools/call: this layer routes, it does not interpret.
         const payload = await apiPost("/mcp", { jsonrpc: "2.0", id: Date.now(), method: "tools/call", params: { name: tool.name, arguments: input } });
         const result = (payload as { result?: unknown }).result;
-        return result ?? payload;
+        if (result !== undefined) return result;
+        // `enableJsonResponse` (src/mcp/server.ts) means a request-level failure still arrives as HTTP 200,
+        // with a JSON-RPC `{ error }` envelope in place of `result` -- apiPost only throws on a non-2xx, so
+        // that envelope (or, degenerately, neither key at all) would otherwise be handed back verbatim as
+        // if it were the tool's own answer. Shape it into a real CallToolResult instead: the numeric
+        // JSON-RPC `code` is not a member of the closed telemetry vocabulary, so it is never surfaced as
+        // one -- resolveErrorCode reclassifies from the message, the same as every other server here.
+        const rpcError = (payload as { error?: { message?: unknown } }).error;
+        const message =
+          typeof rpcError?.message === "string" ? rpcError.message : "The remote MCP server returned neither a result nor an error for this call.";
+        return {
+          content: [{ type: "text" as const, text: message }],
+          structuredContent: { error: { code: resolveErrorCode(message), message } },
+          isError: true as const,
+        };
       }) as (...args: unknown[]) => Promise<unknown>,
       "proxied",
     ) as never,

test/unit/mcp-gateway-mount-inprocess.test.ts

@@ -167,19 +167,24 @@ describe("mountRemoteTools against the real server (#9526)", () => {
     }
   });
 
-  it("hands back the WHOLE payload when the remote's envelope carries no `result`", async () => {
-    // The `??` fallback, and a real posture rather than a defensive shrug: a remote that answers a shape
-    // this package does not model must still reach the caller intact, so the caller can see what came back
-    // instead of an empty success.
+  it("shapes a resultless envelope into a conformant isError result rather than handing it back raw (#10036)", async () => {
+    // A remote answering neither `result` nor `error` is not a CallToolResult either -- returning it
+    // verbatim used to hand the client a bare `{ jsonrpc, id, note }` object with no `content`/`isError` at
+    // all. It must get the same treatment as a JSON-RPC error: a readable isError:true result.
     await mod.mountRemoteTools({
       argv: ["--stdio"],
       fetchImpl: remoteToolsFetch([{ name: "loopover_gateway_resultless" }]),
     });
     const client = await connect("gateway-resultless");
     try {
-      const raw = (await client.callTool({ name: "loopover_gateway_resultless", arguments: {} })) as { note?: string; isError?: boolean };
-      expect(raw.isError).toBeFalsy();
-      expect(raw.note, "the envelope itself reaches the caller when it carries no result").toBe("no result member");
+      const result = (await client.callTool({ name: "loopover_gateway_resultless", arguments: {} })) as {
+        isError?: boolean;
+        content?: Array<{ type: string; text?: string }>;
+        structuredContent?: { error?: { code?: string; message?: string } };
+      };
+      expect(result.isError).toBe(true);
+      expect(result.content?.[0]?.text).toBeTruthy();
+      expect(result.structuredContent?.error?.code).toBeTruthy();
     } finally {
       await client.close();
     }

test/unit/mcp-gateway-proxy.test.ts

@@ -0,0 +1,105 @@
+// REGRESSION (#10036): registerProxiedTool's handler used to hand a remote JSON-RPC error envelope
+// back to the caller AS IF it were the tool's own result. `apiPost` only throws on a non-2xx HTTP
+// status, and the remote runs with `enableJsonResponse: true`, so a request-level failure -- an
+// unknown tool, bad arguments, whatever -- comes back as HTTP 200 with `{ jsonrpc, id, error }` and no
+// `result` key. `result ?? payload` returned that raw envelope verbatim: no `content`, no `isError`, not
+// a CallToolResult at all.
+//
+// Drives the real `registerProxiedTool` in-process (mounted through `mountRemoteTools`, connected over
+// an in-memory transport) rather than unit-testing a helper pulled out for the occasion: the bug lived
+// in the handler closure itself, and `packages/loopover-mcp/bin/loopover-mcp.ts` reports zero coverage
+// under subprocess spawn, so only an in-process call attributes these lines to the patch (mirrors
+// test/contract/validate-mcp.test.ts, which imports this same module the same way).
+import { Client } from "@modelcontextprotocol/sdk/client/index.js";
+import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js";
+import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";
+import { MCP_TELEMETRY_ERROR_CODES } from "@loopover/contract";
+import type { GatewayFetch, RemoteToolDescriptor } from "../../packages/loopover-mcp/lib/gateway";
+
+type ToolCallResult = { isError?: boolean; content?: Array<{ type: string; text?: string }>; structuredContent?: unknown };
+
+const REMOTE_TOOL: RemoteToolDescriptor = {
+  name: "loopover_gateway_proxy_probe",
+  title: "Gateway proxy probe",
+  description: "A remote-only tool this package does not model, mounted purely to exercise the proxy handler.",
+  inputSchema: { type: "object" },
+};
+
+/** Answers the gateway's OWN discovery call (`mountRemoteTools`'s `fetchImpl`) with one remote tool. */
+const discoveryFetch: GatewayFetch = async () => ({
+  ok: true,
+  status: 200,
+  json: async () => ({ result: { tools: [REMOTE_TOOL] } }),
+});
+
+/** What the proxied tool's own `tools/call` (routed through `apiPost`, i.e. the real global `fetch`) answers. */
+let nextCallResponse: unknown;
+
+let client: Client;
+
+beforeAll(async () => {
+  vi.stubEnv("LOOPOVER_API_TOKEN", "test-session-token");
+  vi.stubGlobal(
+    "fetch",
+    vi.fn(async () => ({
+      ok: true,
+      status: 200,
+      headers: { get: () => null },
+      text: async () => JSON.stringify(nextCallResponse),
+    })),
+  );
+
+  const { server, mountRemoteTools } = await import("../../packages/loopover-mcp/bin/loopover-mcp");
+  const mounted = await mountRemoteTools({ argv: [], fetchImpl: discoveryFetch });
+  if (mounted.status !== "mounted" || !mounted.tools.some((tool) => tool.name === REMOTE_TOOL.name)) {
+    throw new Error(`expected ${REMOTE_TOOL.name} to mount, got: ${JSON.stringify(mounted)}`);
+  }
+
+  const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
+  client = new Client({ name: "mcp-gateway-proxy-test", version: "0.0.0" });
+  await Promise.all([server.connect(serverTransport), client.connect(clientTransport)]);
+});
+
+afterAll(async () => {
+  await client.close().catch(() => undefined);
+  vi.unstubAllEnvs();
+  vi.unstubAllGlobals();
+});
+
+describe("registerProxiedTool's handler translates the remote's JSON-RPC envelope (#10036)", () => {
+  it("REGRESSION: a remote JSON-RPC error must not be returned as the tool's result", async () => {
+    nextCallResponse = { jsonrpc: "2.0", id: 1, error: { code: -32602, message: "Tool loopover_x not found" } };
+
+    const result = (await client.callTool({ name: REMOTE_TOOL.name, arguments: {} })) as ToolCallResult;
+    expect(result.isError).toBe(true);
+    expect(result.content?.length).toBeGreaterThan(0);
+    expect(result.content?.[0]?.text).toContain("Tool loopover_x not found");
+    const structured = result.structuredContent as { error?: { code?: unknown; message?: unknown } };
+    expect(structured.error?.message).toBe("Tool loopover_x not found");
+    expect(MCP_TELEMETRY_ERROR_CODES).toContain(structured.error?.code);
+    // The JSON-RPC numeric code is not part of the closed telemetry vocabulary and must never leak through.
+    expect(structured.error?.code).not.toBe(-32602);
+  });
+
+  it("a payload carrying neither result nor error also becomes an isError:true result", async () => {
+    nextCallResponse = { jsonrpc: "2.0", id: 1 };
+
+    const result = (await client.callTool({ name: REMOTE_TOOL.name, arguments: {} })) as ToolCallResult;
+    expect(result.isError).toBe(true);
+    const structured = result.structuredContent as { error?: { code?: unknown; message?: unknown } };
+    expect(MCP_TELEMETRY_ERROR_CODES).toContain(structured.error?.code);
+  });
+
+  it("a payload carrying a result is still returned verbatim, unwrapped, with no added isError", async () => {
+    nextCallResponse = {
+      jsonrpc: "2.0",
+      id: 1,
+      result: { content: [{ type: "text", text: "hello from the remote" }], structuredContent: { ok: true } },
+    };
+
+    const result = (await client.callTool({ name: REMOTE_TOOL.name, arguments: {} })) as ToolCallResult;
+    expect(result.isError).toBeUndefined();
+    expect(result.content?.[0]?.text).toBe("hello from the remote");
+    expect(result.structuredContent).toEqual({ ok: true });
+  });
+});

test/unit/mcp-local-telemetry.test.ts

@@ -372,6 +372,29 @@ describe("recordStdioToolTelemetry / wrapStdioToolHandler (#8690)", () => {
     expect(usage.properties).toMatchObject({ surface: "stdio", transport: "proxied" });
   });
 
+  // #10036: the counterpart to the assertion just above. registerProxiedTool's handler used to hand back
+  // the remote's raw JSON-RPC `{ error }` envelope, which has no `isError`, so `ok = result?.isError !==
+  // true` read every remote refusal as a SUCCESS -- a proxied failure recorded no differently from a
+  // proxied success, with gateway failure rate unmeasurable. Now that the handler shapes a conformant
+  // `isError: true` result with a closed-set envelope, this must record ok:false + the resolved error_code.
+  it("wrapStdioToolHandler records a PROXIED remote refusal as a failure with a resolved error_code", async () => {
+    vi.stubEnv("LOOPOVER_MCP_POSTHOG_API_KEY", "phc_test");
+    const wrapped = wrapStdioToolHandler(
+      "loopover_lint_pr_text",
+      () => true,
+      async () => ({
+        isError: true,
+        content: [{ type: "text", text: "Tool loopover_x not found" }],
+        structuredContent: { error: { code: "not_found", message: "Tool loopover_x not found" } },
+      }),
+      "proxied",
+    );
+    await wrapped({});
+
+    const usage = h.captureSpy.mock.calls.map((entry) => entry[0] as CapturedMessage).find((message) => message.event === "usage_event")!;
+    expect(usage.properties).toMatchObject({ surface: "stdio", transport: "proxied", ok: false, error_code: "not_found" });
+  });
+
   // #9659: the stdio wrapper passed NO error on the returned-failure path, so `resolveErrorCode(undefined)`
   // classified every one of them as `unknown_error` no matter what the tool told its caller.
   it("wrapStdioToolHandler resolves the error code from the result's own envelope", async () => {

test/unit/support/mcp-cli-harness.ts

@@ -270,7 +270,7 @@ export async function startFixtureServer(
       request.on("end", () => {
         const parsed = JSON.parse(raw || "{}") as { id?: unknown; params?: { name?: string; arguments?: unknown } };
         // A tool named `*_resultless` gets an envelope with NO `result` member, so the gateway's
-        // "hand back the whole payload" fallback can be exercised against a real response rather than a
+        // neither-result-nor-error handling (#10036) can be exercised against a real response rather than a
         // hand-built object. Keyed on the name because one fixture serves every gateway test.
         if (parsed.params?.name?.endsWith("_resultless")) {
           response.end(JSON.stringify({ jsonrpc: "2.0", id: parsed.id ?? 1, note: "no result member" }));