From 14b4465107d5b9e85bade8ab64ab6581d5246bff Mon Sep 17 00:00:00 2001 From: Om Narayan Date: Wed, 8 Apr 2026 21:21:28 +0530 Subject: [PATCH] Fix env var default value syntax (${VAR || "default"}) Resolve ${VAR || "default"} and ${VAR ?? "fallback"} patterns matching Maestro's behavior. Previously, undefined variables caused a JS ReferenceError that silently left the raw ${...} expression in place. Fixes #49, fixes #50 --- pkg/executor/flow_runner.go | 5 ++- pkg/executor/scripting.go | 3 +- pkg/executor/scripting_test.go | 24 +++++++++++++++ pkg/jsengine/engine.go | 43 ++++++++++++++++++++++++-- pkg/jsengine/engine_test.go | 56 ++++++++++++++++++++++++++++++++++ 5 files changed, 127 insertions(+), 4 deletions(-) diff --git a/pkg/executor/flow_runner.go b/pkg/executor/flow_runner.go index c538798..2b0355b 100644 --- a/pkg/executor/flow_runner.go +++ b/pkg/executor/flow_runner.go @@ -74,7 +74,10 @@ func (fr *FlowRunner) Run() FlowResult { if appID := fr.flow.Config.EffectiveAppID(); appID != "" { fr.script.SetVariable("APP_ID", appID) } - fr.script.SetVariables(fr.flow.Config.Env) + // Expand flow config env values to support ${VAR || "default"} syntax + for k, v := range fr.flow.Config.Env { + fr.script.SetVariable(k, fr.script.ExpandVariables(v)) + } // Apply commandTimeout if specified - overrides driver's default find timeout if fr.flow.Config.CommandTimeout > 0 { diff --git a/pkg/executor/scripting.go b/pkg/executor/scripting.go index 2dc80f2..9978b0b 100644 --- a/pkg/executor/scripting.go +++ b/pkg/executor/scripting.go @@ -476,11 +476,12 @@ func conditionTimeout(cond flow.Condition, sel *flow.Selector) int { } // 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() { oldVars := make(map[string]string) for k, v := range env { oldVars[k] = se.GetVariable(k) - se.SetVariable(k, v) + se.SetVariable(k, se.ExpandVariables(v)) } return func() { for k, v := range oldVars { diff --git a/pkg/executor/scripting_test.go b/pkg/executor/scripting_test.go index 1b5d0a3..9f73762 100644 --- a/pkg/executor/scripting_test.go +++ b/pkg/executor/scripting_test.go @@ -322,6 +322,30 @@ func TestScriptEngine_withEnvVars(t *testing.T) { } } +func TestScriptEngine_withEnvVars_DefaultValues(t *testing.T) { + se := NewScriptEngine() + defer se.Close() + + se.SetVariable("CLI_VAR", "from_cli") + + // withEnvVars should expand ${VAR || "default"} syntax + restore := se.withEnvVars(map[string]string{ + "APP_ID": `${APP_ID || "com.example.default"}`, + "USER_ID": `${CLI_VAR || "fallback"}`, + }) + + // APP_ID was undefined, should get default + if got := se.GetVariable("APP_ID"); got != "com.example.default" { + t.Errorf("APP_ID = %q, want %q", got, "com.example.default") + } + // CLI_VAR was defined, should get its value + if got := se.GetVariable("USER_ID"); got != "from_cli" { + t.Errorf("USER_ID = %q, want %q", got, "from_cli") + } + + restore() +} + func TestScriptEngine_GetOutput(t *testing.T) { se := NewScriptEngine() defer se.Close() diff --git a/pkg/jsengine/engine.go b/pkg/jsengine/engine.go index 2df7664..8cbe41b 100644 --- a/pkg/jsengine/engine.go +++ b/pkg/jsengine/engine.go @@ -442,8 +442,8 @@ func (e *Engine) ExpandVariables(text string) (string, error) { // Extract expression expr := result[idx+2 : end-1] - // Evaluate expression - value, err := e.EvalString(expr) + // Evaluate expression, auto-defining undefined variables on ReferenceError + value, err := e.evalWithUndefinedFallback(expr) if err != nil { // If evaluation fails, leave as-is or return error start = end @@ -458,6 +458,45 @@ func (e *Engine) ExpandVariables(text string) (string, error) { return result, nil } +// evalWithUndefinedFallback evaluates a JS expression, automatically defining +// undefined variables to prevent ReferenceError. This matches Maestro's behavior +// where undeclared variables are treated as undefined (falsy) rather than errors. +// Supports patterns like: ${VAR || "default"}, ${VAR ?? "fallback"} +func (e *Engine) evalWithUndefinedFallback(expr string) (string, error) { + const maxRetries = 10 + for i := 0; i < maxRetries; i++ { + value, err := e.EvalString(expr) + if err == nil { + return value, nil + } + // Check if it's a ReferenceError for an undefined variable + varName := extractUndefinedVarName(err.Error()) + if varName == "" { + return "", err // Not a ReferenceError, return original error + } + e.DefineUndefinedIfMissing(varName) + } + return "", fmt.Errorf("too many undefined variables in expression: %s", expr) +} + +// extractUndefinedVarName extracts the variable name from a goja ReferenceError. +// Example: "JS eval error: ReferenceError: APP_ID is not defined at :1:1(0)" +// Returns "APP_ID", or empty string if not a ReferenceError. +func extractUndefinedVarName(errMsg string) string { + const prefix = "ReferenceError: " + const suffix = " is not defined" + idx := strings.Index(errMsg, prefix) + if idx == -1 { + return "" + } + after := errMsg[idx+len(prefix):] + endIdx := strings.Index(after, suffix) + if endIdx == -1 { + return "" + } + return after[:endIdx] +} + // Close cleans up the engine (stops timers, etc.) // Safe to call multiple times. func (e *Engine) Close() { diff --git a/pkg/jsengine/engine_test.go b/pkg/jsengine/engine_test.go index 1c81833..f480a45 100644 --- a/pkg/jsengine/engine_test.go +++ b/pkg/jsengine/engine_test.go @@ -107,6 +107,62 @@ func TestExpandVariables(t *testing.T) { } } +func TestExpandVariables_DefaultValues(t *testing.T) { + engine := New() + defer engine.Close() + + // Set one variable, leave others undefined + engine.SetVariable("DEFINED_VAR", "existing_value") + + tests := []struct { + name string + input string + expected string + }{ + {"undefined var with || fallback", `${APP_ID || "com.example.app"}`, "com.example.app"}, + {"undefined var with || single quotes", `${APP_ID || 'com.example.app'}`, "com.example.app"}, + {"defined var with || fallback", `${DEFINED_VAR || "fallback"}`, "existing_value"}, + {"undefined var with ?? fallback", `${UNDEF_VAR ?? "nullish_default"}`, "nullish_default"}, + {"multiple undefined in chain", `${UNDEF_A || UNDEF_B || "last"}`, "last"}, + {"default value in text", `App: ${APP_ID || "com.example.app"}`, "App: com.example.app"}, + {"ternary with undefined", `${UNDEF_X ? "yes" : "no"}`, "no"}, + {"mixed defined and default", `${DEFINED_VAR}-${UNDEF_VAR || "default"}`, "existing_value-default"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := engine.ExpandVariables(tt.input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != tt.expected { + t.Errorf("expected %q, got %q", tt.expected, result) + } + }) + } +} + +func TestExtractUndefinedVarName(t *testing.T) { + tests := []struct { + errMsg string + expected string + }{ + {"JS eval error: ReferenceError: APP_ID is not defined at :1:1(0)", "APP_ID"}, + {"JS eval error: ReferenceError: myVar is not defined at :1:1(0)", "myVar"}, + {"JS eval error: TypeError: something went wrong", ""}, + {"random error", ""}, + } + + for _, tt := range tests { + t.Run(tt.errMsg, func(t *testing.T) { + result := extractUndefinedVarName(tt.errMsg) + if result != tt.expected { + t.Errorf("expected %q, got %q", tt.expected, result) + } + }) + } +} + func TestConsoleLog(t *testing.T) { engine := New() defer engine.Close()