[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 reservations no longer suppress recommendations for live Single reservations.
  • Live Shared reservations that cannot be attributed still prevent inventory retrieval, including when mixed with terminal or split-parent reservations.
  • Succeeded Shared reservations expiring today continue to return an expiry error when the expiry date cannot be determined unambiguously.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-go
• Branch: main
• File: .coderabbit.yaml
• Commit: 62f6f2a
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: f1b4ddf2-c323-4a98-8505-13ee64a856f7

Commits

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

Files selected for processing: 3

• providers/azure/services/compute/inventory_scope_sdk_test.go
• providers/azure/services/internal/reservations/strict.go
• providers/azure/services/internal/reservations/strict_test.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 delegates scope attribution and expiry handling to attributeReservation. Added tests cover inventory and recommendation 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, providers/azure/services/internal/reservations/strict_test.go	StrictCommitment delegates scope attribution and expiry checks to attributeReservation. Tests cover Shared reservations across billing scopes and lifecycle states.
Inventory behavior coverage providers/azure/services/compute/inventory_scope_sdk_test.go	SDK-backed tests cover terminal reservations, live Shared attribution errors, expiry-today errors, split parent and child rows, and recommendation filtering.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: cristim

Merge Risk: Minimal · up to 7604d

The change is mergeable with normal checks. An expiry-today test has a rare UTC-midnight timing flake, but no material behavior issue is established.

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 correctly defers unknown, missing, and Shared scope errors for terminal states, and the …	Gate validateReservationExpiry on !isTerminalCommitmentState(commitment.State), or otherwise apply the #315 requirement that terminal commitments cannot fail scope or lifecycle attribution checks. Update the current-date expiry test to …

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: skipping scope checks for terminal Azure compute reservations.
Out of Scope Changes check		The changes stay within issue #315. The shared attribution helper supports the Azure compute inventory path, preserves fail-closed behavior for live unattributable reservations, and skips terminal sco…
Docstring Coverage		Docstring coverage is 84.62% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 3 files.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #315. The shared attribution helper supports the Azure compute inventory path, preserves fail-closed behavior for live unattributable reservations, and skips terminal scope failures. The added internal and SDK-backed tests verify the issue behavior. 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 correctly defers unknown, missing, and Shared scope errors for terminal states, and the tests cover those cases. However, attributeReservation calls validateReservationExpiry before the terminal check. A Succeeded reservation with an expiry date equal to the current UTC date converts to Expired but still returns an error. The new tests explicitly require this behavior, so the lifecycle requirement in #315 remains unmet.

Resolution

Gate validateReservationExpiry on !isTerminalCommitmentState(commitment.State), or otherwise apply the #315 requirement that terminal commitments cannot fail scope or lifecycle attribution checks. Update the current-date expiry test to cover the required terminal outcome.

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.

[IAmJSD]

Merged main to resolve the conflict in providers/azure/services/internal/reservations/strict.go: kept main's reservationOwnedBy billingScopeId attribution (#423) in a new attributeReservation helper, so a Shared reservation billed to another subscription is skipped in any state, the expiry-today check still runs on every other row, and an unattributable scope fails closed only on live rows (#315). This replaces the PR's validateAppliedScope. The composition is pinned by a separate test commit, 882a937, with TestStrictCommitment_SharedAttributionAcrossLifecycle, so this is a non-trivial resolution for delta review. Head is now 882a937.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• providers/azure/services/internal/reservations/strict.go:60-74: a terminal Shared row whose purchaser can't be identified (no subscription billingScopeId) is kept and attributed to every subscription whose applied list contains it, so it can show up once per subscription in any report that sums inventory across subscriptions. It has no effect on the duplicate guard (recfilter ignores Canceled/Expired/Failed/Retired), and the PR body lists it as out of scope. A follow-up issue would be enough.
• inventory_scope_sdk_test.go TestGetExistingCommitmentsSDKSharedExpiryTodayStillFails builds expiry = time.Now() in the test, and GetExistingCommitments takes its own time.Now(). If a run crosses midnight UTC between the two, the test flakes. The existing ...ExpiryTodayFailsClosed test already does the same, so this isn't new.
• BillingFailed maps to Failed, which is terminal, so an unattributable BillingFailed Shared row no longer blocks inventory. That matches fix(azure/compute): fail closed on incomplete live VM reservations #317's gate and the recfilter semantics. Noting it only because the purchase path treats BillingFailed as orderBlocked rather than dead.

Attacked:

• Traced every ProvisioningState through commitmentStateFromProvisioning. A nil or unknown state maps to "", which is non-terminal, so it still fails closed. Succeeded with a past expiry maps to Expired and is exempt. Expiry-today still errors on Succeeded rows, whether owned or unattributable. A Shared row billed to another subscription is still skipped before any lifecycle check (fix(azure): shared-scope vm ri purchases make existing-commitment listing fail for the term #293 ordering kept). StrictCommitment has one caller, compute/client.go:299.
• Fail-before: I reverted only strict.go to origin/main. That made TerminalSharedVMDoesNotFail, TerminalSharedMixedWithLive, SharedExpiryTodayStillFails (wrong error text) and TestStrictCommitment_SharedAttributionAcrossLifecycle fail. They pass again with the fix restored.
• Ran in providers/azure: go build ./..., go vet ./... and go vet -tags integration ./... are clean. go test -race -short -count=1 ./services/compute/... ./services/internal/reservations/... passes, with and without GOWORK=off. golangci-lint v2.10.1 on both packages reports 0 issues, and gocyclo -over 10 strict.go is silent.

Body-stale:

• The "Fix" section describes a split into validateExpiryLifecycle and validateAppliedScope. Neither function exists at this head. After the merge with fix(azure): shared-scope vm ri purchases make existing-commitment listing fail for the term #293, the logic is attributeReservation plus reservationOwnedBy.
• "Live Shared/missing-scope rows still fail closed" is now only true for Shared rows without a /subscriptions/{sub} billingScopeId. A live Shared row billed to this subscription is counted, and one billed to another subscription is skipped (fix(azure): shared-scope vm ri purchases make existing-commitment listing fail for the term #293).
• The fail-before baseline "main 740dfbd" is out of date. Main has moved since.

_{Reviewer: rB1. 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 882a9377d227701a018bd96238fa532a11deb206

Blocking:

• none

Non-blocking:

• providers/azure/services/internal/reservations/strict.go:59-73 (attributeReservation): a terminal row whose purchaser can't be identified (missing or unknown appliedScopeType, or Shared without a /subscriptions/{sub} billingScopeId) is now kept and attributed to whichever subscription is queried. In a multi-subscription run, the same cancelled reservation therefore shows up in every subscription's inventory. recfilter.isRecentActiveCommitment drops Canceled/Expired/Failed/Retired, so the duplicate guard isn't affected. A reporting consumer that lists existing commitments per subscription would see duplicates, though. The PR body's "Out of scope" acknowledges this. Dropping these rows (return false, nil) would be the tidier follow-up.
• The Retired (Split/Merged) exemption depends on Azure listing the live children as their own rows. The new comment documents that, and TestGetExistingCommitmentsSDKSplitParentLiveSharedChildStillFails pins that a live Shared child still fails closed. Good.

Attacked:

• Walked through every ordering in attributeReservation against main:
  • A Shared row billed to another subscription is still skipped in every state, including expiry-today, the same as main.
  • The expiry-today check still runs on unattributable rows, so a possibly-live Succeeded row expiring today still errors.
  • A nil or unrecognized provisioning state maps to "", which isn't terminal, so it still fails closed.
  • BillingFailed maps to Failed, which is terminal.
• Checked that pkg/recfilter/dedupe.go:108-110 ignores every state that isTerminalCommitmentState exempts. StrictCommitment has a single caller, compute.
• Fail-before: restored origin/main strict.go and kept the new tests. TestGetExistingCommitmentsSDKTerminalSharedVMDoesNotFail, ...TerminalSharedMixedWithLive, ...SharedExpiryTodayStillFails and TestStrictCommitment_SharedAttributionAcrossLifecycle all FAIL. They pass at the head.
• Ran in providers/azure: go build ./..., go vet ./..., go vet -tags integration ./..., and go test -race -short -count=1 on services/compute and services/internal/reservations, both with go.work and with GOWORK=off. Also ran golangci-lint v2.10.1 on both packages (0 issues), gocyclo -over 10 strict.go (silent) and gofmt (clean). Neither package has integration-tagged tests.

Body-stale:

• "Fix: validateReservationScope is split into validateExpiryLifecycle ... and validateAppliedScope": none of these functions exist at this head. After the main merge (fix(azure): shared-scope vm ri purchases make existing-commitment listing fail for the term #293's reservationOwnedBy), the fix is the new attributeReservation, which defers the reservationOwnedBy error to live rows and runs validateReservationExpiry on every attributed or unattributable row.
• "on current main (740dfbd)" is stale. The body also omits the later tests TestGetExistingCommitmentsSDKSplitParentLiveSharedChildStillFails and TestStrictCommitment_SharedAttributionAcrossLifecycle.

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

Merged origin/main (now 7604d329) so the branch is up to date, as the ruleset requires. It was a clean merge, and the PR's diff against main is byte-identical (same patch-id), so the rB1 and rB30 reviews of 882a9377 carry over. Locally on the merged tree, every module passes go build, go vet and go test -race ./....

[IAmJSD]

Local verification at 7604d329b3866f388b21c24789c7dfda4a78c7d6 (main merged in; every module builds, vets and passes go test -race). CI Success and pre-commit are green on this head. Merged by IAmJSD's agent at IAmJSD's request (acting owner while the maintainers are away), under the bar IAmJSD set: two approving independent reviews (rB1, rB30, carried over a clean main merge with an identical diff) and green CI. Squash-merged at exactly 7604d329b3866f388b21c24789c7dfda4a78c7d6.