fix(streaming): keep errored-turn assistant response visible (#5941) - #5950
nesquena-hermes wants to merge 2 commits into
Conversation
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.
|
| Filename | Overview |
|---|---|
| static/ui.js | Classifies failed terminal states and keeps their content-bearing worklogs open by default while respecting saved disclosure state. |
| tests/test_issue5941_errored_turn_response_visible.py | Covers failure-state classification and the settled worklog collapse matrix. |
| tests/test_issue4970_stream_done_shrink_regression.py | Updates the existing stream-settle assertion for the extended collapse expression. |
Reviews (2): Last reviewed commit: "fix(review): update #4970 brittle collap..." | Re-trigger Greptile
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.
nesquena
left a comment
There was a problem hiding this comment.
Review — end-to-end ✅ (approved after one CI-red fix I pushed)
Independent review of #5941 (b3nw): on an errored turn the assistant's produced response (tool calls + reasoning) was auto-collapsed behind the error card, reading as "nothing came back." This keeps that content visible by default on error/failure turns.
What this ships
static/ui.js (one predicate + one term in the settled-render collapsed expression), tests/test_issue5941_errored_turn_response_visible.py (+new). Agent-authored (nesquena-hermes). MERGEABLE, CI green (18/18 after my fix).
Root cause + fix — traced correct
_renderSettledAnchorSceneForMessage built the settled worklog with collapsed: !keepSettledWorklogOpen unconditionally, never consulting the turn's terminal_state. So an errored-but-content-bearing turn collapsed like a normal completed one, hiding the response. The fix:
_anchorSceneHasErroredTerminalState(scene)matchesterminal_state ∈ {error, no_response, degraded, connection_lost, tool_limit_reached, compression_exhausted}. I verified againststatic/assistant_turn_anchors.jsthat these are the actual terminal-state values, and that the set correctly excludescompleted/null (normal) andcancelled/interrupted(user stops with their own cards + #5224 preservation). ✓erroredWorklogKeepOpen = errored && _readActivityDisclosureState(activityKey) !== 'closed'→ keep open by default but respect a user's explicit collapse. ✓collapsed: !(keepSettledWorklogOpen || erroredWorklogKeepOpen)— adds the errored keep-open to the existing settle keep-open. ✓
Interaction with #5860 (lazy worklog) + #5224 — sound
- #5860: for an errored turn
collapsedis nowfalse, so the render takes the eager path (_renderAnchorSceneRowsIntoWorklog) — it never enters #5860's collapsed-defer branch, so the produced rows materialize immediately (exactly the intent). Collapsed non-errored worklogs still defer as before. No conflict. ✓ - Empty errored turn: the function's top gate (
if(!_anchorSceneSceneHasWorklogWorthyRows(...)) return false;, line 3) early-returns when there are zero tool/thinking/compression rows — so a genuineno_responsewith no produced content never reaches the collapse decision and still shows only its bare error card (no phantom empty body). ✓
What I caught — full-suite CI-red the PR missed
The PR's targeted tests + its cited crown-jewel regressions passed, but the full suite turned up test_issue4970_stream_done_shrink_regression::test_helper_and_token_threaded_through_render failing: it asserted the literal collapsed:!keepSettledWorklogOpen, which this PR changed to collapsed:!(keepSettledWorklogOpen||erroredWorklogKeepOpen). The PR updated its own touched tests but missed this pre-existing brittle source-string assertion.
Pushed 8ad3a781: re-anchored #4970 to collapsed:!(keepSettledWorklogOpen — still guards #4970's invariant (keepSettledWorklogOpen negates into the settled-render collapsed flag) without pinning the exact term list. #4970 + #5941 both green; ruff clean.
Edge-case matrix
| Turn | Worklog |
|---|---|
| errored (error/no_response/degraded/…) with content | expanded by default — response visible ✅ (was hidden) |
errored + user explicitly collapsed (closed) |
stays collapsed (respected) ✅ |
| errored with zero produced content | early-return → bare error card, no phantom body ✅ |
| completed (normal) turn | collapsed as before ✅ |
| cancelled / interrupted | unchanged (own cards + #5224) ✅ |
| collapsed non-errored worklog | still #5860-deferred ✅ |
Tests
tests/test_issue5941_errored_turn_response_visible.py— 3/3 (render-gate wiring lock + Node behavioral matrix: errored→visible, errored+user-collapsed→collapsed, completed→collapsed).- RED-on-master verified independently: the decision-matrix test fails against master's
ui.js. Load-bearing. - settled-scene/worklog blast-radius — 278 passed, 5 skipped.
- Full suite: 12583 passed / 0 failed in 439s after
8ad3a781(deselected the known darwin CRLF flake). CI green (18/18).
Recommendation
✅ Approved after fix. Parked at approval — ready for the release agent's merge/tag pipeline.
The smallest correct change on the settled-render collapse decision: errored/failure turns with produced content stay expanded (respecting an explicit user collapse), empty errored turns are gated out (no phantom body), and completed/cancelled/interrupted are untouched — with the #5860 lazy-render and #5224 interactions verified. The one gap — a pre-existing #4970 brittle assertion the expression change broke — I fixed in 8ad3a781. RED on master; full suite clean. 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>
Summary
On an errored turn, the assistant's actual produced response (tool calls + reasoning) was auto-collapsed behind a single header above the error card, so users reasonably concluded that nothing came back — even though the full response was still there, one click away. This keeps that produced content visible by default on an errored turn.
Root cause + fix
When a turn ends in a provider/agent failure it still projects a settled anchor "activity scene" (the collapsible worklog rail) carrying the tool/reasoning rows it produced, plus a
terminal_stateoferror/no_response/tool_limit_reached/ etc. The settled-scene renderer_renderSettledAnchorSceneForMessage(static/ui.js) built that worklog withcollapsed: !keepSettledWorklogOpenunconditionally — it never consulted the turn'sterminal_state, so an errored-but-content-bearing turn collapsed exactly like a normal completed turn, hiding the response and reading as data loss. The fix classifies the scene'sterminal_state: when it is an error/failure state (not a normalcompleted), the worklog is kept expanded by default so the produced response stays visible. A user who has explicitly collapsed that turn's worklog (savedcloseddisclosure state) is still respected, and the genuinely-empty errored turn is untouched — the render gate at the top of the function already requires a worklog-worthy scene (≥1 tool/thinking/compression row), so a realno_responsewith zero produced content never reaches the collapse decision and still shows only its bare error card.Scope / safety
collapsedexpression. No refactor.cancelled/interruptedare deliberately excluded (they own dedicated cards + Bug: Terminal stream failures can hide an existing chat transcript #5224 transcript preservation).Tests
New:
tests/test_issue5941_errored_turn_response_visible.py— structural lock on the render-gate wiring plus behavioral (Node-executed) coverage of the terminal-state predicate and the full collapse decision matrix (errored default → visible; errored + user-collapsed → collapsed; completed → collapsed as before).Related crown-jewel regressions re-run clean (no regressions):
Attribution
Reported by @b3nw (Discord
#report-bugs).Closes #5941