diff --git a/.github/workflows/e2e-desktop.yml b/.github/workflows/e2e-desktop.yml index c07a94be8a3..364f23b73d0 100644 --- a/.github/workflows/e2e-desktop.yml +++ b/.github/workflows/e2e-desktop.yml @@ -58,7 +58,8 @@ jobs: command: uv sync --locked --python 3.11 --extra all --extra dev # ── Build desktop app ───────────────────────────────────────────── - - run: npm run --prefix apps/desktop build + # The Playwright step below runs `npm run build` before testing so + # dist/ is always fresh — no separate build step needed here. # ── Restore visual baseline screenshots from main ────────────────── # Baselines are generated on main (via --update-snapshots) and cached. @@ -79,16 +80,18 @@ jobs: # xvfb runs at a fixed 1280x1024 screen so the 1220x800 Electron # window always has a consistent viewport for screenshot comparison. # On main, we run with --update-snapshots to generate baselines. + # `npm run test:e2e` builds dist/ as a pretest hook so the renderer + # is always fresh — no separate build step needed. - name: Run Playwright E2E tests working-directory: apps/desktop run: | if [ "${{ github.ref_name }}" = "main" ]; then echo "On main — generating/updating baseline screenshots" - xvfb-run -a --server-args="-screen 0 1280x1024x24" \ + npm run build && xvfb-run -a --server-args="-screen 0 1280x1024x24" \ npx playwright test --reporter=list --update-snapshots else echo "On PR — comparing against cached baselines" - xvfb-run -a --server-args="-screen 0 1280x1024x24" \ + npm run build && xvfb-run -a --server-args="-screen 0 1280x1024x24" \ npx playwright test --reporter=list fi env: diff --git a/apps/desktop/e2e/fixtures.ts b/apps/desktop/e2e/fixtures.ts index 93acc6f8623..3e9c30da83b 100644 --- a/apps/desktop/e2e/fixtures.ts +++ b/apps/desktop/e2e/fixtures.ts @@ -140,10 +140,18 @@ export function createSandbox(prefix: string): Sandbox { * Write a config.yaml that pre-configures a mock provider pointing at the * mock inference server. The provider is set as the active model provider so * the desktop app skips onboarding and boots straight to the chat UI. + * + * @param extraConfig optional YAML lines appended to the `display:` section, + * used by the interim-message e2e test to toggle + * `display.interim_assistant_messages`. */ -export function writeMockProviderConfig(hermesHome: string, mockUrl: string): void { +export function writeMockProviderConfig(hermesHome: string, mockUrl: string, extraConfig?: string): void { const configPath = path.join(hermesHome, 'config.yaml') + const displaySection = extraConfig + ? `\ndisplay:\n${extraConfig}\n` + : '' + const config = `# Auto-generated by E2E test fixtures model: default: mock-model @@ -157,7 +165,7 @@ providers: models: mock-model: {} context_length: 4096 -` +${displaySection}` fs.writeFileSync(configPath, config, 'utf8') } @@ -323,6 +331,15 @@ export interface MockBackendFixture { cleanup: () => Promise } +export interface MockBackendOptions { + /** + * Optional YAML lines to inject under the `display:` section of the + * generated config.yaml. Used by the interim-message e2e test to toggle + * `display.interim_assistant_messages`. + */ + extraDisplayConfig?: string +} + /** * Set up a full mock-backend E2E environment: * 1. Start the mock inference server @@ -330,13 +347,13 @@ export interface MockBackendFixture { * 3. Launch the desktop app * 4. Return handles for test interaction */ -export async function setupMockBackend(): Promise { +export async function setupMockBackend(options: MockBackendOptions = {}): Promise { // 1. Start mock server const mock = await startMockServer() // 2. Create sandbox + write config const sandbox = createSandbox('mock') - writeMockProviderConfig(sandbox.hermesHome, mock.url) + writeMockProviderConfig(sandbox.hermesHome, mock.url, options.extraDisplayConfig) writeEnvFile(sandbox.hermesHome) // 3. Build env + launch diff --git a/apps/desktop/e2e/interim-messages.spec.ts b/apps/desktop/e2e/interim-messages.spec.ts new file mode 100644 index 00000000000..2f6da013912 --- /dev/null +++ b/apps/desktop/e2e/interim-messages.spec.ts @@ -0,0 +1,215 @@ +/** + * E2E test for the interim-assistant-message preservation fix (#65919). + * + * Reproduces the bug across all three layers (agent core → tui_gateway → + * desktop renderer): when the agent emits assistant text alongside a tool + * call, then completes the turn with a *different* final answer, the + * interim text must survive in the transcript — not be wiped when + * message.complete replaces the streaming bubble. + * + * The mock server walks through a multi-turn script when it sees the + * trigger keyword: + * + * Turn 1: "Let me start by planning the approach." + todo tool_call + * Turn 2: "Now checking the details before answering." + todo tool_call + * Turn 3: (no text) + todo tool_call → NO interim (no visible text) + * Turn 4: "Found something interesting worth noting." + todo tool_call + * Turn 5: "All done! Here is the complete summary..." (final, stop) + * + * Two describe blocks exercise the config flag both ways: + * + * display.interim_assistant_messages: true (default) + * → ALL interim texts AND the final text must be visible in the + * transcript. + * + * display.interim_assistant_messages: false + * → only the final text is visible (no message.interim events emitted, + * so all streamed interim text is replaced at message.complete). + * + * Prerequisite: `npm run build` must have been run so dist/ exists. + */ + +import { expect, test, type Page } from '@playwright/test' + +import { + type MockBackendFixture, + setupMockBackend, + waitForAppReady, +} from './fixtures' +import { INTERIM_TEXTS, restartMockServer } from './mock-server' + +// ─── Helpers ────────────────────────────────────────────────────────── + +/** Unique trigger keyword the mock server detects to switch to the script. */ +const TRIGGER = 'E2E_INTERIM_TRIGGER' + +/** + * Send a message and wait for BOTH the user's message and the agent's + * final response to appear in the transcript. Returns when the final text + * is visible, which means message.complete has fired and the transcript + * has settled. + */ +async function sendInterimMessage(page: Page): Promise { + const composer = page.locator('[contenteditable="true"]').first() + await composer.waitFor({ state: 'visible', timeout: 10_000 }) + await composer.click() + await composer.type(TRIGGER, { delay: 20 }) + await page.keyboard.press('Enter') + + // Wait for the user's trigger message to appear. + await page.waitForFunction( + () => (document.body.textContent ?? '').includes('E2E_INTERIM_TRIGGER'), + undefined, + { timeout: 15_000 }, + ) + + // Wait for the agent's FINAL response (last turn). This means + // message.complete has fired and the transcript is settled. + await page.waitForFunction( + (finalText) => (document.body.textContent ?? '').includes(finalText), + INTERIM_TEXTS.finalText, + { timeout: 90_000 }, + ) + + // Give the renderer a moment to settle any final state updates + // (hydration, session refresh) before asserting. + await page.waitForTimeout(2000) +} + +/** + * Count how many times `text` appears as distinct text in the chat transcript + * (excluding the session sidebar, whose session-preview label shows the + * first streamed text as a title). + * + * The desktop app renders the transcript inside a + * `[data-slot="aui_thread-viewport"]` container (from @assistant-ui/react). + * The session sidebar's preview labels live outside that container, so + * scoping the DOM walk to the viewport cleanly excludes them. + */ +async function countTranscriptMessagesContaining(page: Page, text: string): Promise { + return page.evaluate( + (search) => { + const viewport = document.querySelector('[data-slot="aui_thread-viewport"]') + if (!viewport) { + return 0 + } + + let count = 0 + const walker = document.createTreeWalker( + viewport, + NodeFilter.SHOW_ELEMENT, + { + acceptNode: (node) => { + const el = node as HTMLElement + const directText = el.textContent ?? '' + if (!directText.includes(search)) { + return NodeFilter.FILTER_SKIP + } + // Only count leaf-ish elements to avoid double-counting. + const hasChildWithText = Array.from(el.children).some( + (child) => (child.textContent ?? '').includes(search), + ) + if (hasChildWithText) { + return NodeFilter.FILTER_SKIP + } + return NodeFilter.FILTER_ACCEPT + }, + }, + ) + while (walker.nextNode()) { + count++ + } + return count + }, + text, + ) +} + +// ─── Flag ON: interim_assistant_messages = true (default) ───────────── + +test.describe('interim assistant messages — flag ON (default)', () => { + test.describe.configure({ mode: 'serial' }) + + let fixture: MockBackendFixture + + test.beforeAll(async () => { + restartMockServer() + fixture = await setupMockBackend() + await waitForAppReady(fixture, 120_000) + }) + + test.afterAll(async () => { + await fixture?.cleanup() + }) + + test('all interim texts survive alongside the final response', async () => { + const page = fixture.page + await sendInterimMessage(page) + + // Every interim text (turns with visible text + tool calls) must be + // present in the transcript as its own sealed message — NOT wiped by + // message.complete. + for (const interimText of INTERIM_TEXTS.interims) { + await expect + .poll( + () => countTranscriptMessagesContaining(page, interimText), + { timeout: 15_000, message: `interim text "${interimText}" should be visible` }, + ) + .toBeGreaterThanOrEqual(1) + } + + // The final text must also be visible. + await expect + .poll( + () => countTranscriptMessagesContaining(page, INTERIM_TEXTS.finalText), + { timeout: 15_000, message: 'final text should be visible' }, + ) + .toBeGreaterThanOrEqual(1) + }) +}) + +// ─── Flag OFF: interim_assistant_messages = false ──────────────────── + +test.describe('interim assistant messages — flag OFF', () => { + test.describe.configure({ mode: 'serial' }) + + let fixture: MockBackendFixture + + test.beforeAll(async () => { + restartMockServer() + fixture = await setupMockBackend({ + extraDisplayConfig: ' interim_assistant_messages: false', + }) + await waitForAppReady(fixture, 120_000) + }) + + test.afterAll(async () => { + await fixture?.cleanup() + }) + + test('only the final response is visible; all interim texts are wiped', async () => { + const page = fixture.page + await sendInterimMessage(page) + + // The final text must be visible. + await expect + .poll( + () => countTranscriptMessagesContaining(page, INTERIM_TEXTS.finalText), + { timeout: 15_000, message: 'final text should be visible' }, + ) + .toBeGreaterThanOrEqual(1) + + // NONE of the interim texts should be visible — with the flag off, + // the tui_gateway never installs interim_assistant_callback, so no + // message.interim events are emitted. All streamed interim text is + // accumulated into the streaming bubble and replaced by + // message.complete. + for (const interimText of INTERIM_TEXTS.interims) { + const count = await countTranscriptMessagesContaining(page, interimText) + expect( + count, + `interim text "${interimText}" should NOT be visible when flag is off`, + ).toBe(0) + } + }) +}) diff --git a/apps/desktop/e2e/mock-server.ts b/apps/desktop/e2e/mock-server.ts index 680cba30e7a..08f48af6f4b 100644 --- a/apps/desktop/e2e/mock-server.ts +++ b/apps/desktop/e2e/mock-server.ts @@ -15,10 +15,70 @@ */ import http from 'node:http' +import type { ServerResponse } from 'node:http' /** A canned assistant reply used for every chat completion request. */ const CANNED_REPLY = 'Hello from the mock inference server! The full boot chain is working.' +// ─── Multi-turn interim script ───────────────────────────────────────── +// +// When the user's message contains the trigger keyword, the mock server +// walks through a scripted sequence of responses that exercise the +// interim-assistant-message fix (#65919) across several patterns: +// +// 1. text + single tool_call → should produce an interim message +// 2. text + single tool_call → another interim message +// 3. no text + tool_call → NO interim (no visible text alongside tools) +// 4. text + single tool_call → another interim message +// 5. final answer (stop) → message.complete, different from all interims +// +// Each "turn" is one API call. The agent executes the tool after each +// tool_calls response, then re-calls the API, advancing to the next turn. + +export interface ScriptedTurn { + /** Assistant text content to stream. Empty string = no visible text. */ + text: string + /** Tool calls to emit. Empty array = final turn (finish_reason: stop). */ + toolCalls?: Array<{ + name: string + args: Record + }> +} + +const INTERIM_SCRIPT: ScriptedTurn[] = [ + { + text: 'Let me start by planning the approach.', + toolCalls: [{ name: 'todo', args: { todos: [{ id: '1', content: 'Plan', status: 'in_progress' }] } }], + }, + { + text: 'Now checking the details before answering.', + toolCalls: [{ name: 'todo', args: { todos: [{ id: '2', content: 'Check details', status: 'in_progress' }] } }], + }, + { + // No visible text alongside this tool call — should NOT produce an + // interim message. The agent fires _emit_interim_assistant_message + // but _interim_assistant_visible_text returns "" so it's a no-op. + text: '', + toolCalls: [{ name: 'todo', args: { todos: [{ id: '3', content: 'Silent step', status: 'completed' }] } }], + }, + { + text: 'Found something interesting worth noting.', + toolCalls: [{ name: 'todo', args: { todos: [{ id: '4', content: 'Note finding', status: 'completed' }] } }], + }, + { + // Final answer — different from all interim texts. + text: 'All done! Here is the complete summary of what I found.', + }, +] + +/** Per-server request counter so we can walk through the script turns. */ +let _scriptIndex = 0 + +/** Reset the script index (called between tests via restartMockServer). */ +function resetScriptIndex(): void { + _scriptIndex = 0 +} + /** * Start the mock server on an ephemeral port. * @@ -78,85 +138,33 @@ export function startMockServer(): Promise<{ port: number; url: string; close: ( const stream = parsed.stream === true const model = parsed.model || 'mock-model' - if (stream) { - res.writeHead(200, { - 'Content-Type': 'text/event-stream', - 'Cache-Control': 'no-cache', - Connection: 'keep-alive', - }) + // Detect the interim-message test trigger: the user's message + // contains a specific keyword. The mock walks through the + // INTERIM_SCRIPT turns in sequence. + // + // The trigger keyword is chosen so normal chat tests (which send + // "Hello, can you hear me?" etc.) never hit this path. + const messages: any[] = Array.isArray(parsed.messages) ? parsed.messages : [] + const lastUserMsg = [...messages].reverse().find(m => m?.role === 'user') + const userText = typeof lastUserMsg?.content === 'string' ? lastUserMsg.content : '' + const isInterimTrigger = userText.includes('E2E_INTERIM_TRIGGER') - // Send the content in a few chunks to simulate streaming. - const words = CANNED_REPLY.split(' ') - let i = 0 + if (isInterimTrigger) { + const turn = INTERIM_SCRIPT[_scriptIndex] ?? INTERIM_SCRIPT[INTERIM_SCRIPT.length - 1] + _scriptIndex++ - const sendChunk = () => { - if (i >= words.length) { - // Final chunk with finish_reason - res.write( - `data: ${JSON.stringify({ - id: 'mock-completion', - object: 'chat.completion.chunk', - created: 0, - model, - choices: [ - { - index: 0, - delta: {}, - finish_reason: 'stop', - }, - ], - })}\n\n`, - ) - res.write('data: [DONE]\n\n') - res.end() - return - } - - const word = i === 0 ? words[i] : ' ' + words[i] - res.write( - `data: ${JSON.stringify({ - id: 'mock-completion', - object: 'chat.completion.chunk', - created: 0, - model, - choices: [ - { - index: 0, - delta: { content: word }, - finish_reason: null, - }, - ], - })}\n\n`, - ) - i++ - // Small delay between chunks to simulate real streaming. - setTimeout(sendChunk, 20) + if (stream) { + streamScriptedTurn(res, model, turn) + } else { + nonStreamingScriptedTurn(res, model, turn) } + return + } - sendChunk() + if (stream) { + streamTextResponse(res, model, CANNED_REPLY) } else { - // Non-streaming response - res.writeHead(200, { 'Content-Type': 'application/json' }) - res.end( - JSON.stringify({ - id: 'mock-completion', - object: 'chat.completion', - created: 0, - model, - choices: [ - { - index: 0, - message: { role: 'assistant', content: CANNED_REPLY }, - finish_reason: 'stop', - }, - ], - usage: { - prompt_tokens: 10, - completion_tokens: 20, - total_tokens: 30, - }, - }), - ) + nonStreamingTextResponse(res, model, CANNED_REPLY) } }) @@ -201,3 +209,205 @@ export function startMockServer(): Promise<{ port: number; url: string; close: ( }) }) } + +// ─── Response helpers ────────────────────────────────────────────────── + +/** SSE chunk shape for a streaming chat completion. */ +function sseChunk(model: string, delta: Record, finishReason: string | null = null): string { + return `data: ${JSON.stringify({ + id: 'mock-completion', + object: 'chat.completion.chunk', + created: 0, + model, + choices: [{ index: 0, delta, finish_reason: finishReason }], + })}\n\n` +} + +/** + * Stream a plain text response (no tool calls) as SSE, finishing with + * `finish_reason: "stop"`. This is the default canned-reply path. + */ +function streamTextResponse(res: ServerResponse, model: string, text: string): void { + res.writeHead(200, { + 'Content-Type': 'text/event-stream', + 'Cache-Control': 'no-cache', + Connection: 'keep-alive', + }) + + const words = text.split(' ') + let i = 0 + + const sendChunk = (): void => { + if (i >= words.length) { + res.write(sseChunk(model, {}, 'stop')) + res.write('data: [DONE]\n\n') + res.end() + return + } + + const word = i === 0 ? words[i] : ' ' + words[i] + res.write(sseChunk(model, { content: word })) + i++ + setTimeout(sendChunk, 20) + } + + sendChunk() +} + +/** Non-streaming plain text response. */ +function nonStreamingTextResponse(res: ServerResponse, model: string, text: string): void { + res.writeHead(200, { 'Content-Type': 'application/json' }) + res.end( + JSON.stringify({ + id: 'mock-completion', + object: 'chat.completion', + created: 0, + model, + choices: [ + { + index: 0, + message: { role: 'assistant', content: text }, + finish_reason: 'stop', + }, + ], + usage: { prompt_tokens: 10, completion_tokens: 20, total_tokens: 30 }, + }), + ) +} + +/** + * Stream a single scripted turn: first the text content (word by word), + * then a chunk carrying the tool_calls (if any), with the appropriate + * finish_reason. + * + * If the turn has no text and no tool calls, it's an empty final response. + * If it has text but no tool calls, it's a final answer (finish_reason: stop). + * If it has tool calls (with or without text), finish_reason is "tool_calls". + */ +function streamScriptedTurn( + res: ServerResponse, + model: string, + turn: ScriptedTurn, +): void { + res.writeHead(200, { + 'Content-Type': 'text/event-stream', + 'Cache-Control': 'no-cache', + Connection: 'keep-alive', + }) + + const hasToolCalls = turn.toolCalls && turn.toolCalls.length > 0 + const finishReason = hasToolCalls ? 'tool_calls' : 'stop' + + // If there's no text to stream, go straight to the tool_calls / finish. + if (!turn.text) { + if (hasToolCalls) { + res.write( + sseChunk(model, { + tool_calls: turn.toolCalls!.map((tc, idx) => ({ + index: idx, + id: `call_e2e_${_scriptIndex}_${idx}`, + type: 'function', + function: { name: tc.name, arguments: JSON.stringify(tc.args) }, + })), + }, finishReason), + ) + } else { + res.write(sseChunk(model, {}, finishReason)) + } + res.write('data: [DONE]\n\n') + res.end() + return + } + + // Stream the text word by word, then emit tool_calls if present. + const words = turn.text.split(' ') + let i = 0 + + const sendChunk = (): void => { + if (i >= words.length) { + // All text streamed — emit tool_calls if present, then finish. + if (hasToolCalls) { + res.write( + sseChunk(model, { + tool_calls: turn.toolCalls!.map((tc, idx) => ({ + index: idx, + id: `call_e2e_${_scriptIndex}_${idx}`, + type: 'function', + function: { name: tc.name, arguments: JSON.stringify(tc.args) }, + })), + }, finishReason), + ) + } else { + res.write(sseChunk(model, {}, finishReason)) + } + res.write('data: [DONE]\n\n') + res.end() + return + } + + const word = i === 0 ? words[i] : ' ' + words[i] + res.write(sseChunk(model, { content: word })) + i++ + setTimeout(sendChunk, 20) + } + + sendChunk() +} + +/** Non-streaming version of a scripted turn. */ +function nonStreamingScriptedTurn( + res: ServerResponse, + model: string, + turn: ScriptedTurn, +): void { + const hasToolCalls = turn.toolCalls && turn.toolCalls.length > 0 + const finishReason = hasToolCalls ? 'tool_calls' : 'stop' + + const message: Record = { role: 'assistant' } + if (turn.text) { + message.content = turn.text + } + if (hasToolCalls) { + message.tool_calls = turn.toolCalls!.map((tc, idx) => ({ + id: `call_e2e_${_scriptIndex}_${idx}`, + type: 'function', + function: { name: tc.name, arguments: JSON.stringify(tc.args) }, + })) + } + + res.writeHead(200, { 'Content-Type': 'application/json' }) + res.end( + JSON.stringify({ + id: 'mock-completion', + object: 'chat.completion', + created: 0, + model, + choices: [{ index: 0, message, finish_reason: finishReason }], + usage: { prompt_tokens: 10, completion_tokens: 20, total_tokens: 30 }, + }), + ) +} + +/** + * Restart the mock server's script index so each test starts from turn 0. + * Call this between tests that use the interim trigger. + */ +export function restartMockServer(): void { + resetScriptIndex() +} + +/** + * The interim script's text constants, exported for test assertions. + * Each entry is the visible text of one turn. Turns with empty text + * produce no interim message and are excluded from this list. + */ +export const INTERIM_TEXTS = { + /** All interim texts that should appear as sealed messages when the flag is ON. */ + interims: INTERIM_SCRIPT + .filter((t) => t.text && t.toolCalls) + .map((t) => t.text), + /** The final answer text. */ + finalText: INTERIM_SCRIPT[INTERIM_SCRIPT.length - 1].text, + /** Text that should NOT produce an interim (empty-text tool turn). */ + silentTurnIndex: INTERIM_SCRIPT.findIndex((t) => !t.text && t.toolCalls), +} as const diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 743269c1047..b1af8075822 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -51,9 +51,9 @@ "test": "vitest run", "preview": "node scripts/assert-root-install.mjs && vite preview --host 127.0.0.1 --port 4174", "check": "npm run typecheck && npm run test && npm run test:desktop:all", - "test:e2e": "playwright test e2e/", - "test:e2e:visual": "WLR_BACKENDS=headless WLR_NO_HARDWARE_CURSORS=1 cage -- npx playwright test e2e/ --reporter=list", - "test:e2e:update-snapshots": "WLR_BACKENDS=headless WLR_NO_HARDWARE_CURSORS=1 cage -- npx playwright test e2e/ --reporter=list --update-snapshots" + "test:e2e": "npm run build && playwright test e2e/", + "test:e2e:visual": "npm run build && WLR_BACKENDS=headless WLR_NO_HARDWARE_CURSORS=1 cage -- npx playwright test e2e/ --reporter=list", + "test:e2e:update-snapshots": "npm run build && WLR_BACKENDS=headless WLR_NO_HARDWARE_CURSORS=1 cage -- npx playwright test e2e/ --reporter=list --update-snapshots" }, "dependencies": { "@assistant-ui/react": "^0.14.23",