fix: reject malformed events at the ingestion boundary
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
This commit is contained in:
@@ -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<i64, &'static str>;
|
||||
|
||||
fn history() -> History<i64, trueskill_tt::ConstantDrift, trueskill_tt::NullObserver, &'static str>
|
||||
{
|
||||
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());
|
||||
}
|
||||
Reference in New Issue
Block a user