feat: list models with all configured keys for provider#700
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors ListModels to accept multiple keys and perform concurrent per-key requests. Adds Bifrost.getAllSupportedKeys for key aggregation, utilities to orchestrate and merge multi-key responses, updates provider implementations to per-key helpers and multi-key flows, and extends model schemas with token limit fields. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Bifrost
participant getAllKeys as getAllSupportedKeys
participant Provider
participant MultiHandler as handleMultipleListModelsRequests
participant PerKey
Client->>Bifrost: ListModelsRequest(provider)
Bifrost->>getAllKeys: collect keys (ctx, provider)
getAllKeys-->>Bifrost: []Key
Bifrost->>Provider: ListModels(ctx, []Key, request)
alt provider uses per-key orchestration
Provider->>MultiHandler: orchestrate keys
par per-key requests
MultiHandler->>PerKey: key[0]
MultiHandler->>PerKey: key[1]
MultiHandler->>PerKey: key[N]
end
PerKey-->>MultiHandler: response / error + latency
MultiHandler->>MultiHandler: extract successes, aggregate & dedupe
MultiHandler-->>Provider: aggregated response
else provider forwards to OpenAI-style handler
Provider->>Provider: handleOpenAIListModelsRequest(keys)
end
Provider-->>Bifrost: BifrostListModelsResponse
Bifrost-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TejasGhatte
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
9a992c3 to
c931eed
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: 3
🧹 Nitpick comments (24)
core/schemas/providers/cohere/models.go (1)
14-21: Consider validating model.Name for robust ID construction.The Model ID is constructed by concatenating
providerKey + "/" + model.Name. Ifmodel.Nameis empty or contains special characters (e.g., "/"), this could result in malformed IDs or downstream parsing issues.Apply this diff to add basic validation:
for _, model := range response.Models { + if model.Name == "" { + continue + } bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ ID: string(providerKey) + "/" + model.Name,core/schemas/providers/vertex/models.go (1)
10-21: Inefficient slice initialization pattern.The code initializes
Datawith a pre-sized capacity on Line 11, then usesappendin Line 15. Since the capacity is already set, directly indexing would be more efficient and clearer.Apply this diff to use direct indexing:
- bifrostResponse := &schemas.BifrostListModelsResponse{ - Data: make([]schemas.Model, 0, len(response.Models)), - } - - for _, model := range response.Models { - bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ + bifrostResponse := &schemas.BifrostListModelsResponse{ + Data: make([]schemas.Model, len(response.Models)), + } + + for i, model := range response.Models { + bifrostResponse.Data[i] = schemas.Model{ ID: string(schemas.Vertex) + "/" + model.Name, Name: schemas.Ptr(model.DisplayName), Description: schemas.Ptr(model.Description), Created: schemas.Ptr(model.VersionCreateTime.Unix()), - }) + } }core/bifrost.go (2)
254-266: Enrich ListModels error with provider/request metadata.When key collection fails, include ExtraFields for better diagnostics.
- return nil, &schemas.BifrostError{ - IsBifrostError: false, - Error: &schemas.ErrorField{ - Message: "failed to get all supported keys", - Error: err, - }, - } + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{ + Message: "failed to get all supported keys", + Error: err, + }, + ExtraFields: schemas.BifrostErrorExtraFields{ + Provider: req.Provider, + RequestType: schemas.ListModelsRequest, + }, + }
2155-2189: Keyless providers: avoid hard error when no keys are configured.Current logic errors on zero keys; providers like Ollama may not need keys. Allow a sentinel key when the base provider doesn’t require a key.
keys, err := bifrost.account.GetKeysForProvider(ctx, providerKey) if err != nil { return nil, err } - if len(keys) == 0 { - return nil, fmt.Errorf("no keys found for provider: %v", providerKey) - } + if len(keys) == 0 { + // If the base provider doesn't require a key, proceed with a sentinel key + if !providerRequiresKey(baseProviderType) { + return []schemas.Key{{ID: "default", Value: ""}}, nil + } + return nil, fmt.Errorf("no keys found for provider: %v", providerKey) + }Please confirm behavior with a keyless provider (e.g., GET /v1/models?provider=ollama) when no keys exist. If it fails with “no keys found,” this change is required.
core/providers/mistral.go (2)
80-86: Release fasthttp request/response per iteration; don’t defer inside the loop.Defers in a loop hold resources until function exit. Release each iteration to reduce memory pressure.
- for _, key := range keys { - // Create request - req := fasthttp.AcquireRequest() - resp := fasthttp.AcquireResponse() - defer fasthttp.ReleaseRequest(req) - defer fasthttp.ReleaseResponse(resp) - ... - } + for _, key := range keys { + func() { + req := fasthttp.AcquireRequest() + resp := fasthttp.AcquireResponse() + defer fasthttp.ReleaseRequest(req) + defer fasthttp.ReleaseResponse(resp) + ... + }() + }
96-101: Use logger formatting instead of fmt.Sprintf in Debug.This avoids extra allocations and matches existing logger usage elsewhere.
- provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", key.ID, bifrostErr.Error.Message)) + provider.logger.Debug("failed to list models with key %s: %s", key.ID, bifrostErr.Error.Message) - provider.logger.Debug(fmt.Sprintf("error from %s provider for key %s", providerName, key.ID)) + provider.logger.Debug("error from %s provider for key %s", providerName, key.ID)Also applies to: 115-118
core/providers/openrouter.go (3)
74-80: Release fasthttp request/response per iteration; don’t defer inside the loop.Mirror the per-iteration release pattern to avoid accumulation.
- for _, key := range keys { - req := fasthttp.AcquireRequest() - resp := fasthttp.AcquireResponse() - defer fasthttp.ReleaseRequest(req) - defer fasthttp.ReleaseResponse(resp) + for _, key := range keys { + func() { + req := fasthttp.AcquireRequest() + resp := fasthttp.AcquireResponse() + defer fasthttp.ReleaseRequest(req) + defer fasthttp.ReleaseResponse(resp) ... - } + }() + }
99-101: Avoid logging full response bodies in Debug.Large/structured bodies can flood logs. Log status and a truncated snippet instead.
- provider.logger.Debug(fmt.Sprintf("error from %s provider: %s", schemas.OpenRouter, string(resp.Body()))) + body := resp.Body() + max := 1024 + if len(body) > max { body = body[:max] } + provider.logger.Debug("error from %s provider; status=%d body_snippet=%s", + providerName, resp.StatusCode(), string(body))
113-115: Use providerName to prefix IDs for consistency.Minor consistency/readability improvement.
- openrouterResponse.Data[i].ID = string(schemas.OpenRouter) + "/" + openrouterResponse.Data[i].ID + openrouterResponse.Data[i].ID = string(providerName) + "/" + openrouterResponse.Data[i].IDcore/providers/gemini.go (2)
87-93: Release fasthttp request/response per iteration; don’t defer in the loop.Same rationale as other providers.
- for _, key := range keys { - req := fasthttp.AcquireRequest() - resp := fasthttp.AcquireResponse() - defer fasthttp.ReleaseRequest(req) - defer fasthttp.ReleaseResponse(resp) + for _, key := range keys { + func() { + req := fasthttp.AcquireRequest() + resp := fasthttp.AcquireResponse() + defer fasthttp.ReleaseRequest(req) + defer fasthttp.ReleaseResponse(resp) ... - } + }() + }
106-107: Prefer logger formatting over fmt.Sprintf in Debug.Keeps logging consistent and avoids extra allocations.
- provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", key.ID, bifrostErr.Error.Message)) + provider.logger.Debug("failed to list models with key %s: %s", key.ID, bifrostErr.Error.Message) - provider.logger.Debug(fmt.Sprintf("error from %s provider for key %s", providerName, key.ID)) + provider.logger.Debug("error from %s provider for key %s", providerName, key.ID)Also applies to: 123-124
core/providers/anthropic.go (3)
206-212: Release fasthttp objects per-iteration (avoid defers in loop).Deferring ReleaseRequest/ReleaseResponse in a loop holds all buffers until function return. Scope them to each iteration to free memory sooner.
- for _, key := range keys { - // Create request - req := fasthttp.AcquireRequest() - resp := fasthttp.AcquireResponse() - defer fasthttp.ReleaseRequest(req) - defer fasthttp.ReleaseResponse(resp) - ... - } + for _, key := range keys { + func() { + req := fasthttp.AcquireRequest() + resp := fasthttp.AcquireResponse() + defer fasthttp.ReleaseRequest(req) + defer fasthttp.ReleaseResponse(resp) + ... + // replace `continue` with `return` inside this closure + }() + }
269-284: Preserve error metadata when all keys fail.When returning lastError, ensure Provider/RequestType are set for observability.
- if len(successfulResponses) == 0 { - if lastError != nil { - return nil, lastError - } + if len(successfulResponses) == 0 { + if lastError != nil { + if lastError.ExtraFields.Provider == "" { + lastError.ExtraFields.Provider = providerName + lastError.ExtraFields.RequestType = schemas.ListModelsRequest + } + return nil, lastError + }
205-268: Optional: parallelize per-key requests.Consider errgroup.WithContext to issue per-key GETs concurrently; reduces tail latency across many keys.
core/providers/cohere.go (3)
113-117: Release fasthttp objects per-iteration (avoid defers in loop).Same resource-retention risk. Scope to an IIFE so releases run each iteration.
- req := fasthttp.AcquireRequest() - resp := fasthttp.AcquireResponse() - defer fasthttp.ReleaseRequest(req) - defer fasthttp.ReleaseResponse(resp) + func() { + req := fasthttp.AcquireRequest() + resp := fasthttp.AcquireResponse() + defer fasthttp.ReleaseRequest(req) + defer fasthttp.ReleaseResponse(resp) + ... + }()
169-183: Preserve error metadata when all keys fail.Populate Provider/RequestType before returning lastError.
- if len(successfulResponses) == 0 { - if lastError != nil { - return nil, lastError - } + if len(successfulResponses) == 0 { + if lastError != nil { + if lastError.ExtraFields.Provider == "" { + lastError.ExtraFields.Provider = providerName + lastError.ExtraFields.RequestType = schemas.ListModelsRequest + } + return nil, lastError + }
121-123: Optional: honor request.PageSize when constructing provider call.If request.PageSize > 0, clamp to a sane max and pass it to reduce payloads.
- req.SetRequestURI(fmt.Sprintf("%s/v1/models?page_size=%d", provider.networkConfig.BaseURL, schemas.DefaultPageSize)) + pageSize := schemas.DefaultPageSize + if request.PageSize > 0 && request.PageSize < schemas.DefaultPageSize { + pageSize = request.PageSize + } + req.SetRequestURI(fmt.Sprintf("%s/v1/models?page_size=%d", provider.networkConfig.BaseURL, pageSize))core/providers/bedrock.go (1)
503-518: Preserve error metadata when all keys fail.Add Provider/RequestType before returning lastError.
- if len(successfulResponses) == 0 { - if lastError != nil { - return nil, lastError - } + if len(successfulResponses) == 0 { + if lastError != nil { + if lastError.ExtraFields.Provider == "" { + lastError.ExtraFields.Provider = providerName + lastError.ExtraFields.RequestType = schemas.ListModelsRequest + } + return nil, lastError + }core/providers/openai.go (3)
106-110: Release fasthttp objects per-iteration (avoid defers in loop).Scope Acquire/Release to each iteration to prevent buffer retention.
- req := fasthttp.AcquireRequest() - resp := fasthttp.AcquireResponse() - defer fasthttp.ReleaseRequest(req) - defer fasthttp.ReleaseResponse(resp) + func() { + req := fasthttp.AcquireRequest() + resp := fasthttp.AcquireResponse() + defer fasthttp.ReleaseRequest(req) + defer fasthttp.ReleaseResponse(resp) + ... + }()
159-173: Preserve error metadata when all keys fail.Ensure Provider/RequestType are set on lastError before returning.
- if len(successfulResponses) == 0 { - if lastError != nil { - return nil, lastError - } + if len(successfulResponses) == 0 { + if lastError != nil { + if lastError.ExtraFields.Provider == "" { + lastError.ExtraFields.Provider = providerName + lastError.ExtraFields.RequestType = schemas.ListModelsRequest + } + return nil, lastError + }
104-157: Optional: parallelize per-key GETs.Use errgroup.WithContext for concurrent per-key requests to cut tail latency.
core/providers/vertex.go (3)
209-238: Close response bodies per-iteration (avoid defer in loop).Deferring resp.Body.Close() in a loop delays connection reuse; scope the close to each iteration.
- resp, err := client.Do(req) - latency := time.Since(startTime) - if err != nil { + resp, err := client.Do(req) + latency := time.Since(startTime) + if err != nil { ... - } - defer resp.Body.Close() + } + // scope Close() to this iteration + defer resp.Body.Close()Wrap the per-key logic in an inline
func(){ ... }()and usereturninstead ofcontinueso the deferred Close runs per-iteration.
306-320: Preserve error metadata when all keys fail.Populate Provider/RequestType on lastError before returning.
- if len(successfulResponses) == 0 { - if lastError != nil { - return nil, lastError - } + if len(successfulResponses) == 0 { + if lastError != nil { + if lastError.ExtraFields.Provider == "" { + lastError.ExtraFields.Provider = providerName + lastError.ExtraFields.RequestType = schemas.ListModelsRequest + } + return nil, lastError + }
200-207: Optional: refine Vertex client cache key for default credentials.When AuthCredentials is empty, all keys share the same pooled client. If you want stricter isolation across projects/regions, include projectID/region in the cache key when credentials are empty.
// e.g., derive cache key: // if authCredentials == "" { clientKey = fmt.Sprintf("%s:%s:%s", "adc", key.VertexKeyConfig.ProjectID, key.VertexKeyConfig.Region) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/bifrost.go(2 hunks)core/providers/anthropic.go(1 hunks)core/providers/azure.go(1 hunks)core/providers/bedrock.go(1 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(1 hunks)core/providers/gemini.go(1 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(2 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(1 hunks)core/providers/openrouter.go(1 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(1 hunks)core/providers/vertex.go(1 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(0 hunks)core/schemas/providers/vertex/models.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/providers/gemini/models.go
🧰 Additional context used
🧬 Code graph analysis (18)
core/providers/ollama.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/utils.go (1)
core/schemas/models.go (2)
BifrostListModelsResponse(28-37)Model(39-56)
core/schemas/provider.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/groq.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/sgl.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (1)
BifrostListModelsRequest(15-26)
core/providers/anthropic.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)DefaultPageSize(11-11)core/schemas/bifrost.go (6)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/anthropic/types.go (2)
AnthropicError(303-309)AnthropicListModelsResponse(281-286)core/schemas/provider.go (1)
Provider(197-224)
transports/bifrost-http/handlers/server.go (1)
plugins/maxim/main.go (1)
Init(62-92)
core/providers/bedrock.go (5)
core/schemas/account.go (2)
Key(8-17)BedrockKeyConfig(39-46)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (6)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/bedrock/types.go (3)
DefaultBedrockRegion(4-4)BedrockError(349-353)BedrockListModelsResponse(456-458)core/schemas/provider.go (1)
Provider(197-224)
core/providers/parasail.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/bifrost.go (3)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/bifrost.go (3)
BifrostError(322-331)ModelProvider(32-32)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
core/providers/cohere.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)DefaultPageSize(11-11)core/schemas/bifrost.go (6)
BifrostError(322-331)Cohere(39-39)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/cohere/types.go (2)
CohereError(521-525)CohereListModelsResponse(540-543)core/schemas/provider.go (1)
Provider(197-224)
core/providers/vertex.go (5)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (6)
BifrostError(322-331)ErrorField(340-347)RequestCancelled(282-282)BifrostErrorExtraFields(389-393)RequestType(79-79)ListModelsRequest(82-82)core/schemas/providers/vertex/types.go (2)
MaxPageSize(50-50)VertexListModelsResponse(61-64)core/schemas/provider.go (1)
Provider(197-224)
core/providers/openrouter.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (6)
BifrostError(322-331)OpenRouter(48-48)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/openai.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (7)
BifrostError(322-331)OpenAI(35-35)ListModelsRequest(82-82)ModelProvider(32-32)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/openai/types.go (1)
OpenAIListModelsResponse(140-143)core/schemas/provider.go (1)
Provider(197-224)
core/providers/gemini.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)DefaultPageSize(11-11)core/schemas/bifrost.go (6)
BifrostError(322-331)Gemini(47-47)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/gemini/types.go (1)
GeminiListModelsResponse(1272-1275)core/schemas/provider.go (1)
Provider(197-224)
core/providers/azure.go (6)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (6)
BifrostError(322-331)Azure(36-36)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/utils.go (1)
Ptr(12-14)core/schemas/providers/azure/types.go (2)
DefaultAzureAPIVersion(4-4)AzureListModelsResponse(31-34)core/schemas/provider.go (1)
Provider(197-224)
core/providers/cerebras.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/mistral.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (5)
BifrostError(322-331)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/mistral/types.go (1)
MistralListModelsResponse(31-34)core/schemas/provider.go (1)
Provider(197-224)
🔇 Additional comments (13)
core/schemas/providers/cohere/models.go (3)
3-3: LGTM: Import simplification.The simplified import statement is cleaner and more maintainable.
6-12: LGTM: Defensive nil check and efficient initialization.The nil guard prevents potential panics, and pre-allocating the Data slice capacity is a good performance optimization.
10-12: Verify that omitting NextPageToken is intentional.The response initialization no longer includes a
NextPageTokenfield. Given the PR's multi-key aggregation approach, pagination across multiple keys may not be feasible. Please confirm this omission is intentional and that pagination support is either not required or handled differently in the new design.core/schemas/provider.go (1)
201-201: LGTM: Interface signature updated for multi-key support.The signature change from single
keytokeys []Keyaligns with the PR's objective to enable model discovery across multiple API keys.core/schemas/providers/vertex/models.go (1)
6-8: LGTM: Nil check added.The nil receiver check is a good defensive coding practice that prevents potential panics.
core/providers/groq.go (1)
70-82: LGTM: Multi-key signature adopted.The updated signature and delegation to
handleOpenAIListModelsRequestwith thekeysslice is consistent with the multi-key aggregation pattern.core/providers/cerebras.go (1)
65-67: LGTM: Multi-key signature adopted.Consistent with the multi-key pattern used across OpenAI-compatible providers.
core/providers/sgl.go (1)
72-84: LGTM: Multi-key signature adopted.Consistent with the multi-key pattern used across OpenAI-compatible providers.
core/providers/utils.go (1)
672-725: LGTM: Solid aggregation implementation.The function correctly:
- Handles edge cases (empty input, single response)
- Deduplicates models by ID while preserving first occurrence
- Aggregates latencies and raw responses appropriately
- Guards against nil responses
core/providers/ollama.go (1)
72-77: LGTM: Multi-key signature adopted.The updated signature and delegation pattern is consistent with other OpenAI-compatible providers.
core/providers/azure.go (2)
133-220: LGTM: Well-structured per-key processing with proper error handling.The per-key iteration correctly:
- Validates each key's Azure configuration
- Handles errors gracefully by continuing with remaining keys
- Tracks the last error for diagnostics
- Copies response bodies before release to avoid use-after-free
- Enriches responses with latency and raw data when configured
222-247: LGTM: Proper aggregation and error handling.The final aggregation logic correctly:
- Returns an appropriate error when all keys fail
- Aggregates successful responses using the utility function
- Applies pagination to the combined result
- Sets standard extra fields for provider and request type
core/providers/parasail.go (1)
70-82: LGTM - all verifications pass.Helper signature at
handleOpenAIListModelsRequest(openai.go:90-100) properly acceptskeys []schemas.Keyas the 6th parameter. All 14 provider implementations consistently pass keys, and parasail.go:70-82 correctly invokes the helper with the keys parameter.
TejasGhatte
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
c931eed to
813c716
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/bedrock.go (1)
67-71: Compile error: cannot range over int.Use a counted for-loop to pre-warm the pool.
- // Pre-warm response pools - for range config.ConcurrencyAndBufferSize.Concurrency { - bedrockChatResponsePool.Put(&bedrock.BedrockConverseResponse{}) - } + // Pre-warm response pools + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { + bedrockChatResponsePool.Put(&bedrock.BedrockConverseResponse{}) + }
♻️ Duplicate comments (1)
core/providers/bedrock.go (1)
438-441: Good: response body is closed per iteration.Addresses earlier feedback about defers in loops; avoids connection leaks.
🧹 Nitpick comments (9)
core/schemas/providers/cohere/models.go (1)
16-19: Optional: guard against unexpected width on ContextLengthIf Cohere’s ContextLength ever exceeds int width on 32‑bit builds, int() could truncate. Low risk, but you could store as int64 in Model or clamp.
core/providers/mistral.go (1)
133-144: Harden logging; consider parallelizing per‑key requests
- Logging can panic if bifrostErr.Error is nil. Use a safe message.
- Optional: run per‑key requests concurrently to cut latency when many keys.
- response, _, bifrostErr := provider.listModelsByKey(ctx, key) + response, _, bifrostErr := provider.listModelsByKey(ctx, key) if bifrostErr != nil { - provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", key.ID, bifrostErr.Error.Message)) + msg := "unknown error" + if bifrostErr.Error != nil && bifrostErr.Error.Message != "" { + msg = bifrostErr.Error.Message + } + provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", key.ID, msg)) lastError = bifrostErr continue }Concurrency (optional sketch):
// Use a bounded worker pool or errgroup with a semaphore to limit concurrency.Also applies to: 145-161, 137-139
core/providers/openai.go (1)
165-174: Safer error log + optional concurrency for per‑key fetches
- Protect against nil Error field when logging.
- Optionally parallelize per‑key list calls to improve tail latency.
- response, _, bifrostErr := listModelsByKeyOpenAI(ctx, client, url, key, extraHeaders, providerName, sendBackRawResponse) + response, _, bifrostErr := listModelsByKeyOpenAI(ctx, client, url, key, extraHeaders, providerName, sendBackRawResponse) if bifrostErr != nil { - logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", key.ID, bifrostErr.Error.Message)) + msg := "unknown error" + if bifrostErr.Error != nil && bifrostErr.Error.Message != "" { + msg = bifrostErr.Error.Message + } + logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", key.ID, msg)) lastError = bifrostErr continue }Concurrency (optional sketch):
// Use errgroup and a semaphore to fan-out by key with a small concurrency limit.Also applies to: 176-189, 168-170
core/providers/anthropic.go (1)
266-275: Run per-key fetches concurrently (bounded) for lower tail latency.Use errgroup with a semaphore to cap concurrency; preserve best‑effort behavior.
// sketch g, ctx := errgroup.WithContext(ctx) sem := make(chan struct{}, provider.networkConfig.MaxConnsPerHost) // or a small constant for _, k := range keys { k := k sem <- struct{}{} g.Go(func() error { defer func(){ <-sem }() resp, _, err := provider.listModelsByKey(ctx, k) if err != nil { provider.logger.Debug(...); mu.Lock(); lastError = err; mu.Unlock(); return nil } mu.Lock(); successfulResponses = append(successfulResponses, resp); mu.Unlock() return nil }) } _ = g.Wait()core/providers/cohere.go (2)
67-81: Honor proxy config for Cohere (consistency with other providers).Other providers call configureProxy; add it here.
streamClient := &http.Client{ Timeout: time.Second * time.Duration(config.NetworkConfig.DefaultRequestTimeoutInSeconds), } - // Pre-warm response pools + // Configure proxy if provided + client = configureProxy(client, config.ProxyConfig, logger) + + // Pre-warm response pools for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ {
167-176: Optional: parallelize per-key ListModels calls.Same pattern as other providers; using errgroup and a small concurrency cap will reduce p95 latency.
core/providers/gemini.go (1)
140-149: Optional: parallelize per-key ListModels calls.Adopt bounded concurrency to decrease overall latency.
core/providers/openrouter.go (1)
127-137: Optional: parallelize per-key ListModels calls.Use errgroup + semaphore to issue per-key GETs concurrently.
core/providers/bedrock.go (1)
361-369: Optional: parallelize per-key ListModels calls.Bounded concurrency will improve latency while keeping best‑effort semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/bifrost.go(3 hunks)core/providers/anthropic.go(3 hunks)core/providers/azure.go(3 hunks)core/providers/bedrock.go(1 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(2 hunks)core/providers/gemini.go(2 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(3 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(2 hunks)core/providers/openrouter.go(2 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(1 hunks)core/providers/vertex.go(1 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(0 hunks)core/schemas/providers/vertex/models.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/providers/gemini/models.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/providers/parasail.go
- core/schemas/providers/vertex/models.go
🧰 Additional context used
🧬 Code graph analysis (17)
core/providers/cerebras.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/cohere.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(28-37)DefaultPageSize(11-11)BifrostListModelsRequest(15-26)core/schemas/bifrost.go (6)
BifrostError(322-331)Cohere(39-39)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/cohere/types.go (2)
CohereError(521-525)CohereListModelsResponse(540-543)core/schemas/provider.go (1)
Provider(197-224)
core/providers/groq.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/openai.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (6)
BifrostError(322-331)ListModelsRequest(82-82)ModelProvider(32-32)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/logger.go (1)
Logger(28-55)core/schemas/provider.go (1)
Provider(197-224)
core/providers/sgl.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (1)
BifrostListModelsRequest(15-26)
core/providers/gemini.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(28-37)DefaultPageSize(11-11)BifrostListModelsRequest(15-26)core/schemas/bifrost.go (5)
BifrostError(322-331)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/schemas/provider.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/utils.go (1)
core/schemas/models.go (2)
BifrostListModelsResponse(28-37)Model(39-56)
core/providers/vertex.go (5)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (6)
BifrostError(322-331)ErrorField(340-347)RequestCancelled(282-282)BifrostErrorExtraFields(389-393)RequestType(79-79)ListModelsRequest(82-82)core/schemas/providers/vertex/types.go (2)
MaxPageSize(50-50)VertexListModelsResponse(61-64)core/schemas/provider.go (5)
ErrRequestCancelled(23-23)ErrProviderRequestTimedOut(22-22)ErrProviderRequest(24-24)ErrProviderResponseUnmarshal(25-25)Provider(197-224)
core/bifrost.go (3)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/bifrost.go (4)
BifrostError(322-331)ErrorField(340-347)ModelProvider(32-32)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
core/providers/openrouter.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsResponse(28-37)BifrostListModelsRequest(15-26)core/schemas/bifrost.go (6)
BifrostError(322-331)ListModelsRequest(82-82)OpenRouter(48-48)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/anthropic.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(28-37)DefaultPageSize(11-11)BifrostListModelsRequest(15-26)core/schemas/bifrost.go (6)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/anthropic/types.go (2)
AnthropicError(303-309)AnthropicListModelsResponse(281-286)core/schemas/provider.go (1)
Provider(197-224)
core/providers/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/models.go (2)
BifrostListModelsResponse(28-37)BifrostListModelsRequest(15-26)core/schemas/bifrost.go (6)
BifrostError(322-331)Azure(36-36)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
transports/bifrost-http/handlers/server.go (1)
plugins/maxim/main.go (1)
Init(62-92)
core/providers/bedrock.go (5)
core/schemas/account.go (2)
Key(8-17)BedrockKeyConfig(39-46)core/schemas/models.go (2)
BifrostListModelsRequest(15-26)BifrostListModelsResponse(28-37)core/schemas/bifrost.go (6)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/bedrock/types.go (3)
DefaultBedrockRegion(4-4)BedrockError(349-353)BedrockListModelsResponse(456-458)core/schemas/provider.go (4)
ErrProviderRequestTimedOut(22-22)ErrProviderRequest(24-24)ErrProviderResponseUnmarshal(25-25)Provider(197-224)
core/providers/ollama.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (1)
BifrostListModelsRequest(15-26)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/mistral.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsResponse(28-37)BifrostListModelsRequest(15-26)core/schemas/bifrost.go (5)
BifrostError(322-331)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
🔇 Additional comments (18)
transports/bifrost-http/handlers/server.go (1)
214-214: Past critical issue resolved.The function call now correctly passes both required parameters to
maxim.Init. The signature mismatch flagged in the previous review has been fixed.Note: This change appears unrelated to the PR's stated objective of implementing multi-key support for ListModels. Based on learnings.
core/providers/sgl.go (1)
72-84: SGL ListModels: migrated to []schemas.Key correctlySignature + call-site updated as expected. No further issues here.
core/providers/ollama.go (1)
71-77: Ollama ListModels: multi-key migration with base_url guard looks solidGood: explicit base_url validation and forwarding keys to handler. No other concerns.
core/providers/groq.go (1)
70-77: Groq ListModels: migration to []schemas.Key is correctDefault BaseURL "https://api.groq.com/openai" + "/v1/models" aligns with their OpenAI-compatible endpoint. Looks good.
core/providers/cerebras.go (1)
65-67: Review verified: all concerns satisfiedThe multi-key aggregation path is correctly wired:
handleOpenAIListModelsRequestiterates through the keys slice (line 165 in openai.go)- Collects successful responses into
successfulResponses- Calls
aggregateListModelsResponsesfor deduplication and aggregation (line 192)- Applies pagination and returns the result
CerebrasProvider.ListModels properly forwards the keys parameter through this entire flow.
core/schemas/providers/cohere/models.go (1)
6-12: Nil-guard + preallocation look goodReturning nil on nil provider response and preallocating Data by model count are fine for the aggregation flow. No issues spotted.
core/bifrost.go (1)
254-266: ListModels now aggregates keys — goodFetching all supported keys and passing to provider.ListModels aligns with multi‑key design. Looks correct.
core/schemas/provider.go (1)
201-201: Verify CHANGELOG documents ListModels breaking changeBreaking change is correctly implemented in code: all 13 in-tree Provider implementations consistently use the new
[]Keysignature with no old single-key code remaining. However, documentation review needed: ensurecore/changelog.mdand any upgrade/migration guides explicitly call out the ListModels interface change as breaking for out-of-tree provider implementations.core/providers/anthropic.go (1)
227-233: Good: safe copy of fasthttp body before release.Prevents use-after-free on pooled buffers.
core/providers/cohere.go (1)
132-140: Good: copy response body before releasing fasthttp response.Prevents buffer reuse issues.
core/providers/gemini.go (1)
106-114: Good: copy response body before releasing fasthttp response.Avoids use-after-free.
core/providers/azure.go (2)
126-204: Resource leak from previous review has been addressed.The refactoring to extract per-key logic into a helper function correctly addresses the defer-in-loop resource leak flagged in the previous review. The
deferstatements at lines 151-152 now execute when the helper returns (once per key), ensuring resources are released promptly after each key is processed.The helper function correctly:
- Validates configuration
- Handles authentication (Bearer token or api-key)
- Copies response body before release (line 184) to avoid use-after-free
- Tracks per-key latency and attaches it to the response
- Returns structured errors for different failure scenarios
Based on learnings
206-249: Best-effort aggregation correctly implemented.The multi-key orchestration properly implements the best-effort approach described in the PR objectives:
- Iterates through all keys and collects successful responses (lines 213-222)
- Continues processing remaining keys even when individual keys fail
- Returns the last error only if all keys fail (lines 224-238)
- Aggregates successful responses and applies pagination to the combined result (lines 240-242)
The error handling is appropriate: per-key failures are logged for debugging while the operation continues.
core/providers/vertex.go (5)
145-163: Configuration validation is thorough.The per-key validation properly checks for required Vertex-specific configuration (VertexKeyConfig, ProjectID, Region) and logs debug messages with the specific key ID for troubleshooting. Failed validations correctly update
lastErrorand continue processing remaining keys, maintaining the best-effort approach.
169-207: Request creation and authentication handling is robust.The code properly:
- Creates context-aware HTTP requests (line 169)
- Handles context cancellation, timeouts, and general errors with distinct error types (lines 171-193)
- Retrieves authenticated clients from the pool with error handling (lines 200-207)
- Removes faulty clients from the pool when authentication fails (line 203), preventing reuse of invalid credentials
This ensures failed keys don't block the operation while maintaining proper resource hygiene.
210-251: Request execution and response reading correctly managed.The implementation properly:
- Tracks per-key latency (line 212) for observability
- Handles execution errors with context awareness (lines 214-236)
- Closes response body immediately after reading (line 240) to prevent resource leaks
- Continues processing remaining keys after failures
Resource management is correct and aligns with Go best practices.
254-302: Error response parsing and success handling is comprehensive.The code handles:
- Non-OK status codes with appropriate client pool cleanup for auth errors (lines 255-257)
- Multiple error formats (Vertex and OpenAI) with graceful fallback parsing (lines 260-280)
- Successful response parsing with per-key latency and optional raw response attachment (lines 284-300)
The approach maximizes successful data collection while properly handling provider-specific error formats.
306-330: Aggregation logic correctly implements best-effort pattern.The implementation properly:
- Returns the last error if all keys fail (lines 307-309), providing debugging context
- Returns a descriptive error with provider metadata if no last error exists (lines 310-320)
- Aggregates successful responses and applies pagination to the combined result (lines 323-324)
- Sets provider metadata in the aggregated response (lines 327-328)
This aligns with the PR objectives of aggregating results from multiple keys with deduplication.
TejasGhatte
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
813c716 to
caed214
Compare
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 (4)
core/schemas/providers/cohere/models.go (1)
10-12: Add NextPageToken mapping to Cohere provider response construction.The
ToBifrostListModelsResponsemethod in the Cohere provider is missing theNextPageTokenfield. The inputCohereListModelsResponseincludesNextPageToken, but it's not being mapped to theBifrostListModelsResponse. This breaks pagination, as the bifrost layer checksNextPageTokento determine if more pages exist.Add
NextPageToken: response.NextPageToken,to thebifrostResponseinitialization at line 10-12:bifrostResponse := &schemas.BifrostListModelsResponse{ Data: make([]schemas.Model, 0, len(response.Models)), NextPageToken: response.NextPageToken, }This aligns with how other providers (e.g., Anthropic) handle pagination tokens.
core/providers/vertex.go (1)
71-76: Fix compile error: invalidfor rangeover int.Use a counted loop to pre-warm the pool.
Apply this diff:
- // Pre-warm response pools - for range config.ConcurrencyAndBufferSize.Concurrency { - // openAIResponsePool.Put(&schemas.BifrostResponse{}) - anthropicChatResponsePool.Put(&anthropic.AnthropicMessageResponse{}) - } + // Pre-warm response pools + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { + // openAIResponsePool.Put(&schemas.BifrostResponse{}) + anthropicChatResponsePool.Put(&anthropic.AnthropicMessageResponse{}) + }core/providers/bedrock.go (1)
68-70: Fix compile error: invalidfor rangeover int.Use a counted loop to pre-warm the pool.
Apply this diff:
- for range config.ConcurrencyAndBufferSize.Concurrency { - bedrockChatResponsePool.Put(&bedrock.BedrockConverseResponse{}) - } + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { + bedrockChatResponsePool.Put(&bedrock.BedrockConverseResponse{}) + }core/providers/openai.go (1)
211-219: Nil-guard request before pagination.Prevent NPE when request is nil.
Apply this diff:
- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)
♻️ Duplicate comments (3)
core/providers/cohere.go (1)
213-221: Nil-guard request before pagination.Line 214 accesses
request.PageSizeandrequest.PageTokenwithout checking ifrequestis nil, which can cause a panic.Apply this diff to add nil-guard:
response := aggregateListModelsResponses(successfulResponses) -response = response.ApplyPagination(request.PageSize, request.PageToken) +pageSize := schemas.DefaultPageSize +pageToken := "" +if request != nil { + pageSize = request.PageSize + pageToken = request.PageToken +} +response = response.ApplyPagination(pageSize, pageToken)core/bifrost.go (1)
254-266: Prevent ctx nil panic and reject empty direct keys (and add ctx fallback).
- Calling (*ctx).Value without guarding *ctx can panic when ctx points to a nil interface.
- If a direct key is provided but empty for providers that require keys, fail fast.
- Align ListModelsRequest with other APIs by defaulting a nil ctx.
Apply:
@@ func (bifrost *Bifrost) ListModelsRequest(ctx context.Context, req *schemas.BifrostListModelsRequest) (*schemas.BifrostListModelsResponse, *schemas.BifrostError) { + // Align with other request paths: guard nil ctx to avoid downstream panics + if ctx == nil { + ctx = bifrost.ctx + } @@ - keys, err := bifrost.getAllSupportedKeys(&ctx, req.Provider, baseProvider) + keys, err := bifrost.getAllSupportedKeys(&ctx, req.Provider, baseProvider) if err != nil { return nil, &schemas.BifrostError{ IsBifrostError: false, Error: &schemas.ErrorField{ Message: err.Error(), Error: err, }, } } @@ -// getAllSupportedKeys retrieves all valid keys for a ListModels request. -// allowing the provider to aggregate results from multiple keys. +// getAllSupportedKeys retrieves all valid keys for a ListModels request, +// allowing the provider to aggregate results from multiple keys. func (bifrost *Bifrost) getAllSupportedKeys(ctx *context.Context, providerKey schemas.ModelProvider, baseProviderType schemas.ModelProvider) ([]schemas.Key, error) { - // Check if key has been set in the context explicitly - if ctx != nil { - key, ok := (*ctx).Value(schemas.BifrostContextKeyDirectKey).(schemas.Key) + // Check if key has been set in the context explicitly + if ctx != nil && *ctx != nil { + key, ok := (*ctx).Value(schemas.BifrostContextKeyDirectKey).(schemas.Key) if ok { - // If a direct key is specified, return it as a single-element slice - return []schemas.Key{key}, nil + // If provider requires a key, reject empty direct key to avoid confusing failures + if providerRequiresKey(baseProviderType) && strings.TrimSpace(key.Value) == "" { + return nil, fmt.Errorf("direct key is empty for provider: %v", providerKey) + } + // If a direct key is specified, return it as a single-element slice + return []schemas.Key{key}, nil } } keys, err := bifrost.account.GetKeysForProvider(ctx, providerKey) if err != nil { return nil, err } if len(keys) == 0 { return nil, fmt.Errorf("no keys found for provider: %v", providerKey) } // Filter keys for ListModels - only check if key has a value var supportedKeys []schemas.Key for _, k := range keys { if strings.TrimSpace(k.Value) != "" || canProviderKeyValueBeEmpty(baseProviderType) { supportedKeys = append(supportedKeys, k) } } if len(supportedKeys) == 0 { return nil, fmt.Errorf("no valid keys found for provider: %v", providerKey) } return supportedKeys, nil }Also applies to: 2155-2189
core/providers/bedrock.go (1)
531-541: Nil-guard request before pagination.Prevent NPE when request is nil.
Apply this diff:
- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)
🧹 Nitpick comments (12)
core/providers/mistral.go (1)
126-191: Centralize multi-key orchestration to reduce duplication across providers.The WaitGroup + buffered results channel pattern repeats in multiple providers. Extract a shared helper (e.g., runListModelsAcrossKeys) in providers/utils.go to launch per-key calls, collect successes, carry last error, and return the aggregated response.
Example shape:
+// providers/utils.go +func RunListModelsAcrossKeys( + ctx context.Context, + keys []schemas.Key, + perKey func(context.Context, schemas.Key) (*schemas.BifrostListModelsResponse, time.Duration, *schemas.BifrostError), + provider schemas.ModelProvider, + req *schemas.BifrostListModelsRequest, + logger schemas.Logger, +) (*schemas.BifrostListModelsResponse, *schemas.BifrostError) { + // spawn goroutines, collect, aggregate, ApplyPagination, set ExtraFields... +}Then call it here with provider.listModelsByKey.
core/providers/gemini.go (1)
128-196: Reuse a shared helper for multi-key ListModels flow.Same concurrency/aggregation logic appears in multiple providers. Consider a common helper (e.g., RunListModelsAcrossKeys) to reduce boilerplate and keep behaviors consistent.
core/providers/anthropic.go (1)
251-321: Deduplicate the concurrent per-key aggregation across providers.Extract the repeated ListModels orchestration into a shared providers/utils.go helper to minimize drift and simplify future fixes.
core/providers/openrouter.go (1)
120-185: Factor out shared multi-key ListModels orchestration.Adopt a common helper (e.g., RunListModelsAcrossKeys) for the WaitGroup + results channel pattern used here and in other providers.
core/providers/vertex.go (2)
303-311: Nil-guard error logging.Avoid dereferencing Err.Error when it is nil.
Apply this diff:
- if result.Err != nil { - provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", result.KeyID, result.Err.Error.Message)) + if result.Err != nil { + msg := "" + if result.Err.Error != nil { + msg = result.Err.Error.Message + } + provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", result.KeyID, msg)) lastError = result.Err continue }
279-284: Early guard when no keys.Return a clear error instead of “all keys failed” for empty input.
Apply this diff:
startTime := time.Now() providerName := provider.GetProviderKey() + if len(keys) == 0 { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{Message: "no keys provided"}, + ExtraFields: schemas.BifrostErrorExtraFields{Provider: providerName, RequestType: schemas.ListModelsRequest}, + } + } + results := make(chan schemas.ListModelsByKeyResult, len(keys))core/providers/bedrock.go (2)
504-512: Nil-guard error logging.Avoid dereferencing Err.Error when it is nil.
Apply this diff:
- if result.Err != nil { - provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", result.KeyID, result.Err.Error.Message)) + if result.Err != nil { + msg := "" + if result.Err.Error != nil { + msg = result.Err.Error.Message + } + provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", result.KeyID, msg)) lastError = result.Err continue }
480-486: Early guard when no keys.Return a clear error instead of “all keys failed” for empty input.
Apply this diff:
startTime := time.Now() providerName := provider.GetProviderKey() + if len(keys) == 0 { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{Message: "no keys provided"}, + ExtraFields: schemas.BifrostErrorExtraFields{Provider: providerName, RequestType: schemas.ListModelsRequest}, + } + } + results := make(chan schemas.ListModelsByKeyResult, len(keys))core/providers/azure.go (2)
233-241: Nil-guard error logging.Avoid dereferencing Err.Error when it is nil.
Apply this diff:
- if result.Err != nil { - provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", result.KeyID, result.Err.Error.Message)) + if result.Err != nil { + msg := "" + if result.Err.Error != nil { + msg = result.Err.Error.Message + } + provider.logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", result.KeyID, msg)) lastError = result.Err continue }
211-218: Early guard when no keys.Return a clear error instead of “all keys failed” for empty input.
Apply this diff:
startTime := time.Now() results := make(chan schemas.ListModelsByKeyResult, len(keys)) + if len(keys) == 0 { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{Message: "no keys provided"}, + ExtraFields: schemas.BifrostErrorExtraFields{Provider: schemas.Azure, RequestType: schemas.ListModelsRequest}, + } + } var wg sync.WaitGroupcore/providers/openai.go (2)
185-193: Nil-guard error logging.Avoid dereferencing Err.Error when it is nil.
Apply this diff:
- if result.Err != nil { - logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", result.KeyID, result.Err.Error.Message)) + if result.Err != nil { + msg := "" + if result.Err.Error != nil { + msg = result.Err.Error.Message + } + logger.Debug(fmt.Sprintf("failed to list models with key %s: %s", result.KeyID, msg)) lastError = result.Err continue }
163-170: Early guard when no keys.Return a clear error instead of “all keys failed” for empty input.
Apply this diff:
startTime := time.Now() results := make(chan schemas.ListModelsByKeyResult, len(keys)) + if len(keys) == 0 { + return nil, &schemas.BifrostListModelsResponse{ + ExtraFields: schemas.BifrostResponseExtraFields{ + Provider: providerName, + RequestType: schemas.ListModelsRequest, + }, + }, &schemas.BifrostError{IsBifrostError: false, Error: &schemas.ErrorField{Message: "no keys provided"}} + } var wg sync.WaitGroup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/bifrost.go(3 hunks)core/providers/anthropic.go(3 hunks)core/providers/azure.go(4 hunks)core/providers/bedrock.go(7 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(2 hunks)core/providers/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(3 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(3 hunks)core/providers/openrouter.go(3 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(1 hunks)core/providers/vertex.go(6 hunks)core/schemas/models.go(3 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(0 hunks)core/schemas/providers/vertex/models.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/providers/gemini/models.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/groq.go
- core/providers/ollama.go
- transports/bifrost-http/handlers/server.go
🧰 Additional context used
🧬 Code graph analysis (16)
core/providers/cerebras.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)
core/providers/bedrock.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/anthropic.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/anthropic/types.go (2)
AnthropicError(303-309)AnthropicListModelsResponse(281-286)core/schemas/provider.go (1)
Provider(197-224)
core/schemas/models.go (1)
core/schemas/bifrost.go (2)
BifrostError(322-331)BifrostResponseExtraFields(254-262)
core/providers/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)Azure(36-36)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/schemas/provider.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/mistral.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (5)
BifrostError(322-331)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/mistral/types.go (1)
MistralListModelsResponse(31-34)core/schemas/provider.go (1)
Provider(197-224)
core/providers/sgl.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (1)
BifrostListModelsRequest(23-34)
core/providers/utils.go (1)
core/schemas/models.go (2)
BifrostListModelsResponse(36-45)Model(47-64)
core/bifrost.go (3)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/bifrost.go (3)
BifrostError(322-331)ModelProvider(32-32)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
core/providers/openrouter.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)ListModelsRequest(82-82)OpenRouter(48-48)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/gemini.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (5)
BifrostError(322-331)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/openai.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (7)
BifrostError(322-331)OpenAI(35-35)ListModelsRequest(82-82)ModelProvider(32-32)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/openai/types.go (1)
OpenAIListModelsResponse(140-143)core/schemas/provider.go (1)
Provider(197-224)
core/providers/parasail.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)
core/providers/vertex.go (4)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)ErrorField(340-347)RequestCancelled(282-282)BifrostErrorExtraFields(389-393)RequestType(79-79)ListModelsRequest(82-82)core/schemas/provider.go (3)
ErrProviderRequestTimedOut(22-22)ErrProviderRequest(24-24)Provider(197-224)
core/providers/cohere.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)Cohere(39-39)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/cohere/types.go (2)
CohereError(521-525)CohereListModelsResponse(540-543)core/schemas/provider.go (1)
Provider(197-224)
🔇 Additional comments (11)
core/schemas/providers/cohere/models.go (2)
3-3: Good defensive coding and clean imports.The simplified imports and nil guard are well-implemented defensive coding practices.
Also applies to: 6-8
14-21: No type safety issue exists; the conversion is redundant but safe.The
ContextLengthfield inCohereModelis already defined as typeint(line 534, types.go), notint64oruint64. The conversionint(model.ContextLength)is redundant but poses no overflow risk since it convertsinttoint. The code is type-safe as written.Likely an incorrect or invalid review comment.
core/providers/utils.go (1)
672-720: LGTM! Clean aggregation logic with proper deduplication.The function is well-designed with clear separation of concerns:
- Handles edge cases (empty input, single response)
- Deduplicates by model ID while preserving first occurrence
- Aggregates raw responses into a slice for multi-key transparency
- Leaves metadata enrichment (provider, latency, request type) to callers
The implementation correctly supports the multi-key orchestration pattern across providers.
core/schemas/providers/vertex/models.go (1)
5-24: LGTM! Simplified transformation aligns with multi-key pattern.The function now focuses solely on converting Vertex models to Bifrost format:
- Nil-check prevents panics
- Pre-sized slice allocation is efficient
- ID format
Vertex/{name}maintains provider namespace- Pagination now handled by aggregation layer (consistent with other providers)
core/schemas/models.go (1)
13-21: LGTM! New types support multi-key orchestration.The additions are well-designed:
MaxPaginationRequestsprovides a reasonable safety limit (20) for pagination loopsListModelsByKeyResultcleanly captures per-key outcomes (response, error, key ID) for concurrent processingThese types enable the multi-key aggregation pattern seen across providers.
core/providers/parasail.go (1)
70-82: LGTM! Signature correctly updated for multi-key support.The changes align with the updated Provider interface:
- Method signature accepts
keys []schemas.Key- Call to
handleOpenAIListModelsRequestforwards the keys slice- Multi-key orchestration handled by shared OpenAI logic
core/providers/sgl.go (1)
72-84: LGTM! Signature correctly updated for multi-key support.The changes match the Provider interface update:
- Method now accepts
keys []schemas.Key- Forwards keys to shared OpenAI handling logic
- Consistent with other OpenAI-compatible providers
core/providers/cerebras.go (1)
65-67: LGTM! Signature correctly updated for multi-key support.The update follows the standard pattern:
- Method signature accepts
keys []schemas.Key- Delegates to shared OpenAI-compatible handler
- Consistent with other providers using the OpenAI flow
core/providers/cohere.go (2)
98-152: LGTM! Per-key helper is clean and well-structured.The
listModelsByKeyfunction correctly:
- Makes a single-key request to Cohere's API
- Handles errors with proper context
- Returns response, latency, and error tuple
- Copies response body before releasing (line 133)
- Properly tracks latency for each key
This helper enables the concurrent multi-key pattern in the main ListModels method.
154-210: LGTM! Multi-key orchestration follows best-effort pattern correctly.The implementation properly:
- Checks operation permissions before processing
- Launches concurrent goroutines for all keys (lines 168-175)
- Collects per-key results via channel
- Logs individual key failures while continuing (line 187)
- Returns last error only if all keys fail (lines 196-210)
The concurrent pattern with WaitGroup and result channel is correct.
core/schemas/provider.go (1)
201-201: LGTM! Interface signature correctly updated for multi-key support.The Provider interface signature change from
key Keytokeys []Keyenables:
- Multi-key model discovery and aggregation
- Best-effort approach across multiple API keys
- Consistent interface across all provider implementations
All providers in this PR have been updated to implement the new signature.
TejasGhatte
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
caed214 to
ac6befd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
core/providers/openai.go (1)
152-221: Guard against nil request before calling ApplyPagination.Line 212 dereferences
request.PageSizeandrequest.PageTokenwithout a nil check. Ifrequestis nil, this will panic.Apply this defensive guard:
response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + pageSize, pageToken := 0, "" + if request != nil { + pageSize = request.PageSize + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken) // Set ExtraFields latency := time.Since(startTime)Note:
ApplyPaginationshould handlepageSize == 0gracefully (e.g., use a default).core/providers/gemini.go (1)
325-333: Fix provider name in comment.Docstring references Anthropic; this file is Gemini.
-// Responses performs a chat completion request to Anthropic's API. -// It formats the request, sends it to Anthropic, and processes the response. +// Responses performs a chat completion request to the Gemini API. +// It formats the request, sends it to Gemini, and processes the response.core/providers/vertex.go (1)
71-76: Fix compile error: cannot range over int.Pre-warm with a counted loop.
- // Pre-warm response pools - for range config.ConcurrencyAndBufferSize.Concurrency { - // openAIResponsePool.Put(&schemas.BifrostResponse{}) - anthropicChatResponsePool.Put(&anthropic.AnthropicMessageResponse{}) - } + // Pre-warm response pools + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { + // openAIResponsePool.Put(&schemas.BifrostResponse{}) + anthropicChatResponsePool.Put(&anthropic.AnthropicMessageResponse{}) + }core/providers/bedrock.go (1)
68-70: Fix compile error: cannot range over int.Use a counted loop to pre-warm the pool.
- for range config.ConcurrencyAndBufferSize.Concurrency { - bedrockChatResponsePool.Put(&bedrock.BedrockConverseResponse{}) - } + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { + bedrockChatResponsePool.Put(&bedrock.BedrockConverseResponse{}) + }core/schemas/providers/cohere/models.go (1)
10-23: Add NextPageToken assignment to Cohere transformation to enable full model pagination.The Cohere models transformation is missing NextPageToken propagation. While CohereListModelsResponse includes a NextPageToken field, the
ToBifrostListModelsResponsefunction only initializes the Data field and leaves NextPageToken empty. This causes the pagination loop in ListAllModels to break after the first API call, resulting in incomplete model listings (only ~1000 models instead of all available).In contrast, Anthropic's transformation explicitly populates NextPageToken when pagination data is available, enabling full pagination within the multi-key aggregation. The Cohere transformation should do the same:
if response.NextPageToken != "" { bifrostResponse.NextPageToken = response.NextPageToken }Add this assignment after initializing
bifrostResponseto ensure all Cohere models are retrieved during pagination.
♻️ Duplicate comments (7)
core/providers/anthropic.go (1)
248-319: Guard against nil request before calling ApplyPagination.Line 310 dereferences
request.PageSizeandrequest.PageTokenwithout a nil check, identical to the issue in openai.go. Ifrequestis nil, this will panic.Apply this defensive guard:
response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + pageSize, pageToken := 0, "" + if request != nil { + pageSize = request.PageSize + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken) // Set ExtraFields latency := time.Since(startTime)Note: Ensure
ApplyPaginationhandlespageSize == 0gracefully.core/bifrost.go (1)
2155-2189: Prevent panic on nil interface ctx and validate empty direct key.Calling (*ctx).Value when *ctx is a nil interface will panic. Also, a direct key with an empty value should be rejected for providers that require a key.
func (bifrost *Bifrost) getAllSupportedKeys(ctx *context.Context, providerKey schemas.ModelProvider, baseProviderType schemas.ModelProvider) ([]schemas.Key, error) { // Check if key has been set in the context explicitly - if ctx != nil { - key, ok := (*ctx).Value(schemas.BifrostContextKeyDirectKey).(schemas.Key) + if ctx != nil && *ctx != nil { + key, ok := (*ctx).Value(schemas.BifrostContextKeyDirectKey).(schemas.Key) if ok { - // If a direct key is specified, return it as a single-element slice - return []schemas.Key{key}, nil + // Fail fast if provider requires a key but the direct key is empty + if providerRequiresKey(baseProviderType) && strings.TrimSpace(key.Value) == "" { + return nil, fmt.Errorf("direct key is empty for provider: %v", providerKey) + } + // If a direct key is specified, return it as a single-element slice + return []schemas.Key{key}, nil } }core/providers/azure.go (1)
260-267: Add nil-guard before pagination to avoid panic.Even if callers typically pass a non-nil request, this function accepts a nil pointer. Guard defensively.
Apply this diff:
- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)To verify call sites never pass nil today:
#!/bin/bash rg -nP --type=go -C2 '\.\s*ListModels\s*\([^,]+,\s*[^,]+,\s*([^)]+)\)' | sed -n '1,80p'core/providers/vertex.go (2)
331-333: Add nil-guard before pagination.- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { pageSize = request.PageSize } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)
154-156: Handle Vertex global endpoint correctly.For region == "global", host must be aiplatform.googleapis.com.
- requestURL := fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1/projects/%s/locations/%s/models?pageSize=%d", region, projectID, region, vertex.MaxPageSize) + var host string + if region == "global" { + host = "aiplatform.googleapis.com" + } else { + host = fmt.Sprintf("%s-aiplatform.googleapis.com", region) + } + requestURL := fmt.Sprintf("https://%s/v1/projects/%s/locations/%s/models?pageSize=%d", host, projectID, region, vertex.MaxPageSize)core/providers/bedrock.go (2)
531-540: Add nil-guard before pagination.- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { pageSize = request.PageSize } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)
392-396: Latency is effectively zero; measure correctly around Do(req).- // Execute the request - resp, err := provider.client.Do(req) + // Execute the request + startTime := time.Now() + resp, err := provider.client.Do(req) @@ - response.ExtraFields.Latency = time.Since(time.Now()).Milliseconds() + response.ExtraFields.Latency = time.Since(startTime).Milliseconds()Also applies to: 463-463
🧹 Nitpick comments (7)
core/providers/gemini.go (1)
566-575: Avoid shadowing the scanner buffer variable in streaming path.The inner variable buf shadows the outer scanner buffer and hurts readability. Rename the inner one.
- var buf []byte + var audioBuf []byte for _, part := range candidate.Content.Parts { if part.InlineData != nil && part.InlineData.Data != nil { - buf = append(buf, part.InlineData.Data...) + audioBuf = append(audioBuf, part.InlineData.Data...) } } - if len(buf) > 0 { - audioChunk = buf + if len(audioBuf) > 0 { + audioChunk = audioBuf }core/providers/azure.go (2)
215-223: Bound per-key goroutines to avoid thundering-herd.Throttle to the HTTP client’s capacity to reduce burst load and GC pressure.
+ // Throttle concurrent per-key requests to client capacity + sem := make(chan struct{}, provider.client.MaxConnsPerHost) for _, key := range keys { wg.Add(1) - go func(k schemas.Key) { + sem <- struct{}{} + go func(k schemas.Key) { - defer wg.Done() + defer wg.Done() + defer func(){ <-sem }() resp, _, bifrostErr := provider.listModelsByKey(ctx, k) results <- schemas.ListModelsByKeyResult{Response: resp, Err: bifrostErr, KeyID: k.ID} }(key) }
243-257: Return a clearer error when no keys are provided.Differentiate “no keys configured” from “all keys failed.”
- if len(successfulResponses) == 0 { + if len(successfulResponses) == 0 { + if len(keys) == 0 { + return nil, &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{ Message: "no keys configured for provider" }, + ExtraFields: schemas.BifrostErrorExtraFields{ + Provider: schemas.Azure, + RequestType: schemas.ListModelsRequest, + }, + } + } if lastError != nil { return nil, lastError }core/providers/vertex.go (2)
286-293: Throttle per-key concurrency.Prevent unbounded goroutine fan-out when many keys are configured.
Consider the same token-bucket pattern shown in Azure (using a buffered channel) sized to a reasonable limit or derived from config. Keep the buffered results channel.
314-327: Clarify error for empty key set.Return a specific “no keys configured for provider” error when len(keys) == 0 before aggregating, mirroring the Azure suggestion.
core/providers/bedrock.go (2)
487-494: Throttle per-key concurrency.Avoid spinning up a goroutine per key; cap to a sensible limit or config-driven value.
515-529: Return specific error when no keys are provided.Mirror the Azure suggestion to return “no keys configured for provider” when len(keys) == 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/bifrost.go(3 hunks)core/providers/anthropic.go(3 hunks)core/providers/azure.go(4 hunks)core/providers/bedrock.go(7 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(2 hunks)core/providers/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(3 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(3 hunks)core/providers/openrouter.go(3 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(1 hunks)core/providers/vertex.go(6 hunks)core/schemas/models.go(3 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(0 hunks)core/schemas/providers/vertex/models.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/providers/gemini/models.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/providers/ollama.go
- core/schemas/models.go
- core/providers/cerebras.go
- transports/bifrost-http/handlers/server.go
🧰 Additional context used
🧬 Code graph analysis (15)
core/schemas/provider.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/parasail.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/cohere.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)Cohere(39-39)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/cohere/types.go (2)
CohereError(521-525)CohereListModelsResponse(540-543)core/schemas/provider.go (1)
Provider(197-224)
core/providers/openrouter.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (5)
BifrostError(322-331)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/utils.go (1)
core/schemas/models.go (2)
BifrostListModelsResponse(36-45)Model(47-64)
core/providers/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)Azure(36-36)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/vertex.go (5)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)ErrorField(340-347)RequestCancelled(282-282)BifrostErrorExtraFields(389-393)RequestType(79-79)ListModelsRequest(82-82)core/schemas/providers/vertex/types.go (1)
MaxPageSize(50-50)core/schemas/provider.go (1)
Provider(197-224)
core/providers/gemini.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)Gemini(47-47)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/gemini/types.go (1)
GeminiListModelsResponse(1272-1275)core/schemas/provider.go (1)
Provider(197-224)
core/bifrost.go (3)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/bifrost.go (3)
BifrostError(322-331)ModelProvider(32-32)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
core/providers/groq.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/mistral.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (5)
BifrostError(322-331)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/providers/mistral/types.go (1)
MistralListModelsResponse(31-34)core/schemas/provider.go (1)
Provider(197-224)
core/providers/openai.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)ListModelsRequest(82-82)ModelProvider(32-32)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/logger.go (1)
Logger(28-55)core/schemas/provider.go (1)
Provider(197-224)
core/providers/sgl.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/anthropic.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (5)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)core/schemas/provider.go (1)
Provider(197-224)
core/providers/bedrock.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
🔇 Additional comments (14)
core/providers/utils.go (1)
672-720: LGTM! Aggregation logic is clean and correct.The deduplication by model ID, raw response collection, and empty/single-input optimizations are well implemented. Metadata fields (Provider, RequestType, Latency) are correctly left to callers to populate after aggregation.
core/schemas/provider.go (1)
201-201: LGTM! Interface signature updated for multi-key support.The change from
key Keytokeys []Keyaligns with the multi-key orchestration pattern implemented across all providers.core/providers/parasail.go (1)
70-82: LGTM! Clean delegation to shared multi-key handler.The signature update and parameter forwarding correctly implement the multi-key pattern by delegating to
handleOpenAIListModelsRequest.core/providers/sgl.go (1)
72-84: LGTM! Consistent multi-key delegation.Mirrors the pattern used in parasail.go—clean signature update with proper forwarding to the shared handler.
core/providers/openai.go (2)
81-89: LGTM! Signature updated and gating check preserved.The multi-key signature change is correct, and operation gating is properly enforced before delegation.
91-150: LGTM! Per-key helper is well-structured.The helper correctly:
- Handles per-key HTTP requests with auth
- Returns latency for tracking
- Propagates errors with latency preserved
- Sets ExtraFields.Latency on successful responses
core/schemas/providers/vertex/models.go (1)
5-24: LGTM! Nil guard and data transformation are correct.The nil receiver check prevents panics, and the model transformation correctly maps Vertex fields (DisplayName, Description, VersionCreateTime) to the Bifrost schema.
core/providers/groq.go (1)
70-82: LGTM! Multi-key delegation consistent with other OpenAI-compatible providers.Clean signature update with proper forwarding to the shared handler.
core/providers/anthropic.go (1)
191-246: LGTM! Per-key helper follows consistent pattern.The helper correctly handles per-key requests with Anthropic-specific headers and returns latency for tracking, mirroring the openai.go implementation.
core/providers/mistral.go (1)
126-191: Multi-key ListModels orchestration looks solid.Concurrent per-key fetch, aggregation, best-effort error handling, and pagination are implemented correctly.
core/providers/cohere.go (1)
154-223: LGTM on multi-key ListModels.Per-key helper, concurrency, aggregation, pagination, and metadata handling are correct and consistent with the project pattern.
core/providers/openrouter.go (1)
120-185: LGTM on multi-key ListModels.Concurrency, aggregation, pagination, and ID namespacing via "openrouter/" are handled well.
core/schemas/providers/cohere/models.go (2)
3-3: LGTM: Import statement.The import statement is clean and appropriate for the types used in this file.
6-8: LGTM: Nil check addition.The nil guard is good defensive programming and prevents potential nil pointer dereferences when accessing
response.Models.
TejasGhatte
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
ac6befd to
9114542
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 (4)
core/providers/utils.go (2)
32-37: Fix goroutine leak on context cancel in makeRequestWithContext.If ctx is done, the Do() goroutine can block sending to errChan forever. Send non‑blocking when ctx is cancelled.
- go func() { - // client.Do is a blocking call. - // It will send an error (or nil for success) to errChan when it completes. - errChan <- client.Do(req, resp) - }() + go func() { + // client.Do is a blocking call. + err := client.Do(req, resp) + // Avoid blocking send if caller already returned due to ctx.Done() + select { + case errChan <- err: + case <-ctx.Done(): + // Drop the result; caller has already returned. + } + }()
149-165: Authorization skip and header skipping should use canonicalized keys.Current checks compare the original key, letting "authorization" bypass the skip and be set from user headers. Critical.
func setExtraHeaders(req *fasthttp.Request, extraHeaders map[string]string, skipHeaders []string) { if extraHeaders == nil { return } + // Build a canonicalized skip set + skipSet := map[string]struct{}{} + if len(skipHeaders) > 0 { + for _, h := range skipHeaders { + skipSet[textproto.CanonicalMIMEHeaderKey(h)] = struct{}{} + } + } + for key, value := range extraHeaders { canonicalKey := textproto.CanonicalMIMEHeaderKey(key) - // Skip Authorization header for security reasons - if key == "Authorization" { + // Skip Authorization header for security reasons (case-insensitive) + if canonicalKey == "Authorization" { continue } - if skipHeaders != nil { - if slices.Contains(skipHeaders, key) { - continue - } - } + if _, found := skipSet[canonicalKey]; found { + continue + } // Only set the header if it doesn't already exist to avoid overwriting important headers if len(req.Header.Peek(canonicalKey)) == 0 { req.Header.Set(canonicalKey, value) } } } func setExtraHeadersHTTP(req *http.Request, extraHeaders map[string]string, skipHeaders []string) { if extraHeaders == nil { return } + // Build a canonicalized skip set + skipSet := map[string]struct{}{} + if len(skipHeaders) > 0 { + for _, h := range skipHeaders { + skipSet[textproto.CanonicalMIMEHeaderKey(h)] = struct{}{} + } + } + for key, value := range extraHeaders { canonicalKey := textproto.CanonicalMIMEHeaderKey(key) - // Skip Authorization header for security reasons - if key == "Authorization" { + // Skip Authorization header for security reasons (case-insensitive) + if canonicalKey == "Authorization" { continue } - if skipHeaders != nil { - if slices.Contains(skipHeaders, key) { - continue - } - } + if _, found := skipSet[canonicalKey]; found { + continue + } // Only set the header if it doesn't already exist to avoid overwriting important headers if req.Header.Get(canonicalKey) == "" { req.Header.Set(canonicalKey, value) } } }Also applies to: 177-193
core/providers/vertex.go (1)
71-76: Compile error: invalidfor rangeover int.Use a counted loop to pre‑warm pools.
- // Pre-warm response pools - for range config.ConcurrencyAndBufferSize.Concurrency { - // openAIResponsePool.Put(&schemas.BifrostResponse{}) - anthropicChatResponsePool.Put(&anthropic.AnthropicMessageResponse{}) - } + // Pre-warm response pools + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ { + // openAIResponsePool.Put(&schemas.BifrostResponse{}) + anthropicChatResponsePool.Put(&anthropic.AnthropicMessageResponse{}) + }core/providers/bedrock.go (1)
392-407: Fix near-zero latency measurement in listModelsByKey.
response.ExtraFields.Latency = time.Since(time.Now())always yields ~0. Capture start beforeDo(req)and use it.@@ - // Execute the request - resp, err := provider.client.Do(req) + // Execute the request and measure latency + startTime := time.Now() + resp, err := provider.client.Do(req) @@ - response.ExtraFields.Latency = time.Since(time.Now()).Milliseconds() + response.ExtraFields.Latency = time.Since(startTime).Milliseconds()Also applies to: 463-465
♻️ Duplicate comments (6)
core/providers/cohere.go (1)
186-189: Nil-guard ApplyPagination to avoid NPE when request is nil.- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)core/providers/openrouter.go (1)
147-149: Nil-guard ApplyPagination to avoid NPE when request is nil.- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)core/providers/anthropic.go (1)
282-290: Nil-guard ApplyPagination to avoid NPE when request is nil.- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)core/providers/vertex.go (2)
154-156: Fix Vertex global endpoint host for region "global".- requestURL := fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1/projects/%s/locations/%s/models?pageSize=%d", region, projectID, region, vertex.MaxPageSize) + var host string + if region == "global" { + host = "aiplatform.googleapis.com" + } else { + host = fmt.Sprintf("%s-aiplatform.googleapis.com", region) + } + requestURL := fmt.Sprintf("https://%s/v1/projects/%s/locations/%s/models?pageSize=%d", host, projectID, region, vertex.MaxPageSize)
304-306: Nil-guard ApplyPagination to avoid NPE when request is nil.- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)core/providers/bedrock.go (1)
505-514: Re: nil-guard for pagination — safe via upstream.This mirrors an earlier comment;
ListModelsRequestalways allocates a non-nilrequestincore/bifrost.gobefore calling providers. No change needed.
🧹 Nitpick comments (4)
core/providers/mistral.go (1)
71-124: Per‑key worker solid; add Accept header for completeness.Request construction, error handling, latency, and raw-response handling look good. Consider adding an explicit Accept header.
req.Header.SetMethod(http.MethodGet) req.Header.SetContentType("application/json") +req.Header.Set("Accept", "application/json") req.Header.Set("Authorization", "Bearer "+key.Value)core/providers/anthropic.go (1)
227-235: Guard against fasthttp buffer reuse by copying body before parsing.handleProviderResponse spawns goroutines; copying ensures safety and parity with other providers.
- // Parse Anthropic's response - var anthropicResponse anthropic.AnthropicListModelsResponse - rawResponse, bifrostErr := handleProviderResponse(resp.Body(), &anthropicResponse, provider.sendBackRawResponse) + // Parse Anthropic's response (copy body to avoid buffer reuse issues) + bodyCopy := append([]byte(nil), resp.Body()...) + var anthropicResponse anthropic.AnthropicListModelsResponse + rawResponse, bifrostErr := handleProviderResponse(bodyCopy, &anthropicResponse, provider.sendBackRawResponse)core/providers/bedrock.go (1)
364-369: Avoid shadowing the imported package nameurl.Rename the local variable to reduce confusion with
net/url.- // List models endpoint uses the bedrock service (not bedrock-runtime) - url := fmt.Sprintf("https://bedrock.%s.amazonaws.com/foundation-models", region) + // List models endpoint uses the bedrock service (not bedrock-runtime) + endpoint := fmt.Sprintf("https://bedrock.%s.amazonaws.com/foundation-models", region) @@ - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil)core/providers/azure.go (1)
127-130: Docstring/name mismatch.Update comment to match the function name
listModelsByKey.-// listModelsForKey performs a list models request for a single key. +// listModelsByKey performs a list models request for a single key. -// Returns the response and latency, or an error if the request fails. +// Returns the response and latency, or an error if the request fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/bifrost.go(3 hunks)core/providers/anthropic.go(3 hunks)core/providers/azure.go(4 hunks)core/providers/bedrock.go(7 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(2 hunks)core/providers/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(3 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(3 hunks)core/providers/openrouter.go(3 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(1 hunks)core/providers/vertex.go(6 hunks)core/schemas/models.go(3 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(0 hunks)core/schemas/providers/vertex/models.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/providers/gemini/models.go
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/bifrost-http/handlers/server.go
- core/providers/ollama.go
- core/schemas/models.go
- core/schemas/provider.go
- core/providers/groq.go
- core/providers/gemini.go
🧰 Additional context used
🧬 Code graph analysis (13)
core/providers/openrouter.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (2)
BifrostError(322-331)ListModelsRequest(82-82)core/schemas/provider.go (1)
Provider(197-224)
core/providers/parasail.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/vertex.go (5)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)ErrorField(340-347)ListModelsRequest(82-82)core/schemas/providers/vertex/types.go (1)
MaxPageSize(50-50)core/schemas/provider.go (3)
ErrProviderRequestTimedOut(22-22)ErrProviderRequest(24-24)Provider(197-224)
core/providers/cerebras.go (1)
core/schemas/account.go (1)
Key(8-17)
core/providers/sgl.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (1)
BifrostListModelsRequest(23-34)
core/providers/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)Azure(36-36)ListModelsRequest(82-82)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/cohere.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Cohere(39-39)ListModelsRequest(82-82)core/schemas/providers/cohere/types.go (2)
CohereError(521-525)CohereListModelsResponse(540-543)core/schemas/provider.go (1)
Provider(197-224)
core/providers/utils.go (4)
core/schemas/models.go (3)
BifrostListModelsResponse(36-45)Model(47-64)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
ModelProvider(32-32)BifrostError(322-331)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)ListModelsRequest(82-82)core/schemas/logger.go (1)
Logger(28-55)core/schemas/provider.go (1)
Provider(197-224)
core/providers/anthropic.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)core/schemas/providers/anthropic/types.go (2)
AnthropicError(303-309)AnthropicListModelsResponse(281-286)core/schemas/provider.go (1)
Provider(197-224)
core/providers/openai.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)OpenAI(35-35)ListModelsRequest(82-82)ModelProvider(32-32)core/schemas/providers/openai/types.go (1)
OpenAIListModelsResponse(140-143)core/schemas/logger.go (1)
Logger(28-55)
core/providers/mistral.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/providers/mistral/types.go (1)
MistralListModelsResponse(31-34)core/schemas/provider.go (1)
Provider(197-224)
core/bifrost.go (3)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/bifrost.go (4)
BifrostError(322-331)ErrorField(340-347)ModelProvider(32-32)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
core/providers/bedrock.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
🔇 Additional comments (10)
core/schemas/providers/cohere/models.go (2)
3-3: Nil-guard LGTM; confirm pagination shift is intentional.Dropping NextPageToken here implies pagination is now solely handled at the aggregator after multi-key merge. If that's the plan, all good; otherwise we may lose provider-native pagination signals.
Would you confirm that ListModels pagination is exclusively applied post-aggregation and that provider-level page tokens are no longer consumed?
14-21: No action needed—types already match.Verification confirms both fields are
[]string:model.Endpoints(from cohere/types.go:532) andschemas.Model.SupportedMethods(from schemas/models.go:63). The assignment is type-safe and correct as written.core/providers/parasail.go (1)
70-82: Multi‑key delegation looks correct.Passing keys to handleOpenAIListModelsRequest aligns with the new flow. No issues spotted.
Please confirm the shared handler performs deduplication + ApplyPagination and sets ExtraFields (Provider, RequestType, Latency).
core/providers/sgl.go (1)
72-84: Good handoff to shared multi‑key path.Signature and call-site updates look consistent with the new contract.
core/providers/cerebras.go (1)
65-67: Signature/call-site update LGTM.Forwarding keys into the shared handler is consistent. No functional concerns here.
core/providers/mistral.go (2)
157-163: ExtraFields population is correct.Provider, RequestType, and aggregated Latency set after pagination — good ordering.
131-156: No issues found—code type signatures are compatible.The function
extractSuccessfulListModelsResponsesreturns([]*schemas.BifrostListModelsResponse, *schemas.BifrostError), which matches exactly whataggregateListModelsResponsesexpects. The code will compile correctly.Likely an incorrect or invalid review comment.
core/schemas/providers/vertex/models.go (1)
16-20: No action needed — code is already compliant.The
schemas.Model.Createdfield is defined as*int64, and it is documented as a Unix timestamp in seconds. Verification across all providers shows consistent use of seconds:
- Vertex and Anthropic correctly use
.Unix()(returns seconds)- Azure, Mistral, and OpenAI pass already-converted int64 values in seconds
- No provider uses
UnixMilli()or milliseconds anywhereThe code already aligns across all providers with no gaps.
core/providers/utils.go (1)
672-720: Aggregator looks good.Dedupe + raw-response aggregation is correct; final latency and metadata are set by callers. No change needed.
core/providers/openai.go (1)
152-196: Multi‑key ListModels orchestration looks solid.Concurrent per‑key calls, safe channel usage, proper aggregation/pagination, and overall latency recorded.
TejasGhatte
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
9114542 to
6093e1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
core/providers/vertex.go (1)
154-156: Fix Vertex global endpoint host construction.When region == "global", the host must be aiplatform.googleapis.com (not global-aiplatform.googleapis.com). Incorrect host breaks requests.
- requestURL := fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1/projects/%s/locations/%s/models?pageSize=%d", region, projectID, region, vertex.MaxPageSize) + var host string + if region == "global" { + host = "aiplatform.googleapis.com" + } else { + host = fmt.Sprintf("%s-aiplatform.googleapis.com", region) + } + requestURL := fmt.Sprintf("https://%s/v1/projects/%s/locations/%s/models?pageSize=%d", host, projectID, region, vertex.MaxPageSize)core/providers/bedrock.go (1)
463-463: Remove zero-latency assignment (or compute correctly).
time.Since(time.Now())is effectively 0 and misleading. Either remove the per-key latency here (overall latency is set in ListModels) or record it properly.Apply minimal fix:
- response.ExtraFields.Latency = time.Since(time.Now()).Milliseconds() + // Per-key latency not tracked here; overall latency is set in ListModels.
🧹 Nitpick comments (18)
core/schemas/models.go (3)
17-21: Consider adding per-key latency to ListModelsByKeyResult.You rely on response.ExtraFields.Latency. Adding a Latency field here simplifies logging/metrics when Err != nil and Response is nil.
130-146: Invalid page tokens silently reset to start. Consider trace logging hook.decodePaginationCursor returns an empty cursor on errors, and ApplyPagination resets offset to 0. That’s user-friendly, but you may want a (debug-level) log upstream to aid ops visibility for tampered/buggy tokens.
174-234: Cap page size to a safe upper bound.Prevent overly large responses by clamping pageSize to DefaultPageSize while preserving current behavior for non-positive values.
Apply this diff:
func (response *BifrostListModelsResponse) ApplyPagination(pageSize int, pageToken string) *BifrostListModelsResponse { @@ if pageSize <= 0 { return response } + // Cap page size to a safe upper bound + if pageSize > DefaultPageSize { + pageSize = DefaultPageSize + } @@ }core/providers/mistral.go (5)
32-36: Normalize MaxConnsPerHost to BufferSize (consistency).Other providers use BufferSize here. Using Concurrency may under-provision sockets.
Apply this diff:
- MaxConnsPerHost: config.ConcurrencyAndBufferSize.Concurrency, + MaxConnsPerHost: config.ConcurrencyAndBufferSize.BufferSize,
85-89: Unnecessary Content-Type on GET.No body is sent; Content-Type is redundant. Safe to drop.
Apply this diff:
req.Header.SetMethod(http.MethodGet) -req.Header.SetContentType("application/json") req.Header.Set("Authorization", "Bearer "+key.Value)
134-143: Bound concurrency to avoid socket/remote saturation.Spawning one goroutine per key can overwhelm hosts with many keys. Add a small worker semaphore.
Apply this diff:
- // Launch concurrent requests for all keys - for _, key := range keys { - wg.Add(1) - go func(k schemas.Key) { - defer wg.Done() - resp, _, bifrostErr := provider.listModelsByKey(ctx, k) - results <- schemas.ListModelsByKeyResult{Response: resp, Err: bifrostErr, KeyID: k.ID} - }(key) - } + // Launch concurrent requests with a bounded worker pool + maxWorkers := 16 + if len(keys) < maxWorkers { + maxWorkers = len(keys) + } + sem := make(chan struct{}, maxWorkers) + for _, key := range keys { + wg.Add(1) + go func(k schemas.Key) { + sem <- struct{}{} + defer func() { <-sem; wg.Done() }() + resp, _, bifrostErr := provider.listModelsByKey(ctx, k) + results <- schemas.ListModelsByKeyResult{Response: resp, Err: bifrostErr, KeyID: k.ID} + }(key) + }
153-162: Nil-safety before pagination.If aggregation returns nil unexpectedly, ApplyPagination would panic. Guard and continue.
Apply this diff:
- // Aggregate all successful responses - response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + // Aggregate all successful responses + response := aggregateListModelsResponses(successfulResponses) + if response == nil { + response = &schemas.BifrostListModelsResponse{Data: []schemas.Model{}} + } + response = response.ApplyPagination(request.PageSize, request.PageToken)
131-151: Add early return if no keys provided.Defensive check helps produce a clear error instead of flowing to empty aggregation.
Example:
if len(keys) == 0 { return nil, newProviderRequestError("mistral", "no keys provided") }If a canonical error helper exists, prefer it.
core/providers/gemini.go (2)
132-146: Return a clear error when no keys are provided.Short‑circuit for len(keys) == 0 to avoid ambiguous aggregation errors.
func (provider *GeminiProvider) ListModels(ctx context.Context, keys []schemas.Key, request *schemas.BifrostListModelsRequest) (*schemas.BifrostListModelsResponse, *schemas.BifrostError) { if err := checkOperationAllowed(schemas.Gemini, provider.customProviderConfig, schemas.ListModelsRequest); err != nil { return nil, err } startTime := time.Now() providerName := provider.GetProviderKey() + + if len(keys) == 0 { + return nil, newBifrostOperationError("no API keys configured for provider", nil, providerName) + } results := make(chan schemas.ListModelsByKeyResult, len(keys)) var wg sync.WaitGroup
89-95: Optional: honor requested page size to reduce over‑fetch.If request.PageSize is set smaller than DefaultPageSize, consider passing it to Gemini’s pageSize query to lower payloads. Would require threading page size into listModelsByKey or adjusting the helper’s signature.
core/providers/openrouter.go (1)
122-129: Guard against empty key set.Return a clear error when no keys are supplied.
func (provider *OpenRouterProvider) ListModels(ctx context.Context, keys []schemas.Key, request *schemas.BifrostListModelsRequest) (*schemas.BifrostListModelsResponse, *schemas.BifrostError) { startTime := time.Now() providerName := provider.GetProviderKey() + + if len(keys) == 0 { + return nil, newBifrostOperationError("no API keys provided", nil, providerName) + } results := make(chan schemas.ListModelsByKeyResult, len(keys)) var wg sync.WaitGroupcore/providers/anthropic.go (2)
257-265: Return early when keys are empty.Avoid running the fan‑out when len(keys) == 0; return a precise error.
startTime := time.Now() providerName := provider.GetProviderKey() + + if len(keys) == 0 { + return nil, newBifrostOperationError("no API keys configured for provider", nil, providerName) + } results := make(chan schemas.ListModelsByKeyResult, len(keys)) var wg sync.WaitGroup
205-211: Optional: reduce over‑fetch by using requested page size in limit.If a smaller request.PageSize is specified, consider using min(DefaultPageSize, request.PageSize) for the limit query to reduce response sizes. This would require passing page size into listModelsByKey or reading it from request.
core/providers/cohere.go (2)
161-169: Add an early exit for empty key slices.Short‑circuit and return a clear error when len(keys) == 0.
startTime := time.Now() providerName := provider.GetProviderKey() + + if len(keys) == 0 { + return nil, newBifrostOperationError("no API keys configured for provider", nil, providerName) + } results := make(chan schemas.ListModelsByKeyResult, len(keys)) var wg sync.WaitGroup
112-118: Optional: respect requested PageSize in the per‑key URL.If request.PageSize is smaller than DefaultPageSize, use it (clamped to provider max) in page_size to reduce payload. Would require threading PageSize into listModelsByKey.
core/providers/vertex.go (1)
279-287: Early return when no keys are supplied.Provide a clear configuration error instead of proceeding with an empty fan‑out.
startTime := time.Now() providerName := provider.GetProviderKey() + + if len(keys) == 0 { + return nil, newConfigurationError("no API keys configured for Vertex", providerName) + } results := make(chan schemas.ListModelsByKeyResult, len(keys)) var wg sync.WaitGroupcore/providers/azure.go (1)
210-246: Aggregation + pagination + latency are implemented correctly.Concurrent fan‑out across keys with best‑effort semantics is good. Optional: consider bounding goroutine fan‑out when key count is very large.
Example pattern:
- for _, key := range keys { + sem := make(chan struct{}, provider.client.MaxConnsPerHost) + for _, key := range keys { + sem <- struct{}{} wg.Add(1) go func(k schemas.Key) { defer wg.Done() + defer func(){ <-sem }() resp, _, bifrostErr := provider.listModelsByKey(ctx, k) results <- schemas.ListModelsByKeyResult{Response: resp, Err: bifrostErr, KeyID: k.ID} }(key) }core/providers/openai.go (1)
152-196: Concurrent aggregation and final response metadata look good.Best‑effort merge with dedupe + pagination + overall latency is implemented correctly. Optional: bound goroutine fan‑out if key counts can spike.
Pattern:
- for _, key := range keys { + sem := make(chan struct{}, client.MaxConnsPerHost) + for _, key := range keys { + sem <- struct{}{} wg.Add(1) go func(k schemas.Key) { defer wg.Done() + defer func(){ <-sem }() resp, _, bifrostErr := listModelsByKeyOpenAI(ctx, client, url, k, extraHeaders, providerName, sendBackRawResponse) results <- schemas.ListModelsByKeyResult{Response: resp, Err: bifrostErr, KeyID: k.ID} }(key) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/bifrost.go(3 hunks)core/providers/anthropic.go(3 hunks)core/providers/azure.go(4 hunks)core/providers/bedrock.go(7 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(2 hunks)core/providers/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(3 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(3 hunks)core/providers/openrouter.go(3 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(2 hunks)core/providers/vertex.go(6 hunks)core/schemas/models.go(3 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(0 hunks)core/schemas/providers/vertex/models.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/providers/gemini/models.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/providers/ollama.go
- core/schemas/provider.go
- core/providers/groq.go
- core/providers/cerebras.go
- core/providers/utils.go
- transports/bifrost-http/handlers/server.go
- core/schemas/providers/cohere/models.go
🧰 Additional context used
🧬 Code graph analysis (13)
core/providers/openrouter.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/provider.go (1)
Provider(197-224)
core/providers/bedrock.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/vertex.go (5)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)ErrorField(340-347)ListModelsRequest(82-82)core/schemas/providers/vertex/types.go (1)
MaxPageSize(50-50)core/schemas/provider.go (1)
Provider(197-224)
core/providers/sgl.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/parasail.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/schemas/models.go (1)
core/schemas/bifrost.go (2)
BifrostError(322-331)BifrostResponseExtraFields(254-262)
core/providers/azure.go (3)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/provider.go (1)
Provider(197-224)
core/providers/mistral.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)ListModelsRequest(82-82)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/gemini.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/providers/gemini/types.go (1)
GeminiListModelsResponse(1272-1275)core/schemas/provider.go (1)
Provider(197-224)
core/providers/openai.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)OpenAI(35-35)ListModelsRequest(82-82)ModelProvider(32-32)core/schemas/providers/openai/types.go (1)
OpenAIListModelsResponse(140-143)core/schemas/logger.go (1)
Logger(28-55)
core/bifrost.go (3)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/bifrost.go (3)
BifrostError(322-331)ModelProvider(32-32)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
core/providers/anthropic.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)core/schemas/providers/anthropic/types.go (2)
AnthropicError(303-309)AnthropicListModelsResponse(281-286)core/schemas/provider.go (1)
Provider(197-224)
core/providers/cohere.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Cohere(39-39)ListModelsRequest(82-82)core/schemas/providers/cohere/types.go (2)
CohereError(521-525)CohereListModelsResponse(540-543)core/schemas/provider.go (1)
Provider(197-224)
🔇 Additional comments (10)
core/schemas/providers/vertex/models.go (1)
10-23: Solid adapter; matches new aggregation flow.Nil receiver guard, pre-sized slice, and Created from Unix() look good. Omitting NextPageToken is correct since pagination happens post-aggregation.
core/providers/parasail.go (1)
69-82: LGTM; matches the new multi-key ListModels contract.Keys slice and logger are correctly passed through to the shared handler.
core/schemas/models.go (2)
14-14: No action required — constant is actively used.The verification confirms that
MaxPaginationRequestsis referenced incore/bifrost.go:319to enforce pagination request limits and is logged in the warning message on line 320. The constant is properly utilized and should not be removed.Likely an incorrect or invalid review comment.
51-51: No issues found—all providers correctly use Unix seconds.Mistral's
createdfield and Azure'screated_atfield are both Unix epoch timestamps in seconds. The code correctly handles this:
- Vertex and Anthropic receive time objects and convert via
.Unix()to seconds- Mistral and Azure receive
int64values already in seconds and use them directlyAll providers consistently set the
Createdfield as Unix seconds as intended.core/providers/sgl.go (1)
71-84: All call sites verified and correctly updated.All 6 invocations of handleOpenAIListModelsRequest (groq, sgl, parasail, ollama, cerebras, openai) consistently pass both
keysandprovider.loggerdownstream, confirming uniform multi-key design and interface alignment across all providers.core/providers/mistral.go (1)
148-151: Helpers verified correct, but test coverage gaps exist.Both helpers are properly implemented:
aggregateListModelsResponsescorrectly dedupes by Model.ID and maintains first occurrence ordering.extractSuccessfulListModelsResponsessurfaces a BifrostError when all keys fail.ApplyPaginationis called after aggregation, preserving dedup results.However, the requested test scenarios are not covered:
- No unit tests exist for the helper functions.
- Tests do not cover mixed success/failure keys, fully failing scenarios, or deduplication verification.
- Mistral specifically has
ListModels: falsein tests, so even pagination tests are disabled for this provider.Add unit tests covering: mixed key outcomes, all-keys-fail scenarios, dedup by ID verification, and pagination interaction with aggregation.
core/providers/bedrock.go (1)
472-516: Multi-key aggregation, pagination, and ExtraFields look solid.Concurrent per-key calls, best-effort aggregation, and final latency on the combined response are implemented correctly.
core/providers/azure.go (1)
149-205: Per-key request flow is correct and leak-safe.Good use of Acquire/Release, auth handling (Bearer vs api-key), error mapping, and latency on the response.
core/providers/openai.go (1)
93-150: Per-key request is clean and efficient.Correct Acquire/Release, auth/header setup, error mapping, body copy before release, and per-key latency on response.
core/bifrost.go (1)
2227-2234: Remove nil interface panic concern; fix empty direct key validation; remove inapplicable nil guard suggestion.The review comment contains misguided technical concerns. Analysis:
Nil interface panic risk: Invalid. In Go,
ctxas acontext.Contextvalue parameter cannot be nil in a way that causes panic on(*ctx).Value(). The pointer checkif ctx != nilat line 2228 is defensive but the underlying concern is unfounded.Empty direct key bypass: Valid. The code at lines 2229–2232 returns a direct key from context without validating emptiness, while bulk keys retrieved at lines 2248–2250 are filtered to exclude empty values. This inconsistency can permit invalid keys and cause confusing provider errors. This concern should be preserved and fixed.
ListModelsRequest nil guard: Inapplicable. The function signature shows
ctx context.Context(value parameter, not pointer). The suggested nil-guard patternif ctx == nil { ctx = bifrost.ctx }does not match this signature and is unnecessary for value parameters. The pattern shown at lines 1440–1441 and 1513–1514 applies in different contexts with different signatures.Fix the empty direct key validation inconsistency at lines 2229–2232:
- if ok { - // If a direct key is specified, return it as a single-element slice - return []schemas.Key{key}, nil - } + if ok { + // Validate direct key is not empty (consistent with bulk key filtering) + if strings.TrimSpace(key.Value) == "" && !canProviderKeyValueBeEmpty(baseProviderType) { + return nil, fmt.Errorf("direct key is empty for provider: %v", providerKey) + } + // If a direct key is specified, return it as a single-element slice + return []schemas.Key{key}, nil + }Likely an incorrect or invalid review comment.
TejasGhatte
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
6093e1c to
d132a4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/providers/bedrock.go (1)
463-463: Remove per-key latency; aggregate response already measures correctly.The
time.Since(time.Now())pattern at line 463 inlistModelsByKey()always returns ~0ms. This is unnecessary since the parentListModels()function capturesstartTimeat line 489 and correctly sets aggregate latency at line 513 using measured time. Per-key latency in concurrent calls is misleading; remove the assignment.- response.ExtraFields.Latency = time.Since(time.Now()).Milliseconds()core/bifrost.go (1)
2225-2231: Add defensive nil interface check for context dereferencing.Per previous review discussion, while the pointer
ctxwon't benil, the dereferenced interface*ctxcould benil, causing a panic when calling(*ctx).Value(...). AlthoughListModelsRequestnow checks for nil context at line 214-216, adding a defensive check here makesgetAllSupportedKeysmore robust if called from other paths.Apply this diff:
func (bifrost *Bifrost) getAllSupportedKeys(ctx *context.Context, providerKey schemas.ModelProvider, baseProviderType schemas.ModelProvider) ([]schemas.Key, error) { // Check if key has been set in the context explicitly - if ctx != nil { + if ctx != nil && *ctx != nil { key, ok := (*ctx).Value(schemas.BifrostContextKeyDirectKey).(schemas.Key) if ok { // If a direct key is specified, return it as a single-element slice return []schemas.Key{key}, nil } }Based on learnings
🧹 Nitpick comments (3)
core/providers/openrouter.go (1)
119-156: Add nil-guard for request before pagination.While the interface likely guarantees a non-nil
request, defensive programming suggests guarding against NPE at line 148.Apply this diff:
response := aggregateListModelsResponses(successfulResponses) -response = response.ApplyPagination(request.PageSize, request.PageToken) +if request != nil { + response = response.ApplyPagination(request.PageSize, request.PageToken) +}Alternatively, if the contract guarantees non-nil, document that assumption.
Based on learnings
core/providers/utils.go (1)
673-726: Consider NextPageToken propagation for consistency.The aggregator deduplicates models and collects raw responses but doesn't propagate
NextPageToken. If multiple keys return consistent pagination tokens, consider surfacing it in the aggregated response. If tokens are inconsistent or not supported for multi-key operations, this is acceptable—just ensure it's intentional.Optional enhancement:
// Aggregate all models with deduplication, and collect raw responses var rawResponses []interface{} +var firstToken string +tokens := make(map[string]struct{}) for _, response := range responses { if response == nil { continue } // Add models, skipping duplicates based on ID for _, model := range response.Data { if _, exists := seenIDs[model.ID]; !exists { seenIDs[model.ID] = struct{}{} aggregated.Data = append(aggregated.Data, model) } } // Collect raw response if present if response.ExtraFields.RawResponse != nil { rawResponses = append(rawResponses, response.ExtraFields.RawResponse) } + + if t := response.NextPageToken; t != "" { + if firstToken == "" { + firstToken = t + } + tokens[t] = struct{}{} + } } // Sort models alphabetically by ID sort.Slice(aggregated.Data, func(i, j int) bool { return aggregated.Data[i].ID < aggregated.Data[j].ID }) if len(rawResponses) > 0 { aggregated.ExtraFields.RawResponse = rawResponses } + +// Only propagate token if all responses agree +if firstToken != "" && len(tokens) == 1 { + aggregated.NextPageToken = firstToken +} return aggregatedcore/providers/mistral.go (1)
125-164: Add nil-guard for request before pagination.Similar to OpenRouter, defensive programming suggests guarding against potential NPE at line 155.
Apply this diff:
// Aggregate all successful responses response := aggregateListModelsResponses(successfulResponses) -response = response.ApplyPagination(request.PageSize, request.PageToken) +if request != nil { + response = response.ApplyPagination(request.PageSize, request.PageToken) +} // Set ExtraFields latency := time.Since(startTime)Alternatively, document the non-nil contract assumption.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/bifrost.go(4 hunks)core/providers/anthropic.go(3 hunks)core/providers/azure.go(4 hunks)core/providers/bedrock.go(7 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(2 hunks)core/providers/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(3 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(3 hunks)core/providers/openrouter.go(3 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(2 hunks)core/providers/vertex.go(6 hunks)core/schemas/models.go(3 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(0 hunks)core/schemas/providers/vertex/models.go(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/providers/gemini/models.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/providers/cohere/models.go
- core/providers/groq.go
- core/schemas/models.go
- core/schemas/providers/vertex/models.go
🧰 Additional context used
🧬 Code graph analysis (17)
core/schemas/provider.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)
core/providers/ollama.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)
core/providers/utils.go (3)
core/schemas/models.go (3)
BifrostListModelsResponse(36-45)Model(47-64)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
ModelProvider(32-32)BifrostError(322-331)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)ListModelsRequest(82-82)core/schemas/logger.go (1)
Logger(28-55)
core/providers/cerebras.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (1)
BifrostListModelsRequest(23-34)
core/providers/vertex.go (5)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)ErrorField(340-347)ListModelsRequest(82-82)core/schemas/providers/vertex/types.go (1)
MaxPageSize(50-50)core/schemas/provider.go (1)
Provider(197-224)
core/providers/cohere.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/providers/cohere/types.go (2)
CohereError(521-525)CohereListModelsResponse(540-543)core/schemas/provider.go (1)
Provider(197-224)
core/providers/parasail.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (1)
BifrostListModelsRequest(23-34)
transports/bifrost-http/handlers/server.go (1)
plugins/maxim/main.go (1)
Init(62-92)
core/providers/sgl.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/anthropic.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)core/schemas/providers/anthropic/types.go (2)
AnthropicError(303-309)AnthropicListModelsResponse(281-286)core/schemas/provider.go (1)
Provider(197-224)
core/providers/gemini.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Gemini(47-47)ListModelsRequest(82-82)core/schemas/providers/gemini/types.go (1)
GeminiListModelsResponse(1272-1275)core/schemas/provider.go (1)
Provider(197-224)
core/bifrost.go (3)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/bifrost.go (3)
BifrostError(322-331)ModelProvider(32-32)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
core/providers/openrouter.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)ListModelsRequest(82-82)OpenRouter(48-48)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/bedrock.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)core/schemas/provider.go (1)
Provider(197-224)
core/providers/azure.go (4)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Azure(36-36)ListModelsRequest(82-82)core/schemas/provider.go (1)
Provider(197-224)
core/providers/openai.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)OpenAI(35-35)ListModelsRequest(82-82)ModelProvider(32-32)core/schemas/providers/openai/types.go (1)
OpenAIListModelsResponse(140-143)core/schemas/logger.go (1)
Logger(28-55)
core/providers/mistral.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/providers/mistral/types.go (1)
MistralListModelsResponse(31-34)core/schemas/provider.go (1)
Provider(197-224)
🔇 Additional comments (24)
core/schemas/provider.go (1)
201-201: Interface update aligns with multi-key aggregation goal.The signature change from a single
keytokeys []Keyis a clean interface evolution that enables the multi-key aggregation feature across all providers.core/providers/ollama.go (1)
72-76: LGTM! Clean delegation to shared handler.The signature update and parameter forwarding correctly implement the multi-key interface.
core/providers/sgl.go (1)
72-83: LGTM! Consistent with other OpenAI-compatible providers.The multi-key signature and delegation follow the established pattern.
core/providers/cerebras.go (1)
65-66: LGTM! Follows the established pattern.Clean delegation to the shared OpenAI-compatible handler.
core/providers/parasail.go (1)
70-81: LGTM! Standard multi-key delegation.Consistent with the other OpenAI-compatible provider implementations.
core/providers/openrouter.go (1)
65-117: LGTM! Well-structured per-key helper.The helper correctly:
- Tracks per-key latency
- Copies response body before resource release
- Handles errors with latency propagation
- Prefixes model IDs with provider key
core/providers/utils.go (1)
728-766: LGTM! Correct partial-success handling.The function correctly:
- Tracks the last error for reporting when all keys fail
- Returns successful responses with
nilerror on partial success- Provides clear error messages with provider context
core/providers/mistral.go (1)
71-123: LGTM! Well-implemented per-key helper.The helper correctly:
- Tracks and returns per-key latency
- Copies response body before resource cleanup
- Handles errors with latency propagation
- Parses Mistral-specific response format
core/providers/gemini.go (2)
75-123: Per‑key helper is clean and correct.Good fasthttp usage, error handling, and raw response handling. Latency is captured and returned.
132-166: Concurrent aggregation and shaping look solid.Correct goroutine orchestration, channel closing, aggregation, pagination, and ExtraFields population.
core/providers/vertex.go (2)
154-162: Global host handling: LGTM.Correctly switches to aiplatform.googleapis.com for region == "global".
282-319: Multi‑key aggregation: LGTM.Concurrency, result collection, aggregation, pagination, and latency are implemented correctly.
core/providers/cohere.go (2)
98-152: Per‑key list implementation: LGTM.Good request construction, error parsing, body copy safety, and latency capture.
154-196: Aggregation flow: LGTM.Concurrency, collection, aggregation, pagination, and latency are correct.
core/providers/anthropic.go (2)
191-246: Per‑key list path: LGTM.Correct GET setup, error conversion, response mapping, and latency propagation.
248-292: Concurrent multi‑key aggregation: LGTM.Pattern matches other providers; looks correct and robust.
core/providers/azure.go (2)
127-205: Per‑key model listing: LGTM.Auth (api-key/Bearer) selection, error parsing via parseOpenAIError, and body copy safety are handled well.
207-246: Multi‑key orchestration and pagination: LGTM.Concurrency, aggregation, pagination, and latency fields are correctly applied.
core/bifrost.go (3)
214-216: LGTM! Nil context check now consistent with other request handlers.This addition aligns with the pattern used in
handleRequestandhandleStreamRequest, preventing potential blocking if a nil context is passed.
264-270: LGTM! Multi-key ListModels integration implemented correctly.The change from
selectKeyFromProviderForModeltogetAllSupportedKeysenables multi-key aggregation as intended. Error handling withnewBifrostErroris appropriate.
292-292: LGTM! Better error propagation.Using
err.Error()instead of a hardcoded string provides more helpful diagnostic information.core/providers/openai.go (3)
16-16: LGTM! Multi-key ListModels signature and sync import.The updated
ListModelssignature accepting[]schemas.Keyaligns with the Provider interface changes. Thesyncimport is necessary for the concurrent orchestration inhandleOpenAIListModelsRequest.Also applies to: 81-89
91-150: LGTM! Well-structured per-key request handler.The
listModelsByKeyOpenAIfunction properly:
- Manages resources with defer statements
- Copies the response body before releasing (line 132) to prevent use-after-release
- Tracks per-key latency
- Handles errors at each step
The function returns a three-tuple
(response, latency, error)that enables the aggregation logic to track per-key performance.
152-196: LGTM! Concurrent multi-key orchestration implemented correctly.The
handleOpenAIListModelsRequestfunction properly:
- Launches concurrent requests for all keys with proper WaitGroup synchronization
- Sizes the results channel to
len(keys)to prevent goroutine blocking- Extracts successful responses with best-effort error handling
- Aggregates results and applies pagination
- Tracks overall latency from start to finish (line 190)
Note: Per-key latency from
listModelsByKeyOpenAIis intentionally discarded (line 172) in favor of the overall request latency, which is more meaningful for the aggregated response.
TejasGhatte
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
d132a4c to
b97af11
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
transports/bifrost-http/handlers/server.go (1)
203-203: CRITICAL: Function signature mismatch persists.This issue was previously flagged: the
maxim.Initcall passes only one parameter (maximConfig), but the function signature inplugins/maxim/main.go:61still requires two parameters:func Init(config *Config, logger schemas.Logger).The AI summary claims the signature was updated, but the relevant code snippets show the two-parameter signature still exists. This will cause a compilation error.
Additionally, this change appears unrelated to the PR's objective (listing models with multiple keys). Consider whether this modification belongs in a separate PR.
Verify the actual
maxim.Initsignature to confirm whether it was updated:#!/bin/bash # Description: Verify the current signature of maxim.Init echo "=== Current maxim.Init signature ===" ast-grep --pattern $'func Init($$$) ($$$) { $$$ }' echo -e "\n=== All maxim.Init call sites ===" rg -nP '\bmaxim\.Init\s*\(' --type=go -C2core/providers/bedrock.go (1)
506-509: Prevent panic when ListModels is called with a nil request.The providers previously accepted a
nilrequest pointer; dereferencingrequest.PageSize/request.PageTokenhere will now panic in that case. Please default the pagination parameters whenrequestisnilbefore invokingApplyPagination.- // Aggregate all successful responses - response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + // Aggregate all successful responses + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)core/providers/vertex.go (1)
307-309: Guard pagination against nil requests.If a caller invokes
Provider.ListModelswithrequest == nil, the current code will dereference a nil pointer and panic. Please defaultpageSize/pageTokenwhenrequestis nil before callingApplyPagination.- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)core/providers/cohere.go (1)
187-189: Handle nil requests before pagination.This code path will panic when
request == nil, even though theProvider.ListModelscontract allows callers to omit the request (and the old implementation tolerated that). Please computepageSize/pageTokendefensively before callingApplyPagination.- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)core/providers/azure.go (1)
235-238: Guard against nil requests before pagination.
Provider.ListModelsis part of the public interface and callers have historically been able to passnilto get default pagination. Dereferencingrequest.PageSize/request.PageTokenwill panic in that scenario. Please computepageSize/pageTokendefensively (defaulting to the schema defaults whenrequest == nil) before callingApplyPagination.- response := aggregateListModelsResponses(successfulResponses) - response = response.ApplyPagination(request.PageSize, request.PageToken) + response := aggregateListModelsResponses(successfulResponses) + pageSize, pageToken := schemas.DefaultPageSize, "" + if request != nil { + if request.PageSize > 0 { + pageSize = request.PageSize + } + pageToken = request.PageToken + } + response = response.ApplyPagination(pageSize, pageToken)core/bifrost.go (1)
2324-2332: Guard against typed-nil context before dereferencing.This still panics if
*ctxis a typed-nil interface (e.g. Bifrost initialized with a nil context):(*ctx).Value(...)dereferences a nil value. The nil guard added inListModelsRequestis not sufficient becausebifrost.ctxcan itself be nil. Please ensure*ctx != nilbefore callingValue, and reject empty direct keys for providers that require them to prevent confusing failures.- if ctx != nil { - key, ok := (*ctx).Value(schemas.BifrostContextKeyDirectKey).(schemas.Key) + if ctx != nil && *ctx != nil { + key, ok := (*ctx).Value(schemas.BifrostContextKeyDirectKey).(schemas.Key) if ok { - // If a direct key is specified, return it as a single-element slice + if providerRequiresKey(baseProviderType) && strings.TrimSpace(key.Value) == "" { + return nil, fmt.Errorf("direct key is empty for provider: %v", providerKey) + } + // If a direct key is specified, return it as a single-element slice return []schemas.Key{key}, nil } }
🧹 Nitpick comments (8)
core/schemas/models.go (2)
127-131: Document token format explicitly (URL-safe base64 without padding).You’re using base64.RawURLEncoding, which produces URL-safe tokens without “=” padding. Add this to the comment to avoid confusion and keep docs consistent.
-// Use URL-safe base64 encoding without padding for opaque token +// Tokens use URL-safe base64 encoding without padding (base64.RawURLEncoding). Treat as opaque.
157-175: Simplify cursor validation; remove unreachable branch.Given prior bounds checks, prevIndex is always within range. Streamline for clarity.
func validatePaginationCursor(cursor paginationCursor, data []Model) bool { if cursor.LastID == "" { return true } if cursor.Offset <= 0 || cursor.Offset > len(data) { return false } - prevIndex := cursor.Offset - 1 - if prevIndex >= 0 && prevIndex < len(data) { - return data[prevIndex].ID == cursor.LastID - } - - return true + prevIndex := cursor.Offset - 1 + return data[prevIndex].ID == cursor.LastID }docs/apis/openapi.json (3)
6573-6577: Make next_page_token example/description match server encoding.Server encodes tokens as URL-safe base64 without padding. Mark token as opaque and update example to a padding‑less variant to avoid confusion.
- "example": "eyJwYWdlIjoxfQ==" + "description": "Token to retrieve the next page of results. Opaque, URL-safe base64 without padding.", + "example": "eyJvIjoyLCJsIjoiZ3B0LTRvIn0"
293-299: Document multi‑key aggregation semantics.Add that models are aggregated across all configured keys for the provider, deduped by model ID; partial key failures are tolerated (best‑effort); ordering is deterministic for pagination.
312-320: Specify defaults and bounds for page_size.State default (e.g., 1000) and a reasonable maximum to protect the service, matching ApplyPagination behavior.
- "schema": { "type": "integer" } + "schema": { "type": "integer", "default": 1000, "maximum": 1000, "minimum": 1 }core/schemas/providers/gemini/models.go (1)
57-62: Optional: Set canonical slug when available.If Gemini exposes a canonical name distinct from display name, consider setting Model.CanonicalSlug for consistency with other providers.
core/providers/utils.go (1)
716-719: Clarify the ordering contractThe comment above promises to “maintain the original ordering,” but the
sort.Sliceblock enforces alphabetical order instead. Either drop the sort to genuinely preserve first occurrence order or update the docstring to spell out that we now normalize to an alpha-sorted list so future readers understand the intentional behavior.core/providers/openai.go (1)
172-172: Consider capturing per-key latency for observability.The per-key latency returned by
listModelsByKeyOpenAIis currently discarded. While overall latency is tracked (line 190), per-key latency could provide valuable observability insights, such as:
- Identifying slow or problematic keys
- Debugging key-specific performance issues
- Populating per-key metrics for monitoring
If capturing per-key latency would be valuable, consider modifying
schemas.ListModelsByKeyResultto include aLatencyfield and propagate it through the results channel.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
core/bifrost.go(4 hunks)core/providers/anthropic.go(3 hunks)core/providers/azure.go(4 hunks)core/providers/bedrock.go(7 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(2 hunks)core/providers/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(3 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(3 hunks)core/providers/openrouter.go(3 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(2 hunks)core/providers/vertex.go(2 hunks)core/schemas/models.go(3 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(2 hunks)core/schemas/providers/vertex/models.go(1 hunks)docs/apis/openapi.json(1 hunks)transports/bifrost-http/handlers/server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/groq.go
- core/schemas/provider.go
- core/schemas/providers/cohere/models.go
🧰 Additional context used
🧬 Code graph analysis (17)
core/providers/cerebras.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/sgl.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/ollama.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)
core/providers/mistral.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/providers/mistral/types.go (1)
MistralListModelsResponse(31-34)core/schemas/provider.go (1)
Provider(197-224)
core/providers/anthropic.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)core/schemas/providers/anthropic/types.go (2)
AnthropicError(315-321)AnthropicListModelsResponse(293-298)core/schemas/provider.go (1)
Provider(197-224)
core/providers/bedrock.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)core/schemas/provider.go (1)
Provider(197-224)
core/providers/azure.go (3)
core/schemas/account.go (2)
Key(8-17)AzureKeyConfig(21-25)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/provider.go (1)
Provider(197-224)
transports/bifrost-http/handlers/server.go (1)
plugins/maxim/main.go (1)
Init(62-92)
core/providers/utils.go (3)
core/schemas/models.go (3)
BifrostListModelsResponse(36-45)Model(47-66)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (6)
ModelProvider(32-32)BifrostError(322-331)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)ListModelsRequest(82-82)core/schemas/logger.go (1)
Logger(28-55)
core/providers/openrouter.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)ListModelsRequest(82-82)OpenRouter(48-48)RequestType(79-79)core/schemas/provider.go (1)
Provider(197-224)
core/providers/vertex.go (3)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (3)
BifrostListModelsResponse(36-45)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/providers/vertex/types.go (3)
VertexModel(52-59)MaxPageSize(50-50)VertexListModelsResponse(61-64)
core/schemas/models.go (1)
core/schemas/bifrost.go (2)
BifrostError(322-331)BifrostResponseExtraFields(254-262)
core/providers/openai.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)OpenAI(35-35)ListModelsRequest(82-82)ModelProvider(32-32)core/schemas/providers/openai/types.go (1)
OpenAIListModelsResponse(140-143)core/schemas/logger.go (1)
Logger(28-55)
core/providers/gemini.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (4)
BifrostError(322-331)Gemini(47-47)ListModelsRequest(82-82)RequestType(79-79)core/schemas/providers/gemini/types.go (1)
GeminiListModelsResponse(1272-1275)core/schemas/provider.go (1)
Provider(197-224)
core/providers/parasail.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/cohere.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (4)
BifrostListModelsResponse(36-45)DefaultPageSize(11-11)BifrostListModelsRequest(23-34)ListModelsByKeyResult(17-21)core/schemas/bifrost.go (3)
BifrostError(322-331)Cohere(39-39)ListModelsRequest(82-82)core/schemas/providers/cohere/types.go (2)
CohereError(522-526)CohereListModelsResponse(541-544)core/schemas/provider.go (1)
Provider(197-224)
core/bifrost.go (3)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/bifrost.go (3)
BifrostError(322-331)ModelProvider(32-32)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
🔇 Additional comments (13)
core/schemas/models.go (3)
16-21: Result envelope looks good for per-key fan‑out.Fields are appropriate for aggregation and error capture.
13-15: MaxPaginationRequests is actively enforcing per-provider pagination bounds.The constant is used at
core/bifrost.go:316within the pagination loop for each provider key. Aniterationscounter is incremented for each pagination request and the loop breaks with a warning wheniterations > MaxPaginationRequests. The implementation correctly bounds per-key pagination as intended.
176-236: Default pageSize when <= 0; sorting and deduplication are already handled by callers.The pageSize <= 0 check should default to DefaultPageSize instead of skipping pagination. However, the concern about stable ordering and deduplication is already addressed:
aggregateListModelsResponses()sorts models deterministically by ID (line 716-719 in core/providers/utils.go) and deduplicates by ID before calling ApplyPagination in all call paths. LastID validation will work correctly with this ordering.- if pageSize <= 0 { - return response - } + if pageSize <= 0 { + pageSize = DefaultPageSize + } + if pageSize > DefaultPageSize { + pageSize = DefaultPageSize + }Likely an incorrect or invalid review comment.
docs/apis/openapi.json (1)
6635-6644: LGTM: Expose max_input_tokens and max_output_tokens.Matches core schema changes.
core/providers/sgl.go (1)
71-84: LGTM: Multi‑key ListModels wiring is consistent.Signature and call match the shared handler pattern.
core/providers/cerebras.go (1)
65-67: LGTM: Multi‑key ListModels forwarder.Matches the new handler signature.
core/providers/parasail.go (1)
70-82: LGTM: Multi‑key ListModels wiring.Consistent with other providers.
core/schemas/providers/gemini/models.go (1)
22-31: LGTM: Maps Gemini limits → Bifrost, keeps IDs clean.ContextLength and max token fields are correctly populated.
core/schemas/providers/vertex/models.go (1)
5-24: LGTM: Vertex mapper is safe and minimal.Nil guard + created timestamp mapping look correct.
core/providers/openai.go (4)
16-16: LGTM! Sync package import added for concurrency.The
syncpackage is correctly imported to support the concurrent per-key request pattern usingsync.WaitGroup.
81-89: LGTM! ListModels signature updated for multi-key support.The method signature correctly accepts a slice of keys and delegates to the orchestration handler. The change aligns with the PR objective to aggregate models from multiple API keys.
181-187: Both best-effort and deduplication requirements are correctly implemented.The verification confirms:
Best-effort approach:
extractSuccessfulListModelsResponses(lines 739-747) continues processing on individual key failures and only returns an error when all keys fail (lines 749-763).Deduplication:
aggregateListModelsResponses(lines 689-707) uses a map-based approach (seenIDs) to deduplicate models by ID, keeping the first occurrence of each unique ID.
115-117: Incorrect assumption about empty key handling.Empty keys are already filtered by
getAllSupportedKeys()in core/bifrost.go (lines 2341–2347) before reachinglistModelsByKeyOpenAI(). The function validates thatstrings.TrimSpace(k.Value) != ""for each key (unless the provider allows empty keys), and returns an error if no valid keys remain. The defensive check at lines 115–117 is redundant since only pre-validated keys reach this function.Likely an incorrect or invalid review comment.
Pratham-Mishra04
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
b97af11 to
5fe52fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/bifrost.go (1)
2323-2330: Validate empty direct key for providers that require a key.If a direct key is provided but empty (for providers requiring a key), calls will 401. Reject early.
Apply:
if ctx != nil { key, ok := (*ctx).Value(schemas.BifrostContextKeyDirectKey).(schemas.Key) if ok { - // If a direct key is specified, return it as a single-element slice - return []schemas.Key{key}, nil + // If a direct key is specified, validate and return it + if providerRequiresKey(baseProviderType) && strings.TrimSpace(key.Value) == "" { + return nil, fmt.Errorf("direct key is empty for provider: %v", providerKey) + } + return []schemas.Key{key}, nil } }
🧹 Nitpick comments (3)
core/providers/bedrock.go (1)
652-652: messageID is always empty; consider removing or populating.Currently never set; emitted chunks will have blank IDs. Either derive an ID when available or drop the field.
core/providers/gemini.go (1)
88-93: Optional: copy body before parsing for consistency with other providers.Avoid relying on fasthttp buffer lifetime; mirror the copy pattern used elsewhere.
- rawResponse, bifrostErr := handleProviderResponse(resp.Body(), &geminiResponse, provider.sendBackRawResponse) + responseBody := append([]byte(nil), resp.Body()...) + rawResponse, bifrostErr := handleProviderResponse(responseBody, &geminiResponse, provider.sendBackRawResponse)Also applies to: 106-112
core/providers/utils.go (1)
673-726: Update documentation to reflect actual behavior.The function comment at line 674 states it "sums up latencies across all responses," but the implementation doesn't set latency—the caller (
handleMultipleListModelsRequests) does. Similarly,ProviderandRequestTypefields are set by the caller, not this function.Consider updating the documentation:
-// aggregateListModelsResponses merges multiple BifrostListModelsResponse objects into a single response. -// It concatenates all model arrays, deduplicates based on model ID, sums up latencies across all responses, -// and concatenates raw responses into an array. -// When duplicate IDs are found, the first occurrence is kept to maintain the original ordering. +// aggregateListModelsResponses merges multiple BifrostListModelsResponse objects into a single response. +// It concatenates all model arrays, deduplicates based on model ID, and concatenates raw responses into an array. +// When duplicate IDs are found, the first occurrence is kept. Models are sorted alphabetically by ID. +// Note: ExtraFields (Provider, RequestType, Latency) are not set by this function and must be set by the caller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
core/bifrost.go(5 hunks)core/providers/anthropic.go(5 hunks)core/providers/azure.go(3 hunks)core/providers/bedrock.go(8 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(5 hunks)core/providers/gemini.go(4 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(3 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(4 hunks)core/providers/openrouter.go(3 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(2 hunks)core/providers/vertex.go(2 hunks)core/schemas/models.go(3 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/anthropic/models.go(0 hunks)core/schemas/providers/bedrock/chat.go(3 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(2 hunks)core/schemas/providers/vertex/models.go(1 hunks)docs/apis/openapi.json(1 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/providers/anthropic/models.go
🚧 Files skipped from review as they are similar to previous changes (6)
- core/providers/cerebras.go
- core/schemas/providers/vertex/models.go
- core/schemas/providers/cohere/models.go
- docs/apis/openapi.json
- core/schemas/models.go
- core/providers/ollama.go
🧰 Additional context used
🧬 Code graph analysis (16)
core/providers/parasail.go (2)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)
core/bifrost.go (4)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/models.go (2)
BifrostListModelsResponse(36-45)MaxPaginationRequests(14-14)core/schemas/bifrost.go (3)
BifrostError(322-331)ModelProvider(32-32)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
core/providers/anthropic.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)DefaultPageSize(11-11)core/schemas/bifrost.go (4)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)ModelProvider(32-32)core/schemas/providers/anthropic/types.go (1)
AnthropicError(315-321)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(25-25)
core/providers/utils.go (5)
core/schemas/models.go (4)
BifrostListModelsResponse(36-45)Model(47-66)ListModelsByKeyResult(17-21)BifrostListModelsRequest(23-34)core/schemas/bifrost.go (6)
ModelProvider(32-32)BifrostError(322-331)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)ListModelsRequest(82-82)core/schemas/logger.go (1)
Logger(28-55)core/schemas/provider.go (1)
Provider(197-224)core/schemas/account.go (1)
Key(8-17)
core/providers/azure.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (2)
BifrostError(322-331)ListModelsRequest(82-82)
core/providers/openrouter.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (2)
BifrostError(322-331)ListModelsRequest(82-82)
core/providers/openai.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (4)
BifrostError(322-331)OpenAI(35-35)ListModelsRequest(82-82)ModelProvider(32-32)core/schemas/logger.go (1)
Logger(28-55)
core/providers/sgl.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (1)
BifrostListModelsRequest(23-34)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/cohere.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)DefaultPageSize(11-11)core/schemas/bifrost.go (3)
BifrostError(322-331)Cohere(39-39)ListModelsRequest(82-82)core/schemas/providers/cohere/types.go (1)
CohereListModelsResponse(541-544)
core/providers/mistral.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (2)
BifrostError(322-331)ListModelsRequest(82-82)core/schemas/providers/mistral/types.go (1)
MistralListModelsResponse(31-34)
core/providers/gemini.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)DefaultPageSize(11-11)core/schemas/bifrost.go (3)
BifrostError(322-331)Gemini(47-47)ListModelsRequest(82-82)
core/providers/vertex.go (5)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (4)
BifrostError(322-331)Vertex(40-40)ErrorField(340-347)RequestCancelled(282-282)core/schemas/providers/vertex/types.go (3)
VertexModel(52-59)MaxPageSize(50-50)VertexListModelsResponse(61-64)core/utils.go (1)
Ptr(46-48)
core/providers/groq.go (1)
core/schemas/account.go (1)
Key(8-17)
core/schemas/provider.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/schemas/providers/bedrock/chat.go (2)
core/schemas/providers/bedrock/types.go (1)
BedrockConverseResponse(230-238)core/schemas/chatcompletions.go (1)
BifrostChatResponse(20-30)
core/providers/bedrock.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)Model(47-66)core/schemas/bifrost.go (3)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)
🔇 Additional comments (29)
core/schemas/providers/gemini/models.go (1)
27-28: LGTM! Token limit mappings are well-implemented.The bidirectional mappings for
MaxInputTokensandMaxOutputTokensare clean and consistent, properly handling nil values and using appropriate pointer semantics.Also applies to: 57-62
core/providers/sgl.go (1)
72-84: LGTM! Multi-key signature correctly implemented.The signature update to accept multiple keys and the corresponding parameter changes in the
handleOpenAIListModelsRequestcall are consistent with the multi-key pattern adopted across providers.core/schemas/providers/bedrock/chat.go (1)
47-47: LGTM! Response standardization is well-implemented.The addition of the
modelparameter and the population of standard response fields (ID, Model, Object, Created) improve traceability and consistency across the multi-provider architecture.Also applies to: 141-146
core/schemas/provider.go (1)
201-201: LGTM! Interface updated to support multi-key operations.The signature change from a single
keytokeys []Keyis the foundation of this PR's multi-key model listing feature, enabling concurrent requests across multiple API keys for improved model discovery.core/providers/parasail.go (1)
70-82: LGTM! Multi-key support correctly implemented.The changes align with the multi-key pattern and delegate to the shared OpenAI-compatible handler appropriately.
core/providers/groq.go (1)
70-82: LGTM! Multi-key implementation is consistent.The signature update and parameter changes correctly implement the multi-key pattern for the Groq provider.
core/providers/azure.go (2)
126-204: LGTM! Per-key helper is well-implemented.The
listModelsByKeyfunction properly handles:
- Azure-specific configuration validation
- Resource acquisition and cleanup via defer
- Authentication (both Bearer token and api-key)
- Latency tracking
- Error handling and response conversion
206-217: LGTM! Multi-key orchestration properly delegated.The refactored
ListModelscleanly delegates tohandleMultipleListModelsRequests, enabling concurrent per-key processing with proper aggregation and error handling.core/providers/anthropic.go (3)
177-228: LGTM! Per-key helper is correctly implemented.The
listModelsByKeyfunction properly handles:
- Resource management with defer cleanup
- Anthropic-specific headers and authentication
- Error parsing with proper type information
- Response conversion with latency tracking
230-245: LGTM! Multi-key ListModels correctly delegates.The refactored
ListModelsfunction:
- Validates operation permissions via custom provider config
- Delegates to
handleMultipleListModelsRequestsfor concurrent processing- Comments accurately describe the best-effort, concurrent approach
845-857: LGTM! Stream error parser is well-implemented.The
parseStreamAnthropicErrorfunction properly:
- Reads and closes the response body
- Unmarshals error responses with fallback on parse failure
- Returns properly formatted
BifrostErrorwith status code and type informationcore/providers/openrouter.go (2)
64-116: LGTM: per‑key ListModels path is clean and safe.
- Copies body before release, sets latency/raw when enabled, and normalizes model IDs.
- Error handling via parseOpenAIError aligns with OpenRouter’s OpenAI‑compat API.
118-127: LGTM: multi‑key orchestration is consistent with utils.Delegating to handleMultipleListModelsRequests matches the PR’s design. No further issues.
core/providers/mistral.go (2)
70-122: LGTM: single‑key listing is correct.Auth, error mapping, latency, and raw response handling look good.
124-134: LGTM: multi‑key ListModels wiring.Uses the shared handler; no issues spotted.
core/providers/bedrock.go (3)
349-473: LGTM: Bedrock list‑models per‑key flow is robust.
- Proper SigV4 signing fallback, immediate body close after ReadAll, structured error handling, and conversion.
466-467: LGTM: latency captured correctly.StartTime before Do, used after; accurate measurement.
475-489: LGTM: operation gating + multi‑key orchestration.Matches custom provider gating and shared aggregator usage.
core/bifrost.go (1)
230-232: LGTM: nil context guard.Prevents panics and aligns with other request paths.
core/providers/gemini.go (2)
124-137: LGTM: gating + multi‑key orchestration.Matches provider pattern and shared handler usage.
171-185: No changes needed; code is correct as-is.The web search clarifies that the OpenAI-compatible Chat Completions endpoint requires
Authorization: Bearer <API_KEY>, while native Gemini endpoints usex-goog-api-key. The code at lines 171-185 correctly targets the OpenAI-compatible endpoint (/openai/chat/completions) and uses the appropriateAuthorizationheader. The suggested changes in the review comment would break authentication for this endpoint.Likely an incorrect or invalid review comment.
core/providers/openai.go (3)
80-88: LGTM: ListModels with gating + multi‑key path.Consistent with PR’s approach.
125-149: LGTM: per‑key listing is robust.Headering, body copy, error mapping, latency/raw all good.
151-172: LGTM: thin wrapper to shared multi‑key handler.Clear separation of concerns.
core/providers/vertex.go (2)
136-278: LGTM! Pagination loop handles multi-page responses correctly.The implementation properly:
- Validates Vertex-specific configuration
- Derives the correct host for global vs. regional endpoints
- Accumulates all models across paginated responses
- Tracks total latency across all pages
- Handles errors appropriately (context cancellation, timeouts, auth failures)
- Escapes page tokens in URLs
280-290: LGTM! ListModels correctly delegates to multi-key orchestration.The function properly delegates to
handleMultipleListModelsRequests, passing the per-key handlerlistModelsByKeyfor concurrent execution.core/providers/utils.go (1)
728-766: LGTM! Partial success handling is correct.The function correctly returns successful responses with
nilerror when at least one key succeeds, and only returns an error when all keys fail. This allows callers to process available models even if some keys fail.core/providers/cohere.go (2)
100-166: LGTM! Per-key request implementation is correct.The function properly:
- Builds query parameters with proper URL encoding
- Copies response body before releasing the fasthttp objects
- Handles errors appropriately
- Tracks per-key latency
- Conditionally includes raw responses
168-181: LGTM! ListModels correctly delegates to multi-key orchestration.The function properly checks operation permissions and delegates to
handleMultipleListModelsRequestsfor concurrent per-key execution.
TejasGhatte
force-pushed
the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
from
5fe52fe to
ddc542d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/sgl.go (1)
104-117: Likely missing Authorization header for streaming calls.Both TextCompletionStream and ChatCompletionStream pass nil auth headers; unless BaseURL requires no auth and ExtraHeaders injects it, this will 401. Mirror Parasail by explicitly setting Bearer auth.
- return handleOpenAITextCompletionStreaming( + authHeader := map[string]string{"Authorization": "Bearer " + key.Value} + return handleOpenAITextCompletionStreaming( ctx, provider.streamClient, provider.networkConfig.BaseURL+"/v1/completions", request, - nil, + authHeader, provider.networkConfig.ExtraHeaders, provider.sendBackRawResponse, provider.GetProviderKey(), postHookRunner, provider.logger, ) - return handleOpenAIChatCompletionStreaming( + authHeader := map[string]string{"Authorization": "Bearer " + key.Value} + return handleOpenAIChatCompletionStreaming( ctx, provider.streamClient, provider.networkConfig.BaseURL+"/v1/chat/completions", request, - nil, + authHeader, provider.networkConfig.ExtraHeaders, provider.sendBackRawResponse, schemas.SGL, postHookRunner, provider.logger, )Also applies to: 139-152
♻️ Duplicate comments (1)
core/bifrost.go (1)
2356-2363: Defend against nil context pointers.
ctxis a pointer to an interface, soctx != nildoes not guarantee*ctxis non-nil. If the caller passes a nil context (which is still valid Go usage) and we hit a fallback wherebifrost.ctxis also nil,(*ctx).Value(...)panics. Tighten the guard to ensure*ctx != nilbefore dereferencing so we degrade gracefully instead of crashing.- if ctx != nil { + if ctx != nil && *ctx != nil { key, ok := (*ctx).Value(schemas.BifrostContextKeyDirectKey).(schemas.Key) if ok { // If a direct key is specified, return it as a single-element slice return []schemas.Key{key}, nil }
🧹 Nitpick comments (1)
docs/apis/openapi.json (1)
6635-6644: Clarify semantics for new token limit fields.Please add a brief note explaining how max_input_tokens and max_output_tokens relate to context_length, ModelTopProvider.max_completion_tokens, and ModelPerRequestLimits.{prompt_tokens,completion_tokens} to avoid client confusion. If applicable, document precedence and units (tokens).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
core/bifrost.go(5 hunks)core/providers/anthropic.go(5 hunks)core/providers/azure.go(3 hunks)core/providers/bedrock.go(9 hunks)core/providers/cerebras.go(1 hunks)core/providers/cohere.go(5 hunks)core/providers/gemini.go(4 hunks)core/providers/groq.go(1 hunks)core/providers/mistral.go(3 hunks)core/providers/ollama.go(1 hunks)core/providers/openai.go(4 hunks)core/providers/openrouter.go(3 hunks)core/providers/parasail.go(1 hunks)core/providers/sgl.go(1 hunks)core/providers/utils.go(2 hunks)core/providers/vertex.go(2 hunks)core/schemas/models.go(3 hunks)core/schemas/provider.go(1 hunks)core/schemas/providers/anthropic/models.go(0 hunks)core/schemas/providers/bedrock/chat.go(3 hunks)core/schemas/providers/cohere/models.go(1 hunks)core/schemas/providers/gemini/models.go(2 hunks)core/schemas/providers/vertex/models.go(1 hunks)docs/apis/openapi.json(1 hunks)
💤 Files with no reviewable changes (1)
- core/schemas/providers/anthropic/models.go
🚧 Files skipped from review as they are similar to previous changes (6)
- core/providers/cerebras.go
- core/providers/groq.go
- core/schemas/providers/cohere/models.go
- core/schemas/models.go
- core/providers/ollama.go
- core/schemas/providers/gemini/models.go
🧰 Additional context used
🧬 Code graph analysis (15)
core/schemas/provider.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/parasail.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/gemini.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)DefaultPageSize(11-11)core/schemas/bifrost.go (3)
BifrostError(322-331)Gemini(47-47)ListModelsRequest(82-82)
core/providers/mistral.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (2)
BifrostError(322-331)ListModelsRequest(82-82)core/schemas/providers/mistral/types.go (1)
MistralListModelsResponse(31-34)
core/bifrost.go (4)
core/schemas/provider.go (1)
Provider(197-224)core/schemas/models.go (5)
BifrostListModelsResponse(36-45)Model(47-66)BifrostListModelsRequest(23-34)DefaultPageSize(11-11)MaxPaginationRequests(14-14)core/schemas/bifrost.go (4)
BifrostError(322-331)ModelProvider(32-32)ListModelsRequest(82-82)BifrostContextKeyDirectKey(104-104)core/schemas/account.go (1)
Key(8-17)
core/providers/sgl.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (1)
BifrostError(322-331)
core/providers/azure.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (2)
BifrostError(322-331)ListModelsRequest(82-82)
core/providers/openrouter.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (2)
BifrostError(322-331)ListModelsRequest(82-82)
core/providers/utils.go (5)
core/schemas/models.go (4)
BifrostListModelsResponse(36-45)Model(47-66)ListModelsByKeyResult(17-21)BifrostListModelsRequest(23-34)core/schemas/bifrost.go (6)
ModelProvider(32-32)BifrostError(322-331)ErrorField(340-347)BifrostErrorExtraFields(389-393)RequestType(79-79)ListModelsRequest(82-82)core/schemas/logger.go (1)
Logger(28-55)core/schemas/provider.go (1)
Provider(197-224)core/schemas/account.go (1)
Key(8-17)
core/schemas/providers/bedrock/chat.go (2)
core/schemas/providers/bedrock/types.go (1)
BedrockConverseResponse(230-238)core/schemas/chatcompletions.go (1)
BifrostChatResponse(20-30)
core/providers/openai.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/logger.go (1)
Logger(28-55)
core/providers/anthropic.go (5)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)DefaultPageSize(11-11)core/schemas/bifrost.go (4)
BifrostError(322-331)Anthropic(37-37)ListModelsRequest(82-82)ModelProvider(32-32)core/schemas/providers/anthropic/types.go (1)
AnthropicError(315-321)core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(25-25)
core/providers/bedrock.go (3)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)Model(47-66)core/schemas/bifrost.go (3)
BifrostError(322-331)Bedrock(38-38)ListModelsRequest(82-82)
core/providers/cohere.go (4)
core/schemas/account.go (1)
Key(8-17)core/schemas/models.go (3)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)DefaultPageSize(11-11)core/schemas/bifrost.go (2)
Cohere(39-39)ListModelsRequest(82-82)core/schemas/providers/cohere/types.go (1)
CohereListModelsResponse(541-544)
core/providers/vertex.go (6)
core/schemas/account.go (2)
Key(8-17)VertexKeyConfig(29-33)core/schemas/models.go (2)
BifrostListModelsRequest(23-34)BifrostListModelsResponse(36-45)core/schemas/bifrost.go (4)
BifrostError(322-331)Vertex(40-40)ErrorField(340-347)RequestCancelled(282-282)core/schemas/providers/vertex/types.go (3)
VertexModel(52-59)MaxPageSize(50-50)VertexListModelsResponse(61-64)core/utils.go (1)
Ptr(46-48)core/schemas/provider.go (3)
ErrProviderRequestTimedOut(22-22)ErrProviderRequest(24-24)ErrProviderResponseUnmarshal(25-25)
🔇 Additional comments (8)
core/providers/parasail.go (1)
69-82: LGTM on multi‑key ListModels.Keys slice + logger passed to the shared handler is consistent with the new pattern.
core/schemas/providers/bedrock/chat.go (1)
46-54: Good: explicit model propagation and stable identifiers.Nil-guard plus setting ID, Model, Object, and Created improves parity with public schema and observability.
Also applies to: 139-147
core/providers/azure.go (2)
126-204: Per‑key list implementation looks solid.Validates config, handles auth, copies body before release, converts and attaches latency/raw response.
206-217: LGTM on multi‑key orchestration.Delegation to handleMultipleListModelsRequests keeps concurrency concerns centralized.
core/schemas/providers/vertex/models.go (1)
14-21: No type mismatch—code is correct as-is.The
Createdfield is*int64(verified incore/schemas/models.go:51), andUnix()returnsint64. The codeschemas.Ptr(model.VersionCreateTime.Unix())correctly wraps theint64result into*int64. No cast is needed.Likely an incorrect or invalid review comment.
core/providers/sgl.go (1)
72-84: Helper signature verified—parameter order correct across all call sites.The function
handleOpenAIListModelsRequestsignature confirms it accepts parameters in the order specified: (ctx, client, request, url, keys, extraHeaders, providerName, sendBackRawResponse, logger). All six call sites—SGL, Parasail, OpenAI, Ollama, Groq, and Cerebras—consistently pass arguments in the same order. The SGL provider at lines 72–84 correctly wires the keys parameter to the shared handler and passesschemas.SGLas the provider name.core/schemas/provider.go (1)
196-204: All Provider implementations successfully updated with new ListModels signature.Verification confirms all 14 provider implementations (Anthropic, Azure, Bedrock, Cerebras, Cohere, Gemini, Groq, Mistral, Ollama, OpenAI, OpenRouter, Parasail, SGL, Vertex) have been updated to the new
ListModels(ctx context.Context, keys []schemas.Key, request *BifrostListModelsRequest)signature matching the interface definition in core/schemas/provider.go:201. No incomplete implementations detected.core/providers/bedrock.go (1)
365-384: Good call on forwarding provider filters.The query builder cleanly propagates Bedrock-specific selectors from
ExtraParams, so multi-key listing can surface the exact foundation-model filters without additional per-key plumbing.
Merge activity
|
Pratham-Mishra04
deleted the
10-27-feat_list_models_with_all_configured_keys_for_provider
branch
## Summary Enhance the ListModels functionality to aggregate results from multiple API keys, improving model discovery and availability. ## Changes - Modified `ListModels` interface to accept multiple keys instead of a single key - Implemented key aggregation logic that combines model lists from multiple API keys - Added deduplication of models based on model ID when aggregating results - Created a best-effort approach that continues processing remaining keys even if some fail - Added a new helper function `getAllSupportedKeys` to retrieve all valid keys for a provider - Updated all provider implementations to support the multi-key approach ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (Next.js) - [ ] Docs ## How to test Test the ListModels endpoint with multiple API keys configured for the same provider: ```sh # Core/Transports go version go test ./... # Test with multiple keys for a provider (e.g., OpenAI) curl -X GET "http://localhost:8000/v1/models?provider=openai" -H "Authorization: Bearer your-bifrost-token" ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues Improves model discovery by aggregating results from multiple API keys. ## Security considerations The implementation maintains the same security model as before, with proper error handling to avoid leaking sensitive information. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
2 participants
Summary
Enhance the ListModels functionality to aggregate results from multiple API keys, improving model discovery and availability.
Changes
ListModelsinterface to accept multiple keys instead of a single keygetAllSupportedKeysto retrieve all valid keys for a providerType of change
Affected areas
How to test
Test the ListModels endpoint with multiple API keys configured for the same provider:
Breaking changes
Related issues
Improves model discovery by aggregating results from multiple API keys.
Security considerations
The implementation maintains the same security model as before, with proper error handling to avoid leaking sensitive information.
Checklist