From 5fac5712e269b114c88f83475c0b0c6f425cdd5a Mon Sep 17 00:00:00 2001 From: Dennison Date: Tue, 8 Sep 2026 08:56:39 -0400 Subject: [PATCH] feat(tui): remember the last used model across restarts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pick a model with /model, quit, restart, and you were back on the daemon default: newTUIConfig never set Model, so selectedModel started empty every session. harnessconfig already persisted starred models, gateway, API keys, history and theme, so this adds fields to that store rather than inventing one. Model, provider and reasoning effort are stored together because they are chosen together — a provider without its model describes nothing. Applying is guarded on an empty requested model, so an explicit choice always wins: remembering a preference must never override an instruction. Empty still means "let the daemon choose", so a first run and an unreadable config both behave exactly as before. This exposed a worse problem than it fixed. Persisting the model meant the TUI test suite began writing to the developer's real ~/.config/harnesscli/config.json — a run left a model there that no human had chosen, and three tests then failed against it. A package TestMain now redirects HOME once before any test runs, and initModel resets the scratch config per test. t.Setenv in a shared helper cannot do this: it panics for tests calling t.Parallel, and several here do. Closes #1424 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5 --- cmd/harnesscli/config/config.go | 9 ++ cmd/harnesscli/config/model_memory_test.go | 50 +++++++++++ cmd/harnesscli/tui/escape_test.go | 14 +++ cmd/harnesscli/tui/main_test.go | 35 ++++++++ cmd/harnesscli/tui/model.go | 20 +++++ cmd/harnesscli/tui/model_memory_test.go | 100 +++++++++++++++++++++ docs/logs/engineering-log.md | 40 +++++++++ 7 files changed, 268 insertions(+) create mode 100644 cmd/harnesscli/config/model_memory_test.go create mode 100644 cmd/harnesscli/tui/main_test.go create mode 100644 cmd/harnesscli/tui/model_memory_test.go diff --git a/cmd/harnesscli/config/config.go b/cmd/harnesscli/config/config.go index 8a89703be..ef5eda1ca 100644 --- a/cmd/harnesscli/config/config.go +++ b/cmd/harnesscli/config/config.go @@ -13,6 +13,15 @@ type Config struct { APIKeys map[string]string `json:"api_keys,omitempty"` HistoryEntries []string `json:"history_entries,omitempty"` // newest-first command history Theme string `json:"theme,omitempty"` // selected color theme name (epic #810) + + // Model, Provider and ReasoningEffort remember the last model chosen in the + // TUI so a restart resumes on it (issue #1424). They are stored together + // because they are chosen together: a provider or reasoning effort without + // its model describes nothing. All are omitempty, so an older config file + // loads unchanged and an older binary ignores them. + Model string `json:"model,omitempty"` + Provider string `json:"provider,omitempty"` + ReasoningEffort string `json:"reasoning_effort,omitempty"` } func configPath() (string, error) { diff --git a/cmd/harnesscli/config/model_memory_test.go b/cmd/harnesscli/config/model_memory_test.go new file mode 100644 index 000000000..d9f7553af --- /dev/null +++ b/cmd/harnesscli/config/model_memory_test.go @@ -0,0 +1,50 @@ +package config + +import ( + "testing" +) + +// TestConfigRoundTripsModelSelection pins issue #1424: the model a user picks +// must survive a restart, together with the provider and reasoning effort that +// were chosen in the same moment and mean nothing apart from it. +func TestConfigRoundTripsModelSelection(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + if err := Save(&Config{ + Model: "gpt-4.1-mini", + Provider: "openai", + ReasoningEffort: "medium", + }); err != nil { + t.Fatalf("Save: %v", err) + } + + got, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if got.Model != "gpt-4.1-mini" { + t.Errorf("Model = %q, want %q", got.Model, "gpt-4.1-mini") + } + if got.Provider != "openai" { + t.Errorf("Provider = %q, want %q", got.Provider, "openai") + } + if got.ReasoningEffort != "medium" { + t.Errorf("ReasoningEffort = %q, want %q", got.ReasoningEffort, "medium") + } +} + +// TestConfigWithNoModelRemembersNothing is the false-positive control: an empty +// store must stay empty, so "remembering" cannot be faked by defaulting to some +// model nobody chose. +func TestConfigWithNoModelRemembersNothing(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + got, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if got.Model != "" || got.Provider != "" || got.ReasoningEffort != "" { + t.Errorf("empty store should remember nothing, got model=%q provider=%q effort=%q", + got.Model, got.Provider, got.ReasoningEffort) + } +} diff --git a/cmd/harnesscli/tui/escape_test.go b/cmd/harnesscli/tui/escape_test.go index 777290cfd..b4882f390 100644 --- a/cmd/harnesscli/tui/escape_test.go +++ b/cmd/harnesscli/tui/escape_test.go @@ -2,6 +2,7 @@ package tui_test import ( "os" + "path/filepath" "strings" "sync" "testing" @@ -11,9 +12,22 @@ import ( tui "go-agent-harness/cmd/harnesscli/tui" ) +// resetPersistedConfig clears the scratch config between tests. +// +// TestMain gives the package one scratch HOME, so persisted state written by +// one test is visible to the next. Since issue #1424 that includes the selected +// model, which made tests asserting "no model chosen" fail against a model a +// previous test had picked. Starting each test from an empty store restores the +// old property that construction sees nothing persisted unless the test says so. +func resetPersistedConfig(t *testing.T) { + t.Helper() + _ = os.Remove(filepath.Join(os.Getenv("HOME"), ".config", "harnesscli", "config.json")) +} + // initModel creates a Model with a given terminal size. func initModel(t *testing.T, w, h int) tui.Model { t.Helper() + resetPersistedConfig(t) m := tui.New(tui.DefaultTUIConfig()) m2, _ := m.Update(tea.WindowSizeMsg{Width: w, Height: h}) return m2.(tui.Model) diff --git a/cmd/harnesscli/tui/main_test.go b/cmd/harnesscli/tui/main_test.go new file mode 100644 index 000000000..3712955a4 --- /dev/null +++ b/cmd/harnesscli/tui/main_test.go @@ -0,0 +1,35 @@ +package tui_test + +import ( + "os" + "testing" +) + +// TestMain redirects HOME to a scratch directory for the whole package before +// any test runs. +// +// The TUI reads and writes ~/.config/harnesscli/config.json — starred models, +// theme, gateway, and since issue #1424 the last used model. Without this, a +// test that opens the model switcher writes into the developer's real config, +// and later tests inherit whatever it selected. That is not hypothetical: it +// was caught here by three tests failing against a model no human had chosen. +// +// It has to happen once, here, rather than via t.Setenv in a shared helper: +// t.Setenv cannot be used by tests that call t.Parallel, and several in this +// package do. Setting it before any test starts is also race-free, where +// mutating the environment from running tests would not be. Tests that need +// their own HOME still override it locally for their own duration. +func TestMain(m *testing.M) { + home, err := os.MkdirTemp("", "harnesscli-tui-test-home") + if err != nil { + panic("create scratch HOME: " + err.Error()) + } + if err := os.Setenv("HOME", home); err != nil { + panic("redirect HOME: " + err.Error()) + } + + code := m.Run() + + _ = os.RemoveAll(home) + os.Exit(code) +} diff --git a/cmd/harnesscli/tui/model.go b/cmd/harnesscli/tui/model.go index 65165ff6a..c4554efea 100644 --- a/cmd/harnesscli/tui/model.go +++ b/cmd/harnesscli/tui/model.go @@ -512,6 +512,18 @@ func New(cfg TUIConfig) Model { if persistCfg, err := harnessconfig.Load(); err == nil { m.modelSwitcher = m.modelSwitcher.WithStarred(persistCfg.StarredModels) m.selectedGateway = persistCfg.Gateway + // Resume on the model chosen last session (issue #1424). Guarded on an + // empty cfg.Model so an explicitly requested model always wins: + // remembering a preference must never override an instruction. Provider + // and reasoning effort come along because they were chosen in the same + // moment and describe nothing on their own. + if cfg.Model == "" && persistCfg.Model != "" { + m.selectedModel = persistCfg.Model + m.selectedProvider = persistCfg.Provider + m.selectedReasoningEffort = persistCfg.ReasoningEffort + m.modelSwitcher = modelswitcher.New(persistCfg.Model) + m.modelSwitcher = m.modelSwitcher.WithCurrentReasoning(persistCfg.ReasoningEffort) + } m.pendingAPIKeys = persistCfg.APIKeys if len(persistCfg.HistoryEntries) > 0 { m.historyStore = inputarea.NewHistoryWithEntries(100, persistCfg.HistoryEntries) @@ -5189,6 +5201,14 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.selectedModel = msg.ModelID m.selectedProvider = msg.Provider m.selectedReasoningEffort = msg.ReasoningEffort + // Remember it for the next session (issue #1424). Best-effort, like the + // other persisted preferences: losing this is never worth interrupting + // a run the user is in the middle of. + _ = persistConfigField(func(c *harnessconfig.Config) { + c.Model = msg.ModelID + c.Provider = msg.Provider + c.ReasoningEffort = msg.ReasoningEffort + }) m.modelSwitcher = modelswitcher.New(msg.ModelID) m.modelSwitcher = m.modelSwitcher.WithCurrentReasoning(msg.ReasoningEffort) m.modelSwitcher = m.modelSwitcher.WithStarred(currentStarred) diff --git a/cmd/harnesscli/tui/model_memory_test.go b/cmd/harnesscli/tui/model_memory_test.go new file mode 100644 index 000000000..b8b6b917e --- /dev/null +++ b/cmd/harnesscli/tui/model_memory_test.go @@ -0,0 +1,100 @@ +package tui + +import ( + "testing" + + harnessconfig "go-agent-harness/cmd/harnesscli/config" +) + +// TestRememberedModelAppliedAtStartup pins issue #1424: a model chosen in a +// previous session is active on the next start, with no user action. +// +// It asserts the same three fields a live selection sets, because the status +// bar, the switcher's highlight, the context window and the submitted run all +// read from them — restoring only selectedModel would leave the UI agreeing +// with itself while runs went somewhere else. +func TestRememberedModelAppliedAtStartup(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + if err := harnessconfig.Save(&harnessconfig.Config{ + Model: "gpt-4.1-mini", + Provider: "openai", + ReasoningEffort: "medium", + }); err != nil { + t.Fatalf("seed config: %v", err) + } + + m := New(TUIConfig{}) // no explicit model requested + + if m.selectedModel != "gpt-4.1-mini" { + t.Errorf("selectedModel = %q, want the remembered %q", m.selectedModel, "gpt-4.1-mini") + } + if m.selectedProvider != "openai" { + t.Errorf("selectedProvider = %q, want %q", m.selectedProvider, "openai") + } + if m.selectedReasoningEffort != "medium" { + t.Errorf("selectedReasoningEffort = %q, want %q", m.selectedReasoningEffort, "medium") + } +} + +// TestExplicitModelBeatsRememberedModel guards the precedence rule: remembering +// a preference must never override an instruction. Today the TUI has no wired +// -model flag, so this is the guard that keeps precedence correct when one is +// added rather than a live path. +func TestExplicitModelBeatsRememberedModel(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + if err := harnessconfig.Save(&harnessconfig.Config{ + Model: "remembered-model", + Provider: "openai", + }); err != nil { + t.Fatalf("seed config: %v", err) + } + + m := New(TUIConfig{Model: "explicitly-requested"}) + + if m.selectedModel != "explicitly-requested" { + t.Errorf("selectedModel = %q, want the explicitly requested %q; a remembered "+ + "preference must not override an instruction", m.selectedModel, "explicitly-requested") + } +} + +// TestNoRememberedModelLeavesDaemonDefault is the false-positive control. An +// empty selectedModel means "let the daemon choose"; if this test could pass +// with some value invented here, the feature would be indistinguishable from +// hardcoding a default nobody asked for. +func TestNoRememberedModelLeavesDaemonDefault(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + m := New(TUIConfig{}) + + if m.selectedModel != "" { + t.Errorf("selectedModel = %q, want empty so the daemon default applies", m.selectedModel) + } +} + +// TestModelSelectionIsPersisted pins the write half: choosing a model in the +// TUI stores it, or there is nothing to remember next time. +func TestModelSelectionIsPersisted(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + m := New(TUIConfig{}) + updated, _ := m.Update(ModelSelectedMsg{ + ModelID: "claude-sonnet-5", + Provider: "anthropic", + ReasoningEffort: "high", + }) + _ = updated + + stored, err := harnessconfig.Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if stored.Model != "claude-sonnet-5" { + t.Errorf("stored Model = %q, want %q", stored.Model, "claude-sonnet-5") + } + if stored.Provider != "anthropic" { + t.Errorf("stored Provider = %q, want %q", stored.Provider, "anthropic") + } + if stored.ReasoningEffort != "high" { + t.Errorf("stored ReasoningEffort = %q, want %q", stored.ReasoningEffort, "high") + } +} diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index 006ee76e2..e7342a134 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -1,5 +1,45 @@ # Engineering Log +## 2026-09-08 — Issue #1424 remember the last used model + +- Symptom: the TUI forgot the model you picked. `/model`, quit, restart, and you + were back on the daemon default. `newTUIConfig` (`cmd/harnesscli/main.go:562`) + never set `Model`, so `selectedModel` started empty every session. +- Fix: `harnessconfig.Config` gains `Model`, `Provider` and `ReasoningEffort` + (all `omitempty`, so old files load unchanged and older binaries ignore them). + `ModelSelectedMsg` persists all three through the existing + `persistConfigField` idiom; the constructor applies them where starred models, + gateway, keys and history are already applied. The three are stored together + because they are chosen together — a provider or reasoning effort without its + model describes nothing. +- Precedence: applying is guarded on `cfg.Model == ""`, so an explicitly + requested model always wins. Remembering a preference must never override an + instruction. Empty still means "let the daemon choose", so a first run and a + corrupt config both degrade to today's behavior. +- Reused rather than invented: `~/.config/harnesscli/config.json` already + persisted starred models, gateway, API keys, history and theme. The model was + the conspicuous omission, and it is the one users change most. +- **Test-isolation defect this exposed, worth more than the feature.** Making + model selection persistent meant the TUI test suite began writing to the + developer's real `~/.config/harnesscli/config.json` — a run left + `"model": "gpt-4.1-mini"` there that no human had chosen, and three tests then + failed against it. Fixed with a package `TestMain` that redirects HOME once + before any test runs, plus a per-test reset of the scratch config in + `initModel`. + - `t.Setenv` in a shared helper does not work here: it panics for any test + that calls `t.Parallel`, and several in this package do. Redirecting once in + `TestMain`, before any test starts, is both parallel-safe and race-free. + - The reset is needed as well as the redirect: one scratch HOME is shared by + the whole package, so without it a test that selects a model leaks into + every test after it. +- Durable lesson: adding persistence to a UI component silently converts its + test suite into something that mutates the developer's machine. When making + anything persistent, check what the tests write before checking what the + feature reads. +- Noted, not fixed: `runTUI` never receives the `-model` flag, so + `harnesscli --tui -model X` ignores it today. Separate defect; the precedence + guard above is written so wiring it needs no further change here. + ## 2026-09-08 — Issue #1422 interrupt test flake, and a diagnosis that was wrong twice - Symptom: `TestGoCodeScriptStopsHarnessdOnInterrupt` failed on Linux CI with