From fd060e86e4898622bfbfc850b53476711ca0c5c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Sun, 26 Jul 2026 13:38:53 +0200 Subject: [PATCH 1/5] fix(adapters): give psmux a per-window option channel (#310) --- CHANGELOG.md | 15 + docs/multiplexer-backends.md | 12 + src/bmad_loop/adapters/multiplexer.py | 16 +- src/bmad_loop/adapters/psmux_backend.py | 396 +++++++++++++++++++- tests/test_psmux_backend.py | 469 +++++++++++++++++++++++- 5 files changed, 884 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29153af3..b4eb63d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,6 +125,21 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **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 + every `list-windows` row, letting a prune in one project `kill-window` another project's + window; the parked-return option always read empty, so an attached client was never handed + back to its origin. The window-option verbs, the `@`-prefixed columns of `list_windows` and the + parked trailer now use a session-scoped option whose key carries the window id + (`@bmad_project_@3` for window `@3` — the `_@` shape keeps foreign config options out of the + cleanup sweeps), routed with an explicit `-t ` (the in-pane parked trailer rides + `$TMUX` instead). Values that cannot survive psmux's + control-line transport verbatim are refused with a warning instead of stored corrupted. Keys are + freed on `kill_window` and reconciled at parked-window launch; the return move itself is restored + for the detach leg, while the `switch-client` leg stays inert until psmux/psmux#483 lands + upstream. Builtin window options keep the `-w` path; tmux is untouched. + - **Session-qualify the psmux TUI-side window ids (#291).** #254 covered the engine seam but left the launcher's surfaces bare, and that process usually runs _outside_ any pane — where a bare `@N` resolves through psmux's most-recent-session fallback, not the session that minted it. The diff --git a/docs/multiplexer-backends.md b/docs/multiplexer-backends.md index 45ba7c29..9d40d8a2 100644 --- a/docs/multiplexer-backends.md +++ b/docs/multiplexer-backends.md @@ -49,6 +49,18 @@ selection falls through). Native Windows is still experimental — see the [roadmap](ROADMAP.md#native-windows-multiplexer-backend) for the remaining work. WSL is unaffected: it _is_ Linux and uses tmux. +Two model differences matter if you port a backend or read psmux argv. psmux runs one server +per session, so window ids are minted per server and the backend session-qualifies every id it +hands out (`session:@N`). And psmux has no per-window user options — one scope exists per +server — so the window-option verbs and the `@`-prefixed columns of `list_windows` are served +by a session-scoped option whose key carries the window id (`@bmad_project_@3` for window `@3`). +Both are properties of psmux's model, not gaps awaiting an upstream release. Practical +consequence: such a value is **not** readable via `psmux show-options -w` by hand — read it with +`psmux show-options -qv -t "@bmad_project_@N"` instead. One visible limit: a value +that cannot survive psmux's control-line transport verbatim (e.g. a project path that is both +UNC and spaced) is refused with a stderr warning at every launch, and that project's windows +stay untagged — the prune then scopes them through the run-dir fallback instead of the tag. + ## External backends Every backend beyond the two bundled ones is a separate package that you co-install with bmad-loop; it diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index bc17577d..e7265857 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -182,7 +182,13 @@ def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]] probe, so a sentinel is safe). A ``window_id`` column carries the same id form :meth:`current_window_id` - returns; core compares the two directly.""" + returns; core compares the two directly. + + An ``@``-prefixed option column may carry a backend-private opaque + marker meaning "the read FAILED" (distinct from ``""`` = unset). + Callers must only ever compare such a column against their own known + values — a non-matching value means "not mine", never a displayable + tag.""" @abstractmethod def window_alive(self, session: str, window_id: str) -> bool: @@ -209,7 +215,13 @@ def select_window(self, target: str) -> None: 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). ``target`` is a :meth:`target` token or a - backend-native window id.""" + backend-native window id. + + The contract is the (window, option) keying, not the storage: a backend + without per-window option scope may key the value however it likes + (psmux does), so read it back only through :meth:`show_window_option` + or :meth:`list_windows`, never by running the multiplexer's own option + verbs by hand.""" @abstractmethod def unset_window_option(self, target: str, option: str) -> None: diff --git a/src/bmad_loop/adapters/psmux_backend.py b/src/bmad_loop/adapters/psmux_backend.py index 8e861ba4..d2f79421 100644 --- a/src/bmad_loop/adapters/psmux_backend.py +++ b/src/bmad_loop/adapters/psmux_backend.py @@ -21,8 +21,11 @@ ``select_window`` is the one verb that cannot take that form: psmux validates a scoped target's window part CLI-side against window index/name only, so the override resolves the id -back to an index first. The window-option verbs need no override: psmux -drops the window slot of a ``-w`` target either way (#310). +back to an index first. Per-window user options do not exist at all — +psmux keeps one user-option scope per server and every ``-w`` read of an +``@``-prefixed name is empty — so the window-option verbs, the +``@``-prefixed columns of ``list_windows`` and the parked trailer route +through a session-scoped option whose key carries the window id (#310). ``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 @@ -109,12 +112,43 @@ def _parked_trailer(self, return_opt: str) -> str: # The base's trailer re-expressed in pwsh — the tmux verbs are protocol- # identical across the family. Errors go to $null: a client or pane that # is already gone means the window just parks as-is. + # + # The base reads the return target with `show-options -wqv`, which is + # dead on psmux (#310), so this reads the session-scoped id-keyed option + # instead. Unlike every other caller of that channel, the trailer cannot + # be handed its own window id — it is built before the window exists — + # so it probes for it in-pane. `-t $env:TMUX_PANE` pins the probe to the + # pane's own window (psmux sets TMUX_PANE in every pane, and a bare `%N` + # target resolves globally via DisplayMessageById); a target-less probe + # would resolve the server's *active* window, which is another window's + # key the moment focus moves after Enter. No `-t `: running + # inside the pane, $TMUX already routes to this window's own server. + # + # Both captures go through "$(...)".Trim(): a bare capture yields an + # array when psmux emits more than one line, and `-eq` on an array + # filters instead of comparing — silently taking neither branch. mux = self._BINARY + probe = ( + '"$(' + mux + " display-message -p -t $env:TMUX_PANE '#{window_id}' 2>$null)\".Trim()" + ) + read_key = '"$(' + mux + ' show-options -qv $key 2>$null)".Trim()' return ( - f"$ret = {mux} show-options -wqv {_pwsh_quote(return_opt)} 2>$null; " + f"$wid = {probe}; " + # A failed probe skips the return AND the key free; the orphan + # sweep at a later parked-window launch reclaims the key once the + # window is gone. + f"if ($wid) {{ $key = {_pwsh_quote(return_opt + '_')} + $wid; " + f"$ret = {read_key}; " f"if ($ret -eq '{PARKED_RETURN_DETACH}') {{ {mux} detach-client 2>$null }} " + # The switch leg is still inert at 3.3.7 (psmux/psmux#483: the + # server splits the target without parse_target); only the detach + # return actually moves a client today. Kept: it is the correct + # verb the moment upstream lands the fix. f"elseif ($ret) {{ {mux} switch-client -t $ret 2>$null; " - f"if ($LASTEXITCODE -ne 0) {{ {mux} switch-client -l 2>$null }} }}" + f"if ($LASTEXITCODE -ne 0) {{ {mux} switch-client -l 2>$null }} }} " + # Free the key on the way out: the ctl session outlives every run, + # so a key left behind is one this server carries for its whole life. + f"{mux} set-option -u $key 2>$null }}" ) def _window_launch(self, env: dict[str, str], command: str) -> list[str]: @@ -232,23 +266,353 @@ def new_parked_window( # falls through to the most-recent-session fallback rather than the # session that just minted it. window_id = super().new_parked_window(session, name, cwd, argv, return_opt) + # Launch time is the reconcile point for keys whose window is gone — + # Enter-dismissing a parked window closes it without kill_window ever + # running, so its keys would otherwise outlive it (#310). + self._sweep_orphan_keys(session) return self._qualified_window_id(session, window_id) def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]]: - # Qualify only the `window_id` columns — the prune replays exactly those - # values as kill-window targets, and a bare one can take down another - # server's identically-numbered window. - rows = super().list_windows(session, fields) - id_columns = [i for i, field in enumerate(fields) if field == "window_id"] - if not id_columns: + # Two corrections on the columns the prune reads. `window_id` is + # qualified — the prune replays those values as kill-window targets, and + # a bare one can hit another server's identically-numbered window. An + # `@` field never reaches psmux: `#{@name}` expands from the one + # per-server map, so every row would carry the same value. Probe the + # window id in its place and fill from the id-keyed options, fetched as + # ONE full listing — a flat extra call per list_windows, regardless of + # row or column count (#310). + opt_columns = {i: field for i, field in enumerate(fields) if field.startswith("@")} + probe_fields = [ + "window_id" if i in opt_columns else field for i, field in enumerate(fields) + ] + rows = super().list_windows(session, probe_fields) + id_columns = {i for i, field in enumerate(fields) if field == "window_id"} + if not opt_columns and not id_columns: return rows - return [ - tuple( - self._qualified_window_id(session, value) if i in id_columns else value - for i, value in enumerate(row) + # The #221 degrade: an empty or `:`-bearing session cannot be routed + # with `-t`, and an unrouted read would answer from whichever server + # the fallback picks — fill "" without issuing reads at all. + degraded = not session or ":" in session + options = self._scoped_options(session) if opt_columns and not degraded else None + if opt_columns and not degraded and options is None: + # Without this line the failure is invisible: every fill poisons, + # so a prune dry-run would report nothing prunable with no trace. + print( + f"warning: show-options listing failed on {session}; " + "option columns filled with a read-failure marker", + file=sys.stderr, ) - for row in rows - ] + out: list[tuple[str, ...]] = [] + for row in rows: + values = list(row) + for i, option in opt_columns.items(): + # values[i] is the bare `@N` probed in the option column's place. + digits = self._id_digits(values[i]) + if degraded: + values[i] = "" + elif not digits or options is None: + # A failed id probe or failed listing (≠ every key unset) — + # see _OPT_READ_FAILED. + values[i] = self._OPT_READ_FAILED + else: + values[i] = options.get(self._scoped_option_key(option, digits), "") + for i in id_columns: + values[i] = self._qualified_window_id(session, values[i]) + out.append(tuple(values)) + return out + + # ------------------------------------ per-window option channel (#310) + # + # Per-window user options do not exist on psmux: there is one scope per + # server, and every `-w` read of an `@` name returns '' before the map is + # consulted (a 14-name builtin allowlist gates it). Deliberate, per + # psmux/psmux#321 — a boundary to route around, not a bug to wait out. + # + # Substitute: a SESSION-scoped option whose key carries the window id + # (`@bmad_project_@3` for `@3`). Two rules keep it correct — + # - `-t ` on every out-of-pane write and read (the parked + # trailer runs in-pane and rides `$TMUX` instead — see + # `_parked_trailer`). psmux picks the server from the target, and + # without an explicit session it falls back to $TMUX / most-recent — + # i.e. some other server. The session comes from the qualified ids + # this backend already mints, which is why #310 lands after #291. + # - the key ends with the full id (`_@3`, never bare digits): the ctl + # server loads the user's psmux config, so the map is shared, and the + # `_@` shape is what keeps foreign config options out of the generic + # cleanup sweeps. Not airtight — psmux accepts any `@` name, so a + # hand-written `@theme_@3` WOULD match — but no naming convention in + # the tmux/psmux ecosystem puts `@` mid-name, and a bmad-owned prefix + # guard would hardcode caller names into the backend. The session + # stays out of the key; routing already carries it. + # + # Builtin window options keep the base's `-w` argv — psmux accepts those + # allowlisted names (only `automatic-rename` has true per-window storage; + # the rest read the global value), and rerouting them into this channel + # would break reads that work today. + + # A window target as the seam composes it: `session:@N`, `=session:@N`, or + # `=session:`. Deliberately looser than _QUALIFIED_ID, which + # only admits the id form — set_return_pane passes a name token. + _SESSION_WINDOW = re.compile(r"^(?P[^:]+):(?P.+)$") + + @staticmethod + def _id_digits(window_id: str) -> str: + """`@3` -> `3`; `""` for anything that is not a bare window id.""" + return window_id[1:] if PsmuxMultiplexer._BARE_ID.fullmatch(window_id) else "" + + def _option_scope(self, target: str) -> tuple[str, str] | None: + """``(session, id-digits)`` for a window target, or None when it carries + no session and so cannot be routed to a specific server. + + A sessionless target reaches here only where `_qualified_window_id` + degraded. Guessing a server for it is the misrouting qualification + exists to prevent, so it returns None and the caller declines to act. + """ + match = self._SESSION_WINDOW.fullmatch(target.removeprefix("=")) + if match is None: + return None + session, window = match["session"], match["window"] + digits = self._id_digits(window) + if not digits: + # A name token; same one-round-trip renumber race as _window_index. + digits = self._id_digits(self._window_id_for_name(session, window) or "") + return (session, digits) if digits else None + + def _window_id_for_name(self, session: str, name: str) -> str | None: + """Bare window id of the window called ``name`` in ``session``, or None.""" + # super(): the base emits unqualified ids, the form the key needs. + for win_id, win_name in super().list_windows(session, ["window_id", "window_name"]): + if win_name == name: + return win_id + return None + + @staticmethod + def _scoped_option_key(option: str, digits: str) -> str: + return f"{option}_@{digits}" + + # The channel's key suffix. Anchored: `@bmad_project_@13` must not read as + # a key of window `@3`, and a foreign `@color_3` (no `_@` shape) never + # matches (see the namespace note in the channel comment above). + _KEY_SUFFIX = re.compile(r"_@(\d+)$") + + # Fill value for an option column whose read FAILED (≠ unset). Non-empty on + # purpose: "" would drop a genuinely-tagged window into the prune's + # untagged run-dir fallback, while an impossible tag reads as "some other + # project" and the window is skipped — the safe direction for a kill scan. + _OPT_READ_FAILED = "\x00option-read-failed" + + def _read_scoped(self, session: str, option: str, digits: str) -> str | None: + # No `-w`: the session-scoped read is the one that reaches the map. + # None = transport failure, "" = unset; only list_windows' fill keeps + # the distinction, the ABC read below collapses both to "". + try: + proc = self._run( + ["show-options", "-qv", "-t", session, self._scoped_option_key(option, digits)], + check=False, + ) + except (subprocess.SubprocessError, OSError): + return None + return proc.stdout.strip() if proc.returncode == 0 else None + + def _warn_unroutable(self, verb: str, target: str, option: str) -> None: + # Silence would leave a write that looks like it landed but did not. + print( + f"warning: {verb} {option} skipped — {target} does not resolve to a " + "window id on a routable session (unqualified target, unknown window " + "name, or the name-resolve listing failed), so the per-window option " + "channel is unavailable", + file=sys.stderr, + ) + + @staticmethod + def _transportable(value: str) -> bool: + # The value crosses psmux's CLI→server control line: the client wraps a + # spaced value in double quotes escaping only `"`, and the server + # tokenizer treats a bare `'` as a quote opener, drops `-`-leading + # tokens, and collapses `\\` inside double quotes. A value that cannot + # survive that hop verbatim is refused loudly instead of stored + # corrupted — a tag that reads back different from what the prune + # compares against makes the window silently unprunable. + # + # The two branches deliberately ban DIFFERENT shapes. Inside the + # client's double quotes `'` is literal and a mid-token `;` survives, + # so a spaced `O'Brien Files` or `a; b` path passes. But the one-shot + # chain splitter (config.rs `split_chained_commands`) is NOT + # quote-aware: it cuts on whitespace-delimited `;`/`\;` TOKENS, so + # `a ; b` stores as `a` and hands the rest to the server as a command + # (live-verified on 3.3.7; psmux/psmux#499). `\\` collapses and a `"` can close the + # wrapper early (client-escaped `\"` after a backslash reads back as + # `\\` + closing quote). So the spaced branch refuses `"`, `\\`, a + # trailing `\`, and standalone `;`/`\;` tokens. + # Non-ASCII-space whitespace (NBSP, tab, …) is refused outright: the + # client quotes only on ASCII `' '` (main.rs `s.contains(' ')`) while + # the server tokenizer splits on Unicode `is_whitespace()` — an NBSP in + # an unquoted value splits the token server-side. Leading/trailing + # ASCII space is refused too: it survives the wire, but this backend's + # own reads strip/Trim, so the value could never read back equal. + if not value or value.startswith("-") or any(c.isspace() and c != " " for c in value): + return False + if value != value.strip(): + return False + if " " in value: # will be double-quoted by the psmux client + if '"' in value or "\\\\" in value or value.endswith("\\"): + return False + return all(tok not in (";", "\\;") for tok in value.split()) + return not any(c in value for c in ";'\"") + + def set_window_option(self, target: str, option: str, value: str) -> None: + if not option.startswith("@"): + super().set_window_option(target, option, value) + return + if not self._transportable(value): + print( + f"warning: set-option {option} skipped — value does not survive " + "psmux's control-line transport verbatim; the option stays unset", + file=sys.stderr, + ) + return + scope = self._option_scope(target) + if scope is None: + self._warn_unroutable("set-option", target, option) + return + session, digits = scope + # A write that silently failed re-opens the mis-scoped prune this + # channel exists to close, so failures are said out loud (the verbs + # stay best-effort — warn, never raise). + key = self._scoped_option_key(option, digits) + try: + proc = self._run(["set-option", "-t", session, key, value], check=False) + if proc.returncode != 0: + print( + f"warning: set-option {key} failed: {proc.stderr.strip()}", + file=sys.stderr, + ) + except (subprocess.SubprocessError, OSError) as exc: + print(f"warning: set-option {key} failed: {exc}", file=sys.stderr) + + def unset_window_option(self, target: str, option: str) -> None: + if not option.startswith("@"): + super().unset_window_option(target, option) + return + scope = self._option_scope(target) + if scope is None: + self._warn_unroutable("set-option -u", target, option) + return + session, digits = scope + # `-u` genuinely frees the key: the server's SetOptionUnset handler + # removes `@`-prefixed names from the map (verified at v3.3.7). `-u` is + # a write like set — a silently failed unset leaves a live return key + # that replays the return move when the window's command exits, so + # failures are said out loud here too (warn, never raise). + key = self._scoped_option_key(option, digits) + try: + proc = self._run(["set-option", "-u", "-t", session, key], check=False) + if proc.returncode != 0: + print( + f"warning: set-option -u {key} failed: {proc.stderr.strip()}", + file=sys.stderr, + ) + except (subprocess.SubprocessError, OSError) as exc: + print(f"warning: set-option -u {key} failed: {exc}", file=sys.stderr) + + def show_window_option(self, target: str, option: str) -> str: + if not option.startswith("@"): + return super().show_window_option(target, option) + scope = self._option_scope(target) + if scope is None: + # An unroutable target warns for reads the same as for writes — + # no verb is sent; "" already means "unset" to every caller. + self._warn_unroutable("show-options", target, option) + return "" + value = self._read_scoped(scope[0], option, scope[1]) + if value is None: + # Transport failure, not a miss. The return value still degrades + # to "" ("unset") — that is all the ABC read admits — but say so. + print( + f"warning: show-options {option} failed on {target}; treating as unset", + file=sys.stderr, + ) + return "" + return value + + def _scoped_options(self, session: str) -> dict[str, str] | None: + """All `@`-prefixed options on ``session`` as ``{key: value}``, or None + on any failure (distinct from {} — an empty map is a real answer). + Live-verified on 3.3.7: `show-options -q -t ` lists user + options as `@key "value"` lines; accepted channel values can contain + neither `"` nor newlines (see _transportable), so the quote strip is + lossless for every value this backend wrote. Known hole: one server + request handler (the plugin-drain copy, server/mod.rs:465 at v3.3.7) + answers this listing empty-with-success while keys exist, so a + surprising {} is possible and is not proof that no keys are set.""" + try: + proc = self._run(["show-options", "-q", "-t", session], check=False) + except (subprocess.SubprocessError, OSError): + return None + if proc.returncode != 0: + return None + options: dict[str, str] = {} + for line in proc.stdout.splitlines(): + name, _, rest = line.partition(" ") + if not name.startswith("@"): + continue + if len(rest) >= 2 and rest.startswith('"') and rest.endswith('"'): + rest = rest[1:-1] + options[name] = rest + return options + + def _sweep_orphan_keys(self, session: str) -> None: + # Free `_@N` keys whose window no longer exists (Enter-dismissed parked + # windows never pass through kill_window). The `_@` shape keeps foreign + # config options out of the sweep; window ids are never recycled within + # a server, so a swept key cannot belong to a future window. + # + # Order matters: keys are snapshotted BEFORE the live-window listing, so + # a window minted-and-tagged between the two calls has its key outside + # the snapshot and cannot be swept as a false orphan. + try: + options = self._scoped_options(session) + if options is None: + return + live = set(super().list_window_ids(session)) # base = bare ids + if not live: + # A session being swept just minted a window, so an empty live + # list is a failed probe, not an empty session — treating it as + # truth would sweep every key, live windows included. + return + for name in options: + match = self._KEY_SUFFIX.search(name) + if match and f"@{match.group(1)}" not in live: + self._run(["set-option", "-u", "-t", session, name], check=False) + except (subprocess.SubprocessError, OSError, TmuxError) as exc: + # Reconcile is opportunistic; the mint must never fail on it. But a + # sweep that fails every launch leaks keys for the server's whole + # life, so the failure is at least visible. + print(f"warning: orphan-key sweep failed on {session}: {exc}", file=sys.stderr) + + def kill_window(self, target: str) -> None: + # Free the window's id-keyed session options before the kill: the ctl + # session outlives every run, so a leaked key lives as long as the + # server. Discovery is generic by the `_@` suffix — the backend + # must not know which option names callers use. Best-effort throughout: + # cleanup failure never blocks the kill. Known ceiling, accepted: the + # kill itself is best-effort, so a kill that then fails leaves a live + # window without its keys — the prune's untagged run-dir fallback still + # scopes the retry, and a lost return key just parks the window as-is. + # ponytail: one listing round-trip per kill, two for a name target + # (name-resolve, then keys; agent-window kills pay it too, for + # nothing); skip-by-session-name if that ever measures. + scope = self._option_scope(target) + if scope is not None: + session, digits = scope + suffix = f"_@{digits}" + try: + for name in self._scoped_options(session) or (): + if name.endswith(suffix): + self._run(["set-option", "-u", "-t", session, name], check=False) + except (subprocess.SubprocessError, OSError): + pass + super().kill_window(target) # What _qualified_window_id composes: `:@`. The session part # excludes `:` because that is exactly when qualification degrades to a bare diff --git a/tests/test_psmux_backend.py b/tests/test_psmux_backend.py index ac1c855f..7596e240 100644 --- a/tests/test_psmux_backend.py +++ b/tests/test_psmux_backend.py @@ -386,7 +386,8 @@ def test_return_target_bare_pane_on_unqualifiable_session_name(monkeypatch): def test_new_parked_window_composes_pwsh_source(rec, tmp_path): PsmuxMultiplexer().new_parked_window("s", "n", tmp_path, ["claude", "--resume"], "%3") - source = _pwsh_payload(rec.argv) + # calls[0]: the mint; the orphan-key sweep spawns after it (own test below) + source = _pwsh_payload(rec.calls[0][0]) prefix_end = source.index("& 'claude' '--resume'") assert "Remove-Item" in source[:prefix_end] # teammate-clear prelude first # A not-recognized command leaves $LASTEXITCODE unset but the source keeps @@ -397,11 +398,20 @@ def test_new_parked_window_composes_pwsh_source(rec, tmp_path): 'Write-Host "[bmad-loop exited $ec — press enter]"; Read-Host; ' in source ) # trailer: same tmux-family verbs as the POSIX one, pwsh control flow, - # issued through the psmux binary - assert "$ret = psmux show-options -wqv '%3' 2>$null; " in source + # issued through the psmux binary. The read is session-scoped and keyed by + # the window id the trailer probes for itself (#310) — `-wqv` is dead here. + assert "-wqv" not in source + assert ( + '$wid = "$(psmux display-message -p -t $env:TMUX_PANE' + " '#{window_id}' 2>$null)\".Trim(); " in source + ) + assert "if ($wid) { " in source + assert "$key = '%3_' + $wid; " in source + assert '$ret = "$(psmux show-options -qv $key 2>$null)".Trim(); ' in source assert "if ($ret -eq 'detach') { psmux detach-client 2>$null }" in source assert "psmux switch-client -t $ret 2>$null" in source assert "psmux switch-client -l 2>$null" in source + assert "psmux set-option -u $key 2>$null" in source # ------------------------------------------------------------------ pipe_pane @@ -597,9 +607,9 @@ def test_new_parked_window_falsy_id_passes_through(monkeypatch, tmp_path): def test_list_windows_qualifies_only_the_window_id_column(monkeypatch): - _rows_fake(monkeypatch, "@1\tshell\t\n@2\trun-x\tproj-tag\n") - rows = PsmuxMultiplexer().list_windows("ctl", ["window_id", "window_name", "@bmad_project"]) - assert rows == [("ctl:@1", "shell", ""), ("ctl:@2", "run-x", "proj-tag")] + _rows_fake(monkeypatch, "@1\tshell\n@2\trun-x\n") + rows = PsmuxMultiplexer().list_windows("ctl", ["window_id", "window_name"]) + assert rows == [("ctl:@1", "shell"), ("ctl:@2", "run-x")] def test_list_windows_without_id_column_is_untouched(monkeypatch): @@ -756,3 +766,450 @@ def test_tmux_backend_keeps_bare_tui_ids(monkeypatch, tmp_path): mux = TmuxMultiplexer() assert mux.new_parked_window("ctl", "run-x", tmp_path, ["prog"], "@ret") == "@2" assert mux.list_windows("ctl", ["window_id", "window_name"]) == [("@1", "shell")] + + +# ------------------------------------- per-window option channel (#310) + + +def _option_fake(monkeypatch, *, rows: str = "", value: str = "", rc: int = 0): + """Record every spawn; answer list-windows with `rows` and show-options with + `value`.""" + recorder = _RecordRun() + + def fake(argv, **kwargs): + recorder.calls.append((argv, kwargs)) + out = rows if argv[1] == "list-windows" else value + return subprocess.CompletedProcess(argv, rc, stdout=out, stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + return recorder + + +def test_set_window_option_writes_id_keyed_session_option(monkeypatch): + # The whole point: `-w` is dead on psmux, so the write goes to session scope + # with the window id in the KEY, routed by an explicit `-t `. + rec_ = _option_fake(monkeypatch) + PsmuxMultiplexer().set_window_option("ctl:@3", "@bmad_project", "C:/p") + assert rec_.argv[1:] == ["set-option", "-t", "ctl", "@bmad_project_@3", "C:/p"] + assert "-w" not in rec_.argv + + +def test_set_window_option_resolves_a_name_token(monkeypatch): + # set_return_pane passes `=session:`, not an id — the channel + # has to resolve it or the return target is recorded under no window at all. + rec_ = _option_fake(monkeypatch, rows="@1\tshell\n@4\trun-abc\n") + PsmuxMultiplexer().set_window_option("=ctl:run-abc", "@bmad_return_pane", "=ctl:%7") + assert rec_.calls[0][0][1] == "list-windows" + assert rec_.argv[1:] == ["set-option", "-t", "ctl", "@bmad_return_pane_@4", "=ctl:%7"] + + +def test_set_window_option_value_with_spaces_stays_one_argv_element(monkeypatch): + # project_tag() is an absolute path; on Windows it routinely holds spaces. + rec_ = _option_fake(monkeypatch) + PsmuxMultiplexer().set_window_option("ctl:@2", "@bmad_project", r"C:\Users\Some User\p") + assert rec_.argv[-1] == r"C:\Users\Some User\p" + + +def test_show_window_option_reads_the_id_keyed_session_option(monkeypatch): + rec_ = _option_fake(monkeypatch, value="C:/p\n") + assert PsmuxMultiplexer().show_window_option("ctl:@3", "@bmad_project") == "C:/p" + assert rec_.argv[1:] == ["show-options", "-qv", "-t", "ctl", "@bmad_project_@3"] + + +def test_show_window_option_miss_reads_empty(monkeypatch, capsys): + # A miss on live psmux is rc 0 + empty stdout (`-q` suppresses the line); + # it is a real answer, so no warning. + _option_fake(monkeypatch, value="", rc=0) + assert PsmuxMultiplexer().show_window_option("ctl:@3", "@bmad_return_pane") == "" + assert capsys.readouterr().err == "" + + +def test_show_window_option_transport_failure_warns(monkeypatch, capsys): + # rc≠0 (dead server / bad session) is NOT a miss. The ABC read can only + # degrade to "", but the failure must not be indistinguishable from unset. + _option_fake(monkeypatch, value="", rc=1) + assert PsmuxMultiplexer().show_window_option("ctl:@3", "@bmad_return_pane") == "" + assert "failed" in capsys.readouterr().err + + +def test_unset_window_option_failure_warns(monkeypatch, capsys): + # `-u` is a write: a silently failed unset leaves a live return key that + # replays the return move when the parked window's command exits. + rec_ = _option_fake(monkeypatch, rc=1) + PsmuxMultiplexer().unset_window_option("ctl:@3", "@bmad_return_pane") + assert rec_.calls # the unset was attempted + assert "failed" in capsys.readouterr().err + + +def test_unset_window_option_frees_the_key(monkeypatch): + # `-u` genuinely removes it (user_options.remove), so a later read is + # "unset" rather than an empty value that still occupies the map. + rec_ = _option_fake(monkeypatch) + PsmuxMultiplexer().unset_window_option("ctl:@3", "@bmad_return_pane") + assert rec_.argv[1:] == ["set-option", "-u", "-t", "ctl", "@bmad_return_pane_@3"] + + +@pytest.mark.parametrize("target", ["@3", "", "run-abc"]) +def test_option_verbs_refuse_an_unroutable_target(monkeypatch, capsys, target): + # No session means no `-t`, and without `-t` psmux routes by the + # most-recent-session fallback — the misrouting this change exists to stop. + # Declining loudly beats writing into an arbitrary server. Both mutating + # verbs take the same gate. + rec_ = _option_fake(monkeypatch) + mux = PsmuxMultiplexer() + mux.set_window_option(target, "@bmad_project", "C:/p") + mux.unset_window_option(target, "@bmad_project") + assert rec_.calls == [] + assert capsys.readouterr().err.count("does not resolve") == 2 + + +def test_set_window_option_unresolvable_name_token_warns(monkeypatch, capsys): + # A routable session but a name no window carries (died between listing and + # targeting): the resolve comes back empty and the write must decline, not + # invent a key. + rec_ = _option_fake(monkeypatch, rows="@1\tshell\n") + PsmuxMultiplexer().set_window_option("=ctl:run-gone", "@bmad_project", "C:/p") + assert [c[0][1] for c in rec_.calls] == ["list-windows"] + assert "does not resolve" in capsys.readouterr().err + + +def test_show_window_option_unroutable_target_reads_empty_with_warning(monkeypatch, capsys): + # "" already means "unset" to every caller, but the miss is still said out + # loud — an unroutable target sends no verb and warns on stderr, for reads + # the same as for writes. + rec_ = _option_fake(monkeypatch) + assert PsmuxMultiplexer().show_window_option("@3", "@bmad_project") == "" + assert rec_.calls == [] + assert "does not resolve" in capsys.readouterr().err + + +def test_builtin_window_option_still_takes_the_w_path(monkeypatch): + # The 14 real window options are NOT broken on psmux; only `@` names are. + # Rewriting `automatic-rename` to `automatic-rename_3` would break a + # working verb. + rec_ = _option_fake(monkeypatch) + PsmuxMultiplexer().set_window_option("ctl:@3", "automatic-rename", "off") + assert rec_.argv[1:] == ["set-option", "-w", "-t", "ctl:@3", "automatic-rename", "off"] + + +def test_list_windows_fills_option_columns_per_window(monkeypatch): + # `#{@bmad_project}` expands from the single per-server map, so asking psmux + # for it hands every row the same value and the prune cannot discriminate. + # The column is filled per window id instead, from ONE full option listing + # (a flat extra call however many rows or `@` columns there are). + listing = '@bmad_project_@2 "proj-b"\n@bmad_project_@3 "proj-a"\n' + seen = [] + + def fake(argv, **kwargs): + seen.append(argv) + if argv[1] == "list-windows": + out = "@1\tshell\t@1\n@2\trun-x\t@2\n@3\trun-y\t@3\n" + else: + out = listing + return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + rows = PsmuxMultiplexer().list_windows("ctl", ["window_id", "window_name", "@bmad_project"]) + assert rows == [ + ("ctl:@1", "shell", ""), # no key in the listing = unset + ("ctl:@2", "run-x", "proj-b"), + ("ctl:@3", "run-y", "proj-a"), + ] + # the `@` field never reaches psmux's format string — that expansion is the bug + assert "#{@bmad_project}" not in seen[0][-1] + assert seen[0][-1] == "#{window_id}\t#{window_name}\t#{window_id}" + assert len(seen) == 2 # the row listing + one option listing, nothing per-row + + +def test_list_windows_without_option_column_spawns_one_call(monkeypatch): + # No `@` field means no per-row fill: the prune's common path must not pay + # N extra round-trips for nothing. + rec_ = _option_fake(monkeypatch, rows="@1\tshell\n") + PsmuxMultiplexer().list_windows("ctl", ["window_id", "window_name"]) + assert len(rec_.calls) == 1 + + +def test_tmux_backend_keeps_real_window_options(monkeypatch): + # The divergence is psmux-only. tmux has genuine per-window user options, so + # rewriting the key there would move state to a place nothing reads. + rec_ = _option_fake(monkeypatch, value="C:/p\n") + mux = TmuxMultiplexer() + mux.set_window_option("ctl:@3", "@bmad_project", "C:/p") + assert rec_.argv[1:] == ["set-option", "-w", "-t", "ctl:@3", "@bmad_project", "C:/p"] + assert mux.show_window_option("ctl:@3", "@bmad_project") == "C:/p" + assert rec_.argv[1:] == ["show-options", "-wqv", "-t", "ctl:@3", "@bmad_project"] + + +def test_kill_window_frees_only_that_windows_keys(monkeypatch): + # Generic by `_@` suffix: both bmad keys of @3 go, @13's key (a + # suffix near-miss), @1's key and a foreign config option (`@color_3`, no + # `_@` shape) stay, and the kill itself still fires. + listing = ( + '@bmad_project_@3 "proj-a"\n' + '@bmad_return_pane_@3 "=ctl:%7"\n' + '@bmad_project_@13 "proj-b"\n' + '@bmad_project_@1 "proj-c"\n' + '@color_3 "cfg"\n' + "mouse on\n" + ) + recorder = _RecordRun() + + def fake(argv, **kwargs): + recorder.calls.append((argv, kwargs)) + out = listing if argv[1] == "show-options" else "" + return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + PsmuxMultiplexer().kill_window("ctl:@3") + unset = [c[0][5] for c in recorder.calls if c[0][1] == "set-option"] + assert sorted(unset) == ["@bmad_project_@3", "@bmad_return_pane_@3"] + assert recorder.argv[1:] == ["kill-window", "-t", "ctl:@3"] + + +def test_kill_window_cleanup_failure_never_blocks_the_kill(monkeypatch): + sent = [] + + def fake(argv, **kwargs): + if argv[1] == "show-options": + raise subprocess.TimeoutExpired(argv, 1) + sent.append(argv) + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + PsmuxMultiplexer().kill_window("ctl:@3") # must not raise + assert sent and sent[-1][1:] == ["kill-window", "-t", "ctl:@3"] + + +def test_kill_window_unroutable_target_skips_cleanup_but_kills(monkeypatch): + rec_ = _option_fake(monkeypatch) + PsmuxMultiplexer().kill_window("@3") + assert [c[0][1] for c in rec_.calls] == ["kill-window"] + + +@pytest.mark.parametrize( + "value", + [ + "a;b", # server splits top-level `;` — remainder would EXECUTE + "-flag", # dropped as a flag server-side (or flips to unset via `u`) + "it's", # bare `'` opens a quote in the server tokenizer + 'x"y', # bare `"` toggles quoting + "bad\nline", # control line is `\n`-terminated — command injection + "", # empty write is a silent server-side no-op; unset exists for this + "\\\\srv\\share My Proj", # spaced → client double-quotes → `\\` collapses + "C:\\dir with space\\", # spaced + trailing `\` → `\"` eats the close + 'x " y', # spaced + `"`: after a `\` the client's `\"` closes the wrapper + "a ; b", # standalone `;` token splits even inside the client's quotes + "a \\; b", # standalone `\;` token splits the same way + " C:\\p x", # leading space survives the wire but this backend's reads strip + "C:\\p x ", # trailing space, same round-trip loss + ], +) +def test_set_window_option_refuses_untransportable_values(monkeypatch, capsys, value): + # The CLI→server hop is lossy for these shapes (verified against psmux's + # client re-quoting, chain splitter and server tokenizer at v3.3.7; the + # `;`-token corruption reproduced on a live 3.3.7: `a ; b` stores as `a`). + # A corrupted stored tag never equals project_tag again — the window turns + # silently unprunable — so refuse loudly instead of storing garbage. + rec_ = _option_fake(monkeypatch) + PsmuxMultiplexer().set_window_option("ctl:@3", "@bmad_project", value) + assert rec_.calls == [] + assert "transport" in capsys.readouterr().err + + +@pytest.mark.parametrize("value", ["a; b", "C:\\Users\\O'Brien Files\\proj"]) +def test_set_window_option_accepts_quote_safe_spaced_values(monkeypatch, value): + # Inside the client's double quotes `'` is literal and a mid-token `;` + # survives the whitespace-token chain splitter (live-verified: `a; b` + # round-trips on 3.3.7) — so these MUST pass. A blanket `;`/`'` ban would + # silently untag every project path with an apostrophe. + rec_ = _option_fake(monkeypatch) + PsmuxMultiplexer().set_window_option("ctl:@3", "@bmad_project", value) + assert rec_.argv[-1] == value + + +def test_set_window_option_write_failure_warns(monkeypatch, capsys): + # A tag that silently failed to land re-opens the mis-scoped prune: the + # window reads as untagged and falls into the run-dir fallback. + rec_ = _option_fake(monkeypatch, rc=1) + PsmuxMultiplexer().set_window_option("ctl:@3", "@bmad_project", "C:/p") + assert rec_.calls # the write was attempted + assert "failed" in capsys.readouterr().err + + +def test_list_windows_option_read_failure_poisons_not_unset(monkeypatch, capsys): + # error ≠ unset: "" would drop a tagged window into the prune's untagged + # run-dir fallback (kill-eligible); the poison tag reads as some other + # project's window and is skipped — the safe direction for a kill scan. + # The failure also warns: without it a prune --dry-run reports "nothing + # prunable" with no trace of why. + def fake(argv, **kwargs): + if argv[1] == "show-options": + raise subprocess.TimeoutExpired(argv, 1) + return subprocess.CompletedProcess(argv, 0, stdout="@1\tshell\t@1\n", stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + rows = PsmuxMultiplexer().list_windows("ctl", ["window_id", "window_name", "@bmad_project"]) + assert rows == [("ctl:@1", "shell", PsmuxMultiplexer._OPT_READ_FAILED)] + assert PsmuxMultiplexer._OPT_READ_FAILED # non-empty, or the fallback fires + assert "listing failed" in capsys.readouterr().err + + +def test_list_windows_malformed_id_probe_poisons_not_unset(monkeypatch): + # A probe value that is not a bare `@N` is a malfunction, the same failure + # class as a dead listing — it must poison (skip), not read as "untagged" + # (kill-eligible via the run-dir fallback). + rec_ = _option_fake(monkeypatch, rows="weird\tshell\tweird\n", value="") + rows = PsmuxMultiplexer().list_windows("ctl", ["window_id", "window_name", "@bmad_project"]) + assert rows[0][2] == PsmuxMultiplexer._OPT_READ_FAILED + assert rec_.calls # the listing was still attempted + + +def test_list_windows_option_fill_declines_on_unroutable_session(monkeypatch): + # The #221 degrade: `-t a:b` would route to server `a`. The id columns + # degrade to bare ids; the option fill must decline the same way — "" with + # no reads issued — rather than present another server's value as a tag. + rec_ = _option_fake(monkeypatch, rows="@1\tshell\t@1\n") + rows = PsmuxMultiplexer().list_windows("a:b", ["window_id", "window_name", "@bmad_project"]) + assert rows == [("@1", "shell", "")] + assert len(rec_.calls) == 1 # the listing only — no show-options spawned + + +def test_new_parked_window_sweeps_orphan_keys(monkeypatch, tmp_path): + # Enter-dismissing a parked window closes it without kill_window, so its + # keys outlive it; launch reconciles. `_@7` has no window → freed. `_@2` is + # live → kept. `@color_3` is a foreign config option (no `_@`) → untouched. + listing = '@bmad_project_@7 "gone"\n@bmad_project_@2 "live"\n@color_3 "cfg"\n' + calls = [] + + def fake(argv, **kwargs): + calls.append(argv) + out = {"new-window": "@2\n", "list-windows": "@1\n@2\n", "show-options": listing}.get( + argv[1], "" + ) + return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + win = PsmuxMultiplexer().new_parked_window("ctl", "run-x", tmp_path, ["prog"], "@ret") + assert win == "ctl:@2" + unset = [c[5] for c in calls if c[1] == "set-option"] + assert unset == ["@bmad_project_@7"] + + +def test_tmux_backend_forwards_option_columns_to_format(monkeypatch): + # The tmux prune path depends on `#{@bmad_project}` reaching the -F format + # string — real per-window options expand per row there. Only psmux + # synthesizes the column. + recorder = _RecordRun(stdout="@1\tshell\tproj\n") + monkeypatch.setattr(tmux_base.subprocess, "run", recorder) + rows = TmuxMultiplexer().list_windows("ctl", ["window_id", "window_name", "@bmad_project"]) + assert "#{@bmad_project}" in recorder.argv[-1] + assert rows == [("@1", "shell", "proj")] + + +@pytest.mark.parametrize("value", ["nb\u00a0sp", "tab\there", "with space\u00a0nb"]) +def test_set_window_option_refuses_non_ascii_whitespace(monkeypatch, capsys, value): + # The client quotes only on ASCII space while the server tokenizer splits + # on Unicode whitespace — an NBSP/tab in an unquoted value splits the token. + rec_ = _option_fake(monkeypatch) + PsmuxMultiplexer().set_window_option("ctl:@3", "@bmad_project", value) + assert rec_.calls == [] + assert "transport" in capsys.readouterr().err + + +def test_sweep_snapshots_keys_before_live_windows(monkeypatch, tmp_path): + # Order is the race guard: a window minted-and-tagged between the two + # listings must have its key OUTSIDE the snapshot, so it can never read as + # a false orphan. Pin the call order. + order = [] + + def fake(argv, **kwargs): + order.append(argv[1]) + out = {"new-window": "@2\n", "list-windows": "@1\n@2\n", "show-options": ""}.get( + argv[1], "" + ) + return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + PsmuxMultiplexer().new_parked_window("ctl", "run-x", tmp_path, ["prog"], "@ret") + sweep = [v for v in order if v in ("show-options", "list-windows")] + assert sweep == ["show-options", "list-windows"] + + +def test_sweep_treats_empty_live_list_as_failed_probe(monkeypatch, tmp_path): + # We just minted a window in this session, so an empty live listing is a + # failed probe, not an empty session — believing it would sweep every key, + # live windows included. + listing = '@bmad_project_@2 "live"\n@bmad_project_@7 "gone"\n' + calls = [] + + def fake(argv, **kwargs): + calls.append(argv) + if argv[1] == "list-windows": + return subprocess.CompletedProcess(argv, 1, stdout="", stderr="no server") + out = {"new-window": "@2\n", "show-options": listing}.get(argv[1], "") + return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + PsmuxMultiplexer().new_parked_window("ctl", "run-x", tmp_path, ["prog"], "@ret") + assert [c[1] for c in calls if c[1] == "set-option"] == [] + + +def test_sweep_transport_exception_never_fails_the_mint(monkeypatch, tmp_path, capsys): + # The live-window probe can RAISE (the base converts a timeout to + # TmuxError), not just answer empty — the sweep must swallow it (with a + # trace) and the mint must still hand back the window id. + def fake(argv, **kwargs): + if argv[1] == "list-windows": + raise subprocess.TimeoutExpired(argv, 1) + out = {"new-window": "@2\n", "show-options": '@bmad_project_@7 "gone"\n'}.get(argv[1], "") + return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + win = PsmuxMultiplexer().new_parked_window("ctl", "run-x", tmp_path, ["prog"], "@ret") + assert win == "ctl:@2" + assert "sweep failed" in capsys.readouterr().err + + +@pytest.mark.parametrize("value", ["a; b", "C:\\Users\\O'Brien Files\\proj", "two spaces", "C:/p"]) +def test_accepted_values_round_trip_through_the_listing_parse(monkeypatch, value): + # The write gate and the read parser are two halves of one invariant: + # every accepted value must read back IDENTICAL from the `@key "value"` + # listing shape psmux emits — else the prune's equality compare breaks. + assert PsmuxMultiplexer._transportable(value) + _option_fake(monkeypatch, value=f'@bmad_project_@3 "{value}"\n') + options = PsmuxMultiplexer()._scoped_options("ctl") + assert options == {"@bmad_project_@3": value} + + +def test_ctl_prune_scan_discriminates_projects_end_to_end(monkeypatch, tmp_path): + # Acceptance-level: the real _ctl_window_candidates over the real psmux + # backend (subprocess faked) — only this project's dead-run window is a + # candidate; the other project's window and the shell window survive. + from bmad_loop import runs + from bmad_loop.tui import launch + + mine = str(tmp_path.resolve()) + listing = f'@bmad_project_@2 "{mine}"\n@bmad_project_@3 "C:/elsewhere"\n' + + def fake(argv, **kwargs): + if argv[1] == "list-windows": + out = "@1\tshell\t@1\n@2\trun-20260726-1\t@2\n@3\trun-20260726-2\t@3\n" + elif argv[1] == "show-options": + out = listing + elif argv[1] == "has-session": + out = "" + else: + out = "" + return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") + + mux = PsmuxMultiplexer() + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + monkeypatch.setattr(mux, "available", lambda: True) + monkeypatch.setattr(mux, "current_window_id", lambda: None) # outside any pane + monkeypatch.setattr(launch, "get_multiplexer", lambda: mux) + monkeypatch.setattr(launch, "mux_usable", lambda _mux: True) + monkeypatch.setattr(runs, "is_run", lambda _dir: True) + monkeypatch.setattr(runs, "engine_alive", lambda _dir: False) + + candidates = launch._ctl_window_candidates(tmp_path) + assert candidates == [("bmad-loop-ctl:@2", "run-20260726-1")] From d8f8d93520299647442fe1adf2d3ad12cef3209e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Sun, 26 Jul 2026 18:52:59 +0200 Subject: [PATCH 2/5] fix(adapters): free the stale key when a psmux option write is refused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A refused rewrite left the prior value live — a stale parked return target could replay later. The refusal now unsets the key so the option reads as unset, matching the reworded warning (#315 review). Also: pin (source + round-trip test) that a spaceless UNC path rides the unquoted hop verbatim — the tokenizer's backslash collapse fires only inside double quotes — and de-tautologize the empty-live-list sweep assertion. --- src/bmad_loop/adapters/psmux_backend.py | 12 ++++++++++-- tests/test_psmux_backend.py | 15 ++++++++++----- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/bmad_loop/adapters/psmux_backend.py b/src/bmad_loop/adapters/psmux_backend.py index d2f79421..65119b8c 100644 --- a/src/bmad_loop/adapters/psmux_backend.py +++ b/src/bmad_loop/adapters/psmux_backend.py @@ -443,7 +443,11 @@ def _transportable(value: str) -> bool: # (live-verified on 3.3.7; psmux/psmux#499). `\\` collapses and a `"` can close the # wrapper early (client-escaped `\"` after a backslash reads back as # `\\` + closing quote). So the spaced branch refuses `"`, `\\`, a - # trailing `\`, and standalone `;`/`\;` tokens. + # trailing `\`, and standalone `;`/`\;` tokens. Outside double quotes + # the tokenizer's escape branch never fires (commands.rs:690 requires + # in_double_quotes) — an unquoted backslash is pushed literally, psmux + # being Windows-native — so the unspaced branch does not ban `\\` and a + # spaceless UNC path (`\\srv\share`) rides verbatim. # Non-ASCII-space whitespace (NBSP, tab, …) is refused outright: the # client quotes only on ASCII `' '` (main.rs `s.contains(' ')`) while # the server tokenizer splits on Unicode `is_whitespace()` — an NBSP in @@ -467,9 +471,13 @@ def set_window_option(self, target: str, option: str, value: str) -> None: if not self._transportable(value): print( f"warning: set-option {option} skipped — value does not survive " - "psmux's control-line transport verbatim; the option stays unset", + "psmux's control-line transport verbatim; the key is freed and " + "the option reads as unset", file=sys.stderr, ) + # Free any prior value: a refused REwrite must not leave the stale + # one to be replayed later (e.g. a parked return target). + self.unset_window_option(target, option) return scope = self._option_scope(target) if scope is None: diff --git a/tests/test_psmux_backend.py b/tests/test_psmux_backend.py index 7596e240..317a9333 100644 --- a/tests/test_psmux_backend.py +++ b/tests/test_psmux_backend.py @@ -1009,10 +1009,12 @@ def test_set_window_option_refuses_untransportable_values(monkeypatch, capsys, v # client re-quoting, chain splitter and server tokenizer at v3.3.7; the # `;`-token corruption reproduced on a live 3.3.7: `a ; b` stores as `a`). # A corrupted stored tag never equals project_tag again — the window turns - # silently unprunable — so refuse loudly instead of storing garbage. + # silently unprunable — so refuse loudly instead of storing garbage. The + # refusal also frees any prior value: a refused REwrite must read as unset, + # not replay the stale value (e.g. an old parked return target). rec_ = _option_fake(monkeypatch) PsmuxMultiplexer().set_window_option("ctl:@3", "@bmad_project", value) - assert rec_.calls == [] + assert [c[0][1:4] for c in rec_.calls] == [["set-option", "-u", "-t"]] assert "transport" in capsys.readouterr().err @@ -1112,7 +1114,7 @@ def test_set_window_option_refuses_non_ascii_whitespace(monkeypatch, capsys, val # on Unicode whitespace — an NBSP/tab in an unquoted value splits the token. rec_ = _option_fake(monkeypatch) PsmuxMultiplexer().set_window_option("ctl:@3", "@bmad_project", value) - assert rec_.calls == [] + assert [c[0][1:4] for c in rec_.calls] == [["set-option", "-u", "-t"]] assert "transport" in capsys.readouterr().err @@ -1151,7 +1153,7 @@ def fake(argv, **kwargs): monkeypatch.setattr(tmux_base.subprocess, "run", fake) PsmuxMultiplexer().new_parked_window("ctl", "run-x", tmp_path, ["prog"], "@ret") - assert [c[1] for c in calls if c[1] == "set-option"] == [] + assert not [c for c in calls if c[1] == "set-option"] def test_sweep_transport_exception_never_fails_the_mint(monkeypatch, tmp_path, capsys): @@ -1170,7 +1172,10 @@ def fake(argv, **kwargs): assert "sweep failed" in capsys.readouterr().err -@pytest.mark.parametrize("value", ["a; b", "C:\\Users\\O'Brien Files\\proj", "two spaces", "C:/p"]) +@pytest.mark.parametrize( + "value", + ["a; b", "C:\\Users\\O'Brien Files\\proj", "two spaces", "C:/p", "\\\\srv\\share"], +) def test_accepted_values_round_trip_through_the_listing_parse(monkeypatch, value): # The write gate and the read parser are two halves of one invariant: # every accepted value must read back IDENTICAL from the `@key "value"` From 363afcb37b4e414876b23f37c88f5748e480d714 Mon Sep 17 00:00:00 2001 From: pbean Date: Sun, 26 Jul 2026 14:17:34 -0700 Subject: [PATCH 3/5] fix(adapters): degrade psmux option columns to unset (#310 review) The read-failure poison marker guarded a hazard the caller's untagged fallback already covers: the ctl prune claims an untagged window only when its run dir exists under this project, and run ids are unique by construction, so "" is ownership-safe in both directions. It was also inconsistent with the channel's own documented hole - a psmux server handler can answer the option listing empty-with-success while keys exist, and that path filled "" anyway. Degrade to "unset" everywhere, keep the stderr warning, and drop the cross-backend rule that an option column may carry a backend-private opaque marker. --- src/bmad_loop/adapters/multiplexer.py | 8 +------ src/bmad_loop/adapters/psmux_backend.py | 32 ++++++++++++------------- tests/test_psmux_backend.py | 24 +++++++++---------- 3 files changed, 28 insertions(+), 36 deletions(-) diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index e7265857..7aaddf4e 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -182,13 +182,7 @@ def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]] probe, so a sentinel is safe). A ``window_id`` column carries the same id form :meth:`current_window_id` - returns; core compares the two directly. - - An ``@``-prefixed option column may carry a backend-private opaque - marker meaning "the read FAILED" (distinct from ``""`` = unset). - Callers must only ever compare such a column against their own known - values — a non-matching value means "not mine", never a displayable - tag.""" + returns; core compares the two directly.""" @abstractmethod def window_alive(self, session: str, window_id: str) -> bool: diff --git a/src/bmad_loop/adapters/psmux_backend.py b/src/bmad_loop/adapters/psmux_backend.py index 65119b8c..fb86126c 100644 --- a/src/bmad_loop/adapters/psmux_backend.py +++ b/src/bmad_loop/adapters/psmux_backend.py @@ -295,11 +295,14 @@ def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]] degraded = not session or ":" in session options = self._scoped_options(session) if opt_columns and not degraded else None if opt_columns and not degraded and options is None: - # Without this line the failure is invisible: every fill poisons, - # so a prune dry-run would report nothing prunable with no trace. + # Say it: every column degrades to "unset" at once, so a prune reads + # as if nothing were ever tagged. Visible on the CLI prune (cli.py, + # incl. the --dry-run this most affects); NOT under the TUI, which + # captures stderr for the app's whole run (tui/app.py's run_tui + # note) — the select_window precedent below, same deliberate ceiling. print( f"warning: show-options listing failed on {session}; " - "option columns filled with a read-failure marker", + "option columns read as unset", file=sys.stderr, ) out: list[tuple[str, ...]] = [] @@ -308,12 +311,15 @@ def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]] for i, option in opt_columns.items(): # values[i] is the bare `@N` probed in the option column's place. digits = self._id_digits(values[i]) - if degraded: + if degraded or not digits or options is None: + # Unreadable degrades to "" ("unset") rather than to a + # "read failed" sentinel: the caller's untagged fallback + # proves ownership on its own (the ctl prune claims an + # untagged window only when the run dir exists under THIS + # project, and run ids are unique — runs.new_run_id), so a + # third state in the column would buy only the skip of our + # own dead window. values[i] = "" - elif not digits or options is None: - # A failed id probe or failed listing (≠ every key unset) — - # see _OPT_READ_FAILED. - values[i] = self._OPT_READ_FAILED else: values[i] = options.get(self._scoped_option_key(option, digits), "") for i in id_columns: @@ -395,16 +401,10 @@ def _scoped_option_key(option: str, digits: str) -> str: # matches (see the namespace note in the channel comment above). _KEY_SUFFIX = re.compile(r"_@(\d+)$") - # Fill value for an option column whose read FAILED (≠ unset). Non-empty on - # purpose: "" would drop a genuinely-tagged window into the prune's - # untagged run-dir fallback, while an impossible tag reads as "some other - # project" and the window is skipped — the safe direction for a kill scan. - _OPT_READ_FAILED = "\x00option-read-failed" - def _read_scoped(self, session: str, option: str, digits: str) -> str | None: # No `-w`: the session-scoped read is the one that reaches the map. - # None = transport failure, "" = unset; only list_windows' fill keeps - # the distinction, the ABC read below collapses both to "". + # None = transport failure, "" = unset. The ABC read admits only "", so + # the caller collapses them — the distinction survives as a warning. try: proc = self._run( ["show-options", "-qv", "-t", session, self._scoped_option_key(option, digits)], diff --git a/tests/test_psmux_backend.py b/tests/test_psmux_backend.py index 317a9333..a866dd5a 100644 --- a/tests/test_psmux_backend.py +++ b/tests/test_psmux_backend.py @@ -1038,12 +1038,12 @@ def test_set_window_option_write_failure_warns(monkeypatch, capsys): assert "failed" in capsys.readouterr().err -def test_list_windows_option_read_failure_poisons_not_unset(monkeypatch, capsys): - # error ≠ unset: "" would drop a tagged window into the prune's untagged - # run-dir fallback (kill-eligible); the poison tag reads as some other - # project's window and is skipped — the safe direction for a kill scan. - # The failure also warns: without it a prune --dry-run reports "nothing - # prunable" with no trace of why. +def test_list_windows_option_read_failure_degrades_to_unset_with_a_warning(monkeypatch, capsys): + # A failed listing reads as "untagged", the same answer a genuinely untagged + # window gives — safe because _ctl_window_candidates only claims an untagged + # window whose run dir exists under THIS project, and run ids are unique. The + # failure still warns: without it a prune --dry-run reports "nothing + # prunable" with no trace of why. The other columns must survive intact. def fake(argv, **kwargs): if argv[1] == "show-options": raise subprocess.TimeoutExpired(argv, 1) @@ -1051,18 +1051,16 @@ def fake(argv, **kwargs): monkeypatch.setattr(tmux_base.subprocess, "run", fake) rows = PsmuxMultiplexer().list_windows("ctl", ["window_id", "window_name", "@bmad_project"]) - assert rows == [("ctl:@1", "shell", PsmuxMultiplexer._OPT_READ_FAILED)] - assert PsmuxMultiplexer._OPT_READ_FAILED # non-empty, or the fallback fires + assert rows == [("ctl:@1", "shell", "")] # id column still qualified assert "listing failed" in capsys.readouterr().err -def test_list_windows_malformed_id_probe_poisons_not_unset(monkeypatch): - # A probe value that is not a bare `@N` is a malfunction, the same failure - # class as a dead listing — it must poison (skip), not read as "untagged" - # (kill-eligible via the run-dir fallback). +def test_list_windows_malformed_id_probe_degrades_to_unset(monkeypatch): + # A probe value that is not a bare `@N` yields no key to look up, so the + # column reads unset — same degrade as a dead listing. rec_ = _option_fake(monkeypatch, rows="weird\tshell\tweird\n", value="") rows = PsmuxMultiplexer().list_windows("ctl", ["window_id", "window_name", "@bmad_project"]) - assert rows[0][2] == PsmuxMultiplexer._OPT_READ_FAILED + assert rows[0][2] == "" assert rec_.calls # the listing was still attempted From 443bcf330de53157c71d7e8f83737a2b6e570d58 Mon Sep 17 00:00:00 2001 From: pbean Date: Sun, 26 Jul 2026 14:17:54 -0700 Subject: [PATCH 4/5] fix(adapters): resolve psmux option scope before the transport gate A refused value re-entered unset_window_option to free the stale key, so an unroutable target warned twice - once about the transport, once about a `set-option -u` the caller never issued. Resolve the scope first (an unroutable target has nothing to write and nothing to free), then gate the value, and free the key through the resolved scope. The two mutating verbs now share one spawn-and-warn body. --- src/bmad_loop/adapters/psmux_backend.py | 60 ++++++++++++------------- tests/test_psmux_backend.py | 8 ++++ 2 files changed, 36 insertions(+), 32 deletions(-) diff --git a/src/bmad_loop/adapters/psmux_backend.py b/src/bmad_loop/adapters/psmux_backend.py index fb86126c..a0716903 100644 --- a/src/bmad_loop/adapters/psmux_backend.py +++ b/src/bmad_loop/adapters/psmux_backend.py @@ -464,10 +464,34 @@ def _transportable(value: str) -> bool: return all(tok not in (";", "\\;") for tok in value.split()) return not any(c in value for c in ";'\"") + def _write_scoped(self, verb: list[str], key: str) -> None: + # Both mutating verbs, one body. A write that silently failed re-opens + # the mis-scoped prune this channel exists to close, and a silently + # failed `-u` leaves a live return key that replays the return move when + # the window's command exits — so failures are said out loud either way + # (the verbs stay best-effort: warn, never raise). + label = " ".join(verb[: verb.index("-t")]) # `set-option` / `set-option -u` + try: + proc = self._run(verb, check=False) + if proc.returncode != 0: + print(f"warning: {label} {key} failed: {proc.stderr.strip()}", file=sys.stderr) + except (subprocess.SubprocessError, OSError) as exc: + print(f"warning: {label} {key} failed: {exc}", file=sys.stderr) + def set_window_option(self, target: str, option: str, value: str) -> None: if not option.startswith("@"): super().set_window_option(target, option, value) return + # Scope first, transport second: an unroutable target has nothing to + # write AND nothing to free, and resolving here keeps the refusal path + # from re-entering unset_window_option and warning twice about a verb + # the caller never issued. + scope = self._option_scope(target) + if scope is None: + self._warn_unroutable("set-option", target, option) + return + session, digits = scope + key = self._scoped_option_key(option, digits) if not self._transportable(value): print( f"warning: set-option {option} skipped — value does not survive " @@ -477,26 +501,9 @@ def set_window_option(self, target: str, option: str, value: str) -> None: ) # Free any prior value: a refused REwrite must not leave the stale # one to be replayed later (e.g. a parked return target). - self.unset_window_option(target, option) - return - scope = self._option_scope(target) - if scope is None: - self._warn_unroutable("set-option", target, option) + self._write_scoped(["set-option", "-u", "-t", session, key], key) return - session, digits = scope - # A write that silently failed re-opens the mis-scoped prune this - # channel exists to close, so failures are said out loud (the verbs - # stay best-effort — warn, never raise). - key = self._scoped_option_key(option, digits) - try: - proc = self._run(["set-option", "-t", session, key, value], check=False) - if proc.returncode != 0: - print( - f"warning: set-option {key} failed: {proc.stderr.strip()}", - file=sys.stderr, - ) - except (subprocess.SubprocessError, OSError) as exc: - print(f"warning: set-option {key} failed: {exc}", file=sys.stderr) + self._write_scoped(["set-option", "-t", session, key, value], key) def unset_window_option(self, target: str, option: str) -> None: if not option.startswith("@"): @@ -508,20 +515,9 @@ def unset_window_option(self, target: str, option: str) -> None: return session, digits = scope # `-u` genuinely frees the key: the server's SetOptionUnset handler - # removes `@`-prefixed names from the map (verified at v3.3.7). `-u` is - # a write like set — a silently failed unset leaves a live return key - # that replays the return move when the window's command exits, so - # failures are said out loud here too (warn, never raise). + # removes `@`-prefixed names from the map (verified at v3.3.7). key = self._scoped_option_key(option, digits) - try: - proc = self._run(["set-option", "-u", "-t", session, key], check=False) - if proc.returncode != 0: - print( - f"warning: set-option -u {key} failed: {proc.stderr.strip()}", - file=sys.stderr, - ) - except (subprocess.SubprocessError, OSError) as exc: - print(f"warning: set-option -u {key} failed: {exc}", file=sys.stderr) + self._write_scoped(["set-option", "-u", "-t", session, key], key) def show_window_option(self, target: str, option: str) -> str: if not option.startswith("@"): diff --git a/tests/test_psmux_backend.py b/tests/test_psmux_backend.py index a866dd5a..e2e4f090 100644 --- a/tests/test_psmux_backend.py +++ b/tests/test_psmux_backend.py @@ -861,6 +861,14 @@ def test_option_verbs_refuse_an_unroutable_target(monkeypatch, capsys, target): mux.unset_window_option(target, "@bmad_project") assert rec_.calls == [] assert capsys.readouterr().err.count("does not resolve") == 2 + # An unroutable target with an untransportable value warns ONCE: the scope + # gate runs first, so the refusal never re-enters the unset verb and + # reports a `set-option -u` the caller never issued. + mux.set_window_option(target, "@bmad_project", "a ; b") + err = capsys.readouterr().err + assert err.count("does not resolve") == 1 + assert "transport" not in err + assert rec_.calls == [] def test_set_window_option_unresolvable_name_token_warns(monkeypatch, capsys): From 99385fee1b5f1c2aff148ea8563de775636ac580 Mon Sep 17 00:00:00 2001 From: pbean Date: Sun, 26 Jul 2026 14:17:54 -0700 Subject: [PATCH 5/5] docs(adapters): pin the spaceless-apostrophe transport refusal The docs cited only a UNC-and-spaced path, but the psmux client quotes a value only when it holds an ASCII space and `'` is literal inside those quotes - so `C:\Users\O'Brien\dev` is refused while its spaced sibling is accepted. Name the inversion and pin it with a parametrize case. Also point the module docstring at the channel block instead of restating the model, and drop a stray word from the kill_window cost note. --- docs/multiplexer-backends.md | 10 +++++++--- src/bmad_loop/adapters/psmux_backend.py | 13 ++++++------- tests/test_psmux_backend.py | 3 +++ 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/docs/multiplexer-backends.md b/docs/multiplexer-backends.md index 9d40d8a2..d95ff6b4 100644 --- a/docs/multiplexer-backends.md +++ b/docs/multiplexer-backends.md @@ -57,9 +57,13 @@ by a session-scoped option whose key carries the window id (`@bmad_project_@3` f Both are properties of psmux's model, not gaps awaiting an upstream release. Practical consequence: such a value is **not** readable via `psmux show-options -w` by hand — read it with `psmux show-options -qv -t "@bmad_project_@N"` instead. One visible limit: a value -that cannot survive psmux's control-line transport verbatim (e.g. a project path that is both -UNC and spaced) is refused with a stderr warning at every launch, and that project's windows -stay untagged — the prune then scopes them through the run-dir fallback instead of the tag. +that cannot survive psmux's control-line transport verbatim is refused with a stderr warning at +every launch, and that project's windows stay untagged — the prune then scopes them through the +run-dir fallback instead of the tag. Which paths those are is counter-intuitive, because the +psmux client quotes a value only when it contains an ASCII space and `'` is literal inside those +quotes: `C:\Users\O'Brien\dev` is **refused** while `C:\Users\O'Brien Files\dev` is accepted, and +a spaced UNC path (`\\server\share\My Proj`) is refused while the spaceless `\\server\share\proj` +is accepted. ## External backends diff --git a/src/bmad_loop/adapters/psmux_backend.py b/src/bmad_loop/adapters/psmux_backend.py index a0716903..aecadf47 100644 --- a/src/bmad_loop/adapters/psmux_backend.py +++ b/src/bmad_loop/adapters/psmux_backend.py @@ -21,12 +21,11 @@ ``select_window`` is the one verb that cannot take that form: psmux validates a scoped target's window part CLI-side against window index/name only, so the override resolves the id -back to an index first. Per-window user options do not exist at all — -psmux keeps one user-option scope per server and every ``-w`` read of an -``@``-prefixed name is empty — so the window-option verbs, the -``@``-prefixed columns of ``list_windows`` and the parked trailer route -through a session-scoped option whose key carries the window id (#310). -``available()`` additionally gates on +back to an index first. Per-window user options do not exist at all, so +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 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 @@ -603,7 +602,7 @@ def kill_window(self, target: str) -> None: # kill itself is best-effort, so a kill that then fails leaves a live # window without its keys — the prune's untagged run-dir fallback still # scopes the retry, and a lost return key just parks the window as-is. - # ponytail: one listing round-trip per kill, two for a name target + # Cost, accepted: one listing round-trip per kill, two for a name target # (name-resolve, then keys; agent-window kills pay it too, for # nothing); skip-by-session-name if that ever measures. scope = self._option_scope(target) diff --git a/tests/test_psmux_backend.py b/tests/test_psmux_backend.py index e2e4f090..d73a9824 100644 --- a/tests/test_psmux_backend.py +++ b/tests/test_psmux_backend.py @@ -1010,6 +1010,9 @@ def test_kill_window_unroutable_target_skips_cleanup_but_kills(monkeypatch): "a \\; b", # standalone `\;` token splits the same way " C:\\p x", # leading space survives the wire but this backend's reads strip "C:\\p x ", # trailing space, same round-trip loss + "C:\\Users\\O'Brien\\dev", # SPACELESS `'` — unquoted, so the server tokenizer + # opens a quote on it. The spaced sibling below is accepted; the docs call + # out the inversion because an apostrophe in a Windows home is common. ], ) def test_set_window_option_refuses_untransportable_values(monkeypatch, capsys, value):