diff --git a/src/tui-opentui/diff.ts b/src/tui-opentui/diff.ts index 241b0522d..9e73839de 100644 --- a/src/tui-opentui/diff.ts +++ b/src/tui-opentui/diff.ts @@ -445,6 +445,8 @@ export type ToolCallRowInput = { readonly name: string /** Raw JSON arguments as streamed by the model; may be absent or partial. */ readonly arguments?: string + /** Runtime call id, when the source (live bridge, saved history) carried one. */ + readonly callId?: string } /** @@ -492,6 +494,7 @@ export function toolCallRow(input: ToolCallRowInput): StreamRow { meta, pending: true, callKey, + ...(input.callId !== undefined ? { callId: input.callId } : {}), ...(diff !== null ? { diff } : {}), ...(verb !== undefined ? { verb } : {}), // A summarised call may deliberately have no subject — its verb already diff --git a/src/tui-opentui/history-hydrate.test.ts b/src/tui-opentui/history-hydrate.test.ts index ab97a5ab2..cdea2cce7 100644 --- a/src/tui-opentui/history-hydrate.test.ts +++ b/src/tui-opentui/history-hydrate.test.ts @@ -208,6 +208,23 @@ describe("hydrateHistoryRows", () => { ]) }) + // CL-5562: a resumed transcript with three parallel `task` dispatches has + // three tool_call blocks that all share name "task" — the callId each + // block carries is what tells them apart on replay. + test("resolves parallel same-name tool_call/tool_result pairs by callId", () => { + const rows = hydrateHistoryRows([ + { type: "tool_call", name: "task", arguments: '{"description":"Fix CL-5559"}', callId: "c1" }, + { type: "tool_call", name: "task", arguments: '{"description":"Fix CL-5560"}', callId: "c2" }, + { type: "tool_call", name: "task", arguments: '{"description":"Fix CL-5561"}', callId: "c3" }, + { type: "tool_result", name: "task", content: "done c2", callId: "c2" }, + { type: "tool_result", name: "task", content: "done c1", callId: "c1" }, + { type: "tool_result", name: "task", content: "done c3", callId: "c3" }, + ]) + expect(rows.length).toBe(3) + expect(rows.every((r) => r.pending !== true)).toBe(true) + expect(rows.map((r) => r.text)).toEqual(["done c1", "done c2", "done c3"]) + }) + test("non-array returns empty", () => { expect(hydrateHistoryRows(undefined)).toEqual([]) expect(hydrateHistoryRows(null)).toEqual([]) diff --git a/src/tui-opentui/history-hydrate.ts b/src/tui-opentui/history-hydrate.ts index 456f6c995..c2457c944 100644 --- a/src/tui-opentui/history-hydrate.ts +++ b/src/tui-opentui/history-hydrate.ts @@ -24,6 +24,13 @@ export type HistoryBlock = { readonly isError?: boolean /** tool_call argument payload when `content` is absent (ContentBlockData). */ readonly arguments?: string + /** + * Call id carried by a `tool_call` / `tool_result` block. Two saved calls to + * the same tool are indistinguishable by name alone — a resumed transcript + * with parallel sub-agent dispatches needs this to pair each result with + * its own call rather than the newest pending call of that name. + */ + readonly callId?: string /** view block payload — validated before it reaches the layout pass. */ readonly node?: unknown /** plan block payload. */ @@ -51,6 +58,7 @@ function asHistoryBlock(raw: unknown): HistoryBlock | null { message?: string isError?: boolean arguments?: string + callId?: string node?: unknown steps?: unknown tasks?: unknown @@ -60,6 +68,7 @@ function asHistoryBlock(raw: unknown): HistoryBlock | null { if (typeof o.message === "string") out.message = o.message if (typeof o.isError === "boolean") out.isError = o.isError if (typeof o.arguments === "string") out.arguments = o.arguments + if (typeof o.callId === "string") out.callId = o.callId if (o.node !== undefined) out.node = o.node if (o.steps !== undefined) out.steps = o.steps if (o.tasks !== undefined) out.tasks = o.tasks @@ -135,6 +144,7 @@ export function rowFromHistoryBlock(block: HistoryBlock): StreamRow | null { return toolCallRow({ name: block.name ?? "tool", ...(args !== undefined ? { arguments: args } : {}), + ...(block.callId !== undefined ? { callId: block.callId } : {}), }) } case "tool_result": @@ -142,6 +152,7 @@ export function rowFromHistoryBlock(block: HistoryBlock): StreamRow | null { name: block.name ?? "tool", content: block.content ?? (block.isError ? "error" : "ok"), isError: block.isError === true, + ...(block.callId !== undefined ? { callId: block.callId } : {}), }) case "view": { const text = viewText(block.node) || block.content?.trim() || "" @@ -211,6 +222,7 @@ function pushHistoryBlock(rows: StreamRow[], block: HistoryBlock): void { pushToolCall(rows, { name: block.name ?? "tool", ...(args !== undefined ? { arguments: args } : {}), + ...(block.callId !== undefined ? { callId: block.callId } : {}), }) return } @@ -219,6 +231,7 @@ function pushHistoryBlock(rows: StreamRow[], block: HistoryBlock): void { name: block.name ?? "tool", content: block.content ?? (block.isError ? "error" : "ok"), isError: block.isError === true, + ...(block.callId !== undefined ? { callId: block.callId } : {}), }) return } diff --git a/src/tui-opentui/mcp-view.ts b/src/tui-opentui/mcp-view.ts index a9afa465e..ba5dbb36f 100644 --- a/src/tui-opentui/mcp-view.ts +++ b/src/tui-opentui/mcp-view.ts @@ -264,6 +264,8 @@ export type ToolResultRowInput = { readonly name: string readonly content: string readonly isError?: boolean + /** Runtime call id this result answers, when the source carried one. */ + readonly callId?: string } /** Bodies at or under this many lines read faster than a sentence about them. */ @@ -487,6 +489,7 @@ export function toolResultRow(input: ToolResultRowInput): StreamRow { role: "tool" as const, text: input.content, meta: input.name, + ...(input.callId !== undefined ? { callId: input.callId } : {}), } if (failed) return { ...base, failed: true } diff --git a/src/tui-opentui/observe-map.ts b/src/tui-opentui/observe-map.ts index 7d82f482f..9b01f934f 100644 --- a/src/tui-opentui/observe-map.ts +++ b/src/tui-opentui/observe-map.ts @@ -33,12 +33,14 @@ export function rowFromBridgeEvent(event: BridgeInboundEvent): StreamRow | null return toolCallRow({ name: event.name, ...(event.detail !== undefined ? { arguments: event.detail } : {}), + ...(event.callId !== undefined ? { callId: event.callId } : {}), }) case "tool_result": return toolResultRow({ name: event.name, content: event.detail ?? (event.isError ? "error" : "ok"), isError: event.isError === true, + ...(event.callId !== undefined ? { callId: event.callId } : {}), }) case "system": return { role: "system", text: event.text } @@ -94,6 +96,7 @@ function pushBridgeEvent( pushToolCall(rows, { name: event.name, ...(event.detail !== undefined ? { arguments: event.detail } : {}), + ...(event.callId !== undefined ? { callId: event.callId } : {}), }) return } @@ -102,6 +105,7 @@ function pushBridgeEvent( name: event.name, content: event.detail ?? (event.isError ? "error" : "ok"), isError: event.isError === true, + ...(event.callId !== undefined ? { callId: event.callId } : {}), }) return } diff --git a/src/tui-opentui/runner-host.test.ts b/src/tui-opentui/runner-host.test.ts index cd9a4d822..9d4f0d926 100644 --- a/src/tui-opentui/runner-host.test.ts +++ b/src/tui-opentui/runner-host.test.ts @@ -72,6 +72,7 @@ describe("rowFromTranscriptEntry", () => { verb: "Grep", pending: true, callKey: "grep Grep ", + callId: "c", }) expect( rowFromTranscriptEntry({ @@ -81,7 +82,7 @@ describe("rowFromTranscriptEntry", () => { content: "boom", isError: true, }), - ).toEqual({ role: "tool", text: "boom", meta: "grep", failed: true }) + ).toEqual({ role: "tool", text: "boom", meta: "grep", failed: true, callId: "c" }) expect(rowFromTranscriptEntry({ kind: "report", content: "done" })).toEqual({ role: "assistant", text: "done", diff --git a/src/tui-opentui/runner-host.ts b/src/tui-opentui/runner-host.ts index a05232ef0..e27b8645e 100644 --- a/src/tui-opentui/runner-host.ts +++ b/src/tui-opentui/runner-host.ts @@ -141,12 +141,13 @@ export function rowFromTranscriptEntry(entry: SubAgentTranscriptEntry): StreamRo case "thinking": return { role: "system", text: entry.content, meta: "thinking" } case "tool": - return toolCallRow({ name: entry.name, arguments: entry.arguments }) + return toolCallRow({ name: entry.name, arguments: entry.arguments, callId: entry.callId }) case "tool_result": return toolResultRow({ name: entry.name, content: entry.content, isError: entry.isError, + callId: entry.callId, }) case "report": return { role: "assistant", text: entry.content, meta: "report" } @@ -164,7 +165,7 @@ export function rowsFromTranscript( const rows: StreamRow[] = [] for (const entry of entries) { if (entry.kind === "tool") { - pushToolCall(rows, { name: entry.name, arguments: entry.arguments }) + pushToolCall(rows, { name: entry.name, arguments: entry.arguments, callId: entry.callId }) continue } if (entry.kind === "tool_result") { @@ -172,6 +173,7 @@ export function rowsFromTranscript( name: entry.name, content: entry.content, isError: entry.isError, + callId: entry.callId, }) continue } diff --git a/src/tui-opentui/runtime-bridge.test.ts b/src/tui-opentui/runtime-bridge.test.ts index ec7f5327d..5ba250cd8 100644 --- a/src/tui-opentui/runtime-bridge.test.ts +++ b/src/tui-opentui/runtime-bridge.test.ts @@ -432,3 +432,61 @@ describe("committed inference retry", () => { ) }) }) + +describe("parallel sub-agent dispatch on the live session bridge", () => { + // The live main-session path tracks a call's row by callId in its own map + // (applyToolCall/applyToolResult), independent of tool-rows.ts's name-based + // pendingCallIndex — this pins that down so a future change to either path + // cannot silently reintroduce CL-5562's misattribution on the parent + // transcript specifically (the observe overlay and resumed history are + // covered separately in tool-rows.test.ts / history-hydrate.test.ts). + test("three parallel task calls resolve to three rows, each with its own result", async () => { + await withTestRenderer( + async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: false, + run: "idle", + }) + const bridge = attachSessionBridge(shell, createRecordingPort()) + try { + const events = [ + { type: "inference.start", data: {} }, + { + type: "inference.tool_call.end", + data: { name: "task", callId: "c1", arguments: { description: "Fix CL-5559" } }, + }, + { + type: "inference.tool_call.end", + data: { name: "task", callId: "c2", arguments: { description: "Fix CL-5560" } }, + }, + { + type: "inference.tool_call.end", + data: { name: "task", callId: "c3", arguments: { description: "Fix CL-5561" } }, + }, + { type: "inference.done", data: {} }, + { type: "tool.start", data: { call: { id: "c1", name: "task" } } }, + { type: "tool.start", data: { call: { id: "c2", name: "task" } } }, + { type: "tool.start", data: { call: { id: "c3", name: "task" } } }, + // Completion order does not follow dispatch order. + { type: "tool.done", data: { result: { callId: "c2", name: "task", content: "done c2" } } }, + { type: "tool.done", data: { result: { callId: "c1", name: "task", content: "done c1" } } }, + { type: "tool.done", data: { result: { callId: "c3", name: "task", content: "done c3" } } }, + { type: "reactor.done", data: {} }, + ] as const + for (const event of events) bridge.handle(event) + + const toolRows = shell.streamLog.filter((r) => r.role === "tool") + expect(toolRows.length).toBe(3) + expect(toolRows.every((r) => r.pending !== true)).toBe(true) + expect(toolRows.every((r) => r.failed !== true)).toBe(true) + expect(toolRows.map((r) => r.text)).toEqual(["done c1", "done c2", "done c3"]) + } finally { + bridge.dispose() + shell.dispose() + } + }, + { width: 80, height: 24 }, + ) + }) +}) diff --git a/src/tui-opentui/stream.ts b/src/tui-opentui/stream.ts index f42873a2c..5b207b2a6 100644 --- a/src/tui-opentui/stream.ts +++ b/src/tui-opentui/stream.ts @@ -71,6 +71,15 @@ export type StreamRow = { * onto a single row. */ readonly callKey?: string + /** + * Runtime id of the call this row answers, when the source carried one + * (a live reactor callId, a resumed transcript's saved id). A result finds + * the exact row it resolves by this id first — the tool name alone is + * ambiguous the moment two calls to the same tool are in flight at once, + * which parallel sub-agent dispatch does on every turn that fires more + * than one `task` call. + */ + readonly callId?: string /** * Row standing for a run of repeated calls. Its subject stays the call the * run repeats (never a total across them, which would be a claim the diff --git a/src/tui-opentui/tool-rows.test.ts b/src/tui-opentui/tool-rows.test.ts index 5307293c4..5dbab1eb4 100644 --- a/src/tui-opentui/tool-rows.test.ts +++ b/src/tui-opentui/tool-rows.test.ts @@ -9,12 +9,13 @@ import { withTestRenderer } from "./harness" import { attachSessionBridge, createRecordingPort } from "./runtime-bridge" import { createAppShell } from "./shell" import { + isCollapsibleRow, paintStreamRow, toolSentenceLines, type RowLayout, type StreamRow, } from "./stream" -import { pushToolCall, pushToolResult } from "./tool-rows" +import { pendingCallIndex, pushToolCall, pushToolResult } from "./tool-rows" const LAYOUT: RowLayout = { width: 72, multiAgent: false } @@ -124,6 +125,91 @@ describe("a run of identical calls", () => { }) }) +describe("parallel calls to the same tool", () => { + // CL-5562: three `task` calls dispatched in one turn all carry + // meta === "task" — name alone cannot tell them apart, so a result must + // find its own row by call id or it resolves whichever pending "task" row + // happens to be newest, leaving the others stranded pending forever and + // turning any later same-name result into an orphaned extra row. + test("each result resolves its own call by id, not the newest pending call of that name", () => { + const rows: StreamRow[] = [] + pushToolCall(rows, { + name: "task", + arguments: JSON.stringify({ agent: "intern", description: "Fix CL-5559 heading shake" }), + callId: "c1", + }) + pushToolCall(rows, { + name: "task", + arguments: JSON.stringify({ agent: "intern", description: "Fix CL-5560 approval UI" }), + callId: "c2", + }) + pushToolCall(rows, { + name: "task", + arguments: JSON.stringify({ agent: "intern", description: "Fix CL-5561 scroll/history" }), + callId: "c3", + }) + expect(rows.length).toBe(3) + + // Results land out of dispatch order, as real sub-agent completion does. + pushToolResult(rows, { name: "task", content: "done c2", callId: "c2" }) + pushToolResult(rows, { name: "task", content: "done c1", callId: "c1" }) + pushToolResult(rows, { name: "task", content: "done c3", callId: "c3" }) + + expect(rows.length).toBe(3) + expect(rows.every((r) => r.pending !== true)).toBe(true) + expect(rows.every((r) => r.failed !== true)).toBe(true) + expect(rows[0]?.summary).toBe("Fix CL-5559 heading shake") + expect(rows[0]?.text).toBe("done c1") + expect(rows[1]?.summary).toBe("Fix CL-5560 approval UI") + expect(rows[1]?.text).toBe("done c2") + expect(rows[2]?.summary).toBe("Fix CL-5561 scroll/history") + expect(rows[2]?.text).toBe("done c3") + }) + + // A miss must not fall back to "the newest pending row of that name" — that + // fallback is exactly the LIFO misattribution this test file exists to rule + // out, and every live caller (the bridge's own call map, subagent session + // entries, resumed history with ids) always carries a real id, so a miss + // here means the id genuinely does not belong to anything on the log. + test("an id that matches nothing on the log answers nothing, not the newest pending call", () => { + const rows: StreamRow[] = [ + { role: "tool", text: "", meta: "task", pending: true, callId: "a1" }, + { role: "tool", text: "", meta: "task", pending: true, callId: "b1" }, + ] + expect(pendingCallIndex(rows, "task", "zzz-does-not-exist")).toBe(-1) + + pushToolResult(rows, { name: "task", content: "orphan", callId: "zzz-does-not-exist" }) + // Answers nothing on the log — appended as its own row rather than + // resolving (and thereby corrupting) an unrelated in-flight call. + expect(rows.length).toBe(3) + expect(rows[0]?.pending).toBe(true) + expect(rows[1]?.pending).toBe(true) + }) + + // Acceptance criterion: a failed sub-agent surfaces its error inline + // (expandable), not a bare mark with nothing behind it. `mergeToolRows` / + // `toolResultRow` already carry the failed result's own text into `detail` + // — untouched by this fix, but only reachable per-call once results resolve + // to the right row instead of a neighbour's. + test("a failed call keeps its error text behind the expand arrow", () => { + const rows: StreamRow[] = [] + pushToolCall(rows, { + name: "task", + arguments: JSON.stringify({ agent: "intern", description: "Fix CL-5559 heading shake" }), + callId: "c1", + }) + pushToolResult(rows, { + name: "task", + content: 'Error: sub-agent "Fix CL-5559 heading shake" failed: boom', + isError: true, + callId: "c1", + }) + expect(rows[0]?.failed).toBe(true) + expect(isCollapsibleRow(rows[0]!)).toBe(true) + expect(rows[0]?.detail?.[0]?.[0]?.text).toContain("boom") + }) +}) + describe("a long subject", () => { test("is cut to one line rather than wrapped", () => { const row = toolCallRow({ diff --git a/src/tui-opentui/tool-rows.ts b/src/tui-opentui/tool-rows.ts index f62e15a64..f3d18636a 100644 --- a/src/tui-opentui/tool-rows.ts +++ b/src/tui-opentui/tool-rows.ts @@ -158,14 +158,34 @@ export function coalesceCallRows(tail: StreamRow, next: StreamRow): StreamRow { } /** - * Index of the call row a result belongs to: the newest unanswered call by the - * same tool, else the newest unanswered call at all. -1 when the result answers - * nothing on the log (a hydrated transcript that kept only results, say). + * Index of the call row a result belongs to. + * + * A carried call id is exact and wins outright — it is the only thing that + * tells two in-flight calls to the same tool apart, which parallel sub-agent + * dispatch produces on every turn that fires more than one `task` call (three + * dispatches all show `meta === "task"`; name alone cannot tell them apart). + * An id that matches nothing on the log still returns -1 rather than falling + * through to the name scan below: every current caller (the live bridge's own + * call map, `SubAgentTranscriptEntry`, `BridgeInboundEvent`) always carries an + * id, so a miss here is a real mismatch, not a legacy record, and papering + * over it with the newest same-name row is the exact misattribution this + * function exists to prevent. + * + * The name scan only runs when `callId` is `undefined` — saved history from + * before ids were threaded through `HistoryBlock` (`history-hydrate.ts`) is + * the one caller that still omits it. */ export function pendingCallIndex( rows: readonly StreamRow[], name: string, + callId?: string, ): number { + if (callId !== undefined) { + for (let i = rows.length - 1; i >= 0; i--) { + if (rows[i]?.callId === callId) return i + } + return -1 + } let fallback = -1 for (let i = rows.length - 1; i >= 0; i--) { const row = rows[i] @@ -196,7 +216,7 @@ export function pushToolResult( input: ToolResultRowInput, ): void { const result = toolResultRow(input) - const index = pendingCallIndex(rows, input.name) + const index = pendingCallIndex(rows, input.name, input.callId) const call = index === -1 ? undefined : rows[index] if (call === undefined) { rows.push(result)