feat: blacklisted models field at provider level#2262
Merged
Conversation
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds per-key BlacklistedModels and enforces denylist precedence across key selection, provider model listing/conversion, persistence/migrations, HTTP handlers, server bootstrap, UI, and the model catalog; blacklisted models are excluded from routing and list-models outputs. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API_Server
participant DB
participant Provider
participant ModelCatalog
Client->>API_Server: request list-models / select key (model)
API_Server->>DB: fetch provider keys (include Models + BlacklistedModels)
DB-->>API_Server: return keys
API_Server->>Provider: call provider ListModels API (if needed)
Provider-->>API_Server: return provider models
Note over API_Server,DB: For each key: check BlacklistedModels first, then Models allowlist
API_Server->>ModelCatalog: UpsertModelDataForProvider(modelData, allowedModels, deniedModels)
ModelCatalog-->>API_Server: store/update catalog (apply denied set when allowed empty)
API_Server-->>Client: return filtered model list or selected key
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
transports/bifrost-http/server/server.go (1)
529-540:⚠️ Potential issue | 🟠 MajorReloadProvider path still re-adds blacklisted models to the catalog.
Line 535 currently appends all
key.Modelswithout excludingkey.BlacklistedModels, unlike Line 766 and Line 1258. After a provider reload, blacklisted models can reappear in catalog data until the next bootstrap/force reload.🔧 Suggested fix
modelsInKeys := make([]schemas.Model, 0) for _, key := range providerKeys { for _, model := range key.Models { + if slices.Contains(key.BlacklistedModels, model) { + continue + } modelsInKeys = append(modelsInKeys, schemas.Model{ ID: string(provider) + "/" + model, }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/server/server.go` around lines 529 - 540, The ReloadProvider path currently rebuilds modelsInKeys by appending every model from each key (providerKeys -> key.Models) without checking key.BlacklistedModels, causing blacklisted models to be reintroduced; update the loop in the ReloadProvider logic (where providerKeys is retrieved via s.Config.ConfigStore.GetKeysByProvider and modelsInKeys is built) to skip any model present in key.BlacklistedModels (e.g., convert key.BlacklistedModels to a set/map and only append models from key.Models that are not in that set) so the behavior matches the blacklist filtering used elsewhere (lines similar to the logic at the other locations that already exclude blacklisted models).core/providers/bedrock/bedrock.go (1)
78-88:⚠️ Potential issue | 🟠 MajorForce HTTP/2 for Bedrock transport instead of config-gating it.
ForceAttemptHTTP2is currently controlled by config, but Bedrock transport in this repo should always force HTTP/2.🔧 Suggested fix
transport := &http.Transport{ Proxy: http.ProxyFromEnvironment, MaxConnsPerHost: config.NetworkConfig.MaxConnsPerHost, MaxIdleConns: schemas.DefaultMaxIdleConnsPerHost, MaxIdleConnsPerHost: schemas.DefaultMaxIdleConnsPerHost, IdleConnTimeout: 30 * time.Second, TLSHandshakeTimeout: 10 * time.Second, ResponseHeaderTimeout: requestTimeout, ExpectContinueTimeout: 1 * time.Second, - ForceAttemptHTTP2: config.NetworkConfig.EnforceHTTP2, + ForceAttemptHTTP2: true, }As per coding guidelines,
core/providers/bedrock/*.go: Bedrock usesnet/httpwithhttp.Transportconfigured withForceAttemptHTTP2: trueandMaxConnsPerHostfromNetworkConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 78 - 88, Update the transport initialization in bedrock.go so that ForceAttemptHTTP2 is unconditionally true instead of using config.NetworkConfig.EnforceHTTP2: in the block where the http.Transport is created (the variable named transport), set ForceAttemptHTTP2 to true and keep MaxConnsPerHost sourced from config.NetworkConfig.MaxConnsPerHost (leave other fields intact) so Bedrock always attempts HTTP/2.framework/configstore/rdb.go (1)
283-290:⚠️ Potential issue | 🔴 CriticalUpdate
GenerateKeyHashto includeBlacklistedModelsin the hash computation.The PR persists
BlacklistedModelsto the database (lines 283–290, 452–460, 574–582 in rdb.go), butGenerateKeyHashinframework/configstore/clientconfig.godoes not include this field in the hash. Consequently, a blacklist-only configuration change will not flipConfigHashand will be skipped as unchanged by reconciliation logic. AddBlacklistedModelsto the hash (similar to howModelsis hashed), and add a regression test covering a blacklist-only change scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 283 - 290, GenerateKeyHash in framework/configstore/clientconfig.go currently omits BlacklistedModels, so update GenerateKeyHash to include BlacklistedModels in the same manner Models is included (serialize/sort and feed into the hash alongside Provider, ProviderID, KeyID, Name, Value, Models) so that blacklist-only changes alter ConfigHash; then add a regression test that creates two keys differing only in BlacklistedModels and asserts their generated ConfigHash values differ and that reconciliation treats them as changed. Ensure you reference the GenerateKeyHash function and the ConfigHash comparison path in tests when asserting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 6099-6107: Direct keys built from request headers are returned
early by getKeysForBatchAndFileOps and selectKeyFromProviderForModel, bypassing
the BlacklistedModels checks; update those functions to apply the same blacklist
logic to direct keys before returning. Specifically, when you construct/return a
direct key (the Key object created from header credentials in
getKeysForBatchAndFileOps and selectKeyFromProviderForModel), run the same
checks used around BlacklistedModels/Models (slices.Contains on
k.BlacklistedModels and k.Models for the requested model) and skip/deny the
direct key if the model is blacklisted or not allowed; ensure you reference the
Key struct fields BlacklistedModels and Models and the model parameter in your
condition so direct-key flows are filtered identically to provider-key flows.
In `@core/providers/gemini/models.go`:
- Around line 35-39: The blacklist check uses only the trimmed modelName and
misses variants like "models/..." and base-model matches (e.g., "gemini-1.5-pro"
vs "gemini-1.5-pro-001"); update the filtering to normalize names before
comparing by creating/using a normalizeModelName helper (trim spaces, strip any
"models/" prefix, and lower-case if needed) and then replace the direct
slices.Contains(blacklistedModels, modelName) check with a check that tests the
normalized name against blacklistedModels both for exact equality and for
base-prefix matches (e.g., strings.HasPrefix(normalizedCandidate,
blacklistedBase) or normalize both and compare prefixes). Apply the same
normalized matching in both locations that currently use slices.Contains (the
block with allowedModels/blacklistedModels and the later block at lines ~55-58)
so all comparisons use the normalized form.
In `@core/providers/mistral/models.go`:
- Around line 23-25: Replace exact-match denylist checks that use
slices.Contains(blacklistedModels, model.ID) with the shared fuzzy-matching
utility providerUtils.ModelMatchesDenylist so version-suffixes can't bypass the
denylist; update both occurrences referencing blacklistedModels and model.ID
(the check around lines 23 and the block at 40-42) to call
providerUtils.ModelMatchesDenylist(blacklistedModels, model.ID) and use its
boolean result for the continue/skip logic.
In `@core/providers/openai/models.go`:
- Around line 21-25: Replace the literal-ID blacklist checks that use
slices.Contains with the shared denylist matcher so pattern-based entries (e.g.,
"gpt-4o") catch variant IDs like "gpt-4o-2024-08-06"; update the check around
slices.Contains(blacklistedModels, model.ID) in core/providers/openai/models.go
to call the common denylist matcher (same API used elsewhere in the codebase)
against model.ID, and ensure the denylist check runs before allowing models
(i.e., deny-first flow) so that both the blacklistedModels check and the similar
check at lines 37-40 use the matcher rather than exact string contains.
In `@core/providers/vertex/models.go`:
- Around line 281-283: The blacklist check for publisher models uses exact
matching via slices.Contains on blacklistedModels and modelID, which is
inconsistent with the custom-models path that uses
providerUtils.ModelMatchesDenylist (SameBaseModel semantics); replace the
slices.Contains check with a call to
providerUtils.ModelMatchesDenylist(blacklistedModels, modelID) (or the
equivalent helper used elsewhere) so blacklist entries block variants like
"gemini-1.5-pro-001" when "gemini-1.5-pro" is listed.
In `@framework/configstore/clientconfig.go`:
- Around line 296-304: The code references key.BlacklistedModels and assigns it
into schemas.Key (in the redactedConfig.Keys population), but the schemas.Key
type does not define a BlacklistedModels field, causing a compile error; update
the assignment to match the schemas.Key definition by either (A) removing
BlacklistedModels usage and not setting that field when constructing schemas.Key
inside the loop that builds redactedConfig.Keys, or (B) if the schema should
include it, add a BlacklistedModels []string field to the schemas.Key type and
update any schema serialization/usage accordingly; locate the construction of
schemas.Key (redactedConfig.Keys[i] = schemas.Key{...}) and adjust that line and
the key.BlacklistedModels reference to be consistent with the schemas.Key
struct.
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 757-766: The keyAllowsModelForList function uses exact string
matching (slices.Contains) which is inconsistent with provider converters that
use schemas.SameBaseModel; update keyAllowsModelForList to perform
base-model-aware comparisons by iterating key.BlacklistedModels and key.Models
and using schemas.SameBaseModel(model, entry) (or the
providerUtils.ModelMatchesDenylist helper if preferred) for each comparison so
blacklist and whitelist checks mirror
anthropicModelBlacklisted/providerUtils.ModelMatchesDenylist semantics and avoid
mismatches between listing and request-time validation.
In `@transports/bifrost-http/lib/config.go`:
- Around line 3009-3019: ConfigData.UnmarshalJSON() currently constructs a
schemas.Key literal that omits copying tableKey.BlacklistedModels, so inline
keys loaded from config.json lose their denylist; update the schemas.Key{...}
creation inside ConfigData.UnmarshalJSON() to set BlacklistedModels:
tableKey.BlacklistedModels (or default to an empty slice when nil) so the field
is preserved (mirror the logic used where configstoreTables.TableKey is
converted, and ensure the same nil-to-empty fallback is applied).
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 222-242: The data-testid is currently placed on FormItem but needs
to be on the interactive control; update the JSX that renders ModelMultiselect
so it receives a test id like "apikey-blacklisted-models-input" (pass a prop
such as testId or dataTestId from the call site where
ModelMultiselect(provider={providerName} value={field.value || []}
onChange={field.onChange}) is used) and then modify the ModelMultiselect
component signature to accept and apply that prop to its input/trigger element
(or root interactive element) so tests can target the actual control; ensure the
prop name is forwarded through any intermediate wrappers and used on the
appropriate element inside ModelMultiselect.
---
Outside diff comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 78-88: Update the transport initialization in bedrock.go so that
ForceAttemptHTTP2 is unconditionally true instead of using
config.NetworkConfig.EnforceHTTP2: in the block where the http.Transport is
created (the variable named transport), set ForceAttemptHTTP2 to true and keep
MaxConnsPerHost sourced from config.NetworkConfig.MaxConnsPerHost (leave other
fields intact) so Bedrock always attempts HTTP/2.
In `@framework/configstore/rdb.go`:
- Around line 283-290: GenerateKeyHash in framework/configstore/clientconfig.go
currently omits BlacklistedModels, so update GenerateKeyHash to include
BlacklistedModels in the same manner Models is included (serialize/sort and feed
into the hash alongside Provider, ProviderID, KeyID, Name, Value, Models) so
that blacklist-only changes alter ConfigHash; then add a regression test that
creates two keys differing only in BlacklistedModels and asserts their generated
ConfigHash values differ and that reconciliation treats them as changed. Ensure
you reference the GenerateKeyHash function and the ConfigHash comparison path in
tests when asserting behavior.
In `@transports/bifrost-http/server/server.go`:
- Around line 529-540: The ReloadProvider path currently rebuilds modelsInKeys
by appending every model from each key (providerKeys -> key.Models) without
checking key.BlacklistedModels, causing blacklisted models to be reintroduced;
update the loop in the ReloadProvider logic (where providerKeys is retrieved via
s.Config.ConfigStore.GetKeysByProvider and modelsInKeys is built) to skip any
model present in key.BlacklistedModels (e.g., convert key.BlacklistedModels to a
set/map and only append models from key.Models that are not in that set) so the
behavior matches the blacklist filtering used elsewhere (lines similar to the
logic at the other locations that already exclude blacklisted models).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fee1bb33-07cd-4335-8421-46cbf7c09668
📒 Files selected for processing (38)
core/bifrost.gocore/bifrost_test.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/models.gocore/providers/azure/azure.gocore/providers/azure/models.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/models.gocore/providers/cohere/cohere.gocore/providers/cohere/models.gocore/providers/elevenlabs/elevenlabs.gocore/providers/elevenlabs/models.gocore/providers/gemini/gemini.gocore/providers/gemini/models.gocore/providers/huggingface/huggingface.gocore/providers/huggingface/models.gocore/providers/mistral/mistral.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openai/openai.gocore/providers/replicate/models.gocore/providers/replicate/replicate.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
TejasGhatte
force-pushed
the
03-24-feat_blacklisted_models_field_at_provider_level
branch
from
61c4902 to
665343b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/openrouter/openrouter.go (1)
223-227:⚠️ Potential issue | 🟠 MajorBlacklist not applied when no allowlist is configured.
When
len(allowedModels) == 0(no allowlist, meaning all models are implicitly allowed), the else branch skips blacklist filtering entirely. This means a key with onlyBlacklistedModelsconfigured (noModelsallowlist) will not have those models filtered from the listing response, which contradicts the "blacklist precedence" semantics described in the PR.Consider separating the
Unfilteredcase (where bypassing all filters may be intentional) from the "no allowlist" case where blacklist should still apply.🐛 Proposed fix to apply blacklist when no allowlist exists
} else { + // Apply blacklist filtering even when there's no allowlist + filteredData := make([]schemas.Model, 0, len(openrouterResponse.Data)) for i := range openrouterResponse.Data { - openrouterResponse.Data[i].ID = providerPrefix + openrouterResponse.Data[i].ID + rawID := openrouterResponse.Data[i].ID + if !request.Unfiltered && providerUtils.ModelMatchesDenylist(blacklistedModels, rawID, providerPrefix+rawID) { + continue + } + openrouterResponse.Data[i].ID = providerPrefix + rawID + filteredData = append(filteredData, openrouterResponse.Data[i]) } + openrouterResponse.Data = filteredData }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openrouter/openrouter.go` around lines 223 - 227, The current else branch that only prefixes IDs (loop over openrouterResponse.Data) skips applying BlacklistedModels when allowedModels is empty; change the logic in the block handling openrouterResponse.Data so that you always apply blacklist filtering unless the request is explicitly marked Unfiltered: after prefixing with providerPrefix in the loop, filter out any items whose model (post-prefix or original ID as appropriate) appears in the BlacklistedModels set, but only skip all filters when the Unfiltered flag is true; update references to allowedModels, BlacklistedModels, and the loop that mutates openrouterResponse.Data to implement this split between "no allowlist" (apply blacklist) and explicit Unfiltered bypass.
♻️ Duplicate comments (3)
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
222-243:⚠️ Potential issue | 🟡 MinorPut the test id on the multiselect control.
data-testidis attached toFormItem, but the new interactive element isModelMultiselect. Move the selector to the control/trigger itself (for example,apikey-blacklisted-models-input) so tests can target it directly. IfModelMultiselectdoes not accept a test-id prop yet, thread one through.As per coding guidelines, "Add new interactive UI elements with
data-testidattributes following the pattern:data-testid="<entity>-<element>-<qualifier>"."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 222 - 243, The data-testid is currently on FormItem but needs to be attached to the interactive ModelMultiselect so tests can target the control; update the JSX to move the test id to the ModelMultiselect component (e.g., data-testid="apikey-blacklisted-models-input"), and if ModelMultiselect does not accept a test-id prop yet, add a prop (e.g., dataTestId or data-testid) to ModelMultiselect's props and thread it through to the underlying control/trigger; keep the existing value={field.value || []}, onChange={field.onChange} and unfiltered={true} usage unchanged.core/providers/vertex/models.go (1)
212-214:⚠️ Potential issue | 🟠 MajorFinish switching the remaining Vertex blacklist checks to the shared matcher.
The first pass already uses
providerUtils.ModelMatchesDenylist, but these remaining exact matches can still backfill or publish blacklisted variants. This is the same unresolved matcher gap previously called out on the publisher-model path.🔧 Proposed fix
- if slices.Contains(blacklistedModels, alias) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, alias, deploymentValue) { continue } @@ - if !unfiltered && slices.Contains(blacklistedModels, allowedModel) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, allowedModel) { continue } @@ - if !unfiltered && slices.Contains(blacklistedModels, modelID) { + if !unfiltered && providerUtils.ModelMatchesDenylist(blacklistedModels, modelID) { continue }Also applies to: 236-238, 281-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/models.go` around lines 212 - 214, Replace the direct exact-match checks using slices.Contains(blacklistedModels, alias) with the shared denylist matcher providerUtils.ModelMatchesDenylist to ensure all blacklist logic is centralized; locate occurrences that reference blacklistedModels and alias (including the shown slices.Contains call and the similar checks around the same area) and call providerUtils.ModelMatchesDenylist(alias, blacklistedModels) instead, removing the slices.Contains usage so backfilled/published variants are also matched by the shared matcher.core/providers/mistral/models.go (1)
23-25:⚠️ Potential issue | 🟠 MajorUse the shared denylist matcher for the remaining Mistral blacklist checks.
This is still the same unresolved matcher gap from the earlier review: exact
slices.Containschecks here can miss version/base-model matches, so blacklisted Mistral variants can still appear from both the API response and the allowlist backfill path.🔧 Proposed fix
import ( "slices" + providerUtils "github.com/maximhq/bifrost/core/providers/utils" "github.com/maximhq/bifrost/core/schemas" ) @@ - if slices.Contains(blacklistedModels, model.ID) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, model.ID) { continue } @@ - if slices.Contains(blacklistedModels, allowedModel) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, allowedModel) { continue }Also applies to: 40-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/mistral/models.go` around lines 23 - 25, Replace the exact slices.Contains check against blacklistedModels (using model.ID) with the project’s shared denylist matcher so variant/version/base-model patterns are matched; locate the occurrences around the blacklistedModels and model.ID usage (including the similar check at lines ~40-42) and call the central denylist matching function (the shared "denylist matcher" utility used elsewhere in the repo) instead of slices.Contains, passing model.ID (or model name) to that matcher and continuing when it returns true.
🧹 Nitpick comments (3)
core/providers/openrouter/openrouter.go (1)
201-213: UseproviderUtils.ModelMatchesDenylistfor consistent denylist matching.The inline blacklist checks use only exact string matching via
slices.Contains, but the shared utilityproviderUtils.ModelMatchesDenylist(used by Azure, Bedrock) also performs base-model matching throughschemas.SameBaseModel. This could lead to inconsistent behavior where version-aware model patterns match in other providers but not in OpenRouter.♻️ Proposed refactor to use shared utility
if !(slices.Contains(allowedModels, rawID) || slices.Contains(allowedModels, providerPrefix+rawID)) { continue } - if slices.Contains(blacklistedModels, rawID) || slices.Contains(blacklistedModels, providerPrefix+rawID) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, rawID, providerPrefix+rawID) { continue }And similarly for the backfill loop:
for _, allowedModel := range allowedModels { rawID := strings.TrimPrefix(allowedModel, providerPrefix) - if slices.Contains(blacklistedModels, rawID) || slices.Contains(blacklistedModels, providerPrefix+rawID) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, rawID, providerPrefix+rawID) { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openrouter/openrouter.go` around lines 201 - 213, Replace the inline exact-match blacklist checks (slices.Contains on blacklistedModels for rawID and providerPrefix+rawID) with the shared utility providerUtils.ModelMatchesDenylist so denylist matching is consistent with other providers; update both the main loop that filters openrouterResponse.Data (where rawID, providerPrefix, includedModels are used) and the backfill loop over allowedModels (where rawID is computed from providerPrefix) to call providerUtils.ModelMatchesDenylist(blacklistedModels, candidateModel) instead of slices.Contains, preserving the continue logic and the appended/assigned values.core/bifrost.go (1)
6099-6107: Extract the model-eligibility rule into one helper.The blacklist/allow-list precedence now lives here and again in
selectKeyFromProviderForModel(). A shared predicate will keep the batch/file path and the single-key path from drifting the next time model-routing rules change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 6099 - 6107, Extract the model-eligibility check into a single helper (e.g., isKeyEligibleForModel or key.AllowsModel) and replace the duplicate logic in both the batch/file path (the block that checks k.BlacklistedModels and k.Models when iterating keys) and in selectKeyFromProviderForModel(); the helper should accept the key (k) and the model pointer (model *string) and implement the three rules described (blacklist wins, empty Models means allow, non-empty Models requires membership), then call that helper from the loop and from selectKeyFromProviderForModel() so both paths use the same predicate.framework/configstore/tables/key.go (1)
103-111: Factor the string-slice JSON round-trip into helpers.
ModelsandBlacklistedModelsnow duplicate the same marshal/unmarshal/default-to-[]flow in four places. A tiny helper would keep nil/empty-slice semantics aligned and make future changes safer.♻️ Helper sketch
- if k.BlacklistedModels != nil { - data, err := json.Marshal(k.BlacklistedModels) - if err != nil { - return err - } - k.BlacklistedModelsJSON = string(data) - } else { - k.BlacklistedModelsJSON = "[]" - } + blacklistedJSON, err := marshalStringSlice(k.BlacklistedModels) + if err != nil { + return err + } + k.BlacklistedModelsJSON = blacklistedJSON- if k.BlacklistedModelsJSON != "" { - if err := json.Unmarshal([]byte(k.BlacklistedModelsJSON), &k.BlacklistedModels); err != nil { - return err - } - } else { - k.BlacklistedModels = []string{} - } + k.BlacklistedModels, err = unmarshalStringSlice(k.BlacklistedModelsJSON) + if err != nil { + return err + }func marshalStringSlice(v []string) (string, error) { if v == nil { return "[]", nil } data, err := json.Marshal(v) if err != nil { return "", err } return string(data), nil } func unmarshalStringSlice(raw string) ([]string, error) { if raw == "" { return []string{}, nil } var out []string if err := json.Unmarshal([]byte(raw), &out); err != nil { return nil, err } return out, nil }Also applies to: 501-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/key.go` around lines 103 - 111, The code duplicates JSON marshal/unmarshal logic for string slices (e.g., k.BlacklistedModels <-> k.BlacklistedModelsJSON and the similar Models handling); extract that logic into two helpers (suggested names marshalStringSlice and unmarshalStringSlice) and replace the repeated blocks so setters use marshalStringSlice to produce a JSON string defaulting to "[]" for nil slices and getters use unmarshalStringSlice to parse "" or JSON into []string; update all occurrences (including the block around k.BlacklistedModels and the similar Models block at lines ~501-507) to call these helpers and propagate/return errors from the helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 787-789: When filtering provider models by requested keys, change
the logic so that if the request provided keys (keys not empty) and
len(matchedKeys) == 0 you return an empty models result instead of returning the
full models slice; modify the branch that currently does "if len(matchedKeys) ==
0 { return models }" to check whether keys were supplied and, when they were,
return an empty slice (preserving the original behavior of returning all models
only when keys is empty). Ensure you reference/adjust the variables keys,
matchedKeys and models in providers.go accordingly.
---
Outside diff comments:
In `@core/providers/openrouter/openrouter.go`:
- Around line 223-227: The current else branch that only prefixes IDs (loop over
openrouterResponse.Data) skips applying BlacklistedModels when allowedModels is
empty; change the logic in the block handling openrouterResponse.Data so that
you always apply blacklist filtering unless the request is explicitly marked
Unfiltered: after prefixing with providerPrefix in the loop, filter out any
items whose model (post-prefix or original ID as appropriate) appears in the
BlacklistedModels set, but only skip all filters when the Unfiltered flag is
true; update references to allowedModels, BlacklistedModels, and the loop that
mutates openrouterResponse.Data to implement this split between "no allowlist"
(apply blacklist) and explicit Unfiltered bypass.
---
Duplicate comments:
In `@core/providers/mistral/models.go`:
- Around line 23-25: Replace the exact slices.Contains check against
blacklistedModels (using model.ID) with the project’s shared denylist matcher so
variant/version/base-model patterns are matched; locate the occurrences around
the blacklistedModels and model.ID usage (including the similar check at lines
~40-42) and call the central denylist matching function (the shared "denylist
matcher" utility used elsewhere in the repo) instead of slices.Contains, passing
model.ID (or model name) to that matcher and continuing when it returns true.
In `@core/providers/vertex/models.go`:
- Around line 212-214: Replace the direct exact-match checks using
slices.Contains(blacklistedModels, alias) with the shared denylist matcher
providerUtils.ModelMatchesDenylist to ensure all blacklist logic is centralized;
locate occurrences that reference blacklistedModels and alias (including the
shown slices.Contains call and the similar checks around the same area) and call
providerUtils.ModelMatchesDenylist(alias, blacklistedModels) instead, removing
the slices.Contains usage so backfilled/published variants are also matched by
the shared matcher.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 222-243: The data-testid is currently on FormItem but needs to be
attached to the interactive ModelMultiselect so tests can target the control;
update the JSX to move the test id to the ModelMultiselect component (e.g.,
data-testid="apikey-blacklisted-models-input"), and if ModelMultiselect does not
accept a test-id prop yet, add a prop (e.g., dataTestId or data-testid) to
ModelMultiselect's props and thread it through to the underlying
control/trigger; keep the existing value={field.value || []},
onChange={field.onChange} and unfiltered={true} usage unchanged.
---
Nitpick comments:
In `@core/bifrost.go`:
- Around line 6099-6107: Extract the model-eligibility check into a single
helper (e.g., isKeyEligibleForModel or key.AllowsModel) and replace the
duplicate logic in both the batch/file path (the block that checks
k.BlacklistedModels and k.Models when iterating keys) and in
selectKeyFromProviderForModel(); the helper should accept the key (k) and the
model pointer (model *string) and implement the three rules described (blacklist
wins, empty Models means allow, non-empty Models requires membership), then call
that helper from the loop and from selectKeyFromProviderForModel() so both paths
use the same predicate.
In `@core/providers/openrouter/openrouter.go`:
- Around line 201-213: Replace the inline exact-match blacklist checks
(slices.Contains on blacklistedModels for rawID and providerPrefix+rawID) with
the shared utility providerUtils.ModelMatchesDenylist so denylist matching is
consistent with other providers; update both the main loop that filters
openrouterResponse.Data (where rawID, providerPrefix, includedModels are used)
and the backfill loop over allowedModels (where rawID is computed from
providerPrefix) to call providerUtils.ModelMatchesDenylist(blacklistedModels,
candidateModel) instead of slices.Contains, preserving the continue logic and
the appended/assigned values.
In `@framework/configstore/tables/key.go`:
- Around line 103-111: The code duplicates JSON marshal/unmarshal logic for
string slices (e.g., k.BlacklistedModels <-> k.BlacklistedModelsJSON and the
similar Models handling); extract that logic into two helpers (suggested names
marshalStringSlice and unmarshalStringSlice) and replace the repeated blocks so
setters use marshalStringSlice to produce a JSON string defaulting to "[]" for
nil slices and getters use unmarshalStringSlice to parse "" or JSON into
[]string; update all occurrences (including the block around k.BlacklistedModels
and the similar Models block at lines ~501-507) to call these helpers and
propagate/return errors from the helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b78db4d3-facc-4ead-a146-84c54e8d21a7
📒 Files selected for processing (39)
core/bifrost.gocore/bifrost_test.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/models.gocore/providers/azure/azure.gocore/providers/azure/models.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/models.gocore/providers/cohere/cohere.gocore/providers/cohere/models.gocore/providers/elevenlabs/elevenlabs.gocore/providers/elevenlabs/models.gocore/providers/gemini/gemini.gocore/providers/gemini/models.gocore/providers/huggingface/huggingface.gocore/providers/huggingface/models.gocore/providers/mistral/mistral.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openai/openai.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/replicate/replicate.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (5)
- ui/app/workspace/providers/views/providerKeyForm.tsx
- core/providers/elevenlabs/elevenlabs.go
- core/providers/huggingface/huggingface.go
- transports/bifrost-http/server/server.go
- core/providers/mistral/mistral.go
🚧 Files skipped from review as they are similar to previous changes (20)
- ui/lib/types/schemas.ts
- core/providers/cohere/cohere.go
- core/providers/anthropic/anthropic.go
- core/providers/replicate/replicate.go
- core/providers/vertex/utils.go
- core/providers/huggingface/models.go
- core/providers/anthropic/models.go
- core/providers/replicate/models.go
- core/providers/elevenlabs/models.go
- framework/configstore/clientconfig.go
- core/providers/utils/utils.go
- core/providers/gemini/models.go
- core/providers/vertex/vertex.go
- core/providers/bedrock/bedrock.go
- core/providers/openai/models.go
- core/providers/bedrock/models.go
- framework/configstore/migrations.go
- core/providers/azure/models.go
- transports/bifrost-http/lib/config.go
- core/providers/cohere/models.go
TejasGhatte
force-pushed
the
03-24-feat_blacklisted_models_field_at_provider_level
branch
2 times, most recently
from
d12c30f to
f7750bf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/anthropic/models.go (1)
55-65:⚠️ Potential issue | 🟡 MinorKeep the duplicate-suppression map updated in the backfill path.
This branch appends a synthetic allowed model but never records it in
includedModels, so repeated values inallowedModelscan still show up multiple times in the final response.🔧 Proposed fix
if !includedModels[allowedModel] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ ID: string(providerKey) + "/" + allowedModel, Name: schemas.Ptr(allowedModel), }) + includedModels[allowedModel] = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/models.go` around lines 55 - 65, In the backfill branch (when !unfiltered && len(allowedModels) > 0) you append synthetic allowed models to bifrostResponse.Data but never mark them in the duplicate-suppression map includedModels; update includedModels[allowedModel] = true immediately after appending (and keep the existing providerUtils.ModelMatchesDenylist check and ID construction using providerKey) so repeated entries in allowedModels are suppressed in subsequent iterations and the final response.core/providers/gemini/models.go (1)
54-63:⚠️ Potential issue | 🟡 MinorRecord backfilled models in
includedModels.Now that blacklisting can force more models through the backfill branch, this path needs to keep the dedupe map in sync too. Otherwise duplicate values in
allowedModelsstill produce duplicatelist-modelsentries.🔧 Proposed fix
if !includedModels[allowedModel] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ ID: string(providerKey) + "/" + allowedModel, Name: schemas.Ptr(allowedModel), }) + includedModels[allowedModel] = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/models.go` around lines 54 - 63, The backfill branch that appends models to bifrostResponse.Data when iterating allowedModels doesn't update the dedupe map includedModels, so duplicate entries in allowedModels can produce duplicate list-models results; after appending the schemas.Model for an allowedModel (the code block that builds ID using providerKey and sets Name via schemas.Ptr), set includedModels[allowedModel] = true to keep the dedupe map in sync with the backfilled entries and prevent duplicates.
♻️ Duplicate comments (3)
core/providers/vertex/models.go (1)
281-283:⚠️ Potential issue | 🟠 MajorUse the shared denylist matcher for publisher models too.
Line 180 already uses
providerUtils.ModelMatchesDenylistfor the custom-model path, but this publisher-model branch falls back to exact matching. That means a blacklist entry likegemini-1.5-procan still surface a versioned publisher ID such asgemini-1.5-pro-001here.🔧 Proposed fix
- if !unfiltered && slices.Contains(blacklistedModels, modelID) { + if !unfiltered && providerUtils.ModelMatchesDenylist(blacklistedModels, modelID) { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/models.go` around lines 281 - 283, Replace the exact-match check against blacklistedModels in the publisher-model branch with the shared denylist matcher: instead of using slices.Contains(blacklistedModels, modelID) inside the if (!unfiltered && ...) block, call providerUtils.ModelMatchesDenylist(modelID, blacklistedModels) so versioned publisher IDs (e.g., gemini-1.5-pro-001) are correctly caught; keep the same !unfiltered guard and continue behavior.ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
222-242:⚠️ Potential issue | 🟡 MinorMove the test id onto the multiselect control.
data-testidonFormItemdoes not give tests a stable handle for the new interactive element. Pass something likeapikey-blacklisted-models-inputthroughModelMultiselectand apply it on the actual trigger/input.As per coding guidelines, "Add new interactive UI elements with
data-testidattributes following the pattern:data-testid=\"<entity>-<element>-<qualifier>\"."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 222 - 242, The test id is currently on FormItem but needs to be on the actual interactive control; update the usage of ModelMultiselect (where provider={providerName} value={field.value || []} onChange={field.onChange}) to accept and forward a data-testid (e.g., "apikey-blacklisted-models-input") and remove the data-testid from FormItem; modify ModelMultiselect to apply that prop to its trigger/input element so tests get a stable handle for the interactive control.transports/bifrost-http/handlers/providers.go (1)
786-800:⚠️ Potential issue | 🟠 MajorUnfiltered fallback when no keys match may produce unexpected results for cross-provider queries.
When the caller requests models filtered by specific key IDs (e.g.,
keys=openai-key-1) and this function is invoked for a different provider (e.g., Anthropic),matchedKeyswill be empty because none of the requested key IDs exist in that provider's configuration. The current behavior returns all models unfiltered, which means/api/models?keys=openai-key-1would include every Anthropic model—likely not the intended result.Consider returning an empty slice when
len(keyIDs) > 0but no keys matched, reserving the unfiltered fallback only for whenkeyIDsitself is empty.🔧 Proposed fix
- // Unknown key IDs (or empty keyIDs): do not filter + // No matching keys for this provider: if caller specified key IDs, return empty; + // if no key IDs were requested, return unfiltered. if len(matchedKeys) == 0 { + if len(keyIDs) > 0 { + return []string{} + } return models }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/providers.go` around lines 786 - 800, The current logic builds keysByID and matchedKeys but returns the unfiltered models when matchedKeys is empty, causing cross-provider queries (e.g., keys=openai-key-1 on Anthropic) to return all models; change the condition around matchedKeys so that if len(matchedKeys) == 0 and len(keyIDs) == 0 you return models (unfiltered), but if len(matchedKeys) == 0 and len(keyIDs) > 0 you return an empty slice (no models match the requested keys); update the branch in the block that references keysByID, matchedKeys, keyIDs, and models to implement this behavior.
🧹 Nitpick comments (2)
framework/configstore/rdb.go (1)
290-290: Extract a sharedschemas.Key→tables.TableKeymapper.This mapping now has to stay in sync across three write paths. Centralizing it would make the next key-field rollout much harder to miss in one branch.
Also applies to: 459-459, 581-581
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` at line 290, Multiple write paths in rdb.go duplicate the mapping from schemas.Key to tables.TableKey (e.g., the assignment using BlacklistedModels: key.BlacklistedModels), which must be centralized to avoid drift; extract a single helper function (e.g., MapKeyToTableKey or KeyToTableKey) that accepts schemas.Key and returns tables.TableKey and replace the three duplicated mapping sites with calls to that helper (ensure all fields of schemas.Key are mapped consistently inside the new function and update references where currently constructing tables.TableKey directly).core/bifrost.go (1)
6099-6107: Extract the blacklist/allow-list check into one helper.These two selectors now inline the same access-control rule. Pulling the model-support decision into a shared helper will keep single-key selection and batch/file selection aligned the next time blacklist semantics change.
Also applies to: 6174-6205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 6099 - 6107, Extract the duplicated model allowlist/blacklist logic into a single helper (e.g., a function like supportsModel or a method on the key type) that takes the key (k) and the model pointer/string and returns a bool indicating whether the key supports that model; move the current inline checks (the slices.Contains checks on k.BlacklistedModels and k.Models around model != nil && *model != "") into that helper and replace both occurrences (the current block around k.BlacklistedModels/k.Models and the similar block at lines 6174-6205) to call this helper so single-key selection and batch/file selection share the same decision logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 189-201: The inline key hydration is dropping provider-specific
fields ReplicateKeyConfig and VLLMKeyConfig when building the schemas.Key
entries; update the append to keysToAddToProvider in the hydration code (the
block that constructs schemas.Key from tableKey) to copy
tableKey.ReplicateKeyConfig and tableKey.VLLMKeyConfig into the new schemas.Key
so the full provider-specific configs are preserved when adding keys to
cd.Providers.
In `@transports/changelog.md`:
- Line 1: The changelog entry for the bullet starting with "fix: add support for
`x-bf-mcp-include-clients` and `x-bf-mcp-include-tools` request headers..." is
incomplete; finish the sentence to explain the context and effect (for example:
"...to filter MCP tools/list responses when using bifrost as an MCP proxy or
gateway."), ensuring the bullet becomes a complete, clear release note
referencing the two headers and that they filter the tools/clients list when
bifrost is acting as a proxy/gateway.
---
Outside diff comments:
In `@core/providers/anthropic/models.go`:
- Around line 55-65: In the backfill branch (when !unfiltered &&
len(allowedModels) > 0) you append synthetic allowed models to
bifrostResponse.Data but never mark them in the duplicate-suppression map
includedModels; update includedModels[allowedModel] = true immediately after
appending (and keep the existing providerUtils.ModelMatchesDenylist check and ID
construction using providerKey) so repeated entries in allowedModels are
suppressed in subsequent iterations and the final response.
In `@core/providers/gemini/models.go`:
- Around line 54-63: The backfill branch that appends models to
bifrostResponse.Data when iterating allowedModels doesn't update the dedupe map
includedModels, so duplicate entries in allowedModels can produce duplicate
list-models results; after appending the schemas.Model for an allowedModel (the
code block that builds ID using providerKey and sets Name via schemas.Ptr), set
includedModels[allowedModel] = true to keep the dedupe map in sync with the
backfilled entries and prevent duplicates.
---
Duplicate comments:
In `@core/providers/vertex/models.go`:
- Around line 281-283: Replace the exact-match check against blacklistedModels
in the publisher-model branch with the shared denylist matcher: instead of using
slices.Contains(blacklistedModels, modelID) inside the if (!unfiltered && ...)
block, call providerUtils.ModelMatchesDenylist(modelID, blacklistedModels) so
versioned publisher IDs (e.g., gemini-1.5-pro-001) are correctly caught; keep
the same !unfiltered guard and continue behavior.
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 786-800: The current logic builds keysByID and matchedKeys but
returns the unfiltered models when matchedKeys is empty, causing cross-provider
queries (e.g., keys=openai-key-1 on Anthropic) to return all models; change the
condition around matchedKeys so that if len(matchedKeys) == 0 and len(keyIDs) ==
0 you return models (unfiltered), but if len(matchedKeys) == 0 and len(keyIDs) >
0 you return an empty slice (no models match the requested keys); update the
branch in the block that references keysByID, matchedKeys, keyIDs, and models to
implement this behavior.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 222-242: The test id is currently on FormItem but needs to be on
the actual interactive control; update the usage of ModelMultiselect (where
provider={providerName} value={field.value || []} onChange={field.onChange}) to
accept and forward a data-testid (e.g., "apikey-blacklisted-models-input") and
remove the data-testid from FormItem; modify ModelMultiselect to apply that prop
to its trigger/input element so tests get a stable handle for the interactive
control.
---
Nitpick comments:
In `@core/bifrost.go`:
- Around line 6099-6107: Extract the duplicated model allowlist/blacklist logic
into a single helper (e.g., a function like supportsModel or a method on the key
type) that takes the key (k) and the model pointer/string and returns a bool
indicating whether the key supports that model; move the current inline checks
(the slices.Contains checks on k.BlacklistedModels and k.Models around model !=
nil && *model != "") into that helper and replace both occurrences (the current
block around k.BlacklistedModels/k.Models and the similar block at lines
6174-6205) to call this helper so single-key selection and batch/file selection
share the same decision logic.
In `@framework/configstore/rdb.go`:
- Line 290: Multiple write paths in rdb.go duplicate the mapping from
schemas.Key to tables.TableKey (e.g., the assignment using BlacklistedModels:
key.BlacklistedModels), which must be centralized to avoid drift; extract a
single helper function (e.g., MapKeyToTableKey or KeyToTableKey) that accepts
schemas.Key and returns tables.TableKey and replace the three duplicated mapping
sites with calls to that helper (ensure all fields of schemas.Key are mapped
consistently inside the new function and update references where currently
constructing tables.TableKey directly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3c59f1c-7f9c-4ee5-9bc3-6a541e1ac024
📒 Files selected for processing (41)
core/bifrost.gocore/bifrost_test.gocore/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/models.gocore/providers/azure/azure.gocore/providers/azure/models.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/models.gocore/providers/cohere/cohere.gocore/providers/cohere/models.gocore/providers/elevenlabs/elevenlabs.gocore/providers/elevenlabs/models.gocore/providers/gemini/gemini.gocore/providers/gemini/models.gocore/providers/huggingface/huggingface.gocore/providers/huggingface/models.gocore/providers/mistral/mistral.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openai/openai.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/replicate/replicate.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (10)
- core/changelog.md
- transports/bifrost-http/server/server.go
- core/providers/openrouter/openrouter.go
- ui/app/workspace/providers/views/providerKeyForm.tsx
- core/providers/huggingface/huggingface.go
- core/providers/elevenlabs/elevenlabs.go
- core/providers/replicate/replicate.go
- framework/configstore/migrations.go
- core/bifrost_test.go
- core/providers/openai/openai.go
🚧 Files skipped from review as they are similar to previous changes (16)
- core/schemas/account.go
- core/providers/azure/azure.go
- core/providers/vertex/utils.go
- core/providers/anthropic/anthropic.go
- core/providers/mistral/mistral.go
- framework/configstore/clientconfig.go
- ui/lib/types/schemas.ts
- core/providers/replicate/models.go
- core/providers/azure/models.go
- core/providers/vertex/vertex.go
- core/providers/utils/utils.go
- framework/configstore/tables/key.go
- core/providers/elevenlabs/models.go
- core/providers/openai/models.go
- core/providers/cohere/models.go
- core/providers/bedrock/models.go
Contributor
Confidence Score: 3/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
TejasGhatte
force-pushed
the
03-24-feat_blacklisted_models_field_at_provider_level
branch
from
f7750bf to
443cbc3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
core/providers/vertex/models.go (1)
212-214:⚠️ Potential issue | 🟡 MinorInconsistent blacklist matching: uses exact match instead of
ModelMatchesDenylist.This deployment backfill path uses
slices.Containsfor exact string matching, while line 180 usesproviderUtils.ModelMatchesDenylistwhich providesSameBaseModelsemantics.This inconsistency means blacklisting
gemini-1.5-prowould blockgemini-1.5-pro-001in the custom models pass but NOT in this deployment backfill pass.🔧 Proposed fix for consistency
- if slices.Contains(blacklistedModels, alias) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, alias) { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/models.go` around lines 212 - 214, The blacklist check in the deployment backfill uses slices.Contains(blacklistedModels, alias) which does exact matching; replace it with the same denylist matcher used elsewhere (providerUtils.ModelMatchesDenylist or ModelMatchesDenylist) so the check uses SameBaseModel semantics; specifically, change the condition that references slices.Contains and alias to call providerUtils.ModelMatchesDenylist(alias, blacklistedModels) (or the project's ModelMatchesDenylist helper) so entries like "gemini-1.5-pro" also block "gemini-1.5-pro-001".transports/bifrost-http/handlers/providers.go (1)
797-800:⚠️ Potential issue | 🟡 MinorDistinguish between "no keys requested" and "requested keys don't match this provider".
The current logic conflates two different scenarios:
- Empty
keyIDs: User didn't request key filtering → returning all models is correct- Non-empty
keyIDsbutlen(matchedKeys) == 0: User requested specific keys, but none belong to this provider → returning all models is incorrectFor
/api/models?keys=<key-from-provider-A>:
- Provider A should return filtered models ✓
- Provider B (whose keys don't match) currently returns ALL models, but should return
[]🔧 Proposed fix to handle the distinction
- // Unknown key IDs (or empty keyIDs): do not filter + // No keys requested: return all models (no filtering) + if len(keyIDs) == 0 { + return models + } + // Keys requested but none belong to this provider: return empty if len(matchedKeys) == 0 { - return models + return []string{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/providers.go` around lines 797 - 800, The current branch treats empty matchedKeys the same whether the client passed no keyIDs or passed keyIDs that don't belong to this provider; update the logic in the handler that uses matchedKeys/keyIDs (the block that currently does "if len(matchedKeys) == 0 { return models }") to first check if the incoming keyIDs slice is empty—if keyIDs is empty, continue returning models; otherwise (keyIDs non-empty but matchedKeys is empty) return an empty models slice. Ensure you reference the existing symbols matchedKeys, keyIDs and models when making this conditional change so providers that have no matching keys return [] instead of all models.
🧹 Nitpick comments (1)
core/providers/elevenlabs/models.go (1)
23-25: Replaceslices.ContainswithproviderUtils.ModelMatchesDenylistfor consistent base-model matching.All other providers in the codebase (anthropic, azure, bedrock, vertex) use
ModelMatchesDenylistfromcore/providers/utils/utils.gofor blacklist checks. This utility handles both exact string matching and base-model equivalence viaschemas.SameBaseModel, preventing model aliases from bypassing the blacklist.Lines 23–25 and 36–38 currently use
slices.Contains, which only performs exact matching. This creates an inconsistency and could miss blacklist entries for base-model aliases.Add the import and update both checks:
import ( "slices" "github.com/maximhq/bifrost/core/schemas" + providerUtils "github.com/maximhq/bifrost/core/providers/utils" )Then replace at line 23 and line 36.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/elevenlabs/models.go` around lines 23 - 25, Replace the exact-match blacklist checks that use slices.Contains with the utility that handles base-model equivalence: import providerUtils (from core/providers/utils) and replace both occurrences where code checks slices.Contains(blacklistedModels, model.ModelID) (and the similar check later) with providerUtils.ModelMatchesDenylist(blacklistedModels, model.ModelID, model.ModelName) so the denylist comparison uses schemas.SameBaseModel-aware logic; update the import block to include providerUtils and ensure both checks in core/providers/elevenlabs/models.go use ModelMatchesDenylist instead of slices.Contains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/azure/models.go`:
- Around line 150-152: The backfill currently only checks alias/allowed-model
names against the denylist, so entries whose deployment value is denylisted
still get added; update the denylist check in the backfill logic to call
providerUtils.ModelMatchesDenylist against both the alias and the mapped
deployment name (the deployment value used for registration) wherever the code
checks blacklistedModels (e.g., the blocks around
providerUtils.ModelMatchesDenylist(blacklistedModels, alias) at lines ~150 and
~165); if either check returns true, skip/continue. Add unit tests that exercise
a denylist containing the deployment value while the alias is allowed to confirm
the entry is omitted.
In `@core/providers/bedrock/models.go`:
- Around line 323-325: The current backfill branches only check the
alias/allowedModel against the denylist via
providerUtils.ModelMatchesDenylist(blacklistedModels, alias), so when Bedrock
omits the real deployment ID the synthesized entry can be re-added; update both
branches to call providerUtils.ModelMatchesDenylist for both the
alias/allowedModel and the resolved deployment target (the variable holding the
underlying deployment ID — e.g., deploymentID, resolvedDeployment, or similar)
before continuing/adding to listModels, and add a regression test that simulates
an alias-only backfill where the alias is allowed but the resolved deployment is
in blacklistedModels to ensure the entry is denied.
In `@core/providers/cohere/cohere.go`:
- Line 291: The Cohere model converter is still using exact matching
(slices.Contains) in functions inside core/providers/cohere/models.go, causing
versioned variants to bypass a base-model denylist; update those checks to call
providerUtils.ModelMatchesDenylist(modelName, key.BlacklistedModels) (or
equivalent signature used elsewhere) wherever slices.Contains or exact
comparisons are used, and add a regression test that adds a base-model denylist
entry (e.g., "command-r") then calls ToBifrostListModelsResponse / the Cohere
listing path to assert versioned variants are filtered out so listModels
behavior matches the deny-first matcher.
In `@core/providers/gemini/gemini.go`:
- Line 230: The denylist check in the Gemini conversion uses exact
slices.Contains matches and must be widened to catch base/variant forms; update
core/providers/gemini/models.go to replace the direct slices.Contains calls with
a helper like isBlacklisted(model string, blacklist []string) that returns true
when any blacklisted entry matches case-insensitively, or when the model is a
variant of a blacklisted base (e.g., blacklisted == base name and model has
delimiters like "-" or ":" or the blacklisted string is a prefix of the model),
and use that helper in both the main model filtering loop and the allowed-model
backfill logic so denylisted base/variant names cannot slip through (refer to
geminiResponse.ToBifrostListModelsResponse and key.BlacklistedModels to locate
call sites).
In `@core/providers/mistral/mistral.go`:
- Line 119: The blacklist check is too strict: change the exact-match
slices.Contains checks in core/providers/mistral/models.go (the filtering and
backfill code paths that call slices.Contains) to perform denylist-aware
matching for variants/base-models—e.g., normalize IDs (strip variant suffixes or
split on ":"/"/") and compare base IDs, or treat blacklist entries as prefixes
and use strings.HasPrefix (or a normalized equality) so a blacklisted base model
also matches its variants; update both the primary filtering and the backfill
logic that currently use slices.Contains to use this relaxed matching so
ToBifrostListModelsResponse(key.Models, key.BlacklistedModels) behaves
consistently.
In `@core/providers/vertex/models.go`:
- Around line 236-238: Replace the inconsistent and redundant check that
currently reads `if !unfiltered && slices.Contains(blacklistedModels,
allowedModel) { continue }` with a single call to the denylist helper: use
`providerUtils.ModelMatchesDenylist(allowedModel, blacklistedModels)` (or the
equivalent function signature used elsewhere) and remove the redundant
`!unfiltered` guard because this code already executes inside the `if
!unfiltered && len(allowedModels) > 0` block; in short, change the condition to
only call `providerUtils.ModelMatchesDenylist` for `allowedModel` against
`blacklistedModels` and continue when it returns true.
In `@core/providers/vertex/utils.go`:
- Around line 200-212: buildResponseFromConfig currently filters deployments
using an exact-match map (blacklistedSet[alias]) which is inconsistent with
models.go’s ModelMatchesDenylist behavior; replace the direct lookup in the
deployments loop in buildResponseFromConfig with a call to
providerUtils.ModelMatchesDenylist(alias, blacklistedModels) (and do the same
for the similar check around line 231) so base-model matches (SameBaseModel) are
respected, and add the providerUtils import to the file.
In `@transports/bifrost-http/server/server.go`:
- Around line 766-768: Blacklist filtering is being bypassed because when a Key
has Models == [] (deny-only) the existing logic leaves allowedModels empty which
is later treated as "unrestricted" and reintroduces blacklisted models; update
the loops that build allowedModels (look for references to key.Models,
key.BlacklistedModels, and allowedModels in the upsert/catalog handling) to
compute allowedModels as the intersection of either key.Models (if non-empty) or
discovered provider models, then subtract key.BlacklistedModels (i.e., allowed =
(key.Models if non-empty else discoveredModels) \ key.BlacklistedModels),
ensuring blacklist precedence so deny-only keys do not end up allowing
blacklisted models; apply the same change in the second loop noted (around the
other occurrence at lines similar to 1258-1260).
---
Duplicate comments:
In `@core/providers/vertex/models.go`:
- Around line 212-214: The blacklist check in the deployment backfill uses
slices.Contains(blacklistedModels, alias) which does exact matching; replace it
with the same denylist matcher used elsewhere
(providerUtils.ModelMatchesDenylist or ModelMatchesDenylist) so the check uses
SameBaseModel semantics; specifically, change the condition that references
slices.Contains and alias to call providerUtils.ModelMatchesDenylist(alias,
blacklistedModels) (or the project's ModelMatchesDenylist helper) so entries
like "gemini-1.5-pro" also block "gemini-1.5-pro-001".
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 797-800: The current branch treats empty matchedKeys the same
whether the client passed no keyIDs or passed keyIDs that don't belong to this
provider; update the logic in the handler that uses matchedKeys/keyIDs (the
block that currently does "if len(matchedKeys) == 0 { return models }") to first
check if the incoming keyIDs slice is empty—if keyIDs is empty, continue
returning models; otherwise (keyIDs non-empty but matchedKeys is empty) return
an empty models slice. Ensure you reference the existing symbols matchedKeys,
keyIDs and models when making this conditional change so providers that have no
matching keys return [] instead of all models.
---
Nitpick comments:
In `@core/providers/elevenlabs/models.go`:
- Around line 23-25: Replace the exact-match blacklist checks that use
slices.Contains with the utility that handles base-model equivalence: import
providerUtils (from core/providers/utils) and replace both occurrences where
code checks slices.Contains(blacklistedModels, model.ModelID) (and the similar
check later) with providerUtils.ModelMatchesDenylist(blacklistedModels,
model.ModelID, model.ModelName) so the denylist comparison uses
schemas.SameBaseModel-aware logic; update the import block to include
providerUtils and ensure both checks in core/providers/elevenlabs/models.go use
ModelMatchesDenylist instead of slices.Contains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4277d6b-58e9-46b7-a456-05d37002dfb8
📒 Files selected for processing (41)
core/bifrost.gocore/bifrost_test.gocore/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/models.gocore/providers/azure/azure.gocore/providers/azure/models.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/models.gocore/providers/cohere/cohere.gocore/providers/cohere/models.gocore/providers/elevenlabs/elevenlabs.gocore/providers/elevenlabs/models.gocore/providers/gemini/gemini.gocore/providers/gemini/models.gocore/providers/huggingface/huggingface.gocore/providers/huggingface/models.gocore/providers/mistral/mistral.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openai/openai.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/replicate/replicate.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (12)
- core/changelog.md
- ui/app/workspace/providers/views/providerKeyForm.tsx
- core/providers/huggingface/huggingface.go
- core/providers/replicate/replicate.go
- core/schemas/account.go
- core/bifrost_test.go
- core/providers/openai/openai.go
- core/providers/elevenlabs/elevenlabs.go
- core/providers/utils/utils.go
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- framework/configstore/tables/key.go
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (13)
- ui/lib/types/schemas.ts
- core/providers/openrouter/openrouter.go
- core/providers/anthropic/anthropic.go
- core/providers/azure/azure.go
- transports/bifrost-http/lib/config.go
- core/providers/openai/models.go
- framework/configstore/clientconfig.go
- core/providers/replicate/models.go
- core/providers/vertex/vertex.go
- core/providers/cohere/models.go
- core/providers/mistral/models.go
- core/providers/gemini/models.go
- core/bifrost.go
TejasGhatte
force-pushed
the
03-24-feat_blacklisted_models_field_at_provider_level
branch
from
443cbc3 to
c6da80e
Compare
TejasGhatte
force-pushed
the
03-24-feat_blacklisted_models_field_at_provider_level
branch
from
c6da80e to
a265fe5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/modelcatalog/main.go (1)
690-705:⚠️ Potential issue | 🟠 MajorThis fallback turns a per-key denylist into a provider-wide denylist.
If one key is unrestricted and another key blacklists
gpt-4o,allowedModelsis still empty, so this branch subtractsgpt-4ofromproviderModelsfor the whole provider. On a list-models fallback, the catalog will hide a model that the unrestricted key can still route. This needs “allowed by any key” evaluation, notproviderModels - union(deniedModels).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 690 - 705, The current fallback treats an empty allowedModels as "no restrictions for the whole provider" but then subtracts the union of all deniedModels from providerModels, causing a per-key deny to hide models for other unrestricted keys; change the logic so for each provider model you check "allowed-by-any-key": iterate providerModels and for each model determine if any API key would permit it (i.e., if any key's allowedModels includes the model, or the key has empty allowedModels and the model is not present in that key's deniedModels). Use the same parsing helper (schemas.ParseModelString) and the existing denied/allowed collections for keys to build per-key denied/allowed lookups, then append the model to finalModelList if at least one key permits it rather than subtracting the union of deniedModels.
♻️ Duplicate comments (2)
core/providers/bedrock/models.go (1)
323-325:⚠️ Potential issue | 🟡 MinorBackfill deny checks need the resolved deployment ID too.
Line 323 and Line 338 only test
alias/allowedModel. If a key blacklists the underlying deployment ID, these synthesized entries are still re-added when Bedrock omits them from the upstream response, so the catalog can expose a model that request-time routing will reject.🔧 Proposed fix
- if providerUtils.ModelMatchesDenylist(blacklistedModels, alias) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, alias, deploymentValue) { continue } @@ - if providerUtils.ModelMatchesDenylist(blacklistedModels, allowedModel) { + denyCandidates := []string{allowedModel} + if deploymentValue, ok := deployments[allowedModel]; ok { + denyCandidates = append(denyCandidates, deploymentValue) + } + if providerUtils.ModelMatchesDenylist(blacklistedModels, denyCandidates...) { continue }Also applies to: 338-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/models.go` around lines 323 - 325, The backfill logic currently only calls providerUtils.ModelMatchesDenylist(blacklistedModels, alias) (and later checks allowedModel) so synthesized catalog entries whose underlying deployment ID is blacklisted get re-added; update both places to also resolve and check the underlying deployment ID (e.g., resolvedDeploymentID or allowedModel.DeploymentID) against providerUtils.ModelMatchesDenylist(blacklistedModels, <deploymentID>) and skip/backfill only if neither the alias nor the resolved deployment ID is allowed; modify the branches that reference alias and allowedModel to perform this dual check before continuing or adding the synthesized entry.transports/bifrost-http/handlers/providers.go (1)
786-810:⚠️ Potential issue | 🟠 MajorFail closed when the requested keys do not belong to this provider.
Line 799 returns the full
modelsslice whenmatchedKeysis empty. With/api/models?keys=<provider-a-key>, that makes every other provider appear fully accessible because none of its keys match. Preserve the current "no filter" behavior only for an actually emptykeyIDsinput.🔧 Proposed fix
- // Unknown key IDs (or empty keyIDs): do not filter - if len(matchedKeys) == 0 { + if len(keyIDs) == 0 { return models } + if len(matchedKeys) == 0 { + return []string{} + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/providers.go` around lines 786 - 810, The current filter builds keysByID/matchedKeys from keyIDs and returns the full models slice when matchedKeys is empty, which incorrectly exposes all models for requests carrying unknown keys; change the logic in the model-filtering block (the code using keysByID, matchedKeys, keyIDs, models and keyAllowsModelForList) so that you only preserve the "no filter" behavior when the original keyIDs input is empty (len(keyIDs) == 0), but when keyIDs was provided and matchedKeys is empty return an empty slice (no models) to fail closed for unknown keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/clientconfig.go`:
- Around line 296-307: GenerateKeyHash currently omits key.BlacklistedModels so
denylist-only changes don't update the key hash; update GenerateKeyHash to
include the BlacklistedModels field in its hashing input (use the same
normalized empty-array vs nil logic used when building redactedConfig.Keys) so
changes to key.BlacklistedModels alter the resulting hash; reference the
GenerateKeyHash function and the key.BlacklistedModels property and ensure the
hash uses a deterministic serialization of BlacklistedModels (matching how
redactedConfig.Keys sets BlacklistedModels) when computing the hash.
In `@transports/bifrost-http/server/server.go`:
- Around line 533-549: The loop that builds allowedInKeys/deniedInKeys uses
slices.Contains on key.Models so base-model denies (e.g. "claude-3-5-sonnet")
don't block longer variants; replace the slices.Contains check with the same
base-model-aware deny matcher used by the provider converters (the same matching
utility/function the provider side uses) when deciding whether to include a
model in allowedInKeys, and ensure deniedInKeys is populated using that same
matcher too before calling
s.Config.ModelCatalog.UpsertModelDataForProvider(provider, allModels,
allowedInKeys, deniedInKeys); apply the same change to the other occurrences
noted (the blocks around lines 771-787 and 1268-1284).
---
Outside diff comments:
In `@framework/modelcatalog/main.go`:
- Around line 690-705: The current fallback treats an empty allowedModels as "no
restrictions for the whole provider" but then subtracts the union of all
deniedModels from providerModels, causing a per-key deny to hide models for
other unrestricted keys; change the logic so for each provider model you check
"allowed-by-any-key": iterate providerModels and for each model determine if any
API key would permit it (i.e., if any key's allowedModels includes the model, or
the key has empty allowedModels and the model is not present in that key's
deniedModels). Use the same parsing helper (schemas.ParseModelString) and the
existing denied/allowed collections for keys to build per-key denied/allowed
lookups, then append the model to finalModelList if at least one key permits it
rather than subtracting the union of deniedModels.
---
Duplicate comments:
In `@core/providers/bedrock/models.go`:
- Around line 323-325: The backfill logic currently only calls
providerUtils.ModelMatchesDenylist(blacklistedModels, alias) (and later checks
allowedModel) so synthesized catalog entries whose underlying deployment ID is
blacklisted get re-added; update both places to also resolve and check the
underlying deployment ID (e.g., resolvedDeploymentID or
allowedModel.DeploymentID) against
providerUtils.ModelMatchesDenylist(blacklistedModels, <deploymentID>) and
skip/backfill only if neither the alias nor the resolved deployment ID is
allowed; modify the branches that reference alias and allowedModel to perform
this dual check before continuing or adding the synthesized entry.
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 786-810: The current filter builds keysByID/matchedKeys from
keyIDs and returns the full models slice when matchedKeys is empty, which
incorrectly exposes all models for requests carrying unknown keys; change the
logic in the model-filtering block (the code using keysByID, matchedKeys,
keyIDs, models and keyAllowsModelForList) so that you only preserve the "no
filter" behavior when the original keyIDs input is empty (len(keyIDs) == 0), but
when keyIDs was provided and matchedKeys is empty return an empty slice (no
models) to fail closed for unknown keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 954b9150-9ce3-4d9d-b916-7a6d878dc353
📒 Files selected for processing (44)
core/bifrost.gocore/bifrost_test.gocore/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/models.gocore/providers/azure/azure.gocore/providers/azure/models.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/models.gocore/providers/cohere/cohere.gocore/providers/cohere/models.gocore/providers/elevenlabs/elevenlabs.gocore/providers/elevenlabs/models.gocore/providers/gemini/gemini.gocore/providers/gemini/models.gocore/providers/huggingface/huggingface.gocore/providers/huggingface/models.gocore/providers/mistral/mistral.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openai/openai.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/replicate/replicate.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.godocs/features/keys-management.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.goframework/modelcatalog/main.goplugins/governance/http_transport_prehook_test.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (8)
- core/changelog.md
- ui/lib/types/config.ts
- ui/app/workspace/providers/views/providerKeyForm.tsx
- core/providers/vertex/vertex.go
- core/providers/huggingface/huggingface.go
- core/bifrost_test.go
- core/providers/openai/openai.go
- framework/configstore/tables/key.go
🚧 Files skipped from review as they are similar to previous changes (19)
- transports/changelog.md
- core/providers/bedrock/bedrock.go
- core/providers/azure/models.go
- core/providers/openai/models.go
- core/providers/cohere/models.go
- core/providers/azure/azure.go
- core/providers/elevenlabs/elevenlabs.go
- core/providers/mistral/mistral.go
- core/providers/anthropic/anthropic.go
- core/providers/utils/utils.go
- core/providers/replicate/replicate.go
- core/providers/elevenlabs/models.go
- core/providers/huggingface/models.go
- core/providers/gemini/models.go
- core/providers/mistral/models.go
- core/providers/replicate/models.go
- core/providers/vertex/models.go
- core/providers/openrouter/openrouter.go
- core/bifrost.go
TejasGhatte
force-pushed
the
03-24-feat_blacklisted_models_field_at_provider_level
branch
from
a265fe5 to
8a32a8d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/modelcatalog/main.go (1)
667-705:⚠️ Potential issue | 🟠 MajorDenylist is skipped when
allowedModelsis non-empty.At Lines 667-687 and Line 690,
deniedModelsis only enforced in thelen(allowedModels) == 0path. In mixed allow+deny scenarios, denied models can still be added tofinalModelList, which breaks blacklist precedence.🔧 Suggested fix
func (mc *ModelCatalog) UpsertModelDataForProvider(provider schemas.ModelProvider, modelData *schemas.BifrostListModelsResponse, allowedModels []schemas.Model, deniedModels []schemas.Model) { @@ finalModelList := make([]string, 0) seenModels := make(map[string]bool) + deniedSet := make(map[string]struct{}, len(deniedModels)) + for _, d := range deniedModels { + _, deniedModel := schemas.ParseModelString(d.ID, "") + deniedSet[deniedModel] = struct{}{} + } @@ if len(modelData.Data) == 0 && len(allowedModels) > 0 { for _, allowedModel := range allowedModels { parsedProvider, parsedModel := schemas.ParseModelString(allowedModel.ID, "") if parsedProvider != provider { continue } + if _, denied := deniedSet[parsedModel]; denied { + continue + } if !seenModels[parsedModel] { seenModels[parsedModel] = true finalModelList = append(finalModelList, parsedModel) } } } for _, model := range modelData.Data { parsedProvider, parsedModel := schemas.ParseModelString(model.ID, "") if parsedProvider != provider { continue } + if _, denied := deniedSet[parsedModel]; denied { + continue + } if !seenModels[parsedModel] { seenModels[parsedModel] = true finalModelList = append(finalModelList, parsedModel) } } if len(allowedModels) == 0 { - deniedSet := make(map[string]struct{}, len(deniedModels)) - for _, d := range deniedModels { - _, modelName := schemas.ParseModelString(d.ID, "") - deniedSet[modelName] = struct{}{} - } for _, model := range providerModels { - if _, denied := deniedSet[model]; denied { + _, modelName := schemas.ParseModelString(model, "") + if _, denied := deniedSet[modelName]; denied { continue } if !seenModels[model] { seenModels[model] = true finalModelList = append(finalModelList, model) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 667 - 705, The current logic only applies deniedModels when len(allowedModels) == 0, allowing denied models through when allowedModels is non-empty; fix by building a deniedSet from deniedModels (using schemas.ParseModelString to get model names) before assembling finalModelList and check that set before appending in both loops that iterate allowedModels and modelData.Data (and also when appending providerModels) so that any model present in deniedSet is skipped even when allowedModels is provided; reuse seenModels to avoid duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/rdb.go`:
- Around line 1724-1729: Preload callbacks using Preload("ProviderConfigs.Keys",
...) are still projecting only models_json and must include
blacklisted_models_json so nested keys hydrate with denylist data; locate each
Preload("ProviderConfigs.Keys", func(db *gorm.DB) *gorm.DB { ... }) usage (used
by functions like GetVirtualKeys*, GetGovernanceConfig) and update the
Select(...) inside those callbacks to include blacklisted_models_json alongside
id, key_id, name, models_json, weight (matching the manual Select changes
already made).
In `@transports/bifrost-http/lib/config.go`:
- Around line 189-203: The legacy key-hash fallback in reconcileProviderKeys and
mergeProviderKeys omits the new fields BlacklistedModels, ReplicateKeyConfig,
and VLLMKeyConfig when calling GenerateKeyHash, causing unchanged inline keys to
appear modified; update both call-sites (in reconcileProviderKeys and
mergeProviderKeys) to include these fields in the hash input (alongside existing
fields like Models, AzureKeyConfig, VertexKeyConfig, BedrockKeyConfig, etc.) so
GenerateKeyHash computes the same ConfigHash for legacy/UI-created keys that
lack ConfigHash.
---
Outside diff comments:
In `@framework/modelcatalog/main.go`:
- Around line 667-705: The current logic only applies deniedModels when
len(allowedModels) == 0, allowing denied models through when allowedModels is
non-empty; fix by building a deniedSet from deniedModels (using
schemas.ParseModelString to get model names) before assembling finalModelList
and check that set before appending in both loops that iterate allowedModels and
modelData.Data (and also when appending providerModels) so that any model
present in deniedSet is skipped even when allowedModels is provided; reuse
seenModels to avoid duplicates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 317d8a86-2b25-429c-8f4d-cee93ad45580
📒 Files selected for processing (44)
core/bifrost.gocore/bifrost_test.gocore/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/models.gocore/providers/azure/azure.gocore/providers/azure/models.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/models.gocore/providers/cohere/cohere.gocore/providers/cohere/models.gocore/providers/elevenlabs/elevenlabs.gocore/providers/elevenlabs/models.gocore/providers/gemini/gemini.gocore/providers/gemini/models.gocore/providers/huggingface/huggingface.gocore/providers/huggingface/models.gocore/providers/mistral/mistral.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openai/openai.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/replicate/replicate.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.godocs/features/keys-management.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.goframework/modelcatalog/main.goplugins/governance/http_transport_prehook_test.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (12)
- plugins/governance/http_transport_prehook_test.go
- core/changelog.md
- ui/app/workspace/providers/views/providerKeyForm.tsx
- core/providers/anthropic/anthropic.go
- core/providers/cohere/cohere.go
- core/providers/huggingface/huggingface.go
- core/providers/elevenlabs/elevenlabs.go
- core/providers/replicate/replicate.go
- core/providers/utils/utils.go
- core/providers/vertex/vertex.go
- transports/bifrost-http/server/server.go
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (20)
- core/providers/openrouter/openrouter.go
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- core/providers/mistral/mistral.go
- core/providers/azure/azure.go
- ui/lib/types/config.ts
- docs/features/keys-management.mdx
- core/providers/gemini/models.go
- core/providers/elevenlabs/models.go
- core/providers/openai/openai.go
- core/providers/vertex/utils.go
- core/providers/huggingface/models.go
- core/providers/cohere/models.go
- core/providers/openai/models.go
- core/providers/mistral/models.go
- core/providers/replicate/models.go
- framework/configstore/migrations.go
- core/providers/bedrock/bedrock.go
- core/bifrost.go
- core/providers/anthropic/models.go
- core/providers/vertex/models.go
TejasGhatte
force-pushed
the
03-24-feat_blacklisted_models_field_at_provider_level
branch
from
8a32a8d to
e8ccb62
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/modelcatalog/main.go (1)
658-705:⚠️ Potential issue | 🟠 MajorMove the deny check ahead of every append path.
When
modelData.Datais empty and a model appears in both lists, Lines 668-676 still append it, and Lines 690-704 never run becauseallowedModelsis non-empty. That breaks the new deny-first behavior for the key-only fallback, and it also leaves this method dependent on upstream filtering formodelData.Data. Build the deny set once and gate every append through it so blacklist precedence holds consistently.Suggested fix
finalModelList := make([]string, 0) seenModels := make(map[string]bool) + deniedSet := make(map[string]struct{}, len(deniedModels)) + for _, deniedModel := range deniedModels { + _, deniedName := schemas.ParseModelString(deniedModel.ID, "") + deniedSet[deniedName] = struct{}{} + } + appendModel := func(model string) { + if _, denied := deniedSet[model]; denied { + return + } + if !seenModels[model] { + seenModels[model] = true + finalModelList = append(finalModelList, model) + } + } // Case where list models failed but we have allowed models from keys if len(modelData.Data) == 0 && len(allowedModels) > 0 { for _, allowedModel := range allowedModels { parsedProvider, parsedModel := schemas.ParseModelString(allowedModel.ID, "") if parsedProvider != provider { continue } - if !seenModels[parsedModel] { - seenModels[parsedModel] = true - finalModelList = append(finalModelList, parsedModel) - } + appendModel(parsedModel) } } for _, model := range modelData.Data { parsedProvider, parsedModel := schemas.ParseModelString(model.ID, "") if parsedProvider != provider { continue } - if !seenModels[parsedModel] { - seenModels[parsedModel] = true - finalModelList = append(finalModelList, parsedModel) - } + appendModel(parsedModel) } if len(allowedModels) == 0 { - deniedSet := make(map[string]struct{}, len(deniedModels)) - for _, d := range deniedModels { - _, modelName := schemas.ParseModelString(d.ID, "") - deniedSet[modelName] = struct{}{} - } for _, model := range providerModels { - if _, denied := deniedSet[model]; denied { - continue - } - if !seenModels[model] { - seenModels[model] = true - finalModelList = append(finalModelList, model) - } + appendModel(model) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 658 - 705, The deny-first logic is broken because you only build and check deniedModels when allowedModels is empty; create the deniedSet (from deniedModels using schemas.ParseModelString) once before assembling finalModelList and check it in every append path (both the allowed-models-only loop that iterates allowedModels and the loop over modelData.Data, as well as when iterating providerModels) so any candidate model is skipped if it exists in deniedSet; keep using seenModels to avoid duplicates and append parsedModel only when not denied and not seen.
♻️ Duplicate comments (4)
core/providers/cohere/models.go (1)
61-63:⚠️ Potential issue | 🟡 MinorUse the shared denylist matcher instead of exact string checks.
Line 61 and Line 76 use exact
slices.Contains, so base-model deny entries can still leak versioned variants in list output.Suggested fix
import ( "encoding/json" "slices" + providerUtils "github.com/maximhq/bifrost/core/providers/utils" "github.com/maximhq/bifrost/core/schemas" ) @@ - if !unfiltered && slices.Contains(blacklistedModels, model.Name) { + if !unfiltered && providerUtils.ModelMatchesDenylist(blacklistedModels, model.Name) { continue } @@ - if slices.Contains(blacklistedModels, allowedModel) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, allowedModel) { continue }Also applies to: 76-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/cohere/models.go` around lines 61 - 63, Replace the exact string checks using slices.Contains(blacklistedModels, model.Name) with the project’s shared denylist matcher so base-model deny entries also match versioned variants; locate the checks in the CoHere model listing code (the blocks referencing blacklistedModels and model.Name) and call the shared matcher function (the common denylist/matcher utility used elsewhere in the repo) to test model.Name, updating imports as needed and applying the same change to both occurrences.core/providers/bedrock/models.go (1)
323-325:⚠️ Potential issue | 🟡 MinorBackfill deny checks should include resolved deployment targets too.
Line 323 and Line 338 only check alias/allowed model names. If the denylist contains the underlying deployment value, a denied entry can still be backfilled.
Suggested fix
- if providerUtils.ModelMatchesDenylist(blacklistedModels, alias) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, alias, deploymentValue) { continue } @@ - if providerUtils.ModelMatchesDenylist(blacklistedModels, allowedModel) { + if providerUtils.ModelMatchesDenylist(blacklistedModels, allowedModel, deployments[allowedModel]) { continue }Also applies to: 338-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/models.go` around lines 323 - 325, The denylist check currently calls providerUtils.ModelMatchesDenylist(blacklistedModels, alias) which only tests the alias; update the check(s) to also test the resolved deployment target/value so entries denied by their underlying deployment cannot be backfilled (apply the change wherever alias is checked, including the other occurrence around lines 338-340). Concretely, call providerUtils.ModelMatchesDenylist for both alias and the resolved deployment identifier (e.g., deployment/target variable used in the same scope) and continue if either call returns true.core/providers/gemini/gemini.go (1)
230-230:⚠️ Potential issue | 🟠 MajorDenylist precedence is still exact-only in the Gemini list-model conversion path.
Line 230 correctly passes
key.BlacklistedModels, but the downstream filtering incore/providers/gemini/models.gostill uses exactslices.Containschecks. Base deny entries can miss variant model names, so denylisted variants may still appear in list-model results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/gemini.go` at line 230, The Gemini list-model conversion is only doing exact-match denylist checks (using slices.Contains) so variants of denylisted model names slip through; update the filtering in core/providers/gemini/models.go (the code path invoked by geminiResponse.ToBifrostListModelsResponse) to treat denylist entries as prefixes/variants rather than exact strings — replace slices.Contains checks with a function that iterates blacklisted entries and rejects any model where strings.HasPrefix(model, blacklisted) (or otherwise normalizes/regex-matches variants as your denylist semantics require) while preserving the existing key.BlacklistedModels argument and behavior for exact matches.ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
222-242:⚠️ Potential issue | 🟡 MinorPut the test id on the
ModelMultiselectcontrol.Line 222 tags the
FormItem, but the new interactive element is theModelMultiselect. Tests still need a stable selector on the trigger/input itself, e.g.apikey-blacklisted-models-input; if the component does not accept one yet, thread a prop through instead of relying on descendant selectors.As per coding guidelines, "Add new interactive UI elements with
data-testidattributes following the pattern:data-testid=\"<entity>-<element>-<qualifier>\"."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 222 - 242, The FormItem currently holds the data-testid but the interactive control is ModelMultiselect; update the JSX so the data-testid="apikey-blacklisted-models-input" is placed on the ModelMultiselect component (or thread a new prop like inputTestId through to ModelMultiselect if it doesn't accept arbitrary props) and leave FormItem test id removed or kept for container-level checks; modify the call to ModelMultiselect (the instance using provider={providerName} value={field.value} onChange={field.onChange}) to accept and pass that test id to the actual input/trigger element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 189-203: The current logic treats a key as a reference whenever
tableKey.Value.GetValue() == "", which skips keys that have empty Value but
contain inline provider configs (AzureKeyConfig, VertexKeyConfig,
BedrockKeyConfig, VLLMKeyConfig, ReplicateKeyConfig), so the block that appends
schemas.Key to keysToAddToProvider never propagates those keys into
cd.Providers; update the discriminator check used before appending to consider
any non-nil/non-empty provider-specific config (tableKey.AzureKeyConfig,
tableKey.VertexKeyConfig, tableKey.BedrockKeyConfig, tableKey.VLLMKeyConfig,
tableKey.ReplicateKeyConfig) or a non-empty Value as indicating an inline key,
and only treat keys as references when all of those are absent/empty so that
keys created with provider configs are copied into keysToAddToProvider and
ultimately cd.Providers.
---
Outside diff comments:
In `@framework/modelcatalog/main.go`:
- Around line 658-705: The deny-first logic is broken because you only build and
check deniedModels when allowedModels is empty; create the deniedSet (from
deniedModels using schemas.ParseModelString) once before assembling
finalModelList and check it in every append path (both the allowed-models-only
loop that iterates allowedModels and the loop over modelData.Data, as well as
when iterating providerModels) so any candidate model is skipped if it exists in
deniedSet; keep using seenModels to avoid duplicates and append parsedModel only
when not denied and not seen.
---
Duplicate comments:
In `@core/providers/bedrock/models.go`:
- Around line 323-325: The denylist check currently calls
providerUtils.ModelMatchesDenylist(blacklistedModels, alias) which only tests
the alias; update the check(s) to also test the resolved deployment target/value
so entries denied by their underlying deployment cannot be backfilled (apply the
change wherever alias is checked, including the other occurrence around lines
338-340). Concretely, call providerUtils.ModelMatchesDenylist for both alias and
the resolved deployment identifier (e.g., deployment/target variable used in the
same scope) and continue if either call returns true.
In `@core/providers/cohere/models.go`:
- Around line 61-63: Replace the exact string checks using
slices.Contains(blacklistedModels, model.Name) with the project’s shared
denylist matcher so base-model deny entries also match versioned variants;
locate the checks in the CoHere model listing code (the blocks referencing
blacklistedModels and model.Name) and call the shared matcher function (the
common denylist/matcher utility used elsewhere in the repo) to test model.Name,
updating imports as needed and applying the same change to both occurrences.
In `@core/providers/gemini/gemini.go`:
- Line 230: The Gemini list-model conversion is only doing exact-match denylist
checks (using slices.Contains) so variants of denylisted model names slip
through; update the filtering in core/providers/gemini/models.go (the code path
invoked by geminiResponse.ToBifrostListModelsResponse) to treat denylist entries
as prefixes/variants rather than exact strings — replace slices.Contains checks
with a function that iterates blacklisted entries and rejects any model where
strings.HasPrefix(model, blacklisted) (or otherwise normalizes/regex-matches
variants as your denylist semantics require) while preserving the existing
key.BlacklistedModels argument and behavior for exact matches.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 222-242: The FormItem currently holds the data-testid but the
interactive control is ModelMultiselect; update the JSX so the
data-testid="apikey-blacklisted-models-input" is placed on the ModelMultiselect
component (or thread a new prop like inputTestId through to ModelMultiselect if
it doesn't accept arbitrary props) and leave FormItem test id removed or kept
for container-level checks; modify the call to ModelMultiselect (the instance
using provider={providerName} value={field.value} onChange={field.onChange}) to
accept and pass that test id to the actual input/trigger element.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c8766ee-0f11-467d-90d4-e2d6da70a200
📒 Files selected for processing (44)
core/bifrost.gocore/bifrost_test.gocore/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/models.gocore/providers/azure/azure.gocore/providers/azure/models.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/models.gocore/providers/cohere/cohere.gocore/providers/cohere/models.gocore/providers/elevenlabs/elevenlabs.gocore/providers/elevenlabs/models.gocore/providers/gemini/gemini.gocore/providers/gemini/models.gocore/providers/huggingface/huggingface.gocore/providers/huggingface/models.gocore/providers/mistral/mistral.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openai/openai.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/replicate/replicate.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.godocs/features/keys-management.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.goframework/modelcatalog/main.goplugins/governance/http_transport_prehook_test.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (7)
- core/changelog.md
- plugins/governance/http_transport_prehook_test.go
- core/providers/anthropic/anthropic.go
- core/providers/huggingface/huggingface.go
- core/providers/utils/utils.go
- core/providers/mistral/mistral.go
- ui/app/workspace/providers/views/providerKeyForm.tsx
🚧 Files skipped from review as they are similar to previous changes (23)
- core/providers/elevenlabs/elevenlabs.go
- ui/lib/types/config.ts
- transports/changelog.md
- ui/lib/types/schemas.ts
- core/providers/openrouter/openrouter.go
- core/providers/elevenlabs/models.go
- core/providers/vertex/vertex.go
- core/providers/mistral/models.go
- core/providers/openai/models.go
- core/providers/replicate/models.go
- docs/features/keys-management.mdx
- core/providers/gemini/models.go
- core/providers/vertex/utils.go
- framework/configstore/migrations.go
- core/providers/openai/openai.go
- transports/bifrost-http/handlers/providers.go
- core/providers/anthropic/models.go
- core/providers/huggingface/models.go
- core/providers/azure/azure.go
- core/bifrost.go
- core/providers/vertex/models.go
- framework/configstore/rdb.go
- core/providers/replicate/replicate.go
TejasGhatte
force-pushed
the
03-24-feat_blacklisted_models_field_at_provider_level
branch
from
e8ccb62 to
76648fc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/bifrost-http/lib/config.go (1)
3021-3037:⚠️ Potential issue | 🟠 MajorClone the model slices before returning them.
Line 3025 adds
BlacklistedModelsby reusing the slice stored inc.Providers, andModelsis still passed through the same way. That means the returnedTableKeycan mutate live in-memory config if any caller edits the result afterGetAllKeys()returns. Please clone both slices here before appending.Suggested fix
- models := key.Models + models := slices.Clone(key.Models) if models == nil { models = []string{} } - blacklisted := key.BlacklistedModels + blacklisted := slices.Clone(key.BlacklistedModels) if blacklisted == nil { blacklisted = []string{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config.go` around lines 3021 - 3037, The GetAllKeys() path is returning the original slices from c.Providers into configstoreTables.TableKey (fields Models and BlacklistedModels), allowing callers to mutate live config; clone both slices before assigning to TableKey by creating new slice copies of the local variables `models` and `blacklisted` (e.g., allocate a new []string and copy/append the items) so TableKey.Models and TableKey.BlacklistedModels hold independent copies; update the code around `models := key.Models` / `blacklisted := key.BlacklistedModels` and the place constructing `configstoreTables.TableKey` to use the cloned slices.framework/modelcatalog/main.go (1)
629-705:⚠️ Potential issue | 🟠 MajorA flat provider-wide deny set breaks multi-key fallback.
The server passes
deniedModelshere as a union across all keys for the provider, so Line 691 turns “blacklisted on some key” into “unavailable for the provider.” With two keys that blacklist different models, this removes both frommodelPooleven though each model is still reachable through the other key. This needs per-key availability aggregation; only drop a model when no enabled key can serve it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 629 - 705, UpsertModelDataForProvider currently treats deniedModels as a provider-wide union which incorrectly removes models that are only blacklisted by some keys; change the function to aggregate availability per key instead: modify the signature or caller to accept deniedModels grouped by key (e.g., map[string][]schemas.Model or []schemas.Model with Key/Source identifier) and in UpsertModelDataForProvider build a model->availableKeyCount map by iterating each key's allowed/denied list (use schemas.ParseModelString to get model names) and only exclude a model when availableKeyCount[model]==0; update the branch that currently builds deniedSet (the code around deniedModels handling and finalModelList population) to consult this per-key availability map before skipping providerModels and ensure modelPool[provider] is populated from finalModelList.
♻️ Duplicate comments (1)
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
222-242:⚠️ Potential issue | 🟡 MinorPut the new test id on
ModelMultiselect, not on the wrapper.Attaching
data-testidtoFormItemstill leaves the actual chooser/trigger unaddressable, so tests have to drill into descendants again. Pass a prop throughModelMultiselectand apply it to the interactive input/trigger instead, e.g.apikey-blacklisted-models-input.As per coding guidelines, "Add new interactive UI elements with
data-testidattributes following the pattern:data-testid=\"<entity>-<element>-<qualifier>\"."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 222 - 242, The data-testid should be moved from the FormItem wrapper to the interactive component so tests can target the chooser directly: update the JSX where ModelMultiselect is rendered (currently using provider={providerName} value={field.value} onChange={field.onChange} unfiltered={true}) to accept a new prop (e.g. dataTestId or testId) and pass "apikey-blacklisted-models-input"; then modify ModelMultiselect to apply that prop to its actual input/trigger element so the interactive control is addressable by tests instead of placing data-testid on FormItem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/vertex/models.go`:
- Around line 179-181: The denylist check currently uses exact matching with
slices.Contains on customModelID and deploymentAlias against blacklistedModels;
replace that with the shared matching helper (providerUtils.ModelMatchesDenylist
or equivalent) to ensure version-suffix and variant-aware matching for both
customModelID and deploymentAlias, i.e., use
providerUtils.ModelMatchesDenylist(blacklistedModels, customModelID) and
providerUtils.ModelMatchesDenylist(blacklistedModels, deploymentAlias) in place
of slices.Contains so behavior is consistent with other providers.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 225-230: Replace the non-focusable <span> used as the
TooltipTrigger child with a focusable button: inside the Tooltip >
TooltipTrigger block (symbols: TooltipProvider, Tooltip, TooltipTrigger, Info)
render a <button type="button"> that includes an accessible label (aria-label or
visually hidden text) and add the required data-testid attribute so keyboard
users can focus the trigger and tests can target it; ensure styling and the Info
icon remain the same and that no default form submit behavior is introduced.
---
Outside diff comments:
In `@framework/modelcatalog/main.go`:
- Around line 629-705: UpsertModelDataForProvider currently treats deniedModels
as a provider-wide union which incorrectly removes models that are only
blacklisted by some keys; change the function to aggregate availability per key
instead: modify the signature or caller to accept deniedModels grouped by key
(e.g., map[string][]schemas.Model or []schemas.Model with Key/Source identifier)
and in UpsertModelDataForProvider build a model->availableKeyCount map by
iterating each key's allowed/denied list (use schemas.ParseModelString to get
model names) and only exclude a model when availableKeyCount[model]==0; update
the branch that currently builds deniedSet (the code around deniedModels
handling and finalModelList population) to consult this per-key availability map
before skipping providerModels and ensure modelPool[provider] is populated from
finalModelList.
In `@transports/bifrost-http/lib/config.go`:
- Around line 3021-3037: The GetAllKeys() path is returning the original slices
from c.Providers into configstoreTables.TableKey (fields Models and
BlacklistedModels), allowing callers to mutate live config; clone both slices
before assigning to TableKey by creating new slice copies of the local variables
`models` and `blacklisted` (e.g., allocate a new []string and copy/append the
items) so TableKey.Models and TableKey.BlacklistedModels hold independent
copies; update the code around `models := key.Models` / `blacklisted :=
key.BlacklistedModels` and the place constructing `configstoreTables.TableKey`
to use the cloned slices.
---
Duplicate comments:
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 222-242: The data-testid should be moved from the FormItem wrapper
to the interactive component so tests can target the chooser directly: update
the JSX where ModelMultiselect is rendered (currently using
provider={providerName} value={field.value} onChange={field.onChange}
unfiltered={true}) to accept a new prop (e.g. dataTestId or testId) and pass
"apikey-blacklisted-models-input"; then modify ModelMultiselect to apply that
prop to its actual input/trigger element so the interactive control is
addressable by tests instead of placing data-testid on FormItem.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d41fd6b1-670d-42a3-a5dc-7133cf755751
📒 Files selected for processing (44)
core/bifrost.gocore/bifrost_test.gocore/changelog.mdcore/providers/anthropic/anthropic.gocore/providers/anthropic/models.gocore/providers/azure/azure.gocore/providers/azure/models.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/models.gocore/providers/cohere/cohere.gocore/providers/cohere/models.gocore/providers/elevenlabs/elevenlabs.gocore/providers/elevenlabs/models.gocore/providers/gemini/gemini.gocore/providers/gemini/models.gocore/providers/huggingface/huggingface.gocore/providers/huggingface/models.gocore/providers/mistral/mistral.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openai/openai.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/replicate/replicate.gocore/providers/utils/utils.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.godocs/features/keys-management.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.goframework/modelcatalog/main.goplugins/governance/http_transport_prehook_test.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (6)
- core/changelog.md
- core/schemas/account.go
- ui/app/workspace/providers/views/providerKeyForm.tsx
- ui/lib/types/config.ts
- docs/features/keys-management.mdx
- core/bifrost.go
🚧 Files skipped from review as they are similar to previous changes (24)
- core/providers/openrouter/openrouter.go
- core/providers/cohere/cohere.go
- core/providers/huggingface/huggingface.go
- core/providers/vertex/utils.go
- plugins/governance/http_transport_prehook_test.go
- transports/changelog.md
- core/providers/elevenlabs/elevenlabs.go
- core/providers/replicate/replicate.go
- core/providers/replicate/models.go
- core/providers/elevenlabs/models.go
- core/providers/vertex/vertex.go
- core/bifrost_test.go
- core/providers/openai/openai.go
- core/providers/openai/models.go
- core/providers/azure/models.go
- framework/configstore/rdb.go
- framework/configstore/clientconfig.go
- core/providers/gemini/models.go
- core/providers/huggingface/models.go
- core/providers/bedrock/models.go
- transports/bifrost-http/server/server.go
- core/providers/cohere/models.go
- transports/bifrost-http/handlers/providers.go
- core/providers/anthropic/models.go
Contributor
Merge activity
|
akshaydeo
deleted the
03-24-feat_blacklisted_models_field_at_provider_level
branch
This was referenced Mar 26, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds support for blacklisted models at the key level, allowing fine-grained control over which models specific API keys cannot access. The blacklist takes precedence over the allowed models list, providing a deny-first approach to model access control.
Changes
BlacklistedModelsfield to theKeyschema and database tablebifrost.goto check blacklisted models before allowed modelsblacklisted_models_jsoncolumnType of change
Affected areas
How to test
Test the blacklist functionality with different key configurations:
Test scenarios:
Screenshots/Recordings
The UI now includes a "Blacklisted models" field in the API key configuration form, allowing users to specify models that should never be used with that particular key.
Breaking changes
This is a backward-compatible addition. Existing keys without blacklisted models will continue to work as before.
Related issues
Enables more granular model access control for enterprise deployments where certain keys should be restricted from accessing specific models.
Security considerations
This feature enhances security by providing explicit deny capabilities for model access. The blacklist takes precedence over allow lists, ensuring that accidentally allowing a blacklisted model cannot override the security restriction.
Checklist
docs/contributing/README.mdand followed the guidelines