From 4028c00475fa91073db93320d1638d838d93774f Mon Sep 17 00:00:00 2001 From: Daniel Pupius Date: Tue, 12 May 2026 14:34:59 -0700 Subject: [PATCH 1/4] fix(tui): keep footer pinned when child resets scroll region MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Children like Claude Code (via Ink) defensively emit ESC[r at startup to reset DECSTBM. That undid moat's scroll region, so every newline at the bottom row scrolled the footer into scrollback and the 50ms debounced redraw stamped a fresh copy at the bottom — producing a trail of footers in the user's scrollback and intermittent overlap with the child's input area. Make moat the sole authority over the scroll region: - In scroll mode, intercept DECSTBM (ESC[Pt;Pb r) from the child; swallow it and re-emit ESC[1;height-1 r in its place. - DECSTR (ESC[!p) passes through with our DECSTBM re-asserted after, wrapped in DECSC/DECRC so the cursor stays put. - RIS (ESC c) passes through with scroll region + footer re-established and cursor returned to home. - DEC private-mode restores (ESC[?Pn r) and other look-alikes are explicitly not matched. - In compositor mode, DECSTBM is passed to the emulator unchanged. Belt-and-suspenders: report height-1 to the container in interactive mode so the child's terminal model can't reach the footer row even if some new sequence escapes interception. --- cmd/moat/cli/exec.go | 23 ++- internal/run/manager.go | 11 +- internal/tui/writer.go | 176 +++++++++++++++++++ internal/tui/writer_test.go | 335 ++++++++++++++++++++++++++++++++++++ 4 files changed, 540 insertions(+), 5 deletions(-) diff --git a/cmd/moat/cli/exec.go b/cmd/moat/cli/exec.go index 2f79b5ef..1bb220cb 100644 --- a/cmd/moat/cli/exec.go +++ b/cmd/moat/cli/exec.go @@ -86,6 +86,18 @@ func executeRunWrapper(ctx context.Context, opts intcli.ExecOptions) (*intcli.Ex }, nil } +// containerTTYHeight returns the height to report to the container, reserving +// the bottom row for the status bar when one is active. Keeping the child's +// view of the terminal one row shorter prevents it from drawing on (or +// scrolling content over) the footer line. Paired with DECSTBM ownership in +// tui.Writer to fully isolate the child from moat's chrome. +func containerTTYHeight(statusWriter *tui.Writer, actual int) int { + if statusWriter != nil && actual > 1 { + return actual - 1 + } + return actual +} + // setupStatusBar creates a status bar for interactive container sessions. // Returns the writer (which wraps stdout with status bar compositing), a cleanup // function that must be deferred, and the output writer to use for container output. @@ -497,8 +509,9 @@ func RunInteractiveAttached(ctx context.Context, manager *run.Manager, r *run.Ru if term.IsTerminal(os.Stdout) { width, height := term.GetSize(os.Stdout) if width > 0 && height > 0 { + h := containerTTYHeight(statusWriter, height) // #nosec G115 -- width/height are validated positive above - if err := manager.ResizeTTY(ctx, r.ID, uint(height), uint(width)); err != nil { + if err := manager.ResizeTTY(ctx, r.ID, uint(h), uint(width)); err != nil { log.Debug("failed to resize TTY", "error", err) } } @@ -519,9 +532,10 @@ func RunInteractiveAttached(ctx context.Context, manager *run.Manager, r *run.Ru } ringRecorder.AddResize(width, height) _ = statusWriter.Resize(width, height) - // Also resize container TTY + // Also resize container TTY, reserving the footer row. + h := containerTTYHeight(statusWriter, height) // #nosec G115 -- width/height are validated positive above - _ = manager.ResizeTTY(ctx, r.ID, uint(height), uint(width)) + _ = manager.ResizeTTY(ctx, r.ID, uint(h), uint(width)) } } continue // Don't break out of loop @@ -659,8 +673,9 @@ func resetTUI(ctx context.Context, manager *run.Manager, r *run.Run, statusWrite if term.IsTerminal(os.Stdout) { if width, height := term.GetSize(os.Stdout); width > 0 && height > 0 { + h := containerTTYHeight(statusWriter, height) // #nosec G115 -- width/height validated positive - if err := manager.ResizeTTY(ctx, r.ID, uint(height), uint(width)); err != nil { + if err := manager.ResizeTTY(ctx, r.ID, uint(h), uint(width)); err != nil { log.Debug("post-reset resize nudge failed", "error", err) } } diff --git a/internal/run/manager.go b/internal/run/manager.go index 8c7cc05d..ca45e330 100644 --- a/internal/run/manager.go +++ b/internal/run/manager.go @@ -3120,11 +3120,20 @@ func (m *Manager) StartAttached(ctx context.Context, runID string, stdin io.Read // Pass initial terminal size so the container can be resized immediately // after starting, before the process queries terminal dimensions. + // + // In interactive mode the CLI reserves the bottom row for a status bar + // (see internal/tui.Writer). Subtract 1 from the reported height so the + // child renders in rows 1..height-1 and can't collide with the footer + // slot. Subsequent ResizeTTY calls from the CLI use the same adjustment. if useTTY && term.IsTerminal(os.Stdout) { width, height := term.GetSize(os.Stdout) if width > 0 && height > 0 { - // #nosec G115 -- width/height are validated positive above + if r.Interactive && height > 1 { + height-- + } + // #nosec G115 -- width is validated positive above attachOpts.InitialWidth = uint(width) + // #nosec G115 -- height is validated positive above (and only decremented when > 1) attachOpts.InitialHeight = uint(height) } } diff --git a/internal/tui/writer.go b/internal/tui/writer.go index 1dab9b96..50ba8c01 100644 --- a/internal/tui/writer.go +++ b/internal/tui/writer.go @@ -248,6 +248,25 @@ func (w *Writer) processDataLocked(data []byte) error { return nil } + // In scroll mode, intercept terminal-state escapes that would + // clobber moat's scroll region (DECSTBM, DECSTR, RIS). The + // emulator owns its own scroll region in compositor mode, so we + // don't intercept there. + if !w.altScreen { + res := matchControlSeq(data) + if res.needsMore && len(data) <= maxControlSeqBufLen { + w.escBuf = append(w.escBuf[:0], data...) + return nil + } + if res.kind != ctrlNone { + if err := w.handleControlSeqLocked(res, data[:res.length]); err != nil { + return err + } + data = data[res.length:] + continue + } + } + // Not an alt screen sequence - output the ESC and continue if err := w.outputLocked(data[:1]); err != nil { return err @@ -257,6 +276,163 @@ func (w *Writer) processDataLocked(data []byte) error { return nil } +// maxControlSeqBufLen bounds how much of a partial DECSTBM/DECSTR sequence +// we'll buffer before giving up and passing the bytes through. Realistic +// DECSTBMs are 3–8 bytes; the generous cap leaves room for the pathological +// case of many-paramed sequences split across Write boundaries while +// preventing a malformed never-terminating sequence from pinning memory. +const maxControlSeqBufLen = 256 + +// Assumes 7-bit ANSI input (ESC [, not the 8-bit C1 byte 0x9B). All real +// children writing through this Writer emit UTF-8, where 0x9B can only +// appear as a continuation byte. If we ever support a non-UTF-8 child +// encoding, matchControlSeq will need to grow a C1 branch. + +// controlSeqKind tags terminal-state sequences that affect the scroll region. +// In scroll mode, moat owns the scroll region; the child can't be allowed to +// change it directly. +type controlSeqKind int + +const ( + ctrlNone controlSeqKind = iota + ctrlDECSTBM // CSI Pt;Pb r — set scroll region. Swallow and re-emit moat's region. + ctrlDECSTR // CSI ! p — soft terminal reset; clears DECSTBM as a side effect. Pass through, then re-emit. + ctrlRIS // ESC c — hard reset; clears screen and DECSTBM. Pass through, then re-establish layout. +) + +// controlSeqResult is the outcome of matchControlSeq. +type controlSeqResult struct { + kind controlSeqKind + length int // bytes consumed; 0 when no match or partial + needsMore bool // true if data is a viable prefix of DECSTBM/DECSTR and more data may complete it +} + +// matchControlSeq checks whether data starts with a DECSTBM, DECSTR, or RIS +// sequence. Returns needsMore=true if the buffer holds a prefix that could +// still resolve into one of these; caller should buffer and retry. +// +// CSI sequences that share the same final byte but have different syntax +// (e.g. CSI ? 2026 r, a DEC private mode restore) are explicitly NOT matched +// — they pass through to the terminal unmodified. +func matchControlSeq(data []byte) controlSeqResult { + if len(data) == 0 || data[0] != 0x1b { + return controlSeqResult{} + } + if len(data) == 1 { + // Bare ESC at end of buffer — anything could follow. + return controlSeqResult{needsMore: true} + } + // RIS: ESC c + if data[1] == 'c' { + return controlSeqResult{kind: ctrlRIS, length: 2} + } + // Everything else we care about starts with CSI (ESC [). + if data[1] != '[' { + return controlSeqResult{} + } + + i := 2 + paramStart := i + onlyDigitsAndSemi := true + for i < len(data) && data[i] >= 0x30 && data[i] <= 0x3F { + b := data[i] + if !((b >= '0' && b <= '9') || b == ';') { + onlyDigitsAndSemi = false + } + i++ + } + paramLen := i - paramStart + + intStart := i + var firstIntermediate byte + for i < len(data) && data[i] >= 0x20 && data[i] <= 0x2F { + if i == intStart { + firstIntermediate = data[i] + } + i++ + } + intLen := i - intStart + + if i >= len(data) { + // Incomplete CSI. Could it still be DECSTBM or DECSTR? + if intLen == 0 && onlyDigitsAndSemi { + return controlSeqResult{needsMore: true} // could be DECSTBM + } + if paramLen == 0 && intLen == 1 && firstIntermediate == '!' { + return controlSeqResult{needsMore: true} // could be DECSTR + } + return controlSeqResult{} + } + + final := data[i] + if final < 0x40 || final > 0x7E { + // Not a valid CSI final byte — let the original parser handle it. + return controlSeqResult{} + } + length := i + 1 + + // DECSTBM: digit/semi params (or none), no intermediates, final 'r'. + if final == 'r' && intLen == 0 && onlyDigitsAndSemi { + return controlSeqResult{kind: ctrlDECSTBM, length: length} + } + // DECSTR: no params, single '!' intermediate, final 'p'. + if final == 'p' && paramLen == 0 && intLen == 1 && firstIntermediate == '!' { + return controlSeqResult{kind: ctrlDECSTR, length: length} + } + return controlSeqResult{} +} + +// handleControlSeqLocked applies the policy for a matched DECSTBM/DECSTR/RIS: +// - DECSTBM (any args from the child) is swallowed; moat's own scroll +// region command is emitted in its place. +// - DECSTR passes through (other resets may be intended), and moat's +// DECSTBM is re-asserted right after so the footer slot stays reserved. +// The pair is wrapped in DECSC/DECRC so the cursor — which DECSTR +// preserves but DECSTBM moves — is restored. +// - RIS passes through (it clears the screen and homes the cursor), then +// moat re-establishes its scroll region and footer and returns the +// cursor to home so the child can resume drawing. +// +// Caller passes raw bytes of the matched sequence so RIS/DECSTR can be +// forwarded verbatim. +func (w *Writer) handleControlSeqLocked(res controlSeqResult, raw []byte) error { + switch res.kind { + case ctrlDECSTBM: + return w.outputLocked(w.scrollRegionBytes()) + case ctrlDECSTR: + // Deviation: DECSTR resets the DECSC slot to home (1,1). Our + // DECSC here overwrites that with the live cursor instead. No + // known TUI relies on DECSTR's saved-cursor reset; we accept the + // trade to keep the visible cursor stable across the re-emit. + var buf bytes.Buffer + buf.Write(raw) + buf.WriteString("\x1b7") // save cursor (DECSTR preserves it) + buf.Write(w.scrollRegionBytes()) + buf.WriteString("\x1b8") // restore cursor (DECSTBM moves it) + return w.outputLocked(buf.Bytes()) + case ctrlRIS: + var buf bytes.Buffer + buf.Write(raw) + buf.Write(w.scrollRegionBytes()) + // RIS cleared the screen; redraw the footer. + fmt.Fprintf(&buf, "\x1b[%d;1H\x1b[2K", w.height) + buf.WriteString(w.bar.Render()) + // Return cursor to home so child resumes drawing where RIS left it. + buf.WriteString("\x1b[H") + return w.outputLocked(buf.Bytes()) + } + return nil +} + +// scrollRegionBytes returns the DECSTBM command that pins moat's footer at +// the bottom row. Empty when the terminal is too short to have a region. +func (w *Writer) scrollRegionBytes() []byte { + if w.height <= 1 { + return nil + } + return []byte(fmt.Sprintf("\x1b[1;%dr", w.height-1)) +} + // outputLocked sends data to either the real terminal (scroll mode) or the // VT emulator (compositor mode). func (w *Writer) outputLocked(data []byte) error { diff --git a/internal/tui/writer_test.go b/internal/tui/writer_test.go index e8df0a12..4f603a70 100644 --- a/internal/tui/writer_test.go +++ b/internal/tui/writer_test.go @@ -2,6 +2,7 @@ package tui import ( "bytes" + "fmt" "strings" "testing" ) @@ -919,3 +920,337 @@ func TestWriter_Reset_ExitsAltScreen(t *testing.T) { t.Errorf("emulator should be nil after Reset") } } + +// --- DECSTBM/DECSTR/RIS interception in scroll mode --- +// +// In scroll mode, the child must not be allowed to clobber moat's scroll +// region. Bare DECSTBM resets (`ESC[r`) and arbitrary scroll-region changes +// (`ESC[Pt;Pb r`) are swallowed; moat's `ESC[1;height-1 r` is re-emitted in +// their place. DECSTR (`ESC[!p`) and RIS (`ESC c`) pass through but are +// followed by re-asserted DECSTBM so the footer slot stays reserved. + +func TestWriter_InterceptsDECSTBM_NoArgs(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // Child sends a bare DECSTBM reset. + if _, err := w.Write([]byte("\x1b[r")); err != nil { + t.Fatalf("Write: %v", err) + } + + out := buf.String() + if strings.Contains(out, "\x1b[r") { + t.Errorf("expected bare ESC[r to be swallowed, got %q", out) + } + if !strings.Contains(out, "\x1b[1;23r") { + t.Errorf("expected moat's DECSTBM (ESC[1;23r) to be re-emitted, got %q", out) + } + w.Cleanup() +} + +func TestWriter_InterceptsDECSTBM_WithArgs(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // Child tries to set its own scroll region — must be overridden. + if _, err := w.Write([]byte("\x1b[5;15r")); err != nil { + t.Fatalf("Write: %v", err) + } + + out := buf.String() + if strings.Contains(out, "\x1b[5;15r") { + t.Errorf("expected child's DECSTBM (ESC[5;15r) to be swallowed, got %q", out) + } + if !strings.Contains(out, "\x1b[1;23r") { + t.Errorf("expected moat's DECSTBM to be re-emitted, got %q", out) + } + w.Cleanup() +} + +func TestWriter_InterceptsDECSTBM_SurroundedByText(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // DECSTBM embedded between content — text passes through, DECSTBM is replaced. + if _, err := w.Write([]byte("before\x1b[rafter")); err != nil { + t.Fatalf("Write: %v", err) + } + + out := buf.String() + if !strings.Contains(out, "before") || !strings.Contains(out, "after") { + t.Errorf("expected surrounding text to pass through, got %q", out) + } + if strings.Contains(out, "\x1b[r") { + t.Errorf("expected ESC[r to be swallowed, got %q", out) + } + if !strings.Contains(out, "\x1b[1;23r") { + t.Errorf("expected moat's DECSTBM to be re-emitted, got %q", out) + } + w.Cleanup() +} + +func TestWriter_InterceptsDECSTBM_SplitAcrossWrites(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // Split DECSTBM across two writes. The first write must not pass the + // partial bytes through to the terminal — otherwise the terminal would + // see ESC[1; and then ;23r and execute the original DECSTBM. + if _, err := w.Write([]byte("\x1b[1;")); err != nil { + t.Fatalf("Write 1: %v", err) + } + if strings.Contains(buf.String(), "\x1b[1;") { + t.Errorf("partial DECSTBM (ESC[1;) leaked through before completion: %q", buf.String()) + } + if _, err := w.Write([]byte("60r")); err != nil { + t.Fatalf("Write 2: %v", err) + } + + out := buf.String() + if strings.Contains(out, "\x1b[1;60r") { + t.Errorf("expected child DECSTBM ESC[1;60r to be swallowed, got %q", out) + } + if !strings.Contains(out, "\x1b[1;23r") { + t.Errorf("expected moat's DECSTBM to be re-emitted, got %q", out) + } + w.Cleanup() +} + +func TestWriter_DECSTR_PassesThroughWithDECSTBMRestored(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // DECSTR (soft terminal reset) clears DECSTBM as a side effect. We + // pass it through (child may need its other effects) and re-emit our + // DECSTBM right after. + if _, err := w.Write([]byte("\x1b[!p")); err != nil { + t.Fatalf("Write: %v", err) + } + + out := buf.String() + softIdx := strings.Index(out, "\x1b[!p") + stbmIdx := strings.Index(out, "\x1b[1;23r") + if softIdx < 0 { + t.Errorf("expected DECSTR (ESC[!p) to pass through, got %q", out) + } + if stbmIdx < 0 { + t.Errorf("expected DECSTBM re-assertion after DECSTR, got %q", out) + } + if softIdx >= 0 && stbmIdx >= 0 && softIdx >= stbmIdx { + t.Errorf("DECSTR must precede DECSTBM re-emit, got positions %d and %d", softIdx, stbmIdx) + } + w.Cleanup() +} + +func TestWriter_RIS_PassesThroughWithDECSTBMRestored(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // RIS (full reset, ESC c) wipes the screen and DECSTBM. Pass it + // through, then re-establish our region and footer. + if _, err := w.Write([]byte("\x1bc")); err != nil { + t.Fatalf("Write: %v", err) + } + + out := buf.String() + risIdx := strings.Index(out, "\x1bc") + stbmIdx := strings.Index(out, "\x1b[1;23r") + if risIdx < 0 { + t.Errorf("expected RIS (ESC c) to pass through, got %q", out) + } + if stbmIdx < 0 { + t.Errorf("expected DECSTBM re-assertion after RIS, got %q", out) + } + if risIdx >= 0 && stbmIdx >= 0 && risIdx >= stbmIdx { + t.Errorf("RIS must precede DECSTBM re-emit, got positions %d and %d", risIdx, stbmIdx) + } + w.Cleanup() +} + +func TestWriter_DECPrivateModeRestore_NotIntercepted(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // ESC[?2026r is a DEC private mode restore — different semantics from + // DECSTBM despite the same final byte. It must pass through unchanged. + if _, err := w.Write([]byte("\x1b[?2026r")); err != nil { + t.Fatalf("Write: %v", err) + } + + out := buf.String() + if !strings.Contains(out, "\x1b[?2026r") { + t.Errorf("expected DEC private mode restore to pass through unchanged, got %q", out) + } + // And moat's DECSTBM should NOT be erroneously emitted in response. + if strings.Count(out, "\x1b[1;23r") > 0 { + t.Errorf("DEC private mode restore should not trigger DECSTBM re-emit, got %q", out) + } + w.Cleanup() +} + +func TestWriter_InterceptsDECSTBM_MultipleInOneWrite(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // Two DECSTBMs back-to-back in a single Write — both must be swallowed, + // and moat's region must appear twice. + if _, err := w.Write([]byte("\x1b[r\x1b[5;15r")); err != nil { + t.Fatalf("Write: %v", err) + } + + out := buf.String() + if strings.Contains(out, "\x1b[r") || strings.Contains(out, "\x1b[5;15r") { + t.Errorf("expected both child DECSTBMs to be swallowed, got %q", out) + } + if got := strings.Count(out, "\x1b[1;23r"); got != 2 { + t.Errorf("expected moat's DECSTBM emitted twice (once per intercept), got %d in %q", got, out) + } + w.Cleanup() +} + +func TestWriter_InterceptsDECSTBM_LongPartialBuffersOK(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // Build a long-but-valid DECSTBM prefix: ESC[ then a long chain of + // "N;" params. Stays under maxControlSeqBufLen so it can be buffered. + var first bytes.Buffer + first.WriteString("\x1b[") + for i := 0; i < 30; i++ { + fmt.Fprintf(&first, "%d;", i) + } + if _, err := w.Write(first.Bytes()); err != nil { + t.Fatalf("Write 1: %v", err) + } + if buf.Len() != 0 { + t.Errorf("expected partial DECSTBM (%d bytes) to be buffered, but %d bytes leaked: %q", + first.Len(), buf.Len(), buf.String()) + } + + // Complete the sequence. + if _, err := w.Write([]byte("99r")); err != nil { + t.Fatalf("Write 2: %v", err) + } + + out := buf.String() + if strings.Contains(out, "\x1b[0;1;2;") { + t.Errorf("child's long DECSTBM should not have leaked, got %q", out) + } + if !strings.Contains(out, "\x1b[1;23r") { + t.Errorf("expected moat's DECSTBM to be re-emitted, got %q", out) + } + w.Cleanup() +} + +// TestWriter_InkStartupSequence pins the regression that motivated this work: +// Ink (used by Claude Code) emits \x1b7\x1b[r\x1b8\x1b[?25h shortly after +// startup to defensively reset the scroll region. Before this fix, that +// \x1b[r undid moat's DECSTBM and any subsequent newline at the bottom row +// scrolled the footer into scrollback. With interception, moat re-emits its +// own region in place of the child's reset. +func TestWriter_InkStartupSequence_PreservesScrollRegion(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + // The exact sequence captured in tui-debug traces from a glitchy run. + if _, err := w.Write([]byte("\x1b7\x1b[r\x1b8\x1b[?25h")); err != nil { + t.Fatalf("Write: %v", err) + } + + out := buf.String() + // DECSC/DECRC and show-cursor pass through. + if !strings.Contains(out, "\x1b7") { + t.Errorf("expected DECSC (ESC 7) to pass through, got %q", out) + } + if !strings.Contains(out, "\x1b8") { + t.Errorf("expected DECRC (ESC 8) to pass through, got %q", out) + } + if !strings.Contains(out, "\x1b[?25h") { + t.Errorf("expected show-cursor to pass through, got %q", out) + } + // The bare DECSTBM reset must be swallowed and replaced. + if strings.Contains(out, "\x1b[r") { + t.Errorf("expected bare ESC[r to be swallowed, got %q", out) + } + if !strings.Contains(out, "\x1b[1;23r") { + t.Errorf("expected moat's DECSTBM to be re-emitted in place, got %q", out) + } + w.Cleanup() +} + +func TestWriter_DECSTBM_CompositorMode_PassesToEmulator(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + + // Enter compositor mode. + if _, err := w.Write([]byte("\x1b[?1049h")); err != nil { + t.Fatalf("Write alt-screen enter: %v", err) + } + buf.Reset() + + // In compositor mode, DECSTBM from the child goes to the emulator — + // it must NOT be replayed on the host terminal. + if _, err := w.Write([]byte("\x1b[r")); err != nil { + t.Fatalf("Write: %v", err) + } + + out := buf.String() + if strings.Contains(out, "\x1b[1;23r") { + t.Errorf("in compositor mode, moat must not emit host DECSTBM, got %q", out) + } + w.Cleanup() +} From 4b2800dc771b53e3b0e8bdf5b240d5ac151c30d4 Mon Sep 17 00:00:00 2001 From: Daniel Pupius Date: Tue, 12 May 2026 14:52:48 -0700 Subject: [PATCH 2/4] fix(tui): address review-bot findings on footer scroll-region fix - Document the oversized-partial-sequence fallthrough in processDataLocked (real DECSTBMs are <10 bytes; bound-exceeded case passes through). - Explain the alt-screen / DECSTBM priority ordering in processDataLocked. - Note in handleControlSeqLocked why DECSTR relies on the debounced footer redraw while RIS redraws inline (RIS clears the screen, DECSTR doesn't). - Mark the trailing return in handleControlSeqLocked unreachable. - Cross-reference manager.go's r.Interactive predicate with exec.go's statusWriter check. - TestWriter_InkStartupSequence_PreservesScrollRegion now verifies DECSC -> DECSTBM -> DECRC order, not just presence. - Add TestWriter_AltScreenExit_ThenDECSTBMInSameWrite covering the mode-flip path with a trailing intercept in one Write. --- internal/run/manager.go | 8 ++++++ internal/tui/writer.go | 26 +++++++++++++++++-- internal/tui/writer_test.go | 52 +++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 2 deletions(-) diff --git a/internal/run/manager.go b/internal/run/manager.go index ca45e330..a5425e67 100644 --- a/internal/run/manager.go +++ b/internal/run/manager.go @@ -3125,6 +3125,14 @@ func (m *Manager) StartAttached(ctx context.Context, runID string, stdin io.Read // (see internal/tui.Writer). Subtract 1 from the reported height so the // child renders in rows 1..height-1 and can't collide with the footer // slot. Subsequent ResizeTTY calls from the CLI use the same adjustment. + // + // Predicate note: this site checks r.Interactive while the CLI's + // containerTTYHeight helper checks statusWriter != nil. They're + // equivalent today because both are gated by term.IsTerminal(os.Stdout) + // and exec.go only constructs a statusWriter when r.Interactive is true. + // If a future caller invokes StartAttached for an Interactive run in a + // non-TTY context, this branch is unreached (the outer term.IsTerminal + // guard fails first), so the predicates stay consistent. if useTTY && term.IsTerminal(os.Stdout) { width, height := term.GetSize(os.Stdout) if width > 0 && height > 0 { diff --git a/internal/tui/writer.go b/internal/tui/writer.go index 50ba8c01..70402b0c 100644 --- a/internal/tui/writer.go +++ b/internal/tui/writer.go @@ -241,7 +241,13 @@ func (w *Writer) processDataLocked(data []byte) error { continue } - // Check if this could be a partial match at the end of the buffer + // Check if this could be a partial match at the end of the buffer. + // This runs before the DECSTBM/DECSTR matcher below; both paths + // share w.escBuf, but the priority ordering is safe: a 2-byte + // ESC[ prefix matches alt-screen first and gets buffered, then on + // the next Write the combined buffer is re-classified by both + // matchers in turn — if it turns out to be DECSTBM, the second + // pass routes it correctly. if w.isPrefixOfAltScreen(data) && len(data) < maxAltScreenSeqLen() { // Buffer it for the next Write call w.escBuf = append(w.escBuf[:0], data...) @@ -265,6 +271,12 @@ func (w *Writer) processDataLocked(data []byte) error { data = data[res.length:] continue } + // If needsMore but data exceeded maxControlSeqBufLen, we fall + // through and emit the ESC byte. The remaining bytes pass to + // the terminal in order, which reassembles the original + // CSI — interception silently fails, but real DECSTBMs are + // well under 10 bytes, so this only fires for pathological + // input. Memory bound > correctness coverage here. } // Not an alt screen sequence - output the ESC and continue @@ -404,6 +416,13 @@ func (w *Writer) handleControlSeqLocked(res controlSeqResult, raw []byte) error // DECSC here overwrites that with the live cursor instead. No // known TUI relies on DECSTR's saved-cursor reset; we accept the // trade to keep the visible cursor stable across the re-emit. + // + // We intentionally do NOT redraw the footer here. DECSTR + // preserves on-screen content (only modes/state are reset), so + // the existing footer pixels remain. The debounced redraw fires + // shortly after this Write returns and repairs the row in case + // the child writes content immediately afterward. Inline redraw + // would risk clobbering the child's mid-frame rendering. var buf bytes.Buffer buf.Write(raw) buf.WriteString("\x1b7") // save cursor (DECSTR preserves it) @@ -411,16 +430,19 @@ func (w *Writer) handleControlSeqLocked(res controlSeqResult, raw []byte) error buf.WriteString("\x1b8") // restore cursor (DECSTBM moves it) return w.outputLocked(buf.Bytes()) case ctrlRIS: + // Unlike DECSTR, RIS clears the screen — the footer pixels are + // gone. Redraw it inline rather than waiting for the debounce so + // the row isn't visibly blank in the gap. var buf bytes.Buffer buf.Write(raw) buf.Write(w.scrollRegionBytes()) - // RIS cleared the screen; redraw the footer. fmt.Fprintf(&buf, "\x1b[%d;1H\x1b[2K", w.height) buf.WriteString(w.bar.Render()) // Return cursor to home so child resumes drawing where RIS left it. buf.WriteString("\x1b[H") return w.outputLocked(buf.Bytes()) } + // Unreachable: callers gate this on res.kind != ctrlNone. return nil } diff --git a/internal/tui/writer_test.go b/internal/tui/writer_test.go index 4f603a70..6da5f593 100644 --- a/internal/tui/writer_test.go +++ b/internal/tui/writer_test.go @@ -1225,6 +1225,58 @@ func TestWriter_InkStartupSequence_PreservesScrollRegion(t *testing.T) { if !strings.Contains(out, "\x1b[1;23r") { t.Errorf("expected moat's DECSTBM to be re-emitted in place, got %q", out) } + // Ordering: the child's DECSC/DECRC must still bracket moat's + // DECSTBM, so the cursor preservation the child intended still works. + decsc := strings.Index(out, "\x1b7") + stbm := strings.Index(out, "\x1b[1;23r") + decrc := strings.Index(out, "\x1b8") + if !(decsc >= 0 && stbm >= 0 && decrc >= 0 && decsc < stbm && stbm < decrc) { + t.Errorf("expected order DECSC -> DECSTBM -> DECRC, got positions %d, %d, %d in %q", + decsc, stbm, decrc, out) + } + w.Cleanup() +} + +// Exiting compositor mode followed immediately by the child's defensive +// ESC[r in the same Write exercises the mode-flip path: alt-screen exit +// triggers exitCompositorLocked (which re-emits scroll region + footer), +// then the DECSTBM matcher needs to fire on the trailing bytes now that +// w.altScreen is false. Both must be handled in one Write. +func TestWriter_AltScreenExit_ThenDECSTBMInSameWrite(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + + // Enter compositor mode. + if _, err := w.Write([]byte("\x1b[?1049h")); err != nil { + t.Fatalf("Write enter: %v", err) + } + buf.Reset() + + // Exit + Ink-style DECSTBM reset in one Write. + if _, err := w.Write([]byte("\x1b[?1049l\x1b[r")); err != nil { + t.Fatalf("Write exit+reset: %v", err) + } + + w.mu.Lock() + inAlt := w.altScreen + w.mu.Unlock() + if inAlt { + t.Error("expected altScreen=false after exit") + } + + out := buf.String() + if strings.Contains(out, "\x1b[r") { + t.Errorf("child's DECSTBM reset must be swallowed in scroll mode, got %q", out) + } + // exitCompositorLocked emits one DECSTBM and the post-exit intercept + // emits another. We don't pin the count; just require at least one. + if !strings.Contains(out, "\x1b[1;23r") { + t.Errorf("expected moat's DECSTBM after the mode flip, got %q", out) + } w.Cleanup() } From 24c8358fd66635409a6aaf01dab48ab8aa001849 Mon Sep 17 00:00:00 2001 From: Daniel Pupius Date: Tue, 12 May 2026 14:59:34 -0700 Subject: [PATCH 3/4] test(tui): tighten DECSTR ordering and cover bare-ESC split - TestWriter_DECSTR_PassesThroughWithDECSTBMRestored now asserts the full DECSTR -> DECSC -> DECSTBM -> DECRC ordering so a regression that dropped the cursor-preservation pair would be caught. - New TestWriter_InterceptsDECSTBM_SplitAfterBareESC exercises the len(data)==1 needsMore branch in matchControlSeq: a lone ESC byte arriving in one write must be buffered, then assembled with the following [24r to be intercepted. --- internal/tui/writer_test.go | 45 +++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/internal/tui/writer_test.go b/internal/tui/writer_test.go index 6da5f593..7fbba73a 100644 --- a/internal/tui/writer_test.go +++ b/internal/tui/writer_test.go @@ -1055,14 +1055,55 @@ func TestWriter_DECSTR_PassesThroughWithDECSTBMRestored(t *testing.T) { out := buf.String() softIdx := strings.Index(out, "\x1b[!p") stbmIdx := strings.Index(out, "\x1b[1;23r") + decsc := strings.Index(out, "\x1b7") + decrc := strings.Index(out, "\x1b8") if softIdx < 0 { t.Errorf("expected DECSTR (ESC[!p) to pass through, got %q", out) } if stbmIdx < 0 { t.Errorf("expected DECSTBM re-assertion after DECSTR, got %q", out) } - if softIdx >= 0 && stbmIdx >= 0 && softIdx >= stbmIdx { - t.Errorf("DECSTR must precede DECSTBM re-emit, got positions %d and %d", softIdx, stbmIdx) + // The handler wraps DECSTBM in DECSC/DECRC so the cursor (preserved + // by DECSTR, moved by DECSTBM) ends up where the child expects. + // Verify the full ordering: DECSTR -> DECSC -> DECSTBM -> DECRC. + if !(softIdx >= 0 && decsc >= 0 && stbmIdx >= 0 && decrc >= 0 && + softIdx < decsc && decsc < stbmIdx && stbmIdx < decrc) { + t.Errorf("expected order DECSTR -> DECSC -> DECSTBM -> DECRC, got positions %d, %d, %d, %d in %q", + softIdx, decsc, stbmIdx, decrc, out) + } + w.Cleanup() +} + +// Splitting after the bare ESC byte exercises the len(data)==1 needsMore +// branch in matchControlSeq — Write 1 is just \x1b, Write 2 completes the +// DECSTBM. The bare ESC must be buffered (not leaked to the terminal), +// and on Write 2 the combined sequence is intercepted. +func TestWriter_InterceptsDECSTBM_SplitAfterBareESC(t *testing.T) { + var buf bytes.Buffer + bar := NewStatusBar("run_abc123", "my-agent", "docker") + bar.SetDimensions(60, 24) + + w := NewWriter(&buf, bar, "docker") + _ = w.Setup() + buf.Reset() + + if _, err := w.Write([]byte("\x1b")); err != nil { + t.Fatalf("Write 1: %v", err) + } + if buf.Len() != 0 { + t.Errorf("bare ESC must be buffered, but %d bytes leaked: %q", buf.Len(), buf.String()) + } + + if _, err := w.Write([]byte("[24r")); err != nil { + t.Fatalf("Write 2: %v", err) + } + + out := buf.String() + if strings.Contains(out, "\x1b[24r") { + t.Errorf("child's DECSTBM must be swallowed across the split, got %q", out) + } + if !strings.Contains(out, "\x1b[1;23r") { + t.Errorf("expected moat's DECSTBM after the split, got %q", out) } w.Cleanup() } From a55dfbdc69676973eb08be87d4ce8139f3076871 Mon Sep 17 00:00:00 2001 From: Daniel Pupius Date: Tue, 12 May 2026 15:06:02 -0700 Subject: [PATCH 4/4] test(tui): pin DECSTBM-replacement count in NoArgs/SurroundedByText Switch from "doesn't contain ESC[r" + "contains ESC[1;23r" to an exact Count(...) == 1 check on the replacement. The previous form would pass if interception double-emitted or skipped the replacement; the count makes the contract explicit. Matches the pattern already used in TestWriter_InterceptsDECSTBM_MultipleInOneWrite. --- internal/tui/writer_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/tui/writer_test.go b/internal/tui/writer_test.go index 7fbba73a..7d3e2326 100644 --- a/internal/tui/writer_test.go +++ b/internal/tui/writer_test.go @@ -947,8 +947,10 @@ func TestWriter_InterceptsDECSTBM_NoArgs(t *testing.T) { if strings.Contains(out, "\x1b[r") { t.Errorf("expected bare ESC[r to be swallowed, got %q", out) } - if !strings.Contains(out, "\x1b[1;23r") { - t.Errorf("expected moat's DECSTBM (ESC[1;23r) to be re-emitted, got %q", out) + // Exactly one re-emit — a regression that double-emitted or skipped + // the replacement would slip past a Contains check. + if got := strings.Count(out, "\x1b[1;23r"); got != 1 { + t.Errorf("expected moat's DECSTBM emitted exactly once, got %d in %q", got, out) } w.Cleanup() } @@ -998,8 +1000,8 @@ func TestWriter_InterceptsDECSTBM_SurroundedByText(t *testing.T) { if strings.Contains(out, "\x1b[r") { t.Errorf("expected ESC[r to be swallowed, got %q", out) } - if !strings.Contains(out, "\x1b[1;23r") { - t.Errorf("expected moat's DECSTBM to be re-emitted, got %q", out) + if got := strings.Count(out, "\x1b[1;23r"); got != 1 { + t.Errorf("expected moat's DECSTBM emitted exactly once, got %d in %q", got, out) } w.Cleanup() }