diff --git a/src/host/aec/mod.rs b/src/host/aec/mod.rs index ffe78be..a4b2218 100644 --- a/src/host/aec/mod.rs +++ b/src/host/aec/mod.rs @@ -87,10 +87,18 @@ pub enum AecParseError { /// /// The index accepts values `> u32::MAX` (v3.4 §5.2: `pulse.module.id` sits /// next to the `object.serial` u32-truncation bug, so it is only ever compared -/// as `u64`) and rejects anything Rust's [`u64`] parser rejects — a leading -/// sign, surrounding or interior whitespace, non-decimal digits, and overflow. -/// Matching is exact and case-sensitive: the argument is machine-generated by -/// peerspeak from `EchoCancelGuard::module_index`, not typed by a user. +/// as `u64`) and requires a **bare decimal** — stricter than Rust's [`u64`] +/// parser, which also accepts a leading `+`. Rejected: any sign, surrounding or +/// interior whitespace, non-decimal digits, and overflow. Matching is exact and +/// case-sensitive: the argument is machine-generated by peerspeak from +/// `EchoCancelGuard::module_index`, not typed by a user. +/// +/// ⚠️ **Producer contract** (Codex phase-4 review, finding 5): because the +/// grammar is narrower than Rust's parser, peerspeak must emit a bare decimal. +/// `pactl load-module` returns an unsigned decimal, so the stored index is +/// already canonical and no reachable value is rejected; if peerspeak ever +/// changes how it formats the index it must canonicalize (`value.to_string()`), +/// not widen this parser — the narrow grammar is the point. pub fn parse_aec_arg(value: &str) -> Result { if value.is_empty() { return Err(AecParseError::Empty); @@ -227,6 +235,16 @@ impl AecValidator { }; match self.state { AecState::Validating => { + // Presence is checked *before* the deadline on purpose: a + // demonstrably-present identity validates regardless of the + // clock, even if the node is first seen just past the deadline + // (Codex phase-4 review, finding 2). The deadline only bounds + // the wait for an identity that is never seen — seeing it, late + // or not, is ground truth that the module exists, and excluding + // a real echo leg is always the safe answer. (A `Failed` can + // still pre-empt this when a `Tick`-only observation crosses the + // deadline first; that only makes the machine *more* fail-closed, + // never less.) if self.index_present(snapshot, target) { self.state = AecState::Validated; return; @@ -240,7 +258,39 @@ impl AecValidator { } } AecState::Validated => { - // Revocation is the whole identity gone, not one leg corking. + // Revocation is the whole identity gone (no node bears the + // index), not one leg corking — see the module header. + // + // ⚠️ **Deliberately NOT gated on `graph_ready`** (Codex + // phase-4 review, findings 1 + 4). Two forces pull opposite + // ways and this is the resolution: + // + // - Gating revoke on readiness would avoid a *spurious* revoke + // from a transient empty snapshot seen while the module is + // still live. But for the AEC that transient does not exist: + // its four nodes are two `Stream/*` legs plus a null-sink-like + // virtual sink/source, none of which claim a `device.id`, so + // the phase-3 observer never *withholds* them + // (`observer::classify` withholds only device-claiming nodes). + // `index_present` therefore goes false only on a genuine + // `global_remove` of every leg — a real unload — and a real + // unload *should* revoke. + // - Worse, gating on readiness would REOPEN the reused-index + // alias trap: if an unload+reload (indices recycle, §5.2 + // correction 3) both complete inside one not-ready churn + // window, the ready snapshot would already show the *new* + // module's node and we would never observe the empty gap — + // silently aliasing onto an unrelated module. Revoking the + // instant the gap appears, ready or not, is what closes it. + // + // This correctness rests on the phase-5/6 integration contract: + // **one `observe` per graph event, no coalescing across a module + // lifetime boundary.** Under coalescing, the empty gap between an + // old unload and a reused-index reload can be skipped. The + // robust fix that would not depend on that contract is a + // serial-continuity / observer-generation signal (the AEC nodes' + // `object.serial`s are fresh across a reload even when the index + // is not) — owed to a later hardening round, not built here. if !self.index_present(snapshot, target) { self.state = AecState::Revoked; } diff --git a/src/host/aec/tests.rs b/src/host/aec/tests.rs index 35b538f..dd462fb 100644 --- a/src/host/aec/tests.rs +++ b/src/host/aec/tests.rs @@ -206,6 +206,56 @@ fn deadline_is_not_armed_until_graph_ready() { // Still no Failed even much later, as long as ready stays false. v.observe(&empty(), false, 100 * TIMEOUT); assert_eq!(v.state(), AecState::Validating); + + // And when readiness finally arrives, the FULL budget starts *there*, not + // relative to construction (Codex phase-4 review, finding 3): a mutant that + // armed a construction-relative deadline would fail immediately here. + let late = 200_000; + v.observe(&empty(), true, late); // first ready → arm at `late` + assert_eq!(v.state(), AecState::Validating); + v.observe(&empty(), true, late + TIMEOUT - 1); + assert_eq!(v.state(), AecState::Validating); + v.observe(&empty(), true, late + TIMEOUT); + assert_eq!(v.state(), AecState::Failed); +} + +#[test] +fn late_positive_evidence_wins_over_expired_deadline() { + // A node first seen just past the deadline still validates: the deadline + // only bounds the wait for an identity that is never seen, and a + // demonstrably-present module is ground truth (Codex phase-4 review, + // finding 2). Reachable only when the first post-deadline observation + // carries the node with no intervening Tick-only observation. + let mut v = AecValidator::new(AecConfig::PulseModule(IDX), TIMEOUT); + v.observe(&empty(), true, 0); // arm deadline at 2000 + v.observe(&snapshot(vec![node(1, Some(IDX))]), true, TIMEOUT + 1); + assert_eq!(v.state(), AecState::Validated); + assert_eq!(v.validated_module_id(), Some(IDX)); + + // Whereas a Tick-only observation that crosses the deadline first pre-empts + // it to Failed (stickily), even if the node then shows up — fail-closed. + let mut w = AecValidator::new(AecConfig::PulseModule(IDX), TIMEOUT); + w.observe(&empty(), true, 0); + w.observe(&empty(), true, TIMEOUT); // Tick-only crosses the line first + assert_eq!(w.state(), AecState::Failed); + w.observe(&snapshot(vec![node(1, Some(IDX))]), true, TIMEOUT + 1); + assert_eq!(w.state(), AecState::Failed); +} + +#[test] +fn revokes_on_empty_even_while_not_ready() { + // Revocation is deliberately NOT gated on graph_ready (Codex phase-4 review, + // findings 1 + 4): the instant every node bearing the index is gone we + // revoke, ready or not, because gating on readiness would let an + // unload+reload that reused the index inside one not-ready churn window + // silently alias onto the new module. A mutant adding `&& graph_ready` to + // the revoke guard survives every other test but dies here. + let mut v = AecValidator::new(AecConfig::PulseModule(IDX), TIMEOUT); + v.observe(&snapshot(vec![node(1, Some(IDX))]), true, 0); + assert_eq!(v.state(), AecState::Validated); + v.observe(&empty(), false, 10); // identity gone during not-ready churn + assert_eq!(v.state(), AecState::Revoked); + assert!(!v.fan_out_permitted()); } #[test]