feat(health): operator-confirmable QuickFix to complete speaker pairing (#329)

Closes the loop on the empty-<margeAccountUUID> finding from
RegisterSpeakerInfoReachable. Operators in #329 quoted that finding
verbatim and asked "What is the recommended way to complete pairing?"
— the framework detected the condition but offered no in-UI recourse.

The QuickFix completes pairing in-place by dispatching through
setup.Manager.PairAccount, which tries HTTP /setMargeAccount first
and falls back to telnet `envswitch accountid set` — same code path
the existing POST /setup/pair-account/{deviceId} handler uses.

Account ID is picked at finding-time when a real (7-digit) account
directory already contains this device on disk (typical scenario:
AfterTouch remembers a previous pairing the speaker forgot). When
no such account exists, the executor generates a fresh 7-digit ID
via setup.GenerateAccountID at click time. Either way, the chosen
ID is named in the Confirm dialog and the CLI ManualCommand
fallback so the operator can see what's about to happen.

Architecturally: the FixID constant lives in the health package
alongside the check that emits the finding, but the executor is
registered from handlers/server.go where setup.Manager is
available. This keeps the health package's transitive dep surface
small (the boundary comment near speakerInfoXML deliberately
forbids importing setup, which would pull SSH/telnet/certmgr).

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

pkg/service/handlers/handlers_pairing.go

@@ -2,8 +2,11 @@ package handlers
 
 import (
 	"encoding/json"
+	"fmt"
 	"net/http"
+	"strings"
 
+	"github.com/gesellix/bose-soundtouch/pkg/service/health"
 	"github.com/gesellix/bose-soundtouch/pkg/service/setup"
 	"github.com/go-chi/chi/v5"
 )
@@ -117,6 +120,66 @@ func (s *Server) HandlePairAccount(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+// completeSpeakerPairingFix is the FixFunc registered for
+// (CheckIDSpeakerInfoReachable, FixIDCompleteSpeakerPairing). It
+// completes pairing on a speaker whose /info reports an empty
+// <margeAccountUUID> by:
+//
+//  1. Looking up the device's current IP via ListAllDevices.
+//  2. Choosing the pair-with account: target.Account when the
+//     detection-side suggestion populated it, else generating a fresh
+//     7-digit ID with setup.GenerateAccountID.
+//  3. Dispatching through setup.Manager.PairAccount, which tries
+//     /setMargeAccount over HTTP first and falls back to telnet.
+//
+// Returns a user-facing success message that names the chosen account
+// and the method that succeeded; the framework forwards it to the UI.
+func (s *Server) completeSpeakerPairingFix(target health.Target) (string, error) {
+	if target.Device == "" {
+		return "", fmt.Errorf("device is required")
+	}
+
+	deviceIP, err := s.resolveDeviceIDToIP(target.Device)
+	if err != nil {
+		return "", fmt.Errorf("locate device %s: %w", target.Device, err)
+	}
+
+	accountID := target.Account
+	if !setup.IsValidAccountID(accountID) {
+		known, _ := s.ds.ListAccounts()
+
+		generated, genErr := setup.GenerateAccountID(known)
+		if genErr != nil {
+			return "", fmt.Errorf("generate account ID: %w", genErr)
+		}
+
+		accountID = generated
+	}
+
+	var t setup.TelnetClient
+	if s.sm.NewTelnet != nil {
+		t = s.sm.NewTelnet(deviceIP)
+		if dialErr := t.Dial(); dialErr != nil {
+			t = nil
+		} else {
+			defer func() { _ = t.Close() }()
+		}
+	}
+
+	result, output, err := s.sm.PairAccount(deviceIP, accountID, t)
+	if err != nil {
+		return "", fmt.Errorf("pair speaker %s with account %s: %w (path output: %s)", target.Device, accountID, err, strings.TrimSpace(output))
+	}
+
+	method := result.Method
+	if method == "" {
+		method = "unknown"
+	}
+
+	return fmt.Sprintf("Paired speaker %s with account %s via %s. The speaker will re-fetch /full on its own; playback selection should start working within seconds.",
+		target.Device, accountID, method), nil
+}
+
 // jsonErrorBody is the static shape of error responses from this file.
 // Avoiding map[string]interface{} keeps errchkjson satisfied: the typed
 // struct guarantees encoding can't fail with a runtime type error.

pkg/service/handlers/server.go

@@ -133,6 +133,18 @@ func NewServer(ds *datastore.DataStore, sm *setup.Manager, serverURL string, red
 	health.RegisterPresetsCountCheck(s.healthRegistry, ds)
 	health.RegisterPresetsConsistencyCheck(s.healthRegistry, ds)
 	health.RegisterRefreshSourcesCheck(s.healthRegistry, ds)
+
+	// Health QuickFix executor for the empty-margeAccountUUID
+	// finding from RegisterSpeakerInfoReachable. Lives here (not in
+	// the health package) because the executor needs setup.Manager
+	// to drive PairAccount — and the health package deliberately
+	// avoids importing setup to keep its transitive dep surface
+	// small (see the boundary comment near speakerInfoXML).
+	s.healthRegistry.RegisterFix(
+		health.CheckIDSpeakerInfoReachable,
+		health.FixIDCompleteSpeakerPairing,
+		s.completeSpeakerPairingFix,
+	)
 	health.RegisterDNSSanityCheck(
 		s.healthRegistry,
 		s.GetDNSRunning,

pkg/service/health/checks_speaker_info.go

@@ -9,10 +9,104 @@ import (
 	"github.com/gesellix/bose-soundtouch/pkg/service/datastore"
 )
 
+// suggestAccountForPairing picks an account ID to pre-fill into the
+// "Complete pairing" QuickFix's Target.Account. Returns the first
+// real-looking (7-digit) account directory that already contains the
+// device's deviceID on disk — typical scenario: AfterTouch and the
+// speaker were partially paired earlier, the speaker forgot but our
+// datastore remembers. Returns "" when no such account exists, in
+// which case the executor generates a fresh ID at click time.
+func suggestAccountForPairing(ds *datastore.DataStore, deviceID string) string {
+	if ds == nil {
+		return ""
+	}
+
+	for _, acc := range ds.AllAccountsForDevice(deviceID) {
+		if isSevenDigitAccountID(acc) {
+			return acc
+		}
+	}
+
+	return ""
+}
+
+// isSevenDigitAccountID mirrors setup.IsValidAccountID without
+// importing the setup package (which would pull in SSH/telnet/certmgr
+// transitively — see the boundary comment near speakerInfoXML).
+func isSevenDigitAccountID(s string) bool {
+	if len(s) != 7 {
+		return false
+	}
+
+	for _, ch := range s {
+		if ch < '0' || ch > '9' {
+			return false
+		}
+	}
+
+	return true
+}
+
+// buildEmptyMargeFinding constructs the "Speaker reports an empty
+// <margeAccountUUID>" finding with a QuickFix that completes pairing
+// in-place plus a ManualCommand fallback the operator can run from a
+// shell on the speaker's LAN. The executor is registered separately by
+// the caller that has setup.Manager access.
+//
+// Target.Account intentionally carries the *pair-with* account
+// (whichever we picked from disk), not the current binding — that's
+// what the fix executor needs at click time, and the framework passes
+// Finding.Target through to the FixFunc verbatim. The UI grouping
+// follows Target.Account too; that's a minor side effect we accept.
+func buildEmptyMargeFinding(ds *datastore.DataStore, _ Target, deviceID, ipAddress string) Finding {
+	suggested := suggestAccountForPairing(ds, deviceID)
+
+	confirm := "Pair this speaker with a Marge account so playback selection stops failing with INVALID_SOURCE? "
+
+	if suggested != "" {
+		confirm += "AfterTouch will reuse the existing account " + suggested + " (already on disk for this device)."
+	} else {
+		confirm += "AfterTouch will generate a fresh 7-digit account ID for this speaker (private deployment — the ID is opaque)."
+	}
+
+	cliAccount := suggested
+	if cliAccount == "" {
+		cliAccount = "<7-digit-account-id>"
+	}
+
+	return Finding{
+		Severity: SeverityWarning,
+		Target:   Target{Account: suggested, Device: deviceID},
+		Message:  "Speaker reports an empty <margeAccountUUID>.",
+		Details:  "The speaker is reachable but isn't bound to any Marge account. Playback selection will fail with INVALID_SOURCE until pairing completes. See discussion #223 and issue #329.",
+		QuickFixes: []QuickFix{{
+			ID:      FixIDCompleteSpeakerPairing,
+			Label:   "Complete pairing",
+			Confirm: confirm,
+		}},
+		ManualCommands: []ManualCommand{{
+			Label:   "Or pair from a host on the speaker's LAN:",
+			Command: fmt.Sprintf("soundtouch-cli setup pair --host=%s --mode=bare --account=%s", ipAddress, cliAccount),
+			Hint:    "The --mode=bare path sends just setMargeAccount without the full state machine; sufficient on FW 27.0.6.",
+		}},
+	}
+}
+
 // CheckIDSpeakerInfoReachable is the registry id of the speaker
 // reachability check.
 const CheckIDSpeakerInfoReachable = "speaker_info_reachable"
 
+// FixIDCompleteSpeakerPairing is the QuickFix that completes pairing
+// on a speaker that reports an empty <margeAccountUUID> — i.e. the
+// speaker is reachable and configured to point at AfterTouch but
+// hasn't been bound to a Marge account, so every playback selection
+// fails with INVALID_SOURCE (see #329, discussion #223).
+//
+// The executor lives in pkg/service/handlers/server.go (where
+// setup.Manager is available); the constant is defined here so the
+// check that emits the finding stays self-contained.
+const FixIDCompleteSpeakerPairing = "complete_speaker_pairing"
+
 // speakerInfoXML mirrors only the fields we need from the
 // speaker's :8090/info XML response. Duplicated here (rather than
 // imported from pkg/service/setup) to keep the health package
@@ -63,22 +157,24 @@ func runSpeakerInfoReachable(ds *datastore.DataStore) []Finding {
 			continue
 		}
 
-		findings = append(findings, probeAndAssessSpeaker(dev.AccountID, dev.DeviceID, dev.IPAddress)...)
+		findings = append(findings, probeAndAssessSpeaker(ds, dev.AccountID, dev.DeviceID, dev.IPAddress)...)
 	}
 
 	return findings
 }
 
-func probeAndAssessSpeaker(account, deviceID, ipAddress string) []Finding {
+func probeAndAssessSpeaker(ds *datastore.DataStore, account, deviceID, ipAddress string) []Finding {
 	probeURL := fmt.Sprintf("http://%s:8090/info", ipAddress)
-	return probeAndAssessSpeakerWithURL(account, deviceID, probeURL)
+	return probeAndAssessSpeakerWithURL(ds, account, deviceID, ipAddress, probeURL)
 }
 
 // probeAndAssessSpeakerWithURL is the same as probeAndAssessSpeaker
 // but takes the full URL directly. Used by tests that need to point
 // at an httptest.Server, since those bind to random ports rather
-// than :8090.
-func probeAndAssessSpeakerWithURL(account, deviceID, probeURL string) []Finding {
+// than :8090. ds and ipAddress feed the QuickFix attached to the
+// empty-margeAccountUUID finding (suggestion for an existing
+// account-on-disk, and the CLI ManualCommand fallback).
+func probeAndAssessSpeakerWithURL(ds *datastore.DataStore, account, deviceID, ipAddress, probeURL string) []Finding {
 	target := Target{Account: account, Device: deviceID}
 
 	res := ProbeGet(context.Background(), probeURL, 2*time.Second)
@@ -119,12 +215,7 @@ func probeAndAssessSpeakerWithURL(account, deviceID, probeURL string) []Finding
 	var out []Finding
 
 	if parsed.MargeAccountUUID == "" {
-		out = append(out, Finding{
-			Severity: SeverityWarning,
-			Target:   target,
-			Message:  "Speaker reports an empty <margeAccountUUID>.",
-			Details:  "The speaker is reachable but isn't bound to any Marge account. Playback selection will fail with INVALID_SOURCE until pairing completes. See discussion #223 for the full symptom chain.",
-		})
+		out = append(out, buildEmptyMargeFinding(ds, target, deviceID, ipAddress))
 	}
 
 	if parsed.MargeURL == "" {

pkg/service/health/checks_speaker_info_test.go

@@ -106,14 +106,14 @@ func TestSpeakerInfoReachable_FlagsEmptyMargeAccountUUID(t *testing.T) {
 		ID:    CheckIDSpeakerInfoReachable,
 		Title: "Speakers respond on :8090/info",
 		Run: func() []Finding {
-			return probeAndAssessSpeaker("1000001", "DEVICEID01", hostport+"_skip_port_append")
+			return probeAndAssessSpeaker(ds, "1000001", "DEVICEID01", hostport+"_skip_port_append")
 		},
 	})
 
 	// Probe directly with the actual hostport to bypass the
 	// ":8090" formatting in production code (since httptest
 	// servers can't bind to 8090 in tests).
-	got := probeAndAssessSpeakerWithURL("1000001", "DEVICEID01", "http://"+hostport+"/info")
+	got := probeAndAssessSpeakerWithURL(nil, "1000001", "DEVICEID01", hostport, "http://"+hostport+"/info")
 
 	if len(got) != 1 || got[0].Severity != SeverityWarning {
 		t.Fatalf("expected one warning, got %+v", got)
@@ -136,7 +136,7 @@ func TestSpeakerInfoReachable_NoFindingsWhenHealthy(t *testing.T) {
 
 	_, hostport := stubSpeakerServer(t, body)
 
-	got := probeAndAssessSpeakerWithURL("1000001", "DEVICEID01", "http://"+hostport+"/info")
+	got := probeAndAssessSpeakerWithURL(nil, "1000001", "DEVICEID01", hostport, "http://"+hostport+"/info")
 	if len(got) != 0 {
 		t.Errorf("expected no findings, got %+v", got)
 	}
@@ -149,7 +149,7 @@ func TestSpeakerInfoReachable_FlagsHTTPError(t *testing.T) {
 	defer srv.Close()
 
 	u, _ := url.Parse(srv.URL)
-	got := probeAndAssessSpeakerWithURL("1000001", "DEVICEID01", "http://"+u.Host+"/info")
+	got := probeAndAssessSpeakerWithURL(nil, "1000001", "DEVICEID01", u.Host, "http://"+u.Host+"/info")
 
 	if len(got) != 1 || got[0].Severity != SeverityWarning {
 		t.Fatalf("expected one warning for HTTP 500, got %+v", got)
@@ -167,7 +167,7 @@ func TestSpeakerInfoReachable_FlagsMalformedXML(t *testing.T) {
 	defer srv.Close()
 
 	u, _ := url.Parse(srv.URL)
-	got := probeAndAssessSpeakerWithURL("1000001", "DEVICEID01", "http://"+u.Host+"/info")
+	got := probeAndAssessSpeakerWithURL(nil, "1000001", "DEVICEID01", u.Host, "http://"+u.Host+"/info")
 
 	if len(got) != 1 || got[0].Severity != SeverityWarning {
 		t.Fatalf("expected one warning for malformed XML, got %+v", got)

pkg/service/health/complete_pairing_test.go

@@ -0,0 +1,137 @@
+package health
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/gesellix/bose-soundtouch/pkg/service/datastore"
+)
+
+// TestEmptyMargeAccountFinding_CarriesCompletePairingQuickFix verifies
+// the finding for the GH-329 case (speaker /info has empty
+// margeAccountUUID) carries the Complete-pairing QuickFix plus the
+// CLI ManualCommand fallback. The executor itself lives in
+// handlers/server.go (where setup.Manager is available); this test
+// just pins the finding shape.
+func TestEmptyMargeAccountFinding_CarriesCompletePairingQuickFix(t *testing.T) {
+	body := `<?xml version="1.0" encoding="UTF-8"?>
+<info deviceID="DEVICEID01">
+    <name>SoundTouch 20</name>
+    <margeAccountUUID></margeAccountUUID>
+    <margeURL>http://aftertouch.local:8000</margeURL>
+</info>`
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/xml")
+		_, _ = w.Write([]byte(body))
+	}))
+	defer srv.Close()
+
+	u, _ := url.Parse(srv.URL)
+
+	tempDir, _ := os.MkdirTemp("", "complete-pairing-empty-*")
+	defer func() { _ = os.RemoveAll(tempDir) }()
+
+	ds := datastore.NewDataStore(tempDir)
+
+	got := probeAndAssessSpeakerWithURL(ds, "default", "DEVICEID01", u.Host, "http://"+u.Host+"/info")
+	if len(got) != 1 {
+		t.Fatalf("expected one finding for empty margeAccountUUID, got %d: %+v", len(got), got)
+	}
+
+	if got[0].Severity != SeverityWarning {
+		t.Errorf("expected SeverityWarning, got %v", got[0].Severity)
+	}
+
+	if len(got[0].QuickFixes) != 1 || got[0].QuickFixes[0].ID != FixIDCompleteSpeakerPairing {
+		t.Errorf("expected QuickFix with ID=%s; got %+v", FixIDCompleteSpeakerPairing, got[0].QuickFixes)
+	}
+
+	if !strings.Contains(got[0].QuickFixes[0].Confirm, "fresh 7-digit account") {
+		t.Errorf("expected Confirm to mention fresh-account fallback when no on-disk account exists; got %q", got[0].QuickFixes[0].Confirm)
+	}
+
+	if len(got[0].ManualCommands) != 1 || !strings.Contains(got[0].ManualCommands[0].Command, "setup pair") {
+		t.Errorf("expected ManualCommand with CLI setup pair invocation; got %+v", got[0].ManualCommands)
+	}
+
+	if !strings.Contains(got[0].ManualCommands[0].Command, "--mode=bare") {
+		t.Errorf("expected ManualCommand to use --mode=bare path; got %q", got[0].ManualCommands[0].Command)
+	}
+}
+
+// TestEmptyMargeAccountFinding_SuggestsExistingAccountWhenOnDisk pins
+// the on-disk-account suggestion: when an account directory matching
+// the 7-digit shape already contains this device (typical state when
+// the speaker was partially paired earlier but forgot its binding),
+// the QuickFix target carries that account and the Confirm copy
+// names it explicitly.
+func TestEmptyMargeAccountFinding_SuggestsExistingAccountWhenOnDisk(t *testing.T) {
+	body := `<?xml version="1.0" encoding="UTF-8"?>
+<info deviceID="DEVICEID01">
+    <margeAccountUUID></margeAccountUUID>
+    <margeURL>http://aftertouch.local:8000</margeURL>
+</info>`
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(body))
+	}))
+	defer srv.Close()
+
+	u, _ := url.Parse(srv.URL)
+
+	tempDir, _ := os.MkdirTemp("", "complete-pairing-suggest-*")
+	defer func() { _ = os.RemoveAll(tempDir) }()
+
+	if err := os.MkdirAll(filepath.Join(tempDir, "accounts", "1234567", "devices", "DEVICEID01"), 0755); err != nil {
+		t.Fatalf("mkdir: %v", err)
+	}
+
+	ds := datastore.NewDataStore(tempDir)
+
+	got := probeAndAssessSpeakerWithURL(ds, "default", "DEVICEID01", u.Host, "http://"+u.Host+"/info")
+	if len(got) != 1 {
+		t.Fatalf("expected one finding, got %d: %+v", len(got), got)
+	}
+
+	if got[0].Target.Account != "1234567" {
+		t.Errorf("expected Target.Account to be the on-disk suggestion 1234567, got %q", got[0].Target.Account)
+	}
+
+	if !strings.Contains(got[0].QuickFixes[0].Confirm, "1234567") {
+		t.Errorf("expected Confirm to name the on-disk account 1234567, got %q", got[0].QuickFixes[0].Confirm)
+	}
+
+	if !strings.Contains(got[0].ManualCommands[0].Command, "--account=1234567") {
+		t.Errorf("expected ManualCommand to pre-fill the on-disk account, got %q", got[0].ManualCommands[0].Command)
+	}
+}
+
+// TestIsSevenDigitAccountID pins the local validator we use in
+// suggestAccountForPairing to avoid importing setup.
+func TestIsSevenDigitAccountID(t *testing.T) {
+	cases := []struct {
+		in   string
+		want bool
+	}{
+		{"1234567", true},
+		{"0000001", true},
+		{"9999999", true},
+		{"123456", false},   // 6 digits
+		{"12345678", false}, // 8 digits
+		{"default", false},
+		{"123456a", false},
+		{"", false},
+	}
+
+	for _, tc := range cases {
+		if got := isSevenDigitAccountID(tc.in); got != tc.want {
+			t.Errorf("isSevenDigitAccountID(%q) = %v, want %v", tc.in, got, tc.want)
+		}
+	}
+}