Skip to content

Commit b8f47f1

Browse files
committed
Add step reference validation for conditions
Validate that step references in conditions (steps.foo) point to steps that exist and come before the current step. This catches forward references, self-references, and typos at validation time. Compile (compile.go): - Add stepRefFinder AST visitor to find steps.X patterns - Add ExtractStepRefs() to extract step names from expressions - Handle both dot notation (steps.foo) and bracket notation (steps["foo"]) Validation (validate.go): - Add validateStepRefs() to check step references - Detect unknown steps, forward references, and self-references - Expand aliases to check step refs hidden in alias definitions - Distinct error messages for each case Documentation (README.md): - Add "Step references" paragraph in Condition Expressions section Tests: - Add TestExtractStepRefs with 8 cases (dot, bracket, string literal, dedup) - Add TestValidate_StepReferences with 6 cases (valid, unknown, forward, alias, self, multiple)
1 parent b7fead3 commit b8f47f1

8 files changed

Lines changed: 434 additions & 9 deletions

File tree

‎README.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1778,6 +1778,8 @@ Aliases are expanded at compile time, so `"found && is_owner"` becomes `"(steps.
17781778

17791779
**Division safety:** Conditions with dynamic divisors must use `divOr(a, b, fallback)` instead of `a / b`. This prevents runtime panics from division by zero. Division by literal non-zero values (e.g., `x / 2`) is allowed. Violations are caught at workflow compile time.
17801780

1781+
**Step references:** Conditions using `steps.X` are validated at compile time. Steps can only reference results from earlier steps - forward references and self-references are errors. Unknown step names are also caught.
1782+
17811783
### Error Handling
17821784

17831785
Control step failure behavior with `on_error`:

‎TESTS.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -564,6 +564,7 @@ Run `make test-cover` for current coverage statistics.
564564
- **TestExprFunc_isValidPublicID**: TestExprFunc_isValidPublicID tests the isValidPublicID expr function.
565565
- **TestExprFuncs_InConditions**: TestExprFuncs_InConditions tests that common functions from tmpl.ExprFuncs
566566
- **TestValidateDivisions**: TestValidateDivisions tests static validation of division operations
567+
- **TestExtractStepRefs**: TestExtractStepRefs tests step reference extraction from expressions
567568

568569
### config_test.go
569570

@@ -699,6 +700,7 @@ Run `make test-cover` for current coverage statistics.
699700
- **TestValidate_HTTPCallRetry**: TestValidate_HTTPCallRetry verifies httpcall retry configuration validation
700701
- **TestValidate_HTTPCallRetryValid**: TestValidate_HTTPCallRetryValid verifies valid httpcall retry configuration passes
701702
- **TestValidate_DivisionSafety**: TestValidate_DivisionSafety tests that unsafe divisions are caught during validation
703+
- **TestValidate_StepReferences**: TestValidate_StepReferences tests that step references are validated
702704

703705

704706
---

‎internal/config/static_template.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,5 +193,3 @@ func filterDynamicPaths(paths []string) []string {
193193
}
194194
return dynamic
195195
}
196-
197-

‎internal/tmpl/engine_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1778,7 +1778,7 @@ func TestPrivateIDFunc(t *testing.T) {
17781778
{"wrong namespace prefix", "order", validPublicID, true, "invalid prefix"},
17791779
{"invalid base62 characters", "user", "usr_!@#$%^&*()", true, "invalid"},
17801780
{"empty public ID", "user", "", true, "invalid prefix"},
1781-
{"prefix only error", "user", "usr_", true, "invalid length"}, // Empty encoded part rejected
1781+
{"prefix only error", "user", "usr_", true, "invalid length"}, // Empty encoded part rejected
17821782
{"missing prefix separator", "user", "usr00000000001", true, "invalid prefix"},
17831783
{"too short encoded part", "user", "usr_abc", true, "invalid length"}, // Must be exactly 11 chars
17841784
{"no prefix configured namespace", "order", "ord_" + strings.Repeat("0", 11), false, ""},

‎internal/workflow/compile.go‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -684,6 +684,64 @@ func ValidateDivisions(exprStr string) error {
684684
return nil
685685
}
686686

687+
// stepRefFinder is a visitor that collects step names referenced via steps.X patterns.
688+
type stepRefFinder struct {
689+
refs []string
690+
seen map[string]bool
691+
}
692+
693+
func (f *stepRefFinder) Visit(node *ast.Node) {
694+
if node == nil || *node == nil {
695+
return
696+
}
697+
698+
// Look for member access patterns: steps.foo or steps["foo"]
699+
member, ok := (*node).(*ast.MemberNode)
700+
if !ok {
701+
return
702+
}
703+
704+
// Check if the object is the "steps" identifier
705+
ident, ok := member.Node.(*ast.IdentifierNode)
706+
if !ok || ident.Value != "steps" {
707+
return
708+
}
709+
710+
// Extract the step name from the property
711+
var stepName string
712+
switch prop := member.Property.(type) {
713+
case *ast.StringNode:
714+
// steps["foo"]
715+
stepName = prop.Value
716+
case *ast.IdentifierNode:
717+
// steps.foo
718+
stepName = prop.Value
719+
default:
720+
// Dynamic access like steps[variable] - can't validate statically
721+
return
722+
}
723+
724+
if stepName != "" && !f.seen[stepName] {
725+
f.refs = append(f.refs, stepName)
726+
f.seen[stepName] = true
727+
}
728+
}
729+
730+
// ExtractStepRefs finds step names referenced in an expression via steps.X patterns.
731+
// Returns a list of step names that are accessed (e.g., "fetch" from "steps.fetch.count > 0").
732+
func ExtractStepRefs(exprStr string) ([]string, error) {
733+
tree, err := parser.Parse(exprStr)
734+
if err != nil {
735+
return nil, err
736+
}
737+
738+
finder := &stepRefFinder{
739+
seen: make(map[string]bool),
740+
}
741+
ast.Walk(&tree.Node, finder)
742+
return finder.refs, nil
743+
}
744+
687745
// EvalCondition evaluates a compiled condition against an environment.
688746
func EvalCondition(prog *vm.Program, env map[string]any) (bool, error) {
689747
result, err := vm.Run(prog, env)

‎internal/workflow/compile_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1546,3 +1546,79 @@ func TestValidateDivisions(t *testing.T) {
15461546
}
15471547
})
15481548
}
1549+
1550+
// TestExtractStepRefs tests step reference extraction from expressions
1551+
func TestExtractStepRefs(t *testing.T) {
1552+
tests := []struct {
1553+
name string
1554+
expr string
1555+
expected []string
1556+
}{
1557+
{
1558+
name: "simple step reference",
1559+
expr: "steps.fetch.count > 0",
1560+
expected: []string{"fetch"},
1561+
},
1562+
{
1563+
name: "multiple step references",
1564+
expr: "steps.auth.valid && steps.fetch.count > 0",
1565+
expected: []string{"auth", "fetch"},
1566+
},
1567+
{
1568+
name: "nested property access",
1569+
expr: "steps.fetch.row.user_id == steps.auth.row.id",
1570+
expected: []string{"fetch", "auth"},
1571+
},
1572+
{
1573+
name: "no step references",
1574+
expr: "trigger.params.id != \"\"",
1575+
expected: []string{},
1576+
},
1577+
{
1578+
name: "bracket notation",
1579+
expr: `steps["my-step"].count > 0`,
1580+
expected: []string{"my-step"},
1581+
},
1582+
{
1583+
name: "mixed dot and bracket",
1584+
expr: `steps.fetch.count > 0 && steps["other"].valid`,
1585+
expected: []string{"fetch", "other"},
1586+
},
1587+
{
1588+
name: "steps in string literal not extracted",
1589+
expr: `name == "steps.foo"`,
1590+
expected: []string{},
1591+
},
1592+
{
1593+
name: "duplicate refs deduplicated",
1594+
expr: "steps.fetch.count > 0 && steps.fetch.valid",
1595+
expected: []string{"fetch"},
1596+
},
1597+
}
1598+
1599+
for _, tt := range tests {
1600+
t.Run(tt.name, func(t *testing.T) {
1601+
refs, err := ExtractStepRefs(tt.expr)
1602+
if err != nil {
1603+
t.Fatalf("unexpected error: %v", err)
1604+
}
1605+
1606+
// Check length
1607+
if len(refs) != len(tt.expected) {
1608+
t.Errorf("expected %d refs, got %d: %v", len(tt.expected), len(refs), refs)
1609+
return
1610+
}
1611+
1612+
// Check each expected ref is present
1613+
refSet := make(map[string]bool)
1614+
for _, r := range refs {
1615+
refSet[r] = true
1616+
}
1617+
for _, exp := range tt.expected {
1618+
if !refSet[exp] {
1619+
t.Errorf("expected ref %q not found in %v", exp, refs)
1620+
}
1621+
}
1622+
})
1623+
}
1624+
}

‎internal/workflow/validate.go‎

Lines changed: 63 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ func Validate(cfg *WorkflowConfig, ctx *ValidationContext) *ValidationResult {
9595
stepNames[step.Name] = i
9696
}
9797

98-
validateStep(&step, stepPrefix, stepNames, ctx, r)
98+
validateStep(&step, stepPrefix, i, stepNames, cfg.Conditions, ctx, r)
9999

100100
// Track response steps
101101
if step.IsResponse() {
@@ -285,11 +285,14 @@ func validateRateLimit(cfg *RateLimitRefConfig, prefix string, ctx *ValidationCo
285285
}
286286
}
287287

288-
func validateStep(cfg *StepConfig, prefix string, stepNames map[string]int, ctx *ValidationContext, r *ValidationResult) {
289-
// Validate condition syntax
288+
func validateStep(cfg *StepConfig, prefix string, stepIndex int, stepNames map[string]int, aliases map[string]string, ctx *ValidationContext, r *ValidationResult) {
289+
// Validate condition syntax and step references
290290
if cfg.Condition != "" {
291291
if err := validateExprSyntax(cfg.Condition); err != nil {
292292
r.addError("%s.condition: invalid expression: %v", prefix, err)
293+
} else {
294+
// Validate step references in condition
295+
validateStepRefs(cfg.Condition, prefix+".condition", stepIndex, stepNames, aliases, r)
293296
}
294297
}
295298

@@ -335,7 +338,7 @@ func validateStep(cfg *StepConfig, prefix string, stepNames map[string]int, ctx
335338
case "response":
336339
validateResponseStep(cfg, prefix, r)
337340
case "block":
338-
validateBlockStep(cfg, prefix, stepNames, ctx, r)
341+
validateBlockStep(cfg, prefix, stepNames, aliases, ctx, r)
339342
}
340343
}
341344

@@ -402,7 +405,7 @@ func validateResponseStep(cfg *StepConfig, prefix string, r *ValidationResult) {
402405
}
403406
}
404407

405-
func validateBlockStep(cfg *StepConfig, prefix string, parentStepNames map[string]int, ctx *ValidationContext, r *ValidationResult) {
408+
func validateBlockStep(cfg *StepConfig, prefix string, parentStepNames map[string]int, aliases map[string]string, ctx *ValidationContext, r *ValidationResult) {
406409
// Validate iterate if present
407410
if cfg.Iterate != nil {
408411
iterPrefix := prefix + ".iterate"
@@ -445,7 +448,7 @@ func validateBlockStep(cfg *StepConfig, prefix string, parentStepNames map[strin
445448
continue
446449
}
447450

448-
validateStep(&step, stepPrefix, nestedNames, ctx, r)
451+
validateStep(&step, stepPrefix, i, nestedNames, aliases, ctx, r)
449452
}
450453

451454
// Validate outputs reference valid step names or special values
@@ -482,6 +485,60 @@ func validateExprSyntax(exprStr string) error {
482485
return nil
483486
}
484487

488+
// validateStepRefs validates step references in an expression.
489+
// Checks that referenced steps exist and come before the current step.
490+
func validateStepRefs(exprStr, prefix string, currentStepIndex int, stepNames map[string]int, aliases map[string]string, r *ValidationResult) {
491+
// Collect all step refs from the expression and any aliases it uses
492+
allRefs := make(map[string]bool)
493+
494+
// Extract direct step refs from expression
495+
refs, err := ExtractStepRefs(exprStr)
496+
if err != nil {
497+
// Syntax errors already caught by validateExprSyntax
498+
return
499+
}
500+
for _, ref := range refs {
501+
allRefs[ref] = true
502+
}
503+
504+
// Find aliases used in expression and extract their step refs too
505+
if len(aliases) > 0 {
506+
aliasNames := make(map[string]bool, len(aliases))
507+
for name := range aliases {
508+
aliasNames[name] = true
509+
}
510+
511+
usedAliases, err := extractAliasRefs(exprStr, aliasNames)
512+
if err != nil {
513+
return
514+
}
515+
516+
// For each used alias, extract step refs from its source
517+
for _, aliasName := range usedAliases {
518+
aliasSource := aliases[aliasName]
519+
aliasRefs, err := ExtractStepRefs(aliasSource)
520+
if err != nil {
521+
continue
522+
}
523+
for _, ref := range aliasRefs {
524+
allRefs[ref] = true
525+
}
526+
}
527+
}
528+
529+
// Validate each step reference
530+
for stepRef := range allRefs {
531+
refIndex, exists := stepNames[stepRef]
532+
if !exists {
533+
r.addError("%s: references unknown step '%s'", prefix, stepRef)
534+
} else if refIndex == currentStepIndex {
535+
r.addError("%s: step cannot reference itself ('%s')", prefix, stepRef)
536+
} else if refIndex > currentStepIndex {
537+
r.addError("%s: references step '%s' which comes after current step (forward reference)", prefix, stepRef)
538+
}
539+
}
540+
}
541+
485542
// pathParamRegex matches path parameters like {id} or {user_id}
486543
var pathParamRegex = regexp.MustCompile(`\{([a-zA-Z_][a-zA-Z0-9_]*)\}`)
487544

0 commit comments

Comments
 (0)