feat(tui): remember the last used model across restarts - #1425
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Live restart verifiedDriven through a pty against a real daemon on this branch's binaries, with Before any selection — status bar shows the placeholder, no model: After selecting a model via Scratch config after quitting: {
"model": "gpt-4.1",
"provider": "openai"
}After relaunching, without touching anything: Step 8 matches step 4. The model survives the restart. Two details worth recording rather than glossing:
The switcher in this build is a flat fuzzy filter across providers, not the two-level provider drill-down I described in the issue. My description was wrong; the behavior under test is unaffected. Hygiene: the real |
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5
4613abb to
5fac571
Compare
Closes #1424
The problem
Pick a model with
/model, quit, restart — back on the daemon default.newTUIConfig(cmd/harnesscli/main.go:562) never setModel, soselectedModelstarted empty every session.Not a missing store:
~/.config/harnesscli/config.jsonalready persists starred models, gateway, API keys, command history and theme. The model was the conspicuous omission, and it is the one users change most often.What changed
harnessconfig.ConfiggainsModel,ProviderandReasoningEffort, allomitemptyso older files load unchanged and older binaries ignore them.ModelSelectedMsgpersists all three through the existingpersistConfigFieldidiom; the constructor applies them right where starred models, gateway, keys and history are already applied.The three travel together because they are chosen together — a provider or reasoning effort without its model describes nothing, and restoring only the model would leave the status bar agreeing with itself while runs went elsewhere.
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 an unreadable config both behave exactly as today.The bigger thing this exposed
Making the model persistent turned the TUI test suite into something that mutates the developer's machine. A test run wrote into my real
~/.config/harnesscli/config.json:A model no human had chosen. Three tests then failed against it — including one asserting "no model chosen" — and they failed for a reason that had nothing to do with what they test.
Fixed with a package
TestMainthat redirectsHOMEonce before any test runs, plus a per-test reset of the scratch config ininitModel.Both parts are needed, and the obvious approach does not work:
t.Setenvin the sharedinitModelhelper panics for any test callingt.Parallel, and several in this package do. Redirecting once inTestMain, before any test starts, is both parallel-safe and race-free.I also tried
t.Setenvconditionally in the helper; it broke four feedback tests that set their ownHOMEand assert on files written under it.Verification
Four tests, red first:
TestNoRememberedModelLeavesDaemonDefaultandTestExplicitModelBeatsRememberedModelare the controls — they passed before the change and must keep passing, so "remembering" cannot be faked by defaulting to something nobody picked, and a preference cannot outrank an instruction.go test ./cmd/harnesscli/... -racegreen. My real config after a full race run — nomodelkey, which is the check that the isolation actually holds:Live restart is being verified separately and I will post the result here. The unit tests prove persist and restore; they do not prove a real TUI session survives a quit. I will not claim it until I have watched it.
Out of scope
runTUInever receives the-modelflag, soharnesscli --tui -model Xignores it today. That is a real defect, recorded in the engineering log, and deserves its own issue. The precedence guard here is written so wiring it needs no further change.🤖 Generated with Claude Code
https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5