feat: add AllowAllKeys flag and fix virtual key config semantics#2008
Closed
Pratham-Mishra04 wants to merge 1 commit intographite-base/2008from
Closed
Conversation
|
|
Collaborator
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds explicit allow-vs-deny semantics for virtual key provider and MCP configs, persists an AllowAllKeys flag into provider config hashing, expands governance virtual key entries during config load with deprecation warnings, wires a package logger for DB tables, and includes minor UI and test import tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CFG as Config File Loader
participant PRE as preprocessGovernanceVirtualKeys
participant MERGE as Governance Merger
participant STORE as Governance Store
participant HASH as GenerateVirtualKeyHash
CFG->>PRE: provide configData (virtual_keys)
PRE->>PRE: expand absent provider_configs -> full provider list (emit deprecation warning)
PRE->>PRE: expand absent mcp_configs -> full MCP list (emit deprecation warning)
PRE->>MERGE: return processed configData
MERGE->>STORE: compare/merge governance entries
STORE->>HASH: compute provider config hashes (includes AllowAllKeys)
HASH-->>STORE: return deterministic hashes
MERGE->>STORE: apply updates (propagate AllowAllKeys to existing entries)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate 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: 2
🧹 Nitpick comments (2)
transports/changelog.md (2)
3-3: Consider clarifying the MCP tools permission mechanism.The phrase "tools not permitted by the VK" could be more specific. Consider explicitly mentioning that this refers to the
tools_to_executeconfiguration in the virtual key's MCP config, e.g., "tools not listed in the VK'smcp_configs[*].tools_to_execute".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` at line 3, Update the changelog entry to explicitly state which configuration controls allowed tools: replace "tools not permitted by the VK" with a precise reference to the virtual key's MCP config (e.g., the mcp_configs[*].tools_to_execute field) so it reads that tools not listed in the VK's mcp_configs[*].tools_to_execute are blocked at inference and MCP tool execution, ensuring readers know the exact symbol/name controlling permissions.
22-23: Clarify the migration guidance for empty provider_configs/mcp_configs.The instruction "list entries explicitly instead of using an empty array" could be more specific. Does this mean:
- Enumerate each provider/MCP client you want to allow (e.g.,
provider_configs: [{provider_id: "openai", ...}, {provider_id: "anthropic", ...}])?- Or is there wildcard support like with
allowed_keys?Based on the feature description in line 1 (wildcard support mentioned only for key_ids), it appears wildcards are not supported for top-level
provider_configs/mcp_configs. If that's correct, consider clarifying: "list all providers/MCP clients you want to allow by including an explicit entry for each one" or similar wording.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` around lines 22 - 23, Update the changelog text to explicitly state that wildcards are not supported for top-level provider_configs and mcp_configs and instruct users to enumerate each allowed entry; e.g., clarify that you must list all providers or MCP clients you want to permit by adding an explicit entry for each (such as a provider_configs entry with provider_id "openai" and its config), rather than using an empty array or a wildcard like allowed_keys supports for key_ids.
🤖 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 229-245: The deprecation log uses mc.MCPClientName before it is
assigned, so it prints empty; fix by either assigning mc.MCPClientName =
temp.MCPClientName before the log or change the log to reference
temp.MCPClientName (instead of mc.MCPClientName) when logging the absent
tools_to_execute deprecation; ensure you still set mc.ToolsToExecute from
temp.ToolsToExecute and keep the MCPClientName capture (temp.MCPClientName ->
mc.MCPClientName) after the tools handling.
In `@transports/bifrost-http/lib/config.go`:
- Around line 1073-1129: Move the implicit expansion loop (the block that
expands absent vk.ProviderConfigs and vk.MCPConfigs) out of
mergeGovernanceConfig and run it earlier, before the codepath that branches for
creating vs merging DB state (i.e., call it from loadGovernanceConfigFromFile /
createGovernanceConfigInStore paths). In that helper, build provider expansions
from the runtime config that includes store-backed providers (use
config.Providers rather than configData.Providers), and for provider entries
resolve to stable DB identifiers the same way MCP configs are resolved (mirror
the resolveMCPConfigClientIDs approach for provider entries or call the existing
provider-ID resolver if present) so vk.ProviderConfigs and vk.MCPConfigs are
normalized (with resolved IDs) before either DB branch runs. Ensure the helper
is invoked prior to any persistence so the persisted VirtualKeys never contain a
nil/empty ambiguity.
---
Nitpick comments:
In `@transports/changelog.md`:
- Line 3: Update the changelog entry to explicitly state which configuration
controls allowed tools: replace "tools not permitted by the VK" with a precise
reference to the virtual key's MCP config (e.g., the
mcp_configs[*].tools_to_execute field) so it reads that tools not listed in the
VK's mcp_configs[*].tools_to_execute are blocked at inference and MCP tool
execution, ensuring readers know the exact symbol/name controlling permissions.
- Around line 22-23: Update the changelog text to explicitly state that
wildcards are not supported for top-level provider_configs and mcp_configs and
instruct users to enumerate each allowed entry; e.g., clarify that you must list
all providers or MCP clients you want to permit by adding an explicit entry for
each (such as a provider_configs entry with provider_id "openai" and its
config), rather than using an empty array or a wildcard like allowed_keys
supports for key_ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87d83a58-eaf4-4aa1-9596-5fa87e38da97
📒 Files selected for processing (6)
framework/configstore/clientconfig.goframework/configstore/tables/virtualkey.goplugins/governance/resolver.gotransports/bifrost-http/lib/config.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
Pratham-Mishra04
force-pushed
the
03-10-fix_backwards_compact_with_config.json_loading_for_vk_changes
branch
from
8569ce1 to
6a9f904
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 89-92: The logger.Warn calls in virtualkey.go use printf verbs but
the Logger interface treats args as structured fields, so replace the two calls
that pass format strings (the one referencing pc.Provider and the one
referencing temp.MCPClientName) with calls that pass a fully formatted string
created via fmt.Sprintf; update imports to include fmt if missing and change the
calls that currently read logger.Warn("[DEPRECATED] ... %q ...", pc.Provider)
and logger.Warn("[DEPRECATED] ... %q ...", temp.MCPClientName) to
logger.Warn(fmt.Sprintf("...%q...", pc.Provider)) and
logger.Warn(fmt.Sprintf("...%q...", temp.MCPClientName)) respectively.
In `@transports/bifrost-http/lib/config.go`:
- Around line 986-1004: The GenerateVirtualKeyHash is only stable when
vk.MCPConfigs contains resolved MCPClientID values, but currently only the
synthetic allow-all branch calls resolveMCPConfigClientIDs; fix by resolving
explicit vk.MCPConfigs as well (call resolveMCPConfigClientIDs(ctx,
config.ConfigStore, vk.MCPConfigs, vk.ID) whenever vk.MCPConfigs is non-nil or
has entries) before invoking GenerateVirtualKeyHash so that MCPClientName
entries are converted to MCPClientID and the resulting ConfigHash will change
when the target MCP client changes.
In `@transports/changelog.md`:
- Around line 22-30: Update the changelog wording to clarify that the
"allow-all" behavior is a startup snapshot produced by
preprocessGovernanceVirtualKeys: when provider_configs or mcp_configs (and their
nested allowed_keys or tools_to_execute) are omitted,
preprocessGovernanceVirtualKeys expands them only to the set of providers and
MCP clients present at boot time, not to any providers/clients added later; make
this explicit and mention that the implicit allow-all is not a live wildcard and
will not include subsequent runtime additions so users must explicitly list
entries to cover future providers/clients.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ec4986a-9e0b-4c04-aedf-db45b10302e7
📒 Files selected for processing (9)
framework/configstore/clientconfig.goframework/configstore/tables/encryption_test.goframework/configstore/tables/init.goframework/configstore/tables/virtualkey.goplugins/governance/resolver.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/main.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
- plugins/governance/resolver.go
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-10-fix_backwards_compact_with_config.json_loading_for_vk_changes
branch
from
6a9f904 to
3632e84
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)
framework/configstore/clientconfig.go (1)
655-673:⚠️ Potential issue | 🟠 MajorIgnore
KeyIDswhenAllowAllKeysis true.
framework/configstore/tables/virtualkey.goalready treatsKeysas inactive onceAllowAllKeysis set, but this hash still includes every key ID. That makes reconciliation depend on stale/preloaded associations instead of the effective policy, so wildcard configs can keep hashing differently after migration.💡 Suggested fix
- keyIDs := make([]string, len(pc.Keys)) - for j, k := range pc.Keys { - keyIDs[j] = k.KeyID - } - sort.Strings(keyIDs) + keyIDs := []string{} + if !pc.AllowAllKeys { + keyIDs = make([]string, len(pc.Keys)) + for j, k := range pc.Keys { + keyIDs[j] = k.KeyID + } + sort.Strings(keyIDs) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/clientconfig.go` around lines 655 - 673, The hash input currently includes every KeyID even when pc.AllowAllKeys is true; change the logic in the VirtualKeyProviderConfigHashInput construction so KeyIDs is omitted/empty when pc.AllowAllKeys is true (e.g., after collecting/sorting keyIDs, if pc.AllowAllKeys then set keyIDs = nil or an empty slice) so providerConfigsForHash entries reflect the effective policy rather than stale key associations; ensure this uses the pc.AllowAllKeys check and assigns the adjusted keyIDs to the KeyIDs field.
♻️ Duplicate comments (3)
framework/configstore/tables/virtualkey.go (1)
89-92:⚠️ Potential issue | 🟡 MinorPre-format these deprecation warnings before calling
Warn.Both call sites still pass
%qas thoughlogger.Warnwere printf-style. IfWarntreats extra args as structured fields, the provider/client name will not be interpolated and the warning text will be misleading.💡 Suggested fix
- logger.Warn("[DEPRECATED] virtual key provider config for provider %q has no allowed_keys in config.json — "+ - "defaulting to allow all keys. This implicit allow-all will be removed in the next major version. "+ - "Use allowed_keys: [\"*\"] to allow all keys explicitly, or list specific key names.", - pc.Provider) + logger.Warn(fmt.Sprintf("[DEPRECATED] virtual key provider config for provider %q has no allowed_keys in config.json — "+ + "defaulting to allow all keys. This implicit allow-all will be removed in the next major version. "+ + "Use allowed_keys: [\"*\"] to allow all keys explicitly, or list specific key names.", + pc.Provider)) ... - logger.Warn("[DEPRECATED] virtual key MCP config for client %q has no tools_to_execute in config.json — "+ - "defaulting to allow all tools. This implicit allow-all will be removed in the next major version. "+ - "Use tools_to_execute: [\"*\"] to allow all tools explicitly, or list specific tool names.", - temp.MCPClientName) + logger.Warn(fmt.Sprintf("[DEPRECATED] virtual key MCP config for client %q has no tools_to_execute in config.json — "+ + "defaulting to allow all tools. This implicit allow-all will be removed in the next major version. "+ + "Use tools_to_execute: [\"*\"] to allow all tools explicitly, or list specific tool names.", + temp.MCPClientName))#!/bin/bash set -euo pipefail echo "Logger interface:" sed -n '/type Logger interface/,/}/p' core/schemas/logger.go echo echo "Warn call in provider config unmarshal:" sed -n '84,96p' framework/configstore/tables/virtualkey.go echo echo "Warn call in MCP config unmarshal:" sed -n '228,240p' framework/configstore/tables/virtualkey.goAlso applies to: 233-236
🤖 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 89 - 92, The deprecation warnings pass a printf-style format and extra args to logger.Warn (e.g., logger.Warn("...%q...", pc.Provider)), but Warn treats extra args as structured fields; pre-format the message before calling Warn by using fmt.Sprintf to interpolate pc.Provider (and the other similar call site that interpolates the MCP/provider name) so the final call is logger.Warn(fmt.Sprintf("...%q...", pc.Provider)) (do the same for the second Warn at the MCP config unmarshal that currently passes its provider/client name as a separate arg).transports/changelog.md (1)
22-30:⚠️ Potential issue | 🟡 MinorClarify that omitted top-level configs expand to a startup snapshot, not a live wildcard.
The migration note still reads like absent
provider_configs/mcp_configsautomatically cover later additions. InpreprocessGovernanceVirtualKeys, they expand only to the providers and MCP clients present during boot, so runtime additions are not implicitly included.📝 Suggested wording
-**`provider_configs: []` / `mcp_configs: []`** — previously allowed all; now deny all. To keep allow-all behaviour, list entries explicitly instead of using an empty array. +**`provider_configs: []` / `mcp_configs: []`** — previously allowed all; now deny all. To keep the current implicit behaviour, list the providers / MCP clients explicitly instead of using an empty array. Omitting the field today expands only to whatever is configured at startup; it is not a live wildcard for providers or MCP clients added later. @@ -Omitting `provider_configs`, `mcp_configs`, `allowed_keys`, or `tools_to_execute` entirely (absent key) currently defaults to allow-all and emits a startup warning. **This implicit allow-all will be removed in the next major version** — absent and `[]` will both mean deny-all (deny-by-default). Migrate by always specifying these fields explicitly in config.json. +Omitting `provider_configs`, `mcp_configs`, `allowed_keys`, or `tools_to_execute` entirely (absent key) currently defaults to an allow-all startup snapshot and emits a startup warning. **This implicit allow-all will be removed in the next major version** — absent and `[]` will both mean deny-all (deny-by-default). Migrate by always specifying these fields explicitly in config.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/changelog.md` around lines 22 - 30, Update the changelog text to clarify that omitting top-level provider_configs or mcp_configs is expanded as a startup snapshot (not a live wildcard): change wording to state that preprocessGovernanceVirtualKeys expands absent configs only to providers and MCP clients present at boot time and will not automatically include providers/clients added at runtime; ensure the deprecation notice and the migration guidance reflect this boot-time snapshot behavior and reference preprocessGovernanceVirtualKeys so readers know where expansion happens.transports/bifrost-http/lib/config.go (1)
988-1004:⚠️ Potential issue | 🟠 MajorResolve explicit
mcp_configsbefore generatingConfigHash.
preprocessGovernanceVirtualKeysonly resolves client names for the synthetic allow-all branch. Explicitmcp_configsfromconfig.jsonstill hit Line 1139 and Line 1480 withMCPClientID == 0, so changingmcp_client_namecan leaveConfigHashunchanged and skip reconciliation.💡 Proposed fix
if vk.MCPConfigs == nil && config.MCPConfig != nil && len(config.MCPConfig.ClientConfigs) > 0 { logger.Warn( "virtual key %q has no mcp_configs in config.json — expanding to all %d configured MCP client(s). "+ "This implicit allow-all behaviour is DEPRECATED and will be removed in the next major version. "+ "Please add an explicit mcp_configs list to your config.json.", vk.ID, len(config.MCPConfig.ClientConfigs), ) unresolved := make([]configstoreTables.TableVirtualKeyMCPConfig, 0, len(config.MCPConfig.ClientConfigs)) for _, client := range config.MCPConfig.ClientConfigs { unresolved = append(unresolved, configstoreTables.TableVirtualKeyMCPConfig{ MCPClientName: client.Name, ToolsToExecute: []string{"*"}, }) } - // Resolve client names → DB IDs now so the hash is stable (MCPClientID is in the hash) - vk.MCPConfigs = resolveMCPConfigClientIDs(ctx, config.ConfigStore, unresolved, vk.ID) + vk.MCPConfigs = unresolved + } + + if len(vk.MCPConfigs) > 0 { + // Resolve client names → DB IDs now so the hash is stable (MCPClientID is in the hash) + vk.MCPConfigs = resolveMCPConfigClientIDs(ctx, config.ConfigStore, vk.MCPConfigs, vk.ID) } } }Also applies to: 1138-1176, 1477-1507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config.go` around lines 988 - 1004, preprocessGovernanceVirtualKeys currently only calls resolveMCPConfigClientIDs for the synthetic "allow-all" branch, leaving explicit vk.MCPConfigs from config.json unresolved (MCPClientID==0) which makes ConfigHash insensitive to mcp_client_name changes; fix by ensuring explicit vk.MCPConfigs are resolved to DB IDs before computing ConfigHash — i.e., in preprocessGovernanceVirtualKeys (and the other mentioned branches) call resolveMCPConfigClientIDs with the explicit vk.MCPConfigs (or convert them to unresolved TableVirtualKeyMCPConfig if needed) and assign the result back to vk.MCPConfigs so MCPClientID is populated prior to ConfigHash computation.
🤖 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 `@framework/configstore/clientconfig.go`:
- Around line 655-673: The hash input currently includes every KeyID even when
pc.AllowAllKeys is true; change the logic in the
VirtualKeyProviderConfigHashInput construction so KeyIDs is omitted/empty when
pc.AllowAllKeys is true (e.g., after collecting/sorting keyIDs, if
pc.AllowAllKeys then set keyIDs = nil or an empty slice) so
providerConfigsForHash entries reflect the effective policy rather than stale
key associations; ensure this uses the pc.AllowAllKeys check and assigns the
adjusted keyIDs to the KeyIDs field.
---
Duplicate comments:
In `@framework/configstore/tables/virtualkey.go`:
- Around line 89-92: The deprecation warnings pass a printf-style format and
extra args to logger.Warn (e.g., logger.Warn("...%q...", pc.Provider)), but Warn
treats extra args as structured fields; pre-format the message before calling
Warn by using fmt.Sprintf to interpolate pc.Provider (and the other similar call
site that interpolates the MCP/provider name) so the final call is
logger.Warn(fmt.Sprintf("...%q...", pc.Provider)) (do the same for the second
Warn at the MCP config unmarshal that currently passes its provider/client name
as a separate arg).
In `@transports/bifrost-http/lib/config.go`:
- Around line 988-1004: preprocessGovernanceVirtualKeys currently only calls
resolveMCPConfigClientIDs for the synthetic "allow-all" branch, leaving explicit
vk.MCPConfigs from config.json unresolved (MCPClientID==0) which makes
ConfigHash insensitive to mcp_client_name changes; fix by ensuring explicit
vk.MCPConfigs are resolved to DB IDs before computing ConfigHash — i.e., in
preprocessGovernanceVirtualKeys (and the other mentioned branches) call
resolveMCPConfigClientIDs with the explicit vk.MCPConfigs (or convert them to
unresolved TableVirtualKeyMCPConfig if needed) and assign the result back to
vk.MCPConfigs so MCPClientID is populated prior to ConfigHash computation.
In `@transports/changelog.md`:
- Around line 22-30: Update the changelog text to clarify that omitting
top-level provider_configs or mcp_configs is expanded as a startup snapshot (not
a live wildcard): change wording to state that preprocessGovernanceVirtualKeys
expands absent configs only to providers and MCP clients present at boot time
and will not automatically include providers/clients added at runtime; ensure
the deprecation notice and the migration guidance reflect this boot-time
snapshot behavior and reference preprocessGovernanceVirtualKeys so readers know
where expansion happens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e92bae4-fdca-4264-ac55-ee10f10b7c99
📒 Files selected for processing (8)
framework/configstore/clientconfig.goframework/configstore/tables/encryption_test.goframework/configstore/tables/init.goframework/configstore/tables/virtualkey.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/main.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/main.go
- framework/configstore/tables/init.go
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-10-fix_backwards_compact_with_config.json_loading_for_vk_changes
branch
from
3632e84 to
d69f60c
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)
framework/configstore/clientconfig.go (1)
653-673:⚠️ Potential issue | 🟠 MajorHash specific
allowed_keysby name until IDs are resolved.
AllowAllKeysfixes the allow-all/deny-all ambiguity, but this loop still hashes onlyk.KeyID. In the new config-file path,framework/configstore/tables/virtualkey.gomaterializes specificallowed_keysasTableKey{Name: ...}first, so["key-a"]and["key-b"]both collapse to the same empty-ID hash here. That meansmergeGovernanceConfigcan miss a real key-permission change and leave stale VK associations in store.Suggested fix
- keyIDs := make([]string, len(pc.Keys)) + keyRefs := make([]string, len(pc.Keys)) for j, k := range pc.Keys { - keyIDs[j] = k.KeyID + switch { + case k.KeyID != "": + keyRefs[j] = "id:" + k.KeyID + case k.Name != "": + keyRefs[j] = "name:" + k.Name + } } - sort.Strings(keyIDs) + sort.Strings(keyRefs) @@ - KeyIDs: keyIDs, + KeyIDs: keyRefs,Based on learnings: "In framework/configstore/clientconfig.go, when implementing GenerateKeyHash, prefix hashed keys with distinct identifiers: use 'env:' for environment-sourced values and 'val:' for literal values, mirroring the existing 'GenerateMCPClientHash' pattern to prevent hash collisions."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10d2e4ab-2887-4f8b-a8c0-bfd11cb2b231
📒 Files selected for processing (8)
framework/configstore/clientconfig.goframework/configstore/tables/encryption_test.goframework/configstore/tables/init.goframework/configstore/tables/virtualkey.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/main.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/tables/init.go
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
- framework/configstore/tables/encryption_test.go
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-10-fix_backwards_compact_with_config.json_loading_for_vk_changes
branch
from
d69f60c to
4af0fb2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/init.go`:
- Around line 5-9: The package-global logger variable is nil and UnmarshalJSON
in virtualkey.go calls logger.Warn() unguarded; initialize logger to a safe
no-op default that implements schemas.Logger (so logging calls are safe before
SetLogger) and keep SetLogger(l schemas.Logger) to replace that default;
alternatively, add nil checks around logger usage in UnmarshalJSON (functions
named UnmarshalJSON at virtualkey.go lines ~89 and ~231) to call logger only if
non-nil. Ensure the no-op implements the same methods as schemas.Logger so all
existing logger.Warn/info/error calls remain valid.
In `@framework/configstore/tables/virtualkey.go`:
- Around line 79-107: The code is using len(pc.Keys) == 0 to detect the
"config-file format" which causes an API/DB payload that explicitly set keys: []
to be treated as absent and default to allow-all; fix by detecting presence of
the keys field explicitly instead of relying on its length. Modify the
parsing/temp struct used here (the struct with temp.DBAllowAllKeys and
temp.AllowedKeys) to capture whether "keys" was present (e.g., make Keys a
pointer or add a KeysPresent bool populated during unmarshalling), then replace
the len(pc.Keys) == 0 check with a presence check (e.g., if !temp.KeysPresent)
so the branch that applies absent-vs-empty semantics for AllowedKeys only runs
when the config-file shape is actually parsed; keep the existing handling of
temp.DBAllowAllKeys, temp.AllowedKeys, pc.AllowAllKeys and pc.Keys otherwise.
🪄 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: 3e451ea0-06e2-41f7-9325-183c2b66c5c5
📒 Files selected for processing (8)
framework/configstore/clientconfig.goframework/configstore/tables/encryption_test.goframework/configstore/tables/init.goframework/configstore/tables/virtualkey.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/main.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/configstore/clientconfig.go
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
Comment on lines
+5
to
+9
| var logger schemas.Logger | ||
|
|
||
| // SetLogger sets the logger for the tables package. | ||
| func SetLogger(l schemas.Logger) { | ||
| logger = l |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tables package logger initialization:"
sed -n '1,40p' framework/configstore/tables/init.go
echo
echo "tables package logger call sites:"
rg -n 'logger\.(Warn|Info|Debug|Error|Fatal)\(' framework/configstore/tables -g '*.go'Repository: maximhq/bifrost
Length of output: 1008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Context around virtualkey.go logger calls (line 89) ==="
sed -n '85,95p' framework/configstore/tables/virtualkey.go
echo
echo "=== Context around virtualkey.go logger calls (line 231) ==="
sed -n '225,235p' framework/configstore/tables/virtualkey.go
echo
echo "=== Check for nil guards before logger usage ==="
rg -B5 'logger\.Warn' framework/configstore/tables/virtualkey.go | head -30
echo
echo "=== Search for SetLogger call sites ==="
rg -n 'SetLogger' --type go
echo
echo "=== Check if logger interface has a noop/nil-safe implementation ==="
rg -n 'type Logger' core/schemas -A10 --type goRepository: maximhq/bifrost
Length of output: 6880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full Logger interface definition ==="
sed -n '28,60p' core/schemas/logger.go
echo
echo "=== Search for noop or default logger implementations ==="
rg -n 'type.*Logger' core/schemas --type go -A3 | grep -A3 'struct'
echo
echo "=== Check where virtualkey.go is used (imported/loaded) ==="
rg -n 'virtualkey' --type go | head -20
echo
echo "=== Check for any config loading outside main.go ==="
rg -n 'LoadConfig\|configstore\|tables' core/bifrost.go --type go -B2 -A2
echo
echo "=== Check if there are other entry points or transports ==="
fd -t f 'main.go' --type go | grep -v testRepository: maximhq/bifrost
Length of output: 1501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if there are other entry points or transports ==="
find . -name 'main.go' -type f | grep -v test | grep -v '.git'
echo
echo "=== Where is virtualkey.go imported from? ==="
rg -n 'import.*virtualkey\|from.*virtualkey' --type go
echo
echo "=== Who imports tables package? ==="
rg -n 'configstore/tables' --type go | grep import
echo
echo "=== Check core/bifrost.go around line 183 (providerUtils.SetLogger) ==="
sed -n '175,195p' core/bifrost.go
echo
echo "=== Check core/bifrost.go for tables initialization/usage ==="
rg -n 'tables\.' core/bifrost.go --type go -B2 -A2Repository: maximhq/bifrost
Length of output: 913
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Who imports tables package? ==="
rg -n 'configstore/tables' --type go | grep -E '(import|from)' | head -20
echo
echo "=== Check core/bifrost.go for tables or configstore usage ==="
rg -n 'tables\|configstore' core/bifrost.go --type go -B1 -A1
echo
echo "=== Check cli/main.go for logger and config initialization ==="
sed -n '1,80p' cli/main.go
echo
echo "=== Check if there are integration tests that load configs before SetLogger ==="
rg -n 'tables\.' --type go | grep -E 'test|_test' | head -10Repository: maximhq/bifrost
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find where virtualkey.go is actually used/called ==="
rg -n 'VirtualKey\|UnmarshalJSON' framework/configstore/tables --type go | head -20
echo
echo "=== Check what loads config and calls virtualkey unmarshal ==="
rg -n 'json.Unmarshal\|json.Decoder' framework/configstore --type go -B2 -A2
echo
echo "=== Check if there's an init() function in tables package that might initialize logger ==="
rg -n 'func init' framework/configstore/tables --type go
echo
echo "=== Full search for where UnmarshalJSON is triggered ==="
rg -n 'UnmarshalJSON' framework/configstore/tables -A15 --type go | head -40
echo
echo "=== Check if virtualkey package is only used during HTTP server startup ==="
grep -r 'virtualkey\|tables\.' transports/bifrost-http --include='*.go' | grep -v test | head -20Repository: maximhq/bifrost
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List all files in framework/configstore/tables ==="
ls -la framework/configstore/tables/
echo
echo "=== Check content of virtualkey.go ==="
head -100 framework/configstore/tables/virtualkey.go
echo
echo "=== Search for json.Unmarshal calls in framework/configstore ==="
rg -n 'json\.' framework/configstore --type go -A3 | head -50
echo
echo "=== Check how tables package is initialized in transports/bifrost-http/main.go ==="
sed -n '140,160p' transports/bifrost-http/main.goRepository: maximhq/bifrost
Length of output: 10559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for any nil checks before logger.Warn calls in virtualkey.go ==="
sed -n '70,100p' framework/configstore/tables/virtualkey.go
echo
echo "=== Check around line 231 as well ==="
sed -n '215,245p' framework/configstore/tables/virtualkey.go
echo
echo "=== Check all files in CLI that might load configs ==="
head -50 cli/main.go
echo
echo "=== Check if logger interface could be nil-safe (checking Warn signature) ==="
grep -A3 'Warn(' core/schemas/logger.goRepository: maximhq/bifrost
Length of output: 3996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if CLI loads/unmarshals configs before bootstrap ==="
rg -n 'config\|Config\|Unmarshal' cli/internal/app --type go -B2 -A2 | head -100
echo
echo "=== Check if any other main binaries (oauth2, plugins) set logger ==="
for f in framework/oauth2/main.go plugins/*/main.go; do
if [ -f "$f" ]; then
echo "=== $f ==="
grep -n 'SetLogger\|tables\.' "$f" || echo " (no SetLogger or tables refs)"
fi
doneRepository: maximhq/bifrost
Length of output: 121
🏁 Script executed:
#!/bin/bash
set -euoripefail
echo "=== List files in cli/internal/app ==="
ls -la cli/internal/app/
echo
echo "=== Check main app file ==="
head -100 cli/internal/app/app.go
echo
echo "=== Search for where configstore or tables are used in CLI ==="
rg -n 'configstore\|tables' cli/ --type go
echo
echo "=== Search broadly for JSON unmarshalling in cli ==="
rg -n 'json\|Unmarshal' cli/ --type go | head -30Repository: maximhq/bifrost
Length of output: 733
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List files in cli/internal/app ==="
ls -la cli/internal/app/
echo
echo "=== Check main app file ==="
head -100 cli/internal/app/app.go
echo
echo "=== Search for where configstore or tables are used in CLI ==="
rg -n 'configstore\|tables' cli/ --type go
echo
echo "=== Search broadly for JSON unmarshalling in cli ==="
rg -n 'json\|Unmarshal' cli/ --type go | head -30Repository: maximhq/bifrost
Length of output: 3072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check all main.go files to see which ones might use configstore/tables ==="
for mainfile in $(find . -name 'main.go' -type f | grep -v test | grep -v '.git'); do
echo "=== $mainfile ==="
grep -l 'configstore\|tables\|SetLogger' "$mainfile" && echo "USES CONFIGSTORE/TABLES" || echo "does not use configstore"
done
echo
echo "=== Check framework/oauth2/main.go ==="
cat framework/oauth2/main.go
echo
echo "=== Check core/bifrost.go for tables/configstore usage (around line 183) ==="
sed -n '150,200p' core/bifrost.goRepository: maximhq/bifrost
Length of output: 27179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check plugins/governance/main.go for SetLogger call ==="
grep -n 'SetLogger\|tables\.' plugins/governance/main.go
echo
echo "=== Check plugins/logging/main.go for SetLogger call ==="
grep -n 'SetLogger\|tables\.' plugins/logging/main.go
echo
echo "=== Check framework/modelcatalog/main.go for SetLogger call ==="
grep -n 'SetLogger\|tables\.' framework/modelcatalog/main.go
echo
echo "=== Check the actual imports in these files to see what configstore they use ==="
head -50 plugins/governance/main.go | grep -A5 'import'
echo
echo "=== Check if any plugin loads virtualkey configs during init/main ==="
rg -n 'virtual.*key\|Virtual.*Key' plugins/ framework/modelcatalog/ --type go | head -20Repository: maximhq/bifrost
Length of output: 118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List what's in plugins/governance ==="
ls -la plugins/governance/
echo
echo "=== Check plugins/governance/main.go ==="
cat plugins/governance/main.go
echo
echo "=== Check plugins/logging/main.go ==="
cat plugins/logging/main.goRepository: maximhq/bifrost
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if governance plugin calls tables.SetLogger ==="
grep -n 'tables\.SetLogger\|configstoreTables\.SetLogger' plugins/governance/main.go
echo
echo "=== Check framework/modelcatalog/main.go ==="
head -100 framework/modelcatalog/main.go
echo
echo "=== Check if modelcatalog calls tables.SetLogger ==="
grep -n 'tables\.SetLogger\|SetLogger' framework/modelcatalog/main.go
echo
echo "=== Search for all SetLogger calls to tables package across the codebase ==="
rg -n 'tables\.SetLogger' --type goRepository: maximhq/bifrost
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ALL places where tables.SetLogger is actually called ==="
rg -n 'tables\.SetLogger' --type go
echo
echo "=== Check if governance plugin loads virtual key configs in Init ==="
grep -n 'VirtualKey\|UnmarshalJSON\|configstore' plugins/governance/main.go | head -20
echo
echo "=== Check what tables are loaded by governance in NewLocalGovernanceStore ==="
sed -n '1,100p' plugins/governance/store.go | grep -A10 'NewLocalGovernanceStore\|configStore'
echo
echo "=== Confirm: does governance plugin's store load virtual keys? ==="
rg -n 'GetVirtualKey\|TableVirtualKey' plugins/governance/store.go --type go | head -10Repository: maximhq/bifrost
Length of output: 1824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check transports/bifrost-http/main.go initialization order ==="
sed -n '130,160p' transports/bifrost-http/main.go
echo
echo "=== Check where governance plugin Init is called in the HTTP transport ==="
rg -n 'governance\.Init\|NewGovernancePlugin' transports/bifrost-http --type go -B3 -A3
echo
echo "=== Check server.go or server initialization for governance setup ==="
grep -n 'governance\|GovernancePlugin' transports/bifrost-http/server/server.go | head -20Repository: maximhq/bifrost
Length of output: 1167
Initialize the tables logger to a safe default or add nil checks before logging calls.
logger is an uninitialized package-global variable. The UnmarshalJSON methods in virtualkey.go (lines 89, 231) call logger.Warn() unconditionally without nil guards. Any code that unmarshals virtual key configs before tables.SetLogger() runs will panic on a nil interface method call. While the HTTP transport calls tables.SetLogger() before bootstrap, other entry points (governance plugin, oauth2 framework, modelcatalog) may load configs without wiring the logger. Initialize this package logger to a no-op default implementation, or guard all logger calls with nil checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/configstore/tables/init.go` around lines 5 - 9, The package-global
logger variable is nil and UnmarshalJSON in virtualkey.go calls logger.Warn()
unguarded; initialize logger to a safe no-op default that implements
schemas.Logger (so logging calls are safe before SetLogger) and keep SetLogger(l
schemas.Logger) to replace that default; alternatively, add nil checks around
logger usage in UnmarshalJSON (functions named UnmarshalJSON at virtualkey.go
lines ~89 and ~231) to call logger only if non-nil. Ensure the no-op implements
the same methods as schemas.Logger so all existing logger.Warn/info/error calls
remain valid.
f5103e6 to
2a794d7
Compare
Pratham-Mishra04
force-pushed
the
03-10-fix_backwards_compact_with_config.json_loading_for_vk_changes
branch
from
4af0fb2 to
695fc49
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
framework/configstore/tables/virtualkey.go (2)
89-90:⚠️ Potential issue | 🟡 MinorDeprecation warnings still use printf verbs directly in
logger.WarnBoth warnings pass
%qformat strings directly tologger.Warn. IfWarn(msg string, args ...any)is structured-logging style,%qis logged literally instead of interpolated.💡 Suggested fix
-logger.Warn("[DEPRECATED] virtual key provider config for provider %q has no allowed_keys in config.json — defaulting to allow all keys. This implicit allow-all will be removed in the next major version. Use allowed_keys: [\"*\"] to allow all keys explicitly, or list specific key names.", - pc.Provider) +logger.Warn(fmt.Sprintf("[DEPRECATED] virtual key provider config for provider %q has no allowed_keys in config.json — defaulting to allow all keys. This implicit allow-all will be removed in the next major version. Use allowed_keys: [\"*\"] to allow all keys explicitly, or list specific key names.", + pc.Provider)) -logger.Warn("[DEPRECATED] virtual key MCP config for client %q has no tools_to_execute in config.json — defaulting to allow all tools. This implicit allow-all will be removed in the next major version. Use tools_to_execute: [\"*\"] to allow all tools explicitly, or list specific tool names.", - temp.MCPClientName) +logger.Warn(fmt.Sprintf("[DEPRECATED] virtual key MCP config for client %q has no tools_to_execute in config.json — defaulting to allow all tools. This implicit allow-all will be removed in the next major version. Use tools_to_execute: [\"*\"] to allow all tools explicitly, or list specific tool names.", + temp.MCPClientName))Also applies to: 231-232
🤖 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 89 - 90, The warning uses printf verbs directly in logger.Warn which causes the `%q` to be logged literally; update the call that references pc.Provider in the virtual key provider deprecation message so the provider value is interpolated—either pre-format the message with fmt.Sprintf(...) and pass the resulting string to logger.Warn, or switch to structured logging by supplying the provider as a field (e.g., logger.With("provider", pc.Provider).Warn(...)). Apply the same fix to the other identical warning occurrence referenced in the file.
79-107:⚠️ Potential issue | 🟠 Major
keys: []is still misclassified as “absent”, causing unintended allow-allAt Line 82,
len(pc.Keys) == 0is used to detect config-file format. That makes an explicit DB/API payload withkeys: []fall into the deprecated absent-allowed_keyspath and setsAllowAllKeys = true, which violates the new explicit deny semantics.💡 Suggested fix
type Alias TableVirtualKeyProviderConfig type TempProviderConfig struct { Alias + DBKeys *[]TableKey `json:"keys"` // DB/API format; nil = absent AllowedKeys *[]string `json:"allowed_keys"` // Config file format; nil = absent DBAllowAllKeys *bool `json:"allow_all_keys"` // DB/API format; nil = absent } var temp TempProviderConfig if err := json.Unmarshal(data, &temp); err != nil { return err } *pc = TableVirtualKeyProviderConfig(temp.Alias) +if temp.DBKeys != nil { + pc.Keys = *temp.DBKeys +} if temp.DBAllowAllKeys != nil { pc.AllowAllKeys = *temp.DBAllowAllKeys -} else if len(pc.Keys) == 0 { +} else if temp.DBKeys == nil { // Config file format semantics for allowed_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 79 - 107, The code incorrectly uses len(pc.Keys) == 0 to detect whether the config-file "allowed_keys" field was present, causing an explicit DB/API payload with keys: [] to be treated as "absent" and default to allow-all; change the detection to check the parsed temp.AllowedKeys presence instead (i.e., replace the else if len(pc.Keys) == 0 branch condition with else if temp.AllowedKeys != nil and adjust the block to treat nil vs empty slice correctly), keeping the existing handling of temp.DBAllowAllKeys, pc.AllowAllKeys, pc.Keys, and the special ["*"] wildcard behavior.
🤖 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 89-90: The warning uses printf verbs directly in logger.Warn which
causes the `%q` to be logged literally; update the call that references
pc.Provider in the virtual key provider deprecation message so the provider
value is interpolated—either pre-format the message with fmt.Sprintf(...) and
pass the resulting string to logger.Warn, or switch to structured logging by
supplying the provider as a field (e.g., logger.With("provider",
pc.Provider).Warn(...)). Apply the same fix to the other identical warning
occurrence referenced in the file.
- Around line 79-107: The code incorrectly uses len(pc.Keys) == 0 to detect
whether the config-file "allowed_keys" field was present, causing an explicit
DB/API payload with keys: [] to be treated as "absent" and default to allow-all;
change the detection to check the parsed temp.AllowedKeys presence instead
(i.e., replace the else if len(pc.Keys) == 0 branch condition with else if
temp.AllowedKeys != nil and adjust the block to treat nil vs empty slice
correctly), keeping the existing handling of temp.DBAllowAllKeys,
pc.AllowAllKeys, pc.Keys, and the special ["*"] wildcard behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae2187e1-3277-4d7f-b805-08bb27dcfd2d
📒 Files selected for processing (4)
framework/configstore/clientconfig.goframework/configstore/tables/encryption_test.goframework/configstore/tables/init.goframework/configstore/tables/virtualkey.go
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/tables/init.go
- framework/configstore/clientconfig.go
- framework/configstore/tables/encryption_test.go
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-fix_backwards_compact_with_config.json_loading_for_vk_changes
branch
from
695fc49 to
736cc1f
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/configstore/tables/init.go (1)
5-9:⚠️ Potential issue | 🟠 MajorInitialize logger to a no-op default to prevent nil pointer panics.
The package-level
loggervariable isnilby default.UnmarshalJSONmethods invirtualkey.go(Lines 89 and 231) calllogger.Warn()unconditionally. If any code path unmarshals virtual key configs beforetables.SetLogger()is called (e.g., governance plugin, CLI tools, or tests), this will panic on a nil interface method call.Initialize the logger to a safe no-op default:
,
🛡️ Proposed fix
package tables -import "github.com/maximhq/bifrost/core/schemas" +import ( + bifrost "github.com/maximhq/bifrost/core" + "github.com/maximhq/bifrost/core/schemas" +) -var logger schemas.Logger +// logger defaults to a no-op logger to prevent nil panics if SetLogger is not called +// before config unmarshalling occurs. +var logger schemas.Logger = bifrost.NewDefaultLogger(schemas.LogLevelWarn) // SetLogger sets the logger for the tables package. func SetLogger(l schemas.Logger) { logger = l }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/init.go` around lines 5 - 9, The package-level variable logger is nil by default which causes panics when virtualkey.go's UnmarshalJSON methods call logger.Warn() before SetLogger() runs; initialize logger to a safe no-op implementation at declaration (e.g., assign a schemas.Logger no-op instance or a local noop struct that implements Warn/Info/Error) and keep SetLogger(l schemas.Logger) to replace it, ensuring UnmarshalJSON in virtualkey.go (the methods that call logger.Warn) can always call logger safely.
🧹 Nitpick comments (1)
transports/bifrost-http/lib/config.go (1)
1006-1013: Consider sorting expanded providers for deterministic ordering.Map iteration order in Go is non-deterministic. While this doesn't affect correctness (each provider config is independent), it may cause non-deterministic ordering in logs and serialized output. For consistency:
♻️ Optional: Sort providers for deterministic expansion order
+import "sort" + expanded := make([]configstoreTables.TableVirtualKeyProviderConfig, 0, len(config.Providers)) + providerNames := make([]string, 0, len(config.Providers)) for providerName := range config.Providers { + providerNames = append(providerNames, string(providerName)) + } + sort.Strings(providerNames) + for _, providerName := range providerNames { expanded = append(expanded, configstoreTables.TableVirtualKeyProviderConfig{ - Provider: string(providerName), + Provider: providerName, AllowAllKeys: true, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config.go` around lines 1006 - 1013, The expansion of provider configs uses map iteration over config.Providers which is non-deterministic; before building expanded (used to set vk.ProviderConfigs) collect the provider names into a slice, sort that slice, then iterate the sorted names to append TableVirtualKeyProviderConfig entries (Provider and AllowAllKeys) so the resulting expanded slice and any logs/serialization are deterministic; update the loop that currently ranges over config.Providers to use the sorted names.
🤖 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/init.go`:
- Around line 5-9: The package-level variable logger is nil by default which
causes panics when virtualkey.go's UnmarshalJSON methods call logger.Warn()
before SetLogger() runs; initialize logger to a safe no-op implementation at
declaration (e.g., assign a schemas.Logger no-op instance or a local noop struct
that implements Warn/Info/Error) and keep SetLogger(l schemas.Logger) to replace
it, ensuring UnmarshalJSON in virtualkey.go (the methods that call logger.Warn)
can always call logger safely.
---
Nitpick comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 1006-1013: The expansion of provider configs uses map iteration
over config.Providers which is non-deterministic; before building expanded (used
to set vk.ProviderConfigs) collect the provider names into a slice, sort that
slice, then iterate the sorted names to append TableVirtualKeyProviderConfig
entries (Provider and AllowAllKeys) so the resulting expanded slice and any
logs/serialization are deterministic; update the loop that currently ranges over
config.Providers to use the sorted names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48dd7c90-4ea5-4c97-a4bd-653c7441ea79
📒 Files selected for processing (8)
framework/configstore/clientconfig.goframework/configstore/tables/encryption_test.goframework/configstore/tables/init.goframework/configstore/tables/virtualkey.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/main.gotransports/changelog.mdui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/clientconfig.go
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
- framework/configstore/tables/encryption_test.go
Pratham-Mishra04
changed the base branch from
03-10-feat_vk_provider_config_key_ids_now_supports_wildcard_to_allow_all_keys_empty_key_ids_denies_all
to
graphite-base/2008
18 tasks
This was referenced Mar 17, 2026
fix: apply MCP tool filtering headers to tools/list response when using bifrost as MCP gateway
#2127
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Implements explicit allow/deny semantics for virtual key configurations to replace implicit behaviors. Previously, absent configuration fields had inconsistent behavior - some defaulted to allow-all, others to deny-all. This change establishes clear semantics where explicit empty arrays mean deny-all, while absent fields temporarily default to allow-all with deprecation warnings.
Changes
[]) forprovider_configs,mcp_configs, andallowed_keysnow explicitly deny access instead of allowing allallowed_keysandtools_to_executefields now allow all (previously buggy deny-all) with deprecation warningsAllowAllKeysboolean field toVirtualKeyProviderConfigHashInputand related hash generation logicAllowAllKeysis trueprovider_configs/mcp_configs) to all configured providers/clients during config mergeType of change
Affected areas
How to test
Test the new explicit semantics with various virtual key configurations:
Verify deprecation warnings appear in logs when using absent fields, and that explicit empty arrays properly deny access.
Breaking changes
Migration required for configurations using explicit empty arrays:
provider_configs: []allowed_keys: []["*"]for allow-all or list specific keysmcp_configs: []Absent fields now consistently allow-all with deprecation warnings. In the next major version, both absent and
[]will mean deny-all.Security considerations
This change improves security by making access controls explicit and preventing accidental over-permissioning through empty arrays. The temporary allow-all behavior for absent fields maintains backward compatibility while encouraging explicit configuration through deprecation warnings.
Checklist
docs/contributing/README.mdand followed the guidelines