From 4a844ac6fafc108f22204d2e6da997a87f95c019 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Fri, 24 Jul 2026 15:07:01 -0400 Subject: [PATCH] host/aec: address Codex phase-4 review (no merge-blockers; docs + test holes) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex adversarial review found no merge-blocking defects. Five worth-checking items, all triaged for reachability: - F1 (spurious pre-ready revoke): unreachable — the AEC's four nodes are two Stream/* legs + a null-sink-like virtual sink/source, none claiming a device.id, so the phase-3 observer never withholds them; index_present goes false only on a genuine full unload. Documented why revoke is NOT gated on graph_ready, and why gating it would reopen the reused-index alias trap (F4) during a hot-reload-under-churn. Pinned with revokes_on_empty_even_while_not_ready (mutation-verified: `&& graph_ready` on the revoke guard dies here). - F4 (test relies on observing the empty gap): documented the phase-5/6 integration contract it rests on (one observe per graph event, no coalescing across a module lifetime boundary) and owed the robust fix (serial-continuity / observer-generation) to a later hardening round. - F2 (late positive evidence beats the deadline): intentional and correct — a demonstrably-present identity is ground truth. Documented + late_positive_evidence_wins_over_expired_deadline (both arms: node-first validates, Tick-first fails closed and stays sticky). - F3 (real P3 coverage hole): strengthened deadline_is_not_armed_until_ graph_ready to prove the budget starts at first-ready, not construction (mutation-verified: a construction-relative deadline now dies). - F5 (`+7` grammar mismatch): documented the producer contract — peerspeak emits bare decimal (pactl returns unsigned decimal), the narrow parser is deliberate. Unreachable on the measured stack. No core logic change. 24 pure aec tests, cargo test --bins green (145 unit + 1 ignored live), clippy -D warnings + fmt clean. Co-Authored-By: Claude Opus 4.8 --- src/host/aec/mod.rs | 60 +++++++++++++++++++++++++++++++++++++++---- src/host/aec/tests.rs | 50 ++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 5 deletions(-) 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]