From d29eaeafc1fc1ca86f0ccb5c6b2b3d3668502132 Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Thu, 23 Apr 2026 23:16:26 -0700 Subject: [PATCH] fix(cli-runner): fire before_agent_reply on cron-triggered turns (#70950) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(cli-runner): RED — assert before_agent_reply fires on cron triggers Mirrors src/agents/pi-embedded-runner/run.before-agent-reply-cron.test.ts for the CLI runner. Asserts: 1. When trigger=cron and a before_agent_reply hook claims the turn (handled: true), runCliAgent must NOT invoke the codex subprocess and must return the hook's reply text in payloads[0]. 2. When the hook claims without a reply body, the synthesized payload uses SILENT_REPLY_TOKEN. 3. Non-cron triggers do not invoke the hook (no behavior change for normal user/heartbeat traffic). 4. Without a registered hook, falls through to the CLI subprocess. Currently fails (RED): tests 1 and 2 fail because runCliAgent never fires before_agent_reply — the hook gate exists only in the embedded PI runner (src/agents/pi-embedded-runner/run.ts:326). This is the CLI-backed-agent dreaming gap reported in #70940 and identified in PR #70737 review. Next commit: implement the hook gate in runPreparedCliAgent (GREEN). * fix(cli-runner): GREEN — fire before_agent_reply for cron-triggered turns Mirrors the embedded PI runner gate from src/agents/pi-embedded-runner/run.ts:326 so plugin-managed cron jobs (notably memory-core dreaming) can short-circuit a CLI-backed agent turn before the codex/claude/gemini subprocess is spawned. Without this, configuring a default agent's model to a CLI backend (codex-cli, claude-cli, gemini-cli, or any third-party `registerCliBackend` provider) silently broke dreaming: the cron sentinel was sent to the underlying LLM as a literal user prompt and the dreaming hook never executed. See openclaw/openclaw#70940 for the empirical repro (codex-cli observed sending the dream-token to GPT-5.5 with no `memory-core: dreaming promotion complete` line). Also extracts `buildHandledReplyPayloads` locally; eventually that should be unified with the embedded PI runner's helper, but that's a mechanical refactor for a follow-up. Closes #70940 once both this PR and #70737 land — this fix is only useful if cron-driven dreaming exists, which is what #70737 introduces. TDD trail: - prior commit: RED test asserting the hook gate (4 cases) - this commit: implementation that turns those tests green (4/4 pass). Verified: pnpm test src/agents/cli-runner.before-agent-reply-cron.test.ts 4/4 passed; pnpm test src/agents/cli-runner 21/21 passed; lint clean on touched files; pre-existing tsgo failure in src/plugin-sdk/provider-tools.ts is unrelated to these changes. --- ...cli-runner.before-agent-reply-cron.test.ts | 167 ++++++++++++++++++ src/agents/cli-runner.ts | 54 ++++++ 2 files changed, 221 insertions(+) create mode 100644 src/agents/cli-runner.before-agent-reply-cron.test.ts diff --git a/src/agents/cli-runner.before-agent-reply-cron.test.ts b/src/agents/cli-runner.before-agent-reply-cron.test.ts new file mode 100644 index 00000000000..72418f80b7e --- /dev/null +++ b/src/agents/cli-runner.before-agent-reply-cron.test.ts @@ -0,0 +1,167 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { SILENT_REPLY_TOKEN } from "../auto-reply/tokens.js"; + +// vi.mock factories are hoisted above imports, so any references inside them +// must come from vi.hoisted() so they exist at hoist time (otherwise they'd +// be TDZ-undefined and the mocks would silently misbehave). This test only +// exercises the hook-gate decision at the runCliAgent entry point — we mock +// the prepareCliRunContext + executePreparedCliRun seams so no broader CLI +// runtime needs to load. +type BeforeAgentReplyResult = + | undefined + | { + handled?: boolean; + reply?: { text?: string }; + }; + +const { + hasHooksMock, + runBeforeAgentReplyMock, + executePreparedCliRunMock, + prepareCliRunContextMock, +} = vi.hoisted(() => ({ + hasHooksMock: vi.fn<(hookName: string) => boolean>(() => false), + runBeforeAgentReplyMock: vi.fn<(event: unknown, ctx: unknown) => Promise>( + async () => undefined, + ), + executePreparedCliRunMock: vi.fn(async (_context: unknown, _cliSessionIdToUse?: string) => ({ + text: "", + })), + prepareCliRunContextMock: vi.fn(), +})); + +vi.mock("../plugins/hook-runner-global.js", () => ({ + getGlobalHookRunner: vi.fn(() => ({ + hasHooks: hasHooksMock, + runBeforeAgentReply: runBeforeAgentReplyMock, + })), +})); + +vi.mock("./cli-runner/prepare.runtime.js", () => ({ + prepareCliRunContext: prepareCliRunContextMock, +})); + +vi.mock("./cli-runner/execute.runtime.js", () => ({ + executePreparedCliRun: executePreparedCliRunMock, +})); + +const baseRunParams = { + sessionId: "test-session", + sessionKey: "test-session-key", + agentId: "main", + sessionFile: "/tmp/test-session.jsonl", + workspaceDir: "/tmp/test-workspace", + prompt: "__openclaw_memory_core_short_term_promotion_dream__", + provider: "codex-cli", + model: "gpt-5.5", + timeoutMs: 30_000, + runId: "test-run-id", +} as const; + +function makeStubContext(params: typeof baseRunParams & { trigger?: string }) { + return { + params, + started: Date.now(), + workspaceDir: params.workspaceDir, + modelId: params.model, + normalizedModel: params.model, + systemPrompt: "", + systemPromptReport: {}, + bootstrapPromptWarningLines: [], + authEpochVersion: 0, + backendResolved: {}, + preparedBackend: {}, + reusableCliSession: {}, + } as unknown; +} + +beforeEach(() => { + hasHooksMock.mockReset(); + hasHooksMock.mockReturnValue(false); + runBeforeAgentReplyMock.mockReset(); + runBeforeAgentReplyMock.mockResolvedValue(undefined); + executePreparedCliRunMock.mockReset(); + executePreparedCliRunMock.mockResolvedValue({ text: "" }); + prepareCliRunContextMock.mockReset(); + prepareCliRunContextMock.mockImplementation(async (params) => + makeStubContext(params as typeof baseRunParams & { trigger?: string }), + ); +}); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe("runCliAgent cron before_agent_reply seam", () => { + it("lets before_agent_reply claim cron runs before the CLI subprocess is invoked", async () => { + const { runCliAgent } = await import("./cli-runner.js"); + hasHooksMock.mockImplementation((hookName) => hookName === "before_agent_reply"); + runBeforeAgentReplyMock.mockResolvedValue({ + handled: true, + reply: { text: "dreaming claimed via cli runner" }, + }); + + const result = await runCliAgent({ ...baseRunParams, trigger: "cron" }); + + expect(runBeforeAgentReplyMock).toHaveBeenCalledTimes(1); + expect(runBeforeAgentReplyMock).toHaveBeenCalledWith( + { cleanedBody: baseRunParams.prompt }, + expect.objectContaining({ + agentId: baseRunParams.agentId, + sessionId: baseRunParams.sessionId, + sessionKey: baseRunParams.sessionKey, + workspaceDir: baseRunParams.workspaceDir, + trigger: "cron", + }), + ); + expect(executePreparedCliRunMock).not.toHaveBeenCalled(); + expect(result.payloads?.[0]?.text).toBe("dreaming claimed via cli runner"); + }); + + it("does not run prepareCliRunContext when the cron hook claims (no resource allocation, no leak)", async () => { + // Regression for PR #70950 review (greptile-apps, P1): the gate must fire + // before any backend resources are allocated, otherwise preparedBackend.cleanup + // is silently skipped on every claimed cron turn. + const { runCliAgent } = await import("./cli-runner.js"); + hasHooksMock.mockImplementation((hookName) => hookName === "before_agent_reply"); + runBeforeAgentReplyMock.mockResolvedValue({ handled: true }); + + await runCliAgent({ ...baseRunParams, trigger: "cron" }); + + expect(prepareCliRunContextMock).not.toHaveBeenCalled(); + expect(executePreparedCliRunMock).not.toHaveBeenCalled(); + }); + + it("returns a silent payload when a cron hook claims without a reply body", async () => { + const { runCliAgent } = await import("./cli-runner.js"); + hasHooksMock.mockImplementation((hookName) => hookName === "before_agent_reply"); + runBeforeAgentReplyMock.mockResolvedValue({ handled: true }); + + const result = await runCliAgent({ ...baseRunParams, trigger: "cron" }); + + expect(executePreparedCliRunMock).not.toHaveBeenCalled(); + expect(result.payloads?.[0]?.text).toBe(SILENT_REPLY_TOKEN); + }); + + it("does not invoke before_agent_reply for non-cron triggers", async () => { + const { runCliAgent } = await import("./cli-runner.js"); + hasHooksMock.mockImplementation((hookName) => hookName === "before_agent_reply"); + executePreparedCliRunMock.mockResolvedValue({ text: "real reply" }); + + await runCliAgent({ ...baseRunParams, trigger: "user" }); + + expect(runBeforeAgentReplyMock).not.toHaveBeenCalled(); + expect(executePreparedCliRunMock).toHaveBeenCalled(); + }); + + it("falls through to the CLI subprocess when no before_agent_reply hook is registered", async () => { + const { runCliAgent } = await import("./cli-runner.js"); + hasHooksMock.mockReturnValue(false); + executePreparedCliRunMock.mockResolvedValue({ text: "real reply" }); + + await runCliAgent({ ...baseRunParams, trigger: "cron" }); + + expect(runBeforeAgentReplyMock).not.toHaveBeenCalled(); + expect(executePreparedCliRunMock).toHaveBeenCalled(); + }); +}); diff --git a/src/agents/cli-runner.ts b/src/agents/cli-runner.ts index 41d28529340..ecb23331387 100644 --- a/src/agents/cli-runner.ts +++ b/src/agents/cli-runner.ts @@ -1,3 +1,5 @@ +import type { ReplyPayload } from "../auto-reply/reply-payload.js"; +import { SILENT_REPLY_TOKEN } from "../auto-reply/tokens.js"; import { formatErrorMessage } from "../infra/errors.js"; import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import { loadCliSessionHistoryMessages } from "./cli-runner/session-history.js"; @@ -12,6 +14,21 @@ import { import { classifyFailoverReason, isFailoverErrorMessage } from "./pi-embedded-helpers.js"; import type { EmbeddedPiRunResult } from "./pi-embedded-runner.js"; +function buildHandledReplyPayloads(reply?: ReplyPayload) { + const normalized = reply ?? { text: SILENT_REPLY_TOKEN }; + return [ + { + text: normalized.text, + mediaUrl: normalized.mediaUrl, + mediaUrls: normalized.mediaUrls, + replyToId: normalized.replyToId, + audioAsVoice: normalized.audioAsVoice, + isError: normalized.isError, + isReasoning: normalized.isReasoning, + }, + ]; +} + function buildCliHookUserMessage(prompt: string): unknown { return { role: "user", @@ -45,6 +62,43 @@ function buildCliHookAssistantMessage(params: { } export async function runCliAgent(params: RunCliAgentParams): Promise { + // Cron gate must fire before prepareCliRunContext — that call allocates + // backend resources released only by runPreparedCliAgent's try…finally. + if (params.trigger === "cron") { + const startedAt = Date.now(); + const hookRunner = getGlobalHookRunner(); + if (hookRunner?.hasHooks("before_agent_reply")) { + const hookContext = { + runId: params.runId, + agentId: params.agentId, + sessionKey: params.sessionKey, + sessionId: params.sessionId, + workspaceDir: params.workspaceDir, + messageProvider: params.messageProvider, + trigger: params.trigger, + channelId: params.messageChannel ?? params.messageProvider, + } as const; + const hookResult = await hookRunner.runBeforeAgentReply( + { cleanedBody: params.prompt }, + hookContext, + ); + if (hookResult?.handled) { + return { + payloads: buildHandledReplyPayloads(hookResult.reply), + meta: { + durationMs: Date.now() - startedAt, + agentMeta: { + sessionId: params.sessionId, + provider: params.provider, + model: params.model ?? "", + }, + finalAssistantVisibleText: hookResult.reply?.text ?? SILENT_REPLY_TOKEN, + finalAssistantRawText: hookResult.reply?.text ?? SILENT_REPLY_TOKEN, + }, + }; + } + } + } const { prepareCliRunContext } = await import("./cli-runner/prepare.runtime.js"); const context = await prepareCliRunContext(params); return runPreparedCliAgent(context);