Skip to content

Commit 83fa9eb

Browse files
ampagentPierrunoYT
andcommitted
fix: preserve live provider identity across session resume
Address PR #892 review feedback: resume retains the live client identity and model, stored-key case collisions explain manual recovery, and persistence errors appear only once. Regression tests fail on the reviewed implementation and pass with these changes. Full tests, focused config/CLI/TUI race tests, vet, formatting, release build/smoke, and vulnerability checks pass. Static lint reports four existing suggestions in unchanged files. Amp-Thread-ID: https://ampcode.com/threads/T-01a0d88d-fcb4-774c-adf5-d34813060e5a Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
1 parent dff3fbf commit 83fa9eb

6 files changed

Lines changed: 49 additions & 12 deletions

File tree

‎internal/config/writer.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,11 @@ func RepairUnnamedProvider(path string, replacement string) (result FileConfig,
194194
name = "openai"
195195
}
196196
}
197+
if cfg.Providers[unnamed].APIKeyStored {
198+
if sibling, collides := conflictingProviderRowName(cfg, unnamed, legacyName); collides && sibling != legacyName {
199+
return FileConfig{}, "", fmt.Errorf("the unnamed provider's stored credential uses identity %q, which conflicts with persisted provider %q; manually rename the sibling %q in config.json and reconcile any of its credential references before retrying `zero providers repair-config`; --name cannot safely move the stored credential", legacyName, sibling, sibling)
200+
}
201+
}
197202
if conflict, collides := conflictingProviderRowName(cfg, unnamed, name); collides {
198203
if explicit {
199204
return FileConfig{}, "", fmt.Errorf(

‎internal/config/writer_test.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1499,6 +1499,27 @@ func TestRepairUnnamedProviderAllowsExplicitUniqueName(t *testing.T) {
14991499
}
15001500
}
15011501

1502+
func TestRepairUnnamedStoredProviderCaseCollisionExplainsManualRecovery(t *testing.T) {
1503+
path := filepath.Join(t.TempDir(), "config.json")
1504+
before := writeConfigFixture(t, path, FileConfig{
1505+
ActiveProvider: "legacy",
1506+
Providers: []ProviderProfile{
1507+
{APIKeyStored: true},
1508+
{Name: "LEGACY"},
1509+
},
1510+
}, 0o600)
1511+
for _, name := range []string{"", "work", "Legacy"} {
1512+
_, _, err := RepairUnnamedProvider(path, name)
1513+
if err == nil || !strings.Contains(err.Error(), `manually rename the sibling "LEGACY" in config.json`) {
1514+
t.Fatalf("repair with name %q must explain the manual recovery: %v", name, err)
1515+
}
1516+
after, readErr := os.ReadFile(path)
1517+
if readErr != nil || !bytes.Equal(before, after) {
1518+
t.Fatal("refused repair changed config")
1519+
}
1520+
}
1521+
}
1522+
15021523
func TestEnsureCatalogProviderValidatesBeforeExistingProfileShortcut(t *testing.T) {
15031524
path := filepath.Join(t.TempDir(), "config.json")
15041525
before := writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "xai"}, {Name: "XAI"}}}, 0o600)

‎internal/tui/command_center.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -633,11 +633,11 @@ func (m model) switchProviderModel(providerName, modelID string) (model, string,
633633
switch {
634634
case err != nil:
635635
persistErr = err
636-
persistNote = "\nNote: the switch applies to this session, but config.json could not be read: " + redaction.RedactString(err.Error(), redaction.Options{})
636+
persistNote = "\nNote: the switch applies to this session, but config.json could not be read."
637637
case owner.UserBacked:
638638
if cfg, err := config.SetActiveProviderModel(m.userConfigPath, owner.PersistedName, target.Model); err != nil {
639639
persistErr = err
640-
persistNote = "\nNote: the switch applies to this session, but config.json was not updated: " + redaction.RedactString(err.Error(), redaction.Options{})
640+
persistNote = "\nNote: the switch applies to this session, but config.json was not updated."
641641
} else {
642642
persisted = true
643643
// Reconcile the in-memory list the manager and picker read from,

‎internal/tui/command_center_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,9 @@ func TestSwitchProviderModelReportsPersistenceFailures(t *testing.T) {
188188
if !strings.Contains(status, "config.json could not be read") {
189189
t.Fatalf("status = %q, want a persistence note", status)
190190
}
191+
if strings.Count(status, err.Error()) != 1 {
192+
t.Fatal("persistence error must appear exactly once in the switch status")
193+
}
191194
// The session did switch, which is exactly why the note has to be there.
192195
if next.providerName != "OpenAI" {
193196
t.Fatalf("providerName = %q, want OpenAI", next.providerName)
@@ -208,6 +211,9 @@ func TestSwitchProviderModelReportsPersistenceFailures(t *testing.T) {
208211
if !strings.Contains(status, "config.json was not updated") {
209212
t.Fatalf("status = %q, want the active-provider persistence note", status)
210213
}
214+
if strings.Count(status, err.Error()) != 1 {
215+
t.Fatal("persistence error must appear exactly once in the switch status")
216+
}
211217
})
212218

213219
t.Run("env-derived provider stays silent", func(t *testing.T) {

‎internal/tui/model_test.go‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1243,7 +1243,7 @@ func TestResumeAfterDeletingLiveProjectRow(t *testing.T) {
12431243
deleteLiveRow bool
12441244
sameSession bool
12451245
}{
1246-
{name: "different session clears retained identity", deleteLiveRow: true},
1246+
{name: "different session retains deleted identity", deleteLiveRow: true},
12471247
{name: "same session retains deleted identity", deleteLiveRow: true, sameSession: true},
12481248
{name: "no deletion preserves nonempty live fields"},
12491249
} {
@@ -1277,18 +1277,28 @@ func TestResumeAfterDeletingLiveProjectRow(t *testing.T) {
12771277
m.providerWizard = nil
12781278
}
12791279
wantProvider, wantModel, wantRemoved := m.providerName, m.modelName, m.removedLiveRow
1280+
liveClient, liveProfile := m.provider, m.providerProfile
12801281
if tc.sameSession {
12811282
target = previous
1282-
} else if tc.deleteLiveRow {
1283-
wantProvider, wantModel, wantRemoved = "work", "resumed-model", ""
12841283
}
12851284
next, message := m.handleResumeCommand(target.SessionID)
12861285
if message != "" || next.activeSession.SessionID != target.SessionID {
12871286
t.Fatalf("resume failed: %s", message)
12881287
}
1288+
// Resume loads history, not a new provider. Labels and retained row
1289+
// ownership must still describe the client that answers the next turn.
1290+
if next.provider != liveClient || len(built) != 0 || !reflect.DeepEqual(next.providerProfile, liveProfile) {
1291+
t.Fatal("resume changed the live client or its endpoint/credential profile")
1292+
}
12891293
if next.removedLiveRow != wantRemoved || next.providerName != wantProvider || next.modelName != wantModel || next.activeProviderRowName() != wantProvider {
12901294
t.Fatalf("resume identity = (%q, %q, %q, %q), want (%q, %q, %q, %q)", next.removedLiveRow, next.providerName, next.modelName, next.activeProviderRowName(), wantRemoved, wantProvider, wantModel, wantProvider)
12911295
}
1296+
if !transcriptContains(next.transcript, "model: project-model") || !transcriptContains(next.transcript, "provider: WORK") {
1297+
t.Fatal("resume summary does not describe the live project client")
1298+
}
1299+
if !tc.sameSession && !transcriptContains(next.transcript, "(recorded: resumed-model)") {
1300+
t.Fatal("resume summary does not distinguish the recorded model from the live model")
1301+
}
12921302
assertUserRowUntouched(t, next, before)
12931303
})
12941304
}

‎internal/tui/session.go‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -236,13 +236,8 @@ func (m model) handleResumeCommand(args string) (model, string) {
236236
m.activeSession = *session
237237
m.pendingSessionTitle = ""
238238
m.sessionEvents = append([]sessions.Event{}, events...)
239-
if session.SessionID != previousID && m.removedLiveRow != "" {
240-
// A deleted row belongs to the previous session. Adopt the resumed
241-
// metadata instead of carrying that retained identity into this one.
242-
m.removedLiveRow = ""
243-
m.providerName = session.Provider
244-
m.modelName = session.ModelID
245-
}
239+
// Resume replaces conversation history, not the live client. Keep its
240+
// identity (including a removed row) and model until an actual provider switch.
246241
if m.providerName == "" {
247242
m.removedLiveRow = ""
248243
m.providerName = session.Provider

0 commit comments

Comments
 (0)