diff --git a/src/connectors/tools.ts b/src/connectors/tools.ts index 822194fa..a0767941 100644 --- a/src/connectors/tools.ts +++ b/src/connectors/tools.ts @@ -200,7 +200,42 @@ export function createOpenWikiConnectorTools(): StructuredToolInterface[] { ), ), }), - ]; + ].map(withToolErrorsAsResults); +} + +/** + * Return thrown tool errors as the tool *result* instead of rejecting (#427). + * + * A thrown tool error aborts the agent run, and even when the run survives, the + * model never sees the message. That matters because these messages are written + * for the model — `callMcpTool` answers a wrong tool name with + * + * MCP tool notion-get-page-content was not returned by tools/list for notion. + * Run openwiki_list_mcp_tools first and use an exact discovered name. + * + * which is precisely the hint needed to retry correctly. Returning it lets the + * model self-correct; throwing it just ends the turn. + * + * Errors are surfaced, not swallowed: the result is prefixed `Tool error:` so a + * failure is never mistaken for data, and the run transcript still shows it. + */ +export function withToolErrorsAsResults( + tool: DynamicStructuredTool, +): DynamicStructuredTool { + const originalFunc = tool.func.bind(tool) as ( + ...args: Parameters + ) => Promise; + + tool.func = (async (...args: Parameters) => { + try { + return await originalFunc(...args); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return `Tool error: ${message}`; + } + }) as DynamicStructuredTool["func"]; + + return tool; } async function listConnectors() { diff --git a/test/connector-tool-errors.test.ts b/test/connector-tool-errors.test.ts new file mode 100644 index 00000000..793bf067 --- /dev/null +++ b/test/connector-tool-errors.test.ts @@ -0,0 +1,115 @@ +import { DynamicStructuredTool } from "@langchain/core/tools"; +import { describe, expect, test } from "vitest"; + +import { + createOpenWikiConnectorTools, + withToolErrorsAsResults, +} from "../src/connectors/tools.ts"; + +// The connector tools threw on error instead of returning the message as the +// tool result (issue #427). A thrown tool error aborts the agent run, and even +// when it doesn't, the model never sees the text — which matters because these +// messages are written *for* the model. `callMcpTool` answers a wrong tool name +// with "Run openwiki_list_mcp_tools first and use an exact discovered name", +// exactly the hint needed to retry, and the model never received it. +// +// Note on scope: schema violations are rejected by LangChain before `func` runs +// and are a different layer. These tests cover errors thrown *inside* the tool, +// which is what the issue is about. + +function toolNamed(name: string) { + const tool = createOpenWikiConnectorTools().find( + (candidate) => candidate.name === name, + ); + if (!tool) { + throw new Error(`tool ${name} not registered`); + } + return tool; +} + +describe("withToolErrorsAsResults", () => { + test("a thrown Error becomes a labelled result", async () => { + const tool = withToolErrorsAsResults( + new DynamicStructuredTool({ + description: "d", + func: () => { + throw new Error("the model should read this"); + }, + name: "boom", + schema: { additionalProperties: false, properties: {}, type: "object" }, + }), + ); + + await expect(tool.invoke({})).resolves.toBe( + "Tool error: the model should read this", + ); + }); + + test("a rejected promise becomes a labelled result", async () => { + const tool = withToolErrorsAsResults( + new DynamicStructuredTool({ + description: "d", + func: () => Promise.reject(new Error("async failure")), + name: "boom-async", + schema: { additionalProperties: false, properties: {}, type: "object" }, + }), + ); + + await expect(tool.invoke({})).resolves.toBe("Tool error: async failure"); + }); + + test("a non-Error throw is still reported", async () => { + const tool = withToolErrorsAsResults( + new DynamicStructuredTool({ + description: "d", + // A provider that rejects with a bare string, which the wrapper must + // still report rather than losing. + // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors + func: () => Promise.reject("rate limited"), + name: "boom-string", + schema: { additionalProperties: false, properties: {}, type: "object" }, + }), + ); + + await expect(tool.invoke({})).resolves.toBe("Tool error: rate limited"); + }); + + test("a successful result passes through untouched", async () => { + const tool = withToolErrorsAsResults( + new DynamicStructuredTool({ + description: "d", + func: () => Promise.resolve("real output"), + name: "fine", + schema: { additionalProperties: false, properties: {}, type: "object" }, + }), + ); + + await expect(tool.invoke({})).resolves.toBe("real output"); + }); +}); + +describe("registered connector tools", () => { + test("a wrong MCP tool name returns the self-correction hint instead of throwing", async () => { + // The issue's headline case: the model guesses `notion-get-page-content` + // instead of a discovered name. Schema-valid, so it reaches the tool body. + const tool = toolNamed("openwiki_call_mcp_tool"); + + const result = String( + await tool.invoke({ + args: {}, + connectorId: "notion", + toolName: "notion-get-page-content", + }), + ); + + expect(result.startsWith("Tool error:")).toBe(true); + expect(result.length).toBeGreaterThan("Tool error: ".length); + }); + + test("a successful connector tool call is unaffected", async () => { + const result = String(await toolNamed("openwiki_list_connectors").invoke({})); + + expect(result).not.toContain("Tool error:"); + expect(() => JSON.parse(result) as unknown).not.toThrow(); + }); +}); diff --git a/test/raw-connector-tools.test.ts b/test/raw-connector-tools.test.ts index 881f9236..a9b27b22 100644 --- a/test/raw-connector-tools.test.ts +++ b/test/raw-connector-tools.test.ts @@ -85,13 +85,15 @@ describe("raw connector tools", () => { const linkRelativePath = await createSymlinkRawItem(home, "x", runId); const tools = await loadConnectorTools(home); - await expect( - getTool(tools, "openwiki_read_raw_item").invoke({ + // Surfaced as a labelled tool result rather than a rejection (#427); + // the symlink is still refused, and the reason still reaches the model. + const result = String(await getTool(tools, "openwiki_read_raw_item").invoke({ connectorId: "x", maxBytes: 100, path: linkRelativePath, - }), - ).rejects.toThrow(/symbolic links/u); + })); + expect(result).toMatch(/^Tool error:/u); + expect(result).toMatch(/symbolic links/u); }); test("rejects symlink raw directories before listing", async () => { @@ -99,9 +101,11 @@ describe("raw connector tools", () => { await createSymlinkRawDir(home, "x"); const tools = await loadConnectorTools(home); - await expect( - getTool(tools, "openwiki_list_raw_items").invoke({ connectorId: "x" }), - ).rejects.toThrow(/symbolic links/u); + // Surfaced as a labelled tool result rather than a rejection (#427); + // the symlink is still refused, and the reason still reaches the model. + const result = String(await getTool(tools, "openwiki_list_raw_items").invoke({ connectorId: "x" })); + expect(result).toMatch(/^Tool error:/u); + expect(result).toMatch(/symbolic links/u); }); test("rejects symlink raw directories before reading", async () => { @@ -109,13 +113,15 @@ describe("raw connector tools", () => { const rawItemPath = await createSymlinkRawDir(home, "x"); const tools = await loadConnectorTools(home); - await expect( - getTool(tools, "openwiki_read_raw_item").invoke({ + // Surfaced as a labelled tool result rather than a rejection (#427); + // the symlink is still refused, and the reason still reaches the model. + const result = String(await getTool(tools, "openwiki_read_raw_item").invoke({ connectorId: "x", maxBytes: 100, path: rawItemPath, - }), - ).rejects.toThrow(/symbolic links/u); + })); + expect(result).toMatch(/^Tool error:/u); + expect(result).toMatch(/symbolic links/u); }); });