feat(commitmentopts): use AWS SP probe in Validate path (closes #134) by cristim · Pull Request #825 · LeanerCloud/CUDly

cristim · 2026-05-28T18:55:55Z

Summary

Removes stale comment in commitmentopts.Validate that described Savings Plans as "not commitment-capable in our probe set" -- PR feat(commitmentopts): probe Savings Plans offerings live (closes #108) #569 added SavingsPlansProber to DefaultProbers, making that comment incorrect.
Adds four Validate tests covering SP service keys (savings-plans-compute, -ec2instance, -sagemaker, -database) to close the coverage gap that the chunk-0 bucket-A verification flagged.

Precedence chain

probe snapshot present for service key -> reject invalid combos
service key absent from snapshot -> permissive fallback (plan type not offered in region)
ErrNoData (cold store / no AWS account) -> permissive fallback (never block saves)

What PR #569 left incomplete vs claimed

The probe code and DefaultProbers wiring were complete. The gap was:

A stale comment in Validate still implied SP was unprobed.
No tests exercised the Validate path with the real SP service keys, so the probe-first behavior was unverified.

The Validate logic itself was already correct post #569 -- Get routes through probeAndPersist which fans out all DefaultProbers including SavingsPlansProber. This PR closes the documentation and test coverage gap.

Test plan

go test github.com/LeanerCloud/CUDly/internal/commitmentopts/... -- 83 pass (79 pre-existing + 4 new)
go vet ./internal/commitmentopts/... -- clean

Summary by CodeRabbit

Bug Fixes
- Enhanced error handling to properly handle wrapped errors during validation, improving system resilience.
Tests
- Expanded test coverage for savings plans validation behavior and edge cases.

coderabbitai · 2026-05-28T18:56:02Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 54 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3863e859-065f-451a-8b60-ec836e37c233

📥 Commits

Reviewing files that changed from the base of the PR and between 229b4fa and 61e2fec.

📒 Files selected for processing (2)

internal/commitmentopts/service.go
internal/commitmentopts/service_test.go

📝 Walkthrough

Walkthrough

Service.Validate in internal/commitmentopts/service.go switches from a direct equality check to errors.Is(err, ErrNoData) for detecting wrapped ErrNoData, with an accompanying errors import and refreshed comments. Four new test functions in service_test.go cover the savings-plan probe-hit, key-missing, cold-start, and end-to-end probe-precedence scenarios.

Changes

Service.Validate ErrNoData fix and savings-plan test coverage

Layer / File(s)	Summary
`errors.Is` fix and comment refresh in `Service.Validate` `internal/commitmentopts/service.go`	Adds `errors` import, changes `err == ErrNoData` to `errors.Is(err, ErrNoData)` so wrapped errors are recognized as "no probe data", and refreshes the comments on the permissive fallback block.
Savings-plan probe validation tests `internal/commitmentopts/service_test.go`	Updates `TestService_Validate_UnknownServiceUnderKnownProviderPermissive` for the savings-plans key, and adds `TestService_Validate_SavingsPlansProbeHit`, `TestService_Validate_SavingsPlansKeyMissingInSnapshot`, `TestService_Validate_SavingsPlansErrNoDataFallback`, and `TestService_Validate_SavingsPlansProbeConsulted`; also includes minor test double field reorderings in `fakeStore`, `fakeAccounts`, `stubProber`, and `proberFunc`.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

LeanerCloud/CUDly#240: Directly coupled at the Service.Validate / ErrNoData handling layer — the saveAccountServiceOverride guard that calls checkCommitmentOptionCombo depends on the same permissive-validation behavior being fixed here.

Suggested labels

urgency/this-quarter, effort/s

Poem

🐇 Hoppity-hop through the error chain,
errors.Is wraps the pain away.
No data? No worries — be permissive, they say!
Savings plans probed, the snapshot consulted with care,
A cold start stays true — the bunny finds that fair. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: integrating AWS SP (Savings Plans) probe into the Validate path, closing issue `#134`. It is concise, specific, and directly corresponds to the primary objective of the PR.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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/134-wave9

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

cristim · 2026-05-28T18:56:08Z

@coderabbitai review

coderabbitai · 2026-05-28T18:56:17Z

✅ Actions performed

Review triggered.

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.

cristim · 2026-05-30T10:22:20Z

@coderabbitai full review

coderabbitai · 2026-05-30T10:22:27Z

✅ Actions performed

Full review triggered.

cristim · 2026-06-04T14:04:05Z

@coderabbitai review

coderabbitai · 2026-06-04T14:04:14Z

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 39 minutes and 21 seconds before sending another message.

cristim · 2026-06-08T00:39:10Z

@coderabbitai full review

Generated by Claude Code

coderabbitai · 2026-06-08T00:40:20Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details
gh pr view 825 --repo LeanerCloud/CUDly

echo "=== PR DIFF ==="
gh pr diff 825 --repo LeanerCloud/CUDly

Length of output: 8604

@cristim: Full review complete.

`internal/commitmentopts/service.go`

The comment replacement is accurate and well-scoped. The old text ("not commitment-capable in our probe set (e.g. Savings Plans)") was factually wrong after PR #569; the new wording correctly describes the permissive fallback as a missing-key semantic (plan type absent from region), not a probe-set exclusion. No logic is touched.

`internal/commitmentopts/service_test.go`

All four new tests look correct.

Test	Path covered	Assessment
`TestService_Validate_SavingsPlansProbeHit`	Probe snapshot present for key → probe data governs	✅ Correctly seeds snapshot, validates present combo, rejects absent combo
`TestService_Validate_SavingsPlansKeyMissingInSnapshot`	Key absent from snapshot (other keys present) → permissive	✅ Correct — distinguishes "probe ran but returned no combos for this key" from ErrNoData
`TestService_Validate_SavingsPlansErrNoDataFallback`	Cold store + nil accounts → ErrNoData → permissive	✅ `fakeStore{}` (no `has:true`) + `fakeAccounts{accounts:nil}` is consistent with the rest of the test suite
`TestService_Validate_SavingsPlansProbeConsulted`	End-to-end: stubProber → Get populates cache → Validate uses probe result	✅ The `require.Contains(t, opts["aws"], "savings-plans-compute")` guard will fail cleanly (not panic) if the prober result isn't persisted under the expected key

The three-tier precedence chain (probe data → absent key → ErrNoData) is now fully exercised. The comment update on TestService_Validate_UnknownServiceUnderKnownProviderPermissive is a nice cleanup too.

No issues found. The PR is a focused, low-risk cleanup: one comment fix backed by four targeted regression tests.

[approve]

🧠 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-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/services/rds/client.go`, `findOfferingID` validates AZConfig with a switch that has a `default: error` case covering non-empty invalid values (e.g. "typo-az"), not just an empty-string check. A bad non-empty AZConfig previously fell through to `multiAZ==false` in `paginateRDSOfferings` (same mis-buy as the old default). Fixed in PR `#1085` commit 681fd9e81. Regression test added.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:45:38.069Z
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-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: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.445Z
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-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/recommendations/parser_services.go`, `parseRDSDetails` uses an exhaustive switch on `DeploymentOption`: "Multi-AZ" maps to "multi-az", "Single-AZ" maps to "single-az", nil leaves AZConfig unset, and any other non-nil value returns an error. Silent defaulting to "single-az" for unknown values was removed in PR `#1085` commit 681fd9e81. Regression test added.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly `providers/gcp/recommendations.go`, `collectRegion` fans out to all four GCP service clients (computeengine, cloudsql, memorystore, cloudstorage) concurrently (fixed in PR `#1047` commit 95a390f, H-2 finding). `regionResult` gains `cache` and `storage` slices; merge is updated accordingly. The decision was to wire (not trim) memorystore and cloudstorage because both have complete `GetRecommendations` implementations and their no-op `PurchaseCommitment` paths are orthogonal to surfacing advisory recommendations. Regression tests: `TestRegionResult_HasCacheAndStorageFields` and `TestShouldIncludeService_Cache_Storage`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
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-06-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/services/rds/client.go`, `normalizeEngineName` uses `==` (not `strings.Contains`) for oracle and sqlserver checks. Edition-qualified tokens (oracle-se2, oracle-ee, sqlserver-se, sqlserver-web, sqlserver-ex, etc.) must pass through as valid RDS ProductDescription values. Using Contains was self-contradictory: the error message told callers to supply the exact edition tokens. Fixed in PR `#1085` commit 681fd9e81. `TestNormalizeEngineName_EditionTokensPassThrough` covers 10 edition variants.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
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-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly GCP service clients (computeengine, cloudsql, memorystore, cloudstorage), `GetRecommendations` filters out non-ACTIVE recommendations using the typed `recommenderpb.RecommendationStateInfo_ACTIVE` enum (CLAIMED/SUCCEEDED/FAILED/DISMISSED are skipped). State is accessed nil-safely via the proto getter. Fixed in PR `#1047` commit 95a390f (H-1 finding). Regression tests: `TestGetRecommendations_FiltersNonActiveStates` (5-rec mock, only ACTIVE survives) and `TestGetRecommendations_ActiveRecIncluded`. Existing mock tests in all four services had StateInfo added to satisfy the new filter.

cristim · 2026-06-08T04:28:26Z

Rebased onto feat/multicloud-web-frontend and resolved the merge conflict in internal/commitmentopts/service.go.

Conflict resolution (Validate absent-service-key branch): the base branch had independently rewritten the same comment and added a logging.Warnf on the permissive fallback path; this PR had only rewritten the comment. Resolved by preserving both intents: kept the base branch's operator-visible Warnf (so probe gaps / misconfigured service names are observable rather than silently permissive) and merged both comment variants into one. No semantic change to the return true, nil behaviour on either side.

This PR's substantive change is unaffected: the four Savings Plans probe-path tests survive the rebase intact (TestService_Validate_SavingsPlansProbeHit, ...SavingsPlansKeyMissingInSnapshot, ...SavingsPlansErrNoDataFallback, ...SavingsPlansProbeConsulted).

Verification: go build ./... clean; go vet ./internal/commitmentopts/... clean; go test ./internal/commitmentopts/... 83 passed; gofmt clean on both touched files.

No prior CodeRabbit review completed on this PR (every earlier trigger hit the org review-rate/credit limit), and there are zero unresolved review threads. Requesting a full review.

@coderabbitai full review

coderabbitai · 2026-06-08T04:29:13Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr view 825 --repo LeanerCloud/CUDly

echo "=== PR DIFF ==="
gh pr diff 825 --repo LeanerCloud/CUDly

Length of output: 9128

@cristim: Post-rebase review complete.

`internal/commitmentopts/service.go`

The merged comment is an improvement over both ancestors: it correctly articulates the "probe ran but returned no combos for this key" semantics (distinct from ErrNoData), preserves the operator-visible Warnf from the base branch, and keeps the return true, nil path unchanged. The resolution is semantically faithful to both branches.

`internal/commitmentopts/service_test.go`

Test	Tier exercised	Assessment
`TestService_Validate_SavingsPlansProbeHit`	Probe data present → governs	✅ Valid combo accepted, absent combo rejected
`TestService_Validate_SavingsPlansKeyMissingInSnapshot`	Probe ran, key absent → permissive	✅ Correctly distinguishes "no key" from "ErrNoData"
`TestService_Validate_SavingsPlansErrNoDataFallback`	Cold store / nil accounts → ErrNoData → permissive	✅ Loops over all four SP service keys
`TestService_Validate_SavingsPlansProbeConsulted`	End-to-end: stubProber → Get → Validate uses probe result	✅ `require.Contains` guard fails cleanly before Validate if prober result isn't stored under expected key

The updated comment on TestService_Validate_UnknownServiceUnderKnownProviderPermissive ("A service key absent from the probe snapshot must not block saves") is more precise than the old "savingsplans has no probe — don't block" — accurate cleanup.

No issues found. The PR is a well-scoped documentation fix + test coverage close, and the rebase resolution is correct.

[approve]

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/services/rds/client.go`, `findOfferingID` validates AZConfig with a switch that has a `default: error` case covering non-empty invalid values (e.g. "typo-az"), not just an empty-string check. A bad non-empty AZConfig previously fell through to `multiAZ==false` in `paginateRDSOfferings` (same mis-buy as the old default). Fixed in PR `#1085` commit 681fd9e81. Regression test added.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly `providers/gcp/recommendations.go`, `collectRegion` fans out to all four GCP service clients (computeengine, cloudsql, memorystore, cloudstorage) concurrently (fixed in PR `#1047` commit 95a390f, H-2 finding). `regionResult` gains `cache` and `storage` slices; merge is updated accordingly. The decision was to wire (not trim) memorystore and cloudstorage because both have complete `GetRecommendations` implementations and their no-op `PurchaseCommitment` paths are orthogonal to surfacing advisory recommendations. Regression tests: `TestRegionResult_HasCacheAndStorageFields` and `TestShouldIncludeService_Cache_Storage`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
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.433Z
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-06-07T17:45:38.069Z
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-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-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly GCP service clients (computeengine, cloudsql, memorystore, cloudstorage), `GetRecommendations` filters out non-ACTIVE recommendations using the typed `recommenderpb.RecommendationStateInfo_ACTIVE` enum (CLAIMED/SUCCEEDED/FAILED/DISMISSED are skipped). State is accessed nil-safely via the proto getter. Fixed in PR `#1047` commit 95a390f (H-1 finding). Regression tests: `TestGetRecommendations_FiltersNonActiveStates` (5-rec mock, only ACTIVE survives) and `TestGetRecommendations_ActiveRecIncluded`. Existing mock tests in all four services had StateInfo added to satisfy the new filter.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.445Z
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-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/recommendations/parser_services.go`, `parseRDSDetails` uses an exhaustive switch on `DeploymentOption`: "Multi-AZ" maps to "multi-az", "Single-AZ" maps to "single-az", nil leaves AZConfig unset, and any other non-nil value returns an error. Silent defaulting to "single-az" for unknown values was removed in PR `#1085` commit 681fd9e81. Regression test added.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-08T04:21:35.802Z
Learning: In LeanerCloud/CUDly `frontend/src/recommendations.ts` (PR `#838`, commit d7c5764), `perRecPayments` in `FanOutBucket` must be seeded **sparsely** (override-only: only recs whose resolved payment differs from the bucket-level default). The execute path in `frontend/src/app.ts` resolves each rec's payment as `perRecPayments.get(rec.id) ?? b.payment`; if the map is fully populated the fallback is dead and changing the bucket-level dropdown has no effect on unedited rows. The per-rec change handler deletes the map entry when the user reverts to the bucket default (resuming tracking via the fallback); the bucket-level change handler re-syncs visible per-rec selects that have no override, skipping rows whose service does not support the new payment (mixed-SP buckets). Tests (i) and (j) cover the override-only contract and bucket-level propagation respectively; 322 tests pass, `tsc --noEmit` clean.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.445Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, the `memMBPerVCPU = 4096` const was removed in PR `#1047` commit 8f9a787. MEMORY MB is now extracted directly from the Recommender payload via `memoryMBFromOperationGroups` (sibling of `vcpuCountFromOperationGroups`). `buildInsertRequest` and `GroupCommitments` call `memoryMBFromDetails(rec)` which returns an error (no silent fallback) when `Details.MemoryGB` is absent. This ensures high-memory families (e.g. N2-highmem) get the correct MB from the payload rather than the GENERAL_PURPOSE approximation. Regression test: `TestBuildInsertRequest_RefusesMissingMemory`.

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: 0
File: :0-0
Timestamp: 2026-06-07T19:32:26.182Z
Learning: In LeanerCloud/CUDly `providers/aws/services/rds/client.go`, `normalizeEngineName` uses `==` (not `strings.Contains`) for oracle and sqlserver checks. Edition-qualified tokens (oracle-se2, oracle-ee, sqlserver-se, sqlserver-web, sqlserver-ex, etc.) must pass through as valid RDS ProductDescription values. Using Contains was self-contradictory: the error message told callers to supply the exact edition tokens. Fixed in PR `#1085` commit 681fd9e81. `TestNormalizeEngineName_EditionTokensPassThrough` covers 10 edition variants.

cristim · 2026-06-08T06:18:25Z

@coderabbitai full review

coderabbitai · 2026-06-08T06:18:56Z

✅ Action performed

Full review finished.

cristim · 2026-06-19T21:08:42Z

@coderabbitai review

coderabbitai · 2026-06-19T21:08:50Z

✅ 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.

cristim · 2026-07-09T21:20:58Z

@coderabbitai review

coderabbitai · 2026-07-09T21:21:04Z

✅ 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.

cristim · 2026-07-10T14:56:08Z

@coderabbitai review

coderabbitai · 2026-07-10T14:56:14Z

✅ 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.

Fix stale comment in Validate that still listed Savings Plans as "not in the probe set" after PR #569 added SavingsPlansProber to DefaultProbers. Add four Validate tests covering the SP probe path: - SavingsPlansProbeHit: probe snapshot governs hit/miss for SP keys - SavingsPlansKeyMissingInSnapshot: absent key stays permissive - SavingsPlansErrNoDataFallback: cold-start never blocks saves - SavingsPlansProbeConsulted: end-to-end probe-first precedence Precedence: probe snapshot -> permissive fallback (ErrNoData or absent service key). The Validate logic was already correct post #569; this commit closes the test coverage gap and removes the misleading comment.

…vice branch Keep the base operator-visible logging.Warnf (05-M4) and replace the generic probe comment with the SP-specific wording from fix/134-wave9: "a plan type not sold in the account's region".

cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/many Affects most users effort/m Days type/feat New capability labels May 28, 2026

cristim force-pushed the fix/134-wave9 branch from 966a19e to 62014ea Compare June 8, 2026 04:27

cristim changed the base branch from feat/multicloud-web-frontend to main June 9, 2026 15:45

cristim force-pushed the fix/134-wave9 branch from 9248276 to 229b4fa Compare June 19, 2026 21:08

cristim force-pushed the fix/134-wave9 branch from 229b4fa to 94f0a87 Compare July 10, 2026 14:55

cristim added 2 commits July 17, 2026 21:30

fix(commitmentopts): adopt SP-specific comment in Validate absent-ser…

61e2fec

…vice branch Keep the base operator-visible logging.Warnf (05-M4) and replace the generic probe comment with the SP-specific wording from fix/134-wave9: "a plan type not sold in the account's region".

cristim force-pushed the fix/134-wave9 branch from 94f0a87 to 61e2fec Compare July 17, 2026 18:31

cristim merged commit 0ef84f3 into main Jul 19, 2026
19 checks passed

cristim deleted the fix/134-wave9 branch July 27, 2026 11:09

Uh oh!

Conversation

cristim commented May 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Precedence chain

What PR #569 left incomplete vs claimed

Test plan

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Review limit reached

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Poem

❌ Failed checks (1 warning)

Uh oh!

cristim commented May 28, 2026

Uh oh!

coderabbitai Bot commented May 28, 2026

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented Jun 4, 2026

Uh oh!

coderabbitai Bot commented Jun 4, 2026

Rate Limit Exceeded

Uh oh!

cristim commented Jun 8, 2026

Uh oh!

coderabbitai Bot commented Jun 8, 2026

internal/commitmentopts/service.go

internal/commitmentopts/service_test.go

Uh oh!

cristim commented Jun 8, 2026

Uh oh!

coderabbitai Bot commented Jun 8, 2026

internal/commitmentopts/service.go

internal/commitmentopts/service_test.go

Uh oh!

cristim commented Jun 8, 2026

Uh oh!

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

Uh oh!

Uh oh!

cristim commented Jun 19, 2026

Uh oh!

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

Uh oh!

Uh oh!

cristim commented Jul 9, 2026

Uh oh!

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

Uh oh!

Uh oh!

cristim commented Jul 10, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristim commented May 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 28, 2026 •

edited

Loading

`internal/commitmentopts/service.go`

`internal/commitmentopts/service_test.go`

`internal/commitmentopts/service.go`

`internal/commitmentopts/service_test.go`

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading

coderabbitai Bot commented Jun 19, 2026 •

edited

Loading

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading