From 73e56d087612df46f242b4f6292ae1041b3baac3 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 25 Dec 2025 16:48:27 -0800 Subject: [PATCH] Fix in-place mutation in ApplyBranches/ApplyGates (gt-v1pcg) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ApplyBranches and ApplyGates now clone steps before modifying, matching the immutability pattern of ApplyLoops and ApplyAdvice - Added internal applyBranchesWithMap/applyGatesWithMap for efficiency - ApplyControlFlow builds stepMap once for both (gt-gpgdv optimization) - Added cloneStepsRecursive helper - Added TestApplyBranches_Immutability and TestApplyGates_Immutability 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/formula/controlflow.go | 93 +++++++++++++++++++++------- internal/formula/controlflow_test.go | 91 +++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 23 deletions(-) diff --git a/internal/formula/controlflow.go b/internal/formula/controlflow.go index 9d97423b..b786b5ad 100644 --- a/internal/formula/controlflow.go +++ b/internal/formula/controlflow.go @@ -328,37 +328,54 @@ func chainExpandedIterations(steps []*Step, loopID string, count int) []*Step { // - All branch steps depend on the 'from' step // - The 'join' step depends on all branch steps // -// Returns the modified steps slice (steps are modified in place for dependencies). +// Returns a new steps slice with dependencies added. +// The original steps slice is not modified. func ApplyBranches(steps []*Step, compose *ComposeRules) ([]*Step, error) { if compose == nil || len(compose.Branch) == 0 { return steps, nil } - // Build step map for quick lookup - stepMap := buildStepMap(steps) + // Clone steps to avoid mutating input (gt-v1pcg) + cloned := cloneStepsRecursive(steps) + stepMap := buildStepMap(cloned) + + if err := applyBranchesWithMap(stepMap, compose); err != nil { + return nil, err + } + + return cloned, nil +} + +// applyBranchesWithMap applies branch rules using a pre-built stepMap. +// This is the internal implementation used by both ApplyBranches and ApplyControlFlow. +// The stepMap entries are modified in place. +func applyBranchesWithMap(stepMap map[string]*Step, compose *ComposeRules) error { + if compose == nil || len(compose.Branch) == 0 { + return nil + } for _, branch := range compose.Branch { // Validate the branch rule if branch.From == "" { - return nil, fmt.Errorf("branch: from is required") + return fmt.Errorf("branch: from is required") } if len(branch.Steps) == 0 { - return nil, fmt.Errorf("branch: steps is required") + return fmt.Errorf("branch: steps is required") } if branch.Join == "" { - return nil, fmt.Errorf("branch: join is required") + return fmt.Errorf("branch: join is required") } // Verify all steps exist if _, ok := stepMap[branch.From]; !ok { - return nil, fmt.Errorf("branch: from step %q not found", branch.From) + return fmt.Errorf("branch: from step %q not found", branch.From) } if _, ok := stepMap[branch.Join]; !ok { - return nil, fmt.Errorf("branch: join step %q not found", branch.Join) + return fmt.Errorf("branch: join step %q not found", branch.Join) } for _, stepID := range branch.Steps { if _, ok := stepMap[stepID]; !ok { - return nil, fmt.Errorf("branch: parallel step %q not found", stepID) + return fmt.Errorf("branch: parallel step %q not found", stepID) } } @@ -375,7 +392,7 @@ func ApplyBranches(steps []*Step, compose *ComposeRules) ([]*Step, error) { } } - return steps, nil + return nil } // ApplyGates adds gate conditions to steps. @@ -383,34 +400,51 @@ func ApplyBranches(steps []*Step, compose *ComposeRules) ([]*Step, error) { // - The target step gets a "gate:condition" label // - At runtime, the patrol executor evaluates the condition // -// Returns the modified steps slice. +// Returns a new steps slice with gate labels added. +// The original steps slice is not modified. func ApplyGates(steps []*Step, compose *ComposeRules) ([]*Step, error) { if compose == nil || len(compose.Gate) == 0 { return steps, nil } - // Build step map for quick lookup - stepMap := buildStepMap(steps) + // Clone steps to avoid mutating input (gt-v1pcg) + cloned := cloneStepsRecursive(steps) + stepMap := buildStepMap(cloned) + + if err := applyGatesWithMap(stepMap, compose); err != nil { + return nil, err + } + + return cloned, nil +} + +// applyGatesWithMap applies gate rules using a pre-built stepMap. +// This is the internal implementation used by both ApplyGates and ApplyControlFlow. +// The stepMap entries are modified in place. +func applyGatesWithMap(stepMap map[string]*Step, compose *ComposeRules) error { + if compose == nil || len(compose.Gate) == 0 { + return nil + } for _, gate := range compose.Gate { // Validate the gate rule if gate.Before == "" { - return nil, fmt.Errorf("gate: before is required") + return fmt.Errorf("gate: before is required") } if gate.Condition == "" { - return nil, fmt.Errorf("gate: condition is required") + return fmt.Errorf("gate: condition is required") } // Validate the condition syntax _, err := ParseCondition(gate.Condition) if err != nil { - return nil, fmt.Errorf("gate: invalid condition %q: %w", gate.Condition, err) + return fmt.Errorf("gate: invalid condition %q: %w", gate.Condition, err) } // Find the target step step, ok := stepMap[gate.Before] if !ok { - return nil, fmt.Errorf("gate: target step %q not found", gate.Before) + return fmt.Errorf("gate: target step %q not found", gate.Before) } // Add gate label for runtime evaluation using JSON for unambiguous parsing @@ -420,31 +454,35 @@ func ApplyGates(steps []*Step, compose *ComposeRules) ([]*Step, error) { step.Labels = appendUnique(step.Labels, gateLabel) } - return steps, nil + return nil } // ApplyControlFlow applies all control flow operators in the correct order: // 1. Loops (expand iterations) // 2. Branches (wire fork-join dependencies) // 3. Gates (add condition labels) +// +// Returns a new steps slice. The original steps slice is not modified. func ApplyControlFlow(steps []*Step, compose *ComposeRules) ([]*Step, error) { var err error - // Apply loops first (expands steps) + // Apply loops first (expands steps) - ApplyLoops already returns new slice steps, err = ApplyLoops(steps) if err != nil { return nil, fmt.Errorf("applying loops: %w", err) } + // Build stepMap once for branches and gates (gt-gpgdv optimization) + // No need to clone here since ApplyLoops already returned a new slice + stepMap := buildStepMap(steps) + // Apply branches (wires dependencies) - steps, err = ApplyBranches(steps, compose) - if err != nil { + if err := applyBranchesWithMap(stepMap, compose); err != nil { return nil, fmt.Errorf("applying branches: %w", err) } // Apply gates (adds labels) - steps, err = ApplyGates(steps, compose) - if err != nil { + if err := applyGatesWithMap(stepMap, compose); err != nil { return nil, fmt.Errorf("applying gates: %w", err) } @@ -465,6 +503,15 @@ func cloneStepDeep(s *Step) *Step { return clone } +// cloneStepsRecursive creates a deep copy of a slice of steps (gt-v1pcg). +func cloneStepsRecursive(steps []*Step) []*Step { + result := make([]*Step, len(steps)) + for i, step := range steps { + result[i] = cloneStepDeep(step) + } + return result +} + // cloneLoopSpec creates a deep copy of a LoopSpec (gt-zn35j). func cloneLoopSpec(loop *LoopSpec) *LoopSpec { if loop == nil { diff --git a/internal/formula/controlflow_test.go b/internal/formula/controlflow_test.go index 5f09af77..0230c3ea 100644 --- a/internal/formula/controlflow_test.go +++ b/internal/formula/controlflow_test.go @@ -731,3 +731,94 @@ func TestApplyLoops_NestedLoopsOuterChaining(t *testing.T) { "Expected Needs to contain %q, got %v", expectedDep, step2.Needs) } } + +// gt-v1pcg: Tests for immutability of ApplyBranches and ApplyGates + +func TestApplyBranches_Immutability(t *testing.T) { + // Create steps with no initial dependencies + steps := []*Step{ + {ID: "setup", Title: "Setup", Needs: nil}, + {ID: "test", Title: "Run tests", Needs: nil}, + {ID: "deploy", Title: "Deploy", Needs: nil}, + } + + compose := &ComposeRules{ + Branch: []*BranchRule{ + {From: "setup", Steps: []string{"test"}, Join: "deploy"}, + }, + } + + // Call ApplyBranches + result, err := ApplyBranches(steps, compose) + if err != nil { + t.Fatalf("ApplyBranches failed: %v", err) + } + + // Verify original steps are NOT mutated + for _, step := range steps { + if len(step.Needs) != 0 { + t.Errorf("Original step %q was mutated: Needs = %v (expected nil)", step.ID, step.Needs) + } + } + + // Verify result has the expected dependencies + resultMap := make(map[string]*Step) + for _, s := range result { + resultMap[s.ID] = s + } + + if len(resultMap["test"].Needs) != 1 || resultMap["test"].Needs[0] != "setup" { + t.Errorf("Result test step should need setup, got %v", resultMap["test"].Needs) + } +} + +func TestApplyGates_Immutability(t *testing.T) { + // Create steps with no initial labels + steps := []*Step{ + {ID: "tests", Title: "Run tests", Labels: nil}, + {ID: "deploy", Title: "Deploy", Labels: nil}, + } + + compose := &ComposeRules{ + Gate: []*GateRule{ + {Before: "deploy", Condition: "tests.status == 'complete'"}, + }, + } + + // Call ApplyGates + result, err := ApplyGates(steps, compose) + if err != nil { + t.Fatalf("ApplyGates failed: %v", err) + } + + // Verify original steps are NOT mutated + for _, step := range steps { + if len(step.Labels) != 0 { + t.Errorf("Original step %q was mutated: Labels = %v (expected nil)", step.ID, step.Labels) + } + } + + // Verify result has the expected labels + var deployResult *Step + for _, s := range result { + if s.ID == "deploy" { + deployResult = s + break + } + } + + if deployResult == nil { + t.Fatal("deploy step not found in result") + } + + hasGate := false + for _, label := range deployResult.Labels { + if len(label) > 5 && label[:5] == "gate:" { + hasGate = true + break + } + } + if !hasGate { + t.Errorf("Result deploy step should have gate label, got %v", deployResult.Labels) + } +}