From 8c3cc793b70c62d404f2710c2324e052061d25cc Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 02:48:51 +0100 Subject: [PATCH] fix: dedupe before_tool_call in embedded runtime (#15635) (thanks @lailoo) --- CHANGELOG.md | 2 +- .../pi-embedded-subscribe.handlers.tools.ts | 19 +------ ...nition-adapter.after-tool-call.e2e.test.ts | 49 +++++++++++++++++-- src/agents/pi-tool-definition-adapter.ts | 33 ++++++++++--- src/agents/pi-tools.before-tool-call.ts | 32 +++++++++++- .../wired-hooks-after-tool-call.e2e.test.ts | 4 ++ 6 files changed, 108 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d76c24431b..d1c4347f344 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,7 @@ Docs: https://docs.openclaw.ai - Discord: route autoThread replies to existing threads instead of the root channel. (#8302) Thanks @gavinbmoore, @thewilloftheshadow. - Discord/Agents: apply channel/group `historyLimit` during embedded-runner history compaction to prevent long-running channel sessions from bypassing truncation and overflowing context windows. (#11224) Thanks @shadril238. - Telegram: scope skill commands to the resolved agent for default accounts so `setMyCommands` no longer triggers `BOT_COMMANDS_TOO_MUCH` when multiple agents are configured. (#15599) -- Plugins/Hooks: fire `before_tool_call` hook exactly once per tool invocation instead of twice when tools pass through both `wrapToolWithBeforeToolCallHook` and `toToolDefinitions`. (#15635) Thanks @lailoo. +- Plugins/Hooks: fire `before_tool_call` hook exactly once per tool invocation in embedded runs by removing duplicate dispatch paths while preserving parameter mutation semantics. (#15635) Thanks @lailoo. - Agents/Image tool: cap image-analysis completion `maxTokens` by model capability (`min(4096, model.maxTokens)`) to avoid over-limit provider failures while still preventing truncation. (#11770) Thanks @detecti1. - TUI/Streaming: preserve richer streamed assistant text when final payload drops pre-tool-call text blocks, while keeping non-empty final payload authoritative for plain-text updates. (#15452) Thanks @TsekaLuk. - Inbound/Web UI: preserve literal `\n` sequences when normalizing inbound text so Windows paths like `C:\\Work\\nxxx\\README.md` are not corrupted. (#11547) Thanks @mcaxtr. diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.ts b/src/agents/pi-embedded-subscribe.handlers.tools.ts index 3ab11f985f9..ba2151be8da 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.ts @@ -1,8 +1,5 @@ import type { AgentEvent } from "@mariozechner/pi-agent-core"; -import type { - PluginHookAfterToolCallEvent, - PluginHookBeforeToolCallEvent, -} from "../plugins/types.js"; +import type { PluginHookAfterToolCallEvent } from "../plugins/types.js"; import type { EmbeddedPiSubscribeContext } from "./pi-embedded-subscribe.handlers.types.js"; import { emitAgentEvent } from "../infra/agent-events.js"; import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; @@ -61,20 +58,6 @@ export async function handleToolExecutionStart( // Track start time and args for after_tool_call hook toolStartData.set(toolCallId, { startTime: Date.now(), args }); - // Call before_tool_call hook - const hookRunner = ctx.hookRunner ?? getGlobalHookRunner(); - if (hookRunner?.hasHooks?.("before_tool_call")) { - try { - const hookEvent: PluginHookBeforeToolCallEvent = { - toolName, - params: args && typeof args === "object" ? (args as Record) : {}, - }; - await hookRunner.runBeforeToolCall(hookEvent, { toolName }); - } catch (err) { - ctx.log.debug(`before_tool_call hook failed: tool=${toolName} error=${String(err)}`); - } - } - if (toolName === "read") { const record = args && typeof args === "object" ? (args as Record) : {}; const filePath = typeof record.path === "string" ? record.path.trim() : ""; diff --git a/src/agents/pi-tool-definition-adapter.after-tool-call.e2e.test.ts b/src/agents/pi-tool-definition-adapter.after-tool-call.e2e.test.ts index 8cfdade34cb..cbcca9625b0 100644 --- a/src/agents/pi-tool-definition-adapter.after-tool-call.e2e.test.ts +++ b/src/agents/pi-tool-definition-adapter.after-tool-call.e2e.test.ts @@ -7,6 +7,8 @@ const hookMocks = vi.hoisted(() => ({ hasHooks: vi.fn(() => false), runAfterToolCall: vi.fn(async () => {}), }, + isToolWrappedWithBeforeToolCallHook: vi.fn(() => false), + consumeAdjustedParamsForToolCall: vi.fn(() => undefined), runBeforeToolCallHook: vi.fn(async ({ params }: { params: unknown }) => ({ blocked: false, params, @@ -18,6 +20,8 @@ vi.mock("../plugins/hook-runner-global.js", () => ({ })); vi.mock("./pi-tools.before-tool-call.js", () => ({ + consumeAdjustedParamsForToolCall: hookMocks.consumeAdjustedParamsForToolCall, + isToolWrappedWithBeforeToolCallHook: hookMocks.isToolWrappedWithBeforeToolCallHook, runBeforeToolCallHook: hookMocks.runBeforeToolCallHook, })); @@ -26,6 +30,10 @@ describe("pi tool definition adapter after_tool_call", () => { hookMocks.runner.hasHooks.mockReset(); hookMocks.runner.runAfterToolCall.mockReset(); hookMocks.runner.runAfterToolCall.mockResolvedValue(undefined); + hookMocks.isToolWrappedWithBeforeToolCallHook.mockReset(); + hookMocks.isToolWrappedWithBeforeToolCallHook.mockReturnValue(false); + hookMocks.consumeAdjustedParamsForToolCall.mockReset(); + hookMocks.consumeAdjustedParamsForToolCall.mockReturnValue(undefined); hookMocks.runBeforeToolCallHook.mockReset(); hookMocks.runBeforeToolCallHook.mockImplementation(async ({ params }) => ({ blocked: false, @@ -35,6 +43,10 @@ describe("pi tool definition adapter after_tool_call", () => { it("dispatches after_tool_call once on successful adapter execution", async () => { hookMocks.runner.hasHooks.mockImplementation((name: string) => name === "after_tool_call"); + hookMocks.runBeforeToolCallHook.mockResolvedValue({ + blocked: false, + params: { mode: "safe" }, + }); const tool = { name: "read", label: "Read", @@ -48,13 +60,42 @@ describe("pi tool definition adapter after_tool_call", () => { expect(result.details).toMatchObject({ ok: true }); expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledTimes(1); - // after_tool_call receives the params as passed to toToolDefinitions; - // before_tool_call adjustment happens in wrapToolWithBeforeToolCallHook - // (upstream), not inside toToolDefinitions (#15502). expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledWith( { toolName: "read", - params: { path: "/tmp/file" }, + params: { mode: "safe" }, + result, + }, + { toolName: "read" }, + ); + }); + + it("uses wrapped-tool adjusted params for after_tool_call payload", async () => { + hookMocks.runner.hasHooks.mockImplementation((name: string) => name === "after_tool_call"); + hookMocks.isToolWrappedWithBeforeToolCallHook.mockReturnValue(true); + hookMocks.consumeAdjustedParamsForToolCall.mockReturnValue({ mode: "safe" }); + const tool = { + name: "read", + label: "Read", + description: "reads", + parameters: {}, + execute: vi.fn(async () => ({ content: [], details: { ok: true } })), + } satisfies AgentTool; + + const defs = toToolDefinitions([tool]); + const result = await defs[0].execute( + "call-ok-wrapped", + { path: "/tmp/file" }, + undefined, + undefined, + ); + + expect(result.details).toMatchObject({ ok: true }); + expect(hookMocks.runBeforeToolCallHook).not.toHaveBeenCalled(); + expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledWith( + { + toolName: "read", + params: { mode: "safe" }, result, }, { toolName: "read" }, diff --git a/src/agents/pi-tool-definition-adapter.ts b/src/agents/pi-tool-definition-adapter.ts index 3d6cceedf73..ee02c2f9045 100644 --- a/src/agents/pi-tool-definition-adapter.ts +++ b/src/agents/pi-tool-definition-adapter.ts @@ -8,7 +8,11 @@ import type { ClientToolDefinition } from "./pi-embedded-runner/run/params.js"; import { logDebug, logError } from "../logger.js"; import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import { isPlainObject } from "../utils.js"; -import { runBeforeToolCallHook } from "./pi-tools.before-tool-call.js"; +import { + consumeAdjustedParamsForToolCall, + isToolWrappedWithBeforeToolCallHook, + runBeforeToolCallHook, +} from "./pi-tools.before-tool-call.js"; import { normalizeToolName } from "./tool-policy.js"; import { jsonResult } from "./tools/common.js"; @@ -83,6 +87,7 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] { return tools.map((tool) => { const name = tool.name || "tool"; const normalizedName = normalizeToolName(name); + const beforeHookWrapped = isToolWrappedWithBeforeToolCallHook(tool); return { name, label: tool.label ?? name, @@ -90,12 +95,23 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] { parameters: tool.parameters, execute: async (...args: ToolExecuteArgs): Promise> => { const { toolCallId, params, onUpdate, signal } = splitToolExecuteArgs(args); + let executeParams = params; try { - // NOTE: before_tool_call hook is NOT called here — it is already - // invoked by wrapToolWithBeforeToolCallHook (applied in pi-tools.ts) - // before the tool reaches toToolDefinitions. Calling it again would - // fire the hook twice per invocation (#15502). - const result = await tool.execute(toolCallId, params, signal, onUpdate); + if (!beforeHookWrapped) { + const hookOutcome = await runBeforeToolCallHook({ + toolName: name, + params, + toolCallId, + }); + if (hookOutcome.blocked) { + throw new Error(hookOutcome.reason); + } + executeParams = hookOutcome.params; + } + const result = await tool.execute(toolCallId, executeParams, signal, onUpdate); + const afterParams = beforeHookWrapped + ? (consumeAdjustedParamsForToolCall(toolCallId) ?? executeParams) + : executeParams; // Call after_tool_call hook const hookRunner = getGlobalHookRunner(); @@ -104,7 +120,7 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] { await hookRunner.runAfterToolCall( { toolName: name, - params: isPlainObject(params) ? params : {}, + params: isPlainObject(afterParams) ? afterParams : {}, result, }, { toolName: name }, @@ -128,6 +144,9 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] { if (name === "AbortError") { throw err; } + if (beforeHookWrapped) { + consumeAdjustedParamsForToolCall(toolCallId); + } const described = describeToolExecutionError(err); if (described.stack && described.stack !== described.message) { logDebug(`tools: ${normalizedName} failed stack:\n${described.stack}`); diff --git a/src/agents/pi-tools.before-tool-call.ts b/src/agents/pi-tools.before-tool-call.ts index aeca0af7540..26761f3127f 100644 --- a/src/agents/pi-tools.before-tool-call.ts +++ b/src/agents/pi-tools.before-tool-call.ts @@ -12,6 +12,9 @@ type HookContext = { type HookOutcome = { blocked: true; reason: string } | { blocked: false; params: unknown }; const log = createSubsystemLogger("agents/tools"); +const BEFORE_TOOL_CALL_WRAPPED = Symbol("beforeToolCallWrapped"); +const adjustedParamsByToolCallId = new Map(); +const MAX_TRACKED_ADJUSTED_PARAMS = 1024; export async function runBeforeToolCallHook(args: { toolName: string; @@ -71,7 +74,7 @@ export function wrapToolWithBeforeToolCallHook( return tool; } const toolName = tool.name || "tool"; - return { + const wrappedTool: AnyAgentTool = { ...tool, execute: async (toolCallId, params, signal, onUpdate) => { const outcome = await runBeforeToolCallHook({ @@ -83,12 +86,39 @@ export function wrapToolWithBeforeToolCallHook( if (outcome.blocked) { throw new Error(outcome.reason); } + if (toolCallId) { + adjustedParamsByToolCallId.set(toolCallId, outcome.params); + if (adjustedParamsByToolCallId.size > MAX_TRACKED_ADJUSTED_PARAMS) { + const oldest = adjustedParamsByToolCallId.keys().next().value; + if (oldest) { + adjustedParamsByToolCallId.delete(oldest); + } + } + } return await execute(toolCallId, outcome.params, signal, onUpdate); }, }; + Object.defineProperty(wrappedTool, BEFORE_TOOL_CALL_WRAPPED, { + value: true, + enumerable: false, + }); + return wrappedTool; +} + +export function isToolWrappedWithBeforeToolCallHook(tool: AnyAgentTool): boolean { + const taggedTool = tool as unknown as Record; + return taggedTool[BEFORE_TOOL_CALL_WRAPPED] === true; +} + +export function consumeAdjustedParamsForToolCall(toolCallId: string): unknown { + const params = adjustedParamsByToolCallId.get(toolCallId); + adjustedParamsByToolCallId.delete(toolCallId); + return params; } export const __testing = { + BEFORE_TOOL_CALL_WRAPPED, + adjustedParamsByToolCallId, runBeforeToolCallHook, isPlainObject, }; diff --git a/src/plugins/wired-hooks-after-tool-call.e2e.test.ts b/src/plugins/wired-hooks-after-tool-call.e2e.test.ts index 0256f6f3b62..cddbf3b42ba 100644 --- a/src/plugins/wired-hooks-after-tool-call.e2e.test.ts +++ b/src/plugins/wired-hooks-after-tool-call.e2e.test.ts @@ -6,6 +6,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const hookMocks = vi.hoisted(() => ({ runner: { hasHooks: vi.fn(() => false), + runBeforeToolCall: vi.fn(async () => {}), runAfterToolCall: vi.fn(async () => {}), }, })); @@ -23,6 +24,8 @@ describe("after_tool_call hook wiring", () => { beforeEach(() => { hookMocks.runner.hasHooks.mockReset(); hookMocks.runner.hasHooks.mockReturnValue(false); + hookMocks.runner.runBeforeToolCall.mockReset(); + hookMocks.runner.runBeforeToolCall.mockResolvedValue(undefined); hookMocks.runner.runAfterToolCall.mockReset(); hookMocks.runner.runAfterToolCall.mockResolvedValue(undefined); }); @@ -84,6 +87,7 @@ describe("after_tool_call hook wiring", () => { ); expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledTimes(1); + expect(hookMocks.runner.runBeforeToolCall).not.toHaveBeenCalled(); const [event, context] = hookMocks.runner.runAfterToolCall.mock.calls[0]; expect(event.toolName).toBe("read");