diff --git a/changelog.d/659.fixed.md b/changelog.d/659.fixed.md new file mode 100644 index 00000000..98a15148 --- /dev/null +++ b/changelog.d/659.fixed.md @@ -0,0 +1 @@ +**Protocol errors reach SDK clients with a single `MCP error :` prefix** — every `McpError` thrown from a request handler (argument validation, an unknown tool/prompt/resource, `expectedContent` on an attach session, …) arrived at `@modelcontextprotocol/sdk` clients as `MCP error -32602: MCP error -32602: …`: the SDK copies the thrown error's `.message` — which its own constructor has already prefixed — verbatim onto the wire, and the client-side constructor prefixes it again (the dev proxy had been hiding it with a dedupe). The tools/call, prompt and resource handlers now convert at the JSON-RPC boundary (`WireMcpError`: same class, code and data, bare `.message`), so the wire carries the reason once; in-process throw sites and logs are unchanged (#659) diff --git a/src/errors/debug-errors.ts b/src/errors/debug-errors.ts index 8f1b532f..721b7ceb 100644 --- a/src/errors/debug-errors.ts +++ b/src/errors/debug-errors.ts @@ -15,10 +15,11 @@ import { ErrorCode as McpErrorCode } from '@modelcontextprotocol/sdk/types.js'; * Base for mcp-debugger's typed errors. * * McpError's constructor bakes `MCP error : ` into `.message`, which is - * right on the JSON-RPC path (the SDK derives the wire error from it) and wrong - * inside a tool result envelope, where it reads like a transport failure - * (issue #647). The SDK keeps no copy of the plain text, so this base records - * it as `detail`; tool result envelopes and getErrorMessage() report that. + * right for logs and in-process callers and wrong inside a tool result + * envelope, where it reads like a transport failure (issue #647). The SDK + * keeps no copy of the plain text, so this base records it as `detail`; tool + * result envelopes and getErrorMessage() report that, and the JSON-RPC + * boundary sends it on the wire (toWireError, issue #659). */ export abstract class DebugError extends McpError { /** The message without the SDK's `MCP error : ` prefix. */ @@ -168,6 +169,51 @@ export class DebugSessionCreationError extends DebugError { } } +/** + * The plain text of an McpError: a DebugError's recorded `detail`, otherwise + * `.message` with the single `MCP error : ` prefix its constructor added. + * Only the error's own code is stripped, and only once — a message that + * legitimately begins with that text for another reason is left alone. + */ +export function mcpErrorDetail(error: McpError): string { + if (error instanceof DebugError) { + return error.detail; + } + const prefix = `MCP error ${error.code}: `; + return error.message.startsWith(prefix) ? error.message.slice(prefix.length) : error.message; +} + +/** + * The McpError shape the JSON-RPC boundary throws (issue #659). + * + * The SDK's Protocol copies a thrown error's `.code` and `.message` verbatim + * into the JSON-RPC error, and the client-side McpError constructor prefixes + * `MCP error : ` again — so an ordinary McpError thrown from a request + * handler reaches SDK clients as `MCP error -32602: MCP error -32602: …`. + * This subclass keeps the class (instanceof, `.code`, `.data`) and resets + * `.message` to the bare detail so the wire carries the text once. It is for + * the request-handler boundary only; everything upstream keeps throwing + * McpError / DebugError, whose prefixed `.message` is right in logs. + */ +export class WireMcpError extends McpError { + constructor(code: McpErrorCode, detail: string, data?: unknown) { + super(code, detail, data); + this.message = detail; + } +} + +/** Convert an McpError for the JSON-RPC boundary; a WireMcpError passes through. */ +export function toWireError(error: McpError): WireMcpError { + if (error instanceof WireMcpError) { + return error; + } + const wire = new WireMcpError(error.code, mcpErrorDetail(error), error.data); + if (error.stack) { + wire.stack = error.stack; + } + return wire; +} + /** * Helper to extract a user-facing error message safely. A DebugError reports * its plain `detail` rather than the prefixed McpError message (issue #647). diff --git a/src/server/output-resources.ts b/src/server/output-resources.ts index 34922c85..a5603cc2 100644 --- a/src/server/output-resources.ts +++ b/src/server/output-resources.ts @@ -13,10 +13,10 @@ import { ReadResourceRequestSchema, SubscribeRequestSchema, UnsubscribeRequestSchema, - ErrorCode as McpErrorCode, - McpError + ErrorCode as McpErrorCode } from '@modelcontextprotocol/sdk/types.js'; import { ILogger, type IFileSystem } from '@debugmcp/shared'; +import { WireMcpError } from '../errors/debug-errors.js'; import type { SessionManager } from '../session/session-manager.js'; import { proxyLogPathFor } from '../proxy/session-log-layout.js'; import { readProxyLogTail } from '../session/launch/proxy-failure-diagnostics.js'; @@ -162,11 +162,11 @@ export function registerResourceHandlers( const sessionId = outputSessionId ?? proxyLogSessionId; const session = sessionId ? sessionManager.getSession(sessionId) : undefined; if (!session) { - throw new McpError(McpErrorCode.InvalidParams, `Unknown resource: ${uri}`); + throw new WireMcpError(McpErrorCode.InvalidParams, `Unknown resource: ${uri}`); } if (proxyLogSessionId) { if (!session.logDir) { - throw new McpError(McpErrorCode.InvalidParams, `Unknown resource: ${uri}`); + throw new WireMcpError(McpErrorCode.InvalidParams, `Unknown resource: ${uri}`); } const text = await readProxyLogTail( fileSystem, @@ -190,7 +190,7 @@ export function registerResourceHandlers( const uri = request.params.uri; const sessionId = parseOutputResourceUri(uri); if (!sessionId || !sessionManager.getSession(sessionId)) { - throw new McpError(McpErrorCode.InvalidParams, `Unknown resource: ${uri}`); + throw new WireMcpError(McpErrorCode.InvalidParams, `Unknown resource: ${uri}`); } notifier.subscribe(uri); return {}; diff --git a/src/server/prompts.ts b/src/server/prompts.ts index 4a6f1be3..646ce72b 100644 --- a/src/server/prompts.ts +++ b/src/server/prompts.ts @@ -8,10 +8,10 @@ import { Server } from '@modelcontextprotocol/sdk/server/index.js'; import { ListPromptsRequestSchema, GetPromptRequestSchema, - ErrorCode as McpErrorCode, - McpError + ErrorCode as McpErrorCode } from '@modelcontextprotocol/sdk/types.js'; import { IEnvironment } from '@debugmcp/shared'; +import { WireMcpError } from '../errors/debug-errors.js'; import { buildDebuggingWorkflowPrompt } from '../skill-content.js'; import { getBpAddressingMode } from '../utils/bp-addressing.js'; @@ -28,7 +28,7 @@ export function registerPromptHandlers(server: Server, environment: IEnvironment server.setRequestHandler(GetPromptRequestSchema, async (request) => { if (request.params.name !== promptDescriptor.name) { - throw new McpError(McpErrorCode.InvalidParams, `Unknown prompt: ${request.params.name}`); + throw new WireMcpError(McpErrorCode.InvalidParams, `Unknown prompt: ${request.params.name}`); } return { description: promptDescriptor.description, diff --git a/src/server/tool-dispatch.ts b/src/server/tool-dispatch.ts index 70162d4b..25655df2 100644 --- a/src/server/tool-dispatch.ts +++ b/src/server/tool-dispatch.ts @@ -12,6 +12,7 @@ import { McpError, ServerResult } from '@modelcontextprotocol/sdk/types.js'; +import { toWireError } from '../errors/debug-errors.js'; import { coerceToolArguments, ToolArguments } from './tool-arguments.js'; import { extractPayloadSuccess, sanitizeRequest } from './tool-result.js'; import { buildToolDefinitions, isToolName } from './tool-schemas.js'; @@ -85,8 +86,13 @@ export function registerToolHandlers(server: Server, ctx: ToolContext): void { timestamp: Date.now() }); - if (error instanceof McpError) throw error; - throw new McpError(McpErrorCode.InternalError, `Failed to execute tool ${toolName}: ${errorMessage}`); + // The JSON-RPC boundary: the SDK copies .message onto the wire and the + // client prefixes it again, so send the bare detail (issue #659). + throw toWireError( + error instanceof McpError + ? error + : new McpError(McpErrorCode.InternalError, `Failed to execute tool ${toolName}: ${errorMessage}`) + ); } } ); diff --git a/tests/core/unit/errors/debug-errors.test.ts b/tests/core/unit/errors/debug-errors.test.ts index 30bdf397..411a2190 100644 --- a/tests/core/unit/errors/debug-errors.test.ts +++ b/tests/core/unit/errors/debug-errors.test.ts @@ -1,7 +1,8 @@ /** * The typed error hierarchy keeps two views of one message (issue #647): - * `.message` carries the SDK's `MCP error : ` prefix for the JSON-RPC - * path, `.detail` is the plain text tool result envelopes report. + * `.message` carries the SDK's `MCP error : ` prefix for logs and + * in-process callers, `.detail` is the plain text tool result envelopes report + * and the JSON-RPC boundary sends on the wire (issue #659). */ import { describe, it, expect } from 'vitest'; import { McpError } from '@modelcontextprotocol/sdk/types.js'; @@ -16,7 +17,10 @@ import { SessionNotFoundError, SessionTerminatedError, UnsupportedFeatureError, - UnsupportedLanguageError + UnsupportedLanguageError, + WireMcpError, + mcpErrorDetail, + toWireError } from '../../../../src/errors/debug-errors.js'; describe('DebugError hierarchy', () => { @@ -99,3 +103,33 @@ describe('DebugError hierarchy', () => { expect(getErrorMessage('text')).toBe('text'); }); }); + +describe('the JSON-RPC boundary shape (issue #659)', () => { + it('mcpErrorDetail strips exactly one prefix, matching the error\'s own code', () => { + expect(mcpErrorDetail(new McpError(McpErrorCode.InvalidParams, 'bad arg'))).toBe('bad arg'); + expect(mcpErrorDetail(new SessionNotFoundError('s1'))).toBe('Session not found: s1'); + // A detail that itself starts with a prefix (a client re-wrapping a wire + // message) keeps that one: only the layer this error added comes off. + expect(mcpErrorDetail(new McpError(McpErrorCode.InvalidParams, 'MCP error -32602: nested'))) + .toBe('MCP error -32602: nested'); + }); + + it('WireMcpError is an McpError whose .message is the bare detail', () => { + const error = new WireMcpError(McpErrorCode.InvalidParams, 'bad arg', { k: 1 }); + expect(error).toBeInstanceOf(McpError); + expect(error.code).toBe(McpErrorCode.InvalidParams); + expect(error.message).toBe('bad arg'); + expect(error.data).toEqual({ k: 1 }); + }); + + it('toWireError keeps code, data and stack and is idempotent', () => { + const source = new ProxyNotRunningError('s1', 'pause'); + const wire = toWireError(source); + expect(wire).toBeInstanceOf(WireMcpError); + expect(wire.code).toBe(source.code); + expect(wire.data).toEqual(source.data); + expect(wire.stack).toBe(source.stack); + expect(wire.message).toBe('Cannot pause: no active proxy for session s1'); + expect(toWireError(wire)).toBe(wire); + }); +}); diff --git a/tests/core/unit/server/mcp-wire-errors.test.ts b/tests/core/unit/server/mcp-wire-errors.test.ts new file mode 100644 index 00000000..407b4db3 --- /dev/null +++ b/tests/core/unit/server/mcp-wire-errors.test.ts @@ -0,0 +1,138 @@ +/** + * Protocol errors as an SDK client sees them (issue #659). + * + * The SDK's Protocol copies a thrown error's `.message` verbatim into the + * JSON-RPC error and the client-side McpError constructor prefixes + * `MCP error : ` again, so an McpError thrown from a request handler + * used to reach clients doubled. These tests drive the real handlers through + * a real Server/Client pair over the SDK's in-memory transport and pin the + * exact client-side message: one prefix, then the reason. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { Server } from '@modelcontextprotocol/sdk/server/index.js'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; +import { McpError, ErrorCode as McpErrorCode } from '@modelcontextprotocol/sdk/types.js'; +import { registerToolHandlers } from '../../../../src/server/tool-dispatch.js'; +import { registerPromptHandlers } from '../../../../src/server/prompts.js'; +import { registerResourceHandlers } from '../../../../src/server/output-resources.js'; +import type { ToolContext } from '../../../../src/server/tool-context.js'; +import type { SessionManager } from '../../../../src/session/session-manager.js'; +import type { OutputResourceNotifier } from '../../../../src/server/output-resources.js'; +import { SessionNotFoundError } from '../../../../src/errors/debug-errors.js'; + +const logger = { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() }; +const environment = { + get: (): string | undefined => undefined, + getAll: () => ({}), + getCurrentWorkingDirectory: () => '/' +}; + +function fakeContext(overrides: Partial = {}): ToolContext { + return { + logger, + environment, + getSessionName: () => 'sess', + getSupportedLanguagesAsync: async () => ['mock'], + validateSession: (sessionId: string) => { + throw new SessionNotFoundError(sessionId); + }, + ...overrides + } as unknown as ToolContext; +} + +async function connect(server: Server): Promise { + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: 'wire-test', version: '0.0.0' }); + await Promise.all([server.connect(serverTransport), client.connect(clientTransport)]); + return client; +} + +async function caught(promise: Promise): Promise { + try { + await promise; + } catch (error) { + expect(error).toBeInstanceOf(McpError); + return error as McpError; + } + throw new Error('expected the request to be rejected'); +} + +describe('protocol errors reach SDK clients with a single MCP error prefix', () => { + let server: Server; + let client: Client; + + beforeEach(() => { + server = new Server( + { name: 'wire-test-server', version: '0.0.0' }, + { capabilities: { tools: {}, prompts: {}, resources: { subscribe: true } } } + ); + }); + + afterEach(async () => { + await client?.close(); + await server.close(); + }); + + it('argument validation (tools/call)', async () => { + registerToolHandlers(server, fakeContext()); + client = await connect(server); + + const error = await caught(client.callTool({ name: 'set_breakpoint', arguments: { file: '/x.py', line: 1 } })); + expect(error.code).toBe(McpErrorCode.InvalidParams); + expect(error.message).toBe(`MCP error ${McpErrorCode.InvalidParams}: Missing required parameter: sessionId`); + }); + + it('a handler rejection that is not an McpError is wrapped once as InternalError', async () => { + registerToolHandlers( + server, + fakeContext({ + getAdapterRegistry: () => { + throw new Error('registry exploded'); + } + }) + ); + client = await connect(server); + + const error = await caught(client.callTool({ name: 'list_supported_languages', arguments: {} })); + expect(error.code).toBe(McpErrorCode.InternalError); + expect(error.message).toBe( + `MCP error ${McpErrorCode.InternalError}: Failed to list supported languages: registry exploded` + ); + }); + + it('an unknown tool', async () => { + registerToolHandlers(server, fakeContext()); + client = await connect(server); + + const error = await caught(client.callTool({ name: 'no_such_tool', arguments: {} })); + expect(error.code).toBe(McpErrorCode.MethodNotFound); + expect(error.message).toBe(`MCP error ${McpErrorCode.MethodNotFound}: Unknown tool: no_such_tool`); + }); + + it('an unknown prompt', async () => { + registerPromptHandlers(server, environment); + client = await connect(server); + + const error = await caught(client.getPrompt({ name: 'nope' })); + expect(error.code).toBe(McpErrorCode.InvalidParams); + expect(error.message).toBe(`MCP error ${McpErrorCode.InvalidParams}: Unknown prompt: nope`); + }); + + it('an unknown resource (read and subscribe)', async () => { + const sessionManager = { + getAllSessions: () => [], + getSession: () => undefined + } as unknown as SessionManager; + const notifier = { subscribe: vi.fn(), unsubscribe: vi.fn() } as unknown as OutputResourceNotifier; + registerResourceHandlers(server, sessionManager, notifier, { readTail: vi.fn() }); + client = await connect(server); + + const uri = 'debug://sessions/ghost/output'; + for (const request of [client.readResource({ uri }), client.subscribeResource({ uri })]) { + const error = await caught(request); + expect(error.code).toBe(McpErrorCode.InvalidParams); + expect(error.message).toBe(`MCP error ${McpErrorCode.InvalidParams}: Unknown resource: ${uri}`); + } + }); +});