fix(cron): keep no-channel implicit cron runs successful instead of failing delivery (#56078)#91399
fix(cron): keep no-channel implicit cron runs successful instead of failing delivery (#56078)#91399MoerAI wants to merge 5 commits into
Conversation
openclaw-barnacle
Bot
added
size: S
triage: needs-real-behavior-proof
|
Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 8:34 AM ET / 12:34 UTC. Summary PR surface: Source +100, Tests +252. Total +352 across 8 files. Reproducibility: yes. source-level: current main defaults detached cron output to announce/last, can resolve that no-channel target as ok:false, and then hard-fails unresolved non-best-effort delivery. I did not run the WSL2 Control UI scenario in this read-only review. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Land the narrow no-channel cron delivery fix only after docs alignment plus redacted installed no-channel agentTurn cron proof or maintainer proof override, while leaving broader default-delivery and warning-status policy to their canonical issues. Do we have a high-confidence way to reproduce the issue? Yes, source-level: current main defaults detached cron output to announce/last, can resolve that no-channel target as ok:false, and then hard-fails unresolved non-best-effort delivery. I did not run the WSL2 Control UI scenario in this read-only review. Is this the best way to solve the issue? Yes as a bounded mitigation: the PR limits silent ok to the no-channel implicit route and preserves fail-closed behavior for explicit or channel-resolved failures. It still needs proof, docs alignment, and maintainer semantic acceptance before merge. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 60f0749b7fb3. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +100, Tests +252. Total +352 across 8 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (3 earlier review cycles)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4b3473fcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // sendable target (local/no-channel setups, #56078); hard-erroring would | ||
| // mark a successful agent turn as failed and drop its output. Downgrade | ||
| // those to the silent-ok path, same as best-effort delivery already does. | ||
| if (params.deliveryTargetExplicit && !params.deliveryBestEffort) { |
There was a problem hiding this comment.
When an omitted/default isolated cron delivery (channel: "last") resolves a channel but then fails to resolve the remembered or configured target, resolveDeliveryTarget returns ok:false with channel populated for the docked/target-resolution errors (src/cron/isolated-agent/delivery-target.ts:308-335). Because such jobs have no concrete fields in the delivery plan, deliveryTargetExplicit is false, so this new guard now downgrades those real delivery failures to status:"ok" with no retry/alert instead of failing the run. Limit the silent-ok downgrade to the no-channel/no-previous-channel case (or otherwise check the failure shape) so stale/invalid implicit routes do not get silently dropped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Applied in 3745614e. Good catch — you're right that resolveDeliveryTarget returns ok:false with channel populated for the docked/target-resolution failures (src/cron/isolated-agent/delivery-target.ts:308-335, 348-354), which are real delivery failures that must stay visible. The original guard keyed only on deliveryTargetExplicit, so an implicit job whose channel resolved but whose remembered/configured route was stale/invalid would have been wrongly downgraded to status:"ok".
The downgrade is now limited to the genuine no-channel case:
const targetResolvedButFailed = params.resolvedDelivery.channel != null;
if (!params.deliveryBestEffort && (params.deliveryTargetExplicit || targetResolvedButFailed)) {
return { result: failDeliveryTarget(...), ... };
}So only channel == null (announce-to-last with no previous channel, delivery-target.ts:249-259) downgrades to silent-ok; any ok:false that carries a resolved channel still fail-closes with errorKind:"delivery-target" (retry/alert preserved). Added a regression case still fails an implicit delivery whose resolved channel has a stale/invalid target (#56078 P1).
pnpm test src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts — 71 passed; pnpm tsgo:core — clean.
There was a problem hiding this comment.
Applied in 69e0f54 (fix(cron): preserve silent no-channel delivery state (#56078)).
The missing piece was in finalizeCronRun: when dispatch returned a silent-ok result for implicit delivery.channel=last with no previous channel, the dispatch state had delivered: false but the result object itself did not. The runner now preserves deliveryResult.delivered when merging delivery metadata, so the successful cron run persists as not delivered instead of falling back to unknown.
Verification:
node scripts/run-vitest.mjs src/cron/isolated-agent/run.message-tool-policy.test.ts— passedpnpm exec oxfmt --check --threads=1 src/cron/isolated-agent/run.ts src/cron/isolated-agent/run.message-tool-policy.test.ts— passedpnpm tsgo— passed (engine warning only: local Node v22.14.0 vs repo wants >=22.19.0)- LSP diagnostics clean for both changed files
clawsweeper
Bot
added
rating: 🧂 unranked krab
b4b3473 to
3745614
Compare
clawsweeper
Bot
added
P1
3745614 to
68b41e5
Compare
openclaw-barnacle
Bot
added
the
triage: needs-pr-context
|
Refreshed onto current Preview alignment (P2 —
|
dfef8a8 to
557c7f4
Compare
clawsweeper
Bot
added
status: 🛠️ actively grinding
openclaw-barnacle
Bot
added
size: M
and removed
size: S
triage: needs-pr-context
|
Addressed the #56078 proof gap and reran the focused behavior checks on the current head ( Proof run 1: focused regression tests for the two important branches: pnpm test src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts -t "downgrades an unresolved implicit last-channel delivery to a silent ok|still fails an implicit delivery whose resolved channel has a stale/invalid target" --reporter verboseResult: PASS, 1 file, 2 tests passed, 83 skipped. Proof run 2: Node 22.19 local runtime proof against {
"node": "v22.19.0",
"plan": {
"mode": "announce",
"channel": "last",
"requested": true,
"explicit": false
},
"resolved": {
"ok": false,
"mode": "implicit",
"error": "Channel is required when delivery.channel=last has no previous channel."
},
"dispatch": {
"resultStatus": "ok",
"delivered": false,
"deliveryAttempted": false,
"resultDeliveryAttempted": false
}
}Control proof: explicit/stale delivery target still hard-fails, so the P1 guard remains intact. {
"node": "v22.19.0",
"plan": {
"mode": "announce",
"channel": "telegram",
"to": "chat:missing",
"requested": true,
"explicit": true
},
"resolved": {
"ok": false,
"channel": "telegram",
"mode": "explicit",
"error": "Delivering to Telegram requires target <chatId>"
},
"dispatch": {
"resultStatus": "error",
"resultError": "Delivering to Telegram requires target <chatId>",
"delivered": false,
"deliveryAttempted": false,
"resultDeliveryAttempted": false
}
}A full CLI/gateway proof was attempted first, but the local gateway path required pairing/token auth before @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
69e0f54 to
4ed8b86
Compare
…ailing delivery (openclaw#56078) Backend/Control-UI isolated agentTurn cron jobs with no explicit delivery config default to implicit announce-to-last (delivery-plan.ts resolveCronDeliveryPlan). On a local/no-channel setup resolveDeliveryTarget returns ok:false ('Channel is required when delivery.channel=last has no previous channel'), and dispatchCronDelivery hard-failed the whole run (status=error), so a successful agent turn that e.g. wrote a report file was reported as failed (issue openclaw#56078). Now the unresolved-target hard-fail is gated on deliveryTargetExplicit=hasExplicitCronDeliveryTarget(deliveryPlan): implicit announce-to-last with no channel downgrades to the existing silent-ok path (delivered=false), while explicit targets still fail-closed (no message loss for intended deliveries). Regression test covers both sides. 70/70 delivery-dispatch.double-announce.test.ts pass; tsgo core clean.
…penclaw#56078 codex P1) Codex P1 on delivery-dispatch.ts:1232: the deliveryTargetExplicit gate also downgraded implicit jobs whose channel resolved but whose remembered/configured target was stale/invalid (delivery-target.ts:308-335 returns ok:false WITH channel populated) — real failures that must stay visible with retry/alert. Now only the no-channel case (resolvedDelivery.channel == null) downgrades to silent-ok; channel-resolved target-resolution failures still fail-closed. Added regression test for the channel-populated implicit case. 71/71 delivery-dispatch.double-announce.test.ts pass; tsgo core clean.
…penclaw#56078 codex P2) Codex P2 on delivery-dispatch.ts:1234: the runtime now returns ok/not-delivered for an implicit announce-to-last cron run with no resolvable channel (openclaw#56078), but resolveCronDeliveryPreview still formatted that same unresolved last target as 'will fail-closed', so CLI/UI delivery previews contradicted runtime. The preview now mirrors the exact runtime predicate: an unresolved last target is shown fail-closed only when !bestEffort AND (hasExplicitCronDeliveryTarget(plan) OR a channel resolved but its route is stale, resolved.channel != null); the implicit no-channel case is shown 'will skip delivery (ok, not delivered)'. Added focused preview regression coverage for both the implicit-no-channel (silent ok) and channel-resolved-but-stale (fail-closed) sides. delivery-preview.test.ts + delivery-dispatch.double-announce.test.ts pass; tsgo core + core:test clean on touched files.
Preserve the dispatch-level delivered=false metadata when unresolved implicit announce-to-last delivery returns a silent ok result. This keeps successful no-channel cron runs visible as not delivered instead of falling back to an unknown delivery state.\n\nVerification:\n- node scripts/run-vitest.mjs src/cron/isolated-agent/run.message-tool-policy.test.ts\n- pnpm exec oxfmt --check --threads=1 src/cron/isolated-agent/run.ts src/cron/isolated-agent/run.message-tool-policy.test.ts\n- pnpm tsgo
Command cron jobs inherit the announce-to-last default used by detached cron output. When the command exits successfully but the implicit last-channel target cannot resolve because there is no channel history or configured channel, record the run as successful with not-delivered metadata instead of converting the command result into a run failure. Constraint: Explicit or stale configured delivery targets must continue to fail the run. Rejected: Treat every command delivery failure as best-effort | would hide configured target breakages. Confidence: high Scope-risk: narrow Tested: focused gateway regression for implicit command no-channel runs Tested: focused cron delivery preview and isolated dispatch regressions Tested: oxfmt check on eight PR files Tested: tsdown build --no-clean Tested: live loopback token-auth gateway with OPENCLAW_SKIP_CHANNELS=1; implicit command cron stayed ok/not-delivered and explicit telegram target stayed error Not-tested: pnpm check:changed broad Testbox gate
4ed8b86 to
4fda0b5
Compare
|
Rebased and pushed a current-main proof update at What changed in this update:
Live behavior proof on rebuilt gateway bundle:
Verification on final head:
Note: Blacksmith/Testbox CLI is unavailable on this native Windows host ( @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
Closing this one on my side. The fix is real and still reproduces on current main, but the remaining merge gate is a maintainer product decision I can't resolve as an external contributor: it changes no-channel implicit cron delivery status semantics ( Rather than hold this open while the direction is decided in those canonical threads, I'm closing it and leaving the no-channel cron status call to them. If a maintainer wants this narrow |
1 participant
What Problem This Solves
Cron
agentTurnjobs with no configured channel could run the agent successfully but still mark the whole cron run as failed because implicit delivery tolastcould not resolve a channel. Users saw a failed cron despite the useful work already completing.Evidence
Focused proof from this maintenance cycle:
node scripts/run-vitest.mjs src/cron/isolated-agent/run.message-tool-policy.test.tspassed, targeted oxfmt passed,pnpm tsgopassed with only the repo Node engine warning,git diff --checkpassed, and LSP diagnostics were clean forsrc/cron/isolated-agent/run.tsandsrc/cron/isolated-agent/run.message-tool-policy.test.ts. The fix was committed as69e0f54fb5and pushed.Summary
On a local / no-channel setup, backend- or Control-UI-created isolated
agentTurncron jobs fail their whole run withChannel is required (no configured channels detected)even though the agent turn ran successfully (e.g. it wrote a report file). The run is markedstatus:"error"and the user sees no output. This patch keeps such runs successful: an implicit announce-to-lastdelivery that cannot resolve a channel now finishes as a silent ok (delivered:false) instead of hard-failing, while explicit delivery targets still fail-closed.Root Cause
src/cron/delivery-plan.ts:resolveCronDeliveryPlan: isolated/current/sessionagentTurn|commandjobs with no explicit delivery config default tomode:"announce",channel:"last",requested:true.src/cron/isolated-agent/delivery-target.ts:resolveDeliveryTarget(≈249-260): with no previously-used channel, returns{ ok:false, error:"Channel is required when delivery.channel=last has no previous channel." }.src/cron/isolated-agent/delivery-dispatch.ts:dispatchCronDelivery(theif (deliveryRequested && !skipHeartbeatDelivery && !sourceDeliverySatisfied) { if (!resolvedDelivery.ok) { if (!deliveryBestEffort) ... } }branch): for the non-best-effort default it calledfailDeliveryTarget(...)→status:"error",errorKind:"delivery-target". Execution path:run.ts dispatchCronDelivery -> resolvedDelivery.ok===false -> failDeliveryTarget.announcedelivery againstlast, so a local/no-channel setup can still hit the reportedChannel is requireddelivery-target failure."An implicit announce-to-
lastwith no resolvable channel never had a sendable target, so hard-erroring delivers nothing — it only marks a successful turn as failed and drops its output.Changes
src/cron/isolated-agent/delivery-dispatch.ts: adddeliveryTargetExplicittoDispatchCronDeliveryParams; gate the unresolved-target hard-fail on it —if (deliveryTargetExplicit && !deliveryBestEffort)still callsfailDeliveryTarget, otherwise it takes the existing silent-ok path (logCronDeliveryWarn+status:"ok",delivered:false,deliveryAttempted:false), the same graceful path best-effort delivery already uses.src/cron/isolated-agent/run.ts: passdeliveryTargetExplicit: hasExplicitCronDeliveryTarget(prepared.deliveryPlan)(already-imported helper;delivery-plan.ts:25-29returns true only for a concretechannel!=="last"/to/threadId/accountId).src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts: regression coverage for both sides.Reproduction (before fix)
Verification (after fix)
Test
pnpm test src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts— 70 passed (was 69/70 RED before the one-line gate; 68 pre-existing cases unchanged)pnpm tsgo:core— passed (clean)Notes
lastwith an unresolvable channel is downgraded; jobs that named a concrete channel/recipient/thread/account still fail-closed exactly as before. This mirrors the already-shipped best-effort branch.hasExplicitCronDeliveryTarget(deliveryPlan), notresolvedDelivery.mode—modeonly tracks an explicitto, so an explicit channel-only job would otherwise be wrongly downgraded.status:"ok"+delivered:falserecordsnot-deliveredand intentionally stops retry/backoff/error alerts for the no-channel default; explicit targets keep the prior error path.dispatchCronDelivery+tsgo:core.Closes #56078