diff --git a/CHANGELOG.md b/CHANGELOG.md index b4eb63d3..7fad34c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,6 +125,28 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **`return_attached_client` no longer claims a return that never happened (#227).** It discarded + `switch_client`'s result and answered `True` unconditionally, so a failed switch plus a failed + `-l` fallback still journaled the return and sent the sweep unattended while the human sat in + the sweep window with their terminal never handed back. `RETURN_OPTION` is now cleared only on a + real return — a failed one is left for the parked window's trailer, which is a second chance if + someone dismisses the park prompt, not a rescue for an unattended window. + + The two failures are reported apart, because they point opposite ways: a failed switch leaves + the client in this window with a human in front of it (`ATTENDED` — an attended sweep keeps + prompting, which is the fix), while a failed detach means there was no client to detach at all + (`UNREACHABLE` — the sweep goes unattended, or a `--repeat` cycle blocks forever on a prompt no + one can see). Announcing it stays tied to a real return; `UNREACHABLE` journals + `sweep-return-no-client` and prints nothing. + + `TerminalMultiplexer.detach_client` widens from `None` to `bool` to carry that, and both client + verbs now owe **effect, not dispatch**. tmux reads it off the exit code. psmux cannot — every arm + of its `detach-client` / `switch-client` exits 0 whether or not a client moved — so it measures + the session's attached-client count across the call and answers on the drop; unobservable + degrades to `False`, never a vacuous `True` (#317). That path went live with the option channel + in #310, so on Windows the return was reachable and dishonest at the same time. Out-of-tree + backends still returning `None` read as "nothing detached" — degraded, not broken. + - **Give psmux a working per-window option channel (#310).** psmux keeps one user-option scope per server and returns `''` for every `-w` read of an `@`-prefixed name, so both mechanisms riding per-window options were broken. The ctl-window project tag read the same bled value for diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 9893a524..36fe4f3b 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -95,9 +95,41 @@ seams of a full OS port are in parked-window return hop, replayed opaquely by `switch_client` and the parked trailer; concrete default = the native pane id, so most backends inherit it — override only when your ids do not resolve from another session's context, - as psmux does to emit `=session:%N`), `detach_client`, `switch_client` (with - an optional last-client fallback), `available` (is this backend usable on - the current host). + as psmux does to emit `=session:%N`), `detach_client` / `switch_client` (with + an optional last-client fallback — both answer a bool the parked-window + return path trusts: report **effect**, not that the command was dispatched, + so a backend with no real detach returns `False`. If your CLI's exit code + already means "a client moved" you are done (tmux: `detach-client` fails with + _no current client_). If it does not, **measure** — psmux is the worked + example: every arm of its `detach-client` / `switch-client` exits 0 whether or + not a client moved, so the backend counts the session's attached clients + across the call and answers on the drop. Match the measure to what the verb is + meant to change, and record what it cannot see: that count reaches a detach, + and reaches the switch the return path normally performs — the recorded target + is a pane in the session the client came _from_, so a real switch leaves this + session — but a switch _within_ this session moves the client between windows + without moving the count, and so reads as no effect. Failing that way is + conformant: the caller keeps prompting, which is the safe direction. psmux + carries exactly that blind spot, in its `Residue:` note. Where even a measure + is unavailable, answer `False` and record the gap in your degradation ledger; + a vacuous `True` is the one answer that strands a human), + `available` (is this backend usable on the current host). + + The caller's two failures are not symmetric, which is what makes the rule + worth the round-trip. A failed `switch_client` is positive evidence: the + client is still in the window it was already in, so someone may well be + watching. A failed `detach_client` is not evidence of anything — `False` there + means only "no verified hand-back", and covers all three of nothing was + attached (tmux says so outright), the effect could not be observed, and the + backend has no detach verb at all (herdr, whose client only a manual chord + releases). `tui.launch.return_attached_client` reports the two as `ATTENDED` + and `UNREACHABLE`, and an attended sweep keeps prompting on the first but goes + unattended on the second. `UNREACHABLE` is therefore the caller's _policy_ for + an unverified detach rather than a claim the window is empty: assuming a human + is still there costs a `--repeat` cycle blocked forever on `input()` in a + window nobody may be viewing, which is the worse way to be wrong. What no + backend may do is answer `True` when nothing happened — that collapses both + failures into "the human has their terminal back", which is #227. **Window targets.** The target-taking methods (`kill_window`, `select_window`, the window-option trio, `attach_target_argv`, `switch_client`) receive one of two @@ -156,7 +188,10 @@ tee — runs a per-window **poller** thread that snapshots `pane read` into the whenever the content changes, which is exactly enough to drive the two log consumers a tmux tee would (`generic._log_activity_key`'s stall re-arm and `probe`'s marker discovery). Its module docstring is a **degradation ledger** of every such -divergence (sidecar options, poller `pipe_pane`, no-op `detach_client`, the attach +divergence (sidecar options, poller `pipe_pane`, the no-op `detach_client` — +which the widened seam now requires to answer `False`, not `None`, so the +parked-return path reads it as `UNREACHABLE` and an attended sweep stops +prompting into a window whose client only a manual chord can release — the attach argv, the advisory geometry, the protocol-version policy) — the reference for what "implement fresh" costs when the host has no tmux-shaped CLI. The operator-facing view — what a herdr _user_ notices and does — is diff --git a/docs/tui-guide.md b/docs/tui-guide.md index 486120d1..dacb4668 100644 --- a/docs/tui-guide.md +++ b/docs/tui-guide.md @@ -481,7 +481,9 @@ toast. Then: 3. Detach with `ctrl-b d`. (On tmux this step is belt-and-suspenders — answering already hands your terminal back. On backends without a detach verb — e.g. the herdr adapter — the hand-back cannot be automatic: press its detach - chord, `ctrl+b q` on herdr.) + chord, `ctrl+b q` on herdr. Either way the sweep knows it can no longer reach + you and goes unattended for the rest of the run, so detaching first never + leaves it waiting on a prompt you can't see.) The banner clears on the next poll after the sweep journals anything further (the answer is recorded as a `decision:` line in `deferred-work.md`). Sweeps diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index 7aaddf4e..44005226 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -277,16 +277,27 @@ def current_return_target(self) -> str | None: return self.current_pane_id() or None @abstractmethod - def detach_client(self) -> None: - """Detach the client viewing the current session (best-effort: a no-op on - a transport failure).""" + def detach_client(self) -> bool: + """Detach the client viewing the current session. Returns True iff a + client was actually detached — **effect, not dispatch**: a transport + failure answers False, and so does a backend with no real detach. + tmux gets this from the exit code (`detach-client` fails with "no + current client"); a backend whose CLI exits 0 either way measures it + instead (psmux counts the session's attached clients across the call). + Callers that only want the terminal handed back may ignore the answer; + the parked-window return path cannot — it clears its return option on a + True, and a vacuous one strands the human, while a False is positive + evidence that nobody is watching this window any more (see + tui.launch.return_attached_client).""" @abstractmethod def switch_client(self, target: str, last_fallback: bool = False) -> bool: """Switch the current client to ``target`` (optionally falling back to - the last client on failure). Returns True iff a switch happened — so a - transport failure returns False. ``target`` is a :meth:`target` token - or a backend-native id.""" + the last client on failure). Returns True iff a switch happened — the + same effect-not-dispatch rule as :meth:`detach_client`, so a transport + failure returns False and a backend whose CLI cannot report the move + measures it or answers False. ``target`` is a :meth:`target` token or a + backend-native id.""" @abstractmethod def available(self) -> bool: diff --git a/src/bmad_loop/adapters/psmux_backend.py b/src/bmad_loop/adapters/psmux_backend.py index aecadf47..8a501f32 100644 --- a/src/bmad_loop/adapters/psmux_backend.py +++ b/src/bmad_loop/adapters/psmux_backend.py @@ -25,7 +25,11 @@ the window-option verbs, the ``@``-prefixed columns of ``list_windows`` and the parked trailer route through a substitute channel — see the ``per-window option channel (#310)`` block below for the model and its -rules. ``available()`` additionally gates on +rules. ``detach-client`` and ``switch-client`` report dispatch rather than +effect — every arm exits 0 whether or not a client moved — so both seam +booleans are measured against the session's attached-client count instead of +read off the exit code; see the ``client verbs: observed effect (#317)`` +block. ``available()`` additionally gates on the reported version: psmux releases up to 3.3.6 kill recycled PIDs during pane/session teardown without a process-identity check, which can take down an unrelated long-lived process mid-run. The psmux behaviors cited in this @@ -721,6 +725,85 @@ def current_return_target(self) -> str | None: return pane return self.target(session, pane) + # ------------------------------- client verbs: observed effect (#317) + # + # psmux's client verbs report *dispatch*, not effect. At ``v3.3.7`` both CLI + # arms end in ``send_control(cmd)?; return Ok(())``, so the only nonzero exit + # is an unreachable session server: ``detach-client`` succeeds with zero + # clients attached (and a flag-less detach is promoted server-side to + # detach-all), and *no* form of ``switch-client`` — ``-t`` included — + # carries a server reply. Later builds narrow this but do not close it: a + # response path landed after the tag and only for ``-t``, leaving ``-l`` + # exit-0 regardless. Taking those exit codes as the seam's booleans is the + # rc-0 no-op (#228) reached through Python instead of a shell fallback, and + # it is what ``tui.launch.return_attached_client`` would consume to decide a + # human has been handed their terminal back. + # + # So the exit code is discarded and the effect is measured: count the + # clients attached to this session before and after the verb, and answer on + # the DELTA. Never on an absolute count — a ``-t`` read against a session + # whose server is gone answers from whichever server the fallback picks, so + # "zero attached" on its own proves nothing (#315). A drop cannot be + # manufactured that way: it needs two successful reads of the same session, + # the first of them nonzero. + # + # Unobservable answers False, never a vacuous True. For the detach that is + # safe in both directions — the caller's UNREACHABLE and RETURNED agree that + # nobody is left at this terminal, and the only cost is the parked trailer + # re-issuing a detach that no-ops. For the switch it is also the truth + # today: the switch leg is inert at 3.3.7 (psmux/psmux#483), so no client + # ever moves. When upstream lands that fix the probe reports it without a + # code change here — which is why this measures rather than hardcoding. + # + # Residue: a switch whose target pane lives in THIS session moves the client + # between windows without changing the session's attached count, so it reads + # as no effect. Reachable only when the return target was recorded from + # inside a control-session window; the caller then keeps prompting, which is + # the safe direction. + + def _attached_clients(self, session: str) -> int | None: + """Clients attached to ``session``, or None when psmux cannot say. + + Self-detecting on purpose: an unsupported format field cannot answer a + plausible integer, so a psmux build that does not carry + ``#{session_attached}`` degrades to None instead of a wrong count. + """ + try: + proc = self._run( + ["display-message", "-p", "-t", session, "#{session_attached}"], check=False + ) + except (subprocess.SubprocessError, OSError): + return None + if proc.returncode != 0: + return None + text = proc.stdout.strip() + return int(text) if text.isdigit() else None + + def _client_left(self, verb: list[str]) -> bool: + """Run a client verb and answer whether a client left this session.""" + session = self.current_session() + if not session: + # Not inside a pane (or the probe failed): there is no "this + # session" to measure against, and no client of ours to move. + return False + before = self._attached_clients(session) + try: + self._run(verb, check=False) + except (subprocess.SubprocessError, OSError): + return False + after = self._attached_clients(session) + if before is None or after is None: + return False + return before > 0 and after < before + + def detach_client(self) -> bool: + return self._client_left(["detach-client"]) + + def switch_client(self, target: str, last_fallback: bool = False) -> bool: + if self._client_left(["switch-client", "-t", target]): + return True + return last_fallback and self._client_left(["switch-client", "-l"]) + def pipe_pane(self, window_id: str, log_file: Path) -> None: # The base's POSIX `cat >>` sink assumes a POSIX host shell, and psmux # strips every dash-flag token from the piped command before spawning it diff --git a/src/bmad_loop/adapters/tmux_base.py b/src/bmad_loop/adapters/tmux_base.py index dcfc2225..3ddca43d 100644 --- a/src/bmad_loop/adapters/tmux_base.py +++ b/src/bmad_loop/adapters/tmux_base.py @@ -429,11 +429,14 @@ def _display_message(self, fmt: str) -> str | None: return None return proc.stdout.strip() if proc.returncode == 0 else None - def detach_client(self) -> None: + def detach_client(self) -> bool: + # Returns True iff a client was detached; a transport failure didn't + # detach anything, so False is the honest answer. try: - self._run(["detach-client"], check=False) + proc = self._run(["detach-client"], check=False) except (subprocess.SubprocessError, OSError): - pass + return False + return proc.returncode == 0 def switch_client(self, target: str, last_fallback: bool = False) -> bool: # Returns True iff a switch happened; a transport failure didn't switch, so diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 988d57b7..41feda0a 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -1006,16 +1006,33 @@ def _return_after_decisions(self) -> None: detach a plain-shell client, switch a tmux client back to its origin. A plain foreground sweep (nobody attached, no return target) is untouched. - After a successful return we go unattended for the rest of the run: a - later --repeat cycle's input() would otherwise block forever in a window - no one is viewing. New decisions then defer via the unattended path, - recorded for `bmad-loop decisions` or the next attended sweep.""" + We then go unattended for the rest of the run: a later --repeat cycle's + input() would otherwise block forever in a window no one is viewing. New + decisions defer via the unattended path instead, recorded for + `bmad-loop decisions` or the next attended sweep. + + The trigger for that is "nobody can be relied on to answer here any + more", NOT "the hand-back succeeded" — the two come apart on a failed + return, in opposite directions. A failed switch is evidence the client + is still in this window with a human in front of it (ATTENDED: keep + prompting, which is the whole point of #227). A failed detach reports + only that no hand-back was verified — nothing attached, an effect the + backend cannot observe, or no detach verb at all — and under that + uncertainty going unattended is the outcome that does not strand a + --repeat cycle on input(); the decisions it defers stay reachable via + `bmad-loop decisions`. Only a real return is announced: UNREACHABLE + prints nothing, since there may be no one to read it.""" from .tui import launch # import-light: launch.py has no textual imports - if launch.return_attached_client(): + outcome = launch.return_attached_client() + if outcome is launch.ReturnOutcome.ATTENDED: + return + self.prompting = False + if outcome is launch.ReturnOutcome.RETURNED: self.journal.append("sweep-returned-after-decisions") self.prompter.print_fn("✓ decisions recorded — sweep continues in the background") - self.prompting = False + else: + self.journal.append("sweep-return-no-client") def _apply_decision_effect(self, decision: Decision, option: DecisionOption) -> None: ledger = self.workspace.paths.deferred_work diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index b413ebad..4ace4f2a 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -15,6 +15,7 @@ import re import subprocess import sys +from enum import StrEnum from pathlib import Path from .. import runs @@ -121,13 +122,41 @@ def in_ctl_session() -> bool: return current_session() == CTL_SESSION -def detach_client() -> None: +def detach_client() -> bool: """Detach the tmux client viewing the current session, handing the terminal - back to the user. Processes in the session keep running.""" - get_multiplexer().detach_client() - - -def return_attached_client() -> bool: + back to the user. Processes in the session keep running. Returns True iff a + client was actually detached — False both when the transport failed and when + there was nothing attached (see TerminalMultiplexer.detach_client for how + each backend establishes that).""" + return get_multiplexer().detach_client() + + +class ReturnOutcome(StrEnum): + """What return_attached_client managed to do — and, for a caller that goes + unattended on the strength of it, whether a human can still answer here. + + A plain boolean cannot carry that: "the hand-back succeeded" and "there is + still someone at this terminal" are independent, and the two failures point + opposite ways. A failed *switch* leaves the client sitting in this very + window; a failed *detach* reports no verified hand-back, which is not the + same claim and does not license the same response.""" + + RETURNED = "returned" + #: No hand-back, but a human may still be here: nothing was recorded to + #: return to (a plain foreground sweep), the backend is unusable, or the + #: switch failed with the client still in this window. The conservative + #: answer — a caller must keep talking to the terminal. + ATTENDED = "attended" + #: A hand-back was attempted and did not verifiably happen: the detach found + #: nothing attached, the effect could not be observed, or the backend has no + #: detach verb at all (herdr). A caller must not rely on anyone answering a + #: prompt in this window — a policy for the uncertainty, not a proof that + #: the window is empty (see return_attached_client for why it is the safe + #: way to be wrong). + UNREACHABLE = "unreachable" + + +def return_attached_client() -> ReturnOutcome: """Hand an attached client back to its origin *now*, mid-process — the parked-window return move (see start_detached) executed while the window's command keeps running in the background, instead of after it exits. @@ -137,23 +166,41 @@ def return_attached_client() -> bool: psmux): switch that client back there (`-l` fallback if it's gone); - RETURN_DETACH: detach the client so a blocking `tmux attach` returns; - unset/empty: nobody attached with a return target — do nothing. - The option is then cleared so the post-exit return trailer doesn't fire a - second time. Returns True iff a client was actually returned.""" + The option is cleared only on RETURNED: a real return must not make the + parked window's trailer fire a second one, a failed return is left for the + trailer to retry. That retry is a second chance, not a rescue — + new_parked_window parks on a blocking read *before* the trailer, so it runs + only once a human dismisses the park prompt, never in the unattended case. + + The two failures are not interchangeable, which is why this answers a + ReturnOutcome and not a bool. A failed switch is positive evidence that the + client is still in this window, so ATTENDED keeps the caller prompting. A + failed detach carries no such evidence in general: on tmux it does + (`detach-client` fails with "no current client"), but off tmux False also + covers an effect the backend could not observe and a detach verb it does + not have at all — herdr, whose False rather than None is exactly what the + seam's widened return type buys. UNREACHABLE is the policy for all three, + because the two ways of being wrong are not equally bad: prompting into a + window no one is viewing blocks a --repeat sweep on input() forever, while + going unattended in front of a human only defers this cycle's decisions to + `bmad-loop decisions` or the next attended sweep.""" mux = get_multiplexer() if not mux_usable(mux): - return False + return ReturnOutcome.ATTENDED win = mux.current_window_id() if win is None: - return False + return ReturnOutcome.ATTENDED ret = mux.show_window_option(win, RETURN_OPTION) if not ret: - return False + return ReturnOutcome.ATTENDED if ret == RETURN_DETACH: - mux.detach_client() + outcome = ReturnOutcome.RETURNED if mux.detach_client() else ReturnOutcome.UNREACHABLE else: - mux.switch_client(ret, last_fallback=True) - mux.unset_window_option(win, RETURN_OPTION) - return True + switched = mux.switch_client(ret, last_fallback=True) + outcome = ReturnOutcome.RETURNED if switched else ReturnOutcome.ATTENDED + if outcome is ReturnOutcome.RETURNED: + mux.unset_window_option(win, RETURN_OPTION) + return outcome def decision_pending(run_dir: Path) -> bool: diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index 7286c5b4..2c07f192 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -239,11 +239,11 @@ def test_seam_methods_never_leak_raw_subprocess_error(boom_run, tmp_path): assert mux.show_window_option("@1", "opt") == "" assert mux.switch_client("s") is False assert mux.switch_client("s", last_fallback=True) is False + assert mux.detach_client() is False assert mux.kill_window("@1") is None assert mux.select_window("@1") is None assert mux.set_window_option("@1", "opt", "val") is None assert mux.unset_window_option("@1", "opt") is None - assert mux.detach_client() is None assert mux.pipe_pane("@1", tmp_path / "log") is None assert mux.window_pane_pids("@1") == [] diff --git a/tests/test_psmux_backend.py b/tests/test_psmux_backend.py index d73a9824..a34e419e 100644 --- a/tests/test_psmux_backend.py +++ b/tests/test_psmux_backend.py @@ -1227,3 +1227,95 @@ def fake(argv, **kwargs): candidates = launch._ctl_window_candidates(tmp_path) assert candidates == [("bmad-loop-ctl:@2", "run-20260726-1")] + + +# ---------------------------------------- client verbs: observed effect (#317) + + +def _client_fake(monkeypatch, *, attached, session="ctl", verb_rc=0): + """Script the two probes the client verbs measure with. + + ``attached`` is the successive answers to ``#{session_attached}``, consumed + in call order — so a detach that worked is ["1", "0"] and psmux's rc-0 no-op + is ["1", "1"]. Every verb exits ``verb_rc`` (0 by default: on psmux the exit + code says nothing, which is the whole point). + """ + monkeypatch.setenv("TMUX", "/tmp/psmux-1000/default,123,0") # inside a pane + counts = list(attached) + calls: list[list] = [] + + def fake(argv, **kwargs): + calls.append(list(argv)) + if argv[1] == "display-message" and argv[-1] == "#{session_name}": + return subprocess.CompletedProcess(argv, 0, stdout=f"{session}\n", stderr="") + if argv[1] == "display-message" and argv[-1] == "#{session_attached}": + return subprocess.CompletedProcess(argv, 0, stdout=f"{counts.pop(0)}\n", stderr="") + return subprocess.CompletedProcess(argv, verb_rc, stdout="", stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + return calls + + +def test_psmux_detach_reports_the_observed_drop(monkeypatch): + calls = _client_fake(monkeypatch, attached=["1", "0"]) + assert PsmuxMultiplexer().detach_client() is True + assert ["psmux", "detach-client"] in calls + # both probes routed to the session, never left to the most-recent fallback + assert calls.count(["psmux", "display-message", "-p", "-t", "ctl", "#{session_attached}"]) == 2 + + +def test_psmux_detach_with_nothing_attached_is_false(monkeypatch): + """psmux's detach-client exits 0 with zero clients attached, so the exit + code cannot answer this — the count has to. Reading rc here is exactly the + vacuous True that strands the human (#317).""" + calls = _client_fake(monkeypatch, attached=["0", "0"]) + assert PsmuxMultiplexer().detach_client() is False + assert ["psmux", "detach-client"] in calls # attempted, just not effective + + +def test_psmux_client_verbs_never_claim_an_unobservable_effect(monkeypatch): + """A psmux build that does not carry #{session_attached} echoes something + non-numeric; that degrades to False, never to a vacuous True.""" + _client_fake(monkeypatch, attached=["#{session_attached}", "#{session_attached}"]) + assert PsmuxMultiplexer().detach_client() is False + + +def test_psmux_detach_outside_a_pane_is_false(monkeypatch): + """No TMUX, so current_session answers None: there is no session to measure + and no client of ours to move. Answer False without issuing a flag-less + detach, which psmux promotes server-side to detach-all.""" + # Scripted counts a probe COULD read, so removing the guard fails this on + # its assertions rather than on the fixture running dry. + calls = _client_fake(monkeypatch, attached=["1", "0"]) + monkeypatch.delenv("TMUX", raising=False) + assert PsmuxMultiplexer().detach_client() is False + assert not any(c[1] == "detach-client" for c in calls) + + +def test_psmux_switch_reports_the_drop(monkeypatch): + calls = _client_fake(monkeypatch, attached=["1", "0"]) + assert PsmuxMultiplexer().switch_client("ctl:%9") is True + assert ["psmux", "switch-client", "-t", "ctl:%9"] in calls + assert not any(c[2:] == ["-l"] for c in calls) # no fallback when -t moved it + + +def test_psmux_switch_falls_back_then_reports_the_drop(monkeypatch): + calls = _client_fake(monkeypatch, attached=["1", "1", "1", "0"]) + assert PsmuxMultiplexer().switch_client("ctl:%9", last_fallback=True) is True + assert ["psmux", "switch-client", "-l"] in calls + + +def test_psmux_switch_is_false_while_the_leg_is_inert(monkeypatch): + """psmux/psmux#483: the switch moves no client, and every arm still exits 0. + Both legs are attempted and both read as no effect, so the caller keeps + prompting instead of being told the human was handed their terminal back.""" + calls = _client_fake(monkeypatch, attached=["1", "1", "1", "1"]) + assert PsmuxMultiplexer().switch_client("ctl:%9", last_fallback=True) is False + assert ["psmux", "switch-client", "-t", "ctl:%9"] in calls + assert ["psmux", "switch-client", "-l"] in calls + + +def test_psmux_switch_without_fallback_does_not_attempt_it(monkeypatch): + calls = _client_fake(monkeypatch, attached=["1", "1"]) + assert PsmuxMultiplexer().switch_client("ctl:%9") is False + assert not any(c[1:3] == ["switch-client", "-l"] for c in calls) diff --git a/tests/test_sweep.py b/tests/test_sweep.py index b77f97a0..8e0d1303 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -36,6 +36,7 @@ SweepPolicy, ) from bmad_loop.sweep import DecisionPrompter, SweepEngine, validate_migration, validate_triage +from bmad_loop.tui import launch from bmad_loop.verify import worktree_clean QUIET = NotifyPolicy(desktop=False, file=True) @@ -831,16 +832,17 @@ def _close_decision_plan(): ) -def test_interactive_decisions_return_client_goes_unattended(project, monkeypatch): - """When a client was attached to answer, the sweep hands the terminal back - after the decisions and goes unattended so later cycles don't block on a - detached window.""" - returned: list[bool] = [] +def _stub_return(monkeypatch, outcome): + """Pin return_attached_client's answer and record how often it was asked.""" + asked: list[object] = [] monkeypatch.setattr( "bmad_loop.tui.launch.return_attached_client", - lambda: bool(returned.append(True)) or True, + lambda: (asked.append(outcome), outcome)[1], ) - write_ledger(project, {"DW-1": "open"}) + return asked + + +def _run_one_decision_sweep(project): engine, _adapter = make_sweep( project, [triage_effect(_close_decision_plan())], @@ -849,7 +851,17 @@ def test_interactive_decisions_return_client_goes_unattended(project, monkeypatc ) summary = engine.run() assert not summary.paused - assert returned == [True] # asked exactly once, after the decisions phase + return engine + + +def test_interactive_decisions_return_client_goes_unattended(project, monkeypatch): + """When a client was attached to answer, the sweep hands the terminal back + after the decisions and goes unattended so later cycles don't block on a + detached window.""" + asked = _stub_return(monkeypatch, launch.ReturnOutcome.RETURNED) + write_ledger(project, {"DW-1": "open"}) + engine = _run_one_decision_sweep(project) + assert len(asked) == 1 # asked exactly once, after the decisions phase assert engine.prompting is False assert '"sweep-returned-after-decisions"' in journal_text(engine) @@ -857,20 +869,27 @@ def test_interactive_decisions_return_client_goes_unattended(project, monkeypatc def test_interactive_decisions_no_attach_stays_attended(project, monkeypatch): """A plain foreground sweep (nobody attached, no return target) keeps prompting and never emits the return event.""" - monkeypatch.setattr("bmad_loop.tui.launch.return_attached_client", lambda: False) + _stub_return(monkeypatch, launch.ReturnOutcome.ATTENDED) write_ledger(project, {"DW-1": "open"}) - engine, _adapter = make_sweep( - project, - [triage_effect(_close_decision_plan())], - answers=["1"], - prompting=True, - ) - summary = engine.run() - assert not summary.paused + engine = _run_one_decision_sweep(project) assert engine.prompting is True assert '"sweep-returned-after-decisions"' not in journal_text(engine) +def test_interactive_decisions_unreachable_goes_unattended(project, monkeypatch): + """A failed detach is not the same non-return as a failed switch: it means + there was no client to hand back, so nobody can answer here any more. The + sweep must go unattended anyway — keeping `prompting` would leave a + --repeat cycle blocked on input() in a window no one is viewing — but it + must not claim a hand-back that did not happen.""" + _stub_return(monkeypatch, launch.ReturnOutcome.UNREACHABLE) + write_ledger(project, {"DW-1": "open"}) + engine = _run_one_decision_sweep(project) + assert engine.prompting is False + assert '"sweep-return-no-client"' in journal_text(engine) + assert '"sweep-returned-after-decisions"' not in journal_text(engine) + + def test_unattended_skips_decisions(project): write_ledger(project, {"DW-1": "open", "DW-2": "open"}) plan = triage_result( diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index a488aae8..ef37ad8a 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -475,9 +475,12 @@ def test_detach_client_argv(fake_run): assert fake_run.calls == [["tmux", "detach-client"]] -def _return_fake(monkeypatch, *, win="@5", option="=main:%9", switch_rc=0): +def _return_fake( + monkeypatch, *, win="@5", option="=main:%9", switch_rc=0, fallback_rc=0, detach_rc=0 +): """Script tmux for return_attached_client: display-message -> window id, - show-options -> the recorded RETURN_OPTION, switch-client -> switch_rc. + show-options -> the recorded RETURN_OPTION, switch-client -t -> switch_rc, + switch-client -l -> fallback_rc, detach-client -> detach_rc. return_attached_client runs inside a ctl window, so TMUX is set (the backend's current_window_id answers None otherwise).""" monkeypatch.setenv("TMUX", "/tmp/tmux-1000/default,123,0") @@ -492,6 +495,10 @@ def fake(argv, **kwargs): out, rc = (f"{option}\n" if option else "", 0) elif verb == "switch-client" and argv[2] == "-t": out, rc = "", switch_rc + elif verb == "switch-client" and argv[2] == "-l": + out, rc = "", fallback_rc + elif verb == "detach-client": + out, rc = "", detach_rc else: out, rc = "", 0 return subprocess.CompletedProcess(argv, rc, stdout=out, stderr="") @@ -503,7 +510,7 @@ def fake(argv, **kwargs): def test_return_attached_client_switches_to_pane(monkeypatch): calls = _return_fake(monkeypatch, option="=main:%9") - assert launch.return_attached_client() is True + assert launch.return_attached_client() is launch.ReturnOutcome.RETURNED assert ["tmux", "switch-client", "-t", "=main:%9"] in calls assert ["tmux", "set-option", "-wu", "-t", "@5", "@bmad_return_pane"] in calls assert ["tmux", "switch-client", "-l"] not in calls # no fallback when -t works @@ -512,21 +519,47 @@ def test_return_attached_client_switches_to_pane(monkeypatch): def test_return_attached_client_switch_fallback(monkeypatch): calls = _return_fake(monkeypatch, option="=main:%9", switch_rc=1) - assert launch.return_attached_client() is True + assert launch.return_attached_client() is launch.ReturnOutcome.RETURNED assert ["tmux", "switch-client", "-l"] in calls + # the fallback returned a client too, so the option is consumed — without + # this the unset could regress to primary-success-only and stay green + assert ["tmux", "set-option", "-wu", "-t", "@5", "@bmad_return_pane"] in calls + + +def test_return_attached_client_switch_fails_stays_attended(monkeypatch): + """Stale target plus no last client: the client never left this window, so + the human is still in front of it — ATTENDED, and RETURN_OPTION stays set + or the post-exit trailer loses its retry.""" + calls = _return_fake(monkeypatch, option="=main:%9", switch_rc=1, fallback_rc=1) + assert launch.return_attached_client() is launch.ReturnOutcome.ATTENDED + assert ["tmux", "switch-client", "-l"] in calls # fallback was attempted + assert not any(c[1] == "set-option" for c in calls) # option survives + + +def test_return_attached_client_detach_fails_is_unreachable(monkeypatch): + """`detach-client` fails only when there is no current client, so a failed + detach is positive evidence that nobody is watching — the opposite of a + failed switch, and NOT the same answer. RETURN_OPTION still survives.""" + calls = _return_fake(monkeypatch, option="detach", detach_rc=1) + assert launch.return_attached_client() is launch.ReturnOutcome.UNREACHABLE + assert ["tmux", "detach-client"] in calls + assert not any(c[1] == "set-option" for c in calls) def test_return_attached_client_detaches(monkeypatch): calls = _return_fake(monkeypatch, option="detach") - assert launch.return_attached_client() is True + assert launch.return_attached_client() is launch.ReturnOutcome.RETURNED assert ["tmux", "detach-client"] in calls assert ["tmux", "set-option", "-wu", "-t", "@5", "@bmad_return_pane"] in calls assert not any(c[1] == "switch-client" for c in calls) def test_return_attached_client_noop_when_unset(monkeypatch): + """No return target recorded — a plain foreground sweep. Nothing was + attempted, so nothing can be concluded about who is at the terminal: the + conservative ATTENDED, never UNREACHABLE.""" calls = _return_fake(monkeypatch, option="") - assert launch.return_attached_client() is False + assert launch.return_attached_client() is launch.ReturnOutcome.ATTENDED assert not any(c[1] in ("switch-client", "detach-client", "set-option") for c in calls) @@ -540,7 +573,7 @@ def test_return_attached_client_noop_without_tmux(monkeypatch): ran: list = [] monkeypatch.setattr(tmux_base.shutil, "which", lambda name: None) monkeypatch.setattr(tmux_base.subprocess, "run", lambda *a, **k: ran.append(a)) - assert launch.return_attached_client() is False + assert launch.return_attached_client() is launch.ReturnOutcome.ATTENDED assert ran == [] # never shells out when tmux is missing