Skip to content

Commit efbefce

Browse files
vincentkocvincentkoc
committed
fix(cron): preserve explicit delivery and timeout semantics
1 parent 1c30bb8 commit efbefce

16 files changed

Lines changed: 106 additions & 18 deletions

‎src/agents/cli-runner/prepare.test.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,8 @@ function createTestMcpLoopbackServerConfig(port: number) {
126126
"x-openclaw-current-inbound-audio": "${OPENCLAW_MCP_CURRENT_INBOUND_AUDIO}",
127127
"x-openclaw-inbound-event-kind": "${OPENCLAW_MCP_INBOUND_EVENT_KIND}",
128128
"x-openclaw-source-reply-delivery-mode": "${OPENCLAW_MCP_SOURCE_REPLY_DELIVERY_MODE}",
129+
"x-openclaw-require-explicit-message-target":
130+
"${OPENCLAW_MCP_REQUIRE_EXPLICIT_MESSAGE_TARGET}",
129131
},
130132
},
131133
},
@@ -1586,6 +1588,7 @@ describe("shouldSkipLocalCliCredentialEpoch", () => {
15861588
currentMessageId: "reply-message-1",
15871589
currentInboundAudio: true,
15881590
sourceReplyDeliveryMode: "message_tool_only",
1591+
requireExplicitMessageTarget: true,
15891592
});
15901593

15911594
expect(context.preparedBackend.env).toMatchObject({
@@ -1596,6 +1599,7 @@ describe("shouldSkipLocalCliCredentialEpoch", () => {
15961599
OPENCLAW_MCP_CURRENT_INBOUND_AUDIO: "true",
15971600
OPENCLAW_MCP_INBOUND_EVENT_KIND: "room_event",
15981601
OPENCLAW_MCP_SOURCE_REPLY_DELIVERY_MODE: "message_tool_only",
1602+
OPENCLAW_MCP_REQUIRE_EXPLICIT_MESSAGE_TARGET: "true",
15991603
});
16001604
} finally {
16011605
fs.rmSync(dir, { recursive: true, force: true });

‎src/agents/cli-runner/prepare.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,8 @@ export async function prepareCliRunContext(
385385
OPENCLAW_MCP_CURRENT_INBOUND_AUDIO: params.currentInboundAudio === true ? "true" : "",
386386
OPENCLAW_MCP_INBOUND_EVENT_KIND: params.currentInboundEventKind ?? "",
387387
OPENCLAW_MCP_SOURCE_REPLY_DELIVERY_MODE: params.sourceReplyDeliveryMode ?? "",
388+
OPENCLAW_MCP_REQUIRE_EXPLICIT_MESSAGE_TARGET:
389+
params.requireExplicitMessageTarget === true ? "true" : "",
388390
}
389391
: undefined,
390392
warn: (message) => cliBackendLog.warn(message),

‎src/agents/cli-runner/types.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ export type RunCliAgentParams = {
7575
jobId?: string;
7676
extraSystemPrompt?: string;
7777
sourceReplyDeliveryMode?: SourceReplyDeliveryMode;
78+
requireExplicitMessageTarget?: boolean;
7879
silentReplyPromptMode?: SilentReplyPromptMode;
7980
allowEmptyAssistantReplyAsSilent?: boolean;
8081
/** Static portion of extraSystemPrompt (excluding per-message inbound metadata) for session reuse hashing. */

‎src/cron/isolated-agent/run-executor.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ export function createCronPromptExecutor(params: {
249249
skillsSnapshot: params.skillsSnapshot,
250250
messageChannel,
251251
sourceReplyDeliveryMode,
252+
requireExplicitMessageTarget: params.sourceDelivery.messageTool.requireExplicitTarget,
252253
abortSignal: params.abortSignal,
253254
onExecutionStarted: params.onExecutionStarted,
254255
onExecutionPhase: params.onExecutionPhase,

‎src/cron/isolated-agent/run.message-tool-policy.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
cleanupDirectCronSessionMock,
1010
dispatchCronDeliveryMock,
1111
getChannelPluginMock,
12+
isCliProviderMock,
1213
isHeartbeatOnlyResponseMock,
1314
loadRunCronIsolatedAgentTurn,
1415
makeCronSession,
@@ -20,6 +21,7 @@ import {
2021
resolveCronDeliveryPlanMock,
2122
resolveDeliveryTargetMock,
2223
restoreFastTestEnv,
24+
runCliAgentMock,
2325
runEmbeddedAgentMock,
2426
} from "./run.test-harness.js";
2527

@@ -347,6 +349,7 @@ describe("runCronIsolatedAgentTurn message tool policy", () => {
347349
},
348350
messageToolEnabled: true,
349351
messageToolForced: false,
352+
requireExplicitMessageTarget: true,
350353
requireExplicitMessageTargetEvidence: true,
351354
directFallback: true,
352355
}),
@@ -705,10 +708,37 @@ describe("runCronIsolatedAgentTurn message tool policy", () => {
705708
expectEmbeddedRunFields({
706709
sourceReplyDeliveryMode: undefined,
707710
forceMessageTool: false,
711+
requireExplicitMessageTarget: true,
708712
messageChannel: "messagechat",
709713
messageTo: "123",
710714
currentChannelId: "123",
711715
});
716+
expect(expectEmbeddedRunPrompt()).toContain("with an explicit target");
717+
});
718+
719+
it("requires explicit message targets for CLI-backed announce delivery", async () => {
720+
mockRunCronFallbackPassthrough();
721+
resolveCronDeliveryPlanMock.mockReturnValue(makeAnnounceDeliveryPlan());
722+
isCliProviderMock.mockReturnValue(true);
723+
runCliAgentMock.mockResolvedValue({
724+
payloads: [{ text: "done" }],
725+
meta: { agentMeta: { usage: { input: 10, output: 20 } } },
726+
});
727+
728+
await runCronIsolatedAgentTurn({
729+
...makeParams(),
730+
job: makeAnnounceMessageToolJob(),
731+
});
732+
733+
expect(runCliAgentMock).toHaveBeenCalledTimes(1);
734+
expectRecordFields(
735+
getMockCallArg(runCliAgentMock, 0, 0, "CLI run"),
736+
{
737+
messageChannel: "messagechat",
738+
requireExplicitMessageTarget: true,
739+
},
740+
"CLI run params",
741+
);
712742
});
713743

714744
it("keeps automatic exec completion notifications when announce delivery is active", async () => {

‎src/cron/isolated-agent/run.meta-error-status.test.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,11 @@ describe("runCronIsolatedAgentTurn - meta.error status propagation", () => {
6969

7070
it("keeps cron timeout result when executor rejects after the cron abort signal fires", async () => {
7171
const abortController = new AbortController();
72-
abortController.abort("cron: job execution timed out (last phase: model_call_started)");
72+
const timeoutError = new Error(
73+
"cron: job execution timed out (last phase: model_call_started)",
74+
);
75+
timeoutError.name = "TimeoutError";
76+
abortController.abort(timeoutError);
7377
runWithModelFallbackMock.mockRejectedValueOnce(
7478
new Error(
7579
'All models failed (2): openai/gpt-5.5: Command lane "cron-nested" task timed out after 330000ms (timeout)',

‎src/cron/isolated-agent/run.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import {
4646
createCronRunDiagnosticsFromError,
4747
mergeCronRunDiagnostics,
4848
} from "../run-diagnostics.js";
49+
import { resolveCronAbortReasonText } from "../service/execution-errors.js";
4950
import type {
5051
CronAgentExecutionPhaseUpdate,
5152
CronAgentExecutionStarted,
@@ -341,6 +342,7 @@ function resolveCronSourceDeliveryPlan(params: {
341342
target,
342343
messageToolEnabled: true,
343344
messageToolForced: false,
345+
requireExplicitMessageTarget: true,
344346
requireExplicitMessageTargetEvidence: true,
345347
directFallback: true,
346348
skipFallbackWhenMessageToolSentToTarget: params.resolvedDelivery.ok,
@@ -425,14 +427,16 @@ function appendCronDeliveryInstruction(params: {
425427
deliveryRequested: boolean;
426428
messageToolEnabled: boolean;
427429
resolvedDeliveryOk: boolean;
430+
requireExplicitMessageTarget: boolean;
428431
}) {
429432
if (!params.deliveryRequested) {
430433
return params.commandBody;
431434
}
432435
if (params.messageToolEnabled) {
433-
const targetHint = params.resolvedDeliveryOk
434-
? "for the current chat"
435-
: "with an explicit target";
436+
const targetHint =
437+
params.requireExplicitMessageTarget || !params.resolvedDeliveryOk
438+
? "with an explicit target"
439+
: "for the current chat";
436440
return `${params.commandBody}\n\nUse the message tool if you need to notify the user directly ${targetHint}. If you do not send directly, your final plain-text reply will be delivered automatically.`.trim();
437441
}
438442
return `${params.commandBody}\n\nReturn your response as plain text; it will be delivered automatically. If the task explicitly calls for messaging a specific external recipient, note who/where it should go instead of sending it yourself.`.trim();
@@ -833,6 +837,7 @@ async function prepareCronRunContext(params: {
833837
toolsAllow: agentPayload?.toolsAllow,
834838
}),
835839
resolvedDeliveryOk: resolvedDelivery.ok,
840+
requireExplicitMessageTarget: sourceDelivery.messageTool.requireExplicitTarget,
836841
});
837842

838843
const skillsSnapshot = await resolveCronSkillsSnapshot({
@@ -1271,12 +1276,8 @@ export async function runCronIsolatedAgentTurn(params: {
12711276
const admittedLifecycleGeneration = getAgentEventLifecycleGeneration();
12721277
const abortSignal = params.abortSignal ?? params.signal;
12731278
const isAborted = () => abortSignal?.aborted === true;
1274-
const abortReason = () => {
1275-
const reason = abortSignal?.reason;
1276-
return typeof reason === "string" && reason.trim()
1277-
? reason.trim()
1278-
: "cron: job execution timed out";
1279-
};
1279+
const abortReason = () =>
1280+
resolveCronAbortReasonText(abortSignal?.reason) ?? "cron: job execution timed out";
12801281
const isFastTestEnv = process.env.OPENCLAW_TEST_FAST === "1";
12811282
const prepared = await prepareCronRunContext({ input: params, isFastTestEnv });
12821283
if (!prepared.ok) {

‎src/cron/service/execution-errors.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,19 @@ export function preExecutionTimeoutErrorMessage(execution?: CronAgentExecutionSt
3939
}
4040

4141
/** Extracts a human timeout/abort reason, falling back to the canonical cron timeout text. */
42-
export function abortErrorMessage(signal?: AbortSignal): string {
43-
const reason = signal?.reason;
42+
export function resolveCronAbortReasonText(reason: unknown): string | undefined {
4443
if (typeof reason === "string" && reason.trim()) {
4544
return reason.trim();
4645
}
47-
return timeoutErrorMessage();
46+
if (reason instanceof Error && reason.message.trim()) {
47+
return reason.message.trim();
48+
}
49+
return undefined;
50+
}
51+
52+
/** Extracts a human timeout/abort reason, falling back to the canonical cron timeout text. */
53+
export function abortErrorMessage(signal?: AbortSignal): string {
54+
return resolveCronAbortReasonText(signal?.reason) ?? timeoutErrorMessage();
4855
}
4956

5057
function isAbortError(err: unknown): boolean {

‎src/cron/service/timer.regression.test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2527,6 +2527,7 @@ describe("cron service timer regressions", () => {
25272527
let now = scheduledAt;
25282528
const wallStart = Date.now();
25292529
let abortWallMs: number | undefined;
2530+
let abortReason: unknown;
25302531
const started = createDeferred<void>();
25312532

25322533
const state = createCronServiceState({
@@ -2553,13 +2554,15 @@ describe("cron service timer regressions", () => {
25532554
}
25542555
if (abortSignal.aborted) {
25552556
abortWallMs = Date.now();
2557+
abortReason = abortSignal.reason;
25562558
resolve();
25572559
return;
25582560
}
25592561
abortSignal.addEventListener(
25602562
"abort",
25612563
() => {
25622564
abortWallMs = Date.now();
2565+
abortReason = abortSignal.reason;
25632566
resolve();
25642567
},
25652568
{ once: true },
@@ -2582,6 +2585,10 @@ describe("cron service timer regressions", () => {
25822585

25832586
const elapsedMs = (abortWallMs ?? Date.now()) - wallStart;
25842587
expect(elapsedMs).toBeGreaterThanOrEqual(timeoutSeconds * 1_000);
2588+
expect(abortReason).toMatchObject({
2589+
name: "TimeoutError",
2590+
message: "cron: job execution timed out",
2591+
});
25852592

25862593
const job = state.store?.jobs.find((entry) => entry.id === "timeout-fraction-29774");
25872594
expect(job?.state.lastStatus).toBe("error");
@@ -2872,6 +2879,7 @@ describe("cron service timer regressions", () => {
28722879
let now = scheduledAt;
28732880
const started = createDeferred<void>();
28742881
let abortObserved = false;
2882+
let abortReason: unknown;
28752883
const cleanupTimedOutAgentRun = vi.fn(async () => {});
28762884
const onIsolatedAgentSetupTimeout = vi.fn();
28772885
const state = createCronServiceState({
@@ -2912,6 +2920,7 @@ describe("cron service timer regressions", () => {
29122920
"abort",
29132921
() => {
29142922
abortObserved = true;
2923+
abortReason = abortSignal.reason;
29152924
},
29162925
{ once: true },
29172926
);
@@ -2931,6 +2940,10 @@ describe("cron service timer regressions", () => {
29312940
expect(job.state.lastStatus).toBe("error");
29322941
expect(job.state.lastError).toContain("stalled before execution start");
29332942
expect(job.state.lastError).toContain("context-engine");
2943+
expect(abortReason).toMatchObject({
2944+
name: "TimeoutError",
2945+
message: expect.stringContaining("context-engine"),
2946+
});
29342947
expect(cleanupTimedOutAgentRun).toHaveBeenCalledTimes(1);
29352948
const cleanupArgs = requireRecord(firstMockArg(cleanupTimedOutAgentRun));
29362949
expect(requireRecord(cleanupArgs.job).id).toBe("isolated-pre-model-timeout-74803");

‎src/cron/service/timer.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,9 @@ export async function executeJobCoreWithTimeout(
227227
const triggerTimeout = (reason: string) => {
228228
timeoutReason = reason;
229229
if (!runAbortController.signal.aborted) {
230-
runAbortController.abort(reason);
230+
const timeoutError = new Error(reason);
231+
timeoutError.name = "TimeoutError";
232+
runAbortController.abort(timeoutError);
231233
}
232234
resolveTimeout?.(timeoutMarker);
233235
};

0 commit comments

Comments
 (0)