From a8b97b5d6f84138b6c17724e46d479d8fbac65d8 Mon Sep 17 00:00:00 2001 From: Tom Barlow <60068+tombee@users.noreply.github.com> Date: Thu, 8 Jan 2026 13:51:03 +0000 Subject: [PATCH] Add conditional step execution in YAML workflows Implement ergonomic conditional execution for workflow steps: - Add `if` field to StepDefinition as alias for `condition.expression` - Add template pre-processor to resolve {{.steps.id.field}} references in expressions - Add step ID validation at parse time to catch errors early - Enforce strict boolean evaluation with nil-as-false semantics - Update skipped step output format to match specification - Include comprehensive documentation and example workflows --- docs/features/conditions.md | 273 ++++++++++++++--- examples/README.md | 5 +- examples/showcase/conditional-workflow.yaml | 189 ++++++++++++ pkg/workflow/definition.go | 20 ++ pkg/workflow/definition_test.go | 265 ++++++++++++++++ pkg/workflow/executor.go | 27 +- pkg/workflow/executor_condition_test.go | 227 +++++++++++++- pkg/workflow/expression/evaluator.go | 7 +- pkg/workflow/expression/evaluator_test.go | 102 ++++++ pkg/workflow/expression/template.go | 127 ++++++++ pkg/workflow/expression/template_test.go | 324 ++++++++++++++++++++ pkg/workflow/expression/validate.go | 97 ++++++ pkg/workflow/expression/validate_test.go | 196 ++++++++++++ 13 files changed, 1802 insertions(+), 57 deletions(-) create mode 100644 examples/showcase/conditional-workflow.yaml create mode 100644 pkg/workflow/expression/template.go create mode 100644 pkg/workflow/expression/template_test.go create mode 100644 pkg/workflow/expression/validate.go create mode 100644 pkg/workflow/expression/validate_test.go diff --git a/docs/features/conditions.md b/docs/features/conditions.md index b2df4152..59f5962b 100644 --- a/docs/features/conditions.md +++ b/docs/features/conditions.md @@ -2,9 +2,9 @@ Execute steps conditionally based on expressions. -## Basic Conditions +## The `if` Field -Skip steps based on expressions: +The recommended way to conditionally execute steps is with the `if` field: ```yaml steps: @@ -12,102 +12,272 @@ steps: llm: prompt: "Is it raining? Answer yes or no." - id: bring_umbrella - condition: ${steps.check.output == "yes"} + if: "{{.steps.check.output}} == 'yes'" llm: prompt: "Remind me to bring an umbrella" ``` -The `bring_umbrella` step only runs if the condition is true. +The `bring_umbrella` step only runs if the condition evaluates to `true`. If `false`, the step is skipped (not errored). + +### Basic Syntax + +The `if` field accepts a string expression that must evaluate to boolean: + +```yaml +# Template syntax (recommended) +if: "{{.steps.check.stdout}} == 'value'" + +# Bare syntax (also supported) +if: "steps.check.stdout == 'value'" + +# Workflow inputs +if: "{{.inputs.mode}} == 'strict'" +if: "inputs.mode == 'strict'" + +# Boolean values +if: "{{.inputs.enabled}}" +if: "inputs.enabled == true" +``` + +### Relationship to `condition.expression` + +The `if` field is shorthand for `condition.expression`. These are equivalent: + +```yaml +# Using if (recommended) +- id: step1 + if: "inputs.enabled == true" + +# Using condition +- id: step2 + condition: + expression: "inputs.enabled == true" +``` + +You cannot use both `if` and `condition` on the same step. + +## Basic Conditions (Legacy Syntax) + +The older `condition` syntax is still supported: + +```yaml +steps: + - id: check + llm: + prompt: "Is it raining? Answer yes or no." + - id: bring_umbrella + condition: ${steps.check.output == "yes"} + llm: + prompt: "Remind me to bring an umbrella" +``` -## Condition Syntax +## Expression Operators -Conditions use simple expressions: +The `if` field supports standard comparison and boolean operators: ```yaml # Equality -condition: ${steps.step1.output == "expected"} +if: "{{.steps.step1.output}} == 'expected'" # Inequality -condition: ${steps.step1.output != "skip"} +if: "{{.steps.step1.output}} != 'skip'" # Numeric comparison -condition: ${steps.count.output > 10} +if: "{{.steps.count.output}} > 10" # Boolean -condition: ${inputs.enabled == true} +if: "{{.inputs.enabled}} == true" +if: "inputs.enabled" -# Contains -condition: ${steps.check.output | contains("keyword")} +# String contains +if: "contains({{.steps.check.output}}, 'keyword')" ``` -## Multiple Conditions +### Comparison Operators +- `==` - Equal +- `!=` - Not equal +- `<` - Less than +- `>` - Greater than +- `<=` - Less than or equal +- `>=` - Greater than or equal + +### Boolean Operators Combine conditions with logical operators: ```yaml -# AND -condition: ${steps.check1.output == "yes" && steps.check2.output == "yes"} +# AND - All conditions must be true +if: "inputs.env == 'prod' && inputs.region == 'us'" + +# OR - At least one condition must be true +if: "inputs.region == 'us' || inputs.region == 'eu'" -# OR -condition: ${steps.check1.output == "yes" || steps.check2.output == "yes"} +# NOT - Negate a condition +if: "!inputs.dry_run" -# NOT -condition: ${!(steps.check.output == "skip")} +# Complex expressions +if: "inputs.env == 'prod' && (inputs.region == 'us' || inputs.region == 'eu') && !inputs.dry_run" ``` -## Input-Based Conditions +### Array Membership -Conditionally execute based on inputs: +Check if a value exists in an array: ```yaml +# Using 'in' operator +if: "'feature' in inputs.features" + +# Check multiple values +if: "'security' in inputs.personas || 'compliance' in inputs.personas" +``` + +## Common Patterns + +### Input-Based Conditions + +Branch based on workflow inputs: + +```yaml +inputs: + - name: environment + type: string + steps: - id: production_deploy - condition: ${inputs.environment == "production"} + if: "inputs.environment == 'production'" shell: command: ./deploy-production.sh + - id: staging_deploy - condition: ${inputs.environment == "staging"} + if: "inputs.environment == 'staging'" shell: command: ./deploy-staging.sh ``` -## Default Values +### Conditional Based on Previous Step Output -Handle missing data with defaults: +Execute steps only if previous steps produce specific output: ```yaml steps: - - id: optional_step - condition: ${inputs.runOptional | default(false)} + - id: check_tests + shell: + command: test -d tests && echo 'has_tests' || echo 'no_tests' + + - id: run_tests + if: "{{.steps.check_tests.stdout}} == 'has_tests'" + shell: + command: npm test + + - id: skip_message + if: "{{.steps.check_tests.stdout}} == 'no_tests'" llm: - prompt: "This runs if runOptional is true" + prompt: "No tests directory found, skipping test execution" ``` -## Condition Functions +### Chained Conditions + +Reference multiple previous steps: + +```yaml +steps: + - id: validate + shell: + command: ./validate.sh + + - id: build + if: "{{.steps.validate.exit_code}} == 0" + shell: + command: ./build.sh + + - id: deploy + if: "steps.validate.status == 'success' && steps.build.status == 'success'" + shell: + command: ./deploy.sh +``` -Available functions: +### Available Functions -- `contains(substring)` - Check if string contains substring -- `startsWith(prefix)` - Check if string starts with prefix -- `endsWith(suffix)` - Check if string ends with suffix -- `default(value)` - Use default if undefined -- `length()` - Get array/string length -- `isEmpty()` - Check if empty +The `if` field supports these expression functions: + +- `contains(string, substring)` - Check if string contains substring +- `startsWith(string, prefix)` - Check if string starts with prefix +- `endsWith(string, suffix)` - Check if string ends with suffix +- `length(array)` - Get array/string length ```yaml -# Length check -condition: ${steps.items.output | length() > 0} +# Check substring +if: "contains({{.steps.result.output}}, 'success')" + +# Check array length +if: "length(inputs.features) > 0" -# Empty check -condition: ${!(steps.result.output | isEmpty())} +# Check prefix +if: "startsWith({{.steps.check.output}}, 'ERROR')" ``` -## Skipped Steps +## Skipped Step Behavior + +When a step is skipped because its `if` condition evaluates to `false`: + +- The step has status `skipped` (not `failed` or `success`) +- Output fields are set to zero values: strings → `""`, numbers → `0`, booleans → `false` +- Downstream steps can safely reference the skipped step's outputs (they return empty values) +- Skipped steps don't trigger error handlers +- The workflow continues executing subsequent steps + +Example: -When a step is skipped by a condition: -- Its output is undefined -- Steps depending on it are also skipped -- The workflow continues with other steps +```yaml +steps: + - id: optional_check + if: "inputs.run_check == true" + shell: + command: ./check.sh + + - id: continue + # This runs even if optional_check was skipped + llm: + prompt: "Continuing workflow..." +``` + +## Type Safety + +The `if` field requires boolean expressions. Non-boolean results cause an error: + +```yaml +# ✓ Valid - evaluates to boolean +if: "inputs.enabled == true" +if: "{{.steps.count.output}} > 5" +if: "inputs.enabled" + +# ✗ Invalid - evaluates to non-boolean +if: "{{.steps.check.stdout}}" # Error: returns string +if: "{{.steps.count.output}}" # Error: returns number +if: "42" # Error: not a boolean +``` + +Use explicit comparisons: + +```yaml +# Check if output is non-empty +if: "{{.steps.check.stdout}} != ''" + +# Check if count is non-zero +if: "{{.steps.count.output}} > 0" +``` + +### Nil Handling + +When a step is skipped, its outputs return nil. Use explicit nil checks: + +```yaml +# Safe nil handling +if: "steps.previous.result != nil && steps.previous.result > 5" + +# Without nil check (may cause error if previous was skipped) +if: "steps.previous.result > 5" +``` ## Error Handling @@ -119,13 +289,22 @@ steps: http: method: GET url: https://api.example.com/data + - id: fallback - condition: ${steps.try_api.error != null} + if: "steps.try_api.status != 'success'" file: action: read path: cached-data.json ``` +## Best Practices + +1. **Use template syntax** - `{{.steps.id.field}}` is more consistent with the rest of Conductor +2. **Check for success explicitly** - `if: "steps.validate.status == 'success'"` is clearer than relying on truthy values +3. **Handle nil safely** - Always check for nil when referencing potentially skipped steps +4. **Keep expressions simple** - Complex logic should be in steps, not conditions +5. **Document intent** - Use step names that explain why something is conditional + ## Performance -Conditions are evaluated before step execution. Skipped steps don't consume resources. +Conditions are evaluated before step execution. Skipped steps don't consume resources or call external services. diff --git a/examples/README.md b/examples/README.md index 68022e66..43bf6ccd 100644 --- a/examples/README.md +++ b/examples/README.md @@ -18,9 +18,10 @@ Progressive meal planning workflows from the [documentation tutorial](../docs/tu Complete real-world workflows demonstrating best practices: -| Directory | Description | -|-----------|-------------| +| File | Description | +|------|-------------| | `showcase/code-review.yaml` | Multi-persona code review | +| `showcase/conditional-workflow.yaml` | Conditional execution patterns with `if` field | | `showcase/issue-triage.yaml` | GitHub issue classification | | `showcase/slack-notify.yaml` | Formatted Slack notifications | diff --git a/examples/showcase/conditional-workflow.yaml b/examples/showcase/conditional-workflow.yaml new file mode 100644 index 00000000..a0e52a3c --- /dev/null +++ b/examples/showcase/conditional-workflow.yaml @@ -0,0 +1,189 @@ +# yaml-language-server: $schema=../../schemas/workflow.schema.json +name: conditional-workflow +description: Demonstrates conditional step execution patterns using the if field + +inputs: + - name: environment + type: string + required: true + description: Target environment (development, staging, production) + + - name: run_tests + type: boolean + required: false + description: Whether to run test suite + + - name: features + type: array + required: false + description: List of feature flags to enable + +steps: + # Example 1: Conditional based on workflow input + - id: development_setup + name: Development Environment Setup + if: "inputs.environment == 'development'" + type: shell + command: | + echo "Setting up development environment" + echo "Debug mode: enabled" + + - id: production_setup + name: Production Environment Setup + if: "inputs.environment == 'production'" + type: shell + command: | + echo "Setting up production environment" + echo "Debug mode: disabled" + echo "Monitoring: enabled" + + # Example 2: Conditional based on boolean input + - id: run_test_suite + name: Run Tests + if: "inputs.run_tests == true" + type: shell + command: | + echo "Running test suite..." + echo "Tests: PASSED" + + - id: skip_tests_message + name: Skip Tests Message + if: "inputs.run_tests == false || inputs.run_tests == nil" + type: llm + model: fast + prompt: "Generate a message explaining that tests were skipped." + + # Example 3: Environment detection + - id: check_docker + name: Check Docker Availability + type: shell + command: | + if command -v docker &> /dev/null; then + echo "available" + else + echo "unavailable" + fi + + - id: docker_deploy + name: Deploy with Docker + if: "{{.steps.check_docker.stdout}} == 'available'" + type: shell + command: | + echo "Deploying with Docker..." + echo "Container started: app-${ENVIRONMENT}" + + - id: manual_deploy + name: Manual Deployment + if: "{{.steps.check_docker.stdout}} == 'unavailable'" + type: shell + command: | + echo "Docker not available, using manual deployment" + echo "Starting application manually..." + + # Example 4: Array membership check + - id: security_scan + name: Security Scan + if: "'security' in inputs.features" + type: llm + model: balanced + prompt: | + Perform a security scan analysis. + + Environment: {{.inputs.environment}} + Docker status: {{.steps.check_docker.stdout}} + + Provide a security checklist for this deployment. + + - id: performance_analysis + name: Performance Analysis + if: "'performance' in inputs.features || inputs.environment == 'production'" + type: llm + model: balanced + prompt: | + Analyze performance considerations for this deployment. + + Environment: {{.inputs.environment}} + Docker: {{.steps.check_docker.stdout}} + + Provide performance optimization recommendations. + + # Example 5: Chained conditions with boolean operators + - id: production_validation + name: Production Pre-Deploy Validation + if: "inputs.environment == 'production' && {{.steps.check_docker.stdout}} == 'available' && (inputs.run_tests == true || inputs.run_tests == nil)" + type: shell + command: | + echo "Running production validation checks..." + echo "✓ Environment: production" + echo "✓ Docker: available" + echo "✓ Tests: completed or skipped intentionally" + echo "Validation: PASSED" + + # Example 6: Conditional based on previous step status + - id: check_validation + name: Check Validation Status + if: "steps.production_validation.status == 'success'" + type: llm + model: fast + prompt: | + The production validation passed successfully. + Generate a deployment approval message. + + # Example 7: Error recovery pattern + - id: deployment + name: Deploy Application + if: "inputs.environment != 'development'" + type: shell + command: | + echo "Deploying to {{.inputs.environment}}..." + # Simulate deployment + if [ "{{.inputs.environment}}" = "production" ]; then + echo "Production deployment complete" + exit 0 + else + echo "Staging deployment complete" + exit 0 + fi + + - id: deployment_success + name: Deployment Success Notification + if: "steps.deployment.status == 'success'" + type: llm + model: fast + prompt: | + Generate a success notification for the deployment to {{.inputs.environment}}. + Include the key steps that were executed. + + - id: deployment_failure + name: Deployment Failure Recovery + if: "steps.deployment.status != 'success' && steps.deployment.status != 'skipped'" + type: llm + model: fast + prompt: | + The deployment to {{.inputs.environment}} failed. + Generate a recovery plan and rollback instructions. + + # Example 8: Negation operator + - id: non_production_notice + name: Non-Production Notice + if: "!inputs.environment == 'production'" + type: shell + command: | + echo "Note: This is a non-production environment" + echo "Additional logging and debugging enabled" + +outputs: + - name: environment + type: string + value: "{{.inputs.environment}}" + description: Target environment + + - name: docker_status + type: string + value: "{{.steps.check_docker.stdout}}" + description: Docker availability status + + - name: deployment_result + type: string + value: "{{.steps.deployment.stdout}}" + description: Deployment result message diff --git a/pkg/workflow/definition.go b/pkg/workflow/definition.go index c074b37b..05a2e263 100644 --- a/pkg/workflow/definition.go +++ b/pkg/workflow/definition.go @@ -169,6 +169,10 @@ type StepDefinition struct { // Only valid for type: integration steps Operation string `yaml:"operation,omitempty" json:"operation,omitempty"` + // If is a simplified condition expression (ergonomic alias for condition.expression) + // Mutually exclusive with Condition field + If string `yaml:"if,omitempty" json:"if,omitempty"` + // Condition defines when this step should execute Condition *ConditionDefinition `yaml:"condition,omitempty" json:"condition,omitempty"` @@ -1486,6 +1490,22 @@ func (s *StepDefinition) UnmarshalYAML(unmarshal func(interface{}) error) error s.hasExplicitID = true } + // Validate mutual exclusivity of 'if' and 'condition' BEFORE normalization + if s.If != "" && s.Condition != nil { + // If condition has any content (expression, then_steps, or else_steps), it's an error + if s.Condition.Expression != "" || len(s.Condition.ThenSteps) > 0 || len(s.Condition.ElseSteps) > 0 { + return fmt.Errorf("'if' and 'condition' fields are mutually exclusive - use only one") + } + } + + // Normalize 'if' field to 'condition.expression' + if s.If != "" { + if s.Condition == nil { + s.Condition = &ConditionDefinition{} + } + s.Condition.Expression = s.If + } + return nil } diff --git a/pkg/workflow/definition_test.go b/pkg/workflow/definition_test.go index d650a3a9..27c88581 100644 --- a/pkg/workflow/definition_test.go +++ b/pkg/workflow/definition_test.go @@ -880,3 +880,268 @@ func contains(s, substr string) bool { } return false } + +// TestIfFieldParsing tests that the 'if' field is correctly parsed from YAML +func TestIfFieldParsing(t *testing.T) { + tests := []struct { + name string + definition string + wantErr bool + validate func(t *testing.T, def *Definition) + }{ + { + name: "valid if field", + definition: ` +name: test-workflow +steps: + - id: check + type: llm + prompt: "Check something" + + - id: conditional_step + type: llm + if: "{{.steps.check.response}} == 'yes'" + prompt: "Do something" +`, + wantErr: false, + validate: func(t *testing.T, def *Definition) { + if len(def.Steps) < 2 { + t.Fatal("Expected at least 2 steps") + } + step := def.Steps[1] + if step.If != "{{.steps.check.response}} == 'yes'" { + t.Errorf("If field not parsed correctly, got %v", step.If) + } + // Check normalization: If should be copied to Condition.Expression + if step.Condition == nil { + t.Fatal("Condition should not be nil after normalization") + } + if step.Condition.Expression != step.If { + t.Errorf("Condition.Expression = %v, want %v", step.Condition.Expression, step.If) + } + }, + }, + { + name: "if field with simple expression", + definition: ` +name: test-workflow +steps: + - id: conditional_step + type: llm + if: "inputs.enabled" + prompt: "Do something" +`, + wantErr: false, + validate: func(t *testing.T, def *Definition) { + step := def.Steps[0] + if step.If != "inputs.enabled" { + t.Errorf("If field = %v, want 'inputs.enabled'", step.If) + } + if step.Condition == nil || step.Condition.Expression != "inputs.enabled" { + t.Error("If field not normalized to Condition.Expression") + } + }, + }, + { + name: "if field with boolean operators", + definition: ` +name: test-workflow +steps: + - id: conditional_step + type: llm + if: "inputs.enabled && !inputs.dry_run" + prompt: "Do something" +`, + wantErr: false, + validate: func(t *testing.T, def *Definition) { + step := def.Steps[0] + expectedIf := "inputs.enabled && !inputs.dry_run" + if step.If != expectedIf { + t.Errorf("If field = %v, want %v", step.If, expectedIf) + } + if step.Condition == nil || step.Condition.Expression != expectedIf { + t.Error("If field not normalized to Condition.Expression") + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + def, err := ParseDefinition([]byte(tt.definition)) + if (err != nil) != tt.wantErr { + t.Errorf("ParseDefinition() error = %v, wantErr %v", err, tt.wantErr) + return + } + if err == nil && tt.validate != nil { + tt.validate(t, def) + } + }) + } +} + +// TestIfFieldNormalization tests that 'if' is normalized to 'condition.expression' +func TestIfFieldNormalization(t *testing.T) { + tests := []struct { + name string + definition string + wantErr bool + expectedExpression string + }{ + { + name: "if normalizes to condition.expression", + definition: ` +name: test-workflow +steps: + - id: step1 + type: llm + if: "true" + prompt: "test" +`, + wantErr: false, + expectedExpression: "true", + }, + { + name: "if with template syntax", + definition: ` +name: test-workflow +steps: + - id: check + type: llm + prompt: "check" + - id: step2 + type: llm + if: "{{.steps.check.response}} == 'proceed'" + prompt: "test" +`, + wantErr: false, + expectedExpression: "{{.steps.check.response}} == 'proceed'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + def, err := ParseDefinition([]byte(tt.definition)) + if (err != nil) != tt.wantErr { + t.Errorf("ParseDefinition() error = %v, wantErr %v", err, tt.wantErr) + return + } + if err == nil { + // Find the step with 'if' field (not the first step in some tests) + var targetStep *StepDefinition + for i := range def.Steps { + if def.Steps[i].If != "" { + targetStep = &def.Steps[i] + break + } + } + if targetStep == nil { + t.Fatal("No step with 'if' field found") + } + if targetStep.Condition == nil { + t.Fatal("Condition should be set after normalization") + } + if targetStep.Condition.Expression != tt.expectedExpression { + t.Errorf("Condition.Expression = %v, want %v", + targetStep.Condition.Expression, tt.expectedExpression) + } + } + }) + } +} + +// TestIfAndConditionMutualExclusivity tests that 'if' and 'condition' cannot both be set +func TestIfAndConditionMutualExclusivity(t *testing.T) { + tests := []struct { + name string + definition string + wantErr bool + errMsg string + }{ + { + name: "both if and condition.expression set", + definition: ` +name: test-workflow +steps: + - id: step1 + type: llm + if: "inputs.enabled" + condition: + expression: "inputs.mode == 'strict'" + prompt: "test" +`, + wantErr: true, + errMsg: "mutually exclusive", + }, + { + name: "if and condition with then_steps", + definition: ` +name: test-workflow +steps: + - id: step1 + type: llm + if: "inputs.enabled" + condition: + expression: "inputs.enabled" + then_steps: ["step2"] + prompt: "test" + - id: step2 + type: llm + prompt: "test2" +`, + wantErr: true, + errMsg: "mutually exclusive", + }, + { + name: "only if field (valid)", + definition: ` +name: test-workflow +steps: + - id: step1 + type: llm + if: "inputs.enabled" + prompt: "test" +`, + wantErr: false, + }, + { + name: "only condition field (valid)", + definition: ` +name: test-workflow +steps: + - id: step1 + type: llm + condition: + expression: "inputs.enabled" + prompt: "test" +`, + wantErr: false, + }, + { + name: "neither if nor condition (valid)", + definition: ` +name: test-workflow +steps: + - id: step1 + type: llm + prompt: "test" +`, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := ParseDefinition([]byte(tt.definition)) + if (err != nil) != tt.wantErr { + t.Errorf("ParseDefinition() error = %v, wantErr %v", err, tt.wantErr) + return + } + if err != nil && tt.errMsg != "" { + if !contains(err.Error(), tt.errMsg) { + t.Errorf("ParseDefinition() error = %v, want error containing %q", err, tt.errMsg) + } + } + }) + } +} diff --git a/pkg/workflow/executor.go b/pkg/workflow/executor.go index 5212339e..0b274343 100644 --- a/pkg/workflow/executor.go +++ b/pkg/workflow/executor.go @@ -323,9 +323,13 @@ func (e *Executor) Execute(ctx context.Context, step *StepDefinition, workflowCo ) result.Status = StepStatusSkipped result.Output = map[string]interface{}{ - "content": "", - "skipped": true, - "reason": "condition evaluated to false", + "skipped": true, + "reason": "condition evaluated to false", + "stdout": "", + "stderr": "", + "exit_code": 0, + "content": "", + "status": "skipped", } result.CompletedAt = time.Now() result.Duration = result.CompletedAt.Sub(result.StartedAt) @@ -1654,18 +1658,31 @@ func copyWorkflowContext(ctx map[string]interface{}) map[string]interface{} { // - has(inputs.personas, "security") // - inputs.mode == "strict" // - inputs.count > 5 && inputs.enabled +// - {{.steps.check.stdout}} == "success" (template syntax) func (e *Executor) evaluateCondition(expr string, workflowContext map[string]interface{}) (bool, error) { // Build expression context from workflow context ctx := expression.BuildContext(workflowContext) + // Preprocess template expressions ({{...}}) before evaluation + // This allows conditions like: {{.steps.check.stdout}} == "success" + preprocessed, err := expression.PreprocessTemplate(expr, ctx) + if err != nil { + return false, fmt.Errorf("template preprocessing failed: %w", err) + } + + e.logger.Debug("condition preprocessed", + "original", expr, + "preprocessed", preprocessed, + ) + // Evaluate the expression - result, err := e.exprEval.Evaluate(expr, ctx) + result, err := e.exprEval.Evaluate(preprocessed, ctx) if err != nil { return false, err } e.logger.Debug("condition evaluated", - "expression", expr, + "expression", preprocessed, "result", result, ) diff --git a/pkg/workflow/executor_condition_test.go b/pkg/workflow/executor_condition_test.go index 8595062b..f3c93c64 100644 --- a/pkg/workflow/executor_condition_test.go +++ b/pkg/workflow/executor_condition_test.go @@ -257,9 +257,232 @@ func TestExecute_SkippedStepOutput(t *testing.T) { result, err := executor.Execute(context.Background(), step, workflowContext) require.NoError(t, err) - // Verify skipped step has proper output structure + // Verify skipped step has proper output structure matching spec FR4 assert.Equal(t, StepStatusSkipped, result.Status) + assert.Equal(t, true, result.Output["skipped"]) + assert.Equal(t, "condition evaluated to false", result.Output["reason"]) + assert.Equal(t, "", result.Output["stdout"]) + assert.Equal(t, "", result.Output["stderr"]) + assert.Equal(t, 0, result.Output["exit_code"]) assert.Equal(t, "", result.Output["content"]) + assert.Equal(t, "skipped", result.Output["status"]) +} + +// TestExecute_ConditionWithTemplateExpression tests Phase 3: template syntax in conditions +func TestExecute_ConditionWithTemplateExpression(t *testing.T) { + executor := NewExecutor(nil, &mockLLMProvider{response: "analysis complete"}) + + step := &StepDefinition{ + ID: "analyze", + Type: StepTypeLLM, + Condition: &ConditionDefinition{ + Expression: `{{.steps.check.stdout}} == "success"`, + }, + Prompt: "Analyze the data", + } + + workflowContext := map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{ + "stdout": "success", + "stderr": "", + }, + }, + } + + result, err := executor.Execute(context.Background(), step, workflowContext) + require.NoError(t, err) + + assert.Equal(t, StepStatusSuccess, result.Status) + assert.Contains(t, result.Output["response"], "analysis complete") +} + +// TestExecute_ConditionWithTemplateExpression_False tests skipped step with template +func TestExecute_ConditionWithTemplateExpression_False(t *testing.T) { + executor := NewExecutor(nil, &mockLLMProvider{response: "should not run"}) + + step := &StepDefinition{ + ID: "analyze", + Type: StepTypeLLM, + Condition: &ConditionDefinition{ + Expression: `{{.steps.check.stdout}} == "success"`, + }, + Prompt: "Analyze the data", + } + + workflowContext := map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{ + "stdout": "failed", + "stderr": "error occurred", + }, + }, + } + + result, err := executor.Execute(context.Background(), step, workflowContext) + require.NoError(t, err) + + assert.Equal(t, StepStatusSkipped, result.Status) + assert.Equal(t, true, result.Output["skipped"]) +} + +// TestExecute_SkippedStepReferencedByDownstream verifies downstream steps can reference skipped steps +func TestExecute_SkippedStepReferencedByDownstream(t *testing.T) { + executor := NewExecutor(nil, &mockLLMProvider{response: "using skipped output"}) + + // First step gets skipped + step1 := &StepDefinition{ + ID: "optional_step", + Type: StepTypeLLM, + Condition: &ConditionDefinition{ + Expression: `false`, // Always skip + }, + Prompt: "This won't run", + } + + workflowContext := map[string]interface{}{ + "inputs": map[string]interface{}{}, + } + + result1, err := executor.Execute(context.Background(), step1, workflowContext) + require.NoError(t, err) + assert.Equal(t, StepStatusSkipped, result1.Status) + + // Add skipped step output to context + workflowContext["steps"] = map[string]interface{}{ + "optional_step": result1.Output, + } + + // Second step references the skipped step using template syntax + step2 := &StepDefinition{ + ID: "next_step", + Type: StepTypeLLM, + Condition: &ConditionDefinition{ + Expression: `{{.steps.optional_step.status}} == "skipped"`, + }, + Prompt: "Handle skipped case", + } + + result2, err := executor.Execute(context.Background(), step2, workflowContext) + require.NoError(t, err) + + // This step should run because the condition evaluates to true + assert.Equal(t, StepStatusSuccess, result2.Status) + assert.Contains(t, result2.Output["response"], "using skipped output") +} + +// TestExecute_ChainedConditionsWithTemplates tests multiple conditions with template expressions +func TestExecute_ChainedConditionsWithTemplates(t *testing.T) { + executor := NewExecutor(nil, &mockLLMProvider{response: "final step"}) + + // First step succeeds + step1 := &StepDefinition{ + ID: "step1", + Type: StepTypeLLM, + Prompt: "First step", + } + + workflowContext := map[string]interface{}{ + "inputs": map[string]interface{}{}, + } + + result1, err := executor.Execute(context.Background(), step1, workflowContext) + require.NoError(t, err) + assert.Equal(t, StepStatusSuccess, result1.Status) + + // Second step checks first step's status + workflowContext["steps"] = map[string]interface{}{ + "step1": map[string]interface{}{ + "response": result1.Output["response"], + "status": "success", + }, + } + + step2 := &StepDefinition{ + ID: "step2", + Type: StepTypeLLM, + Condition: &ConditionDefinition{ + Expression: `{{.steps.step1.status}} == "success"`, + }, + Prompt: "Second step", + } + + result2, err := executor.Execute(context.Background(), step2, workflowContext) + require.NoError(t, err) + assert.Equal(t, StepStatusSuccess, result2.Status) + + // Third step checks both previous steps using complex condition + workflowContext["steps"].(map[string]interface{})["step2"] = map[string]interface{}{ + "response": result2.Output["response"], + "status": "success", + } + + step3 := &StepDefinition{ + ID: "step3", + Type: StepTypeLLM, + Condition: &ConditionDefinition{ + Expression: `{{.steps.step1.status}} == "success" && {{.steps.step2.status}} == "success"`, + }, + Prompt: "Final step", + } + + result3, err := executor.Execute(context.Background(), step3, workflowContext) + require.NoError(t, err) + assert.Equal(t, StepStatusSuccess, result3.Status) + assert.Contains(t, result3.Output["response"], "final step") +} + +// TestExecute_SkippedStepDoesNotTriggerOnError verifies skipped steps don't invoke error handlers +func TestExecute_SkippedStepDoesNotTriggerOnError(t *testing.T) { + executor := NewExecutor(nil, &mockLLMProvider{response: "should not run"}) + + step := &StepDefinition{ + ID: "conditional_step", + Type: StepTypeLLM, + Condition: &ConditionDefinition{ + Expression: `false`, // Always skip + }, + OnError: &ErrorHandlingDefinition{ + Strategy: ErrorStrategyFail, + }, + Prompt: "This won't run", + } + + workflowContext := map[string]interface{}{} + + result, err := executor.Execute(context.Background(), step, workflowContext) + require.NoError(t, err) // No error should be returned + + assert.Equal(t, StepStatusSkipped, result.Status) + assert.Empty(t, result.Error) // Error field should be empty for skipped steps assert.Equal(t, true, result.Output["skipped"]) - assert.NotEmpty(t, result.Output["reason"]) +} + +// TestExecute_ConditionWithNestedStepFields tests template expressions accessing nested fields +func TestExecute_ConditionWithNestedStepFields(t *testing.T) { + executor := NewExecutor(nil, &mockLLMProvider{response: "processing"}) + + step := &StepDefinition{ + ID: "process", + Type: StepTypeLLM, + Condition: &ConditionDefinition{ + Expression: `{{.steps.api_call.exit_code}} == 0`, + }, + Prompt: "Process the API response", + } + + workflowContext := map[string]interface{}{ + "steps": map[string]interface{}{ + "api_call": map[string]interface{}{ + "stdout": "API response data", + "stderr": "", + "exit_code": 0, + }, + }, + } + + result, err := executor.Execute(context.Background(), step, workflowContext) + require.NoError(t, err) + + assert.Equal(t, StepStatusSuccess, result.Status) } diff --git a/pkg/workflow/expression/evaluator.go b/pkg/workflow/expression/evaluator.go index 2758d56a..08968b84 100644 --- a/pkg/workflow/expression/evaluator.go +++ b/pkg/workflow/expression/evaluator.go @@ -70,11 +70,16 @@ func (e *Evaluator) Evaluate(expression string, ctx map[string]interface{}) (boo } } + // Handle nil as false in boolean context + if result == nil { + return false, nil + } + boolResult, ok := result.(bool) if !ok { return false, &errors.ValidationError{ Field: "expression", - Message: fmt.Sprintf("expression must return boolean, got %T (%v)", result, result), + Message: fmt.Sprintf("Condition must evaluate to boolean, got %T", result), Suggestion: "use comparison operators (==, !=, <, >, etc.) or boolean functions", } } diff --git a/pkg/workflow/expression/evaluator_test.go b/pkg/workflow/expression/evaluator_test.go index f3cc5333..f7cf114b 100644 --- a/pkg/workflow/expression/evaluator_test.go +++ b/pkg/workflow/expression/evaluator_test.go @@ -470,3 +470,105 @@ func TestEvaluator_LengthFunction(t *testing.T) { }) } } + +func TestEvaluator_BooleanEnforcement(t *testing.T) { + e := New() + + tests := []struct { + name string + expr string + ctx map[string]interface{} + want bool + wantErr bool + errMsg string + }{ + { + name: "nil evaluates to false", + expr: `inputs.value`, + ctx: map[string]interface{}{ + "inputs": map[string]interface{}{ + "value": nil, + }, + }, + want: false, + wantErr: false, + }, + { + name: "nil in comparison still works", + expr: `inputs.value == nil`, + ctx: map[string]interface{}{ + "inputs": map[string]interface{}{ + "value": nil, + }, + }, + want: true, + wantErr: false, + }, + { + name: "string literal produces error", + expr: `"string"`, + ctx: map[string]interface{}{}, + // This will fail at compile time with expr.AsBool() + wantErr: true, + errMsg: "expected bool", + }, + { + name: "integer literal produces error", + expr: `42`, + ctx: map[string]interface{}{}, + // This will fail at compile time with expr.AsBool() + wantErr: true, + errMsg: "expected bool", + }, + { + name: "valid boolean expression", + expr: `inputs.enabled == true`, + ctx: map[string]interface{}{ + "inputs": map[string]interface{}{ + "enabled": true, + }, + }, + want: true, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := e.Evaluate(tt.expr, tt.ctx) + + if tt.wantErr { + require.Error(t, err) + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg) + } + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} + +func TestEvaluator_NilHandling(t *testing.T) { + e := New() + + // Test that nil values in boolean context evaluate to false + ctx := map[string]interface{}{ + "inputs": map[string]interface{}{ + "nilValue": nil, + "trueValue": true, + "falseValue": false, + }, + } + + // Direct nil reference should evaluate to false (not error) + result, err := e.Evaluate(`inputs.nilValue`, ctx) + require.NoError(t, err) + assert.False(t, result, "nil should evaluate to false in boolean context") + + // Missing key should also evaluate to false + result, err = e.Evaluate(`inputs.missingKey`, ctx) + require.NoError(t, err) + assert.False(t, result, "missing key should evaluate to false in boolean context") +} diff --git a/pkg/workflow/expression/template.go b/pkg/workflow/expression/template.go new file mode 100644 index 00000000..4d3654cc --- /dev/null +++ b/pkg/workflow/expression/template.go @@ -0,0 +1,127 @@ +package expression + +import ( + "fmt" + "regexp" + "strconv" + "strings" +) + +// Template pattern matches {{...}} expressions +var templatePattern = regexp.MustCompile(`\{\{([^}]+)\}\}`) + +// PreprocessTemplate resolves Go template-style expressions ({{.steps.id.field}}) +// by replacing them with expr-lang compatible literals before evaluation. +// +// This function performs a single-pass resolution for security: +// - Finds all {{...}} patterns +// - Resolves each path from the context +// - Replaces with appropriate literal (quoted strings, numbers, booleans, etc.) +// +// Example: +// +// PreprocessTemplate(`{{.steps.check.status}} == "success"`, ctx) +// => `"success" == "success"` +// +// Returns the processed expression or an error if template syntax is invalid +// or if a referenced path cannot be resolved. +func PreprocessTemplate(expression string, ctx map[string]interface{}) (string, error) { + if expression == "" { + return expression, nil + } + + var lastErr error + result := templatePattern.ReplaceAllStringFunc(expression, func(match string) string { + // Extract path from {{...}} + path := strings.TrimSpace(match[2 : len(match)-2]) // Remove {{ and }} + + // Remove leading dot if present (.steps.id => steps.id) + path = strings.TrimPrefix(path, ".") + + // Resolve the value from context + value, err := resolvePath(path, ctx) + if err != nil { + lastErr = err + return match // Keep original on error + } + + // Convert value to expr-lang literal + literal := valueToLiteral(value) + return literal + }) + + if lastErr != nil { + return "", fmt.Errorf("template resolution failed: %w", lastErr) + } + + return result, nil +} + +// resolvePath resolves a dot-separated path in the context. +// Example: "steps.check.status" => ctx["steps"]["check"]["status"] +func resolvePath(path string, ctx map[string]interface{}) (interface{}, error) { + if path == "" { + return nil, fmt.Errorf("empty path") + } + + parts := strings.Split(path, ".") + var current interface{} = ctx + + for i, part := range parts { + part = strings.TrimSpace(part) + if part == "" { + return nil, fmt.Errorf("invalid path: empty segment at position %d", i) + } + + // Navigate into the current value + switch v := current.(type) { + case map[string]interface{}: + val, ok := v[part] + if !ok { + return nil, fmt.Errorf("path not found: %s (missing key '%s')", path, part) + } + current = val + default: + return nil, fmt.Errorf("path not found: %s (cannot index into %T at '%s')", path, current, part) + } + } + + return current, nil +} + +// valueToLiteral converts a Go value to an expr-lang literal string. +// - Strings are quoted and escaped +// - Numbers are rendered as-is +// - Booleans are rendered as true/false +// - nil becomes nil +// - Other types use string representation (best effort) +func valueToLiteral(value interface{}) string { + switch v := value.(type) { + case nil: + return "nil" + case bool: + return strconv.FormatBool(v) + case int: + return strconv.Itoa(v) + case int64: + return strconv.FormatInt(v, 10) + case int32: + return strconv.FormatInt(int64(v), 10) + case float64: + return strconv.FormatFloat(v, 'f', -1, 64) + case float32: + return strconv.FormatFloat(float64(v), 'f', -1, 32) + case string: + // Escape quotes and backslashes in string + escaped := strings.ReplaceAll(v, "\\", "\\\\") + escaped = strings.ReplaceAll(escaped, "\"", "\\\"") + return fmt.Sprintf(`"%s"`, escaped) + default: + // For arrays, maps, and other types, convert to string and quote + // This is a fallback - most cases should be primitives + str := fmt.Sprintf("%v", v) + escaped := strings.ReplaceAll(str, "\\", "\\\\") + escaped = strings.ReplaceAll(escaped, "\"", "\\\"") + return fmt.Sprintf(`"%s"`, escaped) + } +} diff --git a/pkg/workflow/expression/template_test.go b/pkg/workflow/expression/template_test.go new file mode 100644 index 00000000..d699af26 --- /dev/null +++ b/pkg/workflow/expression/template_test.go @@ -0,0 +1,324 @@ +package expression + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPreprocessTemplate(t *testing.T) { + tests := []struct { + name string + expression string + ctx map[string]interface{} + want string + wantErr bool + errMsg string + }{ + { + name: "no template markers", + expression: `steps.check.status == "success"`, + ctx: map[string]interface{}{}, + want: `steps.check.status == "success"`, + wantErr: false, + }, + { + name: "empty expression", + expression: "", + ctx: map[string]interface{}{}, + want: "", + wantErr: false, + }, + { + name: "string value replacement", + expression: `{{.steps.check.status}} == "success"`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{ + "status": "success", + }, + }, + }, + want: `"success" == "success"`, + wantErr: false, + }, + { + name: "string value without leading dot", + expression: `{{steps.check.status}} == "success"`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{ + "status": "success", + }, + }, + }, + want: `"success" == "success"`, + wantErr: false, + }, + { + name: "integer value replacement", + expression: `{{.steps.analyze.score}} > 80`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "analyze": map[string]interface{}{ + "score": 95, + }, + }, + }, + want: `95 > 80`, + wantErr: false, + }, + { + name: "boolean value replacement", + expression: `{{.steps.validate.passed}} == true`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "validate": map[string]interface{}{ + "passed": true, + }, + }, + }, + want: `true == true`, + wantErr: false, + }, + { + name: "nil value replacement", + expression: `{{.steps.optional.value}} == nil`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "optional": map[string]interface{}{ + "value": nil, + }, + }, + }, + want: `nil == nil`, + wantErr: false, + }, + { + name: "multiple templates", + expression: `{{.steps.a.x}} > {{.steps.b.y}}`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "a": map[string]interface{}{"x": 10}, + "b": map[string]interface{}{"y": 5}, + }, + }, + want: `10 > 5`, + wantErr: false, + }, + { + name: "string with quotes needs escaping", + expression: `{{.steps.check.message}} == "ok"`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{ + "message": `He said "hello"`, + }, + }, + }, + want: `"He said \"hello\"" == "ok"`, + wantErr: false, + }, + { + name: "string with backslashes needs escaping", + expression: `{{.steps.check.path}}`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{ + "path": `C:\Users\test`, + }, + }, + }, + want: `"C:\\Users\\test"`, + wantErr: false, + }, + { + name: "path not found error", + expression: `{{.steps.missing.value}} == true`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{}, + }, + }, + wantErr: true, + errMsg: "path not found", + }, + { + name: "nested path missing key", + expression: `{{.steps.check.missing}} == true`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{ + "status": "success", + }, + }, + }, + wantErr: true, + errMsg: "missing key 'missing'", + }, + { + name: "cannot index into non-map", + expression: `{{.steps.check.status.invalid}} == true`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{ + "status": "success", + }, + }, + }, + wantErr: true, + errMsg: "cannot index into string", + }, + { + name: "float value", + expression: `{{.steps.measure.value}} > 3.14`, + ctx: map[string]interface{}{ + "steps": map[string]interface{}{ + "measure": map[string]interface{}{ + "value": 3.14159, + }, + }, + }, + want: `3.14159 > 3.14`, + wantErr: false, + }, + { + name: "inputs reference", + expression: `{{.inputs.name}} == "test"`, + ctx: map[string]interface{}{ + "inputs": map[string]interface{}{ + "name": "test", + }, + }, + want: `"test" == "test"`, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := PreprocessTemplate(tt.expression, tt.ctx) + + if tt.wantErr { + require.Error(t, err) + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg) + } + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} + +func TestResolvePath(t *testing.T) { + ctx := map[string]interface{}{ + "steps": map[string]interface{}{ + "check": map[string]interface{}{ + "status": "success", + "code": 0, + }, + }, + "inputs": map[string]interface{}{ + "name": "test", + }, + } + + tests := []struct { + name string + path string + want interface{} + wantErr bool + errMsg string + }{ + { + name: "simple path", + path: "inputs.name", + want: "test", + wantErr: false, + }, + { + name: "nested path", + path: "steps.check.status", + want: "success", + wantErr: false, + }, + { + name: "integer value", + path: "steps.check.code", + want: 0, + wantErr: false, + }, + { + name: "map value", + path: "steps.check", + want: map[string]interface{}{"status": "success", "code": 0}, + wantErr: false, + }, + { + name: "empty path", + path: "", + wantErr: true, + errMsg: "empty path", + }, + { + name: "missing key", + path: "steps.missing", + wantErr: true, + errMsg: "path not found", + }, + { + name: "path with spaces", + path: " steps . check . status ", + want: "success", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := resolvePath(tt.path, ctx) + + if tt.wantErr { + require.Error(t, err) + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg) + } + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} + +func TestValueToLiteral(t *testing.T) { + tests := []struct { + name string + value interface{} + want string + }{ + {name: "nil", value: nil, want: "nil"}, + {name: "true", value: true, want: "true"}, + {name: "false", value: false, want: "false"}, + {name: "int", value: 42, want: "42"}, + {name: "int64", value: int64(42), want: "42"}, + {name: "int32", value: int32(42), want: "42"}, + {name: "float64", value: float64(3.14), want: "3.14"}, + {name: "float32", value: float32(3.14), want: "3.14"}, + {name: "string", value: "hello", want: `"hello"`}, + {name: "empty string", value: "", want: `""`}, + {name: "string with quotes", value: `He said "hello"`, want: `"He said \"hello\""`}, + {name: "string with backslash", value: `C:\Users`, want: `"C:\\Users"`}, + {name: "string with both", value: `Path "C:\test"`, want: `"Path \"C:\\test\""`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := valueToLiteral(tt.value) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/workflow/expression/validate.go b/pkg/workflow/expression/validate.go new file mode 100644 index 00000000..7dc0111f --- /dev/null +++ b/pkg/workflow/expression/validate.go @@ -0,0 +1,97 @@ +package expression + +import ( + "fmt" + "regexp" + "strings" +) + +// Step reference patterns to extract step IDs from expressions +var ( + // Matches {{.steps.id.field}} or {{steps.id.field}} + templateStepPattern = regexp.MustCompile(`\{\{\.?steps\.([^.}\s]+)`) + // Matches steps.id.field (expr-lang syntax) + // Allow alphanumeric, underscore, and hyphen in step IDs + exprStepPattern = regexp.MustCompile(`\bsteps\.([a-zA-Z_][a-zA-Z0-9_-]*)`) +) + +// ValidateStepReferences validates that all step IDs referenced in an expression exist. +// It extracts step IDs from both template syntax ({{.steps.id}}) and expr-lang syntax (steps.id). +// Returns an error if any referenced step ID is not in the known steps list. +// +// Parameters: +// - expression: The expression to validate +// - knownStepIDs: List of valid step IDs in the workflow +// +// Example: +// +// err := ValidateStepReferences(`steps.check.status == "success"`, []string{"check", "build"}) +// // Returns nil (check exists) +// +// err := ValidateStepReferences(`steps.missing.status == "success"`, []string{"check", "build"}) +// // Returns error (missing not in known steps) +func ValidateStepReferences(expression string, knownStepIDs []string) error { + if expression == "" { + return nil + } + + // Extract all referenced step IDs + referencedSteps := extractStepReferences(expression) + if len(referencedSteps) == 0 { + return nil // No step references to validate + } + + // Create a set of known step IDs for fast lookup + knownSteps := make(map[string]bool) + for _, id := range knownStepIDs { + knownSteps[id] = true + } + + // Check each referenced step + var invalidSteps []string + for _, stepID := range referencedSteps { + if !knownSteps[stepID] { + invalidSteps = append(invalidSteps, stepID) + } + } + + if len(invalidSteps) > 0 { + return fmt.Errorf( + "expression references unknown step(s): %s (known steps: %s)", + strings.Join(invalidSteps, ", "), + strings.Join(knownStepIDs, ", "), + ) + } + + return nil +} + +// extractStepReferences extracts all unique step IDs referenced in an expression. +// It checks both template syntax ({{.steps.id}}) and expr-lang syntax (steps.id). +func extractStepReferences(expression string) []string { + stepSet := make(map[string]bool) + + // Extract from template patterns: {{.steps.id}} or {{steps.id}} + templateMatches := templateStepPattern.FindAllStringSubmatch(expression, -1) + for _, match := range templateMatches { + if len(match) > 1 { + stepSet[match[1]] = true + } + } + + // Extract from expr-lang patterns: steps.id + exprMatches := exprStepPattern.FindAllStringSubmatch(expression, -1) + for _, match := range exprMatches { + if len(match) > 1 { + stepSet[match[1]] = true + } + } + + // Convert set to slice + steps := make([]string, 0, len(stepSet)) + for step := range stepSet { + steps = append(steps, step) + } + + return steps +} diff --git a/pkg/workflow/expression/validate_test.go b/pkg/workflow/expression/validate_test.go new file mode 100644 index 00000000..99029d1e --- /dev/null +++ b/pkg/workflow/expression/validate_test.go @@ -0,0 +1,196 @@ +package expression + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateStepReferences(t *testing.T) { + knownSteps := []string{"check", "build", "test", "deploy"} + + tests := []struct { + name string + expression string + wantErr bool + errMsg string + }{ + { + name: "valid step reference in expr syntax", + expression: `steps.check.status == "success"`, + wantErr: false, + }, + { + name: "valid step reference in template syntax with dot", + expression: `{{.steps.check.status}} == "success"`, + wantErr: false, + }, + { + name: "valid step reference in template syntax without dot", + expression: `{{steps.build.result}} == "ok"`, + wantErr: false, + }, + { + name: "multiple valid step references", + expression: `steps.check.status == "success" && steps.build.status == "success"`, + wantErr: false, + }, + { + name: "mixed template and expr syntax", + expression: `{{.steps.check.status}} == "success" && steps.build.ok`, + wantErr: false, + }, + { + name: "no step references", + expression: `inputs.mode == "strict"`, + wantErr: false, + }, + { + name: "empty expression", + expression: "", + wantErr: false, + }, + { + name: "invalid step reference", + expression: `steps.missing.status == "success"`, + wantErr: true, + errMsg: "unknown step(s): missing", + }, + { + name: "multiple invalid step references", + expression: `steps.missing.status == "success" && steps.invalid.ok`, + wantErr: true, + errMsg: "unknown step(s)", + }, + { + name: "mix of valid and invalid", + expression: `steps.check.status == "success" && steps.missing.ok`, + wantErr: true, + errMsg: "unknown step(s): missing", + }, + { + name: "invalid step in template", + expression: `{{.steps.missing.value}} == true`, + wantErr: true, + errMsg: "unknown step(s): missing", + }, + { + name: "step-like word but not a reference", + expression: `inputs.steps == "value"`, + wantErr: false, + }, + { + name: "complex expression with valid steps", + expression: `(steps.check.status == "success" && steps.build.status == "success") || steps.test.skip`, + wantErr: false, + }, + { + name: "step ID with hyphens", + expression: `steps.build-step.status == "success"`, + wantErr: true, + errMsg: "unknown step(s): build-step", + }, + { + name: "step ID with underscores", + expression: `steps.build_step.status == "success"`, + wantErr: true, + errMsg: "unknown step(s): build_step", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateStepReferences(tt.expression, knownSteps) + + if tt.wantErr { + require.Error(t, err) + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg) + } + } else { + require.NoError(t, err) + } + }) + } +} + +func TestValidateStepReferences_EmptyKnownSteps(t *testing.T) { + // When there are no known steps, any step reference should be invalid + err := ValidateStepReferences(`steps.check.status == "success"`, []string{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown step(s): check") +} + +func TestExtractStepReferences(t *testing.T) { + tests := []struct { + name string + expression string + want []string + }{ + { + name: "single expr syntax", + expression: `steps.check.status == "success"`, + want: []string{"check"}, + }, + { + name: "single template syntax with dot", + expression: `{{.steps.check.status}}`, + want: []string{"check"}, + }, + { + name: "single template syntax without dot", + expression: `{{steps.check.status}}`, + want: []string{"check"}, + }, + { + name: "multiple different steps", + expression: `steps.check.status == "success" && steps.build.ok`, + want: []string{"check", "build"}, + }, + { + name: "duplicate step references", + expression: `steps.check.status == "success" && steps.check.code == 0`, + want: []string{"check"}, // Deduplicated + }, + { + name: "mixed syntax", + expression: `{{.steps.check.status}} && steps.build.ok`, + want: []string{"check", "build"}, + }, + { + name: "no step references", + expression: `inputs.mode == "strict"`, + want: []string{}, + }, + { + name: "empty expression", + expression: "", + want: []string{}, + }, + { + name: "step ID with underscores", + expression: `steps.build_step.status`, + want: []string{"build_step"}, + }, + { + name: "step ID with hyphens", + expression: `steps.build-step.status`, + want: []string{"build-step"}, + }, + { + name: "complex expression", + expression: `(steps.a.x > 5 && steps.b.y < 10) || steps.c.z == true`, + want: []string{"a", "b", "c"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractStepReferences(tt.expression) + + // Sort both slices for comparison since order doesn't matter + assert.ElementsMatch(t, tt.want, got) + }) + } +}