fix: record cron delivery failures as warnings#80139
Conversation
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: current main still has the cron delivery/status problem, but the clean, proof-positive path is now #95419, while this broader warning-status branch is conflicting and has a concrete QA wait regression. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Canonical path: Close this conflicting branch and continue the cron delivery-status semantics decision on #95419. So I’m closing this here and keeping the remaining discussion on #95419. Review detailsBest possible solution: Close this conflicting branch and continue the cron delivery-status semantics decision on #95419. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main has only Is this the best way to solve the issue? No, not as the best landing path now. A first-class Security review: Security review cleared: No concrete security or supply-chain regression was found; the diff adds no dependencies, workflows, permissions, secrets handling, install hooks, or third-party code execution. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 9d68f877ac3e. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix: record cron delivery failures as warnings This is item 1/1 in the current shard. Shard 3/4. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
Fixes #49190.
Problem summary
Cron isolated-agent runs currently collapse final delivery failures into the primary
errorrun 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.applyJobResultonly distinguishedok,skipped, anderror, 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:
warningto the cron run status union, gateway validation schema, run-log filters, plugin hook type, CLI/UI mirrors, and QA helper wait logic.error;finalizeCronRunonly asks delivery dispatch to downgrade delivery failures towarningwhen the agent payload did not already indicate a fatal execution failure.delivered: false, persistlastDeliveryStatus: "not-delivered", and keeplastDeliveryError.ok,error, orskippedsemantics.Real behavior proof
warninginstead oferror.hemant/49190-cron-warning-statusafter rebasing ontoupstream/main(6e18c8ec15). Runtime used Node 24.14.0 throughnode --import tsx; no live Slack credentials or private endpoints were used.node --import tsx --input-type=module. The script loaded the realCronServiceanddispatchCronDeliverymodules, created an isolated cron job with Slack announce delivery, simulated the delivery target failureoutbound not configured for channel slack, ran the job withcron.run(job.id, "force"), and printed the persisted job state plus dispatcher result.{ "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" } }manualRunOk: true, persistedlastRunStatus: "warning", keptconsecutiveErrors: 0, marked delivery asnot-delivered, and returned the dispatcher delivery-target failure aswarningwithdelivered: false.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.tspnpm 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.tspnpm lint:corepnpm lint:scriptspnpm lint:extensionspnpm lint:ui:no-raw-window-openpnpm docs:check-mdxpnpm lint:docs -- docs/cli/cron.md docs/automation/cron-jobs.mdpnpm exec oxfmt --check --threads=1 ...changed files...pnpm tsgo:corepnpm tsgo:core:testpnpm tsgo:test:uipnpm tsgo:extensionspnpm tsgo:core(after rebasing onto latestupstream/main, to verify the OpenAI stream type CI repair)pnpm tsgo:extensions:testpnpm check:madge-import-cycles(after rebasing onto latestupstream/main)pnpm test src/cron/run-log.test.ts -- --reporter=dotpnpm 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=dotpnpm exec oxfmt --check --threads=1 src/cron/run-log.ts src/agents/openai-transport-stream.ts-pnpm ui:i18n:check(after rebasing onto latestupstream/main; local Windows run reached translation subprocessspawn EINVALafter raw-copy baseline passed)pnpm check:changedgit diff --checkLocal note: the full
pnpm test src/cron/run-log.test.ts -- --reporter=dotinitially reproduced a Windows append-path failure fromappendRegularFile(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 reranpnpm test src/cron/run-log.test.ts -- --reporter=dotand the grouped cron/OpenAI targeted command successfully. Also,ui:i18n:syncinitially 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 newcron.runs.runStatusWarningkey, which updated the locale metadata fallback keys and passedcontrol-ui-i18ncheck.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 asAnyCodable/string-compatible where applicable.Backward compatibility
Existing stored
ok,error, andskippedrun records remain valid. Newwarningrecords are accepted by the gateway schema, CLI/UI type mirrors, run-log filters, and plugin hooks. Older external clients that do not knowwarningwill see a new string status but can still inspectdeliveryStatus/deliveryErrorfor details.Screenshots/logs
No screenshots; this is a scheduler/protocol/UI-state change. Relevant command logs are summarized above.