From d4f91fd2210b43d7e4419a93f64f1560faf7a2da Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Mon, 7 Sep 2026 15:48:31 +0200 Subject: [PATCH] fix: reject convergence options that silently disable inference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Game::ranked` and `Game::scored` validated `p_draw` and `score_sigma` but never `convergence`. `ConvergenceOptions` has public fields and `GameOptions` carries one, so a caller could hand the engine a set that `HistoryBuilder`'s eager asserts never saw. Past that, the only guard was a `debug_assert!`, which is gone in the profile users ship. An `alpha` of zero is the bad case, and it fails silently rather than loudly. Measured in release before the fix: likelihoods: [[Gaussian { pi: 0.0, tau: 0.0 }], [Gaussian { pi: 0.0, tau: 0.0 }]] Every EP update unapplied, every likelihood uninformative, inference returning the priors it was given — and an `OwnedGame` that looks entirely ordinary to the caller. `HistoryBuilder::convergence` already documents exactly this hazard; the `Game` constructors just did not share the check. Adds `ConvergenceOptions::validate`, called by both constructors. Rejects `alpha` outside `(0.0, 1.0]` and negative `epsilon`; NaN fails both comparisons and is rejected too. `tests/validation.rs` states the release-mode guarantee for the whole public surface, not just this hole, and CI already runs the suite in release. Probing the other conditions #18 lists found five of eight already enforced — ties without a draw probability, per-event score sigma, weight/team dimensions, draw-probability range, score-sigma range — so this closes the remaining gap rather than the whole issue. The engine keeps its `debug_assert!`s as invariant documentation. Refs #18 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- src/convergence.rs | 31 ++++++++ src/game.rs | 9 ++- tests/validation.rs | 186 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 224 insertions(+), 2 deletions(-) create mode 100644 tests/validation.rs diff --git a/src/convergence.rs b/src/convergence.rs index 153a5cc..114da2d 100644 --- a/src/convergence.rs +++ b/src/convergence.rs @@ -20,6 +20,37 @@ pub struct ConvergenceOptions { pub alpha: f64, } +impl ConvergenceOptions { + /// Reject values that would make inference silently meaningless. + /// + /// `HistoryBuilder::convergence` asserts these eagerly, but the fields are + /// public and `GameOptions` carries a `ConvergenceOptions` — so a caller + /// can hand `Game::ranked` a set the builder never saw. In release the + /// engine's `debug_assert!`s are gone, and an `alpha` of zero leaves every + /// EP update unapplied: inference returns the priors, with every likelihood + /// uninformative and nothing to indicate anything went wrong. + /// + /// # Errors + /// + /// `InvalidParameter` if `alpha` is outside `(0.0, 1.0]` or `epsilon` is + /// negative. NaN fails both comparisons and is rejected. + pub(crate) fn validate(&self) -> Result<(), crate::InferenceError> { + if !(self.alpha > 0.0 && self.alpha <= 1.0) { + return Err(crate::InferenceError::InvalidParameter { + name: "alpha", + value: self.alpha, + }); + } + if self.epsilon.is_nan() || self.epsilon < 0.0 { + return Err(crate::InferenceError::InvalidParameter { + name: "epsilon", + value: self.epsilon, + }); + } + Ok(()) + } +} + impl Default for ConvergenceOptions { fn default() -> Self { Self { diff --git a/src/game.rs b/src/game.rs index 3c7ca99..1dbdd17 100644 --- a/src/game.rs +++ b/src/game.rs @@ -433,6 +433,9 @@ impl<'a, T: Time, D: Drift> Game<'a, T, D> { impl> Game<'_, T, D> { /// # Errors /// + /// - `InvalidParameter` if `options.convergence` is out of range — an + /// `alpha` of zero would leave every EP update unapplied and silently + /// return the priors. /// - `InvalidProbability` if `options.p_draw` is outside `[0.0, 1.0)`. /// - `MismatchedShape` if the outcome's rank count differs from `teams.len()`. /// - `WrongOutcomeKind` if `outcome` is not `Outcome::Ranked`. @@ -444,6 +447,7 @@ impl> Game<'_, T, D> { outcome: crate::Outcome, options: &GameOptions, ) -> Result, crate::InferenceError> { + options.convergence.validate()?; if !(0.0..1.0).contains(&options.p_draw) { return Err(crate::InferenceError::InvalidProbability { value: options.p_draw, @@ -491,8 +495,8 @@ impl> Game<'_, T, D> { /// # Errors /// - /// - `InvalidParameter` if `options.score_sigma` is not strictly positive, - /// or is NaN. + /// - `InvalidParameter` if `options.score_sigma` is not strictly positive + /// or is NaN, or if `options.convergence` is out of range. /// - `MismatchedShape` if the outcome's score count differs from `teams.len()`. /// - `WrongOutcomeKind` if `outcome` is not `Outcome::Scored`. pub fn scored( @@ -500,6 +504,7 @@ impl> Game<'_, T, D> { outcome: crate::Outcome, options: &GameOptions, ) -> Result, crate::InferenceError> { + options.convergence.validate()?; if options.score_sigma <= 0.0 || options.score_sigma.is_nan() { return Err(crate::InferenceError::InvalidParameter { name: "score_sigma", diff --git a/tests/validation.rs b/tests/validation.rs new file mode 100644 index 0000000..2bb9cea --- /dev/null +++ b/tests/validation.rs @@ -0,0 +1,186 @@ +//! Input validation must hold in **release**, where `debug_assert!` is gone. +//! +//! The engine guards itself with `debug_assert!`, which documents invariants +//! but vanishes in the profile users actually ship. Anything reachable from the +//! public API has to be rejected with an `InferenceError` instead, at the +//! boundary, rather than becoming NaN or an out-of-bounds panic deep inside +//! `run_chain`. +//! +//! `GameOptions` and `ConvergenceOptions` both have public fields, so the +//! eager asserts on `HistoryBuilder` do not cover the `Game` constructors — +//! a caller can build the options struct directly. + +use smallvec::smallvec; +use trueskill_tt::{ + ConstantDrift, ConvergenceOptions, Event, Game, GameOptions, Gaussian, History, InferenceError, + Member, Outcome, Rating, Team, +}; + +type R = Rating; + +fn rating() -> R { + R::new( + Gaussian::from_ms(25.0, 25.0 / 3.0), + 25.0 / 6.0, + ConstantDrift(0.0), + ) +} + +fn options_with_alpha(alpha: f64) -> GameOptions { + GameOptions { + convergence: ConvergenceOptions { + alpha, + ..ConvergenceOptions::default() + }, + ..GameOptions::default() + } +} + +/// `alpha == 0.0` leaves every EP update unapplied, so inference silently +/// returns the priors — the worst possible failure, since the output looks +/// entirely reasonable. +#[test] +fn ranked_rejects_a_zero_damping_factor() { + let (a, b) = (rating(), rating()); + let err = Game::::ranked( + &[&[a], &[b]], + Outcome::winner(0, 2), + &options_with_alpha(0.0), + ) + .expect_err("alpha = 0 must be rejected"); + assert!( + matches!(err, InferenceError::InvalidParameter { name: "alpha", .. }), + "got {err:?}" + ); +} + +#[test] +fn ranked_rejects_an_out_of_range_damping_factor() { + let (a, b) = (rating(), rating()); + for alpha in [-0.5, 1.5, f64::NAN] { + let err = Game::::ranked( + &[&[a], &[b]], + Outcome::winner(0, 2), + &options_with_alpha(alpha), + ) + .expect_err("alpha out of (0, 1] must be rejected"); + assert!( + matches!(err, InferenceError::InvalidParameter { name: "alpha", .. }), + "alpha={alpha}: got {err:?}" + ); + } +} + +#[test] +fn scored_rejects_a_bad_damping_factor() { + let (a, b) = (rating(), rating()); + let err = Game::::scored( + &[&[a], &[b]], + Outcome::scores([21.0, 9.0]), + &options_with_alpha(0.0), + ) + .expect_err("alpha = 0 must be rejected"); + assert!( + matches!(err, InferenceError::InvalidParameter { name: "alpha", .. }), + "got {err:?}" + ); +} + +/// Already covered by `Game::ranked`, asserted here so the release-mode +/// guarantee is stated in one place. +#[test] +fn ranked_rejects_an_out_of_range_draw_probability() { + let (a, b) = (rating(), rating()); + for p_draw in [-0.5, 1.0, 1.5] { + let options = GameOptions { + p_draw, + ..GameOptions::default() + }; + assert!( + Game::::ranked(&[&[a], &[b]], Outcome::winner(0, 2), &options).is_err(), + "p_draw={p_draw} must be rejected" + ); + } +} + +#[test] +fn scored_rejects_a_non_positive_noise() { + let (a, b) = (rating(), rating()); + for score_sigma in [0.0, -1.0, f64::NAN] { + let options = GameOptions { + score_sigma, + ..GameOptions::default() + }; + assert!( + Game::::scored(&[&[a], &[b]], Outcome::scores([21.0, 9.0]), &options).is_err(), + "score_sigma={score_sigma} must be rejected" + ); + } +} + +/// A tie with no draw probability makes the truncation margin zero and the +/// two-sided update evaluate 0/0. Ingestion must refuse it. +#[test] +fn ingestion_rejects_a_tie_without_a_draw_probability() { + let mut h = History::builder().p_draw(0.0).build(); + let err = h + .add_events(vec![Event { + time: 0, + teams: smallvec![ + Team::with_members([Member::new("a")]), + Team::with_members([Member::new("b")]), + ], + outcome: Outcome::draw(2), + }]) + .expect_err("a tie with p_draw = 0 must be rejected"); + assert!( + matches!(err, InferenceError::TieWithoutDrawProbability { .. }), + "got {err:?}" + ); +} + +/// `Outcome::scores_with_sigma` documents that a non-positive sigma is +/// accepted at construction and rejected at ingestion. +#[test] +fn ingestion_rejects_a_non_positive_per_event_score_sigma() { + for sigma in [0.0, -1.0, f64::NAN] { + let mut h = History::builder().build(); + let err = h + .add_events(vec![Event { + time: 0, + teams: smallvec![ + Team::with_members([Member::new("a")]), + Team::with_members([Member::new("b")]), + ], + outcome: Outcome::scores_with_sigma([21.0, 9.0], sigma), + }]) + .expect_err("a non-positive per-event sigma must be rejected"); + assert!( + matches!(err, InferenceError::InvalidParameter { .. }), + "sigma={sigma}: got {err:?}" + ); + } +} + +/// Per-team weights must match that team's membership. The top-level length +/// checks in ingestion do not cover the inner dimension. +#[test] +fn ingestion_rejects_weights_that_do_not_match_their_team() { + let mut h = History::builder().build(); + let mut team = Team::with_members([Member::new("a"), Member::new("b")]); + team.members[0].weight = 1.0; + + let err = h + .event(0) + .team(["a", "b"]) + .team(["c"]) + // Three weights for a two-member team. + .weights([1.0, 1.0, 1.0]) + .winner(0) + .commit() + .expect_err("a weight/member length mismatch must be rejected"); + assert!( + matches!(err, InferenceError::MismatchedShape { .. }), + "got {err:?}" + ); +}