mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-28 05:16:23 -06:00
fix(agents): prevent duplicate before-tool-call hooks (#93009)
Prevent duplicate `before_tool_call` execution when an already wrapped tool passes through schema normalization and coding-tool assembly. Preserve the normalized schema while replacing stale wrapper context with the current agent/session/run context. Fixes #92973. Co-authored-by: zengLingbiao <zeng.lingbiao@xydigit.com>
This commit is contained in:
@@ -1357,11 +1357,20 @@ export function rewrapToolWithBeforeToolCallHook(
|
||||
wrappedContext && typeof wrappedContext === "object"
|
||||
? (wrappedContext as HookContext)
|
||||
: undefined;
|
||||
return wrapToolWithBeforeToolCallHook(
|
||||
source && typeof source === "object" ? (source as AnyAgentTool) : tool,
|
||||
ctx ?? preservedContext,
|
||||
options,
|
||||
);
|
||||
const sourceTool = source && typeof source === "object" ? (source as AnyAgentTool) : tool;
|
||||
if (sourceTool === tool) {
|
||||
return wrapToolWithBeforeToolCallHook(tool, ctx ?? preservedContext, options);
|
||||
}
|
||||
// Keep schema and metadata replacements applied after the original wrap while
|
||||
// restoring the unwrapped execute function for the new hook context.
|
||||
const rewrapSource: AnyAgentTool = {
|
||||
...tool,
|
||||
execute: sourceTool.execute,
|
||||
};
|
||||
delete (rewrapSource as unknown as Record<symbol, unknown>)[BEFORE_TOOL_CALL_WRAPPED];
|
||||
copyPluginToolMeta(tool, rewrapSource);
|
||||
copyChannelAgentToolMeta(tool as never, rewrapSource as never);
|
||||
return wrapToolWithBeforeToolCallHook(rewrapSource, ctx ?? preservedContext, options);
|
||||
}
|
||||
|
||||
/** Copy before_tool_call marker metadata when another wrapper replaces a tool. */
|
||||
|
||||
@@ -20,6 +20,7 @@ import { createMockPluginRegistry } from "../plugins/hooks.test-helpers.js";
|
||||
import "./test-helpers/fast-bash-tools.js";
|
||||
import "./test-helpers/fast-coding-tools.js";
|
||||
import "./test-helpers/fast-openclaw-tools.js";
|
||||
import { wrapToolWithBeforeToolCallHook } from "./agent-tools.before-tool-call.js";
|
||||
import { createOpenClawCodingTools } from "./agent-tools.js";
|
||||
import type { AuthProfileStore } from "./auth-profiles/types.js";
|
||||
import * as openClawPluginTools from "./openclaw-plugin-tools.js";
|
||||
@@ -213,6 +214,36 @@ describe("createOpenClawCodingTools", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("re-wraps existing before_tool_call hooks once with the current context", async () => {
|
||||
const beforeToolCall = vi.fn();
|
||||
initializeGlobalHookRunner(
|
||||
createMockPluginRegistry([{ hookName: "before_tool_call", handler: beforeToolCall }]),
|
||||
);
|
||||
const execute = vi.fn().mockResolvedValue({ content: [], details: { ok: true } });
|
||||
const wrapped = wrapToolWithBeforeToolCallHook(
|
||||
{
|
||||
name: "already_wrapped",
|
||||
label: "Already wrapped",
|
||||
description: "Already wrapped tool",
|
||||
parameters: {},
|
||||
execute,
|
||||
},
|
||||
{ agentId: "main", sessionId: "session-original" },
|
||||
);
|
||||
vi.mocked(createOpenClawTools).mockReturnValueOnce([wrapped as never]);
|
||||
|
||||
const tools = createOpenClawCodingTools({ agentId: "main", sessionId: "session-new" });
|
||||
const tool = requireTool(tools, "already_wrapped");
|
||||
await requireToolExecute(tool)("call-wrapped", {});
|
||||
|
||||
expect(beforeToolCall).toHaveBeenCalledTimes(1);
|
||||
expect(beforeToolCall.mock.calls[0]?.[1]).toEqual(
|
||||
expect.objectContaining({ agentId: "main", sessionId: "session-new" }),
|
||||
);
|
||||
expect(execute).toHaveBeenCalledTimes(1);
|
||||
expect(tool.parameters).toEqual({ type: "object", properties: {} });
|
||||
});
|
||||
|
||||
it("adds Tool Search control tools when explicitly requested", () => {
|
||||
const tools = createOpenClawCodingTools({
|
||||
includeToolSearchControls: true,
|
||||
|
||||
@@ -7,7 +7,11 @@ import { runAgentLoop, type AgentEvent, type StreamFn } from "openclaw/plugin-sd
|
||||
import { createAssistantMessageEventStream, validateToolArguments } from "openclaw/plugin-sdk/llm";
|
||||
import { Type, type TSchema } from "typebox";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { wrapToolWithBeforeToolCallHook } from "./agent-tools.before-tool-call.js";
|
||||
import {
|
||||
isToolWrappedWithBeforeToolCallHook,
|
||||
testing as beforeToolCallTesting,
|
||||
wrapToolWithBeforeToolCallHook,
|
||||
} from "./agent-tools.before-tool-call.js";
|
||||
import {
|
||||
cleanToolSchemaForGemini,
|
||||
normalizeToolParameterSchema,
|
||||
@@ -655,6 +659,19 @@ function makeTool(parameters: TSchema): AnyAgentTool {
|
||||
}
|
||||
|
||||
describe("normalizeToolParameters", () => {
|
||||
it("preserves before_tool_call wrapper metadata", () => {
|
||||
const source = makeTool(Type.Object({ value: Type.String() }));
|
||||
const hookContext = { agentId: "main", sessionId: "session-before-normalize" };
|
||||
const wrapped = wrapToolWithBeforeToolCallHook(source, hookContext);
|
||||
|
||||
const normalized = normalizeToolParameters(wrapped);
|
||||
const tagged = normalized as unknown as Record<symbol, unknown>;
|
||||
|
||||
expect(isToolWrappedWithBeforeToolCallHook(normalized)).toBe(true);
|
||||
expect(tagged[beforeToolCallTesting.BEFORE_TOOL_CALL_SOURCE_TOOL]).toBe(source);
|
||||
expect(tagged[beforeToolCallTesting.BEFORE_TOOL_CALL_HOOK_CONTEXT]).toBe(hookContext);
|
||||
});
|
||||
|
||||
it("normalizes truly empty schemas to type:object with properties:{} (MCP parameter-free tools)", () => {
|
||||
const tool: AnyAgentTool = {
|
||||
name: "get_flux_instance",
|
||||
|
||||
@@ -8,6 +8,7 @@ import {
|
||||
normalizeToolParameterSchema,
|
||||
type ToolParameterSchemaOptions,
|
||||
} from "./agent-tools-parameter-schema.js";
|
||||
import { copyBeforeToolCallHookMarker } from "./agent-tools.before-tool-call.js";
|
||||
import type { AnyAgentTool } from "./agent-tools.types.js";
|
||||
import { copyChannelAgentToolMeta } from "./channel-tools.js";
|
||||
|
||||
@@ -72,6 +73,7 @@ export function normalizeToolParameters(
|
||||
function preserveToolMeta(target: AnyAgentTool): AnyAgentTool {
|
||||
copyPluginToolMeta(tool, target);
|
||||
copyChannelAgentToolMeta(tool as never, target as never);
|
||||
copyBeforeToolCallHookMarker(tool, target);
|
||||
return target;
|
||||
}
|
||||
const schema =
|
||||
|
||||
+23
-21
@@ -31,6 +31,8 @@ import { resolveGatewayMessageChannel } from "../utils/message-channel.js";
|
||||
import { resolveAgentConfig } from "./agent-scope.js";
|
||||
import { wrapToolWithAbortSignal } from "./agent-tools.abort.js";
|
||||
import {
|
||||
isToolWrappedWithBeforeToolCallHook,
|
||||
rewrapToolWithBeforeToolCallHook,
|
||||
type ToolOutcomeObserver,
|
||||
wrapToolWithBeforeToolCallHook,
|
||||
} from "./agent-tools.before-tool-call.js";
|
||||
@@ -1173,28 +1175,28 @@ export function createOpenClawCodingTools(options?: {
|
||||
}),
|
||||
);
|
||||
options?.recordToolPrepStage?.("schema-normalization");
|
||||
const hookContext = {
|
||||
agentId,
|
||||
...(options?.config ? { config: options.config } : {}),
|
||||
cwd: codingRoot,
|
||||
workspaceDir: workspaceRoot,
|
||||
...(options?.skillsSnapshot ? { skillsSnapshot: options.skillsSnapshot } : {}),
|
||||
...(sandboxRoot && allowWorkspaceWrites
|
||||
? { sandbox: { root: sandboxRoot, bridge: sandboxFsBridge! } }
|
||||
: {}),
|
||||
sessionKey: options?.sessionKey,
|
||||
sessionId: options?.sessionId,
|
||||
runId: options?.runId,
|
||||
channelId: options?.hookChannelId ?? options?.currentChannelId,
|
||||
...(options?.trace ? { trace: options.trace } : {}),
|
||||
loopDetection: resolveToolLoopDetectionConfig({ cfg: options?.config, agentId }),
|
||||
onToolOutcome: options?.onToolOutcome,
|
||||
};
|
||||
const hookOptions = { emitDiagnostics: options?.emitBeforeToolCallDiagnostics };
|
||||
const withHooks = normalized.map((tool) =>
|
||||
wrapToolWithBeforeToolCallHook(
|
||||
tool,
|
||||
{
|
||||
agentId,
|
||||
...(options?.config ? { config: options.config } : {}),
|
||||
cwd: codingRoot,
|
||||
workspaceDir: workspaceRoot,
|
||||
...(options?.skillsSnapshot ? { skillsSnapshot: options.skillsSnapshot } : {}),
|
||||
...(sandboxRoot && allowWorkspaceWrites
|
||||
? { sandbox: { root: sandboxRoot, bridge: sandboxFsBridge! } }
|
||||
: {}),
|
||||
sessionKey: options?.sessionKey,
|
||||
sessionId: options?.sessionId,
|
||||
runId: options?.runId,
|
||||
channelId: options?.hookChannelId ?? options?.currentChannelId,
|
||||
...(options?.trace ? { trace: options.trace } : {}),
|
||||
loopDetection: resolveToolLoopDetectionConfig({ cfg: options?.config, agentId }),
|
||||
onToolOutcome: options?.onToolOutcome,
|
||||
},
|
||||
{ emitDiagnostics: options?.emitBeforeToolCallDiagnostics },
|
||||
),
|
||||
isToolWrappedWithBeforeToolCallHook(tool)
|
||||
? rewrapToolWithBeforeToolCallHook(tool, hookContext, hookOptions)
|
||||
: wrapToolWithBeforeToolCallHook(tool, hookContext, hookOptions),
|
||||
);
|
||||
options?.recordToolPrepStage?.("tool-hooks");
|
||||
const withAbort = options?.abortSignal
|
||||
|
||||
@@ -131,6 +131,9 @@ vi.mock("../apply-patch.js", () => ({
|
||||
}));
|
||||
|
||||
vi.mock("../agent-tools.before-tool-call.js", () => ({
|
||||
copyBeforeToolCallHookMarker: vi.fn(),
|
||||
isToolWrappedWithBeforeToolCallHook: vi.fn(() => false),
|
||||
rewrapToolWithBeforeToolCallHook: vi.fn((tool) => tool),
|
||||
wrapToolWithBeforeToolCallHook: vi.fn((tool) => tool),
|
||||
}));
|
||||
|
||||
|
||||
Reference in New Issue
Block a user