From c93ef47fd06d76e4c8a0b7d0833015e92a30178a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Fri, 2 Oct 2026 21:54:30 +0200 Subject: [PATCH 1/3] feat(tui,adapters): warn when a reused tmux server would plant a stale state root A tmux server is long-lived, and its panes inherit the environment it was started with. A server cold-started under BMAD_LOOP_STATE_DIR=S1, then reused by a launch under S2, hands S1 to the parked engine window and every window-0 shell. The engine then writes its control plane where the launcher never looks, and the live run reads as gone (#731). This is the interim from the #731 design: detect it and say so; carrying the root explicitly is a later change. - TerminalMultiplexer.inherited_env(session, name, *, on_fault=None) is a new non-abstract query answering what a new pane will inherit: None (unknown, the seam default), the UNSET sentinel (known absent), or the value ("" included). It never raises; a failed query reports through on_fault instead of folding into unknown. - TmuxMultiplexer implements it with show-environment -t =S NAME, plus the -g fallback on an exact "unknown variable" miss. It sits on TmuxMultiplexer, not BaseTmuxBackend, so psmux keeps "unknown": its show-environment cannot see inherited values. - runs.state_root is now runs.resolve_state_root(os.environ, passwd_home), byte-identical over 2560 env combinations on Windows and Linux. The passwd home is looked up lazily and used only when HOME is absent: absent and empty HOME are different inputs. - _ensure_ctl_session compares the root a new pane would resolve with the launcher's own, after both the create and the reuse arm, and warns once per process through the TUI with a shell-quoted remedy. Unknown answers are silent, faults are reported, and the launch is never blocked. Out-of-tree backends written against today's seam are unaffected: a StubMux implementing only the released abstract set completes both arms with no warning. Known gaps: - Windows test_runs has 4 environmental symlink WinError 1314 failures. - WSL ran the 6 touched test files only (1586 passed). The rest of the Linux suite is unverified locally. - trunk check could not run locally (a broken ruff plugin); ruff, prettier and pyright were run directly. Refs #731 --- CHANGELOG.md | 4 + docs/multiplexer-backends.md | 22 +++ src/bmad_loop/adapters/multiplexer.py | 45 +++++ src/bmad_loop/adapters/tmux_backend.py | 55 +++++- src/bmad_loop/envvars.py | 9 +- src/bmad_loop/runs.py | 77 +++++++- src/bmad_loop/tui/app.py | 9 +- src/bmad_loop/tui/launch.py | 91 ++++++++++ tests/test_multiplexer.py | 144 ++++++++++++++- tests/test_runs.py | 151 ++++++++++++++++ tests/test_tui_app.py | 27 +++ tests/test_tui_launch.py | 240 ++++++++++++++++++++++++- 12 files changed, 852 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 567e79919..a2b7af02c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,10 @@ breaking changes may land in a minor release. cannot be listed, or a session left standing, is journalled and warned about. A TUI-launched resume sweeps the TUI's displaced root too (forwarded to the child), except a share-root shape older PowerShell would corrupt. +- Warn once in the TUI when its tmux control session would hand new windows a + different state root than its own (a server started under another + `BMAD_LOOP_STATE_DIR`, `XDG_STATE_HOME` or `HOME`), naming both roots and the + `tmux set-environment` remedy, instead of letting the run read as gone (#731). - Escalate an environment fault at the review-budget rescue gate instead of deferring the story as unconverged (DW-523). - Explain that unpinned result-artifact scans search only the configured artifact diff --git a/docs/multiplexer-backends.md b/docs/multiplexer-backends.md index c49df09e0..08b833ddc 100644 --- a/docs/multiplexer-backends.md +++ b/docs/multiplexer-backends.md @@ -58,6 +58,28 @@ parses and 3.2 lies a range that may run without complaint, and that nobody test psmux carries a separate version requirement of its own, for unrelated reasons — see below. The two floors are independent and neither implies the other. +### A server started under another state root + +A tmux server is long-lived, and its panes inherit the environment the _server_ started +with, not the environment of the client asking for the pane. A server started while +`BMAD_LOOP_STATE_DIR` (or `XDG_STATE_HOME`, or `HOME`) named one state root, and reused +later by a bmad-loop under another, runs every parked engine window under the first root, so +a live run reads as gone (#731). When the TUI launches into its control session it asks tmux +(`show-environment`) what a new window there would inherit, resolves the state root from +that, and warns once — naming both roots — when it differs from its own. The launch still +goes ahead; the warning detects, it does not fix. The remedy: + +- For new windows: `tmux set-environment -t =bmad-loop-ctl BMAD_LOOP_STATE_DIR `. The + session scope matters, because a session value overrides the global one; add the same + command with `-g` to cover new sessions as well, or `tmux kill-server` to start clean. +- Shells already open there, window 0 included, keep the environment they started with. They + need `export BMAD_LOOP_STATE_DIR=`, or recreating. + +psmux cannot answer the question — its `show-environment` does not report inherited values +— so there is no such warning on psmux. Its per-project registry is keyed on the state root, +so a psmux launcher under one root never reaches a server started under another through +the ordinary path. + ## psmux (native Windows, experimental) On a native-Windows host the bundled **psmux** backend is the platform default. psmux is a diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index 9e5ee5f92..6b90324dd 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -38,6 +38,7 @@ from collections.abc import Callable from dataclasses import dataclass from pathlib import Path +from typing import Final, final from .. import envvars from .entrypoints import record_load_error @@ -67,6 +68,24 @@ def parse_target(target: str) -> tuple[str, str | None] | None: return (session, window or None) +@final +class Unset: + """The type of :data:`UNSET`: a variable confirmed absent for new panes, as + answered by :meth:`TerminalMultiplexer.inherited_env`. Its own type rather + than ``""`` because absent and set-to-empty are different inputs to the + state-root cascade (an absent ``HOME`` takes the passwd fallback, an empty + one names ``/``). Compare with ``is UNSET``.""" + + __slots__ = () + + def __repr__(self) -> str: + return "UNSET" + + +#: The one :class:`Unset` instance. +UNSET: Final = Unset() + + class TerminalMultiplexer(ABC): """Transport backend for agent sessions: sessions, windows, and clients. @@ -546,6 +565,32 @@ def legacy_registries(self) -> list[TerminalMultiplexer]: ``docs/porting-to-a-new-os.md``.""" return [] + def inherited_env( + self, + session: str, + name: str, + *, + on_fault: Callable[[str], None] | None = None, + ) -> str | Unset | None: + """What a new pane in ``session`` will inherit for the environment + variable ``name``: its value (``""`` included), :data:`UNSET` when it is + confirmed absent, or ``None`` when this transport cannot tell. + + A multiplexer server is long-lived, and its panes inherit the + environment the server started with rather than the one of the client + asking for the pane, so a launcher cannot read the answer off its own + environment (#731). Must not raise. A query the backend supports but + could not complete (timeout, missing binary, an unexpected reply) + answers ``None`` **and** hands ``on_fault`` a one-line description, so a + caller can tell "tried and failed" from "cannot tell"; with no sink the + backend says nothing. + + Non-abstract, defaulting to ``None`` with nothing reported, so released + out-of-tree backends keep working unchanged. psmux keeps this default: + its ``show-environment`` reports only values set through psmux itself, + never inherited ones.""" + 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 diff --git a/src/bmad_loop/adapters/tmux_backend.py b/src/bmad_loop/adapters/tmux_backend.py index dbd3f914c..b18c227d0 100644 --- a/src/bmad_loop/adapters/tmux_backend.py +++ b/src/bmad_loop/adapters/tmux_backend.py @@ -6,7 +6,8 @@ native-Windows "psmux") can replace them wholesale. All argv construction and the single spawn primitive live in :class:`~.tmux_base.BaseTmuxBackend`; this leaf is the POSIX implementation and inherits the full contract, adding only -the POSIX launch-pid prelude to each coding-CLI window (DW-507). See +the POSIX launch-pid prelude to each coding-CLI window (DW-507) and the +``inherited_env`` query psmux must not inherit (#731). See :mod:`.multiplexer` for the contract. ``subprocess`` and ``shutil`` are imported (and re-exported) here so existing @@ -17,8 +18,10 @@ from __future__ import annotations import shutil # noqa: F401 — re-exported for callers/tests reaching the spawn seam -import subprocess # noqa: F401 — re-exported for callers/tests reaching the spawn seam +import subprocess +from collections.abc import Callable +from .multiplexer import UNSET, Unset from .tmux_base import PARKED_RETURN_DETACH # noqa: F401 — re-exported for back-compat from .tmux_base import TMUX_TIMEOUT_S # noqa: F401 — re-exported for back-compat from .tmux_base import TmuxError # noqa: F401 — re-exported for back-compat @@ -63,3 +66,51 @@ def _window_launch(self, env: dict[str, str], command: str) -> list[str]: """ *env_args, command = super()._window_launch(env, command) return [*env_args, "/bin/sh", "-c", LAUNCH_PRELUDE, "sh", command] + + def inherited_env( + self, + session: str, + name: str, + *, + on_fault: Callable[[str], None] | None = None, + ) -> str | Unset | None: + """The seam query (#731), answered by ``show-environment``: a new pane's + env is the global env overlaid with the session env, so the session + scope is asked first and the global scope only on a session miss. + + Replies, measured on tmux 3.4: ``NAME=value`` is the value (``NAME=`` + is a set-empty ``""``), ``-NAME`` is tmux's removal marker (known-unset), + and rc 1 with exactly ``unknown variable: NAME`` is a miss in that scope. Anything + else — another error such as ``no such session``, a timeout, a missing + binary, an unparseable reply — is a fault: ``None``, reported once + through ``on_fault``. + + Here and not on :class:`~.tmux_base.BaseTmuxBackend`, which would hand + it to psmux: psmux's ``show-environment`` ignores the variable name and + hides inherited values, so its replies do not mean what this parse + reads them as.""" + for scope in (["-t", f"={session}"], ["-g"]): + argv = ["show-environment", *scope, name] + try: + proc = self._run(argv, check=False) + except (subprocess.SubprocessError, OSError, UnicodeError) as exc: + return self._env_fault(argv, str(exc), on_fault) + reply = proc.stdout.removesuffix("\n") + if proc.returncode == 0: + if reply == f"-{name}": + return UNSET + if reply.startswith(f"{name}=") and "\n" not in reply: + return reply[len(name) + 1 :] + return self._env_fault(argv, f"unexpected reply {reply!r}", on_fault) + # A miss is exactly tmux's own line for exactly this name; anything + # merely containing the words (a socket path, say) is a fault. + if proc.returncode != 1 or proc.stderr.strip() != f"unknown variable: {name}": + detail = proc.stderr.strip() or f"exit {proc.returncode}" + return self._env_fault(argv, detail, on_fault) + return UNSET + + def _env_fault( + self, argv: list[str], detail: str, on_fault: Callable[[str], None] | None + ) -> None: + if on_fault is not None: + on_fault(f"{self._BINARY} {' '.join(argv)} failed: {detail}") diff --git a/src/bmad_loop/envvars.py b/src/bmad_loop/envvars.py index 606435a42..6146590b9 100644 --- a/src/bmad_loop/envvars.py +++ b/src/bmad_loop/envvars.py @@ -28,6 +28,7 @@ import math import os +from collections.abc import Mapping #: Overrides the per-session wall-clock budget, in seconds (test / E2E hook). SESSION_TIMEOUT_S = "BMAD_LOOP_SESSION_TIMEOUT_S" @@ -85,8 +86,10 @@ def process_host() -> str | None: return os.environ.get(PROCESS_HOST) -def state_dir() -> str | None: - """The overriding bmad-loop state root, or ``None`` when unset. +def state_dir(env: Mapping[str, str] | None = None) -> str | None: + """The overriding bmad-loop state root, or ``None`` when unset. Read from + ``env`` when given (:func:`runs.resolve_state_root` asks on behalf of another + process's environment), else from this process's. Verbatim like the two name readers above — :func:`runs.state_root` uses the value as the state root itself, so an operator who names a directory gets that @@ -108,4 +111,4 @@ def state_dir() -> str | None: *current directory*, so honouring it would silently root the control plane at whatever cwd the loop happened to be launched from. """ - return os.environ.get(STATE_DIR) or None + return (os.environ if env is None else env).get(STATE_DIR) or None diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 71146c3be..1c3d4e612 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -530,7 +530,69 @@ def state_root() -> Path: silent — a control plane at the cwd, or at ``/``, that the *next* process to ask resolves somewhere else. """ - override = envvars.state_dir() + return resolve_state_root(os.environ, passwd_home() if needs_passwd_home(os.environ) else None) + + +def needs_passwd_home(env: Mapping[str, str]) -> bool: + """Whether :func:`resolve_state_root` over ``env`` reaches the passwd + fallback: the POSIX cascade with no override, no usable + ``XDG_STATE_HOME`` and no ``HOME`` — the one arm where + ``os.path.expanduser("~")`` consults the passwd database. Callers ask before + looking it up, so the lookup (an NSS query, possibly a network directory) + runs exactly where it did before the cascade was extracted, and nowhere + else.""" + return ( + sys.platform != "win32" + and not envvars.state_dir(env) + and _state_base(env.get("XDG_STATE_HOME")) is None + and "HOME" not in env + ) + + +def state_root_inputs() -> tuple[str, ...]: + """The environment variables :func:`resolve_state_root` reads on this + platform, in cascade order. A process that wants to know which root + *another* process would resolve (a multiplexer pane, #731) asks for exactly + these and nothing else.""" + if sys.platform == "win32": + return (envvars.STATE_DIR, "LOCALAPPDATA", "USERPROFILE") + return (envvars.STATE_DIR, "XDG_STATE_HOME", "HOME") + + +def passwd_home() -> str | None: + """The current user's passwd home directory, or ``None`` when there is no + passwd database (Windows) or no entry for this uid. It is the one input of + the POSIX cascade that is not in the environment: ``os.path.expanduser`` + falls back to it when ``HOME`` is absent.""" + if sys.platform == "win32": + return None + try: + import pwd + except ImportError: # posixpath.expanduser's own guard: no passwd database + return None + try: + return pwd.getpwuid(os.getuid()).pw_dir + except KeyError: # no entry for this uid (bpo-10496) + return None + + +def resolve_state_root(env: Mapping[str, str], passwd_home: str | None) -> Path: + """The state root a process with environment ``env`` resolves: the + :func:`state_root` cascade over an explicit mapping, so the answer can be + computed for an environment other than this process's (a multiplexer pane + inheriting a server's env, #731). :func:`state_root` is this function over + ``os.environ``; the rules and their reasons are documented there. + + ``passwd_home`` is the passwd entry's home directory, the one cascade input + that is not an environment variable, and ``None`` when there is no entry. + It is used only when ``HOME`` is **absent** from ``env``, which is + ``posixpath.expanduser``'s rule: a present ``HOME``, empty included, is + taken as given (an empty one folds to ``/``, which :func:`_state_base` + rejects), and an absent one with no passwd entry leaves ``~`` unexpanded, + which is relative and rejected the same way. The win32 arm never reads it. + + Raises :class:`StateRootError` when no candidate answers.""" + override = envvars.state_dir(env) if override: # `os.path.isabs` on the raw string, matching `_state_base` exactly rather # than `Path.is_absolute` — the rule and its reason are stated there. @@ -544,17 +606,20 @@ def state_root() -> Path: ) return Path(override) if sys.platform == "win32": - local = _state_base(os.environ.get("LOCALAPPDATA")) + local = _state_base(env.get("LOCALAPPDATA")) if local: return local / "bmad-loop" / "state" - profile = _state_base(os.environ.get("USERPROFILE")) + profile = _state_base(env.get("USERPROFILE")) if profile: return profile / "AppData" / "Local" / "bmad-loop" / "state" else: - xdg = _state_base(os.environ.get("XDG_STATE_HOME")) + xdg = _state_base(env.get("XDG_STATE_HOME")) if xdg: return xdg / "bmad-loop" - home = _state_base(os.path.expanduser("~")) + raw = env["HOME"] if "HOME" in env else passwd_home + # posixpath.expanduser's fold: trailing separators stripped, and an + # empty result is the root. No home at all leaves "~", which is relative. + home = _state_base("~" if raw is None else raw.rstrip("/") or "/") if home: return home / ".local" / "state" / "bmad-loop" raise StateRootError( @@ -7105,7 +7170,7 @@ def rearm_event_notice( f"the recorded spec for this story ({spec}) could not be re-opened to " f"`{status}` — it is not a readable file from here{mount}, so the " "re-drive reads that same path and finds no spec there to route on", - f"Restore the recorded spec path{_redrive_status_clause(entry)} before " "resuming", + f"Restore the recorded spec path{_redrive_status_clause(entry)} before resuming", ) # No next_step, and deliberately: on this leg there is nothing to do to THIS # file. Whether anything is left to do at all is decided by the committed spec, diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index c6cd018b3..d3febe00b 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -1973,5 +1973,12 @@ def run_tui(project: Path) -> int: mux_usable() except MultiplexerError: pass - BmadLoopApp(project).run() + app = BmadLoopApp(project) + # Launch warnings (the #731 state-root check) would print into the stderr + # Textual captures, so they toast instead — for the app's run only. + launch.warn_sink = lambda message: app.notify(message, severity="warning", markup=False) + try: + app.run() + finally: + launch.warn_sink = None return 0 diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index c31226522..42559556c 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -15,17 +15,21 @@ import os import re +import shlex import stat import subprocess import sys +from collections.abc import Callable from enum import StrEnum from pathlib import Path +from .. import envvars from .. import policy as policy_mod from .. import runs from ..adapters.multiplexer import ( MultiplexerError, TerminalMultiplexer, + Unset, get_multiplexer, mux_usable, ) @@ -891,9 +895,96 @@ def _ensure_ctl_session(project: Path) -> str: mux.new_session(name, project) except MultiplexerError as e: raise LaunchError(f"multiplexer ctl-session setup failed: {e}") from e + # After both arms: a session created just now on a stale server inherits + # that server's env as surely as a reused one does. + _warn_if_stale_state_root(mux, name) return name +# Where launch-time warnings go: a callable taking the operator-facing line, or +# None for stderr. `run_tui` installs a toast here for the app's run, because +# Textual captures stderr for that whole run and a print would reach nobody. +warn_sink: Callable[[str], None] | None = None + +# Keys of the warnings already given: each is said once per process, since the +# condition it names outlives any one launch. +_WARNED: set[str] = set() +_STALE_ROOT = "stale-state-root" + + +def _warn_once(key: str, message: str) -> None: + if key in _WARNED: + return + _WARNED.add(key) + if warn_sink is None: + print(f"warning: {message}", file=sys.stderr) + else: + warn_sink(message) + + +def _warn_if_stale_state_root(mux: TerminalMultiplexer, session: str) -> None: + """Warn once when a new pane in ``session`` would resolve a different state + root than this process (#731): a multiplexer server hands its panes the env + it started with, so a server started under another root runs every parked + window there, and a live run reads as gone. + + Compares resolved roots, not raw values: each input the platform's cascade + reads is asked of the transport (``inherited_env``), the pane's root is + resolved from the answers with this process's passwd home (a server this + process can reach runs as the same user), and only a different root — or + none at all — warns. Any unknown answer makes the comparison unknown and + silent, while a query fault is reported in its own words. Never raises and + never blocks the launch: the warning detects, it does not refuse.""" + if _STALE_ROOT in _WARNED: + return + try: + own = runs.state_root() + except runs.StateRootError as exc: + _warn_once( + f"own-root:{exc}", + f"cannot check which state root {session} windows resolve: {exc}", + ) + return + + def fault(detail: str) -> None: + _warn_once( + f"fault:{detail}", + f"cannot check which state root {session} windows resolve: {detail}", + ) + + pane_env: dict[str, str] = {} + for name in runs.state_root_inputs(): + try: + value = mux.inherited_env(session, name, on_fault=fault) + except Exception as exc: # the seam says must-not-raise; a backend may still + fault(f"{type(mux).__name__}.inherited_env raised {exc!r}") + return + if value is None: + return + if not isinstance(value, Unset): + pane_env[name] = value + try: + passwd = runs.passwd_home() if runs.needs_passwd_home(pane_env) else None + pane: Path | None = runs.resolve_state_root(pane_env, passwd) + except runs.StateRootError: + pane = None + if pane == own: + return + resolved = str(pane) if pane is not None else "no usable state root" + # Quoted for the POSIX shell the operator pastes them into. + root = shlex.quote(str(own)) + _warn_once( + _STALE_ROOT, + f"new windows in {session} would resolve {resolved}, not this process's " + f"state root {own}: its multiplexer server was started under a different " + "environment, so runs launched there can read as gone (#731). For new " + f"windows: tmux set-environment -t {shlex.quote('=' + session)} " + f"{envvars.STATE_DIR} {root} " + "(add -g for new sessions, or tmux kill-server to restart clean). Shells " + f"already open there need export {envvars.STATE_DIR}={root}, or recreating.", + ) + + def cli_argv(*tail: str) -> list[str]: """`sys.executable -m bmad_loop.cli ...` — immune to PATH/venv drift inside tmux windows.""" diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index 6099e99d7..364e9fcd7 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -19,7 +19,13 @@ from bmad_loop.adapters import multiplexer, tmux_base from bmad_loop.adapters.base import SessionSpec from bmad_loop.adapters.generic import GenericAdapter -from bmad_loop.adapters.multiplexer import MultiplexerError, TerminalMultiplexer, parse_target +from bmad_loop import envvars +from bmad_loop.adapters.multiplexer import ( + UNSET, + MultiplexerError, + TerminalMultiplexer, + parse_target, +) from bmad_loop.adapters.profile import get_profile from bmad_loop.adapters.tmux_backend import TmuxMultiplexer from bmad_loop.policy import LimitsPolicy, Policy @@ -1534,7 +1540,7 @@ def test_new_window_launch_runs_the_command_under_the_panes_shell(monkeypatch, t record = tmp_path / "shell-invoked" fake_shell = tmp_path / "fake-shell" fake_shell.write_text( - "#!/bin/sh\n" f"printf '%s\\n' \"$@\" > {shlex.quote(str(record))}\n" 'exec /bin/sh "$@"\n' + f'#!/bin/sh\nprintf \'%s\\n\' "$@" > {shlex.quote(str(record))}\nexec /bin/sh "$@"\n' ) fake_shell.chmod(0o755) command = shlex.join([sys.executable, "-c", _PID_PROBE]) @@ -1689,3 +1695,137 @@ def test_parse_target_passes_native_ids_through(native): # non-"=" targets are backend-native ids: the decoder answers None and the # backend resolves them itself assert parse_target(native) is None + + +# ------------------------------------------------- inherited_env (#731) + + +class _EnvReplies: + """Scripted `show-environment` replies keyed by scope (`-t` / `-g`), recording + every argv. A scope with no script fails the test: it was not meant to be + asked.""" + + def __init__(self, **by_scope: tuple[int, str, str]): + self.by_scope = by_scope + self.calls: list[list[str]] = [] + + def __call__(self, argv, **_k): + self.calls.append(list(argv)) + assert argv[1] == "show-environment" + rc, out, err = self.by_scope[argv[2].lstrip("-")] + return subprocess.CompletedProcess(argv, rc, stdout=out, stderr=err) + + +_MISS = (1, "", "unknown variable: BMAD_LOOP_STATE_DIR\n") + + +@pytest.mark.usefixtures("force_tmux_backend") +@pytest.mark.parametrize( + ("replies", "expected", "asked"), + [ + pytest.param({"t": (0, "BMAD_LOOP_STATE_DIR=/s1\n", "")}, "/s1", ["t"], id="session-set"), + pytest.param({"t": (0, "BMAD_LOOP_STATE_DIR=\n", "")}, "", ["t"], id="set-empty"), + pytest.param( + {"t": (0, "BMAD_LOOP_STATE_DIR=/a b=c\n", "")}, "/a b=c", ["t"], id="value-with-eq" + ), + pytest.param({"t": (0, "-BMAD_LOOP_STATE_DIR\n", "")}, UNSET, ["t"], id="removed"), + pytest.param( + {"t": _MISS, "g": (0, "BMAD_LOOP_STATE_DIR=/g\n", "")}, + "/g", + ["t", "g"], + id="global-hit", + ), + pytest.param({"t": _MISS, "g": _MISS}, UNSET, ["t", "g"], id="global-miss"), + pytest.param( + {"t": _MISS, "g": (0, "-BMAD_LOOP_STATE_DIR\n", "")}, + UNSET, + ["t", "g"], + id="global-removed", + ), + ], +) +def test_tmux_inherited_env_parses_show_environment(monkeypatch, replies, expected, asked): + """The tmux replies, as measured on tmux 3.4: `NAME=value` is the value (set + empty stays `""`, never UNSET — absent and empty are different cascade + inputs), `-NAME` is the removal marker, and `unknown variable` in the session + scope falls back to the global scope, where it means known-unset. The + session is addressed exact-match. Nothing reaches `on_fault`.""" + fake = _EnvReplies(**replies) + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + faults: list[str] = [] + + got = TmuxMultiplexer().inherited_env("ctl", "BMAD_LOOP_STATE_DIR", on_fault=faults.append) + + assert got == expected and type(got) is type(expected) + assert faults == [] + scopes = {"t": ["-t", "=ctl"], "g": ["-g"]} + assert fake.calls == [ + ["tmux", "show-environment", *scopes[s], "BMAD_LOOP_STATE_DIR"] for s in asked + ] + + +@pytest.mark.usefixtures("force_tmux_backend") +@pytest.mark.parametrize( + "failure", + [ + pytest.param((1, "", "no such session: =ctl\n"), id="session-gone"), + pytest.param((1, "", ""), id="silent-nonzero"), + pytest.param( + (1, "", "no server running on /tmp/unknown variable/default\n"), + id="miss-words-inside-a-fault", + ), + pytest.param((1, "", "unknown variable: OTHER\n"), id="miss-for-another-name"), + pytest.param((0, "SOMETHING_ELSE=1\n", ""), id="unexpected-reply"), + pytest.param(subprocess.TimeoutExpired(["tmux"], 5), id="timeout"), + pytest.param(FileNotFoundError("tmux"), id="missing-binary"), + ], +) +def test_tmux_inherited_env_reports_a_failed_query_as_unknown(monkeypatch, failure): + """A query tmux could not answer is Unknown (`None`) AND one `on_fault` + call — never folded into a silent Unknown, and never raised. Without a sink + it still answers `None` and raises nothing. + + Ablation: drop the `on_fault` call from `_env_fault` and every row fails on + the fault count.""" + + def run(argv, **_k): + if isinstance(failure, BaseException): + raise failure + rc, out, err = failure + return subprocess.CompletedProcess(argv, rc, stdout=out, stderr=err) + + monkeypatch.setattr(tmux_base.subprocess, "run", run) + faults: list[str] = [] + mux = TmuxMultiplexer() + + assert mux.inherited_env("ctl", "HOME", on_fault=faults.append) is None + assert len(faults) == 1 and "show-environment" in faults[0] + assert mux.inherited_env("ctl", "HOME") is None + + +def test_psmux_inherited_env_is_unknown_and_asks_nothing(monkeypatch): + """psmux keeps the seam default: its `show-environment` ignores the name and + hides inherited values, so parsing it as tmux's would read every inherited + variable as unset. Unknown, no spawn, nothing reported. + + Ablation: move `inherited_env` from `TmuxMultiplexer` to `BaseTmuxBackend` + and this fails on the spawn.""" + from bmad_loop.adapters.psmux_backend import PsmuxMultiplexer + + def no_spawn(argv, **_k): + raise AssertionError(f"psmux inherited_env spawned {argv}") + + monkeypatch.setattr(tmux_base.subprocess, "run", no_spawn) + faults: list[str] = [] + got = PsmuxMultiplexer().inherited_env("ctl", envvars.STATE_DIR, on_fault=faults.append) + assert got is None + assert faults == [] + + +def test_inherited_env_seam_default_is_unknown_for_an_out_of_tree_backend(): + """A backend written against the released abstract set inherits Unknown and + reports nothing.""" + faults: list[str] = [] + assert StubMux().inherited_env("ctl", "HOME", on_fault=faults.append) is None + assert faults == [] + assert repr(UNSET) == "UNSET" diff --git a/tests/test_runs.py b/tests/test_runs.py index 713ea6452..7a352b41d 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -2155,6 +2155,157 @@ def test_state_root_refuses_a_home_that_cannot_root_a_control_plane(monkeypatch, runs.state_root() +_CASCADE_ARMS = [ + # (platform, env spec, expected root under tmp_path or None for a refusal) + pytest.param( + "linux", {"override": "o", "XDG_STATE_HOME": "x", "HOME": "h"}, "o", id="posix-override" + ), + pytest.param("linux", {"XDG_STATE_HOME": "x", "HOME": "h"}, "x/bmad-loop", id="posix-xdg"), + pytest.param( + "linux", + {"XDG_STATE_HOME": "rel", "HOME": "h"}, + "h/.local/state/bmad-loop", + id="posix-relative-xdg", + ), + pytest.param("linux", {"HOME": "h"}, "h/.local/state/bmad-loop", id="posix-home"), + pytest.param( + "linux", {"HOME": "h/"}, "h/.local/state/bmad-loop", id="posix-home-trailing-slash" + ), + pytest.param("linux", {"HOME": "rel"}, None, id="posix-relative-home"), + pytest.param("linux", {"override": "rel", "HOME": "h"}, None, id="posix-relative-override"), + pytest.param( + "win32", + {"override": "o", "LOCALAPPDATA": "l", "USERPROFILE": "p"}, + "o", + id="win-override", + ), + pytest.param( + "win32", {"LOCALAPPDATA": "l", "USERPROFILE": "p"}, "l/bmad-loop/state", id="win-local" + ), + pytest.param( + "win32", + {"USERPROFILE": "p", "XDG_STATE_HOME": "x"}, + "p/AppData/Local/bmad-loop/state", + id="win-profile", + ), + pytest.param("win32", {"XDG_STATE_HOME": "x", "HOME": "h"}, None, id="win-none"), +] + + +@pytest.mark.parametrize(("platform", "spec", "expected"), _CASCADE_ARMS) +def test_resolve_state_root_matches_state_root_on_every_cascade_arm( + tmp_path, monkeypatch, platform, spec, expected +): + """`state_root()` is `resolve_state_root` over this process's environment + (#731), so on every cascade arm both must give the same, literal answer — + the root or the refusal. A spec value `rel` is written relative; every other + one is an absolute path under `tmp_path` (a trailing `/` kept as spelled). + + `USERPROFILE` mirrors `HOME` on the POSIX rows, as `_fake_home` does, so a + faked-POSIX row means the same thing on a Windows host. HOME stays set on + every POSIX row, so the passwd lookup never runs and `passwd_home=None` is + the faithful argument.""" + monkeypatch.setattr(runs.sys, "platform", platform) + for name in (envvars.STATE_DIR, "XDG_STATE_HOME", "LOCALAPPDATA", "USERPROFILE", "HOME"): + monkeypatch.delenv(name, raising=False) + env: dict[str, str] = {} + for key, value in spec.items(): + name = envvars.STATE_DIR if key == "override" else key + if value == "rel": + env[name] = value + else: + env[name] = str(tmp_path / value.rstrip("/")) + ("/" if value.endswith("/") else "") + if platform == "linux": + env["USERPROFILE"] = env["HOME"] + for name, value in env.items(): + monkeypatch.setenv(name, value) + + if expected is None: + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.state_root() + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.resolve_state_root(env, None) + else: + assert runs.state_root() == tmp_path / expected + assert runs.resolve_state_root(env, None) == tmp_path / expected + + +def test_resolve_state_root_reads_the_passwd_home_only_when_home_is_absent(tmp_path, monkeypatch): + """The one cascade input that is not an environment variable: with `HOME` + absent, `expanduser("~")` falls back to the passwd entry, so the resolver + takes that home as an explicit argument (#731) and uses it on that arm only. + + - absent HOME + a passwd home: that home's root + - absent HOME + no passwd entry (`None`): refused, which is what + `state_root()` does today when `expanduser` hands back `"~"` + - `HOME=""` with a passwd home given: still refused — a present HOME is used + as given, and an empty one folds to `/` + + Ablation target: use `passwd_home` whenever HOME is falsy (`env.get("HOME") + or passwd_home`) and the `HOME=""` row fails, resolving the passwd root.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + passwd = str(tmp_path / "pw") + unusable = {"XDG_STATE_HOME": "relative"} + + assert runs.resolve_state_root(unusable, passwd) == ( + tmp_path / "pw" / ".local" / "state" / "bmad-loop" + ) + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.resolve_state_root(unusable, None) + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.resolve_state_root({**unusable, "HOME": ""}, passwd) + + +@pytest.mark.parametrize("answering", ["override", "xdg"]) +def test_state_root_skips_the_passwd_lookup_when_an_earlier_arm_answers( + tmp_path, monkeypatch, answering +): + """With HOME absent, `expanduser` would consult passwd only once the cascade + reached the HOME arm; an override or a usable XDG_STATE_HOME answers first, + and the lookup (an NSS query, possibly a network directory) never runs. + + Ablation: pass `passwd_home()` whenever HOME is absent, without + `needs_passwd_home`, and both rows fail on the raising stub.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + _fake_home(monkeypatch, tmp_path / "unused") + monkeypatch.delenv("HOME") + if answering == "override": + monkeypatch.setenv(envvars.STATE_DIR, str(tmp_path / "o")) + expected = tmp_path / "o" + else: + monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "x")) + expected = tmp_path / "x" / "bmad-loop" + + def no_lookup() -> str: + raise AssertionError("passwd consulted although an earlier arm answers") + + monkeypatch.setattr(runs, "passwd_home", no_lookup) + assert runs.state_root() == expected + + +def test_state_root_without_home_consults_the_passwd_home(tmp_path, monkeypatch): + """`state_root()` hands the resolver the passwd home only on the arm that + reads it — a POSIX env with no HOME — so the extraction keeps today's + `expanduser` fallback. Graded through a faked `passwd_home`, since the real + lookup cannot be steered from a test.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + _fake_home(monkeypatch, tmp_path / "unused") + monkeypatch.delenv("HOME") + asked: list[str] = [] + + def fake_passwd_home() -> str: + asked.append("passwd") + return str(tmp_path / "pw") + + monkeypatch.setattr(runs, "passwd_home", fake_passwd_home) + assert runs.state_root() == tmp_path / "pw" / ".local" / "state" / "bmad-loop" + assert asked == ["passwd"] + + monkeypatch.setenv("HOME", str(tmp_path / "h")) + assert runs.state_root() == tmp_path / "h" / ".local" / "state" / "bmad-loop" + assert asked == ["passwd"] # HOME present: the lookup never runs + + def test_state_dir_for_is_keyed_on_project_identity_not_spelling(tmp_path, monkeypatch): """One project reached by two spellings must key to ONE control plane. diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index 128c8ca27..8d43938dc 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -8951,6 +8951,33 @@ def run(self): assert ran == [True] +def test_run_tui_toasts_launch_warnings_for_the_app_run_only(monkeypatch, tmp_path): + """Launch warnings (the #731 state-root check) default to stderr, which + Textual captures for the app's whole run, so run_tui routes them to a + warning toast while the app runs, and restores stderr afterwards, so a + finished app is never handed a late warning.""" + from bmad_loop.tui import app as tui_app + + toasts: list[tuple[str, dict]] = [] + + class _StubApp: + def __init__(self, _project): + pass + + def notify(self, message, **kwargs): + toasts.append((message, kwargs)) + + def run(self): + assert launch.warn_sink is not None + launch.warn_sink("stale root") + + monkeypatch.setattr(tui_app, "BmadLoopApp", _StubApp) + monkeypatch.setattr(tui_app, "mux_usable", lambda: True) + assert tui_app.run_tui(tmp_path) == 0 + assert toasts == [("stale root", {"severity": "warning", "markup": False})] + assert launch.warn_sink is None + + def _write_two_triage_decisions(run_dir: Path) -> None: """A sweep triage carrying TWO decisions, so a walk has somewhere to continue to.""" import json diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index 250f60aea..d1e2d1da4 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -20,7 +20,7 @@ import pytest -from bmad_loop import runs +from bmad_loop import envvars, runs from bmad_loop.adapters import tmux_base from bmad_loop.adapters.multiplexer import MultiplexerError, get_multiplexer from bmad_loop.tui import launch @@ -38,13 +38,31 @@ class FakeRun: `new-window` just minted, which is what a real backend does — and what ctl_window_recorded re-proves the record against.""" - def __init__(self, has_session_rc: int = 1, windows: str = "@7\tresume-RID\n"): + def __init__( + self, + has_session_rc: int = 1, + windows: str = "@7\tresume-RID\n", + pane_env: dict[str, str] | None = None, + env_stderr: str | None = None, + ): self.calls: list[list[str]] = [] self.has_session_rc = has_session_rc self.windows = windows + # What `show-environment` reports a new pane inherits (#731): this + # process's own env unless scripted (a server this launcher started + # itself); `env_stderr` fails every such query instead. + self.pane_env = pane_env + self.env_stderr = env_stderr def __call__(self, argv, **kwargs): self.calls.append(list(argv)) + if argv[1] == "show-environment": + if self.env_stderr is not None: + return subprocess.CompletedProcess(argv, 1, stdout="", stderr=self.env_stderr) + name = argv[-1] + env = os.environ if self.pane_env is None else self.pane_env + out = f"{name}={env[name]}\n" if name in env else f"-{name}\n" + return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") rc = self.has_session_rc if argv[1] == "has-session" else 0 out = "" if argv[1] == "new-window": @@ -57,6 +75,14 @@ def by_verb(self, verb: str) -> list[list[str]]: return [c for c in self.calls if c[1] == verb] +@pytest.fixture(autouse=True) +def _fresh_launch_warnings(monkeypatch): + """Launch warnings are once per process: give every test a fresh latch and + the default (stderr) sink, so no test's warning is consumed by another.""" + monkeypatch.setattr(launch, "_WARNED", set()) + monkeypatch.setattr(launch, "warn_sink", None) + + @pytest.fixture def fake_run(monkeypatch) -> FakeRun: fake = FakeRun() @@ -80,17 +106,17 @@ def test_start_run_detached_argv(fake_run, tmp_path: Path): nw0 = fake_run.by_verb("new-window")[0] assert nw0[nw0.index("-F") + 1] == "#{window_id}" - # control session was missing: has-session, new-session, new-window, then - # the project tag is stamped on the new window so cross-project cleanup - # never closes it + # control session was missing: has-session, new-session, the state-root + # check asks what a new pane inherits for each cascade input (#731), + # new-window, then the project tag is stamped on the new window so + # cross-project cleanup never closes it assert [c[1] for c in fake_run.calls] == [ "has-session", "new-session", + *["show-environment"] * len(runs.state_root_inputs()), "new-window", "set-option", ] - from bmad_loop import runs - assert fake_run.by_verb("set-option")[0] == [ "tmux", "set-option", @@ -208,12 +234,14 @@ def test_existing_ctl_session_reused(monkeypatch, tmp_path: Path): monkeypatch.setattr(tmux_base.subprocess, "run", fake) monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") launch.resume_detached(tmp_path, "RID") - # No new-session: the ctl session already answered has-session. The trailing + # No new-session: the ctl session already answered has-session. A reused + # session is still asked what its new panes inherit (#731). The trailing # list-windows is resume's own check that the lookup now names the window it # minted — the one launch that mints a second window under a run id pays for # the answer it warns on. assert [c[1] for c in fake.calls] == [ "has-session", + *["show-environment"] * len(runs.state_root_inputs()), "new-window", "set-option", "list-windows", @@ -2562,3 +2590,199 @@ def test_run_captured_streams_real_subprocess(): assert rc == 0 assert "bmad-loop" in out assert err == "" + + +# ------------------------------------------- stale state-root warning (#731) + + +def _posix_env(monkeypatch, **env: str) -> dict[str, str]: + """Fake the POSIX cascade and set exactly ``env`` among its inputs for this + process, the launcher. Returns ``env`` for scripting a pane alike.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + for name in runs.state_root_inputs(): + monkeypatch.delenv(name, raising=False) + for name, value in env.items(): + monkeypatch.setenv(name, value) + return env + + +def _launch_against(monkeypatch, tmp_path: Path, fake: FakeRun) -> list[str]: + """Launch a run against ``fake``; returns what reached the warn sink.""" + warned: list[str] = [] + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + monkeypatch.setattr(launch, "warn_sink", warned.append) + launch.start_run_detached(tmp_path, "RID") + assert fake.by_verb("new-window"), "the warning must never block the launch" + return warned + + +@pytest.mark.parametrize("reuse", [True, False], ids=["reused", "created"]) +def test_stale_server_root_warns_once_after_either_arm(monkeypatch, tmp_path: Path, reuse): + """A server started under another root hands every new pane that root — + on a reused control session AND on one created just now, since a new + session on a stale server inherits its global env too. One warning names + both roots and the remedy, through the sink, once per process; the launch + goes ahead. + + Ablation: drop the `_warn_if_stale_state_root` call from + `_ensure_ctl_session` and both rows fail on the empty sink.""" + home = str(tmp_path / "home") + _posix_env(monkeypatch, **{envvars.STATE_DIR: str(tmp_path / "s2"), "HOME": home}) + pane = {envvars.STATE_DIR: str(tmp_path / "s1"), "HOME": home} + fake = FakeRun(has_session_rc=0 if reuse else 1, pane_env=pane) + + warned = _launch_against(monkeypatch, tmp_path, fake) + assert len(warned) == 1 + assert str(tmp_path / "s1") in warned[0] and str(tmp_path / "s2") in warned[0] + assert f"tmux set-environment -t ={runs.CTL_SESSION} BMAD_LOOP_STATE_DIR" in warned[0] + + queries = len(fake.by_verb("show-environment")) + launch.start_run_detached(tmp_path, "RID2") + assert len(warned) == 1 # once per process + assert len(fake.by_verb("show-environment")) == queries # and no re-asking + + +def test_warns_when_only_xdg_state_home_differs(monkeypatch, tmp_path: Path): + """Neither side sets the override, but the pane's default cascade still + lands elsewhere: the comparison is of resolved roots, from every input.""" + home = str(tmp_path / "home") + _posix_env(monkeypatch, XDG_STATE_HOME=str(tmp_path / "x2"), HOME=home) + pane = {"XDG_STATE_HOME": str(tmp_path / "x1"), "HOME": home} + + warned = _launch_against(monkeypatch, tmp_path, FakeRun(pane_env=pane)) + assert len(warned) == 1 and str(tmp_path / "x1" / "bmad-loop") in warned[0] + + +def test_silent_when_an_override_names_the_default_root(monkeypatch, tmp_path: Path): + """An override that names exactly the root the pane's default reaches is + the same root: equal resolutions never warn, whichever inputs made them. + + Ablation: compare the raw override values instead of resolved roots and + this warns.""" + xdg = str(tmp_path / "xdg") + _posix_env( + monkeypatch, + **{envvars.STATE_DIR: str(tmp_path / "xdg" / "bmad-loop"), "XDG_STATE_HOME": xdg}, + ) + pane = {"XDG_STATE_HOME": xdg} + + assert _launch_against(monkeypatch, tmp_path, FakeRun(pane_env=pane)) == [] + + +def test_warns_when_the_pane_value_is_relative(monkeypatch, tmp_path: Path): + """A relative inherited override is refused by the pane's own resolution, + so the pane cannot land on the launcher's root: a mismatch, not a crash.""" + _posix_env(monkeypatch, **{envvars.STATE_DIR: str(tmp_path / "s2")}) + pane = {envvars.STATE_DIR: "relative/state"} + + warned = _launch_against(monkeypatch, tmp_path, FakeRun(pane_env=pane)) + assert len(warned) == 1 and "no usable state root" in warned[0] + + +def test_unknown_is_silent_but_a_query_fault_is_reported(monkeypatch, tmp_path: Path): + """A failed query makes the comparison unknown: no mismatch is claimed, + but the fault itself reaches the sink rather than vanishing into silence. + + Ablation: drop the `on_fault=fault` argument in `_warn_if_stale_state_root` + and the sink is empty.""" + _posix_env(monkeypatch, **{envvars.STATE_DIR: str(tmp_path / "s2")}) + fake = FakeRun(env_stderr="no server running on /tmp/tmux-1000/default\n") + + warned = _launch_against(monkeypatch, tmp_path, fake) + assert len(warned) == 1 + assert "no server running" in warned[0] and "would resolve" not in warned[0] + + +def test_an_underivable_launcher_root_is_reported_and_still_launches(monkeypatch, tmp_path: Path): + """With no root of its own there is nothing to compare: say so through the + sink, raise nothing, and launch anyway — refusing is not this check's job.""" + _posix_env(monkeypatch, **{envvars.STATE_DIR: "relative/state"}) + + warned = _launch_against(monkeypatch, tmp_path, FakeRun()) + assert len(warned) == 1 and envvars.STATE_DIR in warned[0] + + +def test_without_a_sink_the_warning_goes_to_stderr(monkeypatch, tmp_path: Path, capsys): + """The CLI-side default: no sink installed means a `warning:` line.""" + _posix_env(monkeypatch, **{envvars.STATE_DIR: str(tmp_path / "s2")}) + pane = {envvars.STATE_DIR: str(tmp_path / "s1")} + monkeypatch.setattr(tmux_base.subprocess, "run", FakeRun(pane_env=pane)) + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + + launch.start_run_detached(tmp_path, "RID") + assert "warning: new windows in" in capsys.readouterr().err + + +def test_an_out_of_tree_backend_goes_through_both_arms_silently( + monkeypatch, tmp_path: Path, capsys +): + """A backend implementing only the released abstract set inherits the + Unknown default: `_ensure_ctl_session` completes on the create arm and on + the reuse arm, and nothing is warned on either channel.""" + from test_multiplexer import StubMux + + stub = StubMux() + warned: list[str] = [] + monkeypatch.setattr(launch, "get_multiplexer", lambda: stub) + monkeypatch.setattr(launch, "warn_sink", warned.append) + _posix_env(monkeypatch, **{envvars.STATE_DIR: str(tmp_path / "s2")}) + + name = launch._ensure_ctl_session(tmp_path) # create + assert launch._ensure_ctl_session(tmp_path) == name # reuse + assert stub.calls == ["has_session", "new_session", "has_session"] + assert stub.inherited_env(name, "HOME") is None + assert warned == [] + assert capsys.readouterr().err == "" + + +@pytest.mark.parametrize( + ("pane_home", "warns"), + [pytest.param(None, False, id="unset-takes-passwd"), pytest.param("", True, id="empty")], +) +def test_pane_home_unset_and_empty_are_different_inputs( + monkeypatch, tmp_path: Path, pane_home, warns +): + """An inherited HOME confirmed absent takes the passwd fallback, exactly as + the launcher's own absent HOME does — the same root, silent. An inherited + `HOME=""` is present and folds to `/`, which no cascade accepts — a mismatch. + Folding the two would hide the second. + + Ablation: keep only truthy answers in the pane mapping (`if value:`) and the + `empty` row stops warning.""" + _posix_env(monkeypatch) # the launcher: no override, no XDG, no HOME + monkeypatch.setattr(runs, "passwd_home", lambda: str(tmp_path / "pw")) + pane = {} if pane_home is None else {"HOME": pane_home} + + warned = _launch_against(monkeypatch, tmp_path, FakeRun(pane_env=pane)) + assert len(warned) == int(warns) + if warns: + assert "no usable state root" in warned[0] + + +def test_the_comparison_skips_the_passwd_lookup_it_does_not_need(monkeypatch, tmp_path: Path): + """Both sides resolve from an override, so neither needs the passwd entry, + and the comparison does not look it up.""" + _posix_env(monkeypatch, **{envvars.STATE_DIR: str(tmp_path / "s")}) + + def no_lookup() -> str: + raise AssertionError("passwd consulted although no side needs it") + + monkeypatch.setattr(runs, "passwd_home", no_lookup) + assert _launch_against(monkeypatch, tmp_path, FakeRun()) == [] + + +def test_the_remedy_commands_carry_the_root_through_a_shell_intact(monkeypatch, tmp_path: Path): + """The remedy is meant to be pasted into a shell, so a root with a space and + an apostrophe must survive as one word in both commands. + + Ablation: interpolate `own` unquoted and both splits break the root apart.""" + own = str(tmp_path / "o'brien state") + _posix_env(monkeypatch, **{envvars.STATE_DIR: own}) + pane = {envvars.STATE_DIR: str(tmp_path / "s1")} + + (warning,) = _launch_against(monkeypatch, tmp_path, FakeRun(pane_env=pane)) + set_env = warning[warning.index("tmux set-environment") : warning.index(" (add -g")] + assert shlex.split(set_env)[-2:] == [envvars.STATE_DIR, own] + export = warning[warning.index("export ") : warning.index(", or recreating")] + assert shlex.split(export) == ["export", f"{envvars.STATE_DIR}={own}"] From c490166aeeee9b60f3e1a2bb36accffd7105585f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Fri, 2 Oct 2026 21:58:26 +0200 Subject: [PATCH 2/3] test(adapters): sort test_multiplexer imports the way isort expects --- tests/test_multiplexer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index 364e9fcd7..7cd5629e5 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -16,10 +16,10 @@ import pytest from conftest import needs_strict_codec +from bmad_loop import envvars from bmad_loop.adapters import multiplexer, tmux_base from bmad_loop.adapters.base import SessionSpec from bmad_loop.adapters.generic import GenericAdapter -from bmad_loop import envvars from bmad_loop.adapters.multiplexer import ( UNSET, MultiplexerError, From 2cc963820fcef1a97eaccfd3218d69b31954f42d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Mon, 5 Oct 2026 08:50:52 +0200 Subject: [PATCH 3/3] fix(tui): warn that tmux's control session is shared before offering set-environment On tmux, bmad-loop-ctl is one session shared by every bmad-loop project on that server, so the stale-state-root remedy re-roots new windows for all of them. The warning and the docs now say so, offer set-environment only where no other project uses another state root, and say that kill-server ends every session on the server, live runs included. Tests cover the shared-server wording and a set-empty inherited override, which stays silent; the cascade spec gains empty-override rows; two redundant force_tmux_backend marks are dropped. Refs #731 --- CHANGELOG.md | 3 ++- docs/multiplexer-backends.md | 12 +++++++++--- src/bmad_loop/tui/launch.py | 14 ++++++++------ tests/test_multiplexer.py | 2 -- tests/test_runs.py | 19 ++++++++++++++++-- tests/test_tui_launch.py | 37 ++++++++++++++++++++++++++++++++++++ 6 files changed, 73 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2b7af02c..1e784c312 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,7 +57,8 @@ breaking changes may land in a minor release. - Warn once in the TUI when its tmux control session would hand new windows a different state root than its own (a server started under another `BMAD_LOOP_STATE_DIR`, `XDG_STATE_HOME` or `HOME`), naming both roots and the - `tmux set-environment` remedy, instead of letting the run read as gone (#731). + `tmux set-environment` remedy, with its limits on a server shared by several + projects, instead of letting the run read as gone (#731). - Escalate an environment fault at the review-budget rescue gate instead of deferring the story as unconverged (DW-523). - Explain that unpinned result-artifact scans search only the configured artifact diff --git a/docs/multiplexer-backends.md b/docs/multiplexer-backends.md index 08b833ddc..e68ae969a 100644 --- a/docs/multiplexer-backends.md +++ b/docs/multiplexer-backends.md @@ -69,12 +69,18 @@ a live run reads as gone (#731). When the TUI launches into its control session that, and warns once — naming both roots — when it differs from its own. The launch still goes ahead; the warning detects, it does not fix. The remedy: -- For new windows: `tmux set-environment -t =bmad-loop-ctl BMAD_LOOP_STATE_DIR `. The - session scope matters, because a session value overrides the global one; add the same - command with `-g` to cover new sessions as well, or `tmux kill-server` to start clean. +- For new windows: `tmux set-environment -t =bmad-loop-ctl BMAD_LOOP_STATE_DIR `. On + tmux, `bmad-loop-ctl` is one session shared by every bmad-loop project on that server, so this + re-roots new windows for all of them: use it only when no project there runs under another + state root. The session scope matters, because a session value overrides the global one; add + the same command with `-g` to cover new sessions as well. `tmux kill-server` also starts clean, + but it ends every session on the server, live runs and your own sessions included. - Shells already open there, window 0 included, keep the environment they started with. They need `export BMAD_LOOP_STATE_DIR=`, or recreating. +Two projects that need different state roots cannot share one tmux server's control session +safely until each launched run is handed its state root directly (#731). + psmux cannot answer the question — its `show-environment` does not report inherited values — so there is no such warning on psmux. Its per-project registry is keyed on the state root, so a psmux launcher under one root never reaches a server started under another through diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index 42559556c..75d88cf7f 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -976,12 +976,14 @@ def fault(detail: str) -> None: _warn_once( _STALE_ROOT, f"new windows in {session} would resolve {resolved}, not this process's " - f"state root {own}: its multiplexer server was started under a different " - "environment, so runs launched there can read as gone (#731). For new " - f"windows: tmux set-environment -t {shlex.quote('=' + session)} " - f"{envvars.STATE_DIR} {root} " - "(add -g for new sessions, or tmux kill-server to restart clean). Shells " - f"already open there need export {envvars.STATE_DIR}={root}, or recreating.", + f"state root {own}: its tmux server was started under a different " + "environment, so runs launched there can read as gone (#731, which tracks " + f"the lasting fix). {session} is shared by every bmad-loop project on this " + "tmux server, so set its root only if none of them uses another state root: " + f"tmux set-environment -t {shlex.quote('=' + session)} {envvars.STATE_DIR} {root} " + "(add -g for new sessions). tmux kill-server also starts clean, but it ends " + "every session on this server, live runs and your own sessions included. " + f"Shells already open there need export {envvars.STATE_DIR}={root}, or recreating.", ) diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index 7cd5629e5..8601da25e 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -1719,7 +1719,6 @@ def __call__(self, argv, **_k): _MISS = (1, "", "unknown variable: BMAD_LOOP_STATE_DIR\n") -@pytest.mark.usefixtures("force_tmux_backend") @pytest.mark.parametrize( ("replies", "expected", "asked"), [ @@ -1764,7 +1763,6 @@ def test_tmux_inherited_env_parses_show_environment(monkeypatch, replies, expect ] -@pytest.mark.usefixtures("force_tmux_backend") @pytest.mark.parametrize( "failure", [ diff --git a/tests/test_runs.py b/tests/test_runs.py index 7a352b41d..cdcafaae4 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -2173,6 +2173,12 @@ def test_state_root_refuses_a_home_that_cannot_root_a_control_plane(monkeypatch, ), pytest.param("linux", {"HOME": "rel"}, None, id="posix-relative-home"), pytest.param("linux", {"override": "rel", "HOME": "h"}, None, id="posix-relative-override"), + pytest.param( + "linux", + {"override": "empty", "XDG_STATE_HOME": "x", "HOME": "h"}, + "x/bmad-loop", + id="posix-empty-override", + ), pytest.param( "win32", {"override": "o", "LOCALAPPDATA": "l", "USERPROFILE": "p"}, @@ -2189,6 +2195,12 @@ def test_state_root_refuses_a_home_that_cannot_root_a_control_plane(monkeypatch, id="win-profile", ), pytest.param("win32", {"XDG_STATE_HOME": "x", "HOME": "h"}, None, id="win-none"), + pytest.param( + "win32", + {"override": "empty", "LOCALAPPDATA": "l", "USERPROFILE": "p"}, + "l/bmad-loop/state", + id="win-empty-override", + ), ] @@ -2198,8 +2210,9 @@ def test_resolve_state_root_matches_state_root_on_every_cascade_arm( ): """`state_root()` is `resolve_state_root` over this process's environment (#731), so on every cascade arm both must give the same, literal answer — - the root or the refusal. A spec value `rel` is written relative; every other - one is an absolute path under `tmp_path` (a trailing `/` kept as spelled). + the root or the refusal. A spec value `rel` is written relative, and `empty` + as `""` (an empty override reads as unset); every other one is an absolute + path under `tmp_path` (a trailing `/` kept as spelled). `USERPROFILE` mirrors `HOME` on the POSIX rows, as `_fake_home` does, so a faked-POSIX row means the same thing on a Windows host. HOME stays set on @@ -2213,6 +2226,8 @@ def test_resolve_state_root_matches_state_root_on_every_cascade_arm( name = envvars.STATE_DIR if key == "override" else key if value == "rel": env[name] = value + elif value == "empty": + env[name] = "" else: env[name] = str(tmp_path / value.rstrip("/")) + ("/" if value.endswith("/") else "") if platform == "linux": diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index d1e2d1da4..4fadffa3c 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -2786,3 +2786,40 @@ def test_the_remedy_commands_carry_the_root_through_a_shell_intact(monkeypatch, assert shlex.split(set_env)[-2:] == [envvars.STATE_DIR, own] export = warning[warning.index("export ") : warning.index(", or recreating")] assert shlex.split(export) == ["export", f"{envvars.STATE_DIR}={own}"] + + +@pytest.mark.parametrize("launcher", ["s2", "s1"]) +def test_stale_root_warning_on_a_shared_server(monkeypatch, tmp_path: Path, launcher): + """Two roots on one tmux server: `bmad-loop-ctl` is shared by every project + there, so the session-scoped remedy re-roots all of them. A launcher under + S2 against a server whose new panes resolve S1 warns, naming the shared + session, the condition on `set-environment` and the cost of `kill-server`; + a launcher under S1 against the same server stays silent. + + Ablation: drop the shared-session clause from the warning and the S2 row + fails.""" + _posix_env(monkeypatch, **{envvars.STATE_DIR: str(tmp_path / launcher)}) + pane = {envvars.STATE_DIR: str(tmp_path / "s1")} + + warned = _launch_against(monkeypatch, tmp_path, FakeRun(has_session_rc=0, pane_env=pane)) + if launcher == "s1": + assert warned == [] + return + assert len(warned) == 1 + assert "shared by every bmad-loop project" in warned[0] + assert "only if none of them uses another state root" in warned[0] + assert "ends every session on this server" in warned[0] + + +def test_silent_on_a_set_empty_inherited_override(monkeypatch, tmp_path: Path): + """Unlike HOME, an empty override reads as unset: a pane reporting + `BMAD_LOOP_STATE_DIR=` resolves exactly as if it had none, so with both + defaults agreeing there is no mismatch to report. + + Ablation: read the override by key presence in `resolve_state_root` and + the empty value refuses as relative, which warns.""" + home = str(tmp_path / "h") + _posix_env(monkeypatch, HOME=home) + pane = {envvars.STATE_DIR: "", "HOME": home} + + assert _launch_against(monkeypatch, tmp_path, FakeRun(pane_env=pane)) == []