fix(model-fallback): re-throw LiveSessionModelSwitchError for outer retry loop (#101676)#101715
fix(model-fallback): re-throw LiveSessionModelSwitchError for outer retry loop (#101676)#101715LZY3538 wants to merge 10 commits into
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 5:30 AM ET / 09:30 UTC. Summary PR surface: Source +37, Tests +155, Other +483. Total +675 across 4 files. Reproducibility: yes. at source level. Current main throws LiveSessionModelSwitchError from the embedded runner, then model fallback wraps a redirect miss as FailoverError before the existing outer retry loop can catch it. Review metrics: none identified. 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 the focused model-fallback fix after exact-head and merge-result checks are green, preserving coverage for non-candidate switches, auth-profile set/clear, later-candidate redirects, and stale in-chain fallback rotation. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main throws LiveSessionModelSwitchError from the embedded runner, then model fallback wraps a redirect miss as FailoverError before the existing outer retry loop can catch it. Is this the best way to solve the issue? Yes. Rethrowing at the model-fallback boundary is the narrow maintainable fix because the existing agent and cron callers already own bounded retries and state application, while candidate membership preserves stale fallback-loop protection. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4ae8d735bf54. Label changesLabel justifications:
Evidence reviewedPR surface: Source +37, Tests +155, Other +483. Total +675 across 4 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 (16 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
49d0d88 to
f476d12
Compare
openclaw-barnacle
Bot
added
scripts
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
🦞🧹 I asked ClawSweeper to review this item again. |
abb2392 to
808c9d4
Compare
clawsweeper
Bot
added
the
merge-risk: 🚨 automation
|
🦞🧹 I asked ClawSweeper to review this item again. |
…etry loop (openclaw#101676) When a live model switch is requested mid-attempt and the switch target is not a later fallback candidate, the fallback loop was wrapping LiveSessionModelSwitchError as FailoverError. With no remaining fallbacks configured this reached throwFallbackFailureSummary which produced a FallbackSummaryError — not a LiveSessionModelSwitchError, so the outer retry loop in agent-command.ts never caught it and the run terminated. Instead, re-throw LiveSessionModelSwitchError directly so the outer retry loop can restart the turn against the newly-selected model. The outer loop has its own bounded retry count (MAX_LIVE_SWITCH_RETRIES), so this cannot loop forever. The existing findLiveSessionModelSwitchRedirectIndex redirect-to-later-candidate behavior is preserved unchanged. Co-Authored-By: Claude <[email protected]>
…utside fallback list (openclaw#101676) The initial fix re-threw LiveSessionModelSwitchError for every redirect miss, including stale same/earlier targets that are still in the configured fallback chain. This would regress openclaw#58496 by letting conflicting fallback switches escape to the outer retry loop instead of advancing to configured fallback candidates. Narrow the rethrow: only propagate the error when the switch target is not in the candidate list at all (a genuine user-requested switch). Stale same/earlier targets that ARE in the candidate list remain in fallback rotation, preserving the existing stale-switch guard. Co-Authored-By: Claude <[email protected]>
…dalone live-switch proof (openclaw#101676) - Add integration test bridging the mock gap: real runWithModelFallback → isLiveSessionModelSwitchTargetInCandidates → false → throw err → outer retry loop catch → retry with switched model → success - Add standalone proof script (scripts/live-model-switch-proof.ts) that exercises the real runWithModelFallback through production-equivalent outer retry loops for main agent, subagent, and bounded retry guard - All 214 related tests pass (117+21+76) Co-Authored-By: Claude <[email protected]>
Co-Authored-By: Claude <[email protected]>
- Remove unnecessary template literal (no-unnecessary-template-expression) - Add :unknown type to catch callback (use-unknown-in-catch-callback-variable) Co-Authored-By: Claude <[email protected]>
808c9d4 to
804e2f6
Compare
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
removed
the
merge-risk: 🚨 automation
… outer loop (openclaw#101676) isLiveSessionModelSwitchTargetInCandidates only compared provider/model, so credential-only switches (same model, different authProfileId) were treated as stale in-chain switches, wrapped as FailoverError, and the outer retry loop never applied the new auth profile. Return false when the error carries an authProfileId so the outer loop can apply the credential change via applyLiveModelSwitchToRun. Co-Authored-By: Claude <[email protected]>
|
🦞🧹 I asked ClawSweeper to review this item again. |
…penclaw#101676) isLiveSessionModelSwitchTargetInCandidates only checked authProfileId (truthy), so auth-profile clears (same model, authProfileId=undefined but authProfileIdSource set) fell through to candidate matching and were wrapped as FailoverError. The outer retry loop never received the clear signal, leaving the session on stale credentials. Check authProfileId || authProfileIdSource to cover set, change, and clear of auth profiles. Co-Authored-By: Claude <[email protected]>
|
🦞🧹 I asked ClawSweeper to review this item again. |
…ect (openclaw#101676) Move the auth-profile guard above findLiveSessionModelSwitchRedirectIndex so credential changes (set/change/clear) always re-throw to the outer retry loop regardless of whether the target provider/model happens to be a later fallback candidate. Previously the redirect check ran first, so auth-only switches whose model appeared later in the candidate list were redirected instead of reaching the outer loop — losing the credential change. Co-Authored-By: Claude <[email protected]>
Real Behavior Proof — PR Head
|
| Path | Scenario | Verifies |
|---|---|---|
| 1 | Model switch to non-candidate target | LiveSessionModelSwitchError re-thrown → outer loop retries → success |
| 2 | Auth profile set (same model) | Auth re-throw before redirect → authProfileId preserved |
| 3 | Auth profile clear (same model) | Auth re-throw before redirect → authProfileId=undefined preserved |
Harness
scripts/proof-harness.ts — uses real runWithModelFallback (not mocked), verifies exact HEAD, exercises all three control-flow branches.
@clawsweeper re-review
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
proof: sufficient
PROOF_HEAD env var is now optional; harness runs on current HEAD without a stale default. The SHA lock is applied via the PR comment that invokes the harness, not the harness file itself. Co-Authored-By: Claude <[email protected]>
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
This pull request has been automatically marked as stale due to inactivity. |
1 participant
What Problem This Solves
When a user changes the model mid-turn (via
/model, session_status override, or cron job) while the AI is actively generating a response, the run terminates with an error instead of retrying the turn against the newly-selected model.Observed before fix (#101676):
Why This Change Was Made
The
runWithModelFallbackloop catchesLiveSessionModelSwitchErrorfrom the embedded runner. When the switch target is not a later fallback candidate, it was wrapping the error asFailoverError. With no more candidates,throwFallbackFailureSummaryproducedFallbackSummaryError— notLiveSessionModelSwitchError— so the outer retry loop never caught it.Fix: A new helper
isLiveSessionModelSwitchTargetInCandidatesdistinguishes between:The helper checks
authProfileId || authProfileIdSourceto handle credential set, change, and clear (not just set).Evidence
Runtime Before/After: Reproduced on This PR Head
Before fix (origin/main): LiveSessionModelSwitchError wrapped as FailoverError → turn fails.
After fix (this PR head): LiveSessionModelSwitchError re-thrown → outer loop retries → turn completes.
Focused Tests — Eight Control-Flow Branches
Full Test Suite
Changes
src/agents/model-fallback.tssrc/agents/model-fallback.test.tsscripts/live-model-switch-proof.tsCloses #101676.
🤖 Generated with Claude Code