feat: add global GORM vault callbacks with VaultPathKeyer interface and map[string]EnvVar support, replacing per-model BeforeSave/AfterDelete vault hooks - #4404
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughVault secret persistence is centralized from per-table GORM hooks ( ChangesGlobal Vault Callback Refactor
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Config as AddProviderKey/UpdateProviderKey
participant Store as ConfigStore (Postgres)
participant VaultCB as RegisterVaultCallbacks (before create/update)
participant VaultStore as VaultStoreHook
participant Memory as In-Memory Key Cache
Client->>Config: AddProviderKey(key with plaintext secret)
Config->>Store: CreateProviderKey(key)
Store->>VaultCB: before create trigger
VaultCB->>VaultCB: forEachModel → VaultPathKeyer check
VaultCB->>VaultStore: store plaintext at bifrost/<table>/<VaultPathKey()>/field
VaultStore-->>VaultCB: vault.<path> ref
VaultCB->>Store: rewrite SecretVar fields to vault refs in DB row
Store-->>Config: persisted row
Config->>Store: GetProviderKey(id)
Store-->>Config: vault-rewritten key
Config->>Memory: replace in-memory entry with vault-rewritten key
Memory-->>Client: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ 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 |
15 tasks
Contributor
Author
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
2 times, most recently
from
June 15, 2026 13:38
55e944d to
d409fc5
Compare
BearTS
force-pushed
the
06-15-feat_extend_envvar_for_vault_support
branch
from
June 15, 2026 20:20
fd9708d to
c91216f
Compare
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 15, 2026 20:20
d409fc5 to
32dd80b
Compare
BearTS
force-pushed
the
06-15-feat_extend_envvar_for_vault_support
branch
from
June 15, 2026 20:32
c91216f to
a2e21fd
Compare
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 15, 2026 20:32
32dd80b to
38d9e04
Compare
BearTS
force-pushed
the
06-15-feat_extend_envvar_for_vault_support
branch
from
June 15, 2026 20:45
a2e21fd to
dbee1f1
Compare
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 15, 2026 20:45
38d9e04 to
ad24adf
Compare
18 tasks
BearTS
changed the title
VaultPathKeyer interface and map[string]EnvVar support, replacing per-model BeforeSave/AfterDelete vault hooks
BearTS
changed the base branch from
06-15-feat_extend_envvar_for_vault_support
to
graphite-base/4404
June 16, 2026 13:48
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 16, 2026 13:48
ad24adf to
a5d2d54
Compare
BearTS
force-pushed
the
graphite-base/4404
branch
from
June 16, 2026 13:48
dbee1f1 to
574c6a5
Compare
BearTS
changed the base branch from
graphite-base/4404
to
convert_envvar_to_secretVar
June 16, 2026 13:48
BearTS
marked this pull request as ready for review
June 16, 2026 13:51
![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: 10
🧹 Nitpick comments (3)
core/schemas/vault_test.go (1)
147-147: 💤 Low valueConsider renaming test functions for consistency.
Once the function calls are fixed to use
*SecretVars, the test function names (TestStoreOwnedVaultEnvVars_WalksMap,TestRemoveOwnedVaultEnvVars_WalksMap) will be misleading. Consider renaming toTestStoreOwnedVaultSecretVars_WalksEnvVarMapandTestRemoveOwnedVaultSecretVars_WalksEnvVarMapto accurately describe that they test the*SecretVarsfunctions' handling ofmap[string]EnvVarfields.Also applies to: 179-179
🤖 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 `@core/schemas/vault_test.go` at line 147, The test function names are misleading after the function calls are updated to use *SecretVars. Rename TestStoreOwnedVaultEnvVars_WalksMap at line 147 to TestStoreOwnedVaultSecretVars_WalksEnvVarMap to accurately reflect that it tests the *SecretVars function's handling of map[string]EnvVar fields. Similarly, rename TestRemoveOwnedVaultEnvVars_WalksMap at line 179 to TestRemoveOwnedVaultSecretVars_WalksEnvVarMap for consistency. These new names will better describe what the tests actually do.transports/bifrost-http/lib/config.go (1)
5645-5649: ⚡ Quick winSurface read-after-write refetch failures instead of silently swallowing them.
If
GetProviderKeyfails, these paths quietly keep the pre-write in-memory key, which makes vault-ref propagation drift hard to detect and debug. Please at least log a warning on failure in both methods.♻️ Suggested patch
- if storedKey, err := c.ConfigStore.GetProviderKey(ctx, provider, key.ID); err == nil { + if storedKey, err := c.ConfigStore.GetProviderKey(ctx, provider, key.ID); err == nil { if idx := slices.IndexFunc(updatedConfig.Keys, func(k schemas.Key) bool { return k.ID == key.ID }); idx != -1 { updatedConfig.Keys[idx] = *storedKey } + } else { + logger.Warn("failed to re-read provider key %s for provider %s after create: %v", key.ID, provider, err) }- if storedKey, err := c.ConfigStore.GetProviderKey(ctx, provider, keyID); err == nil { + if storedKey, err := c.ConfigStore.GetProviderKey(ctx, provider, keyID); err == nil { updatedConfig.Keys[index] = *storedKey + } else { + logger.Warn("failed to re-read provider key %s for provider %s after update: %v", keyID, provider, err) }Also applies to: 5708-5710
🤖 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 5645 - 5649, When the GetProviderKey call fails with an error, the code currently silently ignores it and retains the pre-write in-memory key, making vault-ref propagation drift hard to detect. Add a warning log statement in the error path (when err != nil) after the GetProviderKey call to surface these failures. This same fix should be applied at all locations where GetProviderKey is called with this read-after-write pattern to ensure failures are consistently logged across the codebase.helm-charts/bifrost/templates/_helpers.tpl (1)
728-749: 💤 Low valueConsider adding validation for vault store configuration.
Other features in this file (e.g., plugins, cluster config, MCP, vector store) have corresponding validation in the
bifrost.validatetemplate (lines 1545-2037). The vault store configuration has no validation, so invalid configurations (e.g.,enabled: truewith missingtype, ortype: aws-secrets-managerwithout required AWS credentials) will produce runtime errors instead of clear Helm install failures.🤖 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 `@helm-charts/bifrost/templates/_helpers.tpl` around lines 728 - 749, Add validation for vault store configuration in the bifrost.validate template to match the validation pattern used for other features. Create validation checks for the vault store block that ensure when vaultStore is enabled, the required type field is provided, and based on the selected type (aws-secrets-manager, gcp-secret-manager, or hashicorp), the corresponding cloud-specific credentials (aws, gcp, or hashicorp) are present. This will catch invalid configurations at Helm install time rather than allowing them to produce runtime errors.
🤖 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 `@core/schemas/vault_test.go`:
- Line 198: The test is calling a function named RemoveOwnedVaultEnvVars that
does not exist in vault.go. Change the function name in the test call from
RemoveOwnedVaultEnvVars to RemoveOwnedVaultSecretVars to match the actual
function name defined in vault.go.
- Around line 160-162: The test is calling StoreOwnedVaultEnvVars which does not
exist in vault.go. Replace this undefined function call with
StoreOwnedVaultSecretVars, which is the actual function defined in vault.go and
already handles the map[string]EnvVar fields as intended by this test case.
In `@core/schemas/vault.go`:
- Around line 178-182: The function StoreVaultEnvVar is called on line 179 but
it is not defined in the code, only StoreVaultSecretVar exists. Add a new
function StoreVaultEnvVar that mirrors the implementation and signature of the
existing StoreVaultSecretVar function but operates on *EnvVar type instead of
*SecretVar. This function should follow the same pattern as StoreVaultSecretVar
including the nil checks, condition guards (IsFromEnv, IsFromVault, empty value,
IsRedacted), vault store hook invocation, and field updates (setting VaultRef
and FromVault flag).
- Around line 73-77: The reflect.TypeOf declarations for envVarType,
envVarPtrType, and envVarMapType are referencing an undefined EnvVar type.
Verify whether EnvVar is defined elsewhere in the schemas package (it should
have methods like GetValue() and IsSet() based on design learnings), and either
add the necessary import statement to bring it into scope in vault.go, or ensure
that EnvVar is properly defined in this file if it's meant to be part of this
PR. Check your dependencies and PR scope to confirm whether a related PR
containing the EnvVar definition needs to be merged first.
In `@framework/configstore/postgres.go`:
- Around line 24-32: The vault callbacks are registered only for the connection
opened in openPostresConnection, but the runtime and refreshed pools opened via
postgresconn.Open in newPostgresConfigStore are not registering these callbacks.
Locate the lines in newPostgresConfigStore where the runtime pool and refreshed
pool are opened (around lines 109-110 and 140-141) and call
RegisterVaultCallbacks on each of those DB instances immediately after they are
successfully opened, ensuring all Postgres-backed config store connections have
vault store/remove callbacks properly wired in.
- Line 25: The postgres.New and postgres.Config references at line 25 require
the gorm.io/driver/postgres package to be imported, but this import is missing
from the file. Add the import statement for gorm.io/driver/postgres at the top
of the file with the other imports to resolve the compilation error.
In `@framework/configstore/vault_callbacks.go`:
- Around line 15-16: The vault callback registration for `bifrost:vault_store`
is running before GORM's model hooks, which means it executes before the
`TableKey.BeforeSave` method has a chance to populate the provider config
secrets (like AzureKeyConfig, VertexKeyConfig, BedrockKeyConfig, VLLMKeyConfig,
OllamaKeyConfig, and SGLKeyConfig) into the SecretVar columns. This causes the
secrets to bypass vault storage. Change the callback registration to use
`After("gorm:before_create")` and `After("gorm:before_update")` instead of
`Before` to ensure the vault callback runs after the model hooks have populated
the SecretVar fields with the actual secret values.
In `@helm-charts/bifrost/templates/_helpers.tpl`:
- Around line 738-746: The aws, gcp, and hashicorp backend configurations are
being passed directly to the vaultStore without transforming their field names
from camelCase (Helm convention) to snake_case (config schema expectation). This
creates inconsistency with how other credential configs are handled in the
template (such as S3 object storage which explicitly maps camelCase keys to
snake_case). Either verify that values.schema.json defines these backend
credentials with snake_case keys matching the config schema expectations, or
implement explicit field-by-field mapping for the aws, gcp, and hashicorp
objects to transform their camelCase property names (e.g., accessKeyId,
secretAccessKey) to the required snake_case names (e.g., access_key_id,
secret_access_key) before assigning them to the vaultStore.
In `@helm-charts/bifrost/values.schema.json`:
- Around line 3340-3367: The vaultStore schema definition is too permissive and
allows invalid configurations to pass validation. Tighten the vaultStore object
by: (1) adding additionalProperties set to false to disallow unknown fields, (2)
adding proper schema definitions for the backend objects aws, gcp, and hashicorp
instead of leaving them as empty unconstrained objects, (3) adding conditional
validation requirements such that when type is set to a specific backend type or
when enabled is true, the corresponding backend configuration is required, and
(4) ensure the schema structure and constraints align with the source of truth
defined in transports/config.schema.json. This will prevent invalid Helm values
from passing chart validation and failing later at runtime.
In `@transports/config.schema.json`:
- Around line 1124-1183: The vault_store block is currently nested as a property
under config_store, but according to the schema contract it should be a
top-level property at the root level. Move the entire vault_store object
(including all its properties: enabled, type, prefix, access_mode, aws, gcp, and
hashicorp) out of the config_store properties and place it as a sibling to
config_store in the root properties section. This ensures valid configs using
top-level vault_store will pass schema validation, while keeping config_store
scoped to database settings only.
---
Nitpick comments:
In `@core/schemas/vault_test.go`:
- Line 147: The test function names are misleading after the function calls are
updated to use *SecretVars. Rename TestStoreOwnedVaultEnvVars_WalksMap at line
147 to TestStoreOwnedVaultSecretVars_WalksEnvVarMap to accurately reflect that
it tests the *SecretVars function's handling of map[string]EnvVar fields.
Similarly, rename TestRemoveOwnedVaultEnvVars_WalksMap at line 179 to
TestRemoveOwnedVaultSecretVars_WalksEnvVarMap for consistency. These new names
will better describe what the tests actually do.
In `@helm-charts/bifrost/templates/_helpers.tpl`:
- Around line 728-749: Add validation for vault store configuration in the
bifrost.validate template to match the validation pattern used for other
features. Create validation checks for the vault store block that ensure when
vaultStore is enabled, the required type field is provided, and based on the
selected type (aws-secrets-manager, gcp-secret-manager, or hashicorp), the
corresponding cloud-specific credentials (aws, gcp, or hashicorp) are present.
This will catch invalid configurations at Helm install time rather than allowing
them to produce runtime errors.
In `@transports/bifrost-http/lib/config.go`:
- Around line 5645-5649: When the GetProviderKey call fails with an error, the
code currently silently ignores it and retains the pre-write in-memory key,
making vault-ref propagation drift hard to detect. Add a warning log statement
in the error path (when err != nil) after the GetProviderKey call to surface
these failures. This same fix should be applied at all locations where
GetProviderKey is called with this read-after-write pattern to ensure failures
are consistently logged across the codebase.
🪄 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: 9753aa3d-8ba9-4a58-a2d6-4a69e63ec88d
📒 Files selected for processing (12)
core/schemas/vault.gocore/schemas/vault_test.goframework/configstore/postgres.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goframework/configstore/vault_callbacks.goframework/configstore/vault_callbacks_test.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsontransports/bifrost-http/lib/config.gotransports/config.schema.json
Contributor
Confidence Score: 4/5Safe to merge with one issue to verify: AddProviderKey/UpdateProviderKey return an error after a successful DB+vault write when the re-read fails, which can leave the created key permanently unreachable via the create path. The global callback wiring, VaultStoreSelfManaged guard, map[string]SecretVar store/remove symmetry, and per-pool registration are all correct. The one concrete defect is in config.go: a transient re-read failure after a successful CreateProviderKey surfaces as a write error to the caller. On retry the caller hits ErrAlreadyExists, with no built-in recovery path other than directly calling GetProviderKey. This issue is isolated to vault-enabled enterprise deployments and the uncommon transient-DB-error scenario, but it is a real breakage when it occurs. transports/bifrost-http/lib/config.go — the AddProviderKey re-read error path (around line 5648) and the matching UpdateProviderKey path (around line 5720). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant Config
participant GORM
participant VaultCallback as bifrost:vault_store callback
participant BeforeSave
participant DB
participant Vault
Caller->>Config: AddProviderKey(key)
Config->>GORM: "db.Create(&tableKey)"
rect rgb(230, 240, 255)
note over GORM,Vault: VaultPathKeyer models (MCP, OAuth)
GORM->>VaultCallback: Before gorm:before_create
VaultCallback->>Vault: StoreVaultSecretVar(path, plaintext)
Vault-->>VaultCallback: VaultRef written back to field
end
rect rgb(255, 240, 230)
note over GORM,DB: TableKey (VaultStoreSelfManaged)
GORM->>BeforeSave: gorm:before_create → BeforeSave
BeforeSave->>Vault: StoreOwnedVaultSecretVars
BeforeSave->>BeforeSave: encrypt remaining plaintext fields
end
GORM->>DB: INSERT row (vault refs, not plaintext)
DB-->>GORM: success
rect rgb(230, 255, 230)
note over Config,DB: Re-read so in-memory config carries vault ref
Config->>DB: GetProviderKey (re-read)
DB-->>Config: row with vault refs
Config->>Config: "updatedConfig.Keys[idx] = storedKey"
end
Config->>Config: "c.Providers[provider] = updatedConfig"
note over Caller,DB: On delete: bifrost:vault_remove fires after gorm:after_delete
%%{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 Caller
participant Config
participant GORM
participant VaultCallback as bifrost:vault_store callback
participant BeforeSave
participant DB
participant Vault
Caller->>Config: AddProviderKey(key)
Config->>GORM: "db.Create(&tableKey)"
rect rgb(230, 240, 255)
note over GORM,Vault: VaultPathKeyer models (MCP, OAuth)
GORM->>VaultCallback: Before gorm:before_create
VaultCallback->>Vault: StoreVaultSecretVar(path, plaintext)
Vault-->>VaultCallback: VaultRef written back to field
end
rect rgb(255, 240, 230)
note over GORM,DB: TableKey (VaultStoreSelfManaged)
GORM->>BeforeSave: gorm:before_create → BeforeSave
BeforeSave->>Vault: StoreOwnedVaultSecretVars
BeforeSave->>BeforeSave: encrypt remaining plaintext fields
end
GORM->>DB: INSERT row (vault refs, not plaintext)
DB-->>GORM: success
rect rgb(230, 255, 230)
note over Config,DB: Re-read so in-memory config carries vault ref
Config->>DB: GetProviderKey (re-read)
DB-->>Config: row with vault refs
Config->>Config: "updatedConfig.Keys[idx] = storedKey"
end
Config->>Config: "c.Providers[provider] = updatedConfig"
note over Caller,DB: On delete: bifrost:vault_remove fires after gorm:after_delete
Reviews (12): Last reviewed commit: "chore: update config schema and helm" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
BearTS
force-pushed
the
convert_envvar_to_secretVar
branch
from
June 17, 2026 05:06
574c6a5 to
b1e4bd8
Compare
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 17, 2026 05:06
a5d2d54 to
2892d85
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/schemas/vault.go`:
- Around line 31-36: The documentation comment for the VaultStoreWriteEnabled
function contains a duplicate/incomplete sentence fragment at the end. In the
comment block for VaultStoreWriteEnabled, remove or complete the trailing phrase
"since those calls in BeforeSave hooks." which appears to be a copy-paste error,
ensuring the comment ends with a complete, grammatically correct sentence that
properly explains the purpose and usage of the function.
In `@helm-charts/bifrost/values.schema.json`:
- Around line 3458-3513: The credential fields under aws, gcp, and hashicorp
objects in the vaultStore configuration (such as region, accessKeyId,
secretAccessKey, sessionToken, roleArn, kmsKeyId under aws; projectId and
credentialsJson under gcp; and address, token, namespace, mountPath, roleId,
secretId under hashicorp) are currently restricted to type string only. Update
each of these credential fields to accept both string values and EnvVar object
shapes (with canonical properties value, env_var, and from_env) by using a oneOf
or anyOf schema pattern that allows either a string type or an object type
matching the EnvVar shape, applying this pattern consistently across all three
backend credential objects.
🪄 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: 777b3ac0-1383-4a4f-9ced-33d46d2f8350
📒 Files selected for processing (12)
core/schemas/vault.gocore/schemas/vault_test.goframework/configstore/postgres.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goframework/configstore/vault_callbacks.goframework/configstore/vault_callbacks_test.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsontransports/bifrost-http/lib/config.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (9)
- core/schemas/vault_test.go
- framework/configstore/tables/mcp.go
- transports/bifrost-http/lib/config.go
- framework/configstore/tables/oauth.go
- framework/configstore/postgres.go
- transports/config.schema.json
- framework/configstore/vault_callbacks.go
- framework/configstore/tables/key.go
- framework/configstore/vault_callbacks_test.go
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 17, 2026 05:45
2892d85 to
e37b214
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
framework/configstore/vault_callbacks_test.go (1)
88-122: ⚡ Quick winTest does not validate vault+encryption interaction.
Line 93 disables encryption by calling
encrypt.Init(""), which means this test does not exercise the scenario described invault_callbacks.go:34-36where the after-phase callback observes ciphertext when encryption is enabled.If encryption + vault are both enabled in production, and the after-phase callback stores ciphertext instead of plaintext (as the comment suggests), this test would not catch that behavior.
Add a test case that:
- Enables encryption via
encrypt.Init(testKey, ...)- Creates a
TableKeywithBedrockKeyConfig- Asserts the vault stores plaintext (not ciphertext)
This will validate whether the encryption+vault interaction is correct or if the vault callback needs to run before encryption.
🤖 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 `@framework/configstore/vault_callbacks_test.go` around lines 88 - 122, The current TestVaultCallbacks_AfterPhaseStoresFlatColumns test disables encryption by calling encrypt.Init(""), which means it does not validate the vault and encryption interaction described in vault_callbacks.go. Add a new test case (e.g., TestVaultCallbacks_AfterPhaseWithEncryption) that enables encryption by calling encrypt.Init with a test key instead of an empty string, creates a TableKey with BedrockKeyConfig (similar to the existing test), and then asserts that the vault stores the plaintext secret value (not ciphertext) to verify that the after-phase callback correctly handles the encryption scenario and stores the original value to vault before it gets encrypted.
🤖 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/vault_callbacks.go`:
- Around line 30-36: The vault callback mechanism is storing encrypted
ciphertext to the vault instead of plaintext secrets because the after-phase
callback (registered at Before("gorm:create") and Before("gorm:update")) runs
after TableKey.BeforeSave has already encrypted the SecretVar columns. To fix
this, either modify TableKey.BeforeSave to skip encryption for vault-owned
fields (allowing the vault callback to handle them before encryption occurs), or
reorder the callbacks so the vault callback's StoreOwnedVaultSecretVars runs
before the encryption phase in BeforeSave. Update the comment block accordingly
to reflect the chosen approach and remove any statements labeling this as
"accepted behavior" unless the fix is implemented and documented with proper
justification.
---
Nitpick comments:
In `@framework/configstore/vault_callbacks_test.go`:
- Around line 88-122: The current TestVaultCallbacks_AfterPhaseStoresFlatColumns
test disables encryption by calling encrypt.Init(""), which means it does not
validate the vault and encryption interaction described in vault_callbacks.go.
Add a new test case (e.g., TestVaultCallbacks_AfterPhaseWithEncryption) that
enables encryption by calling encrypt.Init with a test key instead of an empty
string, creates a TableKey with BedrockKeyConfig (similar to the existing test),
and then asserts that the vault stores the plaintext secret value (not
ciphertext) to verify that the after-phase callback correctly handles the
encryption scenario and stores the original value to vault before it gets
encrypted.
🪄 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: db82a3ea-8a1d-42ca-81d7-d6c84dc4a2cc
📒 Files selected for processing (12)
core/schemas/vault.gocore/schemas/vault_test.goframework/configstore/postgres.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goframework/configstore/vault_callbacks.goframework/configstore/vault_callbacks_test.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsontransports/bifrost-http/lib/config.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (7)
- transports/bifrost-http/lib/config.go
- framework/configstore/tables/mcp.go
- framework/configstore/tables/oauth.go
- transports/config.schema.json
- framework/configstore/tables/key.go
- core/schemas/vault.go
- framework/configstore/postgres.go
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 17, 2026 08:15
5c3f63f to
e0125f8
Compare
BearTS
force-pushed
the
convert_envvar_to_secretVar
branch
from
June 17, 2026 08:15
c6f7153 to
360965a
Compare
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 17, 2026 08:43
e0125f8 to
ccadc7c
Compare
Merged
18 tasks
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 21, 2026 19:36
ccadc7c to
46072a4
Compare
BearTS
force-pushed
the
convert_envvar_to_secretVar
branch
from
June 21, 2026 19:36
360965a to
40c3ff1
Compare
This was referenced Jun 21, 2026
Closed
BearTS
force-pushed
the
convert_envvar_to_secretVar
branch
from
June 22, 2026 13:18
40c3ff1 to
598f9d6
Compare
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 22, 2026 13:18
46072a4 to
58fc774
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
transports/bifrost-http/lib/config.go (1)
6329-6337: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider preserving the env var reference for auto-detected keys.
Using
schemas.NewSecretVar(apiKey)stores the resolved value but loses the provenance information. Using"env." + envVarinstead would preserve that the key originated fromOPENAI_API_KEY(etc.), improving UX by showing the source in the UI.♻️ Optional improvement
{ ID: keyID, Name: fmt.Sprintf("%s_auto_detected", envVar), - Value: *schemas.NewSecretVar(apiKey), + Value: *schemas.NewSecretVar("env." + envVar), Models: schemas.WhiteList{"*"}, Weight: 1.0, },🤖 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 6329 - 6337, The current implementation in the Key configuration stores the resolved API key value directly using schemas.NewSecretVar(apiKey), which loses the original source information. Instead, preserve the environment variable reference by passing a string reference like "env." concatenated with the envVar variable to schemas.NewSecretVar(), so that the key provenance (that it originated from OPENAI_API_KEY, etc.) is maintained and can be displayed in the UI.
🤖 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.
Nitpick comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 6329-6337: The current implementation in the Key configuration
stores the resolved API key value directly using schemas.NewSecretVar(apiKey),
which loses the original source information. Instead, preserve the environment
variable reference by passing a string reference like "env." concatenated with
the envVar variable to schemas.NewSecretVar(), so that the key provenance (that
it originated from OPENAI_API_KEY, etc.) is maintained and can be displayed in
the UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bf67f58-f5c7-4047-8176-54b3c075cd35
📒 Files selected for processing (12)
core/schemas/vault.gocore/schemas/vault_test.goframework/configstore/postgres.goframework/configstore/tables/key.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goframework/configstore/vault_callbacks.goframework/configstore/vault_callbacks_test.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsontransports/bifrost-http/lib/config.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (11)
- helm-charts/bifrost/templates/_helpers.tpl
- transports/config.schema.json
- framework/configstore/postgres.go
- framework/configstore/vault_callbacks_test.go
- framework/configstore/tables/key.go
- helm-charts/bifrost/values.schema.json
- framework/configstore/vault_callbacks.go
- framework/configstore/tables/oauth.go
- framework/configstore/tables/mcp.go
- core/schemas/vault_test.go
- core/schemas/vault.go
Contributor
Merge activity
|
akshaydeo
changed the base branch from
convert_envvar_to_secretVar
to
graphite-base/4404
June 22, 2026 16:30
akshaydeo
dismissed
coderabbitai[bot]’s stale review
June 22, 2026 16:33
The base branch was changed.
akshaydeo
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 22, 2026 16:33
58fc774 to
2770bfc
Compare
Comment on lines
+5648
to
+5658
| storedKey, err := c.ConfigStore.GetProviderKey(ctx, provider, key.ID) | ||
| if err != nil { | ||
| // The DB write succeeded but we could not re-read the vault-rewritten | ||
| // key. Failing here avoids committing the original plaintext into | ||
| // c.Providers (and serving it via the keys API) on vault deployments. | ||
| logger.Error("failed to re-read stored key %s for provider %s after create: %v", key.ID, provider, err) | ||
| return fmt.Errorf("failed to re-read provider key after create: %w", err) | ||
| } | ||
| if idx := slices.IndexFunc(updatedConfig.Keys, func(k schemas.Key) bool { return k.ID == key.ID }); idx != -1 { | ||
| updatedConfig.Keys[idx] = *storedKey | ||
| } |
Contributor
There was a problem hiding this comment.
CreateProviderKey succeeds (the DB row exists, vault secret is written), but if the subsequent GetProviderKey call fails (transient DB error, connection blip), the function returns an error. The caller treats the entire operation as failed and may retry — but CreateProviderKey now returns ErrAlreadyExists on retry. There is no recovery path: the key sits in the DB with vault refs, but the caller can never successfully complete the "create" path and the key never lands in c.Providers. The user would have to call UpdateProviderKey or GetProviderKey manually to recover, which is not obvious from the error message.
Handling options: (1) return nil but log a warning that in-memory state may lag DB until the next GetProviderKey, (2) return a dedicated sentinel error that signals "DB succeeded, re-read failed — call GetProviderKey to refresh", or (3) issue the GetProviderKey with retries before returning an error. The same pattern applies to UpdateProviderKey at line 5720–5728, where a re-read failure also surfaces as a write-level error to the caller.
akshaydeo
pushed a commit
that referenced
this pull request
Jun 24, 2026
… and `map[string]EnvVar` support, replacing per-model `BeforeSave`/`AfterDelete` vault hooks (#4404) ## Summary This PR centralises vault secret management into a single pair of global GORM callbacks (`bifrost:vault_store` and `bifrost:vault_remove`), removing the duplicated per-model `BeforeSave`/`AfterDelete` vault logic from `TableKey`, `TableMCPClient`, and `TableOauthConfig`. Models opt in by implementing the new `VaultPathKeyer` interface. It also extends vault support to `map[string]EnvVar` fields (e.g. MCP `Headers`), which previously were not walked by the store/remove helpers. ## Changes - **`VaultPathKeyer` interface** added to `core/schemas/vault.go`. Models that implement `VaultPathKey() string` are automatically handled by the global callbacks without any per-model wiring. - **`VaultStoreEnabled()` renamed to `VaultStoreWriteEnabled()`** and now requires both `VaultStoreHook` and `VaultRemoveHook` to be non-nil before write operations are attempted. - **`map[string]EnvVar` support** added to both `StoreOwnedVaultEnvVars` and `RemoveOwnedVaultEnvVars`. Each map entry is stored at `basePath/<column>/<mapKey>`. Fragment refs (`#key`) pointing at externally-managed shared secrets are never auto-deleted. - **`removeOwnedVaultEnvVar`** extracted as a private helper to deduplicate the single-field removal logic used by both the struct-field and map-entry paths. - **`RegisterVaultCallbacks(db)`** introduced in `framework/configstore/vault_callbacks.go`. It registers `vaultStoreCallback` (before create/update) and `vaultRemoveCallback` (after delete) on any `*gorm.DB`. Called from `openPostresConnection` so every pool gets the callbacks automatically. - **Per-model `BeforeSave` vault blocks and `AfterDelete` hooks removed** from `TableKey`, `TableMCPClient`, and `TableOauthConfig`. Each model now only implements `VaultPathKey()`. - **`AddProviderKey` / `UpdateProviderKey`** in `transports/bifrost-http/lib/config.go` re-read the stored key after a DB write so the in-memory copy reflects the vault reference rather than the original plaintext. - **Postgres helper functions** (`buildPostgresDSN`, `openPostresConnection`, `closeDbConn`, `applyPostgresPoolTuning`) extracted into `framework/configstore/postgres.go` to reduce duplication in the two-pool lifecycle. - **Helm chart and JSON schemas** updated to expose `vaultStore` configuration under `storage.configStore`, including `type`, `prefix`, `accessMode`, and backend-specific blocks for AWS, GCP, and HashiCorp Vault. ## Type of change - [ ] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... ./framework/configstore/... ``` - `TestStoreOwnedVaultEnvVars_WalksMap` — verifies that `map[string]EnvVar` entries are stored individually and converted to vault refs. - `TestRemoveOwnedVaultEnvVars_WalksMap` — verifies that only owned (non-fragment) map entries are removed. - `TestVaultCallbacks_AutoStoreAndRemove` — end-to-end test using an in-memory SQLite DB: creates a `TableMCPClient` with a plaintext `Authorization` header, asserts the vault store callback fires and the persisted `HeadersJSON` holds the vault ref, then deletes the row and asserts the remove callback fires. - `TestVaultCallbacks_NoOpWhenDisabled` — asserts no vault refs appear in the DB when hooks are not installed. To exercise vault configuration via Helm, set `storage.configStore.vaultStore.enabled: true` with the appropriate `type` and backend block. ## Breaking changes - [x] Yes - [ ] No `VaultStoreEnabled()` has been renamed to `VaultStoreWriteEnabled()`. Any enterprise or external code calling `VaultStoreEnabled()` must be updated to use `VaultStoreWriteEnabled()`. The semantics also changed slightly: write operations now require both `VaultStoreHook` and `VaultRemoveHook` to be wired. ## Related issues N/A ## Security considerations - Plaintext secrets are pushed to the vault backend before the DB row is written; the DB row stores only the `vault.<path>` reference. - Fragment refs (`vault.<path>#<key>`) pointing at externally-managed shared secrets are explicitly excluded from auto-deletion to prevent accidental removal of secrets owned by other systems. - The `read_only` access mode (resolvable via config schema) prevents auto-store and auto-delete when only secret resolution is needed. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
This was referenced Jun 27, 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
This PR centralises vault secret management into a single pair of global GORM callbacks (
bifrost:vault_storeandbifrost:vault_remove), removing the duplicated per-modelBeforeSave/AfterDeletevault logic fromTableKey,TableMCPClient, andTableOauthConfig. Models opt in by implementing the newVaultPathKeyerinterface. It also extends vault support tomap[string]EnvVarfields (e.g. MCPHeaders), which previously were not walked by the store/remove helpers.Changes
VaultPathKeyerinterface added tocore/schemas/vault.go. Models that implementVaultPathKey() stringare automatically handled by the global callbacks without any per-model wiring.VaultStoreEnabled()renamed toVaultStoreWriteEnabled()and now requires bothVaultStoreHookandVaultRemoveHookto be non-nil before write operations are attempted.map[string]EnvVarsupport added to bothStoreOwnedVaultEnvVarsandRemoveOwnedVaultEnvVars. Each map entry is stored atbasePath/<column>/<mapKey>. Fragment refs (#key) pointing at externally-managed shared secrets are never auto-deleted.removeOwnedVaultEnvVarextracted as a private helper to deduplicate the single-field removal logic used by both the struct-field and map-entry paths.RegisterVaultCallbacks(db)introduced inframework/configstore/vault_callbacks.go. It registersvaultStoreCallback(before create/update) andvaultRemoveCallback(after delete) on any*gorm.DB. Called fromopenPostresConnectionso every pool gets the callbacks automatically.BeforeSavevault blocks andAfterDeletehooks removed fromTableKey,TableMCPClient, andTableOauthConfig. Each model now only implementsVaultPathKey().AddProviderKey/UpdateProviderKeyintransports/bifrost-http/lib/config.gore-read the stored key after a DB write so the in-memory copy reflects the vault reference rather than the original plaintext.buildPostgresDSN,openPostresConnection,closeDbConn,applyPostgresPoolTuning) extracted intoframework/configstore/postgres.goto reduce duplication in the two-pool lifecycle.vaultStoreconfiguration understorage.configStore, includingtype,prefix,accessMode, and backend-specific blocks for AWS, GCP, and HashiCorp Vault.Type of change
Affected areas
How to test
go test ./core/schemas/... ./framework/configstore/...TestStoreOwnedVaultEnvVars_WalksMap— verifies thatmap[string]EnvVarentries are stored individually and converted to vault refs.TestRemoveOwnedVaultEnvVars_WalksMap— verifies that only owned (non-fragment) map entries are removed.TestVaultCallbacks_AutoStoreAndRemove— end-to-end test using an in-memory SQLite DB: creates aTableMCPClientwith a plaintextAuthorizationheader, asserts the vault store callback fires and the persistedHeadersJSONholds the vault ref, then deletes the row and asserts the remove callback fires.TestVaultCallbacks_NoOpWhenDisabled— asserts no vault refs appear in the DB when hooks are not installed.To exercise vault configuration via Helm, set
storage.configStore.vaultStore.enabled: truewith the appropriatetypeand backend block.Breaking changes
VaultStoreEnabled()has been renamed toVaultStoreWriteEnabled(). Any enterprise or external code callingVaultStoreEnabled()must be updated to useVaultStoreWriteEnabled(). The semantics also changed slightly: write operations now require bothVaultStoreHookandVaultRemoveHookto be wired.Related issues
N/A
Security considerations
vault.<path>reference.vault.<path>#<key>) pointing at externally-managed shared secrets are explicitly excluded from auto-deletion to prevent accidental removal of secrets owned by other systems.read_onlyaccess mode (resolvable via config schema) prevents auto-store and auto-delete when only secret resolution is needed.Checklist
docs/contributing/README.mdand followed the guidelines