fix(recipe): deep-copy Validation in Merge to prevent cached store pollution

RecipeMetadataSpec.Merge aliased other.Validation when the destination
was nil, then later wrote phase pointers through that alias. Across
successive calls on the cached MetadataStore, this mutated whichever
overlay's cached ValidationConfig the alias happened to point at — so
`BuildRecipeResult` was order-dependent, and an overlay that genuinely
lacked a phase could appear compliant after a prior query planted that
phase via pollution.

Fix:

  - Add cloneValidationConfig + cloneValidationPhase helpers that deep-
    copy phases and their backing slices / NodeSelection.
  - Clone in Merge whenever a phase pointer is assigned (both the
    nil-destination branch and the per-phase overwrite branch).
  - Clone in initBaseMergedSpec so the merged spec never aliases
    s.Base.Spec.Validation.

Regression test in metadata_test.go: two sequential merges into a fresh
destination must not mutate the first source. Without the fix the test
detects Deployment leaking from the second merge into the first source's
ValidationConfig.

Refs #970

Author: mchmarny

pkg/recipe/metadata.go

@@ -456,22 +456,27 @@ func (s *RecipeMetadataSpec) Merge(other *RecipeMetadataSpec) {
 		return s.ComponentRefs[i].Name < s.ComponentRefs[j].Name
 	})
 
-	// Merge validation config - overlay phases take precedence
+	// Merge validation config - overlay phases take precedence. Phase
+	// pointers are cloned (not aliased) so successive merges cannot mutate
+	// the source's cached ValidationConfig — a previous version aliased
+	// other.Validation when s.Validation was nil, then later writes through
+	// s.Validation.Deployment etc. corrupted whichever overlay's cached
+	// metadata the alias pointed at.
 	if other.Validation != nil {
 		if s.Validation == nil {
-			s.Validation = other.Validation
+			s.Validation = cloneValidationConfig(other.Validation)
 		} else {
 			if other.Validation.Readiness != nil {
-				s.Validation.Readiness = other.Validation.Readiness
+				s.Validation.Readiness = cloneValidationPhase(other.Validation.Readiness)
 			}
 			if other.Validation.Deployment != nil {
-				s.Validation.Deployment = other.Validation.Deployment
+				s.Validation.Deployment = cloneValidationPhase(other.Validation.Deployment)
 			}
 			if other.Validation.Performance != nil {
-				s.Validation.Performance = other.Validation.Performance
+				s.Validation.Performance = cloneValidationPhase(other.Validation.Performance)
 			}
 			if other.Validation.Conformance != nil {
-				s.Validation.Conformance = other.Validation.Conformance
+				s.Validation.Conformance = cloneValidationPhase(other.Validation.Conformance)
 			}
 		}
 	}

pkg/recipe/metadata_store.go

@@ -636,11 +636,13 @@ func (s *MetadataStore) buildMixinConstraintCandidateIndex(candidateOverlays []s
 }
 
 // initBaseMergedSpec creates a copy of the base spec for overlay merging.
+// Validation is deep-cloned so downstream Merge calls cannot reach back
+// into the cached base ValidationConfig and mutate it.
 func (s *MetadataStore) initBaseMergedSpec() (RecipeMetadataSpec, []string) {
 	mergedSpec := RecipeMetadataSpec{
 		Constraints:   make([]Constraint, len(s.Base.Spec.Constraints)),
 		ComponentRefs: make([]ComponentRef, len(s.Base.Spec.ComponentRefs)),
-		Validation:    s.Base.Spec.Validation,
+		Validation:    cloneValidationConfig(s.Base.Spec.Validation),
 	}
 	copy(mergedSpec.Constraints, s.Base.Spec.Constraints)
 	copy(mergedSpec.ComponentRefs, s.Base.Spec.ComponentRefs)

pkg/recipe/metadata_test.go

@@ -1027,6 +1027,39 @@ func TestMergeValidationConfig(t *testing.T) {
 			t.Fatal("validation should be preserved from base when overlay has nil")
 		}
 	})
+
+	// Regression: a prior version of Merge aliased other.Validation when the
+	// destination's was nil. Subsequent merges then wrote through that alias
+	// into whichever overlay's cached ValidationConfig the alias pointed at,
+	// polluting it across calls. The fix deep-copies via cloneValidationConfig.
+	t.Run("merge does not alias source validation", func(t *testing.T) {
+		source := &ValidationConfig{
+			Conformance: &ValidationPhase{Checks: []string{"check-from-source"}},
+		}
+		first := RecipeMetadataSpec{
+			Validation: source,
+		}
+		second := RecipeMetadataSpec{
+			Validation: &ValidationConfig{
+				Deployment: &ValidationPhase{Checks: []string{"deployment-from-second"}},
+			},
+		}
+
+		// dest starts with nil Validation, so without the fix it would alias
+		// source. Merging second then plants Deployment via the alias into
+		// the source — corrupting subsequent reads of the source.
+		var dest RecipeMetadataSpec
+		dest.Merge(&first)
+		dest.Merge(&second)
+
+		if source.Deployment != nil {
+			t.Errorf("source.Deployment leaked to %v — Merge aliased the source ValidationConfig",
+				source.Deployment.Checks)
+		}
+		if dest.Validation.Conformance == source.Conformance {
+			t.Error("dest.Validation.Conformance aliases source.Conformance — phase pointers must be cloned")
+		}
+	})
 }
 
 func TestFinalizeRecipeResultIncludesValidation(t *testing.T) {

pkg/recipe/validation.go

@@ -14,6 +14,8 @@
 
 package recipe
 
+import "maps"
+
 // ValidationConfig defines validation phases and settings.
 type ValidationConfig struct {
 	// Readiness defines readiness validation phase settings.
@@ -59,3 +61,50 @@ type NodeSelection struct {
 	// ExcludeNodes lists node names to exclude from validation.
 	ExcludeNodes []string `json:"excludeNodes,omitempty" yaml:"excludeNodes,omitempty"`
 }
+
+// cloneValidationConfig returns a deep copy of v. RecipeMetadataSpec.Merge
+// uses this to avoid aliasing the source's nested phase pointers — without
+// it, successive merges mutate whichever cached overlay's ValidationConfig
+// the destination aliased.
+func cloneValidationConfig(v *ValidationConfig) *ValidationConfig {
+	if v == nil {
+		return nil
+	}
+	return &ValidationConfig{
+		Readiness:   cloneValidationPhase(v.Readiness),
+		Deployment:  cloneValidationPhase(v.Deployment),
+		Performance: cloneValidationPhase(v.Performance),
+		Conformance: cloneValidationPhase(v.Conformance),
+	}
+}
+
+// cloneValidationPhase returns a deep copy of p with independent backing
+// slices and a freshly allocated NodeSelection, so callers writing through
+// the clone cannot reach the source's cached metadata.
+func cloneValidationPhase(p *ValidationPhase) *ValidationPhase {
+	if p == nil {
+		return nil
+	}
+	out := *p
+	if p.Constraints != nil {
+		out.Constraints = make([]Constraint, len(p.Constraints))
+		copy(out.Constraints, p.Constraints)
+	}
+	if p.Checks != nil {
+		out.Checks = make([]string, len(p.Checks))
+		copy(out.Checks, p.Checks)
+	}
+	if p.NodeSelection != nil {
+		ns := *p.NodeSelection
+		if p.NodeSelection.Selector != nil {
+			ns.Selector = make(map[string]string, len(p.NodeSelection.Selector))
+			maps.Copy(ns.Selector, p.NodeSelection.Selector)
+		}
+		if p.NodeSelection.ExcludeNodes != nil {
+			ns.ExcludeNodes = make([]string, len(p.NodeSelection.ExcludeNodes))
+			copy(ns.ExcludeNodes, p.NodeSelection.ExcludeNodes)
+		}
+		out.NodeSelection = &ns
+	}
+	return &out
+}