Skip to content

Commit 60d701d

Browse files
PierrunoYTampagent
andcommitted
fix(tui): persist provider model selection atomically
Amp-Thread-ID: https://ampcode.com/threads/T-01a04c92-2d1d-7508-91bc-416341b7e8b0 Co-authored-by: Amp <amp@ampcode.com>
1 parent 6eec32e commit 60d701d

8 files changed

Lines changed: 246 additions & 39 deletions

File tree

‎internal/config/writer.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,51 @@ func SetActiveProvider(path string, name string) (result FileConfig, err error)
225225
return FileConfig{}, fmt.Errorf("provider %q not found", name)
226226
}
227227

228+
// SetActiveProviderModel persists a provider selection as one lock-held
229+
// read-modify-write. The active provider and that provider's model are one UI
230+
// choice; publishing them separately can leave a partially applied selection
231+
// if another writer wins the lock between calls.
232+
func SetActiveProviderModel(path string, name string, model string) (result FileConfig, err error) {
233+
path = strings.TrimSpace(path)
234+
if path == "" {
235+
return FileConfig{}, fmt.Errorf("config path is required")
236+
}
237+
name = strings.TrimSpace(name)
238+
if name == "" {
239+
return FileConfig{}, fmt.Errorf("provider name is required")
240+
}
241+
model = strings.TrimSpace(model)
242+
if model == "" {
243+
return FileConfig{}, fmt.Errorf("model is required")
244+
}
245+
246+
unlock, err := lockConfigFileFn(path)
247+
if err != nil {
248+
return FileConfig{}, err
249+
}
250+
defer func() { err = errors.Join(err, unlock()) }()
251+
252+
data, err := os.ReadFile(path)
253+
if err != nil {
254+
return FileConfig{}, fmt.Errorf("read config %s: %w", path, err)
255+
}
256+
cfg := FileConfig{}
257+
if err := json.Unmarshal(data, &cfg); err != nil {
258+
return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err)
259+
}
260+
for index := range cfg.Providers {
261+
if strings.EqualFold(strings.TrimSpace(cfg.Providers[index].Name), name) {
262+
cfg.ActiveProvider = cfg.Providers[index].Name
263+
cfg.Providers[index].Model = model
264+
if err := writeConfigFile(path, cfg); err != nil {
265+
return FileConfig{}, err
266+
}
267+
return cfg, nil
268+
}
269+
}
270+
return FileConfig{}, fmt.Errorf("provider %q not found", name)
271+
}
272+
228273
// ProviderPersisted reports whether a provider profile named name actually has
229274
// a row in the config file at path. A provider can appear in the resolved/
230275
// in-memory provider list without ever being written to config.json — e.g.

‎internal/config/writer_test.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,32 @@ func TestSetActiveProviderTightensExistingConfigFilePermissions(t *testing.T) {
151151
}
152152
}
153153

154+
func TestSetActiveProviderModelPersistsSelectionTogether(t *testing.T) {
155+
path := filepath.Join(t.TempDir(), "zero.json")
156+
writeConfigFixture(t, path, FileConfig{
157+
ActiveProvider: "openai",
158+
Providers: []ProviderProfile{
159+
{Name: "openai", ProviderKind: ProviderKindOpenAI, Model: "gpt-4.1"},
160+
{Name: "Anthropic", ProviderKind: ProviderKindAnthropic, Model: "old-model"},
161+
},
162+
}, 0o600)
163+
164+
cfg, err := SetActiveProviderModel(path, " anthropic ", " claude-sonnet-4.5 ")
165+
if err != nil {
166+
t.Fatalf("SetActiveProviderModel() error = %v", err)
167+
}
168+
if cfg.ActiveProvider != "Anthropic" || cfg.Providers[1].Model != "claude-sonnet-4.5" {
169+
t.Fatalf("returned selection = active %q model %q", cfg.ActiveProvider, cfg.Providers[1].Model)
170+
}
171+
persisted := readConfigFixture(t, path)
172+
if persisted.ActiveProvider != "Anthropic" || persisted.Providers[1].Model != "claude-sonnet-4.5" {
173+
t.Fatalf("persisted selection = active %q model %q", persisted.ActiveProvider, persisted.Providers[1].Model)
174+
}
175+
if persisted.Providers[0].Model != "gpt-4.1" {
176+
t.Fatalf("unrelated provider changed: %#v", persisted.Providers[0])
177+
}
178+
}
179+
154180
func TestSetProviderModelUpdatesConfiguredProvider(t *testing.T) {
155181
path := filepath.Join(t.TempDir(), "zero.json")
156182
writeConfigFixture(t, path, FileConfig{

‎internal/tui/command_center.go‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -523,21 +523,22 @@ func (m model) handleModelCommand(args string) (model, string) {
523523
// picker calls this when a model from a non-active provider is chosen, so the
524524
// picker can list every saved provider and switch across them (like a unified
525525
// provider+model selector). The key is loaded from the encrypted store / env.
526-
// The returned bool reports whether the switch actually committed — callers
527-
// that branch on the outcome (the provider manager) must use it, never the
526+
// The returned bool reports whether the in-session switch committed — callers
527+
// that branch on that outcome (the provider manager) must use it, never the
528528
// display text: UI copy is not a control-flow contract (a refusal quoting a
529529
// provider name could contain any substring, and rewording the success line
530-
// must not change behavior).
531-
func (m model) switchProviderModel(providerName, modelID string) (model, string, bool, tea.Cmd) {
530+
// must not change behavior). The returned error reports that the in-session
531+
// switch succeeded but its provider/model transaction was not durably saved.
532+
func (m model) switchProviderModel(providerName, modelID string) (model, string, bool, tea.Cmd, error) {
532533
if m.pending {
533-
return m, "Model\nCannot switch providers while a run is active.", false, nil
534+
return m, "Model\nCannot switch providers while a run is active.", false, nil, nil
534535
}
535536
if m.newProvider == nil {
536-
return m, "Model\nProvider rebuild is not available for this TUI session.", false, nil
537+
return m, "Model\nProvider rebuild is not available for this TUI session.", false, nil, nil
537538
}
538539
target, ok := m.savedProviderByName(providerName)
539540
if !ok {
540-
return m, "Model\nunknown provider " + strconv.Quote(providerName), false, nil
541+
return m, "Model\nunknown provider " + strconv.Quote(providerName), false, nil, nil
541542
}
542543
previousProviderName := m.providerName
543544
previousModel := m.modelName
@@ -551,11 +552,11 @@ func (m model) switchProviderModel(providerName, modelID string) (model, string,
551552
// keyless on purpose so newProvider attaches the bearer resolver + login key.
552553
if strings.TrimSpace(target.APIKey) == "" && strings.TrimSpace(target.AuthHeaderValue) == "" &&
553554
(!hasDescriptor || !descriptor.Local) && !oauthLoginAvailable(target) {
554-
return m, "Model\nprovider " + strconv.Quote(providerName) + " has no usable credential — run setup or `zero auth login " + providerName + "`.", false, nil
555+
return m, "Model\nprovider " + strconv.Quote(providerName) + " has no usable credential — run setup or `zero auth login " + providerName + "`.", false, nil, nil
555556
}
556557
next, err := m.newProvider(target)
557558
if err != nil {
558-
return m, "Model\n" + redaction.RedactString(err.Error(), redaction.Options{ExtraSecretValues: []string{target.APIKey}}), false, nil
559+
return m, "Model\n" + redaction.RedactString(err.Error(), redaction.Options{ExtraSecretValues: []string{target.APIKey}}), false, nil, nil
559560
}
560561
m.provider = next
561562
m.providerProfile = target
@@ -569,20 +570,24 @@ func (m model) switchProviderModel(providerName, modelID string) (model, string,
569570
// carried (pre-existing behavior) while the profile's own fill stays
570571
// conservative — it only ever applies where support is known.
571572
m = m.reconcileProfileAfterModelSwitch(m.availableReasoningEfforts())
572-
// Record the outgoing pair too — see the matching comment in
573-
// handleModelCommand for why (keeps the session's starting model from
574-
// silently dropping out of "Recent" on the first switch away from it).
575-
// recordRecentModels batches both into a single normalize+persist instead
576-
// of two separate disk writes for this one switch.
577-
m = m.recordRecentModels(
578-
config.RecentModelEntry{Provider: previousProviderName, Model: previousModel},
579-
config.RecentModelEntry{Provider: target.Name, Model: target.Model},
580-
)
581573
// Keep sub-agent child processes on the same provider we just switched to.
582574
config.SetActiveProviderEnv(target.Name)
583-
if strings.TrimSpace(m.userConfigPath) != "" {
584-
_, _ = config.SetActiveProvider(m.userConfigPath, target.Name)
585-
_, _ = config.SetProviderModel(m.userConfigPath, target.Name, target.Model)
575+
path := strings.TrimSpace(m.userConfigPath)
576+
var persistErr error
577+
if path != "" {
578+
_, persistErr = config.SetActiveProviderModel(path, target.Name, target.Model)
579+
}
580+
recent := []config.RecentModelEntry{
581+
{Provider: previousProviderName, Model: previousModel},
582+
{Provider: target.Name, Model: target.Model},
583+
}
584+
if persistErr == nil {
585+
// Record the outgoing pair too — see the matching comment in
586+
// handleModelCommand for why. Persist history only after the selection
587+
// transaction succeeds, so one unavailable lock causes one wait.
588+
m = m.recordRecentModels(recent...)
589+
} else {
590+
m, _ = m.updateRecentModels(recent...)
586591
}
587592
// Warm discovery for the provider we just switched to, same as Init() does
588593
// for the provider active at launch — otherwise the context-usage gauge has
@@ -597,10 +602,15 @@ func (m model) switchProviderModel(providerName, modelID string) (model, string,
597602
}
598603
}
599604
status := fmt.Sprintf("Model\nSwitched to %s · %s", target.Name, target.Model)
605+
if path != "" && persistErr == nil {
606+
status += " · saved"
607+
} else if persistErr != nil {
608+
status += " · not saved (" + persistErr.Error() + ")"
609+
}
600610
if warn := m.visionDropWarning(); warn != "" {
601611
status += "\n" + warn
602612
}
603-
return m, status, true, tea.Batch(cmds...)
613+
return m, status, true, tea.Batch(cmds...), persistErr
604614
}
605615

606616
// profileWithCredential fills a profile's APIKey for provider construction the same

‎internal/tui/model.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4464,18 +4464,23 @@ func (m model) choosePicker() (tea.Model, tea.Cmd) {
44644464
case pickerModel:
44654465
previousProvider, previousModel := m.providerName, m.modelName
44664466
text := ""
4467+
var switchPersistErr error
44674468
owner := strings.TrimSpace(item.OwnerProvider)
44684469
_, ownerIsSavedProvider := m.savedProviderByName(owner)
44694470
if owner != "" && !strings.EqualFold(owner, strings.TrimSpace(m.providerName)) && ownerIsSavedProvider {
44704471
// A model from another saved provider: switch provider + model together.
4471-
m, text, _, cmd = m.switchProviderModel(owner, item.Value)
4472+
m, text, _, cmd, switchPersistErr = m.switchProviderModel(owner, item.Value)
44724473
} else {
44734474
// OwnerProvider is blank, matches the active provider, or (registry-fallback
44744475
// / stale-history rows) doesn't resolve to any saved provider: apply against
44754476
// the active provider instead of attempting an unresolvable provider switch.
44764477
m, text = m.handleModelCommand(item.Value)
44774478
}
44784479
if m.providerName != previousProvider || m.modelName != previousModel {
4480+
if switchPersistErr != nil {
4481+
notice := m.modelAppliedNotice() + " · not saved (" + switchPersistErr.Error() + ")"
4482+
return m.showTransientNoticeInline(notice, transientNoticeWarning), cmd
4483+
}
44794484
return m.showTransientNoticeInline(m.modelAppliedNotice(), transientNoticeSuccess), cmd
44804485
}
44814486
m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text})

‎internal/tui/picker.go‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -945,6 +945,23 @@ func (m model) persistFavoriteModels() error {
945945
// of the session. Returns the receiver unchanged (no re-normalization, no
946946
// write) when every pair has a blank model id.
947947
func (m model) recordRecentModels(pairs ...config.RecentModelEntry) model {
948+
m, changed := m.updateRecentModels(pairs...)
949+
if !changed {
950+
return m
951+
}
952+
if path := strings.TrimSpace(m.userConfigPath); path != "" {
953+
if _, err := config.SetRecentModels(path, m.recentModels); err != nil {
954+
m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "recent model save error: " + err.Error()})
955+
}
956+
}
957+
return m
958+
}
959+
960+
// updateRecentModels performs recordRecentModels' in-memory half. A model
961+
// switch whose selection transaction failed still belongs in this session's
962+
// picker history, but must not make another blocking persistence attempt while
963+
// the same config lock is unavailable.
964+
func (m model) updateRecentModels(pairs ...config.RecentModelEntry) (model, bool) {
948965
entries := append([]config.RecentModelEntry{}, m.recentModels...)
949966
changed := false
950967
for _, pair := range pairs {
@@ -956,15 +973,10 @@ func (m model) recordRecentModels(pairs ...config.RecentModelEntry) model {
956973
entries = append([]config.RecentModelEntry{{Provider: strings.TrimSpace(pair.Provider), Model: modelID}}, entries...)
957974
}
958975
if !changed {
959-
return m
976+
return m, false
960977
}
961978
m.recentModels = normalizeRecentModelEntries(entries)
962-
if path := strings.TrimSpace(m.userConfigPath); path != "" {
963-
if _, err := config.SetRecentModels(path, m.recentModels); err != nil {
964-
m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendError, text: "recent model save error: " + err.Error()})
965-
}
966-
}
967-
return m
979+
return m, true
968980
}
969981

970982
// normalizeRecentModelEntries trims, drops entries with no model id,

0 commit comments

Comments
 (0)