# P2 Fixes Design

**Date:** 2026-06-10

**Goal:** Resolve the three P2 findings in `TODO.md` — the Codex session-id
capture race, the absence of real-tmux smoke tests, and unvalidated project
paths — without touching unrelated behavior.

**Scope:** These three findings only. No other P2 hardening, no refactors beyond
what each fix strictly requires.

---

## Finding 1 — Codex session-id capture race

### Problem

`SessionService` captures a Codex rollout id after spawning by scanning
`~/.codex/sessions/rollout-*.jsonl` for the earliest rollout whose `cwd` matches
the project path and whose `started_at >= since` (the spawn timestamp). If a
second Codex session starts in the same project during the retry window
(`CAPTURE_MAX_ATTEMPTS=6` × `CAPTURE_RETRY_INTERVAL=0.5s` ≈ 3s), two rollouts
match and the code blindly picks the earliest — which may be the wrong
conversation. A later `codex resume <id>` then restores the wrong session.

### Approach: serialize hqt starts + refuse ambiguous matches

Two complementary changes. The lock removes the race between hqt-launched
sessions; the ambiguity guard makes a wrong capture impossible even when the
lock cannot help (a Codex started manually by the user in the same directory).

**1. Serialize the spawn→capture critical section.**

Add an `asyncio.Lock` to `SessionService`:

```python
def __init__(self, factory, tmux, harnesses):
    self.factory = factory
    self.tmux = tmux
    self.harnesses = harnesses
    # Serializes the spawn->capture window for harnesses that capture a
    # session id (codex), so two concurrent hqt starts in the same project
    # never overlap and confuse capture_session_id.
    self._capture_lock = asyncio.Lock()
```

Hold the lock around the `since = time.time()` → `tmux.spawn`/`respawn` →
`_capture_session_id_with_retry` region, but **only when**
`configurator.captures_session_id` is true (non-capturing harnesses need no
serialization). This applies in two places:

- `create_session` — the initial spawn + capture.
- `_respawn_with_fallback` rung 2 — the fresh-spawn + capture.

Holding the lock across the retry loop means it can be held for up to ~3s, so
rapid session creation against capturing harnesses queues. Session creation is
user-initiated and infrequent, so this is acceptable.

**2. Refuse ambiguous captures in `CodexConfigurator.capture_session_id`.**

Today the method sorts candidates and returns the earliest. Change it so that:

- **0 candidates** → return `None` (unchanged; retry loop tries again).
- **exactly 1 candidate** → return its id (the lock-protected normal case).
- **more than 1 candidate** → ambiguous; log a warning and return `None`
  rather than guess.

```python
candidates.sort(key=lambda t: t[0])
if len(candidates) > 1:
    log.warning(
        "capture_session_id: %d rollouts match cwd=%s since=%s; "
        "refusing to guess",
        len(candidates), project_path, since,
    )
    return None
return candidates[0][1] if candidates else None
```

`codex.py` gains a module-level `log = logging.getLogger(__name__)`.

This guard also hardens the poller path
(`SessionService._maybe_capture_missing_session_id`), which calls
`capture_session_id` without the lock: an ambiguous match there now keeps the
placeholder id instead of risking a wrong one.

### Outcome

The worst case becomes "keep the placeholder id" (a missing-but-safe capture),
never "store the wrong id." The placeholder path already exists and is logged.

### Tests (`tests/test_harnesses.py`, `tests/test_sessions.py`)

- `capture_session_id` with two matching rollouts in the same cwd/time window
  returns `None` and logs a warning (use `tmp_path` as a fake `~/.codex`,
  monkeypatching `Path.home`).
- `capture_session_id` with exactly one matching rollout returns its id
  (regression guard for the normal case).
- `SessionService` exposes `_capture_lock` as an `asyncio.Lock`; a unit test
  asserts `create_session` acquires it for a capturing harness. (Drive via the
  existing fake/mocked tmux + a stub configurator with
  `captures_session_id=True`; assert the lock is held during the spawn call,
  e.g. by checking `service._capture_lock.locked()` from inside the stub's
  `capture_session_id`.)

---

## Finding 2 — Real-tmux smoke tests

### Problem

tmux behavior is exercised almost entirely through mocked `_exec`. Theming, new
windows, respawn, and labels are never validated against a real tmux binary, so
a wrong flag or format string passes the suite.

### Approach: isolated real-tmux server via `TMUX_TMPDIR`, auto-skip

Add `tests/test_tmux_smoke.py`. No source change to `runner.py` is required:
tmux places its server socket in `$TMUX_TMPDIR`, so pointing that at a fresh
temp dir gives a throwaway server fully isolated from the user's real sessions.

**Gating.** Module-level skip when tmux is unavailable:

```python
import shutil
import pytest

pytestmark = [
    pytest.mark.tmux,
    pytest.mark.skipif(shutil.which("tmux") is None, reason="tmux not installed"),
]
```

**Isolation fixture.**

```python
@pytest.fixture
def tmux_runner(tmp_path, monkeypatch):
    monkeypatch.setenv("TMUX_TMPDIR", str(tmp_path))
    session = "hqt-test"
    subprocess.run(
        ["tmux", "new-session", "-d", "-s", session, "-x", "200", "-y", "50"],
        check=True,
        env={**os.environ, "TMUX_TMPDIR": str(tmp_path)},
    )
    runner = TmuxRunner(tmux_path="tmux", session_name=session)
    try:
        yield runner
    finally:
        subprocess.run(
            ["tmux", "kill-server"],
            env={**os.environ, "TMUX_TMPDIR": str(tmp_path)},
            check=False,
        )
```

`TmuxRunner._exec` uses `asyncio.create_subprocess_exec` without an explicit
`env`, so it inherits the monkeypatched `TMUX_TMPDIR` and targets the isolated
server automatically.

**Coverage (one test each, all `async`/`pytest.mark.asyncio`):**

- **new window:** `await runner.new_window("hqt-1", str(tmp_path), "sh")` returns
  a pane/window id; the window appears in `await runner.list_windows()`.
- **label round-trip:** `await runner.set_window_label("hqt-1", "•hqt-1")`, then
  read the `@hqt_label` user option back via a raw
  `tmux show-options -w -t hqt-test:=hqt-1` (or `runner` accessor if present)
  and assert it equals what was set.
- **respawn after death:** create a window running a command that exits, confirm
  the pane is dead (`await runner.is_pane_dead(...)` is true), then
  `await runner.respawn_pane(...)` with a long-lived command and assert the pane
  is alive again via `await runner.verify_window_alive(...)`. (Test the runner
  directly; `TmuxManager.respawn_verified` is out of scope here.)
- **theme:** `await runner.apply_theme()`, then assert a representative session
  option (e.g. `status-justify left` or `status` on) is set via raw
  `tmux show-options -t hqt-test`.

**Marker registration (`pyproject.toml`).** Register the `tmux` marker so pytest
emits no unknown-marker warning:

```toml
[tool.pytest.ini_options]
markers = [
    "tmux: real-tmux smoke tests; require a tmux binary and run on an isolated TMUX_TMPDIR server",
]
```

(If a `[tool.pytest.ini_options]` block does not yet exist, add it; otherwise
extend it.)

### Outcome

Real-tmux tests run by default wherever tmux exists (developer machines, CI with
tmux installed) and skip cleanly elsewhere. They never touch the user's live
tmux server because every invocation is scoped to the temp `TMUX_TMPDIR`.

---

## Finding 3 — Project path validation

### Problem

`ProjectService.create`/`update` store whatever path string they are given. A
nonexistent or non-directory path is accepted and only fails later when a
session spawned there dies, surfacing as a confusing dead session rather than an
early, clear rejection.

### Approach: validate and normalize at create/update

Add a private helper to `ProjectService`:

```python
from pathlib import Path

def _normalize_path(self, raw: str) -> str:
    """Expand ~, require an existing directory, return the resolved absolute path.

    Raises ServiceError if the path does not exist or is not a directory, so the
    TUI surfaces it as a notification instead of letting the bad path become a
    dead session later.
    """
    path = Path(raw).expanduser()
    if not path.is_dir():
        raise ServiceError(f"Path does not exist or is not a directory: {raw}")
    return str(path.resolve())
```

`create` and `update` call `path = self._normalize_path(path)` before
constructing/assigning the `Project`, so the stored path is always an absolute,
existing directory. The existing duplicate-path `IntegrityError` handling is
unchanged and runs after normalization (so duplicate detection compares resolved
paths consistently).

**No TUI change.** `action_add_project` (`app.py:201-205`) and
`action_edit_project` (`app.py:223-227`) already catch `ServiceError` and call
`self.notify(str(err), severity="error")`, so a rejected path shows as an error
notification with no further wiring.

### Tests (`tests/test_services.py`)

- `create` with a nonexistent path raises `ServiceError`.
- `create` with a path that is a file (not a directory) raises `ServiceError`.
- `create` with a valid directory stores the resolved absolute path
  (assert `project.path == str(tmp_path.resolve())`).
- `create` with a `~`-prefixed path expands it (monkeypatch `Path.home` or
  `HOME` to `tmp_path` and assert expansion).
- `update` with a nonexistent path raises `ServiceError` and leaves the row
  unchanged (re-read after `db.expire_all()`).

---

## Out of scope

- Any change to `src/hqt/tmux/runner.py` (it carries unrelated uncommitted WIP;
  the chosen designs deliberately avoid touching it).
- Broader Codex correctness work (e.g. matching on an injected marker, or
  isolating `CODEX_HOME` — rejected because a per-session `CODEX_HOME` would also
  isolate Codex's `config.toml`/auth and break the harness).
- Any new P3-level hardening or refactors.