# Quality Refactor Implementation Plan

> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.

**Goal:** Fix all quality issues identified in the codebase audit: critical bugs, code duplication, Grid encapsulation, monolith decomposition, and miscellaneous improvements.

**Architecture:** Incremental refactoring in 5 phases, each ending with `cargo test` to catch regressions. Grid encapsulation is done last since it's the largest change and all other fixes are independent of it.

**Tech Stack:** Rust, VTE, tokio

---

### Task 1: Fix `expect()` in `SessionManager::get_or_create`

**Files:**
- Modify: `src/session.rs:259-272`

**Step 1: Replace `expect` with `Entry` API**

Replace the `get_or_create` method body with HashMap's entry API to eliminate the `expect`:

```rust
pub fn get_or_create(&mut self, name: &str, cols: u16, rows: u16, history: usize) -> anyhow::Result<(&mut Session, bool)> {
    validate_session_name(name)?;
    use std::collections::hash_map::Entry;
    match self.sessions.entry(name.to_string()) {
        Entry::Occupied(e) => {
            tracing::debug!(session = %name, "reattaching to existing session");
            Ok((e.into_mut(), false))
        }
        Entry::Vacant(e) => {
            let c = if cols > 0 { cols } else { DEFAULT_COLS };
            let r = if rows > 0 { rows } else { DEFAULT_ROWS };
            tracing::debug!(session = %name, cols = c, rows = r, "creating new session");
            let session = Session::new(name.to_string(), c, r, history)?;
            Ok((e.insert(session), true))
        }
    }
}
```

**Step 2: Run tests**

Run: `cargo test`
Expected: All tests pass

**Step 3: Commit**

```
fix: replace expect() with Entry API in get_or_create
```

---

### Task 2: Fix `expect()` in `Grid::remove_visible_row`

**Files:**
- Modify: `src/screen/grid.rs:255-258`

**Step 1: Add debug_assert and safe fallback**

Replace the method:

```rust
/// Remove a visible row by index, returning it.
pub fn remove_visible_row(&mut self, y: usize) -> Row {
    let idx = self.scrollback_len + y;
    debug_assert!(idx < self.cells.len(), "remove_visible_row: index {} out of bounds (cells len {})", idx, self.cells.len());
    self.cells.remove(idx).unwrap_or_else(|| Row::new(self.cols as usize))
}
```

**Step 2: Run tests**

Run: `cargo test`
Expected: All tests pass

**Step 3: Commit**

```
fix: replace expect() with safe fallback in remove_visible_row
```

---

### Task 3: Remove `catch_unwind` in persistent reader loop

**Files:**
- Modify: `src/session.rs:106-139`

**Step 1: Replace catch_unwind with direct lock + process**

Replace the entire `let mut responses = { ... };` block (lines 110-139) with:

```rust
let mut responses = {
    let mut scr = match screen.lock() {
        Ok(s) => s,
        Err(e) => {
            tracing::error!(error = %e, "screen mutex poisoned in reader loop, terminating");
            break;
        }
    };
    scr.process(&buf[..n]);
    let responses = scr.take_responses();
    if !has_client.load(Ordering::Acquire) {
        let _ = scr.take_pending_scrollback();
        let _ = scr.take_passthrough();
    }
    responses
};
```

Remove the `use std::panic;` import if it becomes unused.

**Step 2: Run tests**

Run: `cargo test`
Expected: All tests pass

**Step 3: Commit**

```
fix: remove catch_unwind masking bugs in persistent reader loop

If VTE parser panics, let the mutex poison rather than masking the
bug with AssertUnwindSafe. The reader loop already handles poisoned
mutex gracefully by terminating the loop.
```

---

### Task 4: Document lock ordering

**Files:**
- Modify: `src/server/session_bridge.rs:1-10`

**Step 1: Add lock ordering documentation**

Add after the existing imports (before `const RENDER_THROTTLE`):

```rust
// Lock ordering (to prevent deadlocks):
//
//   manager (tokio::Mutex)
//     → screen (StdMutex)
//       → master (StdMutex)
//     → pty_writer (StdMutex)  [try_lock in reader loop to avoid deadlock]
//     → dims (StdMutex)
//
// The persistent reader loop (session.rs) uses try_lock on pty_writer
// because client_to_pty may hold it during a blocking write while the
// child process waits for a DA response that the reader needs to deliver.
```

**Step 2: Commit**

```
docs: document lock ordering in session_bridge.rs
```

---

### Task 5: Extract `Row::fix_wide_char_orphan_at`

**Files:**
- Modify: `src/screen/cell.rs` (add method after `pop`)
- Modify: `src/screen/grid.rs:364-378` (use new method in `resize`)
- Modify: `src/screen/performer.rs:144-157` (use new method in `exit_alt_screen`)

**Step 1: Write the failing test in cell.rs**

Add at the bottom of the `tests` module in `src/screen/cell.rs`:

```rust
#[test]
fn row_fix_wide_char_orphan_at_width2() {
    let mut row = Row::new(10);
    row[4].width = 2;
    row[5].width = 0; // continuation
    row.fix_wide_char_orphan_at(5);
    assert_eq!(row[4].c, ' ');
    assert_eq!(row[4].width, 1);
    assert_eq!(row[5].c, ' ');
    assert_eq!(row[5].width, 1);
}

#[test]
fn row_fix_wide_char_orphan_at_continuation() {
    let mut row = Row::new(10);
    row[3].width = 2;
    row[4].width = 0; // continuation
    row.fix_wide_char_orphan_at(4);
    assert_eq!(row[3].c, ' ');
    assert_eq!(row[3].width, 1);
    assert_eq!(row[4].c, ' ');
    assert_eq!(row[4].width, 1);
}

#[test]
fn row_fix_wide_char_orphan_at_normal() {
    let mut row = Row::new(10);
    row[4].c = 'A';
    row.fix_wide_char_orphan_at(5);
    // Normal cell — no change
    assert_eq!(row[4].c, 'A');
}
```

**Step 2: Run tests to verify they fail**

Run: `cargo test row_fix_wide_char_orphan`
Expected: FAIL — method doesn't exist

**Step 3: Implement `fix_wide_char_orphan_at` on Row**

Add to `impl Row` in `src/screen/cell.rs`, after the `pop` method:

```rust
/// Clean up orphaned wide-char halves at a column boundary.
/// If `boundary` is the last visible column, blank any wide char
/// that straddles it (first half at boundary or continuation at boundary).
pub fn fix_wide_char_orphan_at(&mut self, boundary: usize) {
    if boundary >= self.cells.len() || boundary == 0 {
        return;
    }
    let last = boundary - 1;
    if self.cells[last].width == 2 {
        // Wide char's first half at boundary-1, continuation would be at boundary (truncated)
        self.cells[last] = Cell::default();
    } else if self.cells[boundary - 1].width == 1 && boundary < self.cells.len() && self.cells[boundary].width == 0 {
        // This doesn't match the orphan pattern — skip
    }
    // Also check: continuation cell at the boundary itself
    if boundary < self.cells.len() && self.cells[boundary].width == 0 && boundary > 0 {
        self.cells[boundary] = Cell::default();
        if self.cells[boundary - 1].width == 2 {
            self.cells[boundary - 1] = Cell::default();
        }
    }
}
```

Wait — let me look at the actual duplicated logic more carefully. The pattern in both grid.rs and performer.rs is:

```rust
if row.len() > cols_usize && cols_usize > 0 {
    let last = cols_usize - 1;
    if row[last].width == 2 {
        row[last] = Cell::default();
    } else if last > 0 && row[last].width == 0 {
        row[last] = Cell::default();
        row[last - 1] = Cell::default();
    }
}
```

This checks the cell at `cols - 1` (the new right edge). The method should be:

```rust
/// Clean up orphaned wide-char halves at the new right edge during resize/restore.
/// `new_cols` is the number of columns the row will be truncated to.
/// Must be called BEFORE `resize()` when the row is longer than `new_cols`.
pub fn fix_wide_char_orphan_at_boundary(&mut self, new_cols: usize) {
    if new_cols == 0 || self.cells.len() <= new_cols {
        return;
    }
    let last = new_cols - 1;
    if self.cells[last].width == 2 {
        self.cells[last] = Cell::default();
    } else if last > 0 && self.cells[last].width == 0 {
        self.cells[last] = Cell::default();
        self.cells[last - 1] = Cell::default();
    }
}
```

**Step 4: Run tests to verify they pass**

Run: `cargo test row_fix_wide_char_orphan`
Expected: PASS

**Step 5: Use the new method in Grid::resize**

In `src/screen/grid.rs`, replace lines 365-377:

```rust
for row in self.cells.iter_mut().skip(self.scrollback_len) {
    // Clean up orphaned wide chars at the new right edge
    if row.len() > cols_usize && cols_usize > 0 {
        let last = cols_usize - 1;
        if row[last].width == 2 {
            row[last] = Cell::default();
        } else if last > 0 && row[last].width == 0 {
            row[last] = Cell::default();
            row[last - 1] = Cell::default();
        }
    }
    row.resize(cols_usize, Cell::default());
}
```

With:

```rust
for row in self.cells.iter_mut().skip(self.scrollback_len) {
    row.fix_wide_char_orphan_at_boundary(cols_usize);
    row.resize(cols_usize, Cell::default());
}
```

**Step 6: Use the new method in performer exit_alt_screen**

In `src/screen/performer.rs`, replace lines 145-157:

```rust
for row in self.grid.visible_rows_mut() {
    // Clean up orphaned wide chars at the new right edge (same as Grid::resize)
    if row.len() > cols_usize && cols_usize > 0 {
        let last = cols_usize - 1;
        if row[last].width == 2 {
            row[last] = Cell::default();
        } else if last > 0 && row[last].width == 0 {
            row[last] = Cell::default();
            row[last - 1] = Cell::default();
        }
    }
    row.resize(cols_usize, Cell::default());
}
```

With:

```rust
for row in self.grid.visible_rows_mut() {
    row.fix_wide_char_orphan_at_boundary(cols_usize);
    row.resize(cols_usize, Cell::default());
}
```

**Step 7: Run all tests**

Run: `cargo test`
Expected: All tests pass

**Step 8: Commit**

```
refactor: extract Row::fix_wide_char_orphan_at_boundary

Eliminates duplicated wide-char orphan cleanup between
Grid::resize() and ScreenPerformer::exit_alt_screen().
```

---

### Task 6: Extract resize/SIGWINCH helper in session_bridge.rs

**Files:**
- Modify: `src/server/session_bridge.rs`

**Step 1: Create the helper function**

Add after `resize_pty` function (around line 62):

```rust
/// Resize the PTY + screen and update stored dimensions, or send SIGWINCH if same size.
/// Runs blocking operations on spawn_blocking.
async fn resize_or_sigwinch(
    master: &crate::pty::SharedMasterPty,
    screen: &crate::session::SharedScreen,
    dims: &Arc<StdMutex<crate::screen::grid::TerminalSize>>,
    cols: u16,
    rows: u16,
    current_dims: crate::screen::grid::TerminalSize,
    session_name: &str,
) -> anyhow::Result<()> {
    let master = master.clone();
    let screen = screen.clone();
    let dims = dims.clone();
    let name = session_name.to_string();

    if current_dims.cols != cols || current_dims.rows != rows {
        debug!(
            session = %session_name,
            old_cols = current_dims.cols, old_rows = current_dims.rows,
            new_cols = cols, new_rows = rows,
            "resizing session"
        );
        tokio::task::spawn_blocking(move || -> anyhow::Result<()> {
            resize_pty(&master, &screen, cols, rows)?;
            match dims.lock() {
                Ok(mut d) => *d = crate::screen::grid::sanitize_dimensions(cols, rows),
                Err(e) => warn!(session = %name, error = %e, "dims mutex poisoned during resize"),
            }
            Ok(())
        }).await??;
    } else {
        debug!(session = %session_name, "sending SIGWINCH (same dimensions)");
        tokio::task::spawn_blocking(move || -> anyhow::Result<()> {
            let sanitized = crate::screen::grid::sanitize_dimensions(cols, rows);
            let m = lock_mutex(&master, "master")?;
            m.resize(portable_pty::PtySize {
                rows: sanitized.rows,
                cols: sanitized.cols,
                pixel_width: 0,
                pixel_height: 0,
            }).map_err(|e| anyhow::anyhow!("{}", e))
        }).await??;
    }
    Ok(())
}
```

**Step 2: Use it in setup_session (reattach path)**

Replace lines 189-238 in `setup_session` (the `if !is_new { ... }` block) with:

```rust
if !is_new {
    let master = session.pty.master_arc();
    let screen = session.screen.clone();
    let dims_ref = session.dims.clone();
    let name_owned = name.to_string();
    if let Err(e) = resize_or_sigwinch(&master, &screen, &dims_ref, cols, rows, cur_dims, &name_owned).await {
        warn!(session = %name, error = %e, "failed to resize/SIGWINCH on reattach");
        has_client.store(false, Ordering::Release);
        anyhow::bail!("failed to resize/SIGWINCH on reattach to '{}'", name);
    }
}
```

**Step 3: Use it in client_to_pty (resize message)**

Replace lines 446-458 in `client_to_pty` (the `ClientMsg::Resize` arm) with:

```rust
ClientMsg::Resize { cols, rows } => {
    let cur = h.dims.lock().map(|d| *d).unwrap_or(
        crate::screen::grid::TerminalSize { cols: DEFAULT_COLS, rows: DEFAULT_ROWS }
    );
    resize_or_sigwinch(&h.master, &h.screen, &h.dims, cols, rows, cur, &h.name).await?;
}
```

**Step 4: Run tests**

Run: `cargo test`
Expected: All tests pass

**Step 5: Commit**

```
refactor: extract resize_or_sigwinch helper in session_bridge

Replaces 3 duplicated resize/SIGWINCH patterns with a single function.
```

---

### Task 7: Use existing constants in server_launcher.rs

**Files:**
- Modify: `src/client/server_launcher.rs:87-88`

**Step 1: Replace hardcoded values**

Replace:
```rust
for _ in 0..50 {
    tokio::time::sleep(std::time::Duration::from_millis(100)).await;
```

With:
```rust
for _ in 0..SERVER_POLL_MAX {
    tokio::time::sleep(SERVER_POLL_INTERVAL).await;
```

**Step 2: Run tests**

Run: `cargo test`
Expected: All tests pass

**Step 3: Commit**

```
fix: use SERVER_POLL_MAX/INTERVAL constants instead of hardcoded values
```

---

### Task 8: Miscellaneous improvements

**Files:**
- Modify: `src/server/session_bridge.rs:17` (add comment to BINCODE_LINE_OVERHEAD)
- Modify: `src/server/session_bridge.rs:305` (add comment to size_limit)
- Modify: `src/screen/performer.rs:529` (move MAX_COMBINING to module level)
- Modify: `src/session.rs:167` (name the 1ms constant)
- Modify: `src/client/server_launcher.rs:73-79` (cap fd loop)
- Modify: `src/session.rs:275` (change `get` to take `&self`)

**Step 1: Add comment to BINCODE_LINE_OVERHEAD**

In `session_bridge.rs:15-17`, change to:

```rust
/// Estimated per-line bincode overhead: 8 bytes for Vec length prefix +
/// ~8 bytes for enum variant tag and alignment padding.
/// Conservative upper bound — actual overhead is typically 12-14 bytes.
const BINCODE_LINE_OVERHEAD: usize = 16;
```

**Step 2: Add comment to size_limit**

Find `let size_limit = protocol::codec::MAX_FRAME_SIZE / 2;` and add a comment:

```rust
// Leave headroom for bincode framing (length prefix, enum tags)
let size_limit = protocol::codec::MAX_FRAME_SIZE / 2;
```

**Step 3: Move MAX_COMBINING to module level in performer.rs**

Add at the top of `src/screen/performer.rs` (after the imports, before `ScreenPerformer`):

```rust
/// Maximum number of combining marks per cell (prevents unbounded memory from malicious input).
const MAX_COMBINING: usize = 16;
```

Remove the `const MAX_COMBINING: usize = 16;` from inside the `print` method (line 529).

**Step 4: Name the 1ms retry constant in session.rs**

Add near the top constants in `src/session.rs`:

```rust
/// Delay before retrying pty_writer lock in the reader loop.
const PTY_WRITER_RETRY_MS: u64 = 1;
```

Replace `std::time::Duration::from_millis(1)` on line 167 with `std::time::Duration::from_millis(PTY_WRITER_RETRY_MS)`.

**Step 5: Cap fd loop in server_launcher.rs**

In `src/client/server_launcher.rs`, replace lines 73-78:

```rust
let max_fd = match nix::libc::sysconf(nix::libc::_SC_OPEN_MAX) {
    n if n > 0 => n as i32,
    _ => 1024,
};
for fd in 3..max_fd {
```

With:

```rust
// Cap at 4096 to avoid iterating millions of fds on systems
// with high RLIMIT_NOFILE. A daemon inherits <20 fds typically.
let max_fd = match nix::libc::sysconf(nix::libc::_SC_OPEN_MAX) {
    n if n > 0 => (n as i32).min(4096),
    _ => 1024,
};
for fd in 3..max_fd {
```

**Step 6: Change `SessionManager::get` to take `&self`**

In `src/session.rs`, change:
```rust
pub fn get(&mut self, name: &str) -> Option<&mut Session> {
    self.sessions.get_mut(name)
}
```

To:
```rust
pub fn get(&self, name: &str) -> Option<&Session> {
    self.sessions.get(name)
}

/// Get a mutable reference to an existing session by name.
pub fn get_mut(&mut self, name: &str) -> Option<&mut Session> {
    self.sessions.get_mut(name)
}
```

Then update callers in `src/server/session_bridge.rs` — both `mgr.get(name)` calls in the `CreateOnly` and `AttachOnly` arms need to become `mgr.get_mut(name)` since they return `&mut Session`. The `CreateOnly` arm also calls `mgr.get(name).is_some()` for existence check — this one stays as `get`.

Check: `mgr.get(name).is_some()` on line 133 — this is a read-only check, keep as `get`.
`mgr.get(name)` on line 143 — this returns into `(s, true)` where s needs to be `&mut Session` — change to `get_mut`.
`mgr.get(name)` on line 153 — same, change to `get_mut`.

**Step 7: Run tests**

Run: `cargo test`
Expected: All tests pass

**Step 8: Commit**

```
refactor: miscellaneous quality improvements

- Add explanatory comments to magic numbers
- Move MAX_COMBINING to module level
- Name PTY_WRITER_RETRY_MS constant
- Cap pre_exec fd loop at 4096
- Split SessionManager::get into get(&self) and get_mut(&mut self)
```

---

### Task 9: Decompose `setup_session`

**Files:**
- Modify: `src/server/session_bridge.rs:107-253`

**Step 1: Extract `acquire_session` helper**

Add before `setup_session`:

```rust
/// Look up or create a session based on connect mode.
/// On error, sends an Error message to the client and returns Err.
async fn acquire_session<'a>(
    stream: &mut tokio::net::UnixStream,
    mgr: &'a mut SessionManager,
    name: &str,
    cols: u16,
    rows: u16,
    history: usize,
    mode: crate::protocol::ConnectMode,
) -> anyhow::Result<(&'a mut Session, bool)> {
    use crate::protocol::ConnectMode;
    match mode {
        ConnectMode::CreateOrAttach => {
            mgr.get_or_create(name, cols, rows, history).map_err(|e| {
                // Error sent below
                e
            })
        }
        ConnectMode::CreateOnly => {
            if mgr.get(name).is_some() {
                let resp = protocol::encode(&ServerMsg::Error(format!("session '{}' already exists", name)))?;
                stream.write_all(&resp).await?;
                anyhow::bail!("session '{}' already exists", name);
            }
            if let Err(e) = mgr.create(name.to_string(), cols, rows, history) {
                let resp = protocol::encode(&ServerMsg::Error(format!("{}", e)))?;
                stream.write_all(&resp).await?;
                return Err(e);
            }
            match mgr.get_mut(name) {
                Some(s) => Ok((s, true)),
                None => {
                    let resp = protocol::encode(&ServerMsg::Error("session disappeared after creation".into()))?;
                    stream.write_all(&resp).await?;
                    anyhow::bail!("session '{}' disappeared after creation", name);
                }
            }
        }
        ConnectMode::AttachOnly => {
            match mgr.get_mut(name) {
                Some(s) => Ok((s, false)),
                None => {
                    let resp = protocol::encode(&ServerMsg::Error(format!("session '{}' not found", name)))?;
                    stream.write_all(&resp).await?;
                    anyhow::bail!("session '{}' not found", name);
                }
            }
        }
    }
}
```

Wait — there's a borrow issue. `acquire_session` takes `&mut SessionManager` but the caller has `manager.lock().await` which is `MutexGuard`. And `stream.write_all` is async but we need to handle errors. Let me reconsider.

Actually, the simpler approach: just extract the match block into `acquire_session` and keep the rest in `setup_session`. But the borrow checker with async + mutable references makes this tricky. The `stream.write_all` calls inside the match arms require `&mut stream` which complicates extraction.

Better approach: move the error-sending to the caller, and have `acquire_session` return a specific error type.

Actually, looking again, the simplest decomposition that works with Rust's borrow checker:

```rust
/// Acquire or create the session, set up eviction, resize, and extract handles.
async fn setup_session(
    stream: &mut tokio::net::UnixStream,
    manager: &Arc<Mutex<SessionManager>>,
    name: &str,
    history: usize,
    cols: u16,
    rows: u16,
    mode: crate::protocol::ConnectMode,
) -> anyhow::Result<SessionSetup> {
    let mut mgr = manager.lock().await;

    let (session, is_new) = acquire_session(&mut mgr, name, cols, rows, history, mode)
        .map_err(|e| {
            // Don't try to send error here — let the caller handle stream cleanup
            e
        })?;
    // ... rest of setup_session
```

This won't work cleanly because the original code sends error messages on the stream inside the match. Let me just keep the match inline but extract the post-acquire logic.

Let me simplify: just refactor the **resize** part (which is the bulkiest duplication) and leave the match as-is. The match itself is reasonably readable.

So for Task 9, we already extracted `resize_or_sigwinch` in Task 6, which significantly shrinks `setup_session`. The remaining function should be ~80 lines instead of 130. That's acceptable.

**Step 1: Verify setup_session is now shorter after Task 6**

After Task 6, the reattach resize block (previously 50 lines) is now ~10 lines. The function should be roughly 80 lines. This is acceptable — skip further decomposition.

**Step 2: Commit (no-op — skip this task)**

Task 9 is satisfied by the work done in Task 6.

---

### Task 10: Extract InputFilter from `run_stdin_to_socket`

**Files:**
- Modify: `src/client/mod.rs`

**Step 1: Write the failing test**

Add at the bottom of `src/client/mod.rs`:

```rust
#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn input_filter_passthrough() {
        let mut filter = InputFilter::new();
        let result = filter.process(b"hello");
        assert_eq!(result, vec![FilterAction::Forward(b"hello".to_vec())]);
    }

    #[test]
    fn input_filter_detach() {
        let mut filter = InputFilter::new();
        let result = filter.process(b"abc\x1cdef");
        assert_eq!(result, vec![
            FilterAction::Forward(b"abc".to_vec()),
            FilterAction::Detach,
        ]);
    }

    #[test]
    fn input_filter_focus_in() {
        let mut filter = InputFilter::new();
        let result = filter.process(b"\x1b[I");
        assert_eq!(result, vec![FilterAction::FocusIn]);
    }

    #[test]
    fn input_filter_focus_out() {
        let mut filter = InputFilter::new();
        let result = filter.process(b"\x1b[O");
        assert_eq!(result, vec![]);
    }

    #[test]
    fn input_filter_carry_esc() {
        let mut filter = InputFilter::new();
        // Lone ESC at end — should carry
        let result = filter.process(b"abc\x1b");
        assert_eq!(result, vec![FilterAction::Forward(b"abc".to_vec())]);
        // Next chunk completes it
        let result = filter.process(b"[Amore");
        assert_eq!(result, vec![FilterAction::Forward(b"\x1b[Amore".to_vec())]);
    }

    #[test]
    fn input_filter_carry_esc_bracket() {
        let mut filter = InputFilter::new();
        let result = filter.process(b"\x1b[");
        assert_eq!(result, vec![]);
        let result = filter.process(b"Irest");
        assert_eq!(result, vec![
            FilterAction::FocusIn,
            FilterAction::Forward(b"rest".to_vec()),
        ]);
    }
}
```

**Step 2: Run tests to verify they fail**

Run: `cargo test --bin retach input_filter`
Expected: FAIL — types don't exist

**Step 3: Implement InputFilter**

Add before `run_stdin_to_socket` in `src/client/mod.rs`:

```rust
/// Action produced by the input filter.
#[derive(Debug, PartialEq)]
enum FilterAction {
    /// Forward filtered bytes to the server.
    Forward(Vec<u8>),
    /// Client requested detach (Ctrl+\).
    Detach,
    /// Terminal reported focus gained — trigger screen refresh.
    FocusIn,
}

/// Filters stdin input: handles detach key, focus events, and split escape sequences.
struct InputFilter {
    carry: Vec<u8>,
}

impl InputFilter {
    fn new() -> Self {
        Self { carry: Vec::with_capacity(2) }
    }

    /// Process raw input bytes, returning a list of actions.
    fn process(&mut self, input: &[u8]) -> Vec<FilterAction> {
        let raw: Vec<u8> = if self.carry.is_empty() {
            input.to_vec()
        } else {
            let mut combined = std::mem::take(&mut self.carry);
            combined.extend_from_slice(input);
            combined
        };

        // Check for detach key first
        if let Some(pos) = raw.iter().position(|&b| b == DETACH_KEY) {
            let mut actions = Vec::new();
            if pos > 0 {
                actions.push(FilterAction::Forward(raw[..pos].to_vec()));
            }
            actions.push(FilterAction::Detach);
            return actions;
        }

        let mut actions = Vec::new();
        let mut filtered = Vec::with_capacity(raw.len());
        let mut i = 0;
        while i < raw.len() {
            if raw[i] == 0x1b {
                if i + 1 < raw.len() {
                    if raw[i + 1] == b'[' {
                        if i + 2 < raw.len() {
                            if raw[i + 2] == FOCUS_IN {
                                if !filtered.is_empty() {
                                    actions.push(FilterAction::Forward(std::mem::take(&mut filtered)));
                                }
                                actions.push(FilterAction::FocusIn);
                                i += 3;
                                continue;
                            } else if raw[i + 2] == FOCUS_OUT {
                                i += 3;
                                continue;
                            }
                            // ESC [ <other> — pass through
                        } else {
                            // ESC [ at end — carry
                            self.carry.extend_from_slice(&raw[i..]);
                            break;
                        }
                    } else {
                        // ESC <non-[> — pass both through
                        filtered.push(raw[i]);
                        i += 1;
                        continue;
                    }
                } else {
                    // Lone ESC at end — carry
                    self.carry.push(0x1b);
                    break;
                }
            }
            filtered.push(raw[i]);
            i += 1;
        }
        if !filtered.is_empty() {
            actions.push(FilterAction::Forward(filtered));
        }
        actions
    }

    /// Flush any remaining carry bytes as a Forward action.
    fn flush(&mut self) -> Option<FilterAction> {
        if self.carry.is_empty() {
            None
        } else {
            Some(FilterAction::Forward(std::mem::take(&mut self.carry)))
        }
    }
}
```

**Step 4: Run tests**

Run: `cargo test --bin retach input_filter`
Expected: All PASS

**Step 5: Rewrite `run_stdin_to_socket` to use InputFilter**

Replace the function body of `run_stdin_to_socket` (keep the signature):

```rust
async fn run_stdin_to_socket(sw: SocketWriter) -> anyhow::Result<()> {
    let mut filter = InputFilter::new();

    loop {
        let result = tokio::task::spawn_blocking(|| {
            let mut buf = [0u8; 1024];
            let n = io::stdin().read(&mut buf)?;
            Ok::<_, io::Error>((buf, n))
        })
        .await;

        match result {
            Ok(Ok((_buf, 0))) => {
                if let Some(FilterAction::Forward(data)) = filter.flush() {
                    let msg = protocol::encode(&ClientMsg::Input(data))?;
                    let mut w = sw.lock().await;
                    w.write_all(&msg).await?;
                }
                break;
            }
            Ok(Ok((buf, n))) => {
                for action in filter.process(&buf[..n]) {
                    match action {
                        FilterAction::Forward(data) => {
                            let msg = protocol::encode(&ClientMsg::Input(data))?;
                            let mut w = sw.lock().await;
                            w.write_all(&msg).await?;
                        }
                        FilterAction::Detach => {
                            let mut w = sw.lock().await;
                            if let Ok(msg) = protocol::encode(&ClientMsg::Detach) {
                                w.write_all(&msg).await?;
                            }
                            drop(w);
                            return Ok(());
                        }
                        FilterAction::FocusIn => {
                            if let Ok(msg) = protocol::encode(&ClientMsg::RefreshScreen) {
                                let mut w = sw.lock().await;
                                let _ = w.write_all(&msg).await;
                            }
                        }
                    }
                }
            }
            Ok(Err(e)) => return Err(anyhow::Error::from(e)),
            Err(e) => return Err(anyhow::Error::from(e)),
        }
    }
    Ok(())
}
```

**Step 6: Run all tests**

Run: `cargo test`
Expected: All tests pass

**Step 7: Commit**

```
refactor: extract InputFilter from run_stdin_to_socket

Separates escape sequence parsing (detach, focus events, carry buffer)
from async I/O, making both independently testable.
```

---

### Task 11: Grid encapsulation — add accessor methods

**Files:**
- Modify: `src/screen/grid.rs`

This is the foundation task. Add all accessor methods to Grid before making fields private.

**Step 1: Add public read-only accessors**

Add to `impl Grid` in `src/screen/grid.rs`, after the `new` method:

```rust
/// Number of columns.
#[inline]
pub fn cols(&self) -> u16 { self.cols }

/// Number of visible rows.
#[inline]
pub fn rows(&self) -> u16 { self.rows }

/// Current cursor position (x, y), both 0-based.
#[inline]
pub fn cursor_pos(&self) -> (u16, u16) { (self.cursor_x, self.cursor_y) }

/// Whether the cursor is currently visible (DECTCEM).
#[inline]
pub fn cursor_visible(&self) -> bool { self.cursor_visible }

/// Current scroll region (top, bottom), both 0-based inclusive.
#[inline]
pub fn scroll_region(&self) -> (u16, u16) { (self.scroll_top, self.scroll_bottom) }

/// Terminal modes (read-only).
#[inline]
pub fn modes(&self) -> &TerminalModes { &self.modes }

/// Number of scrollback rows.
#[inline]
pub fn scrollback_len(&self) -> usize { self.scrollback_len }

/// Maximum scrollback lines to retain.
#[inline]
pub fn scrollback_limit(&self) -> usize { self.scrollback_limit }

/// Index where unsent scrollback begins.
#[inline]
pub fn pending_start(&self) -> usize { self.pending_start }

/// Style table (read-only).
#[inline]
pub fn style_table(&self) -> &StyleTable { &self.style_table }

/// Whether a deferred wrap is pending.
#[inline]
pub fn wrap_pending(&self) -> bool { self.wrap_pending }
```

**Step 2: Add pub(super) mutation accessors for performer**

```rust
// --- Mutation accessors (for performer) ---

/// Current cursor X position.
#[inline]
pub(super) fn cursor_x(&self) -> u16 { self.cursor_x }

/// Current cursor Y position.
#[inline]
pub(super) fn cursor_y(&self) -> u16 { self.cursor_y }

/// Set cursor X, clamped to `cols - 1`.
#[inline]
pub(super) fn set_cursor_x(&mut self, x: u16) {
    self.cursor_x = x.min(self.cols.saturating_sub(1));
}

/// Set cursor Y, clamped to `rows - 1`.
#[inline]
pub(super) fn set_cursor_y(&mut self, y: u16) {
    self.cursor_y = y.min(self.rows.saturating_sub(1));
}

/// Set cursor position, clamped to grid bounds, and clear wrap_pending.
#[inline]
pub(super) fn set_cursor(&mut self, x: u16, y: u16) {
    self.cursor_x = x.min(self.cols.saturating_sub(1));
    self.cursor_y = y.min(self.rows.saturating_sub(1));
    self.wrap_pending = false;
}

/// Set cursor X without clamping (caller must ensure valid).
#[inline]
pub(super) fn set_cursor_x_unclamped(&mut self, x: u16) {
    self.cursor_x = x;
}

/// Set cursor Y without clamping (caller must ensure valid).
#[inline]
pub(super) fn set_cursor_y_unclamped(&mut self, y: u16) {
    self.cursor_y = y;
}

/// Set wrap_pending flag.
#[inline]
pub(super) fn set_wrap_pending(&mut self, val: bool) {
    self.wrap_pending = val;
}

/// Set cursor visibility.
#[inline]
pub(super) fn set_cursor_visible(&mut self, visible: bool) {
    self.cursor_visible = visible;
}

/// Set scroll region. Caller must ensure top <= bottom < rows.
#[inline]
pub(super) fn set_scroll_region(&mut self, top: u16, bottom: u16) {
    self.scroll_top = top;
    self.scroll_bottom = bottom;
}

/// Scroll region top (0-based inclusive).
#[inline]
pub(super) fn scroll_top(&self) -> u16 { self.scroll_top }

/// Scroll region bottom (0-based inclusive).
#[inline]
pub(super) fn scroll_bottom(&self) -> u16 { self.scroll_bottom }

/// Terminal modes (mutable).
#[inline]
pub(super) fn modes_mut(&mut self) -> &mut TerminalModes { &mut self.modes }

/// Set terminal modes directly (for restore).
#[inline]
pub(super) fn set_modes(&mut self, modes: TerminalModes) {
    self.modes = modes;
}

/// Style table (mutable).
#[inline]
pub(super) fn style_table_mut(&mut self) -> &mut StyleTable { &mut self.style_table }

/// Set a tab stop at the given column.
pub(super) fn set_tab_stop(&mut self, col: u16) {
    let c = col as usize;
    if c < self.tab_stops.len() { self.tab_stops[c] = true; }
}

/// Clear a tab stop at the given column.
pub(super) fn clear_tab_stop(&mut self, col: u16) {
    let c = col as usize;
    if c < self.tab_stops.len() { self.tab_stops[c] = false; }
}

/// Clear all tab stops.
pub(super) fn clear_all_tab_stops(&mut self) {
    for stop in self.tab_stops.iter_mut() { *stop = false; }
}

/// Reset tab stops to default (every 8 columns).
pub(super) fn reset_tab_stops(&mut self) {
    self.tab_stops = default_tab_stops(self.cols);
}

/// Set maximum scrollback limit.
#[inline]
pub(super) fn set_scrollback_limit(&mut self, limit: usize) {
    self.scrollback_limit = limit;
}

/// Set pending_start index.
#[inline]
pub(super) fn set_pending_start(&mut self, val: usize) {
    self.pending_start = val;
}

/// Iterate over scrollback rows.
pub fn scrollback_rows(&self) -> impl Iterator<Item = &Row> {
    self.cells.iter().take(self.scrollback_len)
}

/// Iterate over pending (unsent) scrollback rows.
pub fn pending_scrollback_iter(&self) -> impl Iterator<Item = &Row> {
    self.cells.iter().skip(self.pending_start).take(self.scrollback_len - self.pending_start)
}

/// Drain visible rows for alt screen save. Returns saved rows.
pub(super) fn drain_visible(&mut self) -> VecDeque<Row> {
    self.cells.drain(self.scrollback_len..).collect()
}

/// Replace visible rows (for alt screen restore).
pub(super) fn replace_visible(&mut self, rows: VecDeque<Row>) {
    self.cells.truncate(self.scrollback_len);
    for row in rows {
        self.cells.push_back(row);
    }
}

/// Add blank visible rows for alt screen.
pub(super) fn fill_visible_blank(&mut self) {
    for _ in 0..self.rows as usize {
        self.cells.push_back(Row::new(self.cols as usize));
    }
}

/// Adjust visible row count to match `self.rows` (trim or pad).
pub(super) fn adjust_visible_to_fit(&mut self) {
    let rows_usize = self.rows as usize;
    while self.visible_row_count() > rows_usize {
        self.cells.pop_back();
    }
    while self.visible_row_count() < rows_usize {
        self.cells.push_back(Row::new(self.cols as usize));
    }
}

/// Decrement scrollback_len by `count` (for restoring scrollback on vertical grow).
pub(super) fn restore_scrollback(&mut self, count: usize) {
    self.scrollback_len -= count;
    self.pending_start = self.pending_start.min(self.scrollback_len);
}
```

**Step 2: Run tests**

Run: `cargo test`
Expected: All tests pass (new methods are additive, nothing changed yet)

**Step 3: Commit**

```
feat: add accessor methods to Grid for encapsulation
```

---

### Task 12: Grid encapsulation — migrate performer.rs

**Files:**
- Modify: `src/screen/performer.rs` (all ~200 direct field accesses)

This is the largest single task. Replace all direct `self.grid.field` accesses with method calls.

**Step 1: Migrate performer.rs**

The key replacements (apply all at once):

**Reads:**
- `self.grid.cursor_x` → `self.grid.cursor_x()`
- `self.grid.cursor_y` → `self.grid.cursor_y()`
- `self.grid.cols` → `self.grid.cols()`
- `self.grid.rows` → `self.grid.rows()`
- `self.grid.scroll_top` → `self.grid.scroll_top()`
- `self.grid.scroll_bottom` → `self.grid.scroll_bottom()`
- `self.grid.cursor_visible` → `self.grid.cursor_visible()`
- `self.grid.wrap_pending` (reads) → `self.grid.wrap_pending()`
- `self.grid.scrollback_limit` (reads) → `self.grid.scrollback_limit()`
- `self.grid.modes.field` (reads) → `self.grid.modes().field`

**Writes:**
- `self.grid.cursor_x = expr` → `self.grid.set_cursor_x(expr)` or `self.grid.set_cursor_x_unclamped(expr)` when the value is already clamped
- `self.grid.cursor_y = expr` → `self.grid.set_cursor_y(expr)` or `self.grid.set_cursor_y_unclamped(expr)`
- `self.grid.wrap_pending = val` → `self.grid.set_wrap_pending(val)`
- `self.grid.cursor_visible = val` → `self.grid.set_cursor_visible(val)`
- `self.grid.scroll_top = val` → use `self.grid.set_scroll_region(top, bottom)`
- `self.grid.scroll_bottom = val` → use `self.grid.set_scroll_region(top, bottom)`
- `self.grid.modes.field = val` → `self.grid.modes_mut().field = val`
- `self.grid.modes = val` → `self.grid.set_modes(val)`
- `self.grid.tab_stops[x] = true` → `self.grid.set_tab_stop(x as u16)`
- `self.grid.tab_stops[x] = false` → `self.grid.clear_tab_stop(x as u16)`
- `for stop in self.grid.tab_stops.iter_mut() { *stop = false; }` → `self.grid.clear_all_tab_stops()`
- `self.grid.tab_stops = default_tab_stops(...)` → `self.grid.reset_tab_stops()`
- `self.grid.scrollback_limit = val` → `self.grid.set_scrollback_limit(val)`
- `self.grid.style_table.method()` → `self.grid.style_table_mut().method()` for mut, `self.grid.style_table().method()` for read

**Alt screen:**
- `self.grid.cells.drain(self.grid.scrollback_len..)` → `self.grid.drain_visible()`
- `self.grid.cells.push_back(...)` for alt screen rows → `self.grid.fill_visible_blank()`
- `self.grid.cells.truncate(self.grid.scrollback_len)` → part of `replace_visible`
- `self.grid.visible_row_count()` comparison loops → `self.grid.adjust_visible_to_fit()`

**Special cases in `print()` that do `self.grid.cursor_x += char_width` and then check bounds:**
This pattern can't use `set_cursor_x` (which clamps) because the code intentionally lets cursor go to `cols` then handles wrap. Use `set_cursor_x_unclamped` and then the subsequent clamping logic:

```rust
// Before:
self.grid.cursor_x += char_width;
if self.grid.cursor_x >= self.grid.cols {
    self.grid.cursor_x = self.grid.cols - 1;
    if self.grid.modes.autowrap_mode { self.grid.wrap_pending = true; }
}

// After:
let new_x = self.grid.cursor_x() + char_width;
if new_x >= self.grid.cols() {
    self.grid.set_cursor_x_unclamped(self.grid.cols() - 1);
    if self.grid.modes().autowrap_mode { self.grid.set_wrap_pending(true); }
} else {
    self.grid.set_cursor_x_unclamped(new_x);
}
```

**Cursor movement methods** like `csi_cursor_up` that do `self.grid.cursor_y = self.grid.cursor_y.saturating_sub(n).max(top)`:
```rust
// Before:
self.grid.cursor_y = self.grid.cursor_y.saturating_sub(n).max(top);
// After:
self.grid.set_cursor_y_unclamped(self.grid.cursor_y().saturating_sub(n).max(top));
```

These use `_unclamped` because the value is already computed within valid bounds.

**Step 2: Run tests**

Run: `cargo test`
Expected: All tests pass

**Step 3: Commit**

```
refactor: migrate performer.rs to Grid accessor methods
```

---

### Task 13: Grid encapsulation — migrate render.rs

**Files:**
- Modify: `src/screen/render.rs`

**Step 1: Replace all direct Grid field accesses**

In `render_screen_impl` and `render_screen_with_scrollback`:

- `grid.rows` → `grid.rows()`
- `grid.scroll_top` → `grid.scroll_region().0`
- `grid.scroll_bottom` → `grid.scroll_region().1`
- `grid.cursor_y` → `grid.cursor_pos().1`
- `grid.cursor_x` → `grid.cursor_pos().0`
- `grid.cursor_visible` → `grid.cursor_visible()`
- `grid.modes` / `&grid.modes` → `grid.modes()`
- `grid.style_table.get(...)` → `grid.style_table().get(...)`

Note: Several render.rs tests directly set Grid fields for test setup (e.g., `grid.modes.cursor_key_mode = true`). These tests use `Grid::new` directly and set fields for testing — they need to use the `pub(super)` setter methods since tests are within the `screen` module.

**Step 2: Run tests**

Run: `cargo test`
Expected: All tests pass

**Step 3: Commit**

```
refactor: migrate render.rs to Grid accessor methods
```

---

### Task 14: Grid encapsulation — migrate mod.rs and tests

**Files:**
- Modify: `src/screen/mod.rs`
- Modify: `src/screen/tests_screen.rs`
- Modify: `src/screen/tests_reattach.rs`
- Modify: `src/screen/tests_resize.rs`
- Modify: all other test files under `src/screen/`

**Step 1: Migrate mod.rs**

In `Screen::rows()`:
```rust
pub fn rows(&self) -> u16 { self.grid.rows() }
```

In `Screen::resize()`:
```rust
let old_rows = self.grid.rows();
// ...
let grow = (rows - old_rows) as usize;
let restore_count = grow.min(self.grid.scrollback_len());
self.grid.restore_scrollback(restore_count);
self.grid.set_cursor_y(self.grid.cursor_y().saturating_add(
    u16::try_from(restore_count).unwrap_or(u16::MAX)
));
```

In `Screen::take_pending_scrollback()`:
```rust
pub fn take_pending_scrollback(&mut self) -> Vec<Vec<u8>> {
    let start = self.grid.pending_start();
    let count = self.grid.pending_scrollback_count();
    self.grid.set_pending_start(self.grid.scrollback_len());
    let st = self.grid.style_table();
    self.grid.pending_scrollback_iter()... // wait, pending_start was just updated
```

Hmm — `pending_scrollback_iter` reads `self.pending_start` which we just set. Need to capture start/count first. Use the existing `cells.iter().skip(start).take(count)` pattern but through the accessor:

Actually, the cleanest way: add a `take_pending_scrollback_rows` method on Grid that returns an iterator and updates pending_start atomically:

Add to Grid:
```rust
/// Take pending scrollback rows: returns iterator over unsent rows and advances pending_start.
pub fn take_pending_scrollback_rows(&mut self) -> impl Iterator<Item = &Row> {
    let start = self.pending_start;
    let count = self.scrollback_len - self.pending_start;
    self.pending_start = self.scrollback_len;
    self.cells.iter().skip(start).take(count)
}
```

Then `Screen::take_pending_scrollback`:
```rust
pub fn take_pending_scrollback(&mut self) -> Vec<Vec<u8>> {
    let st = self.grid.style_table();
    // Can't call both methods on grid due to borrow — inline:
    ...
```

Wait, there's a borrow issue: `take_pending_scrollback_rows` borrows `&mut self` while we need `style_table()` which borrows `&self`. This is a classic split borrow problem.

Best approach: have `Grid` expose what we need separately. Keep the current approach in mod.rs but use accessors:

```rust
pub fn take_pending_scrollback(&mut self) -> Vec<Vec<u8>> {
    let start = self.grid.pending_start();
    let count = self.grid.pending_scrollback_count();
    self.grid.set_pending_start(self.grid.scrollback_len());
    // Access cells and style_table through the grid's public accessors
    self.grid.pending_scrollback_iter_from(start, count)
        .map(|row| render::render_line(row, self.grid.style_table()))
        .collect()
}
```

Hmm, but `pending_scrollback_iter_from` would need a separate method. Actually the simplest approach: since `pending_start` and `scrollback_len` are read-only accessors, and `set_pending_start` is a write, we can read first, then mutate, then read again:

```rust
pub fn take_pending_scrollback(&mut self) -> Vec<Vec<u8>> {
    let start = self.grid.pending_start();
    let count = self.grid.pending_scrollback_count();
    self.grid.set_pending_start(self.grid.scrollback_len());
    self.grid.scrollback_rows().skip(start).take(count)
        .map(|row| render::render_line(row, self.grid.style_table()))
        .collect()
}
```

Wait, `scrollback_rows()` returns `impl Iterator<Item = &Row>` borrowing `&self.grid`, and `style_table()` also borrows `&self.grid`. Both are shared borrows, so this should work.

And `set_pending_start` on the line before is `&mut self.grid`, which ends before the shared borrows begin. This compiles.

In `Screen::get_history`:
```rust
pub fn get_history(&self) -> Vec<Vec<u8>> {
    self.grid.scrollback_rows()
        .map(|row| render::render_line(row, self.grid.style_table()))
        .collect()
}
```

In `compact_styles`:
```rust
pub(crate) fn compact_styles(grid: &mut Grid, saved_grid: Option<&grid::SavedGrid>) {
    let cap = grid.style_table().capacity();
    if cap <= 1 { return; }
    let mut live = vec![false; cap];
    live[0] = true;
    // Need to iterate all cells — use scrollback_rows() + visible_rows()
    for row in grid.scrollback_rows().chain(grid.visible_rows()) {
        for cell in row.iter() {
            let id = cell.style_id.index();
            if id < cap { live[id] = true; }
        }
    }
    // ... saved_grid iteration stays the same (SavedGrid fields are pub(super))
    grid.style_table_mut().reclaim(&live);
}
```

Wait — `scrollback_rows()` iterates `cells.iter().take(scrollback_len)` and `visible_rows()` iterates `cells.iter().skip(scrollback_len).take(rows)`. Chaining them gives us all cells. But the original code was `grid.cells.iter()` which iterates everything. Are `scrollback_rows().chain(visible_rows())` equivalent to `cells.iter()`? Yes, if `cells.len() == scrollback_len + rows`. Which should always be true.

**Step 2: Migrate test files**

Tests within `src/screen/` are `pub(super)` so they can use the `pub(super)` accessor methods.

Common patterns in tests:
- `screen.grid.cursor_x` → `screen.grid.cursor_x()` (read) or `screen.grid.cursor_pos().0`
- `screen.grid.cursor_y` → `screen.grid.cursor_y()` (read)
- `screen.grid.cols` → `screen.grid.cols()`
- `screen.grid.rows` → `screen.grid.rows()`
- `screen.grid.scroll_top` → `screen.grid.scroll_top()`
- `screen.grid.scroll_bottom` → `screen.grid.scroll_bottom()`
- `screen.grid.wrap_pending` → `screen.grid.wrap_pending()`
- `screen.grid.modes.field` → `screen.grid.modes().field`
- `screen.grid.scrollback_len` → `screen.grid.scrollback_len()`
- `screen.grid.pending_start` → `screen.grid.pending_start()`
- `screen.grid.scrollback_limit` → `screen.grid.scrollback_limit()`
- `screen.grid.cursor_visible` → `screen.grid.cursor_visible()`
- `screen.grid.cells[0]` → `screen.grid.scrollback_rows().next().unwrap()` or keep direct access through a test helper

For tests that directly set Grid fields (e.g., in grid.rs internal tests):
- `grid.cursor_x = 79` → `grid.set_cursor_x_unclamped(79)` (tests within screen module have pub(super) access)
- `grid.scroll_top = 5` → `grid.set_scroll_region(5, grid.scroll_bottom())`
- `grid.tab_stops[3] = true` → `grid.set_tab_stop(3)`
- `grid.modes.field = val` → `grid.modes_mut().field = val`

For grid.rs internal tests that read `grid.cells[0]` to check scrollback content: add a `pub(super) fn scrollback_row(&self, idx: usize) -> &Row` accessor.

**Step 3: Run tests**

Run: `cargo test`
Expected: All tests pass

**Step 4: Commit**

```
refactor: migrate mod.rs and tests to Grid accessor methods
```

---

### Task 15: Grid encapsulation — make fields private

**Files:**
- Modify: `src/screen/grid.rs:162-189`

**Step 1: Remove `pub(super)` from all Grid fields**

Change all field declarations from `pub(super)` to just private (no visibility modifier):

```rust
pub struct Grid {
    cols: u16,
    rows: u16,
    cells: VecDeque<Row>,
    cursor_x: u16,
    cursor_y: u16,
    wrap_pending: bool,
    scroll_top: u16,
    scroll_bottom: u16,
    cursor_visible: bool,
    modes: TerminalModes,
    tab_stops: Vec<bool>,
    scrollback_len: usize,
    scrollback_limit: usize,
    pending_start: usize,
    style_table: StyleTable,
}
```

Also make `SavedGrid` fields private and add accessors if needed (check if anything outside grid.rs reads them — `compact_styles` in mod.rs reads `saved.visible_cells`). Add:

```rust
impl SavedGrid {
    /// Iterate over saved visible rows (for style compaction).
    pub(super) fn visible_rows(&self) -> impl Iterator<Item = &Row> {
        self.visible_cells.iter()
    }

    /// Scrollback limit that was saved.
    pub(super) fn scrollback_limit(&self) -> usize {
        self.scrollback_limit
    }
}
```

Make SavedGrid fields private too:
```rust
pub struct SavedGrid {
    visible_cells: VecDeque<Row>,
    scrollback_limit: usize,
}
```

Update `compact_styles` to use `saved.visible_rows()` instead of `saved.visible_cells.iter()`.

Update performer's `enter_alt_screen` to construct SavedGrid through a constructor:
```rust
impl SavedGrid {
    pub(super) fn new(visible_cells: VecDeque<Row>, scrollback_limit: usize) -> Self {
        Self { visible_cells, scrollback_limit }
    }
}
```

**Step 2: Run tests**

Run: `cargo test`
Expected: All tests pass. If any test still accesses fields directly, fix them.

**Step 3: Commit**

```
refactor: make Grid and SavedGrid fields private

All access now goes through accessor methods with bounds checking.
Cursor position is validated on set, preventing out-of-bounds bugs.
```

---

### Task 16: Final verification

**Step 1: Run full test suite**

Run: `cargo test`
Expected: All 531+ tests pass

**Step 2: Run clippy**

Run: `cargo clippy -- -D warnings`
Expected: No warnings

**Step 3: Test manually**

Run: `cargo build && ./target/debug/retach new test-session`
Verify: session creates, terminal works, detach with Ctrl+\, reattach works.

**Step 4: Commit any final fixes if needed**

---

## Implementation Order Summary

| Task | Description | Risk | Est. Lines Changed |
|------|-------------|------|-------------------|
| 1 | Fix expect() in get_or_create | Low | ~15 |
| 2 | Fix expect() in remove_visible_row | Low | ~5 |
| 3 | Remove catch_unwind | Low | ~20 |
| 4 | Document lock ordering | None | ~10 |
| 5 | Extract Row::fix_wide_char_orphan_at_boundary | Low | ~30 |
| 6 | Extract resize_or_sigwinch helper | Medium | ~60 |
| 7 | Use constants in server_launcher | Low | ~2 |
| 8 | Miscellaneous improvements | Low | ~30 |
| 9 | (Satisfied by Task 6) | — | — |
| 10 | Extract InputFilter | Medium | ~150 |
| 11 | Grid accessors (additive) | Low | ~150 |
| 12 | Migrate performer.rs | High | ~200 |
| 13 | Migrate render.rs | Medium | ~30 |
| 14 | Migrate mod.rs + tests | High | ~200 |
| 15 | Make Grid fields private | Medium | ~20 |
| 16 | Final verification | None | ~0 |
