diff --git a/docs/fix-chat-thinking-order/intent.md b/docs/fix-chat-thinking-order/intent.md new file mode 100644 index 0000000..5f15be1 --- /dev/null +++ b/docs/fix-chat-thinking-order/intent.md @@ -0,0 +1,27 @@ +# Intent: Preserve Chat thinking block order +Author: SpireCode maintainer. Status: approved. + +## Problem + +Chat often renders an agent's thinking section after the final answer even though Pi stores and streams thinking before the answer. SpireCode currently flattens an assistant message into one text item first and appends thinking items afterward, losing Pi's content-block order in both restored and live conversations. + +## Proposed outcome + +Render assistant thinking, text, and tool-call blocks in the same order supplied by Pi. Live streaming and restored session snapshots must produce the same timeline order, without duplicate blocks or position jumps as partial messages update. + +## Affected users and systems + +- Chat transcript rendering for all reasoning-capable providers, including TraeX. +- Electron Main's Pi event normalization. +- Renderer Chat timeline state and process grouping. + +## Constraints + +- Preserve the sandbox boundary and existing narrow Chat IPC. +- Treat Pi's assistant content array and stream `contentIndex` ordering as authoritative. +- Preserve tool execution updates and process grouping. +- Do not reorder unrelated user messages, notices, or todo snapshots. + +## Open questions + +None. diff --git a/docs/fix-chat-thinking-order/plan.md b/docs/fix-chat-thinking-order/plan.md new file mode 100644 index 0000000..1164cc2 --- /dev/null +++ b/docs/fix-chat-thinking-order/plan.md @@ -0,0 +1,35 @@ +# Plan: Preserve Chat thinking block order (from docs/fix-chat-thinking-order/spec.md 2026-09-23) + +## Files that change + +- `electron/domains/chat/wire.ts` — preserve assistant content-block order in snapshot and event normalization. +- `electron/domains/chat/wire.test.ts` — add restored and live ordering regressions. +- `src/features/chat/sessionReducer.test.ts` — prove keyed streaming updates retain block positions. +- `docs/fix-chat-thinking-order/intent.md` — capture the approved user-visible correction. +- `docs/fix-chat-thinking-order/spec.md` — define block ordering and compatibility requirements. +- `docs/fix-chat-thinking-order/plan.md` — record implementation order, risks, and proof. + +## Order of work + +1. Add regression assertions showing that `thinking -> text -> tool -> thinking -> text` is currently flattened out of order in restored and live normalization. +2. Run the focused tests and retain the expected failure as red evidence. +3. Refactor assistant normalization around stable per-content-index block IDs, emitting timeline items/events in Pi source order. +4. Ensure tool block IDs correlate with execution events and preserve error fallback behavior. +5. Add reducer coverage proving later partial/final updates replace blocks in place rather than appending them. +6. Run focused Chat normalization/reducer/display tests. +7. Run `pnpm check`, inspect the diff, commit, push, and open a pull request. + +## Risks + +- The most dangerous change is replacing one aggregated assistant text item with block-level text items; React keys and reducer IDs must remain stable across stream updates. +- Tool cards can arrive from assistant content and tool execution lifecycle events; mismatched IDs would create duplicate cards. +- Providers may expose sparse content arrays during streaming. Iteration must preserve defined indices without inventing unstable IDs. +- A renderer-only reorder is rejected because the original content index is already discarded by the backend DTO. +- A simple “put thinking before message” swap is rejected because it still fails interleaved multi-block responses. + +## Proof + +- Red: focused wire test fails because current output places aggregated text before thinking. +- Green: `pnpm test -- electron/domains/chat/wire.test.ts src/features/chat/sessionReducer.test.ts src/features/chat/chatDisplayItems.test.tsx` (or the matching existing `.ts` path) exits 0. +- `pnpm check` exits 0. +- `git diff --check` exits 0. diff --git a/docs/fix-chat-thinking-order/spec.md b/docs/fix-chat-thinking-order/spec.md new file mode 100644 index 0000000..c367d83 --- /dev/null +++ b/docs/fix-chat-thinking-order/spec.md @@ -0,0 +1,28 @@ +# Spec: Preserve Chat thinking block order + +## Requirements + +1. Restored assistant messages must project `thinking`, `text`, and `toolCall` blocks in source-array order. +2. Live assistant events must create and update timeline blocks according to Pi's authoritative assistant message content order. +3. Repeated stream updates must replace existing blocks without moving them to the end or duplicating them. +4. Multiple text blocks in one assistant turn must remain independently ordered around thinking and tool blocks. +5. Empty completed assistant text placeholders must remain hidden where the display projection currently uses them only to join adjacent tool operations. +6. Existing tool result correlation, todo ordering, and notice behavior must remain unchanged. + +## Design + +Introduce stable block-level IDs derived from the assistant message ID and content index. Normalize each assistant block into its own timeline item: + +- `thinking` becomes a thinking item. +- `text` becomes an assistant/error message item. +- `toolCall` becomes a tool item when restoring history; live tool cards continue to receive authoritative execution lifecycle events. + +For live `message_start`, `message_update`, and `message_end`, emit block events in source order rather than emitting one aggregate text message before all thinking blocks. The reducer's existing keyed upsert then preserves the first observed position while replacing partial content. The final `message_end` snapshot supplies the canonical status and content for every block. + +User messages remain one message item because they are not mixed with assistant reasoning/tool blocks in this UI contract. + +## Concerns + +- Some providers may emit a provisional empty assistant content array at `message_start`; no visible item should be created until a block exists. +- Tool execution may start after a tool-call block has already been projected. Both paths must use the same `toolCallId` so the reducer updates rather than duplicates the tool. +- Error responses with no text block still need one visible error item carrying `errorMessage`. diff --git a/electron/domains/chat/wire.test.ts b/electron/domains/chat/wire.test.ts index ccda727..89dfeaf 100644 --- a/electron/domains/chat/wire.test.ts +++ b/electron/domains/chat/wire.test.ts @@ -39,6 +39,8 @@ describe("chat wire normalization", () => { name: "read", arguments: { path: "a.ts" }, }, + { type: "thinking", thinking: "verify" }, + { type: "text", text: "confirmed" }, ], timestamp: 2, }, @@ -57,20 +59,20 @@ describe("chat wire normalization", () => { status: "complete", createdAt: 1, }, + { + type: "thinking", + id: "message:assistant:2:thinking:0", + content: "inspect", + status: "complete", + }, { type: "message", - id: "message:assistant:2", + id: "message:assistant:2:text:1", role: "assistant", content: "done", status: "complete", createdAt: 2, }, - { - type: "thinking", - id: "message:assistant:2:thinking:0", - content: "inspect", - status: "complete", - }, { type: "tool", toolCallId: "call-1", @@ -79,6 +81,113 @@ describe("chat wire normalization", () => { result: "file", status: "done", }, + { + type: "thinking", + id: "message:assistant:2:thinking:3", + content: "verify", + status: "complete", + }, + { + type: "message", + id: "message:assistant:2:text:4", + role: "assistant", + content: "confirmed", + status: "complete", + createdAt: 2, + }, + ]); + }); + + it("preserves interleaved assistant block order in live events", () => { + expect( + normalizeEvent({ + type: "message_update", + message: { + role: "assistant", + timestamp: 2, + content: [ + { type: "thinking", thinking: "inspect" }, + { type: "text", text: "before" }, + { + type: "toolCall", + id: "call-1", + name: "read", + arguments: { path: "a.ts" }, + }, + { type: "thinking", thinking: "verify" }, + { type: "text", text: "after" }, + ], + }, + }), + ).toEqual([ + { + type: "thinking_update", + thinking: { + id: "message:assistant:2:thinking:0", + content: "inspect", + status: "streaming", + }, + }, + { + type: "message_update", + message: { + id: "message:assistant:2:text:1", + role: "assistant", + content: "before", + status: "streaming", + createdAt: 2, + }, + }, + { + type: "tool_execution_start", + toolCallId: "call-1", + toolName: "read", + arguments: { path: "a.ts" }, + }, + { + type: "thinking_update", + thinking: { + id: "message:assistant:2:thinking:3", + content: "verify", + status: "streaming", + }, + }, + { + type: "message_update", + message: { + id: "message:assistant:2:text:4", + role: "assistant", + content: "after", + status: "streaming", + createdAt: 2, + }, + }, + ]); + }); + + it("preserves a visible error when an assistant failure has no content blocks", () => { + expect( + normalizeEvent({ + type: "message_end", + message: { + role: "assistant", + timestamp: 3, + content: [], + stopReason: "error", + errorMessage: "provider failed", + }, + }), + ).toEqual([ + { + type: "message_end", + message: { + id: "message:assistant:3:error", + role: "error", + content: "provider failed", + status: "error", + createdAt: 3, + }, + }, ]); }); diff --git a/electron/domains/chat/wire.ts b/electron/domains/chat/wire.ts index 6fb7e07..8d253b5 100644 --- a/electron/domains/chat/wire.ts +++ b/electron/domains/chat/wire.ts @@ -135,19 +135,7 @@ export function normalizeMessages(messages: unknown): unknown[] { ? raw.content : [{ type: "text", text: textOf(raw.content) }]; const failed = raw.stopReason === "error"; - items.push({ - type: "message", - id: base, - role: failed ? "error" : "assistant", - content: blocks - .filter((block) => isRecord(block) && block.type === "text") - .map((block) => String((block as Record).text ?? "")) - .join(""), - status: failed ? "error" : "complete", - ...(epoch(raw.timestamp) === undefined - ? {} - : { createdAt: epoch(raw.timestamp) }), - }); + let hasText = false; blocks.forEach((block, blockIndex) => { if (!isRecord(block)) return; if (block.type === "thinking" && typeof block.thinking === "string") { @@ -157,6 +145,18 @@ export function normalizeMessages(messages: unknown): unknown[] { content: block.thinking, status: "complete", }); + } else if (block.type === "text" && typeof block.text === "string") { + hasText = true; + items.push({ + type: "message", + id: `${base}:text:${blockIndex}`, + role: failed ? "error" : "assistant", + content: block.text, + status: failed ? "error" : "complete", + ...(epoch(raw.timestamp) === undefined + ? {} + : { createdAt: epoch(raw.timestamp) }), + }); } else if (block.type === "toolCall" && typeof block.id === "string") { const item = { type: "tool", @@ -169,6 +169,18 @@ export function normalizeMessages(messages: unknown): unknown[] { items.push(item); } }); + if (failed && !hasText) { + items.push({ + type: "message", + id: `${base}:error`, + role: "error", + content: stringValue(raw.errorMessage) ?? "Agent run failed", + status: "error", + ...(epoch(raw.timestamp) === undefined + ? {} + : { createdAt: epoch(raw.timestamp) }), + }); + } return; } if (raw.role === "toolResult" && typeof raw.toolCallId === "string") { @@ -235,53 +247,82 @@ function normalizeMessageEvent( ) return []; const failed = message.role === "assistant" && message.stopReason === "error"; - const status = failed + const messageStatus = failed ? "error" : type === "message_end" ? "complete" : "streaming"; + const blockEventSuffix = type.slice("message_".length); const id = messageId(message); const blocks = Array.isArray(message.content) ? message.content : [{ type: "text", text: textOf(message.content) }]; - const content = blocks - .filter((block) => isRecord(block) && block.type === "text") - .map((block) => String((block as Record).text ?? "")) - .join(""); - const events: Array> = [ - { - type, - message: { - id, - role: failed ? "error" : message.role, - content: - content || - (failed - ? (stringValue(message.errorMessage) ?? "Agent run failed") - : ""), - status, - ...(epoch(message.timestamp) === undefined - ? {} - : { createdAt: epoch(message.timestamp) }), + + if (message.role === "user") { + return [ + { + type, + message: { + id, + role: "user", + content: textOf(message.content), + status: messageStatus, + ...(epoch(message.timestamp) === undefined + ? {} + : { createdAt: epoch(message.timestamp) }), + }, }, - }, - ]; - if (message.role === "assistant") { - blocks.forEach((block, index) => { - if ( - !isRecord(block) || - block.type !== "thinking" || - typeof block.thinking !== "string" - ) - return; + ]; + } + + const events: Array> = []; + let hasText = false; + blocks.forEach((block, index) => { + if (!isRecord(block)) return; + if (block.type === "thinking" && typeof block.thinking === "string") { events.push({ - type: `thinking_${type.slice("message_".length)}`, + type: `thinking_${blockEventSuffix}`, thinking: { id: `${id}:thinking:${index}`, content: block.thinking, - status, + status: type === "message_end" ? "complete" : "streaming", }, }); + } else if (block.type === "text" && typeof block.text === "string") { + hasText = true; + events.push({ + type, + message: { + id: `${id}:text:${index}`, + role: failed ? "error" : "assistant", + content: block.text, + status: messageStatus, + ...(epoch(message.timestamp) === undefined + ? {} + : { createdAt: epoch(message.timestamp) }), + }, + }); + } else if (block.type === "toolCall" && typeof block.id === "string") { + events.push({ + type: "tool_execution_start", + toolCallId: block.id, + toolName: stringValue(block.name) ?? "unknown", + arguments: block.arguments, + }); + } + }); + if (failed && !hasText) { + events.push({ + type, + message: { + id: `${id}:error`, + role: "error", + content: stringValue(message.errorMessage) ?? "Agent run failed", + status: "error", + ...(epoch(message.timestamp) === undefined + ? {} + : { createdAt: epoch(message.timestamp) }), + }, }); } return events; diff --git a/src/features/chat/sessionReducer.test.ts b/src/features/chat/sessionReducer.test.ts index 4861194..6c1957a 100644 --- a/src/features/chat/sessionReducer.test.ts +++ b/src/features/chat/sessionReducer.test.ts @@ -92,6 +92,68 @@ describe("sessionReducer", () => { }); }); + it("updates streamed assistant blocks without moving their first positions", () => { + let state = createInitialChatState("session-1", "worktree-1", "idle"); + state = reduce(state, 1, { + type: "thinking_start", + thinking: { + id: "assistant-1:thinking:0", + content: "", + status: "streaming", + }, + }); + state = reduce(state, 2, { + type: "thinking_update", + thinking: { + id: "assistant-1:thinking:0", + content: "inspect", + status: "streaming", + }, + }); + state = reduce(state, 3, { + type: "message_update", + message: { + id: "assistant-1:text:1", + role: "assistant", + content: "answer", + status: "streaming", + }, + }); + state = reduce(state, 4, { + type: "thinking_end", + thinking: { + id: "assistant-1:thinking:0", + content: "inspected", + status: "complete", + }, + }); + state = reduce(state, 5, { + type: "message_end", + message: { + id: "assistant-1:text:1", + role: "assistant", + content: "final answer", + status: "complete", + }, + }); + + expect(state.items).toEqual([ + { + type: "thinking", + id: "assistant-1:thinking:0", + content: "inspected", + status: "complete", + }, + { + type: "message", + id: "assistant-1:text:1", + role: "assistant", + content: "final answer", + status: "complete", + }, + ]); + }); + it("updates and clears transient extension activity", () => { let state = createInitialChatState("session-1", "worktree-1", "streaming"); state = reduce(state, 1, {