feat: support raw response accumulation in stream accumulator #999
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds optional RawResponse fields to streaming chunk and accumulated types; records per-chunk ExtraFields.RawResponse for chat/audio/transcription/responses, concatenates them into AccumulatedData.RawResponse on finalization, clears per-chunk RawResponse before pooling, and surfaces accumulated raw payload to logging and changelogs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant StreamProc as Stream Processor
participant Acc as Accumulator
participant Logger
participant DB as Log DB
Client->>StreamProc: Send streaming event (chat/audio/transcription/response)
StreamProc->>Acc: Append chunk (include ExtraFields.RawResponse if present)
Acc-->>StreamProc: Ack stored chunk
alt final chunk received
StreamProc->>Acc: Trigger finalization
Acc->>Acc: Sort chunks by ChunkIndex and concatenate RawResponse ("\n\n")
Acc-->>StreamProc: Return AccumulatedData (RawResponse, messages, usage, cost)
StreamProc->>Logger: Emit final streamResponse with AccumulatedData.RawResponse
Logger->>DB: Persist log entry (includes raw_response)
DB-->>Logger: Persisted
else non-final chunk
StreamProc-->>Logger: Emit delta/partial update (may include per-chunk RawResponse)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/logging/operations.go (1)
163-170: Operator precedence bug: condition may not work as intended.The condition
p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nilis evaluated as(p.disableContentLogging == nil) || ((!*p.disableContentLogging) && (data.RawResponse != nil))due to&&having higher precedence than||.This means when
disableContentLoggingisnil, raw response is logged even ifdata.RawResponseisnil(which would cause a nil dereference insonic.Marshal).- if p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nil { + if (p.disableContentLogging == nil || !*p.disableContentLogging) && data.RawResponse != nil {
🧹 Nitpick comments (5)
framework/streaming/audio.go (1)
94-105: Trailing separator remains in final accumulated response.The accumulation appends
"\n\n"after each chunk, including the last one. This leaves a trailing separator in the finalRawResponse. Consider trimming the trailing separator or using a different accumulation pattern if this is unintentional.// Accumulate raw response if len(accumulator.AudioStreamChunks) > 0 { + var rawParts []string for _, chunk := range accumulator.AudioStreamChunks { if chunk.RawResponse != nil { - if data.RawResponse == nil { - data.RawResponse = bifrost.Ptr(*chunk.RawResponse + "\n\n") - } else { - *data.RawResponse += *chunk.RawResponse + "\n\n" - } + rawParts = append(rawParts, *chunk.RawResponse) } } + if len(rawParts) > 0 { + data.RawResponse = bifrost.Ptr(strings.Join(rawParts, "\n\n")) + } }Note: This would require adding
"strings"to the imports.framework/streaming/transcription.go (1)
106-117: Same trailing separator issue as audio.go.The raw response accumulation follows the same pattern with a trailing
"\n\n"separator. Consider applying the same refactor suggested foraudio.gofor consistency.framework/streaming/responses.go (3)
663-674: Same trailing separator pattern - consider consolidating.This is the same raw response accumulation pattern with trailing
"\n\n"used in audio.go and transcription.go. Consider extracting this into a helper function to avoid code duplication across all streaming types.
708-713: Inconsistent RawResponse extraction between OpenAI and non-OpenAI paths.The OpenAI path uses type assertion and silently ignores non-string values:
rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) }While the non-OpenAI path at lines 809-811 uses
fmt.Sprintf("%v", ...)which converts any type. Consider aligning the approaches for consistency.if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { - rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) - if ok { - chunk.RawResponse = bifrost.Ptr(rawResponse) - } + chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)) }
719-728: Variable shadowing:accumulatorre-declared within same function.The variable
accumulatoris declared at line 691 and shadowed at line 721. While this works correctly, it can be confusing. Consider renaming the inner variable or reusing the outer one since both refer to the same request's accumulator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/changelog.mdui/app/workspace/logs/views/logDetailsSheet.tsxui/app/workspace/logs/views/speechView.tsxframework/streaming/chat.goplugins/logging/operations.goframework/streaming/accumulator.goframework/streaming/audio.goframework/streaming/transcription.goframework/streaming/types.goframework/streaming/responses.goui/app/workspace/logs/views/transcriptionView.tsxtransports/changelog.md
🧬 Code graph analysis (3)
ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
ui/app/workspace/logs/views/transcriptionView.tsx (1)
TranscriptionView(13-160)
framework/streaming/chat.go (1)
core/utils.go (1)
Ptr(51-53)
framework/streaming/audio.go (1)
core/utils.go (1)
Ptr(51-53)
⏰ 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). (8)
- 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 (20)
ui/app/workspace/logs/views/speechView.tsx (1)
52-52: LGTM - UI simplification aligns with PR objectives.The simplified display of speech input directly shows the input text, replacing the previous complex memoized formatting of voice, instructions, and format fields. This aligns with the PR's goal of simplifying input metadata display in the UI.
ui/app/workspace/logs/views/transcriptionView.tsx (1)
30-33: LGTM - Input section simplified appropriately.The transcription input now displays just the audio file label and player, consistent with the PR's UI simplification objectives.
ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
300-304: LGTM - Redundant header removed.The fragment wrapper and "Transcription" header have been removed since
TranscriptionViewalready includes its own section headers internally ("Transcription Input" and "Transcription Output").framework/streaming/types.go (5)
46-46: LGTM - RawResponse field added consistently.The
RawResponse *stringfields have been added uniformly across all streaming data structures to support raw response accumulation. Using pointer types appropriately allows nil values to indicate absence of raw response data.
59-59: LGTM - RawResponse field added consistently.
72-72: LGTM - RawResponse field added consistently.
85-85: LGTM - RawResponse field added consistently.
98-98: LGTM - RawResponse field added consistently.framework/changelog.md (1)
1-1: LGTM - Changelog entry added.The changelog appropriately documents the new raw response accumulation feature.
framework/streaming/chat.go (1)
242-244: LGTM - RawResponse extraction from ExtraFields.The logic correctly extracts and formats the raw response from
ExtraFields.RawResponsewhen present.transports/changelog.md (1)
5-5: LGTM - Changelog entry added.The changelog appropriately documents the new raw response accumulation feature for streaming.
framework/streaming/accumulator.go (4)
46-46: LGTM - RawResponse properly cleared in chunk cleanup.The
RawResponsefield is correctly set to nil when returningChatStreamChunkto the pool, preventing data leakage between reused chunks. This pattern is consistently applied across all chunk types.
64-64: LGTM - RawResponse properly cleared in chunk cleanup.
82-82: LGTM - RawResponse properly cleared in chunk cleanup.
100-100: LGTM - RawResponse properly cleared in chunk cleanup.plugins/logging/operations.go (1)
296-299: LGTM!The raw response handling for streaming is correctly placed within the
disableContentLoggingcheck and properly guards against nil before assignment.framework/streaming/audio.go (1)
133-135: LGTM!The extraction of
RawResponsefromExtraFieldsis correctly guarded with a nil check before converting to string.framework/streaming/transcription.go (1)
138-153: LGTM!Good implementation:
- Deep copy of delta avoids stack memory issues
- Properly separates delta assignment from usage assignment
- RawResponse extraction is correctly guarded
framework/streaming/responses.go (2)
737-784: LGTM!The hybrid approach is correct for OpenAI-compatible providers where the final chunk contains the complete response. The implementation correctly:
- Extracts
RawResponsefrom accumulated chunks- Gets structured response data from the final result
- Handles error status appropriately
- Calculates cost via pricing manager
809-811: Consistent with other streaming types (audio, transcription).The non-OpenAI path uses the same
fmt.Sprintf("%v", ...)pattern as audio.go and transcription.go.
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
6cf3108 to
4b4a584
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
framework/streaming/chat.go (1)
182-197: Avoid trailing separators when accumulatingRawResponseThis concatenates
RawResponsefor every chunk with"\n\n"appended, leaving an extra separator at the end and adding one after the very first chunk as well. Consider only inserting separators between chunks, e.g. initializedata.RawResponsewith the first non-nil chunk without a suffix, and for subsequent chunks prepend"\n\n"before appending:- for _, chunk := range accumulator.ChatStreamChunks { - if chunk.RawResponse != nil { - if data.RawResponse == nil { - data.RawResponse = bifrost.Ptr(*chunk.RawResponse + "\n\n") - } else { - *data.RawResponse += *chunk.RawResponse + "\n\n" - } - } - } + for _, chunk := range accumulator.ChatStreamChunks { + if chunk.RawResponse != nil { + if data.RawResponse == nil { + data.RawResponse = bifrost.Ptr(*chunk.RawResponse) + } else { + *data.RawResponse += "\n\n" + *chunk.RawResponse + } + } + }
🧹 Nitpick comments (5)
ui/app/workspace/logs/views/speechView.tsx (2)
3-3: Remove unuseduseMemoimport.The
useMemohook is no longer used after removing the memoization logic forspeechInput.voice.-import React, { Component, useMemo } from "react"; +import React, { Component } from "react";
45-55: Verify the removal of metadata sections aligns with the raw response feature design.The Speech Input display has been significantly simplified—removing the VOICE, INSTRUCTIONS, FORMAT, and TEXT TO SYNTHESIZE sections—to show only
speechInput.input. Given this PR's focus on raw response accumulation and that it targets a feature branch, this simplification may be intentional. Please confirm this change aligns with the broader stack of PRs and the intended UX for displaying raw streaming responses.Optionally, consider adding a null/empty check or fallback message for robustness:
<div className="space-y-4 p-6"> - <div className="font-mono text-xs">{speechInput.input}</div> + <div className="font-mono text-xs"> + {speechInput.input || <span className="text-gray-400">No input provided</span>} + </div> </div>Based on coding guidelines, reviewing in context of the PR stack.
framework/streaming/audio.go (1)
94-109: NormalizeRawResponseconcatenation to avoid trailing"\n\n"As in the chat accumulator, this logic appends
"\n\n"after every chunk, so the final accumulated string ends with an extra separator. You can avoid that by only inserting separators between chunks:- for _, chunk := range accumulator.AudioStreamChunks { - if chunk.RawResponse != nil { - if data.RawResponse == nil { - data.RawResponse = bifrost.Ptr(*chunk.RawResponse + "\n\n") - } else { - *data.RawResponse += *chunk.RawResponse + "\n\n" - } - } - } + for _, chunk := range accumulator.AudioStreamChunks { + if chunk.RawResponse != nil { + if data.RawResponse == nil { + data.RawResponse = bifrost.Ptr(*chunk.RawResponse) + } else { + *data.RawResponse += "\n\n" + *chunk.RawResponse + } + } + }framework/streaming/transcription.go (1)
106-121: Tidy up transcriptionRawResponseaccumulation separatorsThe accumulator currently adds
"\n\n"after every chunk, leaving a trailing separator. Align with the other suggested accumulators by only inserting separators between chunks:- for _, chunk := range accumulator.TranscriptionStreamChunks { - if chunk.RawResponse != nil { - if data.RawResponse == nil { - data.RawResponse = bifrost.Ptr(*chunk.RawResponse + "\n\n") - } else { - *data.RawResponse += *chunk.RawResponse + "\n\n" - } - } - } + for _, chunk := range accumulator.TranscriptionStreamChunks { + if chunk.RawResponse != nil { + if data.RawResponse == nil { + data.RawResponse = bifrost.Ptr(*chunk.RawResponse) + } else { + *data.RawResponse += "\n\n" + *chunk.RawResponse + } + } + }framework/streaming/responses.go (1)
663-678: ConsistentRawResponseconcatenation without trailing"\n\n"Same as other accumulators, this appends
"\n\n"after every chunk, leaving a trailing separator. Recommend only inserting separators between chunks:- for _, chunk := range accumulator.ResponsesStreamChunks { - if chunk.RawResponse != nil { - if data.RawResponse == nil { - data.RawResponse = bifrost.Ptr(*chunk.RawResponse + "\n\n") - } else { - *data.RawResponse += *chunk.RawResponse + "\n\n" - } - } - } + for _, chunk := range accumulator.ResponsesStreamChunks { + if chunk.RawResponse != nil { + if data.RawResponse == nil { + data.RawResponse = bifrost.Ptr(*chunk.RawResponse) + } else { + *data.RawResponse += "\n\n" + *chunk.RawResponse + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- framework/changelog.md
- plugins/logging/operations.go
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- framework/streaming/accumulator.go
- ui/app/workspace/logs/views/transcriptionView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/streaming/audio.goframework/streaming/chat.goframework/streaming/types.goframework/streaming/transcription.gotransports/changelog.mdframework/streaming/responses.goui/app/workspace/logs/views/speechView.tsx
🧬 Code graph analysis (4)
framework/streaming/audio.go (1)
core/utils.go (1)
Ptr(51-53)
framework/streaming/chat.go (1)
core/utils.go (1)
Ptr(51-53)
framework/streaming/transcription.go (3)
core/utils.go (1)
Ptr(51-53)core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(119-126)plugins/mocker/main.go (1)
Usage(138-142)
framework/streaming/responses.go (4)
core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (2)
Ptr(51-53)IsFinalChunk(198-213)core/schemas/bifrost.go (3)
OpenAI(35-35)OpenRouter(49-49)Azure(36-36)framework/streaming/types.go (1)
AccumulatedData(28-47)
⏰ 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). (5)
- 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 (6)
transports/changelog.md (1)
4-5: Changelog entries look consistent and clearThe new feature bullets are concise and aligned with the streaming/raw-response changes elsewhere in the stack.
framework/streaming/chat.go (1)
246-248: Per-chunkRawResponsecapture wiring looks correctPopulating
chunk.RawResponsefromresult.ChatResponse.ExtraFields.RawResponsekeeps the raw payload attached to each chunk for later accumulation and is consistent with other streaming paths.framework/streaming/audio.go (1)
137-139: Per-chunk audioRawResponseextraction is wired consistentlyReading
ExtraFields.RawResponseintochunk.RawResponsemirrors the chat/transcription handling and enables accumulation; no issues here.framework/streaming/transcription.go (1)
142-149: Delta copy + per-chunk transcriptionRawResponsecapture look soundAlways setting
chunk.Deltafromresult.TranscriptionStreamResponse(not only on final chunks) matches howbuildCompleteMessageFromTranscriptionStreamChunksreconstructsText, and wiringExtraFields.RawResponseintochunk.RawResponsekeeps raw payloads available for accumulation.Also applies to: 155-157
framework/streaming/types.go (1)
46-46:RawResponsefields are added consistently across streaming typesExtending
AccumulatedDataand all stream chunk structs withRawResponse *stringis coherent with existing patterns (pointer fields, optional metadata) and matches how the accumulators now build concatenated raw payloads.Also applies to: 59-59, 72-72, 85-85, 98-98
framework/streaming/responses.go (1)
813-815: Non-OpenAIRawResponsecapture is consistent with other streamsStoring
ExtraFields.RawResponseviafmt.Sprintfintochunk.RawResponsematches the chat/audio/transcription paths and enables accumulation in the shared responses accumulator.
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
4b4a584 to
37ebe6b
Compare
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
facc630 to
0822b25
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)
plugins/logging/operations.go (1)
163-170: Operator precedence bug in condition.The condition
p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nilis parsed as:
p.disableContentLogging == nil || (!*p.disableContentLogging && data.RawResponse != nil)This means when
disableContentLoggingis nil, the raw response is logged regardless of whetherdata.RawResponseis nil, potentially causing issues. Compare with line 248 which uses proper grouping.- if p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nil { + if (p.disableContentLogging == nil || !*p.disableContentLogging) && data.RawResponse != nil {
♻️ Duplicate comments (2)
framework/streaming/responses.go (2)
697-699: Stale timestamps used in final response.
startTimestampandendTimestampare captured at lines 697-699 before callingaddResponsesStreamChunk(line 719), butaddResponsesStreamChunkupdatesaccumulator.StartTimestampandaccumulator.FinalTimestamp. The stale values are then used at lines 749-750, potentially missing the actual stream time span.Consider reading timestamps after
addResponsesStreamChunkreturns:- accumulator := a.getOrCreateStreamAccumulator(requestID) - accumulator.mu.Lock() - startTimestamp := accumulator.StartTimestamp - endTimestamp := accumulator.FinalTimestamp - accumulator.mu.Unlock() ... + accumulator.mu.Lock() + startTimestamp := accumulator.StartTimestamp + endTimestamp := accumulator.FinalTimestamp + accumulator.mu.Unlock() + data := &AccumulatedData{ ... StartTimestamp: startTimestamp, EndTimestamp: endTimestamp,Also applies to: 749-750
706-721: ChunkIndex not set for OpenAI provider chunks.The OpenAI path creates chunks but doesn't set
ChunkIndex(defaults to 0). SinceprocessAccumulatedResponsesStreamingChunkssorts byChunkIndexbefore concatenatingRawResponse, all OpenAI chunks will have undefined order. SetChunkIndexfromresult.ResponsesStreamResponse.ExtraFields.ChunkIndexas done in the non-OpenAI path (line 827).if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) } } + chunk.ChunkIndex = result.ResponsesStreamResponse.ExtraFields.ChunkIndex
🧹 Nitpick comments (5)
ui/app/workspace/logs/views/speechView.tsx (1)
52-53: Preserve whitespace and wrap long speech input for readabilityRight now
speechInput.inputis rendered in a plaindiv, so newlines are collapsed and long lines may overflow. Consider preserving formatting and wrapping long text:- <div className="font-mono text-xs">{speechInput.input}</div> + <div className="font-mono text-xs whitespace-pre-wrap break-words"> + {speechInput.input} + </div>This makes multi-line inputs and long utterances easier to read in the logs.
framework/streaming/audio.go (1)
94-109: Redundant sort operation.The chunks are already sorted by
ChunkIndexat line 15-17 viabuildCompleteMessageFromAudioStreamChunks(called at line 52) before this code executes. The sort at lines 97-99 is redundant for the final chunk path.Consider removing the redundant sort or extracting sorting to a single location:
// Accumulate raw response if len(accumulator.AudioStreamChunks) > 0 { - // Sort chunks by chunk index - sort.Slice(accumulator.AudioStreamChunks, func(i, j int) bool { - return accumulator.AudioStreamChunks[i].ChunkIndex < accumulator.AudioStreamChunks[j].ChunkIndex - }) for _, chunk := range accumulator.AudioStreamChunks {framework/streaming/transcription.go (1)
106-121: Same redundant sort pattern as audio.go.Chunks are already sorted at lines 16-18 via
buildCompleteMessageFromTranscriptionStreamChunks(called at line 59). Consider consolidating the sort logic to avoid redundancy.framework/streaming/responses.go (2)
663-678: Consistent accumulation pattern.Raw response accumulation follows the same pattern as audio and transcription streams. Same optional refactor opportunity for redundant sort applies here.
712-717: Inconsistent RawResponse extraction between provider paths.OpenAI path (lines 712-717) uses type assertion expecting a string, while non-OpenAI path (lines 813-815) uses
fmt.Sprintf("%v", ...). This could lead to different behavior ifRawResponseis not a string type.Consider unifying the extraction logic:
if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { - rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) - if ok { - chunk.RawResponse = bifrost.Ptr(rawResponse) - } + chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)) }Or vice versa, use type assertion consistently in both paths for stricter typing.
Also applies to: 813-815
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
✅ Files skipped from review due to trivial changes (1)
- framework/changelog.md
🚧 Files skipped from review as they are similar to previous changes (5)
- framework/streaming/chat.go
- framework/streaming/accumulator.go
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- transports/changelog.md
- ui/app/workspace/logs/views/transcriptionView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/streaming/audio.goframework/streaming/types.goui/app/workspace/logs/views/speechView.tsxplugins/logging/operations.goframework/streaming/transcription.goframework/streaming/responses.go
🧬 Code graph analysis (2)
framework/streaming/audio.go (1)
core/utils.go (1)
Ptr(51-53)
framework/streaming/transcription.go (2)
core/utils.go (1)
Ptr(51-53)core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(119-126)
⏰ 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). (15)
- 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
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
plugins/logging/operations.go (1)
296-299: LGTM!The raw response handling for streaming log entries is correctly placed within the content logging check block (lines 248-299) and properly checks for nil before dereferencing.
framework/streaming/types.go (1)
46-46: LGTM!The
RawResponse *stringfields are consistently added across all streaming chunk types and the accumulated data struct. Using pointer type is appropriate for optional data.Also applies to: 59-59, 72-72, 85-85, 98-98
framework/streaming/audio.go (1)
137-139: LGTM!Raw response propagation from
ExtraFields.RawResponseto chunk is correctly implemented.framework/streaming/transcription.go (1)
142-157: LGTM!Good improvements:
- Delta deep-copy now applies to all chunks, preventing stack memory reference issues
- RawResponse extraction is consistent with the audio streaming pattern
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
37ebe6b to
cbd9ab9
Compare
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
0822b25 to
05daf39
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
ui/app/workspace/logs/views/transcriptionView.tsx (1)
56-81: Fix operator precedence to avoid rendering raw strings instead of the gridThis expression still has the same issue previously flagged: if
taskorlanguageare non-empty strings, React will render that string directly instead of the 3‑column grid.You want to first check if any of
task,language, ordurationexist, and only then render the grid:- {transcriptionOutput?.task || - transcriptionOutput?.language || - (transcriptionOutput?.duration && ( - <div className="grid grid-cols-3 gap-4"> + {(transcriptionOutput?.task || + transcriptionOutput?.language || + transcriptionOutput?.duration) && ( + <div className="grid grid-cols-3 gap-4"> {transcriptionOutput?.task && ( <div> <div className="text-muted-foreground mb-2 text-xs font-medium">TASK</div> <div className="font-mono text-xs">{transcriptionOutput.task}</div> </div> )} @@ - {transcriptionOutput?.duration && ( + {transcriptionOutput?.duration && ( <div> <div className="text-muted-foreground mb-2 text-xs font-medium">DURATION</div> <div className="font-mono text-xs">{transcriptionOutput.duration.toFixed(1)}s</div> </div> )} - </div> - ))} + </div> + )}This keeps the inner per-field checks as-is while ensuring the grid renders only when at least one of the fields is present.
framework/streaming/chat.go (1)
182-197: Remove trailing separator from RawResponse accumulation.The current logic appends
"\n\n"after every chunk, including the last one, resulting in trailing newlines in the accumulated raw response.Apply this diff to append separators only between chunks:
// Accumulate raw response if len(accumulator.ChatStreamChunks) > 0 { // Sort chunks by chunk index sort.Slice(accumulator.ChatStreamChunks, func(i, j int) bool { return accumulator.ChatStreamChunks[i].ChunkIndex < accumulator.ChatStreamChunks[j].ChunkIndex }) for _, chunk := range accumulator.ChatStreamChunks { if chunk.RawResponse != nil { if data.RawResponse == nil { - data.RawResponse = bifrost.Ptr(*chunk.RawResponse + "\n\n") + data.RawResponse = bifrost.Ptr(*chunk.RawResponse) } else { - *data.RawResponse += *chunk.RawResponse + "\n\n" + *data.RawResponse += "\n\n" + *chunk.RawResponse } } } }framework/streaming/responses.go (1)
706-721: Set ChunkIndex and capture timestamps after accumulation.Two issues in the OpenAI-compatible provider branch:
Missing ChunkIndex assignment: All OpenAI chunks default to index 0, causing undefined raw payload order during sorting at line 667. The non-OpenAI path correctly sets
chunk.ChunkIndexat line 827.Stale timestamps: Lines 695-699 capture timestamps before
addResponsesStreamChunk(line 719) updates the accumulator, then use these stale values at lines 749-750 instead of the actual stream timespan.Apply this diff to fix both issues:
accumulator := a.getOrCreateStreamAccumulator(requestID) - accumulator.mu.Lock() - startTimestamp := accumulator.StartTimestamp - endTimestamp := accumulator.FinalTimestamp - accumulator.mu.Unlock() // For OpenAI-compatible providers, the last chunk already contains the whole accumulated response // so just return it as is // We maintain the accumulator only for raw response accumulation if provider == schemas.OpenAI || provider == schemas.OpenRouter || (provider == schemas.Azure && !schemas.IsAnthropicModel(model)) { isFinalChunk := bifrost.IsFinalChunk(ctx) chunk := a.getResponsesStreamChunk() chunk.Timestamp = time.Now() chunk.ErrorDetails = bifrostErr if bifrostErr != nil { chunk.FinishReason = bifrost.Ptr("error") } else if result != nil && result.ResponsesStreamResponse != nil { + chunk.ChunkIndex = result.ResponsesStreamResponse.ExtraFields.ChunkIndex if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) } } } if addErr := a.addResponsesStreamChunk(requestID, chunk, isFinalChunk); addErr != nil { return nil, fmt.Errorf("failed to add responses stream chunk for request %s: %w", requestID, addErr) } + + accumulator.mu.Lock() + startTimestamp := accumulator.StartTimestamp + endTimestamp := accumulator.FinalTimestamp + accumulator.mu.Unlock() + if isFinalChunk { // ... rest of the logic
🧹 Nitpick comments (1)
ui/app/workspace/logs/views/speechView.tsx (1)
51-53: Consider preserving whitespace/line breaks for speech input displayRight now the plain
<div>will collapse consecutive spaces and newlines inspeechInput.input. If this field can contain multi-line or formatted text (common in logs/prompts), you may want to render it with preserved whitespace for readability, e.g. by switching to a<pre>wrapper or applying awhite-space: pre-wrapstyle so line breaks and indentation are visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- framework/streaming/audio.go
- plugins/logging/operations.go
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- framework/streaming/types.go
- transports/changelog.md
- framework/changelog.md
- framework/streaming/transcription.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/streaming/accumulator.goframework/streaming/chat.goframework/streaming/responses.goui/app/workspace/logs/views/transcriptionView.tsxui/app/workspace/logs/views/speechView.tsx
🧬 Code graph analysis (2)
framework/streaming/chat.go (1)
core/utils.go (1)
Ptr(51-53)
framework/streaming/responses.go (3)
core/schemas/utils.go (2)
Ptr(16-18)IsAnthropicModel(1043-1045)core/utils.go (1)
Ptr(51-53)framework/streaming/types.go (1)
AccumulatedData(28-47)
⏰ 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 (3)
ui/app/workspace/logs/views/transcriptionView.tsx (1)
30-32: Audio input block looks goodAlways showing the AUDIO FILE label with the
AudioPlayerkeeps the UI simple and consistent for the transcription input; no issues from this change.framework/streaming/accumulator.go (1)
46-46: LGTM! Proper pool hygiene for RawResponse fields.The RawResponse field cleanup is correctly implemented across all four chunk pool return functions, preventing stale raw response data from leaking through pooled objects.
Also applies to: 64-64, 82-82, 100-100
framework/streaming/responses.go (1)
813-815: Verify type handling for RawResponse conversion.Using
fmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)has the same type-handling concerns as chat.go line 247. IfExtraFields.RawResponseis already a string, use type assertion; if it's structured data,%vmay not provide appropriate serialization.Consider type assertion for efficiency:
if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { if rawStr, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string); ok { chunk.RawResponse = bifrost.Ptr(rawStr) } else { // Use appropriate serialization if not a string chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)) } }
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 (1)
plugins/logging/operations.go (1)
163-170: Operator precedence bug causes incorrect conditional evaluation.The condition on line 163 has an operator precedence issue. Due to
&&having higher precedence than||, the expression parses as:(p.disableContentLogging == nil) || (!*p.disableContentLogging && data.RawResponse != nil)This means when
disableContentLoggingis nil, the block executes regardless of whetherdata.RawResponseis nil, potentially causing a nil pointer dereference on line 164.Apply this diff to fix the precedence:
- if p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nil { + if (p.disableContentLogging == nil || !*p.disableContentLogging) && data.RawResponse != nil {
♻️ Duplicate comments (3)
framework/streaming/chat.go (1)
182-197: Trailing separator in RawResponse accumulation still present.This issue was flagged in a previous review. The current logic appends
"\n\n"after every chunk, including the last one, resulting in trailing newlines in the accumulated raw response.framework/streaming/responses.go (2)
706-721: OpenAI path still missing ChunkIndex assignment.As flagged in a previous review, the OpenAI/OpenRouter/Azure branch doesn't set
chunk.ChunkIndexfromresult.ResponsesStreamResponse.ExtraFields.ChunkIndex. SinceprocessAccumulatedResponsesStreamingChunkssorts byChunkIndexbefore concatenatingRawResponse, all OpenAI chunks default to index 0, resulting in undefined raw payload order.Add ChunkIndex assignment to align with the non-OpenAI path (line 827):
} else if result != nil && result.ResponsesStreamResponse != nil { if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) } } + chunk.ChunkIndex = result.ResponsesStreamResponse.ExtraFields.ChunkIndex }
745-753: Stale timestamps used in final response.As flagged in a previous review,
startTimestampandendTimestampare captured at lines 697-699 beforeaddResponsesStreamChunkis called, but that method updates the accumulator's timestamps. The final response (lines 749-750) uses these stale values instead of the updated ones from the accumulator.Consider capturing timestamps after
addResponsesStreamChunkor reading from the accumulator after processing:+ accumulator.mu.Lock() + data.StartTimestamp = accumulator.StartTimestamp + data.EndTimestamp = accumulator.FinalTimestamp + accumulator.mu.Unlock() - data.StartTimestamp = startTimestamp - data.EndTimestamp = endTimestamp
🧹 Nitpick comments (1)
plugins/logging/operations.go (1)
296-299: Inconsistent raw response handling between streaming and non-streaming paths.The streaming path stores
RawResponsedirectly as a string, while the non-streaming path inupdateLogEntry(lines 164-168) marshals it to JSON usingsonic.Marshal. This inconsistency could cause issues for consumers expecting a uniform format.Consider aligning the handling, or verify this difference is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- framework/streaming/accumulator.go
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- ui/app/workspace/logs/views/speechView.tsx
- framework/streaming/types.go
- transports/changelog.md
- framework/streaming/audio.go
- ui/app/workspace/logs/views/transcriptionView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/streaming/chat.goframework/streaming/transcription.goframework/streaming/responses.goplugins/logging/operations.goframework/changelog.md
🧬 Code graph analysis (2)
framework/streaming/chat.go (1)
core/utils.go (1)
Ptr(51-53)
framework/streaming/responses.go (4)
core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (2)
Ptr(51-53)IsFinalChunk(198-213)framework/streaming/types.go (3)
AccumulatedData(28-47)ProcessedStreamResponse(116-123)StreamType(10-10)core/schemas/chatcompletions.go (1)
BifrostLLMUsage(812-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). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
framework/changelog.md (1)
1-1: LGTM!The changelog entry accurately describes the feature being added.
framework/streaming/chat.go (1)
246-248: LGTM!The RawResponse capture from ExtraFields is correctly implemented and follows a consistent pattern with other streaming processors.
framework/streaming/transcription.go (1)
142-157: LGTM!The delta handling and RawResponse capture are correctly implemented. The deep copy of the delta ensures no shared memory issues between chunks.
framework/streaming/responses.go (1)
813-815: LGTM!The non-OpenAI path correctly captures RawResponse from ExtraFields and sets ChunkIndex (line 827), maintaining consistency with other streaming processors.
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
cbd9ab9 to
feb7ebe
Compare
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
05daf39 to
6c16eed
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)
plugins/logging/operations.go (1)
163-170: Operator precedence bug in conditional check.The condition
p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nilhas incorrect operator precedence. Due to&&binding tighter than||, this evaluates as:
(p.disableContentLogging == nil) || (!*p.disableContentLogging && data.RawResponse != nil)This means when
disableContentLoggingisnil, the block executes regardless of whetherdata.RawResponseis nil, potentially causing a nil pointer marshal.Apply this diff to fix the precedence:
- if p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nil { + if (p.disableContentLogging == nil || !*p.disableContentLogging) && data.RawResponse != nil { rawResponseBytes, err := sonic.Marshal(data.RawResponse)
♻️ Duplicate comments (4)
framework/streaming/chat.go (1)
246-248: Inconsistent RawResponse type handling.
ExtraFields.RawResponseisinterface{}and can hold non-string types (maps, arrays) from providers like Bedrock and Vertex. Usingfmt.Sprintf("%v", ...)converts structured data to Go syntax strings likemap[key:value]instead of preserving JSON.The
responses.goimplementation uses a safer type assertion pattern. Consider aligning for consistency:- if result.ChatResponse.ExtraFields.RawResponse != nil { - chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ChatResponse.ExtraFields.RawResponse)) - } + if result.ChatResponse.ExtraFields.RawResponse != nil { + if rawStr, ok := result.ChatResponse.ExtraFields.RawResponse.(string); ok { + chunk.RawResponse = bifrost.Ptr(rawStr) + } else { + chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ChatResponse.ExtraFields.RawResponse)) + } + }framework/streaming/transcription.go (1)
155-157: Same type handling inconsistency as chat.go.For consistency with
responses.go, consider using type assertion before falling back tofmt.Sprintf:- if result.TranscriptionStreamResponse.ExtraFields.RawResponse != nil { - chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.TranscriptionStreamResponse.ExtraFields.RawResponse)) - } + if result.TranscriptionStreamResponse.ExtraFields.RawResponse != nil { + if rawStr, ok := result.TranscriptionStreamResponse.ExtraFields.RawResponse.(string); ok { + chunk.RawResponse = bifrost.Ptr(rawStr) + } else { + chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.TranscriptionStreamResponse.ExtraFields.RawResponse)) + } + }framework/streaming/responses.go (2)
695-699: Start/End timestamps for OpenAI path are stale; prefer using accumulator‑derived valuesIn the OpenAI/OpenRouter/Azure branch,
startTimestampandendTimestampare read from the accumulator before the current chunk is added (lines 695–699), but the finalAccumulatedDatafor the response uses those pre‑update values (lines 745–751). SinceaddResponsesStreamChunkandprocessAccumulatedResponsesStreamingChunkscan updateaccumulator.StartTimestamp/FinalTimestamp, the returneddata.StartTimestamp/EndTimestampmay omit the final chunk (and be zero for single‑chunk streams), whileLatencycomes fromresult.GetExtraFields().Latency.For consistency with the non‑OpenAI path and to ensure timestamps cover the full stream, consider deriving
StartTimestamp,EndTimestamp, and possiblyLatencyfrom theAccumulatedDatareturned byprocessAccumulatedResponsesStreamingChunksinstead of the pre‑chunk snapshot.Also applies to: 741-754
663-678: SetChunkIndexfor OpenAI/OpenRouter/Azure responses to ensure deterministic RawResponse ordering
processAccumulatedResponsesStreamingChunkssortsaccumulator.ResponsesStreamChunksbyChunkIndexbefore concatenatingRawResponse(lines 663–678), and the non‑OpenAI path explicitly setschunk.ChunkIndex = result.ResponsesStreamResponse.ExtraFields.ChunkIndex(line 827). In the OpenAI/OpenRouter/Azure (non‑Anthropic) path, the new chunk only setsTimestamp,ErrorDetails, andRawResponse(lines 706–717) and never assignsChunkIndexbefore callingaddResponsesStreamChunk(lines 719–721). As a result, all such chunks keep the defaultChunkIndex(likely zero), so the sort no longer has a meaningful key and the accumulated RawResponse ordering becomes implementation‑dependent rather than matching stream order.To keep ordering deterministic and consistent with the non‑OpenAI branch, assign
chunk.ChunkIndexfromresult.ResponsesStreamResponse.ExtraFields.ChunkIndexin the OpenAI/OpenRouter/Azure path before adding the chunk, mirroring the non‑OpenAI logic.Also applies to: 704-721, 827-827
🧹 Nitpick comments (2)
framework/streaming/responses.go (2)
663-678: RawResponse accumulation behavior is correct; consider more efficient concatenationThe accumulation correctly orders by
ChunkIndexand avoids trailing separators, and the semantics look good. However, repeatedly doing*data.RawResponse += "\n\n" + *chunk.RawResponsecan be quadratic for large numbers of chunks. Consider collecting chunk strings into a slice and usingstrings.Join(or astrings.Builder) once at the end to reduce allocations and copying.
813-815: ConfirmExtraFields.RawResponsetype;fmt.Sprintf("%v", ...)may not be truly “raw”For non‑OpenAI providers,
chunk.RawResponseis set viafmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)(lines 813–815), whereas the OpenAI path assertsRawResponsetostring. IfExtraFields.RawResponsecan be[]byteor a structured value,%vwill yield a debug representation (e.g.,[123 34 ...]or a Go struct dump), which may not match the intended “raw provider payload” semantics and can differ across providers.Please double‑check the concrete type(s) of
ExtraFields.RawResponsefor these transports and consider normalizing to a true raw representation (e.g., casting[]bytetostringor encoding a JSON value explicitly) to keep behavior consistent with the rest of the stack.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
✅ Files skipped from review due to trivial changes (1)
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/streaming/accumulator.go
- framework/streaming/audio.go
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- ui/app/workspace/logs/views/speechView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
plugins/logging/operations.goframework/changelog.mdframework/streaming/responses.goframework/streaming/transcription.goui/app/workspace/logs/views/transcriptionView.tsxframework/streaming/types.goframework/streaming/chat.go
🧬 Code graph analysis (2)
framework/streaming/transcription.go (3)
core/utils.go (1)
Ptr(51-53)core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(119-126)plugins/mocker/main.go (1)
Usage(138-142)
framework/streaming/chat.go (1)
core/utils.go (1)
Ptr(51-53)
⏰ 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). (7)
- 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 (11)
framework/changelog.md (1)
1-1: LGTM!The changelog entry accurately describes the feature being added.
plugins/logging/operations.go (1)
296-299: LGTM!The streaming path correctly handles the accumulated
RawResponsewhich is already a*stringtype from the accumulation logic.ui/app/workspace/logs/views/transcriptionView.tsx (1)
30-32: LGTM!The simplified transcription input section correctly shows the audio file label and player unconditionally when input is present.
framework/streaming/chat.go (1)
182-197: LGTM!The RawResponse accumulation logic correctly sorts chunks by index and concatenates with
"\n\n"separators only between chunks (not trailing).framework/streaming/types.go (5)
46-46: LGTM!The
RawResponse *stringfield addition toAccumulatedDatacorrectly captures the final accumulated raw response.
59-59: LGTM!Consistent field addition across
AudioStreamChunk.
72-72: LGTM!Consistent field addition across
TranscriptionStreamChunk.
85-85: LGTM!Consistent field addition across
ChatStreamChunkwith helpful inline comment.
98-98: LGTM!Consistent field addition across
ResponsesStreamChunk.framework/streaming/transcription.go (2)
106-121: LGTM!The RawResponse accumulation logic correctly sorts chunks and concatenates with separators only between chunks.
142-148: LGTM!Creating a deep copy of the delta struct is appropriate to avoid pointing to stack memory that could be overwritten.
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
6c16eed to
1219509
Compare
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
feb7ebe to
899722a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
framework/streaming/responses.go (2)
695-699: Timestamps captured beforeaddResponsesStreamChunkmay be stale.
startTimestampandendTimestampare read from the accumulator (lines 697-698) before callingaddResponsesStreamChunk(line 719), which updatesaccumulator.StartTimestampandaccumulator.FinalTimestamp. The stale timestamps are then used in the final response (lines 749-750), potentially missing the actual time span of the full stream.Consider capturing timestamps after
addResponsesStreamChunkcompletes:- accumulator := a.getOrCreateStreamAccumulator(requestID) - accumulator.mu.Lock() - startTimestamp := accumulator.StartTimestamp - endTimestamp := accumulator.FinalTimestamp - accumulator.mu.Unlock() - // For OpenAI-compatible providers... if provider == schemas.OpenAI || provider == schemas.OpenRouter || (provider == schemas.Azure && !schemas.IsAnthropicModel(model)) { isFinalChunk := bifrost.IsFinalChunk(ctx) // ... chunk setup and addResponsesStreamChunk ... if addErr := a.addResponsesStreamChunk(requestID, chunk, isFinalChunk); addErr != nil { return nil, fmt.Errorf("failed to add responses stream chunk for request %s: %w", requestID, addErr) } if isFinalChunk { + accumulator := a.getOrCreateStreamAccumulator(requestID) + accumulator.mu.Lock() + startTimestamp := accumulator.StartTimestamp + endTimestamp := accumulator.FinalTimestamp // ... rest of final chunk handling using updated timestamps ...Also applies to: 749-750
706-721: SetChunkIndexfor OpenAI/OpenRouter/Azure chunks to ensure deterministic RawResponse ordering.The chunk is created but
ChunkIndexis never assigned, so all OpenAI chunks default to index 0. SinceprocessAccumulatedResponsesStreamingChunkssorts byChunkIndexbefore concatenatingRawResponse(lines 666-668), this results in undefined ordering for the accumulated raw payload.if bifrostErr != nil { chunk.FinishReason = bifrost.Ptr("error") } else if result != nil && result.ResponsesStreamResponse != nil { + chunk.ChunkIndex = result.ResponsesStreamResponse.ExtraFields.ChunkIndex if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) } } }
🧹 Nitpick comments (2)
framework/streaming/audio.go (1)
137-139: Minor inconsistency: Consider using type assertion like in responses.go.This code uses
fmt.Sprintf("%v", ...)to convertRawResponseto a string, whileresponses.go(lines 713-716) uses a type assertionresult.ResponsesStreamResponse.ExtraFields.RawResponse.(string). Thefmt.Sprintfapproach is safer (won't panic) but may produce different string representations if the underlying value isn't already a string.For consistency across streaming types, consider aligning the approach—either use type assertion with a fallback, or use
fmt.Sprintfin both places.framework/streaming/responses.go (1)
813-815: Inconsistent RawResponse extraction approach within the same file.The non-OpenAI path uses
fmt.Sprintf("%v", ...)while the OpenAI path (lines 713-716) uses a type assertion. For consistency and predictable string representation, consider aligning both approaches:} else if result != nil && result.ResponsesStreamResponse != nil { if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { - chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)) + rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) + if ok { + chunk.RawResponse = bifrost.Ptr(rawResponse) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- framework/streaming/transcription.go
- framework/changelog.md
- ui/app/workspace/logs/views/transcriptionView.tsx
- framework/streaming/types.go
- plugins/logging/operations.go
- framework/streaming/accumulator.go
- ui/app/workspace/logs/views/speechView.tsx
- framework/streaming/chat.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/streaming/audio.goframework/streaming/responses.gotransports/changelog.md
🧬 Code graph analysis (2)
framework/streaming/audio.go (1)
core/utils.go (1)
Ptr(51-53)
framework/streaming/responses.go (4)
core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (1)
Ptr(51-53)core/schemas/bifrost.go (3)
OpenAI(35-35)OpenRouter(49-49)Azure(36-36)framework/streaming/types.go (1)
AccumulatedData(28-47)
⏰ 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). (9)
- 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 (4)
transports/changelog.md (1)
1-1: LGTM!Changelog entry appropriately documents the new raw response accumulation feature for streaming.
framework/streaming/audio.go (1)
94-109: LGTM!The RawResponse accumulation logic correctly:
- Sorts chunks by ChunkIndex to ensure proper ordering
- Concatenates with
"\n\n"separator only between chunks (avoiding trailing separator)- Uses proper nil checks before pointer dereference
framework/streaming/responses.go (2)
663-678: LGTM!The trailing separator issue from past reviews has been correctly addressed. The accumulation now properly prepends
"\n\n"only between chunks, avoiding trailing newlines.
745-754: Good integration of accumulated RawResponse into final data.The final
AccumulatedDatacorrectly inheritsRawResponsefrom the accumulated chunks viaaccumulatedData.RawResponse(line 753), ensuring the raw payload is available in the final response.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
framework/streaming/responses.go (2)
706-718:ChunkIndexnot set for OpenAI/OpenRouter/Azure chunks - ordering may be undefined.The OpenAI path creates a chunk and captures
RawResponse, but does not setchunk.ChunkIndexfromresult.ResponsesStreamResponse.ExtraFields.ChunkIndex. SinceprocessAccumulatedResponsesStreamingChunkssorts chunks byChunkIndex(lines 666-668) before concatenating raw responses, all OpenAI chunks will have the default value (0), resulting in undefined ordering when multiple chunks exist.The non-OpenAI path correctly sets
chunk.ChunkIndexat line 827. Align the OpenAI path for consistency.if bifrostErr != nil { chunk.FinishReason = bifrost.Ptr("error") } else if result != nil && result.ResponsesStreamResponse != nil { + chunk.ChunkIndex = result.ResponsesStreamResponse.ExtraFields.ChunkIndex if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) } } }
745-754: Stale timestamps captured before chunk addition.
startTimestampandendTimestampare captured at lines 697-699 beforeaddResponsesStreamChunkis called at line 719. However,addResponsesStreamChunkupdatesaccumulator.StartTimestampandaccumulator.FinalTimestampbased on the chunk being added. The finalAccumulatedDataat lines 749-750 uses these stale values, potentially missing the actual time span of the stream.Consider capturing timestamps after
addResponsesStreamChunkreturns, or re-reading them from the accumulator before building the final response.if addErr := a.addResponsesStreamChunk(requestID, chunk, isFinalChunk); addErr != nil { return nil, fmt.Errorf("failed to add responses stream chunk for request %s: %w", requestID, addErr) } if isFinalChunk { shouldProcess := false // Get the accumulator to check if processing has already been triggered accumulator := a.getOrCreateStreamAccumulator(requestID) accumulator.mu.Lock() shouldProcess = !accumulator.IsComplete // Mark as complete when we're about to process if shouldProcess { accumulator.IsComplete = true } + // Capture updated timestamps after all chunks have been added + startTimestamp := accumulator.StartTimestamp + endTimestamp := accumulator.FinalTimestamp accumulator.mu.Unlock()Then remove the earlier timestamp capture at lines 697-699, or move it inside the
isFinalChunkblock after the chunk is added.
🧹 Nitpick comments (3)
framework/streaming/transcription.go (2)
142-157: Consider type assertion instead offmt.Sprintffor RawResponse conversion.Lines 155-157 use
fmt.Sprintf("%v", ...)to convertRawResponseto a string. If the underlying type is already a string (as seen in the OpenAI path inresponses.goat lines 713-716 where a type assertion is used), a type assertion would be more explicit and avoid unnecessary formatting overhead.- if result.TranscriptionStreamResponse.ExtraFields.RawResponse != nil { - chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.TranscriptionStreamResponse.ExtraFields.RawResponse)) - } + if result.TranscriptionStreamResponse.ExtraFields.RawResponse != nil { + if rawStr, ok := result.TranscriptionStreamResponse.ExtraFields.RawResponse.(string); ok { + chunk.RawResponse = bifrost.Ptr(rawStr) + } else { + chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.TranscriptionStreamResponse.ExtraFields.RawResponse)) + } + }
109-111: Redundant sort - slice already sorted earlier in the function.The chunks slice is already sorted at line 16-18 during the
buildCompleteMessageFromTranscriptionStreamChunkscall at line 59. This second sort at line 109-110 is redundant for the final chunk path. While the overhead is minimal (O(n) for sorted input), you could remove it or add a comment explaining the defensive intent.framework/streaming/responses.go (1)
813-815: Inconsistent RawResponse conversion - use type assertion like the OpenAI path.The non-OpenAI path uses
fmt.Sprintf("%v", ...)for RawResponse conversion, while the OpenAI path at lines 713-716 correctly uses a type assertion. For consistency and to avoid unnecessary formatting overhead, apply the same pattern here.- if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { - chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)) - } + if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { + if rawStr, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string); ok { + chunk.RawResponse = bifrost.Ptr(rawStr) + } else { + chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- framework/streaming/accumulator.go
- framework/streaming/audio.go
- transports/changelog.md
- framework/streaming/chat.go
- framework/streaming/types.go
- plugins/logging/operations.go
- ui/app/workspace/logs/views/transcriptionView.tsx
- framework/changelog.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/logs/views/logDetailsSheet.tsxframework/streaming/responses.goui/app/workspace/logs/views/speechView.tsxframework/streaming/transcription.go
🧬 Code graph analysis (2)
ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
ui/app/workspace/logs/views/transcriptionView.tsx (1)
TranscriptionView(13-158)
framework/streaming/transcription.go (3)
core/utils.go (1)
Ptr(51-53)core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(119-126)plugins/mocker/main.go (1)
Usage(138-142)
⏰ 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). (14)
- 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
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
300-305: LGTM—Cleaner component rendering.Removing the wrapper fragment simplifies the component hierarchy without affecting functionality. The
TranscriptionViewcomponent already provides its own internal structure and headers.ui/app/workspace/logs/views/speechView.tsx (1)
52-52: Verify that the simplified input display meets logging/debugging requirements.The Speech Input display was significantly simplified from showing multiple fields (voice, instructions, format) to just
speechInput.input. While this aligns with type changes elsewhere in the stack, confirm that users and developers have sufficient visibility into speech requests for debugging purposes.Based on the AI summary noting type changes removed these fields from
SpeechInput.framework/streaming/transcription.go (1)
106-121: LGTM - Trailing separator issue addressed.The accumulation logic now correctly prepends
"\n\n"to subsequent chunks rather than appending it after every chunk, avoiding trailing newlines. The sorting and nil-check logic is sound.framework/streaming/responses.go (1)
663-678: LGTM - Trailing separator issue addressed.The accumulation logic now correctly prepends
"\n\n"to subsequent chunks rather than appending after every chunk, consistent with the fix applied in other streaming files.
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
552146f to
7bec09b
Compare
TejasGhatte
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
7bec09b to
552146f
Compare
TejasGhatte
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
740fad1 to
727dbd8
Compare
TejasGhatte
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
552146f to
8adce3d
Compare
TejasGhatte
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
727dbd8 to
785a11f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
framework/streaming/responses.go (2)
696-699: Capture timestamps after addResponsesStreamChunk to use updated values.The timestamps are captured from the accumulator before calling
addResponsesStreamChunk(lines 696-699), butaddResponsesStreamChunkupdatesaccumulator.StartTimestampandaccumulator.FinalTimestampbased on the chunk being added. The stale timestamps are then used in the final response (lines 749-750), potentially missing the actual time span of the full stream.Move the timestamp capture after
addResponsesStreamChunkor read the updated values from the accumulator after accumulation completes:accumulator := a.getOrCreateStreamAccumulator(requestID) - accumulator.mu.Lock() - startTimestamp := accumulator.StartTimestamp - endTimestamp := accumulator.FinalTimestamp - accumulator.mu.Unlock() // For OpenAI-compatible providers, the last chunk already contains the whole accumulated response // so just return it as is // We maintain the accumulator only for raw response accumulation if provider == schemas.OpenAI || provider == schemas.OpenRouter || (provider == schemas.Azure && !schemas.IsAnthropicModel(model)) { isFinalChunk := bifrost.IsFinalChunk(ctx) chunk := a.getResponsesStreamChunk() chunk.Timestamp = time.Now() chunk.ErrorDetails = bifrostErr if bifrostErr != nil { chunk.FinishReason = bifrost.Ptr("error") } else if result != nil && result.ResponsesStreamResponse != nil { if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) } } } if addErr := a.addResponsesStreamChunk(requestID, chunk, isFinalChunk); addErr != nil { return nil, fmt.Errorf("failed to add responses stream chunk for request %s: %w", requestID, addErr) } + + // Capture updated timestamps after adding chunk + accumulator.mu.Lock() + startTimestamp := accumulator.StartTimestamp + endTimestamp := accumulator.FinalTimestamp + accumulator.mu.Unlock() + if isFinalChunk {Also applies to: 719-721, 749-750
706-721: Set ChunkIndex for OpenAI responses to ensure correct accumulation order.In the OpenAI/OpenRouter/Azure branch,
chunk.ChunkIndexis never assigned, causing all chunks to default to 0. WhenprocessAccumulatedResponsesStreamingChunkssorts by ChunkIndex (line 666-667) before concatenating RawResponse, the undefined order can produce incorrect raw payload sequences.Apply this diff to assign ChunkIndex from ExtraFields, matching the non-OpenAI path (line 827):
} else if result != nil && result.ResponsesStreamResponse != nil { + chunk.ChunkIndex = result.ResponsesStreamResponse.ExtraFields.ChunkIndex if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) } } }
🧹 Nitpick comments (2)
framework/streaming/transcription.go (1)
155-157: Use type assertion instead of fmt.Sprintf for RawResponse formatting.Using
fmt.Sprintf("%v", ...)for RawResponse conversion can produce inconsistent results depending on the underlying type. For complex objects (maps, structs), it produces Go-formatted output rather than JSON, which may not match expectations for "raw" responses.Consider using type assertion with fallback to JSON marshaling, similar to the approach in
framework/streaming/responses.golines 712-717:- if result.TranscriptionStreamResponse.ExtraFields.RawResponse != nil { - chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.TranscriptionStreamResponse.ExtraFields.RawResponse)) - } + if result.TranscriptionStreamResponse.ExtraFields.RawResponse != nil { + if rawResponse, ok := result.TranscriptionStreamResponse.ExtraFields.RawResponse.(string); ok { + chunk.RawResponse = bifrost.Ptr(rawResponse) + } else { + chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.TranscriptionStreamResponse.ExtraFields.RawResponse)) + } + }Alternatively, if RawResponse should always be JSON-serialized when not a string, use
sonic.Marshalas done in the non-streaming logging path.framework/streaming/responses.go (1)
813-815: Use type assertion for RawResponse to ensure consistent formatting.Similar to line 155-157 in
framework/streaming/transcription.go, usingfmt.Sprintf("%v", ...)can produce inconsistent results. When ExtraFields.RawResponse is a complex object, it produces Go-formatted output rather than preserving the raw format.Consider type assertion similar to the OpenAI branch (lines 712-717):
if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { - chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)) + if rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string); ok { + chunk.RawResponse = bifrost.Ptr(rawResponse) + } else { + chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ResponsesStreamResponse.ExtraFields.RawResponse)) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- framework/streaming/chat.go
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- framework/streaming/accumulator.go
- framework/streaming/types.go
- transports/changelog.md
- framework/streaming/audio.go
- ui/app/workspace/logs/views/speechView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
plugins/logging/operations.goframework/streaming/responses.goframework/streaming/transcription.goframework/changelog.mdui/app/workspace/logs/views/transcriptionView.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
plugins/logging/operations.goframework/streaming/responses.goframework/streaming/transcription.go
🧬 Code graph analysis (1)
framework/streaming/transcription.go (2)
core/utils.go (1)
Ptr(51-53)core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(119-126)
⏰ 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). (7)
- 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 (6)
ui/app/workspace/logs/views/transcriptionView.tsx (2)
30-32: LGTM! Clean simplification of Transcription Input.The unconditional rendering of the audio file label and player streamlines the UI and eliminates edge cases. This is a sensible UX improvement.
56-79: LGTM! Conditional rendering is correct.The operator precedence issue from the past review has been properly resolved. The current implementation correctly wraps the boolean check in parentheses and uses
&&to conditionally render the grid, preventing truthy string values from being rendered directly. Each metadata field is appropriately guarded and only displayed when present. The label change to "DETECTED LANGUAGE" is also more semantically accurate.framework/changelog.md (1)
1-1: LGTM!The changelog entry accurately reflects the new feature for raw response accumulation in the stream accumulator.
plugins/logging/operations.go (1)
296-299: LGTM!The raw response handling in streaming updates is correctly implemented. The direct assignment is appropriate since
streamResponse.Data.RawResponseis already a pre-formatted string accumulated from chunks, unlike the non-streaming path where marshaling is required.framework/streaming/transcription.go (1)
106-121: LGTM!The raw response accumulation logic correctly sorts chunks by index and concatenates them with separators only between chunks, avoiding trailing newlines.
framework/streaming/responses.go (1)
663-678: LGTM!The raw response accumulation logic correctly sorts chunks by index and concatenates them with separators only between chunks, avoiding trailing newlines.
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
8adce3d to
f47ea6b
Compare
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
785a11f to
ef2308c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
framework/streaming/responses.go (1)
695-802: Fix OpenAI branch: setChunkIndexand read timestamps afteraddResponsesStreamChunkmutates the accumulator
Withoutchunk.ChunkIndex, RawResponse ordering is undefined after the sort; andstartTimestamp/endTimestampare captured beforeaddResponsesStreamChunkupdates the accumulator, so the final response can report stale timestamps.@@ - accumulator := a.getOrCreateStreamAccumulator(requestID) - accumulator.mu.Lock() - startTimestamp := accumulator.StartTimestamp - endTimestamp := accumulator.FinalTimestamp - accumulator.mu.Unlock() + // NOTE: read Start/Final timestamps only when needed, after addResponsesStreamChunk, + // since addResponsesStreamChunk may update accumulator timestamps. @@ if provider == schemas.OpenAI || provider == schemas.OpenRouter || (provider == schemas.Azure && !schemas.IsAnthropicModel(model)) { isFinalChunk := bifrost.IsFinalChunk(ctx) chunk := a.getResponsesStreamChunk() chunk.Timestamp = time.Now() chunk.ErrorDetails = bifrostErr if bifrostErr != nil { chunk.FinishReason = bifrost.Ptr("error") } else if result != nil && result.ResponsesStreamResponse != nil { + // Ensure deterministic RawResponse ordering during accumulation + chunk.ChunkIndex = result.ResponsesStreamResponse.ExtraFields.ChunkIndex if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) } } } @@ if shouldProcess { accumulatedData, processErr := a.processAccumulatedResponsesStreamingChunks(requestID, bifrostErr, isFinalChunk) @@ if result != nil && result.ResponsesStreamResponse != nil { + // Read updated timestamps after addResponsesStreamChunk updated the accumulator. + acc := a.getOrCreateStreamAccumulator(requestID) + acc.mu.Lock() + startTimestamp := acc.StartTimestamp + endTimestamp := acc.FinalTimestamp + acc.mu.Unlock() + data := &AccumulatedData{ RequestID: requestID, Status: "success", Stream: true, StartTimestamp: startTimestamp, EndTimestamp: endTimestamp, Latency: result.GetExtraFields().Latency, ErrorDetails: bifrostErr, RawResponse: accumulatedData.RawResponse, }
🧹 Nitpick comments (7)
framework/streaming/transcription.go (2)
106-121: RawResponse accumulation is correct, but consider a builder/join to avoid O(n²) concatenation
Current*data.RawResponse += ...in a loop can get expensive with many chunks;[]string+strings.Join(orstrings.Builder) keeps it linear.
155-157: RawResponse serialization via%vmay not preserve “raw” provider payload
IfExtraFields.RawResponseis amap/[]byte/json.RawMessage,%vcan lose original formatting; consider normalizing (e.g., preferstring, else[]byte, elsejson.Marshal) consistently across stream types in this stack.framework/streaming/audio.go (2)
94-109: Same perf concern: repeated string appends in RawResponse accumulation
Recommend[]string+strings.Join/strings.Builderto keep accumulation linear and reduce allocations.
137-139: Ensure RawResponse conversion preserves provider-original bytes/JSON
fmt.Sprintf("%v", ...)can be lossy for structured/raw types; align conversion with other stream processors (preferstring, else[]byte, elsejson.Marshal).framework/streaming/responses.go (2)
663-678: (Optional) Use join/builder for RawResponse accumulation to avoid quadratic concatenation
Same as other streaming paths: repeated+=can be costly with many chunks.
712-718: Unify RawResponse extraction semantics across OpenAI vs non-OpenAI paths
OpenAI branch only stores RawResponse if it’s astring(otherwise it’s silently dropped), while non-OpenAI stringifies with%v; consider a single helper to “best-effort preserve raw” (string → bytes → JSON marshal fallback) across the stack.Also applies to: 813-815
ui/app/workspace/logs/views/speechView.tsx (1)
3-3: Remove unused import.The
useMemoimport is no longer used after the memoization logic was removed.Apply this diff:
-import React, { Component, useMemo } from "react"; +import React, { Component } from "react";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- framework/streaming/types.go
- framework/streaming/chat.go
- transports/changelog.md
- framework/streaming/accumulator.go
- plugins/logging/operations.go
- framework/changelog.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/streaming/transcription.goui/app/workspace/logs/views/transcriptionView.tsxframework/streaming/audio.goframework/streaming/responses.goui/app/workspace/logs/views/speechView.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
framework/streaming/transcription.goframework/streaming/audio.goframework/streaming/responses.go
🧬 Code graph analysis (2)
framework/streaming/transcription.go (3)
core/utils.go (1)
Ptr(51-53)core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(119-126)plugins/mocker/main.go (1)
Usage(138-142)
framework/streaming/audio.go (1)
core/utils.go (1)
Ptr(51-53)
⏰ 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). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
ui/app/workspace/logs/views/speechView.tsx (1)
52-52: The simplified rendering is correct; no changes needed.The streamlined rendering of
{speechInput.input}at line 52 is appropriate. TheSpeechInputinterface inui/lib/types/logs.tscontains only theinput: stringfield, and the code correctly accesses this property. No references tovoice,instructions, orresponse_formatexist in the TypeScript codebase—these fields belong to the Go backend'sSpeechParametersstruct, representing proper architectural separation between input and configuration parameters. The implementation is sound and consistent.ui/app/workspace/logs/views/transcriptionView.tsx (1)
56-79: LGTM! Operator precedence issue correctly resolved.The conditional rendering logic now correctly wraps the boolean check in parentheses, ensuring that
task,language, ordurationvalues won't be rendered directly as strings. The grid only displays when at least one field exists, and each field is individually conditionally rendered inside. The label change to "DETECTED LANGUAGE" also improves clarity.
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)
plugins/logging/operations.go (1)
163-170: Fix boolean precedence:raw_responsecan be stored even whendata.RawResponse == nil.
p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nilis parsed asA || (B && C). IfAis true, you’ll marshal/store even whendata.RawResponseis nil.- if p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nil { + if (p.disableContentLogging == nil || !*p.disableContentLogging) && data.RawResponse != nil { rawResponseBytes, err := sonic.Marshal(data.RawResponse) if err != nil { p.logger.Error("failed to marshal raw response: %v", err) } else { updates["raw_response"] = string(rawResponseBytes) } }
♻️ Duplicate comments (2)
framework/streaming/responses.go (1)
695-701: OpenAI branch: still missingChunkIndexassignment + timestamps captured beforeaddResponsesStreamChunk.
This appears to match an earlier review concern and can break raw-response ordering and final timestamps. Prefer settingchunk.ChunkIndexand reading timestamps after mutation (or reuseaccumulatedDatatimestamps).- accumulator := a.getOrCreateStreamAccumulator(requestID) - accumulator.mu.Lock() - startTimestamp := accumulator.StartTimestamp - endTimestamp := accumulator.FinalTimestamp - accumulator.mu.Unlock() + // NOTE: don't snapshot timestamps here; addResponsesStreamChunk mutates them. // ... if provider == schemas.OpenAI || provider == schemas.OpenRouter || (provider == schemas.Azure && !schemas.IsAnthropicModel(model)) { isFinalChunk := bifrost.IsFinalChunk(ctx) chunk := a.getResponsesStreamChunk() chunk.Timestamp = time.Now() chunk.ErrorDetails = bifrostErr if bifrostErr != nil { chunk.FinishReason = bifrost.Ptr("error") } else if result != nil && result.ResponsesStreamResponse != nil { + chunk.ChunkIndex = result.ResponsesStreamResponse.ExtraFields.ChunkIndex if result.ResponsesStreamResponse.ExtraFields.RawResponse != nil { rawResponse, ok := result.ResponsesStreamResponse.ExtraFields.RawResponse.(string) if ok { chunk.RawResponse = bifrost.Ptr(rawResponse) } } } if addErr := a.addResponsesStreamChunk(requestID, chunk, isFinalChunk); addErr != nil { return nil, fmt.Errorf("failed to add responses stream chunk for request %s: %w", requestID, addErr) } if isFinalChunk { // ... if shouldProcess { accumulatedData, processErr := a.processAccumulatedResponsesStreamingChunks(requestID, bifrostErr, isFinalChunk) // ... if result != nil && result.ResponsesStreamResponse != nil { data := &AccumulatedData{ RequestID: requestID, Status: "success", Stream: true, - StartTimestamp: startTimestamp, - EndTimestamp: endTimestamp, + StartTimestamp: accumulatedData.StartTimestamp, + EndTimestamp: accumulatedData.EndTimestamp, Latency: result.GetExtraFields().Latency, ErrorDetails: bifrostErr, RawResponse: accumulatedData.RawResponse, }Also applies to: 706-721, 745-754
framework/streaming/chat.go (1)
271-273: Avoid lossyfmt.SprintfforExtraFields.RawResponse(string vs structured types).
This matches a previously raised concern and still appears unresolved. Consider keeping strings as-is and JSON-marshaling non-strings to preserve structure.- if result.ChatResponse.ExtraFields.RawResponse != nil { - chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", result.ChatResponse.ExtraFields.RawResponse)) - } + if rr := result.ChatResponse.ExtraFields.RawResponse; rr != nil { + if s, ok := rr.(string); ok { + chunk.RawResponse = bifrost.Ptr(s) + } else { + // Prefer preserving structure (JSON) over Go's `%v` formatting. + // If you don't want a new dependency here, use encoding/json. + b, err := sonic.Marshal(rr) + if err != nil { + chunk.RawResponse = bifrost.Ptr(fmt.Sprintf("%v", rr)) + } else { + chunk.RawResponse = bifrost.Ptr(string(b)) + } + } + }In maximhq/bifrost, what is the declared type of core/schemas.BifrostResponseExtraFields.RawResponse, and are there any providers that set it to non-string (e.g., map/array) today?
🧹 Nitpick comments (2)
framework/streaming/transcription.go (1)
155-157: Prefer preserving structuredRawResponse(if non-string) instead of%vformatting.
Align with a single strategy acrosschat.go/responses.go/audio.goto avoid mixed formats.framework/streaming/types.go (1)
28-47: Public API expansion is fine, but confirm lifecycle expectations (pool reset + exposure).
IfRawResponseis intended for external consumers, consider whetherToBifrostResponse()should also populateresp.<...>.ExtraFields.RawResponse(currently it doesn’t).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
framework/changelog.md(1 hunks)framework/streaming/accumulator.go(4 hunks)framework/streaming/audio.go(2 hunks)framework/streaming/chat.go(2 hunks)framework/streaming/responses.go(3 hunks)framework/streaming/transcription.go(2 hunks)framework/streaming/types.go(5 hunks)plugins/logging/operations.go(1 hunks)transports/changelog.md(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(1 hunks)ui/app/workspace/logs/views/speechView.tsx(1 hunks)ui/app/workspace/logs/views/transcriptionView.tsx(2 hunks)ui/lib/types/logs.ts(0 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/logs.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- transports/changelog.md
- framework/streaming/audio.go
- framework/streaming/accumulator.go
- ui/app/workspace/logs/views/speechView.tsx
- ui/app/workspace/logs/views/transcriptionView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
plugins/logging/operations.goframework/streaming/chat.goframework/streaming/transcription.goframework/streaming/types.goframework/streaming/responses.goframework/changelog.md
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
plugins/logging/operations.goframework/streaming/chat.goframework/streaming/transcription.goframework/streaming/types.goframework/streaming/responses.go
🧬 Code graph analysis (3)
framework/streaming/chat.go (1)
core/utils.go (1)
Ptr(51-53)
framework/streaming/transcription.go (2)
core/utils.go (1)
Ptr(51-53)core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(119-126)
framework/streaming/responses.go (4)
core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (2)
Ptr(51-53)IsFinalChunk(198-213)core/schemas/bifrost.go (1)
OpenAI(35-35)framework/streaming/types.go (2)
AccumulatedData(28-47)ProcessedStreamResponse(116-123)
⏰ 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). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (5)
framework/changelog.md (1)
1-1: Changelog entry matches the feature scope.framework/streaming/chat.go (1)
182-197: RawResponse accumulation logic looks good (no trailing separator).framework/streaming/transcription.go (1)
106-121: RawResponse accumulation logic looks good (no trailing separator).framework/streaming/types.go (1)
49-60: Chunk structs:RawResponse *stringadditions look consistent.
Given potential size/PII, verify you’re not unintentionally sending this over any public API boundary in the Graphite stack.Also applies to: 62-73, 75-86, 88-99
framework/streaming/responses.go (1)
663-678: RawResponse accumulation logic looks good (no trailing separator).
TejasGhatte
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
f47ea6b to
fc91124
Compare
TejasGhatte
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
ef2308c to
d592b74
Compare
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
fc91124 to
f59299a
Compare
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
d592b74 to
4c3f30d
Compare
TejasGhatte
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
4c3f30d to
b584c84
Compare
TejasGhatte
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
f59299a to
86357e6
Compare
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
86357e6 to
5f77971
Compare
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
b584c84 to
0ea65ac
Compare
TejasGhatte
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
0ea65ac to
e3cbb41
Compare
TejasGhatte
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
5f77971 to
921fdd8
Compare
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
921fdd8 to
5f77971
Compare
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
e3cbb41 to
0ea65ac
Compare
Merge activity
|
Pratham-Mishra04
changed the base branch from
12-01-feat_added_reasoning_support_to_chat_completions
to
graphite-base/999
Pratham-Mishra04
force-pushed
the
12-04-feat_raw_response_accumulation_for_streaming
branch
from
5f77971 to
aff9d5e
Compare
Pratham-Mishra04
deleted the
12-04-feat_raw_response_accumulation_for_streaming
branch
Summary
Added support for raw response accumulation in the stream accumulator, allowing the system to collect and preserve raw API responses throughout streaming operations.
Changes
RawResponsefield to all stream chunk types (Chat, Audio, Transcription, Responses)Type of change
Affected areas
How to test
Test streaming operations with various providers and verify that raw responses are properly accumulated:
Breaking changes
Related issues
This feature enables better debugging and transparency by preserving the original provider responses throughout the streaming process.
Security considerations
Raw responses may contain sensitive information depending on the provider. The system should handle this data with appropriate security measures.
Checklist