Skip to content

Commit e751888

Browse files
committed
fix(cook): propagate cross-expansion dependencies to expanded steps
When bd cook expands placeholder steps via compose.expand, the target step's Needs/DependsOn were discarded. The first substep of each expansion started with needs:[] instead of inheriting the parent step's dependencies, causing molecules to execute steps out of order. Add propagateTargetDeps() to copy the target step's dependencies to the root steps of each expansion. Also rebuild stepMap after each UpdateDependenciesForExpansion call so subsequent iterations see resolved (not stale) dependency references. Fixes both ApplyExpansions (compose.expand/map) and ApplyInlineExpansions (step.expand). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Executed-By: beads/crew/leeloo Rig: beads Role: crew
1 parent 362608a commit e751888

2 files changed

Lines changed: 278 additions & 10 deletions

File tree

internal/formula/expand.go

Lines changed: 59 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,11 @@ func ApplyExpansions(steps []*Step, compose *ComposeRules, parser *Parser) ([]*S
8181
return nil, fmt.Errorf("expand %q: %w", rule.Target, err)
8282
}
8383

84+
// Propagate target step's dependencies to root steps of the expansion.
85+
// Root steps are those whose needs/dependsOn only reference IDs within
86+
// the expansion (or are empty) — they are the entry points.
87+
propagateTargetDeps(targetStep, expandedSteps)
88+
8489
// Replace the target step with expanded steps
8590
result = replaceStep(result, rule.Target, expandedSteps)
8691
expanded[rule.Target] = true
@@ -92,11 +97,8 @@ func ApplyExpansions(steps []*Step, compose *ComposeRules, parser *Parser) ([]*S
9297
result = UpdateDependenciesForExpansion(result, rule.Target, lastStepID)
9398
}
9499

95-
// Update step map with new steps
96-
for _, s := range expandedSteps {
97-
stepMap[s.ID] = s
98-
}
99-
delete(stepMap, rule.Target)
100+
// Rebuild stepMap from result so subsequent iterations see resolved deps
101+
stepMap = buildStepMap(result)
100102
}
101103

102104
// Apply map rules (pattern matching)
@@ -134,6 +136,10 @@ func ApplyExpansions(steps []*Step, compose *ComposeRules, parser *Parser) ([]*S
134136
if err != nil {
135137
return nil, fmt.Errorf("map %q -> %q: %w", rule.Select, targetStep.ID, err)
136138
}
139+
140+
// Propagate target step's dependencies to root steps of the expansion
141+
propagateTargetDeps(targetStep, expandedSteps)
142+
137143
result = replaceStep(result, targetStep.ID, expandedSteps)
138144
expanded[targetStep.ID] = true
139145

@@ -144,11 +150,8 @@ func ApplyExpansions(steps []*Step, compose *ComposeRules, parser *Parser) ([]*S
144150
result = UpdateDependenciesForExpansion(result, targetStep.ID, lastStepID)
145151
}
146152

147-
// Update step map
148-
for _, s := range expandedSteps {
149-
stepMap[s.ID] = s
150-
}
151-
delete(stepMap, targetStep.ID)
153+
// Rebuild stepMap from result so subsequent iterations see resolved deps
154+
stepMap = buildStepMap(result)
152155
}
153156
}
154157

@@ -361,6 +364,49 @@ func UpdateDependenciesForExpansion(steps []*Step, expandedID string, lastExpand
361364
return result
362365
}
363366

367+
// propagateTargetDeps copies the target step's Needs and DependsOn to the root
368+
// steps of an expansion. Root steps are those whose existing dependencies only
369+
// reference other steps within the expansion (i.e., they have no external deps
370+
// from the template). This preserves cross-expansion dependency chains that would
371+
// otherwise be lost when the target step is replaced.
372+
func propagateTargetDeps(target *Step, expandedSteps []*Step) {
373+
if len(target.Needs) == 0 && len(target.DependsOn) == 0 {
374+
return
375+
}
376+
377+
expandedIDs := make(map[string]bool, len(expandedSteps))
378+
for _, s := range expandedSteps {
379+
expandedIDs[s.ID] = true
380+
}
381+
382+
for _, s := range expandedSteps {
383+
isRoot := true
384+
for _, n := range s.Needs {
385+
if expandedIDs[n] {
386+
isRoot = false
387+
break
388+
}
389+
}
390+
if isRoot {
391+
for _, d := range s.DependsOn {
392+
if expandedIDs[d] {
393+
isRoot = false
394+
break
395+
}
396+
}
397+
}
398+
if isRoot {
399+
// Prepend target's deps (new slice to avoid aliasing)
400+
if len(target.Needs) > 0 {
401+
s.Needs = append(append([]string{}, target.Needs...), s.Needs...)
402+
}
403+
if len(target.DependsOn) > 0 {
404+
s.DependsOn = append(append([]string{}, target.DependsOn...), s.DependsOn...)
405+
}
406+
}
407+
}
408+
}
409+
364410
// ApplyInlineExpansions applies Step.Expand fields to inline expansions.
365411
// Steps with the Expand field set are replaced by the referenced expansion template.
366412
// The step's ExpandVars are passed as variable overrides to the expansion.
@@ -414,6 +460,9 @@ func applyInlineExpansionsRecursive(steps []*Step, parser *Parser, depth int) ([
414460
return nil, fmt.Errorf("inline expand on step %q: %w", step.ID, err)
415461
}
416462

463+
// Propagate the original step's dependencies to root steps of the expansion
464+
propagateTargetDeps(step, expandedSteps)
465+
417466
// Recursively process expanded steps for nested inline expansions
418467
processedSteps, err := applyInlineExpansionsRecursive(expandedSteps, parser, depth+1)
419468
if err != nil {

internal/formula/expand_test.go

Lines changed: 219 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,225 @@ func TestApplyExpansionsDuplicateIDs(t *testing.T) {
742742
})
743743
}
744744

745+
func TestApplyExpansionsCrossExpansionDeps(t *testing.T) {
746+
// Regression test: when two steps have a dependency chain and both are expanded,
747+
// the first substep of the second expansion must inherit the resolved dependency
748+
// on the last substep of the first expansion.
749+
tmpDir := t.TempDir()
750+
751+
// Expansion template: target → target.sub-a, target.sub-b (chained)
752+
expansion := `{
753+
"formula": "two-step-expansion",
754+
"type": "expansion",
755+
"version": 1,
756+
"template": [
757+
{"id": "{target}.sub-a", "title": "Sub A of {target.title}"},
758+
{"id": "{target}.sub-b", "title": "Sub B of {target.title}", "needs": ["{target}.sub-a"]}
759+
]
760+
}`
761+
err := os.WriteFile(filepath.Join(tmpDir, "two-step-expansion.formula.json"), []byte(expansion), 0644)
762+
if err != nil {
763+
t.Fatal(err)
764+
}
765+
766+
parser := NewParser(tmpDir)
767+
768+
t.Run("expand two chained steps preserves cross-expansion deps", func(t *testing.T) {
769+
steps := []*Step{
770+
{ID: "step-1", Title: "Step 1"},
771+
{ID: "step-2", Title: "Step 2", Needs: []string{"step-1"}},
772+
}
773+
774+
compose := &ComposeRules{
775+
Expand: []*ExpandRule{
776+
{Target: "step-1", With: "two-step-expansion"},
777+
{Target: "step-2", With: "two-step-expansion"},
778+
},
779+
}
780+
781+
result, err := ApplyExpansions(steps, compose, parser)
782+
if err != nil {
783+
t.Fatalf("ApplyExpansions failed: %v", err)
784+
}
785+
786+
if len(result) != 4 {
787+
t.Fatalf("expected 4 steps, got %d", len(result))
788+
}
789+
790+
// Verify step IDs
791+
expectedIDs := []string{"step-1.sub-a", "step-1.sub-b", "step-2.sub-a", "step-2.sub-b"}
792+
for i, exp := range expectedIDs {
793+
if result[i].ID != exp {
794+
t.Errorf("result[%d].ID = %q, want %q", i, result[i].ID, exp)
795+
}
796+
}
797+
798+
// KEY ASSERTION: step-2.sub-a must need step-1.sub-b (the last step of step-1's expansion)
799+
// Before the fix, this was [] (empty).
800+
step2SubA := result[2]
801+
if len(step2SubA.Needs) != 1 || step2SubA.Needs[0] != "step-1.sub-b" {
802+
t.Errorf("step-2.sub-a.Needs = %v, want [step-1.sub-b]", step2SubA.Needs)
803+
}
804+
805+
// step-2.sub-b should need step-2.sub-a (internal template dep)
806+
step2SubB := result[3]
807+
if len(step2SubB.Needs) != 1 || step2SubB.Needs[0] != "step-2.sub-a" {
808+
t.Errorf("step-2.sub-b.Needs = %v, want [step-2.sub-a]", step2SubB.Needs)
809+
}
810+
811+
// step-1.sub-a should have no deps (root of entire chain)
812+
if len(result[0].Needs) != 0 {
813+
t.Errorf("step-1.sub-a.Needs = %v, want []", result[0].Needs)
814+
}
815+
})
816+
817+
t.Run("expand three chained steps preserves full dependency chain", func(t *testing.T) {
818+
steps := []*Step{
819+
{ID: "step-1", Title: "Step 1"},
820+
{ID: "step-2", Title: "Step 2", Needs: []string{"step-1"}},
821+
{ID: "step-3", Title: "Step 3", Needs: []string{"step-2"}},
822+
}
823+
824+
compose := &ComposeRules{
825+
Expand: []*ExpandRule{
826+
{Target: "step-1", With: "two-step-expansion"},
827+
{Target: "step-2", With: "two-step-expansion"},
828+
{Target: "step-3", With: "two-step-expansion"},
829+
},
830+
}
831+
832+
result, err := ApplyExpansions(steps, compose, parser)
833+
if err != nil {
834+
t.Fatalf("ApplyExpansions failed: %v", err)
835+
}
836+
837+
if len(result) != 6 {
838+
t.Fatalf("expected 6 steps, got %d", len(result))
839+
}
840+
841+
// step-2.sub-a needs step-1.sub-b
842+
if len(result[2].Needs) != 1 || result[2].Needs[0] != "step-1.sub-b" {
843+
t.Errorf("step-2.sub-a.Needs = %v, want [step-1.sub-b]", result[2].Needs)
844+
}
845+
846+
// step-3.sub-a needs step-2.sub-b
847+
if len(result[4].Needs) != 1 || result[4].Needs[0] != "step-2.sub-b" {
848+
t.Errorf("step-3.sub-a.Needs = %v, want [step-2.sub-b]", result[4].Needs)
849+
}
850+
})
851+
852+
t.Run("expand with depends_on preserves cross-expansion deps", func(t *testing.T) {
853+
steps := []*Step{
854+
{ID: "step-1", Title: "Step 1"},
855+
{ID: "step-2", Title: "Step 2", DependsOn: []string{"step-1"}},
856+
}
857+
858+
compose := &ComposeRules{
859+
Expand: []*ExpandRule{
860+
{Target: "step-1", With: "two-step-expansion"},
861+
{Target: "step-2", With: "two-step-expansion"},
862+
},
863+
}
864+
865+
result, err := ApplyExpansions(steps, compose, parser)
866+
if err != nil {
867+
t.Fatalf("ApplyExpansions failed: %v", err)
868+
}
869+
870+
// step-2.sub-a must depend on step-1.sub-b via DependsOn
871+
step2SubA := result[2]
872+
if len(step2SubA.DependsOn) != 1 || step2SubA.DependsOn[0] != "step-1.sub-b" {
873+
t.Errorf("step-2.sub-a.DependsOn = %v, want [step-1.sub-b]", step2SubA.DependsOn)
874+
}
875+
})
876+
877+
t.Run("non-expanded step after expanded step gets deps rewritten", func(t *testing.T) {
878+
// Verify that non-expanded steps still get their refs rewritten (existing behavior)
879+
steps := []*Step{
880+
{ID: "step-1", Title: "Step 1"},
881+
{ID: "step-2", Title: "Step 2", Needs: []string{"step-1"}},
882+
}
883+
884+
compose := &ComposeRules{
885+
Expand: []*ExpandRule{
886+
{Target: "step-1", With: "two-step-expansion"},
887+
// step-2 is NOT expanded
888+
},
889+
}
890+
891+
result, err := ApplyExpansions(steps, compose, parser)
892+
if err != nil {
893+
t.Fatalf("ApplyExpansions failed: %v", err)
894+
}
895+
896+
if len(result) != 3 {
897+
t.Fatalf("expected 3 steps, got %d", len(result))
898+
}
899+
900+
// step-2 (non-expanded) should now need step-1.sub-b
901+
if len(result[2].Needs) != 1 || result[2].Needs[0] != "step-1.sub-b" {
902+
t.Errorf("step-2.Needs = %v, want [step-1.sub-b]", result[2].Needs)
903+
}
904+
})
905+
}
906+
907+
func TestApplyInlineExpansionsCrossExpansionDeps(t *testing.T) {
908+
tmpDir := t.TempDir()
909+
910+
expansion := `{
911+
"formula": "inline-exp",
912+
"type": "expansion",
913+
"version": 1,
914+
"template": [
915+
{"id": "{target}.first", "title": "First of {target.title}"},
916+
{"id": "{target}.second", "title": "Second of {target.title}", "needs": ["{target}.first"]}
917+
]
918+
}`
919+
err := os.WriteFile(filepath.Join(tmpDir, "inline-exp.formula.json"), []byte(expansion), 0644)
920+
if err != nil {
921+
t.Fatal(err)
922+
}
923+
924+
parser := NewParser(tmpDir)
925+
926+
t.Run("inline expand preserves needs on expanded step", func(t *testing.T) {
927+
steps := []*Step{
928+
{ID: "setup", Title: "Setup"},
929+
{
930+
ID: "work",
931+
Title: "Work",
932+
Needs: []string{"setup"},
933+
Expand: "inline-exp",
934+
},
935+
}
936+
937+
result, err := ApplyInlineExpansions(steps, parser)
938+
if err != nil {
939+
t.Fatalf("ApplyInlineExpansions failed: %v", err)
940+
}
941+
942+
// setup + work.first + work.second = 3
943+
if len(result) != 3 {
944+
t.Fatalf("expected 3 steps, got %d", len(result))
945+
}
946+
947+
// work.first should inherit the "setup" dependency from the original step
948+
workFirst := result[1]
949+
if workFirst.ID != "work.first" {
950+
t.Fatalf("result[1].ID = %q, want work.first", workFirst.ID)
951+
}
952+
if len(workFirst.Needs) != 1 || workFirst.Needs[0] != "setup" {
953+
t.Errorf("work.first.Needs = %v, want [setup]", workFirst.Needs)
954+
}
955+
956+
// work.second should only need work.first (internal template dep)
957+
workSecond := result[2]
958+
if len(workSecond.Needs) != 1 || workSecond.Needs[0] != "work.first" {
959+
t.Errorf("work.second.Needs = %v, want [work.first]", workSecond.Needs)
960+
}
961+
})
962+
}
963+
745964
func TestFindDuplicateStepIDs(t *testing.T) {
746965
tests := []struct {
747966
name string

0 commit comments

Comments
 (0)