feat: add support for Anthropic models in Azure - #965
Conversation
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds Anthropic/Claude support across Azure and Vertex with deployment-aware routing and headers, centralizes Vertex error parsing, renames Anthropic streaming APIs and converters, marshals NetworkConfig backoff fields as milliseconds in JSON, clamps runtime backoff to configured max, adds model-family helpers, and updates branding/docs/UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Gateway
participant AzureProvider
participant AnthropicAPI
participant OpenAIEndpoint
participant PostHook
Client->>Gateway: Send request (model, optional deployment)
Gateway->>AzureProvider: getModelDeployment(key, model)
alt deployment is Anthropic
AzureProvider->>AnthropicAPI: Build Anthropic request (deployment URL, OAuth header), stream
AnthropicAPI-->>AzureProvider: Streamed chunks (include modelName)
AzureProvider->>PostHook: postResponseConverter per chunk (nil-safe)
PostHook-->>Gateway: Converted stream chunks
Gateway-->>Client: Streamed chunks
else OpenAI-style deployment
AzureProvider->>OpenAIEndpoint: Build OpenAI-style request (deployments/{deployment}/...), send
OpenAIEndpoint-->>AzureProvider: Final response
AzureProvider->>PostHook: postResponseConverter on final response
PostHook-->>Gateway: Converted response
Gateway-->>Client: Final response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
e69ca18 to
bf4722e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/bedrock/bedrock.go (1)
529-544: Perfect! I now have all the information needed to verify this review comment. The issue is confirmed.Use
request.Modelinstead ofdeploymentfor model type detection in TextCompletion response unmarshalingThe refactored code checks
schemas.IsAnthropicModel(deployment)andschemas.IsMistralModel(deployment)at lines 529 and 536, butdeploymentmay be empty when no deployment mapping exists inBedrockKeyConfig.Deployments. This causes both checks to fail even if the actual model supports text completion.The inconsistency is clear: request conversion in
text.goline 43 checksbifrostReq.Modelusing the samestrings.Containslogic. Response unmarshaling should do the same. Whendeploymentis empty butrequest.Modelis "anthropic.claude-3-sonnet", the function incorrectly returns an "unsupported model type" error at line 544.Fix: Replace
deploymentwithrequest.Modelin the model type checks:case schemas.IsAnthropicModel(request.Model):and
case schemas.IsMistralModel(request.Model):This maintains consistency with request/response conversion logic and allows text completion to work without mandatory deployment mappings.
core/providers/anthropic/anthropic.go (1)
863-893: MissingpostResponseConverterinvocation in streaming loop.The
postResponseConverterparameter is declared (Line 723) but never called within the streaming loop. In contrast,HandleAnthropicChatCompletionStreamingapplies the converter at lines 564-570. This means callers like Vertex'sResponsesStream(which passes apostResponseConverterto setModelDeployment) won't have their converter applied.Add the converter invocation before setting
ExtraFields:for i, response := range responses { if response != nil { + if postResponseConverter != nil { + response = postResponseConverter(response) + if response == nil { + logger.Warn("postResponseConverter returned nil; skipping chunk") + continue + } + } + response.ExtraFields = schemas.BifrostResponseExtraFields{ RequestType: schemas.ResponsesStreamRequest, Provider: providerName,
♻️ Duplicate comments (4)
docs/apis/openapi.json (4)
2003-2032: Same deployment-id wording consideration as chat completionsSame comment as for the chat-completions deployment path: you may want to harmonize the
deployment-idparameter description with the new generic “OpenAI-compatible … for specific deployments” phrasing.
2115-2144: Embedding deployments path shares the same minor docs nitThis embeddings deployment endpoint has the same minor Azure-vs-generic deployment-id wording as noted earlier; consider updating here together with the other deployment endpoints if you decide to adjust it.
2224-2225: Speech deployments description follows same patternThe speech deployment endpoint’s summary/description are now generic “OpenAI Compatible … for specific deployments”; it shares the same optional deployment-id wording tweak discussed in the chat/completions comment.
2332-2333: Transcription deployments description follows same patternLikewise for the audio transcriptions deployment endpoint; see earlier note about possibly standardizing the
deployment-idparameter description across all these Azure-style routes.
🧹 Nitpick comments (8)
ui/app/workspace/logs/views/logChatMessageView.tsx (1)
29-29: Good addition ofbreak-words— consider applying consistently.Adding
break-wordsprevents overflow of long unbreakable strings. However, for consistency, consider applying the same text wrapping approach to other non-JSON text renderings in this file (lines 89 and 132) that also display user-generated content.Apply this diff to add consistent text wrapping to the thought content:
-<div className="text-muted-foreground px-6 py-2 font-mono text-xs whitespace-pre-wrap italic">{message.thought}</div> +<div className="text-muted-foreground px-6 py-2 font-mono text-xs break-words whitespace-pre-wrap italic">{message.thought}</div>And to the string content:
-<div className="px-6 py-2 font-mono text-xs whitespace-pre-wrap">{message.content}</div> +<div className="px-6 py-2 font-mono text-xs break-words whitespace-pre-wrap">{message.content}</div>core/changelog.md (1)
1-2: Polish wording in changelog entry.Minor copy nit: consider standardizing spelling and phrasing for clarity:
-- enhancement: using naive anthropic convertors for Vertex Anthropic responses and responses stream +- enhancement: using naive Anthropic converters for Vertex Anthropic responses and response streamsPurely cosmetic; feel free to skip if you prefer the current wording.
docs/integrations/litellm-sdk.mdx (1)
73-79: Azure branding updates are correct; optional clarification on env vars.The renames to “Azure models” and “Using Azure with direct Azure key” read well and align with the new terminology. Since the example still uses
AZURE_OPENAI_DEPLOYMENT, you might (optionally) add a short note that these env var names are legacy/compatible Azure naming to avoid confusion for readers skimming just this section.Also applies to: 146-162
core/providers/vertex/errors.go (1)
10-38: Consider renaming shadowed variable for clarity.Line 17 declares
var vertexErr VertexErrorwhich shadows the array variablevar vertexErr []VertexErrorfrom Line 12. While this is valid Go (different scopes), it reduces readability. Consider using distinct names likevertexErrArrayandvertexErrSingle.func parseVertexError(providerName schemas.ModelProvider, resp *fasthttp.Response) *schemas.BifrostError { var openAIErr schemas.BifrostError - var vertexErr []VertexError + var vertexErrArray []VertexError - if err := sonic.Unmarshal(resp.Body(), &openAIErr); err != nil || openAIErr.Error == nil { + if err := sonic.Unmarshal(resp.Body(), &openAIErr); err != nil || openAIErr.Error == nil { // Try Vertex error format if OpenAI format fails or is incomplete - if err := sonic.Unmarshal(resp.Body(), &vertexErr); err != nil { + if err := sonic.Unmarshal(resp.Body(), &vertexErrArray); err != nil { //try with single Vertex error format - var vertexErr VertexError - if err := sonic.Unmarshal(resp.Body(), &vertexErr); err != nil { + var vertexErrSingle VertexError + if err := sonic.Unmarshal(resp.Body(), &vertexErrSingle); err != nil { // Try VertexValidationError format (validation errors from Mistral endpoint) var validationErr VertexValidationError if err := sonic.Unmarshal(resp.Body(), &validationErr); err != nil { return providerUtils.NewBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err, schemas.Vertex) } if len(validationErr.Detail) > 0 { return providerUtils.NewProviderAPIError(validationErr.Detail[0].Msg, nil, resp.StatusCode(), schemas.Vertex, nil, nil) } return providerUtils.NewProviderAPIError("Unknown error", nil, resp.StatusCode(), schemas.Vertex, nil, nil) } - return providerUtils.NewProviderAPIError(vertexErr.Error.Message, nil, resp.StatusCode(), schemas.Vertex, nil, nil) + return providerUtils.NewProviderAPIError(vertexErrSingle.Error.Message, nil, resp.StatusCode(), schemas.Vertex, nil, nil) } - if len(vertexErr) > 0 { - return providerUtils.NewProviderAPIError(vertexErr[0].Error.Message, nil, resp.StatusCode(), schemas.Vertex, nil, nil) + if len(vertexErrArray) > 0 { + return providerUtils.NewProviderAPIError(vertexErrArray[0].Error.Message, nil, resp.StatusCode(), schemas.Vertex, nil, nil) } return providerUtils.NewProviderAPIError("Unknown error", nil, resp.StatusCode(), schemas.Vertex, nil, nil) } // OpenAI error format succeeded with valid Error field return providerUtils.NewProviderAPIError(openAIErr.Error.Message, nil, resp.StatusCode(), schemas.Vertex, nil, nil) }core/schemas/utils.go (1)
1042-1050: Model detection helpers are fine; consider generalizing commentsThe new
IsAnthropicModelandIsMistralModelhelpers provide a simple, centralized way to detect Anthropic/Claude and Mistral/Codestral models via substring checks, which is consistent with how models are named elsewhere in the codebase.The only nit is that both comments mention “in Vertex”, but these helpers are now used across multiple providers. You might want to generalize the comments to avoid confusion, e.g.:
-// IsAnthropicModel checks if the model is an Anthropic model in Vertex. +// IsAnthropicModel checks if the model string represents an Anthropic/Claude model. @@ -// IsMistralModel checks if the model is a Mistral or Codestral model in Vertex. +// IsMistralModel checks if the model string represents a Mistral or Codestral model.This keeps the behavior as-is but better reflects actual usage.
docs/apis/openapi.json (1)
1881-1911: Align deployment-id wording across Azure-style OpenAI endpointsHere and in the other
/openai/deployments/{deployment-id}/…paths, the summaries now say “OpenAI Compatible … for specific deployments”, but the path param description still reads “Azure deployment ID”. Consider standardizing this text (e.g., “Deployment ID (for example, an Azure model deployment identifier)”) across all such endpoints for consistency and to avoid confusing users about whether these are Azure-only.core/providers/azure/azure.go (2)
360-370: Consider usingdeploymentinstead ofrequest.Modelfor consistency.The model check at Line 361 uses
request.Modelwhile Line 364 setsreqBody.Model = deployment. For consistency with the streaming path (which usesdeploymentin the check), consider usingschemas.IsAnthropicModel(deployment)here.- if schemas.IsAnthropicModel(request.Model) { + if schemas.IsAnthropicModel(deployment) {
533-557: Consider usingdeploymentfor model check consistency.Similar to ChatCompletion, lines 533 and 553 use
request.Modelfor the Anthropic check, while the streaming counterpart (ResponsesStream at Line 622) usesdeployment. For consistency across all methods, consider usingdeployment.- if schemas.IsAnthropicModel(request.Model) { + if schemas.IsAnthropicModel(deployment) {And at Line 553:
- if schemas.IsAnthropicModel(request.Model) { + if schemas.IsAnthropicModel(deployment) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(1 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/schemas/utils.go(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/streaming/responses.go(2 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/changelog.md(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)
💤 Files with no reviewable changes (1)
- core/providers/utils/utils.go
🧰 Additional context used
🧬 Code graph analysis (8)
core/providers/bedrock/bedrock.go (3)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/bedrock/types.go (1)
BedrockAnthropicTextResponse(235-239)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
core/providers/openai/chat.go (2)
core/schemas/utils.go (1)
IsMistralModel(1048-1050)core/schemas/models.go (1)
Model(109-129)
core/providers/anthropic/chat.go (2)
core/schemas/chatcompletions.go (1)
BifrostChatRequest(12-19)core/providers/anthropic/types.go (1)
AnthropicMessageRequest(39-58)
core/providers/vertex/vertex.go (11)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)Ptr(16-18)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/providers/anthropic/anthropic.go (1)
HandleAnthropicResponsesStream(713-908)core/providers/utils/utils.go (5)
HandleProviderResponse(343-387)CheckContextAndGetRequestBody(256-274)NewConfigurationError(435-445)SetExtraHeaders(178-208)NewBifrostOperationError(449-460)core/schemas/bifrost.go (7)
BifrostResponseExtraFields(284-293)RequestType(83-83)ChatCompletionRequest(89-89)BifrostError(353-362)Vertex(40-40)ResponsesRequest(91-91)BifrostStream(318-325)core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-35)core/schemas/responses.go (3)
BifrostResponsesRequest(32-39)BifrostResponsesResponse(45-82)BifrostResponsesStreamResponse(1421-1459)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(284-383)core/providers/vertex/types.go (1)
DefaultVertexAnthropicVersion(8-8)core/schemas/provider.go (1)
PostHookRunner(204-204)core/utils.go (1)
Ptr(51-53)
core/providers/azure/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)Ptr(16-18)core/providers/azure/types.go (1)
AzureAnthropicAPIVersionDefault(6-6)core/utils.go (1)
Ptr(51-53)
core/providers/anthropic/anthropic.go (4)
core/providers/anthropic/types.go (1)
AnthropicMessageResponse(272-281)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/schemas/bifrost.go (4)
ModelProvider(32-32)BifrostResponseExtraFields(284-293)RequestType(83-83)BifrostError(353-362)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
framework/streaming/responses.go (2)
core/schemas/bifrost.go (3)
OpenAI(35-35)OpenRouter(49-49)Azure(36-36)core/schemas/utils.go (1)
IsAnthropicModel(1043-1045)
ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
ui/components/ui/sheet.tsx (1)
SheetContent(140-140)
🔇 Additional comments (47)
docs/features/unified-interface.mdx (2)
91-91: Branding update from "Azure OpenAI" to "Azure" is correct.The provider name change aligns with the PR objective and properly reflects that Azure now supports multiple model families (OpenAI and Anthropic). The format
azure/<model>is consistent with other provider naming conventions.
95-95: Verify Elevenlabs TTS/STT capability flags are accurate and intentional.The Elevenlabs row was updated to show ✅ for TTS and ✅ for STT (previously all ❌). Please confirm these capability updates reflect actual Elevenlabs support and are not coincidental changes. If these are unrelated to the Anthropic/Azure feature, consider whether they should be in a separate commit for clarity.
ui/app/workspace/logs/views/logDetailsSheet.tsx (2)
39-39: Improved spacing layout for better readability.Adding
gap-4to the SheetContent creates consistent vertical spacing between child elements, enhancing visual hierarchy and readability without affecting functionality. The component already displays provider and model information generically (lines 92, 97), so it will seamlessly accommodate Anthropic models from Azure.
52-56: Minor JSX formatting adjustment.The closing
>tag repositioning is a non-functional whitespace/code-layout change.transports/bifrost-http/integrations/router.go (1)
304-304: LGTM! Formatting-only changes.These changes adjust whitespace and brace formatting without altering any functional behavior, control flow, or error handling logic.
Also applies to: 328-328, 536-536
docs/quickstart/gateway/multimodal.mdx (1)
292-298: Azure label rename looks good.Renaming the provider from “Azure OpenAI” to “Azure” in the support table is consistent with the rest of the branding changes; no further tweaks needed.
transports/changelog.md (1)
1-2: Changelog entry is clear and aligned with core changes.The new “feat: added support for Anthropic models in Azure” bullet succinctly reflects the transport‑level addition and matches the core changelog; no functional concerns here.
docs/quickstart/go-sdk/multimodal.mdx (1)
339-345: Go SDK provider table rename is consistent.The Azure row label change to “Azure” matches the rest of the docs and doesn’t affect examples; all good.
docs/integrations/what-is-an-integration.mdx (1)
88-97: Azure tab rename fits the provider-prefixed model pattern.Using “Azure” as the tab title and “# Azure models” keeps the examples consistent with other providers and the new naming; no further changes needed.
core/internal/testutil/account.go (1)
648-651: Comment update matches Azure behavior and naming.The updated comment for Azure’s
TextModel(no text completion in newer models) reflects current capabilities and aligns with the broader Azure renaming; no code changes required.framework/streaming/responses.go (1)
595-599: Azure Anthropic correctly excluded from OpenAI-style streaming fast path.The updated condition
if provider == schemas.OpenAI || provider == schemas.OpenRouter || (provider == schemas.Azure && !schemas.IsAnthropicModel(model)) {ensures Azure Claude/Anthropic deployments go through the accumulation path instead of assuming the final chunk carries the complete response, while keeping OpenAI, OpenRouter, and non-Anthropic Azure models on the cheaper short‑circuit path. The tightened cleanup comment in
processAccumulatedResponsesStreamingChunksalso accurately documents the “cleanup before unlock” ordering without changing behavior. This looks sound given the new Azure Anthropic support.Also applies to: 686-745
docs/integrations/langchain-sdk.mdx (1)
221-221: LGTM! Documentation branding update is consistent.The terminology change from "Azure OpenAI" to "Azure" aligns with the broader provider rebranding across the PR and improves clarity.
Also applies to: 269-269
docs/quickstart/gateway/provider-configuration.mdx (1)
762-774: LGTM! Provider-specific authentication section updated correctly.The Azure section heading and descriptive text updates maintain consistency with the rebranding effort.
ui/README.md (1)
84-84: LGTM! UI documentation updated consistently.The supported providers list now correctly reflects the "Azure" branding.
ui/lib/constants/logs.ts (1)
45-45: LGTM! UI provider label updated correctly.The ProviderLabels constant now reflects the updated "Azure" branding consistently across the UI.
docs/features/keys-management.mdx (1)
198-198: LGTM! Keys management documentation updated consistently.The deployment mapping section heading now uses "Azure" branding.
core/providers/azure/azure_test.go (1)
32-32: LGTM! Test comment updated consistently.The comment now uses "Azure" terminology consistently with the broader rebranding effort.
docs/integrations/anthropic-sdk.mdx (2)
104-109: Azure label rename is consistent and clearThe “Azure models” label matches the
azure/gpt-4oprefix and the new naming used elsewhere in the docs; no behavioral concerns.
151-156: JS Azure section matches Python example and namingThe JavaScript “Azure models” label and example mirror the Python snippet and are aligned with the updated Azure terminology.
docs/integrations/genai-sdk.mdx (2)
98-102: Azure naming update is consistent with provider prefixUsing “Azure models” for the
azure/gpt-4oexample keeps the provider list consistent and avoids the older “Azure OpenAI” wording.
133-136: JavaScript Azure example correctly mirrors Python sectionThe JS heading/comment now use “Azure models”, matching the Python tab and the rest of the rebranding.
core/providers/openai/chat.go (1)
48-54: Centralizing Mistral detection viaschemas.IsMistralModellooks correctUsing
schemas.IsMistralModel(bifrostReq.Model)in the Vertex branch keeps the Mistral compatibility logic while aligning with the new shared helpers; scope is still only Vertex models so impact is controlled.core/providers/azure/types.go (1)
3-6: New Azure Anthropic API version constant is reasonableAdding
AzureAnthropicAPIVersionDefault = "2023-06-01"alongside the generalAzureAPIVersionDefaultcleanly separates Anthropic-specific API versioning; comment text for the default Azure API version also reads correctly.core/chatbot_test.go (3)
632-641: Whitespace/formatting tweak around synthesis input is safeThe adjustment around the
Input: conversationWithSynthesisfield doesn’t alter behavior; structure of the synthesis request is unchanged.
774-782: Azure provider description text matches updated brandingThe help text now lists “Azure (Azure models)”, which is consistent with other docs and the provider naming used in the rest of this file.
836-843: Azure env-var guidance remains accurate after wording changeThe hint about
AZURE_API_KEYandAZURE_ENDPOINTis still correct; the tweak to say “for Azure” just matches the new terminology.docs/integrations/openai-sdk.mdx (3)
99-103: Python Azure example label matches provider prefixRenaming the comment to “Azure models” aligns with the
azure/gpt-4omodel prefix and the broader Azure terminology used elsewhere.
141-145: JS Azure example label is consistent with Python and other docsThe JavaScript “Azure models” label matches the Python tab and keeps provider naming uniform across SDK docs.
319-365: Azure section wording now matches integration behavior and namingThe introductory sentence now refers generically to “Azure” while still documenting the
x-bf-azure-endpointrequirement and Bifrost/openaiendpoint, which fits the updated branding without changing the integration semantics.docs/apis/openapi.json (1)
1759-1788: Deployment completions endpoint wording looks consistentThe updated summary/description/response text for
/openai/deployments/{deployment-id}/completionsnow match the “OpenAI-compatible” positioning used elsewhere; nothing blocking here.docs/quickstart/go-sdk/provider-configuration.mdx (1)
274-280: Azure provider-auth wording matches the new branding and config shapeThe updated intro and “Azure” tab title cleanly align with the broader shift away from “Azure OpenAI” wording, and the brief description (endpoint URL, deployment mappings, API version) correctly reflects what the Azure key config requires.
core/providers/anthropic/chat.go (1)
384-386: Function rename successfully verified—no stale references remainingThe search confirms that all references to
ToAnthropicChatCompletionRequesthave been updated across the codebase. The new nameToAnthropicChatRequestaccurately reflects the function's role of converting a Bifrost chat request to Anthropic format, and the updated comment clearly documents this purpose. The rename is complete and ready.core/providers/vertex/vertex.go (4)
299-336: LGTM on centralized Anthropic model detection and request building.The switch to
schemas.IsAnthropicModel(deployment)provides cleaner, centralized model detection. The double marshal/unmarshal pattern (noted with TODO) is understood as a workaround for converting struct to map.
443-495: LGTM on response handling with proper pool usage.The response handling correctly uses the renamed pool functions and conditionally sets
ModelDeploymentonly when it differs fromModelRequested, avoiding redundant data.
519-524: LGTM on postResponseConverter pattern for streaming.The closure correctly captures
deploymentto populateModelDeploymentin streaming responses when it differs fromModelRequested.
1064-1066: LGTM on consistent ModelDeployment handling.The conditional assignment of
ModelDeploymentis now consistent across ChatCompletion, Responses, ResponsesStream, and Embedding methods.core/providers/azure/azure.go (5)
63-127: LGTM on refactored completeRequest with deployment-aware routing.The function correctly branches between Anthropic and OpenAI paths:
- Anthropic: uses
x-api-keyheader and direct URL without api-version- OpenAI: uses Bearer token or
api-keyheader with api-version query parameterThe expanded signature properly supports deployment-scoped URL construction.
245-268: LGTM on TextCompletion with centralized deployment lookup.TextCompletion correctly uses the new
getModelDeploymenthelper and doesn't need an Anthropic path since Anthropic doesn't support legacy text completion endpoints.
442-513: LGTM on ChatCompletionStream with proper Anthropic/OpenAI routing.The streaming implementation correctly:
- Uses
deploymentfor the model type check (Line 449)- Sets appropriate headers for each path
- Uses the corresponding streaming handler
614-690: LGTM on ResponsesStream with correct deployment-based routing.The implementation correctly uses
deploymentfor the model check and properly routes to either the Anthropic or OpenAI streaming handler with appropriate converters.
789-800: LGTM on centralized deployment lookup helper.The
getModelDeploymenthelper provides clean, centralized deployment resolution. The redundant nil check withvalidateKeyConfigis acceptable defensive coding.core/providers/anthropic/anthropic.go (6)
30-56: LGTM on pool and accessor function renames.The rename from
anthropicChatResponsePooltoanthropicMessageResponsePoolbetter reflects thatAnthropicMessageResponseis the underlying type. The public accessors are correctly updated.
318-332: LGTM on consistent usage of renamed functions.
ToAnthropicChatRequestand the renamed pool functions are used consistently throughout ChatCompletion and ChatCompletionStream.
407-421: LGTM on enhanced streaming handler signature.The addition of
postResponseConverterparameter enables callers (like Vertex and Azure providers) to inject deployment-specific metadata into streaming responses. The parameter rename fromproviderTypetoproviderNamebetter reflects its usage.
564-570: LGTM on postResponseConverter integration.The defensive nil checks and warning log for converter-returned nil values are good practices. The converter is applied before populating
ExtraFields, allowing it to modify the response structure if needed.
685-708: LGTM on ResponsesStream refactor to use shared handler.The refactored code follows the same pattern as
ChatCompletionStream, building headers in a map and delegating to the sharedHandleAnthropicResponsesStreamfunction. This improves code reuse across providers.
392-404: LGTM on internal provider usage passing nil converter.The Anthropic provider correctly passes
nilforpostResponseConverterwhen calling its own streaming handler, as it doesn't need deployment metadata injection (unlike Azure/Vertex which have deployment mappings).
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/anthropic/anthropic.go (1)
863-893:postResponseConverterparameter is declared but never used.The
HandleAnthropicResponsesStreamfunction accepts apostResponseConverterparameter (line 723) but never applies it to responses before sending. Compare withHandleAnthropicChatCompletionStreaming(lines 564-570) which correctly applies its converter.Add the converter application within the response processing loop:
for i, response := range responses { if response != nil { + if postResponseConverter != nil { + response = postResponseConverter(response) + if response == nil { + logger.Warn("postResponseConverter returned nil; skipping chunk") + continue + } + } + response.ExtraFields = schemas.BifrostResponseExtraFields{ RequestType: schemas.ResponsesStreamRequest, Provider: providerName,
🧹 Nitpick comments (10)
ui/app/workspace/logs/views/logChatMessageView.tsx (1)
14-32: Improved wrapping for long plain-text content blocksAdding
break-wordsto the non-JSONblock.textcontainer should prevent horizontal overflow for long tokens/URLs and makes the log view more readable. If you want perfectly consistent behavior, you could later mirror this on the top-levelmessage.contentnon-JSON path, but it’s not required for this PR.docs/integrations/openai-sdk.mdx (1)
319-369: Add note for Anthropic-on-Azure usage.Since this PR routes Anthropic models via Azure, add a brief note/example showing a Claude deployment with AzureOpenAI client and any API-version/header nuances if different from OpenAI deployments.
docs/quickstart/go-sdk/provider-configuration.mdx (1)
274-306: Nice Azure provider-auth example; consider Anthropic-in-Azure details.Please add:
- A Claude deployment mapping example alongside GPT deployments.
- A note on the default API versions for Azure OpenAI vs Azure Anthropic (and when to override).
docs/apis/openapi.json (5)
1881-1923: Make parameter description generic (“Deployment ID”).This block still says “Azure deployment ID”. Recommend aligning with the earlier change.
- "description": "Azure deployment ID", + "description": "Deployment ID",
2003-2045: Make parameter description generic (“Deployment ID”).- "description": "Azure deployment ID", + "description": "Deployment ID",
2115-2153: Make parameter description generic (“Deployment ID”).- "description": "Azure deployment ID", + "description": "Deployment ID",
2223-2263: Make parameter description generic (“Deployment ID”).- "description": "Azure deployment ID", + "description": "Deployment ID",
2330-2371: Make parameter description generic (“Deployment ID”).- "description": "Azure deployment ID", + "description": "Deployment ID",core/providers/vertex/errors.go (1)
12-18: Variable shadowing reduces readability.The inner
vertexErron line 17 shadows the outervertexErrslice declared on line 12. While this works correctly due to Go's scoping rules, consider renaming the inner variable for clarity.var vertexErr []VertexError if err := sonic.Unmarshal(resp.Body(), &openAIErr); err != nil || openAIErr.Error == nil { // Try Vertex error format if OpenAI format fails or is incomplete if err := sonic.Unmarshal(resp.Body(), &vertexErr); err != nil { //try with single Vertex error format - var vertexErr VertexError - if err := sonic.Unmarshal(resp.Body(), &vertexErr); err != nil { + var singleVertexErr VertexError + if err := sonic.Unmarshal(resp.Body(), &singleVertexErr); err != nil {core/providers/vertex/vertex.go (1)
682-830: Consider extracting common Anthropic request handling.The Anthropic path in
Responsesshares significant code withChatCompletion(validation, URL construction, auth token handling, HTTP request setup). This is acceptable for now, but if more methods need Anthropic support, consider extracting a common helper likemakeAnthropicRequest(ctx, key, path, body)to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(1 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/schemas/utils.go(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/streaming/responses.go(2 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/changelog.md(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)
💤 Files with no reviewable changes (1)
- core/providers/utils/utils.go
🧰 Additional context used
🧬 Code graph analysis (7)
core/providers/openai/chat.go (2)
core/schemas/utils.go (1)
IsMistralModel(1048-1050)core/schemas/models.go (1)
Model(109-129)
framework/streaming/responses.go (2)
core/schemas/bifrost.go (3)
OpenAI(35-35)OpenRouter(49-49)Azure(36-36)core/schemas/utils.go (1)
IsAnthropicModel(1043-1045)
core/providers/anthropic/chat.go (2)
core/schemas/chatcompletions.go (1)
BifrostChatRequest(12-19)core/providers/anthropic/types.go (1)
AnthropicMessageRequest(39-58)
ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
ui/components/ui/sheet.tsx (1)
SheetContent(140-140)
core/providers/vertex/errors.go (5)
core/providers/vertex/vertex.go (1)
VertexError(25-31)core/providers/vertex/types.go (1)
VertexValidationError(154-161)core/providers/utils/utils.go (2)
NewBifrostOperationError(449-460)NewProviderAPIError(464-479)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(29-29)core/schemas/bifrost.go (1)
Vertex(40-40)
core/providers/anthropic/anthropic.go (5)
core/providers/anthropic/types.go (1)
AnthropicMessageResponse(272-281)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/schemas/bifrost.go (4)
ModelProvider(32-32)BifrostResponseExtraFields(284-293)RequestType(83-83)BifrostError(353-362)core/schemas/provider.go (2)
PostHookRunner(204-204)Provider(207-234)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
core/providers/bedrock/bedrock.go (3)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/bedrock/types.go (1)
BedrockAnthropicTextResponse(235-239)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
🔇 Additional comments (41)
docs/integrations/litellm-sdk.mdx (1)
73-78: Documentation branding updates look good.The renaming from "Azure OpenAI models" to "Azure models" and "Using Azure OpenAI with direct Azure key" to "Using Azure with direct Azure key" correctly reflects the expanded scope of Azure support to include Anthropic models alongside OpenAI. The examples remain accurate—environment variable names (
AZURE_OPENAI_DEPLOYMENT) and endpoint URLs appropriately retain their original conventions since they follow LiteLLM and Azure service naming, not Bifrost branding.Also applies to: 146-162
transports/bifrost-http/integrations/router.go (1)
528-542: SpeechResponseConverter nil fallback is correct.The else branch at lines 536–542 properly handles the case where
SpeechResponseConverterisnilby sending raw audio with appropriate headers (Content-Type, Content-Disposition, Content-Length). This complements the converter path above and ensures speech responses can still be served even when no converter is configured. The early return prevents further processing.One minor note: ensure that upstream handlers never pass a
nilspeechResponseto this code path. Looking at lines 523–526, a nil check happens before this block, so this is already safe.ui/app/workspace/logs/views/logDetailsSheet.tsx (2)
39-39: LGTM! Spacing improvement enhances visual clarity.The
gap-4utility adds consistent vertical spacing between sections in the sheet, improving readability and visual hierarchy.
52-56: LGTM! Formatting adjustment with no functional impact.The reformatting of the closing braces is purely cosmetic and maintains the existing delete functionality.
ui/README.md (1)
84-88: Azure branding in provider list looks consistentUsing just "Azure" in the Supported Providers list matches the new branding used elsewhere in the repo. No further changes needed.
docs/integrations/anthropic-sdk.mdx (1)
104-109: Azure model examples match routing semanticsLabeling these as “Azure models” and using the
azure/gpt-4oprefix aligns with the provider-prefixed routing convention and updated branding. Looks good.Also applies to: 151-156
core/providers/openai/chat.go (1)
35-55: Centralizing Mistral detection viaschemas.IsMistralModelSwitching the Vertex branch to use
schemas.IsMistralModel(bifrostReq.Model)is a nice consolidation and keeps the Mistral compatibility logic in one place. Since this is now substring-based on"mistral"/"codestral", it will apply to any Vertex model IDs following that convention.It’d be good to ensure there are tests that:
- Exercise a Vertex Mistral model slug (e.g.,
mistral-large-*) and confirmapplyMistralCompatibility()is hit.- Exercise a non-Mistral Vertex model and confirm it’s not.
Tagging this as a verification point only; the change itself looks correct.
docs/integrations/what-is-an-integration.mdx (1)
88-97: Azure tab/docs text aligned with provider-prefix usageRenaming the tab to “Azure” and the comment to “Azure models” matches the
azure/...model prefix pattern and overall branding. No issues.docs/integrations/langchain-sdk.mdx (1)
221-238: Azure direct-key headings consistent with new namingUpdating the headings to “Using Azure with direct Azure key” keeps terminology consistent with the rest of the docs while leaving the actual LangChain examples unchanged. Looks good.
Also applies to: 269-288
transports/changelog.md (1)
1-2: Changelog entry for Azure Anthropic support is clearAdding a separate bullet for “feat: added support for Anthropic models in Azure” is clear and matches the feature scope of this PR.
ui/lib/constants/logs.ts (1)
42-59: Azure provider label now matches global brandingUpdating the
azurelabel to “Azure” keeps the UI consistent with the rest of the documentation and provider naming. Mapping and helper logic remain unchanged.docs/integrations/openai-sdk.mdx (2)
99-104: LGTM on branding update.
141-146: LGTM on branding update.docs/apis/openapi.json (1)
1759-1801: Good: generalized wording for deployments endpoint.You updated summary/description and the param to “Deployment ID”. Suggest applying the same generic param description across all other deployments endpoints for consistency.
docs/features/keys-management.mdx (1)
198-202: LGTM on Azure heading rename.core/changelog.md (1)
1-2: Changelog entries look good.docs/integrations/genai-sdk.mdx (2)
98-103: LGTM on branding update.
133-136: LGTM on branding update.docs/quickstart/go-sdk/multimodal.mdx (1)
344-344: LGTM! Documentation branding update.The provider name change from "Azure OpenAI" to "Azure" aligns with the broader branding updates across the repository and accurately reflects that Azure can now host multiple model types (OpenAI and Anthropic).
core/providers/azure/azure_test.go (1)
32-32: LGTM! Comment branding update.The comment update aligns with the broader Azure branding changes in the PR.
core/providers/bedrock/bedrock.go (1)
529-541: LGTM! Good refactoring to centralize model detection.Replacing inline string checks with
schemas.IsAnthropicModel()andschemas.IsMistralModel()centralizes the model detection logic, making it more maintainable and consistent across the codebase.core/internal/testutil/account.go (1)
650-650: LGTM! Comment branding update.Consistent branding change aligning with the broader Azure terminology updates.
core/schemas/utils.go (1)
1042-1050: LGTM! Clean centralized model type detection.These helper functions centralize model type detection logic, making it more maintainable and consistent across providers. The implementation is straightforward and appropriate.
Note: The functions use case-sensitive matching, which should be fine given that model identifiers are typically lowercase.
core/chatbot_test.go (2)
779-779: LGTM! Help text branding update.Consistent update to reflect the broader Azure branding changes.
841-841: LGTM! Startup guidance text update.Consistent with the Azure branding changes throughout the PR.
framework/streaming/responses.go (1)
686-686: Approve streaming logic for Azure model routing.The condition correctly routes Azure requests based on model type:
- Azure with OpenAI models → uses OpenAI-compatible final-chunk path
- Azure with Anthropic models → uses accumulation path for delta streaming
This aligns with Azure's support for both OpenAI and Anthropic model deployments. The logic appears sound:
(provider == schemas.Azure && !schemas.IsAnthropicModel(model))ensures only non-Anthropic Azure models use the OpenAI path.Please verify this behavior with both Azure OpenAI and Azure Anthropic streaming requests to ensure correct chunk handling for both model types.
core/providers/azure/types.go (1)
6-6: The API version is correct and appropriately used.The value
"2023-06-01"is Anthropic's documented API version identifier for theanthropic-versionHTTP header, not a calendar date indicating staleness. This is the correct version for Anthropic's/v1/messagesendpoint and is properly distinguished from Azure's separateapi-versionquery parameter (which uses"2024-10-21"for Azure OpenAI). The implementation correctly sets this header only for Anthropic models.docs/quickstart/gateway/provider-configuration.mdx (1)
762-779: LGTM! Documentation branding updates are consistent.The terminology changes from "Azure OpenAI" to "Azure" are applied consistently across the header, description, and UI navigation instructions. This aligns with the broader rebranding effort in this PR.
core/providers/anthropic/chat.go (1)
384-386: LGTM! Function rename is appropriate.The rename from
ToAnthropicChatCompletionRequesttoToAnthropicChatRequestis more concise and better reflects the Anthropic API terminology (Messages API). The function signature and logic remain unchanged.core/providers/azure/azure.go (4)
63-128: LGTM! Well-structured request routing for Anthropic and OpenAI paths.The
completeRequestmethod cleanly separates:
- Authentication:
x-api-key+anthropic-versionfor Anthropic models vsapi-key/Bearer for OpenAI- URL construction:
anthropic/v1/...path for Anthropic vsopenai/deployments/...for OpenAIThe deployment parameter addition enables per-request routing decisions.
347-426: LGTM! Clean implementation of Anthropic routing in ChatCompletion.The implementation correctly:
- Routes to appropriate request converter based on model type
- Uses separate API paths for Anthropic (
anthropic/v1/messages) vs OpenAI- Leverages object pooling for Anthropic responses
- Populates ExtraFields consistently for both paths
428-514: LGTM! Streaming implementation correctly delegates to appropriate handlers.The branching logic for Anthropic vs OpenAI streaming is well-implemented:
- Anthropic models use
HandleAnthropicChatCompletionStreaming- OpenAI models use
HandleOpenAIChatCompletionStreaming- The
postResponseConverterensuresModelDeploymentis set consistently for both paths
789-800: LGTM! Clean deployment resolution helper.The
getModelDeploymenthelper provides centralized deployment lookup with clear error handling. The nil check on line 790 provides defensive programming even thoughvalidateKeyConfigis typically called first.core/providers/vertex/vertex.go (2)
279-496: LGTM! ChatCompletion correctly integrates Anthropic model support.Key improvements:
- Uses
schemas.IsAnthropicModelfor consistent model detection- Correctly uses renamed
ToAnthropicChatRequestconverter- Centralized error handling via
parseVertexError- Properly manages Anthropic response pooling with acquire/release pattern
- Conditionally sets
ModelDeploymentonly when it differs from the requested model
832-944: LGTM! ResponsesStream correctly handles Anthropic models with fallback.The implementation appropriately:
- Uses
HandleAnthropicResponsesStreamfor Anthropic models- Falls back to
ChatCompletionStreamfor non-Anthropic models, converting viaToChatRequest()- Sets a context value to indicate the fallback, allowing downstream handling to adjust if needed
core/providers/anthropic/anthropic.go (6)
30-56: LGTM on the pool renaming.The pool and accessor functions are correctly renamed from
AnthropicChatResponsetoAnthropicMessageResponse, aligning with theAnthropicMessageResponsetype. The implementation properly resets the struct on acquire and handles nil-checks on release.
318-332: LGTM on the renamed function calls.The updated function names (
ToAnthropicChatRequest,AcquireAnthropicMessageResponse,ReleaseAnthropicMessageResponse) are used correctly and consistently with the pool renames.
380-404: LGTM on the streaming setup.The headers map is correctly constructed with required Anthropic headers, and passing
nilforpostResponseConverteris appropriate for the native Anthropic provider since no post-response transformation is needed.
564-570: Good defensive nil-check on postResponseConverter.The implementation correctly handles the case where
postResponseConverterreturnsnil, logging a warning and skipping the chunk rather than proceeding with a nil response. This prevents potential nil-pointer dereferences downstream.
935-940: LGTM on error parsing helper.The function correctly extracts status code and body from the response and delegates to
NewProviderAPIErrorfor consistent error handling.
638-639: LGTM on pool usage in Responses method.The renamed pool accessors are used consistently with the non-streaming
ChatCompletionmethod.
cc73f7c to
c31e74e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/apis/openapi.json (1)
3109-3236:TextCompletionRequestschema requires a non-existenttextfield.In
TextCompletionRequest, therequiredarray lists"model"and"text", but the actual property is namedpromptand there is notextfield. This makes the OpenAPI schema formally incorrect for validation and codegen.Recommend changing the required field to
"prompt"(or adding atextproperty if that was intended).
🧹 Nitpick comments (8)
core/providers/gemini/errors.go (3)
48-48: Remove unnecessary type cast.The
StatusCode()method already returnsint, making the explicit cast redundant.Apply this diff:
- StatusCode: schemas.Ptr(int(resp.StatusCode())), + StatusCode: schemas.Ptr(resp.StatusCode()),
39-90: Harmonize error handling between streaming and non-streaming parsers.The two parsing functions have several inconsistencies that make error behavior unpredictable:
Line 58 vs 84:
parseStreamGeminiErrorusesinterface{}forrawResponse, whileparseGeminiErrorusesmap[string]interface{}. Consider using the same type for consistency.Line 60 vs 86: Different error messages for the same unmarshal failure—one uses the constant
schemas.ErrProviderResponseUnmarshal, the other uses a literal string"failed to parse error response". Prefer the constant for consistency.Line 63 vs 89: The streaming error includes the HTTP status code in the message format (
"Gemini streaming error (HTTP %d): %v"), but the non-streaming error omits it ("Gemini error: %v"). Including the status code in both improves debuggability.Example fix for consistency:
func parseGeminiError(providerName schemas.ModelProvider, resp *fasthttp.Response) *schemas.BifrostError { body := append([]byte(nil), resp.Body()...) // Try to parse as JSON first var errorResp GeminiGenerationError if err := sonic.Unmarshal(body, &errorResp); err == nil { bifrostErr := &schemas.BifrostError{ IsBifrostError: false, StatusCode: schemas.Ptr(resp.StatusCode()), Error: &schemas.ErrorField{ Code: schemas.Ptr(strconv.Itoa(errorResp.Error.Code)), Message: errorResp.Error.Message, }, } return bifrostErr } - var rawResponse map[string]interface{} + var rawResponse interface{} if err := sonic.Unmarshal(body, &rawResponse); err != nil { - return providerUtils.NewBifrostOperationError("failed to parse error response", err, providerName) + return providerUtils.NewBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err, providerName) } - return providerUtils.NewBifrostOperationError(fmt.Sprintf("Gemini error: %v", rawResponse), fmt.Errorf("HTTP %d", resp.StatusCode()), providerName) + return providerUtils.NewBifrostOperationError(fmt.Sprintf("Gemini error (HTTP %d): %v", resp.StatusCode(), rawResponse), fmt.Errorf("HTTP %d", resp.StatusCode()), providerName) }
39-90: Consider consolidating duplicate error parsing logic.Both
parseStreamGeminiErrorandparseGeminiErrorfollow nearly identical logic with only minor differences. Extracting a common helper would reduce duplication and simplify maintenance.Example refactored approach:
// parseGeminiErrorInternal is a common helper for both streaming and non-streaming func parseGeminiErrorInternal(providerName schemas.ModelProvider, resp *fasthttp.Response, isStreaming bool) *schemas.BifrostError { body := append([]byte(nil), resp.Body()...) // Try to parse as JSON first var errorResp GeminiGenerationError if err := sonic.Unmarshal(body, &errorResp); err == nil { bifrostErr := &schemas.BifrostError{ IsBifrostError: false, StatusCode: schemas.Ptr(resp.StatusCode()), Error: &schemas.ErrorField{ Code: schemas.Ptr(strconv.Itoa(errorResp.Error.Code)), Message: errorResp.Error.Message, }, } return bifrostErr } var rawResponse interface{} if err := sonic.Unmarshal(body, &rawResponse); err != nil { return providerUtils.NewBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err, providerName) } errorType := "Gemini error" if isStreaming { errorType = "Gemini streaming error" } return providerUtils.NewBifrostOperationError(fmt.Sprintf("%s (HTTP %d): %v", errorType, resp.StatusCode(), rawResponse), fmt.Errorf("HTTP %d", resp.StatusCode()), providerName) } func parseStreamGeminiError(providerName schemas.ModelProvider, resp *fasthttp.Response) *schemas.BifrostError { return parseGeminiErrorInternal(providerName, resp, true) } func parseGeminiError(providerName schemas.ModelProvider, resp *fasthttp.Response) *schemas.BifrostError { return parseGeminiErrorInternal(providerName, resp, false) }core/providers/bedrock/bedrock.go (1)
526-545: Make Anthropic/Mistral detection resilient whendeploymentis emptyRight now the switch uses only
deployment:switch { case schemas.IsAnthropicModel(deployment): ... case schemas.IsMistralModel(deployment): ... default: return nil, providerUtils.NewConfigurationError( fmt.Sprintf("unsupported model type for text completion: %s", request.Model), providerName, ) }However,
getModelPathcan return an emptydeploymentwhen there is noBedrockKeyConfig.Deploymentsentry or ARN configured, whilerequest.Modelmay still be a full Bedrock model id (e.g., an Anthropic or Mistral model). In that situation we’d now treat a potentially valid model as “unsupported” purely becausedeploymentis blank.To keep detection robust (and likely preserve existing behaviour when deployments aren’t configured), consider deriving a single identifier that falls back to
request.Model:- // Handle model-specific response conversion - var bifrostResponse *schemas.BifrostTextCompletionResponse - switch { - case schemas.IsAnthropicModel(deployment): + // Handle model-specific response conversion + var bifrostResponse *schemas.BifrostTextCompletionResponse + modelIdentifier := deployment + if modelIdentifier == "" { + modelIdentifier = request.Model + } + switch { + case schemas.IsAnthropicModel(modelIdentifier): var response BedrockAnthropicTextResponse if err := sonic.Unmarshal(body, &response); err != nil { return nil, providerUtils.NewBifrostOperationError("error parsing anthropic response", err, providerName) } bifrostResponse = response.ToBifrostTextCompletionResponse() - case schemas.IsMistralModel(deployment): + case schemas.IsMistralModel(modelIdentifier): var response BedrockMistralTextResponse if err := sonic.Unmarshal(body, &response); err != nil { return nil, providerUtils.NewBifrostOperationError("error parsing mistral response", err, providerName) } bifrostResponse = response.ToBifrostTextCompletionResponse()This keeps the new centralized predicates (
IsAnthropicModel/IsMistralModel) while avoiding unexpected “unsupported model type” errors when deployments aren’t set up but the model name alone is sufficient to infer the family.docs/apis/openapi.json (1)
1757-1801: Azure deployment endpoints wording is mostly consistent; check remaining “Azure deployment ID” mentions.The updated summaries/descriptions for the
/openai/deployments/{deployment-id}/...endpoints look good and match the new “OpenAI-compatible” branding. However, only thecompletionspath’s parameter description was normalized to “Deployment ID”, while the others still say “Azure deployment ID”. Consider standardizing thedeployment-idparameter description across these endpoints (either all “Deployment ID” or all “Azure deployment ID”) to avoid small but confusing wording differences.Also applies to: 1879-1923, 2001-2045, 2113-2151, 2222-2263, 2330-2370
core/providers/vertex/errors.go (1)
10-37: Centralized Vertex error parsing looks correct and usesproviderNameconsistently.The new
parseVertexErrorcovers OpenAI-style, array-of-Vertex, single-Vertex, and validation error formats, and now correctly passesproviderNameinto the variousNew*Errorhelpers instead of hardcoding the provider. The slice vs singlevertexErrshadowing is a little hard to follow but functionally fine; refactoring the inner one to a different name would slightly improve readability if you touch this again.core/providers/azure/azure.go (2)
297-305: Consider usinggetModelDeploymentinTextCompletionStreamfor consistency.
TextCompletionStreamstill does:deployment := key.AzureKeyConfig.Deployments[request.Model] if deployment == "" { return nil, providerUtils.NewConfigurationError( fmt.Sprintf("deployment not found for model %s", request.Model), provider.GetProviderKey(), ) }While the rest of the code now centralizes this logic in
getModelDeployment. It’s not wrong, but usinggetModelDeploymenthere as well would keep configuration error handling uniform and reduce duplication:- deployment := key.AzureKeyConfig.Deployments[request.Model] - if deployment == "" { - return nil, providerUtils.NewConfigurationError( - fmt.Sprintf("deployment not found for model %s", request.Model), - provider.GetProviderKey(), - ) - } + deployment, cfgErr := provider.getModelDeployment(key, request.Model) + if cfgErr != nil { + return nil, cfgErr + }
63-72: <!-- [scratchpad_start]
[task verify_review_comment 1/1]
[observations]
FINAL SUMMARY:
- 9 total IsAnthropicModel calls
- 3 PROBLEMS (using deployment when shouldn't):
- Line 84: completeRequest
- Line 449: ChatCompletionStream
- Line 622: ResponsesStream
- 6 CORRECT (using request.Model):
- Lines 361, 377, 399: ChatCompletion/ChatCompletionStream path/body setup
- Lines 533, 553, 575: Responses/ResponsesStream path/body setup
[review comment accuracy assessment]
The review's CORE RECOMMENDATION is CORRECT:
✓ Line 84 (completeRequest) should use model parameter instead of deployment
✓ The principle of using canonical model string is sound
✓ The risk explanation is accurate
✓ The diff example is correct for line 84The review's SCOPE/REFERENCES are INCOMPLETE/INACCURATE:
✗ Missing ChatCompletionStream (line 449) from analysis
✗ "Also applies to" ranges don't clearly map to actual problems
✗ Includes Embedding (701-724) which has no IsAnthropicModel
✗ The ranges seem to point to functions rather than specific problem lines[decision]
The review comment is FUNDAMENTALLY CORRECT but has:
- Correct problem identification at line 84
- Correct solution suggested
- INCOMPLETE scope specification in "Also applies to"
- Missing identification of line 449 and clear specification of line 622
This should be tagged as because while the main recommendation is sound, the scope is incomplete and could mislead the developer about all the places needing fixes.
Actually, wait - let me reconsider. The review says:
"In ChatCompletionStream and ResponsesStream, keep the branch on schemas.IsAnthropicModel(request.Model)"This could mean:
- These functions SHOULD already be using request.Model (they're not - they use deployment at lines 449, 622)
- Or it could mean after the fix to completeRequest, ensure these also use request.Model
Looking at the code flow:
- ChatCompletionStream/ResponsesStream call completeRequest with the model parameter
- So if completeRequest is fixed to use model, that part is covered
- But ChatCompletionStream/ResponsesStream ALSO have their own IsAnthropicModel branches at 449 and 622 that are independent
The review says "keep the branch on request.Model" - this suggests those functions should be using request.Model for their own branches, not relying solely on completeRequest.
So the review is actually:
- Suggesting to fix line 84 in completeRequest
- Saying ChatCompletionStream/ResponsesStream should KEEP their request.Model branches
- But they don't currently have request.Model branches - they have deployment branches!
This is a DISCREPANCY in the review comment. It says to "keep" something that doesn't exist.
Actually, maybe I need to re-read more carefully...
"In ChatCompletionStream and ResponsesStream, keep the branch on schemas.IsAnthropicModel(request.Model) (as you already do for path/body), and let completeRequest rely on that model argument."
This suggests:
- ChatCompletionStream/ResponsesStream already use request.Model for path/body (TRUE - lines 361, 377, 399, 533, 553, 575)
- They should ALSO use request.Model for the authentication/url branch (currently they use deployment at 449, 622)
- The word "keep" is misleading - it should be "change" or "align"
[CONCLUSION]
The review is PARTIALLY INCORRECT:
✓ Line 84 fix is correct
✗ Says to "keep" something that doesn't match current code
✓ The principle is correct but execution/wording is off
✗ "Also applies to" is incomplete/inaccurateThis should be marked as due to inconsistency in language and incomplete scope.
Hmm, but actually let me reconsider one more time. Looking at the actual code structure:
ChatCompletionStream has:
- Line 449: IsAnthropicModel(deployment) for setting auth headers and URL
- Lines 361, 377, 399: IsAnthropicModel(request.Model) for setting path and body
So there ARE two different branches, one for auth/URL setup and one for path/body setup!
The review is saying:
- The path/body setup (lines 361, 377, 399) correctly uses request.Model
- The auth/URL setup (line 449) incorrectly uses deployment
- They should be aligned
So the "keep the branch on request.Model" means keep BOTH branches on request.Model, not just the path/body ones.
This makes more sense! The review is saying:
- completeRequest should use model parameter for auth/URL setup (currently uses deployment at line 84)
- ChatCompletionStream/ResponsesStream should use request.Model for auth/URL setup (currently use deployment at lines 449, 622)
- This ensures consistency with their own path/body setup logic
[FINAL VERDICT]
The review is SUBSTANTIALLY CORRECT:
✓ Core problem identified correctly
✓ Solution is sound
✓ Risk explanation is accurate
✗ "Also applies to" is poorly specified and incomplete (missing line 449, unclear line ranges)
✓ The wording "keep" is slightly misleading but can be understood in contextClassification: The core recommendation is correct and actionable. The scope/references are incomplete but not fundamentally wrong.
[DECISION - use optional_refactors_recommended because]:
- The review suggests refactoring for consistency and safety
- It's not a critical bug fix but a best practice recommendation
- No current bugs are reported, just a risk mitigation strategy
- The "Also applies to" incompleteness doesn't negate the core recommendation
Actually, re-reading the review tags... it has and
So the review is categorizing this as a recommended refactor (not a bug fix), which aligns with my assessment.
The question is: is this review comment CORRECT or does it need adjustment?
ASSESSMENT:
- The main recommendation (line 84) is correct
- The underlying principle is correct
- The "Also applies to" is incomplete but not disqualifying
- The review could be improved but is fundamentally sound
I should mark this as since:
- The recommendation is correct
- No critical issues beyond what's stated
- The incomplete scope doesn't make the recommendation wrong, just incomplete
But I should also note the incompleteness in my rewrite.
</scratchpad_end] -->Perfect! Now I have a complete understanding of the issue. Let me generate the rewritten review comment based on my verification findings.
Align Anthropic/OpenAI branching on the canonical model string consistently across all functions.
Right now, three functions use
deploymentfor the Anthropic/OpenAI branch decision when they should use the canonical model string:
completeRequest(line 84) usesschemas.IsAnthropicModel(deployment)instead of themodelparameterChatCompletionStream(line 449) usesschemas.IsAnthropicModel(deployment)instead ofrequest.ModelResponsesStream(line 622) usesschemas.IsAnthropicModel(deployment)instead ofrequest.ModelMeanwhile, their path/body setup logic already correctly branches on
schemas.IsAnthropicModel(request.Model)(lines 361, 377, 399, 533, 553, 575).If an Azure deployment name doesn't encode the model type clearly (e.g.,
my-claude-prod), these mismatches can cause subtle routing bugs: you can end up using an Anthropic path but the OpenAI-style auth branch, or vice versa.To avoid this, base all Anthropic/OpenAI decisions consistently on the canonical model string:
- In
completeRequest, useschemas.IsAnthropicModel(model)instead ofdeployment.- In
ChatCompletionStreamandResponsesStream, useschemas.IsAnthropicModel(request.Model)for the auth/URL setup (lines 449, 622) to match their existing path/body setup.For example in
completeRequest:-func (provider *AzureProvider) completeRequest( +func (provider *AzureProvider) completeRequest( ctx context.Context, jsonData []byte, path string, key schemas.Key, deployment string, model string, requestType schemas.RequestType, ) ([]byte, string, time.Duration, *schemas.BifrostError) { - var url string - if schemas.IsAnthropicModel(deployment) { + var url string + if schemas.IsAnthropicModel(model) { // Anthropic-on-Azure branch... } else { // OpenAI-on-Azure branch... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
core/bifrost.go(2 hunks)core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(1 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/gemini/errors.go(2 hunks)core/providers/gemini/gemini.go(0 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/schemas/utils.go(1 hunks)core/utils.go(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/streaming/responses.go(2 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/bifrost-http/lib/config.go(4 hunks)transports/changelog.md(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/utils/utils.go
- core/providers/gemini/gemini.go
✅ Files skipped from review due to trivial changes (2)
- docs/integrations/genai-sdk.mdx
- transports/bifrost-http/integrations/router.go
🚧 Files skipped from review as they are similar to previous changes (20)
- docs/integrations/what-is-an-integration.mdx
- core/schemas/utils.go
- core/providers/azure/azure_test.go
- docs/integrations/litellm-sdk.mdx
- core/internal/testutil/account.go
- core/chatbot_test.go
- core/providers/openai/chat.go
- framework/streaming/responses.go
- transports/changelog.md
- core/providers/anthropic/chat.go
- docs/integrations/openai-sdk.mdx
- docs/features/keys-management.mdx
- ui/lib/constants/logs.ts
- ui/app/workspace/logs/views/logChatMessageView.tsx
- docs/quickstart/gateway/multimodal.mdx
- docs/quickstart/go-sdk/multimodal.mdx
- core/changelog.md
- ui/README.md
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- docs/features/unified-interface.mdx
🧰 Additional context used
🧬 Code graph analysis (6)
core/utils.go (2)
ui/lib/types/config.ts (1)
NetworkConfig(94-102)core/schemas/provider.go (1)
NetworkConfig(36-44)
core/providers/vertex/errors.go (4)
core/providers/vertex/vertex.go (1)
VertexError(25-31)core/providers/vertex/types.go (1)
VertexValidationError(154-161)core/providers/utils/utils.go (2)
NewBifrostOperationError(449-460)NewProviderAPIError(464-479)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(29-29)
transports/bifrost-http/lib/config.go (2)
ui/lib/types/config.ts (1)
NetworkConfig(94-102)core/schemas/provider.go (1)
NetworkConfig(36-44)
core/providers/gemini/errors.go (5)
core/providers/gemini/types.go (1)
GeminiGenerationError(1353-1365)core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (1)
Ptr(50-52)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(29-29)
core/providers/bedrock/bedrock.go (3)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/bedrock/types.go (1)
BedrockAnthropicTextResponse(235-239)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
core/providers/anthropic/anthropic.go (4)
core/providers/anthropic/types.go (1)
AnthropicMessageResponse(272-281)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/schemas/bifrost.go (5)
ModelProvider(32-32)BifrostResponseExtraFields(284-293)RequestType(83-83)BifrostStream(318-325)BifrostError(353-362)core/providers/utils/utils.go (4)
NewBifrostOperationError(449-460)ReleaseStreamingResponse(767-774)ProcessAndSendError(607-653)CreateBifrostChatCompletionChunkResponse(686-715)
🔇 Additional comments (23)
core/bifrost.go (1)
2025-2067: LGTM! Improved observability and terminology.The debug log at line 2025 adds useful visibility into backoff timing, and the terminology change from "retries" to "attempts" at line 2067 is more accurate since the counter includes the initial attempt.
core/providers/azure/types.go (2)
3-3: Good generalization of comment.The updated comment "Azure API version" correctly reflects that this constant now applies to multiple model types in Azure, not just OpenAI.
6-6: API version "2023-06-01" is correct and documented in official Anthropic sources.The verification confirms that "2023-06-01" is a valid default API version for Anthropic Claude models, as documented in official Anthropic documentation including the Messages API, models list, and versioning guides. The constant is properly defined.
docs/integrations/langchain-sdk.mdx (1)
221-238: Azure direct-key examples text change looks good.Renaming the comments from “Azure OpenAI” to “Azure” in both Python and JavaScript examples aligns with the new branding and doesn’t affect usage of
AzureChatOpenAIor configuration.Also applies to: 269-288
docs/integrations/anthropic-sdk.mdx (1)
104-116: Azure model examples label change is consistent.Updating the comments from “Azure OpenAI models” to “Azure models” matches the broader terminology shift and leaves the Anthropic SDK usage and model prefixes intact.
Also applies to: 151-163
docs/quickstart/go-sdk/provider-configuration.mdx (1)
272-304: Azure Go SDK provider example is consistent with the new Azure config shape.The Azure snippet correctly uses
schemas.AzureandAzureKeyConfigwith endpoint, deployments, andAPIVersionviabifrost.Ptr. This matches how other providers (Bedrock, Vertex) are configured and should compile as-is when imports are wired up in a real project.docs/quickstart/gateway/provider-configuration.mdx (1)
762-779: Azure provider-auth docs now match the new naming and config shape.Renaming the section and text from “Azure OpenAI” to “Azure” aligns with how the provider is modeled (
provider: "azure"andazure_key_config) and keeps the API/config examples accurate.core/providers/vertex/vertex.go (8)
299-336: LGTM! Anthropic model detection and request conversion.The use of
schemas.IsAnthropicModel(deployment)for routing andanthropic.ToAnthropicChatRequestfor request conversion aligns with the centralized predicate and renamed function. The anthropic_version default and model field deletion for Vertex Anthropic API are correctly handled.
373-396: LGTM! Model-based URL routing for chat completion.The URL construction logic correctly routes:
- Anthropic models →
publishers/anthropic/models/{model}:rawPredict- Mistral models →
publishers/mistralai/models/{model}:streamRawPredict- Others → OpenAPI endpoint for Gemini models
440-495: LGTM! Anthropic response handling with ModelDeployment population.The response handling correctly uses
AcquireAnthropicMessageResponse/ReleaseAnthropicMessageResponsefrom the pool, and conditionally setsModelDeploymentwhen it differs from the requested model. Both Anthropic and non-Anthropic paths consistently populate extra fields.
519-524: LGTM! Post-response converter for streaming.The
postResponseConverterclosure correctly capturesdeploymentand conditionally setsModelDeploymentfor streaming responses.
686-727: LGTM! Responses method Anthropic path configuration validation.The Anthropic path in
Responsescorrectly validatesVertexKeyConfig,projectID, andregionbefore making requests. The request body construction properly handlesanthropic_versiondefaults.
1064-1066: LGTM! Embedding path ModelDeployment population.Consistent with other methods, the Embedding path now sets
ModelDeploymentwhen it differs from the requested model.
622-639: Based on my verification of the complete authentication flow, I can now provide the rewritten review comment.Auth flow for Mistral models in ChatCompletionStream is correct; authHeader is properly set via OAuth2 token.
The reviewer's concern appears to be based on a misreading of the code flow. While
authHeaderis declared asnilat line 603, the logic at lines 641-656 ensures it gets properly set for Mistral models. Specifically:
- Mistral models do not set
authQuery(line 605 remains empty after the Mistral branch at lines 622-628)- At line 641, since
authQueryis empty for Mistral, the else branch executes (line 643)- OAuth2 token is obtained (lines 645-651) and
authHeaderis set with Bearer token (lines 653-655)- The populated
authHeaderis then passed toHandleOpenAIChatCompletionStreaming(line 669), which properly copies it to request headers (line 745-748 in openai.go)This is the intended behavior—Mistral models use OAuth2 bearer token authentication rather than API key query parameters. No changes are needed.
935-943: Based on my verification of the codebase, I can now provide the final assessment:Context key is properly isolated from post-hook interference; no unintended side effects occur.
The context value
BifrostContextKeyIsResponsesToChatCompletionFallbackis set at line 936 in vertex.go and passed toChatCompletionStream. Within the OpenAI provider'sChatCompletionStreamimplementation (openai.go:727-733), the key is read once into a local variableisResponsesToChatCompletionsFallback. This flag controls response transformation logic that completes before post-hooks are invoked. Post-hooks receive the final transformed result throughProcessAndSendResponse, not the intermediate flag. While the context is inherited by post-hooks through the plugin system (by design for full request lifecycle visibility), they cannot interfere with the response transformation that has already occurred. Context key leakage to post-hooks follows the established pattern used for all internal bifrost context keys and is intentional.core/providers/anthropic/anthropic.go (8)
30-56: LGTM! Pool and accessor renames.The pool rename from
anthropicChatResponsePooltoanthropicMessageResponsePooland correspondingAcquire/Releasefunction renames are consistent and properly exported for external use (e.g., by Vertex provider).
402-404: Clarify:nilpassed for postResponseConverter.When calling
HandleAnthropicChatCompletionStreamingfrom the Anthropic provider's ownChatCompletionStream,nilis passed forpostResponseConverter. This is intentional since no deployment mapping is needed for direct Anthropic API calls. The nil-safety check at lines 564-570 handles this correctly.
417-420: LGTM! Parameter renames and new postResponseConverter.The
providerType→providerNamerename improves clarity. The newpostResponseConverterparameter enables Vertex and other providers to inject deployment metadata into streaming responses.
564-570: LGTM! Nil-safe postResponseConverter invocation.The nil check before invoking
postResponseConverterand the warning log when it returns nil prevent panics and provide debugging visibility.
685-708: LGTM! ResponsesStream refactored to use shared handler.The
ResponsesStreammethod now builds headers explicitly and delegates toHandleAnthropicResponsesStream, reducing code duplication and enabling reuse by other providers like Vertex.
711-725: LGTM! HandleAnthropicResponsesStream function signature.The new shared streaming function mirrors
HandleAnthropicChatCompletionStreamingwith appropriate parameter types (BifrostResponsesStreamResponseinstead ofBifrostChatResponsefor the converter).
836-838: Minor: modelName extracted from event.Message only once.The
modelNameis captured only on the firstevent.Messagethat has a model. This is correct behavior for SSE streams where the model is typically only inmessage_start. No issue.
876-878: LGTM! Raw response only on last chunk of event batch.The condition
i == len(responses)-1ensures raw response is attached only to the final chunk when multiple responses are emitted from a single SSE event, avoiding duplicate raw data.
5e5008a to
04cf53b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/quickstart/gateway/multimodal.mdx (1)
288-290: Consolidation resolves the past inconsistency—verify unified-interface.mdx is accurate.This change removes the provider capability table and centralizes it in unified-interface.mdx, which is a good documentation pattern to prevent future drift. This addresses the inconsistency flagged in the previous review about Azure TTS (✅ vs ❌ across pages).
However, ensure that unified-interface.mdx has the correct and complete provider capability matrix, especially the Azure TTS capability status.
#!/bin/bash # Verify that unified-interface.mdx contains a complete provider capability matrix with Azure TTS information # Check for the presence of Azure row and TTS capability indicator grep -n -A 50 -B 5 "Azure" docs/features/unified-interface.mdx | grep -A 50 -B 5 "TTS\|Text-to-Speech\|Speech" || echo "No explicit Azure TTS reference found; manual review recommended" # Also check the overall structure of the provider matrix head -100 docs/features/unified-interface.mdx | tail -50core/providers/anthropic/anthropic.go (1)
901-904: Inconsistent error handling: missing defensive else block.Unlike
HandleAnthropicChatCompletionStreaming(lines 599-607), this function lacks anelseblock to emit a final chunk with usage when the scanner completes without error but also without receiving an explicitmessage_stopevent. While Anthropic's API should always sendmessage_stop, the defensive fallback in the chat completion handler provides better resilience against unexpected stream terminations.Consider adding the defensive else block for consistency:
if err := scanner.Err(); err != nil { logger.Warn(fmt.Sprintf("Error reading %s stream: %v", providerName, err)) providerUtils.ProcessAndSendError(ctx, postHookRunner, err, responseChan, schemas.ResponsesStreamRequest, providerName, modelName, logger) +} else { + // Defensive fallback: emit final chunk if stream ended without explicit message_stop + // This mirrors HandleAnthropicChatCompletionStreaming behavior (lines 602-607) + if usage != nil { + finalResponse := &schemas.BifrostResponsesStreamResponse{ + Response: &schemas.BifrostResponsesResponse{ + Usage: usage, + }, + ExtraFields: schemas.BifrostResponseExtraFields{ + RequestType: schemas.ResponsesStreamRequest, + Provider: providerName, + ModelRequested: modelName, + ChunkIndex: chunkIndex, + Latency: time.Since(startTime).Milliseconds(), + }, + } + ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) + providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, finalResponse, nil, nil), responseChan) + } }
🧹 Nitpick comments (7)
docs/integrations/openai-sdk.mdx (1)
319-320: Confirm Azure client + header names still match backend expectationsThe text now generically says “For Azure, you can use the AzureOpenAI client… The
x-bf-azure-endpointheader is required…”. That’s correct as long as:
- The backend still expects
x-bf-azure-endpointfor Azure traffic, andAzureOpenAIremains the recommended client for this integration.Please double-check that these names match the current Azure provider implementation and any new Anthropic-on-Azure routing logic added in this PR.
docs/apis/openapi.json (1)
1759-1788: Align deployment endpoint descriptions and parameter wordingThe summaries/descriptions for all
/openai/deployments/{deployment-id}/…routes now use “OpenAI Compatible …” phrasing, which is consistent with the rest of the spec. However:
/openai/deployments/{deployment-id}/completionsdescribes the path parameter as “Deployment ID”.- The other deployment endpoints (chat, responses, embeddings, audio/speech, audio/transcriptions) still describe it as “Azure deployment ID”.
Consider standardizing this wording across all deployment endpoints—either all generic (“Deployment ID”) or explicitly Azure-specific (“Azure deployment ID”) depending on the intended scope of these routes. That will avoid confusion for users integrating against the OpenAPI document.
Also applies to: 1881-1911, 2003-2032, 2115-2145, 2224-2236, 2332-2344
core/providers/vertex/errors.go (1)
12-30: Variable shadowing:vertexErris redeclared in inner scope.On line 12,
vertexErris declared as[]VertexError, but on line 17, it's redeclared asVertexError(singular). While this compiles and works correctly due to Go's scoping rules, it reduces readability and could cause confusion during maintenance.Consider renaming to clarify intent:
func parseVertexError(providerName schemas.ModelProvider, resp *fasthttp.Response) *schemas.BifrostError { var openAIErr schemas.BifrostError - var vertexErr []VertexError + var vertexErrs []VertexError if err := sonic.Unmarshal(resp.Body(), &openAIErr); err != nil || openAIErr.Error == nil { // Try Vertex error format if OpenAI format fails or is incomplete - if err := sonic.Unmarshal(resp.Body(), &vertexErr); err != nil { + if err := sonic.Unmarshal(resp.Body(), &vertexErrs); err != nil { //try with single Vertex error format var vertexErr VertexErrorAnd update line 31-32 accordingly:
- if len(vertexErr) > 0 { - return providerUtils.NewProviderAPIError(vertexErr[0].Error.Message, nil, resp.StatusCode(), providerName, nil, nil) + if len(vertexErrs) > 0 { + return providerUtils.NewProviderAPIError(vertexErrs[0].Error.Message, nil, resp.StatusCode(), providerName, nil, nil)core/providers/azure/azure.go (2)
361-381: Inconsistent model detection: usesrequest.Modelhere butdeploymentin streaming paths.In
ChatCompletion, the Anthropic model check on lines 361 and 377 usesrequest.Model, butChatCompletionStream(line 449) usesdeployment. Sincedeploymentis the actual Azure deployment name containing the model identifier (e.g.,"claude-3-5-sonnet-20241022"), whilerequest.Modelis the user-facing alias, usingrequest.Modelcould fail if the alias doesn't contain "claude" or "anthropic."For consistency and reliability, consider using
deploymentfor all model-family checks:- func() (any, error) { - if schemas.IsAnthropicModel(request.Model) { + func() (any, error) { + if schemas.IsAnthropicModel(deployment) { reqBody := anthropic.ToAnthropicChatRequest(request)And similarly for line 377:
var path string - if schemas.IsAnthropicModel(request.Model) { + if schemas.IsAnthropicModel(deployment) { path = "anthropic/v1/messages"And response parsing at line 399:
- if schemas.IsAnthropicModel(request.Model) { + if schemas.IsAnthropicModel(deployment) { anthropicResponse := anthropic.AcquireAnthropicMessageResponse()Based on learnings from this PR, the
deploymentvariable contains the actual model identifier needed for routing decisions.
533-557: Same inconsistency in Responses method.Lines 533 and 553 use
request.Modelfor model detection, but should usedeploymentfor consistency with streaming paths.- func() (any, error) { - if schemas.IsAnthropicModel(request.Model) { + func() (any, error) { + if schemas.IsAnthropicModel(deployment) {var path string - if schemas.IsAnthropicModel(request.Model) { + if schemas.IsAnthropicModel(deployment) { path = "anthropic/v1/messages"core/providers/anthropic/anthropic.go (2)
583-583: Consider using utility function for raw response check.For consistency with
HandleAnthropicResponsesStream(line 875-876), consider usingproviderUtils.ShouldSendBackRawResponse(ctx, sendBackRawResponse)instead of directly checkingsendBackRawResponse. The utility function properly checks both the context and provider settings.Apply this diff:
-if sendBackRawResponse { +if providerUtils.ShouldSendBackRawResponse(ctx, sendBackRawResponse) { response.ExtraFields.RawResponse = eventData }
836-838: Consider aligning modelName assignment pattern.The modelName assignment only sets the value once (when empty), whereas
HandleAnthropicChatCompletionStreaming(line 549) overwrites it each time. While this difference may be intentional for the Responses API, consider documenting the reasoning or aligning the behavior for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
core/bifrost.go(2 hunks)core/bifrost_test.go(2 hunks)core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(1 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/gemini/errors.go(2 hunks)core/providers/gemini/gemini.go(0 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/schemas/utils.go(1 hunks)core/utils.go(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/streaming/responses.go(2 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/bifrost-http/lib/config.go(4 hunks)transports/changelog.md(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/gemini/gemini.go
- core/providers/utils/utils.go
✅ Files skipped from review due to trivial changes (1)
- transports/bifrost-http/integrations/router.go
🚧 Files skipped from review as they are similar to previous changes (20)
- docs/quickstart/go-sdk/multimodal.mdx
- docs/integrations/anthropic-sdk.mdx
- ui/app/workspace/logs/views/logChatMessageView.tsx
- docs/integrations/litellm-sdk.mdx
- docs/features/keys-management.mdx
- docs/quickstart/gateway/provider-configuration.mdx
- core/providers/azure/azure_test.go
- core/providers/gemini/errors.go
- docs/integrations/what-is-an-integration.mdx
- core/schemas/utils.go
- transports/bifrost-http/lib/config.go
- core/bifrost.go
- core/providers/azure/types.go
- docs/integrations/genai-sdk.mdx
- docs/quickstart/go-sdk/provider-configuration.mdx
- docs/integrations/langchain-sdk.mdx
- ui/lib/constants/logs.ts
- framework/streaming/responses.go
- transports/changelog.md
- docs/features/unified-interface.mdx
🧰 Additional context used
🧬 Code graph analysis (9)
core/providers/bedrock/bedrock.go (3)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/bedrock/types.go (1)
BedrockAnthropicTextResponse(235-239)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
core/bifrost_test.go (2)
ui/lib/types/config.ts (1)
NetworkConfig(94-102)core/schemas/provider.go (1)
NetworkConfig(36-44)
core/providers/anthropic/chat.go (2)
core/schemas/chatcompletions.go (1)
BifrostChatRequest(12-19)core/providers/anthropic/types.go (1)
AnthropicMessageRequest(39-58)
core/providers/vertex/errors.go (4)
core/providers/vertex/vertex.go (1)
VertexError(25-31)core/providers/vertex/types.go (1)
VertexValidationError(154-161)core/providers/utils/utils.go (2)
NewBifrostOperationError(449-460)NewProviderAPIError(464-479)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(29-29)
core/providers/openai/chat.go (2)
core/schemas/utils.go (1)
IsMistralModel(1048-1050)core/schemas/models.go (1)
Model(109-129)
ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
ui/components/ui/sheet.tsx (1)
SheetContent(140-140)
core/utils.go (2)
ui/lib/types/config.ts (1)
NetworkConfig(94-102)core/schemas/provider.go (1)
NetworkConfig(36-44)
core/providers/vertex/vertex.go (9)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/providers/anthropic/anthropic.go (3)
AcquireAnthropicMessageResponse(45-49)ReleaseAnthropicMessageResponse(52-56)HandleAnthropicResponsesStream(713-908)core/providers/utils/utils.go (4)
HandleProviderResponse(343-387)CheckContextAndGetRequestBody(256-274)NewConfigurationError(435-445)NewBifrostOperationError(449-460)core/schemas/bifrost.go (7)
BifrostResponseExtraFields(284-293)RequestType(83-83)ChatCompletionRequest(89-89)BifrostError(353-362)Vertex(40-40)ResponsesRequest(91-91)BifrostStream(318-325)core/schemas/chatcompletions.go (1)
BifrostChatResponse(26-41)core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-35)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(284-383)core/providers/vertex/types.go (1)
DefaultVertexAnthropicVersion(8-8)
core/providers/azure/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)Ptr(16-18)core/providers/azure/types.go (2)
AzureAnthropicAPIVersionDefault(6-6)AzureAPIVersionDefault(4-4)core/utils.go (1)
Ptr(51-53)
🔇 Additional comments (31)
core/internal/testutil/account.go (1)
650-650: LGTM: Comment updated to reflect Azure's expanded model support.The renaming from "Azure OpenAI" to "Azure" aligns with the PR objective to reflect that Azure now supports both OpenAI and Anthropic models. The comment remains accurate since neither newer OpenAI models nor Anthropic models support text completion.
ui/app/workspace/logs/views/logDetailsSheet.tsx (2)
39-39: Addinggap-4toSheetContentis a sensible spacing tweakUsing
gap-4withflex-colwill space the main sections more evenly without altering logic. This looks good and should improve readability of the sheet content.
55-56: ButtononClickblock change is formatting-onlyThe handler still executes
handleDelete(log)followed byonOpenChange(false); only indentation/line breaks changed. No behavioral impact here.core/utils.go (1)
79-83: Backoff jitter now correctly hard‑capped at configured maxThe extra clamp on the jittered value ensures
calculateBackoffnever returns a duration aboveconfig.NetworkConfig.RetryBackoffMax, while preserving the existing exponential+jitter behavior below the cap. This matches the intended semantics and is consistent with the updated tests.core/bifrost_test.go (2)
332-335: Jitter‑bounds test correctly updated for capped backoffThe test now computes
maxExpectedusingmin(1.2*baseBackoff, RetryBackoffMax)and documents the cap, which aligns with the currentcalculateBackoffimplementation and keeps the bounds accurate under the max-backoff constraint.
353-356: Max‑backoff test now matches hard‑cap semanticsAsserting
backoff <= config.NetworkConfig.RetryBackoffMax(with the updated comment) directly verifies the new guarantee fromcalculateBackoffand removes the previous allowance for jitter to overshoot the configured maximum.ui/README.md (1)
84-88: Provider list branding looks consistentThe provider list now uses the generic “Azure” label alongside other providers, matching the rest of the PR’s branding and current provider set.
core/chatbot_test.go (3)
633-641: Synthesis request input remains correctUsing
conversationWithSynthesisas theInputfor the synthesis request is correct and maintains the intended behavior; this is effectively a formatting-only change.
774-782: Updated Azure provider help text is aligned with new brandingThe help section now refers to “Azure (Azure models)”, which is consistent with the provider enum (
schemas.Azure) and the broader shift away from “Azure OpenAI” wording.
834-843: Azure env var guidance matches new terminologyThe note about
AZURE_API_KEYandAZURE_ENDPOINT“for Azure” reflects the updated naming and keeps the CLI guidance clear for users configuring Azure deployments.docs/integrations/openai-sdk.mdx (2)
93-104: Azure usage example label matches model prefixingThe “# Azure models” label and
model="azure/gpt-4o"example are consistent with the provider-prefixed model convention used elsewhere in the docs.
135-145: JS Azure usage example label is consistentThe “// Azure models” comment correctly mirrors the Python example and the
model: "azure/gpt-4o"pattern.core/providers/openai/chat.go (1)
51-51: LGTM – centralized model detection.Using
schemas.IsMistralModelinstead of the provider-specific utility aligns with the broader refactor to centralize model-family detection across providers.core/providers/bedrock/bedrock.go (1)
529-545: LGTM – centralized model detection in TextCompletion.Using
schemas.IsAnthropicModel(deployment)andschemas.IsMistralModel(deployment)aligns with the centralized model-family detection approach. The switch routing logic correctly directs to appropriate response parsers.core/providers/anthropic/chat.go (1)
384-386: LGTM – function renamed for consistency.Renaming
ToAnthropicChatCompletionRequesttoToAnthropicChatRequestaligns with the naming conventions used elsewhere in the codebase. The implementation remains unchanged.core/providers/azure/azure.go (3)
63-71: LGTM – completeRequest signature updated for deployment-aware routing.Adding the
deploymentparameter allows proper routing based on the actual model identifier rather than the user alias.
83-102: LGTM – Anthropic vs OpenAI header routing.The routing correctly uses
IsAnthropicModel(deployment)to determine whether to use Anthropic-style (x-api-key,anthropic-version) or OpenAI-style (api-key,api-version) authentication. The URL construction appropriately differs between the two paths.
789-799: LGTM – getModelDeployment helper.The helper correctly resolves the deployment name from the key configuration, returning an error if no mapping is found.
core/providers/vertex/vertex.go (7)
299-301: LGTM – centralized model detection and updated converter.Using
schemas.IsAnthropicModel(deployment)andanthropic.ToAnthropicChatRequestaligns with the centralized approach across providers.
440-440: LGTM – centralized error parsing.Using
parseVertexError(providerName, resp)consolidates error handling logic and ensures consistent error formatting across Vertex response paths.
443-465: LGTM – Anthropic response handling with pooling.Correctly uses
AcquireAnthropicMessageResponse/ReleaseAnthropicMessageResponsefor object pooling and conditionally populatesModelDeploymentwhen it differs from the requested model.
519-524: LGTM – postResponseConverter for deployment metadata.The converter correctly attaches
ModelDeploymentto streaming responses when the deployment differs from the requested model, ensuring consistent metadata across streaming chunks.
692-812: LGTM – Anthropic Responses implementation.The implementation correctly:
- Validates key configuration
- Converts requests using
ToAnthropicResponsesRequest- Handles OAuth token authentication
- Uses centralized error parsing
- Pools response objects appropriately
- Attaches deployment metadata conditionally
842-944: LGTM – Anthropic ResponsesStream implementation.The streaming implementation correctly:
- Validates configuration before processing
- Uses
ToAnthropicResponsesRequestfor request conversion- Sets up proper streaming headers and OAuth authentication
- Attaches deployment metadata via
postResponseConverter- Falls back to
ChatCompletionStreamfor non-Anthropic models
813-829: Latency is properly preserved in the non-Anthropic Responses fallback path.The review comment is accurate. In vertex.go lines 813-829, when
ChatCompletion()is called, it returns aBifrostChatResponsewithExtraFields.Latencypopulated (calculated at line 501 of the same file). WhenToBifrostResponsesResponse()is called on line 819, the conversion method incore/schemas/mux.go:909copies the entireExtraFieldsstruct:responsesResp.ExtraFields = cr.ExtraFields. OnlyRequestTypeis then explicitly overwritten (line 910), leavingLatencyintact. The subsequent field assignments on lines 820-822 do not overwriteLatency, so it is preserved through to the returned response.core/providers/anthropic/anthropic.go (6)
30-56: LGTM! Pool and function renaming improves consistency.The renaming from
anthropicChatResponsePooltoanthropicMessageResponsePooland the associated acquire/release functions better aligns with Anthropic's message-based API terminology. The implementation correctly maintains the pool pattern and all usages are updated consistently.Also applies to: 89-89
402-402: LGTM! Post-response converter adds useful extensibility.The addition of the optional
postResponseConverterparameter enables providers (like Azure) to post-process streaming responses. The implementation includes proper nil-safety checks and gracefully handles cases where the converter returns nil by skipping the chunk.Also applies to: 419-419, 564-570
417-417: LGTM! Parameter rename improves clarity.The rename from
providerTypetoproviderNamebetter reflects that this parameter holds a provider name/identifier rather than a type. All occurrences are updated consistently across both streaming handlers.Also applies to: 454-454, 456-456, 462-462, 477-477, 556-556, 575-575, 600-601, 603-603, 721-721, 759-759, 761-761, 767-767, 782-782, 855-855, 867-867, 902-903
685-709: LGTM! Good refactoring to shared streaming helper.The refactoring of
ResponsesStreamto use the new sharedHandleAnthropicResponsesStreamfunction reduces code duplication and improves maintainability. The header construction pattern is consistent withChatCompletionStream.
711-800: LGTM! Shared streaming handler improves code reuse.The new
HandleAnthropicResponsesStreamfunction successfully extracts common streaming logic, reducing duplication between different streaming implementations. The structure mirrorsHandleAnthropicChatCompletionStreamingappropriately.
318-318: Function definition properly renamed and consistent throughout the codebase.The function definition in
core/providers/anthropic/chat.goat line 386 has been correctly renamed toToAnthropicChatRequest, and no remaining references to the old function nameToAnthropicChatCompletionRequestexist in the codebase. The call sites at lines 318 and 369 inanthropic.goare consistent with the updated definition.
04cf53b to
cfc30a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/providers/anthropic/anthropic.go (1)
663-709: Responses streaming helper is close, but consider buffer size, EOF fallback, and the unused converter hook.A few things worth tightening up here:
Scanner buffer size –
HandleAnthropicResponsesStreamuses the defaultbufio.Scannerlimit, while the chat helper explicitly bumps it to 10MB. Large SSEdata:lines for responses could hitErrTooLongand surface as generic stream errors instead of cleanly parsed events. Mirroring the chat helper’sscanner.Bufferconfiguration here would make responses streaming more robust.Unused
postResponseConverter– The function accepts apostResponseConverter func(*schemas.BifrostResponsesStreamResponse) *schemas.BifrostResponsesStreamResponsebut never calls it. If other providers start passing a non‑nil converter expecting per‑chunk adjustments, this will silently do nothing. Either wire the converter in next to where you enrichresponse.ExtraFields(similar to the chat helper), or drop the parameter until it’s needed.No final usage chunk on clean EOF without
isLastChunk– If the stream ends withoutisLastChunkever being set (e.g., missingmessage_stop), the loop exits andscanner.Err()is nil, so no final chunk with accumulatedusageand no explicit stream‑end event is emitted—only channel close. This mirrors an earlier review concern for this function. Adding anelsebranch (like the chat helper now has) to synthesize a final chunk using the accumulatedusageandmodelNamewould make this path more defensive.Also applies to: 713-725, 789-806, 851-904
🧹 Nitpick comments (10)
docs/quickstart/gateway/provider-configuration.mdx (1)
762-779: Azure provider-auth docs align with the new configuration surface, but API version examples could be standardized.The Azure section correctly reflects the
azure_key_configshape (endpoint, deployments,api_version) and the renamed “Azure” provider in the UI. One minor polish item: example API versions now differ between these docs, OpenAPI (2024-02-15-preview), and the default constant (2024-10-21). Consider standardizing to a single recommended example version (with a short note if different versions are needed for different surfaces) to avoid user confusion.Also applies to: 786-807, 813-836
docs/apis/openapi.json (1)
1758-1802: OpenAI deployment endpoint branding is clearer now; consider tightening Azure-specific wording and version examples.
- The summaries/response descriptions for
/openai/deployments/{deployment-id}/…now correctly describe these as “OpenAI compatible” endpoints, which matches their behavior.- For consistency, you may want to either:
- keep all path‑param descriptions Azure‑specific (
"Azure deployment ID"), or- generalize them all (as you did for the completions endpoint, now just
"Deployment ID"). Right now completions is generic, but chat/responses/embeddings/audio/transcriptions still say “Azure deployment ID”.Separately,
Key.azure_key_config.api_versionhere uses"2024-02-15-preview"while other docs/examples show"2024-08-01-preview"and the code default is2024-10-21. Aligning these examples (or briefly noting that multiple versions are valid) would reduce confusion for users copying from the spec.Also applies to: 1880-1924, 2001-2047, 2113-2153, 2222-2265, 2330-2372, 4359-4381
framework/streaming/responses.go (1)
588-664: Azure Anthropic streams correctly bypass OpenAI-style fast path; validate model identifier sourceThe updated condition to exclude Azure Anthropic models from the OpenAI-compatible fast path and send them through the accumulator instead is the right direction for using the Anthropic streaming handlers. The final-chunk cleanup happening while the accumulator lock is still held also matches the comment and avoids returning pooled chunks while they may still be accessed.
One thing to double-check:
schemas.IsAnthropicModel(model)in this condition must receive the canonical model/deployment identifier (not an arbitrary user alias) for Azure, otherwise Anthropic deployments whose aliases don’t containclaude/anthropic.will still be treated as OpenAI-style and skip accumulation. Please confirm whatGetResponseFieldsis populating asmodelfor Azure Responses streams.Also applies to: 684-745
core/providers/vertex/errors.go (1)
10-38: Centralized Vertex error parsing looks solid; consider minor readability tweakThe layered fallbacks (OpenAI-style
BifrostError→[]VertexError→VertexError→VertexValidationError) give good coverage and the function now correctly uses theproviderNameargument for all error constructors.Minor nit: reusing the identifier
vertexErrfor both[]VertexErrorand a nestedVertexErrormakes the function slightly harder to read. Renaming the inner one (e.g.,singleVertexErr) would clarify the intent without behavior change.core/providers/azure/azure.go (3)
245-292: TextCompletion deployment resolution and telemetry look consistentUsing
getModelDeploymentforTextCompletionand wiring the resolved deployment into the path,completeRequest, andExtraFields.ModelDeploymentmakes deployment handling explicit and consistent with other methods. This looks correct and improves error reporting when the deployment mapping is missing.
524-602: Responses and ResponsesStream Azure Anthropic routing and metadata propagation look goodFor
ResponsesandResponsesStream:
getModelDeploymentis used to resolve the deployment once.- Anthropic routing is keyed off
IsAnthropicModel(deployment)in the streaming path, matching the Chat streaming behavior.- Anthropic branches correctly swap to
anthropic/v1/messageswithx-api-key+anthropic-versionand use the shared Anthropic streaming/response handlers.- Non-Anthropic branches keep using OpenAI-style
openai/deployments/.../responsesendpoints withapi-version, Bearer/api-key auth, and OpenAI streaming helpers.postResponseConverterensuresExtraFields.ModelDeploymentis consistently set on streaming responses.Once the non-streaming methods switch their Anthropic checks to
deploymentas suggested above, Azure’s Anthropic routing will be coherent across all paths.Also applies to: 610-691
693-748: Embedding and getModelDeployment helpers are straightforward and consistentUsing
getModelDeploymentfor Embedding and wiring the resolved deployment through the URL andExtraFields.ModelDeploymentis consistent with the other methods and avoids silent misconfigurations when a deployment mapping is missing.
getModelDeploymentitself defensively checksAzureKeyConfigand theDeploymentsmap and returns a clear configuration error when a model mapping is absent, which is useful for debugging.Optional: you could reuse
getModelDeploymentinTextCompletionStreamto avoid duplicating the lookup and error message.Also applies to: 789-800
core/providers/vertex/vertex.go (3)
291-339: Vertex ChatCompletion Anthropic/OpenAPI routing and body shaping look correctChatCompletion now:
- Resolves
deploymentviagetModelDeployment.- Uses
IsAnthropicModel(deployment)to pick the Anthropic vs OpenAI-style body.- Converts Anthropic and non-Anthropic requests into
map[string]interface{}for Vertex, addinganthropic_versionand strippingmodel/regionfor Anthropic bodies.- Constructs URLs that correctly distinguish fine-tuned (numeric IDs), Anthropic publisher, Mistral publisher, and OpenAPI endpoints.
This matches the Vertex endpoint layout, and using
deployment(not alias) for dispatch is the right choice. The double marshal/unmarshal is already noted in the TODO and is acceptable for now.Also applies to: 299-337
517-525: Vertex ChatCompletionStream deployment-aware routing and converters look goodThe streaming chat path:
- Resolves
deploymentonce and usesIsAnthropicModel(deployment)to choose Anthropic streaming vs other models.- For Anthropic: builds
...:streamRawPredictURLs under the Anthropic publisher, injectsanthropic_versionand removesmodel/regionfrom the body, and uses OAuth Bearer auth with the shared Anthropic streaming helper.- For non-Anthropic: adds a separate Mistral
streamRawPredictbranch and otherwise falls back to OpenAPI endpoints, using either API-key query or OAuth Bearer.- Uses
postRequestConverterto rewritereqBody.Model = deploymentandpostResponseConverterto attachModelDeploymentwhen needed.This is a clean design and should keep streaming telemetry consistent across deployments.
Also applies to: 526-602
832-944: Vertex ResponsesStream Anthropic streaming and chat-based fallback are coherentFor streaming responses:
- Anthropic deployments use Vertex Anthropic
:streamRawPredictendpoints with appropriate headers (Accept: text/event-stream, OAuth Bearer auth), and the shared Anthropic streaming helper.postResponseConverterannotatesModelDeploymentonly when alias ≠ deployment.- Non-Anthropic deployments mark the context with
BifrostContextKeyIsResponsesToChatCompletionFallbackand reuseChatCompletionStreamon a derived chat request, which keeps the streaming stack simple and avoids duplicating logic.This structure is clear and should behave as expected for both Anthropic and non-Anthropic models.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
core/bifrost.go(2 hunks)core/bifrost_test.go(2 hunks)core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(1 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/gemini/errors.go(2 hunks)core/providers/gemini/gemini.go(0 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/schemas/utils.go(1 hunks)core/utils.go(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/streaming/responses.go(2 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/bifrost-http/lib/config.go(4 hunks)transports/changelog.md(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/gemini/gemini.go
- core/providers/utils/utils.go
✅ Files skipped from review due to trivial changes (2)
- transports/bifrost-http/integrations/router.go
- core/changelog.md
🚧 Files skipped from review as they are similar to previous changes (20)
- core/bifrost_test.go
- core/providers/gemini/errors.go
- core/bifrost.go
- ui/README.md
- ui/lib/constants/logs.ts
- docs/integrations/langchain-sdk.mdx
- core/schemas/utils.go
- docs/quickstart/go-sdk/multimodal.mdx
- transports/bifrost-http/lib/config.go
- docs/integrations/anthropic-sdk.mdx
- ui/app/workspace/logs/views/logChatMessageView.tsx
- core/chatbot_test.go
- core/utils.go
- core/internal/testutil/account.go
- docs/integrations/what-is-an-integration.mdx
- core/providers/bedrock/bedrock.go
- transports/changelog.md
- docs/quickstart/gateway/multimodal.mdx
- core/providers/anthropic/chat.go
- docs/integrations/openai-sdk.mdx
🧰 Additional context used
🧬 Code graph analysis (3)
core/providers/openai/chat.go (1)
core/schemas/utils.go (1)
IsMistralModel(1048-1050)
framework/streaming/responses.go (2)
core/schemas/bifrost.go (3)
OpenAI(35-35)OpenRouter(49-49)Azure(36-36)core/schemas/utils.go (1)
IsAnthropicModel(1043-1045)
core/providers/azure/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)Ptr(16-18)core/providers/azure/types.go (2)
AzureAnthropicAPIVersionDefault(6-6)AzureAPIVersionDefault(4-4)core/utils.go (1)
Ptr(51-53)
🔇 Additional comments (18)
docs/integrations/genai-sdk.mdx (1)
98-98: Documentation branding updates are accurate and consistent.The changes correctly update the terminology from "Azure OpenAI" to "Azure" to reflect the broader provider support (now including Anthropic models alongside OpenAI). The comments in both the Python and JavaScript examples are clear and accurately describe the "azure/gpt-4o" usage pattern.
Also applies to: 133-133
docs/quickstart/go-sdk/provider-configuration.mdx (1)
274-280: Branding update aligns well with multi-provider Azure support.The rebranding from "Azure OpenAI" to "Azure" is appropriate and reflects the PR's expansion of Azure provider to support both OpenAI and Anthropic models. The updated description and tab title are clearer and more inclusive.
The configuration example (lines 283–304) remains accurate and properly documents the Azure-specific setup pattern with deployments and API versions.
docs/features/keys-management.mdx (1)
198-198: Branding terminology updated for consistency.The heading has been appropriately updated from "Azure OpenAI:" to "Azure:" to align with the broader branding updates in this PR. The description at lines 199–201 accurately reflects Azure's deployment-based routing and remains unchanged, which is correct.
ui/app/workspace/logs/views/logDetailsSheet.tsx (2)
39-39: Addinggap-4toSheetContentis a clean spacing improvementUsing
gap-4on the column flex container should give more consistent vertical spacing between sections without affecting behavior. Looks good and matches the rest of the layout utilities in this file.
52-56: Delete handler formatting change is behavior‑preservingThe
onClickstill performshandleDelete(log)then closes the sheet viaonOpenChange(false); only indentation/brace placement changed. No functional concerns here.docs/features/unified-interface.mdx (2)
91-91: ✅ Branding update aligns with PR objectives.The rebranding from "Azure OpenAI" to "Azure" is consistent with the PR goal to standardize documentation terminology. The Azure capabilities matrix remains accurate—TTS/STT correctly marked as ❌, consistent with the prior issue resolution.
95-95: Verify Elevenlabs Models capability is intentional.The Elevenlabs row shows
Models: ❌, which is reasonable since Elevenlabs focuses on TTS/STT services rather than LLM model selection. However, please confirm this is intentional and that the provider doesn't expose a list-models endpoint for audio/transcription models.docs/integrations/litellm-sdk.mdx (1)
73-79: Azure LiteLLM examples look consistent with the new provider naming.The Azure examples (both generic and direct-key) align with the
azure/<deployment>model naming andx-bf-azure-endpointusage; no issues from a behavior or configuration standpoint.Also applies to: 146-162
core/providers/openai/chat.go (1)
47-55: Centralized Mistral detection for Vertex path looks correct.Using
schemas.IsMistralModel(bifrostReq.Model)here keeps model-family logic in one place and should correctly catch Vertex Mistral/Codestral models for compatibility transforms.core/providers/azure/types.go (1)
3-7: Azure Anthropic API version constant is a good separation.Introducing
AzureAnthropicAPIVersionDefaultalongside the genericAzureAPIVersionDefaultmakes it straightforward to keep Anthropic-on-Azure calls pinned to the right version as you wire up Anthropic deployments.core/providers/anthropic/anthropic.go (2)
30-35: Renamed Anthropic message response pool and its usage look sound.The new
anthropicMessageResponsePoolplusAcquireAnthropicMessageResponse/ReleaseAnthropicMessageResponseare wired consistently inNewAnthropicProvider,ChatCompletion, andResponses, matching the existing text-response pool pattern and avoiding extra allocations.Also applies to: 44-55, 86-90, 331-333, 638-640
314-320: Shared chat streaming helper is a solid refactor for Anthropic-compatible SSE.Routing both direct Anthropic chat streaming and other Anthropic-compatible providers through
HandleAnthropicChatCompletionStreaming(withproviderNameand optionalpostResponseConverter) centralizes SSE parsing, usage accumulation, and final‑chunk emission on clean EOF. ThepostResponseConverterhook is also a good extension point for Azure/Vertex to adjust chunks before hooks/logging.Also applies to: 365-376, 380-405, 407-611
core/providers/azure/azure_test.go (1)
25-36: Updated Azure text-model comment is accurate and consistent with branding.The note that Azure doesn’t support classic text completions on newer models matches how you configure
TextModelandTextCompletionscenarios in this test.core/providers/azure/azure.go (2)
63-105: Auth and URL handling split cleanly between Anthropic and OpenAI-style Azure pathsThe refactored
completeRequestcleanly separates Anthropic (x-api-key +anthropic-versionheader, noapi-versionquery) from OpenAI-style deployments (Bearer/api-key withapi-versionon the query string), and threading bothdeploymentandmodelthrough the call makes downstream logging and error parsing straightforward. No functional issues spotted here.
428-513: Azure ChatCompletionStream Anthropic/OpenAI split looks correct and consistentThe streaming chat method resolves
deploymentonce, usesIsAnthropicModel(deployment)for routing, and:
- For Anthropic: builds the
anthropic/v1/messagesURL, setsx-api-keyandanthropic-version, forcesStream=truein the Anthropic request, and reuses shared Anthropic streaming logic.- For non-Anthropic: uses the OpenAI-style deployments path with
api-versionand supports both Bearer and api-key auth.The
postResponseConverterthat annotatesExtraFields.ModelDeploymentis a nice touch for observability. No issues here.core/providers/vertex/vertex.go (3)
435-441: Centralized Vertex error parsing and Anthropic response conversion are wired correctlyOn non-200 responses, ChatCompletion now delegates to
parseVertexError, which centralizes handling of OpenAI-style, Vertex, and validation errors. For Anthropic deployments, the code reuses the pooledAnthropicMessageResponseand converts to aBifrostChatResponse, then overwritesExtraFieldswith the expected metadata and conditionally setsModelDeploymentwhen alias ≠ deployment. The non-Anthropic path mirrors this behavior.All of this looks consistent and should give better error messages and telemetry for Vertex.
Also applies to: 443-487
682-829: Vertex Responses Anthropic path and ChatCompletion fallback are correctThe new
Responsesimplementation:
- Validates
VertexKeyConfigand resolvesdeployment.- For Anthropic deployments: builds a Vertex Anthropic
rawPredictcall, uses OAuth Bearer auth, reusesAnthropicMessageResponseto convert into aBifrostResponsesResponse, and setsExtraFields(including latency and optionalModelDeployment), with non-200 errors routed throughparseVertexError.- For non-Anthropic deployments: delegates to
ChatCompletionand usesToBifrostResponsesResponse(), then adjustsExtraFields(RequestType,Provider,ModelRequested) while preserving latency and addingModelDeploymentonly when alias ≠ deployment.This keeps Anthropic native and other models on the existing chat-based fallback, with good extra-field hygiene.
946-977: Vertex Embedding deployment metadata propagation is a nice observability improvementEmbedding already used the native Vertex embeddings endpoint. Adding
ModelDeploymentwhen the requested model alias differs from the resolved deployment improves traceability without affecting behavior.The rest of the method (auth, error handling, and conversion) remains unchanged.
Also applies to: 1058-1067
726a7b0 to
55f264c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/anthropic/anthropic.go (1)
711-908: MissingpostResponseConverterusage in streaming loop.The function declares a
postResponseConverterparameter (line 723) but never uses it in the streaming loop (lines 863-893). This is inconsistent withHandleAnthropicChatCompletionStreaming(lines 564-570), where the converter is applied to each response chunk with proper nil checking.Apply the converter before setting ExtraFields:
for i, response := range responses { if response != nil { + if postResponseConverter != nil { + response = postResponseConverter(response) + if response == nil { + logger.Warn("postResponseConverter returned nil; skipping chunk") + continue + } + } response.ExtraFields = schemas.BifrostResponseExtraFields{ RequestType: schemas.ResponsesStreamRequest, Provider: providerName, ModelRequested: modelName, ChunkIndex: chunkIndex, Latency: time.Since(lastChunkTime).Milliseconds(), }core/providers/azure/azure.go (1)
83-128: Add conditional error parsing for Anthropic responses in Azure provider.Line 115 uses
openai.ParseOpenAIErrorfor all error responses, but the authentication routing at line 84 already branches onIsAnthropicModel(deployment). The native Anthropic provider handles errors usingHandleProviderAPIErrorwith theAnthropicErrorstruct (seecore/providers/anthropic/anthropic.golines 151-158). Azure Anthropic responses should follow the same pattern:// Handle error response if resp.StatusCode() != fasthttp.StatusOK { + if schemas.IsAnthropicModel(deployment) { + var errorResp anthropic.AnthropicError + bifrostErr := providerUtils.HandleProviderAPIError(resp, &errorResp) + bifrostErr.Error.Type = &errorResp.Error.Type + bifrostErr.Error.Message = errorResp.Error.Message + return nil, deployment, latency, bifrostErr + } return nil, deployment, latency, openai.ParseOpenAIError(resp, requestType, provider.GetProviderKey(), model) }
🧹 Nitpick comments (6)
ui/app/workspace/logs/views/logChatMessageView.tsx (1)
29-29: Consider applyingbreak-wordsconsistently to other text displays.The addition of
break-wordsprevents overflow for long unbreakable strings, which is a good improvement. However, this class is only applied here and not to other similar text displays in the component (e.g., lines 89 and 132 for thought and content strings). Consider applyingbreak-wordsto those locations as well for consistent overflow handling.Apply this diff to add
break-wordsto the thought display:-<div className="text-muted-foreground px-6 py-2 font-mono text-xs whitespace-pre-wrap italic">{message.thought}</div> +<div className="text-muted-foreground px-6 py-2 font-mono text-xs break-words whitespace-pre-wrap italic">{message.thought}</div>And to the string content display:
-<div className="px-6 py-2 font-mono text-xs whitespace-pre-wrap">{message.content}</div> +<div className="px-6 py-2 font-mono text-xs break-words whitespace-pre-wrap">{message.content}</div>core/providers/bedrock/bedrock.go (1)
520-545: Model-family detection is fine; consider falling back torequest.Modelwhen deployment is empty.Using
IsAnthropicModel/IsMistralModelondeploymentcentralizes model-family checks and the explicit default error is helpful. However, when there’s no entry inBedrockKeyConfig.Deployments,deploymentstays empty, so even valid Anthropics/Mistral model IDs (passed directly asrequest.Model) will hit the default branch.If you intend to support direct model IDs without a deployments map, consider a small tweak:
- // Handle model-specific response conversion - var bifrostResponse *schemas.BifrostTextCompletionResponse - switch { - case schemas.IsAnthropicModel(deployment): + // Handle model-specific response conversion + var bifrostResponse *schemas.BifrostTextCompletionResponse + modelID := deployment + if modelID == "" { + modelID = request.Model + } + switch { + case schemas.IsAnthropicModel(modelID): @@ - case schemas.IsMistralModel(deployment): + case schemas.IsMistralModel(modelID): @@ - default: - return nil, providerUtils.NewConfigurationError(fmt.Sprintf("unsupported model type for text completion: %s", request.Model), providerName) + default: + return nil, providerUtils.NewConfigurationError( + fmt.Sprintf("unsupported model type for text completion: %s", modelID), + providerName, + )If deployments are mandatory for TextCompletion in all current usages, you can defer this, but it’s a cheap safeguard.
framework/streaming/responses.go (1)
595-599: Routing Azure Anthropic responses away from the OpenAI final-chunk path is correct.Updating the condition to:
if provider == schemas.OpenAI || provider == schemas.OpenRouter || (provider == schemas.Azure && !schemas.IsAnthropicModel(model)) {ensures Azure Anthropic models now use the accumulation logic instead of the OpenAI-compatible final-chunk shortcut, which matches their native streaming semantics and avoids misinterpreting partial chunks as complete responses.
Also applies to: 686-745
core/schemas/utils.go (1)
1042-1050: Make IsAnthropicModel / IsMistralModel comments provider‑agnostic.The implementations are generic substring checks and are already used by Bedrock and streaming code (not just Vertex), so the “in Vertex” wording is misleading. Consider something like:
// IsAnthropicModel checks whether a model string refers to an Anthropic/Claude model. func IsAnthropicModel(model string) bool { return strings.Contains(model, "anthropic.") || strings.Contains(model, "claude") } // IsMistralModel checks whether a model string refers to a Mistral or Codestral model. func IsMistralModel(model string) bool { return strings.Contains(model, "mistral") || strings.Contains(model, "codestral") }to better reflect actual usage.
core/providers/vertex/errors.go (1)
10-38: LGTM! Centralized error parsing correctly implemented.The cascading error format parsing appropriately handles OpenAI, Vertex array, single Vertex, and validation error formats, and the previously flagged unused
providerNameparameter is now correctly used throughout. The fallback logic is sound.Optional enhancement: The "Unknown error" fallback messages at lines 27 and 34 could include a snippet of the response body for debugging purposes:
-return providerUtils.NewProviderAPIError("Unknown error", nil, resp.StatusCode(), providerName, nil, nil) +return providerUtils.NewProviderAPIError(fmt.Sprintf("Unknown error: %s", string(resp.Body())), nil, resp.StatusCode(), providerName, nil, nil)docs/apis/openapi.json (1)
1770-1770: Inconsistent deployment ID parameter descriptions after branding update.Line 1770 was updated to "Deployment ID", but the same parameter descriptions at lines 1892, 2014, 2126, 2235, and 2343 still reference "Azure deployment ID". Update all six locations consistently to align with the branding change from "Azure OpenAI" to "OpenAI Compatible".
"/openai/deployments/{deployment-id}/chat/completions": { "post": { ... "parameters": [ { "name": "deployment-id", "in": "path", "required": true, - "description": "Azure deployment ID", + "description": "Deployment ID", "schema": { "type": "string" } } ] } }, "/openai/deployments/{deployment-id}/responses": { "post": { ... "parameters": [ { "name": "deployment-id", "in": "path", "required": true, - "description": "Azure deployment ID", + "description": "Deployment ID", "schema": { "type": "string" } } ] } }, "/openai/deployments/{deployment-id}/embeddings": { "post": { ... "parameters": [ { "name": "deployment-id", "in": "path", "required": true, - "description": "Azure deployment ID", + "description": "Deployment ID", "schema": { "type": "string" } } ] } }, "/openai/deployments/{deployment-id}/audio/speech": { "post": { ... "parameters": [ { "name": "deployment-id", "in": "path", "required": true, - "description": "Azure deployment ID", + "description": "Deployment ID", "schema": { "type": "string" } } ] } }, "/openai/deployments/{deployment-id}/audio/transcriptions": { "post": { ... "parameters": [ { "name": "deployment-id", "in": "path", "required": true, - "description": "Azure deployment ID", + "description": "Deployment ID", "schema": { "type": "string" } } ] } }Also applies to: 1892-1892, 2014-2014, 2126-2126, 2235-2235, 2343-2343
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
core/bifrost.go(2 hunks)core/bifrost_test.go(2 hunks)core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(1 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/gemini/errors.go(2 hunks)core/providers/gemini/gemini.go(0 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/schemas/utils.go(1 hunks)core/utils.go(1 hunks)core/version(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/changelog.md(1 hunks)framework/streaming/responses.go(2 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/bifrost-http/lib/config.go(4 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/utils/utils.go
- core/providers/gemini/gemini.go
✅ Files skipped from review due to trivial changes (16)
- plugins/logging/version
- plugins/maxim/changelog.md
- transports/version
- plugins/mocker/changelog.md
- transports/bifrost-http/integrations/router.go
- plugins/maxim/version
- plugins/semanticcache/changelog.md
- plugins/otel/changelog.md
- framework/changelog.md
- plugins/mocker/version
- plugins/telemetry/changelog.md
- plugins/governance/version
- plugins/logging/changelog.md
- docs/integrations/anthropic-sdk.mdx
- framework/version
- plugins/jsonparser/changelog.md
🚧 Files skipped from review as they are similar to previous changes (22)
- core/internal/testutil/account.go
- core/bifrost.go
- docs/integrations/what-is-an-integration.mdx
- core/providers/gemini/errors.go
- ui/README.md
- core/providers/openai/chat.go
- transports/changelog.md
- core/changelog.md
- core/chatbot_test.go
- docs/integrations/litellm-sdk.mdx
- ui/lib/constants/logs.ts
- core/bifrost_test.go
- transports/bifrost-http/lib/config.go
- core/providers/anthropic/chat.go
- docs/quickstart/go-sdk/multimodal.mdx
- core/utils.go
- docs/quickstart/gateway/provider-configuration.mdx
- core/providers/azure/azure_test.go
- docs/quickstart/gateway/multimodal.mdx
- docs/integrations/genai-sdk.mdx
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- docs/integrations/langchain-sdk.mdx
🧰 Additional context used
🧬 Code graph analysis (5)
core/providers/bedrock/bedrock.go (3)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/bedrock/types.go (1)
BedrockAnthropicTextResponse(235-239)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
core/providers/vertex/errors.go (4)
core/providers/vertex/vertex.go (1)
VertexError(25-31)core/providers/vertex/types.go (1)
VertexValidationError(154-161)core/providers/utils/utils.go (2)
NewBifrostOperationError(449-460)NewProviderAPIError(464-479)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(29-29)
framework/streaming/responses.go (2)
core/schemas/bifrost.go (3)
OpenAI(35-35)OpenRouter(49-49)Azure(36-36)core/schemas/utils.go (1)
IsAnthropicModel(1043-1045)
core/providers/vertex/vertex.go (8)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/providers/anthropic/anthropic.go (3)
AcquireAnthropicMessageResponse(45-49)ReleaseAnthropicMessageResponse(52-56)HandleAnthropicResponsesStream(713-908)core/providers/utils/utils.go (2)
CheckContextAndGetRequestBody(256-274)NewBifrostOperationError(449-460)core/schemas/bifrost.go (5)
BifrostResponseExtraFields(284-293)RequestType(83-83)BifrostError(353-362)Vertex(40-40)BifrostStream(318-325)core/providers/openai/types.go (1)
OpenAIChatRequest(41-54)core/providers/vertex/types.go (1)
DefaultVertexAnthropicVersion(8-8)core/schemas/provider.go (1)
PostHookRunner(204-204)
core/providers/anthropic/anthropic.go (3)
core/providers/anthropic/types.go (1)
AnthropicMessageResponse(272-281)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/providers/utils/utils.go (3)
NewBifrostOperationError(449-460)CreateBifrostChatCompletionChunkResponse(686-715)ShouldSendBackRawResponse(482-487)
🔇 Additional comments (28)
plugins/telemetry/version (1)
1-1: Telemetry plugin version bump is fineVersion updated to
1.3.41; this is a straightforward metadata bump with no behavioral impact in this file.plugins/otel/version (1)
1-1: LGTM!The version bump is consistent with the broader release cycle mentioned in the changelog.
plugins/jsonparser/version (1)
1-1: LGTM!The version bump aligns with other plugin version updates in this release.
core/version (1)
1-1: LGTM!The version bump to 1.2.32 is correctly reflected in the governance plugin's changelog.
plugins/semanticcache/version (1)
1-1: LGTM!The version bump is consistent with the release cadence of other plugins.
plugins/governance/changelog.md (2)
1-1: Verify that this feat entry belongs to the current PR.The feat entry references Google Gemini x-goog-api-key header support, which doesn't appear in this PR's objectives (focused on Anthropic/Azure support). Since this is a Graphite-managed stack, this entry may belong to a different PR in the stack.
Please confirm whether this changelog entry:
- Belongs to this PR (if so, it should be mentioned in the PR objectives)
- Belongs to another PR in the Graphite stack (if so, it should be moved to that PR's changelog)
- Was previously committed but not yet documented
2-2: LGTM!The version update entry accurately reflects the core version bump to 1.2.32 observed in this PR.
docs/features/keys-management.mdx (1)
198-201: Azure label rename is consistent with broader branding updates.Renaming the section to “Azure” keeps this doc aligned with the rest of the provider naming; no functional implications here.
docs/integrations/openai-sdk.mdx (1)
99-105: Azure usage wording and examples look correct.The updated “Azure models” labels and the AzureOpenAI examples (pointing base URL to Bifrost with
x-bf-azure-endpointcarrying the real Azure resource) are coherent and match the intended integration behavior.Also applies to: 141-145, 319-333, 348-356
core/providers/azure/types.go (1)
3-6: Separate Anthropic API version constant is a good addition.Having
AzureAPIVersionDefaultandAzureAnthropicAPIVersionDefaultdistinct makes it clear which version to use per model family and keeps azure.go free to route appropriately.docs/quickstart/go-sdk/provider-configuration.mdx (1)
274-276: Azure provider-auth example correctly reflects the new AzureKeyConfig shape.Using
schemas.AzurewithAzureKeyConfig{Endpoint, Deployments, APIVersion}inschemas.Keylines up with the Azure provider changes and gives users a concrete template for configuring deployments and API versions.Also applies to: 283-303
core/providers/azure/azure.go (6)
789-800: LGTM! Clean deployment resolution helper.The function properly validates the Azure key config, performs the deployment lookup, and returns clear error messages when deployments are not found.
245-282: LGTM! TextCompletion correctly uses deployment resolution.The method properly calls
getModelDeployment, passes the deployment tocompleteRequest, and populatesModelDeploymentin the response ExtraFields.
352-425: LGTM! ChatCompletion correctly implements dual-path routing.The method consistently uses
IsAnthropicModel(deployment)for branching across request body conversion, path construction, and response parsing. The Anthropic branch properly uses pooled response objects and converts to the Bifrost format.
437-513: LGTM! Streaming implementation correctly handles both model types.The method introduces a
postResponseConverterto ensureModelDeploymentis populated in streaming chunks. Both Anthropic and OpenAI branches properly construct authentication headers and delegate to the appropriate streaming handlers.
524-601: LGTM! Responses method mirrors ChatCompletion pattern.The implementation follows the same dual-path routing pattern as
ChatCompletion, with consistent branching onIsAnthropicModel(deployment)for request conversion, path construction, and response parsing.
610-690: LGTM! ResponsesStream properly implements dual-path streaming.The method follows the
ChatCompletionStreampattern withpostResponseConverterfor deployment metadata. The OpenAI branch additionally usespostRequestConverterto set the deployment in the request, which is appropriate for the OpenAI responses endpoint.core/providers/vertex/vertex.go (7)
291-339: LGTM! Request body construction correctly branches on model type.The function properly uses
ToAnthropicChatRequestfor Anthropic models andToOpenAIChatRequestfor others. The TODO comment at line 296 acknowledges the double Marshal/Unmarshal pattern could be optimized in the future.
359-397: LGTM! URL construction correctly routes by model type.The logic appropriately distinguishes fine-tuned models (all digits), Anthropic models, Mistral models, and generic OpenAPI models, constructing the correct publisher and endpoint paths for each. Regional handling is consistent across all paths.
440-495: LGTM! Response parsing correctly handles both paths.The method uses the centralized
parseVertexErrorfor error handling and properly branches on model type. Both Anthropic and non-Anthropic paths conditionally populateModelDeploymentonly when it differs fromModelRequested, avoiding redundancy.
519-678: LGTM! Streaming correctly implements dual-path with deployment metadata.The
postResponseConverterpattern ensuresModelDeploymentis populated in all streaming chunks. Both Anthropic and non-Anthropic branches properly construct URLs with regional handling, set up authentication, and delegate to appropriate streaming handlers.
686-829: LGTM! Responses correctly implements Anthropic-specific path.The method adds a dedicated Anthropic branch (lines 692-812) with full HTTP request handling including OAuth authentication, request body construction, and response parsing. The non-Anthropic fallback to
ChatCompletion(lines 813-829) is appropriate since OpenAI-like models don't have a native responses endpoint.
836-943: LGTM! ResponsesStream properly implements streaming with fallback.The Anthropic branch (lines 842-934) implements full streaming with OAuth, request body construction, and proper delegation to
HandleAnthropicResponsesStream. The non-Anthropic fallback (lines 936-943) appropriately sets a context flag to indicate the responses-to-chat-completion fallback.
1064-1067: LGTM! Embedding method adds deployment metadata consistently.The conditional
ModelDeploymentpopulation follows the same pattern used in other methods, avoiding redundancy when the deployment matches the requested model.core/providers/anthropic/anthropic.go (4)
30-56: LGTM! Pool renaming aligns with Anthropic's Messages API terminology.The rename from
anthropicChatResponsePooltoanthropicMessageResponsePool(and corresponding Acquire/Release functions) better reflects Anthropic's Messages API naming. The pool behavior is unchanged and pre-warming is updated accordingly.Also applies to: 89-89
318-332: LGTM! Converter renaming and streaming parameter addition are correct.The rename from
ToAnthropicChatCompletionRequesttoToAnthropicChatRequestis consistent. The newpostResponseConverterparameter inHandleAnthropicChatCompletionStreamingenables providers like Azure and Vertex to inject deployment metadata while keeping the Anthropic provider simple (passesnil).Also applies to: 369-404
564-570: LGTM! postResponseConverter usage is defensive and correct.The nil check for
postResponseConverteris appropriate since it's optional. The warning log when a converter returns nil helps debugging, and skipping invalid chunks prevents downstream errors.
638-639: LGTM! Responses methods updated consistently.The
Responsesmethod uses the renamed Acquire/Release functions.ResponsesStreamnow builds a headers map and delegates to the newHandleAnthropicResponsesStreamfunction, following the same pattern asChatCompletionStream.Also applies to: 685-708
55f264c to
8570c74
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/azure/azure.go (1)
114-116: Anthropic error responses use a different format than OpenAI.Line 115 uses
openai.ParseOpenAIErrorregardless of whether the request was sent to the Anthropic or OpenAI endpoint. Anthropic's error format differs from OpenAI's (e.g., Anthropic uses{"type":"error","error":{"type":"...","message":"..."}}structure). Consider routing to an Anthropic-specific error parser whenschemas.IsAnthropicModel(deployment)is true.// Handle error response if resp.StatusCode() != fasthttp.StatusOK { - return nil, deployment, latency, openai.ParseOpenAIError(resp, requestType, provider.GetProviderKey(), model) + if schemas.IsAnthropicModel(deployment) { + return nil, deployment, latency, anthropic.ParseAnthropicError(resp, requestType, provider.GetProviderKey(), model) + } + return nil, deployment, latency, openai.ParseOpenAIError(resp, requestType, provider.GetProviderKey(), model) }
♻️ Duplicate comments (2)
docs/features/unified-interface.mdx (1)
95-95: Unresolved: Elevenlabs STT (stream) capability status remains inconsistent.A prior review flagged that the Elevenlabs row should mark STT (stream) as ✅ rather than ❌, since the provider implements
TranscriptionStreamincore/providers/elevenlabs/elevenlabs.go. This issue appears unresolved in the current code.Please verify if the STT (stream) capability for Elevenlabs is intentionally ❌ or if this is an oversight. If the provider does support streaming transcription, update line 95 to mark it as ✅.
core/providers/anthropic/anthropic.go (1)
711-908: Good implementation, but missing defensive final chunk emission.The
HandleAnthropicResponsesStreamfunction follows a consistent pattern withHandleAnthropicChatCompletionStreamingand properly tracks model name and usage. However, there's a defensive programming gap at the scanner completion path (lines 901-904):Issue: Unlike
HandleAnthropicChatCompletionStreaming(lines 602-607), this function lacks anelseblock afterscanner.Err()to emit a final chunk when the stream ends without an explicitmessage_stopevent. While Anthropic's API should always sendmessage_stop, handling unexpected EOF without it would make the code more robust.Consider adding defensive final chunk emission similar to the chat completion handler:
if err := scanner.Err(); err != nil { logger.Warn(fmt.Sprintf("Error reading %s stream: %v", providerName, err)) providerUtils.ProcessAndSendError(ctx, postHookRunner, err, responseChan, schemas.ResponsesStreamRequest, providerName, modelName, logger) + } else { + // Defensive: emit final chunk with usage if stream ended without explicit message_stop + response := &schemas.BifrostResponsesStreamResponse{ + Type: schemas.ResponsesStreamResponseTypeResponseDone, + SequenceNumber: chunkIndex, + Response: &schemas.BifrostResponsesResponse{Usage: usage}, + ExtraFields: schemas.BifrostResponseExtraFields{ + RequestType: schemas.ResponsesStreamRequest, + Provider: providerName, + ModelRequested: modelName, + ChunkIndex: chunkIndex, + Latency: time.Since(startTime).Milliseconds(), + }, + } + ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) + providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, response, nil, nil), responseChan) }Note: This echoes the past review comment that was marked as addressed but only handles the normal case (line 850 when
message_stopis received), not the edge case of unexpected stream termination.
🧹 Nitpick comments (9)
docs/quickstart/go-sdk/provider-configuration.mdx (1)
283-304: Consider adding a Claude deployment example alongside the OpenAI example.The Azure configuration example currently shows only OpenAI models (gpt-4o, gpt-4o-mini). Since the PR adds first-class support for Claude models in Azure, adding a Claude model to the deployment mappings would better demonstrate the new capability and help users understand how to configure both model families.
For example:
Deployments: map[string]string{ "gpt-4o": "gpt-4o-deployment", "gpt-4o-mini": "gpt-4o-mini-deployment", "claude-3-5-sonnet-20241022": "claude-deployment", // Claude model },ui/app/workspace/logs/views/logChatMessageView.tsx (1)
17-32: Word-breaking improvement is good; consider applying it consistentlyAdding
break-wordsonblock.textwill help with long unbroken strings and looks good. To avoid inconsistent behavior, consider adding the same class to other plain-text areas likemessage.thought(Line 89) and the stringmessage.contentbranch (Line 132) as well.For example:
- <div className="text-muted-foreground px-6 py-2 font-mono text-xs whitespace-pre-wrap italic"> + <div className="text-muted-foreground px-6 py-2 font-mono text-xs break-words whitespace-pre-wrap italic"> {message.thought} </div> - <div className="px-6 py-2 font-mono text-xs whitespace-pre-wrap"> + <div className="px-6 py-2 font-mono text-xs break-words whitespace-pre-wrap"> {message.content} </div>docs/apis/openapi.json (1)
1759-1789: Branding updates look good; align deployment parameter descriptionsThe updated summaries/descriptions to “OpenAI Compatible …” for the Azure deployment-style endpoints are consistent and match the rest of the OpenAI-compatible docs.
One minor consistency issue: the
deployment-idpath parameter description was generalized to"Deployment ID"for text completions (Line 1770), but other deployment endpoints still say"Azure deployment ID"(e.g., chat completions, responses, embeddings, speech, transcriptions).To keep the docs uniform and provider-agnostic, consider updating those remaining descriptions as well:
- "description": "Azure deployment ID", + "description": "Deployment ID",for:
/openai/deployments/{deployment-id}/chat/completions/openai/deployments/{deployment-id}/responses/openai/deployments/{deployment-id}/embeddings/openai/deployments/{deployment-id}/audio/speech/openai/deployments/{deployment-id}/audio/transcriptionsAlso applies to: 1881-1911, 2003-2033, 2115-2145, 2224-2225, 2332-2333
framework/modelcatalog/pricing.go (2)
139-146: Guard against nilusagebefore dereferencing in audio-only pathsHere you explicitly allow audio-only flows (
audioSeconds/audioTokenDetails) whenusageis nil, butusage.Costis dereferenced unconditionally on Line 144. If a caller ever passesaudioSeconds != nil(oraudioTokenDetails != nil) withusage == nil, this will panic.A small defensive change keeps current behavior for existing callers while making the function robust for true audio-only invocations:
- // Allow audio-only flows by only returning early if we have no usage data at all - if usage == nil && audioSeconds == nil && audioTokenDetails == nil { - return 0.0 - } - - if usage.Cost != nil && usage.Cost.TotalCost > 0 { - return usage.Cost.TotalCost - } + // Allow audio-only flows by only returning early if we have no usage data at all + if usage == nil && audioSeconds == nil && audioTokenDetails == nil { + return 0.0 + } + + if usage != nil && usage.Cost != nil && usage.Cost.TotalCost > 0 { + return usage.Cost.TotalCost + }
252-260: Cache read pricing change looks correct; confirm fallback semantics and completion-side usageSwitching cached prompt tokens to bill against
CacheReadInputTokenCostinstead ofCacheCreationInputTokenCostaligns with the field naming and avoids overcharging cache reads:inputCost = float64(promptTokens-cachedPromptTokens) * pricing.InputCostPerToken if pricing.CacheReadInputTokenCost != nil { inputCost += float64(cachedPromptTokens) * *pricing.CacheReadInputTokenCost }Two points worth double-checking:
Nil
CacheReadInputTokenCostsemantics
With the current logic, ifCacheReadInputTokenCostis nil, cached prompt tokens are effectively free (since they’re removed from the baseInputCostPerTokencalculation). If the intended behavior for providers without a special cache-read price is “bill cached tokens at the normal input rate,” you might instead want:inputReadRate := pricing.InputCostPerToken if pricing.CacheReadInputTokenCost != nil { inputReadRate = *pricing.CacheReadInputTokenCost } inputCost = float64(promptTokens-cachedPromptTokens) * pricing.InputCostPerToken inputCost += float64(cachedPromptTokens) * inputReadRateIf, however, “nil means free cache reads” is deliberate, the current behavior is fine—just worth confirming.
Use of
CacheCreationInputTokenCostforcachedCompletionTokens
cachedCompletionTokensare still billed viaCacheCreationInputTokenCost. If that field is indeed meant to represent the creation cost for completion-side cache entries, this is consistent; otherwise, it may be worth revisiting or documenting this mapping to avoid confusion for future maintainers.core/providers/gemini/errors.go (2)
39-64: Type inconsistency and redundant fallback logic.
StatusCode type cast inconsistency: Line 48 uses
int(resp.StatusCode())while Line 75 usesresp.StatusCode()directly. Sincefasthttp.Response.StatusCode()returnsint, the cast on Line 48 is redundant but harmless—however, the difference is confusing.Redundant fallback: If
sonic.Unmarshalfails on Line 45, the fallback on Lines 58-59 attempts to unmarshal the same body again. If the body isn't valid JSON, both will fail. Consider returning the raw body string directly instead of attempting a second unmarshal.// If JSON parsing fails, use the raw response body - var rawResponse interface{} - if err := sonic.Unmarshal(body, &rawResponse); err != nil { - return providerUtils.NewBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err, providerName) - } - - return providerUtils.NewBifrostOperationError(fmt.Sprintf("Gemini streaming error (HTTP %d): %v", resp.StatusCode(), rawResponse), fmt.Errorf("HTTP %d", resp.StatusCode()), providerName) + return providerUtils.NewBifrostOperationError(fmt.Sprintf("Gemini streaming error (HTTP %d): %s", resp.StatusCode(), string(body)), fmt.Errorf("HTTP %d", resp.StatusCode()), providerName)
66-90: Same redundant fallback logic applies here.The fallback on Lines 84-87 has the same issue as
parseStreamGeminiError—if the body isn't valid JSON forGeminiGenerationError, it likely won't parse asmap[string]interface{}either (unless it's valid JSON but with a different structure). Consider returning the raw body string directly for non-JSON responses.core/utils.go (1)
79-83: Comment is inaccurate—jitter is ±20%, not 20%.The formula
0.8 + 0.4*rand.Float64()produces values in the range [0.8, 1.2], which represents ±20% variation around the base backoff (not just +20%). The clamping logic on Lines 82-83 is correct and ensures the jittered result never exceedsRetryBackoffMax.- // Add jitter (20%) + // Add jitter (±20%)transports/bifrost-http/lib/config.go (1)
695-698: Config-file providers path uses the same normalization helper; tiny optional micro-optimizationUsing
convertNetworkConfigRetryBackoffoncfg.NetworkConfigkeeps semantics aligned with the DB and store paths, which is good. Note that when a provider already exists inprocessedProviders,cfg.NetworkConfigis currently thrown away (only keys are merged), so this conversion does a bit of redundant work in that case; if you ever touch this loop again, you could optionally guard the call behindif _, exists := processedProviders[provider]; !existsto avoid converting unused configs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (61)
core/bifrost.go(2 hunks)core/bifrost_test.go(2 hunks)core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(1 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/gemini/errors.go(2 hunks)core/providers/gemini/gemini.go(0 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/schemas/utils.go(1 hunks)core/utils.go(1 hunks)core/version(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/changelog.md(1 hunks)framework/modelcatalog/pricing.go(1 hunks)framework/streaming/responses.go(2 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/bifrost-http/lib/config.go(4 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)
💤 Files with no reviewable changes (2)
- core/providers/gemini/gemini.go
- core/providers/utils/utils.go
✅ Files skipped from review due to trivial changes (5)
- docs/integrations/what-is-an-integration.mdx
- plugins/telemetry/changelog.md
- core/version
- docs/integrations/genai-sdk.mdx
- framework/version
🚧 Files skipped from review as they are similar to previous changes (36)
- ui/README.md
- core/changelog.md
- docs/integrations/anthropic-sdk.mdx
- framework/changelog.md
- docs/features/keys-management.mdx
- transports/bifrost-http/integrations/router.go
- core/chatbot_test.go
- plugins/jsonparser/version
- framework/streaming/responses.go
- transports/version
- core/bifrost_test.go
- docs/integrations/langchain-sdk.mdx
- core/providers/azure/azure_test.go
- core/providers/anthropic/chat.go
- plugins/jsonparser/changelog.md
- core/bifrost.go
- plugins/logging/version
- plugins/telemetry/version
- plugins/maxim/changelog.md
- docs/integrations/litellm-sdk.mdx
- plugins/mocker/version
- plugins/semanticcache/changelog.md
- plugins/semanticcache/version
- core/providers/vertex/errors.go
- docs/quickstart/gateway/provider-configuration.mdx
- core/internal/testutil/account.go
- plugins/maxim/version
- plugins/governance/changelog.md
- transports/changelog.md
- core/providers/azure/types.go
- core/providers/bedrock/bedrock.go
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- docs/integrations/openai-sdk.mdx
- docs/quickstart/go-sdk/multimodal.mdx
- plugins/otel/changelog.md
- plugins/otel/version
🧰 Additional context used
🧬 Code graph analysis (7)
core/providers/openai/chat.go (1)
core/schemas/utils.go (1)
IsMistralModel(1048-1050)
core/utils.go (2)
ui/lib/types/config.ts (1)
NetworkConfig(94-102)core/schemas/provider.go (1)
NetworkConfig(36-44)
core/providers/gemini/errors.go (5)
core/providers/gemini/types.go (1)
GeminiGenerationError(1353-1365)core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (1)
Ptr(51-53)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(29-29)
core/providers/azure/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)Ptr(16-18)core/providers/azure/types.go (2)
AzureAnthropicAPIVersionDefault(6-6)AzureAPIVersionDefault(4-4)core/utils.go (1)
Ptr(51-53)
core/providers/anthropic/anthropic.go (4)
core/providers/anthropic/types.go (1)
AnthropicMessageResponse(272-281)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/schemas/bifrost.go (3)
ModelProvider(32-32)BifrostResponseExtraFields(284-293)RequestType(83-83)core/schemas/responses.go (1)
BifrostResponsesStreamResponse(1421-1459)
core/providers/vertex/vertex.go (6)
core/schemas/utils.go (3)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)Ptr(16-18)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/providers/utils/utils.go (4)
HandleProviderResponse(343-387)CheckContextAndGetRequestBody(256-274)NewConfigurationError(435-445)NewBifrostOperationError(449-460)core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-35)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(284-383)core/utils.go (1)
Ptr(51-53)
transports/bifrost-http/lib/config.go (2)
ui/lib/types/config.ts (1)
NetworkConfig(94-102)core/schemas/provider.go (1)
NetworkConfig(36-44)
🔇 Additional comments (27)
docs/quickstart/go-sdk/provider-configuration.mdx (1)
274-274: ✅ Branding update is consistent and accurate.The terminology has been cleanly updated from "Azure OpenAI" to "Azure" across the section heading, tab title, and description. This correctly reflects the broader refactoring to support both OpenAI and Claude models under a unified Azure provider.
Also applies to: 278-278, 280-280
docs/quickstart/gateway/multimodal.mdx (1)
288-290: Consolidation improves maintainability.Replacing the embedded provider support table with a link to the Unified Interface page reduces maintenance burden and establishes a single source of truth. This is a good approach for keeping documentation consistent.
docs/features/unified-interface.mdx (1)
91-91: Azure branding update aligns with PR objectives.The rename from "Azure OpenAI" to "Azure" reflects the PR's support for multiple model types (OpenAI and Anthropic) in Azure, making the provider label more accurate and generic.
plugins/logging/changelog.md (1)
1-1: LGTM!The changelog entry correctly documents the dependency version updates using conventional commit format. The "chore" prefix appropriately categorizes this as a maintenance update.
plugins/governance/version (1)
1-1: LGTM!The patch version increment is consistent with the coordinated version bumps across the codebase documented in this PR.
plugins/mocker/changelog.md (1)
1-1: Clarify the scope of this changelog entry.This entry documents version updates to
coreandframework, but this changelog file is specific to themockerplugin. Typically, a component's changelog documents changes to that component itself.Is this entry meant to record that the mocker plugin version is being bumped for coordination purposes? If so, the entry should clarify this context (e.g., "chore: bump version to coordinate with core 1.2.32 and framework 1.1.41 releases"). If the mocker plugin has functional changes related to the Anthropic/Azure updates in this PR, those should be documented more specifically.
ui/lib/constants/logs.ts (1)
42-59: Azure label rename is consistent and safeRenaming
ProviderLabels.azureto"Azure"aligns with the new branding and doesn’t affect logic. No further changes needed here.core/providers/openai/chat.go (1)
51-51: LGTM!Good refactor to use the centralized
schemas.IsMistralModelhelper instead of the provider-specific utility, improving consistency across the codebase.core/schemas/utils.go (1)
1042-1050: LGTM! Consider case sensitivity.The helper functions are clean and serve their purpose well for centralizing model-family detection. Note that these checks are case-sensitive—if model names could arrive in varying cases (e.g., "Claude" vs "claude"), you may want to use
strings.ToLowerbefore checking. However, if model names are always normalized upstream, this is fine as-is.core/providers/azure/azure.go (5)
347-426: LGTM!The ChatCompletion method correctly routes Anthropic vs OpenAI models using
deploymentfor detection, with appropriate request body conversion, path selection, and response parsing for each model type.
432-513: LGTM!The ChatCompletionStream method correctly handles both Anthropic and OpenAI streaming paths with appropriate headers, URLs, and streaming handlers. The
postResponseConverterconsistently populatesModelDeploymentfor both paths.
519-602: LGTM!The Responses method follows the same consistent pattern as ChatCompletion for routing between Anthropic and OpenAI paths.
605-691: LGTM!The ResponsesStream method correctly handles both Anthropic and OpenAI streaming paths with appropriate request preparation and streaming handlers.
789-800: LGTM!Clean helper function that properly resolves model aliases to deployment names with appropriate error handling.
core/providers/vertex/vertex.go (5)
282-496: LGTM!The ChatCompletion method is well-structured with clear routing logic for Anthropic, Mistral, fine-tuned, and OpenAI/Gemini models. The centralized model checks improve maintainability, and the response handling correctly populates
ExtraFieldsincluding conditionalModelDeployment.
498-680: LGTM!The ChatCompletionStream method cleanly separates Anthropic and non-Anthropic streaming paths with appropriate request/response converters and consistent
ModelDeploymenthandling viapostResponseConverter.
682-830: LGTM!The Responses method correctly implements a dedicated Anthropic path while reusing
ChatCompletionfor non-Anthropic models via the fallback pattern. TheToBifrostResponsesResponseconversion preservesExtraFieldsincluding latency.
832-944: LGTM!The ResponsesStream method correctly implements Anthropic streaming with a fallback to
ChatCompletionStreamfor non-Anthropic models. The context keyBifrostContextKeyIsResponsesToChatCompletionFallbackproperly signals the fallback scenario.
1100-1111: LGTM!The helper correctly falls back to the model name when no deployment mapping exists, which is appropriate for Vertex where model names can be used directly without explicit deployment configuration.
core/providers/anthropic/anthropic.go (6)
30-56: LGTM: Consistent renaming improves clarity.The renaming of
anthropicChatResponsePool→anthropicMessageResponsePooland the associated acquire/release functions aligns well with theAnthropicMessageResponsetype. All references have been updated consistently throughout the file.
318-318: LGTM: Function renaming improves consistency.The rename from
ToAnthropicChatCompletionRequesttoToAnthropicChatRequestis consistent with the broader renaming effort and improves clarity.Also applies to: 369-369
417-419: LGTM: Good extensibility design with postResponseConverter.The addition of the
postResponseConverterparameter enables Azure and Vertex providers to transform Anthropic responses appropriately. The rename fromproviderTypetoproviderNameis more semantically accurate. Passingnilfrom the native Anthropic provider (line 402) is correct since no transformation is needed.
564-570: LGTM: Proper nil-safe postResponseConverter integration.The implementation correctly applies the
postResponseConverterwhen provided, with appropriate nil checks for both the converter function and its return value. The warning log provides good observability if conversion fails.
440-462: LGTM: Consistent error handling with providerName.The error handling updates consistently use the
providerNameparameter instead of callingprovider.GetProviderKey(), which is appropriate for this shared function. The final chunk emission at lines 602-607 properly handles the case where the stream ends without an explicitmessage_stopevent.Also applies to: 600-603
685-708: LGTM: Clean refactoring with proper header construction.The
ResponsesStreammethod now properly constructs headers and delegates to the sharedHandleAnthropicResponsesStreamfunction. The pattern is consistent withChatCompletionStreamand promotes good code reuse.transports/bifrost-http/lib/config.go (2)
355-360: DB bootstrap path correctly applies retry-backoff conversion once for existing providersIn the “no config file” path, calling
convertNetworkConfigRetryBackoffondbProvider.NetworkConfigbefore buildingprocessedProvidersensures legacy DB values are normalized exactly once as they’re loaded into memory; the in-place pointer mutation plus re-wrapping intoProviderConfiglooks sound.
678-684: Store-first providers path consistently normalizes network backoff for all loaded providersHere you normalize
processedProvidersloaded from the store before any merging with config file data, which keeps in-memory configs consistent regardless of whether they originated from DB or JSON. ReassigningprocessedProviders[providerKey] = providerConfigafter mutating the embeddedNetworkConfigpointer is harmless (the pointer is already updated) and keeps the pattern explicit.
2b5557b to
75053e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
plugins/jsonparser/changelog.md (1)
1-1: Changelog entry looks good.The version update is clearly documented and follows conventional commit style. Consider including a brief summary of key features or fixes in future changelog entries for visibility (nice-to-have).
core/providers/gemini/errors.go (2)
39-55: Validate parsed error content before constructingBifrostErrorBoth
parseStreamGeminiErrorandparseGeminiErrortreat any successful unmarshal intoGeminiGenerationErroras a valid provider error:var errorResp GeminiGenerationError if err := sonic.Unmarshal(body, &errorResp); err == nil { // build BifrostError from errorResp.Error.Code / Message }If the response body is JSON but not actually in the expected error shape (e.g., missing or empty
errorfields),Unmarshalwill still succeed and you’ll end up emitting aBifrostErrorwith code"0"and an empty message, losing whatever useful details were in the payload.Consider additionally validating the parsed content before constructing the
BifrostError(for example, checking that whatever you consider mandatory inerrorResp.Erroris present / non‑empty), and falling through to the “rawResponse” path otherwise. That keeps error reporting robust if Gemini ever returns a slightly different JSON structure on error.Also applies to: 71-82
57-64: Reduce duplication and align fallback error handlingThe fallback branches in
parseStreamGeminiErrorandparseGeminiErrorare nearly identical, differing mainly in:
- The concrete type used for
rawResponse(interface{}vsmap[string]interface{}).- The error messages passed into
NewBifrostOperationError.- Use of
schemas.ErrProviderResponseUnmarshalvs a string literal for the non‑streaming case.- A small inconsistency in
StatusCodepointer construction (schemas.Ptr(int(resp.StatusCode()))vsschemas.Ptr(resp.StatusCode())).Functionally this is fine, but you could:
- Factor the common logic into a small shared helper that takes the “context” string (e.g.
"Gemini streaming error"vs"Gemini error") and maybe the desiredrawResponserepresentation.- Standardize on either the constant (
schemas.ErrProviderResponseUnmarshal) or a consistent literal for parse failures, and on one form ofStatusCodepointer construction.This would make future changes to error formatting or behavior less error‑prone.
Also applies to: 84-90
docs/apis/openapi.json (1)
1759-1788: Normalizedeployment-idparameter descriptions across deployment endpointsThe branding change from “Azure OpenAI-compatible …” to “OpenAI-compatible …” on the
/openai/deployments/{deployment-id}endpoints looks good and matches the rest of the OpenAI-compatible surface. One small consistency nit: only the/completionsendpoint now describesdeployment-idas"Deployment ID", while the chat/responses/embeddings/audio endpoints still say"Azure deployment ID".To keep the docs consistent and provider‑agnostic, consider updating the remaining deployment-id descriptions to match:
- "description": "Azure deployment ID", + "description": "Deployment ID",(Apply to the
deployment-idpath parameter under the chat, responses, embeddings, audio/speech, and audio/transcriptions deployment routes.)Also applies to: 1881-1911, 2003-2032, 2115-2144, 2224-2235, 2332-2343
core/utils.go (1)
79-83: Minor: Comment doesn't accurately reflect the jitter range.The formula
(0.8 + 0.4*rand.Float64())produces values in[0.8, 1.2], which is ±20% variation. The comment "Add jitter (20%)" should read "Add jitter (±20%)" to match the previous accurate description.The capping logic itself is correct—the second
min()at line 83 ensures post-jitter values that exceed the maximum (by up to 20%) are properly clamped.- // Add jitter (20%) + // Add jitter (±20%)core/providers/vertex/errors.go (1)
10-38: Variable shadowing in nested error parsing makes the code harder to follow.The function has multiple levels of variable shadowing:
- Line 13 & 15 & 18 & 21:
erris redeclared in each nested scope- Line 17:
vertexErr(single) shadowsvertexErr(array) from line 12While technically correct, this makes the control flow harder to trace. Consider using distinct variable names:
- var vertexErr []VertexError - if err := sonic.Unmarshal(resp.Body(), &openAIErr); err != nil || openAIErr.Error == nil { + var vertexErrs []VertexError + if unmarshalErr := sonic.Unmarshal(resp.Body(), &openAIErr); unmarshalErr != nil || openAIErr.Error == nil { // Try Vertex error format if OpenAI format fails or is incomplete - if err := sonic.Unmarshal(resp.Body(), &vertexErr); err != nil { + if arrayErr := sonic.Unmarshal(resp.Body(), &vertexErrs); arrayErr != nil { //try with single Vertex error format - var vertexErr VertexError - if err := sonic.Unmarshal(resp.Body(), &vertexErr); err != nil { + var singleVertexErr VertexError + if singleErr := sonic.Unmarshal(resp.Body(), &singleVertexErr); singleErr != nil {core/providers/azure/azure.go (1)
297-312: Inconsistent deployment resolution inTextCompletionStream.Other methods (
TextCompletion,ChatCompletion,Responses,Embedding, etc.) use the centralizedgetModelDeploymenthelper, butTextCompletionStreamstill uses inline resolution (lines 302-305). This creates inconsistency in error handling and behavior.Consider refactoring to use
getModelDeploymentfor consistency:- deployment := key.AzureKeyConfig.Deployments[request.Model] - if deployment == "" { - return nil, providerUtils.NewConfigurationError(fmt.Sprintf("deployment not found for model %s", request.Model), provider.GetProviderKey()) - } + deployment, err := provider.getModelDeployment(key, request.Model) + if err != nil { + return nil, err + }core/providers/anthropic/anthropic.go (1)
901-904: Consider adding fallback final chunk emission on scanner completion without explicit message_stop.Per the past review discussion, the
HandleAnthropicChatCompletionStreamingfunction (lines 602-607) has anelseblock that emits a final chunk with accumulated usage when the scanner completes without error. However,HandleAnthropicResponsesStreamlacks this fallback—if the stream ends without amessage_stopevent (e.g., unexpected EOF), accumulated usage data is not sent.While Anthropic's API should always send a
message_stopevent, adding a defensiveelseblock would improve robustness against unexpected stream terminations.if err := scanner.Err(); err != nil { logger.Warn(fmt.Sprintf("Error reading %s stream: %v", providerName, err)) providerUtils.ProcessAndSendError(ctx, postHookRunner, err, responseChan, schemas.ResponsesStreamRequest, providerName, modelName, logger) + } else { + // Defensive: emit final chunk if stream ended without explicit message_stop + response := &schemas.BifrostResponsesStreamResponse{ + Response: &schemas.BifrostResponsesResponse{ + Usage: usage, + }, + ExtraFields: schemas.BifrostResponseExtraFields{ + RequestType: schemas.ResponsesStreamRequest, + Provider: providerName, + ModelRequested: modelName, + ChunkIndex: chunkIndex, + Latency: time.Since(startTime).Milliseconds(), + }, + } + if postResponseConverter != nil { + response = postResponseConverter(response) + } + ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) + providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, response, nil, nil), responseChan) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (69)
core/bifrost.go(2 hunks)core/bifrost_test.go(2 hunks)core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(2 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/anthropic_test.go(1 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/gemini/errors.go(2 hunks)core/providers/gemini/gemini.go(0 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/providers/vertex/vertex_test.go(2 hunks)core/schemas/provider.go(2 hunks)core/schemas/utils.go(1 hunks)core/utils.go(1 hunks)core/version(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/changelog.md(1 hunks)framework/modelcatalog/pricing.go(1 hunks)framework/streaming/responses.go(2 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/main.go(8 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)transports/bifrost-http/handlers/providers.go(9 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/bifrost-http/lib/config.go(1 hunks)transports/bifrost-http/server/server.go(0 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (3)
- transports/bifrost-http/server/server.go
- core/providers/utils/utils.go
- core/providers/gemini/gemini.go
✅ Files skipped from review due to trivial changes (8)
- docs/integrations/what-is-an-integration.mdx
- plugins/otel/changelog.md
- transports/bifrost-http/integrations/router.go
- plugins/semanticcache/changelog.md
- plugins/maxim/changelog.md
- plugins/logging/version
- plugins/otel/version
- plugins/governance/changelog.md
🚧 Files skipped from review as they are similar to previous changes (28)
- docs/integrations/langchain-sdk.mdx
- plugins/semanticcache/version
- framework/modelcatalog/pricing.go
- docs/integrations/anthropic-sdk.mdx
- docs/integrations/litellm-sdk.mdx
- core/providers/openai/chat.go
- plugins/mocker/version
- core/chatbot_test.go
- framework/streaming/responses.go
- framework/version
- transports/version
- plugins/telemetry/version
- transports/changelog.md
- core/providers/azure/types.go
- plugins/maxim/version
- core/bifrost_test.go
- plugins/governance/version
- framework/changelog.md
- plugins/mocker/changelog.md
- core/version
- core/bifrost.go
- plugins/logging/changelog.md
- plugins/telemetry/changelog.md
- docs/quickstart/gateway/provider-configuration.mdx
- ui/app/workspace/logs/views/logChatMessageView.tsx
- docs/quickstart/go-sdk/provider-configuration.mdx
- docs/features/unified-interface.mdx
- docs/integrations/genai-sdk.mdx
🧰 Additional context used
🧬 Code graph analysis (9)
core/utils.go (2)
core/schemas/provider.go (1)
NetworkConfig(45-53)ui/lib/types/config.ts (1)
NetworkConfig(94-102)
core/providers/anthropic/chat.go (2)
core/schemas/chatcompletions.go (1)
BifrostChatRequest(12-19)core/providers/anthropic/types.go (1)
AnthropicMessageRequest(39-58)
core/providers/bedrock/bedrock.go (3)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/bedrock/types.go (1)
BedrockAnthropicTextResponse(235-239)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
core/providers/vertex/errors.go (4)
core/providers/vertex/vertex.go (1)
VertexError(25-31)core/providers/vertex/types.go (1)
VertexValidationError(154-161)core/providers/utils/utils.go (2)
NewBifrostOperationError(449-460)NewProviderAPIError(464-479)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(30-30)
ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
ui/components/ui/sheet.tsx (1)
SheetContent(140-140)
transports/bifrost-http/handlers/providers.go (3)
core/schemas/provider.go (2)
NetworkConfig(45-53)ConcurrencyAndBufferSize(128-131)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)transports/bifrost-http/lib/config.go (2)
MinRetryBackoff(43-43)MaxRetryBackoff(44-44)
plugins/logging/main.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyNumberOfRetries(111-111)
core/providers/azure/azure.go (3)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)Ptr(16-18)core/providers/azure/types.go (2)
AzureAnthropicAPIVersionDefault(6-6)AzureAPIVersionDefault(4-4)core/utils.go (1)
Ptr(51-53)
core/schemas/provider.go (1)
ui/lib/types/config.ts (1)
NetworkConfig(94-102)
🪛 LanguageTool
core/changelog.md
[style] ~3-~3: Consider using a different verb for a more formal wording.
Context: ...en serializing/deserializing from JSON, fixing issues where values were incorrectly in...
(FIX_RESOLVE)
🔇 Additional comments (57)
plugins/jsonparser/version (1)
1-1: Version bump to 1.3.42 looks goodSingle-line version update only; no issues from this file in isolation.
plugins/logging/main.go (7)
192-196: Minor doc-comment formatting changes look fine.The empty
//line additions in the doc comments are stylistic and don't affect functionality.
205-209: Doc-comment formatting is consistent with the other changes.
334-338: Doc-comment formatting is consistent.
359-370: NumberOfRetries retrieval and assignment look correct.The value is properly retrieved from context using
getIntFromContextwith the correct context key (BifrostContextKeyNumberOfRetries), and assigned to the log message for later propagation.
399-402: NumberOfRetries correctly passed to updateLogEntry for error case.
438-442: NumberOfRetries correctly passed to updateStreamingLogEntry.
565-568: NumberOfRetries correctly passed to updateLogEntry for success case.The propagation of
NumberOfRetriesis consistent across all three code paths (error, streaming, and regular response).plugins/logging/operations.go (3)
25-41: Initial log entry creation changes look correct.The struct initialization reformatting and addition of new parsed fields (
ParamsParsed,ToolsParsed,SpeechInputParsed,TranscriptionInputParsed) are consistent with the schema.
57-76: Verify the intent of skippingnumber_of_retrieswhen value is 0.The condition
if numberOfRetries != 0means that when there are no retries (value is 0), the field won't be included in the update. This could be intentional to avoid unnecessary database writes, but it also means you can't distinguish between "0 retries" and "retries not tracked" in the logs.If distinguishing these cases matters, consider using a pointer type
*intor always setting the field.
183-200: Streaming update follows the same pattern for numberOfRetries.The logic mirrors the non-streaming update function, maintaining consistency. The same consideration about
!= 0check applies here.docs/quickstart/go-sdk/multimodal.mdx (1)
335-337: Documentation consolidation improves maintainability.Replacing inline provider capability tables with a single reference to the Unified Interface page reduces duplication and provides a single source of truth. However, ensure the referenced page includes all updated capabilities, especially the newly added Azure Anthropic support.
A previous review flagged an inconsistency in Azure Text-to-Speech capability status across documentation pages. Please verify that the Unified Interface page (docs/features/unified-interface.mdx) accurately reflects current Azure multimodal capabilities before merging this change.
core/internal/testutil/account.go (2)
195-196: LGTM!The new
claude-opus-4-5deployment entry correctly enables testing of Anthropic models on Azure, aligning with the PR's objective to add Claude support in Azure.
651-651: LGTM!Comment updated to reflect the broader "Azure" branding, consistent with the PR's documentation changes.
docs/features/keys-management.mdx (1)
198-198: LGTM!The heading update from "Azure OpenAI" to "Azure" accurately reflects the expanded provider capabilities (supporting both OpenAI and Anthropic models).
transports/bifrost-http/handlers/providers.go (3)
228-234: LGTM!Good addition of retry backoff validation before persisting the provider config. This prevents invalid configurations from being stored.
414-423: LGTM!Creating a local copy
ncbefore validation and assignment is a clean approach that avoids mutating the payload directly.
871-896: LGTM!The
validateRetryBackofffunction is well-implemented with:
- Proper nil check for networkConfig
- Bounds validation against
lib.MinRetryBackoffandlib.MaxRetryBackoff- Relationship validation ensuring initial ≤ max
- Only validates non-zero values, allowing defaults to be applied later
core/schemas/provider.go (3)
37-53: LGTM!Excellent documentation explaining the JSON serialization semantics. The comments clearly describe the milliseconds-in-JSON vs. nanoseconds-in-Go duality.
55-90: LGTM!The
UnmarshalJSONimplementation correctly:
- Uses an alias type to prevent infinite recursion
- Interprets JSON values as milliseconds and converts to
time.Duration- Only converts positive values, allowing zero to trigger default handling
92-117: LGTM!The
MarshalJSONimplementation correctly convertstime.Durationback to milliseconds for JSON output, maintaining consistency with the UI's expected format.core/providers/anthropic/chat.go (1)
384-386: LGTM!The function rename from
ToAnthropicChatCompletionRequesttoToAnthropicChatRequestimproves consistency with the naming conventions used across the codebase.ui/README.md (1)
84-88: Provider branding text is consistent with codeUpdating the supported providers list to use “Azure” (instead of “Azure OpenAI”) matches the new
ProviderLabels.azurevalue and the broader branding shift. No further changes needed here.ui/lib/constants/logs.ts (1)
42-59: Azure provider label update aligns with new brandingRenaming
ProviderLabels.azureto “Azure” keeps logs UI consistent with the updated documentation and README while preserving the existing provider key ("azure"). Looks good.transports/bifrost-http/lib/config.go (1)
41-45: LGTM - well-documented retry backoff constants.The constants are appropriately typed as
time.Durationand clearly documented. The 1000-second maximum is generous but provides flexibility for edge cases with long-running operations.ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
39-56: LGTM - minor UI layout improvement.Adding
gap-4to SheetContent provides consistent vertical spacing between child elements. The button formatting change is purely cosmetic with no behavioral impact.core/providers/vertex/vertex_test.go (1)
30-46: LGTM - good documentation of disabled feature.The commented-out
ReasoningModelhint and explicitReasoning: falsewith explanatory comment clearly document the current limitation and intended future direction. This is helpful for maintainability.core/providers/bedrock/bedrock.go (1)
529-545: LGTM - good refactor to centralized model detection.Switching to
schemas.IsAnthropicModelandschemas.IsMistralModelutilities centralizes model-family detection logic, improving consistency across providers. The newdefaultcase with an explicit error is a valuable addition that prevents silent failures for unsupported model types.core/providers/azure/azure_test.go (1)
32-34:claude-opus-4-5is properly configured for Azure reasoning tests.The model is explicitly registered in Azure's deployment configuration (testutil/account.go:195), confirming it is a valid and supported identifier for Azure Anthropic deployments. The test setup is correct and the environment has the corresponding deployment configured.
core/providers/anthropic/anthropic_test.go (1)
31-32: LGTM!The addition of
ReasoningModelwithclaude-opus-4-5properly complements theReasoning: truescenario configuration and aligns with the test infrastructure for validating reasoning capabilities.core/changelog.md (1)
1-3: LGTM!Clear and informative changelog entries. The breaking change documentation properly explains the behavioral change and the fix applied for JSON millisecond handling.
docs/integrations/openai-sdk.mdx (1)
99-99: LGTM!The terminology update from "Azure OpenAI" to "Azure" correctly reflects that the Azure provider now supports multiple model families (OpenAI and Anthropic), making the documentation more accurate and future-proof.
Also applies to: 141-141, 319-319
ui/lib/types/schemas.ts (1)
217-222: LGTM with observation: Validation bounds are appropriate.The bounds (100ms–1,000,000ms) are sensible for retry backoff configuration in a form context.
Note:
networkConfigSchema(lines 184-185) has slightly different constraints (retry_backoff_maxmin is 1000ms vs 100ms here). This appears intentional—the form schema is more lenient during input, while the stricter schema applies to final configuration. Verify this asymmetry is expected.core/providers/azure/azure.go (5)
63-127: LGTM!The
completeRequestrefactoring properly:
- Accepts
deploymentas a parameter for consistent routing- Uses
IsAnthropicModel(deployment)for model-family detection (per learnings)- Sets appropriate headers:
x-api-key+anthropic-versionfor Anthropic,api-key/Bearer for OpenAI- Constructs correct URLs for each path
352-426: LGTM!
ChatCompletioncorrectly:
- Resolves deployment upfront via
getModelDeployment- Routes request transformation, URL path, and response parsing based on
IsAnthropicModel(deployment)- Uses pooled Anthropic response objects with proper acquire/release pattern
432-513: LGTM!
ChatCompletionStreamcorrectly branches between Anthropic and OpenAI streaming:
- Anthropic: sets
x-api-key,anthropic-version, usesHandleAnthropicChatCompletionStreaming- OpenAI: uses
api-key/Bearer, usesHandleOpenAIChatCompletionStreaming- Both paths apply the
postResponseConverterfor deployment metadata
519-602: LGTM!
ResponsesandResponsesStreamfollow the same consistent pattern:
- Deployment resolution via
getModelDeployment- Anthropic/OpenAI routing via
IsAnthropicModel(deployment)- Appropriate converters (
ToAnthropicResponsesRequest/ToOpenAIResponsesRequest)- Correct streaming handlers for each path
Also applies to: 605-691
789-800: LGTM!The
getModelDeploymenthelper provides centralized deployment resolution with proper validation and clear error messages.core/schemas/utils.go (1)
1041-1050: Case-sensitivity is not a practical concern—cloud providers use lowercase model identifiers.The helpers correctly match lowercase substrings. Real-world model identifiers from Anthropic, Google Vertex, and AWS Bedrock APIs are always lowercase (e.g.,
"claude-3-sonnet-20241022","mistral-7b"), so the current implementation handles all production cases correctly. The case-insensitive refactor is unnecessary.core/providers/vertex/vertex.go (9)
299-339: LGTM! Clean model-family routing for chat completion request body.The refactored request body construction properly uses
schemas.IsAnthropicModelfor centralized model detection and routes to the appropriate converter (ToAnthropicChatRequestvsToOpenAIChatRequest). The Anthropic-specific version handling and field cleanup are correctly applied.
373-397: LGTM! Endpoint URL construction properly routes by model family.The URL logic correctly handles four distinct cases:
- Fine-tuned models (all-digit deployment) → OpenAPI endpoint with API key query
- Anthropic models → Anthropic publisher rawPredict
- Mistral models → mistralai publisher rawPredict
- Other models (Gemini) → OpenAPI endpoint
440-495: LGTM! Response handling uses renamed pool functions correctly.The error parsing is now centralized via
parseVertexError, and the Anthropic response path correctly uses the renamedAcquireAnthropicMessageResponse/ReleaseAnthropicMessageResponsefunctions. ExtraFields population including conditionalModelDeploymentis consistent.
519-524: LGTM! Post-response converter for streaming deployment metadata.Clean closure that conditionally sets
ModelDeploymentwhen it differs from the requested model, consistent with non-streaming paths.
526-601: LGTM! Anthropic streaming path with proper header and converter setup.The streaming logic correctly:
- Converts request via
ToAnthropicChatRequest- Sets up OAuth headers for Vertex authentication
- Passes
postResponseConverterto attach deployment metadata
658-677: LGTM! Non-Anthropic streaming path with converters.The
postRequestConverterandpostResponseConverterare correctly passed to the OpenAI streaming handler for model deployment and response metadata.
684-829: LGTM! Full Anthropic support in Responses method.The new Anthropic path properly:
- Validates configuration upfront
- Converts request via
ToAnthropicResponsesRequest- Handles OAuth authentication
- Uses
parseVertexErrorfor error handling- Populates ExtraFields including conditional
ModelDeploymentThe non-Anthropic fallback correctly delegates to
ChatCompletionand transforms the response.
833-944: LGTM! ResponsesStream with Anthropic and ChatCompletion fallback.The implementation correctly:
- Validates configuration and routes by model family
- Uses
HandleAnthropicResponsesStreamfor Anthropic models with proper headers and converter- Falls back to
ChatCompletionStreamfor non-Anthropic models with context flag for fallback indication
1064-1066: LGTM! Embedding deployment metadata consistency.The conditional
ModelDeploymentassignment matches the pattern used in other methods.core/providers/anthropic/anthropic.go (9)
30-56: LGTM! Clean pool and function renaming.The rename from
anthropicChatResponsePooltoanthropicMessageResponsePool(and corresponding acquire/release functions) better reflects that this pool handlesAnthropicMessageResponseobjects, improving code clarity.
318-332: LGTM! ChatCompletion uses renamed functions consistently.The
ToAnthropicChatRequestandAcquireAnthropicMessageResponse/ReleaseAnthropicMessageResponsecalls are correctly updated.
369-404: LGTM! ChatCompletionStream updated for shared streaming.The
ToAnthropicChatRequestusage is correct, and passingnilforpostResponseConverteris appropriate since the Anthropic provider doesn't need deployment metadata transformation.
407-421: LGTM! Shared streaming signature extended for flexibility.The addition of
postResponseConverterparameter enables providers like Vertex to attach deployment metadata while keeping the Anthropic provider's usage simple withnil.
564-570: Good nil-safety for postResponseConverter.The check ensures that if
postResponseConverterreturnsnil, the chunk is skipped with a warning rather than causing a panic. This defensive coding handles edge cases gracefully.
685-708: LGTM! ResponsesStream refactored to use shared helper.The headers map construction is clean, and delegation to
HandleAnthropicResponsesStreamwithnilforpostResponseConvertermatches the pattern fromChatCompletionStream.
711-725: LGTM! HandleAnthropicResponsesStream signature mirrors chat completion.The shared streaming helper has a consistent signature with
HandleAnthropicChatCompletionStreaming, enabling code reuse across Vertex and Anthropic providers.
836-838: LGTM! Model name tracking for stream events.Capturing
modelNamefrom the firstmessage_startevent ensures error reporting and response metadata include the correct model identifier.
638-639: LGTM! Responses method uses renamed pool functions.Consistent with the rest of the file.
75053e8 to
ec258f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/apis/openapi.json (1)
1757-1804: Inconsistent parameter description in deployment-id—should align with updated branding strategy.The changes to lines 1759-1760 and 1788 correctly rebrand to "OpenAI Compatible," but line 1892's parameter description still references "Azure deployment ID" instead of the generic "Deployment ID" applied at line 1770.
Similarly, the same inconsistency appears in the responses (line 2014), embeddings (line 2126), speech (line 2235), and transcriptions (line 2343) endpoints—some remain "Azure deployment ID" while others should align with the branding shift.
For consistency, update all remaining "Azure deployment ID" descriptions to "Deployment ID" across all deployment endpoints:
"/openai/deployments/{deployment-id}/chat/completions": { "post": { "parameters": [ { "name": "deployment-id", "in": "path", "required": true, - "description": "Azure deployment ID", + "description": "Deployment ID",Apply the same change at:
- Line 1892 (chat/completions)
- Line 2014 (responses)
- Line 2126 (embeddings)
- Line 2235 (audio/speech)
- Line 2343 (audio/transcriptions)
🧹 Nitpick comments (7)
ui/app/workspace/logs/views/logChatMessageView.tsx (1)
29-29: Good addition, consider applying consistently.Adding
break-wordsprevents text overflow for long unbroken strings in content blocks.For consistency, consider applying the same class to other text displays that use
whitespace-pre-wrap:Line 89 (thought content):
-<div className="text-muted-foreground px-6 py-2 font-mono text-xs whitespace-pre-wrap italic">{message.thought}</div> +<div className="text-muted-foreground px-6 py-2 font-mono text-xs break-words whitespace-pre-wrap italic">{message.thought}</div>Line 132 (message content string):
-<div className="px-6 py-2 font-mono text-xs whitespace-pre-wrap">{message.content}</div> +<div className="px-6 py-2 font-mono text-xs break-words whitespace-pre-wrap">{message.content}</div>framework/streaming/responses.go (1)
684-745: Anthropic-on-Azure routing change looks correct; ensure model detection is exhaustiveTightening the predicate to:
if provider == schemas.OpenAI || provider == schemas.OpenRouter || (provider == schemas.Azure && !schemas.IsAnthropicModel(model)) { // OpenAI-compatible final-chunk path }correctly routes Azure Anthropic models to the accumulation logic while preserving the existing fast-path for OpenAI/OpenRouter and non-Anthropic Azure models. This aligns with the intent to treat Azure Anthropic streams like other non-OpenAI-compatible providers.
Two small follow-ups to consider:
- Verify that all expected Azure Anthropic deployment names (e.g., both
anthropic.claude-...andclaude-...forms) are covered byschemas.IsAnthropicModel(model)incore/schemas/utils.go, especially if users can customize deployment names.- Optional: if similar “OpenAI-compatible” checks exist elsewhere, you might factor this predicate into a helper (e.g.,
schemas.IsOpenAICompatibleProvider(provider, model)) to keep routing rules centralized.To double-check coverage, please run a small test matrix hitting streaming for:
- Azure non-Anthropic (e.g., GPT-style) models and confirm they stay on the final-chunk path.
- Azure Anthropic (Claude) models and confirm they use the accumulation path and produce complete
OutputMessagesandTokenUsage.core/providers/gemini/errors.go (2)
39-64: Inconsistent fallback unmarshaling between streaming and non-streaming error parsers.
parseStreamGeminiErrorusesinterface{}for fallback (line 58), whileparseGeminiErrorusesmap[string]interface{}(line 84). If Gemini returns a non-object error (e.g., an array),parseGeminiErrorwill fail to unmarshal it whileparseStreamGeminiErrorwill succeed.Consider using
interface{}consistently in both functions for robustness:- var rawResponse map[string]interface{} + var rawResponse interface{}
84-87: Inconsistent error message with streaming counterpart.Line 86 uses a hardcoded string
"failed to parse error response"while the streaming function (line 60) uses the constantschemas.ErrProviderResponseUnmarshal. For consistency and maintainability, use the same constant.if err := sonic.Unmarshal(body, &rawResponse); err != nil { - return providerUtils.NewBifrostOperationError("failed to parse error response", err, providerName) + return providerUtils.NewBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err, providerName) }core/providers/vertex/errors.go (1)
15-29: Variable shadowing: innervertexErrshadows outer declaration.Line 17 declares a new
vertexErr VertexErrorwhich shadows thevertexErr []VertexErrorfrom line 12. While this works correctly, it can cause confusion during debugging.Consider renaming for clarity:
if err := sonic.Unmarshal(resp.Body(), &vertexErr); err != nil { //try with single Vertex error format - var vertexErr VertexError - if err := sonic.Unmarshal(resp.Body(), &vertexErr); err != nil { + var singleVertexErr VertexError + if err := sonic.Unmarshal(resp.Body(), &singleVertexErr); err != nil { // Try VertexValidationError format (validation errors from Mistral endpoint) var validationErr VertexValidationError if err := sonic.Unmarshal(resp.Body(), &validationErr); err != nil { return providerUtils.NewBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err, providerName) } if len(validationErr.Detail) > 0 { return providerUtils.NewProviderAPIError(validationErr.Detail[0].Msg, nil, resp.StatusCode(), providerName, nil, nil) } return providerUtils.NewProviderAPIError("Unknown error", nil, resp.StatusCode(), providerName, nil, nil) } - return providerUtils.NewProviderAPIError(vertexErr.Error.Message, nil, resp.StatusCode(), providerName, nil, nil) + return providerUtils.NewProviderAPIError(singleVertexErr.Error.Message, nil, resp.StatusCode(), providerName, nil, nil) }core/providers/vertex/vertex.go (1)
299-336: Redundant model-family check.Line 331 re-checks
schemas.IsAnthropicModel(deployment)inside the same if-block that already checked it at line 299. While not harmful, you can eliminate the nested check by moving theanthropic_versionlogic directly into the outer block.Apply this diff to simplify:
if schemas.IsAnthropicModel(deployment) { // Use centralized Anthropic converter reqBody := anthropic.ToAnthropicChatRequest(request) if reqBody == nil { return nil, fmt.Errorf("chat completion input is not provided") } reqBody.Model = deployment // Convert struct to map for Vertex API reqBytes, err := sonic.Marshal(reqBody) if err != nil { return nil, fmt.Errorf("failed to marshal request body: %w", err) } if err := sonic.Unmarshal(reqBytes, &requestBody); err != nil { return nil, fmt.Errorf("failed to unmarshal request body: %w", err) } + if _, exists := requestBody["anthropic_version"]; !exists { + requestBody["anthropic_version"] = DefaultVertexAnthropicVersion + } + delete(requestBody, "model") } else { // Use centralized OpenAI converter for non-Claude models reqBody := openai.ToOpenAIChatRequest(request) if reqBody == nil { return nil, fmt.Errorf("chat completion input is not provided") } reqBody.Model = deployment // Convert struct to map for Vertex API reqBytes, err := sonic.Marshal(reqBody) if err != nil { return nil, fmt.Errorf("failed to marshal request body: %w", err) } if err := sonic.Unmarshal(reqBytes, &requestBody); err != nil { return nil, fmt.Errorf("failed to unmarshal request body: %w", err) } } - - if schemas.IsAnthropicModel(deployment) { - if _, exists := requestBody["anthropic_version"]; !exists { - requestBody["anthropic_version"] = DefaultVertexAnthropicVersion - } - delete(requestBody, "model") - } delete(requestBody, "region") return requestBody, nilcore/providers/anthropic/anthropic.go (1)
901-904: Missing defensive final chunk emission on clean EOF.Unlike
HandleAnthropicChatCompletionStreaming(lines 602-607), this function lacks anelseblock to emit accumulated usage when the scanner completes successfully without an explicitmessage_stopevent. While Anthropic's API should always sendmessage_stop, adding a defensive final chunk would make the code more robust against unexpected stream terminations.Apply this diff to add defensive handling:
if err := scanner.Err(); err != nil { logger.Warn(fmt.Sprintf("Error reading %s stream: %v", providerName, err)) providerUtils.ProcessAndSendError(ctx, postHookRunner, err, responseChan, schemas.ResponsesStreamRequest, providerName, modelName, logger) + } else { + // Emit final chunk with usage if stream ended without explicit message_stop + response := &schemas.BifrostResponsesStreamResponse{ + Response: &schemas.BifrostResponsesResponse{}, + } + if usage != nil { + response.Response.Usage = usage + } + response.ExtraFields = schemas.BifrostResponseExtraFields{ + RequestType: schemas.ResponsesStreamRequest, + Provider: providerName, + ModelRequested: modelName, + ChunkIndex: chunkIndex, + Latency: time.Since(startTime).Milliseconds(), + } + ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) + providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, response, nil, nil), responseChan) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (69)
core/bifrost.go(2 hunks)core/bifrost_test.go(2 hunks)core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(2 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/anthropic_test.go(1 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/gemini/errors.go(2 hunks)core/providers/gemini/gemini.go(0 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/providers/vertex/vertex_test.go(2 hunks)core/schemas/provider.go(2 hunks)core/schemas/utils.go(1 hunks)core/utils.go(1 hunks)core/version(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/changelog.md(1 hunks)framework/modelcatalog/pricing.go(1 hunks)framework/streaming/responses.go(2 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/main.go(8 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)transports/bifrost-http/handlers/providers.go(9 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/bifrost-http/lib/config.go(1 hunks)transports/bifrost-http/server/server.go(0 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (3)
- transports/bifrost-http/server/server.go
- core/providers/utils/utils.go
- core/providers/gemini/gemini.go
✅ Files skipped from review due to trivial changes (6)
- framework/changelog.md
- docs/integrations/genai-sdk.mdx
- plugins/otel/changelog.md
- transports/bifrost-http/integrations/router.go
- plugins/telemetry/version
- plugins/semanticcache/changelog.md
🚧 Files skipped from review as they are similar to previous changes (38)
- docs/quickstart/go-sdk/multimodal.mdx
- plugins/semanticcache/version
- docs/integrations/openai-sdk.mdx
- core/providers/azure/types.go
- ui/lib/constants/logs.ts
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- plugins/logging/version
- core/providers/vertex/vertex_test.go
- core/providers/openai/chat.go
- core/bifrost_test.go
- transports/version
- core/chatbot_test.go
- docs/quickstart/gateway/multimodal.mdx
- core/providers/anthropic/chat.go
- framework/modelcatalog/pricing.go
- core/bifrost.go
- docs/integrations/what-is-an-integration.mdx
- docs/quickstart/go-sdk/provider-configuration.mdx
- plugins/maxim/changelog.md
- core/internal/testutil/account.go
- docs/quickstart/gateway/provider-configuration.mdx
- core/version
- docs/integrations/langchain-sdk.mdx
- core/schemas/utils.go
- docs/features/keys-management.mdx
- core/providers/anthropic/anthropic_test.go
- plugins/governance/version
- transports/bifrost-http/lib/config.go
- docs/integrations/anthropic-sdk.mdx
- framework/version
- plugins/jsonparser/changelog.md
- plugins/otel/version
- plugins/maxim/version
- plugins/mocker/changelog.md
- ui/lib/types/schemas.ts
- plugins/governance/changelog.md
- docs/features/unified-interface.mdx
- plugins/jsonparser/version
🧰 Additional context used
🧬 Code graph analysis (8)
core/utils.go (2)
core/schemas/provider.go (1)
NetworkConfig(45-53)ui/lib/types/config.ts (1)
NetworkConfig(94-102)
framework/streaming/responses.go (2)
core/schemas/bifrost.go (3)
OpenAI(35-35)OpenRouter(49-49)Azure(36-36)core/schemas/utils.go (1)
IsAnthropicModel(1043-1045)
core/schemas/provider.go (1)
ui/lib/types/config.ts (1)
NetworkConfig(94-102)
core/providers/bedrock/bedrock.go (3)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/bedrock/types.go (1)
BedrockAnthropicTextResponse(235-239)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
plugins/logging/main.go (1)
core/schemas/bifrost.go (1)
BifrostContextKeyNumberOfRetries(111-111)
core/providers/vertex/errors.go (4)
core/providers/vertex/vertex.go (1)
VertexError(25-31)core/providers/vertex/types.go (1)
VertexValidationError(154-161)core/providers/utils/utils.go (2)
NewBifrostOperationError(449-460)NewProviderAPIError(464-479)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(30-30)
core/providers/azure/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)Ptr(16-18)core/providers/azure/types.go (2)
AzureAnthropicAPIVersionDefault(6-6)AzureAPIVersionDefault(4-4)core/utils.go (1)
Ptr(51-53)
transports/bifrost-http/handlers/providers.go (4)
core/schemas/provider.go (2)
NetworkConfig(45-53)ConcurrencyAndBufferSize(128-131)ui/lib/types/config.ts (2)
NetworkConfig(94-102)ConcurrencyAndBufferSize(105-108)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)transports/bifrost-http/lib/config.go (2)
MinRetryBackoff(43-43)MaxRetryBackoff(44-44)
🪛 LanguageTool
core/changelog.md
[style] ~3-~3: Consider using a different verb for a more formal wording.
Context: ...en serializing/deserializing from JSON, fixing issues where values were incorrectly in...
(FIX_RESOLVE)
🔇 Additional comments (46)
docs/integrations/litellm-sdk.mdx (1)
73-73: LGTM!The section header updates accurately reflect that Azure now supports both OpenAI and Anthropic models, not just Azure OpenAI. The changes align with the PR's branding updates and improve documentation clarity.
Also applies to: 146-146
ui/README.md (1)
84-84: Documentation update aligns with PR objectives.The branding change from "Azure OpenAI" to "Azure" accurately reflects the feature addition of Anthropic (Claude) model support alongside existing OpenAI models in the Azure provider. The provider list is clear and complete.
plugins/logging/changelog.md (1)
1-2: Verify changelog completeness and version accuracy.The changelog entries follow a clear, conventional format. However, given the broader PR scope (Anthropic support for Azure, framework updates, retry handling improvements), verify that:
- These entries capture all relevant changes to the logging plugin in this PR
- The version numbers (core 1.2.32, framework 1.1.41) are correct and correspond to the actual version bumps
- If the logging plugin had changes related to Anthropic support or other Providers/Integrations updates mentioned in the PR, they should be documented here
If the logging plugin's scope is limited to the version bump and retry fix, these entries are appropriate. Otherwise, consider whether additional entries are needed.
plugins/telemetry/changelog.md (1)
1-1: The changelog entry is correct and follows project conventions.This entry accurately documents the version cascade from core and framework dependency updates. The logging plugin uses the identical format for the same version updates, confirming this is the standard pattern in the project. Telemetry plugin itself had no functional changes—only a version bump due to dependency updates—which is appropriately documented as a
choreentry.Likely an incorrect or invalid review comment.
plugins/logging/main.go (4)
359-370: LGTM!The
numberOfRetriesis correctly retrieved from the context and assigned to the log message. This ensures retry metrics are captured for all request types.
390-403: Correctly propagates retry count in error path.The
numberOfRetriesis properly passed toupdateLogEntryensuring retry metrics are logged even for failed requests.
430-443: LGTM!Streaming log updates correctly include the retry count.
556-569: LGTM!Regular (non-streaming) log updates correctly include the retry count, completing the consistent propagation across all update paths.
plugins/logging/operations.go (3)
24-41: LGTM!The initial log entry correctly omits
numberOfRetriessince retries haven't occurred yet at PreHook time. The retry count is appropriately set later via update functions.
57-76: LGTM!The
numberOfRetriesparameter is correctly added and conditionally included in updates only when non-zero, which is an appropriate optimization.
183-200: LGTM!Consistent implementation with
updateLogEntry. The streaming log update function correctly handles thenumberOfRetriesparameter with the same conditional logic.framework/streaming/responses.go (1)
592-599: Comment accurately documents lock/cleanup orderingThe updated comment now clearly matches the existing behavior (cleanup while holding the lock, then unlock), which is correct for preventing other goroutines from seeing chunks after they’re returned to the pool. No functional issues here.
core/providers/bedrock/bedrock.go (1)
528-545: LGTM! Good refactor to centralize model-type detection.The switch from inline string containment checks to
schemas.IsAnthropicModel()andschemas.IsMistralModel()helpers improves code maintainability and ensures consistent model detection across all providers. The explicit default case with a descriptive error message is a good addition.core/changelog.md (1)
1-3: LGTM! Changelog entries accurately document the changes.The breaking change entry for NetworkConfig retry backoff values is well-documented with clear explanation of the behavioral change (milliseconds in JSON vs. nanoseconds internally).
transports/changelog.md (1)
1-5: LGTM! Changelog entries are well-organized.The entries accurately reflect the features, fixes, and version updates in this PR.
core/schemas/provider.go (2)
55-90: LGTM! Custom JSON marshaling correctly handles millisecond interop.The alias type pattern properly avoids infinite recursion, and the millisecond conversion logic aligns with the UI TypeScript types (
ui/lib/types/config.tslines 97-98). The> 0guard appropriately prevents conversion of unset values.
92-117: LGTM! MarshalJSON implementation is symmetric with UnmarshalJSON.The conversion from
time.Durationto milliseconds via integer division is appropriate for backoff configuration values.transports/bifrost-http/handlers/providers.go (5)
228-234: LGTM!Retry backoff validation is correctly placed before config persistence, with appropriate error messaging.
286-290: LGTM!The
fasthttp.RequestCtxrecycling issue has been correctly addressed by usingcontext.Background()in the goroutine.
414-423: LGTM!The validation flow correctly validates the network config before assignment.
465-469: LGTM!The
fasthttp.RequestCtxrecycling issue has been correctly addressed.
871-896: Consider edge case when only one backoff value is provided.When
RetryBackoffInitial > 0butRetryBackoffMax == 0(or vice versa), the cross-validation at lines 889-893 is skipped. If defaults are applied elsewhere, ensure the provided value doesn't conflict with the default. For example, if a user setsRetryBackoffInitial = 500msbutRetryBackoffMaxdefaults to200ms, this could cause issues.If defaults are guaranteed to be sane and this is handled elsewhere, this is fine as-is.
core/providers/azure/azure_test.go (1)
32-34: Verify reasoning test compatibility with Claude model.The
ReasoningModelchanged from"o1"(OpenAI) to"claude-opus-4-5"(Anthropic). Ensure the reasoning test scenarios are compatible with Claude's response format, as Claude's "extended thinking" differs from OpenAI's reasoning model behavior.core/providers/vertex/errors.go (1)
10-38: LGTM overall - error parsing handles multiple formats correctly.The function properly tries multiple error formats (OpenAI, Vertex array, single Vertex, validation error) and uses
providerNameconsistently. The past review issue regarding the unused parameter has been addressed.core/providers/azure/azure.go (8)
22-28: LGTM!Adding
networkConfigandsendBackRawResponseto the provider struct improves encapsulation.
113-116: Verify error parsing for Anthropic responses.Line 115 uses
openai.ParseOpenAIErrorfor all error responses, including Anthropic models. If Azure's Anthropic endpoint returns Anthropic-style errors (different structure from OpenAI), this may produce unclear error messages or miss error details.Consider branching error parsing based on model type:
if resp.StatusCode() != fasthttp.StatusOK { - return nil, deployment, latency, openai.ParseOpenAIError(resp, requestType, provider.GetProviderKey(), model) + if schemas.IsAnthropicModel(deployment) { + return nil, deployment, latency, anthropic.ParseAnthropicError(resp, requestType, provider.GetProviderKey(), model) + } + return nil, deployment, latency, openai.ParseOpenAIError(resp, requestType, provider.GetProviderKey(), model) }The Anthropic API error format uses "a top-level error object that always includes a type and message value" - this differs from OpenAI's error structure. The current code may not extract error details correctly for Anthropic responses from Azure.
352-412: LGTM!The
ChatCompletionmethod correctly:
- Resolves deployment first via
getModelDeployment- Uses
IsAnthropicModel(deployment)consistently for routing decisions (addressing past review feedback)- Properly branches request body conversion and response parsing for Anthropic vs OpenAI
449-513: LGTM!The streaming implementation correctly branches based on
IsAnthropicModel(deployment):
- Anthropic path: uses
x-api-keyandanthropic-versionheaders, delegates toHandleAnthropicChatCompletionStreaming- OpenAI path: uses
api-keyor Bearer token, delegates toHandleOpenAIChatCompletionStreaming
519-602: LGTM!The
Responsesmethod follows the same correct pattern asChatCompletion, with proper Anthropic/OpenAI branching for request conversion, path construction, and response parsing.
604-691: LGTM!The
ResponsesStreammethod correctly implements the same streaming pattern asChatCompletionStreamwith proper Anthropic/OpenAI branching.
789-800: LGTM!The
getModelDeploymenthelper correctly validates configuration and looks up the deployment from the Azure key config.
294-342: LGTM!
TextCompletionStreamcorrectly uses only the OpenAI path since Anthropic models don't support the text completion API (they use the Messages API instead).core/providers/vertex/vertex.go (8)
443-471: LGTM!The Anthropic response handling correctly uses the renamed pool functions and properly populates ExtraFields with deployment metadata.
486-488: LGTM!The conditional ModelDeployment assignment ensures consistent metadata tracking across both Anthropic and non-Anthropic paths.
519-524: LGTM!The
postResponseConvertercorrectly injects deployment metadata into streaming responses.
526-601: LGTM!The Anthropic streaming path correctly uses the renamed converter, constructs headers, and passes the
postResponseConverterto the shared streaming handler.
658-661: LGTM!The
postRequestConvertercorrectly sets the deployment model name, ensuring consistent request handling across OpenAI-compatible endpoints.
683-829: LGTM!The Responses method correctly implements Anthropic-specific handling with proper ExtraFields population and falls back to ChatCompletion for non-Anthropic models, preserving latency through the conversion.
914-919: LGTM!The
postResponseConverterfor ResponsesStream correctly mirrors the pattern used in ChatCompletionStream, ensuring consistent deployment metadata injection.
1064-1066: LGTM!The conditional ModelDeployment assignment in the Embedding method ensures consistent metadata tracking across all request types.
core/providers/anthropic/anthropic.go (6)
30-56: LGTM!The pool renaming from
anthropicChatResponsePooltoanthropicMessageResponsePoolis consistently applied across all accessor functions.
318-318: LGTM!The renamed
ToAnthropicChatRequestfunction is correctly used in ChatCompletion.
417-420: LGTM!The signature changes correctly add the
postResponseConverterhook and renameproviderTypeto the more accurateproviderName.
564-570: LGTM!The nil-safety check for
postResponseConverterprevents panics and provides helpful debugging output when a converter returns nil.
685-708: LGTM!The ResponsesStream implementation correctly constructs headers as a map and delegates to the shared
HandleAnthropicResponsesStreamhandler.
836-838: LGTM!The modelName tracking correctly captures the model name from the stream events and uses it in error handling and response metadata.
07c2a7d to
e0c170f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/logging/operations.go (1)
163-170: Operator precedence issue may cause unintended behavior.The condition
p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nilevaluates as(nil) || ((!*p.disableContentLogging) && (data.RawResponse != nil))due to&&having higher precedence than||.This means when
p.disableContentLoggingisnil, the block executes regardless of whetherdata.RawResponseisnil, potentially marshaling a nil value.Add parentheses to clarify intent:
- if p.disableContentLogging == nil || !*p.disableContentLogging && data.RawResponse != nil { + if (p.disableContentLogging == nil || !*p.disableContentLogging) && data.RawResponse != nil {core/providers/azure/azure.go (1)
114-116: Use Anthropic-specific error parsing for Anthropic endpoints.When the status code is not OK, the code calls
openai.ParseOpenAIErrorfor both Anthropic and OpenAI responses. Anthropic error responses have a different structure ({"type": "error", "error": {"type": "...", "message": "..."}}) than OpenAI, and the standalone Anthropic provider handles this correctly by parsing theAnthropicErrorstruct. The Azure provider should use similar Anthropic-specific error parsing for Anthropic deployments instead of the OpenAI parser.core/providers/anthropic/anthropic.go (1)
583-583: Refactor: Inconsistent raw response handling between streaming functions.Line 583 in
HandleAnthropicChatCompletionStreamingcheckssendBackRawResponsedirectly:if sendBackRawResponse {However, line 876 in
HandleAnthropicResponsesStreamuses a utility wrapper:if providerUtils.ShouldSendBackRawResponse(ctx, sendBackRawResponse) && i == len(responses)-1 {The
ShouldSendBackRawResponsehelper likely checks for context-based overrides in addition to the boolean flag. For consistency and correctness, both functions should use the same pattern.Update line 583 to use the utility wrapper:
- if sendBackRawResponse { + if providerUtils.ShouldSendBackRawResponse(ctx, sendBackRawResponse) { response.ExtraFields.RawResponse = eventData }Also applies to: 876-876
♻️ Duplicate comments (1)
core/utils.go (1)
75-84: LGTM on clamping logic; comment issue persists.The additional clamping of the final result (
min(result, config.NetworkConfig.RetryBackoffMax)) correctly ensures the backoff never exceeds the configured maximum even after jitter is applied, which is an important safety guarantee.However, the comment on line 79 still says "Add jitter (20%)" when it should say "Add jitter (±20%)" to accurately reflect the range [0.8, 1.2]. This was previously flagged in past review comments.
Apply this diff to fix the comment:
- // Add jitter (20%) + // Add jitter (±20%)
🧹 Nitpick comments (8)
plugins/maxim/changelog.md (1)
1-1: Consider adding a more descriptive changelog entry for feature visibility.The current entry only documents the version bump. Given the PR's scope (Anthropic/Claude model support in Azure, deployment routing, streaming enhancements, etc.), a more detailed changelog entry would help users understand what's new in this release.
Example of a more informative entry:
-- chore: version update core to 1.2.32 and framework to 1.1.41 +- feat: add Anthropic/Claude model support in Azure provider with native streaming +- feat: implement deployment-aware routing and headers for Azure Anthropic models +- fix: centralize Vertex error parsing for improved error handling +- chore: rename Anthropic APIs for consistency and update branding from "Azure OpenAI" to "Azure" +- chore: version update core to 1.2.32 and framework to 1.1.41Alternatively, if minimal version-only entries are your project's standard, this is fine as-is.
ui/app/workspace/logs/views/logChatMessageView.tsx (1)
29-29: Good call addingbreak-words; consider aligning other text blocks for consistencyAdding
break-wordshere should help prevent horizontal scrolling for long, unbroken log text while preservingwhitespace-pre-wrap. Looks correct and low‑risk.You may optionally also add
break-wordsto other non‑JSON text containers (e.g., the plainmessage.contentandmessage.thoughtdivs) so wrapping behavior is consistent across all text sections in this view.core/schemas/utils.go (1)
1042-1050: Consider case-insensitive matching and update function comments.Two observations:
The comments reference "in Vertex" but per the AI summary, these helpers are also used in Bedrock. Consider updating to "checks if the model is an Anthropic/Mistral model" without provider-specific context.
The matching is case-sensitive. If model strings could arrive in mixed case (e.g., "Claude-3" or "MISTRAL"), this would fail to match.
-// IsAnthropicModel checks if the model is an Anthropic model in Vertex. +// IsAnthropicModel checks if the model is an Anthropic model. func IsAnthropicModel(model string) bool { - return strings.Contains(model, "anthropic.") || strings.Contains(model, "claude") + lowerModel := strings.ToLower(model) + return strings.Contains(lowerModel, "anthropic.") || strings.Contains(lowerModel, "claude") } -// IsMistralModel checks if the model is a Mistral or Codestral model in Vertex. +// IsMistralModel checks if the model is a Mistral or Codestral model. func IsMistralModel(model string) bool { - return strings.Contains(model, "mistral") || strings.Contains(model, "codestral") + lowerModel := strings.ToLower(model) + return strings.Contains(lowerModel, "mistral") || strings.Contains(lowerModel, "codestral") }core/providers/gemini/errors.go (1)
39-64: Minor inconsistencies between error parsing functions.The two new error parsing functions have similar logic but some inconsistencies:
StatusCode type casting (lines 48 vs 75):
parseStreamGeminiError:schemas.Ptr(int(resp.StatusCode()))parseGeminiError:schemas.Ptr(resp.StatusCode())Raw response fallback type (lines 58 vs 84):
parseStreamGeminiError:var rawResponse interface{}parseGeminiError:var rawResponse map[string]interface{}These inconsistencies don't cause bugs but reduce maintainability.
// parseStreamGeminiError parses Gemini streaming error responses func parseStreamGeminiError(providerName schemas.ModelProvider, resp *fasthttp.Response) *schemas.BifrostError { body := append([]byte(nil), resp.Body()...) // Try to parse as JSON first var errorResp GeminiGenerationError if err := sonic.Unmarshal(body, &errorResp); err == nil { bifrostErr := &schemas.BifrostError{ IsBifrostError: false, - StatusCode: schemas.Ptr(int(resp.StatusCode())), + StatusCode: schemas.Ptr(resp.StatusCode()), Error: &schemas.ErrorField{ Code: schemas.Ptr(strconv.Itoa(errorResp.Error.Code)), Message: errorResp.Error.Message, }, } return bifrostErr } // If JSON parsing fails, use the raw response body - var rawResponse interface{} + var rawResponse map[string]interface{} if err := sonic.Unmarshal(body, &rawResponse); err != nil { return providerUtils.NewBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err, providerName) }Also applies to: 66-90
docs/apis/openapi.json (1)
1759-1760: Deployment-specific OpenAI-compatible docs look good; consider unifyingdeployment-idwordingThe branding/summary/description updates for the
/openai/deployments/{deployment-id}/…endpoints correctly describe them as OpenAI-compatible deployment-scoped APIs and align with the rest of the OpenAI integration docs.One minor nit: for some deployment endpoints you now use the generic
"Deployment ID"wording for thedeployment-idpath param, while others still say"Azure deployment ID". For consistency across all/openai/deployments/{deployment-id}/…routes, consider normalizing these descriptions to the same generic phrasing.Also applies to: 1770-1770, 1788-1788, 1881-1882, 1910-1910, 2003-2004, 2032-2032, 2115-2116, 2144-2144, 2224-2225, 2332-2333
core/providers/azure/types.go (1)
3-6: Azure Anthropic default API version constant is fine; consider config-driven override laterAdding
AzureAnthropicAPIVersionDefault = "2023-06-01"cleanly separates Anthropic API versioning from the main Azure default. Over time, if regions or deployments diverge on supported versions, you may want this to be driven via per-key or per-deployment config rather than a single global constant, but it’s perfectly reasonable for now.framework/modelcatalog/utils.go (1)
21-30: Bedrock normalization innormalizeProviderimproves catalog consistencyNormalizing any provider string containing
"bedrock"toschemas.Bedrockshould deduplicate pricing/catalog entries that come from different Bedrock labels (e.g., vendor-prefixed names) into a single provider bucket. If more aliases appear for other providers later, this pattern is a good place to extend them.core/providers/anthropic/anthropic.go (1)
836-838: Edge case:modelNamemay remain empty if first event lacks message.The
modelNameis only set whenevent.Message != nil && modelName == ""(lines 836-838). If the first event that should populatemodelNamedoesn't contain aMessagefield,modelNamewill remain an empty string for all subsequent error messages and response metadata.While Anthropic's API should reliably include the model name in the
message_startevent, consider adding a fallback or logging a warning ifmodelNameremains empty after the stream completes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (70)
core/bifrost.go(2 hunks)core/bifrost_test.go(2 hunks)core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(2 hunks)core/providers/anthropic/anthropic.go(22 hunks)core/providers/anthropic/anthropic_test.go(1 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/gemini/errors.go(2 hunks)core/providers/gemini/gemini.go(0 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/providers/vertex/vertex_test.go(2 hunks)core/schemas/provider.go(2 hunks)core/schemas/utils.go(1 hunks)core/utils.go(1 hunks)core/version(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/changelog.md(1 hunks)framework/modelcatalog/pricing.go(7 hunks)framework/modelcatalog/utils.go(1 hunks)framework/streaming/responses.go(2 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/main.go(8 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)transports/bifrost-http/handlers/providers.go(9 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/bifrost-http/lib/config.go(1 hunks)transports/bifrost-http/server/server.go(0 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (3)
- core/providers/utils/utils.go
- transports/bifrost-http/server/server.go
- core/providers/gemini/gemini.go
✅ Files skipped from review due to trivial changes (4)
- framework/version
- transports/changelog.md
- plugins/semanticcache/version
- docs/features/keys-management.mdx
🚧 Files skipped from review as they are similar to previous changes (31)
- core/providers/bedrock/bedrock.go
- core/bifrost_test.go
- ui/README.md
- core/providers/anthropic/chat.go
- transports/version
- plugins/telemetry/version
- plugins/telemetry/changelog.md
- framework/streaming/responses.go
- docs/quickstart/gateway/multimodal.mdx
- plugins/mocker/version
- plugins/mocker/changelog.md
- plugins/jsonparser/changelog.md
- framework/changelog.md
- docs/quickstart/go-sdk/multimodal.mdx
- docs/integrations/genai-sdk.mdx
- core/bifrost.go
- docs/integrations/litellm-sdk.mdx
- plugins/jsonparser/version
- transports/bifrost-http/handlers/providers.go
- ui/app/workspace/logs/views/logDetailsSheet.tsx
- transports/bifrost-http/lib/config.go
- plugins/logging/version
- docs/integrations/what-is-an-integration.mdx
- core/version
- plugins/maxim/version
- plugins/otel/changelog.md
- core/providers/vertex/vertex_test.go
- plugins/semanticcache/changelog.md
- plugins/governance/version
- plugins/logging/main.go
- docs/features/unified-interface.mdx
🧰 Additional context used
🧬 Code graph analysis (9)
framework/modelcatalog/utils.go (1)
core/schemas/bifrost.go (1)
Bedrock(38-38)
core/providers/gemini/errors.go (4)
core/providers/gemini/types.go (1)
GeminiGenerationError(1353-1365)core/schemas/utils.go (1)
Ptr(16-18)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(30-30)
core/providers/openai/chat.go (2)
core/schemas/utils.go (1)
IsMistralModel(1048-1050)core/schemas/models.go (1)
Model(109-129)
core/schemas/provider.go (1)
ui/lib/types/config.ts (1)
NetworkConfig(94-102)
framework/modelcatalog/pricing.go (6)
core/schemas/provider.go (1)
Provider(280-307)core/schemas/bifrost.go (5)
RequestType(83-83)Bedrock(38-38)ResponsesRequest(91-91)ResponsesStreamRequest(92-92)ChatCompletionRequest(89-89)core/schemas/chatcompletions.go (1)
BifrostLLMUsage(640-647)framework/modelcatalog/main.go (1)
ModelCatalog(31-52)core/schemas/transcriptions.go (1)
TranscriptionUsageInputTokenDetails(106-109)core/schemas/utils.go (1)
IsAllDigitsASCII(39-50)
core/providers/vertex/vertex.go (5)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/providers/utils/utils.go (2)
CheckContextAndGetRequestBody(256-274)NewBifrostOperationError(449-460)core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-35)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(284-383)
core/providers/vertex/errors.go (4)
core/providers/vertex/vertex.go (1)
VertexError(25-31)core/providers/vertex/types.go (1)
VertexValidationError(154-161)core/providers/utils/utils.go (2)
NewBifrostOperationError(449-460)NewProviderAPIError(464-479)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(30-30)
core/providers/anthropic/anthropic.go (4)
core/providers/anthropic/types.go (1)
AnthropicMessageResponse(272-281)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)core/schemas/responses.go (1)
BifrostResponsesStreamResponse(1421-1459)
core/utils.go (2)
core/schemas/provider.go (1)
NetworkConfig(45-53)ui/lib/types/config.ts (1)
NetworkConfig(94-102)
🪛 LanguageTool
core/changelog.md
[style] ~3-~3: Consider using a different verb for a more formal wording.
Context: ...en serializing/deserializing from JSON, fixing issues where values were incorrectly in...
(FIX_RESOLVE)
🔇 Additional comments (45)
plugins/otel/version (1)
1-1: Version bump looks goodPlain patch-level version increment to
1.0.41; no issues from this file’s perspective.plugins/logging/changelog.md (1)
1-2: LGTM!Changelog entries are clear and accurately describe the version bump and the fix for propagating
numberOfRetriesthrough update paths.plugins/logging/operations.go (4)
24-46: LGTM!The refactoring to remove
numberOfRetriesfrom the initial log entry is correct—retry count is meaningfully known only after request completion and is now properly propagated through the update paths. The additional parsed fields enhance structured logging.
57-57: LGTM!The
numberOfRetriesparameter addition and conditional update logic are correctly implemented, ensuring retry metadata is only written when retries actually occurred.Also applies to: 74-76
183-183: LGTM!The
numberOfRetriesparameter and conditional update logic are consistent withupdateLogEntry.Also applies to: 198-200
201-213: Verify if key metadata should be preserved on error.When
ErrorDetailsis present, the function returns early with a new map containing onlystatus,latency, anderror_details. This discards the previously built updates includingselected_key_id,selected_key_name,virtual_key_id,virtual_key_name, andnumber_of_retries.If this metadata is valuable for debugging failed requests, consider merging the error fields into the existing
updatesmap instead of creating a new one.transports/bifrost-http/integrations/router.go (1)
304-328: LGTM - formatting adjustments only.The changes in this section are purely formatting adjustments (variable declaration positioning and brace formatting) with no functional impact on request processing logic.
ui/lib/constants/logs.ts (1)
45-45: LGTM - branding update consistent with PR scope.The label change from "Azure OpenAI" to "Azure" aligns with the PR's goal of standardizing Azure branding across the codebase.
docs/integrations/anthropic-sdk.mdx (1)
104-109: LGTM - documentation branding updates.The comment updates from "Azure OpenAI models" to "Azure models" are consistent with the PR's branding standardization efforts.
plugins/governance/changelog.md (1)
1-2: LGTM - changelog updates.Standard changelog entries documenting feature additions and version bumps.
core/internal/testutil/account.go (2)
195-197: LGTM - test configuration for Anthropic deployment support.The addition of the "claude-opus-4-5" deployment entry aligns with the PR's goal of adding Anthropic model support to the Azure provider. The parallel addition of "o1" deployment supports OpenAI reasoning models.
651-651: LGTM - branding update in test comments.Consistent with the broader branding standardization from "Azure OpenAI" to "Azure".
docs/quickstart/go-sdk/provider-configuration.mdx (1)
274-280: LGTM - documentation branding updates.The updates from "Azure OpenAI" to "Azure" in headings and descriptive text are consistent with the PR's branding standardization goals.
ui/lib/types/schemas.ts (1)
217-222: Verify the relaxed minimum forretry_backoff_max.The validation changes add sensible upper bounds (1,000,000 ms), but the minimum for
retry_backoff_maxwas reduced from 1000 ms to 100 ms. This more permissive validation now allows configurations where bothretry_backoff_initialandretry_backoff_maxcould be as low as 100 ms, which may not provide meaningful exponential backoff behavior in practice.While the cross-field validation still ensures
initial <= max, a maximum backoff of 100 ms might be too tight for typical retry scenarios. Consider whether this relaxation was intentional or if the minimum forretry_backoff_maxshould remain higher (e.g., 1000 ms) to ensure effective backoff ranges.docs/integrations/openai-sdk.mdx (1)
99-99: LGTM! Branding updates align with Azure Anthropic support.The documentation updates correctly reflect the expanded Azure provider capabilities (now supporting both OpenAI and Anthropic models), making the terminology more accurate.
Also applies to: 141-141, 319-319
docs/quickstart/gateway/provider-configuration.mdx (1)
762-840: Documentation update for Azure configuration looks comprehensive.The updated Azure configuration section clearly documents the new
azure_key_configstructure with endpoint, deployments, and api_version fields across all configuration methods (Web UI, API, config.json).One consideration: If Azure Anthropic deployments require a different API version than Azure OpenAI deployments (per the AI summary mentioning
AzureAnthropicAPIVersionDefault), it may be helpful to add a note about API version differences between model types.core/schemas/provider.go (1)
55-117: Well-implemented JSON marshaling for NetworkConfig backoff fields.The custom
UnmarshalJSONandMarshalJSONmethods correctly handle the conversion between Go'stime.Duration(nanoseconds) and JSON milliseconds. The alias pattern properly avoids infinite recursion, and the implementation aligns with the UI's TypeScript interface which expects milliseconds.core/providers/azure/azure_test.go (1)
32-35: Azure comprehensive test config now exercises Anthropic reasoning – looks consistentThe updated TextModel comment and the ReasoningModel change to
"claude-opus-4-5"accurately reflect that newer Azure deployments are chat-only while letting this suite cover the new Anthropic-on-Azure reasoning path. No issues from the test harness perspective.docs/integrations/langchain-sdk.mdx (1)
221-221: Azure direct-key headings now match intent; samples remain validRenaming these sections to “Using Azure with direct Azure key” clarifies the scenario without affecting the underlying examples. The
AzureChatOpenAIusage andx-bf-azure-endpointheader still look correct.Also applies to: 269-269
core/providers/anthropic/anthropic_test.go (1)
31-33: Anthropic test config cleanly separates vision and reasoning modelsSetting
VisionModelto"claude-3-7-sonnet-20250219"andReasoningModelto"claude-opus-4-5"gives clear coverage of both capabilities in the comprehensive test suite. This mirrors how the new ReasoningModel is used elsewhere.core/providers/openai/chat.go (1)
51-53: Centralizing Vertex Mistral detection viaschemas.IsMistralModelis appropriateUsing
schemas.IsMistralModel(bifrostReq.Model)in the Vertex branch keeps Mistral-specific transformations applied only when the model string clearly indicates a Mistral/Codestral family, and removes the dependency on provider-specific helpers. Given the surroundingcase schemas.Vertex, this should preserve behavior while simplifying model-family checks.core/chatbot_test.go (1)
637-637: Chatbot synthesis and Azure guidance tweaks are purely cosmeticThe synthesis request still uses
conversationWithSynthesisas intended; the updated Azure descriptions inprintHelpand the env-variable guidance now match the broader “Azure” terminology used elsewhere in the project. No functional impact.Also applies to: 779-779, 842-842
core/changelog.md (1)
1-3: LGTM!The changelog entries are well-structured with clear categorization. The breaking change documentation for
NetworkConfigretry backoff values is particularly helpful for users upgrading.core/providers/vertex/errors.go (1)
10-38: LGTM! Centralized error parsing improves consistency.The cascading error format detection (OpenAI → Vertex array → single Vertex → VertexValidationError) is well-designed. The function correctly uses
providerNamein all error constructors, addressing the prior review feedback.One minor observation: the deeply nested structure (4 levels) could be refactored to early-return patterns for readability, but this is not blocking given the current implementation is correct and localized.
framework/modelcatalog/pricing.go (2)
263-265: Correct fix: use CacheReadInputTokenCost for cached prompt tokens.Using
CacheReadInputTokenCostinstead ofCacheCreationInputTokenCostis semantically correct—cached prompt tokens are being read from cache, not written. This fixes potential over-billing.
152-162: LGTM! Deployment-aware pricing fallback is well-implemented.The fallback logic correctly prioritizes model-based pricing, then falls back to deployment-based pricing when a deployment alias is in use. The debug logging provides good observability.
core/providers/azure/azure.go (4)
83-102: LGTM! Auth header construction correctly differentiates Anthropic vs OpenAI paths.The authentication logic properly handles:
- Anthropic:
x-api-key+anthropic-versionheaders- OpenAI:
api-keyorAuthorization: Bearerwith api-version query paramThe defensive deletion of
api-keyheader when using Bearer token (line 93) prevents accidental header pollution.
360-412: LGTM! ChatCompletion correctly routes Anthropic vs OpenAI requests.The implementation properly:
- Uses
deploymentfor model family detection (per learnings)- Constructs appropriate request bodies via converters
- Routes to correct API paths (
anthropic/v1/messagesvsopenai/deployments/...)- Handles response parsing with pooled Anthropic response objects
448-513: LGTM! Streaming implementations correctly delegate to shared handlers.Both Anthropic and OpenAI streaming paths properly:
- Set appropriate auth headers
- Use shared streaming handlers from respective provider packages
- Apply
postResponseConverterfor consistent deployment metadata
789-800: LGTM! Helper provides safe deployment resolution.The defensive nil checks (lines 790-792, 794) ensure the function is safe to call even if
validateKeyConfigwasn't called first, improving code robustness.core/providers/vertex/vertex.go (7)
299-336: LGTM! Centralized model detection withIsAnthropicModelimproves consistency.The refactoring to use
schemas.IsAnthropicModel(deployment)andschemas.IsMistralModel(deployment)provides consistent model family detection across the codebase. Theanthropic_versionhandling (lines 331-336) correctly injects the version when missing.
440-441: LGTM! Centralized error parsing viaparseVertexError.Replacing inline error parsing with the centralized
parseVertexErrorfunction improves consistency and maintainability across Vertex provider error handling.
443-495: LGTM! Response handling with object pooling is well-implemented.The implementation correctly:
- Uses
AcquireAnthropicMessageResponse/ReleaseAnthropicMessageResponsefor efficient memory management- Converts responses via
ToBifrostChatResponse()- Populates
ExtraFieldscomprehensively- Conditionally sets
ModelDeploymentonly when different fromModelRequested
519-524: LGTM!postResponseConverterensures consistent deployment metadata in streams.The converter correctly adds
ModelDeploymentonly when it differs fromModelRequested, applied consistently to both Anthropic and OpenAI streaming paths.
692-829: LGTM! Responses method properly handles Anthropic vs non-Anthropic paths.The implementation:
- Uses dedicated HTTP flow for Anthropic models with proper OAuth token handling
- Correctly addresses the prior review feedback (error message on line 704)
- Falls back to
ChatCompletionfor non-Anthropic models, withToBifrostResponsesResponse()preserving latency viaExtraFieldscopy
833-943: LGTM! ResponsesStream correctly implements Anthropic streaming with fallback.The implementation:
- Uses
HandleAnthropicResponsesStreamfor Anthropic models with proper OAuth setup- Sets
BifrostContextKeyIsResponsesToChatCompletionFallbackcontext value for non-Anthropic fallback detection- Correctly addresses the prior review feedback (error message on line 860)
1064-1066: LGTM! Embedding method includes deployment metadata consistently.The conditional
ModelDeploymentassignment follows the same pattern as other methods, ensuring consistent metadata across all Vertex provider operations.core/providers/anthropic/anthropic.go (8)
30-56: LGTM: Consistent renaming from "Chat" to "Message".The renaming of the pool and accessor functions improves consistency with Anthropic's API terminology. All references are updated consistently throughout the file.
Also applies to: 89-89
318-318: LGTM: Function renames applied consistently.The usage of renamed converter and pool accessor functions is consistent with the overall refactoring.
Also applies to: 331-332, 369-369
402-402: LGTM: Signature enhancement adds post-processing capability.The addition of the
postResponseConverterparameter provides flexibility for response transformation. The nil-safety checks at lines 564-570 ensure safe handling when no converter is provided.Also applies to: 417-420
454-454: LGTM: Consistent provider naming throughout error paths.Using the
providerNameparameter directly instead of callingGetProviderKey()improves consistency and reduces redundant calls.Also applies to: 456-456, 462-462, 477-477, 556-556, 575-575, 600-603
564-570: LGTM: Proper nil-safety for post-response converter.The defensive nil-checks and warning logs ensure robustness when a converter is provided but returns nil.
638-639: LGTM: Consistent use of renamed pool accessors.
901-904: Existing concern: Missing final chunk emission on unexpected stream termination.This is the same issue previously raised in the past review comments. When the scanner completes without error (
scanner.Err() == nil) but nomessage_stopevent was received (e.g., unexpected EOF), no final chunk with accumulated usage is emitted.
HandleAnthropicChatCompletionStreaminghas a defensiveelseblock (lines 602-607) that sends a final chunk in this scenario, butHandleAnthropicResponsesStreamdoes not.While Anthropic's API should always send proper termination events, adding the defensive
elseblock would improve robustness against unexpected stream terminations.As per the past review discussion, please confirm whether this edge case should be handled similar to the chat completion streaming handler.
685-709: LGTM: ResponsesStream properly delegates to shared handler.The refactoring to use
HandleAnthropicResponsesStreamfollows the same pattern asChatCompletionStreamand reduces code duplication.
…ve anthropic for vertex claude responses api
e0c170f to
a0fc7da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/azure/azure.go (2)
113-116: Anthropic error responses parsed with OpenAI parser.When the Anthropic path returns a non-200 status, errors are parsed using
openai.ParseOpenAIError. Anthropic error responses have a different structure than OpenAI. This may result in incomplete or confusing error messages for Anthropic-related failures.Consider adding Anthropic-specific error parsing:
// Handle error response if resp.StatusCode() != fasthttp.StatusOK { + if schemas.IsAnthropicModel(deployment) { + return nil, deployment, latency, anthropic.ParseAnthropicError(resp, requestType, provider.GetProviderKey(), model) + } return nil, deployment, latency, openai.ParseOpenAIError(resp, requestType, provider.GetProviderKey(), model) }Verify that an
anthropic.ParseAnthropicErrorfunction exists or needs to be created.#!/bin/bash # Check if Anthropic error parsing exists rg -n "ParseAnthropicError|func.*Error.*Anthropic" core/providers/anthropic/ --type=go
701-747: Embedding does not validate Anthropic model unsupported.The
Embeddingfunction routes all requests to OpenAI's embedding endpoint regardless of model type. If an Anthropic deployment is configured, this will send requests to an incorrect endpoint (Anthropic doesn't support embeddings via the same API).Consider adding a guard:
deployment, err := provider.getModelDeployment(key, request.Model) if err != nil { return nil, err } + if schemas.IsAnthropicModel(deployment) { + return nil, providerUtils.NewUnsupportedOperationError(schemas.EmbeddingRequest, provider.GetProviderKey()) + } + // Use centralized converterThis provides a clear error rather than a confusing failure from the Azure API.
🧹 Nitpick comments (6)
core/providers/gemini/errors.go (1)
39-90: Consider consolidating duplicate parsing logic.Both
parseStreamGeminiErrorandparseGeminiErrorfollow identical parsing flows. Since the streaming vs. non-streaming distinction doesn't affect how errors are parsed, consider merging them into a single helper function to reduce duplication and improve maintainability.Example refactor:
// parseGeminiError parses Gemini error responses (both streaming and non-streaming) func parseGeminiError(providerName schemas.ModelProvider, resp *fasthttp.Response) *schemas.BifrostError { body := append([]byte(nil), resp.Body()...) // Try to parse as JSON first var errorResp GeminiGenerationError if err := sonic.Unmarshal(body, &errorResp); err == nil { bifrostErr := &schemas.BifrostError{ IsBifrostError: false, StatusCode: schemas.Ptr(resp.StatusCode()), Error: &schemas.ErrorField{ Code: schemas.Ptr(strconv.Itoa(errorResp.Error.Code)), Message: errorResp.Error.Message, }, } return bifrostErr } var rawResponse map[string]interface{} if err := sonic.Unmarshal(body, &rawResponse); err != nil { return providerUtils.NewBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err, providerName) } return providerUtils.NewBifrostOperationError(fmt.Sprintf("Gemini error (HTTP %d): %v", resp.StatusCode(), rawResponse), fmt.Errorf("HTTP %d", resp.StatusCode()), providerName) } // parseStreamGeminiError is an alias for backward compatibility func parseStreamGeminiError(providerName schemas.ModelProvider, resp *fasthttp.Response) *schemas.BifrostError { return parseGeminiError(providerName, resp) }Alternatively, if you need to maintain separate functions for clarity, at least extract the common parsing logic into a shared helper.
docs/apis/openapi.json (1)
1758-1803: Azure deployment endpoint branding looks consistent; consider harmonizingdeployment-iddescriptionsThe updated summaries/descriptions for the deployment-scoped OpenAI-compatible endpoints look good and match the rest of the OpenAI integration wording.
One minor consistency nit: on
/openai/deployments/{deployment-id}/completionsthedeployment-idparameter description is now"Deployment ID", while the analogous chat/responses/embeddings/audio endpoints still say"Azure deployment ID". You might want to normalize these descriptions one way or the other across all deployment paths for clarity.Also applies to: 1879-1925, 2001-2047, 2113-2155, 2222-2265, 2330-2372
plugins/logging/operations.go (1)
24-41: Retries count now correctly updated for both streaming and non‑streaming logsThe logging changes look sound:
- Initial log entry no longer stores
number_of_retries, which avoids stale values at insert time.updateLogEntryandupdateStreamingLogEntrynow acceptnumberOfRetriesand persist it when non‑zero, fixing the issue where the retries count never advanced.One behavioral nuance: because the field is only written when
numberOfRetries != 0, you cannot reset a previously non‑zero value back to0via this path. If you ever need that, you might later switch this parameter to*int(nil = “don’t touch”, 0 = “reset”), but for the current bugfix this implementation is adequate.Also applies to: 49-60, 74-76, 176-187, 198-200
docs/quickstart/go-sdk/provider-configuration.mdx (1)
274-305: Azure provider doc snippet matches new API; fix minor concurrency snippet typoThe new Azure provider-auth section looks aligned with the code:
- Uses
schemas.Azureas the provider.- Configures
AzureKeyConfigwithEndpoint,Deployments, and optionalAPIVersion, which is what the runtime expects.One unrelated but important doc nit in the same file: in the “Custom Concurrency and Buffer Size” example,
schemas.ConcurrencyAndBufferSizeuses aConcurrencyfield (as seen elsewhere in the codebase), notMaxConcurrency. As written, that snippet won’t compile.You can fix the example like this:
- case schemas.OpenAI: - return &schemas.ProviderConfig{ - NetworkConfig: schemas.DefaultNetworkConfig, - ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ - MaxConcurrency: 100, // Max number of concurrent requests (no of workers) - BufferSize: 500, // Max number of requests in the buffer (queue size) - }, - }, nil + case schemas.OpenAI: + return &schemas.ProviderConfig{ + NetworkConfig: schemas.DefaultNetworkConfig, + ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ + Concurrency: 100, // Max number of concurrent requests (no of workers) + BufferSize: 500, // Max number of requests in the buffer (queue size) + }, + }, nilAlso applies to: 176-198
transports/bifrost-http/handlers/providers.go (2)
228-235: Retry backoff validation is a good safeguard; consider also rejecting negative durationsThe new
validateRetryBackoffhelper and its use in bothaddProviderandupdateProvidercorrectly:
- Enforce per-field bounds using
lib.MinRetryBackoff/lib.MaxRetryBackoff.- Ensure
RetryBackoffInitial <= RetryBackoffMaxwhen both are non‑zero.This will prevent misconfigured providers from setting extreme or inverted backoff windows.
One small hardening opportunity: right now, negative backoff values slip through because the checks only run when the fields are
> 0. Ifschemas.NetworkConfigallows negativeRetryBackoff*values, you may want to explicitly reject< 0as invalid here as well.Also applies to: 414-424, 871-895
528-531:AccessibleByKeysfield is currently unused in listModels responses
ModelResponsenow includes anAccessibleByKeysfield, butlistModelsonly filters the model list by keys and never sets this field, so it will always be omitted (omitempty).If the intent is just to return the filtered models, this is fine and the extra field is harmless. If you eventually want the API to surface which keys can access each model, consider populating
AccessibleByKeysinlistModels(and documenting its behavior), or dropping the field until that behavior is implemented.Also applies to: 563-604, 609-628
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (70)
core/bifrost.go(2 hunks)core/bifrost_test.go(2 hunks)core/changelog.md(1 hunks)core/chatbot_test.go(3 hunks)core/internal/testutil/account.go(2 hunks)core/providers/anthropic/anthropic.go(21 hunks)core/providers/anthropic/anthropic_test.go(1 hunks)core/providers/anthropic/chat.go(1 hunks)core/providers/azure/azure.go(15 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/azure/types.go(1 hunks)core/providers/bedrock/bedrock.go(1 hunks)core/providers/gemini/errors.go(2 hunks)core/providers/gemini/gemini.go(0 hunks)core/providers/openai/chat.go(1 hunks)core/providers/utils/utils.go(0 hunks)core/providers/vertex/errors.go(1 hunks)core/providers/vertex/vertex.go(11 hunks)core/providers/vertex/vertex_test.go(2 hunks)core/schemas/provider.go(2 hunks)core/schemas/utils.go(1 hunks)core/utils.go(1 hunks)core/version(1 hunks)docs/apis/openapi.json(11 hunks)docs/features/keys-management.mdx(1 hunks)docs/features/unified-interface.mdx(1 hunks)docs/integrations/anthropic-sdk.mdx(2 hunks)docs/integrations/genai-sdk.mdx(2 hunks)docs/integrations/langchain-sdk.mdx(2 hunks)docs/integrations/litellm-sdk.mdx(2 hunks)docs/integrations/openai-sdk.mdx(3 hunks)docs/integrations/what-is-an-integration.mdx(1 hunks)docs/quickstart/gateway/multimodal.mdx(1 hunks)docs/quickstart/gateway/provider-configuration.mdx(1 hunks)docs/quickstart/go-sdk/multimodal.mdx(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(1 hunks)framework/changelog.md(1 hunks)framework/modelcatalog/pricing.go(7 hunks)framework/modelcatalog/utils.go(1 hunks)framework/streaming/responses.go(2 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/main.go(8 hunks)plugins/logging/operations.go(5 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)transports/bifrost-http/handlers/providers.go(9 hunks)transports/bifrost-http/integrations/router.go(3 hunks)transports/bifrost-http/lib/config.go(1 hunks)transports/bifrost-http/server/server.go(0 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)ui/README.md(1 hunks)ui/app/workspace/logs/views/logChatMessageView.tsx(1 hunks)ui/app/workspace/logs/views/logDetailsSheet.tsx(2 hunks)ui/lib/constants/logs.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
💤 Files with no reviewable changes (3)
- transports/bifrost-http/server/server.go
- core/providers/utils/utils.go
- core/providers/gemini/gemini.go
✅ Files skipped from review due to trivial changes (2)
- plugins/mocker/version
- plugins/semanticcache/version
🚧 Files skipped from review as they are similar to previous changes (34)
- core/providers/anthropic/chat.go
- plugins/otel/version
- framework/modelcatalog/utils.go
- core/chatbot_test.go
- transports/bifrost-http/integrations/router.go
- docs/integrations/genai-sdk.mdx
- core/utils.go
- plugins/jsonparser/version
- docs/integrations/openai-sdk.mdx
- docs/quickstart/gateway/provider-configuration.mdx
- core/providers/azure/types.go
- core/bifrost_test.go
- framework/changelog.md
- core/version
- docs/integrations/litellm-sdk.mdx
- plugins/telemetry/changelog.md
- core/providers/vertex/errors.go
- plugins/logging/main.go
- plugins/telemetry/version
- core/schemas/utils.go
- plugins/governance/version
- ui/lib/types/schemas.ts
- docs/integrations/what-is-an-integration.mdx
- core/providers/bedrock/bedrock.go
- transports/bifrost-http/lib/config.go
- core/providers/openai/chat.go
- plugins/maxim/changelog.md
- transports/version
- plugins/governance/changelog.md
- core/schemas/provider.go
- framework/version
- plugins/logging/version
- docs/integrations/anthropic-sdk.mdx
- plugins/maxim/version
🧰 Additional context used
🧬 Code graph analysis (6)
core/providers/gemini/errors.go (4)
core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (1)
Ptr(51-53)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(30-30)
ui/app/workspace/logs/views/logDetailsSheet.tsx (1)
ui/components/ui/sheet.tsx (1)
SheetContent(140-140)
framework/streaming/responses.go (2)
core/schemas/bifrost.go (3)
OpenAI(35-35)OpenRouter(49-49)Azure(36-36)core/schemas/utils.go (1)
IsAnthropicModel(1043-1045)
core/providers/azure/azure.go (4)
core/schemas/utils.go (2)
IsAnthropicModel(1043-1045)Ptr(16-18)core/providers/azure/types.go (2)
AzureAnthropicAPIVersionDefault(6-6)AzureAPIVersionDefault(4-4)core/utils.go (1)
Ptr(51-53)core/providers/anthropic/anthropic.go (2)
AcquireAnthropicMessageResponse(45-49)ReleaseAnthropicMessageResponse(52-56)
core/providers/vertex/vertex.go (7)
core/schemas/utils.go (3)
IsAnthropicModel(1043-1045)IsMistralModel(1048-1050)Ptr(16-18)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/providers/anthropic/anthropic.go (1)
HandleAnthropicResponsesStream(712-913)core/providers/utils/utils.go (1)
HandleProviderResponse(343-387)core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-35)core/providers/anthropic/responses.go (1)
ToAnthropicResponsesRequest(284-383)core/utils.go (1)
Ptr(51-53)
core/providers/anthropic/anthropic.go (3)
core/providers/anthropic/types.go (1)
AnthropicMessageResponse(272-281)core/providers/anthropic/chat.go (1)
ToAnthropicChatRequest(386-620)core/providers/utils/utils.go (1)
NewBifrostOperationError(449-460)
🪛 LanguageTool
core/changelog.md
[style] ~3-~3: Consider using a different verb for a more formal wording.
Context: ...en serializing/deserializing from JSON, fixing issues where values were incorrectly in...
(FIX_RESOLVE)
🔇 Additional comments (38)
core/bifrost.go (2)
2025-2025: LGTM! Helpful observability improvement.The debug log for backoff sleep duration aids in monitoring retry behavior at runtime without affecting control flow.
2067-2067: LGTM! Consistent wording.The updated message text aligns with the
attemptsvariable naming and correctly handles singular/plural forms.core/providers/gemini/errors.go (1)
3-11: LGTM! Imports align with new parsing functions.All imports are necessary for the error parsing logic added below.
plugins/jsonparser/changelog.md (1)
1-1: LGTM!Version update note is consistent with the PR's core and framework version bumps.
docs/features/keys-management.mdx (1)
198-198: LGTM!The branding update from "Azure OpenAI" to "Azure" accurately reflects that Azure now supports both OpenAI and Anthropic models per this PR.
ui/lib/constants/logs.ts (1)
45-45: LGTM!Provider label update is consistent with the branding changes across the PR to reflect Azure's support for multiple model families.
ui/README.md (1)
84-84: LGTM!README update correctly reflects the Azure provider's expanded capabilities beyond just OpenAI models.
plugins/semanticcache/changelog.md (1)
1-1: LGTM!Version update note is consistent with the PR's version bumps.
ui/app/workspace/logs/views/logDetailsSheet.tsx (2)
39-39: LGTM!Adding
gap-4improves the vertical spacing between elements in the sheet content for better visual hierarchy.
55-56: LGTM!Formatting improvement for better readability.
plugins/mocker/changelog.md (1)
1-1: LGTM!Version update note matches the consistent version bumps across all plugins.
plugins/otel/changelog.md (1)
1-1: LGTM!Version update note is consistent with other plugin changelogs in this PR.
docs/integrations/langchain-sdk.mdx (1)
221-221: LGTM! Branding update is consistent.The documentation updates from "Azure OpenAI" to "Azure" align with the broader branding changes in this PR.
Also applies to: 269-269
ui/app/workspace/logs/views/logChatMessageView.tsx (1)
29-29: LGTM! UI enhancement for text overflow.The
break-wordsclass prevents text overflow by allowing long words to wrap, which improves the user experience when displaying log content.docs/features/unified-interface.mdx (2)
91-91: LGTM! Provider naming updated for consistency.The Azure OpenAI row has been renamed to "Azure" to align with the broader branding changes in this PR.
95-95: LGTM! Elevenlabs capabilities updated correctly.The Elevenlabs row now accurately reflects the provider's TTS, TTS (stream), and STT capabilities, addressing the previous review feedback.
docs/quickstart/go-sdk/multimodal.mdx (1)
335-337: LGTM! Documentation refactoring improves maintainability.Centralizing multimodal capability information in the Unified Interface page reduces duplication and makes it easier to keep documentation up to date.
transports/changelog.md (1)
1-6: LGTM! Changelog properly documents the PR changes.The changelog entries accurately reflect the main feature addition (Anthropic models in Azure) along with related fixes and improvements.
core/providers/azure/azure_test.go (1)
34-34: Azure deployment configuration correctly includes the Anthropic model.The test configuration using "claude-opus-4-5" as ReasoningModel is properly supported by the test setup, which includes the deployment mapping at core/internal/testutil/account.go:195.
docs/quickstart/gateway/multimodal.mdx (1)
288-290: Update the Unified Interface provider matrix to document Azure Anthropic support.Azure supports Anthropic models (as evidenced by
AzureAnthropicAPIVersionDefaultin the codebase), but this capability is not reflected in the provider matrix. The matrix should clarify that Azure can host Anthropic Claude models via deployment mappings, as documented in the provider configuration guide.framework/streaming/responses.go (1)
686-686: The condition correctly routes Azure Anthropic models to Anthropic-specific streaming.The
IsAnthropicModel()helper function (core/schemas/utils.go:1043–1045) detects Anthropic models by checking for"anthropic."or"claude"substrings. This correctly identifies all Anthropic model variants, including those deployed on Azure (e.g.,claude-3-5-sonnet-20241022). The updated condition properly excludes Azure-hosted Anthropic models from the OpenAI-compatible path, routing them instead to Anthropic-specific streaming handling. The logic is sound and handles Azure's multi-model hosting capability appropriately.plugins/logging/changelog.md (1)
1-2: Changelog entries accurately reflect logging fix and version bumpThe new entries clearly describe the retries logging fix and the corresponding core/framework version updates. No changes needed.
core/internal/testutil/account.go (1)
189-199: Azure test config correctly wires Anthropic/Reasoning deploymentsThe added Azure key with deployments for
"claude-opus-4-5"and"o1"plus the updated comment inAllProviderConfigslook consistent with the new Azure Anthropic support and reasoning tests. ReusingAZURE_API_KEY/AZURE_ENDPOINTin test utilities is reasonable here.Also applies to: 649-652
core/providers/anthropic/anthropic_test.go (1)
24-33: Anthropic test config now exercises vision and reasoning paths explicitlySetting
VisionModeltoclaude-3-7-sonnet-20250219andReasoningModeltoclaude-opus-4-5is a good way to cover Anthropic’s multimodal and reasoning flows in the comprehensive test suite. Looks good.transports/bifrost-http/handlers/providers.go (1)
283-291: Background model refetch now uses a safe, non‑pooled contextThe goroutines that trigger
RefetchModelsForProvideron add/update now usecontext.Background()instead of capturing the request’s*fasthttp.RequestCtx. That avoids using a pooled/recycled request context after the handler returns while still keeping the refetch non‑blocking.Also applies to: 461-470
framework/modelcatalog/pricing.go (5)
93-93: LGTM: Deployment parameter correctly propagated to cost calculation.The addition of
extraFields.ModelDeploymentenables deployment-aware pricing lookups, which is essential for Azure Anthropic models where deployment names differ from model aliases.
110-114: Verify that empty deployment is intentional for cache debug flows.The cache debug cost calculations pass
""for deployment. This is likely correct since cache hits usecacheDebug.ModelUsed(which should be the actual model name), but confirm this won't skip pricing for deployments that require the deployment fallback path.Also applies to: 123-127
152-162: LGTM: Deployment fallback with appropriate logging.The fallback logic correctly attempts pricing lookup using deployment when model lookup fails, and logs appropriately before returning zero cost. This ensures Azure deployments with different model/deployment names are properly priced.
323-341: Improved Bedrock pricing lookup usingIsAnthropicModelhelper.This addresses the previous review concern about overly broad numeric model assumptions. Now explicitly uses
IsAnthropicModel(which checks for "anthropic." or "claude" substrings) to determine when to prepend the "anthropic." prefix, making the logic more precise and less likely to incorrectly modify non-Claude model names.
263-265: Cache read input token cost change is semantically correct.The change from
CacheCreationInputTokenCosttoCacheReadInputTokenCostaligns with Anthropic's pricing model, where cached prompt tokens are billed at the cache read rate (~90% discount, ~$0.30/1M vs. $3/1M base input). Cached prompt tokens represent reads from cache, so usingCacheReadInputTokenCostis correct.core/providers/azure/azure.go (8)
63-71: LGTM: Clean refactoring ofcompleteRequestsignature.Moving deployment resolution out of this function and accepting it as a parameter promotes separation of concerns and ensures consistent deployment handling across all callers.
83-102: LGTM: Proper header and URL construction for Anthropic vs OpenAI.The branching correctly:
- Sets
x-api-keyandanthropic-versionfor Anthropic models- Sets
api-key(or Bearer token) and includesapi-versionquery param for OpenAI models- Constructs appropriate URL paths for each provider type
Based on learnings, using
IsAnthropicModel(deployment)is correct since deployment contains the actual model identifier.
352-412: LGTM: Anthropic detection correctly usesdeploymentinChatCompletion.This addresses the past review comment. The routing now consistently checks
IsAnthropicModel(deployment)for:
- Request body conversion (line 361)
- Path selection (line 377)
- Response parsing (line 399)
The deployment variable contains the actual Azure deployment name with the model identifier.
437-513: LGTM:ChatCompletionStreamproperly branches for Anthropic vs OpenAI.The streaming implementation correctly:
- Resolves deployment upfront using
getModelDeployment- Sets appropriate headers (
x-api-keyvsapi-key/Bearer)- Constructs correct URLs for each provider type
- Uses provider-specific streaming handlers (
HandleAnthropicChatCompletionStreamingvsHandleOpenAIChatCompletionStreaming)- Includes
postResponseConverterto set deployment metadata
524-602: LGTM:Responsescorrectly mirrorsChatCompletionAnthropic handling.Consistent pattern with
ChatCompletion:
- Uses
IsAnthropicModel(deployment)for routing- Converts to Anthropic request format when applicable
- Parses Anthropic response and converts to Bifrost format
- Sets appropriate path ("anthropic/v1/messages" vs OpenAI responses endpoint)
610-690: LGTM:ResponsesStreamproperly implements dual-provider streaming.Mirrors the pattern in
ChatCompletionStreamwith correct:
- Deployment resolution and Anthropic detection
- Header configuration for each provider type
- URL construction
- Streaming handler delegation
The
postRequestConverterfor OpenAI correctly sets the deployment model.
789-800: LGTM: Clean deployment resolution helper.The
getModelDeploymentfunction properly:
- Validates
AzureKeyConfigexists- Looks up deployment from the configured map
- Returns a descriptive configuration error if not found
This centralizes deployment resolution logic for reuse across all methods.
302-312: Anthropic does not support text completion streaming.TextCompletionStreamis intentionally unsupported for Anthropic (returnsUnsupportedOperationError), so Azure's hardcoded OpenAI deployment path in this code is appropriate—it only handles Azure deployments. No cross-provider routing issue exists here.
Merge activity
|
1 participant
Summary
Added support for Anthropic models in Azure, allowing users to use Claude models through their Azure deployments alongside existing OpenAI models.
Changes
anthropicChatResponsePooltoanthropicMessageResponsePoolfor consistencyToAnthropicChatCompletionRequesttoToAnthropicChatRequestfor clarityType of change
Affected areas
How to test
Breaking changes
Related issues
N/A
Security considerations
No new security implications.
Checklist
docs/contributing/README.mdand followed the guidelines