[IAmJSD]

Root cause and fix

Both items are dead code. Deleting them changes no output. Neither was referenced by any test by name, so no existing test had to move.

• A05-019, maskActor (internal/purchase/approvals.go): actor == "" returns early, so len(actor)-1 >= 0 was always true and at was never read. I replaced the guard and the range loop with strings.Index(actor, "@"). range yields the byte offset of each rune start, and '@' (0x40) can never be part of a multi-byte UTF-8 sequence or get swallowed by an invalid one, so the offset is the same for every input, including invalid UTF-8.
• A05-018, collectWithService / dedupeRaw (internal/commitmentopts/probe.go): collectWithService was a one-line pass-through to collect. dedupeRaw kept the first occurrence of each raw (durationSeconds, payment) pair. collect keeps the first occurrence of each normalized (term, payment) pair and drops pairs that don't normalize. durationToTerm and normalizePayment depend only on those two raw fields, so a later copy of a raw pair always maps to a key that is already seen, or it is dropped. Removing it earlier therefore never changes collect's output or its order. probeOnePlanType now returns collect(serviceKey, raw) directly. Both helpers and their comments are gone.

Closes #325

Verification

All of this is unit, mock and fixture evidence. The SP prober runs against a fake DescribeSavingsPlansOfferings, and nothing calls AWS. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

Pin tests: pass before and after. This change preserves behavior, so there is no fail-before. The new tests pin the output, and they passed on unchanged origin/main (2f1dbca) before the deletion.

• TestSavingsPlansProber_DuplicatesAndNormalizedCollisionsPinned goes through the public SavingsPlansProber.Probe across two Compute pages. The pages contain exact raw duplicates (within a page and across pages), raw spellings that normalize to the same pair (All Upfront/ALL_UPFRONT, No Upfront/no-upfront/NO_UPFRONT, Partial Upfront/partial_upfront), duplicated unknown-term pairs and duplicated Heavy Utilization pairs. The test asserts the exact ordered []Combo.
• TestMaskActor pins the log mask for these cases: empty, plain email, leading/trailing @, multiple @, a non-ASCII local part, an invalid-UTF-8 local part, and short and long non-emails.

Exhaustive equivalence on origin/main (one-off, not committed). On main, a temporary in-package test checked reflect.DeepEqual(collect(s, dedupeRaw(raw)), collect(s, raw)). The alphabet was 55 raw offers: 5 durations (1y, 3y, 0, 2y, -1) × 11 payment spellings, including invalid and legacy ones. The test covered nil, the empty slice, every sequence up to length 3, and 300k random sequences up to length 40 drawn from small sub-alphabets to force repeats. Result: 469,457 checks, all equal. A second temporary test compared the old maskActor with the new one: exhaustive over a 16-byte pool that includes @, UTF-8 lead and continuation bytes and 0xff, up to length 4, plus 500k random byte strings. Result: 569,905 checks, all equal.

Mutation probes (reverted afterward):

• Removing the seen check in collect: TestSavingsPlansProber_Probe, _DedupedAcrossProductTypes and the new pin test fail.
• Keying collect on the raw payment instead of the normalized one: only the new pin test fails. The existing suite did not cover normalized collisions.
• strings.LastIndex in maskActor: TestMaskActor fails on first@second@example.com.

Commands (run on this head, from the repo root):

go build ./...                                            # ok
go vet ./...                                              # ok
go test ./internal/commitmentopts/ -count=1               # ok
go test ./internal/purchase/ -count=1                     # ok
go test ./internal/mocks/ -count=1                        # ok
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/commitmentopts/... ./internal/purchase/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/commitmentopts internal/purchase                              # silent
bash scripts/gosec-hook.sh <changed .go files>            # rc 0
make tidy-check; git diff --check                         # clean
git-secrets --scan on the changed files in a fresh isolated repo with .gitallowed   # clean
bash scripts/test-git-secrets-allowlist.sh                # 137 PASS, 0 FAIL

The existing tests in internal/commitmentopts, internal/purchase and internal/mocks pass unchanged.

Out of scope

• sortCombos in probe_test.go has a comment saying probers dedupe "by map keys so iteration order is otherwise nondeterministic". collect actually iterates the input slice, so its order is deterministic, and the new test relies on that. I left the helper and its comment alone.
• No frontend, migration or IaC changes.

Maintainer: please mirror the issue's triage labels onto this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/xs.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 58 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 2f1dbca
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 717c19e6-07f0-44ca-98e6-102bb0687ce9

Commits

Reviewing files that changed from the base of the PR and between 2f1dbca and 14e5222.

Files selected for processing: 4

• internal/commitmentopts/probe.go
• internal/commitmentopts/probe_test.go
• internal/purchase/approvals.go
• internal/purchase/approvals_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 14e5222

This is a removal with no behaviour change. The guard is dead, both helpers had a single caller, and the merged tree builds and passes.

Findings: none.

Attacked:

1. The maskActor guard is dead (internal/purchase/approvals.go:97-110).

   • Base has if at := len(actor)-1; at >= 0 {...} after the actor == "" early return. So len >= 1, the condition is always true, and at was never read.
   • nil cannot reach it, because the parameter is a Go string. The three call sites:
     • approvals.go:57, actor from ApproveExecution, which comes from handler_purchases.go:1044 (the session) or messages.go:183 (SQS msg.ActorEmail);
     • approvals.go:313, actorEmail, already TrimSpace'd and checked non-empty at :294;
     • approvals.go:397, actor in transitionApproveAndExecute.
   • The old loop's range byte index and strings.Index(actor, "@") give the same offset for every input: '@' is the single byte 0x40, and Go's decoder never folds 0x40 into an invalid sequence.

   Old and new were compared side by side for each input class:

Input	Old = new output
""	<anon>
" "	****
admin-api-key	...-key
system	...stem
scheduler	...uler
approver@example.com	***@example.com
Ops.Team+x@Corp.EXAMPLE	***@Corp.EXAMPLE
UUID 123e4567-...	...4000
"@", "@@", "a@", "@A"	***@, ***@@, ***@, ***@a
\xff@x, \xc3@x (invalid UTF-8)	***@x
é@x	***@x
日本@例え.jp	***@例え.jp
abc, abcd	****
ab@cd	***@cd
NUL bytes around '@'	***@\x00
"user\n@evil"	***@evil

Beyond the table, testing/quick.CheckEqual over 200k random strings and a 20 s native fuzz (148k execs) found no divergence.

2. The commitmentopts helpers:

   • dedupeRaw and collectWithService are called only from probe.go:560/562 on base. That is true in head and in the merged tree too: git grep over all *.go, including tests and internal/testutil/mocks.go, finds no other caller. The only remaining mentions are in docs/audits (A05-018).
   • Why removal is safe:
     • collect keeps the first occurrence of each (durationToTerm(dur), normalizePayment(pay)) key, and both normalisers are pure functions of the raw pair.
     • dedupeRaw always kept the first raw occurrence, which is the first occurrence of its normalised key.
     • So order and contents are unchanged.
   • Property probe: 200k random raw slices (valid, invalid and legacy term and payment spellings) compared collect(dedupeRaw(raw)) with collect(raw) using DeepEqual. All were equal.

3. Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 5 were caught:

   • M1, Index changed to LastIndex: TestMaskActor FAILS.
   • M2, actor[at:] changed to actor[at+1:]: TestMaskActor FAILS.
   • M3, len>4 changed to len>=4: TestMaskActor FAILS.
   • M4, collect's seen dedupe removed: the Pinned test FAILS.
   • M5, the service key hard-coded instead of serviceKey: TestSavingsPlansProber FAILS.

4. Fail-before: the new TestMaskActor and the Pinned test pass on base. That is expected for a pure refactor: they pin behaviour, and passing on base confirms the output is unchanged.

5. Blast radius: maskActor is unexported and only feeds %q log lines. No signatures changed, and main has not touched approvals.go or probe.go since the base.

Verified locally:

• git merge-tree --write-tree origin/main 14e52221 against origin/main 6af022f is clean.
• Merged export, with GOTOOLCHAIN=go1.26.10 and no cloud credentials:
  • go build ./... and go vet ./internal/purchase/ ./internal/commitmentopts/: OK.
  • go test -count=1 on both packages: ok.
• Head: gofmt -l on the 4 changed files is clean, and build and tests pass.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE 14e5222

Base: main (not stacked). git merge-tree --write-tree origin/main 14e52221 (origin/main 8f186a8): clean, no conflicts. On the merged tree, go test and go vet for internal/commitmentopts and internal/purchase both pass.

Fixes #325: yes, both checkboxes.

• A05-019: maskActor (internal/purchase/approvals.go:101) now uses strings.Index(actor, "@"). For valid and invalid UTF-8 alike, the byte offset of the first '@' matches what the old range loop found, because 0x40 is never part of a multi-byte sequence and invalid bytes decode as single-byte RuneError. strings was already imported.
• A05-018: dedupeRaw and collectWithService are deleted, and probeOnePlanType returns collect(serviceKey, raw) directly. This is equivalent: collect (probe.go:105) keys on (durationToTerm(dur), normalizePayment(payment)). Both are pure functions of the two raw fields (normalize.go:18, :42), so any later raw duplicate maps either to a key collect has already seen or to a dropped pair. The first-occurrence order is preserved. No other references remain outside docs/audits.

Findings: none blocking.

• nit internal/commitmentopts/probe.go:560 - the new comment says collect dedupes "the same matrix repeated across an SP type's ProductTypes". That is accurate. The old dedupeRaw comment that made the same point is gone, so no information is lost.

Attacked:

• Read collect, durationToTerm, normalizePayment and stripNonAlnumLower to confirm they are pure and that the dedupe key comes only from (durationSeconds, payment).
• Ran go test ./internal/commitmentopts/ and go test ./internal/purchase/ -run 'TestMaskActor|Approv|FourEyes' on the head: ok.
• Mutation probe 1: keyed collect on the raw payment instead of the normalized one. Only the new TestSavingsPlansProber_DuplicatesAndNormalizedCollisionsPinned FAILS, which shows the new test covers a gap. Restored.
• Mutation probe 2: disabled the seen check in collect. TestRDSProber_Probe, TestSavingsPlansProber_Probe, _DedupedAcrossProductTypes and the new pin test all FAIL. Restored.
• Mutation probe 3: strings.LastIndex in maskActor. TestMaskActor FAILS. Restored.
• Mutation probe 4: changed the non-email length threshold from > 4 to > 5. TestMaskActor FAILS (the abcde case). Restored.
• Checked that the restored files are byte-identical to the head.
• Scope: 4 files, 92+/42-. Production changes are limited to the two deletions named in the issue. The test additions pin behavior and do not change it.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 14e522211d99e995a7202cb92e9e8f9b68aad282

Blocking:

• none

Non-blocking:

• none. The issue's "drop both helpers with their tests" has nothing else to drop: on main, dedupeRaw and collectWithService are referenced only by probe.go itself and by the audit doc.

Attacked:

• Equivalence by reading the code. collect keys on (durationToTerm(dur), normalizePayment(pay)), and both are pure functions of the same two raw fields that dedupeRaw keyed on. It iterates the input slice in order (no map iteration) and has no side effects. So a later raw duplicate always maps to an already-seen key or gets dropped, and removing the pre-dedupe can't change content or order. maskActor: range yields byte offsets of rune starts, and 0x40 can never sit inside a multi-byte or invalid UTF-8 sequence, so strings.Index(actor, "@") returns the same offset for every input. strings was already imported.
• Pin tests: with main's (merge-base) probe.go and approvals.go put back under the new tests, TestSavingsPlansProber* and TestMaskActor PASS, which confirms the change preserves behavior (no fail-before is possible). Mutation: keying collect on the raw payment makes TestSavingsPlansProber_DuplicatesAndNormalizedCollisionsPinned FAIL, so the pin has teeth. Reverted afterwards.
• Commands: go build ./..., go vet with and without -tags integration on internal/commitmentopts and internal/purchase, and go test -race -short -count=1 on both (ok).

Body-stale: none

_{Reviewer: rB3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 14e522211d99e995a7202cb92e9e8f9b68aad282 merged onto main (5aa3df5b), together with #1155, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1270, #1272, #1275, #1283, #1286, #1293, #1297, #1299, #1300, #1301, #1304, #1307, #1311, #1313, #1325 in that order (combined tree f3a3331f, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 14e52221 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 14e522211d99e995a7202cb92e9e8f9b68aad282.