Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions prs/fix-toolcall-dropped-leading-deltas.md
Comment thread
awschmeder marked this conversation as resolved.
Outdated
Original file line number Diff line number Diff line change
@@ -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
47 changes: 27 additions & 20 deletions src/core/assistant-message/NativeToolCallParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export class NativeToolCallParser {
private static rawChunkTracker = new Map<
number,
{
id: string
id?: string
name: string
hasStarted: boolean
deltaBuffer: string[]
Expand Down Expand Up @@ -105,10 +105,11 @@ export class NativeToolCallParser {
const events: ToolCallStreamEvent[] = []
const { index, id, name, arguments: args } = chunk

// 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.
let tracked = this.rawChunkTracker.get(index)

// Initialize new tool call tracking when we receive an id
if (id && !tracked) {
if (!tracked) {
tracked = {
id,
name: name || "",
Expand All @@ -118,38 +119,39 @@ export class NativeToolCallParser {
this.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
Comment thread
awschmeder marked this conversation as resolved.
}

// 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,
Expand All @@ -172,10 +174,15 @@ export class NativeToolCallParser {

if (finishReason === "tool_calls" && this.rawChunkTracker.size > 0) {
for (const [, tracked] of this.rawChunkTracker.entries()) {
events.push({
type: "tool_call_end",
id: tracked.id,
})
// Only emit an end for trackers that actually started. A tracker that
// never received an id/name (malformed stream) must not emit a phantom
// end; since start requires an id, hasStarted implies tracked.id is set.
if (tracked.hasStarted && tracked.id) {
Comment thread
awschmeder marked this conversation as resolved.
events.push({
type: "tool_call_end",
id: tracked.id,
})
}
}
}

Expand All @@ -191,7 +198,7 @@ export class NativeToolCallParser {

if (this.rawChunkTracker.size > 0) {
for (const [, tracked] of this.rawChunkTracker.entries()) {
if (tracked.hasStarted) {
if (tracked.hasStarted && tracked.id) {
events.push({
type: "tool_call_end",
id: tracked.id,
Expand Down
199 changes: 198 additions & 1 deletion src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { NativeToolCallParser } from "../NativeToolCallParser"
import { NativeToolCallParser, type ToolCallStreamEvent } from "../NativeToolCallParser"

describe("NativeToolCallParser", () => {
beforeEach(() => {
Expand Down Expand Up @@ -343,4 +343,201 @@ 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<string, ReturnType<typeof NativeToolCallParser.finalizeStreamingToolCall>>()
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)", () => {
Comment thread
awschmeder marked this conversation as resolved.
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") {
Comment thread
awschmeder marked this conversation as resolved.
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") {
Comment thread
awschmeder marked this conversation as resolved.
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)
})

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()
})
})
})
4 changes: 2 additions & 2 deletions src/core/prompts/tools/native-tools/ask_followup_question.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }] }
Expand All @@ -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`
Comment thread
awschmeder marked this conversation as resolved.
Outdated

const FOLLOW_UP_TEXT_DESCRIPTION = `Suggested answer the user can pick`

Expand Down
Loading