Skip to content

fix(model-fallback): re-throw LiveSessionModelSwitchError for genuine user-requested switches without configured fallbacks#101700

Closed
chenxiaoyu209 wants to merge 2 commits into
openclaw:mainfrom
chenxiaoyu209:fix/issue-101676-live-model-switch-no-fallback
Closed

fix(model-fallback): re-throw LiveSessionModelSwitchError for genuine user-requested switches without configured fallbacks#101700
chenxiaoyu209 wants to merge 2 commits into
openclaw:mainfrom
chenxiaoyu209:fix/issue-101676-live-model-switch-no-fallback

Conversation

@chenxiaoyu209

@chenxiaoyu209 chenxiaoyu209 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

When a live model switch is requested mid-turn (via /model, session_status override, cron job override, etc.) and the agent has no configured fallback models, the in-flight turn is killed with a generic failure instead of retrying against the newly-selected model.

Closes #101676

Why This Change Was Made

The root cause is in runWithModelFallbackInternal (src/agents/model-fallback.ts). When LiveSessionModelSwitchError is caught and the switch target is not a later configured fallback candidate, the error is unconditionally wrapped as a FailoverError with fallbackConfigured: false. With no fallback candidates configured, the loop ends immediately after wrapping, and throwFallbackFailureSummary throws a generic error that the outer live-switch retry loops cannot recognize as a model-switch event.

The fix adds a guard: when the switch target is not in the candidate chain at all (genuine external user request) and there are no configured fallbacks, re-throw the original LiveSessionModelSwitchError so the outer retry loops can catch and process it correctly. This preserves the stale-switch loop guard from #58496 — when the target is already in the candidate chain (same or earlier position), the error is still wrapped as FailoverError to prevent infinite retry loops.

User Impact

Any mid-turn model switch (/model, session_status model override, cron job override, etc.) issued while a turn is running, on an agent without configured fallback models, will now correctly retry the turn against the newly-selected model instead of killing it outright.

Evidence

🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]

@chenxiaoyu209
fix(model-fallback): re-throw LiveSessionModelSwitchError for genuine…
58b1484 
… user-requested switches without configured fallbacks (openclaw#101676)
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 10:39 AM ET / 14:39 UTC.

Summary
The PR adds a guard in runWithModelFallback so a no-fallback live model switch to a target outside the candidate chain rethrows LiveSessionModelSwitchError, plus focused regression coverage.

PR surface: Source +13, Tests +25. Total +38 across 2 files.

Reproducibility: yes. source inspection reproduces the current-main path: a no-fallback LiveSessionModelSwitchError is wrapped before the existing outer retry loop can catch it. I did not run a live repro because this review is read-only.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #101676
Summary: This PR is a candidate fix for the canonical no-fallback live model-switch issue; older fallback-loop work is adjacent, and PR 101715 is a competing but currently riskier candidate for the same issue.

Members:

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

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P2] Add redacted real behavior proof of an active no-fallback model switch retrying on the requested model.
  • Keep the focused model-fallback and live-switch regression tests green after any rebase.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists focused test runs only; before merge, the contributor should add redacted live output, logs, a terminal transcript, or a recording showing the no-fallback active switch retrying, then update the PR body for re-review.

Risk before merge

  • [P2] The PR changes provider/model routing at the live-switch and fallback boundary, so a wrong guard could terminate or misroute active turns even with green unit tests.
  • [P2] The PR body still contains test output only; it lacks redacted live output, logs, a terminal transcript, or another real setup proof that a no-fallback active switch retries successfully.

Maintainer options:

  1. Require no-fallback live-switch proof (recommended)
    Wait for redacted live output, terminal logs, or a short recording proving the active turn retries against the requested model while stale-switch tests stay green.
  2. Accept source-level routing confidence
    Maintainers may merge on the focused code/test evidence and accept that real provider-routing behavior was not shown by the contributor.

Next step before merge

  • [P1] Wait for contributor-supplied real behavior proof before merge; I did not find a narrow code repair for automation to apply on this PR.

Maintainer decision needed

  • Question: Should this external PR wait for contributor-provided real no-fallback live-switch proof, or may maintainers accept the source/test evidence as sufficient for this provider-routing change?
  • Rationale: The source patch looks narrowly correct, but repository policy treats external PR real behavior proof as a merge gate for user-visible runtime behavior.
  • Likely owner: steipete — Current blame and recent area history connect steipete to the reviewed live-switch retry and fallback branch.
  • Options:
    • Require real switch proof (recommended): Ask for redacted terminal logs, live output, or a short recording showing an active no-fallback model switch retrying on the requested model before merge.
    • Accept source-only evidence: Maintainers can intentionally merge on the focused tests and code reading while owning the remaining live-provider proof gap.

Security
Cleared: The diff only changes TypeScript fallback control flow and a colocated test; it adds no dependency, workflow, secret, permission, or package surface.

Review details

Best possible solution:

Land this narrow no-fallback guard only after redacted real behavior proof shows an active switch retrying on the requested model, while preserving the stale-switch fallback guard.

Do we have a high-confidence way to reproduce the issue?

Yes, source inspection reproduces the current-main path: a no-fallback LiveSessionModelSwitchError is wrapped before the existing outer retry loop can catch it. I did not run a live repro because this review is read-only.

Is this the best way to solve the issue?

Yes for the no-fallback scope: this is narrower than rethrowing every redirect miss, because it lets real new-target switches escape while preserving stale same/earlier target fallback handling. The remaining proof gap is runtime evidence, not a better code location.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c8d95da14c5b.

Label changes

Label justifications:

  • P1: The PR targets an active agent workflow where a mid-turn model switch can terminate a real user run instead of retrying.
  • merge-risk: 🚨 auth-provider: The diff changes model/provider selection flow across live-switch retry and fallback handling.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists focused test runs only; before merge, the contributor should add redacted live output, logs, a terminal transcript, or a recording showing the no-fallback active switch retrying, then update the PR body for re-review.
Evidence reviewed

PR surface:

Source +13, Tests +25. Total +38 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 13 0 +13
Tests 1 25 0 +25
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 38 0 +38

What I checked:

  • Repository policy read: Read root AGENTS.md and scoped src/agents/AGENTS.md; fallback/provider routing and proof requirements apply to this PR review. (AGENTS.md:1, c8d95da14c5b)
  • Current main failure boundary: Current main catches LiveSessionModelSwitchError after later-candidate redirect misses and normalizes it into a FailoverError, which prevents the outer live-switch retry from seeing a no-fallback external switch. (src/agents/model-fallback.ts:1813, c8d95da14c5b)
  • Outer retry path exists: The agent command loop catches LiveSessionModelSwitchError, updates the active provider/model, and retries with a bounded MAX_LIVE_SWITCH_RETRIES counter. (src/agents/agent-command.ts:2205, c8d95da14c5b)
  • User override disables configured fallbacks: resolveEffectiveModelFallbacks returns an empty override for user-sourced session model overrides, matching this PR's no-fallback regression test shape. (src/agents/agent-scope.ts:552, c8d95da14c5b)
  • PR head implementation: The PR head rethrows only when the switch target is not anywhere in the candidate chain and there are no fallback candidates, while same/earlier stale targets still flow into the existing failover guard. (src/agents/model-fallback.ts:1824, c24c7700d91b)
  • PR regression coverage: The PR head adds a focused test that expects a no-fallback switch to a target outside the candidate chain to escape as LiveSessionModelSwitchError. (src/agents/model-fallback.test.ts:1957, c24c7700d91b)

Likely related people:

  • steipete: Current-main blame in this checkout points the reviewed fallback branch, outer live-switch retry block, and embedded throw site at Peter Steinberger-authored recent area work. (role: recent area contributor; confidence: medium; commits: 903a09ab858d; files: src/agents/model-fallback.ts, src/agents/agent-command.ts, src/agents/embedded-agent-runner/run.ts)
  • vincentkoc: Merged PR 72375 by vincentkoc added adjacent live-switch fallback redirect behavior and explicitly preserved the stale-switch loop guard that this PR depends on. (role: recent fallback contributor; confidence: medium; commits: 43a003b8a062; files: src/agents/model-fallback.ts, src/agents/model-fallback.test.ts, src/agents/live-model-switch.ts)
  • Mlightsnow: Commit 5471548 is the merged April live-session/failover normalization work for the older fallback-loop family referenced by this PR. (role: introduced fallback behavior; confidence: medium; commits: 547154865bc0; files: src/agents/model-fallback.ts, src/agents/model-fallback.test.ts)
  • kiranvk-2011: Commit 251e086 introduced the persisted liveModelSwitchPending distinction between user-initiated switches and fallback rotations. (role: adjacent live-switch contributor; confidence: medium; commits: 251e086eacbd; files: src/agents/live-model-switch.ts, src/agents/embedded-agent-runner/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-07T14:06:19.968Z sha eb6823b :: 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 7, 2026
@chenxiaoyu209
chenxiaoyu209 force-pushed the fix/issue-101676-live-model-switch-no-fallback branch from eb6823b to c24c770 Compare July 7, 2026 14:23
@clawsweeper clawsweeper Bot mentioned this pull request Jul 7, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Live model switch mid-attempt terminates the run instead of retrying against the new model

1 participant

@chenxiaoyu209