fix(attestation): fix deduplication of policy evaluations (#3016)

Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>

Author: jiparis

pkg/attestation/crafter/crafter.go

@@ -19,9 +19,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"maps"
 	"net/url"
 	"os"
-	"reflect"
 	"slices"
 	"strings"
 	"time"
@@ -752,6 +752,13 @@ func (c *Crafter) addMaterial(ctx context.Context, m *schemaapi.CraftingSchema_M
 	return mt, nil
 }
 
+// policyEvalMatches returns true if two policy evaluations refer to the same policy
+// with the same arguments. It treats nil and empty maps as equivalent to handle
+// protojson round-trip serialization where empty maps are omitted.
+func policyEvalMatches(a, b *api.PolicyEvaluation) bool {
+	return proto.Equal(a.GetPolicyReference(), b.GetPolicyReference()) && maps.Equal(a.GetWith(), b.GetWith())
+}
+
 // EvaluateAttestationPolicies evaluates the attestation-level policies and stores them in the attestation state.
 // The phase parameter controls which policies are evaluated based on their attestation_phases spec field.
 func (c *Crafter) EvaluateAttestationPolicies(ctx context.Context, attestationID string, statement *intoto.Statement, phase policies.EvalPhase) error {
@@ -783,7 +790,7 @@ func (c *Crafter) EvaluateAttestationPolicies(ctx context.Context, attestationID
 	for _, ev := range policyEvaluations {
 		var duplicated bool
 		for _, existing := range filteredPolicyEvaluations {
-			if proto.Equal(existing.PolicyReference, ev.PolicyReference) && reflect.DeepEqual(existing.With, ev.With) {
+			if policyEvalMatches(existing, ev) {
 				duplicated = true
 				break
 			}
@@ -808,7 +815,7 @@ func (c *Crafter) EvaluateAttestationPolicies(ctx context.Context, attestationID
 		// Check if this attestation-level evaluation was re-evaluated in the current phase
 		var reEvaluated bool
 		for _, newEv := range policyEvaluations {
-			if proto.Equal(newEv.PolicyReference, ev.PolicyReference) && reflect.DeepEqual(newEv.With, ev.With) {
+			if policyEvalMatches(newEv, ev) {
 				reEvaluated = true
 				break
 			}

pkg/attestation/crafter/crafter_unit_test.go

@@ -1,5 +1,5 @@
 //
-// Copyright 2023 The Chainloop Authors.
+// Copyright 2023-2026 The Chainloop Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -18,15 +18,16 @@ package crafter
 import (
 	"os"
 	"path/filepath"
+	"slices"
 	"testing"
 	"time"
 
+	api "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/api/attestation/v1"
 	"github.com/go-git/go-git/v5"
+	"github.com/go-git/go-git/v5/config"
 	"github.com/go-git/go-git/v5/plumbing/object"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
-	"github.com/go-git/go-git/v5/config"
 	"github.com/stretchr/testify/suite"
 )
 
@@ -214,6 +215,106 @@ func (s *crafterUnitSuite) TestGitRepoHead() {
 	}
 }
 
+func (s *crafterUnitSuite) TestPolicyEvaluationDedup() {
+	// Simulate the protojson round-trip issue:
+	// - Init phase sets With = map[string]string{} (empty map)
+	// - protojson.Marshal omits empty maps
+	// - protojson.Unmarshal sets With = nil (absent field)
+	// - Push phase produces With = map[string]string{} again
+	// The dedup comparison must treat nil and empty map as equal.
+
+	policyRef := &api.PolicyEvaluation_Reference{
+		Name:   "source-commit",
+		Digest: "sha256:abc123",
+	}
+
+	testCases := []struct {
+		name        string
+		existing    []*api.PolicyEvaluation
+		newEvals    []*api.PolicyEvaluation
+		wantCount   int
+		description string
+	}{
+		{
+			name: "nil vs empty map With are deduplicated",
+			existing: []*api.PolicyEvaluation{
+				{Name: "source-commit", PolicyReference: policyRef, With: nil},
+			},
+			newEvals: []*api.PolicyEvaluation{
+				{Name: "source-commit", PolicyReference: policyRef, With: map[string]string{}},
+			},
+			wantCount:   1,
+			description: "after protojson round-trip, nil With should match empty map With",
+		},
+		{
+			name: "empty map vs empty map With are deduplicated",
+			existing: []*api.PolicyEvaluation{
+				{Name: "source-commit", PolicyReference: policyRef, With: map[string]string{}},
+			},
+			newEvals: []*api.PolicyEvaluation{
+				{Name: "source-commit", PolicyReference: policyRef, With: map[string]string{}},
+			},
+			wantCount:   1,
+			description: "identical empty maps should deduplicate",
+		},
+		{
+			name: "nil vs nil With are deduplicated",
+			existing: []*api.PolicyEvaluation{
+				{Name: "source-commit", PolicyReference: policyRef, With: nil},
+			},
+			newEvals: []*api.PolicyEvaluation{
+				{Name: "source-commit", PolicyReference: policyRef, With: nil},
+			},
+			wantCount:   1,
+			description: "both nil should deduplicate",
+		},
+		{
+			name: "different With args are not deduplicated",
+			existing: []*api.PolicyEvaluation{
+				{Name: "source-commit", PolicyReference: policyRef, With: map[string]string{"key": "val1"}},
+			},
+			newEvals: []*api.PolicyEvaluation{
+				{Name: "source-commit", PolicyReference: policyRef, With: map[string]string{"key": "val2"}},
+			},
+			wantCount:   2,
+			description: "different With values should not deduplicate",
+		},
+		{
+			name: "different policy references are not deduplicated",
+			existing: []*api.PolicyEvaluation{
+				{Name: "policy-a", PolicyReference: &api.PolicyEvaluation_Reference{Name: "policy-a"}, With: nil},
+			},
+			newEvals: []*api.PolicyEvaluation{
+				{Name: "policy-b", PolicyReference: &api.PolicyEvaluation_Reference{Name: "policy-b"}, With: nil},
+			},
+			wantCount:   2,
+			description: "different policies should both be kept",
+		},
+	}
+
+	for _, tc := range testCases {
+		s.Run(tc.name, func() {
+			all := slices.Concat(tc.existing, tc.newEvals)
+
+			var filtered []*api.PolicyEvaluation
+			for _, ev := range all {
+				var duplicated bool
+				for _, existing := range filtered {
+					if policyEvalMatches(existing, ev) {
+						duplicated = true
+						break
+					}
+				}
+				if !duplicated {
+					filtered = append(filtered, ev)
+				}
+			}
+
+			s.Len(filtered, tc.wantCount, tc.description)
+		})
+	}
+}
+
 func TestSuite(t *testing.T) {
 	suite.Run(t, new(crafterUnitSuite))
 }