Conversation
|
|
Collaborator
Author
This was referenced Apr 26, 2026
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (25)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds three per-request context flags and allow gates for storage/raw overrides plus a per-request content-logging disable header; centralizes Bifrost context creation behind a HandlerStore; threads a computed content-logging decision through the logging plugin; updates handlers, tests, schemas, docs, and UI to expose and honor the flags. (≤50 words) Changes
sequenceDiagram
participant Client
participant HTTP_Handler as Handler (transport)
participant HandlerStore
participant ConvertCtx as ConvertToBifrostContext
participant RequestWorker
participant LoggerPlugin
participant Storage as Logs/Storage
Client->>HTTP_Handler: send request (may include x-bf-disable-content-logging / x-bf-* headers)
HTTP_Handler->>HandlerStore: read config (allow flags, header matcher)
HTTP_Handler->>ConvertCtx: ConvertToBifrostContext(ctx, HandlerStore)
ConvertCtx->>HandlerStore: derive allow flags & matcher
ConvertCtx-->>HTTP_Handler: bifrostCtx (with allow flags + parsed headers)
HTTP_Handler->>RequestWorker: process request using bifrostCtx
RequestWorker->>LoggerPlugin: call Pre/Post hooks with bifrostCtx
LoggerPlugin->>LoggerPlugin: contentLoggingEnabled(bifrostCtx) decision
alt contentLoggingEnabled == true
LoggerPlugin->>Storage: persist content + metadata
else
LoggerPlugin->>Storage: persist metadata only
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
This was referenced Apr 26, 2026
Contributor
Confidence Score: 4/5Safe to merge after addressing the doc regressions; the core Go logic is correct and previously flagged P1s have been resolved. The remaining open P1 (raw-capture headers silently ignored by default — a breaking change for existing header users) was carried over from the prior review round and has not been addressed. No new P0/P1 issues were found in this revision. The two new P2 findings (docs indentation regression and misleading config location copy) are non-blocking.
Important Files Changed
Reviews (6): Last reviewed commit: "feat: add request level support for disa..." | 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/logging/main.go (1)
1162-1164: SnapshotcontentLoggingEnabledbefore launching the async MCP writers.Both goroutines recompute the flag from
ctxinside the closure. SinceBifrostContextis mutable and the plugin config is live, argument/result logging can become timing-dependent. Resolve the bool alongside the other extracted fields and close over it instead.Based on learnings, `BifrostContext` is a mutable shared context and `SetValue(key, value)` mutates the same instance in place.♻️ Suggested tweak
// Get virtual key information from context - using same method as normal LLM logging virtualKeyID := bifrost.GetStringFromContext(ctx, schemas.BifrostContextKeyGovernanceVirtualKeyID) virtualKeyName := bifrost.GetStringFromContext(ctx, schemas.BifrostContextKeyGovernanceVirtualKeyName) + contentLoggingEnabled := p.contentLoggingEnabled(ctx) // Use the per-tool-call unique MCP log ID (set by agent executor per goroutine) as the // primary key. Fall back to requestID if not set (e.g. direct single tool call). mcpLogID, ok := ctx.Value(schemas.BifrostContextKeyMCPLogID).(string) @@ - // Set arguments if content logging is enabled - if p.contentLoggingEnabled(ctx) { + // Set arguments if content logging is enabled + if contentLoggingEnabled { entry.ArgumentsParsed = arguments }// Extract virtual key ID and name from context (set by governance plugin) virtualKeyID := bifrost.GetStringFromContext(ctx, schemas.BifrostContextKeyGovernanceVirtualKeyID) virtualKeyName := bifrost.GetStringFromContext(ctx, schemas.BifrostContextKeyGovernanceVirtualKeyName) + contentLoggingEnabled := p.contentLoggingEnabled(ctx) go func() { updates := make(map[string]interface{}) @@ - // Store result if content logging is enabled - if p.contentLoggingEnabled(ctx) { + // Store result if content logging is enabled + if contentLoggingEnabled { var result interface{}Also applies to: 1263-1264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/main.go` around lines 1162 - 1164, Snapshot the content-logging flag before starting the async MCP writer goroutines instead of recomputing p.contentLoggingEnabled(ctx) inside each closure: evaluate a local bool (e.g., contentLogging := p.contentLoggingEnabled(ctx)) alongside the other extracted fields and use that local variable when setting entry.ArgumentsParsed and entry.ResultsParsed in the goroutines so the closures close over an immutable snapshot rather than reading the mutable BifrostContext later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/providers/request-options.mdx`:
- Around line 441-469: The new docs section uses a custom Tabs/Tab set with
"Gateway (cURL)" and "Go SDK" but must follow the Mintlify MDX guideline
requiring the standard Web UI / API / config.json tab set; replace the Tabs/Tab
block with the standard Mintlify tabs named "Web UI", "API", and "config.json",
move the cURL example under "API", the Go SDK snippet under "Web UI" or "API" as
appropriate, and add a validated config.json example under "config.json" that
conforms to transports/config.schema.json; ensure the Tab titles match exactly
("Web UI","API","config.json") and that any config.json snippet is
schema-validated before committing.
---
Nitpick comments:
In `@plugins/logging/main.go`:
- Around line 1162-1164: Snapshot the content-logging flag before starting the
async MCP writer goroutines instead of recomputing p.contentLoggingEnabled(ctx)
inside each closure: evaluate a local bool (e.g., contentLogging :=
p.contentLoggingEnabled(ctx)) alongside the other extracted fields and use that
local variable when setting entry.ArgumentsParsed and entry.ResultsParsed in the
goroutines so the closures close over an immutable snapshot rather than reading
the mutable BifrostContext later.
🪄 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: 8e6792fc-5629-4e34-b6ad-6c1b96ba2d7f
📒 Files selected for processing (6)
core/schemas/bifrost.godocs/providers/request-options.mdxplugins/logging/changelog.mdplugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.go
Pratham-Mishra04
force-pushed
the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
from
5bf2a03 to
da454c3
Compare
Pratham-Mishra04
force-pushed
the
04-26-fix_pool_req_reuse_fixes_for_streaming_extra_fields
branch
from
cc6d227 to
a45bd43
Compare
Merged
18 tasks
Pratham-Mishra04
changed the title
Contributor
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/operations.go (1)
384-387:⚠️ Potential issue | 🟠 MajorKeep
cache_debugoutside the content-logging gate.
updateLogEntrystill persistscache_debugeven when content logging is off, but this streaming path now drops it whencontentLoggingEnabled=false. That makes streaming cache-hit logs lose non-content metadata and can breakcalculateCostForLog(), which falls back toCacheDebugParsedwhen usage is missing.Suggested fix
- if contentLoggingEnabled { + if streamResponse.Data.CacheDebug != nil { + entry.CacheDebugParsed = streamResponse.Data.CacheDebug + } + + if contentLoggingEnabled { // Transcription output if streamResponse.Data.TranscriptionOutput != nil { entry.TranscriptionOutputParsed = streamResponse.Data.TranscriptionOutput @@ - // Cache debug - if streamResponse.Data.CacheDebug != nil { - entry.CacheDebugParsed = streamResponse.Data.CacheDebug - } // Output message if streamResponse.Data.OutputMessage != nil { entry.OutputMessageParsed = streamResponse.Data.OutputMessage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/operations.go` around lines 384 - 387, The streaming path currently only assigns streamResponse.Data.CacheDebug to entry.CacheDebugParsed inside the content-logging gate, causing cache_debug to be dropped when contentLoggingEnabled is false; change the logic so the assignment of streamResponse.Data.CacheDebug -> entry.CacheDebugParsed occurs unconditionally (outside any contentLoggingEnabled checks) to match updateLogEntry behavior and ensure calculateCostForLog can fall back to CacheDebugParsed when usage is missing; locate the assignment referencing streamResponse.Data.CacheDebug and entry.CacheDebugParsed and move it out of the content-logging conditional.
🧹 Nitpick comments (1)
transports/bifrost-http/lib/config.go (1)
3260-3268: Make new override accessors nil-safe.Line 3262 and Line 3267 dereference
c.ClientConfigdirectly. A zero-value or partially initializedConfigcan panic here. ReturnfalsewhenClientConfigis nil.Proposed hardening
func (c *Config) ShouldAllowPerRequestStorageOverride() bool { - return c.ClientConfig.AllowPerRequestContentStorageOverride + return c.ClientConfig != nil && c.ClientConfig.AllowPerRequestContentStorageOverride } // ShouldAllowPerRequestRawOverride returns whether per-request raw request/response overrides are permitted. func (c *Config) ShouldAllowPerRequestRawOverride() bool { - return c.ClientConfig.AllowPerRequestRawOverride + return c.ClientConfig != nil && c.ClientConfig.AllowPerRequestRawOverride }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config.go` around lines 3260 - 3268, The two accessors ShouldAllowPerRequestStorageOverride and ShouldAllowPerRequestRawOverride dereference c.ClientConfig directly and can panic if ClientConfig is nil; update both functions to first nil-check c.ClientConfig and return false when it's nil, otherwise return c.ClientConfig.AllowPerRequestContentStorageOverride and c.ClientConfig.AllowPerRequestRawOverride respectively so the methods are nil-safe.
🤖 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_test.go`:
- Around line 645-679: The test fails to set the per-request-override gate that
contentLoggingEnabled checks: when creating the test context in
TestContentLoggingEnabledHelper, set
BifrostContextKeyAllowPerRequestStorageOverride to true for cases that expect
the ctx-level BifrostContextKeyDisableContentLogging override to take effect;
locate the context creation (uses schemas.NewBifrostContext and ctx.SetValue)
and add ctx.SetValue(schemas.BifrostContextKeyAllowPerRequestStorageOverride,
true) whenever tc.ctxOverride is non-nil so LoggerPlugin.contentLoggingEnabled
observes the override as intended.
In `@transports/bifrost-http/handlers/realtime_client_secrets.go`:
- Around line 89-94: The code calls lib.ConvertToBifrostContext and immediately
uses bifrostCtx.SetValue, which will panic if ConvertToBifrostContext returned
an error or nil; update the call site in realtime_client_secrets.go to check the
returned context and error (from ConvertToBifrostContext) before mutating it,
handle the error by returning an appropriate HTTP request error (matching the
pattern used in other handlers), and only defer cancel() and call
bifrostCtx.SetValue(schemas.BifrostContextKeyHTTPRequestType,
schemas.RealtimeRequest) after confirming bifrostCtx is non-nil and no error
occurred.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 133-138: ConvertToBifrostContext dereferences store
unconditionally which causes a panic if store is nil; update the function to
guard against a nil HandlerStore by checking if store != nil before calling its
methods (ShouldAllowDirectKeys, GetHeaderMatcher, GetMCPHeaderCombinedAllowlist,
ShouldAllowPerRequestStorageOverride, ShouldAllowPerRequestRawOverride) and
provide sensible defaults when store is nil (e.g., false for booleans, empty
matcher/allowlist) so the request path does not crash; locate the method
ConvertToBifrostContext and the HandlerStore interface methods above and make
those calls conditional, initializing local variables (allowDirectKeys, matcher,
mcpHeaderCombinedAllowlist, allowPerRequestStorageOverride,
allowPerRequestRawOverride) defensively when store is nil.
In `@ui/app/workspace/config/views/loggingView.tsx`:
- Around line 147-173: The two new Switch components
(id="allow-per-request-content-storage-override" and
id="allow-per-request-raw-override") are missing data-testid attributes required
for E2E selection; add data-testid to each Switch (e.g.,
data-testid="allow-per-request-content-storage-override-switch" and
data-testid="allow-per-request-raw-override-switch") so the UI tests can target
them, leaving their id, checked binding (localConfig.*), and onCheckedChange
(handleConfigChange) logic unchanged.
---
Outside diff comments:
In `@plugins/logging/operations.go`:
- Around line 384-387: The streaming path currently only assigns
streamResponse.Data.CacheDebug to entry.CacheDebugParsed inside the
content-logging gate, causing cache_debug to be dropped when
contentLoggingEnabled is false; change the logic so the assignment of
streamResponse.Data.CacheDebug -> entry.CacheDebugParsed occurs unconditionally
(outside any contentLoggingEnabled checks) to match updateLogEntry behavior and
ensure calculateCostForLog can fall back to CacheDebugParsed when usage is
missing; locate the assignment referencing streamResponse.Data.CacheDebug and
entry.CacheDebugParsed and move it out of the content-logging conditional.
---
Nitpick comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 3260-3268: The two accessors ShouldAllowPerRequestStorageOverride
and ShouldAllowPerRequestRawOverride dereference c.ClientConfig directly and can
panic if ClientConfig is nil; update both functions to first nil-check
c.ClientConfig and return false when it's nil, otherwise return
c.ClientConfig.AllowPerRequestContentStorageOverride and
c.ClientConfig.AllowPerRequestRawOverride respectively so the methods are
nil-safe.
🪄 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: 459dceb5-c5cb-4d77-a890-b074c430a2e0
📒 Files selected for processing (25)
core/bifrost.gocore/schemas/bifrost.godocs/openapi/schemas/management/config.yamldocs/providers/request-options.mdxframework/configstore/clientconfig.goplugins/logging/changelog.mdplugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/realtime_client_secrets.gotransports/bifrost-http/handlers/webrtc_realtime.gotransports/bifrost-http/handlers/webrtc_realtime_test.gotransports/bifrost-http/handlers/wsresponses_test.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/config.schema.jsonui/app/workspace/config/views/loggingView.tsxui/lib/types/config.ts
✅ Files skipped from review due to trivial changes (3)
- plugins/logging/changelog.md
- ui/lib/types/config.ts
- docs/openapi/schemas/management/config.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/providers/request-options.mdx
- core/schemas/bifrost.go
- plugins/logging/main.go
Pratham-Mishra04
force-pushed
the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
from
da454c3 to
c2b6cc9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/logging/operations.go (1)
335-409:⚠️ Potential issue | 🟠 MajorKeep
cache_debugoutside the content gate.
streamResponse.Data.CacheDebugis metadata, andupdateLogEntry()still persists it even when content logging is disabled. Leaving it insideif contentLoggingEnabledmakes streaming-enriched entries lose cache-hit/debug info for callback consumers whenever the per-request disable flag is set.Suggested fix
if contentLoggingEnabled { // Transcription output if streamResponse.Data.TranscriptionOutput != nil { entry.TranscriptionOutputParsed = streamResponse.Data.TranscriptionOutput } // Speech output if streamResponse.Data.AudioOutput != nil { entry.SpeechOutputParsed = streamResponse.Data.AudioOutput } // Image generation output if streamResponse.Data.ImageGenerationOutput != nil { entry.ImageGenerationOutputParsed = streamResponse.Data.ImageGenerationOutput } - // Cache debug - if streamResponse.Data.CacheDebug != nil { - entry.CacheDebugParsed = streamResponse.Data.CacheDebug - } // Output message if streamResponse.Data.OutputMessage != nil { entry.OutputMessageParsed = streamResponse.Data.OutputMessage } // Responses output if streamResponse.Data.OutputMessages != nil { entry.ResponsesOutputParsed = streamResponse.Data.OutputMessages } if shouldStoreRaw { // Raw request if streamResponse.RawRequest != nil && *streamResponse.RawRequest != nil { rawRequestBytes, err := sonic.Marshal(*streamResponse.RawRequest) if err == nil { entry.RawRequest = string(rawRequestBytes) } } // Raw response if streamResponse.Data.RawResponse != nil { entry.RawResponse = *streamResponse.Data.RawResponse } } } + if streamResponse.Data.CacheDebug != nil { + entry.CacheDebugParsed = streamResponse.Data.CacheDebug + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/operations.go` around lines 335 - 409, The cache debug field is treated as metadata but is currently set only inside applyStreamingOutputToEntry's contentLoggingEnabled block, causing CacheDebugParsed to be omitted when content logging is disabled; move the nil-check and assignment for streamResponse.Data.CacheDebug (setting entry.CacheDebugParsed = streamResponse.Data.CacheDebug) out of the if contentLoggingEnabled { ... } block (place it alongside other metadata handling after TokenUsage/Cost or before applyModelAlias) so updateLogEntry() will persist cache debug info regardless of the contentLoggingEnabled flag.
🧹 Nitpick comments (1)
plugins/logging/operations_test.go (1)
645-681: Add explicit “override gate disabled” cases to the precedence table.Current rows validate context overrides when the allow gate is
true. Add rows whereBifrostContextKeyAllowPerRequestStorageOverrideisfalseto verify ctx overrides are ignored and global/default behavior wins.🧪 Suggested table-driven extension
func TestContentLoggingEnabledHelper(t *testing.T) { boolPtr := func(b bool) *bool { return &b } tests := []struct { name string globalDisable *bool ctxOverride *bool // nil = don't set the key + allowOverride *bool // nil = don't set the allow key want bool }{ - {"no config no override → enabled", nil, nil, true}, - {"global disable=false no override → enabled", boolPtr(false), nil, true}, - {"global disable=true no override → disabled", boolPtr(true), nil, false}, - {"ctx override=false global disable=true → enabled", boolPtr(true), boolPtr(false), true}, - {"ctx override=true global disable=false → disabled", boolPtr(false), boolPtr(true), false}, - {"ctx override=true nil global → disabled", nil, boolPtr(true), false}, - {"ctx override=false nil global → enabled", nil, boolPtr(false), true}, + {"no config no override → enabled", nil, nil, nil, true}, + {"global disable=false no override → enabled", boolPtr(false), nil, nil, true}, + {"global disable=true no override → disabled", boolPtr(true), nil, nil, false}, + {"ctx override=false global disable=true → enabled", boolPtr(true), boolPtr(false), boolPtr(true), true}, + {"ctx override=true global disable=false → disabled", boolPtr(false), boolPtr(true), boolPtr(true), false}, + {"ctx override=true nil global → disabled", nil, boolPtr(true), boolPtr(true), false}, + {"ctx override=false nil global → enabled", nil, boolPtr(false), boolPtr(true), true}, + {"ctx override=true but allow=false → follows global(enabled)", boolPtr(false), boolPtr(true), boolPtr(false), true}, + {"ctx override=false but allow=false → follows global(disabled)", boolPtr(true), boolPtr(false), boolPtr(false), false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { p := &LoggerPlugin{disableContentLogging: tc.globalDisable} var ctx *schemas.BifrostContext if tc.ctxOverride != nil { ctx = schemas.NewBifrostContext(context.Background(), schemas.NoDeadline) - ctx.SetValue(schemas.BifrostContextKeyAllowPerRequestStorageOverride, true) + if tc.allowOverride != nil { + ctx.SetValue(schemas.BifrostContextKeyAllowPerRequestStorageOverride, *tc.allowOverride) + } ctx.SetValue(schemas.BifrostContextKeyDisableContentLogging, *tc.ctxOverride) } got := p.contentLoggingEnabled(ctx)🤖 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 645 - 681, The test table in TestContentLoggingEnabledHelper misses cases where the per-request override gate is explicitly disabled; add test rows that set ctx with BifrostContextKeyAllowPerRequestStorageOverride = false and BifrostContextKeyDisableContentLogging = true/false to assert that LoggerPlugin.contentLoggingEnabled ignores the ctx override and falls back to the LoggerPlugin.disableContentLogging (and default) behavior; update the loop to construct the context with schemas.NewBifrostContext and set the two keys when testing the "override gate disabled" scenarios so the precedence (gate blocks override) is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 5541-5546: The current override logic uses
BifrostContextKeyAllowPerRequestStorageOverride to gate
BifrostContextKeyStoreRawRequestResponse, which accidentally ties raw-byte
logging to the storage-override flag; change the gate so raw persistence honors
a dedicated raw-override key: when computing effectiveStore (the variable using
config.StoreRawRequestResponse and BifrostContextKeyStoreRawRequestResponse),
check
req.Context.Value(schemas.BifrostContextKeyAllowPerRequestRawOverride).(bool)
(not BifrostContextKeyAllowPerRequestStorageOverride) before applying the
per-request store override so raw capture remains controlled by
allow_per_request_raw_override while other content-storage overrides remain
separate.
---
Outside diff comments:
In `@plugins/logging/operations.go`:
- Around line 335-409: The cache debug field is treated as metadata but is
currently set only inside applyStreamingOutputToEntry's contentLoggingEnabled
block, causing CacheDebugParsed to be omitted when content logging is disabled;
move the nil-check and assignment for streamResponse.Data.CacheDebug (setting
entry.CacheDebugParsed = streamResponse.Data.CacheDebug) out of the if
contentLoggingEnabled { ... } block (place it alongside other metadata handling
after TokenUsage/Cost or before applyModelAlias) so updateLogEntry() will
persist cache debug info regardless of the contentLoggingEnabled flag.
---
Nitpick comments:
In `@plugins/logging/operations_test.go`:
- Around line 645-681: The test table in TestContentLoggingEnabledHelper misses
cases where the per-request override gate is explicitly disabled; add test rows
that set ctx with BifrostContextKeyAllowPerRequestStorageOverride = false and
BifrostContextKeyDisableContentLogging = true/false to assert that
LoggerPlugin.contentLoggingEnabled ignores the ctx override and falls back to
the LoggerPlugin.disableContentLogging (and default) behavior; update the loop
to construct the context with schemas.NewBifrostContext and set the two keys
when testing the "override gate disabled" scenarios so the precedence (gate
blocks override) is validated.
🪄 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: d8115a5a-dedd-4096-8bf9-037332df20eb
📒 Files selected for processing (25)
core/bifrost.gocore/schemas/bifrost.godocs/openapi/schemas/management/config.yamldocs/providers/request-options.mdxframework/configstore/clientconfig.goplugins/logging/changelog.mdplugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/realtime_client_secrets.gotransports/bifrost-http/handlers/webrtc_realtime.gotransports/bifrost-http/handlers/webrtc_realtime_test.gotransports/bifrost-http/handlers/wsresponses_test.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/config.schema.jsonui/app/workspace/config/views/loggingView.tsxui/lib/types/config.ts
✅ Files skipped from review due to trivial changes (7)
- transports/bifrost-http/handlers/webrtc_realtime_test.go
- plugins/logging/changelog.md
- transports/bifrost-http/integrations/router.go
- transports/bifrost-http/handlers/mcpinference.go
- docs/providers/request-options.mdx
- transports/bifrost-http/handlers/mcpserver.go
- transports/bifrost-http/handlers/asyncinference.go
🚧 Files skipped from review as they are similar to previous changes (9)
- transports/bifrost-http/integrations/bedrock_test.go
- ui/lib/types/config.ts
- ui/app/workspace/config/views/loggingView.tsx
- framework/configstore/clientconfig.go
- core/schemas/bifrost.go
- transports/bifrost-http/lib/ctx.go
- plugins/logging/main.go
- transports/bifrost-http/lib/ctx_test.go
- transports/bifrost-http/handlers/inference.go
Pratham-Mishra04
force-pushed
the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
from
c2b6cc9 to
6e1fb4a
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
plugins/logging/operations_test.go (1)
779-842: Consider addingapplyStreamingOutputToEntrytest coverage.The test file now has good coverage for
updateLogEntry,applyRealtimeOutputToEntry, andapplyNonStreamingOutputToEntrywith bothcontentLoggingEnabledvalues. However,applyStreamingOutputToEntry(updated at line 335 in operations.go) lacks direct unit tests for content logging behavior.🧪 Minimal coverage to add
func TestApplyStreamingOutputToEntryContentLoggingDisabled(t *testing.T) { plugin := &LoggerPlugin{} entry := &logstore.Log{} chatText := "should not appear" streamResponse := &streaming.ProcessedStreamResponse{ Data: &streaming.StreamData{ OutputMessage: &schemas.ChatMessage{ Role: schemas.ChatMessageRoleAssistant, Content: &schemas.ChatMessageContent{ContentStr: &chatText}, }, RawResponse: schemas.Ptr(`{"id":"resp-1"}`), }, RawRequest: schemas.Ptr(map[string]any{"model": "gpt-4o"}), } plugin.applyStreamingOutputToEntry(entry, streamResponse, true, false) if entry.OutputMessageParsed != nil { t.Error("expected OutputMessageParsed to be nil when contentLoggingEnabled=false") } if entry.RawRequest != "" { t.Error("expected RawRequest to be empty when contentLoggingEnabled=false") } }🤖 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 779 - 842, Add unit tests for applyStreamingOutputToEntry to cover content-logging behavior: create TestApplyStreamingOutputToEntryContentLoggingDisabled and TestApplyStreamingOutputToEntryContentLoggingEnabled that construct a LoggerPlugin and a logstore.Log, build a streaming.ProcessedStreamResponse with Data.OutputMessage (schemas.ChatMessage with ContentStr) and RawRequest set, call plugin.applyStreamingOutputToEntry(entry, streamResponse, /*isStreaming*/ true, contentLoggingEnabled), and assert that when contentLoggingEnabled=false entry.OutputMessageParsed is nil and RawRequest is empty, while when contentLoggingEnabled=true entry.OutputMessageParsed is non-nil and RawRequest is preserved; reference applyStreamingOutputToEntry, streaming.ProcessedStreamResponse, Data.OutputMessage, and RawRequest when locating code to exercise.
🤖 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 719-720: When serializing bifrostErr into entry.ErrorDetails,
scrub or nil out bifrostErr.ExtraFields.RawRequest and RawResponse according to
the same storage decision gates used for entry.RawRequest/RawResponse (i.e.,
p.contentLoggingEnabled(ctx), shouldStoreRaw and
allow_per_request_content_storage_override) so raw bytes cannot leak into
ErrorDetails; update the code paths that marshal bifrostErr (the locations that
call json.Marshal / serialize the bifrostErr used to populate ErrorDetails) to
copy bifrostErr, clear ExtraFields.RawRequest/RawResponse when content logging
or raw persistence is disabled, then marshal that sanitized copy; apply the same
change to the other serialization sites referenced (the other bifrostErr ->
ErrorDetails marshal spots noted in the comment).
In `@transports/bifrost-http/handlers/mcpserver.go`:
- Around line 115-116: The MCP handler now inherits ShouldAllowDirectKeys() from
h.config via lib.ConvertToBifrostContext which can re-enable direct-key intake
for MCP endpoints; after calling ConvertToBifrostContext in mcpserver.go (where
bifrostCtx is created) explicitly override bifrostCtx.ShouldAllowDirectKeys to
preserve the original MCP deny policy (i.e., make it return false) before
passing bifrostCtx into HandlerStore or any downstream call; this keeps the new
signature but ensures MCP-specific behavior is enforced regardless of h.config.
In `@transports/config.schema.json`:
- Around line 71-79: Update the schema descriptions so they clearly distinguish
the two flags: state that allow_per_request_content_storage_override not only
controls per-request content logging overrides but also gates per-request
raw-byte persistence (the BifrostContextKeyStoreRawRequestResponse/context key
and store_raw_request_response behavior), whereas allow_per_request_raw_override
only controls send-back raw visibility via x-bf-send-back-raw-request/response;
modify the description strings for the properties
allow_per_request_content_storage_override and allow_per_request_raw_override
(and the matching description at the other occurrence) to explicitly call out
these scoped behaviors so callers cannot confuse persistence vs send-back
visibility.
---
Nitpick comments:
In `@plugins/logging/operations_test.go`:
- Around line 779-842: Add unit tests for applyStreamingOutputToEntry to cover
content-logging behavior: create
TestApplyStreamingOutputToEntryContentLoggingDisabled and
TestApplyStreamingOutputToEntryContentLoggingEnabled that construct a
LoggerPlugin and a logstore.Log, build a streaming.ProcessedStreamResponse with
Data.OutputMessage (schemas.ChatMessage with ContentStr) and RawRequest set,
call plugin.applyStreamingOutputToEntry(entry, streamResponse, /*isStreaming*/
true, contentLoggingEnabled), and assert that when contentLoggingEnabled=false
entry.OutputMessageParsed is nil and RawRequest is empty, while when
contentLoggingEnabled=true entry.OutputMessageParsed is non-nil and RawRequest
is preserved; reference applyStreamingOutputToEntry,
streaming.ProcessedStreamResponse, Data.OutputMessage, and RawRequest when
locating code to exercise.
🪄 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: 3a1c847e-9911-4398-9095-a6c78c7e9778
📒 Files selected for processing (25)
core/bifrost.gocore/schemas/bifrost.godocs/openapi/schemas/management/config.yamldocs/providers/request-options.mdxframework/configstore/clientconfig.goplugins/logging/changelog.mdplugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/realtime_client_secrets.gotransports/bifrost-http/handlers/webrtc_realtime.gotransports/bifrost-http/handlers/webrtc_realtime_test.gotransports/bifrost-http/handlers/wsresponses_test.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/config.schema.jsonui/app/workspace/config/views/loggingView.tsxui/lib/types/config.ts
✅ Files skipped from review due to trivial changes (7)
- plugins/logging/changelog.md
- transports/bifrost-http/handlers/wsresponses_test.go
- transports/bifrost-http/handlers/mcpinference.go
- docs/openapi/schemas/management/config.yaml
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/lib/ctx_test.go
- transports/bifrost-http/handlers/inference.go
🚧 Files skipped from review as they are similar to previous changes (4)
- transports/bifrost-http/handlers/realtime_client_secrets.go
- ui/app/workspace/config/views/loggingView.tsx
- docs/providers/request-options.mdx
- transports/bifrost-http/handlers/asyncinference.go
Pratham-Mishra04
force-pushed
the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
from
6e1fb4a to
cb62c54
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/logging/operations.go (1)
136-150:⚠️ Potential issue | 🟠 MajorDecouple raw-byte persistence from parsed content logging.
The raw-persistence control (
shouldStoreRaw) derived fromBifrostContextKeyStoreRawRequestResponseis currently ineffective for requests that disable parsed content logging. All raw field writes—inapplyStreamingOutputToEntry,applyNonStreamingOutputToEntry,applyRealtimeOutputToEntry, andapplyRealtimeRawRequestBackfill—are guarded by bothshouldStoreRawandcontentLoggingEnabled. This prevents raw bytes from persisting whencontentLoggingEnabled=false, even when thestore_raw_request_responseoverride is explicitly enabled.Per the design,
allow_per_request_content_storage_overridegates both content logging and the raw-byte persistence override (BifrostContextKeyStoreRawRequestResponse). The logging plugin must respect these independently: raw writes should depend only onshouldStoreRaw, not be nested withincontentLoggingEnabledbranches. Remove thecontentLoggingEnabledguard from all raw field assignments.Also applies to: lines 184-257, 294-302, 335-335, 371-409, 422-422, 469-483, 541-563, 581-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/operations.go` around lines 136 - 150, The raw-byte persistence is incorrectly tied to contentLoggingEnabled; update the logging plugin so raw field writes only depend on the per-request override boolean (shouldStoreRaw derived from BifrostContextKeyStoreRawRequestResponse) and not on contentLoggingEnabled. In the functions applyStreamingOutputToEntry, applyNonStreamingOutputToEntry, applyRealtimeOutputToEntry, and applyRealtimeRawRequestBackfill (and any other places noted in the comment), remove the contentLoggingEnabled guard around assignments to raw* fields and ensure those assignments check only shouldStoreRaw; keep contentLoggingEnabled checks only for parsed/structured content writes. Reference the variables shouldStoreRaw and contentLoggingEnabled and the raw field assignments in those functions when making the change.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/mcpserver.go (1)
115-116:⚠️ Potential issue | 🟠 MajorPreserve MCP's direct-key deny policy when building the request context.
Line 115 and Line 156 now pass
h.configthrough toConvertToBifrostContext, which re-enablesShouldAllowDirectKeys()whenever the global config allows it. The previous MCP path explicitly passedfalse, so this changes auth behavior on/mcpand can let provider API keys inAuthorization/x-api-key/x-goog-api-keybe accepted on MCP endpoints.🔧 Suggested fix
+type mcpContextHandlerStore struct { + lib.HandlerStore +} + +func (m mcpContextHandlerStore) ShouldAllowDirectKeys() bool { return false } + func (h *MCPServerHandler) handleMCPServer(ctx *fasthttp.RequestCtx) { @@ - bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, h.config) + bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, mcpContextHandlerStore{HandlerStore: h.config}) @@ func (h *MCPServerHandler) handleMCPServerSSE(ctx *fasthttp.RequestCtx) { @@ - bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, h.config) + bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, mcpContextHandlerStore{HandlerStore: h.config})Also applies to: 156-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcpserver.go` around lines 115 - 116, The MCP handlers currently call lib.ConvertToBifrostContext with h.config which re-enables provider direct-key checks; update both calls that create bifrostCtx (the one assigned to bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, h.config) and the later call around the second handler) to explicitly preserve MCP's deny-by-default behavior by passing the explicit false direct-key flag to ConvertToBifrostContext (i.e., call ConvertToBifrostContext with the argument that enforces ShouldAllowDirectKeys() == false so MCP endpoints do not accept provider API keys).
🧹 Nitpick comments (3)
framework/configstore/clientconfig.go (1)
54-55: Clarify the storage-vs-raw gate split in these comments.
AllowPerRequestContentStorageOverridealso governs the per-request raw-byte persistence override (x-bf-store-raw-request-response), whileAllowPerRequestRawOverrideis only for send-back raw request/response. The current comments read narrower than the actual behavior.Based on learnings: In maximhq/bifrost (PR
#3066),allow_per_request_raw_overridegates only “send back” raw request/response, whileallow_per_request_content_storage_overridegates content logging and the per-request raw-byte persistence override.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/clientconfig.go` around lines 54 - 55, Update the struct field comments to accurately reflect the gate split: clarify that AllowPerRequestContentStorageOverride controls content logging and the per-request raw-byte persistence override (x-bf-store-raw-request-response), while AllowPerRequestRawOverride only controls sending back raw request/response (x-bf-send-back-raw-request and x-bf-send-back-raw-response). Edit the comments on the AllowPerRequestContentStorageOverride and AllowPerRequestRawOverride fields in clientconfig.go to mention these exact headers and behaviors so the comments match the actual runtime gating.docs/providers/request-options.mdx (1)
463-463: Fix precedence wording for the global content-logging toggle.The sentence says content is recorded when override is
false“even if the global toggle is off.” The meaningful override case is when the globaldisable_content_loggingtoggle is on.✏️ Suggested doc wording tweak
-Override the logging plugin's global `disable_content_logging` config for a single request. When set to `true`, messages, parameters, tool arguments, and tool results are omitted from the log record for that request. When set to `false`, content is recorded even if the global toggle is off. +Override the logging plugin's global `disable_content_logging` config for a single request. When set to `true`, messages, parameters, tool arguments, and tool results are omitted from the log record for that request. When set to `false`, content is recorded even if the global toggle is on.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/providers/request-options.mdx` at line 463, The current sentence for the request-level `disable_content_logging` override has incorrect precedence wording: change the phrase that reads "even if the global toggle is off" to correctly state that setting the request override to `false` records content even if the global `disable_content_logging` toggle is on; update the sentence around `disable_content_logging` to reflect that the request-level `false` override takes precedence over a global `true`.plugins/logging/operations_test.go (1)
645-680: Add a deny-by-default override case.This table only exercises
BifrostContextKeyDisableContentLoggingwhenBifrostContextKeyAllowPerRequestStorageOverrideis set totrue. The security-critical branch here is the inverse one: if the override key is present but the allow gate is absent/false,contentLoggingEnabled()must ignore it and fall back to global/default behavior. Please add an explicit row for that path.Based on learnings:
allow_per_request_content_storage_overridegates content logging and the per-request raw-byte persistence override used by the logging plugin.🤖 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 645 - 680, Add a test row that verifies the deny-by-default behavior: create a LoggerPlugin with disableContentLogging set to true/false (and a nil case) then build a BifrostContext via schemas.NewBifrostContext and call ctx.SetValue(schemas.BifrostContextKeyDisableContentLogging, true) (or false) but do NOT set schemas.BifrostContextKeyAllowPerRequestStorageOverride (or set it to false); call p.contentLoggingEnabled(ctx) and assert the result falls back to the global/default behavior (i.e., the context override is ignored when the allow gate is absent/false). Reference contentLoggingEnabled, LoggerPlugin.disableContentLogging, schemas.BifrostContextKeyAllowPerRequestStorageOverride, schemas.BifrostContextKeyDisableContentLogging, and schemas.NewBifrostContext to locate where to add the test.
🤖 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 768-771: The code marshals and assigns a sanitized error to
entry.ErrorDetails but leaves entry.ErrorDetailsParsed set to the original
bifrostErr, which can leak RawRequest/RawResponse; update the assignments around
sanitizeErrorForLogging so that the sanitized error object (the value returned
or produced by sanitizeErrorForLogging) is used for both entry.ErrorDetails
(string) and entry.ErrorDetailsParsed, ensuring you respect
contentLoggingEnabled and shouldStoreRaw; specifically, replace uses of
bifrostErr when setting entry.ErrorDetailsParsed with the sanitized error object
returned from sanitizeErrorForLogging in the same blocks where sonic.Marshal is
called (the branches handling contentLoggingEnabled/shouldStoreRaw), and apply
the same change to the other similar blocks noted (near the locations
referenced).
---
Outside diff comments:
In `@plugins/logging/operations.go`:
- Around line 136-150: The raw-byte persistence is incorrectly tied to
contentLoggingEnabled; update the logging plugin so raw field writes only depend
on the per-request override boolean (shouldStoreRaw derived from
BifrostContextKeyStoreRawRequestResponse) and not on contentLoggingEnabled. In
the functions applyStreamingOutputToEntry, applyNonStreamingOutputToEntry,
applyRealtimeOutputToEntry, and applyRealtimeRawRequestBackfill (and any other
places noted in the comment), remove the contentLoggingEnabled guard around
assignments to raw* fields and ensure those assignments check only
shouldStoreRaw; keep contentLoggingEnabled checks only for parsed/structured
content writes. Reference the variables shouldStoreRaw and contentLoggingEnabled
and the raw field assignments in those functions when making the change.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/mcpserver.go`:
- Around line 115-116: The MCP handlers currently call
lib.ConvertToBifrostContext with h.config which re-enables provider direct-key
checks; update both calls that create bifrostCtx (the one assigned to
bifrostCtx, cancel := lib.ConvertToBifrostContext(ctx, h.config) and the later
call around the second handler) to explicitly preserve MCP's deny-by-default
behavior by passing the explicit false direct-key flag to
ConvertToBifrostContext (i.e., call ConvertToBifrostContext with the argument
that enforces ShouldAllowDirectKeys() == false so MCP endpoints do not accept
provider API keys).
---
Nitpick comments:
In `@docs/providers/request-options.mdx`:
- Line 463: The current sentence for the request-level `disable_content_logging`
override has incorrect precedence wording: change the phrase that reads "even if
the global toggle is off" to correctly state that setting the request override
to `false` records content even if the global `disable_content_logging` toggle
is on; update the sentence around `disable_content_logging` to reflect that the
request-level `false` override takes precedence over a global `true`.
In `@framework/configstore/clientconfig.go`:
- Around line 54-55: Update the struct field comments to accurately reflect the
gate split: clarify that AllowPerRequestContentStorageOverride controls content
logging and the per-request raw-byte persistence override
(x-bf-store-raw-request-response), while AllowPerRequestRawOverride only
controls sending back raw request/response (x-bf-send-back-raw-request and
x-bf-send-back-raw-response). Edit the comments on the
AllowPerRequestContentStorageOverride and AllowPerRequestRawOverride fields in
clientconfig.go to mention these exact headers and behaviors so the comments
match the actual runtime gating.
In `@plugins/logging/operations_test.go`:
- Around line 645-680: Add a test row that verifies the deny-by-default
behavior: create a LoggerPlugin with disableContentLogging set to true/false
(and a nil case) then build a BifrostContext via schemas.NewBifrostContext and
call ctx.SetValue(schemas.BifrostContextKeyDisableContentLogging, true) (or
false) but do NOT set schemas.BifrostContextKeyAllowPerRequestStorageOverride
(or set it to false); call p.contentLoggingEnabled(ctx) and assert the result
falls back to the global/default behavior (i.e., the context override is ignored
when the allow gate is absent/false). Reference contentLoggingEnabled,
LoggerPlugin.disableContentLogging,
schemas.BifrostContextKeyAllowPerRequestStorageOverride,
schemas.BifrostContextKeyDisableContentLogging, and schemas.NewBifrostContext to
locate where to add the test.
🪄 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: cd6c6f1d-8468-4037-af61-52eaa80374cc
📒 Files selected for processing (25)
core/bifrost.gocore/schemas/bifrost.godocs/openapi/schemas/management/config.yamldocs/providers/request-options.mdxframework/configstore/clientconfig.goplugins/logging/changelog.mdplugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.gotransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcpinference.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/realtime_client_secrets.gotransports/bifrost-http/handlers/webrtc_realtime.gotransports/bifrost-http/handlers/webrtc_realtime_test.gotransports/bifrost-http/handlers/wsresponses_test.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/ctx_test.gotransports/config.schema.jsonui/app/workspace/config/views/loggingView.tsxui/lib/types/config.ts
✅ Files skipped from review due to trivial changes (9)
- plugins/logging/changelog.md
- transports/bifrost-http/handlers/mcpinference.go
- docs/openapi/schemas/management/config.yaml
- transports/bifrost-http/handlers/webrtc_realtime_test.go
- transports/bifrost-http/lib/config.go
- core/bifrost.go
- ui/app/workspace/config/views/loggingView.tsx
- transports/bifrost-http/handlers/asyncinference.go
- transports/bifrost-http/lib/ctx_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/bifrost-http/handlers/realtime_client_secrets.go
- transports/bifrost-http/handlers/wsresponses_test.go
- ui/lib/types/config.ts
- transports/bifrost-http/handlers/webrtc_realtime.go
- transports/bifrost-http/integrations/router.go
- core/schemas/bifrost.go
This was referenced Apr 27, 2026
fix: allow self-looping chain rules to continue evaluating subsequent rules instead of halting
#3082
Merged
Contributor
Merge activity
|
akshaydeo
changed the base branch from
04-26-fix_pool_req_reuse_fixes_for_streaming_extra_fields
to
graphite-base/3066
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
Pratham-Mishra04
force-pushed
the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
from
cb62c54 to
3cf0d10
Compare
akshaydeo
deleted the
04-26-feat_add_request_level_support_for_disable_content_storage_in_loggin_plugin
branch
18 tasks
18 tasks
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
Adds two opt-in configuration flags —
allow_per_request_content_storage_overrideandallow_per_request_raw_override— that gate whether per-request context keys and HTTP headers can override the global content logging and raw request/response visibility settings. When enabled, callers can suppress sensitive content (e.g. PII, credentials) from log records on specific requests viax-bf-disable-content-logging, or override raw capture behavior viax-bf-send-back-raw-request/x-bf-send-back-raw-response, without changing global configuration. When disabled (the default), provider-level and global settings remain authoritative and per-request overrides are silently ignored.Changes
BifrostContextKeyDisableContentLogging,BifrostContextKeyAllowPerRequestStorageOverride, andBifrostContextKeyAllowPerRequestRawOverrideto theBifrostContextKeyenum.contentLoggingEnabled(ctx)helper onLoggerPluginthat checks the per-request context override (only whenBifrostContextKeyAllowPerRequestStorageOverrideis set) before falling back to the globaldisableContentLoggingconfig. This replaces all inlinep.disableContentLogging == nil || !*p.disableContentLoggingchecks throughout the logging plugin.contentLoggingEnabledbool as an explicit parameter toupdateLogEntry,applyStreamingOutputToEntry,applyNonStreamingOutputToEntry, andapplyRealtimeOutputToEntry, removing the redundant local re-evaluation insideapplyRealtimeOutputToEntry.BifrostContextKeySendBackRawRequest,BifrostContextKeySendBackRawResponse, andBifrostContextKeyStoreRawRequestResponseper-request overrides inrequestWorkerbehind the newBifrostContextKeyAllowPerRequestRawOverrideflag.AllowPerRequestContentStorageOverrideandAllowPerRequestRawOverridefields toClientConfigwith corresponding hash entries,Configaccessor methods, and two new methods on theHandlerStoreinterface (ShouldAllowPerRequestStorageOverride,ShouldAllowPerRequestRawOverride).ConvertToBifrostContextto accept aHandlerStoreinstead of individual parameters, propagating the new override flags into the bifrost context at request ingress.CoreConfigTypeScript type and defaults.updateLogEntryand the apply-output helpers verifying both suppression and force-enable behavior.x-bf-disable-content-loggingoption indocs/providers/request-options.mdxwith cURL and Go SDK examples, precedence rules, and a prerequisite callout forallow_per_request_content_storage_override.Type of change
Affected areas
How to test
go test ./plugins/logging/... ./transports/bifrost-http/lib/...To validate end-to-end via the gateway, first enable
allow_per_request_content_storage_overridein your logging config, then send a chat completion request with the header and confirm the log record omits message content while still recording token counts and latency:The resulting log record should have empty
input_history,output_message, andraw_request/raw_responsefields, whiletotal_tokens,latency, and routing metadata remain populated.To verify the gate is enforced, send the same request without enabling
allow_per_request_content_storage_override— the header should be ignored and content should appear in the log record as normal.Breaking changes
Security considerations
Both override flags default to
false, meaning existing deployments are unaffected on upgrade. The per-request override is applied only at log-write time and does not affect what is sent to the upstream provider. Operators should consider carefully before enablingallow_per_request_raw_override, as it permits callers to request that raw provider payloads be returned in API responses.Checklist
docs/contributing/README.mdand followed the guidelines