diff --git a/packages/loopover-mcp/bin/loopover-mcp.ts b/packages/loopover-mcp/bin/loopover-mcp.ts index 18b51a1dae..3cf119ca78 100644 --- a/packages/loopover-mcp/bin/loopover-mcp.ts +++ b/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, "proxied", ) as never, diff --git a/test/unit/mcp-gateway-mount-inprocess.test.ts b/test/unit/mcp-gateway-mount-inprocess.test.ts index 20a7ce7a83..a7473ff1c3 100644 --- a/test/unit/mcp-gateway-mount-inprocess.test.ts +++ b/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(); } diff --git a/test/unit/mcp-gateway-proxy.test.ts b/test/unit/mcp-gateway-proxy.test.ts new file mode 100644 index 0000000000..77fa95d0e8 --- /dev/null +++ b/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 }); + }); +}); diff --git a/test/unit/mcp-local-telemetry.test.ts b/test/unit/mcp-local-telemetry.test.ts index dcf190eec9..fcd33fa121 100644 --- a/test/unit/mcp-local-telemetry.test.ts +++ b/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 () => { diff --git a/test/unit/support/mcp-cli-harness.ts b/test/unit/support/mcp-cli-harness.ts index a858eb35ad..eb96121dc3 100644 --- a/test/unit/support/mcp-cli-harness.ts +++ b/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" }));