refactor: rename SecretVar.Ref() to GetSecretRef() and IsFromEnv() to IsFromSecret() - #4600
Closed
BearTS wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRenames the exported ChangesSecretVar API Rename and Provenance Semantics Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Closed
18 tasks
BearTS
changed the title
BearTS
marked this pull request as ready for review
June 22, 2026 07:33
Contributor
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/schemas/vault_test.go (1)
174-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the
X-Envinvariant explicit in this test.This block only validates behavior when metadata is already correct, so a regression in
IsFromSecret()/GetSecretRef()can pass silently. Assert the metadata first, then assert it was not vault-stored.Suggested patch
- if env := m.Headers["X-Env"]; env.IsFromSecret() && env.GetSecretRef() == "env.X" { - if stored["bifrost/m/1/headers/X-Env"] != "" { - t.Error("env-sourced header should not be vault-stored") - } - } + env := m.Headers["X-Env"] + if !env.IsFromSecret() || env.GetSecretRef() != "env.X" { + t.Errorf("env header metadata changed unexpectedly: ref=%q fromSecret=%v", env.GetSecretRef(), env.IsFromSecret()) + } + if stored["bifrost/m/1/headers/X-Env"] != "" { + t.Error("env-sourced header should not be vault-stored") + }🤖 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` around lines 174 - 178, The test in the metadata validation block around the X-Env header check only validates the vault-storage assertion when the IsFromSecret() and GetSecretRef() conditions are already true, which means regressions in those methods could pass silently. Split the validation into two parts: first add explicit assertions that env.IsFromSecret() returns true and env.GetSecretRef() returns "env.X" to make the metadata invariant explicit, then separately assert that stored["bifrost/m/1/headers/X-Env"] is empty. This ensures that any regression in IsFromSecret() or GetSecretRef() will cause the test to fail rather than being skipped.
🤖 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/tables/encryption_test.go`:
- Line 252: The schema file transports/config.schema.json contains outdated
field naming that conflicts with the new provenance field semantics now
reflected in the code tests using IsFromSecret() and GetSecretRef() methods.
Update all occurrences of the env_var property to secret_ref and all occurrences
of the from_env property to from_secret throughout the schema file, including at
mcp_external_client_url (lines 268-269) and all other secret provenance field
definitions listed at lines 1150-1177, 2111-2112, 2143-2144, 2217-2218,
2232-2233, 2255-2256, and 2334-2335, to ensure the schema matches the code
contract and prevents validation drift and serialization failures.
In `@transports/bifrost-http/handlers/config.go`:
- Line 140: The SecretVar serialization in the handler produces "secret_ref" and
"from_secret" fields, but the schema definition for the mcp_external_client_url
field in transports/config.schema.json (lines 259–271) has additionalProperties
set to false and only allows the legacy "env_var" and "from_env" fields. Update
the mcp_external_client_url schema definition to accept both the old format
fields (env_var, from_env) for backward compatibility and the new format fields
(secret_ref, from_secret), or set additionalProperties to true to allow the new
serialized format to pass validation.
In `@transports/bifrost-http/lib/config.go`:
- Line 3647: The runtime code at line 3647 is now serializing secrets using
secret_ref and from_secret field names via schemas.NewSecretVarFromRef(), but
the schema validation in transports/config.schema.json has not been updated to
accept these fields. Update the schema to validate secret_ref and from_secret
alongside the existing env_var and from_env validations. Apply this change to
all secretized fields including mcp_external_client_url and credential fields
for bedrock, vault, azure, gcp, and ollama configurations to ensure the
serialized output will pass schema validation.
In `@transports/bifrost-http/lib/ctx_test.go`:
- Around line 535-537: The error message in the assertion that checks
IsFromSecret() still references the old "from-env" terminology which is
inconsistent with the updated API method name. Update the error message string
in the t.Error() call to use the new API terminology instead of "from-env",
making it consistent with what IsFromSecret() actually validates.
---
Outside diff comments:
In `@core/schemas/vault_test.go`:
- Around line 174-178: The test in the metadata validation block around the
X-Env header check only validates the vault-storage assertion when the
IsFromSecret() and GetSecretRef() conditions are already true, which means
regressions in those methods could pass silently. Split the validation into two
parts: first add explicit assertions that env.IsFromSecret() returns true and
env.GetSecretRef() returns "env.X" to make the metadata invariant explicit, then
separately assert that stored["bifrost/m/1/headers/X-Env"] is empty. This
ensures that any regression in IsFromSecret() or GetSecretRef() will cause the
test to fail rather than being skipped.
🪄 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: e9f826df-9757-4a93-a460-412bebceb942
📒 Files selected for processing (15)
core/providers/utils/utils.gocore/schemas/secretvar.gocore/schemas/secretvar_test.gocore/schemas/vault_test.goframework/configstore/clientconfig.goframework/configstore/clientconfig_redaction_test.goframework/configstore/encryption_test.goframework/configstore/rdb.goframework/configstore/tables/encryption_test.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey_secretvar_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx_test.go
| require.NoError(t, db.First(&found, key.ID).Error) | ||
| // The value should be readable (either the env var value or empty if not set) | ||
| assert.True(t, found.Value.IsFromEnv()) | ||
| assert.True(t, found.Value.IsFromSecret()) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate schema provenance keys =="
rg -n '"mcp_external_client_url"|"env_var"|"from_env"|"secret_ref"|"from_secret"' transports/config.schema.json
echo
echo "== Spot-check new-key usage in code/tests =="
rg -n 'GetSecretRef\(|IsFromSecret\(|"secret_ref"|"from_secret"' core framework transportsRepository: maximhq/bifrost
Length of output: 24649
Update transports/config.schema.json to reflect provenance field naming changes.
The code tests now use IsFromSecret()/GetSecretRef() semantics, but the schema still defines env_var/from_env for all secret provenance fields (including mcp_external_client_url at lines 268–269 and numerous other config surfaces at lines 1150–1177, 2111–2112, 2143–2144, 2217–2218, 2232–2233, 2255–2256, 2334–2335).
Schema is the source of truth for config validation and serialization. This naming mismatch will cause validation drift and serialization failures. Rename all occurrences of env_var → secret_ref and from_env → from_secret in the schema to match the updated code contract.
🤖 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/tables/encryption_test.go` at line 252, The schema file
transports/config.schema.json contains outdated field naming that conflicts with
the new provenance field semantics now reflected in the code tests using
IsFromSecret() and GetSecretRef() methods. Update all occurrences of the env_var
property to secret_ref and all occurrences of the from_env property to
from_secret throughout the schema file, including at mcp_external_client_url
(lines 268-269) and all other secret provenance field definitions listed at
lines 1150-1177, 2111-2112, 2143-2144, 2217-2218, 2232-2233, 2255-2256, and
2334-2335, to ensure the schema matches the code contract and prevents
validation drift and serialization failures.
Source: Coding guidelines
BearTS
force-pushed
the
06-22-refactor_change_ref_-_secretref
branch
from
June 22, 2026 07:46
2867ee5 to
bf0a972
Compare
BearTS
force-pushed
the
06-22-feat_make_secretref_and_fromsecret_non_global_fields
branch
from
June 22, 2026 08:38
983be90 to
6ccf345
Compare
BearTS
force-pushed
the
06-22-refactor_change_ref_-_secretref
branch
from
June 22, 2026 08:38
bf0a972 to
f6e7cf6
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/configstore/tables/encryption_test.go (1)
1808-1810: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert Bedrock provenance boolean in round-trip tests too.
These checks validate
GetSecretRef()but skipIsFromSecret(). Adding parity assertions (as done for Vertex/Azure/Ollama/SGL) strengthens coverage for the encryption no-op contract that keys off secret provenance.Suggested diff
assert.Equal(t, "env.FAKE_AWS_ACCESS_KEY_FOR_TEST", found.BedrockKeyConfig.AccessKey.GetSecretRef(), "env var reference for AccessKey lost on round-trip") + assert.True(t, found.BedrockKeyConfig.AccessKey.IsFromSecret(), + "from_secret flag for AccessKey lost on round-trip") assert.Equal(t, "env.FAKE_AWS_SECRET_KEY_FOR_TEST", found.BedrockKeyConfig.SecretKey.GetSecretRef(), "env var reference for SecretKey lost on round-trip") + assert.True(t, found.BedrockKeyConfig.SecretKey.IsFromSecret(), + "from_secret flag for SecretKey lost on round-trip")🤖 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/tables/encryption_test.go` around lines 1808 - 1810, The round-trip test assertions for BedrockKeyConfig are incomplete. Currently they check GetSecretRef() for both AccessKey and SecretKey but skip validating the IsFromSecret() provenance boolean. Add assertions after the existing GetSecretRef() checks to validate that BedrockKeyConfig.AccessKey.IsFromSecret() and BedrockKeyConfig.SecretKey.IsFromSecret() both return the expected boolean values, following the same pattern used in the test assertions for Vertex, Azure, Ollama, and SGL key configurations to maintain consistency and ensure complete encryption no-op contract validation.
🤖 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 `@framework/configstore/tables/encryption_test.go`:
- Around line 1808-1810: The round-trip test assertions for BedrockKeyConfig are
incomplete. Currently they check GetSecretRef() for both AccessKey and SecretKey
but skip validating the IsFromSecret() provenance boolean. Add assertions after
the existing GetSecretRef() checks to validate that
BedrockKeyConfig.AccessKey.IsFromSecret() and
BedrockKeyConfig.SecretKey.IsFromSecret() both return the expected boolean
values, following the same pattern used in the test assertions for Vertex,
Azure, Ollama, and SGL key configurations to maintain consistency and ensure
complete encryption no-op contract validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 81315b0a-6101-4424-8100-db2d6e9fc928
📒 Files selected for processing (16)
core/providers/utils/utils.gocore/schemas/secretvar.gocore/schemas/secretvar_test.gocore/schemas/vault_test.goframework/configstore/clientconfig.goframework/configstore/clientconfig_redaction_test.goframework/configstore/encryption_test.goframework/configstore/rdb.goframework/configstore/tables/encryption_test.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey_secretvar_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx_test.gotransports/config.schema.json
✅ Files skipped from review due to trivial changes (1)
- transports/bifrost-http/lib/config.go
🚧 Files skipped from review as they are similar to previous changes (10)
- framework/configstore/rdb.go
- transports/bifrost-http/lib/ctx_test.go
- framework/configstore/tables/mcp.go
- framework/configstore/tables/virtualkey_secretvar_test.go
- framework/configstore/clientconfig_redaction_test.go
- framework/configstore/clientconfig.go
- framework/configstore/encryption_test.go
- core/providers/utils/utils.go
- core/schemas/secretvar.go
- transports/bifrost-http/lib/config_test.go
18 tasks
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
1 participant
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
Renames the
Ref()method onSecretVartoGetSecretRef()to improve API clarity and consistency with the existingGetValue()andIsFromSecret()naming conventions. Alongside this rename, tests are updated to useIsFromSecret()instead ofIsFromEnv()andFromEnvfield accesses, and test fixtures are migrated from direct struct literal construction to theNewSecretVarFromRefconstructor.Changes
SecretVar.Ref()toSecretVar.GetSecretRef()across all call sites in core schemas, providers, config store, and HTTP transport layersIsFromEnv()assertions and directFromEnv/EnvVarfield accesses in tests withIsFromSecret()andGetSecretRef()to reflect the unified secret source abstractionSecretVar{Val: "", EnvVar: "...", FromEnv: true}struct literals in tests withNewSecretVarFromRef(...)constructor callsenv_var/from_envtosecret_ref/from_secretType of change
Affected areas
How to test
Breaking changes
Any code outside this repository that calls
SecretVar.Ref()orSecretVar.IsFromEnv()directly, or that accesses theEnvVar/FromEnvstruct fields, will need to be updated to useGetSecretRef()andIsFromSecret()respectively.Security considerations
No functional changes to secret resolution or storage logic. This is a pure rename/refactor with no impact on how secrets are read, written, or redacted.
Checklist
docs/contributing/README.mdand followed the guidelines