From f11ec538dca2938e57302a1bedd5dd7d1e7a9488 Mon Sep 17 00:00:00 2001 From: Elias Meire Date: Wed, 23 Jul 2025 12:54:31 +0200 Subject: [PATCH] fix: Do not throw on tool errors, instead return error message (#17558) --- .../McpClientTool/McpClientTool.node.test.ts | 74 +++++++++++++++++++ .../mcp/McpClientTool/McpClientTool.node.ts | 12 ++- .../nodes/mcp/McpClientTool/utils.ts | 24 +++--- 3 files changed, 94 insertions(+), 16 deletions(-) diff --git a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.test.ts b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.test.ts index 846e1634d8..67d6660982 100644 --- a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.test.ts @@ -2,6 +2,7 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js'; import { mock } from 'jest-mock-extended'; import { + NodeConnectionTypes, NodeOperationError, type ILoadOptionsFunctions, type INode, @@ -284,5 +285,78 @@ describe('McpClientTool', () => { headers: { Accept: 'text/event-stream', Authorization: 'Bearer my-token' }, }); }); + + it('should successfully execute a tool', async () => { + jest.spyOn(Client.prototype, 'connect').mockResolvedValue(); + jest.spyOn(Client.prototype, 'callTool').mockResolvedValue({ content: 'Sunny' }); + jest.spyOn(Client.prototype, 'listTools').mockResolvedValue({ + tools: [ + { + name: 'Weather Tool', + description: 'Gets the current weather', + inputSchema: { type: 'object', properties: { location: { type: 'string' } } }, + }, + ], + }); + + const supplyDataResult = await new McpClientTool().supplyData.call( + mock({ + getNode: jest.fn(() => + mock({ + typeVersion: 1, + }), + ), + logger: { debug: jest.fn(), error: jest.fn() }, + addInputData: jest.fn(() => ({ index: 0 })), + }), + 0, + ); + + expect(supplyDataResult.closeFunction).toBeInstanceOf(Function); + expect(supplyDataResult.response).toBeInstanceOf(McpToolkit); + + const tools = (supplyDataResult.response as McpToolkit).getTools(); + const toolResult = await tools[0].invoke({ location: 'Berlin' }); + expect(toolResult).toEqual('Sunny'); + }); + + it('should handle tool errors', async () => { + jest.spyOn(Client.prototype, 'connect').mockResolvedValue(); + jest + .spyOn(Client.prototype, 'callTool') + .mockResolvedValue({ isError: true, content: [{ text: 'Weather unknown at location' }] }); + jest.spyOn(Client.prototype, 'listTools').mockResolvedValue({ + tools: [ + { + name: 'Weather Tool', + description: 'Gets the current weather', + inputSchema: { type: 'object', properties: { location: { type: 'string' } } }, + }, + ], + }); + + const supplyDataFunctions = mock({ + getNode: jest.fn(() => + mock({ + typeVersion: 1, + }), + ), + logger: { debug: jest.fn(), error: jest.fn() }, + addInputData: jest.fn(() => ({ index: 0 })), + }); + const supplyDataResult = await new McpClientTool().supplyData.call(supplyDataFunctions, 0); + + expect(supplyDataResult.closeFunction).toBeInstanceOf(Function); + expect(supplyDataResult.response).toBeInstanceOf(McpToolkit); + + const tools = (supplyDataResult.response as McpToolkit).getTools(); + const toolResult = await tools[0].invoke({ location: 'Berlin' }); + expect(toolResult).toEqual('Weather unknown at location'); + expect(supplyDataFunctions.addOutputData).toHaveBeenCalledWith( + NodeConnectionTypes.AiTool, + 0, + new NodeOperationError(supplyDataFunctions.getNode(), 'Weather unknown at location'), + ); + }); }); }); diff --git a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.ts b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.ts index 53dd4f9043..dbd5113c68 100644 --- a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/McpClientTool.node.ts @@ -1,3 +1,5 @@ +import { logWrapper } from '@utils/logWrapper'; +import { getConnectionHintNoticeField } from '@utils/sharedFields'; import { NodeConnectionTypes, NodeOperationError, @@ -7,9 +9,6 @@ import { type SupplyData, } from 'n8n-workflow'; -import { logWrapper } from '@utils/logWrapper'; -import { getConnectionHintNoticeField } from '@utils/sharedFields'; - import { getTools } from './loadOptions'; import type { McpServerTransport, McpAuthenticationOption, McpToolIncludeMode } from './types'; import { @@ -294,11 +293,10 @@ export class McpClientTool implements INodeType { logWrapper( mcpToolToDynamicTool( tool, - createCallTool(tool.name, client.result, (error) => { + createCallTool(tool.name, client.result, (errorMessage) => { + const error = new NodeOperationError(node, errorMessage, { itemIndex }); + void this.addOutputData(NodeConnectionTypes.AiTool, itemIndex, error); this.logger.error(`McpClientTool: Tool "${tool.name}" failed to execute`, { error }); - throw new NodeOperationError(node, `Failed to execute tool "${tool.name}"`, { - description: error, - }); }), ), this, diff --git a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/utils.ts b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/utils.ts index 140924acb3..06cbe8ceb0 100644 --- a/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/utils.ts +++ b/packages/@n8n/nodes-langchain/nodes/mcp/McpClientTool/utils.ts @@ -3,6 +3,7 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js'; import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; import { CompatibilityCallToolResultSchema } from '@modelcontextprotocol/sdk/types.js'; +import { convertJsonSchemaToZod } from '@utils/schemaParsing'; import { Toolkit } from 'langchain/agents'; import { createResultError, @@ -13,12 +14,10 @@ import { } from 'n8n-workflow'; import { z } from 'zod'; -import { convertJsonSchemaToZod } from '@utils/schemaParsing'; - import type { McpAuthenticationOption, - McpTool, McpServerTransport, + McpTool, McpToolIncludeMode, } from './types'; @@ -78,17 +77,24 @@ export const getErrorDescriptionFromToolCall = (result: unknown): string | undef }; export const createCallTool = - (name: string, client: Client, onError: (error: string | undefined) => void) => - async (args: IDataObject) => { + (name: string, client: Client, onError: (error: string) => void) => async (args: IDataObject) => { let result: Awaited>; + + function handleError(error: unknown) { + const errorDescription = + getErrorDescriptionFromToolCall(error) ?? `Failed to execute tool "${name}"`; + onError(errorDescription); + return errorDescription; + } + try { result = await client.callTool({ name, arguments: args }, CompatibilityCallToolResultSchema); } catch (error) { - return onError(getErrorDescriptionFromToolCall(error)); + return handleError(error); } if (result.isError) { - return onError(getErrorDescriptionFromToolCall(result)); + return handleError(result); } if (result.toolResult !== undefined) { @@ -105,7 +111,7 @@ export const createCallTool = export function mcpToolToDynamicTool( tool: McpTool, onCallTool: DynamicStructuredToolInput['func'], -): DynamicStructuredTool> { +): DynamicStructuredTool { const rawSchema = convertJsonSchemaToZod(tool.inputSchema); // Ensure we always have an object schema for structured tools @@ -122,7 +128,7 @@ export function mcpToolToDynamicTool( } export class McpToolkit extends Toolkit { - constructor(public tools: Array>>) { + constructor(public tools: DynamicStructuredTool[]) { super(); } }