From 2eb0208e3cbdf5a9e86952f5d5949fdce97bb978 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Sun, 23 Aug 2026 14:34:00 +0000 Subject: [PATCH 1/5] =?UTF-8?q?fix:=20gate=20=CF=84=20on=20the=20peak's=20?= =?UTF-8?q?own=20pre-impulse=20SNR,=20not=20captured=20level?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit τ used to run only when cal_prompt step 2's is_loopback level flag was true — a captured-level proxy for "is this cable patched" against a ±2 dB unity window. A hot loopback (+3.01 dB) or low-gain loopback (-4.19 dB) both failed that check on the rig even though both carried a real, measurable arrival; only a muted route was a genuine absence, and all three printed the same asserted line, "loopback not detected this run." τ is now attempted unconditionally. The gate moved inside measure_tau itself: after the deconvolved peak is located, its pre-impulse SNR (shared with ac-core's ir_stats() via a new sweep::pre_impulse_snr_db, replacing two copies of one formula) is checked against a 24 dB threshold (env-overridable under the tau-window-override feature, same mechanism as TAU_EDGE_MARGIN_FRAC) before the existing edge-margin check runs. A lifecycle below threshold produces a new tau_state, not_measured_low_snr, distinct from the generic error state a real engine/deconvolution failure produces. cal_prompt step 2's own loopback flag is untouched and keeps gating only the DMM pre-fill. closes #368 Co-Authored-By: Claude Sonnet 5 --- ac-rs/ZMQ.md | 42 ++- ac-rs/crates/ac-cli/src/commands/calibrate.rs | 31 +- .../crates/ac-core/src/measurement/report.rs | 19 +- ac-rs/crates/ac-core/src/measurement/sweep.rs | 28 ++ ac-rs/crates/ac-daemon/src/audio/fake.rs | 53 ++- .../ac-daemon/src/handlers/calibrate.rs | 356 +++++++++++++----- ac-rs/crates/ac-daemon/tests/it_protocol.rs | 88 ++++- 7 files changed, 492 insertions(+), 125 deletions(-) diff --git a/ac-rs/ZMQ.md b/ac-rs/ZMQ.md index 720be112..ec990a9e 100644 --- a/ac-rs/ZMQ.md +++ b/ac-rs/ZMQ.md @@ -1483,7 +1483,7 @@ reading either. "vrms_at_0dbfs_in": | null, // post-scale, projected to 0 dBFS "out_state": "measured" | "unchanged" | "absent", "in_state": "measured" | "unchanged" | "absent", - "tau_state": "measured" | "not_measured_no_loopback" | "error" + "tau_state": "measured" | "not_measured_low_snr" | "error" | "disagree_period_shift" | "disagree_other", "tau_s": | null, // interface round-trip delay, seconds; only non-null when tau_state == "measured" "tau_sample_rate": , // condition τ was measured/attempted under @@ -1494,6 +1494,8 @@ reading either. "tau_delta_samples": , // #347: round((reading2 - reading1) * sample_rate) — present only on disagree_* "tau_periods": , // #347: signed period count — present only on tau_state == "disagree_period_shift" "tau_error": "", // present when tau_state is "error", "disagree_period_shift", or "disagree_other" + "tau_pre_impulse_snr_db": , // #368: the (worse-of-two, when both ran) peak's pre-impulse SNR — present on measured / not_measured_low_snr / disagree_*, absent on error + "tau_snr_threshold_db": , // #368: the threshold that SNR was judged against — present alongside tau_pre_impulse_snr_db "error": "" // only present on partial failure (voltage-cal save) } ``` @@ -1507,27 +1509,37 @@ not only what this run measured, and the `*_state` word says which: | `unchanged` | the prompt was skipped; the previously stored value stands | | `absent` | the field holds no value — never set, or just cleared | -**τ (interface latency, #281/#347)** is not prompt-driven — it piggybacks -on the loopback state `cal_prompt` step 2 already established, so there is -no third interactive step and no `unchanged` state (skipping a voltage +**τ (interface latency, #281/#347)** is not prompt-driven — it is not a +third interactive step and has no `unchanged` state (skipping a voltage prompt does not affect it). #347: a single reading is not a measurement of τ on this stack — round-trip latency for a fixed path can vary by exactly one period between client lifetimes, invisible within any one lifetime (stable to 0.001 frames). `calibrate` therefore always runs τ as **two** independent client lifecycles (fresh `start`/`stop` each) and compares -them before storing anything: +them before storing anything. + +**#368**: τ used to run only when `cal_prompt` step 2's `loopback` flag +was `true` — a captured-level proxy for "is this cable patched" that a +loopback 3 dB hot or 4 dB low both failed even though both carried a real, +measurable arrival, and that a loud but uncorrelated interferer could +still pass. τ is now attempted unconditionally; the gate lives inside the +measurement itself, on the deconvolved peak's own pre-impulse SNR, which +is the quantity that actually distinguishes "patched" from "not patched." +`cal_prompt` step 2's own `loopback` flag is unchanged and keeps gating +only whether the DMM prompt pre-fills the output reading — a separate, +still-unity-keyed decision. | `tau_state` | meaning | |-------------|---------| -| `measured` | loopback detected this run; two independent readings agreed to the whole sample and their average was appended to `tau_history` | -| `not_measured_no_loopback` | loopback not detected this run — nothing to measure τ against | -| `error` | loopback was detected but a lifecycle's own measurement failed (`tau_error` names why, including which reading); the voltage-cal legs above are unaffected | +| `measured` | two independent readings agreed to the whole sample and their average was appended to `tau_history` | +| `not_measured_low_snr` | a lifecycle's deconvolved peak was below `tau_snr_threshold_db` pre-impulse SNR — not distinguishable from noise, so nothing was measured | +| `error` | a lifecycle's own measurement failed for a reason other than low SNR (`tau_error` names why, including which reading); the voltage-cal legs above are unaffected | | `disagree_period_shift` | the two readings disagreed by an exact multiple of `tau_period_size` samples — a graph-buffering shift (software), not hardware drift. Nothing is stored. | | `disagree_other` | the two readings disagreed, but not by a period multiple — a different fault class. Nothing is stored. | `tau_sample_rate` / `tau_period_size` are the conditions the attempt ran under (present regardless of `tau_state`, including `error`), so a -`not_measured_no_loopback` or `error` result is still legible against +`not_measured_low_snr` or `error` result is still legible against `cal.json` history without a second round trip. `tau_period_size: null` does not mean unknown — see `AudioEngine::period_size` in `ac-daemon/src/audio/mod.rs`: some backends cannot report a period size @@ -1535,6 +1547,18 @@ at all, which is a documented backend limitation, distinct from a period size that simply wasn't queried (and means `disagree_period_shift` can never fire on that backend — any disagreement there is `disagree_other`). +`tau_pre_impulse_snr_db` / `tau_snr_threshold_db` (#368) are present on +every state where at least one lifecycle reached deconvolution +(`measured`, `not_measured_low_snr`, `disagree_*`) and absent on `error`, +which can fail before a peak was ever located. On `measured` and +`disagree_*`, the SNR reported is the worse (lower) of the two +lifecycles' — both necessarily cleared the threshold, since a lifecycle +that didn't would have produced `not_measured_low_snr` instead, so this is +a diagnostic figure alongside the result rather than a second gate. +`tau_snr_threshold_db` is a derived constant (see +`ac-daemon/src/handlers/calibrate.rs`'s `TAU_SNR_THRESHOLD_DB` doc +comment for its provenance), not measured on this exact sweep. + On either disagreement state, `tau_reading1_s` / `tau_reading2_s` are the raw seconds values from the two lifecycles, shown verbatim rather than compressed to a delta — the fractional part staying identical across a diff --git a/ac-rs/crates/ac-cli/src/commands/calibrate.rs b/ac-rs/crates/ac-cli/src/commands/calibrate.rs index 2785a2ed..a8b53307 100644 --- a/ac-rs/crates/ac-cli/src/commands/calibrate.rs +++ b/ac-rs/crates/ac-cli/src/commands/calibrate.rs @@ -249,14 +249,29 @@ fn print_tau_leg(data: &serde_json::Value) { "disagree_period_shift" | "disagree_other" => { print_tau_disagreement_leg(state, data, sample_rate); } - // "not_measured_no_loopback" and anything unrecognised (older - // daemon without this field): state the observation, not an - // inferred cause — `is_loopback` is what the daemon saw, not a - // claim about physical wiring the instrument cannot verify. - _ => println!( - " {:<8}not measured (loopback not detected this run)", - "Delay:" - ), + // #368: the peak's own SNR fell short of the threshold it was + // judged against — both are what the daemon actually measured, so + // print them rather than an inferred wiring conclusion. + "not_measured_low_snr" => { + match ( + data.get("tau_pre_impulse_snr_db").and_then(|v| v.as_f64()), + data.get("tau_snr_threshold_db").and_then(|v| v.as_f64()), + ) { + (Some(snr), Some(threshold)) => println!( + " {:<8}not measured (peak SNR {snr:.2} dB, need {threshold:.2} dB, \ + threshold derived)", + "Delay:" + ), + // Fields absent (older daemon claiming this state without + // them): fall through to the raw-state rendering below + // rather than assert numbers the daemon never sent. + _ => println!(" {:<8}not measured (state: {state})", "Delay:"), + } + } + // Anything unrecognised (older daemon, or a future state this + // client doesn't know): state the raw wire value, not an inferred + // cause the instrument cannot verify. + _ => println!(" {:<8}not measured (state: {state})", "Delay:"), } } diff --git a/ac-rs/crates/ac-core/src/measurement/report.rs b/ac-rs/crates/ac-core/src/measurement/report.rs index 6d867b31..900a2d3e 100644 --- a/ac-rs/crates/ac-core/src/measurement/report.rs +++ b/ac-rs/crates/ac-core/src/measurement/report.rs @@ -634,21 +634,10 @@ impl MeasurementReport { // Pre-impulse noise floor: everything strictly before the peak, // minus a small guard band so the peak's own skirt doesn't bias - // the floor estimate upward. - let guard = (window_len / 32).max(8); - let pre_end = peak_index.saturating_sub(guard); - let pre_region = &linear_ir[..pre_end]; - let pre_impulse_snr_db = if pre_region.is_empty() { - f64::INFINITY - } else { - let mean_sq = pre_region.iter().map(|v| v * v).sum::() / pre_region.len() as f64; - let rms = mean_sq.sqrt(); - if rms > 0.0 { - 20.0 * (peak_magnitude / rms).log10() - } else { - f64::INFINITY - } - }; + // the floor estimate upward. Shared with `ac-daemon`'s τ gate + // (#368) via `sweep::pre_impulse_snr_db` — one formula, not two. + let pre_impulse_snr_db = + crate::measurement::sweep::pre_impulse_snr_db(linear_ir, peak_index); // Prefer the gate the producer actually applied. #280 stores // `f_low_hz` on the payload precisely so a reader does not diff --git a/ac-rs/crates/ac-core/src/measurement/sweep.rs b/ac-rs/crates/ac-core/src/measurement/sweep.rs index 1752a691..6576491b 100644 --- a/ac-rs/crates/ac-core/src/measurement/sweep.rs +++ b/ac-rs/crates/ac-core/src/measurement/sweep.rs @@ -424,6 +424,34 @@ pub fn extract_irs( }) } +/// Pre-impulse SNR of a linear impulse response, in dB: the located peak's +/// magnitude over the RMS of everything strictly before it, minus a small +/// guard band (`(ir.len() / 32).max(8)` samples) so the peak's own skirt +/// doesn't bias the floor estimate upward. `f64::INFINITY` when the +/// pre-peak region is empty or measures true silence (zero RMS). +/// +/// Lifted out of `report.rs::ir_stats()` (#368) so `ac-daemon`'s τ gate can +/// call the same formula on the same quantity — "does the deconvolution +/// find a peak with adequate SNR" — rather than keeping two copies of one +/// calculation that could drift apart. `peak_index` is the caller's own +/// argmax over `ir`; this does not recompute it. +pub fn pre_impulse_snr_db(ir: &[f64], peak_index: usize) -> f64 { + let guard = (ir.len() / 32).max(8); + let pre_end = peak_index.saturating_sub(guard); + let pre_region = &ir[..pre_end.min(ir.len())]; + if pre_region.is_empty() { + return f64::INFINITY; + } + let peak_magnitude = ir.get(peak_index).map(|v| v.abs()).unwrap_or(0.0); + let mean_sq = pre_region.iter().map(|v| v * v).sum::() / pre_region.len() as f64; + let rms = mean_sq.sqrt(); + if rms > 0.0 { + 20.0 * (peak_magnitude / rms).log10() + } else { + f64::INFINITY + } +} + /// Return `window_len` samples centred on `centre` within `buf`, padding /// with zeros outside the buffer. The IR peak is placed at /// `window_len / 2`. diff --git a/ac-rs/crates/ac-daemon/src/audio/fake.rs b/ac-rs/crates/ac-daemon/src/audio/fake.rs index 762f5d60..e58fbaea 100644 --- a/ac-rs/crates/ac-daemon/src/audio/fake.rs +++ b/ac-rs/crates/ac-daemon/src/audio/fake.rs @@ -153,6 +153,42 @@ fn period_size_override() -> Option { }) } +/// Opt-in, fake-only test hooks (#368): let an external integration test +/// simulate a low/no-SNR capture — the muted-route rig case #368's AC3 +/// needs reachable under `--fake-audio`, which by default always returns a +/// clean, noiseless delayed copy of the played signal (the loopback shape +/// every other τ test relies on). +/// +/// `AC_FAKE_TAU_GAIN_OVERRIDE`: scales the played-signal copy that would +/// otherwise land unattenuated at `delay_samples`. `1.0` (unset) keeps the +/// existing unity loopback; `0.0` simulates a fully muted route. +/// `AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE`: peak amplitude of broadband +/// dither added to every sample of `play_and_capture`'s output. `0.0` +/// (unset) is byte-identical to pre-#368 behaviour — with the gain also at +/// its default, `out[j] = 0.0 + s * 1.0 == s`. Combined with a `0.0` gain, +/// the deconvolved IR then contains only the dither at every position, so +/// the peak the daemon finds is indistinguishable from its own noise +/// floor, matching a real muted route's low pre-impulse SNR. +fn tau_gain_override() -> f32 { + static OVERRIDE: std::sync::OnceLock = std::sync::OnceLock::new(); + *OVERRIDE.get_or_init(|| { + std::env::var("AC_FAKE_TAU_GAIN_OVERRIDE") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(1.0) + }) +} + +fn tau_noise_amplitude_override() -> f32 { + static OVERRIDE: std::sync::OnceLock = std::sync::OnceLock::new(); + *OVERRIDE.get_or_init(|| { + std::env::var("AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(0.0) + }) +} + /// Which shared drain sequence a ring-mode capture should run. The variants /// differ only in whether they clear before waiting and how many channels /// they return — the point of routing all four through one enum is that the @@ -747,13 +783,28 @@ impl AudioEngine for FakeEngine { /// peaks at the expected offset. fn play_and_capture(&mut self, samples: &[f32], tail_s: f64) -> Result> { let delay_samples = next_loopback_delay_samples(); + let gain = tau_gain_override(); + let noise_amp = tau_noise_amplitude_override(); let tail = (tail_s * self.sample_rate as f64).round() as usize; let total = samples.len() + tail; let mut out = vec![0.0f32; total]; + if noise_amp > 0.0 { + // Deterministic LCG (same constants as `Stimulus::Noise` above), + // seeded from the delay so distinct fake sessions get distinct + // dither rather than sharing one repeated sequence. + let mut state: u64 = 0x9E3779B97F4A7C15 ^ (delay_samples as u64); + for v in out.iter_mut() { + state = state + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + let u = ((state >> 40) as f64 / (1u64 << 24) as f64) * 2.0 - 1.0; + *v = (noise_amp as f64 * u) as f32; + } + } for (i, &s) in samples.iter().enumerate() { let j = i + delay_samples; if j < total { - out[j] = s; + out[j] += s * gain; } } Ok(out) diff --git a/ac-rs/crates/ac-daemon/src/handlers/calibrate.rs b/ac-rs/crates/ac-daemon/src/handlers/calibrate.rs index 1fab9c79..10caa2d1 100644 --- a/ac-rs/crates/ac-daemon/src/handlers/calibrate.rs +++ b/ac-rs/crates/ac-daemon/src/handlers/calibrate.rs @@ -94,6 +94,33 @@ const TAU_MIN_HALF_WINDOW_S: f64 = 0.05; /// once measured against real noise floors. const TAU_EDGE_MARGIN_FRAC: f64 = 0.10; +/// Minimum pre-impulse SNR (dB) a τ lifecycle's deconvolved peak must clear +/// before the reading is trusted at all (#368). This replaces the old +/// pre-attempt `is_loopback` gate, which keyed on a *captured level* +/// against a unity-gain expectation — a proxy that a hot cable (3.01 dB +/// over unity) or a low-gain cable (4.19 dB under) both fail even though +/// both carry a perfectly real, measurable arrival, and that a loud but +/// uncorrelated interferer could still pass. This checks the quantity that +/// actually distinguishes "patched" from "not patched": whether the +/// deconvolution the τ sweep produced finds a peak that stands clear of its +/// own pre-impulse noise floor, measured under the exact drive and gain +/// conditions τ was measured under. +/// +/// Provenance: derived, not measured on this exact sweep. Two rig sessions +/// anchor it from different contexts — +/// `work/rig/rig-2026-08-22-tau-window-350-results.md` measured real +/// electrical-loopback τ SNR at 33.8–83.5 dB (the low end a JACK-startup- +/// transient artefact on the first reading after engine start, not a true +/// floor); #376's rig session measured a deconvolution noise cliff at +/// ~16 dB pre-impulse SNR on an unrelated (long-ESS, acoustic) path. 24 dB +/// splits that gap, rounded toward the reject side rather than the +/// midpoint — a false accept (a spurious peak silently stored in +/// `tau_history`) is more expensive than a false refuse (operator sees +/// "not measured" and re-runs). Wired through the same `tau-window- +/// override` env-override mechanism as `TAU_EDGE_MARGIN_FRAC` so a rig +/// session can correct it without a rebuild. +const TAU_SNR_THRESHOLD_DB: f64 = 24.0; + /// Rig-instrument overrides for the two τ window constants (#350). /// /// Compiled in only under the `tau-window-override` feature, which is off @@ -142,12 +169,25 @@ fn tau_edge_margin_frac() -> f64 { TAU_EDGE_MARGIN_FRAC } -/// Per-reading τ diagnostic (#350). `measure_tau` reports only the peak -/// position, so nothing on this path has ever recorded the SNR the peak -/// was located against — which is the quantity #350 exists to measure. -/// `floor` is defined exactly as `it_loopback_ir` and `ir_probe` define -/// it (max |x| over the leading eighth of the window) so the numbers -/// compare directly against #277's record. +#[cfg(feature = "tau-window-override")] +fn tau_snr_threshold_db() -> f64 { + tau_env_f64("AC_TAU_SNR_THRESHOLD_DB", TAU_SNR_THRESHOLD_DB) +} + +#[cfg(not(feature = "tau-window-override"))] +fn tau_snr_threshold_db() -> f64 { + TAU_SNR_THRESHOLD_DB +} + +/// Per-reading τ diagnostic (#350). `snr_db` is the real gate value — +/// `sweep::pre_impulse_snr_db` on this same peak, computed once by the +/// caller and passed in rather than recomputed here (#368: this used to +/// carry its own separate, leading-eighth-window SNR calculation, which +/// became a second implementation of "is this peak real" once the actual +/// gate needed the same number). `floor`/`far_end` below are a distinct, +/// unrelated diagnostic — max |x| over the leading eighth of the window, +/// defined exactly as `it_loopback_ir` and `ir_probe` define it, so those +/// numbers still compare directly against #277's record. #[cfg(feature = "tau-window-override")] fn tau_probe_log( ir: &[f64], @@ -156,13 +196,13 @@ fn tau_probe_log( window_len: usize, half: usize, sr: u32, + snr_db: f64, ) { let far_end = (ir.len() / 8).max(1); let floor = ir[..far_end] .iter() .map(|v| v.abs()) .fold(0.0_f64, f64::max); - let snr_db = 20.0 * (peak_abs / floor.max(1e-15)).log10(); let margin_frac = tau_edge_margin_frac(); let margin = (margin_frac * half as f64).round() as usize; let dist_from_end = window_len.saturating_sub(1).saturating_sub(peak_idx); @@ -188,6 +228,52 @@ fn tau_probe_log( eprintln!("------------------------"); } +/// Distinguishes a τ lifecycle's low-SNR refusal ([`check_peak_snr`]) from +/// a genuine measurement failure (#368), so `measure_tau_twice` can report +/// a distinct `cal_done.tau_state` (`"not_measured_low_snr"`) instead of +/// folding it into the generic `"error"` state a real engine/deconvolution +/// failure produces. Carried as a typed `anyhow::Error` payload, +/// downcast-recovered by `measure_tau_twice`, rather than a string match on +/// the message — a message wording change must not silently break the +/// state split. +#[derive(Debug, Clone, Copy)] +struct LowSnrRefusal { + snr_db: f64, + threshold_db: f64, +} + +impl std::fmt::Display for LowSnrRefusal { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "\u{3c4} peak pre-impulse SNR {:.2} dB is below the {:.2} dB threshold \u{2014} the \ + deconvolution did not find a peak distinguishable from noise, so no value is \ + reported", + self.snr_db, self.threshold_db + ) + } +} + +impl std::error::Error for LowSnrRefusal {} + +/// Refuse a τ lifecycle whose deconvolved peak sits below `threshold_db` +/// pre-impulse SNR — the peak cannot be trusted as a real arrival rather +/// than noise (#368, replacing the old pre-attempt `is_loopback` level +/// gate). Modeled on [`check_peak_within_window`]'s shape — a small pure +/// function over already-computed values, unit-testable without an +/// `AudioEngine` — and called before it in `measure_tau`, since a peak that +/// isn't real shouldn't be judged against the edge margin at all. +fn check_peak_snr(snr_db: f64, threshold_db: f64) -> anyhow::Result<()> { + if snr_db < threshold_db { + return Err(LowSnrRefusal { + snr_db, + threshold_db, + } + .into()); + } + Ok(()) +} + /// Refuse a peak sitting within `margin_frac` of the half-window of /// either edge of a `window_len`-sample gate. Pulled out of `measure_tau` /// so the edge case can be driven directly in tests without an @@ -212,12 +298,15 @@ fn check_peak_within_window( } /// Play a short ESS, deconvolve it, and return the interface round-trip -/// delay in seconds (peak of the linear IR, converted from samples). +/// delay in seconds (peak of the linear IR, converted from samples) +/// alongside that peak's pre-impulse SNR in dB (#368) — the caller needs +/// the SNR value even on success, since `cal_done` reports it on every +/// state that reached deconvolution, not only on a refusal. /// /// Reuses the Farina machinery from `ac_core::measurement::sweep` exactly /// as `plot_ir` does — see `handlers/audio/plot.rs` for the longer-form /// version of the same technique. -fn measure_tau(eng: &mut dyn AudioEngine, amp: f64) -> anyhow::Result { +fn measure_tau(eng: &mut dyn AudioEngine, amp: f64) -> anyhow::Result<(f64, f64)> { let sr = eng.sample_rate(); let f2_hz = (sr as f64 * 0.45).min(20_000.0); let params = SweepParams { @@ -250,24 +339,53 @@ fn measure_tau(eng: &mut dyn AudioEngine, amp: f64) -> anyhow::Result { .map(|(i, v)| (i, *v)) .max_by(|a, b| a.1.abs().partial_cmp(&b.1.abs()).unwrap()) .ok_or_else(|| anyhow::anyhow!("empty IR from τ sweep"))?; + let snr_db = ac_core::measurement::sweep::pre_impulse_snr_db(&irs.linear, peak_idx); #[cfg(feature = "tau-window-override")] - tau_probe_log(&irs.linear, peak_idx, peak_val.abs(), window_len, half, sr); + tau_probe_log( + &irs.linear, + peak_idx, + peak_val.abs(), + window_len, + half, + sr, + snr_db, + ); #[cfg(not(feature = "tau-window-override"))] let _ = peak_val; + // #368: the SNR gate runs before the edge-margin check — a peak that + // isn't distinguishable from noise shouldn't be judged against the + // window edge at all. + check_peak_snr(snr_db, tau_snr_threshold_db())?; check_peak_within_window(peak_idx, window_len, tau_edge_margin_frac())?; let offset_samples = peak_idx as i64 - half as i64; - Ok(offset_samples as f64 / sr as f64) + Ok((offset_samples as f64 / sr as f64, snr_db)) } -/// Outcome of one independent τ lifecycle attempt (#347): either both -/// readings were taken and compared, or a lifecycle itself failed (engine -/// start / measurement error) before a comparison was possible. +/// Outcome of one independent τ lifecycle attempt (#347): both readings +/// were taken and compared, one lifecycle's peak was below the SNR +/// threshold (#368), or a lifecycle itself failed (engine start / +/// measurement error) before either could happen. enum TauAttempt { Compared { conditions: TauConditions, reading1_s: f64, reading2_s: f64, comparison: TauComparison, + /// The worse (lower) of the two lifecycles' pre-impulse SNR. Both + /// necessarily cleared [`TAU_SNR_THRESHOLD_DB`] — a lifecycle that + /// didn't would have produced [`TauAttempt::LowSnr`] instead — so + /// this is a diagnostic figure alongside the comparison, not a + /// second gate. + pre_impulse_snr_db: f64, + }, + /// A lifecycle's peak sits below the SNR threshold (#368). Short- + /// circuits the same way [`TauAttempt::Error`] does: the second + /// lifecycle does not run once the first has already refused, and + /// `conditions` is `Some` only when the refusal happened on the second + /// lifecycle (mirroring `Error`'s own short-circuit shape below). + LowSnr { + conditions: Option, + pre_impulse_snr_db: f64, }, Error { conditions: Option, @@ -290,7 +408,7 @@ fn measure_tau_twice( in_port: &str, amp: f64, ) -> TauAttempt { - let run_once = || -> anyhow::Result<(f64, TauConditions)> { + let run_once = || -> anyhow::Result<((f64, f64), TauConditions)> { let mut eng = make_engine(fake); eng.start(std::slice::from_ref(&out_port.to_string()), Some(in_port))?; let conditions = TauConditions { @@ -304,24 +422,36 @@ fn measure_tau_twice( let reading = measure_tau(&mut *eng, amp); eng.set_silence(); eng.stop(); - reading.map(|t| (t, conditions)) + reading.map(|r| (r, conditions)) }; - let (reading1_s, conditions) = match run_once() { + let ((reading1_s, snr1_db), conditions) = match run_once() { Ok(r) => r, Err(e) => { - return TauAttempt::Error { - conditions: None, - message: format!("\u{3c4} measurement failed (reading 1 of 2): {e}"), + return match e.downcast_ref::() { + Some(refusal) => TauAttempt::LowSnr { + conditions: None, + pre_impulse_snr_db: refusal.snr_db, + }, + None => TauAttempt::Error { + conditions: None, + message: format!("\u{3c4} measurement failed (reading 1 of 2): {e}"), + }, } } }; - let (reading2_s, conditions2) = match run_once() { + let ((reading2_s, snr2_db), conditions2) = match run_once() { Ok(r) => r, Err(e) => { - return TauAttempt::Error { - conditions: Some(conditions), - message: format!("\u{3c4} measurement failed (reading 2 of 2): {e}"), + return match e.downcast_ref::() { + Some(refusal) => TauAttempt::LowSnr { + conditions: Some(conditions), + pre_impulse_snr_db: refusal.snr_db, + }, + None => TauAttempt::Error { + conditions: Some(conditions), + message: format!("\u{3c4} measurement failed (reading 2 of 2): {e}"), + }, } } }; @@ -336,6 +466,7 @@ fn measure_tau_twice( reading1_s, reading2_s, comparison, + pre_impulse_snr_db: snr1_db.min(snr2_db), } } @@ -344,6 +475,11 @@ fn measure_tau_twice( /// appended to `tau_history`. One reading is never a storable outcome /// (#347): `state == "measured"` only when two independent lifecycles /// agreed, and `tau_s` / `agreement_count` are `Some` only in that case. +/// +/// `pre_impulse_snr_db` / `snr_threshold_db` (#368) are `Some` on every +/// state that reached deconvolution at least once (`measured`, +/// `not_measured_low_snr`, `disagree_*`) and `None` on `error`, which can +/// fail before a peak was ever located. struct TauOutcome { state: &'static str, conditions: Option, @@ -354,40 +490,41 @@ struct TauOutcome { delta_samples: Option, periods: Option, error: Option, + pre_impulse_snr_db: Option, + snr_threshold_db: Option, } -impl TauOutcome { - fn not_measured_no_loopback() -> Self { - Self { - state: "not_measured_no_loopback", - conditions: None, +/// Turn a τ attempt into the [`TauOutcome`] `calibrate` reports — the exact +/// decision #281 QA flagged as untestable because it was inlined in the +/// worker closure, reachable only through a full daemon spawn. `attempt` +/// always runs (#368): τ used to be gated on the `is_loopback` flag +/// established at step 2, a captured-level proxy that a hot or low-gain +/// but genuinely patched loopback could fail; the gate now lives inside +/// `measure_tau` itself, on the deconvolved peak's own SNR, so it applies +/// regardless of what step 2 observed. +fn tau_result(attempt: impl FnOnce() -> TauAttempt) -> TauOutcome { + match attempt() { + TauAttempt::Error { + conditions, + message, + } => TauOutcome { + state: "error", + conditions, tau_s: None, agreement_count: None, reading1_s: None, reading2_s: None, delta_samples: None, periods: None, - error: None, - } - } -} - -/// Turn the loopback flag established at step 2 into the [`TauOutcome`] -/// `calibrate` reports — the exact decision #281 QA flagged as untestable -/// because it was inlined in the worker closure, reachable only through a -/// full daemon spawn. `attempt` is only called when `is_loopback`, matching -/// the worker's original behaviour of never running the τ sweep on a run -/// with no loopback detected. -fn tau_result(is_loopback: bool, attempt: impl FnOnce() -> TauAttempt) -> TauOutcome { - if !is_loopback { - return TauOutcome::not_measured_no_loopback(); - } - match attempt() { - TauAttempt::Error { + error: Some(message), + pre_impulse_snr_db: None, + snr_threshold_db: None, + }, + TauAttempt::LowSnr { conditions, - message, + pre_impulse_snr_db, } => TauOutcome { - state: "error", + state: "not_measured_low_snr", conditions, tau_s: None, agreement_count: None, @@ -395,13 +532,16 @@ fn tau_result(is_loopback: bool, attempt: impl FnOnce() -> TauAttempt) -> TauOut reading2_s: None, delta_samples: None, periods: None, - error: Some(message), + error: None, + pre_impulse_snr_db: Some(pre_impulse_snr_db), + snr_threshold_db: Some(tau_snr_threshold_db()), }, TauAttempt::Compared { conditions, reading1_s, reading2_s, comparison: TauComparison::Agree, + pre_impulse_snr_db, } => TauOutcome { state: "measured", conditions: Some(conditions), @@ -417,12 +557,15 @@ fn tau_result(is_loopback: bool, attempt: impl FnOnce() -> TauAttempt) -> TauOut delta_samples: None, periods: None, error: None, + pre_impulse_snr_db: Some(pre_impulse_snr_db), + snr_threshold_db: Some(tau_snr_threshold_db()), }, TauAttempt::Compared { conditions, reading1_s, reading2_s, comparison: TauComparison::Disagree(d), + pre_impulse_snr_db, } => TauOutcome { state: if d.periods.is_some() { "disagree_period_shift" @@ -437,6 +580,8 @@ fn tau_result(is_loopback: bool, attempt: impl FnOnce() -> TauAttempt) -> TauOut delta_samples: Some(d.delta_samples), periods: d.periods, error: Some(d.message()), + pre_impulse_snr_db: Some(pre_impulse_snr_db), + snr_threshold_db: Some(tau_snr_threshold_db()), }, } } @@ -597,7 +742,7 @@ pub fn calibrate(state: &ServerState, cmd: &Value) -> Value { } // Fallback conditions for the `cal_done` wire frame when τ isn't - // measured this run (no-loopback, or a lifecycle error before any + // measured this run (low SNR, or a lifecycle error before any // conditions were captured) — ZMQ.md requires `tau_sample_rate` / // `tau_period_size` present regardless of `tau_state`. let fallback_sample_rate = eng.sample_rate(); @@ -606,20 +751,21 @@ pub fn calibrate(state: &ServerState, cmd: &Value) -> Value { eng.set_silence(); eng.stop(); - // τ (interface latency, #281/#347) — not prompt-driven, so it - // piggybacks on the loopback state established above rather than - // adding a third interactive step. Measured whenever a loopback was - // detected this run, regardless of whether either voltage prompt - // was answered or skipped — the cheap-refresh path (#279: both - // prompts skipped) still refreshes τ. #347: a single reading is not - // a measurement of τ on this stack, so this now runs two - // independent client lifecycles (`measure_tau_twice`), decoupled - // from the voltage-cal `eng` above (already stopped) — see that - // function's doc for why the lifecycle boundary matters. + // τ (interface latency, #281/#347) — not prompt-driven, so it does + // not add a third interactive step. Always attempted regardless of + // the loopback state step 2 established (#368: τ used to be gated + // on that captured-level proxy; it is now gated on its own + // deconvolved peak's SNR instead, inside `measure_tau` itself) and + // regardless of whether either voltage prompt was answered or + // skipped — the cheap-refresh path (#279: both prompts skipped) + // still refreshes τ. #347: a single reading is not a measurement of + // τ on this stack, so this now runs two independent client + // lifecycles (`measure_tau_twice`), decoupled from the voltage-cal + // `eng` above (already stopped) — see that function's doc for why + // the lifecycle boundary matters. let ref_amp = ac_core::shared::generator::dbfs_to_amplitude(ref_dbfs); - let tau_outcome = tau_result(is_loopback, || { - measure_tau_twice(fake, cfg.device, &out_port, &in_port, ref_amp) - }); + let tau_outcome = + tau_result(|| measure_tau_twice(fake, cfg.device, &out_port, &in_port, ref_amp)); // Convert from "Vrms at the played/captured dBFS" → "Vrms at 0 dBFS". let out_scale = 1.0 / ac_core::shared::generator::dbfs_to_amplitude(ref_dbfs); @@ -690,6 +836,12 @@ pub fn calibrate(state: &ServerState, cmd: &Value) -> Value { if let Some(p) = tau_outcome.periods { cal_done_frame["tau_periods"] = json!(p); } + if let Some(snr) = tau_outcome.pre_impulse_snr_db { + cal_done_frame["tau_pre_impulse_snr_db"] = json!(snr); + } + if let Some(threshold) = tau_outcome.snr_threshold_db { + cal_done_frame["tau_snr_threshold_db"] = json!(threshold); + } if let Some(ref e) = save_err { cal_done_frame["error"] = json!(e); } @@ -983,29 +1135,26 @@ mod tests { } } - /// #281 QA correctness issue 3: the no-loopback path is hard to drive - /// end-to-end under `--fake-audio` (the fake backend's step-2 capture - /// always reads as loopback-shaped), so pin the decision down directly - /// instead. `attempt` must not run at all when there's no loopback. + /// #368: replaces `tau_result_no_loopback_short_circuits_without_ + /// measuring` — the pre-attempt `is_loopback` gate this pinned down is + /// gone, `attempt` now always runs, and a low-SNR peak is refused + /// *inside* the attempt instead. This is the "measured because the + /// gate was deleted" guard AC8 of #368 asks for at the `tau_result` + /// level: even though `attempt` ran and returned a real conditions/SNR + /// pair, a `LowSnr` outcome must still surface as + /// `not_measured_low_snr`, not get folded into `measured`. #[test] - fn tau_result_no_loopback_short_circuits_without_measuring() { - let mut called = false; - let outcome = tau_result(false, || { - called = true; - TauAttempt::Compared { - conditions: dummy_conditions(), - reading1_s: 0.001, - reading2_s: 0.001, - comparison: TauComparison::Agree, - } + fn tau_result_low_snr_reports_new_state_and_fields() { + let outcome = tau_result(|| TauAttempt::LowSnr { + conditions: Some(dummy_conditions()), + pre_impulse_snr_db: -3.45, }); - assert_eq!(outcome.state, "not_measured_no_loopback"); + assert_eq!(outcome.state, "not_measured_low_snr"); assert_eq!(outcome.tau_s, None); assert_eq!(outcome.error, None); - assert!( - !called, - "attempt must not run when no loopback was detected" - ); + assert_eq!(outcome.pre_impulse_snr_db, Some(-3.45)); + assert_eq!(outcome.snr_threshold_db, Some(TAU_SNR_THRESHOLD_DB)); + assert!(outcome.conditions.is_some()); } /// #347: two independent readings agreeing is what "measured" means @@ -1013,11 +1162,12 @@ mod tests { /// `agreement_count` must always be `Some(2)` alongside it. #[test] fn tau_result_agreeing_readings_reports_measured_with_agreement_count() { - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.000_667, reading2_s: 0.000_667, comparison: TauComparison::Agree, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state, "measured"); assert_eq!(outcome.tau_s, Some(0.000_667)); @@ -1029,15 +1179,19 @@ mod tests { // correctness 1). assert_eq!(outcome.delta_samples, None); assert_eq!(outcome.periods, None); + // #368: present on every state that reached deconvolution. + assert_eq!(outcome.pre_impulse_snr_db, Some(40.0)); + assert_eq!(outcome.snr_threshold_db, Some(TAU_SNR_THRESHOLD_DB)); } #[test] fn tau_result_averages_two_agreeing_readings() { - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.001_000_00, reading2_s: 0.001_000_02, comparison: TauComparison::Agree, + pre_impulse_snr_db: 40.0, }); let tau_s = outcome.tau_s.expect("measured"); assert!((tau_s - 0.001_000_01).abs() < 1e-9); @@ -1051,11 +1205,12 @@ mod tests { fn tau_result_period_shift_disagreement_refuses_and_names_the_period() { let comparison = compare_tau_readings(4262.064 / 96_000.0, 5286.064 / 96_000.0, 96_000, Some(1024)); - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 4262.064 / 96_000.0, reading2_s: 5286.064 / 96_000.0, comparison, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state, "disagree_period_shift"); assert_eq!(outcome.tau_s, None); @@ -1072,11 +1227,12 @@ mod tests { #[test] fn tau_result_non_period_disagreement_is_a_different_state() { let comparison = compare_tau_readings(0.0, 0.000_5, 48_000, Some(1024)); - let outcome = tau_result(true, || TauAttempt::Compared { + let outcome = tau_result(|| TauAttempt::Compared { conditions: dummy_conditions(), reading1_s: 0.0, reading2_s: 0.000_5, comparison, + pre_impulse_snr_db: 40.0, }); assert_eq!(outcome.state, "disagree_other"); assert_eq!(outcome.tau_s, None); @@ -1087,7 +1243,7 @@ mod tests { #[test] fn tau_result_loopback_err_reports_error_state_and_message() { - let outcome = tau_result(true, || TauAttempt::Error { + let outcome = tau_result(|| TauAttempt::Error { conditions: None, message: "\u{3c4} measurement failed (reading 1 of 2): timeout".to_string(), }); @@ -1098,6 +1254,32 @@ mod tests { msg.contains("timeout"), "error message should name the failure: {msg}" ); + // #368: absent on error — a lifecycle can fail before a peak was + // ever located. + assert_eq!(outcome.pre_impulse_snr_db, None); + assert_eq!(outcome.snr_threshold_db, None); + } + + /// #368: `check_peak_snr` mirrors `check_peak_within_window`'s shape — + /// pin its boundary the same way (refuses strictly below, accepts at + /// and above). + #[test] + fn check_peak_snr_refuses_below_threshold() { + assert!(check_peak_snr(23.99, 24.0).is_err()); + } + + #[test] + fn check_peak_snr_accepts_at_and_above_threshold() { + assert!(check_peak_snr(24.0, 24.0).is_ok()); + assert!(check_peak_snr(83.5, 24.0).is_ok()); + } + + /// The rig's own measured muted-route reading (#368 triage: drive + /// -30 dBFS, captured -83.8 dBFS) — a concrete refusal, not just a + /// boundary probe. + #[test] + fn check_peak_snr_refuses_the_rigs_measured_muted_route() { + assert!(check_peak_snr(-3.45, TAU_SNR_THRESHOLD_DB).is_err()); } /// #340 AC4/AC-test: a peak pinned at the window's far edge — exactly diff --git a/ac-rs/crates/ac-daemon/tests/it_protocol.rs b/ac-rs/crates/ac-daemon/tests/it_protocol.rs index d5137235..d586c071 100644 --- a/ac-rs/crates/ac-daemon/tests/it_protocol.rs +++ b/ac-rs/crates/ac-daemon/tests/it_protocol.rs @@ -1158,8 +1158,9 @@ fn calibrate_measures_tau_against_fake_loopback_delay() { "output_channel": 0, "input_channel": 0})); assert_eq!(r["ok"], json!(true)); - // Both prompts skipped — τ must still be measured (it keys only on - // `is_loopback`, established at step 2, independent of the replies). + // Both prompts skipped — τ must still be measured (#368: it is not + // gated on the step-2 loopback flag at all, and never was gated on + // either voltage reply). for step in 1..=2 { c.wait_for_topic("cal_prompt", Duration::from_secs(5)) .unwrap_or_else(|| panic!("step {step} prompt")); @@ -1186,6 +1187,83 @@ fn calibrate_measures_tau_against_fake_loopback_delay() { // ZMQ.md: tau_delta_samples is present only on disagree_* — an Agree // outcome must not serialize a stray Some(0) (QA #348 correctness 1). assert!(done.get("tau_delta_samples").is_none(), "frame: {done}"); + // #368: present whenever a lifecycle reached deconvolution, including + // a clean "measured" run — not only on the refusal leg. + assert!( + done["tau_pre_impulse_snr_db"].as_f64().is_some(), + "frame: {done}" + ); + assert!( + done["tau_snr_threshold_db"].as_f64().is_some(), + "frame: {done}" + ); +} + +/// #368: the pre-attempt `is_loopback` level gate is gone — τ is refused +/// only when the deconvolved peak itself sits below +/// `tau_snr_threshold_db` pre-impulse SNR. This drives that refusal +/// end-to-end through `--fake-audio`'s new low-SNR test hooks +/// (`AC_FAKE_TAU_GAIN_OVERRIDE` / `AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE`, +/// `audio/fake.rs`): a muted route (gain 0, dither only) must come back +/// `not_measured_low_snr`, not the plausible-looking `measured` a deleted +/// gate would still produce, since the fake backend's default loopback +/// shape has no other codepath capable of returning anything but a clean +/// peak. Pairs with `calibrate_measures_tau_against_fake_loopback_delay` +/// above (a passing, high-SNR loopback) to distinguish "measured because +/// SNR is genuinely adequate" from "measured because the gate was +/// deleted." +#[test] +fn calibrate_reports_not_measured_low_snr_on_muted_fake_loopback() { + let d = Daemon::spawn_with_env(&[ + ("AC_FAKE_TAU_GAIN_OVERRIDE", "0.0"), + ("AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE", "0.01"), + ]); + let c = Client::new(&d); + + let r = c.call(json!({"cmd": "calibrate", "ref_dbfs": -10.0, + "output_channel": 0, "input_channel": 0})); + assert_eq!(r["ok"], json!(true)); + + for step in 1..=2 { + c.wait_for_topic("cal_prompt", Duration::from_secs(5)) + .unwrap_or_else(|| panic!("step {step} prompt")); + let _ = c.call(json!({"cmd": "cal_reply", "vrms": null})); + } + let done = c + .wait_for_topic("cal_done", Duration::from_secs(5)) + .expect("cal_done frame"); + + assert_eq!( + done["tau_state"], + json!("not_measured_low_snr"), + "frame: {done}" + ); + assert_eq!( + done["tau_s"], + json!(null), + "a low-SNR refusal must not report a τ: {done}" + ); + let snr = done["tau_pre_impulse_snr_db"] + .as_f64() + .expect("tau_pre_impulse_snr_db present on a refusal that reached deconvolution"); + let threshold = done["tau_snr_threshold_db"] + .as_f64() + .expect("tau_snr_threshold_db present alongside it"); + assert!( + snr < threshold, + "refused SNR {snr} should be below the {threshold} dB threshold: {done}" + ); + + // Refused, not stored — no entry in tau_history at all. + let cal_path = d.home.join(".config").join("ac").join("cal.json"); + let after = read_cal_entry(&cal_path); + assert!( + after.get("tau_history").is_none() + || after["tau_history"] + .as_array() + .is_some_and(|a| a.is_empty()), + "a low-SNR refusal must not append to tau_history: {after}" + ); } /// QA #348 test-coverage gap: every other disagreement test drives @@ -1253,9 +1331,9 @@ fn calibrate_reports_disagree_period_shift_end_to_end() { /// voltage prompts skipped still refreshes stored state cheaply) is an /// explicit issue acceptance criterion for τ too — a skipped-both-prompts /// run must still append a fresh `tau_history` entry, not just leave the -/// voltage legs alone. Previously asserted only by reading the code (τ's -/// branch is keyed on `is_loopback`, not on either reply); this test pins -/// it down on the wire and on disk. +/// voltage legs alone. Previously asserted only by reading the code (τ is +/// never keyed on either voltage reply, and since #368 not on the step-2 +/// loopback flag either); this test pins it down on the wire and on disk. #[test] fn calibrate_cheap_refresh_still_measures_tau() { let d = Daemon::spawn(); From 8c36a06051bd21b008514459220e680e33cc347c Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Sun, 23 Aug 2026 14:47:52 +0000 Subject: [PATCH 2/5] =?UTF-8?q?test:=20close=20AC8=20gap=20=E2=80=94=20off?= =?UTF-8?q?-unity=20loopback=20proves=20the=20SNR=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA on PR #384 (request-changes): the PR's only "measured" fake-audio test runs at the fake backend's default unity gain, the one case the old is_loopback ±2 dB gate already handled correctly. A regression that reintroduced a captured-level check keyed near unity would pass every existing test. Add calibrate_measures_tau_on_hot_off_unity_fake_loopback, driving the issue's own +3.01 dB hot-loopback case through AC_FAKE_TAU_GAIN_OVERRIDE and asserting measured. Paired with the existing muted-route refusal test, this closes AC8: one test proves "measured because SNR is genuinely adequate" at an off-unity level, the other proves the gate isn't just deleted. Also add a rig-verify-queue.md block for the constants-guard note QA raised: TAU_SNR_THRESHOLD_DB is derived from a different sweep configuration than the one it gates, and no rig block covered verifying it against calibrate's own τ path until now. Co-Authored-By: Claude Sonnet 5 --- ac-rs/crates/ac-daemon/tests/it_protocol.rs | 39 +++++++++++++++++++++ work/rig/rig-verify-queue.md | 17 +++++++++ 2 files changed, 56 insertions(+) diff --git a/ac-rs/crates/ac-daemon/tests/it_protocol.rs b/ac-rs/crates/ac-daemon/tests/it_protocol.rs index d586c071..89398955 100644 --- a/ac-rs/crates/ac-daemon/tests/it_protocol.rs +++ b/ac-rs/crates/ac-daemon/tests/it_protocol.rs @@ -1266,6 +1266,45 @@ fn calibrate_reports_not_measured_low_snr_on_muted_fake_loopback() { ); } +/// #368 AC8 (QA request-changes on PR #384): closes the gap the muted-route +/// test alone leaves. `calibrate_measures_tau_against_fake_loopback_delay` +/// above passes at the fake backend's default unity gain — exactly the one +/// case the old `is_loopback` ±2 dB gate already handled correctly, so it +/// cannot tell "measured because SNR is genuinely adequate" apart from +/// "measured because the gate was deleted" for any off-unity level. This +/// drives the +3.01 dB hot loopback from the issue's own rig case (drive +/// -30 dBFS, captured -30.0 dBFS) through `AC_FAKE_TAU_GAIN_OVERRIDE` and +/// asserts `measured` — a regression that reintroduced any captured-level +/// check keyed near unity would fail this without touching the muted-route +/// test. +#[test] +fn calibrate_measures_tau_on_hot_off_unity_fake_loopback() { + let d = Daemon::spawn_with_env(&[ + ("AC_FAKE_TAU_GAIN_OVERRIDE", "1.4142135623730951"), // +3.01 dB + ]); + let c = Client::new(&d); + + let r = c.call(json!({"cmd": "calibrate", "ref_dbfs": -30.0, + "output_channel": 0, "input_channel": 0})); + assert_eq!(r["ok"], json!(true)); + + for step in 1..=2 { + c.wait_for_topic("cal_prompt", Duration::from_secs(5)) + .unwrap_or_else(|| panic!("step {step} prompt")); + let _ = c.call(json!({"cmd": "cal_reply", "vrms": null})); + } + let done = c + .wait_for_topic("cal_done", Duration::from_secs(5)) + .expect("cal_done frame"); + + assert_eq!( + done["tau_state"], + json!("measured"), + "3.01 dB hot must not be refused (#368 AC1): {done}" + ); + assert!(done["tau_s"].as_f64().is_some(), "frame: {done}"); +} + /// QA #348 test-coverage gap: every other disagreement test drives /// `compare_tau_readings` or `tau_result` as a pure function, never /// `measure_tau_twice` itself — the function that actually spins up two diff --git a/work/rig/rig-verify-queue.md b/work/rig/rig-verify-queue.md index 854d7fd4..57b03d6e 100644 --- a/work/rig/rig-verify-queue.md +++ b/work/rig/rig-verify-queue.md @@ -26,6 +26,23 @@ only planned run producing a legitimately gated ring, which is the case the dropped onset guard must not suppress, so it carries per-frame `median_value` / `negative_lag_median` as well. Full statement in block 4. +- **#368's `TAU_SNR_THRESHOLD_DB` constant — QA on PR #384 (2026-08-23) + flagged it `derived`, not measured on the sweep configuration it gates.** + Its two anchors (33.8–83.5 dB electrical loopback, ~16 dB #376 acoustic + cliff) come from a different window-length rig session + (`rig-2026-08-22-tau-window-350-results.md`) and a different, longer-ESS + acoustic path — neither is `calibrate`'s own short-ESS electrical τ path. + + Run `calibrate`'s actual τ path against the three cases the issue + measured: hot loopback (+3.01 dB), low-gain loopback (-4.19 dB), muted + route (-83.8 dBFS). Record `tau_pre_impulse_snr_db` for each. + + > **Pass: both real loopbacks read at or above 24 dB, and the muted + > route reads below it.** Either real loopback's SNR coming back under + > 24 dB would wrongly refuse a working cable; the muted route's SNR + > coming back over 24 dB would wrongly accept noise as a peak. Both are + > falsifications of the current constant, not readouts to shrug past. + Two things session 3 raised that no block here covers yet: - **The cable change, and the one measurement that verifies it — #243.** Move From dae1f47a645607d0f20ab03a1587fdb15781cc75 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Sun, 30 Aug 2026 00:58:45 +0000 Subject: [PATCH 3/5] chore: refresh .agents, bin, .claude and rig from main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brings this worktree's agent specs and helper scripts up to main: the codex-qa role and its runner, triage.sh, and the master.sh / common.sh / session.sh updates, plus .claude/settings.json. Drops work/sessions/ — session records now live outside the repo, under ac-wt/session/ — and moves work/rig/rig-verify-queue.md to rig/ to match main's layout. The rest of work/rig/ is unchanged; main still tracks it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01B898jEzqgxF4ALLY8m7waY --- .agents/AGENTS.md | 274 +----------------- .agents/architect.md | 50 +--- .agents/codex-qa.md | 36 +-- .agents/developer.md | 58 +--- .agents/qa.md | 234 ++------------- .agents/rig.md | 34 ++- .agents/triage.md | 10 +- .claude/settings.json | 36 ++- bin/codex-qa.sh | 0 bin/common.sh | 57 +++- bin/implement.sh | 24 +- bin/master.sh | 22 +- bin/revise.sh | 1 + bin/session.sh | 37 ++- bin/triage.sh | 0 {work/rig => rig}/rig-verify-queue.md | 38 +-- .../2026-08-12-architect-issue-281.md | 15 - .../2026-08-12-architect-issue-282.md | 6 - work/sessions/2026-08-12-developer-1937543.md | 4 - .../2026-08-12-developer-issue-282.md | 4 - .../2026-08-12-developer-pr-296-rev.md | 17 -- work/sessions/2026-08-12-qa-1954223.md | 4 - work/sessions/2026-08-12-qa-pr-294.md | 13 - work/sessions/2026-08-12-qa-pr-296-delta.md | 17 -- work/sessions/2026-08-12-qa-pr-296.md | 15 - work/sessions/2026-08-12-ux-281.md | 5 - work/sessions/2026-08-12-ux-282.md | 6 - .../2026-08-13-architect-issue-280.md | 11 - .../2026-08-13-developer-issue-193.md | 11 - .../2026-08-13-developer-issue-226.md | 5 - .../2026-08-13-developer-issue-280.md | 12 - .../2026-08-13-developer-issue-281.md | 5 - .../2026-08-13-developer-pr-298-rev.md | 5 - .../2026-08-13-developer-pr-299-rev.md | 12 - .../2026-08-13-developer-pr-301-rev.md | 5 - work/sessions/2026-08-13-qa-pr-298-delta.md | 17 -- work/sessions/2026-08-13-qa-pr-298.md | 9 - work/sessions/2026-08-13-qa-pr-299-delta.md | 7 - work/sessions/2026-08-13-qa-pr-299.md | 15 - work/sessions/2026-08-13-qa-pr-300.md | 16 - work/sessions/2026-08-13-qa-pr-301.md | 13 - work/sessions/2026-08-13-ux-193.md | 18 -- work/sessions/2026-08-13-ux-280.md | 12 - work/sessions/2026-08-13-ux-281.md | 7 - .../2026-08-14-developer-pr-301-rev.md | 5 - work/sessions/2026-08-14-qa-pr-301-delta.md | 7 - .../2026-08-15-architect-issue-284.md | 17 -- .../2026-08-15-developer-issue-277.md | 17 -- .../2026-08-15-developer-issue-284.md | 14 - .../2026-08-15-developer-pr-305-rev.md | 5 - work/sessions/2026-08-15-qa-pr-304.md | 20 -- work/sessions/2026-08-15-qa-pr-305-delta.md | 7 - work/sessions/2026-08-15-qa-pr-305.md | 17 -- work/sessions/2026-08-15-ux-284.md | 7 - .../2026-08-16-architect-issue-286.md | 9 - .../2026-08-16-architect-issue-308.md | 18 -- .../2026-08-16-developer-issue-285.md | 12 - .../2026-08-16-developer-issue-286.md | 18 -- .../2026-08-16-developer-issue-287.md | 7 - .../2026-08-16-developer-issue-288.md | 5 - .../2026-08-16-developer-issue-308.md | 19 -- .../2026-08-16-developer-issue-311.md | 5 - .../2026-08-16-developer-issue-312.md | 5 - .../2026-08-16-developer-issue-313.md | 5 - .../2026-08-16-developer-issue-314.md | 5 - .../2026-08-16-developer-issue-315.md | 5 - .../2026-08-16-developer-pr-306-rev.md | 11 - .../2026-08-16-developer-pr-309-rev.md | 13 - .../2026-08-16-developer-pr-316-rev.md | 5 - work/sessions/2026-08-16-qa-pr-306.md | 7 - work/sessions/2026-08-16-qa-pr-307.md | 14 - work/sessions/2026-08-16-qa-pr-309-delta.md | 13 - work/sessions/2026-08-16-qa-pr-309.md | 11 - work/sessions/2026-08-16-qa-pr-316-delta.md | 12 - work/sessions/2026-08-16-qa-pr-316.md | 13 - work/sessions/2026-08-16-qa-pr-317.md | 7 - work/sessions/2026-08-16-qa-pr-318.md | 9 - work/sessions/2026-08-16-qa-pr-319.md | 17 -- work/sessions/2026-08-16-qa-pr-320.md | 5 - work/sessions/2026-08-16-qa-pr-322.md | 17 -- work/sessions/2026-08-16-ux-286.md | 11 - work/sessions/2026-08-16-ux-308.md | 10 - .../2026-08-17-architect-issue-297.md | 9 - .../2026-08-17-architect-issue-321.md | 9 - .../2026-08-17-architect-issue-329.md | 5 - .../2026-08-17-architect-issue-330.md | 5 - .../2026-08-17-developer-issue-295.md | 5 - .../2026-08-17-developer-issue-297.md | 14 - .../2026-08-17-developer-issue-321.md | 5 - .../2026-08-17-developer-issue-326.md | 5 - .../2026-08-17-developer-issue-327.md | 5 - .../2026-08-17-developer-issue-328.md | 5 - .../2026-08-17-developer-issue-329.md | 7 - .../2026-08-17-developer-issue-330.md | 11 - .../2026-08-17-developer-pr-322-rev.md | 13 - .../2026-08-17-developer-pr-333-rev.md | 5 - .../2026-08-17-developer-pr-335-rev.md | 7 - work/sessions/2026-08-17-qa-pr-322-delta.md | 14 - work/sessions/2026-08-17-qa-pr-323.md | 15 - work/sessions/2026-08-17-qa-pr-324.md | 7 - work/sessions/2026-08-17-qa-pr-331.md | 9 - work/sessions/2026-08-17-qa-pr-332.md | 11 - work/sessions/2026-08-17-qa-pr-333-delta.md | 13 - work/sessions/2026-08-17-qa-pr-333.md | 17 -- work/sessions/2026-08-17-qa-pr-334.md | 9 - work/sessions/2026-08-17-qa-pr-335-delta.md | 5 - work/sessions/2026-08-17-qa-pr-335.md | 13 - work/sessions/2026-08-17-ux-297.md | 5 - work/sessions/2026-08-17-ux-321.md | 14 - 109 files changed, 265 insertions(+), 1578 deletions(-) mode change 100644 => 100755 bin/codex-qa.sh mode change 100644 => 100755 bin/master.sh mode change 100644 => 100755 bin/revise.sh mode change 100644 => 100755 bin/triage.sh rename {work/rig => rig}/rig-verify-queue.md (96%) delete mode 100644 work/sessions/2026-08-12-architect-issue-281.md delete mode 100644 work/sessions/2026-08-12-architect-issue-282.md delete mode 100644 work/sessions/2026-08-12-developer-1937543.md delete mode 100644 work/sessions/2026-08-12-developer-issue-282.md delete mode 100644 work/sessions/2026-08-12-developer-pr-296-rev.md delete mode 100644 work/sessions/2026-08-12-qa-1954223.md delete mode 100644 work/sessions/2026-08-12-qa-pr-294.md delete mode 100644 work/sessions/2026-08-12-qa-pr-296-delta.md delete mode 100644 work/sessions/2026-08-12-qa-pr-296.md delete mode 100644 work/sessions/2026-08-12-ux-281.md delete mode 100644 work/sessions/2026-08-12-ux-282.md delete mode 100644 work/sessions/2026-08-13-architect-issue-280.md delete mode 100644 work/sessions/2026-08-13-developer-issue-193.md delete mode 100644 work/sessions/2026-08-13-developer-issue-226.md delete mode 100644 work/sessions/2026-08-13-developer-issue-280.md delete mode 100644 work/sessions/2026-08-13-developer-issue-281.md delete mode 100644 work/sessions/2026-08-13-developer-pr-298-rev.md delete mode 100644 work/sessions/2026-08-13-developer-pr-299-rev.md delete mode 100644 work/sessions/2026-08-13-developer-pr-301-rev.md delete mode 100644 work/sessions/2026-08-13-qa-pr-298-delta.md delete mode 100644 work/sessions/2026-08-13-qa-pr-298.md delete mode 100644 work/sessions/2026-08-13-qa-pr-299-delta.md delete mode 100644 work/sessions/2026-08-13-qa-pr-299.md delete mode 100644 work/sessions/2026-08-13-qa-pr-300.md delete mode 100644 work/sessions/2026-08-13-qa-pr-301.md delete mode 100644 work/sessions/2026-08-13-ux-193.md delete mode 100644 work/sessions/2026-08-13-ux-280.md delete mode 100644 work/sessions/2026-08-13-ux-281.md delete mode 100644 work/sessions/2026-08-14-developer-pr-301-rev.md delete mode 100644 work/sessions/2026-08-14-qa-pr-301-delta.md delete mode 100644 work/sessions/2026-08-15-architect-issue-284.md delete mode 100644 work/sessions/2026-08-15-developer-issue-277.md delete mode 100644 work/sessions/2026-08-15-developer-issue-284.md delete mode 100644 work/sessions/2026-08-15-developer-pr-305-rev.md delete mode 100644 work/sessions/2026-08-15-qa-pr-304.md delete mode 100644 work/sessions/2026-08-15-qa-pr-305-delta.md delete mode 100644 work/sessions/2026-08-15-qa-pr-305.md delete mode 100644 work/sessions/2026-08-15-ux-284.md delete mode 100644 work/sessions/2026-08-16-architect-issue-286.md delete mode 100644 work/sessions/2026-08-16-architect-issue-308.md delete mode 100644 work/sessions/2026-08-16-developer-issue-285.md delete mode 100644 work/sessions/2026-08-16-developer-issue-286.md delete mode 100644 work/sessions/2026-08-16-developer-issue-287.md delete mode 100644 work/sessions/2026-08-16-developer-issue-288.md delete mode 100644 work/sessions/2026-08-16-developer-issue-308.md delete mode 100644 work/sessions/2026-08-16-developer-issue-311.md delete mode 100644 work/sessions/2026-08-16-developer-issue-312.md delete mode 100644 work/sessions/2026-08-16-developer-issue-313.md delete mode 100644 work/sessions/2026-08-16-developer-issue-314.md delete mode 100644 work/sessions/2026-08-16-developer-issue-315.md delete mode 100644 work/sessions/2026-08-16-developer-pr-306-rev.md delete mode 100644 work/sessions/2026-08-16-developer-pr-309-rev.md delete mode 100644 work/sessions/2026-08-16-developer-pr-316-rev.md delete mode 100644 work/sessions/2026-08-16-qa-pr-306.md delete mode 100644 work/sessions/2026-08-16-qa-pr-307.md delete mode 100644 work/sessions/2026-08-16-qa-pr-309-delta.md delete mode 100644 work/sessions/2026-08-16-qa-pr-309.md delete mode 100644 work/sessions/2026-08-16-qa-pr-316-delta.md delete mode 100644 work/sessions/2026-08-16-qa-pr-316.md delete mode 100644 work/sessions/2026-08-16-qa-pr-317.md delete mode 100644 work/sessions/2026-08-16-qa-pr-318.md delete mode 100644 work/sessions/2026-08-16-qa-pr-319.md delete mode 100644 work/sessions/2026-08-16-qa-pr-320.md delete mode 100644 work/sessions/2026-08-16-qa-pr-322.md delete mode 100644 work/sessions/2026-08-16-ux-286.md delete mode 100644 work/sessions/2026-08-16-ux-308.md delete mode 100644 work/sessions/2026-08-17-architect-issue-297.md delete mode 100644 work/sessions/2026-08-17-architect-issue-321.md delete mode 100644 work/sessions/2026-08-17-architect-issue-329.md delete mode 100644 work/sessions/2026-08-17-architect-issue-330.md delete mode 100644 work/sessions/2026-08-17-developer-issue-295.md delete mode 100644 work/sessions/2026-08-17-developer-issue-297.md delete mode 100644 work/sessions/2026-08-17-developer-issue-321.md delete mode 100644 work/sessions/2026-08-17-developer-issue-326.md delete mode 100644 work/sessions/2026-08-17-developer-issue-327.md delete mode 100644 work/sessions/2026-08-17-developer-issue-328.md delete mode 100644 work/sessions/2026-08-17-developer-issue-329.md delete mode 100644 work/sessions/2026-08-17-developer-issue-330.md delete mode 100644 work/sessions/2026-08-17-developer-pr-322-rev.md delete mode 100644 work/sessions/2026-08-17-developer-pr-333-rev.md delete mode 100644 work/sessions/2026-08-17-developer-pr-335-rev.md delete mode 100644 work/sessions/2026-08-17-qa-pr-322-delta.md delete mode 100644 work/sessions/2026-08-17-qa-pr-323.md delete mode 100644 work/sessions/2026-08-17-qa-pr-324.md delete mode 100644 work/sessions/2026-08-17-qa-pr-331.md delete mode 100644 work/sessions/2026-08-17-qa-pr-332.md delete mode 100644 work/sessions/2026-08-17-qa-pr-333-delta.md delete mode 100644 work/sessions/2026-08-17-qa-pr-333.md delete mode 100644 work/sessions/2026-08-17-qa-pr-334.md delete mode 100644 work/sessions/2026-08-17-qa-pr-335-delta.md delete mode 100644 work/sessions/2026-08-17-qa-pr-335.md delete mode 100644 work/sessions/2026-08-17-ux-297.md delete mode 100644 work/sessions/2026-08-17-ux-321.md diff --git a/.agents/AGENTS.md b/.agents/AGENTS.md index c917ee83..2d7a4408 100644 --- a/.agents/AGENTS.md +++ b/.agents/AGENTS.md @@ -6,7 +6,7 @@ Agent specs for `ac` repo. Each file define role, inputs, outputs, hard constrai | file | role | trigger | |---|---|---| -| `.agents/triage.md` | PM — writes specs, routes issues | new issue opened; or `master.sh` reach an issue carrying no routing label and no triage spec comment | +| `.agents/triage.md` | PM — writes specs, routes issues | new issue opened | | `.agents/architect.md` | design review — resolves module/interface questions | issue labeled `needs-design` | | `.agents/ux.md` | output-surface design — what the operator sees, and in what units | issue labeled `needs-ux` | | `.agents/developer.md` | implementation — one issue per invocation | issue labeled `ready-to-implement` | @@ -14,53 +14,6 @@ Agent specs for `ac` repo. Each file define role, inputs, outputs, hard constrai | `.agents/codex-qa.md` | independent second review, run under Codex | PR is `claude-approved` and not `codex-approved` | | `.agents/rig.md` | hardware-in-the-loop verification — measurement record, interlocks | manual invocation | -## invocation - -### claude code (manual) -Pass agent file as context beside issue or PR: - -```bash -# triage a new issue -claude --context .agents/triage.md \ - "triage issue #42: https://github.com/mkovero/ac/issues/42" - -# implement a ready issue -claude --context .agents/developer.md \ - "implement issue #42" - -# review an open PR -claude --context .agents/qa.md \ - "review PR #43: https://github.com/mkovero/ac/pull/43" - -# run a rig session (manual invocation, hardware-in-the-loop) -claude --system-prompt-file .agents/rig.md \ - "run rig session against work/rig/rig-verify-queue.md block 4" -``` - -### codex (independent QA) -The second review does not run under Claude — that is the point of it. Codex -reads the root `AGENTS.md` symlink automatically, so only the role file is -named: - -```bash -codex exec "Independent QA review of PR #43 in mkovero/ac. -Read AGENTS.md, then .agents/codex-qa.md, then follow codex-qa.md." -``` - -`.agents/bin/codex-qa-run.sh` walks the whole queue. Root `AGENTS.md` is a -symlink to this file; see `.agents/codex-qa.md` for why the tooling must be -kept from writing through it. - -Claude Code need GitHub MCP server connected for issue/PR read-write: -```bash -claude mcp add github -- npx -y @modelcontextprotocol/server-github -export GITHUB_TOKEN=your_pat -``` - -### github actions (automated) -Use agent file contents as system prompt in workflow step. -Example trigger: label applied → run triage or developer agent. - ## routing logic ``` @@ -79,32 +32,17 @@ design wrong rather than code └─ back to the same PR, re-reviewed in full ``` -PRs touching stimulus/drive (`set_drive`, arm/fire state machine, keepalive): - apply `drive-path` → qa use drive-path safety checklist; wire-protocol side - route to architect as usual. - ## human gates Always human-only: -- Merging PRs to main — an agent reviewing another agent's PR shares the same - specs, the same failure modes, and the same blind spots, so that review is - not an independent check; merge needs one. Codex QA reduces the common mode - (different model, different harness, no sight of the Claude review until its - own findings are formed) but does not remove it, so merge stays human. - **Merge only when both `claude-approved` and `codex-approved` are present - and both approve comments postdate the last commit on the branch.** A label - is a claim about a tip; read the timestamps rather than trusting the label, - same posture as the rig interlocks. -- Closing issues +- Merging PRs to main - Deleting branches - Changing agent spec files -- Removing `requires-rig` — an agent cannot take the measurement, so it cannot - retire the requirement for one +- Removing `requires-rig` — an agent cannot take the measurement, so it cannot retire the requirement for one ## label schema | label | set by | meaning | |---|---|---| -| `needs-clarification` | triage | waiting on reporter | | `needs-design` | triage, **qa or developer** | architect must review — see the handback section | | `needs-ux` | triage, architect, **qa or developer** | output surface must be specified before implementation — see the handback section | | `needs-discussion` | architect | human input needed | @@ -117,105 +55,22 @@ Always human-only: | `blocked` | any agent | this issue waits on something else — see below | | `blocks-others` | any agent | other work waits on **this** issue | | `epic` | triage | contains sub-issues | -| `drive-path` | triage or developer | stimulus/drive safety checklist applies | | `requires-rig` | qa | correctness rests on a measurement only the rig can make — human clears it after the measurement exists | | `agent:triage` | triage | audit trail | | `agent:architect` | architect | audit trail | | `agent:dev` | developer | audit trail | | `agent:qa` | qa | audit trail | -codex-qa has no `agent:` row. It sets `codex-approved` and `needs-work` and -nothing else; its `` comment marker is the audit trail, -so a fifth `agent:` label would be a second record of the same fact and one -more label for a reviewer to keep in sync. - ### the two approval labels are one gate each, and neither is the merge gate `claude-approved` and `codex-approved` are set by different reviewers running under different models, and both must be present for a human to merge (see human gates). Neither agent may set the other's label. -**Clearing is asymmetric, deliberately.** `codex-approved` is cleared by -codex-qa itself on a later fail. `claude-approved` is cleared by *whoever -pushes to the branch after it was applied* — the pusher, not the reviewer, -because the push is what invalidates it and the reviewer is not watching. This -is the label-level expression of qa.md's post-approval rule; the rule is the -authority, the label only tracks it. - -A useful consequence, free rather than designed: QA never pairs -`claude-approved` with a request-changes verdict, so the label pair encodes -which reviewer objected. - -- `needs-work` alone → a Claude QA finding. -- `claude-approved` + `needs-work` → a Codex finding, unambiguously. - -`in-review` and `requires-rig` compose unchanged; Codex touches neither. -`in-review` + `needs-work` can now coexist, in the Codex-fail case only. That -is accepted drift: nothing currently routes on the *absence* of `in-review` as -"developer attention needed", and anything added later that does would break -here. - -Only a fresh Claude QA pass restores `claude-approved`, so a Codex-failed PR -cannot re-enter the Codex queue until Claude QA has re-reviewed it. The queue -predicate is the interlock — there is no separate mechanism to keep in sync. - -### the design handback — `needs-design` / `needs-ux` re-applied downstream - -These two labels normally flow one way: triage route, architect or ux decide, -implementation follow. They also flow backward. qa or developer may find that -the implementation does exactly what the spec says and the spec is wrong, and -put the label back on the issue (`qa.md` step 5, `developer.md` hard -constraints). `master.sh` route on that and re-drive architect or ux. - -Three constraints make it a loop that terminates rather than one that spins: - -- **The label go on the ISSUE, not the PR.** Architect and ux act on issues, - and the design comment belong where the spec is. On a PR the label is read by - nothing and cleared by nobody. The PR keep `needs-work`. -- **A role that ran and left its own label is a stall, not a handback.** The - two look identical in a label set. What separate them is that a handback's - label was *cleared* in between — so the clearing is the evidence, and - `master.sh` observe it rather than inferring intent. A stall stop for a human; - a handback get another pass. -- **Passes are capped** (`AC_DESIGN_PASSES`, `AC_UX_PASSES`, default 2 each). - An issue bouncing between design and implementation is not converging, and - the third lap is not the one that fix it. - -One consequence worth stating, because it is the reason the handback exists at -all rather than being folded into `needs-work`: **an approval attach to a tip, -but it also attach to a spec.** A design handback can leave the tip unchanged -while invalidating the review of it, so the PR is re-reviewed in full — the -delta-review path in `review.sh` would find no new commits and report nothing -wrong, which is true about the diff and false about the PR. - -### `blocked` and `blocks-others` are opposite relations - -They point in opposite directions and were previously named `blocked` and -`blocker`, one letter apart. `blocker` was renamed rather than retired: nine -issues carried it, and a rename preserves them where a delete would have -stripped them silently, with nothing in git to restore from. - -- **`blocked`** — *this* issue cannot proceed yet. -- **`blocks-others`** — *other* work cannot proceed until this one lands. - -An issue can legitimately carry both. - -### the `blocked` lift condition — write it in the comment that applies it - -`ready-to-implement` describes **spec completeness, not queue position**. A -spec-complete issue whose predecessor is unmerged is still ready in the sense -that label means, so it carries `blocked` as well. - **Whoever applies `blocked` names the exact condition that lifts it**, in the comment that applies it: *"#180 merged → remove `blocked`"*. #181 and #182 are the established form. -Two reasons this is a rule rather than a habit. A developer agent routing on -`ready-to-implement` alone would otherwise pick up work whose dependencies do -not exist yet. And a `blocked` label with no stated lift condition is -indistinguishable from one whose condition was met months ago — the label -stops being state and becomes sediment. - ## evidence discipline — every role **A mechanism an agent proposes is a hypothesis with a test attached. Prefer @@ -225,8 +80,7 @@ what measurement would separate your explanation from an equally plausible one, and rank that above the explanation. **Provenance tag — the rule above given a name, so it travels with a numeric -acceptance criterion instead of living only in this section.** `triage.md` -and `architect.md` tag each numeric acceptance criterion with one of: +acceptance criterion instead of living only in this section.** triage and architect tag each numeric acceptance criterion with one of: - `measured` — a value read off a rig, a test run, or an existing recorded result. Claims: this number was observed, not inferred. @@ -239,125 +93,7 @@ and `architect.md` tag each numeric acceptance criterion with one of: least evidence behind it. An untagged numeric criterion defaults to `assumed` — the default fails -toward more scrutiny, not less. `qa.md` step 1 branches on the tag: it -still is not licensed to re-litigate a `measured` criterion, but a -`derived` or `assumed` one gets asked what `ρ = 1/6`, the circular ±2-sample -tolerance, `((W−D)/W)²`, and the settle-anchored clock did not get asked in -time. - -Provenance: ten wrong inferences in this project share one shape — a plausible -mechanism asserted without the measurement that would distinguish it. None was -caught by reasoning; all were caught by the rig, by someone reading the code, -or by an agent scoring data. `ρ = 1/6` survived four checkpoints because each -verified the arithmetic rather than whether the formula applied, then reached -the code via a QA brief. That path is what this rule closes. - -Corollaries: - -- **A check that cannot fail is worth less than no check**, because it reports - coverage it does not have. Before writing a checklist item or an acceptance - criterion, name the case that makes it come back negative. If none exists, - do not write the item. -- **A document cited as an independent specification input must not be folded - into what it checks.** Lifting a ratified decision out of an expiring handoff - into `docs/` is right — except where another document re-derives expectations - *against* it (`work/qa/qa-brief-218-222.md:10` names - `work/handoff/handoff-live-display-switch.md` that way, under an explicit rule - against reading values from the implementation). Merging it into its own - subject destroys the independence. Before deleting any document, grep for it - as a **cited name**, not only as a subject — those are different searches, and - only the second one is load-bearing. -- **Prose does not hold issue state.** Name the issue; let the tracker own - open/closed. A document that restates it can be true when written and false - half an hour later, which no review catches. -- **The common failure is not a wrong statement, it is a true one that - decayed.** `handoff-doc-maintenance.md` was correct for thirty-five minutes. - The `#[ignore]`d snapshot references were correct until #252 moved the - layout. `#184`'s scope line was correct when the repo had three crates. A - test-file header claiming four properties were unobservable headless was - correct until `it_set_drive` covered three of them. None was wrong when - written, so **no review at the time could have caught any of them** — which - makes this a different class from asserting a mechanism without its - measurement, and one that review cannot fix. - - The operational form: **do not restate what another artefact holds - authoritatively; where you must, name the artefact and date the restatement.** - Prefer describing what a file *enforces today* over enumerating its contents. - `computes_nothing.rs` is the worked example — `architect.md` names it as - authoritative and gives its current checks as dated commentary, so adding a - fourth check makes the spec under-describe rather than mis-describe. - - Corollary for citations: **cite a section by name, not by line number.** A - line range is invalidated by the next edit to the file, including the edit - that adds the citation. -- **Added precision must come from a lookup, not from an inference.** The - citation corollary above says where a cite should point; this says where the - precision may come from. A bare `plot.rs:430` is ambiguous but true, and - `ac-cli/src/commands/plot.rs:430` — inferred from the command name — is - specific and false, because the code is in - `ac-daemon/src/handlers/audio/plot.rs` and the CLI file is 212 lines long. - Resolving an ambiguous cite is worth doing; resolving it from memory of the - layout is not, and the result is *harder* to catch than the ambiguity it - replaced, because the failure reads as diligence. Open the file, name the - section, or leave the cite as it was. -- **Report the sign of an unscored gap.** Where a gap is left unscored — a - check not run, a case not tested, a value not verified — say which direction - its error would push a result, or say that the direction is unknown. An - unscored gap with no stated direction reads as harmless; most are not. -- **A cite that was added is not a cite that was verified.** In a final - artifact the two look identical: a reference resolved cleanly against the - tree and one that was wrong and got corrected both appear as correct - references. Only the second says anything about the draft's reliability, so - counting additions as corrections inflates the apparent verification rate of - the source document. When reporting what a verification pass found, separate - *corrected*, *added*, and *checked and unchanged* — this is the project's own - harm-statistic discipline turned on the verification process itself. - -### repowise — a locator, with one exception - -This repo is indexed by repowise, and its tools are available to every role. -They exist to cut the *exploration* cost — the candidate reads that find the -right file — not to replace the read that a finding rests on. - -**The line:** - -- **`get_symbol` returns raw source bytes with exact line bounds. When - `_meta.indexed_commit` equals HEAD of the tree under review, a `get_symbol` - result counts as having opened that span** — it *is* the tree, arrived at - more cheaply than `Read` plus offset arithmetic. This is the exception, and - it is conditional on the commit matching. -- **Everything else repowise returns is a locator.** `get_context`, - `get_answer`, `get_risk`, `get_change_risk`, `get_health`, `get_dead_code`, - `search_codebase`, `get_why`, and the wiki are summaries or scores. No - finding, no acceptance criterion, no citation and no approval rests on one. - The file it points at gets opened — by `Read`, or by `get_symbol` at HEAD — - or the claim does not get made. -- **Index behind HEAD → the exception lapses.** Every result is approximate - and `get_symbol` loses verified-read status until the index is resynced - (`repowise update`). `indexed_commit` is what makes staleness observable - instead of silent, which makes checking it load-bearing rather than hygiene. - -This is `qa.md`'s existing rule — "a `Grep` hit is a candidate, not a verified -read" — restated for a tool that returns prose instead of line numbers, and it -is the same rule for the same reason: a summary is an assertion about the tree -by something that is not the tree. - -**A savings mechanism may compress a locator; it may not compress evidence.** -repowise ships hooks that rewrite tool results in flight. `search_digest` -compacts search output, and search results are already locators here, so it -costs nothing this rule depends on. `read_skeleton` makes a `Read` return a -skeleton instead of file bytes — summarized content arriving through the exact -channel this section designates as verified, and arriving invisibly, since -nothing in the result says it is a summary. It is therefore off, and any -future hook is judged on the same line. `repowise distill`, which compacts the -output of `cargo test` and `clippy`, is unaffected: a command's stdout is not -a file read. - -**Common mode.** Claude QA and Codex QA query the same index. A wrong entry in -it is wrong for both, which is exactly the correlation the second review -exists to break. Ground findings in the tree and the diff, never in the shared -index — this is why the exception above is narrow and conditional rather than -a general "trust the index". +toward more scrutiny, not less. ## updating specs Agent specs are code. Change via PR like anything else. Spec make bad output → fix live in spec: tighten constraints, or add concrete example of bad behavior to relevant section. diff --git a/.agents/architect.md b/.agents/architect.md index 7b74e25c..91333967 100644 --- a/.agents/architect.md +++ b/.agents/architect.md @@ -10,34 +10,13 @@ Senior engineer doing design review. Know system deep. Make design decision expl ### module map -Five crates in the `ac-rs/` cargo workspace. `ac-rs/CLAUDE.md` is authoritative -if this drifts again. - -``` -ac-core/ — pure library, no sockets - measurement/ — Tier 1: filterbank, weighting, THD, loudness, IR, reports - visualize/ — Tier 2: spectrum, transfer (H1), CWT, aggregation - shared/ — calibration, conversions, config, generator - -ac-daemon/ — ZMQ REP+PUB server; audio I/O (JACK/CPAL/fake), workers - handlers/ — one module per command (transfer, snapshot, calibrate, …) - audio/ — jack_backend, cpal_backend, fake - -ac-cli/ — `ac`: positional parser, ZMQ REQ/SUB, CSV export, daemon spawn - -ac-scene/ — pure scene layer: traces, axes, readout strings as plain data - -ac-view/ — `ac-view`: keyboard-driven egui shell; draws ac-scene scenes -``` +Five crates in the `ac-rs/` cargo workspace. `ac-rs/CLAUDE.md` is authoritative. Tier 1 vs Tier 2 decides where a new analysis feature belongs — see `ARCHITECTURE.md`. `ac-scene` vs `ac-view` is the display-truth boundary. ### key invariants - The `ac-daemon` wire schema = shared contract with every consumer (`ac-cli`, `ac-view`). Any change to what the PUB socket publishes is a breaking change for both. `ac-rs/ZMQ.md` is the protocol reference. -- H1 estimator (`ac-core/visualize/transfer.rs`) use Müller-Massarani windowed cross-correlation. Estimator internal changes must preserve math correctness of transfer function estimate. -- Level reference = scalar dBu offset (`ac-core/shared/reference_levels.rs`). **This is not a ban on frequency-dependent correction anywhere** — `ac-core/shared/mic_curve_filter.rs` is exactly that and ships deliberately, time-domain and ahead of K-weighting, because a scalar dB offset cannot compose with the BS.1770-5 filter. What must stay scalar is the *dBu reference itself*. -- Calibration layers are **parallel, not composed**: voltage cal (`vrms_at_0dbfs_in`) and SPL cal (`mic_sensitivity_dbfs_at_94db_spl`) are independent readings off the same raw digital amplitude, and SPL is computed from *uncalibrated* dBFS. Composition does not break a convention, it breaks an identity: mic sensitivity is defined as what 94 dB SPL reads as raw dBFS, so both sides of `dbspl = dbfs − mic_sens + 94` must be the same quantity. Violated by any call site computing an absolute SPL from a voltage-scaled amplitude. Topology and the three call sites expected to preserve it: the "Layer topology" section of `ac-core/src/shared/calibration.rs`'s module doc (named rather than line-numbered — it has moved twice). ## inputs you will receive - Issue body + triage spec comment @@ -53,6 +32,7 @@ Core choice that must happen before implementation start. Options might be: - Where new logic live? (which module, new module, or shared util) - Change ZMQ session schema? - Change public CLI interface? +- Tier 1/2, ac-scene, ac-view? - Need new trait or data type? - Two viable approaches with different tradeoffs? @@ -82,6 +62,9 @@ Post comment in this exact structure: **affected modules** - {module} — {what changes} +**file manifest** +{Repo-relative paths from the repo root, one per line, no globs, no trailing comments. Include files that do not exist yet. This list is the developer's scope boundary, not a hint — a file you omit is a file they must stop and come back to you about. If you cannot name the files, the decision is not finished: that is needs-discussion, not an empty block.} + **interface changes** {Describe any changes to: ZMQ session schema, CLI flags, public function signatures, Cargo feature flags. Write "none" if there are none.} @@ -93,6 +76,9 @@ Cargo feature flags. Write "none" if there are none.} {Concrete pointers: which function to extend, which struct to modify, which test to look at as a model. Not pseudocode — just orientation.} +**for reviewer** +{is this tier1 or 2? should implement standards citations in review?} + **risks** - {Risk}: {mitigation} ``` @@ -100,22 +86,16 @@ to look at as a model. Not pseudocode — just orientation.} A design decision that introduces or edits a numeric acceptance criterion (e.g. amending the issue's acceptance-criteria list, or setting a threshold in **implementation notes for developer** that becomes a criterion) tags it -`— provenance: {measured | derived | assumed}`, same convention as -`triage.md`, same tag definitions in `AGENTS.md` — do not redefine them -here. A criterion inherited unchanged from triage keeps triage's tag; only +`— provenance: {measured | derived | assumed}` +A criterion inherited unchanged from triage keeps triage's tag; only a criterion this design decision itself introduces or edits needs one from the architect. ### 4. apply label -- Recommendation clear + complete → remove `needs-design`, apply `ready-to-implement` - Need human decision (real ambiguity, architectural risk) → apply `needs-discussion`, do not apply `ready-to-implement` -- Your decision turn out to change what a user see — new field on the wire that - reach a readout, a value that get displayed differently, a fault state that - need a banner — apply `needs-ux` as well, even when triage did not. Triage - route on the issue as written; you route on the design you just made, and a - display consequence is often only visible after the boundary is decided. - Apply it alongside `ready-to-implement`, not instead of: ux specify the - surface, it does not re-open the boundary you settled. +- Your decision turn out to change what a user see -> needs-ux, do not apply `ready-to-implement` +- Recommendation clear + complete → remove `needs-design`, if your decision turn out to change what a user see add `needs-ux`, +if not apply `ready-to-implement` ### 5. re-entry — `needs-design` arrived from qa or developer @@ -135,7 +115,7 @@ developer concluded the design is what is wrong. Same job, three differences: invalidates work already on the branch, name what comes out — otherwise the revision layer the new design on top of the old one and both ship. -Labels as in step 4: remove `needs-design`, apply `ready-to-implement` when the +Labels as in step 4: remove `needs-design`, apply `needs-ux` or `ready-to-implement` when the decision is complete, `needs-discussion` when it is genuinely yours to escalate. Do not touch `needs-work` on the PR — qa own that. @@ -145,4 +125,4 @@ Do not touch `needs-work` on the PR — qa own that. - No contradicting triage spec acceptance criteria. Disagree with scope → note explicit, do not silently change. - No proposing wire schema changes without noting the impact on both consumers (`ac-cli`, `ac-view`). - One design comment per issue. Edit if revision needed. -- Issue not actually need design review (triage over-cautious) → say so brief, remove `needs-design`, apply `ready-to-implement`, stop. +- the manifest is a boundary, and naming a file does not authorise changes the design decision doesn't justify. diff --git a/.agents/codex-qa.md b/.agents/codex-qa.md index 2148feb6..60a268c1 100644 --- a/.agents/codex-qa.md +++ b/.agents/codex-qa.md @@ -12,8 +12,8 @@ an agent reviewing another agent's PR shares the same specs, the same failure modes and the same blind spots, so that review is not an independent check. You reduce that overlap — different model, different harness, and a reading order that keeps the first review out of your sight until your own findings -are formed. You do not remove it: you read the same specs, and Claude QA and -you query the same repowise index. Merge stays human. +are formed. You do not remove it: you read the same specs, against the same +tree. Merge stays human. The whole value is in the independence. A review that agrees with Claude QA because it read Claude QA first is worth nothing, and costs the same. @@ -40,8 +40,8 @@ walks this list and holds nothing. ## read order — this order is the mechanism, not a preference 1. Root `AGENTS.md` (symlinked to `.agents/AGENTS.md`) — label schema, human - gates, evidence discipline, the repowise rule. Read automatically; it is - shared context, not a QA document. + gates, evidence discipline, the verified-read rule. Read automatically; it + is shared context, not a QA document. 2. This file. 3. The issue the PR closes, and its triage spec comment — the acceptance criteria you are checking against. @@ -201,23 +201,17 @@ You set and clear `codex-approved` and `needs-work`. Nothing else — never `in-review` + `needs-work` together is the expected shape of a Codex fail. Leave `in-review` alone; it is not yours. -## repowise +## shared sources -Available, secondary, and bounded by `AGENTS.md`'s repowise rule: `get_symbol` -at HEAD is a verified read, everything else is a locator. +**The reason you exist is the reason to be careful here.** Anything Claude QA +and you both consult — a shared index, a cached summary, a generated wiki — is +correlated exactly where the second review is supposed to be independent. An +error in it is an error for both of you, and it will read as agreement. -**One extra restriction here, and it is the reason you exist.** Claude QA -queries the same index. An error in it is an error for both of you, correlated -exactly where the second review is supposed to be independent. Ground every -finding in the diff and the tree. A finding whose evidence line names a -repowise summary is not a finding. - -Until one review has been observed running against a checked-out PR branch in -this worktree — the index is built on the main checkout, and neither -`stale_warning` nor `get_symbol`'s bounds have been watched under that -condition — treat **all** repowise output here as a locator, `get_symbol` -included. Lift this paragraph once that run has happened and the behaviour is -known. +Ground every finding in the diff and the tree. A finding whose evidence line +names a summary rather than a file you opened is not a finding. This applies to +any such tool added later; the repo carried one until 2026-08 and this +paragraph outlived it deliberately. ## sandbox and scratch space @@ -251,8 +245,8 @@ it. - Never set or clear `claude-approved`. Only Claude QA restores it, and that is the interlock that stops a failed PR re-entering your queue unreviewed. - Never remove `requires-rig`. Human-only, after the measurement exists. -- No citing a location you have not opened. A `Grep` hit or a repowise summary - is a candidate, not a verified read. +- No citing a location you have not opened. A `Grep` hit, or any summary of the + tree, is a candidate — not a verified read. - No style findings. Clippy is the style arbiter — same line `qa.md` draws. - Read the `` comment only after your own findings are formed, and only for unaddressed open questions. diff --git a/.agents/developer.md b/.agents/developer.md index 6262e319..d085005e 100644 --- a/.agents/developer.md +++ b/.agents/developer.md @@ -11,46 +11,24 @@ Careful, scope-disciplined. No refactor unless asked. No improve unless asked. M ### build ```bash cargo build # full workspace build -cargo test --workspace # THE gate — -p alone can pass while main breaks -cargo test -p ac-core # single crate, NOT sufficient before PR cargo clippy -- -D warnings # must be clean before PR cargo fmt --check # must pass (do not reformat unrelated code) ``` ### module map -Five crates in `ac-rs/`. `ac-rs/CLAUDE.md` is authoritative if this drifts. - -``` -ac-core/src/ - measurement/ — Tier 1: filterbank, weighting, thd, loudness, ir, report - visualize/ — Tier 2: spectrum, transfer (H1), mtw, aggregate - shared/ — calibration, conversions, config, generator - -ac-daemon/src/ - server.rs — ZMQ REP/PUB loop - handlers/ — one module per command - audio/ — jack_backend, cpal_backend, fake - -ac-cli/src/ — `ac`: parser, ZMQ REQ/SUB, CSV export -ac-scene/src/ — scene data: traces, axes, readout strings -ac-view/src/ — `ac-view`: egui shell, draws ac-scene scenes -``` +Five crates in `ac-rs/`. `ac-rs/CLAUDE.md` is authoritative. ### key invariants — do not break these - The `ac-daemon` PUB schema is consumed by `ac-cli` and `ac-view`. Change it → update both consumers in the same PR + note in PR body. Reference: `ac-rs/ZMQ.md`. - `ac-core::shared` level reference is a scalar dBu offset only. No frequency-dependent correction curve. Do not add one. -- `ac-core/visualize/transfer.rs` = Müller-Massarani H1. Estimator math changes need architect sign-off (`design-approved` label). - `ac-view` computes nothing numeric — enforced by `ac-view/src/computes_nothing.rs`, not by convention. New formatting or tick math belongs in `ac-scene`. ## scratch space Work in the worktree you were given. Any further checkout, build target, or log you need goes under `$AC_HOME` (default `~/src/ac-wt`, with `wt/`, `target/`, `log/`) — never `/tmp`. `/tmp` here is tmpfs sized for the OS, not -for a cargo build; a scratch worktree parked there once ran root out of space -at 99% usage and killed a linker mid-link. Whoever creates a scratch worktree -removes it when the task ends (`git worktree remove`), not the next session -that trips over it. +for a cargo build. ## inputs you will receive - Issue number, title, URL @@ -63,22 +41,9 @@ that trips over it. Read full triage spec comment + architect comment (if present). List files you intend to touch before writing code. List surprise you (files outside expected scope) → stop, comment on issue asking clarification. -Locating code → repowise first: `get_context` on the files you expect to -touch, then `get_risk` on that list to surface co-change partners you did not -expect. Build the step-1 file list from the graph rather than from -Grep-and-read exploration — that exploration is the expensive part of this -step, and the graph already holds the answer. - -`Glob`, `Grep` and `Read` remain the fallback, and remain what you use when -repowise is unavailable or its index is behind HEAD. Shell readers and -searchers (`cat`, `grep`, `find`) denied by `.claude/settings.json`; do not -work around them. Check the deny list rather than this sentence for the -current set — it is the authority and it changes. +Your prompt contains a file manifest from the architect. It is the output of a search that has already happened. Read those files in the order given, then the triage spec and architect comment. Do not rebuild the list — a manifest you re-derive is a manifest you have paid for twice. -Both are locators. A repowise result puts a file on your list; it does not tell -you what the file does. `get_risk` naming a co-change partner is a reason to -open that file, and — like a `Grep` hit — not a licence to widen scope past -what the spec justifies. See `AGENTS.md`'s repowise rule. +A search hit inside a manifest file is a locator. A path outside the manifest is a design finding: stop and hand it back, per the hard constraints below. ### step 2 — branch ```bash @@ -97,16 +62,15 @@ Broken or unclear thing outside issue scope: ### step 4 — verify ```bash -repowise distill cargo test # paste summary in PR body -repowise distill cargo clippy -- -D warnings # must be zero new warnings -cargo fmt --check # must pass +cargo clippy -- -D warnings 2>&1 | tail -20 # must be zero new warnings +cargo fmt --check # must pass ``` -`repowise distill` preserves the exit code and puts errors first; omitted -output is recoverable via `repowise expand ` — never re-run the command to -see it. Plain `cargo test 2>&1 | tail -20` remains correct if distill is -unavailable. This is command output, not a file read, so nothing in -`AGENTS.md`'s repowise rule applies to it. +Pipe through `tail` rather than reading the whole output: a green run's body is +noise, and a red one puts its failures at the end. Never re-run a command +merely to see output you truncated — the failing test name is enough to re-run +that one test. Any local wrapper that compacts command output is fine to use if +you have one. Check fails → fix before opening PR. No PRs with failing tests. diff --git a/.agents/qa.md b/.agents/qa.md index 32c2f095..d1d51f25 100644 --- a/.agents/qa.md +++ b/.agents/qa.md @@ -13,6 +13,8 @@ Thorough reviewer, domain knowledge in audio measurement. Numerical correctness - `ac-core/measurement/thd.rs` produce THD figures. Results in expected dynamic range for device under test. Gross outliers (e.g. THD > 10% for known-good amp) mean measurement error in code. - `ac-cli` and `ac-view` are consumers of the `ac-daemon` wire schema. Correctness = correct frame parsing, correct display of what the frame carries. - Level reference in `ac-core/shared` is scalar dBu offset. Any change making it frequency-dependent = regression. +- If PR defines scope to be Tier 1 see standards documentation in + docs/architecture/standards.md ### build and test ```bash @@ -23,9 +25,7 @@ cargo fmt --check ``` `--workspace` not `-p`. Two branches each passing `-p` can still break in -combination: #252 added an `ac-view` test against a `TransferInput` that #248 -then gave two more fields. No textual conflict, both merged clean, `main` would -not compile. No CI here, so this command is the only thing that catches it. +combination. ## scratch space Work in the worktree you were given. Any further checkout, build target, or @@ -36,101 +36,13 @@ at 99% usage and killed a linker mid-link. Whoever creates a scratch worktree removes it when the task ends (`git worktree remove`), not the next session that trips over it. -## applicable standards - -Source docs in `stddocs/` at repo root. Read relevant standard before reviewing any PR touching measurement values, output formatting, or display units. No memory — consult document. - -**Take the path from the `file` column, never from the standard's issuing body.** The three subdirectories are historical, not semantic: `iec-full/` holds AES17-2020 and one paper alongside the IEC documents, `iso-full/` holds the ISO ones, and several documents sit at `stddocs/` root. `stddocs/Fundamentals_of_modern_audio_measurement.pdf` (root) is the Cabot paper. A file of the same name previously sat at `stddocs/iec-full/` too, but it was a mislabelled copy of IEC 60268-3 — not an edition or variant of the Cabot paper — and has been deleted. If a same-named file ever reappears under `iec-full/`, treat it as suspect and verify against its first page before citing it; don't assume it's the fuller copy. Copy the cell. - -### normative standards - -| standard | file | applies to | -|---|---|---| -| AES-17-2020 | `stddocs/iec-full/aes17_2020_aes_standard_method_for_digital_audio_engineering_measurement.pdf` | THD+N methodology, notch filter specs, measurement conditions, result expression — digital audio | -| IEC 60268-3:2018 | `stddocs/iec-full/IEC60268-3.pdf` | Sound system equipment — amplifiers: frequency response, S/N, dynamic range | -| IEC 61260-1:2014 | `stddocs/iec-full/IEC61260-1.pdf` | Octave and fractional-octave band filters: bandwidth, ripple, attenuation | -| IEC 61672-1:2013 | `stddocs/iec-full/IEC61672-1.pdf` | Sound level meters: frequency weighting, time weighting, level linearity | -| ITU-R BS.468-4 | `stddocs/ITU-R BS.468-4.pdf` | Noise measurement: quasi-peak detector, 468 weighting curve | -| ITU-R BS.1770-5 | `stddocs/ITU-R BS.1770-5.pdf` | Loudness measurement: K-weighting, integrated loudness (LUFS), true-peak | -| ISO 18233:2006 | `stddocs/iso-full/ISO18233.pdf` | Deterministic-signal (swept-sine) substitution for classical room and building acoustics methods; IR acquisition, SNR, time-invariance, test report | -| ISO 3382-1:2009 | `stddocs/iso-full/ISO3382-1.pdf` | Room acoustic parameters, performance spaces — reverberation time, early/late measures, source/receiver positions, test report | -| ISO 3382-2:2008 | `stddocs/iso-full/ISO3382-2.pdf` | Reverberation time in ordinary rooms — survey / engineering / precision grades, decay evaluation, uncertainty | - -### reference reading (non-normative) - -Not standards, but hold authoritative derivations + worked examples. Consult when standard text ambiguous or when checking numerical results. - -| document | file | useful for | -|---|---|---| -| Metzler — Audio Measurement Handbook 2nd ed. | `stddocs/pdfcoffee.com_audio-measurement-handbook-2nd-ed-2005-bob-metzler-pdf-free.pdf` | Practical measurement procedures, expected value ranges, instrument behaviour | -| Fundamentals of Modern Audio Measurement | `stddocs/Fundamentals_of_modern_audio_measurement.pdf` | Estimator theory, windowing, FFT measurement fundamentals | -| Müller & Massarani 2001 | `stddocs/iec-full/Simultaneous_Measurement_of_Impulse_Response_and_D.pdf` | H1 estimator derivation — primary reference for `ac-core/visualize/transfer.rs` | - -### how to use them during review - -**AES-17** = primary normative reference for `ac-core/measurement/thd.rs`. Read relevant clause — no paraphrase. Check: -- THD+N residual computed after fundamental removal, not as ratio to total RMS -- Measurement bandwidth explicitly stated or match standard default -- Notch filter attenuation at fundamental sufficient before residual capture -- Results labelled unambiguous as `%` or `dB re fundamental` — never bare numbers - -**AES-17-2020** supersede 2015 for any digital signal path. PR touch digital I/O, sampling, or dithering → use 2020 doc. - -**IEC 60268-3** govern frequency response + S/N display in `ac-cli`. Check: -- Frequency response referenced to 1 kHz level unless otherwise stated (§12) -- S/N expressed as dB relative to rated output, weighting stated (§14) -- Measurement conditions (source impedance, load impedance) present in output if logged - -**IEC 61260-1** apply to any fractional-octave band analysis. Check: -- Filter class (1 or 2) stated in output -- Bandwidth designator follow standard notation (e.g. `1/3-octave`, not `third-octave`) -- Attenuation at band edges meet class requirements - -**IEC 61672-1** apply when A-, C-, or Z-weighting used. Check: -- Weighting designator explicit in output label (`dBA`, `dBC`, `dBZ`) -- Time constant stated when time-weighted levels displayed (`F`, `S`, or `I`) - -**ITU-R BS.468-4** apply to noise measurements using quasi-peak detection or 468-weighted noise figures. Check: -- Detector type stated (`quasi-peak` vs `RMS`) -- Weighting curve identified in output if not unweighted - -**ITU-R BS.1770-5** apply if integrated loudness or true-peak values appear. Check: -- Integrated loudness expressed as `LUFS` (not `LKFS` — both used in wild, LUFS is current preferred term per BS.1770-5 §3) -- True-peak expressed as `dBTP`, not `dBFS` -- Gating behaviour (absolute + relative gates) match §2.7 if implemented - -**ISO 18233** apply to swept-sine / deterministic-signal measurement. It is a *substitution* standard — §1 gives methods used "as substitutes for measurement methods specified in standards covering classical methods", and §9(c) require the report name the applicable classical standard. It never stands alone. Check: -- A room measurement cite ISO 18233 **and** the classical standard it substitutes for (ISO 3382-1 or 3382-2). One without the other is incomplete. -- A quasi-anechoic loudspeaker / PA measurement cite **neither** — no classical method in §1's list covers it. That case want IEC 60268-21, which is not held. AES17-2020 A.4.5 + Farina remain its citation. -- Annex B is normative. Clause strings for it must not say "(informative)"; that qualifier belongs to AES17 A.4 and IEC 61260-1 Annex G. - -### standards check procedure - -Every PR touching output formatting, unit display, or measurement computation: - -1. Identify which standard(s) apply to changed code (use table above) -2. Read relevant clause in actual PDF — no memory, no summary above; summaries are orientation, not authoritative -3. Answer: does implementation match standard's requirements for both value computation AND display/labelling format? -4. Cite standard + clause number in review comment, e.g.: - `AES-17-2015 §6.3: THD+N must be referenced to fundamental level, not total RMS` -5. PR output format differ from standard → flag as correctness issue even if math right — display conformance is part of correctness here - -No applicable standard covers changed behaviour → write -`standards check: not applicable — {reason}` in review comment, not omit section. - - -- PR diff -- PR body (written by dev agent — files touched, test output, open questions) -- Original issue + triage spec comment (acceptance criteria) -- Architect design comment (if present) - ## what you must do ### step 1 — check spec coverage Walk each acceptance criterion in triage spec comment. Each one: addressed by diff? Note gaps. -Branch on the criterion's provenance tag (`triage.md`/`architect.md` set it; +Branch on the criterion's provenance tag (triage/architect set it; tag definitions live in `AGENTS.md`'s evidence-discipline section — no memory, no redefinition here): @@ -159,20 +71,16 @@ into this one. ### step 2 — review the diff -Before opening files, run the two repowise calls that take the range you -already have. Both are locators under `AGENTS.md`'s repowise rule — they say -where to look, never what is true: +Start from the changed-file list itself. Two questions it answers before any +file opens, and both are cheap: -- `get_risk(targets, changed_files=)` — read its `directive` - first. `missing_cochanges` on a daemon-handler diff that does not touch - `ac-cli`/`ac-view` is the wire-schema check firing before any file opens. - `will_break`, `missing_tests` and `tests_to_run` are leads to verify. -- `get_change_risk("..")` — scores the range rather than the paths. - Lead with `risk_percentile`. +- **Does the diff touch `ac-daemon`'s published frame without touching + `ac-cli` and `ac-view`?** That is the wire-schema check firing off the file + list alone. `ac-rs/ZMQ.md` names the contract. +- **Does it touch a crate whose consumers are not in the diff?** Same shape, + one level out. -**A clean report licenses nothing.** The checklist below runs in full either -way; these calls change the order you read in, not whether you read. A finding -that cites either without an opened file is not a finding. +Both are leads, not findings. The checklist below runs in full either way. Check: - **correctness** — implementation do what spec says? @@ -180,33 +88,15 @@ Check: - **wire schema** — `ac-daemon`'s published frame changed → do `ac-cli` and `ac-view` match? (`ac-rs/ZMQ.md`) Cross-crate check (schema match, existing helper, pattern used elsewhere) → -`get_context` on the symbol with `include=["callers"]`, then `get_symbol` on -each hit. When `_meta.indexed_commit` equals the tip you are reviewing, that -`get_symbol` result *is* the verified read and no `Read` is needed; when it -does not, or when the tool is unavailable, fall back to `Grep` for the symbol -then `Read` the hit. Shell readers and searchers denied by +`Grep` for the symbol across the workspace, then `Read` each hit. The `Grep` +tells you where; only the `Read` tells you what, and a citation to a line you +did not open is not a citation. Shell readers and searchers denied by `.claude/settings.json`; do not work around them. - **error handling** — Results propagated, not silently unwrapped? - **test coverage** — new code paths exercised by tests? - **coupled constants** — PR introduce or change a constant whose correct value depends on another constant (same crate or cross-crate — of the - first three instances of this shape, #238 and #247 crossed a crate - boundary; #246 did not, `MIN_PROMINENCE` and `NOISE_FLOOR_PROMINENCE` - both lived in `ac-core/src/visualize/transfer.rs` — so crate boundary is - not the operative reason review misses these) → requires a test asserting - the *relationship* between the two constants, not a test that merely - exercises each value in isolation. Worked example already in tree: - `the_admission_constant_leaves_room_before_the_advice_fires` - (`ac-rs/crates/ac-scene/src/fault.rs:1473`, added by PR #253). That test - carries both failure modes such a test must have — require both: - - fails when the two constants move to a *wrong pair* (measured worst - first-lock attempt no longer clear of the advice threshold); - - fails when either constant moves to a value *nobody has scored* — the - `RIG_WORST_ATTEMPT_TO_FIRST_LOCK` lookup table (same file, line 1453) - has no row for it and the test panics with instructions, rather than - silently passing. - Missing this test on a coupled-constant PR is a `needs-work` blocker, not - a note. + first three instances of this shape). - **scope discipline** — dev touch files outside spec? Yes → flag. - **no dead code** — no commented-out blocks, no unreachable branches @@ -230,10 +120,7 @@ Each new test: - Measurement functions: numeric assertions with tight tolerances? Example: `assert!((result.thd - 0.0023).abs() < 1e-4)` not just `assert!(result.thd > 0.0)` - CLI behavior: output strings or exit codes asserted? - -`get_health` untested-hotspot entries are a useful pointer to where a missing -test would matter most — a locator for this step, not a finding in it. A file -it flags still gets opened before the review says anything about its coverage. +- Do standards testing on Tier 1 scoped PRs (see docs/architecture/standards.md) Tests missing or weak → write missing tests yourself, include in review comment as suggested additions. @@ -299,9 +186,7 @@ would falsify the claim. See step 5.} sees). See below — this is a different verdict from a correctness finding and routes elsewhere. -`claude-approved` is not a merge signal. It puts the PR in the Codex queue -(`.agents/codex-qa.md`); merge needs `codex-approved` as well, and needs a -human. You never set or clear `codex-approved` — if you disagree with a Codex +`claude-approved` is not a merge signal. It puts the PR in the Codex queue merge needs a human. You never set or clear `codex-approved` — if you disagree with a Codex finding, say so in your review comment and leave the label alone. ### sending it back to architect or ux — the design is wrong, not the code @@ -342,6 +227,10 @@ This is a heavier verdict than request-changes and it costs a full re-review at the same tip. A finding you can state as "this line is wrong" is not one of these. Use it when you can state what the *spec* got wrong. + +### loopback IR testing +see docs/runbooks/loopback-ir.md + ### `requires-rig` — you set it, only a human clear it Some claims cannot be settled by reading code or running the workspace suite. @@ -368,7 +257,7 @@ justification — do not downgrade to `request-changes` to express it, because that send the PR back to a developer who cannot take the measurement either. Where the measurement is one the rig role would take, say which block of -`work/rig/rig-verify-queue.md` it belong to, or that it needs a new one. The +`rig/rig-verify-queue.md` it belong to, or that it needs a new one. The rig role produce the measurement record; you do not run the session and you do not act on a result that does not exist yet. @@ -386,8 +275,8 @@ and inconvenient to check. This rule is load-bearing: merge to main is human-only precisely because agent review is not independent (see `AGENTS.md` human gates), so this is the one mechanism stopping an approved-then-amended PR from reaching that human merge -unreviewed. `agent:qa` approval attest to tree **at commit it reviewed**, not branch forever. **Any commit pushed after approval revert PR to `needs-work`, remove -`claude-approved`, and require fresh gate pass** — re-run full check (`cargo test`, `cargo clippy -- -D warnings`, `cargo fmt --check`) against new tip, re-review delta before label return to `in-review`. Hold even when post-approval commit "look harmless" (fmt reflow, comment, doc tweak): gate cannot distinguish whitespace change from logic change by trust, only by running, and highest-consequence PRs (drive-path, wire protocol) are exactly where ungated post-approval commit do most damage. Rule exist because real one slipped through: #197's closure-evidence commit landed on `main` unformatted and CI-red *after* approval (#199). Relay between "approved" and "merged" is seam like any other — close structurally, not by remembering to re-check. +unreviewed. **Any commit pushed after approval revert PR to `needs-work`, remove +`claude-approved`, and require fresh gate pass** — re-run full check (`cargo test`, `cargo clippy -- -D warnings`, `cargo fmt --check`) against new tip, re-review delta before label return to `in-review`. Hold even when post-approval commit "look harmless" (fmt reflow, comment, doc tweak): gate cannot distinguish whitespace change from logic change by trust, only by running, and highest-consequence PRs (drive-path, wire protocol) are exactly where ungated post-approval commit do most damage. Removing `claude-approved` is part of the rule, not bookkeeping after it. The label is what puts a PR in the Codex queue and what a human reads at the merge @@ -408,76 +297,3 @@ them. - No flag style preferences as correctness issues. Clippy is style arbiter. - Bug found outside PR scope → open new issue, no block this PR for it. - One review comment per PR pass. Dev push fix → second pass. -- **Value-display PRs — display-truth gate (A3 rendering half, discharged).** - Value-display PR = any PR changing what get rendered/printed: - spectrum/waterfall/ember/scope trace data, transfer magnitude/phase - traces, coherence mask, delay readout (ms and meters), - input-level meter heights and clip latch, stimulus banner strings, - axis calibration, printed/CSV values, or post-receiver display - buffer feeding them. Old `ac-ui --headless-test` T2/T3 harness (#170) - removed with ac-ui detach (`attic/ac-ui`); rendering half of - A3 since **re-homed**, so this live gate again, not blocking pause. - Enforce as two layers: - - **Scene-computed values** (every number, string, normalized - coordinate) gated by `ac-scene`'s display-truth fixture tests — - pure crate, CI-blocking, no GPU. These authoritative; value-display - PR whose numbers live in `ac-scene` fully gated here. - - **`ac-view` drawing** (affine map to screen, gap rendering, layout) - gated by `ac-view` harness — `it_geometry` (shape/vertex - assertions, mutation-verified), `it_live_end_to_end` and - `it_snapshot_end_to_end` (on-screen string equals `ac-scene`'s - output for same frame, asserted at harness level), `it_remote`, - `it_trace_distinction` — plus, per A3 resolution - (`work/handoff/handoff-ac-view.md`, accepted at M2/M3 signoff), **one manual - real-adapter run with screenshot attached to PR** as pixel-level - evidence. That run documented, not CI-blocking (sandbox - lavapipe segfaults — standing policy); QA judge adequacy. Pixel - truth still no CI harness — accepted M3+ posture, not pending blocker. - PR changing only internal correctness checks (CSV export, cursor - readout) outside this gate. - - **Reference currency (#337).** PR touches `draw_view` or a pane - module → PR body must show one of: the 7 `it_transfer_snapshots` - references regenerated on the rig in this PR (box + date + commit, - matching the provenance line in `it_transfer_snapshots.rs`'s doc - comment), or a stated reason the change cannot affect rendered - pixels. Neither present → `needs-work`, not a note — a stale - reference is a gate reporting coverage it does not have. See - `TESTING.md` → "A3 snapshot reference currency". -- **Daemon-pipeline PRs — I5 temporal soak (A3 soak half, STILL - OUTSTANDING).** No approve daemon-pipeline PR (anything touching - `ac-daemon/src/handlers/audio/monitor.rs`, ring buffers / - time-integration state feeding it, or display buffer it publishes - into) on single-snapshot checks alone. I5 soak - (formerly `ac-ui --headless-test`'s "I5 soak", removed in same - detach) **not** re-homed daemon-side — unlike rendering half above, - this half of A3 genuinely still missing (see tracking issue). - I1-I4 are single-snapshot checks — settle, read one frame, - judge — structurally blind to any bug with onset delay - (ring-buffer wrap, EMA/state poisoning, cadence-boundary mishandling). - Conforming soak run seeded deterministic fake-audio stimulus long - enough to exceed every internal buffer period (derived from - daemon's own reported `lf_fft_n`/`lf_overlap_pct`/`lf_avg_tau_ms`, not - hardcoded) and assert I4-t bounded / I2-t continuity / I5a liveness / - I5b plausibility on every published frame, not just last one. Until - such soak exist daemon-side, require PR-specific temporal argument - (targeted test or reasoned case) for this PR class; no accept - I1-I4 as sufficient. - **Scope note:** this bullet gate *daemon-pipeline* PRs only. Pure - `ac-view` drawing PR or pure `ac-scene` PR touch neither `monitor.rs` - nor daemon pipeline — not subject to it; gated by display-truth layers above. - -### drive-path safety (any PR touching stimulus/`set_drive`) - -Do not approve unless ALL of the following are demonstrated by tests, not by reading: - -- [ ] Sessions launch with drive **off**; no code path starts drive without an explicit - `set_drive on`. -- [ ] Panic stop works from BOTH armed and driving states (state-machine tests). -- [ ] Dead-man: drive drops within 1.5 s of keepalive silence (integration test, - fake-audio); the session itself keeps running. -- [ ] Level is clamped to `drive_max_dbfs` at every entry point — UI (arrow keys, - overlay), CTRL `set_drive`, and every daemon command that puts a stimulus on a - physical output (`plot`, `plot_level`, `plot_ir`, `generate`, `generate_pink`, - `sweep_level`, `sweep_frequency`, `calibrate`, and `transfer_stream`'s own - self-driving `level_dbfs` — #360) — test each entry point, not one representative. -- [ ] `set_drive off` silences output within one audio block (fake-audio energy test). diff --git a/.agents/rig.md b/.agents/rig.md index f298fc16..0bd1ea1e 100644 --- a/.agents/rig.md +++ b/.agents/rig.md @@ -3,8 +3,15 @@ ## identity Rig agent for `ac` repo (github.com/mkovero/ac). Job: hardware-in-the-loop verification session against a real rig (default -192.168.9.25 — RME Babyface Pro, speakers on ADAT out, mic on IN1; confirm -wiring against `work/rig/` before assuming it hasn't moved). Produce a +192.168.9.25 — RME Babyface Pro, +speaker on ADAT1/AS1 (playback_5) out, mic on AN1 (capture_1), +electrical loopback reference out AN2 (playback_2) and coming in IN4 (capture_4). +Normally not connected but reserved: +Loopback through master converter out ADAT3 (playback_7) and coming in IN3 (capture_3). +Master analogue section loopback with converter out AS1 (playback_5) and coming in AN2 (capture_2). +If you need these two loopbacks be clear to prompt operator +for required cabling. +Produce a measurement record with confounds stated. **Permitted, and expected, to decline to conclude** when the data does not support a pass/fail score — the two rig sessions that did this are the good examples this role is @@ -50,13 +57,22 @@ survived contact with this rig and what didn't: the external master clocks the card over ADAT, and ADAT carries the stimulus leg (`playback_5`). Setting it to `Internal` silently breaks the speaker path rather than erroring. +- Set without confirming (at rig): + amixer -c0 cset numid=1 0 # dont monitor mic -> AN1 + amixer -c0 cset numid=14 0 # dont monitor mic -> AN2 + amixer -c0 cset numid=301 36 # mic input gain (36=max) + amixer -c0 cset numid=295 46341 # playback_7 output level + amixer -c0 cset numid=293 16384 # playback_5 output level + amixer -c0 cset numid=294 16384 # playback_6 output level + amixer -c0 cset numid=308 0 # IN4/capture_4 level (no gain) + amixer -c0 cset numid=307 0 # IN3/capture_3 level (no gain) + amixer -c0 cset numid=302 1 # AN1/capture_1 mic input 48V on + amixer -c0 cset numid=305 0 # AN2/capture_2 mic input 48V off + amixer -c0 cset numid=289 16384 # AN1/playback_1 output level + amixer -c0 cset numid=290 16384 # AN2/playback_2 output level + - Record what is physically connected — every leg, reference and - measurement, by output/input index, not by what a handoff document says - it should be. A stale wiring assumption inherited from a handoff cost - three sessions once (`rig-session-2-results.md`'s reference-leg finding: - the handoff's documented wiring was dead, and a different pair was live). - Confirm with a routing probe if there is any doubt, not from memory of a - previous session's layout. + measurement, by output/input index, not by what a handoff document says it should be. Let operator know what is your idea of the outputs/inputs today. - Stop the daemon before installing a build over it. `install -m 755` over a running `ac-daemon` may fail `Text file busy`, or may succeed and leave an ambiguous state — see `work/rig/rig-verify-queue.md` for whether this has @@ -131,7 +147,7 @@ Interlocks. A session may not proceed past these — not guidance, blocking: size+mtime-only check as not having verified the build at all. - **Confound is a required field in the record, every run.** An empty field is a defect in the record, not a claim of a clean run. -- **What is physically connected, and that the clock stayed `AutoSync` +- **What is physically connected (or a stated reason it did not), are required fields.** Do not write a record that omits either. - **When definitions change (config, gate constant, metric being diff --git a/.agents/triage.md b/.agents/triage.md index 551b6ba9..42178a0c 100644 --- a/.agents/triage.md +++ b/.agents/triage.md @@ -101,10 +101,12 @@ to them. Both set → ux comment first, architect still own promotion to Epic (multiple independent work pieces) → `epic`. Break into sub-issues, reference them in comment before labeling parent `epic`. -`get_context` and `get_risk` may inform the "files likely affected" line — -they are cheaper than exploring the tree, and that line is best-effort either -way. It stays best-effort: a repowise result is a locator (`AGENTS.md`), so it -never turns into an acceptance criterion or a claim about how the code works. +The "files likely affected" line is best-effort and stays that way. Name the +crate and the module from the map above where you can, `unknown` where you +cannot. Do not explore the tree to firm it up — that is the developer's step 1, +it is cheaper there, and a guess written confidently here becomes a scope +boundary nobody intended. It never turns into an acceptance criterion or a +claim about how the code works. ## hard constraints - No code or pseudocode in spec comments. diff --git a/.claude/settings.json b/.claude/settings.json index e234e349..894078db 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -1,7 +1,20 @@ { "permissions": { + "allow": [ + "Bash(cargo:*)", + "Bash(git:*)", + "Bash(gh:*)", + "Bash(grep:*)", + "Bash(sed:*)", + "Bash(python3:*)", + "Bash(head:*)", + "Bash(tail:*)", + "Bash(cut:*)", + "Bash(wc:*)", + "Bash(sort:*)", + "Bash(uniq:*)" + ], "deny": [ - "Task(Explore)", "Bash(awk:*)", "Bash(less:*)", "Bash(more:*)", @@ -10,32 +23,17 @@ "Bash(rg:*)", "Bash(ag:*)", "Bash(ack:*)", - "Bash(find:*)", "Bash(fd:*)", - "Bash(ls:*)", "Bash(tree:*)", "Bash(perl:*)", "Bash(ruby:*)", "Bash(node:*)", "Bash(truncate:*)", "Bash(dd:*)", - "Bash(cp:*)", "Bash(mv:*)", - "Bash(rm:*)" - ], - "allow": [ - "Bash(cargo:*)", - "Bash(git:*)", - "Bash(gh:*)", - "Bash(grep:*)", - "Bash(sed:*)", - "Bash(python3:*)", - "Bash(head:*)", - "Bash(tail:*)", - "Bash(cut:*)", - "Bash(wc:*)", - "Bash(sort:*)", - "Bash(uniq:*)" + "Read(./tests/fixtures/**)", + "Read(./work/**)", + "Read(./audit/**)" ] } } diff --git a/bin/codex-qa.sh b/bin/codex-qa.sh old mode 100644 new mode 100755 diff --git a/bin/common.sh b/bin/common.sh index 4fb98625..c846f551 100755 --- a/bin/common.sh +++ b/bin/common.sh @@ -31,7 +31,7 @@ export AC_STDDOCS="${AC_STDDOCS:-$ROOT/stddocs}" AC_HOME="${AC_HOME:-$(dirname "$ROOT")/ac-wt}" WT_BASE="${AC_WT_BASE:-$AC_HOME/wt}" AC_LOG_DIR="${AC_LOG_DIR:-$AC_HOME/log}" -AC_SESSION_DIR="${AC_SESSION_DIR:-$ROOT/work/sessions}" +AC_SESSION_DIR="${AC_SESSION_DIR:-$AC_HOME/session}" # One shared target dir, not one per branch. Per-branch was warm across runs on # the same issue, but cost several GB each and left orphans behind every merge. @@ -84,11 +84,15 @@ export AC_STDDOCS="${AC_STDDOCS:-$ROOT/stddocs}" # Raw transcripts: large, noisy, never committed. The distilled final message # goes to AC_SESSION_DIR, which is in the repo. AC_LOG_DIR="${AC_LOG_DIR:-$AC_HOME/log}" -AC_SESSION_DIR="${AC_SESSION_DIR:-$ROOT/work/sessions}" -# Task = delegation tool. Without it a session cannot reach explorer and reads -# every file itself, in its own context. Verify against a transcript after any -# upgrade: jq -r 'select(.type=="system") | .tools // empty | .[]' +# Task = delegation tool. Whether a session can actually reach a subagent is +# NOT settled by this list: `.claude/settings.json` denies `Task(Explore)` and +# run() exports CLAUDE_AGENT_SDK_DISABLE_BUILTIN_AGENTS=1, either of which is +# enough to make it dead weight. Do not infer the answer from these three +# settings — read it off a transcript, which is the only place it is observable: +# jq -r 'select(.type=="system") | .tools // empty | .[]' +# If Task is absent there, drop it from these lists rather than leaving a tool +# in an allowlist that nothing can call. TOOLS_WRITE="Read,Grep,Glob,Edit,Write,Bash,Task" TOOLS_READ="Read,Grep,Glob,Bash,Task" @@ -189,6 +193,17 @@ link_support() { return 0 } +# Heavy trees an implementation never needs. Cheaper and more reliable than a +# Read deny rule: a file that is not on disk cannot be found by any tool. +sparse_trim() { + local wt="$1" + [[ -n ${AC_NO_SPARSE:-} ]] && return 0 + local -a pat + read -r -a pat <<< "${AC_SPARSE:-/* !/work/ !/audit/}" + git -C "$wt" sparse-checkout init --no-cone 2>/dev/null || return 0 + git -C "$wt" sparse-checkout set "${pat[@]}" +} + # Count QA's output on a PR. It may land as an issue comment OR as a review # (gh pr review --comment creates the latter, and --json comments does not # return those). Count both, or a good review reads as silence. @@ -203,6 +218,15 @@ qa_evidence() { echo $(( ${c:-0} + ${r:-0} )) } +# The architect's file manifest for an issue: repo-relative paths, one per line. +# Empty output means no manifest — the caller decides whether that is fatal. +manifest_of() { + gh_retry gh issue view "$1" -R "$AC_REPO" --json comments \ + --jq '[.comments[] | select(.body | test(""))] | last | .body // ""' \ + | sed -n '/^```files[[:space:]]*$/,/^```[[:space:]]*$/p' \ + | sed '1d;$d; s/^[[:space:]]*//; s/[[:space:]]*$//' \ + | grep -v '^$' || true +} # Extract the session's final message from a finished transcript. # Prefer the result event; fall back to the last assistant text block, because # not every version emits result into the stream — an interrupted run has none @@ -234,6 +258,11 @@ run() { *) turns=80 ;; esac fi + case "$role" in + developer|qa) AC_MODEL=sonnet ;; + architect|ux) AC_MODEL=opus ;; + *) AC_MODEL=sonnet ;; + esac local fg="" tools="$TOOLS_WRITE" deny="$DENY_ASYNC" mode="acceptEdits" arg local -a extra=() for arg in "$@"; do @@ -253,15 +282,29 @@ run() { mkdir -p "$CARGO_TARGET_DIR" local tag="${AC_TAG:-$$}" stamp status=0 + mkdir -p "$AC_LOG_DIR" "$AC_SESSION_DIR" stamp="$(date +%F)-$role-$tag" + + # The tag is not unique. revise.sh uses pr--rev for EVERY round, so round + # two overwrote round one — transcript, distilled output, and the --resume id + # with it. Same for a re-run of implement.sh on one issue in a day. Suffix + # instead of clobbering: the run you want to read is usually the earlier one, + # and a tool that deletes the evidence of its own cost cannot be audited. + if [[ -e "$AC_LOG_DIR/$stamp.jsonl" || -e "$AC_SESSION_DIR/$stamp.md" ]]; then + local i=2 + while [[ -e "$AC_LOG_DIR/$stamp-$i.jsonl" || -e "$AC_SESSION_DIR/$stamp-$i.md" ]]; do + (( ++i )) + done + stamp="$stamp-$i" + fi + local raw="$AC_LOG_DIR/$stamp.jsonl" local out="$AC_SESSION_DIR/$stamp.md" - mkdir -p "$AC_LOG_DIR" "$AC_SESSION_DIR" # Stream to the terminal, keep the raw transcript. Distillation happens after # the run, not inside the pipe — a process-substitution tee races the # pipeline's exit and truncates exactly the long sessions worth reading. - CLAUDE_AGENT_SDK_DISABLE_BUILTIN_AGENTS=1 \ +# CLAUDE_AGENT_SDK_DISABLE_BUILTIN_AGENTS=1 \ claude -p --system-prompt-file "$(spec "$role")" "$prompt" \ --model "${AC_MODEL:-sonnet}" \ --allowedTools "$tools${GH_TOOLS:+,$GH_TOOLS}" \ diff --git a/bin/implement.sh b/bin/implement.sh index c9d7dc50..cdb66685 100755 --- a/bin/implement.sh +++ b/bin/implement.sh @@ -19,8 +19,30 @@ fi require_space "$wt" || exit 1 git fetch -q origin main git worktree add -B "issue-$n" "$wt" origin/main >/dev/null +sparse_trim "$wt" cd "$wt" link_support "$wt" -AC_TAG="issue-$n" run developer "Implement issue #$n in $AC_REPO." "$@" +mapfile -t files < <(manifest_of "$n") + +if (( ${#files[@]} == 0 )); then + echo "no file manifest on #$n — run bin/design.sh $n first" >&2 + echo " (AC_NO_MANIFEST=1 to implement blind, at roughly 3x the cost)" >&2 + [[ -n ${AC_NO_MANIFEST:-} ]] || exit 1 +fi + +for f in "${files[@]}"; do + [[ -e $f ]] || echo "note: manifest names $f — not in tree, assuming new file" >&2 +done + +AC_TAG="issue-$n" run developer "Implement issue #$n in $AC_REPO. + +The architect's design comment names the files this change touches. That list is your scope: + +$(printf '%s\n' "${files[@]}") + +Read those files first, in that order, before anything else. Do not sweep the tree. Do not Glob or Grep to find what to work on — the search has already been done and its result is above. Grep is for locating a symbol inside a file already on this list. + +A file you need that is not on the list is a finding about the design, not a gap for you to fill. Stop, comment on the issue with the path and why it is needed, apply needs-design, and end the run. Adding it silently is the exact failure this list exists to prevent." "$@" + echo "worktree: $wt branch: issue-$n" >&2 diff --git a/bin/master.sh b/bin/master.sh old mode 100644 new mode 100755 index 4cd9496e..348b0257 --- a/bin/master.sh +++ b/bin/master.sh @@ -102,7 +102,7 @@ triage_evidence() { # drive() sets it when architect or ux has just changed the design under a diff # that may already carry an approval of the design it replaced. qa_loop() { - local n="$1" pr="$2" force="${3:-}" ls ils before after head mark ev + local n="$1" pr="$2" force="${3:-}" ls ils before after head mark ev pre post # Not every exit path sets STATE, and drive() re-enters this function after a # handback. A STATE left over from the previous entry would read as a second # handback and loop until the step limit — which looks like cycling labels @@ -142,7 +142,27 @@ qa_loop() { echo " retire the measurement; the label stays for you to clear." fi echo " #$n PR #$pr: revising (round $qa_round)" + pre="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" \ + || { echo " #$n: cannot read the tip — not starting a revise"; return 1; } "$BIN/revise.sh" "$pr" $fg || { echo " #$n: revise failed"; return 1; } + post="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" \ + || { echo " #$n: cannot read the tip — check the PR by hand"; return 1; } + + # A revise that pushed nothing is the developer saying the block is not + # code-fixable. Clearing needs-work here would be this script overruling + # that on the developer's behalf — and worse, the reviewed-SHA cache below + # would then see a tip qa has already reviewed with a comment on it and + # report "raised nothing", which is how a request-changes verdict turns + # into "yours to merge". Leave the label. Stop. + if [[ $pre == "$post" ]]; then + echo " #$n PR #$pr: revise pushed nothing — tip is still $post" + echo " needs-work stays. re-reviewing an identical tip cannot change" + echo " the verdict, so the block is one only you can clear: a rig" + echo " measurement, acceptance of an assumed criterion, a design call." + echo " read the developer's PR comment for which." + STATE=needs-human; return 0 + fi + gh_retry gh pr edit "$pr" -R "$AC_REPO" \ --remove-label needs-work --add-label in-review >/dev/null 2>&1 || true ls="$(pr_labels "$pr")" diff --git a/bin/revise.sh b/bin/revise.sh old mode 100644 new mode 100755 index 738ee1e6..a362283a --- a/bin/revise.sh +++ b/bin/revise.sh @@ -18,6 +18,7 @@ require_space "$WT_BASE/$branch" || exit 1 wt="$(ensure_worktree "$branch" "$WT_BASE/$branch")" \ || { echo "cannot get a worktree for $branch" >&2; exit 1; } cd "$wt" +sparse_trim "$wt" git pull -q --ff-only 2>/dev/null || true link_support "$wt" diff --git a/bin/session.sh b/bin/session.sh index 90923809..bfcac44d 100755 --- a/bin/session.sh +++ b/bin/session.sh @@ -9,10 +9,12 @@ # delegate explorer/subagent calls and their briefs # final last assistant message — the distilled output # errors failed tool results and rate limit events +# weight context each tool put in front of the model, by result bytes +# cost one TSV line: file, turns, usd, seconds # types event type histogram, for when a filter stops matching set -euo pipefail -f="${1:?usage: session.sh [summary|tools|text|files|delegate|final|errors|types]}" +f="${1:?usage: session.sh [summary|tools|text|files|delegate|final|errors|weight|cost|types]}" view="${2:-summary}" # Collapse newlines: a heredoc in a Bash command otherwise becomes several @@ -55,12 +57,45 @@ errors() { | "RATE LIMIT " + (. | tostring | .[0:200])' "$f" } +# How much context each tool actually put in front of the model, by result +# size. Call counts are misleading on their own: one tool returning 40 kB of +# prose costs more than twenty Greps returning line numbers. This is the view +# that settles "is earning its place" — it is a measurement, not an +# impression, and it is the only one that separates a locator from a payload. +weight() { + jq -rs ' + ( [ .[] | select(.type=="assistant") | .message.content[]? + | select(.type=="tool_use") | {key: .id, value: .name} ] + | from_entries ) as $name + | [ .[] | select(.type=="user") | .message.content[]? + | select(.type=="tool_result") + | {name: ($name[.tool_use_id] // "unknown"), + n: (.content | tostring | length)} ] + | group_by(.name) + | map({name: .[0].name, calls: length, bytes: (map(.n) | add)}) + | sort_by(-.bytes) + | (["BYTES","CALLS","TOOL"], (.[] | [.bytes, .calls, .name])) + | @tsv' "$f" +} + +# One line per session, for aggregating across a whole log directory: +# for j in ~/src/ac-wt/log/*.jsonl; do bin/session.sh "$j" cost; done | sort -k3 -rn +cost() { + jq -r --arg f "$(basename "$f")" ' + select(.type=="result") + | [$f, (.num_turns // "?"), (.total_cost_usd // "?"), + ((.duration_ms // 0) / 1000 | floor)] + | @tsv' "$f" +} + case "$view" in tools) tools ;; text) text ;; files) files ;; final) final ;; errors) errors ;; + weight) weight ;; + cost) cost ;; types) jq -r '.type' "$f" | sort | uniq -c | sort -rn ;; delegate) jq -r 'select(.type=="assistant") | .message.content[]? | select(.type=="tool_use" and (.name|test("Agent|Task";"i"))) diff --git a/bin/triage.sh b/bin/triage.sh old mode 100644 new mode 100755 diff --git a/work/rig/rig-verify-queue.md b/rig/rig-verify-queue.md similarity index 96% rename from work/rig/rig-verify-queue.md rename to rig/rig-verify-queue.md index 57b03d6e..f03ecb49 100644 --- a/work/rig/rig-verify-queue.md +++ b/rig/rig-verify-queue.md @@ -26,23 +26,6 @@ only planned run producing a legitimately gated ring, which is the case the dropped onset guard must not suppress, so it carries per-frame `median_value` / `negative_lag_median` as well. Full statement in block 4. -- **#368's `TAU_SNR_THRESHOLD_DB` constant — QA on PR #384 (2026-08-23) - flagged it `derived`, not measured on the sweep configuration it gates.** - Its two anchors (33.8–83.5 dB electrical loopback, ~16 dB #376 acoustic - cliff) come from a different window-length rig session - (`rig-2026-08-22-tau-window-350-results.md`) and a different, longer-ESS - acoustic path — neither is `calibrate`'s own short-ESS electrical τ path. - - Run `calibrate`'s actual τ path against the three cases the issue - measured: hot loopback (+3.01 dB), low-gain loopback (-4.19 dB), muted - route (-83.8 dBFS). Record `tau_pre_impulse_snr_db` for each. - - > **Pass: both real loopbacks read at or above 24 dB, and the muted - > route reads below it.** Either real loopback's SNR coming back under - > 24 dB would wrongly refuse a working cable; the muted route's SNR - > coming back over 24 dB would wrongly accept noise as a peak. Both are - > falsifications of the current constant, not readouts to shrug past. - Two things session 3 raised that no block here covers yet: - **The cable change, and the one measurement that verifies it — #243.** Move @@ -304,12 +287,21 @@ channel, which is #204. **For #346/#352 — score it c-free, not against tape.** AC5's wording anchors to `transfer_stream`'s 4.7 mm, which the tape cannot support. Its intent - survives in the form `rig-test-plan.md` already recommends: - `|Δt_onset − Δt_transfer_stream| ≤ 1.3 samples`, each estimator's own - increment between the two positions, compared in the time domain where tape - and `c` both drop out. Both estimators see the same physical move whatever - the tape says it was. That is the only valid form here, not merely a - stronger one. + survives in the form `rig-test-plan.md` already recommends: compare each + estimator's own increment between the two positions in the time domain, + where tape and `c` both drop out. Both estimators see the same physical + move whatever the tape says it was. That is the only valid form here, not + merely a stronger one. **The 1.3-sample bar itself was also a tape draw + converted to samples** and was re-derived 2026-08-24 (#375) to + `|Δt_est − Δt_transfer_stream| ≤ 3 × se(Δt_est)`, scored against the + candidate estimator's own measured repeatability — see `rig-test-plan.md`. + **The 3σ multiplier was accepted 2026-08-25** (mkovero on #375) and is fixed + before any verification run; do not re-open it once a run's numbers are + known. Two riders are still undecided and are recorded with the bar in + `rig-test-plan.md`: whether to score with a t-multiplier at the actual df or + hold n ≥ 12 per position under the normal approximation, and whether a fixed + physical tolerance is needed alongside `3 × se` — which on its own rewards a + noisy estimator with a wider bar. - **`ac-view` transfer snapshots regenerated for #356 — done 2026-08-20, one open finding.** Ran on 192.168.9.25 (RTX 2070) at `issue-243` diff --git a/work/sessions/2026-08-12-architect-issue-281.md b/work/sessions/2026-08-12-architect-issue-281.md deleted file mode 100644 index c11a6795..00000000 --- a/work/sessions/2026-08-12-architect-issue-281.md +++ /dev/null @@ -1,15 +0,0 @@ - - - - -Done. Issue #281 review posted, labels updated. - -**design comment posted** (comment link: github.com/mkovero/ac/issues/281#issuecomment-5270248019) - -Core call: τ history nests inside existing `CalibrationEntry` as `tau_history: Vec` (same `cal.json`, same `out{N}_in{M}` key), not a separate file. Reason: repo already has a tested pattern for this exact shape (`mic_response` addition), and #279 just paid down a bug in the "load-merge-preserve-save" discipline — a second file duplicates that risk with zero test coverage. - -Flagged gap not in spec: `AudioEngine` trait exposes no `period_size` or device-identity accessor today. Resolved it — `device` reuses existing `Config::device`, new `period_size()` trait method (default `None`, JACK backend implements via `buffer_size()` on the underlying client, queried fresh each run since jackd can rebuffer mid-session). - -Wire impact: `cal_done` gains `tau_state` + `tau_s`, no new prompt step (rides step 2's existing loopback check). `ac-cli` render path needs an arm for it; `ac-view` untouched (doesn't subscribe to calibrate topics). - -Labels: removed `needs-design`, added `ready-to-implement` + `needs-ux` (new field reaches an operator-facing readout in `ac calibrate`'s terminal output). diff --git a/work/sessions/2026-08-12-architect-issue-282.md b/work/sessions/2026-08-12-architect-issue-282.md deleted file mode 100644 index 668d572f..00000000 --- a/work/sessions/2026-08-12-architect-issue-282.md +++ /dev/null @@ -1,6 +0,0 @@ - - - -Posted. Design comment on #282, `needs-design` off, `ready-to-implement` on. - -Core call: rename `sweep_ir`→`plot_ir` on wire (only `ac-cli` consumes it, no `ac-view`/`ac-scene` hit), leave `sweep_level`/`sweep_frequency` wire names alone under new `generate level`/`generate frequency` nouns — precedent already exists (`generate sine`→`"generate"`). Flagged two implementation-level gotchas for the developer: no suffix convention yet for `n_harmonics`/`window_len` (bare int collides with dBFS default), and `tail_s`'s ISO 18233 basis can only be a post-hoc ≥30dB-decay check via existing `Filterbank`, not a pre-capture computed value — no RT60 estimator exists to compute one blind. diff --git a/work/sessions/2026-08-12-developer-1937543.md b/work/sessions/2026-08-12-developer-1937543.md deleted file mode 100644 index 86d621c5..00000000 --- a/work/sessions/2026-08-12-developer-1937543.md +++ /dev/null @@ -1,4 +0,0 @@ - - - -Done. Pushed `f15f8d4` to `issue-279`, all 3 gates green, posted reply comment addressing every QA point (label untouched, no new PR). diff --git a/work/sessions/2026-08-12-developer-issue-282.md b/work/sessions/2026-08-12-developer-issue-282.md deleted file mode 100644 index e88e5b4a..00000000 --- a/work/sessions/2026-08-12-developer-issue-282.md +++ /dev/null @@ -1,4 +0,0 @@ - - - -Now add the sweep.rs unit tests footer check — there were no tests in this file originally (it had none per earlier listing at 328 lines, all handler code, no `#[cfg(test)]`). Good. Now update `handlers/audio/mod.rs`, `handlers/mod.rs`, `workers.rs`, `server.rs`. diff --git a/work/sessions/2026-08-12-developer-pr-296-rev.md b/work/sessions/2026-08-12-developer-pr-296-rev.md deleted file mode 100644 index aaac7ae2..00000000 --- a/work/sessions/2026-08-12-developer-pr-296-rev.md +++ /dev/null @@ -1,17 +0,0 @@ - - - - -Done. Caveman summary: - -Fixed PR #296, branch `issue-282`, pushed commit `00861a9`. No new PR, no label touch. - -**Fix:** `check_tail_decay` window was flat `tail_len/4` — shorter than lowest band's settle time at shipped `tail_s=0.5`, so that band always `NEG_INFINITY`'d and got silently dropped. Now window sizes off widest settle requirement (`Filterbank::settle_samples()`, new accessor), capped at half-tail. Bands still too short to settle get counted (`bands_settled`/`bands_total`) and surfaced in `note()` instead of vanishing. - -Also killed dead disjunct (issue 2), added regression test at shipped default + coverage-assertion test that actually discriminates old vs fixed code. - -`cargo test --workspace` all green, clippy clean, fmt clean. - -Posted reply comment on PR addressing each QA point — including one open disagreement I flagged rather than hid: didn't tie `passed` to full band coverage (kept it scoped to bands actually evaluated + separate visibility for gaps), since that felt like a behavior-semantics call beyond bugfix scope. Said so explicitly, asked reviewer's call. - -Also noted honestly: QA's suggested regression test (ported near-verbatim) doesn't actually discriminate the bug by my analysis — kept it for real gap coverage anyway, but the coverage-assertion test is the one that actually proves the fix. diff --git a/work/sessions/2026-08-12-qa-1954223.md b/work/sessions/2026-08-12-qa-1954223.md deleted file mode 100644 index f26951bb..00000000 --- a/work/sessions/2026-08-12-qa-1954223.md +++ /dev/null @@ -1,4 +0,0 @@ - - - -Standing by for the background test-run notification before finishing the review. diff --git a/work/sessions/2026-08-12-qa-pr-294.md b/work/sessions/2026-08-12-qa-pr-294.md deleted file mode 100644 index 1ee80b4f..00000000 --- a/work/sessions/2026-08-12-qa-pr-294.md +++ /dev/null @@ -1,13 +0,0 @@ - - - -PR #294 reviewed, approved as comment (own-PR block on formal review), label set `in-review`. - -Gate at `f15f8d4`: `cargo test --workspace` 0 failed, clippy 0 warnings, fmt clean — all verified directly, not from PR body claims. - -Round-1's three findings all fixed and independently mutation-verified by me: -1. ZMQ.md cancel/timeout mismatch — step-2 stop check added, symmetric with step-1. Killed by reverting the check. -2. `absent`'s second origin (never-calibrated leg) — new test catches `Skip => "unchanged"` mutation. -3. No keystroke→intent test — `classify_entry` extracted, six unit tests green, wire-encoding negative included. - -Spec coverage (#279, five criteria): all ✓. Standards: unchanged from round 1, §4.2 ref_dbfs guard untouched this round, n/a elsewhere. Scope: exactly the files dev claimed, nothing stray. diff --git a/work/sessions/2026-08-12-qa-pr-296-delta.md b/work/sessions/2026-08-12-qa-pr-296-delta.md deleted file mode 100644 index 54c1921f..00000000 --- a/work/sessions/2026-08-12-qa-pr-296-delta.md +++ /dev/null @@ -1,17 +0,0 @@ - - - - -Done. - -**Re-review of PR #296, commit range `c8ab276..00861a9`:** - -- Gate re-run at new tip, full workspace: `cargo test --workspace` all green, `clippy -- -D warnings` clean, `fmt --check` clean. -- Delta = `sweep.rs` + `filterbank.rs` only. Both blocking issues from first pass fixed, verified independently (recomputed settle-sample math by hand, not just trusted PR body — numbers match: lowest band settle ≈6590 samples, old window 6000 too short, new window 8237 clears it). -- Dead disjunct removed, confirmed. -- Both test-coverage gaps addressed; one new test is genuinely discriminative, other is honestly self-flagged as weak by dev (not misleading). -- Traced the fix through to actual output (`ac-daemon` → `report.notes`) — coverage caveat text really reaches the consumer, not just the struct. -- Open design question dev left unresolved (should incomplete coverage fail `passed`?) — judged acceptable, not blocking, reasoning sound either way. -- No new correctness issues, no scope creep. - -**Verdict: approve.** Posted as PR comment (GH blocked formal review-approve since I share the account with the PR author) at https://github.com/mkovero/ac/pull/296#issuecomment-5270097158. Label swapped `needs-work` → `in-review`. diff --git a/work/sessions/2026-08-12-qa-pr-296.md b/work/sessions/2026-08-12-qa-pr-296.md deleted file mode 100644 index daf9cab2..00000000 --- a/work/sessions/2026-08-12-qa-pr-296.md +++ /dev/null @@ -1,15 +0,0 @@ - - - - -Review posted, label set `needs-work`. - -Gate: `cargo test --workspace` clean, clippy clean, fmt clean — but that not enough. Found real bug in core deliverable. - -**Verdict: request-changes.** - -Main issue: `check_tail_decay` (the ISO 18233 §6.3.2 post-hoc check, acceptance criterion 6, whole point of this feature) silently drops lowest 1/3-oct band from its worst-case comparison whenever that band's filter hasn't settled in the early window — mislabeled in code comment as "no energy" when it's actually "window too short to measure." Verified empirically (built temp instrumented test, ran against `p_default()` at daemon's real shipped default `tail_s=0.5s`): 25.1 Hz band always excluded at default. Means a room whose low end genuinely fails 30dB decay reads PASS — check never evaluates band most likely to violate the standard. - -Gave concrete fix shapes + regression test in review comment (not applied — QA only suggests tests, doesn't patch). - -Everything else (rename, alias forwarding, parser tests, wire docs, scope) clean — no other blockers. diff --git a/work/sessions/2026-08-12-ux-281.md b/work/sessions/2026-08-12-ux-281.md deleted file mode 100644 index de9a831f..00000000 --- a/work/sessions/2026-08-12-ux-281.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -You've hit your monthly spend limit · raise it at claude.ai/settings/usage?from=cc_cli_limit_message diff --git a/work/sessions/2026-08-12-ux-282.md b/work/sessions/2026-08-12-ux-282.md deleted file mode 100644 index cca9101c..00000000 --- a/work/sessions/2026-08-12-ux-282.md +++ /dev/null @@ -1,6 +0,0 @@ - - - -Posted design comment on #282. - -Core finding: `ac sweep ir` today prints zero measurement result — `wait_for_stop` only watches `done`/`error`, drops `measurement/impulse_response` and `measurement/report` on the floor (`commands/generate.rs:200-211`, `handlers/audio/sweep.rs:268-317`). Design covers: peak IR (unity-referenced), ISO 18233 §6.3.2 tail-decay margin as its own line (the quiet number criterion 6 exists for), gate-param echo (`n_harmonics`/`window_len`/`tail_s`), calibration-line parity with sibling `plot`/`plot level`, and flags that nothing currently writes the `MeasurementReport` JSON `ac report` needs to render html/pdf. diff --git a/work/sessions/2026-08-13-architect-issue-280.md b/work/sessions/2026-08-13-architect-issue-280.md deleted file mode 100644 index c557384a..00000000 --- a/work/sessions/2026-08-13-architect-issue-280.md +++ /dev/null @@ -1,11 +0,0 @@ - - - - -Done. - -- Posted design comment on #280: recommends `MeasurementPayload` wrapper (`data: Vec`), citation moved off `MeasurementMethod` onto each payload (`Vec`), new `GateParams` per gated payload, `PositionSnapshot` at report level (one geometry per capture). Rejected bare `Vec` — doesn't fix the `plot.rs:430` citation-slot bug and leaves `GateParams` with no clean home. -- ZMQ impact: nested `report.data` shape changes object→array; checked both consumers (`ac-cli` doesn't index into it live, `ac-view` doesn't consume the frame at all) — archival-only, not a live break. -- Labels: removed `needs-design`, added `ready-to-implement` + `needs-ux` (report_html/report_pdf gain new rendered surface: multi-payload, gate block, f_low_hz, citations). - -Note: had to route around a sandbox restriction on file writes/heredocs — posted the comment as a single `gh issue comment --body` argument instead. diff --git a/work/sessions/2026-08-13-developer-issue-193.md b/work/sessions/2026-08-13-developer-issue-193.md deleted file mode 100644 index 5832a2da..00000000 --- a/work/sessions/2026-08-13-developer-issue-193.md +++ /dev/null @@ -1,11 +0,0 @@ - - - - -PR #301 open. https://github.com/mkovero/ac/pull/301 — closes #193. - -Summary: -- `app.rs` gains `frame_parse_failures` streak + `first_malformed_since` clock, `MALFORMED_GRACE` (10s) constant, `ingest_raw_frame`/`malformed_active`/`status_for_state` methods. -- Status line: `live` → `malformed — host:port — N consecutive frames dropped, not rendering` once streak clears grace window; resets on good parse; single glitch doesn't flicker. -- Test seam fixed per UX's flagged gap: raw-JSON entry (`ingest_raw_for_test`) drives actual `serde_json::from_value` boundary, not the pre-parsed `ingest_frame_for_test` hook. -- 5 new tests, all pass. `cargo test --workspace` clean except pre-existing unrelated `it_live_end_to_end` env failure (hardcoded path ignores this worktree's `CARGO_TARGET_DIR`, reproduces on `main` too). clippy + fmt clean. diff --git a/work/sessions/2026-08-13-developer-issue-226.md b/work/sessions/2026-08-13-developer-issue-226.md deleted file mode 100644 index 58b42238..00000000 --- a/work/sessions/2026-08-13-developer-issue-226.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Now build & run these tests. diff --git a/work/sessions/2026-08-13-developer-issue-280.md b/work/sessions/2026-08-13-developer-issue-280.md deleted file mode 100644 index 1ef6f8a9..00000000 --- a/work/sessions/2026-08-13-developer-issue-280.md +++ /dev/null @@ -1,12 +0,0 @@ - - - - -PR #299 open, closes #280. Summary: - -- `report.rs`: `data` → `Vec`, `SCHEMA_VERSION` 4, new `GateParams`/`PositionSnapshot`, legacy v1/v2/v3 decode shim, `to_csv` per-payload blocks. `MeasurementMethod` loses `standard` (fixes the `plot.rs:430` bug). -- `report_html.rs`/`report_pdf.rs`: render each payload's own citation(s)+gate, new Environment & Geometry section. -- `plot.rs` (daemon): builds payloads instead of bare data, flows `cfg.temperature_c` into report. -- `it_protocol.rs`: two tests updated for new shape/version. - -Tests: full green except one pre-existing infra failure (verified via stash — not caused by this change). Clippy clean, fmt clean. diff --git a/work/sessions/2026-08-13-developer-issue-281.md b/work/sessions/2026-08-13-developer-issue-281.md deleted file mode 100644 index e04a4079..00000000 --- a/work/sessions/2026-08-13-developer-issue-281.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Clean build. Now let's run ac-daemon's existing calibrate-related tests to check for breakage. diff --git a/work/sessions/2026-08-13-developer-pr-298-rev.md b/work/sessions/2026-08-13-developer-pr-298-rev.md deleted file mode 100644 index 3f7bebda..00000000 --- a/work/sessions/2026-08-13-developer-pr-298-rev.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Done. All three blocking gaps got tests, pushed to `issue-281` (`5fd6f09`), replied to review comment #298 point by point — one disagreement flagged (`print_tau_leg`, left untouched, pre-existing thin spot not this PR's scope). Gate clean: 946 tests pass, clippy clean, fmt clean. Label untouched, no new PR. diff --git a/work/sessions/2026-08-13-developer-pr-299-rev.md b/work/sessions/2026-08-13-developer-pr-299-rev.md deleted file mode 100644 index 54566601..00000000 --- a/work/sessions/2026-08-13-developer-pr-299-rev.md +++ /dev/null @@ -1,12 +0,0 @@ - - - - -Done. Commit `6745f36` pushed to `issue-280`, reply posted: https://github.com/mkovero/ac/pull/299#issuecomment-5278846784 - -Summary: -- Blocking (no "number of averages" field) → fixed: `IntegrationParams::n_averages: Option`, all 9 call sites updated, rendered in HTML/PDF Method section when present, tested. -- Section-order nit → disagreed, said why, offered to fold in if wanted. -- stddocs clause-verify gap → can't close it here either (gitignored, absent in checkout), flagged not silently dropped. - -Tests/clippy/fmt all clean, same pre-existing unrelated `it_live_end_to_end` failure as before. Label/PR untouched per instructions. diff --git a/work/sessions/2026-08-13-developer-pr-301-rev.md b/work/sessions/2026-08-13-developer-pr-301-rev.md deleted file mode 100644 index 3bb3042d..00000000 --- a/work/sessions/2026-08-13-developer-pr-301-rev.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -You've hit your monthly spend limit · raise it at claude.ai/settings/usage?from=cc_cli_limit_message diff --git a/work/sessions/2026-08-13-qa-pr-298-delta.md b/work/sessions/2026-08-13-qa-pr-298-delta.md deleted file mode 100644 index 090017d9..00000000 --- a/work/sessions/2026-08-13-qa-pr-298-delta.md +++ /dev/null @@ -1,17 +0,0 @@ - - - - -Done. Summary: - -**PR #298 re-review posted** (commit range `596f4cd..5fd6f09`). - -- Full gate re-run at tip, workspace-wide: `cargo test --workspace` clean (942 tests, 0 failed), `cargo clippy --workspace --all-targets -D warnings` clean, `cargo fmt --check` clean. -- All 3 correctness issues from prior request-changes review closed by test, not just reading: - 1. `measure_tau` end-to-end → new `calibrate_measures_tau_against_fake_loopback_delay` test, ran/passed. - 2. Cheap-refresh keeps measuring τ → new `calibrate_cheap_refresh_still_measures_tau` test, ran/passed. - 3. No-loopback path untestable inline → extracted pure `tau_result()` fn + 3 unit tests, ran/passed. -- No dropped points, no scope creep, standards check n/a (unchanged from first pass — no measurement/display value touched). -- Verdict: **approve**. Label stays `in-review`, no change needed. - -Note: hit env quirk — `CARGO_TARGET_DIR` set to shared cache dir broke `ac-view`'s hardcoded binary lookup. Unrelated to PR diff, worked around by unsetting for the gate run. diff --git a/work/sessions/2026-08-13-qa-pr-298.md b/work/sessions/2026-08-13-qa-pr-298.md deleted file mode 100644 index c45ee0c5..00000000 --- a/work/sessions/2026-08-13-qa-pr-298.md +++ /dev/null @@ -1,9 +0,0 @@ - - - - -Done. Review posted, `needs-work` label applied. - -**Verdict: request-changes.** Logic checks out — hand-traced the τ offset arithmetic in `measure_tau` against `extract_irs`'s gating convention (`peak_idx - TAU_WINDOW_LEN/2` correctly reduces to the true delay `d` in samples), confirmed the one-shot/continuous-tone playback path is mutually exclusive so no contamination, and confirmed the cheap-refresh path works because `is_loopback` is independent of prompt replies. Full gate (`cargo test --workspace`, clippy `-D warnings`, `fmt --check`) reran clean at the PR's tip commit (596f4cd). - -Blocking issue: three acceptance criteria — τ measured correctly end-to-end, explicit no-loopback non-measurement, cheap-refresh — have **zero test coverage**. `measure_tau` itself is never invoked by any test; the `calibration.rs` tests only exercise synthetic `TauEntry` structs directly. The fake audio backend already gives a deterministic 32-sample loopback delay and `it_protocol.rs` has the exact template for this (`plot_ir_emits_impulse_response_with_expected_delay_peak`) — dev didn't write the analogous test for `calibrate`. Included a suggested test in the review comment. diff --git a/work/sessions/2026-08-13-qa-pr-299-delta.md b/work/sessions/2026-08-13-qa-pr-299-delta.md deleted file mode 100644 index fe2d3af6..00000000 --- a/work/sessions/2026-08-13-qa-pr-299-delta.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -Posted. Approve stands, `in-review` label unchanged (already set, no removal needed). - -Summary: re-reviewed `adde33c..6745f36`. Prior blocker (no field for ISO 18233 §9 "number of averages") fixed — `IntegrationParams.n_averages: Option`, wired thru all 4 call sites, tested tight (exact-string round-trip + omission), rendered in both HTML and PDF. Gate clean: fmt clean, clippy zero warnings, workspace test green except one confirmed pre-existing/environmental `ac-view` failure (target-dir redirect artifact of this worktree, unrelated to delta — verified by hand). One minor non-blocking gap noted: PDF render has no present-case test for the averages row (HTML does). stddocs/ still absent from checkout so ISO clause still unverified against primary text — carried forward as open note, not a blocker. Verdict: approve. diff --git a/work/sessions/2026-08-13-qa-pr-299.md b/work/sessions/2026-08-13-qa-pr-299.md deleted file mode 100644 index ead40bc1..00000000 --- a/work/sessions/2026-08-13-qa-pr-299.md +++ /dev/null @@ -1,15 +0,0 @@ - - - - -Posted independent QA re-review on PR #299: https://github.com/mkovero/ac/pull/299#issuecomment-5279157078 - -Verdict: **approve**. Label stays `in-review` (tip unmatched to no new push — `6745f36` reviewed = current `headRefOid`). - -Key findings: -- **stddocs/ now readable** (earlier two QA passes on this PR both flagged it missing). Checked ISO 18233 §9 and ISO 3382-1/2 §9.2 against primary PDF text — issue's paraphrase matches word-for-word, including exact "signal type, signal duration, number of averages" item list that drove the earlier blocking finding. Verification gap closed. -- One minor nuance found not in prior reviews: ISO 3382-2 §9.2(f) makes temp/RH conditional on precision method (3382-1 doesn't). Doesn't affect PR — fields optional regardless — noted, not blocking. -- Re-ran `cargo fmt --check` / `clippy -D warnings` / `test --workspace` myself: clean except same pre-existing `ac-view::it_live_end_to_end` env failure (binary-path lookup vs redirected target dir) both prior passes already identified. -- Spot-checked every cross-crate consumer by hand (`ac-view` doesn't touch `measurement/report`, `ac-cli::plot.rs` reads only `notes`, `ac-cli::report.rs` deserializes generically) — no stragglers. -- Found one incidental non-bug: `draw_method` in `report_pdf.rs` switched label font from regular→bold as a side effect of the `standard` field removal — makes Method section consistent with rest of doc (which already used bold labels). Not a defect. -- Repeat, still-open, still non-blocking gap: `report_pdf.rs` has no present-case test for the new `averages` row (HTML got one, PDF didn't). Suggested test included in comment. diff --git a/work/sessions/2026-08-13-qa-pr-300.md b/work/sessions/2026-08-13-qa-pr-300.md deleted file mode 100644 index 234f6ad6..00000000 --- a/work/sessions/2026-08-13-qa-pr-300.md +++ /dev/null @@ -1,16 +0,0 @@ - - - - -Reviewed PR #300, tip `1d481e5`. Verdict: **approve**, `in-review` label applied. - -Summary: -- Spec: pulled architect's option-A decision comment on #226 as acceptance criteria (13 items) — all covered, no gaps. -- Traced tick-order logic in `transfer.rs` by hand: manual relock → drive-edge → estimate block, no interleave. `driving` provenance captured at accept-time, not re-derived. `flush_pair` correctly spares `pair_delay_attempts`/`pair_prominence`/`mtw_failed`. Edge-seed uses `drive` (legacy param), not hardcoded `false` — matches spec's named trap. -- `it_relock.rs`: all 7 design-doc scenarios present, tight numeric asserts (`delay_samples == LOCK_DELAY_SAMPLES`, strict `attempts` monotonicity), not weak panic-only checks. -- Ran gate myself (not trusted from PR body): `cargo test --workspace` all green (had to symlink `ac-rs/target` → cache target dir first, pre-existing env quirk, unrelated to PR), `clippy -D warnings` clean, `fmt --check` clean. -- Standards: not applicable — no measurement/value/unit code touched. -- Drive-path safety: PR only reads `engine_on`/`drive_state`, doesn't touch arm/panic/clamp/dead-man mechanics; checklist items still green, and PR's own test 5 stresses the dead-man interaction directly. -- Scope: files touched match architect's module list exactly, no drift. - -Note: couldn't post as GitHub "review" — API refuses self-approval since PR author = my authenticated account. Posted as regular PR comment instead, same `` body. diff --git a/work/sessions/2026-08-13-qa-pr-301.md b/work/sessions/2026-08-13-qa-pr-301.md deleted file mode 100644 index 896fad25..00000000 --- a/work/sessions/2026-08-13-qa-pr-301.md +++ /dev/null @@ -1,13 +0,0 @@ - - - - -Posted review, labeled `needs-work`. - -Verdict: **request-changes**. - -Spec coverage: all 5 AC met (counter, malformed status w/ count, plain text no-colour, headless test thru real ingest boundary, happy-path untouched). Gate clean: clippy/fmt clean, `cargo test --workspace` green except `it_live_end_to_end` — confirmed pre-existing env path bug (hardcoded `target/debug`, ignores `CARGO_TARGET_DIR`), reproduced independently, file untouched by diff. - -Bug found: `frame_parse_failures`/`first_malformed_since` (app.rs:143-169) reset only on good parse, never on real `Disconnected` transition. Session can go malformed → truly drop >10s → reconnect still bad, and streak/timer carry over stale. First post-reconnect bad frame skips anti-flicker grace and reports inflated "N consecutive" spanning a real gap — contradicts UX's own "uninterrupted" definition of the count. Gave fix location + suggested test in review comment. - -Standards check: n/a — connection-status text, no measurement value. Display-truth/daemon-soak/drive-path gates: n/a, none of those code paths touched. diff --git a/work/sessions/2026-08-13-ux-193.md b/work/sessions/2026-08-13-ux-193.md deleted file mode 100644 index 5239996e..00000000 --- a/work/sessions/2026-08-13-ux-193.md +++ /dev/null @@ -1,18 +0,0 @@ - - - - -Done. Design comment posted on #193, labels: `needs-ux` off, `ux-approved` on, `ready-to-implement` kept. - -Summary — status line grows fourth state `malformed` (dark-orange register, plain-text carries claim without colour), sits between `live` and `disconnected`: - -``` -no session -live — 192.168.9.25:5556 -malformed — 192.168.9.25:5556 — 212 consecutive frames dropped, not rendering -disconnected — 192.168.9.25:5556 not responding -``` - -Key calls: count is consecutive-since-last-good (not lifetime — lifetime mixed w/ earlier healthy period reads false), gated by staleness (grace window, reuse existing 10s `DISCONNECT_AFTER` by analogy) so single blip doesn't flicker status, recovery instant (no grace on way back up). Word `malformed` replaces `live` entirely in that state — satisfies acceptance criterion literally ("`live` must not be shown while every frame dropped"). - -Flagged 4 open items, none blocking: counter naming (must not conflate w/ Session's existing wire-transport-level `malformed_frames`), grace constant reuse (dev call), test seam (`ingest_frame_for_test` bypasses exact parse boundary issue is about — headless test needs raw-JSON entry point), and #205 cross-ref (vocabulary-sharing note for whoever picks that one up, not this issue's scope). diff --git a/work/sessions/2026-08-13-ux-280.md b/work/sessions/2026-08-13-ux-280.md deleted file mode 100644 index e61309d9..00000000 --- a/work/sessions/2026-08-13-ux-280.md +++ /dev/null @@ -1,12 +0,0 @@ - - - - -Done. #280: posted design comment (comment-5278142692), removed `needs-ux`. - -Summary of what got said: -- **Kill** the report-level `standard` line under Method — with citation moving to payload (architect's Option A), one line can't hold up to 3 citations across IR/gated-FR/bands. -- **Add** per-payload citation lines, a gate block (`start → end (length, window)` in ms + `f_low = 1/gate length` shown inline, not left for reader to compute), and new `Environment & Geometry` section (temp, RH, source/receiver height+distance, derived speed of sound) satisfying ISO 3382 §9.2's knowable subset. -- Showed full literal HTML-rendered mockup with real Farina/ISO18233/ISO3382/IEC61260-1 citation strings pulled from actual `citation()` fns in the repo, plus worst-case column-width check. -- Recommended `to_csv` shape: concatenated per-payload blocks with `#` comment header, grep/pipe-friendly, one artifact per format — flagged as non-blocking, dev's call per architect note. -- Two open (non-blocking) notes: whether derived "speed of sound" earns its line, and that `report_pdf.rs` wasn't inspected this pass — needs same check if it renders independently of the HTML path. diff --git a/work/sessions/2026-08-13-ux-281.md b/work/sessions/2026-08-13-ux-281.md deleted file mode 100644 index 27b68f12..00000000 --- a/work/sessions/2026-08-13-ux-281.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -Done. `needs-ux` gone, `ready-to-implement` stays (already there, still correct). - -Comment posted: adds `Delay:` third leg to `cal_done` output, two states (measured w/ conditions, not-measured), both rendered with real numbers, width-checked. Two open questions flagged, non-blocking: `ac calibrate show` has no τ-history rendering, and stretch outlier-flag wording undefined if that goal gets picked up. diff --git a/work/sessions/2026-08-14-developer-pr-301-rev.md b/work/sessions/2026-08-14-developer-pr-301-rev.md deleted file mode 100644 index 54c8b529..00000000 --- a/work/sessions/2026-08-14-developer-pr-301-rev.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Done. Fixed QA's one correctness finding (streak/grace window not reset on real `Disconnected`), added their suggested regression test, all gates green, comment posted on PR #301. No disagreement to flag — QA's point was correct and the fix matches their suggestion exactly. Left label as-is per instructions. diff --git a/work/sessions/2026-08-14-qa-pr-301-delta.md b/work/sessions/2026-08-14-qa-pr-301-delta.md deleted file mode 100644 index 071e1e41..00000000 --- a/work/sessions/2026-08-14-qa-pr-301-delta.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -Comment posted, label already `in-review`, no change. - -Summary: reviewed `949d504..decca66`. Both prior findings (streak-survives-disconnect bug + missing test) fixed — verified fix reached via unconditional per-frame call site, verified new test matches suggested case. Full `cargo test --workspace` green except same pre-existing `it_live_end_to_end` env failure (unrelated file, confirmed by diff stat). Clippy/fmt clean. Standards: not applicable, this is a status-string bugfix, no measurement value touched. Verdict: **approve**. diff --git a/work/sessions/2026-08-15-architect-issue-284.md b/work/sessions/2026-08-15-architect-issue-284.md deleted file mode 100644 index 2175e667..00000000 --- a/work/sessions/2026-08-15-architect-issue-284.md +++ /dev/null @@ -1,17 +0,0 @@ - - - - -Design comment posted, labels swapped (`needs-design` off, `ready-to-implement` + `needs-ux` on). - -Core decision, short version: - -**New `MeasurementData::GatedFrequencyResponse` variant** (magnitude+phase points), not reuse of `FrequencyResponse` — that struct's THD/clipping fields would sit at false 0 for a producer that never measures them. Fifth variant, same pattern as `SpectrumBands`/`NoiseResult`. - -Pinned four sub-decisions so implementer has zero open questions: -- **Window:** Tukey α=0.25 (new fn, `gate()` in sweep.rs stays rectangular — that's for harmonic isolation, don't touch it). -- **`gate_start_s`:** anchored at the linear-IR peak (arrival), not buffer origin — only stable zero-reference across window-length changes. -- **Noise-tail boundary:** derivable as `duration_s` past the peak (inverse-filter kernel length), capped by captured tail. One shared fn in `sweep.rs`, used by both #283's print path and this issue's report render. Marked as text in the existing `ImpulseResponse` metadata block, not a waveform shade (no IR plot exists in the static renderer — that's #286). -- **Distortion-vs-frequency:** explicit deferral to a follow-up — it's a genuinely separate derivation (needs the sweep's time↔frequency map applied per harmonic IR), not shared code with the gate+FFT path here. - -No ZMQ schema/version bump needed — additive fields only, same trick `NoiseResult.ccir_weighted_dbfs` used. Flagged `needs-ux` too: #280's earlier UX mockup covered magnitude display but not phase or the noise-tail line, so that's new reader-facing surface. diff --git a/work/sessions/2026-08-15-developer-issue-277.md b/work/sessions/2026-08-15-developer-issue-277.md deleted file mode 100644 index 2dd030f8..00000000 --- a/work/sessions/2026-08-15-developer-issue-277.md +++ /dev/null @@ -1,17 +0,0 @@ - - - - -Blocked — need explicit consent before touching hardware. - -**Two things collide:** - -1. **Which rig, which path.** Issue wants Babyface Pro at 96 kHz, not dummy driver. That's `192.168.9.25` (radv dev box) — its PipeWire is locked to `clock.rate 96000` anyway, so sample rate is free. `.40` rig has FF400, not Babyface — wrong box for this issue. Real converter path there needs either digital loopback (`monitor_N → in`) or analog patch (out→in), both count as "signal leaves the interface" under the consent rule. - -2. **Level conflict.** Runbook's `it_loopback_ir.rs` hardcodes `level_dbfs: -6.0` (line 220). Hard consent rule caps real-hardware stimulus at **-40 dBFS** (electrical) unless per-session exception granted. -6 dBFS blows through that ~34 dB. Can't run the test binary verbatim on real hardware as-is — need either your explicit one-off exception for -6 dBFS, or I drive the same sweep manually via ZMQ (`plot_ir` REQ) with `level_dbfs` clamped to -40, bypassing the test's hardcoded value (test itself unedited, scope stays clean). - -**Asking before any signal goes out:** -- Consent to emit on 192.168.9.25 this run — which leg: digital loopback (bit-reproducible, no DAC/ADC) or analog out→in (has real converter in the path, closer to what issue's "not the dummy driver" concern is about)? -- Level: run manual `plot_ir` at -40 dBFS instead of test's -6, or grant exception for -6 this run? - -No hardware touched yet. Say go + which options and I run it, record peak idx/mag/SNR + τ candidate to `work/rig/`, open PR with just that file (issue is data-only, no code fix in scope). diff --git a/work/sessions/2026-08-15-developer-issue-284.md b/work/sessions/2026-08-15-developer-issue-284.md deleted file mode 100644 index dfacc6c8..00000000 --- a/work/sessions/2026-08-15-developer-issue-284.md +++ /dev/null @@ -1,14 +0,0 @@ - - - - -PR #305 opened, closes #284. Branch `issue-284` pushed, all gates green (workspace tests, clippy `-D warnings`, `fmt --check` on touched files) except one pre-existing environment-only `ac-view` test unrelated to this diff. - -Summary of what shipped: -- `sweep.rs`: `tukey_window`, `gated_frequency_response`/`GatedResponsePoint`, `noise_tail_start_s`, `gated_response_citation` — plus hand-derived synthetic tests (flat system, single-pole closed-form DTFT, gate-length→f_low arithmetic, gated-vs-ungated reflection mutation test). -- `report.rs`: new `MeasurementData::GatedFrequencyResponse` variant (option A from the architect review) and `ImpulseResponse.noise_tail_start_s: Option` (additive, no schema bump). -- `report_html.rs`/`report_pdf.rs`: render the gated payload (HTML: two-panel SVG + table; PDF: magnitude plot + 3-column table, per the UX page-budget call) and the new noise-tail line. -- `ac-daemon/handlers/audio/plot.rs`: `plot_ir` now emits the gated-response payload alongside the impulse response, citing both Farina/ISO 18233 and a new AES17-2015 Annex A.4 citation. -- `ZMQ.md` updated for the new `data.kind` and field. - -Distortion-vs-frequency stayed explicitly deferred per the architect's decision #4 — no follow-up issue opened since nothing blocks it landing independently later. diff --git a/work/sessions/2026-08-15-developer-pr-305-rev.md b/work/sessions/2026-08-15-developer-pr-305-rev.md deleted file mode 100644 index 87b84ca8..00000000 --- a/work/sessions/2026-08-15-developer-pr-305-rev.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Done. Pushed `14ba059` (ZMQ.md fix), commented on PR. No label touch, no new PR, per instructions. diff --git a/work/sessions/2026-08-15-qa-pr-304.md b/work/sessions/2026-08-15-qa-pr-304.md deleted file mode 100644 index d55f2c33..00000000 --- a/work/sessions/2026-08-15-qa-pr-304.md +++ /dev/null @@ -1,20 +0,0 @@ - - - - -PR #304 reviewed. Summary: - -**Verdict: approve** (`in-review` label applied) - -All six acceptance criteria from #283 met — CLI prints and persists arrival/distance/peak/SNR/gate off the report frame (not raw IR frame, so archive and printout can't drift). Checked numerically, not just read: - -- `ir_stats` peak-centre calc matches `extract_irs`'s gate placement exactly, survives #278's per-order clamp because both sides read actual array length. -- τ-subtraction formula `distance_m = (arrival_s - tau_s) * c` cross-checked against rig's own known converter constant (`arrival(d) = τ + d/c`) — matches independently, not just self-consistent with PR's own test. -- No-τ fallthrough correctly refused, never derives distance from uncorrected arrival — tested at unit and integration level. -- ISO 18233 §B.5 citation verified against primary PDF (`/home/mui/src/ac/stddocs/iso-full/ISO18233.pdf`, Annex B confirmed normative) — wording is an accurate paraphrase, matches "so as not to confuse decreasing noise floor with reverberant tail" almost verbatim. - -Gate: `cargo fmt --check` clean, `cargo clippy --workspace --all-targets -- -D warnings` clean, `cargo test --workspace` clean except one `ac-view` integration test — traced that to a pre-existing hardcoded target-dir path in the test harness itself (unrelated file, this PR touches none of `ac-view`), not a regression from this diff. - -One non-blocking note: `AC_CTRL_PORT`/`AC_DATA_PORT` addition is outside issue's stated file list but disclosed + justified in PR body (test isolation). Flagged in scope section, not blocking. - -Also flagged (not against this PR): `sweep::citation()`'s existing `verified: false` on its ISO 18233 Annex B cite predates this PR (#291) — untouched here, out of scope to fix under this review. diff --git a/work/sessions/2026-08-15-qa-pr-305-delta.md b/work/sessions/2026-08-15-qa-pr-305-delta.md deleted file mode 100644 index 91a13d8f..00000000 --- a/work/sessions/2026-08-15-qa-pr-305-delta.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -Posted. Approve, label unchanged (already `in-review`). - -Summary: reviewed e44828f..14ba059, doc-only 4-line ZMQ.md diff. Closes prior re-review's stale-citation-doc point (verified against source: `farina_citation()` def + call site match new wording). Full workspace gate (fmt, clippy, test) clean at tip 14ba059. No new issues. diff --git a/work/sessions/2026-08-15-qa-pr-305.md b/work/sessions/2026-08-15-qa-pr-305.md deleted file mode 100644 index 114ea903..00000000 --- a/work/sessions/2026-08-15-qa-pr-305.md +++ /dev/null @@ -1,17 +0,0 @@ - - - - -Reviewed PR #305, posted, labeled `needs-work`. - -Verdict: **request-changes**. - -Summary (caveman): - -- All 7 AC met functionally. Gate math (Tukey window, FFT, f_low) checked by hand — correct. Tests (flat system, single-pole, reflection mutation-verify) numerically sound. -- Build/test/clippy/fmt all green on my own run (workspace) — only failure `ac-view::it_live_end_to_end`, confirmed pre-existing/environment-only (binary exists at redirected `CARGO_TARGET_DIR`, test looks at hardcoded relative path; PR touches no ac-view files). -- **Real bug**: new `GatedFrequencyResponse` payload cites `sweep_citation()` + `gated_response_citation()`. `sweep_citation()` bundles Farina *and* ISO 18233:2006 Annex B into one string. That means gated payload cites ISO 18233 anyway — contradicts architect's own written decision on #284 ("not ISO 18233... that only attaches when classical room standard also applies"). Verified against actual ISO18233 PDF (`stddocs/iso-full/ISO18233.pdf`): §1 scope limits it to substituting ISO 140/3382/17497-1 (room/building acoustics); §9(c) requires naming the paired classical standard whenever cited. Quasi-anechoic loudspeaker capture has none — so this citation is non-conformant, and shows up verbatim in rendered HTML/PDF reports. No test catches it. -- Minor: `gated_response_citation()` doc comment claims stddocs doesn't carry AES17-2015 PDF — it does (5-page preview stub only, so still can't verify Annex A.4 content, but comment's wording is inaccurate). -- AES17-2015 Annex A.4 citation itself: can't verify against primary text, only preview pages available in stddocs (title+TOC, no annex content). Flagged as gap, not carried forward silently. - -Full structured review posted to PR: https://github.com/mkovero/ac/pull/305#issuecomment-5303085694 diff --git a/work/sessions/2026-08-15-ux-284.md b/work/sessions/2026-08-15-ux-284.md deleted file mode 100644 index d3553a3e..00000000 --- a/work/sessions/2026-08-15-ux-284.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -Comment posted, `needs-ux` removed (`ready-to-implement` already stood). - -Summary of design: new `GatedFrequencyResponse` payload — magnitude as primary panel (full weight), phase as second thinner stacked SVG panel sharing x-axis (wrapped ±180°, path breaks at discontinuities, not unwrapped — avoids inventing an unwrap algorithm the acceptance criteria's hand-derived tests can't easily check). Gate/`f_low` line needs zero new code — `write_payload`'s existing generic gate block already covers it once `payload.gate` is set. Added a `noise tail begins` line to the existing `ImpulseResponse` meta block, sharing `noise_tail_start_s()` with #283 so the two surfaces can't drift. Flagged PDF as magnitude-plot-only (phase as table column) since it's a fixed single A4 page and already truncates tables — HTML gets full two-panel since it scrolls. diff --git a/work/sessions/2026-08-16-architect-issue-286.md b/work/sessions/2026-08-16-architect-issue-286.md deleted file mode 100644 index 48872eda..00000000 --- a/work/sessions/2026-08-16-architect-issue-286.md +++ /dev/null @@ -1,9 +0,0 @@ - - - - -Posted design comment on #286, labels now `ready-to-implement` + `needs-ux` (needs-design dropped). - -Core finding: real gap bigger than UX's flagged one. `#284`/`#305` shipped gated-IR only inside Tier 1 report pipeline (`MeasurementPayload`/`GateParams`), not in `.acsnap`/`PairDerivation` — `ac-view`'s snapshot loader never touches that format at all. So Frame C (sweep-derived) has no path into `ac-view` today; that's a separate file-format decision, not fillable inline. - -Decision: build Frame A/B now (live sidecar — data already on wire, `TransferResult.re/im` already on `PairDerivation` too, `impulse_response_from_h` reused for both). File Frame C as its own follow-up issue needing its own architect pass. Keybinding `H`, bound `Scope::Transfer` (toggle, not new `ViewId` — views fixed at launch per `keys.rs:263`). Flagged `needs-ux` since display consequence follows from this boundary call. diff --git a/work/sessions/2026-08-16-architect-issue-308.md b/work/sessions/2026-08-16-architect-issue-308.md deleted file mode 100644 index 3f345226..00000000 --- a/work/sessions/2026-08-16-architect-issue-308.md +++ /dev/null @@ -1,18 +0,0 @@ - - - - -Posted. Design decision for #308: - -**Option A wins** — `ac-view` gets a second loader for `MeasurementReport` JSON, not a `.acsnap`-compatible sidecar. Reasons: - -- Report file already exists on disk (`-plot_ir.json`, written by `ac plot ir` already) — zero daemon work. -- All Frame C math already lives in `ac-core`: `MeasurementReport::ir_stats()` computes arrival, gate window, `f_low_hz` already. `ac-view` just orchestrates, same as `.acsnap`'s `snapshot_flow.rs`. -- Sidecar option would smuggle a Farina-deconvolved `linear_ir` through `.acsnap`'s container, whose documented contract (`SNAPSHOT.md`) is *raw pre-DSP capture* — breaks that invariant or forks the extension into two silent meanings. -- No ZMQ/wire change either way. - -Also resolved the carried-forward concurrency question: **no live+sweep overlay** — Frame C stays a dedicated stored-only panel, same boundary #286 already drew between live-arrival IR and gated IR (different kinds, must not visually conflate). Answers #321's single-trace form too. - -Flagged one real risk for developer: `ir_stats()`'s legacy-report fallback is permissive (infers a gate for old reports) but this issue's spec wants a hard fail when gate is genuinely absent — loader must check `payload.gate.is_some()` directly, not route the fail decision through `ir_stats()`. - -Labels: removed `needs-design`, added `ready-to-implement`. No `needs-ux` needed — UX already fully specified success/fault text; my concurrency answer only simplifies geometry (single panel), doesn't add new surface. diff --git a/work/sessions/2026-08-16-developer-issue-285.md b/work/sessions/2026-08-16-developer-issue-285.md deleted file mode 100644 index 83d6f264..00000000 --- a/work/sessions/2026-08-16-developer-issue-285.md +++ /dev/null @@ -1,12 +0,0 @@ - - - - -PR #306 open, closes #285. Branch pushed, tests green (workspace, 0 failed), clippy clean, fmt clean. - -Summary: -- `plot_ir` now applies mic-curve correction to the **gated derived spectrum** only (frequency-domain, reused `apply_mic_curve_inplace_f64` route) — IR itself, arrival, gate untouched. -- `mic_correction_applied` reports truth instead of hardcoded `false`. -- New `mic::apply_mic_curve_to_gated_response` + load-bearing test `mic_curve_correction_does_not_move_ir_peak`: builds synthetic IR w/ known peak, proves peak stays put under real impl, and computes what `MicCurveFir` time-domain convolution (rejected impl) would've done — group-delay shift — to show test catches that bug. -- SPL: left as-is, riding on existing `calibration.mic_sensitivity_dbfs_at_94db_spl` field already in the report (matches codebase-wide precedent — SPL never baked per-bin into any spectrum array anywhere, incl. `plot`'s own). Flagged as judgment call in PR open questions. -- 2 files touched, no schema change, no new deps. diff --git a/work/sessions/2026-08-16-developer-issue-286.md b/work/sessions/2026-08-16-developer-issue-286.md deleted file mode 100644 index e8935551..00000000 --- a/work/sessions/2026-08-16-developer-issue-286.md +++ /dev/null @@ -1,18 +0,0 @@ - - - - -Done. PR #309 open, closes #286. - -Summary: -- Filed follow-up #308 (Frame C, sweep-derived IR — architect said file before code, none existed) -- `ac-scene`: new `ir.rs` module — `IrInput`/`IrScene`, mirrors `TransferInput`/`TransferScene` split. Both producers (live wire sidecar + `.acsnap` `PairDerivation`) funnel through one path. Reuses `format_delay_readout` verbatim for arrival marker, per design. -- `ac-scene/wire.rs`: new `IrWireFrame` deser type for `visualize/ir` (already-documented frame, zero schema change) -- `ac-scene/ticks.rs`: new `time_to_x`/`time_axis` for linear ms axis -- `ac-view/session.rs`: `poll_frame` now returns `PolledFrame::{Transfer,Ir}` instead of silently discarding the sidecar (fixed the #219 story properly for a second frame type) -- `ac-view/keys.rs`: `H` bound, Scope::Transfer, ledger updated -- `ac-view/view.rs` + `app.rs`: IR panel toggle, replaces mag/phase panes when open, `computes_nothing` stays green (all strings/coords from ac-scene verbatim) - -Tests: workspace green, clippy clean, fmt clean. +32 new tests across ac-scene/ac-view. - -PR: https://github.com/mkovero/ac/pull/309 diff --git a/work/sessions/2026-08-16-developer-issue-287.md b/work/sessions/2026-08-16-developer-issue-287.md deleted file mode 100644 index d7cbc17c..00000000 --- a/work/sessions/2026-08-16-developer-issue-287.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -PR #307 open, closes #287. Done. - -**Summary:** most acceptance criteria already satisfied by #282/#284's own PRs — verified line-by-line, no diff needed. Fixed remaining stale `play_and_capture` claim (3 sites: `ARCHITECTURE.md`, `ZMQ.md`, `plot.rs` doc comment — moved from `sweep.rs` by #282) since both JACK/CPAL backends implement it now. Added §6.2 note in `acoustic-analyze-v1.md` about IR-arrival retiring the phase-slope estimator per #284, without implementing it. `cargo test --workspace` green except one pre-existing worktree-env failure unrelated to this change; clippy clean; fmt clean. diff --git a/work/sessions/2026-08-16-developer-issue-288.md b/work/sessions/2026-08-16-developer-issue-288.md deleted file mode 100644 index d4bd6bfe..00000000 --- a/work/sessions/2026-08-16-developer-issue-288.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Done. No code change — remaining bit is spec-file edit, human-only gate, not developer scope. Commented on #288 with exact state + what's left. diff --git a/work/sessions/2026-08-16-developer-issue-308.md b/work/sessions/2026-08-16-developer-issue-308.md deleted file mode 100644 index 1c02fcf7..00000000 --- a/work/sessions/2026-08-16-developer-issue-308.md +++ /dev/null @@ -1,19 +0,0 @@ - - - - -PR #322 is up, closes #308. - -**Summary (caveman mode, PR/commit text stayed normal per rules):** - -Built the architect-picked "second loader" (option A) for Frame C: - -- `ac-scene::sweep_ir` — new module. `SweepIrScene::from_report()` turns a `MeasurementReport` into trace+axis+arrival+header, or `SweepIrFault` (two named failure modes, UX text verbatim). Gate-absent check reads `payload.gate.is_some()` directly, not through `ir_stats()`'s legacy fallback (architect's explicit risk note). -- `ac-scene::readout::format_sweep_ir_header` — dynamic header string. -- `ac-view::report_flow` — `open_local` + `open_sweep_ir`, mirrors existing `snapshot_flow::open_local` shape, no daemon needed. -- `ac-view::view::draw_sweep_ir_panel` — pub paint fn, refactored shared trace/arrival drawing out of existing `draw_ir_panel`. -- New tests: 9 ac-scene unit, 1 readout, 3 report_flow, 3 `egui_kittest` paint-path integration. - -Deliberately did NOT wire a keybinding/ViewKind/Action — issue's own out-of-scope section + architect comment both defer the file-open UI to #256, matching the existing `Action::OpenSnapshot` no-op precedent in this repo. Flagged as open question in PR for reviewer to override if wanted. - -Gates: `cargo test --workspace` all green (0 failed anywhere), `cargo clippy --workspace --all-targets -- -D warnings` clean, `cargo fmt --check` clean. diff --git a/work/sessions/2026-08-16-developer-issue-311.md b/work/sessions/2026-08-16-developer-issue-311.md deleted file mode 100644 index fc3f954f..00000000 --- a/work/sessions/2026-08-16-developer-issue-311.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -PR #316 open, closes #311. Docs-only fix, both maps repaired + deduped to qa.md as source of truth. Clippy/fmt clean; workspace test failure pre-existing (worktree cache path quirk, confirmed via stash on unmodified tree). diff --git a/work/sessions/2026-08-16-developer-issue-312.md b/work/sessions/2026-08-16-developer-issue-312.md deleted file mode 100644 index 8a8e9f92..00000000 --- a/work/sessions/2026-08-16-developer-issue-312.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -PR #317 open, closes #312. Green on tests/clippy/fmt, `verified` flag left `false` per human-gate rule. diff --git a/work/sessions/2026-08-16-developer-issue-313.md b/work/sessions/2026-08-16-developer-issue-313.md deleted file mode 100644 index 65f4780c..00000000 --- a/work/sessions/2026-08-16-developer-issue-313.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -PR #320 open, closes #313. Tests/clippy/fmt clean, mutation-verified both halves red before revert. diff --git a/work/sessions/2026-08-16-developer-issue-314.md b/work/sessions/2026-08-16-developer-issue-314.md deleted file mode 100644 index 7b0e0a96..00000000 --- a/work/sessions/2026-08-16-developer-issue-314.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -PR #318 open, closes #314. Fix: `ac_daemon_bin()` reads `CARGO_TARGET_DIR` first, falls back to hardcoded path. Verified both named tests pass under `bin/common.sh`'s actual target dir value, full workspace suite green unset, clippy+fmt clean. diff --git a/work/sessions/2026-08-16-developer-issue-315.md b/work/sessions/2026-08-16-developer-issue-315.md deleted file mode 100644 index 8827000c..00000000 --- a/work/sessions/2026-08-16-developer-issue-315.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -PR #319 open, closes #315. Both handoff doc + issue #276 body corrected: 3382-1 §7.3 → `T > 2·T_det`, 3382-2 §7.3 → `T > T_det`, relaxation scoped to 3382-2's NOTE only, undated cross-ref flagged unverifiable. Docs-only, no build/test needed. diff --git a/work/sessions/2026-08-16-developer-pr-306-rev.md b/work/sessions/2026-08-16-developer-pr-306-rev.md deleted file mode 100644 index 5facf83a..00000000 --- a/work/sessions/2026-08-16-developer-pr-306-rev.md +++ /dev/null @@ -1,11 +0,0 @@ - - - - -Done. Pushed `db661b0` to `issue-285`, comment posted: https://github.com/mkovero/ac/pull/306#issuecomment-5306417787 - -Summary: -- correctness issue 1 (mic_correction_applied ambiguity) → fixed, report note added -- test gap (non-finite bins) → fixed, test added verbatim per suggestion -- AES17 clause verify → declined, out of scope (file untouched by this PR, review itself said non-blocking), said so in comment, offered as follow-up -- gate: full workspace test/clippy/fmt clean, no label touched, no new PR. diff --git a/work/sessions/2026-08-16-developer-pr-309-rev.md b/work/sessions/2026-08-16-developer-pr-309-rev.md deleted file mode 100644 index 9e20fa1a..00000000 --- a/work/sessions/2026-08-16-developer-pr-309-rev.md +++ /dev/null @@ -1,13 +0,0 @@ - - - - -Only stray untracked scratch files (never committed, harmless). Tracked tree clean, commit `bfa96da` pushed. - -Done. Summary: - -- Read both QA passes on PR #309. First-pass 4 gaps already closed by prior commit `411e2c5` (test coverage). Re-review left exactly one hold: no real-adapter screenshot for the IR panel snapshot test. -- Ran `snapshot_transfer_ir_panel` on the real GPU box (192.168.9.25, RTX 2070, real X session) per the repo's A3 policy — sandbox here has no GPU/lavapipe segfaults, matches what the dev's own prior comment already flagged. -- Committed `crates/ac-view/tests/snapshots/transfer_ir_panel.png` as `bfa96da`, pushed to `issue-286`. No label touched, no new PR. -- Posted reply comment on #309 with the rendered image, closing the last hold, and flagged (not silently dropped) that the same `UPDATE_SNAPSHOTS=1` run also touched the other five existing snapshots as font/driver noise — reverted those on the box, not committed, out of scope for this PR. -- No disagreement with QA's findings — all valid. diff --git a/work/sessions/2026-08-16-developer-pr-316-rev.md b/work/sessions/2026-08-16-developer-pr-316-rev.md deleted file mode 100644 index 33b900b5..00000000 --- a/work/sessions/2026-08-16-developer-pr-316-rev.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Done. Fixed qa.md:34 contradiction ("two papers"→"one paper"), pushed to issue-311 branch, commented on PR. No label touch, no new PR, no disagreements — QA's one finding was correct, small fix. diff --git a/work/sessions/2026-08-16-qa-pr-306.md b/work/sessions/2026-08-16-qa-pr-306.md deleted file mode 100644 index 0bfec8cf..00000000 --- a/work/sessions/2026-08-16-qa-pr-306.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -Posted. Approve verdict, `in-review` label already correct — no change needed. - -**Summary:** PR #306 tip `db661b0` — reviewed fresh. Ran full gate myself (`cargo test --workspace`, clippy, fmt) — all clean; independently reproduced the `it_live_end_to_end` failure as pre-existing `CARGO_TARGET_DIR` env artifact, not PR bug. Prior QA's `request-changes` point (ambiguous `mic_correction_applied` flag across two differently-corrected payloads) fixed via report note — verified it actually reaches both HTML and PDF renderers. Load-bearing test holds up on read. Re-verified ISO 3382-1 A.3.4 quote myself against primary PDF — matches. Flagged (non-blocking): `stddocs/AES-17-2015-1.pdf` now fully missing from main tree (was at least a preview before), and no end-to-end integration test for `plot_ir`'s mic-correction wiring (same gap pre-exists on `plot`/`plot_level`). Verdict: **approve**. diff --git a/work/sessions/2026-08-16-qa-pr-307.md b/work/sessions/2026-08-16-qa-pr-307.md deleted file mode 100644 index 24e43a34..00000000 --- a/work/sessions/2026-08-16-qa-pr-307.md +++ /dev/null @@ -1,14 +0,0 @@ - - - - -Done. PR #307 (docs currency sweep, closes #287) — **approve**, `in-review` + `agent:qa` labels applied. - -Summary: -- All 6 spec criteria covered — two (§7 no-gating, ARCHITECTURE.md:16/193 + README:164) correctly identified as already-resolved by prior work, not silently skipped; verified myself by reading the doc sections directly. -- Every factual claim in diff checked against real source, not trusted: `jack_backend.rs:375`, `cpal_backend.rs:363` do implement `play_and_capture`; default trait impl (`audio/mod.rs:52`) still bails; `sweep.rs` clean of stale text (moved to `plot.rs` by #282); `ZMQ.md` already carries `plot_ir`/`gated_frequency_response`. -- Re-ran gates myself, didn't trust PR body: `cargo test --workspace` → 1 failure (`it_live_end_to_end`, confirmed env/`CARGO_TARGET_DIR` path issue, unrelated to diff, matches PR body's own explanation), `cargo clippy -- -D warnings` clean, `cargo fmt --check --all` clean. -- Doc-only PR (+ 1 Rust doc-comment string) → standards conformance n/a, no new test surface. -- Scope clean, no correctness issues. - -Review posted as PR comment (not review-approve — GitHub blocks self-approval, PR author == authenticated account). diff --git a/work/sessions/2026-08-16-qa-pr-309-delta.md b/work/sessions/2026-08-16-qa-pr-309-delta.md deleted file mode 100644 index fa7dc455..00000000 --- a/work/sessions/2026-08-16-qa-pr-309-delta.md +++ /dev/null @@ -1,13 +0,0 @@ - - - - -Done. PR #309 re-review posted, verdict **approve**. - -Summary: -- delta = one commit, `transfer_ir_panel.png` only, no source change -- opened PNG myself — real IR trace, header, arrival marker, axes, not placeholder — matches spec -- all 4 prior-review gaps now closed (4th was the screenshot, closed this delta) -- `cargo test --workspace` / `clippy -D warnings` / `fmt --check` all clean at tip `bfa96da` -- standards check: not applicable, no value/format/unit touched -- label stays `in-review` (already correct for approve, no change needed) diff --git a/work/sessions/2026-08-16-qa-pr-309.md b/work/sessions/2026-08-16-qa-pr-309.md deleted file mode 100644 index d1195ebc..00000000 --- a/work/sessions/2026-08-16-qa-pr-309.md +++ /dev/null @@ -1,11 +0,0 @@ - - - - -Reviewed PR #309, tip commit 2174fc2. Comment posted, `needs-work` label applied (no `in-review` was set to remove). - -**Verdict: request-changes** - -Checked spec against #286 + architect/UX comments, ran full workspace test suite (rebuilt ac-daemon so daemon-spawning integration tests actually run instead of skip) — all green, clippy clean, fmt clean. Wire schema (`IrWireFrame`) verified byte-for-byte against `ZMQ.md:2094`. Numerics checked: snapshot-adapter's stride/dt_ms/t_origin_ms math is identical to daemon's own downsample formula, arrival marker correctly reuses `format_delay_readout` verbatim, `poll_frame` frame-skip logic correctly avoids the #219 regression. Scope discipline good — no Frame C leakage, follow-up #308 correctly filed pre-implementation. - -One real gap, blocking: **display-truth gate not discharged for the `ac-view` drawing half.** New `draw_ir_panel` code (panel-replaces-mag/phase branch, arrival-marker geometry, header/no-samples fallback) has zero harness coverage — grepped whole `ac-view/tests/` tree, only hit is a mechanical match arm, not a real test. No `it_geometry`-style shape assertion, no live/snapshot parity check extending the existing SPL pattern, no screenshot attached to PR. `ac-scene` half (pure fixture tests in `ir.rs`) is solid; `ac-view` half isn't. Wrote two suggested test additions in the review comment. diff --git a/work/sessions/2026-08-16-qa-pr-316-delta.md b/work/sessions/2026-08-16-qa-pr-316-delta.md deleted file mode 100644 index 93acda78..00000000 --- a/work/sessions/2026-08-16-qa-pr-316-delta.md +++ /dev/null @@ -1,12 +0,0 @@ - - - - -Posted. Verdict: **approve**. `in-review` label applied (already there, no `needs-work` present to remove — clean). - -Summary: -- Prior issue (contradictory "two papers" line) — fixed, verified against actual `iec-full/` tree (5 files, 1 paper). -- Full `cargo test --workspace` / clippy / fmt re-run at tip 49be82f: clean except pre-existing `it_live_end_to_end` binary-path failure, confirmed unrelated (reproduces at same tip regardless of delta, infra path mismatch not code). -- No new correctness/scope issues in delta. - -Note: mid-gate a stray old stash (`feat-205-drive-path-health`) in this worktree got popped by accident during my repro check, causing a transient conflict in `transfer.rs`. Caught and reverted immediately (`git checkout HEAD -- ...`), tree back to clean `49be82f`, old stash still intact in stash list untouched. No PR content affected, flagging for your awareness only. diff --git a/work/sessions/2026-08-16-qa-pr-316.md b/work/sessions/2026-08-16-qa-pr-316.md deleted file mode 100644 index f38b91ac..00000000 --- a/work/sessions/2026-08-16-qa-pr-316.md +++ /dev/null @@ -1,13 +0,0 @@ - - - - -Reviewed PR #316. Posted comment (couldn't post formal review — own PR, GH blocks self-request-changes). Labels: `needs-work` added. - -Verdict: **request-changes**, one issue. - -Path fixes all check out — verified every cited path against real `stddocs/` at `/home/mui/src/ac` via Glob (Bash `ls` denied by sandbox). All 4 IEC/AES corrections resolve, AES-17-2015 row correctly dropped (file not held), mislabelled `iec-full/Fundamentals_of_modern_audio_measurement.pdf` confirmed deleted (matches Markus's issue comment), root Cabot-paper copy confirmed present. - -Bug: `qa.md:34` rewritten warning self-contradicts. Says `iec-full/` holds "AES17-2020 and **two papers**" — but same sentence describes one of those two (mislabelled Fundamentals twin) as deleted. Post-deletion `iec-full/` has exactly one paper (Müller & Massarani). Same staleness class this PR exists to kill, just not re-checked after own edit. One-line fix: "two papers" → "one paper." - -ARCHITECTURE.md dedup clean, no other stale paths found anywhere in either file. Scope tight, matches issue's file list. Docs-only, no `.rs` touched, no test-gate concern. diff --git a/work/sessions/2026-08-16-qa-pr-317.md b/work/sessions/2026-08-16-qa-pr-317.md deleted file mode 100644 index a850d8d3..00000000 --- a/work/sessions/2026-08-16-qa-pr-317.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -Review posted, `in-review` + `agent:qa` labels applied. - -**verdict: approve.** Citation checked word-for-word against held AES17-2020 PDF (Annex A.4.5, page 35) — text matches code's claim exactly, `(informative)` qualifier correct. All 5 acceptance criteria met, human `verified:false` gate untouched, `cargo test --workspace`/`clippy -D warnings`/`fmt --check` all clean (one pre-existing `it_live_end_to_end` worktree-binary-path failure, reproduced independently, unrelated to diff). One non-blocking note: pre-existing weak test (`gated_frequency_response_citation_shape`) uses `.contains()` checks that wouldn't catch a 2020→2015 regression — suggested a tightened version in the review comment, following repo's existing test-quality convention rather than blocking on it. diff --git a/work/sessions/2026-08-16-qa-pr-318.md b/work/sessions/2026-08-16-qa-pr-318.md deleted file mode 100644 index 215c0584..00000000 --- a/work/sessions/2026-08-16-qa-pr-318.md +++ /dev/null @@ -1,9 +0,0 @@ - - - - -label already applied above (in-review), comment posted since self-approve blocked by GH. - -Verdict: **approve**. Comment: https://github.com/mkovero/ac/pull/318#issuecomment-5309482560 - -Summary — small correct fix. Verified both #314 tests green under real `CARGO_TARGET_DIR` (session already had it set to `bin/common.sh`-shaped path), full `cargo test --workspace` + clippy + fmt clean at tip `38959fe`. Single file, no scope creep, no standards applicable (test harness only). One non-blocking nit: `env::var_os("CARGO_TARGET_DIR")` read twice in `ac_daemon_bin()` — redundant, not a bug. diff --git a/work/sessions/2026-08-16-qa-pr-319.md b/work/sessions/2026-08-16-qa-pr-319.md deleted file mode 100644 index f3106912..00000000 --- a/work/sessions/2026-08-16-qa-pr-319.md +++ /dev/null @@ -1,17 +0,0 @@ - - - - -Reviewed PR #319. Verdict: **approve**. - -Checked both citations against primary PDFs at `/home/mui/src/ac/stddocs/iso-full/`: -- ISO 3382-1:2009 §7.3 Eq.(6)/(7): `BT>16`, `T>2·T_det` — matches PR text exactly (p.10 of the doc). -- ISO 3382-2:2008 §7.3 Eq.(4)/(5): `BT>16`, `T>T_det`, plus the NOTE relaxing to `BT>4`, `T>T_det/4` with the undated `ISO 3382-1:—` cross-ref — matches PR text word for word (p.8). -- Confirmed held ISO 3382-1:2009 §7.3 has no such NOTE and no time-reversal clause, so PR's claim ("not verifiable against the 2009 edition on disk") is correct. -- Issue #276 body checked via `gh issue view` — same corrected block present verbatim, so epic and handoff agree. - -All 4 acceptance criteria met. Docs-only change (1 file), no code touched, so `cargo test`/clippy gate not applicable — PR body states that honestly rather than faking a pass. No scope creep, no correctness issues. - -Couldn't apply formal GitHub "approve" review — GraphQL rejected it as your own PR (you're both author and the gh-authenticated user). Posted full review as a PR comment instead: https://github.com/mkovero/ac/pull/319#issuecomment-5309497275 - -No label change needed (approve state, no `needs-work`). diff --git a/work/sessions/2026-08-16-qa-pr-320.md b/work/sessions/2026-08-16-qa-pr-320.md deleted file mode 100644 index a51ccb76..00000000 --- a/work/sessions/2026-08-16-qa-pr-320.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Posted. Approve — `in-review` stays (already set, no `needs-work` to remove). diff --git a/work/sessions/2026-08-16-qa-pr-322.md b/work/sessions/2026-08-16-qa-pr-322.md deleted file mode 100644 index 32a28804..00000000 --- a/work/sessions/2026-08-16-qa-pr-322.md +++ /dev/null @@ -1,17 +0,0 @@ - - - - -Review posted, labels updated (`needs-work` applied, `in-review` removed). - -**PR #322 — verdict: request-changes.** - -Found real bug: arrival marker prints uncorrected round-trip delay as a physical distance (metres). `SweepIrScene::from_report` (`ac-scene/src/sweep_ir.rs:208`) hardcodes `delay_locked: Some(true)` into `format_delay_readout`, which shows metres unconditionally whenever `arrival_ms >= 0`. Codebase has explicit rule against this — `ArrivalDistance`'s own doc (`report.rs:744-747`): "deliberately no third case that returns an uncorrected arrival as if it were a distance (#283)." Correct helper `MeasurementReport::ir_arrival_distance()` already exists, checks for `interface_latency`, returns `Unavailable` when no τ — PR bypasses it entirely. - -Bug hidden because every test fixture places IR peak exactly at gate window centre, so `arrival_s=0` always — buggy and correct paths print identical `"0.00 ms (0.00 m)"`. No test exercises nonzero delay. - -Minor second issue: header's `gate ± X ms` figure recomputed from `gate_length_s/2` instead of stored `gate_start_s`, diverges by up to half sample for odd-length gates (daemon does integer-truncated division on `gate_start_s` but not `gate_length_s`) — contradicts the "store, don't re-derive" rule the same file cites elsewhere. Untested (fixtures hand-picked symmetric). - -Build gate itself is clean: `cargo test --workspace` (908+ tests) exit 0, `cargo clippy --workspace --all-targets -D warnings` exit 0, `cargo fmt --check` clean — verified by running them directly, not trusting PR body. - -Full review posted as PR comment. diff --git a/work/sessions/2026-08-16-ux-286.md b/work/sessions/2026-08-16-ux-286.md deleted file mode 100644 index 15b72b35..00000000 --- a/work/sessions/2026-08-16-ux-286.md +++ /dev/null @@ -1,11 +0,0 @@ - - - - -Done. Comment updated on #286, `needs-ux` dropped, `ready-to-implement` stays. - -Summary: -- Architect scoped down to option A: Frame A/B (live sidecar) ship now, Frame C (sweep-derived) deferred — separate file format, no path `ac-view` opens yet. -- Rewrote comment: marked A/B in-scope, C out-of-scope-but-spec'd-for-later, moved keybinding + data-path questions to "resolved by architect." -- Left one real open item: follow-up issue for Frame C not yet filed — flagged, not this agent's job to file. -- Removed `needs-ux`, spec complete for #286's actual (reduced) scope. diff --git a/work/sessions/2026-08-16-ux-308.md b/work/sessions/2026-08-16-ux-308.md deleted file mode 100644 index 559fab20..00000000 --- a/work/sessions/2026-08-16-ux-308.md +++ /dev/null @@ -1,10 +0,0 @@ - - - - -Done. #308: posted design comment, removed `needs-ux`. - -Summary: -- Success-path rendering (Frame C) carried forward verbatim from #286 — already fully specified there, not re-derived. -- New work this issue actually needed: fault text for the two ways a file-open can fail — not a `MeasurementReport` at all, vs valid report with `GateParams` absent. Designed both as separate strings, following `fault.rs`'s "name what to check, not the cause" rule (same register as `NO LOCK`). -- Left `needs-design` in place — architect hasn't ruled on second-loader-vs-sidecar yet, not ux's call. Flagged that the loader decision may make one of the two fault strings need a reword later (e.g. if sidecar shares `.acsnap` extension). Did not apply `ready-to-implement` — architect owns that transition per spec. diff --git a/work/sessions/2026-08-17-architect-issue-297.md b/work/sessions/2026-08-17-architect-issue-297.md deleted file mode 100644 index 46e3f677..00000000 --- a/work/sessions/2026-08-17-architect-issue-297.md +++ /dev/null @@ -1,9 +0,0 @@ - - - - -Done. #297: posted design decision, labels swapped `needs-design` → `ready-to-implement`. - -**Decision:** full `tau_history` array over wire (Option A), not daemon-reduced `latest_tau`+count. Reasons — `mic_response` already sets precedent of full-data-over-reduced in same reply; no consumer exists yet to justify centralizing selection logic (grep confirms `ac-view` doesn't touch either command); UX comment explicitly left "which entry" open, baking it into wire schema would foreclose that early. - -CLI picks newest by `measured_at` string compare (same idiom `tau_for` already uses), renders per UX's spec block. New `age_from_iso8601` helper goes in `ac-core/shared/time.rs` (Tier 0 timestamp module), not inline in CLI. No `needs-ux` added — UX comment already covers the render surface and said its spec doesn't change based on this wire choice. diff --git a/work/sessions/2026-08-17-architect-issue-321.md b/work/sessions/2026-08-17-architect-issue-321.md deleted file mode 100644 index d81a2b27..00000000 --- a/work/sessions/2026-08-17-architect-issue-321.md +++ /dev/null @@ -1,9 +0,0 @@ - - - - -Done. Issue #321 posted, labels updated: `needs-design` off, `ready-to-implement` + `needs-ux` + `agent:architect` on. - -Design gist: recommend **option B** — keep `ac_scene::transfer::TransferScene` single-trace, unchanged. Multiplicity (N stored runs, each own `Smoothing`) built by `ac-view` calling existing `TransferScene::from_input` machinery repeatedly (deliverable 3's snapshot-overlay already proved this shape works). No wire change, no `ac-scene` type change. Real cost lands on `ac-view`: `TransferViewState` gets `Vec` + focus, `draw_view` signature changes from one `Option<&TransferScene>` to live+slice. - -Flagged `needs-ux` because the design confirms UX's already-posted mockup (focus marker, per-row smoothing, dim-vs-bright not colour) is exactly what's implementable — worth a UX pass to confirm alignment, not reopen it. diff --git a/work/sessions/2026-08-17-architect-issue-329.md b/work/sessions/2026-08-17-architect-issue-329.md deleted file mode 100644 index 883fbed2..00000000 --- a/work/sessions/2026-08-17-architect-issue-329.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Done. #329: option A (inline `— provenance: {tag}` suffix on checklist line, tag defined once in `AGENTS.md`, qa gates scoped to numeric derived/assumed criteria via existing `needs-work` label, not a new one). Posted, labels swapped `needs-design`→`ready-to-implement` + `agent:architect`. No ZMQ/ux impact — pure `.agents/` spec change. diff --git a/work/sessions/2026-08-17-architect-issue-330.md b/work/sessions/2026-08-17-architect-issue-330.md deleted file mode 100644 index 51cf83d8..00000000 --- a/work/sessions/2026-08-17-architect-issue-330.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Posted. Design comment on #330: option A (new `.agents/rig.md`, `audit.md`-shaped manual-invocation role) recommended over folding into `developer.md`. No code/ZMQ impact — pure `.agents/` spec infra. Labels swapped: `needs-design` off, `ready-to-implement` on. diff --git a/work/sessions/2026-08-17-developer-issue-295.md b/work/sessions/2026-08-17-developer-issue-295.md deleted file mode 100644 index d36b2721..00000000 --- a/work/sessions/2026-08-17-developer-issue-295.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -PR #323 open, closes #295. Test-only, mutation-verified, all gates green. diff --git a/work/sessions/2026-08-17-developer-issue-297.md b/work/sessions/2026-08-17-developer-issue-297.md deleted file mode 100644 index 027e19cc..00000000 --- a/work/sessions/2026-08-17-developer-issue-297.md +++ /dev/null @@ -1,14 +0,0 @@ - - - - -PR #324 open, closes #297. Full workspace test/clippy/fmt all clean. - -Summary: -- `admin.rs`: `get_calibration`/`list_calibrations` now carry `tau_history` (always array, `[]` unmeasured). -- `time.rs`: new `age_from_iso8601` helper, tested. -- `calibrate.rs`: `run_show` gains `Delay:` leg — pure `render_tau_history_leg` fn (testable) picks newest entry, shows value/age/conditions/ports/`+N more`, matches UX mock exactly. -- `it_protocol.rs`: wire test, present + absent history. -- `ZMQ.md`: documented new field both replies. - -PR: https://github.com/mkovero/ac/pull/324 diff --git a/work/sessions/2026-08-17-developer-issue-321.md b/work/sessions/2026-08-17-developer-issue-321.md deleted file mode 100644 index 44f792c8..00000000 --- a/work/sessions/2026-08-17-developer-issue-321.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -You've hit your monthly spend limit · raise it at claude.ai/settings/usage?from=cc_cli_limit_message diff --git a/work/sessions/2026-08-17-developer-issue-326.md b/work/sessions/2026-08-17-developer-issue-326.md deleted file mode 100644 index 2a8c4e24..00000000 --- a/work/sessions/2026-08-17-developer-issue-326.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -PR #331 open, closes #326. Doc-only diff, 4 files, matches spec exactly. Ready for QA. diff --git a/work/sessions/2026-08-17-developer-issue-327.md b/work/sessions/2026-08-17-developer-issue-327.md deleted file mode 100644 index cc36b779..00000000 --- a/work/sessions/2026-08-17-developer-issue-327.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -PR #332 opened, closes #327. Doc-only change: `.agents/qa.md` step 3 gains reachability/ignore/fake-mock checks, both `qa.md` and `developer.md` gain `$AC_HOME` scratch-space rule. Full workspace test green, clippy clean, fmt clean. Scope: exactly 2 files, matches spec's likely-affected list minus `.agents/bin/*.sh` (read-only per issue). diff --git a/work/sessions/2026-08-17-developer-issue-328.md b/work/sessions/2026-08-17-developer-issue-328.md deleted file mode 100644 index 0a63deb4..00000000 --- a/work/sessions/2026-08-17-developer-issue-328.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -PR #333 opened, closes #328. Doc-only change, `.agents/qa.md` +17 lines. Warning about uncommitted change is untracked `.cargo/` dir, not related to this issue — left alone. diff --git a/work/sessions/2026-08-17-developer-issue-329.md b/work/sessions/2026-08-17-developer-issue-329.md deleted file mode 100644 index 4fc0ef50..00000000 --- a/work/sessions/2026-08-17-developer-issue-329.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -PR #334 open, closes #329. Uncommitted-change warning = untracked `.cargo/` dir, unrelated to this issue, left alone. - -**Summary:** spec-only change, no `ac-rs/` code. Tagged `AGENTS.md`/`triage.md`/`architect.md`/`qa.md` with measured/derived/assumed provenance convention per architect's option A. Gate (fmt/clippy/test) ran clean, unaffected since no Rust touched. PR: https://github.com/mkovero/ac/pull/334 diff --git a/work/sessions/2026-08-17-developer-issue-330.md b/work/sessions/2026-08-17-developer-issue-330.md deleted file mode 100644 index b58df6f5..00000000 --- a/work/sessions/2026-08-17-developer-issue-330.md +++ /dev/null @@ -1,11 +0,0 @@ - - - - -PR #335 open, closes #330. - -- `.agents/rig.md` new role: pre-flight, interlocks (hard constraints), record format, decline-to-conclude, work/rig/ expiry rules. -- `.agents/AGENTS.md`: role table row + invocation example. -- `.agents/bin/rig-run.sh`: new, mirrors audit-run.sh. - -No ac-rs code touched, no gate needed beyond fmt sanity (clean). PR: https://github.com/mkovero/ac/pull/335 diff --git a/work/sessions/2026-08-17-developer-pr-322-rev.md b/work/sessions/2026-08-17-developer-pr-322-rev.md deleted file mode 100644 index 63d81d54..00000000 --- a/work/sessions/2026-08-17-developer-pr-322-rev.md +++ /dev/null @@ -1,13 +0,0 @@ - - - - -Fixed both correctness bugs, added tests, pushed to `issue-308-sweep-ir-loader`, commented on PR #322. - -- **Issue 1** (arrival marker leaks uncorrected distance): now gated through `report.ir_arrival_distance()` — metres only when τ measured, ms tau-corrected when it is. -- **Issue 2** (gate ± re-derived not stored): header now reads `gate.gate_start_s` direct, not `gate_window_s/2`. -- Added 4 new tests QA named missing (nonzero-delay fixture, τ-correction, asymmetric gate, degenerate single-sample span). Updated 1 existing test that pinned old buggy behavior. -- `cargo test --workspace` / clippy -D warnings / fmt --check all clean. -- One point I pushed back on in comment: AC1 "unreachable from keybinding" — QA itself flagged that as design-sanctioned (#256 scope), not a defect. Left as-is, said so. - -No label change, no new PR — same branch, same PR #322. diff --git a/work/sessions/2026-08-17-developer-pr-333-rev.md b/work/sessions/2026-08-17-developer-pr-333-rev.md deleted file mode 100644 index 1adbbecf..00000000 --- a/work/sessions/2026-08-17-developer-pr-333-rev.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Done. Fixed crate-boundary claim (#246 confirmed same-file, same-crate via `git show 9522ee3`), named which of the three instances crossed boundary vs not, pushed to `issue-328`, replied on PR. No label touch, no new PR, agreed with QA's finding — no disagreement to flag. diff --git a/work/sessions/2026-08-17-developer-pr-335-rev.md b/work/sessions/2026-08-17-developer-pr-335-rev.md deleted file mode 100644 index 34890814..00000000 --- a/work/sessions/2026-08-17-developer-pr-335-rev.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -Done. Pushed commit `9977721` to `issue-330`, replied on PR #335. - -Bottom line: QA's one correctness finding (claimed `measured`/`derived`/`assumed` taxonomy fabricated) checked false — taxonomy real, in `AGENTS.md:134-145`, already on branch when reviewed. Said so in comment w/ grep proof. Tightened `rig.md` citation anyway (direct quote, dropped shaky "exists because of this role" causal claim) — strictly better, no reason not to. Label left alone, no new PR opened, per instructions. diff --git a/work/sessions/2026-08-17-qa-pr-322-delta.md b/work/sessions/2026-08-17-qa-pr-322-delta.md deleted file mode 100644 index 99b5e944..00000000 --- a/work/sessions/2026-08-17-qa-pr-322-delta.md +++ /dev/null @@ -1,14 +0,0 @@ - - - - -Posted. Done — label already `in-review`, correct for approve, no change made. - -Summary (caveman-terse): -- Range reviewed: f85e69c..812104b. -- Both prior bugs fixed right: arrival distance now τ-gated via `ir_arrival_distance()`, header uses stored `gate_start_s` not re-derived half. -- Both coverage gaps filled: nonzero-delay fixture, degenerate single-sample test. -- Gate green workspace-wide: test/clippy/fmt all clean. -- Standards: n/a, same as first pass. -- One new non-blocking finding: marker dot position (raw arrival) and its label text (τ-corrected in Known branch) now come from different time values — flagged as advisory for architect, not fixed here, not required to block. -- Verdict: approve. Label unchanged (`in-review`). diff --git a/work/sessions/2026-08-17-qa-pr-323.md b/work/sessions/2026-08-17-qa-pr-323.md deleted file mode 100644 index 2fc4394a..00000000 --- a/work/sessions/2026-08-17-qa-pr-323.md +++ /dev/null @@ -1,15 +0,0 @@ - - - - -Posted: https://github.com/mkovero/ac/pull/323#issuecomment-5314790350 - -Verdict: **approve**. Summary: - -- Spec coverage complete — first-prompt cancel test added, mutation-verified claim checked myself (mutated step-1 guard to `if false`, test failed on the "no second cal_prompt" assertion exactly as PR body says, reverted, tree clean). -- No production code touched — test-only diff, scope matches triage's "files likely affected" exactly. -- Full gate green: `cargo test --workspace`, `cargo clippy --workspace --tests -- -D warnings`, `cargo fmt --check`. -- Standards check: n/a — no measurement/display value touched. -- No drive-path / value-display / daemon-pipeline gates apply — none of those code paths touched. - -Note: couldn't apply GitHub `approve` review — same account as PR author (`mkovero`), GitHub blocks self-approval. Posted findings as regular comment instead. Also repo has no `in-review`/`needs-work` labels defined at all (checked `gh label list`), so no label action possible either way — flagged in comment. diff --git a/work/sessions/2026-08-17-qa-pr-324.md b/work/sessions/2026-08-17-qa-pr-324.md deleted file mode 100644 index 8e6de2ed..00000000 --- a/work/sessions/2026-08-17-qa-pr-324.md +++ /dev/null @@ -1,7 +0,0 @@ - - - - -Review posted, `in-review` label applied. - -**Verdict: approve.** All 7 acceptance criteria from #297 covered. Wire shape (`tau_history` on `get_calibration`/`list_calibrations`) verified against `TauEntry`/`TauConditions` structs directly — field types line up. CLI render matches UX mockup verbatim. Traceability criterion (shown conditions = shown value's conditions, not a mismatched pair) has its own dedicated test, not just implied. No scope creep — confirmed diff is exactly this PR's one commit against `origin/main` (local `main` ref was stale, checked separately). `cargo test --workspace`, clippy, fmt all reran clean at tip. Standards check: not applicable — τ is interface round-trip latency, no AES/IEC/ISO/ITU-R clause governs it. diff --git a/work/sessions/2026-08-17-qa-pr-331.md b/work/sessions/2026-08-17-qa-pr-331.md deleted file mode 100644 index 05547ddb..00000000 --- a/work/sessions/2026-08-17-qa-pr-331.md +++ /dev/null @@ -1,9 +0,0 @@ - - - - -Reviewed. Own PR — GH review API refused approve, posted as comment instead. - -**Verdict: approve.** Docs-only change to `.agents/*.md`, all 5 acceptance criteria from #326 met, wording faithful to spec, no scope creep, no code/standards touched. One correctness nit: qa.md's load-bearing why-paragraph runs 3 lines not spec's literal "one line" — content correct, not blocking. - -Flagged one process gap: repo has **no `in-review` or `needs-work` label defined** (`gh label list` confirms) — qa.md step 5 assumes both exist. Couldn't apply label per spec; noted in comment for someone to fix label plumbing. diff --git a/work/sessions/2026-08-17-qa-pr-332.md b/work/sessions/2026-08-17-qa-pr-332.md deleted file mode 100644 index e7bc9e49..00000000 --- a/work/sessions/2026-08-17-qa-pr-332.md +++ /dev/null @@ -1,11 +0,0 @@ - - - - -Review posted (comment, not formal review — gh authed as PR author `mkovero`, API refuse self-approve). Labels: `in-review` + `agent:qa` applied. - -**Verdict: approve.** - -Gate: `cargo test --workspace` green, `clippy -D warnings` clean, `fmt --check` clean, at tip `5d2c8b2`. All 6 acceptance criteria from #327 met verbatim in `qa.md` (reachability/fake-mock/`#[ignore]` bullets in step 3) and `developer.md`/`qa.md` (scratch-space section). Landing order respected — #326 merged (via #331) before this base commit, #328/#329 still open, untouched. `bin/*.sh` left alone per out-of-scope note. - -One non-blocking finding I dug into: doc's `$AC_HOME` var doesn't exist anywhere in `bin/common.sh` — grepped whole tree, zero hits outside the two new lines. Actual scripts use three separately-defaulted vars (`AC_WT_BASE`→`$ROOT/../ac-wt`, `AC_TARGET_ROOT`→`~/.cache/ac-target`, `AC_LOG_DIR`→`~/.local/state/ac`) that don't share a root by default — target/log do NOT default under `ac-wt`. Dev's PR-body claim of having "confirmed" this against `bin/common.sh` overstates what it checked (worktree placement only). Not blocking — wording came verbatim from triage's acceptance criteria, `bin/*.sh` explicitly out of scope. Flagged in review as suggested follow-up issue, not a gate. diff --git a/work/sessions/2026-08-17-qa-pr-333-delta.md b/work/sessions/2026-08-17-qa-pr-333-delta.md deleted file mode 100644 index ace640b4..00000000 --- a/work/sessions/2026-08-17-qa-pr-333-delta.md +++ /dev/null @@ -1,13 +0,0 @@ - - - - -Review posted. Approve verdict — `in-review` label already set, correct state, no change needed. - -Summary: -- Range reviewed: `833ff61..a7a6b1a`. -- Prior point (false "all three crossed crate boundary" claim) — fixed, re-verified independently against `git show 9522ee3`: #246's two constants both sat in `ac-core/src/visualize/transfer.rs`, same crate. -- Delta = one hunk, wording only, no other files touched. -- Gate re-run at new tip: `cargo test --workspace` pass, `cargo clippy --workspace --all-targets -- -D warnings` clean, `cargo fmt --check` clean. -- Standards check: n/a, docs-only delta. -- Verdict: approve. diff --git a/work/sessions/2026-08-17-qa-pr-333.md b/work/sessions/2026-08-17-qa-pr-333.md deleted file mode 100644 index 0cba6998..00000000 --- a/work/sessions/2026-08-17-qa-pr-333.md +++ /dev/null @@ -1,17 +0,0 @@ - - - - -Review posted: https://github.com/mkovero/ac/pull/333#issuecomment-5320820938 - -Verdict: **request-changes**. - -Spec criteria mostly met — cited file/line (`fault.rs:1473`, `:1453`) and PR #253 checked against tree, correct. But new checklist text claims "the first three instances of this shape all crossed a crate boundary, which is why no type check caught them." Checked all three: - -- #238 (`settled` vs ladder) — cross-crate (`ac-scene` reads what `ac-daemon` builds). True. -- #247 (admission vs refusal timer) — cross-crate (`ac-core` vs `ac-scene`). True. -- #246 (`MIN_PROMINENCE` vs `DIRECT_PEAK_FRACTION`) — **same file, same crate.** `git show 9522ee3` (closes #246): both constants lived together in `ac-core/src/visualize/transfer.rs`. No crate boundary involved. - -So doc's blanket claim false for 1 of 3 cited instances — ironic spot for it, since this doc is what tells future QA passes to distrust unverified elaborated claims. Flagged as correctness issue, requested fix before merge. - -Also noted: repo has no `needs-work`/`in-review` label defined (checked `gh label list`), so couldn't apply per spec step 5 — called out in comment instead. diff --git a/work/sessions/2026-08-17-qa-pr-334.md b/work/sessions/2026-08-17-qa-pr-334.md deleted file mode 100644 index 8fc6c2c9..00000000 --- a/work/sessions/2026-08-17-qa-pr-334.md +++ /dev/null @@ -1,9 +0,0 @@ - - - - -Reviewed PR #334, posted, labeled `in-review`. - -**Verdict: approve.** - -Spec-only change (`.agents/*.md`, no `ac-rs/` code) — 7 triage ACs covered, cross-file consistency checked (AGENTS.md defines tags once, other three reference not redefine), "no other script parses template" claim independently verified (grepped all `.agents/bin/*.sh` + `ux.md`/`developer.md`/`audit.md` myself, not trusted from PR body), step numbering in `qa.md` intact despite concurrent #326–328 risk architect flagged. Standards check n/a — no measurement/display code touched. One non-blocking nit: triage.md template's two example criterion lines (one tagged, one not) slightly ambiguous without the prose below, noted in review, not blocking. diff --git a/work/sessions/2026-08-17-qa-pr-335-delta.md b/work/sessions/2026-08-17-qa-pr-335-delta.md deleted file mode 100644 index e96cece9..00000000 --- a/work/sessions/2026-08-17-qa-pr-335-delta.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Posted. Range ca28703..9977721. Gate green (test/clippy/fmt all pass full workspace). Prior finding fixed — verified new quotes against `AGENTS.md:126-141` directly, match. Noted own earlier sub-claim was wrong (taxonomy did exist) but that wasn't core issue; core issue (fabricated causal claim, paraphrase) now fixed. One non-blocking nit: quote merges two sentences without bracket marking. Verdict: approve. Label `in-review` already set, no change needed. diff --git a/work/sessions/2026-08-17-qa-pr-335.md b/work/sessions/2026-08-17-qa-pr-335.md deleted file mode 100644 index 34917a10..00000000 --- a/work/sessions/2026-08-17-qa-pr-335.md +++ /dev/null @@ -1,13 +0,0 @@ - - - - -Review posted (comment, since API blocks self-review): https://github.com/mkovero/ac/pull/335#issuecomment-5321170798. Label set `needs-work`. - -Verdict: **request-changes**. - -Spec coverage full — all 9 triage ACs met, files match "likely affected" list exactly, no scope creep. Standards check n/a (pure `.agents/` spec infra, no `ac-rs/` code). - -Checked every rig-fact citation against primary files (`work/rig/rig-session-2-results.md`, `rig-verify-queue.md`) — all true: −30 dBFS exception w/ `drive_max_dbfs` clamp, sha256-not-mtime, AutoSync/ADAT/playback_5 reasoning, "session 3 supersedes" note, block 1's `median_value`/`negative_lag_median` requirement. All verified real. - -One bug: `.agents/rig.md:24-27` claims `AGENTS.md`'s evidence-discipline section uses a `measured`/`derived`/`assumed` taxonomy. Checked — false. `grep -rn assumed .agents/` = nothing. Section has "derived" once, no three-state schema anywhere. Fabricated cite, in doc whose whole point is citation/measurement discipline. That's the failure worth blocking on. diff --git a/work/sessions/2026-08-17-ux-297.md b/work/sessions/2026-08-17-ux-297.md deleted file mode 100644 index 29cf4f9b..00000000 --- a/work/sessions/2026-08-17-ux-297.md +++ /dev/null @@ -1,5 +0,0 @@ - - - - -Done. Design comment posted on #297 (proposed `Delay:` third leg for `ac calibrate show`, symmetric with Output/Input — value + measured timestamp/age on one line, full τ conditions and ports split onto their own lines since `show` has no live session to imply them, explicit `not measured` state, `+N more` note for multi-entry keys). Removed `needs-ux` only — `needs-design` stays, no `ready-to-implement`, since architect hasn't weighed in yet on the "which stored τ entry does a multi-entry key render" wire-shape question the issue itself flagged. diff --git a/work/sessions/2026-08-17-ux-321.md b/work/sessions/2026-08-17-ux-321.md deleted file mode 100644 index 478d7b55..00000000 --- a/work/sessions/2026-08-17-ux-321.md +++ /dev/null @@ -1,14 +0,0 @@ - - - - -Done. #321: design comment updated (edited existing ux comment, not a new one), covers: - -- legend needs `derot` field per row too, not just smoothing — stored runs pinned `self`, live shows what it tracks and by how much (the phase-convention mismatch the external review flagged). -- `⚠` mismatch marker on rows whose derivation (weighting/`nperseg`/cal) differs from focused trace — cheap signal, not a full dump on every row. -- new `derivation (▸ …)` readout line, focused-trace only, full weighting/nperseg/cal/spl_ref detail — same owner-tagged pattern as delay readout. -- worst-case width render shows filename-vs-alias question is no longer cosmetic (derot's "→ target" repeats a long filename) — flagged in open questions. -- new open question: does `⚠` comparison belong in `ac-view` (indexing over `LoadedRun` metadata) or `ac-scene` — architect's Option B didn't cover this cross-trace comparison. -- new open question: derivation data availability depends on #308's stored format; if absent, readout must say "unknown," never silently omit (would read as false negative). - -Removed `needs-ux` label — proposal concrete, every field justified, literal renderings shown. `ready-to-implement` was already applied, left as-is. From 9351bf3e64b2d855057e26fe10a3e83e2845a03e Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Mon, 14 Sep 2026 21:04:23 +0000 Subject: [PATCH 4/5] docs(rig): restore #368's rig-verify-queue.md block Dropped by dae1f47a's file-replace-from-main during the .agents/bin/.claude refresh; QA re-review on PR #384 flagged it missing at the current tip. Content is unchanged from what QA's prior review accepted at 8c36a06, re-inserted at rig/rig-verify-queue.md (the file's current path after the main-branch move dae1f47a itself made). Co-Authored-By: Claude Sonnet 5 --- rig/rig-verify-queue.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/rig/rig-verify-queue.md b/rig/rig-verify-queue.md index b9fe7030..5f5436eb 100644 --- a/rig/rig-verify-queue.md +++ b/rig/rig-verify-queue.md @@ -26,6 +26,23 @@ only planned run producing a legitimately gated ring, which is the case the dropped onset guard must not suppress, so it carries per-frame `median_value` / `negative_lag_median` as well. Full statement in block 4. +- **#368's `TAU_SNR_THRESHOLD_DB` constant — QA on PR #384 (2026-08-23) + flagged it `derived`, not measured on the sweep configuration it gates.** + Its two anchors (33.8–83.5 dB electrical loopback, ~16 dB #376 acoustic + cliff) come from a different window-length rig session + (`rig-2026-08-22-tau-window-350-results.md`) and a different, longer-ESS + acoustic path — neither is `calibrate`'s own short-ESS electrical τ path. + + Run `calibrate`'s actual τ path against the three cases the issue + measured: hot loopback (+3.01 dB), low-gain loopback (-4.19 dB), muted + route (-83.8 dBFS). Record `tau_pre_impulse_snr_db` for each. + + > **Pass: both real loopbacks read at or above 24 dB, and the muted + > route reads below it.** Either real loopback's SNR coming back under + > 24 dB would wrongly refuse a working cable; the muted route's SNR + > coming back over 24 dB would wrongly accept noise as a peak. Both are + > falsifications of the current constant, not readouts to shrug past. + Two things session 3 raised that no block here covers yet: - **The cable change, and the one measurement that verifies it — #243.** Move From d52b5fd425034cd88cf729c9c0bfca56679e4523 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Mon, 14 Sep 2026 22:28:32 +0000 Subject: [PATCH 5/5] fix: make step-2 captured level honor the loopback gain override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex QA finding on PR #384: calibrate_measures_tau_on_hot_off_unity_fake_loopback claimed to drive #368's AC8 (off-unity captured-level path) but AC_FAKE_TAU_GAIN_OVERRIDE scaled only play_and_capture's ESS, never the step-2 tone capture calibrate reads via capture_rms/capture_block — so step 2 still saw unity loopback and the test's final tau_state==measured assertion proved nothing about the off-unity path a reintroduced level gate could still fail. FakeEngine::capture_block now applies the same gain override, since it models the loopback cable's own gain rather than something specific to the τ ESS. The test pins step 2's captured_dbfs/loopback fields (must land at -30.0 dBFS / loopback:false for the +3.01 dB hot case) before asserting the final measured state. Co-Authored-By: Claude Sonnet 5 --- .../crates/ac-daemon/src/audio/fake/hooks.rs | 11 +++- ac-rs/crates/ac-daemon/src/audio/fake/mod.rs | 15 ++++- .../tests/it_protocol/calibrate/tau.rs | 57 ++++++++++++++----- 3 files changed, 64 insertions(+), 19 deletions(-) diff --git a/ac-rs/crates/ac-daemon/src/audio/fake/hooks.rs b/ac-rs/crates/ac-daemon/src/audio/fake/hooks.rs index 27f82e2d..523686b3 100644 --- a/ac-rs/crates/ac-daemon/src/audio/fake/hooks.rs +++ b/ac-rs/crates/ac-daemon/src/audio/fake/hooks.rs @@ -64,9 +64,14 @@ pub(super) fn period_size_override() -> Option { /// clean, noiseless delayed copy of the played signal (the loopback shape /// every other τ test relies on). /// -/// `AC_FAKE_TAU_GAIN_OVERRIDE`: scales the played-signal copy that would -/// otherwise land unattenuated at `delay_samples`. `1.0` (unset) keeps the -/// existing unity loopback; `0.0` simulates a fully muted route. +/// `AC_FAKE_TAU_GAIN_OVERRIDE`: models the loopback cable's own gain, so it +/// scales both `play_and_capture`'s played-signal copy (the τ ESS) and +/// `capture_block`'s tone synthesis (`calibrate` step 2's captured level, +/// via `capture_rms`) — the same cable, read by two different captures. +/// `1.0` (unset) keeps the existing unity loopback on both paths; `0.0` +/// simulates a fully muted route. Before PR #384's codex-qa finding this +/// scaled only `play_and_capture`, so an off-unity gain never reached step +/// 2's `captured_dbfs`/`loopback` fields. /// `AC_FAKE_TAU_NOISE_AMPLITUDE_OVERRIDE`: peak amplitude of broadband /// dither added to every sample of `play_and_capture`'s output. `0.0` /// (unset) is byte-identical to pre-#368 behaviour — with the gain also at diff --git a/ac-rs/crates/ac-daemon/src/audio/fake/mod.rs b/ac-rs/crates/ac-daemon/src/audio/fake/mod.rs index 9de9d086..a4e22a72 100644 --- a/ac-rs/crates/ac-daemon/src/audio/fake/mod.rs +++ b/ac-rs/crates/ac-daemon/src/audio/fake/mod.rs @@ -221,6 +221,14 @@ impl AudioEngine for FakeEngine { self.gen.set_correlated_pair(gain, delay_samples); } + /// #368 codex-qa finding on PR #384: `AC_FAKE_TAU_GAIN_OVERRIDE` models + /// the loopback cable's own gain, and `calibrate`'s step-2 captured + /// level (read through this path via `capture_rms`) is that same cable + /// — so the override has to reach it, not just `play_and_capture`'s τ + /// ESS. Before this it was applied only there, so a test driving an + /// off-unity gain through this hook could never actually see step 2 + /// report the off-unity `captured_dbfs`/`loopback` it claimed to + /// exercise. Unset (`1.0`) multiplies by 1.0, i.e. unchanged. fn capture_block(&mut self, duration: f64) -> Result> { let n = self.samples_in(duration); if let Some(out) = self.ring_capture(n, duration, RingDrain::Block) { @@ -228,7 +236,12 @@ impl AudioEngine for FakeEngine { } std::thread::sleep(Duration::from_secs_f64(duration)); let port = self.input_port.clone(); - Ok(self.synth().block(port.as_deref(), duration, 0)) + let gain = tau_gain_override(); + let mut block = self.synth().block(port.as_deref(), duration, 0); + for v in block.iter_mut() { + *v *= gain; + } + Ok(block) } /// Non-clearing drain. In ring mode this is the *contiguous* control arm: diff --git a/ac-rs/crates/ac-daemon/tests/it_protocol/calibrate/tau.rs b/ac-rs/crates/ac-daemon/tests/it_protocol/calibrate/tau.rs index 0e68687f..0cde62d9 100644 --- a/ac-rs/crates/ac-daemon/tests/it_protocol/calibrate/tau.rs +++ b/ac-rs/crates/ac-daemon/tests/it_protocol/calibrate/tau.rs @@ -124,17 +124,28 @@ fn calibrate_reports_not_measured_low_snr_on_muted_fake_loopback() { ); } -/// #368 AC8 (QA request-changes on PR #384): closes the gap the muted-route -/// test alone leaves. `calibrate_measures_tau_against_fake_loopback_delay` -/// above passes at the fake backend's default unity gain — exactly the one -/// case the old `is_loopback` ±2 dB gate already handled correctly, so it -/// cannot tell "measured because SNR is genuinely adequate" apart from -/// "measured because the gate was deleted" for any off-unity level. This -/// drives the +3.01 dB hot loopback from the issue's own rig case (drive -/// -30 dBFS, captured -30.0 dBFS) through `AC_FAKE_TAU_GAIN_OVERRIDE` and -/// asserts `measured` — a regression that reintroduced any captured-level -/// check keyed near unity would fail this without touching the muted-route -/// test. +/// #368 AC8 (QA request-changes on PR #384; codex-qa finding on the first +/// attempt at this test — see below). `calibrate_measures_tau_against_ +/// fake_loopback_delay` above passes at the fake backend's default unity +/// gain — exactly the one case the old `is_loopback` ±2 dB gate already +/// handled correctly, so it cannot tell "measured because SNR is genuinely +/// adequate" apart from "measured because the gate was deleted" for any +/// off-unity level. This drives the +3.01 dB hot loopback from the issue's +/// own rig case (drive -30 dBFS, captured -30.0 dBFS) through +/// `AC_FAKE_TAU_GAIN_OVERRIDE` and asserts `measured` — a regression that +/// reintroduced any captured-level check keyed near unity would fail this +/// without touching the muted-route test. +/// +/// codex-qa on PR #384 caught that the first version of this test asserted +/// only the final `tau_state`, never the off-unity level it claimed to +/// drive: `AC_FAKE_TAU_GAIN_OVERRIDE` at the time scaled only +/// `play_and_capture` (the τ ESS), not the step-2 tone capture +/// `capture_rms` reads — so step 2 still saw the unity-loopback level and +/// `measured` proved nothing about the off-unity path. Fixed at the +/// source (`audio/fake/mod.rs::capture_block` now applies the same +/// override) and pinned here: step 2's own `captured_dbfs`/`loopback` +/// fields are asserted before the final `tau_state` check, so a regression +/// in either the fake model or a reintroduced level gate fails this test. #[test] fn calibrate_measures_tau_on_hot_off_unity_fake_loopback() { let d = Daemon::spawn_with_env(&[ @@ -146,10 +157,26 @@ fn calibrate_measures_tau_on_hot_off_unity_fake_loopback() { "output_channel": 0, "input_channel": 0})); assert_eq!(r["ok"], json!(true)); - for step in 1..=2 { - expect_prompt(&c, step); - reply_vrms(&c, None); - } + expect_prompt(&c, 1); + reply_vrms(&c, None); + let step2 = expect_prompt(&c, 2); + // Unity loopback at ref_dbfs -30.0 would capture at -33.01 dBFS + // (the sine peak/RMS factor); the +3.01 dB override must land step 2 + // at -30.0, matching the issue's own hot-loopback rig case, and take + // it outside the old ±2 dB `is_loopback` window. + let captured_dbfs = step2["captured_dbfs"] + .as_f64() + .expect("captured_dbfs present on step 2"); + assert!( + (captured_dbfs - (-30.0)).abs() < 0.1, + "step 2 must see the +3.01 dB hot level (#368 AC1), not unity loopback: {step2}" + ); + assert_eq!( + step2["loopback"], + json!(false), + "3.01 dB off unity must fall outside the ±2 dB is_loopback window: {step2}" + ); + reply_vrms(&c, None); let done = expect_cal_done(&c); assert_eq!(