fix(providers): correct Azure resourceType filters + fail-loud SP money parsing + nil covered-cost (adversarial-review)#1455
Open
cristim wants to merge 1 commit into
Open
Conversation
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
📝 WalkthroughWalkthroughAWS Savings Plans parsing now rejects invalid money fields while warning and defaulting invalid non-money fields. Azure recommendation filters use canonical resource types, zero commitment costs remain unset, and Search returns empty recommendations. ChangesAWS Savings Plans parsing
Azure recommendation corrections
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
…ey parsing + nil covered-cost (adversarial-review) H1: Azure Search GetRecommendations now returns empty immediately; "AzureSearch" is not a valid Consumption API resourceType. Pre-fix queried with scope-only filter, causing Azure to return VirtualMachine recs mislabeled as Search recs. H2: Replace three hand-written Azure Consumption API resourceType strings with package-level typed constants: - Synapse: "SQLDatabaseDTU" -> reservationResourceTypeSynapse = "SqlDataWarehouse" - SQL DB: "SqlDatabase" -> reservationResourceTypeSQLDB = "SQLDatabases" - CosmosDB:"CosmosDb" -> reservationResourceTypeCosmosDB = "CosmosDB" All three pre-fix strings are invalid enum members; the API returned nothing. M1: ExpandPaymentVariants sets RecurringMonthlyCost to nil (not &0.0) when CommitmentCost is 0, distinguishing "data absent" from "known-zero cost" so the frontend renders "-" instead of fabricating "$0". M2: parseOptionalFloat now returns (float64, error) for money fields (HourlyCommitmentToPurchase, EstimatedMonthlySavingsAmount, UpfrontCost). Present-but-unparseable money fields propagate as errors and drop the recommendation; non-money fields (utilization percentages) still warn+0. Mirrors the RI path (parseAWSCostDetails) which already errored on this class. Extract spPlanTypeDisplayString helper to keep parseSavingsPlanDetail below the gocyclo-10 pre-commit threshold. Regression tests added for all four defects; all fail on pre-fix code and pass on post-fix code.
cristim
force-pushed
the
fix/provider-rec-correctness
branch
from
July 17, 2026 18:26
5898bfb to
ce62470
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/aws/recommendations/parser_sp.go`:
- Line 308: Update the recommendation parsing flow around the OnDemandCost
assignment to use the error-propagating monetary parser for
CurrentAverageHourlyOnDemandSpend, so malformed values reject or drop the
recommendation instead of becoming zero. Preserve the monthly conversion and add
this field to the malformed-money regression table.
- Around line 199-203: Update parseOptionalFloat to reject non-finite parsed
values by checking math.IsNaN(val) and math.IsInf(val, 0) after
strconv.ParseFloat succeeds. Return the existing parse error path for NaN and
infinities, while preserving the warn-and-zero behavior for non-money fields.
In `@providers/azure/services/search/client_test.go`:
- Around line 448-489: Strengthen
TestSearchClient_GetRecommendations_AlwaysEmpty to verify immediate return by
asserting pagerWithVMRecs.index remains unchanged after GetRecommendations.
Alternatively, configure MockRecommendationsPager to fail if pagination is
invoked, while preserving the existing empty-result and no-error assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4bff841-9642-4c03-80be-6d602b53ecbe
📒 Files selected for processing (13)
providers/aws/recommendations/parser_sp.goproviders/aws/recommendations/parser_sp_additional_test.goproviders/aws/recommendations/parser_sp_test.goproviders/azure/internal/recommendations/converter.goproviders/azure/internal/recommendations/converter_test.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/cosmosdb/client_test.goproviders/azure/services/database/client.goproviders/azure/services/database/client_test.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
Comment on lines
199
to
+203
| val, err := strconv.ParseFloat(*s, 64) | ||
| if err != nil { | ||
| log.Printf("WARNING: failed to parse %s: %v", field, err) | ||
| return 0, fmt.Errorf("failed to parse %s %q: %w", field, *s, err) | ||
| } | ||
| return val, nil |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the Go strconv.ParseFloat documentation, do "NaN", "+Inf", and "-Inf" return a nil error?
💡 Result:
Yes, according to the Go strconv.ParseFloat documentation, the strings "NaN", "Inf", "Infinity", "+Inf", "+Infinity", "-Inf", and "-Infinity" are recognized as special floating-point values and return a nil error [1][2][3]. The Go documentation explicitly states that ParseFloat recognizes these strings as their respective special floating-point values [1][2][3]. When these valid strings are provided, the function successfully returns the corresponding float64 value and a nil error [1][2]. This is distinct from cases where a string is syntactically well-formed but represents a value exceeding the representable range of the target bitSize, which would return a non-nil error (ErrRange) along with the resulting ±Inf value [1][4][3].
Citations:
- 1: https://pkg.go.dev/strconv
- 2: https://go.dev/pkg/strconv/?m=old
- 3: https://pkg.go.dev/internal/strconv
- 4: https://github.com/golang/go/blob/go1.26.4/src/strconv/number.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant parser and nearby helpers.
file="providers/aws/recommendations/parser_sp.go"
wc -l "$file"
sed -n '150,260p' "$file"
# Find the helper and its call sites.
rg -n "parseOptionalFloatOrWarn|ParseFloat|IsNaN|IsInf|money" providers/aws/recommendations -nRepository: LeanerCloud/CUDly
Length of output: 9018
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '260,340p' providers/aws/recommendations/parser_sp.go
sed -n '492,520p' providers/aws/recommendations/sp_coverage.go
sed -n '1,260p' providers/aws/recommendations/parser_sp_test.go | sed -n '300,380p'Repository: LeanerCloud/CUDly
Length of output: 5653
Reject non-finite numeric values in parseOptionalFloat. strconv.ParseFloat accepts NaN and +/-Inf here, so malformed SP money fields can still flow through as valid recommendations. Reject math.IsNaN(val) and math.IsInf(val, 0) so only non-money fields keep the warn-and-zero path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@providers/aws/recommendations/parser_sp.go` around lines 199 - 203, Update
parseOptionalFloat to reject non-finite parsed values by checking
math.IsNaN(val) and math.IsInf(val, 0) after strconv.ParseFloat succeeds. Return
the existing parse error path for NaN and infinities, while preserving the
warn-and-zero behavior for non-money fields.
| // where monthly_cost reflects only the no-upfront recurring charge, not | ||
| // the full on-demand baseline). See #303. | ||
| onDemandCost := parseOptionalFloat("CurrentAverageHourlyOnDemandSpend", detail.CurrentAverageHourlyOnDemandSpend) * hoursPerMonth | ||
| onDemandCost := parseOptionalFloatOrWarn("CurrentAverageHourlyOnDemandSpend", detail.CurrentAverageHourlyOnDemandSpend) * hoursPerMonth |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not fabricate a zero OnDemandCost.
CurrentAverageHourlyOnDemandSpend is a monetary rate used to populate OnDemandCost. Warning and returning zero forwards a corrupt baseline instead of dropping the recommendation, contradicting the money-field handling objective. Use the error-propagating parser and add this field to the malformed-money regression table.
Proposed fix
- onDemandCost := parseOptionalFloatOrWarn("CurrentAverageHourlyOnDemandSpend", detail.CurrentAverageHourlyOnDemandSpend) * hoursPerMonth
+ onDemandHourlySpend, err := parseOptionalFloat(
+ "CurrentAverageHourlyOnDemandSpend",
+ detail.CurrentAverageHourlyOnDemandSpend,
+ )
+ if err != nil {
+ return nil, err
+ }
+ onDemandCost := onDemandHourlySpend * hoursPerMonth📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onDemandCost := parseOptionalFloatOrWarn("CurrentAverageHourlyOnDemandSpend", detail.CurrentAverageHourlyOnDemandSpend) * hoursPerMonth | |
| onDemandHourlySpend, err := parseOptionalFloat( | |
| "CurrentAverageHourlyOnDemandSpend", | |
| detail.CurrentAverageHourlyOnDemandSpend, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| onDemandCost := onDemandHourlySpend * hoursPerMonth |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@providers/aws/recommendations/parser_sp.go` at line 308, Update the
recommendation parsing flow around the OnDemandCost assignment to use the
error-propagating monetary parser for CurrentAverageHourlyOnDemandSpend, so
malformed values reject or drop the recommendation instead of becoming zero.
Preserve the monthly conversion and add this field to the malformed-money
regression table.
Comment on lines
+448
to
+489
| // TestSearchClient_GetRecommendations_AlwaysEmpty is the H1 regression test. | ||
| // | ||
| // "AzureSearch" is not a valid resourceType in the Consumption | ||
| // ReservationRecommendations API (valid list: VirtualMachines, SQLDatabases, | ||
| // CosmosDB, SqlDataWarehouse, etc.). The pre-fix code queried with only a | ||
| // scope filter, causing Azure to return VirtualMachine recommendations that | ||
| // were then mislabeled as Search recommendations. | ||
| // | ||
| // Post-fix, GetRecommendations returns an empty slice immediately regardless | ||
| // of what the pager contains. This test injects a non-empty pager (mimicking | ||
| // the VM recs Azure would return) and asserts the result is empty. | ||
| func TestSearchClient_GetRecommendations_AlwaysEmpty(t *testing.T) { | ||
| ctx := context.Background() | ||
| client := NewClient(nil, "test-subscription", "eastus") | ||
|
|
||
| // Simulate what the Azure Consumption API returns when queried without a | ||
| // resourceType filter: a VirtualMachines recommendation that must NOT be | ||
| // forwarded as a Search recommendation. | ||
| vmRec := mocks.BuildLegacyReservationRecommendation( | ||
| mocks.WithRegion("eastus"), | ||
| mocks.WithTerm("P1Y"), | ||
| mocks.WithQuantity(1), | ||
| mocks.WithNormalizedSize("Standard_D2s_v3"), | ||
| mocks.WithCosts(100, 70, 30), | ||
| ) | ||
| pagerWithVMRecs := &MockRecommendationsPager{ | ||
| pages: []armconsumption.ReservationRecommendationsClientListResponse{ | ||
| { | ||
| ReservationRecommendationsListResult: armconsumption.ReservationRecommendationsListResult{ | ||
| Value: []armconsumption.ReservationRecommendationClassification{vmRec}, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| client.SetRecommendationsPager(pagerWithVMRecs) | ||
|
|
||
| recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) | ||
| require.NoError(t, err) | ||
| assert.Empty(t, recs, | ||
| "Azure Search has no Consumption API reservation stream; GetRecommendations "+ | ||
| "must always return empty regardless of what the injected pager contains") | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Verify that GetRecommendations does not query the pager.
The test proves that VM recommendations are not returned, but it does not enforce the “return immediately” behavior documented at Line [456] through Line [458]. A future implementation could query the pager and merely filter the result while still passing this test. Assert that pagerWithVMRecs.index remains unchanged, or make the mock fail if pagination is invoked.
Suggested assertion
assert.Empty(t, recs,
"Azure Search has no Consumption API reservation stream; GetRecommendations "+
"must always return empty regardless of what the injected pager contains")
+ assert.Equal(t, 0, pagerWithVMRecs.index,
+ "GetRecommendations must not query the recommendations pager")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TestSearchClient_GetRecommendations_AlwaysEmpty is the H1 regression test. | |
| // | |
| // "AzureSearch" is not a valid resourceType in the Consumption | |
| // ReservationRecommendations API (valid list: VirtualMachines, SQLDatabases, | |
| // CosmosDB, SqlDataWarehouse, etc.). The pre-fix code queried with only a | |
| // scope filter, causing Azure to return VirtualMachine recommendations that | |
| // were then mislabeled as Search recommendations. | |
| // | |
| // Post-fix, GetRecommendations returns an empty slice immediately regardless | |
| // of what the pager contains. This test injects a non-empty pager (mimicking | |
| // the VM recs Azure would return) and asserts the result is empty. | |
| func TestSearchClient_GetRecommendations_AlwaysEmpty(t *testing.T) { | |
| ctx := context.Background() | |
| client := NewClient(nil, "test-subscription", "eastus") | |
| // Simulate what the Azure Consumption API returns when queried without a | |
| // resourceType filter: a VirtualMachines recommendation that must NOT be | |
| // forwarded as a Search recommendation. | |
| vmRec := mocks.BuildLegacyReservationRecommendation( | |
| mocks.WithRegion("eastus"), | |
| mocks.WithTerm("P1Y"), | |
| mocks.WithQuantity(1), | |
| mocks.WithNormalizedSize("Standard_D2s_v3"), | |
| mocks.WithCosts(100, 70, 30), | |
| ) | |
| pagerWithVMRecs := &MockRecommendationsPager{ | |
| pages: []armconsumption.ReservationRecommendationsClientListResponse{ | |
| { | |
| ReservationRecommendationsListResult: armconsumption.ReservationRecommendationsListResult{ | |
| Value: []armconsumption.ReservationRecommendationClassification{vmRec}, | |
| }, | |
| }, | |
| }, | |
| } | |
| client.SetRecommendationsPager(pagerWithVMRecs) | |
| recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) | |
| require.NoError(t, err) | |
| assert.Empty(t, recs, | |
| "Azure Search has no Consumption API reservation stream; GetRecommendations "+ | |
| "must always return empty regardless of what the injected pager contains") | |
| } | |
| // TestSearchClient_GetRecommendations_AlwaysEmpty is the H1 regression test. | |
| // | |
| // "AzureSearch" is not a valid resourceType in the Consumption | |
| // ReservationRecommendations API (valid list: VirtualMachines, SQLDatabases, | |
| // CosmosDB, SqlDataWarehouse, etc.). The pre-fix code queried with only a | |
| // scope filter, causing Azure to return VirtualMachine recommendations that | |
| // were then mislabeled as Search recommendations. | |
| // | |
| // Post-fix, GetRecommendations returns an empty slice immediately regardless | |
| // of what the pager contains. This test injects a non-empty pager (mimicking | |
| // the VM recs Azure would return) and asserts the result is empty. | |
| func TestSearchClient_GetRecommendations_AlwaysEmpty(t *testing.T) { | |
| ctx := context.Background() | |
| client := NewClient(nil, "test-subscription", "eastus") | |
| // Simulate what the Azure Consumption API returns when queried without a | |
| // resourceType filter: a VirtualMachines recommendation that must NOT be | |
| // forwarded as a Search recommendation. | |
| vmRec := mocks.BuildLegacyReservationRecommendation( | |
| mocks.WithRegion("eastus"), | |
| mocks.WithTerm("P1Y"), | |
| mocks.WithQuantity(1), | |
| mocks.WithNormalizedSize("Standard_D2s_v3"), | |
| mocks.WithCosts(100, 70, 30), | |
| ) | |
| pagerWithVMRecs := &MockRecommendationsPager{ | |
| pages: []armconsumption.ReservationRecommendationsClientListResponse{ | |
| { | |
| ReservationRecommendationsListResult: armconsumption.ReservationRecommendationsListResult{ | |
| Value: []armconsumption.ReservationRecommendationClassification{vmRec}, | |
| }, | |
| }, | |
| }, | |
| } | |
| client.SetRecommendationsPager(pagerWithVMRecs) | |
| recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) | |
| require.NoError(t, err) | |
| assert.Empty(t, recs, | |
| "Azure Search has no Consumption API reservation stream; GetRecommendations "+ | |
| "must always return empty regardless of what the injected pager contains") | |
| assert.Equal(t, 0, pagerWithVMRecs.index, | |
| "GetRecommendations must not query the recommendations pager") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@providers/azure/services/search/client_test.go` around lines 448 - 489,
Strengthen TestSearchClient_GetRecommendations_AlwaysEmpty to verify immediate
return by asserting pagerWithVMRecs.index remains unchanged after
GetRecommendations. Alternatively, configure MockRecommendationsPager to fail if
pagination is invoked, while preserving the existing empty-result and no-error
assertions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four provider recommendation-correctness defects found by adversarial review of
origin/main. All four are fixed with regression tests that fail on pre-fix code and pass on post-fix code.H1 - Azure Search GetRecommendations returns empty (HIGH)
"AzureSearch"is not a validresourceTypein the Azure ConsumptionReservationRecommendationsAPI (valid list includes VirtualMachines, SQLDatabases, CosmosDB, SqlDataWarehouse, etc.). The pre-fix code queried with only a scope filter (properties/scope eq 'Shared'), causing Azure to default to returning VirtualMachine recommendations that were then mislabeled as Search recommendations.Fix:
GetRecommendationsnow returns empty immediately. Regression testTestSearchClient_GetRecommendations_AlwaysEmptyinjects a pager containing VM recs and asserts empty is returned.H2 - Three wrong Consumption API resourceType filter strings (HIGH)
The pre-fix code used hand-written string literals that are not valid
resourceTypeenum members, causing the Consumption API to return no recommendations for those services:"SQLDatabaseDTU"reservationResourceTypeSynapse = "SqlDataWarehouse""SqlDatabase"reservationResourceTypeSQLDB = "SQLDatabases""CosmosDb"reservationResourceTypeCosmosDB = "CosmosDB"Fix: Package-level typed constants per
feedback_sdk_enum_string_literals. Regression testsTestReservationResourceType*_IsCanonicalpin the exact string values.M1 - nil RecurringMonthlyCost when CommitmentCost is absent (MEDIUM)
ExpandPaymentVariantsunconditionally setRecurringMonthlyCost = &0.0even whenCommitmentCostis 0 (meaning data was absent), fabricating a non-nil pointer the frontend renders as"$0"instead of"-".Fix: Guard added -
RecurringMonthlyCostis nil for both variants whenCommitmentCost == 0. Perfeedback_nullable_not_zero: absent numbers must be nil, not 0. The existing testTestExpandPaymentVariants_ZeroCommitmentCostis updated to assert nil (it previously asserted the wrong behavior).M2 - Fail-loud on unparseable SP money fields (MEDIUM)
parseOptionalFloatcoerced present-but-unparseable money fields (HourlyCommitmentToPurchase,EstimatedMonthlySavingsAmount,UpfrontCost) to 0 with a warning, fabricating$0figures that would reach the scheduler and frontend. The RI path (parseAWSCostDetails) already errored on the same class.Fix: Two-tier treatment:
parseOptionalFloatreturns(float64, error)for money fields;parseOptionalFloatOrWarn(new) logs and returns 0 for non-money fields (percentages, utilization). Bad money fields cause the recommendation to be dropped (logged as WARNING).spPlanTypeDisplayStringhelper extracted to keepparseSavingsPlanDetailbelow the gocyclo-10 pre-commit threshold.Test plan
go build ./...passes inproviders/awsandproviders/azurego vet ./...passes in both modulesgocyclo -over 10 recommendations/clean inproviders/awsproviders/aws/recommendationsproviders/azure/internal/recommendationssearch,synapse,database,cosmosdbSummary by CodeRabbit
$0when recurring cost data is unavailable.