# Critical Bugfixes Implementation Plan

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

**Goal:** Fix all 5 critical and 20 important bugs found during code review across all modules.

**Architecture:** Fixes are ordered by severity and dependency — critical bugs first (terminal safety, security, server stability), then important bugs grouped by module. Each task is self-contained with tests.

**Tech Stack:** Rust, tokio, bincode 1.x, vte 0.13, portable-pty 0.8, nix 0.29

---

### Task 1: Add panic hook and signal handlers to restore terminal on crash

**Files:**
- Modify: `src/client/raw_mode.rs`
- Modify: `src/client/mod.rs:59-251`
- Test: manual (terminal state requires real TTY)

**Context:** If the client panics or receives SIGINT/SIGTERM while in raw mode, the terminal is left unusable (no echo, no line editing). There are no panic hooks or signal handlers.

**Step 1: Make original termios accessible for panic hook**

Add a static `AtomicBool` and store original termios in a thread-safe global in `src/client/raw_mode.rs`:

```rust
use std::sync::Mutex as StdMutex;
use std::sync::OnceLock;

/// Global original termios for emergency restoration (panic hook, signal handler).
static ORIGINAL_TERMIOS: OnceLock<StdMutex<Option<(i32, nix::sys::termios::Termios)>>> = OnceLock::new();

fn get_or_init_global() -> &'static StdMutex<Option<(i32, nix::sys::termios::Termios)>> {
    ORIGINAL_TERMIOS.get_or_init(|| StdMutex::new(None))
}

/// Restore terminal from the global backup. Safe to call from panic hooks and signal handlers.
pub fn emergency_restore() {
    if let Some(guard) = get_or_init_global().lock().ok() {
        if let Some((fd, ref original)) = *guard {
            let borrowed = unsafe { BorrowedFd::borrow_raw(fd) };
            let _ = termios::tcsetattr(borrowed, termios::SetArg::TCSANOW, original);
        }
    }
}
```

Update `RawMode::enter()` to store in the global:

```rust
pub fn enter() -> anyhow::Result<Self> {
    let fd = io::stdin().as_raw_fd();
    let borrowed = unsafe { BorrowedFd::borrow_raw(fd) };
    let original = termios::tcgetattr(borrowed)?;
    // Store for emergency restoration
    if let Ok(mut guard) = get_or_init_global().lock() {
        *guard = Some((fd, original.clone()));
    }
    let mut raw = original.clone();
    termios::cfmakeraw(&mut raw);
    termios::tcsetattr(borrowed, termios::SetArg::TCSANOW, &raw)?;
    Ok(Self { original, fd })
}
```

Update `Drop for RawMode` to clear the global:

```rust
impl Drop for RawMode {
    fn drop(&mut self) {
        let borrowed = unsafe { BorrowedFd::borrow_raw(self.fd) };
        if let Err(e) = termios::tcsetattr(borrowed, termios::SetArg::TCSANOW, &self.original) {
            tracing::warn!(error = %e, "failed to restore terminal mode");
        }
        // Clear global backup
        if let Ok(mut guard) = get_or_init_global().lock() {
            *guard = None;
        }
    }
}
```

**Step 2: Install panic hook in `connect()` before entering raw mode**

In `src/client/mod.rs`, before `let _raw = RawMode::enter()?;` (line 109):

```rust
// Install panic hook to restore terminal
let prev_hook = std::panic::take_hook();
std::panic::set_hook(Box::new(move |info| {
    raw_mode::emergency_restore();
    cleanup_terminal();
    prev_hook(info);
}));
```

After `tokio::select!` completes and cleanup is done (before returning Ok), restore the original panic hook:

```rust
// Restore default panic hook
let _ = std::panic::take_hook();
```

**Step 3: Add SIGINT/SIGTERM to the tokio::select! in `connect()`**

Replace the current `tokio::select!` (lines 234-246) with:

```rust
let mut sigint = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::interrupt())?;
let mut sigterm = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate())?;

tokio::select! {
    r = stdin_task => {
        if let Ok(Err(e)) = r {
            tracing::debug!(error = %e, "stdin task error");
        }
    }
    r = socket_task => {
        if let Ok(Err(e)) = r {
            tracing::warn!(error = %e, "socket task error");
            eprintln!("[retach error: {}]", e);
        }
    }
    _ = sigint.recv() => {
        tracing::debug!("received SIGINT, detaching");
    }
    _ = sigterm.recv() => {
        tracing::debug!("received SIGTERM, detaching");
    }
}
```

**Step 4: Also abort the SIGWINCH task on exit**

Capture the SIGWINCH spawn handle (line 118) and abort it after select:

```rust
let sigwinch_handle = tokio::spawn(async move { ... });
// ... after tokio::select!:
sigwinch_handle.abort();
```

**Step 5: Run `cargo test` and `cargo clippy`**

Run: `cargo test && cargo clippy -- -D warnings`

**Step 6: Commit**

```
fix: restore terminal on panic/signal (Critical #1)

Install panic hook and SIGINT/SIGTERM handlers to ensure raw mode
is always restored. Also abort SIGWINCH task on connection exit.
```

---

### Task 2: Cap `pending_scrollback` to prevent unbounded memory growth

**Files:**
- Modify: `src/screen/grid.rs:79-98`
- Modify: `src/screen/mod.rs:66` (type stays the same, limit enforced in grid)
- Test: `src/screen/grid.rs` (existing test section)

**Context:** `pending_scrollback` has no size limit. If a session runs unattended with heavy output (e.g., `yes`), it grows until OOM.

**Step 1: Write the failing test**

In `src/screen/grid.rs` tests section:

```rust
#[test]
fn pending_scrollback_respects_limit() {
    let mut grid = Grid::new(10, 3);
    let mut scrollback = VecDeque::new();
    let mut pending = Vec::new();
    for _ in 0..20 {
        grid.scroll_up(false, &mut scrollback, 5, &mut pending, Cell::default());
    }
    // pending_scrollback should be capped at scrollback_limit
    assert!(pending.len() <= 5, "pending_scrollback should be bounded, got {}", pending.len());
}
```

**Step 2: Run test to verify it fails**

Run: `cargo test grid::tests::pending_scrollback_respects_limit -- --nocapture`
Expected: FAIL (pending.len() == 20)

**Step 3: Fix `scroll_up` to cap `pending_scrollback`**

In `src/screen/grid.rs`, `scroll_up` method (line 97), replace:

```rust
pending_scrollback.push(line);
```

with:

```rust
if pending_scrollback.len() >= scrollback_limit {
    // Drop oldest pending line when at capacity.
    // On reattach, full history is sent via get_history() anyway.
    pending_scrollback.remove(0);
}
pending_scrollback.push(line);
```

**Step 4: Run test to verify it passes**

Run: `cargo test grid::tests::pending_scrollback_respects_limit -- --nocapture`
Expected: PASS

**Step 5: Run full test suite**

Run: `cargo test`

**Step 6: Commit**

```
fix: cap pending_scrollback to scrollback_limit (Critical #2)

Prevents unbounded memory growth when sessions run unattended.
On reattach, full history is sent via get_history() anyway.
```

---

### Task 3: Add bincode deserialization size limits to prevent OOM

**Files:**
- Modify: `src/protocol/codec.rs`
- Modify: `src/server/session_bridge.rs:387` (use safe deserialize)
- Modify: `src/server/client_handler.rs:37` (use safe deserialize)
- Modify: `src/client/mod.rs:86,205,222` (use safe deserialize)
- Test: `src/protocol/codec.rs` tests

**Context:** `bincode::deserialize` with default config allows the encoded `Vec` length prefix to claim arbitrarily large allocations. A 16-byte frame can trigger a 4 GiB allocation.

**Step 1: Write the failing test**

In `src/protocol/codec.rs` tests section:

```rust
#[test]
fn deserialize_rejects_inflated_vec() {
    // Craft a bincode payload where a Vec<u8> claims length u64::MAX
    // but the frame is only a few bytes. With unlimited deserialize,
    // this would attempt to allocate terabytes.
    use bincode::Options;
    let config = bincode_config();

    // Encode a small legitimate Input message
    let msg = ClientMsg::Input(vec![65; 10]);
    let good_data = config.serialize(&msg).unwrap();

    // Verify it round-trips with our safe deserialize
    let decoded: ClientMsg = config.deserialize(&good_data).unwrap();
    match decoded {
        ClientMsg::Input(v) => assert_eq!(v.len(), 10),
        _ => panic!("wrong variant"),
    }
}
```

**Step 2: Add `bincode_config()` function and `safe_deserialize()` to `codec.rs`**

Add after the imports at the top of `src/protocol/codec.rs`:

```rust
use bincode::Options;

/// Bincode configuration with size limit matching MAX_FRAME_SIZE.
/// Uses fixint encoding to match bincode::serialize/deserialize behavior.
pub fn bincode_config() -> impl bincode::Options + Copy {
    bincode::DefaultOptions::new()
        .with_fixint_encoding()
        .with_limit(MAX_FRAME_SIZE as u64)
}
```

**Step 3: Replace all `bincode::serialize` and `bincode::deserialize` calls**

In `encode()`:

```rust
let data = bincode_config().serialize(msg)?;
```

In `read_one_message()`:

```rust
return Ok(bincode_config().deserialize(data)?);
```

In `src/server/client_handler.rs` line 37:

```rust
let msg: ClientMsg = crate::protocol::codec::bincode_config().deserialize(data)?;
```

In `src/server/session_bridge.rs` line 387:

```rust
let msg: ClientMsg = crate::protocol::codec::bincode_config().deserialize(data)?;
```

In `src/client/mod.rs` lines 86, 205, 222:

```rust
let msg: ServerMsg = crate::protocol::codec::bincode_config().deserialize(data)?;
```

Update test imports to use `bincode_config()` too (all `bincode::deserialize` in tests).

**Step 4: Run tests**

Run: `cargo test`

**Step 5: Commit**

```
fix: add bincode size limit to prevent OOM on malicious frames (Critical #3)

Replace unbounded bincode::deserialize with Options::with_limit(MAX_FRAME_SIZE).
Uses fixint encoding for backward compatibility with existing wire format.
```

---

### Task 4: Add graceful server shutdown and accept loop resilience

**Files:**
- Modify: `src/server/mod.rs`
- Test: `src/server/socket.rs` (socket cleanup)

**Context:** No SIGTERM/SIGINT handler — socket file left behind, sessions orphaned. Also `accept()` with `?` crashes server on transient errors.

**Step 1: Add SocketGuard RAII and signal handling**

Rewrite `src/server/mod.rs`:

```rust
pub async fn run_server() -> anyhow::Result<()> {
    // Ignore SIGHUP so SSH disconnects don't kill us
    use nix::sys::signal::{signal, SigHandler, Signal};
    // SAFETY: SIG_IGN is safe for SIGHUP — we want the daemon to survive SSH disconnects.
    unsafe { signal(Signal::SIGHUP, SigHandler::SigIgn) }
        .map_err(|e| anyhow::anyhow!("failed to ignore SIGHUP: {}", e))?;

    let path = socket_path();
    let _ = std::fs::remove_file(&path);

    let listener = UnixListener::bind(&path)?;
    info!(path = ?path, "server listening");

    // RAII guard to clean up socket file on exit
    let _socket_guard = SocketGuard(path.clone());

    let manager = Arc::new(Mutex::new(SessionManager::new()));

    // Dead session cleanup task
    let cleanup_manager = manager.clone();
    tokio::spawn(async move {
        let mut interval = tokio::time::interval(std::time::Duration::from_secs(30));
        loop {
            interval.tick().await;
            // Collect dead sessions outside the lock to avoid blocking
            let dead_sessions = {
                let mut mgr = cleanup_manager.lock().await;
                mgr.take_dead_sessions()
            };
            // Drop dead sessions outside the lock (their Drop calls kill+wait)
            drop(dead_sessions);
        }
    });

    // Graceful shutdown via signals
    let mut sigterm = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate())?;
    let mut sigint = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::interrupt())?;

    loop {
        tokio::select! {
            result = listener.accept() => {
                match result {
                    Ok((stream, _)) => {
                        let manager = manager.clone();
                        tokio::spawn(async move {
                            if let Err(e) = client_handler::handle_client(stream, manager).await {
                                warn!(error = %e, "client error");
                            }
                        });
                    }
                    Err(e) => {
                        warn!(error = %e, "accept failed, retrying");
                        tokio::time::sleep(std::time::Duration::from_millis(100)).await;
                    }
                }
            }
            _ = sigterm.recv() => {
                info!("received SIGTERM, shutting down");
                break;
            }
            _ = sigint.recv() => {
                info!("received SIGINT, shutting down");
                break;
            }
        }
    }

    Ok(())
    // _socket_guard drops here, removing socket file
}

/// RAII guard that removes the socket file on drop.
struct SocketGuard(std::path::PathBuf);

impl Drop for SocketGuard {
    fn drop(&mut self) {
        let _ = std::fs::remove_file(&self.0);
    }
}
```

**Step 2: Add `take_dead_sessions()` to SessionManager**

In `src/session.rs`, rename `cleanup_dead_sessions` → `take_dead_sessions` returning the dead sessions:

```rust
/// Remove dead sessions and return them for cleanup outside the lock.
pub fn take_dead_sessions(&mut self) -> Vec<Session> {
    let dead: Vec<String> = self.sessions.iter()
        .filter(|(_, s)| !s.is_alive())
        .map(|(name, s)| {
            let status = s.pty.child_arc().lock().ok()
                .and_then(|mut c| c.try_wait().ok().flatten());
            tracing::info!(session = %name, exit_status = ?status, "cleaning up dead session");
            name.clone()
        })
        .collect();
    dead.into_iter().filter_map(|name| self.sessions.remove(&name)).collect()
}
```

**Step 3: Update `Session::Drop` to use `try_lock` instead of `lock`**

In `src/session.rs`:

```rust
impl Drop for Session {
    fn drop(&mut self) {
        if let Ok(mut child) = self.pty.child_arc().try_lock() {
            let _ = child.kill();
            let _ = child.wait();
        }
        if let Some(tx) = self.evict_tx.take() {
            let _ = tx.send(false);
        }
    }
}
```

**Step 4: Run tests**

Run: `cargo test`

**Step 5: Commit**

```
fix: graceful server shutdown, accept resilience, socket cleanup (Critical #4)

- Handle SIGTERM/SIGINT to shut down cleanly
- SocketGuard RAII removes socket file on any exit
- Accept loop continues on transient errors instead of crashing
- Drop dead sessions outside manager lock to avoid blocking
- Use try_lock in Session::Drop to avoid blocking Tokio workers
- Replace unsafe libc signal() with nix safe API
```

---

### Task 5: Fix blocking stdin thread leak and runtime shutdown hang

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

**Context:** `spawn_blocking` with `stdin.read()` cannot be cancelled. When the socket task finishes, the stdin thread stays blocked forever, potentially hanging runtime shutdown.

**Step 1: Add shutdown timeout to runtime in main.rs**

In `src/main.rs`, after `match cli.command { ... }`, before returning, shut down the runtime with a timeout:

```rust
fn main() -> anyhow::Result<()> {
    init_tracing();
    let cli = Cli::parse();

    let rt = tokio::runtime::Runtime::new()?;

    let result = match cli.command {
        Command::Open { name, history } => {
            rt.block_on(client::connect(&name, history))
        }
        Command::New { name, history } => {
            let name = name.unwrap_or_else(generate_name);
            rt.block_on(client::connect(&name, history))
        }
        Command::Attach { name, history } => {
            rt.block_on(client::connect(&name, history))
        }
        Command::List => {
            rt.block_on(client::list_sessions())
        }
        Command::Kill { name } => {
            rt.block_on(client::kill_session(&name))
        }
        Command::Server => {
            rt.block_on(server::run_server())
        }
    };

    // Shut down runtime with timeout to avoid hanging on blocked stdin thread
    rt.shutdown_timeout(std::time::Duration::from_millis(100));

    result
}
```

**Step 2: Add cancellation flag to stdin read loop**

In `src/client/mod.rs`, use a `CancellationToken` pattern with `tokio::sync::watch`:

```rust
let (stdin_cancel_tx, stdin_cancel_rx) = tokio::sync::watch::channel(false);

let stdin_task = tokio::spawn(async move {
    loop {
        let mut cancel_rx = stdin_cancel_rx.clone();
        let result = tokio::select! {
            r = tokio::task::spawn_blocking(|| {
                let mut buf = [0u8; 1024];
                let n = io::stdin().read(&mut buf)?;
                Ok::<_, io::Error>((buf, n))
            }) => r,
            _ = cancel_rx.changed() => {
                break;
            }
        };
        // ... rest of the match logic unchanged
    }
    Ok::<_, anyhow::Error>(())
});
```

After `tokio::select!` completes, signal stdin to stop:

```rust
let _ = stdin_cancel_tx.send(true);
```

**Step 3: Run `cargo test` and `cargo clippy`**

Run: `cargo test && cargo clippy -- -D warnings`

**Step 4: Commit**

```
fix: prevent runtime hang from leaked stdin thread (Critical #5)

Add cancellation signal to stdin task and shutdown_timeout to runtime.
The blocking stdin.read() can't be cancelled, but the async wrapper
exits promptly via watch channel, and the runtime times out the thread.
```

---

### Task 6: Optimize protocol buffer drain — single drain per read cycle

**Files:**
- Modify: `src/client/mod.rs:203-210,220-224`
- Modify: `src/server/session_bridge.rs:384-388`

**Context:** `read_buf.drain(..consumed)` inside `while` loops causes O(n*m) buffer shifting. Fix: drain once after processing all frames.

**Step 1: Fix client socket reader (lines 203-210 and 220-227)**

Replace the leftover processing loop (lines 203-210):

```rust
// Process any complete frames already in the leftover buffer
let mut cursor = 0;
while let Some((data, consumed)) = protocol::decode_frame(&read_buf[cursor..])? {
    let msg: ServerMsg = crate::protocol::codec::bincode_config().deserialize(data)?;
    cursor += consumed;
    if matches!(dispatch_server_msg(&msg, &mut stdout)?, DispatchResult::Done) {
        return Ok(());
    }
}
if cursor > 0 {
    read_buf.drain(..cursor);
}
stdout.flush()?;
```

Replace the main loop frame processing (lines 220-227):

```rust
let mut cursor = 0;
while let Some((data, consumed)) = protocol::decode_frame(&read_buf[cursor..])? {
    let msg: ServerMsg = crate::protocol::codec::bincode_config().deserialize(data)?;
    cursor += consumed;
    if matches!(dispatch_server_msg(&msg, &mut stdout)?, DispatchResult::Done) {
        return Ok(());
    }
}
if cursor > 0 {
    read_buf.drain(..cursor);
}
```

**Step 2: Fix server client_to_pty loop (session_bridge.rs:384-388)**

Replace:

```rust
while let Some((data, consumed)) = protocol::decode_frame(&read_buf)? {
    let msg: ClientMsg = bincode::deserialize(data)?;
    read_buf.drain(..consumed);
    // ... handle msg
}
```

with:

```rust
let mut cursor = 0;
while let Some((data, consumed)) = protocol::decode_frame(&read_buf[cursor..])? {
    let msg: ClientMsg = crate::protocol::codec::bincode_config().deserialize(data)?;
    cursor += consumed;
    // ... handle msg
}
if cursor > 0 {
    read_buf.drain(..cursor);
}
```

**Step 3: Run tests**

Run: `cargo test`

**Step 4: Commit**

```
fix: single drain per read cycle to avoid quadratic buffer shifts (Important #1)

Process all frames with a cursor offset, then drain once. Reduces
O(n*m) to O(n) per read cycle under burst conditions.
```

---

### Task 7: Chunk History message to stay under MAX_FRAME_SIZE

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

**Context:** `History(Vec<Vec<u8>>)` can exceed `MAX_FRAME_SIZE` for large scrollback, breaking reattachment.

**Step 1: Replace single History message with chunked sending**

In `src/server/session_bridge.rs`, replace lines 195-209:

```rust
let (hist_chunks, screen_msg) = {
    let screen = lock_mutex(&io_handles.screen, "screen")?;
    let hist = screen.get_history();
    let screen_msg = protocol::encode(&ServerMsg::ScreenUpdate(screen.render(true, &mut render_cache)))?;
    (hist, screen_msg)
};

// Send history in chunks that fit within MAX_FRAME_SIZE
if !hist_chunks.is_empty() {
    let mut chunk = Vec::new();
    let mut chunk_size = 0;
    let size_limit = protocol::codec::MAX_FRAME_SIZE / 2; // conservative limit

    for line in hist_chunks {
        let line_size = line.len() + 16; // bincode overhead estimate
        if chunk_size + line_size > size_limit && !chunk.is_empty() {
            let msg = protocol::encode(&ServerMsg::History(std::mem::take(&mut chunk)))?;
            writer.write_all(&msg).await?;
            chunk_size = 0;
        }
        chunk_size += line_size;
        chunk.push(line);
    }
    if !chunk.is_empty() {
        let msg = protocol::encode(&ServerMsg::History(chunk))?;
        writer.write_all(&msg).await?;
    }
}
writer.write_all(&screen_msg).await?;
```

**Step 2: Run tests**

Run: `cargo test`

**Step 3: Commit**

```
fix: chunk History messages to stay under MAX_FRAME_SIZE (Important #2)

Large scrollback buffers could produce History messages exceeding the
16 MiB frame limit, breaking reattachment. Now sends in chunks.
```

---

### Task 8: Clamp DL/IL loop iterations

**Files:**
- Modify: `src/screen/performer.rs:451-483`
- Test: `src/screen/mod.rs` tests

**Context:** `CSI 99999M` iterates 99999 times instead of clamping to scroll region size.

**Step 1: Write the test**

In `src/screen/mod.rs` tests:

```rust
#[test]
fn dl_large_count_clamped() {
    let mut screen = Screen::new(10, 5, 100);
    // Position cursor at row 1 (0-indexed), scroll region is full screen (0-4)
    screen.process(b"\x1b[2;1H"); // row 2, 1-indexed
    // DL with count larger than remaining rows
    screen.process(b"\x1b[99999M");
    // Should not panic and grid should have 5 rows
    assert_eq!(screen.grid.cells.len(), 5);
}
```

**Step 2: Run test (should pass but be slow without fix)**

Run: `cargo test dl_large_count_clamped`

**Step 3: Add clamping**

In `src/screen/performer.rs`, DL handler (line 453):

```rust
'M' => { // DL — Delete Lines (within scroll region)
    let blank = self.blank_cell();
    let n = p(0, 1) as usize;
    let y = self.grid.cursor_y as usize;
    let top = self.grid.scroll_top as usize;
    let bottom = self.grid.scroll_bottom as usize;
    if y >= top && y <= bottom {
        let n = n.min(bottom - y + 1); // clamp to region size
        self.grid.cursor_x = 0;
        self.grid.wrap_pending = false;
        for _ in 0..n {
            if y <= bottom && bottom < self.grid.cells.len() {
                self.grid.cells.remove(y);
                self.grid.cells.insert(bottom, vec![blank.clone(); self.grid.cols as usize]);
            }
        }
    }
}
```

Similarly for IL handler (line 469):

```rust
'L' => { // IL — Insert Lines (within scroll region)
    let blank = self.blank_cell();
    let n = p(0, 1) as usize;
    let y = self.grid.cursor_y as usize;
    let top = self.grid.scroll_top as usize;
    let bottom = self.grid.scroll_bottom as usize;
    if y >= top && y <= bottom {
        let n = n.min(bottom - y + 1); // clamp to region size
        self.grid.cursor_x = 0;
        self.grid.wrap_pending = false;
        for _ in 0..n {
            if y <= bottom && bottom < self.grid.cells.len() {
                self.grid.cells.remove(bottom);
                self.grid.cells.insert(y, vec![blank.clone(); self.grid.cols as usize]);
            }
        }
    }
}
```

Also clamp SU (scroll up, line 444) and SD (scroll down, line 445-449):

```rust
'S' => { let n = p(0, 1).min(self.grid.rows) as usize; for _ in 0..n { self.scroll_up(); } }
'T' => {
    if ps.len() <= 1 {
        let n = p(0, 1).min(self.grid.rows) as usize;
        for _ in 0..n { self.scroll_down(); }
    }
}
```

**Step 4: Run tests**

Run: `cargo test`

**Step 5: Commit**

```
fix: clamp DL/IL/SU/SD loop counts to scroll region size (Important #8)

Prevents unnecessary iterations when programs send large counts
like CSI 99999M.
```

---

### Task 9: Fix alt screen cursor save/restore per xterm spec

**Files:**
- Modify: `src/screen/performer.rs:527-564`
- Test: `src/screen/mod.rs` tests

**Context:** Modes 47 and 1047 should NOT save/restore cursor — only 1049 should.

**Step 1: Write the failing test**

In `src/screen/mod.rs` tests:

```rust
#[test]
fn alt_screen_mode_47_no_cursor_save() {
    let mut screen = Screen::new(10, 5, 100);
    // Move cursor to (3, 2)
    screen.process(b"\x1b[3;4H"); // row 3, col 4 (1-indexed)
    assert_eq!(screen.grid.cursor_y, 2);
    assert_eq!(screen.grid.cursor_x, 3);
    // Save cursor explicitly with ESC 7
    screen.process(b"\x1b7");
    // Enter alt screen with mode 47 (should NOT save cursor again)
    screen.process(b"\x1b[?47h");
    // Move cursor on alt screen
    screen.process(b"\x1b[1;1H");
    assert_eq!(screen.grid.cursor_x, 0);
    assert_eq!(screen.grid.cursor_y, 0);
    // Exit alt screen with mode 47 (should NOT restore cursor)
    screen.process(b"\x1b[?47l");
    // Cursor should remain at (0, 0) since mode 47 doesn't restore
    assert_eq!(screen.grid.cursor_x, 0);
    assert_eq!(screen.grid.cursor_y, 0);
    // But ESC 8 should still restore the original saved cursor
    screen.process(b"\x1b8");
    assert_eq!(screen.grid.cursor_x, 3);
    assert_eq!(screen.grid.cursor_y, 2);
}
```

**Step 2: Run test to verify it fails**

Run: `cargo test alt_screen_mode_47_no_cursor_save`

**Step 3: Split alt screen handling by mode**

In `src/screen/performer.rs`, replace the combined `Some(1049 | 1047 | 47)` arm (lines 527-564):

```rust
Some(1049) => {
    if enable {
        self.save_cursor();
        self.state.saved_grid = Some(self.grid.cells.clone());
        self.state.saved_modes = Some(self.grid.modes.clone());
        self.state.in_alt_screen = true;
        let blank = Cell::default();
        for row in self.grid.cells.iter_mut() {
            for cell in row.iter_mut() { *cell = blank.clone(); }
        }
        self.grid.cursor_x = 0;
        self.grid.cursor_y = 0;
        self.grid.scroll_top = 0;
        self.grid.scroll_bottom = self.grid.rows - 1;
        self.grid.wrap_pending = false;
    } else {
        self.state.in_alt_screen = false;
        if let Some(saved) = self.state.saved_grid.take() {
            self.grid.cells = saved;
            self.grid.cells.resize(
                self.grid.rows as usize,
                vec![Cell::default(); self.grid.cols as usize],
            );
            for row in &mut self.grid.cells {
                row.resize(self.grid.cols as usize, Cell::default());
            }
        }
        if let Some(modes) = self.state.saved_modes.take() {
            self.grid.modes = modes;
        }
        self.restore_cursor();
        self.grid.scroll_top = 0;
        self.grid.scroll_bottom = self.grid.rows - 1;
    }
}
Some(1047 | 47) => {
    if enable {
        // Modes 47/1047: enter alt screen WITHOUT saving cursor
        self.state.saved_grid = Some(self.grid.cells.clone());
        self.state.saved_modes = Some(self.grid.modes.clone());
        self.state.in_alt_screen = true;
        let blank = Cell::default();
        for row in self.grid.cells.iter_mut() {
            for cell in row.iter_mut() { *cell = blank.clone(); }
        }
        self.grid.cursor_x = 0;
        self.grid.cursor_y = 0;
        self.grid.scroll_top = 0;
        self.grid.scroll_bottom = self.grid.rows - 1;
        self.grid.wrap_pending = false;
    } else {
        // Modes 47/1047: exit alt screen WITHOUT restoring cursor
        self.state.in_alt_screen = false;
        if let Some(saved) = self.state.saved_grid.take() {
            self.grid.cells = saved;
            self.grid.cells.resize(
                self.grid.rows as usize,
                vec![Cell::default(); self.grid.cols as usize],
            );
            for row in &mut self.grid.cells {
                row.resize(self.grid.cols as usize, Cell::default());
            }
        }
        if let Some(modes) = self.state.saved_modes.take() {
            self.grid.modes = modes;
        }
        self.grid.scroll_top = 0;
        self.grid.scroll_bottom = self.grid.rows - 1;
    }
}
```

**Step 4: Run tests**

Run: `cargo test`

**Step 5: Commit**

```
fix: only save/restore cursor for mode 1049, not 47/1047 (Important #7)

Per xterm spec, modes 47 and 1047 do not save/restore cursor position.
Only mode 1049 (= 1047 + 1048) includes cursor save/restore.
```

---

### Task 10: Fix CLI semantics — enforce New/Attach/Open contracts

**Files:**
- Modify: `src/protocol/messages.rs`
- Modify: `src/client/mod.rs`
- Modify: `src/cli.rs`
- Modify: `src/main.rs`
- Modify: `src/session.rs`
- Modify: `src/server/client_handler.rs`
- Modify: `src/server/session_bridge.rs`

**Context:** `Open`, `New`, and `Attach` all call the same function with identical semantics. `new` should fail if session exists, `attach` should fail if session doesn't exist.

**Step 1: Add ConnectMode to messages**

In `src/protocol/messages.rs`:

```rust
/// Connection mode for Connect message.
#[derive(Serialize, Deserialize, Debug, Clone)]
pub enum ConnectMode {
    /// Create or attach (Open subcommand)
    CreateOrAttach,
    /// Create only, fail if exists (New subcommand)
    CreateOnly,
    /// Attach only, fail if doesn't exist (Attach subcommand)
    AttachOnly,
}
```

Add `mode` field to `ClientMsg::Connect`:

```rust
Connect { name: String, history: usize, cols: u16, rows: u16, mode: ConnectMode },
```

**Step 2: Add `get`, `create`, and route by mode in SessionManager**

In `src/session.rs`, add a `get` method:

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

**Step 3: Update `handle_session` to accept and enforce mode**

In `src/server/session_bridge.rs`, add `mode` parameter and enforce:

```rust
pub async fn handle_session(
    mut stream: tokio::net::UnixStream,
    manager: Arc<Mutex<SessionManager>>,
    name: String,
    history: usize,
    cols: u16,
    rows: u16,
    leftover: Vec<u8>,
    mode: crate::protocol::ConnectMode,
) -> anyhow::Result<()> {
```

Inside, replace `get_or_create` with mode-specific logic:

```rust
use crate::protocol::ConnectMode;
let (session, is_new) = match mode {
    ConnectMode::CreateOrAttach => mgr.get_or_create(&name, cols, rows, history)?,
    ConnectMode::CreateOnly => {
        if mgr.get(&name).is_some() {
            let resp = protocol::encode(&ServerMsg::Error(format!("session '{}' already exists", name)))?;
            stream.write_all(&resp).await?;
            return Ok(());
        }
        mgr.create(name.clone(), cols, rows, history)?;
        (mgr.get(&name).unwrap(), true)
    }
    ConnectMode::AttachOnly => {
        match mgr.get(&name) {
            Some(s) => (s, false),
            None => {
                let resp = protocol::encode(&ServerMsg::Error(format!("session '{}' not found", name)))?;
                stream.write_all(&resp).await?;
                return Ok(());
            }
        }
    }
};
```

**Step 4: Update client_handler.rs to pass mode through**

In `src/server/client_handler.rs`:

```rust
ClientMsg::Connect { name, history, cols, rows, mode } => {
    match handle_session(stream, manager, name, history, cols, rows, read_buf, mode).await {
```

**Step 5: Update client connect() to accept mode**

In `src/client/mod.rs`:

```rust
pub async fn connect(name: &str, history: usize, mode: crate::protocol::ConnectMode) -> anyhow::Result<()> {
    // ...
    let msg = protocol::encode(&ClientMsg::Connect {
        name: name.to_string(),
        history,
        cols,
        rows,
        mode,
    })?;
```

**Step 6: Update main.rs to pass correct mode**

```rust
Command::Open { name, history } => {
    rt.block_on(client::connect(&name, history, crate::protocol::ConnectMode::CreateOrAttach))?;
}
Command::New { name, history } => {
    let name = name.unwrap_or_else(generate_name);
    rt.block_on(client::connect(&name, history, crate::protocol::ConnectMode::CreateOnly))?;
}
Command::Attach { name, history } => {
    rt.block_on(client::connect(&name, history, crate::protocol::ConnectMode::AttachOnly))?;
}
```

**Step 7: Remove `--history` from Attach (it's meaningless)**

In `src/cli.rs`, remove history from Attach and use a dummy default:

Actually, keep it for backward compatibility but mark it as ignored in the help text.

**Step 8: Run tests**

Run: `cargo test`

**Step 9: Commit**

```
fix: enforce New/Attach/Open CLI semantics with ConnectMode (Important #19)

New fails if session exists, Attach fails if session doesn't exist,
Open creates-or-attaches. Previously all three were identical.
```

---

### Task 11: Fix generate_name to avoid PID-based collisions

**Files:**
- Modify: `src/main.rs:58-60`

**Context:** PIDs are recycled by the OS, causing potential session name collisions.

**Step 1: Replace PID-based name with timestamp-based**

```rust
fn generate_name() -> String {
    let ts = std::time::SystemTime::now()
        .duration_since(std::time::UNIX_EPOCH)
        .unwrap_or_default()
        .as_millis();
    format!("s{}", ts % 1_000_000_000)
}
```

**Step 2: Run tests**

Run: `cargo test`

**Step 3: Commit**

```
fix: use timestamp instead of PID for auto-generated session names (Important #20)

PIDs are recycled by the OS, which could cause session name collisions.
```

---

### Task 12: Fix socket_dir test to handle XDG_RUNTIME_DIR

**Files:**
- Modify: `src/server/socket.rs:62-67`

**Context:** Test hardcodes `/tmp/retach-{uid}` fallback path but fails on systems with `XDG_RUNTIME_DIR`.

**Step 1: Fix the test**

Replace the test:

```rust
#[test]
fn socket_dir_returns_correct_format() {
    let uid = nix::unistd::getuid();
    let dir = socket_dir();
    if let Ok(xdg) = std::env::var("XDG_RUNTIME_DIR") {
        let expected = std::path::PathBuf::from(xdg).join("retach");
        assert_eq!(dir, expected);
    } else {
        let expected = std::path::PathBuf::from(format!("/tmp/retach-{}", uid));
        assert_eq!(dir, expected);
    }
}
```

**Step 2: Run tests**

Run: `cargo test socket`

**Step 3: Commit**

```
fix: socket_dir test handles XDG_RUNTIME_DIR correctly (Important #18)
```

---

### Task 13: Use `render_line` with `to_sgr_with_reset` for efficient scrollback

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

**Context:** `render_line` emits separate `\x1b[0m` reset + `\x1b[...m` set for every style change, while `render_screen` uses the more efficient combined `to_sgr_with_reset()`.

**Step 1: Update render_line to use to_sgr_with_reset**

Replace lines 58-64:

```rust
if cell.style != current {
    out.extend_from_slice(&cell.style.to_sgr_with_reset());
    current = cell.style.clone();
}
```

**Step 2: Update the test that counts resets**

The test `render_line_multiple_style_changes` counts `\x1b[0m` sequences. With `to_sgr_with_reset`, the reset is embedded in the SGR (e.g., `\x1b[0;31m`), so the count changes. Update:

```rust
// Combined reset+set reduces separate reset sequences
// Each style change now uses to_sgr_with_reset (e.g., \x1b[0;31m)
let combined_sgr_count = text.matches("\x1b[0;").count();
assert!(combined_sgr_count >= 2,
    "expected at least 2 combined reset+set SGR sequences, got {}", combined_sgr_count);
```

**Step 3: Run tests**

Run: `cargo test render`

**Step 4: Commit**

```
fix: use to_sgr_with_reset in render_line for efficient scrollback (Important #9)

Reduces bytes per scrollback line by combining reset+set into one
escape sequence, matching render_screen's approach.
```

---

### Task 14: Final verification

**Step 1: Run full test suite**

Run: `cargo test`

**Step 2: Run clippy**

Run: `cargo clippy -- -D warnings`

**Step 3: Build release**

Run: `cargo build --release`

**Step 4: Final commit if any fixups needed**

---

## Summary of tasks

| Task | Issue | Severity | Module |
|------|-------|----------|--------|
| 1 | Terminal not restored on panic/signal | CRITICAL | client |
| 2 | Unbounded pending_scrollback | CRITICAL | screen |
| 3 | Bincode OOM vulnerability | CRITICAL | protocol |
| 4 | No graceful server shutdown | CRITICAL | server |
| 5 | Blocking stdin thread leak | CRITICAL | client |
| 6 | Quadratic buffer drain | IMPORTANT | protocol |
| 7 | History exceeds MAX_FRAME_SIZE | IMPORTANT | server |
| 8 | DL/IL/SU/SD unclamped loops | IMPORTANT | screen |
| 9 | Alt screen cursor save per mode | IMPORTANT | screen |
| 10 | CLI semantics enforcement | IMPORTANT | cli |
| 11 | PID-based name collisions | IMPORTANT | cli |
| 12 | socket_dir test XDG_RUNTIME_DIR | IMPORTANT | server |
| 13 | render_line efficiency | IMPORTANT | screen |
| 14 | Final verification | — | all |
