From f47af1607a11314461960abb2cb523aec66a8fa2 Mon Sep 17 00:00:00 2001
From: Lee James <40045512+leehagoodjames@users.noreply.github.com>
Date: Thu, 14 Aug 2025 18:30:05 -0400
Subject: [PATCH] bug(mcp): catch errors reported by GitHub MCP (#6194)

---
 packages/core/src/tools/mcp-tool.test.ts | 92 ++++++++++++++++++++++++
 packages/core/src/tools/mcp-tool.ts      | 30 ++++++++
 2 files changed, 122 insertions(+)

diff --git a/packages/core/src/tools/mcp-tool.test.ts b/packages/core/src/tools/mcp-tool.test.ts
index 36602d49..357d289a 100644
--- a/packages/core/src/tools/mcp-tool.test.ts
+++ b/packages/core/src/tools/mcp-tool.test.ts
@@ -185,6 +185,98 @@ describe('DiscoveredMCPTool', () => {
       ).rejects.toThrow(expectedError);
     });
 
+    it.each([
+      { isErrorValue: true, description: 'true (bool)' },
+      { isErrorValue: 'true', description: '"true" (str)' },
+    ])(
+      'should consider a ToolResult with isError $description to be a failure',
+      async ({ isErrorValue }) => {
+        const tool = new DiscoveredMCPTool(
+          mockCallableToolInstance,
+          serverName,
+          serverToolName,
+          baseDescription,
+          inputSchema,
+        );
+        const params = { param: 'isErrorTrueCase' };
+
+        const errorResponse = { isError: isErrorValue };
+        const mockMcpToolResponseParts: Part[] = [
+          {
+            functionResponse: {
+              name: serverToolName,
+              response: { error: errorResponse },
+            },
+          },
+        ];
+        mockCallTool.mockResolvedValue(mockMcpToolResponseParts);
+        const expectedError = new Error(
+          `MCP tool '${serverToolName}' reported tool error with response: ${JSON.stringify(
+            mockMcpToolResponseParts,
+          )}`,
+        );
+
+        const invocation = tool.build(params);
+        await expect(
+          invocation.execute(new AbortController().signal),
+        ).rejects.toThrow(expectedError);
+      },
+    );
+
+    it.each([
+      { isErrorValue: false, description: 'false (bool)' },
+      { isErrorValue: 'false', description: '"false" (str)' },
+    ])(
+      'should consider a ToolResult with isError ${description} to be a success',
+      async ({ isErrorValue }) => {
+        const tool = new DiscoveredMCPTool(
+          mockCallableToolInstance,
+          serverName,
+          serverToolName,
+          baseDescription,
+          inputSchema,
+        );
+        const params = { param: 'isErrorFalseCase' };
+        const mockToolSuccessResultObject = {
+          success: true,
+          details: 'executed',
+        };
+        const mockFunctionResponseContent = [
+          {
+            type: 'text',
+            text: JSON.stringify(mockToolSuccessResultObject),
+          },
+        ];
+
+        const errorResponse = { isError: isErrorValue };
+        const mockMcpToolResponseParts: Part[] = [
+          {
+            functionResponse: {
+              name: serverToolName,
+              response: {
+                error: errorResponse,
+                content: mockFunctionResponseContent,
+              },
+            },
+          },
+        ];
+        mockCallTool.mockResolvedValue(mockMcpToolResponseParts);
+
+        const invocation = tool.build(params);
+        const toolResult = await invocation.execute(
+          new AbortController().signal,
+        );
+
+        const stringifiedResponseContent = JSON.stringify(
+          mockToolSuccessResultObject,
+        );
+        expect(toolResult.llmContent).toEqual([
+          { text: stringifiedResponseContent },
+        ]);
+        expect(toolResult.returnDisplay).toBe(stringifiedResponseContent);
+      },
+    );
+
     it('should handle a simple text response correctly', async () => {
       const params = { query: 'test' };
       const successMessage = 'This is a success message.';
diff --git a/packages/core/src/tools/mcp-tool.ts b/packages/core/src/tools/mcp-tool.ts
index fbb104fd..88e89c85 100644
--- a/packages/core/src/tools/mcp-tool.ts
+++ b/packages/core/src/tools/mcp-tool.ts
@@ -104,6 +104,28 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation<
     return confirmationDetails;
   }
 
+  // Determine if the response contains tool errors
+  // This is needed because CallToolResults should return errors inside the response.
+  // ref: https://modelcontextprotocol.io/specification/2025-06-18/schema#calltoolresult
+  isMCPToolError(rawResponseParts: Part[]): boolean {
+    const functionResponse = rawResponseParts?.[0]?.functionResponse;
+    const response = functionResponse?.response;
+
+    interface McpError {
+      isError?: boolean | string;
+    }
+
+    if (response) {
+      const error = (response as { error?: McpError })?.error;
+      const isError = error?.isError;
+
+      if (error && (isError === true || isError === 'true')) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   async execute(): Promise<ToolResult> {
     const functionCalls: FunctionCall[] = [
       {
@@ -113,6 +135,14 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation<
     ];
 
     const rawResponseParts = await this.mcpTool.callTool(functionCalls);
+
+    // Ensure the response is not an error
+    if (this.isMCPToolError(rawResponseParts)) {
+      throw new Error(
+        `MCP tool '${this.serverToolName}' reported tool error with response: ${JSON.stringify(rawResponseParts)}`,
+      );
+    }
+
     const transformedParts = transformMcpContentToParts(rawResponseParts);
 
     return {