fix: propagate corrupt calibration errors

[mkovero]

closes #425

what changed

Calibration reads now happen synchronously before workers spawn, and a shared formatter returns the operator-facing refusal without converting read failures into missing calibration. Monitor receives preloaded per-channel calibration, transfer propagates the same failure across all requested pairs, and save-only preservation rationale remains attached to writes.

Regression coverage drives every affected command family against corrupt calibration data, covers all three plot entry points plus an unreadable regular file, and verifies the original store bytes remain unchanged.

files touched

• ac-rs/crates/ac-core/src/shared/calibration/store.rs — separate read error context from the save-specific preservation refusal
• ac-rs/crates/ac-daemon/src/handlers/mod.rs — add the shared calibration load/refusal formatter and handler guard
• ac-rs/crates/ac-daemon/src/handlers/audio/plot.rs — hoist plot, plot-level, and plot-IR calibration reads before worker creation
• ac-rs/crates/ac-daemon/src/handlers/audio/monitor/mod.rs — preload every monitored channel calibration before worker creation
• ac-rs/crates/ac-daemon/src/handlers/audio/monitor/channel.rs — accept preloaded calibration when constructing channel state
• ac-rs/crates/ac-daemon/src/handlers/test_dut.rs — refuse corrupt calibration before the DUT worker starts
• ac-rs/crates/ac-daemon/src/handlers/test_hw.rs — use one pre-spawn calibration read for both normal and DMM tests
• ac-rs/crates/ac-daemon/src/handlers/transfer/plan.rs — propagate calibration read failures while resolving the synchronous session plan
• ac-rs/crates/ac-daemon/tests/it_protocol/main.rs — register corrupt-calibration protocol regressions
• ac-rs/crates/ac-daemon/tests/it_protocol/corrupt_cal.rs — cover plot, monitor, transfer, DUT, hardware, unreadable input, and byte preservation
• ac-rs/ZMQ.md — document synchronous calibration-store refusal behavior

test output

cargo test -p ac-core shared::calibration::store::tests
13 passed; 0 failed

cargo test -p ac-daemon --test it_protocol
109 passed; 0 failed

cargo test -p ac-daemon --test it_zmq_doc_parity
5 passed; 0 failed

cargo clippy -- -D warnings
Finished successfully; 0 warnings

cargo fmt --check
Passed

ZMQ schema changed

no

new dependencies

none

related

none

open questions for reviewer

none

[mkovero]

spec coverage

criterion	provenance	covered	notes
A corrupt or unreadable calibration store fails every affected read operation with an observable error	measured	✓	All 8 discard sites (plot, plot_level, plot_ir, monitor per-channel, test_dut, test_hw's two reads collapsed to one, transfer planning) now go through load_calibration_or_refuse/cal_guard! and return synchronously before spawn_worker. Verified by reading each handler diff.
No affected operation substitutes an uncalibrated measurement after a calibration read error	measured	✓	Refusal happens pre-spawn in every site — no worker is created, so no partial/uncalibrated result can be published. transfer/plan.rs propagates the error before the reference-channel mic-curve guard runs, closing the exact bypass #425 named.
A corrupt calibration store cannot be overwritten from a blank in-memory entry	measured	✓	Already enforced at the store layer pre-PR (read_all_entries returns the parse error, save propagates it, write is temp+rename); this PR only splits the save-specific context out of the read-path context so ac plot's refusal doesn't carry a save-oriented sentence. Pre-existing test save_over_an_unparseable_file_refuses_instead_of_discarding_it still holds (ran green).
Regression coverage exercises plot, monitor, transfer, DUT, and hardware-test calibration reads	measured	✓	corrupt_cal.rs adds one test per family plus an unreadable-(non-UTF8)-file variant; all 6 pass.

standards conformance

standards check: not applicable — scope-none (architect corrected triage's tier-1 on #425; diff touches ac-daemon handlers and ac-core/shared/calibration store I/O, no ac-core/measurement/ or ac-core/visualize/ estimator code). Own read of the diff agrees with that routing — nothing here computes or gates against a standard.

correctness issues

none found.

Verified specifically:

• monitor/mod.rs: channel_cals is built from &channels (line 151) before channels_worker = channels.clone() (line 161) and in_ports_worker is built from the same channels order — the three vectors zip in matching order, so the per-channel calibration lands on the right channel. This was the one site the architect flagged as structural rather than mechanical, and it holds.
• test_hw.rs: the two reads at the old lines 101 and 150 (mic-curve-loaded flag and the DMM branch) collapse to one pre-spawn cal_ctx, used by both hw_dmm_absolute and hw_dmm_tracking.
• transfer/plan.rs: load_calibration_or_refuse runs and is propagated via ? before the per-pair reference-channel mic-curve check, so a corrupt store can no longer silently drop into "no calibration" and slip past that guard.
• Error field stays a JSON string (format! into String, not a JSON object) — confirmed all three consumers (ac-cli/src/commands/mod.rs:125, ac-view/src/zmq_client.rs:154, ac-view/src/session.rs:94, ac-view/src/snapshot_flow.rs:29) call .as_str() on reply["error"] and would silently degrade to "unknown error" on any other shape; none needed changes because none did.
• Message layout matches UX's proposed block exactly, including the 9-space continuation indent under error: (verified by counting).
• cargo test --workspace, cargo clippy --workspace -- -D warnings, cargo fmt --check all run clean in this worktree at the PR tip (1bb778d): 0 failed, 0 clippy warnings, fmt clean.

test coverage gaps

coverage is adequate. The new tests assert exact refusal wording (starts_with on the fixed prefix, contains on store/cause/data lines), byte-identical store preservation, and hit all 5 command families plus a non-UTF8 read-error variant — each assertion is reachable against the defect it names (pre-PR code took this same corrupt file and returned ok: true).

scope issues

none. Files touched match the architect's manifest exactly (tests/common/mod.rs correctly excluded — used read-only, not edited).

verdict

approve
All four acceptance criteria are covered with reachable, tightly-asserted tests, the structural monitor-ordering risk checks out, the wire contract is unchanged, and the full workspace gate (test/clippy/fmt) is green at the PR tip.

rig verification required

no

[mkovero]

codex qa — PR #439 at 1bb778d

verdict: fail

findings

[severity: major] [confidence: high]

• location: ac-rs/crates/ac-daemon/src/handlers/audio/monitor/mod.rs:104-115,151-156
• problem: A corrupt-calibration refusal leaves monitor_params.active set to true even though no monitor worker was started.
• mechanism: monitor_spectrum writes active: true before resolving ports and loading calibration; the new calibration-error return exits before spawn_worker, and only the worker teardown path resets the flag. set_monitor_params trusts this flag as proof that a monitor is running.
• failure scenario: Start monitor_spectrum with corrupt cal.json → receive the intended synchronous ok: false refusal → call set_monitor_params → receive ok: true and mutate parameters for a nonexistent worker instead of no active monitor.
• evidence: Opened monitor/mod.rs, server.rs, and handlers/admin.rs; ran cargo test -p ac-daemon --test it_protocol corrupt_cal::corrupt_cal_refuses_monitor_and_preserves_store -- --exact (1 passed); then launched the PR daemon with a corrupt store and observed the monitor refusal followed by set_monitor_params returning ok: true with the requested values.
• disproof attempted: Checked whether an early-return guard, worker registration, stop, or another cleanup path resets active; none does. The only reset is at monitor worker exit, which is unreachable because calibration refusal precedes spawn_worker. The runtime sequence reproduced the stale state.
• recommendation: Move the MonitorParams { active: true, ... } assignment until after all synchronous refusal points (including calibration loads), immediately before spawning the worker, and add a regression that a calibration-refused monitor leaves set_monitor_params returning no active monitor.

gate

Claude QA workspace gate: inherited at current tip 1bb778db
Codex targeted tests: cargo test -p ac-daemon --test it_protocol corrupt_cal::corrupt_cal_refuses_monitor_and_preserves_store -- --exact — passed (1 test); focused live CTRL probe — reproduced stale active state

unaddressed open questions

none

scope

none

[mkovero]

Revision pushed at a611241.

Design check: I revised against the architect decision updated 2026-09-03 04:06 UTC on #425. There is no architect or UX comment newer than the prior branch tip (1bb778db), so the design did not change under this implementation.

QA finding accepted. monitor_spectrum now sets monitor_params.active = true only after input-port resolution and every synchronous calibration load have succeeded, immediately before spawn_worker. A corrupt-calibration refusal therefore leaves the monitor inactive.

The existing corrupt-monitor protocol regression now follows the refusal with set_monitor_params and asserts ok: false plus the exact no active monitor error. This directly covers the reported stale-state sequence.

Verification:

• focused corrupt-monitor regression: 1 passed, 0 failed
• cargo test --workspace: passed
• cargo clippy --workspace -- -D warnings: passed with 0 warnings
• cargo fmt --check: passed

No review point is disputed.

[mkovero]

Re-review. Commit range reviewed: 1bb778db9525cfaab41200b1f136c88252f523fe..a611241d4fead17dbec5347770c58dce75508a73 (prior approval was at 1bb778db; new tip is a611241d, "fix: keep refused monitor inactive"). Prior full review stands for everything not discussed below — not re-litigated.

prior points — status

• All 4 spec-coverage criteria (refuse-on-corrupt, no uncalibrated substitution, no blank-overwrite, regression coverage across 5 families): still hold. Delta only touches monitor_spectrum's internal ordering and adds one assertion block to the existing monitor test; none of the four criteria regress.
• Monitor per-channel ordering (channel_cals/channels_worker/in_ports_worker zip): unaffected by this delta — the reorder moves the monitor_params write, not the channel/cal vectors.
• cargo test --workspace / clippy -D warnings / fmt --check: re-ran clean at a611241d (below).

delta review (1bb778db..a611241d)

Bug this commit fixes was real and outside what the prior pass checked: monitor_spectrum set state.monitor_params.active = true before resolving channels/ports and before the per-channel load_calibration_or_refuse loop. A corrupt-calibration refusal returned early with ok: false, but the daemon was left believing a monitor was active — set_monitor_params would then report success against a worker that was never spawned, and (per admin.rs:427) a second monitor request wouldn't be classified correctly either, since active was already true. This is exactly the kind of latent state corruption #425's fix was supposed to eliminate, just in the one handler that mutates shared state before its calibration guard.

Fix moves the monitor_params write (and monitor_params_shared clone) to after the channel_cals loop, so active is only ever set true once the worker is guaranteed to spawn. Read the reordered function in full (monitor/mod.rs:63-173) — confirmed monitor_params_shared has no other read/write site between the old and new position (only reads are inside the worker closure at lines 311 and 845, spawned after line 173), so the reorder has no other collateral effect within this function.

New test appended to corrupt_cal_refuses_monitor_and_preserves_store: after the refused monitor_spectrum call, it calls set_monitor_params and asserts ok: false / error: "no active monitor". This is reachable against the named defect — pre-fix, active would have been left true and this assertion would have failed (ok: true with echoed params). Traced "no active monitor" to its one production site (admin.rs:427) and to the pre-existing ZMQ.md:1332 doc line — unchanged by this PR, so no wire-schema delta here.

No files outside monitor/mod.rs and corrupt_cal.rs touched by this delta.

standards conformance

standards check: not applicable — delta is internal daemon state-ordering (active flag timing) and a protocol-level regression assertion, not a measurement value, output format, or display unit. Same routing as the first pass; nothing in this delta changes that.

correctness issues

none found.

test coverage gaps

coverage is adequate for the delta. One nit, not blocking: the new assertions live inside the corrupt-calibration test file, which is a reasonable place to prove the fix given the fix is one line away from the calibration-guard reorder, but the stale-active bug is reachable via other early-return paths in monitor_spectrum too (e.g. the interval/fft_n validation at lines 97-102 — those already run before the monitor_params write both before and after this delta, so they were never affected, but it's worth a developer note that this class of bug is "any early return after the write" rather than "this one guard" if more pre-spawn checks are added later). Not requesting a test for it now — no such guard exists after the write today.

suggested test additions

none — existing addition is sufficient for the defect it names.

scope issues

none.

verdict

approve
Delta fixes a genuine stale-active-flag bug left over from the ordering the first pass didn't check (the write predated calibration resolution), the new assertion is reachable against the defect and would have failed pre-fix, and the full workspace gate (cargo test --workspace, cargo clippy --workspace -- -D warnings, cargo fmt --check) is clean at a611241d.

sent back to

no

rig verification required

no

[mkovero]

Claude QA approval predates commit a611241d4fead17dbec5347770c58dce75508a73. A fresh Claude QA pass at the current tip is required before independent Codex QA review.

[mkovero]

codex qa — PR #439 at a611241

verdict: pass

gate

Claude QA workspace gate: inherited at current tip a611241d4fead17dbec5347770c58dce75508a73
Codex targeted tests: cargo test -p ac-daemon --test it_protocol corrupt_cal — passed (6 passed, 0 failed, 103 filtered out)

unaddressed open questions

none

scope

none

[mkovero]

qa — PR #439 at 14d5e44

Re-review. Commit range read for this pass: a611241d4fead17dbec5347770c58dce75508a73..14d5e4410bd3b9f467cd6f45733b70efdcea0032.

Note on the range boundary: my prior agent: qa comment (2026-09-03 04:35 UTC) was posted at 1bb778db, not a611241d — timestamps confirm a611241d (04:42 UTC) was pushed after that comment, in response to a codex-qa fail finding (stale monitor_params.active on a refused monitor) filed at 04:38 UTC. claude-approved was then re-applied at 04:47 UTC without a fresh agent: qa comment at a611241d — codex-qa correctly flagged that gap at 04:48 UTC. So a611241d's own change (fix: keep refused monitor inactive) has not previously appeared in one of my review comments; I reviewed it fresh in this pass alongside the merge delta, rather than treating it as already-accepted baseline.

spec coverage

criterion	provenance	covered	notes
A corrupt or unreadable calibration store fails every affected read operation with an observable error	measured	✓	Unchanged by the delta. Re-ran cargo test -p ac-daemon --test it_protocol corrupt_cal: 6/6 pass at the new tip.
No affected operation substitutes an uncalibrated measurement after a calibration read error	measured	✓	Unchanged; still verified pre-spawn_worker in every site (see correctness section for the ordering check against the newly-merged make_engine_for_state calls).
A corrupt calibration store cannot be overwritten from a blank in-memory entry	measured	✓	Unchanged — store-layer behavior not touched by the delta.
Regression coverage exercises plot, monitor, transfer, DUT, and hardware-test calibration reads	measured	✓	corrupt_cal.rs untouched by the delta itself (the file's last change was a611241d, adding the set_monitor_params follow-up assertion — see correctness #1). All 6 tests pass at the tip.

standards conformance

standards check: not applicable — scope-none (unchanged from prior pass). The delta adds a backend provenance field ("jack" | "cpal" | "fake") to report JSON, CSV headers, and wire frames — this is capture-source metadata, not a physical quantity, display unit, or standard-governed measurement value, so it doesn't newly trigger the standards check.

correctness issues

none found in the delta.

Verified specifically:

1. a611241d (fix: keep refused monitor inactive) is correct. Read the current monitor_spectrum (ac-rs/crates/ac-daemon/src/handlers/audio/monitor/mod.rs:63-176): the MonitorParams { active: true, .. } write now sits at line 166-175, after interval/fft_n validation (97-102), input-port resolution (108-134), the per-channel load_calibration_or_refuse loop (138-144), and make_engine_for_state (147-150) — i.e. after every synchronous refusal point, immediately before spawn_worker. The regression test (corrupt_cal_refuses_monitor_and_preserves_store, it_protocol/corrupt_cal.rs:87-108) follows the refused monitor_spectrum call with set_monitor_params and asserts ok: false / "no active monitor" — reachable against the exact defect codex reported (stale active: true surviving a calibration refusal), and it passes.
2. The merge (14d5e4410b) does not disturb the ordering the above fix or the original #425 guards depend on. The merge brings in an unrelated fail-closed audio-backend feature (issue Fail closed instead of silently substituting fake audio #424, already on main via PR docs(rig): move the verify queue out of tree to $AC_HOME #422/fix: fail closed on unavailable audio backend #438) that hoists make_engine(...) out of each worker closure into a fallible make_engine_for_state(state) call made synchronously in the handler, before spawn_worker. I checked every handler this touches that also carries a calibration guard:
   • monitor/mod.rs: make_engine_for_state (147) sits after the calibration loop (138-144) and before the active: true write (166) — consistent with point 1.
   • plot.rs (plot, plot_level, plot_ir): make_engine_for_state and cal_guard! both run before spawn_worker; order between them varies by function (engine-first in plot/plot_ir, cal-first in plot_level), but neither has a side effect before spawn_worker, so the order between two independent pre-spawn refusals doesn't affect the acceptance criterion — either one refuses before a worker exists.
   • test_dut.rs, test_hw.rs: same pattern; test_hw.rs's single collapsed cal_ctx = cal_guard!(out_ch, in_ch) (line 62, the site my prior review called out as the two-old-reads collapse) is untouched by the delta and still precedes spawn_worker.
   • transfer/plan.rs: probe_eng construction (now make_engine_for_state, line 138-139) precedes the per-pair load_calibration_or_refuse loop (line 166) — order changed from "cal-only" to "engine-then-cal", but both still return Err synchronously before any worker or session state is built, so the Propagate corrupt calibration errors instead of measuring uncalibrated #425 bypass this line specifically closes (a corrupt store silently dropping into "no calibration" past the reference-channel mic-curve guard) is still closed.
   • Ran cargo test -p ac-daemon --test it_protocol corrupt_cal -- --test-threads=1 directly (not just via the full-workspace run) to isolate this: 6 passed, 0 failed.
3. Wire-schema delta (report schema_version 5→6, additive backend field, new admin.status fields) is backward-compatible and unrelated to Propagate corrupt calibration errors instead of measuring uncalibrated #425. ac-rs/crates/ac-core/src/measurement/report/mod.rs:85 documents v6: optional capture backend; v1-v5 reports decode with it absent, and report/mod.rs keeps a legacy-v2-decode test that now also asserts r.backend == None (line ~239) — confirms old reports still decode. Field is #[serde(default, skip_serializing_if = "Option::is_none")]. No ac-cli/ac-view source files appear in this delta; grepped both crates for schema_version and found no runtime enforcement of "refuse unknown version" in either (only test-side literal assertions) — so removing that sentence from ZMQ.md in this delta corrects the doc to match code that was already this permissive, rather than dropping an enforced check. This predates the merge and is outside Propagate corrupt calibration errors instead of measuring uncalibrated #425's scope; not filing it against this PR.

test coverage gaps

coverage is adequate for the delta. a611241d added exactly the regression codex asked for (§ correctness #1) and it is reachable against the named defect. The merge's own tests (setup_validates_and_canonicalizes_backend_requirement, explicit_fake_is_available_and_constructed, unavailable_cpal_backend_fails_closed, persisted_plot_artifacts_identify_the_live_backend, status_replies_ok backend assertions) belong to issue #424 and are out of this review's scope, but I confirmed they pass as part of the full-workspace run.

scope issues

none against #425 itself. The 44-file / 852-line-removed delta is dominated by a merge of origin/main into the PR branch (commits 1fb21bb0..2fad1746, all already-merged main history from #422/#438 — an unrelated audio-backend/provenance feature for #424, plus the rig-verify-queue.md out-of-tree move). No conflict markers in the merge; git diff shows a clean union. This is branch hygiene (bringing the PR current before merge), not new implementation added for this ticket, so I'm not flagging it as scope creep — but it's why this "delta" is much larger than the two commits (1bb778db, a611241d) that are actually #425's own work.

verdict

approve
Full workspace gate is green at the tip (cargo test --workspace: 0 failed across all crates; cargo clippy --workspace -- -D warnings: 0 warnings; cargo fmt --check: clean); the codex-reported stale-active defect is fixed and regression-tested; the corrupt-calibration-refusal invariant this PR exists for is verified intact against every guard site after tracing the merge's reordering of engine construction relative to each calibration guard; and the merge itself introduces no PR-439-scoped correctness issue.

rig verification required

no

[mkovero]

codex qa — PR #439 at 14d5e44

verdict: pass

gate

Claude QA workspace gate: inherited at current tip 14d5e4410bd3b9f467cd6f45733b70efdcea0032
Codex targeted tests: not needed

unaddressed open questions

none

scope

none