[cristim]

Summary

convertSavingsPlan (providers/azure/services/savingsplans/client.go) set StartDate only when effectiveDateTime was present. A live savings plan returned without it kept a zero StartDate, recfilter's recency check then treated it as old, and the duplicate-purchase guard let a repeat purchase through. Azure reservations already reject this case (internal/reservations/strict.go); savings plans did not.

• An owned plan that is not provably terminal and has no effectiveDateTime now fails GetExistingCommitments (through the existing "incomplete savings plan inventory ... row N" wrapping), naming the plan ID and its state.
• Terminal states that may lack a start: Failed, Cancelled and Expired (the armbillingbenefits values azureSavingsPlanState maps to failed, canceled and expired). recfilter rejects those before reading StartDate; its own reject set is a superset (it adds retired and pending-return, which Azure never emits), so no state slips between the two. Unknown states are treated as live.
• A plan with nil properties or nil provisioning state still defaults to Active (fix(azure): commitments fabricate State "active" and Region from fields the SDK omits #74), so a missing start fails closed.
• No fallback to purchaseDateTime or benefitStartTime (both exist on the SDK model): the repo does not fabricate a start date.

Closes #481

Tests

TestGetExistingCommitments_EffectiveDateTimeCompleteness, SDK-decoded fixtures:

• live (Succeeded, PendingBilling) and no-state rows without a start error, with the plan ID in the message;
• Cancelled, Expired and Failed rows without a start pass;
• a live plan with a start passes;
• the same rows through the real recfilter.DuplicateChecker: AdjustRecommendationsForExisting returns the error instead of passing the recommendation through.

Four existing fixtures (client_test.go, scope_test.go, client_pager_cap_test.go) gained an effectiveDateTime: they had no state or start and passed only through the old fail-open path.

• Fail-before: with origin/main's client.go, the live, pending and no-state cases fail in both variants.
• Mutation probes (implementer and reviewer, scratch copies): treating Expired as live, returning the commitment for a live plan without a start, and changing the default state all fail the test.

Verification

make build, make vet, (cd providers/azure && GOWORK=off go test -race -short ./services/savingsplans/...), the pinned golangci-lint v2.10.1 on the final code (0 issues), gofmt, and the commit/pre-push hooks pass. Fixture-based evidence only; no live Azure call.

Local review: one adversarial pass by a non-author reviewer (the one that reviewed #494), no actionable findings.

Overlap

#491 also calls convertSavingsPlan (appendCoveringForeignPlan): after a rebase its foreign-plan fixtures need an effectiveDateTime, and a live foreign plan without one will fail the covering list closed, consistent with this fix. #473 only touches a comment in this file.

🤖 Generated with Claude Code

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

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

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 13 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 72 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 23c61b14-5353-4c2e-b40a-88e4f5f8e093

📥 Commits

Reviewing files that changed from the base of the PR and between b5e755f and 7d1bd94.

📒 Files selected for processing: 5

• providers/azure/services/savingsplans/client.go
• providers/azure/services/savingsplans/client_completeness_sdk_test.go
• providers/azure/services/savingsplans/client_pager_cap_test.go
• providers/azure/services/savingsplans/client_test.go
• providers/azure/services/savingsplans/scope_test.go

• 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 f01414261e8e29c4a2b78aca29c6df2b6a93d8a1

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.

State by state. States come from armbillingbenefits@v1.0.0 constants.go:156-163 and are mapped by azureSavingsPlanState (client.go:217-233). Behaviour when effectiveDateTime is missing:

Azure state	Maps to	Missing start
Succeeded	Active	error
Creating, Created, PendingBilling, ConfirmedBilling	PaymentPending	error
Failed, Cancelled, Expired	terminal	allowed
nil properties or nil state	Active (#74)	error
unknown value	passed verbatim, live	error

The terminal set matches recfilter's reject set (pkg/recfilter/dedupe.go:107-114). That set is a superset with Retired and PendingReturn, which Azure never emits, so no state slips between the two checks.

Trade-off for the owner to note (non-blocking). The SDK documents effectiveDateTime as "DateTime of the savings plan starts providing benefit from" (models.go:503-504). It is therefore plausibly absent for the pending states (Creating, Created, PendingBilling, ConfirmedBilling). With this PR, a subscription holding a just-ordered, not-yet-effective plan gets an error from the whole GetExistingCommitments call until the plan activates, which blocks savings plan purchases for it in the meantime.

That is deliberate fail-closed behaviour: it prevents the duplicate purchase this issue is about, and the error names the plan ID and state. Narrowing it to Succeeded only would reopen the fail-open path for pending plans. A one-line code comment saying pending states are included on purpose would help future readers. Not falling back to benefitStartTime or purchaseDateTime is correct (models.go:461-462), because guessing a start date is the fail-open risk.

Other attacks:

• Owned paths. convertSavingsPlan has one caller, appendOwnedPlans (client.go:193). That caller covers subscription-scoped plans and billing-account Single-scope plans (fix(azure): attribute single-scope savings plans billed to a billing account #468), so nothing bypasses the check.
• fix(azure/savingsplans): take the inventory resource type from the SKU, not the ARM name #494. savingsPlanResourceType still runs first (client.go:257), so SKU errors take precedence and are not hidden.
• Fixtures. The four edited fixtures only gained EffectiveDateTime on owned live plans. No assertion was loosened.
• Mutation probes (scratch copy, reverted):
  • Disabling the check fails the live and pending sub-cases.
  • Adding Active to the terminal set fails the live and no-state sub-cases.

Local verification (macOS, exact SHA f014142)

SDK-decoded fixtures; no Azure call.

• Fail-before. With origin/main's client.go and the PR's tests, TestGetExistingCommitments_EffectiveDateTimeCompleteness FAILS. I ran this locally.
• Pass-after.
  • (cd providers/azure && GOWORK=off go test -race -short -count=1 ./...) exits 0 with 13 packages ok.
  • make build and make vet pass.

Labels mirror #481. CodeRabbit hit "Review limit reached". If that holds at merge: CR waived: quota, adversarial review + local verification + green CI.

[cristim]

Updated onto main b5e755f (after #496 and #497) using server-side update-branch. New head 7d1bd949a99eaa78f308b9ee69ed96128c2a5190. git range-diff shows f014142 = 7d1bd94, identical, so the CLEAN verdict carries over. CI had passed on f014142 and is re-running. CR waived: quota, adversarial review + local verification + green CI.

[cristim]

Merged by squash at reviewed head 7d1bd949a99eaa78f308b9ee69ed96128c2a5190 as 36d8b110. The gates were met on that SHA:

• independent review CLEAN, posted above (carried across rebases by identical git range-diff)
• local verification posted above
• CI green on 7d1bd949
• labels mirrored; no unresolved threads

CR waived: quota, adversarial review + local verification + green CI. A retrospective CodeRabbit review will run when quota returns. Closes #481. Note for the owner: plans in pending states (Creating/Created/PendingBilling/ConfirmedBilling) without effectiveDateTime now fail the subscription's savings plan inventory until they activate (fail-closed by design; see the verdict).