feat: add realtime turn logging to logging plugin#2339
Merged
Conversation
Collaborator
Author
This was referenced Mar 27, 2026
|
|
This was referenced Mar 27, 2026
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughPre/Post LLM hooks and logging were extended to recognize realtime requests, merge realtime metadata and session/parent IDs, bypass standard streaming/tracing for realtime, and route realtime results to new processors that backfill transcripts, rebuild input history, extract usage, and expose session retrieval APIs. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PreLLM as Pre-LLM Hook
participant LLM
participant PostLLM as Post-LLM Hook
participant RealtimeProc as Realtime Processor
participant LogStore
Client->>PreLLM: send request (realtime)
PreLLM->>PreLLM: mark realtime, set object/params, merge metadata, derive parent/session IDs
PreLLM-->>LLM: forward request
LLM-->>PostLLM: return result
PostLLM->>PostLLM: detect realtime, set pending parent ID, merge metadata
PostLLM->>RealtimeProc: applyRealtimeOutputToEntry(entry, result)
RealtimeProc->>RealtimeProc: extract transcripts, normalize raw request/response, rebuild input history, set usage
RealtimeProc-->>PostLLM: enriched entry
PostLLM->>LogStore: persist enriched log entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
18 tasks
Contributor
Confidence Score: 5/5Safe to merge — no blocking issues found All prior P0/P1 concerns from previous review threads are resolved; the one remaining observation is a P2 style note on fallthrough behavior in collectRealtimeRawTextFragments that does not affect correctness for the realtime protocol in practice No files require special attention; plugins/logging/operations.go has one minor style note Important Files Changed
Reviews (15): Last reviewed commit: "feat: add realtime turn logging to loggi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plugins/logging/operations.go (1)
808-817: Consider populatingChatToolMessage.ToolCallIDfor tool outputs.The
ChatToolMessageis initialized as an empty struct, but tool outputs typically have acall_idthat links them to the original function call. This could help with log correlation.♻️ Suggested enhancement
case event.Item.Type == "function_call_output": if content := extractRealtimeRawItemContent(event.Item); content != "" { + var toolCallID *string + if event.Item.CallID != "" { + toolCallID = schemas.Ptr(event.Item.CallID) + } messages = append(messages, schemas.ChatMessage{ Role: schemas.ChatMessageRoleTool, Content: &schemas.ChatMessageContent{ ContentStr: schemas.Ptr(content), }, - ChatToolMessage: &schemas.ChatToolMessage{}, + ChatToolMessage: &schemas.ChatToolMessage{ + ToolCallID: toolCallID, + }, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/operations.go` around lines 808 - 817, The ChatToolMessage is being created empty when handling event.Item.Type == "function_call_output"; populate ChatToolMessage.ToolCallID with the call identifier from the event item (e.g., event.Item.CallID or event.Item.CallId) so the appended schemas.ChatMessage links to the original function call; update the messages append block (where extractRealtimeRawItemContent is used and a schemas.ChatMessage is constructed) to set ChatToolMessage.ToolCallID = &callID (or appropriate pointer) after extracting the call id from event.Item and handle missing/empty call ids gracefully.plugins/logging/utils.go (1)
533-541: Redundant condition in content block extraction.Lines 537-538 check
block.ResponsesOutputMessageContentText != nil && block.Text != nil, butblock.Textwas already checked and appended on lines 535-536. This case will never be reached.♻️ Suggested fix
for _, block := range content.ContentBlocks { switch { case block.Text != nil && strings.TrimSpace(*block.Text) != "": parts = append(parts, strings.TrimSpace(*block.Text)) - case block.ResponsesOutputMessageContentText != nil && block.Text != nil && strings.TrimSpace(*block.Text) != "": - parts = append(parts, strings.TrimSpace(*block.Text)) case block.ResponsesOutputMessageContentRefusal != nil && strings.TrimSpace(block.Refusal) != "": parts = append(parts, strings.TrimSpace(block.Refusal)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/utils.go` around lines 533 - 541, The second switch case is redundant because it checks block.Text again; update the logic in the loop over content.ContentBlocks to either remove that case or, if you intended to handle the nested ResponsesOutputMessageContentText payload, change the condition to check block.ResponsesOutputMessageContentText's own text field (e.g., block.ResponsesOutputMessageContentText.Text != nil && strings.TrimSpace(*block.ResponsesOutputMessageContentText.Text) != "") and append the trimmed value; ensure you still keep the first case for block.Text and the refusal case for block.ResponsesOutputMessageContentRefusal.plugins/logging/operations_test.go (1)
315-337: Useschemas.Ptr()instead of the address operator for consistency.Per repository conventions, prefer
schemas.Ptr()(orbifrost.Ptr()) over&for creating pointers to values. This applies toassistantText,messageType, andassistantRole.♻️ Suggested refactor
func TestApplyRealtimeOutputToEntryBackfillsUserTranscriptFromRawRequest(t *testing.T) { plugin := &LoggerPlugin{} entry := &logstore.Log{} - assistantText := "Hello!" - messageType := schemas.ResponsesMessageTypeMessage - assistantRole := schemas.ResponsesInputMessageRoleAssistant result := &schemas.BifrostResponse{ ResponsesResponse: &schemas.BifrostResponsesResponse{ Output: []schemas.ResponsesMessage{{ - Type: &messageType, - Role: &assistantRole, + Type: schemas.Ptr(schemas.ResponsesMessageTypeMessage), + Role: schemas.Ptr(schemas.ResponsesInputMessageRoleAssistant), Content: &schemas.ResponsesMessageContent{ - ContentStr: &assistantText, + ContentStr: schemas.Ptr("Hello!"), }, }},Based on learnings: "In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/operations_test.go` around lines 315 - 337, Replace direct &-addressing with the repository pointer helper in the test: for the values used in TestApplyRealtimeOutputToEntryBackfillsUserTranscriptFromRawRequest (assistantText, messageType, assistantRole) call schemas.Ptr(...) to create pointers instead of using &assistantText, &messageType, &assistantRole so the ResponsesMessage fields use schemas.Ptr(...) consistently; update the instantiation inside the result variable where ResponsesMessage.Content.ContentStr and ResponsesMessage.Type/Role are assigned to use schemas.Ptr(...) calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/logging/operations_test.go`:
- Around line 315-337: Replace direct &-addressing with the repository pointer
helper in the test: for the values used in
TestApplyRealtimeOutputToEntryBackfillsUserTranscriptFromRawRequest
(assistantText, messageType, assistantRole) call schemas.Ptr(...) to create
pointers instead of using &assistantText, &messageType, &assistantRole so the
ResponsesMessage fields use schemas.Ptr(...) consistently; update the
instantiation inside the result variable where
ResponsesMessage.Content.ContentStr and ResponsesMessage.Type/Role are assigned
to use schemas.Ptr(...) calls.
In `@plugins/logging/operations.go`:
- Around line 808-817: The ChatToolMessage is being created empty when handling
event.Item.Type == "function_call_output"; populate ChatToolMessage.ToolCallID
with the call identifier from the event item (e.g., event.Item.CallID or
event.Item.CallId) so the appended schemas.ChatMessage links to the original
function call; update the messages append block (where
extractRealtimeRawItemContent is used and a schemas.ChatMessage is constructed)
to set ChatToolMessage.ToolCallID = &callID (or appropriate pointer) after
extracting the call id from event.Item and handle missing/empty call ids
gracefully.
In `@plugins/logging/utils.go`:
- Around line 533-541: The second switch case is redundant because it checks
block.Text again; update the logic in the loop over content.ContentBlocks to
either remove that case or, if you intended to handle the nested
ResponsesOutputMessageContentText payload, change the condition to check
block.ResponsesOutputMessageContentText's own text field (e.g.,
block.ResponsesOutputMessageContentText.Text != nil &&
strings.TrimSpace(*block.ResponsesOutputMessageContentText.Text) != "") and
append the trimmed value; ensure you still keep the first case for block.Text
and the refusal case for block.ResponsesOutputMessageContentRefusal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ddb52e6-7bc3-4543-a29d-5ea46f02ab71
📒 Files selected for processing (4)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.goplugins/logging/utils.go
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
81edf1b to
f9bb3b3
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
2f86e4c to
16d59fd
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/logging/main.go (1)
754-756: Double merge of realtime metadata is intentional.The metadata is merged once in
PreLLMHook(line 584) and again here inPostLLMHook. This appears intentional to capture realtime context keys (e.g., provider session ID) that may only be available after the provider response. Consider adding a brief comment clarifying this design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/main.go` around lines 754 - 756, Add a short clarifying comment near the second merge of realtime metadata to indicate the double-merge is intentional: explain that metadata is first merged in PreLLMHook and then merged again in PostLLMHook to capture realtime provider-specific keys (e.g., provider session ID) that become available only after the provider response; annotate the code around entry.MetadataParsed = mergeRealtimeMetadata(entry.MetadataParsed, ctx) and reference the PreLLMHook/PostLLMHook merge points so future readers understand the design and avoid accidental removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/logging/main.go`:
- Around line 754-756: Add a short clarifying comment near the second merge of
realtime metadata to indicate the double-merge is intentional: explain that
metadata is first merged in PreLLMHook and then merged again in PostLLMHook to
capture realtime provider-specific keys (e.g., provider session ID) that become
available only after the provider response; annotate the code around
entry.MetadataParsed = mergeRealtimeMetadata(entry.MetadataParsed, ctx) and
reference the PreLLMHook/PostLLMHook merge points so future readers understand
the design and avoid accidental removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fa40d46-3a0f-4923-aea2-36e470c6310d
📒 Files selected for processing (4)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.goplugins/logging/utils.go
✅ Files skipped from review due to trivial changes (1)
- plugins/logging/operations_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/logging/utils.go
- plugins/logging/operations.go
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
16d59fd to
edba174
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
f9bb3b3 to
40f5251
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/logging/utils.go (1)
486-491: Consider usingbifrost.Ptr()for pointer creation.The code uses
schemas.Ptr(content)to create string pointers. Based on repository learnings,bifrost.Ptr()is the preferred helper for creating pointers across the codebase.However, if
schemas.Ptris an alias or equivalent implementation, this is acceptable. Please verify consistency with other files in this package.Also applies to: 500-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/utils.go` around lines 486 - 491, The code creates string pointers using schemas.Ptr(content) when building schemas.ChatMessageContent inside the messages append; replace schemas.Ptr(...) with bifrost.Ptr(...) for consistency across the codebase and update imports to include the bifrost package, or alternatively confirm that schemas.Ptr is an exact alias of bifrost.Ptr and document/replace other occurrences (notably the similar blocks around the schemas.ChatMessageContent construction at lines ~500-506) to keep pointer helpers consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/logging/operations.go`:
- Around line 727-770: applyRealtimeOutputToEntry extracts
RawRequest/RawResponse regardless of p.disableContentLogging; add the same
content-logging gate used in applyNonStreamingOutputToEntry so raw
request/response are only populated when content logging is allowed (i.e., check
if p.disableContentLogging == nil || !*p.disableContentLogging before handling
extraFields.RawRequest and extraFields.RawResponse and before calling
extractRealtimeInputHistoryFromRawRequest); update references inside
applyRealtimeOutputToEntry to only set entry.RawRequest,
entry.InputHistoryParsed, and entry.RawResponse when the gate permits.
---
Nitpick comments:
In `@plugins/logging/utils.go`:
- Around line 486-491: The code creates string pointers using
schemas.Ptr(content) when building schemas.ChatMessageContent inside the
messages append; replace schemas.Ptr(...) with bifrost.Ptr(...) for consistency
across the codebase and update imports to include the bifrost package, or
alternatively confirm that schemas.Ptr is an exact alias of bifrost.Ptr and
document/replace other occurrences (notably the similar blocks around the
schemas.ChatMessageContent construction at lines ~500-506) to keep pointer
helpers consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e32eb5f3-9082-4a61-a2e2-023feb300314
📒 Files selected for processing (4)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.goplugins/logging/utils.go
✅ Files skipped from review due to trivial changes (1)
- plugins/logging/operations_test.go
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
edba174 to
a3e85c4
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
40f5251 to
44d1515
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/logging/utils.go (1)
639-648: Minor: Duplicate context lookup for realtime session ID.The
setfunction at line 639 already callsbifrost.GetStringFromContext(ctx, schemas.BifrostContextKeyRealtimeSessionID), but line 643 performs the same lookup again. Consider capturing the value once to avoid the redundant call.♻️ Suggested optimization
set("realtime_session_id", schemas.BifrostContextKeyRealtimeSessionID) set("provider_session_id", schemas.BifrostContextKeyRealtimeProviderSessionID) set("realtime_source", schemas.BifrostContextKeyRealtimeSource) set("realtime_event_type", schemas.BifrostContextKeyRealtimeEventType) - if bifrost.GetStringFromContext(ctx, schemas.BifrostContextKeyRealtimeSessionID) != "" { + if metadata != nil && metadata["realtime_session_id"] != nil { if metadata == nil { metadata = make(map[string]interface{}) } metadata["realtime"] = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/utils.go` around lines 639 - 648, The code redundantly calls bifrost.GetStringFromContext(ctx, schemas.BifrostContextKeyRealtimeSessionID) twice (once inside set and again in the if); capture the realtime session ID once into a local variable (e.g., realtimeID) before the set calls and reuse that variable in the conditional that sets metadata["realtime"]=true, updating references around the set(...) invocations (functions: set, bifrost.GetStringFromContext, constant: schemas.BifrostContextKeyRealtimeSessionID, variable: metadata) to avoid the duplicate lookup while keeping identical behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/logging/utils.go`:
- Around line 500-506: The ChatToolMessage is being created without copying the
originating tool call ID, so update the code that builds the tool message (in
extractRealtimeToolOutputContent where messages = append(messages,
schemas.ChatMessage{...}) and the similar site referenced in operations.go
around the ChatMessage construction) to populate ChatToolMessage.ToolCallID from
the input ResponsesToolMessage.CallID (e.g., set ChatToolMessage:
&schemas.ChatToolMessage{ToolCallID: item.ResponsesToolMessage.CallID}); ensure
you handle a nil ResponsesToolMessage gracefully before accessing CallID to
avoid panics.
---
Nitpick comments:
In `@plugins/logging/utils.go`:
- Around line 639-648: The code redundantly calls
bifrost.GetStringFromContext(ctx, schemas.BifrostContextKeyRealtimeSessionID)
twice (once inside set and again in the if); capture the realtime session ID
once into a local variable (e.g., realtimeID) before the set calls and reuse
that variable in the conditional that sets metadata["realtime"]=true, updating
references around the set(...) invocations (functions: set,
bifrost.GetStringFromContext, constant:
schemas.BifrostContextKeyRealtimeSessionID, variable: metadata) to avoid the
duplicate lookup while keeping identical behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8aef1e5e-175f-448d-bd99-893d82555d3a
📒 Files selected for processing (4)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.goplugins/logging/utils.go
✅ Files skipped from review due to trivial changes (1)
- plugins/logging/operations_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/logging/operations.go
- plugins/logging/main.go
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
a3e85c4 to
78a4b1f
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
44d1515 to
3062e94
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/logging/utils.go`:
- Around line 531-543: The current helper returns ContentStr when ContentStr is
non-nil even if it trims to empty, which prevents falling back to content
blocks; change the guard in the function that reads ContentStr to require
non-empty trimmed content (e.g., if content.ContentStr != nil &&
strings.TrimSpace(*content.ContentStr) != "" { ... }) so that when ContentStr is
allocated but empty the code continues to assemble parts from ContentBlocks;
make the same change for
ResponsesToolCallOutputStr/ResponsesOutputMessageContentRefusal code paths
(ensure you check trimmed string != "" before returning and only then skip block
fallback).
- Around line 139-146: In GetSessionLogs (PluginLogManager) the sessionID is
trimmed only for validation but the original value is forwarded; trim/normalize
sessionID (e.g., strings.TrimSpace(sessionID)) into a local variable after
validation and pass that normalized value to p.plugin.GetSessionLogs(ctx,
normalizedSessionID, *pagination) so callers like " abc " resolve to the correct
session key while keeping the existing pagination nil check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31fe7a48-4588-4802-8b12-a53d4f272c19
📒 Files selected for processing (4)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.goplugins/logging/utils.go
✅ Files skipped from review due to trivial changes (1)
- plugins/logging/operations.go
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/logging/main.go
- plugins/logging/operations_test.go
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
3062e94 to
abc9e04
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
2 times, most recently
from
51db921 to
c180d40
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
c180d40 to
aee487f
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
5cc0b46 to
79fd370
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/logging/main.go`:
- Around line 740-744: The code reads BifrostContextKeyRealtimeSessionID from
context (via bifrost.GetStringFromContext) to set pending.ParentRequestID for
realtime requests, but that context key is never set; update the realtime
request entry path to set the context key using the session ID from OpenAI
realtime events (bifrostEvent.Session.ID) before the code that checks it, or
remove the retrieval. Specifically, in the realtime event processing flow where
bifrostEvent is available, set the context value for
BifrostContextKeyRealtimeSessionID on the request context so
bifrost.GetStringFromContext returns bifrostEvent.Session.ID, ensuring
pending.ParentRequestID is populated for realtime requests (or delete the unused
lookup if you prefer to not link sessions).
- Around line 602-610: The code reads
ctx.Value(schemas.BifrostContextKeyParentRequestID) into parentRequestID but no
upstream code ever sets that key—upstream only sets
schemas.BifrostMCPAgentOriginalRequestID for the x-bf-parent-request-id
header—so either populate the missing context key or remove the dead check:
update the header handling code that currently sets
BifrostMCPAgentOriginalRequestID to also set BifrostContextKeyParentRequestID
(mirroring the same value) so parentRequestID is populated, or remove the
BifrostContextKeyParentRequestID lookup in the logging plugin (and use
BifrostMCPAgentOriginalRequestID consistently) and adjust references to
parentRequestID accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13df2605-ecaf-4f22-beb8-257b6f6b593c
📒 Files selected for processing (5)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.goplugins/logging/utils.goplugins/maxim/main.go
✅ Files skipped from review due to trivial changes (1)
- plugins/logging/operations_test.go
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
aee487f to
98d9691
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
79fd370 to
5c9625f
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
98d9691 to
480d94e
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
5c9625f to
9f6e663
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/logging/operations.go`:
- Around line 760-763: The current guard in the if block that only runs when
len(entry.InputHistoryParsed) == 0 causes transcript-derived items from
extractRealtimeInputHistoryFromRawRequest(entry.RawRequest) to be skipped
whenever any input history already exists; change this to always attempt to
merge the transcript into entry.InputHistoryParsed by introducing a helper
(e.g., mergeRealtimeInputHistory) that takes the existing
entry.InputHistoryParsed and the inputHistory from
extractRealtimeInputHistoryFromRawRequest and appends new items while
deduplicating against existing ResponsesRequest.Input-derived entries; replace
the len(...) == 0 check with a call to that helper when RawRequest is non-empty
so RawRequest-derived placeholders are merged rather than skipped.
- Around line 880-887: The loop over decoded parts in
extractRealtimeTranscript()/the raw content extractor currently treats a
transcript as missing only when the "transcript" key is absent or nil, so empty
strings ("") slip through and cause user audio items to disappear; update the
check inside the loop that examines part["transcript"] (and the surrounding
logic where decoded/part are handled) to treat empty-string transcripts as
missing as well—e.g., coerce part["transcript"] to a string, trim and test for
zero length or explicitly check for ""—so the code returns true (or falls back
to the unavailable placeholder) when transcript == "" in addition to nil/absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d660609b-190b-4cf6-bbbf-bfd17747c6f5
📒 Files selected for processing (5)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.goplugins/logging/utils.goplugins/maxim/main.go
✅ Files skipped from review due to trivial changes (1)
- plugins/logging/operations_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/maxim/main.go
- plugins/logging/main.go
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
9f6e663 to
aec9dc0
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
480d94e to
4d142a4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/logging/main.go (1)
687-701:⚠️ Potential issue | 🟠 MajorExclude realtime from the PreLLMHook stream accumulator too.
These guards stop
PostLLMHookfrom ever reading or cleaning tracer state forschemas.RealtimeRequest, but Line 438 still callsCreateStreamAccumulator(...)for any request type thatbifrost.IsStreamRequestTypetreats as streaming. That leaves an unused accumulator behind for each realtime turn.♻️ Suggested follow-up
- if bifrost.IsStreamRequestType(req.RequestType) && req.RequestType != schemas.PassthroughStreamRequest { + if bifrost.IsStreamRequestType(req.RequestType) && + req.RequestType != schemas.PassthroughStreamRequest && + req.RequestType != schemas.RealtimeRequest { tracer, traceID, err := bifrost.GetTracerFromContext(ctx) if err == nil && tracer != nil && traceID != "" { tracer.CreateStreamAccumulator(traceID, createdTimestamp) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/main.go` around lines 687 - 701, PreLLMHook is creating a stream accumulator for any bifrost.IsStreamRequestType request but doesn't exclude schemas.RealtimeRequest, leaving unused accumulators; update the PreLLMHook code path that calls CreateStreamAccumulator(...) to only create an accumulator when requestType is a stream AND requestType != schemas.RealtimeRequest (mirror the guard used in PostLLMHook), i.e., add the same requestType != schemas.RealtimeRequest check around the CreateStreamAccumulator call so realtime requests skip accumulator creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/logging/utils.go`:
- Around line 31-35: The change added two methods GetSessionLogs and
GetSessionSummary to the exported LogManager interface which is a breaking API
for downstream implementers; to fix this, revert adding them directly to
logging.LogManager and instead introduce a new interface (e.g.,
SessionLogManager) that declares GetSessionLogs and GetSessionSummary, or
clearly document this as a breaking change if you intend to keep them on
LogManager; update usages to accept the narrower SessionLogManager where
session-level operations are required (refer to the LogManager interface and the
new SessionLogManager, plus the methods GetSessionLogs and GetSessionSummary) so
existing implementers are not forced to change.
---
Outside diff comments:
In `@plugins/logging/main.go`:
- Around line 687-701: PreLLMHook is creating a stream accumulator for any
bifrost.IsStreamRequestType request but doesn't exclude schemas.RealtimeRequest,
leaving unused accumulators; update the PreLLMHook code path that calls
CreateStreamAccumulator(...) to only create an accumulator when requestType is a
stream AND requestType != schemas.RealtimeRequest (mirror the guard used in
PostLLMHook), i.e., add the same requestType != schemas.RealtimeRequest check
around the CreateStreamAccumulator call so realtime requests skip accumulator
creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca5a4f57-b320-4f1b-86e1-d52f89cc0964
📒 Files selected for processing (5)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.goplugins/logging/utils.goplugins/maxim/main.go
✅ Files skipped from review due to trivial changes (2)
- plugins/maxim/main.go
- plugins/logging/operations.go
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
aec9dc0 to
702f62b
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
4d142a4 to
3048cdf
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
3048cdf to
acc3988
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
702f62b to
06d48bd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/logging/operations.go`:
- Around line 768-785: applyRealtimeRawRequestBackfill currently marshals
non-string rawRequest values into a single JSON blob which breaks
extractRealtimeInputHistoryFromRawRequest (it expects blank-line-delimited
single-event JSON strings); change applyRealtimeRawRequestBackfill to normalize
structured inputs first: if rawRequest is []byte decode to string; if it's a
slice/array iterate elements and for each element produce an event string (if
element is string use TrimSpace, if element is non-string marshal to JSON) then
join events with "\n\n"; if rawRequest is a map/struct marshal to a single-event
JSON string (not a quoted blob) and only call
extractRealtimeInputHistoryFromRawRequest when the resulting string is non-empty
and in the expected per-event wire format; reference functions
applyRealtimeRawRequestBackfill, extractRealtimeInputHistoryFromRawRequest,
mergeRealtimeInputHistory to locate and update the logic.
- Around line 842-850: The function mergeRealtimeInputHistory short-circuits
when existing is empty which bypasses realtimeInputHistoryContainsEquivalent and
allows duplicates inside a single backfill blob; change the logic in
mergeRealtimeInputHistory so you always run the de-duplication routine:
initialize merged from existing (or empty) and then iterate over backfill items,
calling realtimeInputHistoryContainsEquivalent(merged, item) for each backfill
entry and only append items that are not equivalent; this ensures dedupe runs
even when existing is empty while keeping the existing merge behavior for
non-empty histories.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17f233e1-398d-4460-ae63-9969d1c17afb
📒 Files selected for processing (5)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.goplugins/logging/utils.goplugins/maxim/main.go
✅ Files skipped from review due to trivial changes (1)
- plugins/maxim/main.go
danpiths
force-pushed
the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
from
06d48bd to
b3aac17
Compare
danpiths
force-pushed
the
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
branch
from
acc3988 to
95e9aa6
Compare
Contributor
Merge activity
|
akshaydeo
changed the base branch from
feature/03-27-feat_add_session_log_storage_and_realtime_request_normalization
to
graphite-base/2339
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
akshaydeo
deleted the
feature/03-27-feat_add_realtime_turn_logging_to_logging_plugin
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends the logging plugin to handle realtime turn requests. Adds
realtime-specific pre/post hook handling, audio transcript backfill from raw
request events, output message extraction, session log delegation, realtime
metadata merging, and session summary polling. Also adds a realtime skip guard
to the Maxim observability plugin.
Changes
PreLLMHookto detectRealtimeRequestand setobject = "realtime.turn", extract params fromResponsesRequestPostLLMHookto setParentRequestIDfrom realtime session context,route to
applyRealtimeOutputToEntryapplyRealtimeOutputToEntryfor extracting usage, output message, andraw request/response from realtime turns
extractRealtimeInputHistoryFromRawRequestfor rebuilding chat messagehistory from raw events (audio transcripts, conversation items)
extractRealtimeOutputMessagefor buildingChatMessagefromResponsesMessagelistmergeRealtimeMetadatafor propagating realtime context keys to logmetadata
GetSessionLogsdelegation to logstore with pagination defaultsGetSessionSummarydelegation to logstore for lightweight pollingextractRealtimeInputHistoryand helpers for mapping Responsesroles/content to chat history
transcripts,
TotalTokensfallback computationPreLLMHookandPostLLMHook—realtime requests bypass Maxim's trace/generation lifecycle since they use
Bifrost's own turn-scoped pipeline
Type of change
Affected areas
How to test
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
No new security implications — realtime metadata follows existing log redaction
patterns. Maxim plugin skip prevents realtime events from creating unexpected
external traces.
Checklist
docs/contributing/README.mdand followed the guidelines