feat: add ResponsesMessage support to logging #573
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds end-to-end Responses support: new Responses types and UI, streaming accumulation and chunk processing, logstore serialization for ChatOutput/ResponsesOutput, latency-based logging updates, provider mappings, transcription model validation, and pricing for responses streams. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Transport
participant Acc as Accumulator
participant Logger
participant Store as Logstore
participant UI
Note over Client,Store: High-level Responses streaming flow
Client->>Transport: Start request
Transport->>Transport: Validate model string (provider/model)
Transport->>Acc: Emit responses stream chunks (deltas)
Acc->>Acc: Accumulate ResponsesStreamChunk (text/refusal/func-args)
alt non-final delta
Acc-->>Logger: ProcessedStreamResponse (delta, latency)
Logger->>Store: updateStreamingLogEntry(requestID, latency, partial fields)
else final chunk
Acc->>Acc: Build complete []ResponsesMessage
Acc-->>Logger: ProcessedStreamResponse (final, latency, usage, cost)
Logger->>Store: updateStreamingLogEntry(requestID, latency, final fields)
end
UI->>Store: Fetch LogEntry
Store-->>UI: LogEntry (includes `responses_output?`, `chat_output?`)
alt responses_output present
UI->>UI: Render LogResponsesOutputView(messages)
else
UI->>UI: Render LogChatMessageView(message)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Pratham-Mishra04
changed the title
Pratham-Mishra04
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
0ff5903 to
5745bcb
Compare
Pratham-Mishra04
force-pushed
the
10-07-feat_sync_pools_added_to_all_internal_structs
branch
2 times, most recently
from
290c61b to
e284b38
Compare
Pratham-Mishra04
force-pushed
the
10-07-feat_responses_added_to_logs
branch
2 times, most recently
from
acc6812 to
0e61bee
Compare
Pratham-Mishra04
force-pushed
the
10-07-feat_sync_pools_added_to_all_internal_structs
branch
from
e284b38 to
0ed8c8e
Compare
Pratham-Mishra04
changed the base branch from
10-07-feat_sync_pools_added_to_all_internal_structs
to
graphite-base/573
Pratham-Mishra04
force-pushed
the
graphite-base/573
branch
from
0ed8c8e to
c511c26
Compare
Pratham-Mishra04
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
0e61bee to
a3d5933
Compare
Pratham-Mishra04
changed the base branch from
graphite-base/573
to
08-30-feat_core_extended
Pratham-Mishra04
force-pushed
the
08-30-feat_core_extended
branch
from
c511c26 to
ebd668c
Compare
Pratham-Mishra04
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
a3d5933 to
d598768
Compare
Pratham-Mishra04
force-pushed
the
08-30-feat_core_extended
branch
from
ebd668c to
555c4fc
Compare
Pratham-Mishra04
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
d598768 to
4e99000
Compare
akshaydeo
changed the base branch from
08-30-feat_core_extended
to
graphite-base/573
akshaydeo
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
4e99000 to
b838fe1
Compare
akshaydeo
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
b838fe1 to
cdff4df
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
framework/logstore/tables.go(5 hunks)plugins/logging/main.go(10 hunks)plugins/logging/operations.go(4 hunks)plugins/logging/pool.go(2 hunks)plugins/logging/utils.go(2 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)transports/bifrost-http/handlers/server.go(0 hunks)ui/app/logs/views/logChatMessageView.tsx(1 hunks)ui/app/logs/views/logDetailsSheet.tsx(4 hunks)ui/app/logs/views/logMessageView.tsx(0 hunks)ui/app/logs/views/logResponsesOutputView.tsx(1 hunks)ui/lib/types/logs.ts(3 hunks)
💤 Files with no reviewable changes (2)
- ui/app/logs/views/logMessageView.tsx
- transports/bifrost-http/handlers/server.go
🧰 Additional context used
🧬 Code graph analysis (8)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/handlers/utils.go (1)
SendError(25-34)
plugins/logging/utils.go (4)
plugins/logging/main.go (1)
LoggerPlugin(86-100)core/schemas/bifrost.go (13)
RequestType(93-93)TextCompletionRequest(96-96)TextCompletionStreamRequest(97-97)ChatCompletionRequest(98-98)ChatCompletionStreamRequest(99-99)ResponsesRequest(100-100)ResponsesStreamRequest(101-101)EmbeddingRequest(102-102)SpeechRequest(103-103)SpeechStreamRequest(104-104)TranscriptionRequest(105-105)TranscriptionStreamRequest(106-106)BifrostRequest(18-30)core/schemas/mux.go (1)
ToChatMessages(424-616)core/schemas/chatcompletions.go (3)
ChatMessageRoleUser(195-195)ChatContentBlock(280-287)ChatContentBlockTypeText(272-272)
ui/app/logs/views/logDetailsSheet.tsx (2)
ui/app/logs/views/logChatMessageView.tsx (1)
LogChatMessageView(146-270)ui/app/logs/views/logResponsesOutputView.tsx (1)
LogResponsesOutputView(361-371)
ui/app/logs/views/logResponsesOutputView.tsx (2)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesMessageContentBlock(344-379)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
ui/app/logs/views/logChatMessageView.tsx (2)
ui/lib/types/logs.ts (2)
ChatMessage(113-121)ContentBlock(98-109)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
plugins/logging/main.go (5)
ui/lib/types/logs.ts (4)
ChatMessage(113-121)ResponsesMessage(405-420)BifrostEmbedding(123-127)BifrostError(218-224)core/schemas/chatcompletions.go (1)
ChatMessage(202-211)core/schemas/responses.go (2)
ResponsesMessage(279-291)ResponsesResponse(85-113)core/schemas/bifrost.go (3)
BifrostEmbedding(364-368)BifrostError(536-545)BifrostNonStreamResponseChoice(433-436)framework/streaming/types.go (1)
StreamResponseTypeFinal(23-23)
framework/logstore/tables.go (2)
ui/lib/types/logs.ts (3)
ResponsesMessage(405-420)BifrostEmbedding(123-127)ResponsesReasoning(399-402)core/schemas/bifrost.go (1)
BifrostEmbedding(364-368)
plugins/logging/operations.go (4)
plugins/logging/main.go (2)
LoggerPlugin(86-100)UpdateLogData(42-55)core/schemas/bifrost.go (1)
BifrostCacheDebug(503-517)framework/streaming/types.go (1)
ProcessedStreamResponse(96-103)framework/logstore/tables.go (2)
Log(70-120)Log(123-125)
🔇 Additional comments (2)
ui/app/logs/views/logChatMessageView.tsx (1)
200-225: Great handling of mixed chat content.Nice upgrade: the component cleanly handles both raw string payloads and structured content blocks, so tool-rich responses render without losing information.
ui/app/logs/views/logResponsesOutputView.tsx (1)
230-258: Reasoning summary UX looks solid.Appreciate the readable path for pure
summary_textreasoning while still falling back to JSON for mixed payloads—makes the Responses output far easier to scan.
TejasGhatte
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
cdff4df to
37f55d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/logging/utils.go (1)
56-86: Fix retry loop compilation error.The retry loop uses
for attempt := range maxRetries, which doesn't compile in Go—you cannot range over an int. This blocks the build.Apply this diff:
- for attempt := range maxRetries { + for attempt := 0; attempt < maxRetries; attempt++ {
🧹 Nitpick comments (2)
ui/app/logs/views/logChatMessageView.tsx (1)
129-144: Consider using or removing the unusedgetRoleColorhelper.The
getRoleColorfunction is defined but never used in the component. Line 150 renders the role title without applying the color styling that this function provides.If role-based coloring is intended, apply the color class at line 150:
- <span className={`inline-block rounded text-sm font-medium`}>{getRoleTitle(message.role)}</span> + <span className={`inline-block rounded px-2 py-1 text-sm font-medium ${getRoleColor(message.role)}`}>{getRoleTitle(message.role)}</span>Otherwise, remove the unused helper to reduce clutter.
ui/app/logs/views/logResponsesOutputView.tsx (1)
8-27: Extract duplicated helpers to a shared utility module.The
isJsonandcleanJsonhelpers are duplicated between this file andlogChatMessageView.tsx. Extracting them to a shared utility module (e.g.,utils.tsorjsonHelpers.ts) would eliminate duplication and improve maintainability.Create a new file
ui/app/logs/views/jsonHelpers.ts:export const isJson = (text: string) => { try { JSON.parse(text); return true; } catch { return false; } }; export const cleanJson = (text: unknown) => { try { if (typeof text === "string") return JSON.parse(text); if (Array.isArray(text)) return text; if (text !== null && typeof text === "object") return text; if (typeof text === "number" || typeof text === "boolean") return text; return "Invalid payload"; } catch { return text; } };Then import in both files:
+import { isJson, cleanJson } from "./jsonHelpers"; -const isJson = (text: string) => { ... }; -const cleanJson = (text: unknown) => { ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
framework/logstore/tables.go(5 hunks)plugins/logging/main.go(10 hunks)plugins/logging/operations.go(4 hunks)plugins/logging/pool.go(2 hunks)plugins/logging/utils.go(2 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)transports/bifrost-http/handlers/server.go(0 hunks)ui/app/logs/views/logChatMessageView.tsx(1 hunks)ui/app/logs/views/logDetailsSheet.tsx(4 hunks)ui/app/logs/views/logMessageView.tsx(0 hunks)ui/app/logs/views/logResponsesOutputView.tsx(1 hunks)ui/lib/types/logs.ts(3 hunks)
💤 Files with no reviewable changes (2)
- ui/app/logs/views/logMessageView.tsx
- transports/bifrost-http/handlers/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/app/logs/views/logDetailsSheet.tsx
- transports/bifrost-http/handlers/inference.go
🧰 Additional context used
🧬 Code graph analysis (6)
ui/app/logs/views/logChatMessageView.tsx (2)
ui/lib/types/logs.ts (2)
ChatMessage(113-121)ContentBlock(98-109)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
ui/app/logs/views/logResponsesOutputView.tsx (2)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesMessageContentBlock(344-379)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
plugins/logging/operations.go (4)
plugins/logging/main.go (2)
LoggerPlugin(86-100)UpdateLogData(42-55)core/schemas/bifrost.go (1)
BifrostCacheDebug(510-524)framework/streaming/types.go (1)
ProcessedStreamResponse(96-103)framework/logstore/tables.go (2)
Log(70-120)Log(123-125)
plugins/logging/utils.go (4)
plugins/logging/main.go (1)
LoggerPlugin(86-100)core/schemas/bifrost.go (13)
RequestType(100-100)TextCompletionRequest(103-103)TextCompletionStreamRequest(104-104)ChatCompletionRequest(105-105)ChatCompletionStreamRequest(106-106)ResponsesRequest(107-107)ResponsesStreamRequest(108-108)EmbeddingRequest(109-109)SpeechRequest(110-110)SpeechStreamRequest(111-111)TranscriptionRequest(112-112)TranscriptionStreamRequest(113-113)BifrostRequest(25-37)core/schemas/mux.go (1)
ToChatMessages(424-616)core/schemas/chatcompletions.go (3)
ChatMessageRoleUser(195-195)ChatContentBlock(280-287)ChatContentBlockTypeText(272-272)
plugins/logging/main.go (4)
ui/lib/types/logs.ts (4)
ChatMessage(113-121)ResponsesMessage(405-420)BifrostEmbedding(123-127)BifrostError(218-224)core/schemas/chatcompletions.go (1)
ChatMessage(202-211)core/schemas/responses.go (2)
ResponsesMessage(279-291)ResponsesResponse(85-113)core/schemas/bifrost.go (3)
BifrostEmbedding(371-375)BifrostError(558-567)BifrostNonStreamResponseChoice(440-443)
framework/logstore/tables.go (3)
ui/lib/types/logs.ts (3)
ResponsesMessage(405-420)BifrostEmbedding(123-127)ResponsesReasoning(399-402)core/schemas/bifrost.go (1)
BifrostEmbedding(371-375)core/schemas/chatcompletions.go (2)
ChatTool(58-62)ChatAssistantMessageToolCall(339-343)
🔇 Additional comments (1)
plugins/logging/main.go (1)
478-485: LGTM! Latency persistence issue resolved.The previous concern about not persisting latency has been addressed. Line 484 now correctly passes
logMsg.LatencytoupdateLogEntry, ensuring the measured latency is properly stored in the database.
TejasGhatte
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
c49532b to
058cd51
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/logging/main.go (1)
323-351: Return pooled LogMessage in non-async paths (leak).logMsg from the pool isn't returned on (a) error path and (b) streaming non-final updates, causing pool growth and memory retention.
Apply these diffs:
Error path release (before return):
if processingErr != nil { p.logger.Error("failed to process log update for request %s: %v", logMsg.RequestID, processingErr) } else { ... } - - return result, bifrostErr, nil + // Release pooled message when not using async goroutine + p.putLogMessage(logMsg) + return result, bifrostErr, nilStreaming path release when not final:
- if streamResponse != nil && streamResponse.Type == streaming.StreamResponseTypeFinal { + if streamResponse != nil && streamResponse.Type == streaming.StreamResponseTypeFinal { ... - } + } else { + // No async worker -> release pooled message + p.putLogMessage(logMsg) + }Also applies to: 360-384
♻️ Duplicate comments (4)
ui/app/logs/views/logChatMessageView.tsx (1)
12-12: Replace all underscores in type labelUse a global replace so all underscores become spaces.
-<div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium capitalize">{block.type.replace("_", " ")}</div> +<div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium capitalize">{block.type.replace(/_/g, " ")}</div>plugins/logging/main.go (1)
396-399: Persist the served model, not the requested one.You guard on result.Model but persist ExtraFields.ModelRequested, so logs show the requested model instead of the actual served model.
Apply this diff:
- if result.Model != "" { - updateData.Model = result.ExtraFields.ModelRequested - } + if result.Model != "" { + updateData.Model = result.Model + }framework/streaming/responses.go (2)
86-91: Ordering bug: keys assumed contiguous; sort indices.messageMap iteration using 0..len(map)-1 drops/shuffles outputs when indices have gaps/out-of-order.
Apply this diff (and add import):
@@ - // Convert map to ordered slice - for i := 0; i < len(messageMap); i++ { - if msg, exists := messageMap[i]; exists { - completeMessages = append(completeMessages, *msg) - } - } + // Convert map to ordered slice by sorted OutputIndex + keys := make([]int, 0, len(messageMap)) + for k := range messageMap { + keys = append(keys, k) + } + sort.Ints(keys) + for _, k := range keys { + if msg, ok := messageMap[k]; ok { + completeMessages = append(completeMessages, *msg) + } + }And at imports:
-import ( +import ( "context" "fmt" + "sort" "time"
275-337: Remove OpenAI special-case; also fixes a compile error.provider (schemas.ModelProvider) is compared to "openai" (string), which won’t compile; and the final-chunk assumption is incorrect. Treat all providers via accumulation.
Apply this diff:
- // For OpenAI provider, the last chunk already contains the whole accumulated response - // so just return it as is - if provider == "openai" { - isFinalChunk := bifrost.IsFinalChunk(ctx) - if isFinalChunk { - ... - return &ProcessedStreamResponse{ ... }, nil - } - // For non-final chunks from OpenAI, just pass through - return &ProcessedStreamResponse{ - Type: StreamResponseTypeDelta, - RequestID: requestID, - StreamType: StreamTypeResponses, - Provider: provider, - Model: model, - Data: nil, - }, nil - } + // All providers use the accumulation logic; no special-casing neededAnd adjust the comment below:
- // For non-OpenAI providers, use the accumulation logic + // Use the accumulation logic
🧹 Nitpick comments (12)
plugins/logging/pool.go (1)
33-46: Simplify/robustify pool reset (zero the struct)Instead of manually clearing fields (easy to miss new ones), zero the struct before putting it back.
func (p *LoggerPlugin) putUpdateLogData(data *UpdateLogData) { - // Reset all fields to avoid memory leaks - data.Status = "" - data.TokenUsage = nil - data.ChatOutput = nil - data.ResponsesOutput = nil - data.ErrorDetails = nil - data.Model = "" - data.Object = "" - data.SpeechOutput = nil - data.TranscriptionOutput = nil - data.EmbeddingOutput = nil - data.Cost = nil + // Reset all fields to avoid memory leaks (future-proof) + *data = UpdateLogData{} p.updateDataPool.Put(data) }ui/lib/utils/validation.ts (2)
467-475: Avoid duplicate JSON-validators; alias to existing isValidJSONThis duplicates isValidJSON above. Alias instead to keep behavior in one place.
-export const isJson = (text: string) => { - try { - JSON.parse(text); - return true; - } catch { - return false; - } -}; +export const isJson = (text: string) => isValidJSON(text);
477-487: cleanJson is fine; consider keeping behavior centralizedNo issues. If you later extend parsing rules, keep it alongside isValidJSON for consistency.
ui/app/logs/views/logDetailsSheet.tsx (4)
119-121: Preserve zero token counts (use ?? not ||)Current
|| "-"hides 0 as “-”. Use nullish coalescing.-<LogEntryDetailsView className="w-full" label="Prompt Tokens" value={log.token_usage?.prompt_tokens || "-"} /> -<LogEntryDetailsView className="w-full" label="Completion Tokens" value={log.token_usage?.completion_tokens || "-"} /> -<LogEntryDetailsView className="w-full" label="Total Tokens" value={log.token_usage?.total_tokens || "-"} /> +<LogEntryDetailsView className="w-full" label="Prompt Tokens" value={log.token_usage?.prompt_tokens ?? "-"} /> +<LogEntryDetailsView className="w-full" label="Completion Tokens" value={log.token_usage?.completion_tokens ?? "-"} /> +<LogEntryDetailsView className="w-full" label="Total Tokens" value={log.token_usage?.total_tokens ?? "-"} />
161-170: Also preserve zeros in cache debug fields
thresholdcan be 0;|| "-"will hide it. Prefer??.-<LogEntryDetailsView className="w-full" label="Threshold" value={log.cache_debug.threshold || "-"} /> +<LogEntryDetailsView className="w-full" label="Threshold" value={log.cache_debug.threshold ?? "-"} />
261-276: Avoid double “Response” sections (prefer responses_output when present)If both output_message and responses_output exist, you render both. Prefer responses_output; fall back to output_message otherwise.
-{log.output_message && !log.error_details?.error.message && ( - <> - <div className="mt-4 flex w-full items-center gap-2"> - <div className="text-sm font-medium">Response</div> - </div> - <LogChatMessageView message={log.output_message} /> - </> -)} -{log.responses_output && log.responses_output.length > 0 && !log.error_details?.error.message && ( +{log.responses_output && log.responses_output.length > 0 && !log.error_details?.error.message ? ( <> <div className="mt-4 w-full text-left text-sm font-medium">Response</div> <LogResponsesOutputView messages={log.responses_output} /> </> -)} +) +: (log.output_message && !log.error_details?.error.message && ( + <> + <div className="mt-4 flex w-full items-center gap-2"> + <div className="text-sm font-medium">Response</div> + </div> + <LogChatMessageView message={log.output_message} /> + </> +))}
35-40: Remove unused variable toolChoiceComputed but never used.
-let toolChoice = null; -if (log.params?.tool_choice) { - try { - toolChoice = JSON.stringify(log.params.tool_choice, null, 2); - } catch (ignored) {} -} +// tool_choice present but not displayed; remove until neededframework/streaming/types.go (1)
167-173: Remove redundant ExtraFields assignmentYou set CacheDebug, then immediately overwrite ExtraFields. Keep a single assignment.
- if p.Data.CacheDebug != nil { - resp.ExtraFields.CacheDebug = p.Data.CacheDebug - } resp.ExtraFields = schemas.BifrostResponseExtraFields{ CacheDebug: p.Data.CacheDebug, Provider: p.Provider, }ui/app/logs/views/logResponsesOutputView.tsx (1)
33-33: Always show block header for consistencyCurrently hidden when text exists; show it to keep sections uniform.
-{!block.text && <div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium">{getBlockTitle(block.type)}</div>} +<div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium">{getBlockTitle(block.type)}</div>framework/streaming/accumulator.go (1)
346-357: Use helper put-methods to zero fields before returning to pools.Direct pool.Put skips zeroing and can retain large references. Call the helper methods instead.
Apply this diff:
- for _, chunk := range accumulator.ChatStreamChunks { - a.chatStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.ChatStreamChunks { + a.putChatStreamChunk(chunk) + } - for _, chunk := range accumulator.ResponsesStreamChunks { - a.responsesStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.ResponsesStreamChunks { + a.putResponsesStreamChunk(chunk) + } - for _, chunk := range accumulator.TranscriptionStreamChunks { - a.transcriptionStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.TranscriptionStreamChunks { + a.putTranscriptionStreamChunk(chunk) + } - for _, chunk := range accumulator.AudioStreamChunks { - a.audioStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.AudioStreamChunks { + a.putAudioStreamChunk(chunk) + }framework/streaming/responses.go (1)
176-182: Avoid nested defer; clean up after unlock directly.Current nested defer is confusing and contrary to the comment. Unlock first, then cleanup when final.
Apply this diff:
- defer func() { - accumulator.mu.Unlock() - if isFinalChunk { - // Before unlocking, we cleanup - defer a.cleanupStreamAccumulator(requestID) - } - }() + defer func() { + accumulator.mu.Unlock() + if isFinalChunk { + a.cleanupStreamAccumulator(requestID) + } + }()ui/lib/types/logs.ts (1)
305-544: Consider strengthening type safety and adding documentation.The comprehensive Responses API type definitions are well-structured, but several areas could benefit from improved type safety:
Index signatures with
any(lines 390, 419, 491):[key: string]: anydisables type checking for additional properties. Consider using more specific types or union types for known properties, reserving index signatures for truly dynamic data.
Record<string, any>usage (lines 510, 433): This weakens type safety. If the structure is known, define explicit interfaces. If it's genuinely dynamic, document why.Missing documentation: Consider adding JSDoc comments for complex types like
ResponsesMessage,ResponsesMessageContentBlock, andResponsesToolto explain their purpose, usage patterns, and when different optional fields are present.Type discriminants:
ResponsesMessagehas many optional fields whose presence depends on thetypefield. Consider using discriminated unions to ensure type safety:type ResponsesMessage = | { type: "message"; role: ResponsesMessageRoleType; content: ResponsesMessageContent; /* ... */ } | { type: "function_call"; call_id: string; name: string; arguments: string; /* ... */ } | { type: "reasoning"; summary: ResponsesReasoningContent[]; /* ... */ } // ... other variantsThis would enable better type narrowing and catch missing required fields at compile time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
framework/logstore/tables.go(5 hunks)framework/streaming/accumulator.go(10 hunks)framework/streaming/responses.go(1 hunks)framework/streaming/types.go(4 hunks)plugins/logging/main.go(11 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/pool.go(2 hunks)plugins/logging/utils.go(2 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)ui/app/logs/views/logChatMessageView.tsx(1 hunks)ui/app/logs/views/logDetailsSheet.tsx(4 hunks)ui/app/logs/views/logMessageView.tsx(0 hunks)ui/app/logs/views/logResponsesOutputView.tsx(1 hunks)ui/lib/constants/logs.ts(2 hunks)ui/lib/types/logs.ts(3 hunks)ui/lib/utils/validation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ui/app/logs/views/logMessageView.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/logstore/tables.go
- plugins/logging/utils.go
- transports/bifrost-http/handlers/inference.go
🧰 Additional context used
🧬 Code graph analysis (8)
ui/app/logs/views/logDetailsSheet.tsx (2)
ui/app/logs/views/logChatMessageView.tsx (1)
LogChatMessageView(65-181)ui/app/logs/views/logResponsesOutputView.tsx (1)
LogResponsesOutputView(337-347)
ui/app/logs/views/logChatMessageView.tsx (3)
ui/lib/types/logs.ts (2)
ChatMessage(113-121)ContentBlock(98-109)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
plugins/logging/main.go (5)
ui/lib/types/logs.ts (4)
ChatMessage(113-121)ResponsesMessage(405-420)BifrostEmbedding(123-127)BifrostError(218-224)core/schemas/chatcompletions.go (2)
ChatMessage(202-211)BifrostNonStreamResponseChoice(370-373)core/schemas/responses.go (2)
ResponsesMessage(280-292)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostError(409-418)RequestType(99-99)framework/streaming/types.go (1)
StreamResponseTypeFinal(24-24)
framework/streaming/responses.go (5)
framework/streaming/accumulator.go (1)
Accumulator(15-31)framework/streaming/types.go (7)
ResponsesStreamChunk(84-92)AccumulatedData(28-47)ProcessedStreamResponse(110-117)StreamResponseTypeFinal(24-24)StreamType(10-10)StreamTypeResponses(17-17)StreamResponseTypeDelta(23-23)core/schemas/responses.go (11)
ResponsesStreamResponseTypeOutputItemAdded(1630-1630)ResponsesStreamResponseTypeContentPartAdded(1633-1633)ResponsesStreamResponseTypeOutputTextDelta(1637-1637)ResponsesStreamResponseTypeRefusalDelta(1640-1640)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)ResponsesOutputMessageContentTypeText(624-624)ResponsesOutputMessageContentText(664-667)ResponsesOutputMessageContentTypeRefusal(625-625)ResponsesOutputMessageContentRefusal(689-691)ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostResponse(238-254)BifrostContextKeyRequestID(120-120)core/utils.go (2)
GetRequestFields(145-151)IsFinalChunk(127-142)
framework/streaming/accumulator.go (2)
framework/streaming/types.go (3)
ResponsesStreamChunk(84-92)StreamAccumulator(95-107)ChatStreamChunk(73-81)core/schemas/bifrost.go (1)
ResponsesStreamRequest(107-107)
ui/app/logs/views/logResponsesOutputView.tsx (3)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesMessageContentBlock(344-379)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
framework/streaming/types.go (2)
core/schemas/responses.go (2)
ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (3)
LLMUsage(296-302)BifrostCacheDebug(361-375)BifrostError(409-418)
plugins/logging/operations.go (4)
plugins/logging/main.go (2)
LoggerPlugin(86-100)UpdateLogData(42-55)core/schemas/bifrost.go (1)
BifrostCacheDebug(361-375)framework/streaming/types.go (1)
ProcessedStreamResponse(110-117)framework/logstore/tables.go (2)
Log(70-120)Log(123-125)
🔇 Additional comments (12)
framework/streaming/types.go (5)
17-18: Add responses stream type — looks goodEnum extension is consistent with existing types.
37-39: AccumulatedData: OutputMessages addition — LGTMName and placement align with responses model.
83-92: ResponsesStreamChunk addition — LGTMFields mirror other chunk structs.
99-101: Accumulator: ResponsesStreamChunks — LGTMConsistent with other chunk lists.
156-161: Populate ResponsesResponse — LGTMCorrectly maps OutputMessages to Bifrost response.
ui/app/logs/views/logResponsesOutputView.tsx (1)
1-346: Overall component — well structuredGood coverage of content types, safe JSON handling, and masking of file_data.
Ensure the ResponsesMessage type in ui/lib/types/logs.ts matches all cases rendered here (e.g., custom_tool_call[_output], mcp_call) to avoid runtime undefined titles.
plugins/logging/operations.go (1)
244-251: Responses output serialization path: LGTM.Writing ResponsesOutput via tempEntry.ResponsesOutputParsed -> SerializeFields -> updates["responses_output"] is correct and consistent with logstore schema.
Also applies to: 66-73
framework/streaming/accumulator.go (1)
441-446: No changes needed: modules use Go 1.24, which supportsfor range 1000.plugins/logging/main.go (1)
316-322: Latency threading validated and retry helper confirmed
retryOnNotFound is implemented in plugins/logging/utils.go:56.ui/lib/constants/logs.ts (1)
26-52: Sync legacy and new request types in filters and server queries. Filters (Type dropdown) now include both “response.” and “responses_”; confirm your UI store/endpoints that send therequest_typeparameter also handle both formats until the DB migration is complete.ui/lib/types/logs.ts (2)
250-250: LGTM!The addition of
responses_output?: ResponsesMessage[]appropriately extends the LogEntry interface to support the Responses API output format.
546-550: LGTM!The WebSocketLogMessage updates appropriately add the
operationfield and use the fullLogEntryas the payload, which aligns with the feature requirements.
TejasGhatte
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
058cd51 to
2bf3a1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/bifrost-http/handlers/inference.go (1)
699-704: Fix partial file reads; use io.ReadAllmultipart.File.Read is not guaranteed to fill the buffer. Use io.ReadAll to avoid truncated uploads.
- // Read file data - fileData := make([]byte, fileHeader.Size) - if _, err := file.Read(fileData); err != nil { - SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to read uploaded file: %v", err), h.logger) - return - } + // Read file data + fileData, err := io.ReadAll(file) + if err != nil { + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to read uploaded file: %v", err), h.logger) + return + }framework/streaming/accumulator.go (1)
346-357: Cleanup should zero fields before returning chunks to pools.Direct pool.Put skips zeroing, retaining references and increasing memory retention.
- for _, chunk := range accumulator.ChatStreamChunks { - a.chatStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.ChatStreamChunks { + a.putChatStreamChunk(chunk) + } - for _, chunk := range accumulator.ResponsesStreamChunks { - a.responsesStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.ResponsesStreamChunks { + a.putResponsesStreamChunk(chunk) + } - for _, chunk := range accumulator.TranscriptionStreamChunks { - a.transcriptionStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.TranscriptionStreamChunks { + a.putTranscriptionStreamChunk(chunk) + } - for _, chunk := range accumulator.AudioStreamChunks { - a.audioStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.AudioStreamChunks { + a.putAudioStreamChunk(chunk) + }
♻️ Duplicate comments (9)
plugins/logging/utils.go (1)
61-83: Range-over-int requires Go ≥1.22; confirm toolchainThe loop is fine in Go 1.22+; earlier Go versions won’t compile this.
Please confirm CI/go.mod uses Go ≥1.22. If compatibility is needed, switch to a classic loop:
- for attempt := range maxRetries { + for attempt := 0; attempt < maxRetries; attempt++ {plugins/logging/pool.go (1)
20-23: Clear StreamResponse and fix outdated commentAvoid retaining references when returning to pool; update comment to reflect actual fields.
- // Don't reset UpdateData and StreamUpdateData here since they're returned - // to their own pools in the defer function - just clear the pointers - msg.UpdateData = nil + // Don't reset UpdateData buffers here (they're returned to their own pool). + // Just clear pointers on the pooled message. + msg.UpdateData = nil + msg.StreamResponse = nilui/app/logs/views/logChatMessageView.tsx (1)
12-12: Global underscore replacement needed (repeat from prior review).Use a global replace so all underscores are converted to spaces.
- <div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium capitalize">{block.type.replace("_", " ")}</div> + <div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium capitalize">{block.type.replace(/_/g, " ")}</div>framework/streaming/responses.go (3)
35-53: Content parts must be placed by index, not appended.The code appends content parts without respecting
ContentIndex, which can reorder parts when chunks arrive out of order. This issue was previously flagged and remains unresolved.Apply the previously suggested diff to fix this ordering issue by using
ContentIndexto place parts at the correct position instead of appending.
86-91: Ordering logic can drop or shuffle outputs.The loop assumes
messageMaphas contiguous keys from 0 to N-1, but if the provider skips indices or sends them out of order, this will miss entries or reorder them. This issue was previously flagged and remains unresolved.Collect all keys from
messageMap, sort them numerically, then iterate the sorted keys to preserve the exact sequence provided by the stream.
277-336: OpenAI final-chunk handling may be incorrect.The code assumes the final chunk contains the complete response at
result.ResponsesStreamResponse.Response.ResponsesResponse.Output, but according to a previous review, OpenAI's final chunks typically only contain usage with empty choices. The message content must be assembled from earlier chunks. This critical issue was previously flagged and remains unresolved.Assemble the response from accumulated chunks rather than extracting it from the final chunk, and merge the final chunk's usage/cost info into the assembled data.
plugins/logging/main.go (1)
395-396: Wrong model persisted (requested vs. served).The code checks
result.Modelbut persistsExtraFields.ModelRequested, causing logs to show the requested model instead of the actual served model. This is inconsistent with the streaming path. This issue was previously flagged and remains unresolved.Apply this diff:
if result.Model != "" { - updateData.Model = result.ExtraFields.ModelRequested + updateData.Model = result.Model }plugins/logging/operations.go (1)
44-45: Latency type mismatch with DB column.The
latencyparameter isint64, but the DB columnLog.Latencyis*float64. Assigningint64directly without conversion can cause driver/type issues. This was previously flagged and remains unresolved.Apply this diff:
if latency != 0 { - updates["latency"] = latency + updates["latency"] = float64(latency) }Also apply similar conversions at lines 163 and 174 where
streamResponse.Data.Latencyis assigned.ui/lib/types/logs.ts (1)
119-119: Clarify or remove misleading tool_calls comment.The comment states "tool calls are now in the content," but neither
ContentBlocknorResponsesMessageContentBlockincludes a field for tool calls. This was previously flagged and remains misleading.Either remove the comment or update it to clarify that
tool_callsis kept for backward compatibility but is not surfaced in the content structures.
🧹 Nitpick comments (8)
plugins/logging/utils.go (2)
88-92: Guard against nil BifrostRequest to avoid panicAdd a quick nil check to make this helper safe.
func (p *LoggerPlugin) extractInputHistory(request *schemas.BifrostRequest) []schemas.ChatMessage { + if request == nil { + return []schemas.ChatMessage{} + }
104-109: Preserve prompt boundaries when concatenating PromptArrayCurrent concat omits separators; join with newline (or space) for readability.
- var stringBuilder strings.Builder - for _, prompt := range request.TextCompletionRequest.Input.PromptArray { - stringBuilder.WriteString(prompt) - } - text = stringBuilder.String() + text = strings.Join(request.TextCompletionRequest. + Input.PromptArray, "\n")transports/bifrost-http/handlers/inference.go (2)
598-607: Remove duplicate initialization of SpeechParametersThis block is repeated immediately below. Keep only one.
- // Extract extra params - if req.SpeechParameters == nil { - req.SpeechParameters = &schemas.SpeechParameters{} - }
44-62: Use “prompt” (not “text”) in known fields for text completionYour TextRequest uses
prompt; keeping “text” markspromptas extra.var textParamsKnownFields = map[string]bool{ "model": true, - "text": true, + "prompt": true, "fallbacks": true, "best_of": true, "echo": true,ui/lib/constants/logs.ts (2)
95-95: Label “list” as “Embedding” for consistencyLegacy “list” represents embeddings; align the label to reduce confusion.
- list: "List", + list: "Embedding",
26-52: Add a RequestType type and enforce map keysStrengthen type-safety to catch missing/extra request-type keys.
// TODO: Remove dependency on old request types (object field in bifrost) by applying migrations to the database export const RequestTypes = [ @@ ] as const; + +export type RequestType = (typeof RequestTypes)[number];Apply outside selected ranges:
// Enforce labels/colors to cover all RequestTypes export const RequestTypeLabels: Record<RequestType, string> = { /* current entries */ }; export const RequestTypeColors: Record<RequestType, string> = { /* current entries */ };framework/logstore/tables.go (2)
79-79: Comment incorrect: field stores a slice, not a single message.Adjust the comment to reflect JSON serialized []schemas.ResponsesMessage.
- ResponsesOutput string `gorm:"type:text" json:"-"` // JSON serialized *schemas.ResponsesMessage + ResponsesOutput string `gorm:"type:text" json:"-"` // JSON serialized []schemas.ResponsesMessage
418-441: Verify reasoning field name for ResponsesMessage in summary extraction.Code uses msg.ResponsesReasoning.Summary; confirm the actual struct field (likely msg.Reasoning.Summary). If mismatched, reasoning text won’t be indexed.
If the field is Reasoning, apply:
- if msg.ResponsesReasoning != nil { - for _, summary := range msg.ResponsesReasoning.Summary { + if msg.Reasoning != nil { + for _, summary := range msg.Reasoning.Summary { parts = append(parts, summary.Text) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
framework/logstore/tables.go(5 hunks)framework/pricing/main.go(1 hunks)framework/streaming/accumulator.go(10 hunks)framework/streaming/responses.go(1 hunks)framework/streaming/types.go(4 hunks)plugins/logging/main.go(11 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/pool.go(2 hunks)plugins/logging/utils.go(2 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)ui/app/logs/views/logChatMessageView.tsx(1 hunks)ui/app/logs/views/logDetailsSheet.tsx(4 hunks)ui/app/logs/views/logMessageView.tsx(0 hunks)ui/app/logs/views/logResponsesOutputView.tsx(1 hunks)ui/lib/constants/logs.ts(2 hunks)ui/lib/types/logs.ts(3 hunks)ui/lib/utils/validation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ui/app/logs/views/logMessageView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/lib/utils/validation.ts
- ui/app/logs/views/logDetailsSheet.tsx
🧰 Additional context used
🧬 Code graph analysis (11)
framework/pricing/main.go (2)
core/schemas/responses.go (2)
ResponsesStreamResponse(1689-1718)ResponsesExtendedResponseUsage(229-234)core/schemas/bifrost.go (1)
LLMUsage(296-302)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/handlers/utils.go (1)
SendError(25-34)
ui/app/logs/views/logResponsesOutputView.tsx (3)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesMessageContentBlock(344-379)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
ui/app/logs/views/logChatMessageView.tsx (3)
ui/lib/types/logs.ts (2)
ChatMessage(113-121)ContentBlock(98-109)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
framework/streaming/responses.go (4)
framework/streaming/accumulator.go (1)
Accumulator(15-31)framework/streaming/types.go (7)
ResponsesStreamChunk(84-92)AccumulatedData(28-47)ProcessedStreamResponse(110-117)StreamResponseTypeFinal(24-24)StreamType(10-10)StreamTypeResponses(17-17)StreamResponseTypeDelta(23-23)core/schemas/bifrost.go (2)
BifrostResponse(238-254)BifrostContextKeyRequestID(120-120)core/utils.go (2)
GetRequestFields(145-151)IsFinalChunk(127-142)
plugins/logging/utils.go (4)
plugins/logging/main.go (1)
LoggerPlugin(86-100)core/schemas/bifrost.go (4)
BifrostRequest(24-36)ResponsesRequest(106-106)TextCompletionRequest(102-102)EmbeddingRequest(108-108)core/schemas/mux.go (1)
ToChatMessages(424-616)core/schemas/chatcompletions.go (3)
ChatMessageRoleUser(195-195)ChatContentBlock(280-287)ChatContentBlockTypeText(272-272)
plugins/logging/operations.go (4)
plugins/logging/main.go (1)
UpdateLogData(42-55)core/schemas/bifrost.go (1)
BifrostCacheDebug(361-375)framework/streaming/types.go (1)
ProcessedStreamResponse(110-117)framework/logstore/tables.go (2)
Log(70-120)Log(123-125)
framework/logstore/tables.go (2)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesReasoning(399-402)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(339-343)
framework/streaming/accumulator.go (2)
framework/streaming/types.go (3)
ResponsesStreamChunk(84-92)StreamAccumulator(95-107)ChatStreamChunk(73-81)core/schemas/bifrost.go (1)
ResponsesStreamRequest(107-107)
framework/streaming/types.go (3)
ui/lib/types/logs.ts (3)
ResponsesMessage(405-420)LLMUsage(189-195)BifrostError(218-224)core/schemas/responses.go (2)
ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (3)
LLMUsage(296-302)BifrostCacheDebug(361-375)BifrostError(409-418)
plugins/logging/main.go (5)
ui/lib/types/logs.ts (4)
ChatMessage(113-121)ResponsesMessage(405-420)BifrostEmbedding(123-127)BifrostError(218-224)core/schemas/chatcompletions.go (2)
ChatMessage(202-211)BifrostNonStreamResponseChoice(370-373)core/schemas/responses.go (2)
ResponsesMessage(280-292)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostError(409-418)RequestType(99-99)framework/streaming/types.go (1)
StreamResponseTypeFinal(24-24)
🔇 Additional comments (16)
transports/bifrost-http/handlers/inference.go (1)
670-674: Good validation for provider/model formatReturning 400 on invalid “provider/model” is correct.
framework/pricing/main.go (1)
169-174: Correctly maps Responses stream usage for pricingUsing ResponsesExtendedResponseUsage + TotalTokens enables accurate cost calc for streaming Responses.
Please confirm non-stream Responses are already covered via
result.Usageor a similar branch.plugins/logging/pool.go (1)
33-46: LCGT on updated fields resetResetting ChatOutput/ResponsesOutput/Cost/EmbeddingOutput is correct.
ui/app/logs/views/logResponsesOutputView.tsx (1)
337-347: UI renderer looks solid.Clear, sectioned rendering for Responses messages; good JSON handling and safe display of file data.
framework/streaming/responses.go (4)
97-124: LGTM!The method correctly initializes content blocks and ensures proper capacity before appending text deltas.
127-169: LGTM!Both refusal and function arguments delta handling follow the same safe pattern of ensuring proper initialization before appending deltas.
172-261: LGTM!The accumulation logic properly manages locking, latency calculation, and distinguishes between delta and final chunks.
339-427: LGTM!The non-OpenAI streaming path correctly uses the accumulation logic with proper chunk pooling, final chunk detection, and duplicate processing prevention via the
IsCompleteflag.plugins/logging/main.go (4)
42-55: LGTM!The
UpdateLogDatastructure correctly reflects the new field naming (ChatOutput,ResponsesOutput) and aligns with the Responses API additions.
316-320: LGTM!Latency is now properly extracted from
result.ExtraFields.Latencywith a safe default when the result is nil.
406-423: LGTM!The output handling correctly maps to
ChatOutputandResponsesOutputfields, properly supporting both chat and responses API outputs.
475-476: LGTM!The latency value is now correctly passed to
updateLogEntryinstead of being hard-coded to 0. This resolves a previously flagged issue.plugins/logging/operations.go (2)
56-82: LGTM!The output serialization correctly handles the renamed fields (
ChatOutput,ResponsesOutput,EmbeddingOutput) with proper error handling.
243-251: LGTM!The streaming path correctly maps
OutputMessagestoResponsesOutputwith proper serialization and error handling.ui/lib/types/logs.ts (2)
242-250: LGTM!The addition of
responses_output?: ResponsesMessage[]properly aligns with the backend changes and supports the new Responses API output format.
305-544: LGTM!The Responses API type definitions are comprehensive and well-structured, covering message types, content blocks, tools, usage, and error structures. These align well with the backend schema additions.
Pratham-Mishra04
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
2bf3a1d to
fff112b
Compare
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)
framework/streaming/accumulator.go (1)
346-357: Use zeroing put helpers in Cleanup to avoid stale chunk data leaks.*Directly
Put-ing without resetting fields can leak prior chunk state into new requests.- for _, chunk := range accumulator.ChatStreamChunks { - a.chatStreamChunkPool.Put(chunk) - } - for _, chunk := range accumulator.ResponsesStreamChunks { - a.responsesStreamChunkPool.Put(chunk) - } - for _, chunk := range accumulator.TranscriptionStreamChunks { - a.transcriptionStreamChunkPool.Put(chunk) - } - for _, chunk := range accumulator.AudioStreamChunks { - a.audioStreamChunkPool.Put(chunk) - } + for _, chunk := range accumulator.ChatStreamChunks { + a.putChatStreamChunk(chunk) + } + for _, chunk := range accumulator.ResponsesStreamChunks { + a.putResponsesStreamChunk(chunk) + } + for _, chunk := range accumulator.TranscriptionStreamChunks { + a.putTranscriptionStreamChunk(chunk) + } + for _, chunk := range accumulator.AudioStreamChunks { + a.putAudioStreamChunk(chunk) + }
♻️ Duplicate comments (3)
framework/streaming/responses.go (3)
86-91: Don’t assume contiguous indices; sort keys before assembling.Using
len(messageMap)assumes 0..N-1 keys and can drop/shuffle outputs. Gather keys and sort.- for i := 0; i < len(messageMap); i++ { - if msg, exists := messageMap[i]; exists { - completeMessages = append(completeMessages, *msg) - } - } + indices := make([]int, 0, len(messageMap)) + for idx := range messageMap { + indices = append(indices, idx) + } + sort.Ints(indices) + for _, i := range indices { + if msg, exists := messageMap[i]; exists { + completeMessages = append(completeMessages, *msg) + } + }Note: add
import "sort".
35-53: Place parts by content_index; appending can reorder.If parts arrive out-of-order, append breaks intended order. Use
content_index.- if messageMap[*streamResp.OutputIndex].Content.ContentBlocks == nil { - messageMap[*streamResp.OutputIndex].Content.ContentBlocks = make([]schemas.ResponsesMessageContentBlock, 0) - } - messageMap[*streamResp.OutputIndex].Content.ContentBlocks = append( - messageMap[*streamResp.OutputIndex].Content.ContentBlocks, - *streamResp.Part, - ) + blocks := messageMap[*streamResp.OutputIndex].Content.ContentBlocks + for len(blocks) <= *streamResp.ContentIndex { + blocks = append(blocks, schemas.ResponsesMessageContentBlock{}) + } + blocks[*streamResp.ContentIndex] = *streamResp.Part + messageMap[*streamResp.OutputIndex].Content.ContentBlocks = blocks
275-337: Remove OpenAI final-chunk special-casing; accumulate like others.Final OpenAI chunks often carry usage-only with empty content; assuming they contain the full output is incorrect. Use the same accumulation path and merge final usage/cost.
- // For OpenAI provider, the last chunk already contains the whole accumulated response - // so just return it as is - if provider == "openai" { - isFinalChunk := bifrost.IsFinalChunk(ctx) - if isFinalChunk { - // ... builds Data from final chunk only ... - return &ProcessedStreamResponse{ Type: StreamResponseTypeFinal, RequestID: requestID, StreamType: StreamTypeResponses, Provider: provider, Model: model, Data: data }, nil - } - // For non-final chunks from OpenAI, just pass through - return &ProcessedStreamResponse{ Type: StreamResponseTypeDelta, RequestID: requestID, StreamType: StreamTypeResponses, Provider: provider, Model: model, Data: nil }, nil - } + // Treat OpenAI like other providers: accumulate all chunks; final chunk typically contains usage.
🧹 Nitpick comments (5)
ui/app/logs/views/logResponsesOutputView.tsx (2)
346-347: Prefer stable keys (use message.id when present).Indexes as keys can cause unnecessary re-renders and mismatches. Use
message.id || index.- return <div className="space-y-4">{messages.map((message, index) => renderMessage(message, index))}</div>; + return <div className="space-y-4">{messages.map((message, index) => renderMessage(message, message.id ?? index))}</div>;
305-321: Deduplicate the exclusion list for “Additional Fields”.The hard-coded list appears twice; extract a shared constant to prevent drift and simplify maintenance.
framework/streaming/responses.go (1)
176-182: Avoid nested defer; call cleanup directly after unlock.Nested
deferinside a deferred func is confusing with no benefit. Simplify.- defer func() { - accumulator.mu.Unlock() - if isFinalChunk { - // Before unlocking, we cleanup - defer a.cleanupStreamAccumulator(requestID) - } - }() + defer func() { + accumulator.mu.Unlock() + if isFinalChunk { + a.cleanupStreamAccumulator(requestID) + } + }()ui/lib/types/logs.ts (2)
385-391: Consider strengthening type safety for extensible interfaces.The
[key: string]: anyindex signature inResponsesToolMessage(line 390) andResponsesMessage(line 419) allows arbitrary properties without type checking, reducing TypeScript's safety guarantees. If tool-specific or message-specific properties are known, prefer explicit optional fields or a discriminated union keyed bytype.If the property set is truly open-ended and documented by the Responses API specification, consider adding a JSDoc comment explaining why the index signature is necessary and what constraints apply to the dynamic properties.
Also applies to: 405-420
484-492: Consider strengthening type safety for extensible tool interface.The
[key: string]: anyindex signature (line 491) permits arbitrary properties without type checking. If the Responses API specification defines specific tool configurations, prefer explicit optional fields or discriminated unions keyed bytype.If the property set is truly open-ended, add a JSDoc comment explaining the extensibility requirement and constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
framework/logstore/tables.go(5 hunks)framework/pricing/main.go(1 hunks)framework/streaming/accumulator.go(10 hunks)framework/streaming/responses.go(1 hunks)framework/streaming/types.go(4 hunks)plugins/logging/main.go(11 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/pool.go(2 hunks)plugins/logging/utils.go(2 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)ui/app/logs/views/logChatMessageView.tsx(1 hunks)ui/app/logs/views/logDetailsSheet.tsx(4 hunks)ui/app/logs/views/logMessageView.tsx(0 hunks)ui/app/logs/views/logResponsesOutputView.tsx(1 hunks)ui/lib/constants/logs.ts(2 hunks)ui/lib/types/logs.ts(3 hunks)ui/lib/utils/validation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ui/app/logs/views/logMessageView.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- framework/pricing/main.go
- transports/bifrost-http/handlers/inference.go
- framework/logstore/tables.go
- framework/streaming/types.go
- plugins/logging/pool.go
🧰 Additional context used
🧬 Code graph analysis (8)
framework/streaming/responses.go (5)
framework/streaming/accumulator.go (1)
Accumulator(15-31)framework/streaming/types.go (4)
ResponsesStreamChunk(84-92)AccumulatedData(28-47)ProcessedStreamResponse(110-117)StreamType(10-10)core/schemas/responses.go (11)
ResponsesStreamResponseTypeOutputItemAdded(1630-1630)ResponsesStreamResponseTypeContentPartAdded(1633-1633)ResponsesStreamResponseTypeOutputTextDelta(1637-1637)ResponsesStreamResponseTypeRefusalDelta(1640-1640)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)ResponsesOutputMessageContentTypeText(624-624)ResponsesOutputMessageContentText(664-667)ResponsesOutputMessageContentTypeRefusal(625-625)ResponsesOutputMessageContentRefusal(689-691)ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostResponse(238-254)BifrostContextKeyRequestID(120-120)core/utils.go (2)
GetRequestFields(145-151)IsFinalChunk(127-142)
ui/app/logs/views/logDetailsSheet.tsx (2)
ui/app/logs/views/logChatMessageView.tsx (1)
LogChatMessageView(65-181)ui/app/logs/views/logResponsesOutputView.tsx (1)
LogResponsesOutputView(337-347)
framework/streaming/accumulator.go (2)
framework/streaming/types.go (3)
ResponsesStreamChunk(84-92)StreamAccumulator(95-107)ChatStreamChunk(73-81)core/schemas/bifrost.go (1)
ResponsesStreamRequest(107-107)
plugins/logging/main.go (5)
ui/lib/types/logs.ts (4)
ChatMessage(113-121)ResponsesMessage(405-420)BifrostEmbedding(123-127)BifrostError(218-224)core/schemas/chatcompletions.go (2)
ChatMessage(202-211)BifrostNonStreamResponseChoice(370-373)core/schemas/responses.go (2)
ResponsesMessage(280-292)ResponsesResponse(85-114)core/schemas/bifrost.go (1)
BifrostError(409-418)framework/streaming/types.go (1)
StreamResponseTypeFinal(24-24)
plugins/logging/operations.go (4)
plugins/logging/main.go (2)
LoggerPlugin(86-100)UpdateLogData(42-55)core/schemas/bifrost.go (1)
BifrostCacheDebug(361-375)framework/streaming/types.go (1)
ProcessedStreamResponse(110-117)framework/logstore/tables.go (2)
Log(70-120)Log(123-125)
ui/app/logs/views/logChatMessageView.tsx (3)
ui/lib/types/logs.ts (2)
ChatMessage(113-121)ContentBlock(98-109)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
ui/app/logs/views/logResponsesOutputView.tsx (3)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesMessageContentBlock(344-379)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
plugins/logging/utils.go (4)
plugins/logging/main.go (1)
LoggerPlugin(86-100)core/schemas/bifrost.go (4)
BifrostRequest(24-36)ResponsesRequest(106-106)TextCompletionRequest(102-102)EmbeddingRequest(108-108)core/schemas/mux.go (1)
ToChatMessages(424-616)core/schemas/chatcompletions.go (3)
ChatMessageRoleUser(195-195)ChatContentBlock(280-287)ChatContentBlockTypeText(272-272)
🔇 Additional comments (6)
plugins/logging/operations.go (3)
43-46: Latency cast fix LGTM.Casting to float64 matches the DB column type.
66-73: responses_output serialization LGTM.Correct use of tempEntry for JSON serialization.
171-176: Streaming latency update LGTM.Using stream-derived latency is correct for both delta and final.
plugins/logging/main.go (2)
316-320: Latency propagation LGTM.Measured latency is now persisted for non-stream updates.
Also applies to: 475-477
421-423: responses_output update LGTM.Correctly persists Responses API results for non-stream calls.
ui/lib/types/logs.ts (1)
250-250: LGTM!The
responses_outputfield addition aligns with the PR objective of adding ResponsesMessage support to logging.
Pratham-Mishra04
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
fff112b to
b9d12c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
framework/streaming/responses.go (2)
414-427: Set Data.Model for delta responses too.Same issue as the final chunk path:
modelis extracted but not assigned todata.Model.Apply this diff:
data, processErr := a.processAccumulatedResponsesStreamingChunks(requestID, bifrostErr, isFinalChunk) if processErr != nil { a.logger.Error("failed to process accumulated responses chunks for request %s: %v", requestID, processErr) return nil, processErr } + data.Model = model return &ProcessedStreamResponse{ Type: StreamResponseTypeDelta,
395-408: Set Data.Model for logging persistence.The
modelvariable is extracted (line 273) but never assigned todata.Modelbefore returning. Without this, the logging plugin cannot persist the model for responses streams, asupdateStreamingLogEntryreadsstreamResponse.Data.Model(plugins/logging/operations.go:177).Apply this diff:
data, processErr := a.processAccumulatedResponsesStreamingChunks(requestID, bifrostErr, isFinalChunk) if processErr != nil { a.logger.Error("failed to process accumulated responses chunks for request %s: %v", requestID, processErr) return nil, processErr } + data.Model = model return &ProcessedStreamResponse{ Type: StreamResponseTypeFinal,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
framework/logstore/tables.go(5 hunks)framework/pricing/main.go(1 hunks)framework/streaming/accumulator.go(10 hunks)framework/streaming/responses.go(1 hunks)framework/streaming/types.go(4 hunks)plugins/logging/main.go(11 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/pool.go(2 hunks)plugins/logging/utils.go(2 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)ui/app/logs/views/logChatMessageView.tsx(1 hunks)ui/app/logs/views/logDetailsSheet.tsx(4 hunks)ui/app/logs/views/logMessageView.tsx(0 hunks)ui/app/logs/views/logResponsesOutputView.tsx(1 hunks)ui/lib/constants/logs.ts(2 hunks)ui/lib/types/logs.ts(3 hunks)ui/lib/utils/validation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ui/app/logs/views/logMessageView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/lib/utils/validation.ts
- framework/pricing/main.go
🧰 Additional context used
🧬 Code graph analysis (11)
ui/app/logs/views/logChatMessageView.tsx (3)
ui/lib/types/logs.ts (2)
ChatMessage(113-121)ContentBlock(98-109)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/handlers/utils.go (1)
SendError(25-34)
plugins/logging/utils.go (5)
plugins/logging/main.go (1)
LoggerPlugin(86-100)core/schemas/bifrost.go (4)
BifrostRequest(24-36)ResponsesRequest(106-106)TextCompletionRequest(102-102)EmbeddingRequest(108-108)transports/bifrost-http/handlers/inference.go (3)
ChatRequest(165-169)ResponsesRequest(195-199)EmbeddingRequest(202-206)core/schemas/mux.go (1)
ToChatMessages(424-616)core/schemas/chatcompletions.go (3)
ChatMessageRoleUser(195-195)ChatContentBlock(280-287)ChatContentBlockTypeText(272-272)
framework/logstore/tables.go (2)
ui/lib/types/logs.ts (3)
ResponsesMessage(405-420)BifrostEmbedding(123-127)ResponsesReasoning(399-402)core/schemas/chatcompletions.go (1)
ChatAssistantMessageToolCall(339-343)
framework/streaming/types.go (3)
ui/lib/types/logs.ts (3)
ResponsesMessage(405-420)LLMUsage(189-195)BifrostError(218-224)core/schemas/responses.go (2)
ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (3)
LLMUsage(296-302)BifrostCacheDebug(361-375)BifrostError(409-418)
ui/app/logs/views/logDetailsSheet.tsx (2)
ui/app/logs/views/logChatMessageView.tsx (1)
LogChatMessageView(65-181)ui/app/logs/views/logResponsesOutputView.tsx (1)
LogResponsesOutputView(337-347)
framework/streaming/accumulator.go (2)
framework/streaming/types.go (3)
ResponsesStreamChunk(84-92)StreamAccumulator(95-107)ChatStreamChunk(73-81)core/schemas/bifrost.go (1)
ResponsesStreamRequest(107-107)
framework/streaming/responses.go (5)
framework/streaming/accumulator.go (1)
Accumulator(15-31)framework/streaming/types.go (7)
ResponsesStreamChunk(84-92)AccumulatedData(28-47)ProcessedStreamResponse(110-117)StreamResponseTypeFinal(24-24)StreamType(10-10)StreamTypeResponses(17-17)StreamResponseTypeDelta(23-23)core/schemas/responses.go (11)
ResponsesStreamResponseTypeOutputItemAdded(1630-1630)ResponsesStreamResponseTypeContentPartAdded(1633-1633)ResponsesStreamResponseTypeOutputTextDelta(1637-1637)ResponsesStreamResponseTypeRefusalDelta(1640-1640)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)ResponsesOutputMessageContentTypeText(624-624)ResponsesOutputMessageContentText(664-667)ResponsesOutputMessageContentTypeRefusal(625-625)ResponsesOutputMessageContentRefusal(689-691)ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostResponse(238-254)BifrostContextKeyRequestID(120-120)core/utils.go (2)
GetRequestFields(145-151)IsFinalChunk(127-142)
ui/app/logs/views/logResponsesOutputView.tsx (3)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesMessageContentBlock(344-379)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
plugins/logging/operations.go (3)
plugins/logging/main.go (2)
LoggerPlugin(86-100)UpdateLogData(42-55)framework/streaming/types.go (1)
ProcessedStreamResponse(110-117)framework/logstore/tables.go (2)
Log(70-120)Log(123-125)
plugins/logging/main.go (5)
ui/lib/types/logs.ts (4)
ChatMessage(113-121)ResponsesMessage(405-420)BifrostEmbedding(123-127)BifrostError(218-224)core/schemas/chatcompletions.go (2)
ChatMessage(202-211)BifrostNonStreamResponseChoice(370-373)core/schemas/responses.go (2)
ResponsesMessage(280-292)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostError(409-418)RequestType(99-99)framework/streaming/types.go (1)
StreamResponseTypeFinal(24-24)
🔇 Additional comments (22)
transports/bifrost-http/handlers/inference.go (1)
671-674: LGTM! Validation prevents malformed model strings.The added validation ensures both provider and model name are non-empty after parsing, consistent with similar validation patterns used in other handlers (lines 295-297, 361-363, 431-433, etc.). This prevents downstream issues from malformed model strings.
plugins/logging/utils.go (2)
88-145: LGTM! Input history extraction is well-structured.The function correctly handles multiple request types (Chat, Responses, TextCompletion, Embedding) and converts them to a uniform
[]schemas.ChatMessageformat. The per-iteration copy at lines 129-130 properly avoids memory aliasing issues.
55-86: Supported Go version confirmed
plugins/logging/go.modspecifiesgo 1.24, which supports thefor attempt := range maxRetriessyntax.ui/lib/types/logs.ts (2)
250-250: LGTM! New field supports Responses API logging.The
responses_output?: ResponsesMessage[]field properly extendsLogEntryto support the new Responses API output format.
305-544: LGTM! Comprehensive type definitions for Responses API.The new type definitions are well-structured and comprehensive, covering:
- Message roles and types
- Content blocks with multiple variants
- Tool configurations and choices
- Reasoning parameters
- Usage and error structures
These align with the backend schemas and provide strong typing for the UI.
plugins/logging/pool.go (2)
20-23: LGTM! Pool cleanup is complete.The comment and pointer clearing for
StreamResponseare now correct and consistent with the pool management pattern.
38-46: LGTM! Field resets align with refactored data model.The changes from
OutputMessage/ToolCallstoChatOutput/ResponsesOutputproperly reflect the updated log data structure that supports the Responses API.ui/app/logs/views/logDetailsSheet.tsx (3)
13-16: LGTM! Import updates support new message views.The import changes correctly replace
LogMessageViewwithLogChatMessageViewand addLogResponsesOutputViewfor rendering the new Responses API output.
248-248: LGTM! Consistent migration to LogChatMessageView.All message rendering now uses
LogChatMessageView, providing a uniform block-based rendering approach for conversation history, input, responses, and embedding output.Also applies to: 257-257, 268-269, 280-289
271-276: LGTM! Responses output rendering is properly conditional.The new block correctly renders
LogResponsesOutputViewwhenresponses_outputexists and there's no error, supporting the Responses API logging feature.framework/logstore/tables.go (3)
79-79: LGTM! New fields support Responses API persistence.The
ResponsesOutput(string) andResponsesOutputParsed(virtual field) properly extend the Log model to store and retrieve Responses API output, following the established pattern for other output types.Also applies to: 108-108
163-169: LGTM! Serialization follows established patterns.The serialization and deserialization logic for
ResponsesOutputcorrectly mirrors the pattern used for other fields likeOutputMessageandEmbeddingOutput.Also applies to: 286-291
418-440: Add nil check for ResponsesReasoning.Line 434 accesses
msg.ResponsesReasoning.Summarywithout checking ifResponsesReasoningis nil, which could cause a panic.Apply this diff to add the nil check:
// Add responses output content if l.ResponsesOutputParsed != nil { for _, msg := range l.ResponsesOutputParsed { if msg.Content != nil { if msg.Content.ContentStr != nil && *msg.Content.ContentStr != "" { parts = append(parts, *msg.Content.ContentStr) } // If content blocks exist, extract text from them if msg.Content.ContentBlocks != nil { for _, block := range msg.Content.ContentBlocks { if block.Text != nil && *block.Text != "" { parts = append(parts, *block.Text) } } } } - if msg.ResponsesReasoning != nil { + if msg.ResponsesReasoning != nil && msg.ResponsesReasoning.Summary != nil { for _, summary := range msg.ResponsesReasoning.Summary { parts = append(parts, summary.Text) } } } }Likely an incorrect or invalid review comment.
ui/app/logs/views/logChatMessageView.tsx (1)
1-181: LGTM! Well-structured chat message view component.The new
LogChatMessageViewcomponent provides comprehensive block-based rendering for chat messages, properly handling:
- Multiple content types (text, images, audio)
- Special fields (thought, refusal, tool calls, annotations)
- JSON and plain text rendering with appropriate editors
- Proper use of helper functions (isJson, cleanJson)
The component is modular, maintainable, and aligns with the new Responses API logging requirements.
framework/streaming/types.go (3)
17-17: LGTM! New fields support Responses API streaming.The
StreamTypeResponsesconstant andOutputMessagesfield properly extend the streaming infrastructure to handle Responses API output.Also applies to: 37-37
83-92: LGTM! ResponsesStreamChunk follows established patterns.The new
ResponsesStreamChunktype andResponsesStreamChunksfield mirror the structure of existing stream chunk types (Chat, Audio, Transcription), providing consistent handling for Responses API streaming.Also applies to: 99-99
156-162: LGTM! Responses output conversion is complete.The
ToBifrostResponsemethod correctly populatesResponsesResponsewith the accumulated output messages and properCreatedAttimestamp, following the established conversion pattern.framework/streaming/accumulator.go (1)
21-21: LGTM! ResponsesStreamChunk pooling follows established patterns.The ResponsesStreamChunk pool initialization, lifecycle methods (get/put/add), cleanup logic, and streaming flow integration mirror the existing patterns for chat, audio, and transcription chunks. The implementation is consistent and correct.
Also applies to: 84-99, 105-106, 196-218, 228-230, 312-336, 349-351, 417-421, 443-443
plugins/logging/operations.go (1)
42-82: LGTM! Log operations updated for Responses API.The changes correctly update field names (OutputMessage → ChatOutput, ToolCalls → ResponsesOutput), handle latency conversion (int64 → float64), and add ResponsesOutput serialization in both non-streaming and streaming paths. The implementation aligns with the schema changes in framework/logstore/tables.go.
Also applies to: 151-174, 243-251
ui/lib/constants/logs.ts (1)
27-37: LGTM! Request type constants updated for streaming variants.The additions cover all streaming variants (text_completion_stream, chat_completion_stream, responses, responses_stream, speech_stream, transcription_stream) with consistent labels and colors. Well-organized and complete.
Also applies to: 78-99, 105-126
ui/app/logs/views/logResponsesOutputView.tsx (1)
1-347: LGTM! Comprehensive Responses output renderer.The component provides thorough rendering for all ResponsesMessage types, including reasoning, function calls, various tool types, content blocks (text, images, files, audio), refusals, annotations, and logprobs. The implementation is well-structured with clear helper functions and appropriate use of CodeEditor for JSON display. Empty state handling is present.
plugins/logging/main.go (1)
42-55: LGTM! Logging plugin updated for Responses API.The changes correctly:
- Update data structures (ChatOutput, ResponsesOutput replacing OutputMessage, ToolCalls)
- Add Latency field and propagate it through update calls
- Use
string(req.RequestType)for Object field- Extract latency from
result.ExtraFields.Latency- Apply field renames consistently throughout pre/post hooks
The implementation aligns with the schema changes and maintains consistency across streaming and non-streaming paths.
Also applies to: 63-63, 210-210, 316-320, 334-335, 366-366, 396-396, 409-422, 476-476
TejasGhatte
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
b9d12c7 to
fbce5d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/schemas/mux.go (1)
948-969: Use Delta for function-call arguments; stop setting ArgumentsFunction-call arguments deltas should populate Delta, not Arguments, to align with the updated schema and other providers (Anthropic/Cohere). Also set ContentIndex explicitly at the delta site.
Apply this diff:
case len(delta.ToolCalls) > 0: // Tool call delta - handle function call arguments toolCall := delta.ToolCalls[0] // Take first tool call if toolCall.Function.Arguments != "" { - streamResp.Type = ResponsesStreamResponseTypeFunctionCallArgumentsDelta - streamResp.Arguments = &toolCall.Function.Arguments + streamResp.Type = ResponsesStreamResponseTypeFunctionCallArgumentsDelta + streamResp.Delta = Ptr(toolCall.Function.Arguments) + streamResp.ContentIndex = Ptr(0) // Set item for function call metadata if this is a new tool call if toolCall.ID != nil || toolCall.Function.Name != nil { messageType := ResponsesMessageTypeFunctionCall streamResp.Item = &ResponsesMessage{ Type: &messageType, Role: Ptr(ResponsesInputMessageRoleAssistant), ResponsesToolMessage: &ResponsesToolMessage{ CallID: toolCall.ID, Name: toolCall.Function.Name, }, } } }core/schemas/providers/anthropic/responses.go (1)
383-395: Good: using Delta for function-call args (PartialJSON). Ensure reverse mapping uses Delta tooThe forward path now correctly uses Delta. Update ToAnthropicResponsesStreamResponse to read Delta, not Arguments, for function-call arguments deltas.
Apply this diff in ToAnthropicResponsesStreamResponse:
- if responsesStream.Arguments != nil { + if responsesStream.Delta != nil { streamResp.Delta = &AnthropicStreamDelta{ Type: AnthropicStreamDeltaTypeInputJSON, - PartialJSON: responsesStream.Arguments, + PartialJSON: responsesStream.Delta, } }
♻️ Duplicate comments (1)
ui/lib/types/logs.ts (1)
495-499: Use JSDoc @deprecated tag for better tooling support.The
generate_summaryfield uses an inline// Deprecatedcomment. TypeScript tooling provides better support (IDE warnings, strikethrough, etc.) when using JSDoc annotations.Apply this diff:
export interface ResponsesParametersReasoning { effort?: "minimal" | "low" | "medium" | "high"; + /** + * @deprecated Use `summary` instead + */ - generate_summary?: string; // Deprecated + generate_summary?: string; summary?: "auto" | "concise" | "detailed"; }
🧹 Nitpick comments (6)
core/schemas/mux.go (1)
910-915: Set ContentIndex per delta; avoid setting it at item-added initializationInitializing ContentIndex at item-added is misleading. Assign it where deltas occur (text/thought/refusal/args) instead.
Apply this diff:
streamResp := &ResponsesStreamResponse{ SequenceNumber: br.ExtraFields.ChunkIndex, - ContentIndex: Ptr(0), OutputIndex: &choice.Index, } @@ case delta.Content != nil && *delta.Content != "": if delta.Content != nil && *delta.Content != "" { // Need this check again because of the fallthrough // Text content delta streamResp.Type = ResponsesStreamResponseTypeOutputTextDelta streamResp.Delta = delta.Content + streamResp.ContentIndex = Ptr(0) } @@ case delta.Thought != nil && *delta.Thought != "": // Reasoning/thought content delta (for models that support reasoning) streamResp.Type = ResponsesStreamResponseTypeReasoningSummaryTextDelta streamResp.Delta = delta.Thought + streamResp.ContentIndex = Ptr(0)Optionally add similar assignment in the refusal delta branch.
Also applies to: 931-936, 938-947
plugins/logging/utils.go (1)
100-109: Preserve prompt boundaries for text completionsJoin PromptArray with a separator to avoid concatenation artifacts.
Apply this diff:
- var stringBuilder strings.Builder - for _, prompt := range request.TextCompletionRequest.Input.PromptArray { - stringBuilder.WriteString(prompt) - } - text = stringBuilder.String() + text = strings.Join(request.TextCompletionRequest.Input.PromptArray, "\n")framework/logstore/tables.go (1)
286-291: Consider consistency in deserialization error handling.When deserialization fails,
ResponsesOutputParsedis initialized to an empty slice[]schemas.ResponsesMessage{}(line 289), whileOutputMessageParsedis set tonil(line 282) andEmbeddingOutputParsedis set tonil(line 296).For consistency, consider initializing all failed deserializations the same way (either all
nilor all empty collections). The current mix could lead to subtle bugs if code assumes consistent nil-vs-empty behavior.ui/app/logs/views/logResponsesOutputView.tsx (3)
305-322: Extract excluded keys to a constant.The list of excluded keys is hardcoded in two places (lines 306 and 319). Extract this to a constant at the top of the file to ensure consistency and ease maintenance.
Add this constant near the top of the file (after imports):
const CORE_MESSAGE_KEYS = ["id", "type", "status", "role", "content", "call_id", "name", "arguments", "summary", "encrypted_content"] as const;Then update both filter calls to use this constant:
- (key) => !["id", "type", "status", "role", "content", "call_id", "name", "arguments", "summary", "encrypted_content"].includes(key), + (key) => !CORE_MESSAGE_KEYS.includes(key as typeof CORE_MESSAGE_KEYS[number]),
206-206: Remove or uncomment the status display.The commented-out status display should either be removed if not needed or uncommented if it's meant to be shown.
9-161: Consider extracting CodeEditor helper to reduce duplication.Multiple CodeEditor instances throughout the component share very similar props (lines 39-48, 58-74, 81-99, 106-115, 130-139, 147-156, etc.). While the current implementation is functional, extracting a helper function or component could improve maintainability.
Example helper function:
const renderJsonEditor = (code: string, maxHeight: number = 200) => ( <CodeEditor className="z-0 w-full" shouldAdjustInitialHeight={true} maxHeight={maxHeight} wrap={true} code={code} lang="json" readonly={true} options={{ scrollBeyondLastLine: false, collapsibleBlocks: true, lineNumbers: "off", alwaysConsumeMouseWheel: false }} /> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
core/schemas/mux.go(1 hunks)core/schemas/providers/anthropic/responses.go(1 hunks)core/schemas/providers/cohere/responses.go(11 hunks)framework/logstore/tables.go(5 hunks)framework/pricing/main.go(1 hunks)framework/streaming/accumulator.go(10 hunks)framework/streaming/responses.go(1 hunks)framework/streaming/types.go(4 hunks)plugins/logging/main.go(11 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/pool.go(2 hunks)plugins/logging/utils.go(2 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)ui/app/logs/views/logChatMessageView.tsx(1 hunks)ui/app/logs/views/logDetailsSheet.tsx(4 hunks)ui/app/logs/views/logMessageView.tsx(0 hunks)ui/app/logs/views/logResponsesOutputView.tsx(1 hunks)ui/lib/constants/logs.ts(2 hunks)ui/lib/types/logs.ts(3 hunks)ui/lib/utils/validation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ui/app/logs/views/logMessageView.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/pricing/main.go
- transports/bifrost-http/handlers/inference.go
- framework/streaming/types.go
- ui/lib/utils/validation.ts
🧰 Additional context used
🧬 Code graph analysis (10)
plugins/logging/utils.go (3)
core/schemas/bifrost.go (4)
BifrostRequest(24-36)ResponsesRequest(106-106)TextCompletionRequest(102-102)EmbeddingRequest(108-108)core/schemas/mux.go (1)
ToChatMessages(424-616)core/schemas/chatcompletions.go (3)
ChatMessageRoleUser(195-195)ChatContentBlock(280-287)ChatContentBlockTypeText(272-272)
ui/app/logs/views/logResponsesOutputView.tsx (3)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesMessageContentBlock(344-379)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
core/schemas/providers/cohere/responses.go (1)
core/schemas/responses.go (1)
ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)
framework/streaming/responses.go (5)
framework/streaming/accumulator.go (1)
Accumulator(15-31)framework/streaming/types.go (7)
ResponsesStreamChunk(84-92)AccumulatedData(28-47)ProcessedStreamResponse(110-117)StreamResponseTypeFinal(24-24)StreamType(10-10)StreamTypeResponses(17-17)StreamResponseTypeDelta(23-23)core/schemas/responses.go (11)
ResponsesStreamResponseTypeOutputItemAdded(1630-1630)ResponsesStreamResponseTypeContentPartAdded(1633-1633)ResponsesStreamResponseTypeOutputTextDelta(1637-1637)ResponsesStreamResponseTypeRefusalDelta(1640-1640)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)ResponsesOutputMessageContentTypeText(624-624)ResponsesOutputMessageContentText(664-667)ResponsesOutputMessageContentTypeRefusal(625-625)ResponsesOutputMessageContentRefusal(689-691)ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostResponse(238-254)BifrostContextKeyRequestID(120-120)core/utils.go (2)
GetRequestFields(145-151)IsFinalChunk(127-142)
framework/streaming/accumulator.go (2)
framework/streaming/types.go (3)
ResponsesStreamChunk(84-92)StreamAccumulator(95-107)ChatStreamChunk(73-81)core/schemas/bifrost.go (1)
ResponsesStreamRequest(107-107)
framework/logstore/tables.go (2)
ui/lib/types/logs.ts (3)
ResponsesMessage(405-420)BifrostEmbedding(123-127)ResponsesReasoning(399-402)core/schemas/chatcompletions.go (2)
ChatTool(58-62)ChatAssistantMessageToolCall(339-343)
ui/app/logs/views/logDetailsSheet.tsx (2)
ui/app/logs/views/logChatMessageView.tsx (1)
LogChatMessageView(65-181)ui/app/logs/views/logResponsesOutputView.tsx (1)
LogResponsesOutputView(337-347)
plugins/logging/main.go (5)
ui/lib/types/logs.ts (4)
ChatMessage(113-121)ResponsesMessage(405-420)BifrostEmbedding(123-127)BifrostError(218-224)core/schemas/chatcompletions.go (2)
ChatMessage(202-211)BifrostNonStreamResponseChoice(370-373)core/schemas/responses.go (2)
ResponsesMessage(280-292)ResponsesResponse(85-114)core/schemas/bifrost.go (1)
BifrostError(409-418)framework/streaming/types.go (1)
StreamResponseTypeFinal(24-24)
plugins/logging/operations.go (4)
plugins/logging/main.go (1)
UpdateLogData(42-55)core/schemas/bifrost.go (1)
BifrostCacheDebug(361-375)framework/streaming/types.go (1)
ProcessedStreamResponse(110-117)framework/logstore/tables.go (2)
Log(70-120)Log(123-125)
ui/app/logs/views/logChatMessageView.tsx (3)
ui/lib/types/logs.ts (2)
ChatMessage(113-121)ContentBlock(98-109)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
🔇 Additional comments (6)
core/schemas/providers/cohere/responses.go (1)
448-454: Cohere streaming alignment looks solidOutputIndex is consistently set; function-call args use Delta with ContentIndex; Completed includes Response. All good.
Also applies to: 566-572, 652-659
plugins/logging/pool.go (1)
20-23: Pool cleanup fix looks correctClearing StreamResponse and new ChatOutput/ResponsesOutput fields prevents leaks and aligns with the refactor.
Also applies to: 38-47
plugins/logging/utils.go (1)
61-83: Range-over-int supported by Go 1.24
plugins/logging/go.moddeclares Go 1.24, sofor attempt := range maxRetriesis valid—no changes needed.ui/app/logs/views/logDetailsSheet.tsx (1)
263-276: Verify intentional dual rendering of Response blocks.Both the
output_messageblock (lines 263-269) and theresponses_outputblock (lines 271-276) will render when both fields are populated and no error exists. This results in two separate "Response" sections appearing in the UI.Is this intentional behavior, or should these blocks be mutually exclusive? If they should be mutually exclusive, consider adding an
else ifor a condition like&& !log.responses_outputto the first block.framework/streaming/responses.go (1)
395-408: Verify Model field is populated in accumulated data.The
ProcessedStreamResponsereturned at line 401-408 includesModel: modelat the response level, butdata.Modelis not explicitly set before returning. The logging code inplugins/logging/operations.goat line 177 readsstreamResponse.Data.Modelto persist the model.Verify that
data.Modelis populated somewhere before this return, or add the assignment:data, processErr := a.processAccumulatedResponsesStreamingChunks(requestID, bifrostErr, isFinalChunk) if processErr != nil { a.logger.Error("failed to process accumulated responses chunks for request %s: %v", requestID, processErr) return nil, processErr } data.Model = model // Ensure model is set for logging return &ProcessedStreamResponse{ Type: StreamResponseTypeFinal, RequestID: requestID, StreamType: StreamTypeResponses, Provider: provider, Model: model, Data: data, }, nilSimilarly at lines 413-420 for the delta response path.
Based on learnings
ui/lib/constants/logs.ts (1)
26-127: LGTM! Comprehensive type, label, and color mappings added.The expanded
RequestTypes,RequestTypeLabels, andRequestTypeColorsprovide complete support for the new Responses API and streaming variants. The dual format (legacy dot-notation and new underscore-notation) appears intentional for backwards compatibility, and all new types have corresponding labels and colors.
TejasGhatte
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
fbce5d1 to
8980865
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 (4)
core/schemas/providers/anthropic/responses.go (2)
555-560: Fix reverse mapping: use Delta, not Arguments, for function-call arguments
ToAnthropicResponsesStreamResponsestill readsArguments, but producers now populateDelta. This drops tool-call argument chunks.Apply this diff:
- if responsesStream.Arguments != nil { - streamResp.Delta = &AnthropicStreamDelta{ - Type: AnthropicStreamDeltaTypeInputJSON, - PartialJSON: responsesStream.Arguments, - } - } + if responsesStream.Delta != nil { + streamResp.Delta = &AnthropicStreamDelta{ + Type: AnthropicStreamDeltaTypeInputJSON, + PartialJSON: responsesStream.Delta, + } + }
1298-1316: Guard against nil Content to avoid panics
convertBifrostMessagesToAnthropicContentdereferencesmsg.Contentwithout nil checks in multiple branches. Unknown/edge types can be nil and crash.Apply this diff:
case schemas.ResponsesMessageTypeMessage: - // Regular text message - if msg.Content.ContentStr != nil { + // Regular text message + if msg.Content != nil && msg.Content.ContentStr != nil { contentBlocks = append(contentBlocks, AnthropicContentBlock{ Type: "text", Text: msg.Content.ContentStr, }) - } else if msg.Content.ContentBlocks != nil { + } else if msg.Content != nil && msg.Content.ContentBlocks != nil { // Convert content blocks for _, block := range msg.Content.ContentBlocks { anthropicBlock := convertContentBlockToAnthropic(block) if anthropicBlock != nil { contentBlocks = append(contentBlocks, *anthropicBlock) } } } ... default: // Handle other types as text if they have content - if msg.Content.ContentStr != nil { + if msg.Content != nil && msg.Content.ContentStr != nil { contentBlocks = append(contentBlocks, AnthropicContentBlock{ Type: AnthropicContentBlockTypeText, Text: msg.Content.ContentStr, }) }Also applies to: 1413-1420
plugins/logging/main.go (1)
361-368: Streaming cache_debug not persisted (nil passed).updateStreamingLogEntry relies on the cacheDebug param, but here it’s always nil for streams. Cache debug from the stream never gets stored.
Apply one of:
- Preferred (centralized): Update operations.go to fall back to streamResponse.Data.CacheDebug when param is nil.
- Minimal (call-site): Pass streamResponse.Data.CacheDebug here.
Example (call-site) diff:
- processingErr := retryOnNotFound(p.ctx, func() error { - return p.updateStreamingLogEntry(p.ctx, logMsg.RequestID, logMsg.SemanticCacheDebug, logMsg.StreamResponse, streamResponse.Type == streaming.StreamResponseTypeFinal) - }) + processingErr := retryOnNotFound(p.ctx, func() error { + // Prefer passing accumulated cache debug if available + var cd *schemas.BifrostCacheDebug + if streamResponse != nil && streamResponse.Data != nil { + cd = streamResponse.Data.CacheDebug + } + return p.updateStreamingLogEntry(p.ctx, logMsg.RequestID, cd, logMsg.StreamResponse, streamResponse.Type == streaming.StreamResponseTypeFinal) + })plugins/logging/operations.go (1)
224-232: Persist cache_debug for streams even when param is nil.Currently only the parameter is used; streams pass nil, so cache_debug is dropped. Fall back to streamResponse.Data.CacheDebug.
- // Handle cache debug - if cacheDebug != nil { - tempEntry.CacheDebugParsed = cacheDebug - if err := tempEntry.SerializeFields(); err != nil { - p.logger.Error("failed to serialize cache debug: %v", err) - } else { - updates["cache_debug"] = tempEntry.CacheDebug - } - } + // Handle cache debug (prefer explicit param; fallback to accumulated data) + effectiveCacheDebug := cacheDebug + if effectiveCacheDebug == nil && streamResponse != nil && streamResponse.Data != nil { + effectiveCacheDebug = streamResponse.Data.CacheDebug + } + if effectiveCacheDebug != nil { + tempEntry.CacheDebugParsed = effectiveCacheDebug + if err := tempEntry.SerializeFields(); err != nil { + p.logger.Error("failed to serialize cache debug: %v", err) + } else { + updates["cache_debug"] = tempEntry.CacheDebug + } + }
♻️ Duplicate comments (6)
framework/streaming/responses.go (2)
86-91: Ordering/indexing of output content may be incorrect.
- Map-to-slice conversion assumes contiguous 0..N-1 indices; gaps will drop/shuffle outputs.
- ContentPartAdded appends parts, ignoring content_index ordering.
Collect and sort keys before appending, and place parts by content_index rather than append.
Also applies to: 35-53
401-409: Model not propagated to Data; logging won’t persist served model.updateStreamingLogEntry reads streamResponse.Data.Model, but Data.Model is never set here.
- return &ProcessedStreamResponse{ + data.Model = model + return &ProcessedStreamResponse{ Type: StreamResponseTypeFinal, RequestID: requestID, StreamType: StreamTypeResponses, Provider: provider, Model: model, Data: data, }, nil(Also set data.Model in the OpenAI final path above after constructing data.)
ui/lib/types/logs.ts (2)
119-121: Clarify/deprecate tool_calls commentContent types don’t surface tool calls; the current inline note is misleading. Mark the field deprecated and clarify intent.
Apply:
- tool_calls?: ToolCall[]; // For backward compatibility, tool calls are now in the content + /** + * @deprecated Kept for backward compatibility only; ChatMessageContent/ContentBlock do not expose tool calls. + */ + tool_calls?: ToolCall[];
495-499: Use @deprecated JSDoc instead of inline commentReplace inline “// Deprecated” with a JSDoc deprecation tag for IDE/tooling support.
export interface ResponsesParametersReasoning { effort?: "minimal" | "low" | "medium" | "high"; - generate_summary?: string; // Deprecated + /** + * @deprecated Use `summary` instead + */ + generate_summary?: string; summary?: "auto" | "concise" | "detailed"; }ui/app/logs/views/logResponsesOutputView.tsx (2)
279-301: Add padding wrapper around Tool Details editor (consistency)Wrap the CodeEditor with a div having px-6 py-2 like other sections.
<div className="border-b last:border-b-0"> <div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium">Tool Details</div> - <CodeEditor + <div className="px-6 py-2"> + <CodeEditor className="z-0 w-full" shouldAdjustInitialHeight={true} maxHeight={200} wrap={true} code={JSON.stringify( { ...(message.call_id && { call_id: message.call_id }), ...(message.name && { name: message.name }), ...(message.arguments && { arguments: isJson(message.arguments) ? cleanJson(message.arguments) : message.arguments }), }, null, 2, )} lang="json" readonly={true} options={{ scrollBeyondLastLine: false, collapsibleBlocks: true, lineNumbers: "off", alwaysConsumeMouseWheel: false }} - /> + /> + </div> </div>
304-331: Add padding wrapper around Additional Fields editor (consistency)Match section spacing by adding the standard px-6 py-2 wrapper.
<div className="border-b last:border-b-0"> <div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium">Additional Fields</div> - <CodeEditor + <div className="px-6 py-2"> + <CodeEditor className="z-0 w-full" shouldAdjustInitialHeight={true} maxHeight={200} wrap={true} code={JSON.stringify( Object.fromEntries( Object.entries(message).filter( ([key]) => !["id", "type", "status", "role", "content", "call_id", "name", "arguments", "summary", "encrypted_content"].includes( key, ), ), ), null, 2, )} lang="json" readonly={true} options={{ scrollBeyondLastLine: false, collapsibleBlocks: true, lineNumbers: "off", alwaysConsumeMouseWheel: false }} - /> + /> + </div> </div>
🧹 Nitpick comments (8)
core/schemas/providers/anthropic/responses.go (1)
505-520: Handle function_call items in reverse streaming mappingWhen
ResponsesStreamResponseTypeOutputItemAddedcarries afunction_callitem, mapping it tomessage_startloses tool-use semantics. Prefer emitting acontent_block_startwithtool_useand setName/ID.Minimal approach: branch on
responsesStream.Item.Type == function_calland populate:
streamResp.Type = AnthropicStreamEventTypeContentBlockStartstreamResp.ContentBlock = { Type: AnthropicContentBlockTypeToolUse, Name, ID }This aligns with how forward mapping emits
tool_usestarts.core/schemas/providers/cohere/responses.go (1)
640-647: Confirm end event type for citations
StreamEventCitationEndmaps toOutputTextAnnotationAddedagain. Should this be...AnnotationDone(if defined) to mark completion?If supported, change:
- Type: schemas.ResponsesStreamResponseTypeOutputTextAnnotationAdded, + Type: schemas.ResponsesStreamResponseTypeOutputTextAnnotationDone,Please verify the exact constant name in
core/schemas/responses.go.framework/logstore/tables.go (1)
79-84: Add DB migration and update filters for new Responses entriesNew columns/fields (
ResponsesOutput,ResponsesOutputParsed) require schema migration, and “object” values may include “response”. Ensure:
- DB migration adds
responses_output(and backfill if needed).- UI/API filters accept the new object type so “Objects” filtering doesn’t miss Responses logs.
I can draft a GORM auto-migrate snippet or a SQL migration if you share your migration path.
Also applies to: 106-113
framework/streaming/accumulator.go (2)
228-236: Return chunks via reset helpers before pooling.Directly pooling chunks without zeroing fields risks stale data reuse. Use the existing put* helpers.
- for _, chunk := range acc.ResponsesStreamChunks { - a.responsesStreamChunkPool.Put(chunk) - } + for _, chunk := range acc.ResponsesStreamChunks { + a.putResponsesStreamChunk(chunk) + }(Optional parity with others):
- for _, chunk := range acc.ChatStreamChunks { a.chatStreamChunkPool.Put(chunk) } + for _, chunk := range acc.ChatStreamChunks { a.putChatStreamChunk(chunk) } - for _, chunk := range acc.TranscriptionStreamChunks { a.transcriptionStreamChunkPool.Put(chunk) } + for _, chunk := range acc.TranscriptionStreamChunks { a.putTranscriptionStreamChunk(chunk) } - for _, chunk := range acc.AudioStreamChunks { a.audioStreamChunkPool.Put(chunk) } + for _, chunk := range acc.AudioStreamChunks { a.putAudioStreamChunk(chunk) }Also applies to: 349-351
176-182: Avoid defer within deferred cleanup; call directly after unlock.The inner defer executes immediately at the end of this closure—use a direct call for clarity.
- defer func() { - accumulator.mu.Unlock() - if isFinalChunk { - // Before unlocking, we cleanup - defer a.cleanupStreamAccumulator(requestID) - } - }() + defer func() { + accumulator.mu.Unlock() + if isFinalChunk { + // Cleanup after unlocking + a.cleanupStreamAccumulator(requestID) + } + }()framework/streaming/responses.go (1)
288-291: Start/End timestamps set to now (OpenAI path).Using time.Now() here breaks latency consistency with accumulator-based paths. Prefer accumulator timestamps or propagate StartTimestamp from PreHook.
ui/lib/types/logs.ts (2)
263-266: Align status documentation with allowed valuesComment lists only "success" and "error" while Statuses includes "processing" and "cancelled". Update the doc to avoid confusion.
Apply:
- status: string; // "success" or "error" + status: string; // "success" | "error" | "processing" | "cancelled"
305-421: Responses types: solid foundation; consider tightening a few unionsDefinitions look comprehensive. Minor type hygiene ideas:
- ResponsesMessage.status and some string-typed fields (e.g., detail on content blocks) could be unions for safer usage.
If safe, narrow:
- detail?: string; // "low" | "high" | "auto" + detail?: "low" | "high" | "auto";And optionally:
- status?: string; // "in_progress" | "completed" | "incomplete" | "interpreting" | "failed" + status?: "in_progress" | "completed" | "incomplete" | "interpreting" | "failed";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
core/schemas/mux.go(1 hunks)core/schemas/providers/anthropic/responses.go(2 hunks)core/schemas/providers/cohere/responses.go(11 hunks)framework/logstore/tables.go(5 hunks)framework/pricing/main.go(1 hunks)framework/streaming/accumulator.go(10 hunks)framework/streaming/responses.go(1 hunks)framework/streaming/types.go(4 hunks)plugins/logging/main.go(11 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/pool.go(2 hunks)plugins/logging/utils.go(2 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)ui/app/logs/views/logChatMessageView.tsx(1 hunks)ui/app/logs/views/logDetailsSheet.tsx(4 hunks)ui/app/logs/views/logMessageView.tsx(0 hunks)ui/app/logs/views/logResponsesOutputView.tsx(1 hunks)ui/lib/constants/logs.ts(2 hunks)ui/lib/types/logs.ts(3 hunks)ui/lib/utils/validation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ui/app/logs/views/logMessageView.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- core/schemas/mux.go
- ui/app/logs/views/logDetailsSheet.tsx
- framework/pricing/main.go
- ui/lib/utils/validation.ts
- transports/bifrost-http/handlers/inference.go
- plugins/logging/pool.go
🧰 Additional context used
🧬 Code graph analysis (11)
ui/app/logs/views/logChatMessageView.tsx (3)
ui/lib/types/logs.ts (2)
ChatMessage(113-121)ContentBlock(98-109)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
plugins/logging/operations.go (4)
plugins/logging/main.go (2)
LoggerPlugin(86-100)UpdateLogData(42-55)core/schemas/bifrost.go (1)
BifrostCacheDebug(361-375)framework/streaming/types.go (1)
ProcessedStreamResponse(110-117)framework/logstore/tables.go (2)
Log(70-120)Log(123-125)
plugins/logging/main.go (4)
ui/lib/types/logs.ts (4)
ChatMessage(113-121)ResponsesMessage(405-420)BifrostEmbedding(123-127)BifrostError(218-224)core/schemas/chatcompletions.go (2)
ChatMessage(202-211)BifrostNonStreamResponseChoice(370-373)core/schemas/responses.go (2)
ResponsesMessage(280-292)ResponsesResponse(85-114)core/schemas/bifrost.go (1)
BifrostError(409-418)
framework/logstore/tables.go (1)
ui/lib/types/logs.ts (3)
ResponsesMessage(405-420)BifrostEmbedding(123-127)ResponsesReasoning(399-402)
ui/app/logs/views/logResponsesOutputView.tsx (3)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesMessageContentBlock(344-379)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
framework/streaming/responses.go (5)
framework/streaming/accumulator.go (1)
Accumulator(15-31)framework/streaming/types.go (7)
ResponsesStreamChunk(84-92)AccumulatedData(28-47)ProcessedStreamResponse(110-117)StreamResponseTypeFinal(24-24)StreamType(10-10)StreamTypeResponses(17-17)StreamResponseTypeDelta(23-23)core/schemas/responses.go (11)
ResponsesStreamResponseTypeOutputItemAdded(1630-1630)ResponsesStreamResponseTypeContentPartAdded(1633-1633)ResponsesStreamResponseTypeOutputTextDelta(1637-1637)ResponsesStreamResponseTypeRefusalDelta(1640-1640)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)ResponsesOutputMessageContentTypeText(624-624)ResponsesOutputMessageContentText(664-667)ResponsesOutputMessageContentTypeRefusal(625-625)ResponsesOutputMessageContentRefusal(689-691)ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostResponse(238-254)BifrostContextKeyRequestID(120-120)core/utils.go (2)
GetRequestFields(145-151)IsFinalChunk(127-142)
framework/streaming/types.go (2)
core/schemas/responses.go (2)
ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (3)
LLMUsage(296-302)BifrostCacheDebug(361-375)BifrostError(409-418)
plugins/logging/utils.go (4)
plugins/logging/main.go (1)
LoggerPlugin(86-100)core/schemas/bifrost.go (4)
BifrostRequest(24-36)ResponsesRequest(106-106)TextCompletionRequest(102-102)EmbeddingRequest(108-108)core/schemas/mux.go (1)
ToChatMessages(424-616)core/schemas/chatcompletions.go (2)
ChatMessageRoleUser(195-195)ChatContentBlock(280-287)
core/schemas/providers/anthropic/responses.go (3)
ui/lib/types/logs.ts (3)
ResponsesMessage(405-420)ContentBlock(98-109)ResponsesToolMessage(385-391)core/schemas/responses.go (2)
ResponsesMessageTypeFunctionCall(261-261)ResponsesStreamResponse(1689-1718)core/schemas/bifrost.go (1)
BifrostResponse(238-254)
framework/streaming/accumulator.go (2)
framework/streaming/types.go (3)
ResponsesStreamChunk(84-92)StreamAccumulator(95-107)ChatStreamChunk(73-81)core/schemas/bifrost.go (1)
ResponsesStreamRequest(107-107)
core/schemas/providers/cohere/responses.go (1)
core/schemas/responses.go (1)
ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)
🔇 Additional comments (12)
core/schemas/providers/cohere/responses.go (1)
447-454: LGTM: OutputIndex is consistently threaded through stream eventsSetting
OutputIndex: Ptr(0)across events improves observability and aligns with the new stream schema.Also applies to: 481-486, 495-501, 509-515, 521-528, 549-555, 565-572, 580-587, 626-634, 642-648, 653-660, 686-690
framework/logstore/tables.go (1)
163-169: LGTM: serialization/deserialization and summary include Responses outputMarshalling/unmarshalling for
ResponsesOutputand summary extraction (including reasoning summaries) look correct and consistent.Also applies to: 286-291, 418-440
ui/app/logs/views/logChatMessageView.tsx (2)
12-12: LGTM: global underscore replacement in type labelsUsing
replaceAll("_", " ")fixes prior issue and improves readability.
115-137: Robust handling of string vs block contentClean string/JSON rendering paths with CodeEditor and monospaced fallback look good.
plugins/logging/utils.go (2)
88-145: Add nil guards to avoid panics when request or nested inputs are nil
extractInputHistorydereferences fields without checkingrequestorInputsub-structs for nil (TextCompletion/Embedding). Add guards.Apply this diff:
func (p *LoggerPlugin) extractInputHistory(request *schemas.BifrostRequest) []schemas.ChatMessage { - if request.ChatRequest != nil { + if request == nil { + return []schemas.ChatMessage{} + } + if request.ChatRequest != nil { return request.ChatRequest.Input } if request.ResponsesRequest != nil { messages := schemas.ToChatMessages(request.ResponsesRequest.Input) if len(messages) > 0 { return messages } } - if request.TextCompletionRequest != nil { + if request.TextCompletionRequest != nil && request.TextCompletionRequest.Input != nil { var text string if request.TextCompletionRequest.Input.PromptStr != nil { text = *request.TextCompletionRequest.Input.PromptStr } else { var stringBuilder strings.Builder - for _, prompt := range request.TextCompletionRequest.Input.PromptArray { + for _, prompt := range request.TextCompletionRequest.Input.PromptArray { stringBuilder.WriteString(prompt) } text = stringBuilder.String() } return []schemas.ChatMessage{ { Role: schemas.ChatMessageRoleUser, Content: &schemas.ChatMessageContent{ ContentStr: &text, }, }, } } - if request.EmbeddingRequest != nil { - texts := request.EmbeddingRequest.Input.Texts + if request.EmbeddingRequest != nil && request.EmbeddingRequest.Input != nil { + texts := request.EmbeddingRequest.Input.Texts if len(texts) == 0 && request.EmbeddingRequest.Input.Text != nil { texts = []string{*request.EmbeddingRequest.Input.Text} } contentBlocks := make([]schemas.ChatContentBlock, len(texts)) for i, text := range texts { // Create a per-iteration copy to avoid reusing the same memory address t := text contentBlocks[i] = schemas.ChatContentBlock{ Type: schemas.ChatContentBlockTypeText, Text: &t, } } return []schemas.ChatMessage{ { Role: schemas.ChatMessageRoleUser, Content: &schemas.ChatMessageContent{ ContentBlocks: contentBlocks, }, }, } } return []schemas.ChatMessage{} }Likely an incorrect or invalid review comment.
55-86: No changes needed: Go version ≥1.22
plugins/logging/go.mod sets Go 1.24 (CI uses 1.24.1), which supports range-over-int.framework/streaming/types.go (2)
156-162: ResponsesResponse mapping looks correct (CreatedAt populated).CreatedAt sourced from StartTimestamp and Output mapped to OutputMessages is correct. LGTM.
83-92: New Responses stream chunk types and accumulator fields are consistent.The struct and accumulator slice additions fit the existing pattern for chat/audio/transcription streams.
Also applies to: 99-100
framework/streaming/responses.go (1)
273-337: No changes needed for OpenAI final-chunk handling. According to OpenAI’s streaming guide, the terminal event includes the complete response object and usage, so the current extraction logic is valid.ui/lib/types/logs.ts (2)
241-252: LGTM: responses_output fieldAdding responses_output?: ResponsesMessage[] to LogEntry looks correct and aligns with the new UI renderer.
Please confirm backend JSON field name and casing match exactly (responses_output) in the HTTP API serializer so the UI renders it without mapping.
545-550: LGTM: WebSocketLogMessage additionoperation and payload on WebSocketLogMessage are clear and useful.
Confirm all websocket consumers were updated for the required operation and payload fields to avoid runtime breakage.
ui/lib/constants/logs.ts (1)
26-38: LGTM: expanded request types/labels/colors; verify legacy vs new keysNew responses/responses_stream entries align with UI additions. Keeping legacy "response" and "response.completion.chunk" seems intentional for compatibility.
Confirm all emitters use the new RequestTypes while the legacy label/color entries remain only for backward compatibility, to avoid inconsistent badges.
Also applies to: 75-100, 102-127
TejasGhatte
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
8980865 to
6679c24
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ui/app/logs/views/logResponsesOutputView.tsx (2)
280-301: Add consistent padding wrapper around CodeEditor.The Tool Details section lacks the
<div className="px-6 py-2">wrapper around its CodeEditor, which is inconsistent with other sections (e.g., lines 37-52, 57-75). This creates visual inconsistency in spacing.Apply this diff for consistent spacing:
<div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium">Tool Details</div> + <div className="px-6 py-2"> <CodeEditor className="z-0 w-full" shouldAdjustInitialHeight={true} maxHeight={200} wrap={true} code={JSON.stringify( { ...(message.call_id && { call_id: message.call_id }), ...(message.name && { name: message.name }), ...(message.arguments && { arguments: isJson(message.arguments) ? cleanJson(message.arguments) : message.arguments }), }, null, 2, )} lang="json" readonly={true} options={{ scrollBeyondLastLine: false, collapsibleBlocks: true, lineNumbers: "off", alwaysConsumeMouseWheel: false }} /> + </div>Note: This was previously marked as addressed in commit 8980865, so this may be a reintroduction of the issue or a different design decision.
304-332: Add consistent padding wrapper around CodeEditor.Similar to the Tool Details section, the Additional Fields section is missing the
<div className="px-6 py-2">wrapper around its CodeEditor.Apply this diff:
<div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium">Additional Fields</div> + <div className="px-6 py-2"> <CodeEditor className="z-0 w-full" shouldAdjustInitialHeight={true} maxHeight={200} wrap={true} code={JSON.stringify( Object.fromEntries( Object.entries(message).filter( ([key]) => !["id", "type", "status", "role", "content", "call_id", "name", "arguments", "summary", "encrypted_content"].includes( key, ), ), ), null, 2, )} lang="json" readonly={true} options={{ scrollBeyondLastLine: false, collapsibleBlocks: true, lineNumbers: "off", alwaysConsumeMouseWheel: false }} /> + </div>framework/streaming/responses.go (1)
284-293: SetData.Modelin OpenAI final path.The
Modelfield is not set in theAccumulatedDatafor OpenAI's final chunk path, causing the same logging issue as the non-OpenAI path.Apply this diff:
if result != nil && result.ResponsesStreamResponse != nil { // Build the complete response from the final chunk data := &AccumulatedData{ RequestID: requestID, + Model: model, Status: "success", Stream: true, StartTimestamp: time.Now(), // You might want to track this better EndTimestamp: time.Now(), Latency: result.ExtraFields.Latency, ErrorDetails: bifrostErr, Object: result.Object, }
🧹 Nitpick comments (8)
ui/lib/types/logs.ts (1)
497-499: Prefer JSDoc@deprecatedannotation for tooling.Replace the inline
// Deprecatedcomment with a JSDoc@deprecatedblock so IDEs surface a warning and downstream consumers get better typing hints.-export interface ResponsesParametersReasoning { - effort?: "minimal" | "low" | "medium" | "high"; - generate_summary?: string; // Deprecated - summary?: "auto" | "concise" | "detailed"; -} +export interface ResponsesParametersReasoning { + effort?: "minimal" | "low" | "medium" | "high"; + /** + * @deprecated Use `summary` instead. + */ + generate_summary?: string; + summary?: "auto" | "concise" | "detailed"; +}ui/app/logs/views/logDetailsSheet.tsx (2)
64-68: Harden latency numeric handlingCast explicitly to number to avoid string/NaN pitfalls. Also prefer Number.isFinite for the “NA” check.
- value={moment(log.timestamp) - .add(log.latency || 0, "ms") + value={moment(log.timestamp) + .add(Number(log.latency) || 0, "ms") .format("YYYY-MM-DD HH:mm:ss A")} ... - value={isNaN(log.latency || 0) ? "NA" : <div>{(log.latency || 0)?.toFixed(2)}ms</div>} + value={ + Number.isFinite(Number(log.latency)) + ? <div>{Number(log.latency).toFixed(2)}ms</div> + : "NA" + }Also applies to: 71-73
263-276: Avoid duplicating “Response” headers when both chat and responses outputs existRender distinct labels (e.g., “Chat Response” and “Responses”) or group under one section to reduce confusion.
- <div className="mt-4 flex w-full items-center gap-2"> - <div className="text-sm font-medium">Response</div> - </div> + <div className="mt-4 flex w-full items-center gap-2"> + <div className="text-sm font-medium">Chat Response</div> + </div> ... - <div className="mt-4 w-full text-left text-sm font-medium">Response</div> + <div className="mt-4 w-full text-left text-sm font-medium">Responses</div>framework/logstore/tables.go (2)
79-80: Fix comment to reflect slice typeField stores a slice, not a single pointer.
- ResponsesOutput string `gorm:"type:text" json:"-"` // JSON serialized *schemas.ResponsesMessage + ResponsesOutput string `gorm:"type:text" json:"-"` // JSON serialized []schemas.ResponsesMessage
418-441: Consider more defensive access for reasoning summary (optional)If schema evolves, guard nested fields to avoid future nil/empty surprises. Current code is safe for nil slices, but a quick length check helps readability.
- if msg.ResponsesReasoning != nil { - for _, summary := range msg.ResponsesReasoning.Summary { - parts = append(parts, summary.Text) - } - } + if msg.ResponsesReasoning != nil && len(msg.ResponsesReasoning.Summary) > 0 { + for _, summary := range msg.ResponsesReasoning.Summary { + parts = append(parts, summary.Text) + } + }ui/app/logs/views/logChatMessageView.tsx (1)
12-13: Defensive guard for block.type (optional)If malformed data slips through, avoid runtime errors on replaceAll.
- <div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium capitalize">{block.type.replaceAll("_", " ")}</div> + <div className="bg-muted/50 text-muted-foreground px-6 py-2 text-xs font-medium capitalize"> + {(block.type ?? "unknown").toString().replaceAll("_", " ")} + </div>framework/streaming/types.go (1)
169-177: Avoid overwriting ExtraFields struct (optional)You set CacheDebug, then overwrite ExtraFields with a new literal (and set Latency later). Prefer field-wise assignment to avoid accidental drops if more fields are added earlier.
- if p.Data.CacheDebug != nil { - resp.ExtraFields.CacheDebug = p.Data.CacheDebug - } - resp.ExtraFields = schemas.BifrostResponseExtraFields{ - CacheDebug: p.Data.CacheDebug, - Provider: p.Provider, - } + resp.ExtraFields.Provider = p.Provider + if p.Data.CacheDebug != nil { + resp.ExtraFields.CacheDebug = p.Data.CacheDebug + }framework/streaming/responses.go (1)
288-289: Fix timestamp handling in OpenAI final path.Both
StartTimestampandEndTimestampare set totime.Now(), which makes them identical and doesn't reflect the actual stream duration. WhileLatencyis extracted fromresult.ExtraFields.Latency(which is correct), the timestamps should represent the actual start and end of the stream.Consider one of these approaches:
- Track timestamps in the accumulator even for OpenAI streams
- Calculate timestamps from latency:
EndTimestamp = time.Now()andStartTimestamp = EndTimestamp.Add(-time.Duration(result.ExtraFields.Latency) * time.Millisecond)- Extract timestamps from
result.ExtraFieldsif availableExample for approach 2:
+ now := time.Now() + startTime := now + if result.ExtraFields.Latency > 0 { + startTime = now.Add(-time.Duration(result.ExtraFields.Latency) * time.Millisecond) + } + data := &AccumulatedData{ RequestID: requestID, Status: "success", Stream: true, - StartTimestamp: time.Now(), // You might want to track this better - EndTimestamp: time.Now(), + StartTimestamp: startTime, + EndTimestamp: now, Latency: result.ExtraFields.Latency, ErrorDetails: bifrostErr, Object: result.Object, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/schemas/mux.go(1 hunks)core/schemas/providers/anthropic/responses.go(2 hunks)core/schemas/providers/bedrock/responses.go(4 hunks)core/schemas/providers/cohere/responses.go(11 hunks)framework/logstore/tables.go(5 hunks)framework/pricing/main.go(1 hunks)framework/streaming/accumulator.go(10 hunks)framework/streaming/responses.go(1 hunks)framework/streaming/types.go(4 hunks)plugins/logging/main.go(11 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/pool.go(2 hunks)plugins/logging/utils.go(2 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)ui/app/logs/views/logChatMessageView.tsx(1 hunks)ui/app/logs/views/logDetailsSheet.tsx(4 hunks)ui/app/logs/views/logMessageView.tsx(0 hunks)ui/app/logs/views/logResponsesOutputView.tsx(1 hunks)ui/lib/constants/logs.ts(2 hunks)ui/lib/types/logs.ts(3 hunks)ui/lib/utils/validation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ui/app/logs/views/logMessageView.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/pricing/main.go
- ui/lib/utils/validation.ts
- transports/bifrost-http/handlers/inference.go
- core/schemas/providers/anthropic/responses.go
🧰 Additional context used
🧬 Code graph analysis (12)
ui/app/logs/views/logDetailsSheet.tsx (2)
ui/app/logs/views/logChatMessageView.tsx (1)
LogChatMessageView(65-181)ui/app/logs/views/logResponsesOutputView.tsx (1)
LogResponsesOutputView(337-347)
plugins/logging/utils.go (5)
plugins/logging/main.go (1)
LoggerPlugin(86-100)core/schemas/bifrost.go (4)
BifrostRequest(24-36)ResponsesRequest(106-106)TextCompletionRequest(102-102)EmbeddingRequest(108-108)transports/bifrost-http/handlers/inference.go (3)
ChatRequest(165-169)ResponsesRequest(195-199)EmbeddingRequest(202-206)core/schemas/mux.go (1)
ToChatMessages(424-616)core/schemas/chatcompletions.go (3)
ChatMessageRoleUser(195-195)ChatContentBlock(280-287)ChatContentBlockTypeText(272-272)
ui/app/logs/views/logChatMessageView.tsx (3)
ui/lib/types/logs.ts (2)
ChatMessage(113-121)ContentBlock(98-109)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
framework/logstore/tables.go (2)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesReasoning(399-402)core/schemas/chatcompletions.go (2)
ChatTool(58-62)ChatAssistantMessageToolCall(339-343)
core/schemas/providers/bedrock/responses.go (3)
core/schemas/responses.go (10)
ResponsesMessageTypeMessage(256-256)ResponsesInputMessageRoleSystem(568-568)ResponsesMessageTypeFunctionCall(261-261)ResponsesMessageTypeFunctionCallOutput(262-262)ResponsesFunctionToolCallOutput(966-969)ResponsesStreamResponse(1689-1718)ResponsesStreamResponseTypeOutputItemAdded(1630-1630)ResponsesOutputMessageContentTypeText(624-624)ResponsesStreamResponseTypeContentPartAdded(1633-1633)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)core/schemas/providers/bedrock/types.go (8)
BedrockSystemMessage(54-57)BedrockMessage(48-51)BedrockMessageRole(40-40)BedrockContentBlock(60-81)BedrockToolUse(107-111)BedrockMessageRoleAssistant(44-44)BedrockToolResult(114-118)BedrockMessageRoleUser(43-43)core/schemas/bifrost.go (1)
BifrostResponse(238-254)
ui/app/logs/views/logResponsesOutputView.tsx (3)
ui/lib/types/logs.ts (2)
ResponsesMessage(405-420)ResponsesMessageContentBlock(344-379)ui/lib/utils/validation.ts (2)
isJson(468-475)cleanJson(477-487)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
framework/streaming/accumulator.go (2)
framework/streaming/types.go (3)
ResponsesStreamChunk(84-92)StreamAccumulator(95-107)ChatStreamChunk(73-81)core/schemas/bifrost.go (1)
ResponsesStreamRequest(107-107)
plugins/logging/main.go (5)
ui/lib/types/logs.ts (4)
ChatMessage(113-121)ResponsesMessage(405-420)BifrostEmbedding(123-127)BifrostError(218-224)core/schemas/chatcompletions.go (2)
ChatMessage(202-211)BifrostNonStreamResponseChoice(370-373)core/schemas/responses.go (2)
ResponsesMessage(280-292)ResponsesResponse(85-114)core/schemas/bifrost.go (1)
BifrostError(409-418)framework/streaming/types.go (1)
StreamResponseTypeFinal(24-24)
core/schemas/providers/cohere/responses.go (1)
core/schemas/responses.go (1)
ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)
framework/streaming/types.go (3)
ui/lib/types/logs.ts (3)
ResponsesMessage(405-420)LLMUsage(189-195)BifrostError(218-224)core/schemas/responses.go (2)
ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (3)
LLMUsage(296-302)BifrostCacheDebug(361-375)BifrostError(409-418)
plugins/logging/operations.go (4)
plugins/logging/main.go (2)
LoggerPlugin(86-100)UpdateLogData(42-55)core/schemas/bifrost.go (1)
BifrostCacheDebug(361-375)framework/streaming/types.go (1)
ProcessedStreamResponse(110-117)framework/logstore/tables.go (2)
Log(70-120)Log(123-125)
framework/streaming/responses.go (5)
framework/streaming/accumulator.go (1)
Accumulator(15-31)framework/streaming/types.go (7)
ResponsesStreamChunk(84-92)AccumulatedData(28-47)ProcessedStreamResponse(110-117)StreamResponseTypeFinal(24-24)StreamType(10-10)StreamTypeResponses(17-17)StreamResponseTypeDelta(23-23)core/schemas/responses.go (11)
ResponsesStreamResponseTypeOutputItemAdded(1630-1630)ResponsesStreamResponseTypeContentPartAdded(1633-1633)ResponsesStreamResponseTypeOutputTextDelta(1637-1637)ResponsesStreamResponseTypeRefusalDelta(1640-1640)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)ResponsesOutputMessageContentTypeText(624-624)ResponsesOutputMessageContentText(664-667)ResponsesOutputMessageContentTypeRefusal(625-625)ResponsesOutputMessageContentRefusal(689-691)ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostResponse(238-254)BifrostContextKeyRequestID(120-120)core/utils.go (2)
GetRequestFields(145-151)IsFinalChunk(127-142)
🔇 Additional comments (4)
plugins/logging/utils.go (1)
61-83: Go version in plugins/logging/go.mod is 1.24.
for range <int>is supported since Go 1.22; no action required.plugins/logging/pool.go (2)
20-25: LGTM: clears pooled pointers correctlyResetting UpdateData and StreamResponse pointers (with actual buffer returns elsewhere) prevents leaks; rest of field resets look good.
35-47: LGTM: UpdateLogData resets align with renamed fieldsResets cover ChatOutput, ResponsesOutput, EmbeddingOutput, Cost, etc. Consistent with new naming.
ui/app/logs/views/logDetailsSheet.tsx (1)
280-289: Confirm content accepts string — JSON.stringify(...) usage is valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/lib/types/logs.ts (1)
427-440: Consider usingunknowninstead ofanyin Record types.Several
Record<string, any>declarations (lines 433, 434, 514) useany, while line 478 correctly usesRecord<string, unknown>. For consistency and type safety, consider usingunknownthroughout.Apply this pattern consistently:
export interface ResponsesTextConfig { format?: { type: "text" | "json_schema" | "json_object"; json_schema?: { name: string; - schema: Record<string, any>; + schema: Record<string, unknown>; type: string; description?: string; strict?: boolean; }; }; verbosity?: "low" | "medium" | "high"; }export interface ResponsesPrompt { id: string; - variables: Record<string, any>; + variables: Record<string, unknown>; version?: string; }Also applies to: 511-515
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/lib/types/logs.ts(3 hunks)
🔇 Additional comments (3)
ui/lib/types/logs.ts (3)
250-250: LGTM!The new
responses_outputfield integrates cleanly with the existing LogEntry structure and aligns with the backend ResponsesOutput support.
305-546: Comprehensive Responses API type system.The new type definitions provide thorough coverage of the Responses API, including message structures, content blocks, tool configurations, reasoning, and usage tracking. The naming conventions are consistent and the structure will support rich UI rendering.
495-502: Proper JSDoc @deprecated implementation.The deprecation of
generate_summaryis now correctly expressed using JSDoc@deprecatedannotation, which will surface warnings in IDE tooling and provide clear guidance to usesummaryinstead.
Pratham-Mishra04
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
a6bdc87 to
e3b13ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
ui/lib/types/logs.ts (1)
390-390: The[key: string]: anyindex signatures still present.Despite being flagged in previous reviews, the index signatures with
anyremain at lines 390, 419, and 491. These defeat TypeScript's type checking and should be replaced withunknownfor type safety.Apply this diff to fix the type safety issue:
export interface ResponsesToolMessage { call_id?: string; name?: string; arguments?: string; - [key: string]: any; + [key: string]: unknown; }export interface ResponsesMessage { id?: string; type?: ResponsesMessageType; status?: string; role?: ResponsesMessageRoleType; content?: ResponsesMessageContent; call_id?: string; name?: string; arguments?: string; summary?: ResponsesReasoningContent[]; encrypted_content?: string; - [key: string]: any; + [key: string]: unknown; }export interface ResponsesTool { type: string; name?: string; description?: string; function?: ResponsesToolFunction; - [key: string]: any; + [key: string]: unknown; }Also applies to: 419-419, 491-491
framework/streaming/responses.go (3)
93-98: Sparse indices drop messages.The loop
for i := 0; i < len(messageMap); i++assumes keys are contiguous 0,1,2,... but if the stream provides indices {0, 2, 4}, only the first three iterations run (i=0,1,2), fetching messageMap[0] and messageMap[2] while dropping messageMap[4]. Collect all keys, sort numerically, and iterate that slice to preserve the exact sequence.Apply this diff:
- // Convert map to ordered slice - for i := 0; i < len(messageMap); i++ { - if msg, exists := messageMap[i]; exists { - completeMessages = append(completeMessages, *msg) + // Collect keys, sort, and iterate to handle sparse indices + keys := make([]int, 0, len(messageMap)) + for k := range messageMap { + keys = append(keys, k) + } + sort.Ints(keys) + for _, k := range keys { + if msg := messageMap[k]; msg != nil { + completeMessages = append(completeMessages, *msg) } }Add import:
import ( "context" "fmt" + "sort" "time"
324-332: Set Data.Model for logging.
updateStreamingLogEntryinplugins/logging/operations.goreadsstreamResponse.Data.Modelto persist the model. Without settingdata.Model = model, the model field won't be stored in logs.Apply this diff:
} + data.Model = model + return &ProcessedStreamResponse{ Type: StreamResponseTypeFinal, RequestID: requestID, StreamType: StreamTypeResponses, Provider: provider, Model: model, Data: data, }, nil
408-415: Set Data.Model for logging.Same issue as the OpenAI path:
data.Modelis never assigned, so the logging plugin cannot persist the model.Apply this diff:
if shouldProcess { data, processErr := a.processAccumulatedResponsesStreamingChunks(requestID, bifrostErr, isFinalChunk) if processErr != nil { a.logger.Error("failed to process accumulated responses chunks for request %s: %v", requestID, processErr) return nil, processErr } + data.Model = model + return &ProcessedStreamResponse{ Type: StreamResponseTypeFinal, RequestID: requestID, StreamType: StreamTypeResponses, Provider: provider, Model: model, Data: data, }, nil }
🧹 Nitpick comments (2)
ui/lib/types/logs.ts (1)
405-420: Consider discriminated unions for type safety.The
ResponsesMessageinterface merges fields from different message types (tool messages, reasoning messages, etc.) based on thetypefield. While this works, a discriminated union approach would provide better type safety and autocompletion. However, this would require a larger refactor and may not be worth it if the current approach works for your use case.Example discriminated union pattern (if you decide to refactor):
type ResponsesMessage = | ResponsesToolMessage | ResponsesReasoningMessage | ResponsesContentMessage // ... other message types // Where each type has a discriminant: interface ResponsesToolMessage { type: "function_call" | "custom_tool_call" | /* other tool types */; call_id: string; name?: string; arguments?: string; // ... }framework/streaming/responses.go (1)
115-117: Resize slice once instead of appending in a loop.Growing the slice one element at a time is inefficient when
contentIndexis large. Resize tocontentIndex+1in one operation.Apply this diff:
- // Ensure we have enough content blocks - for len(message.Content.ContentBlocks) <= contentIndex { - message.Content.ContentBlocks = append(message.Content.ContentBlocks, schemas.ResponsesMessageContentBlock{}) + // Resize to required length if needed + if len(message.Content.ContentBlocks) <= contentIndex { + newBlocks := make([]schemas.ResponsesMessageContentBlock, contentIndex+1) + copy(newBlocks, message.Content.ContentBlocks) + message.Content.ContentBlocks = newBlocks }Also applies to: 145-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
framework/streaming/responses.go(1 hunks)ui/lib/types/logs.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framework/streaming/responses.go (5)
framework/streaming/accumulator.go (1)
Accumulator(15-31)framework/streaming/types.go (7)
ResponsesStreamChunk(84-92)AccumulatedData(28-47)ProcessedStreamResponse(110-117)StreamResponseTypeFinal(24-24)StreamType(10-10)StreamTypeResponses(17-17)StreamResponseTypeDelta(23-23)core/schemas/responses.go (11)
ResponsesStreamResponseTypeOutputItemAdded(1630-1630)ResponsesStreamResponseTypeContentPartAdded(1633-1633)ResponsesStreamResponseTypeOutputTextDelta(1637-1637)ResponsesStreamResponseTypeRefusalDelta(1640-1640)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)ResponsesOutputMessageContentTypeText(624-624)ResponsesOutputMessageContentText(664-667)ResponsesOutputMessageContentTypeRefusal(625-625)ResponsesOutputMessageContentRefusal(689-691)ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostResponse(238-254)BifrostContextKeyRequestID(120-120)core/utils.go (2)
GetRequestFields(145-151)IsFinalChunk(127-142)
TejasGhatte
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
e3b13ec to
f376605
Compare
TejasGhatte
force-pushed
the
10-07-feat_responses_added_to_logs
branch
from
f376605 to
5214fd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
ui/lib/types/logs.ts (3)
385-391: Index signature still usesanyinstead ofunknown.The
[key: string]: anyindex signature was previously flagged as a type safety issue. Usinganybypasses TypeScript's type checking and can lead to runtime errors. Consider changing to[key: string]: unknownto maintain type safety while preserving flexibility.
405-420: Index signature still usesanyinstead ofunknown.The
[key: string]: anyindex signature inResponsesMessagewas previously flagged as a type safety issue. This defeats TypeScript's type checking. Consider changing to[key: string]: unknownto require explicit type narrowing.
484-492: Index signature still usesanyinstead ofunknown.The
[key: string]: anyindex signature inResponsesToolwas previously flagged as a type safety issue. Consider changing to[key: string]: unknownto maintain type safety.framework/streaming/responses.go (2)
189-203: SetData.Modelto persist model in logs.
AccumulatedDatais initialized without setting theModelfield. Based on the codebase context, the logging plugin readsstreamResponse.Data.Modelto persist the model. Without this assignment, the model won't be stored in logs.To fix this, modify the function signature to accept the model as a parameter and set it:
-func (a *Accumulator) processAccumulatedResponsesStreamingChunks(requestID string, respErr *schemas.BifrostError, isFinalChunk bool) (*AccumulatedData, error) { +func (a *Accumulator) processAccumulatedResponsesStreamingChunks(requestID string, respErr *schemas.BifrostError, isFinalChunk bool, model string) (*AccumulatedData, error) { accumulator := a.getOrCreateStreamAccumulator(requestID) // Lock the accumulator accumulator.mu.Lock() ... // Initialize accumulated data data := &AccumulatedData{ RequestID: requestID, + Model: model, Status: "success", ... }Then update the callers:
- Line 407: Pass
modelparameter- Line 294-303 (OpenAI path): Set
data.Model = modelafter creatingAccumulatedData
407-407: Pass model parameter to enable logging persistence.Once
processAccumulatedResponsesStreamingChunksis updated to accept amodelparameter (see comment on lines 189-203), update this call to pass it:- data, processErr := a.processAccumulatedResponsesStreamingChunks(requestID, bifrostErr, isFinalChunk) + data, processErr := a.processAccumulatedResponsesStreamingChunks(requestID, bifrostErr, isFinalChunk, model)
🧹 Nitpick comments (2)
ui/lib/types/logs.ts (2)
428-440: Consider usingunknownin JSON schema Record type.Line 433 uses
Record<string, any>for the JSON schema. This bypasses type checking and can lead to runtime errors. Consider usingRecord<string, unknown>to require explicit type narrowing when accessing schema properties.Apply this diff:
export interface ResponsesTextConfig { format?: { type: "text" | "json_schema" | "json_object"; json_schema?: { name: string; - schema: Record<string, any>; + schema: Record<string, unknown>; type: string; description?: string; strict?: boolean; }; }; verbosity?: "low" | "medium" | "high"; }
511-515: Consider usingunknownin prompt variables Record type.Line 514 uses
Record<string, any>for prompt variables. This bypasses type checking and can lead to runtime errors. Consider usingRecord<string, unknown>to require explicit type narrowing when accessing variable values.Apply this diff:
export interface ResponsesPrompt { id: string; - variables: Record<string, any>; + variables: Record<string, unknown>; version?: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/providers/utils.go(1 hunks)core/schemas/mux.go(2 hunks)framework/streaming/responses.go(1 hunks)ui/lib/types/logs.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/mux.go
🧰 Additional context used
🧬 Code graph analysis (2)
framework/streaming/responses.go (5)
framework/streaming/accumulator.go (1)
Accumulator(15-31)framework/streaming/types.go (7)
ResponsesStreamChunk(84-92)AccumulatedData(28-47)ProcessedStreamResponse(110-117)StreamResponseTypeFinal(24-24)StreamType(10-10)StreamTypeResponses(17-17)StreamResponseTypeDelta(23-23)core/schemas/responses.go (13)
ResponsesStreamResponseTypeOutputItemAdded(1630-1630)ResponsesStreamResponseTypeContentPartAdded(1633-1633)ResponsesStreamResponseTypeOutputTextDelta(1637-1637)ResponsesStreamResponseTypeRefusalDelta(1640-1640)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1644-1644)ResponsesMessageTypeMessage(256-256)ResponsesInputMessageRoleAssistant(566-566)ResponsesOutputMessageContentTypeText(624-624)ResponsesOutputMessageContentText(664-667)ResponsesOutputMessageContentTypeRefusal(625-625)ResponsesOutputMessageContentRefusal(689-691)ResponsesStreamResponse(1689-1718)ResponsesResponse(85-114)core/schemas/bifrost.go (2)
BifrostResponse(238-254)BifrostContextKeyRequestID(120-120)core/utils.go (2)
GetRequestFields(145-151)IsFinalChunk(127-142)
core/providers/utils.go (1)
core/schemas/bifrost.go (2)
RequestType(99-99)ResponsesStreamRequest(107-107)
🔇 Additional comments (5)
ui/lib/types/logs.ts (4)
250-250: LGTM! Responses API integration point added.The
responses_outputfield is well-positioned and appropriately typed as an optional array to support the new Responses API logging without breaking existing code.
305-342: LGTM! Comprehensive Responses API type definitions.The role, message, and content block type definitions provide comprehensive coverage for the Responses API with proper TypeScript literal unions for type safety.
344-379: LGTM! Well-structured content block interface.The
ResponsesMessageContentBlockinterface effectively handles polymorphic content types with appropriate optional fields and nested structures for complex data like annotations and logprobs.
518-546: LGTM! Usage and error types are well-structured.The usage tracking interfaces properly model token details with appropriate nesting, and the error/incomplete details structures are clear and concise.
core/providers/utils.go (1)
588-596: LGTM! Consistent error tagging for responses streams.Unconditionally setting
RequestTypetoResponsesStreamRequestfor errors ensures downstream handlers (especially logging) correctly identify the error origin. Since this converter is specifically for responses streams, marking all errors accordingly is appropriate.
Merge activity
|
3 participants
Summary
This PR adds support for the Responses API output format in the logging system and improves the UI for displaying log entries. It also changes the default log level from debug to info and fixes several issues in the logging plugin.
Changes
ResponsesOutputfield to the Log model to store Responses API outputType of change
Affected areas
How to test
Breaking changes
Related issues
Improves logging functionality for Responses API output format.
Security considerations
No security implications.
Checklist