refactor(registry): require SecretBackend on from_configs_with_models#285
Merged
Destynova2 merged 1 commit intomainfrom Apr 26, 2026
Merged
refactor(registry): require SecretBackend on from_configs_with_models#285Destynova2 merged 1 commit intomainfrom
Destynova2 merged 1 commit intomainfrom
Conversation
7848c17 to
497fb78
Compare
The signature of `ProviderRegistry::from_configs_with_models` previously took raw `&[ProviderConfig]` and trusted callers to invoke `storage::secrets::resolve_provider_secrets` first. That trust failed three times in three months — once per reload path: - PR #280: `preset::build_registry` (CLI `grob validate`) - PR #284: `server::config_guard`, `server::config_api`, `server::rpc::server_ns` (PUT /api/config, POST /api/config/reload, JSON-RPC server/reload) The bug repeated because the API made it optional to do the right thing. This refactor makes resolution mandatory at the type level: every caller must pass `&dyn SecretBackend`, and the function applies `resolve_provider_secrets` internally before constructing any provider. Diff summary: - 1 new required parameter (`secret_backend: &dyn SecretBackend`) - 5 callers simplified (build_backend → pass through, drop the local `resolve_provider_secrets` step that PRs #280/#284 added) - 1 test fixture switched to `EnvBackend` (no-op for literal keys) - 1 new regression test (`from_configs_routes_resolution_through_backend`) that asserts the backend's `get` is called exactly once for a `secret:`-prefixed provider — would have caught all three historical bugs at compile-AND-test time Full test suite passes: 1231 tests, 1231 passed. Closes the bug class entirely. Any future caller that builds a ProviderRegistry now compiles only if it supplies a backend, and any backend it passes will be exercised by the resolver. We can never again "forget to call resolve_provider_secrets". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
497fb78 to
8b1a1d7
Compare
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
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
ProviderRegistry::from_configs_with_modelspreviously took raw&[ProviderConfig]and trusted callers to invokestorage::secrets::resolve_provider_secretsfirst. That trust failed three times in three months — once per reload path:preset::build_registry(CLIgrob validate)server::config_guard::reload_state(PUT/api/config)server::config_api::reload_config(POST/api/config/reload)server::rpc::server_ns::reload_config(JSON-RPCserver/reload)The bug repeated because the API made it optional to do the right thing. The fix: make secret resolution mandatory at the type level.
What changes
Add
secret_backend: &dyn SecretBackendas a required parameter. Applyresolve_provider_secretsinternally before building any provider. Every caller now compiles only if it supplies a backend; every backend it supplies will be exercised by the resolver.Diff summary
src/providers/registry.rsresolve_provider_secretscallsrc/server/init.rssrc/preset/validation.rssrc/server/config_guard.rssrc/server/config_api.rssrc/server/rpc/server_ns.rssrc/providers/registry.rs::testsEnvBackend(no-op for literal keys)Regression test
Added
from_configs_routes_resolution_through_backend. It uses aCountingBackendwhosegetis asserted to be called exactly once for a singlesecret:-prefixed provider. A future caller that compiles but somehow bypasses resolution would surface as a zero-call counter even before reaching production — closing the loop the unit tests onresolve_provider_secretsleft open.Why no broader test was needed
The unit tests on
resolve_provider_secretsalready cover the resolution logic itself (4 tests: secret prefix, unknown name, plain string passthrough, missing api_key). The new regression test covers the integration between the resolver and the registry constructor. Together with the type-level enforcement, the bug class is closed.Test plan
cargo check --all-featurescleancargo nextest run --all-features— 1231 tests, 1231 passedcargo nextest run -E 'test(from_configs)'— 1/1 (new regression test)cargo fmt --all -- --checkcleancargo clippy -- -D warningscleanRelationship to PRs #280 and #284
This refactor subsumes the per-callsite fixes in PR #280 and PR #284. If they merge first, this PR removes the now-redundant explicit
resolve_provider_secretscalls (the resolution moves intofrom_configs_with_models). If this PR merges first, those PRs become no-ops on the affected lines (the type signature change forces correctness).Either ordering works without conflict.
🤖 Generated with Claude Code