From e75cd46ba65838428d23be0a8d618fde7bc12ca0 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 17 Apr 2026 19:25:20 +0100 Subject: [PATCH] test: isolate plugin tools mcp handlers --- src/mcp/plugin-tools-handlers.ts | 66 ++++++++++++++ src/mcp/plugin-tools-serve.test.ts | 135 ++++++++++++----------------- src/mcp/plugin-tools-serve.ts | 58 ++----------- 3 files changed, 128 insertions(+), 131 deletions(-) create mode 100644 src/mcp/plugin-tools-handlers.ts diff --git a/src/mcp/plugin-tools-handlers.ts b/src/mcp/plugin-tools-handlers.ts new file mode 100644 index 00000000000..2d7c67e73a5 --- /dev/null +++ b/src/mcp/plugin-tools-handlers.ts @@ -0,0 +1,66 @@ +import { + isToolWrappedWithBeforeToolCallHook, + wrapToolWithBeforeToolCallHook, +} from "../agents/pi-tools.before-tool-call.js"; +import type { AnyAgentTool } from "../agents/tools/common.js"; +import { formatErrorMessage } from "../infra/errors.js"; + +type CallPluginToolParams = { + name: string; + arguments?: unknown; +}; + +function resolveJsonSchemaForTool(tool: AnyAgentTool): Record { + const params = tool.parameters; + if (params && typeof params === "object" && "type" in params) { + return params as Record; + } + return { type: "object", properties: {} }; +} + +export function createPluginToolsMcpHandlers(tools: AnyAgentTool[]) { + const wrappedTools = tools.map((tool) => { + if (isToolWrappedWithBeforeToolCallHook(tool)) { + return tool; + } + // The ACPX MCP bridge should enforce the same pre-execution hook boundary + // as the agent and HTTP tool execution paths. + return wrapToolWithBeforeToolCallHook(tool); + }); + const toolMap = new Map(); + for (const tool of wrappedTools) { + toolMap.set(tool.name, tool); + } + + return { + listTools: async () => ({ + tools: wrappedTools.map((tool) => ({ + name: tool.name, + description: tool.description ?? "", + inputSchema: resolveJsonSchemaForTool(tool), + })), + }), + callTool: async (params: CallPluginToolParams) => { + const tool = toolMap.get(params.name); + if (!tool) { + return { + content: [{ type: "text", text: `Unknown tool: ${params.name}` }], + isError: true, + }; + } + try { + const result = await tool.execute(`mcp-${Date.now()}`, params.arguments ?? {}); + return { + content: Array.isArray(result.content) + ? result.content + : [{ type: "text", text: String(result.content) }], + }; + } catch (err) { + return { + content: [{ type: "text", text: `Tool error: ${formatErrorMessage(err)}` }], + isError: true, + }; + } + }, + }; +} diff --git a/src/mcp/plugin-tools-serve.test.ts b/src/mcp/plugin-tools-serve.test.ts index 04cdd339cef..cea3e50cb3b 100644 --- a/src/mcp/plugin-tools-serve.test.ts +++ b/src/mcp/plugin-tools-serve.test.ts @@ -1,5 +1,3 @@ -import { Client } from "@modelcontextprotocol/sdk/client/index.js"; -import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { afterEach, describe, expect, it, vi } from "vitest"; import type { AnyAgentTool } from "../agents/tools/common.js"; import { @@ -7,25 +5,17 @@ import { resetGlobalHookRunner, } from "../plugins/hook-runner-global.js"; import { createMockPluginRegistry } from "../plugins/hooks.test-helpers.js"; -import { createPluginToolsMcpServer } from "./plugin-tools-serve.js"; +import { createPluginToolsMcpHandlers } from "./plugin-tools-handlers.js"; -async function connectPluginToolsServer(tools: AnyAgentTool[]) { - const server = createPluginToolsMcpServer({ tools }); - const client = new Client({ name: "plugin-tools-test-client", version: "1.0.0" }); - const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); - await server.connect(serverTransport); - await client.connect(clientTransport); - return { - client, - close: async () => { - await client.close(); - await server.close(); - }, - }; -} +const callGatewayTool = vi.hoisted(() => vi.fn()); + +vi.mock("../agents/tools/gateway.js", () => ({ + callGatewayTool, +})); afterEach(() => { vi.restoreAllMocks(); + callGatewayTool.mockReset(); resetGlobalHookRunner(); }); @@ -47,36 +37,32 @@ describe("plugin tools MCP server", () => { execute, } as unknown as AnyAgentTool; - const session = await connectPluginToolsServer([tool]); - try { - const listed = await session.client.listTools(); - expect(listed.tools).toEqual([ - expect.objectContaining({ - name: "memory_recall", - description: "Recall stored memory", - inputSchema: expect.objectContaining({ - type: "object", - required: ["query"], - }), - }), - ]); - - const result = await session.client.callTool({ + const handlers = createPluginToolsMcpHandlers([tool]); + const listed = await handlers.listTools(); + expect(listed.tools).toEqual([ + expect.objectContaining({ name: "memory_recall", - arguments: { query: "remember this" }, - }); - expect(execute).toHaveBeenCalledWith( - expect.stringMatching(/^mcp-\d+$/), - { - query: "remember this", - }, - undefined, - undefined, - ); - expect(result.content).toEqual([{ type: "text", text: "Stored." }]); - } finally { - await session.close(); - } + description: "Recall stored memory", + inputSchema: expect.objectContaining({ + type: "object", + required: ["query"], + }), + }), + ]); + + const result = await handlers.callTool({ + name: "memory_recall", + arguments: { query: "remember this" }, + }); + expect(execute).toHaveBeenCalledWith( + expect.stringMatching(/^mcp-\d+$/), + { + query: "remember this", + }, + undefined, + undefined, + ); + expect(result.content).toEqual([{ type: "text", text: "Stored." }]); }); it("returns MCP errors for unknown tools and thrown tool errors", async () => { @@ -87,24 +73,20 @@ describe("plugin tools MCP server", () => { execute: vi.fn().mockRejectedValue(new Error("boom")), } as unknown as AnyAgentTool; - const session = await connectPluginToolsServer([failingTool]); - try { - const unknown = await session.client.callTool({ - name: "missing_tool", - arguments: {}, - }); - expect(unknown.isError).toBe(true); - expect(unknown.content).toEqual([{ type: "text", text: "Unknown tool: missing_tool" }]); + const handlers = createPluginToolsMcpHandlers([failingTool]); + const unknown = await handlers.callTool({ + name: "missing_tool", + arguments: {}, + }); + expect(unknown.isError).toBe(true); + expect(unknown.content).toEqual([{ type: "text", text: "Unknown tool: missing_tool" }]); - const failed = await session.client.callTool({ - name: "memory_forget", - arguments: {}, - }); - expect(failed.isError).toBe(true); - expect(failed.content).toEqual([{ type: "text", text: "Tool error: boom" }]); - } finally { - await session.close(); - } + const failed = await handlers.callTool({ + name: "memory_forget", + arguments: {}, + }); + expect(failed.isError).toBe(true); + expect(failed.content).toEqual([{ type: "text", text: "Tool error: boom" }]); }); it("blocks tool execution when before_tool_call requires approval on the MCP bridge", async () => { @@ -129,6 +111,7 @@ describe("plugin tools MCP server", () => { }, ]), ); + callGatewayTool.mockRejectedValueOnce(new Error("gateway unavailable")); const tool = { name: "memory_store", description: "Store memory", @@ -136,20 +119,16 @@ describe("plugin tools MCP server", () => { execute, } as unknown as AnyAgentTool; - const session = await connectPluginToolsServer([tool]); - try { - const result = await session.client.callTool({ - name: "memory_store", - arguments: { text: "remember this" }, - }); - expect(hookCalls).toBe(1); - expect(execute).not.toHaveBeenCalled(); - expect(result.isError).toBe(true); - expect(result.content).toEqual([ - { type: "text", text: "Tool error: Plugin approval required (gateway unavailable)" }, - ]); - } finally { - await session.close(); - } + const handlers = createPluginToolsMcpHandlers([tool]); + const result = await handlers.callTool({ + name: "memory_store", + arguments: { text: "remember this" }, + }); + expect(hookCalls).toBe(1); + expect(execute).not.toHaveBeenCalled(); + expect(result.isError).toBe(true); + expect(result.content).toEqual([ + { type: "text", text: "Tool error: Plugin approval required (gateway unavailable)" }, + ]); }); }); diff --git a/src/mcp/plugin-tools-serve.ts b/src/mcp/plugin-tools-serve.ts index 3414529aad6..8303998b2e1 100644 --- a/src/mcp/plugin-tools-serve.ts +++ b/src/mcp/plugin-tools-serve.ts @@ -10,10 +10,6 @@ import { pathToFileURL } from "node:url"; import { Server } from "@modelcontextprotocol/sdk/server/index.js"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; import { CallToolRequestSchema, ListToolsRequestSchema } from "@modelcontextprotocol/sdk/types.js"; -import { - isToolWrappedWithBeforeToolCallHook, - wrapToolWithBeforeToolCallHook, -} from "../agents/pi-tools.before-tool-call.js"; import type { AnyAgentTool } from "../agents/tools/common.js"; import { loadConfig } from "../config/config.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; @@ -21,15 +17,7 @@ import { formatErrorMessage } from "../infra/errors.js"; import { routeLogsToStderr } from "../logging/console.js"; import { resolvePluginTools } from "../plugins/tools.js"; import { VERSION } from "../version.js"; - -function resolveJsonSchemaForTool(tool: AnyAgentTool): Record { - const params = tool.parameters; - if (params && typeof params === "object" && "type" in params) { - return params as Record; - } - // Fallback: accept any object - return { type: "object", properties: {} }; -} +import { createPluginToolsMcpHandlers } from "./plugin-tools-handlers.js"; function resolveTools(config: OpenClawConfig): AnyAgentTool[] { return resolvePluginTools({ @@ -45,54 +33,18 @@ export function createPluginToolsMcpServer( } = {}, ): Server { const cfg = params.config ?? loadConfig(); - const tools = (params.tools ?? resolveTools(cfg)).map((tool) => { - if (isToolWrappedWithBeforeToolCallHook(tool)) { - return tool; - } - // The ACPX MCP bridge should enforce the same pre-execution hook boundary - // as the agent and HTTP tool execution paths. - return wrapToolWithBeforeToolCallHook(tool); - }); - - const toolMap = new Map(); - for (const tool of tools) { - toolMap.set(tool.name, tool); - } + const tools = params.tools ?? resolveTools(cfg); + const handlers = createPluginToolsMcpHandlers(tools); const server = new Server( { name: "openclaw-plugin-tools", version: VERSION }, { capabilities: { tools: {} } }, ); - server.setRequestHandler(ListToolsRequestSchema, async () => ({ - tools: tools.map((tool) => ({ - name: tool.name, - description: tool.description ?? "", - inputSchema: resolveJsonSchemaForTool(tool), - })), - })); + server.setRequestHandler(ListToolsRequestSchema, handlers.listTools); server.setRequestHandler(CallToolRequestSchema, async (request) => { - const tool = toolMap.get(request.params.name); - if (!tool) { - return { - content: [{ type: "text", text: `Unknown tool: ${request.params.name}` }], - isError: true, - }; - } - try { - const result = await tool.execute(`mcp-${Date.now()}`, request.params.arguments ?? {}); - return { - content: Array.isArray(result.content) - ? result.content - : [{ type: "text", text: String(result.content) }], - }; - } catch (err) { - return { - content: [{ type: "text", text: `Tool error: ${formatErrorMessage(err)}` }], - isError: true, - }; - } + return await handlers.callTool(request.params); }); return server;