Skip to content

fix: centralize environment variable expansion at config boundary#2710

Open
yunus25jmi1 wants to merge 2 commits intodocker:mainfrom
yunus25jmi1:bug/fix-incompatible-env-expansion
Open

fix: centralize environment variable expansion at config boundary#2710
yunus25jmi1 wants to merge 2 commits intodocker:mainfrom
yunus25jmi1:bug/fix-incompatible-env-expansion

Conversation

@yunus25jmi1
Copy link
Copy Markdown
Contributor

Root Cause

Fragmented expansion logic across the codebase caused incompatibility between syntaxes ($VAR vs ${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:

  1. Layer 1: Universal Expansion (Config Loading Phase)

    • Normalizes all syntaxes ($VAR, ${VAR}, ${env.VAR}) into a standard form.
    • Uses a reflection-based walker (pkg/config/expand.go) to visit and expand every exported string field, slice, and map in the configuration immediately after unmarshaling.
    • Provides explicit error reporting via ErrMissingVars instead of silent partial expansion.
  2. Layer 2: Dynamic JS Expressions (Runtime Phase)

    • Simplified the JS expander (pkg/js/expand.go) by removing redundant environment handling.
    • JS engine now focuses strictly on dynamic logic (e.g., ${tool(...)}), as environment variables are guaranteed to be resolved by Layer 1.

Key Changes

  • pkg/environment/expand.go: Added normalization and centralized expansion logic using os.Expand.
  • pkg/config/expand.go: New reflection-based config walker to ensure 100% field coverage.
  • pkg/config/config.go: Integrated expansion into the main Load pipeline.
  • pkg/js/expand.go: Decoupled JS engine from environment resolution.
  • pkg/config/sources.go: Updated Source interface to carry EnvProvider context.

Impact

  • Full support for $VAR, ${VAR}, and ${env.VAR} in all string-based config fields (working_dir, commands, instruction, etc).
  • Elimination of silent expansion failures.
  • Cleaner separation of concerns between configuration loading and execution runtime.

Fixes #2615

@yunus25jmi1 yunus25jmi1 requested a review from a team as a code owner May 8, 2026 07:21
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 yunus25jmi1 force-pushed the bug/fix-incompatible-env-expansion branch from a09ecd7 to 3b35cf0 Compare May 8, 2026 07:25
@dgageot
Copy link
Copy Markdown
Member

dgageot commented May 8, 2026

/review

@docker docker deleted a comment from docker-agent May 8, 2026
@dgageot
Copy link
Copy Markdown
Member

dgageot commented May 8, 2026

/review

@yunus25jmi1
Copy link
Copy Markdown
Contributor Author

@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.

@docker-agent
Copy link
Copy Markdown

PR Review Failed — The review agent encountered an error and could not complete the review. View logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two incompatible env-variable expansion syntaxes coexist silently across agent yaml fields

3 participants