diff --git a/cmd/harnesscli/config/config.go b/cmd/harnesscli/config/config.go index 8a89703b..ef5eda1c 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 00000000..d9f7553a --- /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 777290cf..b4882f39 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 00000000..3712955a --- /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 65165ff6..c4554efe 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 00000000..b8b6b917 --- /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 006ee76e..e7342a13 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