From 0e7d8ff35b307aaab095592deac079945e696856 Mon Sep 17 00:00:00 2001 From: Felix Martin Date: Wed, 10 Jun 2026 19:56:26 -0400 Subject: [PATCH] docs: design spec for P2 fixes Co-Authored-By: Claude Fable 5 --- .../specs/2026-06-10-p2-fixes-design.md | 259 ++++++++++++++++++ 1 file changed, 259 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-10-p2-fixes-design.md diff --git a/docs/superpowers/specs/2026-06-10-p2-fixes-design.md b/docs/superpowers/specs/2026-06-10-p2-fixes-design.md new file mode 100644 index 0000000..c43194c --- /dev/null +++ b/docs/superpowers/specs/2026-06-10-p2-fixes-design.md @@ -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 ` 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.