fix(cron): retry fallback on empty embedded cron results#99911
fix(cron): retry fallback on empty embedded cron results#99911849261680 wants to merge 1 commit into
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 4, 2026, 7:35 AM ET / 11:35 UTC. Summary PR surface: Source +22, Tests +149. Total +171 across 2 files. Reproducibility: yes. at source level. Current main omits result classification in the isolated cron fallback call, while runWithModelFallback only advances returned result-level failures when a classifier is supplied; I did not run tests because this review is read-only. 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:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this PR or an equivalent CLI-scoped embedded-result classifier fix, then close the duplicate candidate PRs and leave the separate shared-timeout claim tracked outside this narrow patch. Do we have a high-confidence way to reproduce the issue? Yes at source level. Current main omits result classification in the isolated cron fallback call, while runWithModelFallback only advances returned result-level failures when a classifier is supplied; I did not run tests because this review is read-only. Is this the best way to solve the issue? Yes for the narrow returned-result gap. Reusing the existing embedded classifier and exhaustion merge helper in isolated cron is the right owner-boundary fix, and the PR's CLI guard is safer than applying embedded classification to every cron result. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 46ad3f9ae94c. Label changesLabel justifications:
Evidence reviewedPR surface: Source +22, Tests +149. Total +171 across 2 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 (1 earlier review cycle)
|
|
@clawsweeper re-review I added redacted loopback behavior proof to the PR body. It shows the old call shape stopping on the fallback-safe incomplete primary result, and the patched cron classifier shape retrying and returning the fallback winner. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
001afb8 to
38f378a
Compare
|
@clawsweeper re-review Please re-review the current head |
|
🦞🧹 I asked ClawSweeper to review this item again. |
38f378a to
c5bd364
Compare
|
@clawsweeper re-review Please re-review the current rebased head |
|
🦞🧹 I asked ClawSweeper to review this item again. |
c5bd364 to
3685da5
Compare
|
@clawsweeper re-review Please re-review the current head |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Thanks for this, and for the loopback evidence. Flagging what I found reading The core described behavior — the isolated cron embedded path passing the embedded result classifier and exhausted-result merge helper into Once rebased (the PR is currently CONFLICTING against #100483, which rewrote these exact lines), the only net change over Suggestion: rebase on Separately, the empty-reply symptom overlaps with #100229 (surface cron errors for empty isolated replies) — those two read as complementary layers (retry via the chain, then surface a clear error once the chain is exhausted) rather than competing, worth cross-linking. The dedupe helper itself reads cleanly. Happy to be corrected if you're seeing a CLI cron result that's being wrongly retried on |
|
This pull request has been automatically marked as stale due to inactivity. |
Related: #97115
What Problem This Solves
Fixes an issue where isolated cron
agentTurnjobs with configured model fallbacks could stop on an embedded primary model that returned a fallback-safe incomplete result, such as reasoning-only or empty visible output, instead of trying the next configured fallback model.This covers the returned-result / zero-token failure mode reported in #97115. The separate shared AbortController timeout claim is a distinct terminal-timeout contract and is intentionally left out of this narrow PR.
Why This Change Was Made
The isolated cron embedded path now passes the existing embedded result fallback classifier and exhausted-result merge helper into
runWithModelFallback, matching the already-supported embedded fallback contract. The classifier is scoped away from CLI-backed execution providers so CLI runtimes continue to own their own terminal classification.User Impact
Scheduled embedded cron runs can now recover through the configured fallback chain when the primary candidate returns a fallback-safe incomplete result. Users should get the healthy fallback model's answer instead of a recurring generic "Agent couldn't generate a response" cron failure for that returned-result case.
AI-assisted.
Evidence
Loopback behavior proof:
OPENCLAW_LOG_LEVEL=error node --import tsx --input-type=modulewith a temporary stdin script importing the productionrunWithModelFallbackhelper and production embedded result classifier from this checkout.incomplete_turn/reasoning-onlyresult foropenai/primary-empty, then returned visible assistant text foranthropic/fallback-winner.Focused validation: