# Bug & Workaround Fixes from Code Review

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

**Goal:** Fix bugs, race conditions, and workarounds (костыли) identified during full-codebase review.

**Architecture:** Targeted fixes across client, server, screen, and protocol modules. No structural changes — each fix is surgical. Ordered by impact: correctness bugs first, then robustness, then cleanup.

**Tech Stack:** Rust, Tokio, VTE, bincode, Unix sockets

---

### Task 1: Fix `delete_lines`/`insert_lines` incorrectly resetting `cursor_x = 0`

Per ECMA-48 and xterm, DL (`CSI M`) and IL (`CSI L`) must NOT change horizontal cursor position. Current code sets `cursor_x = 0`, breaking vim, less, htop column rendering.

**Files:**
- Modify: `src/screen/performer.rs:347-383`

**Step 1: Write failing test**

Add to existing test module in `src/screen/mod.rs` (where integration tests live):

```rust
#[test]
fn delete_lines_preserves_cursor_x() {
    let mut screen = Screen::new(80, 24, 100);
    // Position cursor at column 10, row 5
    screen.process(b"\x1b[6;11H");  // CUP row=6, col=11 (1-indexed)
    assert_eq!(screen.grid.cursor_x, 10);
    assert_eq!(screen.grid.cursor_y, 5);
    // Delete 1 line
    screen.process(b"\x1b[M");
    // cursor_x must be preserved
    assert_eq!(screen.grid.cursor_x, 10, "DL must not change cursor_x");
    assert_eq!(screen.grid.cursor_y, 5, "DL must not change cursor_y");
}

#[test]
fn insert_lines_preserves_cursor_x() {
    let mut screen = Screen::new(80, 24, 100);
    screen.process(b"\x1b[6;11H");
    assert_eq!(screen.grid.cursor_x, 10);
    // Insert 1 line
    screen.process(b"\x1b[L");
    assert_eq!(screen.grid.cursor_x, 10, "IL must not change cursor_x");
    assert_eq!(screen.grid.cursor_y, 5, "IL must not change cursor_y");
}
```

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

Run: `cargo test delete_lines_preserves_cursor_x insert_lines_preserves_cursor_x -- --nocapture`
Expected: FAIL — `cursor_x` is 0, not 10.

**Step 3: Fix implementation**

In `src/screen/performer.rs`, remove `self.grid.cursor_x = 0;` from both `csi_delete_lines` (line 354) and `csi_insert_lines` (line 373). Keep `self.grid.wrap_pending = false;`.

**Step 4: Run tests, verify pass**

Run: `cargo test -- --nocapture`
Expected: All tests pass, including new ones.

**Step 5: Commit**

```
fix: DL/IL must preserve cursor_x per ECMA-48
```

---

### Task 2: Add `read_buf` overflow protection in protocol codec and relay loops

`read_buf` in `read_one_message`, `run_socket_to_stdout`, and `client_to_pty` can grow unbounded if a malformed frame header claims a large size but data never completes.

**Files:**
- Modify: `src/protocol/codec.rs:82-100`
- Modify: `src/client/mod.rs:149-168`
- Modify: `src/server/session_bridge.rs:475-516`

**Step 1: Write failing test**

Add to `src/protocol/codec.rs` tests:

```rust
#[tokio::test]
async fn read_one_message_rejects_overflow() {
    // Send a header claiming MAX_FRAME_SIZE bytes, then keep sending
    // data without completing the frame — read_buf must not grow past limit.
    let (mut write_half, mut read_half) = tokio::io::duplex(65536);
    use tokio::io::AsyncWriteExt;

    // Header: claims MAX_FRAME_SIZE bytes
    let len_bytes = (MAX_FRAME_SIZE as u32).to_be_bytes();
    write_half.write_all(&len_bytes).await.unwrap();
    // Send more than MAX_FRAME_SIZE + 4 bytes of junk
    let junk = vec![0u8; MAX_FRAME_SIZE + 1024];
    write_half.write_all(&junk).await.unwrap();
    drop(write_half);

    let result: Result<crate::protocol::messages::ClientMsg, _> =
        read_one_message(&mut read_half).await;
    // Should get an error (either FrameTooLarge or buffer overflow), not OOM
    assert!(result.is_err(), "should reject oversized buffer accumulation");
}
```

**Step 2: Run test, verify it hangs or OOMs (or passes with large alloc)**

Run: `cargo test read_one_message_rejects_overflow -- --nocapture`

**Step 3: Add overflow guard**

In `read_one_message` (`codec.rs`), after `read_buf.extend_from_slice(...)`:

```rust
if read_buf.len() > MAX_FRAME_SIZE + 4 {
    return Err(ProtocolError::FrameTooLarge {
        size: read_buf.len(),
        max: MAX_FRAME_SIZE,
    });
}
```

In `run_socket_to_stdout` (`client/mod.rs`), after `read_buf.extend_from_slice(...)` (line 155):

```rust
if read_buf.len() > protocol::codec::MAX_FRAME_SIZE + 4 {
    anyhow::bail!("protocol framing error: buffer overflow");
}
```

In `client_to_pty` (`session_bridge.rs`), after `read_buf.extend_from_slice(...)` (line 484):

```rust
if read_buf.len() > protocol::codec::MAX_FRAME_SIZE + 4 {
    anyhow::bail!("protocol framing error: buffer overflow");
}
```

**Step 4: Run all tests**

Run: `cargo test -- --nocapture`
Expected: All pass.

**Step 5: Commit**

```
fix: cap read_buf growth to prevent OOM from malformed frames
```

---

### Task 3: Fix Notify race condition on eviction

`tokio::sync::Notify` pre-signal in `Session::new` contaminates subsequent eviction waits. Two PTY reader threads can race for PTY data.

**Files:**
- Modify: `src/session.rs:17,26-28`
- Modify: `src/server/session_bridge.rs:103,161-166,297,334`

**Step 1: Replace `Notify` with `tokio::sync::Semaphore`**

In `session.rs`, change:
```rust
pub reader_exited: Arc<tokio::sync::Notify>,
```
to:
```rust
pub reader_exited: Arc<tokio::sync::Semaphore>,
```

In `Session::new`, replace:
```rust
let reader_exited = Arc::new(tokio::sync::Notify::new());
reader_exited.notify_one();
```
with:
```rust
// Start with 1 permit so first connection doesn't wait for a non-existent reader.
let reader_exited = Arc::new(tokio::sync::Semaphore::new(1));
```

**Step 2: Update eviction wait in `setup_session`**

In `session_bridge.rs` (lines 161-166), replace:
```rust
let reader_exited = session.reader_exited.clone();
let _ = tokio::time::timeout(
    std::time::Duration::from_millis(200),
    reader_exited.notified(),
).await;
```
with:
```rust
let reader_exited = session.reader_exited.clone();
match tokio::time::timeout(
    std::time::Duration::from_secs(2),
    reader_exited.acquire(),
).await {
    Ok(Ok(permit)) => permit.forget(), // consume the permit
    Ok(Err(_)) => warn!(session = %name, "reader_exited semaphore closed"),
    Err(_) => warn!(session = %name, "timed out waiting for previous PTY reader to exit"),
}
```

**Step 3: Update signal in `pty_to_client` reader thread**

In `session_bridge.rs` (line 334), replace:
```rust
reader_exited.notify_one();
```
with:
```rust
reader_exited.add_permits(1);
```

**Step 4: Run all tests**

Run: `cargo test -- --nocapture`
Expected: All pass.

**Step 5: Commit**

```
fix: replace Notify with Semaphore for reader eviction sync
```

---

### Task 4: Fix `Session::Drop` — use `try_lock()` instead of `lock()`

If `Session` is accidentally dropped in async context, `lock()` + `child.wait()` blocks a Tokio worker.

**Files:**
- Modify: `src/session.rs:46-53`

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

```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 2: Run all tests**

Run: `cargo test -- --nocapture`
Expected: All pass.

**Step 3: Commit**

```
fix: use try_lock in Session::Drop to avoid blocking Tokio workers
```

---

### Task 5: Fix double mutex lock on detach allowing message interleaving

When detach key is pressed, two separate lock acquisitions let SIGWINCH inject messages between Input and Detach.

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

**Step 1: Merge into single lock scope**

Replace lines 79-91:
```rust
if let Some(pos) = buf[..n].iter().position(|&b| b == 0x1c) {
    let mut w = sw.lock().await;
    if pos > 0 {
        if let Ok(msg) = protocol::encode(&ClientMsg::Input(buf[..pos].to_vec())) {
            w.write_all(&msg).await?;
        }
    }
    if let Ok(msg) = protocol::encode(&ClientMsg::Detach) {
        w.write_all(&msg).await?;
    }
    drop(w);
    break;
}
```

**Step 2: Run all tests**

Run: `cargo test -- --nocapture`
Expected: All pass.

**Step 3: Commit**

```
fix: single mutex lock for detach to prevent message interleaving
```

---

### Task 6: Flush stdout for Passthrough messages

OSC52 clipboard and other passthrough data gets stuck in BufWriter until next PTY output.

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

**Step 1: Add flush after Passthrough**

Replace:
```rust
ServerMsg::Passthrough(data) => {
    stdout.write_all(data)?;
}
```
with:
```rust
ServerMsg::Passthrough(data) => {
    stdout.write_all(data)?;
    stdout.flush()?;
}
```

**Step 2: Run all tests**

Run: `cargo test -- --nocapture`
Expected: All pass.

**Step 3: Commit**

```
fix: flush stdout after Passthrough to prevent OSC52 stalling
```

---

### Task 7: Fix `generate_name` collision window

Names repeat every ~11.5 days due to modulo.

**Files:**
- Modify: `src/main.rs:61-67`

**Step 1: Fix implementation**

Replace:
```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)
}
```
with:
```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)
}
```

**Step 2: Run all tests**

Run: `cargo test -- --nocapture`

**Step 3: Commit**

```
fix: remove modulo from generate_name to prevent collisions
```

---

### Task 8: Fix `render_line` blank line workaround

Replace `b" "` workaround with proper empty Vec for blank lines.

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

**Step 1: Update `render_line` to return empty vec**

Replace:
```rust
None => {
    // Entirely blank line — emit a single space so it's not empty
    return b" ".to_vec();
}
```
with:
```rust
None => {
    return Vec::new();
}
```

**Step 2: Update the test**

In `render_line_blank` test, change assertion:
```rust
assert!(result.is_empty(), "blank line should produce empty vec");
```

**Step 3: Run all tests to find any callers that break**

Run: `cargo test -- --nocapture`

If any test fails because it relies on `b" "`, fix the caller to handle empty lines correctly.

**Step 4: Commit**

```
fix: render_line returns empty vec for blank lines, remove workaround
```

---

### Task 9: Fix inconsistent error handling in SIGWINCH handler

Errors on `Resize` write cause `break`, but errors on `RefreshScreen` write are silently ignored.

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

**Step 1: Make both writes consistent**

Replace the SIGWINCH block (lines 240-257):
```rust
let sigwinch_handle = 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: Run all tests**

Run: `cargo test -- --nocapture`

**Step 3: Commit**

```
fix: consistent error handling for resize and refresh in SIGWINCH
```

---

### Task 10: Remove unused `--history` from `Attach` command

The `history` parameter on `Attach` is never used since AttachOnly never creates sessions.

**Files:**
- Modify: `src/cli.rs`
- Modify: `src/main.rs`

**Step 1: Remove from CLI**

In `src/cli.rs`, remove the `history` field from `Attach`:
```rust
Attach {
    /// Session name
    name: String,
},
```

**Step 2: Update main.rs**

Change the `Attach` arm to pass `0` (or any value — it's unused by AttachOnly):
```rust
Command::Attach { name } => {
    rt.block_on(client::connect(&name, 0, protocol::ConnectMode::AttachOnly))
}
```

**Step 3: Run all tests + cargo build**

Run: `cargo test -- --nocapture && cargo build`

**Step 4: Commit**

```
fix: remove unused --history flag from attach command
```

---

### Task 11: Warn on unknown server message types in dispatch

Silently ignoring unexpected messages (`_ => {}`) hides protocol bugs.

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

**Step 1: Replace catch-all with logging**

Replace:
```rust
_ => {}
```
with:
```rust
other => {
    tracing::debug!("ignoring unexpected server message: {:?}", std::mem::discriminant(other));
}
```

Note: `ServerMsg` derives `Debug`, so we can use `discriminant` to log the variant name without printing payload data.

**Step 2: Run all tests**

Run: `cargo test -- --nocapture`

**Step 3: Commit**

```
fix: log unexpected server messages instead of silently ignoring
```

---
