Skip to content

fix: record cron delivery failures as warnings#80047

Closed
HemantSudarshan wants to merge 3 commits into
openclaw:mainfrom
HemantSudarshan:codex/49190-cron-warning-status
Closed

fix: record cron delivery failures as warnings#80047
HemantSudarshan wants to merge 3 commits into
openclaw:mainfrom
HemantSudarshan:codex/49190-cron-warning-status

Conversation

@HemantSudarshan

@HemantSudarshan HemantSudarshan commented May 10, 2026

Copy link
Copy Markdown
Contributor

Fixes #49190.

Problem summary

Cron isolated-agent runs currently collapse final delivery failures into the primary error run status even when the agent execution itself succeeded. That makes notification/channel failures look like execution failures, increments error/backoff state, can mark task records failed, and leaves the Control UI without a first-class status filter for this partial-success case.

Root cause

The isolated delivery dispatcher returned status: "error" for unresolved delivery targets and direct outbound send failures. applyJobResult only distinguished ok, skipped, and error, so delivery-only failures used the same scheduler path as model/tool/runtime failures.

Architectural reasoning

This keeps the change additive and follows the existing cron contract surface:

  • Adds warning to the cron run status union, gateway validation schema, run-log filters, plugin hook type, CLI/UI mirrors, and QA helper wait logic.
  • Leaves real agent/runtime failures as error; finalizeCronRun only asks delivery dispatch to downgrade delivery failures to warning when the agent payload did not already indicate a fatal execution failure.
  • Keeps delivery diagnostics in the delivery-specific fields: warning runs set delivered: false, persist lastDeliveryStatus: "not-delivered", and keep lastDeliveryError.
  • Keeps scheduler behavior aligned with partial success: warning runs reset execution-error counters, do not trigger error backoff/failure alerts, complete task-ledger records as successful execution, and terminal one-shot warning runs are disabled but not auto-deleted so operators can inspect state.
  • Extends Control UI filters/status labels and CLI coloring without changing existing ok, error, or skipped semantics.

Real behavior proof

  • Behavior or issue addressed: Cron isolated-agent runs whose agent execution completes but final announce delivery cannot resolve/send now persist as warning instead of error.
  • Real environment tested: Windows 11 local OpenClaw source checkout on branch codex/49190-cron-warning-status after rebasing onto upstream/main (6e18c8ec15). Runtime used Node 24.14.0 through node --import tsx; no live Slack credentials or private endpoints were used.
  • Exact steps or command run after this patch: Ran a one-off local Node script from the repo root with node --import tsx --input-type=module. The script loaded the real CronService and dispatchCronDelivery modules, created an isolated cron job with Slack announce delivery, simulated the delivery target failure outbound not configured for channel slack, ran the job with cron.run(job.id, "force"), and printed the persisted job state plus dispatcher result.
  • Evidence after fix (screenshot, recording, terminal capture, console output, redacted runtime log, linked artifact, or copied live output): Copied terminal output from the local run:
{
  "proof": "cron delivery warning status after patch",
  "manualRunOk": true,
  "jobState": {
    "enabled": false,
    "lastRunStatus": "warning",
    "lastStatus": "warning",
    "consecutiveErrors": 0,
    "lastDelivered": false,
    "lastDeliveryStatus": "not-delivered",
    "lastDeliveryError": "outbound not configured for channel slack"
  },
  "finishedEvent": {
    "status": "warning",
    "delivered": false,
    "deliveryStatus": "not-delivered",
    "deliveryError": "outbound not configured for channel slack"
  },
  "dispatchResult": {
    "status": "warning",
    "errorKind": "delivery-target",
    "delivered": false,
    "deliveryAttempted": false,
    "error": "outbound not configured for channel slack"
  }
}
  • Observed result after fix: The cron run completed with manualRunOk: true, persisted lastRunStatus: "warning", kept consecutiveErrors: 0, marked delivery as not-delivered, and returned the dispatcher delivery-target failure as warning with delivered: false.
  • What was not tested: No live Slack workspace send was attempted because this proof intentionally used a redacted local setup without channel credentials. Live channel sends remain covered by existing channel runtime paths; this PR only changes the status classification after delivery failure.

Testing performed

  • pnpm test src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts src/cron/service.persists-delivered-status.test.ts src/cron/service/timer.regression.test.ts src/gateway/protocol/cron-validators.test.ts ui/src/ui/views/cron.test.ts ui/src/ui/controllers/cron-filters.test.ts src/tasks/task-registry.test.ts
  • pnpm test src/cron/service.persists-delivered-status.test.ts src/cron/run-log.test.ts -- -t "persists warning|filters warning"
  • pnpm test src/cron/cron-protocol-conformance.test.ts src/cron/cron-protocol-schema.test.ts
  • pnpm lint:core
  • pnpm lint:scripts
  • pnpm lint:extensions
  • pnpm lint:ui:no-raw-window-open
  • pnpm docs:check-mdx
  • pnpm lint:docs -- docs/cli/cron.md docs/automation/cron-jobs.md
  • pnpm exec oxfmt --check --threads=1 ...changed files...
  • pnpm tsgo:core
  • pnpm tsgo:core:test
  • pnpm tsgo:test:ui
  • pnpm tsgo:extensions
  • pnpm tsgo:core (after rebasing onto latest upstream/main, to verify the OpenAI stream type CI repair)
  • pnpm tsgo:extensions:test
  • pnpm check:madge-import-cycles (after rebasing onto latest upstream/main)
  • pnpm test src/cron/run-log.test.ts -- --reporter=dot
  • pnpm test src/cron/run-log.test.ts src/cron/service.persists-delivered-status.test.ts src/cron/service/timer.regression.test.ts src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts src/gateway/protocol/cron-validators.test.ts ui/src/ui/controllers/cron-filters.test.ts ui/src/ui/views/cron.test.ts src/agents/openai-transport-stream.test.ts -- --reporter=dot
  • pnpm exec oxfmt --check --threads=1 src/cron/run-log.ts src/agents/openai-transport-stream.ts- pnpm ui:i18n:check (after rebasing onto latest upstream/main; local Windows run reached translation subprocess spawn EINVAL after raw-copy baseline passed)
  • pnpm check:changed
  • git diff --check

Local note: the full pnpm test src/cron/run-log.test.ts -- --reporter=dot initially reproduced a Windows append-path failure from appendRegularFile (Refusing to append after file changed). I fixed the run-log writer to keep the symlink-parent guard while appending through the fs-safe root writer, then reran pnpm test src/cron/run-log.test.ts -- --reporter=dot and the grouped cron/OpenAI targeted command successfully. Also, ui:i18n:sync initially failed because the configured translation subprocess could not spawn in this Windows shell; I reran the repo sync/check path with translation keys falling back to English for the new cron.runs.runStatusWarning key, which updated the locale metadata fallback keys and passed control-ui-i18n check.

Risk analysis

Risk is medium-low. The protocol change is additive, existing statuses keep their behavior, and the scheduler continues to apply error retry/backoff only to error. The main compatibility surface is clients that exhaustively switch on cron run statuses; this PR updates repo-owned TS/UI/plugin helper surfaces and leaves generated Swift/openclaw-kit fields as AnyCodable/string-compatible where applicable.

Backward compatibility

Existing stored ok, error, and skipped run records remain valid. New warning records are accepted by the gateway schema, CLI/UI type mirrors, run-log filters, and plugin hooks. Older external clients that do not know warning will see a new string status but can still inspect deliveryStatus/deliveryError for details.

Screenshots/logs

No screenshots; this is a scheduler/protocol/UI-state change. Relevant command logs are summarized above.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation app: web-ui App: web-ui gateway Gateway runtime cli CLI command changes scripts Repository scripts extensions: qa-lab size: M triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 10, 2026
@clawsweeper

clawsweeper Bot commented May 10, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

Summary
The PR adds a cron warning status for delivery-only failures across cron dispatch, persistence, gateway schema, CLI/UI/docs/tests, plugin hook types, generated UI locale metadata, plus a small OpenAI stream type and run-log append change.

Reproducibility: yes. Current main is source-reproducible: successful isolated cron execution can flow into delivery-target or direct-send failure paths that return status: "error", and the linked issue plus PR body provide the concrete scenario and after-fix output.

Real behavior proof
Sufficient (live_output): The PR body includes copied after-fix terminal output from a real local source checkout exercising CronService and delivery dispatch and showing the delivery failure persisted as warning.

Next step before merge
A narrow mechanical repair can be made on the PR branch; maintainer review still decides whether the new public cron warning status should land.

Security
Cleared: No concrete security or supply-chain concern found; the diff adds no dependencies, workflows, permissions, secrets handling, or new external code execution paths.

Review findings

  • [P2] Preserve the timestamp guard for all terminal statuses — extensions/qa-lab/src/cron-run-wait.ts:45-48
Review details

Best possible solution:

Fix the QA wait predicate and regression test, then land the additive warning-status model if maintainers accept the public cron status contract.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main is source-reproducible: successful isolated cron execution can flow into delivery-target or direct-send failure paths that return status: "error", and the linked issue plus PR body provide the concrete scenario and after-fix output.

Is this the best way to solve the issue?

No, not merge-ready yet. The additive warning status is a plausible maintainable direction, but the QA wait helper must keep the afterTs guard around every terminal status before this patch is correct.

Full review comments:

  • [P2] Preserve the timestamp guard for all terminal statuses — extensions/qa-lab/src/cron-run-wait.ts:45-48
    This predicate now applies entry.ts >= params.afterTs only to ok runs because && binds more tightly than ||. Any older error, skipped, or warning row for the same job can satisfy the wait before the newly triggered run finishes, so qa-lab scenarios can pass or fail against stale run-log data. Keep the timestamp check outside a parenthesized terminal-status test.
    Confidence: 0.94

Overall correctness: patch is incorrect
Overall confidence: 0.89

Acceptance criteria:

  • pnpm test extensions/qa-lab/src/cron-run-wait.test.ts
  • pnpm exec oxfmt --check --threads=1 extensions/qa-lab/src/cron-run-wait.ts extensions/qa-lab/src/cron-run-wait.test.ts

What I checked:

  • Current main records unresolved delivery targets as errors: On current main, failDeliveryTarget builds a run-session result with status: "error", so an announce target resolution failure is still classified as an execution error. (src/cron/isolated-agent/delivery-dispatch.ts:507, d4ba3615da6f)
  • Current main has no warning cron status contract: CronRunStatus is only ok | error | skipped, and the gateway cron schema mirrors that same set. (src/cron/types.ts:46, d4ba3615da6f)
  • PR head implements the delivery-warning downgrade: Latest PR head passes deliveryFailureStatus: hasFatalErrorPayload ? "error" : "warning" into dispatch and uses it for delivery-target and direct-send failures. (src/cron/isolated-agent/run.ts:998, 6e18c8ec1591)
  • Latest PR head still has the QA predicate bug: Because && binds more tightly than ||, only ok entries require entry.ts >= params.afterTs; older error, skipped, or warning rows can satisfy the wait. (extensions/qa-lab/src/cron-run-wait.ts:45, 6e18c8ec1591)
  • QA test coverage misses stale non-ok terminal rows: The PR test only covers ignoring an older ok row, so it does not catch stale error, skipped, or warning rows before afterTs. (extensions/qa-lab/src/cron-run-wait.test.ts:5, 6e18c8ec1591)
  • Related issue describes the remaining user problem: The linked issue asks for successful cron execution plus failed delivery to surface as warning rather than error, with delivery diagnostics preserved.

Likely related people:

  • @steipete: Current-main blame for dispatchCronDelivery and waitForCronRunCompletion points to Peter Steinberger, and prior PR review context maps the recent cron delivery/QA helper work to this handle. (role: recent cron delivery and QA helper contributor; confidence: high; commits: 8faf133620a9, cb207f97c987, a4b17d65a8ff; files: src/cron/isolated-agent/delivery-dispatch.ts, extensions/qa-lab/src/cron-run-wait.ts, src/gateway/protocol/schema/cron.ts)
  • @kevinslin: Prior review history cites recent work on src/cron/service/timer.ts, status handling, and diagnostics, which are central to warning-state semantics. (role: recent cron status and diagnostics contributor; confidence: medium; commits: 5b9672b4bbfb, 7175b1b5c634, 89db1e5440f5; files: src/cron/service/timer.ts)
  • @BunsDev: Prior review history cites recent Control UI cron view and controller work, which this PR extends for warning labels and filters. (role: recent Control UI cron contributor; confidence: medium; commits: e622223bcd13, aaa19fb9f363, 27a4bba90a58; files: ui/src/ui/views/cron.ts, ui/src/ui/controllers/cron.ts)

Remaining risk / open question:

  • The additive public cron status contract still needs maintainer acceptance even after the mechanical predicate bug is fixed.
  • Current tests and green CI do not cover stale non-ok terminal rows in the QA wait helper.

Codex review notes: model gpt-5.5, reasoning high; reviewed against d4ba3615da6f.

@HemantSudarshan
HemantSudarshan force-pushed the codex/49190-cron-warning-status branch from 334efe0 to 01d16fb Compare May 10, 2026 04:01
@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 10, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 10, 2026
@HemantSudarshan
HemantSudarshan force-pushed the codex/49190-cron-warning-status branch from 01d16fb to e113e2a Compare May 10, 2026 04:14
@openclaw-barnacle openclaw-barnacle Bot added channel: discord Channel integration: discord and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 10, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 10, 2026
@HemantSudarshan
HemantSudarshan force-pushed the codex/49190-cron-warning-status branch from e113e2a to 500d22f Compare May 10, 2026 04:29
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling and removed channel: discord Channel integration: discord proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 10, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 10, 2026
@HemantSudarshan
HemantSudarshan force-pushed the codex/49190-cron-warning-status branch from 500d22f to 527a512 Compare May 10, 2026 05:30
@openclaw-barnacle openclaw-barnacle Bot added size: L and removed size: M proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 10, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 10, 2026
@HemantSudarshan
HemantSudarshan force-pushed the codex/49190-cron-warning-status branch from 527a512 to 6e18c8e Compare May 10, 2026 05:48
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 10, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 10, 2026
@HemantSudarshan
HemantSudarshan deleted the codex/49190-cron-warning-status branch May 10, 2026 06:59
@HemantSudarshan

Copy link
Copy Markdown
Contributor Author

Replaced by #80139: #80139

Closing this copy so the active PR uses the hemant/ source branch prefix. Commit SHAs and author/committer identity are unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling app: web-ui App: web-ui cli CLI command changes docs Improvements or additions to documentation extensions: qa-lab gateway Gateway runtime proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. scripts Repository scripts size: L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Cron should distinguish execution success from delivery failure and show warning instead of error

1 participant