# Quality Fixes Implementation Plan

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

**Goal:** Fix all code quality issues found in code review: error handling, function complexity, code duplication, magic numbers, Drop safety, InputFilter, TOCTOU documentation, dead code removal.

**Architecture:** Incremental fixes organized by risk level. Phase 1 (logging, docs, dead code) is safe. Phase 2 (helpers) is low risk. Phase 3 (function splits) is medium risk. Phase 4 (structural fixes) is medium-high risk. Each phase gets its own commit.

**Tech Stack:** Rust, tokio, tracing, anyhow

---

### Task 1: Phase 1a — Add logging to suppressed errors in session.rs

**Files:**
- Modify: `src/session.rs:134,238,243-244,269-270,274,278`

**Step 1: Add warn/debug logging to all `let _ = ...` sites**

In `session.rs:134` (evict send), change:
```rust
let _ = old_tx.send(false);
```
to:
```rust
if old_tx.send(false).is_err() {
    tracing::debug!(session = %self.name, "evict channel: previous client already disconnected");
}
```

In `session.rs:238` (flush in reader loop), change:
```rust
let _ = w.flush();
```
to:
```rust
if let Err(e) = w.flush() {
    tracing::warn!(error = %e, "failed to flush PTY writer in reader loop");
}
```

In `session.rs:243-244` (deferred queue overflow), change:
```rust
if deferred_responses.len() >= MAX_DEFERRED {
    deferred_responses.pop_front(); // drop oldest
}
```
to:
```rust
if deferred_responses.len() >= MAX_DEFERRED {
    tracing::warn!(
        queue_len = MAX_DEFERRED,
        "deferred DA/DSR response queue full, dropping oldest response"
    );
    deferred_responses.pop_front();
}
```

In `session.rs:269-270` (Drop — child kill/wait), change:
```rust
let _ = child.kill();
let _ = child.wait();
```
to:
```rust
if let Err(e) = child.kill() {
    tracing::debug!(session = %self.name, error = %e, "failed to kill child process (may have already exited)");
}
if let Err(e) = child.wait() {
    tracing::debug!(session = %self.name, error = %e, "failed to wait on child process");
}
```

In `session.rs:274` (Drop — evict send), change:
```rust
let _ = tx.send(false);
```
to:
```rust
if tx.send(false).is_err() {
    tracing::debug!(session = %self.name, "evict channel closed during session drop");
}
```

In `session.rs:278` (Drop — reader join), change:
```rust
let _ = handle.join();
```
to:
```rust
if let Err(_) = handle.join() {
    tracing::warn!(session = %self.name, "reader thread panicked during session drop");
}
```

**Step 2: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 3: Commit**

```bash
git add src/session.rs
git commit -m "fix: add logging to suppressed errors in session.rs

Replace silent `let _ = ...` patterns with tracing::warn/debug.
Log deferred response queue overflow that could lose DA/DSR replies."
```

---

### Task 2: Phase 1b — Add logging to suppressed errors in server/ and client/

**Files:**
- Modify: `src/server/session_relay.rs:77,83`
- Modify: `src/server/session_bridge.rs:140-141`
- Modify: `src/client/mod.rs:227,329-330,384,391,413,429`

**Step 1: Fix session_relay.rs**

In `session_relay.rs:77`, change:
```rust
let _ = writer.write_all(&msg).await;
```
to:
```rust
if let Err(e) = writer.write_all(&msg).await {
    debug!(session = %h.name, error = %e, "failed to send eviction notice to client");
}
```

In `session_relay.rs:83`, change:
```rust
let _ = writer.write_all(&msg).await;
```
to:
```rust
if let Err(e) = writer.write_all(&msg).await {
    debug!(session = %h.name, error = %e, "failed to send session-ended to killed client");
}
```

**Step 2: Fix session_bridge.rs**

In `session_bridge.rs:140-141`, the `take_pending_scrollback()` and `take_passthrough()` return values that are intentionally discarded (drain operation). Add a comment instead of logging — these are NOT errors:
```rust
// Intentionally discarded: draining stale data so relay starts clean.
screen.take_pending_scrollback();
screen.take_passthrough();
```

**Step 3: Fix client/mod.rs**

In `client/mod.rs:227`, change:
```rust
let _ = w.write_all(&msg).await;
```
to:
```rust
if let Err(e) = w.write_all(&msg).await {
    tracing::debug!(error = %e, "failed to send focus-in refresh");
    break;
}
```

In `client/mod.rs:329-330`, change:
```rust
let _ = io::stdout().write_all(b"\x1b[?1004h");
let _ = io::stdout().flush();
```
to:
```rust
if let Err(e) = io::stdout().write_all(b"\x1b[?1004h") {
    tracing::debug!(error = %e, "failed to enable focus reporting");
}
if let Err(e) = io::stdout().flush() {
    tracing::debug!(error = %e, "failed to flush stdout after focus reporting setup");
}
```

In `client/mod.rs:384`, change:
```rust
let _ = w.write_all(&msg).await;
```
to:
```rust
if let Err(e) = w.write_all(&msg).await {
    tracing::debug!(error = %e, "failed to send SIGINT detach");
}
```

In `client/mod.rs:391`, change:
```rust
let _ = w.write_all(&msg).await;
```
to:
```rust
if let Err(e) = w.write_all(&msg).await {
    tracing::debug!(error = %e, "failed to send SIGTERM detach");
}
```

In `client/mod.rs:413,429` (cleanup_terminal), change:
```rust
let _ = stdout.write_all(concat!(...).as_bytes());
let _ = stdout.flush();
```
to:
```rust
// Best-effort terminal cleanup — errors are expected if stdout is broken
let _ = stdout.write_all(concat!(...).as_bytes());
let _ = stdout.flush();
```

Keep `let _ =` here — cleanup_terminal runs on panic and signal paths where logging isn't safe.

**Step 4: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 5: Commit**

```bash
git add src/server/session_relay.rs src/server/session_bridge.rs src/client/mod.rs
git commit -m "fix: add logging to suppressed errors in server/ and client/"
```

---

### Task 3: Phase 1c — Document magic numbers with rationale

**Files:**
- Modify: `src/session.rs:33-35`
- Modify: `src/screen/mod.rs:36-41`
- Modify: `src/screen/cell.rs:11` (MAX_COMBINING, find exact location)
- Modify: `src/server/session_bridge.rs:24-29`
- Modify: `src/server/mod.rs:17-18`
- Modify: `src/server/client_handler.rs:11-12`
- Modify: `src/client/mod.rs:193`
- Modify: `src/screen/grid.rs` (tab stop 8)

**Step 1: Add rationale comments to all constants**

In `session.rs:33-35`, change:
```rust
const MAX_SESSION_NAME_LEN: usize = 128;
const PTY_READ_BUF_SIZE: usize = 4096;
const MAX_DEFERRED: usize = 64;
```
to:
```rust
/// Session names are displayed in CLI output and stored as filesystem-friendly identifiers.
/// 128 bytes is generous for human-readable names while preventing abuse.
const MAX_SESSION_NAME_LEN: usize = 128;

/// PTY read buffer size. 4 KiB matches the typical pipe buffer granularity on
/// Linux/macOS and balances syscall overhead against latency.
const PTY_READ_BUF_SIZE: usize = 4096;

/// Maximum queued DA/DSR responses when pty_writer is contended. 64 is far
/// beyond normal usage (typically 1-2 responses in flight). Overflow means a
/// deadlock-like condition where the client holds pty_writer indefinitely.
const MAX_DEFERRED: usize = 64;
```

In `screen/mod.rs:36-41`, change:
```rust
/// Maximum number of pending responses/passthrough entries per process() call.
/// Prevents unbounded memory growth from malicious or pathological PTY output.
const MAX_PENDING: usize = 1024;

/// Maximum number of queued notifications preserved for replay on reconnect.
const MAX_QUEUED_NOTIFICATIONS: usize = 50;
```
to:
```rust
/// Maximum responses/passthrough entries buffered per process() call.
/// 1024 is a safety cap — normal output produces 0-2 responses (DA, DSR).
/// Pathological PTY output (e.g. 1000 DSR queries in one write) is truncated.
const MAX_PENDING: usize = 1024;

/// Notifications (OSC 9/777) queued for replay on reconnect. 50 prevents
/// a disconnected session from accumulating megabytes of stale notifications
/// while still preserving recent ones for the reconnecting client.
const MAX_QUEUED_NOTIFICATIONS: usize = 50;
```

In `screen/cell.rs`, find `MAX_COMBINING` and change:
```rust
const MAX_COMBINING: usize = 16;
```
to:
```rust
/// Maximum combining marks (diacritics, etc.) attached to a single base character.
/// Unicode NFC normalization typically produces 0-3 marks; 16 handles extreme
/// cases (e.g. Zalgo text) without unbounded allocation per cell.
const MAX_COMBINING: usize = 16;
```

In `server/session_bridge.rs:24-29`, the comments already exist. Add rationale to RENDER_THROTTLE:
```rust
/// Minimum interval between consecutive screen renders to the client.
/// 16ms ≈ 60fps — fast enough for smooth animation (progress bars, htop)
/// while preventing CPU waste from rendering every PTY read (1000s/sec).
pub(super) const RENDER_THROTTLE: std::time::Duration = std::time::Duration::from_millis(16);
```

In `server/mod.rs:17-18`:
```rust
/// Interval between dead session cleanup sweeps. 30s balances responsiveness
/// (dead sessions freed within half a minute) against lock contention overhead.
const CLEANUP_INTERVAL: std::time::Duration = std::time::Duration::from_secs(30);
```

In `server/client_handler.rs:11-12`:
```rust
/// Timeout for reading the initial client message (Connect/List/Kill).
/// 30s is generous for network latency while preventing leaked connections
/// from clients that connect but never send anything.
const INITIAL_MSG_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30);
```

In `client/mod.rs:193`, add a comment above the buffer:
```rust
// 1 KiB stdin buffer — matches typical terminal input rates. Even fast
// paste operations are chunked by the OS at ~4 KiB pipe buffer boundaries,
// so 1 KiB reads keep latency low without excessive syscalls.
let mut buf = [0u8; 1024];
```

In `screen/grid.rs`, find the tab stop initialization `c % 8 == 0` and add:
```rust
// Default tab stops every 8 columns per ECMA-48 / VT100 standard.
```

**Step 2: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 3: Commit**

```bash
git add src/session.rs src/screen/mod.rs src/screen/cell.rs src/server/session_bridge.rs src/server/mod.rs src/server/client_handler.rs src/client/mod.rs src/screen/grid.rs
git commit -m "docs: add rationale comments to magic number constants"
```

---

### Task 4: Phase 1d — Remove dead ScrollbackLine from protocol

**Files:**
- Modify: `src/protocol/messages.rs:36-37`
- Modify: `src/client/mod.rs:39-42`
- Modify: `src/protocol/codec.rs:444`
- Modify: `src/protocol/tests_history_protocol.rs:98-112`

**Step 1: Remove ScrollbackLine variant from ServerMsg**

In `messages.rs`, remove lines 36-37:
```rust
    /// Scrollback line (passthrough to terminal)
    ScrollbackLine(Vec<u8>),
```

**Step 2: Remove ScrollbackLine handling from client dispatch**

In `client/mod.rs:39-42`, remove:
```rust
ServerMsg::ScrollbackLine(line) => {
    stdout.write_all(line)?;
    stdout.write_all(b"\r\n")?;
}
```

**Step 3: Remove ScrollbackLine from codec test**

In `codec.rs:444`, remove from the test vector:
```rust
ServerMsg::ScrollbackLine(vec![0x41]),
```

**Step 4: Remove scrollback_line_message_round_trip test**

In `tests_history_protocol.rs`, remove the entire test function at lines 98-112.

**Step 5: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 6: Commit**

```bash
git add src/protocol/messages.rs src/client/mod.rs src/protocol/codec.rs src/protocol/tests_history_protocol.rs
git commit -m "refactor: remove unused ScrollbackLine protocol message

This variant was never sent by the server — it was an artifact of
an earlier design that was replaced by the History(Vec<Vec<u8>>) message."
```

---

### Task 5: Phase 1e — Document TOCTOU race as acceptable risk

**Files:**
- Modify: `src/server/mod.rs:29-42`

**Step 1: Expand the TOCTOU comment**

Replace the existing comment block at lines 29-41:
```rust
    // Only remove socket if it's stale (no server is listening).
    // This prevents a second server from yanking the socket out from under
    // an already-running server.
    if path.exists() {
        match tokio::net::UnixStream::connect(&path).await {
            Ok(_) => {
                anyhow::bail!("another server is already running on {:?}", path);
            }
            Err(_) => {
                // Stale socket — safe to remove
                let _ = std::fs::remove_file(&path);
            }
        }
    }
```
with:
```rust
    // Only remove socket if it's stale (no server is listening).
    // This prevents a second server from yanking the socket out from under
    // an already-running server.
    //
    // TOCTOU note: there is a small race window between remove_file() and
    // bind() below where another process could create a socket at the same
    // path. This is acceptable because:
    //   1. retach is a single-user tool — concurrent server starts are rare
    //   2. If a race occurs, bind() fails with EADDRINUSE and the user retries
    //   3. Using O_EXCL or flock() would add complexity for a near-zero-probability scenario
    if path.exists() {
        match tokio::net::UnixStream::connect(&path).await {
            Ok(_) => {
                anyhow::bail!("another server is already running on {:?}", path);
            }
            Err(_) => {
                // Stale socket — safe to remove
                if let Err(e) = std::fs::remove_file(&path) {
                    tracing::warn!(path = ?path, error = %e, "failed to remove stale socket");
                }
            }
        }
    }
```

**Step 2: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 3: Commit**

```bash
git add src/server/mod.rs
git commit -m "docs: document TOCTOU race in socket init as acceptable risk"
```

---

### Task 6: Phase 2a — Extract spawn_blocking_with_timeout helper

**Files:**
- Modify: `src/server/mod.rs`
- Modify: `src/server/client_handler.rs:71-77`

**Step 1: Add helper function to server/mod.rs**

Add after the imports (before `run_server`):

```rust
/// Drop a value on a blocking thread with a timeout. Used for Session drops
/// which call blocking kill()+wait() on child processes. The 5s timeout
/// prevents hangs when grandchild processes keep the PTY alive after kill.
async fn drop_blocking_with_timeout<T: Send + 'static>(value: T, label: &str) {
    let label = label.to_string();
    let result = tokio::time::timeout(
        std::time::Duration::from_secs(5),
        tokio::task::spawn_blocking(move || drop(value)),
    ).await;
    if result.is_err() {
        warn!(%label, "timed out dropping value on blocking thread");
    }
}
```

Replace the 2 instances in `mod.rs`:

At lines 65-73, replace:
```rust
if !dead_sessions.is_empty() {
    let count = dead_sessions.len();
    let result = tokio::time::timeout(
        std::time::Duration::from_secs(5),
        tokio::task::spawn_blocking(move || drop(dead_sessions)),
    ).await;
    if result.is_err() {
        warn!(count, "timed out cleaning up dead sessions");
    }
}
```
with:
```rust
if !dead_sessions.is_empty() {
    drop_blocking_with_timeout(dead_sessions, "dead session cleanup").await;
}
```

At lines 117-125, replace:
```rust
if !all_sessions.is_empty() {
    info!(count = all_sessions.len(), "cleaning up sessions on shutdown");
    let result = tokio::time::timeout(
        std::time::Duration::from_secs(5),
        tokio::task::spawn_blocking(move || drop(all_sessions)),
    ).await;
    if result.is_err() {
        warn!("timed out waiting for sessions to clean up");
    }
}
```
with:
```rust
if !all_sessions.is_empty() {
    info!(count = all_sessions.len(), "cleaning up sessions on shutdown");
    drop_blocking_with_timeout(all_sessions, "shutdown session cleanup").await;
}
```

Make the function `pub(super)` so client_handler can use it.

In `client_handler.rs:71-77`, replace:
```rust
let result = tokio::time::timeout(
    std::time::Duration::from_secs(5),
    tokio::task::spawn_blocking(move || drop(session)),
).await;
if result.is_err() {
    tracing::warn!(session = %name, "timed out dropping killed session");
}
```
with:
```rust
super::drop_blocking_with_timeout(session, &format!("kill session '{}'", name)).await;
```

**Step 2: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 3: Commit**

```bash
git add src/server/mod.rs src/server/client_handler.rs
git commit -m "refactor: extract drop_blocking_with_timeout helper in server/"
```

---

### Task 7: Phase 2b — Extract send_error helper in session_setup.rs

**Files:**
- Modify: `src/server/session_setup.rs:102-155`

**Step 1: Add helper function**

Add before `setup_session()`:
```rust
/// Send an error message to the client and return the error for the caller to propagate.
async fn send_error_to_client(
    stream: &mut tokio::net::UnixStream,
    msg: String,
) -> anyhow::Error {
    if let Ok(resp) = protocol::encode(&ServerMsg::Error(msg.clone())) {
        let _ = stream.write_all(&resp).await;
    }
    anyhow::anyhow!("{}", msg)
}
```

**Step 2: Replace repeated patterns in setup_session()**

Replace the match body at lines 114-155 with:
```rust
    let (session, is_new) = match mode {
        ConnectMode::CreateOrAttach => {
            match mgr.get_or_create(name, cols, rows, history) {
                Ok(s) => s,
                Err(e) => {
                    let msg = format!("{}", e);
                    return Err(send_error_to_client(stream, msg).await);
                }
            }
        }
        ConnectMode::CreateOnly => {
            if mgr.get(name).is_some() {
                return Err(send_error_to_client(stream, format!("session '{}' already exists", name)).await);
            }
            if let Err(e) = mgr.create(name.to_string(), cols, rows, history) {
                let msg = format!("{}", e);
                return Err(send_error_to_client(stream, msg).await);
            }
            match mgr.get_mut(name) {
                Some(s) => (s, true),
                None => {
                    return Err(send_error_to_client(stream, "session disappeared after creation".into()).await);
                }
            }
        }
        ConnectMode::AttachOnly => {
            match mgr.get_mut(name) {
                Some(s) => (s, false),
                None => {
                    return Err(send_error_to_client(stream, format!("session '{}' not found", name)).await);
                }
            }
        }
    };
```

**Step 3: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 4: Commit**

```bash
git add src/server/session_setup.rs
git commit -m "refactor: extract send_error_to_client helper in session_setup"
```

---

### Task 8: Phase 3a — Split performer print() into sub-functions

**Files:**
- Modify: `src/screen/performer.rs:435-528`

**Step 1: Extract handle_combining_mark()**

Add as a method on `ScreenPerformer<G>`, before the `Perform` impl:
```rust
/// Attach a zero-width combining mark to the previous cell.
fn handle_combining_mark(&mut self, c: char) {
    let cx = self.grid.cursor_x() as usize;
    let cy = self.grid.cursor_y() as usize;
    if cy >= self.grid.rows() as usize {
        return;
    }
    let tx = if self.grid.wrap_pending() {
        cx
    } else if cx > 0 {
        cx - 1
    } else {
        return;
    };
    if tx >= self.grid.cols() as usize {
        return;
    }
    // If the target is a continuation cell (width==0), step back
    // one more to reach the actual wide character cell.
    let tx = if self.grid.visible_row(cy)[tx].width == 0 && tx > 0 {
        tx - 1
    } else {
        tx
    };
    if self.grid.visible_row(cy).combining_len(tx as u16) < MAX_COMBINING {
        self.grid.visible_row_mut(cy).push_combining(tx as u16, c);
    }
}
```

**Step 2: Extract perform_deferred_wrap()**

```rust
/// Execute a deferred line wrap: advance to column 0 of the next row,
/// scrolling if at the bottom of the scroll region.
fn perform_deferred_wrap(&mut self) {
    self.grid.set_wrap_pending(false);
    self.grid.set_cursor_x_unclamped(0);
    if self.grid.cursor_y() == self.grid.scroll_bottom() {
        self.scroll_up();
    } else if self.grid.cursor_y() < self.grid.rows() - 1 {
        self.grid.set_cursor_y_unclamped(self.grid.cursor_y() + 1);
    }
}
```

**Step 3: Simplify print()**

Replace the body of `print()` with:
```rust
fn print(&mut self, c: char) {
    let c = self.map_charset(c);
    let char_width = UnicodeWidthChar::width(c).unwrap_or(0) as u16;

    if char_width == 0 {
        self.handle_combining_mark(c);
        return;
    }

    if self.grid.wrap_pending() && self.grid.modes().autowrap_mode {
        self.perform_deferred_wrap();
    }

    // Wide char at end of line: fill margin cell with space and wrap
    if char_width == 2 && self.grid.cursor_x() >= self.grid.cols() - 1 {
        if self.grid.modes().autowrap_mode {
            if self.grid.cols() < 2 {
                return;
            }
            let x = self.grid.cursor_x() as usize;
            let y = self.grid.cursor_y() as usize;
            if x < self.grid.cols() as usize && y < self.grid.rows() as usize {
                let blank = self.blank_cell();
                self.grid.visible_row_mut(y)[x] = blank;
            }
            self.perform_deferred_wrap();
        } else {
            return;
        }
    }

    let x = self.grid.cursor_x() as usize;
    let y = self.grid.cursor_y() as usize;
    if x < self.grid.cols() as usize && y < self.grid.rows() as usize {
        let sid = self.intern_with_gc(self.state.current_style);
        self.grid.set_cell(x, y, Cell::new(c, sid, char_width as u8));

        self.state.last_printed_char = c;
        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);
        }
    }
}
```

**Step 4: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass (print() behavior unchanged)

**Step 5: Commit**

```bash
git add src/screen/performer.rs
git commit -m "refactor: split print() into handle_combining_mark() and perform_deferred_wrap()"
```

---

### Task 9: Phase 3b — Split render_screen_impl() into sub-functions

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

**Step 1: Extract render_scrollback()**

Add before `render_screen_impl()`:
```rust
/// Inject scrollback lines into the terminal's native scrollback buffer.
/// Emitted OUTSIDE the synchronized output block — some terminals (Blink/hterm)
/// buffer all sync'd output atomically, preventing intermediate scroll operations
/// from pushing content into native scrollback.
fn render_scrollback(out: &mut Vec<u8>, grid: &Grid, scrollback: &[Vec<u8>]) {
    let rows = grid.rows() as usize;
    out.extend_from_slice(b"\x1b[?25l\x1b[r");

    for chunk in scrollback.chunks(rows) {
        for (i, line) in chunk.iter().enumerate() {
            out.extend_from_slice(b"\x1b[");
            write_u16(out, (i + 1) as u16);
            out.extend_from_slice(b";1H\x1b[0m");
            out.extend_from_slice(line);
            out.extend_from_slice(b"\x1b[K");
        }
        if chunk.len() < rows {
            for i in chunk.len()..rows {
                out.extend_from_slice(b"\x1b[");
                write_u16(out, (i + 1) as u16);
                out.extend_from_slice(b";1H\x1b[2K");
            }
        }
        out.extend_from_slice(b"\x1b[");
        write_u16(out, grid.rows());
        out.extend_from_slice(b";1H");
        out.extend(std::iter::repeat_n(b'\n', chunk.len()));
    }
}
```

**Step 2: Extract render_modes_title_cursor()**

```rust
/// Emit scroll region, cursor position, terminal modes, window title, and
/// cursor visibility. Must be called inside the synchronized output block.
fn render_modes_title_cursor(
    out: &mut Vec<u8>,
    grid: &Grid,
    title: &str,
    full: bool,
    cache: &mut RenderCache,
) {
    // Scroll region (DECSTBM): must be emitted BEFORE cursor position because
    // setting the scroll region resets the cursor to home.
    let scroll_region = grid.scroll_region();
    if full || cache.last_scroll_region != Some(scroll_region) {
        out.extend_from_slice(b"\x1b[");
        write_u16(out, scroll_region.0 + 1);
        out.push(b';');
        write_u16(out, scroll_region.1 + 1);
        out.push(b'r');
        cache.last_scroll_region = Some(scroll_region);
    }

    // Cursor position (after scroll region, since DECSTBM resets cursor)
    out.extend_from_slice(b"\x1b[");
    let (cx, cy) = grid.cursor_pos();
    write_u16(out, cy + 1);
    out.push(b';');
    write_u16(out, cx + 1);
    out.push(b'H');

    // Mode sequences: full sends all, incremental sends delta only
    let modes = grid.modes();
    match &cache.last_modes {
        Some(prev) if !full => emit_mode_delta(out, modes, prev),
        _ => emit_mode(out, modes),
    }
    cache.last_modes = Some(modes.clone());

    // Window title: only send if changed (sanitized to prevent OSC injection)
    if title != cache.last_title {
        out.extend_from_slice(b"\x1b]2;");
        for &b in title.as_bytes() {
            if b >= 0x20 && b != 0x7f {
                out.push(b);
            }
        }
        out.push(0x07);
        cache.last_title = title.to_string();
    }

    // Cursor visibility: always restore since we hide it at the top for redraw
    if grid.cursor_visible() {
        out.extend_from_slice(b"\x1b[?25h");
    }
}
```

**Step 3: Simplify render_screen_impl()**

Replace the function body with:
```rust
fn render_screen_impl(
    grid: &Grid,
    title: &str,
    scrollback: &[Vec<u8>],
    full: bool,
    cache: &mut RenderCache,
) -> Vec<u8> {
    let mut out = Vec::new();

    if !scrollback.is_empty() {
        render_scrollback(&mut out, grid, scrollback);
        cache.invalidate();
    }

    // Synchronized output: begin
    out.extend_from_slice(b"\x1b[?2026h");
    out.extend_from_slice(b"\x1b[?25l");

    let full = full || !scrollback.is_empty();

    if full {
        out.extend_from_slice(b"\x1b[0m\x1b[2J\x1b[H");
        cache.invalidate();
    }

    // Ensure cache row_hashes is the right length
    let num_rows = grid.visible_row_count();
    if cache.row_hashes.len() != num_rows {
        cache.row_hashes.resize(num_rows, u64::MAX);
    }

    for (y, row) in grid.visible_rows().enumerate() {
        let row_hash = hash_row(row);

        if !full && cache.row_hashes[y] == row_hash {
            continue;
        }
        cache.row_hashes[y] = row_hash;

        out.extend_from_slice(b"\x1b[");
        write_u16(&mut out, y as u16 + 1);
        out.extend_from_slice(b";1H");

        let write_len = last_content_position(row)
            .map(|p| p + 1)
            .unwrap_or(0);

        let mut last_sid = StyleId::default();
        for (x, cell) in row.iter().enumerate() {
            if x >= write_len { break; }
            if cell.width == 0 { continue; }
            if cell.style_id != last_sid {
                out.extend_from_slice(&grid.style_table().get(cell.style_id).to_sgr_with_reset());
                last_sid = cell.style_id;
            }
            let mut buf = [0u8; 4];
            out.extend_from_slice(cell.c.encode_utf8(&mut buf).as_bytes());
            for &mark in row.combining(x as u16) {
                out.extend_from_slice(mark.encode_utf8(&mut buf).as_bytes());
            }
        }

        out.extend_from_slice(b"\x1b[0m\x1b[K");
    }

    render_modes_title_cursor(&mut out, grid, title, full, cache);

    // End synchronized output
    out.extend_from_slice(b"\x1b[?2026l");
    out
}
```

**Step 4: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 5: Commit**

```bash
git add src/screen/render.rs
git commit -m "refactor: split render_screen_impl() into render_scrollback() and render_modes_title_cursor()"
```

---

### Task 10: Phase 3c — Split connect() into smaller functions

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

**Step 1: Extract spawn_signal_handlers()**

Add before `connect()`:
```rust
/// Spawn SIGWINCH handler that sends Resize + RefreshScreen on terminal size change.
fn spawn_sigwinch_handler(
    sock_writer: Arc<tokio::sync::Mutex<tokio::net::unix::OwnedWriteHalf>>,
) -> anyhow::Result<tokio::task::JoinHandle<()>> {
    let mut sigwinch =
        tokio::signal::unix::signal(tokio::signal::unix::SignalKind::window_change())?;
    let sw = sock_writer;
    Ok(tokio::spawn(async move {
        while sigwinch.recv().await.is_some() {
            let (cols, rows) = get_terminal_size();
            let mut w = sw.lock().await;
            if let Ok(msg) = protocol::encode(&ClientMsg::Resize { cols, rows }) {
                if let Err(e) = w.write_all(&msg).await {
                    tracing::debug!(error = %e, "failed to send resize");
                    break;
                }
            }
            if let Ok(msg) = protocol::encode(&ClientMsg::RefreshScreen) {
                if let Err(e) = w.write_all(&msg).await {
                    tracing::debug!(error = %e, "failed to send refresh after resize");
                    break;
                }
            }
        }
    }))
}
```

**Step 2: Refactor connect() to use the helper**

In `connect()`, replace the SIGWINCH block (lines 335-360) with:
```rust
let sigwinch_handle = spawn_sigwinch_handler(sock_writer.clone())?;
```

**Step 3: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 4: Commit**

```bash
git add src/client/mod.rs
git commit -m "refactor: extract spawn_sigwinch_handler() from connect()"
```

---

### Task 11: Phase 4a — Fix Session::Drop to use try_lock

**Files:**
- Modify: `src/session.rs:264-281`

**Step 1: Replace blocking lock() with try_lock()**

Replace the Drop impl:
```rust
impl Drop for Session {
    fn drop(&mut self) {
        // Use try_lock() to avoid blocking the async runtime if the child lock
        // is contended. In practice it's never contended during drop because
        // the persistent reader has already exited (or will exit after kill).
        match self.pty.child_arc().try_lock() {
            Ok(mut child) => {
                if let Err(e) = child.kill() {
                    tracing::debug!(session = %self.name, error = %e, "failed to kill child process (may have already exited)");
                }
                if let Err(e) = child.wait() {
                    tracing::debug!(session = %self.name, error = %e, "failed to wait on child process");
                }
            }
            Err(_) => {
                tracing::warn!(session = %self.name, "child mutex contended during drop, skipping kill/wait");
            }
        }
        // Evict any connected client
        if let Some(tx) = self.evict_tx.take() {
            if tx.send(false).is_err() {
                tracing::debug!(session = %self.name, "evict channel closed during session drop");
            }
        }
        // Wait for the reader thread to exit (it will see EOF after child kill).
        if let Some(handle) = self.reader_handle.take() {
            if let Err(_) = handle.join() {
                tracing::warn!(session = %self.name, "reader thread panicked during session drop");
            }
        }
    }
}
```

Note: All callers already use `spawn_blocking()` for Session drop, so try_lock() will succeed. The try_lock() is a safety net — if somehow Drop runs on the async runtime, it won't block.

**Step 2: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass

**Step 3: Commit**

```bash
git add src/session.rs
git commit -m "fix: use try_lock() in Session::Drop to prevent blocking async runtime"
```

---

### Task 12: Phase 4b — Refactor InputFilter to reduce nesting

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

**Step 1: Refactor InputFilter::process()**

Replace the `process()` method with a cleaner version using early returns and helper patterns:

```rust
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 {
            filtered.push(raw[i]);
            i += 1;
            continue;
        }

        // ESC at end of buffer — carry for next call
        let remaining = raw.len() - i;
        if remaining < 2 {
            Self::flush_filtered(&mut actions, &mut filtered);
            self.carry.extend_from_slice(&raw[i..]);
            return actions;
        }

        // ESC <non-[> — pass through as-is
        if raw[i + 1] != b'[' {
            filtered.push(raw[i]);
            i += 1;
            continue;
        }

        // ESC [ at end of buffer — carry for next call
        if remaining < 3 {
            Self::flush_filtered(&mut actions, &mut filtered);
            self.carry.extend_from_slice(&raw[i..]);
            return actions;
        }

        // ESC [ I — focus in
        if raw[i + 2] == FOCUS_IN {
            Self::flush_filtered(&mut actions, &mut filtered);
            actions.push(FilterAction::FocusIn);
            i += 3;
            continue;
        }

        // ESC [ O — focus out (consumed, not forwarded)
        if raw[i + 2] == FOCUS_OUT {
            i += 3;
            continue;
        }

        // ESC [ <other> — pass through
        filtered.push(raw[i]);
        i += 1;
    }
    Self::flush_filtered(&mut actions, &mut filtered);
    actions
}

/// Push accumulated bytes as a Forward action if non-empty.
fn flush_filtered(actions: &mut Vec<FilterAction>, filtered: &mut Vec<u8>) {
    if !filtered.is_empty() {
        actions.push(FilterAction::Forward(std::mem::take(filtered)));
    }
}
```

**Step 2: Run tests**

Run: `cargo test 2>&1 | tail -5`
Expected: all tests pass (InputFilter behavior unchanged — same tests cover it)

**Step 3: Add edge case tests for InputFilter**

Add to the existing test module:

```rust
#[test]
fn input_filter_esc_bracket_at_boundary() {
    let mut f = InputFilter::new();
    // ESC [ split across two calls
    let a1 = f.process(b"\x1b");
    assert!(a1.is_empty(), "lone ESC should be carried");
    let a2 = f.process(b"[I");
    assert_eq!(a2, vec![FilterAction::FocusIn]);
}

#[test]
fn input_filter_esc_nonbracket_passthrough() {
    let mut f = InputFilter::new();
    let actions = f.process(b"\x1bOA"); // ESC O A = cursor up (SS3)
    assert_eq!(actions, vec![FilterAction::Forward(b"\x1bOA".to_vec())]);
}

#[test]
fn input_filter_multiple_focus_events() {
    let mut f = InputFilter::new();
    let actions = f.process(b"a\x1b[Ib\x1b[Oc");
    // FocusIn extracted, FocusOut consumed, "a", "b", "c" forwarded
    assert_eq!(actions, vec![
        FilterAction::Forward(b"a".to_vec()),
        FilterAction::FocusIn,
        FilterAction::Forward(b"bc".to_vec()),
    ]);
}
```

**Step 4: Run tests**

Run: `cargo test input_filter 2>&1`
Expected: all pass

**Step 5: Commit**

```bash
git add src/client/mod.rs
git commit -m "refactor: flatten InputFilter::process() nesting, add edge case tests"
```

---

### Task 13: Final — Run full test suite and verify

**Step 1: Run full test suite**

Run: `cargo test 2>&1 | tail -20`
Expected: all tests pass

**Step 2: Run clippy**

Run: `cargo clippy 2>&1 | tail -20`
Expected: no new warnings

**Step 3: Verify build**

Run: `cargo build --release 2>&1 | tail -5`
Expected: successful build
