fix: streaming tool-call argument loss in NativeToolCallParser (#695)#700
fix: streaming tool-call argument loss in NativeToolCallParser (#695)#700awschmeder wants to merge 8 commits into
Conversation
awschmeder
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNativeToolCallParser now starts tracking on first index sighting, preserves early argument chunks until id and name arrive, and avoids emitting end events for incomplete streams. Task now uses one helper for native tool-call completion, and the spec adds reassembly regressions. ChangesNativeToolCallParser streaming reassembly fix
Sequence Diagram(s)sequenceDiagram
participant NativeToolCallParser
participant Task
participant assistantMessageContent
NativeToolCallParser->>Task: tool_call_end event
Task->>NativeToolCallParser: finalizeStreamingToolCall(id)
NativeToolCallParser-->>Task: finalized tool-use data or null
Task->>assistantMessageContent: update tool-use block and clear streaming state
Task->>Task: presentAssistantMessage(this)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/fix-toolcall-dropped-leading-deltas.md (1)
1-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove this agent-generated changeset file from the PR.
This file conflicts with the repository’s changeset policy and should not be committed in this change.
As per coding guidelines: ".changeset/**: Do NOT create
.changesetfiles for each commit or code change. Changesets are managed separately by maintainers and should not be generated by agents during normal development."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/fix-toolcall-dropped-leading-deltas.md around lines 1 - 6, The .changeset/fix-toolcall-dropped-leading-deltas.md file was auto-generated and conflicts with the repository's changeset policy which specifies that changeset files should not be created by agents during normal development. Remove this file from the PR entirely as changesets are managed separately by maintainers only.Source: Coding guidelines
🧹 Nitpick comments (1)
src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts (1)
373-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one explicit test that exercises
finalizeRawChunks()directly.The new suite validates the
processFinishReasonend path, but this helper clears raw state instead of asserting thefinalizeRawChunksguard that was also changed in this PR. A focused case for that path would harden regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts` around lines 373 - 380, Add a new focused test case within the test suite that directly invokes the NativeToolCallParser.finalizeRawChunks() method to validate its behavior. This test should verify that the guard logic in finalizeRawChunks works correctly, since the current test validates the processFinishReason path which uses clearRawChunkState instead. Include assertions that confirm the expected end events are produced by finalizeRawChunks() and that raw state is properly finalized, ensuring regression coverage for the changes made to this method in this PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.changeset/fix-toolcall-dropped-leading-deltas.md:
- Around line 1-6: The .changeset/fix-toolcall-dropped-leading-deltas.md file
was auto-generated and conflicts with the repository's changeset policy which
specifies that changeset files should not be created by agents during normal
development. Remove this file from the PR entirely as changesets are managed
separately by maintainers only.
---
Nitpick comments:
In `@src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts`:
- Around line 373-380: Add a new focused test case within the test suite that
directly invokes the NativeToolCallParser.finalizeRawChunks() method to validate
its behavior. This test should verify that the guard logic in finalizeRawChunks
works correctly, since the current test validates the processFinishReason path
which uses clearRawChunkState instead. Include assertions that confirm the
expected end events are produced by finalizeRawChunks() and that raw state is
properly finalized, ensuring regression coverage for the changes made to this
method in this PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 583ebfd1-e91f-4526-89e4-69d1726d9c0a
📒 Files selected for processing (5)
.changeset/fix-toolcall-dropped-leading-deltas.mdprs/fix-toolcall-dropped-leading-deltas.mdsrc/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/__tests__/NativeToolCallParser.spec.tssrc/core/prompts/tools/native-tools/ask_followup_question.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks! Had a couple comments on testing some edge cases.
github-actions
Bot
added
the
awaiting-author
…ing 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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts (1)
589-595: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptionally assert
finalizeEventsis empty for sharper failure localization.The combined-length assertion proves no double-fire, but won't show which call emitted the duplicate on regression. Asserting the source split makes the intended contract (finish emits, finalize is a no-op) explicit.
♻️ Optional: assert per-call ends
const allEnds = [...finishEvents, ...finalizeEvents].filter((e) => e.type === "tool_call_end") expect(allEnds).toHaveLength(1) expect(allEnds[0].id).toBe("call_dup") + // finishReason emits the single end; finalize must be a no-op for the same tracker. + expect(finishEvents.filter((e) => e.type === "tool_call_end")).toHaveLength(1) + expect(finalizeEvents.filter((e) => e.type === "tool_call_end")).toHaveLength(0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts` around lines 589 - 595, The test around NativeToolCallParser’s duplicate end handling only checks the combined tool_call_end count, so tighten it by asserting finishEvents contains the single expected end for call_dup and finalizeEvents has no tool_call_end entries. Use the NativeToolCallParser.processFinishReason and NativeToolCallParser.finalizeRawChunks calls to make the contract explicit: finish emits the end event, and finalize is a no-op for that case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts`:
- Around line 589-595: The test around NativeToolCallParser’s duplicate end
handling only checks the combined tool_call_end count, so tighten it by
asserting finishEvents contains the single expected end for call_dup and
finalizeEvents has no tool_call_end entries. Use the
NativeToolCallParser.processFinishReason and
NativeToolCallParser.finalizeRawChunks calls to make the contract explicit:
finish emits the end event, and finalize is a no-op for that case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79060a7f-f372-4ca0-b035-f7d0e4e13f95
📒 Files selected for processing (2)
src/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/__tests__/NativeToolCallParser.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/assistant-message/NativeToolCallParser.ts
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().
|
Pushed a follow-up commit ( Problem: Providers emit a Changes:
Verification:
|
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.
2 participants
Related GitHub Issue
Closes: #695
Description
When a provider streams a tool call whose first delta(s) arrive before the tool-call
idis known, those leading argument bytes are silently discarded byNativeToolCallParser.processRawChunk. This causes downstream "missing required parameter" and other spurious provider-dependent errors even when the model supplied the correct tool use syntax.This PR fixes the issue by centralizing the tracking of streaming tool calls in
NativeToolCallParser. TherawChunkTrackeris now initialized on the first sight of a streamindex, independent of whether anidis present. Allargumentsdeltas are buffered until bothidandnameare known, ensuring no data loss during streaming reassembly.Scope note: accompanying
Task.tschangeThe fix spans two layers because the parser and its streaming consumer must agree on when a tool call is finalized:
NativeToolCallParser(the core fix): buffers pre-idargument deltas and only emitstool_call_endfor started trackers with a non-emptyid, preventing both data loss and phantom end events.Task.ts(required consumer change): providers emit atool_call_endstream chunk onfinish_reason: "tool_calls"(either directly, or viaprocessFinishReason()for openrouter/lm-studio/qwen-code).Task.ts's streamswitchhad notool_call_endcase, so those chunks were silently dropped and tool calls only finalized at stream end viafinalizeRawChunks(). Without this change, the parser's correctly-buffered arguments would still not be presented during streaming. Adding the new case would have created a third copy of the finalize/present logic (the codebase already had two: the per-chunk event loop and thefinalizeRawChunks()loop), so all three sites are consolidated into a single idempotent helper,finalizeStreamingToolCallById(id). Re-finalizing an already-cleared id is a safe no-op, so the new streaming finalization and the end-of-stream pass cannot double-present.Test Procedure
src/core/assistant-message/__tests__/NativeToolCallParser.spec.tswhich verifies that leading argument bytes arriving before theidare correctly preserved and finalized.src/core/task/__tests__/finalizeStreamingToolCallById.spec.ts, which exercises the realTask.finalizeStreamingToolCallByIdhelper across the success, malformed-JSON, untracked-id, and idempotent re-finalize paths (closes the patch-coverage gap on the consumer change).Pre-Submission Checklist
Screenshots / Videos
N/A
Documentation Updates
Additional Notes
N/A
Get in Touch
@awschmeder
Summary by CodeRabbit
id/namearrive after argument chunks.