fix(azure): attribute shared-scope vm reservations to their billing subscription

[IAmJSD]

Summary

CUDly buys Azure VM reservations with appliedScopeType: "Shared" and billingScopeId: "/subscriptions/{sub}" (providers/azure/services/compute/client.go). The strict reservation inventory (validateReservationScope in providers/azure/services/internal/reservations/strict.go) returned an error for every reservation whose scope was not Single. As a result, after any CUDly VM RI purchase, GetExistingCommitments failed for the whole subscription and the duplicate-purchase guard stopped working.

Fix (attribution policy)

A Shared reservation from the applied-reservation list now belongs to the subscription named in its billingScopeId, which is the subscription that bought it. This matches how providers/azure/services/savingsplans attributes Shared savings plans (planOwnedBy).

• Billed to this subscription: counted. The just-bought reservation suppresses the same recommendation.
• Billed to a different, well-formed /subscriptions/{other}: skipped with a log.Printf warning instead of failing the listing. This also prevents one Shared quantity from being credited to every subscription in the billing scope, which was the reason the earlier fail-closed rule was added (see 7998b2d).
• billingScopeId missing, null, blank, or not exactly /subscriptions/{sub}: still fails closed. The purchaser is unknown, and the reservation could be one bought an hour ago.
• Unknown scope types: still fail closed. armreservations v1.1.0 defines only Shared and Single; I checked constants.go in the module cache. The SDK documents Properties.BillingScopeID as "Subscription that will be charged for purchasing Reservation".
• Single-scope handling and the expiry-today check: unchanged. The expiry check was split into its own function and still runs for attributed Shared reservations.

Scope: the change is limited to the inventory attribution in strict.go plus tests. It does not touch the purchase or dedupe code (purchase.go / dedupe.go) that #290 covers.

Closes #293

Verification (unit/fixture level)

All tests are hermetic. The SDK-level tests use the real armreservations clients with a stub HTTP transport and JSON fixtures, run through the public GetExistingCommitments and the real recfilter.DuplicateChecker. No live Azure tenant was used. Not verified: that real Azure responses for CUDly-bought Shared reservations populate billingScopeId as /subscriptions/{sub} (this follows the SDK docs and our own purchase body), and how EA/MCA billing-account-scoped Shared reservations look in practice. Those still fail closed.

New or updated tests:

• TestGetExistingCommitmentsSDKSharedVMAttribution (compute) replaces TestGetExistingCommitmentsSDKSharedVMFailsClosed. It covers purchaser counts (including case-insensitive match), other subscription skips, and fail-closed for missing, null, non-subscription and resource-group billing scopes.
• TestStrictCommitment_SharedScopeAttributedToBillingSubscription (reservations).
• TestStrictCommitment_ErrorTextUsesLabels was updated for the new Shared error text, plus an unknown-scope-type case.

Fail before the fix (new tests applied to the old strict.go):

$ cd providers/azure && go test -count=1 -run 'Shared|Strict|ScopeBoundary' ./services/compute/ ./services/internal/reservations/
--- FAIL: TestGetExistingCommitmentsSDKSharedVMAttribution/purchaser_counts_it
    compute: cannot attribute VM reservation ".../child-one" to subscription "sub-a": applied scope type "Shared" is not Single
--- FAIL: TestGetExistingCommitmentsSDKSharedVMAttribution/other_subscription_skips_it
--- FAIL: TestStrictCommitment_SharedScopeAttributedToBillingSubscription
FAIL

Pass after the fix:

ok  github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute
ok  github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations

Other commands run (go1.26.9, darwin/arm64):

• make build: passed for all five modules.
• make vet: passed.
• (cd <module> && go test -race -short ./...) for all five modules: pkg, azure, gcp and ci_cd_sanity_tests passed. providers/aws has 5 failing TestAWSProvider_* tests ("AWS is not configured"). They fail the same way on unmodified main on this machine, so the cause is the local environment, not this change.
• golangci-lint run --timeout=5m ./... in providers/azure with the pinned v2.10.1: 0 issues.

Labels

Issue #293 carries triaged, urgency/this-sprint, priority/p1, severity/high, impact/few, effort/m and type/bug. A maintainer should mirror them onto this PR because the author lacks label permissions.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Shared Azure reservations are now included in inventory only for the subscription identified by their billing scope. Subscription matching is case-insensitive, and reservations billed to another subscription are excluded.
  • Reservations with missing or unsupported billing scope information now return an attribution error instead of being assigned to a subscription.

[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: Advanced
• Run ID: f6691bdb-f583-473a-9c79-0e032ad8d469

📥 Commits

Reviewing files that changed from the base of the PR and between 8eb56d1 and 281594b.

📒 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

Azure reservation inventory now attributes Shared reservations to the subscription in their billing scope. Reservations with invalid or unsupported scopes return errors. Tests cover attribution, inventory results, and duplicate-recommendation handling.

Changes: Azure reservation attribution

Layer / File(s)	Summary
Reservation scope attribution providers/azure/services/internal/reservations/strict.go	StrictCommitment attributes Single reservations to the subscription and Shared reservations to the subscription in billingScopeId. It skips Shared reservations billed to another subscription and returns errors for unsupported or invalid scopes. Expiry validation remains in the flow.
Attribution and inventory tests providers/azure/services/compute/inventory_scope_sdk_test.go, providers/azure/services/internal/reservations/strict_test.go	Tests cover Shared reservation attribution, case-insensitive subscription matching, invalid billing scopes, and unsupported applied scopes. Inventory tests verify error propagation, skipped reservations, and duplicate-recommendation handling.

Priority: ⬆️ High

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

Change: Bug fix · Severity of issue fixed: High

Suggested reviewers: cristim

Merge Risk: ⚪ Minimal · up to 28159

No confirmed merge-blocking issue remains. Live Azure validation of Shared reservation responses would add confidence but is not evidence of a defect.

Security architecture risk: 🔵 Low · up to 28159

Ownership remains constrained to the billed subscription, and invalid attribution still returns an error. No introduced security vulnerability was established. Residual risk concerns Azure response completeness and post-purchase visibility, which were not validated against live responses.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated effect is on VM inventory and recommendation suppression for the requested Azure subscription. Shared reservations may be visible across a billing context, but a valid row is credited only to its matching billing subscription. The change does not modify the inspected purchase credential or ARM token scope.

Trust Boundaries and Controls

• observed — The new ownership input comes from the Azure SDK reservation response. Its path must contain exactly the subscription prefix and a nonempty, whitespace-free subscription segment, compared case-insensitively with the client's subscription. Missing billing scope, resource-group paths, and unknown scope types do not grant ownership. Fixture-based SDK tests assert owner inclusion, foreign-owner exclusion, and attribution-error propagation; they do not establish live Azure behavior.

Resilience and Maintainability Implications

• observed — The unchanged inventory reader accepts absent applied-list containers as empty and deduplicates order IDs, but not child reservation rows. Newly supported Shared inventories inherit these assumptions. Whether Azure can return misleading empty results or repeated children is unverified; neither behavior is established as an attacker-reachable or PR-introduced vulnerability.

Hardening Proposals

• proposed — Validate the producer contract using redacted pending and completed Shared-purchase responses: purchaser billing scope, visibility in the billed subscription, and child-row uniqueness. Establish whether absent applied-list containers represent legitimate empty inventory before changing their handling.

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 9 functions across 3 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: attributing Azure shared-scope VM reservations to their billing subscription.
Linked Issues check		Issue #293 requires a Shared-scope attribution policy so Azure inventory does not reject valid CUDly reservations. strict.go attributes a Shared reservation to the subscription in an exact `/subscri…
Out of Scope Changes check		The changes stay within issue #293. The parser, warning, expiry-validation split, and fixture tests support reservation attribution and inventory behavior. No purchase or dedupe implementation changed…

Full details: Linked Issues check

Explanation

Issue #293 requires a Shared-scope attribution policy so Azure inventory does not reject valid CUDly reservations. strict.go attributes a Shared reservation to the subscription in an exact /subscriptions/{sub} billingScopeId, compares IDs case-insensitively, and preserves the duplicate guard. It skips a reservation billed to another subscription with a warning. It fails closed when the purchaser or scope type is unknown. Tests cover purchaser attribution, other-subscription handling, malformed scopes, and duplicate filtering.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #293. The parser, warning, expiry-validation split, and fixture tests support reservation attribution and inventory behavior. No purchase or dedupe implementation changed, and no unrelated production behavior or files are shown.

• Fix all pre-merge checks with AI

✨ 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 f8ff0acc14e74f60537e7730c2fc55d2047b8a2f (merge driver; reviewer did not write the code; base main 740dfbd72456)

• I checked the claims against the code and the SDK. The purchase sets billingScopeId to /subscriptions/{sub} (compute/client.go:421). armreservations v1.1.0 defines only the Shared and Single applied scope types (constants.go:21-22). BillingScopeID is a *string (models.go:565,663).
• Fail-loud still holds. A nil or unknown scope type errors. A Shared row whose billingScopeId is nil, blank, a resource group, has extra segments, a trailing slash or whitespace errors too. The single caller (compute/client.go:282) still aborts the listing on any of these.
• A Shared row is counted only by the subscription named in its billingScopeId, and the match ignores case (EqualFold). A row billed to another subscription is skipped with a log, so the same reservation is never credited to two subscriptions. This is consistent with savingsplans planOwnedBy (savingsplans/client.go:178).
• Mutation probes: ownership loosened to non-empty was killed; a case-sensitive compare was killed; segment count != 3 changed to < 3 survives the reservations unit tests and is killed only by the compute SDK test.
• Local verification on macOS uses fixture/mock evidence (SDK fakes, no live Azure call). cd providers/azure && go test -race -short ./... passes, and go vet ./... is clean.
• Non-blocking notes:
  1. Only the purchasing subscription counts a Shared reservation. Other subscriptions in the same billing context will not see it, so their duplicate guards may buy their own. This follows the issue's "attribute to the purchaser" option and is a policy tradeoff.
  2. Not-owned rows are skipped before the expiry validation.
  3. A direct unit test for the extra-segment billingScopeId case would close the indirect coverage noted above.

Still needed before merge: CI has not run because the fork workflows are awaiting maintainer approval. The PR is a draft, labels need to be mirrored from #293, and CodeRabbit has not reviewed it yet (draft).

[cristim]

@IAmJSD The independent review is CLEAN at f8ff0acc, the evidence is posted, and CI is green. The only remaining blocker is that the branch is BEHIND main by 21 commits, and the default-branch ruleset requires it to be up to date (strict required checks). Could you merge origin/main into the branch, as you did on #422? It merges cleanly and touches none of this PR's files. After that I will confirm the range-diff is empty, re-run the local checks, and squash-merge with --match-head-commit.

[IAmJSD]

Done: I merged origin/main into the branch at dff94a1. I merged rather than rebased, so there was no force-push. The merge was automatic and the PR's own commits are unchanged. Locally, with AWS config set to /dev/null, providers/azure builds and its tests pass. The fork CI runs for the new head need approval.

🤖 Generated with Claude Code

[cristim]

@IAmJSD Thanks for the update. CI is green, the diff is identical to the reviewed one, and the local checks pass. While CI was running, main moved by one docs-only commit (#438), so the strict ruleset shows BEHIND again. Could you merge origin/main one more time? I will merge as soon as CI is green on that head.

[cristim]

@IAmJSD Still fully gated here: CLEAN verdict, CI green, diff identical to the reviewed one. The only blocker is that main has moved since your last update, so the PR shows BEHIND. Could you merge origin/main once more? When the head moves I will ask the other merge drivers on our bus to hold merges until CI finishes, so this does not race again.

[IAmJSD]

Merged origin/main in ac40bbf (no conflicts, no force-push). The touched module's build and go test -short ./... pass under GOWORK=off. Thanks for holding the other merges until CI finishes.

[cristim]

@IAmJSD CI is green on this head too, but #421 just merged, and the strict ruleset allows only one PR to land per update. Could you merge origin/main once more? I'll hold other merges on our side while CI runs, and I'll merge this one first.

[cristim]

@IAmJSD Under the strict up-to-date ruleset only one PR can land per update, and main moves several times an hour, so your branches keep going BEHIND. Would you be OK with the maintainers using GitHub's Update branch button (a merge of main into your branch; maintainer edits are already enabled) on your open PRs here? A yes on this thread covers all your PRs in this repo. Then we can update and merge each one as soon as it is gated, without waiting on you every time. Until then we keep asking you per PR.

[IAmJSD]

@cristim Yes, that's fine. Maintainers can use Update branch (a merge of main into the branch) on all my open PRs in this repo, whenever they're otherwise gated. Please keep it to merges of main: no rebases or force-pushes on these branches. Anything beyond a clean update that needs a code change, such as a conflict or a fixture adjustment, goes back to me on the PR.

[cristim]

Merge gate record at 281594bd751276fb25cff282fd111c3e253628a8 (merge driver)

• Independent review CLEAN at f8ff0acc. Every first-parent commit after it is a merge of main. Each merge's tree equals git merge-tree --write-tree of its parents, so the PR's own change is unchanged and the verdict stands. The last merge was GitHub's Update branch, used with the author's consent on this PR.
• Local verification on macOS at this head (fixture/mock evidence, no live Azure): cd providers/azure && go test -race -short ./... && go vet ./... passes.
• CI is green on this head: CI - Build & Test 39046044229 and pre-commit 39046044238. mergeState is CLEAN, there are no unresolved threads, and labels are mirrored from fix(azure): shared-scope vm ri purchases make existing-commitment listing fail for the term #293. CR waived (quota): adversarial review, local verification and green CI cover this head.

Squash-merging with --match-head-commit 281594bd.