diff --git a/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.test.ts b/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.test.ts index b5d6b1df..4f329dbe 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.test.ts +++ b/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.test.ts @@ -3,6 +3,8 @@ import type { ChatMessage } from '../types'; import { buildRows, foldLabel } from './turn-rows'; const user = (text: string): ChatMessage => ({ role: 'user', text }); +/** Sent into a turn that was already running — see `injected` in types.ts. */ +const injected = (text: string): ChatMessage => ({ role: 'user', text, injected: true }); const say = (text: string): ChatMessage => ({ role: 'assistant', id: text, text }); const tool = (id: string, isError?: boolean): ChatMessage => ({ role: 'tool', @@ -23,6 +25,56 @@ const shape = (messages: ChatMessage[], isGenerating = false, historicalCount = buildRows(messages, isGenerating, historicalCount).map((r) => (r.kind === 'message' ? r.message.role : r.kind)); describe('buildRows', () => { + // ── Messages sent INTO a running turn ── + // + // The bug these pin: every user message was a turn boundary, so speaking mid-turn split the live turn + // and the first half — tool calls, no answer yet — satisfied "finished without saying anything". The + // reader got "Turn finished · no closing message" stamped across work that was still running, while the + // header beside it said Working. + + test('an injected message does not end the turn it was sent into', () => { + expect(shape([user('go'), tool('a'), injected('also do X'), tool('b')], true, 0)).toEqual([ + 'user', + 'tool', + 'user', + 'tool', + ]); + }); + + test('the same messages WITHOUT the flag are what used to produce the false ending', () => { + // Kept as the contrast: this is a genuine second turn, and ending the first one is correct there. + expect(shape([user('go'), tool('a'), user('new question'), tool('b')], true, 0)).toEqual([ + 'user', + 'tool', + 'turn-end', + 'user', + 'tool', + ]); + }); + + test('a turn that really did end silently still says so', () => { + expect(shape([user('go'), tool('a')], false, 0)).toEqual(['user', 'tool', 'turn-end']); + }); + + test('an injected message still counts as part of the turn when it folds', () => { + const messages = [user('go'), tool('a'), injected('also do X'), tool('b'), say('done')]; + const rows = buildRows(messages, false, messages.length); + // One turn, so one fold — not two. + expect(rows.filter((r) => r.kind === 'fold')).toHaveLength(1); + expect(rows.some((r) => r.kind === 'turn-end')).toBe(false); + }); + + test('several injections in one turn stay in one turn', () => { + expect(shape([user('go'), tool('a'), injected('x'), tool('b'), injected('y'), say('done')], false, 0)).toEqual([ + 'user', + 'tool', + 'user', + 'tool', + 'user', + 'assistant', + ]); + }); + test('the live turn is untouched — every row stays where it was', () => { expect(shape([user('go'), say('one'), tool('a'), say('two')], true)).toEqual([ 'user', diff --git a/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.ts b/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.ts index efff6af0..2e0448ad 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.ts +++ b/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.ts @@ -159,9 +159,16 @@ function buildTurn({ segment, offset, fold, complete, live }: TurnParams): ChatR export function buildRows(messages: ChatMessage[], isGenerating: boolean, historicalCount = 0): ChatRow[] { if (!messages.length) return []; + // A user message starts a turn — UNLESS it was sent into one that was already running. + // + // Queued messages can be delivered mid-turn: they reach the agent at its next step boundary and it + // carries on with them. Treating those as boundaries split the live turn in two, which made the first + // half `complete` — tool calls, no answer — and printed "Turn finished · no closing message" across a + // turn that was visibly still working, with the header still saying so. Reported as "the output is very + // misleading", and it was: the UI said the opposite of what was happening. const userAt: number[] = []; messages.forEach((m, i) => { - if (m.role === 'user') userAt.push(i); + if (m.role === 'user' && !m.injected) userAt.push(i); }); // Every user message is a turn boundary — including the last one, which used to be excluded because the diff --git a/src/workspaces/officerdev/src/apps/Chat/types.ts b/src/workspaces/officerdev/src/apps/Chat/types.ts index 30788e18..a87bb33a 100644 --- a/src/workspaces/officerdev/src/apps/Chat/types.ts +++ b/src/workspaces/officerdev/src/apps/Chat/types.ts @@ -19,7 +19,19 @@ export type ModelOption = { }; export type ChatMessage = - | { role: 'user'; text: string; images?: { filename: string; dataUrl: string }[] } + | { + role: 'user'; + text: string; + images?: { filename: string; dataUrl: string }[]; + /** + * Sent INTO a turn that was already running, rather than starting one. + * + * The distinction is invisible in the text and load-bearing in the layout: `turn-rows` treats every + * user message as a turn boundary, so without this an injected message ends the turn it was sent + * into — printing "Turn finished · no closing message" under work that was still going. + */ + injected?: true; + } | { role: 'assistant'; id?: string; text: string } | { role: 'system'; text: string } | { diff --git a/src/workspaces/officerdev/src/hooks/useChat.ts b/src/workspaces/officerdev/src/hooks/useChat.ts index 2a88ccdd..84604a72 100644 --- a/src/workspaces/officerdev/src/hooks/useChat.ts +++ b/src/workspaces/officerdev/src/hooks/useChat.ts @@ -576,9 +576,19 @@ export function useChat(initialSessionId?: string, initialModel?: string | null, setHasStarted(true); } + // Read BEFORE `setIsGenerating(true)` below, and from the ref rather than the state: this function is + // called from a closure that may have been built a render ago, and the answer has to be "was a turn + // running when this was sent", which is exactly what the ref holds. + const intoRunningTurn = isGeneratingRef.current; + setMessages((prev) => [ ...prev, - { role: 'user', text: displayText ?? text, ...(images?.length ? { images } : {}) }, + { + role: 'user', + text: displayText ?? text, + ...(images?.length ? { images } : {}), + ...(intoRunningTurn ? { injected: true as const } : {}), + }, ]); setIsGenerating(true); streamingRef.current = '';