mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-19 09:18:38 +00:00
fix: before_tool_call hook still double-fires when abort signal is present
The BEFORE_TOOL_CALL_WRAPPED Symbol is set with enumerable: false, but
wrapToolWithAbortSignal spreads the tool object ({ ...tool }) which only
copies enumerable properties — stripping the Symbol. This causes
toToolDefinitions to not recognize the tool as already wrapped, firing
the hook a second time.
Since every real agent session has an abort signal, this affects all
tool calls in practice.
Fix: change enumerable: false to enumerable: true so the Symbol
survives object spread in wrapToolWithAbortSignal.
Added e2e test that exercises the full wrap → abort → toToolDefinitions
chain and asserts the hook fires exactly once.
This commit is contained in:
committed by
Ayaan Zaidi
parent
583844ecf6
commit
194720827e
@@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest";
|
|||||||
import { resetDiagnosticSessionStateForTest } from "../logging/diagnostic-session-state.js";
|
import { resetDiagnosticSessionStateForTest } from "../logging/diagnostic-session-state.js";
|
||||||
import { getGlobalHookRunner } from "../plugins/hook-runner-global.js";
|
import { getGlobalHookRunner } from "../plugins/hook-runner-global.js";
|
||||||
import { toClientToolDefinitions, toToolDefinitions } from "./pi-tool-definition-adapter.js";
|
import { toClientToolDefinitions, toToolDefinitions } from "./pi-tool-definition-adapter.js";
|
||||||
|
import { wrapToolWithAbortSignal } from "./pi-tools.abort.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");
|
||||||
@@ -162,6 +163,24 @@ describe("before_tool_call hook deduplication (#15502)", () => {
|
|||||||
|
|
||||||
expect(hookRunner.runBeforeToolCall).toHaveBeenCalledTimes(1);
|
expect(hookRunner.runBeforeToolCall).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("fires hook exactly once when tool goes through wrap + abort + toToolDefinitions", async () => {
|
||||||
|
const execute = vi.fn().mockResolvedValue({ content: [], details: { ok: true } });
|
||||||
|
// oxlint-disable-next-line typescript/no-explicit-any
|
||||||
|
const baseTool = { name: "Bash", execute, description: "bash", parameters: {} } as any;
|
||||||
|
|
||||||
|
const abortController = new AbortController();
|
||||||
|
const wrapped = wrapToolWithBeforeToolCallHook(baseTool, {
|
||||||
|
agentId: "main",
|
||||||
|
sessionKey: "main",
|
||||||
|
});
|
||||||
|
const withAbort = wrapToolWithAbortSignal(wrapped, abortController.signal);
|
||||||
|
const [def] = toToolDefinitions([withAbort]);
|
||||||
|
|
||||||
|
await def.execute("call-abort-dedup", { command: "ls" }, 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", () => {
|
||||||
|
|||||||
@@ -227,7 +227,7 @@ export function wrapToolWithBeforeToolCallHook(
|
|||||||
};
|
};
|
||||||
Object.defineProperty(wrappedTool, BEFORE_TOOL_CALL_WRAPPED, {
|
Object.defineProperty(wrappedTool, BEFORE_TOOL_CALL_WRAPPED, {
|
||||||
value: true,
|
value: true,
|
||||||
enumerable: false,
|
enumerable: true,
|
||||||
});
|
});
|
||||||
return wrappedTool;
|
return wrappedTool;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user