singularity/singularity-forge
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Merge pull request #4146 from jeremymcs/fix/4144-inline-tool-calls

Browse source 
fix(tui): render assistant tool calls inline with text instead of grouped at end
This commit is contained in:
Jeremy McSpadden 2026-04-13 17:41:41 -05:00 • committed by GitHub
commit 6ba83c83c2
3 changed files with 352 additions and 51 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

206
packages/pi-coding-agent/src/core/chat-controller-ordering.test.ts
View file

@ -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)");
});

88
packages/pi-coding-agent/src/modes/interactive/components/assistant-message.ts
View file

@ -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));
}
}
}
}

109
packages/pi-coding-agent/src/modes/interactive/controllers/chat-controller.ts
View file

@ -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<any> }): 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.