diff --git a/CHANGELOG.md b/CHANGELOG.md index 567e7991..dacf71a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,19 @@ breaking changes may land in a minor release. child), except a share-root shape older PowerShell would corrupt. - Escalate an environment fault at the review-budget rescue gate instead of deferring the story as unconverged (DW-523). +- Reach only control-session windows carrying this project's tag from `a`/`x`, + so a reused window id or a forged `ctl-window` record can no longer steer + them onto a neighbour's window; the record now only breaks ties among tagged + windows, and a window whose tag write failed is left alone until relaunched + (#750). When a window under the run's name is left alone because its tag reads + empty (never written, or unreadable — psmux's option probe can fail), `x` + warns that the control window was not closed instead of reporting a clean + stop, and `a` and `bmad-loop attach` say why they cannot reach it. A control + listing that fails outright is reported as such instead of reading as "no + window" (by `x`, `a`, `bmad-loop attach` and the window prune) — `a` and + `bmad-loop attach` then still reach the run's agent session — `x` checks + that the window it killed is gone, and a TUI run or sweep launch warns when + its new window cannot be confirmed as this project's. - Explain that unpinned result-artifact scans search only the configured artifact directories themselves, so a nested story spec no longer produces an opaque `no-artifact` breadcrumb (#780). diff --git a/docs/tui-guide.md b/docs/tui-guide.md index 3f7f738b..0a65059f 100644 --- a/docs/tui-guide.md +++ b/docs/tui-guide.md @@ -88,7 +88,12 @@ The TUI never runs an engine in-process. The two halves: 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). + while an older same-run-id window is still parked (#482). Attach/stop only + ever reach a window carrying this project's tag: the record breaks ties + among tagged windows but never vouches for an untagged one, so a window + whose tag write failed is left alone until a relaunch tags one (#750). The + same holds when the tag cannot be read at all, and in both cases `x` warns + that the control window was not closed and `a` says why it cannot reach it. - **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/cli.py b/src/bmad_loop/cli.py index 6c4090ce..65507882 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -5452,9 +5452,27 @@ def cmd_attach(args: argparse.Namespace) -> int: if run_dir is None: print("no runs found", file=sys.stderr) return 1 - plan = launch.attach_plan(project, run_dir.name) + # A ctl listing that could not be read is said, then the attach carries on + # to the agent session rather than failing on a window it could not check. + ctl_faults: list[str] = [] + + def ctl_fault(msg: str) -> None: + ctl_faults.append(msg) + print(f"warning: could not check the run's control window: {msg}", file=sys.stderr) + + plan, unproven = launch.attach_plan(project, run_dir.name, on_fault=ctl_fault) + if unproven: + # A window under this run's name was refused for an empty tag (#750): + # say so before attaching elsewhere — or reporting nothing — so the + # refusal is not mistaken for an absence. Before the attach, which takes + # over the terminal. + notice = launch.unproven_ctl_window_notice(project, run_dir.name, unproven) + print(f"warning: {notice}", file=sys.stderr) if plan is None: - print(f"nothing to attach for run {run_dir.name}", file=sys.stderr) + if not ctl_faults: + # Not after a ctl fault: "nothing to attach" would claim the + # unchecked ctl window absent (#750). The warning above stands. + print(f"nothing to attach for run {run_dir.name}", file=sys.stderr) return 1 argv, return_window = plan # Record where to send the client once the sweep finishes this cycle's diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index c6cd018b..895574af 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -338,7 +338,7 @@ def _start_run_result(self, result: dict | None) -> None: def go() -> None: run_id = runs.new_run_id() try: - launch.start_run_detached( + win_id = launch.start_run_detached( self.project, run_id, spec=spec_folder or None, @@ -349,6 +349,8 @@ def go() -> None: except launch.LaunchError as e: self.notify(str(e), severity="error") return + if not win_id: + self._warn_unreachable_launch("run", run_id) self.notify( f"run {run_id} launched (control session {launch.ctl_session(self.project)})" ) @@ -356,6 +358,20 @@ def go() -> None: self._guarded(go) + def _warn_unreachable_launch(self, kind: str, run_id: str) -> None: + """The launch is running, but its ctl window could not be confirmed + reachable by the lookup `a`/`x` use — its best-effort tag write did not + land (#750), its id was not captured, or the listing could not be read. + Say so beside the success toast instead of letting it imply the + targeting is sound.""" + self.notify( + f"{kind} {run_id} launched, but its control window could not be confirmed " + "as this project's — attach/stop may not reach it; check " + f"{launch.ctl_session(self.project)} and close it by hand when done", + severity="warning", + timeout=15, + ) + def action_start_sweep(self) -> None: if self._mux_missing(): return @@ -374,7 +390,7 @@ def _start_sweep_result(self, result: dict | None) -> None: def go() -> None: run_id = runs.new_run_id() try: - launch.start_sweep_detached( + win_id = launch.start_sweep_detached( self.project, run_id, no_prompt=result["no_prompt"], @@ -384,6 +400,8 @@ def go() -> None: except launch.LaunchError as e: self.notify(str(e), severity="error") return + if not win_id: + self._warn_unreachable_launch("sweep", run_id) self.notify( f"sweep {run_id} launched (control session {launch.ctl_session(self.project)})" ) @@ -549,22 +567,52 @@ 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(self.project, run_id) + # A ctl listing that failed raises rather than reading as "no window" + # (#750). Say so, but do not abort: the agent session may still be + # reachable, so carry on exactly as if there were no ctl window. + ctl_fault: str | None = None + try: + win_id, unproven = launch.ctl_window_lookup(self.project, run_id) + except MultiplexerError as e: + win_id, unproven, ctl_fault = None, 0, str(e) + self.notify( + f"could not check the run's control window: {e}", + severity="warning", + timeout=15, + ) ok, agent_live = self._mux_guarded(lambda: launch.agent_session_exists(session)) if not ok: return # A sweep blocked on a decision prompt has no agent session — the # human answers in the orchestrator's ctl window. Otherwise prefer the # live agent session, falling back to the ctl window between sessions. - if win_id is not None and (self._dashboard.decision_pending is not None or not agent_live): + wants_ctl = self._dashboard.decision_pending is not None or not agent_live + if unproven: + # A window under this run's name was refused because its tag could + # not be read as ours (#750) — possibly the live orchestrator, even + # beside a tagged window answered here. Say so whatever is attached. + lead = ( + "cannot attach to the run window" + if win_id is None and wants_ctl + else "attaching without a window it could not prove" + ) + self.notify( + f"{lead}: {launch.unproven_ctl_window_notice(self.project, run_id, unproven)}", + severity="warning", + timeout=15, + ) + if win_id is not None and wants_ctl: launch.select_ctl_window_id(win_id) self._attach_to_target(launch.ctl_target(self.project), return_window=win_id) return - elif agent_live: + if agent_live: target = runs.session_target(run_id) else: # Textual captures stderr, so agent_session_exists' warning about a # same-named session of another project's never reaches the screen. + # Checked BEFORE the ctl early returns below: the ctl warning says why + # the window is out of reach, and only this says why the agent + # session is too. ok, refusal = self._mux_guarded(lambda: runs.foreign_session_refusal(session)) if not ok: return @@ -574,6 +622,10 @@ def action_attach(self) -> None: f"not attaching: {refusal}", severity="warning", timeout=10, markup=False ) return + if unproven: + return # the warning above already said why + if ctl_fault is not None: + return # "no ctl window" would be a claim the fault toast unsays self.notify( f"nothing to attach: no live agent session ({session}) and no " f"{launch.ctl_session(self.project)} window for this run (runs started outside " @@ -701,8 +753,9 @@ def _launch_resolve(self, run_id: str) -> None: # is lost is the record *later* verbs read, so `a`/`x` after this # window is minted may answer an older one (#482's symptom). self.notify( - "resolve launched but its window id was not recorded — " - "later attach/stop may target an older window for this run", + "resolve launched but its window id was not recorded or its tag did " + "not land — later attach/stop may miss it or target an older window " + "for this run", severity="warning", ) launch.select_ctl_window_id(win_id) @@ -972,8 +1025,9 @@ def _do_resume(self, run_id: str) -> None: # uncaptured id and the unwritten record through this one signal # because they leave the operator in the same place. self.notify( - "resume launched but its window id was not recorded — " - "attach/stop may target an older window for this run", + "resume launched but its window id was not recorded or its tag did " + "not land — attach/stop may miss it or target an older window for " + "this run", severity="warning", ) self.notify( @@ -1567,7 +1621,6 @@ 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(self.project, run_id) except (OSError, StopRunError, ProcessHostError) as e: self.call_from_thread(self.notify, f"stop failed: {e}", severity="error") return @@ -1580,6 +1633,31 @@ def _stop_run_worker(self, run_id: str, run_dir: Path) -> None: severity="warning", markup=False, ) + try: + left = launch.kill_ctl_window(self.project, run_id) + except (OSError, MultiplexerError, UnicodeError) as e: + # The engine is stopped; only the ctl-window half failed — its listing + # could not be read, or the window survived the kill (#750). A plain + # "stopped" would claim that window is gone. + self.call_from_thread( + self.notify, + f"run {run_id} stopped, but its control window may still be running: {e}", + severity="warning", + timeout=15, + ) + return + if left: + # The engine stopped, but a ctl window under its name may still be + # running — even when another one was closed: a plain "stopped" + # would hide that (#750). + self.call_from_thread( + self.notify, + f"run {run_id} stopped, but a control window under its name was not closed: " + f"{launch.unproven_ctl_window_notice(self.project, run_id, left)}", + severity="warning", + timeout=15, + ) + return self.call_from_thread(self.notify, f"run {run_id} stopped") def action_graceful_stop_run(self) -> None: diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index c3122652..a4fc74ad 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -18,6 +18,7 @@ import stat import subprocess import sys +from collections.abc import Callable from enum import StrEnum from pathlib import Path @@ -393,87 +394,116 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: 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 among the rows - that got in — which, since #531, means the first *tagged* match: a row with - no tag needs the record to be a candidate at all (below), so the recordless - case never has an untagged one to fall back to. - - Scoped to `project` by the PROJECT_OPTION tag: the control session is shared - across projects, and a run id is only unique within one (`--run-id` is - caller-supplied), so a same-id window belonging to another project would - otherwise be a legal match here — for `x` that means killing a *live* - orchestrator next door. _ctl_window_candidates reads tags by the same rule, - but only that half is still shared: its untagged windows are admitted by the - run dir this one stopped trusting, because the pruning consumer of that - shape is #419's and the fix is partitioned there. - - An untagged row is admitted only when the record names that exact window. - Holding the run dir was the earlier gate, and a run dir is a coincidence - rather than a claim: `--run-id` is caller-supplied and deterministic, so two - projects scripting the same id both hold one, and each then admitted the - *other's* untagged window — machine-wide on tmux, where the control session - carries a fixed name (#531). The record is the opposite kind of fact: this - project wrote it, about the window its own launch minted. It survives the - failure this fallback exists for because start_detached records *before* it - tags, so a window whose (best-effort) set_window_option never landed still - has one. - - What the proof costs is reach for a window minted before any record exists — - a fresh `run`/`sweep`, where _record_ctl_window deliberately skips because - the run dir is not minted yet, and anything that lost its record since. Those - answer None until a relaunch records one, and the two consumers wear that - differently: `a` falls through to the live agent session, or says "nothing to - attach" when there is none, while `x` kills nothing — kill_ctl_window no-ops - on None — and still reports the run stopped, so the orchestrator window is - left running with no notice. That is the honest price of the gate, and it is - the cheaper half: fail closed is right here because the alternative is not - "reach my window" but "reach *a* window": with nothing proving ownership the - untagged bucket is filled by listing order, and `x` would kill whatever - sorted first — possibly a neighbour's live orchestrator. - - Untagged stays a *fallback*, not a peer: merged into one listing-ordered - list a recorded untagged row would compete on index with a correctly tagged - one, and the tag is the stronger proof of the two, so untagged is consulted - only when nothing carries this project's tag. - - One residual survives, and it is a conjunction rather than a case: a backend - that reuses a freed window id (a supported divergence — see the id-reuse row - in the tests) hands the neighbour the id this project recorded, while both - projects script the same run id AND the neighbour's own tag write failed AND - the window this record named is already gone. Name and id then both match and - the neighbour is admitted. Closing it needs a channel that says *this window - is mine* rather than *an id I once minted*, and the only one available is the - tag — which this function must read, never write: re-tagging on read is - claiming, not proving, and would hand a neighbour's window this project's - tag. So it is left open, deliberately and visibly. Wherever `runs.is_run` still - holds, every condition in that conjunction was already satisfied by the gate - this replaces — which admitted the neighbour on the run-id collision alone, - with no id reuse and no dead window required — so over those states this is a - strict narrowing. It is not a narrowing everywhere, and the exception is the - residual reached from the other side: _read_ctl_window asks nothing about the - run dir, so an untagged row named by a readable record whose run `runs.is_run` - now rejects (a partial prune is one route there, not the only one) is admitted - here and would have been refused by the old gate — and if that recorded id has - since been reused by an untagged neighbour under this run's name, the row - admitted is the neighbour's. That is the whole of what this gate trades away. - Narrowing it is a mechanism question - — an identity channel written at mint time and read here — and this function - is the wrong place to decide it.""" + no-op). With no record at all the answer is the first tagged match. + + Scoped to `project` by the PROJECT_OPTION tag, and by nothing else: only a + row carrying one of this project's accepted tags is a candidate. The control + session is shared across projects, and a run id is only unique within one + (`--run-id` is caller-supplied), so a same-id window belonging to another + project would otherwise be a legal match here — for `x` that means killing a + *live* orchestrator next door. _ctl_window_candidates reads tags by the same + rule, but only that half is shared: its untagged windows are still admitted + by the run dir, because the pruning consumer of that shape is #419's. + + An untagged row is never a candidate, however it is vouched for (#750). + Every proof tried for one was a fact the workspace can forge. The run dir is + a coincidence of a caller-supplied id (#531). The record lives under the + project root every coding session can write, and the window id it names is + a reusable handle — tmux 3.4 and psmux 3.3.8 both restart at `@0`/`@1` after + a server restart. A pane pid recorded beside it is observable rather than + secret: any same-UID process reads it from the process table, along with the + `TMUX` socket path in the pane's environment, and anything minted into the + window at creation passes through the multiplexer client's argv, which the + process table also shows. The tag is the one claim that needs a write + through the multiplexer to forge, so it is the whole proof. + + The honest bar, then: forging ownership here requires a mux write, exactly + as forging the tag does — no more, and no less. On a host where the coding + session runs unsandboxed as the same user, that is any process that can + find the socket, which the pane environment hands out; the gate is only as + strong as the session's isolation from the multiplexer. + + What it costs is reach for a window whose tag cannot be read as ours: one + whose best-effort tag write failed at launch (start_detached records before + it tags, but the record admits nothing on its own), and — every window at + once — a listing whose option column could not be read at all, which psmux + folds to "" rather than failing (PsmuxMultiplexer.list_windows). The seam + hands both back as the same empty tag, so neither this function nor its + callers can tell them apart. Fail closed is still right — the alternative + is not "reach my window" but "reach *a* window", possibly a neighbour's live + orchestrator — but it must not be silent: a None here reads as "no window", + and `x` would then report the run stopped while its window keeps running. + So ctl_window_lookup also counts the same-run rows it refused for want of a + readable tag, and `x` and `a` say so (see kill_ctl_window and the TUI's + attach) instead of passing the refusal off as an absence. + + Read-only throughout: this never writes the record and never touches the + tag — re-tagging on read is claiming, not proving, and would hand a + neighbour's window this project's tag.""" + return ctl_window_lookup(project, run_id)[0] + + +def _list_ctl_windows( + mux: TerminalMultiplexer, session: str, fields: list[str] +) -> list[tuple[str, ...]]: + """`mux.list_windows`, made to fail loud. The seam's list_windows answers + [] both for a session with no windows and for a query that failed — it + only warns, on a stderr the TUI captures — so an empty answer here would + read as a clean absence: `x` reporting a run stopped over a live window, + a prune reporting nothing to close. An empty answer is therefore confirmed + with list_window_ids, whose [] is a positive claim (the session listed + empty, or is proven gone) and which raises MultiplexerError itself when + its listing cannot be taken. Empty here but windows there is the failed + read, and raises the same type. + + One extra query, and only when the listing came back empty. Ceiling: a + window minted between the two reads also lands in the raise — loud, never + silent, and the next look answers it.""" + rows = mux.list_windows(session, fields) + if not rows and mux.list_window_ids(session): + raise MultiplexerError( + f"could not list the windows of {session}: the listing answered none " + "while the session has some" + ) + return rows + + +def ctl_window_lookup(project: Path, run_id: str) -> tuple[str | None, int]: + """ctl_window_id's answer, plus how many windows carrying this run's name + it refused because their tag read empty — unset, or unreadable (see + ctl_window_id). + + A nonzero count is a degraded answer a caller must surface, with or + without a window: no window is not "no window", and a window beside a + refused row may be the parked predecessor of a relaunch whose own tag + write failed — so the live orchestrator is the one refused, and a stop + that closes the predecessor has still left it running. A refused row may + equally be a neighbour's untagged window under the same caller-supplied + run id, so the count is a notice, never a target. + + Raises MultiplexerError when the listing itself could not be read (see + _list_ctl_windows), and when the backend is unavailable at all: neither is + "no window", and every caller surfaces it — the TUI's attach warns and + carries on to the agent session, the stop worker warns the window may still + be running, the CLI attach warns on stderr, ctl_window_recorded says it + could not confirm. Availability is re-read here, not trusted from a + caller's gate: it can change while a confirm modal is open.""" if not mux_available(): - return None + raise MultiplexerError( + "multiplexer backend unavailable: the run's control window could not be looked up" + ) mine = runs.accepted_tags(project) tagged: list[str] = [] - untagged: list[str] = [] - rows = get_multiplexer().list_windows( - ctl_session(project), ["window_id", "window_name", runs.PROJECT_OPTION] + unproven = 0 + rows = _list_ctl_windows( + get_multiplexer(), ctl_session(project), ["window_id", "window_name", runs.PROJECT_OPTION] ) - # Below the listing, above the loop. The loop needs it — it is what admits an - # untagged row — but listing and record are two reads of a state a concurrent + # Below the listing. Listing and record are two reads of a state a concurrent # relaunch can move between them, never one snapshot, so the ordering is the # only thing to get right and this keeps the one the record has always had. # Read FIRST, a relaunch landing in the gap leaves a record older than the # listing: it names the window that relaunch superseded, the listing shows it, - # and `recorded in matches` replays the corpse. Read here, the same relaunch + # and `recorded in tagged` replays the corpse. Read here, the same relaunch # leaves a record newer than the listing, naming a window the listing does not # carry yet — so it fails the re-prove and the answer falls back to a match # that was at least live when the listing was taken. `rows` is materialized @@ -483,8 +513,8 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: # 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 CAN produce an - # empty *tag* — it fills trailing fields — which is exactly the untagged - # case below; window_id stays field 0 of 3.) + # empty *tag* — it fills trailing fields — which the tag test below + # refuses; window_id stays field 0 of 3.) if not win_id: continue # The whole run id, not a suffix of the name: RUN_ID_RE admits `-`, so @@ -506,35 +536,56 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: # digest alone would strand this project's own orchestrator — prunable # by _ctl_window_candidates, which accepts the legacy tag, yet # unreachable by `a` and `x`, which resolve through here. + # An empty tag is refused the same way (#750), but counted: it is unset + # or unreadable, and the caller must be able to say so (see docstrings). if tag in mine: tagged.append(win_id) - elif not tag and win_id == recorded: - # untagged, but this project's own launch recorded this window — - # proof of the mint, not of the tag, so it only counts if nothing - # is tagged - untagged.append(win_id) - matches = tagged or untagged - if not matches: - return None - # Membership in `matches`, not mere presence in the listing: it re-checks the + elif not tag: + unproven += 1 + if not tagged: + return None, unproven + # Membership in `tagged`, not mere presence in the listing: it re-checks the # name and the project-scoping predicates, so a record whose id is absent from # the scoped matches — killed, pruned, renamed onto another run, or naming a - # row this project cannot claim — is not replayed. Not a proof of identity: a - # reused id under this run's name still passes, which is the residual the - # docstring names. But it is what turns a stale id from a replayed target - # into a fallthrough. - return recorded if recorded in matches else matches[0] + # row this project cannot claim — is not replayed. A tie-break among rows the + # tag already proved, never a proof on its own: it is what turns a stale id + # from a replayed target into a fallthrough. + return (recorded if recorded in tagged else tagged[0]), unproven + + +def unproven_ctl_window_notice(project: Path, run_id: str, count: int) -> str: + """Operator wording for a nonzero ctl_window_lookup count: what was refused, + the two causes the seam cannot tell apart, and what to do about it. Worded + by count, so one window and several each read as a sentence.""" + where = f"in {ctl_session(project)} named for run {run_id}" + if count == 1: + return ( + f"a window {where} has no readable project tag, so it cannot be proven " + "this project's: its tag could not be read or was never written. Left " + "untouched — check it in the multiplexer and close it by hand if it is " + "this run's" + ) + return ( + f"{count} windows {where} have no readable project tag, so none can be " + "proven this project's: their tags could not be read or were never written. " + "Left untouched — check them in the multiplexer and close by hand any that " + "are this run's" + ) def ctl_window_recorded(project: Path, run_id: str, win_id: str) -> bool: """Whether `ctl_window_id` now answers `win_id` for this run — i.e. whether the launch's disambiguation actually took. - False means the launch itself succeeded but the lookup is back on the - ambiguous first-match scan, which is exactly #482's symptom and so is - operator-visible: every launcher that mints a second window under a run id + False means the launch itself succeeded but the lookup will not answer + the window it minted: either it is back on the ambiguous first-match scan, + which is exactly #482's symptom, or — since #750 — the window's + best-effort tag write did not land, so the lookup refuses it outright and + `a`/`x` cannot reach it at all. Both are operator-visible: every launcher should report it rather than let an unqualified success toast imply the - targeting is sound. Split out of resume_detached's return so the resolve + targeting is sound (start_run_detached and start_sweep_detached included, + which mint the only window under a fresh run id but still depend on its + tag). Split out of resume_detached's return so the resolve path can warn while still keeping the captured id it attaches with. Asks `ctl_window_id` rather than comparing the record to `win_id`, because @@ -728,32 +779,65 @@ def decision_pending(run_dir: Path) -> bool: return bool(entries) and entries[-1].get("kind") == "decision-pending" -def attach_plan(project: Path, run_id: str) -> tuple[list[str], str | None] | None: +def attach_plan( + project: Path, run_id: str, *, on_fault: Callable[[str], None] | None = None +) -> tuple[tuple[list[str], str | None] | None, int]: """Pick where an interactive attach should land for this run and which window (if any) to record a return target on. Shared by the CLI `attach` command and mirroring the TUI's action_attach logic: prefer the orchestrator's ctl window when a sweep is blocked on a decision or no agent session is live, else the - live agent session. Returns (tmux argv, return_window) or None when there is - nothing to attach to.""" + live agent session. Returns ((tmux argv, return_window) or None when there + is nothing to attach to, unproven) — `unproven` is ctl_window_lookup's count + of same-run windows refused for an empty tag, carried out with the plan + because a None plan is not "no window" when it is nonzero, and an agent + plan may have bypassed the very window a pending decision is waiting in + (#750). The caller must say so; see unproven_ctl_window_notice. + + A ctl lookup that raises (its listing could not be read) is handed to + `on_fault` and the plan carries on as if there were no ctl window: the + agent session may still be reachable, and an attach must not be refused + for a window it could not check. With no `on_fault` the raise propagates.""" session = runs.session_name(run_id) - win_id = ctl_window_id(project, run_id) + try: + win_id, unproven = ctl_window_lookup(project, run_id) + except MultiplexerError as e: + if on_fault is None: + raise + on_fault(str(e)) + win_id, unproven = None, 0 agent_live = agent_session_exists(session) if win_id is not None and ( decision_pending(runs.run_dir_for(project, run_id)) or not agent_live ): select_ctl_window_id(win_id) - return runs.attach_target_argv(ctl_target(project)), win_id + return (runs.attach_target_argv(ctl_target(project)), win_id), unproven if agent_live: - return runs.attach_target_argv(runs.session_target(run_id)), None - return None + return (runs.attach_target_argv(runs.session_target(run_id)), None), unproven + return None, unproven -def kill_ctl_window(project: Path, run_id: str) -> None: +def kill_ctl_window(project: Path, run_id: str) -> int: """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(project, run_id) + if any. A no-op when the run was not launched from the TUI or tmux is gone. + + Returns how many same-run windows were left alive because none could be + proven this project's (ctl_window_lookup's count): 0 means the kill went + through or there was nothing to kill, and anything else is a window the + caller must report as possibly still running rather than as stopped. + + Raises MultiplexerError when the ctl listing could not be read at all + (ctl_window_lookup), and when the window it killed is still listed + afterwards: kill_window is best-effort by contract — a transport failure + is a silent no-op — so the kill is confirmed against list_window_ids, the + same membership verdict prune_ctl_windows takes. Neither may be reported + as a clean stop (#750).""" + win_id, unproven = ctl_window_lookup(project, run_id) if win_id is not None: - get_multiplexer().kill_window(win_id) + mux = get_multiplexer() + mux.kill_window(win_id) + if win_id in mux.list_window_ids(ctl_session(project)): + raise MultiplexerError(f"control window {win_id} survived the kill") + return unproven def _ctl_window_candidates(project: Path) -> list[tuple[str, str]]: @@ -769,13 +853,29 @@ def _ctl_window_candidates(project: Path) -> list[tuple[str, str]]: The control session is shared across projects, so its per-window PROJECT_OPTION accepts current and legacy project tags; untagged windows still require a run directory under this project (mirrors runs.prunable_sessions). + + Residual, left visible rather than fixed: when psmux's option probe fails, + PsmuxMultiplexer.list_windows reads every tag as empty, so an untagged row + of ours whose run dir is gone is skipped here without a report. Telling an + unreadable tag from an unset one needs an `on_fault` on list_windows, which + is a seam change. Likewise an unavailable backend still reads as no + candidates here (the early `return []`), a known residual left for a + separate decision. """ mux = get_multiplexer() ctl = runs.ctl_session_for(project, mux) - if not mux_usable(mux) or not session_exists(ctl): + if not mux_usable(mux): + return [] + # A False has-session is weaker than it looks (its seam note): a refused + # connect reads the same as a missing session. So it only short-circuits + # when list_window_ids agrees there is nothing — whose [] is a positive + # claim, and which raises when its own listing cannot be taken (#750). + if not session_exists(ctl) and not mux.list_window_ids(ctl): return [] current = mux.current_window_id() - rows = mux.list_windows(ctl, ["window_id", "window_name", runs.PROJECT_OPTION]) + # Fail loud on a listing that failed: both prune callers already report a + # raise from this scan, and an empty answer would read as nothing to prune. + rows = _list_ctl_windows(mux, ctl, ["window_id", "window_name", runs.PROJECT_OPTION]) mine = runs.accepted_tags(project) candidates: list[tuple[str, str]] = [] for win_id, name, tag in rows: @@ -1084,6 +1184,21 @@ def start_detached(project: Path, argv_tail: list[str], run_id: str, kind: str) return win_id +def _reachable_window(project: Path, run_id: str, win_id: str | None) -> str | None: + """`win_id` when ctl_window_id will answer it for this run, else None — the + launch-time check every launcher's caller turns into a warning. + + A fresh run or sweep mints the only window under its run id and writes no + record, so the one thing that can still go wrong is the tag: start_detached + stamps it best-effort, and since #750 an untagged window is refused by the + lookup, so `a`/`x` would silently miss it. Re-reading through the lookup + (ctl_window_recorded) asks the consumers' own question, and also catches an + uncaptured id or a listing that could not be read.""" + if win_id and not ctl_window_recorded(project, run_id, win_id): + return None + return win_id + + def start_run_detached( project: Path, run_id: str, @@ -1092,7 +1207,9 @@ def start_run_detached( epic: int | None = None, story: str | None = None, max_stories: int | None = None, -) -> None: +) -> str | None: + """Launch a run in a ctl-session window; returns the window id, or None + when the lookup cannot reach it afterwards — see _reachable_window.""" tail = ["run", "--project", str(project), "--run-id", run_id] if spec: tail += ["--spec", spec] # forces stories mode (folder+id dispatch) @@ -1102,7 +1219,7 @@ def start_run_detached( tail += ["--story", story] if max_stories is not None: tail += ["--max-stories", str(max_stories)] - start_detached(project, tail, run_id, "run") + return _reachable_window(project, run_id, start_detached(project, tail, run_id, "run")) def start_sweep_detached( @@ -1112,7 +1229,9 @@ def start_sweep_detached( no_prompt: bool = False, decisions_only: bool = False, max_bundles: int | None = None, -) -> None: +) -> str | None: + """Launch a sweep in a ctl-session window; returns the window id, or None + when the lookup cannot reach it afterwards — see _reachable_window.""" tail = ["sweep", "--project", str(project), "--run-id", run_id] if no_prompt: tail.append("--no-prompt") @@ -1120,7 +1239,7 @@ def start_sweep_detached( tail.append("--decisions-only") if max_bundles is not None: tail += ["--max-bundles", str(max_bundles)] - start_detached(project, tail, run_id, "sweep") + return _reachable_window(project, run_id, start_detached(project, tail, run_id, "sweep")) def resume_detached(project: Path, run_id: str) -> str | None: @@ -1146,9 +1265,7 @@ def resume_detached(project: Path, run_id: str) -> str | None: win_id = start_detached( project, ["resume", "--project", str(project), run_id], run_id, "resume" ) - if win_id and not ctl_window_recorded(project, run_id, win_id): - return None - return win_id + return _reachable_window(project, run_id, win_id) def start_resolve_detached(project: Path, run_id: str) -> str | None: diff --git a/tests/test_cli.py b/tests/test_cli.py index 6197a6dc..077f7846 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2803,11 +2803,11 @@ def test_attach_records_return_pane_inside_tmux(project, monkeypatch): monkeypatch.setattr( launch, "attach_plan", - lambda proj, rid: ( + lambda proj, rid, **_kw: ( planned.append((proj, rid)) or ( - ["tmux", "switch-client", "-t", "=bmad-loop-ctl"], - "=bmad-loop-ctl:sweep-RID", + (["tmux", "switch-client", "-t", "=bmad-loop-ctl"], "=bmad-loop-ctl:sweep-RID"), + 0, ) ), ) @@ -2833,9 +2833,9 @@ def test_attach_records_detach_outside_tmux(project, monkeypatch): monkeypatch.setattr( launch, "attach_plan", - lambda proj, rid: ( - ["tmux", "attach", "-t", "=bmad-loop-ctl"], - "=bmad-loop-ctl:sweep-RID", + lambda proj, rid, **_kw: ( + (["tmux", "attach", "-t", "=bmad-loop-ctl"], "=bmad-loop-ctl:sweep-RID"), + 0, ), ) monkeypatch.delenv("TMUX", raising=False) @@ -2854,7 +2854,10 @@ def test_attach_agent_session_records_no_return(project, monkeypatch): monkeypatch.setattr( launch, "attach_plan", - lambda proj, rid: (["tmux", "attach", "-t", "=bmad-loop-20260101-000000-aaaa"], None), + lambda proj, rid, **_kw: ( + (["tmux", "attach", "-t", "=bmad-loop-20260101-000000-aaaa"], None), + 0, + ), ) recorded: list = [] monkeypatch.setattr(launch, "set_return_pane", lambda w, p: recorded.append((w, p))) @@ -2870,10 +2873,94 @@ def test_attach_nothing_to_attach(project, monkeypatch, capsys): from bmad_loop.tui import launch _make_run_with_decision(project, run_id="20260101-000000-aaaa") - monkeypatch.setattr(launch, "attach_plan", lambda proj, rid: None) + monkeypatch.setattr(launch, "attach_plan", lambda proj, rid, **_kw: (None, 0)) + + assert cli.main(["attach", "--project", str(project.project), "20260101-000000-aaaa"]) == 1 + err = capsys.readouterr().err + assert "nothing to attach" in err + assert "no readable project tag" not in err + + +def test_attach_explains_an_unproven_ctl_window(project, monkeypatch, capsys): + # #750: a window under this run's name was refused for an empty tag (unset, + # or unreadable). With nothing else to attach the exit code stays the + # "nothing to attach" FAILURE, but stderr says why instead of passing the + # refusal off as an absence. + from bmad_loop.tui import launch + + _make_run_with_decision(project, run_id="20260101-000000-aaaa") + monkeypatch.setattr(launch, "attach_plan", lambda proj, rid, **_kw: (None, 1)) + + assert cli.main(["attach", "--project", str(project.project), "20260101-000000-aaaa"]) == 1 + err = capsys.readouterr().err + assert "no readable project tag" in err + assert "nothing to attach" in err + + +def test_attach_warns_before_falling_back_past_an_unproven_window(project, monkeypatch, capsys): + # The other half: a decision waits in the refused window, so the plan falls + # back to the agent session. The attach still happens - and the operator is + # told which window it went around, BEFORE the attach takes over the + # terminal: stderr is read inside the attach, not after it returns. + from bmad_loop.tui import launch + + _make_run_with_decision(project, run_id="20260101-000000-aaaa") + agent = ["tmux", "attach", "-t", "=bmad-loop-20260101-000000-aaaa"] + monkeypatch.setattr(launch, "attach_plan", lambda proj, rid, **_kw: ((agent, None), 1)) + seen_at_attach: list[str] = [] + + def attach(argv): + seen_at_attach.append(capsys.readouterr().err) + return 0 + + monkeypatch.setattr(cli.subprocess, "call", attach) + + assert cli.main(["attach", "--project", str(project.project), "20260101-000000-aaaa"]) == 0 + assert len(seen_at_attach) == 1 + assert "no readable project tag" in seen_at_attach[0] + + +def test_attach_warns_about_a_ctl_lookup_fault_and_still_attaches(project, monkeypatch, capsys): + # #750: the real attach_plan, with a ctl lookup that raises. cmd_attach + # says so on stderr and still attaches to the live agent session, rc 0. + from bmad_loop.adapters.multiplexer import MultiplexerError + from bmad_loop.tui import launch + + def boom(_proj, _rid): + raise MultiplexerError("could not list the windows of bmad-loop-ctl") + + _make_run_with_decision(project, run_id="20260101-000000-aaaa") + monkeypatch.setattr(launch, "ctl_window_lookup", boom) + monkeypatch.setattr(launch, "agent_session_exists", lambda s: True) + monkeypatch.setattr( + "bmad_loop.tui.launch.runs.attach_target_argv", lambda target: ["mux", "attach", target] + ) + called: list = [] + monkeypatch.setattr(cli.subprocess, "call", lambda argv: called.append(argv) or 0) + + assert cli.main(["attach", "--project", str(project.project), "20260101-000000-aaaa"]) == 0 + assert len(called) == 1 and called[0][:2] == ["mux", "attach"] + assert "could not check the run's control window" in capsys.readouterr().err + + +def test_attach_does_not_claim_absence_after_a_ctl_lookup_fault(project, monkeypatch, capsys): + # With no agent session either, the attach fails (rc 1, FAILURE) — but its + # stderr says the ctl window could not be checked, never "nothing to + # attach", which would claim that unchecked window absent. + from bmad_loop.adapters.multiplexer import MultiplexerError + from bmad_loop.tui import launch + + def boom(_proj, _rid): + raise MultiplexerError("could not list the windows of bmad-loop-ctl") + + _make_run_with_decision(project, run_id="20260101-000000-aaaa") + monkeypatch.setattr(launch, "ctl_window_lookup", boom) + monkeypatch.setattr(launch, "agent_session_exists", lambda s: False) assert cli.main(["attach", "--project", str(project.project), "20260101-000000-aaaa"]) == 1 - assert "nothing to attach" in capsys.readouterr().err + err = capsys.readouterr().err + assert "could not check the run's control window" in err + assert "nothing to attach" not in err def test_attach_multiplexer_error_surfaces_clean_error(project, monkeypatch, capsys): @@ -2884,7 +2971,7 @@ def test_attach_multiplexer_error_surfaces_clean_error(project, monkeypatch, cap from bmad_loop.adapters.multiplexer import MultiplexerError from bmad_loop.tui import launch - def boom(_proj, _rid): + def boom(_proj, _rid, **_kw): raise MultiplexerError("backend server not reachable") _make_run_with_decision(project, run_id="20260101-000000-aaaa") diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index 128c8ca2..f9d98587 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -2703,6 +2703,47 @@ async def test_long_docked_title_keeps_its_markers(project_tree): # ------------------------------------------------------------- run control +@pytest.mark.parametrize("minted", [None, "@7"], ids=["unreachable", "reachable"]) +async def test_start_run_warns_when_its_window_is_unreachable(project, monkeypatch, minted): + # #750: start_run_detached returns None when the lookup `a`/`x` use cannot + # reach the window it just minted (its tag write did not land). The launch + # is still reported, with a warning beside it — and only then. + calls: list = [] + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "start_run_detached", lambda *a, **kw: calls.append(a) or minted) + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await pilot.press("r") + await until(pilot, lambda: isinstance(app.screen, StartRunModal)) + await ready(pilot, "#ok") + await pilot.click("#ok") + await until(pilot, lambda: bool(calls)) + await until(pilot, lambda: any(" launched " in m for m in notifications(app))) + warned = any("could not be confirmed" in m for m in notifications(app)) + assert warned is (minted is None) + + +@pytest.mark.parametrize("minted", [None, "@7"], ids=["unreachable", "reachable"]) +async def test_start_sweep_warns_when_its_window_is_unreachable(project, monkeypatch, minted): + # The sweep twin of the run-launch warning (#750): None from the launcher + # means `a`/`x` cannot reach the window it just minted. + calls: list = [] + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "start_sweep_detached", lambda *a, **kw: calls.append(a) or minted) + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await pilot.press("s") + await until(pilot, lambda: isinstance(app.screen, StartSweepModal)) + await ready(pilot, "#ok") + await pilot.click("#ok") + await until(pilot, lambda: bool(calls)) + await until(pilot, lambda: any(" launched " in m for m in notifications(app))) + warned = any("could not be confirmed" in m for m in notifications(app)) + assert warned is (minted is None) + + async def test_start_run_modal_escape_cancels(project_tree, monkeypatch): calls = [] monkeypatch.setattr(launch, "mux_available", lambda: True) @@ -3769,7 +3810,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 proj, run_id: None) + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: (None, 0)) make_run(project.project, "20260611-100000-aaaa") app = BmadLoopApp(project.project) async with app.run_test() as pilot: @@ -3779,6 +3820,36 @@ async def test_attach_without_agent_session_notifies(project, monkeypatch): await until(pilot, lambda: any("no live agent session" in m for m in notifications(app))) +async def test_attach_says_both_why_with_an_unproven_window_and_a_foreign_session( + project, monkeypatch +): + """An unproven ctl window (#750) AND a same-named agent session that is + another project's: two reasons nothing is attached, and both must reach the + screen — the unproven-window warning, and the foreign-session refusal that + agent_session_exists only wrote to the stderr Textual captures. + + Ablation: move `if unproven: return` back above the refusal check and only + the unproven-window warning appears.""" + attached: list[str] = [] + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "session_exists", lambda session: True) + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: (None, 1)) + monkeypatch.setattr( + "bmad_loop.tui.app.runs.foreign_session_refusal", + lambda session, *_mux: f"{session} in the shared registry is tagged for another project", + ) + make_run(project.project, "20260611-100000-aaaa") + app = BmadLoopApp(project.project) + monkeypatch.setattr(app, "_attach_to_target", lambda target, **_k: attached.append(target)) + 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("a") + await until(pilot, lambda: any("not attaching" in m for m in notifications(app))) + assert any("cannot attach to the run window" in m for m in notifications(app)) + assert attached == [] + + async def test_attach_to_another_projects_session_says_why(project, monkeypatch): """The session EXISTS, and is another project's: the attach must not land on it, and since Textual captures stderr, `agent_session_exists`' warning never @@ -3823,7 +3894,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 proj, run_id: None) + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: (None, 0)) def boom(_target): raise MultiplexerError("backend server not reachable") @@ -3847,7 +3918,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 proj, run_id: None) + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: (None, 0)) def boom(_session): raise MultiplexerError("session probe unreachable") @@ -3938,7 +4009,7 @@ async def test_attach_targets_ctl_window_when_decision_pending(project_tree, mon 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 proj, run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: ("@5", 0)) monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) calls, stamps = _patch_attach_exec(monkeypatch) app = BmadLoopApp(project_tree.project) @@ -3953,16 +4024,62 @@ async def test_attach_targets_ctl_window_when_decision_pending(project_tree, mon assert stamps == [("@5", "=main:%9")] +@pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals +async def test_attach_warns_then_falls_back_to_the_agent_past_an_unproven_window( + project_tree, monkeypatch +): + # #750: the decision prompt's ctl window cannot be proven ours (its tag reads + # empty), so it is out of reach — say so, then still take the live agent + # session. A return after the warning would strand the operator: this pins + # both halves. + run_dir = make_run(project_tree.project, "20260611-100000-aaaa", run_type="sweep", alive=True) + Journal(run_dir).append("decision-pending", dw_id="DW-7", question="q?") + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "session_exists", lambda session: True) + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: (None, 1)) + calls, stamps = _patch_attach_exec(monkeypatch) + app = BmadLoopApp(project_tree.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 any("cannot attach to the run window" in m for m in notifications(app)) + assert calls == [["tmux", "switch-client", "-t", "=bmad-loop-20260611-100000-aaaa"]] + assert stamps == [] + + +@pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals +async def test_attach_warns_about_an_unproven_window_beside_a_proven_one(project_tree, monkeypatch): + # A tagged predecessor is answered, but an untagged window under the same + # run name (a relaunch whose tag write failed) was passed over: the attach + # goes ahead, and the operator still hears about the one left out. + run_dir = make_run(project_tree.project, "20260611-100000-aaaa", run_type="sweep", alive=True) + Journal(run_dir).append("decision-pending", dw_id="DW-7", question="q?") + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "session_exists", lambda session: True) + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: ("@5", 1)) + monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: None) + calls, _stamps = _patch_attach_exec(monkeypatch) + app = BmadLoopApp(project_tree.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 any("no readable project tag" in m for m in notifications(app)) + assert calls == [["tmux", "switch-client", "-t", "=bmad-loop-ctl"]] + + @pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals async def test_attach_uses_the_recorded_ctl_window(project_tree, monkeypatch): - # The one attach test that does NOT replace ctl_window_id, so it pins the + # The one attach test that does NOT replace ctl_window_lookup, 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 under that - # root, so these untagged rows prove nothing and the lookup answers None - # (#531). session_exists is stubbed True here, so `a` then takes the live - # agent session instead of the ctl window: nothing is selected and nothing is - # stamped, and both assertions below fail. + # root, so the tie-break among these tagged rows falls back to the parked + # `@1` and both assertions below fail. Tagged as start_detached leaves them: + # an untagged row is never a candidate (#750), whatever the record says. import subprocess as _subprocess from bmad_loop.adapters import tmux_base @@ -3971,10 +4088,12 @@ async def test_attach_uses_the_recorded_ctl_window(project_tree, monkeypatch): run_dir = make_run(project_tree.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") + tag = runs_mod.project_tag(project_tree.project) selected: list[str] = [] def fake(argv, **kwargs): - out = f"@1\trun-{rid}\n@2\tresume-{rid}\n" if argv[1] == "list-windows" else "" + rows = f"@1\trun-{rid}\t{tag}\n@2\tresume-{rid}\t{tag}\n" + out = rows if argv[1] == "list-windows" else "" return _subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") monkeypatch.setattr(tmux_base.subprocess, "run", fake) @@ -4003,7 +4122,7 @@ async def test_attach_outside_tmux_stamps_detach(project_tree, 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 proj, run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: ("@5", 0)) 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_tree.project) @@ -4020,7 +4139,7 @@ async def test_attach_prefers_agent_session_without_decision(project_tree, monke make_run(project_tree.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 proj, run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: ("@5", 0)) calls, stamps = _patch_attach_exec(monkeypatch) app = BmadLoopApp(project_tree.project) async with app.run_test() as pilot: @@ -4039,7 +4158,7 @@ async def test_attach_falls_back_to_ctl_window(project_tree, 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 proj, run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: ("@5", 0)) monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) calls, stamps = _patch_attach_exec(monkeypatch) app = BmadLoopApp(project_tree.project) @@ -5769,6 +5888,120 @@ async def test_stop_run_stops_and_kills_ctl_window(project_tree, monkeypatch): assert kills == [(project_tree.project, "20260611-100000-aaaa")] +async def test_stop_run_warns_when_the_ctl_window_is_left_running(project_tree, monkeypatch): + # #750: a window under this run's name whose tag reads empty — unset, or + # unreadable (psmux folds a failed option probe to "") — is never killed. + # Reporting a plain "stopped" then hides a control window still running, + # so the stop must say it left one, and must not also claim a clean stop. + from bmad_loop import runs + + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(data, "liveness", lambda run_dir: "alive") + monkeypatch.setattr(runs, "stop_run", lambda rd: True) + monkeypatch.setattr(launch, "kill_ctl_window", lambda proj, rid: 1) + make_run(project_tree.project, "20260611-100000-aaaa", alive=True) + app = BmadLoopApp(project_tree.project) + async with app.run_test() as pilot: + await until(pilot, lambda: dashboard(app).selected_run_id == "20260611-100000-aaaa") + await pilot.press("x") + await until(pilot, lambda: isinstance(app.screen, ConfirmModal)) + await click(pilot, await ready(pilot, "#ok")) + needle = "was not closed" + await until(pilot, lambda: any(needle in m for m in notifications(app))) + assert "run 20260611-100000-aaaa stopped" not in notifications(app) + + +async def test_stop_run_warns_when_the_ctl_listing_cannot_be_read(project_tree, monkeypatch): + # #750: the ctl listing itself failed, so the lookup raises rather than + # answering "no window". The engine stop already went through; the toast + # must say the window could not be checked instead of a clean "stopped". + from bmad_loop import runs + + def boom(proj, rid): + raise MultiplexerError("could not list the windows of bmad-loop-ctl") + + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(data, "liveness", lambda run_dir: "alive") + monkeypatch.setattr(runs, "stop_run", lambda rd: True) + monkeypatch.setattr(launch, "kill_ctl_window", boom) + make_run(project_tree.project, "20260611-100000-aaaa", alive=True) + app = BmadLoopApp(project_tree.project) + async with app.run_test() as pilot: + await until(pilot, lambda: dashboard(app).selected_run_id == "20260611-100000-aaaa") + await pilot.press("x") + await until(pilot, lambda: isinstance(app.screen, ConfirmModal)) + await click(pilot, await ready(pilot, "#ok")) + needle = "may still be running" + await until(pilot, lambda: any(needle in m for m in notifications(app))) + assert "run 20260611-100000-aaaa stopped" not in notifications(app) + assert not any("stop failed" in m for m in notifications(app)) + + +async def test_attach_warns_about_a_ctl_listing_it_could_not_read(project, monkeypatch): + # #750: a raise from the ctl lookup is said — but with no agent session + # either, nothing is attached, and never the plain "nothing to attach", + # which would claim the ctl window is absent. + def boom(proj, rid): + raise MultiplexerError("could not list the windows of bmad-loop-ctl") + + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "agent_session_exists", lambda session: False) + monkeypatch.setattr("bmad_loop.tui.app.runs.foreign_session_refusal", lambda s, *_m: None) + monkeypatch.setattr(launch, "ctl_window_lookup", boom) + make_run(project.project, "20260611-100000-aaaa") + 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("a") + needle = "could not check the run's control window" + await until(pilot, lambda: any(needle in m for m in notifications(app))) + assert not any("nothing to attach" in m for m in notifications(app)) + assert isinstance(app.screen, DashboardScreen) + + +async def test_attach_reaches_the_agent_past_a_ctl_lookup_fault(project, monkeypatch): + # The regression this pins: a ctl lookup that raises must not abort `a`. + # It warns, then resolves the agent session exactly as when there is no + # ctl window. Ablation: restore the guard's early return on the lookup and + # the attach below never happens. + attached: list[str] = [] + + def boom(proj, rid): + raise MultiplexerError("could not list the windows of bmad-loop-ctl") + + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "agent_session_exists", lambda session: True) + monkeypatch.setattr(launch, "ctl_window_lookup", boom) + make_run(project.project, "20260611-100000-aaaa") + app = BmadLoopApp(project.project) + monkeypatch.setattr(app, "_attach_to_target", lambda target, **_k: attached.append(target)) + 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("a") + await until(pilot, lambda: bool(attached)) + assert any("could not check the run's control window" in m for m in notifications(app)) + assert attached == [runs_mod.session_target("20260611-100000-aaaa")] + + +async def test_attach_says_why_an_unproven_ctl_window_is_out_of_reach(project, monkeypatch): + # #750: no window answered, but one carries this run's name with an empty + # tag. "nothing to attach" would pass that refusal off as an absence. + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "session_exists", lambda session: False) + monkeypatch.setattr(launch, "ctl_window_lookup", lambda proj, run_id: (None, 1)) + make_run(project.project, "20260611-100000-aaaa") + 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("a") + needle = "cannot attach to the run window" + await until(pilot, lambda: any(needle in m for m in notifications(app))) + assert not any("nothing to attach" in m for m in notifications(app)) + + async def test_stop_run_says_when_its_backstop_kill_was_refused(project_tree, monkeypatch): """A hard stop whose backstop kill the shared-registry ownership gate refused leaves the session standing and warns on stderr, which Textual captures: the @@ -5784,7 +6017,7 @@ def stop(_run_dir): monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(data, "liveness", lambda run_dir: "alive") monkeypatch.setattr(runs, "stop_run", stop) - monkeypatch.setattr(launch, "kill_ctl_window", lambda proj, rid: None) + monkeypatch.setattr(launch, "kill_ctl_window", lambda proj, rid: 0) make_run(project_tree.project, "20260611-100000-aaaa", alive=True) app = BmadLoopApp(project_tree.project) async with app.run_test() as pilot: diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index 250f60ae..9b336cae 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -36,12 +36,18 @@ class FakeRun: """Records argv; scripts the returncode of `tmux has-session` and the rows `list-windows` answers. The listing defaults to showing the window `new-window` just minted, which is what a real backend does — and what - ctl_window_recorded re-proves the record against.""" + ctl_window_recorded re-proves the record against. + + Like a real backend, a PROJECT_OPTION tag a `set-option` stamps on a window + shows up in its listed row — the only proof of ownership ctl_window_id + accepts (#750). A scripted row with its own third field (an empty one for + an untagged window) keeps it.""" def __init__(self, has_session_rc: int = 1, windows: str = "@7\tresume-RID\n"): self.calls: list[list[str]] = [] self.has_session_rc = has_session_rc self.windows = windows + self.tags: dict[str, str] = {} def __call__(self, argv, **kwargs): self.calls.append(list(argv)) @@ -49,10 +55,17 @@ def __call__(self, argv, **kwargs): out = "" if argv[1] == "new-window": out = "@7\n" + elif argv[1:4] == ["set-option", "-w", "-t"] and argv[5:6] == [runs.PROJECT_OPTION]: + self.tags[argv[4]] = argv[6] # tmux set-option -w -t