From d73fd60d428dc056bef985adbc7c71cb1c930e06 Mon Sep 17 00:00:00 2001 From: Kashish Hora Date: Wed, 19 Nov 2025 19:58:34 +0100 Subject: [PATCH 1/4] fix: add platform field and context_line --- src/modules/exceptions.ts | 38 +++++ src/tests/exceptions.test.ts | 61 +++++++ src/tests/protocol-validation-errors.test.ts | 170 +++++++++++++++++++ src/types.ts | 2 + 4 files changed, 271 insertions(+) create mode 100644 src/tests/protocol-validation-errors.test.ts diff --git a/src/modules/exceptions.ts b/src/modules/exceptions.ts index 97e639b..a8146be 100644 --- a/src/modules/exceptions.ts +++ b/src/modules/exceptions.ts @@ -1,4 +1,5 @@ import { ErrorData, StackFrame, ChainedErrorData } from "../types.js"; +import { readFileSync } from "fs"; // Maximum number of exceptions to capture in a cause chain const MAX_EXCEPTION_CHAIN_DEPTH = 10; @@ -22,12 +23,14 @@ export function captureException(error: unknown): ErrorData { return { message: stringifyNonError(error), type: "NonError", + platform: "javascript", }; } const errorData: ErrorData = { message: error.message || "", type: error.name || error.constructor?.name || "Error", + platform: "javascript", }; // Capture stack trace if available @@ -76,6 +79,7 @@ function parseV8StackTrace(stackTrace: string): StackFrame[] { const frame = parseV8StackFrame(line.trim()); if (frame) { + addContextToFrame(frame); frames.push(frame); } @@ -88,6 +92,40 @@ function parseV8StackTrace(stackTrace: string): StackFrame[] { return frames; } +/** + * Adds context_line to a stack frame by reading the source file. + * + * This function extracts the line of code where the error occurred by: + * 1. Reading the source file using abs_path + * 2. Extracting the line at the specified line number + * 3. Setting the context_line field on the frame + * + * Only extracts context for user code (in_app: true) + * If the file cannot be read or the line number is invalid, context_line remains undefined. + * + * @param frame - The StackFrame to add context to (modified in place) + * @returns The modified StackFrame + */ +function addContextToFrame(frame: StackFrame): StackFrame { + if (!frame.in_app || !frame.abs_path || !frame.lineno) { + return frame; + } + + try { + const source = readFileSync(frame.abs_path, "utf8"); + const lines = source.split("\n"); + const lineIndex = frame.lineno - 1; // Convert to 0-based index + + if (lineIndex >= 0 && lineIndex < lines.length) { + frame.context_line = lines[lineIndex]; + } + } catch { + // File not found or not readable - silently skip + } + + return frame; +} + /** * Parses a location string from a V8 stack frame. * diff --git a/src/tests/exceptions.test.ts b/src/tests/exceptions.test.ts index b61eea0..a47a82c 100644 --- a/src/tests/exceptions.test.ts +++ b/src/tests/exceptions.test.ts @@ -44,6 +44,19 @@ describe("captureException", () => { expect(result.message).toBe("Custom error message"); expect(result.type).toBe("CustomError"); }); + + it("should always set platform to 'javascript'", () => { + const error = new Error("Test error"); + const result = captureException(error); + + expect(result.platform).toBe("javascript"); + }); + + it("should set platform to 'javascript' for non-Error objects", () => { + const result = captureException("string error"); + + expect(result.platform).toBe("javascript"); + }); }); describe("stack trace parsing", () => { @@ -161,6 +174,54 @@ describe("captureException", () => { expect(result.frames).toBeDefined(); expect(result.frames!.length).toBeLessThanOrEqual(50); }); + + it("should capture context_line for in_app frames", () => { + // This test throws a real error, so context_line should be captured + const error = new Error("Test error"); + const result = captureException(error); + + expect(result.frames).toBeDefined(); + const inAppFrames = result.frames!.filter((frame) => frame.in_app); + expect(inAppFrames.length).toBeGreaterThan(0); + + // At least one in_app frame should have context_line + const hasContextLine = inAppFrames.some( + (frame) => frame.context_line !== undefined, + ); + expect(hasContextLine).toBe(true); + }); + + it("should NOT capture context_line for library code (in_app: false)", () => { + // Create a mock stack trace with node_modules + const error = new Error("Test"); + error.stack = `Error: Test + at libFunction (/app/node_modules/some-lib/index.js:42:10) + at internal (node:internal/process:123:45)`; + + const result = captureException(error); + + expect(result.frames).toBeDefined(); + // All frames should be library code and should NOT have context_line + result.frames!.forEach((frame) => { + expect(frame.in_app).toBe(false); + expect(frame.context_line).toBeUndefined(); + }); + }); + + it("should handle missing files gracefully when extracting context_line", () => { + // Create a mock stack trace with a non-existent file + const error = new Error("Test"); + error.stack = `Error: Test + at testFunction (/nonexistent/file/path.ts:10:5)`; + + const result = captureException(error); + + expect(result.frames).toBeDefined(); + expect(result.frames!.length).toBe(1); + // Frame should be in_app but context_line should be undefined (file not found) + expect(result.frames![0].in_app).toBe(true); + expect(result.frames![0].context_line).toBeUndefined(); + }); }); describe("Error.cause chain", () => { diff --git a/src/tests/protocol-validation-errors.test.ts b/src/tests/protocol-validation-errors.test.ts new file mode 100644 index 0000000..9aa6e49 --- /dev/null +++ b/src/tests/protocol-validation-errors.test.ts @@ -0,0 +1,170 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { + setupTestServerAndClient, + resetTodos, +} from "./test-utils/client-server-factory.js"; +import { z } from "zod"; +import { track } from "../index.js"; +import { EventCapture } from "./test-utils.js"; + +describe("Protocol Validation Error Tests", () => { + let eventCapture: EventCapture; + + beforeEach(async () => { + resetTodos(); + eventCapture = new EventCapture(); + await eventCapture.start(); + }); + + afterEach(async () => { + await eventCapture.stop(); + }); + + it("should capture invalid enum parameter errors", async () => { + const { server, client, cleanup } = await setupTestServerAndClient(); + + try { + // Add a calculator tool with enum validation + server.tool( + "calculate", + "Perform calculation", + { + operation: z.enum(["add", "subtract", "multiply", "divide"]), + a: z.number(), + b: z.number(), + }, + async (args: { + operation: "add" | "subtract" | "multiply" | "divide"; + a: number; + b: number; + }) => { + const { operation, a, b } = args; + let result: number; + switch (operation) { + case "add": + result = a + b; + break; + case "subtract": + result = a - b; + break; + case "multiply": + result = a * b; + break; + case "divide": + result = a / b; + break; + } + return { + content: [{ type: "text" as const, text: String(result) }], + }; + }, + ); + + // Track the server AFTER registering tools + track(server, "test-project", { enableTracing: true }); + + // Now try to call with invalid enum value + try { + await client.request({ + method: "tools/call", + params: { + name: "calculate", + arguments: { + operation: "invalid_operation", // This should fail validation + a: 5, + b: 3, + context: "Testing invalid enum", + }, + }, + }); + // If we get here, the validation didn't work as expected + expect.fail("Should have thrown validation error"); + } catch (error: any) { + // This error should be caught by MCPcat + console.log("Caught validation error:", error.message); + expect(error.message).toContain("Invalid"); + } + + // Wait for event to be captured + await new Promise((resolve) => setTimeout(resolve, 200)); + + // Check if MCPcat captured this error + const events = eventCapture.findEventsByResourceName("calculate"); + console.log("Events captured for 'calculate':", events.length); + + if (events.length > 0) { + console.log( + "Event details:", + JSON.stringify( + events.map((e) => ({ + resourceName: e.resourceName, + isError: e.isError, + error: e.error, + })), + null, + 2, + ), + ); + } + + // This is what we're testing: Did MCPcat capture the validation error? + const errorEvent = events.find((e) => e.isError); + + if (!errorEvent) { + console.log("❌ MCPcat did NOT capture the protocol validation error"); + console.log("This confirms the bug that we need to fix"); + // For now, we expect this to fail - that's the bug we're investigating + expect(errorEvent).toBeUndefined(); + } else { + console.log("✅ MCPcat captured the validation error!"); + expect(errorEvent.error?.message).toBeDefined(); + } + } finally { + await cleanup(); + } + }); + + it("should capture invalid tool name errors", async () => { + const { server, client, cleanup } = await setupTestServerAndClient(); + + try { + // Track the server + track(server, "test-project", { enableTracing: true }); + + // Try to call non-existent tool + try { + await client.request({ + method: "tools/call", + params: { + name: "nonexistent_tool", + arguments: { + context: "Testing invalid tool name", + }, + }, + }); + expect.fail("Should have thrown tool not found error"); + } catch (error: any) { + console.log("Caught tool not found error:", error.message); + expect(error.message).toContain("Unknown tool"); + } + + await new Promise((resolve) => setTimeout(resolve, 200)); + + // Check if MCPcat captured this + const events = eventCapture.findEventsByResourceName("nonexistent_tool"); + console.log("Events for nonexistent tool:", events.length); + + const errorEvent = events.find((e) => e.isError); + if (!errorEvent) { + console.log("❌ MCPcat did NOT capture the tool not found error"); + // This is expected - the bug we're investigating + expect(errorEvent).toBeUndefined(); + } else { + console.log("✅ MCPcat captured the tool not found error!"); + expect(errorEvent.error?.message).toBeDefined(); + } + } finally { + await cleanup(); + } + }); +}); diff --git a/src/types.ts b/src/types.ts index 261e2fe..e75609f 100644 --- a/src/types.ts +++ b/src/types.ts @@ -179,6 +179,7 @@ export interface StackFrame { colno?: number; in_app: boolean; abs_path?: string; + context_line?: string; // The line of code where the error occurred } export interface ChainedErrorData { @@ -194,4 +195,5 @@ export interface ErrorData { stack?: string; // Full stack trace string frames?: StackFrame[]; // Parsed stack frames chained_errors?: ChainedErrorData[]; + platform: string; // Platform identifier (e.g., "javascript", "node") } From f0baea14043eb520f189e4849fa1b0d266c46b4c Mon Sep 17 00:00:00 2001 From: Kashish Hora Date: Wed, 19 Nov 2025 20:02:13 +0100 Subject: [PATCH 2/4] test: fix error message assertion --- src/tests/protocol-validation-errors.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/protocol-validation-errors.test.ts b/src/tests/protocol-validation-errors.test.ts index 9aa6e49..5c4ca51 100644 --- a/src/tests/protocol-validation-errors.test.ts +++ b/src/tests/protocol-validation-errors.test.ts @@ -145,7 +145,7 @@ describe("Protocol Validation Error Tests", () => { expect.fail("Should have thrown tool not found error"); } catch (error: any) { console.log("Caught tool not found error:", error.message); - expect(error.message).toContain("Unknown tool"); + expect(error.message).toContain("Tool nonexistent_tool not found"); } await new Promise((resolve) => setTimeout(resolve, 200)); From 33b13b6ae5397fd3a0329a17cb33980c92655415 Mon Sep 17 00:00:00 2001 From: Kashish Hora Date: Wed, 19 Nov 2025 20:05:49 +0100 Subject: [PATCH 3/4] fix: add platform field to all ErrorData objects --- src/modules/tools.ts | 10 ++++++++-- src/modules/tracing.ts | 11 +++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/modules/tools.ts b/src/modules/tools.ts index 979d6ad..35042b6 100644 --- a/src/modules/tools.ts +++ b/src/modules/tools.ts @@ -64,7 +64,10 @@ export function setupMCPCatTools(server: MCPServerLike): void { writeToLog( `Warning: Original list tools handler failed, this suggests an error MCPCat did not cause - ${error}`, ); - event.error = { message: getMCPCompatibleErrorMessage(error) }; + event.error = { + message: getMCPCompatibleErrorMessage(error), + platform: "javascript", + }; event.isError = true; event.duration = (event.timestamp && @@ -85,7 +88,10 @@ export function setupMCPCatTools(server: MCPServerLike): void { writeToLog( "Warning: No tools found in the original list. This is likely due to the tools not being registered before MCPCat.track().", ); - event.error = { message: "No tools were sent to MCP client." }; + event.error = { + message: "No tools were sent to MCP client.", + platform: "javascript", + }; event.isError = true; event.duration = (event.timestamp && diff --git a/src/modules/tracing.ts b/src/modules/tracing.ts index 2dbb9bc..cc418d9 100644 --- a/src/modules/tracing.ts +++ b/src/modules/tracing.ts @@ -74,7 +74,10 @@ export function setupListToolsTracing( writeToLog( `Warning: Original list tools handler failed, this suggests an error MCPCat did not cause - ${error}`, ); - event.error = { message: getMCPCompatibleErrorMessage(error) }; + event.error = { + message: getMCPCompatibleErrorMessage(error), + platform: "javascript", + }; event.isError = true; event.duration = (event.timestamp && @@ -95,7 +98,10 @@ export function setupListToolsTracing( writeToLog( "Warning: No tools found in the original list. This is likely due to the tools not being registered before MCPCat.track().", ); - event.error = { message: "No tools were sent to MCP client." }; + event.error = { + message: "No tools were sent to MCP client.", + platform: "javascript", + }; event.isError = true; event.duration = (event.timestamp && @@ -258,6 +264,7 @@ export function setupToolCallTracing(server: MCPServerLike): void { event.isError = true; event.error = { message: `Tool call handler not found for ${request.params?.name || "unknown"}`, + platform: "javascript", }; event.duration = (event.timestamp && From b84c938af6a50cf96919840417bc060ad755fb23 Mon Sep 17 00:00:00 2001 From: Kashish Hora Date: Wed, 19 Nov 2025 20:20:32 +0100 Subject: [PATCH 4/4] fix: make platform optional and remove unrelated test --- src/modules/tools.ts | 10 +- src/modules/tracing.ts | 11 +- src/tests/protocol-validation-errors.test.ts | 170 ------------------- src/types.ts | 2 +- 4 files changed, 5 insertions(+), 188 deletions(-) delete mode 100644 src/tests/protocol-validation-errors.test.ts diff --git a/src/modules/tools.ts b/src/modules/tools.ts index 35042b6..979d6ad 100644 --- a/src/modules/tools.ts +++ b/src/modules/tools.ts @@ -64,10 +64,7 @@ export function setupMCPCatTools(server: MCPServerLike): void { writeToLog( `Warning: Original list tools handler failed, this suggests an error MCPCat did not cause - ${error}`, ); - event.error = { - message: getMCPCompatibleErrorMessage(error), - platform: "javascript", - }; + event.error = { message: getMCPCompatibleErrorMessage(error) }; event.isError = true; event.duration = (event.timestamp && @@ -88,10 +85,7 @@ export function setupMCPCatTools(server: MCPServerLike): void { writeToLog( "Warning: No tools found in the original list. This is likely due to the tools not being registered before MCPCat.track().", ); - event.error = { - message: "No tools were sent to MCP client.", - platform: "javascript", - }; + event.error = { message: "No tools were sent to MCP client." }; event.isError = true; event.duration = (event.timestamp && diff --git a/src/modules/tracing.ts b/src/modules/tracing.ts index cc418d9..2dbb9bc 100644 --- a/src/modules/tracing.ts +++ b/src/modules/tracing.ts @@ -74,10 +74,7 @@ export function setupListToolsTracing( writeToLog( `Warning: Original list tools handler failed, this suggests an error MCPCat did not cause - ${error}`, ); - event.error = { - message: getMCPCompatibleErrorMessage(error), - platform: "javascript", - }; + event.error = { message: getMCPCompatibleErrorMessage(error) }; event.isError = true; event.duration = (event.timestamp && @@ -98,10 +95,7 @@ export function setupListToolsTracing( writeToLog( "Warning: No tools found in the original list. This is likely due to the tools not being registered before MCPCat.track().", ); - event.error = { - message: "No tools were sent to MCP client.", - platform: "javascript", - }; + event.error = { message: "No tools were sent to MCP client." }; event.isError = true; event.duration = (event.timestamp && @@ -264,7 +258,6 @@ export function setupToolCallTracing(server: MCPServerLike): void { event.isError = true; event.error = { message: `Tool call handler not found for ${request.params?.name || "unknown"}`, - platform: "javascript", }; event.duration = (event.timestamp && diff --git a/src/tests/protocol-validation-errors.test.ts b/src/tests/protocol-validation-errors.test.ts deleted file mode 100644 index 5c4ca51..0000000 --- a/src/tests/protocol-validation-errors.test.ts +++ /dev/null @@ -1,170 +0,0 @@ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { - setupTestServerAndClient, - resetTodos, -} from "./test-utils/client-server-factory.js"; -import { z } from "zod"; -import { track } from "../index.js"; -import { EventCapture } from "./test-utils.js"; - -describe("Protocol Validation Error Tests", () => { - let eventCapture: EventCapture; - - beforeEach(async () => { - resetTodos(); - eventCapture = new EventCapture(); - await eventCapture.start(); - }); - - afterEach(async () => { - await eventCapture.stop(); - }); - - it("should capture invalid enum parameter errors", async () => { - const { server, client, cleanup } = await setupTestServerAndClient(); - - try { - // Add a calculator tool with enum validation - server.tool( - "calculate", - "Perform calculation", - { - operation: z.enum(["add", "subtract", "multiply", "divide"]), - a: z.number(), - b: z.number(), - }, - async (args: { - operation: "add" | "subtract" | "multiply" | "divide"; - a: number; - b: number; - }) => { - const { operation, a, b } = args; - let result: number; - switch (operation) { - case "add": - result = a + b; - break; - case "subtract": - result = a - b; - break; - case "multiply": - result = a * b; - break; - case "divide": - result = a / b; - break; - } - return { - content: [{ type: "text" as const, text: String(result) }], - }; - }, - ); - - // Track the server AFTER registering tools - track(server, "test-project", { enableTracing: true }); - - // Now try to call with invalid enum value - try { - await client.request({ - method: "tools/call", - params: { - name: "calculate", - arguments: { - operation: "invalid_operation", // This should fail validation - a: 5, - b: 3, - context: "Testing invalid enum", - }, - }, - }); - // If we get here, the validation didn't work as expected - expect.fail("Should have thrown validation error"); - } catch (error: any) { - // This error should be caught by MCPcat - console.log("Caught validation error:", error.message); - expect(error.message).toContain("Invalid"); - } - - // Wait for event to be captured - await new Promise((resolve) => setTimeout(resolve, 200)); - - // Check if MCPcat captured this error - const events = eventCapture.findEventsByResourceName("calculate"); - console.log("Events captured for 'calculate':", events.length); - - if (events.length > 0) { - console.log( - "Event details:", - JSON.stringify( - events.map((e) => ({ - resourceName: e.resourceName, - isError: e.isError, - error: e.error, - })), - null, - 2, - ), - ); - } - - // This is what we're testing: Did MCPcat capture the validation error? - const errorEvent = events.find((e) => e.isError); - - if (!errorEvent) { - console.log("❌ MCPcat did NOT capture the protocol validation error"); - console.log("This confirms the bug that we need to fix"); - // For now, we expect this to fail - that's the bug we're investigating - expect(errorEvent).toBeUndefined(); - } else { - console.log("✅ MCPcat captured the validation error!"); - expect(errorEvent.error?.message).toBeDefined(); - } - } finally { - await cleanup(); - } - }); - - it("should capture invalid tool name errors", async () => { - const { server, client, cleanup } = await setupTestServerAndClient(); - - try { - // Track the server - track(server, "test-project", { enableTracing: true }); - - // Try to call non-existent tool - try { - await client.request({ - method: "tools/call", - params: { - name: "nonexistent_tool", - arguments: { - context: "Testing invalid tool name", - }, - }, - }); - expect.fail("Should have thrown tool not found error"); - } catch (error: any) { - console.log("Caught tool not found error:", error.message); - expect(error.message).toContain("Tool nonexistent_tool not found"); - } - - await new Promise((resolve) => setTimeout(resolve, 200)); - - // Check if MCPcat captured this - const events = eventCapture.findEventsByResourceName("nonexistent_tool"); - console.log("Events for nonexistent tool:", events.length); - - const errorEvent = events.find((e) => e.isError); - if (!errorEvent) { - console.log("❌ MCPcat did NOT capture the tool not found error"); - // This is expected - the bug we're investigating - expect(errorEvent).toBeUndefined(); - } else { - console.log("✅ MCPcat captured the tool not found error!"); - expect(errorEvent.error?.message).toBeDefined(); - } - } finally { - await cleanup(); - } - }); -}); diff --git a/src/types.ts b/src/types.ts index e75609f..fa384e9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -195,5 +195,5 @@ export interface ErrorData { stack?: string; // Full stack trace string frames?: StackFrame[]; // Parsed stack frames chained_errors?: ChainedErrorData[]; - platform: string; // Platform identifier (e.g., "javascript", "node") + platform?: string; // Platform identifier (e.g., "javascript", "node") }