Repository navigation
fix(aws): propagate incomplete Savings Plans collections #170
Description
Summary
Savings Plans collection can return success after dropping malformed details or failed plan types. Discovered while working #54 and PR #169.
Current behavior
At commit 8d4be0d1a9286036f8df5f2384becc384480597f:
parseSavingsPlansRecommendationslogs a detail parse error and continues, returning only a slice.fetchSPAllPagestherefore returns the surviving recommendations with a nil error. A malformedHourlyCommitmentToPurchase,EstimatedMonthlySavingsAmount, orUpfrontCostreaches this branch throughparseSavingsPlanDetail.getSavingsPlansRecommendationspropagates API errors for a single requested plan type, but the umbrella path logs failed plan types and returns nil error. This also reports success when every attempted plan type fails.GetAllRecommendationscalls the umbrellaServiceSavingsPlansAllpath. Consumers cannot distinguish these results from complete collections. Strict interactive consumers can display a shortened menu, and batch consumers lack the diagnostic needed to avoid treating missing rows as confirmed absent.
This is separate from #18 and the Provider M1/M2 item in #119, which concern malformed monetary values becoming zero. #18 explicitly permits skipping a rejected detail with a warning; neither issue requires collection-completeness propagation. Some monetary fields now correctly return detail errors, but the collection layer still swallows those errors. This issue does not claim all monetary parsing findings are resolved.
#54 and #169 address RI-origin diagnostics and aggregation of errors that reach the collector. Their scope explicitly leaves Savings Plans parser error swallowing unchanged.
Steps to reproduce
- Supply a real SDK fixture response containing a valid Savings Plans detail and a second detail with malformed HourlyCommitmentToPurchase.
- Call GetRecommendations for a selected Savings Plans service and observe the surviving row with a nil error.
- Call the umbrella Savings Plans service with one successful plan type and one failed API response. Observe success without an incomplete-collection diagnostic. Repeat with all plan types failing.
Expected behavior and acceptance
- Return valid SP survivors with an explicit incomplete-collection diagnostic when any detail or requested plan type fails. Reuse the existing diagnostic contract, keeping failed-detail and failed-scope counts distinct and avoiding double counting across pages, plan types, combinations, and services.
- Preserve ordinary fatal errors when all attempted plan types fail at the API level. Cancellation remains terminal. Clean empty responses remain successful; all-invalid detail responses remain incomplete.
- Verify single-plan-type and umbrella calls, including multi-page responses. Use real SDK fixture transport and demonstrate parent-code failure with valid-plus-malformed details and successful-plus-failed plan types.
- Verify a registered MCP call rejects the incomplete result without a structured short menu, and batch consumers retain valid survivors with diagnostics while excluding incomplete collections from authoritative eviction.
- Keep this change focused on completeness propagation; do not fold monetary-field policy or offering-selection fixes into it.
Proposed fix
Propagate the existing IncompleteRecommendationsError through parseSavingsPlansRecommendations, fetchSPAllPages and getSavingsPlansRecommendations in providers/aws/recommendations/parser_sp.go. Count rejected details and failed requested plan types separately. Preserve surviving rows while retaining terminal cancellation and all-API-failed semantics. Exercise downstream strict and batch policies through their real SDK/protocol/persistence paths.
References
- Parent investigation: fix(aws/recommendations): parseRecommendations silently drops unparseable offers, so a search can present an incomplete menu as complete #54; producer PR fix(aws): report incomplete recommendation collections #169, exact reviewed commit 8d4be0d.
- Related monetary parsing work: fix(providers/aws): fail loud on unparseable SP cost fields + RI utilization hours (COR-07 siblings to #1171) #18 and Provider M1/M2 in Adversarial review of merged PRs -- findings tracker [go part] #119.
- Current source links above establish error swallowing; the reproduction steps require executable regression evidence before a fix is declared complete.
Severity
High: an incomplete offer menu can affect purchase decisions, and an incomplete batch collection can wrongly treat missing rows as confirmed absent.
Activity
cristim commentedon Oct 3, 2026
Producer PR: #172 at
16ed7055b786ba240ff44cf2de2fc2a729bff5d5.The independent producer review and native SDK/adapter verification are recorded on the PR. Issue 170 remains open: completion still requires publishing the producer revision, updating the MCP/CLI/Platform module pins, and proving registered MCP rejection, CLI survivor diagnostics, and real PostgreSQL survivor persistence without authoritative eviction. Those consumer tasks remain tracked by this issue; the producer PR does not complete them.
fix(aws): propagate incomplete savings plans collections (#172)
cristim commentedon Oct 3, 2026
Producer PR #172 was merged at
9962786e06951a20d58980954e8024fbf49e2d0cafter both PR workflows passed on reviewed head16ed7055b786ba240ff44cf2de2fc2a729bff5d5.Remote commit metadata confirms the merge tree is
0e7b56127d44f0080a2531da07bfc5f844fdebd1, exactly the independently reviewed tree. The full independent verdict records native SDK/adapter tests, baseline failures with unchanged tests, successful controls, survivor-loss/double-count mutation checks, and full AWS race/build/lint verification. Linux PR CI also passed unit/integration tests, standalone module builds, lint and security checks. Main-branch pre-commit run 37152844336 passed. Main-branch CI run 37152844314 remains in progress under an active 60-second watcher; its result is not yet claimed.This verifies the producer portion only. Issue 170 remains open for Tasks B-D: published AWS module pins and registered MCP rejection, CLI survivor diagnostics, and PostgreSQL persistence without authoritative eviction. No live-cloud or consumer rollout verification is claimed here.
fix(recommendations): reject incomplete savings plans menus (#38)
cristim commentedon Oct 3, 2026
MCP rollout is merged in cloud-commitments-mcp PR #38 at
86a698e6b53ce8532eba5e7ebf740da5445fcf0f. GitHub commit metadata confirms tree14bf25ecb9b57223d1aa5578cc0332c5e4c718f6, exactly the reviewed tree at headf39c118b54d8b313691c6ca0f0888a5d35e43864.The MCP shipping manifest pins published AWS
v0.0.0-20261003204812-9962786e0695, with no replacements andGOWORK=offverification. The full fresh gpt-6-astra verdict found no actionable findings and independently reproduced all 78 registered MCP protocol cases, the full native race-short suite across three packages, build and pinned lint. The prior published module fails the new behavioral assertions while valid and empty controls pass. An independent diagnostic-loss mutation fails the intended MCP error assertion while its controls pass.PR pre-commit CI and build/test CI passed on the reviewed head. Post-merge pre-commit and build/test both passed on merge
86a698e6b53ce8532eba5e7ebf740da5445fcf0f. Both existing watchers completed successfully and were reaped.The scenario evidence uses bounded AWS HTTP fixtures through the actual registered MCP consumer and real AWS adapter. It does not claim live AWS data or purchases. Issue #170 remains open for CLI and Platform rollout, including CLI survivor diagnostics and real PostgreSQL survivor persistence without authoritative eviction.
cristim commentedon Oct 3, 2026
CLI rollout merged normally: LeanerCloud/cloud-commitments-cli#2130, merge commit 652fc94af4593273946ad8d5eafc2ad430496f1e, reviewed head 57aa3996983d36f1a592460a3350b9fee348f7cf. It consumes published AWS provider v0.0.0-20261003204812-9962786e0695 (origin 9962786). Independent exact-commit Astra review reproduced all 50 actual child-command cases with race, full race-short tests (two preexisting credential-dependent integration skips), build/vet/lint, old-pin baseline with 35 passing controls and 15 intended SP failures, and warning-suppression mutation failure. Full verdict: LeanerCloud/cloud-commitments-cli#2130 (comment). This is synthetic SDK/TLS fixture evidence through the real CLI, not live AWS or purchase verification; no Docker, Windows or deployment proof is claimed. PR CI and pre-commit passed before normal merge. Exact merge-SHA post-merge CI passed: Build & Test run 37157847569 (all eight jobs, including CI Success) and pre-commit run 37157847439 completed successfully at 652fc94af4593273946ad8d5eafc2ad430496f1e. Both watcher processes are terminal and reaped. Keep this issue open: Platform persistence/authoritative-eviction rollout remains pending.
cristim commentedon Oct 5, 2026
Platform rollout merged in PR #490 as
ed44785a9dd8d1daf36c91f021a5a8e35d0f4c09.The remote merge tree
d314013ef523f7d0a96248271956d15fe311d188matches the independently reviewed and locally verified candidate. All 37 connected PostgreSQL checks passed: 24 collection-completeness scenarios, 11 persisted global-policy scenarios, and two parent tests. A separate cold reviewer reproduced the old-SDK failures, fixed results, and diagnostic regression mutation. Evidence is attached to PR #490, comments 5986784635 and 5986786434.Fresh GitHub checks confirm the producer #172, MCP #38, CLI #2130 and Platform #490 are merged. This completes the implementation rollout for all four consumers of this issue. Post-merge Platform CI and deployment verification remain running; no aggregate deployment success is claimed. The pre-existing Azure per-process readiness failure is tracked separately in Platform #488.
Verification used synthetic AWS HTTP fixtures, the published SDK and real local PostgreSQL. It does not claim live-cloud or purchase verification.
cristim commentedon Oct 5, 2026
All four rollout PRs are merged: Go #172, MCP #38, CLI #2130, Platform #490. Platform merge ed44785a has the independently verified tree d314013e. All six post-merge runs succeeded, including CI, pre-commit, frontend sentinel and AWS/GCP/Azure deployment smoke checks. Exact-head cold review, parent/fixed/mutation proofs and 37 connected PostgreSQL checks are recorded on Platform #490. Verification uses synthetic cloud fixtures; live purchase verification is outside this issue. The separate intermittent Azure readiness defect remains tracked in Platform #488.