feat: improve Responses API streaming with OpenAI-style lifecycle events - #815
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughRefactors streaming across providers to use per-stream state objects, unify final/error emission via ProcessAndSendResponse/ProcessAndSendBifrostError with a stream-end context flag, add Responses←Chat fallback state, and add streaming tool-call tests and flags. Error enrichment and per-output lifecycle events are standardized. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bifrost
participant Provider
participant Converter as ToBifrostResponsesStream(state)
participant Sink
Note over Client,Bifrost: Start ResponsesStream (ctx may set fallback)
Client->>Bifrost: Start stream request
Bifrost->>Provider: Open provider stream
Provider->>Bifrost: emits chunk/event
Bifrost->>Converter: pass chunk + stream state
rect rgb(230,245,255)
Converter->>Converter: update stream state (IDs, buffers, mappings)
Converter-->>Sink: emit 0..n Responses-style messages (Created/InProgress, deltas, item_added, args_delta, done)
end
alt Completed (final)
Converter->>Bifrost: emit Completed (usage) with stream-end indicator
Bifrost->>Client: final response (end)
else intermediate
Sink->>Client: streamed partial responses
end
sequenceDiagram
participant Provider
participant OldConv as Old
participant NewConv as New
Provider->>Old: chunk
Old-->>Provider: single Responses response
Provider->>New: chunk + accumulator/state
New->>New: maintain per-output state across chunks
New-->>Provider: multiple Responses messages per chunk (Created/InProgress, deltas, item_added, done)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core/schemas/bifrost.go (1)
117-117: Consider adding inline documentation for the new context key.The new
BifrostContextKeyIsResponsesToChatCompletionFallbackconstant is used across multiple providers but lacks inline documentation explaining its purpose. Consider adding a comment similar to the others in this block (e.g.,// bool (set by bifrost)).- BifrostContextKeySendBackRawResponse BifrostContextKey = "bifrost-send-back-raw-response" // bool - BifrostContextKeyIsResponsesToChatCompletionFallback BifrostContextKey = "bifrost-is-responses-to-chat-completion-fallback" // bool (set by bifrost) + BifrostContextKeySendBackRawResponse BifrostContextKey = "bifrost-send-back-raw-response" // bool + BifrostContextKeyIsResponsesToChatCompletionFallback BifrostContextKey = "bifrost-is-responses-to-chat-completion-fallback" // bool (signals responses use chat-completion fallback path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
core/providers/anthropic/anthropic.go(1 hunks)core/providers/anthropic/responses.go(8 hunks)core/providers/bedrock/bedrock.go(3 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(3 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(1 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(1 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/utils/utils.go
- core/providers/perplexity/responses.go
🧰 Additional context used
🧬 Code graph analysis (18)
core/providers/sgl.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/parasail.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/cerebras.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/mistral/mistral.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
tests/core-providers/tests.go (1)
tests/core-providers/scenarios/tool_calls_streaming.go (1)
RunToolCallsStreamingTest(217-741)
core/providers/gemini/gemini.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/groq.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/bedrock/responses.go (2)
core/providers/bedrock/types.go (1)
BedrockStreamEvent(363-380)core/schemas/responses.go (18)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/perplexity/perplexity.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/vertex/vertex.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/schemas/mux.go (4)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(25-40)core/schemas/responses.go (17)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1378-1378)ResponsesStreamResponseTypeRefusalDelta(1363-1363)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeCompleted(1350-1350)core/schemas/utils.go (1)
Ptr(14-16)core/schemas/bifrost.go (2)
RequestType(81-81)ResponsesStreamRequest(90-90)
core/providers/cohere/responses.go (2)
core/providers/cohere/types.go (13)
CohereStreamEvent(381-386)StreamEventMessageStart(366-366)StreamEventContentStart(367-367)CohereContentBlockTypeText(128-128)CohereContentBlockTypeThinking(130-130)StreamEventContentDelta(368-368)StreamEventContentEnd(369-369)StreamEventToolPlanDelta(370-370)StreamEventToolCallStart(371-371)StreamEventToolCallDelta(372-372)StreamEventToolCallEnd(373-373)StreamEventCitationEnd(375-375)StreamEventMessageEnd(376-376)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeReasoning(297-297)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeOutputTextAnnotationAdded(1401-1401)ResponsesStreamResponseTypeOutputTextAnnotationDone(1402-1402)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/anthropic/responses.go (2)
core/schemas/responses.go (21)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseType(1345-1345)ResponsesStreamResponseTypeMCPCallArgumentsDelta(1386-1386)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesMessageTypeComputerCall(282-282)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesStreamResponseTypeMCPCallArgumentsDone(1387-1387)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/anthropic/types.go (9)
AnthropicStreamEventTypeContentBlockStart(303-303)AnthropicContentBlockTypeText(127-127)AnthropicContentBlockTypeToolUse(129-129)AnthropicContentBlockTypeMCPToolUse(133-133)AnthropicStreamEventTypeContentBlockDelta(304-304)AnthropicStreamDeltaTypeText(326-326)AnthropicStreamDeltaTypeInputJSON(327-327)AnthropicStreamEventTypeMessageDelta(306-306)AnthropicStreamEventTypeMessageStop(302-302)
core/providers/ollama.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/openai/openai.go (4)
core/schemas/mux.go (2)
ChatToResponsesStreamAccumulator(961-976)NewChatToResponsesStreamAccumulator(979-989)core/schemas/bifrost.go (8)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)BifrostError(351-360)ErrorField(369-376)BifrostErrorExtraFields(418-422)RequestType(81-81)ResponsesStreamRequest(90-90)BifrostContextKeyStreamEndIndicator(111-111)ChatCompletionStreamRequest(88-88)core/schemas/responses.go (2)
ResponsesStreamResponseTypeError(1409-1409)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/utils/utils.go (5)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(791-819)ProviderSendsDoneMarker(760-769)ProcessAndSendError(605-651)
tests/core-providers/scenarios/tool_calls_streaming.go (3)
core/schemas/chatcompletions.go (5)
ChatAssistantMessageToolCall(483-488)ChatAssistantMessageToolCallFunction(491-494)BifrostChatRequest(11-18)ChatParameters(154-183)ChatTool(201-205)tests/core-providers/scenarios/utils.go (6)
ToolCallInfo(293-297)CreateBasicChatMessage(218-225)GetSampleChatTool(129-148)SampleToolTypeWeather(69-69)CreateBasicResponsesMessage(227-235)GetSampleResponsesTool(150-169)core/schemas/responses.go (5)
BifrostResponsesStreamResponse(1412-1450)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)
core/providers/cohere/cohere.go (4)
core/providers/cohere/responses.go (1)
NewCohereStreamAccumulator(26-34)core/schemas/bifrost.go (2)
BifrostResponseExtraFields(282-291)RequestType(81-81)core/providers/utils/utils.go (4)
ShouldSendBackRawResponse(480-485)HandleStreamEndWithSuccess(716-724)GetBifrostResponseForStreamResponse(791-819)ProcessAndSendResponse(533-563)core/schemas/responses.go (1)
BifrostResponsesResponse(45-82)
core/providers/bedrock/bedrock.go (3)
core/providers/bedrock/responses.go (2)
NewBedrockStreamAccumulator(28-38)FinalizeBedrockStream(795-886)core/schemas/bifrost.go (2)
BifrostResponseExtraFields(282-291)RequestType(81-81)core/providers/utils/utils.go (3)
ShouldSendBackRawResponse(480-485)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(791-819)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (23)
core/providers/ollama.go (1)
172-180: LGTM! Fallback streaming mode enabled correctly.The ResponsesStream implementation now properly signals the chat-completion fallback path via context and passes the postHookRunner directly, aligning with the unified streaming architecture described in the PR.
tests/core-providers/config/account.go (1)
29-29: LGTM! Test configuration field added appropriately.The new
ToolCallsStreamingfield enables streaming tool call test scenarios across providers, supporting the PR's comprehensive test coverage expansion.core/providers/sgl.go (1)
169-177: LGTM! Consistent with fallback streaming pattern.The changes properly enable chat-completion fallback mode for responses streaming and simplify post-hook handling, consistent with the unified streaming architecture.
core/providers/cerebras.go (1)
172-180: LGTM! Fallback streaming pattern applied correctly.The implementation follows the same pattern as other providers (Ollama, SGL), enabling the unified fallback streaming path for responses.
tests/core-providers/ollama_test.go (1)
35-35: LGTM! Streaming tool call tests enabled.The addition of
ToolCallsStreaming: trueappropriately expands test coverage for Ollama's streaming tool call functionality.tests/core-providers/openai_test.go (1)
46-46: LGTM! Test coverage expanded appropriately.The addition enables streaming tool call tests for OpenAI, aligning with the PR's goal of comprehensive streaming tool call testing across providers.
tests/core-providers/bedrock_test.go (1)
40-40: LGTM! Bedrock streaming tool call tests enabled.The test configuration correctly enables streaming tool call scenarios for Bedrock, consistent with the test expansion across all providers in this PR.
tests/core-providers/azure_test.go (1)
42-42: LGTM! Streaming tool calls test enabled for Azure.This change enables the new streaming tool calls test scenario for Azure, consistent with the PR's objectives to improve Responses API streaming across all providers.
tests/core-providers/anthropic_test.go (1)
38-38: LGTM! Streaming tool calls test enabled for Anthropic.This change enables the new streaming tool calls test scenario for Anthropic, aligning with the broader PR effort to add streaming support across providers.
tests/core-providers/mistral_test.go (1)
38-38: LGTM! Streaming tool calls test enabled for Mistral.Consistent test configuration change enabling streaming tool calls for the Mistral provider.
tests/core-providers/cohere_test.go (1)
36-36: LGTM! Streaming tool calls test enabled for Cohere.This change enables the new streaming tool calls test scenario for Cohere, consistent with the PR's comprehensive test coverage improvements.
tests/core-providers/cerebras_test.go (1)
40-40: LGTM! Streaming tool calls test enabled for Cerebras.Consistent test configuration change enabling streaming tool calls for the Cerebras provider.
core/providers/groq.go (1)
213-219: LGTM! Simplified streaming implementation for Groq.This refactor removes the post-hook converter wrapper and sets the
BifrostContextKeyIsResponsesToChatCompletionFallbackcontext flag, simplifying the Responses-to-Chat streaming path. The change aligns with the PR's objective to replace chat-to-responses conversion with native implementations per provider.tests/core-providers/tests.go (2)
35-35: LGTM! New streaming test scenario added to test runner.This adds the new
RunToolCallsStreamingTestto the comprehensive test suite, enabling streaming tool call tests across all providers.
77-77: LGTM! Test summary updated to include streaming scenario.This ensures the new
ToolCallsStreamingscenario is reflected in the test summary output, providing visibility into streaming test coverage.tests/core-providers/openrouter_test.go (1)
37-37: LGTM! Streaming tool calls test enabled for OpenRouter.This change enables the new streaming tool calls test scenario for OpenRouter, completing the test coverage across all providers in this PR.
core/providers/mistral/mistral.go (1)
203-207: Fallback flag wiring looks goodSetting the fallback marker on the context before delegating to
ChatCompletionStreamand passing the originalpostHookRunnerdirectly matches the updated accumulator flow—no issues from my side.core/providers/parasail.go (1)
145-149: Consistent fallback handlingGood to see Parasail flagging the responses-to-chat fallback and handing
postHookRunnerthrough unchanged; this keeps behavior aligned with the shared streaming path.core/providers/gemini/gemini.go (1)
347-351: Gemini fallback signal matches the new contractApplying the context flag here ensures the shared streaming handler can emit responses semantics during fallback, and keeping the original post-hook runner maintains hook behavior. Looks correct.
tests/core-providers/parasail_test.go (1)
35-37: Streaming scenario coverage enabledTurning on
ToolCallsStreaminghere is the right move to validate the new streaming accumulators for Parasail.tests/core-providers/vertex_test.go (1)
35-37: Vertex scenarios now exercise streaming tool callsEnabling
ToolCallsStreamingensures the Vertex integration is validated against the updated streaming path; no issues spotted.core/providers/perplexity/perplexity.go (2)
212-212: LGTM: Context value properly flags fallback mode.The context value injection correctly signals that this is a Responses-to-ChatCompletion fallback path, enabling downstream handlers to perform appropriate chunk conversions.
213-218: The streaming conversion refactoring is correctly implemented.Verification confirms all requested functionality is in place:
- OpenAI handler properly detects
BifrostContextKeyIsResponsesToChatCompletionFallbackcontext flag (core/providers/openai/openai.go:706-710)- Chat-to-responses chunk conversion is performed via
ToBifrostResponsesStreamResponse()method with the accumulator (core/providers/openai/openai.go:870)- Tool call streaming is fully supported in the conversion path, including function call arguments delta emission (core/schemas/mux.go:1111-1207)
The removal of the
GetResponsesChunkConverterCombinedPostHookRunnerwrapper and direct passing ofpostHookRunneris correct—the conversion logic is properly delegated to the OpenAI streaming handler via context detection.
e1dc603 to
bf47123
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/anthropic/responses.go (1)
484-610: Guard MessageID before formatting item IDs.Lines 484 and 606 build IDs with
fmt.Sprintf("msg_%s...", *accumulator.MessageID, …)prior to checking whetherMessageIDexists. Anthropic can emit chunks without an ID, so this will panic and abort streaming. Please compute the fallback ID first and only append the message prefix when the pointer is non-nil.- itemID := fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } + itemID := fmt.Sprintf("item_%d", outputIndex) + if accumulator.MessageID != nil && *accumulator.MessageID != "" { + itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) + }Apply the same pattern to the reasoning branch just below.
♻️ Duplicate comments (2)
core/schemas/mux.go (1)
1059-1087: Avoid panicking when MessageID is absent.Line 1059 calls
fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, …)before confirmingaccumulator.MessageIDis non-nil. Several providers legitimately emit chunks without IDs, so this path will panic and kill the stream. Please compute the fallback ID first and only touch*accumulator.MessageIDwhen it’s available.- itemID := fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } + itemID := fmt.Sprintf("item_%d", outputIndex) + if accumulator.MessageID != nil && *accumulator.MessageID != "" { + itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) + }core/providers/cohere/responses.go (1)
576-632: Handle missing MessageID without panicking.Lines 577 and 606 dereference
*accumulator.MessageIDwhile building the item ID, and only afterwards fall back toitem_%d. When Cohere omits a message ID (which it does on some streams), this crashes the goroutine. Please flip the logic so the fallback is computed first, and touch*accumulator.MessageIDonly when it’s non-nil/non-empty.- itemID := fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } + itemID := fmt.Sprintf("item_%d", outputIndex) + if accumulator.MessageID != nil && *accumulator.MessageID != "" { + itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) + }Please apply the same guard to the reasoning branch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
core/providers/anthropic/anthropic.go(1 hunks)core/providers/anthropic/responses.go(8 hunks)core/providers/bedrock/bedrock.go(3 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(3 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(1 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(1 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/perplexity/responses.go
- core/providers/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/core-providers/bedrock_test.go
- tests/core-providers/gemini_test.go
- core/providers/vertex/vertex.go
- tests/core-providers/azure_test.go
- tests/core-providers/groq_test.go
- tests/core-providers/config/account.go
- core/providers/cerebras.go
- core/providers/perplexity/perplexity.go
- core/providers/sgl.go
- tests/core-providers/mistral_test.go
- tests/core-providers/anthropic_test.go
- tests/core-providers/parasail_test.go
- tests/core-providers/sgl_test.go
🧰 Additional context used
🧬 Code graph analysis (14)
core/providers/groq.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
tests/core-providers/tests.go (1)
tests/core-providers/scenarios/tool_calls_streaming.go (1)
RunToolCallsStreamingTest(217-741)
core/providers/gemini/gemini.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/openai/openai.go (4)
core/schemas/mux.go (2)
ChatToResponsesStreamAccumulator(961-976)NewChatToResponsesStreamAccumulator(979-989)core/schemas/bifrost.go (8)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)BifrostError(351-360)ErrorField(369-376)BifrostErrorExtraFields(418-422)RequestType(81-81)ResponsesStreamRequest(90-90)BifrostContextKeyStreamEndIndicator(111-111)ChatCompletionStreamRequest(88-88)core/schemas/responses.go (2)
ResponsesStreamResponseTypeError(1409-1409)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/utils/utils.go (4)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)ProviderSendsDoneMarker(760-769)ProcessAndSendError(605-651)
core/providers/ollama.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/bedrock/responses.go (2)
core/providers/bedrock/types.go (1)
BedrockStreamEvent(363-380)core/schemas/responses.go (18)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
core/schemas/mux.go (4)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(25-40)core/schemas/responses.go (19)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1378-1378)ResponsesStreamResponseTypeRefusalDelta(1363-1363)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeCompleted(1350-1350)core/schemas/utils.go (1)
Ptr(14-16)core/schemas/bifrost.go (2)
RequestType(81-81)ResponsesStreamRequest(90-90)
core/providers/mistral/mistral.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/cohere/responses.go (2)
core/providers/cohere/types.go (13)
CohereStreamEvent(381-386)StreamEventMessageStart(366-366)StreamEventContentStart(367-367)CohereContentBlockTypeText(128-128)CohereContentBlockTypeThinking(130-130)StreamEventContentDelta(368-368)StreamEventContentEnd(369-369)StreamEventToolPlanDelta(370-370)StreamEventToolCallStart(371-371)StreamEventToolCallDelta(372-372)StreamEventToolCallEnd(373-373)StreamEventCitationEnd(375-375)StreamEventMessageEnd(376-376)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeReasoning(297-297)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeOutputTextAnnotationAdded(1401-1401)ResponsesStreamResponseTypeOutputTextAnnotationDone(1402-1402)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/anthropic/responses.go (2)
core/schemas/responses.go (21)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseType(1345-1345)ResponsesStreamResponseTypeMCPCallArgumentsDelta(1386-1386)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesMessageTypeComputerCall(282-282)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesStreamResponseTypeMCPCallArgumentsDone(1387-1387)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/anthropic/types.go (9)
AnthropicStreamEventTypeContentBlockStart(303-303)AnthropicContentBlockTypeText(127-127)AnthropicContentBlockTypeToolUse(129-129)AnthropicContentBlockTypeMCPToolUse(133-133)AnthropicStreamEventTypeContentBlockDelta(304-304)AnthropicStreamDeltaTypeText(326-326)AnthropicStreamDeltaTypeInputJSON(327-327)AnthropicStreamEventTypeMessageDelta(306-306)AnthropicStreamEventTypeMessageStop(302-302)
core/providers/cohere/cohere.go (4)
core/providers/cohere/responses.go (1)
NewCohereStreamAccumulator(26-34)core/schemas/bifrost.go (2)
BifrostResponseExtraFields(282-291)RequestType(81-81)core/providers/utils/utils.go (4)
ShouldSendBackRawResponse(480-485)HandleStreamEndWithSuccess(716-724)GetBifrostResponseForStreamResponse(791-819)ProcessAndSendResponse(533-563)core/schemas/responses.go (1)
BifrostResponsesResponse(45-82)
tests/core-providers/scenarios/tool_calls_streaming.go (3)
core/schemas/chatcompletions.go (5)
ChatAssistantMessageToolCall(483-488)ChatAssistantMessageToolCallFunction(491-494)BifrostChatRequest(11-18)ChatParameters(154-183)ChatTool(201-205)tests/core-providers/scenarios/utils.go (5)
ToolCallInfo(293-297)GetSampleChatTool(129-148)SampleToolTypeWeather(69-69)CreateBasicResponsesMessage(227-235)GetSampleResponsesTool(150-169)core/schemas/responses.go (7)
BifrostResponsesRequest(32-39)ResponsesParameters(84-111)BifrostResponsesStreamResponse(1412-1450)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)
core/providers/bedrock/bedrock.go (4)
core/providers/bedrock/responses.go (2)
NewBedrockStreamAccumulator(28-38)FinalizeBedrockStream(795-886)core/schemas/bifrost.go (2)
BifrostResponseExtraFields(282-291)RequestType(81-81)core/schemas/provider.go (1)
Provider(207-234)core/providers/utils/utils.go (3)
ShouldSendBackRawResponse(480-485)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(791-819)
core/providers/parasail.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (28)
tests/core-providers/cerebras_test.go (1)
40-40: No issues found. Cerebras provider fully supports tool call streaming.The test configuration change at line 40 is correct. The Cerebras provider properly delegates streaming operations to the OpenAI-compatible API handler (
HandleOpenAIChatCompletionStreaming), which includes tool call streaming support. While Cerebras wasn't explicitly listed in the PR summary as a provider receiving a separate accumulator implementation, it was already included via the shared OpenAI-compatible streaming logic. The test will execute correctly when theCEREBRAS_API_KEYenvironment variable is provided.tests/core-providers/ollama_test.go (1)
35-35: LGTM! Test configuration updated correctly.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for Ollama, consistent with the PR's goal to add comprehensive streaming tests across providers.core/providers/parasail.go (1)
145-148: LGTM! Streaming refactored correctly.The changes correctly signal fallback mode via context and pass the postHookRunner directly, aligning with the PR's shift to accumulator-based streaming.
core/schemas/bifrost.go (1)
117-117: LGTM! New context key added correctly.The new
BifrostContextKeyIsResponsesToChatCompletionFallbackconstant follows the existing pattern and enables consistent signaling of fallback streaming behavior across providers.tests/core-providers/openrouter_test.go (1)
37-37: LGTM! Test configuration updated correctly.Enabling
ToolCallsStreamingfor OpenRouter tests is consistent with the PR's comprehensive streaming test coverage.core/providers/groq.go (1)
213-216: LGTM! Streaming refactored correctly.The changes align with the PR's architectural shift, correctly signaling fallback mode and passing postHookRunner directly.
core/providers/ollama.go (1)
173-176: LGTM! Streaming refactored correctly.Consistent with the refactoring pattern across other providers, correctly implementing the fallback streaming mode.
tests/core-providers/openai_test.go (1)
46-46: LGTM! Test configuration updated correctly.Enabling
ToolCallsStreamingfor OpenAI tests completes the comprehensive streaming test coverage across providers.core/providers/gemini/gemini.go (1)
347-350: LGTM! Streaming refactored correctly.The Gemini provider correctly implements the fallback streaming pattern, completing the consistent refactoring across all providers in this PR.
tests/core-providers/vertex_test.go (1)
36-36: LGTM!Enabling
ToolCallsStreamingfor Vertex aligns with the broader PR goal of adding comprehensive streaming tool-call tests across providers.core/providers/mistral/mistral.go (1)
203-209: LGTM!The context flag and raw postHookRunner pass-through align with the broader refactoring pattern across providers. This enables the fallback streaming path for Responses API calls via ChatCompletionStream.
tests/core-providers/tests.go (2)
35-35: LGTM!Adding
RunToolCallsStreamingTestto the test scenario list enables end-to-end streaming validation for tool calls across all providers.
77-77: LGTM!Including
ToolCallsStreamingin the test summary provides clear visibility into which providers support streaming tool calls.core/providers/anthropic/anthropic.go (1)
780-781: LGTM!The comment clarifies that lifecycle events (
response.createdandresponse.in_progress) are now emitted byToBifrostResponsesStreamvia the accumulator, aligning with the broader refactoring to standardize streaming behavior.tests/core-providers/cohere_test.go (1)
36-36: LGTM!Enabling
ToolCallsStreamingfor Cohere aligns with the broader PR goal of adding comprehensive streaming tool-call tests across providers.core/providers/openai/openai.go (4)
704-712: LGTM!The initialization of the fallback path and accumulator is clean. Checking the context flag and conditionally creating
ChatToResponsesStreamAccumulatorenables Responses-style streaming via the ChatCompletionStream pathway.
871-921: Verify ChunkIndex usage aligns with accumulator sequence numbers.The fallback streaming logic correctly handles error and completed events, sets stream end indicators, and propagates per-chunk metadata. However,
ChunkIndexis set toresponse.SequenceNumber(line 904) rather than the loop'schunkIndexvariable. IfSequenceNumberfrom the accumulator can diverge from the localchunkIndex, this could cause confusion or inconsistent indexing.Confirm that using
response.SequenceNumberforChunkIndexis intentional and that it produces the expected monotonic sequence for consumers of the stream.
922-995: LGTM!The non-fallback path preserves existing behavior, including usage aggregation, finish reason handling, and per-chunk metadata. The refactoring cleanly separates the two streaming modes.
1002-1009: LGTM!Skipping the final stream-end handler when in fallback mode is correct, as the fallback path emits its own
Completedevent at line 910-915.core/providers/cohere/cohere.go (4)
632-638: LGTM!The accumulator is correctly initialized outside the event loop to persist state across events, and the model is captured from the request. Deferring
Flush()ensures cleanup even if the goroutine exits early.
676-677: LGTM!The comment clarifies that lifecycle events (
response.createdandresponse.in_progress) are now emitted byToBifrostResponsesStreamvia the accumulator, aligning with the broader refactoring.
679-708: LGTM!The refactored streaming loop correctly handles multi-response events from
ToBifrostResponsesStream. Per-response metadata is populated accurately, and theisLastChunkhandling properly sets the stream end indicator and returns early.
720-721: LGTM!Resetting
eventDataat the end of each outer loop iteration ensures state is cleared for the next SSE event.core/providers/bedrock/responses.go (5)
11-53: LGTM!The
BedrockStreamAccumulatordesign is well-structured with clear mappings for content indices, tool buffers, IDs, and state flags. The constructor andFlushmethod ensure proper initialization and cleanup.
587-660: LGTM!The message start handling correctly emits OpenAI-style lifecycle events (
response.createdandresponse.in_progress) and initializes the text output item with a stable ID. The multi-response return pattern is consistent with the broader refactoring.
662-730: Verify sequenceNumber increment doesn't cause index mismatches.Tool use start handling correctly closes the text item if still open and emits a tool call item. However, line 691 increments
sequenceNumber++locally within this function. This local mutation doesn't affect the caller'ssequenceNumbervariable, which could lead to duplicate or skipped sequence numbers across events.Confirm that the local
sequenceNumber++at line 691 is handled correctly by the caller or consider passing sequenceNumber by reference to ensure consistent tracking across all emitted responses.
733-792: LGTM!Delta handling for text and tool arguments correctly accumulates state in the accumulator and emits per-delta responses. The stop reason handling as a no-op is appropriate since finalization is handled explicitly by
FinalizeBedrockStream.
794-886: LGTM!
FinalizeBedrockStreamcomprehensively closes all open items (text and tool calls), emitsfunction_call_arguments.donewith accumulated arguments, and concludes with aresponse.completedevent including usage. This ensures a complete streaming lifecycle.
bf47123 to
9377f71
Compare
e6b4fba to
34ac867
Compare
Pratham-Mishra04
changed the title
9377f71 to
89a1128
Compare
89a1128 to
af3e591
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/openai/openai.go (1)
998-1009: Ensure Completed is emitted in fallback mode on EOFIn Chat→Responses fallback, if the stream ends without an explicit completed event, nothing is emitted at EOF. Emit response.completed using the accumulator to avoid hanging clients.
- // Handle scanner errors first + // Handle scanner errors first if err := scanner.Err(); err != nil { logger.Warn(fmt.Sprintf("Error reading stream: %v", err)) providerUtils.ProcessAndSendError(ctx, postHookRunner, err, responseChan, schemas.ChatCompletionStreamRequest, providerName, request.Model, logger) - } else if !isChatCompletionsResponsesFallback { + } else if !isChatCompletionsResponsesFallback { response := providerUtils.CreateBifrostChatCompletionChunkResponse(messageID, usage, finishReason, chunkIndex, schemas.ChatCompletionStreamRequest, providerName, request.Model) if postResponseConverter != nil { response = postResponseConverter(response) } response.ExtraFields.Latency = time.Since(startTime).Milliseconds() providerUtils.HandleStreamEndWithSuccess(ctx, providerUtils.GetBifrostResponseForStreamResponse(nil, response, nil, nil, nil), postHookRunner, responseChan) + } else { + // Fallback mode: force a response.completed if not already sent + completed := &schemas.BifrostResponsesStreamResponse{ + Type: schemas.ResponsesStreamResponseTypeCompleted, + SequenceNumber: 0, + Response: &schemas.BifrostResponsesResponse{ + CreatedAt: responsesStreamAccumulator.CreatedAt, + ID: responsesStreamAccumulator.MessageID, + }, + } + completed.ExtraFields.RequestType = schemas.ResponsesStreamRequest + completed.ExtraFields.Provider = providerName + completed.ExtraFields.ModelRequested = request.Model + completed.ExtraFields.ChunkIndex = responsesStreamAccumulator.SequenceNumber + completed.ExtraFields.Latency = time.Since(startTime).Milliseconds() + ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) + providerUtils.HandleStreamEndWithSuccess(ctx, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, completed, nil, nil), postHookRunner, responseChan) }core/providers/cohere/cohere.go (1)
445-466: Fix double increment of chunkIndex in chat streamingchunkIndex++ occurs twice per event, causing skipped indices and wrong latencies. Keep one increment.
- chunkIndex++ ... - lastChunkTime = time.Now() - chunkIndex++ + lastChunkTime = time.Now()Optional: initialize chunkIndex to -1 and increment once right before sending to make first chunk index 0.
♻️ Duplicate comments (4)
core/schemas/mux.go (2)
1058-1066: Guard MessageID before formatting the text item ID.If a provider omits
cr.ID,accumulator.MessageIDstays nil and thefmt.Sprintf("msg_%s…")dereference panics before the fallback path runs. Please guard the dereference first so we safely fall back to theitem_%dform.Apply this diff to fix the panic:
- itemID := fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } + var itemID string + if accumulator.MessageID != nil && *accumulator.MessageID != "" { + itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("item_%d", outputIndex) + }
1255-1288: Copy the accumulated args before taking its address.Taking
&argsfrom the range loop reuses the same pointer for every tool call, so eachfunction_call_arguments.donechunk ends up with the last arguments string. Please copy the string before taking its address.Apply this diff to keep each response stable:
- for toolCallID, args := range accumulator.ToolArgumentBuffers { + for toolCallID, args := range accumulator.ToolArgumentBuffers { if args != "" { outputIndex := accumulator.ToolCallOutputIndices[toolCallID] itemID := accumulator.ItemIDs[toolCallID] contentIndex := 1 // Tool calls use content_index:1 - // Emit function_call_arguments.done with full arguments (no item field, just item_id and arguments) - response := &BifrostResponsesStreamResponse{ + // Emit function_call_arguments.done with full arguments (no item field, just item_id and arguments) + argsCopy := args + response := &BifrostResponsesStreamResponse{ Type: ResponsesStreamResponseTypeFunctionCallArgumentsDone, SequenceNumber: accumulator.SequenceNumber, OutputIndex: Ptr(outputIndex), ContentIndex: Ptr(contentIndex), - Arguments: &args, + Arguments: &argsCopy, ExtraFields: cr.ExtraFields, }core/providers/anthropic/responses.go (1)
483-491: Fix nil deref when generating text item IDsDereferencing accumulator.MessageID before nil check can panic on early text deltas. Compute fallback first, then override when MessageID is set.
Apply:
- itemID := fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } + itemID := fmt.Sprintf("item_%d", outputIndex) + if accumulator.MessageID != nil && *accumulator.MessageID != "" { + itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) + }core/providers/cohere/responses.go (1)
576-580: 🔴 Critical: Nil pointer dereference will cause panic.Line 577 dereferences
*accumulator.MessageIDin the format string before line 578 checks if it's nil. When Cohere emitsmessage-startwithout an ID, this will panic.A previous review already flagged this exact issue, but the fix wasn't applied correctly.
Apply this diff:
-// Generate stable ID for text item -itemID := fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) -if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) -} +// Generate stable ID for text item +var itemID string +if accumulator.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) +} else { + itemID = fmt.Sprintf("item_%d", outputIndex) +}
🧹 Nitpick comments (3)
core/providers/anthropic/responses.go (2)
529-535: Include ContentIndex in tool call item-added eventsFor consistency with text blocks and downstream consumers, also set ContentIndex on function_call item-added.
return []*schemas.BifrostResponsesStreamResponse{{ Type: schemas.ResponsesStreamResponseTypeOutputItemAdded, SequenceNumber: sequenceNumber, OutputIndex: schemas.Ptr(outputIndex), - Item: item, + ContentIndex: chunk.Index, + Item: item, }}, nil, false
543-551: Set MCP tool call status to in_progress on startAlign MCP start semantics with function_call starts by marking status "in_progress".
item := &schemas.ResponsesMessage{ ID: chunk.ContentBlock.ID, - Type: schemas.Ptr(schemas.ResponsesMessageTypeMCPCall), + Type: schemas.Ptr(schemas.ResponsesMessageTypeMCPCall), + Status: schemas.Ptr("in_progress"), ResponsesToolMessage: &schemas.ResponsesToolMessage{core/providers/cohere/cohere.go (1)
632-706: Responses accumulator usage looks solid; minor nitsGood: per-response ExtraFields, RawResponse gating, finalization on last chunk. Consider zero-based chunk indices by starting at -1 for parity with other providers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
core/providers/anthropic/anthropic.go(1 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(8 hunks)core/providers/bedrock/bedrock.go(3 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(3 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(1 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(2 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(1 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/perplexity/responses.go
- core/providers/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (18)
- core/providers/groq.go
- core/providers/vertex/vertex.go
- core/providers/ollama.go
- core/providers/cerebras.go
- core/schemas/bifrost.go
- tests/core-providers/anthropic_test.go
- tests/core-providers/mistral_test.go
- tests/core-providers/cerebras_test.go
- tests/core-providers/config/account.go
- tests/core-providers/groq_test.go
- core/providers/perplexity/perplexity.go
- tests/core-providers/gemini_test.go
- tests/core-providers/ollama_test.go
- tests/core-providers/scenarios/tool_calls_streaming.go
- tests/core-providers/sgl_test.go
- core/providers/anthropic/anthropic.go
- tests/core-providers/openai_test.go
- tests/core-providers/cohere_test.go
🧰 Additional context used
🧬 Code graph analysis (13)
core/providers/mistral/mistral.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/parasail.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/cohere/cohere.go (4)
core/providers/cohere/responses.go (1)
NewCohereStreamAccumulator(26-34)core/schemas/bifrost.go (2)
BifrostResponseExtraFields(282-291)RequestType(81-81)core/providers/utils/utils.go (4)
ShouldSendBackRawResponse(480-485)HandleStreamEndWithSuccess(716-724)GetBifrostResponseForStreamResponse(791-819)ProcessAndSendResponse(533-563)core/schemas/responses.go (1)
BifrostResponsesResponse(45-82)
core/providers/openrouter.go (1)
core/providers/utils/utils.go (1)
GetPathFromContext(209-214)
tests/core-providers/tests.go (1)
tests/core-providers/scenarios/tool_calls_streaming.go (1)
RunToolCallsStreamingTest(217-741)
core/providers/cohere/responses.go (2)
core/providers/cohere/types.go (13)
CohereStreamEvent(381-386)StreamEventMessageStart(366-366)StreamEventContentStart(367-367)CohereContentBlockTypeText(128-128)CohereContentBlockTypeThinking(130-130)StreamEventContentDelta(368-368)StreamEventContentEnd(369-369)StreamEventToolPlanDelta(370-370)StreamEventToolCallStart(371-371)StreamEventToolCallDelta(372-372)StreamEventToolCallEnd(373-373)StreamEventCitationEnd(375-375)StreamEventMessageEnd(376-376)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeReasoning(297-297)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeOutputTextAnnotationAdded(1401-1401)ResponsesStreamResponseTypeOutputTextAnnotationDone(1402-1402)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/openai/openai.go (4)
core/schemas/mux.go (2)
ChatToResponsesStreamAccumulator(961-976)NewChatToResponsesStreamAccumulator(979-989)core/schemas/bifrost.go (8)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)BifrostError(351-360)ErrorField(369-376)BifrostErrorExtraFields(418-422)RequestType(81-81)ResponsesStreamRequest(90-90)BifrostContextKeyStreamEndIndicator(111-111)ChatCompletionStreamRequest(88-88)core/schemas/responses.go (2)
ResponsesStreamResponseTypeError(1409-1409)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/utils/utils.go (5)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(791-819)ProviderSendsDoneMarker(760-769)ProcessAndSendError(605-651)
core/providers/anthropic/responses.go (2)
core/schemas/responses.go (17)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseType(1345-1345)ResponsesStreamResponseTypeMCPCallArgumentsDelta(1386-1386)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesStreamResponseTypeMCPCallArgumentsDone(1387-1387)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/anthropic/types.go (9)
AnthropicStreamEventTypeContentBlockStart(303-303)AnthropicContentBlockTypeText(127-127)AnthropicContentBlockTypeToolUse(129-129)AnthropicContentBlockTypeMCPToolUse(133-133)AnthropicStreamEventTypeContentBlockDelta(304-304)AnthropicStreamDeltaTypeText(326-326)AnthropicStreamDeltaTypeInputJSON(327-327)AnthropicStreamEventTypeMessageDelta(306-306)AnthropicStreamEventTypeMessageStop(302-302)
core/providers/bedrock/bedrock.go (3)
core/providers/bedrock/responses.go (2)
NewBedrockStreamAccumulator(28-38)FinalizeBedrockStream(795-886)core/schemas/bifrost.go (2)
BifrostResponseExtraFields(282-291)RequestType(81-81)core/providers/utils/utils.go (3)
ShouldSendBackRawResponse(480-485)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(791-819)
core/providers/bedrock/responses.go (2)
core/providers/bedrock/types.go (1)
BedrockStreamEvent(363-380)core/schemas/responses.go (18)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
core/schemas/mux.go (4)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(25-40)core/schemas/responses.go (17)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1378-1378)ResponsesStreamResponseTypeRefusalDelta(1363-1363)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeCompleted(1350-1350)core/schemas/utils.go (1)
Ptr(14-16)core/schemas/bifrost.go (2)
RequestType(81-81)ResponsesStreamRequest(90-90)
core/providers/sgl.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/gemini/gemini.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
🔇 Additional comments (19)
core/providers/anthropic/chat.go (1)
705-718: LGTM! Correct alignment of tool call ID source.The change from
ContentBlock.ToolUseIDtoContentBlock.IDproperly aligns the streaming path with the rest of the file. Throughout this file,ContentBlock.IDis consistently used for tool_use blocks (lines 135, 288, 573, 679, 887), whileToolUseIDis reserved for tool_result blocks that reference a previous tool use. This fix ensures the streaming implementation follows the same pattern.tests/core-providers/openrouter_test.go (1)
37-37: ****The change is correct and follows the established pattern across all providers. ToolCallsStreaming is intentionally enabled for OpenRouter (consistent with OpenAI, Anthropic, Bedrock, Cohere, and 10+ other providers), and the test infrastructure properly supports this scenario. TestOpenRouter is part of the standard test suite and will execute the ToolCallsStreamingResponses test when the OPENROUTER_API_KEY is provided. While some Beta features are disabled (End2EndToolCalling, ImageURL, etc.), ToolCallsStreaming is explicitly enabled.
tests/core-providers/parasail_test.go (1)
35-36: Streaming scenario flag looks good.This keeps the Parasail comprehensive suite aligned with the new tool-call streaming harness.
tests/core-providers/vertex_test.go (1)
36-37: Vertex streaming flag aligns with the new tests.Enabling ToolCallsStreaming here ensures the Vertex run exercises the new accumulator flow.
tests/core-providers/azure_test.go (1)
42-43: Azure config keeps parity with the streaming coverage.Even with the skip in place, this flag keeps Azure’s scenario matrix consistent with other providers.
tests/core-providers/tests.go (1)
35-36: Exercise and report the new streaming scenario.Wiring
RunToolCallsStreamingTestinto the suite and summary gives us parity with the added config flag.Also applies to: 77-78
tests/core-providers/bedrock_test.go (1)
40-41: LGTM: ToolCallsStreaming enabledThis exercises the new accumulator-based tool-call streaming path in Bedrock tests.
core/providers/parasail.go (1)
145-151: LGTM: Responses→Chat fallback correctly wiredContext flag + direct postHookRunner enables OpenAI-style Responses streaming via fallback.
core/providers/sgl.go (1)
170-176: LGTM: Responses→Chat fallback pathConsistent context flag and hook handling for streaming.
core/providers/mistral/mistral.go (1)
203-210: LGTM: Responses→Chat fallback pathContext flag used; passes postHookRunner directly.
core/providers/gemini/gemini.go (1)
346-354: LGTM: Responses→Chat fallback pathFlag + direct hook aligns Gemini streaming with shared fallback.
core/providers/bedrock/responses.go (3)
11-53: LGTM! Clean accumulator design.The
BedrockStreamAccumulatorstructure and initialization are well-designed, tracking all necessary state for streaming conversion (content-to-output mappings, tool buffers, IDs, and lifecycle flags). TheFlush()method properly resets all state for potential reuse.
587-660: Lifecycle event emission looks correct.The message start handling properly:
- Generates a stable message ID when needed
- Guards against duplicate
created/in_progressevents with flags- Emits
output_item.addedfor the initial text message- Dereferences
MessageIDsafely (it's guaranteed non-nil by lines 594-597)
794-886: Comprehensive finalization logic.
FinalizeBedrockStreamproperly closes all open items:
- Text items with
output_item.done- Tool calls with both
function_call_arguments.doneandoutput_item.done- Final
response.completedwith usageThe sequence number increment (
sequenceNumber + len(responses)) ensures unique sequence numbers across all emitted events.core/providers/bedrock/bedrock.go (3)
869-872: Proper accumulator lifecycle management.The accumulator is created once per stream, model is set, and deferred flush ensures cleanup. This is the correct pattern for managing stateful streaming conversions.
884-904: Clean EOF finalization.On
io.EOF,FinalizeBedrockStreamis called to produce closing events, and each final response is emitted with proper metadata. The chunk index increment and latency tracking are correctly applied.
944-964: Multi-response emission pattern is sound.Iterating over the slice returned by
ToBifrostResponsesStreamand emitting each response with per-response metadata (ChunkIndex, Latency, RawResponse) follows the new streaming contract correctly.core/providers/cohere/responses.go (2)
11-67: LGTM! Accumulator design is sound.The
CohereStreamAccumulatorstructure mirrors the Bedrock pattern effectively. TheGetOrCreateOutputIndexhelper is a nice addition that simplifies content-to-output mapping throughout the streaming logic.
736-808: Tool call handling looks solid.The tool call lifecycle (start → deltas → end) properly:
- Closes any open tool plan item at start
- Assigns unique output indices to avoid collision with text
- Accumulates arguments across deltas
- Emits both
function_call_arguments.doneandoutput_item.doneat end
Pratham-Mishra04
changed the title
af3e591 to
eacd85a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/schemas/mux.go (1)
610-614: Fix nil MessageID dereference in reasoning item IDs.
fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, …)is executed before you confirmaccumulator.MessageIDis non-nil. Cohere, Bedrock, and Anthropic frequently omitmessage.idon early reasoning chunks, so this panics and kills the stream. Build the item ID only after the nil check so we safely fall back to the anonymous ID path.- // Generate stable ID for reasoning item - itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("reasoning_%d", outputIndex) - } + // Generate stable ID for reasoning item + var itemID string + if accumulator.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("reasoning_%d", outputIndex) + }
♻️ Duplicate comments (1)
core/providers/cohere/responses.go (1)
611-614: Prevent nil MessageID panic when emitting reasoning items.
fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, …)still dereferencesMessageIDbefore you know it’s set. Cohere emitsmessage-startwithout IDs, so this panics on the very first reasoning chunk and drops the stream. Guard the dereference exactly as you did for text/tool-plan IDs.- // Generate stable ID for reasoning item - itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("reasoning_%d", outputIndex) - } + // Generate stable ID for reasoning item + var itemID string + if accumulator.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("reasoning_%d", outputIndex) + }
🧹 Nitpick comments (4)
core/providers/cohere/cohere.go (2)
651-657: SSE parsing should accumulate multi-line data blocks before unmarshalingYou parse each "data: " line as a complete event. SSE events can contain multiple data lines and are delimited by a blank line. This can split JSON across lines and cause parse errors or truncated events. Accumulate consecutive "data:" lines and only unmarshal on blank-line delimiter, then clear the buffer.
Minimal sketch:
- var eventData string + var eventBuf []string ... - if after, ok := strings.CutPrefix(line, "data: "); ok { - eventData = after - } else { + if after, ok := strings.CutPrefix(line, "data: "); ok { + eventBuf = append(eventBuf, after) + continue + } + // blank line signals end-of-event + if line == "" && len(eventBuf) > 0 { + eventData := strings.Join(eventBuf, "\n") + eventBuf = eventBuf[:0] + // unmarshal eventData and process... + // ... + continue + } else { continue - } ... - // Reset for next event - eventData = "" + // eventBuf is reset when blank line is seenAlso applies to: 720-722
635-637: Optional: Remove or use accumulator.ModelYou set accumulator.Model but don’t use it in this function. Either use it when building envelopes inside ToBifrostResponsesStream or drop the assignment to avoid confusion.
core/providers/bedrock/responses.go (2)
635-641: Avoid “msg_msg_…” IDs for text itemsMessageID is already prefixed with "msg_". Formatting
msg_%s_item_%dyields "msg_msg__item_0".Use:
- itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) + itemID = fmt.Sprintf("%s_item_%d", *accumulator.MessageID, outputIndex)
824-875: Finalize should also close tool-call items with empty argumentsYou only emit arguments.done + output_item.done when args != "". If a tool call streamed with zero-length args and the stream ends before content_block_stop, the item may remain open.
Consider emitting at least
response.output_item.donefor any outputIndex present in ToolArgumentBuffers or ItemIDs, even whenargs == "". Example:- for outputIndex, args := range accumulator.ToolArgumentBuffers { - if args != "" { + for outputIndex, args := range accumulator.ToolArgumentBuffers { + if true { // always check for closure; gate arguments.done on args != "" // ... emit arguments.done only if args != "" if args != "" { // existing arguments.done emission } // always emit output_item.done // existing output_item.done emission } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
core/changelog.md(1 hunks)core/providers/anthropic/anthropic.go(1 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(8 hunks)core/providers/bedrock/bedrock.go(3 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(3 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(1 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(2 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/utils/utils.go
- core/providers/perplexity/responses.go
🚧 Files skipped from review as they are similar to previous changes (22)
- core/providers/ollama.go
- tests/core-providers/parasail_test.go
- core/providers/gemini/gemini.go
- core/providers/mistral/mistral.go
- tests/core-providers/sgl_test.go
- tests/core-providers/openai_test.go
- core/providers/sgl.go
- tests/core-providers/gemini_test.go
- tests/core-providers/ollama_test.go
- core/providers/anthropic/chat.go
- tests/core-providers/azure_test.go
- core/providers/parasail.go
- tests/core-providers/cohere_test.go
- core/providers/vertex/vertex.go
- tests/core-providers/bedrock_test.go
- tests/core-providers/vertex_test.go
- core/schemas/bifrost.go
- tests/core-providers/groq_test.go
- tests/core-providers/tests.go
- tests/core-providers/openrouter_test.go
- core/providers/groq.go
- tests/core-providers/mistral_test.go
🧰 Additional context used
🧬 Code graph analysis (11)
core/providers/openrouter.go (1)
core/providers/utils/utils.go (1)
GetPathFromContext(209-214)
core/providers/cohere/responses.go (2)
core/providers/cohere/types.go (13)
CohereStreamEvent(381-386)StreamEventMessageStart(366-366)StreamEventContentStart(367-367)CohereContentBlockTypeText(128-128)CohereContentBlockTypeThinking(130-130)StreamEventContentDelta(368-368)StreamEventContentEnd(369-369)StreamEventToolPlanDelta(370-370)StreamEventToolCallStart(371-371)StreamEventToolCallDelta(372-372)StreamEventToolCallEnd(373-373)StreamEventCitationEnd(375-375)StreamEventMessageEnd(376-376)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeReasoning(297-297)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeOutputTextAnnotationAdded(1401-1401)ResponsesStreamResponseTypeOutputTextAnnotationDone(1402-1402)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/perplexity/perplexity.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/bedrock/bedrock.go (4)
core/providers/bedrock/responses.go (2)
NewBedrockStreamAccumulator(28-38)FinalizeBedrockStream(800-891)core/schemas/bifrost.go (3)
BifrostResponseExtraFields(282-291)RequestType(81-81)ResponsesStreamRequest(90-90)core/schemas/provider.go (1)
Provider(207-234)core/providers/utils/utils.go (3)
ShouldSendBackRawResponse(480-485)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(791-819)
core/schemas/mux.go (3)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(25-40)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1378-1378)ResponsesStreamResponseTypeRefusalDelta(1363-1363)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeCompleted(1350-1350)core/schemas/utils.go (1)
Ptr(14-16)
core/providers/openai/openai.go (5)
core/schemas/mux.go (2)
ChatToResponsesStreamAccumulator(961-977)NewChatToResponsesStreamAccumulator(980-991)core/schemas/bifrost.go (8)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)BifrostError(351-360)ErrorField(369-376)BifrostErrorExtraFields(418-422)RequestType(81-81)ResponsesStreamRequest(90-90)BifrostContextKeyStreamEndIndicator(111-111)ChatCompletionStreamRequest(88-88)core/schemas/responses.go (2)
ResponsesStreamResponseTypeError(1409-1409)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/utils/utils.go (5)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(791-819)ProviderSendsDoneMarker(760-769)ProcessAndSendError(605-651)core/schemas/chatcompletions.go (1)
ChatStreamResponseChoice(529-531)
core/providers/cerebras.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/bedrock/responses.go (2)
core/providers/bedrock/types.go (1)
BedrockStreamEvent(363-380)core/schemas/responses.go (18)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
tests/core-providers/scenarios/tool_calls_streaming.go (3)
core/schemas/chatcompletions.go (5)
ChatAssistantMessageToolCall(483-488)ChatAssistantMessageToolCallFunction(491-494)BifrostChatRequest(11-18)ChatParameters(154-183)ChatTool(201-205)tests/core-providers/scenarios/utils.go (5)
ToolCallInfo(293-297)CreateBasicChatMessage(218-225)GetSampleChatTool(129-148)CreateBasicResponsesMessage(227-235)GetSampleResponsesTool(150-169)core/schemas/responses.go (5)
BifrostResponsesStreamResponse(1412-1450)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)
core/providers/anthropic/responses.go (2)
core/schemas/responses.go (21)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseType(1345-1345)ResponsesStreamResponseTypeMCPCallArgumentsDelta(1386-1386)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesMessageTypeComputerCall(282-282)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesStreamResponseTypeMCPCallArgumentsDone(1387-1387)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/anthropic/types.go (9)
AnthropicStreamEventTypeContentBlockStart(303-303)AnthropicContentBlockTypeText(127-127)AnthropicContentBlockTypeToolUse(129-129)AnthropicContentBlockTypeMCPToolUse(133-133)AnthropicStreamEventTypeContentBlockDelta(304-304)AnthropicStreamDeltaTypeText(326-326)AnthropicStreamDeltaTypeInputJSON(327-327)AnthropicStreamEventTypeMessageDelta(306-306)AnthropicStreamEventTypeMessageStop(302-302)
core/providers/cohere/cohere.go (4)
core/providers/cohere/responses.go (1)
NewCohereStreamAccumulator(26-34)core/schemas/bifrost.go (2)
BifrostResponseExtraFields(282-291)RequestType(81-81)core/providers/utils/utils.go (4)
ShouldSendBackRawResponse(480-485)HandleStreamEndWithSuccess(716-724)GetBifrostResponseForStreamResponse(791-819)ProcessAndSendResponse(533-563)core/schemas/responses.go (1)
BifrostResponsesResponse(45-82)
🔇 Additional comments (5)
core/changelog.md (1)
1-3: Verify changelog entries align with PR scope and clarify entry 2.The PR introduces significant streaming refactors across multiple providers (Anthropic, Bedrock, Cohere, etc.) with stateful accumulators and lifecycle events. However, only two new changelog entries are present:
- Entry on line 2 ("responses text output standardization to content blocks") is vague and doesn't convey what "standardization" means or its impact to users.
- Entry on line 3 (openrouter endpoint shift) is clear and specific.
- The removed entry on line 1 lacks context—why was it removed? If superseded, clarify the relationship.
Please clarify whether these entries adequately represent the scope of user-facing changes, or if additional entries are needed for the streaming accumulator and lifecycle event improvements.
core/providers/perplexity/perplexity.go (1)
211-219: LGTM! Clean refactor aligning with PR objectives.The changes properly inject the fallback context key and simplify the hook runner usage by removing the wrapper layer. This aligns with the broader pattern applied across providers in this PR and improves code maintainability.
core/providers/openrouter.go (1)
214-214: OpenRouter/v1/responsesendpoint is supported — changes are correct.OpenRouter provides a Responses API at the v1 endpoint and it is OpenAI Responses‑compatible. The migration from
/alpha/responsesto/v1/responseson lines 214 and 233 is appropriate and consistent across both the non-streaming and streaming response methods.core/providers/cerebras.go (1)
172-180: LGTM! Clean refactoring aligned with the broader streaming improvements.The changes correctly implement the Responses-to-Chat fallback pattern:
- The context flag properly signals the fallback path to downstream handlers
- Passing
postHookRunnerdirectly simplifies the architecture by removing the wrapper layerThese changes align with the PR's goal of standardizing streaming across providers.
Optional verification to ensure integration completeness:
core/providers/anthropic/responses.go (1)
484-491: LGTM: Safe text item ID generationNil-check before dereferencing MessageID prevents panics on reordered/partial streams. Good fix.
eacd85a to
6c2ea61
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
core/providers/cohere/responses.go (1)
610-614: Nil-pointer panic when building reasoning item ID.Dereferences *accumulator.MessageID before nil-check. Reorder.
Apply:
- itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("reasoning_%d", outputIndex) - } + var itemID string + if accumulator.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("reasoning_%d", outputIndex) + }core/providers/bedrock/bedrock.go (1)
885-905: Signal stream completion and set total latency on the last final response.Finalize emits responses but never calls HandleStreamEndWithSuccess; also the final response latency should be total.
Apply:
- for _, finalResponse := range finalResponses { + for i, finalResponse := range finalResponses { finalResponse.ExtraFields = schemas.BifrostResponseExtraFields{ RequestType: schemas.ResponsesStreamRequest, Provider: providerName, ModelRequested: request.Model, ModelDeployment: deployment, ChunkIndex: chunkIndex, - Latency: time.Since(lastChunkTime).Milliseconds(), + Latency: time.Since(lastChunkTime).Milliseconds(), } chunkIndex++ lastChunkTime = time.Now() if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { finalResponse.ExtraFields.RawResponse = "{}" // Final event has no payload } - providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, finalResponse, nil, nil), responseChan) + // Ensure the very last response signals stream end and uses total latency + if i == len(finalResponses)-1 { + finalResponse.ExtraFields.Latency = time.Since(startTime).Milliseconds() + providerUtils.HandleStreamEndWithSuccess( + ctx, + providerUtils.GetBifrostResponseForStreamResponse(nil, nil, finalResponse, nil, nil), + postHookRunner, + responseChan, + ) + } else { + providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, finalResponse, nil, nil), responseChan) + } }
🧹 Nitpick comments (3)
core/schemas/mux.go (1)
1112-1221: Handle multiple tool_call deltas per chunk, not just the first.Only delta.ToolCalls[0] is processed; chunks may carry multiple tool calls. Iterate and emit for each to avoid lost calls.
If helpful, I can draft a safe loop that preserves sequenceNumber per emitted response.
core/providers/cohere/responses.go (2)
576-586: Remove redundant fallback assignment.itemID is set with a nil-safe branch already; the second
if accumulator.MessageID == nil { ... }is dead code.Apply:
- if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - }
804-810: Include ContentIndex on tool-call start events.For parity with deltas and ends, attach ContentIndex to output_item.added for tool calls.
Apply:
- responses = append(responses, &schemas.BifrostResponsesStreamResponse{ + responses = append(responses, &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeOutputItemAdded, SequenceNumber: sequenceNumber + len(responses), OutputIndex: schemas.Ptr(outputIndex), - Item: item, + ContentIndex: chunk.Index, + Item: item, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
core/changelog.md(1 hunks)core/providers/anthropic/anthropic.go(1 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(8 hunks)core/providers/bedrock/bedrock.go(3 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(3 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(1 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(2 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/perplexity/responses.go
- core/providers/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (22)
- tests/core-providers/tests.go
- tests/core-providers/anthropic_test.go
- core/providers/vertex/vertex.go
- core/providers/ollama.go
- tests/core-providers/azure_test.go
- core/providers/cerebras.go
- tests/core-providers/cohere_test.go
- tests/core-providers/vertex_test.go
- tests/core-providers/config/account.go
- core/providers/groq.go
- core/providers/mistral/mistral.go
- core/providers/anthropic/anthropic.go
- core/providers/openrouter.go
- core/changelog.md
- core/providers/gemini/gemini.go
- tests/core-providers/openrouter_test.go
- tests/core-providers/sgl_test.go
- tests/core-providers/bedrock_test.go
- tests/core-providers/scenarios/tool_calls_streaming.go
- core/providers/openai/openai.go
- tests/core-providers/openai_test.go
- core/providers/parasail.go
🧰 Additional context used
🧬 Code graph analysis (8)
core/providers/sgl.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/perplexity/perplexity.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/bedrock/responses.go (2)
core/providers/bedrock/types.go (1)
BedrockStreamEvent(363-380)core/schemas/responses.go (15)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/anthropic/responses.go (2)
core/schemas/responses.go (21)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseType(1345-1345)ResponsesStreamResponseTypeMCPCallArgumentsDelta(1386-1386)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesMessageTypeComputerCall(282-282)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesStreamResponseTypeMCPCallArgumentsDone(1387-1387)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/anthropic/types.go (9)
AnthropicStreamEventTypeContentBlockStart(303-303)AnthropicContentBlockTypeText(127-127)AnthropicContentBlockTypeToolUse(129-129)AnthropicContentBlockTypeMCPToolUse(133-133)AnthropicStreamEventTypeContentBlockDelta(304-304)AnthropicStreamDeltaTypeText(326-326)AnthropicStreamDeltaTypeInputJSON(327-327)AnthropicStreamEventTypeMessageDelta(306-306)AnthropicStreamEventTypeMessageStop(302-302)
core/schemas/mux.go (4)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(25-40)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1378-1378)ResponsesStreamResponseTypeRefusalDelta(1363-1363)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeCompleted(1350-1350)core/schemas/utils.go (1)
Ptr(14-16)core/schemas/bifrost.go (2)
RequestType(81-81)ResponsesStreamRequest(90-90)
core/providers/cohere/responses.go (2)
core/providers/cohere/types.go (13)
CohereStreamEvent(381-386)StreamEventMessageStart(366-366)StreamEventContentStart(367-367)CohereContentBlockTypeText(128-128)CohereContentBlockTypeThinking(130-130)StreamEventContentDelta(368-368)StreamEventContentEnd(369-369)StreamEventToolPlanDelta(370-370)StreamEventToolCallStart(371-371)StreamEventToolCallDelta(372-372)StreamEventToolCallEnd(373-373)StreamEventCitationEnd(375-375)StreamEventMessageEnd(376-376)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeReasoning(297-297)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeOutputTextAnnotationAdded(1401-1401)ResponsesStreamResponseTypeOutputTextAnnotationDone(1402-1402)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/cohere/cohere.go (4)
core/providers/cohere/responses.go (1)
NewCohereStreamAccumulator(26-34)core/schemas/bifrost.go (2)
BifrostResponseExtraFields(282-291)RequestType(81-81)core/providers/utils/utils.go (4)
ShouldSendBackRawResponse(480-485)HandleStreamEndWithSuccess(716-724)GetBifrostResponseForStreamResponse(791-819)ProcessAndSendResponse(533-563)core/schemas/responses.go (1)
BifrostResponsesResponse(45-82)
core/providers/bedrock/bedrock.go (3)
core/providers/bedrock/responses.go (2)
NewBedrockStreamAccumulator(28-38)FinalizeBedrockStream(799-890)core/schemas/bifrost.go (2)
BifrostResponseExtraFields(282-291)RequestType(81-81)core/providers/utils/utils.go (3)
ShouldSendBackRawResponse(480-485)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(791-819)
🔇 Additional comments (22)
tests/core-providers/cerebras_test.go (1)
40-40: LGTM! Enables streaming test coverage for tool calls.This change correctly enables the new
ToolCallsStreamingtest scenario for Cerebras, aligning with the PR's objective to enhance tool call streaming across providers. The test configuration is syntactically correct and consistent with similar updates across other provider tests.core/providers/perplexity/perplexity.go (1)
212-218: LGTM! Consistent fallback streaming implementation.The changes correctly set the fallback context flag and simplify the call by passing
postHookRunnerdirectly. This aligns with the broader PR pattern for enabling Responses-to-Chat fallback streaming across providers.core/schemas/bifrost.go (1)
103-118: LGTM! New context key properly defined.The addition of
BifrostContextKeyIsResponsesToChatCompletionFallbackenables the fallback streaming behavior across providers. The constant is well-named and follows existing conventions.tests/core-providers/groq_test.go (1)
43-43: LGTM! Enables tool call streaming tests.The addition of
ToolCallsStreaming: trueensures that Groq's streaming tool call functionality is properly tested, aligning with the PR's comprehensive streaming improvements.tests/core-providers/parasail_test.go (1)
35-35: LGTM! Enables tool call streaming tests.Consistent with other providers, this enables streaming tool call tests for Parasail.
tests/core-providers/mistral_test.go (1)
38-38: LGTM! Enables tool call streaming tests.This change ensures Mistral's tool call streaming is tested, consistent with the PR's goals.
tests/core-providers/ollama_test.go (2)
26-26: Model version updated.The model was changed from
"llama3.2"to"llama3.1:latest". Please confirm this change is intentional and that llama3.1 provides the necessary tool call support for the streaming tests.
35-35: LGTM! Enables tool call streaming tests.Consistent with other providers, this enables streaming tool call tests for Ollama.
core/providers/sgl.go (1)
170-176: LGTM! Consistent fallback streaming implementation.The changes correctly implement the fallback streaming pattern, matching the approach used in other providers like Perplexity.
core/providers/anthropic/chat.go (1)
706-732: Tool call ID field change is correct.The struct definition in
core/providers/anthropic/types.goexplicitly documents that theIDfield is fortool_usecontent (line 145), whileToolUseIDis fortool_resultcontent (line 144). The code change to usechunk.ContentBlock.IDis appropriate and aligns with the documented field purposes in the Anthropic content block structure.tests/core-providers/gemini_test.go (1)
41-41: Enablement looks good.ToolCallsStreaming: true aligns Gemini with the new streaming tests. No issues.
core/providers/cohere/cohere.go (2)
632-639: Good accumulator setup and teardown.Creating one CohereStreamAccumulator per stream and flushing on exit is correct. Model propagation via accumulator is fine.
679-707: Per-response metadata and chunkIndex handling look correct.Incrementing chunkIndex per emitted response maintains contiguous sequence numbers; latency is computed per response. LGTM.
core/providers/anthropic/responses.go (9)
35-44: LGTM! Clean initialization of the accumulator state.The constructor properly initializes all maps and sets a consistent
CreatedAttimestamp that will be used across lifecycle events.
62-80: LGTM! Solid mapping logic between Anthropic and OpenAI indexing schemes.The method correctly handles both nil content indices and reuses existing mappings, ensuring consistent output index assignment across multiple events for the same content block.
396-442: LGTM! Proper OpenAI-style lifecycle event emission.The implementation correctly emits both
response.createdandresponse.in_progressevents at message start, using consistent timestamps and sequence numbering. The flags prevent duplicate emissions.
478-507: LGTM! Text block handling correctly generates stable item IDs.The nil check for
MessageID(line 485) is correctly ordered before any dereference, using a fallback format when the ID is unavailable. The past review concern has been properly addressed.
509-536: LGTM! Proper initialization of tool call streaming state.The code correctly initializes the
ToolArgumentBuffersfor accumulating arguments and emitsoutput_item.addedwithin_progressstatus, following the OpenAI streaming pattern.
597-635: LGTM! Tool argument streaming properly differentiates between computer tools and regular tools.The logic correctly accumulates computer tool arguments silently (for later parsing) while streaming regular tool arguments incrementally. The defensive initialization (lines 610-612) guards against edge cases, and the MCP differentiation ensures correct event types are emitted.
715-761: LGTM! Proper completion of tool call streaming with argument finalization.The code correctly emits
arguments.doneevents for accumulated tool arguments before emittingoutput_item.done, and properly cleans up per-output state. The sequence number adjustment accounts for the optionalarguments.doneevent.
764-783: LGTM! Clean stream completion with consistent lifecycle events.The
MessageStophandler properly emitsresponse.completedwith the stored message ID and timestamp, and correctly signals stream completion with the boolean return value. TheMessageDeltapassthrough is appropriate since item-level completion is already handled.
394-809: Excellent refactoring! The OpenAI-style lifecycle events implementation is well-structured.The stateful accumulator design elegantly handles the impedance mismatch between Anthropic's content-indexed streaming and OpenAI's output-indexed format. Key strengths:
- Consistent per-output state: The maps (
ItemIDs,ToolArgumentBuffers,MCPCallOutputIndices) maintain state keyed by output index, ensuring deltas and completions reference the correct items- Proper lifecycle emission: The
created → in_progress → [deltas] → completedflow matches OpenAI's pattern- Special-case handling: Computer tools and MCP calls are correctly differentiated with appropriate early returns and event types
- Sequence integrity: Multiple responses per event correctly adjust sequence numbers
The past nil-dereference concern has been properly addressed (lines 485-489).
f6d7664 to
7a0cd1f
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/anthropic/anthropic.go (1)
469-595: Avoid emitting a “success” final chunk after a streaming error in ChatCompletionWhen
event.ToBifrostChatCompletionStream()returns abifrostErr(Line 548), the code:
- enriches the error with
RequestType,Provider, andModelRequested,- sets
BifrostContextKeyStreamEndIndicator, and- calls
ProcessAndSendBifrostError, thenbreaks the scan loop.Because the goroutine then falls through to the post-loop
elseblock (Lines 591–595), it also constructs and sends a final chat completion “done” chunk viaCreateBifrostChatCompletionChunkResponse, effectively signalling both an error and a successful completion for the same stream.That’s misleading for consumers and inconsistent with the new lifecycle semantics.
You likely want to stop the goroutine entirely after sending the error. A minimal fix is:
- if bifrostErr != nil { - bifrostErr.ExtraFields = schemas.BifrostErrorExtraFields{ - RequestType: schemas.ChatCompletionStreamRequest, - Provider: providerType, - ModelRequested: modelName, - } - ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) - providerUtils.ProcessAndSendBifrostError(ctx, postHookRunner, bifrostErr, responseChan, logger) - break - } + if bifrostErr != nil { + bifrostErr.ExtraFields = schemas.BifrostErrorExtraFields{ + RequestType: schemas.ChatCompletionStreamRequest, + Provider: providerType, + ModelRequested: modelName, + } + ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) + providerUtils.ProcessAndSendBifrostError(ctx, postHookRunner, bifrostErr, responseChan, logger) + return + }This avoids sending a trailing “done” chunk after an error while preserving the enriched error event and stream-end flag.
core/providers/anthropic/responses.go (1)
574-647: Function/MCP arguments deltas: Bifrost-side emission is fine, but back-conversion likely drops themOn
AnthropicStreamDeltaTypeInputJSON, you now:
- Accumulate per-output arguments in
ToolArgumentBuffers[outputIndex].- Emit either
response.function_call_arguments.deltaorresponse.mcp_call_arguments.deltawithDelta = chunk.Delta.PartialJSONandOutputIndex/ItemIDset.This is reasonable for Bifrost’s internal representation. However, in the same file,
ToAnthropicResponsesStreamResponse’sResponsesStreamResponseTypeFunctionCallArgumentsDeltabranch currently reads frombifrostResp.Argumentsrather thanbifrostResp.Deltawhen constructingPartialJSON. That means any deltas produced by this new path (which only setDelta) will result in Anthropic SSE events with an emptypartial_jsonwhen converting back to Anthropic.I’d suggest making the converter tolerant of both fields so we don’t break existing callers:
case schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDelta: streamResp.Type = AnthropicStreamEventTypeContentBlockDelta if bifrostResp.ContentIndex != nil { streamResp.Index = bifrostResp.ContentIndex } - if bifrostResp.Arguments != nil { - streamResp.Delta = &AnthropicStreamDelta{ - Type: AnthropicStreamDeltaTypeInputJSON, - PartialJSON: bifrostResp.Arguments, - } - } + var partialJSON *string + if bifrostResp.Delta != nil { + partialJSON = bifrostResp.Delta + } else if bifrostResp.Arguments != nil { // backward-compat + partialJSON = bifrostResp.Arguments + } + if partialJSON != nil { + streamResp.Delta = &AnthropicStreamDelta{ + Type: AnthropicStreamDeltaTypeInputJSON, + PartialJSON: partialJSON, + } + }This keeps your new emission logic intact while ensuring the reverse mapping remains correct for both new and legacy producers.
♻️ Duplicate comments (2)
core/providers/cohere/responses.go (1)
611-614: 🔴 Critical: Fix nil pointer dereference when generating reasoning item ID.Line 611 dereferences
*accumulator.MessageIDbefore checking for nil on line 612. When Cohere emits a message-start event without an ID, this will panic.Apply this fix:
-// Generate stable ID for reasoning item -itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) -if accumulator.MessageID == nil { - itemID = fmt.Sprintf("reasoning_%d", outputIndex) -} +// Generate stable ID for reasoning item +var itemID string +if accumulator.MessageID == nil { + itemID = fmt.Sprintf("reasoning_%d", outputIndex) +} else { + itemID = fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) +}core/providers/bedrock/responses.go (1)
824-857: 🔴 Critical: Fix pointer-to-range-variable bug in tool argument finalization.Taking
&argson line 849 whereargsis the range variable will cause allfunction_call_arguments.doneresponses to point to the same memory location. After the loop completes, they'll all reference the last value, corrupting earlier events.Apply this fix:
// Close any open tool call items and emit function_call_arguments.done for outputIndex, args := range accumulator.ToolArgumentBuffers { if args != "" { itemID := accumulator.ItemIDs[outputIndex] callID := accumulator.ToolCallIDs[outputIndex] toolName := accumulator.ToolCallNames[outputIndex] // Create item with tool message info for the done event var doneItem *schemas.ResponsesMessage if callID != "" || toolName != "" { doneItem = &schemas.ResponsesMessage{ ResponsesToolMessage: &schemas.ResponsesToolMessage{}, } if callID != "" { doneItem.ResponsesToolMessage.CallID = &callID } if toolName != "" { doneItem.ResponsesToolMessage.Name = &toolName } } // Emit function_call_arguments.done with full arguments + argsCopy := args response := &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDone, SequenceNumber: sequenceNumber + len(responses), OutputIndex: schemas.Ptr(outputIndex), - Arguments: &args, + Arguments: &argsCopy, }
🧹 Nitpick comments (10)
tests/core-providers/openrouter_test.go (1)
37-39: Consider clarifying the comment on line 39.The changes appropriately disable
ToolCallsStreamingfor the beta Responses API. However, the comment on line 39 may cause confusion sinceEnd2EndToolCallingis being enabled (set totrue) while the comment references a beta limitation.If
End2EndToolCallinguses a stable non-streaming path or doesn't rely on the beta Responses API, consider updating the comment to clarify this distinction, for example:End2EndToolCalling: true, // Non-streaming path is stableOtherwise, if both fields are affected by the same beta limitation, briefly explain why one is enabled and the other disabled.
tests/core-providers/scenarios/tool_calls_streaming.go (2)
168-178: Consider more robust complete-JSON detection.The heuristic for detecting complete JSON (checking if it starts with
{and ends with}) is somewhat fragile. It won't handle:
- JSON with leading/trailing whitespace
- JSON arrays (
[...])- Multi-line JSON with different formatting
However, the current safeguard of only replacing when
existing.Arguments != ""prevents data loss on the first chunk. A more robust approach might check if the string is valid, complete JSON usingjson.Valid()and comparing length/structure.That said, the current implementation is reasonable given the complexity of handling different provider streaming formats, and the comment acknowledges the edge case.
Example alternative:
- argsStr := *arguments - if len(argsStr) > 0 && argsStr[0] == '{' && argsStr[len(argsStr)-1] == '}' && existing.Arguments != "" { - // This looks like complete arguments, but only replace if we already have partial args - // Otherwise, this might be the first chunk which happens to be complete - existing.Arguments = argsStr + argsStr := strings.TrimSpace(*arguments) + if json.Valid([]byte(argsStr)) && existing.Arguments != "" { + // This is valid complete JSON, and we already have partial args - likely a "done" event + existing.Arguments = argsStr } else { // Incremental chunk, append - existing.Arguments += argsStr + existing.Arguments += *arguments }
772-784: Consider stricter JSON validation for final tool calls.The validation attempts to parse arguments as JSON (line 774) but only logs a warning if parsing fails, then falls back to checking for non-empty content. While this flexibility is useful for handling partial JSON during streaming, by the time we're in
validateStreamingToolCalls, we're validating the final accumulated tool calls, not individual chunks.At this stage, arguments should be complete and valid JSON. Consider making JSON validation mandatory for the final result:
// Try to parse arguments as JSON to ensure they're valid var args map[string]interface{} if err := json.Unmarshal([]byte(toolCall.Arguments), &args); err != nil { - t.Logf("⚠️ %s: Tool call %d arguments are not valid JSON: %v", apiName, i, err) - // Don't fail on this - some providers might send partial JSON during streaming - // But we should at least have some content - if strings.TrimSpace(toolCall.Arguments) == "" { - t.Errorf("❌ %s: Tool call %d has empty arguments", apiName, i) - } + t.Errorf("❌ %s: Tool call %d has invalid JSON arguments: %v. Arguments: %s", apiName, i, err, toolCall.Arguments) } else { t.Logf("✅ %s: Tool call %d has valid JSON arguments: %s", apiName, i, toolCall.Arguments) }If some providers legitimately produce non-JSON arguments, this should be documented with a comment explaining why.
core/providers/gemini/gemini.go (2)
606-620: SpeechStream end-of-stream now uses unified response pipelineSwitching the final
SpeechStream“done” chunk to:
- set
BifrostContextKeyStreamEndIndicatoronctx, and- send via
ProcessAndSendResponse(GetBifrostResponseForStreamResponse(..., response, nil))correctly routes the last event through post-hooks and the shared stream envelope logic, while preserving usage and latency metadata.
If you want strictly uniform semantics, you could also set the stream-end indicator before calling
ProcessAndSendErrorin thescanner.Err()path, mirroring the success path.
863-883: TranscriptionStream final “done” chunk handling is aligned with speech streamThe updated end-of-stream handling:
- constructs a
BifrostTranscriptionStreamResponsewith full text and usage,- sets
BifrostContextKeyStreamEndIndicatoronctx, and- sends via
ProcessAndSendResponse(GetBifrostResponseForStreamResponse(..., response))is consistent with the new lifecycle model and ensures post-hooks see a single, well-formed final event.
Similar to
SpeechStream, consider setting the stream-end indicator in thescanner.Err()error path if you want downstream consumers to rely solely on the context flag to detect completion vs. transport errors.core/providers/anthropic/anthropic.go (1)
726-847: ResponsesStream accumulator and finalization logic look coherentFor
ResponsesStream:
- Missing
BodyStreamnow triggers a Bifrost operation error withBifrostContextKeyStreamEndIndicatorset (Lines 726–733).- A per-stream
AnthropicStreamAccumulatoris created once, flushed viadefer, and passed intoevent.ToBifrostResponsesStream(chunkIndex, accumulator)(Lines 743–749, 794).- Each emitted
responseis wrapped with consistentExtraFields(request type, provider, model, chunk index, per-chunk latency, optional raw response) and sent viaProcessAndSendResponse.- For the final response (
isLastChunk && i == len(responses)-1), the code ensuresresponse.Responseis non-nil, attaches accumulatedusage, overwrites latency with total duration, setsBifrostContextKeyStreamEndIndicator, and returns after sending the final event (Lines 822–832).This matches the accumulator-based lifecycle model introduced elsewhere in the PR and should give clients a clear, single “completed” Responses event.
You may also want to mirror the new pattern by setting
BifrostContextKeyStreamEndIndicatorbeforeProcessAndSendErrorin thescanner.Err()path to keep completion signalling fully uniform between success and transport-error endings.core/providers/cohere/cohere.go (2)
452-485: ChatCompletionStream error path correctly enriched; consider chunkIndex semanticsThe new block after
event.ToBifrostChatCompletionStream:
- populates
BifrostErrorExtraFieldswithRequestType,Provider, andModelRequested,- sets
BifrostContextKeyStreamEndIndicator, and- routes the error through
ProcessAndSendBifrostError,is a solid improvement for observability and consistent stream termination signalling.
Note that
chunkIndexis incremented both just before processing the event (Line 445) and again after sending a response (Line 474). This means indices advance by 2 per event and also bump for events that produce no response. If the intent is “one increment per emitted chunk”, you might consider removing one of these increments to makeChunkIndexstrictly contiguous per client-visible message.
677-723: ResponsesStream multi-response handling and finalization align with the new lifecycle modelWithin the SSE loop:
event.ToBifrostResponsesStream(chunkIndex, accumulator)now returns a slice ofresponsesplusisLastChunk.- On
bifrostErr, the code enriches the error withRequestType,Provider, andModelRequested, setsBifrostContextKeyStreamEndIndicator, and sends it viaProcessAndSendBifrostError, then breaks (Lines 681–689).- For each non-nil
responseinresponses, you set consistentExtraFields(request type, provider, model, currentchunkIndex, per-chunk latency, optional raw payload) and incrementchunkIndexper emitted item (Lines 692–703).- For the final item of the final chunk, you ensure
response.Responseis non-nil, set total latency, mark the stream end in context, send viaProcessAndSendResponse, andreturnfrom the goroutine (Lines 708–715).This is a clean implementation of accumulator-driven Responses streaming and mirrors the Anthropic changes.
As with other providers, you may optionally set
BifrostContextKeyStreamEndIndicatorin thescanner.Err()error path as well, so downstream code sees a consistent “terminal” signal regardless of whether the stream ends normally or via transport error.core/providers/openai/openai.go (1)
705-714: Chat → Responses fallback logic looks sound; consider a tiny readability tweakThe
isResponsesToChatCompletionsFallbackgate andChatToResponsesStreamAccumulatorusage cleanly separate fallback vs native chat streaming; error and completion events correctly mark the stream end via context and use the Responses stream pathway.Only nit: the inner
for _, response := range spreadResponsesin the fallback branch shadows the outerresponsevariable, which can be mildly confusing in a long function. Renaming the inner loop variable (e.g.resp) would slightly improve clarity without behavioral change.Also applies to: 873-923, 1004-1012
core/providers/cohere/responses.go (1)
577-585: Remove redundant duplicate nil check.Lines 578-582 already handle both nil and non-nil cases for
MessageID. The second check on lines 583-585 is redundant and will unconditionally overwrite the previous assignment whenMessageIDis nil, making the else branch (lines 580-582) unreachable in that case.Apply this diff:
// Generate stable ID for text item var itemID string if accumulator.MessageID == nil { itemID = fmt.Sprintf("item_%d", outputIndex) } else { itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) } -if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) -} accumulator.ItemIDs[outputIndex] = itemID
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
core/changelog.md(1 hunks)core/providers/anthropic/anthropic.go(7 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(8 hunks)core/providers/bedrock/bedrock.go(5 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(4 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(6 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)transports/changelog.md(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/perplexity/responses.go
- core/providers/utils/utils.go
✅ Files skipped from review due to trivial changes (1)
- core/changelog.md
🚧 Files skipped from review as they are similar to previous changes (16)
- core/providers/perplexity/perplexity.go
- core/providers/openrouter.go
- tests/core-providers/tests.go
- core/schemas/bifrost.go
- core/providers/groq.go
- tests/core-providers/config/account.go
- tests/core-providers/mistral_test.go
- core/providers/anthropic/chat.go
- tests/core-providers/ollama_test.go
- core/providers/parasail.go
- tests/core-providers/openai_test.go
- core/providers/cerebras.go
- tests/core-providers/gemini_test.go
- tests/core-providers/cerebras_test.go
- core/providers/sgl.go
- tests/core-providers/anthropic_test.go
🧰 Additional context used
🧬 Code graph analysis (13)
core/providers/gemini/gemini.go (2)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)core/providers/utils/utils.go (2)
ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)
core/providers/ollama.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/mistral/mistral.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/vertex/vertex.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/bedrock/responses.go (2)
core/providers/bedrock/types.go (1)
BedrockStreamEvent(363-380)core/schemas/responses.go (15)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
core/schemas/mux.go (4)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(25-40)core/schemas/responses.go (17)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1378-1378)ResponsesStreamResponseTypeRefusalDelta(1363-1363)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeCompleted(1350-1350)core/schemas/utils.go (1)
Ptr(14-16)core/schemas/bifrost.go (2)
RequestType(81-81)ResponsesStreamRequest(90-90)
core/providers/anthropic/anthropic.go (4)
core/schemas/bifrost.go (5)
BifrostContextKeyStreamEndIndicator(111-111)BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)ResponsesStreamRequest(90-90)core/schemas/provider.go (1)
Provider(207-234)core/providers/utils/utils.go (3)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)core/schemas/responses.go (1)
BifrostResponsesResponse(45-82)
core/providers/bedrock/bedrock.go (3)
core/schemas/bifrost.go (6)
BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)BifrostContextKeyStreamEndIndicator(111-111)BifrostResponseExtraFields(282-291)ResponsesStreamRequest(90-90)core/providers/utils/utils.go (4)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ShouldSendBackRawResponse(480-485)core/providers/bedrock/responses.go (2)
NewBedrockStreamAccumulator(28-38)FinalizeBedrockStream(799-890)
core/providers/anthropic/responses.go (2)
core/schemas/responses.go (21)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseType(1345-1345)ResponsesStreamResponseTypeMCPCallArgumentsDelta(1386-1386)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesMessageTypeComputerCall(282-282)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesStreamResponseTypeMCPCallArgumentsDone(1387-1387)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)core/providers/anthropic/types.go (9)
AnthropicStreamEventTypeContentBlockStart(303-303)AnthropicContentBlockTypeText(127-127)AnthropicContentBlockTypeToolUse(129-129)AnthropicContentBlockTypeMCPToolUse(133-133)AnthropicStreamEventTypeContentBlockDelta(304-304)AnthropicStreamDeltaTypeText(326-326)AnthropicStreamDeltaTypeInputJSON(327-327)AnthropicStreamEventTypeMessageDelta(306-306)AnthropicStreamEventTypeMessageStop(302-302)
core/providers/openai/openai.go (4)
core/schemas/bifrost.go (6)
BifrostContextKeyStreamEndIndicator(111-111)BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)BifrostError(351-360)ErrorField(369-376)BifrostErrorExtraFields(418-422)RequestType(81-81)core/providers/utils/utils.go (6)
ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ProcessAndSendBifrostError(569-599)ProviderSendsDoneMarker(749-758)ProcessAndSendError(605-651)CreateBifrostChatCompletionChunkResponse(684-713)core/schemas/mux.go (2)
ChatToResponsesStreamAccumulator(961-977)NewChatToResponsesStreamAccumulator(980-991)core/schemas/responses.go (2)
ResponsesStreamResponseTypeError(1409-1409)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/cohere/cohere.go (3)
core/schemas/bifrost.go (6)
BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)BifrostContextKeyStreamEndIndicator(111-111)ResponsesStreamRequest(90-90)BifrostResponseExtraFields(282-291)core/providers/utils/utils.go (4)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ShouldSendBackRawResponse(480-485)core/providers/cohere/responses.go (1)
NewCohereStreamAccumulator(26-34)
tests/core-providers/scenarios/tool_calls_streaming.go (3)
core/schemas/chatcompletions.go (3)
ChatAssistantMessageToolCall(483-488)ChatAssistantMessageToolCallFunction(491-494)ChatTool(201-205)tests/core-providers/scenarios/utils.go (6)
ToolCallInfo(293-297)CreateBasicChatMessage(218-225)GetSampleChatTool(129-148)SampleToolTypeWeather(69-69)CreateBasicResponsesMessage(227-235)GetSampleResponsesTool(150-169)core/schemas/responses.go (6)
BifrostResponsesRequest(32-39)ResponsesParameters(84-111)BifrostResponsesStreamResponse(1412-1450)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)
core/providers/cohere/responses.go (2)
core/providers/cohere/types.go (13)
CohereStreamEvent(381-386)StreamEventMessageStart(366-366)StreamEventContentStart(367-367)CohereContentBlockTypeText(128-128)CohereContentBlockTypeThinking(130-130)StreamEventContentDelta(368-368)StreamEventContentEnd(369-369)StreamEventToolPlanDelta(370-370)StreamEventToolCallStart(371-371)StreamEventToolCallDelta(372-372)StreamEventToolCallEnd(373-373)StreamEventCitationEnd(375-375)StreamEventMessageEnd(376-376)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeReasoning(297-297)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeOutputTextAnnotationAdded(1401-1401)ResponsesStreamResponseTypeOutputTextAnnotationDone(1402-1402)ResponsesStreamResponseTypeCompleted(1350-1350)
🔇 Additional comments (25)
transports/changelog.md (3)
2-2: Well-documented feature entry for the main streaming improvements.The changelog entry accurately captures the core feature of this PR: unified streaming lifecycle events aligned with OpenAI's response types across all providers. The entry is clear, concise, and appropriately emphasizes the cross-provider scope and the OpenAI alignment benefit.
3-3: Clear and specific chore entry for the OpenRouter provider update.The changelog entry appropriately documents the provider-specific API endpoint shift from
alpha/responsestov1/responses. The entry is precise about the scope (OpenRouter), the change, and the affected component.
2-3: Verify changelog comprehensiveness against the full scope of provider changes.Based on the PR summary, changes span multiple providers (Anthropic, Bedrock, Cohere, Gemini, OpenAI, Groq, Parasail, SGL, Vertex, etc.) with refactoring of streaming accumulators and native implementations. Confirm that the current changelog entries adequately reflect all user-facing and integration-impacting changes, or consider adding separate entries if other providers have breaking or notable API-surface changes similar to OpenRouter's endpoint shift.
tests/core-providers/cohere_test.go (1)
36-36: LGTM! Streaming tool calls enabled for Cohere.Enabling
ToolCallsStreamingis consistent with the PR's goal to add streaming support across providers. The change aligns well with the existingToolCallstest scenario and the test command listed in the PR objectives confirms this functionality has been verified.core/providers/ollama.go (1)
172-180: LGTM! Clean fallback implementation.The changes correctly implement the Responses-to-Chat fallback mechanism:
- The context flag propagation (line 173) enables downstream handlers to detect and handle the fallback scenario appropriately.
- Passing
postHookRunnerdirectly (line 176) aligns with the PR's removal of the combined post-hook converter wrapper, simplifying the streaming pipeline.This implementation is consistent with similar changes across other providers and supports the broader refactoring to improve streaming consistency.
tests/core-providers/groq_test.go (1)
43-43: Groq provider supports tool call streaming through shared OpenAI-compatible handler—verification complete.The OpenAI streaming handler (which Groq delegates to) explicitly processes tool calls in delta events at line 976 of
core/providers/openai/openai.go. Tool calls are detected vialen(choice.ChatStreamResponseChoice.Delta.ToolCalls) > 0and streamed directly to the response channel, soToolCallsStreaming: trueis correctly configured for Groq.tests/core-providers/scenarios/tool_calls_streaming.go (4)
18-42: LGTM! Well-structured accumulator design.The accumulator struct and constructor are well-designed with clear separation between Chat and Responses APIs. The ItemIDToKey mapping is a good addition for efficient lookups during streaming.
188-216: LGTM! Clean getter implementations.The getter methods properly handle nil pointers and provide a consistent interface for both Chat and Responses APIs.
218-385: LGTM! Robust test orchestration with good retry logic.The test implementation has several good practices:
- 3-attempt retry logic handles flaky provider responses
- Safety limit (500 responses) prevents infinite loops
- Logs only on first attempt to reduce noise
- Validates before calling helper to catch issues early
- Early return on success avoids unnecessary retries
The verbose logging with emojis and detailed field tracking is appropriate for this complex streaming behavior test.
387-743: LGTM! Comprehensive Responses API streaming test.The Responses API test properly handles multiple streaming event types and extracts tool call information from various field locations. The detailed field-location logging (e.g., "streamResp.Item.ResponsesToolMessage.CallID") is particularly helpful for debugging provider-specific streaming formats.
core/providers/mistral/mistral.go (1)
201-209: ResponsesStream fallback flag wiring looks correctSetting
BifrostContextKeyIsResponsesToChatCompletionFallbackonctxand passingpostHookRunnerdirectly intoChatCompletionStreamis consistent with the new shared streaming path and doesn’t introduce correctness issues.core/providers/vertex/vertex.go (1)
664-673: Vertex ResponsesStream correctly marks chat-fallback pathUsing
context.WithValuewithBifrostContextKeyIsResponsesToChatCompletionFallbackand forwardingpostHookRunnertoChatCompletionStreamaligns Vertex with other providers’ fallback streaming behavior and keeps authorization and error handling centralized.core/providers/gemini/gemini.go (1)
345-353: Gemini ResponsesStream correctly marks chat-fallback and uses native post-hooksMarking the Responses→Chat fallback via
BifrostContextKeyIsResponsesToChatCompletionFallbackand passing the originalpostHookRunnerintoChatCompletionStreammatches the new global streaming design and keeps Gemini consistent with other providers.core/providers/anthropic/anthropic.go (1)
469-478: Empty BodyStream handling now correctly marks stream endOn the
resp.BodyStream() == nilpath inHandleAnthropicChatCompletionStreaming, the code now setsBifrostContextKeyStreamEndIndicatoronctxbefore callingProcessAndSendBifrostError. This is a good improvement: downstream consumers can reliably treat this as a terminal condition.core/providers/cohere/cohere.go (1)
633-640: Cohere ResponsesStream accumulator initialization is appropriateCreating a
NewCohereStreamAccumulator()once per stream, storingrequest.Modelintoaccumulator.Modelwhen present, and flushing it viadefer accumulator.Flush()provides the necessary per-session state for OpenAI-style Responses lifecycle events without leaking state across requests.core/providers/openai/openai.go (2)
542-555: TextCompletionStream end-of-stream handling is consistentSetting
BifrostContextKeyStreamEndIndicatorbefore the final aggregated chunk and routing it throughProcessAndSendResponsealigns this path with the unified streaming lifecycle and should be safe; no functional issues spotted here.
1336-1344: Unified stream-end signalling across Responses/Speech/Transcription streamingThe added
BifrostContextKeyStreamEndIndicatorassignments before emittingresponse.completedor finalusagechunks for Responses, SpeechStream, and TranscriptionStream are consistent with the rest of the provider-utils pipeline and should help downstream consumers detect terminal events reliably.Also applies to: 1708-1713, 1978-1985
core/providers/anthropic/responses.go (5)
14-60: Accumulator and lifecycle state are well-structuredThe extended
AnthropicStreamAccumulatorplusFlush/GetOrCreateOutputIndexgive a clear, per-output-index state model (IDs, argument buffers, MCP flags, created/in_progress flags), and the reset semantics look correct for multi-response streams. No functional issues here.Also applies to: 62-80
395-441: MessageStart → created/in_progress events match Responses lifecycleThe
MessageStartbranch now emittingresponse.createdandresponse.in_progressonce per stream (guarded byHasEmittedCreated/HasEmittedInProgress) with a stableCreatedAtfrom the accumulator is aligned with OpenAI-style Responses lifecycle and looks correct.
444-571: Output-item creation for text, function, MCP, and computer calls is coherentThe
ContentBlockStarthandling now:
- Maps
content_index→output_indexviaGetOrCreateOutputIndex.- Emits
response.output_item.addedfor text, function_call, MCP_call, and computer_call with stableOutputIndex.- Generates stable
item_ids (with a safe nil check onMessageID), and initializes buffers / MCP flags only where needed.This design cleanly supports multi-output responses and tool calls; I don’t see correctness issues in this block.
657-762: *ContentBlockStop completion and _arguments.done emission are well-orderedFor
ContentBlockStopyou:
- Handle computer tools specially (parsing accumulated JSON, emitting a completed computer_call item, and clearing the dedicated accumulator fields).
- For other tools/text, first emit
function_call_arguments.done/mcp_call_arguments.donewhen an argument buffer exists, then always emitresponse.output_item.donewith status"completed"and any knownitem_id.This sequence matches the expected Responses lifecycle ordering and correctly clears argument buffers/MCP flags to avoid leaking state into subsequent outputs.
770-783: MessageStop → response.completed behaves as expectedThe
MessageStopbranch synthesizing aresponse.completedwithCreatedAtfrom the accumulator and optionalIDis consistent with the earlier created/in_progress events and correctly signals terminal completion (withisStreamCompleted= true to the caller).tests/core-providers/sgl_test.go (1)
24-46: Enabling ToolCallsStreaming for SGL tests is consistent with the new streaming coverageAdding
ToolCallsStreaming: truealongsideToolCalls: truealigns SGL with other providers’ comprehensive streaming tests and should help exercise the new tool-call streaming paths.tests/core-providers/parasail_test.go (1)
24-45: Parasail test config: ToolCallsStreaming flag looks goodTurning on
ToolCallsStreamingin the Parasail scenarios matches the rest of the suite and should expand coverage for streamed tool-call behavior without affecting non-streaming tests.tests/core-providers/bedrock_test.go (1)
24-51: Bedrock tests correctly enable ToolCallsStreamingSetting
ToolCallsStreaming: truehere is in line with the new Bedrock streaming accumulator and ensures the comprehensive test harness will hit the streamed tool-call paths.
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/anthropic/anthropic.go (1)
469-477: Avoid emitting a final “success” chunk after a streaming error in Anthropic chatWhen
event.ToBifrostChatCompletionStream()returns abifrostErr, you set the stream-end indicator and send aBifrostError, but onlybreakthe scan loop. The code then falls through to the “no scanner error” branch and emits a final aggregated completion chunk, so the client sees both an error and a successful end-of-stream event for the same stream.Change the error path to return from the goroutine instead of just breaking, so no success chunk is sent after an error.
- if bifrostErr != nil { - bifrostErr.ExtraFields = schemas.BifrostErrorExtraFields{ - RequestType: schemas.ChatCompletionStreamRequest, - Provider: providerType, - ModelRequested: modelName, - } - ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) - providerUtils.ProcessAndSendBifrostError(ctx, postHookRunner, bifrostErr, responseChan, logger) - break - } + if bifrostErr != nil { + bifrostErr.ExtraFields = schemas.BifrostErrorExtraFields{ + RequestType: schemas.ChatCompletionStreamRequest, + Provider: providerType, + ModelRequested: modelName, + } + ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) + providerUtils.ProcessAndSendBifrostError(ctx, postHookRunner, bifrostErr, responseChan, logger) + return + }Also applies to: 548-558, 587-595
♻️ Duplicate comments (3)
core/providers/cohere/responses.go (1)
610-616: Fix potential nil-pointer dereference when generating reasoning item IDsHere,
accumulator.MessageIDis dereferenced before checking fornil:// Generate stable ID for reasoning item itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) if accumulator.MessageID == nil { itemID = fmt.Sprintf("reasoning_%d", outputIndex) }If Cohere ever emits a reasoning block before a
message_startwith an ID (or omits the ID), this will panic.Reorder the logic to guard the dereference:
-// Generate stable ID for reasoning item -itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) -if accumulator.MessageID == nil { - itemID = fmt.Sprintf("reasoning_%d", outputIndex) -} +// Generate stable ID for reasoning item +var itemID string +if accumulator.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) +} else { + itemID = fmt.Sprintf("reasoning_%d", outputIndex) +}This matches the safe pattern you already use elsewhere for text and tool-plan IDs.
tests/core-providers/scenarios/tool_calls_streaming.go (1)
45-71: Don’t treatIndex == 0as “unset” when keying Chat tool calls
AccumulateChatToolCallstill treatstoolCall.Index == 0as a signal to fall back tochoiceIndex:key = int(toolCall.Index) if key == 0 { key = choiceIndex } ... key = int(toolCall.Index) if key == 0 { key = choiceIndex }But
Indexis a legitimate 0-based position (the first tool call is index 0 in the core accumulator). UsingchoiceIndexhere can mis-bucket tool calls, especially when multiple calls are present.Since you already have a separate
foundflag for ID matches, you can safely useint(toolCall.Index)directly:- key = int(toolCall.Index) - if key == 0 { - key = choiceIndex - } + key = int(toolCall.Index) ... - key = int(toolCall.Index) - if key == 0 { - key = choiceIndex - } + key = int(toolCall.Index)This preserves the true tool-call index and avoids collisions.
core/providers/bedrock/responses.go (1)
11-53: Fix pointer-to-range-variable usage and always finalize tool-call itemsTwo issues in the Bedrock streaming accumulator/finalizer:
Pointer-to-range-variable bug (critical)
InFinalizeBedrockStream, you take addresses of range variables:
Arguments: &argsOutputIndex: &outputIndexinside:
for outputIndex, args := range accumulator.ToolArgumentBuffers { if args != "" { // ... response := &schemas.BifrostResponsesStreamResponse{ // ... OutputIndex: schemas.Ptr(outputIndex), // or &outputIndex currently Arguments: &args, } // ... } }Because
argsandoutputIndexare reused on each iteration, all emitted responses end up pointing at the same underlying variables, so later iterations overwrite earlier events’ArgumentsandOutputIndex. This is the same class of issue that was flagged earlier in this file.Fix by copying the values (or by using
schemas.Ptrconsistently) before taking addresses:for outputIndex, args := range accumulator.ToolArgumentBuffers { if args != "" {
itemID := accumulator.ItemIDs[outputIndex]callID := accumulator.ToolCallIDs[outputIndex]toolName := accumulator.ToolCallNames[outputIndex]
itemID := accumulator.ItemIDs[outputIndex]callID := accumulator.ToolCallIDs[outputIndex]toolName := accumulator.ToolCallNames[outputIndex]// Copy loop variables to avoid pointer-to-range-variable bugsoutputIndexCopy := outputIndexargsCopy := args // ...
response := &schemas.BifrostResponsesStreamResponse{
response := &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDone,
SequenceNumber: sequenceNumber + len(responses),OutputIndex: schemas.Ptr(outputIndex),Arguments: &args,
SequenceNumber: sequenceNumber + len(responses),OutputIndex: schemas.Ptr(outputIndexCopy),Arguments: &argsCopy, } // ...
responses = append(responses, &schemas.BifrostResponsesStreamResponse{
responses = append(responses, &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeOutputItemDone,
SequenceNumber: sequenceNumber + len(responses),OutputIndex: schemas.Ptr(outputIndex),
SequenceNumber: sequenceNumber + len(responses),OutputIndex: schemas.Ptr(outputIndexCopy), Item: outputItemDone, }) }
Tool calls with empty arguments never closed (behavioral edge)
Theif args != ""guard means that tool calls for which no argument deltas were streamed (e.g., functions without arguments) will never get:
function_call_arguments.done, noroutput_item.donefor that tool call.To keep Bedrock aligned with OpenAI-style Responses semantics, you likely want these events even when the arguments string is empty.
Consider removing the
args != ""check and always emitting bothfunction_call_arguments.doneandoutput_item.donefor any entry present inToolArgumentBuffers(using an empty string forArgumentswhen appropriate).Also applies to: 585-596, 798-890
🧹 Nitpick comments (6)
core/providers/cohere/cohere.go (2)
452-484: Guard against double-increment ofchunkIndexin ChatCompletionStreamWithin the chat stream loop,
chunkIndexis incremented once before callingToBifrostChatCompletionStreamand again after enqueuing a non-nil response. This makesExtraFields.ChunkIndexskip values (1, 3, 5, …) which is probably unintended and can confuse consumers relying on contiguous indices:chunkIndex++ ... response.ExtraFields.ChunkIndex = chunkIndex ... lastChunkTime = time.Now() chunkIndex++ // second incrementConsider incrementing
chunkIndexonly once per emitted response (e.g., drop the initial increment or the post-send increment) so chunk indices remain contiguous and easier to reason about.
491-494: Consider marking scanner I/O errors as stream-endingOn scanner errors you call
ProcessAndSendErrorbut don’t setschemas.BifrostContextKeyStreamEndIndicatoron the context, unlike thebifrostErrandisLastChunkpaths. If any downstream hooks rely on that flag to finalize state, consider setting it here as well for consistency:ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) providerUtils.ProcessAndSendError(...)core/providers/cohere/responses.go (2)
576-587: Remove redundantMessageID == nilcheck in text item ID generationIn the text
StreamEventContentStartbranch,itemIDis already set for bothniland non-nilMessageID, then immediately re-checked fornil:var itemID string if accumulator.MessageID == nil { itemID = fmt.Sprintf("item_%d", outputIndex) } else { itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) } if accumulator.MessageID == nil { itemID = fmt.Sprintf("item_%d", outputIndex) }The second
if accumulator.MessageID == nilis dead code and can be removed to simplify the branch.
498-542: Lifecycle emission gated onchunk.IDmay skip events if IDs are absent
response.created/response.in_progressare only emitted whenchunk.ID != nil. If Cohere ever omitsIDonmessage_start, the lifecycle events will be skipped for that stream:case StreamEventMessageStart: if chunk.ID != nil { accumulator.MessageID = chunk.ID ... // emit Created / InProgress }If you want lifecycle events to be guaranteed, consider emitting them regardless of ID presence (using
CreatedAt+ a nilID), and only populatingMessageIDwhen available. If you’re confident Cohere always provides IDs, leaving this as-is is acceptable but does rely on that invariant.tests/core-providers/scenarios/tool_calls_streaming.go (1)
441-691: Consider per-attempt cancellable contexts to avoid goroutine leaks on early exitIn both streaming tests, when
streamErroris set youbreakthefor rangeloop but don’t cancel the underlying request context. The provider goroutine will keep writing to the channel until completion, which can block if the channel buffer fills after the test stops reading.Consider deriving a per-attempt context with
context.WithCanceland callingcancel()when you decide to abort an attempt, so provider-side goroutines can terminate promptly.core/providers/openai/openai.go (1)
542-554: Consistent stream-end signaling across OpenAI streaming variantsAdding
BifrostContextKeyStreamEndIndicatorbefore the finalProcessAndSendResponsein:
HandleOpenAITextCompletionStreamingfinal chunk,- ChatCompletionStreaming’s final aggregate chunk (non-fallback),
- ResponsesStreaming’s
Type == Completedchunk,- SpeechStream and TranscriptionStream chunks carrying
Usage,gives a uniform way for downstream consumers to detect end-of-stream across all OpenAI streaming APIs. The changes are minimal and do not alter chunk ordering.
For readability, consider renaming the inner
responsevariable in the fallback loop (theBifrostResponsesStreamResponse) to avoid shadowing the outerBifrostChatResponse, e.g.respChunk.Also applies to: 1000-1012, 1340-1343, 1708-1712, 1980-1983
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
core/changelog.md(1 hunks)core/providers/anthropic/anthropic.go(7 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(8 hunks)core/providers/bedrock/bedrock.go(5 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(4 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(6 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)transports/changelog.md(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/perplexity/responses.go
- core/providers/utils/utils.go
🔇 Additional comments (52)
tests/core-providers/sgl_test.go (1)
36-36: Enable tool call streaming tests for SGL provider.The
ToolCallsStreamingfield is properly defined in theTestScenariosstruct, and a complete test implementation exists inRunToolCallsStreamingTestthat checks if the flag is enabled before executing the test scenarios. The test is integrated into the framework and tracked with other test scenarios. This change correctly enables the existing tool call streaming test scenario for the SGL provider, following the same pattern used by other providers.tests/core-providers/mistral_test.go (1)
38-38: Review comment is incorrect and should be disregarded.The code change is valid. Mistral provider uses the shared
openai.HandleOpenAIChatCompletionStreaminghandler (lines 170-183 of mistral.go), which is employed by 13+ providers including Anthropic, Bedrock, Cohere, Groq, and others—all of which enableToolCallsStreaming: truein their test configurations. The Mistral test file runsrunAllComprehensiveTests, which includes the tool calls streaming test scenario. Additionally, Mistral API officially supports streaming chat completions together with tool calling through server-sent events, so the test will execute successfully.Likely an incorrect or invalid review comment.
tests/core-providers/tests.go (1)
35-35: LGTM!The additions of
RunToolCallsStreamingTestto the test scenarios list and its corresponding entry in the test summary display are consistent with the PR's objective to add comprehensive streaming tool call tests across providers.Also applies to: 77-77
core/providers/openrouter.go (1)
214-214: LGTM!The endpoint migration from
/alpha/responsesto/v1/responsesaligns OpenRouter with the v1 API surface. This is a straightforward version upgrade with no behavioral changes.Also applies to: 233-233
tests/core-providers/groq_test.go (1)
43-43: LGTM!Enabling
ToolCallsStreaming: truefor Groq aligns with the broader PR pattern of enabling streaming tool call tests across multiple providers.tests/core-providers/config/account.go (1)
29-29: LGTM!The addition of the
ToolCallsStreamingfield to theTestScenariosstruct is well-documented and necessary for enabling streaming tool call tests across providers.core/providers/cerebras.go (1)
173-179: LGTM!The changes to
ResponsesStreamalign with the unified streaming lifecycle pattern:
- Setting
BifrostContextKeyIsResponsesToChatCompletionFallbacksignals the fallback streaming path- Passing
postHookRunnerdirectly (instead of wrapped) is consistent with the refactored approach across other providerstests/core-providers/openrouter_test.go (1)
37-39: LGTM!The test configuration appropriately disables
ToolCallsStreamingfor OpenRouter due to the Beta status of their responses API, while still enablingEnd2EndToolCalling. The inline comment clearly documents the reasoning.core/schemas/bifrost.go (1)
117-117: LGTM!The addition of
BifrostContextKeyIsResponsesToChatCompletionFallbackfollows the established pattern for context keys and is well-documented. This key enables unified fallback streaming behavior across providers, as evidenced by its usage in multiple provider implementations.core/providers/perplexity/perplexity.go (1)
212-218: LGTM!The
ResponsesStreamimplementation follows the same unified streaming pattern applied across other providers:
- Setting
BifrostContextKeyIsResponsesToChatCompletionFallbackenables fallback path detection- Direct
postHookRunnerusage aligns with the refactored streaming architecturecore/providers/sgl.go (1)
168-176: ResponsesStream fallback flag wiring looks correctPropagating
schemas.BifrostContextKeyIsResponsesToChatCompletionFallbackon the context and reusingpostHookRunnerintoChatCompletionStreamis a clean way to mark the fallback path without altering stream semantics. No issues spotted here.core/changelog.md (1)
1-2: Changelog entries accurately describe the streaming changesThe new bullets cleanly summarize the unified lifecycle events and OpenRouter
/v1/responsesmigration and are consistent with the rest of the changelog.core/providers/cohere/responses.go (3)
11-47: Accumulator design for Cohere responses streaming is solid
CohereStreamAccumulatorcleanly centralizes per-stream state (output indices, argument buffers, stable item IDs, lifecycle flags) andFlushcorrectly resets all fields, includingCreatedAt, for reuse. This aligns with the OpenAI-style multi-item streaming model.
681-742: Tool-plan handling and text item reuse look correctThe
StreamEventToolPlanDeltabranch sensibly treats tool plans as a regular assistant text item, reusing a dedicatedoutput_index(default 0), generating a stable item ID, emittingoutput_item.addedonce, and then streaming deltas viaoutput_text.delta. Closing any open tool-plan item inContentStart/ToolCallStartusingToolPlanOutputIndexis a good way to ensure well-formed item lifecycles.
745-888: Tool call start/delta/end sequencing for Responses streaming is well structuredThe
StreamEventToolCallStart,StreamEventToolCallDelta, andStreamEventToolCallEndcases correctly:
- Allocate a dedicated
output_indexper tool call to avoid collisions with text items.- Track stable
itemIDs peroutput_index.- Accumulate
Function.ArgumentsinToolArgumentBuffersacross deltas.- Emit
function_call_arguments.deltaper chunk and a singlefunction_call_arguments.donewith the full JSON payload, followed byoutput_item.done.This matches the OpenAI Responses API tool-call lifecycle and should work well with the new tests.
tests/core-providers/scenarios/tool_calls_streaming.go (4)
101-186: Responses tool-call accumulator is robust and flexible
AccumulateResponsesToolCalluses sensible heuristics (preferringitemID, thencallID, then name) plus theItemIDToKeymap to stitch together deltas, added events, and done events across providers. The “complete JSON vs incremental chunk” handling forArgumentsalso matches how different providers stream payloads. Looks good.
218-385: Streaming test harness for Chat tool calls is comprehensiveThe Chat streaming test:
- Retries up to 3 times with clear logging.
- Validates non-nil channel and responses.
- Accumulates tool-call fragments across chunks and asserts that final calls have ID, name, and arguments before deeper validation.
The overall structure is solid and should catch most regressions in tool-call streaming.
387-742: Responses streaming test harness mirrors Chat coverage effectivelyThe Responses streaming test mirrors the Chat harness, inspecting
FunctionCallArgumentsDelta,OutputItemAdded, andFunctionCallArgumentsDoneevents, accumulating bycallID/itemID, and validating that final tool calls contain ID, name, and arguments. This provides good cross-provider coverage for the new Responses streaming lifecycle.
745-793: Streaming validation helper is clear and appropriately strict
validateStreamingToolCallsgives good diagnostics (including JSON parse attempts) while still enforcing that all tool calls have non-empty ID, name, and arguments viarequire.NotEmpty. This strikes a good balance between robustness and strictness for the tool-call streaming tests.tests/core-providers/anthropic_test.go (1)
38-38: LGTM! Streaming tool calls enabled for Anthropic tests.This change enables streaming tool call tests for Anthropic, aligning with the PR's objective to improve Responses API streaming across providers.
tests/core-providers/openai_test.go (1)
46-46: LGTM! Streaming tool calls enabled for OpenAI tests.Consistent with the streaming enablement across other providers.
tests/core-providers/cohere_test.go (1)
36-36: LGTM! Streaming tool calls enabled for Cohere tests.tests/core-providers/ollama_test.go (2)
26-26: Verify that the model change from llama3.2 to llama3.1:latest is intentional.The model version appears to be downgraded. Please confirm that
llama3.1:latestprovides the necessary tool calling and streaming capabilities that this PR requires, or if this change addresses a specific compatibility issue.
35-35: LGTM! Streaming tool calls enabled for Ollama tests.tests/core-providers/parasail_test.go (1)
35-35: LGTM! Streaming tool calls enabled for Parasail tests.core/providers/mistral/mistral.go (1)
202-210: LGTM! Refactored to use context flag for fallback streaming.This change improves the streaming implementation by:
- Setting
BifrostContextKeyIsResponsesToChatCompletionFallbackin context to signal the fallback path- Passing
postHookRunnerdirectly instead of wrapping it, allowing unified handling downstreamThis aligns with the PR's objective to improve streaming consistency across providers.
core/providers/ollama.go (1)
172-180: LGTM! Consistent refactoring with other providers.This change mirrors the Mistral provider implementation, using the context flag approach for fallback streaming. The consistency across providers strengthens the implementation.
core/providers/anthropic/chat.go (1)
704-732: Field reference corrected for tool call ID.The change from
chunk.ContentBlock.ToolUseIDtochunk.ContentBlock.IDat line 718 appears to be a bug fix. When handling theContentBlockStartevent for tool_use blocks, the tool call ID should be sourced from theIDfield of the content block.Please verify that
ContentBlock.IDis the correct field according to Anthropic's streaming API documentation for tool_use content blocks.core/providers/groq.go (1)
212-220: LGTM: Fallback streaming path correctly implemented.The changes enable a fallback-aware streaming flow for Responses API by setting a context flag and passing the postHookRunner directly to ChatCompletionStream. This aligns with the broader pattern across providers.
tests/core-providers/azure_test.go (1)
36-54: LGTM: Tool call streaming enabled for Azure tests.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for the Azure provider, aligning with the broader test coverage improvements across providers.transports/changelog.md (1)
1-3: LGTM: Changelog entries accurately document the PR changes.The changelog appropriately documents the unified streaming lifecycle events feature and the OpenRouter API path update.
tests/core-providers/bedrock_test.go (1)
34-52: LGTM: Tool call streaming enabled for Bedrock tests.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for the Bedrock provider, consistent with the test coverage improvements across other providers.core/providers/vertex/vertex.go (1)
665-673: LGTM: Fallback streaming path correctly implemented.The changes enable a fallback-aware streaming flow for Responses API by setting a context flag and passing the postHookRunner directly to ChatCompletionStream. This matches the pattern applied consistently across other providers.
tests/core-providers/cerebras_test.go (1)
33-51: LGTM: Tool call streaming enabled for Cerebras tests.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for the Cerebras provider, maintaining consistency with the test suite improvements.tests/core-providers/gemini_test.go (1)
35-57: LGTM: Tool call streaming enabled for Gemini tests.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for the Gemini provider, consistent with the broader test coverage enhancements.core/providers/parasail.go (1)
144-152: LGTM: Fallback streaming path correctly implemented.The changes enable a fallback-aware streaming flow for Responses API by setting a context flag and passing the postHookRunner directly to ChatCompletionStream. This follows the consistent pattern applied across all providers in this PR.
core/providers/gemini/gemini.go (2)
345-353: Responses→ChatCompletion fallback flag wiring looks correctSetting
BifrostContextKeyIsResponsesToChatCompletionFallbackbefore delegating toChatCompletionStreamcleanly opts Gemini into the shared OpenAI-style Responses streaming path without extra converters. No issues from a correctness standpoint.
601-620: Consistent use of stream-end indicator for Gemini speech/transcriptionMarking
BifrostContextKeyStreamEndIndicatorand usingProcessAndSendResponsefor the finalDonechunks aligns Gemini’s speech/transcription streaming lifecycle with the rest of the providers. The change is safe and improves consistency.Also applies to: 881-883
tests/core-providers/vertex_test.go (1)
24-46: Enabling Vertex tool-call streaming tests is appropriateTurning on
ToolCallsStreaming: truehere correctly aligns Vertex tests with the new streaming tooling across providers. Assuming provider support exists, this is a good coverage expansion.Please run the Vertex suite once after this stack lands (when credentials are available) to confirm streaming tool-calls behave as expected.
core/providers/anthropic/anthropic.go (1)
469-477: ResponsesStream accumulator + lifecycle events look correctThe ResponsesStream path for Anthropic now:
- Signals empty-body errors with
BifrostContextKeyStreamEndIndicatorset.- Uses
ToBifrostResponsesStream(chunkIndex, accumulator)to emit multiple lifecycle events per SSE chunk.- Enriches streaming conversion errors with
RequestType/Provider/ModelRequestedand terminates the stream on such errors.- Only attaches usage and stream-end for the last response slice element when
isLastChunkis true.This matches the intended OpenAI-style Responses streaming model. No further issues spotted here.
Also applies to: 726-735, 783-785, 794-804, 806-837
core/providers/openai/openai.go (1)
705-714: Responses→ChatCompletion fallback path in OpenAI streaming looks soundThe new
isResponsesToChatCompletionsFallbackbranch:
- Detects the fallback via
BifrostContextKeyIsResponsesToChatCompletionFallbackand initializes aChatToResponsesStreamAccumulator.- Converts each chat stream chunk into one or more
BifrostResponsesStreamResponseitems, with per-chunkRequestType/Provider/ModelRequested/ChunkIndexset fromSequenceNumber.- Properly short-circuits on error-type responses and on
ResponsesStreamResponseTypeCompleted, marking the stream end viaBifrostContextKeyStreamEndIndicatorand returning from the goroutine.The non-fallback branch retains the prior chat streaming behavior (usage aggregation, finishReason handling, final aggregate chunk) with only the end-indicator added. This all aligns well with the new Responses lifecycle.
Also applies to: 816-823, 851-865, 873-923, 1000-1012
core/providers/anthropic/responses.go (5)
13-32: LGTM: Well-designed accumulator for stateful streaming conversion.The
AnthropicStreamAccumulatorstruct provides comprehensive state tracking for OpenAI-style lifecycle events, content-to-output index mapping, and per-item buffering. The design appropriately uses maps for dynamic tracking and flags for lifecycle management.
34-44: LGTM: Constructor properly initializes all fields.All map fields are correctly initialized using
make(), and the timestamp is set appropriately. The initialization ensures no nil pointer dereferences will occur during streaming.
46-60: LGTM: Comprehensive state reset in Flush.The
Flushmethod correctly resets all accumulator fields, including recreating maps to clear previous entries and resetting lifecycle flags. This ensures clean state between streams.
62-80: LGTM: Correct output index mapping logic.The
GetOrCreateOutputIndexmethod properly handles both nil and non-nil content indices, maintaining stable mappings across streaming chunks. The logic ensures consistent output indices for the same content indices throughout the stream.
394-809: LGTM: Comprehensive streaming conversion with proper lifecycle management.The
ToBifrostResponsesStreammethod correctly:
- Emits OpenAI-style lifecycle events (created, in_progress, completed)
- Uses the accumulator to maintain stable output indices and item IDs
- Handles all content types (text, function calls, MCP calls, computer tools)
- Properly accumulates and emits tool arguments
- Returns multiple responses per chunk when appropriate
- Safely checks for nil before dereferencing (e.g., lines 485-489)
The past review concern about nil dereference has been properly addressed.
core/schemas/mux.go (3)
960-977: LGTM: Well-structured accumulator for Chat-to-Responses conversion.The
ChatToResponsesStreamAccumulatorstruct provides comprehensive state tracking for converting Chat API streaming to Responses API format. The design appropriately includes:
- Tool call tracking with multiple map indices for flexible lookups
- Text item lifecycle flags
- Monotonic sequence numbering
- Lifecycle event flags
979-991: LGTM: Constructor properly initializes all accumulator fields.All map fields are correctly initialized, and counters/timestamps are set appropriately. This ensures safe access during streaming conversion.
993-1350: LGTM: Robust Chat-to-Responses streaming conversion with proper state management.The
ToBifrostResponsesStreamResponsemethod correctly:
- Emits OpenAI-style lifecycle events at appropriate times
- Manages text and tool call items with proper state tracking
- Creates stable item IDs (safely checking for nil at lines 1063-1067)
- Accumulates tool arguments and emits done events on completion
- Uses
argsCopyto avoid range variable address issues (line 1281)- Increments sequence numbers monotonically
- Sets ExtraFields consistently across all responses
Both past review concerns have been properly addressed.
core/providers/bedrock/bedrock.go (3)
725-734: LGTM: Improved error metadata and stream end signaling.The error handling now enriches
BifrostErrorwith request type, provider, and model information, improving observability. Setting the stream end indicator in the context before processing errors and final responses ensures proper stream lifecycle management.Also applies to: 762-763
871-874: LGTM: Proper accumulator initialization and lifecycle management.The
BedrockStreamAccumulatoris correctly initialized before the streaming loop, the model is set for context, andFlushis deferred to ensure cleanup. The accumulator is properly passed toToBifrostResponsesStreamfor stateful conversion.Also applies to: 947-947
886-908: LGTM: Proper stream finalization and multi-response handling.The EOF handling correctly:
- Sets the stream end indicator in context
- Calls
FinalizeBedrockStreamto emit closing events- Populates metadata for each final response
- Includes usage information in finalization
The multi-response loop properly iterates over responses from the accumulator-aware conversion, setting metadata and incrementing counters for each response. This aligns with the new multi-part streaming design.
Also applies to: 958-978
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
core/providers/bedrock/responses.go (1)
675-702: Fix tool-call indexing for multiple calls and pointer-to-range-variable bug in finalizationTwo issues here:
Static
outputIndexfor tool calls (multi-tool-call bug)
Incase chunk.Start.ToolUse != nil(around Line 675), you always useoutputIndex := 1and setCurrentOutputIndex = 2. If Bedrock ever emits multiple concurrent tool uses, they will all shareoutput_index = 1, and later deltas/argument buffers will collide.Use
CurrentOutputIndexto allocate unique indices, reserving 0 for text:
// This is a function call starting - use output_index 1outputIndex := 1accumulator.ContentIndexToOutputIndex[contentBlockIndex] = outputIndexaccumulator.CurrentOutputIndex = 2 // Next available index
// Assign a new output index for this tool call (reserve 0 for text)outputIndex := accumulator.CurrentOutputIndexif outputIndex == 0 {outputIndex = 1}accumulator.CurrentOutputIndex = outputIndex + 1accumulator.ContentIndexToOutputIndex[contentBlockIndex] = outputIndex
Arguments: &argson a range variable (allarguments.doneshare the last value)
InFinalizeBedrockStream(around Line 824),for outputIndex, args := range accumulator.ToolArgumentBuffersand thenArguments: &argsreuses the address of the range variable. Everyfunction_call_arguments.donewill end up pointing at the lastargsvalue, corrupting earlier events.Copy
argsper iteration before taking its address:
- for outputIndex, args := range accumulator.ToolArgumentBuffers {
- for outputIndex, args := range accumulator.ToolArgumentBuffers {
if args != "" {
@@
// Emit function_call_arguments.done with full argumentsresponse := &schemas.BifrostResponsesStreamResponse{
// Emit function_call_arguments.done with full argumentsargsCopy := argsresponse := &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDone, SequenceNumber: sequenceNumber + len(responses), OutputIndex: schemas.Ptr(outputIndex),
Arguments: &args,
Arguments: &argsCopy, }Both fixes are important to keep multi-tool-call streams correct and avoid subtle data corruption.
Also applies to: 823-874
core/providers/cohere/responses.go (1)
576-586: Fix potential nil dereference when generating reasoning item IDsIn
ToBifrostResponsesStream, the reasoning branch (Line 611) formats the item ID as:itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) if accumulator.MessageID == nil { itemID = fmt.Sprintf("reasoning_%d", outputIndex) }This dereferences
accumulator.MessageIDbefore checking for nil. If Cohere ever emits aStreamEventContentStartfor a thinking block before amessage_startwith an ID (or with a missing ID), this will panic during streaming.Align this with the safer pattern used for the text branch by checking first:
- // Generate stable ID for reasoning item - itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("reasoning_%d", outputIndex) - } + // Generate stable ID for reasoning item + var itemID string + if accumulator.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("reasoning_%d", outputIndex) + }(Optionally, you can also drop the redundant second
if accumulator.MessageID == nilin the text branch to keep both branches consistent.)Also applies to: 605-614
tests/core-providers/scenarios/tool_calls_streaming.go (1)
60-71: Fix the fallback logic that incorrectly treats Index=0 as "not set".Lines 61-63 and 68-70 contain logic that falls back to
choiceIndexwhentoolCall.Index == 0. SinceIndexis auint16(not a pointer), zero is its default value and also a valid array index for the first tool call. This logic will incorrectly override legitimateIndex=0values.Based on the codebase analysis in the past review,
framework/streaming/accumulator.go:245assignsIndex=uint16(len(existingToolCalls)), meaning the first tool call legitimately receivesIndex=0.Apply this diff to remove the incorrect zero-check:
func (acc *StreamingToolCallAccumulator) AccumulateChatToolCall(choiceIndex int, toolCall schemas.ChatAssistantMessageToolCall) { // Prefer ID as key if available, otherwise use index key := -1 var found bool if toolCall.ID != nil && *toolCall.ID != "" { // Try to find existing tool call by ID first for k, existing := range acc.ChatToolCalls { if existing.ID != nil && *existing.ID == *toolCall.ID { key = k found = true break } } // If not found by ID, use index if !found { key = int(toolCall.Index) - if key == 0 { - key = choiceIndex - } } } else { // Use the tool call index (if available) or choice index as the key key = int(toolCall.Index) - if key == 0 { - key = choiceIndex - } }If you need to distinguish "not provided" from "provided as 0", change the schema field
Indexto*uint16and check for nil. Otherwise, usetoolCall.Indexdirectly.
🧹 Nitpick comments (3)
core/providers/anthropic/anthropic.go (1)
469-477: ChatCompletion streaming: improved error context and explicit end-of-streamThe updates here are solid:
- Line [469-477]: When
BodyStreamis nil, constructing aBifrostOperationError, markingBifrostContextKeyStreamEndIndicator, and sending viaProcessAndSendBifrostErrormakes the failure explicit to downstream hooks/consumers.- Line [548-558]: For
ToBifrostChatCompletionStreamerrors, enrichingExtraFieldswithRequestType,Provider, andModelRequested, then marking the stream as ended before dispatching the error, significantly improves debuggability and ensures consumers see a terminal event.- Line [587-595]: The final synthetic chunk created by
CreateBifrostChatCompletionChunkResponse, with total latency and stream-end indicator, gives a clean “completed” event that matches the new lifecycle semantics.You might optionally consider also setting
BifrostContextKeyStreamEndIndicatorin the scanner error branch for symmetry, unlessProcessAndSendErroralready handles that internally.Also applies to: 548-558, 587-595
core/providers/openai/openai.go (1)
705-715: Chat→Responses fallback path looks correct; consider ChunkIndex semanticsThe Responses-fallback branch in
HandleOpenAIChatCompletionStreamingis well-structured: it usesChatToResponsesStreamAccumulator, propagates provider/model metadata, and correctly marks the stream end on error andcompletedchunks.One nuance:
response.ExtraFields.ChunkIndexis set fromresponse.SequenceNumberrather than a separate per-chunk counter, so in fallback mode the notion of “chunk index” differs from other OpenAI streaming paths (which usually increment once per SSE chunk). If any consumers assumeChunkIndexis a simple 0..N per-stream counter, consider switching to a dedicated per-response counter (e.g., the existingchunkIndex) and keepingSequenceNumberpurely for Responses-level ordering.Also applies to: 873-923, 1004-1012
core/providers/cohere/cohere.go (1)
414-417: Avoid double-incrementingchunkIndexinChatCompletionStream
chunkIndexis incremented once before processing each SSE event (Line 445) and again after emitting a response (Line 474). BecauseExtraFields.ChunkIndexis set between these increments, the sequence becomes1, 3, 5, ...rather than contiguous 0..N.If you want
ChunkIndexto represent a simple per-chunk counter (matching other providers), drop the pre-scan increment and only increment when you actually emit a response:- chunkIndex++ - // Extract response ID from message-start events if event.Type == StreamEventMessageStart && event.ID != nil { responseID = *event.ID } @@ - lastChunkTime = time.Now() - chunkIndex++ + lastChunkTime = time.Now() + chunkIndex++This keeps
ChunkIndexcontiguous and still monotonic.Also applies to: 445-447, 452-462, 465-475, 480-484
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
core/changelog.md(1 hunks)core/providers/anthropic/anthropic.go(7 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(8 hunks)core/providers/bedrock/bedrock.go(5 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(4 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(6 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)transports/changelog.md(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/perplexity/responses.go
- core/providers/utils/utils.go
🔇 Additional comments (51)
core/providers/perplexity/perplexity.go (1)
212-215: LGTM! Aligns with native streaming implementation.The changes correctly mark the fallback path with the context flag and remove the post-hook wrapper, aligning with the PR's shift toward native per-provider streaming implementations instead of chat-to-responses conversion wrappers.
core/providers/openrouter.go (1)
214-214: LGTM! OpenRouter responses API endpoint updated to v1.The endpoint path has been correctly updated from
/alpha/responsesto/v1/responsesfor both standard and streaming responses methods, reflecting the API maturation.Also applies to: 233-233
transports/changelog.md (1)
2-3: LGTM! Changelog accurately documents the PR changes.The entries correctly capture the unified streaming lifecycle events feature and the OpenRouter endpoint migration, aligning with the PR objectives.
core/providers/anthropic/chat.go (1)
718-718: LGTM! Corrects tool block ID source.The change from
chunk.ContentBlock.ToolUseIDtochunk.ContentBlock.IDcorrectly uses the standard ID field for tool-use blocks during the ContentBlockStart event, ensuring proper tool call identification in streaming.core/changelog.md (1)
1-2: LGTM! Changelog accurately reflects core changes.The entries correctly document the unified streaming lifecycle events and the OpenRouter responses API endpoint migration, consistent with the PR's core enhancements.
tests/core-providers/anthropic_test.go (1)
38-38: LGTM! Enables streaming tool call tests for Anthropic.Adding
ToolCallsStreaming: trueappropriately expands test coverage to validate the new streaming lifecycle events for tool calls, aligning with the PR's comprehensive streaming enhancements.tests/core-providers/cohere_test.go (1)
36-36: LGTM! Enables streaming tool call tests for Cohere.Adding
ToolCallsStreaming: trueappropriately extends test coverage to validate streaming tool call functionality for the Cohere provider, consistent with the PR's cross-provider streaming improvements.tests/core-providers/openai_test.go (1)
46-46: LGTM! Enables streaming tool call tests for OpenAI.Adding
ToolCallsStreaming: trueappropriately expands test coverage for OpenAI's streaming tool calls, which serves as the reference implementation for the unified streaming lifecycle events introduced in this PR.tests/core-providers/tests.go (1)
35-35: LGTM! Test scenario integration is consistent.The new
ToolCallsStreamingTestscenario is properly added to both the test execution list and the summary printout, following the existing pattern for test scenarios.Also applies to: 77-77
tests/core-providers/ollama_test.go (2)
35-35: LGTM! Streaming test scenario enabled.The addition of
ToolCallsStreaming: truealigns with the PR objective of testing streaming tool calls across providers.
26-26: I'll search for information about the llama3.1 model in Ollama and its streaming capabilities to verify the change.Based on my verification, Llama 3.1 405B is the first openly available model that rivals the top AI models when it comes to state-of-the-art capabilities in general knowledge, steerability, math, tool use, and multilingual translation. The model has been officially released in the Ollama library and includes multiple size variants (8B, 70B, and 405B).
Ollama now supports tool calling with popular models such as Llama 3.1, with streaming tool calls capability. Llama 3.1 has been fine-tuned on function callings and supports single, nested, and parallel function calling, as well as multi-turn function calling.
The change from
llama3.2tollama3.1:latestis valid and aligns with Ollama's documented support for streaming tool calls in this model family. The codebase also references both models together in other test files (main.go line 128), confirming their compatibility with the project's testing infrastructure.tests/core-providers/groq_test.go (1)
43-43: LGTM! Streaming test enabled for Groq.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for the Groq provider, consistent with the PR's objective.tests/core-providers/sgl_test.go (1)
36-36: LGTM! Streaming test enabled for SGL.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for the SGL provider.tests/core-providers/mistral_test.go (1)
38-38: LGTM! Streaming test enabled for Mistral.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for the Mistral provider.tests/core-providers/azure_test.go (1)
42-42: LGTM! Streaming test enabled for Azure.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for the Azure provider.core/providers/sgl.go (1)
169-177: LGTM! Unified fallback streaming pattern implemented.The
ResponsesStreammethod now:
- Sets the
BifrostContextKeyIsResponsesToChatCompletionFallbackcontext flag to enable the fallback streaming path- Passes
postHookRunnerdirectly instead of wrapping it with a converterThis aligns with the PR's objective of implementing OpenAI-style streaming lifecycle events and standardizing the streaming approach across providers.
core/providers/mistral/mistral.go (1)
202-210: LGTM! Unified fallback streaming pattern implemented.The
ResponsesStreammethod now follows the same pattern as other providers:
- Sets the
BifrostContextKeyIsResponsesToChatCompletionFallbackcontext flag to enable the fallback streaming path- Passes
postHookRunnerdirectly instead of wrapping itThis change standardizes the streaming approach across providers and aligns with the PR's objective of implementing OpenAI-style streaming lifecycle events.
tests/core-providers/config/account.go (1)
29-29: LGTM!The
ToolCallsStreamingfield addition is well-placed and clearly documented. It logically extends the test scenarios to support streaming tool calls functionality.core/providers/groq.go (1)
213-219: LGTM! Consistent streaming refactoring pattern.The changes properly inject the fallback context flag and pass the
postHookRunnerdirectly, aligning with the unified streaming lifecycle approach across providers. This pattern is consistent with similar changes in Cerebras, Ollama, and other providers in this PR.core/providers/cerebras.go (1)
173-179: LGTM! Consistent streaming refactoring.The streaming refactoring follows the same pattern as other providers (Groq, Ollama), properly setting the fallback context flag and passing the
postHookRunnerdirectly for unified lifecycle handling.core/providers/ollama.go (1)
173-179: LGTM! Consistent streaming refactoring.The changes maintain consistency with the streaming refactoring pattern used in Groq and Cerebras providers, properly implementing the unified streaming lifecycle.
tests/core-providers/parasail_test.go (1)
35-35: LGTM! Enabling tool call streaming for Parasail.The addition of
ToolCallsStreaming: trueproperly enables the new streaming functionality for Parasail tests, consistent with other providers in this PR.tests/core-providers/cerebras_test.go (1)
40-40: LGTM! Enabling tool call streaming for Cerebras.The addition of
ToolCallsStreaming: trueenables the new streaming functionality for Cerebras tests, consistent with the broader streaming enhancement across providers.tests/core-providers/bedrock_test.go (1)
40-40: LGTM! Enabling tool call streaming for Bedrock.The addition of
ToolCallsStreaming: trueenables the new streaming functionality for Bedrock tests, completing the streaming enhancement across all supported providers.tests/core-providers/openrouter_test.go (1)
37-39: The review comment is based on an incorrect assumption about flag dependency.End2EndToolCalling and ToolCallsStreaming are independent features with separate test implementations. The End2EndToolCalling test (tests/core-providers/scenarios/end_to_end_tool_calling.go) only guards on its own flag and makes no reference to ToolCallsStreaming. The test conducts a two-step conversation: first requesting a tool call without streaming, then providing the tool result for conversational synthesis—both steps use non-streaming APIs.
OpenRouter's configuration—with ToolCallsStreaming:false but End2EndToolCalling:true—is correct. The Beta API limitation only prevents streaming of tool calls, not the full end-to-end tool calling workflow.
Likely an incorrect or invalid review comment.
core/providers/vertex/vertex.go (1)
665-672: ResponsesStream fallback flag wiring looks correctSetting
BifrostContextKeyIsResponsesToChatCompletionFallbackonctxand delegating directly toChatCompletionStreamkeeps the fallback behavior explicit and consistent with other providers. No issues spotted here.core/schemas/bifrost.go (1)
101-118: New context key for Responses→Chat fallback is well-scoped
BifrostContextKeyIsResponsesToChatCompletionFallbackis added in the same typed-constant block, with clear semantics and comments. This matches how providers now tag fallback streaming and keeps context key usage type-safe.tests/core-providers/gemini_test.go (1)
35-56: Enabling ToolCallsStreaming for Gemini tests is appropriateTurning on
ToolCallsStreamingalongsideToolCallsfor Gemini will exercise the new streaming tool-call path without changing test structure. Looks good.core/providers/parasail.go (1)
144-151: Parasail ResponsesStream correctly tagged as chat-fallbackUsing
context.WithValueto setBifrostContextKeyIsResponsesToChatCompletionFallbackand delegating directly toChatCompletionStreammatches the intended Responses-via-Chat fallback pattern and stays consistent with other providers.core/providers/gemini/gemini.go (3)
346-353: Gemini ResponsesStream now correctly signals chat-fallbackSetting
BifrostContextKeyIsResponsesToChatCompletionFallbackbefore callingChatCompletionStreamkeeps the Responses-over-OpenAI-chat behavior explicit and aligns Gemini with the unified streaming design.
606-620: SpeechStream finalization uses unified end-of-stream handlingSwitching the final “done” speech chunk to:
- mark
BifrostContextKeyStreamEndIndicatoronctx, and- send via
ProcessAndSendResponsewith aBifrostSpeechStreamResponsemakes Gemini speech streaming consistent with other providers’ lifecycle semantics. Logic and latency/usage fields look correct.
863-883: TranscriptionStream final chunk handling is consistent and statefulThe final transcription “done” chunk now:
- aggregates full text and usage,
- sets
ChunkIndexand totalLatency, and- marks
BifrostContextKeyStreamEndIndicatorbefore callingProcessAndSendResponse.This matches the new unified streaming lifecycle; no issues noticed.
tests/core-providers/vertex_test.go (1)
30-46: Vertex ToolCallsStreaming scenario correctly enabledAdding
ToolCallsStreaming: truenext toToolCalls: trueensures Vertex’s tool-call streaming path is exercised by the comprehensive test suite, consistent with other providers.core/providers/anthropic/anthropic.go (1)
726-735: ResponsesStream: accumulator-based streaming and lifecycle look correctThe Responses streaming changes are well-structured:
- Line [726-735]: Handling empty
BodyStreamby emitting aBifrostOperationErrorwithBifrostContextKeyStreamEndIndicatorset ensures consumers see a definitive terminal error.- Line [783-785]: The comment clarifying that
response.created/response.in_progressnow come fromToBifrostResponsesStreamdocuments the new responsibility split nicely.- Line [794-804]: On
ToBifrostResponsesStreamerrors, enrichingExtraFields(ResponsesStreamRequest, provider, model), marking stream-end, and sending viaProcessAndSendBifrostErrorprovides good observability and avoids silent partial streams.- Line [806-832]: Iterating over the
responsesslice, assigning monotonically increasingChunkIndex/latency, optionally attaching raw SSE payload, and on the last chunk:
- ensuring
response.Responseis non-nil,- attaching aggregated
usage,- overriding latency with total stream duration, and
- marking
BifrostContextKeyStreamEndIndicatorbeforeProcessAndSendResponsematches the desired OpenAI-style lifecycle with a clear final “completed” event.
Overall, the accumulator pattern and lifecycle handling look sound.
Also applies to: 783-785, 794-804, 806-832
core/providers/openai/openai.go (1)
542-554: Consistent stream-end signaling across OpenAI streaming handlersUsing
schemas.BifrostContextKeyStreamEndIndicatorbefore the finalProcessAndSendResponse/ProcessAndSendBifrostErrorcall in Text, Chat, Responses, Speech, and Transcription streams makes the lifecycle explicit and consistent for downstream hooks. The wiring and latency handling look correct and aligned across handlers.Also applies to: 860-862, 1340-1344, 1710-1712, 1982-1984
core/providers/bedrock/responses.go (1)
11-25: Accumulator design for Bedrock streaming is sound
BedrockStreamAccumulatorandFlushcover the right state (content→output mapping, tool IDs/names, item IDs, lifecycle flags, timestamps). The reset logic reinitializes maps and timestamps correctly, which should make reuse safe per stream.Also applies to: 27-53
core/providers/cohere/cohere.go (1)
633-640: ResponsesStream accumulator integration for Cohere looks correctCreating a
CohereStreamAccumulatoronce per stream, threading it intoevent.ToBifrostResponsesStream, and then enriching each emittedResponsesStreamResponsewithChunkIndex, latency, and raw payload is consistent with the new OpenAI-style lifecycle. The end-of-stream branch correctly ensures the final response has total latency and sets the stream-end indicator on the context beforeProcessAndSendResponse.Also applies to: 677-723
core/providers/cohere/responses.go (1)
11-47: CohereStreamAccumulator and helpers look solid
CohereStreamAccumulator,NewCohereStreamAccumulator,Flush, andGetOrCreateOutputIndexcover the needed state for Cohere Responses streaming (content indices, tool arguments, stable item IDs, lifecycle flags). Reset semantics and output-index allocation logic look correct for reuse across a single stream.core/schemas/mux.go (2)
960-991: ChatToResponsesStreamAccumulator captures necessary streaming stateThe accumulator fields and
NewChatToResponsesStreamAccumulatorinitialization cover all the pieces needed for Chat→Responses streaming (tool call argument buffers, item IDs, output indices, lifecycle flags, and a monotonicSequenceNumber). This centralizes state management cleanly and keeps callers simple.
993-1350: Chat→Responses streaming conversion logic looks correct and robust
ToBifrostResponsesStreamResponsecleanly translates Chat stream deltas into Responses-style events:
- Emits
response.createdandresponse.in_progressonce per stream using the accumulator.- Creates a stable text item (
output_index:0) on first content delta and emitsoutput_text.deltaevents with consistentitem_id.- Handles tool-call deltas with per-tool-call output indices, argument accumulation, and both
function_call_arguments.deltaandfunction_call_arguments.donefollowed byoutput_item.done.- Properly emits reasoning and refusal deltas, and a final
response.completedwith converted usage.- Uses local copies (
argsCopy) for arguments when emitting.doneevents to avoid pointer-to-range-variable issues.Overall, the sequencing and lifecycle semantics match the intended OpenAI-style Responses streaming model.
core/providers/bedrock/bedrock.go (3)
725-734: LGTM: Error enrichment improves observability.The error handling correctly enriches BifrostError with request metadata (RequestType, Provider, ModelRequested) and sets the stream end indicator before propagating the error. This ensures consistent error reporting across the streaming path.
762-763: LGTM: Stream end indicator correctly set.The stream end indicator is properly set in context before sending the final response, enabling post-hook runners to detect stream completion.
886-908: Verify that all finalization responses should carry the stream-end indicator.The stream end indicator is set at Line 886 before the finalization loop, which means all final responses (including usage, completed events, etc.) will carry the stream-end indicator. If only the last response should signal stream completion, consider moving the context assignment inside the loop:
ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) if err == io.EOF { // End of stream - finalize any open items finalResponses := FinalizeBedrockStream(accumulator, chunkIndex, usage) for i, finalResponse := range finalResponses { finalResponse.ExtraFields = schemas.BifrostResponseExtraFields{ RequestType: schemas.ResponsesStreamRequest, Provider: providerName, ModelRequested: request.Model, ModelDeployment: deployment, ChunkIndex: chunkIndex, Latency: time.Since(lastChunkTime).Milliseconds(), } chunkIndex++ lastChunkTime = time.Now() if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { finalResponse.ExtraFields.RawResponse = "{}" } // Only set stream end indicator on the last response ctxToUse := ctx if i == len(finalResponses)-1 { ctxToUse = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) } providerUtils.ProcessAndSendResponse(ctxToUse, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, finalResponse, nil, nil), responseChan) } break }If the current behavior (all final responses carry the indicator) is intentional for this provider, this comment can be disregarded.
tests/core-providers/scenarios/tool_calls_streaming.go (3)
102-186: LGTM: Robust tool call accumulation with multi-identifier tracking.The method correctly handles accumulation across multiple streaming chunks by:
- Using itemID as the primary identifier (most stable across chunks)
- Falling back to callID or name when itemID is unavailable
- Migrating to better keys when identifiers become available (lines 152-161)
- Detecting complete JSON arguments to handle both incremental and complete "done" events (lines 169-176)
218-743: LGTM: Comprehensive streaming test with detailed observability.The test harness correctly:
- Tests both Chat Completions and Responses API streaming paths
- Implements retry logic to handle non-deterministic LLM responses
- Provides extensive field-level logging (lines 306-317, 511-525) for debugging provider-specific streaming formats
- Validates final accumulated tool calls for completeness
The detailed logging will be valuable for diagnosing provider-specific streaming issues.
745-793: LGTM: Validation logic appropriately handles streaming edge cases.The validation function correctly:
- Checks for required fields (ID, Name, Arguments)
- Attempts JSON parsing without failing on invalid JSON (lines 774-783), which is appropriate since arguments might be partially streamed
- Uses
require.NotEmptyto ensure test failure when critical fields are missingcore/providers/anthropic/responses.go (5)
13-60: LGTM: Well-designed accumulator for stateful streaming conversion.The
AnthropicStreamAccumulatorcorrectly maintains per-output state for streaming:
ContentIndexToOutputIndexmaps Anthropic's content indices to stable output indices (lines 22)ToolArgumentBuffersaccumulates incremental tool arguments (line 23)MCPCallOutputIndicesdistinguishes MCP calls for correct event type emission (line 24)ItemIDsprovides stable item identifiers (line 25)CreatedAtensures consistent timestamps across lifecycle events (line 29)The
Flushmethod properly resets all state (lines 46-60), preventing state leakage across streams.
62-80: LGTM: Output index mapping ensures stable indexing.The method correctly:
- Handles nil content indices by allocating a new output index (lines 64-69)
- Returns existing mappings for known content indices (lines 71-73)
- Creates new mappings and increments the counter atomically (lines 75-79)
This ensures consistent output indexing across streaming chunks, which is essential for OpenAI-style Responses API streaming.
396-442: LGTM: OpenAI-style lifecycle events correctly implemented.The MessageStart handler properly:
- Initializes accumulator state (MessageID, Model, CreatedAt) at lines 399-404
- Emits
response.createdevent with guard flag to prevent duplicates (lines 409-423)- Emits
response.in_progressevent with same CreatedAt for consistency (lines 426-437)- Increments SequenceNumber for each event in the slice (line 433)
- Returns multiple responses to support multi-event emission (line 440)
This aligns with OpenAI's Responses API streaming lifecycle.
574-635: LGTM: Tool argument accumulation correctly handles MCP and function calls.The ContentBlockDelta handler properly:
- Maps content index to stable output index (lines 576-577)
- Initializes and accumulates tool arguments in buffers (lines 609-613)
- Distinguishes MCP calls from regular function calls using
MCPCallOutputIndices(lines 617-621)- Emits the appropriate delta type (
MCPCallArgumentsDeltavsFunctionCallArgumentsDelta) based on the tool type- Attaches item IDs for stable item references (lines 623-634)
This enables correct incremental tool argument streaming for both MCP and standard function calls.
657-762: LGTM: Content block finalization correctly handles all tool types.The ContentBlockStop handler properly:
- Finalizes computer tool calls by parsing accumulated JSON and converting actions (lines 662-712)
- Emits appropriate
arguments.doneevents for tool calls, distinguishing MCP from function calls (lines 718-742)- Cleans up argument buffers and MCP tracking after emission (lines 740-741)
- Emits
output_item.donefor all content blocks with stable item IDs (lines 744-760)- Uses multiple response emission to send both arguments.done and item.done events (lines 717, 753-760)
This ensures complete tool call lifecycle events are emitted in the correct order.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
core/providers/cohere/responses.go (1)
576-587: Fix nil-pointer dereference in reasoning ID generation and remove redundant text ID branch.In
ToBifrostResponsesStream:
- Text item ID generation:
var itemID string if accumulator.MessageID == nil { itemID = fmt.Sprintf("item_%d", outputIndex) } else { itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) } if accumulator.MessageID == nil { itemID = fmt.Sprintf("item_%d", outputIndex) }The second
if accumulator.MessageID == nilis redundant and can be dropped; it reassigns the same value.
- Reasoning item ID generation:
itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) if accumulator.MessageID == nil { itemID = fmt.Sprintf("reasoning_%d", outputIndex) }This dereferences
accumulator.MessageIDbefore checking for nil, which can panic if Cohere ever omits the message ID on amessage_startevent (or if the code is reused in a different context). This is the same issue that was previously flagged in this file and should be fixed defensively.Suggested fix:
- // Generate stable ID for text item - var itemID string - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } else { - itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) - } - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } + // Generate stable ID for text item + var itemID string + if accumulator.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_item_%d", *accumulator.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("item_%d", outputIndex) + } accumulator.ItemIDs[outputIndex] = itemID @@ - // Generate stable ID for reasoning item - itemID := fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) - if accumulator.MessageID == nil { - itemID = fmt.Sprintf("reasoning_%d", outputIndex) - } + // Generate stable ID for reasoning item + var itemID string + if accumulator.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_reasoning_%d", *accumulator.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("reasoning_%d", outputIndex) + } accumulator.ItemIDs[outputIndex] = itemIDThis preserves the ID format while avoiding any nil dereference and cleans up the redundant branch.
Also applies to: 610-616
core/providers/bedrock/responses.go (1)
798-890: FixArgumentspointer-to-range-variable bug inFinalizeBedrockStream.In
FinalizeBedrockStream, you iterate overaccumulator.ToolArgumentBuffers:for outputIndex, args := range accumulator.ToolArgumentBuffers { … response := &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDone, SequenceNumber: sequenceNumber + len(responses), OutputIndex: schemas.Ptr(outputIndex), Arguments: &args, } … responses = append(responses, response) … }Taking
&args(the range variable) means allresponse.Argumentspointers share the same underlying storage; subsequent iterations overwrite the string value, so earlier events can end up seeing the last tool call’s arguments. This is the same class of bug previously flagged in similar code.Suggested fix (make a per-iteration copy before taking the address):
- for outputIndex, args := range accumulator.ToolArgumentBuffers { + for outputIndex, args := range accumulator.ToolArgumentBuffers { if args != "" { itemID := accumulator.ItemIDs[outputIndex] callID := accumulator.ToolCallIDs[outputIndex] toolName := accumulator.ToolCallNames[outputIndex] @@ - // Emit function_call_arguments.done with full arguments - response := &schemas.BifrostResponsesStreamResponse{ + // Emit function_call_arguments.done with full arguments + argsCopy := args + response := &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDone, SequenceNumber: sequenceNumber + len(responses), OutputIndex: schemas.Ptr(outputIndex), - Arguments: &args, + Arguments: &argsCopy, }This ensures each
function_call_arguments.doneevent gets its own stable pointer to the correct arguments string.tests/core-providers/scenarios/tool_calls_streaming.go (1)
60-71: Remove fallback logic that treats Index=0 as "not set".Lines 61-63 and 68-70 fallback to
choiceIndexwhentoolCall.Index == 0, treating zero as "not set". However,Indexis auint16field (not optional), and zero is a valid value for the first tool call. Past reviews confirmed that providers legitimately sendIndex=0for the first tool call.Apply this diff:
} else { // Use the tool call index (if available) or choice index as the key key = int(toolCall.Index) - if key == 0 { - key = choiceIndex - } }And similarly for lines 61-63:
// If not found by ID, use index if !found { key = int(toolCall.Index) - if key == 0 { - key = choiceIndex - } }Based on past reviews.
🧹 Nitpick comments (2)
core/providers/anthropic/anthropic.go (1)
587-595: Consider marking scanner errors as stream-terminal tooFor successful completion, you set
BifrostContextKeyStreamEndIndicatorbefore emitting the final chunk; for empty-body and per-event errors, you do the same. For consistency, you may want to also set this flag in thescanner.Err()path so downstream consumers can uniformly treat any end condition (success or error) as terminal based on the context flag rather than inferring from channel closure only.core/providers/cohere/cohere.go (1)
411-418: Avoid double-incrementingchunkIndexinChatCompletionStream.
chunkIndexis incremented once before parsing the event and again after sending the response:chunkIndex++ // before processing … response.ExtraFields.ChunkIndex = chunkIndex … lastChunkTime = time.Now() chunkIndex++ // after processingThis yields values like 1, 3, 5… instead of contiguous indices and makes the counter harder to reason about.
Consider keeping a single increment (e.g., initialize
chunkIndex := -1and increment only once when you’re about to emit a chunk) soChunkIndexmatches other providers’ semantics and stays continuous.Also applies to: 445-475
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
core/changelog.md(1 hunks)core/providers/anthropic/anthropic.go(7 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(8 hunks)core/providers/bedrock/bedrock.go(5 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(4 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(6 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)transports/changelog.md(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/perplexity/responses.go
- core/providers/utils/utils.go
🔇 Additional comments (47)
core/changelog.md (1)
1-2: Changelog entries are clear and well-documented.Both entries accurately reflect the core changes in this PR: the introduction of unified streaming lifecycle events across providers and the provider path adjustment. The entries follow the standard changelog format and are descriptive without being verbose.
transports/changelog.md (1)
2-3: LGTM!The changelog entries accurately describe the streaming lifecycle improvements and the OpenRouter API version migration.
core/schemas/bifrost.go (1)
117-117: LGTM!The new context key follows naming conventions and clearly signals the fallback streaming mode. It's used consistently across multiple providers to enable unified streaming behavior.
core/providers/perplexity/perplexity.go (1)
212-218: LGTM - Consistent fallback pattern.The changes enable the unified streaming flow by setting the fallback context flag and passing the postHookRunner directly. This pattern is consistently applied across multiple providers (Perplexity, Mistral, SGL, Vertex, etc.), aligning with the PR's objective to standardize streaming lifecycle events.
core/providers/mistral/mistral.go (1)
203-209: LGTM - Consistent fallback pattern.The changes mirror the same pattern applied across other providers, enabling unified streaming behavior through the fallback context flag.
core/providers/sgl.go (1)
170-176: LGTM - Consistent fallback pattern.The changes align with the unified streaming flow implemented across all providers.
core/providers/vertex/vertex.go (1)
666-672: LGTM - Consistent fallback pattern.The changes complete the unified streaming flow implementation across all providers in this PR.
core/providers/openrouter.go (1)
214-214: No issues found — OpenRouter v1 responses API is documented and available.The search results confirm that the
/v1/responsesendpoint is officially documented at OpenRouter with examples, authentication details, and full feature documentation including streaming, tool calling, and web search support. The endpoint change from/alpha/responsesto/v1/responsesis valid and represents movement to an officially versioned API.core/providers/anthropic/chat.go (1)
718-718: ContentBlock.ID is the correct field for tool use blocks.The ContentBlock "id" on tool_use blocks is a unique identifier for that specific tool invocation and is emitted in the content_block_start for tool_use. The code change from
chunk.ContentBlock.ToolUseIDtochunk.ContentBlock.IDis correct and aligns with Anthropic's streaming API specification.core/providers/cerebras.go (1)
171-179: ResponsesStream fallback flag wiring looks correctSetting
BifrostContextKeyIsResponsesToChatCompletionFallbackon the context and delegating toChatCompletionStreamwhile preservingpostHookRunnercleanly marks this as a responses→chat fallback path without changing behavior otherwise.core/providers/ollama.go (1)
171-179: Consistent responses→chat fallback handlingThe added context flag and direct delegation to
ChatCompletionStreamkeep the behavior consistent with other providers while clearly signaling the fallback mode downstream.core/providers/groq.go (1)
211-219: Groq ResponsesStream correctly tagged as chat-fallbackUsing
context.WithValueto setBifrostContextKeyIsResponsesToChatCompletionFallbackbefore callingChatCompletionStreamgives the unified fallback signal with no other behavior changes.core/providers/anthropic/anthropic.go (5)
469-477: Good: empty chat stream now marks terminal errorOn an empty
BodyStream, you now setBifrostContextKeyStreamEndIndicatorand route theBifrostErrorviaProcessAndSendBifrostError, which makes this condition visible as a proper terminal event to downstream handlers.
548-557: Mid-stream Anthropic chat errors are now enriched and terminalEnriching
bifrostErr.ExtraFieldswith request type, provider, and model, then setting the stream-end indicator and sending viaProcessAndSendBifrostError(followed bybreak) makes mid-stream failures much easier to attribute and correctly terminates the stream.
726-735: Good: empty responses stream is surfaced as terminal errorWhen
BodyStreamis nil inResponsesStream, you’re now marking the stream as ended and emitting a structuredBifrostErrorviaProcessAndSendBifrostError, which aligns with the chat-streaming path and avoids silent truncation.
777-805: Responses accumulator error handling is enriched and terminalOn
ToBifrostResponsesStreamerrors, enrichingExtraFieldswithResponsesStreamRequestmetadata, setting the end-indicator, and dispatching viaProcessAndSendBifrostErrorbefore breaking cleanly terminates the stream and gives callers full context about the failure.
806-833: Final responses chunk handling and end-indicator look coherentIterating over
responseswhile:
- assigning consistent
ExtraFields(includingChunkIndex),- optionally attaching raw event data,
- and, for the last response of the last chunk, attaching usage, overriding latency with total duration, setting the end-indicator, and returning after
ProcessAndSendResponsematches the intended multi-part streaming semantics and clearly marks the terminal event.
tests/core-providers/anthropic_test.go (1)
32-49: Enabling ToolCallsStreaming in Anthropic tests aligns with new behaviorTurning on
ToolCallsStreamingin the comprehensive test config ensures the new streaming tool-call path for Anthropic is exercised end-to-end.tests/core-providers/sgl_test.go (1)
36-36: LGTM! Test configuration updated to enable tool call streaming.The addition of
ToolCallsStreaming: truealigns with the PR's objective to add comprehensive tool call streaming tests across providers.tests/core-providers/ollama_test.go (2)
26-26: Verify the model change rationale.The ChatModel was changed from
"llama3.2"to"llama3.1:latest". This change is not mentioned in the PR summary. Please confirm whether this change is intentional and necessary for tool call streaming support.Consider documenting the reason for this model change in the PR description or a code comment, especially if llama3.1 has better tool calling capabilities than llama3.2.
35-35: LGTM! Test configuration updated to enable tool call streaming.The addition of
ToolCallsStreaming: trueis consistent with other provider test configurations.tests/core-providers/openai_test.go (1)
46-46: LGTM! Test configuration updated to enable tool call streaming.The addition aligns with the PR's objective to enable comprehensive tool call streaming tests across all providers.
tests/core-providers/tests.go (2)
35-35: LGTM! Test scenario properly integrated.The
RunToolCallsStreamingTestfunction is correctly added to the test scenarios slice, ensuring it will be executed during comprehensive testing.
77-77: LGTM! Summary output properly updated.The ToolCallsStreaming scenario is correctly included in the test summary output, providing visibility into which providers support this feature.
tests/core-providers/mistral_test.go (1)
38-38: LGTM! Test configuration updated to enable tool call streaming.The addition is consistent with other provider test configurations in this PR.
tests/core-providers/azure_test.go (1)
42-42: LGTM! Test configuration updated to enable tool call streaming.The addition is consistent with other provider configurations. Note that this test is currently skipped (Line 14), so the streaming functionality won't be exercised until the skip is removed.
tests/core-providers/config/account.go (1)
29-29: LGTM! Field properly defined and documented.The
ToolCallsStreamingfield is well-positioned after theToolCallsfield and includes a clear documentation comment. The naming is consistent with the existing test scenario fields.tests/core-providers/parasail_test.go (1)
35-35: LGTM! Test configuration updated to enable tool call streaming.The addition completes the consistent rollout of tool call streaming support across all provider test configurations.
tests/core-providers/cohere_test.go (1)
36-36: LGTM! Streaming tool calls enabled for Cohere.Enabling
ToolCallsStreamingaligns with the PR's goal of unified streaming across providers.tests/core-providers/openrouter_test.go (1)
37-39: LGTM! Appropriate handling of Beta API limitations.Disabling streaming tool calls for OpenRouter's Beta responses API is a sensible approach, with clear inline documentation. Enabling
End2EndToolCallingseparately provides good test coverage for non-streaming scenarios.tests/core-providers/groq_test.go (1)
43-43: LGTM! Streaming tool calls enabled for Groq.Configuration change aligns with the unified streaming approach across providers.
tests/core-providers/cerebras_test.go (1)
40-40: LGTM! Streaming tool calls enabled for Cerebras.Test configuration change is consistent with the broader streaming enablement.
tests/core-providers/bedrock_test.go (1)
40-40: LGTM! Streaming tool calls enabled for Bedrock.Configuration update aligns with unified streaming approach.
core/providers/parasail.go (1)
144-152: LGTM! Clean refactor to unified streaming flow.The context flag
BifrostContextKeyIsResponsesToChatCompletionFallbacksignals the fallback behavior, and passingpostHookRunnerdirectly (removing the wrapper) simplifies the streaming path. This aligns well with the PR's goal of consistent OpenAI-style streaming across providers.tests/core-providers/vertex_test.go (1)
36-36: LGTM! Streaming tool calls enabled for Vertex.Test configuration aligns with unified streaming enablement across providers.
core/providers/gemini/gemini.go (3)
346-354: LGTM! Unified streaming flow for ResponsesStream.Setting the fallback context flag and passing
postHookRunnerdirectly mirrors the pattern in parasail.go, ensuring consistent streaming behavior across providers.
618-619: LGTM! Consistent stream-end handling in SpeechStream.Using
ProcessAndSendResponsewith the stream-end indicator (line 618) provides unified lifecycle event handling for the final chunk, aligning with OpenAI-style streaming patterns.
881-882: LGTM! Consistent stream-end handling in TranscriptionStream.The same stream-end pattern as SpeechStream ensures uniform lifecycle events across streaming operations.
tests/core-providers/gemini_test.go (1)
35-56: EnablingToolCallsStreamingfor Gemini tests looks consistent.Setting
ToolCallsStreaming: truealongsideToolCalls,MultipleToolCalls, andEnd2EndToolCallingmatches the intent of this PR and should exercise the new Responses-style streaming paths for Gemini without regressions.core/providers/openai/openai.go (1)
552-554: Unified stream-end handling and Responses→Chat fallback look sound.
The new Responses→Chat fallback in
HandleOpenAIChatCompletionStreamingcorrectly:
- Detects fallback via
BifrostContextKeyIsResponsesToChatCompletionFallback.- Uses
ChatToResponsesStreamAccumulator+ToBifrostResponsesStreamResponseto emit Responses-style events.- Enriches errors with
RequestType,Provider, andModelRequested, sends them viaProcessAndSendBifrostError, and terminates the stream.- For non-error chunks, sets
ChunkIndex, per-chunk latency, and optionalRawResponse, and emits a finalCompletedresponse with a stream-end indicator.The consistent use of
BifrostContextKeyStreamEndIndicatorplusProcessAndSendResponseat the terminal chunk in:
HandleOpenAITextCompletionStreaming,HandleOpenAIChatCompletionStreaming(non-fallback path),HandleOpenAIResponsesStreaming,SpeechStream, andTranscriptionStream/TranscriptionStream(usage branch)
keeps lifecycle semantics aligned across request types.I don’t see functional issues in these changes; behavior is more uniform and error reporting is richer.
Also applies to: 705-714, 873-923, 1004-1012, 1340-1344, 1708-1712, 1980-1984
core/providers/cohere/cohere.go (1)
633-640: Accumulator-backed ResponsesStream wiring for Cohere looks correct.
- Creating
accumulator := NewCohereStreamAccumulator()once per stream and storingrequest.Modelon it matches the new Responses accumulator pattern.- Passing
chunkIndexintoevent.ToBifrostResponsesStreamand then incrementingchunkIndexonce per emitted response yields contiguousSequenceNumbers across the stream and per-responseChunkIndexmetadata.- Final chunk handling (
isLastChunk && i == len(responses)-1) correctly:
- Ensures
response.Responseis non-nil,- Sets the final latency from
startTime,- Marks the context with
BifrostContextKeyStreamEndIndicator, and- Emits a single terminal
Completedresponse before returning.This part of the integration looks solid.
Also applies to: 677-719
core/providers/cohere/responses.go (1)
11-47: Accumulator structure and helpers are well-shaped.
CohereStreamAccumulator,NewCohereStreamAccumulator,Flush, andGetOrCreateOutputIndexgive you a clear, reusable state container for mapping Cohere streaming events into Responses-style outputs. The separation of concerns (content index mapping, per-output buffers, lifecycle flags) makes the downstream logic inToBifrostResponsesStreameasier to reason about.No changes needed here.
Also applies to: 49-67
core/providers/bedrock/responses.go (2)
11-38: Bedrock stream accumulator design aligns with the new Responses lifecycle.
BedrockStreamAccumulator,NewBedrockStreamAccumulator, andFlushcleanly encapsulate streaming state: content-index mapping, per-output tool-call data, stable item IDs, and lifecycle flags. This matches the pattern used in other providers and is a solid foundation for the downstream conversion logic.No changes needed here.
Also applies to: 40-53
585-596: Responses-style conversion logic for Bedrock stream events looks correct.
Message-start handling:
- Generates a stable message ID once and emits
response.createdandresponse.in_progresswith consistentCreatedAt.- Adds a text
output_item.addedatoutput_index=0with a stable item ID tied to the message ID.Tool-use lifecycle:
- On tool-use start, closes the open text item once via an
output_item.done.- Maps
contentBlockIndextooutput_index=1, stores tool call IDs/names, initializes the argument buffer, and emits anoutput_item.addedwith typefunction_callandstatus=in_progress.Delta handling:
- Text deltas become
output_text.deltawith correctOutputIndex,ContentIndex, and optionalItemID.- Tool-use deltas are accumulated into
ToolArgumentBuffers[outputIndex]and emitted asfunction_call_arguments.deltaevents, also tagged withOutputIndex,ContentIndex, andItemID.Stop reasons are deliberately ignored here, with closure deferred to
FinalizeBedrockStream, which is appropriate given the richer multi-item lifecycle.The overall structure matches the intended OpenAI-style lifecycle and looks good.
Also applies to: 667-697, 737-787, 789-793
core/schemas/mux.go (1)
960-1350: LGTM! Streaming accumulator and lifecycle event handling are well-implemented.The ChatToResponsesStreamAccumulator type and ToBifrostResponsesStreamResponse implementation correctly handle:
- Safe nil checks before dereferencing MessageID
- Proper variable copying to avoid range loop capture issues
- OpenAI-style lifecycle events (created, in_progress, completed)
- Per-output indexing and stable item ID generation
The past review comments confirm that nil dereference and variable capture issues have been addressed.
core/providers/bedrock/bedrock.go (1)
871-978: LGTM! Stateful streaming with accumulator is correctly implemented.The ResponsesStream method properly:
- Initializes the BedrockStreamAccumulator and sets model context
- Sets the stream-end indicator in context before emitting final responses
- Enriches errors with request metadata (provider, model, request type)
- Emits multiple per-event responses with correct metadata
The past review discussion confirms that setting the context end indicator before finalization is the correct approach for Bedrock's event stream handling.
core/providers/anthropic/responses.go (1)
21-809: LGTM! Anthropic streaming refactor with lifecycle events is well-implemented.The updated AnthropicStreamAccumulator and ToBifrostResponsesStream correctly:
- Track per-output state with ContentIndexToOutputIndex mapping
- Emit OpenAI-style lifecycle events (created, in_progress, completed)
- Generate stable item IDs with safe nil checks for MessageID
- Manage tool argument buffers and MCP call tracking
- Properly sequence arguments.done before item.done events
The past review comments confirm that the nil dereference issue in item ID generation has been addressed (lines 485-489).
790f8e2 to
e11b2d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/providers/cohere/responses.go (1)
669-675: Critical: nil pointer deref when generating reasoning item ID.
fmt.Sprintf("msg_%s_reasoning_%d", *state.MessageID, …)dereferencesstate.MessageIDbefore nil-check; will panic when Cohere omits ID in message-start. Guard before dereference.- // Generate stable ID for reasoning item - itemID := fmt.Sprintf("msg_%s_reasoning_%d", *state.MessageID, outputIndex) - if state.MessageID == nil { - itemID = fmt.Sprintf("reasoning_%d", outputIndex) - } + // Generate stable ID for reasoning item + var itemID string + if state.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_reasoning_%d", *state.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("reasoning_%d", outputIndex) + }core/providers/bedrock/responses.go (1)
920-929: Critical: address-of-range-variable bug in finalize.Taking
&argsinsidefor outputIndex, args := range ...makes all emitted.Argumentspoint to the same memory (last value). Copy before taking the address.- response := &schemas.BifrostResponsesStreamResponse{ + argsCopy := args + response := &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDone, SequenceNumber: sequenceNumber + len(responses), OutputIndex: schemas.Ptr(outputIndex), - Arguments: &args, + Arguments: &argsCopy, }
🧹 Nitpick comments (11)
ui/app/workspace/logs/views/filters.tsx (1)
185-186: Consider adding accessibility labels for the loading spinner.The loading spinner lacks accessibility attributes, so screen reader users won't be informed that content is loading.
Apply this diff to improve accessibility:
{isLoading ? ( - <div className="border-primary h-3 w-3 animate-spin rounded-full border border-t-transparent" /> + <div + className="border-primary h-3 w-3 animate-spin rounded-full border border-t-transparent" + role="status" + aria-label="Loading" + /> ) : ( <Check className="text-primary-foreground size-3" /> )}tests/core-providers/scenarios/tool_calls_streaming.go (1)
45-93: Remove unusedchoiceIndexparameter.The
choiceIndexparameter is declared but never used in the method body. After the fixes from previous reviews (commits 6c2ea61 and 790f8e2), the code now correctly usestoolCall.Indexdirectly without fallback, making this parameter obsolete.Apply this diff to clean up the signature:
-func (acc *StreamingToolCallAccumulator) AccumulateChatToolCall(choiceIndex int, toolCall schemas.ChatAssistantMessageToolCall) { +func (acc *StreamingToolCallAccumulator) AccumulateChatToolCall(toolCall schemas.ChatAssistantMessageToolCall) { // Prefer ID as key if available, otherwise use index key := -1Then update the call site at line 313:
- accumulator.AccumulateChatToolCall(choice.Index, toolCall) + accumulator.AccumulateChatToolCall(toolCall)core/providers/cohere/responses.go (3)
635-646: Redundant/contradictory itemID assignment.
itemIDis set for nil/non-nilMessageID, then re-assigned again on Line 642. Remove the duplicate branch to avoid confusion.- var itemID string - if state.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } else { - itemID = fmt.Sprintf("msg_%s_item_%d", *state.MessageID, outputIndex) - } - if state.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } + var itemID string + if state.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_item_%d", *state.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("item_%d", outputIndex) + }
560-601: Emit lifecycle even when chunk.ID is absent.Currently
response.created/response.in_progressare gated bychunk.ID != nil. Some Cohere streams can omit IDs inmessage-start; you should emit lifecycle with a nil ID as allowed by schema and still setCreatedAt.
604-624: Tool plan and text reuse the same output_index and ID; avoid duplicate item.added with same ID.Flow: tool-plan opens a “message” item at output_index 0, then ContentStart closes it and immediately emits another
output_item.addedfor the same output_index/ID. This produces two items with the same ID, which can confuse consumers.Prefer either:
- Treat tool-plan as part of the same text item (don’t close on ContentStart; continue deltas), or
- After closing tool-plan, allocate a fresh output_index for the new text item and remap
ContentIndexToOutputIndex.Also applies to: 747-785, 656-663
core/providers/cohere/cohere.go (1)
445-475: Double-increment of chunkIndex per event.
chunkIndex++at Line 445 and again at Line 474 causes per-event increments twice. Consider a single, consistent increment per emitted chunk.Would you like me to propose a small diff to increment only after successful emit?
core/providers/anthropic/anthropic.go (1)
469-477: Unify stream-end indicator handling across all terminal paths in chat streamingYou now set
BifrostContextKeyStreamEndIndicatorfor the “empty body” case and whenToBifrostChatCompletionStreamreturns abifrostErr, but not whenscanner.Err()is non-nil at the end of the loop. If downstream post-hooks rely on this flag to flush or finalize state, the scanner error path will behave differently.Consider also setting the flag before
ProcessAndSendErrorso every terminal path (normal completion, provider error, transport error, scanner error) consistently marks the stream as ended:- if err := scanner.Err(); err != nil { - logger.Warn(fmt.Sprintf("Error reading %s stream: %v", providerType, err)) - providerUtils.ProcessAndSendError(ctx, postHookRunner, err, responseChan, schemas.ChatCompletionStreamRequest, providerType, modelName, logger) - } else { + if err := scanner.Err(); err != nil { + logger.Warn(fmt.Sprintf("Error reading %s stream: %v", providerType, err)) + ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) + providerUtils.ProcessAndSendError(ctx, postHookRunner, err, responseChan, schemas.ChatCompletionStreamRequest, providerType, modelName, logger) + } else {Also applies to: 587-590
core/providers/anthropic/responses.go (4)
14-33: State struct and pool lifecycle are sound, but map reset and CreatedAt logic can be simplifiedThe
AnthropicResponsesStreamStateshape and pooling are appropriate for per-stream accumulation, andgetOrCreateOutputIndexgives a clean mapping fromcontent_indextooutput_index. Two minor cleanups:
acquireAnthropicResponsesStreamStateandflushboth reset maps and timestamps; withflushupdated to useclear(see comment in anthropic.go),CreatedAtwill always be set byacquire, so theif state.CreatedAt == 0guard inToBifrostResponsesStreamis effectively dead code.- Given
acquirealways overwritesCreatedAt, you can initializeCreatedAtto0in the poolNewfunction to make intent clearer and avoid confusion about when the timestamp is actually taken.These are small readability/perf wins; behavior is already correct.
Also applies to: 35-112, 114-132
529-560: Stable text item IDs are generated safely; minor optional tweakThe new text block path allocates stable
itemIDs peroutput_indexand stores them instate.ItemIDs, and you now guard theMessageIDdereference with a nil check, avoiding the earlier panic risk.If you want to be extra defensive, you could also treat empty
MessageIDas “unset” so you don’t end up with IDs likemsg__item_0:- if state.MessageID == nil { + if state.MessageID == nil || *state.MessageID == "" { itemID = fmt.Sprintf("item_%d", outputIndex) } else { itemID = fmt.Sprintf("msg_%s_item_%d", *state.MessageID, outputIndex) }Not required for correctness, but it keeps IDs a bit cleaner.
561-589: Function and MCP tooloutput_item.addedevents look good; consider addingContentIndexfor consistencyThe function and MCP tool branches correctly:
- Tag items as
function_callvsmcp_call,- Initialize
ToolArgumentBuffers[outputIndex],- Track
ItemIDsand MCP-specific indices for later*_arguments.delta/doneevents.One minor consistency improvement: unlike the computer/text branches, these
output_item.addedevents don’t populateContentIndex. If downstream consumers expectcontent_indexto be set on all output items (even tools), you may want to mirror the text/computer behavior:return []*schemas.BifrostResponsesStreamResponse{{ Type: schemas.ResponsesStreamResponseTypeOutputItemAdded, SequenceNumber: sequenceNumber, OutputIndex: schemas.Ptr(outputIndex), - Item: item, + ContentIndex: chunk.Index, + Item: item, }}, nil, falsePurely optional; current behavior is consistent with some tooling that only keys tools by
output_index.Also applies to: 590-623
709-814: Tool/computercontent_block_stopclosure is correct; small cleanup opportunityFor
content_block_stopyou:
- Correctly finalize the computer tool path by unmarshaling the accumulated JSON into a
ResponsesComputerToolCallAction, emitting a singleoutput_item.done, and clearing the computer-specific fields.- For function/MCP tools, first emit the appropriate
*_arguments.doneevent when arguments were buffered, then anoutput_item.donewith a completed status and stableitem_id, bumpingSequenceNumberwhen needed.Two minor improvements you could make:
- Even when no arguments were accumulated, you might still
delete(state.ToolArgumentBuffers, outputIndex)anddelete(state.MCPCallOutputIndices, outputIndex)to keep the maps lean untilflush()runs.- For non-tool text blocks,
doneItemonly carriesStatusand optionalID. If clients benefit from knowing the message type here, you could optionally reuse the original type (e.g.,message) when available.Neither affects correctness, but they’d tighten the state lifecycle a bit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
core/changelog.md(1 hunks)core/go.mod(1 hunks)core/providers/anthropic/anthropic.go(8 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(7 hunks)core/providers/bedrock/bedrock.go(5 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/bedrock/signer.go(0 hunks)core/providers/bedrock/signer_test.go(0 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(4 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(6 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/filters.tsx(1 hunks)ui/app/workspace/logs/views/logEntryDetailsView.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- core/providers/utils/utils.go
- core/providers/bedrock/signer_test.go
- core/providers/bedrock/signer.go
- core/providers/perplexity/responses.go
🚧 Files skipped from review as they are similar to previous changes (25)
- core/providers/parasail.go
- core/providers/openrouter.go
- core/providers/anthropic/chat.go
- tests/core-providers/openrouter_test.go
- tests/core-providers/tests.go
- tests/core-providers/azure_test.go
- tests/core-providers/sgl_test.go
- core/providers/mistral/mistral.go
- tests/core-providers/vertex_test.go
- core/providers/ollama.go
- tests/core-providers/cerebras_test.go
- tests/core-providers/gemini_test.go
- tests/core-providers/mistral_test.go
- tests/core-providers/anthropic_test.go
- tests/core-providers/bedrock_test.go
- core/providers/gemini/gemini.go
- tests/core-providers/openai_test.go
- tests/core-providers/cohere_test.go
- tests/core-providers/parasail_test.go
- core/schemas/bifrost.go
- core/providers/vertex/vertex.go
- core/providers/sgl.go
- transports/changelog.md
- core/providers/groq.go
- tests/core-providers/config/account.go
🧰 Additional context used
🧬 Code graph analysis (12)
core/providers/cerebras.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/perplexity/perplexity.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/anthropic/anthropic.go (2)
core/schemas/bifrost.go (5)
BifrostContextKeyStreamEndIndicator(111-111)BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)ResponsesStreamRequest(90-90)core/providers/utils/utils.go (3)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)
ui/app/workspace/logs/views/filters.tsx (1)
ui/lib/constants/logs.ts (1)
RequestTypeLabels(77-102)
core/providers/openai/openai.go (3)
core/schemas/bifrost.go (8)
BifrostContextKeyStreamEndIndicator(111-111)BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)BifrostError(351-360)ErrorField(369-376)BifrostErrorExtraFields(418-422)RequestType(81-81)ResponsesStreamRequest(90-90)ChatCompletionStreamRequest(88-88)core/providers/utils/utils.go (6)
ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ProcessAndSendBifrostError(569-599)ProviderSendsDoneMarker(749-758)ProcessAndSendError(605-651)CreateBifrostChatCompletionChunkResponse(684-713)core/schemas/mux.go (3)
ChatToResponsesStreamState(962-978)AcquireChatToResponsesStreamState(1002-1043)ReleaseChatToResponsesStreamState(1046-1077)
core/providers/bedrock/responses.go (2)
core/providers/bedrock/types.go (1)
BedrockStreamEvent(363-380)core/schemas/responses.go (18)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/bedrock/bedrock.go (3)
core/schemas/bifrost.go (6)
BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)BifrostContextKeyStreamEndIndicator(111-111)BifrostResponseExtraFields(282-291)ResponsesStreamRequest(90-90)core/providers/utils/utils.go (4)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ShouldSendBackRawResponse(480-485)core/providers/bedrock/responses.go (1)
FinalizeBedrockStream(875-966)
tests/core-providers/scenarios/tool_calls_streaming.go (3)
core/schemas/chatcompletions.go (7)
ChatAssistantMessageToolCall(483-488)ChatAssistantMessageToolCallFunction(491-494)BifrostChatRequest(11-18)ChatParameters(154-183)ChatTool(201-205)BifrostChatResponse(25-40)ChatStreamResponseChoice(529-531)tests/core-providers/scenarios/utils.go (5)
ToolCallInfo(293-297)CreateBasicChatMessage(218-225)GetSampleChatTool(129-148)CreateBasicResponsesMessage(227-235)GetSampleResponsesTool(150-169)core/schemas/responses.go (7)
BifrostResponsesRequest(32-39)ResponsesParameters(84-111)BifrostResponsesStreamResponse(1412-1450)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)
core/providers/cohere/responses.go (2)
core/providers/cohere/types.go (13)
CohereStreamEvent(381-386)StreamEventMessageStart(366-366)StreamEventContentStart(367-367)CohereContentBlockTypeText(128-128)CohereContentBlockTypeThinking(130-130)StreamEventContentDelta(368-368)StreamEventContentEnd(369-369)StreamEventToolPlanDelta(370-370)StreamEventToolCallStart(371-371)StreamEventToolCallDelta(372-372)StreamEventToolCallEnd(373-373)StreamEventCitationEnd(375-375)StreamEventMessageEnd(376-376)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeReasoning(297-297)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeOutputTextAnnotationAdded(1401-1401)ResponsesStreamResponseTypeOutputTextAnnotationDone(1402-1402)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/cohere/cohere.go (3)
core/schemas/bifrost.go (6)
BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)BifrostContextKeyStreamEndIndicator(111-111)ResponsesStreamRequest(90-90)BifrostResponseExtraFields(282-291)core/providers/utils/utils.go (4)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ShouldSendBackRawResponse(480-485)core/schemas/responses.go (1)
BifrostResponsesResponse(45-82)
core/schemas/mux.go (3)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(25-40)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1378-1378)ResponsesStreamResponseTypeRefusalDelta(1363-1363)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeCompleted(1350-1350)core/schemas/utils.go (1)
Ptr(14-16)
core/providers/anthropic/responses.go (2)
core/providers/anthropic/types.go (5)
AnthropicStreamEvent(312-321)AnthropicContentBlockTypeToolUse(129-129)AnthropicToolNameComputer(184-184)AnthropicContentBlockTypeText(127-127)AnthropicContentBlockTypeMCPToolUse(133-133)core/schemas/responses.go (22)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseType(1345-1345)ResponsesStreamResponseTypeMCPCallArgumentsDelta(1386-1386)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesComputerToolCallAction(590-600)ResponsesMessageTypeComputerCall(282-282)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesStreamResponseTypeMCPCallArgumentsDone(1387-1387)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
🪛 GitHub Actions: Run tests and upload coverage
core/go.mod
[warning] 1-1: CI: Warning: package version mismatches or unrelated warnings detected in workspace
🔇 Additional comments (38)
core/changelog.md (2)
1-2: Changelog entries accurately reflect PR scope.Both entries accurately capture the key changes and follow conventional commit format. The lifecycle events entry reflects the core feature work, while the OpenRouter entry documents a provider-specific configuration update.
Minor observation: The first entry is somewhat verbose. Consider shortening to:
"feat: unified streaming lifecycle events across providers"if brevity is preferred, though the current wording is also acceptable for clarity.
2-2: Verify scope of provider API path changes.The OpenRouter entry indicates a shift from
alpha/responsestov1/responses. Please confirm whether other providers (Bedrock, Cohere, etc.) received similar API path updates that should also be documented in the changelog, or if this change was OpenRouter-specific.ui/app/workspace/logs/views/logEntryDetailsView.tsx (1)
42-44: LGTM! Appropriate text wrapping for log content.The addition of
break-allprevents horizontal overflow when displaying long technical strings (URLs, tokens, IDs) in log entries, which is appropriate for this log viewer component.core/providers/perplexity/perplexity.go (1)
211-219: Verification successful—changes approved.The downstream handler correctly checks the context flag (lines 708-714 in
HandleOpenAIChatCompletionStreaming), properly converts and emits Responses format chunks when the flag is set (lines 874-924), and maintains correct post-hook runner handling throughout. The refactoring pattern is consistent across all providers and integrates cleanly with the unified streaming approach.core/go.mod (1)
32-32: The move of smithy-go to indirect dependencies is correct and requires no action.Verification confirms that smithy-go v1.23.1 is properly placed as an indirect dependency. The codebase contains no direct imports of
github.com/aws/smithy-go, which means it is correctly pulled transitively by aws-sdk-go-v2 v1.39.5. This is the proper Go module state and aligns with best practices—dependencies that are not directly imported should not appear in the direct dependency block.The change appears to be an intentional result of
go mod tidyduring the PR refactoring, not an unintended side effect. AWS SDK v2 functionality remains intact with this dependency structure.core/providers/cerebras.go (1)
173-176: Downstream handling confirmed; verify test coverage scope.The downstream implementation is correct—
openai/openai.golines 708–714 properly check theBifrostContextKeyIsResponsesToChatCompletionFallbackflag and apply the appropriate conversion logic. The code change is sound.No tests matching the Cerebras
ResponsesStreampattern were found in thetests/directory. Per the PR objectives (which specify tests for OpenAI, Anthropic, Bedrock, and Cohere but not Cerebras), confirm whether Cerebras test coverage is in scope for this PR.ui/app/workspace/logs/views/filters.tsx (1)
164-166: LGTM: Empty category filtering works correctly.The filter prevents rendering CommandGroup sections for categories with no options, which improves the UI by avoiding empty sections.
tests/core-providers/ollama_test.go (2)
35-35: EnablingToolCallsStreamingfor Ollama is supported.Ollama has added streaming tool-call support and lists Llama 3.1 as a supported model. However, some client libraries/SDK integrations have reported streaming-with-tools bugs or incomplete support, so behavior can vary by SDK/version. Ensure your Ollama release and client library versions are compatible to avoid unexpected test failures.
26-26: Update the central Ollama configuration and document the model change rationale.The PR changed
ollama_test.gofromllama3.2tollama3.1:latest, which represents an upgrade from the 3B to the 8B model. However, this creates inconsistencies:
- llama3.1:latest is 4.9GB (8B model), while llama3.2 defaults to the smaller 3B variant
tests/core-providers/config/account.go(line 705) still referencesllama3.2tests/core-providers/README.md(line 392) still references llama3.2 as the default- No code comments explain the model version/size change
The
:latesttag is stable (pinned 11 months ago), so that concern is unfounded. Both models support tool calling, so the upgrade to the larger 8B model may be intentional for improved performance.Fix: Update
config/account.goto match the 8B model choice, and add a comment explaining why the larger model is preferred for testing.tests/core-providers/groq_test.go (1)
43-43: LGTM!The addition of the
ToolCallsStreamingflag enables tool call streaming tests for the Groq provider, consistent with the PR's goal of adding comprehensive streaming tool-call test coverage.tests/core-providers/scenarios/tool_calls_streaming.go (5)
18-42: LGTM!The accumulator types and constructor are well-structured for managing streaming tool call fragments across both Chat Completions and Responses APIs. The separation of concerns with dedicated maps for each API type is clean.
182-210: LGTM!The getter methods cleanly convert the internal map structures to slices of
ToolCallInfo, providing a consistent interface for both Chat Completions and Responses API results.
212-737: LGTM!The test orchestration is comprehensive with appropriate retry logic (3 attempts) to handle flaky provider behavior. The extensive per-chunk logging will be valuable for debugging streaming issues across different providers. The circuit breaker at 500 responses prevents runaway streams.
739-787: LGTM!The validation logic strikes a good balance between strictness and pragmatism. It requires non-empty ID, name, and arguments for each tool call, but allows arguments that aren't valid JSON (with a warning), recognizing that streaming can produce intermediate non-valid JSON states. The use of
require.NotEmptyensures the essential fields are present.
162-171: ****The heuristic is not as fragile as initially suggested. The logic at lines 164-171 includes an important guard: it only replaces arguments when
existing.Arguments != "", which means the first delta chunk is always appended, preventing false positives. The done event handler (line 663) provides the definitive override, passing the complete arguments which will correctly replace any partial accumulation. While the heuristic could theoretically misfire on unusual formatting, the combination of the existing arguments guard and the done event override ensures correct behavior in practice, which aligns with how providers (Anthropic, Bedrock, Cohere) consistently emitResponsesStreamResponseTypeFunctionCallArgumentsDoneevents.Likely an incorrect or invalid review comment.
core/providers/cohere/cohere.go (1)
633-637: State pooling: good acquire/release hygiene.Creating and releasing
streamStateoutside the loop withdeferis correct and prevents leaks.core/schemas/mux.go (2)
1110-1140: Lifecycle emission independent of message ID: LGTM.Emitting
response.created/response.in_progresskeyed off the first role delta avoids ID gating pitfalls seen in providers that omit IDs.
1361-1400: Correct fix for pointer-to-range var on arguments.Copying
argsbefore taking its address prevents aliasing across events. Solid.core/providers/openai/openai.go (2)
874-918: Fallback Responses-stream path looks consistent.
- Splitting chat chunks into Responses events and routing via ProcessAndSendResponse is correct.
- Final-chunk end-indicator is set.
Please confirm tests cover tool-call arguments accumulation in fallback mode.
1341-1345: End-indicator propagation on Completed: LGTM.Setting
BifrostContextKeyStreamEndIndicatorbefore the final send aligns with unified stream termination.core/providers/bedrock/responses.go (2)
666-741: Lifecycle and initial text item emission: LGTM.Creating
response.created/in_progressand firstoutput_item.addedis consistent and sequence numbers are monotonic viasequenceNumber + len(responses).
865-869: Finalization properly invoked on stream end — verification successful.FinalizeBedrockStream is called in the EOF handler at end-of-stream (bedrock.go:889) with streamState, sequenceNumber, and usage parameters, confirming the stream finalization flow is correct.
core/providers/bedrock/bedrock.go (5)
725-734: LGTM! Error enrichment follows unified pattern.The error handling correctly enriches
bifrostErrwith metadata (request type, provider, model) and sets the stream end indicator before emission, aligning with the unified error handling approach across providers.
762-763: LGTM! Stream completion properly signaled.The stream end indicator is correctly set in the context before emitting the final response, consistent with the established pattern confirmed in previous reviews.
871-874: LGTM! Accumulator pattern correctly implemented.The stream state accumulator is properly acquired from a pool, initialized with the model, and deferred for release. This enables the stateful tracking needed for OpenAI-style tool call streaming.
886-908: LGTM! Finalization properly emits all pending items.The finalization path correctly:
- Sets the stream end indicator before calling
FinalizeBedrockStream- Emits all final responses (text item done, tool call done events, response.completed) with proper metadata
- Increments
chunkIndexfor each final response to maintain proper orderingAs confirmed in previous reviews, setting the context value before the finalization loop achieves the same goal as calling
HandleStreamEndWithSuccess.
947-977: LGTM! Per-event processing correctly handles multiple lifecycle responses.The refactored streaming logic properly:
- Calls
ToBifrostResponsesStreamwhich now returns multiple responses per event (enabling lifecycle events likeoutput_item.delta,function_call_arguments.delta)- Emits each response with correctly populated metadata (including incrementing
chunkIndexper response)- Enriches errors with request metadata before emission
The iteration ensures proper ordering and tracking of all emitted responses.
core/providers/anthropic/anthropic.go (5)
548-557: Mid-stream chat errors are correctly enriched and terminatedThe extra
BifrostErrorExtraFields(request type, provider, model) plus setting the stream-end indicator beforeProcessAndSendBifrostErrorgives downstream consumers enough context and a clean terminal event. This is aligned with the new streaming conventions.
587-595: Final chat chunk handling and stream-end signaling look consistentOn clean termination you create a final chunk with usage/finish reason, stamp total latency, mark
BifrostContextKeyStreamEndIndicator, and send it throughProcessAndSendResponse. This matches the intended “single terminal response” pattern.
726-735: ResponsesStream: good use of stream-end indicator on empty bodyMarking
BifrostContextKeyStreamEndIndicatorbefore sending the “provider returned an empty response” error keeps this terminal path aligned with the rest of the Responses streaming lifecycle.
794-804: ResponsesStream mid-stream errors are enriched and treated as terminalThe new block that attaches
RequestType,Provider, andModelRequested, sets the stream-end indicator, and dispatches viaProcessAndSendBifrostErroris consistent with the chat stream path and should make error handling much easier to consume.
806-832: Final Responses chunk logic correctly produces a single terminalresponse.completedThe
isLastChunkbranch ensures that the lastResponsesStreamResponsegets usage attached, total latency, and the end-indicator before being sent, and the earlyreturnavoids double-emitting that final message. This fits the OpenAI-style lifecycle you’re targeting.core/providers/anthropic/responses.go (6)
447-494: MessageStart lifecycle emission matches OpenAI Responses semanticsUsing
AnthropicStreamEventTypeMessageStartto emit bothresponse.createdandresponse.in_progress, withstate.MessageID/state.CreatedAtshared across them andSequenceNumberderived from the incomingsequenceNumber, looks correct. TheHasEmittedCreated/HasEmittedInProgressguards also protect you from duplicate lifecycle events if Anthropic ever repeatsmessage_start.
496-527: Computer tool content_block_start handling is correct and nicely isolatedCapturing
ComputerToolID, the associatedChunkIndex, and accumulatingPartialJSONonly when both match lets you special-case the computer tool path without interfering with normal function/MCP streams. Emitting a singleoutput_item.addedfor the computer_call item aligns with the downstream Responses semantics.
626-687: Tool argument buffering maps onto the various*.deltaevents correctlyThe
content_block_deltapath:
- Reuses
output_indexviagetOrCreateOutputIndex,- Distinguishes computer tool accumulation from function/MCP calls,
- Buffers
PartialJSONperoutput_indexand emitsresponse.function_call_arguments.deltavsresponse.mcp_call_arguments.deltabased onMCPCallOutputIndices,- Adds
ItemIDwhen available.This is the right shape to support OpenAI-style tool argument streaming and should interoperate cleanly with the new
output_item.added/donelifecycle.
822-835: MessageStop correctly emits a terminalresponse.completedwith stream completion flagHandling
AnthropicStreamEventTypeMessageStopby emitting aresponse.completedevent that carriesstate.CreatedAtandstate.MessageID, and returningtrueforisLastChunk, gives the upstream streaming loop exactly one place to attach final usage and latency and mark the stream as ended. This aligns with the lifecycle design used inResponsesStream.
689-699: Reasoning deltas and reverse mapping are consistentFor
AnthropicStreamDeltaTypeThinkingyou now includeOutputIndexonResponsesStreamResponseTypeReasoningSummaryTextDelta, and the reverse mapping inToAnthropicResponsesStreamResponseconverts those back intothinkingcontent_block deltas. That should make reasoning streams behave predictably in both directions.Also applies to: 953-964
2472-2590: Computer tool action conversions look correct and reversible enoughThe
convertResponsesToAnthropicComputerAction/convertAnthropicToResponsesComputerActionpair cover all the expected action types (click variants, move, type, keypress, scroll, drag, screenshot, wait) and correctly translate between OpenAI-style fields and Anthropic’s input JSON (including coordinates and scroll direction/amount).There’s some inevitable lossiness around
scroll_amountdue to integer division, but for UI control that’s acceptable and doesn’t impact correctness of the Responses streaming protocol.Also applies to: 2592-2702
e11b2d2 to
dcd8d8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/providers/bedrock/responses.go (1)
900-949: Fix pointer-to-range variable in finalize: Arguments points to reused memoryTaking &args from a map range causes all .Arguments to share the last value. Copy before taking address.
- for outputIndex, args := range state.ToolArgumentBuffers { + for outputIndex, args := range state.ToolArgumentBuffers { if args != "" { ... - response := &schemas.BifrostResponsesStreamResponse{ + argsCopy := args + response := &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDone, SequenceNumber: sequenceNumber + len(responses), OutputIndex: schemas.Ptr(outputIndex), - Arguments: &args, + Arguments: &argsCopy, }
🧹 Nitpick comments (11)
core/changelog.md (1)
1-1: Consider making the first changelog entry more specific.The entry "added unified streaming lifecycle events across all providers to fully align with OpenAI's streaming response types" is generic. Based on the PR objectives, the changes include:
- OpenAI-style lifecycle events (created, in_progress, completed)
- Streaming accumulators for output item tracking
- Tool call streaming improvements with proper indexing
Consider specifying which providers are affected or which event types are now supported to make the changelog more informative.
For example:
- feat: added unified streaming lifecycle events across all providers to fully align with OpenAI's streaming response types. + feat: added OpenAI-style streaming lifecycle events (created, in_progress, completed, output items) with streaming accumulators across Anthropic, Bedrock, and Cohere providers for improved tool call streaming.ui/app/workspace/logs/views/filters.tsx (1)
170-175: Optional: DRY up the per-categoryisLoadingchecksThe
isLoadingcomputation repeatscategory === ...checks that all map to the samefilterDataLoadingflag for models/keys (Lines 172-174). If you want to tighten this up, you could group those categories:-const isLoading = - (category === "Providers" && providersLoading) || - (category === "Models" && filterDataLoading) || - (category === "Selected Keys" && filterDataLoading) || - (category === "Virtual Keys" && filterDataLoading); +const isKeyDataCategory = + category === "Models" || + category === "Selected Keys" || + category === "Virtual Keys"; +const isLoading = + (category === "Providers" && providersLoading) || + (isKeyDataCategory && filterDataLoading);Purely a readability/maintainability tweak; current logic is already correct.
core/providers/bedrock/responses.go (3)
95-129: Prefer clearing maps over re-allocating in flush()Recreating maps each time increases allocations/GC. Use clear(state.Map) to retain capacity and reduce churn; you already use clear() in acquire().
- state.ContentIndexToOutputIndex = make(map[int]int) - state.ToolArgumentBuffers = make(map[int]string) - state.MCPCallOutputIndices = make(map[int]bool) - state.ItemIDs = make(map[int]string) + clear(state.ContentIndexToOutputIndex) + clear(state.ToolArgumentBuffers) + clear(state.MCPCallOutputIndices) + clear(state.ItemIDs)
900-949: Emit tool-call finals in deterministic orderMap iteration order is random, which can make SequenceNumber ordering non-deterministic across tool calls. Sort outputIndex keys before emitting.
- for outputIndex, args := range state.ToolArgumentBuffers { + keys := make([]int, 0, len(state.ToolArgumentBuffers)) + for k := range state.ToolArgumentBuffers { keys = append(keys, k) } + sort.Ints(keys) + for _, outputIndex := range keys { + args := state.ToolArgumentBuffers[outputIndex] if args != "" { ... } }
675-717: Consider more unique Response ID generationUsing CreatedAt-derived "msg_" risks collisions across concurrent streams started within the same second. Prefer a ULID/uuid or include a monotonic counter.
core/schemas/mux.go (1)
1245-1254: Start tool-call OutputIndex at 0 when no text itemYou skip index 0 even if no text deltas ever arrive, leaving outputs starting at 1. Consider assigning 0 when TextItemAdded=false to keep indices compact and consistent.
- outputIndex := state.CurrentOutputIndex - if outputIndex == 0 { - outputIndex = 1 // Skip 0 if text is using it - } + outputIndex := state.CurrentOutputIndex + if state.TextItemAdded { // only skip 0 if text exists + if outputIndex == 0 { outputIndex = 1 } + }tests/core-providers/scenarios/tool_calls_streaming.go (3)
95-180: Distinguish “delta” vs “done” instead of brace-heuristicThe “complete JSON” check via leading/trailing braces is brittle (arrays, whitespace, nested braces). Pass a boolean (isComplete) from the caller based on event type (delta vs arguments.done) and set/replace accordingly.
-func (acc *StreamingToolCallAccumulator) AccumulateResponsesToolCall(callID *string, name *string, arguments *string, itemID *string) { +func (acc *StreamingToolCallAccumulator) AccumulateResponsesToolCall(callID *string, name *string, arguments *string, itemID *string, isComplete bool) { ... - if arguments != nil && *arguments != "" { - argsStr := *arguments - if len(argsStr) > 0 && argsStr[0] == '{' && argsStr[len(argsStr)-1] == '}' && existing.Arguments != "" { - existing.Arguments = argsStr - } else { - existing.Arguments += argsStr - } - } + if arguments != nil && *arguments != "" { + if isComplete { + existing.Arguments = *arguments + } else { + existing.Arguments += *arguments + } + }Then call with isComplete=true for FunctionCallArgumentsDone.
182-197: Return tool calls in stable orderMap iteration is random; sort by ID or Name for deterministic test behavior and logs.
- for _, toolCall := range acc.ChatToolCalls { + keys := make([]int, 0, len(acc.ChatToolCalls)) + for k := range acc.ChatToolCalls { keys = append(keys, k) } + sort.Ints(keys) + for _, k := range keys { + toolCall := acc.ChatToolCalls[k] ... }
539-679: Plumb isComplete=true for arguments.doneUpdate the calls to AccumulateResponsesToolCall to pass isComplete=false for .delta and true for .done to leverage the stronger accumulator semantics above.
- accumulator.AccumulateResponsesToolCall(callID, name, arguments, itemID) + accumulator.AccumulateResponsesToolCall(callID, name, arguments, itemID, false) ... - accumulator.AccumulateResponsesToolCall(callID, name, streamResp.Arguments, itemID) + accumulator.AccumulateResponsesToolCall(callID, name, streamResp.Arguments, itemID, true)core/providers/anthropic/responses.go (2)
97-112: Use clear() to reuse map capacity in flush()Reallocating maps each flush increases GC. Prefer clear() like you do elsewhere.
- state.ContentIndexToOutputIndex = make(map[int]int) - state.ToolArgumentBuffers = make(map[int]string) - state.MCPCallOutputIndices = make(map[int]bool) - state.ItemIDs = make(map[int]string) + clear(state.ContentIndexToOutputIndex) + clear(state.ToolArgumentBuffers) + clear(state.MCPCallOutputIndices) + clear(state.ItemIDs)
822-836: Include usage in response.completed when availableAnthropic MessageStop often carries usage; propagate chunk.Usage into ResponsesResponse. This aligns with OpenAI-style completion payloads.
- response := &schemas.BifrostResponsesResponse{ - CreatedAt: state.CreatedAt, - } + response := &schemas.BifrostResponsesResponse{ + CreatedAt: state.CreatedAt, + } + if chunk.Usage != nil { + response.Usage = &schemas.ResponsesResponseUsage{ + InputTokens: chunk.Usage.InputTokens, + OutputTokens: chunk.Usage.OutputTokens, + TotalTokens: chunk.Usage.InputTokens + chunk.Usage.OutputTokens, + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
core/changelog.md(1 hunks)core/go.mod(1 hunks)core/providers/anthropic/anthropic.go(8 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(7 hunks)core/providers/bedrock/bedrock.go(5 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/bedrock/signer.go(0 hunks)core/providers/bedrock/signer_test.go(0 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(4 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(6 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/filters.tsx(1 hunks)ui/app/workspace/logs/views/logEntryDetailsView.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- core/providers/perplexity/responses.go
- core/providers/bedrock/signer.go
- core/providers/utils/utils.go
- core/providers/bedrock/signer_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- tests/core-providers/config/account.go
- core/schemas/bifrost.go
- core/providers/perplexity/perplexity.go
- tests/core-providers/cerebras_test.go
- core/providers/openrouter.go
- core/providers/cerebras.go
- tests/core-providers/tests.go
- core/providers/sgl.go
- tests/core-providers/groq_test.go
- tests/core-providers/cohere_test.go
- core/providers/vertex/vertex.go
- core/providers/mistral/mistral.go
- tests/core-providers/mistral_test.go
- core/providers/gemini/gemini.go
- core/providers/bedrock/bedrock.go
- transports/changelog.md
🧰 Additional context used
🧬 Code graph analysis (13)
core/providers/groq.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/parasail.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/ollama.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/anthropic/anthropic.go (2)
core/schemas/bifrost.go (5)
BifrostContextKeyStreamEndIndicator(111-111)BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)ResponsesStreamRequest(90-90)core/providers/utils/utils.go (3)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)
core/providers/anthropic/chat.go (1)
ui/lib/types/logs.ts (1)
ContentBlock(100-111)
core/providers/bedrock/responses.go (2)
core/providers/bedrock/types.go (1)
BedrockStreamEvent(363-380)core/schemas/responses.go (17)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/cohere/responses.go (2)
core/providers/cohere/types.go (13)
CohereStreamEvent(381-386)StreamEventMessageStart(366-366)StreamEventContentStart(367-367)CohereContentBlockTypeText(128-128)CohereContentBlockTypeThinking(130-130)StreamEventContentDelta(368-368)StreamEventContentEnd(369-369)StreamEventToolPlanDelta(370-370)StreamEventToolCallStart(371-371)StreamEventToolCallDelta(372-372)StreamEventToolCallEnd(373-373)StreamEventCitationEnd(375-375)StreamEventMessageEnd(376-376)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeReasoning(297-297)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeOutputTextAnnotationAdded(1401-1401)ResponsesStreamResponseTypeOutputTextAnnotationDone(1402-1402)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/openai/openai.go (3)
core/schemas/bifrost.go (8)
BifrostContextKeyStreamEndIndicator(111-111)BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)BifrostError(351-360)ErrorField(369-376)BifrostErrorExtraFields(418-422)RequestType(81-81)ResponsesStreamRequest(90-90)ChatCompletionStreamRequest(88-88)core/providers/utils/utils.go (6)
ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ProcessAndSendBifrostError(569-599)ProviderSendsDoneMarker(749-758)ProcessAndSendError(605-651)CreateBifrostChatCompletionChunkResponse(684-713)core/schemas/mux.go (3)
ChatToResponsesStreamState(962-978)AcquireChatToResponsesStreamState(1002-1043)ReleaseChatToResponsesStreamState(1046-1077)
core/providers/cohere/cohere.go (2)
core/schemas/bifrost.go (6)
BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)BifrostContextKeyStreamEndIndicator(111-111)ResponsesStreamRequest(90-90)BifrostResponseExtraFields(282-291)core/providers/utils/utils.go (4)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ShouldSendBackRawResponse(480-485)
ui/app/workspace/logs/views/filters.tsx (1)
ui/lib/constants/logs.ts (1)
RequestTypeLabels(77-102)
core/schemas/mux.go (4)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(25-40)core/schemas/responses.go (17)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1378-1378)ResponsesStreamResponseTypeRefusalDelta(1363-1363)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeCompleted(1350-1350)core/schemas/utils.go (1)
Ptr(14-16)core/schemas/bifrost.go (2)
RequestType(81-81)ResponsesStreamRequest(90-90)
core/providers/anthropic/responses.go (2)
core/providers/anthropic/types.go (12)
AnthropicStreamEvent(312-321)AnthropicStreamEventTypeMessageStart(301-301)AnthropicStreamEventTypeContentBlockStart(303-303)AnthropicContentBlockTypeToolUse(129-129)AnthropicToolNameComputer(184-184)AnthropicContentBlockTypeText(127-127)AnthropicContentBlockTypeMCPToolUse(133-133)AnthropicStreamEventTypeContentBlockDelta(304-304)AnthropicStreamDeltaTypeText(326-326)AnthropicStreamDeltaTypeInputJSON(327-327)AnthropicStreamEventTypeMessageDelta(306-306)AnthropicStreamEventTypeMessageStop(302-302)core/schemas/responses.go (21)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseType(1345-1345)ResponsesStreamResponseTypeMCPCallArgumentsDelta(1386-1386)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesMessageTypeComputerCall(282-282)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesStreamResponseTypeMCPCallArgumentsDone(1387-1387)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
tests/core-providers/scenarios/tool_calls_streaming.go (3)
core/schemas/chatcompletions.go (7)
ChatAssistantMessageToolCall(483-488)ChatAssistantMessageToolCallFunction(491-494)BifrostChatRequest(11-18)ChatParameters(154-183)ChatTool(201-205)BifrostChatResponse(25-40)ChatStreamResponseChoice(529-531)tests/core-providers/scenarios/utils.go (6)
ToolCallInfo(293-297)CreateBasicChatMessage(218-225)GetSampleChatTool(129-148)SampleToolTypeWeather(69-69)CreateBasicResponsesMessage(227-235)GetSampleResponsesTool(150-169)core/schemas/responses.go (6)
BifrostResponsesRequest(32-39)ResponsesParameters(84-111)BifrostResponsesStreamResponse(1412-1450)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)
🔇 Additional comments (48)
core/changelog.md (1)
2-2: LGTM!The OpenRouter provider path update is clearly documented and follows the changelog entry format.
ui/app/workspace/logs/views/logEntryDetailsView.tsx (1)
42-44: LGTM! Sensible overflow fix for log entries.The addition of
break-allprevents long strings (IDs, URLs, JSON, etc.) from causing horizontal overflow in the log details view. This is a good UI improvement for displaying technical content.Note: This UI change appears unrelated to the main PR objectives (streaming API improvements), but it's a valid enhancement.
core/providers/groq.go (1)
213-219: Fallback implementation is correct and properly integrated.The context flag
BifrostContextKeyIsResponsesToChatCompletionFallbackis correctly defined, set across all applicable providers, and actively consumed in downstream code (e.g.,core/providers/openai/openai.go:708-714) to handle the Responses→ChatCompletion fallback path.ui/app/workspace/logs/views/filters.tsx (1)
164-200: Loading-state handling and filter-group rendering look solidThe updated block correctly:
- Skips categories with no options (Line 165), avoiding empty groups in the menu.
- Uses provider/filter loading booleans instead of string literals for loading detection (Lines 170-175), addressing the earlier brittleness.
- Disables interactions and shows a spinner consistently while loading (Lines 176-191).
No correctness issues stand out here; this is a clear improvement over the previous implementation.
core/providers/ollama.go (1)
172-180: Code changes verified as correct — no issues found.Verification confirms:
The context flag
BifrostContextKeyIsResponsesToChatCompletionFallbackis properly detected and handled inHandleOpenAIChatCompletionStreaming(openai.go:708-711), with correct type checking and usage.The
postHookRunnerparameter is correctly threaded through the call chain:
- Received by
Ollama.ChatCompletionStream- Passed to
HandleOpenAIChatCompletionStreaming(line 149)- Included in the function signature (openai.go:700)
The implementation pattern is consistent with other providers (SGL, Groq, Parasail, etc.), all following the same unified streaming approach.
Test coverage search was inconclusive due to environment limitations, but code correctness is confirmed.
core/go.mod (1)
32-32: Verified: smithy-go dependency correctly marked as indirect.Confirmation: No direct smithy-go imports exist in the codebase. The Bedrock provider imports AWS SDK v2 packages (aws, protocol/eventstream, signer/v4, config), which transitively depend on smithy-go. The indirect marking at line 32 of
core/go.modis correct and requires no changes.tests/core-providers/sgl_test.go (1)
36-36: LGTM! Streaming test coverage enabled.The addition of
ToolCallsStreaming: truealigns with the PR's goal of adding comprehensive streaming tool call tests across providers.tests/core-providers/ollama_test.go (2)
35-35: LGTM! Streaming test coverage enabled.The addition of
ToolCallsStreaming: truealigns with the PR's goal of adding comprehensive streaming tool call tests across providers.
26-26: Based on the verification results, Ollama now supports streaming responses with tool calling, and Llama 3.1 is listed as a compatible model. However, there are important nuances:The model change is supported, but with known limitations:
Ollama supports streaming responses with tool calling, which enables chat applications to stream content and call tools in real time. The test scenarios you're concerned about (ToolCalls, ToolCallsStreaming, and End2EndToolCalling) should be compatible with llama3.1:latest.
That said, multiple recent GitHub issues indicate that streaming tool call support has had reliability challenges in practice. Workarounds for tool calling and streaming are proving highly unreliable and often barely functional, with streaming of the initial tool-calling intent partially working but getting reliably streamed final answers after tool execution fraught with issues.
tests/core-providers/openrouter_test.go (1)
37-39: LGTM! Appropriate configuration for Beta API.Explicitly setting
ToolCallsStreaming: falsewith the Beta comment is sensible, while still enablingEnd2EndToolCallingfor non-streaming tool call tests.tests/core-providers/anthropic_test.go (1)
38-38: LGTM! Streaming test coverage enabled.The addition of
ToolCallsStreaming: truealigns with the PR's goal of adding comprehensive streaming tool call tests across providers.tests/core-providers/vertex_test.go (1)
36-36: LGTM! Streaming test coverage enabled.The addition of
ToolCallsStreaming: truealigns with the PR's goal of adding comprehensive streaming tool call tests across providers.tests/core-providers/azure_test.go (1)
42-42: LGTM! Streaming test coverage enabled.The addition of
ToolCallsStreaming: truealigns with the PR's goal of adding comprehensive streaming tool call tests across providers.core/providers/anthropic/chat.go (2)
705-705: LGTM! Comment cleanup.Trailing whitespace removed.
718-718: I need to examine the context around line 718 to verify this is indeed handling tool_use content blocks.Now let me check the actual code context around line 718:
Based on my comprehensive verification using official Anthropic API documentation, the struct definition in the codebase, and confirmed field mappings from the Spring AI framework:
The change from
chunk.ContentBlock.ToolUseIDtochunk.ContentBlock.IDis correct.The struct definition clearly separates these fields by use case:
IDfield is for tool_use content blocksToolUseIDfield is for tool_result content blocksThe struct in
core/providers/anthropic/types.gocorrectly reflects this distinction with its field comments and JSON tags. The code change aligns with the Anthropic API specification.tests/core-providers/parasail_test.go (1)
35-35: LGTM! Streaming test coverage enabled.The addition of
ToolCallsStreaming: truealigns with the PR's goal of adding comprehensive streaming tool call tests across providers.tests/core-providers/bedrock_test.go (1)
40-40: LGTM! Test configuration properly enables tool call streaming.The addition of
ToolCallsStreaming: truealigns with the PR's objective to test streaming tool calls across all providers.tests/core-providers/openai_test.go (1)
46-46: LGTM! Test configuration properly enables tool call streaming.The addition of
ToolCallsStreaming: truealigns with the PR's objective to test streaming tool calls for OpenAI.core/providers/parasail.go (1)
145-151: LGTM! Parasail correctly adopts the unified fallback mechanism.Setting
BifrostContextKeyIsResponsesToChatCompletionFallbackand passing through the rawpostHookRunneraligns with the global fallback pattern introduced across providers.tests/core-providers/gemini_test.go (1)
41-41: LGTM! Test configuration properly enables tool call streaming.The addition of
ToolCallsStreaming: truealigns with the PR's objective to test streaming tool calls for Gemini.core/providers/anthropic/anthropic.go (4)
469-478: LGTM! Empty body error handling correctly signals stream end.Setting
BifrostContextKeyStreamEndIndicatorbefore callingProcessAndSendBifrostErrorensures downstream consumers know the stream is terminating.
549-558: LGTM! Mid-stream error handling properly enriches and terminates.Enriching errors with
ExtraFieldsand setting the end indicator before emission ensures consistent error reporting across the streaming flow.
746-748: LGTM! Stateful streaming properly manages lifecycle.Acquiring
AnthropicResponsesStreamStatewith deferred release ensures clean resource management for the new per-stream state tracking.
822-832: LGTM! Final chunk handling correctly terminates the stream.The logic properly identifies the last response in the last chunk (
isLastChunk && i == len(responses)-1), sets the end indicator, and returns early to prevent duplicate emissions.core/providers/openai/openai.go (5)
705-715: LGTM! Fallback detection and state lifecycle properly managed.The type-safe context value extraction and deferred state release ensure clean resource management for the Chat-to-Responses fallback path.
877-902: LGTM! Fallback error handling correctly constructs and terminates.Error construction from response fields and early termination with end indicator ensure proper error propagation in the Chat-to-Responses fallback path.
913-918: LGTM! Fallback completion correctly terminates with metrics.Setting total latency and the end indicator before the final send ensures proper stream completion in the Chat-to-Responses fallback path.
1005-1012: LGTM! Non-fallback final emission properly guarded.The conditional check
!isResponsesToChatCompletionsFallbackprevents duplicate final responses, and the end indicator ensures proper stream termination.
552-553: LGTM! End indicators consistently applied across streaming endpoints.All streaming paths (text completion, responses, speech, transcription) correctly set
BifrostContextKeyStreamEndIndicatorbefore final emission, ensuring uniform stream termination signaling.Also applies to: 1343-1344, 1711-1712, 1983-1984
core/providers/cohere/cohere.go (4)
453-462: LGTM! Chat completion streaming correctly handles errors and completion.Error enrichment with
ExtraFieldsand end indicator management ensure consistent error reporting and proper stream termination.Also applies to: 482-484
633-636: LGTM! Stream state properly initialized with model context.Acquiring
CohereResponsesStreamStatewith deferred release and assigning the model ensures stateful tracking across the streaming session.
677-716: LGTM! Multi-response handling correctly processes and terminates.The iteration over responses with final chunk detection (
isLastChunk && i == len(responses)-1) and early return ensures proper per-chunk processing and clean stream termination.
718-719: LGTM! Event data properly reset between events.Resetting
eventDataafter processing prevents stale data accumulation across SSE events.core/providers/cohere/responses.go (8)
12-40: LGTM! Stream state struct and pool properly designed.
CohereResponsesStreamStatecorrectly tracks content-to-output mapping, tool arguments, and lifecycle flags. The pool initialization ensures clean state objects.
42-106: LGTM! State lifecycle methods properly manage resources.The acquire/release/flush pattern ensures clean state reuse. Defensive nil checks and
clear()usage (Go 1.21+) prevent state leakage across streams.
108-126: LGTM! Output index mapping correctly maintains stable indices.The
getOrCreateOutputIndexmethod ensures deterministic mapping from Cohere's content indices to output indices, supporting stable item IDs across deltas.
559-601: LGTM! Lifecycle events correctly emitted with message ID.The message-start handler properly emits
response.createdandresponse.in_progressevents. Gating onchunk.ID != nilis appropriate per maintainer confirmation that Cohere always provides IDs.
635-645: LGTM! ID generation safely handles missing message IDs.The nil checks precede dereferences in both text and reasoning item ID generation, preventing panics when message IDs are absent. Past review concerns have been addressed.
Also applies to: 670-673
740-801: LGTM! Tool plan streaming correctly creates and accumulates deltas.The logic properly creates a message item on first tool plan delta and safely generates IDs with nil guards. Subsequent deltas correctly accumulate via
output_text.deltaevents.
803-948: LGTM! Tool call lifecycle correctly implements OpenAI-style events.The sequence—closing prior items, creating function_call with arguments buffer, accumulating deltas, emitting arguments.done and item.done—properly mirrors the OpenAI Responses API streaming pattern.
1018-1055: LGTM! Stream completion correctly emits final event with usage.The message-end handler properly constructs
response.completedwith usage metrics and returnsisLastChunk = true, signaling the end of the stream.core/providers/bedrock/responses.go (2)
669-705: Lifecycle emission looks correct and contiguouscreated then in_progress use sequenceNumber + len(responses); no local increments that would skip numbers. Good.
750-773: Closing text item before tool-call start is correctEmits output_item.done for text, then opens function_call with contiguous sequencing. Good.
core/schemas/mux.go (3)
1001-1043: State reset is solidMaps cleared and scalar fields reset; CreatedAt set on acquire for per-stream consistency. Looks good.
1110-1139: Lifecycle events sequencing is correctresponse.created then response.in_progress with monotonic state.SequenceNumber. Good.
1361-1422: Good fix: copy args before taking addressUsing argsCopy avoids the address-of-range-variable pitfall and preserves prior events. 👍
core/providers/anthropic/responses.go (2)
529-560: Stable item ID generation avoids nil deref — goodFallback to "item_" when MessageID is nil prevents panics on re-ordered streams. 👍
869-912: Verify mapping: output_item.added → message_start for non-computer itemsFor ResponsesStreamResponseTypeOutputItemAdded, the non-computer branch emits Anthropic message_start. Typically item-added maps closer to content_block_start (e.g., text/tool-use). Please verify against Anthropic SSE expectations; adjust to content_block_start if needed.
dcd8d8e to
cce49c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/bedrock/bedrock.go (1)
64-72: Fix invalidfor rangeoverconfig.ConcurrencyAndBufferSize.ConcurrencyThe pre-warm loop in
NewBedrockProvideris invalid Go.Concurrencyis aninttype (core/schemas/provider.go:56), andfor rangeonly works with arrays, slices, strings, maps, channels, or pointers to arrays—not integers. The code would fail to compile.Additionally, the nested loop causes
Concurrency²items to be pooled instead ofConcurrency. Both Cohere and Anthropic providers correctly use a single loop.Fix by removing the outer
for rangeloop:- // Pre-warm response pools - for range config.ConcurrencyAndBufferSize.Concurrency { - for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { - bedrockChatResponsePool.Put(&BedrockConverseResponse{}) - } - } + // Pre-warm response pools + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { + bedrockChatResponsePool.Put(&BedrockConverseResponse{}) + }
♻️ Duplicate comments (2)
core/providers/cohere/responses.go (1)
670-673: 🔴 Critical: Nil pointer dereference in reasoning item ID generation.Line 670 dereferences
state.MessageIDbefore checking if it's nil on line 671. This will panic whenMessageIDis nil.Apply this diff to fix:
- // Generate stable ID for reasoning item - itemID := fmt.Sprintf("msg_%s_reasoning_%d", *state.MessageID, outputIndex) - if state.MessageID == nil { - itemID = fmt.Sprintf("reasoning_%d", outputIndex) - } + // Generate stable ID for reasoning item + var itemID string + if state.MessageID != nil { + itemID = fmt.Sprintf("msg_%s_reasoning_%d", *state.MessageID, outputIndex) + } else { + itemID = fmt.Sprintf("reasoning_%d", outputIndex) + }Note: Past review comments indicate this bug was supposedly addressed, but it persists in the current code. Based on learnings.
core/providers/bedrock/responses.go (1)
661-872: Fix pointer-to-range-variable bug in FinalizeBedrockStream’sArgumentsfieldIn
FinalizeBedrockStream, you have:for outputIndex, args := range state.ToolArgumentBuffers { if args != "" { ... response := &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDone, SequenceNumber: sequenceNumber + len(responses), OutputIndex: schemas.Ptr(outputIndex), Arguments: &args, } ... responses = append(responses, response) ... } }Because
argsis the loop variable in afor … rangeover a map, Go reuses that variable for each iteration. Storing&argsmeans allFunctionCallArgumentsDoneresponses end up pointing at the same memory and will see the lastargsstring, corrupting earlier tool-call completions. This is the same issue previously flagged in older commits.You need a per-iteration copy before taking the address:
- for outputIndex, args := range state.ToolArgumentBuffers { + for outputIndex, args := range state.ToolArgumentBuffers { if args != "" { itemID := state.ItemIDs[outputIndex] callID := state.ToolCallIDs[outputIndex] toolName := state.ToolCallNames[outputIndex] ... - // Emit function_call_arguments.done with full arguments - response := &schemas.BifrostResponsesStreamResponse{ + // Emit function_call_arguments.done with full arguments + argsCopy := args + response := &schemas.BifrostResponsesStreamResponse{ Type: schemas.ResponsesStreamResponseTypeFunctionCallArgumentsDone, SequenceNumber: sequenceNumber + len(responses), OutputIndex: schemas.Ptr(outputIndex), - Arguments: &args, + Arguments: &argsCopy, }This ensures each
function_call_arguments.donecarries the correct arguments string corresponding to its own tool call.Also applies to: 874-965
🧹 Nitpick comments (6)
core/providers/cohere/responses.go (1)
636-644: Refactor: Remove redundant nil check.Lines 637-641 correctly handle the nil check, but lines 642-644 redundantly check the same condition and overwrite the result. This is inefficient but not a bug.
Apply this diff to remove the redundancy:
// Generate stable ID for text item var itemID string if state.MessageID == nil { itemID = fmt.Sprintf("item_%d", outputIndex) } else { itemID = fmt.Sprintf("msg_%s_item_%d", *state.MessageID, outputIndex) } - if state.MessageID == nil { - itemID = fmt.Sprintf("item_%d", outputIndex) - } state.ItemIDs[outputIndex] = itemIDcore/providers/anthropic/anthropic.go (1)
469-478: ChatCompletionStream stream-end signaling and error enrichment look correct; consider aligning scanner error pathThe new handling for:
- empty body (
resp.BodyStream() == nil),- mid-stream
bifrostErrfromToBifrostChatCompletionStream, and- the final synthetic chunk
correctly sets
BifrostContextKeyStreamEndIndicatorinctx, enrichesBifrostErrorExtraFields, and routes throughProcessAndSendBifrostError/ProcessAndSendResponse. This matches the intended OpenAI-style lifecycle and centralizes stream termination behavior.One minor consistency gap: in the
scanner.Err()branch (Lines 587–590) you still callProcessAndSendErrorwithout setting the stream-end indicator or attachingBifrostErrorExtraFields. If downstream components rely on the context key to detect terminal events, you may want to set the end-indicator and wrap this error similarly to other mid-stream failures.Also applies to: 548-558, 587-595
core/providers/cohere/cohere.go (1)
414-417: Avoid double-incrementingchunkIndexin ChatCompletionStreamInside
ChatCompletionStreamyou currently:
- Increment
chunkIndexonce per SSE line beforeToBifrostChatCompletionStream()(Line 445), and- Increment
chunkIndexagain after emitting a response (Line 474).Because
response.ExtraFields.ChunkIndexis set using the value between these two increments, the effective sequence ofchunk_indexvalues becomes 1, 3, 5, … instead of contiguous 0, 1, 2, … and differs from other providers’ behavior.Unless you deliberately want to encode both “frame index” and “emitted chunk index” into the same counter, it would be cleaner and more consistent to increment
chunkIndexin just one place—typically only when you actually emit a chunk (i.e., remove the pre-ToBifrostChatCompletionStreamincrement).Also applies to: 445-447, 452-462, 464-487
core/schemas/mux.go (1)
1082-1436: Chat→Responses streaming conversion is comprehensive; be aware of single-tool-call-per-chunk behavior
ToBifrostResponsesStreamResponsenow:
- Emits
response.createdandresponse.in_progressonce per stream when it sees the first chunk with adelta.Role.- Lazily creates and tracks a text output item (
output_index:0) and streams text viaresponse.output_text.delta, closing it withoutput_item.donewhen tool calls start or atFinishReason.- Handles tool calls by:
- Resolving a stable tool call ID (using
IDwhen present, otherwiseToolCallIndexToID).- Assigning each tool call an
output_index(starting from 1), emittingoutput_item.addedfor the function call, and accumulating arguments inToolArgumentBuffers.- Streaming
response.function_call_arguments.deltaas arguments arrive.- On completion, it:
- Closes any open text item with
output_item.done.- Iterates
ToolArgumentBuffersand emitsresponse.function_call_arguments.done(usingargsCopyto avoid pointer-to-range issues) followed byoutput_item.donefor each tool call.- Emits a final
response.completedwith usage converted viaToResponsesResponseUsage.- Finally, it stamps all emitted responses with
ExtraFields.RequestType = ResponsesStreamRequestand copies search results / videos / citations.Two minor behavioral notes:
- Only the first
delta.ToolCallsentry in a chunk is processed; if a provider ever emits multiple tool calls in a single delta, those additional calls will be ignored. If multi-tool-call-per-chunk becomes relevant, you may want to iterate the slice instead of taking just index 0.- For tool-call–only streams (no text deltas), the first tool call uses
output_index1, with index 0 effectively unused. This is structurally valid but slightly different from “first output item is always index 0”; if clients assume contiguous 0-based indices regardless of content type, you may want to conditionally reserve0only when a text item is actually created.Functionally, though, the conversion is consistent and should handle the intended streaming tool-call scenarios correctly.
tests/core-providers/scenarios/tool_calls_streaming.go (1)
160-172: Simplify the complete vs incremental arguments detection.The heuristic at lines 162-171 uses JSON shape (starts with
{, ends with}) andexisting.Arguments != ""to decide whether to replace or append. This is fragile because:
- If the first chunk happens to be complete JSON, it appends (line 170)
- If a subsequent chunk repeats the complete JSON, it replaces (line 167)
- The logic conflates "looks complete" with "is from a done event"
Consider tracking the source event type (delta vs done) in the caller and passing a flag like
isComplete boolto make the logic explicit and less error-prone.core/providers/anthropic/responses.go (1)
98-112: Useclear()consistently in flush() method.The
acquireAnthropicResponsesStreamStatefunction usesclear(map)(lines 59, 64, 69, 74), butflush()recreates maps withmake()(lines 102-105). For consistency and efficiency, useclear()in both places.Apply this diff:
func (state *AnthropicResponsesStreamState) flush() { state.ChunkIndex = nil state.AccumulatedJSON = "" state.ComputerToolID = nil - state.ContentIndexToOutputIndex = make(map[int]int) - state.ToolArgumentBuffers = make(map[int]string) - state.MCPCallOutputIndices = make(map[int]bool) - state.ItemIDs = make(map[int]string) + clear(state.ContentIndexToOutputIndex) + clear(state.ToolArgumentBuffers) + clear(state.MCPCallOutputIndices) + clear(state.ItemIDs) state.CurrentOutputIndex = 0 state.MessageID = nil state.Model = nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
core/changelog.md(1 hunks)core/go.mod(1 hunks)core/providers/anthropic/anthropic.go(8 hunks)core/providers/anthropic/chat.go(2 hunks)core/providers/anthropic/responses.go(7 hunks)core/providers/bedrock/bedrock.go(5 hunks)core/providers/bedrock/responses.go(3 hunks)core/providers/bedrock/signer.go(0 hunks)core/providers/bedrock/signer_test.go(0 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere/cohere.go(4 hunks)core/providers/cohere/responses.go(4 hunks)core/providers/gemini/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral/mistral.go(1 hunks)core/providers/ollama.go(1 hunks)core/providers/openai/openai.go(6 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/perplexity/perplexity.go(1 hunks)core/providers/perplexity/responses.go(0 hunks)core/providers/sgl.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/vertex.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/mux.go(3 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/config/account.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/tool_calls_streaming.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/tests.go(2 hunks)tests/core-providers/vertex_test.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/filters.tsx(1 hunks)ui/app/workspace/logs/views/logEntryDetailsView.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- core/providers/perplexity/responses.go
- core/providers/utils/utils.go
- core/providers/bedrock/signer.go
- core/providers/bedrock/signer_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- tests/core-providers/sgl_test.go
- tests/core-providers/groq_test.go
- core/providers/mistral/mistral.go
- core/providers/gemini/gemini.go
- core/providers/sgl.go
- core/providers/vertex/vertex.go
- tests/core-providers/cerebras_test.go
- tests/core-providers/vertex_test.go
- core/providers/anthropic/chat.go
- core/providers/parasail.go
- core/schemas/bifrost.go
- core/providers/ollama.go
- core/providers/groq.go
- ui/app/workspace/logs/views/logEntryDetailsView.tsx
- tests/core-providers/mistral_test.go
- tests/core-providers/ollama_test.go
- core/providers/perplexity/perplexity.go
- core/go.mod
- core/changelog.md
- tests/core-providers/openai_test.go
- tests/core-providers/parasail_test.go
- tests/core-providers/bedrock_test.go
🧰 Additional context used
🧬 Code graph analysis (13)
core/providers/cerebras.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)
core/providers/openai/openai.go (4)
core/schemas/bifrost.go (8)
BifrostContextKeyStreamEndIndicator(111-111)BifrostContextKeyIsResponsesToChatCompletionFallback(117-117)BifrostError(351-360)ErrorField(369-376)BifrostErrorExtraFields(418-422)RequestType(81-81)ResponsesStreamRequest(90-90)ChatCompletionStreamRequest(88-88)core/providers/utils/utils.go (6)
ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ProcessAndSendBifrostError(569-599)ProviderSendsDoneMarker(749-758)ProcessAndSendError(605-651)CreateBifrostChatCompletionChunkResponse(684-713)core/schemas/mux.go (3)
ChatToResponsesStreamState(962-978)AcquireChatToResponsesStreamState(1002-1043)ReleaseChatToResponsesStreamState(1046-1077)core/schemas/responses.go (2)
ResponsesStreamResponseTypeError(1409-1409)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/anthropic/anthropic.go (2)
core/schemas/bifrost.go (5)
BifrostContextKeyStreamEndIndicator(111-111)BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)ResponsesStreamRequest(90-90)core/providers/utils/utils.go (3)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)
core/providers/bedrock/responses.go (2)
core/providers/bedrock/types.go (1)
BedrockStreamEvent(363-380)core/schemas/responses.go (18)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/cohere/cohere.go (3)
core/schemas/bifrost.go (6)
BifrostErrorExtraFields(418-422)RequestType(81-81)ChatCompletionStreamRequest(88-88)BifrostContextKeyStreamEndIndicator(111-111)ResponsesStreamRequest(90-90)BifrostResponseExtraFields(282-291)core/providers/utils/utils.go (4)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ShouldSendBackRawResponse(480-485)core/schemas/responses.go (1)
BifrostResponsesResponse(45-82)
core/providers/bedrock/bedrock.go (3)
core/schemas/bifrost.go (4)
BifrostErrorExtraFields(418-422)RequestType(81-81)BifrostContextKeyStreamEndIndicator(111-111)BifrostResponseExtraFields(282-291)core/providers/utils/utils.go (4)
ProcessAndSendBifrostError(569-599)ProcessAndSendResponse(533-563)GetBifrostResponseForStreamResponse(780-808)ShouldSendBackRawResponse(480-485)core/providers/bedrock/responses.go (1)
FinalizeBedrockStream(875-966)
tests/core-providers/tests.go (1)
tests/core-providers/scenarios/tool_calls_streaming.go (1)
RunToolCallsStreamingTest(213-737)
core/schemas/mux.go (4)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(25-40)core/schemas/responses.go (20)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1378-1378)ResponsesStreamResponseTypeRefusalDelta(1363-1363)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesResponseUsage(250-257)ResponsesStreamResponseTypeCompleted(1350-1350)core/schemas/utils.go (1)
Ptr(14-16)core/schemas/bifrost.go (2)
RequestType(81-81)ResponsesStreamRequest(90-90)
core/providers/cohere/responses.go (2)
core/providers/cohere/types.go (1)
CohereStreamEvent(381-386)core/schemas/responses.go (16)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessage(304-316)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeOutputTextAnnotationAdded(1401-1401)ResponsesStreamResponseTypeOutputTextAnnotationDone(1402-1402)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/anthropic/responses.go (2)
core/providers/anthropic/types.go (12)
AnthropicStreamEvent(312-321)AnthropicStreamEventTypeMessageStart(301-301)AnthropicStreamEventTypeContentBlockStart(303-303)AnthropicContentBlockTypeToolUse(129-129)AnthropicToolNameComputer(184-184)AnthropicContentBlockTypeText(127-127)AnthropicContentBlockTypeMCPToolUse(133-133)AnthropicStreamEventTypeContentBlockDelta(304-304)AnthropicStreamDeltaTypeText(326-326)AnthropicStreamDeltaTypeInputJSON(327-327)AnthropicStreamEventTypeMessageDelta(306-306)AnthropicStreamEventTypeMessageStop(302-302)core/schemas/responses.go (21)
BifrostResponsesStreamResponse(1412-1450)BifrostResponsesResponse(45-82)ResponsesStreamResponseTypeCreated(1348-1348)ResponsesStreamResponseTypeInProgress(1349-1349)ResponsesMessageTypeMessage(280-280)ResponsesInputMessageRoleAssistant(321-321)ResponsesMessage(304-316)ResponsesMessageContent(328-333)ResponsesMessageContentBlock(388-399)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesToolMessage(450-470)ResponsesStreamResponseTypeOutputTextDelta(1360-1360)ResponsesStreamResponseType(1345-1345)ResponsesStreamResponseTypeMCPCallArgumentsDelta(1386-1386)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesMessageTypeComputerCall(282-282)ResponsesStreamResponseTypeOutputItemDone(1355-1355)ResponsesStreamResponseTypeMCPCallArgumentsDone(1387-1387)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)ResponsesStreamResponseTypeCompleted(1350-1350)
core/providers/openrouter.go (1)
core/providers/utils/utils.go (1)
GetPathFromContext(209-214)
ui/app/workspace/logs/views/filters.tsx (1)
ui/lib/constants/logs.ts (1)
RequestTypeLabels(77-102)
tests/core-providers/scenarios/tool_calls_streaming.go (3)
core/schemas/chatcompletions.go (7)
ChatAssistantMessageToolCall(483-488)ChatAssistantMessageToolCallFunction(491-494)BifrostChatRequest(11-18)ChatParameters(154-183)ChatTool(201-205)BifrostChatResponse(25-40)ChatStreamResponseChoice(529-531)tests/core-providers/scenarios/utils.go (5)
ToolCallInfo(293-297)CreateBasicChatMessage(218-225)GetSampleChatTool(129-148)CreateBasicResponsesMessage(227-235)GetSampleResponsesTool(150-169)core/schemas/responses.go (7)
BifrostResponsesRequest(32-39)ResponsesParameters(84-111)BifrostResponsesStreamResponse(1412-1450)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1366-1366)ResponsesStreamResponseTypeOutputItemAdded(1354-1354)ResponsesMessageTypeFunctionCall(285-285)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1367-1367)
🔇 Additional comments (30)
transports/changelog.md (1)
2-3: LGTM! Changelog entries accurately reflect the PR scope.Both entries clearly capture the major changes: unified streaming lifecycle events alignment with OpenAI-style formatting across all providers, and the OpenRouter API path upgrade. The entries follow the established changelog format and style.
tests/core-providers/openrouter_test.go (2)
37-37: Appropriate to disable streaming for Beta API.Disabling
ToolCallsStreamingis a sensible decision given that OpenRouter's responses API is in Beta and may not fully support the enhanced OpenAI-style streaming lifecycle events introduced in this PR.
39-39: Reconcile Beta API dependency with feature enablement.
End2EndToolCallingis enabled whileToolCallsStreaming,ImageURL, andImageBase64remain disabled—all due to OpenRouter's Responses API being in Beta. The test uses the Responses API, making the inconsistency unclear. Either add a comment explaining whyEnd2EndToolCallingis exempt from Beta concerns, or align the configuration by disabling it to match the conservative approach applied to other Beta-dependent features.core/providers/openrouter.go (1)
214-214: LGTM! OpenRouter v1/responses endpoint is verified and properly implemented.The v1/responses endpoint is in beta stage, and this migration from
/alpha/responsesto/v1/responsesis appropriately made across bothResponsesandResponsesStreammethods. The API is stateless with each request independent, making this endpoint update straightforward. The use ofGetPathFromContextprovides good flexibility for context-based overrides.ui/app/workspace/logs/views/filters.tsx (1)
164-200: Nice improvement to loading state handling!The changes successfully address the previous review concern about incomplete loading detection. The implementation now:
- Covers all loading categories (lines 170-174): Providers, Models, Selected Keys, and Virtual Keys all show loading indicators consistently.
- Filters empty categories (line 165): Prevents rendering filter groups with no options, improving the UI.
- Provides consistent visual feedback (lines 181-191): Loading spinner and checkmark states are clearly distinguished.
The loading items are properly disabled (line 179) and non-interactive during data fetch, which prevents any edge cases with user interaction.
core/providers/cerebras.go (1)
172-180: Approve fallback pattern implementation; context flag flow and postHookRunner delegation verified.The changes correctly implement the responses-to-chat-completion fallback. Verification confirms:
- Context flag:
BifrostContextKeyIsResponsesToChatCompletionFallbackis properly set (line 173) and consumed in the OpenAI handler (core/providers/openai/openai.go lines 708–712) to acquireChatToResponsesStreamStatefor conversion.- PostHookRunner: Correctly passed directly to
ChatCompletionStream(line 176), which accepts this parameter (core/providers/cerebras.go line 135).- Conversion logic: Handled via
ChatToResponsesStreamState.ToBifrostResponsesStreamResponse()in the OpenAI handler.Comprehensive tests exist (tests/core-providers/cerebras_test.go) with streaming scenarios enabled. Verify that the
ResponsesStreammethod is covered by the streaming test suite.core/providers/openai/openai.go (2)
705-716: LGTM! Fallback pathway for Responses-to-Chat-Completion is well-structured.The introduction of the fallback detection mechanism and state management for converting Chat Completion streams to Responses format is clean and follows the established patterns in the codebase. The state is properly acquired and released via defer.
552-553: LGTM! Stream end indicators consistently applied.The unified approach of setting
BifrostContextKeyStreamEndIndicatorin context before final sends across all streaming endpoints (text completion, chat completion, responses, speech, transcription) provides consistent stream lifecycle signaling.Also applies to: 1011-1012, 1343-1344, 1711-1712, 1983-1984
core/providers/cohere/responses.go (1)
12-127: LGTM! Well-structured streaming state management.The introduction of
CohereResponsesStreamStatewith sync.Pool-based lifecycle management, deterministic output index mapping viagetOrCreateOutputIndex, and comprehensive state tracking for content blocks, tool calls, and citations provides robust streaming coordination. The acquire/release pattern withflushensures proper cleanup.tests/core-providers/azure_test.go (1)
42-42: LGTM! Test configuration updated for streaming tool calls.The addition of
ToolCallsStreaming: trueenables streaming tool call tests for Azure, aligning with the broader PR objective of enhancing Responses API streaming across providers.tests/core-providers/config/account.go (1)
29-29: LGTM! New test scenario field added.The
ToolCallsStreamingfield is properly documented and follows the established pattern for test scenario flags in theTestScenariosstruct.tests/core-providers/anthropic_test.go (1)
38-38: LGTM! Anthropic streaming tool calls enabled.Consistent with the PR's goal of enabling streaming tool call tests across providers.
tests/core-providers/cohere_test.go (1)
36-36: LGTM! Cohere streaming tool calls enabled.Enables streaming tool call tests for Cohere provider.
tests/core-providers/gemini_test.go (1)
41-41: LGTM! Gemini streaming tool calls enabled.Enables streaming tool call tests for Gemini provider.
tests/core-providers/tests.go (1)
35-35: LGTM! Streaming tool calls test scenario integrated.The
RunToolCallsStreamingTestscenario is properly integrated into both the test execution flow (line 35) and the summary reporting (line 77), providing comprehensive coverage for streaming tool call functionality across providers.Also applies to: 77-77
core/providers/anthropic/anthropic.go (1)
726-735: Anthropic ResponsesStream stateful accumulator and final-chunk handling look solidThe switch to a pooled
AnthropicResponsesStreamState, plusevent.ToBifrostResponsesStream(chunkIndex, streamState)returning multiple responses, is wired correctly:
- State is acquired once per stream and released via
defer, avoiding per-chunk allocation.- You no longer emit
response.created/response.in_progressmanually;ToBifrostResponsesStreamhandles lifecycle events, and you simply propagate the resulting responses.- Per-response
ExtraFields(RequestType,Provider,ModelRequested,ChunkIndex, per-chunkLatency) and optionalRawResponseare set consistently.- On
isLastChunkfor the last response, you attachusage, overrideLatencywith total duration, set the stream-end indicator inctx, send viaProcessAndSendResponse, andreturnto terminate the goroutine cleanly.- Error cases attach
BifrostErrorExtraFieldsand set the end-indicator beforeProcessAndSendBifrostError.This matches the new unified streaming model and should behave correctly across tool-call scenarios.
Also applies to: 746-749, 783-785, 794-804, 806-832
core/providers/cohere/cohere.go (1)
633-637: Cohere ResponsesStream stateful conversion and stream-end handling look consistentThe new
CohereResponsesStreamStateintegration inResponsesStreamlooks correct:
- State is acquired once per stream, seeded with
request.Model, and released viadefer.event.ToBifrostResponsesStream(chunkIndex, streamState)yields multiple responses; you iterate them, setExtraFields(RequestType,Provider,ModelRequested,ChunkIndex, per-chunkLatency), and optionally attachRawResponse.- On
bifrostErr, you enrichExtraFields, set the stream-end indicator inctx, and route viaProcessAndSendBifrostError, then break out of the loop.- On the last response of the last chunk (
isLastChunk && i == len(responses)-1) you ensureresponse.Responseis non-nil, attach usage and total latency, set the end-indicator, send viaProcessAndSendResponse, andreturnfrom the goroutine.This aligns well with the updated Anthropic/Bedrock flows and should produce well-formed lifecycle events for Responses streaming.
Also applies to: 677-687, 689-715, 718-719
core/providers/bedrock/bedrock.go (2)
724-733: Bedrock ChatCompletionStream: error metadata and stream-end signaling look correctIn
ChatCompletionStream:
- On
bifrostErrfromToBifrostChatCompletionStream, you now setBifrostErrorExtraFields(RequestType,Provider,ModelRequested), mark the stream-end indicator inctx, and dispatch viaProcessAndSendBifrostError.- For the synthetic final chunk created by
CreateBifrostChatCompletionChunkResponse, you setModelDeployment, totalLatency, mark the stream-end indicator, and send throughProcessAndSendResponse.This aligns Bedrock with the unified streaming lifecycle and ensures consumers see consistent metadata and a clear terminal event.
Also applies to: 758-763
871-899: Bedrock ResponsesStream: stateful Responses accumulator and EOF finalization path look goodThe updated
ResponsesStreamusesBedrockResponsesStreamStateandFinalizeBedrockStreamcorrectly:
- State is acquired once per stream, seeded with
request.Model, and released viadefer.- For each eventstream message, you convert via
streamEvent.ToBifrostResponsesStream(chunkIndex, streamState)and iterate the resulting responses, settingExtraFields(RequestType,Provider,ModelRequested,ModelDeployment,ChunkIndex, per-chunkLatency) and optionalRawResponse, then sending them viaProcessAndSendResponse.- On conversion errors (
bifrostErr), you enrichExtraFields, set the stream-end indicator inctx, and send viaProcessAndSendBifrostError, then return.- On decoder EOF, you set the stream-end indicator before calling
FinalizeBedrockStream(streamState, chunkIndex, usage), then loop over the returned final responses, populating metadata (including synthetic"{}"RawResponse), and sending them viaProcessAndSendResponse.Sequence numbers inside
FinalizeBedrockStreamare based on thechunkIndexyou pass in, andChunkIndexinExtraFieldsis incremented in lockstep, so ordering is consistent from a client perspective. The overall pattern matches Anthropic/Cohere’s new stateful Responses streaming.Also applies to: 886-913, 939-977
core/providers/bedrock/responses.go (1)
12-29: BedrockResponsesStreamState pooling and reset logic look correctThe introduction of
BedrockResponsesStreamStateplussync.Poolhelpers is structured well:
- All per-stream maps (
ContentIndexToOutputIndex,ToolArgumentBuffers,ItemIDs,ToolCallIDs,ToolCallNames) are allocated once in the pool constructor and cleared inacquireBedrockResponsesStreamState/flush()rather than reallocated per chunk.- Non-map fields (
CurrentOutputIndex,MessageID,Model,CreatedAt, lifecycle flags) are reset both on acquire and inflush()before returning to the pool, so reused state doesn’t leak data across streams.CreatedAtis stamped once at acquire-time, giving a stablecreated_atacross the stream;MessageIDis generated lazily on firstchunk.RoleinToBifrostResponsesStream.This is a good balance between reuse and safety for the Responses streaming accumulator.
Also applies to: 46-85, 95-129
core/schemas/mux.go (1)
3-7: ChatToResponsesStreamState pooling and lifecycle management look robustThe
ChatToResponsesStreamStatedesign and pooling logic are sound:
- All map fields (
ToolArgumentBuffers,ItemIDs,ToolCallNames,ToolCallIndexToID,ToolCallOutputIndices) are allocated once in the pool constructor and cleared inAcquireChatToResponsesStreamState/ReleaseChatToResponsesStreamState, avoiding per-chunk allocations while preventing state bleed between streams.- Scalar fields (
MessageID,Model,CreatedAt, flags,CurrentOutputIndex,SequenceNumber) are reset on acquire and release, so each stream starts from a clean base.CreatedAtis stamped at acquire-time, giving a stablecreated_atacross lifecycle events for a stream.Overall, this gives you a reusable but safe accumulator for Chat→Responses streaming.
Also applies to: 961-999, 1001-1077
tests/core-providers/scenarios/tool_calls_streaming.go (4)
18-42: LGTM! Clean type definitions and constructor.The accumulator types and constructor are well-structured with proper initialization of all maps.
45-93: LGTM! Key lookup logic correctly fixed.The changes from previous reviews have been properly applied: using
key = -1as a not-found sentinel and treatingIndexdirectly without zero-check fallbacks. The accumulation logic is correct.
183-210: LGTM! Clean getter methods.The getter methods correctly convert accumulated state to
ToolCallInfoslices.
213-787: LGTM! Comprehensive test with good retry and validation logic.The test orchestration is well-structured with:
- Retry logic (3 attempts) for handling flaky LLM responses
- Extensive per-chunk logging for debugging streaming behavior
- Proper validation that all tool calls have ID, name, and arguments
- Graceful handling of invalid JSON during streaming (logs warning but doesn't fail prematurely)
The verbose logging is appropriate for a test file debugging streaming behavior.
core/providers/anthropic/responses.go (5)
14-49: LGTM! Well-structured state type with proper pool initialization.The
AnthropicResponsesStreamStatetype and pool are correctly set up with all maps initialized and lifecycle flags properly defaulted.
115-132: LGTM! Clean output index mapping logic.The
getOrCreateOutputIndexmethod correctly maintains stable mapping between content indices and output indices.
446-494: LGTM! OpenAI-style lifecycle events properly implemented.The
MessageStarthandling correctly emitsresponse.createdandresponse.in_progressevents with proper deduplication via state flags. Sequence number incrementation accounts for previously emitted events in the same batch.
496-624: LGTM! ContentBlockStart handling is correct with nil-safe itemID generation.The handling of different content block types is well-structured:
- Computer tool, text, function call, and MCP tool use cases are all properly handled
- Lines 537-541 correctly check
state.MessageID == nilbefore dereferencing (addresses past review)- State tracking (ItemIDs, ToolArgumentBuffers, MCPCallOutputIndices) is properly initialized
- OutputItemAdded events are emitted with appropriate fields
626-861: LGTM! Comprehensive and correct delta, stop, and completion handling.The streaming conversion logic is well-implemented:
- Text, InputJSON, and thinking deltas properly reference state for itemIDs and accumulation
- ContentBlockStop correctly emits
arguments.donefollowed byoutput_item.done- Sequence number incrementation (line 807) properly accounts for events emitted in the current batch
- Computer tool accumulation and final emission is handled correctly
- State cleanup (buffers, tracking maps) happens at appropriate points
- MessageStop emits final
response.completedevent
Merge activity
|
…nts (#815) ## Summary Enhances the Responses API streaming implementation to better align with OpenAI's streaming format, providing more consistent and reliable tool call streaming across providers. ## Changes - Refactored Anthropic's Responses API streaming to use a stateful accumulator for tracking output items - Added similar streaming accumulators to Bedrock and Cohere providers - Implemented proper OpenAI-style lifecycle events (created, in_progress, completed) for all providers - Improved tool call streaming with proper output indexing and argument accumulation - Replaced the chat-to-responses conversion approach with native implementations for each provider - Added comprehensive tests for tool call streaming functionality ## Type of change - [ ] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (Next.js) - [ ] Docs ## How to test Test the Responses API streaming with tool calls across different providers: ```sh # Run the tool calls streaming tests go test ./tests/core-providers -run TestOpenAI/ToolCallsStreamingResponses go test ./tests/core-providers -run TestAnthropic/ToolCallsStreamingResponses go test ./tests/core-providers -run TestBedrock/ToolCallsStreamingResponses go test ./tests/core-providers -run TestCohere/ToolCallsStreamingResponses ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues Improves the reliability of tool call streaming in the Responses API. ## Security considerations No security implications. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
1 participant
Summary
Enhances the Responses API streaming implementation to better align with OpenAI's streaming format, providing more consistent and reliable tool call streaming across providers.
Changes
Type of change
Affected areas
How to test
Test the Responses API streaming with tool calls across different providers:
Breaking changes
Related issues
Improves the reliability of tool call streaming in the Responses API.
Security considerations
No security implications.
Checklist