diff --git a/apps/desktop/src/app/session/hooks/use-message-stream.ts b/apps/desktop/src/app/session/hooks/use-message-stream.ts index fc0d8ec3e35..f4dbb244e51 100644 --- a/apps/desktop/src/app/session/hooks/use-message-stream.ts +++ b/apps/desktop/src/app/session/hooks/use-message-stream.ts @@ -798,13 +798,16 @@ export function useMessageStream({ ) } } else if (event.type === 'clarify.request') { - if (!isActiveEvent) { - return - } - // Surface the clarify tool's overlay. The Python side is blocked on // `clarify.respond`, so without this handler the agent would hang // forever (see tools/clarify_tool.py + tui_gateway/server.py:_block). + // + // Store the request for whichever session raised it — even a background + // one. clarify.request is a one-shot event; if we dropped it for an + // unfocused session, that session would block on `clarify.respond` + // indefinitely and re-focusing it could never recover (the event is + // gone). Parking it per-session lets the user answer once they switch + // over; the inline ClarifyTool reads the active session's entry. const requestId = typeof payload?.request_id === 'string' ? payload.request_id : '' const question = typeof payload?.question === 'string' ? payload.question : '' @@ -815,6 +818,17 @@ export function useMessageStream({ choices: Array.isArray(payload?.choices) ? payload!.choices!.filter(c => typeof c === 'string') : null, sessionId: sessionId ?? null }) + + // The transcript only renders the active session, so a background + // clarify is otherwise invisible (the row just keeps spinning like + // it's working). Nudge the user toward the chat that needs them. + if (!isActiveEvent) { + notify({ + kind: 'info', + title: 'Another chat needs input', + message: 'Hermes asked a question in a background session. Open it to answer.' + }) + } } } else if (event.type === 'error') { const errorMessage = payload?.message || 'Hermes reported an error' diff --git a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx index 266e554ad11..1fb652f04c2 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx @@ -103,7 +103,7 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { answer }) triggerHaptic('submit') - clearClarifyRequest(matchingRequest.requestId) + clearClarifyRequest(matchingRequest.requestId, matchingRequest.sessionId) // The matching tool.complete will land shortly after, swapping this // panel for the ToolFallback view above. } catch (error) { diff --git a/apps/desktop/src/store/clarify.test.ts b/apps/desktop/src/store/clarify.test.ts new file mode 100644 index 00000000000..269004b49f1 --- /dev/null +++ b/apps/desktop/src/store/clarify.test.ts @@ -0,0 +1,81 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { + $clarifyRequest, + $clarifyRequests, + type ClarifyRequest, + clearClarifyRequest, + setClarifyRequest +} from './clarify' +import { $activeSessionId } from './session' + +function clarify(sessionId: string | null, requestId: string): ClarifyRequest { + return { + requestId, + question: `question-${requestId}`, + choices: null, + sessionId + } +} + +describe('clarify store', () => { + beforeEach(() => { + $clarifyRequests.set({}) + $activeSessionId.set(null) + }) + + afterEach(() => { + $clarifyRequests.set({}) + $activeSessionId.set(null) + }) + + it('keeps clarify requests from concurrent sessions independent', () => { + setClarifyRequest(clarify('session-a', 'req-a')) + setClarifyRequest(clarify('session-b', 'req-b')) + + expect($clarifyRequests.get()['session-a']?.requestId).toBe('req-a') + expect($clarifyRequests.get()['session-b']?.requestId).toBe('req-b') + }) + + it('exposes only the active session via the focus-scoped view', () => { + setClarifyRequest(clarify('session-a', 'req-a')) + setClarifyRequest(clarify('session-b', 'req-b')) + + $activeSessionId.set('session-a') + expect($clarifyRequest.get()?.requestId).toBe('req-a') + + $activeSessionId.set('session-b') + expect($clarifyRequest.get()?.requestId).toBe('req-b') + + $activeSessionId.set('session-c') + expect($clarifyRequest.get()).toBeNull() + }) + + it('clears only the targeted session, leaving the other pending', () => { + setClarifyRequest(clarify('session-a', 'req-a')) + setClarifyRequest(clarify('session-b', 'req-b')) + + clearClarifyRequest('req-a', 'session-a') + + expect($clarifyRequests.get()['session-a']).toBeUndefined() + expect($clarifyRequests.get()['session-b']?.requestId).toBe('req-b') + }) + + it('ignores a stale clear whose request id no longer matches', () => { + setClarifyRequest(clarify('session-a', 'req-a2')) + + clearClarifyRequest('req-a1', 'session-a') + + expect($clarifyRequests.get()['session-a']?.requestId).toBe('req-a2') + }) + + it('clears by request id across sessions when no session hint is given', () => { + setClarifyRequest(clarify('session-a', 'shared')) + setClarifyRequest(clarify('session-b', 'other')) + + clearClarifyRequest('shared') + + expect($clarifyRequests.get()['session-a']).toBeUndefined() + expect($clarifyRequests.get()['session-b']?.requestId).toBe('other') + }) +}) diff --git a/apps/desktop/src/store/clarify.ts b/apps/desktop/src/store/clarify.ts index ce90f48d8c9..14a98f15c88 100644 --- a/apps/desktop/src/store/clarify.ts +++ b/apps/desktop/src/store/clarify.ts @@ -1,4 +1,6 @@ -import { atom } from 'nanostores' +import { atom, computed } from 'nanostores' + +import { $activeSessionId } from './session' export interface ClarifyRequest { requestId: string @@ -7,26 +9,61 @@ export interface ClarifyRequest { sessionId: string | null } -// Holds the request_id (and metadata) for the most recent in-flight -// clarify call. The inline ClarifyTool component (rendered inside the -// assistant message stream) reads this to know which request_id to send -// back over `clarify.respond`. -export const $clarifyRequest = atom(null) +// Pending clarify requests keyed by the runtime session id that raised them. +// Storing per-session (instead of one shared slot) lets a *background* session +// park its clarify request while the user is looking at a different chat, then +// resolve it once they switch over — without a second concurrent clarify +// clobbering the first. A request with no session id lands under the empty key. +const keyFor = (sessionId: string | null | undefined): string => sessionId ?? '' + +export const $clarifyRequests = atom>({}) + +// The clarify request for the currently-viewed session. The inline ClarifyTool +// only ever mounts inside the active session's transcript, so it reads this +// focus-scoped view rather than reaching into the whole map. +export const $clarifyRequest = computed( + [$clarifyRequests, $activeSessionId], + (requests, activeId) => requests[keyFor(activeId)] ?? null +) export function setClarifyRequest(request: ClarifyRequest): void { - $clarifyRequest.set(request) + $clarifyRequests.set({ ...$clarifyRequests.get(), [keyFor(request.sessionId)]: request }) } -export function clearClarifyRequest(requestId?: string): void { - const current = $clarifyRequest.get() +export function clearClarifyRequest(requestId?: string, sessionId?: string | null): void { + const requests = $clarifyRequests.get() + + // Targeted clear when the caller knows the session (the common path from the + // inline ClarifyTool answering its own request). + if (sessionId !== undefined) { + const key = keyFor(sessionId) + const current = requests[key] + + if (!current || (requestId && current.requestId !== requestId)) { + return + } + + const next = { ...requests } + delete next[key] + $clarifyRequests.set(next) - if (!current) { return } - if (requestId && current.requestId !== requestId) { - return + // Fallback with no session hint: drop every entry matching the request id + // (or clear all when none is given). + const next: Record = {} + let changed = false + + for (const [key, value] of Object.entries(requests)) { + if (requestId && value.requestId !== requestId) { + next[key] = value + } else { + changed = true + } } - $clarifyRequest.set(null) + if (changed) { + $clarifyRequests.set(next) + } }