fix(exchange): fail closed on unparseable PaymentDue in daily-cap check - #1229
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Rebased onto main (includes #1437 tflint cache + #1438 trivy skip). The previous Security Scanning CI failure was on the pre-rebase commit; gosec v2.27.1 now passes on all 6 modules (root, pkg, providers/{aws,azure,gcp}, tests/e2e) with 0 issues. Gate results: go build/vet exit 0 (root + pkg), gocyclo on pkg/exchange exit 0, go test ./exchange/... 103 passed, gosec all modules exit 0. |
The auto-exchange daily-cap check warned and counted $0 toward the MaxPaymentDailyUSD guardrail when paymentDueStr failed to parse, in contrast to the dailySpent parse failure five lines above which aborts with a failed record. A silent $0 coercion on a money path undercounts the daily spend cap if any future caller produces a non-decimal value. Make the parse failure abort the exchange and persist a failed record, mirroring the dailySpent branch. The documented nil-means-zero-cost quote case stays separate: processRecommendation still maps a nil PaymentDueUSD to the explicit "0" string before the parse. Regression test exercises processAutoExchange with unparseable and empty PaymentDue values; confirmed failing pre-fix (exchange executed with $0 counted) and passing post-fix (aborted, failed record saved, Execute never called). Closes #1166
Problem
COR-04 (docs/reviews/codebase-review-2026-06-10.md): in
processAutoExchange(pkg/exchange/auto.go), aPaymentDueparse failure only logged a warning and coerced the value to $0 before adding it to theMaxPaymentDailyUSDdaily-cap check. This is a silent fallback on a money path and is inconsistent with thedailySpentparse failure five lines above, which aborts the exchange and persists a failed record. Latent today (current callers always produce a parseable decimal), but any future path producing a non-decimalpaymentDueStrwould silently undercount the daily guardrail.Fix
PaymentDueparse failure now mirrors thedailySpentbranch: log at error level, setoutcome.Error, persist a failed record viasaveFailedRecord, and return without executing the exchange.processRecommendationmaps a nilquote.PaymentDueUSDto the literal"0"string before the parse, now with a comment distinguishing it from parse errors.Test evidence
New regression test
TestProcessAutoExchange_UnparseablePaymentDue_FailsClosedexercisesprocessAutoExchangedirectly with"not-a-number"and""(the publicRunAutoExchangepath always builds a parseable string, which is why the bug was latent). It asserts the outcome errors,Executeis never called (new mock call counter), and afailedrecord is persisted.outcome.Errorempty, exchange executed with $0 counted toward the cap.go test ./exchange/frompkg/- 97 passed.go build ./...succeeds in both the root module and thepkg/module.Closes #1166