diff --git a/CHANGELOG.md b/CHANGELOG.md index ff817c6f..694e96b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,6 +160,17 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Fixed +- **Attach, return-stamp and kill follow the run's live control window, not an older one (#482).** + `-` window names are not unique, so the lookup answered the first match: resuming a + parked run left `a` attaching to the dead `run-` window, the return pane stamped on it (so the + live window had no way back to the operator's origin pane), and `x` killing it while the live one + ran on. Each launch now records the window id it minted in the run dir and the lookup prefers it, + but only while the live listing still shows that id under this run id; with no record the answer is + unchanged. A resume whose window id was not captured warns instead of reporting plain success. + **Adapter authors:** the re-prove pairs `new_parked_window`'s id with the `window_id` column of + `list_windows`, which the seam previously left free to diverge — both bundled backends agree; one + that does not degrades to the ambiguous by-name resolve rather than mistargeting. + - **A native-Windows install driven from a WSL shell now says so (#332).** WSL appends the Windows `PATH` to its own, so a bash prompt can reach a Windows-installed `bmad-loop`: that interpreter reports `win32`, takes the psmux platform default, and never sees the distro's tmux — while diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 2ee41089..829c0979 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -101,7 +101,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Every run is a resumable on-disk state machine: `bmad-loop resume ` continues from a gate, escalation, or interruption. - A graceful stop (`stop --graceful` / TUI `S`) is resumable too: unlike a hard stop killed mid-item, it lets the in-flight item finish through commit and finalizes cleanly, ending as a `stopped` run that `resume` picks up at the next item. -- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `events/` (hook signals); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`. +- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `events/` (hook signals); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`; `ctl-window` (the control-session window id the last TUI launch minted, so attach/stop follow the live window, #482). - `journal.jsonl` records `session-end` for every session unconditionally — even a teardown that throws still lands one (status `aborted` when the outcome is unknowable). A timed-out session's entry carries `fired_at` (wall time the deadline was declared), `teardown_s` (wall seconds from that fire to this entry — the teardown gap), and `expired_clock` (`monotonic` / `wall` / `both` — `wall` alone fingerprints a host suspend that froze the monotonic clock). Every entry whose usage was read carries `tokens` (raw) and `tokens_weighted` (cache reads at `limits.cache_read_weight`), keeping per-session spend reconstructible; both are `null` when the usage read failed, and both are absent on an `aborted` end. `tokens_weighted` is the end-of-session total — distinct from a tripped session's `budget_weighted`, the guard's mid-session sample at trip time. ### Hook-based transport (no pane-scraping) diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 44e9e06c..2b207f60 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -136,7 +136,11 @@ the backend owns those conditions, and applies them uniformly, so the Both are replayed opaquely; neither is parsed by core. psmux applies the same qualification to `new_parked_window`, the `window_id` columns of `list_windows` and `current_window_id`; the latter two must agree, since the ctl-window prune -compares them to skip its own window.) tmux consumes the token natively (it coincides with tmux exact-match +compares them to skip its own window. To preserve unambiguous lookup, +`new_parked_window` must agree with the `list_windows` column too; a backend that +qualifies one side only remains usable but falls back to resolving parked +windows by name, which is ambiguous whenever several kinds share a run id +(#482).) tmux consumes the token natively (it coincides with tmux exact-match syntax), so `BaseTmuxBackend` passes it straight through. A native-id backend calls `parse_target()` first — `None` means "already a native id, use as-is", otherwise resolve `(session, window)` yourself; the herdr adapter's diff --git a/docs/tui-guide.md b/docs/tui-guide.md index 953a4716..d213ba2e 100644 --- a/docs/tui-guide.md +++ b/docs/tui-guide.md @@ -43,7 +43,10 @@ The TUI never runs an engine in-process. The two halves: lives in a separate `bmad-loop-` session; it is torn down when the run finishes (unless `[adapter] cleanup_session_on_finish = false`). These parked `bmad-loop-ctl` windows and any leftover `bmad-loop-` sessions can be - swept with `c` (see [Cleaning up sessions](#cleaning-up-sessions-c)). + swept with `c` (see [Cleaning up sessions](#cleaning-up-sessions-c)). Each + launch over an existing run records the id of the window it minted in the + run dir (`ctl-window`), so attach/stop follow the run's live window even + while an older same-run-id window is still parked (#482). - **Observer** — the dashboard reads only the artifacts the engine writes atomically into `.bmad-loop/runs//`: `state.json`, `journal.jsonl`, `logs/.log`, `ATTENTION`, `engine.pid`. It polls the selected run diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index fe8c0286..40016be2 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -158,7 +158,8 @@ def new_parked_window( """Create a window that runs ``argv`` then *parks* — waiting on a key so the exit status stays inspectable instead of the window closing the moment the process exits — and finally returns an attached client to its origin - (keyed by the per-window ``return_opt``). Returns the native window id.""" + (keyed by the per-window ``return_opt``). Returns the native window id; + for its required form see :meth:`list_window_ids`'s note on #482.""" @abstractmethod def list_window_ids(self, session: str) -> list[str]: @@ -172,10 +173,11 @@ def list_window_ids(self, session: str) -> list[str]: server per session), so a bare ``@N`` replayed as a ``-t`` target routes by the *caller's* server instead of the owning one. - :meth:`new_parked_window` is *outside* the rule — nothing - membership-tests a parked id, it is only replayed as a ``-t`` target by - the TUI — so a backend MAY mint it in a form this list never carries - (psmux happens to qualify it too, #291). + :meth:`new_parked_window` is outside *this* list's rule. To preserve + #482's unambiguous lookup, however, its id must match the ``window_id`` + column of :meth:`list_windows` (psmux qualifies both, #291). A backend + that diverges remains usable, but falls back to the ambiguous by-name + lookup whenever several kinds share a run id. Raises :class:`MultiplexerError` if the transport itself fails (timeout / missing binary): an empty list means "no windows" and must not be diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index a03cc5d6..e8ee910a 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -415,7 +415,7 @@ def action_attach(self) -> None: self.notify("no run selected", severity="warning") return session = runs.session_name(run_id) - win_id = launch.ctl_window_id(run_id) + win_id = launch.ctl_window_id(self.project, run_id) ok, agent_live = self._mux_guarded(lambda: launch.session_exists(session)) if not ok: return @@ -709,10 +709,19 @@ def _do_resume(self, run_id: str) -> None: self.notify(f"run {run_id} may still be live — stop it first", severity="warning") return try: - launch.resume_detached(self.project, run_id) + win_id = launch.resume_detached(self.project, run_id) except launch.LaunchError as e: self.notify(str(e), severity="error") return + if not win_id: + # The resume itself is running; only the disambiguation record is + # lost, so `a`/`x` may target an older same-run_id window (#482's + # symptom). Warn instead of masking it behind the success toast. + self.notify( + "resume launched but its window id was not captured — " + "attach/stop may target an older window for this run", + severity="warning", + ) self.notify(f"resume of {run_id} launched (control session {launch.CTL_SESSION})") def _do_replan(self, run_id: str, spec_path: Path) -> None: @@ -874,7 +883,7 @@ def done(ok: bool | None) -> None: def _stop_run_worker(self, run_id: str, run_dir: Path) -> None: try: runs.stop_run(run_dir) - launch.kill_ctl_window(run_id) + launch.kill_ctl_window(self.project, run_id) except (OSError, StopRunError, ProcessHostError) as e: self.call_from_thread(self.notify, f"stop failed: {e}", severity="error") return diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index 23312094..e4e77a56 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -21,6 +21,7 @@ from .. import runs from ..adapters.multiplexer import MultiplexerError, get_multiplexer, mux_usable from ..journal import Journal +from ..platform_util import atomic_write_text CTL_SESSION = "bmad-loop-ctl" @@ -43,7 +44,86 @@ def session_exists(session: str) -> bool: return get_multiplexer().has_session(session) -def ctl_window_id(run_id: str) -> str | None: +# Run-dir sidecar naming the ctl-session window start_detached minted last for +# this run. `-` is not unique across the four kinds, so the window +# listing alone cannot tell a live resume window from the parked run window it +# superseded — this file names the one we actually created. A hint, never a +# target on its own: ctl_window_id re-proves it against the live listing. +_CTL_WINDOW_FILE = "ctl-window" + + +def _read_ctl_window(project: Path, run_id: str) -> str | None: + """The window id recorded by the run's last launch, or None when there is + none / it cannot be read. Never raises, and that includes decoding: a torn + record can raise UnicodeDecodeError, a ValueError rather than an OSError, + which action_attach (no covering except at all) and _stop_run_worker (whose + except does not include it) would let escape. An unreadable hint is not an + error — it just leaves the caller with the name scan. + + The file is the only channel on purpose: `bmad-loop attach` resolves the same + run from its own process, and one resolve feeding every consumer is the + property ctl_window_id sells. A per-process memo of what this process last + minted would answer a different window than the CLI does.""" + record = runs.run_dir_for(project, run_id) / _CTL_WINDOW_FILE + try: + text = record.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return None + return text.strip() or None + + +def _forget_ctl_window(project: Path, run_id: str) -> None: + """Drop the record. A launch that cannot name the window it just minted must + not leave the *previous* launch's id authoritative — that id now names a + superseded window, and the honest answer is no record at all, which puts the + lookup back on the name scan. + + Ceiling: when the removal itself fails too, the superseded id survives on + disk. It still has to pass ctl_window_id's re-prove, so the worst it can + answer is a live window carrying this run's name — the pre-fix by-name + result, never a wilder target. + + A plain unlink, not retrying_unlink: launches run on the Textual event + loop, and dropping a best-effort hint is not worth ~5s of blocked win32 + backoff — the ceiling above already covers the miss.""" + try: + (runs.run_dir_for(project, run_id) / _CTL_WINDOW_FILE).unlink(missing_ok=True) + except OSError: + pass # a removal we cannot force — see the ceiling + + +def _record_ctl_window(project: Path, run_id: str, win_id: str) -> None: + """Record the window a launch just minted, so ctl_window_id can prefer it + over an older window sharing the run id. + + Best-effort on purpose. The window is already running by the time this + writes, so a failed write must not fail the launch — the lookup degrades to + the name scan, i.e. to the behaviour before this record existed. A failure + forgets the previous record rather than leaving it: degrading to the scan is + the intended fallback, answering a superseded window is not. + + Skipped when there is no run yet: a fresh `run`/`sweep` mints the only + window carrying its run id (nothing to disambiguate), and the run dir is + created by the detached child — this record deliberately never mkdirs one, + and must not be written into a run-dir-shaped directory (pruned, partial) + that runs.is_run reports as not a run. + + Atomic, not a bare write_text: the record is read cross-process (`bmad-loop + attach`), and on win32 an AV/indexer holding the previous record open fails + a plain overwrite with a transient sharing violation — which would swallow + into the forget path and quietly degrade the lookup. atomic_replace retries + exactly that violation, turning most real-world failures into successes. + """ + run_dir = runs.run_dir_for(project, run_id) + if not runs.is_run(run_dir): + return + try: + atomic_write_text(run_dir / _CTL_WINDOW_FILE, win_id) + except OSError: + _forget_ctl_window(project, run_id) + + +def ctl_window_id(project: Path, run_id: str) -> str | None: """Stable window id (bare `@N` on tmux, session-qualified on psmux) of the control-session window hosting this run's orchestrator process (start_detached names windows -), or None when the run was @@ -52,20 +132,38 @@ def ctl_window_id(run_id: str) -> str | None: An id, not a name, because every consumer replays the value as a select/kill/option target: one resolve feeds all of them, so a rename or a window minted between two verbs cannot send them to different windows, and - the value survives tmux's automatic-rename. It does NOT disambiguate the - run_id — `-` is not unique (a resume launched over a still- - parked run window shares it), and this scan takes the first match, the - same window a by-name lookup returned.""" + the value survives tmux's automatic-rename. + + `-` is not unique — a resume launched over a still-parked run + window shares the run id, and nothing reaps the parked one in between — so + the name scan alone answers whichever match the listing emits first (tmux + orders by window *index*, and it gives a new window the lowest free index, + so a superseded window usually but not always sorts ahead of the live one). + The id the run's last launch minted is recorded in the run dir and wins + whenever the listing still shows it under this run id. A record that is gone + (killed, pruned) or now carries another run's name is ignored rather than + replayed: a target that no longer resolves is the dangerous kind of stale — + on psmux an unresolvable `-t` lands on the *active* window (psmux/psmux#545; + tmux merely errors, which the best-effort consumers turn into a silent + no-op). With no record at all the answer is the first match, exactly as + before.""" if not mux_available(): return None + matches: list[str] = [] for win_id, name in get_multiplexer().list_windows(CTL_SESSION, ["window_id", "window_name"]): # win_id can be "": psmux's qualifier passes a falsy id through. An # empty id must never become a target — an empty `-t` resolves against # the *current* window. (The base's short-row padding cannot produce it # here: it fills TRAILING fields, and window_id is field 0 of 2.) if win_id and name.endswith(f"-{run_id}"): - return win_id - return None + matches.append(win_id) + if not matches: + return None + # Membership in `matches`, not mere presence in the listing: it re-proves the + # name too, so a backend that reuses a freed window id cannot hand back a + # different run's window. + recorded = _read_ctl_window(project, run_id) + return recorded if recorded in matches else matches[0] def ctl_target() -> str: @@ -230,7 +328,7 @@ def attach_plan(project: Path, run_id: str) -> tuple[list[str], str | None] | No live agent session. Returns (tmux argv, return_window) or None when there is nothing to attach to.""" session = runs.session_name(run_id) - win_id = ctl_window_id(run_id) + win_id = ctl_window_id(project, run_id) agent_live = session_exists(session) if win_id is not None and ( decision_pending(runs.run_dir_for(project, run_id)) or not agent_live @@ -242,10 +340,10 @@ def attach_plan(project: Path, run_id: str) -> tuple[list[str], str | None] | No return None -def kill_ctl_window(run_id: str) -> None: +def kill_ctl_window(project: Path, run_id: str) -> None: """Kill the control-session window hosting this run's orchestrator process, if any. A no-op when the run was not launched from the TUI or tmux is gone.""" - win_id = ctl_window_id(run_id) + win_id = ctl_window_id(project, run_id) if win_id is not None: get_multiplexer().kill_window(win_id) @@ -341,7 +439,9 @@ def start_detached(project: Path, argv_tail: list[str], run_id: str, kind: str) Returns the new window's stable backend id (bare `@N` on tmux, session-qualified on psmux) so callers can target it unambiguously (window - names collide when several kinds share a run_id). + names collide when several kinds share a run_id). The same id is recorded in + the run dir so ctl_window_id answers this window rather than an older one + under the same run id — see _record_ctl_window. """ mux = get_multiplexer() if not mux_usable(mux): @@ -364,9 +464,19 @@ def start_detached(project: Path, argv_tail: list[str], run_id: str, kind: str) except MultiplexerError as e: raise LaunchError(f"multiplexer new-window failed: {e}") from e if win_id: + # Record before tagging: a window minted but unrecorded puts the lookup + # back on the ambiguous scan, while an *untagged* window already has a + # documented fallback in _ctl_window_candidates — so even a + # non-conforming backend raising from the (contractually best-effort) + # set_window_option must not cost the record. + _record_ctl_window(project, run_id, win_id) # Tag the window with its project so a cleanup in another project never # closes it (the ctl session is shared across projects). mux.set_window_option(win_id, runs.PROJECT_OPTION, runs.project_tag(project)) + else: + # No id to record: the backend did not capture one. Whatever the previous + # launch recorded now names a superseded window, so drop it. + _forget_ctl_window(project, run_id) return win_id @@ -409,8 +519,12 @@ def start_sweep_detached( start_detached(project, tail, run_id, "sweep") -def resume_detached(project: Path, run_id: str) -> None: - start_detached(project, ["resume", "--project", str(project), run_id], run_id, "resume") +def resume_detached(project: Path, run_id: str) -> str | None: + """Resume in a ctl-session window; returns the window id (None when the + backend did not capture one — the caller should warn: resume is the launch + that mints a second window under the run id, so an uncaptured id degrades + the lookup to the ambiguous scan while the launch itself succeeded).""" + return start_detached(project, ["resume", "--project", str(project), run_id], run_id, "resume") def start_resolve_detached(project: Path, run_id: str) -> str | None: diff --git a/tests/test_cli.py b/tests/test_cli.py index ad6e6ee0..5b1ef12d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1501,10 +1501,12 @@ def test_attach_records_return_pane_inside_tmux(project, monkeypatch): from bmad_loop.tui import launch _make_run_with_decision(project, run_id="20260101-000000-aaaa") + planned: list = [] monkeypatch.setattr( launch, "attach_plan", - lambda proj, rid: ( + lambda proj, rid: planned.append((proj, rid)) + or ( ["tmux", "switch-client", "-t", "=bmad-loop-ctl"], "=bmad-loop-ctl:sweep-RID", ), @@ -1519,6 +1521,9 @@ def test_attach_records_return_pane_inside_tmux(project, monkeypatch): assert cli.main(["attach", "--project", str(project.project), "20260101-000000-aaaa"]) == 0 assert recorded == [("=bmad-loop-ctl:sweep-RID", "=main:%3")] assert called == [["tmux", "switch-client", "-t", "=bmad-loop-ctl"]] + # The *value* of the project argument, not just the arity: attach_plan finds + # the run's recorded ctl window only under the --project root (#482). + assert planned == [(project.project, "20260101-000000-aaaa")] def test_attach_records_detach_outside_tmux(project, monkeypatch): diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index d32f6480..c6f1d22f 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -1960,6 +1960,31 @@ async def test_resume_confirm_launches(project, monkeypatch): await until(pilot, lambda: calls == ["20260611-100000-aaaa"]) +async def test_resume_uncaptured_window_id_warns(project, monkeypatch): + # The resume itself is running; only the #482 disambiguation record is lost, + # so attach/stop may target an older same-run_id window. The success toast + # must not mask that (the resolve path already errors on this condition). + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "resume_detached", lambda proj, rid: None) + monkeypatch.setattr(data, "liveness", lambda run_dir: "dead") + make_run( + project.project, + "20260611-100000-aaaa", + paused_stage="DEV_VERIFY", + paused_reason="verify failed", + ) + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await until(pilot, lambda: dashboard(app).selected_run_id is not None) + await pilot.press("e") + await until(pilot, lambda: isinstance(app.screen, ConfirmResumeModal)) + await pilot.click(await ready(pilot, "#ok")) + await until( + pilot, lambda: any("window id was not captured" in m for m in notifications(app)) + ) + + async def test_resume_unknown_pid_warns(project, monkeypatch): monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(data, "liveness", lambda run_dir: "unknown") @@ -2082,7 +2107,7 @@ async def test_attach_without_mux_notifies(project, monkeypatch): async def test_attach_without_agent_session_notifies(project, monkeypatch): monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: False) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: None) make_run(project.project, "20260611-100000-aaaa") app = BmadLoopApp(project.project) async with app.run_test() as pilot: @@ -2099,7 +2124,7 @@ async def test_attach_multiplexer_error_notifies(project, monkeypatch): # TUI must surface the error as a toast, not crash the app. monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: True) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: None) def boom(_target): raise MultiplexerError("backend server not reachable") @@ -2123,7 +2148,7 @@ async def test_attach_session_probe_error_notifies(project, monkeypatch): # torn down in between). action_attach routes it through _mux_guarded, so the # TUI toasts the error and aborts the attach instead of crashing the app. monkeypatch.setattr(launch, "mux_available", lambda: True) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: None) def boom(_session): raise MultiplexerError("session probe unreachable") @@ -2214,7 +2239,7 @@ async def test_attach_targets_ctl_window_when_decision_pending(project, monkeypa selected: list[str] = [] monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: True) # agent up too - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: "@5") monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) calls, stamps = _patch_attach_exec(monkeypatch) app = BmadLoopApp(project.project) @@ -2229,6 +2254,42 @@ async def test_attach_targets_ctl_window_when_decision_pending(project, monkeypa assert stamps == [("@5", "=main:%9")] +@pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals +async def test_attach_uses_the_recorded_ctl_window(project, monkeypatch): + # The one attach test that does NOT replace ctl_window_id, so it pins the + # seam every other one stubs out: that the TUI hands it the same project root + # the launch recorded the window under (#482). Point app.py at anything else + # — the run dir, an unresolved path — and the record is unfindable, the scan + # answers the parked `run-` corpse, and attach + return-stamp both go there. + import subprocess as _subprocess + + from bmad_loop.adapters import tmux_base + + rid = "20260611-100000-aaaa" + run_dir = make_run(project.project, rid, run_type="sweep", alive=True) + Journal(run_dir).append("decision-pending", dw_id="DW-7", question="q?") + (run_dir / launch._CTL_WINDOW_FILE).write_text("@2", encoding="utf-8") + selected: list[str] = [] + + def fake(argv, **kwargs): + out = f"@1\trun-{rid}\n@2\tresume-{rid}\n" if argv[1] == "list-windows" else "" + return _subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + monkeypatch.setattr(launch, "session_exists", lambda session: True) + monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) + calls, stamps = _patch_attach_exec(monkeypatch) + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await until(pilot, lambda: dashboard(app).decision_pending is not None) + await pilot.press("a") + await until(pilot, lambda: bool(calls)) + assert selected == ["@2"] + assert stamps == [("@2", "=main:%9")] + + @pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals async def test_attach_outside_tmux_stamps_detach(project, monkeypatch): # No TMUX: a throwaway client attaches under suspend, so the ctl window is @@ -2240,7 +2301,7 @@ async def test_attach_outside_tmux_stamps_detach(project, monkeypatch): stamps: list[tuple[str, str]] = [] monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: True) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: "@5") monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: None) monkeypatch.setattr(launch, "set_return_pane", lambda w, p: stamps.append((w, p))) app = BmadLoopApp(project.project) @@ -2257,7 +2318,7 @@ async def test_attach_prefers_agent_session_without_decision(project, monkeypatc make_run(project.project, "20260611-100000-aaaa", alive=True) monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: True) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: "@5") calls, stamps = _patch_attach_exec(monkeypatch) app = BmadLoopApp(project.project) async with app.run_test() as pilot: @@ -2276,7 +2337,7 @@ async def test_attach_falls_back_to_ctl_window(project, monkeypatch): selected: list[str] = [] monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: False) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: "@5") monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) calls, stamps = _patch_attach_exec(monkeypatch) app = BmadLoopApp(project.project) @@ -2628,10 +2689,11 @@ async def test_story_checkpoint_stop_marks_stopped(project, monkeypatch): from bmad_loop import runs stops: list[Path] = [] + kills: list[tuple[Path, str]] = [] monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(data, "liveness", lambda run_dir: "dead") monkeypatch.setattr(runs, "stop_run", lambda rd: stops.append(rd) or True) - monkeypatch.setattr(launch, "kill_ctl_window", lambda rid: None) + monkeypatch.setattr(launch, "kill_ctl_window", lambda proj, rid: kills.append((proj, rid))) _stories_paused_run( project.project, stage="story-checkpoint", @@ -2643,7 +2705,9 @@ async def test_story_checkpoint_stop_marks_stopped(project, monkeypatch): async with app.run_test() as pilot: await _open_review(app, pilot, StoryCheckpointModal) await pilot.click(await ready(pilot, "#act-stop")) - await until(pilot, lambda: len(stops) == 1) + await until(pilot, lambda: len(kills) == 1) + assert stops == [project.project / runs.RUNS_DIR / "20260611-100000-aaaa"] + assert kills == [(project.project, "20260611-100000-aaaa")] def test_checkpoint_gate_line_pluralization(): diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index 48af8e22..d7482a36 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -17,8 +17,9 @@ import pytest +from bmad_loop import runs from bmad_loop.adapters import tmux_base -from bmad_loop.adapters.multiplexer import get_multiplexer +from bmad_loop.adapters.multiplexer import MultiplexerError, get_multiplexer from bmad_loop.tui import launch # Every test here asserts tmux-specific argv/behaviour through the multiplexer @@ -256,58 +257,135 @@ def test_session_exists(monkeypatch): assert fake.calls[0] == ["tmux", "has-session", "-t", "=bmad-loop-x"] -def test_ctl_window_id_matches_run_id_suffix(monkeypatch): - # The id, not the name: consumers replay the value as select/kill/option - # targets, where a by-name resolve can land on a duplicate. +def _ctl_listing(monkeypatch, rows: str) -> list[list[str]]: + """Script the ctl-session window listing; returns the recorded argv.""" + calls: list[list[str]] = [] + def fake(argv, **kwargs): - out = "@1\trun-AAAA\n@2\tsweep-RID\n@3\tresume-BBBB\n" if argv[1] == "list-windows" else "" + calls.append(list(argv)) + out = rows if argv[1] == "list-windows" else "" return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") monkeypatch.setattr(tmux_base.subprocess, "run", fake) monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") - assert launch.ctl_window_id("RID") == "@2" - assert launch.ctl_window_id("CCCC") is None + return calls + + +def _write_record(project: Path, run_id: str, win_id: str) -> Path: + """Stand in for a launch having minted `win_id` for this run.""" + run_dir = runs.run_dir_for(project, run_id) + run_dir.mkdir(parents=True, exist_ok=True) + record = run_dir / launch._CTL_WINDOW_FILE + record.write_text(win_id, encoding="utf-8") + return record + + +def test_ctl_window_id_matches_run_id_suffix(monkeypatch, tmp_path: Path): + # The id, not the name: consumers replay the value as select/kill/option + # targets, where a by-name resolve can land on a duplicate. With no record + # of what the run's last launch minted, the answer is the first match. + _ctl_listing(monkeypatch, "@1\trun-AAAA\n@2\tsweep-RID\n@3\tresume-BBBB\n") + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + assert launch.ctl_window_id(tmp_path, "CCCC") is None + + +def test_ctl_window_id_prefers_the_window_the_last_launch_minted(monkeypatch, tmp_path: Path): + # #482: `e` over a parked run leaves `run-RID` in front of the live + # `resume-RID`, and the scan alone answers the parked corpse. The recorded + # id names the window we actually created. + _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n") + _write_record(tmp_path, "RID", "@2") + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + + +def test_ctl_window_id_ignores_a_record_the_listing_no_longer_shows(monkeypatch, tmp_path: Path): + # The recorded window was killed (`x`) or pruned. Replaying a target that no + # longer resolves is the dangerous kind of stale — an unresolvable `-t` + # lands on the *active* window — so fall back to a window that exists. + _ctl_listing(monkeypatch, "@1\trun-RID\n") + _write_record(tmp_path, "RID", "@2") + assert launch.ctl_window_id(tmp_path, "RID") == "@1" + + +def test_ctl_window_id_ignores_a_record_that_now_names_another_run(monkeypatch, tmp_path: Path): + # A backend that reuses a freed window id must not let a stale record hand + # back a foreign run's window: the record is re-proved against the name too. + _ctl_listing(monkeypatch, "@2\trun-OTHER\n@5\tresume-RID\n") + _write_record(tmp_path, "RID", "@2") + assert launch.ctl_window_id(tmp_path, "RID") == "@5" + + +def test_ctl_window_id_none_when_no_window_carries_the_run_id(monkeypatch, tmp_path: Path): + # A record can never resurrect a run whose windows are all gone. + _ctl_listing(monkeypatch, "@1\trun-OTHER\n@3\tshell\n") + _write_record(tmp_path, "RID", "@1") + assert launch.ctl_window_id(tmp_path, "RID") is None + + +def test_ctl_window_id_unreadable_record_falls_back(monkeypatch, tmp_path: Path): + # An unreadable hint is not an error — it just leaves the name scan. + _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n") + run_dir = runs.run_dir_for(tmp_path, "RID") + (run_dir / launch._CTL_WINDOW_FILE).mkdir(parents=True) # a dir, not a file + assert launch.ctl_window_id(tmp_path, "RID") == "@1" -def test_ctl_window_id_skips_empty_id_rows(monkeypatch): +def test_ctl_window_id_invalid_utf8_record_falls_back(monkeypatch, tmp_path: Path): + _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n") + record = _write_record(tmp_path, "RID", "@2") + record.write_bytes(b"\xff") + assert launch.ctl_window_id(tmp_path, "RID") == "@1" + + +def test_ctl_window_id_skips_empty_id_rows(monkeypatch, tmp_path: Path): # An empty id must never be returned as a target — an empty `-t` resolves # against the current window. psmux's qualifier passes a falsy id through. - def fake(argv, **kwargs): - out = "\tsweep-RID\n@7\tsweep-RID\n" if argv[1] == "list-windows" else "" - return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") - - monkeypatch.setattr(tmux_base.subprocess, "run", fake) - monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") - assert launch.ctl_window_id("RID") == "@7" + _ctl_listing(monkeypatch, "\tsweep-RID\n@7\tsweep-RID\n") + assert launch.ctl_window_id(tmp_path, "RID") == "@7" -def test_kill_ctl_window_kills_by_resolved_id_not_a_name_token(monkeypatch): +def test_kill_ctl_window_kills_by_resolved_id_not_a_name_token(monkeypatch, tmp_path: Path): # The kill replays the id this listing resolved, never a `=session:name` - # token the backend would resolve again. Which of two same-named windows - # the scan picks is unchanged (first match, `@7`); what the id buys is that - # a rename or a new window between two verbs cannot re-point the second. - calls: list[list[str]] = [] + # token the backend would resolve again. With no record the scan picks the + # first match (`@7`); what the id buys is that a rename or a new window + # between two verbs cannot re-point the second. + calls = _ctl_listing(monkeypatch, "@2\trun-x\n@7\tsweep-RID\n@9\tsweep-RID\n") + launch.kill_ctl_window(tmp_path, "RID") + assert ["tmux", "kill-window", "-t", "@7"] in calls - def fake(argv, **kwargs): - calls.append(list(argv)) - out = "@2\trun-x\n@7\tsweep-RID\n@9\tsweep-RID\n" if argv[1] == "list-windows" else "" - return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") - monkeypatch.setattr(tmux_base.subprocess, "run", fake) - monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") - launch.kill_ctl_window("RID") - assert ["tmux", "kill-window", "-t", "@7"] in calls +def test_attach_plan_selects_and_returns_the_recorded_window(monkeypatch, tmp_path: Path): + # #482's first two consequences: the window the attach lands on, and the one + # its return_window stamps @bmad_return_pane on, are the same live window. + calls = _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n") + _write_record(tmp_path, "RID", "@2") + monkeypatch.setattr(launch, "session_exists", lambda s: False) + monkeypatch.setattr(launch, "decision_pending", lambda rd: False) + plan = launch.attach_plan(tmp_path, "RID") + assert plan is not None + _argv, return_window = plan + assert return_window == "@2" + assert ["tmux", "select-window", "-t", "@2"] in calls + +def test_kill_ctl_window_follows_the_record(monkeypatch, tmp_path: Path): + # #482's third consequence: `x` must not close the parked window and leave + # the live one running. + calls = _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n") + _write_record(tmp_path, "RID", "@2") + launch.kill_ctl_window(tmp_path, "RID") + assert ["tmux", "kill-window", "-t", "@2"] in calls -def test_ctl_window_id_no_session_or_tmux(monkeypatch): + +def test_ctl_window_id_no_session_or_tmux(monkeypatch, tmp_path: Path): def fake(argv, **kwargs): return subprocess.CompletedProcess(argv, 1, stdout="", stderr="no session") monkeypatch.setattr(tmux_base.subprocess, "run", fake) monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") - assert launch.ctl_window_id("RID") is None + assert launch.ctl_window_id(tmp_path, "RID") is None monkeypatch.setattr(tmux_base.shutil, "which", lambda name: None) - assert launch.ctl_window_id("RID") is None # no subprocess call attempted + assert launch.ctl_window_id(tmp_path, "RID") is None # no subprocess call attempted def test_set_return_pane_argv(fake_run): @@ -370,6 +448,119 @@ def test_start_detached_returns_window_id(fake_run, tmp_path: Path): assert launch.start_resolve_detached(tmp_path, "RID") == "@7" +def _make_run(project: Path, run_id: str = "RID") -> Path: + """A run dir runs.is_run accepts — the state a resume/resolve launches over.""" + run_dir = runs.run_dir_for(project, run_id) + run_dir.mkdir(parents=True, exist_ok=True) + (run_dir / "state.json").write_text("{}", encoding="utf-8") + return run_dir + + +def test_start_detached_records_the_window_it_minted(fake_run, tmp_path: Path): + run_dir = _make_run(tmp_path) + launch.resume_detached(tmp_path, "RID") + assert (run_dir / launch._CTL_WINDOW_FILE).read_text(encoding="utf-8") == "@7" + + +def test_start_detached_records_nothing_without_a_run(fake_run, tmp_path: Path): + # A fresh `run` mints the only window carrying its run id — nothing to + # disambiguate — and the record must never conjure a directory that + # runs.is_run would then report as not a run. The explicit skip keeps this + # expected case out of the OSError swallow; this test pins the outcome. + launch.start_run_detached(tmp_path, "RID") + assert not runs.run_dir_for(tmp_path, "RID").exists() + + +def test_no_record_into_a_dir_that_is_not_a_run(fake_run, tmp_path: Path): + # The case the is_run guard actually gates (the missing-dir sibling above is + # also covered by the OSError swallow — deleting the guard leaves it green): + # a run-dir-shaped directory without state.json (pruned, partial). Here the + # write would *succeed*, so only the guard keeps the sidecar out. + run_dir = runs.run_dir_for(tmp_path, "RID") + run_dir.mkdir(parents=True) + launch.resume_detached(tmp_path, "RID") + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +def test_start_detached_survives_an_unwritable_record(fake_run, tmp_path: Path, monkeypatch): + # The window is already running by the time the record is written, so a + # failed write degrades to the name scan rather than failing the launch. + from bmad_loop import platform_util + + run_dir = _make_run(tmp_path) + (run_dir / launch._CTL_WINDOW_FILE).mkdir() # a dir, not a file + # On win32 the replace-over-a-directory denial looks like the transient + # sharing violation atomic_replace retries; skip the ~5s backoff. + monkeypatch.setattr(platform_util, "_REPLACE_ATTEMPTS", 1) + assert launch.start_resolve_detached(tmp_path, "RID") == "@7" + + +def test_failed_record_forgets_the_previous_one(fake_run, tmp_path: Path, monkeypatch): + # A launch that cannot record the window it minted must not leave the + # *previous* launch's id authoritative — that id names a window this launch + # just superseded, so the honest state is no record at all. + run_dir = _make_run(tmp_path) + _write_record(tmp_path, "RID", "@2") + + def boom(*_a, **_k): + raise OSError("disk full") + + monkeypatch.setattr(launch, "atomic_write_text", boom) + launch.resume_detached(tmp_path, "RID") + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +def test_record_survives_a_raising_window_tag(fake_run, tmp_path: Path, monkeypatch): + # Record-before-tag ordering: the seam declares set_window_option + # best-effort, but a non-conforming backend raising from it must not cost + # the record — swap the two calls in start_detached and this fails. + run_dir = _make_run(tmp_path) + + def boom(self, *_a, **_k): + raise MultiplexerError("tag failed") + + monkeypatch.setattr(type(get_multiplexer()), "set_window_option", boom) + with pytest.raises(MultiplexerError): + launch.resume_detached(tmp_path, "RID") + assert (run_dir / launch._CTL_WINDOW_FILE).read_text(encoding="utf-8") == "@7" + + +def test_uncaptured_window_id_forgets_the_previous_record(monkeypatch, tmp_path: Path): + # new-window answered no id: nothing to record, and the stale record must go. + run_dir = _make_run(tmp_path) + _write_record(tmp_path, "RID", "@2") + + def fake(argv, **kwargs): + # rc 0 throughout, incl. has-session: the ctl session exists, and + # new-window succeeds but answers no id on stdout. + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + assert launch.start_resolve_detached(tmp_path, "RID") is None + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +def test_record_round_trips_a_session_qualified_id(monkeypatch, tmp_path: Path): + # The re-prove is a pure string match, so any qualified form works as long + # as the mint and the window_id column agree (multiplexer's symmetry note); + # `session:@N` is the shape psmux actually emits on both sides. + _ctl_listing( + monkeypatch, + "bmad-loop-ctl:@1\trun-RID\nbmad-loop-ctl:@2\tresume-RID\n", + ) + _write_record(tmp_path, "RID", "bmad-loop-ctl:@2") + assert launch.ctl_window_id(tmp_path, "RID") == "bmad-loop-ctl:@2" + + +def test_record_with_trailing_newline_still_matches(monkeypatch, tmp_path: Path): + # A newline-terminated record (hand-edited, foreign writer) must not fail + # the `recorded in matches` check and silently answer the parked corpse. + _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n") + _write_record(tmp_path, "RID", "@2\n") + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + + def test_prune_ctl_windows(monkeypatch, tmp_path: Path): from bmad_loop import runs @@ -630,7 +821,7 @@ def test_decision_pending_false_when_empty(tmp_path: Path): def test_attach_plan_prefers_ctl_when_decision_pending(monkeypatch): monkeypatch.delenv("TMUX", raising=False) - monkeypatch.setattr(launch, "ctl_window_id", lambda rid: "@2") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, rid: "@2") monkeypatch.setattr(launch, "session_exists", lambda s: True) monkeypatch.setattr(launch, "decision_pending", lambda rd: True) selected: list[str] = [] @@ -643,7 +834,7 @@ def test_attach_plan_prefers_ctl_when_decision_pending(monkeypatch): def test_attach_plan_prefers_ctl_when_no_agent_session(monkeypatch): monkeypatch.delenv("TMUX", raising=False) - monkeypatch.setattr(launch, "ctl_window_id", lambda rid: "@2") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, rid: "@2") monkeypatch.setattr(launch, "session_exists", lambda s: False) monkeypatch.setattr(launch, "decision_pending", lambda rd: False) monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: None) @@ -654,7 +845,7 @@ def test_attach_plan_prefers_ctl_when_no_agent_session(monkeypatch): def test_attach_plan_agent_session_when_no_decision(monkeypatch): monkeypatch.delenv("TMUX", raising=False) - monkeypatch.setattr(launch, "ctl_window_id", lambda rid: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, rid: None) monkeypatch.setattr(launch, "session_exists", lambda s: True) monkeypatch.setattr(launch, "decision_pending", lambda rd: False) assert launch.attach_plan(Path("/proj"), "RID") == ( @@ -664,7 +855,7 @@ def test_attach_plan_agent_session_when_no_decision(monkeypatch): def test_attach_plan_none_when_nothing_to_attach(monkeypatch): - monkeypatch.setattr(launch, "ctl_window_id", lambda rid: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, rid: None) monkeypatch.setattr(launch, "session_exists", lambda s: False) monkeypatch.setattr(launch, "decision_pending", lambda rd: False) assert launch.attach_plan(Path("/proj"), "RID") is None