refactor: standardize empty array conventions for VK Provider Config Allowed Keys#2006
Merged
Pratham-Mishra04 merged 1 commit intov1.5.0from Mar 18, 2026
Conversation
This was referenced Mar 10, 2026
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
|
|
Pratham-Mishra04
changed the title
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdd an allow_all_keys boolean to virtual key provider configs, backfill it via a DB migration (set true for prior empty-key configs and recompute config_hash), propagate the flag through HTTP handlers, resolver flow, UI, tests, and context utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Handler as Bifrost HTTP Handler
participant DB as Database
participant Resolver
rect rgba(200,230,255,0.5)
User->>Handler: POST/PUT /virtual-keys (provider_configs with key_ids = ["*"] or [])
Handler->>Handler: compute allowAllKeys = (key_ids == ["*"])
Handler->>DB: create/update provider_config (allow_all_keys, keys...)
DB-->>Handler: persist result
Handler-->>User: 200 Created/Updated
end
rect rgba(200,255,200,0.5)
User->>Resolver: EvaluateVirtualKeyRequest(ctx, virtualKey, provider, model, requestType, flag)
Resolver->>DB: fetch provider_config (allow_all_keys, keys)
DB-->>Resolver: provider_config
alt allow_all_keys == true
Resolver->>Resolver: clear includeOnlyKeys (no key filtering)
else allow_all_keys == false
Resolver->>Resolver: build includeOnlyKeys from config.keys (apply filter)
end
Resolver-->>User: evaluation result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
📝 Coding Plan
Comment |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/governance.go (1)
404-415:⚠️ Potential issue | 🟠 MajorTreat any
key_idspayload containing"*"as allow-all.The new stack semantics are “contains
*=> wildcard”, but these branches only recognize the single-element form. A request like["*", "key-id"]now falls through the key lookup path, so"*"is handled like a concrete key ID instead of allow-all.Suggested fix
- allowAllKeys := false - if len(pc.KeyIDs) == 1 && pc.KeyIDs[0] == "*" { - allowAllKeys = true - } else if len(pc.KeyIDs) > 0 { + allowAllKeys := false + for _, keyID := range pc.KeyIDs { + if keyID == "*" { + allowAllKeys = true + break + } + } + if !allowAllKeys && len(pc.KeyIDs) > 0 { var err error keys, err = h.configStore.GetKeysByIDs(ctx, pc.KeyIDs) if err != nil {Also applies to: 730-742, 805-819
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 404 - 415, The code treats allow-all only when pc.KeyIDs is exactly ["*"], but must treat any pc.KeyIDs containing "*" as wildcard; update the branches around pc.KeyIDs (the condition that sets allowAllKeys and the subsequent call to h.configStore.GetKeysByIDs) to check for presence of "*" (e.g., iterate pc.KeyIDs or use a helper like containsWildcard) and if found set allowAllKeys = true and skip GetKeysByIDs/length checks; apply the same change to the other similar blocks handling pc.KeyIDs (the blocks around lines 730-742 and 805-819) so "*" anywhere in the slice is treated as allow-all.
🤖 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/tables/virtualkey.go`:
- Around line 64-74: The code currently treats a missing allowed_keys the same
as an explicit empty list, causing omission to become deny-all; modify the
deserialization/handling so you can detect whether allowed_keys was present in
the JSON and only apply the new deny-by-default semantics when the field was
actually provided. Concretely: change the temporary struct used during unmarshal
to track presence (e.g., add an AllowedKeysPresent bool or make AllowedKeys a
pointer like *[]string), then in the branch that inspects temp.AllowedKeys only
proceed if AllowedKeysPresent (or temp.AllowedKeys != nil); if present and
len==1 and "*" set pc.AllowAllKeys=true, if present and len==0 set pc.Keys=nil
and pc.AllowAllKeys=false (explicit empty = deny-all), otherwise map values into
pc.Keys; if not present leave pc.Keys and pc.AllowAllKeys unchanged. Use
temp.AllowedKeys, temp.AllowedKeysPresent (or pointer), pc.Keys, and
pc.AllowAllKeys to implement this.
In `@plugins/governance/resolver.go`:
- Around line 237-249: The loop over vk.ProviderConfigs sets
schemas.BifrostContextKeyGovernanceIncludeOnlyKeys only when pc.AllowAllKeys is
false, leaving any previous include-only slice in ctx when AllowAllKeys is true;
update the branch for AllowAllKeys in the provider-config handling (the loop
that inspects pc and uses ctx.SetValue) to explicitly clear/overwrite the
context key (e.g., set nil or an empty slice via
ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil) or an
empty []string{}) before breaking so stale filters are removed; apply the same
change to the other identical block handling provider configs (the second
occurrence around the other provider-config loop).
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 663-665: The filter that builds disallowed uses key.models without
guarding for undefined, which can throw; update the predicate in the
allowedModels.filter to safely handle absent models by checking key.models first
(e.g., treat missing models as empty: (!key.models || key.models.length === 0 ||
key.models.includes(model))) so the expression referencing key.models (in the
selectedKeys.some callback) never calls .length or .includes on undefined.
- Around line 680-714: The UI currently bails out when providerKeys.length ===
0, hiding the Allowed Keys picker even though allKeyOptions contains the
wildcard; remove or change the early "if (providerKeys.length === 0) return
null;" so the component always renders the Allowed Keys block (Label,
description and AsyncMultiSelect) and ensure selectedProviderKeys and
allKeyOptions logic still work when providerKeys is empty (hasWildcard should
still select the wildcard option and providerKeys.map will produce no concrete
options). This preserves the ability to opt into future keys and to display
allow_all_keys state after concrete keys disappear.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 404-415: The code treats allow-all only when pc.KeyIDs is exactly
["*"], but must treat any pc.KeyIDs containing "*" as wildcard; update the
branches around pc.KeyIDs (the condition that sets allowAllKeys and the
subsequent call to h.configStore.GetKeysByIDs) to check for presence of "*"
(e.g., iterate pc.KeyIDs or use a helper like containsWildcard) and if found set
allowAllKeys = true and skip GetKeysByIDs/length checks; apply the same change
to the other similar blocks handling pc.KeyIDs (the blocks around lines 730-742
and 805-819) so "*" anywhere in the slice is treated as allow-all.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89579dd3-62de-4907-a78b-bc0032228b4c
📒 Files selected for processing (10)
framework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
0c1f9ca to
a7173c4
Compare
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
785179a to
5a214b3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/governance/resolver.go (1)
237-249:⚠️ Potential issue | 🟠 MajorClear
BifrostContextKeyGovernanceIncludeOnlyKeyson the allow-all path.When
pc.AllowAllKeysis true, these branches leave any previously stored include-only slice untouched. If the sameBifrostContextwas populated by an earlier evaluation, the stale filter still constrains key selection and "all keys" silently becomes "previous keys only". Please overwrite/clear this context value on every matching provider config; reserve an empty slice only for the explicit deny-by-default case.Also applies to: 312-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/resolver.go` around lines 237 - 249, The provider-config branch leaves a previously-set include-only keys filter in context when pc.AllowAllKeys is true; update the matching-provider branch in the loop over vk.ProviderConfigs to explicitly clear the schemas.BifrostContextKeyGovernanceIncludeOnlyKeys value (e.g., call ctx.SetValue with nil/empty removal) when pc.AllowAllKeys == true so the "allow all" path cannot be silently constrained by stale data; keep the current behavior of setting a non-nil empty slice only for the explicit deny-by-default case (same change also needs to be applied to the other matching block around the 312-322 region).
🤖 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/migrations.go`:
- Around line 4267-4291: This backfill flips allow_all_keys on
tables.TableVirtualKeyProviderConfig rows but does not refresh
governance_virtual_keys.config_hash, leaving stale hashes; collect all
virtual_key_id values affected (query
governance_virtual_key_provider_config_keys for provider_config ids you update),
dedupe them, and then recompute/update their governance_virtual_keys.config_hash
using the same logic/helper used by migrationBackfillEmptyVirtualKeyConfigs so
the touched VirtualKey rows get their config_hash regenerated (update
governance_virtual_keys for the collected IDs after the backfill).
---
Duplicate comments:
In `@plugins/governance/resolver.go`:
- Around line 237-249: The provider-config branch leaves a previously-set
include-only keys filter in context when pc.AllowAllKeys is true; update the
matching-provider branch in the loop over vk.ProviderConfigs to explicitly clear
the schemas.BifrostContextKeyGovernanceIncludeOnlyKeys value (e.g., call
ctx.SetValue with nil/empty removal) when pc.AllowAllKeys == true so the "allow
all" path cannot be silently constrained by stale data; keep the current
behavior of setting a non-nil empty slice only for the explicit deny-by-default
case (same change also needs to be applied to the other matching block around
the 312-322 region).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03b543f1-52a0-4021-bf84-1b9e88893d9b
📒 Files selected for processing (10)
framework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/lib/types/governance.ts
- plugins/governance/changelog.md
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- framework/configstore/tables/virtualkey.go
12 tasks
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
5a214b3 to
e68b8cf
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
a7173c4 to
237dc14
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
framework/configstore/tables/virtualkey.go (1)
64-76:⚠️ Potential issue | 🟠 MajorPreserve omitted
allowed_keysduring unmarshal.A missing
allowed_keysstill deserializes the same as[], so syncing a legacyconfig.jsonthat omits the field will flip that provider config to deny-all on the next reconcile. Track field presence with*[]string(or equivalent) so only an explicit empty array means deny-all.Suggested fix
type Alias TableVirtualKeyProviderConfig type TempProviderConfig struct { Alias - AllowedKeys []string `json:"allowed_keys"` // Config file format: array of key names + AllowedKeys *[]string `json:"allowed_keys"` // nil = omitted, [] = explicit deny-all } @@ - if len(temp.AllowedKeys) > 0 && len(pc.Keys) == 0 { - // Check for wildcard — ["*"] means allow all keys - if len(temp.AllowedKeys) == 1 && temp.AllowedKeys[0] == "*" { + if temp.AllowedKeys != nil && len(pc.Keys) == 0 { + allowedKeys := *temp.AllowedKeys + pc.AllowAllKeys = false + pc.Keys = nil + + // Check for wildcard — ["*"] means allow all keys + if len(allowedKeys) == 1 && allowedKeys[0] == "*" { pc.AllowAllKeys = true - } else { - pc.Keys = make([]TableKey, len(temp.AllowedKeys)) - for i, keyName := range temp.AllowedKeys { + } else if len(allowedKeys) > 0 { + pc.Keys = make([]TableKey, len(allowedKeys)) + for i, keyName := range allowedKeys { pc.Keys[i] = TableKey{Name: keyName} } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/virtualkey.go` around lines 64 - 76, The unmarshalling currently treats a missing allowed_keys the same as an empty slice, causing configs that omit the field to be treated as deny-all; change the temporary unmarshal type used in virtualkey.go so AllowedKeys is a pointer to a slice (e.g. *[]string) and update the logic in the block that examines temp.AllowedKeys (the code that currently checks len(temp.AllowedKeys) > 0 && len(pc.Keys) == 0) to first test for nil (meaning field was omitted) and only apply the wildcard/empty-array handling when temp.AllowedKeys != nil; ensure you still set pc.AllowAllKeys for ["*"] and populate pc.Keys when the pointer is non-nil and has entries.
🧹 Nitpick comments (1)
transports/changelog.md (1)
1-1: Consider clarifying backwards compatibility and improving readability.The changelog entry accurately describes the feature behavior but could be enhanced for users upgrading:
Missing migration context: The entry doesn't mention that this introduces a database migration with automatic backfill. Users should know existing VK configs will be preserved (
allow_all_keys=truebackfilled automatically).Readability: The entry is quite long for a single line. Consider splitting into sub-bullets:
- feat: VK provider config key_ids now supports wildcard and deny-by-default semantics - `["*"]` wildcard allows all keys (resolved to AllowAllKeys flag, no DB lookups) - Empty key_ids denies all keys - Non-breaking: migration backfills existing configs with allow_all_keys=true to preserve current behaviorThis helps operators understand both the feature and upgrade implications at a glance. As per coding guidelines, since this PR is part of a stack, ensure this changelog entry covers the full scope of the stack's VK provider config changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` at line 1, Update the changelog entry for the VK provider config (referencing key_ids and AllowAllKeys) to explicitly note the DB migration and automatic backfill behavior (existing VK configs are preserved by backfilling allow_all_keys=true), and improve readability by splitting the single long line into a short header plus sub-bullets that state: `["*"]` resolves to AllowAllKeys (no DB lookups), empty key_ids denies all, and the non-breaking migration/backfill behavior for existing configs; ensure the entry covers the full stack scope of VK provider config changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/governance/changelog.md`:
- Line 1: Update the changelog entry to explicitly document the external
wildcard contract by noting that callers express allow-all via key_ids: ["*"]
(in addition to the internal AllowAllKeys flag), clarify that the resolver sets
an empty includeOnlyKeys slice to enforce deny-by-default when no keys are
configured, and fix the wording from “blocks all” to “block all”; reference the
AllowAllKeys flag and includeOnlyKeys behavior so readers see both the
DB/internal flag and the externally visible key_ids: ["*"] semantics.
---
Duplicate comments:
In `@framework/configstore/tables/virtualkey.go`:
- Around line 64-76: The unmarshalling currently treats a missing allowed_keys
the same as an empty slice, causing configs that omit the field to be treated as
deny-all; change the temporary unmarshal type used in virtualkey.go so
AllowedKeys is a pointer to a slice (e.g. *[]string) and update the logic in the
block that examines temp.AllowedKeys (the code that currently checks
len(temp.AllowedKeys) > 0 && len(pc.Keys) == 0) to first test for nil (meaning
field was omitted) and only apply the wildcard/empty-array handling when
temp.AllowedKeys != nil; ensure you still set pc.AllowAllKeys for ["*"] and
populate pc.Keys when the pointer is non-nil and has entries.
---
Nitpick comments:
In `@transports/changelog.md`:
- Line 1: Update the changelog entry for the VK provider config (referencing
key_ids and AllowAllKeys) to explicitly note the DB migration and automatic
backfill behavior (existing VK configs are preserved by backfilling
allow_all_keys=true), and improve readability by splitting the single long line
into a short header plus sub-bullets that state: `["*"]` resolves to
AllowAllKeys (no DB lookups), empty key_ids denies all, and the non-breaking
migration/backfill behavior for existing configs; ensure the entry covers the
full stack scope of VK provider config changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4738cc85-6a3d-434f-8de4-00a18925f5ac
📒 Files selected for processing (10)
framework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/governance/resolver.go
- framework/changelog.md
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
e68b8cf to
ad76bf0
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
237dc14 to
266c534
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/configstore/tables/virtualkey.go (1)
51-76:⚠️ Potential issue | 🟠 Major
allowed_keysomission still falls through to deny-all.A legacy config-file provider that omits
allowed_keysstill unmarshals here asAllowAllKeys=falsewith no keys, so a later config sync can overwrite the migration backfill and block every key for that provider. This needs presence-aware decoding sonilpreserves allow-all,[]means deny-all, and["*"]means allow-all.Suggested fix
type TempProviderConfig struct { Alias - AllowedKeys []string `json:"allowed_keys"` // Config file format: array of key names + AllowedKeys *[]string `json:"allowed_keys"` // Config file format: array of key names } @@ - // If allowed_keys is provided (config file format), convert to Keys or set AllowAllKeys - // This takes precedence if Keys is empty but allowed_keys has values - if len(temp.AllowedKeys) > 0 && len(pc.Keys) == 0 { - // Check for wildcard — ["*"] means allow all keys - if len(temp.AllowedKeys) == 1 && temp.AllowedKeys[0] == "*" { - pc.AllowAllKeys = true - } else { - pc.Keys = make([]TableKey, len(temp.AllowedKeys)) - for i, keyName := range temp.AllowedKeys { - pc.Keys[i] = TableKey{Name: keyName} - } - } + // If keys were not supplied in DB/API format, interpret config-file allowed_keys semantics. + if len(pc.Keys) == 0 { + switch { + case temp.AllowedKeys == nil: + pc.AllowAllKeys = true + pc.Keys = nil + case len(*temp.AllowedKeys) == 1 && (*temp.AllowedKeys)[0] == "*": + pc.AllowAllKeys = true + pc.Keys = nil + default: + pc.AllowAllKeys = false + pc.Keys = make([]TableKey, len(*temp.AllowedKeys)) + for i, keyName := range *temp.AllowedKeys { + pc.Keys[i] = TableKey{Name: keyName} + } + } }As per coding guidelines, "always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/virtualkey.go` around lines 51 - 76, The current unmarshal loses the distinction between omitted vs empty allowed_keys; change TempProviderConfig.AllowedKeys to a pointer type (AllowedKeys *[]string) so you can detect nil (omitted) vs non-nil (present, possibly empty). After json.Unmarshal into temp, keep the existing copy of Alias into *pc, then only run the Keys/AllowAllKeys logic when temp.AllowedKeys != nil: if the slice has one element "*" set pc.AllowAllKeys=true, if len>0 populate pc.Keys from *temp.AllowedKeys (an empty non-nil slice should leave pc.Keys empty to represent deny-all); if temp.AllowedKeys is nil do nothing so existing pc.AllowAllKeys/Keys are preserved. Use the identifiers TempProviderConfig, AllowedKeys, pc, Keys, and AllowAllKeys to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@framework/configstore/tables/virtualkey.go`:
- Around line 51-76: The current unmarshal loses the distinction between omitted
vs empty allowed_keys; change TempProviderConfig.AllowedKeys to a pointer type
(AllowedKeys *[]string) so you can detect nil (omitted) vs non-nil (present,
possibly empty). After json.Unmarshal into temp, keep the existing copy of Alias
into *pc, then only run the Keys/AllowAllKeys logic when temp.AllowedKeys !=
nil: if the slice has one element "*" set pc.AllowAllKeys=true, if len>0
populate pc.Keys from *temp.AllowedKeys (an empty non-nil slice should leave
pc.Keys empty to represent deny-all); if temp.AllowedKeys is nil do nothing so
existing pc.AllowAllKeys/Keys are preserved. Use the identifiers
TempProviderConfig, AllowedKeys, pc, Keys, and AllowAllKeys to locate and
implement the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aaf2764e-b05a-4f45-8534-75053737bb00
📒 Files selected for processing (10)
framework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/lib/types/governance.ts
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- transports/changelog.md
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
ad76bf0 to
f5103e6
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
266c534 to
79350a3
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/governance.go (1)
436-449:⚠️ Potential issue | 🟠 MajorReject malformed
key_idsas a client error.These branches treat mixed wildcard payloads (for example
["*", "key-1"]) and unresolved key IDs as generic transaction failures, so create/update can respond with a 500 for invalid user input. Since*is now part of the public API contract, validate it as mutually exclusive and surface missing IDs as a typed 4xx before persisting anything.Also applies to: 762-775, 837-853
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 436 - 449, The branch that processes pc.KeyIDs must validate the wildcard "*" as mutually exclusive and surface any bad/missing key IDs as a client error instead of a generic transaction failure: in the block that sets allowAllKeys and calls h.configStore.GetKeysByIDs, check if pc.KeyIDs contains "*" together with other IDs and return a typed 4xx client validation error (rather than a 500) for that malformed input; likewise if GetKeysByIDs returns fewer keys than requested, return a 4xx indicating missing key IDs (include pc.Provider and expected vs found) so callers get a client-level validation response; apply the same change to the other similar blocks around the pc.KeyIDs handling (the other occurrences you noted).
♻️ Duplicate comments (1)
framework/configstore/tables/virtualkey.go (1)
47-76:⚠️ Potential issue | 🟠 MajorKeep omitted
allowed_keysdistinct from explicit[].
TempProviderConfig.AllowedKeysis a plain[]string, so a legacy config that omitsallowed_keysis indistinguishable from an explicit empty list. On the next config sync, that can overwrite a migrated row withAllowAllKeys=falseand no keys, effectively denying every key for that provider. Make this field presence-aware (*[]stringor similar) and only apply the new deny-all semantics whenallowed_keyswas actually present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/virtualkey.go` around lines 47 - 76, The UnmarshalJSON should detect whether allowed_keys was present vs omitted: change TempProviderConfig.AllowedKeys from []string to *[]string, unmarshal into that, and only run the conversion/deny-all logic when temp.AllowedKeys != nil (treat nil as omitted and do not mutate pc.Keys or pc.AllowAllKeys); update the wildcard check to use *temp.AllowedKeys and construct pc.Keys from the dereferenced slice when present, leaving existing values unchanged when allowed_keys was omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 436-449: The branch that processes pc.KeyIDs must validate the
wildcard "*" as mutually exclusive and surface any bad/missing key IDs as a
client error instead of a generic transaction failure: in the block that sets
allowAllKeys and calls h.configStore.GetKeysByIDs, check if pc.KeyIDs contains
"*" together with other IDs and return a typed 4xx client validation error
(rather than a 500) for that malformed input; likewise if GetKeysByIDs returns
fewer keys than requested, return a 4xx indicating missing key IDs (include
pc.Provider and expected vs found) so callers get a client-level validation
response; apply the same change to the other similar blocks around the pc.KeyIDs
handling (the other occurrences you noted).
---
Duplicate comments:
In `@framework/configstore/tables/virtualkey.go`:
- Around line 47-76: The UnmarshalJSON should detect whether allowed_keys was
present vs omitted: change TempProviderConfig.AllowedKeys from []string to
*[]string, unmarshal into that, and only run the conversion/deny-all logic when
temp.AllowedKeys != nil (treat nil as omitted and do not mutate pc.Keys or
pc.AllowAllKeys); update the wildcard check to use *temp.AllowedKeys and
construct pc.Keys from the dereferenced slice when present, leaving existing
values unchanged when allowed_keys was omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60974006-f5d3-4bf6-b678-96443126c6a8
📒 Files selected for processing (10)
framework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/lib/types/governance.ts
- framework/configstore/migrations.go
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
79350a3 to
cf6018a
Compare
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
f5103e6 to
2a794d7
Compare
Pratham-Mishra04
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/migrations.go`:
- Around line 4309-4329: The VirtualKeyProviderConfig's AllowAllKeys field is
missing from the hash input causing GenerateVirtualKeyHash to ignore changes to
it; update the VirtualKeyProviderConfigHashInput type/struct (used by
GenerateVirtualKeyHash in clientconfig.go) to include AllowAllKeys and ensure
the code that builds the hash input for each TableVirtualKeyProviderConfig
populates that field so the generated hash changes when AllowAllKeys toggles
(this will make the recomputation loop that loads TableVirtualKey and calls
GenerateVirtualKeyHash pick up the change).
🪄 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: e3c0afb9-3eea-4719-b6f1-83540b2fe7de
📒 Files selected for processing (5)
framework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/resolver.go
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/changelog.md
- plugins/governance/resolver.go
- plugins/governance/changelog.md
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
cf6018a to
d25afc8
Compare
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
2a794d7 to
a237ad1
Compare
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
5463080 to
a6efe9a
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
55c1ab7 to
1124ac4
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
755-824:⚠️ Potential issue | 🟡 MinorAdd a stable selector to the Allowed Keys multiselect.
This is a new interactive control in the sheet, but it’s the only one in this block without a
data-testid. Please add one that follows the existingvk-*-<index>convention so E2E coverage can target it reliably.As per coding guidelines, UI interactive elements must have data-testid attributes following the pattern 'data-testid="--"'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 755 - 824, The Allowed Keys AsyncMultiSelect is missing a stable test selector; add a data-testid attribute following the vk-*-<index> convention (e.g., data-testid={`vk-allowed-keys-${index}`}) to the AsyncMultiSelect component so E2E tests can reliably target it—locate the AsyncMultiSelect usage in this file (the component that uses selectedProviderKeys, hasWildcard, and calls handleUpdateProviderConfig) and add the data-testid prop.
♻️ Duplicate comments (2)
plugins/governance/resolver.go (1)
240-250:⚠️ Potential issue | 🟠 MajorClear stale include-only filters on allow-all path.
At Line 243, whenpc.AllowAllKeysis true,schemas.BifrostContextKeyGovernanceIncludeOnlyKeysis not overwritten. A prior value can leak into this request path and incorrectly restrict keys.🔧 Proposed fix
- // Find the provider config that matches the request's provider and apply key filtering + // Reset key filter first; nil means "no include-only restriction" + ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil) + + // Find the provider config that matches the request's provider and apply key filtering for _, pc := range vk.ProviderConfigs { if schemas.ModelProvider(pc.Provider) == provider { if !pc.AllowAllKeys { // Restrict to specific keys (empty slice = no keys allowed) includeOnlyKeys := make([]string, 0, len(pc.Keys)) for _, dbKey := range pc.Keys { includeOnlyKeys = append(includeOnlyKeys, dbKey.KeyID) } ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, includeOnlyKeys) } break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/resolver.go` around lines 240 - 250, The code sets schemas.BifrostContextKeyGovernanceIncludeOnlyKeys when pc.AllowAllKeys is false but does not clear it when pc.AllowAllKeys is true, allowing a previous request's include-only filter to leak; inside the loop over vk.ProviderConfigs (and specifically in the branch handling schemas.ModelProvider(pc.Provider) == provider) ensure that in the pc.AllowAllKeys == true path you explicitly clear the context key (e.g., call ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil) or set an empty slice) so stale include-only filters are removed for allow-all requests.transports/bifrost-http/handlers/governance.go (1)
499-503:⚠️ Potential issue | 🟠 MajorReject mixed wildcard + explicit key IDs before key lookup.
At Line 499-503, Line 840-843, and Line 915-918,
["*", "<id>"]still falls through to ID lookup and can bubble as a 500 path instead of a deterministic 400 client error. Please validate this input shape up front and fail fast before any DB lookup.♻️ Consolidated fix (single parser reused across all 3 call sites)
+var errInvalidProviderKeyIDs = errors.New("invalid provider key_ids") + +func parseProviderKeyIDs(keyIDs []string) (allowAll bool, explicitIDs []string, err error) { + if len(keyIDs) == 0 { + return false, nil, nil + } + if len(keyIDs) == 1 && keyIDs[0] == "*" { + return true, nil, nil + } + for _, id := range keyIDs { + if id == "*" { + return false, nil, fmt.Errorf("%w: cannot mix '*' with explicit ids", errInvalidProviderKeyIDs) + } + } + return false, keyIDs, nil +}-allowAllKeys := false -if len(pc.KeyIDs) == 1 && pc.KeyIDs[0] == "*" { - allowAllKeys = true -} else if len(pc.KeyIDs) > 0 { +allowAllKeys, explicitKeyIDs, err := parseProviderKeyIDs(pc.KeyIDs) +if err != nil { + return err +} +if len(explicitKeyIDs) > 0 { var err error - keys, err = h.configStore.GetKeysByIDs(ctx, pc.KeyIDs) + keys, err = h.configStore.GetKeysByIDs(ctx, explicitKeyIDs) if err != nil { return fmt.Errorf("failed to get keys by IDs for provider %s: %w", pc.Provider, err) } - if len(keys) != len(pc.KeyIDs) { - return fmt.Errorf("some keys not found for provider %s: expected %d, found %d", pc.Provider, len(pc.KeyIDs), len(keys)) + if len(keys) != len(explicitKeyIDs) { + return fmt.Errorf("some keys not found for provider %s: expected %d, found %d", pc.Provider, len(explicitKeyIDs), len(keys)) } }Also applies to: 840-843, 915-918
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 499 - 503, Reject mixed wildcard and explicit KeyIDs up front: validate pc.KeyIDs in the handlers (where allowAllKeys is computed) and return a deterministic 400 if the slice contains "*" together with any other IDs (e.g., ["*", "<id>"]) before performing any DB/key lookup. Update the logic around the allowAllKeys computation (the pc.KeyIDs handling blocks) to detect this invalid shape and fail fast; factor this check into a small helper/validator (reused at the three call sites) so the same validation runs at the locations currently setting allowAllKeys.
🧹 Nitpick comments (2)
plugins/governance/resolver_test.go (1)
29-29: Add explicit tests forskipRateLimitsAndBudgets=true.
All updated callsites here passfalse, so the new auth-driven bypass branch is currently unverified in resolver tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/resolver_test.go` at line 29, The test currently only calls resolver.EvaluateVirtualKeyRequest with skipRateLimitsAndBudgets=false; add an explicit unit test that calls EvaluateVirtualKeyRequest(ctx, "sk-bf-test", schemas.OpenAI, "gpt-4", schemas.ChatCompletionRequest, true) to exercise the auth-driven bypass branch. In that new test, assert the expected bypass behavior (e.g., that rate limit/budget checks are skipped or that the returned decision reflects bypass) and reuse the same setup/mocks as the existing test to verify the true path in the resolver package.framework/configstore/migrations.go (1)
4363-4389: Collapse the backfill scan into a set-based query.This currently loads every provider config and then runs one
COUNT(*)per row while the global migration lock is held. A singleLEFT JOIN/NOT EXISTSquery will make startup time much more predictable on larger installs.♻️ Example refactor
- var allConfigs []tables.TableVirtualKeyProviderConfig - if err := tx.Find(&allConfigs).Error; err != nil { - return fmt.Errorf("failed to query provider configs: %w", err) - } + type keylessProviderConfig struct { + ID uint + VirtualKeyID string + } + var keylessConfigs []keylessProviderConfig + if err := tx. + Table("governance_virtual_key_provider_configs AS pc"). + Select("pc.id, pc.virtual_key_id"). + Joins("LEFT JOIN governance_virtual_key_provider_config_keys AS pck ON pck.table_virtual_key_provider_config_id = pc.id"). + Where("pck.table_virtual_key_provider_config_id IS NULL"). + Scan(&keylessConfigs).Error; err != nil { + return fmt.Errorf("failed to query keyless provider configs: %w", err) + } @@ - for _, pc := range allConfigs { - var keyCount int64 - if err := tx.Table("governance_virtual_key_provider_config_keys"). - Where("table_virtual_key_provider_config_id = ?", pc.ID). - Count(&keyCount).Error; err != nil { - return fmt.Errorf("failed to count keys for provider config %d: %w", pc.ID, err) - } - - if keyCount == 0 { - if err := tx.Model(&tables.TableVirtualKeyProviderConfig{}). - Where("id = ?", pc.ID). - Update("allow_all_keys", true).Error; err != nil { - return fmt.Errorf("failed to backfill allow_all_keys for provider config %d: %w", pc.ID, err) - } - modifiedVKIDs[pc.VirtualKeyID] = struct{}{} - } + for _, pc := range keylessConfigs { + if err := tx.Model(&tables.TableVirtualKeyProviderConfig{}). + Where("id = ?", pc.ID). + Update("allow_all_keys", true).Error; err != nil { + return fmt.Errorf("failed to backfill allow_all_keys for provider config %d: %w", pc.ID, err) + } + modifiedVKIDs[pc.VirtualKeyID] = struct{}{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 4363 - 4389, The current loop in migrations.go loads all tables.TableVirtualKeyProviderConfig rows then does a per-row COUNT on governance_virtual_key_provider_config_keys while holding the migration lock; replace this with a single set-based UPDATE/SELECT using a LEFT JOIN or NOT EXISTS to find configs with zero keys and update allow_all_keys = true in one statement, then populate modifiedVKIDs with the VirtualKeyID values returned by that query (or by a single SELECT of affected rows) so you avoid per-row COUNTs using tx and keep the same semantics for modifiedVKIDs and the allow_all_keys backfill.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 755-824: The Allowed Keys AsyncMultiSelect is missing a stable
test selector; add a data-testid attribute following the vk-*-<index> convention
(e.g., data-testid={`vk-allowed-keys-${index}`}) to the AsyncMultiSelect
component so E2E tests can reliably target it—locate the AsyncMultiSelect usage
in this file (the component that uses selectedProviderKeys, hasWildcard, and
calls handleUpdateProviderConfig) and add the data-testid prop.
---
Duplicate comments:
In `@plugins/governance/resolver.go`:
- Around line 240-250: The code sets
schemas.BifrostContextKeyGovernanceIncludeOnlyKeys when pc.AllowAllKeys is false
but does not clear it when pc.AllowAllKeys is true, allowing a previous
request's include-only filter to leak; inside the loop over vk.ProviderConfigs
(and specifically in the branch handling schemas.ModelProvider(pc.Provider) ==
provider) ensure that in the pc.AllowAllKeys == true path you explicitly clear
the context key (e.g., call
ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil) or set an
empty slice) so stale include-only filters are removed for allow-all requests.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 499-503: Reject mixed wildcard and explicit KeyIDs up front:
validate pc.KeyIDs in the handlers (where allowAllKeys is computed) and return a
deterministic 400 if the slice contains "*" together with any other IDs (e.g.,
["*", "<id>"]) before performing any DB/key lookup. Update the logic around the
allowAllKeys computation (the pc.KeyIDs handling blocks) to detect this invalid
shape and fail fast; factor this check into a small helper/validator (reused at
the three call sites) so the same validation runs at the locations currently
setting allowAllKeys.
---
Nitpick comments:
In `@framework/configstore/migrations.go`:
- Around line 4363-4389: The current loop in migrations.go loads all
tables.TableVirtualKeyProviderConfig rows then does a per-row COUNT on
governance_virtual_key_provider_config_keys while holding the migration lock;
replace this with a single set-based UPDATE/SELECT using a LEFT JOIN or NOT
EXISTS to find configs with zero keys and update allow_all_keys = true in one
statement, then populate modifiedVKIDs with the VirtualKeyID values returned by
that query (or by a single SELECT of affected rows) so you avoid per-row COUNTs
using tx and keep the same semantics for modifiedVKIDs and the allow_all_keys
backfill.
In `@plugins/governance/resolver_test.go`:
- Line 29: The test currently only calls resolver.EvaluateVirtualKeyRequest with
skipRateLimitsAndBudgets=false; add an explicit unit test that calls
EvaluateVirtualKeyRequest(ctx, "sk-bf-test", schemas.OpenAI, "gpt-4",
schemas.ChatCompletionRequest, true) to exercise the auth-driven bypass branch.
In that new test, assert the expected bypass behavior (e.g., that rate
limit/budget checks are skipped or that the returned decision reflects bypass)
and reuse the same setup/mocks as the existing test to verify the true path in
the resolver package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0aed7f09-2378-40b6-a85d-1742a7840911
📒 Files selected for processing (13)
core/utils.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/changelog.md
- framework/configstore/tables/virtualkey.go
- core/utils.go
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
1124ac4 to
3d47165
Compare
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
a6efe9a to
0e423ab
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
751-824:⚠️ Potential issue | 🟡 MinorAdd a
data-testidon the Allowed Keys selector wrapper.This changed interactive block lacks a test selector in the view, which makes E2E targeting brittle.
Suggested fix
- <AsyncMultiSelect + <div data-testid={`vk-allowed-keys-multiselect-${index}`}> + <AsyncMultiSelect hideSelectedOptions isNonAsync @@ - /> + /> + </div>As per coding guidelines
ui/**/*.{ts,tsx}: UI interactive elements must have data-testid attributes following the patterndata-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/virtual-keys/views/virtualKeySheet.tsx` around lines 751 - 824, The Allowed Keys selector block (the JSX returning the <div className="mx-0.5 space-y-2"> containing <Label> and AsyncMultiSelect) lacks a test selector; add a data-testid attribute on that wrapper div (or the AsyncMultiSelect) following the project pattern data-testid="virtualkey-allowed-keys-wrapper" (or data-testid="virtualkey-allowed-keys-select" if placed on AsyncMultiSelect) so E2E tests can target the element; update the JSX for the wrapper around Label and AsyncMultiSelect (the element returned in the anonymous render where AsyncMultiSelect is used) to include the data-testid string.
♻️ Duplicate comments (1)
framework/configstore/migrations.go (1)
4391-4408:⚠️ Potential issue | 🟠 Major
config_hashrecomputation can still missallow_all_keyschanges.At Line 4391, the migration recomputes hashes, but
GenerateVirtualKeyHash(inframework/configstore/clientconfig.go) appears to build provider-config hash input withoutAllowAllKeys. That means toggling allow-all semantics may not changeconfig_hashwhen key links stay the same.Suggested fix (cross-file)
--- a/framework/configstore/clientconfig.go +++ b/framework/configstore/clientconfig.go @@ type VirtualKeyProviderConfigHashInput struct { Provider string Weight *float64 AllowedModels []string + AllowAllKeys bool BudgetID *string RateLimitID *string KeyIDs []string } @@ providerConfigsForHash[i] = VirtualKeyProviderConfigHashInput{ Provider: pc.Provider, Weight: pc.Weight, AllowedModels: sortedAllowedModels, + AllowAllKeys: pc.AllowAllKeys, BudgetID: pc.BudgetID, RateLimitID: pc.RateLimitID, KeyIDs: keyIDs, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 4391 - 4408, The migration recomputes config_hash for modified virtual keys but GenerateVirtualKeyHash currently omits the allow-all-keys flag from the hash input, so toggling AllowAllKeys doesn't change config_hash; update GenerateVirtualKeyHash (in framework/configstore/clientconfig.go) to include the VirtualKey-level AllowAllKeys (or per-provider AllowAllKeys if present) in the data used to compute the provider-config hash, and ensure the migration loop that loads TableVirtualKey (the block using tx.Preload(...).First(&vk, "id = ?", vkID) and then Update("config_hash", newHash)) will pick up the changed AllowAllKeys value when calling GenerateVirtualKeyHash so hashes are recomputed correctly.
🧹 Nitpick comments (2)
plugins/governance/resolver_test.go (1)
17-33: Consider adding test coverage forskipRateLimitsAndBudgets = true.All tests pass
falsefor the new parameter. Adding at least one test case withskipRateLimitsAndBudgets = truewould verify that rate-limit and budget checks are correctly bypassed when user-level governance handles limits.💡 Example test case
// TestBudgetResolver_EvaluateRequest_SkipRateLimitsAndBudgets tests that rate limits and budgets are bypassed func TestBudgetResolver_EvaluateRequest_SkipRateLimitsAndBudgets(t *testing.T) { logger := NewMockLogger() // VK with rate limit already at max - would normally be blocked rateLimit := buildRateLimitWithUsage("rl1", 10000, 10000, 1000, 0) // Tokens at max vk := buildVirtualKeyWithRateLimit("vk1", "sk-bf-test", "Test VK", rateLimit) store, err := NewLocalGovernanceStore(context.Background(), logger, nil, &configstore.GovernanceConfig{ VirtualKeys: []configstoreTables.TableVirtualKey{*vk}, RateLimits: []configstoreTables.TableRateLimit{*rateLimit}, }, nil) require.NoError(t, err) resolver := NewBudgetResolver(store, nil, logger) ctx := &schemas.BifrostContext{} // With skipRateLimitsAndBudgets=true, should allow despite rate limit being exceeded result := resolver.EvaluateVirtualKeyRequest(ctx, "sk-bf-test", schemas.OpenAI, "gpt-4", schemas.ChatCompletionRequest, true) assertDecision(t, DecisionAllow, result) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/resolver_test.go` around lines 17 - 33, Add a test that verifies skipRateLimitsAndBudgets=true bypasses rate and budget checks: create a VK that references a rate limit already at max using buildRateLimitWithUsage and buildVirtualKeyWithRateLimit, initialize the store via NewLocalGovernanceStore (include the rate limit in GovernanceConfig.RateLimits), call resolver.EvaluateVirtualKeyRequest(...) with the final parameter true, and assert the decision is DecisionAllow and the virtual key is found; use the existing TestBudgetResolver_EvaluateRequest_AllowedRequest pattern for setup and assertions.transports/bifrost-http/handlers/governance.go (1)
499-511: Consider extracting key ID parsing into a helper function.The same pattern for parsing
KeyIDs(wildcard check, key fetching, count validation) is repeated three times acrosscreateVirtualKeyandupdateVirtualKey. A small helper could reduce duplication:♻️ Optional refactor
func (h *GovernanceHandler) parseProviderKeyIDs(ctx context.Context, keyIDs []string, provider string) (allowAllKeys bool, keys []configstoreTables.TableKey, err error) { if len(keyIDs) == 1 && keyIDs[0] == "*" { return true, nil, nil } if len(keyIDs) > 0 { keys, err = h.configStore.GetKeysByIDs(ctx, keyIDs) if err != nil { return false, nil, fmt.Errorf("failed to get keys by IDs for provider %s: %w", provider, err) } if len(keys) != len(keyIDs) { return false, nil, fmt.Errorf("some keys not found for provider %s: expected %d, found %d", provider, len(keyIDs), len(keys)) } } return false, keys, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 499 - 511, Extract the repeated KeyIDs parsing logic used in createVirtualKey and updateVirtualKey into a helper on GovernanceHandler (e.g., parseProviderKeyIDs) that takes ctx, keyIDs []string and provider string and returns (allowAllKeys bool, keys []configstoreTables.TableKey, err error); move the wildcard check (len==1 && "*"), the call to h.configStore.GetKeysByIDs, and the count-validation into that helper and replace the duplicated blocks in createVirtualKey and updateVirtualKey with calls to this helper, preserving the original error messages and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 751-824: The Allowed Keys selector block (the JSX returning the
<div className="mx-0.5 space-y-2"> containing <Label> and AsyncMultiSelect)
lacks a test selector; add a data-testid attribute on that wrapper div (or the
AsyncMultiSelect) following the project pattern
data-testid="virtualkey-allowed-keys-wrapper" (or
data-testid="virtualkey-allowed-keys-select" if placed on AsyncMultiSelect) so
E2E tests can target the element; update the JSX for the wrapper around Label
and AsyncMultiSelect (the element returned in the anonymous render where
AsyncMultiSelect is used) to include the data-testid string.
---
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 4391-4408: The migration recomputes config_hash for modified
virtual keys but GenerateVirtualKeyHash currently omits the allow-all-keys flag
from the hash input, so toggling AllowAllKeys doesn't change config_hash; update
GenerateVirtualKeyHash (in framework/configstore/clientconfig.go) to include the
VirtualKey-level AllowAllKeys (or per-provider AllowAllKeys if present) in the
data used to compute the provider-config hash, and ensure the migration loop
that loads TableVirtualKey (the block using tx.Preload(...).First(&vk, "id = ?",
vkID) and then Update("config_hash", newHash)) will pick up the changed
AllowAllKeys value when calling GenerateVirtualKeyHash so hashes are recomputed
correctly.
---
Nitpick comments:
In `@plugins/governance/resolver_test.go`:
- Around line 17-33: Add a test that verifies skipRateLimitsAndBudgets=true
bypasses rate and budget checks: create a VK that references a rate limit
already at max using buildRateLimitWithUsage and buildVirtualKeyWithRateLimit,
initialize the store via NewLocalGovernanceStore (include the rate limit in
GovernanceConfig.RateLimits), call resolver.EvaluateVirtualKeyRequest(...) with
the final parameter true, and assert the decision is DecisionAllow and the
virtual key is found; use the existing
TestBudgetResolver_EvaluateRequest_AllowedRequest pattern for setup and
assertions.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 499-511: Extract the repeated KeyIDs parsing logic used in
createVirtualKey and updateVirtualKey into a helper on GovernanceHandler (e.g.,
parseProviderKeyIDs) that takes ctx, keyIDs []string and provider string and
returns (allowAllKeys bool, keys []configstoreTables.TableKey, err error); move
the wildcard check (len==1 && "*"), the call to h.configStore.GetKeysByIDs, and
the count-validation into that helper and replace the duplicated blocks in
createVirtualKey and updateVirtualKey with calls to this helper, preserving the
original error messages and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c61ad7c1-c91e-40fa-b399-5b85b62e2e68
📒 Files selected for processing (13)
core/utils.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- framework/changelog.md
- core/utils.go
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- plugins/governance/changelog.md
- transports/changelog.md
- plugins/governance/main.go
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
3d47165 to
10fab26
Compare
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
0e423ab to
102a0a7
Compare
9 tasks
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
755-824:⚠️ Potential issue | 🟡 MinorAdd a
data-testidto the Allowed Keys multiselect control.This interactive element is part of the changed flow but has no selector, which makes E2E targeting brittle.
🧪 Suggested change
<AsyncMultiSelect + data-testid={`provider-allowed-keys-multiselect-${index}`} hideSelectedOptionsAs per coding guidelines,
ui/**/*.{ts,tsx}interactive elements must havedata-testidattributes followingdata-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/virtual-keys/views/virtualKeySheet.tsx` around lines 755 - 824, Add a data-testid to the Allowed Keys multiselect by adding a data-testid prop on the AsyncMultiSelect component (the AsyncMultiSelect instance that uses value={selectedProviderKeys} and onChange={...}) following the project convention, e.g. data-testid="virtual-key-allowed-keys-multiselect" (entity-element-qualifier). Ensure the attribute is added directly to the AsyncMultiSelect JSX so E2E tests can reliably target this control.
🧹 Nitpick comments (3)
transports/bifrost-http/handlers/governance.go (1)
499-502: Consider extracting repeated wildcard-detection logic into a helper.The pattern
len(pc.KeyIDs) == 1 && pc.KeyIDs[0] == "*"is repeated in three places. While the current implementation is correct and consistent, extracting this into a small helper function would reduce duplication and make future changes easier.💡 Optional helper extraction
// isAllowAllKeysWildcard returns true if keyIDs is the single-element wildcard ["*"]. func isAllowAllKeysWildcard(keyIDs []string) bool { return len(keyIDs) == 1 && keyIDs[0] == "*" }Usage:
-allowAllKeys := false -if len(pc.KeyIDs) == 1 && pc.KeyIDs[0] == "*" { - allowAllKeys = true -} else if len(pc.KeyIDs) > 0 { +allowAllKeys := isAllowAllKeysWildcard(pc.KeyIDs) +if !allowAllKeys && len(pc.KeyIDs) > 0 {Also applies to: 840-843, 915-918
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 499 - 502, Extract the repeated wildcard check into a helper like isAllowAllKeysWildcard(keyIDs []string) bool and replace each occurrence of the pattern len(pc.KeyIDs) == 1 && pc.KeyIDs[0] == "*" (and its local allowAllKeys assignments) with a call to isAllowAllKeysWildcard(pc.KeyIDs); update all three spots where pc.KeyIDs is tested (the blocks that set allowAllKeys) to use this helper to reduce duplication and keep behavior identical.framework/configstore/migrations.go (1)
4361-4389: Avoid per-configCOUNTqueries in this backfill loop.Line 4373-4378 performs one query per provider config, which can significantly extend startup migration time on large datasets. Prefer a single
LEFT JOIN ... GROUP BY ... HAVING COUNT(...) = 0scan and batch update IDs.♻️ Proposed refactor (single-pass selection + batch update)
- for _, pc := range allConfigs { - var keyCount int64 - if err := tx.Table("governance_virtual_key_provider_config_keys"). - Where("table_virtual_key_provider_config_id = ?", pc.ID). - Count(&keyCount).Error; err != nil { - return fmt.Errorf("failed to count keys for provider config %d: %w", pc.ID, err) - } - - if keyCount == 0 { - if err := tx.Model(&tables.TableVirtualKeyProviderConfig{}). - Where("id = ?", pc.ID). - Update("allow_all_keys", true).Error; err != nil { - return fmt.Errorf("failed to backfill allow_all_keys for provider config %d: %w", pc.ID, err) - } - modifiedVKIDs[pc.VirtualKeyID] = struct{}{} - } - } + type zeroKeyConfig struct { + ID uint + VirtualKeyID string + } + var zeroKeyConfigs []zeroKeyConfig + if err := tx. + Table("governance_virtual_key_provider_configs pc"). + Select("pc.id, pc.virtual_key_id"). + Joins("LEFT JOIN governance_virtual_key_provider_config_keys k ON k.table_virtual_key_provider_config_id = pc.id"). + Group("pc.id, pc.virtual_key_id"). + Having("COUNT(k.table_key_id) = 0"). + Scan(&zeroKeyConfigs).Error; err != nil { + return fmt.Errorf("failed to find zero-key provider configs: %w", err) + } + + ids := make([]uint, 0, len(zeroKeyConfigs)) + for _, row := range zeroKeyConfigs { + ids = append(ids, row.ID) + modifiedVKIDs[row.VirtualKeyID] = struct{}{} + } + if len(ids) > 0 { + if err := tx.Model(&tables.TableVirtualKeyProviderConfig{}). + Where("id IN ?", ids). + Update("allow_all_keys", true).Error; err != nil { + return fmt.Errorf("failed to backfill allow_all_keys: %w", err) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 4361 - 4389, The loop does a per-config COUNT against governance_virtual_key_provider_config_keys which is slow; instead perform a single query that LEFT JOINs governance_virtual_key_provider_config_keys onto tables.TableVirtualKeyProviderConfig, GROUP BY the provider config id and HAVING COUNT(key_id) = 0 to select all provider-config IDs with zero keys in one pass, then batch UPDATE those IDs to set allow_all_keys = true (using tx.Model(&tables.TableVirtualKeyProviderConfig{}).Where("id IN (?)", ids).Update(...)) and collect the corresponding VirtualKeyID values into modifiedVKIDs for later hash recompute; replace the tx.Find + per-row tx.Table(...).Count loop with this single JOIN+GROUP selection followed by a single batch update and population of modifiedVKIDs.plugins/governance/main.go (1)
1013-1019: Prefer a named flag instead of positional boolean literals inEvaluateVirtualKeyRequest.Line 1015 and Line 1018 pass
true/falsedirectly into a multi-argument API that has changed in this stack. A named flag with a single call makes intent clearer and reduces inversion risk during stacked rebases.♻️ Proposed refactor
- if result.Decision == DecisionAllow && evaluationRequest.VirtualKey != "" { - if evaluationRequest.UserID != "" { - // User auth present: only use VK for routing/filtering (skip rate limits and budgets) - result = p.resolver.EvaluateVirtualKeyRequest(ctx, evaluationRequest.VirtualKey, evaluationRequest.Provider, evaluationRequest.Model, requestType, true) - } else { - // No user auth: full VK governance (routing + limits) - result = p.resolver.EvaluateVirtualKeyRequest(ctx, evaluationRequest.VirtualKey, evaluationRequest.Provider, evaluationRequest.Model, requestType, false) - } - } + if result.Decision == DecisionAllow && evaluationRequest.VirtualKey != "" { + skipRateLimitsAndBudgets := evaluationRequest.UserID != "" + // User auth present => skip VK budgets/rate limits; VK still used for routing/filtering. + result = p.resolver.EvaluateVirtualKeyRequest( + ctx, + evaluationRequest.VirtualKey, + evaluationRequest.Provider, + evaluationRequest.Model, + requestType, + skipRateLimitsAndBudgets, + ) + }As per coding guidelines
**: always check the stack if there is one for the current PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 1013 - 1019, The call sites to EvaluateVirtualKeyRequest use positional boolean literals (true/false) which are error-prone; update both branches to call a single EvaluateVirtualKeyRequest invocation that passes a clearly named option instead of a positional bool (e.g., an options struct or a named parameter like SkipRateLimits/OnlyRouting), so replace the two calls to p.resolver.EvaluateVirtualKeyRequest(...) with one call that constructs and passes the named flag (or options) indicating "only use VK for routing/filtering" when evaluationRequest.UserID != "" and the opposite when empty; ensure you modify the EvaluateVirtualKeyRequest signature or its caller wrapper accordingly so intent is explicit and inversion risk is eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 755-824: Add a data-testid to the Allowed Keys multiselect by
adding a data-testid prop on the AsyncMultiSelect component (the
AsyncMultiSelect instance that uses value={selectedProviderKeys} and
onChange={...}) following the project convention, e.g.
data-testid="virtual-key-allowed-keys-multiselect" (entity-element-qualifier).
Ensure the attribute is added directly to the AsyncMultiSelect JSX so E2E tests
can reliably target this control.
---
Nitpick comments:
In `@framework/configstore/migrations.go`:
- Around line 4361-4389: The loop does a per-config COUNT against
governance_virtual_key_provider_config_keys which is slow; instead perform a
single query that LEFT JOINs governance_virtual_key_provider_config_keys onto
tables.TableVirtualKeyProviderConfig, GROUP BY the provider config id and HAVING
COUNT(key_id) = 0 to select all provider-config IDs with zero keys in one pass,
then batch UPDATE those IDs to set allow_all_keys = true (using
tx.Model(&tables.TableVirtualKeyProviderConfig{}).Where("id IN (?)",
ids).Update(...)) and collect the corresponding VirtualKeyID values into
modifiedVKIDs for later hash recompute; replace the tx.Find + per-row
tx.Table(...).Count loop with this single JOIN+GROUP selection followed by a
single batch update and population of modifiedVKIDs.
In `@plugins/governance/main.go`:
- Around line 1013-1019: The call sites to EvaluateVirtualKeyRequest use
positional boolean literals (true/false) which are error-prone; update both
branches to call a single EvaluateVirtualKeyRequest invocation that passes a
clearly named option instead of a positional bool (e.g., an options struct or a
named parameter like SkipRateLimits/OnlyRouting), so replace the two calls to
p.resolver.EvaluateVirtualKeyRequest(...) with one call that constructs and
passes the named flag (or options) indicating "only use VK for
routing/filtering" when evaluationRequest.UserID != "" and the opposite when
empty; ensure you modify the EvaluateVirtualKeyRequest signature or its caller
wrapper accordingly so intent is explicit and inversion risk is eliminated.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 499-502: Extract the repeated wildcard check into a helper like
isAllowAllKeysWildcard(keyIDs []string) bool and replace each occurrence of the
pattern len(pc.KeyIDs) == 1 && pc.KeyIDs[0] == "*" (and its local allowAllKeys
assignments) with a call to isAllowAllKeysWildcard(pc.KeyIDs); update all three
spots where pc.KeyIDs is tested (the blocks that set allowAllKeys) to use this
helper to reduce duplication and keep behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e62a992-d72a-466f-ae8d-5851fde0f6aa
📒 Files selected for processing (13)
core/utils.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- core/utils.go
- plugins/governance/resolver_test.go
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- ui/lib/types/governance.ts
- plugins/governance/resolver.go
- transports/changelog.md
- plugins/governance/changelog.md
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
10fab26 to
3148afc
Compare
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
102a0a7 to
033e271
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
755-824:⚠️ Potential issue | 🟡 MinorAdd
data-testidto the Allowed Keys selector.This interactive control is part of the changed flow but currently lacks a stable selector.
💡 Suggested fix
<AsyncMultiSelect + data-testid={`vk-keys-${index}`} hideSelectedOptions isNonAsync closeMenuOnSelect={false}As per coding guidelines
ui/**/*.{ts,tsx}: UI interactive elements must have data-testid attributes following the patterndata-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/virtual-keys/views/virtualKeySheet.tsx` around lines 755 - 824, The Allowed Keys AsyncMultiSelect lacks a stable test selector; add a data-testid prop to the AsyncMultiSelect instance (the component rendered in virtualKeySheet.tsx) following the pattern data-testid="virtual-key-allowed-keys-selector" (or similar per entity-element-qualifier rules). Ensure the attribute is placed on the AsyncMultiSelect JSX element (the one with props value={selectedProviderKeys} and onChange={...}) so tests can reliably query it.
♻️ Duplicate comments (3)
framework/configstore/tables/virtualkey.go (1)
64-74:⚠️ Potential issue | 🟠 Major
allowed_keyspresence is still not distinguishable from explicit empty.With
AllowedKeys []string, omittedallowed_keysandallowed_keys: []collapse to the same state during unmarshal. That can unintentionally resolve legacy omitted configs as deny-all in config-sync paths.🔧 Suggested fix
type TempProviderConfig struct { Alias - AllowedKeys []string `json:"allowed_keys"` // Config file format: array of key names + AllowedKeys *[]string `json:"allowed_keys"` // nil => field omitted, [] => explicitly empty } @@ - if len(temp.AllowedKeys) > 0 && len(pc.Keys) == 0 { - // Check for wildcard — ["*"] means allow all keys - if len(temp.AllowedKeys) == 1 && temp.AllowedKeys[0] == "*" { + if temp.AllowedKeys != nil && len(pc.Keys) == 0 { + // Check for wildcard — ["*"] means allow all keys + if len(*temp.AllowedKeys) == 1 && (*temp.AllowedKeys)[0] == "*" { pc.AllowAllKeys = true + pc.Keys = nil + } else if len(*temp.AllowedKeys) == 0 { + // Explicit empty list => deny all + pc.AllowAllKeys = false + pc.Keys = nil } else { - pc.Keys = make([]TableKey, len(temp.AllowedKeys)) - for i, keyName := range temp.AllowedKeys { + pc.AllowAllKeys = false + pc.Keys = make([]TableKey, len(*temp.AllowedKeys)) + for i, keyName := range *temp.AllowedKeys { pc.Keys[i] = TableKey{Name: keyName} } } }#!/bin/bash # Verify whether omitted vs explicit-empty `allowed_keys` is distinguished # and whether tests cover both cases. set -euo pipefail echo "=== Unmarshal implementation ===" sed -n '47,90p' framework/configstore/tables/virtualkey.go echo echo "=== Presence tracking pattern check ===" rg -nP 'AllowedKeys\s+\*?\[\]string|allowed_keys' framework/configstore/tables/virtualkey.go echo echo "=== Coverage check for omitted vs empty allowed_keys semantics ===" rg -nP 'allowed_keys|AllowAllKeys|UnmarshalJSON' --type go framework/configstoreExpected verification outcome:
AllowedKeysshould be pointer-backed (or equivalent presence-tracking mechanism).- There should be tests covering both omitted
allowed_keysand explicit emptyallowed_keys: [].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/virtualkey.go` around lines 64 - 74, The unmarshalling logic in virtualkey.go cannot tell if allowed_keys was omitted vs explicitly set empty because AllowedKeys is a []string; change AllowedKeys to a pointer type (e.g. *[]string) or add an explicit presence flag and update the UnmarshalJSON/parse flow so you can detect nil (omitted) vs non-nil empty slice (explicit empty). Update the branch that currently checks len(temp.AllowedKeys) to instead check for nil to preserve legacy "omitted" semantics (only apply deny-all/AllowAllKeys logic when AllowedKeys is non-nil), and add unit tests for both omitted allowed_keys and explicit allowed_keys: [] to validate behavior. Ensure references to pc.Keys and pc.AllowAllKeys remain consistent with the new presence-aware field.plugins/governance/resolver.go (1)
240-250:⚠️ Potential issue | 🟠 MajorClear include-only key filter on the
AllowAllKeyspath.When
pc.AllowAllKeysis true, the code breaks without overwritingschemas.BifrostContextKeyGovernanceIncludeOnlyKeys. If that key was previously set in the same request context, filtering can remain unintentionally active.💡 Suggested fix
for _, pc := range vk.ProviderConfigs { if schemas.ModelProvider(pc.Provider) == provider { - if !pc.AllowAllKeys { + if pc.AllowAllKeys { + ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil) + } else { // Restrict to specific keys (empty slice = no keys allowed) includeOnlyKeys := make([]string, 0, len(pc.Keys)) for _, dbKey := range pc.Keys { includeOnlyKeys = append(includeOnlyKeys, dbKey.KeyID) } ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, includeOnlyKeys) } break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/resolver.go` around lines 240 - 250, The request context key schemas.BifrostContextKeyGovernanceIncludeOnlyKeys is only set when pc.AllowAllKeys is false, so if a previous provider set that key and a later provider has pc.AllowAllKeys == true the old include-only filter remains; update the branch for pc.AllowAllKeys == true in the loop over vk.ProviderConfigs (within the code handling schemas.ModelProvider(pc.Provider) == provider) to explicitly clear/overwrite the context key via ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil or an empty slice) so the include-only filter is removed when AllowAllKeys is true.transports/bifrost-http/handlers/governance.go (1)
499-503:⚠️ Potential issue | 🟠 MajorReject mixed wildcard + explicit key IDs before key lookup.
key_ids=["*", "<id>"]still falls through toGetKeysByIDs(...), which can surface as a 500-path instead of a deterministic 400 validation error. This is duplicated in all three branches, so the same bug shape exists for create and update flows.💡 Suggested fix (single parser reused in all three call sites)
+var errInvalidProviderKeyIDs = errors.New("invalid provider key_ids") + +func parseProviderKeyIDs(keyIDs []string) (allowAll bool, explicit []string, err error) { + if len(keyIDs) == 0 { + return false, nil, nil + } + if len(keyIDs) == 1 && keyIDs[0] == "*" { + return true, nil, nil + } + for _, id := range keyIDs { + if id == "*" { + return false, nil, fmt.Errorf("%w: cannot mix '*' with explicit key_ids", errInvalidProviderKeyIDs) + } + } + return false, keyIDs, nil +}-allowAllKeys := false -if len(pc.KeyIDs) == 1 && pc.KeyIDs[0] == "*" { - allowAllKeys = true -} else if len(pc.KeyIDs) > 0 { +allowAllKeys, explicitKeyIDs, err := parseProviderKeyIDs(pc.KeyIDs) +if err != nil { + return err +} +if len(explicitKeyIDs) > 0 { var err error - keys, err = h.configStore.GetKeysByIDs(ctx, pc.KeyIDs) + keys, err = h.configStore.GetKeysByIDs(ctx, explicitKeyIDs) if err != nil { return fmt.Errorf("failed to get keys by IDs for provider %s: %w", pc.Provider, err) } - if len(keys) != len(pc.KeyIDs) { - return fmt.Errorf("some keys not found for provider %s: expected %d, found %d", pc.Provider, len(pc.KeyIDs), len(keys)) + if len(keys) != len(explicitKeyIDs) { + return fmt.Errorf("some keys not found for provider %s: expected %d, found %d", pc.Provider, len(explicitKeyIDs), len(keys)) } }Also applies to: 840-844, 915-919
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 499 - 503, Reject any KeyIDs arrays that mix the wildcard "*" with explicit IDs before attempting key lookup: in the governance handlers where pc.KeyIDs is processed (the block that sets allowAllKeys and calls GetKeysByIDs), add a validation that returns a 400 error if len(pc.KeyIDs) > 1 and one of the entries equals "*" (rather than falling through to GetKeysByIDs). Apply the same check to the equivalent parsing code paths used for create and update flows (the duplicated branches that now call GetKeysByIDs) so they all deterministically return a validation error for mixed wildcard+explicit key IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 755-824: The Allowed Keys AsyncMultiSelect lacks a stable test
selector; add a data-testid prop to the AsyncMultiSelect instance (the component
rendered in virtualKeySheet.tsx) following the pattern
data-testid="virtual-key-allowed-keys-selector" (or similar per
entity-element-qualifier rules). Ensure the attribute is placed on the
AsyncMultiSelect JSX element (the one with props value={selectedProviderKeys}
and onChange={...}) so tests can reliably query it.
---
Duplicate comments:
In `@framework/configstore/tables/virtualkey.go`:
- Around line 64-74: The unmarshalling logic in virtualkey.go cannot tell if
allowed_keys was omitted vs explicitly set empty because AllowedKeys is a
[]string; change AllowedKeys to a pointer type (e.g. *[]string) or add an
explicit presence flag and update the UnmarshalJSON/parse flow so you can detect
nil (omitted) vs non-nil empty slice (explicit empty). Update the branch that
currently checks len(temp.AllowedKeys) to instead check for nil to preserve
legacy "omitted" semantics (only apply deny-all/AllowAllKeys logic when
AllowedKeys is non-nil), and add unit tests for both omitted allowed_keys and
explicit allowed_keys: [] to validate behavior. Ensure references to pc.Keys and
pc.AllowAllKeys remain consistent with the new presence-aware field.
In `@plugins/governance/resolver.go`:
- Around line 240-250: The request context key
schemas.BifrostContextKeyGovernanceIncludeOnlyKeys is only set when
pc.AllowAllKeys is false, so if a previous provider set that key and a later
provider has pc.AllowAllKeys == true the old include-only filter remains; update
the branch for pc.AllowAllKeys == true in the loop over vk.ProviderConfigs
(within the code handling schemas.ModelProvider(pc.Provider) == provider) to
explicitly clear/overwrite the context key via
ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil or an empty
slice) so the include-only filter is removed when AllowAllKeys is true.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 499-503: Reject any KeyIDs arrays that mix the wildcard "*" with
explicit IDs before attempting key lookup: in the governance handlers where
pc.KeyIDs is processed (the block that sets allowAllKeys and calls
GetKeysByIDs), add a validation that returns a 400 error if len(pc.KeyIDs) > 1
and one of the entries equals "*" (rather than falling through to GetKeysByIDs).
Apply the same check to the equivalent parsing code paths used for create and
update flows (the duplicated branches that now call GetKeysByIDs) so they all
deterministically return a validation error for mixed wildcard+explicit key IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 751b3822-be4e-4e61-b0ba-30d3a0aee929
📒 Files selected for processing (13)
core/utils.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- core/utils.go
- framework/changelog.md
- plugins/governance/main.go
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- plugins/governance/resolver_test.go
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
3148afc to
a736638
Compare
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
033e271 to
12acd31
Compare
18 tasks
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
framework/configstore/tables/virtualkey.go (1)
51-76:⚠️ Potential issue | 🟠 MajorPreserve omitted
allowed_keyssemantics during unmarshal.On Line 53 and Line 66,
allowed_keysomission and explicit[]are still indistinguishable. That can make legacy config entries that omitallowed_keysdeserialize into deny-all and overwrite migrated allow-all rows during sync.💡 Suggested fix
type TempProviderConfig struct { Alias - AllowedKeys []string `json:"allowed_keys"` // Config file format: array of key names + AllowedKeys *[]string `json:"allowed_keys"` // nil => omitted, [] => explicit deny-all } @@ - if len(temp.AllowedKeys) > 0 && len(pc.Keys) == 0 { - // Check for wildcard — ["*"] means allow all keys - if len(temp.AllowedKeys) == 1 && temp.AllowedKeys[0] == "*" { + if temp.AllowedKeys != nil && len(pc.Keys) == 0 { + allowedKeys := *temp.AllowedKeys + // Check for wildcard — ["*"] means allow all keys + if len(allowedKeys) == 1 && allowedKeys[0] == "*" { pc.AllowAllKeys = true + pc.Keys = nil + } else if len(allowedKeys) == 0 { + // Explicit empty list => deny all + pc.AllowAllKeys = false + pc.Keys = nil } else { - pc.Keys = make([]TableKey, len(temp.AllowedKeys)) - for i, keyName := range temp.AllowedKeys { + pc.AllowAllKeys = false + pc.Keys = make([]TableKey, len(allowedKeys)) + for i, keyName := range allowedKeys { pc.Keys[i] = TableKey{Name: keyName} } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/virtualkey.go` around lines 51 - 76, The unmarshal can't tell whether allowed_keys was omitted or explicitly set to [] because AllowedKeys is a slice; change TempProviderConfig.AllowedKeys to a pointer to a slice (e.g., *[]string) so absence yields nil and an explicit [] yields non-nil/len==0, then update the post-unmarshal logic in the block that inspects temp.AllowedKeys to first check for nil (skip if nil), otherwise handle the wildcard case (["*"] -> set pc.AllowAllKeys) or populate pc.Keys from the slice; keep assignment of standard fields via TableVirtualKeyProviderConfig(temp.Alias) as-is.plugins/governance/resolver.go (1)
240-250:⚠️ Potential issue | 🟠 MajorClear include-only key context explicitly on allow-all provider configs.
When
pc.AllowAllKeysis true, this branch does not overwriteschemas.BifrostContextKeyGovernanceIncludeOnlyKeys. If that context key is already set, key filtering can remain unintentionally active.💡 Suggested fix
for _, pc := range vk.ProviderConfigs { if schemas.ModelProvider(pc.Provider) == provider { - if !pc.AllowAllKeys { - // Restrict to specific keys (empty slice = no keys allowed) - includeOnlyKeys := make([]string, 0, len(pc.Keys)) - for _, dbKey := range pc.Keys { - includeOnlyKeys = append(includeOnlyKeys, dbKey.KeyID) - } - ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, includeOnlyKeys) - } + if pc.AllowAllKeys { + // Clear any previously set key filter + ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil) + } else { + // Restrict to specific keys (empty slice = no keys allowed) + includeOnlyKeys := make([]string, 0, len(pc.Keys)) + for _, dbKey := range pc.Keys { + includeOnlyKeys = append(includeOnlyKeys, dbKey.KeyID) + } + ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, includeOnlyKeys) + } break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/resolver.go` around lines 240 - 250, The code only sets schemas.BifrostContextKeyGovernanceIncludeOnlyKeys when pc.AllowAllKeys is false, leaving a previously set include-only key list in place when pc.AllowAllKeys is true; update the loop over vk.ProviderConfigs (and the pc.AllowAllKeys branch) to explicitly clear that context key for allow-all provider configs by calling ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil) or an empty slice when pc.AllowAllKeys is true so key filtering is disabled; reference vk.ProviderConfigs, pc.AllowAllKeys, ctx.SetValue and schemas.BifrostContextKeyGovernanceIncludeOnlyKeys when making the change.
🧹 Nitpick comments (1)
plugins/governance/resolver_test.go (1)
29-479: Add one focused test forskipRateLimitsAndBudgets=true.All updated call sites pass
false, so the new boolean branch is still unverified in this suite. A dedicated test would prevent regressions where skip-mode accidentally enforces budgets/rate limits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/resolver_test.go` around lines 29 - 479, Add a focused test that calls BudgetResolver.EvaluateVirtualKeyRequest with the skipRateLimitsAndBudgets boolean set to true to verify budgets and rate-limits are bypassed; create a VK that would normally be blocked (e.g., use buildRateLimitWithUsage or buildBudgetWithUsage and wrap with buildVirtualKeyWithRateLimit/buildVirtualKeyWithBudget), call resolver.EvaluateVirtualKeyRequest(ctx, "sk-bf-test", schemas.OpenAI, "gpt-4", schemas.ChatCompletionRequest, true) and assert the decision is DecisionAllow (and that virtual key is found) to ensure the skip branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@framework/configstore/tables/virtualkey.go`:
- Around line 51-76: The unmarshal can't tell whether allowed_keys was omitted
or explicitly set to [] because AllowedKeys is a slice; change
TempProviderConfig.AllowedKeys to a pointer to a slice (e.g., *[]string) so
absence yields nil and an explicit [] yields non-nil/len==0, then update the
post-unmarshal logic in the block that inspects temp.AllowedKeys to first check
for nil (skip if nil), otherwise handle the wildcard case (["*"] -> set
pc.AllowAllKeys) or populate pc.Keys from the slice; keep assignment of standard
fields via TableVirtualKeyProviderConfig(temp.Alias) as-is.
In `@plugins/governance/resolver.go`:
- Around line 240-250: The code only sets
schemas.BifrostContextKeyGovernanceIncludeOnlyKeys when pc.AllowAllKeys is
false, leaving a previously set include-only key list in place when
pc.AllowAllKeys is true; update the loop over vk.ProviderConfigs (and the
pc.AllowAllKeys branch) to explicitly clear that context key for allow-all
provider configs by calling
ctx.SetValue(schemas.BifrostContextKeyGovernanceIncludeOnlyKeys, nil) or an
empty slice when pc.AllowAllKeys is true so key filtering is disabled; reference
vk.ProviderConfigs, pc.AllowAllKeys, ctx.SetValue and
schemas.BifrostContextKeyGovernanceIncludeOnlyKeys when making the change.
---
Nitpick comments:
In `@plugins/governance/resolver_test.go`:
- Around line 29-479: Add a focused test that calls
BudgetResolver.EvaluateVirtualKeyRequest with the skipRateLimitsAndBudgets
boolean set to true to verify budgets and rate-limits are bypassed; create a VK
that would normally be blocked (e.g., use buildRateLimitWithUsage or
buildBudgetWithUsage and wrap with
buildVirtualKeyWithRateLimit/buildVirtualKeyWithBudget), call
resolver.EvaluateVirtualKeyRequest(ctx, "sk-bf-test", schemas.OpenAI, "gpt-4",
schemas.ChatCompletionRequest, true) and assert the decision is DecisionAllow
(and that virtual key is found) to ensure the skip branch is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ac1a76c-660f-4326-9aa6-c6cadd58bc9a
📒 Files selected for processing (13)
core/utils.goframework/changelog.mdframework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/changelog.mdplugins/governance/main.goplugins/governance/resolver.goplugins/governance/resolver_test.gotransports/bifrost-http/handlers/governance.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- core/utils.go
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
- framework/changelog.md
- transports/changelog.md
Pratham-Mishra04
force-pushed
the
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
branch
from
a736638 to
80cf3d6
Compare
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
12acd31 to
04a4831
Compare
Collaborator
Author
Merge activity
|
Pratham-Mishra04
changed the base branch from
03-05-feat_virtual_key_mcp_configs_now_act_as_an_execution-time_allow-list
to
graphite-base/2006
… all keys; empty key_ids denies all
Pratham-Mishra04
force-pushed
the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
from
04a4831 to
9ca7d6c
Compare
Pratham-Mishra04
deleted the
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
branch
18 tasks
This was referenced Apr 1, 2026
Merged
akshaydeo
added a commit
that referenced
this pull request
Apr 18, 2026
* refactor: standardize empty array conventions for VK Provider & MCP Configs, and makes Provider Config weight optional for routing (#1932)
## Summary
Changes Virtual Key provider and MCP configurations from "allow-all by default" to "deny-by-default" security model. Virtual Keys now require explicit provider and MCP client configurations to allow access, improving security posture.
## Changes
- **Provider Configs**: Empty `provider_configs` now blocks all providers instead of allowing all
- **MCP Configs**: Empty `mcp_configs` now blocks all MCP tools instead of allowing all
- **Weight Field**: Changed provider `weight` from required `float64` to optional `*float64` - null weight excludes provider from weighted routing
- **Migration**: Added automatic backfill migration to preserve existing Virtual Key behavior by adding all available providers/MCP clients to VKs with empty configs
- **Documentation**: Updated all references to reflect new deny-by-default behavior
- **UI Updates**: Modified Virtual Key creation/editing interface to reflect new behavior and weight handling
## Type of change
- [x] Feature
- [x] Refactor
- [x] Documentation
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Test Virtual Key creation and provider/MCP access:
```sh
# Core/Transports
go version
go test ./...
# Test Virtual Key with no provider configs blocks requests
curl -X POST http://localhost:8080/v1/chat/completions \
-H "Authorization: Bearer sk-bf-empty-vk" \
-H "Content-Type: application/json" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Should return error about no providers configured
# Test Virtual Key with provider configs allows requests
curl -X POST http://localhost:8080/v1/chat/completions \
-H "Authorization: Bearer sk-bf-configured-vk" \
-H "Content-Type: application/json" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Should work normally
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
## Breaking changes
- [x] Yes
**Impact**: Existing Virtual Keys with empty `provider_configs` or `mcp_configs` would be blocked after this change.
**Migration**: Automatic migration `migrationBackfillEmptyVirtualKeyConfigs` runs on startup to backfill existing Virtual Keys with all available providers/MCP clients, preserving current behavior. New Virtual Keys created after this change will use deny-by-default.
## Security considerations
This change significantly improves security posture by requiring explicit configuration of allowed providers and MCP tools for Virtual Keys. The automatic migration ensures no disruption to existing deployments while new Virtual Keys benefit from the more secure default behavior.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add MCP auto tool injection toggle (#1933)
## Summary
Adds a new configuration option `DisableAutoToolInject` to the MCP (Model Context Protocol) system that allows disabling automatic tool injection into requests. When enabled, MCP tools are only included when explicitly requested via context headers or filters, providing more granular control over tool availability.
## Changes
- Added `DisableAutoToolInject` field to `MCPToolManagerConfig` schema with runtime update support
- Implemented atomic boolean storage in `ToolsManager` to safely handle concurrent access
- Added logic in `ParseAndAddToolsToRequest` to respect the disable flag and only inject tools when explicit context filters are present
- Extended configuration management with database migration, UI controls, and API endpoints
- Added hot-reload capability through `UpdateMCPDisableAutoToolInject` methods across the stack
- Updated UI with a toggle switch and clear documentation about the feature's behavior
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Validate the new MCP auto tool injection toggle:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Test the feature:
1. Configure MCP clients and tools
2. Enable "Disable Auto Tool Injection" in the MCP configuration UI
3. Make requests without explicit tool headers - tools should not be injected
4. Make requests with `x-bf-mcp-include-tools` header - tools should be injected
5. Verify hot-reload works by toggling the setting without server restart
## Screenshots/Recordings
UI changes include a new toggle switch in the MCP configuration view with descriptive text explaining when tools are injected based on explicit headers.
## Breaking changes
- [ ] Yes
- [x] No
This is a backward-compatible addition with a default value of `false` (auto injection enabled).
## Related issues
This addresses the need for more granular control over MCP tool injection behavior in request processing.
## Security considerations
The feature provides better control over tool exposure by allowing administrators to require explicit opt-in for tool injection, potentially reducing unintended tool access.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: VK MCP config now works as an AllowList (#1940)
## Summary
This PR implements MCP tool governance by enforcing virtual key MCP configurations as an execution-time allow-list. When virtual keys have empty MCPConfigs, all MCP tools are denied. When non-empty, each tool is validated against the configured allow-list at both inference time and MCP tool execution.
## Changes
- **Context parameter updates**: Changed MCP-related functions to use `*schemas.BifrostContext` instead of `context.Context` to enable tool tracking
- **Tool tracking**: Added `BifrostContextKeyMCPAddedTools` context key to track which MCP tools are added to requests
- **Governance enforcement**: Virtual key MCP configurations now act as execution-time allow-lists with validation in both `PreMCPHook` and `evaluateGovernanceRequest`
- **Auto-injection control**: Added `DisableAutoToolInject` configuration option that respects the toggle and skips auto-injection when headers are already set by callers
- **Decision type**: Added `DecisionMCPToolBlocked` for MCP tool governance violations
- **UI improvements**: Updated MCP view description and sidebar item naming for better clarity
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Test MCP tool governance with virtual keys:
```sh
# Core/Transports
go version
go test ./...
# Test with virtual key having empty MCPConfigs (should deny all MCP tools)
curl -X POST /v1/chat/completions \
-H "x-bf-virtual-key: test-vk-empty-mcp" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Test with virtual key having specific MCP tool allowlist
curl -X POST /v1/chat/completions \
-H "x-bf-virtual-key: test-vk-with-mcp" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Test disable auto tool inject configuration
curl -X PUT /v1/config/mcp/disable-auto-tool-inject \
-d '{"disable": true}'
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
New configuration options:
- `disable_auto_tool_inject`: Boolean flag to disable automatic MCP tool injection
- Virtual key `MCPConfigs`: Array of MCP client configurations that act as allow-lists
## Screenshots/Recordings
UI changes include updated MCP configuration view with clearer descriptions for the disable auto tool injection toggle and improved sidebar navigation labels.
## Breaking changes
- [x] Yes
- [ ] No
**Impact**: MCP-related function signatures now require `*schemas.BifrostContext` instead of `context.Context`. Virtual keys with empty MCPConfigs will now deny all MCP tools by default.
**Migration**: Update any custom MCP integrations to use the new context parameter type. Configure MCPConfigs on virtual keys that need MCP tool access.
## Related issues
Implements MCP tool governance and execution-time validation for virtual key configurations.
## Security considerations
- **Access control**: Virtual key MCP configurations now enforce strict allow-lists for tool execution
- **Context isolation**: Tool tracking is isolated per request context to prevent cross-request leakage
- **Validation**: Both pre-execution and execution-time validation prevent unauthorized tool access
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: standardize empty array conventions for VK Provider Config Allowed Keys (#2006)
## Summary
Migrates VK provider config allowed keys from implicit allow-all semantics to explicit deny-by-default behavior. Adds `AllowAllKeys` boolean field to enable granular key access control while maintaining backward compatibility.
## Changes
- Added `AllowAllKeys` boolean field to `TableVirtualKeyProviderConfig` with database migration
- Backfilled existing configs with `allow_all_keys=true` to preserve current behavior
- Updated key resolution logic: empty keys now denies all access, `["*"]` wildcard allows all keys
- Modified governance resolver to set empty `includeOnlyKeys` slice when no keys are configured
- Enhanced HTTP handlers to recognize `["*"]` wildcard and set `AllowAllKeys` flag appropriately
- Updated UI to display "Allow All Keys" option and show deny-by-default messaging
- Added JSON unmarshaling support for `["*"]` wildcard in config files
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Validate the migration and new key access control behavior:
```sh
# Core/Transports
go version
go test ./...
# Test migration runs successfully
go run main.go migrate
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Test scenarios:
1. Create VK with empty `key_ids` - should deny all keys
2. Create VK with `key_ids: ["*"]` - should allow all keys
3. Create VK with specific key IDs - should allow only those keys
4. Verify existing VKs maintain their current behavior after migration
## Screenshots/Recordings
UI now shows:
- "Allow All Keys" option in key selection dropdown
- "No keys allowed" vs "All keys allowed" status indicators
- "No providers configured (deny-by-default)" messaging
## Breaking changes
- [ ] Yes
- [x] No
The migration preserves existing behavior by setting `allow_all_keys=true` for configs that previously had no keys specified.
## Related issues
Part of VK access control enhancement initiative.
## Security considerations
Improves security posture by implementing deny-by-default semantics for key access. Existing deployments maintain current access patterns through automatic backfill migration.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: standardize empty array conventions for allowed models (#2113)
## Summary
Standardizes empty array conventions across Bifrost to implement deny-by-default security semantics. Previously, empty arrays for `allowed_models` and `Models` fields meant "allow all", creating potential security gaps. Now `["*"]` explicitly means "allow all" while empty arrays mean "deny all".
## Changes
- **Core Logic**: Updated model filtering in `bifrost.go` and `selectKeyFromProviderForModel` to treat empty `Models` arrays as deny-all and `["*"]` as allow-all
- **Database Migration**: Added `migrationBackfillAllowedModelsWildcard` to convert existing empty arrays to `["*"]` preserving current behavior for existing records
- **Model Catalog**: Updated `IsModelAllowedForProvider` to use wildcard semantics with deny-by-default fallback
- **Schema Defaults**: Changed default `Models` value from `[]` to `["*"]` in table definitions and form schemas
- **UI Components**: Enhanced `ModelMultiselect` with `allowAllOption` prop and updated virtual key forms to handle wildcard selection
- **Documentation**: Updated JSON schemas, comments, and tooltips to reflect new conventions
- **Governance**: Updated provider config filtering logic to use new wildcard semantics
- **Server Bootstrap**: Added wildcard filtering when loading models to prevent literal "*" from appearing as a model name
## Type of change
- [x] Refactor
- [ ] Bug fix
- [ ] Feature
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Validate the migration and new semantics:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Test scenarios:
1. Create new virtual keys - should default to `["*"]` for allowed models
2. Create new provider keys - should default to `["*"]` for models
3. Verify existing keys with empty arrays are migrated to `["*"]`
4. Test that empty arrays now deny all models/keys as expected
5. Verify UI shows "All models allowed" for wildcard and "No models (deny all)" for empty arrays
## Screenshots/Recordings
UI changes include:
- Model multiselect now shows "Allow All Models" option
- Virtual key details display "All Models" badge for wildcard vs "No models (deny all)" for empty
- Form placeholders updated to reflect new semantics
## Breaking changes
- [x] Yes
- [ ] No
**Migration Impact**: The database migration automatically converts existing empty `allowed_models` and `models_json` arrays to `["*"]`, preserving current behavior. However, any new configurations with empty arrays will now deny access instead of allowing all. Applications relying on "empty = allow all" semantics must be updated to use `["*"]` explicitly.
## Related issues
Part of security hardening initiative to implement explicit allow-lists and deny-by-default semantics across Bifrost configuration.
## Security considerations
This change significantly improves security posture by:
- Eliminating ambiguous "empty means allow all" semantics
- Implementing explicit deny-by-default for new configurations
- Requiring intentional wildcard usage via `["*"]` for broad access
- Maintaining backward compatibility through automatic migration
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: replace string slices with WhiteList for allowlist fields (#2125)
## Summary
Introduces a new `WhiteList` type to standardize whitelist behavior across the codebase, replacing manual slice operations and string comparisons with semantic methods for handling allow/deny lists.
## Changes
- Added `WhiteList` type with methods `IsAllowed()`, `IsUnrestricted()`, `IsEmpty()`, `Contains()`, and `Validate()`
- Replaced `[]string` fields with `WhiteList` for model restrictions, tool filtering, and key access controls
- Updated all whitelist logic to use semantic methods instead of manual `slices.Contains()` checks
- Added validation to ensure wildcards ("*") aren't mixed with specific values and prevent duplicates
- Improved case-insensitive matching for whitelist comparisons
## Type of change
- [x] Refactor
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Plugins
## How to test
Verify that whitelist behavior remains consistent across all affected components:
```sh
# Core/Transports
go version
go test ./...
# Test specific whitelist scenarios:
# - Empty lists deny all access
# - ["*"] allows all access
# - Specific lists only allow listed items
# - Mixed wildcards and specific items are rejected
# - Duplicate entries are rejected
```
Test key model filtering, MCP tool execution, and virtual key configurations to ensure whitelist logic works correctly.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
The `WhiteList` type maintains the same JSON serialization format as `[]string`, so existing configurations remain compatible.
## Related issues
N/A
## Security considerations
Improves security by standardizing deny-by-default behavior and adding validation to prevent misconfigured whitelists that could inadvertently grant excessive permissions.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add request-level extra headers support for MCP tool execution (#2126)
## Summary
This PR adds support for request-level extra headers in MCP tool execution, allowing callers to forward specific headers to MCP servers at runtime based on a per-client allowlist configuration.
## Changes
- Added `AllowedExtraHeaders` field to MCP client configuration with allowlist semantics (empty array = deny all, `["*"]` = allow all)
- Introduced `BifrostContextKeyMCPExtraHeaders` context key to track headers forwarded to MCP tools
- Created `core/mcp/utils` package with `GetHeadersForToolExecution` function to merge static and dynamic headers
- Updated MCP tool execution in both regular tool manager and Starlark code mode to use the new header forwarding system
- Added database migration for `allowed_extra_headers_json` column in MCP client table
- Updated UI to include allowed extra headers configuration in MCP client management
- Enhanced auth demo server example to demonstrate tool-execution level authentication patterns
## Type of change
- [x] Feature
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] UI (Next.js)
## How to test
1. Configure an MCP client with allowed extra headers:
```json
{
"name": "test-client",
"connection_string": "http://localhost:3002/",
"auth_type": "headers",
"headers": {
"X-API-Key": "connection-secret"
},
"allowed_extra_headers": ["X-Tool-Token"],
"tools_to_execute": ["*"]
}
```
2. Make requests with extra headers that should be forwarded:
```bash
curl -X POST http://localhost:8080/v1/chat/completions \
-H "Authorization: Bearer your-key" \
-H "X-Tool-Token: tool-execution-secret" \
-d '{
"model": "gpt-4",
"messages": [{"role": "user", "content": "Use the secret_data tool"}],
"tools": [{"type": "function", "function": {"name": "secret_data"}}]
}'
```
3. Test the auth demo server:
```bash
cd examples/mcps/auth-demo-server
go run main.go
# Server demonstrates two-tier auth: connection-level (X-API-Key) and tool-level (X-Tool-Token)
```
4. Run tests:
```sh
go test ./core/mcp/...
go test ./transports/bifrost-http/...
cd ui
pnpm test
pnpm build
```
## Breaking changes
- [ ] Yes
- [x] No
This is a backward-compatible addition. Existing MCP clients will have empty `allowed_extra_headers` (deny all extra headers) which maintains current behavior.
## Security considerations
- Extra headers are filtered through a strict allowlist per MCP client
- Security denylist prevents auth header overrides via extra headers
- Two-tier authentication pattern demonstrated: connection-level + tool-execution level
- Headers are only forwarded to MCP servers that explicitly allow them
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: apply MCP tool filtering headers to tools/list response when using bifrost as MCP gateway (#2127)
## Summary
Adds support for `x-bf-mcp-include-clients` and `x-bf-mcp-include-tools` request headers to filter MCP tools/list response when using Bifrost as an MCP gateway. This ensures that tool filtering is respected at the MCP protocol level, not just during inference.
## Changes
- Implemented dynamic tool filtering in MCP server handlers that respects per-request include headers
- Added `makeIncludeClientsFilter()` function that filters tools based on request context values
- Registered the tool filter on both global and virtual key MCP servers during initialization
- Updated documentation to clarify that `mcp-include-tools` requires `clientName-toolName` format
- Enhanced examples in documentation to show proper tool naming format
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [x] Docs
## How to test
Test MCP gateway functionality with tool filtering:
```sh
# Test tools/list filtering with include-tools header
curl --location 'http://localhost:8080/mcp/tools/list' \
--header 'x-bf-mcp-include-tools: gmail-send_email,filesystem-read_file' \
--header 'Authorization: Bearer your-vk-here'
# Test tools/list filtering with include-clients header
curl --location 'http://localhost:8080/mcp/tools/list' \
--header 'x-bf-mcp-include-clients: gmail,filesystem' \
--header 'Authorization: Bearer your-vk-here'
# Verify chat completions still respect the same headers
curl --location 'http://localhost:8080/v1/chat/completions' \
--header 'x-bf-mcp-include-tools: gmail-send_email' \
--header 'Content-Type: application/json' \
--data '{
"model": "openai/gpt-4o-mini",
"messages": [{"role": "user", "content": "What tools are available?"}]
}'
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
The tool filtering mechanism ensures that virtual key restrictions are properly enforced at the MCP protocol level, preventing unauthorized access to tools that should be filtered out based on request headers.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: parallelize model listing for providers to speed up startup time (#2151)
## Summary
Parallelizes model listing operations for providers during server startup and provider reloading to significantly reduce initialization time. Previously, model listing was performed sequentially for each provider, causing slower startup times especially when multiple providers were configured.
## Changes
- Added concurrent execution using goroutines and sync.WaitGroup for model listing operations in three key functions: `ReloadProvider`, `ForceReloadPricing`, and `Bootstrap`
- In `ReloadProvider`, both filtered and unfiltered model listing requests now run concurrently for the same provider
- In `ForceReloadPricing` and `Bootstrap`, model listing for different providers now runs in parallel instead of sequentially
- Moved provider key retrieval earlier in `ReloadProvider` to ensure it happens before concurrent model listing
- Added proper context cancellation with defer statements for bifrost contexts
## Type of change
- [x] Refactor
## Affected areas
- [x] Transports (HTTP)
## How to test
Test server startup time with multiple providers configured to verify the performance improvement:
```sh
# Core/Transports
go version
go test ./...
# Test with multiple providers configured
# Measure startup time before and after the change
time go run main.go
```
Configure multiple providers in your bifrost configuration and observe faster startup times, especially noticeable when providers have high latency or many models.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications. The change maintains the same authentication and authorization patterns while improving performance through parallelization.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: reorder migrations and set AllowAllKeys to true for virtual key provider configs (#2158)
## Summary
Fixes database migration ordering issue and ensures virtual key configurations are properly initialized with the AllowAllKeys field set to true.
## Changes
- Reordered database migrations to execute `migrationAddAllowAllKeysToProviderConfig` before `migrationBackfillEmptyVirtualKeyConfigs` to ensure the AllowAllKeys column exists before backfilling
- Added `AllowAllKeys: true` to provider configurations created during virtual key backfill migration to enable unrestricted key access by default
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that database migrations run successfully and virtual key configurations are created with proper defaults:
```sh
# Core/Transports
go version
go test ./...
```
Test migration ordering by running against a fresh database to ensure no column reference errors occur.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
This change enables unrestricted key access by default for virtual key configurations, which may have security implications depending on the intended access control model.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: implement scoped pricing override
* refactor: custom pricing refactor
* fix: resolve merge conflicts in config loading and governance functions (#2230)
## Summary
Resolves Git merge conflicts in the bifrost-http configuration loading code by cleaning up duplicate function definitions and consolidating the configuration initialization flow.
## Changes
- Removed Git merge conflict markers and duplicate code blocks from `LoadConfig` function
- Consolidated governance configuration loading by keeping both `loadGovernanceConfigFromFile` and `loadGovernanceConfig` functions with distinct purposes
- Removed duplicate `convertSchemasMCPClientConfigToTable` function definition
- Moved pricing overrides initialization logic to `initFrameworkConfig` function for better organization
- Cleaned up extensive duplicate default configuration loading code that was causing merge conflicts
- Changed error handling for pricing overrides from returning error to logging warning
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that configuration loading works correctly without merge conflicts:
```sh
# Core/Transports
go version
go test ./...
go build ./transports/bifrost-http/...
```
Test configuration loading with various scenarios:
- Config file present
- Config file absent (default loading)
- Store-based configuration
- Governance and MCP configuration loading
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this is a merge conflict resolution that maintains existing functionality.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add Stability AI model support for Bedrock image generation (#2180)
## Summary
Adds support for Stability AI image generation models (stability.stable-image-*) to the Bedrock provider, enabling text-to-image generation with models like stability.stable-image-core-v1:1 and stability.stable-image-ultra-v1:1.
## Changes
- Added `isStabilityAIModel()` function to detect Stability AI models by "stability." prefix
- Created `ToStabilityAIImageGenerationRequest()` to convert Bifrost requests to Stability AI's flat request format
- Implemented `StabilityAIImageGenerationRequest` type with support for prompt, mode, aspect_ratio, output_format, seed, and negative_prompt parameters
- Added conditional routing in `ImageGeneration()` to use Stability AI request format when appropriate
- Extended known fields for image generation parameters to include "aspect_ratio" and "input_images"
- Updated documentation comment to reflect Stability AI model support
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test Stability AI image generation through the Bedrock provider:
```sh
# Core/Transports
go version
go test ./...
# Test with a Stability AI model
curl -X POST http://localhost:8080/v1/images/generations \
-H "Content-Type: application/json" \
-H "Authorization: Bearer your-key" \
-d '{
"model": "stability.stable-image-core-v1:1",
"prompt": "A beautiful sunset over mountains",
"aspect_ratio": "16:9",
"output_format": "PNG"
}'
```
Ensure AWS credentials are configured for Bedrock access and the Stability AI models are available in your region.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No additional security implications beyond existing Bedrock provider authentication and AWS credential handling.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add Stability AI image edit models support to Bedrock provider (#2225)
## Summary
Adds support for Stability AI image editing models in the Bedrock provider, expanding image editing capabilities beyond the existing Titan and Nova Canvas models.
## Changes
- Added `getStabilityAIEditTaskType()` function to infer edit task types from Stability AI model names (inpaint, outpaint, recolor, search-replace, erase-object, remove-bg, control-sketch, control-structure, style-guide, style-transfer, upscale-creative, upscale-conservative, upscale-fast)
- Created `ToStabilityAIImageEditRequest()` function to convert Bifrost requests to Stability AI's flat JSON format, with task-specific field validation
- Added `StabilityAIImageEditRequest` struct with comprehensive field support for all Stability AI edit operations
- Enhanced `BedrockImageGenerationResponse` with Seeds and FinishReasons fields for Stability AI compatibility
- Modified `ImageEdit()` method to route requests to appropriate conversion function based on model type
- Updated documentation to reflect expanded model support
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test with various Stability AI edit models through the Bedrock provider:
```sh
# Core/Transports
go version
go test ./...
# Test image editing with Stability AI models
# Example: stable-image-inpaint, stable-outpaint, stable-creative-upscale, etc.
```
Verify that task-specific parameters are correctly mapped and invalid fields are filtered out based on the detected task type.
## Screenshots/Recordings
N/A - Backend functionality only
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Image data is handled as base64-encoded strings. Mask and image parameters are properly validated before processing.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: send back accumulated usage in MCP agent mode (#2246)
## Summary
This PR fixes token usage tracking in MCP agent mode by accumulating usage across all LLM calls in the agent loop and returning the total usage in the final response.
## Changes
- Added usage accumulation logic in the MCP agent execution loop to track token consumption across multiple LLM calls
- Implemented `mergeUsage` function to combine token counts and costs from multiple `BifrostLLMUsage` values, handling all detail sub-fields including prompt tokens, completion tokens, and cost breakdowns
- Extended agent API adapters with `extractUsage` and `applyUsage` methods to handle usage extraction and application for both Chat API and Responses API
- Applied accumulated usage to the final response before returning it to the client
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test MCP agent mode with multiple tool calls to verify usage accumulation:
```sh
# Core/Transports
go version
go test ./...
# Test MCP agent mode with multiple LLM calls
# Verify that the returned usage reflects the sum of all calls in the agent loop
# Check that both token counts and cost details are properly accumulated
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this change only affects usage tracking and reporting.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* [codemode]: fixing string escape corruption, enable top-level control flow in starlark, refining the prompt of executecode tool (#2206)
## Changes
- **Enhanced Starlark dialect configuration**: Enabled top-level control flow statements (if/for/while), while loops, set() builtin, global variable reassignment, and recursive functions for a more Python-like experience
- **Improved string escape handling**: Removed automatic `\n` to newline conversion, allowing Starlark's native string escape processing to handle `\n`, `\t`, and other escape sequences correctly
- **Updated tool description**: Streamlined the executeToolCode tool description with clearer syntax notes, explicit documentation of Starlark differences from Python (no try/except, no classes, no imports, no f-strings), and emphasis on fresh isolated scope per execution
- **Enhanced error hints**: Added specific error messages for unsupported Python features like try/except/finally/raise, with guidance on alternative approaches and scope persistence warnings
- **Comprehensive test coverage**: Added tests for dialect options, string escape preservation, unsupported feature detection, and end-to-end JSON deserialization scenarios
## Type of change
- [ ] Feature
- [ ] Bug fix
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go) - Starlark CodeMode improvements
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test the enhanced Starlark features with MCP CodeMode:
```sh
# Test dialect options (top-level control flow, while loops, etc.)
make test-mcp TESTCASE=TestStarlarkDialectOptions
# Test string escape handling
make test-mcp PATTERN=TestStarlarkStringEscape
# Test unsupported feature detection
make test-mcp PATTERN=TestStarlarkUnsupportedFeatures
```
## Breaking changes
- [ ] Yes
- [x] No
The Starlark changes are additive and maintain backward compatibility while enabling more Python-like syntax.
## Security considerations
Starlark CodeMode maintains its existing sandboxing with no additional network or filesystem access. The dialect enhancements only affect language features within the existing security boundary.
* logging in plugins (#2215)
## Summary
Reorders middleware initialization in the Bifrost HTTP server to ensure tracing middleware is added before transport interceptor middleware in the inference pipeline.
## Changes
- Moved tracing middleware initialization and setup earlier in the bootstrap process
- Reordered middleware registration so tracing middleware is prepended before transport interceptor middleware
- Updated comments to clarify the middleware ordering logic and rationale
The change ensures that tracing context and trace IDs are properly established before other middleware components process requests.
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that tracing middleware executes before transport interceptor middleware by checking trace logs and middleware execution order.
```sh
# Core/Transports
go version
go test ./...
```
Test with tracing enabled to ensure trace IDs are properly set in context before subsequent middleware processing.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this is a middleware ordering change that affects observability components.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* fix: handling text, vtt, srt response format for transcriptions (#2102)
* feat: add virtual key access management for MCP clients (#2255)
## Summary
Adds virtual key access management to MCP client configuration, allowing administrators to control which virtual keys can access specific MCP servers and which tools they can execute on a per-VK basis.
## Changes
- Added `vk_configs` field to MCP client update API that accepts an array of virtual key configurations
- Each VK config specifies a virtual key ID and the tools it's allowed to execute on that MCP server
- When `vk_configs` is provided, it atomically replaces all existing VK assignments for the MCP client
- Added database method `GetVirtualKeyMCPConfigsByMCPClientID` to retrieve VK configs by MCP client
- Updated OpenAPI documentation to describe the new VK configuration functionality
- Enhanced UI with virtual key access management section in the MCP client sheet
- Added Go SDK context keys for MCP tool filtering: `MCPContextKeyIncludeClients`, `MCPContextKeyIncludeTools`, and `BifrostContextKeyMCPExtraHeaders`
- Updated context keys documentation with comprehensive MCP configuration examples
## Type of change
- [x] Feature
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] UI (Next.js)
- [x] Docs
## How to test
1. Create an MCP client with tools available
2. Create virtual keys in the system
3. Update the MCP client with VK configurations:
```sh
curl -X PUT /api/mcp/client/{id} \
-H "Content-Type: application/json" \
-d '{
"name": "test-client",
"vk_configs": [
{
"virtual_key_id": "vk-123",
"tools_to_execute": ["*"]
},
{
"virtual_key_id": "vk-456",
"tools_to_execute": ["read_file", "write_file"]
}
]
}'
```
4. Verify VK assignments are created/updated in the database
5. Test the UI by opening an MCP client sheet and managing virtual key access
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
## Screenshots/Recordings
The UI now includes a "Virtual Key Access" section in the MCP client configuration sheet where administrators can:
- Add virtual keys to grant access to the MCP server
- Configure which specific tools each virtual key can execute
- Remove virtual key access entirely
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
This enables fine-grained access control for MCP servers at the virtual key level, complementing the existing governance and budgeting features.
## Security considerations
- VK access controls are enforced through the governance plugin during MCP tool execution
- The atomic replacement of VK assignments prevents partial updates that could leave the system in an inconsistent state
- Tool-level restrictions allow principle of least privilege by limiting which MCP tools each virtual key can access
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: adds option to allow MCP clients to run on all virtual keys (#2258)
## Summary
Adds a new `AllowOnAllVirtualKeys` configuration option for MCP clients that enables them to be accessible to all virtual keys without requiring explicit per-key assignment. When enabled, all tools from the MCP client are available to every virtual key.
## Changes
- Added `AllowOnAllVirtualKeys` boolean field to `MCPClientConfig` schema and database table
- Updated MCP client manager to handle the new field during client updates
- Modified governance plugin to check for clients with `AllowOnAllVirtualKeys` enabled and automatically include their tools for all virtual keys
- Added database migration to add the new column to `TableMCPClient`
- Updated UI to include a toggle for the new setting with tooltip explanation
- Added OpenAPI documentation for the new field
- Updated configuration store methods to persist and retrieve the new field
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
1. Create or update an MCP client with `allow_on_all_virtual_keys: true`
2. Verify that the client's tools are available to all virtual keys without explicit assignment
3. Test that the governance plugin correctly allows tools from such clients
4. Verify the UI toggle works correctly in the MCP client edit sheet
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
The new configuration field `allow_on_all_virtual_keys` defaults to `false` to maintain backward compatibility.
## Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
## Breaking changes
- [ ] Yes
- [x] No
This is a backward-compatible addition with the new field defaulting to `false`.
## Related issues
Link related issues and discussions. Example: Closes #123
## Security considerations
This feature reduces access control granularity by allowing MCP clients to bypass virtual key restrictions when enabled. Administrators should carefully consider which MCP clients should have this permission as it grants broad access across all virtual keys.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add provider keys CRUD to configstore and in-memory store (#2159)
## Summary
Adds dedicated CRUD operations for individual provider keys at the data layer
(configstore interface + RDB implementation) and in-memory store. This enables
key-level operations without replacing the entire provider key set, which is
required for the new `/api/providers/{provider}/keys/*` endpoints.
## Changes
- Added `GetProviderKeys`, `GetProviderKey`, `CreateProviderKey`,
`UpdateProviderKey`, `DeleteProviderKey` to `ConfigStore` interface
- Implemented all five methods in `RDBConfigStore` with proper GORM queries,
error handling, and `ErrNotFound` propagation
- Extracted `schemaKeyFromTableKey` and `tableKeyFromSchemaKey` helpers to
deduplicate key conversion logic (previously inlined in `GetProvidersConfig`
and `GetProviderConfig`)
- Added `AddProviderKey`, `UpdateProviderKey`, `RemoveProviderKey` to in-memory
`Config` with mutex locking, DB persistence, and rollback on client update
failure
- Added `GetProviderKeysRaw`, `GetProviderKeysRedacted`, `GetProviderKeyRaw`,
`GetProviderKeyRedacted` read methods
- Added `TestProviderKeyCRUD` and `TestProviderKeyCRUD_ProviderMustExist`
integration tests
- Updated `MockConfigStore` with all five new interface methods
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Run configstore tests
go test ./framework/configstore/... -v -run TestProviderKeyCRUD
# Run config tests (mock store)
go test ./transports/bifrost-http/lib/... -v
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Key values are handled through existing redaction infrastructure. No new secret
exposure paths introduced.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: add provider keys HTTP handlers and refactor optional keys (#2160)
## Summary
Adds HTTP handlers for the dedicated provider keys CRUD endpoints and removes
`keys` from provider API responses and payloads. Keys are now exclusively
managed via `/api/providers/{provider}/keys/*`. Also fixes a context timeout bug
in `ReloadProvider` where model discovery could exhaust the shared context
budget, causing subsequent DB calls to fail.
## Changes
### Provider keys handlers (`provider_keys.go`)
- New file with five handlers: `listProviderKeys`, `getProviderKey`,
`createProviderKey`, `updateProviderKey`, `deleteProviderKey`
- Includes `mergeUpdatedKey` (redacted value preservation logic used by
`updateProviderKey`)
- Key handlers enforce keyless provider validation and trigger model discovery
after mutations
### Provider handlers cleanup (`providers.go`)
- Registered new key routes: `GET/POST /api/providers/{provider}/keys`,
`GET/PUT/DELETE /api/providers/{provider}/keys/{key_id}`
- Extracted inline anonymous structs into named `providerCreatePayload` and
`providerUpdatePayload` types (without `Keys` field)
- Removed `Keys` field from `ProviderResponse`
- Switched `addProvider` from `json.Unmarshal` to `sonic.Unmarshal`
- Removed `oldConfigRedacted` fetch and the entire key merge block
(`mergeKeys`, `hasKeys`, `slices` usage) from `updateProvider`
- Removed `Keys` from `getProviderResponseFromConfig` response builder
- Removed unused `encoding/json` import
### Context timeout fix (`server.go`)
- Split shared `bfCtx` in `ReloadProvider` into separate contexts:
`filteredBfCtx` (15s) for filtered `ListModelsRequest` and `unfilteredBfCtx`
(fresh 15s) for unfiltered `ListModelsRequest`, each cancelled after use
- Changed `GetKeysByProvider` to use `context.Background()` since it's a local
DB call that shouldn't be gated by model discovery timeouts
- Added `hasNoKeys` check to emit warn-level logs instead of errors when model
discovery fails because no keys are configured
- Read in-memory key count via `GetProviderKeysRaw` for the `hasNoKeys` check
### Tests (`providers_test.go`)
- Cleared file (contained only tests for removed inline struct decoding)
## Type of change
- [x] Feature
- [x] Bug fix
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Build
go build ./transports/bifrost-http/...
# Manual: start Bifrost, then test key CRUD
curl -X POST localhost:8080/api/providers/openai/keys -d '{"name":"test-key","value":"sk-test"}'
curl localhost:8080/api/providers/openai/keys
curl -X PUT localhost:8080/api/providers/openai/keys/{key_id} -d '{"name":"updated","value":"sk-new"}'
curl -X DELETE localhost:8080/api/providers/openai/keys/{key_id}
# Verify provider endpoints no longer return keys
curl localhost:8080/api/providers/openai | jq 'has("keys")' # should be false
```
## Screenshots/Recordings
N/A
## Breaking changes
- [x] Yes
- [ ] No
Provider API responses no longer include `keys` field. Provider create/update
payloads no longer accept `keys`. Clients must use the dedicated
`/api/providers/{provider}/keys/*` endpoints for key management.
## Related issues
N/A
## Security considerations
- Key handlers use existing redaction infrastructure (`GetProviderKeyRedacted`)
before returning responses
- Keyless provider validation prevents key creation on providers that don't
support keys
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: migrate frontend to dedicated provider keys API (#2161)
## Summary
Migrates the frontend from reading provider keys via `provider.keys` (removed
from provider API response in PR #2160) to the dedicated `getProviderKeys`
query and `/api/keys` endpoint. Removes `keys` from all provider TypeScript
types. Key mutations patch caches from authoritative server responses; provider
updates invalidate the `ProviderKeys` tag to refresh key statuses after model
discovery. Also adds a read-only routing rule info sheet.
## Changes
### Types (`config.ts`, `schemas.ts`)
- Removed `keys` field from `ModelProviderConfig`, `AddProviderRequest`, and
`UpdateProviderRequest`
- Added `CreateProviderKeyRequest`, `UpdateProviderKeyRequest`,
`ListProviderKeysResponse` types
### Store (`providersApi.ts`, `baseApi.ts`)
- Added `ProviderKeys` tag type to `baseApi`
- Changed `getProviderKeys`/`getProviderKey` from `Providers` tag to
`ProviderKeys` tag (avoids invalidating provider cache on key changes)
- Added `invalidatesTags: [ProviderKeys, DBKeys]` on `updateProvider` mutation
(refreshes key statuses after model discovery)
- Removed `getProvider`/`getProviders` cache patches from `createProviderKey`,
`updateProviderKey`, `deleteProviderKey` (providers no longer carry keys)
- Added duplicate-check guards on `createProviderKey` cache patches to prevent
ghost keys
- Each key mutation patches `getProviderKeys` and `getAllKeys` caches from
authoritative server response
### Components
- **`modelProviderKeysTableView.tsx`**: Already uses `useGetProviderKeysQuery`;
formatting/indentation fixes
- **`page.tsx`**: Removed `keys: []` from fallback provider object and
`createProvider` call; simplified `KeyDiscoveryFailedBadge` to only check
provider-level status (removed per-key status check since keys are no longer
on provider)
- **`routingRuleSheet.tsx`**: `TargetRow` now receives `allKeys` prop (from
`useGetAllKeysQuery`) instead of `providersData` with `.keys`; filters keys
by target provider
- **`routingRuleInfoSheet.tsx`**: New read-only sheet component that displays
routing rule details (conditions, targets with provider icons and weight bars,
fallback chain, scope, priority, timestamps)
- **`settingsPanel.tsx`**: Uses `useGetAllKeysQuery` to determine configured
providers (replaces `p.keys.length > 0` check) and derive
`providerKeyConfigs` per provider
### Other frontend changes (from prior commit, unchanged)
- Added `getProviderKeys`, `getProviderKey` RTK Query endpoints
- Added `createProviderKey`, `updateProviderKey`, `deleteProviderKey` mutations
- Added `buildProviderUpdatePayload` utility for key-free provider updates
- Migrated `providerKeyForm.tsx` to separate create/update mutations
- Updated `addNewKeySheet.tsx` props from `keyIndex` to `keyId`
- Updated all 6 provider form fragments to use `buildProviderUpdatePayload`
- Removed dead `selectedProvider.keys` sync matchers from `providerSlice.ts`
## Type of change
- [x] Feature
- [x] Refactor
- [ ] Bug fix
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
```sh
cd ui
npm run build
npm run lint
```
Manual testing:
1. Navigate to Providers page, select a provider with keys
2. Verify keys table loads correctly from dedicated API
3. Create a new key — verify it appears immediately (no ghost/duplicate)
4. Toggle enable/disable — verify switch updates immediately
5. Edit a key — verify form pre-populates, save works
6. Delete a key — verify it disappears immediately
7. Update provider settings — verify key statuses refresh after save
8. Check sidebar badge shows provider-level discovery failures
9. Open Playground settings — verify provider/key dropdowns work
10. Open Routing Rules — verify target key selector works
11. Click a routing rule row — verify info sheet opens with correct details
(conditions, targets, fallbacks, scope, priority)
## Screenshots/Recordings
N/A — no visual changes to existing features; routing rule info sheet is new.
## Breaking changes
- [ ] Yes
- [x] No
Frontend-only changes consuming the new API shape from PR #2160.
## Related issues
N/A
## Security considerations
No new security considerations. Key values continue to be handled through
existing redaction on the backend.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* refactor: replace string slice with WhiteList type for model restrictions (#2282)
## Summary
Refactored model access control logic by replacing string slice with a dedicated `WhiteList` type for the `Models` field in `TableKey`. This change introduces a more structured approach to handling wildcard permissions and improves code readability.
## Changes
- Changed `Models` field type from `[]string` to `schemas.WhiteList` in `TableKey` struct
- Replaced manual wildcard checking (`model == "*"`) with `IsUnrestricted()` method calls across multiple functions
- Added missing mock method `GetVirtualKeyMCPConfigsByMCPClientIDs` to test configuration store
- Applied the refactoring consistently in `ReloadProvider`, `ForceReloadPricing`, and `Bootstrap` methods
## Type of change
- [x] Refactor
- [ ] Bug fix
- [ ] Feature
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that model access control continues to work correctly with both wildcard and specific model permissions:
```sh
# Core/Transports
go version
go test ./...
# Test specific areas affected by the changes
go test ./framework/configstore/tables/...
go test ./transports/bifrost-http/...
```
Test scenarios should include:
- Keys with wildcard permissions (`["*"]`)
- Keys with specific model restrictions
- Keys with empty model lists (deny-by-default behavior)
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
This refactoring maintains the existing security model for API key permissions. The deny-by-default behavior and wildcard functionality remain unchanged, just implemented through a more structured type system.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: add Plus icon and responsive text to pricing override create button (#2285)
## Summary
Improves the visual design and mobile responsiveness of the pricing overrides section by adding a Plus icon to the create button and optimizing the button text for different screen sizes.
## Changes
- Added Plus icon import from lucide-react
- Enhanced the "Create Override" button with a Plus icon and responsive text that shows "New Override" on larger screens and hides text on mobile
- Adjusted container spacing by removing top margin and changing flex alignment from `items-start` to `items-center` for better visual balance
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Navigate to the custom pricing overrides page and verify:
1. The "New Override" button displays with a Plus icon
2. On mobile screens, only the Plus icon is visible
3. On larger screens (sm and above), both icon and "New Override" text are visible
4. The button functionality remains unchanged when clicked
```sh
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
## Screenshots/Recordings
Before/after screenshots showing the button design changes and responsive behavior would be helpful.
## Breaking changes
- [x] Yes
- [ ] No
## Related issues
## Security considerations
No security implications - this is a purely visual enhancement.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* refactor: blacklist models on new convention (#2305)
## Summary
Implements comprehensive blacklist support for model filtering across all providers. This adds the ability to explicitly deny access to specific models at the key level, with blacklist rules taking precedence over allowlist rules.
## Changes
- Added `BlackList` type with semantic validation (supports wildcard "*" for block-all)
- Updated key selection logic to check both allowlist and blacklist constraints
- Modified all provider model listing functions to filter out blacklisted models
- Enhanced UI to support blacklist configuration with improved UX for wildcard selection
- Added blacklist filtering to model catalog and provider handlers
- Updated test cases to verify blacklist functionality
Key design decisions:
- Blacklist always wins over allowlist when conflicts occur
- Wildcard "*" in blacklist blocks all models for that key
- Empty blacklist blocks nothing (permissive default)
- Consistent filtering logic across all providers (Anthropic, Azure, Bedrock, Cohere, etc.)
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Test blacklist functionality with provider keys:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Example configuration to test:
```json
{
"keys": [{
"id": "test-key",
"models": ["*"],
"blacklisted_models": ["gpt-4", "claude-3"]
}]
}
```
Verify that blacklisted models are excluded from model listings and key selection.
## Screenshots/Recordings
UI now shows "Blocked Models" field with improved tooltips and wildcard handling for denying access to specific models.
## Breaking changes
- [ ] Yes
- [x] No
The `blacklisted_models` field was already present in the schema but not fully implemented. This change makes it functional without breaking existing configurations.
## Related issues
Enhances model access control capabilities for fine-grained permission management.
## Security considerations
Improves security by allowing explicit denial of access to sensitive or expensive models at the key level. Blacklist rules cannot be bypassed by allowlist configurations.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* minor fix add blacklisted model field in tableKeyFromSchemaKey (#2324)
## Summary
This PR adds support for the `BlacklistedModels` field when converting schema keys to table keys in the configuration store's RDB implementation.
## Changes
- Added `BlacklistedModels: key.BlacklistedModels` field mapping in the `tableKeyFromSchemaKey` function
- Ensures that blacklisted model information is properly preserved when converting between schema and table representations
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that configuration keys with blacklisted models are properly stored and retrieved from the RDB configstore.
```sh
# Core/Transports
go version
go test ./...
```
Test creating configuration entries with `BlacklistedModels` specified and ensure they persist correctly through the RDB layer.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
None - this change only adds field mapping for existing blacklisted models functionality.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: add image edit input view on logs (#2321)
## Summary
Adds support for logging image edit and image variation requests by introducing new database columns and UI components to track and display these image manipulation operations alongside existing image generation functionality.
## Changes
- Added `image_edit_input` and `image_variation_input` columns to the logs table with corresponding database migrations
- Extended the Log struct with new fields for storing and parsing image edit/variation input data
- Updated logging plugin to capture image edit and variation request data with large payload threshold handling
- Enhanced UI to display input images and prompts for image edit operations and input images for variation operations
- Added image MIME type detection for proper display of base64-encoded images in the UI
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Do…
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
2 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
Migrates VK provider config allowed keys from implicit allow-all semantics to explicit deny-by-default behavior. Adds
AllowAllKeysboolean field to enable granular key access control while maintaining backward compatibility.Changes
AllowAllKeysboolean field toTableVirtualKeyProviderConfigwith database migrationallow_all_keys=trueto preserve current behavior["*"]wildcard allows all keysincludeOnlyKeysslice when no keys are configured["*"]wildcard and setAllowAllKeysflag appropriately["*"]wildcard in config filesType of change
Affected areas
How to test
Validate the migration and new key access control behavior:
Test scenarios:
key_ids- should deny all keyskey_ids: ["*"]- should allow all keysScreenshots/Recordings
UI now shows:
Breaking changes
The migration preserves existing behavior by setting
allow_all_keys=truefor configs that previously had no keys specified.Related issues
Part of VK access control enhancement initiative.
Security considerations
Improves security posture by implementing deny-by-default semantics for key access. Existing deployments maintain current access patterns through automatic backfill migration.
Checklist
docs/contributing/README.mdand followed the guidelines