fix(chat): honor explicit model pick on post-failure recovery send (#5924) - #5949
nesquena-hermes wants to merge 1 commit into
Conversation
…5924) The onchange explicit-pick marker is single-shot: send() consumes it once. submitEdit() and cmdRetry() truncated and called send() directly without re-arming it, so the recovery send went out with explicit_model_pick=false and the server's compatible-model resolution re-reverted a freshly-picked cross-family model back to the failed/stale value (Facet 1). Facet 4 is the same loop keeping the persisted model_provider pinned across fork/refresh. Re-arm the pending explicit-pick marker from the current selector state immediately before await send() in both recovery paths. Survives a second consecutive recovery send. Normal send path and the #3737/#5731 server repair guard are unchanged. Reported by @b3nw.
|
| Filename | Overview |
|---|---|
| static/commands.js | Adds marker re-arming to /retry, but treats retries without a fresh model choice as explicit selections. |
| static/ui.js | Adds the same unconditional marker re-arming to edit-resubmit. |
| tests/test_issue5924_post_failure_recovery_model_pick.py | Adds server behavior tests and source-text checks, but does not cover recovery without a fresh model pick. |
Reviews (1): Last reviewed commit: "fix(chat): honor explicit model pick on ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if(typeof _rememberPendingSessionModel==='function' && typeof _chatPayloadModel==='function' && S.session){ | ||
| const _recoveryModel=_chatPayloadModel(); | ||
| const _recoveryProvider=(typeof _chatPayloadModelProvider==='function')?_chatPayloadModelProvider(_recoveryModel):null; | ||
| _rememberPendingSessionModel(activeSid, _recoveryModel, _recoveryProvider); | ||
| } |
There was a problem hiding this comment.
When /retry runs without a fresh selector change, this block still creates a pending marker from the stored session model. send() then reports explicit_model_pick:true, so the server skips compatibility repair and can retry the same stale or cross-family model that should have been normalized. The mirrored edit-resubmit block has the same behavior; both paths should re-arm only evidence of an actual explicit pick.
Context Used: AGENTS.md (source)
| "recovery send loses explicit_model_pick and the server re-reverts the model" | ||
| ) | ||
| rearm_idx = body.index("_rememberPendingSessionModel") | ||
| send_idx = body.rindex("await send()") |
There was a problem hiding this comment.
This assertion passes whenever the helper name appears before await send(), including in a comment, dead branch, or call using the wrong session and model. It also omits the no-fresh-pick recovery case, so the unconditional marker creation in both updated functions remains false-green instead of showing that ordinary recovery must retain server normalization.
Context Used: AGENTS.md (source)
nesquena
left a comment
nesquena
left a comment
There was a problem hiding this comment.
Review — end-to-end ✅ (clean approval, no fixes needed)
Independent review of #5924 (b3nw): after a provider failure, changing the model then edit-resubmit or /retry re-sent the failed model — the pick wasn't honored on the recovery send (Facet 1), and the stuck model survived reselect/refresh/fork (Facet 4).
What this ships
static/ui.js (submitEdit re-arm), static/commands.js (cmdRetry re-arm), tests/test_issue5924_post_failure_recovery_model_pick.py (+new). Agent-authored (nesquena-hermes). MERGEABLE, CI green (18/18).
Root cause + fix — traced correct
The explicit-pick marker (_rememberPendingSessionModel, set by modelSelect.onchange) is single-shot — send() consumes it once. The two recovery entry points (submitEdit truncate→send(), cmdRetry after /api/session/retry→send()) never re-armed it, so the recovery send went out explicit_model_pick:false and the server's _resolve_compatible_session_model_state was free to normalize the freshly-picked cross-family model back toward the stale/failed value (the #3737 repair). Facet 4 is the same loop.
The fix re-arms the marker from the current selector/session state (_chatPayloadModel() / _chatPayloadModelProvider()) immediately before await send() in both paths, so a recovery send carries explicit_model_pick:true exactly like a fresh onchange→send. Because it re-arms on every recovery invocation, a second consecutive recovery send still honors the pick (send() re-consumes each time). ✓
- Server guard untouched:
_resolve_compatible_session_model_stateis not modified; it still honorsexplicit_model_pick=Trueand still normalizes stale models when the pick is absent (the #3737/#2761/#5567/#5731 behavior), asserted bytest_non_explicit_send_still_normalizes_stale_model. ✓ - Mechanism verified:
_chatPayloadModel()=S.session.model || modelSelect.value; the selector'sonchangeupdatesS.session.modelto the pick, so the re-arm reads the user's fresh model (not the failed one) — which is what makes Facet 1 actually resolve. ✓ - Normal
send()path unchanged — the re-arm lives only in the two recovery entry points; a plain new message still consumes-then-doesn't-rearm → non-explicit → server normalization still applies. ✓ - Defensive
typeofguards on_rememberPendingSessionModel/_chatPayloadModelkeep it harness-safe. ✓
Edge-case matrix
| Scenario | Behavior |
|---|---|
| Fail → change model → edit-resubmit | fresh model honored (explicit) ✅ (was reverted) |
| Fail → change model → /retry | fresh model honored ✅ |
| Two consecutive recovery sends | both honor the pick (re-armed each time) ✅ |
| Plain new message (no edit/retry) | non-explicit → server normalizes stale model ✅ (no #3737 regression) |
| Recovery send, model unchanged | sends the current session model as explicit ✅ |
Minor observation (non-blocking)
submitEdit/cmdRetry are the generic edit/retry paths, so they now mark every such send explicit — meaning the server's stale-model normalization no longer applies to edit/retry (only to fresh passive sends). In the narrow case of a stale-and-incompatible S.session.model that the user edits without touching the selector, the recovery send would now carry that model verbatim rather than letting the server repair it. This is a defensible design choice (an explicit user action honors the session's configured model, and the #5924 flow is precisely "the user picked a working model"), and the server fallback still covers passive sends — flagging only for awareness, not as a blocker.
Tests
tests/test_issue5924_post_failure_recovery_model_pick.py— 6/6: both paths re-arm beforeawait send()(structural, order-checked), re-arm reads_chatPayloadModel(), explicit recovery pick honored server-side, second-consecutive honored, and non-explicit still normalizes (no #3737 regression).node --checkclean on ui.js + commands.js.- Full suite: 12586 passed / 0 failed in 425s (deselected the known darwin CRLF flake). CI green (18/18).
Recommendation
✅ Approved clean. Parked at approval — ready for the release agent's merge/tag pipeline.
A precise WebUI-side fix at the two recovery entry points: re-arming the single-shot explicit-pick marker from the current selector before the recovery send(), so edit-resubmit and /retry honor a freshly-picked model instead of the failed one (and survive a second consecutive recovery). The server normalization guard is untouched and still covers non-explicit sends (no #3737 regression, asserted). The one nuance (edit/retry always explicit) is a sound design choice with the passive-send fallback intact. RED on master; full suite clean. Ship.
🔬 Self-gate (Codex) — SHIP ONLY WITH FIXES (4 findings, holding for rework)Rebased onto current master (clean) + ran the full Codex gate. The fresh-pick model/provider path is wired correctly and normal
Render surface is clean (crown-jewel stream-regression-gate GREEN on the rebased head, all modes). Holding this for the deliberate-pick-record rework + the 2 session guards + regression tests for no-fresh-pick and the second-await switch race, then re-gate before ship. |
…y honors model pick (#5924) (#5964) * fix(streaming): keep errored-turn assistant response visible (#5941) An errored turn that produced assistant content (tool calls + reasoning) folded that content into a collapsed worklog above the error card, so the user read a lone error bubble as "nothing came back". The settled-scene renderer collapsed the worklog unconditionally, never consulting the turn terminal_state. Now an errored/failure terminal_state keeps the worklog expanded by default (unless the user explicitly collapsed it), while completed turns and genuinely-empty errored turns are unchanged. Reported by @b3nw. * fix(review): update #4970 brittle collapsed-expr assertion for #5941 test_issue4970_stream_done_shrink_regression asserted the literal `collapsed:!keepSettledWorklogOpen` in _renderSettledAnchorSceneForMessage. #5941 OR'd an errored-turn keep-open term into that expression (`collapsed:!(keepSettledWorklogOpen||erroredWorklogKeepOpen)`), so the substring no longer matched and the test failed in the full suite (the PR updated its own touched tests but missed this pre-existing one). Re-anchor to `collapsed:!(keepSettledWorklogOpen` — still guards #4970's invariant (keepSettledWorklogOpen negates into the settled-render collapsed flag) without pinning the exact term list. #4970 + #5941 suites green. * fix(chat): honor explicit model pick on post-failure recovery send (#5924) The onchange explicit-pick marker is single-shot: send() consumes it once. submitEdit() and cmdRetry() truncated and called send() directly without re-arming it, so the recovery send went out with explicit_model_pick=false and the server's compatible-model resolution re-reverted a freshly-picked cross-family model back to the failed/stale value (Facet 1). Facet 4 is the same loop keeping the persisted model_provider pinned across fork/refresh. Re-arm the pending explicit-pick marker from the current selector state immediately before await send() in both recovery paths. Survives a second consecutive recovery send. Normal send path and the #3737/#5731 server repair guard are unchanged. Reported by @b3nw. * fix(#5924): gate recovery re-arm on genuine pick + session-race guards Codex gate found 4 defects on the recovery send path; fixed all: - /retry + edit-resubmit re-armed the explicit-pick marker UNCONDITIONALLY, forcing explicit_model_pick even with no fresh pick (suppressed server compatible-model resolution). Now gated on _recoveryPick (selector model differs from session's own stored model), captured pre-await. - session-switch races: added active-session guards after the retry GET await and the edit truncate await so session A's recovery intent can't apply to session B. - 3 new regression tests (gated re-arm, pre-await capture, post-await re-guard). * fix(#5924) round-2: derive recovery pick from non-default session model, not inference Codex round-2 CORE: the state-comparison predicate false-negatived an already-applied pick (consumed marker → looks unchanged) and false-positived on provider inference (@removed:mistral-large + null stored provider → inferred 'removed' → fake change). Replace with _deliberateSessionModelPick(sid): reports {model,provider} only when the session's OWN model is genuinely non-default vs window._defaultModel/_activeProvider — inference-free + survives marker consumption. Both recovery paths + tests updated. * fix(#5924) round-3: require known-default+owned-provider evidence + fire-time re-arm guard Codex round-3: (1) _deliberateSessionModelPick still false-positived when the profile default was unknown or the provider was only inferred — now requires a session-OWNED provider AND a known window._defaultModel/_activeProvider, else fails closed. (2) new same-session race: a model change DURING the recovery awaits made the pre-await pick stale and clobbered a newer marker — new _reArmRecoveryPick helper re-arms only if the current session model/provider still equals the captured pick AND no different pending marker exists. Both recovery paths route through it. * Release exp-v0.52.45: errored-turn response stays visible (#5950/#5941) + recovery honors model pick (#5949/#5924) * fix: keep absoluteKeepCount capture before _recoveryPick in submitEdit The #5924 _recoveryPick comment contained the word 'await', tripping the pre-existing test_issue_edit_regenerate_absolute_keep_count regex (first \bawait\b must come after the absoluteKeepCount capture). Reorder both synchronous pre-network captures (absoluteKeepCount first) + reword the comment. No behavior change. --------- Co-authored-by: nesquena-hermes <agent@nesquena-hermes> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
2 participants
Summary
After a provider failure, the recovery flow ignored a freshly-picked model/provider: changing the model in the selector and then edit-resubmit or /retry re-sent the failed model, forcing the user to fork the session to escape.
This PR fixes Facet 1 (the pick not being honored on the recovery send) and Facet 4 (stuck model surviving fork/refresh/reselect/update — same root cause) by re-arming the explicit-pick marker on both recovery entry points.
Reported by @b3nw (Discord
#report-bugs).Root cause
The frontend selector path is correct —
boot.jsmodelSelect.onchangewrites a pending explicit-pick marker via_rememberPendingSessionModel(...). The problem is that the marker is single-shot:send()consumes it exactly once (messages.js), and neither recovery path re-arms it.submitEdit()(static/ui.js) truncates and callssend()directly.cmdRetry()(static/commands.js) does the same after/api/session/retry.Because the marker was already consumed by an earlier send, the recovery send goes out with
explicit_model_pick:false. The server's_resolve_compatible_session_model_state(api/routes.py) is then free to "repair" a cross-family model back toward the profile default — re-reverting to the stale/failed value. Facet 4 is the same loop: each non-explicit recovery send lets the server re-pin the persistedmodel_provider, so reselect/refresh/fork never sticks.This is exactly the layer the maintainer's investigation comment identified: WebUI-side, small, on the two recovery entry points — not the server guard (which correctly honors
explicit_model_pick=Trueand must keep normalizing stale models when the pick is absent, per #3737 / #5731).Fix
Re-arm the pending explicit-pick marker from the current selector/session state (
_chatPayloadModel()/_chatPayloadModelProvider()) immediately beforeawait send()in bothsubmitEdit()andcmdRetry(). This makes a recovery send carryexplicit_model_pick:true, exactly like a freshonchange → send. Because the re-arm runs on every recovery invocation, it also survives a second consecutive recovery send even thoughsend()consumes the marker each time._resolve_compatible_session_model_stateguard is untouched; the bug(models): picked model reverts to previous/default model on send (likely #3448 profile-aware resolution, v0.51.290) #3737 / WebUI mid-conversation: "No LLM provider configured" bricks the session until a fork; assistant response sometimes vanishes into an "Auto-compressing..." card; model silently reverts to gpt-5.4-mini #2761 / bug: cross-profile HERMES_HOME race causes turn-init failures with wrong profile's provider (v0.51.849 repro; follow-up to #2321) #5567 / Follow-up to #5567: repair already-poisoned session model_provider at the backend chat-start boundary #5731 repair behavior is preserved (verified by a regression assertion that a non-explicit send still normalizes a stale cross-family model).Files changed
static/ui.js—submitEdit()re-arms the pending pick beforeawait send().static/commands.js—cmdRetry()re-arms the pending pick beforeawait send().tests/test_issue5924_post_failure_recovery_model_pick.py— new regression test.Tests
New file
tests/test_issue5924_post_failure_recovery_model_pick.pypins the two-layer invariant:submitEditandcmdRetryre-arm the marker (from the current selector state) beforeawait send().Also re-ran
tests/test_issue_edit_regenerate_absolute_keep_count.pyandtests/test_model_selection_refresh_persistence.py(6 passed) to confirm no regression on the touched functions.Scope / out-of-scope follow-ups
This is a Facet 1 + Facet 4 fix. Deliberately out of scope (should be separate PRs):
forkFromMessage; likely a stale_oldestIdx/ transcript-not-fully-loaded timing issue on the error turn, needs a focused repro.Addresses #5924 (Facet 1 + Facet 4). Leaving the issue open for Facet 2 and Facet 3 above.