From 0f4d952c4cdfca13b12d19bd5e85bc18bd55805b Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 14:20:55 -0700 Subject: [PATCH 01/11] refactor(terminal): name the key shapes once, close three oracle gaps --- src/protocol.rs | 9 --- src/terminal/ansi.rs | 7 +- src/terminal/codex_tests.rs | 6 +- src/terminal/emulator.rs | 64 +++++++---------- src/terminal/golden.rs | 10 +-- src/terminal/input.rs | 133 +++++++++++++++++------------------- tests/corpus/README.md | 9 +-- 7 files changed, 103 insertions(+), 135 deletions(-) diff --git a/src/protocol.rs b/src/protocol.rs index 864bc8a..7c75312 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -172,15 +172,6 @@ pub struct Mods { pub ctrl: bool, } -impl Mods { - /// Return xterm's modifier parameter `1 + shift + 2·alt + 4·ctrl`, or - /// `None` when no modifier is held. - pub fn param(self) -> Option { - let bits = self.shift as u8 + 2 * self.alt as u8 + 4 * self.ctrl as u8; - (bits != 0).then_some(1 + bits) - } -} - /// A core→client message. The client keeps a local mirror of the task set and /// the watched screen, updated only by these. #[derive(Debug, Clone, PartialEq)] diff --git a/src/terminal/ansi.rs b/src/terminal/ansi.rs index 64b93e1..831d269 100644 --- a/src/terminal/ansi.rs +++ b/src/terminal/ansi.rs @@ -256,9 +256,7 @@ pub fn contents(term: &Term) -> String { } push_zerowidth(&mut out, cell); } - while out.len() > row_start && out.ends_with(' ') { - out.pop(); - } + out.truncate(row_start + out[row_start..].trim_end_matches(' ').len()); } out } @@ -578,7 +576,9 @@ mod tests { }; } + corpus_oracle!(corpus_claude_resume, "claude_resume.bin"); corpus_oracle!(corpus_codex_resume, "codex_resume.bin"); + corpus_oracle!(corpus_grok_resume, "grok_resume.bin"); corpus_oracle!(corpus_tmux_split, "tmux_split.bin"); corpus_oracle!(corpus_vim_session, "vim_session.bin"); corpus_oracle!(corpus_less_altscreen, "less_altscreen.bin"); @@ -587,6 +587,7 @@ mod tests { corpus_oracle!(corpus_build_log, "build_log.bin"); corpus_oracle!(corpus_wide_emoji, "wide_emoji.bin"); corpus_oracle!(corpus_dec_scrollregion, "dec_scrollregion.bin"); + corpus_oracle!(corpus_topregion_scroll, "topregion_scroll.bin"); /// Scrolled-viewport oracle: a nonzero display offset must serialize the /// displayed (offset) content, not the live screen. diff --git a/src/terminal/codex_tests.rs b/src/terminal/codex_tests.rs index 063ef8a..9006e37 100644 --- a/src/terminal/codex_tests.rs +++ b/src/terminal/codex_tests.rs @@ -1,7 +1,6 @@ //! Replay release-binary output; the model responses were scripted locally. -use super::emulator::Emulator; -use crate::protocol::ClipboardKind; +use crate::{emulator::Emulator, protocol::ClipboardKind}; const REPLY: &str = "PHASE0 COMPLETE. Deterministic local response."; const PROMPT: &str = "Phase zero fixture: reply with the completion marker."; @@ -75,7 +74,8 @@ fn codex_release_terminal_transitions() { assert!(!text.contains(PROMPT), "{context}: stale prompt"); assert!(!text.contains(REPLY), "{context}: stale reply"); } - _ => {} + "startup" | "copy-menu" => {} + other => panic!("{context}: unknown checkpoint {other}"), } if checkpoint == "copied" { assert!( diff --git a/src/terminal/emulator.rs b/src/terminal/emulator.rs index 000f8ea..aa5edf3 100644 --- a/src/terminal/emulator.rs +++ b/src/terminal/emulator.rs @@ -9,7 +9,7 @@ use std::{ use alacritty_terminal::{ Term, event::{Event, EventListener}, - grid::{Dimensions, Row, Scroll}, + grid::{Dimensions, Scroll}, index::Line, term::{ Config, TermMode, @@ -293,11 +293,6 @@ impl Emulator { } } - /// Record one parser or synchronized-frame advance. - fn observe_advance(&mut self) { - self.revision += 1; - } - /// Parse raw child output into the grid. Returns the probe replies the /// backend generated that pass the allowlist, in generation order; the /// caller owns delivering them to the child. @@ -308,7 +303,7 @@ impl Emulator { clipboard: &mut self.clipboard, }; self.parser.advance(&mut observed, bytes); - self.observe_advance(); + self.revision += 1; self.bytes_since_sweep = self.bytes_since_sweep.saturating_add(bytes.len()); if self.bytes_since_sweep >= SWEEP_INTERVAL_BYTES { self.bytes_since_sweep = 0; @@ -325,7 +320,7 @@ impl Emulator { clipboard: &mut self.clipboard, }; self.parser.stop_sync(&mut observed); - self.observe_advance(); + self.revision += 1; self.drain_allowed() } @@ -474,9 +469,9 @@ impl Emulator { /// Capture accessors for the dashboard-preview resolution layer /// (`crate::preview`). impl Emulator { - /// Monotonic count of grid advances. Bumps on every `process` call and - /// on each sync-frame landing; equal reads mean the grid did not advance - /// in between, so a poller can skip re-reading it. + /// Monotonic count of grid advances. Bumps on every `process` call, on + /// each sync-frame landing, and on `resize`; equal reads mean the grid did + /// not advance in between, so a poller can skip re-reading it. pub fn revision(&self) -> u64 { self.revision } @@ -549,38 +544,29 @@ fn live_floor_of(term: &Term) -> String { String::new() } -/// Append a grid row's glyphs, omitting wide-character spacers, mapping tabs -/// to spaces, and preserving combining marks. Callers handle trailing spaces. +/// Plain text of one live-viewport row: wide-character spacers omitted, tabs +/// mapped to spaces, combining marks preserved, trailing padding trimmed. Rows +/// `0..screen_lines` address live output regardless of the display +/// offset; only display iteration follows the offset. /// /// Unlike [`crate::ansi::contents`], this scan view preserves glyphs in /// concealed (SGR 8) cells and orphaned wide halves. Harness matchers inspect /// stored grid text, not replay-equivalent display text. -fn push_row_glyphs(out: &mut String, row: &Row) { - for cell in row { +fn live_row_text_of(term: &Term, row: i32) -> String { + let mut text = String::new(); + for cell in &term.grid()[Line(row)] { if cell .flags .intersects(Flags::WIDE_CHAR_SPACER | Flags::LEADING_WIDE_CHAR_SPACER) { continue; } - out.push(if cell.c == '\t' { ' ' } else { cell.c }); + text.push(if cell.c == '\t' { ' ' } else { cell.c }); if let Some(zerowidth) = cell.zerowidth() { - out.extend(zerowidth.iter()); + text.extend(zerowidth.iter()); } } -} - -/// Plain text of one live-viewport row, trailing padding trimmed. Rows -/// `0..screen_lines` address live output regardless of the display -/// offset; only display iteration follows the offset. -fn live_row_text_of(term: &Term, row: i32) -> String { - let grid = term.grid(); - let line = &grid[Line(row)]; - let mut text = String::new(); - push_row_glyphs(&mut text, line); - while text.ends_with(' ') { - text.pop(); - } + text.truncate(text.trim_end_matches(' ').len()); text } @@ -626,9 +612,9 @@ const TITLE_STACK_SHADOW_MAX: usize = 4096; /// /// # Forwarding invariant /// -/// `Handler` methods default to no-ops, so every method must delegate to -/// `Term`. `golden::emulator_wrapper_matches_the_raw_backend_on_every_fixture` -/// compares wrapper and raw-backend replays to detect missing delegation. +/// `Handler` methods default to no-ops, so every method must delegate to `Term`. +/// The `golden::wrapper_*` tests detect missing delegation: each replays one raw fixture +/// through wrapper and raw backend, then compares styled bytes, text, cursor, and alt bit. /// `clipboard_store` is captured by this wrapper instead of delegated. /// /// # Synchronized updates @@ -721,9 +707,7 @@ impl Handler for ObservedTerm<'_> { fn input(&mut self, a0: char) { self.term.input(a0); // Discard a staged primary-screen title on printable output. - if self.alt.staged_title.is_some() { - self.alt.staged_title = None; - } + self.alt.staged_title = None; } delegate! { goto(a0: i32, a1: usize); @@ -816,12 +800,10 @@ impl Handler for ObservedTerm<'_> { } self.clipboard.stores.push((selector, text)); } - fn clipboard_load(&mut self, a0: u8, a1: &str) { + delegate! { // The configured terminal policy denies clipboard loads. - self.term.clipboard_load(a0, a1); - } - fn decaln(&mut self) { - self.term.decaln(); + clipboard_load(a0: u8, a1: &str); + decaln(); } fn push_title(&mut self) { self.term.push_title(); diff --git a/src/terminal/golden.rs b/src/terminal/golden.rs index 160cb75..9ee448d 100644 --- a/src/terminal/golden.rs +++ b/src/terminal/golden.rs @@ -576,14 +576,16 @@ fn semantic_dec_scrollregion_charset_translation() { assert_eq!(al.grid().cursor.point, Point::new(Line(39), Column(0))); } -/// Assert identical screen text, cursor state, and alternate-screen mode for -/// one fixture replayed through [`crate::emulator::Emulator`] and a raw `Term`. +/// Assert identical styled cells, screen text, cursor state, and alternate-screen mode +/// for one fixture replayed through [`crate::emulator::Emulator`] and a raw `Term`. fn assert_wrapper_matches(file: &str, bytes: &[u8]) { let al = alacritty(bytes); let mut emu = crate::testutil::corpus_emulator(); emu.process(bytes); - let (_, al_cursor, al_hidden) = ansi::formatted(&al); - let (_, emu_cursor, emu_hidden) = emu.formatted(); + let (al_bytes, al_cursor, al_hidden) = ansi::formatted(&al); + let (emu_bytes, emu_cursor, emu_hidden) = emu.formatted(); + // `assert!`, not `assert_eq!`: a failure must not dump two serialized screens. + assert!(emu_bytes == al_bytes, "{file}: styled cells"); assert_eq!(emu.contents(), ansi::contents(&al), "{file}: screen"); assert_eq!( (emu_cursor, emu_hidden), diff --git a/src/terminal/input.rs b/src/terminal/input.rs index 848674d..efad8ff 100644 --- a/src/terminal/input.rs +++ b/src/terminal/input.rs @@ -1,10 +1,10 @@ //! The input-direction mirror of `ansi`: protocol events in, VT byte -//! sequences out. Encoding only: no PTY writes and no `Task` state; -//! callers read the child's negotiated modes under their own locks and -//! pass them in. +//! sequences out. Encoding only: no PTY writes and no `Task` state. +//! `key_bytes` and `paste_bytes` take the child's negotiated modes as values; +//! `mouse_bytes` reads its four off the `Emulator` its caller holds locked. use crate::{ - emulator::Emulator, + emulator::{Emulator, MouseProtocolEncoding, MouseProtocolMode}, protocol::{Key, Mods, MouseKind}, }; @@ -53,7 +53,6 @@ pub fn paste_bytes(bracketed: bool, content: &[u8]) -> Vec { /// 1007 are both active. DECSET 1007 defaults on; see [`Emulator::alternate_scroll`]. /// Return `None` for unsupported actions. pub fn mouse_bytes(emu: &Emulator, kind: MouseKind, col: u16, row: u16) -> Option> { - use crate::emulator::{MouseProtocolEncoding, MouseProtocolMode}; let mode = emu.mouse_protocol_mode(); if mode != MouseProtocolMode::None { // Report presses, releases, and wheel events in every supported mode; report @@ -107,19 +106,13 @@ pub fn mouse_bytes(emu: &Emulator, kind: MouseKind, col: u16, row: u16) -> Optio }); } if emu.alternate_scroll() { - let up = match kind { - MouseKind::WheelUp => true, - MouseKind::WheelDown => false, + let key = match kind { + MouseKind::WheelUp => Key::Up, + MouseKind::WheelDown => Key::Down, // Only wheel actions map to alternate-scroll arrows. _ => return None, }; - let arrow: &[u8] = match (emu.application_cursor(), up) { - (true, true) => b"\x1bOA", - (true, false) => b"\x1bOB", - (false, true) => b"\x1b[A", - (false, false) => b"\x1b[B", - }; - return Some(arrow.repeat(3)); + return key_bytes(emu.application_cursor(), key, Mods::default()).map(|b| b.repeat(3)); } None } @@ -158,35 +151,45 @@ fn char_bytes(c: char, mods: Mods) -> Option> { Some(out) } +/// The letter-final shape of cursor keys, Home/End, and F1–F4: SS3 `letter` when +/// unmodified under `ss3`, CSI `letter` when unmodified otherwise, CSI `1;m letter` +/// when modified. +fn letter_bytes(ss3: bool, letter: char, m: Option) -> Vec { + match m { + None if ss3 => format!("\x1bO{letter}"), + None => format!("\x1b[{letter}"), + Some(m) => format!("\x1b[1;{m}{letter}"), + } + .into_bytes() +} + +/// The tilde-final shape of the navigation cluster and F5–F12: CSI `n~`, or CSI +/// `n;m~` when modified. +fn tilde_bytes(n: u8, m: Option) -> Vec { + match m { + None => format!("\x1b[{n}~"), + Some(m) => format!("\x1b[{n};{m}~"), + } + .into_bytes() +} + /// Encode F1–F4 as SS3 when unmodified and CSI when modified. F5–F12 use their CSI /// numeric forms. Emit nothing for numbers outside `1..=12`. fn f_bytes(n: u8, m: Option) -> Option> { - if let Some(letter) = match n { - 1 => Some('P'), - 2 => Some('Q'), - 3 => Some('R'), - 4 => Some('S'), - _ => None, - } { - return Some(match m { - None => format!("\x1bO{letter}").into_bytes(), - Some(m) => format!("\x1b[1;{m}{letter}").into_bytes(), - }); - } - let code = match n { - 5 => 15, - 6 => 17, - 7 => 18, - 8 => 19, - 9 => 20, - 10 => 21, - 11 => 23, - 12 => 24, + Some(match n { + 1 => letter_bytes(true, 'P', m), + 2 => letter_bytes(true, 'Q', m), + 3 => letter_bytes(true, 'R', m), + 4 => letter_bytes(true, 'S', m), + 5 => tilde_bytes(15, m), + 6 => tilde_bytes(17, m), + 7 => tilde_bytes(18, m), + 8 => tilde_bytes(19, m), + 9 => tilde_bytes(20, m), + 10 => tilde_bytes(21, m), + 11 => tilde_bytes(23, m), + 12 => tilde_bytes(24, m), _ => return None, - }; - Some(match m { - None => format!("\x1b[{code}~").into_bytes(), - Some(m) => format!("\x1b[{code};{m}~").into_bytes(), }) } @@ -201,44 +204,32 @@ fn meta_bytes(meta: bool, base: &[u8]) -> Vec { out } +/// Return xterm's modifier parameter `1 + shift + 2·alt + 4·ctrl`, or +/// `None` when no modifier is held. +fn mod_param(mods: Mods) -> Option { + let bits = mods.shift as u8 + 2 * mods.alt as u8 + 4 * mods.ctrl as u8; + (bits != 0).then_some(1 + bits) +} + /// Encode a key for the child. In application-cursor mode, use SS3 for unmodified /// cursor and Home/End keys and CSI for modified forms. Return `None` for unsupported /// key combinations. pub fn key_bytes(app_cursor: bool, code: Key, mods: Mods) -> Option> { - let m = mods.param(); + let m = mod_param(mods); match code { Key::Char(c) => char_bytes(c, mods), Key::F(n) => f_bytes(n, m), - Key::Up | Key::Down | Key::Left | Key::Right | Key::Home | Key::End => { - let letter = match code { - Key::Up => 'A', - Key::Down => 'B', - Key::Right => 'C', - Key::Left => 'D', - Key::Home => 'H', - Key::End => 'F', - _ => unreachable!(), - }; - Some(match m { - None if app_cursor => format!("\x1bO{letter}").into_bytes(), - None => format!("\x1b[{letter}").into_bytes(), - Some(m) => format!("\x1b[1;{m}{letter}").into_bytes(), - }) - } - Key::Insert | Key::Delete | Key::PageUp | Key::PageDown => { - // Navigation-cluster keys always use CSI `~`. - let n = match code { - Key::Insert => 2, - Key::Delete => 3, - Key::PageUp => 5, - Key::PageDown => 6, - _ => unreachable!(), - }; - Some(match m { - None => format!("\x1b[{n}~").into_bytes(), - Some(m) => format!("\x1b[{n};{m}~").into_bytes(), - }) - } + Key::Up => Some(letter_bytes(app_cursor, 'A', m)), + Key::Down => Some(letter_bytes(app_cursor, 'B', m)), + Key::Right => Some(letter_bytes(app_cursor, 'C', m)), + Key::Left => Some(letter_bytes(app_cursor, 'D', m)), + Key::Home => Some(letter_bytes(app_cursor, 'H', m)), + Key::End => Some(letter_bytes(app_cursor, 'F', m)), + // Navigation-cluster keys always use CSI `~`. + Key::Insert => Some(tilde_bytes(2, m)), + Key::Delete => Some(tilde_bytes(3, m)), + Key::PageUp => Some(tilde_bytes(5, m)), + Key::PageDown => Some(tilde_bytes(6, m)), // Encode Enter as ESC CR with Shift or Alt; keep plain CR with Control. Key::Enter => Some(meta_bytes(mods.shift || mods.alt, b"\x0d")), // Prefix Tab with ESC for Alt; keep HT with Control or Shift. diff --git a/tests/corpus/README.md b/tests/corpus/README.md index d678941..edfcf05 100644 --- a/tests/corpus/README.md +++ b/tests/corpus/README.md @@ -132,9 +132,10 @@ Verify display and parser behavior against these fixtures: - `codex_resume`, `wide_emoji`, `dec_scrollregion`, and `topregion_scroll` for parser semantics: scrollback retention, intensity stacking, charset translation, and VS16 width. -- `claude_resume`, `codex_resume`, and `grok_resume` for comparison of the - emulator's final display, cursor, and alternate-screen state with the terminal backend. +- Every raw capture listed above for comparison of the emulator's final styled + cells, text, cursor, and alternate-screen state with the terminal backend, + and for a serialize-and-replay round trip. See `src/terminal/golden.rs` for absolute display and parser expectations and -terminal-backend comparisons, and `src/harness/summary_tests.rs` for -preview-fixture expectations. +terminal-backend comparisons, `src/terminal/ansi.rs` for the round trip, and +`src/harness/summary_tests.rs` for preview-fixture expectations. From b57f6010cfce6e5b2d6799dbcd52fb923c0e841c Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 14:23:07 -0700 Subject: [PATCH 02/11] refactor(ui): drop a mirrored enum, an overlay's copied fields, and a doubled center --- docs/README.md | 18 +++---- src/app_readme_tests.rs | 2 +- src/ui.rs | 116 ++++++++++++++-------------------------- 3 files changed, 51 insertions(+), 85 deletions(-) diff --git a/docs/README.md b/docs/README.md index c4a6517..37a3bdf 100644 --- a/docs/README.md +++ b/docs/README.md @@ -72,14 +72,14 @@ Use the hints for common dashboard actions; press `?` for the expanded key refer fleetcom 2 running · 0 idle · 0 done by state · dir · custom Running - ✻ cargo watch -x test test result: ok. 42 passed 9s - ✻ npm run dev VITE v5.0 ready in 312 ms 4s + ✻ cargo watch -x test test result: ok. 42 passed 9s + ✻ npm run dev VITE v5.0 ready in 312 ms 4s ❯ n run · @ dir · / find · s sort ↑↓ select · enter attach · space peek · ? controls ``` -Each row is `glyph · tag · command · latest output · age`. Age is measured from launch while running, last output once idle, or exit once completed. Press `Space` to peek at the selected task's live screen in a read-only box over the dashboard: +Each row is `marks · glyph · command · latest output · age`. Age is measured from launch while running, last output once idle, or exit once completed. Press `Space` to peek at the selected task's live screen in a read-only box over the dashboard: ```text ┌─ cargo watch -x test ───────────────────────────────┐ @@ -101,10 +101,10 @@ Press `Ctrl-\` to return to the dashboard. Press `m` to tag the selected task "i fleetcom 2 running · 0 idle · 0 done by state · dir · custom In use - ✻ ◆cargo watch -x test test result: ok. 42 passed 1m +◆ ✻ cargo watch -x test test result: ok. 42 passed 1m Running - ✻ npm run dev VITE v5.0 ready in 312 ms 1m + ✻ npm run dev VITE v5.0 ready in 312 ms 1m ``` Press `s` to cycle through state, directory, and custom grouping. The active mode is bold in the header. In custom mode, press `g` to assign the selected task to a named group. Named sections are sorted without regard to case, so `API` and `api` are adjacent. They remain separate because group identity is case-sensitive. Unassigned is listed last when at least one task has no group: @@ -113,10 +113,10 @@ Press `s` to cycle through state, directory, and custom grouping. The active mod fleetcom 2 running · 0 idle · 0 done by state · dir · custom api - ✻ ◆cargo watch -x test test result: ok. 42 passed 2m +◆ ✻ cargo watch -x test test result: ok. 42 passed 2m Unassigned - ✻ npm run dev VITE v5.0 ready in 312 ms 2m + ✻ npm run dev VITE v5.0 ready in 312 ms 2m ``` In custom mode, a new command is assigned the selected task's group, shown in the spawn prompt (`❯ api ▸ cargo run`). Group assignments are stored with task state and preserved across detach and rerun. See the [command reference](commands.md#the-g-group-picker) for picker mechanics. @@ -127,10 +127,10 @@ Press `R` to rename the selected task, starting with its current name. Press `En fleetcom 2 running · 0 idle · 0 done by state · dir · custom api - ✻ ◆api tests test result: ok. 42 passed 3m +◆ ✻ api tests test result: ok. 42 passed 3m Unassigned - ✻ npm run dev VITE v5.0 ready in 312 ms 3m + ✻ npm run dev VITE v5.0 ready in 312 ms 3m ``` In the attached status bar, both are displayed: `[attached] api tests · cargo watch -x test`. Names are stored in daemon state, preserved across detach and rerun, and included in saved [sessions](sessions.md). diff --git a/src/app_readme_tests.rs b/src/app_readme_tests.rs index 0447db0..fc9770d 100644 --- a/src/app_readme_tests.rs +++ b/src/app_readme_tests.rs @@ -6,7 +6,7 @@ use super::*; /// Fixture dimensions. Thirty rows fit every section; 107 columns produce a -/// 71-column preview cell and an 80-column peek box. +/// 72-column preview cell and an 80-column peek box. const FIXTURE_ROWS: u16 = 30; const FIXTURE_COLS: u16 = 107; diff --git a/src/ui.rs b/src/ui.rs index 6f3a3e4..e868e46 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -87,10 +87,10 @@ fn dim(out: &mut impl Write, y: u16, s: &str, cols: usize) -> io::Result<()> { ) } -/// Paint a full-width highlight line: selection and focused-field styling. -fn rev(out: &mut impl Write, y: u16, s: &str, cols: usize, focused: bool) -> io::Result<()> { +/// Paint a full-width highlight line: reverse video while the host terminal has focus, a dark-grey +/// background while it does not, so an unfocused terminal shows no full-strength selection. +fn highlight(out: &mut impl Write, y: u16, s: &str, cols: usize, focused: bool) -> io::Result<()> { queue!(out, MoveTo(0, y))?; - // When the host terminal is unfocused, the highlight is muted to a dark-grey if focused { queue!(out, SetAttribute(Attribute::Reverse))?; } else { @@ -149,7 +149,7 @@ fn render_dashboard(out: &mut impl Write, app: &App) -> io::Result<()> { let display = pad(&plain, cols); let mut chars = display.chars(); queue!(out, MoveTo(0, 0))?; - for (text, intensity) in &segs { + for (text, attr) in &segs { let n = text.chars().count(); if n == 0 { continue; @@ -159,13 +159,9 @@ fn render_dashboard(out: &mut impl Write, app: &App) -> io::Result<()> { break; // ran off the truncated end; attributes are already reset } // Reset between runs so Bold and Dim are not stacked. - let attr = match intensity { - Intensity::Bold => Attribute::Bold, - Intensity::Dim => Attribute::Dim, - }; queue!( out, - SetAttribute(attr), + SetAttribute(*attr), Print(piece), SetAttribute(Attribute::Reset) )?; @@ -184,7 +180,7 @@ fn render_dashboard(out: &mut impl Write, app: &App) -> io::Result<()> { Row::Task(ti) => { let v = &app.views[*ti]; if app.selected_id == Some(v.id) { - rev(out, y, &task_row(v, cols), cols, app.terminal_focused)?; + highlight(out, y, &task_row(v, cols), cols, app.terminal_focused)?; } else if v.preview.source == PreviewSource::Marker { // The marker is a placeholder, not output: dim the // preview cell so it reads as metadata. @@ -281,31 +277,23 @@ fn prompt_line(dir: Option<&str>, group: Option<&str>, input: &str) -> String { line } -/// Header intensity for one output run. Emit Bold or Dim per run, followed by a reset -/// to avoid stacking attributes. -#[derive(Clone, Copy, PartialEq, Eq, Debug)] -enum Intensity { - Bold, - Dim, -} - -/// Split the header into styled runs, emphasizing only the active mode. -fn header_segments(prefix: &str, active: GroupMode, suffix: &str) -> Vec<(String, Intensity)> { +/// Split the header into Bold and Dim runs, emphasizing only the active mode. +fn header_segments(prefix: &str, active: GroupMode, suffix: &str) -> Vec<(String, Attribute)> { // Keep the displayed order aligned with the grouping cycle. const STRIP: [GroupMode; 3] = [GroupMode::State, GroupMode::Dir, GroupMode::Custom]; - let mut segs = vec![(prefix.to_string(), Intensity::Bold)]; + let mut segs = vec![(prefix.to_string(), Attribute::Bold)]; for (i, m) in STRIP.iter().enumerate() { if i > 0 { - segs.push((" · ".to_string(), Intensity::Dim)); + segs.push((" · ".to_string(), Attribute::Dim)); } - let intensity = if *m == active { - Intensity::Bold + let attr = if *m == active { + Attribute::Bold } else { - Intensity::Dim + Attribute::Dim }; - segs.push((m.label().to_string(), intensity)); + segs.push((m.label().to_string(), attr)); } - segs.push((suffix.to_string(), Intensity::Bold)); + segs.push((suffix.to_string(), Attribute::Bold)); segs } @@ -413,9 +401,6 @@ fn top_border(label: &str, inner_w: usize) -> String { /// Centered overlay with a labeled border, padded body, and dim footer. struct Overlay<'a> { - /// Terminal dimensions: columns, then rows. - cols: usize, - rows: usize, /// Box dimensions including borders: columns, then rows. bw: usize, bh: usize, @@ -428,10 +413,10 @@ struct Overlay<'a> { } /// Paint a centered overlay, then overwrite the bottom border with its footer. -fn render_overlay(out: &mut impl Write, o: &Overlay) -> io::Result<()> { +fn render_overlay(out: &mut impl Write, app: &App, o: &Overlay) -> io::Result<()> { // Saturate coordinates at the origin for a box larger than the terminal. - let x0 = o.cols.saturating_sub(o.bw) / 2; - let y0 = o.rows.saturating_sub(o.bh) / 2; + let x0 = (app.cols as usize).saturating_sub(o.bw) / 2; + let y0 = (app.rows as usize).saturating_sub(o.bh) / 2; let (inner_w, inner_h) = (o.bw.saturating_sub(2), o.bh.saturating_sub(2)); queue!( out, @@ -485,9 +470,8 @@ fn render_peek(out: &mut impl Write, app: &App) -> io::Result<()> { ); render_overlay( out, + app, &Overlay { - cols, - rows, bw, bh, label: display_label(v), @@ -497,9 +481,10 @@ fn render_peek(out: &mut impl Write, app: &App) -> io::Result<()> { ) } -/// The peek body is the last `height` lines of the selected task's screen, or all lines -/// when the task is in alternate-screen mode. When the screen is shorter than `height`, -/// all lines are returned. +/// The peek body: at most `height` lines, cropped from the bottom of a window. On the alternate +/// screen the window is the whole grid: a partial repaint leaves real blank canvas rows, and +/// dropping them would shift the view. On the primary screen it ends at the last non-blank row: +/// trailing blanks are grid padding, so short output renders from its first row. fn peek_window(lines: &[String], height: usize, alt_screen: bool) -> &[String] { // `contents()` trims trailing padding, so a blank row is exactly empty. let end = if alt_screen { @@ -665,9 +650,8 @@ fn render_controls(out: &mut impl Write, app: &App) -> io::Result<()> { }; render_overlay( out, + app, &Overlay { - cols, - rows, bw, bh, label: "controls", @@ -680,7 +664,7 @@ fn render_controls(out: &mut impl Write, app: &App) -> io::Result<()> { /// The varying content of a bottom-panel picker; rendered within the shared skeleton in /// `render_panel`. struct Panel<'a> { - /// Header line, painted reverse-video as the focused field. + /// Header line, painted by `highlight` as the focused field. header: String, /// Preformatted row labels; indented and marked with `▸` in the skeleton. labels: &'a [String], @@ -710,7 +694,7 @@ fn render_panel(out: &mut impl Write, app: &App, p: &Panel) -> io::Result<()> { let panel_h = (body + 2) as u16; let top = rows.saturating_sub(panel_h).max(2); - rev(out, top, &p.header, cols, app.terminal_focused)?; + highlight(out, top, &p.header, cols, app.terminal_focused)?; if total == 0 { if let Some(msg) = p.empty { @@ -723,7 +707,7 @@ fn render_panel(out: &mut impl Write, app: &App, p: &Panel) -> io::Result<()> { let marker = if idx == p.sel { "▸ " } else { " " }; let line = format!(" {marker}{}", p.labels[idx]); if idx == p.sel { - rev(out, y, &line, cols, app.terminal_focused)?; + highlight(out, y, &line, cols, app.terminal_focused)?; } else { put(out, y, &line, cols)?; } @@ -748,8 +732,8 @@ fn render_panel(out: &mut impl Write, app: &App, p: &Panel) -> io::Result<()> { } } -/// The `@` picker: a bottom panel over the dashboard. A typed-path input plus -/// the matching subdirectories, `dir_sel` highlighted. +/// The `@` picker: a bottom panel over the dashboard. A typed-path input plus the matching +/// subdirectories, `dir_sel` highlighted. The resolved path is row 0, so the list is never empty. fn render_pickdir(out: &mut impl Write, app: &App) -> io::Result<()> { let labels: Vec = app .dir_candidates @@ -778,7 +762,7 @@ fn render_pickdir(out: &mut impl Write, app: &App) -> io::Result<()> { sel: app.dir_sel, max_rows: 8, hint: format!("{action} · ↑↓ pick · esc"), - empty: Some(" (no matching directories)"), + empty: None, cursor: Some(cx), }, ) @@ -912,15 +896,10 @@ fn render_session_picker(out: &mut impl Write, app: &App) -> io::Result<()> { ) } -/// Center `s` in `width` columns (a full-width string, so it overwrites the row). +/// Indent `s` to center it in `width` columns. The caller's `put`/`dim` clips +/// and pads the result to the row. fn center(s: &str, width: usize) -> String { - let len = s.width(); - if len >= width { - return truncate(s, width); - } - let mut out = " ".repeat((width - len) / 2); - out.push_str(s); - pad(&out, width) + format!("{}{s}", " ".repeat(width.saturating_sub(s.width()) / 2)) } /// Full-screen reconnect prompt shown after a daemon connection drops. @@ -979,7 +958,7 @@ fn render_attached(out: &mut impl Write, app: &App) -> io::Result<()> { chord.as_deref(), cols, ); - rev( + highlight( out, app.rows.saturating_sub(1), &bar, @@ -1396,13 +1375,6 @@ mod tests { assert_eq!(labels.len(), distinct, "a group must be one contiguous run"); } - /// Add one heading row per group to the flat entry rows in grouped form. - #[test] - fn control_forms_shrink_before_they_clip() { - assert_eq!(flat_rows(), CONTROLS.len().div_ceil(2)); - assert_eq!(grouped_rows(), flat_rows() + control_groups().len()); - } - /// Overlay borders remain column-exact for wide and overlong labels. #[test] fn top_border_fills_to_inner_width() { @@ -1584,13 +1556,7 @@ mod tests { assert_eq!(cells[..2], want, "{row:?}"); assert_eq!(cells[2], '✻', "{row:?}"); // Compare all columns after the gutter with the unmarked row. - let rest = |r: &str| -> String { - r.chars() - .enumerate() - .filter(|(i, _)| *i > 1) - .map(|(_, c)| c) - .collect() - }; + let rest = |r: &str| r.chars().skip(2).collect::(); assert_eq!(rest(&row), rest(&plain)); } } @@ -1632,18 +1598,18 @@ mod tests { let plain: String = segs.iter().map(|(t, _)| t.as_str()).collect(); assert_eq!(plain, "by state · dir · custom · tail"); - assert_eq!(segs.first().unwrap(), &("by ".to_string(), Intensity::Bold)); + assert_eq!(segs.first().unwrap(), &("by ".to_string(), Attribute::Bold)); assert_eq!( segs.last().unwrap(), - &(" · tail".to_string(), Intensity::Bold) + &(" · tail".to_string(), Attribute::Bold) ); - for (text, intensity) in &segs[1..segs.len() - 1] { + for (text, attr) in &segs[1..segs.len() - 1] { let expect = if text == active.label() { - Intensity::Bold + Attribute::Bold } else { - Intensity::Dim + Attribute::Dim }; - assert_eq!(*intensity, expect, "run {text:?} with active {active:?}"); + assert_eq!(*attr, expect, "run {text:?} with active {active:?}"); } } } From 7401f9ed4995e320ac18b6c946f6d485150b9231 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 14:24:59 -0700 Subject: [PATCH 03/11] refactor(harness): parse the frame prefix once, move select beside detect --- docs/agent-resume.md | 4 +- src/harness/assets.rs | 20 +++----- src/harness/codex.rs | 6 +-- src/harness/mod.rs | 35 ++++++++------ src/harness/summary.rs | 94 ++++++++++++++---------------------- src/harness/summary_tests.rs | 5 +- src/supervisor.rs | 2 +- src/task.rs | 2 +- src/task_tests.rs | 2 +- 9 files changed, 72 insertions(+), 98 deletions(-) diff --git a/docs/agent-resume.md b/docs/agent-resume.md index 4c9dfb9..b9947c4 100644 --- a/docs/agent-resume.md +++ b/docs/agent-resume.md @@ -68,7 +68,7 @@ You cannot choose a Codex ID at launch. A notify override is injected into both After each turn, the notifier writes the `agent-turn-complete` JSON argument to `FLEETCOM_CAPTURE_FILE`, and the harness reads `thread-id` from it. This captures an in-TUI session change after a turn completes in the resumed conversation. -Replacing a configured notifier would change user behavior, so the harness reads bare top-level keys in `$CODEX_HOME/config.toml` until the first table header. If it finds a one-line `notify` array of non-empty basic strings, it chains that notifier after the capture write. `FLEETCOM_NOTIFY_CHAIN` carries its argv joined by newlines; the capture script appends the notification payload before invoking it. An absent setting or empty array means capture runs alone. This encoding cannot transport empty arguments, newlines, or NUL, so those values disable injection. +Replacing a configured notifier would change user behavior, so the harness reads bare top-level keys in `$CODEX_HOME/config.toml` until the first table header. If it finds a one-line `notify` array of non-empty basic strings, it chains that notifier after the capture write. `FLEETCOM_NOTIFY_CHAIN` carries its argv joined by newlines; the capture script appends the notification payload before invoking it. An absent setting or empty array means capture runs alone. This encoding cannot transport empty arguments, newlines, or NUL, so those values disable injection. Injection is also disabled, and no ID is captured, when `config.toml` exists but cannot be read, or when a line above the first table header is not a bare key assigned a single-line value the reader recognizes. Live validation on 2026-09-28 used the official macOS arm64 Codex 0.157.1 and 0.158.0 executables with isolated homes and a scripted loopback Responses server. In both releases, the real capture script preserved the notification JSON and passed it to the configured notifier. Saving and loading through foreground fleetcom preserved the thread ID and reopened its history. These runs used `--no-daemon`; separate default-launch probes also captured and saved the ID, but their fresh homes had no managed Codex daemon installed or running. Shared-daemon behavior and real-account behavior remain unverified. @@ -121,7 +121,7 @@ Every captured value eventually enters a shell command, so validation accepts on ## Extending capture -Implement the `Harness` trait in [`src/harness/mod.rs`](../src/harness/mod.rs) for each tool. Separate detection, evidence collection, and command construction through these methods: +Implement the `Harness` trait in [`src/harness/mod.rs`](../src/harness/mod.rs) for each tool. Register it in that file's `AGENTS` table, the only list detection consults; each entry also requires a `SummaryAdapter` for the dashboard preview, as implemented in [`src/harness/summary.rs`](../src/harness/summary.rs). Separate detection, evidence collection, and command construction through these methods: - `shape`: supply the program word and resume selector. Accepted and canonical forms are derived from that pair by the default `detect` and `resume_command` implementations. - `instrument`: return spawn-time arguments, environment entries, and an optional pinned ID. diff --git a/src/harness/assets.rs b/src/harness/assets.rs index 4d9bd42..e271806 100644 --- a/src/harness/assets.rs +++ b/src/harness/assets.rs @@ -106,14 +106,11 @@ fn claude_settings_json() -> String { .dump() } -/// Capture-root resolver. An explicit override is what the supervisor passes -/// when `FLEETCOM_RUNTIME_DIR` is set; otherwise `dirs::runtime_dir()/fleetcom`, -/// then `dirs::cache_dir()/fleetcom/run`. Those fallbacks are not -/// `daemon::resolve_runtime_dir`. -pub fn runtime_root(override_dir: Option<&Path>) -> Option { - if let Some(dir) = override_dir { - return Some(dir.to_path_buf()); - } +/// Platform capture root: `dirs::runtime_dir()/fleetcom`, then +/// `dirs::cache_dir()/fleetcom/run`. Those fallbacks are not +/// `daemon::resolve_runtime_dir`. The supervisor calls this only when the +/// launch context carries no `FLEETCOM_RUNTIME_DIR`. +pub fn runtime_root() -> Option { if let Some(run) = dirs::runtime_dir() { return Some(run.join("fleetcom")); } @@ -275,11 +272,9 @@ mod tests { } #[test] - fn runtime_root_prefers_the_override_verbatim() { - let dir = Path::new("/custom/run dir"); - assert_eq!(runtime_root(Some(dir)).as_deref(), Some(dir)); + fn runtime_root_resolves_under_fleetcom() { // Every supported platform resolves a fallback under `fleetcom`. - let fallback = runtime_root(None).expect("platform dirs must resolve"); + let fallback = runtime_root().expect("platform dirs must resolve"); assert!(fallback.components().any(|c| c.as_os_str() == "fleetcom")); } @@ -329,6 +324,7 @@ mod tests { fs::read_to_string(&second.omp_capture).unwrap(), OMP_CAPTURE_MODULE ); + assert!(OMP_CAPTURE_MODULE.contains(CAPTURE_ENV)); assert_eq!( fs::read_to_string(&first.claude_settings).unwrap(), "garbage", diff --git a/src/harness/codex.rs b/src/harness/codex.rs index d97a2a2..55dca49 100644 --- a/src/harness/codex.rs +++ b/src/harness/codex.rs @@ -137,7 +137,7 @@ fn config_notify_route(home: Option<&Path>) -> NotifyRoute { } /// Recognize complete single-line values without interpreting unrelated settings. -/// Multiline strings, arrays, and inline tables are outside the reader's scope. +/// Arrays hold basic strings only; multiline values and inline tables are out of scope. fn complete_value(value: &str) -> bool { if value.starts_with("\"\"\"") || value.starts_with("'''") { return false; @@ -573,9 +573,5 @@ mod tests { config_notify_route(Some(&home)), NotifyRoute::Chain(vec!["/base/hook".to_string()]) ); - - // Duplicate top-level assignments are invalid TOML. - fs::write(&cfg, "notify = [\"/a\"]\nnotify = [\"/b\"]\n").unwrap(); - assert_eq!(config_notify_route(Some(&home)), NotifyRoute::Opaque); } } diff --git a/src/harness/mod.rs b/src/harness/mod.rs index f71c671..7bcdcfe 100644 --- a/src/harness/mod.rs +++ b/src/harness/mod.rs @@ -161,6 +161,19 @@ pub fn detect(cmd: &str) -> Option<(&'static dyn Harness, Invocation)> { .find_map(|a| a.harness.detect(cmd).map(|inv| (a.harness, inv))) } +/// Select a summary adapter by the basename of the command's first +/// whitespace-separated word. Arguments are accepted; do not select an adapter +/// for environment prefixes or compound shell commands. Selection is +/// independent of session-capture instrumentation. +pub fn select(command: &str) -> Option<&'static dyn crate::preview::SummaryAdapter> { + let first = command.split_whitespace().next()?; + let name = Path::new(first).file_name()?.to_str()?; + AGENTS + .iter() + .find(|a| a.harness.shape().0 == name) + .map(|a| a.summary) +} + /// Classification of an accepted agent-CLI command. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Invocation { @@ -552,21 +565,13 @@ mod tests { #[test] fn registry_detect_routes_to_the_matching_harness() { - // Assert the literal count to detect additions to `AGENTS` missing from this - // routing test. - assert_eq!(AGENTS.len(), 4, "route the new harness's command here"); - let (h, inv) = detect("claude").unwrap(); - assert_eq!(h.shape().0, "claude"); - assert_eq!(inv, Invocation::Bare); - let (h, inv) = detect(&format!("codex resume {ID}")).unwrap(); - assert_eq!(h.shape().0, "codex"); - assert_eq!(inv, Invocation::Resume(ID.into())); - let (h, inv) = detect("grok").unwrap(); - assert_eq!(h.shape().0, "grok"); - assert_eq!(inv, Invocation::Bare); - let (h, inv) = detect("omp").unwrap(); - assert_eq!(h.shape().0, "omp"); - assert_eq!(inv, Invocation::Bare); + for a in AGENTS { + let (prog, sel) = a.harness.shape(); + let (h, inv) = detect(prog).unwrap(); + assert_eq!((h.shape().0, inv), (prog, Invocation::Bare)); + let (h, inv) = detect(&format!("{prog} {sel} {ID}")).unwrap(); + assert_eq!((h.shape().0, inv), (prog, Invocation::Resume(ID.into()))); + } assert!(detect("vim").is_none()); assert!(detect("").is_none()); } diff --git a/src/harness/summary.rs b/src/harness/summary.rs index 9004bad..625c9d0 100644 --- a/src/harness/summary.rs +++ b/src/harness/summary.rs @@ -32,57 +32,30 @@ //! selector. Supported screen structures are recorded in the corpus fixtures in //! `tests/corpus`. -use std::path::Path; - use crate::preview::SummaryAdapter; -/// Select an adapter by the basename of the command's first whitespace-separated word. -/// Arguments are accepted; do not select an adapter for environment prefixes or -/// compound shell commands. Selection is independent of session-capture -/// instrumentation. -pub fn select(command: &str) -> Option<&'static dyn SummaryAdapter> { - let first = command.split_whitespace().next()?; - let name = Path::new(first).file_name()?.to_str()?; - super::AGENTS - .iter() - .find(|a| a.harness.shape().0 == name) - .map(|a| a.summary) -} - /// Preview text shared by approval-menu matchers and Claude's registry /// permission prompt. pub(crate) const AWAITING_APPROVAL: &str = "awaiting approval"; -/// Whether `row` is a full-width horizontal rule: nothing but `─`, long -/// enough that box borders and inline list rules never qualify. claude's -/// input box is fenced by two such rows. -fn is_rule_row(row: &str) -> bool { - let mut n = 0usize; - for c in row.trim().chars() { - if c != '─' { - return false; - } - n += 1; - } - n >= 40 -} - /// Whether `c` is a Unicode Braille Patterns code point used as a spinner /// frame by the supported CLIs. fn braille_frame(c: char) -> bool { ('\u{2800}'..='\u{28FF}').contains(&c) } +/// The text after a leading frame char accepted by `is_frame` and one space. +fn after_frame(s: &str, is_frame: impl Fn(char) -> bool) -> Option<&str> { + let mut chars = s.chars(); + (is_frame(chars.next()?) && chars.next()? == ' ').then_some(chars.as_str()) +} + /// The status phrase of a spinner row: a frame char accepted by `is_frame`, /// a space, then text through the first `…` inclusive. The phrase must open /// alphanumeric; past that it is task-derived and unconstrained. Trailing /// text is left for the caller to interpret. fn spinner_text(row: &str, is_frame: impl Fn(char) -> bool) -> Option { - let mut chars = row.chars(); - if !is_frame(chars.next()?) || chars.next()? != ' ' { - return None; - } - let rest = chars.as_str(); + let rest = after_frame(row, is_frame)?; let text = &rest[..rest.find('…')? + '…'.len_utf8()]; text.chars() .next()? @@ -137,15 +110,28 @@ impl SummaryAdapter for ClaudeSummary { /// Strip a recognized claude spinner, braille, or quadrant-circle frame from a /// nonempty title. Return `None` for other title shapes. fn normalize_title(&self, title: &str) -> Option { - let mut chars = title.chars(); - let frame = chars.next()?; - let framed = CLAUDE_SPINNER.contains(&frame) - || braille_frame(frame) - || ('\u{25D0}'..='\u{25D3}').contains(&frame); + let rest = after_frame(title, |c| { + CLAUDE_SPINNER.contains(&c) + || braille_frame(c) + || ('\u{25D0}'..='\u{25D3}').contains(&c) + })?; // An empty payload cannot produce a usable preview. - (framed && chars.next()? == ' ' && !chars.as_str().is_empty()) - .then(|| chars.as_str().to_string()) + (!rest.is_empty()).then(|| rest.to_string()) + } +} + +/// Whether `row` is a full-width horizontal rule: nothing but `─`, long +/// enough that box borders and inline list rules never qualify. claude's +/// input box is fenced by two such rows. +fn is_rule_row(row: &str) -> bool { + let mut n = 0usize; + for c in row.trim().chars() { + if c != '─' { + return false; + } + n += 1; } + n >= 40 } /// Index of the input box's top separator. The bottom-most full-width rule @@ -205,11 +191,7 @@ fn claude_spinner_status(rows: &[String], top: usize) -> Option<(String, &'stati /// Match a spinner-framed `Waiting for {digits} {subject} to finish` row and /// return its text verbatim. The subject must contain one to three words. fn claude_waiting_text(row: &str) -> Option { - let mut chars = row.chars(); - if !CLAUDE_SPINNER.contains(&chars.next()?) || chars.next()? != ' ' { - return None; - } - let text = chars.as_str(); + let text = after_frame(row, |c| CLAUDE_SPINNER.contains(&c))?; let rest = text.strip_prefix("Waiting for ")?; let digits = rest.chars().take_while(char::is_ascii_digit).count(); if digits == 0 { @@ -348,11 +330,11 @@ const CODEX_EFFORT: &[&str] = &[ /// depth, so counting them would push the status row out of reach. const CODEX_STATUS_WINDOW: usize = 10; -/// codex (inline UI, primary screen). The pin is its composer: the bottom-most column-0 -/// prompt-glyph row that is not a modal selector; status rows sit above it, and -/// scrollback beyond the first foreign row is out of bounds. Check for the approval -/// modal first, with the composer absent. The status line or indented hint rows may -/// appear below the composer. +/// codex (inline UI on the primary screen; approval modals also paint on the alternate +/// screen). The pin is its composer: the bottom-most column-0 prompt-glyph row that is +/// not a modal selector; status rows sit above it, and scrollback beyond the first +/// foreign row is out of bounds. Check for the approval modal first, with the composer +/// absent. The status line or indented hint rows may appear below the composer. pub struct CodexSummary; impl SummaryAdapter for CodexSummary { @@ -374,9 +356,7 @@ impl SummaryAdapter for CodexSummary { if let Some(rest) = title.strip_prefix("[ . ] ") { return Some(format!("[ ! ] {rest}")); } - let mut chars = title.chars(); - let frame = chars.next()?; - (braille_frame(frame) && chars.next()? == ' ').then(|| format!("⠋ {}", chars.as_str())) + after_frame(title, braille_frame).map(|rest| format!("⠋ {rest}")) } } @@ -852,11 +832,7 @@ fn omp_input_box(rows: &[String]) -> Option { /// Reject a wrapped row with its hint on the next line: do not extract half a phrase. fn omp_spinner_status(rows: &[String], top: usize) -> Option<(String, &'static str)> { let probe = rows[..top].iter().rev().find(|r| !r.is_empty())?; - let mut chars = probe.trim_start().chars(); - if !braille_frame(chars.next()?) || chars.next()? != ' ' { - return None; - } - let rest = chars.as_str(); + let rest = after_frame(probe.trim_start(), braille_frame)?; let text = OMP_HINTS .iter() .find_map(|h| rest.strip_suffix(h))? diff --git a/src/harness/summary_tests.rs b/src/harness/summary_tests.rs index c77e3a7..1d062e7 100644 --- a/src/harness/summary_tests.rs +++ b/src/harness/summary_tests.rs @@ -3,6 +3,7 @@ use std::time::Instant; use super::*; use crate::{ emulator::Emulator, + harness::select, preview::{MARKER, PreviewState, SummaryAdapter}, protocol::PreviewSource, }; @@ -1471,7 +1472,7 @@ fn omp_ascii_box_glyphs_do_not_anchor() { /// Each offset is a cumulative PTY byte checkpoint, not a standalone repaint. fn codex_0158_checkpoint(bytes: &[u8], offset: usize, alternate: bool) -> Vec { - let mut emu = Emulator::new(40, 120, 2000); + let mut emu = crate::testutil::corpus_emulator(); emu.process(&bytes[..offset]); assert_eq!(emu.alternate_screen(), alternate); emu.live_rows() @@ -2015,7 +2016,7 @@ fn corpus_non_agent_tuis_keep_their_tiers() { .windows(8) .rposition(|w| w == b"\x1b[?1049l") .expect("fixture exits the alt screen"); - let mut emu = Emulator::new(40, 120, 2000); + let mut emu = crate::testutil::corpus_emulator(); emu.process(&bytes[..cut]); assert!(emu.alternate_screen(), "{name}: alt screen active at cut"); let mut st = PreviewState::new(); diff --git a/src/supervisor.rs b/src/supervisor.rs index 042ec8c..8ae095a 100644 --- a/src/supervisor.rs +++ b/src/supervisor.rs @@ -725,7 +725,7 @@ impl Supervisor { let root = self .launch_env_path(path::FLEETCOM_RUNTIME_DIR) .or_else(|| { - assets::runtime_root(None).map(|base| { + assets::runtime_root().map(|base| { let key = self .sessions_root() .map(PathBuf::into_os_string) diff --git a/src/task.rs b/src/task.rs index 91e416b..95120b6 100644 --- a/src/task.rs +++ b/src/task.rs @@ -335,7 +335,7 @@ impl Task { name: None, harness: None, harness_home: None, - summary_adapter: crate::harness::summary::select(command), + summary_adapter: crate::harness::select(command), run: 0, resume_id: None, capture_file: None, diff --git a/src/task_tests.rs b/src/task_tests.rs index 774821a..f147717 100644 --- a/src/task_tests.rs +++ b/src/task_tests.rs @@ -541,7 +541,7 @@ fn summary_adapter_anchors_live_and_freezes_completion_at_exit() { ); let mut t = Task::spawn(42, &cmd, &cmd, &here(), 24, 80, 2000, &sh_env(), no_waker()).unwrap(); assert!(t.summary_adapter.is_none(), "printf selects nothing"); - t.summary_adapter = crate::harness::summary::select("codex"); + t.summary_adapter = crate::harness::select("codex"); assert!(t.summary_adapter.is_some()); let mut live = t.resolve_preview(Instant::now()); From fd3bc214dc1e1e90a6caf1a6c3976c98fb10552f Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 14:35:00 -0700 Subject: [PATCH 04/11] refactor(client): fold hand-encoded state, route n through the spawn opener --- src/app.rs | 169 +++++++++++++++++------------------------------ src/app_tests.rs | 108 +++++++++++++++--------------- src/ui.rs | 2 +- 3 files changed, 115 insertions(+), 164 deletions(-) diff --git a/src/app.rs b/src/app.rs index 71ee4a3..e3b8746 100644 --- a/src/app.rs +++ b/src/app.rs @@ -77,7 +77,7 @@ enum NoticeLevel { Info, } -#[derive(Clone, Copy, PartialEq, Eq)] +#[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum Mode { Dashboard, /// Typing a command to spawn in `spawn_cwd` (bottom command line focused). @@ -86,9 +86,7 @@ pub enum Mode { PickDir, /// Live group picker (the `g` flow). Its target remains fixed if a snapshot /// reorders the dashboard selection. - PickGroup { - target: u64, - }, + PickGroup(u64), /// Find palette for selecting a task from filtered results. Find, /// Typing a name to save the current tasks as a session. @@ -290,11 +288,10 @@ pub struct App { /// Set by an external SIGTERM/SIGHUP/SIGINT; the loop treats it as quit so /// teardown runs and the terminal is restored. term_signal: Arc, - should_quit: bool, - /// How to leave when `should_quit` fires: `q`/Ctrl-C/signals disconnect - /// (daemon + tasks survive), `Q` quits and kills. Defaults to the safe - /// `Disconnect` so an unexpected exit never reaps the daemon. - exit_intent: ExitIntent, + /// Set to leave the run loop, and how: `q`/Ctrl-C/signals disconnect + /// (daemon + tasks survive), `Q` quits and kills. `shutdown` reads `None` as + /// the safe `Disconnect` so an unexpected exit never reaps the daemon. + exit_intent: Option, /// Whether the client currently captures terminal mouse events. mouse_captured: bool, /// Whether the attached task is displaying scrollback. @@ -337,16 +334,16 @@ fn step_down(sel: usize, len: usize) -> usize { (sel + 1).min(len.saturating_sub(1)) } -/// State-section order: In use, Running, Idle, then Completed. +/// State-section rank and label: In use, Running, Idle, then Completed. /// Tags take precedence over lifecycle. -fn section_rank(v: &TaskView) -> u8 { +fn state_section(v: &TaskView) -> (u8, &'static str) { if v.tagged { - 0 + (0, "In use") } else { match v.lifecycle { - Lifecycle::Active => 1, - Lifecycle::Idle => 2, - Lifecycle::Ok | Lifecycle::Failed => 3, + Lifecycle::Active => (1, "Running"), + Lifecycle::Idle => (2, "Idle"), + Lifecycle::Ok | Lifecycle::Failed => (3, "Completed"), } } } @@ -495,11 +492,10 @@ impl App { wait_rx, wait_tx, term_signal: Arc::new(AtomicBool::new(false)), - should_quit: false, mouse_captured: false, view_scroll: false, selection: None, - exit_intent: ExitIntent::Disconnect, + exit_intent: None, } } @@ -571,14 +567,8 @@ impl App { .map(|(i, v)| { let (rank, label) = match self.group_mode { GroupMode::State => { - let b = section_rank(v); - let l = match b { - 0 => "In use", - 1 => "Running", - 2 => "Idle", - _ => "Completed", - }; - (b, l.to_string()) + let (rank, label) = state_section(v); + (rank, label.to_string()) } GroupMode::Dir => { let label = path::abbreviate(&v.cwd); @@ -879,10 +869,9 @@ impl App { if self.term_signal.load(Ordering::Relaxed) { // Detach on a terminating signal; leave tasks running under the daemon. - self.exit_intent = ExitIntent::Disconnect; - self.should_quit = true; + self.exit_intent = Some(ExitIntent::Disconnect); } - if self.should_quit { + if self.exit_intent.is_some() { break; } self.resolve_selection(); @@ -982,7 +971,7 @@ impl App { /// subdirectories of the resolved path come last. fn refresh_dir_candidates(&mut self) { let (base_str, partial) = split_input(&self.dir_input); - let base = self.resolve(base_str); + let base = path::resolve(&self.invocation_dir, base_str); let mut cands = vec![DirCand { label: path::abbreviate(&base), @@ -1044,7 +1033,7 @@ impl App { } /// Lock in `dir` as the spawn target and move to command entry. - fn confirm_dir(&mut self, dir: PathBuf) { + fn open_spawn_prompt(&mut self, dir: PathBuf) { self.spawn_cwd = dir; self.spawn_group = self.inherited_group(); self.input.clear(); @@ -1052,12 +1041,6 @@ impl App { self.mode = Mode::Spawn; } - /// Turn a typed path fragment into a fully-qualified, lexically-clean - /// absolute path (see `path::resolve`), resolved against the invocation dir. - fn resolve(&self, s: &str) -> PathBuf { - path::resolve(&self.invocation_dir, s) - } - /// Navigate into `dir`: retype the input as its path (trailing slash) so /// completion continues inside it, with the dir itself selected as row 0. fn enter_dir(&mut self, dir: PathBuf) { @@ -1080,9 +1063,7 @@ impl App { fn open_group_picker(&mut self) { if let Some(i) = self.selected_task() { // Store the target before building its candidate list. - self.mode = Mode::PickGroup { - target: self.views[i].id, - }; + self.mode = Mode::PickGroup(self.views[i].id); self.group_input.clear(); self.refresh_group_candidates(); } @@ -1093,7 +1074,7 @@ impl App { fn refresh_group_candidates(&mut self) { // Mark the pinned target's group even if dashboard selection changes. let current = match self.mode { - Mode::PickGroup { target } => self + Mode::PickGroup(target) => self .task_index(target) .and_then(|i| self.views[i].group.clone()), _ => None, @@ -1193,7 +1174,7 @@ impl App { self.mode = Mode::Dashboard; } - fn on_key(&mut self, out: &mut Stdout, k: KeyEvent) { + fn on_key(&mut self, out: &mut impl Write, k: KeyEvent) { // Any key dismisses a lingering save/load notice. self.status = None; // Global escape hatch, except while attached (Ctrl-C belongs to the child). @@ -1202,8 +1183,7 @@ impl App { && k.code == KeyCode::Char('c') && k.modifiers.contains(KeyModifiers::CONTROL) { - self.exit_intent = ExitIntent::Disconnect; - self.should_quit = true; + self.exit_intent = Some(ExitIntent::Disconnect); return; } // Reserve `Ctrl-]` from children, like `Ctrl-\`. Handle it before mode @@ -1219,7 +1199,7 @@ impl App { Mode::Dashboard => self.on_key_dashboard(k), Mode::Spawn => self.on_key_spawn(k), Mode::PickDir => self.on_key_pickdir(k), - Mode::PickGroup { .. } => self.on_key_pickgroup(k), + Mode::PickGroup(_) => self.on_key_pickgroup(k), Mode::Find => self.on_key_find(k), Mode::SaveSession => self.on_key_savesession(k), Mode::Rename(_) => self.on_key_rename(k), @@ -1236,7 +1216,9 @@ impl App { // Reconnect only makes sense against a daemon; a dead in-process core // has nothing to reconnect to, so `--foreground` just quits. KeyCode::Char('r') if self.daemon_backed => self.reconnect(), - KeyCode::Char('q') | KeyCode::Char('Q') | KeyCode::Esc => self.should_quit = true, + KeyCode::Char('q') | KeyCode::Char('Q') | KeyCode::Esc => { + self.exit_intent = Some(ExitIntent::Disconnect); + } _ => {} } } @@ -1248,14 +1230,8 @@ impl App { } match k.code { // `q` detaches (daemon + tasks live on); `Q` kills all and stops it. - KeyCode::Char('q') => { - self.exit_intent = ExitIntent::Disconnect; - self.should_quit = true; - } - KeyCode::Char('Q') => { - self.exit_intent = ExitIntent::Quit; - self.should_quit = true; - } + KeyCode::Char('q') => self.exit_intent = Some(ExitIntent::Disconnect), + KeyCode::Char('Q') => self.exit_intent = Some(ExitIntent::Quit), KeyCode::Up | KeyCode::Char('k') => self.select_up(), KeyCode::Down | KeyCode::Char('j') => self.select_down(), KeyCode::Tab => self.select_next_section(), @@ -1289,12 +1265,7 @@ impl App { KeyCode::Char('/') => self.open_find_palette(), // Uppercase R renames; lowercase r reruns. KeyCode::Char('R') => self.open_rename_prompt(), - KeyCode::Char('n') => { - self.input.clear(); - self.spawn_cwd = self.invocation_dir.clone(); - self.spawn_group = self.inherited_group(); - self.mode = Mode::Spawn; - } + KeyCode::Char('n') => self.open_spawn_prompt(self.invocation_dir.clone()), KeyCode::Char('@') => { self.dir_input.clear(); self.refresh_dir_candidates(); @@ -1366,16 +1337,13 @@ impl App { } fn on_key_loadsession(&mut self, k: KeyEvent) { - if matches!(k.code, KeyCode::Tab | KeyCode::BackTab) { - if !self.session_recovery.is_empty() { + match k.code { + KeyCode::Tab | KeyCode::BackTab if !self.session_recovery.is_empty() => { self.session_page = match self.session_page { SessionPage::Saved => SessionPage::Recovery, SessionPage::Recovery => SessionPage::Saved, }; } - return; - } - match k.code { KeyCode::Esc => self.mode = Mode::Dashboard, KeyCode::Up | KeyCode::Down => { let (sel, len) = match self.session_page { @@ -1434,7 +1402,7 @@ impl App { let path = c.path.clone(); match c.kind { // Resolved path or current-task directory: run there. - DirKind::Use | DirKind::Jump => self.confirm_dir(path), + DirKind::Use | DirKind::Jump => self.open_spawn_prompt(path), // Subdirectory: descend and select its resolved-path row. DirKind::Into => self.enter_dir(path), } @@ -1442,7 +1410,7 @@ impl App { } // Caret motion does not affect directory candidates. _ => { - if on_key_edit(&mut self.dir_input, k) == Some(true) { + if on_key_edit(&mut self.dir_input, k) { self.refresh_dir_candidates(); } } @@ -1466,14 +1434,14 @@ impl App { .get(self.group_sel) .and_then(|c| c.group.clone()) }; - if let Mode::PickGroup { target } = self.mode { + if let Mode::PickGroup(target) = self.mode { self.transport.send(Command::SetGroup { id: target, group }); } self.close_group_picker(); } // Caret motion does not affect group candidates. _ => { - if on_key_edit(&mut self.group_input, k) == Some(true) { + if on_key_edit(&mut self.group_input, k) { self.refresh_group_candidates(); } } @@ -1495,7 +1463,7 @@ impl App { } // Caret motion does not affect the matches. _ => { - if on_key_edit(&mut self.find_input, k) == Some(true) { + if on_key_edit(&mut self.find_input, k) { self.refresh_find_candidates(); } } @@ -1529,7 +1497,7 @@ impl App { } } - fn on_key_attached(&mut self, out: &mut Stdout, k: KeyEvent) { + fn on_key_attached(&mut self, out: &mut impl Write, k: KeyEvent) { // Background on Ctrl-\; the chord may be reported by crossterm as Ctrl-4. let detach = k.modifiers.contains(KeyModifiers::CONTROL) && matches!(k.code, KeyCode::Char('\\') | KeyCode::Char('4')); @@ -1615,7 +1583,7 @@ impl App { paste_into(&mut self.dir_input, s); self.refresh_dir_candidates(); } - Mode::PickGroup { .. } => { + Mode::PickGroup(_) => { paste_into(&mut self.group_input, s); self.refresh_group_candidates(); } @@ -1772,7 +1740,7 @@ impl App { } /// Apply input-mode changes for the current focus. - fn sync_input_modes(&mut self, out: &mut Stdout) -> io::Result<()> { + fn sync_input_modes(&mut self, out: &mut impl Write) -> io::Result<()> { let attached = match self.mode { Mode::Attached => self.focused_id.and_then(|id| self.screen_for(id)), _ => None, @@ -1816,7 +1784,7 @@ impl App { /// Background the attached task and return to the dashboard. Preserve the /// selected row. - fn detach(&mut self, out: &mut Stdout) { + fn detach(&mut self, out: &mut impl Write) { self.mode = Mode::Dashboard; self.focused_id = None; self.return_to = None; @@ -1854,7 +1822,7 @@ impl App { /// Navigate to `chord_target`. From the dashboard or Peek, record the /// dashboard as the origin and close Peek. From an attached task, record /// that task instead. On return to a non-flagship task, clear `return_to`. - fn on_chord(&mut self, out: &mut Stdout) { + fn on_chord(&mut self, out: &mut impl Write) { let Some(target) = self.chord_target() else { return; }; @@ -1908,10 +1876,11 @@ impl App { /// With an in-process core (`--foreground`), kill everything for either intent: no /// daemon is available after UI exit. fn shutdown(&mut self) { + let intent = self.exit_intent.unwrap_or(ExitIntent::Disconnect); // Blocks until the transport has acted on the intent. On `Quit` the // tasks are dead before `main` restores the terminal; on `Disconnect` the // daemon keeps running. - self.transport.shutdown(self.exit_intent); + self.transport.shutdown(intent); } } @@ -1966,48 +1935,30 @@ fn is_controls_key(k: KeyEvent) -> bool { } } -/// Apply prompt editing keys. Returns `Some(true)` for text changes, `Some(false)` for -/// caret motion or ignored Ctrl chords, and `None` for unsupported keys. On Ctrl-A or +/// Apply prompt editing keys. Returns `true` for Backspace or an inserted character, +/// `false` for caret motion, ignored Ctrl chords, and unsupported keys. On Ctrl-A or /// Ctrl-E, move to the start or end. -fn on_key_edit(buf: &mut EditBuffer, k: KeyEvent) -> Option { +fn on_key_edit(buf: &mut EditBuffer, k: KeyEvent) -> bool { let ctrl = k.modifiers.contains(KeyModifiers::CONTROL); match k.code { KeyCode::Backspace => { buf.backspace(); - Some(true) - } - KeyCode::Left => { - buf.left(); - Some(false) - } - KeyCode::Right => { - buf.right(); - Some(false) - } - KeyCode::Home => { - buf.home(); - Some(false) - } - KeyCode::End => { - buf.end(); - Some(false) - } - KeyCode::Char('a') if ctrl => { - buf.home(); - Some(false) - } - KeyCode::Char('e') if ctrl => { - buf.end(); - Some(false) + return true; } KeyCode::Char(c) if !ctrl => { buf.insert(c); - Some(true) - } - // Ignore unbound Ctrl chords without inserting their character. - KeyCode::Char(_) => Some(false), - _ => None, - } + return true; + } + KeyCode::Left => buf.left(), + KeyCode::Right => buf.right(), + KeyCode::Home => buf.home(), + KeyCode::End => buf.end(), + KeyCode::Char('a') if ctrl => buf.home(), + KeyCode::Char('e') if ctrl => buf.end(), + // Ignore unsupported keys, and unbound Ctrl chords without inserting their character. + _ => {} + } + false } /// Match a lowercased query against a task's name, command, or group. diff --git a/src/app_tests.rs b/src/app_tests.rs index 92dee08..c4fc709 100644 --- a/src/app_tests.rs +++ b/src/app_tests.rs @@ -259,7 +259,7 @@ fn reconnect_reset_drops_the_pending_spawn_ack() { assert_eq!(app.pending_select, None, "the stale ack must not survive"); assert_eq!(app.selected_id, None); assert!(app.views.is_empty()); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); } /// Dir mode makes one section per distinct cwd (invocation dir first); state @@ -1699,7 +1699,7 @@ fn cycle_tagged_is_noop_without_tags() { app.on_key_dashboard(key(KeyCode::Char('M'))); assert_eq!(app.selected_id, Some(1), "no tags: the selection stands"); - assert!(app.mode == Mode::Dashboard, "no tags: the mode stands"); + assert_eq!(app.mode, Mode::Dashboard, "no tags: the mode stands"); assert!(app.notice().is_none() && app.status.is_none()); } @@ -1781,7 +1781,7 @@ fn cycle_tagged_mutates_no_task_state() { "order is unchanged, so nothing reordered the list" ); assert!(app.notice().is_none() && app.status.is_none()); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); // Press five times over two tags to select the second. assert_eq!(app.selected_id, Some(3)); } @@ -1880,7 +1880,7 @@ fn paste_into_text_entry_strips_controls() { app.mode = Mode::Spawn; app.on_paste("cargo\ttest\r\n --all"); assert_eq!(app.input.as_str(), "cargotest --all"); - assert!(app.mode == Mode::Spawn, "paste must not submit"); + assert_eq!(app.mode, Mode::Spawn, "paste must not submit"); } /// Select mouse capture by screen type. @@ -2033,8 +2033,8 @@ fn attached_wheel_honors_the_childs_1007_veto() { #[test] fn scroll_view_entry_and_exit() { let (mut app, _) = App::attached(30, 100, "sleep 5"); - assert!(app.mode == Mode::Attached); - let mut out = io::stdout(); + assert_eq!(app.mode, Mode::Attached); + let mut out = io::sink(); app.on_key_attached(&mut out, shift(KeyCode::PageUp)); assert!(app.view_scroll, "Shift+PageUp must enter the scroll view"); @@ -2081,14 +2081,14 @@ fn wheel_moves_dashboard_selection() { fn group_picker_opens_on_g_only_with_a_selection() { let mut app = App::new_local(30, 100); app.on_key_dashboard(key(KeyCode::Char('g'))); - assert!(app.mode == Mode::Dashboard, "no selection: g must no-op"); + assert_eq!(app.mode, Mode::Dashboard, "no selection: g must no-op"); let inv = app.invocation_dir.clone(); app.spawn_in("sleep 5", inv); app.pump(); app.resolve_selection(); app.on_key_dashboard(key(KeyCode::Char('g'))); - assert!(app.mode == Mode::PickGroup { target: 1 }); + assert_eq!(app.mode, Mode::PickGroup(1)); } /// Group candidates are distinct, case-insensitively sorted, and follow @@ -2207,7 +2207,7 @@ fn group_enter_on_a_candidate_assigns_it() { app.on_key_dashboard(key(KeyCode::Char('g'))); app.on_key_pickgroup(key(KeyCode::Char('a'))); // highlights "alpha" app.on_key_pickgroup(key(KeyCode::Enter)); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); app.pump(); let v = app.views.iter().find(|v| v.id == 2).unwrap(); assert_eq!(v.group.as_deref(), Some("alpha")); @@ -2261,7 +2261,7 @@ fn group_esc_cancels_without_sending() { app.on_key_pickgroup(key(KeyCode::Char(c))); } app.on_key_pickgroup(key(KeyCode::Esc)); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert!(app.group_input.is_empty() && app.group_candidates.is_empty()); app.pump(); let v = app.views.iter().find(|v| v.id == 1).unwrap(); @@ -2287,7 +2287,7 @@ fn find_type(app: &mut App, text: &str) { fn find_palette_opens_on_slash_only_with_tasks() { let mut app = App::new_local(30, 100); app.on_key_dashboard(key(KeyCode::Char('/'))); - assert!(app.mode == Mode::Dashboard, "empty fleet: / must no-op"); + assert_eq!(app.mode, Mode::Dashboard, "empty fleet: / must no-op"); assert!(app.find_candidates.is_empty()); let inv = app.invocation_dir.clone(); @@ -2296,7 +2296,7 @@ fn find_palette_opens_on_slash_only_with_tasks() { // Exercise opening find without a current selection. app.selected_id = None; app.on_key_dashboard(key(KeyCode::Char('/'))); - assert!(app.mode == Mode::Find); + assert_eq!(app.mode, Mode::Find); assert_eq!(find_ids(&app), vec![1]); } @@ -2469,7 +2469,7 @@ fn find_esc_leaves_the_selection_alone() { find_type(&mut app, "true"); assert_eq!(find_ids(&app), vec![2]); app.on_key_find(key(KeyCode::Esc)); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert_eq!(app.selected_id, Some(1), "Esc must not move the selection"); assert!(app.find_input.is_empty() && app.find_candidates.is_empty()); assert_eq!(app.find_sel, 0); @@ -2488,7 +2488,7 @@ fn find_enter_without_candidates_keeps_the_panel_open() { find_type(&mut app, "zzz"); assert!(find_ids(&app).is_empty()); app.on_key_find(key(KeyCode::Enter)); - assert!(app.mode == Mode::Find, "no match: Enter must not close"); + assert_eq!(app.mode, Mode::Find, "no match: Enter must not close"); assert_eq!(app.selected_id, Some(1), "selection is untouched"); // Editing the query refreshes candidates. @@ -2593,9 +2593,9 @@ fn controls_overlay_opens_on_question_and_closes_on_peeks_key_set() { let mut app = App::new_local(30, 100); for close in [KeyCode::Char('?'), KeyCode::Esc, KeyCode::Char('q')] { app.on_key_dashboard(key(KeyCode::Char('?'))); - assert!(app.mode == Mode::Controls, "? must open the overlay"); + assert_eq!(app.mode, Mode::Controls, "? must open the overlay"); app.on_key_controls(key(close)); - assert!(app.mode == Mode::Dashboard, "{close:?} must close it"); + assert_eq!(app.mode, Mode::Dashboard, "{close:?} must close it"); } } @@ -2611,7 +2611,7 @@ fn controls_overlay_accepts_both_spellings_of_the_chord() { "{chord:?} must open the overlay" ); app.on_key_controls(chord); - assert!(app.mode == Mode::Dashboard, "{chord:?} must close it"); + assert_eq!(app.mode, Mode::Dashboard, "{chord:?} must close it"); } } @@ -2623,7 +2623,7 @@ fn plain_slash_still_opens_the_find_palette() { app.spawn_in("sleep 5", inv); app.pump(); app.on_key_dashboard(key(KeyCode::Char('/'))); - assert!(app.mode == Mode::Find); + assert_eq!(app.mode, Mode::Find); } /// Dashboard bindings are inert while the controls overlay is open. @@ -2639,7 +2639,7 @@ fn controls_overlay_ignores_dashboard_keys() { app.on_key_controls(key(KeyCode::Char('m'))); app.on_key_controls(key(KeyCode::Char('n'))); app.pump(); - assert!(app.mode == Mode::Controls, "neither key closes the overlay"); + assert_eq!(app.mode, Mode::Controls, "neither key closes the overlay"); assert!(!app.views[0].tagged, "m must not reach the task"); assert!(app.input.as_str().is_empty(), "n must not open the prompt"); @@ -2722,14 +2722,14 @@ fn dashboard_hints_defer_the_long_tail_to_the_overlay() { fn rename_prompt_opens_on_shift_r_only_with_a_selection() { let mut app = App::new_local(30, 100); app.on_key_dashboard(key(KeyCode::Char('R'))); - assert!(app.mode == Mode::Dashboard, "no selection: R must no-op"); + assert_eq!(app.mode, Mode::Dashboard, "no selection: R must no-op"); let inv = app.invocation_dir.clone(); app.spawn_in("sleep 5", inv); app.pump(); app.resolve_selection(); app.on_key_dashboard(key(KeyCode::Char('R'))); - assert!(app.mode == Mode::Rename(1)); + assert_eq!(app.mode, Mode::Rename(1)); assert_eq!(app.input.as_str(), "", "an unnamed task prefills empty"); // A named task prefills its name. @@ -2756,7 +2756,7 @@ fn rename_enter_sends_the_typed_name() { app.on_key_rename(key(KeyCode::Char(c))); } app.on_key_rename(key(KeyCode::Enter)); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert!(app.input.is_empty()); app.pump(); let v = app.views.iter().find(|v| v.id == 1).unwrap(); @@ -2803,7 +2803,7 @@ fn rename_esc_cancels_without_sending() { app.on_key_rename(key(KeyCode::Char(c))); } app.on_key_rename(key(KeyCode::Esc)); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert!(app.input.is_empty()); app.pump(); let v = app.views.iter().find(|v| v.id == 1).unwrap(); @@ -3019,7 +3019,7 @@ fn custom_mode_spawn_inherits_the_selected_group() { app.resolve_selection(); app.on_key_dashboard(key(KeyCode::Char('n'))); - assert!(app.mode == Mode::Spawn); + assert_eq!(app.mode, Mode::Spawn); assert_eq!(app.spawn_group.as_deref(), Some("alpha")); for c in "sleep 5".chars() { @@ -3046,10 +3046,10 @@ fn dir_picker_handoff_inherits_the_selected_group_in_custom_mode() { app.resolve_selection(); app.on_key_dashboard(key(KeyCode::Char('@'))); - assert!(app.mode == Mode::PickDir); + assert_eq!(app.mode, Mode::PickDir); // Row 0 is the current dir (DirKind::Use): Enter hands off to Spawn. app.on_key_pickdir(key(KeyCode::Enter)); - assert!(app.mode == Mode::Spawn); + assert_eq!(app.mode, Mode::Spawn); assert_eq!(app.spawn_group.as_deref(), Some("alpha")); } @@ -3497,7 +3497,7 @@ fn coordinate_invalidation_clears_the_selection() { app.on_mouse(drag_to(0, 4)); assert!(app.selection.is_some(), "the drag must be live"); }; - let mut out = io::stdout(); + let mut out = io::sink(); let mut app = App::attached_with_lines(&["hello world"]); start(&mut app); @@ -3507,7 +3507,7 @@ fn coordinate_invalidation_clears_the_selection() { let mut app = App::attached_with_lines(&["hello world"]); start(&mut app); app.on_key_attached(&mut out, ctrl(KeyCode::Char('\\'))); - assert!(app.mode == Mode::Dashboard, "ctrl-\\ detaches"); + assert_eq!(app.mode, Mode::Dashboard, "ctrl-\\ detaches"); assert!(app.selection.is_none(), "detach must clear"); let mut app = App::attached_with_lines(&["hello world"]); @@ -3692,7 +3692,7 @@ fn capture_drop_clears_a_live_selection() { s.alt_screen = true; s.alt_scroll = true; } - app.sync_input_modes(&mut std::io::stdout()).unwrap(); + app.sync_input_modes(&mut io::sink()).unwrap(); assert!(!app.mouse_captured, "premise: capture dropped"); assert!(app.selection().is_none(), "the drop must clear the drag"); assert!(app.pending_clipboard.is_empty(), "nothing may copy"); @@ -3793,7 +3793,7 @@ fn scrollback_wheel_cancels_the_drag() { /// Scrollback navigation and exit keys cancel an active drag. #[test] fn scrollback_keys_clear_the_drag() { - let mut out = io::stdout(); + let mut out = io::sink(); let mut app = App::attached_with_lines(&["old line one"]); app.enter_scrollback(5); app.on_mouse(press(0, 0)); @@ -4026,7 +4026,7 @@ impl App { /// Dispatch `k` through `on_key`, the run loop's entry point. fn hit(&mut self, k: KeyEvent) { - self.on_key(&mut io::stdout(), k); + self.on_key(&mut io::sink(), k); } /// Return the flagship id from the latest snapshot. @@ -4079,7 +4079,7 @@ fn bracket_marks_selected() { app.hit(key(KeyCode::Char(']'))); app.pump(); assert_eq!(app.flagship(), Some(a), "the mark moves to A"); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); } /// Clear the mark with `]` on the flagship. @@ -4117,7 +4117,7 @@ fn bracket_without_selection_noops() { let sent = app.record_sends(); app.hit(key(KeyCode::Char(']'))); assert!(sent.lock().unwrap().is_empty()); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); } /// Handle `]` only on the dashboard; ignore it in Peek. @@ -4125,13 +4125,13 @@ fn bracket_without_selection_noops() { fn bracket_in_peek_noops() { let (mut app, _, q) = flagship_pair(); app.hit(key(KeyCode::Char(' '))); - assert!(app.mode == Mode::Peek, "premise: peeking at A"); + assert_eq!(app.mode, Mode::Peek, "premise: peeking at A"); let sent = app.record_sends(); app.hit(key(KeyCode::Char(']'))); app.pump(); assert!(sent.lock().unwrap().is_empty()); assert_eq!(app.flagship(), Some(q)); - assert!(app.mode == Mode::Peek); + assert_eq!(app.mode, Mode::Peek); } /// Without a flagship, ignore the chord on the dashboard and while attached. @@ -4143,12 +4143,12 @@ fn ctrl_bracket_noops_without_flagship() { let sent = app.record_sends(); app.hit(chord()); - assert!(app.mode == Mode::Attached); + assert_eq!(app.mode, Mode::Attached); assert_eq!(app.focused_id, Some(a)); app.hit(ctrl(KeyCode::Char('\\'))); app.hit(chord()); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert_eq!(app.focused_id, None); assert!(sent.lock().unwrap().is_empty(), "nothing reaches the core"); } @@ -4162,14 +4162,14 @@ fn jump_from_dashboard_and_back() { assert_eq!(app.chord_target(), Some(Target::Flagship(q))); app.hit(chord()); - assert!(app.mode == Mode::Attached); + assert_eq!(app.mode, Mode::Attached); assert_eq!(app.focused_id, Some(q)); assert_eq!(app.return_to, Some(Origin::Dashboard)); assert_eq!(app.selected_id, Some(a), "the jump leaves selection alone"); assert_eq!(app.chord_target(), Some(Target::Dashboard)); app.hit(chord()); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert_eq!(app.focused_id, None); assert!(!app.view_scroll); assert_eq!(app.return_to, None); @@ -4204,7 +4204,7 @@ fn jump_from_task_and_back() { assert_eq!(app.chord_target(), Some(Target::Task(a))); app.hit(chord()); - assert!(app.mode == Mode::Attached); + assert_eq!(app.mode, Mode::Attached); assert_eq!(app.focused_id, Some(a)); assert!(!app.view_scroll); assert_eq!(app.return_to, None); @@ -4223,10 +4223,10 @@ fn jump_from_task_and_back() { fn jump_from_peek_lands_on_dashboard() { let (mut app, a, q) = flagship_pair(); app.hit(key(KeyCode::Char(' '))); - assert!(app.mode == Mode::Peek); + assert_eq!(app.mode, Mode::Peek); app.hit(chord()); - assert!(app.mode == Mode::Attached); + assert_eq!(app.mode, Mode::Attached); assert_eq!(app.focused_id, Some(q)); assert_eq!(app.return_to, Some(Origin::Dashboard)); @@ -4275,7 +4275,7 @@ fn mark_then_enter_before_snapshot_returns_to_dashboard() { assert_eq!(app.chord_target(), Some(Target::Dashboard)); app.hit(chord()); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert_eq!(app.focused_id, None); assert_eq!(app.return_to, None); } @@ -4291,7 +4291,7 @@ fn return_to_finished_origin() { assert_eq!(app.chord_target(), Some(Target::Task(a))); app.hit(chord()); - assert!(app.mode == Mode::Attached); + assert_eq!(app.mode, Mode::Attached); assert_eq!(app.focused_id, Some(a)); assert_eq!(app.return_to, None); } @@ -4309,7 +4309,7 @@ fn return_to_removed_origin_degrades_to_dashboard() { assert_eq!(app.chord_target(), Some(Target::Dashboard)); app.hit(chord()); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert_eq!(app.focused_id, None); assert_eq!(app.return_to, None); assert!(app.task_index(q).is_some()); @@ -4330,7 +4330,7 @@ fn dead_flagship_returns_then_noops() { assert_eq!(app.focused_id, Some(a)); assert_eq!(app.chord_target(), None); app.hit(chord()); - assert!(app.mode == Mode::Attached); + assert_eq!(app.mode, Mode::Attached); assert_eq!(app.focused_id, Some(a), "second press no-ops"); // Entered by `Enter`. @@ -4339,10 +4339,10 @@ fn dead_flagship_returns_then_noops() { app.hit(key(KeyCode::Enter)); app.finish(q); app.hit(chord()); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert_eq!(app.chord_target(), None); app.hit(chord()); - assert!(app.mode == Mode::Dashboard, "second press no-ops"); + assert_eq!(app.mode, Mode::Dashboard, "second press no-ops"); assert_eq!(app.focused_id, None); } @@ -4353,7 +4353,7 @@ fn return_to_clears_on_detach() { app.hit(chord()); assert!(app.return_to.is_some()); app.hit(ctrl(KeyCode::Char('\\'))); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert_eq!(app.return_to, None); } @@ -4366,7 +4366,7 @@ fn return_to_clears_on_vanish() { assert_eq!(app.return_to, Some(Origin::Task(a))); app.views.retain(|v| v.id != q); app.check_attached_task(); - assert!(app.mode == Mode::Dashboard); + assert_eq!(app.mode, Mode::Dashboard); assert_eq!(app.focused_id, None); assert_eq!(app.return_to, None); } @@ -4380,7 +4380,7 @@ fn return_to_clears_on_disconnect() { assert!(app.return_to.is_some()); app.transport = Box::new(Unplugged); app.check_connection(); - assert!(app.mode == Mode::Disconnected); + assert_eq!(app.mode, Mode::Disconnected); assert_eq!(app.return_to, None); app.return_to = Some(Origin::Task(1)); @@ -4396,7 +4396,7 @@ fn ctrl_bracket_matches_legacy_ctrl_5() { app.hit(k); assert_eq!(app.focused_id, Some(q), "{k:?} must jump"); app.hit(k); - assert!(app.mode == Mode::Dashboard, "{k:?} must return"); + assert_eq!(app.mode, Mode::Dashboard, "{k:?} must return"); } } @@ -4429,7 +4429,7 @@ fn flagship_keys_ignored_outside_navigation_modes() { let modes = [ Mode::Spawn, Mode::PickDir, - Mode::PickGroup { target: a }, + Mode::PickGroup(a), Mode::Find, Mode::SaveSession, Mode::Rename(a), @@ -4442,7 +4442,7 @@ fn flagship_keys_ignored_outside_navigation_modes() { for k in [key(KeyCode::Char(']')), chord(), ctrl(KeyCode::Char('5'))] { app.hit(k); app.pump(); - assert!(app.mode == mode, "{k:?} changed the mode"); + assert_eq!(app.mode, mode, "{k:?} changed the mode"); assert_eq!(app.focused_id, None, "{k:?} attached"); assert_eq!(app.return_to, None); assert_eq!(app.flagship(), Some(q), "{k:?} moved the mark"); diff --git a/src/ui.rs b/src/ui.rs index e868e46..d86b280 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -40,7 +40,7 @@ pub fn render(out: &mut impl Write, app: &mut App) -> io::Result { render_dashboard(&mut buf, app)?; render_pickdir(&mut buf, app)?; } - Mode::PickGroup { .. } => { + Mode::PickGroup(_) => { render_dashboard(&mut buf, app)?; render_pickgroup(&mut buf, app)?; } From 42e3beb270952f94f2aa0ca40b46e7aae754c3b5 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 14:36:46 -0700 Subject: [PATCH 05/11] refactor(core): drop two derivable flags, route clear_watch and input through one path --- src/preview.rs | 23 ++------ src/supervisor.rs | 95 +++++++++++++-------------------- src/supervisor_capture_tests.rs | 9 ++-- src/supervisor_tests.rs | 25 +++------ src/task.rs | 30 ++++------- src/task_tests.rs | 19 ++----- 6 files changed, 67 insertions(+), 134 deletions(-) diff --git a/src/preview.rs b/src/preview.rs index 358499b..a658efb 100644 --- a/src/preview.rs +++ b/src/preview.rs @@ -167,13 +167,7 @@ fn cascade( // An indented status line can remain as the idle floor. Trim only the display // candidate: the emulator row must remain verbatim in `live_floor` for // teardown-snapshot comparison. - let floor = screen.live_floor(); - let trimmed = floor.trim_start(); - Preview::floor(if trimmed.len() == floor.len() { - floor - } else { - trimmed.to_string() - }) + Preview::floor(screen.live_floor().trim_start().to_string()) } /// Screen and registry state that invalidates the cached preview candidate. @@ -205,13 +199,6 @@ pub struct PreviewState { /// Finalization combines this with the exit snapshot to detect a restored /// primary screen. rendered_under_alt: bool, - finalized: bool, -} - -impl Default for PreviewState { - fn default() -> Self { - Self::new() - } } impl PreviewState { @@ -224,13 +211,12 @@ impl PreviewState { last_title_render: None, last_key: None, rendered_under_alt: false, - finalized: false, } } /// Whether the preview froze: the caller's once-per-life finalize gate. pub fn finalized(&self) -> bool { - self.finalized + self.rendered.frozen } /// Resolve the rendered preview against the current screen. `now` is a parameter, @@ -245,7 +231,7 @@ impl PreviewState { adapter: Option<&dyn SummaryAdapter>, blocked: Option<(&str, &'static str)>, ) -> &Preview { - if self.finalized { + if self.rendered.frozen { return &self.rendered; } let key = ( @@ -330,10 +316,9 @@ impl PreviewState { /// different final floor is resolved normally. /// Repeated calls are no-ops. pub fn finalize(&mut self, screen: &impl ScreenFacts, adapter: Option<&dyn SummaryAdapter>) { - if self.finalized { + if self.rendered.frozen { return; } - self.finalized = true; self.downgrade_pending_since = None; let alt_torn_down_at_exit = self.rendered_under_alt && !screen.alternate_screen() diff --git a/src/supervisor.rs b/src/supervisor.rs index 8ae095a..96824a8 100644 --- a/src/supervisor.rs +++ b/src/supervisor.rs @@ -1,8 +1,9 @@ //! The task owner: holds every `Task`, allocates ids, reaps exits, and answers //! `Command`s with `Event`s. It speaks only `protocol` types, never UI state. -//! Driven through three calls: `apply` (one `Command`), `tick` (reap, then emit -//! a task snapshot plus the watched screen), and `drain` (take the queued -//! `Event`s). +//! A serving loop drives it through `apply` (one `Command`), `tick` (reap, then +//! emit a task snapshot plus the watched screen), and `drain` (take the queued +//! `Event`s). Between clients the daemon calls `reap` and +//! `recovery_maintenance` directly. use std::{ collections::BTreeMap, @@ -24,8 +25,7 @@ use crate::{ task::{Task, WriteRefused}, }; -/// Quiet period after which a live task becomes idle. Lifecycle and placement -/// use this same threshold. +/// Quiet period after which a live task becomes idle. const IDLE_AFTER: Duration = Duration::from_secs(10); /// Per-dimension PTY size limit. Resizes are clamped to `[1, MAX_DIM]` to keep @@ -155,6 +155,15 @@ fn current_resume_id(task: &Task) -> Option { task.resume_id.clone() } +/// Build the saved or rerun command from the task's best-known ID. +/// Without an ID, preserve the requested command exactly. +fn recipe_command(t: &Task) -> String { + match (t.harness, current_resume_id(t)) { + (Some(h), Some(id)) => h.resume_command(&t.command, &id), + _ => t.command.clone(), + } +} + /// Resolve harness configuration from the task's launch environment. fn harness_home(env: &[(OsString, OsString)], h: &dyn harness::Harness) -> Option { h.resolve_home(&|key| env_get(env, key).map(PathBuf::from)) @@ -193,9 +202,8 @@ fn affects_recipe(cmd: &Command) -> bool { struct Recovery { /// Recovery writes are opt-in in unit tests. enabled: bool, - /// Whether a potentially recipe-changing command awaits a debounced pass. - dirty: bool, - /// The most recent scheduled recipe change: the debounce anchor. + /// The most recent potentially recipe-changing command still awaiting a + /// debounced pass: the debounce anchor. `None` once a pass has run. last_mutation: Option, /// The start of the most recent cadence interval. last_cadence: Instant, @@ -215,7 +223,6 @@ impl Recovery { fn new() -> Self { Self { enabled: !cfg!(test), - dirty: false, last_mutation: None, last_cadence: Instant::now(), last_written: None, @@ -335,15 +342,10 @@ impl Supervisor { /// Clear connection-owned watch state and restore the watched task's live /// viewport before another client connects. pub fn clear_watch(&mut self) { - // Restore the previous target to live output. - if let Some(old) = self.watched - && let Some(t) = self.by_id_mut(old) - { - t.scroll_view(ScrollAction::Live); - } - self.watched = None; - self.watch_attached = false; - self.last_screen = None; + self.apply(Command::Watch { + id: None, + attached: false, + }); } /// Apply one client request. Fire-and-forget: any result (a save/load @@ -352,7 +354,6 @@ impl Supervisor { // Recipe-affecting command variants arm recovery before validation; // fingerprinting filters rejected commands and other no-ops. if affects_recipe(&cmd) { - self.recovery.dirty = true; self.recovery.last_mutation = Some(Instant::now()); } match cmd { @@ -471,10 +472,9 @@ impl Supervisor { } /// One step of the core's own loop: reap exits, then emit a fresh task - /// snapshot (plus the watched task's screen). In process the client calls - /// this each UI tick; in the daemon it runs on the core's thread and the - /// events flow over the socket. Either way the client only ever sees - /// `drain`ed events, never a `Task`. + /// snapshot (plus the watched task's screen). `core::run_loop` is the only + /// production caller, in process and in the daemon alike; the client only + /// ever sees `drain`ed events, never a `Task`. pub fn tick(&mut self) { self.reap(); // Check current state rather than individual transitions to handle exit, @@ -494,11 +494,7 @@ impl Supervisor { // preview resolution reads the grid, letting the same tick ship it. // Resolution mutates per-task hold state; all tasks use one timestamp. // Only the attached watch may forward clipboard stores. - let forwarding = if self.watch_attached { - self.watched - } else { - None - }; + let forwarding = self.watched.filter(|_| self.watch_attached); let mut clipboard = None; let views = self .tasks @@ -579,11 +575,10 @@ impl Supervisor { if !self.recovery.enabled { return; } - let debounce_due = self.recovery.dirty - && self - .recovery - .last_mutation - .is_some_and(|t| now.duration_since(t) >= self.recovery.debounce); + let debounce_due = self + .recovery + .last_mutation + .is_some_and(|t| now.duration_since(t) >= self.recovery.debounce); let cadence_due = now.duration_since(self.recovery.last_cadence) >= self.recovery.cadence; if !(debounce_due || cadence_due) { return; @@ -593,12 +588,12 @@ impl Supervisor { } // Do not replace an existing snapshot with an empty recipe. if self.tasks.is_empty() { - self.recovery.dirty = false; + self.recovery.last_mutation = None; return; } // Skip this pass without a config root. let Some(root) = self.sessions_root() else { - self.recovery.dirty = false; + self.recovery.last_mutation = None; return; }; let cfg = self.session_config(); @@ -613,7 +608,7 @@ impl Supervisor { .as_ref() .is_some_and(|(r, dest, h)| *r == root && *h == hash && std::fs::metadata(dest).is_ok()) { - self.recovery.dirty = false; + self.recovery.last_mutation = None; return; } let label = session::recovery_label(std::time::SystemTime::now()); @@ -636,7 +631,7 @@ impl Supervisor { } } } - self.recovery.dirty = false; + self.recovery.last_mutation = None; } /// Run recovery maintenance between clients without queuing task or screen @@ -682,18 +677,13 @@ impl Supervisor { f: impl FnOnce(&mut Task) -> Result<(), WriteRefused>, ) { if let Some(r) = self.by_id_mut(id).and_then(|t| f(t).err()) { - self.notice_refused(id, what, r.len); + self.status(format!( + "task {id} is not reading input; dropped {} {what}", + crate::format::bytes(r.len) + )); } } - /// Report the task and message size for a bounded writer-queue refusal. - fn notice_refused(&mut self, id: u64, what: &str, len: usize) { - self.status(format!( - "task {id} is not reading input; dropped {} {what}", - crate::format::bytes(len) - )); - } - fn index_of(&self, id: u64) -> Option { self.tasks.iter().position(|t| t.id == id) } @@ -863,7 +853,7 @@ impl Supervisor { // targeted conversation. let (command, cwd) = { let old = &self.tasks[i]; - let command = Self::recipe_command(old); + let command = recipe_command(old); (command, old.cwd.clone()) }; // Preserve the finished task if its replacement cannot start. Use a distinct @@ -901,7 +891,7 @@ impl Supervisor { cfg.entry(path::abbreviate(&t.cwd)) .or_default() .push(SessionEntry { - cmd: Self::recipe_command(t), + cmd: recipe_command(t), group: t.group.clone(), name: t.name.clone(), }); @@ -909,15 +899,6 @@ impl Supervisor { cfg } - /// Build the saved or rerun command from the task's best-known ID. - /// Without an ID, preserve the requested command exactly. - fn recipe_command(t: &Task) -> String { - match (t.harness, current_resume_id(t)) { - (Some(h), Some(id)) => h.resume_command(&t.command, &id), - _ => t.command.clone(), - } - } - /// Session-recipe root for this connection: `FLEETCOM_CONFIG_DIR` from the /// installed launch context's env, else this process's [`session::sessions_dir`]. /// Prefer the launch context because the daemon's own env is frozen from whichever diff --git a/src/supervisor_capture_tests.rs b/src/supervisor_capture_tests.rs index b256f7f..b64a80f 100644 --- a/src/supervisor_capture_tests.rs +++ b/src/supervisor_capture_tests.rs @@ -371,8 +371,7 @@ fn returning_to_a_prior_root_preserves_its_live_captures() { let dir = scratch("cap_aba"); let (bin, root_a, root_b) = (dir.join("bin"), dir.join("run-a"), dir.join("run-b")); install_stub(&bin, "claude", &dir); - let mut s = Supervisor::new(24, 80, 2000); - s.set_launch_context(agent_ctx(&bin, &root_a, dir.to_path_buf())); + let mut s = sup_ctx(agent_ctx(&bin, &root_a, dir.to_path_buf())); spawn(&mut s, "claude", dir.to_path_buf()); let cap_a = s.tasks[0].capture_file.clone().expect("capture file set"); std::fs::write(&cap_a, "{}").unwrap(); @@ -409,8 +408,7 @@ fn remove_deletes_the_capture_file_under_the_spawn_root() { let dir = scratch("cap_remove_cross"); let (bin, root_a, root_b) = (dir.join("bin"), dir.join("run-a"), dir.join("run-b")); install_stub(&bin, "claude", &dir); - let mut s = Supervisor::new(24, 80, 2000); - s.set_launch_context(agent_ctx(&bin, &root_a, dir.to_path_buf())); + let mut s = sup_ctx(agent_ctx(&bin, &root_a, dir.to_path_buf())); spawn(&mut s, "claude", dir.to_path_buf()); let _ = wait_argv(&mut s, &dir.join("argv")); let id = s.tasks[0].id; @@ -991,7 +989,6 @@ fn stale_inherited_notify_chain_is_never_executed() { "codex", &format!("\"${{FLEETCOM_CAPTURE_FILE%/*}}/codex-notify.sh\" '{payload}'"), ); - let mut s = Supervisor::new(24, 80, 2000); let mut ctx = agent_ctx_plus( &bin, &runtime, @@ -1002,7 +999,7 @@ fn stale_inherited_notify_chain_is_never_executed() { crate::harness::NOTIFY_CHAIN_ENV.into(), stale.as_os_str().to_os_string(), )); - s.set_launch_context(ctx); + let mut s = sup_ctx(ctx); spawn(&mut s, "codex", dir.to_path_buf()); assert!(reap_until(&mut s, Duration::from_secs(5), |s| s.tasks[0] .finished diff --git a/src/supervisor_tests.rs b/src/supervisor_tests.rs index 35baa9e..2004214 100644 --- a/src/supervisor_tests.rs +++ b/src/supervisor_tests.rs @@ -1805,11 +1805,7 @@ fn spawn_acks_with_spawned_before_the_snapshot() { #[test] fn refused_spawn_emits_no_spawned() { let mut s = sup(24, 80); - s.apply(Command::Spawn { - command: "x".repeat(MAX_COMMAND_LEN + 1), - cwd: here(), - group: None, - }); + spawn(&mut s, "x".repeat(MAX_COMMAND_LEN + 1), here()); let evs = s.drain(); assert!( !evs.iter().any(|e| matches!(e, Event::Spawned { .. })), @@ -1849,11 +1845,7 @@ fn session_load_emits_no_spawned() { #[test] fn spawn_refuses_over_length_command() { let mut s = sup(24, 80); - s.apply(Command::Spawn { - command: "x".repeat(MAX_COMMAND_LEN + 1), - cwd: here(), - group: None, - }); + spawn(&mut s, "x".repeat(MAX_COMMAND_LEN + 1), here()); assert!( s.drain() .iter() @@ -2042,22 +2034,22 @@ fn recovery_files(config: &Path) -> Vec { names } -/// Read and clear the writer's dirty flag. +/// Read and clear whether a recipe change awaits a debounced pass. fn take_dirty(s: &mut Supervisor) -> bool { - std::mem::replace(&mut s.recovery.dirty, false) + s.recovery.last_mutation.take().is_some() } /// Recipe-changing commands arm recovery even when rejected; tags do not. #[test] fn recovery_arms_on_structural_mutations_not_tag() { let mut s = sup(24, 80); - assert!(!s.recovery.dirty, "a fresh supervisor starts clean"); + assert!(!take_dirty(&mut s), "a fresh supervisor starts clean"); spawn(&mut s, "sleep 30", here()); assert!(take_dirty(&mut s), "Spawn must arm"); let id = first_id(&mut s); - s.recovery.dirty = false; // first_id ticks; reassert a clean baseline + s.recovery.last_mutation = None; // first_id ticks; reassert a clean baseline s.apply(Command::Tag { id, on: true }); assert!( !take_dirty(&mut s), @@ -2229,7 +2221,7 @@ fn flagship_does_not_affect_recipe() { let mut s = sup(24, 80); spawn(&mut s, "sleep 30", here()); let id = spawned_id(&mut s); - s.recovery.dirty = false; // Reset after spawning to test the mark in isolation. + s.recovery.last_mutation = None; // Reset after spawning to test the mark in isolation. s.apply(Command::Flagship { id: Some(id) }); assert!(!take_dirty(&mut s), "marking must not arm"); @@ -2417,7 +2409,7 @@ fn recovery_debounce_coalesces_a_mutation_burst() { for cmd in ["sleep 30", "sleep 31", "sleep 32"] { assert!(text.contains(cmd), "snapshot must carry {cmd:?}: {text}"); } - assert!(!s.recovery.dirty, "a completed pass clears the flag"); + assert!(!take_dirty(&mut s), "a completed pass clears the flag"); } /// Empty fleets do not create or replace recovery snapshots. @@ -2543,7 +2535,6 @@ fn recovery_dedup_is_per_destination_root() { // Move the unchanged recipe to a new destination and arm recovery. s.set_launch_context(config_ctx(&config_b, dir.to_path_buf(), &[])); - s.recovery.dirty = true; s.recovery.last_mutation = Some(Instant::now()); assert!( wait_until(Duration::from_secs(5), || { diff --git a/src/task.rs b/src/task.rs index 95120b6..0774926 100644 --- a/src/task.rs +++ b/src/task.rs @@ -384,11 +384,10 @@ impl Task { /// can reach the grid. A missing reader handle counts as complete; the /// reader treats EOF and read errors identically. fn output_complete(&self) -> bool { - self.finished.is_some() && self.handle.as_ref().is_none_or(JoinHandle::is_finished) + self.finished.is_some() && self.reader_done() } /// Report whether the reader reached EOF without driving the reap loop. - #[cfg(test)] pub(crate) fn reader_done(&self) -> bool { self.handle.as_ref().is_none_or(|h| h.is_finished()) } @@ -553,19 +552,12 @@ impl Task { Ok(()) } - /// Queue `bytes` for the PTY as one message without blocking the caller. - /// Refuse it whole if admission would exceed `MAX_PENDING_WRITE`. - pub fn send_input(&mut self, bytes: &[u8]) -> Result<(), WriteRefused> { - self.snap_live(); - self.queue_write(bytes.to_vec()) - } - - /// Return the viewport to live before queuing input bytes. - fn snap_live(&mut self) { - let mut p = grid(&self.parser); - if p.scrollback() > 0 { - p.set_scrollback(0); - } + /// Return the viewport to live, then queue `bytes` for the PTY as one + /// message without blocking the caller. Refuse it whole if admission would + /// exceed `MAX_PENDING_WRITE`. + pub fn send_input(&mut self, bytes: Vec) -> Result<(), WriteRefused> { + self.scroll_view(ScrollAction::Live); + self.queue_write(bytes) } /// Admit one whole message to the writer queue, or refuse it whole. @@ -599,9 +591,7 @@ impl Task { /// grid lock, then queue it as one PTY write. pub fn send_paste(&mut self, content: &[u8]) -> Result<(), WriteRefused> { let bracketed = grid(&self.parser).bracketed_paste(); - let msg = input::paste_bytes(bracketed, content); - self.snap_live(); - self.queue_write(msg) + self.send_input(input::paste_bytes(bracketed, content)) } /// Encode and queue one mouse action using the child's screen modes. @@ -611,7 +601,7 @@ impl Task { let p = grid(&self.parser); input::mouse_bytes(&p, kind, col, row) }; - bytes.map_or(Ok(()), |b| self.send_input(&b)) + bytes.map_or(Ok(()), |b| self.send_input(b)) } /// Encode and queue one key using the child's cursor-key mode, read under @@ -621,7 +611,7 @@ impl Task { let p = grid(&self.parser); input::key_bytes(p.application_cursor(), code, mods) }; - bytes.map_or(Ok(()), |b| self.send_input(&b)) + bytes.map_or(Ok(()), |b| self.send_input(b)) } /// Return the child's mouse, alternate-screen, and alternate-scroll modes diff --git a/src/task_tests.rs b/src/task_tests.rs index f147717..bfc9e8f 100644 --- a/src/task_tests.rs +++ b/src/task_tests.rs @@ -109,18 +109,7 @@ fn finished_lifecycle_ignores_quiet_time() { #[test] fn resize_is_reflected_in_the_grid() { - let mut t = Task::spawn( - 3, - "sleep 5", - "sleep 5", - &here(), - 24, - 80, - 2000, - &env_here(), - no_waker(), - ) - .unwrap(); + let mut t = spawn(3, "sleep 5"); t.resize(30, 100).unwrap(); assert_eq!(t.parser.lock().size(), (30, 100)); t.terminate(); @@ -229,7 +218,7 @@ fn viewport_scrolls_and_snaps_live_on_input() { t.scroll_view(ScrollAction::Up(10_000)); assert_eq!(t.scroll_offset(), top); // Return the viewport to live output on input. - t.send_input(b"x").unwrap(); + t.send_input(b"x".to_vec()).unwrap(); assert_eq!(t.scroll_offset(), 0); t.terminate(); } @@ -251,8 +240,8 @@ fn screen_lines_yields_one_entry_per_grid_row() { #[test] fn queued_writes_reach_the_child_in_order() { let mut t = spawn(10, "cat"); - t.send_input(b"zqfirstqz\n").unwrap(); - t.send_input(b"zqsecondqz\n").unwrap(); + t.send_input(b"zqfirstqz\n".to_vec()).unwrap(); + t.send_input(b"zqsecondqz\n".to_vec()).unwrap(); let mut contents = String::new(); wait_until(Duration::from_secs(5), || { contents = grid(&t.parser).contents(); From 0f449b2bf09b4d373d36c387402b9a1166049005 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 14:39:05 -0700 Subject: [PATCH 06/11] refactor(wire): dump the event object once, guard event decoding, keep failed tests' directories --- src/daemon.rs | 27 +++++-------- src/frame.rs | 2 +- src/path.rs | 6 +-- src/protocol.rs | 81 ++++++++++++++++----------------------- src/protocol_tests.rs | 68 ++++++++++++++++++++++++-------- src/transport.rs | 21 +++++----- tests/common/mod.rs | 29 ++++++++++++-- tests/daemon_env.rs | 5 +-- tests/daemon_handshake.rs | 8 +--- tests/daemon_kill.rs | 4 +- tests/daemon_lifetime.rs | 2 - tests/daemon_recovery.rs | 1 - tests/daemon_resume.rs | 17 ++++---- tests/daemon_session.rs | 2 - tests/daemon_signal.rs | 2 - 15 files changed, 144 insertions(+), 131 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index d3c6256..ee8fe79 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -93,10 +93,6 @@ fn resolve_runtime_dir( tmp.join(format!("fleetcom-{uid}")) } -fn socket_path() -> PathBuf { - socket_in(&runtime_dir()) -} - /// Return the daemon socket path under `dir`. fn socket_in(dir: &Path) -> PathBuf { dir.join("default.sock") @@ -301,6 +297,9 @@ fn connect_or_autostart() -> io::Result<(UnixStream, DaemonOrigin)> { fn connect_or_autostart_in(dir: &Path) -> io::Result<(UnixStream, DaemonOrigin)> { // Validate before connecting because the hello sends the client's // environment and a successful connection skips daemon-side validation. + // `spawn_daemon` depends on it too: creating `daemon.log` inside an + // unvalidated directory could follow a planted symlink and truncate an + // attacker-chosen file before the daemon checks the directory. ensure_runtime_dir(dir)?; let path = socket_in(dir); if let Ok(s) = UnixStream::connect(&path) { @@ -310,7 +309,7 @@ fn connect_or_autostart_in(dir: &Path) -> io::Result<(UnixStream, DaemonOrigin)> // live daemon's accept backlog is momentarily full. // Starting another daemon is safe because the lock permits only one daemon // to bind or reclaim a stale socket. - spawn_daemon()?; + spawn_daemon(dir)?; for _ in 0..100 { if let Ok(s) = UnixStream::connect(&path) { return Ok((s, DaemonOrigin::Autostarted)); @@ -326,14 +325,10 @@ fn connect_or_autostart_in(dir: &Path) -> io::Result<(UnixStream, DaemonOrigin)> )) } -/// Spawn a detached daemon with terminal I/O disconnected. -fn spawn_daemon() -> io::Result<()> { +/// Spawn a detached daemon with terminal I/O disconnected, logging to +/// `daemon.log` under `dir`, which the caller has validated. +fn spawn_daemon(dir: &Path) -> io::Result<()> { let exe = std::env::current_exe()?; - let dir = runtime_dir(); - // Creating `daemon.log` inside an unvalidated directory could follow a planted - // symlink and truncate an attacker-chosen file before the daemon checks the - // directory. Propagate validation failures before opening the log. - ensure_runtime_dir(&dir)?; let log = fs::File::create(dir.join("daemon.log")).ok(); let mut cmd = std::process::Command::new(exe); cmd.arg("--daemon") @@ -437,7 +432,7 @@ fn run_kill_in(dir: &Path) -> io::Result<()> { pub fn run_daemon() -> io::Result<()> { let dir = runtime_dir(); ensure_runtime_dir(&dir)?; // private 0700 directory - let path = socket_path(); + let path = socket_in(&dir); // Only the holder of `daemon.lock` may own the // socket. A concurrent autostart (two clients racing to spawn a daemon) or a @@ -609,15 +604,13 @@ fn serve_client(sup: &mut Supervisor, stream: UnixStream, stop: &AtomicBool) -> match handshake(&mut stream) { Ok(ctx) => { sup.set_launch_context(ctx); - let (kind, payload) = encode_event(&Event::HelloOk); - if write_frame(&mut stream, kind, &payload).is_err() { + if !send_event(&mut stream, &Event::HelloOk) { return ServeOutcome::Disconnected; } } Err(reason) => { eprintln!("fleetcom: refusing client: {reason}"); - let (kind, payload) = encode_event(&Event::Status(reason)); - let _ = write_frame(&mut stream, kind, &payload); + let _ = send_event(&mut stream, &Event::Status(reason)); return ServeOutcome::Disconnected; } } diff --git a/src/frame.rs b/src/frame.rs index 167781c..50566d5 100644 --- a/src/frame.rs +++ b/src/frame.rs @@ -9,7 +9,7 @@ use std::{ time::Duration, }; -/// jzon-encoded control data (a `Command`, or a `Tasks`/`Status` event). +/// jzon-encoded control data (a `Command`, or any event but `Screen`). pub const KIND_CONTROL: u8 = 1; /// A `Screen` event: a jzon header (id, cursor, lines) followed by the raw /// `contents_formatted` bytes, spliced by [`crate::protocol`]. diff --git a/src/path.rs b/src/path.rs index 591ff1c..0725661 100644 --- a/src/path.rs +++ b/src/path.rs @@ -2,9 +2,9 @@ use std::path::{Component, Path, PathBuf}; -/// Shared env key for the runtime directory. Both the socket/lock resolver -/// (`resolve_runtime_dir`) and the capture-root resolver (`runtime_root`) -/// honor it; their fallbacks differ. +/// Shared env key for the runtime directory. The socket/lock resolver +/// (`resolve_runtime_dir`) takes it from the process environment; the capture root +/// takes it from the launch context, else `runtime_root`. Their fallbacks differ. pub const FLEETCOM_RUNTIME_DIR: &str = "FLEETCOM_RUNTIME_DIR"; /// Shorten a path for display: `$HOME` collapses to `~`. Everything else stays diff --git a/src/protocol.rs b/src/protocol.rs index 7c75312..3775c75 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -361,7 +361,7 @@ pub struct ScreenView { // --- wire format ------------------------------------------------------------- // -// Control messages (every `Command`, and the `Tasks`/`Status` events) go over as +// Control messages (every `Command`, and every event but `Screen`) go over as // jzon: low-frequency and human-debuggable. The `Screen` event is the exception: // its `contents_formatted` bytes are the high-frequency firehose, so they ride a // raw tail after a small jzon header rather than bloating into a JSON number @@ -419,14 +419,19 @@ pub(crate) fn insert_opt_str(o: &mut jzon::JsonValue, key: &str, val: &Option Option> { +/// Decode an optional unsigned integer. Return unset (`Some(None)`) for missing or +/// null values, the integer for a number, or `None` to reject any other type, +/// mirroring [`opt_str`]. +fn opt_u64(v: &jzon::JsonValue) -> Option> { if v.is_null() { return Some(None); } - Some(Some(Duration::from_millis(v.as_u64()?))) + Some(Some(v.as_u64()?)) +} + +/// Decode an optional duration in whole milliseconds, with [`opt_u64`]'s contract. +fn opt_ms(v: &jzon::JsonValue) -> Option> { + Some(opt_u64(v)?.map(Duration::from_millis)) } /// Insert `key` only when the optional duration is set; absence encodes @@ -703,11 +708,7 @@ pub fn decode_command(kind: u8, payload: &[u8]) -> Option { on: v["on"].as_bool()?, }, "flagship" => Command::Flagship { - id: if v["id"].is_null() { - None - } else { - Some(v["id"].as_u64()?) - }, + id: opt_u64(&v["id"])?, }, "group" => Command::SetGroup { id: v["id"].as_u64()?, @@ -722,11 +723,7 @@ pub fn decode_command(kind: u8, payload: &[u8]) -> Option { cols: num_from(&v["cols"])?, }, "watch" => Command::Watch { - id: if v["id"].is_null() { - None - } else { - Some(v["id"].as_u64()?) - }, + id: opt_u64(&v["id"])?, // Reject watch frames that do not specify an attachment mode. attached: v["attached"].as_bool()?, }, @@ -822,15 +819,12 @@ pub fn decode_command(kind: u8, payload: &[u8]) -> Option { Some(cmd) } -/// Serialize an event to `(kind, payload)`. `Tasks`/`Status` are jzon control -/// frames; `Screen` is a `KIND_SCREEN` frame (`[u32 header_len][jzon header] +/// Serialize an event to `(kind, payload)`. Every event but `Screen` is a jzon +/// control frame; `Screen` is a `KIND_SCREEN` frame (`[u32 header_len][jzon header] /// [raw formatted bytes]`), so the formatted firehose stays raw. pub fn encode_event(ev: &Event) -> (u8, Vec) { - match ev { - Event::HelloOk => { - let o = jzon::object! { "t": "hello_ok" }; - (KIND_CONTROL, o.dump().into_bytes()) - } + let o = match ev { + Event::HelloOk => jzon::object! { "t": "hello_ok" }, Event::Tasks(views) => { let mut arr = jzon::JsonValue::new_array(); for tv in views { @@ -855,17 +849,10 @@ pub fn encode_event(ev: &Event) -> (u8, Vec) { insert_opt_ms(&mut o, "finished_ms", tv.finished_ago); let _ = arr.push(o); } - let root = jzon::object! { "t": "tasks", "tasks": arr }; - (KIND_CONTROL, root.dump().into_bytes()) - } - Event::Status(msg) => { - let o = jzon::object! { "t": "status", "msg": msg.as_str() }; - (KIND_CONTROL, o.dump().into_bytes()) - } - Event::Spawned { id } => { - let o = jzon::object! { "t": "spawned", "id": *id }; - (KIND_CONTROL, o.dump().into_bytes()) + jzon::object! { "t": "tasks", "tasks": arr } } + Event::Status(msg) => jzon::object! { "t": "status", "msg": msg.as_str() }, + Event::Spawned { id } => jzon::object! { "t": "spawned", "id": *id }, Event::Sessions { names, recovery } => { let mut rec = jzon::JsonValue::new_array(); for r in recovery { @@ -876,23 +863,19 @@ pub fn encode_event(ev: &Event) -> (u8, Vec) { "age": r.age_secs, }); } - let o = jzon::object! { + jzon::object! { "t": "sessions", "names": names.iter().map(String::as_str).collect::>(), "recovery": rec, - }; - (KIND_CONTROL, o.dump().into_bytes()) + } } // Base64 preserves arbitrary clipboard text in the JSON frame. - Event::ClipboardCopy { id, kind, text } => { - let o = jzon::object! { - "t": "clip", - "id": *id, - "k": kind.selector(), - "text": B64.encode(text.as_bytes()), - }; - (KIND_CONTROL, o.dump().into_bytes()) - } + Event::ClipboardCopy { id, kind, text } => jzon::object! { + "t": "clip", + "id": *id, + "k": kind.selector(), + "text": B64.encode(text.as_bytes()), + }, Event::Screen(sv) => { let header = jzon::object! { "id": sv.id, @@ -910,9 +893,10 @@ pub fn encode_event(ev: &Event) -> (u8, Vec) { payload.extend_from_slice(&(hbytes.len() as u32).to_be_bytes()); payload.extend_from_slice(&hbytes); payload.extend_from_slice(&sv.formatted); - (KIND_SCREEN, payload) + return (KIND_SCREEN, payload); } - } + }; + (KIND_CONTROL, o.dump().into_bytes()) } /// Parse an event from a received frame. `None` on any malformed input, mirroring @@ -926,7 +910,6 @@ pub fn decode_event(kind: u8, payload: &[u8]) -> Option { "tasks" => { let mut views = Vec::new(); for tv in v["tasks"].members() { - let lifecycle = lifecycle_from(tv["life"].as_str()?)?; views.push(TaskView { id: tv["id"].as_u64()?, command: tv["command"].as_str()?.to_string(), @@ -936,7 +919,7 @@ pub fn decode_event(kind: u8, payload: &[u8]) -> Option { // Missing and null both mean unassigned/unnamed. group: opt_str(&tv["group"])?, name: opt_str(&tv["name"])?, - lifecycle, + lifecycle: lifecycle_from(tv["life"].as_str()?)?, preview: Preview { text: tv["preview"].as_str()?.to_string(), source: source_from(tv["src"].as_str()?)?, diff --git a/src/protocol_tests.rs b/src/protocol_tests.rs index b0a8546..e0939d6 100644 --- a/src/protocol_tests.rs +++ b/src/protocol_tests.rs @@ -183,12 +183,59 @@ fn command_round_trips() { } } +/// The `command_round_trips` guard for `decode_event`: every event survives +/// encode→frame-payload→decode, on a control frame for all but `Screen`. The +/// socket reader drops an undecodable frame, so an omitted decode arm passes +/// every in-process transport test and loses the event only in daemon mode. #[test] -fn hello_ok_round_trips() { - let ack = Event::HelloOk; - let (k, p) = encode_event(&ack); - assert_eq!(k, KIND_CONTROL); - assert_eq!(decode_event(k, &p), Some(ack)); +fn event_round_trips() { + let screen = r#"{"id":9,"cursor":[3,12],"hide":false,"mouse":true,"alt":false,"ascr":true,"sb":42,"lines":["row0"]}"#; + let cases = [ + Event::HelloOk, + Event::Tasks(vec![tv(1)]), + decode_event(KIND_SCREEN, &screen_payload(screen)).expect("valid screen header"), + Event::Status("saved 'x'".into()), + Event::Spawned { + // An id above `u32::MAX` must survive. + id: u64::from(u32::MAX) + 7, + }, + Event::Sessions { + names: vec!["work".into()], + recovery: Vec::new(), + }, + Event::ClipboardCopy { + id: 1, + kind: ClipboardKind::Clipboard, + text: "hello".into(), + }, + ]; + // Exhaustive: a new variant fails to compile until it is given an index. + fn variant_index(e: &Event) -> usize { + match e { + Event::HelloOk => 0, + Event::Tasks(_) => 1, + Event::Screen(_) => 2, + Event::Status(_) => 3, + Event::Spawned { .. } => 4, + Event::Sessions { .. } => 5, + Event::ClipboardCopy { .. } => 6, + } + } + let mut seen = [false; 7]; + for ev in cases { + seen[variant_index(&ev)] = true; + let (k, p) = encode_event(&ev); + let control = !matches!(ev, Event::Screen(_)); + assert_eq!(k == KIND_CONTROL, control, "frame kind of {ev:?}"); + assert_eq!(decode_event(k, &p).as_ref(), Some(&ev), "round-trip {ev:?}"); + } + for (i, covered) in seen.iter().enumerate() { + assert!( + covered, + "Event variant #{i} (see variant_index) never round-tripped: \ + add a `cases` entry above and its decode arm in decode_event" + ); + } } /// Handshake environment entries and the cwd round-trip byte-for-byte, @@ -403,17 +450,6 @@ fn tasks_and_status_round_trip() { assert_eq!(decode_event(k, &p), Some(status)); } -/// A spawn acknowledgement round-trips with an id above `u32::MAX`. -#[test] -fn spawned_round_trips() { - let ev = Event::Spawned { - id: u64::from(u32::MAX) + 7, - }; - let (k, p) = encode_event(&ev); - assert_eq!(k, KIND_CONTROL); - assert_eq!(decode_event(k, &p), Some(ev)); -} - /// `SetGroup` emits `"g"` only for an assignment. A missing or null `"g"` /// decodes as a clear. #[test] diff --git a/src/transport.rs b/src/transport.rs index 689b0ff..07460f8 100644 --- a/src/transport.rs +++ b/src/transport.rs @@ -258,16 +258,16 @@ impl SocketTransport { Err(RecvTimeoutError::Disconnected) => break, // reader ended } } + self.join_workers(); + } + + /// Join the reader, then drop the queue sender and join the writer. An idle + /// writer exits `recv`; socket closure releases an in-flight write. Call + /// only after the reader ends or `ctrl` shuts down the socket. + fn join_workers(&mut self) { if let Some(h) = self.reader.take() { let _ = h.join(); } - self.join_writer(); - } - - /// Drop the queue sender and join the writer after socket I/O has ended. An - /// idle writer exits `recv`; socket closure releases an in-flight write. - /// Call only after the reader ends or `ctrl` shuts down the socket. - fn join_writer(&mut self) { self.frame_tx.take(); if let Some(h) = self.writer.take() { let _ = h.join(); @@ -317,10 +317,7 @@ impl Transport for SocketTransport { // before both threads are joined. ExitIntent::Disconnect => { let _ = self.ctrl.shutdown(Shutdown::Both); - if let Some(h) = self.reader.take() { - let _ = h.join(); - } - self.join_writer(); + self.join_workers(); } } } @@ -401,7 +398,7 @@ mod tests { t.reader.take().unwrap().join().unwrap(); // Dropping the sender releases the writer if it has not observed the // failed write yet. - t.join_writer(); + t.join_workers(); } /// A peer that stops reading cannot block `send` on the run-loop thread. diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 83cdff7..b0ec2f1 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -181,14 +181,37 @@ impl Drop for KillOnDrop { } } +/// A test daemon's runtime directory. Dropping it removes the tree unless the +/// test is panicking: a failed test keeps its socket, config, and snapshots +/// for inspection. +pub struct RuntimeDir(PathBuf); +impl std::ops::Deref for RuntimeDir { + type Target = Path; + fn deref(&self) -> &Path { + &self.0 + } +} +impl Drop for RuntimeDir { + fn drop(&mut self) { + if !std::thread::panicking() { + let _ = std::fs::remove_dir_all(&self.0); + } + } +} + /// Start a `fleetcom --daemon` against an isolated runtime dir and connect a /// raw socket to it with no handshake, for tests that exercise the handshake /// itself. `configure` tweaks the daemon's `Command` (extra env vars) before /// spawn. +/// +/// The tuple order is load-bearing: pattern bindings drop in reverse +/// declaration order, so binding the directory first drops it last, after +/// `KillOnDrop` has reaped the daemon that owns the files inside. Bind it by +/// name even when unused (`_dir`): a bare `_` drops it at the end of the `let`. pub fn start_daemon_raw( tag: &str, configure: impl FnOnce(&mut Command), -) -> (PathBuf, KillOnDrop, UnixStream) { +) -> (RuntimeDir, KillOnDrop, UnixStream) { let dir = std::env::temp_dir().join(format!("fleetcom_it_{tag}_{}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); @@ -211,7 +234,7 @@ pub fn start_daemon_raw( stream.is_some() }); let stream = stream.expect("daemon never bound its socket"); - (dir, daemon, stream) + (RuntimeDir(dir), daemon, stream) } /// `start_daemon_raw` plus the standard handshake: the connection is ready for @@ -219,7 +242,7 @@ pub fn start_daemon_raw( pub fn start_daemon( tag: &str, configure: impl FnOnce(&mut Command), -) -> (PathBuf, KillOnDrop, UnixStream) { +) -> (RuntimeDir, KillOnDrop, UnixStream) { let (dir, daemon, mut stream) = start_daemon_raw(tag, configure); let cwd = dir.display().to_string(); shake_hands(&mut stream, &cwd); diff --git a/tests/daemon_env.rs b/tests/daemon_env.rs index 5639109..b70cc2b 100644 --- a/tests/daemon_env.rs +++ b/tests/daemon_env.rs @@ -33,9 +33,7 @@ fn spawn_runs_under_the_hello_env() { r#"printf '%s:%s' "$FLEETCOM_MARKER" "${{FLEETCOM_DAEMON_ONLY:-absent}}" > {}"#, out.display() ); - stream - .write_all(&spawn_frame(&command, dir.as_path())) - .unwrap(); + stream.write_all(&spawn_frame(&command, &dir)).unwrap(); let wrote = wait_until(Duration::from_secs(5), || { std::fs::read_to_string(&out).is_ok_and(|c| !c.is_empty()) @@ -49,5 +47,4 @@ fn spawn_runs_under_the_hello_env() { // Clean shutdown; the task already exited on its own. stop_daemon(&mut daemon); - let _ = std::fs::remove_dir_all(&dir); } diff --git a/tests/daemon_handshake.rs b/tests/daemon_handshake.rs index c97b7b7..7c0c2ef 100644 --- a/tests/daemon_handshake.rs +++ b/tests/daemon_handshake.rs @@ -43,7 +43,6 @@ fn version_mismatch_is_refused_with_both_versions_named() { common::shake_hands(&mut retry, &cwd); drop(daemon); - let _ = std::fs::remove_dir_all(&dir); } /// A hello with version 3 but invalid field encoding is still reported as a version @@ -56,7 +55,6 @@ fn v3_hello_is_refused_as_a_version_mismatch() { stream.write_all(&frame(3, v3.as_bytes())).unwrap(); expect_refusal(&mut stream, "v3"); drop(daemon); - let _ = std::fs::remove_dir_all(&dir); } /// A v2 control-frame hello is reported as a version mismatch. @@ -68,19 +66,17 @@ fn v2_hello_is_refused_as_a_version_mismatch() { stream.write_all(&control_frame(&v2)).unwrap(); expect_refusal(&mut stream, "v2"); drop(daemon); - let _ = std::fs::remove_dir_all(&dir); } #[test] fn pre_handshake_command_is_refused() { - let (dir, daemon, mut stream) = start_daemon_raw("nohello", |_| {}); + let (_dir, daemon, mut stream) = start_daemon_raw("nohello", |_| {}); // A command before `Hello` is rejected. stream .write_all(&control_frame(r#"{"t":"resize","rows":40,"cols":120}"#)) .unwrap(); expect_refusal(&mut stream, "hello"); drop(daemon); - let _ = std::fs::remove_dir_all(&dir); } #[test] @@ -105,7 +101,6 @@ fn silent_client_cannot_wedge_the_daemon() { assert!(served_next, "daemon wedged behind a silent connection"); drop(stream); drop(daemon); - let _ = std::fs::remove_dir_all(&dir); } /// The documented single-client semantics: a second client's hello gets no @@ -155,5 +150,4 @@ fn second_client_queues_until_first_detaches() { ); drop(daemon); - let _ = std::fs::remove_dir_all(&dir); } diff --git a/tests/daemon_kill.rs b/tests/daemon_kill.rs index 9bc39e5..d6335c8 100644 --- a/tests/daemon_kill.rs +++ b/tests/daemon_kill.rs @@ -29,7 +29,7 @@ fn kill_works_while_a_client_is_attached() { // Keep `stream` open while `fleetcom --kill` runs. let mut killer = Command::new(env!("CARGO_BIN_EXE_fleetcom")) .arg("--kill") - .env("FLEETCOM_RUNTIME_DIR", &dir) + .env("FLEETCOM_RUNTIME_DIR", &*dir) .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()) @@ -60,6 +60,4 @@ fn kill_works_while_a_client_is_attached() { "task survived --kill" ); assert!(!sock.exists(), "socket file left behind"); - - let _ = std::fs::remove_dir_all(&dir); } diff --git a/tests/daemon_lifetime.rs b/tests/daemon_lifetime.rs index 896e46e..68112c9 100644 --- a/tests/daemon_lifetime.rs +++ b/tests/daemon_lifetime.rs @@ -34,7 +34,6 @@ fn ordinary_tasks_die_with_a_sigkilled_daemon() { wait_until(Duration::from_secs(5), || kill(task, None).is_err()), "an ordinary task must die with the daemon (PTY hangup)" ); - let _ = std::fs::remove_dir_all(&dir); } #[test] @@ -66,5 +65,4 @@ fn hup_immune_tasks_survive_a_sigkilled_daemon_unowned() { survived, "a HUP-immune task should have outlived the daemon (unowned)" ); - let _ = std::fs::remove_dir_all(&dir); } diff --git a/tests/daemon_recovery.rs b/tests/daemon_recovery.rs index 73ad41a..909e8da 100644 --- a/tests/daemon_recovery.rs +++ b/tests/daemon_recovery.rs @@ -64,5 +64,4 @@ fn detached_daemon_writes_the_pending_snapshot() { } stop_daemon(&mut daemon); - let _ = std::fs::remove_dir_all(&dir); } diff --git a/tests/daemon_resume.rs b/tests/daemon_resume.rs index 0b3643b..0d8437c 100644 --- a/tests/daemon_resume.rs +++ b/tests/daemon_resume.rs @@ -99,7 +99,10 @@ impl Scratch { impl Drop for Scratch { fn drop(&mut self) { - let _ = std::fs::remove_dir_all(&self.root); + // A failed test keeps its stubs, stores, and argv records for inspection. + if !std::thread::panicking() { + let _ = std::fs::remove_dir_all(&self.root); + } } } @@ -266,7 +269,7 @@ fn has_capture(runtime: &Path) -> bool { fn claude_spawn_save_load_resumes_the_conversation() { let s = Scratch::new("claude"); install_claude_stub(&s); - let (dir, mut daemon, mut stream) = start_daemon_raw("resume_claude", |_| {}); + let (_dir, mut daemon, mut stream) = start_daemon_raw("resume_claude", |_| {}); hello(&mut stream, &s); drain_events(&stream); @@ -310,7 +313,6 @@ fn claude_spawn_save_load_resumes_the_conversation() { ); stop_daemon(&mut daemon); - let _ = std::fs::remove_dir_all(&dir); } /// A Codex capture payload persists the resume command, and loading that command @@ -319,7 +321,7 @@ fn claude_spawn_save_load_resumes_the_conversation() { fn codex_capture_file_drives_save_and_load_resumes() { let s = Scratch::new("codex"); install_codex_stub(&s); - let (dir, mut daemon, mut stream) = start_daemon_raw("resume_codex", |_| {}); + let (_dir, mut daemon, mut stream) = start_daemon_raw("resume_codex", |_| {}); hello(&mut stream, &s); drain_events(&stream); @@ -362,7 +364,6 @@ fn codex_capture_file_drives_save_and_load_resumes() { ); stop_daemon(&mut daemon); - let _ = std::fs::remove_dir_all(&dir); } /// A persisted resume command survives daemon replacement and targets the same @@ -373,7 +374,7 @@ fn saved_recipe_resumes_across_a_daemon_restart() { install_claude_stub(&s); let rec = s.record("claude"); - let (dir_a, mut daemon_a, mut stream_a) = start_daemon_raw("resume_restart_a", |_| {}); + let (_dir_a, mut daemon_a, mut stream_a) = start_daemon_raw("resume_restart_a", |_| {}); hello(&mut stream_a, &s); drain_events(&stream_a); stream_a @@ -388,10 +389,9 @@ fn saved_recipe_resumes_across_a_daemon_restart() { ); stop_daemon(&mut daemon_a); drop(stream_a); - let _ = std::fs::remove_dir_all(&dir_a); // Start another daemon with the same config and handshake environment. - let (dir_b, mut daemon_b, mut stream_b) = start_daemon_raw("resume_restart_b", |_| {}); + let (_dir_b, mut daemon_b, mut stream_b) = start_daemon_raw("resume_restart_b", |_| {}); hello(&mut stream_b, &s); drain_events(&stream_b); stream_b @@ -409,5 +409,4 @@ fn saved_recipe_resumes_across_a_daemon_restart() { ); stop_daemon(&mut daemon_b); - let _ = std::fs::remove_dir_all(&dir_b); } diff --git a/tests/daemon_session.rs b/tests/daemon_session.rs index 4fba788..dd0ce35 100644 --- a/tests/daemon_session.rs +++ b/tests/daemon_session.rs @@ -43,7 +43,6 @@ fn load_session_resolves_relative_dirs_against_the_client_cwd() { ); stop_daemon(&mut daemon); - let _ = std::fs::remove_dir_all(&dir); let _ = std::fs::remove_dir_all(&config); } @@ -112,7 +111,6 @@ fn session_commands_follow_the_hello_config_dir() { ); stop_daemon(&mut daemon); - let _ = std::fs::remove_dir_all(&dir); let _ = std::fs::remove_dir_all(&daemon_cfg); let _ = std::fs::remove_dir_all(&client_cfg); } diff --git a/tests/daemon_signal.rs b/tests/daemon_signal.rs index b57e9f1..923ea1e 100644 --- a/tests/daemon_signal.rs +++ b/tests/daemon_signal.rs @@ -41,6 +41,4 @@ fn sigterm_kills_daemon_and_its_tasks() { // Clean shutdown removes the socket. assert!(!sock.exists(), "socket file left behind"); - - let _ = std::fs::remove_dir_all(&dir); } From 9e447467c3291759a07a807e51b338ae898c0456 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 15:48:30 -0700 Subject: [PATCH 07/11] refactor(docs): improve clarity and structure in agent resume documentation --- docs/agent-resume.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/agent-resume.md b/docs/agent-resume.md index b9947c4..989001f 100644 --- a/docs/agent-resume.md +++ b/docs/agent-resume.md @@ -68,7 +68,9 @@ You cannot choose a Codex ID at launch. A notify override is injected into both After each turn, the notifier writes the `agent-turn-complete` JSON argument to `FLEETCOM_CAPTURE_FILE`, and the harness reads `thread-id` from it. This captures an in-TUI session change after a turn completes in the resumed conversation. -Replacing a configured notifier would change user behavior, so the harness reads bare top-level keys in `$CODEX_HOME/config.toml` until the first table header. If it finds a one-line `notify` array of non-empty basic strings, it chains that notifier after the capture write. `FLEETCOM_NOTIFY_CHAIN` carries its argv joined by newlines; the capture script appends the notification payload before invoking it. An absent setting or empty array means capture runs alone. This encoding cannot transport empty arguments, newlines, or NUL, so those values disable injection. Injection is also disabled, and no ID is captured, when `config.toml` exists but cannot be read, or when a line above the first table header is not a bare key assigned a single-line value the reader recognizes. +To preserve a configured notifier, read bare top-level keys in `$CODEX_HOME/config.toml` up to the first table header. For a one-line `notify` array of non-empty basic strings, invoke the notifier after writing the capture file. Store its argv in `FLEETCOM_NOTIFY_CHAIN`, joined by newlines, and append the notification payload before invocation. With an absent setting or empty array, run only the capture hook. + +Do not inject the capture hook for empty arguments, newlines, or NUL: these cannot be represented in the chain encoding. Also skip injection when `config.toml` exists but cannot be read, or when a line above the first table header is not a bare key assigned a recognized single-line value. No ID is captured in these cases. Live validation on 2026-09-28 used the official macOS arm64 Codex 0.157.1 and 0.158.0 executables with isolated homes and a scripted loopback Responses server. In both releases, the real capture script preserved the notification JSON and passed it to the configured notifier. Saving and loading through foreground fleetcom preserved the thread ID and reopened its history. These runs used `--no-daemon`; separate default-launch probes also captured and saved the ID, but their fresh homes had no managed Codex daemon installed or running. Shared-daemon behavior and real-account behavior remain unverified. @@ -121,7 +123,7 @@ Every captured value eventually enters a shell command, so validation accepts on ## Extending capture -Implement the `Harness` trait in [`src/harness/mod.rs`](../src/harness/mod.rs) for each tool. Register it in that file's `AGENTS` table, the only list detection consults; each entry also requires a `SummaryAdapter` for the dashboard preview, as implemented in [`src/harness/summary.rs`](../src/harness/summary.rs). Separate detection, evidence collection, and command construction through these methods: +Implement the `Harness` trait in [`src/harness/mod.rs`](../src/harness/mod.rs) for each tool. Register it in that file's `AGENTS` table, the sole registry for detection. Include a `SummaryAdapter` for the dashboard preview with each entry; see the implementations in [`src/harness/summary.rs`](../src/harness/summary.rs). Separate detection, evidence collection, and command construction through these methods: - `shape`: supply the program word and resume selector. Accepted and canonical forms are derived from that pair by the default `detect` and `resume_command` implementations. - `instrument`: return spawn-time arguments, environment entries, and an optional pinned ID. From bfd33446d3dff193b9a3d3c4d73ecc7efa2003e2 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 15:48:47 -0700 Subject: [PATCH 08/11] refactor(summary): enhance clarity in comments for frame handling and Codex summary adapter --- src/harness/assets.rs | 4 ++-- src/harness/codex.rs | 2 +- src/harness/summary.rs | 19 ++++++++++--------- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/harness/assets.rs b/src/harness/assets.rs index e271806..ebc8d66 100644 --- a/src/harness/assets.rs +++ b/src/harness/assets.rs @@ -108,8 +108,8 @@ fn claude_settings_json() -> String { /// Platform capture root: `dirs::runtime_dir()/fleetcom`, then /// `dirs::cache_dir()/fleetcom/run`. Those fallbacks are not -/// `daemon::resolve_runtime_dir`. The supervisor calls this only when the -/// launch context carries no `FLEETCOM_RUNTIME_DIR`. +/// `daemon::resolve_runtime_dir`. Call this from the supervisor only when +/// `FLEETCOM_RUNTIME_DIR` is absent from the launch context. pub fn runtime_root() -> Option { if let Some(run) = dirs::runtime_dir() { return Some(run.join("fleetcom")); diff --git a/src/harness/codex.rs b/src/harness/codex.rs index 55dca49..00d77cf 100644 --- a/src/harness/codex.rs +++ b/src/harness/codex.rs @@ -137,7 +137,7 @@ fn config_notify_route(home: Option<&Path>) -> NotifyRoute { } /// Recognize complete single-line values without interpreting unrelated settings. -/// Arrays hold basic strings only; multiline values and inline tables are out of scope. +/// Accept only basic strings in arrays; exclude multiline values and inline tables. fn complete_value(value: &str) -> bool { if value.starts_with("\"\"\"") || value.starts_with("'''") { return false; diff --git a/src/harness/summary.rs b/src/harness/summary.rs index 625c9d0..81d20a6 100644 --- a/src/harness/summary.rs +++ b/src/harness/summary.rs @@ -44,7 +44,8 @@ fn braille_frame(c: char) -> bool { ('\u{2800}'..='\u{28FF}').contains(&c) } -/// The text after a leading frame char accepted by `is_frame` and one space. +/// Return the text after a leading frame character accepted by `is_frame` +/// and a single space. fn after_frame(s: &str, is_frame: impl Fn(char) -> bool) -> Option<&str> { let mut chars = s.chars(); (is_frame(chars.next()?) && chars.next()? == ' ').then_some(chars.as_str()) @@ -120,9 +121,9 @@ impl SummaryAdapter for ClaudeSummary { } } -/// Whether `row` is a full-width horizontal rule: nothing but `─`, long -/// enough that box borders and inline list rules never qualify. claude's -/// input box is fenced by two such rows. +/// Match a full-width horizontal rule: at least 40 `─` characters after trimming. +/// Exclude box borders and short inline rules. Claude's input box is fenced +/// by two full-width rules. fn is_rule_row(row: &str) -> bool { let mut n = 0usize; for c in row.trim().chars() { @@ -330,11 +331,11 @@ const CODEX_EFFORT: &[&str] = &[ /// depth, so counting them would push the status row out of reach. const CODEX_STATUS_WINDOW: usize = 10; -/// codex (inline UI on the primary screen; approval modals also paint on the alternate -/// screen). The pin is its composer: the bottom-most column-0 prompt-glyph row that is -/// not a modal selector; status rows sit above it, and scrollback beyond the first -/// foreign row is out of bounds. Check for the approval modal first, with the composer -/// absent. The status line or indented hint rows may appear below the composer. +/// Codex summary adapter for the primary-screen inline UI and approval modals +/// on either screen. Anchor at the composer: the bottom-most column-0 prompt-glyph +/// row that is not a modal selector. Scan above it for status rows, stopping at +/// the first foreign row. Check for an approval modal first, with the composer +/// absent. Also check below the composer for a status line or indented hints. pub struct CodexSummary; impl SummaryAdapter for CodexSummary { From 8d6c6a7a5e63d5acc733a01b53085ee533ccf3a1 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 15:48:55 -0700 Subject: [PATCH 09/11] refactor(docs): improve clarity in comments for emulator, golden, and input modules --- src/terminal/emulator.rs | 27 ++++++++++++++------------- src/terminal/golden.rs | 2 +- src/terminal/input.rs | 16 ++++++++-------- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/terminal/emulator.rs b/src/terminal/emulator.rs index aa5edf3..ab12e9f 100644 --- a/src/terminal/emulator.rs +++ b/src/terminal/emulator.rs @@ -469,9 +469,9 @@ impl Emulator { /// Capture accessors for the dashboard-preview resolution layer /// (`crate::preview`). impl Emulator { - /// Monotonic count of grid advances. Bumps on every `process` call, on - /// each sync-frame landing, and on `resize`; equal reads mean the grid did - /// not advance in between, so a poller can skip re-reading it. + /// Monotonic count of grid advances: incremented on every `process` call, + /// each sync-frame landing, and `resize`. If unchanged between polls, + /// skip re-reading the grid. pub fn revision(&self) -> u64 { self.revision } @@ -545,13 +545,13 @@ fn live_floor_of(term: &Term) -> String { } /// Plain text of one live-viewport row: wide-character spacers omitted, tabs -/// mapped to spaces, combining marks preserved, trailing padding trimmed. Rows -/// `0..screen_lines` address live output regardless of the display -/// offset; only display iteration follows the offset. +/// mapped to spaces, combining marks preserved, trailing padding trimmed. +/// Index rows `0..screen_lines` for live output regardless of the display +/// offset; apply the offset only when iterating for display. /// -/// Unlike [`crate::ansi::contents`], this scan view preserves glyphs in -/// concealed (SGR 8) cells and orphaned wide halves. Harness matchers inspect -/// stored grid text, not replay-equivalent display text. +/// Unlike [`crate::ansi::contents`], preserve glyphs in concealed (SGR 8) cells +/// and orphaned wide halves: match harness output against stored grid text, +/// not replay-equivalent display text. fn live_row_text_of(term: &Term, row: i32) -> String { let mut text = String::new(); for cell in &term.grid()[Line(row)] { @@ -612,10 +612,11 @@ const TITLE_STACK_SHADOW_MAX: usize = 4096; /// /// # Forwarding invariant /// -/// `Handler` methods default to no-ops, so every method must delegate to `Term`. -/// The `golden::wrapper_*` tests detect missing delegation: each replays one raw fixture -/// through wrapper and raw backend, then compares styled bytes, text, cursor, and alt bit. -/// `clipboard_store` is captured by this wrapper instead of delegated. +/// Delegate every `Handler` method to `Term` to avoid the default no-op. +/// Check delegation in `golden::wrapper_*`: replay each raw fixture through +/// the wrapper and raw backend, then compare styled bytes, text, cursor, and +/// alternate-screen state. Capture `clipboard_store` in this wrapper instead +/// of delegating it. /// /// # Synchronized updates /// diff --git a/src/terminal/golden.rs b/src/terminal/golden.rs index 9ee448d..85118d6 100644 --- a/src/terminal/golden.rs +++ b/src/terminal/golden.rs @@ -584,7 +584,7 @@ fn assert_wrapper_matches(file: &str, bytes: &[u8]) { emu.process(bytes); let (al_bytes, al_cursor, al_hidden) = ansi::formatted(&al); let (emu_bytes, emu_cursor, emu_hidden) = emu.formatted(); - // `assert!`, not `assert_eq!`: a failure must not dump two serialized screens. + // Use `assert!` to omit both serialized screens from the failure output. assert!(emu_bytes == al_bytes, "{file}: styled cells"); assert_eq!(emu.contents(), ansi::contents(&al), "{file}: screen"); assert_eq!( diff --git a/src/terminal/input.rs b/src/terminal/input.rs index efad8ff..b2e0816 100644 --- a/src/terminal/input.rs +++ b/src/terminal/input.rs @@ -1,7 +1,7 @@ //! The input-direction mirror of `ansi`: protocol events in, VT byte //! sequences out. Encoding only: no PTY writes and no `Task` state. -//! `key_bytes` and `paste_bytes` take the child's negotiated modes as values; -//! `mouse_bytes` reads its four off the `Emulator` its caller holds locked. +//! Pass the child's negotiated modes as values to `key_bytes` and `paste_bytes`. +//! For `mouse_bytes`, hold the `Emulator` lock while reading its four mouse modes. use crate::{ emulator::{Emulator, MouseProtocolEncoding, MouseProtocolMode}, @@ -151,9 +151,9 @@ fn char_bytes(c: char, mods: Mods) -> Option> { Some(out) } -/// The letter-final shape of cursor keys, Home/End, and F1–F4: SS3 `letter` when -/// unmodified under `ss3`, CSI `letter` when unmodified otherwise, CSI `1;m letter` -/// when modified. +/// Encode cursor keys, Home/End, and F1–F4 with a final letter. For unmodified +/// keys, use SS3 when `ss3` is true and CSI otherwise. With modifiers, use +/// CSI `1;{m}{letter}`. fn letter_bytes(ss3: bool, letter: char, m: Option) -> Vec { match m { None if ss3 => format!("\x1bO{letter}"), @@ -163,8 +163,8 @@ fn letter_bytes(ss3: bool, letter: char, m: Option) -> Vec { .into_bytes() } -/// The tilde-final shape of the navigation cluster and F5–F12: CSI `n~`, or CSI -/// `n;m~` when modified. +/// Encode the navigation cluster and F5–F12 as CSI `n~`, or CSI `n;m~` +/// with modifiers. fn tilde_bytes(n: u8, m: Option) -> Vec { match m { None => format!("\x1b[{n}~"), @@ -225,7 +225,7 @@ pub fn key_bytes(app_cursor: bool, code: Key, mods: Mods) -> Option> { Key::Left => Some(letter_bytes(app_cursor, 'D', m)), Key::Home => Some(letter_bytes(app_cursor, 'H', m)), Key::End => Some(letter_bytes(app_cursor, 'F', m)), - // Navigation-cluster keys always use CSI `~`. + // Encode navigation-cluster keys with CSI `~`. Key::Insert => Some(tilde_bytes(2, m)), Key::Delete => Some(tilde_bytes(3, m)), Key::PageUp => Some(tilde_bytes(5, m)), From 9d5abd097c01d12262244603cb6a17229a3a9f3f Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 15:49:09 -0700 Subject: [PATCH 10/11] refactor(docs): enhance clarity in comments across multiple modules --- src/app.rs | 10 +++++----- src/app_readme_tests.rs | 4 ++-- src/daemon.rs | 10 +++++----- src/path.rs | 7 ++++--- src/protocol.rs | 10 +++++----- src/protocol_tests.rs | 12 ++++++------ src/supervisor.rs | 21 ++++++++++----------- src/supervisor_tests.rs | 4 ++-- src/transport.rs | 7 ++++--- src/ui.rs | 16 ++++++++-------- 10 files changed, 51 insertions(+), 50 deletions(-) diff --git a/src/app.rs b/src/app.rs index e3b8746..c22d3c3 100644 --- a/src/app.rs +++ b/src/app.rs @@ -288,9 +288,9 @@ pub struct App { /// Set by an external SIGTERM/SIGHUP/SIGINT; the loop treats it as quit so /// teardown runs and the terminal is restored. term_signal: Arc, - /// Set to leave the run loop, and how: `q`/Ctrl-C/signals disconnect - /// (daemon + tasks survive), `Q` quits and kills. `shutdown` reads `None` as - /// the safe `Disconnect` so an unexpected exit never reaps the daemon. + /// Pending exit intent: disconnect on `q`, Ctrl-C, or a terminating signal; + /// kill tasks and stop the daemon on `Q`. In `shutdown`, default to + /// `Disconnect` for `None` to preserve the daemon after an unexpected exit. exit_intent: Option, /// Whether the client currently captures terminal mouse events. mouse_captured: bool, @@ -1935,7 +1935,7 @@ fn is_controls_key(k: KeyEvent) -> bool { } } -/// Apply prompt editing keys. Returns `true` for Backspace or an inserted character, +/// Apply prompt editing keys. Return `true` for Backspace or an inserted character, /// `false` for caret motion, ignored Ctrl chords, and unsupported keys. On Ctrl-A or /// Ctrl-E, move to the start or end. fn on_key_edit(buf: &mut EditBuffer, k: KeyEvent) -> bool { @@ -1955,7 +1955,7 @@ fn on_key_edit(buf: &mut EditBuffer, k: KeyEvent) -> bool { KeyCode::End => buf.end(), KeyCode::Char('a') if ctrl => buf.home(), KeyCode::Char('e') if ctrl => buf.end(), - // Ignore unsupported keys, and unbound Ctrl chords without inserting their character. + // Ignore unsupported keys and unbound Ctrl chords without inserting a character. _ => {} } false diff --git a/src/app_readme_tests.rs b/src/app_readme_tests.rs index fc9770d..9b9ddb8 100644 --- a/src/app_readme_tests.rs +++ b/src/app_readme_tests.rs @@ -5,8 +5,8 @@ use super::*; -/// Fixture dimensions. Thirty rows fit every section; 107 columns produce a -/// 72-column preview cell and an 80-column peek box. +/// Use 30 rows to fit every section and 107 columns for a 72-column preview +/// cell and an 80-column peek box. const FIXTURE_ROWS: u16 = 30; const FIXTURE_COLS: u16 = 107; diff --git a/src/daemon.rs b/src/daemon.rs index ee8fe79..e1c4aa2 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -295,11 +295,11 @@ fn connect_or_autostart() -> io::Result<(UnixStream, DaemonOrigin)> { /// Validate `dir`, connect or autostart, and report which path succeeded. fn connect_or_autostart_in(dir: &Path) -> io::Result<(UnixStream, DaemonOrigin)> { - // Validate before connecting because the hello sends the client's - // environment and a successful connection skips daemon-side validation. - // `spawn_daemon` depends on it too: creating `daemon.log` inside an - // unvalidated directory could follow a planted symlink and truncate an - // attacker-chosen file before the daemon checks the directory. + // Validate before connecting: the client's environment is sent in the + // hello, without daemon-side directory validation on an existing connection. + // Validate before `spawn_daemon` too: when creating `daemon.log` in an + // unvalidated directory, we could follow a planted symlink and truncate an + // attacker-chosen file before directory validation in the new daemon. ensure_runtime_dir(dir)?; let path = socket_in(dir); if let Ok(s) = UnixStream::connect(&path) { diff --git a/src/path.rs b/src/path.rs index 0725661..88e2b86 100644 --- a/src/path.rs +++ b/src/path.rs @@ -2,9 +2,10 @@ use std::path::{Component, Path, PathBuf}; -/// Shared env key for the runtime directory. The socket/lock resolver -/// (`resolve_runtime_dir`) takes it from the process environment; the capture root -/// takes it from the launch context, else `runtime_root`. Their fallbacks differ. +/// Shared env key for the runtime directory. For sockets and locks, read it +/// from the process environment in `resolve_runtime_dir`. For captures, read +/// it from the launch context, falling back to `runtime_root`. The fallback +/// directories differ between these uses. pub const FLEETCOM_RUNTIME_DIR: &str = "FLEETCOM_RUNTIME_DIR"; /// Shorten a path for display: `$HOME` collapses to `~`. Everything else stays diff --git a/src/protocol.rs b/src/protocol.rs index 3775c75..2d4467a 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -361,11 +361,11 @@ pub struct ScreenView { // --- wire format ------------------------------------------------------------- // -// Control messages (every `Command`, and every event but `Screen`) go over as -// jzon: low-frequency and human-debuggable. The `Screen` event is the exception: -// its `contents_formatted` bytes are the high-frequency firehose, so they ride a -// raw tail after a small jzon header rather than bloating into a JSON number -// array. A socket peer is just `decode_*(read_frame(...))`. +// Encode control messages (every `Command`, and every event but `Screen`) as +// jzon: low-frequency and human-debuggable. For the high-frequency `Screen` +// events, append raw `contents_formatted` bytes after a small jzon header to +// avoid expansion into a JSON number array. Decode at the socket peer with +// `decode_*(read_frame(...))`. /// Encode an `OsStr` as lossless base64 for a JSON string. fn os_b64(s: &OsStr) -> String { diff --git a/src/protocol_tests.rs b/src/protocol_tests.rs index e0939d6..57dc843 100644 --- a/src/protocol_tests.rs +++ b/src/protocol_tests.rs @@ -183,10 +183,9 @@ fn command_round_trips() { } } -/// The `command_round_trips` guard for `decode_event`: every event survives -/// encode→frame-payload→decode, on a control frame for all but `Screen`. The -/// socket reader drops an undecodable frame, so an omitted decode arm passes -/// every in-process transport test and loses the event only in daemon mode. +/// Round-trip every event through encoding and decoding, using control frames +/// for all but `Screen`. In-process transport tests do not exercise decoding: +/// without a decode arm, the event is discarded as undecodable in daemon mode. #[test] fn event_round_trips() { let screen = r#"{"id":9,"cursor":[3,12],"hide":false,"mouse":true,"alt":false,"ascr":true,"sb":42,"lines":["row0"]}"#; @@ -196,7 +195,7 @@ fn event_round_trips() { decode_event(KIND_SCREEN, &screen_payload(screen)).expect("valid screen header"), Event::Status("saved 'x'".into()), Event::Spawned { - // An id above `u32::MAX` must survive. + // Preserve IDs above `u32::MAX` through the round trip. id: u64::from(u32::MAX) + 7, }, Event::Sessions { @@ -209,7 +208,8 @@ fn event_round_trips() { text: "hello".into(), }, ]; - // Exhaustive: a new variant fails to compile until it is given an index. + // Assign an index to every variant; leave the match exhaustive so adding + // a variant without an index is a compile error. fn variant_index(e: &Event) -> usize { match e { Event::HelloOk => 0, diff --git a/src/supervisor.rs b/src/supervisor.rs index 96824a8..0cd4810 100644 --- a/src/supervisor.rs +++ b/src/supervisor.rs @@ -1,9 +1,8 @@ -//! The task owner: holds every `Task`, allocates ids, reaps exits, and answers -//! `Command`s with `Event`s. It speaks only `protocol` types, never UI state. -//! A serving loop drives it through `apply` (one `Command`), `tick` (reap, then -//! emit a task snapshot plus the watched screen), and `drain` (take the queued -//! `Event`s). Between clients the daemon calls `reap` and -//! `recovery_maintenance` directly. +//! Task ownership, ID allocation, exit reaping, and `Command` handling through +//! `Event` responses. Use only `protocol` types at this boundary, never UI state. +//! In the serving loop, call `apply` for one `Command`, `tick` to reap and emit +//! a task snapshot plus the watched screen, and `drain` to take queued `Event`s. +//! Between clients, call `reap` and `recovery_maintenance` directly in the daemon. use std::{ collections::BTreeMap, @@ -202,8 +201,8 @@ fn affects_recipe(cmd: &Command) -> bool { struct Recovery { /// Recovery writes are opt-in in unit tests. enabled: bool, - /// The most recent potentially recipe-changing command still awaiting a - /// debounced pass: the debounce anchor. `None` once a pass has run. + /// Time of the most recent potentially recipe-changing command pending + /// a debounced pass. Use as the debounce anchor; clear after the pass. last_mutation: Option, /// The start of the most recent cadence interval. last_cadence: Instant, @@ -472,9 +471,9 @@ impl Supervisor { } /// One step of the core's own loop: reap exits, then emit a fresh task - /// snapshot (plus the watched task's screen). `core::run_loop` is the only - /// production caller, in process and in the daemon alike; the client only - /// ever sees `drain`ed events, never a `Task`. + /// snapshot (plus the watched task's screen). Call only from `core::run_loop` + /// in production, both in process and in the daemon. Access state from the + /// client through `drain`ed events, never through a `Task`. pub fn tick(&mut self) { self.reap(); // Check current state rather than individual transitions to handle exit, diff --git a/src/supervisor_tests.rs b/src/supervisor_tests.rs index 2004214..45ee31a 100644 --- a/src/supervisor_tests.rs +++ b/src/supervisor_tests.rs @@ -2034,7 +2034,7 @@ fn recovery_files(config: &Path) -> Vec { names } -/// Read and clear whether a recipe change awaits a debounced pass. +/// Check for a pending debounced recipe pass, then clear its timestamp. fn take_dirty(s: &mut Supervisor) -> bool { s.recovery.last_mutation.take().is_some() } @@ -2049,7 +2049,7 @@ fn recovery_arms_on_structural_mutations_not_tag() { assert!(take_dirty(&mut s), "Spawn must arm"); let id = first_id(&mut s); - s.recovery.last_mutation = None; // first_id ticks; reassert a clean baseline + s.recovery.last_mutation = None; // Reset after the tick in first_id. s.apply(Command::Tag { id, on: true }); assert!( !take_dirty(&mut s), diff --git a/src/transport.rs b/src/transport.rs index 07460f8..b6b442d 100644 --- a/src/transport.rs +++ b/src/transport.rs @@ -261,9 +261,10 @@ impl SocketTransport { self.join_workers(); } - /// Join the reader, then drop the queue sender and join the writer. An idle - /// writer exits `recv`; socket closure releases an in-flight write. Call - /// only after the reader ends or `ctrl` shuts down the socket. + /// Join the reader, then drop the queue sender and join the writer. Drop + /// the sender to unblock an idle writer in `recv`; close the socket to + /// unblock an in-flight write. Call only after the reader has ended or + /// after shutting down the socket through `ctrl`. fn join_workers(&mut self) { if let Some(h) = self.reader.take() { let _ = h.join(); diff --git a/src/ui.rs b/src/ui.rs index d86b280..88990c0 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -87,8 +87,8 @@ fn dim(out: &mut impl Write, y: u16, s: &str, cols: usize) -> io::Result<()> { ) } -/// Paint a full-width highlight line: reverse video while the host terminal has focus, a dark-grey -/// background while it does not, so an unfocused terminal shows no full-strength selection. +/// Paint a full-width highlight line: use reverse video with host-terminal +/// focus and a muted dark-grey background without it. fn highlight(out: &mut impl Write, y: u16, s: &str, cols: usize, focused: bool) -> io::Result<()> { queue!(out, MoveTo(0, y))?; if focused { @@ -481,10 +481,10 @@ fn render_peek(out: &mut impl Write, app: &App) -> io::Result<()> { ) } -/// The peek body: at most `height` lines, cropped from the bottom of a window. On the alternate -/// screen the window is the whole grid: a partial repaint leaves real blank canvas rows, and -/// dropping them would shift the view. On the primary screen it ends at the last non-blank row: -/// trailing blanks are grid padding, so short output renders from its first row. +/// Return at most `height` lines from the bottom of the peek window. On the +/// alternate screen, include the whole grid: preserve blank rows after partial +/// repaints to avoid shifting the view. On the primary screen, end at the last +/// non-blank row to omit grid padding and show short output from its first row. fn peek_window(lines: &[String], height: usize, alt_screen: bool) -> &[String] { // `contents()` trims trailing padding, so a blank row is exactly empty. let end = if alt_screen { @@ -896,8 +896,8 @@ fn render_session_picker(out: &mut impl Write, app: &App) -> io::Result<()> { ) } -/// Indent `s` to center it in `width` columns. The caller's `put`/`dim` clips -/// and pads the result to the row. +/// Indent `s` to center it in `width` columns. Clip and pad to the row with +/// `put` or `dim` at the call site. fn center(s: &str, width: usize) -> String { format!("{}{s}", " ".repeat(width.saturating_sub(s.width()) / 2)) } From 3c834888487369ac1a212ead5021004319bd9f4e Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sun, 4 Oct 2026 15:49:15 -0700 Subject: [PATCH 11/11] refactor(docs): improve clarity in comments for runtime directory and scratch tree handling --- tests/common/mod.rs | 13 ++++++------- tests/daemon_resume.rs | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index b0ec2f1..3dac5a1 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -181,9 +181,8 @@ impl Drop for KillOnDrop { } } -/// A test daemon's runtime directory. Dropping it removes the tree unless the -/// test is panicking: a failed test keeps its socket, config, and snapshots -/// for inspection. +/// A test daemon's runtime directory. Remove the tree on drop, except during +/// unwinding: preserve the socket, config, and snapshots to inspect after failure. pub struct RuntimeDir(PathBuf); impl std::ops::Deref for RuntimeDir { type Target = Path; @@ -204,10 +203,10 @@ impl Drop for RuntimeDir { /// itself. `configure` tweaks the daemon's `Command` (extra env vars) before /// spawn. /// -/// The tuple order is load-bearing: pattern bindings drop in reverse -/// declaration order, so binding the directory first drops it last, after -/// `KillOnDrop` has reaped the daemon that owns the files inside. Bind it by -/// name even when unused (`_dir`): a bare `_` drops it at the end of the `let`. +/// Bind the directory first so it is dropped last: pattern bindings are +/// dropped in reverse declaration order. Reap the daemon through `KillOnDrop` +/// before removing its runtime files. Use a named binding even when unused +/// (`_dir`); with a bare `_`, the directory is dropped at the end of the `let`. pub fn start_daemon_raw( tag: &str, configure: impl FnOnce(&mut Command), diff --git a/tests/daemon_resume.rs b/tests/daemon_resume.rs index 0d8437c..1ff998b 100644 --- a/tests/daemon_resume.rs +++ b/tests/daemon_resume.rs @@ -99,7 +99,7 @@ impl Scratch { impl Drop for Scratch { fn drop(&mut self) { - // A failed test keeps its stubs, stores, and argv records for inspection. + // Preserve stubs, stores, and argv records for inspection after failure. if !std::thread::panicking() { let _ = std::fs::remove_dir_all(&self.root); }