feat: Add OpenAI integration in HTTP transport #63
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe changes introduce a new OpenAI-compatible HTTP integration for the Bifrost system, including route registration, request/response conversion logic, and support for OpenAI's chat completion API with vision features. The GenAI integration was refactored to use the new generic routing framework. The main server now initializes and registers the OpenAI router alongside existing integrations, leveraging the new extensible routing infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenAIRouter
participant BifrostClient
Client->>OpenAIRouter: POST /openai/v1/chat/completions (OpenAI request)
OpenAIRouter->>OpenAIRouter: Parse & validate request
OpenAIRouter->>OpenAIRouter: Convert to BifrostRequest
OpenAIRouter->>BifrostClient: ChatCompletionRequest(BifrostRequest)
BifrostClient-->>OpenAIRouter: BifrostResponse
OpenAIRouter->>OpenAIRouter: Convert to OpenAIChatResponse
OpenAIRouter-->>Client: HTTP 200 + JSON (OpenAIChatResponse)
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
transports/bifrost-http/integrations/openai/router.go(1 hunks)transports/bifrost-http/integrations/openai/types.go(1 hunks)transports/bifrost-http/main.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
transports/bifrost-http/main.go (3)
transports/bifrost-http/integrations/utils.go (1)
ExtensionRouter(5-7)transports/bifrost-http/integrations/genai/router.go (1)
NewGenAIRouter(18-20)transports/bifrost-http/integrations/openai/router.go (1)
NewOpenAIRouter(35-37)
🔇 Additional comments (1)
transports/bifrost-http/main.go (1)
180-183: OpenAI router successfully wired in – nothing blocking
Nice, the new router is appended in the sameextensionsslice as the existing GenAI router, so registration order stays deterministic and no paths collide with the legacy/v1/*routes.
Pratham-Mishra04
force-pushed
the
06-09-feat_openai_integration_added_to_http_transport
branch
from
b015950 to
111399d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
transports/bifrost-http/integrations/openai/router.go (3)
46-58:⚠️ Potential issueMissing validation for empty
messagesarrayOpenAI requires at least one message; forwarding an empty slice will explode deeper in the stack. Add the gate here, similar to what exists in
main.go.+ if len(req.Messages) == 0 { + ctx.SetStatusCode(fasthttp.StatusBadRequest) + ctx.SetContentType("application/json") + ctx.SetBodyString(`{"error":"messages array is required"}`) + return + }
47-52: 🧹 Nitpick (assertive)
Content-Typeheader missing on 400 repliesClients expecting JSON will choke unless the header is set.
- ctx.SetStatusCode(fasthttp.StatusBadRequest) + ctx.SetStatusCode(fasthttp.StatusBadRequest) + ctx.SetContentType("application/json")
64-70: 🛠️ Refactor suggestionReturn correct HTTP status & set Content-Type on errors
Every error is currently surfaced as 500. Use the same 4xx/5xx branching adopted in
main.goand mark the body as JSON.- if err != nil { - ctx.SetStatusCode(fasthttp.StatusInternalServerError) + if err != nil { + if err.IsBifrostError { + ctx.SetStatusCode(fasthttp.StatusInternalServerError) + } else { + ctx.SetStatusCode(fasthttp.StatusBadRequest) + } + ctx.SetContentType("application/json")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
transports/bifrost-http/integrations/openai/router.go(1 hunks)transports/bifrost-http/integrations/openai/types.go(1 hunks)transports/bifrost-http/main.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
transports/bifrost-http/integrations/openai/router.go (2)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.907Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and handles the conversion internally, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.907Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and always returns a pointer to a valid context. It starts with context.Background() and only adds values to it, so nil checks are not needed when calling this function.
🧬 Code Graph Analysis (2)
transports/bifrost-http/main.go (3)
transports/bifrost-http/integrations/utils.go (1)
ExtensionRouter(5-7)transports/bifrost-http/integrations/genai/router.go (1)
NewGenAIRouter(18-20)transports/bifrost-http/integrations/openai/router.go (1)
NewOpenAIRouter(35-37)
transports/bifrost-http/integrations/openai/types.go (3)
core/utils.go (1)
Ptr(3-5)core/schemas/bifrost.go (25)
ToolChoiceTypeFunction(126-126)ToolCall(266-270)FunctionCall(260-263)Function(101-105)Tool(108-112)ToolChoice(137-140)LLMUsage(197-203)ImageContent(174-179)BifrostRequest(54-64)OpenAI(34-34)RequestInput(46-49)BifrostMessage(143-152)ModelParameters(75-89)ModelChatMessageRole(20-20)ModelChatMessageRoleUser(24-24)UserMessage(155-157)ModelChatMessageRoleTool(27-27)ToolMessage(160-163)AssistantMessage(166-171)ToolChoiceType(116-116)ToolChoiceTypeNone(120-120)ToolChoiceTypeAuto(122-122)ToolChoiceTypeRequired(128-128)ToolChoiceFunction(132-134)BifrostResponse(184-194)core/schemas/provider.go (1)
Provider(142-149)
Pratham-Mishra04
force-pushed
the
06-09-feat_openai_integration_added_to_http_transport
branch
from
111399d to
7ee161c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
transports/bifrost-http/main.go (1)
180-183: LGTM: Proper router registration.The OpenAI router is correctly registered alongside the GenAI router using the extensions framework.
The package-level documentation (lines 4-6) needs to be updated to include the newly exposed
/openai/v1/chat/completionsroute alongside the existing routes, as mentioned in previous reviews.transports/bifrost-http/integrations/openai/types.go (1)
185-190:⚠️ Potential issueCompilation failure – cannot
rangeover anint
requestVal.NumField()returns anint, not a slice.
for i := range requestVal.NumField()will not compile.-for i := range requestVal.NumField() { +for i := 0; i < requestVal.NumField(); i++ {Without this fix
go vet,go test, andgo buildall fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
transports/bifrost-http/integrations/genai/router.go(1 hunks)transports/bifrost-http/integrations/genai/types.go(2 hunks)transports/bifrost-http/integrations/openai/router.go(1 hunks)transports/bifrost-http/integrations/openai/types.go(1 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)transports/bifrost-http/main.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
transports/bifrost-http/integrations/openai/router.go (2)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.907Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and handles the conversion internally, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.907Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and always returns a pointer to a valid context. It starts with context.Background() and only adds values to it, so nil checks are not needed when calling this function.
🧬 Code Graph Analysis (3)
transports/bifrost-http/main.go (3)
transports/bifrost-http/integrations/utils.go (1)
ExtensionRouter(15-17)transports/bifrost-http/integrations/genai/router.go (1)
NewGenAIRouter(19-41)transports/bifrost-http/integrations/openai/router.go (1)
NewOpenAIRouter(16-37)
transports/bifrost-http/integrations/openai/router.go (3)
transports/bifrost-http/integrations/utils.go (4)
GenericRouter(52-55)RouteConfig(39-47)RequestConverter(21-21)NewGenericRouter(59-64)core/bifrost.go (2)
Bifrost(39-51)RequestType(20-20)core/schemas/bifrost.go (2)
BifrostRequest(54-64)BifrostResponse(184-194)
transports/bifrost-http/integrations/utils.go (3)
core/schemas/bifrost.go (2)
BifrostRequest(54-64)BifrostResponse(184-194)core/bifrost.go (3)
RequestType(20-20)Bifrost(39-51)ChatCompletionRequest(24-24)transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(43-73)
🔇 Additional comments (7)
transports/bifrost-http/main.go (1)
33-33: LGTM: Clean import addition.The import for the OpenAI integration package is properly added.
transports/bifrost-http/integrations/genai/types.go (2)
15-15: LGTM: Model field addition supports the new routing pattern.The explicit
Modelfield allows the router's pre-callback to set the model from the URL path parameter, which is cleaner than passing it as a separate parameter.
24-27: LGTM: Method signature simplified correctly.The removal of the
modelStrparameter and use ofr.Modelfield aligns with the new generic router framework where the model is set via theextractAndSetModelFromURLpre-callback.transports/bifrost-http/integrations/openai/router.go (1)
9-13: LGTM: Clean router struct definition.The
OpenAIRouterproperly embeds theGenericRouterfollowing the same pattern as the GenAI integration.transports/bifrost-http/integrations/genai/router.go (3)
14-16: LGTM: Successful refactoring to generic router framework.The
GenAIRouternow properly embeds theGenericRouter, eliminating code duplication and leveraging the shared routing infrastructure.Also applies to: 38-41
20-36: LGTM: Clean declarative route configuration.The route configuration clearly defines:
- Path with model parameter:
/genai/v1beta/models/{model}- Request/response conversion functions
- Pre-callback for model extraction
This approach is much cleaner than the previous imperative handler method.
43-68: LGTM: Robust model extraction logic.The
extractAndSetModelFromURLfunction properly:
- Validates the model parameter exists
- Cleans the model string (removes
:generateContentsuffix and trailing colons)- Adds the required
google/prefix- Type-checks the request before setting the model field
- Returns appropriate error messages
Pratham-Mishra04
force-pushed
the
06-09-feat_openai_integration_added_to_http_transport
branch
from
7ee161c to
4ad600d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
transports/bifrost-http/integrations/utils.go (1)
96-103: Re-use of a single request object – identical to previous review feedbackThe same concurrency bug described above was flagged in an earlier review and is still present.
Please address it to avoid panics / corrupted requests.transports/bifrost-http/integrations/openai/types.go (1)
185-190:⚠️ Potential issueCompilation fails – cannot
rangeover anint
reflect.Value.NumField()returns anint, not a slice.
The current loop does not compile.- for i := range requestVal.NumField() { + for i := 0; i < requestVal.NumField(); i++ {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
transports/bifrost-http/integrations/genai/router.go(1 hunks)transports/bifrost-http/integrations/genai/types.go(2 hunks)transports/bifrost-http/integrations/openai/router.go(1 hunks)transports/bifrost-http/integrations/openai/types.go(1 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)transports/bifrost-http/main.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
transports/bifrost-http/integrations/openai/router.go (3)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.907Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and handles the conversion internally, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.907Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and always returns a pointer to a valid context. It starts with context.Background() and only adds values to it, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:0-0
Timestamp: 2025-06-09T14:03:34.187Z
Learning: In the Bifrost HTTP transport layer (transports/bifrost-http/integrations/), request validation like checking for empty messages should be handled by the provider rather than at the transport layer. The transport layer should forward requests to Bifrost core/providers for validation.
🧬 Code Graph Analysis (1)
transports/bifrost-http/integrations/genai/types.go (2)
core/schemas/bifrost.go (2)
BifrostRequest(54-64)Vertex(39-39)core/schemas/provider.go (1)
Provider(142-149)
🔇 Additional comments (7)
transports/bifrost-http/main.go (2)
16-29: Excellent documentation enhancement!The comprehensive documentation clearly explains the integration support pattern and provides concrete examples for OpenAI, GenAI, and Anthropic endpoints. This addresses the previous feedback about updating package documentation to reflect the full API surface area.
194-197: Clean integration following established patterns.The OpenAI router registration alongside GenAI demonstrates consistent use of the generic router framework. Both integrations follow the same
ExtensionRouterinterface pattern.transports/bifrost-http/integrations/openai/router.go (1)
16-37: Well-structured router following established patterns.The OpenAI router implementation effectively leverages the generic router framework with:
- Correct endpoint path
/openai/v1/chat/completions- Proper request type and conversion logic
- Response transformation function
This design avoids code duplication and maintains consistency with the GenAI integration approach.
transports/bifrost-http/integrations/genai/types.go (2)
15-15: Good addition of model field for cleaner architecture.Adding the
Modelfield directly to the request struct simplifies the conversion interface and aligns well with the router's URL-based model extraction pattern.
24-27: Clean refactoring of conversion method.The removal of the
modelStrparameter and use of the embeddedr.Modelfield creates a more cohesive API where the request object contains all necessary data for conversion.transports/bifrost-http/integrations/genai/router.go (2)
20-41: Excellent refactoring to generic router framework.The migration to use
RouteConfigwith defined converters and callbacks demonstrates good architectural consistency. ThePreCallbackfor model extraction ensures the model is properly set before request conversion.
43-68: Robust model extraction with proper error handling.The
extractAndSetModelFromURLfunction correctly:
- Validates model parameter presence
- Handles GenAI-specific URL suffixes (
:generateContent)- Adds required
google/prefix for Bifrost compatibility- Provides appropriate error messages for invalid cases
Pratham-Mishra04
force-pushed
the
06-09-feat_openai_integration_added_to_http_transport
branch
2 times, most recently
from
fbd3a73 to
e0e01b8
Compare
Pratham-Mishra04
force-pushed
the
06-09-feat_openai_integration_added_to_http_transport
branch
from
e0e01b8 to
4cf3b95
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
transports/bifrost-http/integrations/utils.go (2)
40-46: SharedRequestTypeinstance -> data-race persists
The field is stillinterface{}and a single value is re-used for every request. This reproduces the request-bleed / data-race problem already reported in earlier reviews.
98-110:req := config.RequestTypemutates the same object for all calls
Because the previous issue is unresolved,json.Unmarshalwill mutate the shared struct concurrently, corrupting state under load. A per-request factory (NewRequest func() interface{}) is still required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
transports/bifrost-http/integrations/genai/router.go(1 hunks)transports/bifrost-http/integrations/genai/types.go(2 hunks)transports/bifrost-http/integrations/openai/router.go(1 hunks)transports/bifrost-http/integrations/openai/types.go(1 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)transports/bifrost-http/main.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
transports/bifrost-http/integrations/openai/router.go (3)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and handles the conversion internally, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and always returns a pointer to a valid context. It starts with context.Background() and only adds values to it, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:0-0
Timestamp: 2025-06-09T14:03:34.227Z
Learning: In the Bifrost HTTP transport layer (transports/bifrost-http/integrations/), request validation like checking for empty messages should be handled by the provider rather than at the transport layer. The transport layer should forward requests to Bifrost core/providers for validation.
🧬 Code Graph Analysis (2)
transports/bifrost-http/integrations/genai/types.go (2)
core/schemas/bifrost.go (2)
BifrostRequest(54-64)Vertex(39-39)core/schemas/provider.go (1)
Provider(142-149)
transports/bifrost-http/main.go (3)
transports/bifrost-http/integrations/utils.go (1)
ExtensionRouter(15-17)transports/bifrost-http/integrations/genai/router.go (1)
NewGenAIRouter(19-41)transports/bifrost-http/integrations/openai/router.go (1)
NewOpenAIRouter(16-37)
🔇 Additional comments (3)
transports/bifrost-http/integrations/openai/types.go (1)
278-289: Assistant message may duplicate legacy + modern tool-callsIf both
tool_callsandfunction_callare present (some SDKs send both),
the legacy branch is skipped, which is correct.
However, iftool_callsis absent butfunction_callcontains a non-function
tool (future spec), we will silently coerce it intofunction.Please double-check the spec and add a guard / type switch so that only a real
function call is mapped here.transports/bifrost-http/main.go (1)
194-197: Good: router slice kept alphabeticalOpenAI router addition follows existing order and keeps the slice tidy.
transports/bifrost-http/integrations/genai/types.go (1)
24-31: Model field is optional but required downstream
GeminiChatRequest.ConvertToBifrostRequestnow relies onr.Model. If the
URL pre-callback fails to set it (e.g., malformed route), the provider will get
an empty model string and likely 4xx/5xx later.Consider an explicit
if r.Model == "" { return nil /* or error */ }early in
the converter, letting GenericRouter translate that into 400.
Pratham-Mishra04
force-pushed
the
06-09-feat_openai_integration_added_to_http_transport
branch
from
4cf3b95 to
72f5791
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
transports/bifrost-http/integrations/genai/types.go (1)
65-68: 🧹 Nitpick (assertive)Redundant nil-guard after guaranteed allocation
ChatCompletionInputis initialised at line 28, so the subsequent nil check is dead code.
Simplify to avoid misleading future readers:-if bifrostReq.Input.ChatCompletionInput != nil { - *bifrostReq.Input.ChatCompletionInput = append(*bifrostReq.Input.ChatCompletionInput, bifrostMsg) -} +*bifrostReq.Input.ChatCompletionInput = append(*bifrostReq.Input.ChatCompletionInput, bifrostMsg)
♻️ Duplicate comments (2)
transports/bifrost-http/main.go (1)
16-30: Doc-block still duplicates endpoint listsThe new “Integration Support” section (lines 16-30) lists provider-specific paths, yet lines 4-6 above still enumerate only
/v1/*routes. This resurrects the same duplication/contradiction flagged in the previous review.Please consolidate the two lists into a single, ordered set so autogenerated docs and readers see an accurate, non-redundant surface area.
transports/bifrost-http/integrations/openai/types.go (1)
92-117: 🛠️ Refactor suggestion
⚠️ Potential issueSilently drops all but the last image in multi-image prompts
Every time an
image_urlpart is encountered,imageContentis overwritten, so only the URL from the final part survives. The vision spec explicitly allows multiple images; at the moment they are lost without warning.- var imageContent *schemas.ImageContent + var imageList []schemas.ImageContent ... - imageContent = &schemas.ImageContent{ - URL: url, - } + img := schemas.ImageContent{URL: url} + if detail, detailExists := imageURL["detail"].(string); detailExists { + img.Detail = &detail + } + imageList = append(imageList, img) ... - return textContent, imageContent + switch len(imageList) { + case 0: + return textContent, nil + case 1: + return textContent, &imageList[0] + default: + return nil, nil // or return an error stating multi-image not yet supported + }Either collect all images and propagate a slice downstream, or reject multi-image requests with an explicit error to avoid silent data loss.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
transports/bifrost-http/integrations/genai/router.go(1 hunks)transports/bifrost-http/integrations/genai/types.go(2 hunks)transports/bifrost-http/integrations/openai/router.go(1 hunks)transports/bifrost-http/integrations/openai/types.go(1 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)transports/bifrost-http/main.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
transports/bifrost-http/integrations/openai/router.go (3)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and handles the conversion internally, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and always returns a pointer to a valid context. It starts with context.Background() and only adds values to it, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:0-0
Timestamp: 2025-06-09T14:03:34.227Z
Learning: In the Bifrost HTTP transport layer (transports/bifrost-http/integrations/), request validation like checking for empty messages should be handled by the provider rather than at the transport layer. The transport layer should forward requests to Bifrost core/providers for validation.
🧬 Code Graph Analysis (2)
transports/bifrost-http/main.go (3)
transports/bifrost-http/integrations/utils.go (1)
ExtensionRouter(15-17)transports/bifrost-http/integrations/genai/router.go (1)
NewGenAIRouter(19-41)transports/bifrost-http/integrations/openai/router.go (1)
NewOpenAIRouter(16-37)
transports/bifrost-http/integrations/genai/types.go (2)
core/schemas/bifrost.go (2)
BifrostRequest(54-64)Vertex(39-39)core/schemas/provider.go (1)
Provider(142-149)
🔇 Additional comments (1)
transports/bifrost-http/integrations/openai/types.go (1)
139-141: 🧹 Nitpick (assertive)Avoid taking the address of a composite slice literal
&[]schemas.BifrostMessage{}allocates an unnecessary pointer; a zero-value slice is alreadynil, and you can append to it directly.- Input: schemas.RequestInput{ - ChatCompletionInput: &[]schemas.BifrostMessage{}, - }, + Input: schemas.RequestInput{}, ... // later - *bifrostReq.Input.ChatCompletionInput = append(...) + bifrostReq.Input.ChatCompletionInput = + append(bifrostReq.Input.ChatCompletionInput, bifrostMsg)This removes one level of indirection and simplifies subsequent code.
⛔ Skipped due to learnings
Learnt from: Pratham-Mishra04 PR: maximhq/bifrost#54 File: core/schemas/bifrost.go:46-49 Timestamp: 2025-06-04T09:22:18.123Z Learning: In core/schemas/bifrost.go, the RequestInput struct uses ChatCompletionInput *[]BifrostMessage (pointer-to-slice) rather than []BifrostMessage to properly represent union type semantics. For text completion requests, ChatCompletionInput should be nil to indicate "no chat payload at all", while for chat completion requests it should be non-nil (even if empty slice). This distinguishes between different request types rather than just empty vs non-empty chat messages.
Pratham-Mishra04
force-pushed
the
06-09-feat_openai_integration_added_to_http_transport
branch
from
72f5791 to
d68ae37
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Race Condition in `GenericRouter` Causes Data Corruption
The GenericRouter has a race condition where the config.RequestType instance is reused across concurrent HTTP requests. This causes multiple goroutines to unmarshal JSON into the same shared object, leading to data corruption, unpredictable behavior, and data leakage. A new instance of the request type must be created for each request.
transports/bifrost-http/integrations/utils.go#L95-L110
bifrost/transports/bifrost-http/integrations/utils.go
Lines 95 to 110 in d68ae37
BugBot free trial expires on June 17, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
transports/bifrost-http/main.go (1)
47-47: OpenAI integration successfully integrated into the HTTP transport.The OpenAI router is properly imported and registered alongside GenAI using the new generic routing infrastructure. The implementation follows the established pattern.
Also applies to: 194-197
transports/bifrost-http/integrations/openai/router.go (1)
16-37: Router configuration follows the established pattern correctly.The route configuration properly defines the OpenAI endpoint with appropriate converters. The implementation leverages the generic router effectively.
transports/bifrost-http/integrations/genai/router.go (2)
19-41: Well-structured route configuration with appropriate callbacks.The route configuration properly uses the generic framework with a pre-request callback for model extraction, demonstrating good use of the extensibility features.
43-71: Model extraction logic correctly handles URL parameters.The
extractAndSetModelFromURLfunction properly extracts, normalizes, and sets the model parameter with appropriate error handling.transports/bifrost-http/integrations/utils.go (3)
101-110: Appropriate handling of request body parsing for different HTTP methods.The code correctly skips body parsing for GET and DELETE methods, avoiding unnecessary JSON unmarshal errors.
183-187: Fixed: Marshal errors are now properly handled.The code now checks for JSON marshaling errors and sends an appropriate error response instead of silently ignoring failures.
42-43:⚠️ Potential issueCritical: Documentation states "Factory function" but field type doesn't match.
The comment describes
RequestTypeas a "Factory function to create request instance", but the field is declared asinterface{}rather than a function type. This mismatch between documentation and implementation is confusing and the current implementation causes thread safety issues.transports/bifrost-http/integrations/openai/types.go (1)
80-127: Multi-image payloads still silently drop all but the last image
The function keeps overwritingimageContent, so only the finalimage_urlsurvives. This limitation was highlighted (and acknowledged) in earlier PR rounds and is still present.I’m not re-opening the architectural discussion, just linking the prior finding for completeness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
transports/bifrost-http/integrations/genai/router.go(1 hunks)transports/bifrost-http/integrations/genai/types.go(2 hunks)transports/bifrost-http/integrations/openai/router.go(1 hunks)transports/bifrost-http/integrations/openai/types.go(1 hunks)transports/bifrost-http/integrations/utils.go(1 hunks)transports/bifrost-http/main.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
transports/bifrost-http/integrations/openai/router.go (5)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and handles the conversion internally, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and always returns a pointer to a valid context. It starts with context.Background() and only adds values to it, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:0-0
Timestamp: 2025-06-09T14:03:34.227Z
Learning: In the Bifrost HTTP transport layer (transports/bifrost-http/integrations/), request validation like checking for empty messages should be handled by the provider rather than at the transport layer. The transport layer should forward requests to Bifrost core/providers for validation.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#67
File: transports/bifrost-http/integrations/anthropic/router.go:26-34
Timestamp: 2025-06-10T11:19:29.578Z
Learning: The Generic router in transports/bifrost-http/integrations/utils.go already handles nil pointers from RequestConverter functions. When a RequestConverter returns a nil *schemas.BifrostRequest, the Generic router automatically returns an HTTP 400 error, making additional nil checks in individual router implementations redundant.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#67
File: transports/bifrost-http/integrations/anthropic/router.go:26-34
Timestamp: 2025-06-10T11:19:29.578Z
Learning: The Generic router in transports/bifrost-http/integrations/utils.go already handles nil pointers from RequestConverter functions. When a RequestConverter returns a nil *schemas.BifrostRequest, the Generic router automatically returns an HTTP 400 error with "Invalid request" message, making additional nil checks in individual router implementations redundant.
transports/bifrost-http/integrations/openai/types.go (4)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/types.go:89-119
Timestamp: 2025-06-10T11:00:02.852Z
Learning: In the Bifrost OpenAI integration (transports/bifrost-http/integrations/openai/types.go), the convertOpenAIContent function currently only handles the last image URL when multiple images are present in a content array. The user Pratham-Mishra04 has acknowledged this limitation and indicated it's part of a larger architectural issue that will be addressed comprehensively later, rather than with piecemeal fixes.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#64
File: transports/bifrost-http/integrations/genai/types.go:273-313
Timestamp: 2025-06-09T16:35:26.914Z
Learning: In convertGenerationConfigToParams method in transports/bifrost-http/integrations/genai/types.go, pre-allocating the ExtraParams map is preferred over lazy allocation because the method has multiple potential ExtraParams assignments, making the computational overhead of conditional checks exceed the memory savings of an empty map.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/types.go:223-231
Timestamp: 2025-06-10T11:06:06.651Z
Learning: The OpenAI `name` field on messages cannot be preserved when converting to Bifrost format because the `schemas.BifrostMessage` struct in bifrost/core does not support a Name field. This is a known limitation of the Bifrost core schema design.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#64
File: transports/bifrost-http/integrations/genai/types.go:470-515
Timestamp: 2025-06-10T11:06:57.681Z
Learning: Bifrost currently only supports images and does not support non-image blob types like audio, PDF, or other binary data formats.
🧬 Code Graph Analysis (2)
transports/bifrost-http/main.go (3)
transports/bifrost-http/integrations/utils.go (1)
ExtensionRouter(15-17)transports/bifrost-http/integrations/genai/router.go (1)
NewGenAIRouter(19-41)transports/bifrost-http/integrations/openai/router.go (1)
NewOpenAIRouter(16-37)
transports/bifrost-http/integrations/genai/types.go (2)
core/schemas/bifrost.go (2)
BifrostRequest(54-64)Vertex(39-39)core/schemas/provider.go (1)
Provider(142-149)
🔇 Additional comments (11)
transports/bifrost-http/main.go (1)
16-30: LGTM! Clear documentation of integration support.The added documentation provides excellent clarity on Bifrost's multi-provider integration support, including the URL pattern and specific examples for each provider.
transports/bifrost-http/integrations/openai/router.go (1)
9-13: LGTM! Well-structured router implementation.The router properly embeds
GenericRouterand includes clear documentation about its capabilities.transports/bifrost-http/integrations/genai/types.go (2)
15-15: Model field addition enables cleaner request handling.Adding the
Modelfield directly to the struct eliminates the need for parameter passing and aligns well with the generic router's pre-callback pattern.
24-27: Simplified method signature improves encapsulation.The removal of the
modelStrparameter and use of the struct'sModelfield makes the conversion method self-contained and cleaner.transports/bifrost-http/integrations/genai/router.go (1)
14-16: Successful refactoring to use generic router infrastructure.The GenAIRouter now properly embeds
GenericRouter, eliminating code duplication and following the new integration pattern.transports/bifrost-http/integrations/utils.go (6)
13-17: Clean interface definition for integration routers.The
ExtensionRouterinterface provides a clear contract for all integration routers to implement.
19-36: Well-documented type definitions for the routing framework.The converter and callback types are clearly defined with comprehensive documentation explaining their purpose and usage.
52-64: Well-designed generic router struct with clear documentation.The
GenericRouterprovides a solid foundation for all integration routers with proper encapsulation of common functionality.
66-84: Comprehensive route registration with support for multiple HTTP methods.The implementation properly handles different HTTP methods and provides a sensible default.
123-131: Proper validation of converted requests.The handler appropriately checks for nil returns from the converter and validates required fields like the model parameter.
137-142: Improved error status code handling.The code now properly uses the status code from BifrostError when available, providing more accurate HTTP status codes.
|
@Pratham-Mishra04 did you check Race condition comment by cursor? |
|
yes yes, fixed in next pr |
Merge activity
|
akshaydeo
deleted the
06-09-feat_openai_integration_added_to_http_transport
branch
Add OpenAI API compatibility layer to Bifrost HTTP transport
This PR adds an OpenAI-compatible API interface to the Bifrost HTTP transport, allowing applications designed for OpenAI's API to work with Bifrost without modification.
Key features:
/openai/v1/chat/completionsendpointThe implementation includes:
This enables seamless integration with existing OpenAI-compatible clients and tools while leveraging Bifrost's capabilities.