log_evidence_for silently returns whole-history evidence for an unknown key #66

Closed
opened 2026-09-09 17:52:35 +00:00 by logaritmisk · 1 comment
Owner

src/history.rs:741:

let targets: Vec<Index> = keys.iter().filter_map(|k| self.keys.get(*k)).collect();
self.log_evidence_internal(false, &targets)

filter_map drops keys the history has never interned, and an empty targets means no restriction downstream. So a list of entirely unknown keys returns the full-history value.

Measured

12 events, two disjoint cohorts:

log_evidence()             = -0.757480
log_evidence_for(["a"])    = -0.378740
log_evidence_for(["typo"]) = -0.757480   <- unknown key, silently == log_evidence()
log_evidence_for([])       = -0.757480

Why this one matters more than it looks

This is the crate's own stated recurring defect — a public surface reporting a plausible constant — and UnknownKey's doc comment (src/error.rs:106) argues against exactly this:

Reported rather than skipped: dropping unknown keys turns a team of strangers into a confident-looking probability about nobody.

Every neighbouring accessor honours that. current_skill, rating and lookup return None; posterior_of returns Err(UnknownKey); ingestion rejects. This one function does the opposite, returns bare f64 so there is no channel to report on, and is documented as "Useful for leave-one-out cross-validation" — the single workload where a typo'd key silently yields the un-held-out score, which is a plausible number that quietly invalidates the comparison it was computed for.

Fix

pub fn log_evidence_for<Q>(&self, keys: &[&Q]) -> Result<f64, InferenceError>

returning UnknownKey { key, .. } for any key not interned. The empty-slice case should be decided explicitly rather than falling through to "whole history" — either an error or a documented identity.

Breaks: every call site, by return type. Loud and mechanical, which is the right kind of break for this.

Found by an API audit, 2026-09-09; reproduced independently of the agent that reported it.

`src/history.rs:741`: ```rust let targets: Vec<Index> = keys.iter().filter_map(|k| self.keys.get(*k)).collect(); self.log_evidence_internal(false, &targets) ``` `filter_map` drops keys the history has never interned, and an empty `targets` means *no restriction* downstream. So a list of entirely unknown keys returns the **full-history** value. ## Measured 12 events, two disjoint cohorts: ``` log_evidence() = -0.757480 log_evidence_for(["a"]) = -0.378740 log_evidence_for(["typo"]) = -0.757480 <- unknown key, silently == log_evidence() log_evidence_for([]) = -0.757480 ``` ## Why this one matters more than it looks This is the crate's own stated recurring defect — a public surface reporting a plausible constant — and `UnknownKey`'s doc comment (`src/error.rs:106`) argues against exactly this: > Reported rather than skipped: dropping unknown keys turns a team of strangers into a confident-looking probability about nobody. Every neighbouring accessor honours that. `current_skill`, `rating` and `lookup` return `None`; `posterior_of` returns `Err(UnknownKey)`; ingestion rejects. This one function does the opposite, returns bare `f64` so there is no channel to report on, and is documented as *"Useful for leave-one-out cross-validation"* — the single workload where a typo'd key silently yields the **un-held-out** score, which is a plausible number that quietly invalidates the comparison it was computed for. ## Fix ```rust pub fn log_evidence_for<Q>(&self, keys: &[&Q]) -> Result<f64, InferenceError> ``` returning `UnknownKey { key, .. }` for any key not interned. The empty-slice case should be decided explicitly rather than falling through to "whole history" — either an error or a documented identity. **Breaks:** every call site, by return type. Loud and mechanical, which is the right kind of break for this. Found by an API audit, 2026-09-09; reproduced independently of the agent that reported it.
logaritmisk added the apibreakingbug labels 2026-09-09 17:57:50 +00:00
Author
Owner

Fixed in e4a68ba (merged as 60fc3e9).

log_evidence_for now returns Result<f64, InferenceError> and reports UnknownKey { member, .. } naming the offending position, rather than silently filtering unknowns away. The filtered-out case was worse than a missing check: an empty target list means "no restriction" downstream, so an entirely unknown list returned the whole-history value — measured on a two-cohort fixture, log_evidence_for(["typo"]) returned exactly log_evidence().

tests/honest_accessors.rs covers both the all-unknown and the known+unknown mix, each with a control case that must still succeed.

Fixed in e4a68ba (merged as 60fc3e9). `log_evidence_for` now returns `Result<f64, InferenceError>` and reports `UnknownKey { member, .. }` naming the offending position, rather than silently filtering unknowns away. The filtered-out case was worse than a missing check: an empty target list means "no restriction" downstream, so an *entirely* unknown list returned the whole-history value — measured on a two-cohort fixture, `log_evidence_for(["typo"])` returned exactly `log_evidence()`. `tests/honest_accessors.rs` covers both the all-unknown and the known+unknown mix, each with a control case that must still succeed.
Sign in to join this conversation.