fix: centralize environment variable expansion at config boundary

[yunus25jmi1]

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

[dgageot]

/review

[dgageot]

/review

[yunus25jmi1]

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

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

[yunus25jmi1]

@dgageot The PR is now in good shape. All lint issues have been resolved and the checks are passing. Ready for review!

[aheritier]

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:

1. Overreach: A reflection walker that expands all string fields could inadvertently expand literal $ values in user-defined instructions, prompt text, or tool output patterns — any field not intended to support expansion.
2. Interface change: Source carrying EnvProvider is a breaking change to the Source interface used by multiple call sites.
3. JS engine coupling: Decoupling the JS expander from env vars may break existing YAML configs that rely on ${env.VAR} being handled at the JS layer.

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.

[yunus25jmi1]

Hey @dgageot @aheritier — before continuing, I'd like to align on the direction given the triage concerns.

Three questions:

1. Field overreach: The reflection walker expands all exported string fields. Is blanket expansion the right default, or should we scope it to specific fields?

2. Interface change: Adding EnvProvider() to Source is breaking. Would passing env context as a parameter to Load() be preferable?

3. JS engine simplification: Removing env handling from the JS layer may break existing configs relying on ${env.VAR} at the JS layer. Any concerns?

Alternative path: The original issue #2615 suggested replacing os.ExpandEnv in pkg/path/expand.go with the JS expander — a narrower fix targeting just working_dir/path fields. Would the team prefer this as an initial step?

Looking for direction before investing further effort.