fix: match dashboard's hasDiff and exclude deleted resources from fixes

Switches the cost section visibility (hasDiff) and per-project diff check
to key off DiffBreakdown.Resources rather than per-resource Action, matching
the dashboard's projectHasDiff. ProjectResult.Resources is no longer read
anywhere in the comment package and has been removed.

Also filters fixed-issue counts to exclude resources that no longer appear
in any current policy result (passing or failing) — claiming a "fix" for a
resource that has disappeared, or whose policy is no longer applicable, is
misleading.

Author: liamcervante

pkg/vcs/comment/costs.go

@@ -5,7 +5,6 @@ import (
 	"sort"
 
 	"github.com/infracost/go-proto/pkg/rat"
-	"github.com/infracost/proto/gen/go/infracost/provider"
 )
 
 // processProjectCosts builds the project costs table entries, determines which
@@ -120,17 +119,11 @@ func (data *Data) usageCostsMessage() string {
 	return fmt.Sprintf("*Usage costs can be estimated by updating %s, see %s for other options.", cloudSettingsStr, usageDocsStr)
 }
 
-// projectHasDiff returns true if any resource in the project has a non-NOOP action.
+// projectHasDiff returns true if the project's pre-computed diff breakdown
+// contains any resource entries. Matches the dashboard's projectHasDiff in
+// dashboard/api/src/services/templates/helpers.ts.
 func projectHasDiff(project ProjectResult) bool {
-	for _, r := range project.Resources {
-		switch r.Action {
-		case provider.ResourceAction_CREATE,
-			provider.ResourceAction_MODIFY,
-			provider.ResourceAction_DELETE:
-			return true
-		}
-	}
-	return false
+	return project.DiffBreakdown != nil && len(project.DiffBreakdown.Resources) > 0
 }
 
 // costChangeOpts controls formatting of cost change strings.
@@ -242,4 +235,4 @@ func truncateMiddle(s string, maxLen int) string {
 	}
 	half := (maxLen - 3) / 2
 	return s[:half] + "..." + s[len(s)-half:]
-}
+}

pkg/vcs/comment/data.go

@@ -183,11 +183,6 @@ type ProjectResult struct {
 	// Available from runner's Project.Diff.
 	DiffBreakdown *CostBreakdown
 
-	// Resources contains the current resources with their costs from the
-	// provider output. Each resource has an Action field (CREATE, MODIFY,
-	// DELETE, NOOP) indicating whether it changed between runs.
-	Resources []*provider.Resource
-
 	// Diagnostics contains any parsing errors or warnings for this project.
 	Diagnostics []*diagnostic.Diagnostic
 

pkg/vcs/comment/deleted_resources.go

@@ -0,0 +1,62 @@
+package comment
+
+import "github.com/infracost/proto/gen/go/infracost/provider"
+
+// buildDeletedResources returns the set of resource IDs/addresses that were
+// failing in the previous run but no longer appear in any current policy
+// result (passing or failing) across finops, security, and tagging policies.
+//
+// Such a resource was either deleted, or stopped being applicable to every
+// policy that previously covered it. In either case it shouldn't be counted
+// as a "fix" in the comment — claiming we fixed an issue on a resource that
+// has disappeared (or whose policy has retired) is misleading.
+//
+// FinOps results key resources by parser-supplied ID; tagging results key by
+// address. They share the same map: keys are strings and any collision (when
+// a parser uses the address as the ID) is harmless.
+func (data *Data) buildDeletedResources() map[string]bool {
+	current := make(map[string]bool)
+	addFinopsCurrent(current, data.FinOpsPolicyResults)
+	addFinopsCurrent(current, data.SecurityPolicyResults)
+	for _, r := range data.TaggingPolicyResults {
+		for _, fr := range r.FailingResources {
+			current[fr.Address] = true
+		}
+		for _, pr := range r.PassingResources {
+			current[pr.Address] = true
+		}
+	}
+
+	deleted := make(map[string]bool)
+	addFinopsDeleted(deleted, current, data.PreviousFinOpsPolicyResults)
+	addFinopsDeleted(deleted, current, data.PreviousSecurityPolicyResults)
+	for _, r := range data.PreviousTaggingPolicyResults {
+		for _, fr := range r.FailingResources {
+			if !current[fr.Address] {
+				deleted[fr.Address] = true
+			}
+		}
+	}
+	return deleted
+}
+
+func addFinopsCurrent(current map[string]bool, results []*provider.FinopsPolicyResult) {
+	for _, r := range results {
+		for _, fr := range r.FailingResources {
+			current[fr.Id] = true
+		}
+		for _, id := range r.PassingResourceIds {
+			current[id] = true
+		}
+	}
+}
+
+func addFinopsDeleted(deleted, current map[string]bool, previous []*provider.FinopsPolicyResult) {
+	for _, r := range previous {
+		for _, fr := range r.FailingResources {
+			if !current[fr.Id] {
+				deleted[fr.Id] = true
+			}
+		}
+	}
+}

pkg/vcs/comment/fixed_issues.go

@@ -14,9 +14,11 @@ import (
 func (data *Data) processFixedIssues(inputs *Inputs, finopsIndex, securityIndex policyFailureIndex, taggingIndex taggingFailureIndex) {
 	counts := make([]FixedIssueCount, 0, len(data.PreviousFinOpsPolicyResults)+len(data.PreviousSecurityPolicyResults)+len(data.TaggingPolicyResults))
 
-	counts = append(counts, finopsFixedIssueCounts(data.PreviousFinOpsPolicyResults, finopsIndex)...)
-	counts = append(counts, finopsFixedIssueCounts(data.PreviousSecurityPolicyResults, securityIndex)...)
-	counts = append(counts, taggingFixedIssueCounts(data.TaggingPolicyResults, taggingIndex)...)
+	deleted := data.buildDeletedResources()
+
+	counts = append(counts, finopsFixedIssueCounts(data.PreviousFinOpsPolicyResults, finopsIndex, deleted)...)
+	counts = append(counts, finopsFixedIssueCounts(data.PreviousSecurityPolicyResults, securityIndex, deleted)...)
+	counts = append(counts, taggingFixedIssueCounts(data.TaggingPolicyResults, taggingIndex, deleted)...)
 
 	// Sort by count descending, then policy name ascending.
 	sort.Slice(counts, func(i, j int) bool {
@@ -44,8 +46,11 @@ func (data *Data) processFixedIssues(inputs *Inputs, finopsIndex, securityIndex
 }
 
 // finopsFixedIssueCounts computes fixed issue counts for FinOps/security policies.
-// A fixed issue is a resource that was failing previously but is no longer failing.
-func finopsFixedIssueCounts(previousResults []*provider.FinopsPolicyResult, index policyFailureIndex) []FixedIssueCount {
+// A fixed issue is a resource that was failing previously, is no longer failing,
+// and still appears in some current policy result. Resources in the deleted set
+// (gone from every current policy's scope) are excluded so we don't claim a fix
+// for a resource that has disappeared.
+func finopsFixedIssueCounts(previousResults []*provider.FinopsPolicyResult, index policyFailureIndex, deleted map[string]bool) []FixedIssueCount {
 	if len(previousResults) == 0 {
 		return nil
 	}
@@ -59,9 +64,13 @@ func finopsFixedIssueCounts(previousResults []*provider.FinopsPolicyResult, inde
 		currentIDs := index.current[prev.PolicySlug]
 		fixed := 0
 		for _, r := range prev.FailingResources {
-			if currentIDs == nil || !currentIDs[r.Id] {
-				fixed++
+			if currentIDs != nil && currentIDs[r.Id] {
+				continue
+			}
+			if deleted[r.Id] {
+				continue
 			}
+			fixed++
 		}
 
 		if fixed > 0 {
@@ -76,8 +85,10 @@ func finopsFixedIssueCounts(previousResults []*provider.FinopsPolicyResult, inde
 }
 
 // taggingFixedIssueCounts computes fixed issue counts for tagging policies.
-// A fixed issue is an address that was failing previously but is no longer failing.
-func taggingFixedIssueCounts(results []event.TaggingPolicyResult, index taggingFailureIndex) []FixedIssueCount {
+// A fixed issue is an address that was failing previously, is no longer
+// failing, and still appears in some current policy result. Addresses in the
+// deleted set are excluded — see finopsFixedIssueCounts for rationale.
+func taggingFixedIssueCounts(results []event.TaggingPolicyResult, index taggingFailureIndex, deleted map[string]bool) []FixedIssueCount {
 	var counts []FixedIssueCount
 
 	for _, result := range results {
@@ -93,9 +104,13 @@ func taggingFixedIssueCounts(results []event.TaggingPolicyResult, index taggingF
 		currentAddrs := index.current[result.TagPolicyID]
 		fixed := 0
 		for addr := range prevAddrs {
-			if currentAddrs == nil || !currentAddrs[addr] {
-				fixed++
+			if currentAddrs != nil && currentAddrs[addr] {
+				continue
+			}
+			if deleted[addr] {
+				continue
 			}
+			fixed++
 		}
 
 		if fixed > 0 {

pkg/vcs/comment/template.go

@@ -6,8 +6,6 @@ import (
 	"fmt"
 	"strings"
 	"text/template"
-
-	"github.com/infracost/proto/gen/go/infracost/provider"
 )
 
 const (
@@ -109,25 +107,12 @@ func (data *Data) processDisplayCosts(inputs *Inputs, hasGuardrail bool) {
 	}
 
 	hasError := len(inputs.ProjectErrors) > 0
+	hasUnsupported := data.Summary.TotalUnsupportedResources > 0
 	hasDiff := false
-	hasUnsupported := false
 
 	for _, project := range data.Projects {
-		for _, r := range project.Resources {
-			switch r.Action {
-			case provider.ResourceAction_CREATE,
-				provider.ResourceAction_MODIFY,
-				provider.ResourceAction_DELETE:
-				hasDiff = true
-			}
-			if !r.IsSupported && !r.IsFree {
-				hasUnsupported = true
-			}
-			if hasDiff && hasUnsupported {
-				break
-			}
-		}
-		if hasDiff && hasUnsupported {
+		if projectHasDiff(project) {
+			hasDiff = true
 			break
 		}
 	}