From 44183c6eb19eff324819dba74a5b9eae735c5e2f Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Mon, 2 Mar 2026 14:33:37 -0800 Subject: [PATCH] fix(hooks): consolidate after_tool_call context + single-fire behavior (#32201) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(hooks): deduplicate after_tool_call hook in embedded runs (cherry picked from commit c129a1a74ba247460c6c061776ceeb995c757ecf) * 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 (cherry picked from commit b7117384fc1a09d60b25db0f80847a3519ddb3c3) * fix(hooks): thread agentId through to after_tool_call hook context 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 (cherry picked from commit aad01edd3e0d367ad0defeb691d5689ddf2ee617) * 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 Co-authored-by: scoootscooob Co-authored-by: Claude Opus 4.6 --- CHANGELOG.md | 1 + src/agents/pi-embedded-runner/run/attempt.ts | 1 + .../pi-embedded-subscribe.handlers.tools.ts | 13 +- .../pi-embedded-subscribe.handlers.types.ts | 2 +- src/agents/pi-embedded-subscribe.types.ts | 2 + ...adapter.after-tool-call.fires-once.test.ts | 283 ++++++++++++++++++ ...definition-adapter.after-tool-call.test.ts | 102 ++----- src/agents/pi-tool-definition-adapter.ts | 51 +--- .../wired-hooks-after-tool-call.e2e.test.ts | 10 +- 9 files changed, 338 insertions(+), 127 deletions(-) create mode 100644 src/agents/pi-tool-definition-adapter.after-tool-call.fires-once.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 97de9fddbea..2818244f606 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ Docs: https://docs.openclaw.ai - Config/backups hardening: enforce owner-only (`0600`) permissions on rotated config backups and clean orphan `.bak.*` files outside the managed backup ring, reducing credential leakage risk from stale or permissive backup artifacts. (#31718) Thanks @YUJIE2002. - Exec approvals/allowlist matching: escape regex metacharacters in path-pattern literals (while preserving glob wildcards), preventing crashes on allowlisted executables like `/usr/bin/g++` and correctly matching mixed wildcard/literal token paths. (#32162) Thanks @stakeswky. - Agents/tool-result guard: always clear pending tool-call state on interruptions even when synthetic tool results are disabled, preventing orphaned tool-use transcripts that cause follow-up provider request failures. (#32120) Thanks @jnMetaCode. +- Hooks/after_tool_call: include embedded session context (`sessionKey`, `agentId`) and fire the hook exactly once per tool execution by removing duplicate adapter-path dispatch in embedded runs. (#32201) Thanks @jbeno, @scoootscooob, @vincentkoc. - Webchat/stream finalization: persist streamed assistant text when final events omit `message`, while keeping final payload precedence and skipping empty stream buffers to prevent disappearing replies after tool turns. (#31920) Thanks @Sid-Qin. - Cron/store migration: normalize legacy cron jobs with string `schedule` and top-level `command`/`timeout` fields into canonical schedule/payload/session-target shape on load, preventing schedule-error loops on old persisted stores. (#31926) Thanks @bmendonca3. - Gateway/Heartbeat model reload: treat `models.*` and `agents.defaults.model` config updates as heartbeat hot-reload triggers so heartbeat picks up model changes without a full gateway restart. (#32046) Thanks @stakeswky. diff --git a/src/agents/pi-embedded-runner/run/attempt.ts b/src/agents/pi-embedded-runner/run/attempt.ts index 722bae2f79e..fa2b508f15c 100644 --- a/src/agents/pi-embedded-runner/run/attempt.ts +++ b/src/agents/pi-embedded-runner/run/attempt.ts @@ -1186,6 +1186,7 @@ export async function runEmbeddedAttempt( enforceFinalTag: params.enforceFinalTag, config: params.config, sessionKey: sandboxSessionKey, + agentId: sessionAgentId, }); const { diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.ts b/src/agents/pi-embedded-subscribe.handlers.tools.ts index 18dc11193f0..4a76f62ff62 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.ts @@ -18,6 +18,7 @@ import { sanitizeToolResult, } from "./pi-embedded-subscribe.tools.js"; import { inferToolMetaFromArgs } from "./pi-embedded-utils.js"; +import { consumeAdjustedParamsForToolCall } from "./pi-tools.before-tool-call.js"; import { buildToolMutationState, isSameToolMutationAction } from "./tool-mutation.js"; import { normalizeToolName } from "./tool-policy.js"; @@ -363,6 +364,11 @@ export async function handleToolExecutionEnd( startData?.args && typeof startData.args === "object" ? (startData.args as Record) : {}; + const adjustedArgs = consumeAdjustedParamsForToolCall(toolCallId); + const afterToolCallArgs = + adjustedArgs && typeof adjustedArgs === "object" + ? (adjustedArgs as Record) + : startArgs; const isMessagingSend = pendingMediaUrls.length > 0 || (isMessagingTool(toolName) && isMessagingToolSendAction(toolName, startArgs)); @@ -415,10 +421,9 @@ export async function handleToolExecutionEnd( const hookRunnerAfter = ctx.hookRunner ?? getGlobalHookRunner(); if (hookRunnerAfter?.hasHooks("after_tool_call")) { const durationMs = startData?.startTime != null ? Date.now() - startData.startTime : undefined; - const toolArgs = startData?.args; const hookEvent: PluginHookAfterToolCallEvent = { toolName, - params: (toolArgs && typeof toolArgs === "object" ? toolArgs : {}) as Record, + params: afterToolCallArgs, result: sanitizedResult, error: isToolError ? extractToolErrorMessage(sanitizedResult) : undefined, durationMs, @@ -426,8 +431,8 @@ export async function handleToolExecutionEnd( void hookRunnerAfter .runAfterToolCall(hookEvent, { toolName, - agentId: undefined, - sessionKey: undefined, + agentId: ctx.params.agentId, + sessionKey: ctx.params.sessionKey, }) .catch((err) => { ctx.log.warn(`after_tool_call hook failed: tool=${toolName} error=${String(err)}`); diff --git a/src/agents/pi-embedded-subscribe.handlers.types.ts b/src/agents/pi-embedded-subscribe.handlers.types.ts index d5c725528c8..d7488d767ad 100644 --- a/src/agents/pi-embedded-subscribe.handlers.types.ts +++ b/src/agents/pi-embedded-subscribe.handlers.types.ts @@ -132,7 +132,7 @@ export type EmbeddedPiSubscribeContext = { */ export type ToolHandlerParams = Pick< SubscribeEmbeddedPiSessionParams, - "runId" | "onBlockReplyFlush" | "onAgentEvent" | "onToolResult" + "runId" | "onBlockReplyFlush" | "onAgentEvent" | "onToolResult" | "sessionKey" | "agentId" >; export type ToolHandlerState = Pick< diff --git a/src/agents/pi-embedded-subscribe.types.ts b/src/agents/pi-embedded-subscribe.types.ts index afa635d7307..426daf2fd15 100644 --- a/src/agents/pi-embedded-subscribe.types.ts +++ b/src/agents/pi-embedded-subscribe.types.ts @@ -31,6 +31,8 @@ export type SubscribeEmbeddedPiSessionParams = { enforceFinalTag?: boolean; config?: OpenClawConfig; sessionKey?: string; + /** Agent identity for hook context — resolved from session config in attempt.ts. */ + agentId?: string; }; export type { BlockReplyChunking } from "./pi-embedded-block-chunker.js"; diff --git a/src/agents/pi-tool-definition-adapter.after-tool-call.fires-once.test.ts b/src/agents/pi-tool-definition-adapter.after-tool-call.fires-once.test.ts new file mode 100644 index 00000000000..9c77957835d --- /dev/null +++ b/src/agents/pi-tool-definition-adapter.after-tool-call.fires-once.test.ts @@ -0,0 +1,283 @@ +/** + * Integration test: after_tool_call fires exactly once when both the adapter + * (toToolDefinitions) and the subscription handler (handleToolExecutionEnd) + * are active — the production scenario for embedded runs. + * + * Regression guard for the double-fire bug fixed by removing the adapter-side + * after_tool_call invocation (see PR #27283 → dedup in this fix). + */ +import type { AgentTool } from "@mariozechner/pi-agent-core"; +import { Type } from "@sinclair/typebox"; +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; + +const hookMocks = vi.hoisted(() => ({ + runner: { + hasHooks: vi.fn(() => true), + runAfterToolCall: vi.fn(async () => {}), + runBeforeToolCall: vi.fn(async () => {}), + }, +})); + +const beforeToolCallMocks = vi.hoisted(() => ({ + consumeAdjustedParamsForToolCall: vi.fn((_: string): unknown => undefined), + isToolWrappedWithBeforeToolCallHook: vi.fn(() => false), + runBeforeToolCallHook: vi.fn(async ({ params }: { params: unknown }) => ({ + blocked: false, + params, + })), +})); + +vi.mock("../plugins/hook-runner-global.js", () => ({ + getGlobalHookRunner: () => hookMocks.runner, +})); + +vi.mock("../infra/agent-events.js", () => ({ + emitAgentEvent: vi.fn(), +})); + +vi.mock("./pi-tools.before-tool-call.js", () => ({ + consumeAdjustedParamsForToolCall: beforeToolCallMocks.consumeAdjustedParamsForToolCall, + isToolWrappedWithBeforeToolCallHook: beforeToolCallMocks.isToolWrappedWithBeforeToolCallHook, + runBeforeToolCallHook: beforeToolCallMocks.runBeforeToolCallHook, +})); + +function createTestTool(name: string) { + return { + name, + label: name, + description: `test tool: ${name}`, + parameters: Type.Object({}), + execute: vi.fn(async () => ({ + content: [{ type: "text" as const, text: "ok" }], + details: { ok: true }, + })), + } satisfies AgentTool; +} + +function createFailingTool(name: string) { + return { + name, + label: name, + description: `failing tool: ${name}`, + parameters: Type.Object({}), + execute: vi.fn(async () => { + throw new Error("tool failed"); + }), + } satisfies AgentTool; +} + +function createToolHandlerCtx() { + return { + params: { + runId: "integration-test", + session: { messages: [] }, + }, + hookRunner: hookMocks.runner, + state: { + toolMetaById: new Map(), + toolMetas: [] as Array<{ toolName?: string; meta?: string }>, + toolSummaryById: new Set(), + lastToolError: undefined, + pendingMessagingTexts: new Map(), + pendingMessagingTargets: new Map(), + pendingMessagingMediaUrls: new Map(), + messagingToolSentTexts: [] as string[], + messagingToolSentTextsNormalized: [] as string[], + messagingToolSentMediaUrls: [] as string[], + messagingToolSentTargets: [] as unknown[], + blockBuffer: "", + successfulCronAdds: 0, + }, + log: { debug: vi.fn(), warn: vi.fn() }, + flushBlockReplyBuffer: vi.fn(), + shouldEmitToolResult: () => false, + shouldEmitToolOutput: () => false, + emitToolSummary: vi.fn(), + emitToolOutput: vi.fn(), + trimMessagingToolSent: vi.fn(), + }; +} + +let toToolDefinitions: typeof import("./pi-tool-definition-adapter.js").toToolDefinitions; +let handleToolExecutionStart: typeof import("./pi-embedded-subscribe.handlers.tools.js").handleToolExecutionStart; +let handleToolExecutionEnd: typeof import("./pi-embedded-subscribe.handlers.tools.js").handleToolExecutionEnd; + +describe("after_tool_call fires exactly once in embedded runs", () => { + beforeAll(async () => { + ({ toToolDefinitions } = await import("./pi-tool-definition-adapter.js")); + ({ handleToolExecutionStart, handleToolExecutionEnd } = + await import("./pi-embedded-subscribe.handlers.tools.js")); + }); + + beforeEach(() => { + hookMocks.runner.hasHooks.mockClear(); + hookMocks.runner.hasHooks.mockReturnValue(true); + hookMocks.runner.runAfterToolCall.mockClear(); + hookMocks.runner.runAfterToolCall.mockResolvedValue(undefined); + hookMocks.runner.runBeforeToolCall.mockClear(); + hookMocks.runner.runBeforeToolCall.mockResolvedValue(undefined); + beforeToolCallMocks.consumeAdjustedParamsForToolCall.mockClear(); + beforeToolCallMocks.consumeAdjustedParamsForToolCall.mockReturnValue(undefined); + beforeToolCallMocks.isToolWrappedWithBeforeToolCallHook.mockClear(); + beforeToolCallMocks.isToolWrappedWithBeforeToolCallHook.mockReturnValue(false); + beforeToolCallMocks.runBeforeToolCallHook.mockClear(); + beforeToolCallMocks.runBeforeToolCallHook.mockImplementation(async ({ params }) => ({ + blocked: false, + params, + })); + }); + + it("fires after_tool_call exactly once on success when both adapter and handler are active", async () => { + const tool = createTestTool("read"); + const defs = toToolDefinitions([tool]); + const def = defs[0]; + if (!def) { + throw new Error("missing tool definition"); + } + + const toolCallId = "integration-call-1"; + const args = { path: "/tmp/test.txt" }; + const ctx = createToolHandlerCtx(); + + // Step 1: Simulate tool_execution_start event (SDK emits this) + await handleToolExecutionStart( + ctx as never, + { type: "tool_execution_start", toolName: "read", toolCallId, args } as never, + ); + + // Step 2: Execute tool through the adapter wrapper (SDK calls this) + const extensionContext = {} as Parameters[4]; + await def.execute(toolCallId, args, undefined, undefined, extensionContext); + + // Step 3: Simulate tool_execution_end event (SDK emits this after execute returns) + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "read", + toolCallId, + isError: false, + result: { content: [{ type: "text", text: "ok" }] }, + } as never, + ); + + // The hook must fire exactly once — not zero, not two. + expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledTimes(1); + }); + + it("fires after_tool_call exactly once on error when both adapter and handler are active", async () => { + const tool = createFailingTool("exec"); + const defs = toToolDefinitions([tool]); + const def = defs[0]; + if (!def) { + throw new Error("missing tool definition"); + } + + const toolCallId = "integration-call-err"; + const args = { command: "fail" }; + const ctx = createToolHandlerCtx(); + + await handleToolExecutionStart( + ctx as never, + { type: "tool_execution_start", toolName: "exec", toolCallId, args } as never, + ); + + const extensionContext = {} as Parameters[4]; + await def.execute(toolCallId, args, undefined, undefined, extensionContext); + + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "exec", + toolCallId, + isError: true, + result: { status: "error", error: "tool failed" }, + } as never, + ); + + expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledTimes(1); + + const call = (hookMocks.runner.runAfterToolCall as ReturnType).mock.calls[0]; + const event = call?.[0] as { error?: unknown } | undefined; + expect(event?.error).toBeDefined(); + }); + + it("uses before_tool_call adjusted params for after_tool_call payload", async () => { + const tool = createTestTool("read"); + const defs = toToolDefinitions([tool]); + const def = defs[0]; + if (!def) { + throw new Error("missing tool definition"); + } + + const toolCallId = "integration-call-adjusted"; + const args = { path: "/tmp/original.txt" }; + const adjusted = { path: "/tmp/adjusted.txt", mode: "safe" }; + const ctx = createToolHandlerCtx(); + const extensionContext = {} as Parameters[4]; + + beforeToolCallMocks.isToolWrappedWithBeforeToolCallHook.mockReturnValue(true); + beforeToolCallMocks.consumeAdjustedParamsForToolCall.mockImplementation((id: string) => + id === toolCallId ? adjusted : undefined, + ); + + await handleToolExecutionStart( + ctx as never, + { type: "tool_execution_start", toolName: "read", toolCallId, args } as never, + ); + await def.execute(toolCallId, args, undefined, undefined, extensionContext); + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "read", + toolCallId, + isError: false, + result: { content: [{ type: "text", text: "ok" }] }, + } as never, + ); + + expect(beforeToolCallMocks.consumeAdjustedParamsForToolCall).toHaveBeenCalledWith(toolCallId); + const event = (hookMocks.runner.runAfterToolCall as ReturnType).mock + .calls[0]?.[0] as { params?: unknown } | undefined; + expect(event?.params).toEqual(adjusted); + }); + + it("fires after_tool_call exactly once per tool across multiple sequential tool calls", async () => { + const tool = createTestTool("write"); + const defs = toToolDefinitions([tool]); + const def = defs[0]; + if (!def) { + throw new Error("missing tool definition"); + } + + const ctx = createToolHandlerCtx(); + const extensionContext = {} as Parameters[4]; + + for (let i = 0; i < 3; i++) { + const toolCallId = `sequential-call-${i}`; + const args = { path: `/tmp/file-${i}.txt`, content: "data" }; + + await handleToolExecutionStart( + ctx as never, + { type: "tool_execution_start", toolName: "write", toolCallId, args } as never, + ); + + await def.execute(toolCallId, args, undefined, undefined, extensionContext); + + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "write", + toolCallId, + isError: false, + result: { content: [{ type: "text", text: "written" }] }, + } as never, + ); + } + + expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledTimes(3); + }); +}); diff --git a/src/agents/pi-tool-definition-adapter.after-tool-call.test.ts b/src/agents/pi-tool-definition-adapter.after-tool-call.test.ts index 42784f1d726..5e30734129d 100644 --- a/src/agents/pi-tool-definition-adapter.after-tool-call.test.ts +++ b/src/agents/pi-tool-definition-adapter.after-tool-call.test.ts @@ -5,7 +5,7 @@ import { toToolDefinitions } from "./pi-tool-definition-adapter.js"; const hookMocks = vi.hoisted(() => ({ runner: { - hasHooks: vi.fn((_: string) => false), + hasHooks: vi.fn((_: string) => true), runAfterToolCall: vi.fn(async () => {}), }, isToolWrappedWithBeforeToolCallHook: vi.fn(() => false), @@ -39,31 +39,6 @@ function createReadTool() { type ToolExecute = ReturnType[number]["execute"]; const extensionContext = {} as Parameters[4]; -function enableAfterToolCallHook() { - hookMocks.runner.hasHooks.mockImplementation((name: string) => name === "after_tool_call"); -} - -async function executeReadTool(callId: string) { - const defs = toToolDefinitions([createReadTool()]); - const def = defs[0]; - if (!def) { - throw new Error("missing tool definition"); - } - const execute = (...args: Parameters<(typeof defs)[0]["execute"]>) => def.execute(...args); - return await execute(callId, { path: "/tmp/file" }, undefined, undefined, extensionContext); -} - -function expectReadAfterToolCallPayload(result: Awaited>) { - expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledWith( - { - toolName: "read", - params: { mode: "safe" }, - result, - }, - { toolName: "read" }, - ); -} - describe("pi tool definition adapter after_tool_call", () => { beforeEach(() => { hookMocks.runner.hasHooks.mockClear(); @@ -80,32 +55,21 @@ describe("pi tool definition adapter after_tool_call", () => { })); }); - it("dispatches after_tool_call once on successful adapter execution", async () => { - enableAfterToolCallHook(); - hookMocks.runBeforeToolCallHook.mockResolvedValue({ - blocked: false, - params: { mode: "safe" }, - }); - const result = await executeReadTool("call-ok"); + // Regression guard: after_tool_call is handled exclusively by + // handleToolExecutionEnd in the subscription handler to prevent + // duplicate invocations in embedded runs. + it("does not fire after_tool_call from the adapter (handled by subscription handler)", async () => { + const defs = toToolDefinitions([createReadTool()]); + const def = defs[0]; + if (!def) { + throw new Error("missing tool definition"); + } + await def.execute("call-ok", { path: "/tmp/file" }, undefined, undefined, extensionContext); - expect(result.details).toMatchObject({ ok: true }); - expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledTimes(1); - expectReadAfterToolCallPayload(result); + expect(hookMocks.runner.runAfterToolCall).not.toHaveBeenCalled(); }); - it("uses wrapped-tool adjusted params for after_tool_call payload", async () => { - enableAfterToolCallHook(); - hookMocks.isToolWrappedWithBeforeToolCallHook.mockReturnValue(true); - hookMocks.consumeAdjustedParamsForToolCall.mockReturnValue({ mode: "safe" } as unknown); - const result = await executeReadTool("call-ok-wrapped"); - - expect(result.details).toMatchObject({ ok: true }); - expect(hookMocks.runBeforeToolCallHook).not.toHaveBeenCalled(); - expectReadAfterToolCallPayload(result); - }); - - it("dispatches after_tool_call once on adapter error with normalized tool name", async () => { - enableAfterToolCallHook(); + it("does not fire after_tool_call from the adapter on error", async () => { const tool = { name: "bash", label: "Bash", @@ -121,31 +85,27 @@ describe("pi tool definition adapter after_tool_call", () => { if (!def) { throw new Error("missing tool definition"); } - const execute = (...args: Parameters<(typeof defs)[0]["execute"]>) => def.execute(...args); - const result = await execute("call-err", { cmd: "ls" }, undefined, undefined, extensionContext); + await def.execute("call-err", { cmd: "ls" }, undefined, undefined, extensionContext); - expect(result.details).toMatchObject({ - status: "error", - tool: "exec", - error: "boom", - }); - expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledTimes(1); - expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledWith( - { - toolName: "exec", - params: { cmd: "ls" }, - error: "boom", - }, - { toolName: "exec" }, - ); + expect(hookMocks.runner.runAfterToolCall).not.toHaveBeenCalled(); }); - it("does not break execution when after_tool_call hook throws", async () => { - enableAfterToolCallHook(); - hookMocks.runner.runAfterToolCall.mockRejectedValue(new Error("hook failed")); - const result = await executeReadTool("call-ok2"); + it("does not consume adjusted params in adapter for wrapped tools", async () => { + hookMocks.isToolWrappedWithBeforeToolCallHook.mockReturnValue(true); + const defs = toToolDefinitions([createReadTool()]); + const def = defs[0]; + if (!def) { + throw new Error("missing tool definition"); + } + await def.execute( + "call-wrapped", + { path: "/tmp/file" }, + undefined, + undefined, + extensionContext, + ); - expect(result.details).toMatchObject({ ok: true }); - expect(hookMocks.runner.runAfterToolCall).toHaveBeenCalledTimes(1); + expect(hookMocks.runBeforeToolCallHook).not.toHaveBeenCalled(); + expect(hookMocks.consumeAdjustedParamsForToolCall).not.toHaveBeenCalled(); }); }); diff --git a/src/agents/pi-tool-definition-adapter.ts b/src/agents/pi-tool-definition-adapter.ts index a6221586242..1d4823845eb 100644 --- a/src/agents/pi-tool-definition-adapter.ts +++ b/src/agents/pi-tool-definition-adapter.ts @@ -5,12 +5,10 @@ import type { } from "@mariozechner/pi-agent-core"; import type { ToolDefinition } from "@mariozechner/pi-coding-agent"; import { logDebug, logError } from "../logger.js"; -import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import { isPlainObject } from "../utils.js"; import type { ClientToolDefinition } from "./pi-embedded-runner/run/params.js"; import type { HookContext } from "./pi-tools.before-tool-call.js"; import { - consumeAdjustedParamsForToolCall, isToolWrappedWithBeforeToolCallHook, runBeforeToolCallHook, } from "./pi-tools.before-tool-call.js"; @@ -166,29 +164,6 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] { toolName: normalizedName, result: rawResult, }); - const afterParams = beforeHookWrapped - ? (consumeAdjustedParamsForToolCall(toolCallId) ?? executeParams) - : executeParams; - - // Call after_tool_call hook - const hookRunner = getGlobalHookRunner(); - if (hookRunner?.hasHooks("after_tool_call")) { - try { - await hookRunner.runAfterToolCall( - { - toolName: name, - params: isPlainObject(afterParams) ? afterParams : {}, - result, - }, - { toolName: name }, - ); - } catch (hookErr) { - logDebug( - `after_tool_call hook failed: tool=${normalizedName} error=${String(hookErr)}`, - ); - } - } - return result; } catch (err) { if (signal?.aborted) { @@ -201,41 +176,17 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] { if (name === "AbortError") { throw err; } - if (beforeHookWrapped) { - consumeAdjustedParamsForToolCall(toolCallId); - } const described = describeToolExecutionError(err); if (described.stack && described.stack !== described.message) { logDebug(`tools: ${normalizedName} failed stack:\n${described.stack}`); } logError(`[tools] ${normalizedName} failed: ${described.message}`); - const errorResult = jsonResult({ + return jsonResult({ status: "error", tool: normalizedName, error: described.message, }); - - // Call after_tool_call hook for errors too - const hookRunner = getGlobalHookRunner(); - if (hookRunner?.hasHooks("after_tool_call")) { - try { - await hookRunner.runAfterToolCall( - { - toolName: normalizedName, - params: isPlainObject(params) ? params : {}, - error: described.message, - }, - { toolName: normalizedName }, - ); - } catch (hookErr) { - logDebug( - `after_tool_call hook failed: tool=${normalizedName} error=${String(hookErr)}`, - ); - } - } - - return errorResult; } }, } satisfies ToolDefinition; diff --git a/src/plugins/wired-hooks-after-tool-call.e2e.test.ts b/src/plugins/wired-hooks-after-tool-call.e2e.test.ts index 8ec506a5d33..11d073e8356 100644 --- a/src/plugins/wired-hooks-after-tool-call.e2e.test.ts +++ b/src/plugins/wired-hooks-after-tool-call.e2e.test.ts @@ -114,7 +114,9 @@ describe("after_tool_call hook wiring", () => { const event = firstCall?.[0] as | { toolName?: string; params?: unknown; error?: unknown; durationMs?: unknown } | undefined; - const context = firstCall?.[1] as { toolName?: string } | undefined; + const context = firstCall?.[1] as + | { toolName?: string; agentId?: string; sessionKey?: string } + | undefined; expect(event).toBeDefined(); expect(context).toBeDefined(); if (!event || !context) { @@ -125,6 +127,8 @@ describe("after_tool_call hook wiring", () => { expect(event.error).toBeUndefined(); expect(typeof event.durationMs).toBe("number"); expect(context.toolName).toBe("read"); + expect(context.agentId).toBe("main"); + expect(context.sessionKey).toBe("test-session"); }); it("includes error in after_tool_call event on tool failure", async () => { @@ -163,6 +167,10 @@ describe("after_tool_call hook wiring", () => { throw new Error("missing hook call payload"); } expect(event.error).toBeDefined(); + + // agentId should be undefined when not provided + const context = firstCall?.[1] as { agentId?: string } | undefined; + expect(context?.agentId).toBeUndefined(); }); it("does not call runAfterToolCall when no hooks registered", async () => {