Merge pull request #1901 from Xexr/fix/cook-cross-expansion-deps

Reviewed by wickham (PR sheriff). Clean bug fix with good tests. CI failures are unrelated (known dolt issues).

Author: steveyegge

internal/formula/expand.go

@@ -81,6 +81,11 @@ func ApplyExpansions(steps []*Step, compose *ComposeRules, parser *Parser) ([]*S
 			return nil, fmt.Errorf("expand %q: %w", rule.Target, err)
 		}
 
+		// Propagate target step's dependencies to root steps of the expansion.
+		// Root steps are those whose needs/dependsOn only reference IDs within
+		// the expansion (or are empty) — they are the entry points.
+		propagateTargetDeps(targetStep, expandedSteps)
+
 		// Replace the target step with expanded steps
 		result = replaceStep(result, rule.Target, expandedSteps)
 		expanded[rule.Target] = true
@@ -92,11 +97,8 @@ func ApplyExpansions(steps []*Step, compose *ComposeRules, parser *Parser) ([]*S
 			result = UpdateDependenciesForExpansion(result, rule.Target, lastStepID)
 		}
 
-		// Update step map with new steps
-		for _, s := range expandedSteps {
-			stepMap[s.ID] = s
-		}
-		delete(stepMap, rule.Target)
+		// Rebuild stepMap from result so subsequent iterations see resolved deps
+		stepMap = buildStepMap(result)
 	}
 
 	// Apply map rules (pattern matching)
@@ -134,6 +136,10 @@ func ApplyExpansions(steps []*Step, compose *ComposeRules, parser *Parser) ([]*S
 			if err != nil {
 				return nil, fmt.Errorf("map %q -> %q: %w", rule.Select, targetStep.ID, err)
 			}
+
+			// Propagate target step's dependencies to root steps of the expansion
+			propagateTargetDeps(targetStep, expandedSteps)
+
 			result = replaceStep(result, targetStep.ID, expandedSteps)
 			expanded[targetStep.ID] = true
 
@@ -144,11 +150,8 @@ func ApplyExpansions(steps []*Step, compose *ComposeRules, parser *Parser) ([]*S
 				result = UpdateDependenciesForExpansion(result, targetStep.ID, lastStepID)
 			}
 
-			// Update step map
-			for _, s := range expandedSteps {
-				stepMap[s.ID] = s
-			}
-			delete(stepMap, targetStep.ID)
+			// Rebuild stepMap from result so subsequent iterations see resolved deps
+			stepMap = buildStepMap(result)
 		}
 	}
 
@@ -361,6 +364,49 @@ func UpdateDependenciesForExpansion(steps []*Step, expandedID string, lastExpand
 	return result
 }
 
+// propagateTargetDeps copies the target step's Needs and DependsOn to the root
+// steps of an expansion. Root steps are those whose existing dependencies only
+// reference other steps within the expansion (i.e., they have no external deps
+// from the template). This preserves cross-expansion dependency chains that would
+// otherwise be lost when the target step is replaced.
+func propagateTargetDeps(target *Step, expandedSteps []*Step) {
+	if len(target.Needs) == 0 && len(target.DependsOn) == 0 {
+		return
+	}
+
+	expandedIDs := make(map[string]bool, len(expandedSteps))
+	for _, s := range expandedSteps {
+		expandedIDs[s.ID] = true
+	}
+
+	for _, s := range expandedSteps {
+		isRoot := true
+		for _, n := range s.Needs {
+			if expandedIDs[n] {
+				isRoot = false
+				break
+			}
+		}
+		if isRoot {
+			for _, d := range s.DependsOn {
+				if expandedIDs[d] {
+					isRoot = false
+					break
+				}
+			}
+		}
+		if isRoot {
+			// Prepend target's deps (new slice to avoid aliasing)
+			if len(target.Needs) > 0 {
+				s.Needs = append(append([]string{}, target.Needs...), s.Needs...)
+			}
+			if len(target.DependsOn) > 0 {
+				s.DependsOn = append(append([]string{}, target.DependsOn...), s.DependsOn...)
+			}
+		}
+	}
+}
+
 // ApplyInlineExpansions applies Step.Expand fields to inline expansions.
 // Steps with the Expand field set are replaced by the referenced expansion template.
 // The step's ExpandVars are passed as variable overrides to the expansion.
@@ -414,6 +460,9 @@ func applyInlineExpansionsRecursive(steps []*Step, parser *Parser, depth int) ([
 				return nil, fmt.Errorf("inline expand on step %q: %w", step.ID, err)
 			}
 
+			// Propagate the original step's dependencies to root steps of the expansion
+			propagateTargetDeps(step, expandedSteps)
+
 			// Recursively process expanded steps for nested inline expansions
 			processedSteps, err := applyInlineExpansionsRecursive(expandedSteps, parser, depth+1)
 			if err != nil {

internal/formula/expand_test.go

@@ -742,6 +742,225 @@ func TestApplyExpansionsDuplicateIDs(t *testing.T) {
 	})
 }
 
+func TestApplyExpansionsCrossExpansionDeps(t *testing.T) {
+	// Regression test: when two steps have a dependency chain and both are expanded,
+	// the first substep of the second expansion must inherit the resolved dependency
+	// on the last substep of the first expansion.
+	tmpDir := t.TempDir()
+
+	// Expansion template: target → target.sub-a, target.sub-b (chained)
+	expansion := `{
+		"formula": "two-step-expansion",
+		"type": "expansion",
+		"version": 1,
+		"template": [
+			{"id": "{target}.sub-a", "title": "Sub A of {target.title}"},
+			{"id": "{target}.sub-b", "title": "Sub B of {target.title}", "needs": ["{target}.sub-a"]}
+		]
+	}`
+	err := os.WriteFile(filepath.Join(tmpDir, "two-step-expansion.formula.json"), []byte(expansion), 0644)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	parser := NewParser(tmpDir)
+
+	t.Run("expand two chained steps preserves cross-expansion deps", func(t *testing.T) {
+		steps := []*Step{
+			{ID: "step-1", Title: "Step 1"},
+			{ID: "step-2", Title: "Step 2", Needs: []string{"step-1"}},
+		}
+
+		compose := &ComposeRules{
+			Expand: []*ExpandRule{
+				{Target: "step-1", With: "two-step-expansion"},
+				{Target: "step-2", With: "two-step-expansion"},
+			},
+		}
+
+		result, err := ApplyExpansions(steps, compose, parser)
+		if err != nil {
+			t.Fatalf("ApplyExpansions failed: %v", err)
+		}
+
+		if len(result) != 4 {
+			t.Fatalf("expected 4 steps, got %d", len(result))
+		}
+
+		// Verify step IDs
+		expectedIDs := []string{"step-1.sub-a", "step-1.sub-b", "step-2.sub-a", "step-2.sub-b"}
+		for i, exp := range expectedIDs {
+			if result[i].ID != exp {
+				t.Errorf("result[%d].ID = %q, want %q", i, result[i].ID, exp)
+			}
+		}
+
+		// KEY ASSERTION: step-2.sub-a must need step-1.sub-b (the last step of step-1's expansion)
+		// Before the fix, this was [] (empty).
+		step2SubA := result[2]
+		if len(step2SubA.Needs) != 1 || step2SubA.Needs[0] != "step-1.sub-b" {
+			t.Errorf("step-2.sub-a.Needs = %v, want [step-1.sub-b]", step2SubA.Needs)
+		}
+
+		// step-2.sub-b should need step-2.sub-a (internal template dep)
+		step2SubB := result[3]
+		if len(step2SubB.Needs) != 1 || step2SubB.Needs[0] != "step-2.sub-a" {
+			t.Errorf("step-2.sub-b.Needs = %v, want [step-2.sub-a]", step2SubB.Needs)
+		}
+
+		// step-1.sub-a should have no deps (root of entire chain)
+		if len(result[0].Needs) != 0 {
+			t.Errorf("step-1.sub-a.Needs = %v, want []", result[0].Needs)
+		}
+	})
+
+	t.Run("expand three chained steps preserves full dependency chain", func(t *testing.T) {
+		steps := []*Step{
+			{ID: "step-1", Title: "Step 1"},
+			{ID: "step-2", Title: "Step 2", Needs: []string{"step-1"}},
+			{ID: "step-3", Title: "Step 3", Needs: []string{"step-2"}},
+		}
+
+		compose := &ComposeRules{
+			Expand: []*ExpandRule{
+				{Target: "step-1", With: "two-step-expansion"},
+				{Target: "step-2", With: "two-step-expansion"},
+				{Target: "step-3", With: "two-step-expansion"},
+			},
+		}
+
+		result, err := ApplyExpansions(steps, compose, parser)
+		if err != nil {
+			t.Fatalf("ApplyExpansions failed: %v", err)
+		}
+
+		if len(result) != 6 {
+			t.Fatalf("expected 6 steps, got %d", len(result))
+		}
+
+		// step-2.sub-a needs step-1.sub-b
+		if len(result[2].Needs) != 1 || result[2].Needs[0] != "step-1.sub-b" {
+			t.Errorf("step-2.sub-a.Needs = %v, want [step-1.sub-b]", result[2].Needs)
+		}
+
+		// step-3.sub-a needs step-2.sub-b
+		if len(result[4].Needs) != 1 || result[4].Needs[0] != "step-2.sub-b" {
+			t.Errorf("step-3.sub-a.Needs = %v, want [step-2.sub-b]", result[4].Needs)
+		}
+	})
+
+	t.Run("expand with depends_on preserves cross-expansion deps", func(t *testing.T) {
+		steps := []*Step{
+			{ID: "step-1", Title: "Step 1"},
+			{ID: "step-2", Title: "Step 2", DependsOn: []string{"step-1"}},
+		}
+
+		compose := &ComposeRules{
+			Expand: []*ExpandRule{
+				{Target: "step-1", With: "two-step-expansion"},
+				{Target: "step-2", With: "two-step-expansion"},
+			},
+		}
+
+		result, err := ApplyExpansions(steps, compose, parser)
+		if err != nil {
+			t.Fatalf("ApplyExpansions failed: %v", err)
+		}
+
+		// step-2.sub-a must depend on step-1.sub-b via DependsOn
+		step2SubA := result[2]
+		if len(step2SubA.DependsOn) != 1 || step2SubA.DependsOn[0] != "step-1.sub-b" {
+			t.Errorf("step-2.sub-a.DependsOn = %v, want [step-1.sub-b]", step2SubA.DependsOn)
+		}
+	})
+
+	t.Run("non-expanded step after expanded step gets deps rewritten", func(t *testing.T) {
+		// Verify that non-expanded steps still get their refs rewritten (existing behavior)
+		steps := []*Step{
+			{ID: "step-1", Title: "Step 1"},
+			{ID: "step-2", Title: "Step 2", Needs: []string{"step-1"}},
+		}
+
+		compose := &ComposeRules{
+			Expand: []*ExpandRule{
+				{Target: "step-1", With: "two-step-expansion"},
+				// step-2 is NOT expanded
+			},
+		}
+
+		result, err := ApplyExpansions(steps, compose, parser)
+		if err != nil {
+			t.Fatalf("ApplyExpansions failed: %v", err)
+		}
+
+		if len(result) != 3 {
+			t.Fatalf("expected 3 steps, got %d", len(result))
+		}
+
+		// step-2 (non-expanded) should now need step-1.sub-b
+		if len(result[2].Needs) != 1 || result[2].Needs[0] != "step-1.sub-b" {
+			t.Errorf("step-2.Needs = %v, want [step-1.sub-b]", result[2].Needs)
+		}
+	})
+}
+
+func TestApplyInlineExpansionsCrossExpansionDeps(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	expansion := `{
+		"formula": "inline-exp",
+		"type": "expansion",
+		"version": 1,
+		"template": [
+			{"id": "{target}.first", "title": "First of {target.title}"},
+			{"id": "{target}.second", "title": "Second of {target.title}", "needs": ["{target}.first"]}
+		]
+	}`
+	err := os.WriteFile(filepath.Join(tmpDir, "inline-exp.formula.json"), []byte(expansion), 0644)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	parser := NewParser(tmpDir)
+
+	t.Run("inline expand preserves needs on expanded step", func(t *testing.T) {
+		steps := []*Step{
+			{ID: "setup", Title: "Setup"},
+			{
+				ID:     "work",
+				Title:  "Work",
+				Needs:  []string{"setup"},
+				Expand: "inline-exp",
+			},
+		}
+
+		result, err := ApplyInlineExpansions(steps, parser)
+		if err != nil {
+			t.Fatalf("ApplyInlineExpansions failed: %v", err)
+		}
+
+		// setup + work.first + work.second = 3
+		if len(result) != 3 {
+			t.Fatalf("expected 3 steps, got %d", len(result))
+		}
+
+		// work.first should inherit the "setup" dependency from the original step
+		workFirst := result[1]
+		if workFirst.ID != "work.first" {
+			t.Fatalf("result[1].ID = %q, want work.first", workFirst.ID)
+		}
+		if len(workFirst.Needs) != 1 || workFirst.Needs[0] != "setup" {
+			t.Errorf("work.first.Needs = %v, want [setup]", workFirst.Needs)
+		}
+
+		// work.second should only need work.first (internal template dep)
+		workSecond := result[2]
+		if len(workSecond.Needs) != 1 || workSecond.Needs[0] != "work.first" {
+			t.Errorf("work.second.Needs = %v, want [work.first]", workSecond.Needs)
+		}
+	})
+}
+
 func TestFindDuplicateStepIDs(t *testing.T) {
 	tests := []struct {
 		name     string