test(recommendations): verify Savings Plans completeness

[cristim]

Refs LeanerCloud/cloud-commitments-go#170. This is the CLI consumer rollout; the parent issue remains open for the Platform persistence rollout.

Pin the published AWS provider to v0.0.0-20261003204812-9962786e0695 (origin commit 9962786e06951a20d58980954e8024fbf49e2d0c) and extend the existing actual child-command completeness tests. No CLI production logic changes.

The 50-case matrix preserves 15 RI controls and adds 35 SP cases: four explicit plan types plus the CLI umbrella alias, each covering valid, mixed, all-invalid, empty, ordinary API error, failed type, and late-page failure. Strict TLS SDK fixtures assert exact request tuples and read-only operation counts, survivor CSV values, and diagnostic counts. The umbrella alias expands into independent per-type CLI calls, not a direct producer umbrella call.

Local evidence is synthetic SDK fixture transport through the real CLI command, not live AWS account verification. Purchases and unexpected network operations are rejected. No Docker, Windows, deployment, or real purchase verification was performed.

Verification on reviewed commit 57aa3996983d36f1a592460a3350b9fee348f7cf: independent cold Astra reproduced all 50 cases with race, the full race-short suite, build, vet and pinned lint. The full suite retains two preexisting credential-dependent integration skips. Old published AWS pin: 35 controls passed and 15 intended SP cases failed. Warning-suppression mutation failed at the intended assertion. Normal installed commit hooks passed.

Independent exact-commit verdict will be recorded in a PR comment. CodeRabbit is not requested under the user-authorized Astra/local-proof review path. Root owns merge after CI.

Summary by CodeRabbit

• Tests
  • Expanded validation of recommendation completeness across RDS and Savings Plans scenarios, including failed requests, incomplete results, and later-page responses.
  • Added checks for Savings Plans request handling, warning and fetch-failure counts, and generated CSV results.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: fcf0c9c7-4e00-4d77-92e0-c0ad80719fc7

📥 Commits

Reviewing files that changed from the base of the PR and between 1ce4658 and 57aa399.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (3)

• cmd/recommendation_completeness_proxy_test.go
• cmd/recommendation_completeness_test.go
• go.mod

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

---

📝 Walkthrough

Walkthrough

The completeness tests now cover RDS and Savings Plans service selections. The proxy fixture validates Savings Plans requests and returns scenario-specific responses. Assertions check request counts, warnings, fetch failures, surviving plans, and CSV output.

Changes

Completeness test coverage

Layer / File(s)	Summary
Service-aware proxy fixtures cmd/recommendation_completeness_proxy_test.go	The proxy fixture handles Savings Plans inventory and recommendation requests, records requests and RDS engine names, and supplies responses for the configured service and scenario. Request assertions check expected operations and request tuples.
Scenario execution and output assertions cmd/recommendation_completeness_test.go, go.mod	The tests run RDS and Savings Plans scenarios across five service selections, including failed-type and late-page. Assertions check warnings, fetch failures, surviving plans, and CSV output. The AWS provider dependency version is updated.

Priority: ⬇️ Low

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

Change: Other

Merge Risk: ⚪ Minimal · up to 57aa3

The added completeness cases are reported to pass, and no concrete merge-blocking issue remains. Live AWS behavior was not verified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 …	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 clearly and concisely identifies the pull request’s main change: tests that verify Savings Plans recommendation completeness.
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.

Full details: Docstring Coverage

Explanation

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

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

[cristim]

Independent committed CLI review

Verdict: no actionable findings in the complete four-file change at 57aa3996983d36f1a592460a3350b9fee348f7cf. This is a fresh independent GPT-6 Astra review, as requested, with author claims treated as hypotheses. I did not author or modify the shipping change.

Reviewed identity

• Repository: LeanerCloud/cloud-commitments-cli.
• Checkout: /Users/cristi/.claude/worktrees/go170-cli-completeness.
• Base: 1ce4658a0c29ca3f8bd289a428166883a0b0d1de.
• Head: 57aa3996983d36f1a592460a3350b9fee348f7cf.
• Tree: 969860ab5b0fbf6e9d780ab1bdbed5028c844a63.
• Full raw diff SHA-256: ab0053c535d1c33b6b891dd2695ecf8e2bcbfea4b0f06ebd4398c7846be695c8.
• Scope: two existing test files plus go.mod and go.sum; 280 additions and 51 deletions. Raw /usr/bin/git was used for committed source, manifests, identities, and diff.
• Checkout was clean initially, before and after every verification command, and at the final identity check. The four working-file Git blob IDs equal their committed HEAD blobs.

File	Git blob	SHA-256
cmd/recommendation_completeness_test.go	f1fa8caba04b29676fdeabe07cff9276fa51c476	78770c3b43e08648d747c2afd2f43b70f32684c9032e71a2a29cdf7d5552d1b9
cmd/recommendation_completeness_proxy_test.go	55f5e8880711fd9eeca795c813f79544e268564d	cec75c530e0ba7a6e8df7dde1b250a0224fd5b7498957c7f0a01c30e9fa9b8ff
go.mod	c973ecf9e763e94c0bc7b86684879ef020edf74e	65d85903aab11227070d7cb0a035d4b53a47fa7f5c82fb4161fae7c3f72acedf
go.sum	86de8e6cbe0e306f2fb6114eddfc0633752f493d	09b37bf15cd4a2207ba98826a7b0e0d38b9e1921f688085c426bada720afb80d

Runtime evidence

Every following result was independently executed in this review. Logs in this directory contain the exact command, scrubbed environment, pre/post commit and file hashes, elapsed time, and terminal exit status. No author log was substituted for a reproduced result.

All Go commands used /Users/cristi/sdk/go1.26.6/bin/go, GOTOOLCHAIN=local, GOWORK=off, GOMAXPROCS=2, the existing /Users/cristi/Library/Caches/go-build and /Users/cristi/go/pkg/mod caches, and disabled module network access. Tests and build/vet use -p=2; lint uses --concurrency=2. Commands were serialized with persistent lockf -k locks at /tmp/agent-locks/cloud-commitments-cli-test-suite.lock or /tmp/agent-locks/cloud-commitments-cli-build.lock.

Check and exact Go/tool arguments	Exit	Observed result	Log
test -race -p=2 -count=1 -timeout=15m -json ./cmd -run '^TestRecommendationCompletenessCommand$'	0	All 50 named leaf cases passed; zero skips; package 67.565 s	focused-race-local.log
test -race -p=2 -count=1 -short -timeout=15m -json ./...	0	cmd package passed in 145.600 s; 1020 test pass events, 2 existing skips; all 50 focused leaves also passed	full-race-short.log
test -race -p=2 -count=1 -timeout=15m -mod=readonly -modfile=/private/tmp/go170-cli-astra-yL871t/baseline.mod -json ./cmd -run '^TestRecommendationCompletenessCommand$'	1, expected	35 controls passed, 15 behavioral failures, zero skips	baseline-race.log
test -race -p=2 -count=1 -timeout=15m -overlay=/private/tmp/go170-cli-astra-yL871t/warning_drop.json -json ./cmd -run '^TestRecommendationCompletenessCommand$/^savingsplans-compute$/^mixed$'	1, expected	Exactly the selected case failed at the original warning assertion	warning-mutation.log
Same mutation command with warning_survivor.json	1, expected	Exact CSV survivor verified before the original expected-failing warning assertion	warning-survivor.log
build -p=2 -mod=readonly -o /private/tmp/go170-cli-astra-yL871t/cudly ./cmd	0	Native CLI binary built, 17.234 s	build.log
vet -p=2 -mod=readonly ./...	0	No diagnostics, 2.783 s	vet.log
/Users/cristi/go/bin/golangci-lint run --concurrency=2 --timeout=10m ./...	0	Version 2.10.1, built with Go 1.26.6; 0 issues., 47.219 s	lint-local.log
mod download -json github.com/LeanerCloud/cloud-commitments-go/providers/aws@v0.0.0-20261003204812-9962786e0695	0	Cached published origin and sums match the shipping pin	module-origin.log
mod verify	0	all modules verified	module-verify.log
version -m /private/tmp/go170-cli-astra-yL871t/cudly	0	Go 1.26.6, darwin/arm64; exact published AWS dependency and sum, no replacements	binary-modules.log

git diff --check BASE HEAD also exited 0. CI pins golangci-lint v2.10.1 in .github/workflows/ci.yml:46; go.mod:3 specifies Go 1.26.6.

The full suite's two skips are unconditional pre-existing live-AWS tests: cmd/helpers_test.go:198 (TestGetAccountAliasRealFunction) and cmd/main_test.go:292 (TestRunTool). The full count above is test pass events, including parent tests, not 1020 distinct leaf scenarios.

Baseline and mutation non-vacuity

The baseline manifests were extracted independently from the raw base commit into owned scratch files. The final committed test files were unchanged, no overlay was active, and the old published AWS version was v0.0.0-20261002152209-006ef5c8d0a2. All 15 RI controls passed, as did the 20 SP controls covering valid, empty, total API failure, and failed type across five selectors. The 15 failing cases were precisely mixed, all-invalid, and late-page for Compute, EC2Instance, SageMaker, Database, and the umbrella selector. They failed at cmd/recommendation_completeness_test.go:122: expected completeness-warning count 1 (or 4 for umbrella), actual 0. They were not compile, fixture setup, TLS, or inventory-contract failures.

The independent warning mutation removes only AppLogger.Printf(" ⚠️ %v\n", incomplete) from cmd/multi_service_helpers.go:472, leaving the returned SDK survivors intact. The overlay points at owned scratch source outside GOMODCACHE. The unchanged committed mixed-case test reaches and passes its SDK request/inventory assertions, then fails at line 122 with expected 1 versus actual 0.

The additional survivor probe overlays a scratch copy of the test file that adds one assertion before the original mixed-warning assertion. It applies the existing valid-row CSV checks to the actual mixed-response output while requiring zero completeness warnings. It therefore proves the exact surviving CSV row and total are present before the original warning assertion fails. Its log explicitly records:

ASTRA mutation: exact CSV survivor and zero warning verified before original mixed-warning assertion

Both mutation runs observed exactly one SP recommendation request, one SP inventory read, one region read, one RDS instance read, and four RDS engine metadata reads. Neither changed the SDK module or its cache. The survivor probe is separate supplemental evidence; the primary warning mutation and old-module baseline use unchanged committed tests.

Source-contract review

Paths below are relative to the immutable CLI checkout above, unless prefixed AWS or PKG. AWS source root is /Users/cristi/go/pkg/mod/github.com/!leaner!cloud/cloud-commitments-go/providers/aws@v0.0.0-20261003204812-9962786e0695; PKG source root is /Users/cristi/go/pkg/mod/github.com/!leaner!cloud/cloud-commitments-go/pkg@v0.0.0-20260929105827-b3b4cb5e3d80.

Contract examined	Source authority and conclusion
Real command route	cmd/recommendation_completeness_test.go:54 starts the test executable as a fresh child; lines 83-99 load the fixture trust root and call rootCmd.Execute with actual CLI flags. This runs the real configuration and command pipeline, not a helper-only substitute.
Selector normalization	cmd/main.go:173 and :207 expand savingsplans into four per-type service slugs in fixed order. Thus the umbrella CLI makes four per-type SDK calls, not a single SDK umbrella call.
No purchase	cmd/multi_service.go:111 defaults to dry run unless ActualPurchase is true; :480 returns dry-run results. Fixture flags never include purchase or --yes; proxy dispatch rejects purchase operations.
Ancillary reads	cmd/multi_service.go:153 unconditionally fetches RDS engine data. cmd/multi_service_engine_versions.go:44 discovers regions and RDS instances; :218 lists mysql, postgres, aurora-mysql, aurora-postgresql. Fixture expectations at cmd/recommendation_completeness_proxy_test.go:336 match these actual reads.
Account-level command region	cmd/multi_service_helpers.go:262 chooses one us-east-1 collection scope for Savings Plans. :455 supplies the default lookback; :459 carries service, region, payment, term, lookback, and SP filters into the SDK.
Wire request tuple	AWS recommendations/collection_sp.go:37 emits plan type, term, payment, lookback, and LINKED account scope; :91 advances NextPageToken. Pinned AWS Cost Explorer v1.61.0/serializers.go:4678 emits those exact field names. Test proxy :317 and :324 asserts the captured tuple sequence, including the second late-page token.
Malformed details	AWS recommendations/parser_sp.go:29 parses each detail; :31 increments FailedDetails and records its error; :42 returns surviving rows with the typed incomplete error. Numeric parsing is at :156. The CLI's existing typed-error branch at cmd/multi_service_helpers.go:471 prints the warning and retains those rows.
Page failures and ordinary errors	AWS recommendations/collection_sp.go:74 records page errors and merges pages at :99. AWS recommendations/client.go:428 merges results, returns an ordinary error if every scope failed at :448, and otherwise returns an incomplete error with survivors at :451. CLI cmd/multi_service_helpers.go:488 logs ordinary fetch failures and yields no rows for that service.
Adapter preserves diagnostic	AWS service_client.go:76 allows typed incomplete errors through filtering and returns the same error with filtered survivors. Ordinary errors return nil rows immediately.
EC2Instance routing	AWS recommendations/parser_sp.go:116 obtains nested SavingsPlansDetails.Region. AWS service_client.go:156 derives EffectiveRegion, :204 exempts only genuinely global SP types, and :255 filters included regions. Fixture proxy :295 supplies the nested EC2 region; top-level CSV Region remains blank because the parser's Recommendation stores it in Details.
Inventory request	AWS services/savingsplans/client.go:143 requests active/payment-pending/pending-return/queued states, MaxResults 100, and initially no NextToken. Pinned Savings Plans v1.31.0/serializers.go:331 uses POST /DescribeSavingsPlans; :375 encodes lower-camel JSON keys. Fixture proxy :137 validates that route and values, and :340 asserts exact operation counts.
CSV ordering and values	PKG scorer/scorer.go:57 sorts ties by Service, Region, ResourceType. cmd/multi_service_csv.go:263 applies stable upfront-cost order and :275 writes fields. Equal fixture costs preserve scorer order, as expected at cmd/recommendation_completeness_test.go:153. AWS parser :221 produces 2 times 730 = 1460 recurring monthly cost, :237 Count 1, and retains savings 10/upfront 3.
Diagnostic separation	Test cmd/recommendation_completeness_test.go:122 counts typed completeness warnings; :137 counts ordinary fetch failures. The umbrella failed-type case retains the other three types and logs one ordinary failure, matching selector expansion. Total API failure yields ordinary fetch-error output and no CSV.

The cached module metadata identifies VCS git, URL https://github.com/LeanerCloud/cloud-commitments-go, subdirectory providers/aws, full source hash 9962786e06951a20d58980954e8024fbf49e2d0c, and timestamp 2026-10-03T20:48:12Z. The downloaded module sum is h1:DfNEBzFS7/MaBZljLGRRRUaarfnXXZyBE/6iozzp9/k=; go.mod sum is h1:d4nsy61/Ptib0SxqMg0ble3yksVm3+QbtUtmGja82PA=. Shipping go.mod contains no replace directive, GOWORK was off, module verification passed, and the native binary embeds this exact published version and sum.

Six review dimensions and scope

• Completeness: all requested per-type and umbrella scenario classes are present, alongside all 15 retained RI controls. Fixed and baseline outcomes establish that the new pin fixes the missing completeness diagnostics through the command path.
• Correctness: traced selectors, adapter filtering, collector/parser errors, warning counts, CSV survivor values and ordering, ordinary failure behavior, and actual wire serializers. No actionable discrepancy found.
• Security: child environment is explicit, credentials synthetic, config paths absent inside owned temporary homes, metadata disabled, and requests pass through a local TLS proxy with host and operation allowlists. Unexpected operations mark the test failed, including purchase calls. TLS trust is confined to the fixture CA.
• Bugs/concurrency: proxy maps and request collections are mutex-protected; handler lifetime is tracked, socket and subprocess timeouts are bounded, and cleanup waits for handlers. Final focused and full race runs passed.
• Duplication/reuse: the patch extends the existing completeness harness and reuses its actual command runner and RI fixtures. Type/service mappings are independent expected values in a test, not duplicated production routing. No new production abstraction or competing collector was added.
• Over-engineering: the only production behavior change is consuming the published dependency. New test helpers correspond directly to the matrix, request inspection, and CSV assertions. Changed Go files remain 263 and 343 lines, below the project limit. No speculative option or unrelated refactor was introduced.

I also inspected the existing CLI completion behavior: total service API failures are logged and produce no CSV, but the command pipeline still returns normally. This review's ordinary-error result concerns SDK/fetch diagnostics and row rejection; it does not claim a new nonzero CLI process exit contract.

Limits and excluded attempts

• All affected-path evidence is synthetic AWS fixture evidence through the real root command, SDK serializers, adapter, and CSV writer. No live AWS recommendation data, IAM permissions, purchase, deployment, or platform rollout was exercised or claimed.
• Only macOS darwin/arm64 was run locally. Linux CI, Windows, PR state, and merge eligibility are not established by this report. No GitHub writes or publication were performed.
• The first focused attempt (focused-race.log, session 85779) failed before useful execution because the sandbox denied httptest loopback bind. It was excluded and the identical check rerun with scoped permission for local sockets.
• The first lint attempt (lint.log) exited 5 with package-loading failure, no go files to analyze. It was excluded; scoped retry used the same checkout and caches and passed. No tidy or source change was made to mask the failure.
• No existing Compass/graph output or Compass executable was available at the checked standard locations; review used direct committed source and downloaded dependency source instead.
• Review scripts received two static passes before execution: argument/path bounds, exclusive evidence-file creation, scrubbed credentials/proxies, immutable checkout checks, subprocess exit handling, canonical persistent locks, and test-event classification were checked. Neither script deletes files or modifies shipping source.

All review process sessions are terminal. The shared heavy Go slot was explicitly released to the parent before writing this report. Scratch evidence and the native binary remain in this owned temporary directory; nothing was deleted.

[cristim]

Merge gate verified at 57aa399: full independent Astra review and native command-path evidence are recorded above; CI Success and Run pre-commit hooks passed; merge state is CLEAN and there are no review threads. The automatic CodeRabbit summary reports no actionable findings. Its generic docstring-coverage warning is not adopted: these are private test helpers with explicit behavior assertions, not a new public API; adding restatements solely for a percentage conflicts with the project comment convention. No code or check suppression is needed. SharedGo issue #170 remains open for Platform rollout.