From eeb2f702aceb4c8413eaa8ba1a584c5f3404d0d1 Mon Sep 17 00:00:00 2001 From: ss-dev-02 <9e916f802f7932c38e630ac6b4726f5db7ca326c0d98e9bab495309eef13fe5a@buzz.block.builderlab.xyz> Date: Mon, 24 Aug 2026 21:50:44 -0700 Subject: [PATCH] fix(desktop): guard the work-block pulse and stop a finished block going live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reader-visible bugs in the focus-mode work block, both found reviewing this PR rather than by a test. **The running step's pulse ignored reduced motion.** `WorkBlockRailGlyph` applied bare `animate-pulse`. `useWorkBlockMotionEnabled` skips the fold's height animation, but it cannot reach a keyframe animation applied by a utility class, and none of the app's 20 `prefers-reduced-motion: reduce` blocks matches `.animate-pulse` — every one is scoped to a `buzz-*`/`motion-*`/`t-skel-*` class. So a reader who asked for no motion got an indefinite pulse. Now `motion-safe:animate-pulse`, which this repo's Tailwind (4.3.0) compiles to the same declaration wrapped in `@media (prefers-reduced-motion: no-preference)` — and which matches every other pulse in this feature (`AgentStatusBadge`, `ManagedAgentRow`). That swap also repaired three assertions it would otherwise have disarmed. `motion-safe:animate-pulse` is a different class TOKEN, not `animate-pulse` plus a modifier, so the suite's `.animate-pulse` selectors would have stopped matching anything and passed unconditionally. Worse, they were already vacuous: all three ran after `settleToStepCount(0)`, i.e. on a folded rail with no rows, so "nothing pulses" held against a build where abandoned steps pulse forever — the exact Codex finding this PR answers. They now assert on rows that exist (expanded, or mid-settle) and match the exact class token, so neither a substring nor a missing element can fake a pass. **A finished block went live again between turns.** `lastTurnId` walked the display blocks for the newest `turn` block, but a turn that has only emitted setup lifecycle rows (`turn_started`, `session_resolved`) classifies to zero segments and so produces no block at all. For the whole gap between `turn_started` and the next turn's first renderable item, "newest turn with a block" was therefore the turn that had already ENDED, and its own trailing item became the streaming item: a settled 6-step block re-opened, dropped to its last three steps behind a previous-steps disclosure, then folded back. `turn_started` fires on every turn and the gap is real observer-stream latency, so this was every turn, not an edge case. Liveness now comes from the newest turn id in the item stream, which is why `buildConversationTurnMeta` takes `items`. A trailing item is also only reported as streaming when the live turn owns it — the same ownership rule `toolEntryState` already applies to an `executing` status, for the same reason: without it the newest block's tail holds a finished turn's block open regardless of whose turn it is. Tests, each mutation-checked in isolation so none of them is vacuous: - pulse guarded, asserted under BOTH preference values (the class must not depend on it) — reverting to the bare class fails exactly that test; dropping the `running` guard fails the three retargeted negatives. - the meta gap contract over the real 4-frame sequence built from a raw item stream through the real grouping, since the bug is precisely which turns do and do not produce a block — reverting either half of the fix fails it. - the rendered consequence with a 6-step block (above the live window, where the symptom is loudest): folded summary and no rail across the gap. Also fixed a fixture that named the wrong thing: "no turn at all" carried the shared helper's default `turnId: "turn-1"`, so it was asserting that a turn with no block reports no live turn — the opposite of the rule. Full desktop suite 5491 passing / 0 failing, 81 suites; `tsc --noEmit` clean. Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- .../agents/ui/AgentSessionTranscriptList.tsx | 5 +- .../AgentSessionWorkBlock.orphaned.test.mjs | 88 ++++++- .../agents/ui/AgentSessionWorkBlock.test.mjs | 60 ++++- .../agents/ui/AgentSessionWorkBlock.tsx | 13 +- .../ui/AgentSessionWorkBlockTestRig.mjs | 25 ++ .../ui/agentSessionConversationMeta.test.mjs | 244 +++++++++++++++++- .../agents/ui/agentSessionConversationMeta.ts | 84 +++++- 7 files changed, 484 insertions(+), 35 deletions(-) diff --git a/desktop/src/features/agents/ui/AgentSessionTranscriptList.tsx b/desktop/src/features/agents/ui/AgentSessionTranscriptList.tsx index f32dfe9dba..720d762d86 100644 --- a/desktop/src/features/agents/ui/AgentSessionTranscriptList.tsx +++ b/desktop/src/features/agents/ui/AgentSessionTranscriptList.tsx @@ -176,8 +176,9 @@ export function AgentSessionTranscriptList({ // Returns a shared constant for the other variants, so their render output is // untouched. const turnMeta = React.useMemo( - () => buildConversationTurnMeta(displayBlocks, { isTurnLive, variant }), - [displayBlocks, isTurnLive, variant], + () => + buildConversationTurnMeta(displayBlocks, { isTurnLive, items, variant }), + [displayBlocks, isTurnLive, items, variant], ); const scrollContainerClassNames = cn( diff --git a/desktop/src/features/agents/ui/AgentSessionWorkBlock.orphaned.test.mjs b/desktop/src/features/agents/ui/AgentSessionWorkBlock.orphaned.test.mjs index 6998e6c899..17ce5d6339 100644 --- a/desktop/src/features/agents/ui/AgentSessionWorkBlock.orphaned.test.mjs +++ b/desktop/src/features/agents/ui/AgentSessionWorkBlock.orphaned.test.mjs @@ -45,9 +45,22 @@ test("a block whose running step has no live session folds like finished work", "an abandoned step is not known to have failed, so the count stays neutral", ); assert.equal(await view.settleToStepCount(0), 0, "the rail folds away"); - assert.equal( - view.qa(".animate-pulse").length, - 0, + // Then OPEN it and look. Asserting "nothing pulses" on the folded block would + // be vacuous: the fold unmounts every row, so no glyph exists to carry a + // pulse class and the assertion would hold against a build where abandoned + // steps pulse forever — the exact Codex finding. The reader's complaint is + // about what they see when they scroll back and expand, so that is where the + // assertion belongs. + await view.expand(); + assert.equal(view.stepCount(), 2, "the expanded rail has rows to inspect"); + assert.deepEqual( + view.glyphStates(), + ["settled", "settled"], + "the abandoned step reads as settled, not running", + ); + assert.deepEqual( + view.pulseStates(), + [], "nothing pulses when nothing is running", ); }); @@ -63,7 +76,7 @@ test("the same items still hold the block open while the turn is live", async () assert.equal(view.summary(), null, "a live block has no folded line"); assert.equal(view.stepCount(), 2, "the rail stays open"); assert.deepEqual(view.glyphStates(), ["settled", "running"]); - assert.equal(view.qa(".animate-pulse").length, 1, "live work pulses"); + assert.deepEqual(view.pulseStates(), ["running"], "live work pulses"); }); test("an agent live on a later turn does not resurrect an earlier turn's abandoned step", async () => { @@ -79,7 +92,11 @@ test("an agent live on a later turn does not resurrect an earlier turn's abandon "this turn is history even though the agent is busy", ); assert.equal(await view.settleToStepCount(0), 0); - assert.equal(view.qa(".animate-pulse").length, 0); + // Expanded, for the same reason as above: a folded rail has no glyph to pulse, + // so the negative has to be taken on rows that exist. + await view.expand(); + assert.deepEqual(view.glyphStates(), ["settled", "settled"]); + assert.deepEqual(view.pulseStates(), []); }); test("a block live when the session ends folds instead of spinning forever", async () => { @@ -105,8 +122,67 @@ test("a block live when the session ends folds instead of spinning forever", asy assert.ok(view.summary(), "the block settles when its session goes away"); assert.match(view.summary().textContent, /2 steps$/); + // Taken HERE, before the fold finishes: the rail is still mounted for the + // settle frame, so the glyph that was pulsing a moment ago still exists and + // can be asked whether it stopped. Once `settleToStepCount(0)` has run there + // are no rows left and the same assertion proves nothing. + assert.equal(view.stepCount(), 2, "the rail is still mounted to inspect"); + assert.deepEqual(view.glyphStates(), ["settled", "settled"]); + assert.deepEqual( + view.pulseStates(), + [], + "the pulse stops the moment the session goes away, not when the fold finishes", + ); assert.equal(await view.settleToStepCount(0), 0); - assert.equal(view.qa(".animate-pulse").length, 0); +}); + +// ── The gap between two turns ───────────────────────────────────────────────── + +/** + * A finished block must stay folded through the gap before the next turn shows + * anything. + * + * The rendered half of the `buildConversationTurnMeta` gap contract (see + * `agentSessionConversationMeta.test.mjs`). The meta test proves the hints are + * right; this proves the reader sees the consequence, because the symptom was + * never a wrong id — it was a settled 6-step block re-opening, dropping to its + * last three steps behind a "previous steps" disclosure, and then folding back, + * on every single turn. + * + * Six steps rather than two on purpose: the live window only applies above + * three, so a smaller block would hide the loudest part of the regression. + */ +test("a finished block stays folded while the next turn has started but shown nothing", async () => { + const { act } = await import("@testing-library/react"); + const items = ["a", "b", "c", "d", "e", "f"].map((id) => step(id)); + + // Finished: nothing live, so the block is folded to its summary line. + const view = await renderBlock(items, { + liveTurnId: null, + streamingItemId: null, + }); + assert.match(view.summary().textContent, /6 steps$/); + assert.equal(await view.settleToStepCount(0), 0, "it starts folded"); + + // The next turn starts. It owns liveness (turn-2) and, having emitted nothing + // renderable, contributes no streaming item — which is exactly what the fixed + // `latestTurnId`/`streamingIdForTail` pair reports for this frame. + await act(async () => { + view.stream(items, null, "turn-2"); + }); + + assert.ok( + view.summary(), + "the folded summary line survives the next turn starting", + ); + assert.match(view.summary().textContent, /6 steps$/); + assert.equal(view.stepCount(), 0, "the rail does not re-open"); + assert.equal( + view.previousSteps(), + null, + "and the live window's previous-steps disclosure never appears", + ); + assert.deepEqual(view.pulseStates(), []); }); test("an orphaned step keeps its own row detail rather than gaining an interrupted marker", async () => { diff --git a/desktop/src/features/agents/ui/AgentSessionWorkBlock.test.mjs b/desktop/src/features/agents/ui/AgentSessionWorkBlock.test.mjs index 11c5326ba1..1b4f185c62 100644 --- a/desktop/src/features/agents/ui/AgentSessionWorkBlock.test.mjs +++ b/desktop/src/features/agents/ui/AgentSessionWorkBlock.test.mjs @@ -341,12 +341,62 @@ test("a running step pulses and a failed one does not", async () => { { streamingItemId: "b" }, ); const [failed, running] = view.qa("[data-step-state]"); - assert.ok( - running.className.includes("animate-pulse"), - "work in flight pulses", + assert.deepEqual( + view.pulseStates(), + ["running"], + "work in flight pulses and a settled failure does not", ); - assert.ok( - !failed.className.includes("animate-pulse"), + assert.equal( + failed.classList.contains("motion-safe:animate-pulse"), + false, "a settled failure is not in flight", ); + assert.equal( + running.classList.contains("motion-safe:animate-pulse"), + true, + "the running step is the one that pulses", + ); +}); + +/** + * Reduced motion has to reach this pulse through the CLASS, not the hook. + * + * `useWorkBlockMotionEnabled` skips the fold's height animation, but the running + * glyph's pulse is a CSS keyframe animation applied by a Tailwind utility, which + * no hook can switch off — so the guard has to be `motion-safe:animate-pulse`. + * Compiled with this repo's Tailwind (4.3.0), `animate-pulse` is + * `animation: var(--animate-pulse)` with no guard, while + * `motion-safe:animate-pulse` wraps exactly that declaration in + * `@media (prefers-reduced-motion: no-preference)`. jsdom does not evaluate + * media queries, so the assertion is on the class: the bare form would keep + * pulsing forever for a reader who asked for no motion, and none of the app's 20 + * `prefers-reduced-motion: reduce` blocks matches `.animate-pulse` (all are + * scoped to `buzz-*`/`motion-*`/`t-skel-*` classes), so nothing else would catch + * it. + * + * The preference is irrelevant to which class is emitted, and that is the point: + * asserting under BOTH settings is what proves the guard is in the class rather + * than in a conditional that reduced motion could flip. + */ +test("a running glyph's pulse is guarded so reduced motion silences it", async () => { + for (const reduced of [false, true]) { + setPrefersReducedMotion(reduced); + const view = await renderBlock( + [step("a"), step("b", { status: "executing", completedAt: null })], + { streamingItemId: "b" }, + ); + const [, running] = view.qa("[data-step-state]"); + assert.equal( + running.classList.contains("motion-safe:animate-pulse"), + true, + `the pulse is reduced-motion-safe (prefersReducedMotion=${reduced})`, + ); + assert.deepEqual( + view.unguardedPulseStates(), + [], + `no glyph pulses unconditionally (prefersReducedMotion=${reduced})`, + ); + const { cleanup } = await import("@testing-library/react"); + cleanup(); + } }); diff --git a/desktop/src/features/agents/ui/AgentSessionWorkBlock.tsx b/desktop/src/features/agents/ui/AgentSessionWorkBlock.tsx index 27055c1994..99621ae7be 100644 --- a/desktop/src/features/agents/ui/AgentSessionWorkBlock.tsx +++ b/desktop/src/features/agents/ui/AgentSessionWorkBlock.tsx @@ -601,7 +601,18 @@ function WorkBlockRailGlyph({
diff --git a/desktop/src/features/agents/ui/AgentSessionWorkBlockTestRig.mjs b/desktop/src/features/agents/ui/AgentSessionWorkBlockTestRig.mjs index ea6618afc2..4514fec189 100644 --- a/desktop/src/features/agents/ui/AgentSessionWorkBlockTestRig.mjs +++ b/desktop/src/features/agents/ui/AgentSessionWorkBlockTestRig.mjs @@ -305,6 +305,31 @@ export async function renderBlock( qa("[data-step-state]").map((node) => node.getAttribute("data-step-state"), ), + // Which rail glyphs pulse, reported as their step states, and the same + // narrowed to pulses that are NOT reduced-motion-safe. + // + // Both read `classList.contains`, which matches a whole class token, rather + // than a `.animate-pulse` CSS selector or `className.includes` — + // `animate-pulse` is a SUBSTRING of `motion-safe:animate-pulse`, so a + // substring check cannot tell the guarded class from the bare one and would + // report the reduced-motion regression as fixed. + // + // They return step-state STRINGS rather than the elements: a failing + // `deepEqual` over jsdom nodes makes node:test serialize the whole DOM tree + // and the runner dies on SIGKILL instead of printing an assertion, which + // silently turns "the mutant was caught" into an unreadable crash. + pulseStates: () => + qa("[data-step-state]") + .filter( + (node) => + node.classList.contains("motion-safe:animate-pulse") || + node.classList.contains("animate-pulse"), + ) + .map((node) => node.getAttribute("data-step-state")), + unguardedPulseStates: () => + qa("[data-step-state]") + .filter((node) => node.classList.contains("animate-pulse")) + .map((node) => node.getAttribute("data-step-state")), q, qa, }; diff --git a/desktop/src/features/agents/ui/agentSessionConversationMeta.test.mjs b/desktop/src/features/agents/ui/agentSessionConversationMeta.test.mjs index 1e7d9964b2..7a59ed4d15 100644 --- a/desktop/src/features/agents/ui/agentSessionConversationMeta.test.mjs +++ b/desktop/src/features/agents/ui/agentSessionConversationMeta.test.mjs @@ -3,6 +3,7 @@ import test from "node:test"; import { buildConversationTurnMeta } from "./agentSessionConversationMeta.ts"; import { EMPTY_TRANSCRIPT_TURN_META } from "./agentSessionTranscriptContext.ts"; +import { buildTranscriptDisplayBlocks } from "./agentSessionTranscriptGrouping.ts"; import { formatPlanChecklistProgress, parsePlanChecklist, @@ -17,10 +18,114 @@ function item(overrides) { }; } +const RAW_TIMESTAMP = "2026-06-14T19:00:00.000Z"; + +/** + * Raw transcript items, for the tests that go through the real grouping rather + * than asserting on hand-written blocks. `acpSource` is what decides whether an + * item is setup lifecycle, a prompt, or work, so these carry it. + */ +function lifecycleItem(id, acpSource, turnId) { + return item({ + id, + type: "lifecycle", + renderClass: "status", + title: acpSource, + text: "", + timestamp: RAW_TIMESTAMP, + acpSource, + turnId, + }); +} + +function promptItem(id, turnId) { + return item({ + id, + type: "message", + role: "user", + renderClass: "message", + title: "Buzz event", + text: "please look into this", + timestamp: RAW_TIMESTAMP, + acpSource: "session/prompt:user", + turnId, + }); +} + +function toolItem(id, turnId) { + return item({ + id, + type: "tool", + renderClass: "shell", + title: id, + toolName: "shell", + buzzToolName: null, + status: "completed", + args: {}, + result: "ok", + isError: false, + timestamp: RAW_TIMESTAMP, + startedAt: RAW_TIMESTAMP, + completedAt: RAW_TIMESTAMP, + descriptor: { + renderClass: "shell", + label: "Ran command", + preview: id, + source: "shell", + groupKey: "shell:command", + }, + turnId, + }); +} + +function systemPromptItem(id) { + return item({ + id, + type: "metadata", + renderClass: "metadata", + title: "System prompt", + sections: [], + timestamp: RAW_TIMESTAMP, + acpSource: "session/new", + turnId: null, + }); +} + function turnBlock(segments) { return { kind: "turn", turnId: "turn-1", segments }; } +/** + * The leaf items a block is built from, in wire order — including setup + * lifecycle items, which carry a turn id in the real stream even though they + * contribute nothing to the streaming tail. + * + * `buildConversationTurnMeta` reads the item stream as well as the blocks (it + * has to: a turn that has only emitted setup rows produces no block at all), so + * a test that passed blocks alone would be describing a transcript that cannot + * exist. This keeps the two arguments consistent by construction. + */ +function blockItems(blocks) { + return blocks.flatMap((block) => { + if (block.kind === "single") return [block.item]; + if (block.kind !== "turn") return []; + return block.segments.flatMap((segment) => { + if (segment.kind === "prompt") return [segment.user]; + if (segment.kind === "setup") return segment.items; + if (segment.kind === "summary") return segment.summary.items; + return [segment.item]; + }); + }); +} + +/** `buildConversationTurnMeta` with the item stream derived from the blocks. */ +function metaFor(blocks, { items, ...options }) { + return buildConversationTurnMeta(blocks, { + ...options, + items: items ?? blockItems(blocks), + }); +} + function itemSegment(id, overrides) { return { kind: "item", @@ -66,7 +171,7 @@ const toolSegment = (id, timestamp) => test("buildConversationTurnMeta returns the shared empty value for other variants", () => { for (const variant of ["default", "compactPreview"]) { assert.equal( - buildConversationTurnMeta([turnBlock([])], { + metaFor([turnBlock([])], { isTurnLive: true, variant, }), @@ -80,7 +185,7 @@ test("buildConversationTurnMeta reports nothing streaming when the turn is idle" // The hint exists to tell the work block it is still working. A finished turn // has no tail, and reporting one would pin the block open forever. assert.equal( - buildConversationTurnMeta( + metaFor( [ turnBlock([ thoughtSegment("thought:1", "2026-06-14T19:00:02.000Z"), @@ -99,7 +204,7 @@ test("buildConversationTurnMeta names the trailing item of a live turn", () => { ]; assert.equal( - buildConversationTurnMeta(blocks, { + metaFor(blocks, { isTurnLive: true, variant: "conversation", }).streamingItemId, @@ -111,7 +216,7 @@ test("buildConversationTurnMeta names the trailing item of a live turn", () => { test("buildConversationTurnMeta skips setup segments when finding the tail", () => { // Setup renders as a quiet divider, not as work, so it must never be reported // as the streaming item — a lifecycle row would hold the block open. - const meta = buildConversationTurnMeta( + const meta = metaFor( [ turnBlock([ thoughtSegment("thought:1", "2026-06-14T19:00:01.000Z"), @@ -137,7 +242,7 @@ test("buildConversationTurnMeta skips setup segments when finding the tail", () }); test("buildConversationTurnMeta counts a mid-turn steer prompt as the tail", () => { - const meta = buildConversationTurnMeta( + const meta = metaFor( [ turnBlock([ thoughtSegment("thought:1", "2026-06-14T19:00:01.000Z"), @@ -153,7 +258,7 @@ test("buildConversationTurnMeta counts a mid-turn steer prompt as the tail", () test("buildConversationTurnMeta expands a summary segment to reach its last tool", () => { // A summary segment holds several leaf items; the tail is the last of them, // not the summary itself, which has no item id the work block could match. - const meta = buildConversationTurnMeta( + const meta = metaFor( [ turnBlock([ { @@ -180,7 +285,7 @@ test("buildConversationTurnMeta expands a summary segment to reach its last tool }); test("buildConversationTurnMeta reads a trailing single block directly", () => { - const meta = buildConversationTurnMeta( + const meta = metaFor( [ turnBlock([thoughtSegment("thought:1", "2026-06-14T19:00:01.000Z")]), { @@ -209,7 +314,7 @@ test("buildConversationTurnMeta reads a trailing single block directly", () => { * ownership is knowledge only the list has, so it is published here. */ test("buildConversationTurnMeta names the live turn so the block can gate running steps", () => { - const meta = buildConversationTurnMeta( + const meta = metaFor( [turnBlock([thoughtSegment("thought:1", "2026-06-14T19:00:01.000Z")])], { isTurnLive: true, variant: "conversation" }, ); @@ -220,7 +325,7 @@ test("buildConversationTurnMeta names the live turn so the block can gate runnin test("buildConversationTurnMeta reports no live turn when the turn is idle", () => { // This is the case the orphaned-step bug lived in: history reopened, nothing // live, but an item still claiming `executing`. - const meta = buildConversationTurnMeta( + const meta = metaFor( [turnBlock([thoughtSegment("thought:1", "2026-06-14T19:00:01.000Z")])], { isTurnLive: false, variant: "conversation" }, ); @@ -238,7 +343,7 @@ test("buildConversationTurnMeta names the live turn even when a lifecycle row tr // to, so "the last block" is not always the live turn — but the last turn is. // Reading the last block's kind alone would report no live turn and gate every // running step off, which is the mirror image of the bug being fixed. - const meta = buildConversationTurnMeta( + const meta = metaFor( [ turnBlock([thoughtSegment("thought:1", "2026-06-14T19:00:01.000Z")]), { @@ -260,7 +365,12 @@ test("buildConversationTurnMeta names the live turn even when a lifecycle row tr }); test("buildConversationTurnMeta reports no live turn when there is no turn at all", () => { - const meta = buildConversationTurnMeta( + // `turnId: null` explicitly: a session-started row arrives before any turn + // exists, so it belongs to nobody. The shared `item()` helper defaults to + // `turn-1`, which would have made this fixture describe a transcript that + // does have a turn — and the assertion would then be checking that a turn + // with no BLOCK reports no live turn, which is the opposite of the rule. + const meta = metaFor( [ { kind: "single", @@ -271,6 +381,7 @@ test("buildConversationTurnMeta reports no live turn when there is no turn at al title: "Session started", text: "", timestamp: "2026-06-14T19:00:00.000Z", + turnId: null, }), }, ], @@ -280,6 +391,117 @@ test("buildConversationTurnMeta reports no live turn when there is no turn at al assert.equal(meta.liveTurnId, null); }); +// ---- buildConversationTurnMeta: the gap between two turns ---- + +/** + * The next turn owns liveness the moment it appears, even before it has anything + * to show. + * + * A turn that has only emitted setup lifecycle rows (`turn_started`, + * `session_resolved`) classifies to zero segments and therefore produces NO + * block — `agentSessionTranscriptGrouping` only pushes a turn block + * `if (segments.length > 0)`. So "the newest turn with a block" is the turn that + * ALREADY ENDED for the whole gap between `turn_started` and the new turn's + * first prompt or thought, and the ended turn's own trailing item then gets + * reported as the streaming item. Its finished work block reads as live again: + * the folded summary is replaced by the open rail, a 6-step block drops to its + * last three with a previous-steps disclosure, and then it all folds back. + * + * That gap is real observer-stream latency, and `turn_started` fires on every + * turn (`activeAgentTurnsStore`), so this was not an edge case — it was every + * turn, for as long as the agent took to emit its first renderable item. + * + * Built from a raw item stream through the real grouping rather than from + * hand-written blocks, because the whole bug is which turns DO and DO NOT + * produce a block: hand-built blocks would assume the answer away. + */ +test("buildConversationTurnMeta hands liveness to a next turn that has no block yet", () => { + const stream = (...extra) => [ + lifecycleItem("life:start:1", "turn_started", "turn-1"), + promptItem("prompt:1", "turn-1"), + toolItem("tool:1", "turn-1"), + // turn-1's last act is a completed tool: the agent answered by posting + // through buzz-cli, so no assistant message trails its work. + toolItem("tool:2", "turn-1"), + ...extra, + ]; + + const frames = [ + { + label: "turn-2 has only started", + items: stream(lifecycleItem("life:start:2", "turn_started", "turn-2")), + }, + { + label: "the new session's system prompt has landed", + items: stream( + lifecycleItem("life:start:2", "turn_started", "turn-2"), + systemPromptItem("meta:sysprompt"), + ), + }, + { + label: "the session has resolved", + items: stream( + lifecycleItem("life:start:2", "turn_started", "turn-2"), + systemPromptItem("meta:sysprompt"), + lifecycleItem("life:resolved:2", "session_resolved", "turn-2"), + ), + }, + ]; + + for (const frame of frames) { + const meta = buildConversationTurnMeta( + buildTranscriptDisplayBlocks(frame.items), + { isTurnLive: true, items: frame.items, variant: "conversation" }, + ); + assert.equal( + meta.liveTurnId, + "turn-2", + `the newest turn owns liveness once ${frame.label}`, + ); + assert.equal( + meta.streamingItemId, + null, + `turn-1's tail must not read as streaming once ${frame.label}`, + ); + } + + // Fourth frame: turn-2 finally has something renderable, so it gets a block + // and its own prompt becomes the tail. Included so the sequence ends in the + // steady state rather than stopping at the gap. + const renderable = stream( + lifecycleItem("life:start:2", "turn_started", "turn-2"), + systemPromptItem("meta:sysprompt"), + lifecycleItem("life:resolved:2", "session_resolved", "turn-2"), + promptItem("prompt:2", "turn-2"), + ); + const meta = buildConversationTurnMeta( + buildTranscriptDisplayBlocks(renderable), + { isTurnLive: true, items: renderable, variant: "conversation" }, + ); + assert.equal(meta.liveTurnId, "turn-2"); + assert.equal(meta.streamingItemId, "prompt:2"); +}); + +test("buildConversationTurnMeta still names the live turn's own trailing item", () => { + // The other half of the ownership check: when the newest turn IS the turn that + // owns the trailing item, that item is the streaming one. Without this the + // gate would be a blanket "never report a tail" and every live block would + // fold while the reader watched it arrive. + const items = [ + lifecycleItem("life:start:1", "turn_started", "turn-1"), + promptItem("prompt:1", "turn-1"), + toolItem("tool:1", "turn-1"), + ]; + const meta = buildConversationTurnMeta(buildTranscriptDisplayBlocks(items), { + isTurnLive: true, + items, + variant: "conversation", + }); + + assert.equal(meta.liveTurnId, "turn-1"); + assert.equal(meta.streamingItemId, "tool:1"); +}); + // ---- parsePlanChecklist ---- test("parsePlanChecklist reads the checkbox markdown the transcript builds", () => { diff --git a/desktop/src/features/agents/ui/agentSessionConversationMeta.ts b/desktop/src/features/agents/ui/agentSessionConversationMeta.ts index 6456225981..7992caa5df 100644 --- a/desktop/src/features/agents/ui/agentSessionConversationMeta.ts +++ b/desktop/src/features/agents/ui/agentSessionConversationMeta.ts @@ -48,29 +48,91 @@ function turnSegmentItems(segment: TranscriptTurnSegment): TranscriptItem[] { */ export function buildConversationTurnMeta( displayBlocks: TranscriptDisplayBlock[], - options: { isTurnLive: boolean; variant: AgentSessionTranscriptVariant }, + options: { + isTurnLive: boolean; + items: TranscriptItem[]; + variant: AgentSessionTranscriptVariant; + }, ): AgentSessionTranscriptTurnMeta { if (options.variant !== "conversation" || !options.isTurnLive) { return EMPTY_TRANSCRIPT_TURN_META; } const lastBlock = displayBlocks[displayBlocks.length - 1]; - // The live turn is the last turn in the transcript: work arrives in order, so - // a turn with anything after it has already been superseded. Read from the - // blocks rather than from the active-turn store because the store's turn ids - // and the transcript's are populated by different paths, and a mismatch would - // silently gate every step off. - const liveTurnId = lastTurnId(displayBlocks); + // The live turn is the newest turn in the ITEM stream, not the newest turn + // that produced a block. See `latestTurnId` for why the distinction is + // load-bearing. Read from the transcript rather than from the active-turn + // store because the store's turn ids and the transcript's are populated by + // different paths, and a mismatch would silently gate every step off. + const liveTurnId = latestTurnId(options.items, displayBlocks); if (lastBlock?.kind === "single") { - return { liveTurnId, streamingItemId: lastBlock.item.id }; + return { + liveTurnId, + streamingItemId: streamingIdForTail(lastBlock.item, liveTurnId), + }; } if (lastBlock?.kind !== "turn") { return { liveTurnId, streamingItemId: null }; } const items = lastBlock.segments.flatMap(turnSegmentItems); - return { liveTurnId, streamingItemId: items[items.length - 1]?.id ?? null }; + const tail = items[items.length - 1]; + return { + liveTurnId, + streamingItemId: tail ? streamingIdForTail(tail, liveTurnId) : null, + }; +} + +/** + * The trailing item counts as streaming only if the LIVE turn owns it. + * + * Without this check the newest block's tail is reported as streaming no matter + * whose turn it belongs to, so a finished turn's last step would hold that + * turn's work block open while a *different* turn is the live one. That is the + * same class of bug as an abandoned `executing` step: presenting settled history + * as work in flight. + */ +function streamingIdForTail( + tail: TranscriptItem, + liveTurnId: string | null, +): string | null { + // Compared rather than tested for truthiness, for the same reason as + // `toolEntryState`: an item with no turn id is owned by nobody, and + // `null === null` must not read as ownership. + if (liveTurnId === null || tail.turnId !== liveTurnId) return null; + return tail.id; +} + +/** + * The id of the newest turn the transcript has seen — including a turn that has + * arrived but has nothing renderable in it yet. + * + * Read from the items rather than the blocks because a turn that has only + * emitted setup lifecycle rows (`turn_started`, `session_resolved`) classifies + * to zero segments and so produces NO block at all + * (`agentSessionTranscriptGrouping`: `if (segments.length > 0)`). Walking the + * blocks backwards for the last `turn` therefore skipped straight past the new + * turn and returned the turn that had already ended — which then made its own + * trailing item the streaming item, and a settled block visibly re-opened, + * dropped to its last three steps and re-expanded for the whole gap between + * `turn_started` and the new turn's first prompt or thought. That gap is real + * observer-stream latency and happens on every turn, so this was not an edge + * case. + * + * Items are in wire order, so the last one carrying a turn id names the newest + * turn. Falls back to the last turn block for an item stream that carries no + * turn ids at all. + */ +function latestTurnId( + items: TranscriptItem[], + displayBlocks: TranscriptDisplayBlock[], +): string | null { + for (let index = items.length - 1; index >= 0; index -= 1) { + const turnId = items[index]?.turnId; + if (turnId) return turnId; + } + return lastTurnBlockId(displayBlocks); } /** @@ -80,7 +142,9 @@ export function buildConversationTurnMeta( * turn it belongs to and arrives as a `single` block, so the last block is not * always the live turn — but the last *turn* is. */ -function lastTurnId(displayBlocks: TranscriptDisplayBlock[]): string | null { +function lastTurnBlockId( + displayBlocks: TranscriptDisplayBlock[], +): string | null { for (let index = displayBlocks.length - 1; index >= 0; index -= 1) { const block = displayBlocks[index]; if (block.kind === "turn") return block.turnId;