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 6af488231..829a2d819 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 82dae7895..cc33001e6 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 30e5043d5..f15f7cff0 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 000000000..d104f9aba --- /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 6f1442e2c..c3b4a2767 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 005e79705..896e0bc41 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 000000000..d57dbed1c --- /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 f90dcd131..46289cfe3 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 291b80328..0b6f57a2e 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 713840dce..bc6eec7b6 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 000000000..3b89304fc --- /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 63fa7acdf..23672dc30 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