From 89d9d0d97fc200bbd1e681abd38f22f0f360c249 Mon Sep 17 00:00:00 2001 From: Costas Panay Date: Wed, 29 Jul 2026 11:46:33 -0700 Subject: [PATCH] fix(desktop): keep the clarify card until it is answered, skipped, or expired MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../clarify-hydration.test.tsx | 33 ++++- .../hooks/use-message-stream/gateway-event.ts | 18 +++ .../hooks/use-session-actions/index.ts | 13 ++ .../clarify-tool-lifetime.test.tsx | 140 ++++++++++++++++++ .../components/assistant-ui/clarify-tool.tsx | 57 ++++++- apps/desktop/src/lib/gateway-events.ts | 4 + apps/desktop/src/store/clarify-rearm.test.ts | 63 ++++++++ apps/desktop/src/store/clarify.ts | 72 ++++++++- apps/desktop/src/store/gateway-switch.ts | 5 + apps/desktop/src/types/hermes.ts | 11 ++ .../tui_gateway/test_pending_prompt_replay.py | 126 ++++++++++++++++ tui_gateway/server.py | 42 ++++++ 12 files changed, 579 insertions(+), 5 deletions(-) create mode 100644 apps/desktop/src/components/assistant-ui/clarify-tool-lifetime.test.tsx create mode 100644 apps/desktop/src/store/clarify-rearm.test.ts create mode 100644 tests/tui_gateway/test_pending_prompt_replay.py diff --git a/apps/desktop/src/app/session/hooks/use-message-stream/clarify-hydration.test.tsx b/apps/desktop/src/app/session/hooks/use-message-stream/clarify-hydration.test.tsx index 6af488231f..829a2d8195 100644 --- a/apps/desktop/src/app/session/hooks/use-message-stream/clarify-hydration.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-message-stream/clarify-hydration.test.tsx @@ -5,7 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { ClientSessionState } from '@/app/types' import { createClientSessionState } from '@/lib/chat-runtime' -import { clearClarifyRequest } from '@/store/clarify' +import { $clarifyRequests, clearClarifyRequest } from '@/store/clarify' import type { RpcEvent } from '@/types/hermes' import { useMessageStream } from './index' @@ -57,6 +57,9 @@ async function mountStream() { const clarifyRequest = (payload: Record) => act(() => handleEvent!({ payload, session_id: SID, type: 'clarify.request' })) +const clarifyExpire = (payload: Record) => + act(() => handleEvent!({ payload, session_id: SID, type: 'clarify.expire' })) + const toolStart = (payload: Record) => act(() => handleEvent!({ payload, session_id: SID, type: 'tool.start' })) @@ -105,6 +108,34 @@ describe('clarify.request stream hydration', () => { expect(clarifyParts()).toHaveLength(1) }) + it('drops the live request and the needs-input flag when the clarify expires', async () => { + await mountStream() + + clarifyRequest({ choices: ['a'], question: 'Pick', request_id: 'req-exp' }) + + expect($clarifyRequests.get()[SID]).toBeTruthy() + expect(stateRef?.current.get(SID)?.needsInput).toBe(true) + + // The server-side wait elapsed: the request is gone from the gateway's + // `_pending`, so the card must stop pretending to be answerable. + clarifyExpire({ request_id: 'req-exp' }) + + expect($clarifyRequests.get()[SID]).toBeUndefined() + expect(stateRef?.current.get(SID)?.needsInput).toBe(false) + }) + + it('leaves a newer clarify alone when a stale expire arrives', async () => { + await mountStream() + + clarifyRequest({ choices: ['a'], question: 'Pick', request_id: 'req-new' }) + clarifyExpire({ request_id: 'req-old' }) + + expect($clarifyRequests.get()[SID]?.requestId).toBe('req-new') + // The session is still blocked on the newer question, so the sidebar's + // attention indicator must survive a no-op expire. + expect(stateRef?.current.get(SID)?.needsInput).toBe(true) + }) + it('does not duplicate when clarify.request arrives before the tool.start row', async () => { await mountStream() diff --git a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts index 82dae7895c..cc33001e6c 100644 --- a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts +++ b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts @@ -739,6 +739,24 @@ export function useGatewayEventHandler(deps: GatewayEventDeps) { title: translateNow('notifications.native.inputTitle') }) } + } else if (event.type === 'clarify.expire') { + // The server-side wait elapsed (agent.clarify_timeout) and the tool + // returned empty. Without this the card kept offering choices that + // could no longer resolve anything — the request is gone from the + // gateway's `_pending`. Drop the live request so the row settles into + // the "Skipped" card (which keeps the options actionable as a drafted + // follow-up) instead of pretending to still be answerable. + const expiredId = typeof payload?.request_id === 'string' ? payload.request_id : '' + + const cleared = clearClarifyRequest(expiredId || undefined, sessionId ?? undefined) + + // Only when THIS session's live request was the one that expired. A + // stale expire (the request was already superseded or answered) is a + // no-op, and must not drop the sidebar indicator on a session that is + // still blocked on a newer question. + if (cleared && sessionId) { + updateSessionState(sessionId, state => (state.needsInput ? { ...state, needsInput: false } : state)) + } } else if (event.type === 'approval.request') { // Dangerous-command / execute_code approval. The Python side is blocked // in _await_gateway_decision() until approval.respond lands; without diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts index 30e5043d51..f15f7cff02 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts @@ -8,6 +8,7 @@ import { useI18n } from '@/i18n' import { type ChatMessage, preserveLocalAssistantErrors, toChatMessages } from '@/lib/chat-messages' import { isMissingRpcMethod } from '@/lib/gateway-rpc' import { setSessionYolo } from '@/lib/yolo-session' +import { rearmPendingPrompts } from '@/store/clarify' import { migrateSessionDraft } from '@/store/composer' import { clearQueuedPrompts, migrateQueuedPrompts } from '@/store/composer-queue' import { $pinnedSessionIds } from '@/store/layout' @@ -733,6 +734,11 @@ export function useSessionActions({ sessionStateByRuntimeIdRef.current.delete(cachedRuntimeId) dropSessionState(cachedRuntimeId) } else { + // Same re-arm as the cold path: session.activate is how a + // reopened window attaches to an already-live session, which is + // exactly the case where the one-shot clarify.request was missed. + rearmPendingPrompts(cachedRuntimeId, activated.pending_prompts) + const runtimeInfo = applyRuntimeInfo(activated.info) let activatedMessages = @@ -951,6 +957,13 @@ export function useSessionActions({ setActiveSessionId(resumed.session_id) activeSessionIdRef.current = resumed.session_id + // Re-arm anything the session is still blocked on. `clarify.request` is + // a one-shot event, so a window that opened after the tool blocked has + // no live request to answer with — the inline card would render the + // stored question and then be unable to resolve it. A cold resume mints + // a fresh runtime id and so normally carries nothing here; the payload + // is populated on the reuse-live path a genuinely blocked session takes. + rearmPendingPrompts(resumed.session_id, resumed.pending_prompts) const runtimeInfo = applyRuntimeInfo(resumed.info) patchSessionWorkspace(storedSessionId, runtimeInfo?.cwd) diff --git a/apps/desktop/src/components/assistant-ui/clarify-tool-lifetime.test.tsx b/apps/desktop/src/components/assistant-ui/clarify-tool-lifetime.test.tsx new file mode 100644 index 0000000000..d104f9aba9 --- /dev/null +++ b/apps/desktop/src/components/assistant-ui/clarify-tool-lifetime.test.tsx @@ -0,0 +1,140 @@ +import type { ToolCallMessagePartProps } from '@assistant-ui/react' +import { cleanup, render, screen } from '@testing-library/react' +import type { ReactNode } from 'react' +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { I18nProvider } from '@/i18n' +import { clearClarifyRequest, setClarifyRequest } from '@/store/clarify' +import { $gateway } from '@/store/gateway' +import { $activeSessionId } from '@/store/session' + +import { clarifyRequestMatchesRow, ClarifyTool } from './clarify-tool' + +// The turn is NOT running — the state a `session.resume` leaves behind while a +// clarify is still parked on the session (resume sets busy true on entry, then +// back to the resumed `running`, which the deferred-resume path reports as +// false). The card must survive that. +vi.mock('@assistant-ui/react', () => ({ + useAuiState: () => false +})) + +afterEach(() => { + cleanup() + clearClarifyRequest() + $activeSessionId.set(null) + $gateway.set(null) + vi.clearAllMocks() +}) + +function renderClarify(ui: ReactNode) { + return render( + + {ui} + + ) +} + +function clarifyProps(): ToolCallMessagePartProps { + const args = { choices: ['staging', 'production'], question: 'Which deployment target?' } + + return { + addResult: vi.fn(), + args, + argsText: JSON.stringify(args), + isError: false, + respondToApproval: vi.fn(), + result: undefined, + resume: vi.fn(), + status: { type: 'running' }, + toolCallId: 'clarify-live', + toolName: 'clarify', + type: 'tool-call' + } +} + +describe('ClarifyTool lifetime is owned by the request, not the running turn', () => { + it('stays answerable while a request is parked even though the turn reads not-running', () => { + $activeSessionId.set('session-1') + $gateway.set({ request: vi.fn().mockResolvedValue({ ok: true }) } as never) + setClarifyRequest({ + choices: ['staging', 'production'], + question: 'Which deployment target?', + requestId: 'req-live', + sessionId: 'session-1' + }) + + renderClarify() + + // The interactive panel, not the inert ToolFallback row. + expect(screen.getByText('Which deployment target?')).toBeTruthy() + expect(screen.getAllByRole('button', { name: /staging/ }).length).toBeGreaterThan(0) + }) + + it('falls back to the inert row once the request is gone and the turn is not running', () => { + $activeSessionId.set('session-1') + + renderClarify() + + // No live request → nothing to answer with; the panel must not offer + // choices that can no longer resolve anything. + expect(screen.queryByRole('button', { name: /staging/ })).toBeNull() + }) + + it('ignores a request parked on a different session', () => { + $activeSessionId.set('session-1') + setClarifyRequest({ + choices: ['staging', 'production'], + question: 'Which deployment target?', + requestId: 'req-other', + sessionId: 'session-2' + }) + + renderClarify() + + expect(screen.queryByRole('button', { name: /staging/ })).toBeNull() + }) + + it('does not claim a NEWER request belonging to a different row', () => { + // Two clarifies in one transcript: this row is the older, un-resulted one + // (interrupted or expired), and a different question is now live. The old + // row must settle inertly — claiming the live request would leave it stuck + // on a spinner forever, since the pending panel rejects the mismatch. + $activeSessionId.set('session-1') + setClarifyRequest({ + choices: ['yes', 'no'], + question: 'A completely different question?', + requestId: 'req-new', + sessionId: 'session-1' + }) + + const { container } = renderClarify() + + expect(screen.queryByRole('button', { name: /staging/ })).toBeNull() + expect(container.querySelector('[role="status"]')).toBeNull() + }) +}) + +describe('clarifyRequestMatchesRow', () => { + const request = { question: 'Which deployment target?', requestId: 'req-1' } + + it('matches on request id even when the questions differ', () => { + // A row hydrated straight from clarify.request carries the request id as + // its tool call id; the args may not have landed yet. + expect(clarifyRequestMatchesRow(request, 'req-1', undefined)).toBe(true) + expect(clarifyRequestMatchesRow(request, 'req-1', 'stale text')).toBe(true) + }) + + it('matches on question when the ids differ', () => { + // The tool.start row carries the model's own tool_call_id, so ids never + // line up and the question is the only correlator. + expect(clarifyRequestMatchesRow(request, 'call-abc', 'Which deployment target?')).toBe(true) + }) + + it('rejects a different question on a different id', () => { + expect(clarifyRequestMatchesRow(request, 'call-abc', 'Some other question?')).toBe(false) + }) + + it('rejects when there is no request at all', () => { + expect(clarifyRequestMatchesRow(null, 'req-1', 'Which deployment target?')).toBe(false) + }) +}) diff --git a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx index 6f1442e2cf..c3b4a2767f 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx @@ -197,10 +197,65 @@ export const ClarifyTool = (props: ToolCallMessagePartProps) => { return } +/** + * Whether `request` is the one THIS row is showing. + * + * A transcript can hold several clarify rows, so "the session has a clarify + * parked" is not enough to decide a given row is live — an older, un-resulted + * row would otherwise claim a newer request and then reject it downstream, + * leaving a spinner with no exit. Correlate on the request id first (a row + * hydrated from `clarify.request` carries it as its tool call id), then on the + * question text (the `tool.start` row carries the model's own tool_call_id, so + * ids differ and only the args can match them up). + */ +export function clarifyRequestMatchesRow( + request: { question: string; requestId: string } | null, + toolCallId: string | undefined, + argsQuestion: string | undefined +): boolean { + if (!request) { + return false + } + + if (toolCallId && request.requestId && toolCallId === request.requestId) { + return true + } + + // No question on either side to compare — fall back to accepting, which + // preserves the pre-correlation behaviour for rows whose args never arrived. + if (!argsQuestion || !request.question) { + return true + } + + return argsQuestion === request.question +} + function ClarifyToolLive(props: ToolCallMessagePartProps) { const messageRunning = useAuiState(selectMessageRunning) + // The clarify parked on the session this row belongs to. Its presence — not + // the thread's running flag — is what makes the card answerable. + const sessionId = useStore(useSessionView().$runtimeId) + const $request = useMemo(() => sessionClarifyRequest(sessionId), [sessionId]) + const request = useStore($request) + const fromArgs = useMemo(() => readClarifyArgs(props.args), [props.args]) + + // A live request for THIS row outranks `messageRunning`. The thread's running + // flag is a renderer-side turn indicator, and a `session.resume` deliberately + // clears it (use-session-actions sets busy=true on entry, then back to the + // resumed `running`, which the deferred-resume path reports as false). Gating + // the panel on it tore a still-live question off the screen the moment the + // user clicked into the chat — the box appeared, then vanished, with the agent + // still blocked on `clarify.respond`. The card now lives and dies with its own + // request: answered, skipped, expired, or interrupted. + if (clarifyRequestMatchesRow(request, props.toolCallId, fromArgs.question)) { + return + } - // Stopped mid-prompt with no result — don't leave a dead interactive panel. + // No live request of our own — either this row is a stale earlier clarify, or + // the turn was stopped mid-prompt. Don't leave a dead interactive panel. + // `messageRunning` still covers the tool.start → clarify.request race, where + // the row mounts a tick before the request lands (ClarifyToolPending holds a + // spinner until it does). if (!messageRunning) { return } diff --git a/apps/desktop/src/lib/gateway-events.ts b/apps/desktop/src/lib/gateway-events.ts index 005e79705c..896e0bc41d 100644 --- a/apps/desktop/src/lib/gateway-events.ts +++ b/apps/desktop/src/lib/gateway-events.ts @@ -21,6 +21,10 @@ const UNSCOPED_STREAM_EVENT_TYPES = new Set([ 'approval.request', 'browser.progress', 'clarify.request', + // Pairs with clarify.request: the expiry must reach the same pinned session + // even after the user switches chats mid-turn, or a background chat's dead + // card never settles. + 'clarify.expire', 'error', 'message.complete', 'message.delta', diff --git a/apps/desktop/src/store/clarify-rearm.test.ts b/apps/desktop/src/store/clarify-rearm.test.ts new file mode 100644 index 0000000000..d57dbed1c3 --- /dev/null +++ b/apps/desktop/src/store/clarify-rearm.test.ts @@ -0,0 +1,63 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { $clarifyRequests, clearClarifyRequest, rearmPendingPrompts } from './clarify' + +afterEach(() => { + clearClarifyRequest() + vi.restoreAllMocks() +}) + +const parked = (sessionId: string) => Boolean($clarifyRequests.get()[sessionId]) + +const clarifyPrompt = (payload: Record) => ({ event: 'clarify.request', payload }) + +describe('rearmPendingPrompts', () => { + it('restores a clarify the window never saw announced', () => { + rearmPendingPrompts( + 'session-1', + [clarifyPrompt({ choices: ['a', 'b'], question: 'Ship it?', request_id: 'req-1' })] + ) + + const restored = $clarifyRequests.get()['session-1'] + + expect(restored).toMatchObject({ + choices: ['a', 'b'], + question: 'Ship it?', + requestId: 'req-1', + sessionId: 'session-1' + }) + }) + + it('is a no-op for an empty or absent list', () => { + rearmPendingPrompts('session-1', undefined) + rearmPendingPrompts('session-1', []) + + expect(parked('session-1')).toBe(false) + }) + + it('ignores blocking prompts it does not own', () => { + rearmPendingPrompts('session-1', [ + { event: 'sudo.request', payload: { question: 'password?', request_id: 'req-sudo' } }, + { event: 'secret.request', payload: { question: 'token?', request_id: 'req-secret' } } + ]) + + expect(parked('session-1')).toBe(false) + }) + + it('skips a payload missing the request id or question — an unanswerable card is worse than none', () => { + rearmPendingPrompts('session-1', [ + clarifyPrompt({ question: 'no id', request_id: '' }), + clarifyPrompt({ question: '', request_id: 'req-2' }) + ]) + + expect(parked('session-1')).toBe(false) + }) + + it('falls back to free text when every choice normalizes away', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + + rearmPendingPrompts('session-1', [clarifyPrompt({ choices: ['', ' '], question: 'Ship it?', request_id: 'req-3' })]) + + expect($clarifyRequests.get()['session-1'].choices).toBeNull() + }) +}) diff --git a/apps/desktop/src/store/clarify.ts b/apps/desktop/src/store/clarify.ts index f90dcd1317..46289cfe36 100644 --- a/apps/desktop/src/store/clarify.ts +++ b/apps/desktop/src/store/clarify.ts @@ -65,7 +65,13 @@ export function setClarifyRequest(request: ClarifyRequest): void { $clarifyRequests.set({ ...$clarifyRequests.get(), [keyFor(request.sessionId)]: request }) } -export function clearClarifyRequest(requestId?: string, sessionId?: string | null): void { +/** + * Drop a parked clarify. Returns whether anything was actually removed, so a + * caller can avoid acting on a no-op — a stale `clarify.expire` for a request + * that has already been superseded must not clear the sidebar's "needs input" + * flag on a session that is still blocked. + */ +export function clearClarifyRequest(requestId?: string, sessionId?: string | null): boolean { const requests = $clarifyRequests.get() // Targeted clear when the caller knows the session (the common path from the @@ -75,14 +81,14 @@ export function clearClarifyRequest(requestId?: string, sessionId?: string | nul const current = requests[key] if (!current || (requestId && current.requestId !== requestId)) { - return + return false } const next = { ...requests } delete next[key] $clarifyRequests.set(next) - return + return true } // Fallback with no session hint: drop every entry matching the request id @@ -101,4 +107,64 @@ export function clearClarifyRequest(requestId?: string, sessionId?: string | nul if (changed) { $clarifyRequests.set(next) } + + return changed +} + +/** + * Re-arm the blocking prompts a resumed/activated session is still waiting on. + * + * `clarify.request` is emitted exactly once, when the tool blocks. A window + * that opens afterwards — a reopened chat, an app restart, a reconnect that + * dropped the frame — never saw it, so there is no entry here and the inline + * card can never become answerable: it renders the persisted question from the + * tool-call args and then has nothing to respond WITH. The agent stays blocked + * until `agent.clarify_timeout` elapses. + * + * `session.resume` / `session.activate` carry `pending_prompts`, so the + * renderer restores the same state the live event would have produced. + * + * Only `clarify.request` is restored: it is the one blocking prompt whose UI is + * an inline transcript row keyed off a store entry. Approvals, sudo and secret + * prompts render as overlays owned by `store/prompts.ts` and are left alone + * rather than speculatively wired. Lives here, beside the store it writes, so + * the resume path doesn't import through a second module into this one. + */ +export function rearmPendingPrompts( + sessionId: null | string, + prompts: { event: string; payload: Record }[] | undefined +): void { + if (!prompts?.length) { + return + } + + for (const prompt of prompts) { + if (prompt?.event !== 'clarify.request') { + continue + } + + const payload = prompt.payload ?? {} + const requestId = typeof payload.request_id === 'string' ? payload.request_id : '' + const question = typeof payload.question === 'string' ? payload.question : '' + + // A card with no request id could not be answered, and one with no question + // could not be read — an unanswerable card is worse than none. + if (!requestId || !question) { + continue + } + + const rawChoices = payload.choices + const choices = normalizeChoices(rawChoices) + + if (rawChoices != null && choices.length === 0) { + warnDroppedChoices('gateway', question, rawChoices) + } + + setClarifyRequest({ + choices: choices.length > 0 ? choices : null, + question, + requestId, + sessionId + }) + } } diff --git a/apps/desktop/src/store/gateway-switch.ts b/apps/desktop/src/store/gateway-switch.ts index 291b803283..0b6f57a2e5 100644 --- a/apps/desktop/src/store/gateway-switch.ts +++ b/apps/desktop/src/store/gateway-switch.ts @@ -2,6 +2,7 @@ import { atom } from 'nanostores' import { resetSidebarBatchCapability } from '@/hermes' import { invalidateProfileScopedQueries } from '@/lib/query-client' +import { clearClarifyRequest } from '@/store/clarify' import { resetSessionsLimit } from '@/store/layout' import { $unreadFinishedSessionIds, @@ -52,6 +53,10 @@ export function wipeSessionListsForGatewaySwitch(): void { // $attentionSessionIds (computed) and $stalledSessionIds (owned beside it). // $unreadFinishedSessionIds is separate, so wipe it explicitly. clearAllSessionStates() + // Blocking prompts are keyed by runtime session id, which the next backend + // mints fresh — a leftover entry would be unanswerable and, since the card no + // longer self-limits on the running flag, could surface on an unrelated row. + clearClarifyRequest() $unreadFinishedSessionIds.set([]) setSessionsLoading(true) resetSessionsLimit() diff --git a/apps/desktop/src/types/hermes.ts b/apps/desktop/src/types/hermes.ts index 713840dce7..bc6eec7b65 100644 --- a/apps/desktop/src/types/hermes.ts +++ b/apps/desktop/src/types/hermes.ts @@ -470,6 +470,10 @@ export interface SessionResumeResponse { info?: SessionRuntimeInfo message_count: number messages: SessionMessage[] + /** Blocking prompts (clarify/sudo/secret/terminal.read) still waiting on this + * session. The announcing event is one-shot, so a client that attaches after + * the tool blocked re-arms from here. Absent when nothing is pending. */ + pending_prompts?: SessionPendingPrompt[] resumed: string running?: boolean session_id: string @@ -478,6 +482,13 @@ export interface SessionResumeResponse { status?: string } +/** One entry of `SessionResumeResponse.pending_prompts` — the original event + * name plus the verbatim payload that was emitted (it carries `request_id`). */ +export interface SessionPendingPrompt { + event: string + payload: Record +} + export interface SessionRuntimeInfo { approval_mode?: 'manual' | 'off' | 'smart' branch?: string diff --git a/tests/tui_gateway/test_pending_prompt_replay.py b/tests/tui_gateway/test_pending_prompt_replay.py new file mode 100644 index 0000000000..3b89304fcd --- /dev/null +++ b/tests/tui_gateway/test_pending_prompt_replay.py @@ -0,0 +1,126 @@ +"""A blocking prompt must be recoverable after its one-shot event is missed. + +``clarify.request`` (and its sudo/secret/terminal.read siblings) is emitted +once, at the moment the tool blocks. A desktop window that opens afterwards — +reopened chat, app restart, reconnect that dropped the frame — never saw it, so +its inline card can render the stored question but has no ``request_id`` to +answer with, and the agent stays blocked until ``agent.clarify_timeout``. + +``session.resume`` / ``session.activate`` therefore carry ``pending_prompts`` +so a client can re-arm. These tests exercise the real ``_block`` bridge on a +worker thread rather than hand-stuffing the module dicts. +""" + +import threading + +import pytest + +from tui_gateway import server + + +@pytest.fixture(autouse=True) +def _clean_prompt_state(): + with server._prompt_lock: + server._pending.clear() + server._pending_prompt_payloads.clear() + server._answers.clear() + yield + with server._prompt_lock: + server._pending.clear() + server._pending_prompt_payloads.clear() + server._answers.clear() + + +def _block_in_thread(sid: str, payload: dict, event: str = "clarify.request"): + """Start a real ``_block`` wait on ``sid`` and return (thread, answered).""" + result: dict = {} + + def run(): + result["answer"] = server._block(event, sid, dict(payload), timeout=10) + + thread = threading.Thread(target=run, daemon=True) + thread.start() + # Wait until the bridge has registered the prompt. + deadline = threading.Event() + for _ in range(200): + if server._session_pending_prompts(sid): + break + deadline.wait(0.01) + return thread, result + + +def _answer(sid: str) -> str: + prompts = server._session_pending_prompts(sid) + assert prompts, "no pending prompt to answer" + return prompts[0]["payload"]["request_id"] + + +def test_pending_clarify_is_replayable_while_the_agent_waits(monkeypatch): + monkeypatch.setattr(server, "_emit", lambda *a, **k: None) + + thread, result = _block_in_thread("sid-1", {"question": "Ship it?", "choices": ["yes", "no"]}) + + prompts = server._session_pending_prompts("sid-1") + assert len(prompts) == 1 + assert prompts[0]["event"] == "clarify.request" + payload = prompts[0]["payload"] + # The replayed payload must carry everything the one-shot event did — the + # request_id above all, since that is what `clarify.respond` keys off. + assert payload["question"] == "Ship it?" + assert payload["choices"] == ["yes", "no"] + assert payload["request_id"] + + # Answering through the replayed id releases the blocked tool. + server._answers[payload["request_id"]] = "yes" + server._pending[payload["request_id"]][1].set() + thread.join(timeout=5) + assert result["answer"] == "yes" + + +def test_replay_is_empty_once_the_prompt_resolves(monkeypatch): + monkeypatch.setattr(server, "_emit", lambda *a, **k: None) + + thread, _ = _block_in_thread("sid-1", {"question": "Ship it?"}) + request_id = _answer("sid-1") + + server._answers[request_id] = "yes" + server._pending[request_id][1].set() + thread.join(timeout=5) + + # An answered prompt must NOT be replayed — a resuming window would show a + # card for a question that is already settled. + assert server._session_pending_prompts("sid-1") == [] + + +def test_replay_is_scoped_to_its_own_session(monkeypatch): + monkeypatch.setattr(server, "_emit", lambda *a, **k: None) + + thread_a, _ = _block_in_thread("sid-a", {"question": "A?"}) + thread_b, _ = _block_in_thread("sid-b", {"question": "B?"}) + + assert [p["payload"]["question"] for p in server._session_pending_prompts("sid-a")] == ["A?"] + assert [p["payload"]["question"] for p in server._session_pending_prompts("sid-b")] == ["B?"] + assert server._session_pending_prompts("sid-other") == [] + + for sid, thread in (("sid-a", thread_a), ("sid-b", thread_b)): + request_id = _answer(sid) + server._answers[request_id] = "" + server._pending[request_id][1].set() + thread.join(timeout=5) + + +def test_replayed_payload_is_a_copy(monkeypatch): + """A caller mutating the replay must not corrupt the live prompt state.""" + monkeypatch.setattr(server, "_emit", lambda *a, **k: None) + + thread, _ = _block_in_thread("sid-1", {"question": "Ship it?"}) + + replay = server._session_pending_prompts("sid-1") + replay[0]["payload"]["question"] = "tampered" + + assert server._session_pending_prompts("sid-1")[0]["payload"]["question"] == "Ship it?" + + request_id = _answer("sid-1") + server._answers[request_id] = "" + server._pending[request_id][1].set() + thread.join(timeout=5) diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 63fa7acdf1..23672dc30f 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -7125,6 +7125,35 @@ def _(rid, params: dict) -> dict: return _ok(rid, info) +def _session_pending_prompts(sid: str) -> list[dict]: + """Every blocking prompt still waiting on ``sid``, newest last. + + A ``clarify.request`` / ``sudo.request`` / ``secret.request`` / + ``terminal.read.request`` is emitted ONCE, at the moment the tool blocks. + A renderer that was not connected then — a reopened window, an app + restart, a reconnect that dropped the frame — has no way to learn the + question exists, so the card can never become answerable and the agent + stays blocked until its timeout. The state is already tracked for the + lifetime of the wait (``_pending`` + ``_pending_prompt_payloads``); this + hands it back so a resuming client can re-arm. + + Payloads are returned verbatim (they already carry ``request_id``), under + the same lock that writes them, so a prompt resolving concurrently cannot + be half-read. + """ + prompts: list[dict] = [] + with _prompt_lock: + for rid, (owner_sid, _ev) in list(_pending.items()): + if owner_sid != sid: + continue + entry = _pending_prompt_payloads.get(rid) + if not entry: + continue + event, payload = entry + prompts.append({"event": event, "payload": dict(payload)}) + return prompts + + def _session_pending_kind(sid: str) -> str: for rid, (owner_sid, _ev) in list(_pending.items()): if owner_sid != sid: @@ -7339,6 +7368,19 @@ def _live_session_payload( payload["inflight"] = inflight if queued: payload["queued"] = queued + # Blocking prompts outlive the one-shot event that announced them, so a + # client attaching to a live session must be able to re-arm from the + # payload rather than depending on having been connected when the tool + # blocked. This is the only payload that can carry them: a prompt is + # inseparable from the live session whose agent thread is blocked on it, so + # the cold-resume branches (which mint a NEW sid and have no blocked + # thread) have nothing to report by construction. A session actually + # waiting on a clarify is still live, so its resume takes the + # reuse-live fast path through here. Omitted when empty to keep the common + # payload unchanged. + pending_prompts = _session_pending_prompts(sid) + if pending_prompts: + payload["pending_prompts"] = pending_prompts return payload