[cristim]

Bumps pkg, providers/aws, providers/azure and providers/gcp from cloud-commitments-go to main 9cec521d6531 (v0.0.0-20261026094505-9cec521d6531). All four now share one pseudo-version, and there are no replace directives.

What it brings:

• Azure recommendations carry their currency (go#625).
• GCP 1-year and 3-year commitment SKUs are priced separately, and the GCP term parser is strict (go#628, go#630).
• The ladder holds a buffer purchase while its layer has a pending reshape (go#629).
• Earlier library changes the pins were behind on: go#578 (an unknown exchange mode fails closed), go#608 (AWS rejects unrecognised RI terms and payment options) and go#575 (GCP clients no longer store a ctx).

Values the platform can store, checked against the stricter library:

• Exchange mode: the library accepts auto and manual. The DB default is manual (migration 000010). An empty value is rewritten to manual on write. The UI and the exchange handler allow only manual and auto. A free-form value sent through PUT /api/config (fix(config): validate RIExchangeMode on PUT /api/config #1196, not yet validated) now fails closed instead of auto-executing.
• AWS payment and term: aws_payment.go normalises every stored payment to no-upfront, partial-upfront or all-upfront. Terms are sent as 1yr or 3yr.
• GCP term: the platform sends 1yr or 3yr, which the new parser accepts.

Other library changes that refuse input (checked in the first review against what the platform stores or sends):

• go#611, fix(api): require unrestricted scope to refresh recommendations #615 and fix(plans): flag planned purchases with no recommendations instead of showing zeros #616: the upfront ceiling (+5% over rec.UpfrontCost). The platform scales upfront cost with count, and a zero upfront skips the check.
• go#579: an untyped details payload is an error. Every service Azure and GCP emit decodes.
• go#566: aws-cn Savings Plans are refused.
• go#573 and fix(api): return 422 from the Azure revoke quote when the refund amount or currency is missing #591: pagination caps.
• go#619 and fix(dashboard): upcoming purchases show zeros for plan steps with no recommendations #620: Savings Plans date errors.
• go#493: the GCP PerformanceRecommender CUDs are removed.
• go#631: an Azure purchase whose currency doesn't match the billing currency is refused before submission. The execution is marked failed with the reason, the failure notification goes out, and it is not retried.

Product impact (all are explicit refusals, not wrong purchases):

• Azure reservation and Savings Plan purchases on a billing account that isn't in USD now fail before submission (go#631), in line with the USD-only rule decided on go#124.
• AWS: a purchase whose upfront is more than 5% above the approved upfront is refused. That includes a Cost Explorer fractional quantity rounded up (for example 2.5 becomes 3), which is being followed up in cloud-commitments-go.
• GCP: Cloud SQL and Memorystore no longer produce invalid recommendations (go#493, closes go#294). The provider used to turn the per-instance PerformanceRecommender's tuning tips into 1-year CUDs with zero or negative savings. Real CUDs for those services are spend-based, recommended at billing-account scope, and not collected yet. Non-USD CUD recommendations are dropped under the USD-only rule (go#124). Showing that drop to users is tracked separately.
• With 4-eyes off, a free-form exchange mode stored through PUT /api/config now fails the scheduled reshape with a task error and no notification. Validating the mode is fix(config): validate RIExchangeMode on PUT /api/config #1196.

Code changes: the second commit adapts to library API changes. No test expectation changed.

• computeengine.NewClient drops its ctx argument (go#575), so the one call in a test is updated.
• provider.ProviderConfig becomes provider.Config (40 SA1019 deprecation warnings; the old name was an alias).

Local:

• go build ./... and go vet -tags integration ./... are clean.
• go test -race ./... has no failures.
• -tags integration passes on purchase, scheduler, api, config and analytics.
• golangci-lint shows only one older prealloc finding. pre-commit passes.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 39 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 62 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: chore/bump-go-9cec521
• File: .coderabbit.yaml
• Commit: b4851b4
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: b4171f56-062b-48a3-a645-eecadd2fc394

Commits

Reviewing files that changed from the base of the PR and between bd33dd3 and b4851b4.

Files selected for processing: 3

• internal/api/handler.go
• internal/providerfactory/bypass_guard_test.go
• internal/server/ri_exchange_four_eyes.go

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: chore/bump-go-9cec521
• File: .coderabbit.yaml
• Commit: bd33dd3
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: 5a9c75e7-e4f9-417d-b5ab-e35db79dd209

Commits

Reviewing files that changed from the base of the PR and between 01fb35a and bd33dd3.

Files ignored due to path filters: 1

• go.sum is excluded by !**/*.sum

Files selected for processing: 19

• go.mod
• internal/api/combined_plan_purchase_integration_test.go
• internal/api/handler_purchases_pause_claim_integration_test.go
• internal/api/handler_purchases_retry_fanout_test.go
• internal/api/handler_purchases_run_now_integration_test.go
• internal/api/handler_recommendations_global_filters_integration_test.go
• internal/api/recommendation_identity_integration_test.go
• internal/providerfactory/providerfactory.go
• internal/providerfactory/providerfactory_test.go
• internal/purchase/azure_savings_plans_alias_proof_test.go
• internal/purchase/execution.go
• internal/purchase/execution_test.go
• internal/purchase/gcp_history_cost_test.go
• internal/purchase/mocks_test.go
• internal/purchase/plan_step_proofs_test.go
• internal/scheduler/recommendation_completeness_integration_test.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_test.go
• internal/scheduler/zonal_recs_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Walkthrough

The four cloud-commitments-go dependencies now use one pseudo-version. Provider factories, purchase execution, API test factories, and scheduler code now use provider.Config instead of provider.ProviderConfig.

Changes: Provider Config Migration

Layer / File(s)	Summary
Dependency and provider factory configuration go.mod, internal/providerfactory/*	The four dependencies now use the same pseudo-version. Azure and GCP provider factories and their test helper use provider.Config.
Purchase configuration resolution and execution internal/purchase/execution.go, internal/purchase/*_test.go	Purchase credential-resolution results and execution parameters use provider.Config. Related test matchers and calls use the updated type. A GCP test client call no longer passes ctx.
API and scheduler provider factory calls internal/api/*_test.go, internal/scheduler/scheduler.go, internal/scheduler/*_test.go	API test factories, scheduler provider construction, and related mock expectations use provider.Config.

Priority: Pending latest changes

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Possibly related issues

• LeanerCloud/cloud-commitments-go#624 — The dependency update matches the requested platform dependency bump. This diff does not show whether the separate re-drive refusal was removed.
• LeanerCloud/cloud-commitments-go#623 — The dependency update matches the requested platform dependency bump for Azure recommendation currency. This diff does not implement the USD-only plan-cap rule.

Suggested reviewers: iamjsd

Merge Risk: Minimal · up to bd33d

No actionable issue was established that should block merging this dependency and configuration-type update.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 18 files. (1 skipped:…	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 identifies the main change: a dependency update to cloud-commitments-go at commit 9cec521.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 18 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• 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 1 (cold, fresh context):

VERDICT: REQUEST_CHANGES bd33dd3

Findings:

• MEDIUM internal/providerfactory/bypass_guard_test.go:83 - the AST guard only matches composite literals named ProviderConfig. This PR moves every production site to provider.Config (alias, pkg provider/interface.go:121), so the guard no longer sees the natural spelling - a new &provider.Config{Profile: sub} (the platform#41 credential-bypass pattern) passes TestNoRegistryBypassInProductionCode. Reproduced in a scratch copy: a probe file with &provider.Config{Profile:"sub"} -> test PASS; same file with provider.ProviderConfig{...} -> FAIL. Fix: match Config || ProviderConfig (and update the messages at :32/:53/:108). Staticcheck SA1019 on Profile is only a lint backstop that a //nolint can silence.
• LOW (follow-up) internal/server/ri_exchange_four_eyes.go:17-20, internal/api/handler.go:582-590 - comments say pkg/exchange runs every non-"manual" mode as auto. Since go#578 (pkg exchange/auto.go) it rejects them. armed() is still safely conservative. A typo mode stored through PUT /api/config (unvalidated, fix(config): validate RIExchangeMode on PUT /api/config #1196) now makes the scheduled exchange fail loudly instead of auto-executing, which is the correct direction. Validate RIExchangeMode in GlobalConfig.Validate as part of fix(config): validate RIExchangeMode on PUT /api/config #1196.
• LOW (follow-up) internal/purchase/execution.go:1160-1169 - rec.CommitmentCurrency is not copied into common.Recommendation. The library treats "" as USD (azure internal/reservations/purchase.go:242-251, savingsplans/client.go:443-449, aws ec2/client.go:238), so behaviour is correct today. Passing it through would be defense in depth.
• INFO (follow-up) go#441 adds UsageBaseline.Basis=Residual. The platform ladder (internal/server/handler_ladder.go:425) does not set it, so it keeps the old double-subtract behaviour (platform half of go#282). go#614 net Existing is not wired yet (platform#243).
• INFO: the PR body leaves out several fail-closed changes the platform can hit: go#611/fix(api): require unrestricted scope to refresh recommendations #615/fix(plans): flag planned purchases with no recommendations instead of showing zeros #616 (upfront ceiling vs rec.CommitmentCost, +5%), go#579 (untyped details payload is an error), go#566 (aws-cn SP refused), go#573/fix(api): return 422 from the Azure revoke quote when the refund amount or currency is missing #591 (pagination caps fail loud), go#619/fix(dashboard): upcoming purchases show zeros for plan steps with no recommendations #620 (SP date errors), go#493 (GCP PerformanceRecommender CUDs removed).

Attacked:

• Build, vet -tags integration and go mod tidy -diff are clean. go test -race ./... shows no FAIL lines. The integration tests for purchase, scheduler, api, config and analytics all pass. git merge-tree against origin/main gives a clean tree (a9c9c745).
• go.sum: only the 8 hashes for the 4 cloud-commitments-go modules changed. go.mod changed only those 4 requires. Tidy is clean, so there are no new transitive dependencies.
• Rename: the diff is ProviderConfig->Config plus computeengine.NewClient dropping ctx (go#575, test only). Config is a type alias, so no behaviour changes.
• go#608: scheduler.go:1116-1121 sends "%dyr" (DefaultTerm is validated as 1|3, validation.go:456, DB default 3). Payment goes through awsRecommendationPayment (aws_payment.go:15) into the AWS canonical set. Lookback is validated as 7|30|60 (validation.go:395), with 0 mapped to 7. Every value is accepted by converters.go:34-75.
• go#578: handler_ri_exchange.go:162 and ladder_write.go:86 pass the mode through fourEyesExchangeMode. The dedicated endpoint allows only manual|auto (api/handler_ri_exchange.go:3002), an empty value is stored as "manual" (store_postgres.go:313), and the ladder always passes auto.
• go#605: there is no production ReshapeBuffer caller (the ladder is plan-only).
• go#629: Holds are already handled in the plan, and the ladder tests pass.
• go#628/fix(history): signal when the history fetch hit its row cap #630: purchase sends "%dyr" (execution.go:1166). The GCP TermYears parser accepts 1yr and 3yr.
• go#611/fix(api): require unrestricted scope to refresh recommendations #615/fix(plans): flag planned purchases with no recommendations instead of showing zeros #616: approved = rec.UpfrontCost, scaled linearly with Count in planstep/resolve.go:418 and api/purchase_pricing.go:152. Zero skips the check (offeringprice NoUpfrontApproved). Only a stale price beyond 5% is refused, which is intended.
• go#579: every service slug Azure and GCP emit maps to a typed Details, except storage (GCP Cloud Storage sets no Details), so stored rows still decode.
• go#631: a refusal comes back as a PurchaseCommitment error before submission. It goes through execution.go:1216-1221 to applyAccountOutcome status "failed" with the reason (execution.go:412-414), plus the deferred notifyExecutionFailure (execution.go:313). It is not ErrOutcomeUnknown, so there is no recovery or re-drive loop.
• sec(purchase): refuse a capped purchase whose price is not provably USD #1296 (api/purchase_currency.go:23): Azure consumption and SP recs now carry "USD", so capped grants can buy them. That is consistent because go#631 refuses non-USD billing at purchase. Advisor recs stay ""/unknown. step_cap.go:28 has no currency logic and is unchanged.
• No real-account verification was done. GCP Recommender and Azure calculatePrice shapes were checked only against library code.

[cristim]

Independent review 1, delta (same cold reviewer):

VERDICT: APPROVE b4851b4

Delta review of b4851b4 on top of bd33dd3. Three files changed: bypass_guard_test.go, api/handler.go (comment only), server/ri_exchange_four_eyes.go (comment only).
Findings:

• INFO (follow-up, not a regression) internal/providerfactory/bypass_guard_test.go:89 - a dot-import (. ".../pkg/provider" then Config{Profile:...}) still passes the guard. The literal type is an *ast.Ident, not a SelectorExpr. The guard does not claim to cover this case, it also slipped through at bd33dd3, and staticcheck SA1019 on Profile backs it up.
  Attacked:
• Production-like probe file in internal/probex, scanned by TestNoRegistryBypassInProductionCode:
  • &provider.Config{Profile:"sub"}: FAIL, flagged at p.go:5:26 (was PASS at bd33dd3).
  • aliased import p ".../pkg/provider" with &p.Config{Profile:"sub"}: FAIL, flagged at p.go:5:19. The import path is resolved through the local-name map.
  • a real local package foo with foo.Config{Profile}: ok.
  • dot-import: ok (see INFO).
• The new table test TestBypassGuardMatchesConfigNames covers Config, ProviderConfig and an unrelated foo.Config.
• Comment claims checked against the library: pkg exchange/auto.go:195-203 validateMode rejects "" and unknown modes. The armed() and fourEyesExchangeMode code is unchanged; only the comments changed.
• go test -race -count=1 ./internal/providerfactory/... ./internal/api/... ./internal/server/... all ok. go vet ./internal/providerfactory/ is clean. git merge-tree against origin/main is clean (4093d120).
• No go.mod or go.sum change in the delta. The earlier findings at bd33dd3 (build, tests, library compatibility) still hold.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE at b4851b4

Blocking findings: none.

Non-blocking findings:

1. Azure reservation purchases on a non-USD billing account now fail for every rec. The platform never forwards CommitmentCurrency into common.Recommendation (internal/purchase/execution.go:1160-1169). The library treats an empty currency as USD and refuses when calculatePrice bills in anything else or omits billingCurrencyTotal (providers/azure .../services/internal/reservations/purchase.go:244-256, 468). The new Azure recs are retail-priced and stamped USD (internal/recommendations/consumption.go:247, 287), so forwarding the currency would not unblock EUR accounts either. The failure is loud: the purchase errors out. It is intended by go#631, but the PR body does not mention it. Release notes should say that non-USD Azure billing accounts cannot buy reservations after deploy.
2. The bump brings a new AWS approved-upfront ceiling that the body does not mention. EC2, RDS, OpenSearch, Redshift and MemoryDB refuse when FixedPrice x Count exceeds rec.CommitmentCost by more than 5% (aws .../internal/offeringprice/offeringprice.go:139-172; services/ec2/client.go:170-174, 233-251). The platform passes UpfrontCost scaled linearly with count (api/purchase_pricing.go:148-152, planstep/resolve.go:418), so normal flows pass. Two cases are refused, both loudly: a reprice of more than 5% since approval (intended), and a CE fractional quantity rounded up (recommendations/parser_ri.go:173 math.Round). Example: 2.5 is rounded to 3, so 3000 is offered against 2500 approved and refused. Legacy rows with an upfront of 0 skip the check, as before.
3. GCP: Cloud SQL and Memorystore recs are no longer collected (gcp recommendations.go isUnsupportedRecommendationService). Compute CUD recs whose cost projection is not in USD are now skipped with only a log line (services/computeengine/client.go:1183, 469). These recs are advisory, so this is a silent drop with no money impact.
4. Exchange mode: when 4-eyes is off, a stored non-canonical mode now makes the scheduled reshape fail with "auto exchange failed: invalid exchange mode" (pkg exchange/auto.go:197-206, 238) instead of executing as auto. PUT /api/config still stores free-form strings (fix(config): validate RIExchangeMode on PUT /api/config #1196). The error surfaces only as a task error with no notification. This is safe, and it is better than the old behaviour.
5. Stale comments still name ProviderConfig: internal/purchase/execution.go:502 and internal/providerfactory/providerfactory_test.go:145.
6. Gaps in the bypass guard that also existed before this PR: a field assignment (c.Profile = "x") and a local type alias (type t = provider.Config) both get past it.

Fail-closed table (library change / input / newly refused / platform-reachable / overlap / loud or silent / evidence):

• go#578 exchange mode / RIExchangeConfig.Mode / everything except "auto" and "manual" (including "", "Auto", " manual") / "manual" (DB default, migration 000010; store_postgres.go:313 rewrites "" to manual), "auto" (validated route handler_ri_exchange.go:3002, UI), free-form via PUT /api/config / overlap only with 4-eyes off and a free-form stored value / loud task error / pkg exchange/auto.go:197,238; server/ri_exchange_four_eyes.go:23; server/handler_ri_exchange.go:162
• go#578 cancelStalePending / store error / now aborts the run (was a warning) / the adapter can error / loud / auto.go:220-233
• go#608 AWS RI params / Term, Payment, Lookback for CE / non-empty unknown values / "%dyr" with DefaultTerm 1 or 3 (validation.go:456, DB default 3), payment normalised to the three AWS tokens (scheduler/aws_payment.go:15-28), lookback 7/30/60 (types.go:149) / no overlap / loud / aws recommendations/client.go:181-209, converters.go:33-74; scheduler.go:1116-1121
• AWS upfront ceiling / CommitmentCost vs offering / more than 5% over, non-USD offering, missing FixedPrice / see finding 2 / repricing and fractional rounding / loud / as in finding 2
• ElastiCache / Engine / empty engine refused (before, an empty ProductDescription was sent) / legacy rows with no engine (applyEngineFallback, execution.go:1191) / rare / loud / services/elasticache/client.go:285-288
• go#628 and go#630 GCP terms / rec.Term / the purchase termPlan accepted set is unchanged (1yr/1/12mo/3yr/3/36mo); pricing now errors instead of defaulting to 1yr / platform sends "1yr" or "3yr" and never calls GetOfferingDetails / no overlap / not applicable / gcp computeengine/client.go:48-62, internal/skumatch/term.go
• go#631 Azure currency / billingCurrencyTotal vs rec currency / non-USD or missing billing currency / every non-USD Azure billing account / yes / loud / finding 1
• Azure SP / PlanType and currency / unknown PlanType, currency mismatch / library-produced "Compute" and USD / no overlap / loud / services/savingsplans/client.go:443-449, 351-357
• pkg codec / Details on an untyped service slug / now an error (before, nil) / savingsplans-* dash-free slugs, storage, other / no regression: the SP client already refused nil details (old savingsplans/client.go:477-479) / loud / common/service_details_codec.go:97-106
• ladder requireCap nil / BufferReshapeConfig / nil caps / no production caller of ReshapeBuffer / no overlap / not applicable / aws ladder/reshape.go:133-145

Call-site adaptation: the only changes are the ProviderConfig to Config alias rename and the computeengine.NewClient ctx drop in one test. Both preserve behaviour. No library error is swallowed: PurchaseCommitment errors return through execution.go:1215-1230. The b4851b4 comment fixes match the probed behaviour: "auto" executes, any other unknown mode is refused, and 4-eyes downgrades every non-"manual" value.

Probes:

• Exchange (fourEyesExchangeMode into RunAutoExchange, 10 modes x 4-eyes on/off): with 4-eyes off, only "manual" and "auto" pass and the other 8 are refused. With 4-eyes on, all 10 pass as manual.
• Azure compute PurchaseCommitment with a fake ARM HTTP client: an empty rec currency is refused for EUR, GBP and a missing billing currency, and passes the currency check for USD and "usd". An "EUR" rec is refused for USD billing.
• EC2 PurchaseCommitment with a fake API: same price at the same, lower or higher count passes; +4% passes; +6% is refused; an upfront of 0 passes; the fractional 2.5-to-3 rounding is refused.
• Bypass guard on a scratch copy at b4851b4: Config{Profile}, ProviderConfig{Profile}, &Config{Profile}, an aliased-import Config{Profile} and NewAzureProvider each fail the guard. At bd33dd3, Config{Profile}, &Config{Profile} and the aliased form passed the guard, which confirms the fix. Plain Config{Name} is allowed by design.
• Rename audit: the only renamed identifier is provider.ProviderConfig to Config (kept as an alias). The removed pkg/config, pkg/errors, common.Matches and scorer.DefaultScorerConfig are not used by the platform. The Azure and GCP constructor sets are unchanged. No other source-scanning test matches library identifiers.
• No stored-plan fixture exists to replay. The integration tests and the probes above stand in for one.

Commands, all clean:

• At bd33dd3: go build ./...; go vet with and without -tags integration; go mod verify; go mod tidy -diff (empty); gofmt; golangci-lint on purchase, scheduler, api and providerfactory (0 issues); go test ./... (no failures); integration tests under lockf for purchase, scheduler, api and config (all ok).
• At b4851b4: build, both vets, mod verify, tidy, gofmt and lint (0 issues) clean; go test ./... rc=0 with 45 packages ok; git merge-tree --write-tree onto origin/main 01fb35a is clean (4093d120).
• Integration tests were not re-run at b4851b4, because the delta from bd33dd3 is one test file and two comments.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification at b4851b41d60a9c032bb00b5591ec823d7b4085e5 merged onto current main (fba4826d; merge-tree b3a25b30; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration passes on real Postgres (testcontainers) for the touched packages: ./internal/api ./internal/purchase ./internal/scheduler ./internal/server
• Nothing was applied to any cloud.