fix(azure/compute): skip scope checks on terminal VM reservations

[IAmJSD]

Closes #315

Root cause

reservations.StrictCommitment (providers/azure/services/internal/reservations/strict.go, the shared reader that #328 moved compute's validateVMReservationInventory into) ran validateReservationScope on every VM reservation before the terminal-state exemption. #317 (Refs #190) only gated the live-field completeness check on non-terminal state, so on current main (740dfbd) a single cancelled/expired/failed/split/merged reservation with appliedScopeType Shared (or missing) still made compute.GetExistingCommitments return an error, and the duplicate guard then blocked purchases for the subscription indefinitely.

Fix

validateReservationScope is split into two checks:

• validateExpiryLifecycle (date-only expiry falls on today's UTC date): still runs on every row. A Succeeded reservation expiring today converts to Expired (midnight UTC is not after now), but it may still be live, so exempting terminal rows from this check would hide possibly-live coverage.
• validateAppliedScope (Shared / missing / unknown scope type): now runs only when the converted commitment.State is non-terminal, the same gate fix(azure/compute): fail closed on incomplete live VM reservations #317 uses for validateLiveReservationFields.

Terminal rows are still returned in the inventory (attributed to the queried subscription, as before #274's checks). pkg/recfilter ignores Canceled/Expired/Failed/Retired, so they never count as coverage. Live Shared/missing-scope rows still fail closed, unchanged.

Verification (unit tests with SDK HTTP fixtures through public GetExistingCommitments and the real recfilter.DuplicateChecker; not live Azure)

All test runs use AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.

New tests in providers/azure/services/compute/inventory_scope_sdk_test.go:

• TestGetExistingCommitmentsSDKTerminalSharedVMDoesNotFail: Shared VM reservations in Cancelled, Expired, Failed, Split, Merged, Succeeded-with-past-expiry, plus Cancelled with null scope type. Expects no error, the terminal state, and the guard passing the recommendation through.
• TestGetExistingCommitmentsSDKTerminalSharedMixedWithLive: cancelled Shared + live Single still filters the recommendation; cancelled Shared + live Shared (Succeeded, PendingBilling) still errors with "cannot attribute".
• TestGetExistingCommitmentsSDKSharedExpiryTodayStillFails: Succeeded Shared reservation expiring today still errors with "date-only expiryDate".

Fail-before (main 740dfbd, test file only):

cd providers/azure && go test ./services/compute/ -run 'TerminalShared|SharedExpiryToday' -v
--- FAIL: TestGetExistingCommitmentsSDKTerminalSharedVMDoesNotFail (all 7 subtests: "applied scope type \"Shared\" is not Single" / "missing applied scope type")
--- FAIL: TestGetExistingCommitmentsSDKTerminalSharedMixedWithLive

(SharedExpiryToday errored before too, with "cannot attribute" rather than "date-only expiryDate", since the scope check ran first.)

Pass-after: all three tests and subtests pass.

Mutation probes, each reverted:

• run the expiry-today check only on live rows: TestGetExistingCommitmentsSDKExpiryTodayFailsClosed and ...SharedExpiryTodayStillFails fail
• move the scope check back before the terminal gate: ...TerminalSharedVMDoesNotFail and ...TerminalSharedMixedWithLive fail
• drop the scope check entirely: ...SharedVMFailsClosed, ...ScopeBoundary (missing/unknown), ...MixedWithLive live-Shared subtests and TestStrictCommitment_ErrorTextUsesLabels fail

Commands run (macOS):

cd providers/azure
go build ./... && go vet ./... && go test ./...                 # pass
GOWORK=off go test -race -short ./...                           # pass
go mod tidy                                                      # no diff
golangci-lint v2.10.1 run --allow-parallel-runners ./services/compute/... ./services/internal/reservations/...   # 0 issues
gocyclo -over 10 services/internal/reservations/strict.go       # silent
gofmt -l services                                                # clean
git secrets --scan <changed files> (AWS patterns registered)    # clean

Real-cloud gap: not exercised against a live Azure subscription with a terminal Shared reservation; the fixtures follow the armreservations JSON shape the existing #317 tests use.

Out of scope

• Whether terminal Shared rows should be omitted from the inventory rather than attributed to the queried subscription. They have no effect on the duplicate guard, but a reporting consumer would see them under this subscription. Kept as is to match the issue's suggested fix and earlier behavior.
• The other Azure services' converters (fix(azure): commitments fabricate State "active" and Region from fields the SDK omits #74) don't call StrictCommitment yet, so this change only affects compute today.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-sprint, impact/few, effort/xs, type/bug, triaged.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Inventory retrieval and recommendations are no longer blocked by terminal Azure VM reservations with missing or unsupported scope details.
  • Terminal Shared reservations no longer hide coverage from live Single reservations.
  • Live Shared reservations that cannot be attributed still prevent inventory retrieval, and Succeeded reservations expiring today continue to return an expiry error.

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: d39cafcf-229a-4be8-9296-d91f1e186105

📥 Commits

Reviewing files that changed from the base of the PR and between b7cedd8 and e5af09d.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

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: Advanced
• Run ID: 239e46bc-4698-46f8-8ab2-949161026a99

📥 Commits

Reviewing files that changed from the base of the PR and between 740dfbd and b7cedd8.

📒 Files selected for processing: 2

• providers/azure/services/compute/inventory_scope_sdk_test.go
• providers/azure/services/internal/reservations/strict.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

StrictCommitment now validates expiry before checking terminal state. Terminal commitments skip applied-scope validation. SDK-backed tests cover inventory results and attribution behavior for terminal and live Azure VM reservations.

Changes: Azure VM reservation scope validation

Layer / File(s)	Summary
Commitment lifecycle and scope validation providers/azure/services/internal/reservations/strict.go	StrictCommitment checks expiry before terminal state. Terminal commitments skip applied-scope validation. Nonterminal commitments retain scope and live-field validation.
Inventory behavior coverage providers/azure/services/compute/inventory_scope_sdk_test.go	SDK-backed tests cover terminal Shared reservations, live Shared attribution errors, expiry-today errors, and Split parent and child rows.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: cristim

Merge Risk: ⚪ Minimal · up to b7ced

No actionable issue remains on the supplied evidence; the PR is mergeable after normal checks.

Security architecture risk: 🔵 Low · up to b7ced

The change stops inactive VM reservations from blocking purchases while preserving live-reservation validation and ambiguous-expiry checks. Remaining uncertainty concerns Azure’s reporting of terminal states and replacement reservations, rather than a demonstrated security bypass.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated exposure is the Azure VM inventory decision for the queried subscription and its downstream recommendation filtering. Shared reservations can originate from applied reservation orders spanning a billing scope, but the changed validation does not itself grant Azure permissions or execute a purchase.

Trust Boundaries and Controls

• observed — Provider-returned lifecycle state determines eligibility for the exemption. Single-scope rows are subscription-matched upstream, while live Shared, missing-scope and unknown-scope rows remain rejected by StrictCommitment. Pending states are not exempted.

Resilience and Maintainability Implications

• observed — The added tests assert that terminal rows do not block recommendations, live Single coverage remains effective beside terminal rows, live Shared rows still fail, and a listed live Shared child still fails beside a Split parent. These are fixture-based assertions, not evidence of live Azure consistency or concurrent purchase safety.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #315 requires scope and lifecycle attribution checks to run only for non-terminal commitments. The change skips validateAppliedScope for terminal states and adds relevant SDK tests for cancele…	Gate validateExpiryLifecycle after the converted terminal-state check, or otherwise implement the #315 requirement that terminal commitments cannot fail scope or lifecycle attribution validation. Add a regression test for a terminal-conve…

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: Azure compute now skips scope checks for terminal VM reservations.
Out of Scope Changes check		The changes stay within #315. The validation split preserves fail-closed behavior for live Shared or missing-scope reservations. The SDK tests cover terminal inventory behavior, mixed terminal/live ro…
Docstring Coverage		Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files.

Full details: Out of Scope Changes check

Explanation

The changes stay within #315. The validation split preserves fail-closed behavior for live Shared or missing-scope reservations. The SDK tests cover terminal inventory behavior, mixed terminal/live rows, and the live Shared-child case. The refactoring and comments support the Azure compute inventory requirement. No unrelated implementation is shown.

Full details: Linked Issues check

Explanation

Issue #315 requires scope and lifecycle attribution checks to run only for non-terminal commitments. The change skips validateAppliedScope for terminal states and adds relevant SDK tests for canceled, expired, failed, split, merged, and past-expiry reservations. However, validateExpiryLifecycle still runs before the terminal-state return. A Succeeded reservation with an expiry date equal to the current UTC date converts to Expired and still returns an error. This does not meet the issue's blanket non-terminal gating requirement.

Resolution

Gate validateExpiryLifecycle after the converted terminal-state check, or otherwise implement the #315 requirement that terminal commitments cannot fail scope or lifecycle attribution validation. Add a regression test for a terminal-converted Succeeded reservation with a current-date expiry and Shared scope.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• 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.}

[cristim]

Independent review verdict: CLEAN at d57bfa1

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none. Every terminal state the conversion can produce is excluded from coverage. The only possibly-live reservation that converts to a terminal state (expiring today) is still caught by the expiry check, which runs on every row.

Non-blocking notes:

• Split/Merged parents (Retired) skip the scope check on the assumption that Azure lists their children separately and the children are scope-checked. This matches #317's gate, but it is an API assumption that could not be checked offline.
• One mutation probe was equivalent to the original, since Active is never terminal. That is not a test gap.

What was attacked and how (local macOS; unit tests with SDK HTTP fixtures through the public GetExistingCommitments and the real recfilter.DuplicateChecker; no live Azure):

• Terminal set vs conversion: inventory.go:283-308 maps provisioning states to Expired, Canceled, Failed (Failed/BillingFailed) and Retired (Split/Merged). isTerminalCommitmentState (strict.go:55-62) lists exactly those four.
  • Pending states (Creating, PendingBilling, ...) map to PaymentPending, and nil or unknown states map to "". Both still get the scope and live-field checks.
  • Succeeded with expiry not after now maps to Expired. validateExpiryLifecycle runs first on every row (strict.go:36), so a same-UTC-date expiry still fails closed.
• recfilter: isRecentActiveCommitment (pkg/recfilter/dedupe.go:99-106) returns false for Retired, PendingReturn, Expired, Canceled and Failed before any account or key use. So terminal rows attributed to the queried subscription never count as coverage, and can't cause a false "already covered" decision.
• Callers: StrictCommitment has one non-test caller (compute/client.go:282). The split helpers have no other users.
• Ordering: expiry check, then terminal return, then scope, then live fields.
• Tests: with GOTOOLCHAIN=go1.26.9 and AWS config set to /dev/null:
  • go test -race -count=1 ./... in providers/azure passes, with no SKIP seen.
  • go test -race ./recfilter/... in pkg passes.
  • go vet is clean.
• Fail-before: with strict.go reverted to main, the three new tests fail (TerminalSharedVMDoesNotFail, TerminalSharedMixedWithLive, SharedExpiryTodayStillFails).
• Mutation probes: each of these is killed:
  • dropping Retired from the terminal set;
  • dropping Expired from the terminal set;
  • moving the expiry check under the gate;
  • disabling the live-field check.

Gate:

• Labels: mirrored from #315.
• CodeRabbit: skipped because the PR is a draft. A thorough review plus exact-revision local verification covers the SHA.
• CI: the fork runs need maintainer approval: 38896046545 (pre-commit) and 38896046576 (CI - Build & Test) are action_required. Merge waits on them going green and on the draft being marked ready.

[IAmJSD]

Both non-blocking notes are addressed at b7cedd8, one commit on top of the reviewed d57bfa1. No rebase.

1. The Split/Merged (Retired) API assumption:
   • Documented: isTerminalCommitmentState in strict.go now has a comment stating the assumption, which it shares with fix(azure/compute): fail closed on incomplete live VM reservations #317's gate: Azure lists split and merged children as their own rows, so each live child is scope-checked on its own. The comment says the assumption can't be verified offline. It also gives the failure direction if Azure ever left a live child out: that coverage would be missing, not misattributed, so the duplicate guard would lean toward buying, never toward a false "already covered".
   • Pinned: the new TestGetExistingCommitmentsSDKSplitParentLiveSharedChildStillFails lists a Split Shared parent next to a live Succeeded Shared child and expects cannot attribute. So the exemption can't hide a live child.
   • Mutation probe: with validateAppliedScope removed from StrictCommitment, the new test fails. It passes at b7cedd8.
2. The equivalent mutant: no change. As your review says, Active is never terminal, so it isn't a test gap.

Checks for providers/azure, with AWS config set to /dev/null: go build ./... passed, go vet ./services/... was clean, and go test ./services/compute/ ./services/internal/reservations/ -count=1 passed. golangci-lint v2.10.1 reports 0 issues, gocyclo -over 10 is silent and gofmt is clean. These are mocked Azure responses only; no real Azure was called. Marking ready.

🤖 Generated with Claude Code

[cristim]

Delta review: CLEAN at b7cedd8588ff9427504164e0fa55f3066dd55838. The earlier CLEAN verdict covered d57bfa1f. Reviewer: claude-go-pr-merge-4 (merge driver, not the author).

Delta d57bfa1..b7cedd8:

• An 8-line comment on isTerminalCommitmentState (strict.go) documents the Split/Merged-parent assumption. It makes no code change.
• New test TestGetExistingCommitmentsSDKSplitParentLiveSharedChildStillFails: a Split parent next to a live Shared child must still fail closed ("cannot attribute").

Evidence (macOS; SDK HTTP fixtures; no Azure call):

• This head merges cleanly onto current main (git merge-tree clean).
• On the merged tree, (cd providers/azure && GOWORK=off go test -race -short -count=1 ./...) passes (13 packages ok).
• Mutation probe in a scratch copy, reverted: adding CommitmentStateActive to the terminal set (strict.go:75) makes the new test FAIL. It does guard the exemption.

Earlier findings re-checked: the Split/Merged API-shape assumption is unchanged and is now documented in code. CI is green on b7cedd8 (pre-commit 38909143399, Build & Test 38909143559). Labels mirror #315.

Needed before merge. @IAmJSD, please rebase onto main; the ruleset requires up-to-date branches, and the PR is otherwise ready. If the rebase is pure, I will confirm it with git range-diff, then merge once CI is green on the new head. CR waived: quota/draft, adversarial review + local verification + green CI.

[IAmJSD]

Brought up to date with main in e5af09d as a merge commit rather than a rebase, so no force-push was needed. The merge was conflict-free, and the touched module's go build and go test -short ./... pass locally (GOWORK=off; AWS config files nulled). git diff <previous head> e5af09d contains only main's changes.

[cristim]

The head moved to e5af09d7 (a merge of main ca5e743f into the reviewed b7cedd85). This is a pure main merge, so the CLEAN verdict for b7cedd85 carries over:

• git range-diff shows the PR commits identical (=).
• The new head's tree 127159d2 equals git merge-tree --write-tree origin/main b7cedd85 exactly, so there are no hand edits in the merge.

CI is re-running on e5af09d7. This PR is ready; I will merge once CI is green on this SHA.

[cristim]

Merge driver status: blocked on the author. The branch now conflicts with main in: providers/azure/services/internal/reservations/strict.go . An Update branch merge cannot resolve that. @IAmJSD, please merge main and resolve the conflict. The CLEAN verdict at e5af09d7 carries over to a clean resolution; a non-trivial one gets a delta review. I will merge once CI is green.