fix(clarify): SSE-notify on clear_pending + treat 409 as terminal so expired prompts stop bricking the session (#4504) - #4524
Conversation
…owser (nesquena#4504) The agent-side clarify timeout path in api/streaming.py::_clarify_callback_impl calls clear_pending(sid) after the prompt's remaining time hits zero. That removes the server-side _gateway_queues / _pending entries but until now never emitted an SSE notify, so any browser subscribed to the clarify stream had no idea the prompt was gone: - The clarify card stayed visible (no pending=null event arrived). - The composer stayed locked (lockComposerForClarify never released). - The 3000 ms fallback poller had already been stopped by the turn-end handler before the timeout fired, so nothing reconciled the card against the now-empty server state. The session was effectively bricked from the UI side — switching sessions and returning re-rendered the stuck card from the client-side cache, and typing into the clarify input failed with 409 on every submit. Make clear_pending emit _clarify_sse_notify(session_key, None, 0) from inside _lock whenever it actually clears something. That matches the ordering contract submit_pending and resolve_clarify* already follow (notify inside _lock, then publish_session_list_changed + event.set() outside the lock). The browser's existing _handleClarifyEvent branch on pending=null already routes through _hideClarifyCardIfOwner(sid, false, 'expired') — which in turn calls _stashClarifyDraft('expired') so any draft the user typed is preserved in the now-unlocked composer. The no-entries case is a no-op: if nothing was actually cleared we do not push a spurious notify. The .event.set() unblock of the agent-side wait() is unchanged, so the _clarify_callback_impl timeout branch still returns its fallback string on time. 7 new tests in tests/test_4504_clarify_stuck_on_expiry.py cover both the SSE-notify behavior and the no-op case. 50/50 broader clarify tests still pass.
…tryable (nesquena#4504) When the clarify server returns 409 with {stale: true}, the prior client behavior re-enabled the card controls and kept the draft + card visible — on the assumption from nesquena#2639 that the user could simply retry. But the server-side _pending entry is gone in the 409 case, so every retry got 409 forever, and the composer remained locked behind the card. The user had zero affordance to dismiss it short of a full page reload. Route the 409 catch branch to hideClarifyCard(true, 'expired'), which flows through _stashClarifyDraft('expired'): - The current draft is moved into the unlocked composer. - sessionStorage saves the draft as a hermes-clarify-draft-* entry. - A 'Clarification timed out. Your draft was kept in the composer.' notice surfaces. The 'next prompt already loaded' case from nesquena#2639 is not lost: when a new clarify event is queued, the SSE/poll path's showClarifyCard() re-renders the card from scratch with the fresh clarify_id. We simply stop pretending the *current* card is recoverable when the server has already discarded it. The non-409 catch branch (true network / transient errors) keeps the existing 'keep card visible + re-enable controls' behavior so genuine retries still work once connectivity returns. The else branch (server returned ok=false without throwing) is left untouched — _handle_clarify_respond always returns 409 for the failure case today, so that branch is dead code-path but left for forward compatibility. 3 of the 7 tests in tests/test_4504_clarify_stuck_on_expiry.py pin this client-side behavior; the other 4 pin the server-side companion change.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🎬 Cutter preview — PR #4524
|
|
Thanks @Sanjays2402 — this is a real, well-diagnosed bug (#4504) and your fix is on the right track: it does un-brick the session. I ran it through a full Codex + Opus review and there are three things to address before it can ship, all on the client 409-terminal path (which, per the analysis below, is the only path that actually un-bricks the UI — so getting it exactly right matters). 1. The typed answer is silently dropped (must-fix)
2. A late 409 can wipe the next prompt's card — #2639 regression (must-fix)The 200/success path guards with 3. The server-side SSE notify has no browser consumer (not blocking — keep it, but adjust the framing)
TestsThe new Once 1 + 2 (+ their tests) land, this is a strong fix and I'll fast-track the re-gate. Really appreciate the clear root-cause writeup in the PR body. |
nesquena-hermes
added
the
changes-requested
…ng before draft rescue (nesquena#4504) Addresses two reviewer P1 defects in the original PR nesquena#4524 fix: 1. Loading-guard bug (typed answer silently dropped on expiry): respondClarify sets _clarifySetControlsDisabled(true, true) (loading class on #clarifySubmit) at the top, before the try. The prior 409 branch called hideClarifyCard(true, 'expired') → _stashClarifyDraft('expired') — but _stashClarifyDraft bails immediately on submit.classList.contains('loading') (messages.js:~4972). Net: for a free-text/'Other' answer the user typed, the card vanished, the draft was NOT moved to the composer, and no toast fired — directly contradicting the PR's own commentary. Fix: call _clarifySetControlsDisabled(false, false) *before* hideClarifyCard in the same-id branch so the draft actually reaches the unlocked composer and the 'draft kept' toast surfaces. 2. nesquena#2639 regression (late 409 wipes the next prompt's card): The 200/success path is guarded by if (_clarifyId === clarifyId) (messages.js:5188) specifically so a parallel poll that already rendered the next queued prompt B isn't clobbered. The prior 409 branch had no such guard — it unconditionally tore down the visible card. If prompt B rendered while A's response was in flight, A's late 409 dismissed B. Fix: gate the 409-terminal handling on _clarifyId === clarifyId. If it differs, a newer prompt is showing — re-enable controls, surface a 'previous prompt expired — a newer one is showing' status line, and return without touching the visible card. The same-id arm additionally calls _clearClarifyPendingForSession(sid) so the cached pending entry cannot re-render the just-dismissed card, mirroring the success-path contract. Tests expanded from 7 → 11 cases. New coverage: - test_409_clears_loading_before_hide_so_draft_is_rescued — pins the order so _stashClarifyDraft's loading-class guard does not bail. - test_409_is_guarded_by_clarify_id_match — pins the nesquena#2639 guard. - test_409_same_id_branch_clears_session_cache — pins the _clearClarifyPendingForSession(sid) call. - test_409_different_id_branch_does_not_dismiss — structurally pins that exactly one arm dismisses (the same-id arm), and both arms re-enable controls. 50/50 broader clarify tests still pass. ruff clean. node --check clean.
|
Thanks for the careful review @nesquena-hermes — both P1s landed and are now pinned by tests. Pushed in e6910e5. 1. Loading-guard fixed — typed draft now reaches the composerThe 409 same-id branch now calls 2.
|
|
Shipped in v0.51.534 via the rebased release branch (credited in CHANGELOG + release notes). Thanks @Sanjays2402 — clean convergence on all 3 fix-spec points (loading cleared before stash so the typed draft survives; clarify_id-guarded dismissal preserving #2639; same-id stale clear), with the reasoning in the comments and a 261-line test covering each case. Codex SAFE + Opus SAFE + full suite green (9707). This un-bricks the session on clarify-prompt expiry — nice fix. |
…d) + SSE-notify on expiry (nesquena#4504) + v0.51.534 CHANGELOG
…d) + SSE-notify on expiry (nesquena#4504) + v0.51.534 CHANGELOG
2 participants
Closes #4504
What Problem This Solves
When the clarify prompt's countdown hits zero, the agent-side
_clarify_callback_implcallsclear_pending(sid)and returns its fallback string. Until nowclear_pendingcleared server state but emitted no SSE notify, and the 3000 ms fallback poller had already been stopped by the turn-end handler — so the browser never knew the prompt was gone. The card stayed docked, the composer stayed locked, and the user's only "escape" was to submit a response, which_handle_clarify_respondthen rejected with409 {stale: true}. The previous client catch-block treated 409 as retryable, leaving the card + draft visible with controls re-enabled, but every retry returned 409 forever. The user was effectively bricked in the UI — switching sessions and returning re-rendered the same stuck card from the client cache.Per @b3n.w in Discord
#report-bugs(2026-06-19): "the box gets stuck, the input area also gets locked, and you can't clear it. Even if you switch sessions, when you return you're locked in this question box which can't be submitted, and text you can't edit or submit."Why This Change Was Made
The maintainer-authored issue body laid out a 3-phase fix. This PR ships Phase A and Phase B (the user-facing escape hatches); Phase C (UI dismiss button + countdown-zero proactive reconcile) is left as a follow-up since the first two already remove the dead-end.
Phase A — server pushes a "cleared" event on
clear_pending.api/clarify.py::clear_pendingnow calls_clarify_sse_notify(session_key, None, 0)from inside_lockwhenever it actually clears something. That matches the ordering contractsubmit_pendingandresolve_clarify*already follow (notify under_lock, thenpublish_session_list_changedandevent.set()outside). The browser's existing_handleClarifyEventbranch onpending=nullalready routes through_hideClarifyCardIfOwner(sid, false, 'expired')→_stashClarifyDraft('expired'), so the card comes down and any partial draft moves into the now-unlocked composer with a "Clarification timed out. Your draft was kept in the composer." notice.Phase B — client treats 409 as terminal in
respondClarify's catch.static/messages.js::respondClarifynow branches one.status === 409to callhideClarifyCard(true, 'expired')and early-return, instead of re-enabling the controls for an impossible retry. The "next prompt already loaded" case from #2639 isn't lost: when a fresh clarify event arrives, the SSE/poll path'sshowClarifyCard()re-renders the card from scratch with the newclarify_id. The non-409 catch branch (true network / transient errors) keeps the existing keep-card-and-draft behavior so genuine retries still work once connectivity returns.The no-entries case in
clear_pendingis a deliberate no-op (no spurious notify if nothing was actually cleared), and the.event.set()agent-side unblock is preserved so_clarify_callback_impl's timeout branch still returns its fallback string on time.Non-goals (left as Phase C / follow-ups):
/api/clarify/pendingwhen the countdown reaches zero.current_clarify_idfield — current behavior dismisses on any 409, and the next SSE event recovers the wrong-session case naturally.User Impact
Expired clarify prompts no longer brick the session. The card comes down via the SSE notify the moment the server clears the pending entry, the composer unlocks, and any draft the user typed is preserved in the composer. If the SSE message races a user click (the user hits Send during the clear window), the 409 response now dismisses the card the same way instead of leaving them stuck.
Evidence
New regression suite (
tests/test_4504_clarify_stuck_on_expiry.py) — 7 cases:Broader clarify suite (50 cases) still passes:
Lint:
Diff: 3 files, +209 / −5. Two surgical commits — server-side notify, then client-side 409 handling — split so reviewers can flip Phase A vs Phase B independently if scope-cutting is preferred.