feat: expand secret/env refs in all ServerConfig string fields and DataDir#336
Merged
Dumbris merged 3 commits intosmart-mcp-proxy:mainfrom Mar 11, 2026
Merged
Conversation
Related smart-mcp-proxy#333 Adds the full spec-driven development artifacts for expanding ${env:...} and ${keyring:...} reference support to all string fields in ServerConfig and Config.DataDir. ## Artifacts - specs/034-expand-secret-refs/spec.md — user stories, FRs, SCs - specs/034-expand-secret-refs/research.md — 5 key design decisions - specs/034-expand-secret-refs/plan.md — TDD implementation plan - specs/034-expand-secret-refs/tasks.md — 17 ordered tasks - specs/034-expand-secret-refs/checklists/requirements.md — all pass
…verConfig Related smart-mcp-proxy#333 Foundational phase for expanding secret refs in all ServerConfig fields. ## Changes - Add SecretExpansionError type to internal/secret/resolver.go - Add ExpandStructSecretsCollectErrors method: mirrors expandValue with path tracking and collect-errors semantics (no fail-fast) - Export copyServerConfig -> CopyServerConfig in internal/config/merge.go (update 3 internal call sites) ## Tests - 7 new tests in resolver_test.go: HappyPath, PartialFailure, NilPointer, NestedStruct, SliceField, MapField, NoRefs — all pass - All existing config merge tests pass
… fields
Replace the manual 78-line field-by-field expansion allowlist in
NewClientWithOptions with reflection-based ExpandStructSecretsCollectErrors,
which automatically covers all current and future string fields in
ServerConfig (including nested IsolationConfig and OAuthConfig).
Also expands ${env:...} refs in Config.DataDir before os.MkdirAll so the
directory is created at the resolved path.
Changes:
- internal/secret/resolver.go: add SecretExpansionError + ExpandStructSecretsCollectErrors (collects errors, preserves unresolved values on failure)
- internal/config/merge.go: export CopyServerConfig (deep copy with Isolation/OAuth pointer fields)
- internal/upstream/core/client.go: replace manual expansion block with CopyServerConfig + ExpandStructSecretsCollectErrors + error logging
- internal/config/loader.go: add expandDataDir helper, call before MkdirAll in LoadFromFile and Load
- Tests: resolver_test.go (7 cases), client_secret_test.go (5 + reflection regression), config_test.go (DataDir expand + failure)
Member
|
Thank you for this excellent contribution! 🎉 The reflection-based The CI failures are pre-existing flaky quarantine tests ( Thanks again for the solid work! |
This was referenced Mar 11, 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
Fixes the silent expansion gap where
${env:...}and${keyring:...}references were only expanded forEnv,Args, andHeadersinNewClientWithOptions. Fields likeWorkingDir,URL,Command, and allIsolationConfig/OAuthConfigfields were silently ignored.Related #333
Problem
The manual allowlist approach guarantees recurrence: every new string field added to
ServerConfigrequires a manual code change to get expansion. Users configuring"working_dir": "${env:IH_HOME}"get the literal string, causing server startup failures.Approach
Replace the ~78-line manual expansion block with a new
ExpandStructSecretsCollectErrorsreflection-based method that:"Isolation.WorkingDir","Args[0]","Env[MY_VAR]")Also expands
Config.DataDirinloader.gobefore validation.Changes (planned — implementation not yet started)
internal/secret/resolver.go— addSecretExpansionErrortype +ExpandStructSecretsCollectErrorsmethodinternal/config/merge.go— exportCopyServerConfig(deep copy before expansion)internal/upstream/core/client.go— replace lines 105–182 with struct expansion (~12 lines)internal/upstream/core/client_secret_test.go— new: reflection regression test (SC-004)internal/config/loader.go— expandDataDirat 2 call sites beforecfg.Validate()internal/config/config_test.go— DataDir expansion testsSpec Artifacts
specs/034-expand-secret-refs/spec.md— user stories, requirements, success criteriaspecs/034-expand-secret-refs/plan.md— technical implementation planspecs/034-expand-secret-refs/research.md— key design decisionsspecs/034-expand-secret-refs/tasks.md— 17 ordered TDD tasksTest plan
ExpandStructSecretsCollectErrorstests written and passingNewClientWithOptionsexpansion tests + reflection regression test passingDataDirexpansion tests passinggo test ./internal/... -race./scripts/test-api-e2e.sh"working_dir": "${env:HOME}/test"starts with resolved path