test(aws/recs): cover SizeInMiB-nil + assert no NextPage on canceled ctx; drop em-dashes

[cristim]

Summary

The ComputeDetails VCPU/MemoryGB implementation (issue #99) landed via the merged duplicate PR #816
(feat(aws/recs): populate ComputeDetails.VCPU/MemoryGB via DescribeInstanceTypes, closes #218).

This PR is repurposed to carry the remaining test-coverage gaps that #816 missed, plus a comment
cleanup -- nothing net-new in production code.

What is grafted on top of the already-merged #816 implementation:

1. TestExtractInstanceTypeSKUEntry: new sub-case "MemoryInfo present but SizeInMiB nil" -- verifies
   VCPU is populated (8) while MemGB stays 0.0 when the API returns a MemoryInfo struct but omits
   the SizeInMiB pointer.

2. TestFetchInstanceTypeCatalogue_ContextCanceled: after the pre-canceled ctx returns nil, also
   assert pager.callCount == 0 -- the stub already tracks this atomically; the assertion makes
   the no-NextPage-on-canceled-ctx contract explicit and regression-tested.

3. Em-dash (U+2014) cleanup: replaced -- in two Warnf messages in sku.go and in the
   instanceTypeLookup comment in client.go. Em-dashes in parser_services.go are left for a
   follow-up (out of scope here).

What was intentionally dropped from the original #842 branch (cosmetic churn not worth the diff
noise on top of the already-clean #816 implementation): assert.InDelta -> assert.Equal swaps,
require message removals, struct-literal refactor of extractInstanceTypeSKUEntry, pager
renaming.

Closes #99

Summary by CodeRabbit

• Refactor
  • Updated recommendation retrieval APIs across providers and internal schedulers to use pointer-based request parameters for consistency.
  • Improved warning-level visibility for per-service recommendation fetch failures during parallel aggregation (especially in AWS).
• Tests
  • Updated mocks, method signatures, and assertions to match pointer-based parameter passing.
  • Relaxed overly strict argument matching in unit tests to focus on returned recommendation behavior rather than exact parameter struct construction.

[coderabbitai]

📝 Walkthrough

Walkthrough

GetRecommendations now uses pointer-typed common.RecommendationParams across shared interfaces, internal callers, provider adapters, and service implementations. AWS also updates its recommendations client construction, merge handling, and EC2 SKU lookup helpers.

Changes

Pointer-based recommendation parameter update

Layer / File(s)	Summary
Contracts and internal callers pkg/provider/interface.go, internal/scheduler/..., internal/purchase/..., cmd/multi_service_...	GetRecommendations interfaces switch to *common.RecommendationParams; internal callers pass pointers; mocks and cmd expectations are updated to match pointer-based arguments.
AWS recommendations core and SKU helpers providers/aws/recommendations/client.go, providers/aws/recommendations/sku.go, providers/aws/recommendations/*_test.go	AWS recommendations client construction uses *aws.Config; GetRecommendations and GetRecommendationsForService pass pointer params; merge logging emits per-service WARNs; SKU lookup/extraction uses pointer-based instance-type data and updated tests.
AWS adapter and service clients providers/aws/service_client.go, providers/aws/services/..., providers/aws/service_client_test.go	AWS adapter wiring and per-service GetRecommendations methods move to pointer params, and the corresponding tests pass pointers.
Azure adapter and service clients providers/azure/recommendations.go, providers/azure/services/..., providers/azure/recommendations_test.go	Azure adapter fan-out and all Azure service clients switch to pointer params, with tests updated to call the new signatures.
GCP adapter and service clients providers/gcp/recommendations.go, providers/gcp/services/..., providers/gcp/recommendations_test.go	GCP adapter pointer-handling adds nil validation and pointer fan-out, while CloudSQL, CloudStorage, ComputeEngine, and Memorystore clients and tests move to pointer params.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Possibly related PRs

• LeanerCloud/CUDly#123: Changes AWS Savings Plans recommendation dispatch logic in the same GetRecommendations area.
• LeanerCloud/CUDly#592: Introduces the Azure Savings Plans client path that this PR updates to the pointer-based parameter shape.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR only adds test coverage and cleanup; it does not implement AWS DescribeInstanceTypes enrichment to populate VCPU and MemoryGB as required by #218/#99.	Implement the EC2 instance-type catalogue/cache and wire it into the AWS parser, then add tests proving VCPU and MemoryGB are populated.
Out of Scope Changes check	⚠️ Warning	The diff includes broad pointer-signature and interface changes across AWS/Azure/GCP services that are unrelated to the stated AWS test/comment-only scope.	Remove the cross-provider API churn and keep only the three intended AWS test/comment edits.
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and matches the intended remaining change: AWS recommendation test coverage plus comment cleanup.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/99-wave13

---

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

[cristim]

@coderabbitai review

[coderabbitai]

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

Coordination note: PR #816 (closes #218) implements the same DescribeInstanceTypes VCPU/MemoryGB population path independently. The two PRs are effectively duplicates. Whichever lands first should be merged; the other should be closed (or rebased to a tightening-only delta). Both currently open; flagging so the merger doesn't accidentally land both.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[cristim]

@coderabbitai review

[coderabbitai]

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

@coderabbitai review

[coderabbitai]

Rate Limit Exceeded

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

[cristim]

pr-iterate: rebased on feat/multicloud-web-frontend (resolved conflict in providers/aws/recommendations/parser_services.go and parser_services_test.go - merged the base branch ResourceType guard with the PR context.Context threading). PR is otherwise progressing (mergeStateStatus will refresh). Re-invoke /pr-iterate 842 after the org CR billing is topped up to get a substantive CR pass.

[cristim]

Verification note: this PR (closes #99) and PR #816 (closes #218) both populate ComputeDetails VCPU/MemoryGB via EC2 DescribeInstanceTypes on the same base branch. They are duplicate solutions and will conflict. Recommend merging ONE and closing the other's issue as a duplicate. The code here is correct on its own merits. Leaving the pick for a human.

[cristim]

pr-iterate: rebased on feat/multicloud-web-frontend and resolved the merge conflict in providers/aws/recommendations/parser_services.go.

Resolution detail (semantic, not mechanical): the base branch had independently extracted the EC2 helpers this PR added, but under the fail-loud names resolveEC2Tenancy / resolveEC2Scope / enrichFromCatalogue. This PR's later gocyclo-fix commit re-extracted them under ec2Tenancy / ec2Scope / enrichEC2Compute, where ec2Tenancy reintroduced a silent fallback (unknown/"host" tenancy collapsed to "default"). I kept the base's fail-loud resolveEC2Tenancy and dropped the duplicate silent-fallback definitions, so:

• No silent fallback on tenancy: unknown or "host" tenancy errors out (M5 regression tests EC2 host tenancy errors / EC2 unknown tenancy errors in parser_services_test.go stay green).
• parseEC2Details is already at low cyclomatic complexity in base, so the gocyclo gate passes without the reverted commit. The gocyclo-fix commit collapsed to empty and was dropped.
• Also ran gofmt on sku_test.go (import ordering the original commit had perturbed).

Verification: go build ./... success, go vet ./providers/aws/recommendations/... clean, full package tests 326 passed. CI is green (pre-commit hooks incl. gocyclo/golangci-lint pass, sanity pass); mergeStateStatus is CLEAN.

Note (unchanged, for the human): this PR (#99) and PR #816 (#218) are duplicate solutions for the same ComputeDetails VCPU/MemoryGB enrichment. Merge one and close the other's issue as a duplicate.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[cristim]

@coderabbitai full review

[cristim]

Repurpose summary: This branch has been reset to origin/feat/multicloud-web-frontend and the only commit now carries the unique test-coverage grafts from the original #842 diff (that were missing from the merged duplicate #816):

1. TestExtractInstanceTypeSKUEntry: new sub-case for MemoryInfo present but SizeInMiB nil -- VCPU populated, MemGB 0.0.
2. TestFetchInstanceTypeCatalogue_ContextCanceled: added assert.Equal(t, int32(0), atomic.LoadInt32(&pager.callCount)) to assert NextPage is never called on a pre-canceled ctx.
3. Em-dash (U+2014) -> -- in two Warnf messages in sku.go and in the instanceTypeLookup comment in client.go.

Build: go build ./... OK. Tests: 326 passed. The cosmetic churn from the original #842 (InDelta->Equal swaps, require-message removals, struct refactor, pager rename) was intentionally dropped -- base's versions from #816 are fine.

@coderabbitai full review

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr view 842 --repo LeanerCloud/CUDly --json title,body,headRefName,baseRefName,state,mergeable,files
echo "---DIFF---"
gh pr diff 842 --repo LeanerCloud/CUDly

Length of output: 5310

---

---

@cristim: The repurposed diff is exactly what the description says — no production code changes, three tightly-scoped cleanups. Here is the full pass:

---

providers/aws/recommendations/client.go

Line 97 — em-dash → --. ✅ Correct. Go convention for doc comments is plain ASCII dashes; this is the right call.

---

providers/aws/recommendations/sku.go

Lines 73 & 77 — same em-dash → -- in both Warnf messages. ✅ Consistent with the comment cleanup and avoids multi-byte character confusion in log aggregators that process byte ranges.

---

providers/aws/recommendations/sku_test.go

New sub-case "MemoryInfo present but SizeInMiB nil -- MemGB zero"

• MemoryInfo: &ec2types.MemoryInfo{} correctly simulates the API returning the struct with the pointer field unset (nil). ✅
• wantVCPU: 8 / wantMemGB: 0.0 correctly asserts partial population — VCPU extracted from the non-nil VCpuInfo, MemGB gracefully zero from the nil SizeInMiB. ✅
• The test name uses -- — consistent with the em-dash cleanup theme. ✅

TestFetchInstanceTypeCatalogue_ContextCanceled callCount assertion

• assert.Equal(t, int32(0), atomic.LoadInt32(&pager.callCount)) — type is int32 on both sides, matches the atomic counter in the stub. ✅
• The assertion documents the exact contract: the ctx.Err() guard at the top of the loop fires before the first pager.NextPage call, so callCount stays 0. This is a meaningful regression pin. ✅

---

Summary

All three changes are correct, minimal, and serve distinct purposes (comment hygiene, nil-pointer edge-case coverage, contract documentation). No concerns. Approving.

[approve]

---

🧠 Learnings used

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-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-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: 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-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-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-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-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-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-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-06T06:43:53.792Z
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-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]

@coderabbitai review

[coderabbitai]

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

@coderabbitai review

[coderabbitai]

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

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@providers/gcp/recommendations.go`:
- Around line 97-98: Add a nil guard at the beginning of the GetRecommendations
method in RecommendationsClientAdapter before the line that dereferences the
pointer parameter p (where params := *p occurs). Check if p is nil and return an
appropriate error instead of proceeding with the dereference to prevent a
potential nil-pointer panic at runtime.

In `@providers/gcp/services/cloudsql/client.go`:
- Around line 142-143: The GetRecommendations method in CloudSQLClient
dereferences the pointer parameter p without first checking if it is nil, which
will cause a panic if a caller passes nil. Add a nil guard check at the
beginning of the GetRecommendations method that validates p is not nil, and
return an appropriate error (such as "recommendation params cannot be nil") if
the check fails before attempting to dereference p with params := *p.

In `@providers/gcp/services/cloudstorage/client.go`:
- Around line 162-163: The GetRecommendations method in CloudStorageClient
dereferences the pointer parameter p without first checking if it is nil, which
will cause a panic if nil is passed. Add a nil guard at the beginning of the
GetRecommendations method to check if p is nil, and return an appropriate error
value before attempting to dereference it with the params := *p statement.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bdfd6c52-bd98-4c41-9b00-87c02b7dabec

📥 Commits

Reviewing files that changed from the base of the PR and between 451a70f and 6082958.

📒 Files selected for processing (56)

• cmd/multi_service_coverage_test.go
• cmd/multi_service_helpers.go
• cmd/multi_service_test.go
• cmd/multi_service_test_common_test.go
• internal/purchase/mocks_test.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_test.go
• pkg/provider/interface.go
• providers/aws/recommendations/client.go
• providers/aws/recommendations/client_test.go
• providers/aws/recommendations/sku.go
• providers/aws/recommendations/sku_test.go
• providers/aws/service_client.go
• providers/aws/service_client_test.go
• providers/aws/services/ec2/client.go
• providers/aws/services/ec2/client_test.go
• providers/aws/services/elasticache/client.go
• providers/aws/services/elasticache/client_test.go
• providers/aws/services/memorydb/client.go
• providers/aws/services/memorydb/client_test.go
• providers/aws/services/opensearch/client.go
• providers/aws/services/opensearch/client_test.go
• providers/aws/services/rds/client.go
• providers/aws/services/rds/client_test.go
• providers/aws/services/redshift/client.go
• providers/aws/services/redshift/client_test.go
• providers/aws/services/savingsplans/client.go
• providers/aws/services/savingsplans/client_test.go
• providers/azure/recommendations.go
• providers/azure/recommendations_test.go
• providers/azure/services/cache/client.go
• providers/azure/services/cache/client_test.go
• providers/azure/services/compute/client.go
• providers/azure/services/compute/client_test.go
• providers/azure/services/cosmosdb/client.go
• providers/azure/services/cosmosdb/client_test.go
• providers/azure/services/database/client.go
• providers/azure/services/database/client_test.go
• providers/azure/services/managedredis/client.go
• providers/azure/services/managedredis/client_test.go
• providers/azure/services/savingsplans/client.go
• providers/azure/services/savingsplans/client_test.go
• providers/azure/services/search/client.go
• providers/azure/services/search/client_test.go
• providers/azure/services/synapse/client.go
• providers/azure/services/synapse/client_test.go
• providers/gcp/recommendations.go
• providers/gcp/recommendations_test.go
• providers/gcp/services/cloudsql/client.go
• providers/gcp/services/cloudsql/client_test.go
• providers/gcp/services/cloudstorage/client.go
• providers/gcp/services/cloudstorage/client_test.go
• providers/gcp/services/computeengine/client.go
• providers/gcp/services/computeengine/client_test.go
• providers/gcp/services/memorystore/client.go
• providers/gcp/services/memorystore/client_test.go

[cristim]

@coderabbitai The three GCP nil-pointer-dereference findings are false positives. Evidence:

1. All three flagged GetRecommendations methods take params common.RecommendationParams BY VALUE, not a pointer:

   • providers/gcp/recommendations.go:97
   • providers/gcp/services/cloudsql/client.go:142
   • providers/gcp/services/cloudstorage/client.go:162

   None of them dereference a pointer parameter; there is no params := *p line in any of these files.

2. No pointer type exists anywhere. git grep '\*common.RecommendationParams' over providers/, pkg/, and cmd/ returns zero hits. RecommendationParams is a struct (pkg/common/types.go:362) passed by value everywhere, so nil is structurally impossible to pass.

3. The suggested if params == nil { ... } guard does not compile on a value-type struct: invalid operation: params == nil (mismatched types RecommendationParams and untyped nil). Applying it would break the build.

4. These files are not part of this PR's diff: git diff origin/main...HEAD -- providers/gcp/** is empty. This PR only adds AWS test coverage plus lint fixes in providers/aws/recommendations/.

Verification: cd providers/gcp && go build ./... && go test ./... both pass (259 tests). The build succeeding is itself proof the params are value types, since value-type call sites would fail to compile against pointer params.

No code change is warranted. Requesting re-review of those three threads.

[cristim]

@coderabbitai review

[coderabbitai]

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

[coderabbitai]

Tip

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

@cristim You're right — those three nil-pointer-dereference findings were false positives.

common.RecommendationParams is a value type here, so a nil dereference cannot occur on those call paths, and the suggested params == nil guard would indeed be invalid Go for this signature. Also agreed that the flagged GCP files are outside this PR’s scope.

No code change is needed there; please consider those three threads withdrawn.

(・x・)

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/842

Timestamp: 2026-06-24T23:10:40.643Z
Learning: In LeanerCloud/CUDly, `common.RecommendationParams` is a struct passed by value across `providers/`, `pkg/`, and `cmd/`; GCP `GetRecommendations` methods such as `providers/gcp/recommendations.go`, `providers/gcp/services/cloudsql/client.go`, and `providers/gcp/services/cloudstorage/client.go` do not take `*common.RecommendationParams`. Reviews should not suggest nil-pointer-dereference guards like `if params == nil` for these value-type parameters because that would not compile in Go.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[cristim]

@coderabbitai review

[coderabbitai]

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

[coderabbitai]

🧹 Nitpick comments (1)

providers/gcp/services/computeengine/client.go (1)

216-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Duplicated resolveRecommenderClient across service clients.

Identical logic (including doc comment) is also added in providers/gcp/services/memorystore/client.go (lines 154-165), and likely in the cloudsql/cloudstorage clients per this cohort's file list. Consider extracting a shared helper (e.g., accepting the injected client, a constructor func, and clientOpts) into a common internal package to avoid maintaining four copies of the same resolution logic.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@providers/gcp/services/computeengine/client.go` around lines 216 - 227, The
resolveRecommenderClient logic is duplicated across multiple GCP service
clients, including ComputeEngineClient and the same pattern in
MemorystoreClient, so extract the shared resolution flow into a common internal
helper. Move the injected-client check and constructor fallback into a reusable
function that accepts the existing client field, a constructor func, and
clientOpts, then have each service client call that helper from its
resolveRecommenderClient method.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@providers/gcp/services/computeengine/client.go`:
- Around line 216-227: The resolveRecommenderClient logic is duplicated across
multiple GCP service clients, including ComputeEngineClient and the same pattern
in MemorystoreClient, so extract the shared resolution flow into a common
internal helper. Move the injected-client check and constructor fallback into a
reusable function that accepts the existing client field, a constructor func,
and clientOpts, then have each service client call that helper from its
resolveRecommenderClient method.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 34fc773f-03e9-417d-976c-497dc8db4c40

📥 Commits

Reviewing files that changed from the base of the PR and between 6082958 and 41caec2.

📒 Files selected for processing (56)

• cmd/multi_service_coverage_test.go
• cmd/multi_service_helpers.go
• cmd/multi_service_test.go
• cmd/multi_service_test_common_test.go
• internal/purchase/mocks_test.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_test.go
• pkg/provider/interface.go
• providers/aws/recommendations/client.go
• providers/aws/recommendations/client_test.go
• providers/aws/recommendations/sku.go
• providers/aws/recommendations/sku_test.go
• providers/aws/service_client.go
• providers/aws/service_client_test.go
• providers/aws/services/ec2/client.go
• providers/aws/services/ec2/client_test.go
• providers/aws/services/elasticache/client.go
• providers/aws/services/elasticache/client_test.go
• providers/aws/services/memorydb/client.go
• providers/aws/services/memorydb/client_test.go
• providers/aws/services/opensearch/client.go
• providers/aws/services/opensearch/client_test.go
• providers/aws/services/rds/client.go
• providers/aws/services/rds/client_test.go
• providers/aws/services/redshift/client.go
• providers/aws/services/redshift/client_test.go
• providers/aws/services/savingsplans/client.go
• providers/aws/services/savingsplans/client_test.go
• providers/azure/recommendations.go
• providers/azure/recommendations_test.go
• providers/azure/services/cache/client.go
• providers/azure/services/cache/client_test.go
• providers/azure/services/compute/client.go
• providers/azure/services/compute/client_test.go
• providers/azure/services/cosmosdb/client.go
• providers/azure/services/cosmosdb/client_test.go
• providers/azure/services/database/client.go
• providers/azure/services/database/client_test.go
• providers/azure/services/managedredis/client.go
• providers/azure/services/managedredis/client_test.go
• providers/azure/services/savingsplans/client.go
• providers/azure/services/savingsplans/client_test.go
• providers/azure/services/search/client.go
• providers/azure/services/search/client_test.go
• providers/azure/services/synapse/client.go
• providers/azure/services/synapse/client_test.go
• providers/gcp/recommendations.go
• providers/gcp/recommendations_test.go
• providers/gcp/services/cloudsql/client.go
• providers/gcp/services/cloudsql/client_test.go
• providers/gcp/services/cloudstorage/client.go
• providers/gcp/services/cloudstorage/client_test.go
• providers/gcp/services/computeengine/client.go
• providers/gcp/services/computeengine/client_test.go
• providers/gcp/services/memorystore/client.go
• providers/gcp/services/memorystore/client_test.go

✅ Files skipped from review due to trivial changes (4)

• providers/aws/services/opensearch/client_test.go
• providers/gcp/services/cloudstorage/client_test.go
• providers/gcp/services/computeengine/client_test.go
• providers/azure/services/cache/client_test.go

🚧 Files skipped from review as they are similar to previous changes (50)

• providers/aws/services/elasticache/client_test.go
• providers/azure/recommendations_test.go
• providers/azure/services/search/client.go
• internal/scheduler/scheduler_test.go
• providers/aws/services/savingsplans/client_test.go
• providers/azure/services/managedredis/client.go
• pkg/provider/interface.go
• providers/azure/services/compute/client_test.go
• providers/aws/services/redshift/client.go
• providers/azure/services/savingsplans/client.go
• providers/aws/services/ec2/client.go
• cmd/multi_service_test_common_test.go
• providers/aws/services/redshift/client_test.go
• providers/aws/services/rds/client_test.go
• providers/azure/services/savingsplans/client_test.go
• providers/azure/services/search/client_test.go
• internal/scheduler/scheduler.go
• providers/azure/services/synapse/client_test.go
• providers/azure/services/database/client_test.go
• providers/aws/services/memorydb/client.go
• providers/azure/services/cosmosdb/client_test.go
• providers/azure/services/database/client.go
• providers/gcp/recommendations_test.go
• providers/aws/services/savingsplans/client.go
• providers/gcp/services/memorystore/client_test.go
• cmd/multi_service_coverage_test.go
• providers/aws/services/ec2/client_test.go
• providers/aws/services/rds/client.go
• providers/aws/services/memorydb/client_test.go
• providers/azure/services/synapse/client.go
• providers/aws/recommendations/client_test.go
• providers/aws/services/opensearch/client.go
• providers/azure/recommendations.go
• providers/azure/services/compute/client.go
• providers/aws/services/elasticache/client.go
• providers/aws/service_client.go
• providers/aws/service_client_test.go
• providers/azure/services/cosmosdb/client.go
• providers/azure/services/cache/client.go
• providers/gcp/services/cloudsql/client.go
• providers/azure/services/managedredis/client_test.go
• cmd/multi_service_helpers.go
• cmd/multi_service_test.go
• providers/gcp/recommendations.go
• providers/gcp/services/cloudstorage/client.go
• providers/gcp/services/cloudsql/client_test.go
• internal/purchase/mocks_test.go
• providers/aws/recommendations/sku_test.go
• providers/aws/recommendations/sku.go
• providers/aws/recommendations/client.go