diff --git a/packages/pi-coding-agent/src/core/chat-controller-ordering.test.ts b/packages/pi-coding-agent/src/core/chat-controller-ordering.test.ts index eb7795508..383580724 100644 --- a/packages/pi-coding-agent/src/core/chat-controller-ordering.test.ts +++ b/packages/pi-coding-agent/src/core/chat-controller-ordering.test.ts @@ -94,7 +94,7 @@ function createHost() { return host; } -test("chat-controller keeps tool output ahead of delayed assistant text for external tool streams", async () => { +test("chat-controller renders content blocks in content[] index order (tool-first stream)", async () => { // ToolExecutionComponent uses the global theme singleton. // Install a minimal no-op theme implementation for this unit test. (globalThis as any)[Symbol.for("@gsd/pi-coding-agent:theme")] = { @@ -116,7 +116,6 @@ test("chat-controller keeps tool output ahead of delayed assistant text for exte await handleAgentEvent(host, { type: "message_start", message: makeAssistant([]) } as any); - assert.equal(host.streamingComponent, undefined, "assistant component should be deferred at message_start"); assert.equal(host.chatContainer.children.length, 0, "nothing should render before content arrives"); await handleAgentEvent( @@ -140,11 +139,10 @@ test("chat-controller keeps tool output ahead of delayed assistant text for exte } as any, ); - assert.equal(host.streamingComponent, undefined, "assistant text container should remain deferred for tool-only updates"); + // content[0] = toolCall → ToolExecutionComponent renders first assert.equal(host.chatContainer.children.length, 1, "tool execution block should render immediately"); assert.equal(host.chatContainer.children[0]?.constructor?.name, "ToolExecutionComponent"); - // Re-assert required host method before the text-bearing update path. host.getMarkdownThemeWithSettings = () => ({}); await handleAgentEvent( @@ -161,12 +159,13 @@ test("chat-controller keeps tool output ahead of delayed assistant text for exte } as any, ); - assert.equal(host.chatContainer.children.length, 2, "assistant content should render after existing tool output"); + // content[0]=toolCall, content[1]=text → order: tool, then text + assert.equal(host.chatContainer.children.length, 2, "text run should render after tool in content[] order"); assert.equal(host.chatContainer.children[0]?.constructor?.name, "ToolExecutionComponent"); assert.equal(host.chatContainer.children[1]?.constructor?.name, "AssistantMessageComponent"); }); -test("chat-controller keeps serverToolUse output ahead of assistant text when external results arrive", async () => { +test("chat-controller renders serverToolUse before trailing text matching content[] index order", async () => { (globalThis as any)[Symbol.for("@gsd/pi-coding-agent:theme")] = { fg: (_key: string, text: string) => text, bg: (_key: string, text: string) => text, @@ -199,7 +198,7 @@ test("chat-controller keeps serverToolUse output ahead of assistant text when ex } as any, ); - assert.equal(host.streamingComponent, undefined, "assistant content should stay deferred while only tool content streams"); + // content[0] = serverToolUse → ToolExecutionComponent renders first assert.equal(host.chatContainer.children.length, 1, "server tool block should render immediately"); assert.equal(host.chatContainer.children[0]?.constructor?.name, "ToolExecutionComponent"); @@ -229,7 +228,8 @@ test("chat-controller keeps serverToolUse output ahead of assistant text when ex } as any, ); - assert.equal(host.chatContainer.children.length, 2, "assistant text should render after existing server tool output"); + // content[0]=serverToolUse, content[1]=text → order: tool, then text + assert.equal(host.chatContainer.children.length, 2, "text run should render after server tool in content[] order"); assert.equal(host.chatContainer.children[0]?.constructor?.name, "ToolExecutionComponent"); assert.equal(host.chatContainer.children[1]?.constructor?.name, "AssistantMessageComponent"); }); @@ -466,3 +466,193 @@ test("chat-controller does not pin when there are no tool calls", async () => { assert.equal(host.pinnedMessageContainer.children.length, 0, "pinned zone should stay empty without tool calls"); }); + +// Regression test for issue #4144: interleaved text/tool content must render in content[] index order. +// Stream: [text "A", toolCall T1, text "B", toolCall T2, text "C"] +// Expected chatContainer order: textRun(A), toolExec(T1), textRun(B), toolExec(T2), textRun(C) +// Each AssistantMessageComponent must render ONLY its own text — no duplication after message_end. +test("chat-controller renders interleaved text and tool blocks in content[] index order (#4144)", async () => { + (globalThis as any)[Symbol.for("@gsd/pi-coding-agent:theme")] = { + fg: (_key: string, text: string) => text, + bg: (_key: string, text: string) => text, + bold: (text: string) => text, + italic: (text: string) => text, + truncate: (text: string) => text, + }; + + const host = createHost(); + host.getMarkdownThemeWithSettings = () => ({}); + + const t1 = { type: "toolCall", id: "t1", name: "tool_one", arguments: {} }; + const t2 = { type: "toolCall", id: "t2", name: "tool_two", arguments: {} }; + + await handleAgentEvent(host, { type: "message_start", message: makeAssistant([]) } as any); + + // Stream text "A" at index 0 + await handleAgentEvent(host, { + type: "message_update", + message: makeAssistant([{ type: "text", text: "A" }]), + assistantMessageEvent: { + type: "text_delta", + contentIndex: 0, + delta: "A", + partial: makeAssistant([{ type: "text", text: "A" }]), + }, + } as any); + + // Stream toolCall T1 at index 1 + await handleAgentEvent(host, { + type: "message_update", + message: makeAssistant([{ type: "text", text: "A" }, t1]), + assistantMessageEvent: { + type: "toolcall_end", + contentIndex: 1, + toolCall: { + ...t1, + externalResult: { content: [{ type: "text", text: "result1" }], details: {}, isError: false }, + }, + partial: makeAssistant([{ type: "text", text: "A" }, t1]), + }, + } as any); + + // Stream text "B" at index 2 + await handleAgentEvent(host, { + type: "message_update", + message: makeAssistant([{ type: "text", text: "A" }, t1, { type: "text", text: "B" }]), + assistantMessageEvent: { + type: "text_delta", + contentIndex: 2, + delta: "B", + partial: makeAssistant([{ type: "text", text: "A" }, t1, { type: "text", text: "B" }]), + }, + } as any); + + // Stream toolCall T2 at index 3 + await handleAgentEvent(host, { + type: "message_update", + message: makeAssistant([{ type: "text", text: "A" }, t1, { type: "text", text: "B" }, t2]), + assistantMessageEvent: { + type: "toolcall_end", + contentIndex: 3, + toolCall: { + ...t2, + externalResult: { content: [{ type: "text", text: "result2" }], details: {}, isError: false }, + }, + partial: makeAssistant([{ type: "text", text: "A" }, t1, { type: "text", text: "B" }, t2]), + }, + } as any); + + // Stream text "C" at index 4 + const finalContent = [ + { type: "text", text: "A" }, t1, { type: "text", text: "B" }, t2, { type: "text", text: "C" }, + ]; + await handleAgentEvent(host, { + type: "message_update", + message: makeAssistant(finalContent), + assistantMessageEvent: { + type: "text_delta", + contentIndex: 4, + delta: "C", + partial: makeAssistant(finalContent), + }, + } as any); + + // Finalize — exercises the message_end path where a buggy setRange(undefined) would cause duplication + await handleAgentEvent(host, { type: "message_end", message: makeAssistant(finalContent) } as any); + + // Assert interleaved order: textRun(A), toolExec(T1), textRun(B), toolExec(T2), textRun(C) + assert.equal(host.chatContainer.children.length, 5, "should have 5 children in interleaved order"); + assert.equal(host.chatContainer.children[0]?.constructor?.name, "AssistantMessageComponent", "index 0: text run A"); + assert.equal(host.chatContainer.children[1]?.constructor?.name, "ToolExecutionComponent", "index 1: tool T1"); + assert.equal(host.chatContainer.children[2]?.constructor?.name, "AssistantMessageComponent", "index 2: text run B"); + assert.equal(host.chatContainer.children[3]?.constructor?.name, "ToolExecutionComponent", "index 3: tool T2"); + assert.equal(host.chatContainer.children[4]?.constructor?.name, "AssistantMessageComponent", "index 4: text run C"); + + // Helper: collect the text of all Markdown children inside an AssistantMessageComponent. + // Structure: AssistantMessageComponent (Container) -> contentContainer (children[0]) -> Markdown nodes. + function getRenderedTexts(comp: any): string[] { + const contentContainer = comp.children?.[0]; + if (!contentContainer) return []; + return (contentContainer.children ?? []) + .filter((c: any) => c.constructor?.name === "Markdown") + .map((c: any) => (c as any).text as string); + } + + // Each text-run component must contain only its own text — no cross-contamination after message_end + assert.deepEqual(getRenderedTexts(host.chatContainer.children[0]), ["A"], "text run A must contain only 'A'"); + assert.deepEqual(getRenderedTexts(host.chatContainer.children[2]), ["B"], "text run B must contain only 'B'"); + assert.deepEqual(getRenderedTexts(host.chatContainer.children[4]), ["C"], "text run C must contain only 'C'"); +}); + +test("chat-controller does not duplicate text when content is [text, tool, text] (interleaved stream)", async () => { + (globalThis as any)[Symbol.for("@gsd/pi-coding-agent:theme")] = { + fg: (_key: string, text: string) => text, + bg: (_key: string, text: string) => text, + bold: (text: string) => text, + italic: (text: string) => text, + truncate: (text: string) => text, + }; + + const host = createHost(); + host.getMarkdownThemeWithSettings = () => ({}); + + const t1 = { type: "toolCall", id: "t1", name: "tool_one", arguments: {} }; + + await handleAgentEvent(host, { type: "message_start", message: makeAssistant([]) } as any); + + // Step 1: text "A" at index 0 + await handleAgentEvent(host, { + type: "message_update", + message: makeAssistant([{ type: "text", text: "A" }]), + assistantMessageEvent: { + type: "text_delta", + contentIndex: 0, + delta: "A", + partial: makeAssistant([{ type: "text", text: "A" }]), + }, + } as any); + + // Step 2: toolCall at index 1 + await handleAgentEvent(host, { + type: "message_update", + message: makeAssistant([{ type: "text", text: "A" }, t1]), + assistantMessageEvent: { + type: "toolcall_end", + contentIndex: 1, + toolCall: { + ...t1, + externalResult: { content: [{ type: "text", text: "result1" }], details: {}, isError: false }, + }, + partial: makeAssistant([{ type: "text", text: "A" }, t1]), + }, + } as any); + + // Step 3: text "B" at index 2 + const finalContent = [{ type: "text", text: "A" }, t1, { type: "text", text: "B" }]; + await handleAgentEvent(host, { + type: "message_update", + message: makeAssistant(finalContent), + assistantMessageEvent: { + type: "text_delta", + contentIndex: 2, + delta: "B", + partial: makeAssistant(finalContent), + }, + } as any); + + assert.equal(host.chatContainer.children.length, 3); + assert.equal(host.chatContainer.children[0]?.constructor?.name, "AssistantMessageComponent"); + assert.equal(host.chatContainer.children[1]?.constructor?.name, "ToolExecutionComponent"); + assert.equal(host.chatContainer.children[2]?.constructor?.name, "AssistantMessageComponent"); + + const firstText = host.chatContainer.children[0]; + const secondText = host.chatContainer.children[2]; + assert.notEqual(firstText, secondText, "text-before-tool and text-after-tool must be separate component instances"); + assert.deepEqual((firstText as any).range, { startIndex: 0, endIndex: 0 }, "first text-run covers only content[0]"); + assert.deepEqual((secondText as any).range, { startIndex: 2, endIndex: 2 }, "second text-run covers only content[2]"); + + // Finalize — regression guard: range must NOT be cleared on message_end (would cause duplication) + await handleAgentEvent(host, { type: "message_end", message: makeAssistant(finalContent) } as any); + + assert.deepEqual((secondText as any).range, { startIndex: 2, endIndex: 2 }, "range must not be cleared on message_end (would cause duplication)"); +}); diff --git a/packages/pi-coding-agent/src/modes/interactive/components/assistant-message.ts b/packages/pi-coding-agent/src/modes/interactive/components/assistant-message.ts index c558b7cfc..b797cc906 100644 --- a/packages/pi-coding-agent/src/modes/interactive/components/assistant-message.ts +++ b/packages/pi-coding-agent/src/modes/interactive/components/assistant-message.ts @@ -3,8 +3,15 @@ import { Container, Markdown, type MarkdownTheme, Spacer, Text } from "@gsd/pi-t import { getMarkdownTheme, theme } from "../theme/theme.js"; import { formatTimestamp, type TimestampFormat } from "./timestamp.js"; +export interface ContentRange { + startIndex: number; + endIndex: number; +} + /** - * Component that renders a complete assistant message + * Component that renders a complete assistant message, or a sub-range of its content[]. + * When `range` is provided, only content[startIndex..endIndex] (inclusive) is rendered. + * Non-text/thinking blocks within the range are silently skipped. */ export class AssistantMessageComponent extends Container { private contentContainer: Container; @@ -12,18 +19,26 @@ export class AssistantMessageComponent extends Container { private markdownTheme: MarkdownTheme; private lastMessage?: AssistantMessage; private timestampFormat: TimestampFormat; + private range?: ContentRange; + private showMetadata: boolean; constructor( message?: AssistantMessage, hideThinkingBlock = false, markdownTheme: MarkdownTheme = getMarkdownTheme(), timestampFormat: TimestampFormat = "date-time-iso", + range?: ContentRange, ) { super(); this.hideThinkingBlock = hideThinkingBlock; this.markdownTheme = markdownTheme; this.timestampFormat = timestampFormat; + this.range = range; + // No range = legacy full-message rendering; show metadata by default. + // Ranged (interleaved) instances start with metadata hidden; chat-controller + // calls setShowMetadata(true) on the last segment at message_end. + this.showMetadata = !range; // Container for text/thinking content this.contentContainer = new Container(); @@ -34,6 +49,20 @@ export class AssistantMessageComponent extends Container { } } + setRange(range: ContentRange | undefined): void { + this.range = range; + if (this.lastMessage) { + this.updateContent(this.lastMessage); + } + } + + setShowMetadata(show: boolean): void { + this.showMetadata = show; + if (this.lastMessage) { + this.updateContent(this.lastMessage); + } + } + override invalidate(): void { super.invalidate(); if (this.lastMessage) { @@ -51,7 +80,11 @@ export class AssistantMessageComponent extends Container { // Clear content container this.contentContainer.clear(); - const hasVisibleContent = message.content.some( + const start = this.range?.startIndex ?? 0; + const end = this.range?.endIndex ?? message.content.length - 1; + const slice = message.content.slice(start, end + 1); + + const hasVisibleContent = slice.some( (c) => (c.type === "text" && c.text.trim()) || (c.type === "thinking" && c.thinking.trim()), ); @@ -59,9 +92,9 @@ export class AssistantMessageComponent extends Container { this.contentContainer.addChild(new Spacer(1)); } - // Render content in order - for (let i = 0; i < message.content.length; i++) { - const content = message.content[i]; + // Render content in order; non-text/thinking blocks are silently skipped + for (let i = 0; i < slice.length; i++) { + const content = slice[i]; if (content.type === "text" && content.text.trim()) { // Assistant text messages with no background - trim the text // Set paddingY=0 to avoid extra spacing before tool executions @@ -69,7 +102,7 @@ export class AssistantMessageComponent extends Container { } else if (content.type === "thinking" && content.thinking.trim()) { // Add spacing only when another visible assistant content block follows. // This avoids a superfluous blank line before separately-rendered tool execution blocks. - const hasVisibleContentAfter = message.content + const hasVisibleContentAfter = slice .slice(i + 1) .some((c) => (c.type === "text" && c.text.trim()) || (c.type === "thinking" && c.thinking.trim())); @@ -94,30 +127,33 @@ export class AssistantMessageComponent extends Container { } } - // Check if aborted - show after partial content - // But only if there are no tool calls (tool execution components will show the error) - const hasToolCalls = message.content.some((c) => c.type === "toolCall"); - if (!hasToolCalls) { - if (message.stopReason === "aborted") { - const abortMessage = - message.errorMessage && message.errorMessage !== "Request was aborted" - ? message.errorMessage - : "Operation aborted"; - if (hasVisibleContent) { + // Metadata (errors, timestamp): gated on showMetadata so ranged instances stay clean + // until chat-controller explicitly enables it on the last segment at message_end. + if (this.showMetadata) { + // Check if aborted - show after partial content + // But only if there are no tool calls (tool execution components will show the error) + const hasToolCalls = message.content.some((c) => c.type === "toolCall"); + if (!hasToolCalls) { + if (message.stopReason === "aborted") { + const abortMessage = + message.errorMessage && message.errorMessage !== "Request was aborted" + ? message.errorMessage + : "Operation aborted"; + if (hasVisibleContent) { + this.contentContainer.addChild(new Spacer(1)); + } + this.contentContainer.addChild(new Text(theme.fg("error", abortMessage), 1, 0)); + } else if (message.stopReason === "error") { + const errorMsg = message.errorMessage || "Unknown error"; this.contentContainer.addChild(new Spacer(1)); + this.contentContainer.addChild(new Text(theme.fg("error", `Error: ${errorMsg}`), 1, 0)); } - this.contentContainer.addChild(new Text(theme.fg("error", abortMessage), 1, 0)); - } else if (message.stopReason === "error") { - const errorMsg = message.errorMessage || "Unknown error"; - this.contentContainer.addChild(new Spacer(1)); - this.contentContainer.addChild(new Text(theme.fg("error", `Error: ${errorMsg}`), 1, 0)); } - } - // Show timestamp when the message is complete (has a stop reason) - if (message.stopReason && message.timestamp) { - const timeStr = formatTimestamp(message.timestamp, this.timestampFormat); - this.contentContainer.addChild(new Text(theme.fg("dim", timeStr), 1, 0)); + if (message.stopReason && message.timestamp) { + const timeStr = formatTimestamp(message.timestamp, this.timestampFormat); + this.contentContainer.addChild(new Text(theme.fg("dim", timeStr), 1, 0)); + } } } } diff --git a/packages/pi-coding-agent/src/modes/interactive/controllers/chat-controller.ts b/packages/pi-coding-agent/src/modes/interactive/controllers/chat-controller.ts index 1fe373f20..c230e37b7 100644 --- a/packages/pi-coding-agent/src/modes/interactive/controllers/chat-controller.ts +++ b/packages/pi-coding-agent/src/modes/interactive/controllers/chat-controller.ts @@ -10,6 +10,13 @@ import { appKey } from "../components/keybinding-hints.js"; // Tracks the last processed content index to avoid re-scanning all blocks on every message_update let lastProcessedContentIndex = 0; +// --- Segment walker state (per streaming assistant turn) --- +type RenderedSegment = + | { kind: "text-run"; startIndex: number; endIndex: number; component: AssistantMessageComponent } + | { kind: "tool"; contentIndex: number; component: ToolExecutionComponent }; + +let renderedSegments: RenderedSegment[] = []; + function hasVisibleAssistantContent(message: { content: Array }): boolean { return message.content.some( (c) => @@ -80,6 +87,7 @@ export async function handleAgentEvent(host: InteractiveModeStateHost & { lastProcessedContentIndex = 0; lastPinnedText = ""; hasToolsInTurn = false; + renderedSegments = []; if (pinnedBorder) pinnedBorder.stopSpinner(); pinnedBorder = undefined; pinnedTextComponent = undefined; @@ -99,6 +107,7 @@ export async function handleAgentEvent(host: InteractiveModeStateHost & { host.pinnedMessageContainer.clear(); lastPinnedText = ""; hasToolsInTurn = false; + renderedSegments = []; if (pinnedBorder) pinnedBorder.stopSpinner(); pinnedBorder = undefined; pinnedTextComponent = undefined; @@ -273,24 +282,88 @@ export async function handleAgentEvent(host: InteractiveModeStateHost & { } } - // Render assistant text/thinking after tool components so mixed - // streams keep chronological ordering in the chat container. - const hasToolBlocks = hasAssistantToolBlocks(host.streamingMessage); - if (!host.streamingComponent && hasVisibleAssistantContent(host.streamingMessage)) { - host.streamingComponent = new AssistantMessageComponent( - undefined, - host.hideThinkingBlock, - host.getMarkdownThemeWithSettings(), - host.settingsManager.getTimestampFormat(), - ); - host.chatContainer.addChild(host.streamingComponent); - } - if (host.streamingComponent) { - if (hasToolBlocks) { - host.chatContainer.removeChild(host.streamingComponent); - host.chatContainer.addChild(host.streamingComponent); + // Segment walker: render content blocks in stream order, append-only. + // Build desired segment plan from content[]. + { + const blocks = host.streamingMessage.content; + type DesiredSegment = + | { kind: "text-run"; startIndex: number; endIndex: number } + | { kind: "tool"; contentIndex: number; toolId: string }; + const desired: DesiredSegment[] = []; + let runStart = -1; + for (let i = 0; i < blocks.length; i++) { + const b = blocks[i]; + const isText = b.type === "text" || b.type === "thinking"; + const isTool = b.type === "toolCall" || b.type === "serverToolUse"; + if (isText) { + if (runStart === -1) runStart = i; + } else { + if (runStart !== -1) { + desired.push({ kind: "text-run", startIndex: runStart, endIndex: i - 1 }); + runStart = -1; + } + if (isTool) { + desired.push({ kind: "tool", contentIndex: i, toolId: b.id }); + } + } + } + if (runStart !== -1) { + desired.push({ kind: "text-run", startIndex: runStart, endIndex: blocks.length - 1 }); + } + + // Append any newly needed segments (never reorder existing ones). + for (const seg of desired) { + if (seg.kind === "tool") { + // Tool segments are already handled above via pendingTools; just + // register them in renderedSegments if not yet tracked. + const existing = renderedSegments.find( + (s) => s.kind === "tool" && s.contentIndex === seg.contentIndex, + ); + if (!existing) { + const comp = host.pendingTools.get(seg.toolId); + if (comp) { + renderedSegments.push({ kind: "tool", contentIndex: seg.contentIndex, component: comp }); + } + } + } else { + // text-run segment + const existing = renderedSegments.find( + (s) => s.kind === "text-run" && s.startIndex === seg.startIndex, + ); + if (!existing) { + const comp = new AssistantMessageComponent( + undefined, + host.hideThinkingBlock, + host.getMarkdownThemeWithSettings(), + host.settingsManager.getTimestampFormat(), + { startIndex: seg.startIndex, endIndex: seg.endIndex }, + ); + host.chatContainer.addChild(comp); + renderedSegments.push({ kind: "text-run", startIndex: seg.startIndex, endIndex: seg.endIndex, component: comp }); + host.streamingComponent = comp; + } + } + } + + // Update all trailing text-run segments with the latest message so + // streaming text grows in place. + for (const seg of renderedSegments) { + if (seg.kind === "text-run") { + // Find corresponding desired segment to get current endIndex + const d = desired.find((ds) => ds.kind === "text-run" && ds.startIndex === seg.startIndex); + if (d && d.kind === "text-run" && d.endIndex !== seg.endIndex) { + seg.endIndex = d.endIndex; + seg.component.setRange({ startIndex: seg.startIndex, endIndex: seg.endIndex }); + } + seg.component.updateContent(host.streamingMessage); + } + } + + // Keep streamingComponent pointing at the last text-run for message_end compatibility. + const lastTextSeg = [...renderedSegments].reverse().find((s) => s.kind === "text-run"); + if (lastTextSeg && lastTextSeg.kind === "text-run") { + host.streamingComponent = lastTextSeg.component; } - host.streamingComponent.updateContent(host.streamingMessage); } // Update index: fully processed blocks won't need re-scanning. @@ -376,6 +449,7 @@ export async function handleAgentEvent(host: InteractiveModeStateHost & { host.chatContainer.addChild(host.streamingComponent); } if (host.streamingComponent) { + host.streamingComponent.setShowMetadata(true); host.streamingComponent.updateContent(host.streamingMessage); } @@ -399,6 +473,7 @@ export async function handleAgentEvent(host: InteractiveModeStateHost & { } host.streamingComponent = undefined; host.streamingMessage = undefined; + renderedSegments = []; // Clear pinned output once the message is finalized in the chat // container — prevents duplicate display when the agent continues // (e.g. form elicitation) after the assistant message ends.