Expand ${VAR} in step env: blocks across all step types (follow-up to #12)

[hishamMounir]

Summary

#12 fixed the missing ExpandVariables call for appId. The v1.0.7 follow-up extended it to flow Config.Env, withEnvVars (used by runFlow), and LaunchAppStep.Environment. The same class of bug remained for every step type that has a YAML env: block but didn't route through one of those expansion sites:

• runScript — ExecuteRunScript called SetVariable(k, v) directly.
• runBrowserScript / runWebViewScript — the driver layer JSON-encoded step.Env into window.__env without expansion.
• runFlow / retry — Env was expanded by withEnvVars at execute time but the canonical pre-execution mutator (ExpandStep) had only one of the two paths covered, so step.Env values stayed unexpanded for any downstream code that read them off the step struct.

Repro from the runScript path:

- runScript:
    file: seed.js
    env:
      BASE_URL: ${BASE_URL}

with -e BASE_URL=https://api.example.com left the script variable as the literal string ${BASE_URL}. Inside seed.js the script then constructed a URL from the unexpanded placeholder and passed it to http.post, surfacing as a fmt-mangled error from Go's url package:

Post "$%!B(MISSING)BASE_URL%!D(MISSING)/api/...": unsupported protocol scheme ""

Fix

Centralize env-map expansion in pkg/executor/scripting.go:

// expandEnvMap expands ${VAR} / ${VAR || "default"} in every value of a YAML
// `env:` map, in place.
func (se *ScriptEngine) expandEnvMap(env map[string]string) {
    for k, v := range env {
        env[k] = se.ExpandVariables(v)
    }
}

ExpandStep is now the canonical place expansion happens for every step type with an Env field:

Step type	Before	After
LaunchAppStep	inline for k, v := range	expandEnvMap (refactor)
RunFlowStep	inline for k, v := range	expandEnvMap (refactor)
RunScriptStep	missing	expandEnvMap
RunBrowserScriptStep	missing	expandEnvMap
RunWebViewScriptStep	missing	expandEnvMap
RetryStep	missing (withEnvVars handles)	expandEnvMap (defensive)
DefineVariablesStep	missing (Execute handles)	expandEnvMap (defensive)

flow_runner.go now calls fr.script.ExpandStep(step) before ExecuteRunScript / ExecuteDefineVariables, mirroring the pattern already used for LaunchAppStep, RunFlowStep, RunBrowserScriptStep, and RunWebViewScriptStep.

ExecuteRunScript's inline ExpandVariables call is kept as a safety net for callers that bypass the dispatcher (tests, future internal APIs); it's a no-op once values are pre-expanded.

Tests

• TestScriptEngine_RunScript_EnvExpandsVariableRefs — direct RunScript path (original repro).
• TestScriptEngine_ExpandStep_RunScriptStep_Env — dispatch path; includes a literal value to confirm non-${} keys are untouched.
• TestScriptEngine_ExpandStep_RunBrowserScriptStep_Env
• TestScriptEngine_ExpandStep_RunWebViewScriptStep_Env

Test plan

• go test ./pkg/executor/ -count=1 — passes
• go build ./... — clean
• Manually verified: locally built binary runs a real flow whose runScript env block uses ${BASE_URL} and the script now receives the substituted value (previously: literal ${BASE_URL} reaching http.post).

🤖 Generated with Claude Code

[omnarayan]

@hishamMounir Nice fix — the expandEnvMap consolidation is clean and the test coverage hits each new step type.
Consolidates env expansion via expandEnvMap and ensures ExpandStep runs for DefineVariablesStep and RunScriptStep when dispatched through executeNestedStep, which doesn't have a blanket pre-expansion like executeStep does.

Notes

The matching ExpandStep calls added inside executeStep's switch are redundant — executeStep already calls fr.script.ExpandStep(step) unconditionally before the switch (around line 289):

// Expand variables in step before execution
fr.script.ExpandStep(step)
// Execute step - route to appropriate handler
var result *core.CommandResult
switch s := step.(type) {
case *flow.DefineVariablesStep:
    fr.script.ExpandStep(step)   // redundant, ExpandStep ran 7 lines up
    result = fr.script.ExecuteDefineVariables(s)
case *flow.RunScriptStep:
    fr.script.ExpandStep(step)   // redundant, same reason
    result = fr.script.ExecuteRunScript(s)

Idempotent so harmless — can be cleaned up in a follow-up, or left as-is.

The additions in executeNestedStep (lines 798/800) are the real fix, since that dispatcher doesn't have a blanket pre-expansion at the top.