From 8b5168f1a311b035ea0f7a6904fa4a247e6a3f19 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 25 Dec 2025 15:02:07 -0800 Subject: [PATCH] fix: Control flow review fixes (gt-8tmz.4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/formula/controlflow.go | 122 +++++++++++++++++++++------ internal/formula/controlflow_test.go | 101 ++++++++++++++++++---- 2 files changed, 179 insertions(+), 44 deletions(-) diff --git a/internal/formula/controlflow.go b/internal/formula/controlflow.go index 9d4d3d3b..203fea99 100644 --- 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) } diff --git a/internal/formula/controlflow_test.go b/internal/formula/controlflow_test.go index 31bdaba8..1eb754ea 100644 --- 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{