fix: record cron delivery failures as warnings#80047
Conversation
|
Codex review: needs changes before merge. Summary Reproducibility: yes. Current main is source-reproducible: successful isolated cron execution can flow into delivery-target or direct-send failure paths that return Real behavior proof Next step before merge Security Review findings
Review detailsBest 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 Is this the best way to solve the issue? No, not merge-ready yet. The additive Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against d4ba3615da6f. |
334efe0 to
01d16fb
Compare
01d16fb to
e113e2a
Compare
e113e2a to
500d22f
Compare
500d22f to
527a512
Compare
527a512 to
6e18c8e
Compare
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.codex/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.