Skip to content

fix(cron): retry fallback on empty embedded cron results#99911

Open
849261680 wants to merge 1 commit into
openclaw:mainfrom
849261680:fix/97115-cron-fallback-abort
Open

fix(cron): retry fallback on empty embedded cron results#99911
849261680 wants to merge 1 commit into
openclaw:mainfrom
849261680:fix/97115-cron-fallback-abort

Conversation

@849261680

@849261680 849261680 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Related: #97115

What Problem This Solves

Fixes an issue where isolated cron agentTurn jobs 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:

  • Command: OPENCLAW_LOG_LEVEL=error node --import tsx --input-type=module with a temporary stdin script importing the production runWithModelFallback helper and production embedded result classifier from this checkout.
  • Setup: local loopback endpoint returned a fallback-safe embedded incomplete_turn / reasoning-only result for openai/primary-empty, then returned visible assistant text for anthropic/fallback-winner.
  • Redaction: loopback port, local paths, account details, endpoints, and secrets were redacted. No real API keys or external provider accounts were used.
loopback endpoint http://127.0.0.1:<redacted-port>/agent-run
without_cron_classifier summary {"outcome":"completed","winner":"openai/primary-empty","visibleText":null,"firstPayload":"Agent couldn't generate a response.","attempts":[],"loopbackHits":[{"provider":"openai","model":"primary-empty","final":false}]}
with_cron_classifier summary {"outcome":"completed","winner":"anthropic/fallback-winner","visibleText":"fallback winner: cron delivered a visible assistant reply","firstPayload":"fallback winner: cron delivered a visible assistant reply","attempts":[{"provider":"openai","model":"primary-empty","reason":"format","code":"incomplete_result"}],"loopbackHits":[{"provider":"openai","model":"primary-empty","final":false},{"provider":"anthropic","model":"fallback-winner","final":true}]}

Focused validation:

node scripts/run-vitest.mjs src/cron/isolated-agent/run.result-fallback-gap.test.ts
Test Files  1 passed (1)
Tests  2 passed (2)

node scripts/run-vitest.mjs src/cron/isolated-agent/run*.test.ts src/agents/outcome-fallback-runtime-contract.test.ts src/agents/model-fallback.test.ts
Test Files  22 passed (22)
Tests  199 passed (199)
Test Files  2 passed (2)
Tests  100 passed (100)

node_modules/.bin/oxfmt --check src/cron/isolated-agent/run-executor.ts src/cron/isolated-agent/run.result-fallback-gap.test.ts
All matched files use the correct format.

git diff --check
passed

node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/cron/isolated-agent/run-executor.ts src/cron/isolated-agent/run.result-fallback-gap.test.ts
passed

node scripts/run-tsgo.mjs -p tsconfig.core.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core.tsbuildinfo
passed

node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core-test.tsbuildinfo
passed

.agents/skills/autoreview/scripts/autoreview --mode local
autoreview clean: no accepted/actionable findings reported

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 4, 2026, 7:35 AM ET / 11:35 UTC.

Summary
The PR wires embedded-result fallback classification and exhaustion merging into isolated cron model fallback execution, with a focused regression test for incomplete embedded primary results and CLI-provider scoping.

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.

  • Cron fallback decision surface: 1 call changed. One isolated cron runWithModelFallback call now changes provider/model attempts for scheduled embedded jobs when returned results are classified as fallback-safe failures.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #96525
Summary: This PR is a candidate fix for the canonical isolated cron embedded result-level fallback classifier gap; the broader linked issue also includes a separate timeout claim.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P2] Merging intentionally changes scheduled embedded cron provider/model routing: fallback-safe empty, reasoning-only, or incomplete primary results can now invoke configured fallback providers that current main would not call.
  • [P1] There are multiple open candidate PRs for the same root cause, so maintainers should land one canonical branch and close the duplicates after merge.
  • [P2] The broader linked issue also reports a shared AbortController timeout path, which this PR intentionally leaves for separate work.

Maintainer options:

  1. Land The CLI-Scoped Classifier Fix (recommended)
    Accept the provider-routing change with the CLI guard intact so embedded cron follows configured fallbacks without applying embedded-result classification to CLI providers.
  2. Pick A Single Canonical Branch
    If maintainers prefer another open candidate, first reconcile this PR's CLI-provider scoping before closing or superseding this branch.
  3. Defer Timeout Semantics
    Do not expand this PR to the shared AbortController timeout claim unless maintainers decide that broader behavior must land together.

Next step before merge

  • No automated repair is needed; maintainers should choose the canonical cron fallback branch and explicitly accept the provider-routing change before merge.

Security
Cleared: The diff only changes TypeScript cron fallback wiring and a focused test; it does not touch dependencies, workflows, secrets, package metadata, permissions, or install/build scripts.

Review details

Best 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 changes

Label justifications:

  • P1: The PR targets a recurring scheduled-agent workflow that can drop the useful answer instead of recovering through configured fallback models.
  • merge-risk: 🚨 auth-provider: The diff changes cron provider/model fallback routing and can invoke configured fallback providers that current main would not call for returned incomplete results.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes redacted loopback live output showing the baseline stopping on the empty primary and the patched classifier path reaching the fallback winner.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted loopback live output showing the baseline stopping on the empty primary and the patched classifier path reaching the fallback winner.
Evidence reviewed

PR surface:

Source +22, Tests +149. Total +171 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 30 8 +22
Tests 1 149 0 +149
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 179 8 +171

What I checked:

  • Repository policy read: Root policy and the scoped agents test guidance were read; fallback/provider routing is compatibility-sensitive review context here. (AGENTS.md:1, 46ad3f9ae94c)
  • Current main still has the gap: Current main calls runWithModelFallback in isolated cron with fallback overrides but no classifyResult or mergeExhaustedResult, so returned result-level failures can be treated as successful attempts. (src/cron/isolated-agent/run-executor.ts:319, 46ad3f9ae94c)
  • Fallback helper contract: When a candidate returns ok, runWithModelFallback only retries it as a failed candidate if classifyResult returns a classification; otherwise it returns success. (src/agents/model-fallback.ts:403, 46ad3f9ae94c)
  • Existing classifier covers the reported result class: The embedded classifier handles fallback-safe incomplete_turn, empty, reasoning-only, and provider-error payload outcomes while preserving terminal payloads on exhaustion when appropriate. (src/agents/embedded-agent-runner/result-fallback-classifier.ts:176, 46ad3f9ae94c)
  • Sibling paths already use the same hooks: Interactive agent command and auto-reply/followup paths pass embedded result classification and exhausted-result merging into runWithModelFallback, supporting the cron owner-boundary choice. (src/agents/agent-command.ts:1899, 46ad3f9ae94c)
  • PR head fix shape: The PR adds a CLI-aware cron execution-provider resolver, classifies only non-CLI results, and passes the embedded exhausted-result merge helper to the cron fallback call. (src/cron/isolated-agent/run-executor.ts:301, 3685da568e9c)

Likely related people:

  • vincentkoc: Current blame points to commit 2981425 for the current cron fallback call and embedded classifier snapshot, and recent history also shows cron/runtime import work around the same seams. (role: recent area contributor; confidence: high; commits: 29814252e8e3, df4c086c524d, d7fcd23091e1; files: src/cron/isolated-agent/run-executor.ts, src/agents/embedded-agent-runner/result-fallback-classifier.ts, src/cron/isolated-agent/run-execution.runtime.ts)
  • steipete: Earlier history shows the original model fallback feature, later fallback hardening, and the isolated cron runner phase split that created the executor boundary touched by this PR. (role: fallback and cron execution feature-history owner; confidence: high; commits: 734bb6b4fd9a, d2597d5ecf2e, bbb73d3171e5; files: src/agents/model-fallback.ts, src/cron/isolated-agent/run-executor.ts, src/cron/isolated-agent/run.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-04T09:45:56.449Z sha 001afb8 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. labels Jul 4, 2026
@849261680

Copy link
Copy Markdown
Contributor Author

@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.

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@849261680
849261680 force-pushed the fix/97115-cron-fallback-abort branch from 001afb8 to 38f378a Compare July 4, 2026 10:23
@849261680

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Please re-review the current head 38f378aee172d8388870bb13a6bac942ab167043. The PR body now includes redacted loopback behavior proof showing the patched cron classifier path retrying the fallback winner.

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@849261680
849261680 force-pushed the fix/97115-cron-fallback-abort branch from 38f378a to c5bd364 Compare July 4, 2026 10:44
@849261680

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Please re-review the current rebased head c5bd364e3909caec6361e9e4144ddca51d69ed3f. The PR body includes redacted loopback behavior proof for the cron classifier fallback path, and the branch is now rebased onto latest upstream/main.

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@849261680
849261680 force-pushed the fix/97115-cron-fallback-abort branch from c5bd364 to 3685da5 Compare July 4, 2026 11:06
@849261680

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Please re-review the current head 3685da568e9ce1ae743fb8368b0fa8fae71a5b47. OpenClaw CI is now clean, and the PR body includes redacted loopback behavior proof for the cron classifier fallback path.

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 4, 2026
@AmirF194

AmirF194 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, and for the loopback evidence. Flagging what I found reading main alongside head 3685da568e9ce1ae743fb8368b0fa8fae71a5b47:

The core described behavior — the isolated cron embedded path passing the embedded result classifier and exhausted-result merge helper into runWithModelFallback so a reasoning-only/empty primary falls through to the configured chain — already landed on main in aaf5ab910c (#100483). It's live at src/cron/isolated-agent/run-executor.ts:344-346. Test 1 here (run.result-fallback-gap.test.ts, "engages the configured fallback…") passes against main too, since the suite default is isCliProvider -> false (run.test-harness.ts:511), so it isn't exercising anything new.

Once rebased (the PR is currently CONFLICTING against #100483, which rewrote these exact lines), the only net change over main is scoping the classifier away from CLI execution providers: run-executor.ts:363-365 (… isCli ? null : classify …) plus the resolveCronExecutionProvider dedupe helper. My concern is that CLI-scoping. runCliAgent returns an EmbeddedAgentRunResult (src/agents/cli-runner.ts:416), so CLI results do reach the classifier — the gate is load-bearing, not a no-op. But every other caller runs the same classifier for CLI and embedded results without an isCli gate: agent-command.ts:1950, followup-runner.ts:993, agent-runner-execution.ts:2275 (via outcomePlan.classifyRunResult, runtime-plan/build.ts:143). The classifier is already provider-agnostic and guards deliberate silent replies, committed delivery evidence, aborts, and hook-blocks (result-fallback-classifier.ts:176-231). Gating only cron therefore reduces fallback coverage for CLI-backed cron runs relative to main and diverges cron from the shared contract, without a concrete CLI-cron misclassification to point at (#97115 is the embedded-empty case, which #100483 already covers).

Suggestion: rebase on main, then either drop the CLI gate and keep just the resolveCronExecutionProvider dedupe as a small cleanup, or, if there's a real CLI-cron misclassification, add a repro and apply the same gate to the sibling callers so cron doesn't split from them. As is, once rebased the retry behavior is already shipped and the remaining delta looks like a one-sided divergence.

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 main today — a repro would change the picture.

@openclaw-barnacle

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S stale Marked as stale due to inactivity status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants