docs: record spec-review findings on net + bt doctor specs

Reviewed both DRAFT expansion specs against the live engines. Both stay Not ready: three open decisions apiece plus one blocking finding each.

Net: the Security dimension claims connection names are already redacted, but redact.py covers only SSID, MAC, IP, secret-keys, and portal_url, so the new keyfile-perms and rival-manager verdicts would leak a connection name into the wall and --json. Four non-blocking fixes alongside it: nm-restart's scope was overstated, is-enabled isn't used yet, some message text was listed as classifier verdicts, and the priv.py dispatch integration wants naming.

Bt: the doctor has no named-verdict layer to hang the proposed no-adapter-firmware and powered-off-persistent verdicts on. A step carries a pass/fail/warn/info status and the run an ok/warn/fail overall, nothing more. How each new verdict is represented has to be settled before Phase 1. Three non-blocking notes cover the widening sudoers surface, the real diagnostic surface, and the bounded journalctl precedent.

Both designs are sound and target verified-real gaps: neither engine reads the config or logs the specs propose to add.

2 files changed, 43 insertions, 0 deletions

diff --git a/docs/specs/2026-07-11-bt-doctor-expansion-spec.org b/docs/specs/2026-07-11-bt-doctor-expansion-spec.org
index 0775a18..bced674 100644
--- a/docs/specs/2026-07-11-bt-doctor-expansion-spec.org
+++ b/docs/specs/2026-07-11-bt-doctor-expansion-spec.org
@@ -8,6 +8,7 @@
 :PROPERTIES:
 :ID:       3d4d61c4-e5df-44e9-b8e0-40b31452c3f7
 :END:
+- [2026-07-11 Sat @ 00:59:30 -0500] DRAFT — reviewed (spec-review). Stays DRAFT: three decisions open plus one =:blocking:= finding (the bt doctor has no named-verdict layer, so the proposed =no-adapter-firmware=/=powered-off-persistent= verdicts need their representation defined first). Design confirmed against the live engine — the two target gaps (kernel-log firmware hint, boot-persistence read) genuinely do not exist yet. Findings in =* Review findings=.
 - [2026-07-11 Sat @ 00:08:41 -0500] DRAFT — drafted. Extends the existing bt doctor (=~/.dotfiles/bluetooth/=, shipped) using the bluetooth half of the failure taxonomy ([[file:../design/2026-07-10-net-bt-failure-taxonomy.org][2026-07-10-net-bt-failure-taxonomy.org]]). Grounded in a read of the live engine, not memory.
 
 * Metadata
@@ -128,6 +129,20 @@ Decision (proposed): defer to vNext. v1 keeps re-pair strictly user-initiated; t
 Consequences: no risk of the doctor offering a destructive fix for a transient condition in v1; harder — the "keeps failing to connect" case stays unnamed for now, which is the one cluster-3 gap a user might reasonably expect the doctor to catch.
 Owner: Craig. By: before Phase 1 (scope confirmation).
 
+* Review findings [/]
+
+** TODO The bt doctor has no named-verdict layer, so "emit a verdict named X" is undefined :blocking:
+The spec proposes named verdicts throughout — =no-adapter-firmware=, and =powered-off-persistent= "distinct from =powered-off=" — mirroring the net doctor's action-identifier vocabulary. But the bt doctor has no such layer. Verified: a step carries a =status= ∈ =pass/fail/warn/info= and the run has an =overall= ∈ =ok/warn/fail= (=doctor.py:182-183=); there are no verdict identifiers like =powered-off= or =power-on= as *names* — those are message text and the =power-on= auto-fix tier, not a verdict enum. So "add a =powered-off-persistent= verdict distinct from =powered-off=" has no existing mechanism to attach to, and the implementer would have to invent the representation: is the distinction a new =status= value, a field on the powered step, a message variant, an added =fix= identifier? Whatever it is, it also touches =format_doctor_human= (=cli.py:94=) and any consumer of =overall=. Define how each new "verdict" is represented in the step-status model before Phase 1 — this is the one place the spec would force the implementer to invent product/data behavior. (blocking)
+
+** TODO New root repairs expand the NOPASSWD sudoers surface — name it in rollout
+=priv.py= today exposes a single privileged verb, =restart-bluetooth= (=priv.py:27-34=), backed by a NOPASSWD sudoers entry. The three new persistent fixes (edit =/etc/bluetooth/main.conf=, =systemctl enable bluetooth=, edit =/etc/tlp.conf=) are each a new root-capable verb and each widens the passwordless-sudo grant from "restart one service" to "edit system config files as root." That is a heavier privilege surface than the existing tier; the Rollout/compatibility dimension should name the sudoers expansion explicitly and confirm each new verb is a tightly-scoped command (not a general "run as root"), so the privilege model's Confirm floor is the *second* guard, not the only one. (non-blocking)
+
+** TODO The diagnostic surface is =bt doctor='s JSON, not a =bt diag= subcommand
+Phase 0 says "=bt diag --json= (or the equivalent) shows the new signals." Verified: there is no =bt diag= subcommand; =diagnose()= is the report authority (=doctor.py:174=), =doctor()= wraps it (=:207=), and JSON is already the default output of the doctor subparser (=cli.py:173= — the =--json= flag is effectively redundant). Point Phase 0 at the real surface: the new signals land in =diagnose()='s report dict and render through =format_doctor_human= (=cli.py:94=). Cosmetic — the author already hedged with "(or the equivalent)." (non-blocking)
+
+** TODO Firmware-hint probe is new but has a bounded precedent — cite it
+Phase 0's =journalctl -k= reader is genuinely new (=_adapter_step= reads nothing beyond =btctl.show=, =doctor.py:46-50=), but the engine already runs a bounded =journalctl -u bluetooth -n 1= for service-log evidence (=doctor.py:84=, timeout 5s via =cmd.run=). Cite it as the precedent so the implementer reuses the same bounded-=cmd.run= shape for the kernel-log read rather than introducing an unbounded call. Confirms feasibility; not a gap. (non-blocking)
+
 * Implementation phases
 
 Each phase leaves the tree green and independently useful, as the existing bt phases did.
@@ -182,3 +197,8 @@ The bt-audio-profile expansion (vNext) overlaps the audio taxonomy's Bluetooth-m
 - What: drafted the bt doctor expansion from the bluetooth half of the failure taxonomy. v1 adds the dmesg firmware-hint probe (cluster 1) and the boot-enablement probe (cluster 2); the pairing/connection/audio expansions are staged to vNext.
 - Why: the taxonomy showed the doctor's chain is structurally right but blind at its ends — a no-adapter verdict that won't name the firmware blob the kernel already logged, and a power-on that doesn't persist across boots.
 - Artifacts: [[file:../design/2026-07-10-net-bt-failure-taxonomy.org][the net/bt failure taxonomy]]; code read across =~/.dotfiles/bluetooth/src/bt/= (=doctor.py=, =repair.py=, =btctl.py=, =sysio.py=, =audio.py=, =priv.py=).
+
+** 2026-07-11 Sat @ 00:59:30 -0500 — Claude Code (archsetup) — Reviewer
+- What: ran spec-review. Rubric =Not ready=. Recorded four findings, one =:blocking:= (no named-verdict layer to attach the proposed verdicts to) and three non-blocking (sudoers-surface expansion, the real diagnostic surface, the bounded firmware-read precedent). The three proposed decisions remain open.
+- Why: the design is sound and the code read confirmed both target gaps are real — the engine reads no kernel log for firmware hints and nothing for boot persistence (=main.conf=/=is-enabled=/=tlp.conf= all grep-clean). What holds the rubric is that the spec borrows the net doctor's verdict-naming language, but the bt doctor is step-status-based (=status= pass/fail/warn/info, =overall= ok/warn/fail, =doctor.py:182-183=) with no verdict enum — so how each new "verdict" is represented is undefined. Once that representation is decided and Craig accepts the three decisions, this reaches =Ready with caveats= (shared privilege-model code being the sequencing caveat, already in Risks).
+- Artifacts: engine facts verified across =doctor.py= (:25 AUTO_FIX, :46-50 no-adapter branch, :84 bounded journalctl precedent, :100-107 powered step, :174-183 diagnose/overall), =priv.py:27-34= (single =restart-bluetooth= verb), =cli.py:94/173= (formatter + json default), =redact.py:11-20=. Findings in =* Review findings=.
diff --git a/docs/specs/2026-07-11-net-doctor-expansion-spec.org b/docs/specs/2026-07-11-net-doctor-expansion-spec.org
index aed22b8..9f51d74 100644
--- a/docs/specs/2026-07-11-net-doctor-expansion-spec.org
+++ b/docs/specs/2026-07-11-net-doctor-expansion-spec.org
@@ -8,6 +8,7 @@
 :PROPERTIES:
 :ID:       ce29b103-ed9d-4f56-bf8c-9ed8fe680ff3
 :END:
+- [2026-07-11 Sat @ 00:59:30 -0500] DRAFT — reviewed (spec-review). Stays DRAFT: three decisions open plus one =:blocking:= finding (connection names are not redacted today, contra the spec's Security dimension). Design confirmed against the live engine — the two target gaps (=is-enabled= reads, =system-connections= reads) genuinely do not exist yet, so the expansion targets real ground. Findings in =* Review findings=.
 - [2026-07-11 Sat @ 00:08:41 -0500] DRAFT — drafted. Extends the existing net doctor (=~/.dotfiles/net/=, shipped) using the network half of the failure taxonomy ([[file:../design/2026-07-10-net-bt-failure-taxonomy.org][2026-07-10-net-bt-failure-taxonomy.org]]). Grounded in a read of the live engine, not memory.
 
 * Metadata
@@ -140,6 +141,23 @@ Decision (proposed): we will adopt it verbatim — Privileged repairs run where
 Consequences: the net doctor's root repairs are consistent with audio/bt/maint; harder — it couples this spec to the shared privilege-model task, so the ordering across panels has to be settled (which panel lands the shared code).
 Owner: Craig. By: before Phase 1 lands.
 
+* Review findings [/]
+
+** TODO Connection names are not redacted today, but the spec assumes they are :blocking:
+The Security & privacy dimension states "SSIDs and connection names are already redacted by =redact.py=." Verified against the live engine: =redact.py= redacts SSID, MAC, IP, secret-keys, and =portal_url= only (=redact.py:9-53=) — there is no connection-name redaction. The new =rival-manager= and =keyfile-perms= verdicts surface a connection name and a =/etc/NetworkManager/system-connections/*.nmconnection= path (the file basename is the connection name) into the wall and the =--json= output. So the spec ships a privacy leak it believes is already closed. V1 must either add connection-name / keyfile-basename redaction to =redact.py= (and cover it in the redaction test), or make an explicit, recorded decision that connection names are non-sensitive display content. Blocking because it is a privacy contract the spec states as already-true and isn't. (blocking)
+
+** TODO "One =nm-restart= verdict covers the whole control plane" overstates current scope
+Problem/Context and the first Alternative rest the motivation on the claim that the doctor "currently has one verdict (=nm-restart=) covering the whole control plane." Verified: =nm-restart= is one narrow action for a dead NetworkManager *service* only (=classify.py:96-97=, =repair.py:705=); the classifier already carries granular actions (=reset=, =bounce=, =rfkill=, =manage-device=, =resolved-restart=…). The real, verified gap is not "one verdict for everything" — it is that *no* probe detects a rival manager, a masked NM, or a bad keyfile (=is-enabled= and =system-connections= reads genuinely do not exist: grep-clean). Reword the motivation onto those three missing detections so the argument rests on a true premise. Does not change the design; the gap it targets is real. (non-blocking)
+
+** TODO "=systemctl is-enabled= already used by the engine" is inaccurate
+External APIs & deps says =systemctl is-active/is-enabled= are "already used by the engine." Verified: only =is-active= is used (=cmd.py:27=); =is-enabled= has zero hits. Not a design problem — Phase 0 adds it — but the readiness claim should say =is-active= is used today and =is-enabled= is new surface, so the implementer knows to add the bounded call and its fake rather than assuming a helper exists. (non-blocking)
+
+** TODO Problem/Context lists message text as if it were classifier verdicts
+The "already reaches six clusters" evidence lists =no-hardware=, =DHCP-failed=, =DNS-not-resolving=, =dns-override= as classifier verdicts. Verified: the first three are message text, not action identifiers, and =dns-override= is emitted only by the doctor's =FIX_CHAINS= (=doctor.py:24=), not by =classify=. The real classifier action identifiers are =portal=, =vpn-policy=, =tunnel-down=, =nm-restart=, =resolved-restart=, =rfkill=, =reset=, =manage-device=, =dns-test=, =proxy=, =clock-sync=, =bounce= (=classify.py=). Cite those so the six-cluster claim is checkable against the code. Cosmetic — the conclusion holds. (non-blocking)
+
+** TODO Name the =priv.py= / =repair.py= integration point for the new privileged repairs
+The spec says it reuses =priv.py=, which is correct, but the phase plan does not name the concrete integration: privileged repairs dispatch through =priv.py='s =VERBS= table (=priv.py:150=) and =repair.py='s =ACTIONS= registry (=repair.py:695=), not a new path. Phase 1 should state that =disable-rival=, =unmask-nm=, and =chmod-keyfile= are added as =VERBS= + =ACTIONS= entries, so the implementer extends the existing dispatch rather than building a parallel one. (non-blocking)
+
 * Implementation phases
 
 Each phase leaves the tree green and independently useful, as the existing net phases did.
@@ -195,3 +213,8 @@ Coupling to the shared privilege-model task means this spec can't fully land unt
 - What: drafted the net doctor expansion from the network half of the failure taxonomy. v1 adds the control-plane cluster (rival-manager, NM-masked, keyfile-perms) and sharpens the auth verdict; the flaky/drops cluster is staged to vNext.
 - Why: the taxonomy showed the net doctor already reaches six of eight clusters, and the two it misses (control plane, auth naming) are where its verdicts are wrong or vague today.
 - Artifacts: [[file:../design/2026-07-10-net-bt-failure-taxonomy.org][the net/bt failure taxonomy]]; code read across =~/.dotfiles/net/src/net/= (=classify.py=, =diag.py=, =doctor.py=, =repair.py=, =priv.py=).
+
+** 2026-07-11 Sat @ 00:59:30 -0500 — Claude Code (archsetup) — Reviewer
+- What: ran spec-review. Rubric =Not ready=. Recorded five findings, one =:blocking:= (connection-name redaction assumed but absent) and four non-blocking accuracy/integration corrections. The three proposed decisions remain open.
+- Why: the design is sound and the code read confirmed both target gaps are real (=is-enabled= and =system-connections= reads do not exist in the engine today), so the expansion is well-grounded. What holds the rubric is the privacy claim the spec states as already-true (=redact.py= redacts SSID/MAC/IP/secret-keys/portal_url only — not connection names, =redact.py:9-53=) and the still-open decisions. Once the redaction finding is dispositioned and Craig accepts the three decisions, this reaches =Ready with caveats= (the cross-panel shared privilege-model code is the remaining sequencing caveat, already recorded in Risks).
+- Artifacts: engine facts verified across =classify.py= (:96-97, :55-62), =diag.py= (:518-537, bounded probes), =doctor.py= (:112-116 auth-reason), =cmd.py:27= (=is-active= only), =priv.py:150=/=repair.py:695= (dispatch registry), =redact.py:9-53=. Findings in =* Review findings=.