Skip to content

fix(providers/aws): fail loud on unparseable RI cost fields - #1236

Merged
cristim merged 1 commit into
mainfrom
fix/cor-07-fix
Jul 17, 2026
Merged

fix(providers/aws): fail loud on unparseable RI cost fields#1236
cristim merged 1 commit into
mainfrom
fix/cor-07-fix

Conversation

@cristim

@cristim cristim commented Jun 11, 2026

Copy link
Copy Markdown
Member

Problem

parseAWSCostDetails in providers/aws/recommendations/parser_ri.go silently swallowed strconv.ParseFloat errors for UpfrontCost, EstimatedMonthlyOnDemandCost, and RecurringStandardMonthlyCost, leaving the destination field at 0 with no log. An unparseable UpfrontCost would surface an all-upfront RI recommendation showing $0 upfront, a wrong money figure feeding effective-savings math and purchase decisions, with zero signal. This was inconsistent with parseCostInformation in the same file, which fails loud for the savings fields (COR-07 in docs/reviews/codebase-review-2026-06-10.md).

Fix

  • parseAWSCostDetails now returns an error when a present cost string is unparseable, matching the parseCostInformation pattern; absent (nil) fields keep their existing behavior.
  • parseRecommendationDetail propagates the error with context; the existing caller chain (parseRecommendations) logs a warning and skips the affected recommendation detail, so a malformed CE response drops that detail loudly instead of fabricating zeros into money math.

Test evidence

  • New regression test TestParseRecommendationDetail_MalformedCostFields covers all three malformed fields plus a well-formed sanity case. Confirmed FAILING with the pre-fix code (stashed fix: 3 subtests fail with "An error is expected but got nil") and passing after.
  • go test ./recommendations/ in the providers/aws module: 335 passed.
  • go build ./... clean in both the root module and providers/aws.

Closes #1171

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/medium Moderate harm urgency/eventually No deadline impact/few Limited audience effort/xs Trivial / one-liner type/bug Defect labels Jun 11, 2026
@cristim

cristim commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/medium Moderate harm urgency/eventually No deadline impact/few Limited audience effort/xs Trivial / one-liner type/bug Defect labels Jun 11, 2026
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor
✅ 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

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: aa3ae7cb-683d-44ba-9558-12e27e52c2b6

📥 Commits

Reviewing files that changed from the base of the PR and between f692085 and 23d6415.

📒 Files selected for processing (2)
  • providers/aws/recommendations/parser_ri.go
  • providers/aws/recommendations/parser_ri_test.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/cor-07-fix

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

@cristim

cristim commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor
✅ 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

cristim commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

Adversarial review

The in-scope fix is correct and complete:

  • parseAWSCostDetails now returns an error on present-but-unparseable UpfrontCost / EstimatedMonthlyOnDemandCost / RecurringStandardMonthlyCost, matching parseCostInformation's already-loud handling of EstimatedMonthlySavingsAmount / EstimatedMonthlySavingsPercentage.
  • The caller chain (parseRecommendationDetail -> parseRecommendations) propagates with context and skips the affected detail with a warning, so a malformed CE response drops loudly instead of fabricating $0 into money math.
  • Tests assert the invariant they guard (three malformed-field subtests confirmed failing pre-fix per the PR description, plus a well-formed sanity case so failures are attributable to the mutated field).
  • RecurringMonthlyCost is *float64 in pkg/common.Recommendation so the nil-vs-zero distinction is preserved on the recurring side. CommitmentCost / OnDemandCost are bare float64 but with the new fail-loud contract the consumer never sees an ambiguous "0 from parse failure" anymore.

Out-of-scope siblings filed as follow-ups

The same silent-fallback shape still exists in three sibling parsers that were intentionally left out of this PR's RI scope but are equally money-affecting:

CI state (UNSTABLE) -- pre-existing, not caused by this PR

The three failing jobs all hit code this PR did not touch:

  • Lint Code (errcheck) -- failures in cmd/configure_azure.go, cmd/configure_gcp.go, internal/api/handler.go, internal/auth/service_*.go, etc. Covered by PR fix(lint): clear golangci-lint debt in files untouched by open PRs #1276 (leftover lint cleanup).
  • Integration Tests -- TestPostgresStore_UpsertRecommendations_AccountScopedEviction, TestPostgresStore_UpsertRecommendations_AmbientAndRegisteredCoexist, TestPostgresStore_PurchaseExecutions/Get_execution_by_ID_-_not_found in internal/config. Schema / migration concerns, not money parsing.
  • Security Scanning -- JS-YAML quadratic-DoS advisory (GHSA-h67p-54hq-rp68) in NPM deps, not Go code.

These are tracked / fixable elsewhere; they do not block COR-07.

Minor (non-blocking)

  • parser_ri.go:25 uses fmt.Printf("Warning: ...") while the rest of the file uses log.Printf("WARNING: ..."). Cosmetic, pre-existing — not changed here per "don't touch what you weren't asked to touch".

@cristim

cristim commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor
✅ 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

cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor
✅ 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

cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Merge-prep: rebased onto origin/main (27fdb06). Gates: go build exit 0, go vet exit 0, gocyclo -over 10 exit 0, golangci-lint run exit 0 (root module), go test ./recommendations/... exit 0 (1.41s). Branch force-pushed; @coderabbitai review triggered.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@cristim Thanks for the merge-prep validation. I’ll review the rebased branch.

✅ 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

cristim commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Rebased onto origin/main (89658a3 -> a945344) which includes #1437 (tflint plugin cache + retry) and #1438 (trivy \*\*/.terraform skip). Workflow files confirmed to match main. Gates: go build/vet exit 0, gocyclo -over 10 exit 0 (no cycles), golangci-lint ./recommendations/... exit 0 (0 issues in touched file), go test ./recommendations/... ok. Pre-commit should be fully green.

parseAWSCostDetails silently swallowed strconv.ParseFloat errors for
UpfrontCost, EstimatedMonthlyOnDemandCost, and
RecurringStandardMonthlyCost, leaving the destination at 0. An
unparseable UpfrontCost would surface an all-upfront RI recommendation
showing $0 upfront, a wrong money figure feeding effective-savings math
and purchase decisions, with no signal.

Make a present-but-unparseable cost string a hard error, consistent
with parseCostInformation in the same file. The caller chain already
handles this: parseRecommendationDetail propagates the error and
parseRecommendations logs a warning and skips the recommendation, so a
malformed CE response drops that detail loudly instead of fabricating
zeros.

Regression test covers all three malformed fields and was confirmed
failing against the pre-fix code.

Closes #1171
@cristim
cristim merged commit bb9424b into main Jul 17, 2026
19 checks passed
@cristim
cristim deleted the fix/cor-07-fix branch July 27, 2026 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/few Limited audience priority/p3 Polish / idea / may never ship severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

COR-07: AWS RI parser silently zeroes unparseable upfront/on-demand/recurring cost fields

1 participant