From b1869f570f6d8421066ce8579c35084de56b8524 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 6 Aug 2026 21:17:42 -0700 Subject: [PATCH] Resolve tool result rows by call id instead of tool name Matching a result to its call row by tool name alone breaks the moment two calls to the same tool are in flight together, which parallel sub-agent dispatch does on every turn that fires more than one task call. The newest pending row of that name absorbed whichever result landed first, stranding the others pending and turning later results into orphan rows. An id that matches nothing on the log now answers nothing rather than falling back to the newest same-name row, since every current caller carries a real id. The name-based fallback survives only for callId undefined, which saved history from before ids were threaded through this path still produces. --- src/tui-opentui/diff.ts | 3 + src/tui-opentui/history-hydrate.test.ts | 17 +++++ src/tui-opentui/history-hydrate.ts | 13 ++++ src/tui-opentui/mcp-view.ts | 3 + src/tui-opentui/observe-map.ts | 4 ++ src/tui-opentui/runner-host.test.ts | 3 +- src/tui-opentui/runner-host.ts | 6 +- src/tui-opentui/runtime-bridge.test.ts | 58 ++++++++++++++++ src/tui-opentui/stream.ts | 9 +++ src/tui-opentui/tool-rows.test.ts | 88 ++++++++++++++++++++++++- src/tui-opentui/tool-rows.ts | 28 ++++++-- 11 files changed, 224 insertions(+), 8 deletions(-) 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)