From 5537d286dde4d99c8f8341916523dfef7c310da2 Mon Sep 17 00:00:00 2001 From: Ayaan Zaidi Date: Tue, 7 Jul 2026 02:54:05 +0000 Subject: [PATCH] fix(agents): bound finalize hook hangs after compaction retry Behavior: a plugin before_agent_finalize hook that never resolves could previously freeze an agent run forever after a successful compaction retry, with no errors and no recovery from a gateway restart; this was the frozen-runner mechanism behind #84777. before_agent_finalize now has the same 15s default budget as sibling modifying hooks and fails open with the original final answer, converting the freeze into a bounded delay. Surface: plugin hook runner defaults (src/plugins/hooks.ts), docs/plugins/hooks.md. Refs #84777. (cherry picked from commit b2baf799b41edce512b1945b6dec3ad637b9c814) --- docs/plugins/hooks.md | 2 + ...agent-subscribe.handlers.lifecycle.test.ts | 70 +++++++++++++++++++ .../hooks.before-agent-finalize.test.ts | 23 ++++++ src/plugins/hooks.ts | 4 ++ 4 files changed, 99 insertions(+) diff --git a/docs/plugins/hooks.md b/docs/plugins/hooks.md index a9424ead8ca8..3a20f24e42e9 100644 --- a/docs/plugins/hooks.md +++ b/docs/plugins/hooks.md @@ -394,6 +394,8 @@ final assistant answer. It is not the `/stop` cancellation path and does not run when the user aborts a turn. Return `{ action: "revise", reason }` to ask the harness for one more model pass before finalization, `{ action: "finalize", reason? }` to force finalization, or omit a result to continue. +Handlers have a 15s default budget; on timeout, OpenClaw logs the failure and +continues with the original final answer. Codex native `Stop` hooks are relayed into this hook as OpenClaw `before_agent_finalize` decisions. diff --git a/src/agents/embedded-agent-subscribe.handlers.lifecycle.test.ts b/src/agents/embedded-agent-subscribe.handlers.lifecycle.test.ts index 59fa76c14336..acdc06cacee9 100644 --- a/src/agents/embedded-agent-subscribe.handlers.lifecycle.test.ts +++ b/src/agents/embedded-agent-subscribe.handlers.lifecycle.test.ts @@ -2,12 +2,27 @@ // lifecycle events, and deferred reply cleanup. import { describe, expect, it, vi } from "vitest"; import { createInlineCodeState } from "../../packages/markdown-core/src/code-spans.js"; +import { createHookRunner } from "../plugins/hooks.js"; +import { createMockPluginRegistry, TEST_PLUGIN_AGENT_CTX } from "../plugins/hooks.test-helpers.js"; import { handleAgentEnd, handleAgentStart } from "./embedded-agent-subscribe.handlers.lifecycle.js"; import type { EmbeddedAgentSubscribeContext } from "./embedded-agent-subscribe.handlers.types.js"; const { emitAgentEventMock } = vi.hoisted(() => ({ emitAgentEventMock: vi.fn(), })); +const DEFAULT_BEFORE_AGENT_FINALIZE_TIMEOUT_MS = 15_000; +const BEFORE_AGENT_FINALIZE_EVENT = { + runId: "run-1", + sessionId: "session-1", + sessionKey: "agent:main:session-1", + turnId: "turn-1", + provider: "openai", + model: "freeze-e2e", + cwd: "/repo", + transcriptPath: "/tmp/session.jsonl", + stopHookActive: false, + lastAssistantMessage: "done", +}; vi.mock("../infra/agent-events.js", () => ({ emitAgentEvent: emitAgentEventMock, @@ -18,6 +33,7 @@ function createContext( overrides?: { onAgentEvent?: (event: unknown) => void; onBeforeLifecycleTerminal?: () => void | Promise; + onBeforeTerminalDelivery?: () => void | Promise; onBlockReply?: ((payload: unknown) => void) | undefined; onBlockReplyFlush?: () => void | Promise; resolveTerminalStopReason?: () => string | undefined; @@ -35,6 +51,7 @@ function createContext( sessionKey: "agent:main:main", onAgentEvent: overrides?.onAgentEvent, onBeforeLifecycleTerminal: overrides?.onBeforeLifecycleTerminal, + onBeforeTerminalDelivery: overrides?.onBeforeTerminalDelivery, resolveTerminalStopReason: overrides?.resolveTerminalStopReason, ...(onBlockReply ? { onBlockReply } : {}), onBlockReplyFlush: overrides?.onBlockReplyFlush, @@ -842,6 +859,59 @@ describe("handleAgentEnd", () => { await endPromise; }); + it("resolves compaction retry after a timed-out terminal hook finalizes the original answer", async () => { + vi.useFakeTimers(); + try { + const logger = { error: vi.fn(), warn: vi.fn(), debug: vi.fn() }; + const runner = createHookRunner( + createMockPluginRegistry([ + { + hookName: "before_agent_finalize", + handler: vi.fn(() => new Promise(() => {})), + }, + ]), + { logger }, + ); + const onBeforeTerminalDelivery = vi.fn(async () => { + await runner.runBeforeAgentFinalize(BEFORE_AGENT_FINALIZE_EVENT, TEST_PLUGIN_AGENT_CTX); + return undefined; + }); + const ctx = createContext( + { + role: "assistant", + content: [{ type: "text", text: "done" }], + stopReason: "stop", + }, + { onBeforeTerminalDelivery }, + ); + ctx.state.assistantTexts = ["done"]; + ctx.state.pendingCompactionRetry = 1; + + const endPromise = handleAgentEnd(ctx); + await Promise.resolve(); + + expect(onBeforeTerminalDelivery).toHaveBeenCalledTimes(1); + expect(ctx.resolveCompactionRetry).not.toHaveBeenCalled(); + expect(ctx.flushBlockReplyBuffer).not.toHaveBeenCalledWith({ final: true }); + + await vi.advanceTimersByTimeAsync(DEFAULT_BEFORE_AGENT_FINALIZE_TIMEOUT_MS); + await endPromise; + + expect(logger.error).toHaveBeenCalledWith( + "[hooks] before_agent_finalize handler from test-plugin failed: timed out after 15000ms", + ); + expect(ctx.clearDeferredAssistantEvents).not.toHaveBeenCalled(); + expect(ctx.clearDeferredBlockReplies).not.toHaveBeenCalled(); + expect(ctx.flushDeferredAssistantEvents).toHaveBeenCalledTimes(1); + expect(ctx.flushDeferredBlockReplies).toHaveBeenCalledTimes(1); + expect(ctx.flushBlockReplyBuffer).toHaveBeenCalledWith({ final: true }); + expect(ctx.resolveCompactionRetry).toHaveBeenCalledTimes(1); + expect(ctx.maybeResolveCompactionWait).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + it("runs the before-lifecycle callback before the lifecycle end event", async () => { const order: string[] = []; const onAgentEvent = vi.fn(() => { diff --git a/src/plugins/hooks.before-agent-finalize.test.ts b/src/plugins/hooks.before-agent-finalize.test.ts index 215594d3018d..1ff50711a36a 100644 --- a/src/plugins/hooks.before-agent-finalize.test.ts +++ b/src/plugins/hooks.before-agent-finalize.test.ts @@ -15,6 +15,7 @@ const EVENT = { stopHookActive: false, lastAssistantMessage: "done", }; +const DEFAULT_BEFORE_AGENT_FINALIZE_TIMEOUT_MS = 15_000; describe("before_agent_finalize hook runner", () => { it("returns undefined when no hooks are registered", async () => { @@ -202,6 +203,28 @@ describe("before_agent_finalize hook runner", () => { }); }); + it("times out hung handlers and continues with the original final answer", async () => { + vi.useFakeTimers(); + try { + const handler = vi.fn(() => new Promise(() => {})); + const logger = { error: vi.fn(), warn: vi.fn(), debug: vi.fn() }; + const runner = createHookRunner( + createMockPluginRegistry([{ hookName: "before_agent_finalize", handler }]), + { logger }, + ); + + const run = runner.runBeforeAgentFinalize(EVENT, TEST_PLUGIN_AGENT_CTX); + + await vi.advanceTimersByTimeAsync(DEFAULT_BEFORE_AGENT_FINALIZE_TIMEOUT_MS); + await expect(run).resolves.toBeUndefined(); + expect(logger.error).toHaveBeenCalledWith( + "[hooks] before_agent_finalize handler from test-plugin failed: timed out after 15000ms", + ); + } finally { + vi.useRealTimers(); + } + }); + it("hasHooks reports correctly", () => { const runner = createHookRunner( createMockPluginRegistry([{ hookName: "before_agent_finalize", handler: vi.fn() }]), diff --git a/src/plugins/hooks.ts b/src/plugins/hooks.ts index 57abb8ce3ffa..902c32b39fc0 100644 --- a/src/plugins/hooks.ts +++ b/src/plugins/hooks.ts @@ -228,6 +228,10 @@ const DEFAULT_MODIFYING_HOOK_TIMEOUT_MS_BY_HOOK: Partial