fix(cron): allow recovered early tool errors to succeed delivery (#94846)#94862
fix(cron): allow recovered early tool errors to succeed delivery (#94846)#94862ruomuxydt wants to merge 5 commits into
Conversation
…nclaw#94846) When an isolated agentTurn recovers from an early tool error (e.g. missing file) and produces successful later payloads or final assistant output, the cron finalization was incorrectly treating the run as fatal because runLevelError short-circuited the recovery detection. Remove the runLevelError gate from hasSuccessfulPayloadAfterLastError/hasSuccessfulPayloadBeforeLastError so payload-level recovery signals are always respected. Also ensure hasFatalErrorPayload does not trigger on runLevelError alone when the trajectory proves recovery. Co-Authored-By: Claude <[email protected]>
|
Codex review: needs real behavior proof before merge. Reviewed June 21, 2026, 11:15 PM ET / 03:15 UTC. Summary PR surface: Source +13, Tests +97. Total +110 across 2 files. Reproducibility: yes. at source level: current main still lets the linked recovered tool-warning trajectory be classified fatal before dispatch. I did not run a live scheduled cron job 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
Security Review findings
Review detailsBest possible solution: Land one canonical cron outcome fix that recovers known tool-warning trajectories while preserving true fatal structured-error suppression and proving the finalization dispatch boundary. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main still lets the linked recovered tool-warning trajectory be classified fatal before dispatch. I did not run a live scheduled cron job in this read-only review. Is this the best way to solve the issue? No. The helper is the right layer, but this branch still needs a narrower fatality predicate or a finalization guard so generic run-level failures do not fall through to normal delivery. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a1828110704f. Label changesLabel justifications:
Evidence reviewedPR surface: Source +13, Tests +97. Total +110 across 2 files. View PR surface stats
Security concerns:
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
|
…penclaw#94846) ClawSweeper review: the previous broadening let all runLevelError values be overridden by recovery signals. Narrow to non-terminal tool-warning evidence only: when the last error payload is a tool warning and the trajectory proves recovery, runLevelError is not fatal; generic run-level errors remain fatal regardless of recovery. Co-Authored-By: Claude <[email protected]>
|
Addressed review findings: narrowed the run-level recovery predicate to non-terminal tool-warning evidence only. When the last error payload is a tool warning ( Verified: 29/29 unit tests pass (including new test for generic run-level error remaining fatal). Real behavior proof passes on both recovered and genuine-fatal scenarios. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
…penclaw#94846) When a recovered tool-warning run is accepted (hasFatalErrorPayload=false), the output fields (summary, outputText, deliveryPayloads) now reflect the successful payload text, not the formatted run-level error. This addresses ClawSweeper rank-up move: 'Fix the resolver so accepted recovered tool-warning runs return final assistant or successful payload text instead of the formatted run-level error.' Also gates shouldUseRunLevelErrorPayload behind hasFatalErrorPayload so the run-level error string is never surfaced as output when the run is recovered. Co-Authored-By: Claude <[email protected]>
|
Addressed all three rank-up moves from the previous review:
Recovered tool-warning scenario (early sed error + later success + runLevelError present):
Genuine-fatal scenario (no recovery):
29/29 unit tests pass including focused output-field assertions. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
…ext recovery (openclaw#94846) Address ClawSweeper rank-up moves: 1. Preserve original error payload text for generic fatal run-level errors with later success-looking payloads, instead of surfacing the formatted run-level error as summary/outputText. 2. Add recovery path for tool-warning runs with only finalAssistantVisibleText (no successful payloads): when hasRecoveringTerminalOutput && isLastErrorAToolWarning, hasFatalStructuredErrorPayload is now false, so delivery proceeds using the final assistant text. 3. Gate shouldUseRunLevelErrorPayload behind hasFatalErrorPayload so run-level error text is never surfaced when the run is recovered. Co-Authored-By: Claude <[email protected]>
|
Addressed all three rank-up moves from the previous review:
31/31 unit tests pass including focused output-field assertions for both scenarios. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
…law#94846) When a recovered tool-warning run has finalAssistantVisibleText, the output fields (summary, outputText, deliveryPayloads) now use the recovered answer, not the stale warning payload, even for channels that do not prefer final assistant visible text. This addresses ClawSweeper rank-up move: 'Fix the no-preference finalAssistantVisibleText-only recovery case so the delivered payload is the recovered answer.' Co-Authored-By: Claude <[email protected]>
|
Addressed the no-preference rank-up move: When a recovered tool-warning run has finalAssistantVisibleText, the output fields (summary, outputText, deliveryPayloads) now use the recovered answer even for channels that do not prefer final assistant visible text. The fatality escape and final-assistant text selection now use the same predicate (hasRecoveringTerminalOutput && isLastErrorAToolWarning). Added focused test: 'delivers the recovered answer for no-preference channels when finalAssistantVisibleText exists' -- verifies summary/outputText/deliveryPayloads all use the recovered answer. 32/32 unit tests pass. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
maintainer closeout: this candidate is not safe to land. It removes the Current cron has no degraded outcome: The canonical issue remains open for a typed exec failure-phase/process-started contract or a deliberate degraded cron outcome design. |
Summary
Closes #94846.
An isolated scheduled
agentTurnthat recovers from an early missing-file tool error, produces final assistant output, and ends successfully was having delivery skipped. The cron finalization treated the earlier tool error as fatal becauserunLevelError(populated fromfinalRunResult.meta?.error) short-circuited all recovery detection inresolveCronPayloadOutcome.Root cause
In
src/cron/isolated-agent/helpers.ts,hasSuccessfulPayloadAfterLastErrorandhasSuccessfulPayloadBeforeLastErrorwere gated behind!params.runLevelError. When the embedded runner propagated the early tool error asmeta.error(a run-level error), the recovery signals from later successful payloads orfinalAssistantVisibleTextwere ignored.Additionally,
hasFatalErrorPayloadincluded|| runLevelError !== undefined, so even when payload-level recovery was detected, the mere presence ofrunLevelErrorforced the run tostatus: errorand skippeddispatchCronDelivery.Fix
Two changes in
src/cron/isolated-agent/helpers.ts:Remove the
!params.runLevelErrorgate fromhasSuccessfulPayloadAfterLastErrorandhasSuccessfulPayloadBeforeLastError. Payload-level recovery signals (successful payloads after the last error) are now always respected regardless of whetherrunLevelErroris set.Change
hasFatalErrorPayloadfrom... || runLevelError !== undefinedto... || (runLevelError !== undefined && !hasRecoveringTerminalOutput). ArunLevelErroralone does not force fatal when the trajectory proves recovery (later successful payloads or final assistant visible text).The net effect: an early tool error that the agent recovers from no longer blocks delivery, while genuine fatal runs (no recovery evidence) remain correctly classified as
status: error.Real behavior proof
Behavior addressed: Isolated cron
agentTurnjobs that recover from an early tool error (e.g. missing file) and produce final assistant output were having delivery skipped becauserunLevelErrorshort-circuited recovery detection.Real environment tested: Windows 10, Node 22.22.3, OpenClaw source checkout at
014d5f61, real Node process driving the real exportedresolveCronPayloadOutcomefunction.Exact steps or command run after this patch:
Evidence after fix (head
014d5f61):Observed result after fix: Before the patch, the recovered scenario returned
hasFatalErrorPayload=true(the run was incorrectly treated as fatal, skipping delivery). After the patch, it returnshasFatalErrorPayload=false(delivery proceeds). The genuine-fatal control correctly remainshasFatalErrorPayload=true.Unit tests (
src/cron/isolated-agent/helpers.test.ts, 28/28 including 3 new tests covering both scenarios) validate the before/after behavior. Existingrun.meta-error-status.test.ts(5/5) confirms no regression in genuine run-level error handling.What was not tested: Not run against a live scheduled cron job with a real provider/channel; the proof drives the real
resolveCronPayloadOutcomefunction in a real Node process with synthetic payloads that match the issue's trajectory shape.This PR is AI-assisted; the code, tests, and proof output above were produced and verified in my local environment.
Co-Authored-By: Claude [email protected]