[IAmJSD]

Root cause

ExchangeTarget (providers/azure/services/compute/exchange_operations.go) had no billing-plan field, and buildCalculateExchangeRequest hardcoded BillingPlan: ReservationBillingPlanUpfront on every reservationsToPurchase entry. So every Azure reservation exchange was priced, and then committed, as Upfront. Nothing in the request or the response showed this, and it contradicts the file's own contract that nothing is coerced.

Fix (providers/azure only, no pkg/ change)

• New optional field ExchangeTarget.BillingPlan armreservations.ReservationBillingPlan.
  • Unset ("") → Upfront, the same plan as before, so existing callers (the platform today) send a byte-identical body.
  • Upfront / Monthly (exact SDK enum spelling, checked against PossibleReservationBillingPlanValues()) → sent as that target's billingPlan, separately for each target.
  • Anything else (monthly, UPFRONT, Monthly, Annual, ...) → targets[i].billing_plan "<v>" is not a supported billing plan, returned from validateExchangeTargets before any HTTP call. It is never coerced to Upfront.
• Exchange leg: ExchangeRequest holds only sessionId (SDK v1.1.0 ExchangeRequestProperties), so the plan cannot be sent again at execute time and doesn't need to be. Azure commits the purchase terms it priced in the CalculateExchange session, so threading the plan into calculate also carries it into the executed exchange. The tests pin this: the execute body is exactly {"properties":{"sessionId":...}}.
• Test hook: Client.exchangeClientOptions *arm.ClientOptions is nil in production (SDK defaults, same as the old nil) and is set only through export_test.go. With it, the tests run the production buildCalculateExchangeCaller / buildDoExchangeCaller LRO wrappers and the SDK's own JSON serialization against a fake policy.Transporter. They don't stub around them.

Wire body (CalculateExchange reservationsToPurchase[].properties)

Input	Before	After
BillingPlan unset	"billingPlan":"Upfront"	"billingPlan":"Upfront" (full body identical, golden-asserted)
BillingPlan: Monthly	(not expressible; would be "Upfront")	"billingPlan":"Monthly"
BillingPlan: "monthly"	n/a	validation error, 0 HTTP requests

Execute body is {"properties":{"sessionId":"..."}} in every case, before and after.

Does Azure require the target plan to match the source?

No. Microsoft's exchange policy says the new reservation's total lifetime commitment must be at least the returned remaining commitment "whether you choose to pay monthly or upfront", and "If you're exchanging for a different size, series, region, or payment frequency, the term is reset for the new reservation." So either plan is allowed for any source. This module therefore does not tie the target plan to the source's. Whether Monthly is actually offered for a given SKU/agreement is Azure's call, reported back from CalculateExchange (policy error / API error), same as for terms. This is documented on the field.

Platform follow-up (LeanerCloud/cloud-commitments-platform, not in this PR)

The HTTP boundary has no billing-plan input yet, so after this PR every platform exchange is still Upfront, unchanged:

1. internal/api/handler_ri_exchange.go: add BillingPlan string \json:"billing_plan,omitempty"`toAzureExchangeTargetBody`.
2. validateAzureExchangeTargets: return a 400 for a non-empty value that isn't Upfront/Monthly. Decide whether to normalize case at the HTTP layer. The provider is exact-match on purpose.
3. toAzureExchangeTargets: pass BillingPlan: armreservations.ReservationBillingPlan(t.BillingPlan) through.
4. internal/api/ri_exchange_idempotency.go azureExchangeIdempotencyKey: include the (defaulted) billing plan in the target fingerprint. Otherwise an Upfront and a Monthly submit of the same targets collide as one logical exchange.
5. Bump the providers/azure pin in go.mod to the commit that merges this. Surface the plan in the UI/CLI/MCP request builders as needed.

Verification (local, macOS, at 506b8c2)

• providers/azure: go build ./..., go vet ./..., go test ./... (go.work) and GOWORK=off go build/vet + GOWORK=off go test -race -short ./...: all pass. Only this module changed. pkg/ is untouched, so no pkg-first split is needed.
• New services/compute/exchange_billing_plan_sdk_test.go (fake HTTP transport; never reaches Azure, and the package's network-guard proxy would catch it if it did):
  • TestExchangeSDK_UnsetBillingPlanKeepsUpfrontWireBody: full golden CalculateExchange body plus execute body. The same golden passes on origin/main's request builder (with only the options hook patched in), which proves the unset path is byte-identical.
  • TestExchangeSDK_MonthlyBillingPlanReachesWire: [Monthly, Upfront] targets → "Monthly", "Upfront" on the wire. Execute sends only the session.
  • TestExchangeSDK_InvalidBillingPlanRejectedBeforeHTTP: 4 bad spellings → error, zero recorded requests.
• Fail-before: on literal main these tests don't compile (no field). With the field added but main's hardcoded-Upfront / no-validation behavior restored, the Monthly and invalid tests fail.
• Mutation probes, all killed: hardcoded Upfront; validation dropped; unset→Monthly; case-fold/trim coercion; accept-any; first target's plan used for all targets; unset rejected.
• golangci-lint v2.10.1 (--allow-parallel-runners, repo .golangci.yml): 0 issues. gocyclo -over 10 on changed non-test files: clean (validateExchangeTargets = 9).
• git-secrets 1.3.0 (--register-aws + scripts/setup-git-secrets.sh patterns + repo .gitallowed, fresh repo, isolated HOME): changed files scan clean. A password-literal positive control is caught.
• Not done: no live Azure call or exchange (by design). Monthly acceptance by real Azure for a given SKU/agreement is not verified against a real account.

Note: exchange_operations.go was already over the 500-line guideline (570) before this change; it's now 615.

Labels

Mirrored from #89: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/s, type/bug, triaged (applied).

Closes #89

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Reservation exchanges can now specify a billing plan for each target. If no plan is specified, the existing Upfront billing behavior is preserved.
• Bug Fixes
  • Unsupported billing plan values are rejected before an exchange request is sent. Only the exact supported values, Upfront and Monthly, are accepted.

[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: 4c467d92-81a5-4d62-846a-577ed77e3e3c

📥 Commits

Reviewing files that changed from the base of the PR and between 506b8c2 and 478ea02.

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: 2a759398-ba2c-4a7a-8ec4-bb28ecf8f508

📥 Commits

Reviewing files that changed from the base of the PR and between 011be8f and 506b8c2.

📒 Files selected for processing: 5

• CHANGELOG.md
• providers/azure/services/compute/client.go
• providers/azure/services/compute/exchange_billing_plan_sdk_test.go
• providers/azure/services/compute/exchange_operations.go
• providers/azure/services/compute/export_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

Exchange targets now support optional billing plans. Unset plans default to Upfront; unsupported values fail validation before an HTTP request. The selected plans are sent in CalculateExchange requests. SDK-level tests verify the request bodies and execution session ID.

Changes: Azure exchange billing plans

Layer / File(s)	Summary
Validate and send target billing plans providers/azure/services/compute/exchange_operations.go, providers/azure/services/compute/client.go, providers/azure/services/compute/export_test.go, providers/azure/services/compute/exchange_billing_plan_sdk_test.go, CHANGELOG.md	ExchangeTarget accepts a billing plan. Validation defaults unset plans to Upfront and rejects unsupported values. CalculateExchange requests carry each target’s plan, while Exchange receives the priced session ID. SDK tests check the wire bodies and ensure invalid plans are rejected before HTTP requests.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant ComputeClient
  participant CalculateExchange
  participant Exchange
  Caller->>ComputeClient: Submit exchange targets
  ComputeClient->>ComputeClient: Validate and default target billing plans
  ComputeClient->>CalculateExchange: Send purchase requests with billing plans
  CalculateExchange-->>ComputeClient: Return priced session
  ComputeClient->>Exchange: Execute with session ID

Loading

Suggested reviewers: cristim

Merge Risk: ⚪ Minimal · up to 506b8

The provider-level change is ready for normal merge checks. Platform exchanges will continue to use Upfront until the separate follow-up.

Security architecture risk: 🔵 Low · up to 506b8

Billing-plan selection is validated before requests, existing callers retain Upfront behavior, and execution still uses the calculated session. No introduced authorization bypass was demonstrated. Remaining uncertainty concerns Azure’s session guarantees and how downstream callers enforce ownership and purchase guardrails.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed capability affects payment terms for requested reservation purchases under the existing Azure credential. The request carries caller-supplied reservation IDs and billing scopes, so exposure cannot be assumed limited to the client’s subscription string; actual ownership and permission bounds require caller and Azure authorization evidence.

Trust Boundaries and Controls

• observed — The new caller-controlled billing-plan value crosses into the Azure purchase request only after enum validation. Existing source-scope compatibility checks and scope inheritance remain in place. The test-only options setter does not add a production credential, transport, or endpoint-selection API.

Resilience and Maintainability Implications

• inferred — Replay, concurrent execution, and recovery after interrupted execution continue to depend on external session semantics rather than local transaction state or locking. The implementation’s idempotency comment and synchronous success transport do not independently establish those guarantees; this is an existing dependency, not a demonstrated regression.

Hardening Proposals

• proposed — Establish an authoritative session contract covering billing-term binding, expiry, replay, concurrent execution, and reconciliation after cancellation before relying on automated recovery for Monthly exchanges.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #89 requires Azure exchange targets to request a billing plan instead of always using Upfront. The provider implementation adds ExchangeTarget.BillingPlan, validates exact Upfront and `Month…	Add billing_plan to the Azure exchange HTTP target body. Validate non-empty values. Convert the value to ExchangeTarget.BillingPlan. Include the defaulted plan in the Azure exchange idempotency fingerprint. Add tests for Monthly, Upfron…

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 primary change: allowing Azure exchange targets to request a Monthly billing plan.
Out of Scope Changes check		The changes stay within the Azure compute exchange implementation and its tests. The ARM client-options hook exists only to route production SDK wrappers through a fake transport in tests. The changel…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (1 skipped: 1…

Full details: Out of Scope Changes check

Explanation

The changes stay within the Azure compute exchange implementation and its tests. The ARM client-options hook exists only to route production SDK wrappers through a fake transport in tests. The changelog documents the implemented provider behavior. These changes directly support issue #89 and do not show unrelated product behavior.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (1 skipped: 1 unsupported.)

Full details: Linked Issues check

Explanation

Issue #89 requires Azure exchange targets to request a billing plan instead of always using Upfront. The provider implementation adds ExchangeTarget.BillingPlan, validates exact Upfront and Monthly values before HTTP, sends each target plan in CalculateExchange, preserves the unset Upfront behavior, and tests these paths. The HTTP boundary still has no billing-plan input or conversion, as the PR states. Platform exchanges therefore cannot request Monthly, so the end-to-end issue objective remains incomplete.

Resolution

Add billing_plan to the Azure exchange HTTP target body. Validate non-empty values. Convert the value to ExchangeTarget.BillingPlan. Include the defaulted plan in the Azure exchange idempotency fingerprint. Add tests for Monthly, Upfront, invalid values, and distinct idempotency keys, or revise the issue to scope it explicitly to the provider layer.

✨ 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 adversarial review: CLEAN at 506b8c23bb01d6f5e017e4cdd845766c72c9b6cf (claude-go-pr-merge-3). The reviewer was a fresh Opus 5.5 agent with a cold brief that did not write this code, and it treated the PR body as a claim. It reviewed the full diff.

Blocking findings: none.

What the reviewer checked against the SDK source (armreservations v1.1.0, providers/azure/go.mod:14, from the module cache):

• constants.go:429-437: the only ReservationBillingPlan values are Monthly and Upfront.
• models.go:660: PurchaseRequestProperties.BillingPlan has the JSON tag billingPlan,omitempty.
• models.go:389-392: ExchangeRequestProperties contains only sessionId. So the claim holds: the execute leg cannot re-send or override the billing plan.

Isolation: exchangeClientOptions (client.go:136) is read only at exchange_operations.go:186 and exchange_operations.go:209. It is set only in export_test.go, so it is nil in production, which matches the old nil argument.

Logic:

• validateExchangeTargets runs before any HTTP call.
• The billing plan is taken from each target, not from the first target.
• Only "" defaults to Upfront. Any other spelling (monthly, UPFRONT, Monthly) is rejected without coercion.
• When the plan is unset, the request body is byte-identical to before.
• The only users of ExchangeTarget and buildCalculateExchangeRequest are in this package.

Mutation probes: 5 run, all caught by a failing test.

Probe	Caught by
Hardcoded Upfront	MonthlyBillingPlanReachesWire
Validation dropped	InvalidBillingPlanRejectedBeforeHTTP
EqualFold coercion	InvalidBillingPlanRejectedBeforeHTTP
First target's plan applied to all targets	MonthlyBillingPlanReachesWire
Unset defaults to Monthly	UnsetBillingPlanKeepsUpfrontWireBody and RequestBuilderWiring

Non-blocking notes:

1. Issue fix(azure): exchange purchase targets are always billed Upfront; Monthly not requestable #89 is only fully resolved once the platform sends a billing plan. Today the platform's HTTP boundary sends none, so its exchanges stay Upfront, the same as before this PR. The follow-up steps are listed in the PR body.
2. exchange_operations.go was already over the 500-line guideline before this PR: 570 lines, now 615.

Local verification (macOS, fixture/mock: fake HTTP transport around the real SDK clients and serialization; no live Azure call, no exchange):

• At 506b8c23, in providers/azure: go build ./... and go vet ./... pass, and GOWORK=off go test -race -short ./... passes (1875 tests).
• The PR merged onto current main 011be8f, which includes refactor(azure): move savings-plan ownership checks into scope.go #467 and fix(azure): attribute single-scope savings plans billed to a billing account #468 (both touch azure), merges without conflicts. On that merge, go build, go vet and the full GOWORK=off go test -race -short ./... pass. The three TestExchangeSDK_* tests and all TestCalculateExchange_* tests pass.
• Real-Azure acceptance of Monthly was not exercised, by design.

[cristim]

Merge evidence at 506b8c23bb01d6f5e017e4cdd845766c72c9b6cf (claude-go-pr-merge-3): the independent Opus 5.5 review is CLEAN at this SHA (verdict above). There are no open blocking findings or review threads. CI is green on this SHA: pre-commit run 38971604145 and Build & Test run 38971604136. Local verification on macOS used fixtures and mocks, with no live Azure call. It is recorded above, including a test merge onto main 011be8f, which passes. Closes #89, and the labels mirror that issue. CR waived: draft not reviewed by CR; repo gate satisfied by independent review + exact-revision local verification + green CI.

The platform follow-up to send billing_plan from the platform's HTTP boundary is still open; see the PR body.

[cristim]

@IAmJSD this is ready to merge but BEHIND main: the go ruleset requires up-to-date branches, and main moved to 011be8f (#467, #468). Please update the branch with Update branch or by merging origin/main. I don't push to contributors' branches. I already test-merged it onto 011be8f locally, and the full azure -race -short suite passed. A pure update carries the CLEAN verdict forward once I confirm the PR's own commits are unchanged. (claude-go-pr-merge-3)

[cristim]

@IAmJSD gentle ping: this is still blocked only on being BEHIND main (the ruleset requires up-to-date branches). Review, CI and evidence are all done at 506b8c23. Merging origin/main into the branch (or Update branch) is all that's left. After that I'll confirm the update is pure, re-run the azure suite on the new head, and merge. (claude-go-pr-merge-3)

[IAmJSD]

Merged origin/main in 478ea02 (no conflicts, no force-push). providers/azure builds and passes go test -short ./... under GOWORK=off.

[cristim]

Merge evidence at 478ea02941739f037ab9c31fea0a0774bebc573d (claude-go-pr-merge-3).

• Pure update: 478ea029 is a merge commit. Its parents are the reviewed 506b8c23 and current main 3def3f89. diff <(git diff 926d1d55 506b8c23) <(git diff 3def3f89 478ea029) prints nothing: the PR's own diff is byte-identical to the one reviewed. So the independent Opus 5.5 CLEAN verdict at 506b8c23 (above) carries to this head.
• CI green on 478ea029: pre-commit run 39043851370 and Build & Test run 39043851426.
• Local verification at 478ea029 (macOS; fixture/mock evidence, fake HTTP transport around the real SDK; no live Azure call): in providers/azure, go build ./... and go vet ./... pass, and GOWORK=off go test -race -short ./... passes. The TestExchangeSDK_* tests pass.
• Merge gate: no open blocking findings or review threads, and the labels mirror fix(azure): exchange purchase targets are always billed Upfront; Monthly not requestable #89. CR waived: repo gate satisfied by independent review + exact-revision local verification + green CI.

Thanks @IAmJSD for the update.