From eab6fcedaa5507c828be279e6819aa23e166b327 Mon Sep 17 00:00:00 2001 From: Agustin Rivera <31522568+eleqtrizit@users.noreply.github.com> Date: Fri, 10 Apr 2026 13:05:34 -0700 Subject: [PATCH] Ensure ACPX plugin-tools bridge honors before_tool_call (#63886) * fix(acpx): honor tool hook on plugin bridge Co-authored-by: smaeljaish771 * chore(changelog): add ACPX plugin-tools before_tool_call entry --------- Co-authored-by: smaeljaish771 Co-authored-by: Devin Robison --- CHANGELOG.md | 1 + src/mcp/plugin-tools-serve.test.ts | 95 +++++++++++++++++++++++++++++- src/mcp/plugin-tools-serve.ts | 13 +++- 3 files changed, 105 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f9162bb30e..c92a8e8f65f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,7 @@ Docs: https://docs.openclaw.ai - Hooks/security: mark agent hook system events as untrusted and sanitize hook display names before cron metadata reuse. (#64372) Thanks @eleqtrizit. - Media/security: honor sender-scoped `toolsBySender` policy for outbound host-media reads so denied senders cannot trigger host file disclosure via attachment hydration. (#64459) Thanks @eleqtrizit. - Browser/security: reject strict-policy hostname navigation unless the hostname is an explicit allowlist exception or IP literal, and route CDP HTTP discovery through the pinned SSRF fetch path. (#64367) Thanks @eleqtrizit. +- Plugins/ACPX: wrap plugin tools on the MCP bridge with the shared `before_tool_call` handler so block and approval hooks fire consistently across all execution paths. (#63886) Thanks @eleqtrizit. ## 2026.4.9 diff --git a/src/mcp/plugin-tools-serve.test.ts b/src/mcp/plugin-tools-serve.test.ts index 9f7c239c8ff..33358cafcf3 100644 --- a/src/mcp/plugin-tools-serve.test.ts +++ b/src/mcp/plugin-tools-serve.test.ts @@ -2,6 +2,11 @@ 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 { + initializeGlobalHookRunner, + resetGlobalHookRunner, +} from "../plugins/hook-runner-global.js"; +import { createMockPluginRegistry } from "../plugins/hooks.test-helpers.js"; import { createPluginToolsMcpServer } from "./plugin-tools-serve.js"; async function connectPluginToolsServer(tools: AnyAgentTool[]) { @@ -21,6 +26,7 @@ async function connectPluginToolsServer(tools: AnyAgentTool[]) { afterEach(() => { vi.restoreAllMocks(); + resetGlobalHookRunner(); }); describe("plugin tools MCP server", () => { @@ -73,9 +79,14 @@ describe("plugin tools MCP server", () => { name: "memory_store", arguments: { text: "remember this" }, }); - expect(execute).toHaveBeenCalledWith(expect.stringMatching(/^mcp-\d+$/), { - text: "remember this", - }); + expect(execute).toHaveBeenCalledWith( + expect.stringMatching(/^mcp-\d+$/), + { + text: "remember this", + }, + undefined, + undefined, + ); expect(result.content).toEqual([{ type: "text", text: "Stored." }]); } finally { await session.close(); @@ -109,4 +120,82 @@ describe("plugin tools MCP server", () => { await session.close(); } }); + + it("blocks tool execution when before_tool_call requires approval on the MCP bridge", async () => { + let hookCalls = 0; + const execute = vi.fn().mockResolvedValue({ + content: "Stored.", + }); + initializeGlobalHookRunner( + createMockPluginRegistry([ + { + hookName: "before_tool_call", + handler: async () => { + hookCalls += 1; + return { + requireApproval: { + pluginId: "test-plugin", + title: "Approval required", + description: "Approval required", + }, + }; + }, + }, + ]), + ); + const tool = { + name: "memory_store", + description: "Store memory", + parameters: { type: "object", properties: {} }, + 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(); + } + }); + + it("still executes plugin tools on the MCP bridge when no before_tool_call hook is registered", async () => { + const execute = vi.fn().mockResolvedValue({ + content: "Stored.", + }); + const tool = { + name: "memory_store", + description: "Store memory", + parameters: { type: "object", properties: {} }, + 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(execute).toHaveBeenCalledWith( + expect.stringMatching(/^mcp-\d+$/), + { + text: "remember this", + }, + undefined, + undefined, + ); + expect(result.isError).toBeUndefined(); + expect(result.content).toEqual([{ type: "text", text: "Stored." }]); + } finally { + await session.close(); + } + }); }); diff --git a/src/mcp/plugin-tools-serve.ts b/src/mcp/plugin-tools-serve.ts index d587c9cba65..2a8012fa7fd 100644 --- a/src/mcp/plugin-tools-serve.ts +++ b/src/mcp/plugin-tools-serve.ts @@ -10,6 +10,10 @@ 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 type { OpenClawConfig } from "../config/config.js"; import { loadConfig } from "../config/config.js"; @@ -41,7 +45,14 @@ export function createPluginToolsMcpServer( } = {}, ): Server { const cfg = params.config ?? loadConfig(); - const tools = params.tools ?? resolveTools(cfg); + 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) {