[IAmJSD]

Closes #1422

Item 1 (tie the alias tables to the library constants, plus a drift test) was done in #1425. This PR covers item 2.

Root cause

mapSavingsPlansSlug in internal/purchase/execution.go accepts both spellings of each per-type Savings Plans slug: savings-plans-compute (the common.ServiceSavingsPlans* constant that recs carry) and savingsplans-compute. validatePurchaseRecommendation stores the service a client sends as-is, so BuildSuppressions writes the suppression under whichever spelling the client used. config.CanonicalSuppressionService only knew the legacy AWS RI slugs and compared Savings Plans spellings literally. A purchase sent with the dash-free spelling therefore wrote a suppression that never matched the hyphenated rec. The rec it had just bought stayed on show and could be bought again.

Reachability, checked before changing anything: the frontend sends only the hyphenated per-type slugs, and nothing in the repo emits the dash-free ones. So only a direct API client (POST /api/purchases/execute with a hand-written body) can reach this. That matches the maintainer's finding on the issue.

Fix

• config.awsSavingsPlansSpellings maps the four dash-free per-type spellings to the library constants. CanonicalSuppressionService consults it after the legacy RI table, for AWS only.
• The savingsplans umbrella is deliberately not mapped, so one umbrella suppression can never hide every plan type, as the issue asks. Azure's savingsplans service and other non-AWS providers pass through unchanged.
• mapSavingsPlansSlug's map moves to a package var (savingsPlansSlugs) so a drift test can range over it, the same way chore(config): tie suppression slug aliases to the library service constants #1425's test covers legacyAWSServiceSlugs. Lookup behaviour is unchanged.

Verification

• New and extended tests:
  • TestCanonicalSuppressionService: 8 new rows. They cover the 4 dash-free spellings, idempotence on the hyphenated form, the umbrella passthrough, and Azure savingsplans / savingsplans-compute passthrough.
  • TestApplySuppressions_AWSLegacySlugSpellings (scheduler): 5 new rows. They cover a dash-free suppression hiding a hyphenated rec and the reverse, the database plan type, an umbrella suppression not hiding a compute rec, and a compute suppression not hiding a sagemaker rec.
  • TestSavingsPlansSpellingsMatchSuppressionAliases (purchase): every per-type key in savingsPlansSlugs must canonicalise to the slug it buys, and umbrella keys must pass through.
• Fail-before: with only the config/types.go change reverted, all three tests fail. After the change they pass.
• Local checks:
  • go build ./..., go vet ./..., go vet -tags integration ./... and the e2e vet pass.
  • golangci-lint v2.10.1 reports 0 issues on config, scheduler and purchase, with and without --build-tags integration.
  • gocyclo -over 10 is clean on the changed files.
  • go test -race -short passes on config, scheduler and purchase. go test -race -tags integration also passes on all three (testcontainers Postgres; the first attempts timed out starting containers on a loaded host, and the reruns passed).
  • The secrets scan is clean, and scripts/test-git-secrets-allowlist.sh passes.
• No real cloud APIs were called.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 5 billable files and costs up to $1.25.}

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: 2a11540
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: a5a3e964-47fe-464a-b74b-73012877fe5b

Commits

Reviewing files that changed from the base of the PR and between 2a11540 and 32881eb.

Files selected for processing: 5

• internal/config/suppressions_build_test.go
• internal/config/types.go
• internal/purchase/execution.go
• internal/purchase/slug_alias_drift_test.go
• internal/scheduler/scheduler_suppressions_test.go

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

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

[IAmJSD]

Pushed 32881eb (head 32881eb): a single commit that canonicalises the four dash-free per-type Savings Plans spellings in CanonicalSuppressionService, adds a drift test over savingsPlansSlugs, and adds suppression rows to the config and scheduler tables. Local build, vet, lint, gocyclo, race unit tests, integration tests and the secrets scan all pass.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• On current main the bug this PR fixes can't be reached through the route the body names. Since #2073 (internal/api/purchase_pricing.go:33-75), validateExecutePurchaseRequest matches every rec to a stored rec on config.RecIdentityKey, and that key uses the raw rec.Service. Stored recs only ever carry the hyphenated library constants, so a hand-written "service":"savingsplans-compute" gets a 409 ("not in the current recommendation set") before BuildSuppressions runs. A matched rec is also replaced by the stored row, so its service is hyphenated anyway. The plan attach paths (purchase/attach_recommendations.go:35, purchase/plan_step_attach.go:348) build suppressions from resolved stored recs too. The only remaining reach is legacy data: suppression rows, or failed executions retried at handler_purchases.go:2636, that were written before #2073 under a dash-free spelling. The change is still sound defence in depth, and it is small, symmetric (canonicalises both the suppression side and the rec side), AWS-only, per-type only, with the drift test as a guard. So this is not a blocker. Whether to merge it or close chore(config): tie suppression slug aliases to the library constants and cover Savings Plans spellings #1422 as won't-fix (cristim's comment on the issue allows either) is the acting owner's call.

Attacked:

• Over-suppression. I checked that the umbrella (savingsplans, savings-plans) is not mapped, that Azure savingsplans passes through because of the provider != "aws" early return, and that each per-type key maps only to its own constant (compute to compute, and so on). isAWSEC2's == "compute" check is unaffected, because the SP keys map to savings-plans-*, never to compute. Every caller of CanonicalSuppressionService canonicalises both sides: suppressions_build.go:34, scheduler.go:1420 and :1466. No SQL filters purchase_suppressions by service.
• Reachability. I added a throwaway handler test in the scratch tree, cloned from TestHandler_executePurchase_SavingsPlanCountMustMatchStored, that posts savingsplans-compute against a stored savings-plans-compute rec. It got a 409 "not in the current recommendation set". I deleted the test afterwards.
• Fail-before: I restored origin/main's internal/config/types.go. All 3 new tests fail on the 4 dash-free spellings (config: 4 subtests; scheduler: the 3 hide cases; purchase drift test: 4 subtests). All pass with the change.
• Ran go build ./..., go vet with and without -tags integration on config, scheduler and purchase, the e2e vet, and go test -race -short -count=1 on all three packages (ok). I also ran go test -race -tags integration -count=1 on all three (ok; config 468s, scheduler 126s, purchase 232s; testcontainers).

Body-stale:

• "only a direct API client (POST /api/purchases/execute with a hand-written body) can reach this" and "A purchase sent with the dash-free spelling therefore wrote a suppression that never matched": this is no longer true on main. That request is refused with a 409 before any suppression is written (see above). It only held before #2073.

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

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

Blocking:

• none

Non-blocking:

• The umbrella spellings savings-plans and savingsplans still don't canonicalise to each other. That's correct as far as chore(config): tie suppression slug aliases to the library constants and cover Savings Plans spellings #1422 goes (the issue only forbids folding the umbrella into the per-type slugs). If recs never carry an umbrella service, it's unreachable. If one ever does, the same literal mismatch would come back for umbrella buys.
• BuildSuppressions now stores the canonical hyphenated slug for a dash-free purchase. Revoke builds its permission constraint from the stored service (revokeConstraintSet), so an API-key permission scoped to Services: ["savingsplans-compute"] would no longer match its own suppression on revoke. fix(config): canonicalise every AWS service alias in purchase suppressions, not only ec2/compute #1420 already behaves this way for legacy RI slugs, and it's an obscure case, so I'm only noting it.

Attacked:

• Traced every CanonicalSuppressionService caller: the bucket key in BuildSuppressions (write side), both sides of the scheduler's applySuppressions match, and isAWSEC2 in recommendation_identity.go. The new spellings map to savings-plans-*, never to compute, so isAWSEC2 is unaffected. No SQL compares suppression service literally. Suppressions already stored under the dash-free spelling are canonicalised at match time, so they start working without a backfill. Non-AWS providers return before the lookup, so Azure savingsplans passes through as the test asserts.
• The savingsPlansSlugs hoist is a pure move (same 10 keys and values). It also stops rebuilding the map on every call.
• Fail-before: I reverted only internal/config/types.go to origin/main, and all three tests failed (TestCanonicalSuppressionService, TestSavingsPlansSpellingsMatchSuppressionAliases, TestApplySuppressions_AWSLegacySlugSpellings). Restoring the change makes them pass.
• Ran: go build ./...; go vet (with and without -tags integration) on config, purchase and scheduler; go test -race -short -count=1 and go test -race -tags integration -count=1 on all three packages. All passed.
• Item 1 of chore(config): tie suppression slug aliases to the library constants and cover Savings Plans spellings #1422 landed in chore(config): tie suppression slug aliases to the library service constants #1425 (merged), so Closes #1422 is right.

Body-stale:

• none

_{Reviewer: rE1. 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 32881eb6ac18f1cf9a34beefda4f88d97970b53b merged onto main (2baf23d5), together with #1240, #1302, #1309, #1454, #1456, #1463 in that order (combined tree d882e3bf, 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 32881eb6 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 32881eb6ac18f1cf9a34beefda4f88d97970b53b.