fix: centralize environment variable expansion at config boundary#2710
fix: centralize environment variable expansion at config boundary#2710yunus25jmi1 wants to merge 3 commits into
Conversation
Implement Layered Expansion: Layer 1 (Universal POSIX/JS syntax normalization) and Layer 2 (JS expressions). Add reflection-based Config walker to expand all fields during loading. Normalize ${env.VAR} to ${VAR} across all configuration fields. Improve error reporting for missing environment variables. Fixes docker#2615
yunus25jmi1
force-pushed
the
bug/fix-incompatible-env-expansion
branch
from
a09ecd7 to
3b35cf0
Compare
|
/review |
|
/review |
|
@dgageot I am currently working on this PR. I am facing some GO environment system-level configuration problems. I will make a comment when my PR is in good shape. |
|
❌ PR Review Failed — The review agent encountered an error and could not complete the review. View logs. |
- Rename ErrMissingVars to ErrMissingVarsError to conform to XxxError format - Update for loops to use Go 1.22+ integer range syntax - Remove unused testEnvProvider type from js/expand_test.go - Remove unused context import from js/expand_test.go - Update test files to use new ErrMissingVarsError type - Add EnvProvider method to mockSource in tests
|
@dgageot The PR is now in good shape. All lint issues have been resolved and the checks are passing. Ready for review! |
aheritier
added
area/config
|
Triage note: Community PR (contributor) with 5 comments and active discussion. The proposal — a reflection-based config walker that expands all string fields at load time — is a significant change to the config loading pipeline. Key concerns raised in comments:
Current status: not yet approved; needs maintainer sign-off on the design direction before proceeding. The original issue (#2615) may have a narrower fix available. |
|
Hey @dgageot @aheritier — before continuing, I'd like to align on the direction given the triage concerns. Three questions:
Alternative path: The original issue #2615 suggested replacing Looking for direction before investing further effort. |
aheritier
added
status/needs-design
Two incompatible expansion syntaxes coexist in agent YAML (see #2615): JS template literals (`${env.X}`) for prompt-like fields, and shell-style (`$VAR`/`${VAR}`/`~`) for path fields. When the wrong syntax is used in a field, expansion silently no-ops and the literal string is passed through. Emit a slog warning at config-load time when: - a JS-expanded field (description, welcome_message, instruction, commands, headers, env values) contains a shell-style `${UPPER_CASE}` reference and no `${env.X}` is present; - a path field (toolset working_dir, path) contains `${env.X}`. Warnings only — runtime behavior is unchanged. This makes the silent no-op visible while a proper unification is designed (#2710).
aheritier
added
status/needs-rebase
4 participants
Root Cause
Fragmented expansion logic across the codebase caused incompatibility between syntaxes (
$VARvs${env.VAR}) and silent failures. Path fields only supported POSIX style, while command fields used a JS-based expander that often ignored standard environment variables or failed silently.Solution: Layered Expansion Architecture
Implemented a robust two-layer expansion pipeline at the configuration boundary:
Layer 1: Universal Expansion (Config Loading Phase)
$VAR,${VAR},${env.VAR}) into a standard form.pkg/config/expand.go) to visit and expand every exported string field, slice, and map in the configuration immediately after unmarshaling.ErrMissingVarsinstead of silent partial expansion.Layer 2: Dynamic JS Expressions (Runtime Phase)
pkg/js/expand.go) by removing redundant environment handling.${tool(...)}), as environment variables are guaranteed to be resolved by Layer 1.Key Changes
os.Expand.Loadpipeline.Sourceinterface to carryEnvProvidercontext.Impact
$VAR,${VAR}, and${env.VAR}in all string-based config fields (working_dir, commands, instruction, etc).Fixes #2615