From 7fdb09af0a7294002ca1d35987c112c04d153183 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sun, 16 Aug 2026 11:54:09 +0000 Subject: [PATCH] speaking mid-turn no longer declares the turn over MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported as "the output I see with turn complete is very very misleading", and it was: the transcript printed "Turn finished · no closing message" between a tool call and an injected message, while the header two inches away still said Working and a task chip still said 1 RUNNING. The UI stated the opposite of what was happening. `turn-rows` treats every user message as a turn boundary. That was right when the only way to speak was to wait. Delivering a queued message INTO a running turn made it wrong: the boundary split the live turn, the half before it therefore counted as `complete`, and having tool calls and no answer it matched exactly the rule that prints a silent ending. The seam meant for a rare confusing case fired on an ordinary one, and said something false. So a user message now carries `injected` when it was sent while a turn was already running, and only a message that STARTS a turn is a boundary. Read from `isGeneratingRef` at send time rather than from state — `sendPrompt` is called from closures built a render earlier, and the question is "was a turn running when this was sent", which is what the ref holds. Five tests, including the contrast case: the same messages WITHOUT the flag still produce the boundary and the ending, because a genuine second turn should end the first. A turn that really did finish silently still says so — that seam was worth having and is not what was broken. Verified live on the way in: the injection reached this session mid-turn, attached to a tool result, and changed what the turn did. Still outstanding from the same screenshot, not touched here: foreground Bash calls render as background tasks ("Running in the background…", and a chip in the tray). That is a separate misreport. tsgo clean, frontend builds, 878 pass / 7 fail unchanged. Co-Authored-By: Claude Opus 5 --- .../apps/Chat/components/turn-rows.test.ts | 52 +++++++++++++++++++ .../src/apps/Chat/components/turn-rows.ts | 9 +++- .../officerdev/src/apps/Chat/types.ts | 14 ++++- .../officerdev/src/hooks/useChat.ts | 12 ++++- 4 files changed, 84 insertions(+), 3 deletions(-) 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 = '';