[IAmJSD]

Stacked on #1269 (fix/issue-339). This branch starts from #1269's first commit, so until #1269 merges the diff also shows that commit. Review only the last commit, 73bdb19d. It is split out because it needs a migration, while #1269 doesn't.

Refs #339

Root cause (audit finding A06-019)

commitmentTypeFor (internal/analytics) only recognised a service named exactly "SavingsPlans", which purchase_history never stores. So every snapshot was labelled "RI", including AWS Savings Plans (savingsplans / savings-plans-<type>) and GCP committed-use discounts. The audit only mentioned GCP; AWS Savings Plans were mislabelled too.

Fix

• New internal/analytics/commitment_type.go: derives the type from (provider, service) using the pkg/common CommitmentType vocabulary. The collector uses it, and the old literal match in postgres_analytics.go is removed.
• Migration 000145 (savings_snapshots_commitment_type_vocabulary): widens the savings_snapshots CHECK to the new vocabulary and relabels existing rows. The CHECK still accepts the legacy RI and SavingsPlan literals, so an old replica can keep writing during a rolling deploy.
• Down migration: restores the old CHECK. Rows are relabelled back, so a rollback turns GCP and Savings Plans rows into RI again.

Migration order

000145 is after the open chain 000135–000143 (#1116 → … → #1241), after 000144 (#1253) and before 000146 (#1268). Merge it after #1253 and before #1268. Renumbering is a normal commit if another PR takes the number.

Verification

Branch evidence: unit tests (collector_test.go, postgres_analytics_test.go, mock test updates), a real-Postgres test (commitment_type_db_test.go) and a migration test (000145_..._test.go) covering the relabel and the CHECK.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Review manually

• Vocabulary: whether pkg/common's CommitmentType labels are the right ones to store.
• Legacy literals: keeping RI and SavingsPlan in the CHECK during the rollout, and when to remove them in a later migration.
• Rollback: rolling back relabels GCP rows as RI.

Labels to mirror from #339: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/m, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

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

[IAmJSD]

CI fix pushed. New head: ba15e4ae.

Root cause. The unit test TestNoUnfailableMockAssertions (internal/mocks) could not resolve the receiver type of two assertions this branch inherited from #1269's older head 28b9a411:

• internal/purchase/empty_selection_notify_test.go:105: mockEmail came from allowFailedNotification(new(MockEmailSender)).
• internal/purchase/approvals_test.go:307: sender came from the multi-value newApproveManager(t).

The guard is syntactic, so it reports a call result it cannot type as unchecked.

Fix. #1269 (fix/issue-339) already fixed both sites in 75ea9b92:

• In empty_selection_notify_test.go it declares mockEmail := new(MockEmailSender) and then calls allowFailedNotification(mockEmail). The assertion is unchanged.
• In approvals_test.go the no-confirmation check now relies on having no SendPurchaseConfirmation expectation registered, so any send fails the test.

Because this PR is stacked on #1269, I merged #1269's current head 63adf280 into this branch instead of writing a second, diverging fix. That keeps the two branches identical in those files. #1269 itself passes the guard, so it needed no change.

Other changes: I merged origin/main twice as plain merges with no conflicts. The 000145 migration number is unchanged.

Checks run locally on the new head:

• go build ./..., go vet ./..., go vet -tags integration ./... and the tests/e2e vet all pass.
• golangci-lint v2.10.1 reports 0 issues on internal/analytics, internal/database/postgres/migrations, internal/purchase and internal/server, both with and without --build-tags integration.
• gocyclo -over 10 on the changed non-test files reports nothing.
• go test -race -short passes on all four packages, and TestNoUnfailableMockAssertions passes.
• go test -race -tags integration:
  • internal/purchase passes.
  • internal/server passes.
  • internal/database/postgres/migrations passes, including TestMigration_SavingsSnapshotsCommitmentTypeVocabulary.
  • internal/analytics passes.
  • On this heavily loaded host, some tests first failed only because their Postgres container did not start within its 30s wait. I reran those tests until they passed.
• The secrets scan of the changed files prints "clean; planted key flagged".
• bash scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.

[IAmJSD]

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

Blocking:

• internal/analytics/collector_test.go:240 (after merging with current main): undefined: common. Main's test(analytics): fold the hand-rolled ConfigStore mock onto mocks.MockConfigStore #1254 (7749401a, merged after this branch last took main at ba15e4ae) folded the hand-rolled config-store mock onto mocks.MockConfigStore, and that change removed the cloud-commitments-go/pkg/common import from this file. git merge-tree still reports a textually clean merge, so nothing flags it, but the new assertion string(common.CommitmentReservedInstance) no longer compiles. The PR shows mergeStateStatus: CLEAN with a green CI Success from the older merge ref, so merging now would break go test/go vet for internal/analytics on main. Fix: git merge origin/main and restore the import (or compare against the literal). I re-added the import in my scratch tree and everything below passes.

Non-blocking:

• Migration ordering: no test or CI check stops a lower-numbered migration from landing after a higher one. If 000145 is applied before fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239/perf(api): page recommendations from an ID-ordered keyset scan instead of the full list #1240/feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241/fix(db): contract the canceled rename: normalize, narrow the status CHECKs, stop using cancelled_by #1253 (000140-000144) merge, migrate up on that database will never run them. The body says to merge after fix(db): contract the canceled rename: normalize, narrow the status CHECKs, stop using cancelled_by #1253, and merge order is the only safeguard, so please hold this PR until 000140-000144 are in.
• 000145.up.sql: the relabel UPDATE rewrites every savings_snapshots row while holding ACCESS EXCLUSIVE, and ADD CONSTRAINT ... CHECK rescans every partition under the same lock. The table is one row per bucket per collection, so this should be seconds. On a large tenant it blocks dashboard reads for the duration. NOT VALID + VALIDATE would shorten the exclusive window if that ever matters.
• Rows written by an old replica after the migration keep RI/SavingsPlan forever, because nothing re-runs the relabel. monthly_savings_summary doesn't group by commitment_type (000078), so totals are unaffected, and only the per-row label is off for that short window. A later migration that drops the legacy literals from the CHECK has to relabel them first anyway.
• Go and SQL precedence differ in one place. SQL maps commitment_type='SavingsPlan' before checking provider='gcp', while Go puts GCP first. The two only disagree for a GCP row with the legacy SavingsPlan label, which the old mapper couldn't produce.

Attacked:

• I reviewed the effective change, meaning the merge of the head into current main (eb9e480). fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health #1269 is squash-merged, so only the 10 analytics/migration files remain.
• Readers of commitment_type: only the analytics store reads it back. The frontend has no reader (the commitment_type hits are Archera DTOs). No materialized view groups by it. The PK is (id, timestamp), so the relabel can't collide. isSavingsPlanService matches common.IsSavingsPlan's slugs plus savings-plans, and the SQL IN (...) list is the same set.
• Fail-before:
  • The old commitmentTypeFor/validateCommitmentType swapped in makes TestCollectorCollect (2 subtests), TestCommitmentTypeLogic and TestValidateCommitmentType fail, in both unit and integration builds.
  • An empty 000145 up migration makes TestMigration_SavingsSnapshotsCommitmentTypeVocabulary fail. That test also covers down, then up again, in both a named partition and the default partition.
• Checks run on the merged tree with the import restored:
  • go build ./... and go vet (with and without -tags integration) on internal/analytics and the migrations package
  • go test -race -short -count=1 on both packages
  • go test -race -tags integration -count=1 ./internal/analytics/
  • the 000145 test plus the up/down/idempotency tests in the migrations package
  • golangci-lint, with and without the integration tag: 0 issues
  • gocyclo: silent

Body-stale:

• "Stacked on fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health #1269 ... until fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health #1269 merges the diff also shows that commit": fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health #1269 is merged.
• "after the open chain 000135–000143 (fix(auth): bind group account scope entries to account IDs #1116 → … → feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241)": fix(auth): bind group account scope entries to account IDs #1116 now holds 000149, and main has no 000135. The open migrations below this one are 000140-000144 (fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239, perf(api): page recommendations from an ID-ordered keyset scan instead of the full list #1240, feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241, fix(db): contract the canceled rename: normalize, narrow the status CHECKs, stop using cancelled_by #1253).
• Refs #339: fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health #1269 fixed A05-015 and A06-009, and this PR fixes A06-019, the last item. With both merged, this PR could use Closes #339, or someone should close the issue by hand.

_{Reviewer: rB20. 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]

Addressed the review at ba15e4ae: merged origin/main and restored the pkg/common import that #1254 removed from collector_test.go (this branch's new assertion uses it). New head 337d25e4. Build, vet (both tag sets), analytics race unit and integration tests and golangci-lint pass; secrets scan clean. Its migration 000145 still has to merge after 000140-000144.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• Rolling deploy. 000145_..._vocabulary.up.sql relabels only the rows that exist at migration time. Any RI or SavingsPlan row that an old replica writes after the migration and before it is replaced keeps the legacy label for good. Readers are unaffected today: no query filters or groups on commitment_type (QueryByProvider and the monthly summary group by provider and service only). A later migration that drops the legacy literals from the CHECK should relabel those stragglers first. That's worth a line in the follow-up issue.
• Lock time on large deployments. The UPDATE rewrites every existing snapshot row, across all partitions, inside the same transaction as the ACCESS EXCLUSIVE constraint swap. On a big deployment, collector writes block for the length of the rewrite. That's fine at current sizes, but it's something to know when this deploys.
• API change, owner call. The commitment_type in snapshot JSON changes from RI/SavingsPlan to the pkg/common values (reserved-instance, savings-plan, committed-use). The frontend never reads it, and neither does the MCP or CLI main. Any external API consumer would see the new strings.

Attacked:

• Consistency. Compared the Go mapper with the SQL relabel: GCP becomes committed-use, and every common.IsSavingsPlan slug, plus savingsplans and the pre-Do not add the CodeRabbit status to required checks: it reports success while throttled #85 savings-plans, becomes savings-plan. The SQL list matches the five ServiceSavingsPlans* constants in pkg@541e05e5. allowedCommitmentTypes mirrors the new CHECK exactly.
• Keys. The PK is (id, timestamp), so relabelling can't collide. The migration number is 000145, as reserved for this PR, and main is at 000139.
• Commands. go build ./.... go vet, with and without -tags integration, on analytics and migrations. go test -race -short -count=1 on both (ok). go test -race -tags integration -count=1 ./internal/analytics/ (ok). The full ./internal/database/postgres/migrations/ integration package (ok, 218s, including the new up/down/up test across a monthly and the default partition). golangci-lint v2.10.1, with and without the integration tag (0 issues). gocyclo (silent).
• Fail-before, in a separate scratch tree. I restored the merge-base collector and validator, behind a shim with the new signature. TestCollectorCollect, TestCommitmentTypeLogic and TestValidateCommitmentType fail. TestCollector_GCPCommittedUse_DB fails with actual: {"aws/ec2":"RI","aws/savingsplans":"RI","gcp/compute":"RI"}. All pass on the head.

Body-stale:

• "Stacked on fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health #1269 … the diff also shows that commit. Review only the last commit": fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health #1269 has merged, so the three-dot diff now contains only this change.
• "000145 is after the open chain 000135–000143 (fix(auth): bind group account scope entries to account IDs #1116 → … → feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241)": main now has 000135 to 000139.
• Refs #339: fix(purchase,server): skip the $0 purchase confirmation and report a failed lazy DB init on /health #1269 (merged) covered the other two findings in chore(purchase/server/analytics): low correctness findings from the 2026-09-02 audit [platform part] #339, and this PR is the last one. Unless something else remains, this can say Closes #339.

_{Reviewer: rB23. 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.}