From d5f9d16e424eddd5b5c1ea41e0c020fa2d90ae64 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 10 May 2026 18:51:31 +0200 Subject: [PATCH] feat(setup): per-field telnet URLs with envswitch derivation rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactors telnetURLConfigCommands into a telnetURLs value type with explicit per-field URLs (Marge, Stats, SwUpdate, BmxRegistry) and adds telnetURLsFromOptions to resolve those four URLs from a base targetURL plus optional per-field overrides via the migration options map (marge_url, stats_url, sw_update_url, bmx_url). Envswitch derivation rule: arg1 = u.Marge verbatim, arg2 = u.SwUpdate verbatim. The soundcork case (Marge has /marge appended) is handled without any branching — envswitch arg1 carries the same suffix and the parallel persistence layer stays consistent with the runtime layer on the next reboot. The default path is unchanged for users who only enter a base URL: all four fields share targetURL with the canonical /updates/soundtouch and /bmx/registry/v1/services suffixes. MigrateSpeaker plumbs the options map through so the existing handler's option dictionary works for telnet without UI changes; the UI can layer per-field input on top later. Existing telnet migration tests updated to call the new signature. TestMigrateViaTelnet_SoundcorkMargeSuffixPropagatesToEnvswitch is the load-bearing regression test for the derivation rule. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/service/setup/setup.go | 3 +- pkg/service/setup/telnet_migration.go | 90 +++++++++++-- pkg/service/setup/telnet_migration_test.go | 12 +- pkg/service/setup/telnet_urls_test.go | 147 +++++++++++++++++++++ 4 files changed, 231 insertions(+), 21 deletions(-) create mode 100644 pkg/service/setup/telnet_urls_test.go diff --git a/pkg/service/setup/setup.go b/pkg/service/setup/setup.go index 80ffbda..0afa4aa 100644 --- a/pkg/service/setup/setup.go +++ b/pkg/service/setup/setup.go @@ -751,7 +751,8 @@ func (m *Manager) MigrateSpeaker(deviceIP, targetURL, proxyURL string, options m // rw pre-flight, both of which would fail on devices that haven't been // rooted via remote_services. if method == MigrationMethodTelnet { - return m.migrateViaTelnet(deviceIP, targetURL) + urls := telnetURLsFromOptions(targetURL, options) + return m.migrateViaTelnet(deviceIP, targetURL, urls) } var logs string diff --git a/pkg/service/setup/telnet_migration.go b/pkg/service/setup/telnet_migration.go index 8a0fa0e..9ef4c4d 100644 --- a/pkg/service/setup/telnet_migration.go +++ b/pkg/service/setup/telnet_migration.go @@ -6,20 +6,77 @@ import ( "strings" ) -// telnetURLConfigCommands returns the canonical sequence of telnet commands -// that point a SoundTouch device at the given local-service base URL. +// telnetURLs holds the four URLs the migration writes via telnet. Most +// users keep all four pointing at the same service base; per-field +// overrides exist mainly so soundcork users can append /marge to the +// marge URL. +type telnetURLs struct { + Marge string + Stats string + SwUpdate string + BmxRegistry string +} + +// defaultTelnetURLs returns the canonical URL set derived from the +// soundtouch-service base targetURL. +func defaultTelnetURLs(targetURL string) telnetURLs { + return telnetURLs{ + Marge: targetURL, + Stats: targetURL, + SwUpdate: targetURL + "/updates/soundtouch", + BmxRegistry: targetURL + "/bmx/registry/v1/services", + } +} + +// telnetURLsFromOptions resolves the four URLs from targetURL plus +// per-field overrides supplied via the migration options map. Recognised +// keys are marge_url, stats_url, sw_update_url, bmx_url; missing or empty +// entries fall back to the canonical default. // -// Order matters: `sys configuration …` writes the runtime URL, while -// `envswitch boseurls set …` writes a parallel persistence layer that -// otherwise wins on the next reboot. See docs/analysis/TELNET-MIGRATION-METHOD.md -// §2.1 for the discussion this is derived from. -func telnetURLConfigCommands(targetURL string) []string { +// We deliberately do not expose a "proxied"/"original" semantic here +// (unlike the XML method's applyProxyOptions): per the discussion that +// motivated this iteration, the goal is to keep the user model simple — +// one base URL plus optional path suffixes — and let the service layer +// hold any non-trivial logic. +func telnetURLsFromOptions(targetURL string, options map[string]string) telnetURLs { + u := defaultTelnetURLs(targetURL) + + if v := options["marge_url"]; v != "" { + u.Marge = v + } + + if v := options["stats_url"]; v != "" { + u.Stats = v + } + + if v := options["sw_update_url"]; v != "" { + u.SwUpdate = v + } + + if v := options["bmx_url"]; v != "" { + u.BmxRegistry = v + } + + return u +} + +// Commands returns the canonical sequence of telnet commands. Order +// matters: `sys configuration …` writes the runtime layer; the closing +// `envswitch boseurls set …` writes the parallel persistence layer that +// otherwise wins on the next reboot. +// +// Envswitch derivation rule: arg1 mirrors u.Marge verbatim, arg2 mirrors +// u.SwUpdate verbatim. Soundcork users who set Marge to "/marge" +// therefore get "envswitch boseurls set /marge /updates/soundtouch" +// without any extra plumbing — the parallel layer stays consistent with +// the runtime layer. +func (u telnetURLs) Commands() []string { return []string{ - "sys configuration bmxRegistryUrl " + targetURL + "/bmx/registry/v1/services", - "sys configuration statsServerUrl " + targetURL, - "sys configuration margeServerUrl " + targetURL, - "sys configuration swUpdateUrl " + targetURL + "/updates/soundtouch", - "envswitch boseurls set " + targetURL + " " + targetURL + "/updates/soundtouch", + "sys configuration bmxRegistryUrl " + u.BmxRegistry, + "sys configuration statsServerUrl " + u.Stats, + "sys configuration margeServerUrl " + u.Marge, + "sys configuration swUpdateUrl " + u.SwUpdate, + "envswitch boseurls set " + u.Marge + " " + u.SwUpdate, } } @@ -31,7 +88,12 @@ func telnetURLConfigCommands(targetURL string) []string { // The sequence aborts on the first non-OK response so we never half-write the // configuration; the caller can retry safely after fixing the underlying // issue (closed port, hardened firmware, etc.). -func (m *Manager) migrateViaTelnet(deviceIP, targetURL string) (string, error) { +// +// targetURL is kept as a separate verification anchor: most users have +// every URL share that base, so substring-matching it against the +// device's `getpdo` reply is the simplest "did the writes stick?" check +// that still works for the soundcork "/marge on one field" case. +func (m *Manager) migrateViaTelnet(deviceIP, targetURL string, urls telnetURLs) (string, error) { if m.NewTelnet == nil { return "", errors.New("telnet migration not configured: Manager.NewTelnet is nil") } @@ -50,7 +112,7 @@ func (m *Manager) migrateViaTelnet(deviceIP, targetURL string) (string, error) { fmt.Fprintf(&logs, "Telnet banner: %q\n", strings.TrimSpace(banner)) } - for _, cmd := range telnetURLConfigCommands(targetURL) { + for _, cmd := range urls.Commands() { resp, err := t.SendCommand(cmd) if err != nil { return logs.String(), fmt.Errorf("telnet command %q failed: %w", cmd, err) diff --git a/pkg/service/setup/telnet_migration_test.go b/pkg/service/setup/telnet_migration_test.go index e854482..12920d4 100644 --- a/pkg/service/setup/telnet_migration_test.go +++ b/pkg/service/setup/telnet_migration_test.go @@ -66,7 +66,7 @@ func TestMigrateViaTelnet_HappyPath(t *testing.T) { } m := newFakeTelnetManager(f) - logs, err := m.migrateViaTelnet("192.0.2.1", target) + logs, err := m.migrateViaTelnet("192.0.2.1", target, defaultTelnetURLs(target)) if err != nil { t.Fatalf("migrateViaTelnet: %v", err) } @@ -103,7 +103,7 @@ func TestMigrateViaTelnet_DialFailureReturnsError(t *testing.T) { f := &fakeTelnet{dialErr: errors.New("connection refused")} m := newFakeTelnetManager(f) - _, err := m.migrateViaTelnet("192.0.2.1", "http://example:8000") + _, err := m.migrateViaTelnet("192.0.2.1", "http://example:8000", defaultTelnetURLs("http://example:8000")) if err == nil { t.Fatal("expected dial error, got nil") } @@ -126,7 +126,7 @@ func TestMigrateViaTelnet_CommandNotFoundAborts(t *testing.T) { f := &fakeTelnet{responses: resp} m := newFakeTelnetManager(f) - _, err := m.migrateViaTelnet("192.0.2.1", target) + _, err := m.migrateViaTelnet("192.0.2.1", target, defaultTelnetURLs(target)) if err == nil { t.Fatal("expected error when envswitch is rejected, got nil") } @@ -153,7 +153,7 @@ func TestMigrateViaTelnet_VerifyMismatchFails(t *testing.T) { f := &fakeTelnet{responses: resp} m := newFakeTelnetManager(f) - _, err := m.migrateViaTelnet("192.0.2.1", target) + _, err := m.migrateViaTelnet("192.0.2.1", target, defaultTelnetURLs(target)) if err == nil { t.Fatal("expected verification mismatch error, got nil") } @@ -173,7 +173,7 @@ func TestMigrateViaTelnet_TransportErrorAborts(t *testing.T) { } m := newFakeTelnetManager(f) - _, err := m.migrateViaTelnet("192.0.2.1", target) + _, err := m.migrateViaTelnet("192.0.2.1", target, defaultTelnetURLs(target)) if err == nil { t.Fatal("expected transport error, got nil") } @@ -186,7 +186,7 @@ func TestMigrateViaTelnet_TransportErrorAborts(t *testing.T) { func TestMigrateViaTelnet_MissingNewTelnetIsClearError(t *testing.T) { m := &Manager{ServerURL: "http://example:8000"} // NewTelnet deliberately nil - _, err := m.migrateViaTelnet("192.0.2.1", "http://example:8000") + _, err := m.migrateViaTelnet("192.0.2.1", "http://example:8000", defaultTelnetURLs("http://example:8000")) if err == nil { t.Fatal("expected error when NewTelnet is nil") } diff --git a/pkg/service/setup/telnet_urls_test.go b/pkg/service/setup/telnet_urls_test.go new file mode 100644 index 0000000..ac4227c --- /dev/null +++ b/pkg/service/setup/telnet_urls_test.go @@ -0,0 +1,147 @@ +package setup + +import ( + "reflect" + "strings" + "testing" +) + +func TestDefaultTelnetURLs_DerivesAllFourFromBase(t *testing.T) { + got := defaultTelnetURLs("http://example:8000") + + want := telnetURLs{ + Marge: "http://example:8000", + Stats: "http://example:8000", + SwUpdate: "http://example:8000/updates/soundtouch", + BmxRegistry: "http://example:8000/bmx/registry/v1/services", + } + + if !reflect.DeepEqual(got, want) { + t.Errorf("defaultTelnetURLs = %+v, want %+v", got, want) + } +} + +func TestTelnetURLsFromOptions_NilOptionsReturnsDefaults(t *testing.T) { + got := telnetURLsFromOptions("http://example:8000", nil) + want := defaultTelnetURLs("http://example:8000") + + if !reflect.DeepEqual(got, want) { + t.Errorf("telnetURLsFromOptions(nil) = %+v, want defaults %+v", got, want) + } +} + +func TestTelnetURLsFromOptions_EmptyValueFallsBackToDefault(t *testing.T) { + options := map[string]string{ + "marge_url": "", // empty override should be ignored + } + + got := telnetURLsFromOptions("http://example:8000", options) + + if got.Marge != "http://example:8000" { + t.Errorf("Marge with empty override = %q, want default", got.Marge) + } +} + +func TestTelnetURLsFromOptions_PerFieldOverrides(t *testing.T) { + options := map[string]string{ + "marge_url": "http://example:8000/marge", // soundcork-style + "stats_url": "", // ignored + "sw_update_url": "http://example:8000/custom/updates", + "bmx_url": "http://example:8000/custom/bmx", + } + + got := telnetURLsFromOptions("http://example:8000", options) + + if got.Marge != "http://example:8000/marge" { + t.Errorf("Marge = %q, want override", got.Marge) + } + + if got.Stats != "http://example:8000" { + t.Errorf("Stats = %q, want default (empty override)", got.Stats) + } + + if got.SwUpdate != "http://example:8000/custom/updates" { + t.Errorf("SwUpdate = %q, want override", got.SwUpdate) + } + + if got.BmxRegistry != "http://example:8000/custom/bmx" { + t.Errorf("BmxRegistry = %q, want override", got.BmxRegistry) + } +} + +// TestTelnetURLs_Commands_EnvswitchTracksMargeAndSwUpdate is the load-bearing +// test for the soundcork case: if the user added /marge to Marge, the +// envswitch arg1 must follow the same suffix verbatim, otherwise the +// parallel persistence layer will revert margeServerUrl on next reboot +// (the very failure mode the user described as "envswitch silently +// restores my typo"). +func TestTelnetURLs_Commands_EnvswitchTracksMargeAndSwUpdate(t *testing.T) { + urls := telnetURLs{ + Marge: "http://example:8000/marge", + Stats: "http://example:8000", + SwUpdate: "http://example:8000/updates/soundtouch", + BmxRegistry: "http://example:8000/bmx/registry/v1/services", + } + + cmds := urls.Commands() + + var envswitch string + + for _, c := range cmds { + if strings.HasPrefix(c, "envswitch boseurls set ") { + envswitch = c + break + } + } + + if envswitch == "" { + t.Fatalf("Commands missing envswitch boseurls set:\n%v", cmds) + } + + wantEnv := "envswitch boseurls set http://example:8000/marge http://example:8000/updates/soundtouch" + if envswitch != wantEnv { + t.Errorf("envswitch =\n %q\nwant\n %q", envswitch, wantEnv) + } +} + +func TestMigrateViaTelnet_SoundcorkMargeSuffixPropagatesToEnvswitch(t *testing.T) { + target := "http://example:8000" + urls := telnetURLs{ + Marge: "http://example:8000/marge", + Stats: "http://example:8000", + SwUpdate: "http://example:8000/updates/soundtouch", + BmxRegistry: "http://example:8000/bmx/registry/v1/services", + } + + // Build a happy-path responder that matches the *new* command set. + resp := map[string]string{ + "sys configuration bmxRegistryUrl " + urls.BmxRegistry: "OK\n", + "sys configuration statsServerUrl " + urls.Stats: "OK\n", + "sys configuration margeServerUrl " + urls.Marge: "OK\n", + "sys configuration swUpdateUrl " + urls.SwUpdate: "OK\n", + "envswitch boseurls set " + urls.Marge + " " + urls.SwUpdate: "OK\n", + "getpdo CurrentSystemConfiguration": "margeServerUrl=" + urls.Marge + "\n", + } + + f := &fakeTelnet{responses: resp} + m := newFakeTelnetManager(f) + + if _, err := m.migrateViaTelnet("192.0.2.1", target, urls); err != nil { + t.Fatalf("migrateViaTelnet: %v", err) + } + + wantEnvCmd := "envswitch boseurls set http://example:8000/marge http://example:8000/updates/soundtouch" + + var saw bool + + for _, c := range f.commands { + if c == wantEnvCmd { + saw = true + break + } + } + + if !saw { + t.Errorf("never sent expected envswitch command %q\nactual commands:\n%v", wantEnvCmd, f.commands) + } +}