diff --git a/cmd/harnesscli/tui/cancel_test.go b/cmd/harnesscli/tui/cancel_test.go index d18eee20..823bd3d7 100644 --- a/cmd/harnesscli/tui/cancel_test.go +++ b/cmd/harnesscli/tui/cancel_test.go @@ -266,7 +266,6 @@ func TestTUI039_EscDismissesBanner(t *testing.T) { // banner visible (first ctrl+c state) and after the second ctrl+c (Interrupted). func TestTUI039_VisualSnapshot_80x24(t *testing.T) { cfg := tui.DefaultTUIConfig() - cfg.SpinnerSeed = 1 // deterministic verb for a stable committed snapshot m := tui.New(cfg) m2, _ := m.Update(tea.WindowSizeMsg{Width: 80, Height: 24}) @@ -309,7 +308,6 @@ func TestTUI039_VisualSnapshot_80x24(t *testing.T) { // TestTUI039_VisualSnapshot_120x40 renders the TUI at 120x40. func TestTUI039_VisualSnapshot_120x40(t *testing.T) { cfg := tui.DefaultTUIConfig() - cfg.SpinnerSeed = 1 // deterministic verb for a stable committed snapshot m := tui.New(cfg) m2, _ := m.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) @@ -335,7 +333,6 @@ func TestTUI039_VisualSnapshot_120x40(t *testing.T) { // TestTUI039_VisualSnapshot_200x50 renders the TUI at 200x50. func TestTUI039_VisualSnapshot_200x50(t *testing.T) { cfg := tui.DefaultTUIConfig() - cfg.SpinnerSeed = 1 // deterministic verb for a stable committed snapshot m := tui.New(cfg) m2, _ := m.Update(tea.WindowSizeMsg{Width: 200, Height: 50}) diff --git a/cmd/harnesscli/tui/components/spinner/model.go b/cmd/harnesscli/tui/components/spinner/model.go index 3e0253cf..276ea3b4 100644 --- a/cmd/harnesscli/tui/components/spinner/model.go +++ b/cmd/harnesscli/tui/components/spinner/model.go @@ -1,11 +1,13 @@ -// Package spinner implements the TUI-024 thinking spinner with rotating verbs. -// It provides an immutable BubbleTea-style Model that advances frame-by-frame -// and rotates through a pool of whimsical verbs (e.g. "Thinking", "Reasoning"). +// Package spinner implements the TUI thinking spinner. It provides an immutable +// BubbleTea-style Model that advances frame-by-frame while displaying a label +// describing what the run is actually doing. +// +// The label is supplied by the caller through SetAction and changes only when +// the run's state changes — never on a timer. See issue #1415. package spinner import ( "fmt" - "math/rand" "time" "github.com/charmbracelet/lipgloss" @@ -15,9 +17,6 @@ import ( // These are star/asterisk glyphs, not the braille frames in theme.go. var frames = []string{"✶", "·", "✻", "✽", "✳", "✢"} -// verbRotateEvery controls how many Tick() calls trigger a verb rotation. -const verbRotateEvery = 8 - // durationThreshold is the elapsed time after which the spinner shows a duration. const durationThreshold = 2 * time.Second @@ -50,47 +49,31 @@ func DefaultStyles() Styles { // All mutation methods return a new Model value — never modify in place. // This keeps it safe for use in BubbleTea's single-goroutine Update(). type Model struct { - frame int // current frame index [0, len(frames)) - verb string // current displayed verb - action string // current activity label (e.g. running tool name); overrides verb when set - startTime time.Time // when spinner started (for duration) - tokens int // token count stored on Stop() - active bool // true while spinner is running - done bool // true after Stop() - tickCount int // total ticks received (used for verb rotation) - completionFrames int // ticks remaining to show completion line after Stop() - rng *rand.Rand // seeded rng for deterministic testing - - // Seed is the seed used to create rng. Exposed so tests can inspect it. + frame int // current frame index [0, len(frames)) + action string // what the run is currently doing; empty falls back to fallbackLabel + startTime time.Time // when spinner started (for duration) + tokens int // token count stored on Stop() + active bool // true while spinner is running + done bool // true after Stop() + tickCount int // total ticks received + completionFrames int // ticks remaining to show completion line after Stop() + // Seed is retained only so the many existing New(seed) call sites keep + // compiling. Nothing reads it: the label comes from run state, not from a + // random source, so rendering is deterministic without a seed. Issue #1415. Seed int64 - // testVerbs overrides DefaultVerbs when non-nil. For testing only. - testVerbs []string - // styles overrides DefaultStyles when non-nil (theme injection point, // epic #810). styles *Styles } -// New creates a new Model with the given seed. The seed makes verb selection -// deterministic which is essential for snapshot and regression tests. +// New creates a new Model. seed is ignored — it is kept only for call-site +// compatibility, since rendering no longer depends on randomness. See Model.Seed. func New(seed int64) Model { - return Model{ - Seed: seed, - rng: rand.New(rand.NewSource(seed)), //nolint:gosec // not for crypto - } + return Model{Seed: seed} } -// verbPool returns the verb pool in effect: testVerbs override if set, -// otherwise DefaultVerbs. -func (m Model) verbPool() []string { - if m.testVerbs != nil { - return m.testVerbs - } - return DefaultVerbs -} - -// Start activates the spinner, records the start time, and picks an initial verb. +// Start activates the spinner and records the start time. // Returns a new Model; the receiver is unchanged. func (m Model) Start() Model { m.active = true @@ -98,11 +81,11 @@ func (m Model) Start() Model { m.startTime = time.Now() m.frame = 0 m.tickCount = 0 - m.verb = pickVerb(m.verbPool(), m.rng) return m } -// Tick advances the animation by one frame and potentially rotates the verb. +// Tick advances the animation by one frame. The label is deliberately untouched: +// it changes only when the caller reports a new action. // When the spinner is done and completionFrames > 0, decrements completionFrames // toward silence. Has no effect if neither active nor in completion mode. // Returns a new Model; the receiver is unchanged. @@ -120,10 +103,6 @@ func (m Model) Tick() Model { } m.tickCount++ m.frame = (m.frame + 1) % len(frames) - // Rotate verb every verbRotateEvery ticks. - if m.tickCount%verbRotateEvery == 0 { - m.verb = pickVerb(m.verbPool(), m.rng) - } return m } @@ -139,10 +118,9 @@ func (m Model) Stop(tokens int) Model { return m } -// SetAction sets the current activity label (e.g. the name of a running tool -// or a short step description). When non-empty, View() displays it in place -// of the rotating verb so the user sees what is actually happening rather -// than a generic placeholder. Pass "" to fall back to verb rotation. +// SetAction sets what the run is currently doing (e.g. "Running bash", +// "Writing response"). This is the label View() renders. Pass "" only when +// nothing is known, which falls back to fallbackLabel. // Returns a new Model; the receiver is unchanged. func (m Model) SetAction(action string) Model { m.action = action @@ -187,8 +165,7 @@ func (m Model) ElapsedSeconds() float64 { // maximum character width; the view degrades gracefully at narrow widths. // // States: -// - Active: "✻ Thinking... (esc to interrupt)", or with a known action, -// "✻ Running bash (esc to interrupt)"; a duration is inserted once +// - Active: "✻ Running bash (esc to interrupt)"; a duration is inserted once // durationThreshold passes. // - ShowsCompletion() true: CompletionLine using ElapsedSeconds(). // - Done and silent (completionFrames == 0): returns "". @@ -208,11 +185,11 @@ func (m Model) View(width int) string { currentFrame := frames[m.frame] - // Build the base text. When a current action is known, show it instead of - // the rotating verb so the user sees what is actually happening. - label := m.verb + "..." - if m.action != "" { - label = m.action + // The label states what is actually happening. No ellipsis: "Running bash" + // is a fact, and trailing dots would only suggest vagueness it does not have. + label := m.action + if label == "" { + label = fallbackLabel } base := currentFrame + " " + label @@ -229,17 +206,52 @@ func (m Model) View(width int) string { base += " " + CancelHint } + // At narrow widths the label yields before the cancel hint does. Truncating + // from the right would eat "(esc to interrupt)" first, leaving the user + // staring at "(esc to inter" with no way to know how to stop the run — the + // hint is the one part of this line that is actionable. Labels grew long + // enough to hit this when they became truthful ("Waiting for gpt-4.1-mini" + // rather than "Computing..."), so the trade-off is now worth making + // explicit. Issue #1415. + if lipgloss.Width(base) > width { + base = shortenLabel(currentFrame, label, base, width) + } + style := m.stylesOrDefault().Dim rendered := style.Render(base) - // Clamp to width using MaxWidth. - if width < 80 { + // Final clamp: even a shortened line cannot exceed the terminal. + if lipgloss.Width(base) > width { rendered = lipgloss.NewStyle().MaxWidth(width).Render(base) } return rendered } +// shortenLabel rebuilds an over-long spinner line so the cancel hint survives. +// It first drops the duration, then truncates the label itself, and gives up +// only when even " " will not fit — at which point the caller's +// MaxWidth clamp takes over. +func shortenLabel(glyph, label, full string, width int) string { + withoutDuration := glyph + " " + label + " " + CancelHint + if lipgloss.Width(withoutDuration) <= width { + return withoutDuration + } + + // Budget for the label: width minus the glyph, the hint, and the two spaces + // separating them. + budget := width - lipgloss.Width(glyph) - lipgloss.Width(CancelHint) - 2 + if budget < 4 { + // Not even a stub of a label fits; the hint alone is more useful. + return glyph + " " + CancelHint + } + runes := []rune(label) + if len(runes) > budget { + label = string(runes[:budget-1]) + "\u2026" + } + return glyph + " " + label + " " + CancelHint +} + // CompletionLine returns the one-line completion summary shown after the spinner stops. // // Format: "✻ Worked for 5s" or "✻ Worked for 1m 30s" diff --git a/cmd/harnesscli/tui/components/spinner/model_test.go b/cmd/harnesscli/tui/components/spinner/model_test.go index aa3719ab..99cf74a5 100644 --- a/cmd/harnesscli/tui/components/spinner/model_test.go +++ b/cmd/harnesscli/tui/components/spinner/model_test.go @@ -50,26 +50,6 @@ func TestTUI024_SpinnerAddsDurationAfterThreshold(t *testing.T) { } } -// TestTUI024_SpinnerVerbFromSeed verifies that using the same seed always -// produces the same initial verb after Start(). -func TestTUI024_SpinnerVerbFromSeed(t *testing.T) { - const seed = int64(12345) - - m1 := New(seed) - m1 = m1.Start() - - m2 := New(seed) - m2 = m2.Start() - - if m1.verb != m2.verb { - t.Errorf("same seed should produce same verb: m1=%q, m2=%q", m1.verb, m2.verb) - } - - if m1.verb == "" { - t.Error("verb should not be empty after Start()") - } -} - // TestTUI024_SpinnerStopsCleanly verifies that Stop() transitions the model to // done=true, active=false and stores the token count. func TestTUI024_SpinnerStopsCleanly(t *testing.T) { @@ -166,19 +146,6 @@ func TestTUI024_ConcurrentIndependentState(t *testing.T) { wg.Wait() } -// TestTUI024_EmptyVerbFallback verifies that an empty verb pool falls back -// to "Thinking". -func TestTUI024_EmptyVerbFallback(t *testing.T) { - m := New(42) - // Override verbs to empty slice via test helper. - m.testVerbs = []string{} - m = m.Start() - - if m.verb != "Thinking" { - t.Errorf("empty verb pool should fall back to 'Thinking', got %q", m.verb) - } -} - // TestTUI024_BoundaryWidths verifies that View() does not panic at various // terminal widths including very narrow and very wide. func TestTUI024_BoundaryWidths(t *testing.T) { @@ -203,8 +170,8 @@ func TestTUI024_BoundaryWidths(t *testing.T) { } // TestSpinnerShowsCancelHintWhileActive verifies that View() always surfaces -// the cancel hint while the spinner is active, using the rotating verb when -// no current action has been set. +// the cancel hint while the spinner is active, falling back to the neutral +// label when no current action has been set. func TestSpinnerShowsCancelHintWhileActive(t *testing.T) { m := New(42) m = m.Start() @@ -213,15 +180,14 @@ func TestSpinnerShowsCancelHintWhileActive(t *testing.T) { if !strings.Contains(view, CancelHint) { t.Errorf("active View() should contain cancel hint %q, got: %q", CancelHint, view) } - if !strings.Contains(view, m.verb) { - t.Errorf("active View() with no action set should still show the verb %q, got: %q", m.verb, view) + if !strings.Contains(view, fallbackLabel) { + t.Errorf("active View() with no action set should show %q, got: %q", fallbackLabel, view) } } -// TestSpinnerShowsCurrentActionInsteadOfVerb verifies that once SetAction is -// called with a non-empty label, View() displays that label instead of the -// rotating verb, while still showing the cancel hint. -func TestSpinnerShowsCurrentActionInsteadOfVerb(t *testing.T) { +// TestSpinnerShowsCurrentAction verifies that SetAction's label is what View() +// renders, while still showing the cancel hint. +func TestSpinnerShowsCurrentAction(t *testing.T) { m := New(42) m = m.Start() m = m.SetAction("Running bash") @@ -230,17 +196,17 @@ func TestSpinnerShowsCurrentActionInsteadOfVerb(t *testing.T) { if !strings.Contains(view, "Running bash") { t.Errorf("View() should show the current action, got: %q", view) } - if strings.Contains(view, m.verb+"...") { - t.Errorf("View() should not show the rotating verb once an action is set, got: %q", view) + if strings.Contains(view, fallbackLabel) { + t.Errorf("View() should not show the fallback label once an action is set, got: %q", view) } if !strings.Contains(view, CancelHint) { t.Errorf("View() with an action set should still contain cancel hint %q, got: %q", CancelHint, view) } } -// TestSpinnerClearingActionRestoresVerb verifies that SetAction("") reverts -// View() back to the rotating verb. -func TestSpinnerClearingActionRestoresVerb(t *testing.T) { +// TestSpinnerClearingActionRestoresFallback verifies that SetAction("") reverts +// View() to the neutral label. +func TestSpinnerClearingActionRestoresFallback(t *testing.T) { m := New(42) m = m.Start() m = m.SetAction("Running bash") @@ -250,8 +216,8 @@ func TestSpinnerClearingActionRestoresVerb(t *testing.T) { if strings.Contains(view, "Running bash") { t.Errorf("View() should not show a cleared action, got: %q", view) } - if !strings.Contains(view, m.verb) { - t.Errorf("View() should fall back to the verb once action is cleared, got: %q", view) + if !strings.Contains(view, fallbackLabel) { + t.Errorf("View() should fall back to %q once the action is cleared, got: %q", fallbackLabel, view) } } diff --git a/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-120x40.txt b/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-120x40.txt index ded8c397..baf4b683 100644 --- a/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-120x40.txt +++ b/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-120x40.txt @@ -1,10 +1,10 @@ # TUI-024 Spinner Snapshot 120x40 ------------------------------------------------------------------------------------------------------------------------ ## Active (no duration) -✽ Computing... (esc to interrupt) +✽ Working (esc to interrupt) ## Active (5s elapsed) -✽ Computing... (5.0s) (esc to interrupt) +✽ Working (5.0s) (esc to interrupt) ## Completion Line ✽ Worked for 5.0s diff --git a/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-200x50.txt b/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-200x50.txt index 7d9a7c54..5847de7f 100644 --- a/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-200x50.txt +++ b/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-200x50.txt @@ -1,10 +1,10 @@ # TUI-024 Spinner Snapshot 200x50 -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- ## Active (no duration) -✽ Computing... (esc to interrupt) +✽ Working (esc to interrupt) ## Active (5s elapsed) -✽ Computing... (5.0s) (esc to interrupt) +✽ Working (5.0s) (esc to interrupt) ## Completion Line ✽ Worked for 5.0s diff --git a/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-80x24.txt b/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-80x24.txt index 00ed28ba..ae11eb84 100644 --- a/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-80x24.txt +++ b/cmd/harnesscli/tui/components/spinner/testdata/snapshots/TUI-024-spinner-80x24.txt @@ -1,10 +1,10 @@ # TUI-024 Spinner Snapshot 80x24 -------------------------------------------------------------------------------- ## Active (no duration) -✽ Computing... (esc to interrupt) +✽ Working (esc to interrupt) ## Active (5s elapsed) -✽ Computing... (5.0s) (esc to interrupt) +✽ Working (5.0s) (esc to interrupt) ## Completion Line ✽ Worked for 5.0s diff --git a/cmd/harnesscli/tui/components/spinner/truthful_label_test.go b/cmd/harnesscli/tui/components/spinner/truthful_label_test.go new file mode 100644 index 00000000..d14d8bac --- /dev/null +++ b/cmd/harnesscli/tui/components/spinner/truthful_label_test.go @@ -0,0 +1,115 @@ +package spinner + +import ( + "strings" + "testing" + + "github.com/charmbracelet/lipgloss" +) + +// TestSpinnerLabelDoesNotRotateOnTicks pins the core of issue #1415: the label +// changes when the run's state changes, never because a timer advanced. +// +// The old model re-rolled a random verb every 8 ticks (~960ms at the 120ms tick +// rate), so the line changed roughly once a second while telling the user +// nothing new. Motion without novelty reads as a stuck animation. +func TestSpinnerLabelDoesNotRotateOnTicks(t *testing.T) { + m := New(42).Start().SetAction("Running bash") + + first := m.View(80) + for i := 0; i < 30; i++ { // well past the old verbRotateEvery = 8 + m = m.Tick() + if got := m.View(80); labelOf(got) != labelOf(first) { + t.Fatalf("label changed on tick %d without any state change:\n before: %q\n after: %q", + i+1, first, got) + } + } +} + +// TestSpinnerGlyphStillAnimates is the control for the test above: freezing the +// whole line would satisfy "label does not change" while making the spinner look +// hung. Only the word is allowed to hold still. +func TestSpinnerGlyphStillAnimates(t *testing.T) { + m := New(42).Start().SetAction("Running bash") + + seen := map[string]bool{} + for i := 0; i < len(frames); i++ { + seen[strings.Fields(m.View(80))[0]] = true + m = m.Tick() + } + if len(seen) < 2 { + t.Fatalf("glyph never advanced across %d ticks; the spinner would look frozen", len(frames)) + } +} + +// TestSpinnerLabelIsSeedIndependent is the deliberate inversion of the old +// TestTUI024_SpinnerVerbFromSeed. Once the label is a function of run state, two +// spinners in the same state must read identically regardless of seed — there is +// no longer a random source to seed. +func TestSpinnerLabelIsSeedIndependent(t *testing.T) { + a := New(1).Start().SetAction("Thinking") + b := New(999999).Start().SetAction("Thinking") + + if labelOf(a.View(80)) != labelOf(b.View(80)) { + t.Fatalf("label depends on seed: %q vs %q", a.View(80), b.View(80)) + } +} + +// TestSpinnerFallbackLabelWhenNoActionKnown pins that an empty action degrades +// to one neutral, truthful word rather than a random verb. +func TestSpinnerFallbackLabelWhenNoActionKnown(t *testing.T) { + m := New(7).Start() + got := labelOf(m.View(80)) + if got != fallbackLabel { + t.Fatalf("empty action should render %q, got %q", fallbackLabel, got) + } + if strings.Contains(m.View(80), "...") { + t.Errorf("label should not carry an ellipsis: %q", m.View(80)) + } +} + +// labelOf extracts the label from a rendered spinner line, dropping the leading +// glyph and any trailing duration or cancel hint. +func labelOf(view string) string { + fields := strings.Fields(view) + if len(fields) < 2 { + return "" + } + var out []string + for _, f := range fields[1:] { + if strings.HasPrefix(f, "(") { + break + } + out = append(out, f) + } + return strings.Join(out, " ") +} + +// TestSpinnerKeepsCancelHintAtNarrowWidth pins the width trade-off introduced +// with truthful labels: "Waiting for gpt-4.1-mini" is much longer than the +// "Computing..." it replaced, and a right-truncated line would eat the cancel +// hint first — leaving "(esc to inter" and no way to learn how to stop the run. +// The label yields; the hint does not. +func TestSpinnerKeepsCancelHintAtNarrowWidth(t *testing.T) { + m := New(0).Start().SetAction("Waiting for some-extremely-long-model-name-v2") + + for _, width := range []int{40, 50, 60} { + view := m.View(width) + if !strings.Contains(view, CancelHint) { + t.Errorf("width %d: cancel hint dropped, got %q", width, view) + } + if got := lipgloss.Width(view); got > width { + t.Errorf("width %d: line is %d columns wide: %q", width, got, view) + } + } +} + +// TestSpinnerHintSurvivesEvenWhenLabelCannotFit covers the degenerate case: when +// not even a stub of a label fits, the actionable half is what remains. +func TestSpinnerHintSurvivesEvenWhenLabelCannotFit(t *testing.T) { + m := New(0).Start().SetAction("Waiting for a model with an absurd name") + view := m.View(24) + if !strings.Contains(view, CancelHint) { + t.Errorf("cancel hint should outrank the label when space is scarce, got %q", view) + } +} diff --git a/cmd/harnesscli/tui/components/spinner/verbs.go b/cmd/harnesscli/tui/components/spinner/verbs.go index 0505fb01..6ff3fd5b 100644 --- a/cmd/harnesscli/tui/components/spinner/verbs.go +++ b/cmd/harnesscli/tui/components/spinner/verbs.go @@ -1,21 +1,12 @@ package spinner -// DefaultVerbs is the pool of whimsical verbs displayed by the spinner. -// These are the same verbs Claude Code uses in its thinking indicator. -var DefaultVerbs = []string{ - "Thinking", "Reasoning", "Pondering", "Analyzing", "Processing", - "Computing", "Synthesizing", "Evaluating", "Reflecting", "Deliberating", - "Considering", "Examining", "Contemplating", "Strategizing", "Planning", -} - -// fallbackVerb is used when the verb pool is empty. -const fallbackVerb = "Thinking" - -// pickVerb selects a random verb from pool using the provided rng. -// Returns fallbackVerb if pool is empty. -func pickVerb(pool []string, rng interface{ Intn(int) int }) string { - if len(pool) == 0 { - return fallbackVerb - } - return pool[rng.Intn(len(pool))] -} +// fallbackLabel is shown only when the caller has not told the spinner what is +// happening. It is deliberately the single most neutral true statement we can +// make: a run is in progress. +// +// This replaced a pool of fifteen near-synonyms for "thinking" that rotated +// roughly once a second (issue #1415). Rotating decorative words tells the user +// nothing — the label changed but the meaning did not, which reads as a stuck +// animation rather than a live one. Callers should set a real action via +// SetAction; see currentSpinnerAction in the parent tui package. +const fallbackLabel = "Working" diff --git a/cmd/harnesscli/tui/config.go b/cmd/harnesscli/tui/config.go index e0eb2718..74acd9fa 100644 --- a/cmd/harnesscli/tui/config.go +++ b/cmd/harnesscli/tui/config.go @@ -28,10 +28,6 @@ type TUIConfig struct { ResumeConversationID string // PlanMode starts the TUI in enforced plan mode (harnesscli --tui --plan-mode). PlanMode bool - // SpinnerSeed seeds the thinking-spinner's verb selection. Zero (the default) - // uses a time-based seed for whimsical variety in real use; tests set a fixed - // non-zero seed so rendered snapshots are deterministic. - SpinnerSeed int64 // APIKey authenticates requests to the harnessd server // ("Authorization: Bearer "), including the SSE event stream // (see bridge.go's SSEBridgeOptions). Empty means unauthenticated, diff --git a/cmd/harnesscli/tui/model.go b/cmd/harnesscli/tui/model.go index 02f66fe9..65165ff6 100644 --- a/cmd/harnesscli/tui/model.go +++ b/cmd/harnesscli/tui/model.go @@ -485,14 +485,6 @@ type Model struct { } // New creates a new root Model. -// spinnerSeed returns the spinner verb seed: the configured value when non-zero -// (deterministic, used by tests), otherwise a time-based seed for variety. -func spinnerSeed(cfg TUIConfig) int64 { - if cfg.SpinnerSeed != 0 { - return cfg.SpinnerSeed - } - return time.Now().UnixNano() -} func New(cfg TUIConfig) Model { m := Model{ @@ -503,7 +495,7 @@ func New(cfg TUIConfig) Model { contextGrid: contextgrid.New(), statsPanel: statspanel.New(nil), costDisplay: costdisplay.New(), - spinner: spinner.New(spinnerSeed(cfg)), + spinner: spinner.New(0), thinkingBar: thinkingbar.New(), interruptBanner: interruptui.New(), selectedModel: cfg.Model, @@ -1168,20 +1160,33 @@ func spinnerTickCmd() tea.Cmd { return tea.Tick(SpinnerInterval, func(t time.Time) tea.Msg { return spinner.SpinnerTickMsg{T: t} }) } -// currentSpinnerAction returns a short label describing what is currently -// running, for display in the spinner, or "" when nothing more specific than -// "thinking" is known. Only reports the active tool while it is genuinely -// still running — activeToolCallID lingers after completion so it can be -// expanded/collapsed, but by then there is nothing left to announce. +// currentSpinnerAction returns a short label describing what the run is +// actually doing, for display in the spinner. +// +// It never returns "": there is always something true to say, which is what +// makes a pool of decorative synonyms unnecessary (issue #1415). The ladder is +// ordered most-specific first, because several of these are true at once — a +// tool runs *while* a response is streaming, and reasoning arrives before text. +// The user is best served by the narrowest true statement. +// +// The running-tool check is deliberately strict: activeToolCallID lingers after +// completion so the call can still be expanded or collapsed, but by then the +// tool is no longer what is happening. func (m Model) currentSpinnerAction() string { - if m.activeToolCallID == "" { - return "" + if view, ok := m.toolViews[m.activeToolCallID]; m.activeToolCallID != "" && ok && + view.Status == "running" && view.ToolName != "" { + return "Running " + view.ToolName } - view, ok := m.toolViews[m.activeToolCallID] - if !ok || view.Status != "running" || view.ToolName == "" { - return "" + if m.responseStarted { + return "Writing response" + } + if m.thinkingText != "" { + return "Thinking" + } + if m.selectedModel != "" { + return "Waiting for " + m.selectedModel } - return "Running " + view.ToolName + return "Waiting for model" } // StatusTickMsgForTesting returns a statusTickMsg as a tea.Msg for use in @@ -4203,7 +4208,7 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.activeAssistantLineCount = 0 } m.clearThinkingBar() - m.spinner = spinner.New(spinnerSeed(m.config)).WithStyles(spinnerStylesFromTheme(m.theme)).Start() + m.spinner = spinner.New(0).WithStyles(spinnerStylesFromTheme(m.theme)).Start() cmds = append(cmds, spinnerTickCmd()) // Continuations provide their inherited conversation identity. A blank // TUI adopts it; an already selected conversation remains authoritative diff --git a/cmd/harnesscli/tui/spinner_truthful_action_test.go b/cmd/harnesscli/tui/spinner_truthful_action_test.go new file mode 100644 index 00000000..175a793e --- /dev/null +++ b/cmd/harnesscli/tui/spinner_truthful_action_test.go @@ -0,0 +1,87 @@ +package tui + +import ( + "testing" + + "go-agent-harness/cmd/harnesscli/tui/components/tooluse" +) + +// TestCurrentSpinnerActionLadder pins issue #1415: the spinner says what is +// actually happening, and when several things are true at once the most +// specific one wins. +// +// Before this change currentSpinnerAction reported only a running tool and +// returned "" for every other state, leaving a rotating pool of fifteen +// synonyms for "thinking" to fill the silence. +func TestCurrentSpinnerActionLadder(t *testing.T) { + const model = "gpt-4.1-mini" + + runningTool := func(m *Model) { + m.activeToolCallID = "call_1" + m.toolViews = map[string]tooluse.Model{ + "call_1": {ToolName: "bash", Status: "running"}, + } + } + + for _, tc := range []struct { + name string + setup func(*Model) + want string + }{ + { + name: "nothing back yet names the model we are waiting on", + setup: func(m *Model) {}, + want: "Waiting for " + model, + }, + { + name: "reasoning arrived but no text yet", + setup: func(m *Model) { m.thinkingText = "considering the file layout" }, + want: "Thinking", + }, + { + name: "assistant text streaming outranks reasoning", + setup: func(m *Model) { + m.thinkingText = "considering the file layout" + m.responseStarted = true + }, + want: "Writing response", + }, + { + name: "a running tool outranks everything", + setup: func(m *Model) { + m.thinkingText = "considering the file layout" + m.responseStarted = true + runningTool(m) + }, + want: "Running bash", + }, + { + name: "a finished tool no longer counts", + setup: func(m *Model) { + m.activeToolCallID = "call_1" + m.toolViews = map[string]tooluse.Model{ + "call_1": {ToolName: "bash", Status: "completed"}, + } + m.responseStarted = true + }, + want: "Writing response", + }, + } { + t.Run(tc.name, func(t *testing.T) { + m := &Model{selectedModel: model} + tc.setup(m) + if got := m.currentSpinnerAction(); got != tc.want { + t.Fatalf("currentSpinnerAction() = %q, want %q", got, tc.want) + } + }) + } +} + +// TestCurrentSpinnerActionNeverEmptyWhileRunning guards the contract that makes +// the verb pool unnecessary: there is always something true to say. +func TestCurrentSpinnerActionNeverEmptyWhileRunning(t *testing.T) { + m := &Model{} + if got := m.currentSpinnerAction(); got == "" { + t.Fatal("currentSpinnerAction() returned empty; the spinner would fall back to a generic label") + } +} diff --git a/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-120x40.txt b/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-120x40.txt index e81cf696..2f13664d 100644 --- a/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-120x40.txt +++ b/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-120x40.txt @@ -33,7 +33,7 @@ ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── -✶ Examining... (esc to interrupt) +✶ Working (esc to interrupt) ╭───────────────────────────────────────────────────╮ │ ⚠ Press Ctrl+C again to stop, or Esc to continue │ ╰───────────────────────────────────────────────────╯ diff --git a/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-200x50.txt b/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-200x50.txt index 4be5a763..bda3da57 100644 --- a/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-200x50.txt +++ b/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-200x50.txt @@ -43,7 +43,7 @@ ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── -✶ Examining... (esc to interrupt) +✶ Working (esc to interrupt) ╭───────────────────────────────────────────────────╮ │ ⚠ Press Ctrl+C again to stop, or Esc to continue │ ╰───────────────────────────────────────────────────╯ diff --git a/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-80x24.txt b/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-80x24.txt index dabdca07..a4df1a60 100644 --- a/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-80x24.txt +++ b/cmd/harnesscli/tui/testdata/snapshots/TUI-039-cancel-80x24.txt @@ -17,7 +17,7 @@ ──────────────────────────────────────────────────────────────────────────────── -✶ Examining... (esc to interrupt) +✶ Working (esc to interrupt) ╭───────────────────────────────────────────────────╮ │ ⚠ Press Ctrl+C again to stop, or Esc to continue │ ╰───────────────────────────────────────────────────╯ diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index e493a76b..9cd4cce3 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -1,5 +1,76 @@ # Engineering Log +## 2026-09-08 — Issue #1415 truthful spinner label + +- Symptom/motivation: `cmd/harnesscli/tui/components/spinner/verbs.go` held + `DefaultVerbs`, a pool of fifteen near-synonyms for "thinking" ("Thinking", + "Reasoning", "Pondering", "Analyzing", "Processing", "Computing", + "Synthesizing", "Evaluating", "Reflecting", "Deliberating", "Considering", + "Examining", "Contemplating", "Strategizing", "Planning"). `verbRotateEvery + = 8` re-picked one every 8 ticks at the 120ms tick rate, so the word changed + roughly once a second while telling the user nothing new — motion without + novelty reads as a stuck animation, not a live one. A comment on + `DefaultVerbs` also claimed these were "the same verbs Claude Code uses in + its thinking indicator," which was false. +- Research that informed the decision (secondary, not verified against the + actual Claude Code source): an extraction across 139 published + `@anthropic-ai/claude-code` npm versions (the `levindixon/tengu_spinner_words` + repo) reports roughly 90 varied whimsical words plus a runtime-fetched + Statsig dynamic-config set merged in, and a word-rotation interval of + roughly 1000ms. Our own rotation was ~960ms (8 ticks × 120ms) — essentially + the same speed. So speed was never the differentiator here; vocabulary was. + Nobody on this project has extracted the literal interval from the minified + Claude Code bundle, so that 1000ms figure is a well-sourced hypothesis, not + a confirmed fact — treat it accordingly. By contrast, opencode + (`anomalyco/opencode`, `packages/tui/src/component/spinner.tsx`) is a + primary source we could read directly, and it uses no randomized words at + all. +- Decision: matching a 90-plus-word decorative list would mean maintaining it + against a scale we cannot verify, from a secondary source, to reproduce an + effect that depends on a runtime-fetched set we do not have. Rather than + that, or keeping the false "same as Claude Code" claim, say something true + about what the run is doing. The false comment was removed along with `DefaultVerbs`. +- Fix: `verbs.go` now holds one `fallbackLabel = "Working"` constant + (`cmd/harnesscli/tui/components/spinner/verbs.go:12`), used only when no + action is known. `currentSpinnerAction()` in + `cmd/harnesscli/tui/model.go:1175` returns a most-specific-first ladder: a + tool genuinely still running -> "Running "; assistant text streaming + (`responseStarted`) -> "Writing response"; reasoning deltas arrived + (`thinkingText != ""`) -> "Thinking"; otherwise -> "Waiting for " (or + "Waiting for model" with none selected). It never returns "", which is what + makes the verb pool unnecessary — there is always something true to say, and + it is always narrower than "thinking". The glyph still animates every 120ms so the line reads as + live; only the word now holds still until the state actually changes. The + trailing "..." was dropped too ("Running bash" is a fact, not a vague one). + `TUIConfig.SpinnerSeed` and the `spinnerSeed()` helper were removed — they + existed only to make random verb selection deterministic for snapshots, and + rendering is now deterministic by construction, so the field would have + been a setting that no longer did anything. `spinner.New(seed)` keeps its + parameter for its ~115 call sites but ignores it. +- Trade-off: "Waiting for gpt-4.1-mini" is much longer than the "Computing..." + it replaced, and at 40 columns a right-truncated line ate the cancel hint, + rendering "(esc to inter" with no way to tell the user how to stop the run. + `shortenLabel` (`cmd/harnesscli/tui/components/spinner/model.go:235`) now + drops the duration first, then truncates the label with an ellipsis, so + "(esc to interrupt)" always survives — the label yields, the hint does not. + At 40 columns this renders `✶ Waiting for gpt-4.… (esc to interrupt)`. +- Durable note: the six glyphs (`✶ · ✻ ✽ ✳ ✢`, + `cmd/harnesscli/tui/components/spinner/model.go:18`) do match the six + independently reported for Claude Code, so the glyph layer was already + right and was deliberately left alone — only the word layer changed. +- Tests: added `TestSpinnerLabelDoesNotRotateOnTicks`, + `TestSpinnerGlyphStillAnimates` (control against freezing the whole line), + `TestSpinnerLabelIsSeedIndependent`, `TestSpinnerFallbackLabelWhenNoActionKnown`, + `TestSpinnerKeepsCancelHintAtNarrowWidth`, + `TestSpinnerHintSurvivesEvenWhenLabelCannotFit` in + `cmd/harnesscli/tui/components/spinner/truthful_label_test.go`, and + `TestCurrentSpinnerActionLadder` (5 cases) plus + `TestCurrentSpinnerActionNeverEmptyWhileRunning` in + `cmd/harnesscli/tui/spinner_truthful_action_test.go`. Removed + `TestTUI024_SpinnerVerbFromSeed` and `TestTUI024_EmptyVerbFallback`, which + pinned the deleted rotation behavior. Snapshot goldens regenerated: + `✽ Computing... (esc to interrupt)` became `✽ Working (esc to interrupt)`. + ## 2026-09-08 — Issue #1416 closed output pipe orphaned harnessd - Symptom: `go-code runs | head -5` (or piping into a pager the user quits