fix(hooks): consolidate after_tool_call context + single-fire behavior by vincentkoc · Pull Request #32201 · openclaw/openclaw

vincentkoc · 2026-03-02T21:51:01Z

Summary

Why it matters: embedded after_tool_call hooks could miss session context and/or fire more than once per tool execution.
What changed: consolidated the required pieces into one branch by cherry-picking:
- dedupe single-fire behavior from fix(hooks): deduplicate after_tool_call hook in embedded runs [AI-assisted] #27283
- embedded sessionKey propagation from fix(hooks): thread agentId into after_tool_call hook context #30527
- embedded agentId propagation from fix(hooks): thread agentId into after_tool_call hook context #30527
- conflict resolution in run/attempt.ts keeps normalized sessionKey: sandboxSessionKey and adds agentId: sessionAgentId
- changelog credit entry thanking relevant contributors
What did NOT change (scope boundary): no hook contract expansion (toolCallId/runId/sessionId remain follow-up).

AI-assisted: yes

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

User-visible / Behavior Changes

after_tool_call in embedded runs now includes sessionKey and agentId context when available.
after_tool_call now fires exactly once per tool execution path in embedded runs (success and error).

Security Impact (required)

New permissions/capabilities? (No)
Secrets/tokens handling changed? (No)
New/changed network calls? (No)
Command/tool execution surface changed? (No)
Data access scope changed? (No)
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: macOS
Runtime/container: Node 22 / pnpm workspace
Model/provider: N/A (hook wiring)
Integration/channel (if any): Embedded agent tool handlers
Relevant config (redacted): N/A

Steps

Register an after_tool_call hook.
Run embedded tool execution (success + error paths).
Verify hook context and call count.

Expected

Hook context includes sessionKey and agentId in embedded runs.
Hook fires once per tool execution.

Actual

Consolidated behavior now matches expected in targeted tests.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios:
- pnpm check
- pnpm exec vitest run src/agents/pi-tool-definition-adapter.after-tool-call.test.ts src/agents/pi-tool-definition-adapter.after-tool-call.fires-once.test.ts
- pnpm exec vitest run --config vitest.e2e.config.ts src/plugins/wired-hooks-after-tool-call.e2e.test.ts
Edge cases checked:
- Success + error paths both dispatch exactly once.
- Embedded handler context includes sessionKey + agentId.
What you did not verify:
- Full pnpm test / full e2e matrix.

Compatibility / Migration

Backward compatible? (Yes)
Config/env changes? (No)
Migration needed? (No)
If yes, exact upgrade steps:

Failure Recovery (if this breaks)

How to disable/revert this change quickly:
- Revert this PR.
Files/config to restore:
- src/agents/pi-tool-definition-adapter.ts
- src/agents/pi-tool-definition-adapter.after-tool-call.test.ts
- src/agents/pi-tool-definition-adapter.after-tool-call.fires-once.test.ts
- src/agents/pi-embedded-subscribe.handlers.tools.ts
- src/agents/pi-embedded-subscribe.handlers.types.ts
- src/agents/pi-embedded-subscribe.types.ts
- src/agents/pi-embedded-runner/run/attempt.ts
- src/plugins/wired-hooks-after-tool-call.e2e.test.ts
- CHANGELOG.md
Known bad symptoms reviewers should watch for:
- Missing embedded hook context.
- Duplicate after_tool_call hook invocations.

Risks and Mitigations

Risk:
- Existing consumers that implicitly relied on duplicate callback behavior may observe fewer events.
- Mitigation:
- Single-fire semantics are the explicit requirement for this cycle; tests assert one call per tool execution.

(cherry picked from commit c129a1a)

The after_tool_call hook in handleToolExecutionEnd was passing `sessionKey: undefined` in the ToolContext, even though the value is available on ctx.params. This broke plugins that need session context in after_tool_call handlers (e.g., for per-session audit trails or security logging). - Add `sessionKey` to the `ToolHandlerParams` Pick type - Pass `ctx.params.sessionKey` through to the hook context - Add test assertion to prevent regression Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit b711738)

Follow-up to #30511 — the after_tool_call hook context was passing `agentId: undefined` because SubscribeEmbeddedPiSessionParams did not carry the agent identity. This threads sessionAgentId (resolved in attempt.ts) through the session params into the tool handler context, giving plugins accurate agent-scoped context for both before_tool_call and after_tool_call hooks. Changes: - Add `agentId?: string` to SubscribeEmbeddedPiSessionParams - Add "agentId" to ToolHandlerParams Pick type - Pass `agentId: sessionAgentId` at the subscribeEmbeddedPiSession() call site in attempt.ts - Wire ctx.params.agentId into the after_tool_call hook context - Update tests to assert agentId propagation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit aad01ed)

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2628a4f41f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

src/agents/pi-tool-definition-adapter.ts

greptile-apps · 2026-03-02T22:00:44Z

Greptile Summary

This PR consolidates two previously split fixes for the after_tool_call embedded hook into a single branch: (1) removing the duplicate hook dispatch from the toToolDefinitions adapter so the hook fires exactly once per tool execution (via handleToolExecutionEnd), and (2) propagating sessionKey and agentId through SubscribeEmbeddedPiSessionParams → ToolHandlerParams → runAfterToolCall context. Since toToolDefinitions is exclusively consumed by the embedded runner (tool-split.ts), no non-embedded code path is affected.

Key changes:

pi-tool-definition-adapter.ts: Removes after_tool_call hook invocations from success and error paths; retains consumeAdjustedParamsForToolCall to prevent param leaks for wrapped tools.
pi-embedded-subscribe.handlers.tools.ts: Replaces hardcoded undefined with ctx.params.agentId / ctx.params.sessionKey in the runAfterToolCall call.
pi-embedded-subscribe.types.ts + handlers.types.ts: Adds agentId?: string to the params chain so it reaches the handler.
attempt.ts: Threads sessionAgentId (already resolved earlier in the function) into the subscription params.
Tests added/updated: unit test asserting adapter no longer fires the hook, integration test asserting exactly-once semantics across success/error/sequential paths, and E2E assertions for the new context fields.

One minor issue: the doc-comment in the new fires-once test file cites PR #15012 as the source of the dedup fix, whereas the PR description states it was cherry-picked from #27283.

Confidence Score: 4/5

Safe to merge — logic is correct and well-tested; one stale PR reference in a test comment is the only finding.
The consolidation is logically sound: toToolDefinitions is exclusively used in the embedded runner, so removing the hook from the adapter creates no regression on other paths; the handler already owned the single-fire contract. Session context propagation is straightforward and backward-compatible. Test coverage for the new behavior is thorough (unit + integration + E2E). The only non-critical finding is a mismatched PR number (#15012 vs #27283) in a test doc-comment.
No files require special attention beyond the stale PR reference in src/agents/pi-tool-definition-adapter.after-tool-call.fires-once.test.ts.

_{Last reviewed commit: 2628a4f}

greptile-apps

_{9 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/agents/pi-tool-definition-adapter.after-tool-call.fires-once.test.ts

aisle-research-bot · 2026-03-02T22:13:56Z

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

#	Severity	Title
1	🔵 Low	Cross-session/race-prone adjusted tool params lookup keyed only by toolCallId

1. 🔵 Cross-session/race-prone adjusted tool params lookup keyed only by toolCallId

Property	Value
Severity	Low
CWE	CWE-362
Location	`src/agents/pi-embedded-subscribe.handlers.tools.ts:363-371`

Description

after_tool_call params are now sourced by calling consumeAdjustedParamsForToolCall(toolCallId) inside the subscription event handler. The underlying storage for adjusted params is a module-global Map<string, unknown> keyed only by toolCallId, with no run/session scoping.

This creates a shared mutable state issue:

adjustedParamsByToolCallId is global (process-wide) and keyed only by toolCallId (pi-tools.before-tool-call.ts:19).
The subscription handler consumes based only on toolCallId (pi-embedded-subscribe.handlers.tools.ts:367), and now also attaches sessionKey/agentId to the hook context.
If two concurrent embedded sessions (or a later session) reuse/collide on the same toolCallId, the handler may consume another session’s adjusted params, causing cross-session data mix-up and potential leakage into plugin hooks/telemetry.
Because consumption no longer happens in the adapter immediately after execute returns, the lifetime of entries in the global map is extended until tool_execution_end arrives; if that event is missed (crash/abort/stream interruption), the entry can persist (bounded to 1024) and be consumed by an unrelated future call with the same toolCallId.

Vulnerable flow (simplified):

input: toolCallId from tool execution events
shared state: global adjustedParamsByToolCallId
sink: hookRunnerAfter.runAfterToolCall({ params: afterToolCallArgs, ... }, { sessionKey, agentId })

Vulnerable code (new consumption point):

const adjustedArgs = consumeAdjustedParamsForToolCall(toolCallId);
const afterToolCallArgs =
  adjustedArgs && typeof adjustedArgs === "object"
    ? (adjustedArgs as Record<string, unknown>)
    : startArgs;

Underlying shared global state:

const adjustedParamsByToolCallId = new Map<string, unknown>();
export function consumeAdjustedParamsForToolCall(toolCallId: string): unknown {
  const params = adjustedParamsByToolCallId.get(toolCallId);
  adjustedParamsByToolCallId.delete(toolCallId);
  return params;
}

Recommendation

Scope adjusted-params storage/consumption to a run/session (or keep it entirely within per-session handler state), and ensure cleanup on abnormal termination.

Suggested fix options:

Namespace the key by session (minimal change):

// pi-tools.before-tool-call.ts
const adjustedParamsByKey = new Map<string, unknown>();
const keyFor = (sessionKey: string, toolCallId: string) => `${sessionKey}:${toolCallId}`;

export function storeAdjustedParams(sessionKey: string, toolCallId: string, params: unknown) {
  adjustedParamsByKey.set(keyFor(sessionKey, toolCallId), params);
}

export function consumeAdjustedParamsForToolCall(sessionKey: string, toolCallId: string): unknown {
  const key = keyFor(sessionKey, toolCallId);
  const params = adjustedParamsByKey.get(key);
  adjustedParamsByKey.delete(key);
  return params;
}

// pi-embedded-subscribe.handlers.tools.ts
const adjustedArgs =
  ctx.params.sessionKey ? consumeAdjustedParamsForToolCall(ctx.params.sessionKey, toolCallId) : undefined;

Per-context storage: put adjusted params into ctx.state (e.g., ctx.state.adjustedParamsByToolCallId) so different sessions can’t collide.

Also consider:

Using isPlainObject(adjustedArgs) before treating it as a record
Clearing any per-toolCallId state on agent_end/unsubscribe/abort paths to avoid stale retention

Analyzed PR: #32201 at commit d3a6c3e

_{Last updated on: 2026-03-02T22:37:42Z}

openclaw#32201) * fix(hooks): deduplicate after_tool_call hook in embedded runs (cherry picked from commit c129a1a) * fix(hooks): propagate sessionKey in after_tool_call context The after_tool_call hook in handleToolExecutionEnd was passing `sessionKey: undefined` in the ToolContext, even though the value is available on ctx.params. This broke plugins that need session context in after_tool_call handlers (e.g., for per-session audit trails or security logging). - Add `sessionKey` to the `ToolHandlerParams` Pick type - Pass `ctx.params.sessionKey` through to the hook context - Add test assertion to prevent regression Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit b711738) * fix(hooks): thread agentId through to after_tool_call hook context Follow-up to openclaw#30511 — the after_tool_call hook context was passing `agentId: undefined` because SubscribeEmbeddedPiSessionParams did not carry the agent identity. This threads sessionAgentId (resolved in attempt.ts) through the session params into the tool handler context, giving plugins accurate agent-scoped context for both before_tool_call and after_tool_call hooks. Changes: - Add `agentId?: string` to SubscribeEmbeddedPiSessionParams - Add "agentId" to ToolHandlerParams Pick type - Pass `agentId: sessionAgentId` at the subscribeEmbeddedPiSession() call site in attempt.ts - Wire ctx.params.agentId into the after_tool_call hook context - Update tests to assert agentId propagation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit aad01ed) * changelog: credit after_tool_call hook contributors * Update CHANGELOG.md * agents: preserve adjusted params until tool end * agents: emit after_tool_call with adjusted args * tests: cover adjusted after_tool_call params * tests: align adapter after_tool_call expectation --------- Co-authored-by: jbeno <jim@jimbeno.net> Co-authored-by: scoootscooob <zhentongfan@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

openclaw#32201) * fix(hooks): deduplicate after_tool_call hook in embedded runs (cherry picked from commit c129a1a) * fix(hooks): propagate sessionKey in after_tool_call context The after_tool_call hook in handleToolExecutionEnd was passing `sessionKey: undefined` in the ToolContext, even though the value is available on ctx.params. This broke plugins that need session context in after_tool_call handlers (e.g., for per-session audit trails or security logging). - Add `sessionKey` to the `ToolHandlerParams` Pick type - Pass `ctx.params.sessionKey` through to the hook context - Add test assertion to prevent regression (cherry picked from commit b711738) * fix(hooks): thread agentId through to after_tool_call hook context Follow-up to openclaw#30511 — the after_tool_call hook context was passing `agentId: undefined` because SubscribeEmbeddedPiSessionParams did not carry the agent identity. This threads sessionAgentId (resolved in attempt.ts) through the session params into the tool handler context, giving plugins accurate agent-scoped context for both before_tool_call and after_tool_call hooks. Changes: - Add `agentId?: string` to SubscribeEmbeddedPiSessionParams - Add "agentId" to ToolHandlerParams Pick type - Pass `agentId: sessionAgentId` at the subscribeEmbeddedPiSession() call site in attempt.ts - Wire ctx.params.agentId into the after_tool_call hook context - Update tests to assert agentId propagation (cherry picked from commit aad01ed) * changelog: credit after_tool_call hook contributors * Update CHANGELOG.md * agents: preserve adjusted params until tool end * agents: emit after_tool_call with adjusted args * tests: cover adjusted after_tool_call params * tests: align adapter after_tool_call expectation --------- Co-authored-by: jbeno <jim@jimbeno.net> Co-authored-by: scoootscooob <zhentongfan@gmail.com>

jbeno and others added 4 commits March 2, 2026 13:47

fix(hooks): deduplicate after_tool_call hook in embedded runs

94b74c3

(cherry picked from commit c129a1a)

changelog: credit after_tool_call hook contributors

2920040

vincentkoc self-assigned this Mar 2, 2026

openclaw-barnacle bot added agents Agent runtime and tooling size: M maintainer Maintainer-authored PR labels Mar 2, 2026

vincentkoc changed the title ~~fix(hooks): consolidate Option A after_tool_call context + single-fire behavior~~ fix(hooks): consolidate after_tool_call context + single-fire behavior Mar 2, 2026

Update CHANGELOG.md

dbc376b

vincentkoc marked this pull request as ready for review March 2, 2026 21:53

Merge branch 'main' into vincentkoc-code/hooks-option-a-consolidated

2628a4f

chatgpt-codex-connector bot reviewed Mar 2, 2026

View reviewed changes

src/agents/pi-tool-definition-adapter.ts Outdated Show resolved Hide resolved

greptile-apps bot reviewed Mar 2, 2026

View reviewed changes

src/agents/pi-tool-definition-adapter.after-tool-call.fires-once.test.ts Outdated Show resolved Hide resolved

vincentkoc added 4 commits March 2, 2026 14:12

agents: preserve adjusted params until tool end

70a00e7

agents: emit after_tool_call with adjusted args

69ac7ec

tests: cover adjusted after_tool_call params

287f331

tests: align adapter after_tool_call expectation

d3a6c3e

vincentkoc merged commit 44183c6 into main Mar 2, 2026
12 of 14 checks passed

vincentkoc deleted the vincentkoc-code/hooks-option-a-consolidated branch March 2, 2026 22:33

This was referenced Mar 2, 2026

feat: harden after_tool_call correlation for #32201 and legacy runtimes comet-ml/opik-openclaw#7

Merged

fix(hooks): propagate ephemeral sessionId through embedded tool contexts #32273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(hooks): consolidate after_tool_call context + single-fire behavior#32201

fix(hooks): consolidate after_tool_call context + single-fire behavior#32201
vincentkoc merged 10 commits intomainfrom
vincentkoc-code/hooks-option-a-consolidated

vincentkoc commented Mar 2, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

Uh oh!

greptile-apps bot commented Mar 2, 2026

Uh oh!

greptile-apps bot left a comment

Uh oh!

Uh oh!

aisle-research-bot bot commented Mar 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

vincentkoc commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

User-visible / Behavior Changes

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Compatibility / Migration

Failure Recovery (if this breaks)

Risks and Mitigations

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

greptile-apps bot commented Mar 2, 2026

Greptile Summary

Confidence Score: 4/5

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aisle-research-bot bot commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔒 Aisle Security Analysis

1. 🔵 Cross-session/race-prone adjusted tool params lookup keyed only by toolCallId

Description

Recommendation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vincentkoc commented Mar 2, 2026 •

edited

Loading

aisle-research-bot bot commented Mar 2, 2026 •

edited

Loading