feat: add reasoning support for chat completions #978
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactors reasoning from a flat field to a structured object with max_tokens, adds validation (minimum 1024), surfaces reasoning and signatures in streaming deltas across providers, and converts several converter functions to return errors so callers propagate validation failures and token-usage caching into streaming usage reporting. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
d24ab88 to
e1c5d21
Compare
Pratham-Mishra04
force-pushed
the
12-01-refactor_core_providers_error_handling_standardization
branch
from
78eb82c to
3540e84
Compare
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
e1c5d21 to
b5f6f04
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/jsonparser/utils.go (1)
303-323: Preservereasoning_detailsin deep copy of stream deltas
deepCopyChatStreamResponseChoiceDeltanow copiesReasoning, but it still omits the newReasoningDetailsslice. When the JSON parser plugin is enabled, this means anyreasoning_detailson the original delta will be dropped in the copied response.You likely want to keep these details intact, similar to
RefusalandToolCalls:func (p *JsonParserPlugin) deepCopyChatStreamResponseChoiceDelta(original *schemas.ChatStreamResponseChoiceDelta) *schemas.ChatStreamResponseChoiceDelta { if original == nil { return nil } result := &schemas.ChatStreamResponseChoiceDelta{ - Role: original.Role, - Reasoning: original.Reasoning, // Shallow copy - Refusal: original.Refusal, // Shallow copy - ToolCalls: original.ToolCalls, // Shallow copy - we don't modify tool calls + Role: original.Role, + Reasoning: original.Reasoning, // Shallow copy + ReasoningDetails: original.ReasoningDetails, // Shallow copy - not modified by plugin + Refusal: original.Refusal, // Shallow copy + ToolCalls: original.ToolCalls, // Shallow copy - we don't modify tool calls } // Deep copy Content pointer if it exists (this is what we modify) if original.Content != nil { contentCopy := *original.Content result.Content = &contentCopy } return result }core/providers/anthropic/responses.go (1)
296-403: Fix Type field and Nil Effort handling in Anthropic reasoning logicThe
ToAnthropicResponsesRequestreasoning block has two critical bugs:
AnthropicThinking.Type incorrectly set to Effort string. Line 332 sets
Type: string(*bifrostReq.Params.Reasoning.Effort), but the field is documented to accept only"enabled"or"disabled". Effort values like"minimal","low","medium","high","auto"are invalid and will create malformed Anthropic requests.Nil Effort semantics inconsistent with chat path and Bedrock. The current condition (line 326)
Effort != nil && Effort != "none"means nil Effort → disabled. However, the chat.go path and Bedrock both treat nil Effort as enabled (only"none"triggers disabled). This causes the same input to enable reasoning on one path and disable it on another.Apply the suggested fix:
- Change condition to
Effort == nil || Effort != "none"- Change Type to
"enabled"(not the Effort string)
🧹 Nitpick comments (11)
core/schemas/responses.go (1)
231-236: Responses reasoning schema extension is consistentAdding
MaxTokens *int \json:"max_tokens,omitempty"`alongsideEffort` cleanly models reasoning configuration for Responses; this struct remains backward compatible and matches how chat reasoning is modeled elsewhere. Any provider-specific constraints (e.g., Anthropics’s minimum) can be enforced in the converters rather than here.core/schemas/chatcompletions.go (2)
154-185: ChatParameters reasoning migration and legacy support look correctThe
ChatParametersUnmarshalJSON alias pattern cleanly:
- Accepts the new nested
reasoningobject,- Maps legacy
reasoning_effortintoChatReasoning.Effort, and- Rejects payloads that set both, which prevents ambiguous configuration.
ChatReasoningitself mirrors the Responses reasoning shape and keeps the Anthropics-specificmax_tokensas an optional field, which is appropriate here.Also applies to: 186-220, 222-226
468-478: Reasoning details synthesis across assistant messages and streaming deltas is well‑designedThe new JSON handling for:
ChatMessage.UnmarshalJSON(splitting base fields vs embedded assistant/tool structs),ChatAssistantMessage.UnmarshalJSON, andChatStreamResponseChoiceDelta.UnmarshalJSONcorrectly keeps the core message fields intact while auto‑populating
ReasoningDetailsfrom the flatreasoningstring when no details are present. This gives downstream code a single, structured way to consume reasoning without breaking existing payloads that only sendreasoning. The mutual‑exclusion checks and alias usage avoid recursion issues.Also applies to: 480-521, 625-667, 714-731, 755-763, 765-794
core/providers/openai/openai.go (1)
981-988: LGTM! Token details propagation matches text completion pattern.The propagation logic is correct. Minor note: the field order here (PromptTokensDetails, then CompletionTokensDetails) differs from the text completion streaming path (lines 515-520) which does CompletionTokensDetails first. Consider aligning the order for consistency, though this has no functional impact.
framework/streaming/chat.go (1)
55-105: LGTM! ReasoningDetails merging logic is correct.The merge-by-Index approach correctly handles:
- Text, Summary, Data: accumulated via string concatenation
- Signature, Type: overwritten (appropriate for final/immutable values)
- New details: appended when Index not found
The nested loop is O(n×m) but acceptable given typical small detail counts.
For improved readability, consider extracting the detail update logic into a helper function:
func mergeReasoningDetail(existing, incoming *schemas.ChatReasoningDetails) { if incoming.Text != nil { if existing.Text == nil { existing.Text = incoming.Text } else { *existing.Text += *incoming.Text } } // ... similar for other fields }core/providers/openai/utils.go (1)
5-24: Converters look correct; note that ReasoningDetails is intentionally omitted.The OpenAI↔Bifrost message converters correctly map name, role, content, tool fields, and assistant metadata (refusal, reasoning, annotations, tool calls). They don’t map
ChatAssistantMessage.ReasoningDetails, which is fine as long as we never expect those details to survive a round‑trip through OpenAI models; if that becomes a requirement, we’ll need to extendOpenAIChatAssistantMessageor carry details elsewhere.Also applies to: 26-45
core/providers/bedrock/chat.go (1)
58-60: Reasoning extraction for Bedrock chat looks correct; only minor polish possible.
- In
ToBifrostChatResponse, reasoning text/signature fromReasoningContentblocks are mapped intoChatReasoningDetails(type: reasoning.text) and concatenated into a singleassistantMessage.Reasoningstring. The single‑text‑block unwrap (promoting it toContentStr) is a nice compatibility touch.- In
ToBifrostChatCompletionStream,ReasoningContentdeltas emit chunks withReasoningplus a one‑elementReasoningDetailsarray, and signature‑only deltas emitReasoningDetailswith a populatedSignaturefield.Two small nits you might consider (not blockers):
reasoningTextaccumulates with a trailing newline; trimming the final\nbefore assigning toassistantMessage.Reasoningwould avoid a cosmetic artifact.- For signature‑only deltas you reuse
ChatReasoningDetailsTypeText; if the intention is to distinguish encrypted/signature‑only entries (e.g.,"reasoning.encrypted"), adjusting theTypewould make that clearer to downstream consumers.Functionally this is solid and matches the new reasoning schema.
Also applies to: 103-112, 117-121, 135-141, 293-341
core/providers/cohere/chat.go (2)
7-7: Cross-provider coupling: Cohere uses Anthropic's minimum token constant.The Cohere provider imports and uses
anthropic.MinimumReasoningMaxTokensfor validation. This creates a coupling where Cohere's validation depends on Anthropic's constraints.Consider extracting this constant to a shared location (e.g.,
schemaspackage or a dedicatedreasoningconstants file) to decouple providers:-import ( - "github.com/maximhq/bifrost/core/providers/anthropic" -) +import ( + "github.com/maximhq/bifrost/core/schemas" +)Then use
schemas.MinimumReasoningMaxTokensor similar. This makes the validation logic self-contained and allows each provider to potentially define its own minimum in the future.Also applies to: 112-113
598-598: Minor: Trailing newline appended to reasoningText.Line 598 appends
"\n"after each thinking block's text, which will leave a trailing newline in the finalreasoningText. This is consistent with the Anthropic implementation (line 355) but may be undesirable.Consider trimming the trailing newline when setting the final reasoning text, or use
strings.Joinwith newline separator:// Alternative approach using strings.Builder or Join reasoningText = strings.TrimSuffix(reasoningText, "\n")transports/bifrost-http/handlers/inference.go (1)
63-89: Custom ChatRequest unmarshalling and reasoning handling look correct
- Adding
"reasoning"tochatParamsKnownFieldsand wiring it throughChatParameterskeeps reasoning out ofExtraParamsand aligned with the new structuredReasoningobject.- The
ChatRequest.UnmarshalJSONimplementation cleanly separates concerns: it independently unmarshalsBifrostParams,Messages, and then delegates toChatParameters’s own unmarshaller, which avoids the embedded-struct interference you mention in the comment.- Switching
extractExtraParamsover tosonic.Unmarshalkeeps JSON parsing consistent with the rest of the file, while still usingjson.RawMessageas a lightweight buffer type.- Initializing
req := ChatRequest{ChatParameters: &schemas.ChatParameters{}}before unmarshalling is a good defensive step so the custom unmarshaller can safely populate parameters.If this path ever shows up as a hot spot, you could reduce the triple-unmarshal in
ChatRequest.UnmarshalJSONby unmarshalling once into a small helper struct that embeds all three views, but that’s purely an optimization, not a correctness issue.Also applies to: 169-199, 268-288, 472-509
core/providers/anthropic/anthropic.go (1)
303-311: Anthropic conversion error handling is solid; consider aligning cached-token aggregation
- Using
ToAnthropicChatRequest/ToAnthropicResponsesRequestinside theCheckContextAndGetRequestBodyclosures with expliciterrchecks is the right way to surface conversion failures early and avoid sending bad payloads.- Enabling
Stream = Ptr(true)only after a successful conversion in both chat and responses streaming paths is also correct.For the new cached-token usage fields:
- In
HandleAnthropicChatCompletionStreaming, you currently overwritePromptTokensDetails/CompletionTokensDetailseach timeusageToProcesshasCacheReadInputTokensorCacheCreationInputTokens.- In
HandleAnthropicResponsesStream, you do the same forInputTokensDetails/OutputTokensDetails.If Anthropic ever emits multiple usage records per stream, you might want to mirror the “take the max seen so far” pattern you already use for token counts (and that Bedrock uses for cached tokens) instead of blind overwrite, e.g.:
if usageToProcess.CacheReadInputTokens > 0 { if usage.PromptTokensDetails == nil { usage.PromptTokensDetails = &schemas.ChatPromptTokensDetails{} } if usageToProcess.CacheReadInputTokens > usage.PromptTokensDetails.CachedTokens { usage.PromptTokensDetails.CachedTokens = usageToProcess.CacheReadInputTokens } }Same idea for the other cached-token fields. Not strictly required now, but it keeps aggregation semantics consistent across providers.
Also applies to: 354-365, 551-560, 647-655, 696-707, 893-902
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
core/providers/anthropic/anthropic.go(6 hunks)core/providers/anthropic/chat.go(10 hunks)core/providers/anthropic/errors.go(1 hunks)core/providers/anthropic/responses.go(4 hunks)core/providers/anthropic/types.go(2 hunks)core/providers/azure/azure.go(4 hunks)core/providers/bedrock/bedrock.go(4 hunks)core/providers/bedrock/chat.go(5 hunks)core/providers/bedrock/responses.go(4 hunks)core/providers/bedrock/types.go(3 hunks)core/providers/bedrock/utils.go(3 hunks)core/providers/cohere/chat.go(10 hunks)core/providers/cohere/cohere.go(2 hunks)core/providers/cohere/types.go(1 hunks)core/providers/openai/chat.go(3 hunks)core/providers/openai/openai.go(2 hunks)core/providers/openai/types.go(2 hunks)core/providers/openai/utils.go(1 hunks)core/providers/perplexity/chat.go(1 hunks)core/providers/vertex/vertex.go(4 hunks)core/schemas/chatcompletions.go(7 hunks)core/schemas/mux.go(3 hunks)core/schemas/responses.go(1 hunks)framework/streaming/chat.go(1 hunks)plugins/jsonparser/utils.go(1 hunks)plugins/semanticcache/utils.go(1 hunks)transports/bifrost-http/handlers/inference.go(5 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/lib/types/logs.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
core/providers/anthropic/errors.go (2)
ui/lib/types/logs.ts (1)
BifrostError(233-239)core/providers/anthropic/types.go (2)
AnthropicMessageError(376-379)AnthropicMessageErrorStruct(382-385)
core/providers/openai/openai.go (3)
plugins/mocker/main.go (1)
Usage(138-142)core/providers/perplexity/types.go (1)
Usage(69-78)ui/lib/types/logs.ts (1)
CompletionTokensDetails(197-202)
core/providers/vertex/vertex.go (2)
core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(13-282)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(296-403)
ui/app/workspace/logs/views/logChatMessageView.tsx (2)
ui/lib/utils/validation.ts (2)
isJson(539-546)cleanJson(548-558)ui/app/workspace/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
transports/bifrost-http/handlers/inference.go (2)
core/schemas/chatcompletions.go (2)
ChatMessage(469-478)ChatParameters(155-184)ui/lib/types/logs.ts (1)
ChatMessage(115-124)
core/schemas/mux.go (3)
core/schemas/responses.go (3)
ResponsesParametersReasoning(231-236)BifrostResponsesStreamResponse(1422-1460)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1388-1388)core/schemas/chatcompletions.go (1)
ChatReasoning(223-226)core/schemas/utils.go (1)
Ptr(16-18)
core/providers/openai/utils.go (3)
core/providers/openai/types.go (2)
OpenAIMessage(56-65)OpenAIChatAssistantMessage(67-72)core/schemas/chatcompletions.go (3)
ChatMessage(469-478)ChatToolMessage(621-623)ChatAssistantMessage(626-632)ui/lib/types/logs.ts (1)
ChatMessage(115-124)
core/providers/azure/azure.go (4)
core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(13-282)core/utils.go (1)
Ptr(51-53)core/schemas/utils.go (1)
Ptr(16-18)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(296-403)
core/providers/anthropic/responses.go (3)
core/schemas/responses.go (1)
ResponsesParametersReasoning(231-236)core/schemas/utils.go (1)
Ptr(16-18)core/providers/anthropic/types.go (2)
MinimumReasoningMaxTokens(14-14)AnthropicThinking(65-68)
core/providers/openai/chat.go (3)
core/providers/openai/utils.go (2)
ConvertOpenAIMessagesToBifrostMessages(5-24)ConvertBifrostMessagesToOpenAIMessages(26-45)core/schemas/chatcompletions.go (1)
ChatParameters(155-184)core/utils.go (1)
Ptr(51-53)
core/providers/anthropic/anthropic.go (7)
core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(13-282)core/utils.go (1)
Ptr(51-53)core/schemas/utils.go (1)
Ptr(16-18)core/schemas/chatcompletions.go (1)
ChatPromptTokensDetails(821-827)ui/lib/types/logs.ts (3)
CompletionTokensDetails(197-202)ResponsesResponseInputTokens(542-544)ResponsesResponseOutputTokens(546-548)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(296-403)core/schemas/responses.go (2)
ResponsesResponseInputTokens(260-266)ResponsesResponseOutputTokens(268-278)
core/providers/bedrock/bedrock.go (2)
core/providers/utils/utils.go (1)
CreateBifrostChatCompletionChunkResponse(687-716)core/schemas/bifrost.go (1)
ChatCompletionStreamRequest(90-90)
core/providers/bedrock/responses.go (4)
core/schemas/utils.go (4)
SafeExtractFromMap(519-525)SafeExtractString(280-297)SafeExtractInt(300-342)Ptr(16-18)core/schemas/responses.go (1)
ResponsesParametersReasoning(231-236)core/schemas/chatcompletions.go (1)
OrderedMap(268-268)core/providers/anthropic/types.go (1)
MinimumReasoningMaxTokens(14-14)
core/providers/cohere/cohere.go (1)
core/providers/cohere/chat.go (1)
ToCohereChatCompletionRequest(12-199)
core/providers/openai/types.go (2)
core/schemas/models.go (1)
Model(109-129)core/schemas/chatcompletions.go (9)
ChatMessageRole(457-457)ChatMessageContent(524-527)ChatToolMessage(621-623)ChatAssistantMessageAnnotation(670-673)ChatAssistantMessageToolCall(686-692)Alias(189-189)Alias(643-643)Alias(770-770)Alias(860-860)
core/schemas/chatcompletions.go (4)
core/providers/openai/types.go (1)
Alias(78-78)core/schemas/bifrost.go (2)
Alias(383-383)Alias(400-400)ui/lib/types/logs.ts (3)
ChatMessage(115-124)ChatMessageContent(113-113)ReasoningDetails(126-133)core/providers/gemini/types.go (2)
Role(13-13)Content(922-930)
core/providers/bedrock/utils.go (3)
core/schemas/chatcompletions.go (2)
BifrostChatRequest(12-19)OrderedMap(268-268)core/providers/bedrock/types.go (1)
BedrockConverseRequest(44-57)core/providers/anthropic/types.go (1)
MinimumReasoningMaxTokens(14-14)
framework/streaming/chat.go (4)
core/schemas/chatcompletions.go (1)
ChatAssistantMessage(626-632)core/utils.go (1)
Ptr(51-53)ui/lib/types/logs.ts (1)
ReasoningDetails(126-133)core/providers/gemini/types.go (1)
Type(778-778)
core/providers/bedrock/chat.go (2)
core/schemas/chatcompletions.go (4)
ChatReasoningDetails(723-730)ChatReasoningDetailsTypeText(719-719)ChatContentBlockTypeText(582-582)ChatAssistantMessage(626-632)ui/lib/types/logs.ts (1)
ReasoningDetails(126-133)
core/providers/anthropic/chat.go (2)
core/providers/anthropic/types.go (7)
AnthropicMessageRequest(40-59)AnthropicThinking(65-68)MinimumReasoningMaxTokens(14-14)AnthropicContentBlock(147-159)AnthropicContentBlockTypeThinking(143-143)AnthropicContentBlockTypeText(135-135)AnthropicContentBlockTypeToolUse(137-137)core/providers/anthropic/utils.go (1)
ConvertAnthropicFinishReasonToBifrost(27-32)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (34)
ui/lib/types/logs.ts (2)
122-124: LGTM! Type definitions properly structured.The addition of
reasoningandreasoning_detailsfields toChatMessageis well-designed and aligns with the PR's goal of adding unified reasoning support. The optional fields ensure backward compatibility.
126-133: LGTM! Well-designed interface for structured reasoning data.The
ReasoningDetailsinterface is properly structured with appropriate discriminated union types and optional fields that support various reasoning formats (summary, encrypted, text) as mentioned in the PR objectives.ui/app/workspace/logs/views/logChatMessageView.tsx (1)
73-94: Consistent renaming from thought to reasoning.The update is well-executed with consistent changes throughout the rendering logic. The JSON/plain-text rendering approach is maintained appropriately.
Note: The
reasoning_details?: ReasoningDetails[]field in the ChatMessage type is currently not rendered in this view, only themessage.reasoningstring is displayed. This mirrors how therefusalfield is handled (string only, despite optional detailed fields in the type). Verify whether this is intentional (e.g., for future use) or ifreasoning_detailsshould be rendered to display signatures and verification data as suggested by the PR objectives.core/providers/anthropic/types.go (1)
11-15: Reasoning min-tokens constant looks appropriateDefining
MinimumReasoningMaxTokens = 1024at the Anthropic provider level is a good way to centralize the API’s minimum reasoning budget; no issues from this file alone.core/providers/perplexity/chat.go (1)
29-35: Reasoning effort mapping correctly updated to nested paramsThe switch to
bifrostReq.Params.Reasoning.Effortwhile preserving the"minimal" → "low"mapping keeps Perplexity behaviour consistent with the legacyreasoning_effortfield and aligns with the newChatReasoningshape. No further changes needed here.core/providers/bedrock/types.go (1)
83-108: Bedrock reasoning content wiring is structurally soundThe new
ReasoningContentfields andBedrockReasoningContent{,Text}types integrate cleanly into both full content blocks and streaming deltas, and mirror the nested structure used elsewhere in the Bedrock path. From this file alone the types look correct and ready to be consumed by the Bedrock chat/response converters.Also applies to: 152-160, 436-441
core/providers/cohere/types.go (1)
519-524: Streaming thinking deltas are wired consistentlyExtending
CohereStreamContentwithThinking *stringreuses the existing thinking shape fromCohereContentBlockand cleanly surfaces thinking deltas in the stream; no issues found.core/schemas/chatcompletions.go (1)
829-836: ImageTokens field addition is schema-only; provider mapper integration pendingThe new
ImageTokens *intfield inChatCompletionTokensDetailsadds a forward-compatible extension to token accounting, similar toAudioTokensandReasoningTokens. However, verification shows no provider mappers currently populate this field—none of the provider implementations reference or assign values toimage_tokens. If provider support is intended, the mapper implementations need to be updated to extract and populate this field from provider responses.core/providers/cohere/cohere.go (2)
279-286: LGTM! Proper error propagation from request converter.The change correctly propagates errors from
ToCohereChatCompletionRequestthrough the request body builder, aligning with the error-first pattern established across providers in this PR.
331-342: LGTM! Streaming path now properly handles conversion errors.Good defensive pattern: the error is captured and checked before enabling streaming mode. This ensures conversion failures surface to the caller rather than being silently ignored.
core/providers/openai/openai.go (1)
515-521: LGTM! Token details propagation for streaming.Correctly propagates
CompletionTokensDetailsandPromptTokensDetails(includingreasoning_tokens) into the aggregated usage for text completion streams.core/providers/openai/chat.go (3)
14-14: LGTM! Uses centralized message conversion utility.The change to
ConvertOpenAIMessagesToBifrostMessagesproperly handles the new message structure includingChatAssistantMessagefields likeReasoningandReasoningDetails.
28-28: LGTM! Bidirectional conversion with new utility.Uses
ConvertBifrostMessagesToOpenAIMessagesfor the reverse transformation, maintaining consistency with the input conversion path.
62-65: LGTM! Nil-safe nested field access for reasoning effort.The triple nil-check (
Reasoning != nil && Reasoning.Effort != nil && *Effort == "minimal") correctly handles the transition from flatReasoningEffortto nestedReasoning.Effort. The "minimal" to "low" mapping maintains compatibility with non-OpenAI providers.core/providers/bedrock/utils.go (3)
9-11: LGTM! Import for reasoning validation constant.The import of
anthropic.MinimumReasoningMaxTokensenables consistent validation of reasoning token budgets across providers.
14-17: LGTM! Error return enables proper error propagation.The function signature change to return
errorallows callers to handle conversion failures gracefully. The nil check for params is appropriate.
133-133: LGTM! Explicit success return.The
return nilon successful completion completes the error-returning function contract.framework/streaming/chat.go (2)
38-43: LGTM! Consistent pointer initialization for Refusal.Using
bifrost.Ptr(*chunk.Delta.Refusal)aligns with the pattern used elsewhere in the codebase for initializing optional string pointers.
44-54: LGTM! Reasoning content accumulation.The reasoning accumulation logic correctly mirrors the existing Refusal handling pattern, properly initializing
ChatAssistantMessageif needed and concatenating reasoning content across chunks.core/providers/bedrock/responses.go (1)
656-660: New Bedrock→Responses output mapping is straightforward and consistent.The added conversion of
response.Output.MessageviaconvertBedrockMessageToResponsesMessagescleanly populatesbifrostResp.Outputand aligns with the existing Bedrock↔Responses content‑block helpers. No obvious edge‑case or correctness issues here.core/providers/anthropic/responses.go (1)
190-201: Thinking→Reasoning mapping on inbound Anthropic requests looks sound.
AnthropicMessageRequest.Thinkingis correctly surfaced asResponsesParametersReasoning:"enabled"becomesEffort: "auto"withMaxTokensfromBudgetTokens, and any other type producesEffort: "none". This gives Bifrost a normalized view without over‑encoding Anthropic‑specific details.core/providers/bedrock/chat.go (1)
37-40: Good: convertChatParameters errors are now surfaced to callers.Wrapping
convertChatParameterswith error propagation prevents silently constructing invalid Bedrock requests (e.g., bad reasoning.max_tokens) and aligns this path with the new validation logic in the utilities.core/providers/azure/azure.go (1)
361-373: Azure–Anthropic integration correctly handles new error‑returning converters.In all Anthropic branches (chat, chat stream, responses, responses stream):
- You now call
anthropic.ToAnthropicChatRequest/ToAnthropicResponsesRequest, check the returned error, and abort early if non‑nil instead of assuming success.- On success, you override
reqBody.Modelwith the Azure deployment and, for streaming paths, setreqBody.Stream = Ptr(true)before serialization.This keeps the Azure provider aligned with the new Anthropic converter signatures and ensures invalid reasoning configs or other request issues surface as proper errors instead of malformed upstream calls.
Also applies to: 461-471, 537-547, 636-646
core/providers/openai/types.go (2)
56-65: Embedded pointer structs design is sound, but the mutual exclusion is only documented.The comment on lines 61-62 correctly warns that only one embedded pointer can be non-nil at a time. However, this invariant is enforced only by convention. Consider adding a validation method or documenting this more prominently in the consuming code to prevent accidental misuse.
74-101: Custom MarshalJSON correctly shadows and transforms the reasoning field.The implementation properly:
- Uses the type alias pattern to avoid infinite recursion
- Shadows the
Reasoningfield to omit it from JSON output- Extracts
Reasoning.Effortintoreasoning_effortfor OpenAI compatibilitycore/providers/anthropic/chat.go (5)
421-435: Cached token mapping appears semantically inverted.The code maps
CacheReadInputTokenstoPromptTokensDetails.CachedTokens(line 422) andCacheCreationInputTokenstoCompletionTokensDetails.CachedTokens(lines 426, 434).Anthropic's
cache_creation_input_tokensrepresents tokens written to cache (a cost incurred during input processing), whilecache_read_input_tokensrepresents tokens read from cache. Both are input-side metrics. Mapping cache creation tokens toCompletionTokensDetailsseems semantically incorrect since completion tokens typically refer to output tokens.Please verify the intended semantics of these token mappings. If cache creation tokens should be tracked separately from completion details, consider adding a dedicated field or documenting this mapping decision.
647-654: Reasoning details in stream deltas include both summary and details.The streaming response correctly populates both
Reasoning(summary text) andReasoningDetails(structured details) for thinking content. This provides flexibility for consumers.
664-687: Signature streaming correctly creates separate reasoning detail entries.The signature delta handling properly creates a
ReasoningDetailsentry with just the signature, allowing consumers to associate signatures with their corresponding thinking blocks.
744-751: Reasoning-to-thinking conversion in stream response is correct.The conversion from Bifrost's
delta.Reasoningto Anthropic'sThinkingdelta type maintains consistency with the forward conversion path.
118-122: Unreachable code: condition on line 118 can never be true.The condition
bifrostReq.Params.Reasoning != nilon line 118 is inside anelseblock that only executes whenbifrostReq.Params == nil(from line 25). SinceParamsis nil, accessingParams.Reasoningwould cause a nil pointer dereference if the condition were ever evaluated.However, looking more closely, line 118 is at the same indentation level as line 122's closing brace, suggesting this is meant to be outside the
if bifrostReq.Params != nilblock but the logic is incorrect.- } else if bifrostReq.Params.Reasoning != nil && bifrostReq.Params.Reasoning.Effort != nil && *bifrostReq.Params.Reasoning.Effort == "none" { - anthropicReq.Thinking = &AnthropicThinking{ - Type: "disabled", - } }This dead code should be removed. If
bifrostReq.Paramsis nil, thenbifrostReq.Params.Reasoningcannot be accessed.Likely an incorrect or invalid review comment.
core/providers/cohere/chat.go (2)
431-446: Thinking content streaming correctly populates both Reasoning and ReasoningDetails.The streaming handler for thinking content mirrors the Anthropic implementation, providing consistent behavior across providers.
556-672: ToBifrostChatMessage implementation is thorough and handles edge cases well.The method correctly:
- Handles string content, block content, and thinking blocks
- Safely handles nil Function in tool calls (lines 619-622)
- Collapses single text blocks to ContentStr (lines 604-607)
- Accumulates reasoning details with proper indexing
core/providers/bedrock/bedrock.go (1)
22-22: Consistent per-stream UUID looks goodGenerating a single UUID per Bedrock chat stream and reusing it across all chunks and the final summary response is a clean way to provide a stable ID when the provider doesn’t. No functional issues spotted.
Also applies to: 783-785, 871-872, 894-895
core/providers/vertex/vertex.go (1)
291-345: Error propagation from Anthropic converters is correct and saferUsing
ToAnthropicChatRequest/ToAnthropicResponsesRequestwith expliciterrchecks inside theCheckContextAndGetRequestBodyclosures is the right pattern here and avoids streaming or sending requests when conversion fails. The updated blocks inChatCompletion,ChatCompletionStream,Responses, andResponsesStreamall look consistent.Also applies to: 532-539, 696-707, 860-868
| // ToAnthropicChatCompletionError converts a BifrostError to AnthropicMessageError | ||
| func ToAnthropicChatCompletionError(bifrostErr *schemas.BifrostError) *AnthropicMessageError { | ||
| if bifrostErr == nil { | ||
| return nil | ||
| } | ||
|
|
||
| // Provide blank strings for nil pointer fields | ||
| errorType := "" | ||
| if bifrostErr.Type != nil { | ||
| errorType = *bifrostErr.Type | ||
| } | ||
|
|
||
| // Handle nested error fields with nil checks | ||
| errorStruct := AnthropicMessageErrorStruct{ | ||
| Type: errorType, | ||
| Message: bifrostErr.Error.Message, | ||
| } | ||
|
|
||
| return &AnthropicMessageError{ | ||
| Type: "error", // always "error" for Anthropic | ||
| Error: errorStruct, | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against nil bifrostErr.Error in Anthropic error conversion
ToAnthropicChatCompletionError assumes bifrostErr.Error is non‑nil and does:
Message: bifrostErr.Error.Message,If a caller ever passes a BifrostError with Error == nil, this will panic. Even if today’s producers always populate Error, it’s safer for a helper like this to be defensive.
Consider:
func ToAnthropicChatCompletionError(bifrostErr *schemas.BifrostError) *AnthropicMessageError {
if bifrostErr == nil {
return nil
}
// Provide blank strings for nil pointer fields
errorType := ""
if bifrostErr.Type != nil {
errorType = *bifrostErr.Type
}
+
+ // Safely extract message from nested error
+ message := ""
+ if bifrostErr.Error != nil {
+ message = bifrostErr.Error.Message
+ }
// Handle nested error fields with nil checks
errorStruct := AnthropicMessageErrorStruct{
- Type: errorType,
- Message: bifrostErr.Error.Message,
+ Type: errorType,
+ Message: message,
}This keeps the function robust across all BifrostError sources.
🤖 Prompt for AI Agents
In core/providers/anthropic/errors.go around lines 9 to 31, the conversion
assumes bifrostErr.Error is non-nil and directly accesses
bifrostErr.Error.Message which can panic; add a nil check for bifrostErr.Error
and provide a safe default (e.g., empty string) for Message (and any other
nested fields) before constructing AnthropicMessageErrorStruct so the function
never dereferences a nil pointer.
|
|
||
| // Convert reasoning config | ||
| if bifrostReq.Params.Reasoning != nil { | ||
| if bedrockReq.AdditionalModelRequestFields == nil { | ||
| bedrockReq.AdditionalModelRequestFields = make(schemas.OrderedMap) | ||
| } | ||
| if bifrostReq.Params.Reasoning.Effort != nil && *bifrostReq.Params.Reasoning.Effort == "none" { | ||
| bedrockReq.AdditionalModelRequestFields["reasoning_config"] = map[string]string{ | ||
| "type": "disabled", | ||
| } | ||
| } else { | ||
| if bifrostReq.Params.Reasoning.MaxTokens == nil { | ||
| return fmt.Errorf("reasoning.max_tokens is required for reasoning") | ||
| } else if *bifrostReq.Params.Reasoning.MaxTokens < anthropic.MinimumReasoningMaxTokens { | ||
| return fmt.Errorf("reasoning.max_tokens must be greater than or equal to %d", anthropic.MinimumReasoningMaxTokens) | ||
| } | ||
| bedrockReq.AdditionalModelRequestFields["reasoning_config"] = map[string]any{ | ||
| "type": "enabled", | ||
| "budget_tokens": *bifrostReq.Params.Reasoning.MaxTokens, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Reasoning config may be overwritten by ExtraParams handling.
The reasoning config is correctly added to AdditionalModelRequestFields, but lines 70-75 can overwrite the entire map if additionalModelRequestFieldPaths exists in ExtraParams:
if orderedFields, ok := schemas.SafeExtractOrderedMap(requestFields); ok {
bedrockReq.AdditionalModelRequestFields = orderedFields // Overwrites reasoning_config!
}This would silently discard the reasoning_config set here.
Consider merging instead of replacing:
if requestFields, exists := bifrostReq.Params.ExtraParams["additionalModelRequestFieldPaths"]; exists {
if orderedFields, ok := schemas.SafeExtractOrderedMap(requestFields); ok {
- bedrockReq.AdditionalModelRequestFields = orderedFields
+ if bedrockReq.AdditionalModelRequestFields == nil {
+ bedrockReq.AdditionalModelRequestFields = orderedFields
+ } else {
+ for k, v := range orderedFields {
+ bedrockReq.AdditionalModelRequestFields[k] = v
+ }
+ }
}
}🤖 Prompt for AI Agents
In core/providers/bedrock/utils.go around lines 26-47 (and related logic at
~70-75), the reasoning_config added to bedrockReq.AdditionalModelRequestFields
can be lost because later code replaces the whole map when ExtraParams contains
additionalModelRequestFieldPaths; instead of assigning orderedFields directly,
merge them into bedrockReq.AdditionalModelRequestFields: ensure
bedrockReq.AdditionalModelRequestFields is initialized if nil, then iterate over
orderedFields and set each key/value into the existing map (overwriting specific
keys if needed) rather than replacing the whole map so reasoning_config and
other previously set fields are preserved.
| if bcr.Params.Reasoning != nil && bcr.Params.Reasoning.Effort != nil { | ||
| brr.Params.Reasoning = &ResponsesParametersReasoning{ | ||
| Effort: bcr.Params.ReasoningEffort, | ||
| Effort: bcr.Params.Reasoning.Effort, | ||
| } | ||
| } |
There was a problem hiding this comment.
Reasoning.MaxTokens is not preserved across Chat ↔ Responses conversions
In the request conversion helpers you’re only partially round-tripping the new structured reasoning config:
- In
ToResponsesRequest, you gate reasoning mapping onbcr.Params.Reasoning != nil && bcr.Params.Reasoning.Effort != niland then only copyEffortintoResponsesParametersReasoning. Any configuredMaxTokensonChatReasoningis dropped, and if callers setMaxTokenswithoutEffortthis block is skipped entirely. - In
ToChatRequest, you likewise only copyEffortfrombrr.Params.ReasoningintoChatReasoning, soResponsesParametersReasoning.MaxTokensnever makes it back to chat parameters.
Given Reasoning.MaxTokens is what ultimately feeds Anthropic’s reasoning/thinking budget, this means flows that convert via mux (e.g., chat→responses or responses→chat fallbacks) silently lose the max-tokens constraint.
You can keep behavior consistent and avoid losing config by:
- Treating
MaxTokensas a first-class field in both directions, and - Not requiring
Effortto be non-nil in order to propagate reasoning whenMaxTokensis set.
For example:
- // Handle Reasoning from reasoning_effort
- if bcr.Params.Reasoning != nil && bcr.Params.Reasoning.Effort != nil {
- brr.Params.Reasoning = &ResponsesParametersReasoning{
- Effort: bcr.Params.Reasoning.Effort,
- }
- }
+ // Handle structured Reasoning
+ if bcr.Params.Reasoning != nil &&
+ (bcr.Params.Reasoning.Effort != nil || bcr.Params.Reasoning.MaxTokens != nil) {
+ brr.Params.Reasoning = &ResponsesParametersReasoning{
+ Effort: bcr.Params.Reasoning.Effort,
+ MaxTokens: bcr.Params.Reasoning.MaxTokens,
+ }
+ }and:
- // Handle Reasoning from Reasoning
- if brr.Params.Reasoning != nil {
- bcr.Params.Reasoning = &ChatReasoning{
- Effort: brr.Params.Reasoning.Effort,
- }
- }
+ // Handle structured Reasoning from Responses -> Chat
+ if brr.Params.Reasoning != nil {
+ bcr.Params.Reasoning = &ChatReasoning{
+ Effort: brr.Params.Reasoning.Effort,
+ MaxTokens: brr.Params.Reasoning.MaxTokens,
+ }
+ }The new reasoning delta mapping via delta.Reasoning → ResponsesStreamResponseTypeReasoningSummaryTextDelta looks fine and doesn’t need changes.
Also applies to: 851-856, 1357-1364
🤖 Prompt for AI Agents
In core/schemas/mux.go around lines 773-777 (and likewise the similar blocks at
851-856 and 1357-1364), the conversion currently only copies Reasoning.Effort
and drops Reasoning.MaxTokens or skips mapping when Effort is nil; update both
ToResponsesRequest and ToChatRequest conversions to treat MaxTokens as a
first-class field: when building ResponsesParametersReasoning set MaxTokens =
bcr.Params.Reasoning.MaxTokens (and Effort if present) even if Effort is nil,
and when converting back set ChatReasoning.MaxTokens =
brr.Params.Reasoning.MaxTokens (and Effort if present); change the gating
condition to proceed if either Effort is non-nil or MaxTokens is set (i.e.,
present/non-zero) so MaxTokens is preserved across Chat↔Responses round-trips.
| if params.Reasoning != nil && params.Reasoning.Effort != nil { | ||
| metadata["reasoning_effort"] = *params.Reasoning.Effort | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Reasoning struct definition
echo "=== Searching for Reasoning struct definition ==="
ast-grep --pattern 'type Reasoning struct {
$$$
}'
echo -e "\n=== Searching for ChatParameters struct ==="
ast-grep --pattern 'type ChatParameters struct {
$$$
}'
echo -e "\n=== Using rg to find all Reasoning struct definitions ==="
rg -n "type Reasoning struct" --type=go
echo -e "\n=== Checking what fields are being used from Reasoning ==="
rg -n "Reasoning\." --type=go -C2Repository: maximhq/bifrost
Length of output: 25720
🏁 Script executed:
#!/bin/bash
echo "=== Searching for ChatReasoning struct definition ==="
rg -n "type ChatReasoning struct" --type=go -A 10
echo -e "\n=== Verifying MaxTokens usage in ChatReasoning context ==="
rg -n "Reasoning\.MaxTokens" --type=go -C 2Repository: maximhq/bifrost
Length of output: 6441
Extract Reasoning.MaxTokens in cache metadata for accurate cache key generation.
The nil-safety checks are correctly implemented. However, ChatReasoning.MaxTokens is not being extracted at lines 669-671. The MaxTokens field is critical for reasoning behavior—it controls the budget for reasoning output and is actively used across multiple providers (Anthropic, Bedrock, Cohere). Without it in the cache metadata, requests with identical Effort but different MaxTokens could incorrectly hit the same cache entry, leading to incorrect responses.
Add extraction for MaxTokens alongside Effort:
if params.Reasoning != nil {
if params.Reasoning.Effort != nil {
metadata["reasoning_effort"] = *params.Reasoning.Effort
}
if params.Reasoning.MaxTokens != nil {
metadata["reasoning_max_tokens"] = *params.Reasoning.MaxTokens
}
}🤖 Prompt for AI Agents
In plugins/semanticcache/utils.go around lines 669-671, the code only extracts
params.Reasoning.Effort into metadata but omits params.Reasoning.MaxTokens; add
a nil-safe extraction for MaxTokens alongside Effort so the cache metadata
includes reasoning_max_tokens when params.Reasoning and
params.Reasoning.MaxTokens are non-nil, ensuring cache keys differentiate
requests with different MaxTokens values.
Pratham-Mishra04
force-pushed
the
12-01-refactor_core_providers_error_handling_standardization
branch
from
3540e84 to
e8afa58
Compare
Pratham-Mishra04
force-pushed
the
12-01-feat_added_reasoning_support_to_chat_completions
branch
from
b5f6f04 to
facc630
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
core/providers/bedrock/utils.go (1)
26-47: Reasoning config may still be overwritten by ExtraParams handling.The reasoning config is correctly added to
AdditionalModelRequestFieldsat lines 29-47, but lines 72-73 can overwrite the entire map whenadditionalModelRequestFieldPathsexists in ExtraParams:bedrockReq.AdditionalModelRequestFields = orderedFields // Overwrites reasoning_config!This would silently discard the
reasoning_configset earlier.Consider merging instead of replacing:
if requestFields, exists := bifrostReq.Params.ExtraParams["additionalModelRequestFieldPaths"]; exists { if orderedFields, ok := schemas.SafeExtractOrderedMap(requestFields); ok { - bedrockReq.AdditionalModelRequestFields = orderedFields + if bedrockReq.AdditionalModelRequestFields == nil { + bedrockReq.AdditionalModelRequestFields = orderedFields + } else { + for k, v := range orderedFields { + bedrockReq.AdditionalModelRequestFields[k] = v + } + } } }Also applies to: 70-75
core/schemas/mux.go (1)
772-787: PreserveReasoning.MaxTokensin Chat ↔ Responses conversionsThe mux conversions still only copy
Reasoning.Effortand gate on it, soReasoning.MaxTokensis silently dropped and MaxTokens‑only configs are ignored:
ToResponsesRequestonly setsbrr.Params.Reasoningwhenbcr.Params.Reasoning.Effort != nil, and only copies Effort.ToChatRequestonly copies Effort back, never MaxTokens.Given
MaxTokensis what actually drives the Anthropic/Bedrock reasoning budget, this is configuration loss across Chat↔Responses flows.Consider something along these lines:
- // Handle Reasoning from reasoning_effort - if bcr.Params.Reasoning != nil && bcr.Params.Reasoning.Effort != nil { - brr.Params.Reasoning = &ResponsesParametersReasoning{ - Effort: bcr.Params.Reasoning.Effort, - } - } + // Handle structured Reasoning (Effort and MaxTokens) + if bcr.Params.Reasoning != nil && + (bcr.Params.Reasoning.Effort != nil || bcr.Params.Reasoning.MaxTokens != nil) { + brr.Params.Reasoning = &ResponsesParametersReasoning{ + Effort: bcr.Params.Reasoning.Effort, + MaxTokens: bcr.Params.Reasoning.MaxTokens, + } + } - // Handle Reasoning from Reasoning - if brr.Params.Reasoning != nil { - bcr.Params.Reasoning = &ChatReasoning{ - Effort: brr.Params.Reasoning.Effort, - } - } + // Handle structured Reasoning from Responses -> Chat + if brr.Params.Reasoning != nil { + bcr.Params.Reasoning = &ChatReasoning{ + Effort: brr.Params.Reasoning.Effort, + MaxTokens: brr.Params.Reasoning.MaxTokens, + } + }This keeps reasoning budgets intact in both directions, even when Effort is omitted but MaxTokens is set.
Also applies to: 809-856
core/providers/anthropic/errors.go (1)
9-31: Guard against nilbifrostErr.ErrorinToAnthropicChatCompletionError
ToAnthropicChatCompletionErrorstill assumesbifrostErr.Erroris non‑nil and dereferencesbifrostErr.Error.Message, which can panic if upstream code ever omits the nested error struct.You can keep this helper robust with a small nil‑check:
func ToAnthropicChatCompletionError(bifrostErr *schemas.BifrostError) *AnthropicMessageError { if bifrostErr == nil { return nil } // Provide blank strings for nil pointer fields errorType := "" if bifrostErr.Type != nil { errorType = *bifrostErr.Type } - - // Handle nested error fields with nil checks - errorStruct := AnthropicMessageErrorStruct{ - Type: errorType, - Message: bifrostErr.Error.Message, - } + // Safely extract nested error message + message := "" + if bifrostErr.Error != nil { + message = bifrostErr.Error.Message + } + + errorStruct := AnthropicMessageErrorStruct{ + Type: errorType, + Message: message, + }This avoids panics regardless of which code path constructs the
BifrostError.core/providers/bedrock/responses.go (1)
347-367: Unifyreasoning_configmap type and avoid overwriting it with extra field pathsTwo issues remain around
reasoning_configthat can lead to lost reasoning settings:
Type mismatch for disabled reasoning
InToBedrockResponsesRequestyou write:bedrockReq.AdditionalModelRequestFields["reasoning_config"] = map[string]string{ "type": "disabled", }but
ToBifrostResponsesRequestlater does:reasoningConfig, ok := schemas.SafeExtractFromMap(request.AdditionalModelRequestFields, "reasoning_config") if reasoningConfigMap, ok := reasoningConfig.(map[string]interface{}); ok { // ... }If a
BedrockConverseRequestever carries the map produced by your own writer, this type assertion will fail andParams.Reasoningwill not be populated. Use a uniformmap[string]anyfor both branches:
- bedrockReq.AdditionalModelRequestFields["reasoning_config"] = map[string]string{
"type": "disabled",- }
- bedrockReq.AdditionalModelRequestFields["reasoning_config"] = map[string]any{
"type": "disabled",- }
2. **Overwriting `AdditionalModelRequestFields` with `additionalModelRequestFieldPaths`** After setting `reasoning_config` from `Params.Reasoning`, this block: ```go if requestFields, exists := bifrostReq.Params.ExtraParams["additionalModelRequestFieldPaths"]; exists { if orderedFields, ok := schemas.SafeExtractOrderedMap(requestFields); ok { bedrockReq.AdditionalModelRequestFields = orderedFields } }replaces the entire
AdditionalModelRequestFieldsmap, discarding the earlierreasoning_config. If both are meant to coexist, mergeorderedFieldsinto the existing map instead of overwriting it.For example:
if orderedFields, ok := schemas.SafeExtractOrderedMap(requestFields); ok { if bedrockReq.AdditionalModelRequestFields == nil { bedrockReq.AdditionalModelRequestFields = orderedFields } else { for k, v := range orderedFields { bedrockReq.AdditionalModelRequestFields[k] = v } } }This keeps reasoning budgets configured via
Params.Reasoningintact even when callers also useadditionalModelRequestFieldPaths.Also applies to: 452-482
core/providers/cohere/chat.go (1)
126-137: ExtraParams["thinking"] can silently override Params.Reasoning.This issue was flagged in a previous review. When both
Params.ReasoningandExtraParams["thinking"]are provided, ExtraParams wins silently, which may not be the intended behavior.
🧹 Nitpick comments (7)
transports/bifrost-http/handlers/inference.go (1)
64-89: CustomChatRequestunmarshalling fixes embedded params, but double‑checkExtraParamsbehaviorThe custom
ChatRequest.UnmarshalJSONplus initializingChatParametersinchatCompletionis a good way to keepBifrostParams(model,fallbacks,stream, …) separate fromChatParametersdespite its custom unmarshaller, and adding"reasoning"tochatParamsKnownFieldskeeps it out ofExtraParams.One thing to verify: since you now:
- let
schemas.ChatParameters.UnmarshalJSONsee the full payload, and- later always compute
extraParamsviaextractExtraParamsand assignreq.ChatParameters.ExtraParams = extraParams,make sure
ChatParameters.UnmarshalJSONitself is not also trying to buildExtraParams, or its work will be silently overwritten here. If it is, consider consolidatingExtraParamspopulation in one place (either the type’s unmarshaller or this handler) for predictable behavior.Also applies to: 169-199, 476-513
core/schemas/mux.go (1)
1359-1369: Reasoning streaming delta mapping is aligned with newdelta.ReasoningfieldThe new block that emits
ResponsesStreamResponseTypeReasoningSummaryTextDeltafromdelta.Reasoning(withOutputIndex: Ptr(0)andDelta: delta.Reasoning) is consistent with the newChatStreamResponseChoiceDelta.Reasoningfield and the Responses streaming schema. For single‑output streams this looks fine; if you later support multi‑output streaming, you’ll likely want to plumb the actual choice index through instead of hard‑coding 0.core/providers/bedrock/chat.go (1)
38-40: Bedrock reasoning mapping (non‑stream & stream) is coherent with the new schema
ToBedrockChatCompletionRequestnow surfaces errors fromconvertChatParameters, which is important given the new reasoning validation there.ToBifrostChatResponsecorrectly liftsReasoningContentintoChatReasoningDetailsand a consolidatedassistantMessage.Reasoningstring.- Streaming
ReasoningContentdeltas are converted intoChatStreamResponseChoiceDelta.ReasoningplusReasoningDetails, which matches howmux.ToBifrostResponsesStreamResponsenow consumes reasoning deltas.Only very minor nits you might consider later:
reasoningTextis built with a trailing newline; trim if that ever shows up awkwardly in clients.- If a Bedrock reasoning delta ever contains both text and a signature in the same payload, you currently only surface one or the other; merging both into a single
ChatReasoningDetailsentry would preserve more information.Overall, the reasoning plumbing here looks solid.
Also applies to: 58-60, 103-112, 117-120, 135-141, 293-341
core/providers/anthropic/anthropic.go (1)
551-560: Aggregate cached‑token usage with max semantics instead of overwritingIn both streaming handlers you now populate cached‑token details:
- Chat streaming:
usage.PromptTokensDetails/usage.CompletionTokensDetailsfromCacheReadInputTokensandCacheCreationInputTokens.- Responses streaming:
usage.InputTokensDetails/usage.OutputTokensDetailsfrom the same fields.Right now this is “last writer wins”:
if usageToProcess.CacheReadInputTokens > 0 { usage.PromptTokensDetails = &schemas.ChatPromptTokensDetails{ CachedTokens: usageToProcess.CacheReadInputTokens } }If Anthropic ever emits multiple events with incremental or non‑monotonic cache counts, you may want to mirror your token counters and Bedrock’s behavior by taking the max instead of overwriting:
if usageToProcess.CacheReadInputTokens > 0 { if usage.PromptTokensDetails == nil || usageToProcess.CacheReadInputTokens > usage.PromptTokensDetails.CachedTokens { usage.PromptTokensDetails = &schemas.ChatPromptTokensDetails{ CachedTokens: usageToProcess.CacheReadInputTokens, } } }(and similarly for the other cached‑token fields). This keeps the final usage snapshot consistent regardless of emission order.
Also applies to: 893-902
core/providers/anthropic/responses.go (1)
325-344: Simplify nested else clause for cleaner code flow.The
elseat line 330 is redundant since the precedingifreturns. This can be flattened.if bifrostReq.Params.Reasoning != nil { if bifrostReq.Params.Reasoning.Effort != nil && *bifrostReq.Params.Reasoning.Effort != "none" { if bifrostReq.Params.Reasoning.MaxTokens != nil { if *bifrostReq.Params.Reasoning.MaxTokens < MinimumReasoningMaxTokens { return nil, fmt.Errorf("reasoning.max_tokens must be greater than or equal to %d", MinimumReasoningMaxTokens) - } else { - anthropicReq.Thinking = &AnthropicThinking{ - Type: "enabled", - BudgetTokens: bifrostReq.Params.Reasoning.MaxTokens, - } } + anthropicReq.Thinking = &AnthropicThinking{ + Type: "enabled", + BudgetTokens: bifrostReq.Params.Reasoning.MaxTokens, + } } else { return nil, fmt.Errorf("reasoning.max_tokens is required for reasoning") }core/providers/cohere/chat.go (2)
556-672: Well-implemented ToBifrostChatMessage with comprehensive content handling.The method properly handles:
- String vs block content
- Image content blocks
- Thinking/reasoning blocks with details accumulation
- Tool calls with nil-safety
- Assistant message assembly with reasoning
Minor note: Line 598 appends a newline after each thinking block text, which may leave a trailing newline in
reasoningText.} else if block.Type == CohereContentBlockTypeThinking && block.Thinking != nil { reasoningDetails = append(reasoningDetails, schemas.ChatReasoningDetails{ Index: len(reasoningDetails), Type: schemas.BifrostReasoningDetailsTypeText, Text: block.Thinking, }) - reasoningText += *block.Thinking + "\n" + if reasoningText != "" { + reasoningText += "\n" + } + reasoningText += *block.Thinking }
104-121: Move reasoning minimum token validation constant to shared location.The constant
MinimumReasoningMaxTokens = 1024is correctly validated here and matches Cohere's official minimum (≥1000 tokens for reasoning), but it's currently defined in the anthropic package. This creates an unnecessary cross-provider dependency; both Cohere and Bedrock import from the anthropic package for this validation. Consider defining this constant incore/schemaswhere other shared reasoning types (ChatReasoning, ResponsesParametersReasoning) are already centralized, making the constraint universally accessible without provider-specific imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
core/providers/anthropic/anthropic.go(6 hunks)core/providers/anthropic/chat.go(10 hunks)core/providers/anthropic/errors.go(1 hunks)core/providers/anthropic/responses.go(4 hunks)core/providers/anthropic/types.go(2 hunks)core/providers/azure/azure.go(4 hunks)core/providers/bedrock/bedrock.go(4 hunks)core/providers/bedrock/chat.go(5 hunks)core/providers/bedrock/responses.go(4 hunks)core/providers/bedrock/types.go(3 hunks)core/providers/bedrock/utils.go(3 hunks)core/providers/cohere/chat.go(10 hunks)core/providers/cohere/cohere.go(2 hunks)core/providers/cohere/types.go(1 hunks)core/providers/openai/chat.go(3 hunks)core/providers/openai/openai.go(2 hunks)core/providers/openai/types.go(2 hunks)core/providers/openai/utils.go(1 hunks)core/providers/perplexity/chat.go(1 hunks)core/providers/vertex/vertex.go(4 hunks)core/schemas/chatcompletions.go(7 hunks)core/schemas/mux.go(3 hunks)core/schemas/responses.go(2 hunks)framework/streaming/chat.go(1 hunks)plugins/jsonparser/utils.go(1 hunks)plugins/semanticcache/utils.go(1 hunks)transports/bifrost-http/handlers/inference.go(5 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/lib/types/logs.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- core/providers/vertex/vertex.go
- framework/streaming/chat.go
- plugins/semanticcache/utils.go
- ui/lib/types/logs.ts
- core/providers/openai/openai.go
- core/providers/cohere/types.go
- core/providers/openai/utils.go
- core/providers/cohere/cohere.go
- core/providers/openai/types.go
- core/providers/perplexity/chat.go
- core/schemas/responses.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/anthropic/types.gotransports/bifrost-http/handlers/inference.goplugins/jsonparser/utils.gocore/providers/azure/azure.gocore/providers/bedrock/utils.gocore/providers/bedrock/responses.gocore/providers/openai/chat.gocore/schemas/mux.goui/app/workspace/logs/views/logChatMessageView.tsxcore/providers/anthropic/errors.gocore/providers/bedrock/types.gocore/providers/anthropic/responses.gocore/providers/cohere/chat.gocore/providers/bedrock/bedrock.gocore/providers/anthropic/anthropic.gocore/providers/bedrock/chat.gocore/providers/anthropic/chat.gocore/schemas/chatcompletions.go
🧬 Code graph analysis (12)
core/providers/azure/azure.go (2)
core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(13-282)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(296-403)
core/providers/bedrock/utils.go (3)
core/schemas/chatcompletions.go (2)
BifrostChatRequest(12-19)OrderedMap(268-268)core/providers/bedrock/types.go (1)
BedrockConverseRequest(44-57)core/providers/anthropic/types.go (1)
MinimumReasoningMaxTokens(14-14)
core/providers/openai/chat.go (3)
core/providers/openai/utils.go (2)
ConvertOpenAIMessagesToBifrostMessages(5-24)ConvertBifrostMessagesToOpenAIMessages(26-45)core/schemas/chatcompletions.go (1)
ChatParameters(155-184)core/utils.go (1)
Ptr(51-53)
core/schemas/mux.go (3)
core/schemas/responses.go (3)
ResponsesParametersReasoning(232-237)BifrostResponsesStreamResponse(1424-1462)ResponsesStreamResponseTypeReasoningSummaryTextDelta(1390-1390)core/schemas/chatcompletions.go (1)
ChatReasoning(223-226)core/schemas/utils.go (1)
Ptr(16-18)
ui/app/workspace/logs/views/logChatMessageView.tsx (2)
ui/lib/utils/validation.ts (2)
isJson(539-546)cleanJson(548-558)ui/app/workspace/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
core/providers/anthropic/errors.go (1)
core/providers/anthropic/types.go (2)
AnthropicMessageError(376-379)AnthropicMessageErrorStruct(382-385)
core/providers/anthropic/responses.go (2)
core/schemas/responses.go (1)
ResponsesParametersReasoning(232-237)core/providers/anthropic/types.go (2)
MinimumReasoningMaxTokens(14-14)AnthropicThinking(65-68)
core/providers/cohere/chat.go (2)
core/schemas/chatcompletions.go (10)
BifrostChatRequest(12-19)ChatMessage(469-478)ChatReasoning(223-226)ChatStreamResponseChoice(751-753)ChatStreamResponseChoiceDelta(756-763)ChatReasoningDetails(723-730)ChatAssistantMessageToolCall(686-692)ChatMessageContent(524-527)ChatAssistantMessage(626-632)ChatMessageRole(457-457)core/providers/anthropic/types.go (1)
MinimumReasoningMaxTokens(14-14)
core/providers/bedrock/bedrock.go (2)
core/providers/utils/utils.go (1)
CreateBifrostChatCompletionChunkResponse(687-716)core/schemas/bifrost.go (1)
ChatCompletionStreamRequest(90-90)
core/providers/anthropic/anthropic.go (7)
core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(13-282)core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (1)
Ptr(51-53)core/schemas/chatcompletions.go (1)
ChatPromptTokensDetails(821-827)ui/lib/types/logs.ts (3)
CompletionTokensDetails(197-202)ResponsesResponseInputTokens(542-544)ResponsesResponseOutputTokens(546-548)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(296-403)core/schemas/responses.go (2)
ResponsesResponseInputTokens(261-267)ResponsesResponseOutputTokens(269-279)
core/providers/bedrock/chat.go (1)
core/schemas/chatcompletions.go (6)
ChatReasoningDetails(723-730)BifrostReasoningDetailsTypeText(719-719)ChatContentBlockTypeText(582-582)ChatAssistantMessage(626-632)ChatStreamResponseChoice(751-753)ChatStreamResponseChoiceDelta(756-763)
core/schemas/chatcompletions.go (1)
ui/lib/types/logs.ts (3)
ChatMessage(115-124)ChatMessageContent(113-113)ReasoningDetails(126-133)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (27)
core/providers/anthropic/types.go (1)
14-14: LGTM! Well-placed constant for reasoning token validation.The
MinimumReasoningMaxTokensconstant is appropriately defined here and exported for use by other providers (e.g., Bedrock) that need to validate reasoning configurations against Anthropic's requirements.core/providers/anthropic/chat.go (2)
647-687: LGTM! Proper handling of reasoning and signature streaming deltas.The streaming conversion correctly:
- Emits
Reasoningcontent withReasoningDetailsfor thinking deltas- Emits signature-only
ReasoningDetailsfor signature deltasThis aligns with Anthropic's streaming format where thinking content and signatures arrive in separate delta events.
744-751: LGTM! Consistent use of Reasoning field in streaming.The streaming response conversion correctly uses the new
Reasoningfield (renamed fromThought) for thinking content deltas.ui/app/workspace/logs/views/logChatMessageView.tsx (1)
73-94: LGTM! UI correctly updated to display reasoning content.The UI changes properly:
- Rename the field from
thoughttoreasoningto match the backend schema changes- Update the display label to "Reasoning"
- Preserve the JSON detection and formatting behavior
plugins/jsonparser/utils.go (1)
309-314: LGTM! Deep copy correctly updated for Reasoning field rename.The field rename from
ThoughttoReasoningis correctly applied in the deep copy function, maintaining the shallow copy behavior appropriate for string pointers.core/providers/bedrock/bedrock.go (1)
21-22: Per‑stream UUID for Bedrock chat streaming looks correctUsing a goroutine‑local
uuid.New().String()as the stream ID and wiring it through each chunk and the finalCreateBifrostChatCompletionChunkResponsekeeps Bedrock in line with other providers that emit stableresponse.IDs during streaming. No issues spotted.Also applies to: 771-783, 859-885
core/providers/openai/chat.go (1)
11-18: Message and reasoning conversions for OpenAI look consistentUsing
ConvertOpenAIMessagesToBifrostMessages/ConvertBifrostMessagesToOpenAIMessagesensures assistant‑side fields (reasoning, annotations, tool calls) survive round‑trips, and updatingfilterOpenAISpecificParametersto operate onChatParameters.Reasoning.Effortwhile normalizing"minimal"→"low"keeps the previous behavior with the new structured reasoning object. No issues spotted.Also applies to: 21-33, 61-75
core/providers/anthropic/anthropic.go (1)
303-311: Error‑aware Anthropic request construction is wired correctlyUsing
ToAnthropicChatRequest/ToAnthropicResponsesRequestdirectly inside theCheckContextAndGetRequestBodyclosures (and settingStream = Ptr(true)only after a successful conversion in the streaming cases) ensures model/parameter validation errors surface asBifrostErrors instead of slipping through. This applies consistently to both non‑streaming and streaming Chat/Responses paths and matches the broader error‑handling refactor in this stack.Also applies to: 353-365, 647-655, 695-707
core/providers/bedrock/responses.go (1)
656-660: Single conversion of Bedrock output message to Responses output is fineThe new early conversion of
response.Output.MessageviaconvertBedrockMessageToResponsesMessagesintobifrostResp.Outputis straightforward and avoids duplicated work later in the function. No issues here.core/providers/anthropic/responses.go (2)
190-201: LGTM! Clean conversion from Anthropic Thinking to Bifrost Reasoning.The mapping correctly handles both enabled and disabled thinking states, preserving the budget tokens when present.
296-299: Good defensive nil check added.The early return with a descriptive error message prevents nil pointer dereferences downstream.
core/providers/azure/azure.go (2)
353-360: Proper error propagation from converter function.The updated code correctly handles the error return from
ToAnthropicChatRequestand sets the deployment model after validation.
529-533: LGTM! Consistent error handling pattern.Error propagation for
ToAnthropicResponsesRequestfollows the same pattern as the chat completion methods.core/providers/bedrock/types.go (3)
103-104: LGTM! ReasoningContent field properly added.The optional field follows the existing pattern for content blocks and enables reasoning support in Bedrock requests/responses.
152-159: Well-structured reasoning types.The nested structure with
ReasoningTextcontainingTextand optionalSignaturealigns with the Anthropic thinking feature and supports verification via signatures.
438-440: Clean delta structure for streaming reasoning content.The updated
BedrockContentBlockDeltaproperly separates text, reasoning, and tool use deltas, enabling proper streaming of thinking content.core/providers/cohere/chat.go (3)
214-221: LGTM! Clean message conversion using new helper.The refactored code using
ToBifrostChatMessage()is cleaner and more maintainable.
242-256: LGTM! Bidirectional reasoning conversion is consistent.The conversion from Cohere Thinking to Bifrost Reasoning properly mirrors the forward conversion logic, handling both enabled and disabled states.
405-446: LGTM! Stream delta handling properly distinguishes text and thinking content.The nested conditionals correctly route text deltas and thinking deltas to their respective response fields, including populating
ReasoningDetails.core/schemas/chatcompletions.go (8)
165-165: Clean replacement of deprecated field with structured type.Replacing
ReasoningEffort *stringwithReasoning *ChatReasoningprovides a cleaner API while maintaining backward compatibility through the custom unmarshaller.
186-220: Robust backward-compatible unmarshalling with clear error handling.The custom
UnmarshalJSON:
- Correctly prevents both
reasoning_effortandreasoningfrom being set simultaneously- Converts legacy
reasoning_effortto the newReasoningstruct transparentlyThis ensures smooth migration for API consumers.
222-226: LGTM! ChatReasoning struct is well-designed.The struct with
EffortandMaxTokensfields provides the necessary flexibility for reasoning configuration across providers.
480-521: Necessary custom unmarshaller for ChatMessage with embedded types.The explicit handling of embedded
ChatToolMessageandChatAssistantMessageis required because embedded structs with their ownUnmarshalJSONmethods don't compose automatically. The check at lines 514-517 properly gates assistant message assignment.
634-667: Good UX: Synthesizing ReasoningDetails from Reasoning string.When a simple
reasoningstring is provided withoutreasoning_details, the unmarshaller automatically creates a structuredChatReasoningDetailsentry. This improves API ergonomics while maintaining full detail support.
714-730: Well-defined reasoning details types with comprehensive fields.The
ChatReasoningDetailsstruct withIndex,Type,Summary,Text,Signature, andDatafields covers all reasoning output variations across providers (summary, encrypted, text). The type constants align with the UI types inui/lib/types/logs.ts.
768-794: Consistent synthesizing behavior for streaming deltas.The
UnmarshalJSONforChatStreamResponseChoiceDeltafollows the same pattern asChatAssistantMessage, ensuring consistent behavior between streaming and non-streaming responses.
835-835: LGTM! ImageTokens field addition.The optional
ImageTokensfield inChatCompletionTokensDetailsenables tracking image-related token usage for multimodal models.
| } else if bifrostReq.Params.Reasoning != nil && bifrostReq.Params.Reasoning.Effort != nil && *bifrostReq.Params.Reasoning.Effort == "none" { | ||
| anthropicReq.Thinking = &AnthropicThinking{ | ||
| Type: "disabled", | ||
| } | ||
| } |
There was a problem hiding this comment.
Dead code: unreachable condition.
This else if block on line 118 is unreachable. It's placed after the closing brace of the if bifrostReq.Params != nil block (line 122 closes the block started at line 25). Since bifrostReq.Params.Reasoning can only be accessed when bifrostReq.Params != nil, this condition can never be true when reached.
Remove this dead code:
- } else if bifrostReq.Params.Reasoning != nil && bifrostReq.Params.Reasoning.Effort != nil && *bifrostReq.Params.Reasoning.Effort == "none" {
- anthropicReq.Thinking = &AnthropicThinking{
- Type: "disabled",
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if bifrostReq.Params.Reasoning != nil && bifrostReq.Params.Reasoning.Effort != nil && *bifrostReq.Params.Reasoning.Effort == "none" { | |
| anthropicReq.Thinking = &AnthropicThinking{ | |
| Type: "disabled", | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In core/providers/anthropic/chat.go around lines 118 to 122, the else-if that
checks bifrostReq.Params.Reasoning is unreachable because it sits outside the if
block that ensures bifrostReq.Params != nil; remove this dead else-if (or move
its logic into the existing bifrostReq.Params != nil branch) so the reasoning
Effort == "none" case is handled only when Params is non-nil and avoid
referencing Params after its nil-check scope.
| bifrostResponse.Usage = &schemas.BifrostLLMUsage{ | ||
| PromptTokens: response.Usage.InputTokens, | ||
| PromptTokensDetails: &schemas.ChatPromptTokensDetails{ | ||
| CachedTokens: response.Usage.CacheReadInputTokens, | ||
| }, | ||
| CompletionTokens: response.Usage.OutputTokens, | ||
| CompletionTokensDetails: &schemas.ChatCompletionTokensDetails{ | ||
| CachedTokens: response.Usage.CacheCreationInputTokens, | ||
| }, | ||
| TotalTokens: response.Usage.InputTokens + response.Usage.OutputTokens, | ||
| } | ||
| if response.Usage.CacheCreationInputTokens > 0 { | ||
| if bifrostResponse.Usage.CompletionTokensDetails == nil { | ||
| bifrostResponse.Usage.CompletionTokensDetails = &schemas.ChatCompletionTokensDetails{} | ||
| } | ||
| bifrostResponse.Usage.CompletionTokensDetails.CachedTokens = response.Usage.CacheCreationInputTokens | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundant assignment and potentially incorrect cache token mapping.
Two issues with the usage mapping:
-
Duplicate assignment: Lines 425-427 and 430-435 both assign
CacheCreationInputTokenstoCompletionTokensDetails.CachedTokens. The second assignment (lines 430-435) is redundant. -
Semantic mismatch:
CacheCreationInputTokensrepresents tokens used to create the input cache, which is conceptually an input/prompt operation, not a completion operation. Mapping it toCompletionTokensDetails.CachedTokensmay be semantically incorrect.
Consider:
bifrostResponse.Usage = &schemas.BifrostLLMUsage{
PromptTokens: response.Usage.InputTokens,
PromptTokensDetails: &schemas.ChatPromptTokensDetails{
CachedTokens: response.Usage.CacheReadInputTokens,
},
CompletionTokens: response.Usage.OutputTokens,
- CompletionTokensDetails: &schemas.ChatCompletionTokensDetails{
- CachedTokens: response.Usage.CacheCreationInputTokens,
- },
TotalTokens: response.Usage.InputTokens + response.Usage.OutputTokens,
}
- if response.Usage.CacheCreationInputTokens > 0 {
- if bifrostResponse.Usage.CompletionTokensDetails == nil {
- bifrostResponse.Usage.CompletionTokensDetails = &schemas.ChatCompletionTokensDetails{}
- }
- bifrostResponse.Usage.CompletionTokensDetails.CachedTokens = response.Usage.CacheCreationInputTokens
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bifrostResponse.Usage = &schemas.BifrostLLMUsage{ | |
| PromptTokens: response.Usage.InputTokens, | |
| PromptTokensDetails: &schemas.ChatPromptTokensDetails{ | |
| CachedTokens: response.Usage.CacheReadInputTokens, | |
| }, | |
| CompletionTokens: response.Usage.OutputTokens, | |
| CompletionTokensDetails: &schemas.ChatCompletionTokensDetails{ | |
| CachedTokens: response.Usage.CacheCreationInputTokens, | |
| }, | |
| TotalTokens: response.Usage.InputTokens + response.Usage.OutputTokens, | |
| } | |
| if response.Usage.CacheCreationInputTokens > 0 { | |
| if bifrostResponse.Usage.CompletionTokensDetails == nil { | |
| bifrostResponse.Usage.CompletionTokensDetails = &schemas.ChatCompletionTokensDetails{} | |
| } | |
| bifrostResponse.Usage.CompletionTokensDetails.CachedTokens = response.Usage.CacheCreationInputTokens | |
| } | |
| } | |
| bifrostResponse.Usage = &schemas.BifrostLLMUsage{ | |
| PromptTokens: response.Usage.InputTokens, | |
| PromptTokensDetails: &schemas.ChatPromptTokensDetails{ | |
| CachedTokens: response.Usage.CacheReadInputTokens, | |
| }, | |
| CompletionTokens: response.Usage.OutputTokens, | |
| TotalTokens: response.Usage.InputTokens + response.Usage.OutputTokens, | |
| } |
🤖 Prompt for AI Agents
In core/providers/anthropic/chat.go around lines 419 to 436, the code
redundantly assigns response.Usage.CacheCreationInputTokens twice and
incorrectly maps CacheCreationInputTokens to
CompletionTokensDetails.CachedTokens; remove the second redundant block (lines
430-435) and instead assign CacheCreationInputTokens to
bifrostResponse.Usage.PromptTokensDetails.CachedTokens (create
bifrostResponse.Usage.PromptTokensDetails if nil before assigning) so
cache-creation tokens are recorded as prompt/input cache tokens rather than
completion tokens.
| reqBody, err := anthropic.ToAnthropicChatRequest(request) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| reqBody.Model = deployment | ||
| reqBody.Stream = schemas.Ptr(true) | ||
| return reqBody, nil |
There was a problem hiding this comment.
Potential nil pointer dereference after error check.
At line 456-457, after the error check, the code directly accesses reqBody.Model and reqBody.Stream without verifying reqBody is non-nil. If ToAnthropicChatRequest returns (nil, nil), this would panic.
func() (any, error) {
reqBody, err := anthropic.ToAnthropicChatRequest(request)
if err != nil {
return nil, err
}
+ if reqBody == nil {
+ return nil, fmt.Errorf("anthropic request conversion returned nil")
+ }
reqBody.Model = deployment
reqBody.Stream = schemas.Ptr(true)
return reqBody, nil
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| reqBody, err := anthropic.ToAnthropicChatRequest(request) | |
| if err != nil { | |
| return nil, err | |
| } | |
| reqBody.Model = deployment | |
| reqBody.Stream = schemas.Ptr(true) | |
| return reqBody, nil | |
| reqBody, err := anthropic.ToAnthropicChatRequest(request) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if reqBody == nil { | |
| return nil, fmt.Errorf("anthropic request conversion returned nil") | |
| } | |
| reqBody.Model = deployment | |
| reqBody.Stream = schemas.Ptr(true) | |
| return reqBody, nil |
🤖 Prompt for AI Agents
In core/providers/azure/azure.go around lines 452 to 458, the call to
anthropic.ToAnthropicChatRequest may return (nil, nil) so the subsequent direct
accesses to reqBody.Model and reqBody.Stream can cause a nil pointer
dereference; after the err check, add a nil-check for reqBody and, if nil,
return a clear error (or create a new request object) before setting Model and
Stream, otherwise proceed to set reqBody.Model = deployment and reqBody.Stream =
schemas.Ptr(true) and return reqBody, nil.
| reqBody, err := anthropic.ToAnthropicResponsesRequest(request) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| reqBody.Model = deployment | ||
| reqBody.Stream = schemas.Ptr(true) |
There was a problem hiding this comment.
Same nil check concern as ChatCompletionStream.
For consistency and safety, consider adding a nil check after error handling before accessing reqBody fields.
🤖 Prompt for AI Agents
In core/providers/azure/azure.go around lines 627 to 632, after converting the
request with anthropic.ToAnthropicResponsesRequest you need to guard against
reqBody being nil even when err is nil; add a nil check (if reqBody == nil {
return nil, fmt.Errorf("empty request body") } or similar) before setting
reqBody.Model and reqBody.Stream so you don't dereference a nil pointer and
return a proper error upstream.
| func convertChatParameters(bifrostReq *schemas.BifrostChatRequest, bedrockReq *BedrockConverseRequest) error { | ||
| if bifrostReq.Params == nil { | ||
| return | ||
| return fmt.Errorf("bifrost request parameters are nil") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for how other providers handle nil params in conversion functions
echo "=== Searching for nil params handling in conversion functions ==="
rg -nA5 'if.*Params.*==.*nil' --type=go core/providers/
echo -e "\n=== Broader search for params handling patterns ==="
rg -nB2 -A5 'Params.*nil' --type=go core/providers/ | head -60Repository: maximhq/bifrost
Length of output: 14162
Remove the nil params validation error and use defaults instead.
The current implementation returns an error when bifrostReq.Params == nil, but this is inconsistent with the rest of the codebase. Other providers handle nil params gracefully:
- Gemini initializes default
ChatParameterswith emptyExtraParamsmap (gemini/utils.go:202-206) - Vertex and Perplexity conditionally process params only if they exist, continuing without error otherwise
- ElevenLabs returns early without error when params are nil for optional operations
Errors should only be returned for required specific fields (e.g., reasoning.max_tokens when reasoning is enabled), not for nil params at the top level. Follow the Gemini pattern and initialize params with reasonable defaults instead.
🤖 Prompt for AI Agents
In core/providers/bedrock/utils.go around lines 14-17, remove the early error
return when bifrostReq.Params is nil and instead initialize bifrostReq.Params to
a default ChatParameters object (with sensible defaults and an empty ExtraParams
map) so downstream code can proceed; follow the Gemini pattern by creating
defaults when Params is nil rather than erroring, and only return errors later
for required nested fields (e.g., reasoning.max_tokens) if/when those
validations are needed.
Summary
Added support for reasoning capabilities across providers, with a focus on Anthropic's thinking feature. This PR introduces a unified reasoning interface in the Bifrost schema and implements provider-specific mappings.
Changes
reasoningfield toChatParametersto replace the deprecatedreasoning_effortfieldType of change
Affected areas
How to test
Test reasoning capabilities with Anthropic models:
Screenshots/Recordings
N/A
Breaking changes
The
reasoning_effortfield is now deprecated in favor of the newreasoningobject. Existing code usingreasoning_effortwill continue to work but should be updated to use the new structure.Related issues
N/A
Security considerations
Reasoning content is properly sanitized and displayed in the UI.
Checklist
docs/contributing/README.mdand followed the guidelines