Address review feedback: pre-mutation hashing, collect all errors, recovery scenario

- Move phase digest computation before controller mutations in buildBoxcutterPhases
- Collect all mismatched phases in verifyObservedPhases instead of failing on first
- Collect all mutable secrets in verifyReferencedSecretsImmutable
- Add determinism and multi-mismatch tests, empty stored error check
- Fix e2e scenario title double negative, add recovery scenario
- Rename dedup test for clarity

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: pedjak

internal/operator-controller/controllers/clusterobjectset_controller.go

@@ -152,22 +152,12 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl
 		return ctrl.Result{}, nil
 	}
 
-	phases, opts, err := c.buildBoxcutterPhases(ctx, cos)
+	phases, currentPhases, opts, err := c.buildBoxcutterPhases(ctx, cos)
 	if err != nil {
 		setRetryingConditions(cos, err.Error())
 		return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
 	}
 
-	currentPhases := make([]ocv1.ObservedPhase, 0, len(phases))
-	for _, phase := range phases {
-		hash, err := computePhaseDigest(phase)
-		if err != nil {
-			setRetryingConditions(cos, err.Error())
-			return ctrl.Result{}, fmt.Errorf("computing phase digest: %v", err)
-		}
-		currentPhases = append(currentPhases, ocv1.ObservedPhase{Name: phase.GetName(), Digest: hash})
-	}
-
 	if len(cos.Status.ObservedPhases) == 0 {
 		cos.Status.ObservedPhases = currentPhases
 	} else if err := verifyObservedPhases(cos.Status.ObservedPhases, currentPhases); err != nil {
@@ -488,10 +478,10 @@ func (c *ClusterObjectSetReconciler) listPreviousRevisions(ctx context.Context,
 	return previous, nil
 }
 
-func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]boxcutter.Phase, []boxcutter.RevisionReconcileOption, error) {
+func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]boxcutter.Phase, []ocv1.ObservedPhase, []boxcutter.RevisionReconcileOption, error) {
 	previous, err := c.listPreviousRevisions(ctx, cos)
 	if err != nil {
-		return nil, nil, fmt.Errorf("listing previous revisions: %w", err)
+		return nil, nil, nil, fmt.Errorf("listing previous revisions: %w", err)
 	}
 
 	// Convert to []client.Object for boxcutter
@@ -502,7 +492,7 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c
 
 	progressionProbes, err := buildProgressionProbes(cos.Spec.ProgressionProbes)
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, nil, err
 	}
 
 	opts := []boxcutter.RevisionReconcileOption{
@@ -511,9 +501,10 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c
 		boxcutter.WithAggregatePhaseReconcileErrors(),
 	}
 
-	phases := make([]boxcutter.Phase, 0)
+	phases := make([]boxcutter.Phase, 0, len(cos.Spec.Phases))
+	observedPhases := make([]ocv1.ObservedPhase, 0, len(cos.Spec.Phases))
 	for _, specPhase := range cos.Spec.Phases {
-		objs := make([]client.Object, 0)
+		objs := make([]client.Object, 0, len(specPhase.Objects))
 		for _, specObj := range specPhase.Objects {
 			var obj *unstructured.Unstructured
 			switch {
@@ -522,31 +513,42 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c
 			case specObj.Ref.Name != "":
 				resolved, err := c.resolveObjectRef(ctx, specObj.Ref)
 				if err != nil {
-					return nil, nil, fmt.Errorf("resolving ref in phase %q: %w", specPhase.Name, err)
+					return nil, nil, nil, fmt.Errorf("resolving ref in phase %q: %w", specPhase.Name, err)
 				}
 				obj = resolved
 			default:
-				return nil, nil, fmt.Errorf("object in phase %q has neither object nor ref", specPhase.Name)
+				return nil, nil, nil, fmt.Errorf("object in phase %q has neither object nor ref", specPhase.Name)
 			}
 
+			objs = append(objs, obj)
+		}
+
+		// Compute digest from the user-provided objects before controller mutations.
+		digest, err := computePhaseDigest(specPhase.Name, objs)
+		if err != nil {
+			return nil, nil, nil, fmt.Errorf("computing phase digest: %w", err)
+		}
+		observedPhases = append(observedPhases, ocv1.ObservedPhase{Name: specPhase.Name, Digest: digest})
+
+		// Apply controller mutations after digest computation.
+		for i, obj := range objs {
 			objLabels := obj.GetLabels()
 			if objLabels == nil {
 				objLabels = map[string]string{}
 			}
 			objLabels[labels.OwnerNameKey] = cos.Labels[labels.OwnerNameKey]
 			obj.SetLabels(objLabels)
 
-			switch cp := EffectiveCollisionProtection(cos.Spec.CollisionProtection, specPhase.CollisionProtection, specObj.CollisionProtection); cp {
+			switch cp := EffectiveCollisionProtection(cos.Spec.CollisionProtection, specPhase.CollisionProtection, specPhase.Objects[i].CollisionProtection); cp {
 			case ocv1.CollisionProtectionIfNoController, ocv1.CollisionProtectionNone:
 				opts = append(opts, boxcutter.WithObjectReconcileOptions(
 					obj, boxcutter.WithCollisionProtection(cp)))
 			}
-
-			objs = append(objs, obj)
 		}
+
 		phases = append(phases, boxcutter.NewPhase(specPhase.Name, objs))
 	}
-	return phases, opts, nil
+	return phases, observedPhases, opts, nil
 }
 
 // resolveObjectRef fetches the referenced Secret, reads the value at the specified key,
@@ -721,36 +723,48 @@ func markAsArchived(cos *ocv1.ClusterObjectSet) bool {
 }
 
 // computePhaseDigest computes a deterministic SHA-256 digest of a phase's
-// resolved content (name + objects). JSON serialization of unstructured
-// objects produces a canonical encoding with sorted map keys.
-func computePhaseDigest(phase boxcutter.Phase) (string, error) {
+// resolved content (name + objects) before any controller mutations.
+// JSON serialization of unstructured objects produces a canonical encoding
+// with sorted map keys.
+func computePhaseDigest(name string, objects []client.Object) (string, error) {
 	phaseMap := map[string]any{
-		"name":    phase.GetName(),
-		"objects": phase.GetObjects(),
+		"name":    name,
+		"objects": objects,
 	}
 	data, err := json.Marshal(phaseMap)
 	if err != nil {
-		return "", fmt.Errorf("marshaling phase %q: %w", phase.GetName(), err)
+		return "", fmt.Errorf("marshaling phase %q: %w", name, err)
 	}
 	h := sha256.Sum256(data)
 	return fmt.Sprintf("sha256:%x", h), nil
 }
 
 // verifyObservedPhases compares current per-phase digests against stored
-// digests. Returns an error naming the first mismatched phase.
+// digests. Returns an error listing all mismatched phases.
 func verifyObservedPhases(stored, current []ocv1.ObservedPhase) error {
+	if len(stored) == 0 {
+		return fmt.Errorf("stored observedPhases is unexpectedly empty")
+	}
+	if len(stored) != len(current) {
+		return fmt.Errorf("number of phases has changed (expected %d phases, got %d)", len(stored), len(current))
+	}
 	storedMap := make(map[string]string, len(stored))
 	for _, s := range stored {
 		storedMap[s.Name] = s.Digest
 	}
+	var mismatches []string
 	for _, c := range current {
 		if prev, ok := storedMap[c.Name]; ok && prev != c.Digest {
-			return fmt.Errorf(
-				"resolved content of phase %q has changed (expected digest %s, got %s): "+
-					"a referenced object source may have been deleted and recreated with different content",
-				c.Name, prev, c.Digest)
+			mismatches = append(mismatches, fmt.Sprintf(
+				"phase %q (expected digest %s, got %s)", c.Name, prev, c.Digest))
 		}
 	}
+	if len(mismatches) > 0 {
+		return fmt.Errorf(
+			"resolved content of %d phase(s) has changed: %s; "+
+				"a referenced object source may have been deleted and recreated with different content",
+			len(mismatches), strings.Join(mismatches, "; "))
+	}
 	return nil
 }
 
@@ -778,6 +792,7 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex
 		}
 	}
 
+	var mutableSecrets []string
 	for _, ref := range refs {
 		secret := &corev1.Secret{}
 		key := client.ObjectKey{Name: ref.name, Namespace: ref.namespace}
@@ -791,9 +806,14 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex
 		}
 
 		if secret.Immutable == nil || !*secret.Immutable {
-			return fmt.Errorf("secret %s/%s is not immutable: referenced secrets must have immutable set to true", ref.namespace, ref.name)
+			mutableSecrets = append(mutableSecrets, fmt.Sprintf("%s/%s", ref.namespace, ref.name))
 		}
 	}
 
+	if len(mutableSecrets) > 0 {
+		return fmt.Errorf("the following secrets are not immutable (referenced secrets must have immutable set to true): %s",
+			strings.Join(mutableSecrets, ", "))
+	}
+
 	return nil
 }

internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go

@@ -2,6 +2,7 @@ package controllers
 
 import (
 	"context"
+	"fmt"
 	"strings"
 	"testing"
 	"time"
@@ -16,7 +17,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/ptr"
-	"pkg.package-operator.run/boxcutter"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -243,10 +243,6 @@ func (m *mockTrackingCacheInternal) Source(h handler.EventHandler, predicates ..
 }
 
 func TestComputePhaseDigest(t *testing.T) {
-	makePhase := func(name string, objs ...client.Object) boxcutter.Phase {
-		return boxcutter.NewPhase(name, objs)
-	}
-
 	makeObj := func(apiVersion, kind, name string) *unstructured.Unstructured {
 		return &unstructured.Unstructured{
 			Object: map[string]interface{}{
@@ -258,45 +254,66 @@ func TestComputePhaseDigest(t *testing.T) {
 	}
 
 	t.Run("deterministic for same content", func(t *testing.T) {
-		phase := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1"))
-		hash1, err := computePhaseDigest(phase)
+		objs := []client.Object{makeObj("v1", "ConfigMap", "cm1")}
+		hash1, err := computePhaseDigest("deploy", objs)
 		require.NoError(t, err)
-		hash2, err := computePhaseDigest(phase)
+		hash2, err := computePhaseDigest("deploy", objs)
 		require.NoError(t, err)
 		assert.Equal(t, hash1, hash2)
 	})
 
+	t.Run("deterministic with many objects", func(t *testing.T) {
+		objs := make([]client.Object, 100)
+		for i := range objs {
+			objs[i] = makeObj("v1", "ConfigMap", fmt.Sprintf("cm-%d", i))
+		}
+		var first string
+		for i := range 100 {
+			digest, err := computePhaseDigest("deploy", objs)
+			require.NoError(t, err)
+			if i == 0 {
+				first = digest
+			} else {
+				assert.Equal(t, first, digest, "digest changed on iteration %d", i)
+			}
+		}
+	})
+
 	t.Run("different for different object content", func(t *testing.T) {
-		p1 := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1"))
-		p2 := makePhase("deploy", makeObj("v1", "ConfigMap", "cm2"))
-		h1, err := computePhaseDigest(p1)
+		h1, err := computePhaseDigest("deploy", []client.Object{makeObj("v1", "ConfigMap", "cm1")})
 		require.NoError(t, err)
-		h2, err := computePhaseDigest(p2)
+		h2, err := computePhaseDigest("deploy", []client.Object{makeObj("v1", "ConfigMap", "cm2")})
 		require.NoError(t, err)
 		assert.NotEqual(t, h1, h2)
 	})
 
 	t.Run("different for different phase names", func(t *testing.T) {
-		obj := makeObj("v1", "ConfigMap", "cm1")
-		p1 := makePhase("deploy", obj)
-		p2 := makePhase("crds", obj)
-		h1, err := computePhaseDigest(p1)
+		objs := []client.Object{makeObj("v1", "ConfigMap", "cm1")}
+		h1, err := computePhaseDigest("deploy", objs)
+		require.NoError(t, err)
+		h2, err := computePhaseDigest("crds", objs)
+		require.NoError(t, err)
+		assert.NotEqual(t, h1, h2)
+	})
+
+	t.Run("different order produces different digest", func(t *testing.T) {
+		obj1 := makeObj("v1", "ConfigMap", "cm1")
+		obj2 := makeObj("v1", "ConfigMap", "cm2")
+		h1, err := computePhaseDigest("deploy", []client.Object{obj1, obj2})
 		require.NoError(t, err)
-		h2, err := computePhaseDigest(p2)
+		h2, err := computePhaseDigest("deploy", []client.Object{obj2, obj1})
 		require.NoError(t, err)
 		assert.NotEqual(t, h1, h2)
 	})
 
 	t.Run("empty phase produces valid digest", func(t *testing.T) {
-		phase := makePhase("empty")
-		hash, err := computePhaseDigest(phase)
+		hash, err := computePhaseDigest("empty", nil)
 		require.NoError(t, err)
 		assert.NotEmpty(t, hash)
 	})
 
 	t.Run("digest has sha256 prefix", func(t *testing.T) {
-		phase := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1"))
-		digest, err := computePhaseDigest(phase)
+		digest, err := computePhaseDigest("deploy", []client.Object{makeObj("v1", "ConfigMap", "cm1")})
 		require.NoError(t, err)
 		assert.True(t, strings.HasPrefix(digest, "sha256:"), "digest should start with sha256: prefix, got %s", digest)
 	})
@@ -314,21 +331,42 @@ func TestVerifyObservedPhases(t *testing.T) {
 		current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:def456"}}
 		err := verifyObservedPhases(stored, current)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), `resolved content of phase "deploy" has changed`)
+		assert.Contains(t, err.Error(), `resolved content of 1 phase(s) has changed`)
+		assert.Contains(t, err.Error(), `phase "deploy"`)
+	})
+
+	t.Run("reports all mismatched phases", func(t *testing.T) {
+		stored := []ocv1.ObservedPhase{
+			{Name: "deploy", Digest: "sha256:aaa"},
+			{Name: "crds", Digest: "sha256:bbb"},
+		}
+		current := []ocv1.ObservedPhase{
+			{Name: "deploy", Digest: "sha256:xxx"},
+			{Name: "crds", Digest: "sha256:yyy"},
+		}
+		err := verifyObservedPhases(stored, current)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), `resolved content of 2 phase(s) has changed`)
+		assert.Contains(t, err.Error(), `phase "deploy"`)
+		assert.Contains(t, err.Error(), `phase "crds"`)
 	})
 
-	t.Run("passes when current has new phase not in stored", func(t *testing.T) {
+	t.Run("fails when phase count changes", func(t *testing.T) {
 		stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}}
 		current := []ocv1.ObservedPhase{
 			{Name: "deploy", Digest: "sha256:abc123"},
 			{Name: "crds", Digest: "sha256:def456"},
 		}
-		assert.NoError(t, verifyObservedPhases(stored, current))
+		err := verifyObservedPhases(stored, current)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "number of phases has changed")
 	})
 
-	t.Run("passes with empty stored", func(t *testing.T) {
+	t.Run("fails with empty stored", func(t *testing.T) {
 		current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}}
-		assert.NoError(t, verifyObservedPhases(nil, current))
+		err := verifyObservedPhases(nil, current)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "unexpectedly empty")
 	})
 }
 
@@ -428,7 +466,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) {
 		require.NoError(t, err)
 	})
 
-	t.Run("deduplicates refs to same secret", func(t *testing.T) {
+	t.Run("checks secret only once when referenced multiple times", func(t *testing.T) {
 		secret := immutableSecret.DeepCopy()
 		testClient := fake.NewClientBuilder().
 			WithScheme(testScheme).