fix(aws/recommendations): fail the detail on an invalid average-instances or SP utilization by cristim · Pull Request #327 · LeanerCloud/cloud-commitments-go

cristim · 2026-10-09T17:37:29Z

Refs #54 (the typed IncompleteRecommendationsError from #169 is already on main; this closes the remaining silent-zero gap on the same path. The consumer rollout comment on #54 follows after merge.)

What

A present-but-invalid sizing input was turned into 0, which --target-coverage reads as "no signal" and passes the rec through unsized at AWS's full count. IncludesPoolSize (recfilter/filters.go:118) also keeps avg<=0 recs, so a corrupt value bypassed --min-pool-size.

RI AverageNumberOfInstancesUsedPerHour (parser_ri.go) and SP EstimatedAverageUtilization (parser_sp.go) now use parseOptionalFloat: nil stays 0 (genuinely absent); unparsable, non-finite, negative or empty returns an error. The detail is dropped and counted in FailedDetails of IncompleteRecommendationsError.
Comments on parseRIUtilizationSignals and parseOptionalFloatOrWarn now say sizing inputs must error.
The other three parseOptionalFloatOrWarn call sites are display-only and stay: RI AverageUtilization, SP EstimatedSavingsPercentage, SP CurrentAverageHourlyOnDemandSpend.

Consumers

No consumer change. cli (multi_service_helpers.go) and platform (scheduler.go) already handle IncompleteRecommendationsError; mcp fails the search on any error. Platform effect: tolerateIncompleteSweep (scheduler.go:993) leaves the previous row of a pool whose rec was dropped in place, so such a row can be stale until the next clean sweep.

Verification (fixture-based, CE-shaped details, not live AWS)

RI via parseRecommendations and SP via parseSavingsPlansRecommendations: "abc", "NaN", "-1", "Inf", "" each drop one detail with FailedDetails=1 and keep the valid sibling; nil stays kept with no signal. Updated the existing unit cases at parser_ri_test.go and parser_sp_test.go.
Mutation: reverting either site to parseOptionalFloatOrWarn fails the named tests (RI: TestParseRIUtilizationSignals, TestParseRecommendations_InvalidAverageInstancesIsReportedNotZeroed; SP: TestParseSavingsPlanDetail_RecommendedUtilization, TestParseSavingsPlansRecommendations_InvalidEstimatedUtilizationIsReported).
go test ./providers/aws/... passes with go.work and GOWORK=off; golangci-lint 0 issues.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Recommendations with invalid Reserved Instance or Savings Plans utilization data are now skipped or reported as incomplete rather than using misleading default values. Missing utilization values continue to be accepted, and valid recommendations are preserved.

…nces or SP utilization A present-but-invalid AverageNumberOfInstancesUsedPerHour (RI) or EstimatedAverageUtilization (SP) was zeroed, which --target-coverage reads as no signal and passes the rec through unsized; --min-pool-size also keeps avg<=0 recs. These sizing inputs now error like the money fields, so the detail is dropped and reported through IncompleteRecommendationsError. Display-only fields still degrade to 0 with a warning. Refs #54 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-09T17:37:49Z

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
Review profile: CHILL
Plan: Essentials
Run ID: 9eca2401-79b0-411d-a28c-404657757045

📥 Commits

Reviewing files that changed from the base of the PR and between ed44ecf and 66fe9bd.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
Review profile: CHILL
Plan: Essentials
Run ID: 6ed401b3-2fb1-4582-b3cd-59b381ebdf2e

📥 Commits

Reviewing files that changed from the base of the PR and between bbbd18e and ed44ecf.

📒 Files selected for processing (4)

providers/aws/recommendations/parser_ri.go
providers/aws/recommendations/parser_ri_test.go
providers/aws/recommendations/parser_sp.go
providers/aws/recommendations/parser_sp_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

RI and Savings Plans parsers now return errors for invalid present sizing values. Absent values remain zero. Invalid details are skipped while valid sibling recommendations are retained. RI display-only utilization continues to use warning-and-zero handling.

Changes

Recommendation utilization parsing

Layer / File(s)	Summary
RI utilization parsing `providers/aws/recommendations/parser_ri.go`, `providers/aws/recommendations/parser_ri_test.go`	RI parsing returns an error for invalid present average-instance values and propagates it to recommendation parsing. Tests cover invalid and non-finite values, absent values, and retention of valid sibling recommendations. Invalid `AverageUtilization` values still degrade to zero.
Savings Plans utilization parsing `providers/aws/recommendations/parser_sp.go`, `providers/aws/recommendations/parser_sp_test.go`	Savings Plans parsing returns an error for invalid present `EstimatedAverageUtilization` values. Absent values remain zero. Tests check failed details and retention of valid recommendations.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to ed44e

No actionable issue is identified; the change is ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly identifies the AWS recommendations change and specifies that invalid RI average-instance and Savings Plans utilization values now fail the detail.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

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

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

cristim · 2026-10-10T02:08:27Z

Interim gate note (go gate-2): head is now 66fe9bd after update-branch merges for #313, #328 and #329. The PR-owned diff is byte-identical to the reviewed ed44ecf. Local providers/aws and pkg tests at 66fe9bd pass with go.work and with GOWORK=off. Fails-before and mutation evidence were run at ef65bdc. Parent logic with the signature shimmed fails all 4 named tests and my end-to-end SP probe through GetRecommendations. Reverting the RI site to OrWarn fails 2 tests; reverting the SP site fails 3. In the probe, "abc", "NaN", "-1" and "" each give FailedDetails=1 and keep the valid sibling; nil, "0" and "87.5" pass. Testing parseRecommendations directly is adequate: propagation through GetRecommendations goes via the existing addFailure path, which the probe exercises. Waiting for fresh CI.

cristim · 2026-10-10T02:30:02Z

Gate verdict (go gate-2): CLEAN at 66fe9bd. The PR-owned diff is byte-identical to the reviewed ed44ecf. CI was 14/14 green and the PR was CLEAN. Local tests passed with go.work and with GOWORK=off. Fails-before and mutation evidence are in the interim note above. Merged with --match-head-commit as d9ead42.

cristim added triaged Item has been triaged urgency/this-sprint Within the current sprint priority/p1 Next up; this sprint severity/high Significant harm impact/many Affects most users effort/m Days type/bug Defect labels Oct 9, 2026

cristim added 2 commits October 9, 2026 19:42

Merge branch 'main' into fix/54-parse-recommendations-no-silent-drop

07538f7

Merge branch 'main' into fix/54-parse-recommendations-no-silent-drop

ef65bdc

cristim mentioned this pull request Oct 10, 2026

fix(insurance): keep the API key out of every field fmt can print #331

Merged

Merge branch 'main' into fix/54-parse-recommendations-no-silent-drop

66fe9bd

cristim merged commit d9ead42 into main Oct 10, 2026
15 checks passed

cristim mentioned this pull request Oct 10, 2026

fix(aws/recommendations): parseRecommendations silently drops unparseable offers, so a search can present an incomplete menu as complete #54

Closed

Navigation Menu

Repository navigation

fix(aws/recommendations): fail the detail on an invalid average-instances or SP utilization - #327

fix(aws/recommendations): fail the detail on an invalid average-instances or SP utilization#327
cristim merged 4 commits into
mainfrom
fix/54-parse-recommendations-no-silent-drop

cristim commented Oct 9, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 9, 2026 •

edited

Loading

Review skipped

Walkthrough

Changes

Uh oh!

cristim commented Oct 10, 2026

Uh oh!

Uh oh!

cristim commented Oct 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

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

Uh oh!

What

Consumers

Verification (fixture-based, CE-shaped details, not live AWS)

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Review skipped

Walkthrough

Changes

Uh oh!

cristim commented Oct 10, 2026

Uh oh!

Uh oh!

cristim commented Oct 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristim commented Oct 9, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 9, 2026 •

edited

Loading