diff --git a/cmd/harnesscli/tui/components/spinner/model.go b/cmd/harnesscli/tui/components/spinner/model.go index c8377dea..95618265 100644 --- a/cmd/harnesscli/tui/components/spinner/model.go +++ b/cmd/harnesscli/tui/components/spinner/model.go @@ -277,13 +277,53 @@ func shortenLabel(glyph, label, full string, width int) string { // 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" - } + label = truncateToWidth(label, budget) return glyph + " " + label + " " + CancelHint } +// truncateToWidth shortens label to at most budget terminal columns, appending +// an ellipsis when it cuts. +// +// It measures display columns rather than runes. Counting runes was the bug in +// issue #1432: CJK and emoji occupy two columns each, so a rune-budgeted label +// could be twice its allowance in columns, overrun the line, and get clipped +// from the right — taking the cancel hint with it. ASCII hid this completely, +// because there columns and runes are the same number. +// +// Rune-by-rune accumulation is deliberate and imperfect: it does not segment +// grapheme clusters, so a combining mark or ZWJ emoji sequence can still be +// split. That is a strict improvement over rune counting and a much smaller +// change than full segmentation, which is out of scope until a real case +// appears. +func truncateToWidth(label string, budget int) string { + if budget <= 0 { + return "" + } + if lipgloss.Width(label) <= budget { + return label + } + + const ellipsis = "\u2026" + room := budget - lipgloss.Width(ellipsis) + if room <= 0 { + return ellipsis + } + + var ( + out []rune + width int + ) + for _, r := range label { + w := lipgloss.Width(string(r)) + if width+w > room { + break + } + out = append(out, r) + width += w + } + return string(out) + ellipsis +} + // 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/truthful_label_test.go b/cmd/harnesscli/tui/components/spinner/truthful_label_test.go index b8b211d6..e453bf6f 100644 --- a/cmd/harnesscli/tui/components/spinner/truthful_label_test.go +++ b/cmd/harnesscli/tui/components/spinner/truthful_label_test.go @@ -116,3 +116,40 @@ func TestSpinnerHintSurvivesEvenWhenLabelCannotFit(t *testing.T) { t.Errorf("cancel hint should outrank the label when space is scarce, got %q", view) } } + +// TestSpinnerKeepsCancelHintWithWideRunes pins issue #1432: the hint survives +// regardless of the label's script. +// +// shortenLabel budgeted in display columns but truncated by rune count. For +// CJK and emoji, which occupy two columns per rune, the label overran its +// budget and the final MaxWidth clamp ate the hint — the exact outcome +// shortenLabel exists to prevent. Every existing test passed throughout, +// because they were all ASCII, where columns and runes coincide. +// +// The assertion is on the hint's survival, not on the line's width: the width +// was always correct, which is why this went unnoticed. +func TestSpinnerKeepsCancelHintWithWideRunes(t *testing.T) { + for _, tc := range []struct { + name string + label string + }{ + {name: "cjk", label: "Waiting for 模型模型模型模型模型模型模型模型"}, + {name: "emoji", label: "Running 🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀"}, + // ASCII control: a fix that over-truncates everything to satisfy the + // hint assertion would show up here as a needlessly stunted label. + {name: "ascii control", label: "Waiting for some-long-model-name-v2"}, + } { + t.Run(tc.name, func(t *testing.T) { + m := New(0).Start().SetAction(tc.label) + for _, width := range []int{40, 50} { + view := m.View(width) + if !strings.Contains(view, CancelHint) { + t.Errorf("width %d: cancel hint truncated, got %q", width, view) + } + if got := lipgloss.Width(view); got > width { + t.Errorf("width %d: line is %d columns wide: %q", width, got, view) + } + } + }) + } +} diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index 7dba2d52..2aa892d2 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -1,5 +1,44 @@ # Engineering Log +## 2026-09-08 — Issue #1432 wide labels destroyed the cancel hint + +- Symptom: `shortenLabel` guarantees the `(esc to interrupt)` hint survives at + narrow widths, sacrificing the label instead. For double-width characters it + did the opposite: + ``` + width=40 "· Waiting for 模型模型模型… (esc to inte" + width=40 "· Running 🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀… (esc to " + ``` + The line width was always correct — the final `MaxWidth` clamp saw to that — + which is exactly why nothing caught it. The clamp was cutting the hint off + because the label had not been shortened enough. +- Cause: the function mixed two units. The budget was computed in display + columns (`lipgloss.Width`) but the truncation was applied in runes + (`runes[:budget-1]`). CJK and emoji occupy two columns per rune, so a + rune-budgeted label could be twice its allowance in columns. For ASCII the two + units coincide, so every existing test passed. +- Fix: `truncateToWidth` accumulates `lipgloss.Width` per rune until the budget + is reached, counting the ellipsis's own width. Deliberately not + grapheme-cluster aware — combining marks and ZWJ sequences can still be split. + That is a strict improvement over rune counting and a far smaller change than + full segmentation, which stays out of scope until a real case appears. +- Tests: `TestSpinnerKeepsCancelHintWithWideRunes` over CJK, emoji and an ASCII + control at widths 40 and 50, asserting the hint's *survival* rather than the + line's width — the width was never wrong, and asserting it is what let the bug + hide. The ASCII control guards against a fix that satisfies the hint + assertion by over-truncating every label. +- **Provenance, and the reason this entry matters more than the bug.** Found by + `gpt-6-astra` through the Surplus proxy — an untrusted external reviewer given + the packed source and no tools — then confirmed locally by rendering the cases + before believing it. Our own tests were green throughout and stayed green, + because they were ASCII-only; the code asserted a guarantee in a comment that + it did not keep. An outside reader with no ability to run anything caught what + the test suite structurally could not. + - Signal-to-noise from that review, recorded honestly: three findings, one + confirmed (this), one unconfirmed and overlapping with it (a claim that + `MaxWidth` may wrap rather than clip — not observed), and one false positive + (elapsed-time nondeterminism in snapshots, which set `startTime` explicitly). + ## 2026-09-08 — Issue #1428 CLAUDE.md: tickets, real-run proof, delegation - Three practices this session kept proving necessary were absent from