fix: match dashboard hasDiff and exclude deleted resources from fixes (#6)

* 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.

* fix: align comment formatting with dashboard

- formatCost: negative values render as `-$X` (sign before currency
  symbol) and integer parts get thousand-separators (e.g. `-$1,616`).
- formatCarbonWithExample: take a pluralize flag; the run-level
  summary uses it so we say `avoids`/`emits` instead of `avoid`/`emit`.
- formatNumber: rewrite to use the dashboard's "decimal places to
  surface the first non-zero digit" rule. Whole numbers render with no
  decimals (`16`), fractional values keep needed precision (`335.7`),
  and thousand-separators are applied.
- processCostChangesAndBudgets: sort budget-tag-note keys before
  joining to fix non-deterministic ordering.
- Template: blank lines around the governance sentence so HTML blocks
  that follow are recognised as block-level by GitHub's markdown parser.

Author: liamcervante

pkg/vcs/comment/budgets.go

@@ -2,6 +2,7 @@ package comment
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/infracost/go-proto/pkg/rat"
@@ -101,7 +102,11 @@ func (data *Data) processCostChangesAndBudgets(inputs *Inputs) {
 	// Build tag note.
 	var keys []string
 	for k := range tagKeys {
-		keys = append(keys, escapeAndFormatCode(k))
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	for i, k := range keys {
+		keys[i] = escapeAndFormatCode(k)
 	}
 	inputs.BudgetTagNote = fmt.Sprintf(
 		"Note: Tag-based actual costs are calculated using service provider cost data for the current budget period for all resources tagged with %s.",

pkg/vcs/comment/costs.go

@@ -3,9 +3,9 @@ package comment
 import (
 	"fmt"
 	"sort"
+	"strings"
 
 	"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 +120,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.
@@ -205,15 +199,51 @@ func formatPercentChange(oldCost, newCost *rat.Rat) string {
 // formatCost formats a cost value with currency symbol, matching the
 // dashboard's autoDecimals behavior: values with absolute value >= 1 (or zero)
 // are rounded to whole numbers, smaller values show 2 decimal places.
+// Negative values render with the sign before the currency symbol (e.g.
+// "-$1,616") and integer parts are formatted with thousand separators.
 // See: dashboard/api/src/utils/format.ts CurrencyFormatter.formatCost
 func formatCost(d *rat.Rat, currency string) string {
+	sym := currencySymbol(currency)
 	if d == nil || d.IsZero() {
-		return currencySymbol(currency) + "0"
+		return sym + "0"
+	}
+	abs := d.Abs()
+	decimals := 0
+	if abs.LessThan(rat.New(1)) {
+		decimals = 2
+	}
+	formatted := withThousandSeparators(abs.StringFixed(decimals))
+	if d.LessThan(rat.Zero) {
+		return "-" + sym + formatted
+	}
+	return sym + formatted
+}
+
+// withThousandSeparators inserts commas every three digits in the integer
+// portion of a numeric string. The input is expected to be a decimal string
+// like "1234567" or "1234.56".
+func withThousandSeparators(s string) string {
+	intPart := s
+	frac := ""
+	if dot := strings.IndexByte(s, '.'); dot >= 0 {
+		intPart = s[:dot]
+		frac = s[dot:]
+	}
+	if len(intPart) <= 3 {
+		return intPart + frac
 	}
-	if d.Abs().GreaterThanOrEqual(rat.New(1)) {
-		return currencySymbol(currency) + d.StringFixed(0)
+	var b strings.Builder
+	first := len(intPart) % 3
+	if first == 0 {
+		first = 3
 	}
-	return currencySymbol(currency) + d.StringFixed(2)
+	b.WriteString(intPart[:first])
+	for i := first; i < len(intPart); i += 3 {
+		b.WriteByte(',')
+		b.WriteString(intPart[i : i+3])
+	}
+	b.WriteString(frac)
+	return b.String()
 }
 
 // safeSub returns a - b, handling nils as zero.
@@ -242,4 +272,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/format_test.go

@@ -0,0 +1,69 @@
+package comment
+
+import (
+	"testing"
+
+	"github.com/infracost/go-proto/pkg/rat"
+)
+
+func TestFormatCost(t *testing.T) {
+	tests := []struct {
+		name     string
+		value    *rat.Rat
+		currency string
+		want     string
+	}{
+		{name: "nil", value: nil, currency: "USD", want: "$0"},
+		{name: "zero", value: rat.Zero, currency: "USD", want: "$0"},
+		{name: "small fraction", value: ratFrom("0.5"), currency: "USD", want: "$0.50"},
+		{name: "small fraction negative", value: ratFrom("-0.5"), currency: "USD", want: "-$0.50"},
+		{name: "whole number", value: rat.New(50), currency: "USD", want: "$50"},
+		{name: "whole number negative", value: rat.New(-50), currency: "USD", want: "-$50"},
+		{name: "thousands", value: rat.New(1616), currency: "USD", want: "$1,616"},
+		{name: "thousands negative", value: rat.New(-1616), currency: "USD", want: "-$1,616"},
+		{name: "millions", value: rat.New(1234567), currency: "USD", want: "$1,234,567"},
+		{name: "millions negative", value: rat.New(-1234567), currency: "USD", want: "-$1,234,567"},
+		{name: "EUR", value: rat.New(1500), currency: "EUR", want: "€1,500"},
+		{name: "GBP negative", value: rat.New(-2500), currency: "GBP", want: "-£2,500"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := formatCost(tt.value, tt.currency); got != tt.want {
+				t.Errorf("formatCost(%v, %q) = %q, want %q", tt.value, tt.currency, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestFormatNumber(t *testing.T) {
+	tests := []struct {
+		v    float64
+		want string
+	}{
+		{v: 0, want: "0"},
+		{v: 1, want: "1"},
+		{v: 16, want: "16"},
+		{v: 16.0, want: "16"},
+		{v: 335.7, want: "335.7"},
+		{v: 1234.5, want: "1,234.5"},
+		{v: 1234567, want: "1,234,567"},
+		{v: 0.5, want: "0.5"},
+		{v: 0.05, want: "0.05"},
+		{v: 0.005, want: "0.005"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.want, func(t *testing.T) {
+			if got := formatNumber(tt.v); got != tt.want {
+				t.Errorf("formatNumber(%v) = %q, want %q", tt.v, got, tt.want)
+			}
+		})
+	}
+}
+
+func ratFrom(s string) *rat.Rat {
+	r, err := rat.NewFromString(s)
+	if err != nil {
+		panic(err)
+	}
+	return r
+}