fix(recommendations): reject incomplete savings plans menus

[cristim]

Savings Plans recommendations with rejected details, failed plan types, or late-page failures previously escaped as successful MCP menus. Pin the published AWS collection fix so the registered tool rejects those incomplete results and returns no StructuredContent.

The existing real AWS adapter and registered MCP transport tests now cover all four plan types and the umbrella, default and explicit arguments, valid and empty responses, malformed details, API failures, and pagination failure. No MCP production code changes.

Refs LeanerCloud/cloud-commitments-go#170. That issue remains open for CLI and Platform rollout.

Native Go 1.26.6 verification on the exact commit includes 78 passing protocol cases, the full race-short suite across all three packages, build, and pinned golangci-lint 2.10.1. The previous published AWS pin fails the new behavioral assertions while controls pass. An independent diagnostic-loss mutation also fails the intended assertion while valid and empty controls pass. These are bounded AWS HTTP fixtures through the actual MCP consumer path, not live-cloud evidence.

Fresh gpt-6-astra review found no actionable findings at f39c118b54d8b313691c6ca0f0888a5d35e43864, tree 14bf25ecb9b57223d1aa5578cc0332c5e4c718f6. The complete verdict and reproduced commands are posted below. Linux CI is checked separately. The normal installed pre-commit hooks passed. This checkout has no optional pre-push hook; the exact committed full race-short suite was run independently before publication. The existing hooks were preserved and the push used no bypass.

Summary by CodeRabbit

• Tests
  • Expanded coverage for AWS recommendation completeness across database and Savings Plans results, including selected and unselected inputs.
  • Added checks for partial and complete API failures, later-page failures, request details, and diagnostic reporting, helping validate how recommendation results are handled in these scenarios.

[cristim]

Independent committed-source review

Reviewer: fresh-context gpt-6-astra, 2026-10-03. The user-selected model overrides the historical Claude model pin. I did not implement the change. I used the interrogate correctness, root-cause, structure, verification, complexity and security rubric, together with coding-standards and conventions. No additional reviewer was spawned.

Verdict: no actionable findings in the full three-file MCP diff at f39c118b54d8b313691c6ca0f0888a5d35e43864. Native consumer verification is green with the published dependency and demonstrably red with the previous dependency and an independent diagnostic-loss mutation. This verdict does not assert Linux CI success or live-cloud verification, and is not an instruction to merge.

Identity and integrity

Repository: /Users/cristi/.claude/worktrees/go170-mcp-completeness.
Base: e073769ac91505956c5deab733208a264e215fde.
Head: f39c118b54d8b313691c6ca0f0888a5d35e43864.
Tree: 14bf25ecb9b57223d1aa5578cc0332c5e4c718f6.

Start and end git status --porcelain=v1 were empty. Start and end SHA-256 values were identical:

File	SHA-256
go.mod	3ad97609c1010e271968063490f382e04fc17eae4ba972e9ce2d94592aeb8375
go.sum	60b694dffc1ce6763517a21e3383bc4217512b80f933aa30846a5bf36980367e
tools/search_recommendations_completeness_test.go	8a85069529d15e3aebfc0bec042ea8c5a12bc630eb45bd28a98dbd4d60cb8bb1

git diff --exit-code HEAD -- for those files exited 0. The full base-to-head diff has 195 insertions and 82 deletions, exclusively those three files. No MCP production code changed.

The downloaded module .info at /Users/cristi/go/pkg/mod/cache/download/github.com/!leaner!cloud/cloud-commitments-go/providers/aws/@v/v0.0.0-20261003204812-9962786e0695.info independently identifies origin https://github.com/LeanerCloud/cloud-commitments-go, subdir providers/aws, full SHA 9962786e06951a20d58980954e8024fbf49e2d0c, timestamp 2026-10-03T20:48:12Z. The manifest and checksum entries name that published version. There are no shipping replace directives; GOWORK=off was set throughout. I did not independently inspect GitHub PR 172's merge metadata.

Contract checked against source

Producer paths below are under /Users/cristi/go/pkg/mod/github.com/!leaner!cloud/cloud-commitments-go/providers/aws@v0.0.0-20261003204812-9962786e0695/.

• tools/search_recommendations.go:125 validates the request and obtains the provider adapter. At line 151 it consumes fetchSearchCombos; line 153 returns its error. fetchSearchCombos at lines 256-265 rejects any returned diagnostic before appending recommendations. Thus survivors cannot escape as a successful partial menu.
• service_client.go:69 builds the real recommendations adapter. Lines 76-88 preserve an IncompleteRecommendationsError through filtering and return it with the rows. No adapter mock replaces that implementation in the new matrix.
• recommendations/client.go:143 dispatches every SP selector to the SP collector. parser_sp.go:263-285 maps the four selectors; lines 321-350 define the umbrella's deterministic four-plan order. This matches the expected SDK types at test lines 64-68.
• collection_sp.go:37-55 sends the requested plan type, payment, term, lookback and LINKED scope, then merges the results. Lines 69-99 record page identity, parse successful responses, preserve failed-page scope, and carry the next token. parser_sp.go:27-45 counts rejected rows and returns survivors plus the incomplete diagnostic. The malformed field is actually parsed and rejected at lines 156-158.
• recommendations/client.go:428-456 distinguishes all ordinary failures from incomplete collections. collection_error.go:17-36 renders exact failed-detail/scope counts and flattens nested incomplete errors without double counting. The tests check these counts, context tokens, ordinary-fatal distinction, and absent StructuredContent at lines 94-127. They intentionally do not compare the entire rendered error text; the SP assertion checks the page/detail prefix rather than each numerical detail index.
• Pinned SDK /Users/cristi/go/pkg/mod/github.com/aws/aws-sdk-go-v2/service/costexplorer@v1.61.0/serializers.go:4678-4725 serializes AccountScope, LookbackPeriodInDays, NextPageToken, PaymentOption, SavingsPlansType and TermInYears exactly as the fixture's decoded request fields. Test line 92 compares the complete expected sequence of these fields.
• Test lines 35-53 run the production adapter and registered handler through real MCP client/server in-memory transports. Lines 194-243 replace only the AWS HTTP response boundary; unexpected hosts/operations fail locally. Lines 157-171 require successful valid/empty menus, exact counts, and a non-nil recommendations slice.

Independent commands and results

All commands ran natively on Darwin arm64 with /Users/cristi/sdk/go1.26.6/bin/go (go version go1.26.6 darwin/arm64). Pinned /Users/cristi/go/bin/golangci-lint version returned 2.10.1 built with Go 1.26.6, matching .github/workflows/ci.yml:52 and go.mod:3.

Each log is beside this report. run.py records the exact argv and exit. It uses existing canonical GOCACHE /Users/cristi/Library/Caches/go-build, GOMODCACHE /Users/cristi/go/pkg/mod, GOMAXPROCS=2, GOWORK=off, GOTOOLCHAIN=local, GOPROXY=off, synthetic AWS credentials, disabled metadata, /dev/null AWS configuration, and blocked outbound proxies. No fresh caches, Docker, Linux local execution, cloud queries, purchases or deployments were used. Every heavy command used /usr/bin/lockf -k -t 600 /tmp/agent-locks/cloud-commitments-mcp-{test-suite,build}.lock; lock files were not unlinked. The runner received two static reviews: the first removed inherited credential-bearing environment, the second checked argv handling, file exclusivity, timeout, exit propagation and lock ownership.

Evidence	Exact command after lock wrapper	Result
matrix.log	go test -mod=readonly -race -p=2 -count=1 -timeout=5m -v ./tools -run '^TestSearchRecommendationsAWSCompletenessProtocol$'	Exit 0; 78 leaf cases passed, no skips
full-suite.log	go test -mod=readonly -race -short -p=2 -count=1 -timeout=10m -json ./...	Exit 0; all 3 packages passed; 466 test/subtest pass events; zero skips
build.log	go build -mod=readonly -p=2 ./...	Exit 0
lint-retry.log	golangci-lint run --concurrency=2 --timeout=5m ./...	Exit 0; 0 issues
baseline.log	go test -modfile=/private/tmp/go170-mcp-astra-ANOtRi/baseline.mod -mod=readonly -race -p=2 -count=1 -timeout=5m -v ./tools -run '^TestSearchRecommendationsAWSCompletenessProtocol$'	Behavioral exit 1; 34 failed/44 passed
mutation-retry.log	go test -modfile=/private/tmp/go170-mcp-astra-ANOtRi/mutation.mod -mod=readonly -race -p=2 -count=1 -timeout=5m -v ./tools -run '^TestSearchRecommendationsAWSCompletenessProtocol$/^savings-plans-compute$/^selected=false$/^(mixed|valid|empty)$'	Behavioral exit 1; mixed fails, valid/empty pass

The mutation command's real final regex has ordinary | alternation; the backslashes in this Markdown table only escape its column delimiters. The exact argv is in the log.

Baseline mod/sum copies downgrade only AWS to published v0.0.0-20261002152209-006ef5c8d0a2, retaining the final committed tests and all other pins. Malformed SP rows wrongly yield successful menus, all-invalid yields successful empty menus, and late-page failures lack the completeness context. All RDS controls and all SP valid/empty cases pass.

The mutation uses an owned copy of the published AWS module at /private/tmp/go170-mcp-astra-ANOtRi/aws, referenced only by mutation.mod. It changes parser_sp.go:43 from return recommendations, incomplete to return recommendations, nil. The mixed Compute/default case fails at committed test line 94 because a successful one-row menu escapes; valid and empty controls pass. No GOMODCACHE source or author source was edited. The unmodified module hash below confirms the producer remained intact.

Two environmental attempts are retained, not counted as behavioral evidence: mutation.log exited 1 at setup because the sandbox denied a canonical Go cache write; lint.log exited 5 with package-loader "no go files". Scoped escalated retries used the same shared caches and succeeded in executing the intended checks. All run handles are terminal.

Six-dimension assessment

• Completeness: all four SP selectors and umbrella, default/explicit fields, valid/empty, mixed/all-invalid, partial/all-type failure and late-page failure are exercised; RDS controls remain. No omitted requested behavior found.
• Correctness: the handler's reject-on-error behavior consumes the corrected producer contract; the SDK request assertions and producer source agree. Counts do not double count nested scopes. Baseline and mutation prove non-vacuity.
• Security: no new production trust boundary, credential, purchase or external transport path. Synthetic credentials and terminal HTTP fixture keep this matrix isolated.
• Bugs/concurrency: each parallel case owns its adapter, transports and fixture. The fixture's mutex protects recorded requests and final inspection; deferred unlock runs before session teardown. Race tests pass. Context timeout bounds broken pagination.
• Reuse: existing registration helper, fake provider boundary, HTTP fixture and RI controls are extended; no duplicated collector or new production abstraction.
• Complexity/scope: 269-line test file stays under the local size guideline; case-specific assertions directly express the distinct RI/SP contracts. Production change is confined to the published dependency pin. I found no justified structural rewrite or unrelated change to request.

Source hashes

Published authority	SHA-256
recommendations/parser_sp.go	638e92d9779f69f83f0782559eade6ca3467f2e52ce2a5289c62f3a4fbd3f9fb
recommendations/collection_sp.go	b3ac1593479093bc49ed1f9653149d6a1a44c5192556384029f15912503c4bfe
recommendations/collection_error.go	9e9ba60196b86f7b67aa365700315e7624f199a31ba9d747edb162e651a700ae
recommendations/client.go	fdff603c7d0452f5fc9d3d240fad10b71169530bf54b43bf7ae34cb0dc9f6e52
service_client.go	718ff7fa555809148700dccd701d45fda0b2a1458231786f4bc8cf72ea67b4b8
costexplorer v1.61.0 serializers.go	a7e27bfa0afabb7443b2931c9ec281681c05043fe03da33e054cfc664ec414e1
Owned mutated parser_sp.go	36d04388840e08bf93498339a47cc133b387063b7ac968c3172d93af2b635b52

Limits and handoff

This is native execution of the actual registered MCP consumer path with fixture AWS responses and a fake provider factory, not live AWS data or stdio process coverage for this scenario. The full suite separately runs server and binary tests. Do not relabel the matrix as live-cloud proof. Linux CI remains a separate required check after publishing the reviewed commit; no Linux or GitHub status is asserted here. A future commit invalidates this exact-SHA verdict. Compass was unavailable on PATH and the worktree had no Compass graph; direct source tracing supplied the contract map. SharedGo issue 170 must remain open for CLI and Platform B-D rollout. I made no commits, pushes, PR edits, GitHub mutations or deletions, and release the heavy-Go slot.

[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-mcp/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 4d428738-2150-4b4f-87e9-c85671da649e

📥 Commits

Reviewing files that changed from the base of the PR and between e073769 and f39c118.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (2)

• go.mod
• tools/search_recommendations_completeness_test.go

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

---

📝 Walkthrough

Walkthrough

The AWS provider dependency is updated. Protocol tests now cover recommendation completeness for RDS and Savings Plans, including selection and API failure cases.

Changes

AWS recommendation completeness

Layer / File(s)	Summary
Cross-service completeness protocol coverage go.mod, tools/search_recommendations_completeness_test.go	The AWS provider dependency is updated. Tests cover RDS and Savings Plans requests, selected and unselected inputs, successful and incomplete results, diagnostics, and Savings Plans API failures, including a later-page failure.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to f39c1

No concrete issue remains that should block merging. Complete the normal Linux CI check before merge.

🚥 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 1 functions across 1 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 describes the main change: rejecting incomplete Savings Plans recommendation results.
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 1 functions across 1 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.}