From d9834a3e863a6c2a2cdbc9a91f32d045a0740fa8 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 14 Sep 2026 00:39:11 -0700 Subject: [PATCH] =?UTF-8?q?test:=20port=20desktop,=20TUI=20and=20gateway?= =?UTF-8?q?=20suites=20to=20server=E2=86=92client=20request=20frames?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old suites asserted the deleted wire (`*.request` events, `*.respond` RPCs, `pending_clarify` snapshots, `_pending`/`_answers` teardowns). Each test keeps its invariant against the new shape: a seeded live request's `respond` spy receives the answer object, `hasOpenServerRequest` flips, the `approval.respond` RPC fallback is asserted ONLY for queue entries restored without a socket, and Bot Mode rooms answer via `request.answer` / `clarify.lock`. The group-turns test that polled forever for a `clarify.respond` that no longer exists (20-minute hang) now completes. --- .../hooks/use-composer-submit.test.tsx | 32 +-- .../hooks/use-session-actions.test.tsx | 66 ++++-- .../assistant-ui/clarify-tool.test.tsx | 202 +++++++++--------- .../assistant-ui/tool/approval.test.tsx | 55 ++++- .../src/components/prompt-overlays.test.tsx | 65 +++++- .../prompt-overlays.vault-code.test.tsx | 41 ++-- .../prompt-overlays.vault-save-login.test.tsx | 43 ++-- .../prompt-overlays.vault-unlock.test.tsx | 31 ++- apps/desktop/src/lib/gateway-events.test.ts | 4 +- .../plugins/hermes-bots/group-test-utils.ts | 3 +- .../plugins/hermes-bots/group-turns.test.ts | 66 +++--- .../src/plugins/hermes-bots/group-turns.ts | 13 +- ...way-profile-only-owner.integration.test.ts | 11 +- .../store/session-states-runtime-map.test.ts | 2 +- 14 files changed, 386 insertions(+), 248 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx index d17c486257..74be58ba86 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx @@ -6,7 +6,6 @@ import { PaneVisibleContext } from '@/components/pane-shell/pane-visibility' import { $clarifyRequests } from '@/store/clarify' import type { ComposerAttachment } from '@/store/composer' import { clearQueuedPrompts, getQueuedPrompts } from '@/store/composer-queue' -import { $gateway } from '@/store/gateway' import { clearAllPrompts, hasBlockingPromptRequest, @@ -14,6 +13,7 @@ import { setSecretRequest, setSudoRequest } from '@/store/prompts' +import { hasOpenServerRequest, rememberServerRequest, resetServerRequestsForTests } from '@/store/server-requests' import { type ComposerTarget, requestComposerSubmit } from '../focus' import { ComposerScopeProvider, ComposerSurfaceProvider, MAIN_COMPOSER_SCOPE } from '../scope' @@ -457,26 +457,30 @@ describe('useComposerSubmit busy-turn routing', () => { }) describe('useComposerSubmit with a clarify parked on the session', () => { - const gatewayRequest = vi.fn(async () => ({ ok: true })) + // The clarify is a live server→client request: skipping it answers that + // request frame (`{ answer: '' }`), not a `clarify.respond` RPC. + const respond = vi.fn() const parkClarify = (sessionId: string) => { + const requestId = `req-${sessionId}` + + rememberServerRequest({ fail: vi.fn(), id: requestId, method: 'clarify', params: {}, respond }) $clarifyRequests.set({ [sessionId]: { - requestId: `req-${sessionId}`, + requestId, question: 'which one?', choices: ['a', 'b'], multiSelect: false, sessionId } }) - $gateway.set({ request: gatewayRequest } as unknown as ReturnType) } afterEach(() => { cleanup() - gatewayRequest.mockClear() + respond.mockClear() + resetServerRequestsForTests() $clarifyRequests.set({}) - $gateway.set(null) vi.restoreAllMocks() }) @@ -488,16 +492,12 @@ describe('useComposerSubmit with a clarify parked on the session', () => { hook.result.current.submitDraft() }) - await waitFor(() => - expect(gatewayRequest).toHaveBeenCalledWith('clarify.respond', { - request_id: 'req-runtime-session', - answer: '' - }) - ) + await waitFor(() => expect(respond).toHaveBeenCalledWith({ answer: '' })) await waitFor(() => expect(onSubmit).toHaveBeenCalledWith('actually do this instead', expect.objectContaining({ attachments: [] })) ) expect($clarifyRequests.get()['runtime-session']).toBeUndefined() + expect(hasOpenServerRequest('req-runtime-session')).toBe(false) }) it('skips the question before steering a busy turn', async () => { @@ -509,7 +509,7 @@ describe('useComposerSubmit with a clarify parked on the session', () => { }) await waitFor(() => expect(onSteer).toHaveBeenCalledWith('change course')) - expect(gatewayRequest).toHaveBeenCalledWith('clarify.respond', { request_id: 'req-runtime-session', answer: '' }) + expect(respond).toHaveBeenCalledWith({ answer: '' }) }) it('leaves the question alone for an empty Enter (Stop, not an answer)', () => { @@ -520,7 +520,8 @@ describe('useComposerSubmit with a clarify parked on the session', () => { hook.result.current.submitDraft() }) - expect(gatewayRequest).not.toHaveBeenCalled() + expect(respond).not.toHaveBeenCalled() + expect(hasOpenServerRequest('req-runtime-session')).toBe(true) expect($clarifyRequests.get()['runtime-session']).toBeDefined() expect(onCancel).toHaveBeenCalledTimes(1) }) @@ -534,7 +535,8 @@ describe('useComposerSubmit with a clarify parked on the session', () => { }) await waitFor(() => expect(onSubmit).toHaveBeenCalled()) - expect(gatewayRequest).not.toHaveBeenCalled() + expect(respond).not.toHaveBeenCalled() + expect(hasOpenServerRequest('req-other-session')).toBe(true) expect($clarifyRequests.get()['other-session']).toBeDefined() }) }) diff --git a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx index b459a0cc82..905eb38954 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx @@ -1136,16 +1136,26 @@ describe('resumeSession failure recovery', () => { const requestGateway = vi.fn(async (method: string) => { if (method === 'session.resume') { + // The channel re-delivers `open_requests` to the request handler BEFORE + // the caller sees the result; that handler parks the card. Mirror that + // ordering here (no channel in this harness). + setClarifyRequest({ + choices: ['safe', 'fast'], + multiSelect: false, + question: 'Which path?', + receivedAt: Date.now(), + requestId: 'req-resumed', + sessionId: 'runtime-1' + }) + return { info: {}, message_count: 2, messages: [], messages_omitted: true, - pending_clarify: { - choices: ['safe', 'fast'], - question: 'Which path?', - request_id: 'req-resumed' - }, + open_requests: [ + { id: 'req-resumed', method: 'clarify', params: { choices: ['safe', 'fast'], question: 'Which path?' } } + ], resumed: 'stored-1', running: true, session_id: 'runtime-1', @@ -1196,21 +1206,34 @@ describe('resumeSession failure recovery', () => { session_id: 'stored-1' } as never) + const questions = [ + { choices: ['Blue', 'Red'], qid: 'q0', question: 'Color?' }, + { choices: ['Small', 'Large'], qid: 'q1', question: 'Size?' } + ] + const requestGateway = vi.fn(async (method: string) => { if (method === 'session.resume') { + // Request handler parks the batch card (with the server-locked answer) + // from the re-delivered open request before the result lands. + setClarifyRequest({ + choices: null, + lockedAnswers: { q0: 'Blue' }, + multiSelect: false, + question: '', + questions: questions.map(q => ({ ...q, multiSelect: false })), + receivedAt: Date.now(), + requestId: 'req-batch-resumed', + sessionId: 'runtime-1' + }) + return { info: {}, message_count: 2, messages: [], messages_omitted: true, - pending_clarify: { - answers: { q0: 'Blue' }, - questions: [ - { choices: ['Blue', 'Red'], qid: 'q0', question: 'Color?' }, - { choices: ['Small', 'Large'], qid: 'q1', question: 'Size?' } - ], - request_id: 'req-batch-resumed' - }, + open_requests: [ + { id: 'req-batch-resumed', method: 'clarify', params: { answers: { q0: 'Blue' }, questions } } + ], resumed: 'stored-1', running: true, session_id: 'runtime-1', @@ -2900,16 +2923,23 @@ describe('resumeSession warm-cache mapping integrity', () => { const requestGateway = vi.fn(async (method: string) => { if (method === 'session.activate') { + setClarifyRequest({ + choices: ['safe', 'fast'], + multiSelect: false, + question: 'Which path?', + receivedAt: Date.now(), + requestId: 'req-warm', + sessionId: 'rt-A' + }) + return { info: {}, message_count: 2, messages: [], messages_omitted: true, - pending_clarify: { - choices: ['safe', 'fast'], - question: 'Which path?', - request_id: 'req-warm' - }, + open_requests: [ + { id: 'req-warm', method: 'clarify', params: { choices: ['safe', 'fast'], question: 'Which path?' } } + ], resumed: 'stored-A', running: true, session_id: 'rt-A', diff --git a/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx b/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx index 4336cb127b..1e2c047327 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx @@ -2,7 +2,7 @@ import type { ToolCallMessagePartProps } from '@assistant-ui/react' import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' import { atom } from 'nanostores' import type { ReactNode } from 'react' -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { onComposerInsertRequest } from '@/app/chat/composer/focus' import { type SessionView, SessionViewProvider } from '@/app/chat/session-view' @@ -12,6 +12,7 @@ import { I18nProvider } from '@/i18n' import { clearClarifyRequest, setClarifyRequest } from '@/store/clarify' import { $gateway } from '@/store/gateway' import { $profiles } from '@/store/profile' +import { hasOpenServerRequest, rememberServerRequest, resetServerRequestsForTests } from '@/store/server-requests' import { $activeSessionId, _resetSessionOwnerHintsForTests, setSessionOwnerHint } from '@/store/session' import { ClarifyTool, readClarifyBatchResult, readClarifyResult } from './clarify-tool' @@ -38,9 +39,23 @@ vi.mock('@assistant-ui/react', () => ({ useAuiState: () => messageRunning })) +/** Seed a live `clarify` server request; the card answers it synchronously with + * `respondToServerRequest`, so the returned spy sees the response frame. */ +function liveServerRequest(id: string) { + const respond = vi.fn() + rememberServerRequest({ fail: vi.fn(), id, method: 'clarify', params: {}, respond }) + + return respond +} + +beforeEach(() => { + resetServerRequestsForTests() +}) + afterEach(() => { cleanup() clearClarifyRequest() + resetServerRequestsForTests() $activeSessionId.set(null) $gateway.set(null) messageRunning = true @@ -99,6 +114,7 @@ function liveClarifyProps(choices = ['staging', 'production']): ToolCallMessageP function renderLiveClarify({ multiSelect = false }: { multiSelect?: boolean } = {}) { const request = vi.fn().mockResolvedValue({ ok: true }) + const respond = liveServerRequest('request-1') $activeSessionId.set('session-1') $gateway.set({ request } as never) @@ -111,7 +127,7 @@ function renderLiveClarify({ multiSelect = false }: { multiSelect?: boolean } = }) const { rerender } = renderClarify() - return { request, rerender } + return { request, rerender, respond } } describe('ClarifyTool live card stays mounted across settle', () => { @@ -135,13 +151,13 @@ describe('ClarifyTool live card stays mounted across settle', () => { }) it('holds the card through the gap between answering and the settled result', async () => { - const { request, rerender } = renderLiveClarify() + const { rerender, respond } = renderLiveClarify() fireEvent.click(screen.getByRole('button', { name: /staging/ })) fireEvent.click(screen.getByRole('button', { name: /Continue/ })) await waitFor(() => { - expect(request).toHaveBeenCalled() + expect(respond).toHaveBeenCalled() }) // tool.complete is what swaps in the settled card; the turn can already @@ -177,7 +193,7 @@ describe('ClarifyTool live card stays mounted across settle', () => { describe('ClarifyTool choice selection', () => { it('selects independently, deselects and submits multi-select choices as a JSON array', async () => { - const { request } = renderLiveClarify({ multiSelect: true }) + const { respond } = renderLiveClarify({ multiSelect: true }) const staging = screen.getByRole('button', { name: /staging/ }) const production = screen.getByRole('button', { name: /production/ }) @@ -197,15 +213,12 @@ describe('ClarifyTool choice selection', () => { fireEvent.click(screen.getByRole('button', { name: /Continue/ })) await waitFor(() => { - expect(request).toHaveBeenCalledWith('clarify.respond', { - answer: JSON.stringify(['production', 'staging']), - request_id: 'request-1' - }) + expect(respond).toHaveBeenCalledWith({ answer: JSON.stringify(['production', 'staging']) }) }) }) it('keeps single-select replacement and plain-string submission', async () => { - const { request } = renderLiveClarify() + const { respond } = renderLiveClarify() const staging = screen.getByRole('button', { name: /staging/ }) const production = screen.getByRole('button', { name: /production/ }) @@ -218,11 +231,9 @@ describe('ClarifyTool choice selection', () => { fireEvent.click(screen.getByRole('button', { name: /Continue/ })) await waitFor(() => { - expect(request).toHaveBeenCalledWith('clarify.respond', { - answer: 'production', - request_id: 'request-1' - }) + expect(respond).toHaveBeenCalledWith({ answer: 'production' }) }) + expect(hasOpenServerRequest('request-1')).toBe(false) }) }) @@ -397,36 +408,30 @@ describe('ClarifyTool keyboard navigation', () => { }) it('selects by number and confirms the answer with Enter', async () => { - const { request } = renderLiveClarify() + const { respond } = renderLiveClarify() fireEvent.keyDown(window, { key: '2' }) fireEvent.keyDown(window, { key: 'Enter' }) await waitFor(() => { - expect(request).toHaveBeenCalledWith('clarify.respond', { - answer: 'production', - request_id: 'request-1' - }) + expect(respond).toHaveBeenCalledWith({ answer: 'production' }) }) }) it('stages a highlighted multi-select choice with Enter and submits it with Continue', async () => { - const { request } = renderLiveClarify({ multiSelect: true }) + const { respond } = renderLiveClarify({ multiSelect: true }) const production = screen.getByRole('button', { name: /production/ }) fireEvent.keyDown(window, { key: 'ArrowDown' }) fireEvent.keyDown(window, { key: 'Enter' }) expect(production.getAttribute('aria-pressed')).toBe('true') - expect(request).not.toHaveBeenCalled() + expect(respond).not.toHaveBeenCalled() fireEvent.click(screen.getByRole('button', { name: /Continue/ })) await waitFor(() => { - expect(request).toHaveBeenCalledWith('clarify.respond', { - answer: JSON.stringify(['production']), - request_id: 'request-1' - }) + expect(respond).toHaveBeenCalledWith({ answer: JSON.stringify(['production']) }) }) }) @@ -445,23 +450,23 @@ describe('ClarifyTool keyboard navigation', () => { }) it('does not intercept keyboard events while an action button has focus', () => { - const { request } = renderLiveClarify() + const { respond } = renderLiveClarify() const skip = screen.getByRole('button', { name: 'Skip' }) skip.focus() expect(fireEvent.keyDown(window, { key: 'Enter' })).toBe(true) expect(fireEvent.keyDown(window, { key: 'ArrowDown' })).toBe(true) - expect(request).not.toHaveBeenCalled() + expect(respond).not.toHaveBeenCalled() }) }) describe('ClarifyTool recommended option', () => { it('dims the (Recommended) label and answers with the choice the backend sent', async () => { - const request = vi.fn().mockResolvedValue({ ok: true }) + const respond = liveServerRequest('request-1') $activeSessionId.set('session-1') - $gateway.set({ request } as never) + $gateway.set({ request: vi.fn() } as never) setClarifyRequest({ choices: ['staging (Recommended)', 'production'], multiSelect: false, @@ -482,10 +487,7 @@ describe('ClarifyTool recommended option', () => { // The decorated string goes back verbatim; the tool strips the label before // the agent ever sees the answer. await waitFor(() => { - expect(request).toHaveBeenCalledWith('clarify.respond', { - answer: 'staging (Recommended)', - request_id: 'request-1' - }) + expect(respond).toHaveBeenCalledWith({ answer: 'staging (Recommended)' }) }) }) }) @@ -564,6 +566,7 @@ function liveBatchProps(): ToolCallMessagePartProps { function renderLiveBatch(lockedAnswers?: Record, multiSelect = false) { const request = vi.fn().mockResolvedValue({ ok: true, remaining: [] }) + const respond = liveServerRequest('request-batch') $activeSessionId.set('session-1') $gateway.set({ request } as never) @@ -581,7 +584,7 @@ function renderLiveBatch(lockedAnswers?: Record, multiSelect = f }) renderClarify() - return request + return { request, respond } } describe('readClarifyBatchResult', () => { @@ -611,23 +614,20 @@ describe('readClarifyBatchResult', () => { describe('ClarifyTool submit shortcut', () => { it('submits selected choices with Cmd/Ctrl+Enter without toggling the focused option', async () => { for (const modifier of [{ metaKey: true }, { ctrlKey: true }]) { - const { request } = renderLiveClarify({ multiSelect: true }) + const { respond } = renderLiveClarify({ multiSelect: true }) const choice = screen.getByRole('button', { name: /staging/ }) fireEvent.click(choice) choice.focus() fireEvent.keyDown(choice, { key: 'Enter', ...modifier }) - await waitFor(() => expect(request).toHaveBeenCalledTimes(1)) - expect(request).toHaveBeenCalledWith('clarify.respond', { - request_id: 'request-1', - answer: '["staging"]' - }) + await waitFor(() => expect(respond).toHaveBeenCalledTimes(1)) + expect(respond).toHaveBeenCalledWith({ answer: '["staging"]' }) cleanup() } }) it('submits a complete batch from its text field while preserving incomplete and multiline input', async () => { for (const modifier of [{ metaKey: true }, { ctrlKey: true }]) { - const request = renderLiveBatch() + const { request } = renderLiveBatch() const field = screen.getByPlaceholderText('Type your answer…') fireEvent.change(field, { target: { value: 'packet' } }) field.focus() @@ -639,7 +639,7 @@ describe('ClarifyTool submit shortcut', () => { expect(request).not.toHaveBeenCalled() fireEvent.keyDown(field, { key: 'Enter', ...modifier }) await waitFor(() => expect(request).toHaveBeenCalledTimes(2)) - expect(request).toHaveBeenNthCalledWith(2, 'clarify.respond', { + expect(request).toHaveBeenNthCalledWith(2, 'clarify.lock', { answer: 'packet', question_id: 'q1', request_id: 'request-batch' @@ -659,7 +659,7 @@ describe('ClarifyTool batch card', () => { }) it('stages locally and keeps the single confirm disabled until all answered', async () => { - const request = renderLiveBatch() + const { request } = renderLiveBatch() const confirm = screen.getByRole('button', { name: /Confirm and continue/ }) expect((confirm as HTMLButtonElement).disabled).toBe(true) @@ -676,8 +676,8 @@ describe('ClarifyTool batch card', () => { expect((confirm as HTMLButtonElement).disabled).toBe(false) }) - it('confirm sends every per-question lock in order and completes the batch', async () => { - const request = renderLiveBatch() + it('confirm sends every per-question clarify.lock in order and completes the batch', async () => { + const { request } = renderLiveBatch() fireEvent.click(screen.getByRole('button', { name: /red/ })) fireEvent.change(screen.getByPlaceholderText('Type your answer…'), { target: { value: 'packet' } }) @@ -686,20 +686,22 @@ describe('ClarifyTool batch card', () => { await waitFor(() => { expect(request).toHaveBeenCalledTimes(2) }) - expect(request).toHaveBeenNthCalledWith(1, 'clarify.respond', { + expect(request).toHaveBeenNthCalledWith(1, 'clarify.lock', { answer: 'red', question_id: 'q0', request_id: 'request-batch' }) - expect(request).toHaveBeenNthCalledWith(2, 'clarify.respond', { + expect(request).toHaveBeenNthCalledWith(2, 'clarify.lock', { answer: 'packet', question_id: 'q1', request_id: 'request-batch' }) + // The last lock resolves the server request; the card forgets it locally. + await waitFor(() => expect(hasOpenServerRequest('request-batch')).toBe(false)) }) it('a staged answer stays editable before confirm', async () => { - const request = renderLiveBatch() + const { request } = renderLiveBatch() fireEvent.click(screen.getByRole('button', { name: /red/ })) fireEvent.click(screen.getByRole('button', { name: /blue/ })) @@ -710,7 +712,7 @@ describe('ClarifyTool batch card', () => { expect(request).toHaveBeenCalledTimes(2) }) // The re-pick won: blue, not red. - expect(request).toHaveBeenNthCalledWith(1, 'clarify.respond', { + expect(request).toHaveBeenNthCalledWith(1, 'clarify.lock', { answer: 'blue', question_id: 'q0', request_id: 'request-batch' @@ -732,17 +734,15 @@ describe('ClarifyTool batch card', () => { expect(screen.getByText('1 of 2 answered')).toBeTruthy() }) - it('Skip cancels the whole batch without a question_id', async () => { - const request = renderLiveBatch() + it('Skip cancels the whole batch with an empty response (no answers)', async () => { + const { request, respond } = renderLiveBatch() fireEvent.click(screen.getByRole('button', { name: 'Skip' })) await waitFor(() => { - expect(request).toHaveBeenCalledWith('clarify.respond', { - answer: '', - request_id: 'request-batch' - }) + expect(respond).toHaveBeenCalledWith({}) }) + expect(request).not.toHaveBeenCalled() }) it('renders the settled batch with all questions and answers', () => { @@ -773,7 +773,10 @@ describe('ClarifyTool batch card', () => { // foreground focus, so after a profile / Bot Chat switch it can be profile B // while the blocking clarify belongs to profile A — the response lands on a // backend that never held the request and the owner stays blocked until the -// tool times out. Every live clarify.respond now routes by request.sessionId. +// tool times out. A clarify answer is now the server request's own response +// frame — it rides the socket the request arrived on, so no routing decision is +// left to make. Only `clarify.lock` (a real client→server RPC) still dials a +// socket, and it must route by request.sessionId. const OWNER_CONNECTION_ID = 'conn-profile-a' const OWNER_PROFILE = 'profile-a' @@ -794,12 +797,12 @@ function armCrossProfileOwner() { return ambient } -function expectOwnerCall(nth: number, params: Record) { +function expectOwnerLock(nth: number, params: Record) { expect(gatewayMocks.requestGatewayForAgent).toHaveBeenNthCalledWith( nth, OWNER_CONNECTION_ID, OWNER_PROFILE, - 'clarify.respond', + 'clarify.lock', params ) } @@ -811,8 +814,9 @@ describe('ClarifyTool owner routing', () => { gatewayMocks.requestGatewayForAgent.mockClear() }) - it('answers a single clarify on the owner socket, never profile B ambient', async () => { + it('answers a single clarify through its server request, never profile B ambient', async () => { const ambient = armCrossProfileOwner() + const respond = liveServerRequest('request-1') setClarifyRequest({ choices: ['staging', 'production'], @@ -827,14 +831,15 @@ describe('ClarifyTool owner routing', () => { fireEvent.click(screen.getByRole('button', { name: /Continue/ })) await waitFor(() => { - expect(gatewayMocks.requestGatewayForAgent).toHaveBeenCalledTimes(1) + expect(respond).toHaveBeenCalledWith({ answer: 'staging' }) }) - expectOwnerCall(1, { answer: 'staging', request_id: 'request-1' }) + expect(gatewayMocks.requestGatewayForAgent).not.toHaveBeenCalled() expect(ambient).not.toHaveBeenCalled() }) it('sends both sequential batch locks on the owner socket, in order', async () => { const ambient = armCrossProfileOwner() + liveServerRequest('request-batch') setClarifyRequest({ choices: null, @@ -857,13 +862,14 @@ describe('ClarifyTool owner routing', () => { expect(gatewayMocks.requestGatewayForAgent).toHaveBeenCalledTimes(2) }) // The LAST lock resolves the blocked tool, so order is load-bearing. - expectOwnerCall(1, { answer: 'red', question_id: 'q0', request_id: 'request-batch' }) - expectOwnerCall(2, { answer: 'packet', question_id: 'q1', request_id: 'request-batch' }) + expectOwnerLock(1, { answer: 'red', question_id: 'q0', request_id: 'request-batch' }) + expectOwnerLock(2, { answer: 'packet', question_id: 'q1', request_id: 'request-batch' }) expect(ambient).not.toHaveBeenCalled() }) - it('sends a batch skip/cancel on the owner socket', async () => { + it('answers a batch skip/cancel through its server request, never profile B ambient', async () => { const ambient = armCrossProfileOwner() + const respond = liveServerRequest('request-batch') setClarifyRequest({ choices: null, @@ -881,9 +887,9 @@ describe('ClarifyTool owner routing', () => { fireEvent.click(screen.getByRole('button', { name: 'Skip' })) await waitFor(() => { - expect(gatewayMocks.requestGatewayForAgent).toHaveBeenCalledTimes(1) + expect(respond).toHaveBeenCalledWith({}) }) - expectOwnerCall(1, { answer: '', request_id: 'request-batch' }) + expect(gatewayMocks.requestGatewayForAgent).not.toHaveBeenCalled() expect(ambient).not.toHaveBeenCalled() }) }) @@ -915,7 +921,11 @@ describe('ClarifyTool visible-card scoping', () => { return { ...liveClarifyProps(), args, argsText: JSON.stringify(args), toolCallId } } + /** Park a clarify on `sessionId` with its own live server request; the + * returned spy sees that request's (and only that request's) answer. */ function parkClarify(requestId: string, sessionId: string) { + const respond = liveServerRequest(requestId) + setClarifyRequest({ choices: ['staging', 'production'], multiSelect: false, @@ -923,6 +933,8 @@ describe('ClarifyTool visible-card scoping', () => { requestId, sessionId }) + + return respond } /** A card inside an inactive tab layer — mounted and live, just not on screen. */ @@ -937,11 +949,9 @@ describe('ClarifyTool visible-card scoping', () => { } it('answers the visible card, not a background one that mounted first', async () => { - const request = vi.fn().mockResolvedValue({ ok: true }) - - $gateway.set({ request } as never) - parkClarify(BACKGROUND_REQUEST, BACKGROUND_SESSION) - parkClarify(FOREGROUND_REQUEST, FOREGROUND_SESSION) + $gateway.set({ request: vi.fn() } as never) + const background = parkClarify(BACKGROUND_REQUEST, BACKGROUND_SESSION) + const foreground = parkClarify(FOREGROUND_REQUEST, FOREGROUND_SESSION) // The background card is rendered FIRST, so its window listener registers // first. Registration order used to decide the winner, which meant the card @@ -958,29 +968,25 @@ describe('ClarifyTool visible-card scoping', () => { fireEvent.keyDown(window, { key: 'Enter' }) await waitFor(() => { - expect(request).toHaveBeenCalledTimes(1) + expect(foreground).toHaveBeenCalledTimes(1) }) - // Exactly one answer, carrying the FOREGROUND request id — the background - // session's turn must not be resumed by a keystroke aimed at this one. - expect(request).toHaveBeenCalledWith('clarify.respond', { - answer: 'staging', - request_id: FOREGROUND_REQUEST - }) + // Exactly one answer, on the FOREGROUND request — the background session's + // turn must not be resumed by a keystroke aimed at this one. + expect(foreground).toHaveBeenCalledWith({ answer: 'staging' }) + expect(background).not.toHaveBeenCalled() }) it('leaves the key alone when the only pending card is hidden', () => { - const request = vi.fn().mockResolvedValue({ ok: true }) - - $gateway.set({ request } as never) - parkClarify(BACKGROUND_REQUEST, BACKGROUND_SESSION) + $gateway.set({ request: vi.fn() } as never) + const background = parkClarify(BACKGROUND_REQUEST, BACKGROUND_SESSION) renderClarify(backgroundCard()) // Untouched (no preventDefault) ⇒ the keystroke stays available to the // composer, matching what `clarifyCardOwnsKey` reports with no visible card. expect(fireEvent.keyDown(window, { key: 'Enter' })).toBe(true) - expect(request).not.toHaveBeenCalled() + expect(background).not.toHaveBeenCalled() }) /** A card in its own split zone — unlike `backgroundCard` this one IS on @@ -998,11 +1004,9 @@ describe('ClarifyTool visible-card scoping', () => { /** Both zones visible, zone-a first in document order. */ function renderSplit() { - const request = vi.fn().mockResolvedValue({ ok: true }) - - $gateway.set({ request } as never) - parkClarify(ZONE_A_REQUEST, ZONE_A_SESSION) - parkClarify(ZONE_B_REQUEST, ZONE_B_SESSION) + $gateway.set({ request: vi.fn() } as never) + const zoneA = parkClarify(ZONE_A_REQUEST, ZONE_A_SESSION) + const zoneB = parkClarify(ZONE_B_REQUEST, ZONE_B_SESSION) renderClarify( <> @@ -1011,42 +1015,38 @@ describe('ClarifyTool visible-card scoping', () => { ) - return request + return { zoneA, zoneB } } it('answers the later-in-document card when its zone is the focused one', async () => { - const request = renderSplit() + const { zoneA, zoneB } = renderSplit() $activeTreeGroup.set('zone-b') fireEvent.keyDown(window, { key: 'Enter' }) await waitFor(() => { - expect(request).toHaveBeenCalledTimes(1) + expect(zoneB).toHaveBeenCalledTimes(1) }) // Both cards are visible and both hold a live window listener, so this is // the case document order gets wrong: it would answer zone-a's question. - expect(request).toHaveBeenCalledWith('clarify.respond', { - answer: 'staging', - request_id: ZONE_B_REQUEST - }) + expect(zoneB).toHaveBeenCalledWith({ answer: 'staging' }) + expect(zoneA).not.toHaveBeenCalled() }) it('answers the other visible card once the focus moves to its zone', async () => { - const request = renderSplit() + const { zoneA, zoneB } = renderSplit() $activeTreeGroup.set('zone-a') fireEvent.keyDown(window, { key: 'Enter' }) await waitFor(() => { - expect(request).toHaveBeenCalledTimes(1) + expect(zoneA).toHaveBeenCalledTimes(1) }) // The direct pin for "the other visible card then cannot receive its // shortcut": neither zone may be permanently starved of its own keys. - expect(request).toHaveBeenCalledWith('clarify.respond', { - answer: 'staging', - request_id: ZONE_A_REQUEST - }) + expect(zoneA).toHaveBeenCalledWith({ answer: 'staging' }) + expect(zoneB).not.toHaveBeenCalled() }) }) diff --git a/apps/desktop/src/components/assistant-ui/tool/approval.test.tsx b/apps/desktop/src/components/assistant-ui/tool/approval.test.tsx index aa0bf399ca..c1c35fd191 100644 --- a/apps/desktop/src/components/assistant-ui/tool/approval.test.tsx +++ b/apps/desktop/src/components/assistant-ui/tool/approval.test.tsx @@ -1,9 +1,10 @@ import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react' -import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' import type { HermesGateway } from '@/hermes' import { $gateway } from '@/store/gateway' import { $approvalRequest, clearAllPrompts, setApprovalRequest } from '@/store/prompts' +import { hasOpenServerRequest, rememberServerRequest, resetServerRequestsForTests } from '@/store/server-requests' import { $activeSessionId } from '@/store/session' import { PendingApprovalFallback, PendingToolApproval } from './approval' @@ -33,12 +34,20 @@ function part(toolName: string): ToolPart { function setRequest( command = 'rm -rf /tmp/x', allowPermanent?: boolean, - extra: { choices?: string[]; smartDenied?: boolean } = {} + extra: { choices?: string[]; requestId?: string; serverRequestId?: string; smartDenied?: boolean } = {} ) { $activeSessionId.set('sess-1') setApprovalRequest({ allowPermanent, command, description: 'dangerous command', sessionId: 'sess-1', ...extra }) } +/** A live `approval` server request the card answers synchronously. */ +function liveApproval(id = 'srq-approval') { + const respond = vi.fn() + rememberServerRequest({ fail: vi.fn(), id, method: 'approval', params: {}, respond }) + + return respond +} + function mockGateway() { const request = vi.fn().mockResolvedValue({ resolved: true }) $gateway.set({ request } as unknown as HermesGateway) @@ -46,9 +55,14 @@ function mockGateway() { return request } +beforeEach(() => { + resetServerRequestsForTests() +}) + afterEach(() => { cleanup() clearAllPrompts() + resetServerRequestsForTests() $activeSessionId.set(null) $gateway.set(null) }) @@ -83,15 +97,37 @@ describe('PendingToolApproval', () => { expect(screen.getByRole('button', { name: /Reject/ })).toBeTruthy() }) - it('sends approval.respond {choice: "once"} and clears the request on Run', async () => { + it('answers the live approval request with {choice: "once"} and clears the request on Run', async () => { const request = mockGateway() - setRequest() + const respond = liveApproval() + setRequest('rm -rf /tmp/x', undefined, { requestId: 'apr-1', serverRequestId: 'srq-approval' }) render() fireEvent.click(screen.getByRole('button', { name: /Run/ })) await waitFor(() => { - expect(request).toHaveBeenCalledWith('approval.respond', { choice: 'once', session_id: 'sess-1' }) + expect(respond).toHaveBeenCalledWith({ choice: 'once' }) + }) + expect(hasOpenServerRequest('srq-approval')).toBe(false) + expect(request).not.toHaveBeenCalledWith('approval.respond', expect.anything()) + expect($approvalRequest.get()).toBeNull() + }) + + it('falls back to the approval.respond RPC when no live server request is registered', async () => { + // A prompt restored from `approval.pending` (no socket carried the frame): + // the queue-level RPC is the only way to answer it. + const request = mockGateway() + setRequest('rm -rf /tmp/x', undefined, { requestId: 'apr-1' }) + render() + + fireEvent.click(screen.getByRole('button', { name: /Run/ })) + + await waitFor(() => { + expect(request).toHaveBeenCalledWith('approval.respond', { + choice: 'once', + request_id: 'apr-1', + session_id: 'sess-1' + }) }) expect($approvalRequest.get()).toBeNull() }) @@ -109,15 +145,16 @@ describe('PendingToolApproval', () => { expect(screen.getByText(longCommand)).toBeTruthy() }) - it('sends choice "deny" on Reject', async () => { - const request = mockGateway() - setRequest() + it('answers the live approval request with {choice: "deny"} on Reject', async () => { + mockGateway() + const respond = liveApproval() + setRequest('rm -rf /tmp/x', undefined, { requestId: 'apr-1', serverRequestId: 'srq-approval' }) render() fireEvent.click(screen.getByRole('button', { name: /Reject/ })) await waitFor(() => { - expect(request).toHaveBeenCalledWith('approval.respond', { choice: 'deny', session_id: 'sess-1' }) + expect(respond).toHaveBeenCalledWith({ choice: 'deny' }) }) }) diff --git a/apps/desktop/src/components/prompt-overlays.test.tsx b/apps/desktop/src/components/prompt-overlays.test.tsx index dd9cdb7ec0..f5f4730b54 100644 --- a/apps/desktop/src/components/prompt-overlays.test.tsx +++ b/apps/desktop/src/components/prompt-overlays.test.tsx @@ -1,10 +1,11 @@ import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { I18nProvider } from '@/i18n' import { $gateway } from '@/store/gateway' import { notifyError } from '@/store/notifications' import { $secretRequest, $sudoRequest, clearAllPrompts, setSecretRequest, setSudoRequest } from '@/store/prompts' +import { hasOpenServerRequest, rememberServerRequest, resetServerRequestsForTests } from '@/store/server-requests' import { $activeSessionId } from '@/store/session' import { PromptOverlays } from './prompt-overlays' @@ -20,20 +21,27 @@ function renderPrompts(sessionId: string | null = 's1') { ) } +beforeEach(() => { + resetServerRequestsForTests() +}) + afterEach(() => { cleanup() clearAllPrompts() + resetServerRequestsForTests() $activeSessionId.set(null) $gateway.set(null) vi.clearAllMocks() }) describe('PromptOverlays', () => { - it('dismisses a stale sudo dialog when the gateway no longer has the password request', async () => { - const request = vi.fn().mockRejectedValue(new Error('no pending password request')) + it('answers the live sudo request with an empty value on Cancel and clears the dialog', async () => { + const respond = vi.fn() + const request = vi.fn() $activeSessionId.set('s1') $gateway.set({ request } as never) + rememberServerRequest({ fail: vi.fn(), id: 'sudo-1', method: 'sudo', params: {}, respond }) setSudoRequest({ requestId: 'sudo-1', sessionId: 's1' }) renderPrompts() @@ -43,12 +51,55 @@ describe('PromptOverlays', () => { fireEvent.click(screen.getByRole('button', { name: 'Cancel' })) await waitFor(() => expect($sudoRequest.get()).toBeNull()) - expect(request).toHaveBeenCalledWith('sudo.respond', { password: '', request_id: 'sudo-1' }) + expect(respond).toHaveBeenCalledWith({ value: '' }) + expect(hasOpenServerRequest('sudo-1')).toBe(false) + expect(request).not.toHaveBeenCalled() expect(notifyError).not.toHaveBeenCalled() }) - it('dismisses a stale secret dialog when the gateway no longer has the value request', async () => { - const request = vi.fn().mockRejectedValue(new Error('no pending value request')) + it('dismisses a stale sudo dialog when the gateway no longer has the request open', async () => { + const request = vi.fn() + + $activeSessionId.set('s1') + $gateway.set({ request } as never) + // No server request registered under this id: it expired / was cancelled. + setSudoRequest({ requestId: 'sudo-1', sessionId: 's1' }) + + renderPrompts() + + expect(screen.getByText('Administrator password')).toBeTruthy() + + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })) + + await waitFor(() => expect($sudoRequest.get()).toBeNull()) + expect(request).not.toHaveBeenCalled() + expect(notifyError).not.toHaveBeenCalled() + }) + + it('answers the live secret request with an empty value on Cancel and clears the dialog', async () => { + const respond = vi.fn() + const request = vi.fn() + + $activeSessionId.set('s1') + $gateway.set({ request } as never) + rememberServerRequest({ fail: vi.fn(), id: 'secret-1', method: 'secret', params: {}, respond }) + setSecretRequest({ envVar: 'TEST_SECRET', prompt: 'Paste a secret', requestId: 'secret-1', sessionId: 's1' }) + + renderPrompts() + + expect(screen.getByText('TEST_SECRET')).toBeTruthy() + + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })) + + await waitFor(() => expect($secretRequest.get()).toBeNull()) + expect(respond).toHaveBeenCalledWith({ value: '' }) + expect(hasOpenServerRequest('secret-1')).toBe(false) + expect(request).not.toHaveBeenCalled() + expect(notifyError).not.toHaveBeenCalled() + }) + + it('dismisses a stale secret dialog when the gateway no longer has the request open', async () => { + const request = vi.fn() $activeSessionId.set('s1') $gateway.set({ request } as never) @@ -61,7 +112,7 @@ describe('PromptOverlays', () => { fireEvent.click(screen.getByRole('button', { name: 'Cancel' })) await waitFor(() => expect($secretRequest.get()).toBeNull()) - expect(request).toHaveBeenCalledWith('secret.respond', { request_id: 'secret-1', value: '' }) + expect(request).not.toHaveBeenCalled() expect(notifyError).not.toHaveBeenCalled() }) }) diff --git a/apps/desktop/src/components/prompt-overlays.vault-code.test.tsx b/apps/desktop/src/components/prompt-overlays.vault-code.test.tsx index 0e30f525b5..004d1ef070 100644 --- a/apps/desktop/src/components/prompt-overlays.vault-code.test.tsx +++ b/apps/desktop/src/components/prompt-overlays.vault-code.test.tsx @@ -1,16 +1,8 @@ import { cleanup, fireEvent, render, waitFor } from '@testing-library/react' -import { afterEach, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, expect, it, vi } from 'vitest' import { stubResizeObserver } from '@/test/jsdom' -const gatewayMocks = vi.hoisted(() => ({ - requestGatewayForAgent: vi.fn(async () => ({ status: 'ok' })) -})) - -vi.mock('@/store/gateway', async importActual => ({ - ...(await importActual>()), - requestGatewayForAgent: gatewayMocks.requestGatewayForAgent -})) vi.mock('@/lib/haptics', () => ({ triggerHaptic: vi.fn() })) vi.mock('@/store/notifications', () => ({ notify: vi.fn(), notifyError: vi.fn() })) @@ -18,26 +10,35 @@ import { PromptOverlays } from '@/components/prompt-overlays' import { $gateway } from '@/store/gateway' import { $profiles } from '@/store/profile' import { clearAllPrompts, sessionVaultCodeRequest, setVaultCodeRequest } from '@/store/prompts' +import { hasOpenServerRequest, rememberServerRequest, resetServerRequestsForTests } from '@/store/server-requests' import { $activeSessionId, _resetSessionOwnerHintsForTests, setSessionOwnerHint } from '@/store/session' stubResizeObserver() +beforeEach(() => { + resetServerRequestsForTests() +}) + afterEach(() => { cleanup() clearAllPrompts() + resetServerRequestsForTests() _resetSessionOwnerHintsForTests() $gateway.set(null) vi.clearAllMocks() }) -// The 2FA code goes to the OWNING profile socket as vault.code.respond, whitespace/dashes stripped -// (users paste "246 810" from an SMS); Skip answers "". -it('sends the trimmed code to the owning profile socket', async () => { +// The 2FA code answers the `vault.code` server request (the frame rides the socket the +// request arrived on, never the ambient gateway), whitespace/dashes stripped (users paste +// "246 810" from an SMS); Skip answers "". +it('answers the vault.code server request with the trimmed code, never the ambient gateway', async () => { $profiles.set([{ name: 'owner' }, { name: 'profile-b' }] as never) setSessionOwnerHint('session-a', { connectionId: 'conn-1', profile: 'owner' }) const ambient = vi.fn().mockResolvedValue({ status: 'ok' }) + const respond = vi.fn() $activeSessionId.set('session-b') $gateway.set({ request: ambient } as never) + rememberServerRequest({ fail: vi.fn(), id: 'req-c', method: 'vault.code', params: {}, respond }) setVaultCodeRequest({ hint: '', requestId: 'req-c', sessionId: 'session-a', site: 'github.com' }) render() @@ -49,13 +50,9 @@ it('sends the trimmed code to the owning profile socket', async () => { expect(submit.disabled).toBe(false) fireEvent.submit(input.closest('form')!) - await waitFor(() => expect(gatewayMocks.requestGatewayForAgent).toHaveBeenCalledTimes(1)) - expect((gatewayMocks.requestGatewayForAgent.mock.calls[0] as unknown[]).slice(0, 4)).toEqual([ - 'conn-1', - 'owner', - 'vault.code.respond', - { code: '246810', request_id: 'req-c' } - ]) + await waitFor(() => expect(respond).toHaveBeenCalledTimes(1)) + expect(respond).toHaveBeenCalledWith({ value: '246810' }) + expect(hasOpenServerRequest('req-c')).toBe(false) expect(ambient).not.toHaveBeenCalled() await waitFor(() => expect(sessionVaultCodeRequest('session-a').get()).toBeNull()) }) @@ -63,13 +60,15 @@ it('sends the trimmed code to the owning profile socket', async () => { it('Skip answers an empty code and clears the card', async () => { $profiles.set([{ name: 'owner' }] as never) setSessionOwnerHint('session-a', { connectionId: 'conn-1', profile: 'owner' }) + const respond = vi.fn() $gateway.set({ request: vi.fn() } as never) + rememberServerRequest({ fail: vi.fn(), id: 'req-d', method: 'vault.code', params: {}, respond }) setVaultCodeRequest({ hint: '', requestId: 'req-d', sessionId: 'session-a', site: 'github.com' }) render() fireEvent.click(Array.from(document.querySelectorAll('button')).find(b => b.textContent === 'Skip')!) - await waitFor(() => expect(gatewayMocks.requestGatewayForAgent).toHaveBeenCalledTimes(1)) - expect((gatewayMocks.requestGatewayForAgent.mock.calls[0] as unknown[])[3]).toEqual({ code: '', request_id: 'req-d' }) + await waitFor(() => expect(respond).toHaveBeenCalledTimes(1)) + expect(respond).toHaveBeenCalledWith({ value: '' }) await waitFor(() => expect(sessionVaultCodeRequest('session-a').get()).toBeNull()) }) diff --git a/apps/desktop/src/components/prompt-overlays.vault-save-login.test.tsx b/apps/desktop/src/components/prompt-overlays.vault-save-login.test.tsx index 7a70508be5..50e78073e4 100644 --- a/apps/desktop/src/components/prompt-overlays.vault-save-login.test.tsx +++ b/apps/desktop/src/components/prompt-overlays.vault-save-login.test.tsx @@ -1,16 +1,8 @@ import { cleanup, fireEvent, render, waitFor } from '@testing-library/react' -import { afterEach, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, expect, it, vi } from 'vitest' import { stubResizeObserver } from '@/test/jsdom' -const gatewayMocks = vi.hoisted(() => ({ - requestGatewayForAgent: vi.fn(async () => ({ status: 'ok' })) -})) - -vi.mock('@/store/gateway', async importActual => ({ - ...(await importActual>()), - requestGatewayForAgent: gatewayMocks.requestGatewayForAgent -})) vi.mock('@/lib/haptics', () => ({ triggerHaptic: vi.fn() })) vi.mock('@/store/notifications', () => ({ notify: vi.fn(), notifyError: vi.fn() })) @@ -18,26 +10,36 @@ import { PromptOverlays } from '@/components/prompt-overlays' import { $gateway } from '@/store/gateway' import { $profiles } from '@/store/profile' import { clearAllPrompts, sessionVaultSaveLoginRequest, setVaultSaveLoginRequest } from '@/store/prompts' +import { hasOpenServerRequest, rememberServerRequest, resetServerRequestsForTests } from '@/store/server-requests' import { $activeSessionId, _resetSessionOwnerHintsForTests, setSessionOwnerHint } from '@/store/session' stubResizeObserver() +beforeEach(() => { + resetServerRequestsForTests() +}) + afterEach(() => { cleanup() clearAllPrompts() + resetServerRequestsForTests() _resetSessionOwnerHintsForTests() $gateway.set(null) vi.clearAllMocks() }) -// The "save this login" card is the zero-setup path: the pair goes to the OWNING profile's socket as -// one JSON answer, the password field is masked, and Save is disabled until both fields are filled. -it('sends identifier + password as one vault.save_login.respond to the owning profile socket', async () => { +// The "save this login" card is the zero-setup path: the pair answers the `vault.save_login` +// server request as one JSON value (the frame rides the socket the request arrived on, never +// the ambient gateway), the password field is masked, and Save is disabled until both fields +// are filled. +it('answers the vault.save_login server request with identifier + password as one JSON value', async () => { $profiles.set([{ name: 'owner' }, { name: 'profile-b' }] as never) setSessionOwnerHint('session-a', { connectionId: 'conn-1', profile: 'owner' }) const ambient = vi.fn().mockResolvedValue({ status: 'ok' }) + const respond = vi.fn() $activeSessionId.set('session-b') $gateway.set({ request: ambient } as never) + rememberServerRequest({ fail: vi.fn(), id: 'req-s', method: 'vault.save_login', params: {}, respond }) setVaultSaveLoginRequest({ origin: 'https://github.com', requestId: 'req-s', @@ -57,13 +59,13 @@ it('sends identifier + password as one vault.save_login.respond to the owning pr expect(submit.disabled).toBe(false) fireEvent.submit(password.closest('form')!) - await waitFor(() => expect(gatewayMocks.requestGatewayForAgent).toHaveBeenCalledTimes(1)) - const [conn, profile, method, params] = gatewayMocks.requestGatewayForAgent.mock.calls[0] as unknown[] - expect([conn, profile, method]).toEqual(['conn-1', 'owner', 'vault.save_login.respond']) - expect(JSON.parse((params as { login: string }).login)).toEqual({ + await waitFor(() => expect(respond).toHaveBeenCalledTimes(1)) + const [result] = respond.mock.calls[0] as [{ value: string }] + expect(JSON.parse(result.value)).toEqual({ identifier: 'tek@acme.test', password: 'fixture-pw' }) + expect(hasOpenServerRequest('req-s')).toBe(false) expect(ambient).not.toHaveBeenCalled() await waitFor(() => expect(sessionVaultSaveLoginRequest('session-a').get()).toBeNull()) }) @@ -71,7 +73,9 @@ it('sends identifier + password as one vault.save_login.respond to the owning pr it("Don't save answers an empty login and clears the card", async () => { $profiles.set([{ name: 'owner' }] as never) setSessionOwnerHint('session-a', { connectionId: 'conn-1', profile: 'owner' }) + const respond = vi.fn() $gateway.set({ request: vi.fn() } as never) + rememberServerRequest({ fail: vi.fn(), id: 'req-d', method: 'vault.save_login', params: {}, respond }) setVaultSaveLoginRequest({ origin: 'https://github.com', requestId: 'req-d', @@ -83,10 +87,7 @@ it("Don't save answers an empty login and clears the card", async () => { const decline = Array.from(document.querySelectorAll('button')).find(b => b.textContent === "Don't save")! fireEvent.click(decline) - await waitFor(() => expect(gatewayMocks.requestGatewayForAgent).toHaveBeenCalledTimes(1)) - expect((gatewayMocks.requestGatewayForAgent.mock.calls[0] as unknown[])[3]).toEqual({ - login: '', - request_id: 'req-d' - }) + await waitFor(() => expect(respond).toHaveBeenCalledTimes(1)) + expect(respond).toHaveBeenCalledWith({ value: '' }) await waitFor(() => expect(sessionVaultSaveLoginRequest('session-a').get()).toBeNull()) }) diff --git a/apps/desktop/src/components/prompt-overlays.vault-unlock.test.tsx b/apps/desktop/src/components/prompt-overlays.vault-unlock.test.tsx index 901380e3f3..11189aeea6 100644 --- a/apps/desktop/src/components/prompt-overlays.vault-unlock.test.tsx +++ b/apps/desktop/src/components/prompt-overlays.vault-unlock.test.tsx @@ -1,16 +1,8 @@ import { cleanup, fireEvent, render, waitFor } from '@testing-library/react' -import { afterEach, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, expect, it, vi } from 'vitest' import { stubResizeObserver } from '@/test/jsdom' -const gatewayMocks = vi.hoisted(() => ({ - requestGatewayForAgent: vi.fn(async () => ({ status: 'ok' })) -})) - -vi.mock('@/store/gateway', async importActual => ({ - ...(await importActual>()), - requestGatewayForAgent: gatewayMocks.requestGatewayForAgent -})) vi.mock('@/lib/haptics', () => ({ triggerHaptic: vi.fn() })) vi.mock('@/store/notifications', () => ({ notify: vi.fn(), notifyError: vi.fn() })) @@ -18,13 +10,19 @@ import { PromptOverlays } from '@/components/prompt-overlays' import { $gateway } from '@/store/gateway' import { $profiles } from '@/store/profile' import { clearAllPrompts, sessionVaultUnlockRequest, setVaultUnlockRequest } from '@/store/prompts' +import { hasOpenServerRequest, rememberServerRequest, resetServerRequestsForTests } from '@/store/server-requests' import { $activeSessionId, _resetSessionOwnerHintsForTests, setSessionOwnerHint } from '@/store/session' stubResizeObserver() +beforeEach(() => { + resetServerRequestsForTests() +}) + afterEach(() => { cleanup() clearAllPrompts() + resetServerRequestsForTests() _resetSessionOwnerHintsForTests() $gateway.set(null) vi.clearAllMocks() @@ -33,12 +31,16 @@ afterEach(() => { // A master password typed into a background profile's unlock card must reach the // backend that raised the prompt. The window's ambient `$gateway` may be another // profile's socket entirely; sending the password there is a cross-backend leak. -it('routes the master password to the owning profile socket, never the ambient gateway', async () => { +// The answer is the `vault.unlock` server request's own response frame — it rides +// the socket the request arrived on, so the ambient gateway is never dialled. +it('answers the vault.unlock server request with the master password, never the ambient gateway', async () => { $profiles.set([{ name: 'owner' }, { name: 'profile-b' }] as never) setSessionOwnerHint('session-a', { connectionId: 'conn-1', profile: 'owner' }) const ambient = vi.fn().mockResolvedValue({ status: 'ok' }) + const respond = vi.fn() $activeSessionId.set('session-b') $gateway.set({ request: ambient } as never) + rememberServerRequest({ fail: vi.fn(), id: 'req-a', method: 'vault.unlock', params: {}, respond }) setVaultUnlockRequest({ backend: 'bitwarden', displayName: 'Bitwarden', requestId: 'req-a', sessionId: 'session-a' }) render() @@ -46,12 +48,9 @@ it('routes the master password to the owning profile socket, never the ambient g fireEvent.change(input, { target: { value: 'fixture-master' } }) fireEvent.submit(input.closest('form')!) - await waitFor(() => expect(gatewayMocks.requestGatewayForAgent).toHaveBeenCalledTimes(1)) - expect(gatewayMocks.requestGatewayForAgent.mock.calls[0].slice(0, 3)).toEqual([ - 'conn-1', - 'owner', - 'vault.unlock.respond' - ]) + await waitFor(() => expect(respond).toHaveBeenCalledTimes(1)) + expect(respond).toHaveBeenCalledWith({ value: 'fixture-master' }) + expect(hasOpenServerRequest('req-a')).toBe(false) expect(ambient).not.toHaveBeenCalled() await waitFor(() => expect(sessionVaultUnlockRequest('session-a').get()).toBeNull()) }) diff --git a/apps/desktop/src/lib/gateway-events.test.ts b/apps/desktop/src/lib/gateway-events.test.ts index b9af988f8c..5a7efe397e 100644 --- a/apps/desktop/src/lib/gateway-events.test.ts +++ b/apps/desktop/src/lib/gateway-events.test.ts @@ -34,7 +34,9 @@ describe('gateway event routing', () => { expect(gatewayEventRequiresSessionId('message.interim')).toBe(false) expect(gatewayEventRequiresSessionId('reasoning.delta')).toBe(false) expect(gatewayEventRequiresSessionId('tool.start')).toBe(false) - expect(gatewayEventRequiresSessionId('approval.request')).toBe(false) + // Prompts are server→client requests now; the one prompt-related EVENT + // left (`request.cancel`) is likewise the focused turn's own when unscoped. + expect(gatewayEventRequiresSessionId('request.cancel')).toBe(false) }) it('allows global events to remain unscoped', () => { diff --git a/apps/desktop/src/plugins/hermes-bots/group-test-utils.ts b/apps/desktop/src/plugins/hermes-bots/group-test-utils.ts index ff826a2b3d..9429d42f10 100644 --- a/apps/desktop/src/plugins/hermes-bots/group-test-utils.ts +++ b/apps/desktop/src/plugins/hermes-bots/group-test-utils.ts @@ -85,6 +85,7 @@ export interface GatewayOptions { /** Per profile: carry `pending_approval` on its first `until` resumes. */ approvalUntil?: Record; until: number }> /** Per profile: carry `pending_clarify` on its first `until` resumes. */ + /** `payload` is an open-request frame `{ id, method: 'clarify', params }`. */ clarifyUntil?: Record; until: number }> /** Land a competing writer's `ui_meta` under `key` during the FIRST * `profiles.configure`, then reject it as a CAS conflict — the race the @@ -275,7 +276,7 @@ export function createGroupGateway(options: GatewayOptions = {}): ScriptedGatewa running: false, session_id: session.runtime, session_key: session.stored, - ...(clarify && seen <= clarify.until ? { pending_clarify: clarify.payload } : {}), + ...(clarify && seen <= clarify.until ? { open_requests: [clarify.payload] } : {}), ...(approval && seen <= approval.until ? { pending_approval: approval.payload } : {}) } } diff --git a/apps/desktop/src/plugins/hermes-bots/group-turns.test.ts b/apps/desktop/src/plugins/hermes-bots/group-turns.test.ts index 7b5cde48c9..76992f0ffc 100644 --- a/apps/desktop/src/plugins/hermes-bots/group-turns.test.ts +++ b/apps/desktop/src/plugins/hermes-bots/group-turns.test.ts @@ -344,11 +344,12 @@ describe('reply selection (#94376)', () => { // command approval. Those live in a hidden session the room can't see, so the // poll mirrors them into the room and holds the turn open until they resolve. describe('clarify and approvals (#90694)', () => { + /** The member's blocking clarify as `session.resume` now reports it: an + * open server→client request frame (`open_requests`). */ const CLARIFY = { - choices: ['staging', 'prod'], - multi_select: false, - question: 'Which env should I target?', - request_id: 'req-clarify-1' + id: 'req-clarify-1', + method: 'clarify', + params: { choices: ['staging', 'prod'], multi_select: false, question: 'Which env should I target?' } } const APPROVAL = { @@ -365,7 +366,7 @@ describe('clarify and approvals (#90694)', () => { const room = await loadRoom({ clarifyUntil: { research: { payload: CLARIFY, until: 3 } }, // The mirror pass runs while the question is still blocking — this is - // the observable proof the gate inspected pending_clarify. Asserting + // the observable proof the gate inspected open_requests. Asserting // on $groupNeedsYou/$groupClarify AFTER the turn lands proves nothing: // the clarify has already resolved and its mirror is gone by then. onResumePoll: () => { @@ -401,7 +402,7 @@ describe('clarify and approvals (#90694)', () => { const { chat, turns } = await loadRoom() const member: GroupMember = { name: 'research', title: '' } - expect(turns.syncGroupClarify('Core', member, { pending_clarify: CLARIFY })).toBe(true) + expect(turns.syncGroupClarify('Core', member, { open_requests: [CLARIFY] })).toBe(true) const mirrored = Object.values(chat.$groupClarify.get()) @@ -414,7 +415,7 @@ describe('clarify and approvals (#90694)', () => { expect(turns.groupHasPendingClarify(chat.$groupClarify.get(), 'Core')).toBe(true) // Same request again: no new entry, identity preserved. - turns.syncGroupClarify('Core', member, { pending_clarify: CLARIFY }) + turns.syncGroupClarify('Core', member, { open_requests: [CLARIFY] }) expect(Object.values(chat.$groupClarify.get())[0]).toBe(mirrored[0]) @@ -425,45 +426,50 @@ describe('clarify and approvals (#90694)', () => { expect(turns.groupHasPendingClarify(chat.$groupClarify.get(), 'Core')).toBe(false) }) - it('never mirrors a question for older backends without pending_clarify', async () => { + it('never mirrors a question for older backends without open_requests', async () => { const { chat, turns } = await loadRoom() expect(turns.syncGroupClarify('Core', { name: 'research' }, { messages: [] })).toBe(false) expect(Object.keys(chat.$groupClarify.get())).toHaveLength(0) }) - it('routes an answer through clarify.respond and clears the mirror', async () => { + it('answers the open request by id through request.answer and clears the mirror', async () => { const room = await loadRoom() const member: GroupMember = { name: 'research', title: '' } - room.turns.syncGroupClarify('Core', member, { pending_clarify: CLARIFY }) + room.turns.syncGroupClarify('Core', member, { open_requests: [CLARIFY] }) await room.turns.answerGroupClarify(Object.values(room.chat.$groupClarify.get())[0], member, 'staging') - expect(room.gateway.rpcFor('clarify.respond').map(call => call.params)).toEqual([ - { answer: 'staging', request_id: 'req-clarify-1' } + expect(room.gateway.rpcFor('request.answer').map(call => call.params)).toEqual([ + { id: 'req-clarify-1', result: { answer: 'staging' } } ]) expect(Object.keys(room.chat.$groupClarify.get())).toHaveLength(0) }) - it('sends one respond per batch question, in order', async () => { + it('locks one batch question per clarify.lock call, in order', async () => { const room = await loadRoom() const member: GroupMember = { name: 'research', title: '' } room.turns.syncGroupClarify('Core', member, { - pending_clarify: { - questions: [ - { choices: ['staging', 'prod'], qid: 'q0', question: 'Env?' }, - { choices: [], qid: 'q1', question: 'Region?' } - ], - request_id: 'req-batch-1' - } + open_requests: [ + { + id: 'req-batch-1', + method: 'clarify', + params: { + questions: [ + { choices: ['staging', 'prod'], qid: 'q0', question: 'Env?' }, + { choices: [], qid: 'q1', question: 'Region?' } + ] + } + } + ] }) await room.turns.answerGroupClarify(Object.values(room.chat.$groupClarify.get())[0], member, { q0: 'staging', q1: 'eu-west' }) - expect(room.gateway.rpcFor('clarify.respond').map(call => call.params)).toEqual([ + expect(room.gateway.rpcFor('clarify.lock').map(call => call.params)).toEqual([ { answer: 'staging', question_id: 'q0', request_id: 'req-batch-1' }, { answer: 'eu-west', question_id: 'q1', request_id: 'req-batch-1' } ]) @@ -473,8 +479,8 @@ describe('clarify and approvals (#90694)', () => { it('clears only the disbanded room’s mirrored questions', async () => { const room = await loadRoom() - room.turns.syncGroupClarify('Core', { name: 'research' }, { pending_clarify: CLARIFY }) - room.turns.syncGroupClarify('Other', { name: 'ops' }, { pending_clarify: { ...CLARIFY, request_id: 'req-2' } }) + room.turns.syncGroupClarify('Core', { name: 'research' }, { open_requests: [CLARIFY] }) + room.turns.syncGroupClarify('Other', { name: 'ops' }, { open_requests: [{ ...CLARIFY, id: 'req-2' }] }) room.turns.clearGroupClarify('Core') const remaining = Object.values(room.chat.$groupClarify.get()) @@ -489,7 +495,7 @@ describe('clarify and approvals (#90694)', () => { it('keeps pending prompts independent from mention attention through their lifecycle', async () => { const { chat, turns } = await loadRoom() const member = { name: 'research', title: '' } - turns.syncGroupClarify('Core', member, { pending_clarify: CLARIFY }) + turns.syncGroupClarify('Core', member, { open_requests: [CLARIFY] }) expect(chat.$groupNeedsYou.get().Core).toBeFalsy() turns.syncGroupClarify('Core', { name: 'ops' }, { pending_approval: APPROVAL }) expect(Object.values(chat.$groupClarify.get())).toHaveLength(2) @@ -540,7 +546,7 @@ describe('clarify and approvals (#90694)', () => { submitted = true } - if (method === 'clarify.respond') { + if (method === 'request.answer') { answered = true } @@ -550,7 +556,7 @@ describe('clarify and approvals (#90694)', () => { await held } - return { ...result, pending_clarify: CLARIFY } + return { ...result, open_requests: [CLARIFY] } } return result @@ -725,7 +731,7 @@ describe('clarify and approvals (#90694)', () => { if (recreate) { room.chat.updateGroupChat('Core', current => ({ ...current, roomId: 'replacement-room' })) - room.turns.syncGroupClarify('Core', member, { pending_clarify: CLARIFY }) + room.turns.syncGroupClarify('Core', member, { open_requests: [CLARIFY] }) } const roomsBefore = structuredClone(room.chat.$groupChats.get()) @@ -789,7 +795,7 @@ describe('clarify and approvals (#90694)', () => { } } - return { running: true, pending_clarify: CLARIFY } + return { running: true, open_requests: [CLARIFY] } } await room.rounds.runGroupChatRounds('Core', members, 'thread') @@ -889,14 +895,14 @@ describe('clarify and approvals (#90694)', () => { expect(room.gateway.rpcFor('approval.respond').map(call => call.params)).toEqual([ { choice: 'once', request_id: 'req-approval-1', session_id: 'rt-research-1' } ]) - expect(room.gateway.rpcFor('clarify.respond')).toHaveLength(0) + expect(room.gateway.rpcFor('request.answer')).toHaveLength(0) expect(Object.keys(room.chat.$groupClarify.get())).toHaveLength(0) }) it('lets clarify outrank approval when a snapshot carries both', async () => { const { chat, turns } = await loadRoom() - turns.syncGroupClarify('Core', { name: 'research' }, { pending_approval: APPROVAL, pending_clarify: CLARIFY }) + turns.syncGroupClarify('Core', { name: 'research' }, { open_requests: [CLARIFY], pending_approval: APPROVAL }) const entry = Object.values(chat.$groupClarify.get())[0] diff --git a/apps/desktop/src/plugins/hermes-bots/group-turns.ts b/apps/desktop/src/plugins/hermes-bots/group-turns.ts index a0517f9c2f..3f9c8a9e5f 100644 --- a/apps/desktop/src/plugins/hermes-bots/group-turns.ts +++ b/apps/desktop/src/plugins/hermes-bots/group-turns.ts @@ -101,8 +101,10 @@ interface GroupSessionSnapshot { inflight?: boolean message_count?: number messages?: GroupTurnTranscriptMessage[] + /** Still-open server→client requests (`server_requests.open_requests`); the + * member's blocking clarify question lives here as `{ id, method: 'clarify', params }`. */ + open_requests?: { id: string; method: string; params: Record }[] pending_approval?: GroupPendingApproval - pending_clarify?: GroupPendingClarify running?: boolean session_id?: string session_key?: string @@ -437,13 +439,18 @@ const GROUP_TURN_HARD_CAP_MS = 20 * 60000 * `${group}::${memberKey}` (#90694). Returns true while a prompt is * blocking, so the turn poll can extend its deadline — a waiting prompt * must not be eaten by the group-turn timeout. Feature-detected: older - * backends without `pending_clarify`/`pending_approval` in the resume + * backends without `open_requests`/`pending_approval` in the resume * payload always sync to "no prompt". Clarify wins when both are somehow * present (approvals resolve inside tool batches; clarify is the outer * blocker). */ export function syncGroupClarify(group: string, member: GroupMember, state: GroupSessionSnapshot | null): boolean { const key = `${group}::${groupMemberKey(member)}` - const clarify = state && typeof state.pending_clarify === 'object' ? state.pending_clarify : null + const openClarify = Array.isArray(state?.open_requests) + ? state.open_requests.find(entry => entry?.method === 'clarify' && typeof entry.id === 'string' && entry.id) + : null + const clarify: GroupPendingClarify | null = openClarify + ? { ...(openClarify.params as GroupPendingClarify), request_id: openClarify.id } + : null // The `!requestId` bail below is what makes the approval branch reachable, // so an approval read there is never the null arm of this ternary — a fact diff --git a/apps/desktop/src/store/gateway-profile-only-owner.integration.test.ts b/apps/desktop/src/store/gateway-profile-only-owner.integration.test.ts index ecb1eab337..0a2e4f7b64 100644 --- a/apps/desktop/src/store/gateway-profile-only-owner.integration.test.ts +++ b/apps/desktop/src/store/gateway-profile-only-owner.integration.test.ts @@ -129,7 +129,7 @@ afterEach(() => { }) describe('profile-only secondary approval ownership', () => { - it('keeps a profile-only secondary event owner across a focus switch and dispatches approval on that secondary', async () => { + it('keeps a profile-only secondary event owner across a focus switch and dispatches the approval.respond fallback on that secondary', async () => { await expect(ensureGatewayForProfile('research')).resolves.toBeUndefined() const secondary = gatewayMocks.instances[0] @@ -138,7 +138,10 @@ describe('profile-only secondary approval ownership', () => { expect($sessionTiles.get()).toEqual([]) expect($sessionStates.get()).toEqual({}) - secondary.emit({ session_id: 'rt-secondary', type: 'approval.request' }) + // Prompts now arrive as server→client request frames, not events; owner + // scope is still learned from any session-stamped event the secondary + // socket emits (here the turn's status update). + secondary.emit({ session_id: 'rt-secondary', type: 'status.update' }) await expect(ensureGatewayForProfile('default')).resolves.toBeUndefined() expect(knownOwnerForSession('rt-secondary')).toBe('research') @@ -170,8 +173,8 @@ describe('profile-only secondary approval ownership', () => { await ensureGatewayForProfile('research') const secondary = gatewayMocks.instances[0] - secondary.emit({ session_id: 'rt-retired', type: 'approval.request' }) - secondary.emit({ session_id: 'rt-durable', type: 'approval.request' }) + secondary.emit({ session_id: 'rt-retired', type: 'status.update' }) + secondary.emit({ session_id: 'rt-durable', type: 'status.update' }) expect(knownOwnerForSession('rt-retired')).toBe('research') const exact = { connectionId: 'remote-same-name', profile: 'research' } setSessionOwnerHint('rt-durable', exact) diff --git a/apps/desktop/src/store/session-states-runtime-map.test.ts b/apps/desktop/src/store/session-states-runtime-map.test.ts index 8d8f5df9ed..c3429a5f29 100644 --- a/apps/desktop/src/store/session-states-runtime-map.test.ts +++ b/apps/desktop/src/store/session-states-runtime-map.test.ts @@ -64,7 +64,7 @@ describe('storedSessionIdForRuntimeId', () => { }) it('maps a MAIN-PANE runtime id through the per-runtime state mirror (no tile involved)', () => { - // approval.respond from a native notification, a queued send: the caller + // The approval.respond RPC fallback from a native notification, a queued send: the caller // holds the runtime id of the primary thread, which no tile knows. The // state mirror carries the stored id the wiring cache bound. publishSessionState('rt-main', createClientSessionState('stored-main'))