Skip to content

fix(hooks): fire before_tool_call hook exactly once per tool invocation (#15502)#15635

Merged
steipete merged 2 commits intoopenclaw:mainfrom
lailoo:fix/before-tool-call-hook-dedup-15502
Feb 14, 2026
Merged

fix(hooks): fire before_tool_call hook exactly once per tool invocation (#15502)#15635
steipete merged 2 commits intoopenclaw:mainfrom
lailoo:fix/before-tool-call-hook-dedup-15502

Conversation

@lailoo
Copy link

@lailoo lailoo commented Feb 13, 2026

Summary

Remove the duplicate runBeforeToolCallHook call from toToolDefinitions so the before_tool_call hook fires exactly once per tool invocation instead of 2–3 times.

Root Cause

Tools go through two layers that both independently call runBeforeToolCallHook:

  1. wrapToolWithBeforeToolCallHook (pi-tools.ts:460) — wraps tool.execute with hook call, includes agentId/sessionKey context
  2. toToolDefinitions (pi-tool-definition-adapter.ts:95) — wraps again with another hook call, but without context

Since tool-split.ts:15 calls toToolDefinitions(tools) on tools that are already wrapped by step 1, the hook fires twice per invocation.

Fix

Remove the runBeforeToolCallHook call from toToolDefinitions (layer 2). The hook is already invoked by wrapToolWithBeforeToolCallHook (layer 1) which has the full agentId/sessionKey context.

toClientToolDefinitions is not changed — client tools don't go through wrapToolWithBeforeToolCallHook, so they correctly call the hook directly.

  execute: async (...args) => {
    const { toolCallId, params, onUpdate, signal } = splitToolExecuteArgs(args);
    try {
-     const hookOutcome = await runBeforeToolCallHook({ toolName, params, toolCallId });
-     if (hookOutcome.blocked) { throw new Error(hookOutcome.reason); }
-     const adjustedParams = hookOutcome.params;
-     const result = await tool.execute(toolCallId, adjustedParams, signal, onUpdate);
+     // before_tool_call hook is already invoked by wrapToolWithBeforeToolCallHook
+     // (applied in pi-tools.ts) before tools reach toToolDefinitions (#15502).
+     const result = await tool.execute(toolCallId, params, signal, onUpdate);

Before (on main — real environment, zero mocks)

Registered a real before_tool_call hook via initializeGlobalHookRunner with a real PluginRegistry, then executed a tool through the full wrapToolWithBeforeToolCallHooktoToolDefinitionsrunBeforeToolCallHookcreateHookRunner.runBeforeToolCall pipeline:

Hook fired 2 times:
  - toolName=web_fetch, params={"url":"https://example.com"}
  - toolName=web_fetch, params={"url":"https://example.com"}
❌ BUG CONFIRMED: real before_tool_call hook handler fired 2 times (expected 1)

After (on fix branch)

✓ fires hook exactly once when tool goes through wrap + toToolDefinitions
✓ dispatches after_tool_call once on successful adapter execution
✓ passes modified params to client tool callbacks

Test Files  3 passed (3)
     Tests  12 passed (12)

Changes

  • src/agents/pi-tool-definition-adapter.ts: remove duplicate runBeforeToolCallHook call from toToolDefinitions
  • src/agents/pi-tools.before-tool-call.e2e.test.ts: add regression test verifying hook fires exactly once through the full wrap + toToolDefinitions pipeline
  • src/agents/pi-tool-definition-adapter.after-tool-call.e2e.test.ts: update after_tool_call test to reflect that params are no longer adjusted inside toToolDefinitions
  • CHANGELOG.md: add entry

Closes #15502

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 13, 2026

Additional Comments (1)

src/agents/pi-tool-definition-adapter.ts
Missing hook enforcement

toToolDefinitions no longer calls runBeforeToolCallHook, so any tools that reach this adapter without being wrapped by wrapToolWithBeforeToolCallHook will silently skip before_tool_call semantics (blocking and param adjustment). This is a functional/security behavior change that depends on all toToolDefinitions(...) inputs being pre-wrapped; splitSdkTools currently calls toToolDefinitions(tools) directly (src/agents/pi-embedded-runner/tool-split.ts:15). Please ensure all call paths constructing that tools array always apply wrapToolWithBeforeToolCallHook (or restore hook invocation here for unwrapped tools).

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tool-definition-adapter.ts
Line: 82:85

Comment:
**Missing hook enforcement**

`toToolDefinitions` no longer calls `runBeforeToolCallHook`, so any tools that reach this adapter *without* being wrapped by `wrapToolWithBeforeToolCallHook` will silently skip `before_tool_call` semantics (blocking and param adjustment). This is a functional/security behavior change that depends on all `toToolDefinitions(...)` inputs being pre-wrapped; `splitSdkTools` currently calls `toToolDefinitions(tools)` directly (`src/agents/pi-embedded-runner/tool-split.ts:15`). Please ensure all call paths constructing that `tools` array always apply `wrapToolWithBeforeToolCallHook` (or restore hook invocation here for unwrapped tools).

How can I resolve this? If you propose a fix, please make it concise.

@steipete steipete force-pushed the fix/before-tool-call-hook-dedup-15502 branch from 7374a0a to 21c7b02 Compare February 14, 2026 01:49
steipete added a commit to lailoo/openclaw that referenced this pull request Feb 14, 2026
@steipete steipete force-pushed the fix/before-tool-call-hook-dedup-15502 branch from 21c7b02 to ea41da6 Compare February 14, 2026 01:50
@steipete steipete merged commit 8c3cc79 into openclaw:main Feb 14, 2026
6 checks passed
@steipete
Copy link
Contributor

Landed via temp rebase onto main.

  • Gate:
    • pnpm check
    • pnpm build
    • pnpm test
    • pnpm test:e2e (17 unrelated baseline failures on latest main)
    • pnpm check:docs
  • Land commit: 8c3cc79
  • Merge commit: 8c3cc79

Thanks @lailoo!

mverrilli pushed a commit to mverrilli/openclaw that referenced this pull request Feb 14, 2026
GwonHyeok pushed a commit to learners-superpumped/openclaw that referenced this pull request Feb 15, 2026
jiulingyun added a commit to jiulingyun/openclaw-cn that referenced this pull request Feb 15, 2026
hughdidit pushed a commit to hughdidit/DAISy-Agency that referenced this pull request Mar 1, 2026
…anks @lailoo)

(cherry picked from commit 8c3cc79)

# Conflicts:
#	CHANGELOG.md
#	src/agents/pi-embedded-subscribe.handlers.tools.ts
#	src/agents/pi-tool-definition-adapter.after-tool-call.test.ts
#	src/agents/pi-tool-definition-adapter.ts
#	src/plugins/wired-hooks-after-tool-call.test.ts
hughdidit pushed a commit to hughdidit/DAISy-Agency that referenced this pull request Mar 3, 2026
…anks @lailoo)

(cherry picked from commit 8c3cc79)

# Conflicts:
#	CHANGELOG.md
#	src/agents/pi-embedded-subscribe.handlers.tools.ts
#	src/agents/pi-tool-definition-adapter.after-tool-call.test.ts
#	src/agents/pi-tool-definition-adapter.ts
#	src/plugins/wired-hooks-after-tool-call.test.ts
zooqueen pushed a commit to hanzoai/bot that referenced this pull request Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: before_tool_call hook fires 2–3 times per tool invocation

2 participants