From a3998f830a2a0e1871c238f9618afdde405b5b67 Mon Sep 17 00:00:00 2001 From: pbean Date: Tue, 14 Jul 2026 08:59:53 -0700 Subject: [PATCH] feat(mux): seam-canonical window targets Core hand-assembled tmux-shaped =session[:window] strings in 10 places, forcing every non-tmux backend to reverse-engineer tmux target syntax. The grammar is now owned by the seam: a concrete, overridable TerminalMultiplexer.target() encoder (default emits the same grammar, byte-identical), a module-level parse_target() decoder for native-id backends, and core formats every target via runs.session_target / launch.ctl_target. herdr's _parse_target delegates its grammar split; tmux backends keep passing tokens straight through. Zero existing test assertions changed. --- CHANGELOG.md | 9 ++++ docs/adapter-authoring-guide.md | 16 +++++++ src/bmad_loop/adapters/herdr_backend.py | 32 +++++++------- src/bmad_loop/adapters/multiplexer.py | 55 +++++++++++++++++++++---- src/bmad_loop/adapters/tmux_base.py | 5 ++- src/bmad_loop/runs.py | 8 +++- src/bmad_loop/tui/app.py | 8 ++-- src/bmad_loop/tui/launch.py | 14 +++++-- tests/test_multiplexer.py | 54 +++++++++++++++++++++++- 9 files changed, 168 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60089553..f63cc5db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,15 @@ breaking changes may land in a minor release. forced selection. A tmux-less POSIX host still selects `TmuxMultiplexer` and reports it unavailable, exactly as before. +- **Seam-canonical window targets.** The `=session[:window]` target grammar is now owned by the + `TerminalMultiplexer` seam instead of living as hand-assembled tmux syntax in core: a new + concrete `target(session, window=None)` encoder (overridable per backend, tmux inherits the + default and passes it straight through) and a module-level `parse_target()` decoder that + native-id backends reuse instead of re-deriving the grammar (the herdr backend's + `_parse_target` now delegates to it). `runs.py`/`tui/launch.py`/`tui/app.py` format every + target via `target()` (new `runs.session_target` / `launch.ctl_target` helpers) — output is + byte-identical, so no backend or operator behavior changes; the contract is documented in the + adapter authoring guide's new "Window targets" section. - **Herdr TUI-launch surface.** The herdr backend now covers everything `tui/launch.py` drives: parked orchestrator windows (a typed `exec sh -c '; banner; read; trailer'` recipe, tmux-identical from the operator's seat, with the return-to-origin target mirrored into a diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index f0a332b2..618a87ff 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -84,6 +84,22 @@ seams of a full OS port are in `detach_client`, `switch_client` (with an optional last-client fallback), `available` (is this backend usable on the current host). +**Window targets.** The target-taking methods (`kill_window`, `select_window`, +the window-option trio, `attach_target_argv`, `switch_client`) receive one of two +families: the **seam-canonical target token** `=session[:window]` — formatted by +the concrete `TerminalMultiplexer.target(session, window=None)`, decoded by the +module-level `parse_target()` — or the backend's own **native id** (whatever your +`new_window` returned). Core never hand-assembles the grammar; it calls +`target()`. tmux consumes the token natively (it coincides with tmux exact-match +syntax), so `BaseTmuxBackend` passes it straight through. A native-id backend +calls `parse_target()` first — `None` means "already a native id, use as-is", +otherwise resolve `(session, window)` yourself; `herdr_backend._parse_target` is +the worked example (workspace-by-label → tab-by-name → root pane, resolved lazily +at use time). You MAY override `target()` to emit native ids, but the token must +stay a stable _by-name_ reference: core formats targets ahead of use (a parked +window's return target, for one), so eager resolution to a live id goes stale — +inheriting the default and resolving lazily is almost always right. + Operations that can race a window dying (`pipe_pane`) or a session already being gone (`kill_session`) must tolerate it rather than raise; everything else raises a `MultiplexerError` subclass on failure, which call sites catch at the seam (e.g. diff --git a/src/bmad_loop/adapters/herdr_backend.py b/src/bmad_loop/adapters/herdr_backend.py index 768d824e..2933a7b8 100644 --- a/src/bmad_loop/adapters/herdr_backend.py +++ b/src/bmad_loop/adapters/herdr_backend.py @@ -42,8 +42,9 @@ ``herdr tab focus``). POSIX-only, like the ``exec`` launch (pwsh dialect is the Windows follow-up). - ``attach_target_argv`` accepts both target families (native pane id and the - tmux-style ``=session[:window]`` specs ``tui/launch.py`` builds — see - ``_parse_target``): outside herdr it returns ``["herdr", "terminal", + seam-canonical ``=session[:window]`` tokens core formats via + ``TerminalMultiplexer.target`` — see ``_parse_target``): outside herdr it + returns ``["herdr", "terminal", "attach", ]`` (which blocks, and exits when the pane closes); inside a herdr pane it returns the fire-and-forget ``["herdr", "tab", "focus", ]`` — the switch-client move, mirroring tmux's in-``TMUX`` @@ -88,7 +89,7 @@ from pathlib import Path from .. import platform_util -from .multiplexer import MultiplexerError, TerminalMultiplexer +from .multiplexer import MultiplexerError, TerminalMultiplexer, parse_target HERDR_TIMEOUT_S = 30 # The herdr server wire protocol this backend was written against (0.7.3). Read @@ -665,19 +666,20 @@ def _tab_root_pane(self, tab_id: str, *, strict: bool) -> str | None: def _parse_target(self, target: str, *, strict: bool) -> str | None: """Resolve any window target to a native pane id. - Callers hand this two families: a tmux-style ``=session[:window]`` spec - (``tui/launch.py`` builds these) and our own native pane id. The ``=`` - prefix is the discriminator — native ids contain ``:`` (``w1:p1``) but - never lead with ``=``. A native id passes through untouched, with no - server round-trip. For ``=…`` targets: session → workspace (first-match - label), a window name → the tab with that label, no window → the - session-level tab (see :func:`_session_level_tab`), then that tab's - pane. ``strict=True`` raises :class:`HerdrError` when the target cannot - be resolved; ``strict=False`` returns None (the sentinel callers' quiet - failure).""" - if not target.startswith("="): + Callers hand this two families: the seam-canonical ``=session[:window]`` + token (decoded by :func:`multiplexer.parse_target`; core formats these + via ``TerminalMultiplexer.target``) and our own native pane id — native + ids contain ``:`` (``w1:p1``) but never lead with ``=``. A native id + passes through untouched, with no server round-trip. For ``=…`` targets: + session → workspace (first-match label), a window name → the tab with + that label, no window → the session-level tab (see + :func:`_session_level_tab`), then that tab's pane. ``strict=True`` + raises :class:`HerdrError` when the target cannot be resolved; + ``strict=False`` returns None (the sentinel callers' quiet failure).""" + parsed = parse_target(target) + if parsed is None: return target - session, _, window = target[1:].partition(":") + session, window = parsed row = self._workspace_row(session, strict=strict) if row is None: if strict: diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index 11959fd4..e5addbea 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -39,6 +39,24 @@ class MultiplexerError(Exception): without importing a backend.""" +def parse_target(target: str) -> tuple[str, str | None] | None: + """Decode a seam-canonical window target (see :meth:`TerminalMultiplexer.target`). + + Returns ``(session, window)`` for a canonical ``=session[:window]`` token — + ``window`` is None when absent *or* empty, so ``"=s"`` and ``"=s:"`` both + decode to ``("s", None)`` — or None when ``target`` does not start with + ``=``: a backend-native id (``"@1"``, ``"%3"``, ``"w1:p1"``, ...) the caller + resolves itself. The window part is everything after the *first* ``:``; + that split is safe because bmad-loop mints window names + (``-``) that never contain ``:``. Provided so a backend whose + native addressing differs decodes the grammar with one tested helper + instead of re-deriving it (see the herdr backend's ``_parse_target``).""" + if not target.startswith("="): + return None + session, _, window = target[1:].partition(":") + return (session, window or None) + + class TerminalMultiplexer(ABC): """Transport backend for agent sessions: sessions, windows, and clients. @@ -49,6 +67,21 @@ class TerminalMultiplexer(ABC): ``tui/launch.py``, ``probe.py``, and ``tui/data.py`` migrate onto it. """ + # ------------------------------------------------------------ targets + + def target(self, session: str, window: str | None = None) -> str: + """Format the seam-canonical target token for ``session`` (optionally + one of its windows, *by name*). The default grammar is + ``=session[:window]`` — historically tmux's exact-match syntax, now + owned by the seam: every target-taking method below accepts both this + token and the backend's native ids, and :func:`parse_target` is the + matching decoder. Backends MAY override to emit native ids, but the + result must stay a stable *by-name* reference: callers format targets + ahead of use (e.g. a parked window's return target), so eager + resolution to a live native id can go stale — keeping the token + symbolic and resolving lazily at use time is the recommended default.""" + return f"={session}:{window}" if window else f"={session}" + # ----------------------------------------------------------- sessions @abstractmethod @@ -132,28 +165,33 @@ def window_alive(self, session: str, window_id: str) -> bool: @abstractmethod def kill_window(self, target: str) -> None: """Kill the targeted window (tolerant of it already being gone, and a - no-op on a transport failure).""" + no-op on a transport failure). ``target`` is a :meth:`target` token or + a backend-native window id.""" @abstractmethod def select_window(self, target: str) -> None: """Make ``target`` the current window of its session (best-effort: a no-op - on a transport failure).""" + on a transport failure). ``target`` is a :meth:`target` token or a + backend-native window id.""" @abstractmethod def set_window_option(self, target: str, option: str, value: str) -> None: """Set a user option on the targeted window (best-effort: a no-op on a - transport failure).""" + transport failure). ``target`` is a :meth:`target` token or a + backend-native window id.""" @abstractmethod def unset_window_option(self, target: str, option: str) -> None: """Remove a user option from the targeted window (so a later read sees it as unset, not as an empty value). Best-effort: a no-op on a transport - failure.""" + failure. ``target`` is a :meth:`target` token or a backend-native + window id.""" @abstractmethod def show_window_option(self, target: str, option: str) -> str: """Value of a user option on the targeted window ('' if unset, and '' on a - transport failure).""" + transport failure). ``target`` is a :meth:`target` token or a + backend-native window id.""" @abstractmethod def pipe_pane(self, window_id: str, log_file: Path) -> None: @@ -168,7 +206,9 @@ def send_text(self, window_id: str, text: str) -> None: @abstractmethod def attach_target_argv(self, target: str) -> list[str]: - """argv that attaches the caller's terminal to ``target``.""" + """argv that attaches the caller's terminal to ``target`` (a + :meth:`target` token — session-only or session+window — or a + backend-native id).""" @abstractmethod def current_pane_id(self) -> str | None: @@ -194,7 +234,8 @@ def detach_client(self) -> None: 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.""" + transport failure returns False. ``target`` is a :meth:`target` token + or a backend-native id.""" @abstractmethod def available(self) -> bool: diff --git a/src/bmad_loop/adapters/tmux_base.py b/src/bmad_loop/adapters/tmux_base.py index 38937818..9173042c 100644 --- a/src/bmad_loop/adapters/tmux_base.py +++ b/src/bmad_loop/adapters/tmux_base.py @@ -43,7 +43,10 @@ class TmuxError(MultiplexerError): class BaseTmuxBackend(TerminalMultiplexer): """tmux-family backend: all argv construction and every contract method, with - one overridable subprocess primitive (:meth:`_run`) every call funnels through.""" + one overridable subprocess primitive (:meth:`_run`) every call funnels through. + The seam-canonical target grammar (``=session[:window]``, see + :meth:`TerminalMultiplexer.target`) coincides with tmux's exact-match target + syntax, so targets pass straight through to tmux — never parsed here.""" #: Output decoding for captured tmux text. ``None`` (POSIX) = locale default, #: byte-identical to a bare ``text=True``; a Windows leaf sets ``"utf-8"``. diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 52f438f5..ca24b56e 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -120,8 +120,14 @@ def attach_target_argv(target: str) -> list[str]: return get_multiplexer().attach_target_argv(target) +def session_target(run_id: str) -> str: + """Seam-canonical target token for the run's agent session (see + :meth:`TerminalMultiplexer.target`).""" + return get_multiplexer().target(session_name(run_id)) + + def attach_argv(run_id: str) -> list[str]: - return attach_target_argv(f"={session_name(run_id)}") + return attach_target_argv(session_target(run_id)) # ---------------------------------------------------- run resolution / liveness diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index daab212a..450ff79e 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -360,12 +360,12 @@ def action_attach(self) -> None: if window is not None and (self._dashboard.decision_pending is not None or not agent_live): launch.select_ctl_window(window) self._attach_to_target( - f"={launch.CTL_SESSION}", - return_window=f"={launch.CTL_SESSION}:{window}", + launch.ctl_target(), + return_window=launch.ctl_target(window), ) return elif agent_live: - target = f"={session}" + target = runs.session_target(run_id) else: self.notify( f"nothing to attach: no live agent session ({session}) and no " @@ -459,7 +459,7 @@ def _launch_resolve(self, run_id: str) -> None: self.notify("resolve launched but its window id was not captured", severity="error") return launch.select_ctl_window_id(win_id) - self._attach_to_target(f"={launch.CTL_SESSION}", return_window=win_id) + self._attach_to_target(launch.ctl_target(), return_window=win_id) # -------------------------------------------------------- HITL pause review diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index a79af20f..e57450a9 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -51,10 +51,16 @@ def ctl_window(run_id: str) -> str | None: return None +def ctl_target(window: str | None = None) -> str: + """Seam-canonical target token for the control session (optionally one of + its windows, by name); see :meth:`TerminalMultiplexer.target`.""" + return get_multiplexer().target(CTL_SESSION, window) + + def select_ctl_window(window: str) -> None: """Make `window` the control session's current window, so a plain attach to the session lands on it (attach-session itself takes no window).""" - get_multiplexer().select_window(f"={CTL_SESSION}:{window}") + get_multiplexer().select_window(ctl_target(window)) def select_ctl_window_id(window_id: str) -> None: @@ -162,9 +168,9 @@ def attach_plan(project: Path, run_id: str) -> tuple[list[str], str | None] | No decision_pending(runs.run_dir_for(project, run_id)) or not agent_live ): select_ctl_window(window) - return runs.attach_target_argv(f"={CTL_SESSION}"), f"={CTL_SESSION}:{window}" + return runs.attach_target_argv(ctl_target()), ctl_target(window) if agent_live: - return runs.attach_target_argv(f"={session}"), None + return runs.attach_target_argv(runs.session_target(run_id)), None return None @@ -173,7 +179,7 @@ def kill_ctl_window(run_id: str) -> None: if any. A no-op when the run was not launched from the TUI or tmux is gone.""" window = ctl_window(run_id) if window is not None: - get_multiplexer().kill_window(f"={CTL_SESSION}:{window}") + get_multiplexer().kill_window(ctl_target(window)) def _ctl_window_candidates(project: Path) -> list[tuple[str, str]]: diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index a67062e0..74c81ecb 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -17,7 +17,8 @@ from bmad_loop.adapters import tmux_base from bmad_loop.adapters.base import SessionSpec from bmad_loop.adapters.generic import GenericAdapter -from bmad_loop.adapters.multiplexer import MultiplexerError, TerminalMultiplexer +from bmad_loop.adapters.herdr_backend import HerdrMultiplexer +from bmad_loop.adapters.multiplexer import 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 @@ -506,3 +507,54 @@ def test_dialect_leaf_new_window_routes_launch_through_hook(monkeypatch, tmp_pat "wrapped:cmd", ] assert "-e" not in rec.argv # env strategy fully delegated to the hook + + +# ------------------------------------------------------------ target contract +# +# target() is the seam-canonical encoder core uses instead of hand-assembling +# "=session[:window]" strings; parse_target is the matching decoder a native-id +# backend reuses instead of re-deriving the grammar. Pure string work: no +# subprocess, no env sensitivity, safe on every CI leg. Both backends are +# constructed directly (their constructors are documented side-effect-free). + + +def test_target_default_grammar(): + mux = TmuxMultiplexer() + assert mux.target("s") == "=s" + assert mux.target("s", "w") == "=s:w" + # falsy window collapses to the session-only form, mirroring parse_target's + # "=s:" -> ("s", None) decode + assert mux.target("s", None) == "=s" + assert mux.target("s", "") == "=s" + + +def test_herdr_inherits_the_default_encoder(): + # herdr resolves targets lazily at use time (_parse_target), so it must NOT + # override target() to eagerly emit native ids — a token formatted ahead of + # use (e.g. attach_plan's return_window) would go stale. + assert "target" not in HerdrMultiplexer.__dict__ + assert HerdrMultiplexer().target("s", "w") == "=s:w" + + +@pytest.mark.parametrize( + ("session", "window"), + [("s", None), ("s", "w"), ("bmad-loop-ctl", "run-20260714-abc")], +) +def test_parse_target_round_trips_the_encoder(session, window): + mux = TmuxMultiplexer() + assert parse_target(mux.target(session, window)) == (session, window) + + +def test_parse_target_edges(): + # empty window part decodes like the session-only form + assert parse_target("=s:") == ("s", None) + # window is everything after the FIRST colon (minted names carry no colon, + # but the split rule is pinned regardless) + assert parse_target("=s:a:b") == ("s", "a:b") + + +@pytest.mark.parametrize("native", ["@1", "%3", "w1:p1"]) +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