From cb517d497fe0863c26c9338991c50c71c8d6f68c Mon Sep 17 00:00:00 2001 From: Hisham Alkhudrawi Date: Sun, 10 May 2026 13:28:17 -0400 Subject: [PATCH] Expand ${VAR} in step env: blocks across all step types (follow-up to #12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #12 fixed the missing ExpandVariables call for `appId`, and 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}", surfacing as a fmt-mangled error from Go's url package when the JS later constructed a URL and called http.post: Post "$%!B(MISSING)BASE_URL%!D(MISSING)/api/...": unsupported protocol scheme "" Fix --- Centralize env-map expansion in pkg/executor/scripting.go: expandEnvMap(env) — single helper that walks a map[string]string and applies ExpandVariables to every value in place. No-op for values without ${...}, so plain literals pass through untouched. ExpandStep is now the canonical place that expansion happens for every step type with an Env field: - LaunchAppStep.Environment — replaced inline loop with helper. - RunFlowStep.Env — replaced inline loop with helper. - RunScriptStep.Env — new case. - RunBrowserScriptStep.Env — new case. - RunWebViewScriptStep.Env — new case. - RetryStep.Env — new case (defensive; withEnvVars also expands at execute time, which is now idempotent against pre-expansion). - DefineVariablesStep.Env — new case (same idempotency rationale). 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 — covers the direct RunScript path (the original repro from this report). - TestScriptEngine_ExpandStep_RunScriptStep_Env — covers the dispatch path including a literal value to confirm non-${} keys are untouched. - TestScriptEngine_ExpandStep_RunBrowserScriptStep_Env - TestScriptEngine_ExpandStep_RunWebViewScriptStep_Env All pkg/executor tests pass (go test ./pkg/executor/ -count=1). --- pkg/executor/flow_runner.go | 4 ++ pkg/executor/scripting.go | 38 +++++++++++--- pkg/executor/scripting_test.go | 90 ++++++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 8 deletions(-) diff --git a/pkg/executor/flow_runner.go b/pkg/executor/flow_runner.go index b357489..b060d85 100644 --- a/pkg/executor/flow_runner.go +++ b/pkg/executor/flow_runner.go @@ -294,8 +294,10 @@ func (fr *FlowRunner) executeStep(idx int, step flow.Step) (report.Status, strin switch s := step.(type) { // JS/Scripting steps - handled by ScriptEngine case *flow.DefineVariablesStep: + fr.script.ExpandStep(step) result = fr.script.ExecuteDefineVariables(s) case *flow.RunScriptStep: + fr.script.ExpandStep(step) result = fr.script.ExecuteRunScript(s) case *flow.EvalScriptStep: result = fr.script.ExecuteEvalScript(s) @@ -796,8 +798,10 @@ func (fr *FlowRunner) executeNestedStep(step flow.Step) *core.CommandResult { switch s := step.(type) { case *flow.DefineVariablesStep: + fr.script.ExpandStep(step) result = fr.script.ExecuteDefineVariables(s) case *flow.RunScriptStep: + fr.script.ExpandStep(step) result = fr.script.ExecuteRunScript(s) case *flow.EvalScriptStep: result = fr.script.ExecuteEvalScript(s) diff --git a/pkg/executor/scripting.go b/pkg/executor/scripting.go index 9978b0b..acb5c92 100644 --- a/pkg/executor/scripting.go +++ b/pkg/executor/scripting.go @@ -166,9 +166,13 @@ func (se *ScriptEngine) RunScript(script string, env map[string]string) error { // Expand variables in script script = se.ExpandVariables(script) - // Apply env variables + // Apply env variables. Values are expanded through ExpandVariables so + // `${VAR}` / `${VAR || "default"}` references resolve against the parent + // scope (CLI -e flags, flow Config.Env, prior runScript output). Mirrors + // withEnvVars used by runFlow — keeps env semantics consistent across + // step types so the YAML pattern `MY_KEY: ${MY_KEY}` works the same way. for k, v := range env { - se.SetVariable(k, v) + se.SetVariable(k, se.ExpandVariables(v)) } // Pre-define potential env variables as undefined to avoid ReferenceError. @@ -475,6 +479,18 @@ func conditionTimeout(cond flow.Condition, sel *flow.Selector) int { return 0 // Optional=true on the step means driver uses OptionalFindTimeout (7s) } +// expandEnvMap expands ${VAR} / ${VAR || "default"} in every value of a YAML +// `env:` map, in place. Used by ExpandStep for any step type whose Env field +// references parent-scope variables — keeps env semantics identical across +// runScript, runFlow, runBrowserScript, runWebViewScript, retry, and +// LaunchAppStep.Environment / .Arguments. No-op for keys whose values contain +// no `${...}` so plain literals stay untouched. +func (se *ScriptEngine) expandEnvMap(env map[string]string) { + for k, v := range env { + env[k] = se.ExpandVariables(v) + } +} + // withEnvVars applies environment variables and returns a restore function. // Values are expanded through ExpandVariables to support ${VAR || "default"} syntax. func (se *ScriptEngine) withEnvVars(env map[string]string) func() { @@ -536,9 +552,17 @@ func (se *ScriptEngine) ExpandStep(step flow.Step) { s.Arguments[k] = se.ExpandVariables(str) } } - for k, v := range s.Environment { - s.Environment[k] = se.ExpandVariables(v) - } + se.expandEnvMap(s.Environment) + case *flow.RunScriptStep: + se.expandEnvMap(s.Env) + case *flow.RunBrowserScriptStep: + se.expandEnvMap(s.Env) + case *flow.RunWebViewScriptStep: + se.expandEnvMap(s.Env) + case *flow.RetryStep: + se.expandEnvMap(s.Env) + case *flow.DefineVariablesStep: + se.expandEnvMap(s.Env) case *flow.StopAppStep: s.AppID = se.ExpandVariables(s.AppID) case *flow.KillAppStep: @@ -565,9 +589,7 @@ func (se *ScriptEngine) ExpandStep(step flow.Step) { s.When.Platform = se.ExpandVariables(s.When.Platform) } } - for k, v := range s.Env { - s.Env[k] = se.ExpandVariables(v) - } + se.expandEnvMap(s.Env) } } diff --git a/pkg/executor/scripting_test.go b/pkg/executor/scripting_test.go index 9f73762..441f64e 100644 --- a/pkg/executor/scripting_test.go +++ b/pkg/executor/scripting_test.go @@ -188,6 +188,31 @@ func TestScriptEngine_RunScript_WithEnv(t *testing.T) { } } +func TestScriptEngine_RunScript_EnvExpandsVariableRefs(t *testing.T) { + // runScript's `env:` block must expand ${VAR} against the parent scope, + // matching runFlow's withEnvVars semantics. This is the YAML pattern + // - runScript: + // env: + // BASE_URL: ${BASE_URL} + // where the outer BASE_URL came from a CLI -e flag or flow Config.Env. + se := NewScriptEngine() + defer se.Close() + + se.SetVariable("BASE_URL", "https://api.example.com") + + err := se.RunScript("output.url = BASE_URL", map[string]string{ + "BASE_URL": "${BASE_URL}", + }) + if err != nil { + t.Fatalf("RunScript() error = %v", err) + } + + if got := se.GetVariable("url"); got != "https://api.example.com" { + t.Errorf("url = %q, want %q (env value was passed through verbatim instead of expanded)", + got, "https://api.example.com") + } +} + func TestScriptEngine_RunScript_Error(t *testing.T) { se := NewScriptEngine() defer se.Close() @@ -1018,6 +1043,71 @@ func TestScriptEngine_ExpandStep_LaunchAppStep_Environment(t *testing.T) { } } +func TestScriptEngine_ExpandStep_RunScriptStep_Env(t *testing.T) { + se := NewScriptEngine() + defer se.Close() + + se.SetVariable("BASE_URL", "https://api.example.com") + se.SetVariable("STAGE", "staging") + + step := &flow.RunScriptStep{ + File: "seed.js", + Env: map[string]string{ + "BASE_URL": "${BASE_URL}", + "ENV": "${STAGE}", + "LITERAL": "hardcoded_value", + }, + } + + se.ExpandStep(step) + + if step.Env["BASE_URL"] != "https://api.example.com" { + t.Errorf("Env[BASE_URL] = %q, want %q", step.Env["BASE_URL"], "https://api.example.com") + } + if step.Env["ENV"] != "staging" { + t.Errorf("Env[ENV] = %q, want %q", step.Env["ENV"], "staging") + } + if step.Env["LITERAL"] != "hardcoded_value" { + t.Errorf("Env[LITERAL] = %q (literals must pass through untouched)", step.Env["LITERAL"]) + } +} + +func TestScriptEngine_ExpandStep_RunBrowserScriptStep_Env(t *testing.T) { + se := NewScriptEngine() + defer se.Close() + + se.SetVariable("API_KEY", "secret123") + + step := &flow.RunBrowserScriptStep{ + File: "probe.js", + Env: map[string]string{"KEY": "${API_KEY}"}, + } + + se.ExpandStep(step) + + if step.Env["KEY"] != "secret123" { + t.Errorf("Env[KEY] = %q, want %q", step.Env["KEY"], "secret123") + } +} + +func TestScriptEngine_ExpandStep_RunWebViewScriptStep_Env(t *testing.T) { + se := NewScriptEngine() + defer se.Close() + + se.SetVariable("API_KEY", "secret123") + + step := &flow.RunWebViewScriptStep{ + File: "probe.js", + Env: map[string]string{"KEY": "${API_KEY}"}, + } + + se.ExpandStep(step) + + if step.Env["KEY"] != "secret123" { + t.Errorf("Env[KEY] = %q, want %q", step.Env["KEY"], "secret123") + } +} + func TestScriptEngine_ExpandStep_StopAppStep(t *testing.T) { se := NewScriptEngine() defer se.Close()