fix: enforce EventBuilder weight/team length in release
Part of #18. `EventBuilder::weights` guarded the length match with a `debug_assert!`, so release builds accepted a mismatch, silently dropped the weights, and ingested the event anyway. That is the exact shape #18 is about: validation that exists only where it is least needed. The setters return `Self` to keep the chain fluent, so they cannot return a `Result`. The builder now records the first failure and `commit` returns it as `MismatchedShape`. The weights are not applied on mismatch either, so a partially-weighted team cannot reach the history by another route. Two tests in tests/degenerate_inputs.rs, whose CI job runs in release — which is the only place the old behaviour differed. The second test needed strengthening before it was worth anything. As first written it committed a ONE-team event, which ingestion rejects for an unrelated reason, so it passed under a mutation that disabled the whole check. It now uses two teams, so ingestion would otherwise succeed and the assertion is actually load-bearing. Both tests were then mutation-proved together: disabling the error path in `commit` fails both in release. #18 stays open. The remaining debug_asserts live in `ranked_with_arena` and `scored_with_arena`, and promoting those means threading `Result` up through `Event::compute`, `TimeSlice::iteration`, `log_evidence` and `filtered_step` — which lands on the public API as `log_evidence() -> Result<f64>` and `filtered_learning_curve() -> Result<...>`. That is a trade-off about what the query API should look like, not a mechanical change, so it is not mine to decide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T5SYDExxL4vZgvunrcNSMc
This commit is contained in:
+35
-8
@@ -19,6 +19,14 @@ where
|
||||
history: &'h mut History<T, D, O, K>,
|
||||
event: Event<T, K>,
|
||||
current_team_idx: Option<usize>,
|
||||
/// First validation failure seen while building, surfaced by `commit`.
|
||||
///
|
||||
/// The setters return `Self` so the chain stays fluent; they cannot return
|
||||
/// a `Result` without breaking that. Recording the failure and reporting it
|
||||
/// at `commit` keeps the check enforced in release, where the previous
|
||||
/// `debug_assert!` was compiled out and a mismatched event was ingested
|
||||
/// silently.
|
||||
error: Option<InferenceError>,
|
||||
}
|
||||
|
||||
impl<'h, T, D, O, K> EventBuilder<'h, T, D, O, K>
|
||||
@@ -37,6 +45,7 @@ where
|
||||
outcome: Outcome::Ranked(SmallVec::new()),
|
||||
},
|
||||
current_team_idx: None,
|
||||
error: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -50,24 +59,36 @@ where
|
||||
|
||||
/// Set per-member weights for the most recently added team.
|
||||
///
|
||||
/// A length mismatch is recorded and returned by [`EventBuilder::commit`]
|
||||
/// as `InferenceError::MismatchedShape`, in both debug and release. The
|
||||
/// weights are not applied in that case, so a partially-weighted team
|
||||
/// cannot reach the history.
|
||||
///
|
||||
/// # Panics
|
||||
///
|
||||
/// Panics if called before any `.team(...)`. In debug builds, also panics
|
||||
/// if the number of weights does not match the team's member count.
|
||||
/// Panics if called before any `.team(...)`.
|
||||
pub fn weights<I: IntoIterator<Item = f64>>(mut self, weights: I) -> Self {
|
||||
let idx = self
|
||||
.current_team_idx
|
||||
.expect(".weights(...) called before any .team(...)");
|
||||
|
||||
let ws: Vec<f64> = weights.into_iter().collect();
|
||||
let team = &mut self.event.teams[idx];
|
||||
debug_assert_eq!(
|
||||
ws.len(),
|
||||
team.members.len(),
|
||||
"weights length must match team size"
|
||||
);
|
||||
|
||||
if ws.len() != team.members.len() {
|
||||
self.error.get_or_insert(InferenceError::MismatchedShape {
|
||||
kind: "weights",
|
||||
expected: team.members.len(),
|
||||
got: ws.len(),
|
||||
});
|
||||
|
||||
return self;
|
||||
}
|
||||
|
||||
for (m, w) in team.members.iter_mut().zip(ws) {
|
||||
m.weight = w;
|
||||
}
|
||||
|
||||
self
|
||||
}
|
||||
|
||||
@@ -108,8 +129,14 @@ where
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Forwards to [`History::add_events`] and returns its errors.
|
||||
/// Returns the first validation failure recorded while building — see
|
||||
/// [`EventBuilder::weights`] — otherwise forwards to
|
||||
/// [`History::add_events`] and returns its errors.
|
||||
pub fn commit(self) -> Result<(), InferenceError> {
|
||||
if let Some(error) = self.error {
|
||||
return Err(error);
|
||||
}
|
||||
|
||||
self.history.add_events(std::iter::once(self.event))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -141,6 +141,54 @@ fn converge_on_an_empty_history_with_owned_keys() {
|
||||
assert!(report.converged);
|
||||
}
|
||||
|
||||
/// A weights/team length mismatch used to be a `debug_assert!`, so release
|
||||
/// builds ingested the event with the weights silently unapplied. This file's
|
||||
/// CI job runs in release too, which is the point of pinning it here.
|
||||
#[test]
|
||||
fn event_builder_rejects_a_weights_length_mismatch() {
|
||||
let mut h = History::default();
|
||||
|
||||
let err = h
|
||||
.event(1)
|
||||
.team(["a"])
|
||||
.weights([1.0, 2.0])
|
||||
.team(["b"])
|
||||
.winner(0)
|
||||
.commit()
|
||||
.unwrap_err();
|
||||
|
||||
assert!(
|
||||
matches!(
|
||||
err,
|
||||
InferenceError::MismatchedShape {
|
||||
kind: "weights",
|
||||
expected: 1,
|
||||
got: 2,
|
||||
}
|
||||
),
|
||||
"expected a weights MismatchedShape, got {err:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// The mismatch must not be applied even partially — a half-weighted team
|
||||
/// reaching the history would be worse than the error.
|
||||
#[test]
|
||||
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.
|
||||
let _ = h
|
||||
.event(1)
|
||||
.team(["a"])
|
||||
.weights([1.0, 2.0])
|
||||
.team(["b"])
|
||||
.winner(0)
|
||||
.commit();
|
||||
|
||||
assert!(h.learning_curve("a").is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn empty_event_stream_then_converge() {
|
||||
let mut h = History::default();
|
||||
|
||||
Reference in New Issue
Block a user