fix(desktop): keep the clarify card until it is answered, skipped, or expired - #4
Merged
Merged
Conversation
… expired Clicking into a chat with an unanswered clarify flashed the question card and then lost it, leaving the question unrecoverable and the agent blocked until agent.clarify_timeout (3600s). Three independent defects: 1. The card's lifetime was gated on `messageRunning` (thread.isRunning && message.status === 'running'). thread.isRunning is fed from `busy`, and resumeSession sets busyRef true on entry then back to the resumed `running` in its finally block — which the deferred resume path reports as false. So the panel mounted from the persisted tool-call args, then unmounted a beat later. It now lives and dies with its own request, correlated by request id (a row hydrated from clarify.request) or question text (a tool.start row, whose id is the model's tool_call_id and never matches). Correlating per ROW, not per session, keeps an older un-resulted clarify from claiming a newer request and stranding itself on a spinner. 2. clarify.request is a one-shot event. The gateway tracked the wait in _pending/_pending_prompt_payloads but never handed it back, so a reopened window, an app restart, or a reconnect that dropped the frame could never re-arm and the card had no request_id to answer with. _session_pending_prompts() now replays still-blocked prompts through _live_session_payload as `pending_prompts`, and the renderer re-arms on both session.activate and session.resume. Only a live session can hold a blocked prompt, so this is the payload that can carry one. 3. clarify.expire was emitted by the gateway and had no desktop handler at all. It now drops the request and the sidebar's needs-input flag — but only when the clear actually happened, so a stale expire cannot silence a session still blocked on a newer question. clearClarifyRequest() returns that verdict. Also clears parked requests on a gateway switch: they key off runtime session ids the next backend mints fresh, and the card no longer self-limits via the running flag. Verified: desktop vitest 2918 passed, tests/tui_gateway 487 passed, typecheck and eslint clean. The multi-clarify test was confirmed to fail against the un-correlated version of the fix.
૮ >ﻌ< ა ci reviewran on 89d9d0d all good! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clicking into a chat that had an unanswered
clarifyflashed the question card and then lost it — the question became unrecoverable and the agent stayed blocked untilagent.clarify_timeout(3600s).Three independent defects, all fixed here.
1. The card's lifetime was gated on the wrong thing
ClarifyToolLivegated the interactive panel onmessageRunning(thread.isRunning && message.status === 'running').thread.isRunningis fed frombusy, andresumeSessionsetsbusyReftrue on entry then back to the resumedrunningin itsfinally— which the deferred resume path reports asfalse. So the panel mounted from the persisted tool-call args, then unmounted a beat later.It now lives and dies with its own request, correlated by request id (a row hydrated from
clarify.request) or question text (atool.startrow, whose id is the model'stool_call_idand never matches).Correlating per row rather than per session is load-bearing: a transcript can hold several clarify rows, and a session-wide match let an older un-resulted row claim a newer request, then strand itself on a spinner when the pending panel rejected the mismatch. That regression was caught in review and is covered by a test verified to fail against the un-correlated version.
2.
clarify.requestis one-shot and was never replayableThe gateway tracked the wait in
_pending/_pending_prompt_payloadsbut never handed it back, so a reopened window, an app restart, or a reconnect that dropped the frame could never re-arm — the card rendered the stored question with norequest_idto answer with._session_pending_prompts()now replays still-blocked prompts through_live_session_payloadaspending_prompts, and the renderer re-arms on bothsession.activateandsession.resume. Only a live session can hold a blocked prompt, so that is the payload that can carry one.3.
clarify.expirehad no desktop handler at allThe gateway emitted it; nothing listened. It now drops the request and the sidebar's needs-input flag — but only when the clear actually happened, so a stale expire cannot silence a session still blocked on a newer question (
clearClarifyRequest()returns that verdict).Also clears parked requests on a gateway switch: they key off runtime session ids the next backend re-mints, and the card no longer self-limits via the running flag.
Verification
scripts/run_tests.sh tests/tui_gateway/: 487 passed, 0 failednpm run typecheckandeslintclean_blockbridge on a worker thread and asserts the replayedrequest_idactually releases the blocked tool — not a mock.Reviewed adversarially by a second model, which cleared lock discipline on
_prompt_lock(no deadlock), payload safety (sudo -S -p '' payload is{}; secret carries the request, never the value), and backward compat in both directions.