docs: design spec for P2 fixes

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-06-10 19:56:26 -04:00
parent c29d862774
commit ca0d028a87
1 changed files with 259 additions and 0 deletions
@@ -0,0 +1,259 @@
 # 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.