fix(azure): derive Advisor fields from the payload and fail loud on truncation

[cristim]

Closes #292
Refs #252 (item 1, the Advisor page cap; the subscription cap and the other uncapped pagers stay open)

Stacked on #366 (needs azrecs.SkipLog / RecordSkip). Base is fix/azure-consumption-skips-loud; merge #366 first, then retarget this to main.

Root cause (providers/azure/recommendations.go, at e9a33bb)

• convertAdvisorRecommendation hardcoded CommitmentReservedInstance, Term: "1yr", PaymentOption: "upfront" and let extInt default qty silently, so resource-level resize/shutdown recommendations (Microsoft.Compute impacted field) became RI purchase rows with no SKU and Count 0 (fix(azure): advisor rightsizing and shutdown recs emitted as ri purchase recs with count 0 #292).
• collectAdvisorRecommendations broke out of the loop on the 50-page cap or a page error and returned (recs, nil) (fix(azure): Advisor page cap returns partial results as complete; subscriptions cap size unproven #252).

Change

• Only subscription-scoped reservation (reservedResourceType) and Savings Plan (recommendationSubCategory) recommendations are converted; others are ignored. An unmappable reservedResourceType is a recorded skip.
• CommitmentType: SavingsPlan subcategory gives savings-plan, else RI. Term from term (P1Y/P3Y, or 1yr/3yr). Savings must carry savingsCurrency = USD. Reservations require sku and a positive qty. Anything absent or unknown is skipped and recorded, surfacing as the PartialResultError from fix(azure): surface skipped consumption recommendations as a typed partial-result error #366.
• Page cap and page error return the partial list plus errAdvisorTruncated, which GetRecommendations reports through the same PartialResultError.
• Key names: the SDK types ExtendedProperties as map[string]*string, so keys are not in the SDK. annualSavingsAmount, savingsCurrency, term, lookbackPeriod are the names in Microsoft's Advisor Resource Graph samples; reservedResourceType, recommendationSubCategory, sku, qty are the keys the collector already used. I could not confirm them against a live payload.

Savings Plan overlap with go#373

dropCoveredAdvisorSavingsPlans drops an Advisor SavingsPlan row when the savings-plan client returned an SP row for the same (account, term). It keys instead of dropping all Advisor SP rows so advice survives when the Benefit Recommendations source returns nothing (or #373 is not merged yet), and the Benefit row wins because it carries the commitment amount. Test: TestGetRecommendations_AdvisorSavingsPlanDedupedAgainstBenefitRecs. The key does not distinguish multiple SP rows for one term (e.g. different scopes); if #373 emits several, the key needs a scope field.

Not done, with reason

• Payment option: the payload carries no payment-plan field (none is documented), so the upfront and monthly variants are fanned out as on the Consumption path rather than inventing one.
• Cost fields: the payload has no price. RecurringMonthlyCost stays nil (unknown). CommitmentCost is a plain float64, so unknown can only be 0; representing that needs a pkg/common change, which is the architect's. Pricing from the retail catalog was left out of scope.

Evidence (fixture-based, built from armadvisor types; no live Advisor data)

New tests cover derivation, rejection of each unknown or missing value, the non-purchase filter, nil recurring cost, and truncation surfacing through GetRecommendations. The old tests asserting Term == "1yr" and PaymentOption == "upfront" for a no-SKU compute recommendation were removed. They encoded the bug, and the new tests do not compile against the old signatures, so there is no meaningful before-run. go test ./... in providers/azure passes, go vet and GOWORK=off go build ./... are clean, and pre-commit gocyclo passes.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 80e1f1e1-fe16-4b64-86a2-4be94f8597dd

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

• 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 (claude-opus-5-5) of 462df7e: APPROVE

Re-review against parent #366 (2de6006). Merge-base = 2de6006. Review only.

Source of truth for the extended-property keys:

• annualSavingsAmount, savingsCurrency, term, lookbackPeriod: Microsoft's "Active cost recommendations" query, learn.microsoft.com/azure/advisor/advisor-azure-resource-graph.
• sku, qty, reservedResourceType: microsoft/finops-toolkit src/power-bi/kql/Shared.Dataset/definition/tables/AdvisorReservationRecommendations.tmdl.
• recommendationSubCategory under extendedProperties: microsoft/mcp tools/Azure.Mcp.Tools.Optimization/src/Services/OptimizationKqlQueries.cs:130.

My earlier findings are resolved:

• An unmapped reservedResourceType is ignored with a debug log, not recorded as a skip.
• extFloat coverage is restored (TestConvertAdvisor_RejectsBadSavingsAmounts, including the real-zero case).
• SP rows use ServiceSavingsPlansAll.
• The dead extractServiceType/serviceFromImpactedField and their tests are gone.

Evidence (local, fresh clone, macOS, fixture-based):

• go vet and go test ./... in providers/azure pass.
• Mutations, each caught by a test: dropping the NaN/Inf guard and dropping the negative guard both fail RejectsBadSavingsAmounts; mapping SP to compute fails DerivesFieldsFromPayload; recording an unmapped RRT as a skip fails UnmappedReservedResourceTypeIsIgnoredNotSkipped; a dedupe key without term fails AdvisorSavingsPlanDedupedAgainstBenefitRecs; not recording truncation fails AdvisorTruncationIsSurfaced.
• CI has not run on this head, because the base is the fix(azure): surface skipped consumption recommendations as a typed partial-result error #366 branch. Fresh green CI on the exact head is required after retarget.

Non-blocking:

1. Advisor SP rows carry no commitment amount (nil Details, Count 0). They are advisory only: the savings plan PurchaseCommitment rejects them ("invalid service details"), so they cannot be bought by mistake.
2. Advisor emits one row per lookbackPeriod (the finops workbook renders it as "Last N days"), so RI rows can repeat across lookbacks. This was true before this PR; it is a follow-up item.
3. dropCoveredAdvisorSavingsPlans does nothing until feat(azure/savingsplans): implement Savings Plan recommendations from benefitRecommendations #373's Benefit Recommendations client is on this branch.