From ddee8b34d76fb131b904155b8f5de3980717238c Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sat, 5 Sep 2026 22:22:54 +0200 Subject: [PATCH] fix(setup): read the URLs back when a telnet command is not confirmed A rejected or unrecognised command response aborted the sequence with "read back and reconcile all four URL fields before rebooting", leaving the user to do by hand what the service can do in one read-only command. That advice also assumes the write failed, which the reply shape does not prove. A telnet console is a shared stream and firmware echoes vary: an interleaved log line, a normalised URL, or a banner arriving late all produce a response the parser does not recognise, for a write that landed. This is the same class as the earlier parser reporting HTTP 500 when all four writes had succeeded, just narrower. The abort itself is kept, so no further write is sent and a rejected sequence cannot spread. Before returning, `getpdo CurrentSystemConfiguration` now runs and the reported error carries what the device actually holds. Nothing is claimed to have succeeded on ambiguous evidence; the user simply gets the evidence. The two tests asserting that nothing at all follows a rejection now assert the property that matters, that no further command CHANGES the device, since a read-only read-back does not. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/service/setup/telnet_migration.go | 50 ++++++++++++++++- pkg/service/setup/telnet_migration_test.go | 64 ++++++++++++++++++---- 2 files changed, 102 insertions(+), 12 deletions(-) diff --git a/pkg/service/setup/telnet_migration.go b/pkg/service/setup/telnet_migration.go index 049af4cc..c1561284 100644 --- a/pkg/service/setup/telnet_migration.go +++ b/pkg/service/setup/telnet_migration.go @@ -312,8 +312,20 @@ func (m *Manager) migrateViaTelnet(deviceIP string, urls telnetURLs) (string, er fmt.Fprintf(&logs, "→ %s\n%s\n", cmd, strings.TrimRight(resp, "\r\n")) if err := validateTelnetMutationResponse(cmd, resp, persistenceCommand, urls); err != nil { - return logs.String(), fmt.Errorf("%w; %s", err, - telnetWriteFailureContext(runtimeWrites, persistenceCommand)) + // Aborting here is deliberate: no further writes are sent, so a + // rejected sequence cannot spread. But the advice this produces is + // "read back and reconcile all four URL fields", which the service + // can simply do, and must, because an unrecognised reply is not + // proof the write failed. A telnet console is a shared stream and + // firmware echoes vary, so the readback is better evidence than + // the reply shape. It is read-only and changes nothing. + // Read back before snapshotting logs: the return values are + // evaluated left to right, so logs.String() inline would capture + // the builder before the read-back appends to it. + readBack := telnetReadBackSummary(t, &logs) + + return logs.String(), fmt.Errorf("%w; %s%s", err, + telnetWriteFailureContext(runtimeWrites, persistenceCommand), readBack) } if !persistenceCommand { @@ -401,6 +413,40 @@ func hasTelnetPersistenceConfirmation(response, expected string) bool { len(meaningful) == 2 && meaningful[0] == expected && meaningful[1] == "OK" } +// telnetReadBackSummary reads the live URL configuration so a failure report +// says what the device actually holds now, instead of asking the user to go +// and find out. Returns an empty string when the readback itself fails, since +// there is then nothing trustworthy to add. +func telnetReadBackSummary(t TelnetClient, logs *strings.Builder) string { + verify, err := t.SendCommand("getpdo CurrentSystemConfiguration") + if err != nil { + fmt.Fprintf(logs, "Read-back after the failure also failed: %v\n", err) + + return "" + } + + fmt.Fprintf(logs, "→ getpdo CurrentSystemConfiguration (read-back after the failure)\n%s\n", + strings.TrimRight(verify, "\r\n")) + + current := parseGetpdoConfig(verify) + if len(current) == 0 { + return "" + } + + fields := make([]string, 0, len(current)) + for _, field := range canonicalBoseTelnetURLs().fields() { + if value, ok := current[field.configName]; ok && value != "" { + fields = append(fields, field.configName+"="+value) + } + } + + if len(fields) == 0 { + return "" + } + + return "; the device currently reports " + strings.Join(fields, ", ") +} + func telnetWriteFailureContext(runtimeWrites int, persistenceAttempted bool) string { if persistenceAttempted { return "all four runtime URL writes were confirmed, but the persistence outcome is uncertain; " + diff --git a/pkg/service/setup/telnet_migration_test.go b/pkg/service/setup/telnet_migration_test.go index e0a7b627..9dafe7e1 100644 --- a/pkg/service/setup/telnet_migration_test.go +++ b/pkg/service/setup/telnet_migration_test.go @@ -270,13 +270,7 @@ func TestMigrateViaTelnet_CommandNotFoundAborts(t *testing.T) { t.Errorf("err = %v, want to mention the rejected command", err) } - // The verification command must NOT have been sent — the run aborts on - // the first rejection. - for _, c := range f.commands { - if c == "getpdo CurrentSystemConfiguration" { - t.Errorf("verification was sent after a rejected command: %v", f.commands) - } - } + assertNoTelnetWritesAfterRejection(t, f.commands, "envswitch") } func TestMigrateViaTelnet_GenericRuntimeRejectionReportsPartialState(t *testing.T) { @@ -322,9 +316,30 @@ func TestMigrateViaTelnet_EnvswitchRejectionReportsUncertainPersistence(t *testi t.Errorf("err = %v, want uncertain-persistence classification", err) } - for _, command := range f.commands { - if command == "getpdo CurrentSystemConfiguration" { - t.Errorf("verification was sent after unconfirmed envswitch response: %v", f.commands) + assertNoTelnetWritesAfterRejection(t, f.commands, "envswitch") +} + +// assertNoTelnetWritesAfterRejection checks the property that matters once a +// command is rejected: no further command may CHANGE the device. The +// read-only getpdo read-back is expected, since the failure report says what +// the device actually holds rather than telling the user to go and find out. +func assertNoTelnetWritesAfterRejection(t *testing.T, commands []string, rejected string) { + t.Helper() + + seenRejected := false + for _, command := range commands { + if strings.Contains(command, rejected) { + seenRejected = true + + continue + } + + if !seenRejected { + continue + } + + if strings.HasPrefix(command, "sys configuration") || strings.HasPrefix(command, "envswitch") { + t.Errorf("a write was sent after the rejected command: %v", commands) } } } @@ -499,3 +514,32 @@ func TestTelnetRevertNotOfferedOnUnmigratedSpeakers(t *testing.T) { }) } } + +// TestMigrateViaTelnet_FailureReportsWhatTheDeviceHolds: the failure advice is +// "read back and reconcile all four URL fields", which the service can do +// itself. An unrecognised reply is not proof the write failed, so the readback +// is better evidence than the reply shape. +func TestMigrateViaTelnet_FailureReportsWhatTheDeviceHolds(t *testing.T) { + target := "http://example:8000" + urls := defaultTelnetURLs(target) + resp := happyResponses(target) + resp["envswitch boseurls set "+urls.Marge+" "+urls.SwUpdate] = "something unfamiliar\n" + + f := &fakeTelnet{responses: resp} + m := newFakeTelnetManager(f) + + logs, err := m.migrateViaTelnet("192.0.2.1", urls) + if err == nil { + t.Fatal("expected an error for an unrecognised envswitch response") + } + + if !strings.Contains(err.Error(), "the device currently reports") { + t.Errorf("err = %v, want it to carry the live URL state", err) + } + if !strings.Contains(err.Error(), "margeServerUrl="+target) { + t.Errorf("err = %v, want the actual margeServerUrl value", err) + } + if !strings.Contains(logs, "read-back after the failure") { + t.Errorf("logs did not record the read-back:\n%s", logs) + } +}