From d2f71773cb89aacd734bc3fdf95bdfd0301fbd87 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 25 Dec 2025 16:35:28 -0800 Subject: [PATCH] fix: Chain iterations after nested loop expansion (gt-zn35j) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes a bug where outer iteration chaining was lost when nested loops were expanded. The problem was: 1. chainLoopIterations set: outer.iter2.inner depends on outer.iter1.inner 2. ApplyLoops recursively expanded nested loops 3. outer.iter2.inner became outer.iter2.inner.iter1.work, etc. 4. The dependency was lost (referenced non-existent ID) The fix: - Move recursive ApplyLoops BEFORE chaining - Add chainExpandedIterations that finds iteration boundaries by ID prefix - Works with variable step counts per iteration (nested loops expand differently) Now outer.iter2's first step correctly depends on outer.iter1's LAST step, ensuring sequential execution of outer iterations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/formula/controlflow.go | 64 +++++++++++++++++++++++++--- internal/formula/controlflow_test.go | 63 +++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 6 deletions(-) diff --git a/internal/formula/controlflow.go b/internal/formula/controlflow.go index 0ee3e239..9d97423b 100644 --- a/internal/formula/controlflow.go +++ b/internal/formula/controlflow.go @@ -12,6 +12,7 @@ package formula import ( "encoding/json" "fmt" + "strings" ) // ApplyLoops expands loop bodies in a formula's steps. @@ -102,9 +103,17 @@ func expandLoop(step *Step) ([]*Step, error) { result = append(result, iterSteps...) } - // Chain iterations: each iteration depends on previous - if len(step.Loop.Body) > 0 && step.Loop.Count > 1 { - result = chainLoopIterations(result, step.Loop.Body, step.Loop.Count) + // Recursively expand any nested loops FIRST (gt-zn35j) + var err error + result, err = ApplyLoops(result) + if err != nil { + return nil, err + } + + // THEN chain iterations on the expanded result + // This must happen AFTER recursive expansion so we chain the final steps + if step.Loop.Count > 1 { + result = chainExpandedIterations(result, step.ID, step.Loop.Count) } } else { // Conditional loop: expand once with loop metadata @@ -126,11 +135,14 @@ func expandLoop(step *Step) ([]*Step, error) { firstStep.Labels = append(firstStep.Labels, fmt.Sprintf("loop:%s", string(loopJSON))) } - result = iterSteps + // Recursively expand any nested loops (gt-zn35j) + result, err = ApplyLoops(iterSteps) + if err != nil { + return nil, err + } } - // Recursively expand any nested loops in the result (gt-zn35j) - return ApplyLoops(result) + return result, nil } // expandLoopIteration expands a single iteration of a loop. @@ -271,6 +283,46 @@ func chainLoopIterations(steps []*Step, body []*Step, count int) []*Step { return steps } +// chainExpandedIterations chains iterations AFTER nested loop expansion (gt-zn35j). +// Unlike chainLoopIterations, this handles variable step counts per iteration +// by finding iteration boundaries via ID prefix matching. +func chainExpandedIterations(steps []*Step, loopID string, count int) []*Step { + if len(steps) == 0 || count < 2 { + return steps + } + + // Find the first and last step index of each iteration + // Iteration N has steps with ID prefix: {loopID}.iter{N}. + iterFirstIdx := make(map[int]int) // iteration -> index of first step + iterLastIdx := make(map[int]int) // iteration -> index of last step + + for i, s := range steps { + for iter := 1; iter <= count; iter++ { + prefix := fmt.Sprintf("%s.iter%d.", loopID, iter) + if strings.HasPrefix(s.ID, prefix) { + if _, found := iterFirstIdx[iter]; !found { + iterFirstIdx[iter] = i + } + iterLastIdx[iter] = i + break + } + } + } + + // Chain: first step of iteration N+1 depends on last step of iteration N + for iter := 2; iter <= count; iter++ { + firstIdx, hasFirst := iterFirstIdx[iter] + prevLastIdx, hasPrevLast := iterLastIdx[iter-1] + + if hasFirst && hasPrevLast { + lastStepID := steps[prevLastIdx].ID + steps[firstIdx].Needs = appendUnique(steps[firstIdx].Needs, lastStepID) + } + } + + return steps +} + // ApplyBranches wires fork-join dependency patterns. // For each branch rule: // - All branch steps depend on the 'from' step diff --git a/internal/formula/controlflow_test.go b/internal/formula/controlflow_test.go index fbdfe878..5f09af77 100644 --- a/internal/formula/controlflow_test.go +++ b/internal/formula/controlflow_test.go @@ -668,3 +668,66 @@ func TestApplyLoops_ThreeLevelNesting(t *testing.T) { t.Errorf("Last step ID wrong: %s", result[7].ID) } } + +func TestApplyLoops_NestedLoopsOuterChaining(t *testing.T) { + // Verify that outer iterations are chained AFTER nested loop expansion. + // outer.iter2's first step should depend on outer.iter1's LAST step. + steps := []*Step{ + { + ID: "outer", + Title: "Outer loop", + Loop: &LoopSpec{ + Count: 2, + Body: []*Step{ + { + ID: "inner", + Title: "Inner loop", + Loop: &LoopSpec{ + Count: 2, + Body: []*Step{ + {ID: "work", Title: "Do work"}, + }, + }, + }, + }, + }, + }, + } + + result, err := ApplyLoops(steps) + if err != nil { + t.Fatalf("ApplyLoops failed: %v", err) + } + + // Should have 4 steps + if len(result) != 4 { + t.Fatalf("Expected 4 steps, got %d", len(result)) + } + + // Expected order: + // 0: outer.iter1.inner.iter1.work + // 1: outer.iter1.inner.iter2.work (depends on above via inner chaining) + // 2: outer.iter2.inner.iter1.work (should depend on step 1 via outer chaining!) + // 3: outer.iter2.inner.iter2.work (depends on above via inner chaining) + + // Verify outer chaining: step 2 should depend on step 1 + step2 := result[2] + if step2.ID != "outer.iter2.inner.iter1.work" { + t.Fatalf("Step 2 ID wrong: %s", step2.ID) + } + + // This is the key assertion: outer.iter2's first step must depend on + // outer.iter1's last step (outer.iter1.inner.iter2.work) + expectedDep := "outer.iter1.inner.iter2.work" + found := false + for _, need := range step2.Needs { + if need == expectedDep { + found = true + break + } + } + if !found { + t.Errorf("outer.iter2 first step should depend on outer.iter1 last step.\n"+ + "Expected Needs to contain %q, got %v", expectedDep, step2.Needs) + } +}