fix(cron): surface successful run delivery errors to run logs

Alix-007 · Alix-007 · commit 0b88da287bea · 2026-07-01T00:30:59.000+08:00
A successful isolated agent turn keeps status=ok when post-run delivery fails (#94058), but the ok/not-delivered resolveRunOutcome branch only forwarded delivery diagnostics and dropped the delivery dispatch error. That left lastDeliveryError empty and the finished-event deliveryError field blank, so CLI/UI/API run logs could not show why delivery did not land for an otherwise successful run. Thread the delivery error on a dedicated CronRunOutcome.deliveryError field from resolveRunOutcome through executeDetachedCronJob and applyJobResult into resolveDeliveryState, so it persists as lastDeliveryError and reaches the finished event (and thus the run log) without conflating it with a run-level error (lastError stays empty on a successful run). Add a service/run-log readback test proving the delivery error survives the cron boundary into a persisted run-log entry, and extend the isolated-turn regression to assert the dedicated deliveryError field.
diff --git a/src/cron/isolated-agent/run.message-tool-policy.test.ts b/src/cron/isolated-agent/run.message-tool-policy.test.ts
@@ -1865,6 +1865,11 @@ describe("runCronIsolatedAgentTurn delivery instruction", () => {
     // Execution succeeded: status stays ok despite the delivery failure.
     expect(result.status).toBe("ok");
     expect(result.error).toBeUndefined();
+    // The delivery dispatch error is surfaced on a dedicated `deliveryError`
+    // field (not the run-level `error`) so the service can persist it as
+    // `lastDeliveryError` and emit it on the finished event for CLI/UI/API run
+    // logs (#95419) without mislabeling the successful run as a failure.
+    expect(result.deliveryError).toBe("Message failed");
     // Delivery failure metadata is preserved and decoupled from status.
     expect(result.delivered).toBe(false);
     expect(result.deliveryAttempted).toBe(true);
diff --git a/src/cron/isolated-agent/run.ts b/src/cron/isolated-agent/run.ts
@@ -1233,12 +1233,19 @@ async function finalizeCronRun(params: {
     deliveryAttempted?: boolean;
     delivery?: CronDeliveryTrace;
     deliveryDiagnostics?: RunCronAgentTurnResult["diagnostics"];
+    deliveryError?: string;
   }) =>
     prepared.withRunSession({
       status: hasFatalErrorPayload ? "error" : "ok",
       ...(hasFatalErrorPayload
         ? { error: embeddedRunError ?? "cron isolated run returned an error payload" }
         : {}),
+      // A delivery dispatch failure on an otherwise successful run keeps
+      // `status: "ok"` (#94058); surface the delivery error separately so the
+      // service can persist it as `lastDeliveryError` and emit it on the
+      // finished event for CLI/UI/API run logs (#95419) without mislabeling
+      // the run-level `error`.
+      ...(result?.deliveryError ? { deliveryError: result.deliveryError } : {}),
       summary,
       outputText,
       delivered: result?.delivered,
@@ -1384,6 +1391,10 @@ async function finalizeCronRun(params: {
           deliveryAttempted: resultWithDeliveryMeta.deliveryAttempted,
           delivery: deliveryTrace,
           deliveryDiagnostics: resultWithDeliveryMeta.diagnostics,
+          // Carry the delivery dispatch error so it persists as
+          // `lastDeliveryError` and reaches CLI/UI/API run logs (#95419)
+          // instead of being dropped when status is kept `ok`.
+          deliveryError: deliveryResult.result.error,
         });
       }
       return resultWithDeliveryMeta;
diff --git a/src/cron/service.persists-delivered-status.test.ts b/src/cron/service.persists-delivered-status.test.ts
@@ -1,5 +1,6 @@
 // Delivered status tests cover persistence of cron delivery outcomes.
 import { describe, expect, it, vi } from "vitest";
+import { appendCronRunLog, readCronRunLogEntriesSync } from "./run-log.js";
 import { CronService } from "./service.js";
 import {
   createFinishedBarrier,
@@ -92,10 +93,13 @@ function createIsolatedCronWithFinishedBarrier(params: {
   status?: "ok" | "error";
   delivered?: boolean;
   error?: string;
+  deliveryError?: string;
   onFinished?: (evt: {
     jobId: string;
+    error?: string;
     delivered?: boolean;
     deliveryStatus?: string;
+    deliveryError?: string;
     failureNotificationDelivery?: {
       delivered?: boolean;
       status: string;
@@ -114,14 +118,17 @@ function createIsolatedCronWithFinishedBarrier(params: {
       status: params.status ?? ("ok" as const),
       summary: "done",
       ...(params.error === undefined ? {} : { error: params.error }),
+      ...(params.deliveryError === undefined ? {} : { deliveryError: params.deliveryError }),
       ...(params.delivered === undefined ? {} : { delivered: params.delivered }),
     })),
     onEvent: (evt) => {
       if (evt.action === "finished") {
         params.onFinished?.({
           jobId: evt.jobId,
+          error: evt.error,
           delivered: evt.delivered,
           deliveryStatus: evt.deliveryStatus,
+          deliveryError: evt.deliveryError,
           failureNotificationDelivery: evt.failureNotificationDelivery,
         });
       }
@@ -190,10 +197,13 @@ async function runIsolatedJobAndReadState(params: {
   status?: "ok" | "error";
   delivered?: boolean;
   error?: string;
+  deliveryError?: string;
   onFinished?: (evt: {
     jobId: string;
+    error?: string;
     delivered?: boolean;
     deliveryStatus?: string;
+    deliveryError?: string;
     failureNotificationDelivery?: {
       delivered?: boolean;
       status: string;
@@ -208,6 +218,7 @@ async function runIsolatedJobAndReadState(params: {
     ...(params.status !== undefined ? { status: params.status } : {}),
     ...(params.delivered !== undefined ? { delivered: params.delivered } : {}),
     ...(params.error !== undefined ? { error: params.error } : {}),
+    ...(params.deliveryError !== undefined ? { deliveryError: params.deliveryError } : {}),
     onFinished: (evt) => {
       params.onFinished?.(evt);
       finishedEvents.get(evt.jobId)?.(evt);
@@ -457,4 +468,78 @@ describe("CronService persists delivered status", () => {
     expect(capturedEvent?.delivered).toBe(true);
     expect(capturedEvent?.deliveryStatus).toBe("delivered");
   });
+
+  it("surfaces a successful run's delivery error to CLI/UI/API run logs across the cron boundary", async () => {
+    // Regression for https://github.com/openclaw/openclaw/issues/95419:
+    // when an isolated turn succeeds but post-run delivery fails, the run keeps
+    // `status: "ok"` (#94058) while the runner now reports the dispatch failure
+    // on a dedicated `deliveryError` field. That diagnostic must travel through
+    // service state -> the finished event -> the persisted run-log entry so the
+    // CLI/UI/API run logs can show *why* delivery did not land, instead of the
+    // failure being silently dropped because the run is not marked an error.
+    let capturedEvent:
+      | {
+          jobId: string;
+          error?: string;
+          delivered?: boolean;
+          deliveryStatus?: string;
+          deliveryError?: string;
+        }
+      | undefined;
+    const updated = await runIsolatedJobAndReadState({
+      job: buildAnnounceIsolatedAgentTurnJob("delivery-error-readback"),
+      status: "ok",
+      delivered: false,
+      deliveryError: "Message delivery failed",
+      onFinished: (evt) => {
+        capturedEvent = evt;
+      },
+    });
+
+    // The run itself succeeded: the run-level error stays empty so the run is
+    // not mislabeled as a failure, while delivery is recorded as not-delivered
+    // and `lastDeliveryError` carries the dispatch diagnostic.
+    expectSuccessfulCronRun(updated);
+    expect(updated?.state.lastError).toBeUndefined();
+    expect(updated?.state.lastDelivered).toBe(false);
+    expect(updated?.state.lastDeliveryStatus).toBe("not-delivered");
+    expect(updated?.state.lastDeliveryError).toBe("Message delivery failed");
+
+    // The finished event mirrors the persisted state: it carries the delivery
+    // error (the field the gateway forwards into the run log) without polluting
+    // the run-level error.
+    expect(capturedEvent?.error).toBeUndefined();
+    expect(capturedEvent?.delivered).toBe(false);
+    expect(capturedEvent?.deliveryStatus).toBe("not-delivered");
+    expect(capturedEvent?.deliveryError).toBe("Message delivery failed");
+
+    // Cross-cron-boundary readback: persist the finished event into the run log
+    // exactly as the gateway does (server-cron.ts forwards `evt.deliveryError`),
+    // then read it back the way the CLI/UI/API do. The delivery diagnostic must
+    // survive the round-trip while the run-log `error` column stays empty.
+    const runLogStore = await makeStorePath();
+    await appendCronRunLog({
+      storePath: runLogStore.storePath,
+      entry: {
+        ts: Date.now(),
+        jobId: capturedEvent!.jobId,
+        action: "finished",
+        status: "ok",
+        error: capturedEvent?.error,
+        delivered: capturedEvent?.delivered,
+        deliveryStatus: "not-delivered",
+        deliveryError: capturedEvent?.deliveryError,
+      },
+    });
+
+    const readBack = readCronRunLogEntriesSync({
+      storePath: runLogStore.storePath,
+      jobId: capturedEvent!.jobId,
+    });
+    expect(readBack).toHaveLength(1);
+    expect(readBack[0]?.status).toBe("ok");
+    expect(readBack[0]?.error).toBeUndefined();
+    expect(readBack[0]?.deliveryStatus).toBe("not-delivered");
+    expect(readBack[0]?.deliveryError).toBe("Message delivery failed");
+  });
 });
diff --git a/src/cron/service/ops.ts b/src/cron/service/ops.ts
@@ -960,6 +960,7 @@ async function finishPreparedManualRun(
         {
           status: coreResult.status,
           error: coreResult.error,
+          deliveryError: coreResult.deliveryError,
           diagnostics: coreResult.diagnostics,
           delivered: coreResult.delivered,
           provider: coreResult.provider,
diff --git a/src/cron/service/timer.ts b/src/cron/service/timer.ts
@@ -655,6 +655,7 @@ export function applyJobResult(
   result: {
     status: CronRunStatus;
     error?: string;
+    deliveryError?: string;
     diagnostics?: CronRunOutcome["diagnostics"];
     delivered?: boolean;
     provider?: string;
@@ -703,7 +704,11 @@ export function applyJobResult(
     job,
     runStatus: result.status,
     delivered: result.delivered,
-    error: result.error,
+    // A successful run keeps `error` empty but may carry a dedicated
+    // `deliveryError` when post-run delivery failed (#94058/#95419); prefer it
+    // so `lastDeliveryError` is populated without conflating it with a
+    // run-level failure. Error runs fall back to the run error as before.
+    error: result.deliveryError ?? result.error,
     globalFailureDestination: state.deps.cronConfig?.failureDestination,
   });
   job.state.lastDelivered = deliveryState.delivered;
@@ -974,6 +979,7 @@ function applyOutcomeToStoredJob(state: CronServiceState, result: TimedCronRunOu
       applyJobResult(state, result.job, {
         status: result.status,
         error: result.error,
+        deliveryError: result.deliveryError,
         diagnostics: result.diagnostics,
         delivered: result.delivered,
         provider: result.provider,
@@ -997,6 +1003,7 @@ function applyOutcomeToStoredJob(state: CronServiceState, result: TimedCronRunOu
   const shouldDelete = applyJobResult(state, job, {
     status: result.status,
     error: result.error,
+    deliveryError: result.deliveryError,
     diagnostics: result.diagnostics,
     delivered: result.delivered,
     provider: result.provider,
@@ -2098,6 +2105,7 @@ async function executeDetachedCronJob(
     return {
       status: res.status,
       error: res.error,
+      deliveryError: res.deliveryError,
       summary: res.summary,
       delivered: res.delivered,
       deliveryAttempted: res.deliveryAttempted,
@@ -2150,6 +2158,10 @@ async function executeDetachedCronJob(
   return {
     status: res.status,
     error: res.error,
+    // Forward the post-run delivery failure recorded on an otherwise
+    // successful run so the service can persist it as `lastDeliveryError` and
+    // emit it on the finished event for CLI/UI/API run logs (#95419).
+    deliveryError: res.deliveryError,
     summary: res.summary,
     delivered: res.delivered,
     deliveryAttempted: res.deliveryAttempted,
diff --git a/src/cron/types.ts b/src/cron/types.ts
@@ -173,6 +173,18 @@ export type CronRunDiagnostics = {
 export type CronRunOutcome = {
   status: CronRunStatus;
   error?: string;
+  /**
+   * Post-run delivery dispatch failure recorded on an otherwise successful run.
+   *
+   * A successful isolated agent turn keeps `status: "ok"` even when the
+   * post-run delivery phase fails (#94058). The execution `error` field stays
+   * empty in that case so the run is not mislabeled as a failure, but the
+   * delivery diagnostic must still reach `lastDeliveryError` and the finished
+   * event so CLI/UI/API run logs can surface why delivery did not land
+   * (#95419). This field carries that delivery error across the cron boundary
+   * without conflating it with a run-level `error`.
+   */
+  deliveryError?: string;
   /** Optional classifier for execution errors to guide fallback behavior. */
   errorKind?: "delivery-target";
   summary?: string;

Original file line number	Diff line number	Diff line change
`@@ -960,6 +960,7 @@ async function finishPreparedManualRun(`
`960`	`960`	`{`
`961`	`961`	`status: coreResult.status,`
`962`	`962`	`error: coreResult.error,`
	`963`	`+ deliveryError: coreResult.deliveryError,`
`963`	`964`	`diagnostics: coreResult.diagnostics,`
`964`	`965`	`delivered: coreResult.delivered,`
`965`	`966`	`provider: coreResult.provider,`