fix(tui): keep footer pinned when child resets scroll region

[dpup]

Summary

When Claude Code (and other Ink-based TUIs) start up, they defensively emit ESC[r to reset the host terminal's DECSTBM scroll region. This undid moat's region, so every newline at the bottom row scrolled the footer into scrollback. The 50ms debounced footer redraw then stamped a fresh copy at the bottom — leaving a trail of footers in the user's scrollback and intermittently overlapping the child's input area. The exact sequence was captured in TUI debug traces from a glitchy session.

This PR fixes the rendering by making moat the sole authority over the scroll region and reserving the footer row at the TTY level.

Scroll-mode interception (internal/tui/writer.go)

processDataLocked now scans for and replaces terminal-state escapes that would clobber moat's region:

• DECSTBM (ESC[r, ESC[Pt;Pb r) — swallowed; moat re-emits ESC[1;height-1 r.
• DECSTR (ESC[!p) — passes through, then DECSC + our DECSTBM + DECRC so the visible cursor stays put.
• RIS (ESC c) — passes through, then region re-set, footer redrawn, cursor returned to home.
• DEC private-mode restores (ESC[?Pn r) and other CSIs that share r/!p syntactically but not semantically are explicitly not matched.
• In compositor mode (alt-screen), DECSTBM goes to the VT emulator unchanged — the emulator owns its scroll region there.

Partial sequences split across Write() calls are buffered through the existing escBuf mechanism (bounded at 256 bytes — generous headroom over realistic DECSTBM sizes).

Footer-row reservation (internal/run/manager.go, cmd/moat/cli/exec.go)

When a status bar is active, report height-1 to the container's TTY:

• StartAttached subtracts 1 from attachOpts.InitialHeight for interactive runs.
• New containerTTYHeight(statusWriter, h) helper used by all three ResizeTTY call sites (startup goroutine, SIGWINCH, post-Ctrl+/ r reset nudge).

This is belt-and-suspenders with the interception: the child literally cannot address the footer row, so even sequences we don't recognize can't collide with the footer slot.

Test plan

• 11 new unit tests in internal/tui/writer_test.go covering: bare ESC[r, parameterized DECSTBM, embedded in surrounding text, split across writes, DECSTR with cursor preservation, RIS, multiple DECSTBMs in one write, long partials buffered correctly, the exact Ink startup sequence as a named regression test, DEC private-mode restore left alone, and compositor-mode passthrough.
• go test ./internal/tui/ -race -count=2 passes
• make test-unit passes
• golangci-lint run clean for files touched (pre-existing staticcheck warnings in internal/run/docker_test.go are unrelated)
• Manual: launch moat run claude --dangerously-skip-permissions and confirm the footer stays at the bottom row through extended streaming output (resize, scroll-up to inspect scrollback, etc.)

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 6m 32s —— View job

---

Code Review

• Gather context and read changed files
• Review internal/tui/writer.go — escape sequence interception logic
• Review internal/tui/writer_test.go — test coverage and correctness
• Review cmd/moat/cli/exec.go — TTY height adjustments
• Review internal/run/manager.go — containerTTYHeight helper

---

Overall

The approach is correct and well-reasoned. Owning the scroll region from moat's side (both intercepting escapes and shrinking the child's TTY view) is the right way to solve this permanently rather than patching up after the fact. Test coverage is strong for a terminal-escape-parsing change, especially the Ink regression test and the split-write case.

A few findings below, ordered by severity.

---

internal/tui/writer.go

Medium — silent passthrough on oversized partial sequence

// writer.go:257
if res.needsMore && len(data) <= maxControlSeqBufLen {
    w.escBuf = append(w.escBuf[:0], data...)
    return nil
}

When res.needsMore is true and len(data) > maxControlSeqBufLen, neither branch runs. Execution falls through to w.outputLocked(data[:1]), which emits the lone ESC byte. The remaining bytes ([1;...r) are then output as literal text in the next loop iterations — but a real terminal accumulates ESC + the subsequent bytes and executes the original DECSTBM. The interception silently fails.

This can't happen with realistic DECSTBM sequences (they're 3–8 bytes vs the 256-byte cap), but it's a correctness hole that isn't documented. The existing comment on maxControlSeqBufLen explains the bound but not what happens when it's exceeded. Consider adding a brief comment at the fallthrough point: // Sequence exceeds buffer cap — pass through; real DECSTBMs are < 10 bytes.

Fix this →

---

Low — dead default in handleControlSeqLocked

func (w *Writer) handleControlSeqLocked(res controlSeqResult, raw []byte) error {
    switch res.kind {
    case ctrlDECSTBM: ...
    case ctrlDECSTR:  ...
    case ctrlRIS:     ...
    }
    return nil  // ← only reachable for ctrlNone, which callers never pass
}

The function is only called when res.kind != ctrlNone. The trailing return nil is dead code. Not a bug, but could mislead a reader into thinking ctrlNone is a valid argument. A default: panic("unreachable") or a // unreachable comment would clarify intent.

---

Low — DECSTR handler doesn't immediately redraw the footer

For DECSTR, handleControlSeqLocked re-asserts the scroll region but doesn't redraw the footer row. The footer is protected by the debounced scheduleFooterRedrawLocked() in Write(), so it eventually reappears. But there's a visible window where the footer row is blank (DECSTR clears DECSTBM, which means the next newline from the child can scroll into that row before the debounced redraw fires).

For RIS this is handled correctly inline (footer redrawn immediately). It's worth a comment in the DECSTR branch explaining the deliberate choice to rely on debounced redraw, so the asymmetry doesn't look accidental.

---

Design note — alt-screen prefix check takes priority over DECSTBM buffering

The ordering in processDataLocked means isPrefixOfAltScreen runs before matchControlSeq. A bare \x1b[ (2 bytes) is a prefix of \x1b[?1049h, so it gets buffered by the alt-screen check. On the next Write, the combined buffer is re-evaluated and correctly routed to DECSTBM handling. This is correct but subtle — the two buffering paths share escBuf and rely on the priority ordering. A comment at line 245 noting this interaction would help future readers.

---

internal/tui/writer_test.go

Coverage is strong. A few observations:

Missing test: alt-screen exit immediately followed by DECSTBM

The sequence \x1b[?1049l\x1b[r (exit compositor, then Ink's DECSTBM reset) isn't tested. This exercises the mode-switch path mid-Write where w.altScreen flips from true to false and the DECSTBM in the same buffer is then in scroll mode. Given this PR's stated motivation (Ink startup sequence), this combination seems plausible.

TestWriter_InkStartupSequence_PreservesScrollRegion — DECSC/DECRC are not verified in order

The test checks that \x1b7 and \x1b8 appear in the output, but not that they appear before and after moat's DECSTBM respectively. The Ink sequence \x1b7\x1b[r\x1b8 means the original DECSC/DECRC bracket the reset. After interception, we want \x1b7 then \x1b[1;23r (moat's region) then \x1b8. The current test would pass even if the ordering were wrong.

---

internal/run/manager.go

Low — predicate inconsistency with exec.go

// manager.go:3131
if r.Interactive && height > 1 {
    height--
}

// exec.go:95
func containerTTYHeight(statusWriter *tui.Writer, actual int) int {
    if statusWriter != nil && actual > 1 {
        return actual - 1
    }
    return actual
}

manager.go uses r.Interactive; exec.go uses statusWriter != nil. They're equivalent in practice because both are guarded by term.IsTerminal(os.Stdout). But a future caller of StartAttached for an Interactive run in a non-TTY context would silently under-report height without a status bar to consume the reserved row. A comment in manager.go linking this to the containerTTYHeight logic (and explaining why the predicates differ) would prevent the two sites from diverging silently.

---

cmd/moat/cli/exec.go

Looks good. The containerTTYHeight helper is clean and is correctly applied at all three ResizeTTY call sites. The #nosec comment on InitialHeight is accurate.

---

Summary

No blocking issues. The implementation is sound. The main things worth addressing before merge:

1. A comment at the oversized-partial-sequence fallthrough path (currently silently passes the sequence through — should be documented).
2. A comment explaining why DECSTR doesn't inline-redraw the footer (deliberate reliance on debounced redraw).
3. Optional: order-verification in the Ink startup regression test.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 4m 28s —— View job

---

Claude finished @dpup's task ——

---

Code Review

• Gather context and read changed files
• Review internal/tui/writer.go — escape sequence interception logic
• Review internal/tui/writer_test.go — test coverage and correctness
• Review cmd/moat/cli/exec.go — TTY height adjustments
• Review internal/run/manager.go — containerTTYHeight helper

---

Previous review findings — all addressed ✓

All six points from the prior review are resolved in this iteration: the oversized-partial fallthrough is documented (lines 275–279 of writer.go), the dead return nil has an "unreachable" comment (line 445), the DECSTR footer-redraw asymmetry is explained inline (lines 417–425), the Ink startup test now verifies DECSC→DECSTBM→DECRC ordering (lines 1230–1237 of writer_test.go), the missing alt-screen-exit-then-DECSTBM test is present (TestWriter_AltScreenExit_ThenDECSTBMInSameWrite), and the predicate-inconsistency between manager.go and exec.go is explained in the comment block at manager.go:3123.

---

New findings

Medium — DECSTR test doesn't verify cursor-preservation machinery

// writer_test.go:1039
func TestWriter_DECSTR_PassesThroughWithDECSTBMRestored(t *testing.T) {
    ...
    softIdx := strings.Index(out, "\x1b[!p")
    stbmIdx := strings.Index(out, "\x1b[1;23r")
    // DECSC (\x1b7) and DECRC (\x1b8) are never checked

The DECSTR handler deliberately wraps the re-emitted DECSTBM with \x1b7/\x1b8 to preserve cursor position — and the code comment calls this out as a non-obvious deviation from DECSTR's behavior. But the test only checks that \x1b[!p precedes \x1b[1;23r. If the \x1b7/\x1b8 pair were accidentally dropped, the test would still pass. Add ordering checks:

decsc := strings.Index(out, "\x1b7")
stbm  := strings.Index(out, "\x1b[1;23r")
decrc := strings.Index(out, "\x1b8")
if !(decsc >= 0 && stbm >= 0 && decrc >= 0 && softIdx < decsc && decsc < stbm && stbm < decrc) {
    t.Errorf("expected order DECSTR -> DECSC -> DECSTBM -> DECRC, ...")
}

Fix this →

---

Low — bare ESC split (\x1b | [r) not directly tested

matchControlSeq has an early-return for len(data)==1 (line 333) that returns needsMore: true for a lone ESC byte. None of the split-across-writes tests exercise this path — they all send at least \x1b[1; in the first write. A test where Write 1 is \x1b and Write 2 is [r would explicitly pin the lone-ESC buffering path. Unlikely to trigger in practice but easy to add.

Fix this →

---

Rest of the code

internal/tui/writer.go — the matchControlSeq parser is correct across all the cases that matter: bare \x1b[r, parameterized DECSTBM, DEC private-mode restores (the ? byte sets onlyDigitsAndSemi=false → falls through), DECSTR, and RIS. The two-path buffering (alt-screen prefix via isPrefixOfAltScreen + DECSTBM partial via matchControlSeq) is subtle but the priority comment at lines 245–250 is accurate and the shared-escBuf interaction is safe.

The handleControlSeqLocked RIS case schedules an inline footer redraw (correct — screen was cleared) and then the scheduleFooterRedrawLocked() call in Write() fires a 50ms debounced redraw on top. That second redraw is redundant but harmless; the debounce handles the general case and the inline draw handles the "blank row" window.

internal/tui/writer_test.go — eleven new tests; coverage is thorough. The regression test (TestWriter_InkStartupSequence_PreservesScrollRegion) now verifies the full DECSC→DECSTBM→DECRC ordering. The compositor-mode passthrough test correctly confirms that DECSTBM in alt-screen goes to the emulator without triggering a host re-emit.

cmd/moat/cli/exec.go — containerTTYHeight is clean and is correctly applied at all three ResizeTTY call sites (startup goroutine, SIGWINCH handler, post-reset nudge in resetTUI).

internal/run/manager.go — the r.Interactive && height > 1 guard is equivalent to containerTTYHeight's predicate; the comment block explaining the difference is accurate and sufficient.

---

Summary

No blocking issues. The two findings above are test-only gaps; the interception logic itself is correct. The implementation is ready to merge once the DECSTR cursor-preservation test is tightened.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 4m 18s —— View job

---

Code Review

• Gather context and read changed files
• Review internal/tui/writer.go — escape sequence interception logic
• Review internal/tui/writer_test.go — test coverage and correctness
• Review cmd/moat/cli/exec.go — TTY height adjustments
• Review internal/run/manager.go — containerTTYHeight helper

---

Previous review findings — all addressed ✓

All findings from the prior two review rounds are resolved: the DECSTR ordering test now verifies the full DECSTR → DECSC → DECSTBM → DECRC sequence (lines 1069–1073 of writer_test.go), and TestWriter_InterceptsDECSTBM_SplitAfterBareESC covers the bare-ESC buffering path (lines 1081–1109).

---

New findings

Low — TestWriter_InterceptsDECSTBM_NoArgs and _SurroundedByText use a fragile "not present" check

Both tests assert !strings.Contains(out, "\x1b[r"). This works today because \x1b[r (3 bytes: ESC, [, r) is not a byte-substring of moat's replacement \x1b[1;23r (8 bytes — r is preceded by 3, not [). But the assertion is proving a negative; it would silently pass if the interception produced a different replacement. TestWriter_InterceptsDECSTBM_MultipleInOneWrite uses strings.Count(out, "\x1b[1;23r") == 2 which is the stronger pattern. Adding a strings.Count(out, "\x1b[1;23r") == 1 check to the two simpler tests would make them self-documenting about the replacement. Not a bug, but easy to tighten.

---

Low — compositor mode render goroutine races with buf.String() in TestWriter_DECSTBM_CompositorMode_PassesToEmulator

The test enters compositor mode, which starts a renderLoop goroutine with a 16ms tick. It then writes \x1b[r (which sets w.dirty = true), immediately captures buf.String(), and checks for the absence of \x1b[1;23r. If the render tick fires in the ~16ms window between the Write and the buf capture, the render output is mixed into buf — which doesn't contain \x1b[1;23r so the assertion still passes, but the output is no longer a clean capture of the interception result.

This is a pre-existing pattern in the compositor tests (not introduced by this PR), but worth noting for a future test-hardening pass: stopping the render loop with w.mu.Lock(); w.stopRenderLoop(); w.mu.Unlock() before capturing buf.String() would make the test deterministic.

---

Rest of the code

internal/tui/writer.go — The matchControlSeq parser is correct across all edge cases: bare \x1b[r, parameterized DECSTBM, \x1b[?Pr DEC private restores (the ? byte is in the param-byte range 0x30–0x3F but is not a digit or semicolon, so onlyDigitsAndSemi=false → falls through), DECSTR, and RIS. The two-path buffering (alt-screen prefix via isPrefixOfAltScreen, DECSTBM partial via matchControlSeq) is safe: \x1b[ gets buffered by the alt-screen path, then on the next Write the combined buffer is re-evaluated — if it resolves to DECSTBM, the control-seq matcher fires correctly. The priority comment at lines 245–250 is accurate.

The scrollRegionBytes() → \x1b[1;height-1 r substitution preserves identical cursor-homing behavior to the swallowed \x1b[r (both DECSTBM commands home the cursor to the top of the scroll region), so Ink's \x1b7 \x1b[r \x1b8 bracket still restores cursor position correctly after interception.

internal/tui/writer_test.go — 11 new tests with solid coverage. The regression test TestWriter_InkStartupSequence_PreservesScrollRegion verifies full DECSC → DECSTBM → DECRC ordering. TestWriter_AltScreenExit_ThenDECSTBMInSameWrite covers the mode-flip path. The bare-ESC split test pins the len(data)==1 needsMore branch.

cmd/moat/cli/exec.go — containerTTYHeight is clean and applied consistently at all three ResizeTTY call sites (startup goroutine at line 514, SIGWINCH at line 538, resetTUI at line 678).

internal/run/manager.go — The r.Interactive && height > 1 guard is equivalent to containerTTYHeight's statusWriter != nil predicate; the comment block (lines 3123–3133) explains why they differ and why they remain consistent. Accurate and sufficient.

---

Summary

No blocking issues. The two new findings are both low-severity test observations — neither affects the correctness of the interception logic. The implementation is ready to merge.

[claude]

Claude finished @dpup's task in 4m 15s —— View job

---

Code Review

• Gather context
• Review internal/tui/writer.go
• Review internal/tui/writer_test.go
• Review cmd/moat/cli/exec.go
• Review internal/run/manager.go

---

Previous review findings — all addressed ✓

The latest commit (a55dfbd) properly adds strings.Count(out, "\x1b[1;23r") == 1 count assertions to TestWriter_InterceptsDECSTBM_NoArgs (line 951–953) and TestWriter_InterceptsDECSTBM_SurroundedByText (line 1001–1004), closing the fragile negative-only check that the last review flagged.

---

Fresh pass — no new blocking issues

internal/tui/writer.go

The escape intercept logic is correct across all edge cases I tested mentally:

• \x1b alone routes through isPrefixOfAltScreen (which returns true for bare ESC since all alt-screen seqs start with \x1b) — so it's buffered via the alt-screen prefix path, not the DECSTBM needsMore path. On the next Write the combined buffer re-enters the loop and is correctly routed. The TestWriter_InterceptsDECSTBM_SplitAfterBareESC test exercises exactly this path.

• Split-RIS (\x1b | c) follows the same route: bare ESC buffered by alt-screen prefix check; next Write sees \x1bc, which matchAltScreen doesn't match, isPrefixOfAltScreen returns false, and matchControlSeq correctly identifies ctrlRIS. Not explicitly tested, but the path is sound and covered by the bare-ESC buffer logic.

• DEC private mode restores (\x1b[?…r) — the ? byte is 0x3F, in the param byte range [0x30, 0x3F], so the param loop consumes it but sets onlyDigitsAndSemi=false, correctly preventing a DECSTBM match.

• handleControlSeqLocked DECSTR cursor-preservation: DECSC saves the live cursor before DECSTBM homes it; DECRC restores afterward. The comment accurately notes this overwrites DECSTR's own saved-cursor reset (which sets it to 1,1) — an intentional trade-off with no known TUI dependents.

• Cleanup() emits \x1b[r directly to w.out — bypassing processDataLocked, so it doesn't trigger its own interceptor. Correct.

internal/tui/writer_test.go

13 new tests; all prior review findings addressed. Assertion quality is now uniform: where a single replacement is expected, Count == 1 is verified rather than just Contains. The two multi-DECSTBM tests use Count == 2 and Count > 0 appropriately.

One pre-existing benign observation (noted in round 3, not introduced here): TestWriter_DECSTBM_CompositorMode_PassesToEmulator captures buf.String() while the 16ms render goroutine may write to buf concurrently — a data race under -race if the tick fires in the ~16ms window between the Write call and the buf.String() check. This doesn't affect test correctness (the render output doesn't contain \x1b[1;23r) and is a pre-existing pattern unrelated to this PR.

cmd/moat/cli/exec.go

containerTTYHeight is clean and applied correctly at all three ResizeTTY call sites (startup goroutine at line 514, SIGWINCH handler at line 536, resetTUI at line 676). The actual > 1 guard prevents underflow when the terminal is one row tall.

internal/run/manager.go

The r.Interactive && height > 1 guard in StartAttached is equivalent to containerTTYHeight's statusWriter != nil predicate. The comment block at lines 3129–3135 correctly explains why the predicates differ at their respective call sites and why they remain consistent in practice.

---

Summary

Four rounds of review, every finding addressed cleanly. The implementation is correct and the test suite is thorough. Ready to merge.