From 5620d290d653bd08fde7fe156ce79fd2f537acd5 Mon Sep 17 00:00:00 2001 From: Kashish Hora Date: Thu, 20 Nov 2025 19:36:12 +0100 Subject: [PATCH 1/3] refactor: wrap setRequestHandler for high-level servers --- src/modules/tracingV2.ts | 259 ++++++++++++----------- src/tests/context-parameters.test.ts | 67 +++++- src/tests/error-capture.test.ts | 256 +++++++++++++++++++--- src/tests/tracing-initialization.test.ts | 148 +++++++++++++ 4 files changed, 572 insertions(+), 158 deletions(-) create mode 100644 src/tests/tracing-initialization.test.ts diff --git a/src/modules/tracingV2.ts b/src/modules/tracingV2.ts index 309753b..5368d36 100644 --- a/src/modules/tracingV2.ts +++ b/src/modules/tracingV2.ts @@ -233,10 +233,9 @@ function addMCPcatToolsToServer(server: HighLevelMCPServerLike): void { function addTracingToToolCallback( tool: RegisteredTool, toolName: string, - server: HighLevelMCPServerLike, + _server: HighLevelMCPServerLike, ): RegisteredTool { const originalCallback = tool.callback; - const lowLevelServer = server.server as MCPServerLike; // Check if this callback has already been wrapped if (wrappedCallbacks.has(originalCallback)) { @@ -277,156 +276,168 @@ function addTracingToToolCallback( return args; }; - try { - const data = getServerTrackingData(lowLevelServer); - if (!data) { - writeToLog( - "Warning: MCPCat is unable to find server tracking data. Please ensure you have called track(server, options) before using tool calls.", - ); + // Remove context from args before calling original callback + // BUT keep it for get_more_tools since it's a required parameter + const cleanedArgs = + toolName === "get_more_tools" ? args : removeContextFromArgs(args); + + // Call original callback with cleaned args + if (cleanedArgs === undefined) { + return await ( + originalCallback as ( + extra: CompatibleRequestHandlerExtra, + ) => Promise + )(extra); + } else { + return await ( + originalCallback as ( + args: any, + extra: CompatibleRequestHandlerExtra, + ) => Promise + )(cleanedArgs, extra); + } + }; - // Remove context from args before calling original callback - // BUT keep it for get_more_tools since it's a required parameter - const cleanedArgs = - toolName === "get_more_tools" ? args : removeContextFromArgs(args); - - // Call with original params - return await (cleanedArgs === undefined - ? ( - originalCallback as ( - extra: CompatibleRequestHandlerExtra, - ) => Promise - )(extra) - : ( - originalCallback as ( - args: any, - extra: CompatibleRequestHandlerExtra, - ) => Promise - )(cleanedArgs, extra)); - } + // Mark the original callback as wrapped + wrappedCallbacks.set(originalCallback, true); - const sessionId = getServerSessionId(lowLevelServer, extra); + // Mark the wrapped callback as well (in case it gets re-wrapped) + wrappedCallbacks.set(wrappedCallback, true); - // Create a request-like object for compatibility with existing code - const request = { - params: { - name: toolName, - arguments: args, - }, - }; + // Create a new tool object with the wrapped callback + const wrappedTool = { + ...tool, + callback: wrappedCallback as RegisteredTool["callback"], + }; - let event: UnredactedEvent = { - sessionId: sessionId, - resourceName: toolName, - parameters: { - request: request, - extra: extra, - }, - eventType: PublishEventRequestEventTypeEnum.mcpToolsCall, - timestamp: new Date(), - redactionFn: data.options.redactSensitiveInformation, - }; + // Mark the tool as processed + (wrappedTool as any)[MCPCAT_PROCESSED] = true; - try { - // Try to identify the session if identify function is provided - await handleIdentify(lowLevelServer, data, request, extra); + return wrappedTool; +} + +function setupToolsCallHandlerWrapping(server: HighLevelMCPServerLike): void { + const lowLevelServer = server.server as MCPServerLike; + + // Check if tools/call handler already exists + const existingHandler = lowLevelServer._requestHandlers.get("tools/call"); + if (existingHandler) { + const wrappedHandler = createToolsCallWrapper( + existingHandler, + lowLevelServer, + ); + lowLevelServer._requestHandlers.set("tools/call", wrappedHandler); + } + + // Intercept future calls to setRequestHandler for tools registered after track() + const originalSetRequestHandler = + lowLevelServer.setRequestHandler.bind(lowLevelServer); + + lowLevelServer.setRequestHandler = function ( + requestSchema: any, + handler: any, + ) { + const method = requestSchema?.shape?.method?.value; + + // Only wrap tools/call handler + if (method === "tools/call") { + const wrappedHandler = createToolsCallWrapper(handler, lowLevelServer); + return originalSetRequestHandler(requestSchema, wrappedHandler); + } + + // Pass through all other handlers unchanged + return originalSetRequestHandler(requestSchema, handler); + } as any; +} - // Update event sessionId in case handleIdentify reconnected to a different session +function createToolsCallWrapper( + originalHandler: any, + server: MCPServerLike, +): any { + return async (request: any, extra: any) => { + const startTime = new Date(); + let shouldPublishEvent = false; + let event: UnredactedEvent | null = null; + + try { + const data = getServerTrackingData(server); + + if (!data) { + writeToLog( + "Warning: MCPCat is unable to find server tracking data. Please ensure you have called track(server, options) before using tool calls.", + ); + } else { + shouldPublishEvent = true; + + const sessionId = getServerSessionId(server, extra); + + event = { + sessionId, + resourceName: request.params?.name || "Unknown Tool", + parameters: { request, extra }, + eventType: PublishEventRequestEventTypeEnum.mcpToolsCall, + timestamp: startTime, + redactionFn: data.options.redactSensitiveInformation, + }; + + // Identify user session + await handleIdentify(server, data, request, extra); event.sessionId = data.sessionId; - // Extract context for userIntent if present - if (args && typeof args === "object" && "context" in args) { - event.userIntent = args.context; + // Extract context for userIntent + if ( + data.options.enableToolCallContext && + request.params?.arguments?.context + ) { + event.userIntent = request.params.arguments.context; } + } + } catch (error) { + // If tracing setup fails, log it but continue with tool execution + writeToLog( + `Warning: MCPCat tracing failed for tool ${request.params?.name}, falling back to original handler - ${error}`, + ); + } - // Remove context from args before calling original callback - // BUT keep it for get_more_tools since it's a required parameter - const cleanedArgs = - toolName === "get_more_tools" ? args : removeContextFromArgs(args); - - let result = await (cleanedArgs === undefined - ? ( - originalCallback as ( - extra: CompatibleRequestHandlerExtra, - ) => Promise - )(extra) - : ( - originalCallback as ( - args: any, - extra: CompatibleRequestHandlerExtra, - ) => Promise - )(cleanedArgs, extra)); - - // Check if the result indicates an error + // Execute the tool (this should always happen, even if tracing setup failed) + try { + const result = await originalHandler(request, extra); + + if (event && shouldPublishEvent) { + // Check for execution errors (SDK converts them to CallToolResult) if (isToolResultError(result)) { event.isError = true; event.error = captureException(result); } event.response = result; - event.duration = - (event.timestamp && - new Date().getTime() - event.timestamp.getTime()) || - undefined; - publishEvent(lowLevelServer, event); - return result; - } catch (error) { + event.duration = new Date().getTime() - startTime.getTime(); + publishEvent(server, event); + } + + return result; + } catch (error) { + // Validation errors, unknown tool, disabled tool + if (event && shouldPublishEvent) { event.isError = true; event.error = captureException(error); - event.duration = - (event.timestamp && - new Date().getTime() - event.timestamp.getTime()) || - undefined; - publishEvent(lowLevelServer, event); - throw error; + event.duration = new Date().getTime() - startTime.getTime(); + publishEvent(server, event); } - } catch (error) { - // If any error occurs in our tracing code, log it and call the original callback - writeToLog( - `Warning: MCPCat tracing failed for tool ${toolName}, falling back to original callback - ${error}`, - ); - // Remove context from args before calling original callback - // BUT keep it for get_more_tools since it's a required parameter - const cleanedArgs = - toolName === "get_more_tools" ? args : removeContextFromArgs(args); - - return await (cleanedArgs === undefined - ? ( - originalCallback as ( - extra: CompatibleRequestHandlerExtra, - ) => Promise - )(extra) - : ( - originalCallback as ( - args: any, - extra: CompatibleRequestHandlerExtra, - ) => Promise - )(cleanedArgs, extra)); + // Re-throw so Protocol converts to JSONRPC error response + throw error; } }; - - // Mark the original callback as wrapped - wrappedCallbacks.set(originalCallback, true); - - // Mark the wrapped callback as well (in case it gets re-wrapped) - wrappedCallbacks.set(wrappedCallback, true); - - // Create a new tool object with the wrapped callback - const wrappedTool = { - ...tool, - callback: wrappedCallback as RegisteredTool["callback"], - }; - - // Mark the tool as processed - (wrappedTool as any)[MCPCAT_PROCESSED] = true; - - return wrappedTool; } export function setupTracking(server: HighLevelMCPServerLike): void { try { const mcpcatData = getServerTrackingData(server.server); + // Setup handler wrapping before any tools are registered + setupToolsCallHandlerWrapping(server); + setupInitializeTracing(server); // Modify existing tools to include context parameters in their inputSchemas if (mcpcatData?.options.enableToolCallContext) { diff --git a/src/tests/context-parameters.test.ts b/src/tests/context-parameters.test.ts index 27d275b..cf7d423 100644 --- a/src/tests/context-parameters.test.ts +++ b/src/tests/context-parameters.test.ts @@ -315,7 +315,8 @@ describe("Context Parameters", () => { const listEvent = events.find( (e) => e.eventType === PublishEventRequestEventTypeEnum.mcpToolsCall && - e.resourceName === "list_todos", + e.resourceName === "list_todos" && + !e.isError, // Find the successful event, not the validation errors ); expect(listEvent).toBeDefined(); @@ -396,5 +397,69 @@ describe("Context Parameters", () => { ); }); }); + + it("should remove context parameter before calling tool callback", async () => { + // Variable to capture what arguments the tool callback actually receives + let capturedCallbackArguments: any = null; + + // Register a test tool that captures its arguments + const { z } = await import("zod"); + server.tool( + "test_context_removal", + "Test tool that captures callback arguments", + { + testParam: z.string().describe("A test parameter"), + }, + async (args: any) => { + // Capture exactly what arguments this callback receives + capturedCallbackArguments = { ...args }; + return { + content: [ + { + type: "text", + text: "Arguments captured", + }, + ], + }; + }, + ); + + // Enable tracking with context parameters + await track(server, { + projectId: "test-project", + enableTracing: true, + }); + + // Call the test tool WITH context parameter + const result = await client.request( + { + method: "tools/call", + params: { + name: "test_context_removal", + arguments: { + testParam: "test-value", + context: "This context should be removed before callback", + }, + }, + }, + CallToolResultSchema, + ); + + // Wait for processing + await new Promise((resolve) => setTimeout(resolve, 100)); + + // The tool call should succeed (successful calls have undefined isError) + expect(result).toBeDefined(); + expect(result.isError).not.toBe(true); + + // Verify that the callback received the testParam + expect(capturedCallbackArguments).not.toBeNull(); + expect(capturedCallbackArguments).toHaveProperty("testParam"); + expect(capturedCallbackArguments.testParam).toBe("test-value"); + + // This is the key assertion: context should NOT be in the arguments + // that the tool callback received (it should have been removed by the wrapper) + expect(capturedCallbackArguments).not.toHaveProperty("context"); + }); }); }); diff --git a/src/tests/error-capture.test.ts b/src/tests/error-capture.test.ts index c2b14de..f08f9e4 100644 --- a/src/tests/error-capture.test.ts +++ b/src/tests/error-capture.test.ts @@ -7,6 +7,7 @@ import { track } from "../index.js"; import { CallToolResultSchema } from "@modelcontextprotocol/sdk/types"; import { EventCapture } from "./test-utils.js"; import { PublishEventRequestEventTypeEnum } from "mcpcat-api"; +import { z } from "zod"; describe("Error Capture Integration Tests", () => { let eventCapture: EventCapture; @@ -63,24 +64,11 @@ describe("Error Capture Integration Tests", () => { // Verify error structure expect(errorEvent!.error).toBeDefined(); expect(errorEvent!.error!.message).toContain("not found"); - expect(errorEvent!.error!.type).toBe("Error"); - - // Verify stack trace is captured - expect(errorEvent!.error!.stack).toBeDefined(); - expect(typeof errorEvent!.error!.stack).toBe("string"); - expect(errorEvent!.error!.stack!.length).toBeGreaterThan(0); - - // Verify stack frames are parsed - expect(errorEvent!.error!.frames).toBeDefined(); - expect(Array.isArray(errorEvent!.error!.frames)).toBe(true); - expect(errorEvent!.error!.frames!.length).toBeGreaterThan(0); + // SDK converts execution errors to CallToolResult, losing Error type + expect(errorEvent!.error!.type).toBe("NonError"); - // Verify frame structure - const firstFrame = errorEvent!.error!.frames![0]; - expect(firstFrame).toHaveProperty("filename"); - expect(firstFrame).toHaveProperty("function"); - expect(firstFrame).toHaveProperty("in_app"); - expect(typeof firstFrame.in_app).toBe("boolean"); + // Execution errors don't preserve stack traces (SDK limitation) + // Only validation errors preserve full Error objects } finally { await cleanup(); } @@ -135,21 +123,12 @@ describe("Error Capture Integration Tests", () => { const errorEvent = events.find((e) => e.isError); expect(errorEvent).toBeDefined(); - // Verify main error - expect(errorEvent!.error!.message).toBe("Wrapper error"); - expect(errorEvent!.error!.type).toBe("Error"); - - // Verify cause chain is captured - expect(errorEvent!.error!.chained_errors).toBeDefined(); - expect(errorEvent!.error!.chained_errors!.length).toBe(1); - expect(errorEvent!.error!.chained_errors![0].message).toBe( - "Root cause error", - ); - expect(errorEvent!.error!.chained_errors![0].type).toBe("Error"); + // Execution errors are converted to CallToolResult by SDK + expect(errorEvent!.error!.type).toBe("NonError"); + expect(errorEvent!.error!.message).toContain("Wrapper error"); - // Verify cause has its own stack trace - expect(errorEvent!.error!.chained_errors![0].stack).toBeDefined(); - expect(errorEvent!.error!.chained_errors![0].frames).toBeDefined(); + // Error.cause chains are lost when SDK converts to CallToolResult + // This is an SDK limitation for execution errors } finally { await cleanup(); } @@ -199,7 +178,8 @@ describe("Error Capture Integration Tests", () => { const errorEvent = events.find((e) => e.isError); expect(errorEvent).toBeDefined(); - expect(errorEvent!.error!.type).toBe("TypeError"); + // Execution errors lose their specific type when SDK converts to CallToolResult + expect(errorEvent!.error!.type).toBe("NonError"); expect(errorEvent!.error!.message).toContain("null"); } finally { await cleanup(); @@ -247,7 +227,7 @@ describe("Error Capture Integration Tests", () => { expect(errorEvent).toBeDefined(); expect(errorEvent!.error!.type).toBe("NonError"); - expect(errorEvent!.error!.message).toBe("This is a string error"); + expect(errorEvent!.error!.message).toContain("This is a string error"); // Non-Error objects don't have stack traces expect(errorEvent!.error!.stack).toBeUndefined(); expect(errorEvent!.error!.frames).toBeUndefined(); @@ -429,4 +409,214 @@ describe("Error Capture Integration Tests", () => { await cleanup(); } }); + + it("should capture validation errors for invalid enum values", async () => { + const { server, client, cleanup } = await setupTestServerAndClient(); + + try { + // Add a tool with enum validation + server.tool( + "calculate", + "Perform calculations", + { + operation: z.enum(["add", "subtract", "multiply", "divide"]), + a: z.number(), + b: z.number(), + }, + async (args) => { + return { + content: [{ type: "text", text: `Result: ${args.a}` }], + }; + }, + ); + + // Track the server + await track(server, { + projectId: "test-project", + enableTracing: true, + }); + + // Call with invalid enum value - should throw before callback executes + try { + await client.request( + { + method: "tools/call", + params: { + name: "calculate", + arguments: { + operation: "modulo", // Invalid enum value + a: 10, + b: 3, + context: "Testing validation error capture", + }, + }, + }, + CallToolResultSchema, + ); + expect.fail("Should have thrown validation error"); + } catch (error: any) { + // MCP SDK throws validation errors, doesn't return CallToolResult + expect(error).toBeDefined(); + } + + // Wait for event + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Find the tool call event + const events = eventCapture.findEventsByResourceName("calculate"); + expect(events.length).toBeGreaterThan(0); + + const errorEvent = events.find((e) => e.isError); + expect(errorEvent).toBeDefined(); + + // Verify error captured with full Error object + expect(errorEvent!.error).toBeDefined(); + expect(errorEvent!.error!.message).toContain("Invalid"); + expect(errorEvent!.error!.type).toBe("McpError"); + + // Verify stack trace is captured + expect(errorEvent!.error!.stack).toBeDefined(); + expect(errorEvent!.error!.stack!.length).toBeGreaterThan(0); + + // Verify stack frames parsed + expect(errorEvent!.error!.frames).toBeDefined(); + expect(errorEvent!.error!.frames!.length).toBeGreaterThan(0); + } finally { + await cleanup(); + } + }); + + it("should capture errors for unknown tool names", async () => { + const { server, client, cleanup } = await setupTestServerAndClient(); + + try { + // Track the server + await track(server, { + projectId: "test-project", + enableTracing: true, + }); + + // Call non-existent tool + try { + await client.request( + { + method: "tools/call", + params: { + name: "nonexistent_tool", + arguments: { + context: "Testing unknown tool error", + }, + }, + }, + CallToolResultSchema, + ); + expect.fail("Should have thrown unknown tool error"); + } catch (error: any) { + // SDK throws error for unknown tools + expect(error).toBeDefined(); + } + + // Wait for event + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Find the tool call event + const events = eventCapture.findEventsByResourceName("nonexistent_tool"); + expect(events.length).toBeGreaterThan(0); + + const errorEvent = events.find((e) => e.isError); + expect(errorEvent).toBeDefined(); + + // Verify error captured + expect(errorEvent!.error!.message).toContain("not found"); + expect(errorEvent!.error!.type).toBe("McpError"); + + // Verify stack trace + expect(errorEvent!.error!.stack).toBeDefined(); + expect(errorEvent!.error!.frames).toBeDefined(); + } finally { + await cleanup(); + } + }); + + it("should capture validation errors for missing required parameters", async () => { + const { server, client, cleanup } = await setupTestServerAndClient(); + + try { + // add_todo requires 'text' parameter + await track(server, { + projectId: "test-project", + enableTracing: true, + }); + + // Call without required parameter + try { + await client.request( + { + method: "tools/call", + params: { + name: "add_todo", + arguments: { + context: "Testing missing parameter", + // 'text' parameter is missing + }, + }, + }, + CallToolResultSchema, + ); + expect.fail("Should have thrown validation error"); + } catch (error: any) { + expect(error).toBeDefined(); + } + + // Wait for event + await new Promise((resolve) => setTimeout(resolve, 100)); + + const events = eventCapture.findEventsByResourceName("add_todo"); + const errorEvent = events.find((e) => e.isError); + + expect(errorEvent).toBeDefined(); + expect(errorEvent!.error!.message).toContain("Invalid"); + expect(errorEvent!.error!.type).toBe("McpError"); + expect(errorEvent!.error!.stack).toBeDefined(); + } finally { + await cleanup(); + } + }); + + it("should publish exactly one event per tool call", async () => { + const { server, client, cleanup } = await setupTestServerAndClient(); + + try { + await track(server, { + projectId: "test-project", + enableTracing: true, + }); + + // Make a successful tool call + await client.request( + { + method: "tools/call", + params: { + name: "list_todos", + arguments: { + context: "Testing single event publishing", + }, + }, + }, + CallToolResultSchema, + ); + + // Wait for event + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify exactly one event for this call + const events = eventCapture.findEventsByResourceName("list_todos"); + const successEvents = events.filter((e) => !e.isError); + + // Should be exactly 1 event, not 2 (which would indicate double publishing) + expect(successEvents.length).toBe(1); + } finally { + await cleanup(); + } + }); }); diff --git a/src/tests/tracing-initialization.test.ts b/src/tests/tracing-initialization.test.ts new file mode 100644 index 0000000..7e2b769 --- /dev/null +++ b/src/tests/tracing-initialization.test.ts @@ -0,0 +1,148 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { + setupTestServerAndClient, + resetTodos, +} from "./test-utils/client-server-factory.js"; +import { track } from "../index.js"; +import { CallToolResultSchema } from "@modelcontextprotocol/sdk/types.js"; +import { EventCapture } from "./test-utils.js"; + +describe("Tracing Initialization Tests", () => { + let eventCapture: EventCapture; + + beforeEach(async () => { + resetTodos(); + eventCapture = new EventCapture(); + await eventCapture.start(); + }); + + afterEach(async () => { + await eventCapture.stop(); + }); + + it("should not create duplicate events when track() is called multiple times", async () => { + const { server, client, cleanup } = await setupTestServerAndClient(); + + try { + // Call track() multiple times on the same server instance + await track(server, { + projectId: "test-project", + enableTracing: true, + }); + + await track(server, { + projectId: "test-project", + enableTracing: true, + }); + + await track(server, { + projectId: "test-project", + enableTracing: true, + }); + + // First add a todo so we can complete it + await client.request( + { + method: "tools/call", + params: { + name: "add_todo", + arguments: { + text: "Test todo for double-wrapping", + context: "Setup for double-wrapping test", + }, + }, + }, + CallToolResultSchema, + ); + + // Wait for event to be published + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Clear events from setup + eventCapture.clear(); + + // Execute the actual test tool call + const result = await client.request( + { + method: "tools/call", + params: { + name: "complete_todo", + arguments: { + id: "1", + context: "Testing double-wrapping protection", + }, + }, + }, + CallToolResultSchema, + ); + + // Wait for any events to be published + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify the tool call succeeded (successful calls have undefined isError) + expect(result).toBeDefined(); + expect(result.isError).not.toBe(true); + + // Get all events published + const events = eventCapture.getEvents(); + + // Should have exactly 1 event, not 3 (one per track() call) + expect(events.length).toBe(1); + expect(events[0].resourceName).toBe("complete_todo"); + expect(events[0].isError).toBeUndefined(); + } finally { + await cleanup(); + } + }); + + it("should publish events for successful tool calls with handler-level architecture", async () => { + const { server, client, cleanup } = await setupTestServerAndClient(); + + try { + // Initialize tracing with track() + await track(server, { + projectId: "test-project", + enableTracing: true, + }); + + // Execute a successful tool call + const result = await client.request( + { + method: "tools/call", + params: { + name: "add_todo", + arguments: { + text: "Test successful handler wrapping", + context: "Testing handler-level event publishing", + }, + }, + }, + CallToolResultSchema, + ); + + // Wait for event to be published + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify tool call succeeded (successful calls have undefined isError) + expect(result).toBeDefined(); + expect(result.isError).not.toBe(true); + + // Get events + const events = eventCapture.getEvents(); + + // Should have exactly 1 event for the successful call + expect(events.length).toBe(1); + expect(events[0].resourceName).toBe("add_todo"); + expect(events[0].isError).toBeUndefined(); + expect(events[0].userIntent).toBe( + "Testing handler-level event publishing", + ); + + // Verify event has the expected structure + expect(events[0]).toHaveProperty("eventType"); + expect(events[0]).toHaveProperty("timestamp"); + } finally { + await cleanup(); + } + }); +}); From 4fd4baaf8ff94ccd17bbe6e5ee977a9333e002c8 Mon Sep 17 00:00:00 2001 From: Kashish Hora Date: Thu, 20 Nov 2025 21:38:36 +0100 Subject: [PATCH 2/3] catch error in tool execution and put it in extra param --- src/modules/exceptions.ts | 62 +++++++++++++++++++++++- src/modules/tracingV2.ts | 51 +++++++++++++------ src/tests/error-capture.test.ts | 86 +++++++++++++++++++++++---------- 3 files changed, 158 insertions(+), 41 deletions(-) diff --git a/src/modules/exceptions.ts b/src/modules/exceptions.ts index a8146be..b15361d 100644 --- a/src/modules/exceptions.ts +++ b/src/modules/exceptions.ts @@ -15,9 +15,18 @@ const MAX_STACK_FRAMES = 50; * detects whether each frame is user code (in_app: true) or library code (in_app: false). * * @param error - The error to capture (can be Error, string, object, or any value) + * @param contextStack - Optional Error object to use for stack context (for validation errors) * @returns ErrorData object with structured error information */ -export function captureException(error: unknown): ErrorData { +export function captureException( + error: unknown, + contextStack?: Error, +): ErrorData { + // Handle CallToolResult objects (SDK 1.21.0+ converts errors to these) + if (isCallToolResult(error)) { + return captureCallToolResultError(error, contextStack); + } + // Handle non-Error objects if (!(error instanceof Error)) { return { @@ -708,6 +717,57 @@ function unwrapErrorCauses(error: Error): ChainedErrorData[] { return chainedErrors; } +/** + * Detects if a value is a CallToolResult object (SDK 1.21.0+ error format). + * + * SDK 1.21.0+ converts errors to CallToolResult format: + * { content: [{ type: "text", text: "error message" }], isError: true } + * + * @param value - Value to check + * @returns True if value is a CallToolResult object + */ +function isCallToolResult(value: unknown): boolean { + return ( + value !== null && + typeof value === "object" && + "isError" in value && + "content" in value && + Array.isArray((value as any).content) + ); +} + +/** + * Extracts error information from CallToolResult objects. + * + * SDK 1.21.0+ converts errors to CallToolResult, losing original stack traces. + * This extracts the error message from the content array. + * + * @param result - CallToolResult object with error + * @param _contextStack - Optional Error object for stack context (unused, kept for compatibility) + * @returns ErrorData with extracted message (no stack trace) + */ +function captureCallToolResultError( + result: any, + _contextStack?: Error, +): ErrorData { + // Extract message from content array + const message = + result.content + ?.filter((c: any) => c.type === "text") + .map((c: any) => c.text) + .join(" ") + .trim() || "Unknown error"; + + const errorData: ErrorData = { + message, + type: "NonError", // Can't determine actual type from CallToolResult + platform: "javascript", + // No stack or frames - SDK stripped the original error information + }; + + return errorData; +} + /** * Converts non-Error objects to string representation for error messages. * diff --git a/src/modules/tracingV2.ts b/src/modules/tracingV2.ts index 5368d36..ef15bf3 100644 --- a/src/modules/tracingV2.ts +++ b/src/modules/tracingV2.ts @@ -281,20 +281,30 @@ function addTracingToToolCallback( const cleanedArgs = toolName === "get_more_tools" ? args : removeContextFromArgs(args); - // Call original callback with cleaned args - if (cleanedArgs === undefined) { - return await ( - originalCallback as ( - extra: CompatibleRequestHandlerExtra, - ) => Promise - )(extra); - } else { - return await ( - originalCallback as ( - args: any, - extra: CompatibleRequestHandlerExtra, - ) => Promise - )(cleanedArgs, extra); + // Call original callback with cleaned args, capturing any errors + try { + if (cleanedArgs === undefined) { + return await ( + originalCallback as ( + extra: CompatibleRequestHandlerExtra, + ) => Promise + )(extra); + } else { + return await ( + originalCallback as ( + args: any, + extra: CompatibleRequestHandlerExtra, + ) => Promise + )(cleanedArgs, extra); + } + } catch (error) { + // Store original error for handler to use + if (error instanceof Error) { + (extra as any).__mcpcat_error = error; + } + + // Re-throw so SDK can process it normally + throw error; } }; @@ -407,7 +417,18 @@ function createToolsCallWrapper( // Check for execution errors (SDK converts them to CallToolResult) if (isToolResultError(result)) { event.isError = true; - event.error = captureException(result); + + // Check if callback captured the original error (has full stack) + const capturedError = (extra as any).__mcpcat_error; + + if (capturedError) { + // Use captured error from callback + event.error = captureException(capturedError); + delete (extra as any).__mcpcat_error; // Cleanup + } else { + // SDK 1.21.0+ converted error (no stack trace available) + event.error = captureException(result); + } } event.response = result; diff --git a/src/tests/error-capture.test.ts b/src/tests/error-capture.test.ts index f08f9e4..2b20575 100644 --- a/src/tests/error-capture.test.ts +++ b/src/tests/error-capture.test.ts @@ -64,11 +64,12 @@ describe("Error Capture Integration Tests", () => { // Verify error structure expect(errorEvent!.error).toBeDefined(); expect(errorEvent!.error!.message).toContain("not found"); - // SDK converts execution errors to CallToolResult, losing Error type - expect(errorEvent!.error!.type).toBe("NonError"); - // Execution errors don't preserve stack traces (SDK limitation) - // Only validation errors preserve full Error objects + // Make sure execution error is properly recognized as an error + expect(errorEvent!.error!.type).toBe("Error"); + expect(errorEvent!.error!.stack).toBeDefined(); + expect(errorEvent!.error!.frames).toBeDefined(); + expect(errorEvent!.error!.frames!.length).toBeGreaterThan(0); } finally { await cleanup(); } @@ -123,12 +124,20 @@ describe("Error Capture Integration Tests", () => { const errorEvent = events.find((e) => e.isError); expect(errorEvent).toBeDefined(); - // Execution errors are converted to CallToolResult by SDK - expect(errorEvent!.error!.type).toBe("NonError"); + // Ensure we get the real Error type + expect(errorEvent!.error!.type).toBe("Error"); expect(errorEvent!.error!.message).toContain("Wrapper error"); - // Error.cause chains are lost when SDK converts to CallToolResult - // This is an SDK limitation for execution errors + // Verify we captured the full error with stack trace + expect(errorEvent!.error!.stack).toBeDefined(); + expect(errorEvent!.error!.frames).toBeDefined(); + + // Error.cause chains should be captured + expect(errorEvent!.error!.chained_errors).toBeDefined(); + expect(errorEvent!.error!.chained_errors!.length).toBe(1); + expect(errorEvent!.error!.chained_errors![0].message).toBe( + "Root cause error", + ); } finally { await cleanup(); } @@ -178,9 +187,11 @@ describe("Error Capture Integration Tests", () => { const errorEvent = events.find((e) => e.isError); expect(errorEvent).toBeDefined(); - // Execution errors lose their specific type when SDK converts to CallToolResult - expect(errorEvent!.error!.type).toBe("NonError"); + // With callback-level capture, we preserve the specific error type + expect(errorEvent!.error!.type).toBe("TypeError"); expect(errorEvent!.error!.message).toContain("null"); + expect(errorEvent!.error!.stack).toBeDefined(); + expect(errorEvent!.error!.frames).toBeDefined(); } finally { await cleanup(); } @@ -228,7 +239,8 @@ describe("Error Capture Integration Tests", () => { expect(errorEvent).toBeDefined(); expect(errorEvent!.error!.type).toBe("NonError"); expect(errorEvent!.error!.message).toContain("This is a string error"); - // Non-Error objects don't have stack traces + // Non-Error throws don't have stack traces + // (SDK converts them, we can't capture at callback level) expect(errorEvent!.error!.stack).toBeUndefined(); expect(errorEvent!.error!.frames).toBeUndefined(); } finally { @@ -469,18 +481,26 @@ describe("Error Capture Integration Tests", () => { const errorEvent = events.find((e) => e.isError); expect(errorEvent).toBeDefined(); - // Verify error captured with full Error object + // Verify error captured expect(errorEvent!.error).toBeDefined(); expect(errorEvent!.error!.message).toContain("Invalid"); - expect(errorEvent!.error!.type).toBe("McpError"); - // Verify stack trace is captured - expect(errorEvent!.error!.stack).toBeDefined(); - expect(errorEvent!.error!.stack!.length).toBeGreaterThan(0); + // Type can vary by SDK version + // SDK 1.11.5: "McpError", SDK 1.21.0+: "NonError" + expect(["McpError", "NonError", "Error"]).toContain( + errorEvent!.error!.type, + ); - // Verify stack frames parsed - expect(errorEvent!.error!.frames).toBeDefined(); - expect(errorEvent!.error!.frames!.length).toBeGreaterThan(0); + // Stack trace may be present (older SDK) or not (newer SDK) + // Don't require it but verify format if present + if (errorEvent!.error!.stack) { + expect(errorEvent!.error!.stack!.length).toBeGreaterThan(0); + } + + // Frames may be present + if (errorEvent!.error!.frames) { + expect(errorEvent!.error!.frames!.length).toBeGreaterThan(0); + } } finally { await cleanup(); } @@ -528,11 +548,19 @@ describe("Error Capture Integration Tests", () => { // Verify error captured expect(errorEvent!.error!.message).toContain("not found"); - expect(errorEvent!.error!.type).toBe("McpError"); - // Verify stack trace - expect(errorEvent!.error!.stack).toBeDefined(); - expect(errorEvent!.error!.frames).toBeDefined(); + // Type can vary by SDK version + expect(["McpError", "NonError", "Error"]).toContain( + errorEvent!.error!.type, + ); + + // Stack trace may be present (verify if present) + if (errorEvent!.error!.stack) { + expect(errorEvent!.error!.stack!.length).toBeGreaterThan(0); + } + if (errorEvent!.error!.frames) { + expect(errorEvent!.error!.frames!.length).toBeGreaterThan(0); + } } finally { await cleanup(); } @@ -576,8 +604,16 @@ describe("Error Capture Integration Tests", () => { expect(errorEvent).toBeDefined(); expect(errorEvent!.error!.message).toContain("Invalid"); - expect(errorEvent!.error!.type).toBe("McpError"); - expect(errorEvent!.error!.stack).toBeDefined(); + + // Type can vary by SDK version + expect(["McpError", "NonError", "Error"]).toContain( + errorEvent!.error!.type, + ); + + // Stack trace may be present (verify if present) + if (errorEvent!.error!.stack) { + expect(errorEvent!.error!.stack!.length).toBeGreaterThan(0); + } } finally { await cleanup(); } From 191446ffef30a1b69fcbd5b1477538a9dde73588 Mon Sep 17 00:00:00 2001 From: Kashish Hora Date: Thu, 20 Nov 2025 21:53:14 +0100 Subject: [PATCH 3/3] update NonError -> UnknownErrorType to be more clear --- src/modules/exceptions.ts | 6 +++--- src/tests/error-capture.test.ts | 10 +++++----- src/tests/exceptions.test.ts | 16 ++++++++-------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/modules/exceptions.ts b/src/modules/exceptions.ts index b15361d..b9184ca 100644 --- a/src/modules/exceptions.ts +++ b/src/modules/exceptions.ts @@ -31,7 +31,7 @@ export function captureException( if (!(error instanceof Error)) { return { message: stringifyNonError(error), - type: "NonError", + type: "UnknownErrorType", platform: "javascript", }; } @@ -686,7 +686,7 @@ function unwrapErrorCauses(error: Error): ChainedErrorData[] { if (!(currentError instanceof Error)) { chainedErrors.push({ message: stringifyNonError(currentError), - type: "NonError", + type: "UnknownErrorType", }); break; } @@ -760,7 +760,7 @@ function captureCallToolResultError( const errorData: ErrorData = { message, - type: "NonError", // Can't determine actual type from CallToolResult + type: "UnknownErrorType", // Can't determine actual type from CallToolResult platform: "javascript", // No stack or frames - SDK stripped the original error information }; diff --git a/src/tests/error-capture.test.ts b/src/tests/error-capture.test.ts index 2b20575..2f5ee85 100644 --- a/src/tests/error-capture.test.ts +++ b/src/tests/error-capture.test.ts @@ -237,7 +237,7 @@ describe("Error Capture Integration Tests", () => { const errorEvent = events.find((e) => e.isError); expect(errorEvent).toBeDefined(); - expect(errorEvent!.error!.type).toBe("NonError"); + expect(errorEvent!.error!.type).toBe("UnknownErrorType"); expect(errorEvent!.error!.message).toContain("This is a string error"); // Non-Error throws don't have stack traces // (SDK converts them, we can't capture at callback level) @@ -486,8 +486,8 @@ describe("Error Capture Integration Tests", () => { expect(errorEvent!.error!.message).toContain("Invalid"); // Type can vary by SDK version - // SDK 1.11.5: "McpError", SDK 1.21.0+: "NonError" - expect(["McpError", "NonError", "Error"]).toContain( + // SDK 1.11.5: "McpError", SDK 1.21.0+: "UnknownErrorType" + expect(["McpError", "UnknownErrorType", "Error"]).toContain( errorEvent!.error!.type, ); @@ -550,7 +550,7 @@ describe("Error Capture Integration Tests", () => { expect(errorEvent!.error!.message).toContain("not found"); // Type can vary by SDK version - expect(["McpError", "NonError", "Error"]).toContain( + expect(["McpError", "UnknownErrorType", "Error"]).toContain( errorEvent!.error!.type, ); @@ -606,7 +606,7 @@ describe("Error Capture Integration Tests", () => { expect(errorEvent!.error!.message).toContain("Invalid"); // Type can vary by SDK version - expect(["McpError", "NonError", "Error"]).toContain( + expect(["McpError", "UnknownErrorType", "Error"]).toContain( errorEvent!.error!.type, ); diff --git a/src/tests/exceptions.test.ts b/src/tests/exceptions.test.ts index a47a82c..81037ac 100644 --- a/src/tests/exceptions.test.ts +++ b/src/tests/exceptions.test.ts @@ -272,7 +272,7 @@ describe("captureException", () => { expect(result.chained_errors).toBeDefined(); expect(result.chained_errors!.length).toBe(1); expect(result.chained_errors![0].message).toBe("string cause"); - expect(result.chained_errors![0].type).toBe("NonError"); + expect(result.chained_errors![0].type).toBe("UnknownErrorType"); }); it("should detect circular cause references", () => { @@ -306,7 +306,7 @@ describe("captureException", () => { const result = captureException("string error"); expect(result.message).toBe("string error"); - expect(result.type).toBe("NonError"); + expect(result.type).toBe("UnknownErrorType"); expect(result.stack).toBeUndefined(); expect(result.frames).toBeUndefined(); }); @@ -315,35 +315,35 @@ describe("captureException", () => { const result = captureException(42); expect(result.message).toBe("42"); - expect(result.type).toBe("NonError"); + expect(result.type).toBe("UnknownErrorType"); }); it("should handle boolean errors", () => { const result = captureException(false); expect(result.message).toBe("false"); - expect(result.type).toBe("NonError"); + expect(result.type).toBe("UnknownErrorType"); }); it("should handle null", () => { const result = captureException(null); expect(result.message).toBe("null"); - expect(result.type).toBe("NonError"); + expect(result.type).toBe("UnknownErrorType"); }); it("should handle undefined", () => { const result = captureException(undefined); expect(result.message).toBe("undefined"); - expect(result.type).toBe("NonError"); + expect(result.type).toBe("UnknownErrorType"); }); it("should handle object errors", () => { const result = captureException({ code: 404, message: "Not found" }); expect(result.message).toBe('{"code":404,"message":"Not found"}'); - expect(result.type).toBe("NonError"); + expect(result.type).toBe("UnknownErrorType"); }); it("should handle objects with circular references", () => { @@ -352,7 +352,7 @@ describe("captureException", () => { const result = captureException(obj); - expect(result.type).toBe("NonError"); + expect(result.type).toBe("UnknownErrorType"); // Should not throw, should return some string representation expect(typeof result.message).toBe("string"); });