diff --git a/CHANGELOG.md b/CHANGELOG.md index ff817c6f..881899b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,6 +160,18 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Fixed +- **`cleanup` no longer reports a surviving ctl window as removed (#435).** Killing a window is + best-effort and reports nothing, so the prune counted every _attempted_ kill as a removal. It now + verifies with one liveness listing and partitions into removed / survived / unverifiable. + `ctl_windows.removed` means _verifiably gone_ and gains `survived` / `unverifiable` siblings, so + `CLEANUP_SCHEMA_VERSION` is **2**; text mode names the two non-removed arms on stderr, still at + exit 0. `sessions.removed` is untouched and still an attempted kill. +- **A crashed version probe is distinguishable from "reports no version" (#428).** A binary on + `PATH` that dies answering `-V` — corrupt install, AV-blocked exe, hung server — collapsed to the + same `None` a quiet binary returns, and its stderr was gone. `version()` keeps that contract, but + a new `version_error()` seam accessor carries the dropped diagnostic, and `bmad-loop mux` prints + it as a whitespace-collapsed `warning:` line on stderr below the table — the `-` in the VERSION + column cannot say which of the two happened. - **A native-Windows install driven from a WSL shell now says so (#332).** WSL appends the Windows `PATH` to its own, so a bash prompt can reach a Windows-installed `bmad-loop`: that interpreter reports `win32`, takes the psmux platform default, and never sees the distro's tmux — while diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 2ee41089..fb722732 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -171,7 +171,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Each run drives agents in a dedicated `bmad-loop-` session; `attach` to watch live. - Auto-teardown on finish (`cleanup_session_on_finish`, disable to inspect); a hard `stop` always kills it, a graceful `stop --graceful` tears it down under the same `cleanup_session_on_finish` gate a normal finish uses; paused/interrupted runs keep the session for `resume`. - `bmad-loop cleanup` (or `c` in the TUI) sweeps leftover sessions/windows for finished/stopped/orphaned runs **of the current project**; live runs, and anything belonging to another project, are never touched. -- `--json` emits a stable machine-readable document per the [contract below](#machine-readable-output---json) (schema-versioned; the run ids whose sessions were removed, the live ids left alone, the ctl windows closed, and a `dry_run` flag) instead of the text. Plan and outcome share one schema — same fields, same meanings, with `dry_run` saying which one you are holding — so a script can pre-flight a sweep and compare it against what actually happened. (Values are each invocation's own sample, not a promise the two agree: a live session can die between the preview and the real run.) The unverifiable-pid warning, which text mode writes to stderr, becomes `sessions.unverifiable_pid` in the document, leaving stderr empty. +- `--json` emits a stable machine-readable document per the [contract below](#machine-readable-output---json) (schema-versioned; the run ids whose sessions were removed, the live ids left alone, the ctl windows closed, and a `dry_run` flag) instead of the text. `ctl_windows` is a three-way partition — `removed` (verified gone after the kill; under `--dry-run` it is the would-close plan), `survived` (still listed) and `unverifiable` (the liveness listing itself failed) — because killing a window is best-effort and reports nothing; a survivor is retried by the next `cleanup`, and text mode marks the stdout count and names both non-removed arms on stderr rather than counting them as removed. Exit stays 0 either way — the verdict is the text/document, not the code. `sessions.removed` keeps its older, weaker meaning: an attempted kill. Plan and outcome share one schema — same fields, same meanings, with `dry_run` saying which one you are holding — so a script can pre-flight a sweep and compare it against what actually happened. (Values are each invocation's own sample, not a promise the two agree: a live session can die between the preview and the real run.) The unverifiable-pid warning, which text mode writes to stderr, becomes `sessions.unverifiable_pid` in the document, leaving stderr empty. ### Disk reclamation (`[cleanup]`) @@ -191,7 +191,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - `bmad-loop init` — install skills, hooks, policy, gitignore. - `bmad-loop validate` — preflight all prerequisites. `--json` instead emits a stable machine-readable document (schema-versioned; the `ok` verdict, the queue `mode`/`spec_folder`, per-severity `counts`, and every check as a flat emission-ordered finding with a stable `check` id, `severity`, human `message` and structured `detail`) per the [contract below](#machine-readable-output---json); a failing check still emits the whole document, at exit 1 — the nonzero code is the verdict, not a failure to produce one. -- `bmad-loop mux` — list registered terminal-multiplexer backends (platform · availability · version · which is selected and why); `mux set ` persists a machine-scoped choice into policy.toml (`--clear` reverts to auto, `--force` allows a name only registered on the target machine). Bundled backend: `tmux`; external backends (e.g. the herdr adapter) register via the `bmad_loop.mux_backends` entry-point group — see [Terminal multiplexer backends](multiplexer-backends.md). +- `bmad-loop mux` — list registered terminal-multiplexer backends (platform · availability · version · which is selected and why; a backend whose binary is present but crashed the version probe gets a `warning:` on stderr carrying the probe's own failure, since the `-` in the VERSION column cannot tell that apart from a binary that reports no version); `mux set ` persists a machine-scoped choice into policy.toml (`--clear` reverts to auto, `--force` allows a name only registered on the target machine). Bundled backend: `tmux`; external backends (e.g. the herdr adapter) register via the `bmad_loop.mux_backends` entry-point group — see [Terminal multiplexer backends](multiplexer-backends.md). - `bmad-loop run` — drive the dev → review → verify → commit loop. - `bmad-loop sweep` — triage + execute open deferred-work entries. - `bmad-loop resume ` — continue a paused/interrupted run. diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index fe8c0286..84de9790 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -190,7 +190,13 @@ def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]] probe, so a sentinel is safe). A ``window_id`` column carries the same id form :meth:`current_window_id` - returns; core compares the two directly.""" + AND :meth:`list_window_ids` return; core compares all three directly. The + second pairing is load-bearing for the ctl-window prune's kill verdict, + which is a membership test of this column against that listing + (:func:`bmad_loop.tui.launch.prune_ctl_windows`): a backend that + qualifies one side and not the other reports every killed window as + verifiably gone — silently, and in the optimistic direction the verdict + exists to remove (#435).""" @abstractmethod def window_alive(self, session: str, window_id: str) -> bool: @@ -330,6 +336,27 @@ def version(self) -> str | None: folding backend keeps the identifying version in the first segment.""" return None + def version_error(self) -> str | None: + """Why the most recent :meth:`version` call answered None despite the + binary being there — a crashing probe, a hung server, an AV-blocked exe. + None when that call succeeded, when there was no binary to ask, when no + probe has run yet, or when the backend keeps no such record (the default + here, so an out-of-tree backend inherits silence rather than breaking). + + This is a *diagnostic*, not a second contract: `version()` keeps its None + sentinel (observation may degrade) and this only recovers the identity of + the failure it dropped, which is otherwise indistinguishable from "the + binary reports no version" (#428). Must not raise. + + It describes the LAST probe, so read it directly after :meth:`version`, + **on an instance you own** — nothing recomputes it, a later successful + probe clears it, and the record is unsynchronized per-instance state. The + process-wide :func:`get_multiplexer` backend is shared across the TUI's + worker threads, so a caller reading the accessor off THAT instance can be + handed another thread's probe. :func:`detect_multiplexers` is the one + in-tree reader and builds its own instance per row.""" + return None + def window_pane_pids(self, target: str) -> list[int]: """Best-effort OS pids of ``target``'s pane root processes, for the kill escalation. Not abstract: backends that can't (or don't) report pids @@ -638,6 +665,10 @@ class MuxBackendInfo: version: str | None selected: bool reason: str # "" unless selected: env | policy | platform-default | first-match | fallback + # The diagnostic version() dropped, when it answered None with the binary + # present (TerminalMultiplexer.version_error). Defaulted so it is additive + # for anyone constructing this row positionally. + version_error: str | None = None def detect_multiplexers() -> list[MuxBackendInfo]: @@ -666,6 +697,7 @@ def detect_multiplexers() -> list[MuxBackendInfo]: except Exception: matches_platform = False version: str | None = None + version_error: str | None = None try: backend = factory() available = _usable(backend) @@ -679,6 +711,14 @@ def detect_multiplexers() -> list[MuxBackendInfo]: version = fold_version(backend.version()) except Exception: version = None + if version is None: + # Read only after version(), which is what it describes, and + # only when there is a None to explain. Guarded like every other + # probe here — this function never raises. + try: + version_error = backend.version_error() + except Exception: + version_error = None selected = name == selected_name rows.append( MuxBackendInfo( @@ -688,6 +728,7 @@ def detect_multiplexers() -> list[MuxBackendInfo]: version=version, selected=selected, reason=reason if selected else "", + version_error=version_error, ) ) return rows diff --git a/src/bmad_loop/adapters/tmux_base.py b/src/bmad_loop/adapters/tmux_base.py index 399e2516..21ff7404 100644 --- a/src/bmad_loop/adapters/tmux_base.py +++ b/src/bmad_loop/adapters/tmux_base.py @@ -63,6 +63,15 @@ class BaseTmuxBackend(TerminalMultiplexer): #: the default strict handler; a Windows leaf sets ``"backslashreplace"`` so #: a stray non-UTF-8 byte degrades visibly instead of raising mid-capture. _ERRORS: str | None = None + #: Diagnostic from the last :meth:`version` probe (see + #: :meth:`TerminalMultiplexer.version_error`). A class-level default so an + #: instance that never probed answers None instead of AttributeError. + #: Per-instance and unsynchronized: only a caller that OWNS the instance may + #: read it back (``detect_multiplexers`` builds one per row). The + #: ``get_multiplexer()`` singleton is shared across the TUI's worker threads, + #: and ``mux_usable`` probes ``version()`` on it — a reader there can be + #: handed another thread's failure. + _version_error: str | None = None def _run( self, @@ -456,11 +465,29 @@ def available(self) -> bool: return shutil.which(self._BINARY) is not None def version(self) -> str | None: + # Every exit path rewrites the diagnostic, so it always describes THIS + # call (the seam's read-it-after-version rule) — a probe that recovers + # must not leave the old failure standing for `mux` to warn about. + self._version_error = None if not shutil.which(self._BINARY): return None try: raw = self._tmux("-V") - except (MultiplexerError, subprocess.SubprocessError, OSError): + # UnicodeError is in the list because _run decodes with the LOCALE codec + # and the strict handler on POSIX (_ENCODING/_ERRORS are None there; + # the Windows leaf sets utf-8/backslashreplace, so this arm is POSIX- + # only). A byte that codec cannot decode — UTF-8 in practice under PEP + # 538/540 — raises: a corrupt install, or a binary emitting text in + # another encoding, exactly what this diagnostic exists for. It is a + # ValueError, outside the SubprocessError/OSError family, so it escaped + # as a raw crash for every caller above to guard. + except (MultiplexerError, subprocess.SubprocessError, OSError, UnicodeError) as exc: + # None stays the seam's answer, but the identity of the failure is + # what separates a crashing binary from one that reports no version + # (#428). On the nonzero-exit arm _run has already folded the probe's + # stderr into the TmuxError text, so str(exc) carries it; the other + # arms carry only the failure itself, which is all there is to carry. + self._version_error = str(exc) return None # The seam promises one line (TerminalMultiplexer.version). `-V` is one # line on tmux, two on psmux (a `tmux X.Y.Z` compat line then its own), @@ -469,3 +496,6 @@ def version(self) -> str | None: # parses the compat segment with an anchored match, so the first # segment must stay first. return fold_version(raw) + + def version_error(self) -> str | None: + return self._version_error diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 64b02b70..f4e14762 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -623,6 +623,18 @@ def cmd_mux(args: argparse.Namespace) -> int: "note: AVAILABLE means the binary answers here, not that the backend supports " f"{sys.platform} — {', '.join(stranded)} can only be reached by forcing the choice" ) + # A VERSION of `-` is the same cell whether the binary reports no version or + # crashed answering (#428). The row can't carry the difference — a table cell + # holds no stderr — so the dropped diagnostic is named here, beside the other + # reason a backend looks absent for no visible cause. + for r in rows: + if r.version_error: + # Whitespace-collapsed: the text carries the probe's own stderr, which + # is routinely multi-line, and a warning that spans lines reads as + # several unrelated ones. Not length-bounded like a table cell (#321) + # — nothing here sizes a column, and the diagnostic IS the payload. + detail = " ".join(r.version_error.split()) + print(f"warning: {r.name} version probe failed: {detail}", file=sys.stderr) # A failed external package is invisible in the table (it never registered), # so name it here — the one place an operator looks when a backend is missing. for ep_name, reason in sorted(external_backend_errors().items()): @@ -2650,6 +2662,7 @@ def cmd_archive(args: argparse.Namespace) -> int: def cmd_cleanup(args: argparse.Namespace) -> int: + from .adapters.multiplexer import MultiplexerError from .tui import launch # pure stdlib; no textual import project = _project(args) @@ -2663,13 +2676,36 @@ def cmd_cleanup(args: argparse.Namespace) -> int: # holds after the fact too. In JSON mode this lives in the document # instead (sessions.unverifiable_pid), leaving stderr empty. print(f"run {run_id}: engine may still be live (unverifiable pid)", file=sys.stderr) - windows = ( - launch.prunable_ctl_windows(project) if args.dry_run else launch.prune_ctl_windows(project) - ) + # The ctl-window half is raiser-side (its candidate scan probes has_session), + # and the sessions above are ALREADY killed by the time it runs. Letting the + # raise reach main()'s backstop prints an error and returns 1 with stdout + # empty — which in --json mode destroys the record of those kills, leaving a + # consumer unable to tell "killed nothing" from "killed three, lost the + # receipt". The repair succeeded; only the observation failed, and + # observation degrades. Mirrors what the TUI worker already does. + # + # dry-run kills nothing, so there is no kill outcome to partition: the + # candidate list IS the plan, and the other two arms stay empty. + try: + if args.dry_run: + windows, survived, unverifiable = launch.prunable_ctl_windows(project), [], [] + else: + windows, survived, unverifiable = launch.prune_ctl_windows(project) + except MultiplexerError as e: + # Three empty lists is the honest answer: the raise comes from the + # candidate scan, so no window was killed or even chosen. + print(f"ctl window prune failed: {e}", file=sys.stderr) + windows, survived, unverifiable = [], [], [] if args.json: machine.emit( cleanup_document( - dry_run=args.dry_run, killed=killed, live=live, unknown=unknown, windows=windows + dry_run=args.dry_run, + killed=killed, + live=live, + unknown=unknown, + windows=windows, + windows_survived=survived, + windows_unverifiable=unverifiable, ) ) return 0 @@ -2684,7 +2720,30 @@ def cmd_cleanup(args: argparse.Namespace) -> int: if live: print(f"leaving {len(live)} live session(s) untouched") return 0 - print(f"removed {len(killed)} session(s), {len(windows)} ctl window(s)") + # The count now excludes non-removals, so on stdout alone a smaller number is + # indistinguishable from a quieter sweep — and `cleanup > log` keeps only + # stdout. The marker travels with the count; the names stay on stderr, the + # unverifiable_pid precedent. + unaccounted = len(survived) + len(unverifiable) + print( + f"removed {len(killed)} session(s), {len(windows)} ctl window(s)" + + (f" ({unaccounted} not verified — see stderr)" if unaccounted else "") + ) + # Only ever printed when a kill did not verifiably land — silence on the + # normal path, and the count above now excludes these rather than counting + # them as removed (#435). Both are retried by the next cleanup. + if survived: + # Same wording as the TUI toast: one claim, one phrase, so an operator + # moving between the two surfaces is reading the same thing. + print(f"ctl window(s) still open after the kill: {', '.join(survived)}", file=sys.stderr) + if unverifiable: + # Not "killed but unverifiable": kill_window is a silent no-op on a + # transport failure, so whether the kill even reached the server is part + # of what is unknown here. + print( + f"ctl window(s) kill attempted, outcome unverifiable: {', '.join(unverifiable)}", + file=sys.stderr, + ) if live: print(f"left {len(live)} live session(s) untouched") return 0 diff --git a/src/bmad_loop/documents.py b/src/bmad_loop/documents.py index cafea8a6..2c00c172 100644 --- a/src/bmad_loop/documents.py +++ b/src/bmad_loop/documents.py @@ -365,7 +365,7 @@ def list_document(infos: list[RunInfo]) -> dict[str, object]: } -CLEANUP_SCHEMA_VERSION = 1 +CLEANUP_SCHEMA_VERSION = 2 def cleanup_document( @@ -375,6 +375,8 @@ def cleanup_document( live: list[str], unknown: set[str], windows: list[str], + windows_survived: list[str], + windows_unverifiable: list[str], ) -> dict[str, object]: """The `cleanup --json` document: the multiplexer artifacts this invocation removed, or — under ``--dry-run`` — would remove. @@ -390,6 +392,21 @@ def cleanup_document( empty. It never blocks cleanup: pruning kills the tmux session, never the engine pid. Nothing to clean up is a valid document of empty lists at exit 0, never an error. + + `ctl_windows` is a three-way partition, disjoint by window id (the values + are names): `removed` was verified gone after the kill, `survived` was still + listed, and `unverifiable` is a kill whose outcome could not be probed at + all. Schema 2 narrowed `removed` from "a kill was attempted" to "the window + is verifiably gone" (#435) — a meaning change, hence the version bump rather + than a bare field addition. Under `--dry-run` nothing is killed, so `removed` + is the would-close plan and the other two are empty — the shared + plan/outcome shape holds, with `dry_run` still the field that says which one + you are holding. + + `sessions.removed` did NOT get the same treatment and is still the pre-kill + prunable partition — an *attempted* kill, since `kill_session` is best-effort + and silent in exactly the way `kill_window` is. #435 narrowed the windows + half only; read the sessions half with that in mind. """ return { "schema_version": CLEANUP_SCHEMA_VERSION, @@ -399,7 +416,11 @@ def cleanup_document( "live": list(live), "unverifiable_pid": sorted(unknown), }, - "ctl_windows": {"removed": list(windows)}, + "ctl_windows": { + "removed": list(windows), + "survived": list(windows_survived), + "unverifiable": list(windows_unverifiable), + }, } diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index a03cc5d6..fe69d57b 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -1047,13 +1047,15 @@ def _cleanup_sessions_worker(self) -> None: # raiser-side call; on a worker thread the toast must be marshalled, and # notify() must not be called directly (see _mux_guarded — foreground only). try: - windows = launch.prune_ctl_windows(self.project) + windows, survived, unverifiable = launch.prune_ctl_windows(self.project) except MultiplexerError as e: # prune_sessions already killed the agent sessions above; surface the # ctl-window failure but keep reporting that completed work (and the # unknown-pid warning) rather than swallowing it on an early return. - self.call_from_thread(self.notify, str(e), severity="error") - windows = [] + # Named: a bare transport message next to a "removed N session(s), 0 + # window(s)" toast reads as a successful window sweep. + self.call_from_thread(self.notify, f"ctl window prune failed: {e}", severity="error") + windows, survived, unverifiable = [], [], [] if unknown: self.call_from_thread( self.notify, @@ -1061,6 +1063,25 @@ def _cleanup_sessions_worker(self) -> None: f"(may still be live): {', '.join(sorted(unknown))}", severity="warning", ) + # A kill that did not verifiably land gets its own toast rather than a + # silent subtraction from the count below (#435) — the count now reports + # only verified removals, so without this the windows would just vanish + # from the report. Kept apart because they are different claims: one is + # positive evidence the window is still there, the other is the absence + # of any evidence at all. Both are retried by the next cleanup. + if survived: + self.call_from_thread( + self.notify, + f"{len(survived)} ctl window(s) still open after the kill: {', '.join(survived)}", + severity="warning", + ) + if unverifiable: + self.call_from_thread( + self.notify, + f"{len(unverifiable)} ctl window(s) kill attempted, outcome unverifiable: " + f"{', '.join(unverifiable)}", + severity="warning", + ) self.call_from_thread( self.notify, f"removed {len(killed)} session(s), {len(windows)} window(s)", diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index 23312094..46265177 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -300,15 +300,56 @@ def prunable_ctl_windows(project: Path) -> list[str]: return [name for _, name in _ctl_window_candidates(project)] -def prune_ctl_windows(project: Path) -> list[str]: +def prune_ctl_windows(project: Path) -> tuple[list[str], list[str], list[str]]: """Close parked control-session windows whose run is no longer live; returns - the names of the windows that were closed (see _ctl_window_candidates).""" + (removed, survived, unverifiable) window names (see _ctl_window_candidates). + A three-list tuple like runs.prune_sessions, but do NOT read the arms across: + that one partitions BEFORE its kills, so its `killed` is still an attempted + kill, its `live` is "deliberately not touched" rather than "survived", and its + `unknown` is a pid question and a SUBSET of `killed`. These three are disjoint + (by window id — the values are names) and all three are about kill outcome. + + kill_window is best-effort by contract (a hang, a missing binary, and a + refused kill are all the same silent no-op), so an attempted kill is not a + removal and must not be reported as one (#435). The verdict is taken here + rather than pushed into the seam because this is the caller that both needs + it and already holds the session: kill_window(target) alone cannot verify + anything on a backend whose liveness listing is session-scoped, which is all + of them. + + ONE listing after the whole fan-out, not a probe per window: the answer is a + set membership either way, so the verdict costs one extra round trip instead + of N. A transport fault raises and nothing can be claimed there. + + Two ceilings, both deliberate: + + - The membership test pairs list_windows' `window_id` column with + list_window_ids. The seam states its symmetry rules pairwise and this pair + is stated because of THIS caller — a backend qualifying one side and not + the other reads every candidate as removed, which is #435 restored on the + optimistic side, with no error anywhere. + - `[]` is read as "the session went with its last window". The seam's `[]` + is wider than that: BaseTmuxBackend folds EVERY nonzero exit to `[]`, so a + server that errors while its windows live would report them removed. The + common cause by far is the session really being gone, and pessimism there + would invent a phantom survivor on every future sweep. Narrowing the + sentinel is a change to the engine's liveness probe, not to this function. + """ mux = get_multiplexer() - killed: list[str] = [] - for win_id, name in _ctl_window_candidates(project): + candidates = _ctl_window_candidates(project) + if not candidates: + return [], [], [] + for win_id, _name in candidates: mux.kill_window(win_id) - killed.append(name) - return killed + try: + live = set(mux.list_window_ids(CTL_SESSION)) + except MultiplexerError: + # The kills may well have landed; nothing here can say so. Claiming the + # optimistic half is exactly the bug — the next cleanup pass retries. + return [], [], [name for _win_id, name in candidates] + removed = [name for win_id, name in candidates if win_id not in live] + survived = [name for win_id, name in candidates if win_id in live] + return removed, survived, [] def _ensure_ctl_session(project: Path) -> None: diff --git a/tests/test_backend_registry.py b/tests/test_backend_registry.py index be955807..28b92127 100644 --- a/tests/test_backend_registry.py +++ b/tests/test_backend_registry.py @@ -27,9 +27,10 @@ class _Stub: """Minimal backend double for selection tests: fixed availability/version. Selection only touches available()/version(), so the full ABC is overkill.""" - def __init__(self, avail=True, version=None): + def __init__(self, avail=True, version=None, version_error=None): self._avail = avail self._version = version + self._version_error = version_error def available(self): if isinstance(self._avail, Exception): @@ -41,6 +42,11 @@ def version(self): raise self._version return self._version + def version_error(self): + if isinstance(self._version_error, Exception): + raise self._version_error + return self._version_error + def _platform_default_name(): """This host's platform-default backend name (win32 differs), so the tests @@ -356,6 +362,66 @@ def test_detect_multiplexers_version_crash_keeps_availability(fresh_registry): assert rows["verless"].selected is True and rows["verless"].reason == "first-match" +def test_detect_multiplexers_carries_the_dropped_version_diagnostic(fresh_registry): + """The row is what `bmad-loop mux` renders, so the diagnostic version() drops + has to survive the trip out of the backend (#428) — a VERSION cell of `-` + otherwise says the same thing for a crashed probe and a quiet binary.""" + fresh_registry._BUILTINS_LOADED = True + fresh_registry.register_multiplexer( + "crasher", + lambda p: p == sys.platform, + lambda: _Stub(avail=True, version=None, version_error="tmux -V failed: killed"), + ) + rows = {r.name: r for r in fresh_registry.detect_multiplexers()} + assert rows["crasher"].version is None + assert rows["crasher"].version_error == "tmux -V failed: killed" + + +def test_detect_multiplexers_carries_the_diagnostic_off_an_unavailable_backend(fresh_registry): + """The flagship #428 shape. psmux's available() gates on its own version() + (psmux_backend), so an AV-blocked or corrupt binary reads available=False — + the row that most needs an explanation is the one where the availability + verdict already failed. The read hangs off the factory `try`, not off the + availability answer, and nothing else pins that.""" + fresh_registry._BUILTINS_LOADED = True + fresh_registry.register_multiplexer( + "blocked", + lambda p: p == sys.platform, + lambda: _Stub(avail=False, version=None, version_error="psmux -V failed: [WinError 5]"), + ) + rows = {r.name: r for r in fresh_registry.detect_multiplexers()} + assert rows["blocked"].available is False + assert rows["blocked"].version_error == "psmux -V failed: [WinError 5]" + + +def test_detect_multiplexers_reads_no_diagnostic_off_a_working_version(fresh_registry): + """Only a None version has a failure to explain. Asking a backend that just + answered would report a stale error from some earlier probe — the accessor + describes the LAST call, and this one succeeded.""" + fresh_registry._BUILTINS_LOADED = True + fresh_registry.register_multiplexer( + "fine", + lambda p: p == sys.platform, + lambda: _Stub(avail=True, version="fine 1.0", version_error="stale"), + ) + rows = {r.name: r for r in fresh_registry.detect_multiplexers()} + assert rows["fine"].version == "fine 1.0" + assert rows["fine"].version_error is None + + +def test_detect_multiplexers_guards_a_raising_version_error(fresh_registry): + """The never-raises contract covers the new probe too: a backend whose + version_error() blows up must still produce a row.""" + fresh_registry._BUILTINS_LOADED = True + fresh_registry.register_multiplexer( + "rude", + lambda p: p == sys.platform, + lambda: _Stub(avail=True, version=None, version_error=RuntimeError("boom")), + ) + rows = {r.name: r for r in fresh_registry.detect_multiplexers()} + assert rows["rude"].available is True and rows["rude"].version_error is None + + # ------------------------------------------------ fold_version, directly (#321) # # The seam folds and then every inline consumer folds again defensively, so the diff --git a/tests/test_cli.py b/tests/test_cli.py index ad6e6ee0..00a289b1 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2786,7 +2786,7 @@ def test_cleanup_prunes_sessions_and_windows(tmp_path, monkeypatch, capsys): from bmad_loop.tui import launch monkeypatch.setattr(runs, "prune_sessions", lambda _proj, dry_run=False: (["fin-1"], [], set())) - monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: ["sweep-fin-1"]) + monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: (["sweep-fin-1"], [], [])) assert cli.main(["cleanup", "--project", str(tmp_path)]) == 0 assert "removed 1 session(s), 1 ctl window(s)" in capsys.readouterr().out @@ -2799,7 +2799,7 @@ def test_cleanup_warns_per_unknown_session(tmp_path, monkeypatch, capsys): monkeypatch.setattr( runs, "prune_sessions", lambda _proj, dry_run=False: (["fin-1", "odd-1"], [], {"odd-1"}) ) - monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: []) + monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: ([], [], [])) assert cli.main(["cleanup", "--project", str(tmp_path)]) == 0 captured = capsys.readouterr() @@ -2827,7 +2827,7 @@ def test_cleanup_json_dry_run_plans_without_pruning(tmp_path, monkeypatch, capsy assert doc["schema_version"] == cli.CLEANUP_SCHEMA_VERSION assert doc["dry_run"] is True assert doc["sessions"] == {"removed": ["fin-1"], "live": ["live-1"], "unverifiable_pid": []} - assert doc["ctl_windows"] == {"removed": ["sweep-fin-1"]} + assert doc["ctl_windows"] == {"removed": ["sweep-fin-1"], "survived": [], "unverifiable": []} assert dry_runs == [True] # the kill stayed suppressed @@ -2836,7 +2836,7 @@ def test_cleanup_json_real_run_reports_what_it_did(tmp_path, monkeypatch, capsys from bmad_loop.tui import launch monkeypatch.setattr(runs, "prune_sessions", lambda _proj, dry_run=False: (["fin-1"], [], set())) - monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: ["sweep-fin-1"]) + monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: (["sweep-fin-1"], [], [])) doc = machine_json(["cleanup", "--project", str(tmp_path), "--json"], capsys) @@ -2854,7 +2854,7 @@ def test_cleanup_json_carries_unverifiable_pid_with_empty_stderr(tmp_path, monke monkeypatch.setattr( runs, "prune_sessions", lambda _proj, dry_run=False: (["fin-1", "odd-1"], [], {"odd-1"}) ) - monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: []) + monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: ([], [], [])) doc = machine_json(["cleanup", "--project", str(tmp_path), "--json"], capsys) @@ -2867,13 +2867,119 @@ def test_cleanup_json_nothing_to_clean_up_is_a_valid_empty_document(tmp_path, mo from bmad_loop.tui import launch monkeypatch.setattr(runs, "prune_sessions", lambda _proj, dry_run=False: ([], [], set())) - monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: []) + monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: ([], [], [])) doc = machine_json(["cleanup", "--project", str(tmp_path), "--json"], capsys) assert doc["schema_version"] == cli.CLEANUP_SCHEMA_VERSION assert doc["sessions"] == {"removed": [], "live": [], "unverifiable_pid": []} - assert doc["ctl_windows"] == {"removed": []} + assert doc["ctl_windows"] == {"removed": [], "survived": [], "unverifiable": []} + + +def test_cleanup_json_still_emits_its_document_when_the_ctl_prune_raises( + tmp_path, monkeypatch, capsys +): + """prune_sessions has already killed the sessions by the time the ctl half + runs, and the ctl half is raiser-side. An unguarded raise reaches main()'s + backstop, which leaves stdout EMPTY — so the record of those kills is gone + and a consumer cannot tell "killed nothing" from "killed one and lost the + receipt". The repair succeeded; only the observation failed.""" + from bmad_loop import runs + from bmad_loop.adapters.multiplexer import MultiplexerError + from bmad_loop.tui import launch + + monkeypatch.setattr(runs, "prune_sessions", lambda _proj, dry_run=False: (["fin-1"], [], set())) + + def boom(_proj): + raise MultiplexerError("tmux has-session failed: server gone") + + monkeypatch.setattr(launch, "prune_ctl_windows", boom) + + # err_contains, not the strict empty-stderr default: the failure is chatter + # this command now documents, and stdout must still be the document alone. + doc = machine_json( + ["cleanup", "--project", str(tmp_path), "--json"], + capsys, + err_contains="ctl window prune failed", + ) + + assert doc["sessions"]["removed"] == ["fin-1"] # the receipt survives + assert doc["ctl_windows"] == {"removed": [], "survived": [], "unverifiable": []} + + +def test_cleanup_text_reports_a_raising_ctl_prune_without_losing_the_sessions( + tmp_path, monkeypatch, capsys +): + from bmad_loop import runs + from bmad_loop.adapters.multiplexer import MultiplexerError + from bmad_loop.tui import launch + + monkeypatch.setattr(runs, "prune_sessions", lambda _proj, dry_run=False: (["fin-1"], [], set())) + + def boom(_proj): + raise MultiplexerError("tmux has-session failed: server gone") + + monkeypatch.setattr(launch, "prune_ctl_windows", boom) + + assert cli.main(["cleanup", "--project", str(tmp_path)]) == 0 + captured = capsys.readouterr() + assert "removed 1 session(s), 0 ctl window(s)" in captured.out + assert "ctl window prune failed: tmux has-session failed" in captured.err + + +def test_cleanup_schema_version_is_2_after_removed_narrowed(tmp_path, monkeypatch, capsys): + """`ctl_windows.removed` changed meaning (attempted -> verifiably gone), which + the contract says bumps the version rather than riding an additive field.""" + from bmad_loop import runs + from bmad_loop.tui import launch + + monkeypatch.setattr(runs, "prune_sessions", lambda _proj, dry_run=False: ([], [], set())) + monkeypatch.setattr(launch, "prune_ctl_windows", lambda _proj: ([], [], [])) + + doc = machine_json(["cleanup", "--project", str(tmp_path), "--json"], capsys) + + assert doc["schema_version"] == 2 + + +def test_cleanup_json_separates_survivors_from_removals(tmp_path, monkeypatch, capsys): + from bmad_loop import runs + from bmad_loop.tui import launch + + monkeypatch.setattr(runs, "prune_sessions", lambda _proj, dry_run=False: ([], [], set())) + monkeypatch.setattr( + launch, "prune_ctl_windows", lambda _proj: (["gone-1"], ["stuck-1"], ["dunno-1"]) + ) + + doc = machine_json(["cleanup", "--project", str(tmp_path), "--json"], capsys) + + assert doc["ctl_windows"] == { + "removed": ["gone-1"], + "survived": ["stuck-1"], + "unverifiable": ["dunno-1"], + } + + +def test_cleanup_text_counts_only_verified_removals_and_names_the_rest( + tmp_path, monkeypatch, capsys +): + """The count is what an operator reads as "it worked", so a window that + survived its kill must not be in it — and must not vanish silently either.""" + from bmad_loop import runs + from bmad_loop.tui import launch + + monkeypatch.setattr(runs, "prune_sessions", lambda _proj, dry_run=False: ([], [], set())) + monkeypatch.setattr( + launch, "prune_ctl_windows", lambda _proj: (["gone-1"], ["stuck-1"], ["dunno-1"]) + ) + + assert cli.main(["cleanup", "--project", str(tmp_path)]) == 0 + captured = capsys.readouterr() + assert "removed 0 session(s), 1 ctl window(s) (2 not verified — see stderr)" in captured.out + # Bound name-to-label, not just name-present: the two arms make different + # claims (still open vs no idea), and a bare membership check passes just as + # happily when the code reports one under the other's wording. + assert "still open after the kill: stuck-1" in captured.err + assert "kill attempted, outcome unverifiable: dunno-1" in captured.err def test_resume_kills_stale_session_before_running(project, monkeypatch): @@ -4492,8 +4598,9 @@ def test_dry_run_stories_relativizes_absolute_folder(project, capsys): class _MuxStub: """Selection-surface double (available/version only — `mux` needs no more).""" - def __init__(self, avail=True, version=None): + def __init__(self, avail=True, version=None, version_error=None): self._avail, self._version = avail, version + self._version_error = version_error def available(self): return self._avail @@ -4501,6 +4608,9 @@ def available(self): def version(self): return self._version + def version_error(self): + return self._version_error + @pytest.fixture def mux_registry(monkeypatch): @@ -4648,6 +4758,73 @@ def test_mux_keeps_the_placeholder_for_a_blank_version(mux_registry, tmp_path, c assert "-" in row and "* first available platform match" in row +def test_mux_names_the_diagnostic_a_crashed_version_probe_dropped(mux_registry, tmp_path, capsys): + """VERSION `-` is the same cell for "reports no version" and "crashed being + asked" (#428). The table cannot carry the difference, so the probe's own + failure text goes to stderr — otherwise a corrupt/AV-blocked binary is + indistinguishable from a quiet one, with nothing left to diagnose it by.""" + import sys as _sys + + mux_registry.register_multiplexer( + "alpha", + lambda p: p == _sys.platform, + lambda: _MuxStub(avail=True, version_error="tmux -V failed: [WinError 5] Access is denied"), + ) + assert cli.main(["mux", "--project", str(tmp_path)]) == 0 + + captured = capsys.readouterr() + row = next(line for line in captured.out.splitlines() if line.startswith("alpha")) + assert "-" in row # the table is unchanged; the diagnostic is not a column + assert "warning: alpha version probe failed: tmux -V failed: [WinError 5]" in captured.err + + +def test_mux_keeps_a_multiline_diagnostic_on_one_warning_line(mux_registry, tmp_path, capsys): + """The text carries the probe's own stderr, which is routinely multi-line — + left raw it reads as several unrelated warnings (the #321 lesson, applied to + the diagnostic rather than the table cell). Collapsed, not truncated: the + diagnostic is the payload here, not a sized column.""" + import sys as _sys + + mux_registry.register_multiplexer( + "alpha", + lambda p: p == _sys.platform, + lambda: _MuxStub(avail=True, version_error="tmux -V failed:\n no server\n running\n"), + ) + assert cli.main(["mux", "--project", str(tmp_path)]) == 0 + + warnings = [ln for ln in capsys.readouterr().err.splitlines() if "version probe failed" in ln] + assert warnings == ["warning: alpha version probe failed: tmux -V failed: no server running"] + + +def test_mux_warns_for_an_unavailable_backend_too(mux_registry, tmp_path, capsys): + """psmux's availability probe gates on its own version(), so the AV-blocked + binary the warning exists for lands on an available=no row. Warning only for + available backends would drop the diagnostic exactly where it is needed.""" + import sys as _sys + + mux_registry.register_multiplexer( + "alpha", + lambda p: p == _sys.platform, + lambda: _MuxStub(avail=False, version_error="psmux -V failed: [WinError 5]"), + ) + assert cli.main(["mux", "--project", str(tmp_path)]) == 0 + + assert "warning: alpha version probe failed: psmux -V failed" in capsys.readouterr().err + + +def test_mux_stays_silent_when_a_backend_simply_reports_no_version(mux_registry, tmp_path, capsys): + import sys as _sys + + mux_registry.register_multiplexer( + "alpha", lambda p: p == _sys.platform, lambda: _MuxStub(avail=True, version=None) + ) + assert cli.main(["mux", "--project", str(tmp_path)]) == 0 + + # Owns one mutation: warning unconditionally rather than per version_error. + # It cannot own "the loop was deleted" — the two positive tests do that. + assert "version probe failed" not in capsys.readouterr().err + + def test_mux_omits_the_note_when_every_available_backend_matches(mux_registry, tmp_path, capsys): import sys as _sys diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index 45f13794..cc12f894 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -383,6 +383,84 @@ def test_version_is_bounded_not_just_flattened(monkeypatch): assert got.startswith("tmux 3.4 ") and got.endswith("…") +def test_version_error_records_the_probe_crash_version_swallows(monkeypatch): + """A binary on PATH that dies answering `-V` is the case version()'s None + cannot express (#428). None stays the seam's answer; the diagnostic keeps + the identity of the failure — including the probe's stderr, which _run + already folds into the error text.""" + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + monkeypatch.setattr( + tmux_base.subprocess, + "run", + lambda argv, **k: subprocess.CompletedProcess(argv, 1, stdout="", stderr="Access denied"), + ) + mux = TmuxMultiplexer() + + assert mux.version() is None + error = mux.version_error() + assert error is not None and "Access denied" in error + + +def test_version_error_records_undecodable_probe_output(monkeypatch): + """Pins the `UnicodeError` arm of version()'s catch. A strictly-decoding + _run (POSIX: _ENCODING/_ERRORS both None) raises UnicodeDecodeError out of + subprocess itself on a binary emitting an undecodable byte — a corrupt + install, the very case #428 is about — and it is a ValueError, outside the + SubprocessError/OSError family, so without the arm it escapes as a raw crash + that every guard above turns back into an unexplained None. The raise is + injected rather than decoded for real: this owns the except clause, not the + decoding (tmux_base documents that half).""" + + def boom(argv, **k): + raise UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte") + + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + monkeypatch.setattr(tmux_base.subprocess, "run", boom) + mux = TmuxMultiplexer() + + assert mux.version() is None + error = mux.version_error() + assert error is not None and "invalid start byte" in error + + +def test_version_error_is_none_when_the_binary_is_simply_absent(monkeypatch): + """Nothing was asked, so there is no failure to report — a missing binary is + already legible from AVAILABLE and must not also raise a warning.""" + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: None) + mux = TmuxMultiplexer() + + assert mux.version() is None + assert mux.version_error() is None + + +def test_version_error_never_outlives_the_probe_it_describes(monkeypatch): + """It describes the LAST call. A recovered probe that left the old failure + standing would have `mux` warning about a backend that just answered fine.""" + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + mux = TmuxMultiplexer() + monkeypatch.setattr( + tmux_base.subprocess, + "run", + lambda argv, **k: subprocess.CompletedProcess(argv, 1, stdout="", stderr="transient"), + ) + assert mux.version() is None and mux.version_error() is not None + + _version_stdout(monkeypatch, "tmux 3.4\n") + + assert mux.version() == "tmux 3.4" + assert mux.version_error() is None + + +def test_version_error_of_a_backend_that_keeps_no_record_is_none(): + """The seam default: an out-of-tree backend inherits silence rather than an + AttributeError, so the accessor is safe to call on anything registered — and + non-abstract, so adding it does not break a backend that never heard of it. + Called unbound because the body reads no state (an ABC subclass cannot be + instantiated to hold any).""" + assert "version_error" not in multiplexer.TerminalMultiplexer.__abstractmethods__ + assert multiplexer.TerminalMultiplexer.version_error(object()) is None # type: ignore[arg-type] + + def test_version_of_a_real_probe_is_never_truncated(monkeypatch): # The bound must clear the probes that actually exist by a wide margin — # otherwise it trades one unreadable cell for a useless one. diff --git a/tests/test_psmux_backend.py b/tests/test_psmux_backend.py index 1e7560a2..aa12a818 100644 --- a/tests/test_psmux_backend.py +++ b/tests/test_psmux_backend.py @@ -187,6 +187,22 @@ def test_list_window_ids_returns_session_qualified_ids(monkeypatch): assert PsmuxMultiplexer().list_window_ids("s") == ["s:@1", "s:@2"] +def test_list_windows_id_column_is_findable_in_list_window_ids(monkeypatch): + """The ctl prune's kill verdict is a set membership ACROSS these two methods + (#435): candidates carry list_windows' `window_id` column, the post-kill + liveness check answers list_window_ids. Qualify one side only and every + candidate reads as removed — the optimism the verdict exists to remove, with + no error anywhere. The live gate (test_psmux_live) pins this too but needs + Windows + psmux and is a manual CI gate; this one runs everywhere.""" + _window_fake(monkeypatch, listed="@1\t0\n@2\trun-x\n") + rows = PsmuxMultiplexer().list_windows("s", ["window_id", "window_name"]) + assert [r[0] for r in rows] == ["s:@1", "s:@2"] # guard: the column is populated + + _window_fake(monkeypatch, listed="@1\n@2\n") + live = PsmuxMultiplexer().list_window_ids("s") + assert all(row[0] in live for row in rows) + + def test_qualification_degrades_to_bare_on_colon_session(monkeypatch, tmp_path): # A `:` in the session name would split the target at the wrong colon on # replay — both methods degrade to the bare id identically (the #221 rule). diff --git a/tests/test_psmux_live.py b/tests/test_psmux_live.py index 27bb807c..82f0bea7 100644 --- a/tests/test_psmux_live.py +++ b/tests/test_psmux_live.py @@ -70,7 +70,21 @@ def test_prune_kills_only_the_owning_projects_window(tmp_path: Path, monkeypatch monkeypatch.setattr(mux, "current_window_id", lambda: None) monkeypatch.setattr(runs, "engine_alive", lambda _dir: False) - assert launch.prune_ctl_windows(proj_a) == ["run-20260726-1"] + # First with the kill suppressed: the candidate is provably still alive, + # so it must land in `survived`. This is the half that pins the id-form + # symmetry against a real server — a removal reads "gone" whether or not + # the candidate's qualified `session:@N` matches the liveness listing's + # form, but a SURVIVOR only reads "still there" when they do match, and + # psmux qualifies both sides (#254/#291). No kill is sent, so nothing + # here disturbs the verified-removal assertions below. + with monkeypatch.context() as no_kill: + no_kill.setattr(mux, "kill_window", lambda _t: None) + assert launch.prune_ctl_windows(proj_a) == ([], ["run-20260726-1"], []) + assert mux.window_alive(session, win_a) # the suppressed kill really was a no-op + + # Then for real: the kill lands, so the verdict is a verified removal + # with both other arms empty. + assert launch.prune_ctl_windows(proj_a) == (["run-20260726-1"], [], []) live = mux.list_window_ids(session) assert win_a not in live diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index d32f6480..5fb4e03c 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -2003,7 +2003,7 @@ async def test_cleanup_unknown_sessions_notifies(project, monkeypatch): monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(runs, "prune_sessions", lambda _p: (["odd-1"], [], {"odd-1"})) - monkeypatch.setattr(launch, "prune_ctl_windows", lambda _p: []) + monkeypatch.setattr(launch, "prune_ctl_windows", lambda _p: ([], [], [])) make_run(project.project, "20260611-100000-aaaa") app = BmadLoopApp(project.project) async with app.run_test() as pilot: @@ -2054,6 +2054,38 @@ def boom(_p): assert isinstance(app.screen, DashboardScreen) # worker failed soft, no crash +async def test_cleanup_warns_about_ctl_windows_that_survived_the_kill(project, monkeypatch): + # The summary counts only verified removals now (#435), so a window that + # outlived its kill would otherwise just be missing from the toast with + # nothing anywhere saying it is still there. Survived and unverifiable get + # separate toasts: one is evidence the window is still open, the other is the + # absence of evidence — merging them reports the first as the second. + from bmad_loop import runs + + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(runs, "prune_sessions", lambda _p: ([], [], set())) + monkeypatch.setattr( + launch, "prune_ctl_windows", lambda _p: (["gone-1"], ["stuck-1"], ["dunno-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 pilot.press("c") + await until(pilot, lambda: isinstance(app.screen, ConfirmModal)) + await pilot.click(await ready(pilot, "#ok")) + await until( + pilot, + lambda: any("still open after the kill: stuck-1" in m for m in notifications(app)), + ) + await until( + pilot, lambda: any("outcome unverifiable: dunno-1" in m for m in notifications(app)) + ) + await until( + pilot, lambda: any("removed 0 session(s), 1 window(s)" in m for m in notifications(app)) + ) + + async def test_resume_finished_run_refused(project, monkeypatch): monkeypatch.setattr(launch, "mux_available", lambda: True) make_run(project.project, "20260611-100000-aaaa", finished=True) diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index 48af8e22..293491fe 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -370,7 +370,21 @@ def test_start_detached_returns_window_id(fake_run, tmp_path: Path): assert launch.start_resolve_detached(tmp_path, "RID") == "@7" -def test_prune_ctl_windows(monkeypatch, tmp_path: Path): +def _ctl_prune_fake( + monkeypatch, tmp_path: Path, *, kill: str = "lands" +) -> tuple[list[list[str]], list[int]]: + """Stand a fake ctl session up for the prune; returns (kill-argv log, liveness + probe log) — the second is what proves the verdict costs ONE listing. + + Two tagged-ours orphans (`@3`, `@6`) are the candidates — two, so a wrong + one-probe-per-window implementation cannot pass the probe-count assertion. + ``kill`` picks what the + post-kill liveness listing then shows: `lands` (gone), `fails` (still there), + `unknowable` (the listing itself dies in transport), `session-gone` (empty — + the session died with its last window). Those are the prune's whole verdict + space (#435), and the listing is the only thing that distinguishes them — + `kill-window` exits 0 in all of them. + """ from bmad_loop import runs mine = runs.project_tag(tmp_path) @@ -381,14 +395,16 @@ def test_prune_ctl_windows(monkeypatch, tmp_path: Path): runs.write_pid(live) # window format is window_id\twindow_name\t@bmad_project - windows = ( - "@1\t0\t\n" # the session's initial shell — not a run window - f"@2\trun-20260101-000000-live\t{mine}\n" # live run, ours — keep - f"@3\tsweep-20260101-000000-dead\t{mine}\n" # tagged-ours orphan — kill - "@5\tsweep-20260101-000000-other\t/some/other/project\n" # another project — skip - f"@4\tresume-20260101-000000-cur\t{mine}\n" # matches, but is the current window - ) + rows = [ + ("@1", "0", ""), # the session's initial shell — not a run window + ("@2", "run-20260101-000000-live", mine), # live run, ours — keep + ("@3", "sweep-20260101-000000-dead", mine), # tagged-ours orphan — kill + ("@5", "sweep-20260101-000000-other", "/some/other/project"), # not ours — skip + ("@4", "resume-20260101-000000-cur", mine), # matches, but is the current window + ("@6", "run-20260101-000000-dead2", mine), # a SECOND orphan — kill + ] killed: list[list[str]] = [] + probes: list[int] = [] def fake(argv, **kwargs): verb = argv[1] @@ -397,7 +413,28 @@ def fake(argv, **kwargs): if verb == "display-message": # we are sitting in @4 return subprocess.CompletedProcess(argv, 0, stdout="@4\n", stderr="") if verb == "list-windows": - return subprocess.CompletedProcess(argv, 0, stdout=windows, stderr="") + if argv[-1] == "#{window_id}": # the post-kill liveness probe + # The session it asks about is half the verdict: tmux exits + # nonzero on a session it cannot find, which list_window_ids + # folds to [] — so a probe aimed at the wrong session reads every + # candidate as removed, the pre-#435 optimism restored silently. + assert argv[argv.index("-t") + 1] == f"={launch.CTL_SESSION}" + probes.append(len(killed)) + if kill == "unknowable": + raise OSError("server gone") + if kill == "session-gone": + # rc 1, not rc 0 with empty stdout: real tmux answers a + # vanished session with a nonzero exit and list_window_ids + # folds it to [] — same verdict, and the path the transport + # actually takes. + return subprocess.CompletedProcess(argv, 1, stdout="", stderr="") + gone = {a[-1] for a in killed} if kill == "lands" else set() + return subprocess.CompletedProcess( + argv, 0, stdout="\n".join(r[0] for r in rows if r[0] not in gone), stderr="" + ) + return subprocess.CompletedProcess( + argv, 0, stdout="".join("\t".join(r) + "\n" for r in rows), stderr="" + ) if verb == "kill-window": killed.append(list(argv)) return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") @@ -405,11 +442,78 @@ def fake(argv, **kwargs): monkeypatch.setenv("TMUX", "/tmp/tmux-1000/default,123,0") # we sit in a pane of @4 monkeypatch.setattr(tmux_base.subprocess, "run", fake) monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + return killed, probes - assert launch.prunable_ctl_windows(tmp_path) == ["sweep-20260101-000000-dead"] + +def test_prune_ctl_windows(monkeypatch, tmp_path: Path): + killed, probes = _ctl_prune_fake(monkeypatch, tmp_path) + + both = ["sweep-20260101-000000-dead", "run-20260101-000000-dead2"] + assert launch.prunable_ctl_windows(tmp_path) == both assert killed == [] # dry-run view kills nothing - assert launch.prune_ctl_windows(tmp_path) == ["sweep-20260101-000000-dead"] - assert killed == [["tmux", "kill-window", "-t", "@3"]] + assert probes == [] # ...and asks nothing about liveness either + assert launch.prune_ctl_windows(tmp_path) == (both, [], []) + assert killed == [ + ["tmux", "kill-window", "-t", "@3"], + ["tmux", "kill-window", "-t", "@6"], + ] + # ONE listing for BOTH windows, and only after every kill: the recorded value + # is the kill count at probe time, so a per-window implementation would read + # [1, 2] and a probe-before-kill 0. + assert probes == [2] + + +def test_prune_ctl_windows_reports_a_survivor_separately(monkeypatch, tmp_path: Path): + """kill-window is best-effort and exits 0 either way, so a window still in the + post-kill listing must land in `survived`, never in `removed` (#435) — the + whole point is that the report stops being optimistic.""" + _ctl_prune_fake(monkeypatch, tmp_path, kill="fails") + + assert launch.prune_ctl_windows(tmp_path) == ( + [], + ["sweep-20260101-000000-dead", "run-20260101-000000-dead2"], + [], + ) + + +def test_prune_ctl_windows_unprobeable_liveness_claims_nothing(monkeypatch, tmp_path: Path): + """A transport failure on the liveness listing says nothing about the kill — + it may well have landed — so the candidate is neither removed nor survived, + and the raise must not escape a prune that already fired its kills.""" + _ctl_prune_fake(monkeypatch, tmp_path, kill="unknowable") + + assert launch.prune_ctl_windows(tmp_path) == ( + [], + [], + ["sweep-20260101-000000-dead", "run-20260101-000000-dead2"], + ) + + +def test_prune_ctl_windows_reads_an_empty_listing_as_the_session_going_with_it( + monkeypatch, tmp_path: Path +): + """`[]` is the seam's "no windows", not a failed probe (only a transport fault + raises) — a ctl session that died with its last window really did take the + candidate, so pessimism here would report a phantom survivor forever.""" + _ctl_prune_fake(monkeypatch, tmp_path, kill="session-gone") + + assert launch.prune_ctl_windows(tmp_path) == ( + ["sweep-20260101-000000-dead", "run-20260101-000000-dead2"], + [], + [], + ) + + +def test_prune_ctl_windows_with_no_candidates_never_probes(monkeypatch, tmp_path: Path): + """The listing is a real round trip; a prune with nothing to kill must not + pay for it (and must not read an empty ctl session as anything at all).""" + _killed, probes = _ctl_prune_fake(monkeypatch, tmp_path) + # no runs dir for this project => every window is another project's / untagged + other = tmp_path / "elsewhere" + other.mkdir() + + assert launch.prune_ctl_windows(other) == ([], [], []) + assert probes == [] def test_prune_ctl_windows_skips_invalid_run_ids(monkeypatch, tmp_path: Path): @@ -444,6 +548,12 @@ def fake(argv, **kwargs): if verb == "display-message": # current window is none of the rows return subprocess.CompletedProcess(argv, 0, stdout="@1\n", stderr="") if verb == "list-windows": + if argv[-1] == "#{window_id}": # post-kill liveness: the kill landed + gone = {a[-1] for a in killed} + ids = [line.split("\t")[0] for line in windows.splitlines()] + return subprocess.CompletedProcess( + argv, 0, stdout="\n".join(i for i in ids if i not in gone), stderr="" + ) return subprocess.CompletedProcess(argv, 0, stdout=windows, stderr="") if verb == "kill-window": killed.append(list(argv)) @@ -454,7 +564,7 @@ def fake(argv, **kwargs): monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") assert launch.prunable_ctl_windows(tmp_path) == ["sweep-20260101-000000-dead"] - assert launch.prune_ctl_windows(tmp_path) == ["sweep-20260101-000000-dead"] + assert launch.prune_ctl_windows(tmp_path) == (["sweep-20260101-000000-dead"], [], []) assert killed == [["tmux", "kill-window", "-t", "@2"]] @@ -464,7 +574,7 @@ def fake(argv, **kwargs): # has-session reports the ctl session is gone monkeypatch.setattr(tmux_base.subprocess, "run", fake) monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") - assert launch.prune_ctl_windows(tmp_path) == [] + assert launch.prune_ctl_windows(tmp_path) == ([], [], []) def test_select_ctl_window_id_argv(fake_run):