fix(aws/savingsplans): validate plan type, term and payment before trusting a Cost Explorer offering ID

[cristim]

Summary

One slice of the #53 findings tracker. In providers/aws/services/savingsplans/client.go, findOfferingID returned a Cost Explorer offering ID before running resolveSPPlanType (including the client-scope check), convertTermToSeconds and convertPaymentOption. A purchase carrying a CE offering ID therefore skipped plan-type, scope, term and payment validation. The three validations now run first; valid inputs keep the fast path, which still skips DescribeSavingsPlansOfferings.

No valid CE-ID purchase is newly rejected: CE offering IDs exist only for EC2 Instance SPs, whose plan type, term and payment values (from the CE recommendation converters) are all accepted by the validators. The only new rejections are hand-built or persisted recs with non-canonical spellings, or a rec routed to a client of the wrong scope (an EC2 ID bought through a Compute client).

Not done: cross-checking the validated values against the offering the ID refers to, which needs an extra DescribeSavingsPlansOfferings call.

Refs #53

Tests

• TestFindOfferingID_CEOfferingIDStillValidatesInputs: invalid term, invalid payment, unsupported plan type, plan type outside the client scope. Each asserts that both findOfferingID and PurchaseCommitment error and that CreateSavingsPlan and DescribeSavingsPlansOfferings are never called. All four fail with origin/main's client.go.
• TestFindOfferingID_CEOfferingIDFastPath: valid input with an offering ID still skips the offerings lookup.
• Mutation probes (scratch copy): moving the early return back above each validator in turn makes subtests fail.

Verification

make build, make vet, (cd providers/aws && GOWORK=off go test -race -short ./services/savingsplans/...) (140 tests), pinned golangci-lint v2.10.1 (0 issues), gofmt, and the commit/pre-push hooks pass. Unit-level evidence only.

Local review: an adversarial pass by a non-author reviewer, no actionable findings.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Savings plan recommendations are now validated for supported plan types, terms, and payment options even when they include an offering ID. Invalid values are rejected instead of proceeding with the supplied ID, while valid IDs continue to be accepted without an additional offering lookup.

[cristim]

@coderabbitai review

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 81fa5fda-4c4f-41b9-9aa7-03ff20289e8a

📥 Commits

Reviewing files that changed from the base of the PR and between 2c1111d and a050628.

📒 Files selected for processing: 2

• providers/aws/services/savingsplans/client.go
• providers/aws/services/savingsplans/client_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

findOfferingID now validates plan type, term, and payment option before using a Cost Explorer-provided offering ID. Tests cover invalid inputs and confirm that valid inputs still return the supplied ID without an offering lookup.

Changes: Offering ID validation

Layer / File(s)	Summary
Validate inputs before using the offering ID providers/aws/services/savingsplans/client.go, providers/aws/services/savingsplans/client_test.go	findOfferingID resolves and validates plan type, term, and payment option before using a supplied offering ID. Tests verify errors for invalid inputs, no offering lookup or plan creation in those cases, and the valid-ID fast path.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to a0506

The EC2Instance purchase path already has regression coverage; no actionable merge risk remains after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: validating plan type, term, and payment before trusting a Cost Explorer offering ID.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

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

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

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Independent adversarial review: CLEAN at 6d2dd22c934b523d4ae367e46602213a27843c78

Reviewer: an independent agent that did not write this change (Sonnet, high effort), cold brief, full diff. The PR body was treated as a claim.

Blocking findings: none.

Regression check: no valid purchase is newly rejected. There is one producer of SavingsPlanDetails.OfferingID. pkg/common/types.go:723 is only a JSON pass-through.

providers/aws/recommendations/parser_sp.go:274-280 sets it via extractEC2SPFields (:122-131). The values it can produce:

Field	Values	Where the values come from	Accepted by
PlanType	EC2Instance	:136-148	convertPlanType (client.go:564-577) and the EC2Instance-scoped resolveSPPlanType (:313-330)
Term	1yr, 1, 3yr, 3	rec.Term, already validated by convertSavingsPlansTermInYears (converters.go:63-72) before CE is queried	convertTermToSeconds (client.go:582-591), same set
PaymentOption	all-upfront, partial-upfront, no-upfront	rec.PaymentOption, validated by convertPaymentOptionE (converters.go:47-58)	convertPaymentOption (client.go:597-608), which also accepts the title-case spellings

The PR's claim that offering IDs exist only for EC2 Instance plans holds in the code. extractEC2SPFields returns an empty struct for every other plan type (parser_sp.go:123). The SDK's SavingsPlansDetails.OfferingId (costexplorer types.go:2295) is generic, but this repo gates it.

Routing. The ec2instance slug goes to an EC2Instance-scoped client (provider.go:480-485). The legacy "" service goes to the unscoped client (:496). A valid EC2Instance rec has no path to a Compute-scoped client.

Ordering. findOfferingID (client.go:466-480) runs resolveSPPlanType, then the term check, then the payment check, then the OfferingID fast path. PurchaseCommitment calls it before CreateSavingsPlan (:229), and ValidateOffering and GetOfferingDetails do the same. No AWS call happens before validation.

Mutation probes (scratch copy, reverted):

• Moving the fast path above the term check fails the invalid-term and invalid-payment subtests.
• Moving it back to the top (main's behaviour) fails all 4 validation subtests.

Non-blocking:

1. The new tests use a Compute-scoped client and a Compute rec, while the real producer is EC2Instance. The validators are shared, so this is a coverage gap only.
2. Recs persisted outside this repo could carry other spellings. Such recs would already fail on the non-CE path.

Local verification (macOS, exact SHA 6d2dd22)

Mock evidence; no AWS call, no purchase.

• Fail-before: with origin/main's client.go and the PR's tests, all 4 subtests of TestFindOfferingID_CEOfferingIDStillValidatesInputs FAIL. The fast-path test passes on both versions.
• Pass-after: (cd providers/aws && GOWORK=off go test -race -short ./...) passes, and make build and make vet exit 0.

Labels mirror #53. CodeRabbit is quota-limited. If that still holds at merge: CR waived: quota, adversarial review + local verification + green CI.