feat(health): operator-confirmable QuickFix to reassign canonical source IDs

og-gh's #343 reproducer is built-in radio sources sitting on non-canonical IDs (the 2000001+i fallback that GetConfiguredSources hands out when on-disk sources lack canonical IDs). After re-pair churn, presets binding by <sourceid> end up rebound to whichever source happened to get the colliding numeric ID — silently rewriting e.g. a TUNEIN preset to RADIOPLAYER on the next /full fetch. The strict-match commit (aa449fb) keeps that drift from corrupting emission downstream, but the underlying Sources.xml is still wrong and the operator has to either pull-from-speaker (online) or hand-edit XML (tedious). This commit adds an offline QuickFix that rewrites the source IDs in Sources.xml back to canonical (TUNEIN→10004, INTERNET_RADIO→10002, LOCAL_INTERNET_RADIO→10003, RADIO_BROWSER→10005) and updates every <sourceid> reference in Presets.xml/Recents.xml in lockstep. Skipped when the canonical ID is already in use by another source (e.g. duplicate TUNEIN entries from manual XML editing) — collisions need operator review. Idempotent: a second click is a no-op when everything is already canonical. The fix is reachable from the consistency check finding, gated by the framework's standard Confirm dialog which enumerates the exact ID rewrites before executing. No speaker contact required; the speaker re-fetches /full on its own and picks up the new IDs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-08-19 00:56:16 +00:00 · 2026-05-20 22:41:32 +02:00
parent 99d7111514
commit 884e19c791
2 changed files with 459 additions and 0 deletions
@@ -20,6 +20,13 @@ const CheckIDPresetsConsistency = "presets_recents_sources_consistency"
 // speaker isn't currently targeting it.
 const FixIDDeleteOrphanAccountEntry = "delete_orphan_account_entry"

+// FixIDReclassifyCanonicalSourceIDs is the QuickFix that rewrites
+// non-canonical IDs on built-in radio sources back to their canonical
+// values (TUNEIN→10004, INTERNET_RADIO→10002, …), and updates any
+// preset/recent <sourceid> references to match. Offline-only — no
+// speaker access required.
+const FixIDReclassifyCanonicalSourceIDs = "reclassify_canonical_source_ids"
+
 // speakerPresetsConsistencyXML mirrors enough of :8090/presets to extract
 // slot id, source/location and itemName for cross-side comparison.
 type speakerPresetsConsistencyXML struct {
@@ -78,6 +85,10 @@ func RegisterPresetsConsistencyCheck(r *Registry, ds *datastore.DataStore) {
 	r.RegisterFix(CheckIDPresetsConsistency, FixIDDeleteOrphanAccountEntry, func(target Target) (string, error) {
 		return deleteOrphanAccountEntry(ds, target)
 	})
+
+	r.RegisterFix(CheckIDPresetsConsistency, FixIDReclassifyCanonicalSourceIDs, func(target Target) (string, error) {
+		return reclassifyCanonicalSourceIDs(ds, target)
+	})
 }

 func runPresetsConsistencyCheck(ds *datastore.DataStore) []Finding {
@@ -211,6 +222,225 @@ func deleteOrphanAccountEntry(ds *datastore.DataStore, target Target) (string, e
 // package's exported surface.
 const accountIDDefaultPlaceholder = "default"

+// reclassifiableSource captures a single built-in radio source whose
+// on-disk ID drifted from the canonical value. Built up by
+// findReclassifiableSources and consumed by reclassifyCanonicalSourceIDs.
+type reclassifiableSource struct {
+	OldID   string
+	NewID   string
+	KeyType string // TUNEIN / INTERNET_RADIO / …
+	ProvID  string // canonical sourceproviderid (e.g. "25")
+}
+
+// canonicalIDByKeyType returns the canonical built-in source ID for one
+// of the four well-known radio provider key types, or ("", "") for any
+// other type. Mirrors datastore.getDefaultSources() and
+// canonicalDefaultsByType in pkg/service/marge.
+func canonicalIDByKeyType(keyType string) (id, providerID string) {
+	switch keyType {
+	case "INTERNET_RADIO":
+		return "10002", "2"
+	case "LOCAL_INTERNET_RADIO":
+		return "10003", "11"
+	case "TUNEIN":
+		return "10004", "25"
+	case "RADIO_BROWSER":
+		return "10005", "39"
+	}
+
+	return "", ""
+}
+
+// findReclassifiableSources walks a ConsistencyView's sources and
+// returns the entries that:
+//   - have a SourceKeyType matching one of the four built-in radio
+//     providers (the ones with a canonical ID),
+//   - currently sit on a non-canonical ID, and
+//   - would not collide with another source already at that canonical
+//     ID.
+//
+// The collision check is intentionally strict — when two entries claim
+// the same SourceKeyType, leaving them in place is safer than guessing
+// which one is the "real" one and leaving the other broken.
+func findReclassifiableSources(v ConsistencyView) []reclassifiableSource {
+	usedIDs := map[string]bool{}
+
+	for _, s := range v.Sources {
+		if s.ID != "" {
+			usedIDs[s.ID] = true
+		}
+	}
+
+	var out []reclassifiableSource
+
+	for _, s := range v.Sources {
+		newID, providerID := canonicalIDByKeyType(s.Type)
+		if newID == "" || s.ID == newID {
+			continue
+		}
+
+		// Don't try to re-classify if the canonical ID is already
+		// occupied by a different source — would create a collision
+		// the rest of the codebase isn't prepared to handle.
+		if usedIDs[newID] {
+			continue
+		}
+
+		out = append(out, reclassifiableSource{
+			OldID:   s.ID,
+			NewID:   newID,
+			KeyType: s.Type,
+			ProvID:  providerID,
+		})
+	}
+
+	return out
+}
+
+func reclassifyDetailMessage(in []reclassifiableSource) string {
+	out := "Each preset binding by ID may end up bound to the wrong source after re-pair churn — exactly the GH-343 footprint. Re-classifying restores canonical IDs:\n"
+
+	for _, r := range in {
+		out += "  • " + r.KeyType + ": " + r.OldID + " → " + r.NewID + " (sourceproviderid " + r.ProvID + ")\n"
+	}
+
+	return out
+}
+
+func reclassifyConfirmDetail(in []reclassifiableSource) string {
+	out := "Changes:"
+
+	for _, r := range in {
+		out += " " + r.KeyType + " " + r.OldID + "→" + r.NewID + ";"
+	}
+
+	return out
+}
+
+// reclassifyCanonicalSourceIDs is the QuickFix body for
+// FixIDReclassifyCanonicalSourceIDs. Re-reads Sources.xml / Presets.xml
+// / Recents.xml for the device, builds the old-ID → new-ID mapping for
+// each eligible built-in radio source, rewrites the source IDs in
+// Sources.xml plus any matching <sourceid> references in
+// Presets.xml/Recents.xml, and persists all three. The datastore's
+// SaveX helpers each use atomic-rename internally; a failure mid-way
+// leaves earlier files updated but the operation as a whole is
+// idempotent — re-running it produces the same result.
+func reclassifyCanonicalSourceIDs(ds *datastore.DataStore, target Target) (string, error) {
+	if target.Account == "" || target.Device == "" {
+		return "", fmt.Errorf("account and device are both required")
+	}
+
+	view, err := loadServiceView(ds, target.Account, target.Device)
+	if err != nil {
+		return "", fmt.Errorf("load service state: %w", err)
+	}
+
+	plans := findReclassifiableSources(view)
+	if len(plans) == 0 {
+		return "Nothing to do — all built-in radio sources already on canonical IDs.", nil
+	}
+
+	rename := map[string]string{}
+	canonicalProviderID := map[string]string{}
+
+	for _, p := range plans {
+		rename[p.OldID] = p.NewID
+		canonicalProviderID[p.OldID] = p.ProvID
+	}
+
+	sources, err := ds.GetConfiguredSources(target.Account, target.Device)
+	if err != nil {
+		return "", fmt.Errorf("read Sources.xml: %w", err)
+	}
+
+	for i := range sources {
+		if newID, ok := rename[sources[i].ID]; ok {
+			log.Printf("[Health] Re-classify %s: id %s → %s (account=%s device=%s)",
+				sources[i].SourceKeyType, sources[i].ID, newID, target.Account, target.Device)
+
+			sources[i].ID = newID
+
+			if provID := canonicalProviderID[sources[i].ID]; provID != "" {
+				sources[i].SourceProviderID = provID
+			}
+		}
+	}
+
+	if saveErr := ds.SaveConfiguredSources(target.Account, target.Device, sources); saveErr != nil {
+		return "", fmt.Errorf("save Sources.xml: %w", saveErr)
+	}
+
+	if err := rewritePresetSourceIDs(ds, target, rename); err != nil {
+		return "", err
+	}
+
+	if err := rewriteRecentSourceIDs(ds, target, rename); err != nil {
+		return "", err
+	}
+
+	return fmt.Sprintf("Re-classified %d source ID(s) for device %s (account %s). Presets/Recents references updated accordingly. The speaker will pick up the new IDs on its next /full fetch.",
+		len(plans), target.Device, target.Account), nil
+}
+
+// rewritePresetSourceIDs walks Presets.xml and updates any <sourceid>
+// that appears as a key in rename to the mapped value. Persists only
+// when at least one preset changed. Silent on read errors (missing
+// Presets.xml is a valid state — the operator just doesn't have any
+// presets to update).
+func rewritePresetSourceIDs(ds *datastore.DataStore, target Target, rename map[string]string) error {
+	presets, err := ds.GetPresets(target.Account, target.Device)
+	if err != nil {
+		return fmt.Errorf("read Presets.xml: %w", err)
+	}
+
+	dirty := false
+
+	for i := range presets {
+		if newID, ok := rename[presets[i].SourceID]; ok {
+			presets[i].SourceID = newID
+			dirty = true
+		}
+	}
+
+	if !dirty {
+		return nil
+	}
+
+	if err := ds.SavePresets(target.Account, target.Device, presets); err != nil {
+		return fmt.Errorf("save Presets.xml: %w", err)
+	}
+
+	return nil
+}
+
+// rewriteRecentSourceIDs is the recents-side twin of rewritePresetSourceIDs.
+func rewriteRecentSourceIDs(ds *datastore.DataStore, target Target, rename map[string]string) error {
+	recents, err := ds.GetRecents(target.Account, target.Device)
+	if err != nil {
+		return fmt.Errorf("read Recents.xml: %w", err)
+	}
+
+	dirty := false
+
+	for i := range recents {
+		if newID, ok := rename[recents[i].SourceID]; ok {
+			recents[i].SourceID = newID
+			dirty = true
+		}
+	}
+
+	if !dirty {
+		return nil
+	}
+
+	if err := ds.SaveRecents(target.Account, target.Device, recents); err != nil {
+		return fmt.Errorf("save Recents.xml: %w", err)
+	}
+
+	return nil
+}
+
 func checkOneDeviceConsistency(ds *datastore.DataStore, account, deviceID, ipAddress string) []Finding {
 	target := Target{Account: account, Device: deviceID}

@@ -232,6 +462,25 @@ func checkOneDeviceConsistency(ds *datastore.DataStore, account, deviceID, ipAdd
 	// (which would always trigger spurious "dangling" findings).
 	findings = append(findings, issuesToFindings(target, CheckInternalConsistency(serviceView), SeverityWarning)...)

+	// GH-343-style detection: built-in radio sources sitting on
+	// non-canonical IDs (the 2000001+i fallback that GetConfiguredSources
+	// hands out when on-disk sources don't carry canonical IDs). Surface
+	// as a finding with an offline QuickFix that rewrites both the
+	// source IDs and the preset/recent <sourceid> references atomically.
+	if reclassifiable := findReclassifiableSources(serviceView); len(reclassifiable) > 0 {
+		findings = append(findings, Finding{
+			Severity: SeverityWarning,
+			Target:   target,
+			Message:  "Sources.xml has " + plural(len(reclassifiable), "built-in radio source", "built-in radio sources") + " on non-canonical IDs (GH-343 trigger).",
+			Details:  reclassifyDetailMessage(reclassifiable),
+			QuickFixes: []QuickFix{{
+				ID:      FixIDReclassifyCanonicalSourceIDs,
+				Label:   "Re-assign canonical IDs",
+				Confirm: "Rewrite " + plural(len(reclassifiable), "source ID", "source IDs") + " in Sources.xml and matching <sourceid> references in Presets.xml/Recents.xml? " + reclassifyConfirmDetail(reclassifiable) + " Offline operation — no speaker contact needed; the speaker will re-fetch /full on its own.",
+			}},
+		})
+	}
+
 	if ipAddress == "" {
 		findings = append(findings, Finding{
 			Severity: SeverityInfo,
@@ -0,0 +1,210 @@
+package health
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/gesellix/bose-soundtouch/pkg/service/datastore"
+)
+
+// TestReclassifyCanonicalSourceIDs_GH343 reproduces the situation from
+// issue #343: a device's Sources.xml has built-in radio sources at
+// non-canonical "fallback" IDs (2000003/2000004/2000008), and Presets.xml
+// has presets bound to those IDs by <sourceid>. The QuickFix rewrites
+// the source IDs to canonical values and updates the references in
+// Presets.xml/Recents.xml atomically; running it twice is a no-op.
+func TestReclassifyCanonicalSourceIDs_GH343(t *testing.T) {
+	tempDir, err := os.MkdirTemp("", "health-reclassify-*")
+	if err != nil {
+		t.Fatalf("tempdir: %v", err)
+	}
+	defer func() { _ = os.RemoveAll(tempDir) }()
+
+	account := "1111111"
+	device := "AABBCCDDEEFF"
+
+	deviceDir := filepath.Join(tempDir, "accounts", account, "devices", device)
+	if err := os.MkdirAll(deviceDir, 0755); err != nil {
+		t.Fatalf("mkdir: %v", err)
+	}
+
+	// Mirrors og-gh's Sources.xml from issue #343: built-in radio
+	// sources at fallback IDs.
+	sourcesXML := `<?xml version="1.0" encoding="UTF-8"?>
+<sources>
+    <source id="2000003" secret="" secretType="token" type="Audio" sourceproviderid="2">
+        <credential type=""></credential>
+        <sourceKey type="INTERNET_RADIO" account=""></sourceKey>
+    </source>
+    <source id="2000004" secret="aaa" secretType="token" type="Audio" sourceproviderid="11">
+        <credential type="token">aaa</credential>
+        <sourceKey type="LOCAL_INTERNET_RADIO" account=""></sourceKey>
+    </source>
+    <source id="2000008" secret="bbb" secretType="token" type="Audio" sourceproviderid="25">
+        <credential type="token">bbb</credential>
+        <sourceKey type="TUNEIN" account=""></sourceKey>
+    </source>
+    <source id="10005" secret="" secretType="token" type="Audio" sourceproviderid="39">
+        <credential type=""></credential>
+        <sourceKey type="RADIO_BROWSER" account=""></sourceKey>
+    </source>
+</sources>`
+	if err := os.WriteFile(filepath.Join(deviceDir, "Sources.xml"), []byte(sourcesXML), 0644); err != nil {
+		t.Fatalf("write Sources.xml: %v", err)
+	}
+
+	// Presets bound by ID to the non-canonical TuneIn (2000008) and
+	// LocalInternetRadio (2000004) sources.
+	presetsXML := `<?xml version="1.0" encoding="UTF-8"?>
+<presets>
+    <preset id="1" createdOn="0" updatedOn="0">
+        <contentItem source="TUNEIN" type="stationurl" location="/v1/playback/station/s6634">
+            <itemName>MDR JUMP</itemName>
+        </contentItem>
+        <sourceid>2000008</sourceid>
+    </preset>
+    <preset id="5" createdOn="0" updatedOn="0">
+        <contentItem source="LOCAL_INTERNET_RADIO" type="stationurl" location="http://example/custom/v1/playback/abc">
+            <itemName>laut.fm</itemName>
+        </contentItem>
+        <sourceid>2000004</sourceid>
+    </preset>
+</presets>`
+	if err := os.WriteFile(filepath.Join(deviceDir, "Presets.xml"), []byte(presetsXML), 0644); err != nil {
+		t.Fatalf("write Presets.xml: %v", err)
+	}
+
+	// And a recent bound to TuneIn by ID.
+	recentsXML := `<?xml version="1.0" encoding="UTF-8"?>
+<recents>
+    <recent id="rec-1">
+        <contentItem source="TUNEIN" type="stationurl" location="/v1/playback/station/s6634">
+            <itemName>MDR JUMP</itemName>
+        </contentItem>
+        <sourceid>2000008</sourceid>
+    </recent>
+</recents>`
+	if err := os.WriteFile(filepath.Join(deviceDir, "Recents.xml"), []byte(recentsXML), 0644); err != nil {
+		t.Fatalf("write Recents.xml: %v", err)
+	}
+
+	ds := datastore.NewDataStore(tempDir)
+
+	msg, err := reclassifyCanonicalSourceIDs(ds, Target{Account: account, Device: device})
+	if err != nil {
+		t.Fatalf("reclassifyCanonicalSourceIDs: %v", err)
+	}
+
+	if !strings.Contains(msg, "Re-classified 3") {
+		t.Errorf("success message should report 3 re-classifications, got %q", msg)
+	}
+
+	// Sources.xml: IDs rewritten to canonical.
+	sources, err := ds.GetConfiguredSources(account, device)
+	if err != nil {
+		t.Fatalf("re-read Sources.xml: %v", err)
+	}
+
+	want := map[string]string{
+		"INTERNET_RADIO":       "10002",
+		"LOCAL_INTERNET_RADIO": "10003",
+		"TUNEIN":               "10004",
+		"RADIO_BROWSER":        "10005",
+	}
+
+	for _, s := range sources {
+		if expected, ok := want[s.SourceKeyType]; ok && s.ID != expected {
+			t.Errorf("%s: expected id %s, got %s", s.SourceKeyType, expected, s.ID)
+		}
+	}
+
+	// Presets.xml: <sourceid> references rewritten.
+	presets, err := ds.GetPresets(account, device)
+	if err != nil {
+		t.Fatalf("re-read Presets.xml: %v", err)
+	}
+
+	for _, p := range presets {
+		switch p.Source {
+		case "TUNEIN":
+			if p.SourceID != "10004" {
+				t.Errorf("TUNEIN preset slot %s: expected sourceid=10004, got %q", p.ButtonNumber, p.SourceID)
+			}
+		case "LOCAL_INTERNET_RADIO":
+			if p.SourceID != "10003" {
+				t.Errorf("LIR preset slot %s: expected sourceid=10003, got %q", p.ButtonNumber, p.SourceID)
+			}
+		}
+	}
+
+	// Recents.xml: same.
+	recents, err := ds.GetRecents(account, device)
+	if err != nil {
+		t.Fatalf("re-read Recents.xml: %v", err)
+	}
+
+	for _, r := range recents {
+		if r.Source == "TUNEIN" && r.SourceID != "10004" {
+			t.Errorf("TUNEIN recent: expected sourceid=10004, got %q", r.SourceID)
+		}
+	}
+
+	// Idempotency: a second run should be a no-op.
+	msg2, err := reclassifyCanonicalSourceIDs(ds, Target{Account: account, Device: device})
+	if err != nil {
+		t.Fatalf("second reclassify: %v", err)
+	}
+
+	if !strings.Contains(msg2, "Nothing to do") {
+		t.Errorf("second run should be a no-op, got %q", msg2)
+	}
+}
+
+// TestReclassifyCanonicalSourceIDs_SkipsCollision verifies the
+// collision-avoidance guard: if the canonical ID is already in use by
+// another source (e.g. operator hand-edited and now has two TUNEIN
+// entries, one at 10004 and one at 2000008), we leave both alone
+// rather than create an ID conflict.
+func TestReclassifyCanonicalSourceIDs_SkipsCollision(t *testing.T) {
+	tempDir, err := os.MkdirTemp("", "health-reclassify-collision-*")
+	if err != nil {
+		t.Fatalf("tempdir: %v", err)
+	}
+	defer func() { _ = os.RemoveAll(tempDir) }()
+
+	account := "1111111"
+	device := "AABBCCDDEEFF"
+
+	deviceDir := filepath.Join(tempDir, "accounts", account, "devices", device)
+	if err := os.MkdirAll(deviceDir, 0755); err != nil {
+		t.Fatalf("mkdir: %v", err)
+	}
+
+	sourcesXML := `<?xml version="1.0" encoding="UTF-8"?>
+<sources>
+    <source id="10004" secret="aaa" secretType="token" type="Audio" sourceproviderid="25">
+        <credential type="token">aaa</credential>
+        <sourceKey type="TUNEIN" account="acct-A"></sourceKey>
+    </source>
+    <source id="2000008" secret="bbb" secretType="token" type="Audio" sourceproviderid="25">
+        <credential type="token">bbb</credential>
+        <sourceKey type="TUNEIN" account="acct-B"></sourceKey>
+    </source>
+</sources>`
+	if err := os.WriteFile(filepath.Join(deviceDir, "Sources.xml"), []byte(sourcesXML), 0644); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+
+	ds := datastore.NewDataStore(tempDir)
+
+	msg, err := reclassifyCanonicalSourceIDs(ds, Target{Account: account, Device: device})
+	if err != nil {
+		t.Fatalf("reclassifyCanonicalSourceIDs: %v", err)
+	}
+
+	if !strings.Contains(msg, "Nothing to do") {
+		t.Errorf("expected no-op when canonical collides; got %q", msg)
+	}
+}