Conversation
…ession delta Analyzer failures in the plot/plot_level point loops used to eprintln! and continue, then archive the completed prefix as a successful sweep. They now stop the engine, publish a terminal error frame carrying requested_points/completed_points, and return before any of frequency_response/complete, measurement/report, the report file, or done are emitted. xruns accounting switched from summing AudioEngine::xruns() (a cumulative since-start counter) once per point to a single wrapping-safe delta between a baseline taken at engine creation and a reading at sweep completion. Extended the fake backend with a capture_block xrun-injection hook (AC_FAKE_CAPTURE_BLOCK_XRUNS_OVERRIDE), kept independent from the existing play_and_capture-scoped AC_FAKE_XRUNS_OVERRIDE so the tau call-index contract used by existing calibrate tests is undisturbed. Closes #428. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mkovero
left a comment
There was a problem hiding this comment.
spec coverage
| criterion | provenance | covered | notes |
|---|---|---|---|
| An analyzer failure cannot produce an artifact represented as a complete successful sweep | n/a (non-numeric) | ✓ | plot.rs error arm returns before set_silence/report build/done; verified by reading the full function body — no path from the Err(e) arm reaches the report/done code. |
| The completion result preserves the distinction between requested and completed points | n/a (non-numeric) | ✓ | error frame carries requested_points/completed_points for both plot and plot_level. |
| Archived xrun count is the session delta, not a sum of repeated cumulative readings | n/a (non-numeric) | ✓ | xruns_start snapshot at engine creation, wrapping_sub at completion, replacing the old per-point xruns += eng.xruns() sum. |
| A duration-zero request that produces captures shorter than the 256-sample analyzer minimum cannot archive partial success | derived | ✓ | Gap closed with evidence in the PR itself: plot_duration_zero_fails_atomically_instead_of_archiving_partial_sweep derives the 562.5 Hz threshold from 3.0/freq * 48_000 == 256 in its own doc comment and asserts on error/no-done/no-measurement/report, completed_points>0, completed_points<requested_points, and the message containing "256". Traced thd::analyze's bail string (ac-core/src/measurement/thd.rs:84, "need at least 256 samples, got {n}") to confirm the substring assertion is real, not incidental. |
| A multi-point regression with one injected xrun reports exactly one session xrun | measured | ✓ | plot_reports_session_xrun_delta_not_cumulative_sum traced call-index-to-injection manually: with the bug (old per-point cumulative sum) present, the same fixture would total xruns=2 (double-counted after the injected point), not 1 — the test is reachable against the defect it names. |
standards conformance
standards check: not applicable — diff changes only sweep-termination control flow and session xrun bookkeeping (baseline+delta vs. per-point sum); no measurement value, output format, or unit-display code in thd.rs or elsewhere is touched. Checked docs/architecture/standards.md's document map and standard-by-standard checklist; none of AES-17, IEC 60268-3 §15.12.3.2, IEC 61260-1, IEC 61672-1, BS.468-4, BS.1770-5, or ISO 18233 govern sweep-abort/error-frame semantics or cumulative-counter bookkeeping. Label is tier-1 on the issue (correctly applied — plot.rs is a consumer of thd.rs's Tier 1 output) but the specific diff has no clause to check against.
correctness issues
none found.
Verified: eng.xruns() is documented "since start" on the AudioEngine trait (ac-daemon/src/audio/mod.rs:143) and both real backends (cpal_backend.rs:421, jack_backend.rs:549) read a monotonically-accumulating AtomicU32-backed counter — the baseline/wrapping-delta approach is correct against that contract, not just the fake backend. FakeEngine.xruns is u32 (fake/mod.rs:55), matching wrapping_sub's type.
Confirmed the atomic-failure return in both plot and plot_level sits inside the analyzer Err match arm, strictly before the loop-exit eng.set_silence()/eng.stop()/report-build code — no report file, measurement/frequency_response/complete, measurement/report, or done frame can be reached past it.
Confirmed wire-schema consumers: ac-cli/src/commands/plot.rs's collect_sweep reads only data["message"] off the error topic with unwrap_or, and never inspects xruns off anything but the done branch — the new additive requested_points/completed_points fields are ignored, not misparsed. ac-view has no consumer of the plot/plot_level/error topic at all (grepped session.rs, zmq_client.rs, snapshot_flow.rs — only an unrelated transfer_stream error read). Matches the PR body's claim.
test coverage gaps
coverage is adequate.
scope issues
none — all six touched files match the architect's file manifest on #428 exactly.
verdict
approve — atomic-failure control flow is correct by direct trace, xrun delta math matches all three backends' documented "since start" counter contract, both new regression tests are reachable against the defect they name with tight assertions, and the full workspace gate is green at this tip (cargo test --workspace: 0 failed across all crates; cargo clippy --workspace -- -D warnings: clean; cargo fmt --check: clean — all re-run locally against b58c81ca, not just taken from the PR body).
sent back to
no
rig verification required
no — no real-hardware value, no derived/assumed-tagged constant newly acted upon, and the fake backend's new capture_block xrun hook models exactly the field (FakeEngine.xruns) the regression test asserts on.
codex qa — PR #451 at b58c81cverdict: fail spec coverageThe daemon-side abort path, requested/completed counters, and session-delta xrun calculation cover their criteria. The derived 256-sample boundary was exercised against the 48 kHz fake backend by the duration-zero regression, and the measured one-xrun criterion was exercised by the injected multi-point regression. The end-to-end “no successful artifact after analyzer failure” criterion is not covered because the CLI still persists the streamed prefix after the new terminal error. findings[severity: major] [confidence: high]
gateClaude QA workspace gate: inherited at current tip
GitHub checks: none reported for the branch. unaddressed open questionsnone scopeThe six files changed match the architect manifest. The blocking behavior is in the existing |
codex qa — PR #451 at b58c81cverdict: fail findings[severity: major] [confidence: high]
gateClaude QA workspace gate: inherited at current tip unaddressed open questionsnone scopeThe six changed files match the architect manifest. The missing spec evidenceThe duration-zero numeric boundary is tagged |
Codex QA on PR #451 (#428) found that although the daemon now fails atomically on an analyzer error, the ac-cli consumer did not: both `ac plot` and `ac plot level` treated any nonempty collect_sweep() result as a completed sweep, so a terminal error arriving after some points had streamed still printed a normal-looking summary and wrote an unmarked CSV — an artifact that reads as a complete successful sweep, which is exactly what the issue's first acceptance criterion forbids. collect_sweep already tracked a SweepOutcome (Done/Failed) and both call sites already gated on `outcome == Done`; that half of the fix was already staged uncommitted in this worktree. This commit adds the regression Codex asked for: collect_sweep's frame loop is split out into collect_sweep_frames(), generic over the frame source, so the atomic-failure gating can be unit-tested directly (point frames then a terminal error must yield Failed and only the completed prefix) without standing up a real AcClient/socket or a daemon capable of producing the failure through the CLI's own argument surface (`ac plot` has no way to pass duration=0, the only case that currently forces a sub-256-sample capture). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
response to codex qa (2026-09-15T01:01:01Z, tip b58c81c)design check: re-read issue #428's triage/architect/ux comments — all three predate this branch's only commit (2026-09-14T23:04) and no newer design comment exists. No design drift; still building against the architect's option A (atomic sweep result) and the UX spec's "no successful summary/artifact after a partial failure." finding addressed — Fixed by:
why a unit test instead of a full e2e verify: labels: removed |
mkovero
left a comment
There was a problem hiding this comment.
Re-review. Commit range: b58c81ca..ce33e28e. Prior review (at b58c81ca) was approve; re-read below.
earlier review points — status
- Spec coverage row "analyzer failure cannot produce an artifact represented as a complete successful sweep" (marked ✓ at
b58c81ca) was verified only against the daemon's own frames/report file. It missed thatac-cli'scollect_sweepstill built and printed a summary table and wrote a CSV from the partialresultsit had collected, wheneverresultswas non-empty at the point of a terminalerror— a client-side artifact that itself read as a complete sweep. This delta closes that gap directly, by havingcollect_sweep/collect_sweep_framesreturn(results, SweepOutcome)and gating bothprint_summaryandsave_resultsinrun/run_levelonoutcome == SweepOutcome::Done. Confirmed by readingrun(plot.rs:68-73) andrun_level(plot.rs:140-145):outcome != SweepOutcome::Done || results.is_empty()returns before either call in both. - All four other spec-coverage rows (requested/completed distinction, xrun delta correctness, duration-zero atomic test, multi-point xrun regression) are untouched by this delta — nothing to re-verify, no regression: confirmed by diffstat (
git diff --stat b58c81ca..ce33e28e— one file,ac-cli/src/commands/plot.rs, no daemon-side files touched). - No correctness issues, test-coverage gaps, or scope issues were raised at
b58c81ca(verdict was clean approve) — nothing outstanding to check off here.
what changed this delta
collect_sweep/collect_sweep_frames now return a SweepOutcome (Done | Failed, default Failed) alongside results. Done is set only on the "done" topic branch immediately before break; every other exit path ("error" topic, timeout on None from next_frame) leaves it Failed. run/run_level gate print_summary/save_results on outcome == Done. The "error" branch also now appends (completed of requested points completed; no report written) to the printed message when requested_points/completed_points are present in the frame (both are u64-parsed via as_u64(), matching ZMQ.md's <int> typing at lines 1246/1304 — confirmed by reading those lines).
Side effect I want to flag as a positive, not a defect: this also fixes the timeout path, which had the identical bug — a timeout with a non-empty results previously fell through to print_summary/save_results exactly like the error case did, since the old gate was only results.is_empty(). That path is not named in #428's acceptance text but is the same defect shape and is fixed by the same gate.
correctness issues
none found.
Traced collect_sweep_frames's loop: outcome starts Failed, the only assignment to Done is on the "done" arm directly before its break, and that arm is unreachable after an "error" or timeout break in the same call (loop terminates via break, no continue/retry path back into a state where "done" could still be reached post-error). The "error" arm's partial string only renders when both requested_points and completed_points parse as u64; otherwise it's "", so a plain/legacy error frame prints unchanged (matches the daemon's other, non-#428 error paths that don't carry those fields).
standards conformance
standards check: not applicable — delta changes only the CLI's own outcome classification and error-message text (which points, if any, were captured before failure); it touches no measurement value, unit, or numeric display. print_summary/save_results (the code that does render frequency-response values) are unchanged by this delta, only gated on whether they run at all.
test coverage gaps
coverage is adequate. collect_sweep_frames is unit-tested directly (bypassing the socket via an injected FnMut() -> Option<(String, Value)> frame source) with two cases:
error_after_points_is_failed_not_done: 2 points then a terminalerrorwithrequested_points/completed_points, followed by adoneframe the loop must never reach. Assertsoutcome == Failedandresults.len() == 2. Reachable against the defect it names: revert the gate (outcome alwaysDone, or the oldresults.is_empty()-only check) and this fails — both the outcome assertion and, at therun/run_levelcall site, the CSV/summary would fire on 2 partial points.done_after_points_is_done: sanity check that the normal path still setsDone.
Both run in the real cargo test --workspace pass (confirmed present and passing: commands::plot::tests::done_after_points_is_done, commands::plot::tests::error_after_points_is_failed_not_done), not just claimed in the PR body.
suggested test additions
None required. Optional, not blocking: no unit test constructs a frame source that returns None first (immediate timeout, zero points) to confirm outcome stays Failed and results stays empty through that path too — the existing run/run_level gate already gets this right by construction (outcome defaults Failed), so this would be belt-and-suspenders, not a coverage gap.
scope issues
ac-cli/src/commands/plot.rs was outside the architect's file manifest for #428 per the PR body's own "open questions for reviewer" note (which called the CLI-rendering question out of scope). I don't read this delta as scope creep, though: the issue's acceptance text is "analyzer failure cannot produce a normal completed artifact," and the CLI's own summary print + CSV write is such an artifact, not a UX nicety layered on top of one. This is the same criterion the daemon-side commit already satisfied on its own side; this commit satisfies it on the client's. Worth a one-line note to the architect that the manifest under-scoped #428, but not a reason to hold the PR.
verdict
approve — the gate is correct by direct trace (single Done assignment point, unreachable after any failure exit), the new tests are reachable against the defect they name and both pass in a real cargo test --workspace run (not just the PR body's claim), and the full workspace gate is clean at ce33e28e: cargo test --workspace 0 failed (40/40 test result: ok blocks, including the two new tests, exit code checked directly — not through a truncating pipe), cargo clippy --workspace -- -D warnings clean, cargo fmt --check clean, all re-run locally against this tip.
sent back to
no
rig verification required
no — no hardware-dependent value, no derived/assumed constant newly acted on; this delta is pure client-side control flow over frames already covered by the daemon-side regression tests from the prior commit.
codex qa — PR #451 at ce33e28verdict: pass spec coverageAll five acceptance criteria are covered. Analyzer errors in both daemon sweep loops stop the engine, publish requested/completed counts, and return before report, archive, completion, or For the derived boundary, the separating measurement is the sample count actually delivered to standards conformanceStandards check: no changed measurement computation or unit/reference expression. I checked IEC 60268-3:2018 §15.11.1 against the frequency-response path; this PR changes failed-sweep termination and artifact gating, not the specified gain-frequency response calculation or presentation of successful results. gateClaude QA workspace gate: inherited at current tip unaddressed open questionsnone scopenone — the added |
Conflict in ac-daemon/src/audio/fake/mod.rs, import list only: this branch added hooks::next_capture_block_xruns_delta (#428 xrun injection), main (PR #384, #368) added hooks::tau_gain_override and tau_noise_amplitude_override. Resolved as the union. capture_block merged cleanly and keeps both changes: #428's xrun delta, then #368's gain scaling of the synthesized block. Also brings in PR #449, #384 and #450. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuCG3RuLHArnC8snRZVDYG
|
Merged |
mkovero
left a comment
There was a problem hiding this comment.
Re-review. Commit range: ce33e28e..0073162. Prior review (at ce33e28e) was approve; re-read above.
what this delta actually is
0073162 is Merge origin/main into issue-428 — parents ce33e28e (this PR's own tip) and 190c51b2 (main). git log --first-parent ce33e28e..0073162 shows exactly one commit in that range: the merge itself. No commit was authored on issue-428 between the two reviewed tips. Confirmed with git diff-tree -c on the merge — combined diff shows plain unions, no conflict-resolution hunks.
Everything in git diff --stat ce33e28e..0073162 (ZMQ.md, ac-cli/commands/calibrate.rs, ir_stats.rs, harmonics.rs, sweep/mod.rs, fake/hooks.rs, fake/mod.rs, handlers/calibrate/**, it_protocol/calibrate/tau.rs, it_protocol/modes.rs, scripts/ff400*.sh) traces to two already-merged, already-reviewed main PRs pulled in by this sync: 5e144e82 (PR #384, issue-368, τ pre-impulse SNR gating) and 190c51b2/20a787b5 (PR #450, issue-444, ff400.sh mixer-width fix, marked "codex-qa PR #450" in its own commit message). Confirmed via git log ce33e28e..190c51b2 --oneline.
Checked gh pr diff 451 (base-relative diff, what actually lands) against this two-dot diff: the base-relative diff lists only 7 files (ZMQ.md, ac-cli/commands/plot.rs, fake/hooks.rs, fake/mod.rs, handlers/audio/plot.rs, it_protocol/basics.rs, it_protocol/out_of_range.rs) — confirming the tau/harmonics/ff400 content is main's, not this PR's net contribution.
fake/hooks.rs and fake/mod.rs appear in both lists, so I checked those two directly rather than taking the "it's all main's" conclusion on faith: git diff ce33e28e..0073162 -- fake/hooks.rs fake/mod.rs shows only new tau_gain_override/tau_noise_amplitude_override hooks and their call sites in capture_block/play_and_capture — purely additive, applied via multiplication/addition to values next_xruns_delta/next_capture_block_xruns_delta (#428/#369's own hooks) don't touch. No interaction with this PR's xrun-delta hooks.
Confirmed zero diff on every file the prior two reviews actually verified: git diff ce33e28e..0073162 -- ac-cli/src/commands/plot.rs ac-daemon/src/handlers/audio/plot.rs ac-daemon/tests/it_protocol/basics.rs ac-daemon/tests/it_protocol/out_of_range.rs is empty. The atomic-failure control flow, xrun-delta math, and both regression tests reviewed previously are byte-identical at this tip.
earlier review points — status
- All spec-coverage rows, correctness findings, test-coverage assessment, and the "CLI-side artifact" fix from the
ce33e28ereview: unaffected, files untouched (see zero-diff check above). Nothing to re-verify. - No open points were carried from either prior review (both were clean approves with no outstanding gaps).
mechanical gate (workspace, at 0073162)
cargo test --workspace: 0 failed, 40test result: okblocks, checked directly against the log rather than a truncating pipe (grep -c "test result: FAILED"→ 0).cargo clippy --workspace -- -D warnings: clean.cargo fmt --check: clean.
standards conformance
standards check: not applicable to this delta — the only code this delta net-changes for issue-428 is nothing (zero diff, see above). The τ/SNR content it pulls in from main touches ISO 18233's IR-acquisition/SNR territory, but that content was authored and reviewed under its own PR (#384, issue-368) before this branch ever merged it; re-auditing it here would be re-reviewing a different, already-merged PR under this one's number. Flagging for the record, not blocking: if there's an unresolved standards question in #384's τ-SNR gating, it belongs on issue-368, not here.
correctness issues
none found.
test coverage gaps
coverage is adequate — no new #428-relevant code path exists in this delta to cover.
scope issues
none. The merge is a routine sync-with-main, not scope creep by this PR's author — no commit was authored on issue-428 in this range, and the branch's own files are untouched.
verdict
approve — mechanical gate is green at 0073162 (workspace test/clippy/fmt all clean), and the delta contains zero net change to any file this PR's own scope covers. Per the "approval voided by later push" rule, I re-ran the full gate and re-verified the diff rather than treating the prior approve as still current.
sent back to
no
rig verification required
no — unchanged from prior review; this delta adds no new hardware-dependent or derived/assumed value to issue-428's own code.
closes #428
what changed
plot/plot_level's point-loop analyzer failure previously logged to stderr and continued, then archived the completed subset as a normal successful sweep. It now stops the engine and publishes a terminalerrorframe carryingrequested_points/completed_points, and returns beforemeasurement/frequency_response/complete,measurement/report, the report file, ordonecan be emitted — an analyzer failure can no longer produce an artifact that reads as a complete sweep. Sessionxrunsaccounting switched from summingAudioEngine::xruns()(a cumulative since-start counter) once per point to a single wrapping-safe delta between a baseline taken at engine creation and a reading at sweep completion.files touched
ac-rs/crates/ac-daemon/src/handlers/audio/plot.rs— atomic failure exit on analyzer error in bothplotandplot_level; xrun baseline/delta instead of per-point cumulative sumac-rs/crates/ac-daemon/src/audio/fake/hooks.rs— newAC_FAKE_CAPTURE_BLOCK_XRUNS_OVERRIDEhook, kept independent of the existingplay_and_capture-scopedAC_FAKE_XRUNS_OVERRIDEso the tau call-index contract in existing calibrate tests is undisturbedac-rs/crates/ac-daemon/src/audio/fake/mod.rs—capture_blocknow consumes the new hook (inert when unset)ac-rs/crates/ac-daemon/tests/it_protocol/basics.rs— regression: one injected xrun mid-sweep reports exactly 1 in the terminaldone, not a per-point sumac-rs/crates/ac-daemon/tests/it_protocol/out_of_range.rs— regression:duration: 0(captures above 562.5 Hz fall under analyze's 256-sample minimum) fails atomically, never reachingdone/measurement/reportac-rs/ZMQ.md— documents the additiverequested_points/completed_pointsfields on the analyzer-failureerrorframe and the correctedxrunssession-delta semantics for both commandstest output
ZMQ schema changed
yes — additive-only.
plot/plot_levelanalyzer-failureerrorDATA frames gainrequested_points/completed_points. Successfuldone/measurement/frequency_response/complete/measurement/reportshapes are unchanged (samexrunsfield, now correctly computed). ac-cli and ac-view are unaffected: both already terminate on theerrortopic without inspecting extra fields.new dependencies
none
related
none
open questions for reviewer
The UX comment's proposed CLI terminal rendering (
error: plot failed after 6 of 31 points, session-qualifiedxrunsline, etc.) is a display concern inac-cli, which the architect's file manifest does not include — out of scope for this PR. The wire fields it depends on (requested_points/completed_points) are in place for a follow-up to consume.