Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughApplies safer runtime extraction and per-scenario retry config, tightens nil/empty guards, and adds mutex synchronization to streaming accumulators and getters; also introduces OpenAI model conversion helpers and increases pricing output precision formatting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/utils/utils.go (1)
1623-1624: Redundant cleanup call - already handled by defer.
ClearDeferredSpanis already called viadeferat line 1564, so this explicit call at line 1624 results in double invocation. The defer pattern alone is sufficient and preferable for cleanup.🔧 Suggested fix - remove redundant call
} else { tracer.EndSpan(handle, schemas.SpanStatusOk, "") } - - // Clear the deferred span from TraceStore - tracer.ClearDeferredSpan(traceID) }framework/streaming/accumulator.go (1)
140-147: Fix non-atomic get-or-create to avoid duplicate accumulators under concurrency.
Load()+create()+Store()can return different*StreamAccumulators to concurrent callers for the samerequestID, which breaks accumulation / ref-counting / cleanup correctness. PreferLoadOrStore.Proposed fix
func (a *Accumulator) createStreamAccumulator(requestID string) *StreamAccumulator { now := time.Now() sc := &StreamAccumulator{ RequestID: requestID, ChatStreamChunks: make([]*ChatStreamChunk, 0), ResponsesStreamChunks: make([]*ResponsesStreamChunk, 0), TranscriptionStreamChunks: make([]*TranscriptionStreamChunk, 0), AudioStreamChunks: make([]*AudioStreamChunk, 0), ChatChunksSeen: make(map[int]struct{}), ResponsesChunksSeen: make(map[int]struct{}), TranscriptionChunksSeen: make(map[int]struct{}), AudioChunksSeen: make(map[int]struct{}), MaxChatChunkIndex: -1, MaxResponsesChunkIndex: -1, MaxTranscriptionChunkIndex: -1, MaxAudioChunkIndex: -1, IsComplete: false, - mu: sync.Mutex{}, Timestamp: now, StartTimestamp: now, // Set default StartTimestamp for proper TTFT/latency calculation } - a.streamAccumulators.Store(requestID, sc) return sc } func (a *Accumulator) getOrCreateStreamAccumulator(requestID string) *StreamAccumulator { if accumulator, exists := a.streamAccumulators.Load(requestID); exists { return accumulator.(*StreamAccumulator) } - // Create new accumulator if it doesn't exist - return a.createStreamAccumulator(requestID) + // Create new accumulator if it doesn't exist (atomically) + sc := a.createStreamAccumulator(requestID) + actual, loaded := a.streamAccumulators.LoadOrStore(requestID, sc) + if loaded { + return actual.(*StreamAccumulator) + } + return sc }
🤖 Fix all issues with AI agents
In @core/schemas/plugin_wasm.go:
- Around line 6-7: The top-of-file comment in core/schemas/plugin_wasm.go
contains a minor grammar issue: remove the extra double space and insert "or"
between the two variants so the sentence reads something like "It can contain
either a response (success short-circuit) or an error (error short-circuit).
Streams are not supported in WASM plugins."; update that comment accordingly.
🧹 Nitpick comments (3)
plugins/governance/advancedscenarios_test.go (1)
458-497: LGTM! Explicit budget exhaustion tracking improves test reliability.The addition of the
budgetExhaustedflag to explicitly track whether a budget rejection was observed is a solid improvement. This approach is more reliable than inferring exhaustion from consumed amounts, which could be affected by cost calculation discrepancies.Consider using
sawBudgetRejectionfor naming consistency with the other tests in this file (e.g.,TestVKBudgetUpdateAfterExhaustionat line 576,TestTeamBudgetUpdateAfterExhaustionat line 717). This is a minor nit and not required.framework/streaming/accumulator.go (1)
113-138:mu: sync.Mutex{}initialization is redundant (but OK).Go mutexes are usable as a zero value, so this explicit init isn’t required; keep it only if you want the clarity.
plugins/semanticcache/stream.go (1)
30-38: ConsiderLoadOrStoreto make plugin accumulator creation atomic.If
getOrCreateStreamAccumulatorcan be hit concurrently perrequestID, switch toLoadOrStoreto avoid duplicate accumulators (same race asframework/streaming/accumulator.go).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/internal/testutil/file_base64.gocore/providers/anthropic/responses.gocore/providers/bedrock/utils.gocore/providers/gemini/chat.gocore/providers/gemini/gemini.gocore/providers/gemini/responses.gocore/providers/gemini/transcription.gocore/providers/gemini/types.gocore/providers/utils/utils.gocore/schemas/plugin_wasm.gocore/schemas/utils.goframework/streaming/accumulator.goframework/streaming/types.goplugins/governance/advancedscenarios_test.goplugins/semanticcache/stream.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/bedrock/utils.gocore/providers/gemini/gemini.gocore/providers/gemini/types.goframework/streaming/accumulator.gocore/providers/gemini/transcription.gocore/providers/gemini/chat.goframework/streaming/types.gocore/internal/testutil/file_base64.goplugins/governance/advancedscenarios_test.gocore/providers/gemini/responses.gocore/providers/utils/utils.goplugins/semanticcache/stream.gocore/schemas/plugin_wasm.gocore/providers/anthropic/responses.gocore/schemas/utils.go
🧠 Learnings (10)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/bedrock/utils.gocore/providers/gemini/gemini.gocore/providers/gemini/types.goframework/streaming/accumulator.gocore/providers/gemini/transcription.gocore/providers/gemini/chat.goframework/streaming/types.gocore/internal/testutil/file_base64.goplugins/governance/advancedscenarios_test.gocore/providers/gemini/responses.gocore/providers/utils/utils.goplugins/semanticcache/stream.gocore/schemas/plugin_wasm.gocore/providers/anthropic/responses.gocore/schemas/utils.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
core/providers/bedrock/utils.gocore/providers/gemini/gemini.gocore/providers/gemini/types.goframework/streaming/accumulator.gocore/providers/gemini/transcription.gocore/providers/gemini/chat.goframework/streaming/types.gocore/internal/testutil/file_base64.goplugins/governance/advancedscenarios_test.gocore/providers/gemini/responses.gocore/providers/utils/utils.goplugins/semanticcache/stream.gocore/schemas/plugin_wasm.gocore/providers/anthropic/responses.gocore/schemas/utils.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/bedrock/utils.gocore/providers/gemini/gemini.gocore/providers/gemini/types.gocore/providers/gemini/transcription.gocore/providers/gemini/chat.gocore/providers/gemini/responses.gocore/providers/utils/utils.gocore/providers/anthropic/responses.go
📚 Learning: 2025-12-26T05:46:14.625Z
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 1172
File: core/providers/bedrock/responses.go:2929-2985
Timestamp: 2025-12-26T05:46:14.625Z
Learning: In Bedrock file handling (core/providers/bedrock/responses.go and utils.go), ensure that the FileData field is treated as either a proper data URL (prefix 'data:') or as plain text. Do not expect or accept naked base64-encoded strings without the data URL prefix. The code should first check for the data URL prefix; if missing, process the value as plain text. This pattern applies to all Go files in the bedrock provider directory.
Applied to files:
core/providers/bedrock/utils.go
📚 Learning: 2026-01-10T11:27:47.535Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 1256
File: core/providers/openai/openai.go:2276-2385
Timestamp: 2026-01-10T11:27:47.535Z
Learning: Validate image generation requests for nil and missing prompts before dispatch. Follow the same pattern used here: core/bifrost.go validates nil/empty prompts, providerUtils.CheckContextAndGetRequestBody returns a structured error when the request converter yields nil, and apply this across all providers (including OpenAI) to avoid sending null bodies.
Applied to files:
core/providers/bedrock/utils.gocore/providers/gemini/gemini.gocore/providers/gemini/types.gocore/providers/gemini/transcription.gocore/providers/gemini/chat.gocore/providers/gemini/responses.gocore/providers/utils/utils.gocore/providers/anthropic/responses.go
📚 Learning: 2025-12-19T08:29:20.286Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: core/internal/testutil/count_tokens.go:30-67
Timestamp: 2025-12-19T08:29:20.286Z
Learning: In core/internal/testutil test files, enforce using GetTestRetryConfigForScenario() to obtain a generic retry config, then construct a typed retry config (e.g., CountTokensRetryConfig, EmbeddingRetryConfig, TranscriptionRetryConfig) with an empty Conditions slice. Copy only MaxAttempts, BaseDelay, MaxDelay, OnRetry, and OnFinalFail from the generic config. This convention should be consistently applied across all test files in this directory.
Applied to files:
core/internal/testutil/file_base64.go
📚 Learning: 2025-12-22T10:50:40.990Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1154
File: plugins/governance/store.go:1165-1186
Timestamp: 2025-12-22T10:50:40.990Z
Learning: In the Bifrost governance plugin, budgets and rate limits have 1:1 relationships with their parent entities (virtual keys, teams, customers). Do not assume sharing; ensure cascade deletion logic only deletes budgets/rate limits when there are no shared references. Enforce invariants in code and add tests to verify no cross-entity sharing and that cascade deletes only remove the specific child of the parent. If a counterexample arises, adjust data model or add guards.
Applied to files:
plugins/governance/advancedscenarios_test.go
📚 Learning: 2026-01-10T15:52:10.995Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1298
File: core/providers/utils/utils.go:983-1006
Timestamp: 2026-01-10T15:52:10.995Z
Learning: In Go with fasthttp v1.68.0, Response.BodyStream() returns an io.Reader, but the concrete type also implements io.Closer. In reviews, consider adding a safe type assertion pattern to close the stream when possible, e.g. if closer, ok := bodyStream.(io.Closer); ok { if err := closer.Close(); err != nil { // handle } }. This check should be applied in all code paths where BodyStream() is consumed, not only in this specific file, to ensure proper resource cleanup when the underlying reader supports closing.
Applied to files:
core/providers/utils/utils.go
📚 Learning: 2026-01-11T14:08:10.341Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1298
File: core/providers/anthropic/anthropic.go:682-699
Timestamp: 2026-01-11T14:08:10.341Z
Learning: In Anthroplic streaming implementations (and analogous providers), ensure that the final 'summary' chunk, which carries usage information and metadata, is emitted after all delta chunks and uses a chunk index of last_delta_index + 1. This differentiates the summary chunk from content delta chunks. Apply this convention consistently in the anthropic provider code and in similar streaming providers, and consider adding a targeted test to assert the ordering and chunk index logic.
Applied to files:
core/providers/anthropic/responses.go
📚 Learning: 2026-01-12T06:41:13.643Z
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 1153
File: core/schemas/utils.go:1159-1183
Timestamp: 2026-01-12T06:41:13.643Z
Learning: In core/schemas/utils.go, IsGrokReasoningModel (and the grokReasoningModels list) is the canonical utility for model matching. Treat it as the source of truth and not as a duplicate; if extending model support, update this file rather than introducing separate matching logic, and verify changes here to avoid diverging behavior.
Applied to files:
core/schemas/utils.go
🧬 Code graph analysis (5)
core/providers/bedrock/utils.go (1)
core/schemas/utils.go (1)
SafeExtractStringMap(527-547)
core/providers/gemini/transcription.go (2)
core/providers/gemini/types.go (1)
SafeExtractSafetySettings(103-133)core/schemas/utils.go (1)
SafeExtractStringMap(527-547)
core/providers/gemini/chat.go (2)
core/providers/gemini/types.go (1)
SafeExtractSafetySettings(103-133)core/schemas/utils.go (1)
SafeExtractStringMap(527-547)
core/internal/testutil/file_base64.go (1)
core/internal/testutil/test_retry_framework.go (3)
GetTestRetryConfigForScenario(1186-1222)ResponsesRetryConfig(212-219)ResponsesRetryCondition(138-141)
core/providers/gemini/responses.go (1)
core/providers/gemini/types.go (1)
SafeExtractSafetySettings(103-133)
🔇 Additional comments (12)
core/providers/anthropic/responses.go (1)
1505-1516: LGTM! Good defensive check for empty instructions.The added
*bifrostReq.Params.Instructions != ""check correctly prevents empty instruction strings from being converted into Anthropic system messages with empty text content. This avoids sending unnecessary empty system content blocks to the API.Verify if similar patterns exist elsewhere that might benefit from the same empty string guard:
#!/bin/bash # Search for similar instruction handling patterns that might need empty string checks rg -n -A3 -B3 'Instructions\s*!=\s*nil' --type=go -g '!*_test.go' | head -80core/providers/utils/utils.go (1)
1563-1566: LGTM on adding deferred cleanup for panic safety.The defer statements ensure cleanup executes even if downstream code panics, which is a good defensive pattern.
core/schemas/utils.go (1)
525-547: LGTM!The
SafeExtractStringMapimplementation is well-structured:
- Handles both direct
map[string]stringand JSON-deserializedmap[string]interface{}cases.- Properly fails fast if any value cannot be converted to string.
- Follows the established pattern of other
SafeExtract*utilities in this file.core/providers/bedrock/utils.go (1)
278-282: LGTM!Using
SafeExtractStringMapinstead of a direct type assertion improves type safety by gracefully handling bothmap[string]stringand JSON-deserializedmap[string]interface{}inputs. This aligns with the broader pattern of safe extraction utilities being adopted across providers.core/internal/testutil/file_base64.go (1)
207-215: LGTM!This change aligns with the established test convention of using
GetTestRetryConfigForScenario()to obtain scenario-specific retry parameters, then constructing a typedResponsesRetryConfigwith an emptyConditionsslice. This makes the Responses API test consistent with the Chat Completions test pattern already present in lines 99 and 127-134.Based on learnings, this is the expected pattern for test files in this directory.
plugins/semanticcache/stream.go (1)
14-28: Mutex addition is good; explicitsync.Mutex{}init is optional.The new
mufield enables safe locking around accumulator mutations; the explicitsync.Mutex{}initializer can be omitted (zero value works).core/providers/gemini/gemini.go (1)
1035-1037: Good nil-guard forrequest.ParamsinSpeech.This prevents a potential panic when
Paramsis absent.core/providers/gemini/transcription.go (1)
114-133: Safer ExtraParams parsing is a clear improvement; add coverage for mixed-type inputs.Please ensure tests cover:
safety_settingscoming in as[]interface{}ofmap[string]anylabelscoming in asmap[string]any(and that invalid value types fail safely without panic)core/providers/gemini/responses.go (1)
123-128: Good replacement of unsafe assertion withSafeExtractSafetySettings.Please confirm there’s test coverage for
ExtraParams["safety_settings"]arriving as[]interface{}(e.g., from JSON/wasm) so we don’t regress silently.core/providers/gemini/types.go (1)
101-133: LGTM! Well-structured safe extraction utility.The function correctly handles multiple input types (direct
[]SafetySettingand JSON-deserialized[]interface{}), with proper nil checks and type assertions. The pattern aligns well withSafeExtractStringMapincore/schemas/utils.go.One observation: if a
map[string]interface{}item has none of the expected keys (method,category,threshold), an emptySafetySetting{}will still be appended. This is likely acceptable since the fields are optional per the struct definition, but worth noting for documentation purposes.core/providers/gemini/chat.go (1)
39-55: LGTM! Type safety improvements using centralized extraction utilities.The changes replace direct type assertions with the safer
SafeExtractSafetySettingsandschemas.SafeExtractStringMapfunctions. This prevents potential panics from unexpected input types (e.g., JSON-deserialized[]interface{}vs. direct[]SafetySetting) and aligns with similar updates intranscription.goandresponses.go.framework/streaming/types.go (1)
126-183: Thread-safety is correctly implemented throughout.The getter methods properly guard read access with mutex locks. Verification confirms that all write operations—appending to chunk slices and updating
MaxXxxChunkIndexfields—also acquire the same mutex inaddChatStreamChunk,addTranscriptionStreamChunk,addAudioStreamChunk, andaddResponsesStreamChunk. The defer pattern ensures safe cleanup on early returns. Complete thread-safety is achieved.
akshaydeo
force-pushed
the
01-12-review_fixes
branch
from
0649267 to
a07137f
Compare
akshaydeo
force-pushed
the
01-12-review_fixes
branch
from
a07137f to
bec61ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/providers/openai/models.go:
- Around line 40-42: The response struct OpenAIListModelsResponse is missing the
required Object field value per the OpenAI spec; set Object to "list" when
constructing openaiResponse (e.g., initialize OpenAIListModelsResponse with
Object: "list" and Data: make(...), or assign openaiResponse.Object = "list"
after creation) so the returned payload includes the correct Object value.
🧹 Nitpick comments (1)
plugins/governance/advancedscenarios_test.go (1)
458-458: Improved test reliability with explicit budget exhaustion tracking.The addition of the
budgetExhaustedflag provides a more robust assertion than relying onconsumedBudgetcomparison. This directly verifies that the server returned a budget rejection, which is the actual behavior under test.Minor consistency note: other similar tests in this file use
sawBudgetRejectionas the variable name (e.g., lines 576, 717, 876). Consider using consistent naming across tests for maintainability, though this is not blocking.♻️ Optional: Rename for consistency with other tests
- budgetExhausted := false + sawBudgetRejection := false- budgetExhausted = true + sawBudgetRejection = true- if !budgetExhausted { + if !sawBudgetRejection { t.Fatalf("Budget should have been exhausted within 150 requests, but no budget rejection was observed (consumed: $%.6f)", consumedBudget) }Also applies to: 475-475, 495-497
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
core/internal/testutil/file_base64.gocore/providers/anthropic/responses.gocore/providers/bedrock/utils.gocore/providers/gemini/chat.gocore/providers/gemini/gemini.gocore/providers/gemini/responses.gocore/providers/gemini/transcription.gocore/providers/gemini/types.gocore/providers/openai/models.gocore/providers/openai/types.gocore/providers/utils/utils.gocore/schemas/plugin_wasm.gocore/schemas/utils.goframework/streaming/accumulator.goframework/streaming/types.goplugins/governance/advancedscenarios_test.goplugins/semanticcache/stream.gotransports/bifrost-http/handlers/inference.go
✅ Files skipped from review due to trivial changes (1)
- core/providers/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (11)
- core/providers/anthropic/responses.go
- framework/streaming/accumulator.go
- core/providers/gemini/types.go
- core/providers/bedrock/utils.go
- core/internal/testutil/file_base64.go
- core/schemas/utils.go
- core/providers/gemini/gemini.go
- core/schemas/plugin_wasm.go
- plugins/semanticcache/stream.go
- core/providers/gemini/responses.go
- framework/streaming/types.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/gemini/chat.gotransports/bifrost-http/handlers/inference.gocore/providers/gemini/transcription.gocore/providers/openai/types.goplugins/governance/advancedscenarios_test.gocore/providers/openai/models.go
🧠 Learnings (9)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/gemini/chat.gotransports/bifrost-http/handlers/inference.gocore/providers/gemini/transcription.gocore/providers/openai/types.goplugins/governance/advancedscenarios_test.gocore/providers/openai/models.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
core/providers/gemini/chat.gotransports/bifrost-http/handlers/inference.gocore/providers/gemini/transcription.gocore/providers/openai/types.goplugins/governance/advancedscenarios_test.gocore/providers/openai/models.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/gemini/chat.gocore/providers/gemini/transcription.gocore/providers/openai/types.gocore/providers/openai/models.go
📚 Learning: 2026-01-10T11:27:47.535Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 1256
File: core/providers/openai/openai.go:2276-2385
Timestamp: 2026-01-10T11:27:47.535Z
Learning: Validate image generation requests for nil and missing prompts before dispatch. Follow the same pattern used here: core/bifrost.go validates nil/empty prompts, providerUtils.CheckContextAndGetRequestBody returns a structured error when the request converter yields nil, and apply this across all providers (including OpenAI) to avoid sending null bodies.
Applied to files:
core/providers/gemini/chat.gocore/providers/gemini/transcription.gocore/providers/openai/types.gocore/providers/openai/models.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/inference.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/handlers/inference.go
📚 Learning: 2025-12-11T11:58:25.307Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: core/providers/openai/responses.go:42-84
Timestamp: 2025-12-11T11:58:25.307Z
Learning: In core/providers/openai/responses.go (and related OpenAI response handling), document and enforce the API format constraint: if ResponsesReasoning != nil and the response contains content blocks, all content blocks should be treated as reasoning blocks by default. Implement type guards or parsing logic accordingly, and add unit tests to verify that when ResponsesReasoning is non-nil, content blocks are labeled as reasoning blocks. Include clear comments in the code explaining the rationale and ensure downstream consumers rely on this behavior.
Applied to files:
core/providers/openai/types.gocore/providers/openai/models.go
📚 Learning: 2025-12-14T14:43:30.902Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 980
File: core/providers/openai/images.go:10-22
Timestamp: 2025-12-14T14:43:30.902Z
Learning: Enforce the OpenAI image generation SSE event type values across the OpenAI image flow in the repository: use "image_generation.partial_image" for partial chunks, "image_generation.completed" for the final result, and "error" for errors. Apply this consistently in schemas, constants, tests, accumulator routing, and UI code within core/providers/openai (and related Go files) to ensure uniform event typing and avoid mismatches.
Applied to files:
core/providers/openai/types.gocore/providers/openai/models.go
📚 Learning: 2025-12-22T10:50:40.990Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1154
File: plugins/governance/store.go:1165-1186
Timestamp: 2025-12-22T10:50:40.990Z
Learning: In the Bifrost governance plugin, budgets and rate limits have 1:1 relationships with their parent entities (virtual keys, teams, customers). Do not assume sharing; ensure cascade deletion logic only deletes budgets/rate limits when there are no shared references. Enforce invariants in code and add tests to verify no cross-entity sharing and that cascade deletes only remove the specific child of the parent. If a counterexample arises, adjust data model or add guards.
Applied to files:
plugins/governance/advancedscenarios_test.go
🧬 Code graph analysis (3)
core/providers/gemini/chat.go (2)
core/providers/gemini/types.go (1)
SafeExtractSafetySettings(103-133)core/schemas/utils.go (1)
SafeExtractStringMap(527-547)
transports/bifrost-http/handlers/inference.go (2)
core/schemas/utils.go (1)
Ptr(14-16)core/utils.go (1)
Ptr(56-58)
core/providers/gemini/transcription.go (2)
core/providers/gemini/types.go (1)
SafeExtractSafetySettings(103-133)core/schemas/utils.go (1)
SafeExtractStringMap(527-547)
🔇 Additional comments (7)
core/providers/gemini/transcription.go (2)
117-121: LGTM! Proper use of safe extraction for safety settings.The change from direct type assertion to
SafeExtractSafetySettingshandles multiple input types ([]SafetySettingand[]interface{}) and prevents panics from malformed data. This is consistent with the same pattern applied inchat.go.
129-133: LGTM! Consistent safe extraction for labels.Using
schemas.SafeExtractStringMapcorrectly handles bothmap[string]stringandmap[string]interface{}inputs, providing robust conversion with graceful failure handling.core/providers/gemini/chat.go (2)
39-43: LGTM! Safe extraction prevents runtime panics.Replacing direct type assertion with
SafeExtractSafetySettingsis the correct approach. The helper function (fromtypes.go:102-132) handles both native[]SafetySettingand JSON-deserialized[]interface{}inputs, which is essential when data passes through different serialization layers.
51-55: LGTM! Consistent with transcription.go and other Gemini handlers.The
schemas.SafeExtractStringMaphelper ensures that labels can be extracted whether they come asmap[string]stringormap[string]interface{}, providing resilience against type variations from JSON unmarshaling.transports/bifrost-http/handlers/inference.go (1)
509-516: LGTM! Pricing precision fix correctly addresses issue #1306.The change from
%f(6 decimal places) to%.10f(10 decimal places) ensures that very small per-token prices (e.g., 2e-7 = 0.0000002) are no longer truncated to "0.000000". The fix is applied consistently across all pricing fields.core/providers/openai/types.go (1)
528-544: LGTM! Well-structured OpenAI model types.The type definitions correctly model the OpenAI API response structure with appropriate optional fields using pointer types and
omitemptytags. The GROQ-specific fields are properly documented.core/providers/openai/models.go (1)
9-33: LGTM! Conversion logic is correct.The function properly handles nil input, filters by allowed models when specified, and correctly maps all fields including the provider-prefixed ID.
2 participants
Summary
This PR fixes several type safety issues in provider implementations, improves error handling, and enhances stream accumulator thread safety to prevent race conditions.
Fixes
Closes #1306
Changes
SafeExtractStringMapType of change
Affected areas
How to test
Breaking changes
Related issues
Fixes type safety issues and race conditions in stream processing.
Security considerations
Improves type safety which helps prevent potential security issues from unexpected data types.
Checklist