From 9142677865c6a68a60e8343553ae3c77b16c40fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Sun, 9 Aug 2026 21:22:29 +0200 Subject: [PATCH 1/3] fix(tui): resolve a run's control window by identity, not listing order Control-session windows are named - over four kinds, so the name is not unique and ctl_window_id answered the first matching row. A resume launched over a still-parked run window therefore drove attach, the @bmad_return_pane stamp and the kill to the dead window while the live one kept running. Each launch now records the window id it minted in the run dir, and the lookup prefers that id only after re-proving it against the live listing: a record that was killed or pruned, or whose id now carries another run's name, is ignored rather than replayed as a target that no longer resolves (an unresolvable -t lands on the active window). A launch that cannot record its own window forgets the previous record instead of leaving a superseded id authoritative. With no record the answer is unchanged. 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 already agree, and one that does not degrades to the ambiguous by-name resolve rather than mistargeting. The seam docstring and the adapter authoring guide now say so. Closes #482 --- CHANGELOG.md | 14 ++ docs/adapter-authoring-guide.md | 6 +- src/bmad_loop/adapters/multiplexer.py | 9 +- src/bmad_loop/tui/app.py | 4 +- src/bmad_loop/tui/launch.py | 117 ++++++++++++-- tests/test_tui_app.py | 57 +++++-- tests/test_tui_launch.py | 219 ++++++++++++++++++++++---- 7 files changed, 365 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff817c6f..99b412bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,6 +160,20 @@ 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).** + Control-session windows are named `-` over four kinds, so the name is not unique and + the lookup returned the first match — on tmux the lowest-index one. Resuming a parked run (`e`) + minted `resume-` while the dead `run-` window still sat in front of it, so `a` attached to + the parked corpse, the return pane was stamped on it (leaving the live window with no way back to + the operator's origin pane), and `x` killed it while the live one kept running. Each launch now + records the window id it minted in the run dir, and the lookup prefers that id — but only after + re-proving it against the live listing: a record that was killed or pruned, or whose id now carries + another run's name, is ignored rather than replayed as a target that no longer resolves. Runs with + no record answer exactly as before. **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 already 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/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/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index fe8c0286..0f9160a7 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -172,10 +172,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..77db70d7 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 @@ -874,7 +874,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..e7c08350 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 retrying_unlink CTL_SESSION = "bmad-loop-ctl" @@ -43,7 +44,74 @@ 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 + or hand-edited record raises UnicodeDecodeError, a ValueError rather than an + OSError, and it would escape into action_attach and _stop_run_worker, whose + excepts do not cover it. 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.""" + try: + text = (runs.run_dir_for(project, run_id) / _CTL_WINDOW_FILE).read_text(encoding="utf-8") + except (OSError, ValueError): + 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.""" + try: + retrying_unlink(runs.run_dir_for(project, run_id) / _CTL_WINDOW_FILE) + except OSError: + pass # nothing recorded, or 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), the run dir is created + by the detached child, and a record must not conjure a directory that + runs.is_run then reports as not a run. + """ + run_dir = runs.run_dir_for(project, run_id) + if not runs.is_run(run_dir): + return + try: + (run_dir / _CTL_WINDOW_FILE).write_text(win_id, encoding="utf-8") + 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 +120,36 @@ 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, + because an unresolvable `-t` lands on the *active* window. 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 +314,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 +326,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 +425,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 +450,18 @@ 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: set_window_option sits outside the try above and + # can still raise, and 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. + _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 diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index d32f6480..e87b7b1f 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -2082,7 +2082,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 +2099,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 +2123,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 +2214,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 +2229,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 +2276,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 +2293,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 +2312,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 +2664,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 +2680,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..a769590d 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -17,6 +17,7 @@ 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.tui import launch @@ -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_skips_empty_id_rows(monkeypatch): + +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_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="") + _ctl_listing(monkeypatch, "\tsweep-RID\n@7\tsweep-RID\n") + assert launch.ctl_window_id(tmp_path, "RID") == "@7" - 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" - -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,79 @@ 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_start_detached_survives_an_unwritable_record(fake_run, tmp_path: Path): + # 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. + run_dir = _make_run(tmp_path) + (run_dir / launch._CTL_WINDOW_FILE).mkdir() # a dir, not a file + 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(Path, "write_text", boom) + launch.resume_detached(tmp_path, "RID") + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +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 if argv[1] == "has-session" else 0 + return subprocess.CompletedProcess(argv, rc, 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): + # psmux qualifies both the parked id it mints and the window_id column, so + # the re-prove is a string match on the qualified form (multiplexer's + # symmetry note). Pinned here because the two shapes only differ off tmux. + _ctl_listing( + monkeypatch, + "=bmad-loop-ctl:@1\trun-RID\n=bmad-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_prune_ctl_windows(monkeypatch, tmp_path: Path): from bmad_loop import runs @@ -630,7 +781,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 +794,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 +805,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 +815,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 From cc26fb84b65b972444b80a3ec393dace7237328f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Sun, 9 Aug 2026 22:23:52 +0200 Subject: [PATCH 2/3] fix(tui): harden the ctl-window record's failure paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the #482 fix: - Write the record atomically: it is read cross-process (bmad-loop attach), and on win32 an AV/indexer holding the previous record failed a plain overwrite into the forget path — atomic_replace retries exactly that violation, turning most real-world failures into successes, and a torn read can no longer degrade the lookup. - Forget the record with 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. - resume_detached returns its window id and the TUI warns when it was not captured: resume is the launch that mints a second window under the run id, so a silently lost record re-created the #482 symptoms behind a success toast (the resolve path already surfaced this). - Narrow _read_ctl_window's except to the UnicodeDecodeError its docstring documents, so an unrelated ValueError cannot hide as a degraded lookup. - Prose: the unresolvable -t hazard is psmux-specific (psmux/psmux#545; tmux merely errors), set_window_option is contractually best-effort, and new_parked_window's docstring now cross-references its required id form. - Docs: the run-dir inventories in FEATURES.md and the TUI guide list the ctl-window sidecar. - Tests: pin the is_run guard against a run-dir-shaped non-run (the missing-dir case alone also passes via the OSError swallow), record-before-tag ordering under a raising tag, trailing-newline records, the session:@N shape psmux actually emits, the resume warning, and the CLI attach project value. --- CHANGELOG.md | 10 +++-- docs/FEATURES.md | 2 +- docs/tui-guide.md | 5 ++- src/bmad_loop/adapters/multiplexer.py | 3 +- src/bmad_loop/tui/app.py | 11 ++++- src/bmad_loop/tui/launch.py | 65 +++++++++++++++++---------- tests/test_cli.py | 7 ++- tests/test_tui_app.py | 25 +++++++++++ tests/test_tui_launch.py | 57 +++++++++++++++++++---- 9 files changed, 144 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99b412bd..7a6693a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -169,10 +169,12 @@ whose seams had diverged enough that several ports needed a different fix, and t records the window id it minted in the run dir, and the lookup prefers that id — but only after re-proving it against the live listing: a record that was killed or pruned, or whose id now carries another run's name, is ignored rather than replayed as a target that no longer resolves. Runs with - no record answer exactly as before. **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 already agree; one that does not degrades to the ambiguous by-name resolve - rather than mistargeting. + no record answer exactly as before. The record is written atomically (a torn or overwrite-blocked + record would silently degrade the lookup), and a resume whose window id was not captured now warns + instead of hiding the degraded lookup behind the success toast. **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 already 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 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/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 0f9160a7..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]: diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index 77db70d7..e8ee910a 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -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: diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index e7c08350..e4e77a56 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -21,7 +21,7 @@ from .. import runs from ..adapters.multiplexer import MultiplexerError, get_multiplexer, mux_usable from ..journal import Journal -from ..platform_util import retrying_unlink +from ..platform_util import atomic_write_text CTL_SESSION = "bmad-loop-ctl" @@ -55,18 +55,19 @@ def session_exists(session: str) -> bool: 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 - or hand-edited record raises UnicodeDecodeError, a ValueError rather than an - OSError, and it would escape into action_attach and _stop_run_worker, whose - excepts do not cover it. An unreadable hint is not an error — it just leaves - the caller with the name scan. + 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 = (runs.run_dir_for(project, run_id) / _CTL_WINDOW_FILE).read_text(encoding="utf-8") - except (OSError, ValueError): + text = record.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): return None return text.strip() or None @@ -80,11 +81,15 @@ def _forget_ctl_window(project: Path, run_id: str) -> None: 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.""" + 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: - retrying_unlink(runs.run_dir_for(project, run_id) / _CTL_WINDOW_FILE) + (runs.run_dir_for(project, run_id) / _CTL_WINDOW_FILE).unlink(missing_ok=True) except OSError: - pass # nothing recorded, or a removal we cannot force — see the ceiling + pass # a removal we cannot force — see the ceiling def _record_ctl_window(project: Path, run_id: str, win_id: str) -> None: @@ -98,15 +103,22 @@ def _record_ctl_window(project: Path, run_id: str, win_id: str) -> None: 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), the run dir is created - by the detached child, and a record must not conjure a directory that - runs.is_run then reports as not a run. + 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: - (run_dir / _CTL_WINDOW_FILE).write_text(win_id, encoding="utf-8") + atomic_write_text(run_dir / _CTL_WINDOW_FILE, win_id) except OSError: _forget_ctl_window(project, run_id) @@ -130,9 +142,11 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: 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, - because an unresolvable `-t` lands on the *active* window. With no record at - all the answer is the first match, exactly as before.""" + 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] = [] @@ -450,10 +464,11 @@ 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: set_window_option sits outside the try above and - # can still raise, and 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. + # 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). @@ -504,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 e87b7b1f..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") diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index a769590d..f4fb7f7c 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -19,7 +19,7 @@ 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 @@ -471,11 +471,27 @@ def test_start_detached_records_nothing_without_a_run(fake_run, tmp_path: Path): assert not runs.run_dir_for(tmp_path, "RID").exists() -def test_start_detached_survives_an_unwritable_record(fake_run, tmp_path: Path): +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" @@ -489,11 +505,26 @@ def test_failed_record_forgets_the_previous_one(fake_run, tmp_path: Path, monkey def boom(*_a, **_k): raise OSError("disk full") - monkeypatch.setattr(Path, "write_text", boom) + 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) @@ -510,15 +541,23 @@ def fake(argv, **kwargs): def test_record_round_trips_a_session_qualified_id(monkeypatch, tmp_path: Path): - # psmux qualifies both the parked id it mints and the window_id column, so - # the re-prove is a string match on the qualified form (multiplexer's - # symmetry note). Pinned here because the two shapes only differ off tmux. + # 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\n=bmad-loop-ctl:@2\tresume-RID\n", + "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" + _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 251f77e471ddf26dcca65e374ab3c29e3af90d7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Sun, 9 Aug 2026 22:34:48 +0200 Subject: [PATCH 3/3] =?UTF-8?q?fix(tui):=20address=20review=20=E2=80=94=20?= =?UTF-8?q?trim=20the=20#482=20changelog=20entry,=20drop=20a=20useless=20c?= =?UTF-8?q?onditional?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 23 +++++++++-------------- tests/test_tui_launch.py | 5 +++-- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a6693a4..694e96b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -161,20 +161,15 @@ 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).** - Control-session windows are named `-` over four kinds, so the name is not unique and - the lookup returned the first match — on tmux the lowest-index one. Resuming a parked run (`e`) - minted `resume-` while the dead `run-` window still sat in front of it, so `a` attached to - the parked corpse, the return pane was stamped on it (leaving the live window with no way back to - the operator's origin pane), and `x` killed it while the live one kept running. Each launch now - records the window id it minted in the run dir, and the lookup prefers that id — but only after - re-proving it against the live listing: a record that was killed or pruned, or whose id now carries - another run's name, is ignored rather than replayed as a target that no longer resolves. Runs with - no record answer exactly as before. The record is written atomically (a torn or overwrite-blocked - record would silently degrade the lookup), and a resume whose window id was not captured now warns - instead of hiding the degraded lookup behind the success toast. **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 already agree; one that does not degrades - to the ambiguous by-name resolve rather than mistargeting. + `-` 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 diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index f4fb7f7c..d7482a36 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -531,8 +531,9 @@ def test_uncaptured_window_id_forgets_the_previous_record(monkeypatch, tmp_path: _write_record(tmp_path, "RID", "@2") def fake(argv, **kwargs): - rc = 0 if argv[1] == "has-session" else 0 - return subprocess.CompletedProcess(argv, rc, stdout="", stderr="") + # 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}")