diff --git a/pkg/service/handlers/handlers_pairing.go b/pkg/service/handlers/handlers_pairing.go index b7e099d..9fd315e 100644 --- a/pkg/service/handlers/handlers_pairing.go +++ b/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 +// 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. diff --git a/pkg/service/handlers/server.go b/pkg/service/handlers/server.go index 32de894..9a21024 100644 --- a/pkg/service/handlers/server.go +++ b/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, diff --git a/pkg/service/health/checks_speaker_info.go b/pkg/service/health/checks_speaker_info.go index 137f279..3adcd06 100644 --- a/pkg/service/health/checks_speaker_info.go +++ b/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 +// " 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 .", + 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 — 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 .", - 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 == "" { diff --git a/pkg/service/health/checks_speaker_info_test.go b/pkg/service/health/checks_speaker_info_test.go index 5fc60f2..f7aa390 100644 --- a/pkg/service/health/checks_speaker_info_test.go +++ b/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) diff --git a/pkg/service/health/complete_pairing_test.go b/pkg/service/health/complete_pairing_test.go new file mode 100644 index 0000000..dd85aad --- /dev/null +++ b/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 := ` + + SoundTouch 20 + + http://aftertouch.local:8000 +` + + 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 := ` + + + http://aftertouch.local:8000 +` + + 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) + } + } +}