docs: design spec for P2 fixes
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
Reference in New Issue
Block a user