docs: complete the code audit (S5-S7) and triage findings into tasks

Ran read-only audits over the last three coverage slices: the VM test framework (S5), the Python (S6), and the elisp prototype (S7). That closes the sentry code-inspection sweep -- all seven slices covered.

S5 turned up five bugs (one Major: run-net-scenarios prints a diagnose failure but exits green) and three refactors. S6 found one bug plus a High test-quality defect -- the pacman-hook-order test compares two string literals, so the boot-critical hook ordering it claims to guard is never actually measured. S7 found three Minor bugs in the gallery-widget prototype (the needle clamps while the readout doesn't, a missing cl-lib require, a nil-path load) plus refactor and coverage gaps.

Appended every finding to the sentry findings doc, marked the coverage tracker, and filed six graded tasks: the two Major/High defects as [#C], and four [#D] clusters (VM-framework robustness, prototype elisp bugs, test-quality, and a test-framework/prototype refactor group). Detection only -- nothing fixed here.

2 files changed, 97 insertions, 3 deletions

diff --git a/docs/design/2026-07-19-sentry-code-findings.org b/docs/design/2026-07-19-sentry-code-findings.org
index 3816ebc..add1829 100644
--- a/docs/design/2026-07-19-sentry-code-findings.org
+++ b/docs/design/2026-07-19-sentry-code-findings.org
@@ -26,9 +26,9 @@ Each fire takes the next uncovered slice. Mark done as fires complete.
 - [X] S2 — archsetup installer, lines ~1050-2100 (post-chroot system config: locale, mirrors, user, dotfiles/stow, AUR, services, snapshot tooling, WM packages). NB still NO partitioning/mkfs/mount in this range either — the disk path is in S3 or delegated to a separate tool.
 - [X] S3 — archsetup installer, lines ~2100-3124 (desktop/WM, gaming, boot_ux: mkinitcpio/GRUB, finalize). RESOLVED: archsetup does NO partitioning/mkfs/mount anywhere in the file — it is a post-install config layer; disk setup is upstream (archinstall/separate tool).
 - [X] S4 — scripts/*.sh production scripts (audit-packages, cmail-setup-finish, post-install, import-wireguard, normalize-notify-sounds, generate-palette, easyeffects, games, setup-chess)
-- [ ] S5 — scripts/testing/ framework + lib (run-test, vm-utils, validation, logging, network-diagnostics, testinfra, run-maint*, create-base-vm, cleanup, debug, setup-testing-env)
-- [ ] S6 — Python: gen_tokens + test suites (refactor lens; test-quality per testing.md)
-- [ ] S7 — elisp (gallery-widget.el)
+- [X] S5 — scripts/testing/ framework + lib (run-test, vm-utils, validation, logging, network-diagnostics, testinfra, run-maint*, create-base-vm, cleanup, debug, setup-testing-env)
+- [X] S6 — Python: gen_tokens + test suites (refactor lens; test-quality per testing.md)
+- [X] S7 — elisp (gallery-widget.el)
 
 * Findings
 
@@ -172,3 +172,79 @@ info/ok/skip/warn/err printf helpers re-defined in setup-chess.sh:61-65, cmail-s
 Per-file: audit-packages clean; cmail 1 (Major); post-install clean (deliberate best-effort); import-wireguard 1 (Minor, key handling itself correct); normalize-notify 1 (Minor); games clean; setup-chess 2 refactors; generate-palette clean; easyeffects clean.
 
 S4 verdict: 3 bugs (1 Major, 2 Minor), 2 refactors (2 Low). Plus a SECURITY item escalated to the approval queue (tracked WireGuard private keys).
+
+** S5 — scripts/testing/ framework + lib (speedrun audit, 2026-07-20 ~16:00)
+
+*** [BUG Major] scripts/testing/run-net-scenarios.sh:103 — diagnose-check failure is printed but never counted, run exits green
+The scenario_diagnose_expect branch prints fail "$name: diagnose did NOT name it" but does not exit 1 (unlike the break and assert branches). Because the whole scenario runs inside ( ... ) || fails=$((fails+1)), a subshell whose only failure is the diagnose check still exits 0, so fails is not incremented and the script prints "all scenarios passed" and exit 0. A regression where net doctor can no longer identify a fault it is supposed to name is reported as a visible FAIL line yet a green exit code, so CI/automated callers treat the run as passing. Fix: make the diagnose-else branch force a non-zero subshell exit (exit 1 like the other two checks). First-draft harness per its own header — reviewed, not yet relied on.
+
+*** [BUG Minor] scripts/testing/debug-vm.sh:49 — hardcoded base-disk name ignores FS_PROFILE
+BASE_DISK is hardcoded to the btrfs image archsetup-base.qcow2, while init_vm_paths (line 56) already computes the profile-correct DISK_PATH (archsetup-base-zfs.qcow2 for FS_PROFILE=zfs). FS_PROFILE=zfs ./debug-vm.sh boots the btrfs base or fatals "Base disk not found." Fix: use DISK_PATH from init_vm_paths. Debug-only tool, zfs path only.
+
+*** [BUG Minor] scripts/testing/lib/vm-utils.sh:284 — snapshot restore races the force-killed QEMU's qcow2 lock
+kill_qemu sends kill -9 and deletes the PID file without waiting for reaping, so vm_is_running reports false at once. In the force-kill fallback, restore_snapshot then runs qemu-img snapshot -a while the dying qemu may still hold the qcow2 lock; qemu-img refuses a locked image, the restore fails, and in the trap it is swallowed by || true, leaving the base image dirty with no signal. Only bites on ACPI-powerdown timeout. Fix: wait for the killed PID (or poll /proc/$pid) before returning.
+
+*** [BUG Minor] scripts/testing/lib/vm-utils.sh:69 — runtime paths not profile-suffixed, parallel btrfs+zfs runs collide
+init_vm_paths suffixes DISK_PATH and OVMF_VARS by FS_PROFILE but leaves PID_FILE, MONITOR_SOCK, SERIAL_LOG fixed. A concurrent btrfs and zfs run shares those three: the second run's stop_qemu/vm_is_running reads the first run's PID file and can shutdown/kill the other VM; both truncate the same serial log. The default SSH_PORT check partially masks it. Fix: suffix these three like DISK_PATH.
+
+*** [BUG Minor] scripts/testing/run-test.sh:287 — ARCHSETUP_EXIT_CODE never holds the installer's real exit status
+The variable is set from a completion-marker grep, not archsetup's real exit code — the installer is launched detached (setsid ... &) and runs with set -e off, so it can error mid-run, continue, reach the end, and still write the marker. The report prints "ArchSetup Exit Code: 0" and the final gate treats that as success. Testinfra is the real backstop, so a broken install is still caught, but the reported code is misleading. Same pattern run-test-baremetal.sh:234. Fix: rename (ARCHSETUP_COMPLETED) and/or capture the true exit status via a file.
+
+*** [REFACTOR Medium] scripts/testing/run-test.sh + run-test-baremetal.sh — large-scale duplication of the run/poll/report skeleton
+The two runners duplicate the git-bundle transfer, the 30s MAX_POLLS loop, the completion-marker exit derivation, the Testinfra invocation, and the near-identical report heredoc — and have drifted (VM: setsid launch + copy_to_vm/copy_from_vm helpers; baremetal: nohup + hand-rolled sshpass scp). Extract the shared poll-and-report core into a sourced helper so baremetal inherits the sturdier launch/copy paths.
+
+*** [REFACTOR Low] scripts/testing/run-maint-nspawn.sh:66 + run-maint-scenarios.sh:78 — duplicated scenario plumbing beyond the acknowledged helper trio
+_scenario_var, _validate_scenario, and the mexec/mmaint/mfix/massert_metric helpers are byte-for-byte duplicated. The transport-specific trio (ssh vs nspawn) is defensibly duplicated, but _scenario_var and _validate_scenario are transport-independent pure functions that could live in a sourced lib/maint-scenario.sh. run_scenario is also nearly identical.
+
+*** [REFACTOR Low] scripts/testing/run-test.sh:251,265 — two different mechanisms to detect the same process
+The launch check uses pgrep -f 'bash archsetup' (251) and the poll loop uses ps aux | grep '[b]ash archsetup' (265) for the identical liveness question. Use one mechanism to remove a maintenance trap.
+
+S5 verdict: 5 bugs (1 Major, 4 Minor), 3 refactors (1 Medium, 2 Low). No syntax errors; temp-file traps, snapshot-stopped guards, mfix PIPESTATUS, and the errexit-off window handled correctly.
+
+** S6 — Python: gen_tokens + test suites (speedrun audit, 2026-07-20 ~16:00)
+
+*** [TEST-QUALITY High] tests/installer-steps/test_pacman_hook_order.py:20 — ordering "invariant" is a tautology that never touches the file
+test_safety_hooks_precede_mkinitcpio_removal claims to guard that 05-zfs-snapshot and 10-hypr-live-update-guard hooks are created before 60-mkinitcpio-remove. But the two assertLess calls compare two string literals ("05..." < "60...") — a constant ASCII fact always true regardless of file content. The ordering is never verified; the test would still pass if the hooks appeared after the removal or the removal moved above them. Only the assertIn presence checks do real work. Fix: assert on positions — text.index("05-zfs-snapshot.hook") < text.index("60-mkinitcpio-remove.hook") (and the guard hook). Matters because a real reorder can remove the current initramfs without a rebuild (unbootable) and this test would ship it green.
+
+*** [BUG Minor] scripts/testing/tests/test_desktop.py:96 — shell glob passed to `test -S` breaks on zero or multiple compositor sockets
+test_hyprland_socket runs host.run("test -S /tmp/hypr/*/.socket.sock"). The remote shell expands the glob: zero matches leaves the literal pattern (rc 1), two+ instances pass multiple args (rc 2 "too many arguments"). Either way the assertion fails for a reason unrelated to socket existence. Masked today because the test gates on compositor_running, which the headless VM never satisfies (always skips). Fix: resolve one socket first (find /tmp/hypr -name .socket.sock -type s | head -1).
+
+*** [TEST-QUALITY Low] tests/gallery-tokens/test_gen_tokens.py:181 — degenerate-marker test asserts properties too weak to notice garbled output
+test_start_marker_on_final_line_without_newline feeds "keep\n<START> <END>" and asserts only startswith("keep\n"), END present, "X" present — all hold, but the actual output is malformed (duplicated start marker, content spliced mid-line). Passes on broken output. The input can't occur in real HTML (markers always on their own newline-terminated lines), so severity is low; tighten to the exact expected string or drop as impossible.
+
+*** [REFACTOR Low] docs/prototypes/gen_tokens.py:78 — four emitters repeat the section-iteration skeleton
+emit_web_css, emit_waybar_gtk, emit_elisp each walk COLOR_SECTIONS then glow (resolving through resolve_color), then font/timing for two of three. A generator yielding resolved (name, color_value) pairs with each emitter supplying its formatter would remove the triplicated traversal. Readability only — divergences are load-bearing (waybar's font/timing omission is deliberate); keep them visible.
+
+S6 verdict: 1 bug (Minor), 2 test-quality (1 High, 1 Low), 1 refactor (Low). gen_tokens.py has no correctness bugs; the sed-extract unit tiers import real function bodies, fake only at the system boundary, assert behavior not prose, carry N/B/E coverage, no shared state or hardcoded dates. Testinfra tiers sound.
+
+** S7 — elisp: gallery-widget.el + gallery-tokens.el (speedrun audit, 2026-07-20 ~16:00)
+
+*** [BUG Minor] docs/prototypes/gallery-widget.el:139 — readout renders raw value while the needle clamps
+The needle angle is clamped 0-100 in gallery-widget--needle-angle, but the readout at line 139 uses (round value) on the unclamped input. Value 150 draws the needle pinned at +60 degrees (x2=82.64) while the text reads "150%" — confirmed live. The two halves disagree at any out-of-range input. Fix: compute a single clamped value once and format both the angle and the readout from it.
+
+*** [BUG Minor] docs/prototypes/gallery-widget.el:69 — cl-loop used without (require 'cl-lib)
+gallery-widget--node calls cl-loop, but the file requires only svg and dom. Works today only via cl-loop's autoload cookie; bites on byte-compile in a context that macroexpands cl-loop without cl-lib, or if a future cl- macro isn't autoloaded (cold-load void-function). Fix: add (require 'cl-lib).
+
+*** [BUG Minor] docs/prototypes/gallery-widget.el:29 — file-name-directory on a possibly-nil path
+The token load computes its directory from (or load-file-name buffer-file-name); both are nil when the form is eval'd outside a load and outside a file buffer, so file-name-directory receives nil and errors. Doesn't bite in the normal load/require path or the test harness, but makes interactive re-eval fragile. Fix: fall back to default-directory when neither is set.
+
+*** [REFACTOR Medium] docs/prototypes/gallery-widget.el:95,136 — hardcoded SVG path strings duplicate the geometry the constants encode
+The arc path "M 1 48 A 47 47 0 0 1 95 48" and hub path "M 44 48 A 4 4 0 0 1 52 48 Z" bake in cx=48, cy=48, radius 47, hub radius 4 — the same values expressed numerically elsewhere and passed to --polar for ticks/needle. Change cx/cy/radii and ticks/needle move while arc/hub silently stay put (dial desyncs, no error). Derive the arc endpoints and hub path from cx/cy/radius via the existing --polar/--fmt machinery, or name the magic numbers as shared defconsts.
+
+*** [REFACTOR Low] docs/prototypes/gallery-widget.el:72,84 — reliance on the private svg--append internal
+gallery-widget--node (72) and the defs/filter insertion (84) call svg--append, a double-dash private of svg.el with no cross-version stability guarantee. Low severity (prototype pinned to a known Emacs); prefer public svg.el builders or isolate the private call behind the single --node helper.
+
+*** [TEST-QUALITY Medium] tests/gallery-widgets/test-gallery-widget.el:77 — obtuse tick count via split-string + cl-count-if :start 1
+The "exactly three ticks" assertion splits on class="tick" then cl-count-if with an always-true predicate and :start 1 to count length-minus-one — a coincidence of split semantics, not a match count. Brittle: an empty leading/trailing segment or a stray class="tick" substring elsewhere throws it off. Replace with a direct occurrence count.
+
+*** [TEST-QUALITY Medium] tests/gallery-widgets/test-gallery-widget.el:47 — clamp coverage tests the angle helper but never the rendered readout
+The boundary test asserts gallery-widget--needle-angle clamps -5 and 150, but nothing asserts the full gauge's readout at an out-of-range value — exactly why the readout/needle mismatch (S7 bug 1) ships green. Add a gauge-level boundary case checking both the needle endpoint and the readout string at an out-of-range input.
+
+*** [TEST-QUALITY Low] tests/gallery-widgets/test-gallery-widget.el:159 — output helper gallery-widget-write-svg is untested
+gallery-widget-svg-string is exercised indirectly, but gallery-widget-write-svg has no coverage. A Normal case (write to temp, assert file exists, head is "<svg", return value equals the path) closes the last uncovered public entry point.
+
+S7 verdict: 3 bugs (3 Minor), 2 refactors (1 Medium, 1 Low), 3 test-quality (2 Medium, 1 Low). No Critical/Major; SVG geometry, needle math, token lookup, error handling otherwise sound; all 10 existing tests pass.
+
+** Audit complete (S1-S7)
+
+Installer (S1-S3): 16 bugs, 10 refactors. Scripts (S4): 3 bugs, 2 refactors + WireGuard secrets (remediated). Test framework (S5): 5 bugs, 3 refactors. Python (S6): 1 bug, 2 test-quality, 1 refactor. Elisp (S7): 3 bugs, 2 refactors, 3 test-quality. All slices covered. Speedrun triaged S1-S4 bugs into tasks (chpasswd, df, run_step, sudoers, wireguard, idempotency, chmod/cp, normalize-notify shipped; GRUB-overwrite + others filed). S5-S7 triaged into the tasks below.
diff --git a/todo.org b/todo.org
index aee3dbc..7297b05 100644
--- a/todo.org
+++ b/todo.org
@@ -1492,6 +1492,12 @@ CLOSED: [2026-07-20 Mon]
 Fixed in 36daf76: the down now runs before the rename modify (targets the stable UUID), so a failed modify under set -e can't leave a live full-tunnel VPN. Added a connection-down case to fake-nmcli and two ordering tests.
 Grading: Major severity (all traffic silently routed through Proton until manual cleanup) x rare (nmcli modify failure) = P3 = [#C].
 scripts/import-wireguard-configs.sh:51-62 imports (which brings the 0.0.0.0/0 tunnel up), renames, then deactivates; under set -e a failed modify aborts before the down, leaving the tunnel live. Fix: bring the connection down right after parsing the UUID, before the rename. See findings doc (S4).
+** TODO [#C] net-scenarios diagnose failure exits green :bug:test:solo:
+Grading: Major severity (a net-doctor diagnosis regression is reported as a passing run — false green on a diagnostic tool) x rare edge case (only when a diagnosis regresses and this first-draft harness is relied on) = P3 = [#C].
+scripts/testing/run-net-scenarios.sh:103 — the scenario_diagnose_expect else-branch prints fail "...diagnose did NOT name it" but never forces a non-zero subshell exit, so ( ... ) || fails=... leaves fails unincremented and the script prints "all scenarios passed" + exit 0. Fix: exit 1 in that branch like the other two checks. See findings doc (S5).
+** TODO [#C] pacman-hook-order test is a tautology :test:solo:quick:
+Grading: Major severity (guards boot-critical hook ordering — a reorder that removes the current initramfs without a rebuild is unbootable, and this test would ship it green) x rare (hook order rarely changes) = P3 = [#C].
+tests/installer-steps/test_pacman_hook_order.py:20 — the two assertLess calls compare string literals ("05..." < "60..."), a constant ASCII fact always true regardless of file content; the ordering the test exists to protect is never measured. Only the assertIn presence checks do real work. Fix: assert on positions — text.index("05-zfs-snapshot.hook") < text.index("60-mkinitcpio-remove.hook") (and the guard hook). See findings doc (S6).
 ** TODO [#C] Add inetutils to install base :feature:solo:quick:network:
 TRAMP's /ftp: method (ange-ftp) shells out to a command-line ftp client; Arch ships none by default. GNU inetutils provides =/usr/bin/ftp=. Craig's dirvish config has an FTP quick-access entry (phone FTP server), so it's a config dependency. Installed manually on ratio 2026-07-14; velox needs it once it boots. Add to the install base so future machines get it for free; verify via VM test. From .emacs.d handoff 2026-07-14-1751.
 
@@ -1672,6 +1678,18 @@ scripts/normalize-notify-sounds.sh:39-46 has no EXIT trap on the mktemp and does
 Grading: no behavior change; parking lot. 10 refactors remain from the sentry audit — duplicated GPU-modalias scan, triple hand-rolled retry loop, stow x4, display_server/window_manager dispatch dup, Maia ELO range x3, per-script log helpers, GRUB/snapper/fsck sed clusters, waybar-battery positional sed. Full list with line numbers in [[file:docs/design/2026-07-19-sentry-code-findings.org][sentry code findings]] (High/Medium/Low tagged). Pull individual ones out as their own tasks when tackled. The system-mutation sed clusters (snapper/fsck/GRUB/waybar) want characterization coverage before any rewrite.
 *** 2026-07-20 Mon @ 16:35:00 -0500 Extracted validate_yesno and the NVIDIA_MIN_DRIVER constant
 In 67d0b6e: the four yes/no config-validation blocks collapse into validate_yesno (TDD), and the driver-floor literal 535 becomes NVIDIA_MIN_DRIVER. The safe, purely-testable slice; the remaining 10 (structural / system-mutation) stay parked above.
+** TODO [#D] VM test-framework robustness cluster :bug:test:solo:
+Grading: Minor severity x rare edge case (each fires only in a narrow test-harness path) = P4 = [#D]. Group of four small framework bugs from the S5 audit.
+scripts/testing/debug-vm.sh:49 hardcodes the btrfs base disk, ignoring the profile-correct DISK_PATH from init_vm_paths (FS_PROFILE=zfs boots the wrong base or fatals); lib/vm-utils.sh:284 kill_qemu -9's and deletes the PID file without waiting, so a force-kill restore races the dying qemu's qcow2 lock and silently leaves the base image dirty (fix: wait for the PID); lib/vm-utils.sh:69 leaves PID_FILE/MONITOR_SOCK/SERIAL_LOG un-suffixed so parallel btrfs+zfs runs collide (fix: suffix by FS_PROFILE like DISK_PATH); run-test.sh:287 (and run-test-baremetal.sh:234) reports a completion-marker grep as ARCHSETUP_EXIT_CODE, not the installer's real exit — misleading since the installer runs set -e off and can error then still write the marker (fix: rename + capture the true status). Testinfra remains the real pass/fail backstop. See findings doc (S5).
+** TODO [#D] Gallery-widget prototype elisp bugs :bug:design:solo:quick:
+Grading: Minor severity x rare edge case (out-of-range input / cold byte-compile / interactive re-eval) = P4 = [#D]. Prototype code, all three Minor.
+docs/prototypes/gallery-widget.el:139 renders the readout from the unclamped value while the needle clamps 0-100, so at value 150 the needle pins at +60 degrees but the text reads "150%" (fix: clamp once, format both from it); :69 calls cl-loop without (require 'cl-lib) — works only via the autoload cookie, bites on a cold byte-compile (fix: add the require); :29 computes its dir from (or load-file-name buffer-file-name), both nil on interactive re-eval outside a load/file buffer (fix: fall back to default-directory). See findings doc (S7).
+** TODO [#D] Audit test-quality cluster (Python + elisp) :test:solo:
+Grading: no runtime behavior change; test-suite quality. Group of five weak/missing tests from the S6/S7 audit.
+scripts/testing/tests/test_desktop.py:96 passes a shell glob to `test -S`, which breaks on zero or multiple sockets (masked today because the test always skips); tests/gallery-tokens/test_gen_tokens.py:181 asserts properties too weak to notice the marker output is garbled (impossible input, so low); tests/gallery-widgets/test-gallery-widget.el:77 counts ticks via split-string + cl-count-if :start 1 (a coincidence of split semantics, not a match count); :47 tests the needle-angle helper's clamp but never the rendered readout at an out-of-range value (exactly why the S7 readout/needle bug ships green — add a gauge-level boundary case); :159 leaves gallery-widget-write-svg uncovered (add a Normal write-to-temp case). See findings doc (S6, S7).
+** TODO [#D] Test-framework + prototype refactor cluster :refactor:
+Grading: no behavior change; parking lot. Refactors from the S5-S7 audit, distinct from the installer refactor rollup above.
+scripts/testing/run-test.sh + run-test-baremetal.sh duplicate the run/poll/report skeleton and have drifted (VM uses setsid + copy helpers, baremetal uses nohup + hand-rolled sshpass scp) — extract the shared core so baremetal inherits the sturdier paths; run-maint-nspawn.sh:66 + run-maint-scenarios.sh:78 duplicate the transport-independent _scenario_var/_validate_scenario/run_scenario (a sourced lib/maint-scenario.sh); run-test.sh:251,265 uses two different mechanisms (pgrep vs ps|grep) for the same liveness check; docs/prototypes/gen_tokens.py:78 repeats the section-iteration skeleton across four emitters; gallery-widget.el:95,136 hardcodes SVG arc/hub path strings that duplicate the cx/cy/radius geometry (dial desyncs silently on a constant change); gallery-widget.el:72,84 leans on the private svg--append. See findings doc (S5, S6, S7).
 ** TODO [#D] Net doctor vNext :feature:dotfiles:network:
 Deferred from the [[file:docs/specs/2026-07-11-net-doctor-expansion-spec.org][net doctor expansion spec]] (IMPLEMENTED, v1 shipped): event-log correlation for the flaky/drops cluster (powersave, roaming stalls, USB autosuspend, no-reconnect-after-resume, firmware crashloop drop signatures — needs history a one-shot probe can't see); a DoT/DNSSEC-specific verdict distinguishing "the venue resolver mangles DNSSEC" from the generic DNS-not-resolving; per-profile autoconnect/duplicate-profile hygiene.