From 3532ea2c6cfe28dbd412dfcb6bd73a43744a64d2 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 8 Aug 2026 09:17:03 -0700 Subject: [PATCH] Canonicalize tool call identity so it settles activeToolCalls once A streamed inference.tool_call.start/end with no callId registered the call under its name, while the executed tool.start for the same call registered it under a real id. One tool.done only removed the id-keyed entry, so the name-keyed duplicate leaked and pinned activeToolCalls above zero forever, blocking inference.done and connector.reply from ever settling the turn. Goal mode surfaced this worst since its self-continuing governor has no other terminator. Identity is now resolved once at the event boundary: the first id seen for a tool name is recorded, so a later id-bearing announcement for the same call replaces an earlier name-only placeholder in place instead of adding a second entry. The mapping is cleared once its call resolves, so a later call reusing the same tool name in one turn starts clean rather than inheriting a finished call's id. --- src/tui-opentui/turn-state.test.ts | 84 ++++++++++++ src/tui-opentui/turn-state.ts | 212 ++++++++++++++++++++++------- 2 files changed, 244 insertions(+), 52 deletions(-) diff --git a/src/tui-opentui/turn-state.test.ts b/src/tui-opentui/turn-state.test.ts index f987c20e5..d2a1722e5 100644 --- a/src/tui-opentui/turn-state.test.ts +++ b/src/tui-opentui/turn-state.test.ts @@ -81,6 +81,90 @@ describe("turnStateFromEvent", () => { ).toBe("grep") }) + test("a call's own name-only start and end announcements do not double-count", () => { + const running = fold([ + { type: "inference.start" }, + { type: "inference.tool_call.start", data: { name: "bash" } }, + { type: "inference.tool_call.end", data: { name: "bash" } }, + ]) + expect(running.activeToolCalls).toHaveLength(1) + }) + + test("a name-only streamed announcement and an id-bearing tool.start for the same call settle on one tool.done", () => { + // Regression for CL-5645: inference.tool_call.start streamed the call + // under its name (no callId yet); tool.start then announced the same + // call under a real id. One tool.done must clear both records, not + // leave a name-keyed duplicate pinning activeToolCalls forever. + const running = fold([ + { type: "inference.start" }, + { type: "inference.tool_call.start", data: { name: "bash" } }, + { type: "tool.start", data: { call: { id: "call_1", name: "bash" } } }, + ]) + expect(running.activeToolCalls).toHaveLength(1) + + const done = turnStateFromEvent( + running, + { type: "tool.done", data: { result: { callId: "call_1" } } }, + 200, + ) + expect(done.activeToolCalls).toHaveLength(0) + + const settled = turnStateFromEvent(done, { type: "inference.done" }, 201) + expect(settled.status).toBe("done") + expect(settled.isProcessing).toBe(false) + }) + + test("two concurrent calls to the same tool resolve independently", () => { + const running = fold([ + { type: "inference.start" }, + { type: "inference.tool_call.start", data: { name: "grep" } }, + { type: "inference.tool_call.start", data: { name: "grep" } }, + { type: "tool.start", data: { call: { id: "call_1", name: "grep" } } }, + { type: "tool.start", data: { call: { id: "call_2", name: "grep" } } }, + ]) + expect(running.activeToolCalls).toHaveLength(2) + + const oneDone = turnStateFromEvent( + running, + { type: "tool.done", data: { result: { callId: "call_1" } } }, + 200, + ) + expect(oneDone.activeToolCalls).toHaveLength(1) + + const bothDone = turnStateFromEvent( + oneDone, + { type: "tool.done", data: { result: { callId: "call_2" } } }, + 201, + ) + expect(bothDone.activeToolCalls).toHaveLength(0) + }) + + test("a second call to the same tool name does not inherit a finished call's id", () => { + const firstDone = fold([ + { type: "inference.start" }, + { type: "inference.tool_call.start", data: { name: "bash" } }, + { type: "tool.start", data: { call: { id: "call_1", name: "bash" } } }, + { type: "tool.done", data: { result: { callId: "call_1" } } }, + ]) + expect(firstDone.activeToolCalls).toHaveLength(0) + + const secondRunning = [ + { type: "inference.tool_call.start", data: { name: "bash" } }, + { type: "tool.start", data: { call: { id: "call_2", name: "bash" } } }, + ].reduce( + (state, event, i) => turnStateFromEvent(state, event, 100 + i), + firstDone, + ) + expect(secondRunning.activeToolCalls).toEqual(["call_2"]) + + const secondDone = turnStateFromEvent( + secondRunning, + { type: "tool.done", data: { result: { callId: "call_2" } } }, + 200, + ) + expect(secondDone.activeToolCalls).toHaveLength(0) + }) + test("reactor.done settles back to idle", () => { const s = fold([{ type: "inference.start" }, { type: "reactor.done" }]) expect(s.status).toBe("idle") diff --git a/src/tui-opentui/turn-state.ts b/src/tui-opentui/turn-state.ts index d0cb54cc8..9a0fcad24 100644 --- a/src/tui-opentui/turn-state.ts +++ b/src/tui-opentui/turn-state.ts @@ -96,6 +96,12 @@ export type TurnState = { * so the settle decision needs the outstanding ids, not just the last name. */ readonly activeToolCalls: readonly string[] + /** + * Real id for a tool name once one has been seen this turn, so a + * name-only announcement and its later id-bearing counterpart collapse + * onto one `activeToolCalls` entry. See `registerActiveCall`. + */ + readonly callIdByName: Readonly> /** * Tail of the text/thinking output streamed in the current uninterrupted * streaming cycle. A tool call ends the cycle and clears it: a model @@ -148,6 +154,7 @@ export function initialTurnState(nowMs: number): TurnState { lastActivityAt: nowMs, quota: null, activeToolCalls: [], + callIdByName: {}, streamText: "", streamCharsSeen: 0, repetitionCheckedAt: 0, @@ -170,6 +177,7 @@ export function turnStateOnSubmit(state: TurnState, nowMs: number): TurnState { streamTokenCount: 0, lastActivityAt: nowMs, activeToolCalls: [], + callIdByName: {}, streamText: "", streamCharsSeen: 0, repetitionCheckedAt: 0, @@ -215,11 +223,6 @@ function deltaText(event: { readonly data?: unknown; readonly text?: string }): return event.text ?? "" } -const namedCallData = type({ "name?": "string" }) -const toolStartData = type({ - call: { "name?": "string" }, -}) - function quotaFromInferenceError( data: unknown, nowMs: number, @@ -231,49 +234,45 @@ function quotaFromInferenceError( return { retryAfterMs, retryAt: nowMs + retryAfterMs } } -function toolName(data: unknown): string | null { - const named = namedCallData(data) - if (!(named instanceof type.errors) && named.name !== undefined) { - return named.name - } - const started = toolStartData(data) - if (!(started instanceof type.errors) && started.call.name !== undefined) { - return started.call.name +type CallIdentity = { readonly id?: string; readonly name?: string } + +// Both flat streamed shapes (`{ callId?, name? }`) and the nested tool.start +// shape (`{ call: { id?, callId?, name? } }`) are parsed here so every call +// site — the tool name shown in the UI and the activeToolCalls bookkeeping — +// reads one identity off one parse, instead of two schemas that could drift. +const callEventData = type({ + "callId?": "string", + "name?": "string", + "call?": { "id?": "string", "callId?": "string", "name?": "string" }, +}) + +function streamedCallIdentity(data: unknown): CallIdentity { + const parsed = callEventData(data) + if (parsed instanceof type.errors) return {} + const id = parsed.callId ?? parsed.call?.id ?? parsed.call?.callId + const name = parsed.name ?? parsed.call?.name + return { + ...(id !== undefined ? { id } : {}), + ...(name !== undefined ? { name } : {}), } - return null } -const callIdData = type({ "callId?": "string", "name?": "string" }) -const toolStartCallData = type({ - call: { "id?": "string", "callId?": "string", "name?": "string" }, -}) +function toolName(data: unknown): string | null { + return streamedCallIdentity(data).name ?? null +} + const toolDoneData = type({ result: { "callId?": "string", "name?": "string" }, }) -/** - * Stable handle for one outstanding tool call. Providers that stream a callId - * give a real one; the rest fall back to the name so at least the count is - * right, which is all the settle decision reads. - */ -function streamedCallId(data: unknown): string { - const parsed = callIdData(data) - if (!(parsed instanceof type.errors)) { - if (parsed.callId !== undefined) return parsed.callId - if (parsed.name !== undefined) return parsed.name - } - const started = toolStartCallData(data) - if (!(started instanceof type.errors)) { - const { id, callId, name } = started.call - return id ?? callId ?? name ?? "tool" - } - return "tool" -} - -function resultCallId(data: unknown): string { +function resultIdentity(data: unknown): CallIdentity { const parsed = toolDoneData(data) - if (parsed instanceof type.errors) return "tool" - return parsed.result.callId ?? parsed.result.name ?? "tool" + if (parsed instanceof type.errors) return {} + const { callId, name } = parsed.result + return { + ...(callId !== undefined ? { id: callId } : {}), + ...(name !== undefined ? { name } : {}), + } } function withActiveCall( @@ -296,6 +295,110 @@ function withoutActiveCall( return active.slice(1) } +type CallTracking = { + readonly activeToolCalls: readonly string[] + /** + * Real id for a tool name once one has been seen. A name-only announcement + * (start/end with no callId) and the id-bearing tool.start for the same + * call share this mapping so the second collapses onto the first entry + * instead of adding a duplicate. Two concurrent calls to the same tool + * still collide here — the event stream carries no signal to tell them + * apart until both have real ids — but that ambiguity predates this fix: + * the original name-keyed tracking collapsed them identically. + */ + readonly callIdByName: Readonly> +} + +/** + * Canonicalize one logical call's identity at the event boundary: a + * name-only announcement (no callId yet) and a later id-bearing one for the + * same call must collapse onto a single activeToolCalls entry, not two. + */ +function registerActiveCall( + tracking: CallTracking, + identity: CallIdentity, +): CallTracking { + const { activeToolCalls, callIdByName } = tracking + + if (identity.id !== undefined) { + const nextCallIdByName = + identity.name !== undefined + ? { ...callIdByName, [identity.name]: identity.id } + : callIdByName + // A provisional entry may already be tracking this call under its name — + // promote it onto the real id in place instead of adding a duplicate. + const withoutPlaceholder = + identity.name !== undefined && activeToolCalls.includes(identity.name) + ? activeToolCalls.filter((c) => c !== identity.name) + : activeToolCalls + return { + activeToolCalls: withActiveCall(withoutPlaceholder, identity.id), + callIdByName: nextCallIdByName, + } + } + + if (identity.name !== undefined) { + const id = callIdByName[identity.name] ?? identity.name + return { activeToolCalls: withActiveCall(activeToolCalls, id), callIdByName } + } + + return { activeToolCalls: withActiveCall(activeToolCalls, "tool"), callIdByName } +} + +function withoutCallIdByName( + callIdByName: Readonly>, + name: string, +): Readonly> { + if (!(name in callIdByName)) return callIdByName + return Object.fromEntries( + Object.entries(callIdByName).filter(([n]) => n !== name), + ) +} + +/** + * Which tool name (if any) maps to this id — tool.done rarely carries the + * name itself, so resolving the id back to its name is the only way to clear + * a finished call's entry without depending on the result payload's shape. + */ +function nameForCallId( + callIdByName: Readonly>, + id: string, +): string | undefined { + return Object.entries(callIdByName).find(([, v]) => v === id)?.[0] +} + +function unregisterActiveCall( + tracking: CallTracking, + identity: CallIdentity, +): CallTracking { + const { activeToolCalls, callIdByName } = tracking + + if (identity.id !== undefined) { + // Clear the mapping once its call resolves, or a later call reusing the + // same tool name would resolve straight to this now-finished id instead + // of tracking its own — reproducing the leak this function exists to fix. + const resolvedName = identity.name ?? nameForCallId(callIdByName, identity.id) + const nextCallIdByName = + resolvedName !== undefined + ? withoutCallIdByName(callIdByName, resolvedName) + : callIdByName + return { + activeToolCalls: withoutActiveCall(activeToolCalls, identity.id), + callIdByName: nextCallIdByName, + } + } + + if (identity.name !== undefined) { + const id = callIdByName[identity.name] ?? identity.name + return { + activeToolCalls: withoutActiveCall(activeToolCalls, id), + callIdByName: withoutCallIdByName(callIdByName, identity.name), + } + } + + return { activeToolCalls: withoutActiveCall(activeToolCalls, "tool"), callIdByName } +} + const streaming = ( state: TurnState, kind: "text" | "thinking", @@ -432,14 +535,10 @@ export function turnStateFromEvent( case "inference.tool_call.start": case "inference.tool_call.end": case "tool.start": { - const running = runningTool(state, toolName(event.data), nowMs) - return { - ...running, - activeToolCalls: withActiveCall( - state.activeToolCalls, - streamedCallId(event.data), - ), - } + const identity = streamedCallIdentity(event.data) + const running = runningTool(state, identity.name ?? null, nowMs) + const tracking = registerActiveCall(running, identity) + return { ...running, ...tracking } } case "tool_call": { @@ -455,7 +554,18 @@ export function turnStateFromEvent( // Tool finished: the model is being called again, so the awaiting-response // clock restarts rather than the tool clock continuing. - case "tool.done": + case "tool.done": { + const tracking = unregisterActiveCall(state, resultIdentity(event.data)) + return { + ...state, + ...tracking, + awaitingResponse: true, + streamingType: null, + currentToolName: null, + lastActivityAt: nowMs, + } + } + case "tool_result": return { ...state, @@ -465,9 +575,7 @@ export function turnStateFromEvent( lastActivityAt: nowMs, activeToolCalls: withoutActiveCall( state.activeToolCalls, - event.type === "tool.done" - ? resultCallId(event.data) - : (event.name ?? "tool"), + event.name ?? "tool", ), }