diff --git a/docs/error-handling-guide.md b/docs/error-handling-guide.md index 217a8a30..b93beac7 100644 --- a/docs/error-handling-guide.md +++ b/docs/error-handling-guide.md @@ -73,8 +73,8 @@ if (!session.proxyManager?.isRunning()) { The error handling uses a mixed strategy across three layers: 1. **Implementation Layer (Backend)** - Uses typed `McpError` subclasses (e.g., `SessionNotFoundError`, `ProxyNotRunningError`) for infrastructure failures (unknown session, terminated session, proxy not running). These are defined in `src/errors/debug-errors.ts`. Operation-level failures (e.g., session not paused, missing thread ID) return structured `DebugResult` objects with `success: false` rather than throwing. -2. **Server Layer** - The MCP server (`src/server.ts`) throws `McpError` directly only for invalid parameters before reaching the session manager (e.g., missing `sessionId`) or for unexpected non-`Error` throws. The typed session-lifecycle errors from the implementation layer (`SessionNotFoundError`, `SessionTerminatedError`, `ProxyNotRunningError`) are caught in each tool handler and converted into a successful tool response whose JSON payload has `success: false, error: ` -- they are not propagated as protocol-level `McpError`. `DebugResult` failures are likewise returned as successful tool responses with `success: false`. -3. **Client Layer** - Receives either a protocol-level MCP error (only from invalid-parameter checks or unexpected non-`Error` throws) or a structured failure result with `success: false` (for typed session errors and `DebugResult` failures), depending on which layer the failure originated in +2. **Server Layer** - The MCP server (`src/server.ts`) throws `McpError` directly for invalid parameters before reaching the session manager (e.g., missing `sessionId`) and for unexpected internal errors (wrapped as `McpError` with `InternalError`). The typed session-lifecycle errors (`SessionNotFoundError`, `SessionTerminatedError`, `ProxyNotRunningError`) -- thrown by the server's own `validateSession` pre-check and by the implementation layer -- are caught in each session-scoped tool handler and converted into a successful tool response whose JSON payload has `success: false, error: ` -- they are not propagated as protocol-level `McpError`. `DebugResult` failures are likewise returned as successful tool responses with `success: false`. +3. **Client Layer** - Receives either a protocol-level MCP error (from invalid-parameter checks or unexpected internal errors) or a structured failure result with `success: false` (for typed session errors and `DebugResult` failures), depending on which layer the failure originated in ```typescript // Implementation (throws typed error for infrastructure failures) diff --git a/docs/tool-reference.md b/docs/tool-reference.md index 74264e5e..0d940297 100644 --- a/docs/tool-reference.md +++ b/docs/tool-reference.md @@ -767,8 +767,10 @@ Tools can return errors in two formats: 2. **Application-level failures**: JSON payloads with `{ "success": false, "error": "..." }`. Most tool failures use this format, where the HTTP/transport layer succeeds but the operation itself failed. ### Common Error Codes (MCP transport errors) -- `-32603`: Internal error (feature not implemented, session not found, etc.) -- `-32602`: Invalid parameters +- `-32603`: Internal error (feature not implemented, unexpected failures) +- `-32602`: Invalid parameters (e.g., missing `sessionId`) + +Session-lifecycle failures (unknown/terminated session, proxy not running) are application-level failures: they return `{ "success": false, "error": "..." }` rather than an MCP transport error. ### MCP Error Response Format ```json diff --git a/src/server.ts b/src/server.ts index a50d7be0..6785dcaf 100644 --- a/src/server.ts +++ b/src/server.ts @@ -273,11 +273,13 @@ export class DebugMcpServer { private validateSession(sessionId: string): void { const session = this.sessionManager.getSession(sessionId); if (!session) { - throw new McpError(McpErrorCode.InvalidParams, `Session not found: ${sessionId}`); + // Typed subclass of McpError (same code/message) so per-tool catch blocks + // can convert session-lifecycle failures into {success: false} results + throw new SessionNotFoundError(sessionId); } // Check the new lifecycle state instead of legacy state if (session.sessionLifecycle === SessionLifecycleState.TERMINATED) { - throw new McpError(McpErrorCode.InvalidRequest, `Session is terminated: ${sessionId}`); + throw new SessionTerminatedError(sessionId); } } @@ -1282,12 +1284,12 @@ export class DebugMcpServer { return { content: [{ type: 'text', text: JSON.stringify(result) }] }; } catch (error) { this.logger.error('Failed to pause execution', { error }); - if (error instanceof McpError) throw error; if (error instanceof SessionTerminatedError || error instanceof SessionNotFoundError || error instanceof ProxyNotRunningError) { return { content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }) }] }; } + if (error instanceof McpError) throw error; throw new McpError(McpErrorCode.InternalError, `Failed to pause execution: ${(error as Error).message}`); } } @@ -1299,6 +1301,11 @@ export class DebugMcpServer { return { content: [{ type: 'text', text: JSON.stringify({ success: true, threads }) }] }; } catch (error) { this.logger.error('Failed to list threads', { error }); + if (error instanceof SessionTerminatedError || + error instanceof SessionNotFoundError || + error instanceof ProxyNotRunningError) { + return { content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }) }] }; + } if (error instanceof McpError) throw error; throw new McpError(McpErrorCode.InternalError, `Failed to list threads: ${(error as Error).message}`); } @@ -1428,6 +1435,11 @@ export class DebugMcpServer { }; } catch (error) { this.logger.error('Failed to get source context', { error }); + if (error instanceof SessionTerminatedError || + error instanceof SessionNotFoundError || + error instanceof ProxyNotRunningError) { + return { content: [{ type: 'text', text: JSON.stringify({ success: false, error: error.message }) }] }; + } if (error instanceof McpError) throw error; throw new McpError(McpErrorCode.InternalError, `Failed to get source context: ${(error as Error).message}`); } diff --git a/tests/core/unit/server/server-control-tools.test.ts b/tests/core/unit/server/server-control-tools.test.ts index c439feaf..099425bf 100644 --- a/tests/core/unit/server/server-control-tools.test.ts +++ b/tests/core/unit/server/server-control-tools.test.ts @@ -4,10 +4,9 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { Server } from '@modelcontextprotocol/sdk/server/index.js'; import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; -import { McpError } from '@modelcontextprotocol/sdk/types.js'; import { DebugMcpServer } from '../../../../src/server.js'; import { SessionManager } from '../../../../src/session/session-manager.js'; -import { Breakpoint } from '@debugmcp/shared'; +import { Breakpoint, SessionLifecycleState } from '@debugmcp/shared'; import { ErrorMessages } from '../../../../src/utils/error-messages.js'; import { createProductionDependencies } from '../../../../src/container/dependencies.js'; import { @@ -504,13 +503,37 @@ describe('Server Control Tools Tests', () => { it('should handle pause on non-existent session', async () => { mockSessionManager.getSession.mockReturnValue(null); - await expect(callToolHandler({ + const result = await callToolHandler({ method: 'tools/call', params: { name: 'pause_execution', arguments: { sessionId: 'non-existent' } } - })).rejects.toThrow(McpError); + }); + + // Session-lifecycle failures surface as structured results, not protocol errors + const content = JSON.parse(result.content[0].text); + expect(content.success).toBe(false); + expect(content.error).toContain('Session not found: non-existent'); + }); + + it('should handle pause on terminated session', async () => { + mockSessionManager.getSession.mockReturnValue({ + id: 'test-session', + sessionLifecycle: SessionLifecycleState.TERMINATED + }); + + const result = await callToolHandler({ + method: 'tools/call', + params: { + name: 'pause_execution', + arguments: { sessionId: 'test-session' } + } + }); + + const content = JSON.parse(result.content[0].text); + expect(content.success).toBe(false); + expect(content.error).toContain('Session is terminated: test-session'); }); }); @@ -545,13 +568,37 @@ describe('Server Control Tools Tests', () => { it('should handle list_threads on non-existent session', async () => { mockSessionManager.getSession.mockReturnValue(null); - await expect(callToolHandler({ + const result = await callToolHandler({ + method: 'tools/call', + params: { + name: 'list_threads', + arguments: { sessionId: 'test-session' } + } + }); + + // Session-lifecycle failures surface as structured results, not protocol errors + const content = JSON.parse(result.content[0].text); + expect(content.success).toBe(false); + expect(content.error).toContain('Session not found: test-session'); + }); + + it('should handle list_threads on terminated session', async () => { + mockSessionManager.getSession.mockReturnValue({ + id: 'test-session', + sessionLifecycle: SessionLifecycleState.TERMINATED + }); + + const result = await callToolHandler({ method: 'tools/call', params: { name: 'list_threads', arguments: { sessionId: 'test-session' } } - })).rejects.toThrow(McpError); + }); + + const content = JSON.parse(result.content[0].text); + expect(content.success).toBe(false); + expect(content.error).toContain('Session is terminated: test-session'); }); it('should reject list_threads with missing sessionId', async () => { diff --git a/tests/core/unit/server/server-inspection-tools.test.ts b/tests/core/unit/server/server-inspection-tools.test.ts index 9bf782be..3ec903d1 100644 --- a/tests/core/unit/server/server-inspection-tools.test.ts +++ b/tests/core/unit/server/server-inspection-tools.test.ts @@ -146,33 +146,18 @@ describe('Server Inspection Tools Tests', () => { // Mock getSession to return null - session not found mockSessionManager.getSession.mockReturnValue(null); - let result; - try { - result = await callToolHandler({ - method: 'tools/call', - params: { - name: 'get_variables', - arguments: { - sessionId: 'test-session', - scope: 100 - } + // Session-lifecycle failures must surface as structured results, not thrown protocol errors + const result = await callToolHandler({ + method: 'tools/call', + params: { + name: 'get_variables', + arguments: { + sessionId: 'test-session', + scope: 100 } - }); - } catch (error) { - // If error is thrown, convert it to the expected format - result = { - content: [{ - type: 'text', - text: JSON.stringify({ - success: false, - error: error.message - }) - }] - }; - } + } + }); - // An unknown session makes validateSession throw McpError('Session not found'); the try/catch above - // normalizes that thrown error into the success:false shape asserted below. const content = JSON.parse(result.content[0].text); expect(content.success).toBe(false); expect(content.error).toContain('Session not found: test-session'); @@ -210,29 +195,15 @@ describe('Server Inspection Tools Tests', () => { it('should handle missing session', async () => { mockSessionManager.getSession.mockReturnValue(null); - let result; - try { - result = await callToolHandler({ - method: 'tools/call', - params: { - name: 'get_stack_trace', - arguments: { sessionId: 'non-existent' } - } - }); - } catch (error) { - // If error is thrown, convert it to the expected format - result = { - content: [{ - type: 'text', - text: JSON.stringify({ - success: false, - error: error.message - }) - }] - }; - } + // Session-lifecycle failures must surface as structured results, not thrown protocol errors + const result = await callToolHandler({ + method: 'tools/call', + params: { + name: 'get_stack_trace', + arguments: { sessionId: 'non-existent' } + } + }); - // The server now returns a success response with error message const content = JSON.parse(result.content[0].text); expect(content.success).toBe(false); expect(content.error).toContain('Session not found: non-existent'); diff --git a/tests/unit/server-coverage.test.ts b/tests/unit/server-coverage.test.ts index d7cc1e50..7a616c5d 100644 --- a/tests/unit/server-coverage.test.ts +++ b/tests/unit/server-coverage.test.ts @@ -612,11 +612,15 @@ describe('Server Coverage - Error Paths and Edge Cases', () => { }); }); - it('handlePause throws McpError when session validation fails', async () => { + it('handlePause returns success:false when session validation fails', async () => { mockSessionManager.pause = vi.fn().mockRejectedValue(new Error('some pause error')); - (server as any).validateSession = vi.fn().mockImplementation(() => { throw new McpError(McpErrorCode.InvalidParams, 'Session not found: test-session'); }); + mockSessionManager.getSession.mockReturnValue(null); + + const result = await (server as any).handlePause({ sessionId: 'test-session' }); + const payload = JSON.parse(result.content[0].text); - await expect((server as any).handlePause({ sessionId: 'test-session' })).rejects.toThrow('Session not found'); + expect(payload.success).toBe(false); + expect(payload.error).toContain('Session not found: test-session'); }); }); @@ -782,7 +786,7 @@ describe('Server Coverage - Error Paths and Edge Cases', () => { expect(payload.threads[0]).toEqual({ id: 1, name: 'main' }); }); - it('should re-throw McpError subclasses (SessionTerminatedError etc.)', async () => { + it('converts typed session errors (SessionTerminatedError etc.) into success:false results', async () => { mockSessionManager.getSession.mockReturnValue({ id: 'test-session', sessionLifecycle: SessionLifecycleState.ACTIVE @@ -790,8 +794,11 @@ describe('Server Coverage - Error Paths and Edge Cases', () => { const { SessionTerminatedError } = await import('../../src/errors/debug-errors'); mockSessionManager.listThreads = vi.fn().mockRejectedValue(new SessionTerminatedError('test-session')); - await expect((server as any).handleListThreads({ sessionId: 'test-session' })) - .rejects.toThrow(McpError); + const result = await (server as any).handleListThreads({ sessionId: 'test-session' }); + const payload = JSON.parse(result.content[0].text); + + expect(payload.success).toBe(false); + expect(payload.error).toContain('Session is terminated: test-session'); }); it('should throw McpError for unknown errors', async () => { @@ -866,26 +873,32 @@ describe('Server Coverage - Error Paths and Edge Cases', () => { expect(payload.state).toBe('paused'); }); - it('re-throws SessionTerminatedError (extends McpError)', async () => { + it('converts SessionTerminatedError into a success:false result', async () => { mockSessionManager.getSession.mockReturnValue({ id: 'test-session', sessionLifecycle: SessionLifecycleState.ACTIVE }); mockSessionManager.pause = vi.fn().mockRejectedValue(new SessionTerminatedError('test-session')); - await expect((server as any).handlePause({ sessionId: 'test-session' })) - .rejects.toThrow(McpError); + const result = await (server as any).handlePause({ sessionId: 'test-session' }); + const payload = JSON.parse(result.content[0].text); + + expect(payload.success).toBe(false); + expect(payload.error).toContain('Session is terminated: test-session'); }); - it('re-throws ProxyNotRunningError (extends McpError)', async () => { + it('converts ProxyNotRunningError into a success:false result', async () => { mockSessionManager.getSession.mockReturnValue({ id: 'test-session', sessionLifecycle: SessionLifecycleState.ACTIVE }); mockSessionManager.pause = vi.fn().mockRejectedValue(new ProxyNotRunningError('test-session')); - await expect((server as any).handlePause({ sessionId: 'test-session' })) - .rejects.toThrow(McpError); + const result = await (server as any).handlePause({ sessionId: 'test-session' }); + const payload = JSON.parse(result.content[0].text); + + expect(payload.success).toBe(false); + expect(payload.error).toContain('test-session'); }); it('wraps generic errors as McpError', async () => {