From c98c11b5c32ce8ecf7ab9dfe89a865a819f8234e Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Mon, 31 Aug 2026 09:37:13 +0100 Subject: [PATCH] fix(maestro): replay tmux's screen to fresh attaches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A new attach replayed the tail of the raw byte buffer. That tail starts mid-stream, long after a full-screen program set the modes, colours and cursor position that give those bytes meaning, so a browser refresh painted garbage over every agent TUI until something forced a repaint — which is why opening the same session in the TUI (a resize) fixed it. Ask tmux for the pane's current screen instead, and fall back to the byte buffer when there is no pane left to capture, as for a dead session. Co-Authored-By: Claude Opus 5 (1M context) --- crates/maestro-daemon/src/panes.rs | 23 ++ crates/maestro-daemon/src/pty.rs | 15 +- crates/maestro-daemon/src/session.rs | 23 +- crates/maestro-daemon/src/tmux.rs | 257 ++++++++++++++++++ .../maestro-daemon/tests/session_manager.rs | 58 ++++ 5 files changed, 374 insertions(+), 2 deletions(-) diff --git a/crates/maestro-daemon/src/panes.rs b/crates/maestro-daemon/src/panes.rs index 1769025..088a65e 100644 --- a/crates/maestro-daemon/src/panes.rs +++ b/crates/maestro-daemon/src/panes.rs @@ -39,6 +39,10 @@ pub trait Panes: std::fmt::Debug + Send + Sync + 'static { fn pipe_pane(&self, id: Uuid, sink_command: &str) -> Result<(), TmuxError>; fn resize_window(&self, id: Uuid, cols: u16, rows: u16) -> Result<(), TmuxError>; + + /// Escape-sequence snapshot of the pane's current screen, or `None` when + /// there is no pane to capture. See [`Tmux::capture_pane`]. + fn capture_pane(&self, id: Uuid) -> Result>, TmuxError>; } impl Panes for Tmux { @@ -76,6 +80,10 @@ impl Panes for Tmux { fn resize_window(&self, id: Uuid, cols: u16, rows: u16) -> Result<(), TmuxError> { Tmux::resize_window(self, id, cols, rows) } + + fn capture_pane(&self, id: Uuid) -> Result>, TmuxError> { + Tmux::capture_pane(self, id) + } } // --------------------------------------------------------------------------- @@ -92,6 +100,7 @@ pub enum Op { SendKeys, PipePane, ResizeWindow, + CapturePane, } /// One recorded call, in the order the manager made it. Rollback ordering is a @@ -116,6 +125,7 @@ pub enum Call { cols: u16, rows: u16, }, + CapturePane(Uuid), } /// In-memory [`Panes`] for tests. @@ -133,6 +143,7 @@ struct FakeState { live: HashSet, panes: HashMap, calls: Vec, + captures: HashMap>, fail: HashMap, } @@ -162,6 +173,12 @@ impl FakePanes { self.lock().live.contains(&id) } + /// Screen snapshot `capture_pane` hands back for `id`. Unset means "no + /// pane", which is what a dead session looks like. + pub fn set_capture(&self, id: Uuid, bytes: impl Into>) { + self.lock().captures.insert(id, bytes.into()); + } + pub fn set_pane_info(&self, id: Uuid, info: PaneInfo) { self.lock().panes.insert(id, info); } @@ -242,6 +259,12 @@ impl Panes for FakePanes { fn resize_window(&self, id: Uuid, cols: u16, rows: u16) -> Result<(), TmuxError> { self.record(Call::ResizeWindow { id, cols, rows }, Op::ResizeWindow) } + + fn capture_pane(&self, id: Uuid) -> Result>, TmuxError> { + self.record(Call::CapturePane(id), Op::CapturePane)?; + let st = self.lock(); + Ok(st.captures.get(&id).cloned()) + } } #[cfg(test)] diff --git a/crates/maestro-daemon/src/pty.rs b/crates/maestro-daemon/src/pty.rs index bc4855d..44f079d 100644 --- a/crates/maestro-daemon/src/pty.rs +++ b/crates/maestro-daemon/src/pty.rs @@ -141,8 +141,21 @@ impl PtyBridge { /// `subscribe()` keeps the reader task from publishing in between, so /// the seam has no gap and no duplicates. pub fn subscribe_with_replay(&self) -> (Vec, broadcast::Receiver>) { + self.subscribe_with_snapshot(|| None) + } + + /// As [`Self::subscribe_with_replay`], but lets the caller supply the + /// replay payload instead of the raw byte buffer — used to hand a fresh + /// client tmux's own view of the screen, which the byte tail cannot + /// reconstruct for a full-screen program. `capture` runs while the reader + /// is held off, so the payload and the live stream still meet with no gap + /// and no duplicate. Returning `None` falls back to the byte buffer. + pub fn subscribe_with_snapshot(&self, capture: F) -> (Vec, broadcast::Receiver>) + where + F: FnOnce() -> Option>, + { let guard = self.replay.lock_for_snapshot(); - let snapshot = guard.snapshot(); + let snapshot = capture().unwrap_or_else(|| guard.snapshot()); let rx = match &self.sender { Some(tx) => tx.subscribe(), // Sealed: hand back the scrollback plus a receiver that is already diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 8aff997..d34468a 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -260,8 +260,29 @@ impl SessionManager { &self, id: Uuid, ) -> Option<(Vec, broadcast::Receiver>)> { + let tmux = self.inner.tmux.clone(); let sessions = self.inner.sessions.lock().await; - sessions.get(&id).map(|r| r.bridge.subscribe_with_replay()) + let rec = sessions.get(&id)?; + // For a live session, tmux's own screen beats the byte tail: the tail + // starts mid-stream, so a full-screen program (an agent TUI) replayed + // from it renders as garbage until something forces a repaint. A dead + // session has no pane left to capture, so it keeps the byte tail. + let alive = rec.session.status == SessionStatus::Running; + // The capture shells out to tmux while the replay lock is held, which + // parks the reader for the length of one tmux round-trip. That only + // happens on attach, and the alternative is a seam in the stream. + Some(rec.bridge.subscribe_with_snapshot(|| { + if !alive { + return None; + } + match tmux.capture_pane(id) { + Ok(snapshot) => snapshot, + Err(e) => { + tracing::warn!(error = ?e, %id, "capture-pane failed; replaying raw bytes"); + None + } + } + })) } pub async fn has(&self, id: Uuid) -> bool { diff --git a/crates/maestro-daemon/src/tmux.rs b/crates/maestro-daemon/src/tmux.rs index 1d278b9..545c02f 100644 --- a/crates/maestro-daemon/src/tmux.rs +++ b/crates/maestro-daemon/src/tmux.rs @@ -246,6 +246,66 @@ impl Tmux { check(out) } + /// Escape-sequence snapshot of a pane's current screen (#…), for clients + /// that just attached. + /// + /// The raw byte replay buffer cannot reconstruct a full-screen program: + /// its tail starts mid-stream, long after the app set up the modes, + /// colours and cursor position that give those bytes meaning. A browser + /// refresh therefore painted garbage until something forced the app to + /// repaint. tmux already holds the authoritative screen, so ask it. + /// + /// Returns `Ok(None)` when the session or the tmux server is gone — the + /// caller falls back to the byte replay, which is all a dead session has. + pub fn capture_pane(&self, id: Uuid) -> Result>, TmuxError> { + let name = session_name(id); + let start = format!("-{CAPTURE_HISTORY_LINES}"); + // One invocation, so tmux runs both commands in a single pass of its + // command queue: the cursor position cannot belong to an older screen + // than the captured content. Two invocations raced — a program that + // switched to the alternate screen in between produced a snapshot with + // the new screen and the old cursor. + let out = self + .cmd() + .args([ + "display-message", + "-p", + "-t", + &name, + "-F", + "#{cursor_x} #{cursor_y} #{pane_height} #{alternate_on} #{cursor_flag}", + ";", + "capture-pane", + "-p", + "-e", + "-t", + &name, + "-S", + &start, + ]) + .output()?; + let Some(stdout) = soft_output(out)? else { + return Ok(None); + }; + let Some((head, content)) = stdout.split_once('\n') else { + return Ok(None); + }; + let Some(geom) = parse_screen_geometry(head) else { + return Ok(None); + }; + let mut lines: Vec<&str> = content + .strip_suffix('\n') + .unwrap_or(content) + .split('\n') + .collect(); + // Scrollback belongs to the normal screen. Replaying it under an + // alternate-screen program would paste the shell's history behind it. + if geom.alternate { + lines.drain(..lines.len().saturating_sub(geom.pane_height)); + } + Ok(Some(render_screen_snapshot(&lines, geom))) + } + /// `tmux resize-window -t maestro- -x -y `. pub fn resize_window(&self, id: Uuid, cols: u16, rows: u16) -> Result<(), TmuxError> { let name = session_name(id); @@ -446,6 +506,124 @@ fn new_session_args( args } +/// How much scrollback a fresh attach inherits from tmux's history, in lines. +/// Enough to land in context after a browser refresh without making the first +/// frame huge. +const CAPTURE_HISTORY_LINES: usize = 300; + +/// Cursor position and screen geometry of a pane, from `display-message`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct ScreenGeometry { + cursor_x: usize, + cursor_y: usize, + pane_height: usize, + alternate: bool, + cursor_visible: bool, +} + +fn parse_screen_geometry(s: &str) -> Option { + let line = s.lines().next()?; + let mut it = line.split_whitespace(); + let cursor_x = it.next()?.parse().ok()?; + let cursor_y = it.next()?.parse().ok()?; + let pane_height: usize = it.next()?.parse().ok()?; + let alternate = it.next()? == "1"; + // Older tmux without `cursor_flag` just means "assume visible". + let cursor_visible = it.next().map(|f| f == "1").unwrap_or(true); + if pane_height == 0 { + return None; + } + Some(ScreenGeometry { + cursor_x, + cursor_y, + pane_height, + alternate, + cursor_visible, + }) +} + +/// Turn captured pane lines into bytes a fresh terminal can replay: clear, +/// print the lines, then put the cursor back where the pane has it. +/// +/// The cursor is restored with *relative* moves so the snapshot renders the +/// same whatever height the client's terminal has — it may not match the +/// pane's yet, since the client's first `Resize` arrives after this. +fn render_screen_snapshot(lines: &[&str], geom: ScreenGeometry) -> Vec { + // Screen rows are the last `pane_height` captured lines; anything before + // them is scrollback. + let screen_top = lines.len().saturating_sub(geom.pane_height); + let cursor_row = (screen_top + geom.cursor_y).min(lines.len().saturating_sub(1)); + // Trailing blank rows would scroll the real content off a short client + // terminal, so drop them — but never the cursor's own row or anything + // above it, which is where a program that draws below its cursor lives. + let last = lines + .iter() + .rposition(|l| !strip_sgr(l).trim().is_empty()) + .unwrap_or(0) + .max(cursor_row); + + let mut out = Vec::new(); + // Put the client on the same buffer the pane is using, so a program's + // repaints stay out of the client's scrollback while it runs. + if geom.alternate { + out.extend_from_slice(b"\x1b[?1049h"); + } + out.extend_from_slice(b"\x1b[H\x1b[2J\x1b[m"); + for (i, line) in lines[..=last].iter().enumerate() { + if i > 0 { + out.extend_from_slice(b"\r\n"); + } + out.extend_from_slice(line.as_bytes()); + } + out.extend_from_slice(b"\x1b[m\r"); + if last > cursor_row { + out.extend_from_slice(format!("\x1b[{}A", last - cursor_row).as_bytes()); + } + if geom.cursor_x > 0 { + out.extend_from_slice(format!("\x1b[{}C", geom.cursor_x).as_bytes()); + } + if !geom.cursor_visible { + out.extend_from_slice(b"\x1b[?25l"); + } + out +} + +/// Text of a line with SGR escapes removed, for emptiness checks. +fn strip_sgr(line: &str) -> String { + let mut out = String::with_capacity(line.len()); + let mut chars = line.chars(); + while let Some(c) = chars.next() { + if c != '\x1b' { + out.push(c); + continue; + } + // Skip CSI ... final-byte. Anything else escape-ish is not produced by + // `capture-pane -e`, so treating it the same way is harmless. + for c in chars.by_ref() { + if c.is_ascii_alphabetic() { + break; + } + } + } + out +} + +/// `Ok(None)` when tmux failed because the server or the target is gone, +/// `Ok(Some(stdout))` on success, `Err` for anything else. +fn soft_output(out: Output) -> Result, TmuxError> { + if out.status.success() { + return Ok(Some(String::from_utf8_lossy(&out.stdout).into_owned())); + } + let stderr = String::from_utf8_lossy(&out.stderr); + if is_no_server_error(&stderr) || stderr.contains("can't find") { + return Ok(None); + } + Err(TmuxError::NonZero { + code: out.status.code().unwrap_or(-1), + stderr: stderr.into_owned(), + }) +} + fn check(out: Output) -> Result<(), TmuxError> { if out.status.success() { Ok(()) @@ -461,6 +639,85 @@ fn check(out: Output) -> Result<(), TmuxError> { mod tests { use super::*; + #[test] + fn parse_screen_geometry_reads_cursor_and_alternate_flag() { + let g = parse_screen_geometry("12 3 40 1 0\n").unwrap(); + assert_eq!( + g, + ScreenGeometry { + cursor_x: 12, + cursor_y: 3, + pane_height: 40, + alternate: true, + cursor_visible: false, + } + ); + let g = parse_screen_geometry("0 0 24 0").unwrap(); + assert!(!g.alternate); + assert!(g.cursor_visible, "missing cursor_flag means visible"); + assert!(parse_screen_geometry("0 0 0 0").is_none()); + assert!(parse_screen_geometry("garbage").is_none()); + } + + #[test] + fn snapshot_clears_prints_lines_and_parks_the_cursor() { + // Two scrollback lines, then a 3-row screen with the cursor on row 1. + let lines = ["old", "older", "prompt> hi", "", ""]; + let out = render_screen_snapshot( + &lines, + ScreenGeometry { + cursor_x: 10, + cursor_y: 0, + pane_height: 3, + alternate: false, + cursor_visible: true, + }, + ); + let out = String::from_utf8(out).unwrap(); + assert_eq!( + out, + "\x1b[H\x1b[2J\x1b[mold\r\nolder\r\nprompt> hi\x1b[m\r\x1b[10C" + ); + } + + #[test] + fn snapshot_keeps_rows_below_the_cursor_and_moves_back_up() { + let lines = ["prompt> hi", "hint line", ""]; + let out = render_screen_snapshot( + &lines, + ScreenGeometry { + cursor_x: 0, + cursor_y: 0, + pane_height: 3, + alternate: true, + cursor_visible: false, + }, + ); + let out = String::from_utf8(out).unwrap(); + assert!(out.starts_with("\x1b[?1049h\x1b[H\x1b[2J"), "{out:?}"); + assert!( + out.ends_with("prompt> hi\r\nhint line\x1b[m\r\x1b[1A\x1b[?25l"), + "{out:?}" + ); + } + + #[test] + fn snapshot_ignores_colour_only_rows_when_trimming() { + let lines = ["text", "\x1b[38;5;4m\x1b[m", ""]; + let out = String::from_utf8(render_screen_snapshot( + &lines, + ScreenGeometry { + cursor_x: 4, + cursor_y: 0, + pane_height: 3, + alternate: false, + cursor_visible: true, + }, + )) + .unwrap(); + assert_eq!(out, "\x1b[H\x1b[2J\x1b[mtext\x1b[m\r\x1b[4C"); + } + /// Every phrasing must be recognised: an unmatched one makes /// `list_sessions` fail, and a failed pass reaps nothing. #[test] diff --git a/crates/maestro-daemon/tests/session_manager.rs b/crates/maestro-daemon/tests/session_manager.rs index 5389026..a07114a 100644 --- a/crates/maestro-daemon/tests/session_manager.rs +++ b/crates/maestro-daemon/tests/session_manager.rs @@ -188,6 +188,64 @@ async fn create_captures_initial_output_in_replay_buffer() { fx.manager.kill(s.id, false).await.unwrap(); } +/// A fresh attach must land on the pane's *current* screen. Replaying the raw +/// byte tail instead reproduced a browser refresh painting garbage over an +/// agent TUI: the tail starts mid-stream, so the clears, modes and cursor +/// moves that gave those bytes meaning are long gone. +#[tokio::test] +async fn attach_replay_is_the_current_screen_not_the_byte_tail() { + let fx = DaemonFixture::new(); + let cwd = TempDir::new().unwrap(); + + let s = fx + .manager + .create(CreateSessionRequest { + cwd: cwd.path().to_path_buf(), + // Print a line, then switch to the alternate screen the way a + // full-screen program does on startup. Only the second marker is + // on the pane; the first belongs to the screen behind it. + command: "sh -c 'printf STALE_MARKER_LINE\\\\n; sleep 0.5; printf \"\\\\033[?1049h\\\\033[2J\\\\033[HLIVE_MARKER\"; exec sleep 30'" + .into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: None, + }) + .await + .expect("create"); + + let want = b"LIVE_MARKER"; + let deadline = std::time::Instant::now() + Duration::from_secs(5); + let mut replay = Vec::new(); + while std::time::Instant::now() < deadline { + let (bytes, _rx) = fx + .manager + .subscribe_pty_with_replay(s.id) + .await + .expect("subscribe"); + replay = bytes; + if replay.windows(want.len()).any(|w| w == want) { + break; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + + let text = String::from_utf8_lossy(&replay).into_owned(); + assert!( + replay.starts_with(b"\x1b[?1049h\x1b[H\x1b[2J"), + "replay did not start with an alternate-screen snapshot: {text:?}" + ); + assert!( + text.contains("LIVE_MARKER"), + "replay lost the live screen: {text:?}" + ); + assert!( + !text.contains("STALE_MARKER_LINE"), + "replay carried bytes from behind the alternate screen: {text:?}" + ); + + fx.manager.kill(s.id, false).await.unwrap(); +} + /// Regression for NAP-228: after `recover`, PTY output must reach new /// subscribers. Before the fix tmux's stale `pipe-pane` (with `-o`) silently /// kept feeding the old FIFO and the revived manager's subscribers saw