fix(hooks): deduplicate before_tool_call hook in toToolDefinitions (#15502)

This commit is contained in:
damaozi
2026-02-14 01:44:13 +08:00
committed by Peter Steinberger
parent b4430c126a
commit 534e4213a1
4 changed files with 50 additions and 18 deletions

View File

@@ -27,6 +27,7 @@ Docs: https://docs.openclaw.ai
- Discord: route autoThread replies to existing threads instead of the root channel. (#8302) Thanks @gavinbmoore, @thewilloftheshadow. - 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. - 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) - 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.
- 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. - 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. - 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. - 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.

View File

@@ -35,10 +35,6 @@ describe("pi tool definition adapter after_tool_call", () => {
it("dispatches after_tool_call once on successful adapter execution", async () => { it("dispatches after_tool_call once on successful adapter execution", async () => {
hookMocks.runner.hasHooks.mockImplementation((name: string) => name === "after_tool_call"); hookMocks.runner.hasHooks.mockImplementation((name: string) => name === "after_tool_call");
hookMocks.runBeforeToolCallHook.mockResolvedValue({
blocked: false,
params: { mode: "safe" },
});
const tool = { const tool = {
name: "read", name: "read",
label: "Read", label: "Read",
@@ -52,10 +48,13 @@ describe("pi tool definition adapter after_tool_call", () => {
expect(result.details).toMatchObject({ ok: true }); expect(result.details).toMatchObject({ ok: true });
expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledTimes(1); 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( expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledWith(
{ {
toolName: "read", toolName: "read",
params: { mode: "safe" }, params: { path: "/tmp/file" },
result, result,
}, },
{ toolName: "read" }, { toolName: "read" },

View File

@@ -91,17 +91,11 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] {
execute: async (...args: ToolExecuteArgs): Promise<AgentToolResult<unknown>> => { execute: async (...args: ToolExecuteArgs): Promise<AgentToolResult<unknown>> => {
const { toolCallId, params, onUpdate, signal } = splitToolExecuteArgs(args); const { toolCallId, params, onUpdate, signal } = splitToolExecuteArgs(args);
try { try {
// Call before_tool_call hook // NOTE: before_tool_call hook is NOT called here — it is already
const hookOutcome = await runBeforeToolCallHook({ // invoked by wrapToolWithBeforeToolCallHook (applied in pi-tools.ts)
toolName: name, // before the tool reaches toToolDefinitions. Calling it again would
params, // fire the hook twice per invocation (#15502).
toolCallId, const result = await tool.execute(toolCallId, params, signal, onUpdate);
});
if (hookOutcome.blocked) {
throw new Error(hookOutcome.reason);
}
const adjustedParams = hookOutcome.params;
const result = await tool.execute(toolCallId, adjustedParams, signal, onUpdate);
// Call after_tool_call hook // Call after_tool_call hook
const hookRunner = getGlobalHookRunner(); const hookRunner = getGlobalHookRunner();
@@ -110,7 +104,7 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] {
await hookRunner.runAfterToolCall( await hookRunner.runAfterToolCall(
{ {
toolName: name, toolName: name,
params: isPlainObject(adjustedParams) ? adjustedParams : {}, params: isPlainObject(params) ? params : {},
result, result,
}, },
{ toolName: name }, { toolName: name },

View File

@@ -1,6 +1,6 @@
import { beforeEach, describe, expect, it, vi } from "vitest"; import { beforeEach, describe, expect, it, vi } from "vitest";
import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import { getGlobalHookRunner } from "../plugins/hook-runner-global.js";
import { toClientToolDefinitions } from "./pi-tool-definition-adapter.js"; import { toClientToolDefinitions, toToolDefinitions } from "./pi-tool-definition-adapter.js";
import { wrapToolWithBeforeToolCallHook } from "./pi-tools.before-tool-call.js"; import { wrapToolWithBeforeToolCallHook } from "./pi-tools.before-tool-call.js";
vi.mock("../plugins/hook-runner-global.js"); vi.mock("../plugins/hook-runner-global.js");
@@ -108,6 +108,44 @@ describe("before_tool_call hook integration", () => {
}); });
}); });
describe("before_tool_call hook deduplication (#15502)", () => {
let hookRunner: {
hasHooks: ReturnType<typeof vi.fn>;
runBeforeToolCall: ReturnType<typeof vi.fn>;
};
beforeEach(() => {
hookRunner = {
hasHooks: vi.fn(() => true),
runBeforeToolCall: vi.fn(async () => undefined),
};
// oxlint-disable-next-line typescript/no-explicit-any
mockGetGlobalHookRunner.mockReturnValue(hookRunner as any);
});
it("fires hook exactly once when tool goes through wrap + toToolDefinitions", async () => {
const execute = vi.fn().mockResolvedValue({ content: [], details: { ok: true } });
// oxlint-disable-next-line typescript/no-explicit-any
const baseTool = { name: "web_fetch", execute, description: "fetch", parameters: {} } as any;
const wrapped = wrapToolWithBeforeToolCallHook(baseTool, {
agentId: "main",
sessionKey: "main",
});
const [def] = toToolDefinitions([wrapped]);
await def.execute(
"call-dedup",
{ url: "https://example.com" },
undefined,
undefined,
undefined,
);
expect(hookRunner.runBeforeToolCall).toHaveBeenCalledTimes(1);
});
});
describe("before_tool_call hook integration for client tools", () => { describe("before_tool_call hook integration for client tools", () => {
let hookRunner: { let hookRunner: {
hasHooks: ReturnType<typeof vi.fn>; hasHooks: ReturnType<typeof vi.fn>;