fix(agents): make wrapToolWithBeforeToolCallHook idempotent to prevent double hook execution (fixes #92973) by zenglingbiao · Pull Request #93009 · openclaw/openclaw

zenglingbiao · 2026-06-14T15:45:36Z

Summary

Preserve before_tool_call wrapper provenance through tool schema normalization.
Replace an existing hook wrapper with exactly one wrapper using the current agent/session/run/sandbox context.
Retain the normalized tool schema and argument-preparation behavior during rewrap, protecting OpenAI/Anthropic tool definitions from reverting to the original unnormalized schema.
Fixes before_tool_call hook fires twice: tools double-wrapped after normalizeToolParameters strips the wrap marker #92973.

Verification

node scripts/run-vitest.mjs src/agents/agent-tools.create-openclaw-coding-tools.test.ts src/agents/agent-tools.schema.test.ts src/agents/agent-tools.before-tool-call.integration.e2e.test.ts src/agents/agent-tools.before-tool-call.e2e.test.ts src/agents/agent-tool-definition-adapter.test.ts src/agents/tool-search.test.ts src/mcp/plugin-tools-handlers.cancel.test.ts
- 242 tests passed across unit, agents, and E2E shards.
Production-path Node/tsx probe using createOpenClawCodingTools, a real active channel registry, and one deliberately prewrapped tool:
- openai: one hook call, one execution, current session/run context, normalized empty-object schema retained.
- anthropic: one hook call, one execution, current session/run context, normalized empty-object schema retained.
Testbox pnpm check:changed: tbx_01kv3n790s40ertvxzv2xsbbzf passed.
Full agents-tools Testbox shard: tbx_01kv3nqsg2yc40gszdpscnjs7m passed, 53 files and 951 tests.
Fresh autoreview: clean, no accepted/actionable findings.
Exact-head GitHub CI: 95 successful checks, 1 neutral, 3 skipped, no failures.

Real behavior proof

Behavior addressed: An already wrapped tool passing through normalizeToolParameters and createOpenClawCodingTools could acquire a nested before_tool_call wrapper, causing duplicate policy/plugin hook execution.

Real environment tested: macOS host, Node with tsx, production OpenClaw tool assembly and plugin registry paths; Linux Testbox for changed-surface checks.

Exact steps or command run after this patch: Ran a Node/tsx production-path probe that registered a channel tool already wrapped with stale context, assembled it once with modelProvider: "openai" and once with modelProvider: "anthropic", then executed each resulting tool.

Evidence after fix: Copied live console output from the production-path probe:

{"providers":["openai","anthropic"],"executeCalls":2,"hookCalls":2,"contexts":[{"agentId":"main","sessionId":"live-openai","runId":"run-openai"},{"agentId":"main","sessionId":"live-anthropic","runId":"run-anthropic"}]}

Observed result after fix: Each provider-policy path executed the underlying tool once, fired the hook once, adopted the current run context, and retained { "type": "object", "properties": {} } after normalization.

What was not tested: No external model API request was needed because this change does not touch provider serialization or transport. Official OpenAI and Anthropic tool schema paths were exercised through production assembly; direct provider tool calls remain covered by unchanged provider code and CI.

clawsweeper · 2026-06-14T15:47:20Z

Codex review: needs real behavior proof before merge. Reviewed June 14, 2026, 12:27 PM ET / 16:27 UTC.

Summary
The PR changes the before_tool_call wrapper and tool schema normalizer so already wrapped tools are rewrapped once with current context and Symbol metadata is copied through normalization, with focused regression tests.

PR surface: Source +21, Tests +86. Total +107 across 3 files.

Reproducibility: yes. Source inspection shows createOpenClawCodingTools normalizes tools for the active provider/model and then applies the before_tool_call wrapper, so an already wrapped tool must dedupe without losing the current normalized schema.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

[P1] Preserve normalized parameters in the rewrap path and add a schema-survival assertion.
Remove the unused baseTool variable or use it in the new test.
[P1] Add redacted real cron or isolated-agent plugin-hook output if feasible.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR body supplies targeted Vitest output, but no redacted real cron or isolated-agent output with an actual before_tool_call plugin; screenshots, terminal output, logs, recordings, or linked artifacts with private details redacted would satisfy the contributor proof gate. After adding proof, updating the PR body should trigger a fresh ClawSweeper review automatically; if it does not, a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

[P1] Already wrapped tools can still lose the active provider/model-normalized parameters when this PR rebuilds from the saved source tool.
[P1] The PR still lacks redacted real cron or isolated-agent output from an actual before_tool_call plugin, so the external-contributor real behavior proof gate remains unmet.
[P1] The changed path carries per-run/session/workspace context and approval/veto/audit hook behavior, so schema and context preservation need conservative review before merge.

Maintainer options:

Repair The Rewrap Target (recommended)
Change the already-wrapped branch to rebuild from the current normalized tool shape while replacing only stale wrapper context, and cover that with a parameter-schema assertion.
Accept The Remaining Boundary Risk
Maintainers could intentionally accept the source-tool rewrap behavior, but existing users may still hit provider-schema drift on already wrapped tools.
Leave The Linked Issue Canonical
If this branch cannot be narrowed, keep the linked issue as the fix target and replace this PR with a smaller schema-preserving repair.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Preserve provider/model-normalized `parameters` when `wrapToolWithBeforeToolCallHook` receives an already wrapped tool, remove the unused `baseTool` test variable, and add a focused regression asserting the normalized schema survives normalize -> rewrap while the hook fires once.

Next step before merge

[P1] The remaining blockers are narrow code/test repairs that automation can attempt on the PR branch, while contributor real-behavior proof remains a separate merge gate.

Security
Needs attention: The diff touches the before_tool_call approval/audit boundary, and the current rewrap target can still lose the active normalized tool schema on that path.

Review findings

[P1] Preserve normalized parameters when rewrapping — src/agents/agent-tools.before-tool-call.ts:1144
[P2] Remove the unused test variable — src/agents/agent-tools.before-tool-call.e2e.test.ts:435

Review details

Best possible solution:

Rebuild one current-context before_tool_call wrapper while preserving the current normalized tool shape, then remove the unused test variable and add coverage that asserts the final wrapper keeps normalized parameters.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows createOpenClawCodingTools normalizes tools for the active provider/model and then applies the before_tool_call wrapper, so an already wrapped tool must dedupe without losing the current normalized schema.

Is this the best way to solve the issue?

No. The PR fixes the stale-context direction but still rebuilds from the saved source tool instead of the normalized tool shape, so it is not yet the best safe fix.

Full review comments:

[P1] Preserve normalized parameters when rewrapping — src/agents/agent-tools.before-tool-call.ts:1144
This branch unwraps to the saved source tool and wraps that object, discarding the normalized tool object that createOpenClawCodingTools just produced. For already wrapped tools, the returned tool can lose the active provider/model-normalized parameters; rebuild from the current normalized shape while replacing only the stale wrapper context, and assert that schema survives normalize -> rewrap.
Confidence: 0.88
[P2] Remove the unused test variable — src/agents/agent-tools.before-tool-call.e2e.test.ts:435
The new normalized-wrapper test declares baseTool but still wraps a separate object literal, leaving the lint failure introduced by this PR. Use baseTool in the wrapper setup or delete it.
Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against aefd49909d41.

Label changes

Label justifications:

P2: The PR targets a normal-priority agents/plugin-hook correctness bug with duplicate hook side effects and limited blast radius.
merge-risk: 🚨 compatibility: The changed rewrap path can affect provider-normalized tool parameter schemas for already wrapped tools.
merge-risk: 🚨 session-state: The wrapper stores and replaces run/session/workspace context used during tool execution.
merge-risk: 🚨 security-boundary: before_tool_call hooks can approve, veto, audit, or enforce policy on tool calls.
rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body supplies targeted Vitest output, but no redacted real cron or isolated-agent output with an actual before_tool_call plugin; screenshots, terminal output, logs, recordings, or linked artifacts with private details redacted would satisfy the contributor proof gate. After adding proof, updating the PR body should trigger a fresh ClawSweeper review automatically; if it does not, a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence reviewed

PR surface:

Source +21, Tests +86. Total +107 across 3 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	2	22	1	+21
Tests	1	86	0	+86
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	3	108	1	+107

Security concerns:

[medium] Policy wrapper can still lose normalized tool schema — src/agents/agent-tools.before-tool-call.ts:1144
Rewrapping the saved source tool can bypass the just-normalized provider schema for already wrapped tools, which affects the tool object passed through the policy hook boundary.
Confidence: 0.72

Acceptance criteria:

[P1] node scripts/run-vitest.mjs src/agents/agent-tools.before-tool-call.e2e.test.ts src/agents/agent-tools.before-tool-call.integration.e2e.test.ts src/agents/agent-tools.before-tool-call.embedded-mode.test.ts src/agents/agent-tools.schema.test.ts.
[P1] Use the repository lint lane or the narrow lint output for the touched test file to confirm the unused variable is gone.

What I checked:

Root policy read: Root AGENTS.md was read fully and applied; it requires full-path PR review, proof review, and conservative handling of plugin/session/security-sensitive boundaries. (AGENTS.md:1, aefd49909d41)
Scoped agents policy read: The scoped agents guide was read and applied; it reinforces checking agent hot paths and test import/runtime boundaries for agents-path changes. (src/agents/AGENTS.md:1, aefd49909d41)
Current normalization and wrap path: Current main normalizes each tool with provider/model options, then immediately wraps the normalized tool with the active agent/session/run/workspace hook context. (src/agents/agent-tools.ts:1168, aefd49909d41)
Wrapper metadata contract: The wrapper stores the original source tool and hook context as non-enumerable Symbols, so using that saved source during rewrap can differ from the current normalized tool object. (src/agents/agent-tools.before-tool-call.ts:1321, aefd49909d41)
PR diff still rewraps the saved source: The latest PR head recursively wraps source as AnyAgentTool for already wrapped tools, which discards the normalized wrapper object just produced by normalizeToolParameters. (src/agents/agent-tools.before-tool-call.ts:1144, aa5b75f43a4d)
PR diff still leaves unused test variable: The latest PR head declares baseTool in the new normalized-wrapper test but wraps a separate object literal instead. (src/agents/agent-tools.before-tool-call.e2e.test.ts:435, aa5b75f43a4d)

Likely related people:

vincentkoc: Live GitHub history shows recent merged before_tool_call trusted-policy work in the same wrapper/policy boundary. (role: recent area contributor; confidence: medium; commits: be967545c500; files: src/agents/agent-tools.before-tool-call.ts)
steipete: Live path history links steipete to agent tool guard documentation and the broad agent-runtime internalization that touched the schema/wrapper area. (role: recent area contributor; confidence: medium; commits: c25800ccc1a6, bb46b79d3c14; files: src/agents/agent-tools.before-tool-call.ts, src/agents/agent-tools.schema.ts)
sreuter: Authored a prior merged fix for before_tool_call double firing caused by wrapper marker loss across another wrapping path. (role: prior marker-preservation fix author; confidence: medium; commits: 60dc3741c036; files: src/agents/agent-tools.before-tool-call.ts, src/agents/agent-tools.before-tool-call.integration.e2e.test.ts)
lailoo: Authored earlier before_tool_call single-fire work in the tool-definition adapter path, which shares the dedupe invariant. (role: prior dedupe fix contributor; confidence: medium; commits: 31b392683d8c; files: src/agents/agent-tool-definition-adapter.ts, src/agents/agent-tools.before-tool-call.e2e.test.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

zenglingbiao · 2026-06-14T15:59:08Z

@clawsweeper re-review

clawsweeper · 2026-06-14T15:59:11Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/27504285849
Updated: 2026-06-14T16:06:02.067Z

zenglingbiao · 2026-06-14T16:12:45Z

@clawsweeper re-review

clawsweeper · 2026-06-14T16:12:48Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/27504671828
Updated: 2026-06-14T16:28:06.524Z

Co-authored-by: zengLingbiao <[email protected]>

steipete · 2026-06-14T18:25:33Z

Land-ready verification for exact head fe1f2fa740afd7435e63bca03c29c57299b38917:

Focused regression/E2E command:
node scripts/run-vitest.mjs src/agents/agent-tools.create-openclaw-coding-tools.test.ts src/agents/agent-tools.schema.test.ts src/agents/agent-tools.before-tool-call.integration.e2e.test.ts src/agents/agent-tools.before-tool-call.e2e.test.ts src/agents/agent-tool-definition-adapter.test.ts src/agents/tool-search.test.ts src/mcp/plugin-tools-handlers.cancel.test.ts
Result: 242 tests passed.
CI regression follow-up:
node scripts/run-vitest.mjs src/agents/tools/image-tool.test.ts
Result: 94 tests passed.
Changed-surface Testbox: tbx_01kv3n790s40ertvxzv2xsbbzf, passed.
Full agents-tools Testbox: tbx_01kv3nqsg2yc40gszdpscnjs7m, 53 files and 951 tests passed.
Production-path Node/tsx probe used createOpenClawCodingTools, the active channel registry, and a deliberately prewrapped tool for both openai and anthropic. Copied output:

{"providers":["openai","anthropic"],"executeCalls":2,"hookCalls":2,"contexts":[{"agentId":"main","sessionId":"live-openai","runId":"run-openai"},{"agentId":"main","sessionId":"live-anthropic","runId":"run-anthropic"}]}

The probe also confirmed the normalized empty-object schema remained intact after rewrap. Fresh autoreview found no accepted/actionable findings. Exact-head CI is green, including the real-behavior-proof gate.

Known proof gap: no external provider request was run specifically for this patch because it changes wrapper lifecycle only, not OpenAI/Anthropic serialization or transport. Both provider-policy assembly paths were exercised directly, and the unchanged provider/tool-call lanes are green in CI.

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 openclaw#92973. Co-authored-by: zengLingbiao <[email protected]>

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 <[email protected]> (cherry picked from commit 07dfdd4)

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 openclaw#92973. Co-authored-by: zengLingbiao <[email protected]>

openclaw-barnacle Bot added agents Agent runtime and tooling size: XS proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 14, 2026

clawsweeper Bot added the merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. label Jun 14, 2026

openclaw-barnacle Bot added size: S and removed size: XS labels Jun 14, 2026

fix(agents): prevent duplicate before_tool_call hooks

f903512

Co-authored-by: zengLingbiao <[email protected]>

steipete force-pushed the fix/issue-92973-idempotent-hook-wrap branch from aa5b75f to f903512 Compare June 14, 2026 18:13

openclaw-barnacle Bot added triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. and removed proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 14, 2026

test(agents): update before-tool-call mock exports

fe1f2fa

openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 14, 2026

steipete merged commit 07dfdd4 into openclaw:main Jun 14, 2026
175 of 179 checks passed

github-actions Bot mentioned this pull request Jun 14, 2026

📡 Upstream Digest — 2026-06-14 19:12 UTC curtismercier/openclaw-mods#1079

Open

This was referenced Jun 14, 2026

feat(gateway): wire coding tools into /tools/invoke HTTP surface #63919

Open

fix(agents): preserve unreadable tool schemas through wrapping #90219

Closed

Runtime safety: break out of repeated identical failed tool calls #86454

Closed

This was referenced Jun 15, 2026

Add credential blast-radius guard #73130

Closed

feat(gateway): wire read coding tool into HTTP /tools/invoke (narrow) #85664

Open

clawsweeper Bot mentioned this pull request Jun 19, 2026

fix(agents): avoid enumerating wrapped tool descriptors #89717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(agents): make wrapToolWithBeforeToolCallHook idempotent to prevent double hook execution (fixes #92973)#93009

fix(agents): make wrapToolWithBeforeToolCallHook idempotent to prevent double hook execution (fixes #92973)#93009
steipete merged 2 commits into
openclaw:mainfrom
zenglingbiao:fix/issue-92973-idempotent-hook-wrap

zenglingbiao commented Jun 14, 2026 •

edited by steipete

Loading

Uh oh!

clawsweeper Bot commented Jun 14, 2026 •

edited

Loading

Uh oh!

zenglingbiao commented Jun 14, 2026

Uh oh!

clawsweeper Bot commented Jun 14, 2026 •

edited

Loading

Uh oh!

zenglingbiao commented Jun 14, 2026

Uh oh!

clawsweeper Bot commented Jun 14, 2026 •

edited

Loading

Uh oh!

steipete commented Jun 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

zenglingbiao commented Jun 14, 2026 • edited by steipete Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Real behavior proof

Uh oh!

clawsweeper Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zenglingbiao commented Jun 14, 2026

Uh oh!

clawsweeper Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zenglingbiao commented Jun 14, 2026

Uh oh!

clawsweeper Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jun 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zenglingbiao commented Jun 14, 2026 •

edited by steipete

Loading

clawsweeper Bot commented Jun 14, 2026 •

edited

Loading

clawsweeper Bot commented Jun 14, 2026 •

edited

Loading

clawsweeper Bot commented Jun 14, 2026 •

edited

Loading