From 801ec8aee56f275ddefa7384e0103f886886f554 Mon Sep 17 00:00:00 2001 From: awschmeder Date: Tue, 23 Jun 2026 09:55:48 -0700 Subject: [PATCH 01/10] fix: streaming tool-call argument loss in NativeToolCallParser (#695) --- .../fix-toolcall-dropped-leading-deltas.md | 5 + prs/fix-toolcall-dropped-leading-deltas.md | 39 +++++ .../assistant-message/NativeToolCallParser.ts | 33 ++-- .../__tests__/NativeToolCallParser.spec.ts | 144 +++++++++++++++++- .../native-tools/ask_followup_question.ts | 4 +- 5 files changed, 207 insertions(+), 18 deletions(-) create mode 100644 .changeset/fix-toolcall-dropped-leading-deltas.md create mode 100644 prs/fix-toolcall-dropped-leading-deltas.md diff --git a/.changeset/fix-toolcall-dropped-leading-deltas.md b/.changeset/fix-toolcall-dropped-leading-deltas.md new file mode 100644 index 0000000000..d4bedb753d --- /dev/null +++ b/.changeset/fix-toolcall-dropped-leading-deltas.md @@ -0,0 +1,5 @@ +--- +"zoo-code": patch +--- + +Fix streaming tool-call arguments being dropped when argument deltas arrive before the tool-call id, which caused spurious "missing required parameter" errors (affects LiteLLM, OpenAI-compatible, and DeepSeek providers). diff --git a/prs/fix-toolcall-dropped-leading-deltas.md b/prs/fix-toolcall-dropped-leading-deltas.md new file mode 100644 index 0000000000..a464ba5eb2 --- /dev/null +++ b/prs/fix-toolcall-dropped-leading-deltas.md @@ -0,0 +1,39 @@ +### Related GitHub Issue + +Closes: #695 + +### Description + +When a provider streams a tool call whose first delta(s) arrive _before_ the tool-call `id` is known, those leading argument bytes are silently discarded by `NativeToolCallParser.processRawChunk`. This causes downstream "missing required parameter" errors even when the model supplied the data. + +This PR fixes the issue by centralizing the tracking of streaming tool calls in `NativeToolCallParser`. The `rawChunkTracker` is now initialized on the first sight of a stream `index`, independent of whether an `id` is present. All `arguments` deltas are buffered until both `id` and `name` are known, ensuring no data loss during streaming reassembly. + +### Test Procedure + +1. Ran the newly added unit test in `src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts` which verifies that leading argument bytes arriving before the `id` are correctly preserved and finalized. +2. Verified that existing provider tests in the same test file pass. + +### Pre-Submission Checklist + +- [x] **Issue Linked**: This PR is linked to an approved GitHub Issue. +- [x] **Scope**: My changes are focused on the linked issue (one major feature/fix per PR). +- [x] **Self-Review**: I have performed a thorough self-review of my code. +- [x] **Testing**: New and/or updated tests have been added to cover my changes. +- [x] **Documentation Impact**: I have considered if my changes require documentation updates. +- [x] **Contribution Guidelines**: I have read and agree to the [Contributor Guidelines](/CONTRIBUTING.md). + +### Screenshots / Videos + +N/A + +### Documentation Updates + +- [x] No documentation updates are required. + +### Additional Notes + +N/A + +### Get in Touch + +@awschmeder diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index 10a4446182..0dab1b265e 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/src/core/assistant-message/NativeToolCallParser.ts @@ -61,7 +61,7 @@ export class NativeToolCallParser { // Raw chunk tracking state (keyed by index from one API stream) private static rawChunkTrackersByScope = new WeakMap< object, - Map + Map >() public static createScope(): object { @@ -124,8 +124,10 @@ export class NativeToolCallParser { let tracked = rawChunkTracker.get(index) - // Initialize new tool call tracking when we receive an id - if (id && !tracked) { + // Create the tracker on first sight of this index, independent of whether + // an id has arrived yet. Keying the lifecycle by index (not id) ensures any + // `arguments` that stream before the id is known are buffered rather than dropped. + if (!tracked) { tracked = { id, name: name || "", @@ -135,38 +137,39 @@ export class NativeToolCallParser { rawChunkTracker.set(index, tracked) } - if (!tracked) { - return events + // Record id and name as they arrive (they may come in separate chunks). + if (id) { + tracked.id = id } - - // Update name if present in chunk and not yet set if (name) { tracked.name = name } - // Emit start event when we have the name - if (!tracked.hasStarted && tracked.name) { + // Emit start event only once both id and name are known. Using a local + // non-null id keeps emitted events typed as id: string. + if (!tracked.hasStarted && tracked.id && tracked.name) { + const startedId = tracked.id events.push({ type: "tool_call_start", - id: tracked.id, + id: startedId, name: tracked.name, }) tracked.hasStarted = true - // Flush buffered deltas + // Flush buffered deltas accumulated during the pre-start window. for (const bufferedDelta of tracked.deltaBuffer) { events.push({ type: "tool_call_delta", - id: tracked.id, + id: startedId, delta: bufferedDelta, }) } tracked.deltaBuffer = [] } - // Emit delta event for argument chunks + // Emit delta event for argument chunks, buffering until start is emitted. if (args) { - if (tracked.hasStarted) { + if (tracked.hasStarted && tracked.id) { events.push({ type: "tool_call_delta", id: tracked.id, @@ -190,7 +193,7 @@ export class NativeToolCallParser { if (rawChunkTracker) { for (const [, tracked] of rawChunkTracker.entries()) { - if (tracked.hasStarted) { + if (tracked.hasStarted && tracked.id) { events.push({ type: "tool_call_end", id: tracked.id, diff --git a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts index 47ea2a36cc..28a40cb487 100644 --- a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts +++ b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts @@ -1,4 +1,4 @@ -import { NativeToolCallParser } from "../NativeToolCallParser" +import { NativeToolCallParser, type ToolCallStreamEvent } from "../NativeToolCallParser" describe("NativeToolCallParser", () => { describe("parseToolCall", () => { @@ -450,4 +450,146 @@ describe("NativeToolCallParser", () => { }) }) }) + + describe("processRawChunk streaming reassembly", () => { + // Mirror the sequencing Task.ts performs: feed each raw chunk through + // processRawChunk, drive startStreamingToolCall on tool_call_start, feed + // tool_call_delta into processStreamingChunk, and finalize at the end. + // Returns the ordered event types/ids plus the finalized tool uses by id. + const drive = ( + rawChunks: Array<{ index: number; id?: string; name?: string; arguments?: string }>, + finishReason: string | null = "tool_calls", + ) => { + const events: ToolCallStreamEvent[] = [] + + const handleEvent = (event: ToolCallStreamEvent) => { + events.push(event) + if (event.type === "tool_call_start") { + NativeToolCallParser.startStreamingToolCall(event.id, event.name) + } else if (event.type === "tool_call_delta") { + NativeToolCallParser.processStreamingChunk(event.id, event.delta) + } + } + + for (const chunk of rawChunks) { + for (const event of NativeToolCallParser.processRawChunk(chunk)) { + handleEvent(event) + } + } + + // Task.ts emits ends via processFinishReason on finish_reason: "tool_calls". + // Use clearRawChunkState (not finalizeRawChunks) for cleanup so we don't + // double-count the end events both paths would produce. + for (const event of NativeToolCallParser.processFinishReason(finishReason)) { + handleEvent(event) + } + NativeToolCallParser.clearRawChunkState() + + const finalized = new Map>() + const startIds = events.filter((e) => e.type === "tool_call_start").map((e) => e.id) + for (const id of startIds) { + finalized.set(id, NativeToolCallParser.finalizeStreamingToolCall(id)) + } + + return { events, finalized } + } + + it("preserves leading argument bytes that arrive before the id", () => { + // First chunk carries arguments but NO id; id+name arrive later, then more args. + const fullArgs = JSON.stringify({ path: "src/leading.ts", mode: "slice" }) + const firstHalf = fullArgs.slice(0, 10) + const secondHalf = fullArgs.slice(10) + + const { events, finalized } = drive([ + { index: 0, arguments: firstHalf }, + { index: 0, id: "call_late_id", name: "read_file" }, + { index: 0, arguments: secondHalf }, + ]) + + // Exactly one start, in the right order, with the late id. + const starts = events.filter((e) => e.type === "tool_call_start") + expect(starts).toHaveLength(1) + expect(starts[0].id).toBe("call_late_id") + + // The finalized arguments must contain the complete, uncorrupted payload. + const result = finalized.get("call_late_id") + expect(result).not.toBeNull() + expect(result?.type).toBe("tool_use") + if (result?.type === "tool_use") { + const nativeArgs = result.nativeArgs as { path: string; mode?: string } + expect(nativeArgs.path).toBe("src/leading.ts") + expect(nativeArgs.mode).toBe("slice") + } + }) + + it("handles id and name arriving in separate chunks (issue #218)", () => { + const fullArgs = JSON.stringify({ path: "src/split.ts" }) + + const { events, finalized } = drive([ + { index: 0, id: "call_split" }, + { index: 0, name: "read_file" }, + { index: 0, arguments: fullArgs }, + ]) + + const starts = events.filter((e) => e.type === "tool_call_start") + expect(starts).toHaveLength(1) + expect(starts[0].id).toBe("call_split") + + const result = finalized.get("call_split") + expect(result?.type).toBe("tool_use") + if (result?.type === "tool_use") { + const nativeArgs = result.nativeArgs as { path: string } + expect(nativeArgs.path).toBe("src/split.ts") + } + }) + + it("keeps two parallel tool calls on distinct indices isolated", () => { + const argsA = JSON.stringify({ path: "src/a.ts" }) + const argsB = JSON.stringify({ path: "src/b.ts" }) + + const { events, finalized } = drive([ + { index: 0, arguments: argsA.slice(0, 8) }, + { index: 1, arguments: argsB.slice(0, 8) }, + { index: 0, id: "call_a", name: "read_file" }, + { index: 1, id: "call_b", name: "read_file" }, + { index: 0, arguments: argsA.slice(8) }, + { index: 1, arguments: argsB.slice(8) }, + ]) + + const starts = events.filter((e) => e.type === "tool_call_start") + expect(starts).toHaveLength(2) + + const resultA = finalized.get("call_a") + const resultB = finalized.get("call_b") + if (resultA?.type === "tool_use") { + expect((resultA.nativeArgs as { path: string }).path).toBe("src/a.ts") + } + if (resultB?.type === "tool_use") { + expect((resultB.nativeArgs as { path: string }).path).toBe("src/b.ts") + } + }) + + it("emits the same event sequence for the single-chunk-with-id flow (regression guard)", () => { + const fullArgs = JSON.stringify({ path: "src/single.ts" }) + + const { events, finalized } = drive([ + { index: 0, id: "call_single", name: "read_file", arguments: fullArgs }, + ]) + + expect(events.map((e) => e.type)).toEqual(["tool_call_start", "tool_call_delta", "tool_call_end"]) + expect(events.every((e) => e.id === "call_single")).toBe(true) + + const result = finalized.get("call_single") + if (result?.type === "tool_use") { + expect((result.nativeArgs as { path: string }).path).toBe("src/single.ts") + } + }) + + it("does not emit a phantom tool_call_end for a tracker that never received an id", () => { + const { events } = drive([{ index: 0, arguments: '{"path":"orphan.ts"}' }]) + + expect(events.filter((e) => e.type === "tool_call_start")).toHaveLength(0) + expect(events.filter((e) => e.type === "tool_call_end")).toHaveLength(0) + }) + }) }) diff --git a/src/core/prompts/tools/native-tools/ask_followup_question.ts b/src/core/prompts/tools/native-tools/ask_followup_question.ts index b0591206ad..8f064eb20b 100644 --- a/src/core/prompts/tools/native-tools/ask_followup_question.ts +++ b/src/core/prompts/tools/native-tools/ask_followup_question.ts @@ -4,7 +4,7 @@ const ASK_FOLLOWUP_QUESTION_DESCRIPTION = `Ask the user a question to gather add Parameters: - question: (required) A clear, specific question addressing the information needed -- follow_up: (required) A list of 2-4 suggested answers. Suggestions must be complete, actionable answers without placeholders. Optionally include mode to switch modes (code/architect/etc.) +- follow_up: (required) An array of 1-4 suggested answers. Always provide this as an array, even when there is only one suggestion. Each suggestion must be a complete, actionable answer without placeholders. Suggestions optionally include mode to switch modes (code/architect/etc.) Example: Asking for file path { "question": "What is the path to the frontend-config.json file?", "follow_up": [{ "text": "./src/frontend-config.json", "mode": null }, { "text": "./config/frontend-config.json", "mode": null }, { "text": "./frontend-config.json", "mode": null }] } @@ -14,7 +14,7 @@ Example: Asking with mode switch const QUESTION_PARAMETER_DESCRIPTION = `Clear, specific question that captures the missing information you need` -const FOLLOW_UP_PARAMETER_DESCRIPTION = `Required list of 2-4 suggested responses; each suggestion must be a complete, actionable answer and may include a mode switch` +const FOLLOW_UP_PARAMETER_DESCRIPTION = `Required array of 1-4 suggested responses, always an array even for a single suggestion; each suggestion must be a complete, actionable answer and may include a mode switch` const FOLLOW_UP_TEXT_DESCRIPTION = `Suggested answer the user can pick` From 3eed2255199ca1d3e25de98e2c851822d4ee2c90 Mon Sep 17 00:00:00 2001 From: awschmeder Date: Tue, 23 Jun 2026 11:35:03 -0700 Subject: [PATCH 02/10] chore: remove agent-generated changeset file per policy --- .changeset/fix-toolcall-dropped-leading-deltas.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/fix-toolcall-dropped-leading-deltas.md diff --git a/.changeset/fix-toolcall-dropped-leading-deltas.md b/.changeset/fix-toolcall-dropped-leading-deltas.md deleted file mode 100644 index d4bedb753d..0000000000 --- a/.changeset/fix-toolcall-dropped-leading-deltas.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"zoo-code": patch ---- - -Fix streaming tool-call arguments being dropped when argument deltas arrive before the tool-call id, which caused spurious "missing required parameter" errors (affects LiteLLM, OpenAI-compatible, and DeepSeek providers). From 98b19322d0d81c373bc001219c7b8990d323f2c8 Mon Sep 17 00:00:00 2001 From: awschmeder Date: Tue, 23 Jun 2026 11:40:04 -0700 Subject: [PATCH 03/10] test: add direct coverage for finalizeRawChunks() in NativeToolCallParser --- .../__tests__/NativeToolCallParser.spec.ts | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts index 28a40cb487..245ae74198 100644 --- a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts +++ b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts @@ -591,5 +591,60 @@ describe("NativeToolCallParser", () => { expect(events.filter((e) => e.type === "tool_call_start")).toHaveLength(0) expect(events.filter((e) => e.type === "tool_call_end")).toHaveLength(0) }) + + it("finalizeRawChunks() emits end events and guards against missing id", () => { + // Simulate a started tool call: process chunks to populate state + const chunks = [ + { index: 0, id: "call_finalize", name: "read_file" }, + { index: 0, arguments: '{"path":"file.ts"' }, + { index: 0, arguments: ',"mode":"slice"}' }, + ] + + const events: Array<{ type: string; id?: string }> = [] + for (const chunk of chunks) { + for (const event of NativeToolCallParser.processRawChunk(chunk)) { + events.push(event) + if (event.type === "tool_call_start") { + NativeToolCallParser.startStreamingToolCall(event.id, event.name) + } else if (event.type === "tool_call_delta") { + NativeToolCallParser.processStreamingChunk(event.id, event.delta) + } + } + } + + // Now finalize the raw chunks to emit the end event + const finalizeEvents = NativeToolCallParser.finalizeRawChunks() + for (const event of finalizeEvents) { + events.push(event) + } + + // Verify the end event was produced by finalizeRawChunks + const ends = events.filter((e) => e.type === "tool_call_end") + expect(ends).toHaveLength(1) + expect(ends[0].id).toBe("call_finalize") + + // Finalize the tool call to ensure it contains the complete arguments + const result = NativeToolCallParser.finalizeStreamingToolCall("call_finalize") + expect(result?.type).toBe("tool_use") + if (result?.type === "tool_use") { + expect((result.nativeArgs as { path: string }).path).toBe("file.ts") + } + }) + + it("finalizeRawChunks() does not emit end for tracker without id", () => { + // Start a tracker with arguments but no id, then finalize + const chunks = [{ index: 0, arguments: '{"incomplete":true}' }] + + for (const chunk of chunks) { + NativeToolCallParser.processRawChunk(chunk) + } + + // Finalize should not emit an end event if id was never set + const finalizeEvents = NativeToolCallParser.finalizeRawChunks() + const ends = finalizeEvents.filter((e) => e.type === "tool_call_end") + expect(ends).toHaveLength(0) + + NativeToolCallParser.clearRawChunkState() + }) }) }) From f0a986047b1271391f1c1e683522feff0babc45e Mon Sep 17 00:00:00 2001 From: awschmeder Date: Fri, 26 Jun 2026 12:32:31 -0700 Subject: [PATCH 04/10] fix: address PR #700 review feedback for tool-call streaming reassembly - Guard finalize results with not.toBeNull() in parallel-index and single-chunk tests so a null result fails instead of passing silently - Add reverse-ordering test (name -> buffered args -> id) covering the start-gate id requirement - Use name !== undefined recording plus a nameSeen flag in the start-gate as a defensive guard against an empty tool name - Clear rawChunkTracker in processFinishReason so finalizeRawChunks is a safe no-op; add a regression test asserting no double tool_call_end - Remove unrelated ask_followup_question wording change from PR scope - Remove prs/fix-toolcall-dropped-leading-deltas.md from the diff --- prs/fix-toolcall-dropped-leading-deltas.md | 39 ------------- .../assistant-message/NativeToolCallParser.ts | 18 +++++- .../__tests__/NativeToolCallParser.spec.ts | 56 +++++++++++++++++++ .../native-tools/ask_followup_question.ts | 4 +- 4 files changed, 73 insertions(+), 44 deletions(-) delete mode 100644 prs/fix-toolcall-dropped-leading-deltas.md diff --git a/prs/fix-toolcall-dropped-leading-deltas.md b/prs/fix-toolcall-dropped-leading-deltas.md deleted file mode 100644 index a464ba5eb2..0000000000 --- a/prs/fix-toolcall-dropped-leading-deltas.md +++ /dev/null @@ -1,39 +0,0 @@ -### Related GitHub Issue - -Closes: #695 - -### Description - -When a provider streams a tool call whose first delta(s) arrive _before_ the tool-call `id` is known, those leading argument bytes are silently discarded by `NativeToolCallParser.processRawChunk`. This causes downstream "missing required parameter" errors even when the model supplied the data. - -This PR fixes the issue by centralizing the tracking of streaming tool calls in `NativeToolCallParser`. The `rawChunkTracker` is now initialized on the first sight of a stream `index`, independent of whether an `id` is present. All `arguments` deltas are buffered until both `id` and `name` are known, ensuring no data loss during streaming reassembly. - -### Test Procedure - -1. Ran the newly added unit test in `src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts` which verifies that leading argument bytes arriving before the `id` are correctly preserved and finalized. -2. Verified that existing provider tests in the same test file pass. - -### Pre-Submission Checklist - -- [x] **Issue Linked**: This PR is linked to an approved GitHub Issue. -- [x] **Scope**: My changes are focused on the linked issue (one major feature/fix per PR). -- [x] **Self-Review**: I have performed a thorough self-review of my code. -- [x] **Testing**: New and/or updated tests have been added to cover my changes. -- [x] **Documentation Impact**: I have considered if my changes require documentation updates. -- [x] **Contribution Guidelines**: I have read and agree to the [Contributor Guidelines](/CONTRIBUTING.md). - -### Screenshots / Videos - -N/A - -### Documentation Updates - -- [x] No documentation updates are required. - -### Additional Notes - -N/A - -### Get in Touch - -@awschmeder diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index 0dab1b265e..122579c2a0 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/src/core/assistant-message/NativeToolCallParser.ts @@ -61,7 +61,17 @@ export class NativeToolCallParser { // Raw chunk tracking state (keyed by index from one API stream) private static rawChunkTrackersByScope = new WeakMap< object, - Map + Map< + number, + { + id?: string + name: string + // Track whether the provider sent a name, including an empty name. + nameSeen: boolean + hasStarted: boolean + deltaBuffer: string[] + } + > >() public static createScope(): object { @@ -131,6 +141,7 @@ export class NativeToolCallParser { tracked = { id, name: name || "", + nameSeen: name !== undefined, hasStarted: false, deltaBuffer: [], } @@ -141,13 +152,14 @@ export class NativeToolCallParser { if (id) { tracked.id = id } - if (name) { + if (name !== undefined) { tracked.name = name + tracked.nameSeen = true } // Emit start event only once both id and name are known. Using a local // non-null id keeps emitted events typed as id: string. - if (!tracked.hasStarted && tracked.id && tracked.name) { + if (!tracked.hasStarted && tracked.id && tracked.nameSeen) { const startedId = tracked.id events.push({ type: "tool_call_start", diff --git a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts index 245ae74198..bad3abd0a9 100644 --- a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts +++ b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts @@ -543,6 +543,36 @@ describe("NativeToolCallParser", () => { } }) + it("handles name arriving before id with buffered args in between (reverse ordering)", () => { + const fullArgs = JSON.stringify({ path: "src/reverse.ts" }) + const firstHalf = fullArgs.slice(0, 9) + const secondHalf = fullArgs.slice(9) + + const { events, finalized } = drive([ + { index: 0, name: "read_file" }, + { index: 0, arguments: firstHalf }, + { index: 0, id: "call_reverse" }, + { index: 0, arguments: secondHalf }, + ]) + + // Start must not fire until the id arrives, so exactly one start with the late id. + const starts = events.filter((e) => e.type === "tool_call_start") + expect(starts).toHaveLength(1) + expect(starts[0].id).toBe("call_reverse") + + // The buffered delta must be flushed only after the start event. + const startIndex = events.findIndex((e) => e.type === "tool_call_start") + const firstDeltaIndex = events.findIndex((e) => e.type === "tool_call_delta") + expect(startIndex).toBeLessThan(firstDeltaIndex) + + const result = finalized.get("call_reverse") + expect(result).not.toBeNull() + expect(result?.type).toBe("tool_use") + if (result?.type === "tool_use") { + expect((result.nativeArgs as { path: string }).path).toBe("src/reverse.ts") + } + }) + it("keeps two parallel tool calls on distinct indices isolated", () => { const argsA = JSON.stringify({ path: "src/a.ts" }) const argsB = JSON.stringify({ path: "src/b.ts" }) @@ -561,6 +591,8 @@ describe("NativeToolCallParser", () => { const resultA = finalized.get("call_a") const resultB = finalized.get("call_b") + expect(resultA).not.toBeNull() + expect(resultB).not.toBeNull() if (resultA?.type === "tool_use") { expect((resultA.nativeArgs as { path: string }).path).toBe("src/a.ts") } @@ -580,6 +612,8 @@ describe("NativeToolCallParser", () => { expect(events.every((e) => e.id === "call_single")).toBe(true) const result = finalized.get("call_single") + expect(result).not.toBeNull() + expect(result?.type).toBe("tool_use") if (result?.type === "tool_use") { expect((result.nativeArgs as { path: string }).path).toBe("src/single.ts") } @@ -646,5 +680,27 @@ describe("NativeToolCallParser", () => { NativeToolCallParser.clearRawChunkState() }) + + it("does not double-fire end events across processFinishReason and finalizeRawChunks", () => { + // Drive a started tool call through the raw chunk path. + const chunks = [ + { index: 0, id: "call_dup", name: "read_file" }, + { index: 0, arguments: '{"path":"file.ts"}' }, + ] + for (const chunk of chunks) { + NativeToolCallParser.processRawChunk(chunk) + } + + // Task.ts emits ends via processFinishReason, then calls finalizeRawChunks + // unconditionally. Both must not emit an end for the same tracker. + const finishEvents = NativeToolCallParser.processFinishReason("tool_calls") + const finalizeEvents = NativeToolCallParser.finalizeRawChunks() + + const allEnds = [...finishEvents, ...finalizeEvents].filter((e) => e.type === "tool_call_end") + expect(allEnds).toHaveLength(1) + expect(allEnds[0].id).toBe("call_dup") + + NativeToolCallParser.clearRawChunkState() + }) }) }) diff --git a/src/core/prompts/tools/native-tools/ask_followup_question.ts b/src/core/prompts/tools/native-tools/ask_followup_question.ts index 8f064eb20b..b0591206ad 100644 --- a/src/core/prompts/tools/native-tools/ask_followup_question.ts +++ b/src/core/prompts/tools/native-tools/ask_followup_question.ts @@ -4,7 +4,7 @@ const ASK_FOLLOWUP_QUESTION_DESCRIPTION = `Ask the user a question to gather add Parameters: - question: (required) A clear, specific question addressing the information needed -- follow_up: (required) An array of 1-4 suggested answers. Always provide this as an array, even when there is only one suggestion. Each suggestion must be a complete, actionable answer without placeholders. Suggestions optionally include mode to switch modes (code/architect/etc.) +- follow_up: (required) A list of 2-4 suggested answers. Suggestions must be complete, actionable answers without placeholders. Optionally include mode to switch modes (code/architect/etc.) Example: Asking for file path { "question": "What is the path to the frontend-config.json file?", "follow_up": [{ "text": "./src/frontend-config.json", "mode": null }, { "text": "./config/frontend-config.json", "mode": null }, { "text": "./frontend-config.json", "mode": null }] } @@ -14,7 +14,7 @@ Example: Asking with mode switch const QUESTION_PARAMETER_DESCRIPTION = `Clear, specific question that captures the missing information you need` -const FOLLOW_UP_PARAMETER_DESCRIPTION = `Required array of 1-4 suggested responses, always an array even for a single suggestion; each suggestion must be a complete, actionable answer and may include a mode switch` +const FOLLOW_UP_PARAMETER_DESCRIPTION = `Required list of 2-4 suggested responses; each suggestion must be a complete, actionable answer and may include a mode switch` const FOLLOW_UP_TEXT_DESCRIPTION = `Suggested answer the user can pick` From 060f714afd30ef128f6e961dd1bf4f2f8cb9c9af Mon Sep 17 00:00:00 2001 From: awschmeder Date: Fri, 26 Jun 2026 13:20:12 -0700 Subject: [PATCH 05/10] fix: handle provider-emitted tool_call_end chunks during streaming Task.ts had no stream-level case for tool_call_end, so end chunks emitted by providers on finish_reason: "tool_calls" were silently dropped; tool calls only finalized at stream end via finalizeRawChunks(). Add a tool_call_end case so tools finalize and present during streaming, and extract the triplicated finalize/present logic into a shared idempotent helper. Correct the NativeToolCallParser test drive helper to finalize via finalizeRawChunks() (matching production) instead of processFinishReason(). --- .../__tests__/NativeToolCallParser.spec.ts | 16 +-- src/core/task/Task.ts | 110 +++++++++--------- 2 files changed, 58 insertions(+), 68 deletions(-) diff --git a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts index bad3abd0a9..30b3a4d6ff 100644 --- a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts +++ b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts @@ -454,12 +454,10 @@ describe("NativeToolCallParser", () => { describe("processRawChunk streaming reassembly", () => { // Mirror the sequencing Task.ts performs: feed each raw chunk through // processRawChunk, drive startStreamingToolCall on tool_call_start, feed - // tool_call_delta into processStreamingChunk, and finalize at the end. + // tool_call_delta into processStreamingChunk, and emit ends at stream close + // via finalizeRawChunks() (the same call Task.ts makes after the stream ends). // Returns the ordered event types/ids plus the finalized tool uses by id. - const drive = ( - rawChunks: Array<{ index: number; id?: string; name?: string; arguments?: string }>, - finishReason: string | null = "tool_calls", - ) => { + const drive = (rawChunks: Array<{ index: number; id?: string; name?: string; arguments?: string }>) => { const events: ToolCallStreamEvent[] = [] const handleEvent = (event: ToolCallStreamEvent) => { @@ -477,13 +475,11 @@ describe("NativeToolCallParser", () => { } } - // Task.ts emits ends via processFinishReason on finish_reason: "tool_calls". - // Use clearRawChunkState (not finalizeRawChunks) for cleanup so we don't - // double-count the end events both paths would produce. - for (const event of NativeToolCallParser.processFinishReason(finishReason)) { + // Task.ts finalizes any tool calls still open at stream end via + // finalizeRawChunks(), which emits the tool_call_end events. + for (const event of NativeToolCallParser.finalizeRawChunks()) { handleEvent(event) } - NativeToolCallParser.clearRawChunkState() const finalized = new Map>() const startIds = events.filter((e) => e.type === "tool_call_start").map((e) => e.id) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index d5313f68cf..b56d6fe3a5 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -500,6 +500,46 @@ export class Task extends EventEmitter implements TaskLike { // Native tool call streaming state (track which index each tool is at) private streamingToolCallIndices: Map = new Map() + /** + * Finalize a streaming native tool call by id and present it. + * + * Shared by every site that observes a tool_call_end: the per-chunk event + * loop, the stream-level tool_call_end case, and the end-of-stream + * finalizeRawChunks() pass. Calling it again for an already-finalized id is a + * safe no-op because finalizeStreamingToolCall() and the index map entry are + * both cleared on first finalize. + */ + private finalizeStreamingToolCallById(id: string, nativeToolCallParserScope: object): void { + const finalToolUse = NativeToolCallParser.finalizeStreamingToolCall(id, nativeToolCallParserScope) + const toolUseIndex = this.streamingToolCallIndices.get(id) + + if (finalToolUse) { + ;(finalToolUse as any).id = id + if (toolUseIndex !== undefined) { + this.assistantMessageContent[toolUseIndex] = finalToolUse + } + this.streamingToolCallIndices.delete(id) + this.userMessageContentReady = false + /* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */ + this.presentAssistantMessageSafe() + } else if (toolUseIndex !== undefined) { + // finalizeStreamingToolCall returned null (malformed JSON or missing args). + // Clear partial arguments before presentation so truncated values cannot run + // or enter conversation history. + const existingToolUse = this.assistantMessageContent[toolUseIndex] + if (existingToolUse && existingToolUse.type === "tool_use") { + existingToolUse.partial = false + existingToolUse.nativeArgs = undefined + existingToolUse.params = {} + ;(existingToolUse as any).id = id + } + this.streamingToolCallIndices.delete(id) + this.userMessageContentReady = false + /* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */ + this.presentAssistantMessageSafe() + } + } + // Cached model info for current streaming session (set at start of each API request) // This prevents excessive getModel() calls during tool execution cachedStreamingModel?: { id: string; info: ModelInfo } @@ -3426,11 +3466,22 @@ export class Task extends EventEmitter implements TaskLike { this.presentAssistantMessageSafe() } } + } else if (event.type === "tool_call_end") { + this.finalizeStreamingToolCallById(event.id, nativeToolCallParserScope) } } break } + case "tool_call_end": { + // Providers emit a tool_call_end chunk when finish_reason is + // "tool_calls" (either directly or via processFinishReason). + // Finalize the streaming tool call now so it is presented during + // streaming rather than waiting for finalizeRawChunks() at stream end. + this.finalizeStreamingToolCallById(chunk.id, nativeToolCallParserScope) + break + } + case "tool_call": { // Legacy: Handle complete tool calls (for backward compatibility) // Convert native tool call to ToolUse format @@ -3798,64 +3849,7 @@ export class Task extends EventEmitter implements TaskLike { const finalizeEvents = NativeToolCallParser.finalizeRawChunks(nativeToolCallParserScope) for (const event of finalizeEvents) { if (event.type === "tool_call_end") { - // Finalize the streaming tool call - const finalToolUse = NativeToolCallParser.finalizeStreamingToolCall( - event.id, - nativeToolCallParserScope, - ) - - // Get the index for this tool call - const toolUseIndex = this.streamingToolCallIndices.get(event.id) - - if (finalToolUse) { - // Store the tool call ID - ;(finalToolUse as any).id = event.id - - // Get the index and replace partial with final - if (toolUseIndex !== undefined) { - this.assistantMessageContent[toolUseIndex] = finalToolUse - } - - // Clean up tracking - this.streamingToolCallIndices.delete(event.id) - - // Mark that we have new content to process - this.userMessageContentReady = false - - // Present the finalized tool call - /* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */ - this.presentAssistantMessageSafe() - } else if (toolUseIndex !== undefined) { - // finalizeStreamingToolCall returned null (malformed JSON or missing args). - // existingToolUse is the same object the streaming phase was mutating in - // place, so it still carries nativeArgs AND params built from the incomplete - // partial parse (e.g. a truncated write_to_file `content` string) - both were - // only ever meant for live progress display, never for execution or for - // ending up in conversation history. Mark the tool as non-partial so it's - // presented as complete, and clear both so presentAssistantMessage's - // `!block.nativeArgs` guard short-circuits with a structured tool_result - // instead of executing the truncated value, and so the toolUse.nativeArgs || - // toolUse.params fallback used when recording history doesn't fall through to - // the same truncated data under a different name. - const existingToolUse = this.assistantMessageContent[toolUseIndex] - if (existingToolUse && existingToolUse.type === "tool_use") { - existingToolUse.partial = false - existingToolUse.nativeArgs = undefined - existingToolUse.params = {} - // Ensure it has the ID for native protocol - ;(existingToolUse as any).id = event.id - } - - // Clean up tracking - this.streamingToolCallIndices.delete(event.id) - - // Mark that we have new content to process - this.userMessageContentReady = false - - // Present the tool call - validation will handle missing params - /* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */ - this.presentAssistantMessageSafe() - } + this.finalizeStreamingToolCallById(event.id, nativeToolCallParserScope) } } From 48f73b19d50f01b45d18a2410d1ffd127380d544 Mon Sep 17 00:00:00 2001 From: awschmeder Date: Fri, 26 Jun 2026 18:08:30 -0700 Subject: [PATCH 06/10] test: cover Task.finalizeStreamingToolCallById streaming finalization Add a focused spec that invokes the real Task.prototype.finalizeStreamingToolCallById via .call() with mocked presentAssistantMessage and NativeToolCallParser, covering the success, null-finalize (malformed JSON), untracked-id no-op, and idempotent re-finalize paths. Closes the codecov/patch gap on the new helper. --- .../finalizeStreamingToolCallById.spec.ts | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts diff --git a/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts b/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts new file mode 100644 index 0000000000..da1872ab15 --- /dev/null +++ b/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts @@ -0,0 +1,118 @@ +// npx vitest run core/task/__tests__/finalizeStreamingToolCallById.spec.ts + +import { Task } from "../Task" +import { presentAssistantMessage } from "../../assistant-message" +import { NativeToolCallParser } from "../../assistant-message/NativeToolCallParser" +import type { ToolUse } from "../../../shared/tools" + +// presentAssistantMessage is invoked by finalizeStreamingToolCallById to flush the +// finalized tool use; mocking it isolates the helper from the full presentation pipeline. +vi.mock("../../assistant-message", async (importOriginal) => { + const actual = (await importOriginal()) as Record + return { + ...actual, + presentAssistantMessage: vi.fn(), + } +}) + +const mockedPresent = vi.mocked(presentAssistantMessage) + +/** + * Invoke the private finalizeStreamingToolCallById against a minimal `this` stub. + * + * Instantiating a full Task requires a provider, context, and async setup that are + * irrelevant to this helper. The method only touches assistantMessageContent, + * streamingToolCallIndices, and userMessageContentReady, so a stub carrying those + * fields exercises the real source lines without the constructor. + */ +function callFinalize( + stub: { + assistantMessageContent: any[] + streamingToolCallIndices: Map + userMessageContentReady: boolean + }, + id: string, +): void { + ;(Task.prototype as any).finalizeStreamingToolCallById.call(stub, id) +} + +describe("Task.finalizeStreamingToolCallById", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("replaces the partial block with the finalized tool use and presents it", () => { + const finalToolUse = { type: "tool_use", name: "read_file", partial: false } as unknown as ToolUse + const finalizeSpy = vi + .spyOn(NativeToolCallParser, "finalizeStreamingToolCall") + .mockReturnValue(finalToolUse) + + const stub = { + assistantMessageContent: [{ type: "tool_use", id: "call_abc", name: "read_file", partial: true }], + streamingToolCallIndices: new Map([["call_abc", 0]]), + userMessageContentReady: true, + } + + callFinalize(stub, "call_abc") + + expect(finalizeSpy).toHaveBeenCalledWith("call_abc") + expect(stub.assistantMessageContent[0]).toBe(finalToolUse) + expect((stub.assistantMessageContent[0] as any).id).toBe("call_abc") + expect(stub.streamingToolCallIndices.has("call_abc")).toBe(false) + expect(stub.userMessageContentReady).toBe(false) + expect(mockedPresent).toHaveBeenCalledTimes(1) + }) + + it("marks the existing block non-partial when finalize returns null (malformed JSON)", () => { + vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall").mockReturnValue(null) + + const existingBlock = { type: "tool_use", id: "call_bad", name: "write_to_file", partial: true } + const stub = { + assistantMessageContent: [existingBlock], + streamingToolCallIndices: new Map([["call_bad", 0]]), + userMessageContentReady: true, + } + + callFinalize(stub, "call_bad") + + expect(existingBlock.partial).toBe(false) + expect((existingBlock as any).id).toBe("call_bad") + expect(stub.streamingToolCallIndices.has("call_bad")).toBe(false) + expect(stub.userMessageContentReady).toBe(false) + expect(mockedPresent).toHaveBeenCalledTimes(1) + }) + + it("is a no-op when the id is not tracked", () => { + vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall").mockReturnValue(null) + + const stub = { + assistantMessageContent: [] as any[], + streamingToolCallIndices: new Map(), + userMessageContentReady: true, + } + + callFinalize(stub, "call_unknown") + + expect(stub.assistantMessageContent).toHaveLength(0) + expect(stub.userMessageContentReady).toBe(true) + expect(mockedPresent).not.toHaveBeenCalled() + }) + + it("is idempotent: a second call for the same id does nothing", () => { + const finalToolUse = { type: "tool_use", name: "read_file", partial: false } as unknown as ToolUse + vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall") + .mockReturnValueOnce(finalToolUse) + .mockReturnValue(null) + + const stub = { + assistantMessageContent: [{ type: "tool_use", id: "call_once", name: "read_file", partial: true }], + streamingToolCallIndices: new Map([["call_once", 0]]), + userMessageContentReady: true, + } + + callFinalize(stub, "call_once") + callFinalize(stub, "call_once") // id no longer tracked -> no-op + + expect(mockedPresent).toHaveBeenCalledTimes(1) + }) +}) From d42923475e5939f3169e118564cf42f0caff34a2 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Sun, 27 Sep 2026 03:45:30 +0000 Subject: [PATCH 07/10] test: adapt tool-call streaming coverage to scoped parser state --- .../__tests__/NativeToolCallParser.spec.ts | 47 ++++++----- .../finalizeStreamingToolCallById.spec.ts | 81 ++++++++++--------- 2 files changed, 68 insertions(+), 60 deletions(-) diff --git a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts index 30b3a4d6ff..05cec513b8 100644 --- a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts +++ b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts @@ -395,7 +395,10 @@ describe("NativeToolCallParser", () => { ).toEqual([]) expect( NativeToolCallParser.processRawChunk({ index: 0, id: "call_reprobe", name: "read_file" }, firstScope), - ).toEqual([{ type: "tool_call_start", id: "call_reprobe", name: "read_file" }]) + ).toEqual([ + { type: "tool_call_start", id: "call_reprobe", name: "read_file" }, + { type: "tool_call_delta", id: "call_reprobe", delta: "ignored-after-cleanup" }, + ]) }) describe("read_file tool", () => { @@ -459,32 +462,33 @@ describe("NativeToolCallParser", () => { // Returns the ordered event types/ids plus the finalized tool uses by id. const drive = (rawChunks: Array<{ index: number; id?: string; name?: string; arguments?: string }>) => { const events: ToolCallStreamEvent[] = [] + const scope = NativeToolCallParser.createScope() const handleEvent = (event: ToolCallStreamEvent) => { events.push(event) if (event.type === "tool_call_start") { - NativeToolCallParser.startStreamingToolCall(event.id, event.name) + NativeToolCallParser.startStreamingToolCall(event.id, event.name, scope) } else if (event.type === "tool_call_delta") { - NativeToolCallParser.processStreamingChunk(event.id, event.delta) + NativeToolCallParser.processStreamingChunk(event.id, event.delta, scope) } } for (const chunk of rawChunks) { - for (const event of NativeToolCallParser.processRawChunk(chunk)) { + for (const event of NativeToolCallParser.processRawChunk(chunk, scope)) { handleEvent(event) } } // Task.ts finalizes any tool calls still open at stream end via // finalizeRawChunks(), which emits the tool_call_end events. - for (const event of NativeToolCallParser.finalizeRawChunks()) { + for (const event of NativeToolCallParser.finalizeRawChunks(scope)) { handleEvent(event) } const finalized = new Map>() const startIds = events.filter((e) => e.type === "tool_call_start").map((e) => e.id) for (const id of startIds) { - finalized.set(id, NativeToolCallParser.finalizeStreamingToolCall(id)) + finalized.set(id, NativeToolCallParser.finalizeStreamingToolCall(id, scope)) } return { events, finalized } @@ -623,6 +627,7 @@ describe("NativeToolCallParser", () => { }) it("finalizeRawChunks() emits end events and guards against missing id", () => { + const scope = NativeToolCallParser.createScope() // Simulate a started tool call: process chunks to populate state const chunks = [ { index: 0, id: "call_finalize", name: "read_file" }, @@ -632,18 +637,18 @@ describe("NativeToolCallParser", () => { const events: Array<{ type: string; id?: string }> = [] for (const chunk of chunks) { - for (const event of NativeToolCallParser.processRawChunk(chunk)) { + for (const event of NativeToolCallParser.processRawChunk(chunk, scope)) { events.push(event) if (event.type === "tool_call_start") { - NativeToolCallParser.startStreamingToolCall(event.id, event.name) + NativeToolCallParser.startStreamingToolCall(event.id, event.name, scope) } else if (event.type === "tool_call_delta") { - NativeToolCallParser.processStreamingChunk(event.id, event.delta) + NativeToolCallParser.processStreamingChunk(event.id, event.delta, scope) } } } // Now finalize the raw chunks to emit the end event - const finalizeEvents = NativeToolCallParser.finalizeRawChunks() + const finalizeEvents = NativeToolCallParser.finalizeRawChunks(scope) for (const event of finalizeEvents) { events.push(event) } @@ -654,7 +659,7 @@ describe("NativeToolCallParser", () => { expect(ends[0].id).toBe("call_finalize") // Finalize the tool call to ensure it contains the complete arguments - const result = NativeToolCallParser.finalizeStreamingToolCall("call_finalize") + const result = NativeToolCallParser.finalizeStreamingToolCall("call_finalize", scope) expect(result?.type).toBe("tool_use") if (result?.type === "tool_use") { expect((result.nativeArgs as { path: string }).path).toBe("file.ts") @@ -662,41 +667,41 @@ describe("NativeToolCallParser", () => { }) it("finalizeRawChunks() does not emit end for tracker without id", () => { + const scope = NativeToolCallParser.createScope() // Start a tracker with arguments but no id, then finalize const chunks = [{ index: 0, arguments: '{"incomplete":true}' }] for (const chunk of chunks) { - NativeToolCallParser.processRawChunk(chunk) + NativeToolCallParser.processRawChunk(chunk, scope) } // Finalize should not emit an end event if id was never set - const finalizeEvents = NativeToolCallParser.finalizeRawChunks() + const finalizeEvents = NativeToolCallParser.finalizeRawChunks(scope) const ends = finalizeEvents.filter((e) => e.type === "tool_call_end") expect(ends).toHaveLength(0) - NativeToolCallParser.clearRawChunkState() + NativeToolCallParser.clearRawChunkState(scope) }) - it("does not double-fire end events across processFinishReason and finalizeRawChunks", () => { + it("does not double-fire end events across repeated finalizeRawChunks calls", () => { + const scope = NativeToolCallParser.createScope() // Drive a started tool call through the raw chunk path. const chunks = [ { index: 0, id: "call_dup", name: "read_file" }, { index: 0, arguments: '{"path":"file.ts"}' }, ] for (const chunk of chunks) { - NativeToolCallParser.processRawChunk(chunk) + NativeToolCallParser.processRawChunk(chunk, scope) } - // Task.ts emits ends via processFinishReason, then calls finalizeRawChunks - // unconditionally. Both must not emit an end for the same tracker. - const finishEvents = NativeToolCallParser.processFinishReason("tool_calls") - const finalizeEvents = NativeToolCallParser.finalizeRawChunks() + const finishEvents = NativeToolCallParser.finalizeRawChunks(scope) + const finalizeEvents = NativeToolCallParser.finalizeRawChunks(scope) const allEnds = [...finishEvents, ...finalizeEvents].filter((e) => e.type === "tool_call_end") expect(allEnds).toHaveLength(1) expect(allEnds[0].id).toBe("call_dup") - NativeToolCallParser.clearRawChunkState() + NativeToolCallParser.clearRawChunkState(scope) }) }) }) diff --git a/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts b/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts index da1872ab15..19e382d9ed 100644 --- a/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts +++ b/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts @@ -1,21 +1,17 @@ // npx vitest run core/task/__tests__/finalizeStreamingToolCallById.spec.ts import { Task } from "../Task" -import { presentAssistantMessage } from "../../assistant-message" import { NativeToolCallParser } from "../../assistant-message/NativeToolCallParser" import type { ToolUse } from "../../../shared/tools" -// presentAssistantMessage is invoked by finalizeStreamingToolCallById to flush the -// finalized tool use; mocking it isolates the helper from the full presentation pipeline. -vi.mock("../../assistant-message", async (importOriginal) => { - const actual = (await importOriginal()) as Record - return { - ...actual, - presentAssistantMessage: vi.fn(), - } -}) +type FinalizeStub = { + assistantMessageContent: ToolUse[] + streamingToolCallIndices: Map + userMessageContentReady: boolean + presentAssistantMessageSafe: ReturnType +} -const mockedPresent = vi.mocked(presentAssistantMessage) +type FinalizeMethod = (this: FinalizeStub, id: string, scope: object) => void /** * Invoke the private finalizeStreamingToolCallById against a minimal `this` stub. @@ -25,15 +21,10 @@ const mockedPresent = vi.mocked(presentAssistantMessage) * streamingToolCallIndices, and userMessageContentReady, so a stub carrying those * fields exercises the real source lines without the constructor. */ -function callFinalize( - stub: { - assistantMessageContent: any[] - streamingToolCallIndices: Map - userMessageContentReady: boolean - }, - id: string, -): void { - ;(Task.prototype as any).finalizeStreamingToolCallById.call(stub, id) +function callFinalize(stub: FinalizeStub, id: string, scope: object): void { + const finalize = (Task.prototype as unknown as { finalizeStreamingToolCallById: FinalizeMethod }) + .finalizeStreamingToolCallById + finalize.call(stub, id, scope) } describe("Task.finalizeStreamingToolCallById", () => { @@ -42,64 +33,75 @@ describe("Task.finalizeStreamingToolCallById", () => { }) it("replaces the partial block with the finalized tool use and presents it", () => { - const finalToolUse = { type: "tool_use", name: "read_file", partial: false } as unknown as ToolUse - const finalizeSpy = vi - .spyOn(NativeToolCallParser, "finalizeStreamingToolCall") - .mockReturnValue(finalToolUse) + const scope = NativeToolCallParser.createScope() + const finalToolUse: ToolUse = { type: "tool_use", name: "read_file", params: {}, partial: false } + const finalizeSpy = vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall").mockReturnValue(finalToolUse) const stub = { assistantMessageContent: [{ type: "tool_use", id: "call_abc", name: "read_file", partial: true }], streamingToolCallIndices: new Map([["call_abc", 0]]), userMessageContentReady: true, + presentAssistantMessageSafe: vi.fn(), } - callFinalize(stub, "call_abc") + callFinalize(stub, "call_abc", scope) - expect(finalizeSpy).toHaveBeenCalledWith("call_abc") + expect(finalizeSpy).toHaveBeenCalledWith("call_abc", scope) expect(stub.assistantMessageContent[0]).toBe(finalToolUse) - expect((stub.assistantMessageContent[0] as any).id).toBe("call_abc") + expect(stub.assistantMessageContent[0].id).toBe("call_abc") expect(stub.streamingToolCallIndices.has("call_abc")).toBe(false) expect(stub.userMessageContentReady).toBe(false) - expect(mockedPresent).toHaveBeenCalledTimes(1) + expect(stub.presentAssistantMessageSafe).toHaveBeenCalledTimes(1) }) it("marks the existing block non-partial when finalize returns null (malformed JSON)", () => { + const scope = NativeToolCallParser.createScope() vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall").mockReturnValue(null) - const existingBlock = { type: "tool_use", id: "call_bad", name: "write_to_file", partial: true } + const existingBlock: ToolUse = { + type: "tool_use", + id: "call_bad", + name: "write_to_file", + params: {}, + partial: true, + } const stub = { assistantMessageContent: [existingBlock], streamingToolCallIndices: new Map([["call_bad", 0]]), userMessageContentReady: true, + presentAssistantMessageSafe: vi.fn(), } - callFinalize(stub, "call_bad") + callFinalize(stub, "call_bad", scope) expect(existingBlock.partial).toBe(false) - expect((existingBlock as any).id).toBe("call_bad") + expect(existingBlock.id).toBe("call_bad") expect(stub.streamingToolCallIndices.has("call_bad")).toBe(false) expect(stub.userMessageContentReady).toBe(false) - expect(mockedPresent).toHaveBeenCalledTimes(1) + expect(stub.presentAssistantMessageSafe).toHaveBeenCalledTimes(1) }) it("is a no-op when the id is not tracked", () => { + const scope = NativeToolCallParser.createScope() vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall").mockReturnValue(null) const stub = { - assistantMessageContent: [] as any[], + assistantMessageContent: [] as ToolUse[], streamingToolCallIndices: new Map(), userMessageContentReady: true, + presentAssistantMessageSafe: vi.fn(), } - callFinalize(stub, "call_unknown") + callFinalize(stub, "call_unknown", scope) expect(stub.assistantMessageContent).toHaveLength(0) expect(stub.userMessageContentReady).toBe(true) - expect(mockedPresent).not.toHaveBeenCalled() + expect(stub.presentAssistantMessageSafe).not.toHaveBeenCalled() }) it("is idempotent: a second call for the same id does nothing", () => { - const finalToolUse = { type: "tool_use", name: "read_file", partial: false } as unknown as ToolUse + const scope = NativeToolCallParser.createScope() + const finalToolUse: ToolUse = { type: "tool_use", name: "read_file", params: {}, partial: false } vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall") .mockReturnValueOnce(finalToolUse) .mockReturnValue(null) @@ -108,11 +110,12 @@ describe("Task.finalizeStreamingToolCallById", () => { assistantMessageContent: [{ type: "tool_use", id: "call_once", name: "read_file", partial: true }], streamingToolCallIndices: new Map([["call_once", 0]]), userMessageContentReady: true, + presentAssistantMessageSafe: vi.fn(), } - callFinalize(stub, "call_once") - callFinalize(stub, "call_once") // id no longer tracked -> no-op + callFinalize(stub, "call_once", scope) + callFinalize(stub, "call_once", scope) // id no longer tracked -> no-op - expect(mockedPresent).toHaveBeenCalledTimes(1) + expect(stub.presentAssistantMessageSafe).toHaveBeenCalledTimes(1) }) }) From 974b1f7ed71c60d4dc20d027476e33cc5340457d Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Sun, 27 Sep 2026 16:34:54 +0000 Subject: [PATCH 08/10] test: type finalizeStreamingToolCallById fixtures as FinalizeStub --- .../__tests__/NativeToolCallParser.spec.ts | 67 ++++++++++++++++++- src/core/task/__tests__/Task.spec.ts | 41 ++++++++++++ .../finalizeStreamingToolCallById.spec.ts | 14 ++-- 3 files changed, 115 insertions(+), 7 deletions(-) diff --git a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts index 05cec513b8..ee79d7963f 100644 --- a/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts +++ b/src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts @@ -289,6 +289,69 @@ describe("NativeToolCallParser", () => { }) describe("processStreamingChunk", () => { + it("preserves read_file arguments that arrive before the call id and name", () => { + const scope = NativeToolCallParser.createScope() + const argumentsJson = JSON.stringify({ path: "src/leading.ts", mode: "slice", offset: 1, limit: 2000 }) + const split = 24 + + expect( + NativeToolCallParser.processRawChunk({ index: 0, arguments: argumentsJson.slice(0, split) }, scope), + ).toEqual([]) + + const identifiedEvents = NativeToolCallParser.processRawChunk( + { index: 0, id: "call_late_identity", name: "read_file" }, + scope, + ) + expect(identifiedEvents).toEqual([ + { type: "tool_call_start", id: "call_late_identity", name: "read_file" }, + { + type: "tool_call_delta", + id: "call_late_identity", + delta: argumentsJson.slice(0, split), + }, + ]) + + NativeToolCallParser.startStreamingToolCall("call_late_identity", "read_file", scope) + for (const event of identifiedEvents) { + if (event.type === "tool_call_delta") { + NativeToolCallParser.processStreamingChunk(event.id, event.delta, scope) + } + } + + const trailingEvents = NativeToolCallParser.processRawChunk( + { index: 0, arguments: argumentsJson.slice(split) }, + scope, + ) + expect(trailingEvents).toEqual([ + { + type: "tool_call_delta", + id: "call_late_identity", + delta: argumentsJson.slice(split), + }, + ]) + for (const event of trailingEvents) { + if (event.type === "tool_call_delta") { + NativeToolCallParser.processStreamingChunk(event.id, event.delta, scope) + } + } + + expect(NativeToolCallParser.finalizeRawChunks(scope)).toEqual([ + { type: "tool_call_end", id: "call_late_identity" }, + ]) + const result = NativeToolCallParser.finalizeStreamingToolCall("call_late_identity", scope) + expect(result?.type).toBe("tool_use") + if (result?.type === "tool_use") { + expect(result.nativeArgs).toEqual({ path: "src/leading.ts", mode: "slice", offset: 1, limit: 2000 }) + } + }) + + it("does not emit an end event for argument chunks that never receive an identity", () => { + const scope = NativeToolCallParser.createScope() + NativeToolCallParser.processRawChunk({ index: 0, arguments: '{"path":"orphan.ts"}' }, scope) + + expect(NativeToolCallParser.finalizeRawChunks(scope)).toEqual([]) + }) + it("retains peer calls until each call in a scope is finalized", () => { const scope = NativeToolCallParser.createScope() NativeToolCallParser.startStreamingToolCall("call_first", "read_file", scope) @@ -391,13 +454,13 @@ describe("NativeToolCallParser", () => { expect(NativeToolCallParser.finalizeRawChunks(firstScope)).toEqual([]) expect(NativeToolCallParser.finalizeStreamingToolCall("call_first", firstScope)).toBeNull() expect( - NativeToolCallParser.processRawChunk({ index: 0, arguments: "ignored-after-cleanup" }, firstScope), + NativeToolCallParser.processRawChunk({ index: 0, arguments: "buffered-after-cleanup" }, firstScope), ).toEqual([]) expect( NativeToolCallParser.processRawChunk({ index: 0, id: "call_reprobe", name: "read_file" }, firstScope), ).toEqual([ { type: "tool_call_start", id: "call_reprobe", name: "read_file" }, - { type: "tool_call_delta", id: "call_reprobe", delta: "ignored-after-cleanup" }, + { type: "tool_call_delta", id: "call_reprobe", delta: "buffered-after-cleanup" }, ]) }) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index eae85cf9f2..a2dff7baae 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -576,6 +576,47 @@ describe("Cline", () => { }) describe("native tool-call request isolation", () => { + it("reassembles Astra-style read_file arguments that arrive before tool identity", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "late tool identity test", + startTask: false, + }) + + vi.spyOn(task.diffViewProvider, "reset").mockResolvedValue(undefined) + vi.spyOn(getTaskTestAccess(task), "safeEnsureModelFetched").mockResolvedValue(stubModelInfo) + vi.spyOn(getTaskTestAccess(task), "presentAssistantMessageSafe").mockImplementation(() => {}) + vi.spyOn(task, "attemptApiRequest").mockImplementation(() => + asyncStreamFrom([ + { type: "tool_call_partial", index: 0, arguments: '{"path":"scripts/final-review-' }, + { type: "tool_call_partial", index: 0, id: "call_astra_read", name: "read_file" }, + { + type: "tool_call_partial", + index: 0, + arguments: 'smoke/driver.mts","mode":"slice","offset":1,"limit":2000}', + }, + ]), + ) + + await task.recursivelyMakeClineRequests([{ type: "text", text: "review the driver" }]) + + const assistantMessage = task.apiConversationHistory.find((message) => message.role === "assistant") + expect(assistantMessage?.content).toEqual([ + { + type: "tool_use", + id: "call_astra_read", + name: "read_file", + input: { + path: "scripts/final-review-smoke/driver.mts", + mode: "slice", + offset: 1, + limit: 2000, + }, + }, + ]) + }) + it("keeps overlapping Task parser state scoped to each request", async () => { const firstTask = new Task({ provider: mockProvider, diff --git a/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts b/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts index 19e382d9ed..fecbdd8506 100644 --- a/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts +++ b/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts @@ -37,8 +37,10 @@ describe("Task.finalizeStreamingToolCallById", () => { const finalToolUse: ToolUse = { type: "tool_use", name: "read_file", params: {}, partial: false } const finalizeSpy = vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall").mockReturnValue(finalToolUse) - const stub = { - assistantMessageContent: [{ type: "tool_use", id: "call_abc", name: "read_file", partial: true }], + const stub: FinalizeStub = { + assistantMessageContent: [ + { type: "tool_use", id: "call_abc", name: "read_file", params: {}, partial: true }, + ], streamingToolCallIndices: new Map([["call_abc", 0]]), userMessageContentReady: true, presentAssistantMessageSafe: vi.fn(), @@ -65,7 +67,7 @@ describe("Task.finalizeStreamingToolCallById", () => { params: {}, partial: true, } - const stub = { + const stub: FinalizeStub = { assistantMessageContent: [existingBlock], streamingToolCallIndices: new Map([["call_bad", 0]]), userMessageContentReady: true, @@ -106,8 +108,10 @@ describe("Task.finalizeStreamingToolCallById", () => { .mockReturnValueOnce(finalToolUse) .mockReturnValue(null) - const stub = { - assistantMessageContent: [{ type: "tool_use", id: "call_once", name: "read_file", partial: true }], + const stub: FinalizeStub = { + assistantMessageContent: [ + { type: "tool_use", id: "call_once", name: "read_file", params: {}, partial: true }, + ], streamingToolCallIndices: new Map([["call_once", 0]]), userMessageContentReady: true, presentAssistantMessageSafe: vi.fn(), From 03e114f48867eea9bfa9f872f91fc7f7eacb2b81 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 28 Sep 2026 13:49:32 +0000 Subject: [PATCH 09/10] fix(task): defer finalized tool execution until history persists --- src/core/task/Task.ts | 25 ++++-- .../task/__tests__/Task.persistence.spec.ts | 76 +++++++++++++++++++ .../flushPendingToolResultsToHistory.spec.ts | 5 +- 3 files changed, 96 insertions(+), 10 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index b56d6fe3a5..ca5b311f42 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -437,7 +437,7 @@ export class Task extends EventEmitter implements TaskLike { * Reset to `false` at the start of each API request. * Set to `true` only after the assistant message is durably saved. */ - assistantMessageSavedToHistory = false + assistantMessageSavedToHistory = true private assistantMessagePersistencePromise!: Promise private resolveAssistantMessagePersistence!: (result: AssistantMessagePersistenceResult) => void private assistantMessagePersistenceCancellation?: AssistantMessagePersistenceCancellation @@ -1074,6 +1074,9 @@ export class Task extends EventEmitter implements TaskLike { * If the message resolves a pending action, retries the save on initial failure before clearing the action. */ private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string): Promise { + if (message.role === "assistant") { + this.assistantMessageSavedToHistory = false + } const resolvesPendingAction = this.pendingAction && message.role === "user" && @@ -1191,17 +1194,18 @@ export class Task extends EventEmitter implements TaskLike { * So we usually only need to flush the pending user message with tool_results. */ public async flushPendingToolResultsToHistory(): Promise { - // Only flush if there's actually pending content to save - if (this.userMessageContent.length === 0) { - return true - } if (this.abort) { return false } - // CRITICAL: Wait for the assistant message to be saved to API history first. - // Without this, tool_result blocks would appear BEFORE tool_use blocks in the - // conversation history, causing API errors like: + // CRITICAL: Wait for the assistant message to be saved to API history before + // any early return, including the empty-content case below. Delegation relies + // on this barrier: an auto-approved new_task can execute while the parent + // assistant turn still awaits persistence, and disposing the parent at that + // point would leave its tool_use turn out of the durable history. + // + // Without the wait, tool_result blocks would also appear BEFORE tool_use blocks + // in the conversation history, causing API errors like: // "unexpected `tool_use_id` found in `tool_result` blocks" // // This can happen when parallel tools are called (e.g., update_todo_list + new_task). @@ -1226,6 +1230,11 @@ export class Task extends EventEmitter implements TaskLike { } } + // Only flush if there's actually pending content to save + if (this.userMessageContent.length === 0) { + return true + } + // If task was aborted while waiting, don't flush if (this.abort) { return false diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 8d3314a9a6..c5910ab310 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -923,6 +923,7 @@ describe("Task persistence", () => { task: "test task", startTask: false, }) + task.assistantMessageSavedToHistory = false task.userMessageContent = [{ type: "tool_result", tool_use_id: "tool-1", content: "done" }] vi.spyOn(task, "waitForCurrentAssistantMessagePersistence").mockResolvedValue(false) @@ -965,6 +966,81 @@ describe("Task persistence", () => { vi.useRealTimers() } }) + + it("waits for current assistant persistence before an empty-result flush", async () => { + vi.useFakeTimers() + const assistantSave = createDeferred() + mockSaveApiMessages.mockReturnValueOnce(assistantSave.promise) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + try { + const saving = getTaskPersistenceAccess(task).addToApiConversationHistory({ + role: "assistant", + content: [{ type: "text", text: "turn still being written" }], + }) + expect(task.assistantMessageSavedToHistory).toBe(false) + + let settled = false + const flushing = task.flushPendingToolResultsToHistory().then((value) => { + settled = true + return value + }) + + await vi.advanceTimersByTimeAsync(0) + expect(settled).toBe(false) + + assistantSave.resolve(undefined) + await vi.runAllTimersAsync() + await expect(flushing).resolves.toBe(true) + await saving + + expect(task.assistantMessageSavedToHistory).toBe(true) + expect(task.userMessageContent).toEqual([]) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) + } finally { + assistantSave.resolve(undefined) + mockSaveApiMessages.mockResolvedValue(undefined) + vi.useRealTimers() + } + }) + + it("does not report success for an empty-result flush when assistant persistence fails", async () => { + vi.useFakeTimers() + mockSaveApiMessages.mockRejectedValue(new Error("assistant write failed")) + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => undefined) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + try { + await getTaskPersistenceAccess(task).addToApiConversationHistory({ + role: "assistant", + content: [{ type: "text", text: "turn still being written" }], + }) + + const flushing = task.flushPendingToolResultsToHistory() + await vi.runAllTimersAsync() + + await expect(flushing).resolves.toBe(false) + expect(task.assistantMessageSavedToHistory).toBe(false) + expect(consoleWarn).toHaveBeenCalledWith( + expect.stringContaining("failed to persist assistant message"), + expect.any(Error), + ) + } finally { + consoleWarn.mockRestore() + mockSaveApiMessages.mockResolvedValue(undefined) + vi.useRealTimers() + } + }) }) // ── saveClineMessages ──────────────────────────────────────────────── diff --git a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts index f32a6e2507..1fb6b462e5 100644 --- a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts +++ b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts @@ -238,7 +238,7 @@ describe("flushPendingToolResultsToHistory", () => { const initialHistoryLength = task.apiConversationHistory.length // Call flush - await task.flushPendingToolResultsToHistory() + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(true) // History should not have changed since userMessageContent was empty expect(task.apiConversationHistory.length).toBe(initialHistoryLength) @@ -399,7 +399,8 @@ describe("flushPendingToolResultsToHistory", () => { startTask: false, }) - // Flag is false by default - assistant message not yet saved + // Model an assistant write that has started but has not settled. + task.assistantMessageSavedToHistory = false expect(task.assistantMessageSavedToHistory).toBe(false) // Set up pending tool result From b844471fedcb59660d7c8b74bc2356c91ac1b63a Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 28 Sep 2026 22:38:15 +0000 Subject: [PATCH 10/10] refactor(parser): cut PR #700 to parser-only streaming scope Restore Task.ts persistence coordination and stream-time tool_call_end finalization to main; retain only the #695 parser fixes with their focused tests. --- src/core/task/Task.ts | 135 +++++++++--------- .../task/__tests__/Task.persistence.spec.ts | 76 ---------- src/core/task/__tests__/Task.spec.ts | 41 ------ .../finalizeStreamingToolCallById.spec.ts | 125 ---------------- .../flushPendingToolResultsToHistory.spec.ts | 5 +- 5 files changed, 68 insertions(+), 314 deletions(-) delete mode 100644 src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index ca5b311f42..d5313f68cf 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -437,7 +437,7 @@ export class Task extends EventEmitter implements TaskLike { * Reset to `false` at the start of each API request. * Set to `true` only after the assistant message is durably saved. */ - assistantMessageSavedToHistory = true + assistantMessageSavedToHistory = false private assistantMessagePersistencePromise!: Promise private resolveAssistantMessagePersistence!: (result: AssistantMessagePersistenceResult) => void private assistantMessagePersistenceCancellation?: AssistantMessagePersistenceCancellation @@ -500,46 +500,6 @@ export class Task extends EventEmitter implements TaskLike { // Native tool call streaming state (track which index each tool is at) private streamingToolCallIndices: Map = new Map() - /** - * Finalize a streaming native tool call by id and present it. - * - * Shared by every site that observes a tool_call_end: the per-chunk event - * loop, the stream-level tool_call_end case, and the end-of-stream - * finalizeRawChunks() pass. Calling it again for an already-finalized id is a - * safe no-op because finalizeStreamingToolCall() and the index map entry are - * both cleared on first finalize. - */ - private finalizeStreamingToolCallById(id: string, nativeToolCallParserScope: object): void { - const finalToolUse = NativeToolCallParser.finalizeStreamingToolCall(id, nativeToolCallParserScope) - const toolUseIndex = this.streamingToolCallIndices.get(id) - - if (finalToolUse) { - ;(finalToolUse as any).id = id - if (toolUseIndex !== undefined) { - this.assistantMessageContent[toolUseIndex] = finalToolUse - } - this.streamingToolCallIndices.delete(id) - this.userMessageContentReady = false - /* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */ - this.presentAssistantMessageSafe() - } else if (toolUseIndex !== undefined) { - // finalizeStreamingToolCall returned null (malformed JSON or missing args). - // Clear partial arguments before presentation so truncated values cannot run - // or enter conversation history. - const existingToolUse = this.assistantMessageContent[toolUseIndex] - if (existingToolUse && existingToolUse.type === "tool_use") { - existingToolUse.partial = false - existingToolUse.nativeArgs = undefined - existingToolUse.params = {} - ;(existingToolUse as any).id = id - } - this.streamingToolCallIndices.delete(id) - this.userMessageContentReady = false - /* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */ - this.presentAssistantMessageSafe() - } - } - // Cached model info for current streaming session (set at start of each API request) // This prevents excessive getModel() calls during tool execution cachedStreamingModel?: { id: string; info: ModelInfo } @@ -1074,9 +1034,6 @@ export class Task extends EventEmitter implements TaskLike { * If the message resolves a pending action, retries the save on initial failure before clearing the action. */ private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string): Promise { - if (message.role === "assistant") { - this.assistantMessageSavedToHistory = false - } const resolvesPendingAction = this.pendingAction && message.role === "user" && @@ -1194,18 +1151,17 @@ export class Task extends EventEmitter implements TaskLike { * So we usually only need to flush the pending user message with tool_results. */ public async flushPendingToolResultsToHistory(): Promise { + // Only flush if there's actually pending content to save + if (this.userMessageContent.length === 0) { + return true + } if (this.abort) { return false } - // CRITICAL: Wait for the assistant message to be saved to API history before - // any early return, including the empty-content case below. Delegation relies - // on this barrier: an auto-approved new_task can execute while the parent - // assistant turn still awaits persistence, and disposing the parent at that - // point would leave its tool_use turn out of the durable history. - // - // Without the wait, tool_result blocks would also appear BEFORE tool_use blocks - // in the conversation history, causing API errors like: + // CRITICAL: Wait for the assistant message to be saved to API history first. + // Without this, tool_result blocks would appear BEFORE tool_use blocks in the + // conversation history, causing API errors like: // "unexpected `tool_use_id` found in `tool_result` blocks" // // This can happen when parallel tools are called (e.g., update_todo_list + new_task). @@ -1230,11 +1186,6 @@ export class Task extends EventEmitter implements TaskLike { } } - // Only flush if there's actually pending content to save - if (this.userMessageContent.length === 0) { - return true - } - // If task was aborted while waiting, don't flush if (this.abort) { return false @@ -3475,22 +3426,11 @@ export class Task extends EventEmitter implements TaskLike { this.presentAssistantMessageSafe() } } - } else if (event.type === "tool_call_end") { - this.finalizeStreamingToolCallById(event.id, nativeToolCallParserScope) } } break } - case "tool_call_end": { - // Providers emit a tool_call_end chunk when finish_reason is - // "tool_calls" (either directly or via processFinishReason). - // Finalize the streaming tool call now so it is presented during - // streaming rather than waiting for finalizeRawChunks() at stream end. - this.finalizeStreamingToolCallById(chunk.id, nativeToolCallParserScope) - break - } - case "tool_call": { // Legacy: Handle complete tool calls (for backward compatibility) // Convert native tool call to ToolUse format @@ -3858,7 +3798,64 @@ export class Task extends EventEmitter implements TaskLike { const finalizeEvents = NativeToolCallParser.finalizeRawChunks(nativeToolCallParserScope) for (const event of finalizeEvents) { if (event.type === "tool_call_end") { - this.finalizeStreamingToolCallById(event.id, nativeToolCallParserScope) + // Finalize the streaming tool call + const finalToolUse = NativeToolCallParser.finalizeStreamingToolCall( + event.id, + nativeToolCallParserScope, + ) + + // Get the index for this tool call + const toolUseIndex = this.streamingToolCallIndices.get(event.id) + + if (finalToolUse) { + // Store the tool call ID + ;(finalToolUse as any).id = event.id + + // Get the index and replace partial with final + if (toolUseIndex !== undefined) { + this.assistantMessageContent[toolUseIndex] = finalToolUse + } + + // Clean up tracking + this.streamingToolCallIndices.delete(event.id) + + // Mark that we have new content to process + this.userMessageContentReady = false + + // Present the finalized tool call + /* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */ + this.presentAssistantMessageSafe() + } else if (toolUseIndex !== undefined) { + // finalizeStreamingToolCall returned null (malformed JSON or missing args). + // existingToolUse is the same object the streaming phase was mutating in + // place, so it still carries nativeArgs AND params built from the incomplete + // partial parse (e.g. a truncated write_to_file `content` string) - both were + // only ever meant for live progress display, never for execution or for + // ending up in conversation history. Mark the tool as non-partial so it's + // presented as complete, and clear both so presentAssistantMessage's + // `!block.nativeArgs` guard short-circuits with a structured tool_result + // instead of executing the truncated value, and so the toolUse.nativeArgs || + // toolUse.params fallback used when recording history doesn't fall through to + // the same truncated data under a different name. + const existingToolUse = this.assistantMessageContent[toolUseIndex] + if (existingToolUse && existingToolUse.type === "tool_use") { + existingToolUse.partial = false + existingToolUse.nativeArgs = undefined + existingToolUse.params = {} + // Ensure it has the ID for native protocol + ;(existingToolUse as any).id = event.id + } + + // Clean up tracking + this.streamingToolCallIndices.delete(event.id) + + // Mark that we have new content to process + this.userMessageContentReady = false + + // Present the tool call - validation will handle missing params + /* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */ + this.presentAssistantMessageSafe() + } } } diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index c5910ab310..8d3314a9a6 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -923,7 +923,6 @@ describe("Task persistence", () => { task: "test task", startTask: false, }) - task.assistantMessageSavedToHistory = false task.userMessageContent = [{ type: "tool_result", tool_use_id: "tool-1", content: "done" }] vi.spyOn(task, "waitForCurrentAssistantMessagePersistence").mockResolvedValue(false) @@ -966,81 +965,6 @@ describe("Task persistence", () => { vi.useRealTimers() } }) - - it("waits for current assistant persistence before an empty-result flush", async () => { - vi.useFakeTimers() - const assistantSave = createDeferred() - mockSaveApiMessages.mockReturnValueOnce(assistantSave.promise) - const task = new Task({ - provider: mockProvider, - apiConfiguration: mockApiConfig, - task: "test task", - startTask: false, - }) - - try { - const saving = getTaskPersistenceAccess(task).addToApiConversationHistory({ - role: "assistant", - content: [{ type: "text", text: "turn still being written" }], - }) - expect(task.assistantMessageSavedToHistory).toBe(false) - - let settled = false - const flushing = task.flushPendingToolResultsToHistory().then((value) => { - settled = true - return value - }) - - await vi.advanceTimersByTimeAsync(0) - expect(settled).toBe(false) - - assistantSave.resolve(undefined) - await vi.runAllTimersAsync() - await expect(flushing).resolves.toBe(true) - await saving - - expect(task.assistantMessageSavedToHistory).toBe(true) - expect(task.userMessageContent).toEqual([]) - expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) - } finally { - assistantSave.resolve(undefined) - mockSaveApiMessages.mockResolvedValue(undefined) - vi.useRealTimers() - } - }) - - it("does not report success for an empty-result flush when assistant persistence fails", async () => { - vi.useFakeTimers() - mockSaveApiMessages.mockRejectedValue(new Error("assistant write failed")) - const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => undefined) - const task = new Task({ - provider: mockProvider, - apiConfiguration: mockApiConfig, - task: "test task", - startTask: false, - }) - - try { - await getTaskPersistenceAccess(task).addToApiConversationHistory({ - role: "assistant", - content: [{ type: "text", text: "turn still being written" }], - }) - - const flushing = task.flushPendingToolResultsToHistory() - await vi.runAllTimersAsync() - - await expect(flushing).resolves.toBe(false) - expect(task.assistantMessageSavedToHistory).toBe(false) - expect(consoleWarn).toHaveBeenCalledWith( - expect.stringContaining("failed to persist assistant message"), - expect.any(Error), - ) - } finally { - consoleWarn.mockRestore() - mockSaveApiMessages.mockResolvedValue(undefined) - vi.useRealTimers() - } - }) }) // ── saveClineMessages ──────────────────────────────────────────────── diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index a2dff7baae..eae85cf9f2 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -576,47 +576,6 @@ describe("Cline", () => { }) describe("native tool-call request isolation", () => { - it("reassembles Astra-style read_file arguments that arrive before tool identity", async () => { - const task = new Task({ - provider: mockProvider, - apiConfiguration: mockApiConfig, - task: "late tool identity test", - startTask: false, - }) - - vi.spyOn(task.diffViewProvider, "reset").mockResolvedValue(undefined) - vi.spyOn(getTaskTestAccess(task), "safeEnsureModelFetched").mockResolvedValue(stubModelInfo) - vi.spyOn(getTaskTestAccess(task), "presentAssistantMessageSafe").mockImplementation(() => {}) - vi.spyOn(task, "attemptApiRequest").mockImplementation(() => - asyncStreamFrom([ - { type: "tool_call_partial", index: 0, arguments: '{"path":"scripts/final-review-' }, - { type: "tool_call_partial", index: 0, id: "call_astra_read", name: "read_file" }, - { - type: "tool_call_partial", - index: 0, - arguments: 'smoke/driver.mts","mode":"slice","offset":1,"limit":2000}', - }, - ]), - ) - - await task.recursivelyMakeClineRequests([{ type: "text", text: "review the driver" }]) - - const assistantMessage = task.apiConversationHistory.find((message) => message.role === "assistant") - expect(assistantMessage?.content).toEqual([ - { - type: "tool_use", - id: "call_astra_read", - name: "read_file", - input: { - path: "scripts/final-review-smoke/driver.mts", - mode: "slice", - offset: 1, - limit: 2000, - }, - }, - ]) - }) - it("keeps overlapping Task parser state scoped to each request", async () => { const firstTask = new Task({ provider: mockProvider, diff --git a/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts b/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts deleted file mode 100644 index fecbdd8506..0000000000 --- a/src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts +++ /dev/null @@ -1,125 +0,0 @@ -// npx vitest run core/task/__tests__/finalizeStreamingToolCallById.spec.ts - -import { Task } from "../Task" -import { NativeToolCallParser } from "../../assistant-message/NativeToolCallParser" -import type { ToolUse } from "../../../shared/tools" - -type FinalizeStub = { - assistantMessageContent: ToolUse[] - streamingToolCallIndices: Map - userMessageContentReady: boolean - presentAssistantMessageSafe: ReturnType -} - -type FinalizeMethod = (this: FinalizeStub, id: string, scope: object) => void - -/** - * Invoke the private finalizeStreamingToolCallById against a minimal `this` stub. - * - * Instantiating a full Task requires a provider, context, and async setup that are - * irrelevant to this helper. The method only touches assistantMessageContent, - * streamingToolCallIndices, and userMessageContentReady, so a stub carrying those - * fields exercises the real source lines without the constructor. - */ -function callFinalize(stub: FinalizeStub, id: string, scope: object): void { - const finalize = (Task.prototype as unknown as { finalizeStreamingToolCallById: FinalizeMethod }) - .finalizeStreamingToolCallById - finalize.call(stub, id, scope) -} - -describe("Task.finalizeStreamingToolCallById", () => { - beforeEach(() => { - vi.clearAllMocks() - }) - - it("replaces the partial block with the finalized tool use and presents it", () => { - const scope = NativeToolCallParser.createScope() - const finalToolUse: ToolUse = { type: "tool_use", name: "read_file", params: {}, partial: false } - const finalizeSpy = vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall").mockReturnValue(finalToolUse) - - const stub: FinalizeStub = { - assistantMessageContent: [ - { type: "tool_use", id: "call_abc", name: "read_file", params: {}, partial: true }, - ], - streamingToolCallIndices: new Map([["call_abc", 0]]), - userMessageContentReady: true, - presentAssistantMessageSafe: vi.fn(), - } - - callFinalize(stub, "call_abc", scope) - - expect(finalizeSpy).toHaveBeenCalledWith("call_abc", scope) - expect(stub.assistantMessageContent[0]).toBe(finalToolUse) - expect(stub.assistantMessageContent[0].id).toBe("call_abc") - expect(stub.streamingToolCallIndices.has("call_abc")).toBe(false) - expect(stub.userMessageContentReady).toBe(false) - expect(stub.presentAssistantMessageSafe).toHaveBeenCalledTimes(1) - }) - - it("marks the existing block non-partial when finalize returns null (malformed JSON)", () => { - const scope = NativeToolCallParser.createScope() - vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall").mockReturnValue(null) - - const existingBlock: ToolUse = { - type: "tool_use", - id: "call_bad", - name: "write_to_file", - params: {}, - partial: true, - } - const stub: FinalizeStub = { - assistantMessageContent: [existingBlock], - streamingToolCallIndices: new Map([["call_bad", 0]]), - userMessageContentReady: true, - presentAssistantMessageSafe: vi.fn(), - } - - callFinalize(stub, "call_bad", scope) - - expect(existingBlock.partial).toBe(false) - expect(existingBlock.id).toBe("call_bad") - expect(stub.streamingToolCallIndices.has("call_bad")).toBe(false) - expect(stub.userMessageContentReady).toBe(false) - expect(stub.presentAssistantMessageSafe).toHaveBeenCalledTimes(1) - }) - - it("is a no-op when the id is not tracked", () => { - const scope = NativeToolCallParser.createScope() - vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall").mockReturnValue(null) - - const stub = { - assistantMessageContent: [] as ToolUse[], - streamingToolCallIndices: new Map(), - userMessageContentReady: true, - presentAssistantMessageSafe: vi.fn(), - } - - callFinalize(stub, "call_unknown", scope) - - expect(stub.assistantMessageContent).toHaveLength(0) - expect(stub.userMessageContentReady).toBe(true) - expect(stub.presentAssistantMessageSafe).not.toHaveBeenCalled() - }) - - it("is idempotent: a second call for the same id does nothing", () => { - const scope = NativeToolCallParser.createScope() - const finalToolUse: ToolUse = { type: "tool_use", name: "read_file", params: {}, partial: false } - vi.spyOn(NativeToolCallParser, "finalizeStreamingToolCall") - .mockReturnValueOnce(finalToolUse) - .mockReturnValue(null) - - const stub: FinalizeStub = { - assistantMessageContent: [ - { type: "tool_use", id: "call_once", name: "read_file", params: {}, partial: true }, - ], - streamingToolCallIndices: new Map([["call_once", 0]]), - userMessageContentReady: true, - presentAssistantMessageSafe: vi.fn(), - } - - callFinalize(stub, "call_once", scope) - callFinalize(stub, "call_once", scope) // id no longer tracked -> no-op - - expect(stub.presentAssistantMessageSafe).toHaveBeenCalledTimes(1) - }) -}) diff --git a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts index 1fb6b462e5..f32a6e2507 100644 --- a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts +++ b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts @@ -238,7 +238,7 @@ describe("flushPendingToolResultsToHistory", () => { const initialHistoryLength = task.apiConversationHistory.length // Call flush - await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(true) + await task.flushPendingToolResultsToHistory() // History should not have changed since userMessageContent was empty expect(task.apiConversationHistory.length).toBe(initialHistoryLength) @@ -399,8 +399,7 @@ describe("flushPendingToolResultsToHistory", () => { startTask: false, }) - // Model an assistant write that has started but has not settled. - task.assistantMessageSavedToHistory = false + // Flag is false by default - assistant message not yet saved expect(task.assistantMessageSavedToHistory).toBe(false) // Set up pending tool result