fix: Control flow review fixes (gt-8tmz.4)

Critical bug fixes identified during code review: 1. External dependency rewriting: Dependencies on steps OUTSIDE the loop are now preserved as-is instead of being incorrectly prefixed. 2. Children dependency rewriting: Child step dependencies are now properly rewritten with iteration context. 3. Missing fields: Added Expand, ExpandVars, Gate fields to loop expansion. 4. Condition validation: Loop `until` conditions are now validated with ParseCondition to catch syntax errors early. 5. Label format: Changed gate and loop labels from colon-delimited to JSON format for unambiguous parsing: - gate:{"condition":"expr"} - loop:{"until":"expr","max":N} Added TestApplyLoops_ExternalDependencies to verify the fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2025-12-25 15:02:07 -08:00
parent 4f065024b5
commit 8b5168f1a3
2 changed files with 179 additions and 44 deletions
--- a/internal/formula/controlflow.go
+++ b/internal/formula/controlflow.go
@@ -10,6 +10,7 @@
 package formula

 import (
+	"encoding/json"
 	"fmt"
 )

@@ -77,6 +78,13 @@ func validateLoopSpec(loop *LoopSpec, stepID string) error {
 		return fmt.Errorf("loop %q: max must be positive", stepID)
 	}

+	// Validate until condition syntax if present
+	if loop.Until != "" {
+		if _, err := ParseCondition(loop.Until); err != nil {
+			return fmt.Errorf("loop %q: invalid until condition %q: %w", stepID, loop.Until, err)
+		}
+	}
+
 	return nil
 }

@@ -109,9 +117,13 @@ func expandLoop(step *Step) ([]*Step, error) {
 		// Add loop metadata to first step for runtime evaluation
 		if len(iterSteps) > 0 {
 			firstStep := iterSteps[0]
-			// Add labels for runtime loop control
-			firstStep.Labels = append(firstStep.Labels, fmt.Sprintf("loop:until:%s", step.Loop.Until))
-			firstStep.Labels = append(firstStep.Labels, fmt.Sprintf("loop:max:%d", step.Loop.Max))
+			// Add labels for runtime loop control using JSON for unambiguous parsing
+			loopMeta := map[string]interface{}{
+				"until": step.Loop.Until,
+				"max":   step.Loop.Max,
+			}
+			loopJSON, _ := json.Marshal(loopMeta)
+			firstStep.Labels = append(firstStep.Labels, fmt.Sprintf("loop:%s", string(loopJSON)))
 		}

 		result = iterSteps
@@ -125,6 +137,9 @@ func expandLoop(step *Step) ([]*Step, error) {
 func expandLoopIteration(step *Step, iteration int) ([]*Step, error) {
 	result := make([]*Step, 0, len(step.Loop.Body))

+	// Build set of step IDs within the loop body (for dependency rewriting)
+	bodyStepIDs := collectBodyStepIDs(step.Loop.Body)
+
 	for _, bodyStep := range step.Loop.Body {
 		// Create unique ID for this iteration
 		iterID := fmt.Sprintf("%s.iter%d.%s", step.ID, iteration, bodyStep.ID)
@@ -138,6 +153,17 @@ func expandLoopIteration(step *Step, iteration int) ([]*Step, error) {
 			Assignee:    bodyStep.Assignee,
 			Condition:   bodyStep.Condition,
 			WaitsFor:    bodyStep.WaitsFor,
+			Expand:      bodyStep.Expand,
+			Gate:        bodyStep.Gate,
+			// Note: Loop field intentionally not copied - nested loops need explicit support
+		}
+
+		// Clone ExpandVars if present
+		if len(bodyStep.ExpandVars) > 0 {
+			clone.ExpandVars = make(map[string]string, len(bodyStep.ExpandVars))
+			for k, v := range bodyStep.ExpandVars {
+				clone.ExpandVars[k] = v
+			}
 		}

 		// Clone labels
@@ -146,30 +172,13 @@ func expandLoopIteration(step *Step, iteration int) ([]*Step, error) {
 			copy(clone.Labels, bodyStep.Labels)
 		}

-		// Clone dependencies - prefix with iteration context
-		if len(bodyStep.DependsOn) > 0 {
-			clone.DependsOn = make([]string, len(bodyStep.DependsOn))
-			for i, dep := range bodyStep.DependsOn {
-				clone.DependsOn[i] = fmt.Sprintf("%s.iter%d.%s", step.ID, iteration, dep)
-			}
-		}
+		// Clone dependencies - only prefix references to steps WITHIN the loop body
+		clone.DependsOn = rewriteLoopDependencies(bodyStep.DependsOn, step.ID, iteration, bodyStepIDs)
+		clone.Needs = rewriteLoopDependencies(bodyStep.Needs, step.ID, iteration, bodyStepIDs)

-		if len(bodyStep.Needs) > 0 {
-			clone.Needs = make([]string, len(bodyStep.Needs))
-			for i, need := range bodyStep.Needs {
-				clone.Needs[i] = fmt.Sprintf("%s.iter%d.%s", step.ID, iteration, need)
-			}
-		}
-
-		// Recursively handle children
+		// Recursively handle children with proper dependency rewriting
 		if len(bodyStep.Children) > 0 {
-			children := make([]*Step, 0, len(bodyStep.Children))
-			for _, child := range bodyStep.Children {
-				childClone := cloneStepDeep(child)
-				childClone.ID = fmt.Sprintf("%s.iter%d.%s", step.ID, iteration, child.ID)
-				children = append(children, childClone)
-			}
-			clone.Children = children
+			clone.Children = expandLoopChildren(bodyStep.Children, step.ID, iteration, bodyStepIDs)
 		}

 		result = append(result, clone)
@@ -178,6 +187,63 @@ func expandLoopIteration(step *Step, iteration int) ([]*Step, error) {
 	return result, nil
 }

+// collectBodyStepIDs collects all step IDs within a loop body (including nested children).
+func collectBodyStepIDs(body []*Step) map[string]bool {
+	ids := make(map[string]bool)
+	var collect func([]*Step)
+	collect = func(steps []*Step) {
+		for _, s := range steps {
+			ids[s.ID] = true
+			if len(s.Children) > 0 {
+				collect(s.Children)
+			}
+		}
+	}
+	collect(body)
+	return ids
+}
+
+// rewriteLoopDependencies rewrites dependency references for loop expansion.
+// Only dependencies referencing steps WITHIN the loop body are prefixed.
+// External dependencies are preserved as-is.
+func rewriteLoopDependencies(deps []string, loopID string, iteration int, bodyStepIDs map[string]bool) []string {
+	if len(deps) == 0 {
+		return nil
+	}
+
+	result := make([]string, len(deps))
+	for i, dep := range deps {
+		if bodyStepIDs[dep] {
+			// Internal dependency - prefix with iteration context
+			result[i] = fmt.Sprintf("%s.iter%d.%s", loopID, iteration, dep)
+		} else {
+			// External dependency - preserve as-is
+			result[i] = dep
+		}
+	}
+	return result
+}
+
+// expandLoopChildren expands children within a loop iteration.
+// Rewrites IDs and dependencies appropriately.
+func expandLoopChildren(children []*Step, loopID string, iteration int, bodyStepIDs map[string]bool) []*Step {
+	result := make([]*Step, len(children))
+	for i, child := range children {
+		clone := cloneStepDeep(child)
+		clone.ID = fmt.Sprintf("%s.iter%d.%s", loopID, iteration, child.ID)
+		clone.DependsOn = rewriteLoopDependencies(child.DependsOn, loopID, iteration, bodyStepIDs)
+		clone.Needs = rewriteLoopDependencies(child.Needs, loopID, iteration, bodyStepIDs)
+
+		// Recursively handle nested children
+		if len(child.Children) > 0 {
+			clone.Children = expandLoopChildren(child.Children, loopID, iteration, bodyStepIDs)
+		}
+
+		result[i] = clone
+	}
+	return result
+}
+
 // chainLoopIterations adds dependencies between loop iterations.
 // Each iteration's first step depends on the previous iteration's last step.
 func chainLoopIterations(steps []*Step, body []*Step, count int) []*Step {
@@ -291,8 +357,10 @@ func ApplyGates(steps []*Step, compose *ComposeRules) ([]*Step, error) {
 			return nil, fmt.Errorf("gate: target step %q not found", gate.Before)
 		}

-		// Add gate label for runtime evaluation
-		gateLabel := fmt.Sprintf("gate:condition:%s", gate.Condition)
+		// Add gate label for runtime evaluation using JSON for unambiguous parsing
+		gateMeta := map[string]string{"condition": gate.Condition}
+		gateJSON, _ := json.Marshal(gateMeta)
+		gateLabel := fmt.Sprintf("gate:%s", string(gateJSON))
 		step.Labels = appendUnique(step.Labels, gateLabel)
 	}

--- a/internal/formula/controlflow_test.go
+++ b/internal/formula/controlflow_test.go
@@ -1,6 +1,7 @@
 package formula

 import (
+	"strings"
 	"testing"
 )

@@ -88,24 +89,22 @@ func TestApplyLoops_Conditional(t *testing.T) {
 		t.Errorf("Expected 1 step for conditional loop, got %d", len(result))
 	}

-	// Should have loop metadata labels
+	// Should have loop metadata label with JSON format
 	step := result[0]
-	hasUntil := false
-	hasMax := false
+	hasLoopLabel := false
 	for _, label := range step.Labels {
-		if label == "loop:until:step.status == 'complete'" {
-			hasUntil = true
-		}
-		if label == "loop:max:5" {
-			hasMax = true
+		// Label format: loop:{"max":5,"until":"step.status == 'complete'"}
+		if len(label) > 5 && label[:5] == "loop:" {
+			hasLoopLabel = true
+			// Verify it contains the expected values
+			if !strings.Contains(label, `"until"`) || !strings.Contains(label, `"max"`) {
+				t.Errorf("Loop label missing expected fields: %s", label)
+			}
 		}
 	}

-	if !hasUntil {
-		t.Error("Missing loop:until label")
-	}
-	if !hasMax {
-		t.Error("Missing loop:max label")
+	if !hasLoopLabel {
+		t.Error("Missing loop metadata label")
 	}
 }

@@ -289,12 +288,15 @@ func TestApplyGates(t *testing.T) {
 		t.Fatal("deploy step not found")
 	}

-	// Check for gate label
+	// Check for gate label with JSON format
 	found := false
-	expectedLabel := "gate:condition:tests.status == 'complete'"
 	for _, label := range deploy.Labels {
-		if label == expectedLabel {
+		// Label format: gate:{"condition":"tests.status == 'complete'"}
+		if len(label) > 5 && label[:5] == "gate:" {
 			found = true
+			if !strings.Contains(label, `"condition"`) || !strings.Contains(label, "tests.status") {
+				t.Errorf("Gate label missing expected content: %s", label)
+			}
 			break
 		}
 	}
@@ -382,7 +384,8 @@ func TestApplyControlFlow_Integration(t *testing.T) {

 	hasGate := false
 	for _, label := range cleanup.Labels {
-		if label == "gate:condition:steps.complete >= 2" {
+		// Label format: gate:{"condition":"steps.complete >= 2"}
+		if len(label) > 5 && label[:5] == "gate:" && strings.Contains(label, "steps.complete") {
 			hasGate = true
 			break
 		}
@@ -415,6 +418,70 @@ func TestApplyLoops_NoLoops(t *testing.T) {
 	}
 }

+func TestApplyLoops_ExternalDependencies(t *testing.T) {
+	// Test that dependencies on steps OUTSIDE the loop are preserved as-is
+	steps := []*Step{
+		{ID: "setup", Title: "Setup"},
+		{
+			ID:    "process",
+			Title: "Process items",
+			Loop: &LoopSpec{
+				Count: 2,
+				Body: []*Step{
+					{ID: "work", Title: "Do work", Needs: []string{"setup"}}, // External dep
+					{ID: "save", Title: "Save", Needs: []string{"work"}},     // Internal dep
+				},
+			},
+		},
+	}
+
+	result, err := ApplyLoops(steps)
+	if err != nil {
+		t.Fatalf("ApplyLoops failed: %v", err)
+	}
+
+	// Should have: setup, process.iter1.work, process.iter1.save, process.iter2.work, process.iter2.save
+	if len(result) != 5 {
+		t.Errorf("Expected 5 steps, got %d", len(result))
+	}
+
+	// Find iter1.work - should have external dep on "setup" (not "process.iter1.setup")
+	var work1 *Step
+	for _, s := range result {
+		if s.ID == "process.iter1.work" {
+			work1 = s
+			break
+		}
+	}
+
+	if work1 == nil {
+		t.Fatal("process.iter1.work not found")
+	}
+
+	// External dependency should be preserved as-is
+	if len(work1.Needs) != 1 || work1.Needs[0] != "setup" {
+		t.Errorf("External dependency should be 'setup', got %v", work1.Needs)
+	}
+
+	// Find iter1.save - should have internal dep on "process.iter1.work"
+	var save1 *Step
+	for _, s := range result {
+		if s.ID == "process.iter1.save" {
+			save1 = s
+			break
+		}
+	}
+
+	if save1 == nil {
+		t.Fatal("process.iter1.save not found")
+	}
+
+	// Internal dependency should be prefixed
+	if len(save1.Needs) != 1 || save1.Needs[0] != "process.iter1.work" {
+		t.Errorf("Internal dependency should be 'process.iter1.work', got %v", save1.Needs)
+	}
+}
+
 func TestApplyLoops_NestedChildren(t *testing.T) {
 	// Test that children are preserved when recursing
 	steps := []*Step{