feat: use schemas.SecretVar for virtual key values to support env store - #4817
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (31)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (27)
📝 WalkthroughSummary by CodeRabbit
WalkthroughVirtual-key values now flow through ChangesVirtual Key SecretVar Migration
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BearTS
changed the title
TableVirtualKey.Value from string to schemas.SecretVar
BearTS
marked this pull request as ready for review
July 1, 2026 07:33
Contributor
Confidence Score: 5/5Safe to merge; both remaining findings are non-blocking quality issues that do not affect runtime correctness or security. The core refactor is thorough and consistent across all 31 files. The framework/configstore/tables/virtualkey.go (EncryptionStatus guard) and ui/lib/utils/validation.ts (isValidVertexAuthCredentials length check now dead code). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant GovernanceHandler
participant GORM
participant BeforeSave
participant VaultStore
participant DB
Client->>GovernanceHandler: POST /governance/virtual-keys
GovernanceHandler->>GORM: "Create(TableVirtualKey{Value: SecretVar})"
GORM->>BeforeSave: vaultStoreCallback (skipped — VaultStoreSelfManaged)
GORM->>BeforeSave: BeforeSave hook
BeforeSave->>BeforeSave: resolve GetValue() → error if empty
BeforeSave->>BeforeSave: HashSHA256(resolved) → ValueHash
BeforeSave->>VaultStore: StoreOwnedVaultSecretVars (if vault enabled)
VaultStore-->>BeforeSave: vault ref written to SecretVar.ref
BeforeSave->>BeforeSave: encryptSecretVar (plain-text only)
BeforeSave-->>GORM: ok
GORM->>DB: INSERT with encrypted/ref value
DB-->>GovernanceHandler: row saved
GovernanceHandler->>Client: MarshalJSON → value as plain string
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant GovernanceHandler
participant GORM
participant BeforeSave
participant VaultStore
participant DB
Client->>GovernanceHandler: POST /governance/virtual-keys
GovernanceHandler->>GORM: "Create(TableVirtualKey{Value: SecretVar})"
GORM->>BeforeSave: vaultStoreCallback (skipped — VaultStoreSelfManaged)
GORM->>BeforeSave: BeforeSave hook
BeforeSave->>BeforeSave: resolve GetValue() → error if empty
BeforeSave->>BeforeSave: HashSHA256(resolved) → ValueHash
BeforeSave->>VaultStore: StoreOwnedVaultSecretVars (if vault enabled)
VaultStore-->>BeforeSave: vault ref written to SecretVar.ref
BeforeSave->>BeforeSave: encryptSecretVar (plain-text only)
BeforeSave-->>GORM: ok
GORM->>DB: INSERT with encrypted/ref value
DB-->>GovernanceHandler: row saved
GovernanceHandler->>Client: MarshalJSON → value as plain string
Reviews (11): Last reviewed commit: "feat: add secretVar support to VK" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 3
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/mcpserver.go (1)
348-365: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject unresolved VK secrets before server lookup/cache insertion.
GetValue()is now the per-VK server key, but an unresolved env/vault-backed VK resolves to"". That makes distinct broken VKs sharevkMCPServers[""], so JWT- and user-scoped MCP requests can hit the wrong scoped server instead of failing closed.Suggested guard
- vkServer, err := h.ensureVKMCPServerByValue(ctx, vk.Value.GetValue()) + vkValue := vk.Value.GetValue() + if vkValue == "" { + return nil, fmt.Errorf("virtual key value is unresolved") + } + vkServer, err := h.ensureVKMCPServerByValue(ctx, vkValue)As per coding guidelines, transport/governance auth paths should fail closed, and as per path instructions this PR’s secret-backed VK values must be keyed off the resolved plaintext accessor without silently accepting ambiguous empty identities.
Also applies to: 661-662, 752-753, 839-839
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/handlers/mcpserver.go` around lines 348 - 365, Reject unresolved VK secrets before using SyncVKMCPServer’s cache key: currently vk.Value.GetValue() can return an empty string for env/vault-backed VKs, causing distinct invalid VKs to share vkMCPServers[""] and potentially reuse the wrong MCPServer. Add a fail-closed guard at the start of MCPServerHandler.SyncVKMCPServer (before the vkMCPServers lookup/insertion) to require a resolved, non-empty plaintext VK identity, and return or skip syncing when it is missing. Apply the same check wherever the VK server key is derived in the related call sites that use GetValue().Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/governance/store.go`:
- Around line 2360-2362: The virtual-key indexing in gs.storeVirtualKey should
fail closed when vk.Value.GetValue() resolves to an empty string, since
secret-backed VKs can be missing and collapse into a shared empty-key entry. Add
a guard in the virtual key handling paths (including the loops around
gs.virtualKeys) to skip or reject empty resolved values before caching, and keep
the lookup keyed by the resolved plaintext value only when it is non-empty.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2269-2277: The virtual key validation in the governance config
path is only checking the prefix for non-secret values, so env/vault-backed
values can bypass the same rule. Update the VirtualKeys handling in the
governance config loader to validate the resolved plaintext from
Value.GetValue() for all entries, including secret-backed ones, and only accept
values that already have the governance.VirtualKeyPrefix; otherwise generate a
new key and replace the Value using the existing NewSecretVar flow.
- Around line 2262-2278: The virtual key sync logic in config.go regenerates a
new value inside the existingVirtualKey.ConfigHash / forceFileSync branch
whenever the config file contains an invalid explicit value, which causes
repeated VK rotation on reload. Update the sync path around the virtual key loop
to preserve the already-stored/generated value from existingVirtualKey.Value
when the config value is invalid, or normalize the config value once before the
hash comparison so future forced syncs do not trigger regeneration. Use the
configData.Governance.VirtualKeys[i] update flow and the
existingVirtualKey.Value / governance.GenerateVirtualKey handling to keep stable
VKs across reloads.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcpserver.go`:
- Around line 348-365: Reject unresolved VK secrets before using
SyncVKMCPServer’s cache key: currently vk.Value.GetValue() can return an empty
string for env/vault-backed VKs, causing distinct invalid VKs to share
vkMCPServers[""] and potentially reuse the wrong MCPServer. Add a fail-closed
guard at the start of MCPServerHandler.SyncVKMCPServer (before the vkMCPServers
lookup/insertion) to require a resolved, non-empty plaintext VK identity, and
return or skip syncing when it is missing. Apply the same check wherever the VK
server key is derived in the related call sites that use GetValue().
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a68642f7-4b60-4e67-8e43-46d1efef3154
📒 Files selected for processing (25)
framework/configstore/clientconfig.goframework/configstore/encryption_test.goframework/configstore/migrations_test.goframework/configstore/rdb_deadlock_postgres_test.goframework/configstore/rdb_mcp_sessions_test.goframework/configstore/rdb_oauth2_test.goframework/configstore/rdb_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/virtualkey.goframework/logstore/asyncjob_test.goplugins/governance/store.goplugins/governance/test_utils.gotests/cmd/seed/seed.gotests/cmd/seedvks/main.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/bifrost-http/handlers/list_models_vk_test.gotransports/bifrost-http/handlers/mcpoauth2consent_test.gotransports/bifrost-http/handlers/mcpoauth2jwt.gotransports/bifrost-http/handlers/mcpoauth2jwt_test.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/mcpserver_auth_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.go
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
3 times, most recently
from
July 1, 2026 08:26
bd7a2f4 to
205d025
Compare
BearTS
changed the title
TableVirtualKey.Value from string to schemas.SecretVarTableVirtualKey.Value from string to schemas.SecretVar
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 1, 2026 09:10
205d025 to
c0b80f5
Compare
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 1, 2026 09:18
c0b80f5 to
22dd587
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2271-2280: Update the warning messages in the virtual key handling
logic so they match the actual branch behavior in config.go around the
VirtualKeys sync path. In the code that decides between existingVirtualKey.Value
and governance.GenerateVirtualKey(), keep the warning for invalid config-file
literals when a fresh key is actually generated, but change the unresolved
env/vault ref warning to explicitly mention the fallback action used in that
branch. Use the existing identifiers configData.Governance.VirtualKeys,
existingVirtualKey.Value, schemas.NewSecretVar, and
governance.GenerateVirtualKey() to locate the affected conditions and make each
warning action-specific.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d41a4889-2123-4e4a-90cf-0feba26f16cb
📒 Files selected for processing (28)
framework/configstore/clientconfig.goframework/configstore/encryption_test.goframework/configstore/migrations_test.goframework/configstore/rdb_deadlock_postgres_test.goframework/configstore/rdb_mcp_sessions_test.goframework/configstore/rdb_oauth2_test.goframework/configstore/rdb_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/virtualkey.goframework/logstore/asyncjob_test.goplugins/governance/store.goplugins/governance/store_test.goplugins/governance/test_utils.gotests/cmd/seed/seed.gotests/cmd/seedvks/main.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/bifrost-http/handlers/list_models_vk_test.gotransports/bifrost-http/handlers/mcpoauth2consent_test.gotransports/bifrost-http/handlers/mcpoauth2jwt.gotransports/bifrost-http/handlers/mcpoauth2jwt_test.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/mcpserver_auth_test.gotransports/bifrost-http/handlers/requestpayload_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.goui/lib/utils/validation.ts
✅ Files skipped from review due to trivial changes (5)
- ui/lib/utils/validation.ts
- framework/logstore/asyncjob_test.go
- framework/configstore/rdb_oauth2_test.go
- plugins/governance/store_test.go
- framework/configstore/encryption_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- transports/bifrost-http/handlers/mcpoauth2jwt_test.go
- transports/bifrost-http/handlers/requestpayload_test.go
- tests/cmd/seedvks/main.go
- transports/bifrost-http/handlers/mcpoauth2jwt.go
- framework/configstore/clientconfig.go
- plugins/governance/test_utils.go
- transports/bifrost-http/server/server.go
- framework/configstore/rdb_mcp_sessions_test.go
- transports/bifrost-http/handlers/list_models_vk_test.go
- tests/cmd/seed/seed.go
- transports/bifrost-http/handlers/governance.go
- transports/bifrost-http/handlers/mcpoauth2consent_test.go
- transports/bifrost-http/handlers/mcpserver.go
- transports/bifrost-http/handlers/mcpserver_auth_test.go
- framework/configstore/rdb_deadlock_postgres_test.go
- framework/configstore/tables/virtualkey.go
- framework/configstore/rdb_test.go
- transports/bifrost-http/handlers/governance_test.go
- framework/configstore/tables/encryption_test.go
- framework/configstore/migrations_test.go
- plugins/governance/store.go
- transports/bifrost-http/lib/config_test.go
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 1, 2026 09:24
22dd587 to
21fbc2d
Compare
BearTS
changed the title
TableVirtualKey.Value from string to schemas.SecretVarschemas.SecretVar for virtual key values to support env store
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 1, 2026 11:57
21fbc2d to
992339d
Compare
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 1, 2026 12:20
992339d to
74209a0
Compare
akshaydeo
dismissed
coderabbitai[bot]’s stale review
July 1, 2026 12:24
The merge-base changed after approval.
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 1, 2026 12:57
74209a0 to
f847319
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/governance/store.go`:
- Around line 908-910: Fail closed when two different virtual-key records
resolve to the same plaintext value in the virtual-key storage path. Update the
logic around the Store/GetVirtualKey handling in governance store code so the
resolved value is treated as ambiguous and rejected instead of overwriting the
existing entry in gs.virtualKeys; keep gs.virtualKeysByID consistent, but ensure
duplicate resolved values cannot silently pick a winner. Use the existing
virtual key identifiers and lookup helpers in gs.virtualKeys and
gs.virtualKeysByID to detect collisions and return a security failure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5079150f-f1ac-4e44-969d-04e96f5c7eb8
📒 Files selected for processing (30)
core/schemas/vault.goframework/configstore/clientconfig.goframework/configstore/encryption_test.goframework/configstore/migrations_test.goframework/configstore/rdb_deadlock_postgres_test.goframework/configstore/rdb_mcp_sessions_test.goframework/configstore/rdb_oauth2_test.goframework/configstore/rdb_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/virtualkey.goframework/configstore/vault_callbacks.goframework/configstore/vault_callbacks_test.goframework/logstore/asyncjob_test.goplugins/governance/store.goplugins/governance/store_test.goplugins/governance/test_utils.gotests/cmd/seed/seed.gotests/cmd/seedvks/main.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/bifrost-http/handlers/list_models_vk_test.gotransports/bifrost-http/handlers/mcpoauth2consent_test.gotransports/bifrost-http/handlers/mcpoauth2jwt.gotransports/bifrost-http/handlers/mcpoauth2jwt_test.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/mcpserver_auth_test.gotransports/bifrost-http/handlers/requestpayload_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.go
💤 Files with no reviewable changes (9)
- transports/bifrost-http/handlers/mcpoauth2jwt.go
- transports/bifrost-http/handlers/requestpayload_test.go
- transports/bifrost-http/handlers/mcpoauth2jwt_test.go
- transports/bifrost-http/server/server.go
- transports/bifrost-http/handlers/mcpserver.go
- transports/bifrost-http/handlers/mcpserver_auth_test.go
- transports/bifrost-http/handlers/mcpoauth2consent_test.go
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/lib/config_test.go
✅ Files skipped from review due to trivial changes (3)
- plugins/governance/store_test.go
- framework/configstore/rdb_oauth2_test.go
- transports/bifrost-http/handlers/list_models_vk_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- framework/logstore/asyncjob_test.go
- framework/configstore/vault_callbacks.go
- tests/cmd/seedvks/main.go
- plugins/governance/test_utils.go
- framework/configstore/rdb_mcp_sessions_test.go
- framework/configstore/encryption_test.go
- tests/cmd/seed/seed.go
- framework/configstore/clientconfig.go
- framework/configstore/migrations_test.go
- framework/configstore/tables/encryption_test.go
- framework/configstore/rdb_test.go
- framework/configstore/rdb_deadlock_postgres_test.go
- core/schemas/vault.go
- transports/bifrost-http/handlers/governance.go
- framework/configstore/tables/virtualkey.go
- transports/bifrost-http/handlers/governance_test.go
- framework/configstore/vault_callbacks_test.go
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 1, 2026 14:00
f847319 to
36fe9ca
Compare
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 1, 2026 14:32
36fe9ca to
988a261
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
transports/bifrost-http/lib/config.go (1)
2265-2280: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate resolved VKs consistently and keep fallback generation stable.
Secret-backed values with a non-empty
GetValue()currently bypass thesk-bf-validation that literals get, so an env/vault value likenot-a-vkis accepted. Also, in forced config sync, invalid literal values can still regenerate a fresh fallback on every reload unless the existing stored/generated VK is reused. As per path instructions,governance.virtual_keys[].valueenv/vault refs are resolved to plaintext at load time, so validation should use the resolved plaintext consistently.Also applies to: 2298-2309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2265 - 2280, The virtual key handling in config.go is inconsistent: secret-backed values in the VirtualKeys update path bypass the same sk-bf- prefix check used for literals, and invalid literals can keep regenerating new fallback keys on reload. Update the VirtualKeys logic to validate the resolved plaintext from Value.GetValue() for both secret and literal sources, and when forcing config sync, reuse the existing stored/generated key instead of creating a new one if the current value is invalid. Use the VirtualKeys loop and the existing checks around ShouldPreserveStored, IsFromSecret, GetValue, and GenerateVirtualKey to locate the fix.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/clientconfig.go`:
- Around line 866-868: Update the VK hashing logic in the config store client
config so the hash includes the secret reference identity, not just
vk.Value.GetValue(). In the VK hash block, incorporate the ref/source shape from
the existing value representation (including vault.<path> and
vault.<path>#<jsonKey> for vault refs) together with the resolved plaintext so a
literal value and an env/vault reference resolving to the same string no longer
collide. Keep the change localized to the VK hash generation path and preserve
the current secret-rotation detection behavior while also forcing re-sync when
the stored reference shape changes.
---
Duplicate comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2265-2280: The virtual key handling in config.go is inconsistent:
secret-backed values in the VirtualKeys update path bypass the same sk-bf-
prefix check used for literals, and invalid literals can keep regenerating new
fallback keys on reload. Update the VirtualKeys logic to validate the resolved
plaintext from Value.GetValue() for both secret and literal sources, and when
forcing config sync, reuse the existing stored/generated key instead of creating
a new one if the current value is invalid. Use the VirtualKeys loop and the
existing checks around ShouldPreserveStored, IsFromSecret, GetValue, and
GenerateVirtualKey to locate the fix.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bd45dae-9d8c-4fbf-83d4-0cee7cadc80d
📒 Files selected for processing (31)
core/schemas/vault.goframework/configstore/clientconfig.goframework/configstore/encryption_test.goframework/configstore/migrations_test.goframework/configstore/rdb_deadlock_postgres_test.goframework/configstore/rdb_mcp_sessions_test.goframework/configstore/rdb_oauth2_test.goframework/configstore/rdb_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/virtualkey.goframework/configstore/vault_callbacks.goframework/configstore/vault_callbacks_test.goframework/logstore/asyncjob_test.goplugins/governance/store.goplugins/governance/store_test.goplugins/governance/test_utils.gotests/cmd/seed/seed.gotests/cmd/seedvks/main.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/bifrost-http/handlers/list_models_vk_test.gotransports/bifrost-http/handlers/mcpoauth2consent_test.gotransports/bifrost-http/handlers/mcpoauth2jwt.gotransports/bifrost-http/handlers/mcpoauth2jwt_test.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/mcpserver_auth_test.gotransports/bifrost-http/handlers/requestpayload_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.goui/lib/utils/validation.ts
✅ Files skipped from review due to trivial changes (3)
- plugins/governance/test_utils.go
- transports/bifrost-http/handlers/list_models_vk_test.go
- transports/bifrost-http/handlers/requestpayload_test.go
🚧 Files skipped from review as they are similar to previous changes (26)
- plugins/governance/store_test.go
- framework/configstore/rdb_mcp_sessions_test.go
- tests/cmd/seedvks/main.go
- framework/logstore/asyncjob_test.go
- transports/bifrost-http/handlers/mcpoauth2jwt.go
- framework/configstore/vault_callbacks.go
- transports/bifrost-http/handlers/mcpoauth2jwt_test.go
- transports/bifrost-http/handlers/governance.go
- tests/cmd/seed/seed.go
- framework/configstore/encryption_test.go
- transports/bifrost-http/handlers/mcpserver.go
- framework/configstore/rdb_oauth2_test.go
- transports/bifrost-http/server/server.go
- framework/configstore/vault_callbacks_test.go
- transports/bifrost-http/handlers/mcpoauth2consent_test.go
- framework/configstore/tables/encryption_test.go
- core/schemas/vault.go
- framework/configstore/rdb_deadlock_postgres_test.go
- framework/configstore/migrations_test.go
- ui/lib/utils/validation.ts
- plugins/governance/store.go
- transports/bifrost-http/handlers/mcpserver_auth_test.go
- transports/bifrost-http/handlers/governance_test.go
- framework/configstore/rdb_test.go
- framework/configstore/tables/virtualkey.go
- transports/bifrost-http/lib/config_test.go
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 1, 2026 15:32
988a261 to
aaf6710
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
transports/bifrost-http/lib/config.go (1)
2274-2276: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the existing VK before generating on forced sync.
With
source_of_truth=config.json,forceFileSyncre-enters this branch on every reload. If the file still has an invalid explicit value, Line 2276 generates a fresh VK each time and breaks clients using the previously generated key. Prefer the existing stored value when it is already valid; only generate when neither side has a valid prefixed value.As per path instructions,
transports/config.schema.jsonis the source of truth for config fields, and VKvalueremains user-provided config input that resolves to plaintext.Proposed fix
if !strings.HasPrefix(resolvedVal, governance.VirtualKeyPrefix) { - logger.Warn("virtual key %s has a value in the config file that does not have %s prefix. We are generating a new one for you.", newVirtualKey.ID, governance.VirtualKeyPrefix) - configData.Governance.VirtualKeys[i].Value = *schemas.NewSecretVar(governance.GenerateVirtualKey()) + existingResolvedVal := existingVirtualKey.Value.GetValue() + if strings.HasPrefix(existingResolvedVal, governance.VirtualKeyPrefix) { + logger.Warn("virtual key %s has a value in the config file that does not have %s prefix. Preserving the existing stored value.", existingVirtualKey.ID, governance.VirtualKeyPrefix) + configData.Governance.VirtualKeys[i].Value = existingVirtualKey.Value + } else { + logger.Warn("virtual key %s has a value in the config file that does not have %s prefix. We are generating a new one for you.", existingVirtualKey.ID, governance.VirtualKeyPrefix) + configData.Governance.VirtualKeys[i].Value = *schemas.NewSecretVar(governance.GenerateVirtualKey()) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 2274 - 2276, The forced-sync path in the config handling is regenerating a new virtual key every reload when the existing stored VK value is invalid, which breaks clients that already learned the previous key. Update the branch that checks the resolved VK value in the governance VirtualKeys loop so it first preserves and reuses any already-generated valid secret (for example, the existing config value or the current secret var) and only calls governance.GenerateVirtualKey() when neither the existing value nor the stored source value has the governance.VirtualKeyPrefix. Keep the logger.Warn in this branch, but ensure configData.Governance.VirtualKeys[i].Value is only replaced when regeneration is truly necessary.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2274-2276: The forced-sync path in the config handling is
regenerating a new virtual key every reload when the existing stored VK value is
invalid, which breaks clients that already learned the previous key. Update the
branch that checks the resolved VK value in the governance VirtualKeys loop so
it first preserves and reuses any already-generated valid secret (for example,
the existing config value or the current secret var) and only calls
governance.GenerateVirtualKey() when neither the existing value nor the stored
source value has the governance.VirtualKeyPrefix. Keep the logger.Warn in this
branch, but ensure configData.Governance.VirtualKeys[i].Value is only replaced
when regeneration is truly necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 24775b60-470e-469f-b4f3-c2d4026bf354
📒 Files selected for processing (31)
core/schemas/vault.goframework/configstore/clientconfig.goframework/configstore/encryption_test.goframework/configstore/migrations_test.goframework/configstore/rdb_deadlock_postgres_test.goframework/configstore/rdb_mcp_sessions_test.goframework/configstore/rdb_oauth2_test.goframework/configstore/rdb_test.goframework/configstore/tables/encryption_test.goframework/configstore/tables/virtualkey.goframework/configstore/vault_callbacks.goframework/configstore/vault_callbacks_test.goframework/logstore/asyncjob_test.goplugins/governance/store.goplugins/governance/store_test.goplugins/governance/test_utils.gotests/cmd/seed/seed.gotests/cmd/seedvks/main.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/bifrost-http/handlers/list_models_vk_test.gotransports/bifrost-http/handlers/mcpoauth2consent_test.gotransports/bifrost-http/handlers/mcpoauth2jwt.gotransports/bifrost-http/handlers/mcpoauth2jwt_test.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/mcpserver_auth_test.gotransports/bifrost-http/handlers/requestpayload_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.goui/lib/utils/validation.ts
✅ Files skipped from review due to trivial changes (3)
- transports/bifrost-http/handlers/list_models_vk_test.go
- transports/bifrost-http/handlers/requestpayload_test.go
- framework/configstore/encryption_test.go
🚧 Files skipped from review as they are similar to previous changes (27)
- transports/bifrost-http/handlers/mcpoauth2jwt_test.go
- plugins/governance/store_test.go
- framework/configstore/rdb_mcp_sessions_test.go
- tests/cmd/seed/seed.go
- tests/cmd/seedvks/main.go
- framework/configstore/clientconfig.go
- plugins/governance/test_utils.go
- framework/configstore/vault_callbacks.go
- transports/bifrost-http/handlers/governance.go
- ui/lib/utils/validation.ts
- framework/configstore/rdb_deadlock_postgres_test.go
- transports/bifrost-http/handlers/mcpoauth2jwt.go
- framework/logstore/asyncjob_test.go
- framework/configstore/rdb_oauth2_test.go
- framework/configstore/vault_callbacks_test.go
- transports/bifrost-http/server/server.go
- framework/configstore/rdb_test.go
- plugins/governance/store.go
- framework/configstore/tables/encryption_test.go
- framework/configstore/migrations_test.go
- transports/bifrost-http/handlers/mcpoauth2consent_test.go
- transports/bifrost-http/handlers/mcpserver_auth_test.go
- core/schemas/vault.go
- transports/bifrost-http/handlers/mcpserver.go
- transports/bifrost-http/handlers/governance_test.go
- framework/configstore/tables/virtualkey.go
- transports/bifrost-http/lib/config_test.go
BearTS
force-pushed
the
07-01-feat_add_secretvar_support_to_vk
branch
from
July 2, 2026 04:53
aaf6710 to
cde46bf
Compare
akshaydeo
approved these changes
Jul 2, 2026
Contributor
Merge activity
|
yangtuooc
added a commit
to yangtuooc/bifrost
that referenced
this pull request
Jul 3, 2026
* upstream/dev: fix: honor model_parameters_url changes in config.json like pricing_url (maximhq#4864) feat: use `schemas.SecretVar` for virtual key values to support env store (maximhq#4817)
This was referenced Jul 3, 2026
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
TableVirtualKey.Valuewas a plainstring, which meant the raw secret was passed around in-memory with no type-level distinction between a resolved plaintext value, an environment-variable reference, or a future vault reference. This PR changes the field type toschemas.SecretVar, a wrapper that carries the value's origin (plain text, env var, vault ref) and exposes it only throughGetValue(). All call sites are updated to construct values viaschemas.NewSecretVar(...)and read them via.GetValue().Changes
TableVirtualKey.Valuechanged fromstringtoschemas.SecretVaracross the model definition, GORM hooks (BeforeSave,AfterFind), and all consumers.BeforeSavenow callsencryptSecretVar/decryptSecretVarhelpers instead of operating on a raw string, and integrates with the vault write path viaStoreOwnedVaultSecretVarswhen vault storage is enabled.MarshalJSONis added toTableVirtualKeyso REST API responses continue to emitvalueas a plain string rather than aSecretVarobject.VaultPathKeyandVaultStoreSelfManagedare implemented onTableVirtualKeyso the global vault callback skips it andBeforeSavemanages vault writes directly.mergeGovernanceConfigis simplified: env-var and vault references are resolved automatically bySecretVar, removing the explicitenv.prefix handling and the separateenvutils.ProcessEnvValuecalls. Plain-text values still enforce the virtual-key prefix or trigger auto-generation.GenerateVirtualKeyHashupdated to hashvk.Value.GetValue()instead of the raw field.rebuildInMemoryStructures,CreateVirtualKeyInMemory,UpdateVirtualKeyInMemory) updated to key the sync map on.GetValue()..GetValue()when the string value is needed.TableVirtualKeywith*schemas.NewSecretVar(...)and assert against.GetValue().Type of change
Affected areas
How to test
Verify that:
"value": "sk-bf-..."(plain string) in the response body.env.VAR_NAMEvalues resolve correctly without the explicit prefix-stripping logic.sk-bf-prefix and the old MCP server entry is evicted.Breaking changes
Any code outside this repository that directly assigns or reads
TableVirtualKey.Valueas astringmust be updated to use*schemas.NewSecretVar(value)for writes and.GetValue()for reads. JSON serialization ofTableVirtualKeyis unchanged —valueis still emitted as a plain string.Security considerations
The
SecretVarwrapper makes the origin of a secret explicit at the type level, reducing the risk of accidentally treating an unresolved env-var reference as a usable key value. Vault write integration inBeforeSaveensures that when vault storage is enabled, the plaintext is written to vault before encryption, keeping the database free of unprotected secrets.Checklist
docs/contributing/README.mdand followed the guidelines