Fix TUI agent-pane live-stream TOCTOU race (BUG-075)

context/knowledge/gotchas/pty-terminal.md

@@ -29,6 +29,7 @@
 - **Never resize the agent's real PTY from the preview path.** A Resize RPC per cursor move would SIGWINCH every live agent the user scrolls past, triggering full repaints. Width mismatch is resolved emulation-side only (emulate wide, clip to pane).
 - **New emulators must default `cursorVisible` to `false`.** Agents send `\e[?25l` early, but after ring buffer wrap or emu rebuild, that sequence is lost. Defaulting to `true` causes a phantom cursor at bottom-left. Also, `lastContentRow` must not extend to cursor position when cursor is hidden.
 - **`renderLive` must skip `RecentOutput()` when `newBytes == 0`.** The 256KB ring buffer copy on every draw causes typing lag — keystroke redraws trigger `Draw()` before PTY echo arrives, so copying is wasted. `emuFedTotal` must only advance when bytes are actually fed to the emulator; advancing it on an empty `raw` silently skips those bytes permanently.
+- **The live incremental-feed path must read `raw` and its paired `total` from ONE atomic call (`RecentOutputTailWithTotal`), never `TotalWritten()` and `RecentOutput()` as two separate calls (BUG-075).** `readLoop` is a live, independent goroutine — for an actively streaming session (no bind, no resize, nothing but ordinary agent output arriving), bytes can land in the gap between two unsynchronized calls, making `raw` (sampled later) longer than the earlier `totalWritten` accounts for. Slicing `raw[len(raw)-newBytes:]` against that stale `newBytes` then feeds the WRONG suffix: it silently skips the true next bytes (visible as a couple of dropped characters, e.g. "Independent" → "Indepe dent") and instead feeds bytes from further ahead, recording `emuFedTotal` short of what was actually fed — the very next frame then re-feeds that same already-fed tail (visible as a short recent phrase duplicating). Both artifacts land together, on a pane that's actively being watched with no bind/resize/rebuild involved at all, which made this easy to mistake for the bind-time reconstruction defects (BUG-068/BUG-073/BUG-074) — it is a distinct, plain TOCTOU race in the steady-state live-feed path, not a reconstruction-from-history problem. `agent.Session.RecentOutputTailWithTotal`'s own doc comment already named this exact hazard (for the `/output` HTTP endpoint's cursor) — `renderLive`'s live-feed path just hadn't been updated to use it. Fix: fetch `(raw, totalWritten)` together via `sess.RecentOutputTailWithTotal(256*1024)` and recompute `newBytes` from that SAME call, but only when `!emuMissing` — the `emuMissing` (fresh-attach) branch is unconditionally `fullReplay` regardless of `raw`/`newBytes` (short-circuited by the `||`) and calls `readLiveRebuildHistory`, which does its own independent ring read, so fetching here too would just be a wasted extra 256KB copy. Regression test: `TestRenderLive_LiveFeedUsesAtomicSnapshotNotStaleTotal` (a `raceAdapter` mock whose `TotalWritten()` always reports an earlier snapshot than `RecentOutputTailWithTotal()`, simulating readLoop advancing in between — confirmed to fail on the pre-fix code with exactly the predicted "HELLOLD" garble before being fixed).
 - **`paintEmu` must cache cells for replay on idle redraws.** tview's `screen.Clear()` defeats tcell's dirty tracking (fills all cells with spaces → every `SetContent` marks cells dirty → full terminal I/O). On no-change redraws, replay cached `[]cachedCell` directly — skips 10K+ mutex ops, allocations, and style conversions per frame. Invalidate cache on scroll, reset, or session change.
 - **`startAgentRedrawLoop` must skip `QueueUpdateDraw` when idle.** Keystroke and resize events trigger their own tview redraws; the 200ms loop only needs to fire when new PTY output arrives (`TotalWritten` changed).
 - **`spinnerLoop` must only fire redraws for active (non-idle) running tasks.** Idle tasks show a static moon icon, not the spinner. Firing `QueueUpdateDraw` at 100ms when all tasks are idle causes unnecessary full-screen repaints that interfere with tmux hyperlink hover and waste CPU.

context/knowledge/index.md

@@ -5,7 +5,7 @@ Non-obvious invariants and gotchas, split by topic. Read the relevant file when
 | File | Topic | Bullets |
 | --- | --- | --- |
 | [gotchas/daemon-rpc.md](gotchas/daemon-rpc.md) | Daemon lifecycle, RPC timeouts, reconciliation races, session resume, Claude /clear recapture, binary staleness (SHA-256 content hash, not mtime), self-update, launchd auto-start + PATH, stream Since offset, paste-boundary flush, *.test fork-bomb backstop, singleton flock, PR poller (eligibility, terminal-state skip, batched per-repo graphql w/ alias-safe ids + chunked keep-stale), evidence-based completion (ExitInfo.CleanExit predicate; reconcile→InReview never Complete), hera worker finish policy (BUG-050 RollHeraWorkerToReview), startup hera-binding reconciliation, session-supervisor P1–P4 (dark PTY-owner, daemon-as-client behind cfg.Supervisor.Enabled, re-attach on bounce, default ON + in-process rollback, #707 cache-vs-EOF relay race), callWithTimeout nil-rpc guard, TUI supervisor restart, go-install skew (doctor restart-vs-path-divergence, supervisor-checked-on-auto-start, ProtocolVersion 2→3 old-supervisor-unknown, double-confirm supervisor restart), revive-restores-in_progress (BUG-B ReviveHeraWorkerToInProgress, inverse of RollHeraWorkerToReview), host-suspend watchdog (ARGUS_HOST_SUSPENDED advisory note — wall-clock gap>3m between 30s ticks, unconditional not Hera-gated, sibling of sendBounceSignals, one-shot no-dedup baseline-before-loop, monotonic-strip required, advisory-only no state mutation), Claude Code's own background-session supervisor (orphaned-worker root cause: single-PID SIGTERM can never reach a session Claude Code itself detached to its per-user supervisor; `claude agents`/`claude stop` detection+fix SHIPPED via internal/claudeagents + Runner.Stop fire-and-forget reap, not a signal-scoping bug), doctor Stop-hook registration check (detect-missing-coord-hook: REGISTERED/NOT REGISTERED/UNKNOWN, advisory-only, never gates the binary-coherence exit code), resume-time session-ID recapture (agent.RefreshResumeSessionID mirrors the exit hook because hera workers idle/StreamLost never reach captureSessionIDPostExit; Claude-only; wired at reattachSupervised orphans + TUI startSession + REST resume/restart; idempotent, never blanks/fabricates), doctor diligence-profile-library check (add-doctor-profile-check: FOUND/NONE FOUND/UNKNOWN, library-existence-only not per-project binding, missing-dir vs unreadable-dir tri-state, advisory-only) | 108 |
-| [gotchas/pty-terminal.md](gotchas/pty-terminal.md) | PTY sizing, x/vt emulator, ring buffer, replay cache, paint cache, lazyScreen, test concurrency, ESC-boundary alignment, live rebuild from log tail, monotonic firstByteOffset, scrollOffset clamp, waitLoop close-after-drain order, rerender gates (unchanged-cols, cache invalidation, blocked-on-prompt), OSC 0x9C-in-UTF8 strip filter, persistent preview emulator (PreviewVT reuse-via-RIS), plugin terminalpane cursor sync, alt-screen keyboard-scroll + scroll-mode-entry suppression (BUG-031, not just the wheel), scroll-past-window lazy extend (BUG-E), scroll replay authored-width emulate-clip (live-scroll corruption), dimension-change resize-in-place instead of lossy 8MB-tail rebuild (BUG-068, overlapping/garbled live-view corruption), ring-wrap exact-offset log catch-up instead of lossy 8MB-tail rebuild (BUG-073, BUG-068's ring-wrap sibling — reached by backgrounding a busy agent's pane, not resize) | 64 |
+| [gotchas/pty-terminal.md](gotchas/pty-terminal.md) | PTY sizing, x/vt emulator, ring buffer, replay cache, paint cache, lazyScreen, test concurrency, ESC-boundary alignment, live rebuild from log tail, monotonic firstByteOffset, scrollOffset clamp, waitLoop close-after-drain order, rerender gates (unchanged-cols, cache invalidation, blocked-on-prompt), OSC 0x9C-in-UTF8 strip filter, persistent preview emulator (PreviewVT reuse-via-RIS), plugin terminalpane cursor sync, alt-screen keyboard-scroll + scroll-mode-entry suppression (BUG-031, not just the wheel), scroll-past-window lazy extend (BUG-E), scroll replay authored-width emulate-clip (live-scroll corruption), dimension-change resize-in-place instead of lossy 8MB-tail rebuild (BUG-068, overlapping/garbled live-view corruption), ring-wrap exact-offset log catch-up instead of lossy 8MB-tail rebuild (BUG-073, BUG-068's ring-wrap sibling — reached by backgrounding a busy agent's pane, not resize), live incremental-feed atomic (raw,total) snapshot instead of two separate racy calls (BUG-075, TOCTOU race distinct from BUG-068/073/074 — reachable on an actively-streamed pane with no bind/resize at all, causes a duplicated recent phrase + a couple of dropped characters) | 65 |
 | [gotchas/ui-threading.md](gotchas/ui-threading.md) | tview thread safety, tick-goroutine rules, lazyScreen fill invariant, paste/input batching, tmux UX-tearing post-mortem (no Sync; 3 legit repair callsites), OnBranchChange log-only contract, EventFocus drift recovery, stderr/stdout-after-Init fd 2 guards, status-bar notice auto-expire (15s TTL, lazy revert via 1s tick, no Sync/timer), SetScreen swallows tcell Init() errors (no-ctty nil-tty EnableMouse panic, probeTerminal preflight guard), probeTerminal false-positive-on-every-real-terminal regression (tcell devTty.Close() nil-`f` → os.ErrInvalid, fix discards Close() err + probeTerminalDev pty-slave test seam) | 29 |
 | [gotchas/ci-gates.md](gotchas/ci-gates.md) | `make pre-pr` per-gate failure recipes (fmt-check, test-cover-gate floor, lint-pr new-from-rev, vuln stdlib continue-on-error, macOS PTY-exhaustion flake in internal/agent under full-suite -race) | 5 |
 | [gotchas/sandbox.md](gotchas/sandbox.md) | macOS sandbox-exec SBPL profiles, symlink resolution, allowed paths, Chrome support dir for Playwright, AppleEvent allowlist, Messages.app legacy alias, picker modal, TCC re-prompt fix (stable-signed local binary via workflow-neutral `make install-signed`; first-sign Keychain "Always Allow") | 26 |

internal/tui/terminal/terminalpane.go

@@ -1629,8 +1629,33 @@ func (tp *TerminalPane) renderLive(screen tcell.Screen, x, y, w, h int, ptyCols,
 
 	if newBytes > 0 || emuMissing {
 		var raw []byte
-		if sess != nil {
-			raw = sess.RecentOutput()
+		if sess != nil && !emuMissing {
+			// Atomic (raw, total) snapshot — NOT the totalWritten sampled
+			// above paired with a separate RecentOutput() call. readLoop is
+			// a live, independent goroutine: for an actively streaming
+			// session, bytes can arrive in the gap between two
+			// unsynchronized calls, making raw LONGER than the earlier
+			// totalWritten accounts for. Slicing raw[len(raw)-newBytes:]
+			// against that stale newBytes then feeds the WRONG suffix — it
+			// silently skips the true next bytes (dropped characters) and
+			// instead feeds bytes from further ahead, recording emuFedTotal
+			// short of what was actually fed; the next frame re-feeds that
+			// same already-fed tail (a duplicated recent phrase). Reachable
+			// on an actively-worked pane with no bind/resize/rebuild
+			// involved at all — the resulting garble sits alongside, and is
+			// easy to mistake for, the bind-time reconstruction defects
+			// (BUG-068/BUG-073/BUG-074) since it looks similar, but this is
+			// a plain TOCTOU race in the steady-state live-feed path. Re-
+			// deriving totalWritten from this SAME atomic read (discarding
+			// the racy one above) keeps raw and newBytes always consistent.
+			//
+			// Skipped when emuMissing: fullReplay is unconditionally true
+			// in that case (short-circuited below), and the fullReplay
+			// branch calls readLiveRebuildHistory, which does its own
+			// independent ring read — fetching raw/totalWritten here too
+			// would just be a wasted extra 256KB copy.
+			raw, totalWritten = sess.RecentOutputTailWithTotal(256 * 1024)
+			newBytes = totalWritten - tp.emuFedTotal
 		}
 		// "Full replay" is required when the emulator was just created
 		// (no prior state to preserve) OR the ring wrapped past our last

internal/tui/terminal/terminalpane_test.go

@@ -1057,15 +1057,21 @@ func TestTerminalPane_ResetVTClearsReplayCache(t *testing.T) {
 	}
 }
 
-// countingAdapter wraps mockAdapter and counts RecentOutput calls.
+// countingAdapter wraps mockAdapter and counts the expensive ring-buffer
+// copy calls renderLive's live-feed path makes. It instruments
+// RecentOutputTailWithTotal — the atomic (raw, total) snapshot renderLive
+// uses — not RecentOutput, which that path no longer calls (see the
+// live-streaming-race fix: reading totalWritten and raw via two separate,
+// unsynchronized calls let readLoop advance the ring in between for an
+// actively streaming session, misaligning the incremental-feed slice).
 type countingAdapter struct {
 	mockAdapter
 	recentOutputCalls int
 }
 
-func (c *countingAdapter) RecentOutput() []byte {
+func (c *countingAdapter) RecentOutputTailWithTotal(n int) ([]byte, uint64) {
 	c.recentOutputCalls++
-	return c.mockAdapter.RecentOutput()
+	return c.mockAdapter.RecentOutputTailWithTotal(n)
 }
 
 func TestTerminalPane_RenderLiveSkipsCopyWhenIdle(t *testing.T) {
@@ -1087,7 +1093,7 @@ func TestTerminalPane_RenderLiveSkipsCopyWhenIdle(t *testing.T) {
 
 	firstEmu := tp.emu
 
-	// Second render with same TotalWritten — should NOT call RecentOutput.
+	// Second render with same TotalWritten — should NOT call RecentOutputTailWithTotal.
 	tp.renderLive(screen, 0, 0, 40, 10, 40, 10)
 	testutil.Equal(t, sess.recentOutputCalls, 1) // still 1
 	if tp.emu != firstEmu {
@@ -3136,6 +3142,98 @@ func TestRenderLive_EmuRebuildInvalidatesPaintCache(t *testing.T) {
 	}
 }
 
+// raceAdapter simulates the live-streaming TOCTOU race renderLive must not
+// be vulnerable to: TotalWritten() always reports an EARLIER snapshot
+// (staleTotal), while RecentOutput()/RecentOutputTailWithTotal() always
+// report a LATER one (freshOutput/freshTotal) — exactly what a real Session
+// looks like when readLoop advances the ring between two separately-called,
+// unsynchronized accessors. A correct renderLive must derive newBytes from
+// the SAME atomic call that produced raw (RecentOutputTailWithTotal), never
+// from a totalWritten sampled by a separate, earlier TotalWritten() call.
+type raceAdapter struct {
+	alive       bool
+	staleTotal  uint64
+	freshOutput []byte
+	freshTotal  uint64
+}
+
+func (r *raceAdapter) WriteInput(p []byte) (int, error) { return len(p), nil }
+func (r *raceAdapter) Resize(rows, cols uint16) error   { return nil }
+func (r *raceAdapter) RecentOutput() []byte             { return r.freshOutput }
+func (r *raceAdapter) RecentOutputTail(n int) []byte {
+	if n >= len(r.freshOutput) {
+		return r.freshOutput
+	}
+	return r.freshOutput[len(r.freshOutput)-n:]
+}
+func (r *raceAdapter) RecentOutputTailWithTotal(n int) ([]byte, uint64) {
+	return r.RecentOutputTail(n), r.freshTotal
+}
+func (r *raceAdapter) TotalWritten() uint64 { return r.staleTotal }
+func (r *raceAdapter) Alive() bool          { return r.alive }
+func (r *raceAdapter) PTYSize() (int, int)  { return 80, 24 }
+
+// TestRenderLive_LiveFeedUsesAtomicSnapshotNotStaleTotal is a regression test
+// for a live-streaming race distinct from BUG-068/BUG-073/BUG-074 (all of
+// which fire only around a bind/resize/rebuild event): renderLive samples
+// totalWritten via a separate, EARLIER TotalWritten() call, then later reads
+// raw via RecentOutput() — readLoop is a live, independent goroutine, so for
+// an actively streaming session (no pane switch, no resize, nothing but
+// ordinary output arriving) bytes can land in the gap between those two
+// calls, making raw longer than the earlier totalWritten accounts for.
+// Slicing raw[len(raw)-newBytes:] against that stale newBytes feeds the
+// WRONG suffix — it silently skips the true next bytes (dropped characters)
+// and instead feeds bytes from further ahead, understating emuFedTotal; the
+// very next frame then re-feeds that same already-fed tail (a duplicated
+// recent phrase) — together producing exactly the "duplicated recent text +
+// a couple of dropped characters, on an actively-worked pane, no bind
+// involved" signature. Fix: re-derive totalWritten from the SAME atomic
+// RecentOutputTailWithTotal call that produces raw.
+func TestRenderLive_LiveFeedUsesAtomicSnapshotNotStaleTotal(t *testing.T) {
+	tp := NewTerminalPane()
+	sess := &raceAdapter{alive: true, staleTotal: 5, freshOutput: []byte("HELLO"), freshTotal: 5}
+	tp.SetSession(sess)
+
+	screen := tcell.NewSimulationScreen("UTF-8")
+	if err := screen.Init(); err != nil {
+		t.Fatalf("screen.Init: %v", err)
+	}
+	defer screen.Fini()
+	screen.SetSize(80, 24)
+	tp.renderLive(screen, 0, 0, 80, 10, 80, 10)
+
+	// Simulate the race: by the time TotalWritten() was sampled, only "WO"
+	// (2 of the next 5 bytes) had landed — but by the time RecentOutput()
+	// (or, post-fix, the atomic RecentOutputTailWithTotal) is actually
+	// called, all 5 new bytes ("WORLD") are already in the ring.
+	sess.staleTotal = 7 // 5 + 2: what a separately-called TotalWritten() would have seen
+	sess.freshOutput = []byte("HELLOWORLD")
+	sess.freshTotal = 10 // the true total by the time output is read
+
+	screen2 := tcell.NewSimulationScreen("UTF-8")
+	if err := screen2.Init(); err != nil {
+		t.Fatalf("screen2.Init: %v", err)
+	}
+	defer screen2.Fini()
+	screen2.SetSize(80, 24)
+	tp.renderLive(screen2, 0, 0, 80, 10, 80, 10)
+
+	if tp.emuFedTotal != 10 {
+		t.Errorf("emuFedTotal = %d, want 10 (fully caught up to the TRUE total, not the stale TotalWritten() sample)", tp.emuFedTotal)
+	}
+	if s, _, _ := screen2.Get(0, 0); s != "H" {
+		t.Fatalf("row corrupted at (0,0): got %q, want \"H\" (start of HELLOWORLD)", s)
+	}
+	got := ""
+	for col := 0; col < len("HELLOWORLD"); col++ {
+		s, _, _ := screen2.Get(col, 0)
+		got += s
+	}
+	if got != "HELLOWORLD" {
+		t.Errorf("row 0 = %q, want \"HELLOWORLD\" (no dropped chars, no duplicated tail from the race)", got)
+	}
+}
+
 // TestTerminalPane_ResizePreservesEmulatorState is a regression test for a
 // bug where a PTY dimension change (a real terminal resize, the Ctrl+Z
 // fullscreen toggle, a split/zoom layout change — anything that alters the