From 14e522211d99e995a7202cb92e9e8f9b68aad282 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 16:17:25 +0100 Subject: [PATCH] chore(purchase): drop maskActor's dead guard and inert commitmentopts helpers maskActor rejects an empty actor before its `len(actor)-1 >= 0` check, so that `if` always held and its variable was never read. Replace the guard and loop with strings.Index, which returns the same first-'@' byte offset for every input, including invalid UTF-8. In commitmentopts, collectWithService was a pass-through to collect, and dedupeRaw's (durationSeconds, payment) dedupe is subsumed by collect's first-occurrence dedupe on the normalized (term, payment) key: both normalizers are pure functions of those two fields. probeOnePlanType now calls collect directly. Add TestSavingsPlansProber_DuplicatesAndNormalizedCollisionsPinned, which pins the exact ordered Probe output across two pages with raw duplicates, normalized collisions and dropped pairs, and TestMaskActor, which pins the log mask. Both pass before and after the change. Closes #325 Co-Authored-By: Claude Opus 5.5 --- internal/commitmentopts/probe.go | 40 ++--------------- internal/commitmentopts/probe_test.go | 63 +++++++++++++++++++++++++++ internal/purchase/approvals.go | 8 +--- internal/purchase/approvals_test.go | 23 ++++++++++ 4 files changed, 92 insertions(+), 42 deletions(-) diff --git a/internal/commitmentopts/probe.go b/internal/commitmentopts/probe.go index 2a0cc5e35..f96e68900 100644 --- a/internal/commitmentopts/probe.go +++ b/internal/commitmentopts/probe.go @@ -557,42 +557,10 @@ func (p *SavingsPlansProber) probeOnePlanType( if err != nil { return nil, err } - raw = dedupeRaw(raw) - // Use collect but override the service key to the per-product slug. - combos := collectWithService(serviceKey, raw) - return combos, nil -} - -// dedupeRaw removes duplicate (durationSeconds, payment) pairs so that -// collect does not see the same pair multiple times across an SP type's -// pages. SP offerings differ by ProductType (EC2, Lambda, Fargate ...) but -// share the same (duration, payment) matrix — we only need unique combos. -func dedupeRaw(raw []rawOffer) []rawOffer { - type key struct { - dur int64 - payment string - } - seen := make(map[key]struct{}, len(raw)) - out := make([]rawOffer, 0, len(raw)) - for _, r := range raw { - k := key{r.durationSeconds, r.payment} - if _, dup := seen[k]; dup { - continue - } - seen[k] = struct{}{} - out = append(out, r) - } - return out -} - -// collectWithService is like collect but uses the supplied serviceKey -// instead of the service embedded in each rawOffer (rawOffer has no -// service field; collect() uses the `service` argument, which is the -// per-prober name). Extracted to avoid a second copy of the -// normalization loop when the service name comes from outside the -// prober's Service() method (as it does for SP plan types). -func collectWithService(service string, raw []rawOffer) []Combo { - return collect(service, raw) + // collect dedupes on the normalized (term, payment) pair, which also + // collapses the same matrix repeated across an SP type's ProductTypes + // (EC2, Lambda, Fargate ...) and pages. + return collect(serviceKey, raw), nil } func (p *SavingsPlansProber) client(cfg aws.Config) SavingsPlansDescribeOfferings { diff --git a/internal/commitmentopts/probe_test.go b/internal/commitmentopts/probe_test.go index 48dbda377..d0d71aebb 100644 --- a/internal/commitmentopts/probe_test.go +++ b/internal/commitmentopts/probe_test.go @@ -651,6 +651,69 @@ func TestSavingsPlansProber_DedupedAcrossProductTypes(t *testing.T) { assert.Len(t, compute, 1, "duplicate (dur, pay) must collapse to one Combo") } +// TestSavingsPlansProber_DuplicatesAndNormalizedCollisionsPinned pins the +// exact, ordered output for one plan type whose pages repeat raw +// (durationSeconds, payment) pairs and also carry different raw spellings +// that normalize to the same (term, payment). collect() alone is the dedupe: +// the output keeps the first occurrence of each normalized pair, in page +// order, and drops unknown durations and legacy utilization tokens. Issue +// #325 removed a raw-pair pre-dedupe that ran before collect(); this test +// shows the output is unchanged without it. +func TestSavingsPlansProber_DuplicatesAndNormalizedCollisionsPinned(t *testing.T) { + const twoYearSeconds = 2 * 31536000 + sp := func(dur int64, pay string) savingsplanstypes.SavingsPlanOffering { + return spOffering(dur, savingsplanstypes.SavingsPlanPaymentOption(pay)) + } + pages := [][]savingsplanstypes.SavingsPlanOffering{ + { + sp(threeYearSeconds, "No Upfront"), + sp(oneYearSeconds, "All Upfront"), + sp(oneYearSeconds, "All Upfront"), // exact raw duplicate + sp(oneYearSeconds, "ALL_UPFRONT"), // normalizes to (1, all-upfront) + sp(twoYearSeconds, "All Upfront"), // unknown term: dropped + sp(twoYearSeconds, "All Upfront"), // duplicate of a dropped pair + sp(oneYearSeconds, "Heavy Utilization"), + sp(oneYearSeconds, "Heavy Utilization"), + }, + { + sp(threeYearSeconds, "no-upfront"), // normalized collision across pages + sp(oneYearSeconds, "All Upfront"), // exact raw duplicate across pages + sp(oneYearSeconds, "Partial Upfront"), // first new pair on page 2 + sp(threeYearSeconds, "NO_UPFRONT"), // another normalized collision + sp(oneYearSeconds, "partial_upfront"), // collides with the pair above + sp(threeYearSeconds, "Partial Upfront"), // second new pair on page 2 + }, + } + calls := 0 + fake := &fakeSavingsPlans{ + fn: func(in *savingsplans.DescribeSavingsPlansOfferingsInput) (*savingsplans.DescribeSavingsPlansOfferingsOutput, error) { + if in.PlanTypes[0] != savingsplanstypes.SavingsPlanTypeCompute { + return &savingsplans.DescribeSavingsPlansOfferingsOutput{}, nil + } + out := &savingsplans.DescribeSavingsPlansOfferingsOutput{SearchResults: pages[calls]} + calls++ + if calls < len(pages) { + out.NextToken = aws.String("page-2") + } + return out, nil + }, + } + p := &SavingsPlansProber{NewClient: func(_ aws.Config) SavingsPlansDescribeOfferings { return fake }} + + got, err := p.Probe(context.Background(), aws.Config{}) + require.NoError(t, err) + require.Equal(t, len(pages), calls, "both Compute pages must be read") + + const svc = "savings-plans-compute" + want := []Combo{ + {Provider: "aws", Service: svc, TermYears: 3, Payment: "no-upfront"}, + {Provider: "aws", Service: svc, TermYears: 1, Payment: "all-upfront"}, + {Provider: "aws", Service: svc, TermYears: 1, Payment: "partial-upfront"}, + {Provider: "aws", Service: svc, TermYears: 3, Payment: "partial-upfront"}, + } + assert.Equal(t, want, got, "first occurrence of each normalized pair, in page order") +} + // TestDefaultProbers_IncludesSavingsPlans verifies that SavingsPlansProber is // registered in DefaultProbers. func TestDefaultProbers_IncludesSavingsPlans(t *testing.T) { diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index b43544d12..5fb6479b2 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -98,12 +98,8 @@ func maskActor(actor string) string { if actor == "" { return "" } - if at := len(actor) - 1; at >= 0 { - for i, c := range actor { - if c == '@' { - return "***" + actor[i:] - } - } + if at := strings.Index(actor, "@"); at >= 0 { + return "***" + actor[at:] } // Not an email address -- log only the last 4 chars (e.g. a username). if len(actor) > 4 { diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index be7e9f2d4..75feeeeb8 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -1375,3 +1375,26 @@ func TestManager_ApproveAndExecute_FourEyesOn_SystemCreated(t *testing.T) { require.ErrorIs(t, gate(nil, " ", nil), ErrFourEyesDenied, "anonymous actor") require.ErrorIs(t, gate(&creatorID, "", &creatorID), ErrFourEyesDenied, "flag must not override a creator") } + +// TestMaskActor pins the PII mask written to the approval logs: everything +// before the first '@' is hidden, a non-email keeps only its last 4 bytes, +// and an empty actor is "". +func TestMaskActor(t *testing.T) { + cases := []struct{ in, want string }{ + {"", ""}, + {"approver@example.com", "***@example.com"}, + {"a@b", "***@b"}, + {"@example.com", "***@example.com"}, + {"trailing@", "***@"}, + {"first@second@example.com", "***@second@example.com"}, + {"josé@example.com", "***@example.com"}, + {"\xff\xfe@example.com", "***@example.com"}, + {"a", "****"}, + {"abcd", "****"}, + {"abcde", "...bcde"}, + {"service-account", "...ount"}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, maskActor(tc.in), "maskActor(%q)", tc.in) + } +}