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) + } +}