Repository navigation
fix: preserve parallel tool call deltas - #1830
Merged
akshaydeo merged 3 commits intoMar 1, 2026
Merged
Conversation
Match tool call deltas by id/index during chat stream accumulation to prevent interleaved tool calls from being merged, and add a regression test.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAccumulate and routing logic updated so streaming function-call argument deltas are routed by tool_call ID or Index to the correct ChatMessage/tool-call entry (or a new stub message) instead of always appending to the most recent entry, preventing merged/incorrect tool-call accumulation across parallel calls. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StreamHandler
participant Accumulator
participant MessageStore
Client->>StreamHandler: stream chunk (Item / ArgumentsDelta, includes resp.ItemID?)
StreamHandler->>MessageStore: append Item as new ChatMessage (if resp.Item present)
StreamHandler->>Accumulator: send ArgumentsDelta (include resp.ItemID)
Accumulator->>MessageStore: lookup target by ToolCall ID or Index (use idToIndex / indexToIndex)
alt target found
Accumulator->>MessageStore: update existing tool-call arguments/name
else target not found but resp.ItemID present
Accumulator->>MessageStore: create stub ChatMessage with resp.ItemID, add delta
else fallback
Accumulator->>MessageStore: append delta to last message (fallback)
end
MessageStore-->>Client: updated ChatMessage(s) in stream state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/streaming/accumulator_test.go (1)
291-309: Preferbifrost.Ptr(...)for pointer construction consistency in tests.♻️ Proposed consistency refactor
accumulator.accumulateToolCallsInMessage(message, []schemas.ChatAssistantMessageToolCall{ - makeDelta(0, &toolCallID0, &toolNameAdd, ""), + makeDelta(0, bifrost.Ptr(toolCallID0), bifrost.Ptr(toolNameAdd), ""), }) accumulator.accumulateToolCallsInMessage(message, []schemas.ChatAssistantMessageToolCall{ - makeDelta(1, &toolCallID1, &toolNameMultiply, ""), + makeDelta(1, bifrost.Ptr(toolCallID1), bifrost.Ptr(toolNameMultiply), ""), })Based on learnings: In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&), including in test utilities/code paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/streaming/accumulator_test.go` around lines 291 - 309, Replace direct address-of pointer constructions used in the makeDelta invocations with bifrost.Ptr(...) for consistency: update the calls to makeDelta in the accumulator_test.go block (the ones passed into accumulator.accumulateToolCallsInMessage) so that expressions like &toolCallID0, &toolNameAdd, &toolCallID1, &toolNameMultiply are replaced by bifrost.Ptr(toolCallID0), bifrost.Ptr(toolNameAdd), etc.; leave nil values as-is and keep the same order of arguments so accumulateToolCallsInMessage and makeDelta usage remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/streaming/accumulator_test.go`:
- Around line 291-309: Replace direct address-of pointer constructions used in
the makeDelta invocations with bifrost.Ptr(...) for consistency: update the
calls to makeDelta in the accumulator_test.go block (the ones passed into
accumulator.accumulateToolCallsInMessage) so that expressions like &toolCallID0,
&toolNameAdd, &toolCallID1, &toolNameMultiply are replaced by
bifrost.Ptr(toolCallID0), bifrost.Ptr(toolNameAdd), etc.; leave nil values as-is
and keep the same order of arguments so accumulateToolCallsInMessage and
makeDelta usage remain unchanged.
Contributor
Author
|
CodeRabbit notes: TestAccumulateToolCallsInterleavedParallel appears once in framework/streaming/accumulator_test.go (no duplicate). No new exported funcs beyond tests; docstring coverage unchanged. |
Collaborator
|
Hey @KTS-o7, thanks for the PR! ❤️ This looks good overall. Just two quick items:
Thanks! |
Contributor
Author
|
Will do |
- Route FunctionCallArgumentsDelta by ItemID in buildCompleteMessageFromResponsesStreamChunks to prevent parallel tool call arguments from merging - Add regression test TestBuildCompleteMessageFromResponsesStreamChunksParallelToolCalls - Add changelog entries to framework/changelog.md and transports/changelog.md
Contributor
Author
|
@Pratham-Mishra04 Both items are done:
All tests pass. Ready for re-review! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/streaming/responses.go`:
- Around line 523-531: The current logic falls back to the last message when
resp.ItemID is provided but not found, which can merge deltas into the wrong
message; change the flow in the block that searches messages for resp.ItemID so
that if resp.ItemID != nil and no matching messages[i].ID is found you do not
set targetIdx or append the delta — instead skip calling
a.appendFunctionArgumentsDeltaToResponsesMessage (or log/return early). Update
the search for resp.ItemID (the loop that assigns targetIdx) to detect the “not
found” case and only call
a.appendFunctionArgumentsDeltaToResponsesMessage(&messages[targetIdx],
*resp.Delta) when a valid match was located.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
framework/changelog.mdframework/streaming/accumulator_test.goframework/streaming/responses.gotransports/changelog.md
✅ Files skipped from review due to trivial changes (1)
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/streaming/accumulator_test.go
Collaborator
|
Oh great, that was super fast! I noticed a comment from CodeRabbit though, could you please resolve it? |
…ming When resp.ItemID is present but no matching message is found, create a stub message with that ID instead of falling back to the last message. Prevents parallel tool call argument deltas from merging into the wrong call when the OutputItemAdded event arrives out of order.
Contributor
Author
|
@Pratham-Mishra04 CodeRabbit comment resolved — when |
akshaydeo
approved these changes
Mar 1, 2026
loss-and-quick
pushed a commit
to loss-and-quick/bifrost
that referenced
this pull request
Mar 7, 2026
* fix: preserve parallel tool call deltas Match tool call deltas by id/index during chat stream accumulation to prevent interleaved tool calls from being merged, and add a regression test. * fix: route parallel tool call deltas by id in Responses API streaming - Route FunctionCallArgumentsDelta by ItemID in buildCompleteMessageFromResponsesStreamChunks to prevent parallel tool call arguments from merging - Add regression test TestBuildCompleteMessageFromResponsesStreamChunksParallelToolCalls - Add changelog entries to framework/changelog.md and transports/changelog.md * fix: create stub message when ItemID unmatched in Responses API streaming When resp.ItemID is present but no matching message is found, create a stub message with that ID instead of falling back to the last message. Prevents parallel tool call argument deltas from merging into the wrong call when the OutputItemAdded event arrives out of order.
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
* fix: preserve parallel tool call deltas Match tool call deltas by id/index during chat stream accumulation to prevent interleaved tool calls from being merged, and add a regression test. * fix: route parallel tool call deltas by id in Responses API streaming - Route FunctionCallArgumentsDelta by ItemID in buildCompleteMessageFromResponsesStreamChunks to prevent parallel tool call arguments from merging - Add regression test TestBuildCompleteMessageFromResponsesStreamChunksParallelToolCalls - Add changelog entries to framework/changelog.md and transports/changelog.md * fix: create stub message when ItemID unmatched in Responses API streaming When resp.ItemID is present but no matching message is found, create a stub message with that ID instead of falling back to the last message. Prevents parallel tool call argument deltas from merging into the wrong call when the OutputItemAdded event arrives out of order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tool_call.id/tool_call.indexto avoid cross-call merging during parallel tool callingChanges
framework/streaming/accumulator.go: build id/index lookup maps, update or create tool calls based on identityframework/streaming/accumulator_test.go: add interleaved parallel tool-call accumulator testType of change
Affected areas
How to test
go test ./streaming(fromframework/)Checklist
Fixes #1829