chore(azure/gcp): medium/low findings from the 2026-07-28 full review #52

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Tracking issue for the MEDIUM and LOW findings from the 2026-07-28 review of providers/azure (21 non-test files) and providers/gcp (7 non-test files). The CRITICAL and HIGH findings from the same pass are filed as individual issues.

Context: neither module is covered by CI (LeanerCloud/cloud-commitments-cli#1478), so nothing in this checklist has been vetted by go vet, golangci-lint, or the unit/integration suites.

Findings from this pass that are already tracked and are therefore not repeated as checklist items here (new evidence added as comments on each):

• Arbitrary first-ACTIVE-project selection in getDefaultProject -> LeanerCloud/cloud-commitments-cli#1535
• Unverified reservedResourceType literals for managedredis and search -> LeanerCloud/cloud-commitments-cli#1508
• NewClientWithHTTP nil-guard missing in 5 of 8 Azure clients -> LeanerCloud/cloud-commitments-cli#1304 (exact duplicate, still live at be11bdcb5)
• Silent extFloat coercion of annualSavingsAmount to 0 -> LeanerCloud/cloud-commitments-cli#1309 (the term half of that finding is item 1 below, and is not covered by LeanerCloud/cloud-commitments-cli#1309)

Each checklist item is independently actionable. Please keep the file:line, failure scenario and fix direction with the item when splitting any of these out.

• Azure Advisor recommendations carry an ISO-8601 term the purchase path always rejects (MEDIUM)

  • Where: providers/azure/recommendations.go:426-427 (populateFromExtendedProperties sets rec.Term = extString(ext, "term", "1yr")); reservations.ParseTermYears at providers/azure/services/internal/reservations/purchase.go:66-75; the two sibling parsers that do accept the ISO form, normaliseTerm at providers/azure/internal/recommendations/converter.go:274-287 and normalizeReservationTerm at pkg/common/reservation_name.go:163-175
  • What: Azure Advisor's reservation extendedProperties.term is the ISO-8601 duration form ("P1Y" / "P3Y"). ParseTermYears, the parser the purchase gate actually uses, explicitly rejects it: its allowlist is "" / 1 / 1yr / 1y and 3 / 3yr / 3y. The codebase carries three divergent term parsers and the Advisor path calls neither of the two that would handle the value.
  • Failure scenario: an Advisor-sourced recommendation reaches PurchaseCommitment and fails with unsupported reservation term: P3Y. Every Advisor recommendation is permanently un-purchasable while being displayed to the operator as actionable, and the failure surfaces only at purchase time, after approval.
  • Fix: route the Advisor term through the shared normaliser (or extend ParseTermYears to accept P1Y / P3Y) and consolidate the three parsers onto one. Add a test that an Advisor payload with term: "P3Y" purchases as a 3-year reservation.
  • Note: the savings half of the original finding (rec.EstimatedSavings = extFloat(ext, "annualSavingsAmount") / 12 returning a fabricated 0 on a missing key or parse error, at recommendations.go:442 with extFloat at :459-466) is already tracked by LeanerCloud/cloud-commitments-cli#1309 and should be fixed there. One dimension LeanerCloud/cloud-commitments-cli#1309 does not mention: Advisor's savingsCurrency extended property is never read either, so a non-USD savings figure is silently treated as USD.

• GCP maxRecsPages caps recommendation ITEMS (not pages), counts skipped ones, and errors out the whole region (MEDIUM)

  • Where: providers/gcp/services/computeengine/client.go:31 (the constant), :263-268 (the loop), :284-287 (the non-ACTIVE skip); correctly-named sibling constant maxMachineTypeItems at :39
  • What: the loop is for pageIdx := 0; ; pageIdx++ around it.Next(), which yields one recommendation per call, not one page. So the cap is 20 recommendations, not 20 pages. The counter also increments for recommendations skipped by the non-ACTIVE filter, and exceeding the cap returns an error rather than truncating.
  • Failure scenario: a project whose Recommender has accumulated 20 CLAIMED / SUCCEEDED CUD recommendations plus one ACTIVE one. The iterator hits pageIdx == 20 while walking the skipped entries, GetRecommendations returns the cap error, collectRegion logs a WARN, and the region contributes zero recommendations. The single actionable recommendation is never surfaced, and because the other services succeeded the run is reported as a success.
  • Fix: increment the counter only for ACTIVE recommendations actually converted (or count real pages), rename the constant to match its semantics, and raise the cap well above realistic recommendation counts.

• GCP cost projection ignores both CostProjection.Duration and Money.CurrencyCode (MEDIUM)

  • Where: providers/gcp/services/computeengine/client.go:1045-1065 (extractCostImpactFromRecommendation)
  • What: it reads costProj.Cost (google.type.Money) and negates Units + Nanos/1e9 into rec.EstimatedSavings, treating it unconditionally as a monthly figure. recommenderpb.CostProjection also carries Duration, which defines the period the cost applies to, and Money.CurrencyCode, which defines the units. Neither is read or validated.
  • Failure scenario: GCP returns a CUD cost projection over a 1-year Duration, so the recommendation is stored with EstimatedSavings 12x overstated; or the billing account is denominated in GBP and the figure is silently compared against USD thresholds and a USD-denominated MaxPurchaseAmount cap. Rankings and any savings-based approval gate act on the wrong number with no signal that anything is off.
  • Fix: normalise the projected cost by Duration to a monthly run-rate, and fail loud (drop the rec with a log) when CurrencyCode is absent or is not the expected billing currency. Money caps in this repo are USD-denominated with no currency field, so a non-USD figure must fail closed rather than be compared.

• Azure Redis reservations and recommendations are emitted twice under two different services (MEDIUM)

  • Where: providers/azure/services/cache/client.go:157 plus convertRedisReservation, vs providers/azure/services/managedredis/client.go:41 plus reservationDetailToCommitment; both purchase with armreservations.ReservedResourceTypeRedisCache (cache/client.go:306, managedredis/client.go:274); token derivation at pkg/common/tokens.go:64
  • What: both clients issue the identical Consumption filter properties/resourceType eq 'RedisCache', apply the identical commitment filter strings.Contains(lower(SKUName), "redis"), and differ only in the Service label they stamp (ServiceCache vs ServiceMemoryDB). provider.go:GetSupportedServices advertises both, and newServiceClientForSubscription constructs both. Nothing deduplicates them.
  • Failure scenario: a subscription with one Standard_C1 Redis reservation, swept over both services, yields two common.Commitment rows with the SAME CommitmentID under two service labels, so committed Redis spend is double-counted in inventory and in coverage/utilization denominators. On the recommendation side (reachable via GetServiceClient(...).GetRecommendations, for example the CLI multi-service path), the same Azure recommendation appears twice with distinct rec indices, so the idempotency token, derived only from (executionID, recIndex), differs between the two and the tag-based dedupe guard cannot stop a double purchase if both are approved.
  • Fix: decide which service owns Azure Redis. If both must exist, key deduplication on CommitmentID or (region, SKU, term) rather than on the service label, and fold the target SKU into the idempotency token. This interacts with LeanerCloud/cloud-commitments-cli#1508: if managedredis turns out to have no distinct reservation product, the cleanest resolution is to remove its purchase path entirely.

• GroupCommitments is exported dead code with a non-idempotent, timestamp-derived name (LOW)

  • Where: providers/gcp/services/computeengine/client.go:416-464
  • What: no non-test caller exists anywhere in the repo. It builds CommitmentRequests with Name: fmt.Sprintf("cud-%s-%d-%d", region, time.Now().UnixNano(), counter), bypassing the idempotentCommitmentName derivation (:1218) that the live purchase path relies on for double-buy protection, and it aggregates vCPU and memory across machine families into one commitment.
  • Failure scenario: no live impact today. If a future caller wires it into the purchase path, every re-drive mints a fresh name and buys a duplicate CUD, which is the exact failure the idempotent-name work (feat(purchases): make GCP Compute commitment creation idempotent (remaining slice of #641) cloud-commitments-cli#654) was added to prevent. The cross-family aggregation would additionally compound the separately filed CRITICAL about the commitment Type being hardcoded to GENERAL_PURPOSE.
  • Fix: delete it, or route its Name through idempotentCommitmentName and key the grouping on machine family before anything calls it.

Checked and explicitly NOT a defect

Recorded here so it is not re-derived or re-litigated.

Consumption ODATA resourceType filter literals vs Capacity reservedResourceType purchase literals. The two Azure APIs genuinely use different spellings and the repo has them right on both sides. armconsumption@v1.1.0/models.go:2463-2465 documents the Consumption filter's allowed values verbatim as ['VirtualMachines', 'SQLDatabases', 'PostgreSQL', 'ManagedDisk', 'MySQL', 'RedHat', 'MariaDB', 'RedisCache', 'CosmosDB', 'SqlDataWarehouse', 'SUSELinux', 'AppService', 'BlockBlob', ...]. Every filter in the module is drawn from that list: 'VirtualMachines' (compute/client.go:191), 'RedisCache' (cache/client.go:157, managedredis/client.go:41), 'CosmosDB' (cosmosdb/client.go:159 via reservationResourceTypeCosmosDB), 'SQLDatabases' (database/client.go:189 via reservationResourceTypeSQLDB), 'SqlDataWarehouse' (synapse/client.go:138 via reservationResourceTypeSynapse). The purchase side correctly uses the Capacity enum, which spells two of them differently: CosmosDb and SqlDatabases (armreservations@v1.1.0/constants.go:498,510). So the CosmosDB / CosmosDb and SQLDatabases / SqlDatabases divergences are correct, not drift. The same citation has been added to LeanerCloud/cloud-commitments-cli#1318, which was opened when three of these literals were still the older 'SqlDatabase' / 'CosmosDb' / 'SQLDatabaseDTU' spellings.

providers/azure/ri_utilization.go is correctly subscription-scoped (/subscriptions/{id} resourceScope at :96-99), unlike the savingsplans and exchange listings covered by the HIGH findings from this pass.

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A08b-032 (high)

The GroupCommitments bullet's contrast case is itself defective. idempotentCommitmentName (computeengine/client.go:1414-1417) returns cud- when the token is empty, and the live purchase call site at :787 passes opts.IdempotencyToken through with no check that one was set. Since the whole GCP dedupe mechanism is the name collision, an empty token means a retry a minute later gets a different name and buys a second commitment for the same capacity; two distinct purchases inside the same second derive the same name and the second is reported as a duplicate when it is genuinely new. The function's own doc at :1409-1410 records that the CLI path reaches the empty branch by design. Fixing GroupCommitments to route through this helper does not help until the helper rejects an empty token. Finding A08b-032.

A08-017 (medium)

The Advisor item has a third half neither this issue's term bullet nor LeanerCloud/cloud-commitments-cli#1309's savings bullet covers. convertAdvisorRecommendation (providers/azure/recommendations.go:420-433) stamps PaymentOption: 'upfront' on every Advisor recommendation, and populateFromExtendedProperties (:437-446) never writes that field. Advisor does not report a payment option at all, so the value is asserted rather than observed, and it reaches BillingPlanForPaymentOption as Upfront: the whole commitment is charged immediately for a recommendation the operator may have expected to bill monthly. Since all three defects are in the same twenty lines, worth fixing together: route the term through normaliseTerm, make extFloat distinguish absent from zero, and leave PaymentOption empty so the purchase path refuses it rather than defaulting. Finding A08-017.

A08b-020 (medium)

This item is three files and one constant short. The same per-item-cap-named-Pages shape is in cloudsql/client.go:181 (maxRecsPages at :22), cloudstorage/client.go:201 (:23) and memorystore/client.go:194 (:24), each incrementing pageIdx once per it.Next() with the non-ACTIVE skip after the increment, so all four services return zero recommendations and an error for a project with 21 Recommender rows. Separately, maxCommitmentsPages = 50 (computeengine/client.go:34) caps the existing-CUD walk at client.go:478 the same way, so a project holding 51 commitments fails coverage collection entirely, which is the worse direction: coverage silently reads as unknown on the input to sizing. Renaming the constants to items and raising them to values a real account cannot legitimately exceed should cover all five sites at once. Audit finding A08b-020, which also groups the sibling maxMachineTypeItems = 20 cap. Verified at commit 3c0f8ac9 (line numbers moved since be11bdcb5).

A08b-025 (medium)

The Redis double-emission item has a general form worth recording next to it. Each Azure service client claims any reservation whose lowercased SKU name contains its own keyword: database/client.go:278 on "sql", cache/client.go:247 on "redis", cosmosdb/client.go:248 on "cosmos", search/client.go:193 on "search", synapse/client.go:217-219 on a dw/scu prefix or a "synapse" substring. Nothing anchors these to a reserved-resource-type family, so any SKU string containing another service's keyword is cross-assigned, and two clients can both claim one reservation and report it under two different ServiceType values. The database/synapse pair is the likely live case (SqlDW-family names), though which client double-claims depends on the SKU string Azure actually returns, so it needs a live check rather than a source read. The durable fix is to classify on the reservation's declared reserved-resource type from the inventory API rather than substring-matching the SKU string, which would also close the Redis case structurally. Audit finding A08b-025.