mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 09:50:42 +00:00
fix(cli-runner): fire before_agent_reply on cron-triggered turns (#70950)
* 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.
This commit is contained in:
167
src/agents/cli-runner.before-agent-reply-cron.test.ts
Normal file
167
src/agents/cli-runner.before-agent-reply-cron.test.ts
Normal file
@@ -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<BeforeAgentReplyResult>>(
|
||||
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();
|
||||
});
|
||||
});
|
||||
@@ -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<EmbeddedPiRunResult> {
|
||||
// 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);
|
||||
|
||||
Reference in New Issue
Block a user