From 9bfa92b1b127e6dfe466da5e6fa9f96e201d2561 Mon Sep 17 00:00:00 2001 From: Dennison Date: Tue, 8 Sep 2026 09:55:47 -0400 Subject: [PATCH] fix(cli): --tui -model reaches the TUI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit harnesscli --tui -model X parsed the flag and discarded it. The TUI started on the daemon default or, since #1424, on the remembered model — exactly what a user passing -model is trying to override. Silently: no warning, no error. main.go called runTUI without *model, and newTUIConfig never set TUIConfig.Model. The non-TUI path passed the flag correctly all along, so the defect was confined to the TUI branch. The model is now threaded through the same route planMode already takes. Nothing in the TUI changed: selectedModel: cfg.Model and #1424's precedence guard were already right and simply had no producer. Precedence is now real — flag, then remembered model, then daemon default. The flag does not write to the persisted config: a flag is a one-off instruction, not a preference, and silently rewriting the saved model would be a worse bug than this one. Closes #1426 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5 --- cmd/harnesscli/main.go | 15 +++++++++---- cmd/harnesscli/main_test.go | 39 +++++++++++++++++++++++++++++---- cmd/harnesscli/main_tui_test.go | 2 +- docs/logs/engineering-log.md | 34 +++++++++++++++++++++++++--- 4 files changed, 78 insertions(+), 12 deletions(-) diff --git a/cmd/harnesscli/main.go b/cmd/harnesscli/main.go index 5b9c3dcc..6518f280 100644 --- a/cmd/harnesscli/main.go +++ b/cmd/harnesscli/main.go @@ -176,7 +176,7 @@ func run(args []string) int { workspacePath := resolveWorkspacePath(*workspace) if *enableTUI { - if err := runTUI(*baseURL, workspacePath, *resume, *planMode); err != nil { + if err := runTUI(*baseURL, workspacePath, *resume, *model, *planMode); err != nil { fmt.Fprintf(stderr, "harnesscli: tui: %v\n", err) return exitClientError } @@ -535,7 +535,13 @@ func resolveWorkspacePath(workspace string) string { return workspacePath } -func newTUIConfig(baseURL, workspace, resumeConversationID string) tui.TUIConfig { +// newTUIConfig assembles the TUI's configuration. +// +// model is the explicitly requested model (the -model flag) and may be empty. +// Empty is meaningful: it lets the model remembered from the last session +// (issue #1424) apply, and failing that the daemon default. A non-empty value +// outranks both — a flag is an instruction, not a preference. Issue #1426. +func newTUIConfig(baseURL, workspace, resumeConversationID, model string) tui.TUIConfig { // Default to auto-detection; HARNESS_COLOR_PROFILE overrides (truecolor, 256, // ansi, none). Resolved and applied in runTUI before the program starts. colorProfile := strings.TrimSpace(os.Getenv("HARNESS_COLOR_PROFILE")) @@ -568,14 +574,15 @@ func newTUIConfig(baseURL, workspace, resumeConversationID string) tui.TUIConfig ResumeConversationID: resumeConversationID, APIKey: apiKey, Theme: themeName, + Model: model, } } -func runTUI(baseURL, workspace, resumeConversationID string, planMode bool) error { +func runTUI(baseURL, workspace, resumeConversationID, model string, planMode bool) error { if !term.IsTerminal(int(os.Stdout.Fd())) { return fmt.Errorf("--tui requires a terminal; pipe output or use without --tui for streaming mode") } - tuiCfg := newTUIConfig(baseURL, workspace, resumeConversationID) + tuiCfg := newTUIConfig(baseURL, workspace, resumeConversationID, model) tuiCfg.PlanMode = planMode // Resolve and apply the color profile to the renderer before building the // model, and store the effective profile back for accurate display. diff --git a/cmd/harnesscli/main_test.go b/cmd/harnesscli/main_test.go index ce7b441b..e53fba7e 100644 --- a/cmd/harnesscli/main_test.go +++ b/cmd/harnesscli/main_test.go @@ -184,7 +184,7 @@ func TestRunCreatesAndStreamsToCompletion(t *testing.T) { } func TestNewTUIConfigIncludesWorkspace(t *testing.T) { - cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "") + cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "", "") if cfg.BaseURL != "http://127.0.0.1:8080" { t.Fatalf("BaseURL = %q", cfg.BaseURL) } @@ -200,7 +200,7 @@ func TestNewTUIConfigIncludesWorkspace(t *testing.T) { } func TestNewTUIConfigIncludesResumeConversationID(t *testing.T) { - cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "conv-resume-42") + cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "conv-resume-42", "") if cfg.ResumeConversationID != "conv-resume-42" { t.Fatalf("ResumeConversationID = %q, want %q", cfg.ResumeConversationID, "conv-resume-42") } @@ -328,7 +328,7 @@ func TestNewTUIConfigLoadsSavedTheme(t *testing.T) { t.Fatalf("config.Save: %v", err) } - cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "") + cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "", "") if cfg.Theme != "ocean" { t.Errorf("TUIConfig.Theme = %q, want %q", cfg.Theme, "ocean") } @@ -339,8 +339,39 @@ func TestNewTUIConfigLoadsSavedTheme(t *testing.T) { func TestNewTUIConfigNoSavedThemeIsEmpty(t *testing.T) { t.Setenv("HOME", t.TempDir()) - cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "") + cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "", "") if cfg.Theme != "" { t.Errorf("TUIConfig.Theme = %q, want empty", cfg.Theme) } } + +// TestNewTUIConfigCarriesExplicitModel pins issue #1426: an explicitly +// requested model must reach the TUI. +// +// `harnesscli --tui -model X` parsed the flag and threw it away, so the TUI +// started on the daemon default or — after #1424 — on the remembered model, +// which is precisely the thing a user passing -model is trying to override. +// +// runTUI itself cannot be unit tested (it requires a terminal), so this is the +// closest honest seam: the point where the flag's value becomes TUIConfig.Model. +func TestNewTUIConfigCarriesExplicitModel(t *testing.T) { + cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "", "gpt-4.1-mini") + + if cfg.Model != "gpt-4.1-mini" { + t.Fatalf("TUIConfig.Model = %q, want %q; an explicitly requested model must reach the TUI", + cfg.Model, "gpt-4.1-mini") + } +} + +// TestNewTUIConfigWithoutModelLeavesItEmpty is the control. Empty must stay +// empty: it is what lets the remembered model (#1424) and then the daemon +// default apply. A fix that defaulted to some model would satisfy the test +// above and silently break both. +func TestNewTUIConfigWithoutModelLeavesItEmpty(t *testing.T) { + cfg := newTUIConfig("http://127.0.0.1:8080", "/tmp/tui-project", "", "") + + if cfg.Model != "" { + t.Fatalf("TUIConfig.Model = %q, want empty so the remembered model and daemon default still apply", + cfg.Model) + } +} diff --git a/cmd/harnesscli/main_tui_test.go b/cmd/harnesscli/main_tui_test.go index a415f7f8..bc42ede7 100644 --- a/cmd/harnesscli/main_tui_test.go +++ b/cmd/harnesscli/main_tui_test.go @@ -13,7 +13,7 @@ func TestRunTUIRequiresTerminal(t *testing.T) { t.Skip("stdout is a terminal in this environment") } - err := runTUI("http://localhost:8080", "/tmp/project", "", false) + err := runTUI("http://localhost:8080", "/tmp/project", "", "", false) if err == nil { t.Fatal("expected non-terminal runTUI call to fail") } diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index e7342a13..479d09d1 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -1,5 +1,33 @@ # Engineering Log +## 2026-09-08 — Issue #1426 --tui -model reaches the TUI + +- Symptom: `harnesscli --tui -model X` parsed the flag and threw it away. The + TUI started on the daemon default or, after #1424, on the remembered model — + which is precisely what a user passing `-model` is trying to override. No + warning, no error: the flag was silently discarded. +- Cause: `main.go:179` called `runTUI(*baseURL, workspacePath, *resume, + *planMode)`. `*model` was never among the arguments, and `newTUIConfig` never + set `TUIConfig.Model`. The non-TUI path (`main.go:216`) passed the flag + correctly all along, so the defect was confined to the TUI branch. +- Fix: `model` threaded from the dispatch through `runTUI` into + `newTUIConfig`, following the route `planMode` already takes. Nothing in the + TUI changed — `selectedModel: cfg.Model` and #1424's `cfg.Model == ""` guard + were already correct and simply had no producer of a non-empty value. +- Precedence, now real: flag, then the model remembered from last session, then + the daemon default. The flag deliberately does **not** write to + `~/.config/harnesscli/config.json`: a flag is a one-off instruction, not a + preference, and silently rewriting the saved model would be a worse bug than + the one being fixed. +- Tests: `TestNewTUIConfigCarriesExplicitModel` and, as its control, + `TestNewTUIConfigWithoutModelLeavesItEmpty` — empty must stay empty, or a fix + that defaulted to some model would pass the first test while breaking #1424. + `runTUI` itself requires a terminal and cannot be unit tested, so + `newTUIConfig` is the closest honest seam; the consumer half is already + covered by `TestExplicitModelBeatsRememberedModel` in the `tui` package. + Neither test alone proves the flag works — producer and consumer are pinned + separately and meet only in the live check. + ## 2026-09-08 — Issue #1424 remember the last used model - Symptom: the TUI forgot the model you picked. `/model`, quit, restart, and you @@ -36,9 +64,9 @@ 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. +- Noted, not fixed here: `runTUI` never receives the `-model` flag, so + `harnesscli --tui -model X` ignored it. Filed as #1426 and fixed there; the + precedence guard above needed no change, exactly as predicted. ## 2026-09-08 — Issue #1422 interrupt test flake, and a diagnosis that was wrong twice