# Review Fixes Implementation Plan

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

**Goal:** Fix all issues found during full project code review — bug fixes, missing terminal sequences, DECOM origin mode, underline color.

**Architecture:** Incremental fixes grouped by phase. Each phase is independently testable and committable. No architectural changes — all fixes are within existing module boundaries.

**Tech Stack:** Rust, VTE terminal emulation, tokio async runtime.

---

### Task 1: Session::Drop — skip join when kill skipped

**Files:**
- Modify: `src/session.rs:278-309`

**Step 1: Fix the drop implementation**

In `Session::Drop`, when `try_lock()` fails and the child is not killed, detach the reader thread instead of joining it (which would block forever):

```rust
impl Drop for Session {
    fn drop(&mut self) {
        match self.pty.child_arc().try_lock() {
            Ok(mut child) => {
                if let Err(e) = child.kill() {
                    tracing::debug!(error = %e, session = %self.name, "child already exited before kill");
                }
                if let Err(e) = child.wait() {
                    tracing::debug!(error = %e, session = %self.name, "child already reaped before wait");
                }
            }
            Err(_) => {
                tracing::warn!(session = %self.name, "child mutex contended during drop, skipping kill/wait/join");
                // Detach the reader thread — joining would block forever
                // because the child was not killed and the reader is waiting
                // on PTY read. The thread will exit when the PTY fd closes.
                self.reader_handle.take();
                return;
            }
        }
        // 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: client already disconnected during drop");
            }
        }
        // Wait for the reader thread to exit (it will see EOF after child kill).
        if let Some(handle) = self.reader_handle.take() {
            if handle.join().is_err() {
                tracing::warn!(session = %self.name, "PTY reader thread panicked during join");
            }
        }
    }
}
```

Note: The `return` after detaching skips eviction and join — both are unnecessary when the child couldn't be killed (eviction happens elsewhere, and the reader thread is detached).

**Step 2: Run tests**

Run: `cargo test --bin retach 2>&1 | tail -5`
Expected: all tests pass, no regressions.

---

### Task 2: drop_blocking_with_timeout — handle JoinError

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

**Step 1: Fix the error handling**

Replace `if result.is_err()` with exhaustive match:

```rust
pub(super) 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;
    match result {
        Ok(Ok(())) => {}
        Ok(Err(join_err)) => warn!(%label, error = %join_err, "drop task panicked"),
        Err(_) => warn!(%label, "timed out dropping value on blocking thread"),
    }
}
```

**Step 2: Run tests**

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

---

### Task 3: FocusIn break scope fix

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

**Step 1: Add labeled loop and fix break**

Change `loop {` to `'stdin: loop {` and change `break;` inside `FocusIn` to `break 'stdin;`:

The outer loop at line 196 becomes:
```rust
    'stdin: loop {
```

The break inside FocusIn (line 237) becomes:
```rust
                    break 'stdin;
```

**Step 2: Run tests**

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

---

### Task 4: Clippy dead code — remove unused GridMutator methods

**Files:**
- Modify: `src/screen/grid_mutator.rs:74-80`

**Step 1: Remove unused methods from the trait**

Remove these three lines from the `GridMutator` trait:

```rust
    fn restore_scrollback(&mut self, count: usize);
    fn scrollback_len(&self) -> usize;
    fn set_pending_start(&mut self, val: usize);
```

These methods exist on `Grid` directly but are never called through the trait.

**Step 2: Remove the trait impl for those methods from Grid**

In `src/screen/grid.rs`, find the `impl GridMutator for Grid` block and remove the implementations of `restore_scrollback`, `scrollback_len`, and `set_pending_start` from that impl block. The inherent methods on `Grid` itself should remain.

**Step 3: Run clippy and tests**

Run: `cargo clippy --bin retach 2>&1 | grep warning`
Expected: no warnings (or fewer warnings).

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

**Step 4: Commit Phase 1**

```bash
git add src/session.rs src/server/mod.rs src/client/mod.rs src/screen/grid_mutator.rs src/screen/grid.rs
git commit -m "fix: Phase 1 bug fixes — Session::Drop, JoinError, FocusIn break, clippy"
```

---

### Task 5: NEL (ESC E — Next Line)

**Files:**
- Modify: `src/screen/performer.rs` (esc_dispatch method)
- Test: `src/screen/tests_screen.rs`

**Step 1: Write failing test**

Add to `src/screen/tests_screen.rs`:

```rust
#[test]
fn nel_next_line() {
    let mut screen = Screen::new(10, 3, 0);
    screen.process(b"AB\x1bECD");
    // ESC E = CR + LF: cursor moves to col 0 of next row
    assert_eq!(screen.grid.visible_row(0)[0].c, 'A');
    assert_eq!(screen.grid.visible_row(0)[1].c, 'B');
    assert_eq!(screen.grid.visible_row(1)[0].c, 'C');
    assert_eq!(screen.grid.visible_row(1)[1].c, 'D');
}

#[test]
fn nel_at_scroll_bottom_scrolls() {
    let mut screen = Screen::new(10, 3, 100);
    screen.process(b"Line1\r\nLine2\r\nLine3");
    // Cursor is at row 2 (scroll bottom), ESC E should scroll
    screen.process(b"\x1bE");
    assert_eq!(screen.grid.visible_row(0)[0].c, 'L'); // Line2
    assert_eq!(screen.grid.visible_row(2)[0].c, ' '); // blank new line
}
```

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

Run: `cargo test --bin retach nel_ 2>&1 | tail -10`
Expected: FAIL — ESC E not handled, 'C'/'D' not where expected.

**Step 3: Implement NEL in esc_dispatch**

In `src/screen/performer.rs`, inside `esc_dispatch`, add after the IND handler (after line 637):

```rust
            ([], b'E') => { // NEL — Next Line (CR + LF)
                self.grid.set_cursor_x_unclamped(0);
                self.grid.set_wrap_pending(false);
                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 4: Run test to verify it passes**

Run: `cargo test --bin retach nel_ 2>&1 | tail -10`
Expected: PASS.

---

### Task 6: DECALN (ESC # 8 — Screen Alignment)

**Files:**
- Modify: `src/screen/performer.rs` (esc_dispatch method)
- Test: `src/screen/tests_screen.rs`

**Step 1: Write failing test**

Add to `src/screen/tests_screen.rs`:

```rust
#[test]
fn decaln_fills_screen_with_e() {
    let mut screen = Screen::new(5, 3, 0);
    screen.process(b"Hello\r\nWorld");
    screen.process(b"\x1b#8");
    for y in 0..3 {
        for x in 0..5 {
            assert_eq!(screen.grid.visible_row(y)[x].c, 'E',
                "expected 'E' at ({}, {})", x, y);
        }
    }
    // Cursor should be at home position
    assert_eq!(screen.grid.cursor_pos(), (0, 0));
}
```

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

Run: `cargo test --bin retach decaln_ 2>&1 | tail -10`
Expected: FAIL.

**Step 3: Implement DECALN in esc_dispatch**

Add to `esc_dispatch` in `src/screen/performer.rs`:

```rust
            ([b'#'], b'8') => { // DECALN — Screen Alignment Pattern
                self.grid.reset_scroll_region();
                self.grid.set_cursor_x_unclamped(0);
                self.grid.set_cursor_y_unclamped(0);
                self.grid.set_wrap_pending(false);
                let sid = self.intern_with_gc(Style::default());
                let e_cell = Cell::new('E', sid, 1);
                for y in 0..self.grid.rows() as usize {
                    for x in 0..self.grid.cols() as usize {
                        self.grid.set_cell(x, y, e_cell.clone());
                    }
                }
            }
```

**Step 4: Run test**

Run: `cargo test --bin retach decaln_ 2>&1 | tail -10`
Expected: PASS.

---

### Task 7: REP double charset mapping fix

**Files:**
- Modify: `src/screen/performer.rs:476-513` (print method) and line 581 (REP handler)
- Test: `src/screen/tests_screen.rs`

**Step 1: Write failing test**

Add to `src/screen/tests_screen.rs`:

```rust
#[test]
fn rep_with_line_drawing_charset() {
    let mut screen = Screen::new(10, 1, 0);
    // Enter line drawing mode: ESC ( 0
    // 'q' in line drawing = '─' (horizontal line)
    // CSI 3 b = repeat last char 3 times
    screen.process(b"\x1b(0q\x1b[3b");
    // Should have 4 horizontal lines total (1 original + 3 repeated)
    for x in 0..4 {
        assert_eq!(screen.grid.visible_row(0)[x].c, '─',
            "expected '─' at col {}, got '{}'", x, screen.grid.visible_row(0)[x].c);
    }
}
```

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

Run: `cargo test --bin retach rep_with_line 2>&1 | tail -10`
Expected: FAIL — REP double-maps, the character may be wrong for certain mappings.

Note: This test may pass because `map_charset('─')` for line drawing returns `'─'` unchanged (it's not in the mapping table). The bug is latent but semantically wrong. Fix anyway for correctness.

**Step 3: Fix — store pre-map character**

In `src/screen/performer.rs`, in the `print` method, move the `last_printed_char` assignment BEFORE charset mapping:

Change the print method. Currently line 477 maps first, then line 513 saves `c`. Instead:

```rust
    fn print(&mut self, c: char) {
        self.state.last_printed_char = c;  // Save pre-map character for REP
        let c = self.map_charset(c);
```

And remove the `self.state.last_printed_char = c;` from line 513.

**Step 4: Run tests**

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

---

### Task 8: CSI t — Title Push/Pop

**Files:**
- Modify: `src/screen/mod.rs:48-59` (ScreenState)
- Modify: `src/screen/performer.rs:621` (CSI t handler)
- Test: `src/screen/tests_screen.rs`

**Step 1: Write failing test**

Add to `src/screen/tests_screen.rs`:

```rust
#[test]
fn title_push_pop() {
    let mut screen = Screen::new(10, 3, 0);
    // Set title to "First"
    screen.process(b"\x1b]2;First\x07");
    assert_eq!(screen.title(), "First");
    // Push title (CSI 22;0 t)
    screen.process(b"\x1b[22;0t");
    // Change title to "Second"
    screen.process(b"\x1b]2;Second\x07");
    assert_eq!(screen.title(), "Second");
    // Pop title (CSI 23;0 t)
    screen.process(b"\x1b[23;0t");
    assert_eq!(screen.title(), "First");
}

#[test]
fn title_pop_empty_stack_noop() {
    let mut screen = Screen::new(10, 3, 0);
    screen.process(b"\x1b]2;Title\x07");
    // Pop from empty stack — title unchanged
    screen.process(b"\x1b[23;0t");
    assert_eq!(screen.title(), "Title");
}
```

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

Run: `cargo test --bin retach title_push 2>&1 | tail -10`
Expected: FAIL — title not restored after pop.

**Step 3: Add title_stack to ScreenState**

In `src/screen/mod.rs`, add to `ScreenState`:

```rust
    pub(super) title_stack: Vec<String>,
```

**Step 4: Implement CSI t in performer**

In `src/screen/performer.rs`, replace the `'t' => {}` line with:

```rust
            't' => {
                let cmd = p(0, 0);
                let scope = p(1, 0);
                match cmd {
                    22 if scope == 0 || scope == 2 => {
                        // Push title
                        if self.state.title_stack.len() < 16 {
                            self.state.title_stack.push(self.state.title.clone());
                        }
                    }
                    23 if scope == 0 || scope == 2 => {
                        // Pop title
                        if let Some(title) = self.state.title_stack.pop() {
                            self.state.title = title;
                        }
                    }
                    _ => {}
                }
            }
```

**Step 5: Run tests**

Run: `cargo test --bin retach title_ 2>&1 | tail -10`
Expected: PASS.

**Step 6: Commit Phase 2**

```bash
git add src/screen/performer.rs src/screen/tests_screen.rs src/screen/mod.rs
git commit -m "feat: Phase 2 — NEL, DECALN, REP charset fix, title push/pop"
```

---

### Task 9: DECOM (Origin Mode) — full implementation

**Files:**
- Modify: `src/screen/grid.rs:64-94` (TerminalModes)
- Modify: `src/screen/performer.rs` (multiple methods)
- Modify: `src/screen/mod.rs:27-34` (SavedCursor)
- Modify: `src/screen/render.rs:105-196` (emit_mode/emit_mode_delta)
- Test: `src/screen/tests_screen.rs`

**Step 1: Write failing tests**

Add to `src/screen/tests_screen.rs`:

```rust
#[test]
fn decom_cursor_position_relative_to_scroll_region() {
    let mut screen = Screen::new(10, 10, 0);
    // Set scroll region rows 3-7 (1-indexed)
    screen.process(b"\x1b[3;7r");
    // Enable origin mode (?6h)
    screen.process(b"\x1b[?6h");
    // CUP 1;1 should go to row 3 (top of scroll region), col 1
    screen.process(b"\x1b[1;1H");
    assert_eq!(screen.grid.cursor_pos(), (0, 2)); // 0-indexed: col 0, row 2
}

#[test]
fn decom_cursor_clamped_to_scroll_region() {
    let mut screen = Screen::new(10, 10, 0);
    screen.process(b"\x1b[3;7r");
    screen.process(b"\x1b[?6h");
    // CUP 99;1 should clamp to bottom of scroll region (row 7 = index 6)
    screen.process(b"\x1b[99;1H");
    assert_eq!(screen.grid.cursor_pos(), (0, 6));
}

#[test]
fn decom_off_cursor_absolute() {
    let mut screen = Screen::new(10, 10, 0);
    screen.process(b"\x1b[3;7r");
    // Origin mode off (default)
    screen.process(b"\x1b[1;1H");
    assert_eq!(screen.grid.cursor_pos(), (0, 0)); // absolute row 0
}

#[test]
fn decom_set_scrolling_region_homes_to_origin() {
    let mut screen = Screen::new(10, 10, 0);
    screen.process(b"\x1b[?6h");
    screen.process(b"\x1b[5;5H"); // move cursor away
    screen.process(b"\x1b[3;7r"); // set scroll region — cursor goes to origin
    assert_eq!(screen.grid.cursor_pos(), (0, 2)); // top of scroll region
}

#[test]
fn decom_saved_and_restored() {
    let mut screen = Screen::new(10, 10, 0);
    screen.process(b"\x1b[3;7r");
    screen.process(b"\x1b[?6h");
    screen.process(b"\x1b[2;3H"); // cursor at origin-relative (2,3)
    screen.process(b"\x1b7"); // DECSC save
    screen.process(b"\x1b[?6l"); // turn off origin mode
    screen.process(b"\x1b[1;1H"); // absolute
    assert_eq!(screen.grid.cursor_pos(), (0, 0));
    screen.process(b"\x1b8"); // DECRC restore
    // Origin mode should be restored
    assert!(screen.grid.modes().origin_mode);
}
```

**Step 2: Run tests to verify failure**

Run: `cargo test --bin retach decom_ 2>&1 | tail -10`
Expected: FAIL — `origin_mode` field doesn't exist.

**Step 3: Add origin_mode to TerminalModes**

In `src/screen/grid.rs`, add to `TerminalModes`:

```rust
    pub(super) origin_mode: bool,       // ?6 DECOM
```

Add to `Default` impl:

```rust
            origin_mode: false,
```

**Step 4: Handle ?6 in csi_set_dec_private_mode**

In `src/screen/performer.rs`, in `csi_set_dec_private_mode`, add before `Some(7)`:

```rust
                Some(6) => {
                    self.grid.modes_mut().origin_mode = enable;
                    if enable {
                        // DECOM set: cursor moves to origin (top of scroll region)
                        self.grid.set_cursor_x_unclamped(0);
                        self.grid.set_cursor_y_unclamped(self.grid.scroll_top());
                        self.grid.set_wrap_pending(false);
                    }
                }
```

**Step 5: Origin-aware cursor positioning**

In `src/screen/performer.rs`, modify `csi_cursor_position`:

```rust
    fn csi_cursor_position(&mut self, row: u16, col: u16) {
        self.grid.set_wrap_pending(false);
        if self.grid.modes().origin_mode {
            let top = self.grid.scroll_top();
            let bottom = self.grid.scroll_bottom();
            self.grid.set_cursor_y_unclamped(
                (top + row.saturating_sub(1)).min(bottom)
            );
        } else {
            self.grid.set_cursor_y_unclamped(
                row.saturating_sub(1).min(self.grid.rows() - 1)
            );
        }
        self.grid.set_cursor_x_unclamped(
            col.saturating_sub(1).min(self.grid.cols() - 1)
        );
    }
```

Modify `csi_line_position_absolute`:

```rust
    fn csi_line_position_absolute(&mut self, row: u16) {
        self.grid.set_wrap_pending(false);
        if self.grid.modes().origin_mode {
            let top = self.grid.scroll_top();
            let bottom = self.grid.scroll_bottom();
            self.grid.set_cursor_y_unclamped(
                (top + row.saturating_sub(1)).min(bottom)
            );
        } else {
            self.grid.set_cursor_y_unclamped(
                row.saturating_sub(1).min(self.grid.rows() - 1)
            );
        }
    }
```

Modify `csi_set_scrolling_region` — cursor goes to origin:

```rust
    fn csi_set_scrolling_region(&mut self, top: u16, bottom: u16) {
        let top = top.saturating_sub(1);
        let bottom = bottom.saturating_sub(1).min(self.grid.rows() - 1);
        if top <= bottom {
            self.grid.set_scroll_region(top, bottom);
            self.grid.set_cursor_x_unclamped(0);
            if self.grid.modes().origin_mode {
                self.grid.set_cursor_y_unclamped(top);
            } else {
                self.grid.set_cursor_y_unclamped(0);
            }
            self.grid.set_wrap_pending(false);
        }
    }
```

**Step 6: Save/restore origin_mode in cursor save**

In `src/screen/mod.rs`, add to `SavedCursor`:

```rust
    pub(super) origin_mode: bool,
```

In `src/screen/performer.rs`, in `save_cursor`:

```rust
            origin_mode: self.grid.modes().origin_mode,
```

In `restore_cursor`, add after setting autowrap:

```rust
            let origin = saved.origin_mode;
            // ... (after existing lines)
            self.grid.modes_mut().origin_mode = origin;
```

**Step 7: Emit origin mode in render**

In `src/screen/render.rs`, in `emit_mode`, add after autowrap line:

```rust
    emit_dec_mode(out, 6, modes.origin_mode);
```

In `emit_mode_delta`, add after autowrap delta:

```rust
    if modes.origin_mode != prev.origin_mode {
        emit_dec_mode(out, 6, modes.origin_mode);
    }
```

**Step 8: Run all tests**

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

**Step 9: Commit Phase 3**

```bash
git add src/screen/grid.rs src/screen/performer.rs src/screen/mod.rs src/screen/render.rs src/screen/tests_screen.rs
git commit -m "feat: Phase 3 — DECOM origin mode (?6) full implementation"
```

---

### Task 10: Underline Color (SGR 58/59)

**Files:**
- Modify: `src/screen/style.rs:54-65` (Style struct)
- Modify: `src/screen/style.rs:92-247` (apply_sgr, write_sgr_to)
- Test: `src/screen/style.rs` (inline tests module)

**Step 1: Write failing test**

Add to the `#[cfg(test)] mod tests` block in `src/screen/style.rs`:

```rust
    #[test]
    fn underline_color_indexed() {
        let mut style = Style::default();
        // SGR 58;5;196 — set underline color to indexed 196
        style.apply_sgr(&[vec![58], vec![5], vec![196]]);
        assert_eq!(style.underline_color, Some(Color::Indexed(196)));
    }

    #[test]
    fn underline_color_rgb() {
        let mut style = Style::default();
        // SGR 58;2;255;128;0 — set underline color to RGB
        style.apply_sgr(&[vec![58], vec![2], vec![255], vec![128], vec![0]]);
        assert_eq!(style.underline_color, Some(Color::Rgb(255, 128, 0)));
    }

    #[test]
    fn underline_color_reset() {
        let mut style = Style::default();
        style.apply_sgr(&[vec![58], vec![5], vec![196]]);
        assert!(style.underline_color.is_some());
        // SGR 59 — reset underline color
        style.apply_sgr(&[vec![59]]);
        assert_eq!(style.underline_color, None);
    }

    #[test]
    fn underline_color_colon_separated() {
        let mut style = Style::default();
        // SGR 58:5:196 — colon-separated
        style.apply_sgr(&[vec![58, 5, 196]]);
        assert_eq!(style.underline_color, Some(Color::Indexed(196)));
    }

    #[test]
    fn underline_color_emitted_in_sgr() {
        let mut style = Style::default();
        style.underline_color = Some(Color::Indexed(196));
        let sgr = style.to_sgr();
        let s = String::from_utf8_lossy(&sgr);
        assert!(s.contains("58;5;196"), "SGR should contain underline color, got: {}", s);
    }
```

**Step 2: Run test to verify failure**

Run: `cargo test --bin retach underline_color 2>&1 | tail -10`
Expected: FAIL — `underline_color` field doesn't exist.

**Step 3: Add underline_color to Style**

In `src/screen/style.rs`, add to `Style` struct (after `bg`):

```rust
    pub underline_color: Option<Color>,
```

**Step 4: Parse SGR 58/59 in apply_sgr**

In `src/screen/style.rs`, in `apply_sgr`, add after the `49 =>` arm (around line 240):

```rust
                58 => {
                    if let Some(color) = parse_extended_color(params, &mut i) {
                        self.underline_color = Some(color);
                    }
                }
                59 => self.underline_color = None,
```

**Step 5: Emit underline color in write_sgr_to**

In `src/screen/style.rs`, in `write_sgr_to`, add after the existing `Self::write_color_to` lines for fg/bg (after line 125):

```rust
        // Underline color (SGR 58)
        if let Some(ref color) = self.underline_color {
            sep!(out, need_sep);
            match color {
                Color::Indexed(c) => {
                    out.extend_from_slice(b"58;5;");
                    write_u8(out, *c);
                }
                Color::Rgb(r, g, b) => {
                    out.extend_from_slice(b"58;2;");
                    write_u8(out, *r);
                    out.push(b';');
                    write_u8(out, *g);
                    out.push(b';');
                    write_u8(out, *b);
                }
            }
        }
```

**Step 6: Run tests**

Run: `cargo test --bin retach underline_color 2>&1 | tail -10`
Expected: PASS.

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

**Step 7: Commit Phase 4**

```bash
git add src/screen/style.rs
git commit -m "feat: Phase 4 — underline color (SGR 58/59)"
```

---

### Task 11: Fix MEMORY.md version

**Files:**
- Modify: `/Users/hexabyte/.claude/projects/-Users-hexabyte-Dev-Local-retach/memory/MEMORY.md`

**Step 1: Update version**

Change `Version: 0.6.3` to `Version: 0.7.3`.

No commit — this is a local Claude memory file, not tracked in git.

---

### Task 12: Final verification

**Step 1: Run full test suite**

Run: `cargo test --bin retach 2>&1 | tail -10`
Expected: all tests pass.

**Step 2: Run clippy**

Run: `cargo clippy --bin retach 2>&1 | grep warning`
Expected: no warnings (or significantly fewer).

**Step 3: Run a quick smoke test**

Run: `cargo build --release 2>&1 | tail -3`
Expected: builds successfully.
