From 369887f642a53e5e289a76c1c560cf98c1172c68 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sat, 5 Sep 2026 12:22:16 +0200 Subject: [PATCH] fix(cli): stop pushing to Marge, make its preflight read advisory Same fix as -service and -player, for consistency: newGroupCoordinator proactively pushed group cleanup/rename to an external Marge backend the CLI doesn't own. A speaker's own firmware already self-reports that create/rename/teardown to whatever Marge backend it's configured with -- that's the entire reason HandleMargeAddGroup/HandleMargeModifyGroup/ HandleMargeDeleteGroup exist, they're only ever called by speakers. Extracted the wiring into cliStereoPairGenerationPersistence (mirrors the -service/-player equivalents): cleanup and rename are now no-ops, and preflight's read-only dangling-generation check stays but its failure no longer blocks Create -- it's printed as a warning instead. Co-Authored-By: Claude Sonnet 5 --- cmd/soundtouch-cli/cmd_group.go | 48 ++++++++++++++++++++++------ cmd/soundtouch-cli/cmd_group_test.go | 44 +++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 9 deletions(-) diff --git a/cmd/soundtouch-cli/cmd_group.go b/cmd/soundtouch-cli/cmd_group.go index 51c1973a..c7c17bdb 100644 --- a/cmd/soundtouch-cli/cmd_group.go +++ b/cmd/soundtouch-cli/cmd_group.go @@ -182,21 +182,51 @@ func newGroupCoordinator(config *ClientConfig) *stereopair.Coordinator { } cleanupClient := &http.Client{Timeout: lifecycleConfig.Timeout} + cleanup, preflight, rename := cliStereoPairGenerationPersistence(cleanupClient) return stereopair.NewWithGenerationLifecyclePersistence( groupClientFactory(&lifecycleConfig), - func(ref stereopair.GenerationRef) error { - return stereopair.DeleteMargeGroupGeneration(cleanupClient, ref) - }, - func(refs []stereopair.GenerationRef) error { - return stereopair.EnsureMargeNoGroupGenerations(cleanupClient, refs) - }, - func(ref stereopair.GenerationRef, name string) error { - return stereopair.RenameMargeGroupGeneration(cleanupClient, ref, name) - }, + cleanup, preflight, rename, ) } +// cliStereoPairGenerationPersistence wires generation-lifecycle hooks for +// the CLI: cleanup and rename are no-ops, and preflight's read-only +// dangling-generation check is advisory (mirrors -service's and -player's +// own equivalents). +// +// A speaker self-reports its own group create/rename/teardown to whatever +// Marge backend it's configured with -- that's the entire reason +// HandleMargeAddGroup/HandleMargeModifyGroup/HandleMargeDeleteGroup exist, +// they're only ever called by speakers, never by us. Proactively pushing the +// same update ourselves would duplicate that against a backend we generally +// can't authenticate to anyway (real Bose cloud, another AfterTouch/SoundCork +// instance, ...). The one part with a distinct purpose -- checking for a +// dangling stale generation before a new Create -- is still attempted, but +// its failure must not block Create: it's a best-effort safety net on top of +// the coordinator's own physical preflight, not the primary guard. +func cliStereoPairGenerationPersistence( + cleanupClient *http.Client, +) (stereopair.GenerationCleanup, stereopair.GenerationPreflight, stereopair.GenerationRename) { + cleanup := func(stereopair.GenerationRef) error { + return nil + } + + preflight := func(refs []stereopair.GenerationRef) error { + if err := stereopair.EnsureMargeNoGroupGenerations(cleanupClient, refs); err != nil { + PrintWarning(fmt.Sprintf("stereo-pair external generation preflight inconclusive, proceeding: %v", err)) + } + + return nil + } + + rename := func(stereopair.GenerationRef, string) error { + return nil + } + + return cleanup, preflight, rename +} + // groupClientFactory addresses every member directly while retaining the // effective CLI port and timeout. func groupClientFactory(config *ClientConfig) stereopair.ClientFactory { diff --git a/cmd/soundtouch-cli/cmd_group_test.go b/cmd/soundtouch-cli/cmd_group_test.go index 70ef8699..807c67c9 100644 --- a/cmd/soundtouch-cli/cmd_group_test.go +++ b/cmd/soundtouch-cli/cmd_group_test.go @@ -209,3 +209,47 @@ func testServerHostPort(t *testing.T, serverURL string) (string, int) { return host, port } + +// TestCLIStereoPairGenerationPersistenceSkipsWritesButAttemptsRead covers +// the CLI's generation-lifecycle wiring: cleanup and rename must never push +// to a Marge backend -- the speaker itself self-reports its own group +// teardown/rename to whatever backend it's configured with (see +// HandleMargeDeleteGroup/HandleMargeModifyGroup, only ever called by +// speakers). Preflight still attempts its read-only dangling-generation +// check, but a failure there (network error, wrong credentials, real Bose +// cloud rejecting us, ...) must not block Create. +func TestCLIStereoPairGenerationPersistenceSkipsWritesButAttemptsRead(t *testing.T) { + getCalls := 0 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/device/LEFT-ID/group") { + getCalls++ + http.Error(w, "unauthorized", http.StatusUnauthorized) + + return + } + + t.Fatalf("unexpected external %s %s: cleanup/rename must not write to a backend the CLI doesn't own", r.Method, r.URL.Path) + })) + defer server.Close() + + cleanup, preflight, rename := cliStereoPairGenerationPersistence(server.Client()) + + ref := stereopair.GenerationRef{ + DeviceID: "LEFT-ID", AccountID: "ACCOUNT1", MargeURL: server.URL + "/marge", + GroupID: "7654321", + } + + if err := rename(ref, "Renamed living room"); err != nil { + t.Fatalf("rename = %v, want nil (speaker self-reports its own rename)", err) + } + if err := cleanup(ref); err != nil { + t.Fatalf("cleanup = %v, want nil (speaker self-reports its own teardown)", err) + } + if err := preflight([]stereopair.GenerationRef{ref}); err != nil { + t.Fatalf("preflight = %v, want nil: an unauthenticated external check must not block Create", err) + } + if getCalls != 1 { + t.Fatalf("external GET calls = %d, want exactly 1 (preflight must still attempt the read)", getCalls) + } +}