diff --git a/internal/formula/condition.go b/internal/formula/condition.go index 2ab9e5fb..b2819063 100644 --- a/internal/formula/condition.go +++ b/internal/formula/condition.go @@ -114,6 +114,9 @@ var ( // steps.complete >= 3 stepsStatPattern = regexp.MustCompile(`^steps\.(\w+)\s*([=!<>]+)\s*(\d+)$`) + + // children(x).count(...) >= 3 (trailing comparison) + countComparePattern = regexp.MustCompile(`\s*([=!<>]+)\s*(\d+)$`) ) // ParseCondition parses a condition string into a Condition struct. @@ -163,8 +166,7 @@ func ParseCondition(expr string) (*Condition, error) { } // Handle count comparison: children(x).count(...) >= 3 if m[5] != "" { - countMatch := regexp.MustCompile(`\s*([=!<>]+)\s*(\d+)$`).FindStringSubmatch(m[5]) - if countMatch != nil { + if countMatch := countComparePattern.FindStringSubmatch(m[5]); countMatch != nil { cond.AggregateFunc = "count" cond.Operator = Operator(countMatch[1]) cond.Value = countMatch[2] @@ -317,6 +319,14 @@ func (c *Condition) evaluateAggregate(ctx *ConditionContext) (*ConditionResult, // Apply the aggregate function switch c.AggregateFunc { case "all": + // Empty set: "all children complete" with no children is false + // (avoids gates passing prematurely before children are created) + if len(steps) == 0 { + return &ConditionResult{ + Satisfied: false, + Reason: fmt.Sprintf("no %s to evaluate", c.AggregateOver), + }, nil + } for _, s := range steps { satisfied, _ := matchStep(s, c.Field, c.Operator, c.Value) if !satisfied { @@ -328,7 +338,7 @@ func (c *Condition) evaluateAggregate(ctx *ConditionContext) (*ConditionResult, } return &ConditionResult{ Satisfied: true, - Reason: fmt.Sprintf("all %d steps match", len(steps)), + Reason: fmt.Sprintf("all %d %s match", len(steps), c.AggregateOver), }, nil case "any": @@ -361,7 +371,10 @@ func (c *Condition) evaluateAggregate(ctx *ConditionContext) (*ConditionResult, } } } - expected, _ := strconv.Atoi(c.Value) + expected, err := strconv.Atoi(c.Value) + if err != nil { + return nil, fmt.Errorf("count comparison requires integer value, got %q: %w", c.Value, err) + } satisfied, reason := compareInt(count, c.Operator, expected) return &ConditionResult{ Satisfied: satisfied, @@ -428,6 +441,31 @@ func getNestedValue(m map[string]interface{}, path string) interface{} { } func compare(actual interface{}, op Operator, expected string) (bool, string) { + // Handle nil values explicitly + if actual == nil { + switch op { + case OpEqual: + // nil == "" or nil == "nil" are both false unless expected is literally empty + satisfied := expected == "" + return satisfied, fmt.Sprintf("nil %s %q: %v", op, expected, satisfied) + case OpNotEqual: + satisfied := expected != "" + return satisfied, fmt.Sprintf("nil %s %q: %v", op, expected, satisfied) + default: + return false, fmt.Sprintf("nil cannot be compared with %s", op) + } + } + + // Handle bool values (from JSON unmarshaling) + if b, ok := actual.(bool); ok { + actualStr := strconv.FormatBool(b) + satisfied := actualStr == expected + if op == OpNotEqual { + satisfied = actualStr != expected + } + return satisfied, fmt.Sprintf("%v %s %q: %v", b, op, expected, satisfied) + } + actualStr := fmt.Sprintf("%v", actual) switch op { @@ -491,6 +529,10 @@ func compareFloat(actual float64, op Operator, expected float64) (bool, string) func compareString(actual string, op Operator, expected string) (bool, string) { var satisfied bool switch op { + case OpEqual: + satisfied = actual == expected + case OpNotEqual: + satisfied = actual != expected case OpGreater: satisfied = actual > expected case OpGreaterEqual: diff --git a/internal/formula/condition_test.go b/internal/formula/condition_test.go index 0b1f8d28..881921f5 100644 --- a/internal/formula/condition_test.go +++ b/internal/formula/condition_test.go @@ -435,3 +435,95 @@ func TestCompareOperators(t *testing.T) { }) } } + +func TestEvaluateCondition_EmptyChildren(t *testing.T) { + ctx := &ConditionContext{ + CurrentStep: "parent", + Steps: map[string]*StepState{ + "parent": { + ID: "parent", + Status: "in_progress", + Children: []*StepState{}, // Empty children + }, + }, + } + + // "all children complete" with no children should be false (not vacuous truth) + result, err := EvaluateCondition("children(parent).all(status == 'complete')", ctx) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.Satisfied { + t.Errorf("empty children 'all' should be false, got true") + } +} + +func TestEvaluateCondition_NilOutput(t *testing.T) { + ctx := &ConditionContext{ + CurrentStep: "test", + Steps: map[string]*StepState{ + "test": { + ID: "test", + Status: "complete", + Output: nil, // No output + }, + }, + } + + // Comparing nil output field should not panic + result, err := EvaluateCondition("test.output.missing == 'value'", ctx) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.Satisfied { + t.Errorf("nil field comparison should be false") + } +} + +func TestEvaluateCondition_BoolOutput(t *testing.T) { + ctx := &ConditionContext{ + CurrentStep: "test", + Steps: map[string]*StepState{ + "test": { + ID: "test", + Status: "complete", + Output: map[string]interface{}{ + "approved": true, // actual bool, not string + }, + }, + }, + } + + result, err := EvaluateCondition("test.output.approved == true", ctx) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !result.Satisfied { + t.Errorf("bool true should match 'true', got: %s", result.Reason) + } +} + +func TestEvaluateCondition_CountError(t *testing.T) { + ctx := &ConditionContext{ + Steps: map[string]*StepState{ + "s1": {ID: "s1", Status: "complete"}, + }, + } + + // Directly construct a condition with invalid count value + // (regex validation normally prevents this, but test defensive code) + cond := &Condition{ + Raw: "steps.complete >= notanumber", + Type: ConditionTypeAggregate, + AggregateOver: "steps", + AggregateFunc: "count", + Field: "complete", + Operator: OpGreaterEqual, + Value: "notanumber", // Invalid: not an integer + } + + _, err := cond.Evaluate(ctx) + if err == nil { + t.Error("expected error for non-integer count value") + } +}