Skip to content

docs(rig): one runbook and scripts for testing binaries on a rig - #441

Open
mkovero wants to merge 10 commits into
mainfrom
rig-testing-runbook
Open

mkovero wants to merge 10 commits into
mainfrom
rig-testing-runbook

Conversation

@mkovero

@mkovero mkovero commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Why

Testing ac binaries on real hardware was described in six places that disagreed with each other and with the code:

  • .agents/rig.md hardcoded the Babyface box at 192.168.9.25 (its ALSA numids, its clock rule) as the default rig.
  • docs/runbooks/loopback-ir.md, docs/runbooks/rig-headless.md and ir_probe all said plot_ir ignores drive_max_dbfs. That has been false since drive_max_dbfs governs set_drive alone — plot_ir and calibrate emit unclamped, and calibrate defaults to -10 dBFS #360 (handlers/audio/plot.rs clamps it).
  • TESTING.md and it_loopback_ir.rs (4 places) pointed at an ARCHITECTURE.md "Loopback IR runbook" section that no longer exists.
  • README said install.sh installs only ac and ac-daemon; it also installs ac-view.
  • No script covered building portable binaries, shipping them, or checking a rig's state, so every session rebuilt that by hand.

What

what where
the one procedure docs/runbooks/rig-testing.md (absorbs the two old runbooks)
interlocks + record fields .agents/rig.md (unchanged rules; pre-flight now via scripts, pupu default)
scripts scripts/rig/
per-rig facts docs/rigs/pupu.md
per-rig machine profile scripts/rig/hosts/pupu.env
rig access (address, user, SSH key) $AC_HOME/rig-hosts/<rig>.access.env — private, never in the repo
Babyface audio setup docs/superseded/rig-babyface-audio.md (192.168.9.25 stays for ac-view GPU snapshots)

Scripts, silent unless marked:

  • build-portable.sh: build for target-cpu=x86-64 with one target dir per commit, staged under $AC_HOME/target-rig-stage/<rev>/ with MANIFEST.txt + SHA256SUMS. It refuses a dirty ac-rs/ unless --allow-dirty.
  • ship.sh <rig> [rev] [--install]: copy, verify sha256 on the rig, link it_loopback_ir's compile-time daemon path to the shipped daemon. --install stops ac-daemon first and verifies the installed hashes.
  • preflight.sh <rig> [rev]: JACK service/rate/period/flags, where the analog capture block sits (silently — the FF400's port order moves, scripts/ff400.sh hardcodes one FF400 port order; snd_fireface puts the ADAT block first or last #444), the interface's ALSA baseline, ac config ceiling and channel map, staged hashes, recent xruns. Output is a record block; exit 1 on FAIL.
  • noise-snapshot.sh: mic broadband + band levels.
  • xrun-soak.sh: capture-only ac monitor --tui load, both xrun counters.
  • EMITS probe-outputs.sh, run-loopback-ir.sh, acoustic-ir.sh: each requires --consent "<text>", refuses a level above the profile ceiling, and refuses to run when the analog capture block is not where the profile says. pupu uses −40 dBFS standing and −50 dBFS on anything driving the speaker (operator's call after a −40 sweep was too loud). The speaker ceiling is script-enforced only, because the daemon's drive_max_dbfs can't tell outputs apart.

Source changes are comments and panic-message text only: it_loopback_ir.rs, ir_probe.rs.

Traps the scripts encode (each one bit during this work):

  • ac finds ac-daemon on PATH before its own directory (spawn.rs). A staged ac would silently auto-spawn the installed daemon, so scripts prefix PATH and print /proc/<pid>/exe.
  • cargo build --examples alone skips the binaries.
  • A remote script fed to ssh … bash -s is swallowed by anything reading stdin (script, the TUI). The first xrun soak exited 0 having run nothing. Scripts now use ssh -n … bash -c.
  • A stage dir named anything but target-* would be tracked by $AC_HOME's git.
  • The FF400's JACK port order is not stable: the ADAT/S/PDIF block comes first on some boots and last on others (scripts/ff400.sh hardcodes one FF400 port order; snd_fireface puts the ADAT block first or last #444), and the port count changes with rate (18 capture ports at 48 kHz, 14 at 96 kHz). Unconnected ADAT/S/PDIF captures read exact digital zero while analog inputs never do, so a silent 1 s capture locates the analog block (scripts/rig/lib/port_order.py).

Verification

Development host, branch tip:

  • cargo test --workspace: exit 0, 0 failed.
  • cargo clippy --workspace --all-targets -- -D warnings: exit 0.
  • cargo fmt --check: exit 0.
  • shellcheck -x -S warning scripts/rig/*.sh: clean.

On pupu (FF400, 96 kHz / 256 / 3, jackd -S), with the operator's per-run consent for every emitting run:

step result
build-portable.sh (main 0891cf9) 5 binaries staged, compiled_this_run=yes, embedded daemon path checked
ship.sh pupu sha256 OK on the rig for all 5; daemon link resolves to the shipped daemon
preflight.sh pupu latest all PASS (22 checks; rerun after the port-order commit: 23, port order live 1-8 of 14)
noise-snapshot.sh pupu 5 −67.1 dBFS broadband
xrun-soak.sh pupu 20 staged daemon confirmed by /proc/<pid>/exe, 2 JACK ports, jackd xruns 0, daemon counter 0
probe-outputs.sh --level -60 AN1 → mic −65.7 dBFS; AN2 → IN2 −57.5 dBFS (wiring matches profile)
run-loopback-ir.sh --level -40 --route ref pass: peak +17.99 ms, SNR 32.8 dB
acoustic-ir.sh --level -40 peak +22.92 ms, SNR 25.6 dB (run before the −50 speaker ceiling existed)
closing preflight.sh pupu none all PASS, no daemon left running, config channels restored
port-order gate, profile expecting 11-18 (simulated moved block) refused with "MOVED", nothing emitted
port_order.py synthetic captures analog-first passes; ADAT-first, stray live ADAT channel, dead analog input all fail

Not verified / will not fix

  • The binaries exercised on the rig are main 0891cf9, before this PR's ac-rs/ edits. Those edits are comments and panic text only, but the rig never ran this tip's build.
  • ship.sh --install and build-portable.sh --allow-dirty were not exercised. Nothing was installed to /usr/local/bin on pupu.
  • The −50 speaker-ceiling refusal was checked locally (−45 refused, −50 and −40 accepted), not re-run against the rig.
  • transfer_probe has no wrapper. The runbook gives the manual command, and nothing enforces the speaker ceiling on that path.
  • Only the analog-first order occurred during this session, so the port-order refusal was exercised against a simulated profile, not a real moved block. The playback order is assumed to follow the capture order (it has whenever both were checked).
  • The emission runs in the table predate the port-order gate; the gate itself was verified silently afterwards.
  • Not changed here, filed separately:
  • Layout deviation from the plan: per-rig profiles live in docs/rigs/, not a root-level rig/, per CLAUDE.md's "root holds entry points only".

🤖 Generated with Claude Code

https://claude.ai/code/session_01TmxMmDBeKtES1YyvE5dyDK

mkovero and others added 2 commits September 14, 2026 19:29
Rig testing was described in six places that disagreed: .agents/rig.md
hardcoded the Babyface box, both runbooks said plot_ir ignores
drive_max_dbfs (untrue since #360), and TESTING.md and it_loopback_ir.rs
pointed at an ARCHITECTURE.md section that no longer exists. No script
covered building portable binaries, shipping them, or checking a rig.

- docs/runbooks/rig-testing.md: the one procedure; absorbs loopback-ir.md
  and rig-headless.md.
- scripts/rig/: build-portable, ship (sha256 verified on the rig),
  preflight, noise-snapshot, probe-outputs, run-loopback-ir, acoustic-ir,
  xrun-soak and a record template; per-rig profile in hosts/pupu.env,
  including a -50 dBFS ceiling for anything that drives the speaker.
- docs/rigs/pupu.md: facts for the dedicated rig, now the
  default. The Babyface audio setup moves to docs/superseded/;
  192.168.9.25 stays for the ac-view GPU snapshot tests.
- .agents/rig.md runs pre-flight through the scripts; qa.md, TESTING.md
  and README point at the runbook.
- it_loopback_ir.rs, ir_probe.rs: fix the stale pointers and the
  drive-ceiling text (comments and panic messages only).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TmxMmDBeKtES1YyvE5dyDK
The Fireface 400's channel order under snd_fireface is not stable: the
ADAT/S/PDIF block sometimes precedes the analog block and sometimes
follows it (#444). The pupu profile names ports and ac indices for the
analog-first order, so on a boot with the other order every emitting
script would have driven an ADAT port while reporting the profile's names.

Unconnected ADAT/S/PDIF captures read exact digital zero and analog
inputs never do, so a silent one-second capture of every system capture
port shows where the analog block sits (scripts/rig/lib/port_order.py).
preflight.sh reports it; probe-outputs, run-loopback-ir and acoustic-ir
refuse to run when the block is not at RIG_ANALOG_CAPTURE_FIRST.

Also corrects docs/rigs/pupu.md, which stated analog-first as a fixed
fact, and records that JACK exposes 18 capture ports at 48 kHz but 14 at
96 kHz.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TmxMmDBeKtES1YyvE5dyDK
@mkovero

mkovero commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

PR #445 (closes #442) is open against main, ahead of this branch — so if it merges before this one, three passages here will state a gap #445 has already closed:

  1. docs/runbooks/rig-testing.md — the -10 isolated-HOME fallback described there is fixed on the real-port route; it now carries drive_max_dbfs = -40.0 written by it_loopback_ir.rs's own config builder.
  2. scripts/rig/run-loopback-ir.sh header comment (lines ~13-15) — the --level check is still correctly described as a convenience, but framing it as "the thing holding the rig ceiling" will no longer be accurate; the interlock becomes server-side, in the daemon's own config.
  3. ac-rs/crates/ac-daemon/tests/it_loopback_ir.rs's doc comments and Routing::new's (formerly from_env's) mandatory-level panic message — both rewritten on fix: real-port loopback_ir daemon config carries the rig's -40 dBFS ceiling #445's branch; a rebase past it will surface the conflict/duplication here.

docs/runbooks/loopback-ir.md (deleted on this branch) had its "only limit on what reaches the converter" paragraph updated by #445 first — no extra action needed there beyond the deletion going through as planned.

If this PR merges first instead, the correction runs the other way: #445 already carries a comment noting it'll pick up the fix on rebase.

@mkovero

mkovero commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

docs/rigs/pupu.md: FF400 reset claim contradicted on the rig, 2026-09-15 ~00:35 EEST

docs/rigs/pupu.md lines 59–68 say a power cycle resets the FF400 to driver defaults and that "JACK restarts and rate changes do not". One sudo systemctl restart jack-ac.service on pupu contradicted both halves of the restore guidance.

Measured after the restart, same cabling, same probe-outputs.sh pupu --level -60 --outputs 1:

before (19:49Z) after JACK restart after documented restore after toggle restore
IN2 (loopback) 1 kHz tone −57.5 dBFS −51.5 −51.5 −57.5
IN1 (mic, phantom) rms −73.9 −106.2 (dead floor) −105.3 −73.2
amixer cget 93/94/89/81/90 2/2/2/20,0/on,off 2/2/2/20,0/on,off same same

So after the restart the device ran at driver defaults, about +6 dB loopback gain with phantom off, while ALSA readback still showed the baseline. preflight.sh's ALSA rows passed throughout.

The documented restore block doesn't restore the device. Writing the value the driver already holds seems to be skipped. Only writing a different value first, then the baseline, restored both readings:

C=Fireface400
amixer -c $C cset numid=93 1; amixer -c $C cset numid=94 1; amixer -c $C cset numid=89 1
amixer -c $C cset numid=93 2; amixer -c $C cset numid=94 2; amixer -c $C cset numid=89 2
amixer -c $C cset numid=81 0,0; amixer -c $C cset numid=90 off,off; sleep 1
amixer -c $C cset numid=90 on,off; sleep 2
amixer -c $C cset numid=81 20,0

This was one restart, so it doesn't say whether every JACK restart does this, or only this one. What the doc and preflight should not rely on:

  • ALSA readback as evidence of FF400 device state after a restart or power cycle.
  • The same-value restore block.

A check that can fail on this is a −60 dBFS probe of output 1 expecting IN2 ≈ −57.5 dBFS. Preflight's ALSA rows cannot fail on it.

Why the restart happened: jackd's socket /dev/shm/jack_default_1000_0 had been unlinked while jackd kept running. mui has Linger=no with logind RemoveIPC at its default, so the socket went when the last login session closed. Every client, including preflight, then reads JACK as absent. Host-side fix (lingering or RemoveIPC=no) belongs in the private sys notes, not this repo.

Evidence: rig record on #445, #445.

Brings in #445 (#442: it_loopback_ir's real-port daemon config carries
drive_max_dbfs -40) and #388. Conflicts resolved:
- it_loopback_ir.rs: take main's module doc and expect message, which
  describe the now-closed isolated-HOME ceiling gap correctly; this
  branch's doc-pointer edits (rig-testing.md, pupu port examples) kept.
- docs/runbooks/loopback-ir.md: keep this branch's deletion; its #445
  paragraph is carried into rig-testing.md step 7c.

Corrects the passages #442's architect note named for this branch:
- rig-testing.md steps 5 and 7c, run-loopback-ir.sh header: the test's
  isolated-HOME daemon now clamps at -40 dBFS; the script's --level check
  is a convenience, and the only enforcement of the speaker ceiling.

Adds rig findings from 2026-09-14/15 on pupu:
- docs/rigs/pupu.md: a jack-ac.service restart reset the FF400 to driver
  defaults while ALSA readback kept the baseline; the same-value restore
  did nothing, toggle writes restored it, verified by a -60 dBFS probe.
- rig-testing.md step 3: preflight's ALSA rows read the driver cache, not
  the interface.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FuCG3RuLHArnC8snRZVDYG
@mkovero mkovero added needs-work QA requested changes — PR not ready to merge requires-rig labels Sep 14, 2026
@mkovero

mkovero commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

spec coverage

No issue is linked to this PR (closingIssuesReferences empty; no Fixes #/Closes #). Checked #330 (the rig-protocol issue this work descends from) — closed 2026-08-17, a month before this PR, so it is not this PR's spec either. Treating the PR body's own "What" table and "Verification" section as the spec, since that is the only acceptance criteria on record. This is itself a triage gap worth naming to triage (see below), not a reason to skip the check.

criterion provenance covered notes
one procedure doc absorbs loopback-ir.md + rig-headless.md n/a docs/runbooks/rig-testing.md present, both old files deleted, content (transfer_probe/ir_probe invocation, drive-ceiling text) carried forward into 7c/7d/7e
.agents/rig.md interlocks unchanged, pre-flight now via scripts n/a hard constraints section textually unchanged (−40 dBFS standing ceiling, sha256-only verification, stop-before-install, confound-required); step 1 now points at build-portable.sh/ship.sh/preflight.sh
scripts listed in PR body exist and match described behavior n/a all 9 scripts + 3 python helpers present; read each, behavior matches description (see correctness section)
per-rig facts doc (docs/rigs/pupu.md) n/a present, internally consistent with hosts/pupu.env
Babyface setup superseded, 192.168.9.25 kept for GPU snapshots n/a ✓ (content not reviewed) docs/superseded/rig-babyface-audio.md is denied to Read by this repo's own .claude/settings.json; confirmed via diff stat only (38 lines added, archival move). TESTING.md/rig-testing.md still point 192.168.9.25 at the A3 snapshot policy, consistent.
README: install.sh installs ac, ac-daemon, ac-view n/a verified against ac-rs/install.sh directly — matches exactly
source changes are comments/panic text only n/a diffed ir_probe.rs/it_loopback_ir.rs against origin/main — no logic changed, only doc comments, panic message, and one #[ignore = "..."] string
plot_ir clamps to drive_max_dbfs (#360), cited in the rewritten comments measured apply_drive_ceiling(cfg.drive_max_dbfs, level_dbfs) present in ac-daemon/src/handlers/audio/plot.rs:52,646 — comment now describes actual code
RIG_SPEAKER_CEILING_DBFS = -50 dBFS (new constant, pupu) assumed (untagged numeric) partial Value comes from one operator's real-time loudness judgment during the 2026-09-14 session ("a −40 dBFS sweep through the 1083 is too loud"), not from any SPL reference or hearing-safety threshold. Separating measurement: an SPL/dBA reading at the listening position during a −50 dBFS sweep, compared against a stated exposure limit — nothing like that exists yet. Not blocking: the value is more conservative than the pre-existing −40 baseline, is honestly documented as a judgment call rather than a derivation, and is cited to an actual in-session operator decision on 2026-09-14 (docs/rigs/pupu.md, PR body) — that citation is the evidence the withholding rule asks for. Still: the number itself has never been checked against anything but one ear.
port-order gate refuses emission when the FF400's ADAT/analog block has moved assumed for the real case gap PR body states plainly: "the port-order refusal was exercised against a simulated profile, not a real moved block" — the FF400 never actually booted ADAT-first during this session. require_level/require_port_order logic reads correctly (below), but the thing it is meant to catch has not once been observed live. See requires-rig.

standards conformance

standard clause check result

standards check: not applicable — diff touches .agents/, docs/runbooks/, README.md, TESTING.md, scripts/rig/* (bash/python rig tooling), and two Rust files where only comments and a panic string changed. No docs/architecture/standards.md document-map module (thd.rs, filterbank.rs, weighting.rs, noise.rs, reference_levels.rs, ccir468.rs, loudness/, sweep/mod.rs) is touched, and nothing here implements a rule one of those modules defines. No linked issue carries a scope label either way, so there is no tier-1 to raise past — the diff itself is the basis for this call. scripts/rig/lib/{chan_levels,bands}.py do compute levels (Goertzel tone amplitude, RBJ octave bands) but are rig wiring/noise diagnostics, not a shipped measurement path any standard governs; I still checked their math by hand under numerical correctness below.

correctness issues

  1. scripts/rig/preflight.sh:97 and scripts/rig/xrun-soak.sh:43 — the jackd-xrun-log check can silently read PASS when journalctl itself fails, not just when there are no xruns. Both lines pipe journalctl ... 2>/dev/null | grep -ciE 'jackd.*xrun' into a variable and never check journalctl's own exit status — only grep -c's count ends up in the variable, and neither script has set -o pipefail. Concretely: if the SSH account used by these scripts is not in systemd-journal/adm on a rig (or the rig's jackd doesn't log through journald at all), journalctl exits non-zero and writes its error to the now-discarded stderr; stdout is empty; grep -c on empty input prints 0; the script reports "jackd xrun log lines: 0" / exits 0 — indistinguishable from a genuinely clean 10-minute window or 60 s soak. This is exactly the failure mode preflight.sh's own docstring promises not to have ("Exit 1 on any FAIL line") and the one this PR's other checks (sha256, port order) are careful to avoid — a check that cannot go red is not a check. The PR's rig-verification table reports these rows as passing on pupu, but nothing in it confirms the SSH user's journalctl read permission was ever exercised as a negative case, so this can't be told apart from "0 xruns" in the evidence given. Fix: check journalctl's exit status explicitly (or set -o pipefail and branch on PIPESTATUS[0]) and report a distinct FAIL/INFO row ("xrun log unreadable") rather than folding a tool failure into the count.

Everything else checked out:

  • require_level/speaker_ceiling (lib.sh:124-136) — sign convention correct: level <= ceiling on negative dBFS values refuses anything louder (less negative) than the ceiling; verified against both routes in probe-outputs.sh, run-loopback-ir.sh, acoustic-ir.sh.
  • port_order.py — live/expected span comparison is correct; the MOVED/dead-input/extra-input branches are mutually exclusive and exhaustive over the possible mismatch shapes.
  • chan_levels.py's Goertzel tone amplitude (amp = 2*sqrt(q1²+q2²−k·q1·q2)/m/full) is the standard non-bin-aligned Goertzel magnitude, correct regardless of whether the captured window holds an integer number of cycles (the boundary error is O(1), not O(N), and N here is tens of thousands of samples) — no leakage bias at the levels this tool reports to.
  • bands.py's RBJ constant-skirt-gain bandpass coefficients and difference equation match the RBJ cookbook exactly, including the b1 = 0 omission.
  • db() in both python helpers uses 20*log10, correct for amplitude (not power) ratios.
  • ir_probe.rs/it_loopback_ir.rs comment rewrites were checked against the actual code they describe (plot.rs's apply_drive_ceiling call sites, Routing::new's drive_max_dbfs: Some(RIG_DRIVE_CEILING_DBFS)) rather than taken on faith.
  • ship.sh's --install sha256 grep (' (ac|ac-daemon)$') is correctly anchored — cannot false-match ir_probe/transfer_probe/it_loopback_ir lines.
  • rig_bash's ssh -n ... bash -c "$(printf %q ...)" correctly avoids the stdin-swallowing trap the commit message calls out (contrast noise-snapshot.sh, which deliberately uses plain rig_ssh with an open stdin to pipe bands.py's own source in — a different, correct use of stdin, not the trap).

test coverage gaps

  • require_level/speaker_ceiling (scripts/rig/lib.sh) gate every emitting path and are exactly the kind of coupled constant this project's own history flags (two thresholds, -40 standing and -50 speaker, that only work together with nothing in the repo recording the coupling). The PR body says the -45/-50/-40 boundary was "checked locally... not re-run against the rig" — meaning even the one-off manual check left no artifact. This is pure arithmetic with no hardware dependency and nothing stops a small regression test for it.
  • chan_levels.py's Goertzel and bands.py's RBJ bandpass were checked by hand in this review; neither has a synthetic-signal regression test (e.g., a generated sine at a known dBFS piped through chan_levels.py, asserting the reported tone level within a tight tolerance). Both are pure-stdlib and could be tested without any hardware or JACK.

suggested test additions

# scripts/rig/lib_test.sh — no rig required, run with: bash scripts/rig/lib_test.sh
source "$(dirname "$0")/lib.sh"
RIG_DRIVE_CEILING_DBFS=-40 RIG_SPEAKER_CEILING_DBFS=-50

assert_refuses() { require_level "$1" "$2" 2>/dev/null && { echo "FAIL: $1 should be refused at ceiling $2"; exit 1; }; }
assert_allows()  { require_level "$1" "$2" || { echo "FAIL: $1 should be allowed at ceiling $2"; exit 1; }; }

assert_allows  -40 "$RIG_DRIVE_CEILING_DBFS"      # at the standing ceiling
assert_refuses -39 "$RIG_DRIVE_CEILING_DBFS"      # 1 dB over
assert_allows  -50 "$(speaker_ceiling)"           # at the speaker ceiling
assert_refuses -45 "$(speaker_ceiling)"           # between the two ceilings — the case #445/this PR actually cares about
assert_allows  -60 "$(speaker_ceiling)"           # well under
echo "lib.sh require_level: all cases as expected"
# scripts/rig/lib/chan_levels_test.py — no rig required
import math, struct, subprocess, sys, tempfile, wave

fr, dur, f0, level_dbfs = 48000, 2.0, 1000.0, -20.0
amp = 10 ** (level_dbfs / 20) * (1 << 23)
n = int(fr * dur)
with tempfile.NamedTemporaryFile(suffix=".wav") as f:
    w = wave.open(f.name, "wb")
    w.setnchannels(1); w.setsampwidth(3); w.setframerate(fr)
    w.writeframes(b"".join(
        int(amp * math.sin(2 * math.pi * f0 * i / fr)).to_bytes(3, "little", signed=True)
        for i in range(n)
    ))
    w.close()
    out = subprocess.run(["python3", "lib/chan_levels.py", f.name, str(f0)],
                          capture_output=True, text=True, check=True).stdout
    reported = float(out.strip().splitlines()[-1].split("|")[4])
    assert abs(reported - level_dbfs) < 0.05, f"got {reported}, want {level_dbfs}"
print("chan_levels.py: tone level within 0.05 dB of the synthetic input")

scope issues

none.

verdict

request-changes
journalctl's exit status is never checked in preflight.sh and xrun-soak.sh, so a permission or journald-absence failure on the SSH side reads as "0 xruns" / PASS — the one thing this PR's own design principle (checks that can actually fail) says a rig script must not do; everything else reviewed (script logic, sign conventions, Goertzel/RBJ math, comment-only Rust changes, cargo test --workspace/clippy/fmt/shellcheck all clean) holds up.

sent back to

no

rig verification required

Two items, for $AC_HOME/rig-verify-queue.md (new block — nothing existing covers this):

  1. Exercise the port-order gate against a genuinely ADAT-first-booted pupu (not the simulated profile used this session) — confirm preflight.sh reports MOVED and exits 1, and that probe-outputs.sh/run-loopback-ir.sh/acoustic-ir.sh all refuse via require_port_order with nothing emitted. Falsified by any of those three scripts proceeding to emit, or by preflight.sh passing, while the block is confirmed moved by manual jack_lsp/jack_rec inspection.
  2. Run ship.sh --install and build-portable.sh --allow-dirty at least once end-to-end on pupu — neither has been exercised on real hardware per the PR's own "Not verified" section. Falsified by either command failing, or by the post-install sha256 check passing against a binary that doesn't match what was actually installed.

…on tests

preflight.sh and xrun-soak.sh piped journalctl straight into grep -c, so a
journalctl failure (no journald read permission on the rig's SSH account,
or jackd not logging through journald at all) produced empty stdout, which
grep -c reported as 0 — indistinguishable from a genuinely clean window.
Both scripts now check journalctl's own exit status and report a distinct
FAIL / 'unreadable' value instead of folding a tool failure into the count.

Adds two no-rig-required regression tests raised in QA review:
- scripts/rig/lib_test.sh: require_level/speaker_ceiling boundary cases,
  including the -45 dBFS case between the two coupled ceilings.
- scripts/rig/lib/chan_levels_test.py: a synthetic -20 dBFS tone through
  chan_levels.py's Goertzel path, asserting the reported level within
  0.05 dB.

The QA-suggested lib_test.sh snippet used require_level's exit status
directly inside an && list; under lib.sh's set -e that does not work,
because require_level dies with a hard exit() on refusal, which is
unconditional and not exempted by && the way a plain nonzero return would
be — so the refusal case silently killed the whole test script instead of
reaching the FAIL branch. Fixed by running require_level in a subshell.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mkovero

mkovero commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

design check

No issue is linked to this PR (closingIssuesReferences empty, same as QA found). No architect/ux comment to supersede against — nothing to revise the design to.

correctness issue — journalctl exit status (fixed)

Confirmed as filed: preflight.sh:97 and xrun-soak.sh:43 piped journalctl straight into grep -c with no pipefail, so a journalctl failure (permission, or jackd not logging through journald) read as 0 xruns — indistinguishable from clean. Fixed both to check journalctl's own exit status via if journalctl ...; then grep -c <<<"$out"; else <FAIL row / log=unreadable>; fi rather than adding set -o pipefail/PIPESTATUS (simpler, and this repo's set -u-only remote heredocs don't otherwise use pipefail). preflight.sh now emits a distinct FAIL row ("journalctl unreadable, not 0: ..."); xrun-soak.sh prints jackd xrun log lines: unreadable and its final pass/fail check no longer reads that as 0.

test coverage gaps — added, with one fix to the suggested snippet

Added scripts/rig/lib_test.sh and scripts/rig/lib/chan_levels_test.py, both runnable with no rig. Both pass locally; shellcheck -x -S warning scripts/rig/*.sh stays clean.

One correction to the suggested lib_test.sh: require_level "$1" "$2" 2>/dev/null && { echo FAIL; exit 1; } doesn't work under lib.sh's set -erequire_level refuses by calling die(), which does a hard exit, and exit is unconditional; it isn't exempted from &&/|| the way a plain nonzero return is. So the refusal case (the one we want to assert on) killed the whole test script silently before reaching the FAIL branch — I checked this by running the suggested version first, it exits 1 with no output on a passing run. Fixed by running require_level inside a subshell, so its exit only ends the subshell. Left a comment in the file explaining why.

rig verification required — queued, not run

No rig access in this session. Added both items as a new dated block in $AC_HOME/rig-verify-queue.md ("Queued 2026-09-14 — PR #441 rig scripts (not yet run)"), same pass/fail wording as the QA comment, committed there (own repo, per CLAUDE.md). Not run — that stays true to the PR's own "Not verified" section; this just gives it a queue entry instead of leaving it unrecorded.

everything else in the review

No disagreement with anything else raised (spec-coverage table, standards conformance, the sign-convention/Goertzel/RBJ correctness checks, scope). Nothing else required a code change.

verify

shellcheck -x -S warning scripts/rig/*.sh: clean. bash scripts/rig/lib_test.sh: pass. python3 scripts/rig/lib/chan_levels_test.py: pass (reported −20.00 dBFS vs synthetic −20 dBFS). No Rust changed, so cargo test/clippy/fmt results are unchanged from the PR body.

@mkovero mkovero added in-review QA reviewed, awaiting human merge and removed needs-work QA requested changes — PR not ready to merge labels Sep 14, 2026
@mkovero

mkovero commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Re-review. Commit range: fd566ff..074da85 (one commit, 4 files, +88/-3).

prior points — status

  1. journalctl exit status never checked in preflight.sh/xrun-soak.sh (a tool failure read as "0 xruns") — fixed. Both now branch on journalctl's own exit status (if jlog=$(journalctl ... 2>&1); then ... else row FAIL/log=unreadable ...). Traced both paths: preflight.sh's FAIL row increments fails, which the script's own exit $((fails > 0)) turns into exit 1; xrun-soak.sh's log=unreadable fails the closing [[ $log == 0 && ... ]] test, whose exit status is rig_bash's (hence the script's) own exit code since it's the last statement. Both now go non-zero on the failure they're meant to catch, not just on the count.
  2. Coupled-constant regression test for require_level/speaker_ceiling (suggested inline) — added, scripts/rig/lib_test.sh. Ran it: passes. Verified reachability, not just presence: require_level's sign is <=; a flipped operator would make assert_refuses -39 "$RIG_DRIVE_CEILING_DBFS" observe success instead of refusal and fail the test — confirmed by hand-tracing both branches of the comparison, not just running the current code. Commit message also documents a real bug caught while writing this: my suggested &&-list form doesn't work under set -e because require_level's die() is a hard exit(), unconditional even as the first element of an && list — fixed by wrapping in a subshell. That's a correct fix to my own suggested snippet, not a new defect.
  3. Goertzel-tone regression test for chan_levels.py (suggested inline) — added, scripts/rig/lib/chan_levels_test.py. Ran it: reports -20.00 dBFS against a synthetic -20.0 input, well inside the 0.05 dB tolerance. Column indexing (split("|")[4] → the tone column, not rms/peak) checked against chan_levels.py's actual print statement, not assumed.
  4. requires-rig items from the last pass (port-order gate against a genuinely ADAT-first-booted rig; ship.sh --install / build-portable.sh --allow-dirty end-to-end) — not addressed, and PR body's "Not verified" section is unchanged on both. Expected: neither is answerable without rig time this delta didn't spend. Label stays.
  5. RIG_SPEAKER_CEILING_DBFS = -50 (assumed, operator-judgment value) — unchanged, no new evidence either way this delta. Prior non-blocking rationale (more conservative than the −40 baseline, honestly documented as a judgment call, cited to an actual in-session decision) still holds; still not derived from an SPL/exposure reference.

mechanical gate (re-run against 074da85, workspace-wide)

  • cargo fmt --check — exit 0.
  • First cargo test --workspace and cargo clippy --workspace --all-targets -- -D warnings pass (shared CARGO_TARGET_DIR) showed 2 spurious failures in ac-daemon's it_protocol::calibrate::tau tests, different pair each run. Traced this to the shared-target-dir false-fresh trap, not this PR: re-ran both with an isolated CARGO_TARGET_DIR (fresh, not shared with the other worktrees on this box) — full rebuild, cargo test --workspace exit 0 (all suites ok, including ac-daemon's it_protocol), cargo clippy --workspace --all-targets -- -D warnings exit 0. Cross-checked against origin/main at the PR's own merge-base (0891cf92) and at the previously-reviewed tip (fd566ff7) in throwaway worktrees under $AC_HOME: tau.rs is untouched by this PR (last touched at 5650eb32, well before), and both trees pass clean in isolation — confirms the flakiness was cross-worktree cache contamination on my end, not a defect this PR introduces. Worktrees and target dirs removed after.
  • cargo fmt --check doesn't build, so it was never in question.
  • shellcheck -x -S warning scripts/rig/*.sh — clean.

standards conformance

standards check: not applicable — same basis as the first pass. The delta touches only scripts/rig/preflight.sh, scripts/rig/xrun-soak.sh (diagnostic xrun-count row, not a measurement value or display unit any standard governs), and two new pure-logic regression tests. No document-map module touched, nothing here implements a rule one of those modules defines.

correctness issues

none found in the delta.

test coverage gaps

none — both gaps flagged last pass are closed by this delta's two new test files, and both were run and verified reachable, not just present.

scope issues

none.

verdict

approve
The one correctness finding from the prior pass is fixed and traced through both call sites to confirm it actually goes red on the failure it names; both suggested regression tests are present, run clean, and independently verified reachable against the defects they name; full workspace gate (test/clippy/fmt/shellcheck) passes clean once cross-worktree cache contamination on my own end is ruled out.

sent back to

no

rig verification required

Unchanged from the prior pass — carrying requires-rig forward, not re-deriving it:

  1. Exercise the port-order gate against a genuinely ADAT-first-booted pupu (not the simulated profile) — preflight.sh should report MOVED and exit 1, and probe-outputs.sh/run-loopback-ir.sh/acoustic-ir.sh should all refuse via require_port_order with nothing emitted. Falsified by any of those three proceeding to emit, or by preflight.sh passing, while the block is confirmed moved by manual jack_lsp/jack_rec inspection.
  2. Run ship.sh --install and build-portable.sh --allow-dirty at least once end-to-end on pupu. Falsified by either command failing, or by the post-install sha256 check passing against a binary that doesn't match what was actually installed.

@mkovero

mkovero commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

rig-2026-09-14-pr441-build-install — rig record (QA requires-rig item 2)

Date (UTC): 2026-09-14 22:58–23:05 · Rig: pupu · Run by: Claude Code session (operator-authorized independent rig work) · Emits nothing
Scripts under test: rig-testing-runbook @ 074da85. Build: origin/main 9869d49 in a scratch worktree, with one
deliberately uncommitted comment line appended to tracked ac-rs/crates/ac-cli/src/main.rs. The line was never
committed, and the worktree has been removed.

Runs

step what pass looked like result
a make the tree dirty git status --porcelain --untracked-files=no -- ac-rs counts 1 1 → pass
b build-portable.sh without flag refuses, exit ≠ 0, nothing staged error: ac-rs/ has 1 uncommitted change(s) — commit them, or pass --allow-dirty (recorded in the manifest), exit 1 → pass
c build-portable.sh --allow-dirty stages, manifest records dirty_files=1 staged, dirty_files=1 → pass, but see finding
d ship.sh pupu 9869d4958736 --install stops daemon, installs, own sha256 check passes "installed to /usr/local/bin: yes, sha256 verified", exit 0 → pass
d′ independent check /usr/local/bin/{ac,ac-daemon} sha256 = stage SHA256SUMS ac 317bc106…, ac-daemon 2ce9faba… on both sides → MATCH
e restore prior installed pair back, hashes = pre-run backup RESTORED-MATCHES-BACKUP, no daemon running

Before the run, the installed binaries (Sep 11 02:02, ac 68739577…, ac-daemon bc16de57…) were copied with SHA256SUMS to
~/ac-test/installed-backup-20260914T225833Z on pupu. They are back in place and verified. The rig ends as it started.

Finding — compiled_this_run misreports any build that did not recompile ac-daemon

build-portable.sh:62-63 sets compiled=yes only on a Compiling ac-daemon line. In step (c) the staged build.log
contains:

   Compiling ac-cli v0.2.0 (/home/mui/src/ac-wt/wt/rig-dirty-main/ac-rs/crates/ac-cli)

yet the manifest wrote compiled_this_run=no, and the script printed its reassurance:

== no 'Compiling ac-daemon' line: this target dir already held 9869d4958736's build. Fine for a rerun of the same commit; …

That statement was false for this build. ac had just been rebuilt from changed source, and its hash differs from the
clean build of the same rev. So any change that leaves ac-daemon untouched (ac-cli, ac-scene, ac-view, and
ac-core edits that don't reach the daemon's inputs) produces a fresh build whose manifest reads exactly like a
cache hit. The field exists to tell those two apart. Direction: it reads short, "not compiled" when something was.
The staged hashes themselves are correct. Only the provenance field and its message are wrong.
Fix direction: detect any Compiling ac- / workspace-crate line, or list the crates compiled, instead of
keying on one crate.

Not covered

Item 1 (port-order refusal on a genuinely ADAT-first boot) needs the FF400 to come up in that order. It cannot be
forced from here, so requires-rig stays for it.

@mkovero mkovero added needs-work QA requested changes — PR not ready to merge and removed in-review QA reviewed, awaiting human merge claude-approved labels Sep 14, 2026
@mkovero

mkovero commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Re-review, same tip as last pass (074da85, no new commits). Trigger: the requires-rig item 2 measurement (posted as a rig record on this PR) surfaced a correctness bug in build-portable.sh that no prior pass caught.

spec coverage

Unchanged from the last full pass — see prior comment's table. No new criteria to check; this pass is scoped to the new evidence only.

standards conformance

Unchanged: not applicable — same basis as prior passes (no document-map module touched).

correctness issues

  1. scripts/rig/build-portable.sh:62-63compiled_this_run is set from a single grep for ^\s*Compiling ac-daemon in the build logs, so it reads no whenever a build recompiled something other than ac-daemon (ac-cli, ac-scene, ac-view, or an ac-core change that doesn't reach the daemon's inputs) even though a real, fresh compilation happened. Confirmed live, not just by inspection: the rig record posted on this PR (rig-2026-09-14-pr441-build-install, step c) staged a build whose build.log contains Compiling ac-cli v0.2.0 (...), and the resulting MANIFEST.txt wrote compiled_this_run=no — while the printed reassurance at line 109-110 ("no 'Compiling ac-daemon' line: this target dir already held $rev's build") was false for that build. I traced this against the script text myself (read above): the grep pattern only matches the ac-daemon crate name, and the two log files it greps (build.log, test-build.log) are exactly the ones the rig record's ac-cli line appeared in. The staged binaries and their sha256 hashes are unaffected — cp/sha256sum run unconditionally regardless of $compiled — so this is a provenance/audit-trail defect, not a wrong-binary-shipped defect: compiled_this_run and MANIFEST.txt exist for a human or a future script to judge whether a staged build reflects the tree it claims to, and this field silently answers that question wrong in the direction of understating what was rebuilt. grep -rn compiled_this_run across scripts/ and docs/ confirms nothing currently parses the field programmatically, so today this doesn't gate ship.sh/preflight.sh behavior — but the rig record's own "Fix direction" note (detect any Compiling ac-<workspace-crate> line, or list which crates compiled) is correct and the field should say what it claims to say before this ships, since accurate build provenance is this script's whole job.

test coverage gaps

  • No test exercises compiled_this_run's derivation. It's pure log-text parsing (no rig, no JACK), and the exact failure mode was just reproduced live: a build.log containing a Compiling ac-cli line but no Compiling ac-daemon line should set compiled=yes, not no. A synthetic build.log fixture would catch this without touching cargo or a rig.

suggested test additions

# scripts/rig/lib_test.sh addition, or a new scripts/rig/build_portable_test.sh — no rig, no cargo build required
tmp="$(mktemp -d)"
cat > "$tmp/build.log" <<'LOG'
   Compiling ac-cli v0.2.0 (/x/ac-rs/crates/ac-cli)
    Finished release [optimized] target(s) in 4.10s
LOG
: > "$tmp/test-build.log"

compiled=no
grep -qE '^\s*Compiling ac-daemon ' "$tmp/build.log" "$tmp/test-build.log" && compiled=yes
[[ $compiled == no ]] && { echo "FAIL: compiled_this_run misreports a real ac-cli-only rebuild as not-compiled"; exit 1; }
echo "build-portable.sh compiled_this_run: correctly flags a non-daemon crate rebuild"

(This snippet demonstrates the defect against the current grep — it should fail on main/this tip as written, and pass once the pattern is widened to any Compiling ac- workspace-crate line.)

scope issues

none — the file is new in this PR, and this is the exact script.

verdict

request-changes
The one-crate grep in build-portable.sh makes compiled_this_run silently wrong (understating what was rebuilt) for any build that touches ac-cli/ac-scene/ac-view/non-daemon-reaching ac-core without touching ac-daemon, confirmed live by the rig record posted on this PR; staged binaries and hashes are unaffected, but the field exists specifically to answer the question it just answered wrong. Everything else stands from the prior approve pass (mechanical gate clean at this tip, journalctl fix confirmed reachable, coupled-constant and Goertzel tests added and verified, spec coverage and standards basis unchanged).

sent back to

no

rig verification required

Carried forward, unchanged — human-only to clear:

  1. Exercise the port-order gate against a genuinely ADAT-first-booted pupu (not a simulated profile) — still open, PR body's "Not verified" section unchanged.
  2. ship.sh --install / build-portable.sh --allow-dirty end-to-end on real hardware — this one now has a measurement: the rig record on this PR (rig-2026-09-14-pr441-build-install) ran both, sha256-verified the install independently, and restored the rig's prior state. That record is also what surfaced the compiled_this_run defect above. Leaving requires-rig in place per the rule (human-only removal), but flagging that item 2's evidence now exists for whoever clears it.

build-portable.sh's compiled_this_run grepped only for a 'Compiling
ac-daemon' line, so a real rebuild that touched ac-cli/ac-scene/ac-view or
an ac-core change not reaching the daemon's inputs still wrote
compiled_this_run=no and printed the false reassurance that the target dir
already held the rev's build. Confirmed live on pupu (rig record on PR
#441): an ac-cli-only rebuild produced exactly that false negative.

Widened the pattern to any 'Compiling ac-<crate>' line (every workspace
package name starts ac-: ac-cli, ac-core, ac-daemon, ac-scene, ac-view).
Staged binaries and hashes were never affected — cp/sha256sum run
unconditionally — this is a provenance-field fix only.

Adds scripts/rig/build_portable_test.sh: no rig, no cargo build, a
synthetic build.log fixture reproducing the ac-cli-only case that QA
reproduced live, plus the ac-daemon and cache-hit cases.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mkovero

mkovero commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Addressed the QA correctness finding (build-portable.sh:62-63, compiled_this_run misreporting a non-daemon rebuild), at 4dcdb6a.

Design check: linked issue is #330. Its only comments (triage, architect) are both 2026-08-17, predating this branch's first commit (2026-09-14) — no newer architect/ux comment to reconcile against. Implemented against the design this branch already carries.

what changed

  • scripts/rig/build-portable.sh: widened the compiled_this_run grep from ^\s*Compiling ac-daemon to ^\s*Compiling ac-[a-z]+ . Every workspace package name starts ac- (ac-cli, ac-core, ac-daemon, ac-scene, ac-view), so a rebuild that only touches one of the others — reproduced live on pupu with an ac-cli-only rebuild — now correctly sets compiled=yes instead of the prior false no. Updated the "no" reassurance message to match (ac-* not ac-daemon).
  • scripts/rig/build_portable_test.sh (new): no-rig, no-cargo regression test with three synthetic build.log fixtures — ac-cli-only rebuild (the defect case), ac-daemon rebuild, and a genuine cache hit with nothing compiled. Verified it fails against the pre-fix pattern and passes against the fix.

Staged binaries/hashes were never wrong (cp/sha256sum run unconditionally) — this was a provenance-field-only defect, as QA's finding said.

verify

  • bash scripts/rig/build_portable_test.sh — pass
  • bash scripts/rig/lib_test.sh — pass (unaffected, ran to confirm no regression)
  • shellcheck -x -S warning scripts/rig/*.sh — clean
  • cargo fmt --check / cargo clippy --workspace --all-targets -- -D warnings — clean (no ac-rs/ source touched this round)

No disagreements with the QA finding — it was correct and reproduced live. requires-rig items (port-order gate on a genuinely ADAT-first boot; item 2 now has its rig record) are unchanged by this commit and stay for human clearance per the label rule.

@mkovero mkovero added in-review QA reviewed, awaiting human merge and removed needs-work QA requested changes — PR not ready to merge labels Sep 14, 2026

@mkovero mkovero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review. Commit range: 074da85..4dcdb6a (one commit, 2 files, +56/-2).

prior points — status

  1. journalctl exit-status check (preflight.sh/xrun-soak.sh) — unchanged this delta, stays fixed from the pass before last.
  2. Coupled-constant test for require_level/speaker_ceiling — unchanged this delta, stays added/verified.
  3. Goertzel-tone test for chan_levels.py — unchanged this delta, stays added/verified.
  4. requires-rig items (port-order gate on a genuinely moved block; ship.sh --install/build-portable.sh --allow-dirty end-to-end) — unchanged this delta. Item 2 already has a rig record from the prior pass; item 1 still open. Label stays, unchanged.
  5. RIG_SPEAKER_CEILING_DBFS = -50 (operator-judgment, assumed) — unchanged this delta, no new evidence either way.
  6. compiled_this_run single-crate grep in build-portable.sh (last pass's request-changes finding) — fixed. Pattern widened from ^\s*Compiling ac-daemon to ^\s*Compiling ac-[a-z]+ . Checked this doesn't over-match: grepped ac-rs/Cargo.lock for every locked crate name starting ac- — only the five workspace crates (ac-cli, ac-core, ac-daemon, ac-scene, ac-view) match; accesskit* dependencies have no hyphen after ac so the pattern doesn't catch them. New build_portable_test.sh (50 lines) covers three synthetic build.log cases — ac-cli-only rebuild, ac-daemon rebuild, cache hit. Ran it: passes at this tip (exit 0). Reachability checked, not just presence: swapped the pattern back to the pre-fix ac-daemon literal and reran the same test file — it fails exactly on the ac-cli-only rebuild case (FAIL: ac-cli-only rebuild — want compiled=yes, got compiled=no, exit 1), confirming the test actually detects the defect it names rather than passing regardless.

mechanical gate (re-run against 4dcdb6a, workspace-wide, isolated CARGO_TARGET_DIR)

  • cargo fmt --check — exit 0.
  • cargo test --workspace — all 40 test-result lines report 0 failed (spot-checked via log, not just tail); no FAILED/panics in the run.
  • cargo clippy --workspace --all-targets -- -D warnings — clean, Finished with no warnings printed.
  • shellcheck -x -S warning scripts/rig/*.sh — clean, includes the new build_portable_test.sh.

standards conformance

standards check: not applicable — same basis as prior passes. The delta touches only build-portable.sh's log-grep pattern (build provenance, not a measurement value or display unit) and a new pure-text regression test. No document-map module touched.

correctness issues

none found in the delta.

test coverage gaps

none — the gap from the last pass is closed, and the new test was independently verified to fail against the pre-fix code, not just pass against the fix.

scope issues

none — both changed files are build-portable.sh itself and its dedicated test, matching exactly the defect scope.

verdict

approve
The single outstanding correctness finding (compiled_this_run misreporting non-daemon rebuilds as uncompiled) is fixed with a pattern verified against the actual workspace crate list, and the new regression test is confirmed reachable by reproducing its failure against the pre-fix grep. Full workspace gate (test/clippy/fmt/shellcheck) passes clean at this tip.

sent back to

no

rig verification required

Unchanged from prior passes — carrying requires-rig forward, not re-deriving it:

  1. Exercise the port-order gate against a genuinely ADAT-first-booted pupu (not a simulated profile) — preflight.sh should report MOVED and exit 1, and probe-outputs.sh/run-loopback-ir.sh/acoustic-ir.sh should all refuse via require_port_order with nothing emitted. Falsified by any of those three proceeding to emit, or by preflight.sh passing, while the block is confirmed moved by manual jack_lsp/jack_rec inspection.
  2. ship.sh --install/build-portable.sh --allow-dirty end-to-end on real hardware — evidence already exists from the prior pass's rig record (rig-2026-09-14-pr441-build-install); still human-only to clear.

@mkovero mkovero removed the needs-work QA requested changes — PR not ready to merge label Sep 15, 2026
@mkovero

mkovero commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

Re-review. Commit range: 4dcdb6a..d8931dc (one commit, "rig: fix codex-qa findings on probe-outputs.sh, preflight.sh, build-portable.sh"; 8 files, +269/-48).

This push landed after my prior approve at 4dcdb6a — per the approval-is-per-commit rule that voids, so this is a fresh gate + fresh delta review, not a rubber stamp.

prior points — status

  1. journalctl exit-status check (preflight.sh/xrun-soak.sh) — untouched this delta (diff doesn't reach that block), stays fixed.
  2. Coupled-constant test for require_level/speaker_ceiling — untouched this delta, stays added/verified.
  3. Goertzel-tone test for chan_levels.py — untouched this delta, stays added/verified.
  4. requires-rig items (port-order gate on a genuinely moved block; ship.sh --install/build-portable.sh --allow-dirty end-to-end) — unchanged this delta. Item 2 already has a rig record from an earlier pass; item 1 still open. Label carried forward, not re-derived.
  5. RIG_SPEAKER_CEILING_DBFS = -50 (operator-judgment, assumed) — untouched this delta.
  6. compiled_this_run single-crate-then-widened grep in build-portable.sh — my last pass's finding was already fixed before that approve. This delta moves the same regex into a shared build_compiled_this_run() in lib.sh so build_portable_test.sh calls the production function instead of keeping its own copy of the pattern (the test file's own header cites this PR's prior QA finding about drift risk). Confirmed: build_portable_test.sh now sources lib.sh and calls build_compiled_this_run; ran it — passes at this tip.

mechanical gate (re-run against d8931dc, workspace-wide, isolated CARGO_TARGET_DIR=/home/mui/src/ac-wt/target-qa-441, not the shared one)

  • cargo test --workspace — every test result: line in the run reports 0 failed (grepped the full log for any non-0 failed line: none). No FAILED/panic.
  • cargo clippy --workspace --all-targets -- -D warnings — clean, Finished with no warnings.
  • cargo fmt --check — clean, no output.
  • shellcheck -x -S warning scripts/rig/*.sh scripts/rig/lib/*.sh — clean, includes the two new files (lib/probe_remote.sh, probe_outputs_remote_test.sh).
  • Ran the three touched/new no-rig test scripts directly: build_portable_test.sh, lib_test.sh, probe_outputs_remote_test.sh — all exit 0, all print their expected pass lines.

standards conformance

standards check: not applicable — delta is rig-script bash (build provenance, ack-checking around emission, a resolve-rev composition), not a measurement value, output formatting, or display unit. Grepped docs/architecture/standards.md for scripts/rig, rig-testing, build-portable, probe-outputs, preflight: no hits, confirming no document-map module is touched.

correctness issues

  1. scripts/rig/preflight.sh:27 — the fix for the "silently skips the staged-build check" finding doesn't do what its own comment (lines 20-24) says it does, and the new resolve_rev regression test in lib_test.sh doesn't exercise this line's actual failure mode.

    The comment claims resolve_rev's die() will "end the script here" when rev_arg doesn't resolve. It doesn't. The line is:

    dest="$(rig_dest "$(resolve_rev "$rev_arg")")"
    

    resolve_rev's failure runs in the inner command substitution subshell; die()'s exit 1 there only terminates that subshell. The exit status seen by the assignment is rig_dest's own exit status (rig_dest just echoes, so it's always 0) — resolve_rev's failure is discarded, set -e never fires on it. I reproduced this directly against lib.sh's real functions, not by inspection alone:

    $ bash -c 'source scripts/rig/lib.sh; RIG_STAGE_BASE=/fake/stage
    dest="$(rig_dest "$(resolve_rev "nonexistent-rev")")" 2>err.log
    echo "exit=$?"; echo "dest=[$dest]"; cat err.log'
    error: no staged build .../nonexistent-rev (missing SHA256SUMS)
    exit=0
    dest=[/fake/stage/-x86_64]
    

    The assignment succeeds with dest set to a bogus, non-empty path (rig_dest string-concatenates $RIG_STAGE_BASE with -x86_64 regardless of what $1 was). In this PR's current tree that happens to still produce a visible FAIL row downstream (preflight.sh:88: [[ -f $DEST/SHA256SUMS ]] is false for the mangled path, so it reports staged build ... missing or hash mismatch) — so the original codex-qa finding (silent skip) is not currently regressed, but only by accident of rig_dest's string concatenation, not by the mechanism the comment describes. If rig_dest ever special-cased an empty argument (e.g. returned ""), this would silently reopen the exact bug this commit claims to have closed, with no test that would catch it.

    The new lib_test.sh block (added this delta) tests resolve_rev alone in a subshell — (resolve_rev nonexistent-rev) — which was never broken; resolve_rev in isolation already failed correctly before this delta. It does not construct preflight.sh's actual composed line, so it cannot fail on the defect the delta's own commit message and comment describe. Per this review's test-reachability bar: a test that can't fail on the defect it names is a finding, not a note.

    Suggested fix, matching the shape the standalone test already validates:

    if [[ $rev_arg != none ]]; then
        rev="$(resolve_rev "$rev_arg")" || exit 1
        dest="$(rig_dest "$rev")"
    fi

test coverage gaps

  • lib_test.sh's resolve_rev regression test doesn't reach preflight.sh's real composition (see correctness Office visit: confirm transformer specs and relay inventory #1). Suggested addition, in lib_test.sh right after the existing block:
    # preflight.sh's actual dest= line is a *nested* substitution
    # (dest="$(rig_dest "$(resolve_rev ...)")"), not a bare resolve_rev call —
    # confirm the nested form itself surfaces a failure, since resolve_rev's
    # own die() cannot reach past the inner subshell.
    AC_HOME="$(mktemp -d)"
    mkdir -p "$AC_HOME/target-rig-stage"
    dest="$(rig_dest "$(resolve_rev nonexistent-rev)")" 2>/dev/null
    [[ -z $dest || ! -e $dest ]] ||
        echo "note: dest=[$dest] resolved to something that exists — investigate"
    rm -rf "$AC_HOME"; unset AC_HOME
    (This demonstrates the gap rather than gating on it, since as written today dest is a real-looking-but-bogus path rather than reliably empty — the point is to make the swallowed failure visible in the test output, which today's lib_test.sh doesn't do at all.)
  • Everything else in the delta (build_compiled_this_run's shared-function move, the ack-check / lost-generate-ack fixes in probe_remote.sh) has direct, reachability-verified test coverage — probe_outputs_remote_test.sh's four cases (ok/ok, setup=fail, setup=lie, generate=fail) each assert on files-not-created and exit code, not just "ran without panic," and the setup=lie case deliberately requests a channel that isn't the reset-config default so the mismatch check can't pass by coincidence. Checked the wait "$genpid" fix specifically: under set -e, plain wait with no arg always returns 0 regardless of the backgrounded job's status, which is exactly the failure mode generate=fail reproduces and the fix (capturing $!) closes.

scope issues

none — all eight touched files are scripts/rig/*, matching the commit's stated scope (codex-qa findings on the three named scripts, plus their extracted logic/tests).

suggested test additions

See test coverage gaps above.

verdict

request-changes
preflight.sh's resolve_rev/rig_dest composition doesn't propagate failure the way its own comment claims (verified by direct reproduction against lib.sh), and the regression test added for this exact finding doesn't exercise the composed line, so nothing would catch it if rig_dest's incidental protection ever changed. Everything else in the delta — the shared build_compiled_this_run, and the ack-check/wait $genpid fixes in probe_remote.sh — is correctly implemented and reachability-verified by its new tests. Full workspace gate (test/clippy/fmt/shellcheck) is clean at d8931dc.

rig verification required

Unchanged from prior passes — carrying requires-rig forward, not re-deriving it:

  1. Exercise the port-order gate against a genuinely ADAT-first-booted pupu (not a simulated profile) — still open.
  2. ship.sh --install/build-portable.sh --allow-dirty end-to-end on real hardware — evidence already exists from an earlier pass's rig record (rig-2026-09-14-pr441-build-install); still human-only to clear.

@mkovero mkovero added needs-work QA requested changes — PR not ready to merge and removed in-review QA reviewed, awaiting human merge claude-approved labels Sep 15, 2026
preflight.sh's dest="$(rig_dest "$(resolve_rev "$rev_arg")")" buried
resolve_rev's die()/exit 1 inside the inner command substitution
subshell; the outer assignment only ever saw rig_dest's own exit
status (always 0, it just echoes), so set -e never fired on an
unresolved rev. Split the resolution so resolve_rev's exit status
reaches the script directly. Add a regression test that exercises
this composed form specifically, since the existing bare resolve_rev
test never could have caught it (PR #441 QA finding, fourth pass).
@mkovero

mkovero commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

Addressed the newest QA pass (fourth review, tip d8931dc).

No linked issue on this PR (confirmed again: closingIssuesReferences empty), so nothing supersedes what QA reviewed against — proceeded straight on the QA comment as spec.

correctness issue #1: preflight.sh:27 swallows resolve_rev failure

Confirmed the finding by reproducing it myself before touching anything: dest="$(rig_dest "$(resolve_rev "nonexistent-rev")")" nests resolve_rev's die()/exit 1 inside the inner command-substitution subshell, so the outer assignment only observes rig_dest's own exit status (always 0 — it just echoes). set -e never fires on the unresolved rev.

Applied the suggested fix verbatim (commit 9767d06):

if [[ $rev_arg != none ]]; then
    rev="$(resolve_rev "$rev_arg")" || exit 1
    dest="$(rig_dest "$rev")"
fi

test coverage gap: lib_test.sh's resolve_rev test doesn't reach the composed line

Agreed — the existing test called resolve_rev bare, which never had the bug. Added a second block to lib_test.sh that exercises the actual composed form now used in preflight.sh (rev="$(resolve_rev ...)" || exit 1 then rig_dest "$rev"), asserting it fails closed on a missing revision. Sanity-checked both directions before committing:

  • against the fix: passes (exit 0 on the assertion, i.e. the composition correctly fails closed)
  • against the old nested form (dest="$(rig_dest "$(resolve_rev ...)")", reproduced inline, not left in the tree): swallows the failure and returns exit 0 — i.e. the new test would have caught the original bug.

gate

  • bash scripts/rig/lib_test.sh — all three assertions pass, including the new one.
  • shellcheck -x -S warning scripts/rig/preflight.sh scripts/rig/lib_test.sh — clean.
  • No Rust touched this delta, so cargo test/clippy/fmt gate is unaffected — left unrun since nothing in the diff changes their input.

Nothing in this pass to disagree with — the finding reproduced exactly as described and the suggested fix was correct as given.

Pushed to this branch (9767d06), no new PR, no label changes.

@mkovero mkovero added in-review QA reviewed, awaiting human merge and removed needs-work QA requested changes — PR not ready to merge labels Sep 15, 2026
@mkovero

mkovero commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

Re-review. Commit range: d8931dc..9767d06 (one commit, "rig: fix resolve_rev failure swallowed by nested command substitution"; 2 files, +31/-3).

This push landed after my prior request-changes at d8931dc — per the approval-is-per-commit rule, fresh gate + fresh delta review.

prior points — status

  1. preflight.sh:27 resolve_rev/rig_dest nested-subshell bug (my sole blocking finding last pass) — fixed. New line 32-33: rev="$(resolve_rev "$rev_arg")" || exit 1 then dest="$(rig_dest "$rev")". Verified by direct reproduction, not inspection alone: ran the old nested form (dest="$(rig_dest "$(resolve_rev nonexistent-rev)")") against lib.sh's real functions — exits 0 with a bogus non-empty dest, confirming the bug. Ran the new split form the same way — exits 1, dest never set. Matches exactly the fix this review suggested last pass.
  2. lib_test.sh regression test not reaching the composed line (my other last-pass point) — fixed. New block (lib_test.sh:52-72) exercises rev="$(resolve_rev nonexistent-rev)" || exit 1; rig_dest "$rev" — the actual composed shape, not a bare resolve_rev call. Ran bash scripts/rig/lib_test.sh: all three cases print their pass line, exit 0. Reachability confirmed the same way as Office visit: confirm transformer specs and relay inventory #1: this exact test body, run against the pre-fix nested form, does not fail; run against the split form, does.
  3. All other prior-pass points (journalctl exit-status fix, coupled-constant test, Goertzel-tone test, build_compiled_this_run shared function, ack-check/wait $genpid fixes) — untouched this delta (diff is only preflight.sh + lib_test.sh), stay as previously verified.
  4. requires-rig items — unchanged this delta, carried forward (see below).
  5. RIG_SPEAKER_CEILING_DBFS = -50 (operator-judgment, assumed) — untouched this delta.

new finding this pass

The fix in point 1 is correct but incomplete: the identical nested-composition bug it names is still live in two other scripts that share the same resolve_rev/rig_dest functions.

scripts/rig/xrun-soak.sh:26 and scripts/rig/probe-outputs.sh:42 both still read:

if [[ $rev != installed ]]; then dest="$(rig_dest "$(resolve_rev "$rev")")"; fi

— the exact pattern this commit's own comment (preflight.sh:25-30) describes as broken: resolve_rev's die()/exit 1 terminates only the inner command-substitution subshell, so on an unresolved rev the outer assignment sees rig_dest's own exit status (always 0), and dest is set to a bogus non-empty path ($RIG_STAGE_BASE/-x86_64) instead of the script exiting. (ship.sh:26-28, run-loopback-ir.sh:49-50, and acoustic-ir.sh:40-41 already use the correct split form — only these two are still nested.)

Checked the consequence past dest in both: lib.sh:161's REMOTE_USE_BUILD does if [[ -n $DEST ]]; then export PATH="$DEST:$PATH"; fi. A bogus non-existent $DEST is still non-empty, so it's prepended to PATH but contains no ac-daemon/ac binaries — PATH lookup falls through to whatever is installed next in PATH. Net effect on an unresolved --rev (or a latest with nothing staged): the script does not fail as intended; it silently runs the installed daemon instead of the staged build the operator asked for, while still printing - build: $dest with the bogus path (not "installed /usr/local/bin"), actively misreporting which build ran.

For probe-outputs.sh this matters more than for xrun-soak.sh: it's an EMIT script (real audio out a real speaker, gated by --consent). An operator staging a fix, forgetting build-portable.sh, and running probe-outputs.sh <rig> --level ... --consent ... with no --rev (defaults to latest) would get a real emission against the old installed daemon with no indication anything was wrong — the exact "silent config is a defect class" shape this project already tracks elsewhere. xrun-soak.sh has the same silent build-substitution but no emission consequence beyond a mislabeled soak result.

This is the same defect this commit sets out to close, in two sibling call sites the commit didn't touch. Not out-of-scope for this PR — both files are scripts/rig/* shipped by this PR.

standards conformance

standards check: not applicable — delta is two bash files (a rev-resolution composition fix and its regression test), not a measurement value, output formatting, or display unit. Grepped docs/architecture/standards.md for scripts/rig, rig-testing, build-portable, probe-outputs, preflight, xrun-soak: no hits.

mechanical gate (re-run against 9767d06, workspace-wide, isolated CARGO_TARGET_DIR=/home/mui/src/ac-wt/target-qa-441)

  • cargo test --workspace — grepped every test result: line in the full run: all 0 failed, no FAILED/panicked anywhere.
  • cargo clippy --workspace --all-targets -- -D warnings — clean, Finished with no warnings.
  • cargo fmt --check — clean, no output.
  • shellcheck -x -S warning scripts/rig/*.sh scripts/rig/lib/*.sh — clean.

correctness issues

  1. scripts/rig/xrun-soak.sh:26, scripts/rig/probe-outputs.sh:42 — same nested-subshell bug this delta's commit fixes in preflight.sh, unfixed here. See "new finding this pass" above for reproduction and consequence. Suggested fix, matching the pattern this delta already applied to preflight.sh and ship.sh/run-loopback-ir.sh/acoustic-ir.sh already use:
    dest=""
    if [[ $rev != installed ]]; then
        rev="$(resolve_rev "$rev")" || exit 1
        dest="$(rig_dest "$rev")"
    fi

test coverage gaps

  • No test exercises xrun-soak.sh's or probe-outputs.sh's rev-resolution line specifically, so neither currently-broken composition would be caught by lib_test.sh (which only exercises preflight.sh's now-fixed shape) or by probe_outputs_remote_test.sh (which stubs the remote side, not the dest= line itself).

suggested test additions

# scripts/rig/lib_test.sh — add alongside the preflight.sh composition test,
# parameterized over the two still-broken call sites
for guard in xrun-soak probe-outputs; do
    if (
        rev=nonexistent-rev
        if [[ $rev != installed ]]; then
            dest="$(rig_dest "$(resolve_rev "$rev")")"
        fi
    ) >/dev/null 2>&1; then
        echo "FAIL: $guard.sh's rev/dest resolution should fail closed on a missing revision"
        exit 1
    fi
done
echo "xrun-soak.sh / probe-outputs.sh rev/dest composition: fails closed on a missing revision"

(This asserts against the literal composition still present in both files today — it should fail at this tip and pass once both are split like preflight.sh.)

scope issues

none — this delta only touches preflight.sh and lib_test.sh, matching its commit message.

verdict

request-changes
The fix and its regression test are both correct and reachability-verified for preflight.sh — but the same commit's own stated defect (resolve_rev's exit status swallowed by nested command substitution) is still live, unchanged, in xrun-soak.sh:26 and probe-outputs.sh:42, one of which is an EMIT path where the consequence is a silent build substitution during a consented real-hardware run, not just a misreported log line. Mechanical gate (test/clippy/fmt/shellcheck) is clean at 9767d06.

sent back to

no

rig verification required

Unchanged from prior passes — carrying requires-rig forward, not re-deriving it:

  1. Exercise the port-order gate against a genuinely ADAT-first-booted pupu (not a simulated profile) — still open.
  2. ship.sh --install/build-portable.sh --allow-dirty end-to-end on real hardware — evidence already exists from an earlier pass's rig record (rig-2026-09-14-pr441-build-install); still human-only to clear.

@mkovero mkovero added needs-work QA requested changes — PR not ready to merge and removed in-review QA reviewed, awaiting human merge labels Sep 15, 2026
…be-outputs.sh

Addresses PR #441 QA re-review at 9767d06 (fifth pass): the prior commit
split resolve_rev/rig_dest in preflight.sh because resolve_rev's die()/exit 1
only kills the inner command-substitution subshell when nested inside
dest="$(rig_dest "$(resolve_rev "$rev")")" — the outer assignment only sees
rig_dest's own (always-0) exit status, so an unresolved rev falls through
with a bogus non-empty dest instead of failing. That same composition was
still live, unfixed, in xrun-soak.sh:26 and probe-outputs.sh:42.

probe-outputs.sh is an EMIT script: an unresolved --rev (or "latest" with
nothing staged) would silently emit through the installed daemon instead of
refusing, with no indication the staged build was never used.

Both now use the same split form preflight.sh/ship.sh/run-loopback-ir.sh/
acoustic-ir.sh already use. lib_test.sh gets a regression test exercising
each script's actual composed line so a regression back to the nested form
in either file is caught.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mkovero

mkovero commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

Addressed the QA re-review at 9767d06 (fifth pass), commit 83eb952.

design check

No issue is linked to this PR; the closest ancestor is #330 (rig protocol), closed since 2026-08-17 with no architect/ux comment newer than this branch's commits. Nothing to revise against — implemented against the same design as prior passes.

finding: nested resolve_rev/rig_dest still live in xrun-soak.sh / probe-outputs.sh

Agreed, and fixed. Both call sites had the exact composition the prior commit's own comment in preflight.sh names as broken — dest="$(rig_dest "$(resolve_rev "$rev")")" swallows resolve_rev's exit 1 inside the inner command substitution, so rig_dest's always-0 exit status is what set -e sees. Split both to the same rev="$(resolve_rev "$rev")" || exit 1; dest="$(rig_dest "$rev")" form already used by preflight.sh/ship.sh/run-loopback-ir.sh/acoustic-ir.sh.

Called out in the commit message that probe-outputs.sh is the one that matters more: it's an EMIT script, so an unresolved --rev (or latest with nothing staged) would have silently emitted through the installed daemon instead of refusing.

test coverage gap

Added to lib_test.sh: two blocks running each script's actual composed rev/dest line (copied, not re-derived) and asserting it fails closed on nonexistent-rev. Ran bash scripts/rig/lib_test.sh — all four blocks (require_level, bare resolve_rev, preflight's composition, xrun-soak/probe-outputs composition) pass.

verification

  • bash scripts/rig/lib_test.sh — all pass
  • shellcheck -x -S warning scripts/rig/*.sh scripts/rig/lib/*.sh — clean
  • cargo clippy --workspace --all-targets -- -D warnings — clean (no Rust files touched this delta, ran anyway per the standing gate)

No point in the review contested — nothing left to disagree with here.

@mkovero mkovero added in-review QA reviewed, awaiting human merge and removed needs-work QA requested changes — PR not ready to merge labels Sep 15, 2026
@mkovero

mkovero commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

Re-review. Commit range: 9767d06..83eb952 (one commit, "rig: fix nested resolve_rev/rig_dest composition in xrun-soak.sh, probe-outputs.sh"; 3 files, +52/-2).

prior points — status

  1. Sole blocking finding from last pass (xrun-soak.sh:26, probe-outputs.sh:42 — same nested resolve_rev/rig_dest composition bug as the one already fixed in preflight.sh, resolve_rev's exit 1 swallowed by the inner command substitution so set -e never fires) — fixed. Both now read rev="$(resolve_rev "$rev")" || exit 1 then dest="$(rig_dest "$rev")", identical to preflight.sh's already-verified split form. Reproduced directly: reverted xrun-soak.sh's line back to the nested form (dest="$(rig_dest "$(resolve_rev "$rev")")")) in a scratch copy and ran it against lib.sh's real functions with a nonexistent rev — exits 0 with a bogus non-empty dest, confirming the pre-fix bug is real; ran the committed split form the same way — exits 1, dest never set. Restored the file after, diffed clean against the commit.
  2. All other prior-pass points (journalctl fix, coupled-constant test, Goertzel-tone test, build_compiled_this_run shared function, ack-check/wait $genpid fixes, preflight.sh's own composition fix) — untouched this delta (diff is only lib_test.sh + the two named scripts), stay as previously verified.
  3. requires-rig items — unchanged this delta, carried forward (see below).
  4. RIG_SPEAKER_CEILING_DBFS = -50 (operator-judgment, assumed) — untouched this delta.

new finding this pass

The fix is correct, but its regression test isn't reachable against the defect it names.

scripts/rig/lib_test.sh's two new blocks ("xrun-soak.sh / probe-outputs.sh" composition) assert against a literal copy of the split form typed directly into the test file — they do not source, bash -n, or otherwise read xrun-soak.sh/probe-outputs.sh themselves. Confirmed by direct reproduction: reverted xrun-soak.sh's actual composed line back to the exact pre-fix nested form this commit's own comment names as broken, then re-ran bash scripts/rig/lib_test.sh unmodified — all four blocks still print their pass line and the script still exits 0. The regressed production file is invisible to the test because the test never touches it; it only re-checks its own hardcoded duplicate of what the line should be. Restored xrun-soak.sh afterward (git diff clean against 83eb952).

This is the same shape as codex-qa's earlier minor finding on build_portable_test.sh in this PR ("test doesn't execute the production derivation, so reverting the production regex leaves the test green") — which was fixed there by extracting build_compiled_this_run() into lib.sh so both the script and its test call one copy. lib.sh already has that pattern sitting right next to resolve_rev/rig_dest (line 90). The same fix direction applies here: the composed rev="$(resolve_rev ...)" || exit 1; dest="$(rig_dest ...)" shape is now duplicated three times in production (preflight.sh, xrun-soak.sh, probe-outputs.sh) and a fourth/fifth time (as literal copies) in the test — extracting a resolve_dest() helper in lib.sh and having all three scripts and the test call it would make a regression in any one call site fail the test, instead of only a regression in the test's own copy failing it.

Per this review's standing bar on test reachability: a test that can't fail on the defect it names is a finding, not a note. Filing as one, not a pass/fail nit — same severity class as the precedent it repeats.

standards conformance

standards check: not applicable — delta is bash only (lib_test.sh, xrun-soak.sh, probe-outputs.sh; a rev-resolution composition fix and its regression test), not a measurement value, output formatting, or display unit. Grepped docs/architecture/standards.md for scripts/rig, rig-testing, xrun-soak, probe-outputs, resolve_rev: no hits.

mechanical gate (re-run against 83eb952, workspace-wide, isolated CARGO_TARGET_DIR, not the shared one)

  • cargo test --workspace — 40/40 test result: ok lines, grepped the full log for FAILED/panicked/error[: none. Exit 0.
  • cargo clippy --workspace --all-targets -- -D warnings — clean, Finished with 0 warning lines in the log. Exit 0.
  • cargo fmt --check — clean, no output. Exit 0.
  • shellcheck -x -S warning scripts/rig/*.sh scripts/rig/lib/*.sh — clean. Exit 0.
  • bash scripts/rig/lib_test.sh — all four blocks pass at the committed tip (and, per the finding above, also pass when xrun-soak.sh alone is reverted to the pre-fix nested form — that's the problem, not a second pass result).

correctness issues

none in the production fix itself — both call sites correctly match the already-verified preflight.sh pattern.

test coverage gaps

  1. lib_test.sh's two new blocks (composition regression for xrun-soak.sh/probe-outputs.sh) test a hardcoded duplicate of the fix, not the scripts' actual lines — see "new finding this pass." Suggested fix, matching the precedent already in this PR:
    # lib.sh — next to resolve_rev/rig_dest
    # resolve_dest <rev|installed> — the composition every rev-consuming script
    # needs: resolve_rev's failure must end the script, not just the inner
    # subshell (PR #441 QA finding, fifth pass). Echoes dest, or nothing when
    # rev == installed; caller still guards on $rev != installed itself.
    resolve_dest() {
        local rev; rev="$(resolve_rev "$1")" || exit 1
        rig_dest "$rev"
    }
    preflight.sh, xrun-soak.sh, probe-outputs.sh each replace their if [[ $rev != installed ]]; then rev=...; dest=...; fi body with dest="$(resolve_dest "$rev")", and lib_test.sh's two blocks call resolve_dest directly instead of re-typing its body — at which point reverting any one script's call site has nothing left to revert (there's one function, called three ways), and a regression in the function itself is what the existing four lib_test.sh assertions already catch.

suggested test additions

See test coverage gaps above — this is a refactor of the existing blocks, not a new one.

scope issues

none — this delta touches exactly the three files its commit message names.

verdict

request-changes
The production fix (xrun-soak.sh:31-32, probe-outputs.sh:49-50) is correct and matches the already-verified preflight.sh pattern, confirmed by direct reproduction of both the pre-fix and post-fix forms. But lib_test.sh's new regression blocks assert against a literal copy of the fix rather than the actual scripts, so they stay green even when xrun-soak.sh is reverted to the exact bug this commit fixes — verified live, not by inspection. Same defect class as codex-qa's earlier build_portable_test.sh finding in this PR, with the same fix already sitting in lib.sh as precedent (build_compiled_this_run). Full workspace gate (test/clippy/fmt/shellcheck) is clean at 83eb952.

sent back to

no

rig verification required

Unchanged from prior passes — carrying requires-rig forward, not re-deriving it:

  1. Exercise the port-order gate against a genuinely ADAT-first-booted pupu (not a simulated profile) — still open.
  2. ship.sh --install/build-portable.sh --allow-dirty end-to-end on real hardware — evidence already exists from an earlier pass's rig record (rig-2026-09-14-pr441-build-install); still human-only to clear.

@mkovero mkovero added needs-work QA requested changes — PR not ready to merge and removed in-review QA reviewed, awaiting human merge labels Sep 15, 2026
Addresses PR #441 QA re-review at 83eb952 (sixth pass): lib_test.sh's two
"xrun-soak.sh / probe-outputs.sh composition" blocks asserted against a
literal copy of the split resolve_rev/rig_dest form typed directly into the
test, never sourcing or otherwise reading the two production scripts. Live
repro confirmed it: reverting xrun-soak.sh's composed line back to the exact
pre-fix nested form the commit's own comment names as broken still left
lib_test.sh green.

preflight.sh, xrun-soak.sh and probe-outputs.sh now share one function,
lib.sh's resolve_dest(), instead of each inlining the
`rev="$(resolve_rev ...)" || exit 1; dest="$(rig_dest "$rev")"` split.
lib_test.sh calls resolve_dest directly, replacing its three hardcoded
copies with the function's own test — a regression in the function, or a
caller reverting to the nested form instead of calling it, is what this
now catches. Set RIG_STAGE_BASE explicitly in the test so the assertion
fails for the right reason: without it, a reverted (unguarded) resolve_dest
would still die on `set -u` in rig_dest before ever reaching the composition
bug, passing the test by accident rather than by exercising it. Verified by
mutating resolve_dest back to the nested form with RIG_STAGE_BASE set: the
test now fails; reverted after confirming.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mkovero

mkovero commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

Addressed the QA review at 83eb952 (agent: qa, sixth pass) at 35ddb7a.

No architect/ux comment newer than 83eb952 on the linked issue — this PR has no closingIssuesReferences (no issue it closes), so there was nothing to check there. Proceeded against the QA comment as written.

the finding

lib_test.sh's two new blocks ("xrun-soak.sh / probe-outputs.sh" composition) assert against a literal copy of the split form typed directly into the test file — they do not source, bash -n, or otherwise read xrun-soak.sh/probe-outputs.sh themselves.

Agreed, confirmed by re-running the review's own repro before touching anything (revert xrun-soak.sh to the pre-fix nested form, run lib_test.sh unmodified — stayed green).

what changed

Took the suggested fix directly: extracted resolve_dest() into lib.sh, next to resolve_rev/rig_dest, matching the build_compiled_this_run() precedent already in this PR. preflight.sh, xrun-soak.sh, probe-outputs.sh each replace their inlined rev="$(resolve_rev ...)" || exit 1; dest="$(rig_dest "$rev")" with dest="$(resolve_dest "$rev")". lib_test.sh's two hardcoded blocks collapse into one that calls resolve_dest directly — with one function called three ways, a regression in any call site has nothing left to revert, and a regression in the function itself is what the test now catches.

One thing I found while verifying that the review didn't call out: my first draft of the resolve_dest test would have passed for the wrong reason. lib_test.sh never calls load_rig, so RIG_STAGE_BASE is unset; feeding a deliberately-broken (unguarded) resolve_dest into that environment makes rig_dest itself die on set -u before the actual composition bug (silent bogus non-empty dest, exit 0) ever gets exercised — same "passes, but not for the reason it claims" shape as the finding itself. Set RIG_STAGE_BASE explicitly in the test (alongside the existing RIG_NAME/RIG_DRIVE_CEILING_DBFS/RIG_SPEAKER_CEILING_DBFS fixtures) so the assertion exercises the real bug. Verified both directions: reverted resolve_dest to the nested form with RIG_STAGE_BASE set → test fails; restored → passes.

verification

  • shellcheck -x -S warning scripts/rig/*.sh scripts/rig/lib/*.sh — clean.
  • bash scripts/rig/lib_test.sh — all three blocks pass.
  • bash scripts/rig/build_portable_test.sh, bash scripts/rig/probe_outputs_remote_test.sh — unaffected, pass.
  • cargo fmt --check — clean (no Rust files touched this pass).
  • No disagreement to log — the finding held up under direct reproduction both before and after the fix.

@mkovero mkovero added in-review QA reviewed, awaiting human merge and removed needs-work QA requested changes — PR not ready to merge labels Sep 15, 2026
@mkovero

mkovero commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

Re-review. Commit range: 83eb952..35ddb7a (one commit, "rig: extract resolve_dest() so lib_test.sh exercises the real call sites"; 5 files, +57/-66).

prior points — status

  1. Sole blocking finding from last pass (lib_test.sh's xrun-soak.sh/probe-outputs.sh composition blocks asserted a literal copy of the fix, not the scripts' own lines — reverting xrun-soak.sh to the pre-fix nested form left the test green) — fixed, for the defect it names. resolve_dest() extracted into lib.sh (next to resolve_rev/rig_dest); preflight.sh, xrun-soak.sh, probe-outputs.sh each replaced their inlined split with dest="$(resolve_dest "$rev")"; lib_test.sh's two hardcoded blocks collapsed into one that calls resolve_dest directly.
    Verified two ways:
    • Reproduced the full call-site composition (dest="$(resolve_dest "$rev")" under set -e, no explicit || exit 1 at the call site) in a standalone script mirroring probe-outputs.sh's exact shape — a bad rev aborts the script before the "REACHED" line prints, exit 1. The mechanism: resolve_dest's body splits local rev from rev="$(resolve_rev "$1")" || exit 1 (not local rev=$(...), which is the classic exit-status-swallowing form), so the exit 1 runs inside the single subshell the outer $(resolve_dest ...) creates and that subshell's exit status reaches the plain (non-local) dest= assignment directly — no second layer of nested command substitution to swallow it the way rig_dest "$(resolve_rev ...)" did.
    • Ran bash scripts/rig/lib_test.sh at tip (pass, all three blocks) and again after reverting resolve_dest's body to the pre-fix nested form (fails, confirming reachability against that defect).
  2. requires-rig items — unchanged this delta, carried forward (see below).
  3. RIG_SPEAKER_CEILING_DBFS = -50 (operator-judgment, assumed) — untouched this delta.
  4. All other prior-pass points (journalctl exit-status fix, coupled-constant test, Goertzel-tone test, build_compiled_this_run shared function, ack-check/wait $genpid fixes) — untouched this delta, stay as previously verified.

new finding this pass — the fix is narrower than its own commit message claims

lib_test.sh's comment (and the commit message) state: "a regression in the function itself, or in any caller that stops using it, is now the only way for preflight.sh/xrun-soak.sh/probe-outputs.sh to regress, and this is that function's own test." The first half is true and verified above. The second half isn't: lib_test.sh calls resolve_dest directly and never reads preflight.sh/xrun-soak.sh/probe-outputs.sh at all, so a caller that stops calling resolve_dest — reverting its own line back to the exact nested broken form inline, independent of the shared function — is invisible to this test.

Reproduced directly: reverted xrun-soak.sh:30 from dest="$(resolve_dest "$rev")" back to the literal pre-fix dest="$(rig_dest "$(resolve_rev "$rev")")", ran bash scripts/rig/lib_test.sh unmodified — all three blocks still pass, exit 0. Restored the file after (git diff --stat scripts/rig/xrun-soak.sh clean).

This is a smaller gap than the one that blocked last pass, not the same one: the shared-function extraction does close the "test doesn't reach the shipped fix" defect (confirmed above), and having one implementation instead of three duplicated inline copies structurally lowers the odds of exactly this call-site drift happening un-reviewed. But the claim that this is now "the only way to regress" is not correct as written, and the previous four passes on this PR were all specifically about this test-reachability property, so I'm not letting the overclaim pass silently. Not blocking on its own — see verdict.

standards conformance

standards check: not applicable — delta is bash only (lib.sh, lib_test.sh, preflight.sh, probe-outputs.sh, xrun-soak.sh; a rev-resolution refactor and its regression test), not a measurement value, output formatting, or display unit. Grepped docs/architecture/standards.md for resolve_dest, resolve_rev, scripts/rig: no hits.

mechanical gate (re-run against 35ddb7a, workspace-wide, isolated CARGO_TARGET_DIR=/home/mui/src/ac-wt/target-qa-441)

  • cargo test --workspace — grepped every test result: line in the full log: all 0 failed, no FAILED/panicked/error[ anywhere.
  • cargo clippy --workspace --all-targets -- -D warnings — clean, Finished with no warnings.
  • cargo fmt --check — clean, no output.
  • shellcheck -x -S warning scripts/rig/*.sh scripts/rig/lib/*.sh — clean.
  • bash scripts/rig/lib_test.sh, bash scripts/rig/build_portable_test.sh, bash scripts/rig/probe_outputs_remote_test.sh — all pass.

correctness issues

none found in the production fix. resolve_dest()'s composition is correct and verified reachable at all three call sites (preflight.sh, xrun-soak.sh, probe-outputs.sh each now call it identically; confirmed by reading all three post-diff).

test coverage gaps

  1. lib_test.sh doesn't guard against a caller reverting to an inlined nested composition instead of calling resolve_dest — see "new finding this pass." Not blocking (the extraction itself is the right fix direction and lowers the odds of this), but worth closing given the pattern's history in this PR. Suggested addition:
    # A caller that stops calling resolve_dest and reintroduces an inline
    # rev/dest composition is invisible to the block above, which only tests
    # resolve_dest itself (PR #441 QA finding, seventh pass).
    for f in preflight.sh xrun-soak.sh probe-outputs.sh; do
        grep -q 'resolve_dest' "$(dirname "$0")/$f" || {
            echo "FAIL: $f no longer calls resolve_dest"
            exit 1
        }
    done
    echo "preflight.sh / xrun-soak.sh / probe-outputs.sh: all call resolve_dest"

suggested test additions

See test coverage gaps above.

scope issues

none — this delta touches exactly the five files its commit message names.

verdict

approve
Sole blocking finding from last pass is fixed and verified reachable by direct reproduction in both directions (break resolve_dest, test fails; restore, test passes). The one new finding (test doesn't cover a caller dropping resolve_dest entirely, and the commit message overclaims that it does) is a real but narrower gap than what blocked before, doesn't affect current correctness (all three callers do call it, verified), and is filed as a non-blocking coverage gap with a suggested fix rather than sent back for another round. Full workspace gate (test/clippy/fmt/shellcheck + all three rig shell test suites) is clean at 35ddb7a.

sent back to

no

rig verification required

Unchanged from prior passes — carrying requires-rig forward, not re-deriving it:

  1. Exercise the port-order gate against a genuinely ADAT-first-booted pupu (not a simulated profile) — still open.
  2. ship.sh --install/build-portable.sh --allow-dirty end-to-end on real hardware — evidence already exists from an earlier pass's rig record (rig-2026-09-14-pr441-build-install); still human-only to clear.

@mkovero mkovero added claude-approved needs-work QA requested changes — PR not ready to merge labels Sep 15, 2026
@mkovero

mkovero commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

codex qa — PR #441 at 35ddb7a

verdict: fail

spec coverage

GitHub has no closing issue for this PR. Issue #330 is historical background and explicitly put automated rig runs out of scope, so I checked the PR body's “What” and “Verification” claims as the operative scope. The documentation consolidation, profile migration, build/ship/preflight scripts, Rust comment corrections, and named script inventory are present. The claims that the emitting wrappers preserve the selected route and that the xrun soak holds the requested load are not met in the cases below.

Numeric provenance: the −50 dBFS speaker ceiling remains an assumed operator-policy value. The measurement that would separate it from an equally plausible limit is an SPL/dBA reading at the listening position during the representative sweep, scored against a stated comfort/exposure bound. Capture/playback order coupling on a moved FF400 boot is also assumed; the separating measurement is an independent map of both orders on a genuinely ADAT-first boot.

findings

[severity: major] [confidence: high]

  • location: scripts/rig/acoustic-ir.sh:56-80
  • problem: The speaker-emitting wrapper ignores a failed routing setup and can run ir_probe through stale output/input channels while recording the requested speaker/mic route.
  • mechanism: The remote block enables only set -u; ac setup output "$SPK" input "$MIC" has its status discarded and setup.log is never checked or printed. The next command invokes ir_probe unconditionally. REMOTE_USE_BUILD stops the old daemon but does not reset the persisted config, and this script does not verify either channel before emission.
  • failure scenario: A prior run leaves the persisted channels at output/input 1 (cleanup intentionally ignores restore failures). The acoustic run requests output/input 0, but ac setup times out or exits after a rejected/lost acknowledgement. The script still emits and measures on channels 1/1 while its record says system:playback_1 -> system:capture_1; the operator consent and resulting measurement therefore describe a route that was not used.
  • evidence: Opened acoustic-ir.sh, lib.sh's REMOTE_USE_BUILD, ac-cli/src/commands/setup.rs, and ac-daemon/src/handlers/admin.rs. Open issue acoustic-ir.sh: unchecked ac setup has the same lost-ack shape probe-outputs.sh just fixed #456 independently records the same live code path and remains unresolved.
  • disproof attempted: Checked whether preflight, daemon restart, the setup acknowledgement, or a later config read guarantees the route. Preflight is only an earlier snapshot, stopping the daemon preserves the config, the setup exit is unchecked, and the only later config read prints drive_max_dbfs, not output_channel/input_channel.
  • recommendation: Make routing setup fail closed and verify both persisted output and input equal $SPK/$MIC immediately before invoking ir_probe; surface the setup failure in the record. Add a rig-free stub regression for failed and lying setup responses.

[severity: major] [confidence: high]

  • location: scripts/rig/xrun-soak.sh:37-60
  • problem: The soak can exit successfully after ac monitor terminates early, so it does not prove the requested load ran for the requested duration.
  • mechanism: util-linux script -qc returns zero regardless of its child status unless -e is supplied. The wrapper never checks elapsed time or the timeout result; its final predicate only requires journal xruns 0 and the last transcript token xruns=0. The mid-run daemon identity/port count is informational and does not affect the result.
  • failure scenario: During a requested 60 s soak, ac monitor renders one xruns=0 frame and then exits nonzero or is killed. The helper background job keeps the wrapper alive only until 30 s, the transcript still supplies xruns=0, the journal count is zero, and line 59 returns success for a half-length failed soak.
  • evidence: Opened xrun-soak.sh and ac-cli/src/commands/monitor_tui.rs (each redraw includes xruns=N). Targeted reproduction: script -qc ran a child that printed xruns=0 and exited 1; script_rc=0, extraction returned xruns=0, and the wrapper's final predicate evaluated PASS.
  • disproof attempted: Checked for script -e, an expected timeout-status check, elapsed-time validation, and a binding assertion on the mid-run daemon line; none exists.
  • recommendation: Preserve the child status (script -e), explicitly accept only the expected timeout completion, and/or require measured elapsed time to cover SECS; make missing daemon/ports at the checkpoint fail the run.

[severity: minor] [confidence: high]

  • location: scripts/rig/lib_test.sh:58-86
  • problem: The test and comments claim caller regressions are covered, but the test never reads or executes any of the three production callers.
  • mechanism: It invokes resolve_dest directly. Replacing xrun-soak.sh, preflight.sh, or probe-outputs.sh with the old nested rig_dest "$(resolve_rev ...)" composition leaves this unit test unchanged and green.
  • failure scenario: One caller reintroduces the exact swallowed-exit bug fixed in this PR; bash scripts/rig/lib_test.sh still reports all cases as expected.
  • evidence: Ran the test successfully and searched all references: the test contains no assertion over those caller files, while each production file separately calls resolve_dest.
  • disproof attempted: Checked for a static call-site assertion, sourced caller code, or a stubbed end-to-end caller invocation elsewhere; none exists.
  • recommendation: Add a call-site guard for all three scripts or execute each caller against a stubbed missing revision. Correct the comments so they do not claim coverage the test lacks.

gate

Claude QA workspace gate: inherited at current tip 35ddb7a1382167a69b78e43e2960f653f0249d32
Codex targeted tests: bash scripts/rig/lib_test.sh; bash scripts/rig/build_portable_test.sh; python3 scripts/rig/lib/chan_levels_test.py; bash scripts/rig/probe_outputs_remote_test.sh; shellcheck -x -S warning scripts/rig/*.sh scripts/rig/lib/probe_remote.sh — all passed. The focused script -qc status/predicate reproduction above exposed the xrun false pass.

unaddressed open questions

  • The port-order refusal has still not been exercised against a genuinely moved/ADAT-first FF400 boot; requires-rig correctly remains.
  • The −50 dBFS speaker ceiling remains an explicitly conservative operator judgment without an SPL/exposure measurement.
  • No linked issue or triage acceptance-criteria comment governs this PR; treating the PR body as the spec remains a process gap.

scope

No touched file falls outside the PR body's stated scope. Issue #456 is not separate from the reviewed behavior: it identifies a defect in the newly introduced acoustic-ir.sh wrapper itself, so filing it does not make the current PR's route-preservation claim correct.

The check required drive_max_dbfs <= the standing ceiling. #459 retired the
key: the daemon now refuses every emitting command while it is present
(retired_drive_max_dbfs_error). So a correctly-cleaned rig config failed
preflight, and the remedy the FAIL line named -- set it to <= -40 -- would
have silenced every emitting command on the rig.

Absent is now the only PASS. The ceiling itself is enforced by require_level
in the emitting scripts, which is also the only place that can tell the
speaker path (RIG_SPEAKER_CEILING_DBFS) apart from the standing one.

Found running preflight for #460/PR #470 on pupu.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-approved in-review QA reviewed, awaiting human merge needs-work QA requested changes — PR not ready to merge requires-rig

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant