fix(aws/savingsplans): paginate GetExistingCommitments via NextToken loop by cristim · Pull Request #1039 · LeanerCloud/cloud-commitments-cli

cristim · 2026-06-07T09:57:53Z

Summary

GetExistingCommitments in services/savingsplans/client.go was calling DescribeSavingsPlans once with no NextToken loop, silently truncating to the first page of results.
On accounts with more SPs than one page, CUDly under-reported existing commitments, causing duplicate-detection and expiry-coverage math to recommend or auto-purchase redundant SPs (financial overspend).
Adds the standard NextToken accumulator loop with a ctx.Err() terminal guard and a maxCommitmentPages=100 page cap, consistent with the MemoryDB and OpenSearch commitment fetches and the ctx-cancel pattern from the feedback_ctx_cancel_terminal memory entry.
Also fixes a pre-existing build break: two TestLookupOfferingID_* tests called findOfferingID with 2 args after its signature grew to 3 args on this branch; updated to pass empty string as execID.

Test plan

TestClient_GetExistingCommitments_Pagination: two-page mock asserts both items accumulated (would fail on pre-fix code returning only page 1)
TestClient_GetExistingCommitments_CtxCancellation: cancel fires after page 1; asserts context.Canceled propagates verbatim (not swallowed)
All 57 savingsplans tests pass (go test ./services/savingsplans/...)
All 726 aws provider tests pass (go test ./... from providers/aws)
Root-level go build ./... passes; 2 pre-existing auth test failures are unrelated to this change (confirmed by running without changes)

Scope expanded

Added finding 08-L1 (SP portion) from the code-review remaining-findings plan (FOLD-1039 bucket):

findOfferingID called DescribeSavingsPlansOfferings without narrowing by
currency or region, then returned SearchResults[0] without sorting. AWS does
not guarantee response ordering, so when multiple offerings matched (e.g. USD
vs CNY, or global vs region-scoped) the selected offering ID was
nondeterministic.

Changes in commit 9d292dcf5:

Added Currencies: [USD] to DescribeSavingsPlansOfferingsInput to exclude non-USD offerings server-side.
For EC2Instance SPs (the only region-scoped SP type), added a SavingsPlanOfferingFilterAttributeRegion filter element using c.region. Compute, SageMaker, and Database SPs are global and receive no region filter.
Changed lookupOfferingID to accumulate all offering IDs across pages, sort lexicographically, then return the first - stable regardless of AWS response ordering.
Added 5 regression tests covering: USD filter sent, EC2Instance region filter sent, Compute no region filter, deterministic single-page sort, deterministic cross-page sort.
All 731 aws provider tests pass after the change.

Also closes #1066

…loop DescribeSavingsPlans paginates (NextToken on both input and output) but GetExistingCommitments was making a single un-paginated call, silently truncating the SP list to the first page. On accounts with more SPs than one page, CUDly under-reported existing commitments, causing duplicate- detection and expiry-coverage math to see lower coverage than reality and recommend or auto-purchase redundant SPs (financial overspend). Add the standard NextToken accumulator loop consistent with the MemoryDB and OpenSearch commitment fetches: ctx.Err() guard at the top of each iteration (cancellation is terminal, not accumulated), MaxResults=100 per page, and a maxCommitmentPages=100 page cap that logs a warning and returns partial results (data loss is worse than a hard error for the commitment listing path). Also add two regression tests: - two-page mock asserts all items are accumulated (fails pre-fix, passes post-fix) - cancellation test asserts ctx.Err() propagates verbatim on cancel Fix pre-existing build break: two TestLookupOfferingID_* tests called findOfferingID with 2 args after its signature grew to 3 args in the same branch; pass empty string as execID to match ValidateOffering. Closes #1019

coderabbitai · 2026-06-07T09:57:58Z

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 45 minutes and 11 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9f16f69b-ecb9-4468-8d2c-3f3d86f2c856

📥 Commits

Reviewing files that changed from the base of the PR and between 202f70f and 9d292dc.

📒 Files selected for processing (2)

providers/aws/services/savingsplans/client.go
providers/aws/services/savingsplans/client_test.go

📝 Walkthrough

Walkthrough

The PR fixes issue #1019 by enabling pagination in GetExistingCommitments. The method now loops through all DescribeSavingsPlans pages using NextToken, accumulates commitments across pages with per-iteration context cancellation checks, enforces a page cap of 100, and maintains existing plan-type filtering and timestamp parsing. Tests validate pagination accumulation and context cancellation behavior, and test invocation signatures are updated for compatibility.

Changes

Savings Plans Pagination Fix

Layer / File(s)	Summary
Pagination implementation with page limit constant `providers/aws/services/savingsplans/client.go`	`maxCommitmentPages` constant (100) is introduced, and `GetExistingCommitments` is refactored from a single API call to a `NextToken` pagination loop that accumulates all commitments across pages, checks context cancellation per iteration, warns and returns partial results when the page cap is exceeded, and retains plan-type filtering and timestamp parsing.
Pagination test coverage `providers/aws/services/savingsplans/client_test.go`	Two new tests validate that `GetExistingCommitments` accumulates commitments across multiple `DescribeSavingsPlans` pages and correctly propagates `context.Canceled` when the context is cancelled during pagination.
Test invocation signature updates `providers/aws/services/savingsplans/client_test.go`	Existing test calls to `client.findOfferingID` are updated to pass the additional third argument to match the updated API signature.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🐰 Pagination loops now complete the tale,
NextToken by NextToken, no result shall fail.
With context checks standing guard at the gate,
A hundred pages max—balanced and great!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: implementing pagination for GetExistingCommitments using NextToken, which directly addresses the primary fix described in the PR.
Linked Issues check	✅ Passed	The PR implements all requirements from issue `#1019`: adds NextToken pagination loop, includes ctx.Err() guard, enforces maxCommitmentPages cap, and properly accumulates commitments across pages.
Out of Scope Changes check	✅ Passed	All changes are within scope: pagination implementation in client.go and corresponding test updates in client_test.go directly address issue `#1019` objectives.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/aws-sp-pagination

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

cristim · 2026-06-07T09:57:58Z

@coderabbitai review

coderabbitai · 2026-06-07T09:58:03Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

The pagination loop added in this branch pushed GetExistingCommitments to cyclomatic complexity 15 (budget 10), failing the pre-commit gocyclo hook in CI. Pull the per-SavingsPlan conversion (nil-ID guard, plan-type filter, and the two RFC3339 time-parse branches) into a toCommitment helper that returns (Commitment, ok). This is a behavior-preserving extraction: the loop now skips entries when ok is false, identical to the previous continue branches. GetExistingCommitments drops back under the limit; the existing two-page and cancellation regression tests still pass unchanged.

cristim · 2026-06-07T11:03:01Z

pr-iterate: the failing CI (the gocyclo pre-commit hook flagging GetExistingCommitments at complexity 15) is fixed in 34cdbe3 by extracting a toCommitment helper; the function is now back under the budget and all pre-commit hooks pass locally. CodeRabbit shows "Review skipped" and the org's CR usage credits appear exhausted (billing-blocked), so no substantive CR pass is available right now. This PR is otherwise on track to merge-ready once CI goes green. Re-invoke /pr-iterate 1039 after the org's CR billing is topped up to get a CR review. To clear the local block marker after billing is fixed: rm ~/.claude/agent-comms/cr-rate-limit-deadline.txt

… deterministic sort to findOfferingID DescribeSavingsPlansOfferings was called without currency or region constraints, so non-USD offerings and (for EC2Instance SPs) off-region offerings could appear in SearchResults. The first element was then returned without sorting, making the selected offering ID nondeterministic across API calls. - Set Currencies: [USD] on DescribeSavingsPlansOfferingsInput so non-USD currency variants are excluded server-side. - For EC2Instance SPs (the only region-scoped SP type), add a SavingsPlanOfferingFilterAttributeRegion filter element using the client's region. Compute, SageMaker, and Database SPs are global and receive no region filter. - In lookupOfferingID, accumulate all matching offering IDs across pages, sort lexicographically, then return the first so the result is stable regardless of AWS response ordering. Closes #1066. Finding 08-L1 (SP portion).

cristim · 2026-06-07T11:22:11Z

FOLD-1039: added finding 08-L1 (SP portion)

Finding: findOfferingID always picks SearchResults[0] without ordering guarantee (08-L1, Low severity)

Root cause: DescribeSavingsPlansOfferings was called without currency or region constraints, and the first result from an unordered API response was returned blindly.

Commit: 9d292dcf5 -- fix(aws/savingsplans): add USD filter, EC2Instance region filter, and deterministic sort to findOfferingID

Changes:

providers/aws/services/savingsplans/client.go: Added Currencies: [USD] to DescribeSavingsPlansOfferingsInput; for EC2Instance SPs added a region filter element (SavingsPlanOfferingFilterAttributeRegion = c.region); changed lookupOfferingID to accumulate all IDs across pages, sort lexicographically, and return the first.
providers/aws/services/savingsplans/client_test.go: 5 new regression tests -- USD filter asserted, EC2Instance region filter asserted, Compute no-region-filter asserted, deterministic single-page sort, deterministic cross-page sort.

Verification: All 731 aws provider tests pass; root go build ./... passes.

Closes #1066.

@coderabbitai review

coderabbitai · 2026-06-07T11:23:06Z

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the updated savingsplans client.go to review the findOfferingID / lookupOfferingID changes
fd -e go -p 'savingsplans/client' | xargs cat

Length of output: 94144

@cristim The implementation for finding 08-L1 looks correct and well-covered:

USD currency filter — added server-side in findOfferingID, asserted in TestFindOfferingID_SendsUSDCurrencyFilter. ✓
EC2Instance region filter — conditional on c.planType == SavingsPlanTypeEc2Instance, with a graceful empty-region guard and log warning. Asserted in TestFindOfferingID_EC2InstanceSendsRegionFilter and the inverse in TestFindOfferingID_ComputeNoRegionFilter. ✓
Deterministic sort — lookupOfferingID accumulates all IDs across pages, sorts lexicographically, returns offeringIDs[0]. Asserted by both TestLookupOfferingID_DeterministicSort (single-page) and TestLookupOfferingID_DeterministicSortAcrossPages (cross-page). ✓
ctx.Err() guard — present at the top of the lookupOfferingID loop, consistent with the pattern used in GetExistingCommitments. ✓

One minor style note (not a blocking concern): lookupOfferingID advances pagination by mutating input.NextToken = result.NextToken in place, while GetExistingCommitments uses a separate nextToken local variable. Both are correct; mentioning it only for future consistency if a style guide is ever codified.

The 5 regression tests are a solid safety net against regression. Triggering the review now.

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 339
File: providers/aws/recommendations/coverage.go:177-205
Timestamp: 2026-05-13T21:30:43.093Z
Learning: In the CUDly project (LeanerCloud/CUDly), the CLI flag for target-based coverage sizing is `--target-coverage` (not `--target-utilization`). The PR renamed it during design iteration: "utilization" (how full each RI is) stays ~100% under the new sizing, while "coverage" (% of demand covered) is the user-facing knob. `ce:GetReservationCoverage` is required only when `--target-coverage` is set, gated by `cfg.TargetCoverage <= 0` in `cmd/multi_service.go`. The legacy `--coverage` path is unaffected and pays no CE cost or IAM change.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…t default convertPaymentOption and convertTermToSeconds previously logged a warning and silently substituted All Upfront (largest cash outlay) or 1 year when given an unrecognized or empty value, risking a mis-purchase on any typo or new/renamed AWS enum value. Both functions now return an explicit error on unrecognized input so the purchase and offering-lookup paths fail loud instead of buying the wrong product. Mirrors the pattern already used by the RDS convertPaymentOption. Regression tests for H1 (unknown/empty payment option) and H2 (unknown/empty term) are added to TestClient_FindOfferingID_AllPaymentOptions and TestClient_FindOfferingID_TermVariations; the previously-passing "default term" case is updated to assert the error path. Fixes H1 and H2 from docs/code-review/19-hardcoded-fallbacks-aws.md (PR #1039).

cristim · 2026-06-07T18:14:31Z

Scope expanded: H1 + H2 silent-default fixes (commit `bcc1f96`)

Added fixes for the two High-severity findings from the docs/code-review/19-hardcoded-fallbacks-aws.md audit assigned to this PR.

H1: convertPaymentOption now returns (SavingsPlanPaymentOption, error) and errors on unknown/empty input instead of silently defaulting to All Upfront. Error threaded through findOfferingID.

H2: convertTermToSeconds now returns (int64, error) and errors on unknown/empty term instead of silently defaulting to 1 year. Error threaded through findOfferingID.

Regression tests in TestClient_FindOfferingID_AllPaymentOptions and TestClient_FindOfferingID_TermVariations assert the error path for unknown/empty values and confirm the AWS API is never called.

go test -race ./services/savingsplans/...: 66 passed.

@coderabbitai review

coderabbitai · 2026-06-07T18:15:39Z

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd -e go -p 'savingsplans/client' | xargs cat

Length of output: 94046

@cristim The code fetched in my analysis snapshot still shows the pre-fix convertPaymentOption and convertTermToSeconds signatures (single return value, silent defaults) — a stale-snapshot artifact consistent with the bcc1f96 commit landing after the snapshot was taken. CodeRabbit will pick up the new commit incrementally.

While the review runs, here's what I'll be verifying in the diff:

H1 — convertPaymentOption

Signature is now (types.SavingsPlanPaymentOption, error)
Empty string input returns an error (not AllUpfront)
Unknown token returns an error (not a WARN+default)
Error propagated through findOfferingID (both return paths checked)

H2 — convertTermToSeconds

Signature is now (int64, error)
Empty string input returns an error (not 1yr default)
Unknown token returns an error (not a WARN+default)
Error propagated through findOfferingID

Tests (TestClient_FindOfferingID_AllPaymentOptions / TestClient_FindOfferingID_TermVariations)

Both contain a case for unknown/empty input that expects an error
Both assert DescribeSavingsPlansOfferings is not called on the error path (client-side guard fires first)

One pre-emptive flag: if either function accepts "" as a valid token (e.g. matching the Azure toAzureTerm pattern where empty defaults to 1yr), that's a deliberate choice — but given the audit finding classifies it as High, I'd expect explicit rejection of empty for the purchase path. Happy to confirm once the diff is visible.

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:45:38.040Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/cloudsql/client.go`, `getSQLPricing` had the same per-hour/term-total unit mismatch as memorystore and cloudstorage: `commitmentPrice` (per-hour SKU rate) was passed directly to savings math that expected a term total, producing ~99.99% savings, and `HourlyRate` was set to `per-hour / hoursInTerm` (near-zero). Fixed in PR `#1047` commit 20590c6b4 (issue `#1078` folded in): `commitmentPriceTerm := commitmentPrice * hoursInTerm`; `HourlyRate = commitmentPrice` (raw per-hour). `convertGCPRecommendation` also hardcoded `rec.Term = "1yr"`, ignoring `params.Term`; fixed with same defaulting pattern as memorystore/cloudstorage. Regression tests: `TestGetSQLPricing_CommitmentPriceIsTermTotal` and `TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams`. PaymentOption was already correct (`if paymentOption == "" { paymentOption = "monthly" }`).

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.421Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` no longer hardcodes `Term = "1yr"` (fixed in PR `#1047` commit 8f9a787, H-3 audit finding). It propagates `params.Term` with a `"1yr"` default, validates via `termPlan`, and returns nil on unknown term. This is stricter than memorystore/cloudstorage/cloudsql (which default and continue): computeengine returns nil on unknown term because it is on the purchase path. Regression tests: `TestConvertGCPRecommendation_PropagatesParamsTerm`, `TestConvertGCPRecommendation_RejectsUnknownTerm`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.416Z
Learning: In LeanerCloud/CUDly GCP service converters (`convertGCPRecommendation` in memorystore/client.go and cloudstorage/client.go), `rec.Term` must be derived from `params.Term` with a `"1yr"` default — not hardcoded to `"1yr"`. Without this, 3-year callers always emit 1-year commitments even though the downstream `termYears` derivation from `rec.Term` is correct. Fixed in PR `#1047` commit c6280c390 (F2 for memorystore, F4 for cloudstorage). Regression tests: `TestConvertGCPRecommendation_PropagatesTermFromParams` and `TestCloudStorageConvertGCPRecommendation_PropagatesTermFromParams`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.416Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` must force `paymentOption = "monthly"` unconditionally (logging a WARN when a non-monthly value such as "upfront", "all-upfront", or "partial-upfront" is supplied) because GCP CUDs are monthly-only and any non-monthly value passed through would be a silent misconfiguration. Fixed in PR `#1047` commit c6280c390 (F1). Regression test: `TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 277
File: frontend/src/__tests__/recommendations.test.ts:2472-2476
Timestamp: 2026-05-05T07:46:01.902Z
Learning: In the CUDly frontend (`frontend/src/recommendations.ts`), `effectiveSavingsPct` intentionally has NO plausibility guard (no null return or warning for reconstructed percentages above per-term ceilings). The maintainer explicitly rejected that approach in PR `#277`. The fix for inflated percentages (issue `#274`) is purely a data-path fix: plumbing the provider's canonical `on_demand_cost` through to the frontend so the denominator is not reconstructed from potentially misleading `monthly_cost + savings + amortized`. Tests that pin the reconstructed >80% behavior are intentional documentation of the pre-fix path, not bugs.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.421Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `termPlan` returns `(string, error)` using `computepb.Commitment_TWELVE_MONTH.String()` and `computepb.Commitment_THIRTY_SIX_MONTH.String()` as the canonical plan strings (fixed in PR `#1047` commit 8f9a787). It accepts 1yr/1/12mo and 3yr/3/36mo; it errors on empty or unrecognized input (no silent 12-month default). Regression tests: `TestTermPlan_UsesSdkEnumConstants`, `TestTermPlan_RejectsUnknownTerm`, `TestTermPlan_AcceptsAllDocumentedForms`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-25T21:45:57.471Z
Learning: In this repository (CUDly), GCP CUD commitments are inherently monthly-billed. The GCP CUD purchase API (`providers/gcp/services/computeengine/client.go:350-373`, `buildCommitmentRequests`) takes only a `Plan` field (TWELVE_MONTH / THIRTY_SIX_MONTH) and never reads a payment-option field. Therefore, the only valid payment option for GCP in `ValidPaymentOptionsByProvider` is `"monthly"`. The `NormalizePaymentOption` GCP branch should collapse any non-monthly token (including legacy `"upfront"`) to `"monthly"`, with a WARN log at the `scheduler.convertRecommendations` boundary.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-25T21:45:57.471Z
Learning: In this repository (CUDly), `internal/commitmentopts/normalizePayment` is unexported and AWS-spelling-focused (parses "All Upfront"/"ALL_UPFRONT" → canonical AWS tokens). `internal/api/validation.go:purchasePaymentWhitelist` is the purchase-execute boundary. `pkg/common/reservation_name.go:normalizeReservationPayment` produces short-form name segments. These are all distinct concerns from the cross-provider `NormalizePaymentOption` helper in `internal/config/validation.go`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 339
File: providers/aws/recommendations/coverage.go:177-205
Timestamp: 2026-05-13T21:30:43.093Z
Learning: In the CUDly project (LeanerCloud/CUDly), the CLI flag for target-based coverage sizing is `--target-coverage` (not `--target-utilization`). The PR renamed it during design iteration: "utilization" (how full each RI is) stays ~100% under the new sizing, while "coverage" (% of demand covered) is the user-facing knob. `ce:GetReservationCoverage` is required only when `--target-coverage` is set, gated by `cfg.TargetCoverage <= 0` in `cmd/multi_service.go`. The legacy `--coverage` path is unaffected and pays no CE cost or IAM change.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-25T21:45:57.471Z
Learning: In this repository (CUDly), Azure service clients alias payment options in pairs: `{all-upfront, upfront}` and `{monthly, no-upfront}`. All 7 Azure service-client switches (compute, cache, cosmosdb, database, search, synapse, managedredis client.go) use these aliases. The canonical Azure payment option set in `ValidPaymentOptionsByProvider` is `{upfront, monthly}` (with synonyms `all-upfront → upfront`, `no-upfront → monthly`, `partial-upfront → upfront`).

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…ilent defaults (#1085) * fix(aws/savingsplans): error on unknown payment-option/term, no silent default convertPaymentOption and convertTermToSeconds previously logged a warning and silently substituted All Upfront (largest cash outlay) or 1 year when given an unrecognized or empty value, risking a mis-purchase on any typo or new/renamed AWS enum value. Both functions now return an explicit error on unrecognized input so the purchase and offering-lookup paths fail loud instead of buying the wrong product. Mirrors the pattern already used by the RDS convertPaymentOption. Regression tests for H1 (unknown/empty payment option) and H2 (unknown/empty term) are added to TestClient_FindOfferingID_AllPaymentOptions and TestClient_FindOfferingID_TermVariations; the previously-passing "default term" case is updated to assert the error path. Fixes H1 and H2 from docs/code-review/19-hardcoded-fallbacks-aws.md (PR #1039). * fix(aws): fail loud on unknown payment-option/term/engine Fixes H3, H4, M1-M6, L1, L2 from the 19-hardcoded-fallbacks-aws.md audit plus H1/H2 from the now-merged #1039 branch (savingsplans). H3 (converters.go): add convertPaymentOptionE/convertTermInYearsE/ convertLookbackPeriodE that error on unrecognized input; wire into the SP recommendation path (parser_sp.go). The legacy non-erroring wrappers are retained for the RI path in client.go (owned by #865/#1075) with a deprecation comment and a follow-up reference. H4 (elasticache/client.go): convertPaymentOption now returns (string, error) mirroring the RDS sibling that already errors correctly. H1/H2 (savingsplans/client.go): convertPaymentOption and convertTermToSeconds now return (type, error); cherry-picked from the merged #1039 branch where the fix was staged. M4 (parser_services.go, rds/client.go): parseRDSDetails no longer silently defaults AZConfig to "single-az" when CE omits DeploymentOption; findOfferingID rejects empty AZConfig with an explicit error. M5 (parser_services.go): resolveEC2Tenancy now maps only the known CE values ("shared"/nil -> default, "dedicated" -> dedicated); any other value (e.g. "host" for Dedicated Hosts) returns an error rather than silently collapsing to default tenancy. M6 (rds/client.go): normalizeEngineName now returns (string, error) and errors for ambiguous Oracle ("oracle*"), SQL Server ("sqlserver*"/"sql-server*"), and bare Aurora inputs; unambiguous engines (mysql, postgresql, mariadb, aurora-mysql, aurora-postgresql) pass through unchanged. M1/M2/M3 (ec2/client.go): replace raw "convertible" literals with types.OfferingClassTypeConvertible; buildEC2OfferingQuery errors on empty Platform (purchase path); centralize "Linux/UNIX" literal into defaultEC2Platform constant (exchange-path helpers retain their safe fallback; the purchase path does not). Regression tests added for H3, H1/H2, H4, M2, M4, M5, M6, L1, L2; each fails pre-fix and passes post-fix. Closes #1084 * test(aws/rds): clarify ambiguous-engine regression test Simplify TestNormalizeEngineName_AmbiguousErrors: remove the confusing oracle-se2 case (strings.Contains("oracle-se2","oracle") is true, so it errors like all other oracle inputs -- no special case needed) and remove the misleading inline comment. All remaining cases assert that any Oracle, SQL Server, or bare Aurora input errors, which is the invariant under test. * refactor(aws): extract helpers to fix gocyclo violations in rds and savingsplans gocyclo -over 10 fails on two functions added by the fail-loud refactor: - savingsplans.(*Client).findOfferingID (complexity 12): extract resolveSPPlanType and buildSPOfferingsInput helpers - rds.(*Client).paginateRDSOfferings (complexity 11): extract fetchRDSOfferingPage to isolate the per-page API call and scan Also apply gofmt to rds/client.go (trailing whitespace / brace style). Behaviour and error paths are unchanged; all rds and savingsplans tests still pass. * fix(aws/rds): tighten AZ-config and engine-name validation (CR #1085) Three related fail-loud fixes in the AWS RDS purchase path: 1. parseRDSDetails: unknown DeploymentOption now errors instead of silently folding into "single-az". Only "Multi-AZ" and "Single-AZ" are accepted; any other non-nil value (e.g. "Multi-AZ-Readable-Standbys") returns an explicit error so the bad token surfaces rather than driving findOfferingID to the wrong RI class. 2. findOfferingID: AZConfig validation now uses a switch with an explicit default error case rather than only checking for empty string. A non-empty but invalid value (typo, future AWS enum addition) would previously fall through to multiAZ==false in paginateRDSOfferings, silently treating the bad input as single-AZ. 3. normalizeEngineName: Oracle and SQL Server checks changed from strings.Contains to == so that edition-qualified tokens (oracle-se2, oracle-ee, sqlserver-se, sqlserver-web, etc.) pass through as valid RDS ProductDescription values. The previous Contains check contradicted the error message itself, which told callers to supply those exact edition tokens. Regression tests added for all three: each new test was verified to fail against the pre-fix code and pass after.

cristim added bug Something isn't working triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/now Drop other things impact/all-users Affects every user effort/s Hours type/bug Defect labels Jun 7, 2026

cristim mentioned this pull request Jun 7, 2026

refactor(arch): provider-module fragmentation forces copy-paste (fan-out, env reader, savings formula); + pkg/errors orphan, logging split #1035

Closed

11 tasks

cristim mentioned this pull request Jun 7, 2026

fix(aws/savingsplans): add USD currency + region filter and deterministic sort to findOfferingID #1066

Closed

cristim merged commit b7012e7 into feat/multicloud-web-frontend Jun 7, 2026
4 checks passed

Navigation Menu

Repository navigation

fix(aws/savingsplans): paginate GetExistingCommitments via NextToken loop - #1039

fix(aws/savingsplans): paginate GetExistingCommitments via NextToken loop#1039
cristim merged 3 commits into
feat/multicloud-web-frontendfrom
fix/aws-sp-pagination

cristim commented Jun 7, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Uh oh!

cristim commented Jun 7, 2026

Uh oh!

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Uh oh!

cristim commented Jun 7, 2026

Uh oh!

cristim commented Jun 7, 2026

Uh oh!

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Uh oh!

Uh oh!

cristim commented Jun 7, 2026

Uh oh!

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

cristim commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Scope expanded

Uh oh!

coderabbitai Bot commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Uh oh!

cristim commented Jun 7, 2026

Uh oh!

coderabbitai Bot commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 7, 2026

Uh oh!

cristim commented Jun 7, 2026

FOLD-1039: added finding 08-L1 (SP portion)

Uh oh!

coderabbitai Bot commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cristim commented Jun 7, 2026

Scope expanded: H1 + H2 silent-default fixes (commit bcc1f96)

Uh oh!

coderabbitai Bot commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristim commented Jun 7, 2026 •

edited

Loading

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Scope expanded: H1 + H2 silent-default fixes (commit `bcc1f96`)

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading