fix: rating service now fails closed on NaN/Inf values#311
Conversation
- Added validatePlayerMetrics() that returns error instead of resetting - Updated sanitizePlayerStats() to return error on invalid values - Fixed unrelated sprintf bug in debatevsbot.go
📝 WalkthroughWalkthroughRemoved an unused format placeholder from the debate opening-statement template and introduced stricter validation and error propagation in the rating service to detect NaN/Inf and invalid RD/volatility, causing early failure instead of silently resetting or correcting player ratings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/rating_service.go (1)
72-100:⚠️ Potential issue | 🟠 Major
sanitizePlayerMetricsclosure silently resets values, makingvalidatePlayerMetricsunreachable for those conditions.The closure on lines 72–94 still performs silent fail-open resets (NaN/Inf → default, RD ≤ 0 → default, Volatility ≤ 0 → default). Since
sanitizePlayerStats(lines 64–69) already catches NaN/Inf and returns an error, the closure is dead code for those cases. However, forRD <= 0(whichsanitizePlayerStatsdoes not check), the closure silently resets it beforevalidatePlayerMetricsever runs, so the validation on line 95–100 can never trigger for that condition. This contradicts the fail-closed objective from issue#298.The closure should be removed. Let
validatePlayerMetricsbe the single validation gate, and add theRD <= 0check tosanitizePlayerStatsif you want to catch it early:Proposed fix
ratingSystem.UpdateMatch(userPlayer, opponentPlayer, outcome, debateTime) if err := sanitizePlayerStats(userPlayer, preUserRating, preUserRD); err != nil { return nil, nil, fmt.Errorf("user stats validation failed: %w", err) } if err := sanitizePlayerStats(opponentPlayer, preOpponentRating, preOpponentRD); err != nil { return nil, nil, fmt.Errorf("opponent stats validation failed: %w", err) } - // Sanitize rating outputs to avoid NaN/Inf values - sanitizePlayerMetrics := func(player *rating.Player) { - initialRating := ratingSystem.Config.InitialRating - initialRD := ratingSystem.Config.InitialRD - initialVol := ratingSystem.Config.InitialVol - - if math.IsNaN(player.Rating) || math.IsInf(player.Rating, 0) { - player.Rating = initialRating - } - if math.IsNaN(player.RD) || math.IsInf(player.RD, 0) { - player.RD = initialRD - } - if math.IsNaN(player.Volatility) || math.IsInf(player.Volatility, 0) { - player.Volatility = initialVol - } - if player.RD <= 0 { - player.RD = initialRD - } - if player.Volatility <= 0 { - player.Volatility = initialVol - } - } - sanitizePlayerMetrics(userPlayer) - sanitizePlayerMetrics(opponentPlayer) if err := validatePlayerMetrics(userPlayer); err != nil { return nil, nil, fmt.Errorf("user Rating calculation error: %w", err) }
🤖 Fix all issues with AI agents
In `@backend/services/rating_service.go`:
- Around line 222-235: sanitizePlayerStats currently declares unused parameters
fallbackRating and fallbackRD; remove these parameters from the
sanitizePlayerStats signature and implementation (keep function name
sanitizePlayerStats and behavior that validates Rating, RD, Volatility, and sets
LastUpdate) and then update every call site that passed the magic numbers (e.g.,
1200.0, 350.0) to call sanitizePlayerStats(player) instead; ensure imports/uses
still compile after the signature change and delete the now-unused variables at
those call sites so no fallback magic numbers remain.
🧹 Nitpick comments (2)
backend/services/rating_service.go (2)
175-194:validatePlayerMetricslargely duplicatessanitizePlayerStats.Both functions check NaN/Inf on Rating, RD, and Volatility, and both check Volatility ≤ 0. The only addition here is
RD <= 0. Consider consolidating into a single validation function to avoid maintaining two nearly identical check lists that can drift apart.
205-220: Defensive re-validation is reasonable, but note the magic numbers.
1200.0and350.0on line 207 are presumablyInitialRatingandInitialRD— but since those parameters go unused, this is doubly confusing. Once the unused params are removed per the earlier suggestion, this call site simplifies naturally.
- Removed fallbackRating and fallbackRD parameters (were unused) - Updated all call sites to use simplified signature - No more magic numbers (1200.0, 350.0) in code
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/rating_service.go (1)
72-94:⚠️ Potential issue | 🔴 CriticalThe old
sanitizePlayerMetricsclosure is fail-open code that directly contradicts the PR objective.This closure silently resets NaN/Inf/non-positive values to initial defaults — exactly the behavior issue
#298describes and this PR is supposed to eliminate. Because it runs aftersanitizePlayerStats(which now correctly errors out on NaN/Inf) but beforevalidatePlayerMetrics, it masks any remaining invalid values (e.g., negative RD), makingvalidatePlayerMetricson lines 95-100 a no-op for those cases.Remove this entire closure and its two call sites. The fail-closed checks are already handled by
sanitizePlayerStats(lines 64-69) andvalidatePlayerMetrics(lines 95-100).Proposed fix
if err := sanitizePlayerStats(opponentPlayer); err != nil { return nil, nil, fmt.Errorf("opponent stats validation failed: %w", err) } - // Sanitize rating outputs to avoid NaN/Inf values - sanitizePlayerMetrics := func(player *rating.Player) { - initialRating := ratingSystem.Config.InitialRating - initialRD := ratingSystem.Config.InitialRD - initialVol := ratingSystem.Config.InitialVol - - if math.IsNaN(player.Rating) || math.IsInf(player.Rating, 0) { - player.Rating = initialRating - } - if math.IsNaN(player.RD) || math.IsInf(player.RD, 0) { - player.RD = initialRD - } - if math.IsNaN(player.Volatility) || math.IsInf(player.Volatility, 0) { - player.Volatility = initialVol - } - if player.RD <= 0 { - player.RD = initialRD - } - if player.Volatility <= 0 { - player.Volatility = initialVol - } - } - sanitizePlayerMetrics(userPlayer) - sanitizePlayerMetrics(opponentPlayer) if err := validatePlayerMetrics(userPlayer); err != nil {
🧹 Nitpick comments (1)
backend/services/rating_service.go (1)
64-69: Redundant triple-validation of the same player data — consider consolidating.Each player's metrics are validated up to three times through the same flow:
sanitizePlayerStats(line 64/67),validatePlayerMetrics(line 95/98), thensanitizePlayerStatsagain inupdateUserRating(line 207). The two functions (sanitizePlayerStatsandvalidatePlayerMetrics) perform nearly identical NaN/Inf checks with slightly different coverage (see RD <= 0 gap above).Consider merging these into a single validation function to reduce duplication and eliminate the risk of the two diverging over time. The
LastUpdatezero-value fix fromsanitizePlayerStatscan remain as a separate concern or a flag.Also applies to: 95-100, 207-209
|
@Zahnentferner please review my pr |
|
@bhavik-mangla please review this pr |
1 participant
Closes #298
Summary by CodeRabbit