From 8e4d6a637da75c9eaed4a5d4b85700769fd0fcc5 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 8 Sep 2026 12:29:14 +0200 Subject: [PATCH 1/3] fix: reject malformed events at the ingestion boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A one-team event reached `run_chain`, which builds one diff link per adjacent pair of teams, leaving it to index `links[1..]` on an empty vector. That panicked with "range start index 1 out of range for slice of length 0" — from `History::add_events`, in a release build, through entirely safe API. An empty team was the quieter half of the same gap. It contributes no performance, so a malformed event converged and handed back a finite, plausible-looking posterior for whoever it was matched against. That is this crate's characteristic defect: a public surface reporting a constant that looks like an answer. A non-finite score was the third. `converge` did report NonFiniteResult, so it was detected — but a caller reading `current_skill` before converging was handed `tau: NaN` with nothing to say so. `NotEnoughTeams` and `EmptyTeam` already existed. They were checked on the prediction paths and nowhere else, which is exactly why ingestion could still manufacture the states they describe. The checks go in `add_events_with_prior` alongside the tie check, for the same reason that one is there: every ingestion route lands on it, so `record_winner`, `record_draw` and `EventBuilder` inherit them rather than each needing their own. Also corrects documentation that had been stating the opposite of the code since 8c087ad in 0.4.0. README.md and the `with_prior` / `with_drift_scale` doc comments all still said competitor configuration was "captured at first appearance" and had "no effect" on a known key. It now applies whenever supplied and refits the whole history. A reader would have concluded late configuration was impossible and built a workaround for a limitation that does not exist. CI compiles README code blocks but not prose, which is why it survived three releases. The comment in tests/degenerate_inputs.rs claiming a one-team event was "rejected for an unrelated reason" was wrong when written — it panicked. Refs #18, #26 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- README.md | 13 +++- src/event.rs | 7 +- src/history.rs | 44 +++++++++++ tests/degenerate_inputs.rs | 5 +- tests/ingestion_shape.rs | 147 +++++++++++++++++++++++++++++++++++++ 5 files changed, 208 insertions(+), 8 deletions(-) create mode 100644 tests/ingestion_shape.rs diff --git a/README.md b/README.md index 1ab442d..6411954 100644 --- a/README.md +++ b/README.md @@ -134,10 +134,15 @@ h.add_events(vec![Event { h.converge().unwrap(); ``` -Like `with_prior`, the scale is **competitor configuration captured at first -appearance** — setting it on a key the history already knows has no effect. It -must be finite and non-negative; ingestion otherwise fails with -`InferenceError::InvalidParameter`. +Like `with_prior`, the scale is **competitor configuration, not a per-event +value**: it applies to the competitor for the whole history, and it applies +whenever it is supplied — including on a key the history already knows. +Configuring one late still refits the whole history rather than taking effect +only from that event onward, because `converge` refits from competitor state. +Repeating the same value is inert; supplying two *different* values for one +competitor within a single batch is `InferenceError::ConflictingCompetitorConfig`, +since events in a batch have no order. The scale must be finite and +non-negative; ingestion otherwise fails with `InferenceError::InvalidParameter`. Note that the fluent `EventBuilder` (`h.event(t).team([...])`) sets weights but not `drift_scale` or `prior`; those need the typed `Event` / `Team` / `Member` diff --git a/src/event.rs b/src/event.rs index d7ed014..341dd54 100644 --- a/src/event.rs +++ b/src/event.rs @@ -88,7 +88,9 @@ impl Member { /// Set this competitor's starting skill estimate. /// - /// Captured at the competitor's first appearance; see the type docs. + /// Competitor configuration, not a per-event value: it applies for the + /// whole history and applies whenever it is supplied, including on a key + /// the history already knows. See the type docs. pub fn with_prior(mut self, prior: Gaussian) -> Self { self.prior = Some(prior); self @@ -104,7 +106,8 @@ impl Member { /// shares a scale with moving competitors but should not itself move: a bot /// at a known strength, a rating floor, a course difficulty. /// - /// Captured at the competitor's first appearance; see the type docs. + /// Applies for the whole history and whenever it is supplied, including on + /// a key the history already knows; see the type docs. /// Must be finite and non-negative, or ingestion fails with /// [`InferenceError::InvalidParameter`](crate::InferenceError::InvalidParameter). pub fn with_drift_scale(mut self, scale: f64) -> Self { diff --git a/src/history.rs b/src/history.rs index 989138a..8046fad 100644 --- a/src/history.rs +++ b/src/history.rs @@ -1505,6 +1505,50 @@ impl, O: Observer, K: Eq + Hash + Clone> History "rank", + EventKind::Scored { .. } => "score", + }; + for value in event_results { + if !value.is_finite() { + return Err(InferenceError::InvalidParameter { + name, + value: *value, + }); + } + } + } + } + // Chokepoint for tie validation: every ingestion route lands here, // including `record_draw`, which builds its results directly rather // than going through `Outcome`. diff --git a/tests/degenerate_inputs.rs b/tests/degenerate_inputs.rs index 3c783b2..01ef560 100644 --- a/tests/degenerate_inputs.rs +++ b/tests/degenerate_inputs.rs @@ -170,8 +170,9 @@ fn event_builder_rejects_a_weights_length_mismatch() { fn event_builder_weights_mismatch_leaves_the_history_untouched() { let mut h = History::default(); - // Two teams, so ingestion would otherwise succeed — a one-team event is - // rejected for an unrelated reason and would pass this vacuously. + // Two teams, so ingestion would otherwise succeed. A one-team event is + // rejected as `NotEnoughTeams` before the weights are ever examined, so + // building this with one team would pass vacuously. let _ = h .event(1) .team(["a"]) diff --git a/tests/ingestion_shape.rs b/tests/ingestion_shape.rs new file mode 100644 index 0000000..7b1c36e --- /dev/null +++ b/tests/ingestion_shape.rs @@ -0,0 +1,147 @@ +//! Malformed events must be rejected at the ingestion boundary. +//! +//! Every case here was reachable from safe public API in a release build. Two +//! of them are the two shapes this crate's defects keep taking: a panic from +//! deep inside inference, and a finite, plausible-looking posterior computed +//! from an event that should never have been accepted. +//! +//! `InferenceError::NotEnoughTeams` and `EmptyTeam` already existed when these +//! were found — they were checked on the prediction paths and nowhere else, so +//! ingestion could still manufacture the states they describe. + +use smallvec::smallvec; +use trueskill_tt::{Event, History, InferenceError, Member, Outcome, Team}; + +type Ev = Event; + +fn history() -> History +{ + History::builder().score_sigma(1.0).build() +} + +fn teams(names: &[&[&'static str]]) -> smallvec::SmallVec<[Team<&'static str>; 4]> { + names + .iter() + .map(|team| Team::with_members(team.iter().map(|k| Member::new(*k)))) + .collect() +} + +/// The regression this file exists for: `run_chain` builds one diff link per +/// adjacent pair of teams, so a one-team event left it indexing `links[1..]` +/// on an empty vector and panicked — in release, from `History::add_events`. +#[test] +fn a_one_team_event_is_an_error_not_a_panic() { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: teams(&[&["a"]]), + outcome: Outcome::winner(0, 1), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::NotEnoughTeams { got: 1 }), + "{err:?}" + ); +} + +#[test] +fn a_zero_team_event_is_an_error() { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: smallvec![], + outcome: Outcome::ranking([]), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::NotEnoughTeams { got: 0 }), + "{err:?}" + ); +} + +/// The quiet half. An empty team contributes no performance, so before this +/// was rejected the event converged and handed back a finite posterior for its +/// opponent — a plausible constant computed from nothing. +#[test] +fn an_empty_team_is_an_error_rather_than_a_free_win() { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: teams(&[&[], &["b"]]), + outcome: Outcome::winner(0, 2), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::EmptyTeam { team: 0 }), + "{err:?}" + ); + // Nothing was recorded, so the history is still empty. + assert!(h.current_skill(&"b").is_none()); +} + +#[test] +fn an_empty_team_is_reported_by_position() { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: teams(&[&["a"], &[]]), + outcome: Outcome::winner(0, 2), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::EmptyTeam { team: 1 }), + "{err:?}" + ); +} + +/// A NaN score used to ingest cleanly. `converge` reported `NonFiniteResult`, +/// but a caller who read `current_skill` first was handed `tau: NaN` with +/// nothing to say so. +#[test] +fn a_non_finite_score_is_rejected_at_ingestion() { + for bad in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: teams(&[&["a"], &["b"]]), + outcome: Outcome::scores([bad, 0.0]), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::InvalidParameter { name: "score", .. }), + "{bad}: {err:?}" + ); + assert!(h.current_skill(&"a").is_none(), "{bad} was recorded anyway"); + } +} + +/// The fluent builder routes through the same chokepoint, so it inherits the +/// checks rather than needing its own. +#[test] +fn the_event_builder_inherits_the_shape_checks() { + let mut h = history(); + let err = h.event(1).team(["a"]).winner(0).commit().unwrap_err(); + assert!( + matches!(err, InferenceError::NotEnoughTeams { got: 1 }), + "{err:?}" + ); +} + +/// A well-formed event is untouched by any of this. +#[test] +fn a_well_formed_event_still_ingests() { + let mut h = history(); + h.add_events(vec![Ev { + time: 1, + teams: teams(&[&["a"], &["b"]]), + outcome: Outcome::scores([3.0, 1.0]), + }]) + .unwrap(); + assert!(h.converge().unwrap().converged); + assert!(h.current_skill(&"a").unwrap().mu() > h.current_skill(&"b").unwrap().mu()); +} From f57784c14187a37795034777a86d0d876bd8e43f Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 8 Sep 2026 12:29:40 +0200 Subject: [PATCH 2/3] docs: record the rayon opt-in deviation in spec section 6 Issue #5 asked for a decision, not an implementation: either flip rayon to default-on, or record why the spec was deviated from and close. Opt-in stands. The measured speedups are 1.0x realistic / 1.3x pathological (#4), so default-on would cost every downstream user a thread pool and a dependency for approximately nothing. The condition the decision was waiting on cannot be met: #5 was blocked on re-measuring after cross-slice dirty-bit skipping landed, and #4 was closed by removing the inert slices_skipped field rather than by implementing it. There is no forthcoming measurement to wait for. Also corrects the spec's own reasoning. It cited an unsafe concurrent write through SkillStore as a cost of going default-on; the crate is forbid(unsafe_code) and the compute/apply split avoids that entirely. The case for opt-in is the measurements, not a safety argument. Closes #5 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- ...-04-23-trueskill-engine-redesign-design.md | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/specs/2026-04-23-trueskill-engine-redesign-design.md b/docs/superpowers/specs/2026-04-23-trueskill-engine-redesign-design.md index 7e0c95d..66685fc 100644 --- a/docs/superpowers/specs/2026-04-23-trueskill-engine-redesign-design.md +++ b/docs/superpowers/specs/2026-04-23-trueskill-engine-redesign-design.md @@ -500,6 +500,26 @@ All public traits (`Time`, `Drift`, `Observer`, `Factor`, `Schedule`) require `S `rayon` as default-on feature; with `default-features = false`, parallel paths fall back to sequential iterators behind `cfg(feature = "rayon")`. +> **Not implemented. Deliberate deviation, decided 2026-09-08 (issue #5).** +> +> `rayon` ships **opt-in**: `Cargo.toml` has no `default = [...]` key. The +> measured speedups are 1.0x on realistic workloads and 1.3x on a pathological +> one (issue #4), because typical slices hold too few events to amortize +> rayon's task-spawn overhead. Default-on would hand every downstream user a +> thread pool and a dependency for approximately no gain. +> +> This section made the trade conditional on cross-slice dirty-bit skipping +> landing and changing the parallel story. It did not land: #4 was closed on +> 2026-08-27 by removing the inert `ConvergenceReport::slices_skipped` field +> rather than by implementing the mechanism, so the re-measurement this was +> waiting on will not arrive. +> +> The "Trade-offs" note below also cited an `unsafe` concurrent-write path +> through `SkillStore` as a cost of default-on. That cost does not exist: the +> crate is `#![forbid(unsafe_code)]`, and the compute/apply split on the +> internal `Event` is what lets a color group run in parallel without it. The +> case for opt-in rests on the measurements alone. + ### Expected speedup ballpark For 1000 players, 60 events/slice × 1000 slices, 30 convergence iterations: @@ -521,7 +541,7 @@ These are pre-implementation estimates. Each tier validates with criterion. - Color-group parallelism requires up-front graph coloring at ingestion. Cost: linear in events, run once per `add_events`. Cheap. - Default = asynchronous EP (preserves current semantics). Synchronous opt-in only. - Cross-slice sweep stays sequential; no speculative parallel sweeps. -- Rayon default-on but feature-gated. +- Rayon default-on but feature-gated. **Superseded — shipped opt-in; see the deviation note in Section 6.** ### Open question From 911b48faba70f05df7af16d08cd77b92b5bb0063 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 8 Sep 2026 12:32:00 +0200 Subject: [PATCH 3/3] feat: add EventBuilder::members for per-member configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EventBuilder` could set weights and nothing else, so `prior` and `drift_scale` were reachable only through the typed `Event`/`Team`/`Member` shape plus `add_events`. Which ingestion route a competitor arrived through decided whether it could be configured. `members(...)` takes `Member` values directly, so `Member`'s own builder expresses everything. `team(...)` stays the common case. One escape hatch rather than `priors` and `drift_scales` setters beside `weights`, as the issue suggested and then argued against itself: a parallel array per field means a parallel length check per field, and each one is a new way to get the lengths wrong. `Member` already has a builder; this just lets the fluent path reach it. `record_winner`/`record_draw` are deliberately left alone. They are the two-argument convenience path, and extending them would be a breaking signature change. The issue's reason for wanting them extended has also weakened: it said a competitor arriving through them was "permanently stuck on the history defaults", and since 8c087ad that is no longer true — a later `add_events` carrying the `Member` refits the whole history. Measured, late configuration through that route reaches mu 40.000000000, identical to configuring from the start. Refs #37 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- README.md | 7 +- src/event_builder.rs | 36 ++++++ tests/event_builder_members.rs | 193 +++++++++++++++++++++++++++++++++ 3 files changed, 233 insertions(+), 3 deletions(-) create mode 100644 tests/event_builder_members.rs diff --git a/README.md b/README.md index 6411954..fdf3c67 100644 --- a/README.md +++ b/README.md @@ -144,9 +144,10 @@ competitor within a single batch is `InferenceError::ConflictingCompetitorConfig since events in a batch have no order. The scale must be finite and non-negative; ingestion otherwise fails with `InferenceError::InvalidParameter`. -Note that the fluent `EventBuilder` (`h.event(t).team([...])`) sets weights but -not `drift_scale` or `prior`; those need the typed `Event` / `Team` / `Member` -shape shown above. +The fluent `EventBuilder` reaches this too: `.team([...])` is the common case +and leaves both unset, while `.members([...])` takes `Member` values directly, +so `h.event(t).members([Member::new("layout_7").with_drift_scale(0.0)])` is +equivalent to the typed shape above. ## Scored outcomes diff --git a/src/event_builder.rs b/src/event_builder.rs index 01fca05..ecc1555 100644 --- a/src/event_builder.rs +++ b/src/event_builder.rs @@ -50,6 +50,8 @@ where } /// Add a team by its member keys (weight 1.0 each, no prior overrides). + /// + /// Use [`EventBuilder::members`] to set `prior` or `drift_scale`. pub fn team>(mut self, keys: I) -> Self { let members: SmallVec<[Member; 4]> = keys.into_iter().map(Member::new).collect(); self.event.teams.push(Team { members }); @@ -57,6 +59,40 @@ where self } + /// Add a team from fully-specified [`Member`] values. + /// + /// [`EventBuilder::team`] is the common case and builds members with + /// `Member::new`, which leaves `prior` and `drift_scale` unset. This is the + /// escape hatch for when they matter: + /// + /// ``` + /// # use trueskill_tt::{Gaussian, History, Member}; + /// # let mut h = History::builder().build(); + /// h.event(0) + /// .team(["player"]) + /// .members([Member::new("layout_7") + /// .with_drift_scale(0.0) + /// .with_prior(Gaussian::from_ms(0.0, 1.0))]) + /// .ranking([0, 1]) + /// .commit()?; + /// # Ok::<(), trueskill_tt::InferenceError>(()) + /// ``` + /// + /// One method rather than a `priors` and a `drift_scales` setter beside + /// `weights`: those would have to grow a parallel array — and a parallel + /// length check — every time `Member` gains a field, and each one would be + /// a new way to get the lengths wrong. `Member`'s own builder already + /// expresses all of it. + /// + /// `prior` and `drift_scale` are competitor configuration rather than + /// per-event values; see [`Member`] for what that means for a key the + /// history already knows. + pub fn members>>(mut self, members: I) -> Self { + self.event.teams.push(Team::with_members(members)); + self.current_team_idx = Some(self.event.teams.len() - 1); + self + } + /// Set per-member weights for the most recently added team. /// /// A length mismatch is recorded and returned by [`EventBuilder::commit`] diff --git a/tests/event_builder_members.rs b/tests/event_builder_members.rs new file mode 100644 index 0000000..eb5ccc8 --- /dev/null +++ b/tests/event_builder_members.rs @@ -0,0 +1,193 @@ +//! `EventBuilder::members` must reach exactly what the typed path reaches. +//! +//! Before this existed, `EventBuilder` could set weights and nothing else, so +//! `prior` and `drift_scale` were expressible only through `Event`/`Team`/ +//! `Member` + `add_events`. Which ingestion route a competitor arrived through +//! decided whether it could be configured at all. + +use smallvec::smallvec; +use trueskill_tt::{ + ConstantDrift, ConvergenceOptions, Event, Gaussian, History, InferenceError, Member, Outcome, + Team, +}; + +type H = History; + +fn history() -> H { + History::builder() + .mu(0.0) + .sigma(6.0) + .beta(1.0) + .score_sigma(2.0) + .drift(ConstantDrift(0.5)) + .convergence(ConvergenceOptions { + max_iter: 20_000, + epsilon: 1e-13, + alpha: 1.0, + }) + .build() +} + +const PRIOR: Gaussian = Gaussian::from_ms(3.0, 1.5); + +/// The contract that makes the escape hatch worth having: same configuration, +/// same fit, bit for bit. +#[test] +fn members_matches_the_typed_path_exactly() { + let mut typed = history(); + typed + .add_events(vec![Event { + time: 1, + teams: smallvec![ + Team::with_members([Member::new("player")]), + Team::with_members([Member::new("layout_7") + .with_drift_scale(0.0) + .with_prior(PRIOR)]), + ], + outcome: Outcome::scores([5.0, 2.0]), + }]) + .unwrap(); + assert!(typed.converge().unwrap().converged); + + let mut fluent = history(); + fluent + .event(1) + .team(["player"]) + .members([Member::new("layout_7") + .with_drift_scale(0.0) + .with_prior(PRIOR)]) + .scores([5.0, 2.0]) + .commit() + .unwrap(); + assert!(fluent.converge().unwrap().converged); + + for key in ["player", "layout_7"] { + let a = typed.current_skill(&key).unwrap(); + let b = fluent.current_skill(&key).unwrap(); + assert_eq!(a.pi(), b.pi(), "{key} pi"); + assert_eq!(a.tau(), b.tau(), "{key} tau"); + } +} + +/// The configuration has to actually take effect, not merely round-trip: a +/// competitor pinned with `drift_scale = 0.0` must not move across slices, +/// where an unpinned one does. +/// +/// The comparison is against a control rather than against a fixed epsilon. +/// Pinned marginals are not bit-identical across slices — each slice combines +/// its own forward and backward messages, so the arithmetic order differs and +/// the last bit moves. What "pinned" promises is that no drift variance +/// accumulates, and the control is what makes that measurable. +#[test] +fn a_drift_scale_set_through_members_is_applied() { + fn spread(h: &H, key: &'static str) -> f64 { + let curve = h.learning_curve(&key); + assert!(curve.len() >= 2, "{key}: expected several appearances"); + let (lo, hi) = curve.iter().fold((f64::MAX, f64::MIN), |(lo, hi), (_, g)| { + (lo.min(g.sigma()), hi.max(g.sigma())) + }); + (hi - lo) / hi + } + + let mut h = history(); + for t in 1..=4 { + h.event(t) + .team(["player"]) + .members([Member::new("pinned").with_drift_scale(0.0)]) + .scores([5.0, 2.0]) + .commit() + .unwrap(); + // Same shape, no pinning: the control. + h.event(t) + .team(["rival"]) + .team(["drifting"]) + .scores([5.0, 2.0]) + .commit() + .unwrap(); + } + assert!(h.converge().unwrap().converged); + + let pinned = spread(&h, "pinned"); + let drifting = spread(&h, "drifting"); + assert!(pinned < 1e-9, "pinned competitor moved: {pinned:e}"); + assert!( + drifting > 1e-3, + "control did not move, so the test proves nothing: {drifting:e}" + ); +} + +/// `weights` still applies to a team added through `members`, and still +/// records a mismatch rather than partially applying it. +#[test] +fn weights_still_guards_a_members_team() { + let mut h = history(); + let err = h + .event(1) + .team(["a"]) + .members([Member::new("b"), Member::new("c")]) + .weights([1.0]) + .winner(0) + .commit() + .unwrap_err(); + assert!( + matches!( + err, + InferenceError::MismatchedShape { + kind: "weights", + expected: 2, + got: 1 + } + ), + "{err:?}" + ); + assert!(h.current_skill(&"b").is_none(), "nothing may reach history"); +} + +/// An invalid `drift_scale` surfaces from `commit`, not from a panic and not +/// silently. +#[test] +fn an_invalid_drift_scale_surfaces_from_commit() { + for bad in [-1.0, f64::NAN, f64::INFINITY] { + let mut h = history(); + let err = h + .event(1) + .team(["a"]) + .members([Member::new("b").with_drift_scale(bad)]) + .winner(0) + .commit() + .unwrap_err(); + assert!( + matches!( + err, + InferenceError::InvalidParameter { + name: "drift_scale", + .. + } + ), + "{bad}: {err:?}" + ); + assert!(h.current_skill(&"b").is_none(), "{bad} reached the history"); + } +} + +/// `members` and `team` compose in either order. +#[test] +fn members_and_team_interleave() { + let mut h = history(); + h.event(1) + .members([Member::new("a").with_prior(PRIOR)]) + .team(["b"]) + .scores([3.0, 1.0]) + .commit() + .unwrap(); + h.event(2) + .team(["b"]) + .members([Member::new("c").with_prior(PRIOR)]) + .scores([2.0, 4.0]) + .commit() + .unwrap(); + assert!(h.converge().unwrap().converged); + for key in ["a", "b", "c"] { + assert!(h.current_skill(&key).is_some(), "{key} missing"); + } +}