diff --git a/CHANGELOG.md b/CHANGELOG.md index 84a047f7..b56ce44f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -188,15 +188,14 @@ whose seams had diverged enough that several ports needed a different fix, and t truncated tag that reads as another project's. The prune scan then skipped the project's own parked control windows. The last requested field now keeps its delimiters. -- **A project path a window listing cannot carry no longer strands — or crashes — the scans over it.** - Listings are one row per window, split with `str.splitlines()` and decoded strictly, and two kinds - of byte defeat that while being perfectly legal in a POSIX path. A line separator (LF, CR, VT, FF, - FS, GS, RS, NEL, U+2028, U+2029) put the tag on a row of its own, so it never matched and the prune - scan skipped the project's own parked windows and sessions. A byte that is not valid in the - filesystem encoding arrived surrogate-escaped and made the listing read raise `UnicodeDecodeError` - outright. Both are now percent-encoded in the tag; every other path is tagged byte-identically, so - tags already stored on live windows and sessions keep comparing equal. Reading a tag stored raw by - an older version is the decode half, tracked in #380. +- **A project path the multiplexer cannot carry no longer strands the scans over it (#419).** The + ownership tag held the resolved path, and two transports mangled it: psmux's control line refuses + a spaced UNC share, and a listing row splits on any separator `splitlines()` knows (LF, CR, VT, + FF, FS, GS, RS, NEL, U+2028, U+2029) or fails a strict decode on a non-UTF-8 filename byte. Either + way the session or window went untagged — leaking once `clean` removed its run dir, and prunable + by another project on a reused `--run-id`. The tag is now a 16-hex digest of the path, safe on + both transports by construction; pruning still accepts the legacy path tag, so state surviving the + upgrade keeps its ownership. Reading a legacy raw tag is the decode half (#380). - **A run id that is a suffix of another no longer resolves to the neighbour's control window.** `--run-id` is caller-supplied and may contain `-`, so `run-other-RID` satisfied the lookup for diff --git a/docs/multiplexer-backends.md b/docs/multiplexer-backends.md index 447a10a2..986a95a9 100644 --- a/docs/multiplexer-backends.md +++ b/docs/multiplexer-backends.md @@ -76,17 +76,16 @@ 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__blw@N"` instead. Session-scoped options need no such substitute — one server per session means that server's single map _is_ the session's — -but they cross the same control line, so the session project tag is gated the same way. One +but they cross the same control line, so session-scoped `@` options are gated the same way. One visible limit: a value that cannot survive psmux's control-line transport verbatim is refused -with a stderr warning at every launch, and that project's windows and agent sessions 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. The -fallback that catches those refusals has a lifecycle ceiling -([#419](https://github.com/bmad-code-org/bmad-loop/issues/419)): an untagged session whose run -directory is later removed by `clean` or `archive` can no longer be pruned by any project. +with a stderr warning and the option reads as unset. Which values 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. The project ownership tag no +longer meets this gate: it is stored as a hex digest of the project path, transportable by +construction ([#419](https://github.com/bmad-code-org/bmad-loop/issues/419)), so sessions stay +tagged whatever the path and the run-dir fallback remains only for genuinely untagged state. ## External backends diff --git a/src/bmad_loop/adapters/psmux_backend.py b/src/bmad_loop/adapters/psmux_backend.py index 8a446390..9112ee20 100644 --- a/src/bmad_loop/adapters/psmux_backend.py +++ b/src/bmad_loop/adapters/psmux_backend.py @@ -506,15 +506,9 @@ def set_session_option(self, name: str, option: str, value: str) -> None: # tag is non-empty and never equals the caller's tag again, so the # prune skips that session forever. # - # Refusing leaves the option UNSET, which is the correct degradation - # and not the lesser evil: the prune's untagged path falls back to the - # run dir, claiming our own dead runs and skipping foreign ones. State - # the bound rather than the slogan — that fallback proves ownership by - # run-id collision on disk, not by identity, so it skips a foreign - # session only while no run dir HERE shares its run id. Ids are - # timestamped plus two random bytes, but `--run-id` is caller-supplied, - # so untagged is weaker proof than a tag even though it beats a - # corrupted one. Both edges of that fallback are bounded in #419. + # Refusing leaves the option unset. Project ownership now uses a hex + # digest that clears this gate by construction (#419), but the gate stays + # as the general contract for every `@` session option. # # The refusal frees the key rather than just returning. A session this # backend just created is NOT a blank map — the server loads the user's @@ -526,7 +520,7 @@ def set_session_option(self, name: str, option: str, value: str) -> None: print( f"warning: set-option {option} skipped on session {name} — value does " "not survive psmux's control-line transport verbatim; the key is freed " - "and ownership falls back to the run dir", + "and the option reads as unset", file=sys.stderr, ) self._write_scoped(["set-option", "-u", "-t", name, option], option) diff --git a/src/bmad_loop/adapters/tmux_base.py b/src/bmad_loop/adapters/tmux_base.py index 0fd315bd..444a442c 100644 --- a/src/bmad_loop/adapters/tmux_base.py +++ b/src/bmad_loop/adapters/tmux_base.py @@ -361,15 +361,18 @@ def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]] return [] rows: list[tuple[str, ...]] = [] for line in probe.stdout.splitlines(): - # Bounded split, so the LAST field may itself contain tabs. Fields - # carrying arbitrary text do exist — PROJECT_OPTION holds a resolved - # filesystem path, and a tab is a legal POSIX filename byte — and an - # unbounded split turns one such row into extra parts that the slice - # below then truncates, silently corrupting the field's value. - # Callers requesting a free-text field must therefore ask for it - # last; every current caller does. (A newline in that value still - # splits the row, which no parse here can undo — so runs.project_tag - # encodes a path holding one rather than leaning on this split.) + # Bounded split, so the LAST field may itself contain tabs. An + # unbounded split turns a row whose last field carries arbitrary + # text into extra parts that the slice below then truncates, + # silently corrupting the value. Callers requesting a free-text + # field must therefore ask for it last; every current caller does. + # This is the seam's standing contract, not a fix for one caller: + # no field requested today can hold a tab — PROJECT_OPTION is a + # digest and window names carry a shape-validated run id — so the + # bound is here for the next free-text field rather than these. + # (A newline in a value still splits the row, which no parse here + # can undo; runs.project_tag's digest is what keeps the tag clear + # of one.) parts = line.split("\t", len(fields) - 1) parts += [""] * (len(fields) - len(parts)) # tolerate unset trailing fields rows.append(tuple(parts[: len(fields)])) diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 71d8bc3c..e62997b7 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -2,17 +2,16 @@ from __future__ import annotations +import hashlib import json import math import os import re import secrets import shutil -import sys import tarfile import time from pathlib import Path -from urllib.parse import quote from . import devcontract, verify from .adapters.multiplexer import get_multiplexer @@ -296,99 +295,53 @@ def kill_session(run_id: str) -> None: # prunable_sessions and tui.launch. PROJECT_OPTION = "@bmad_project" -# Marks a tag whose project path could not ride a listing verbatim. A resolved -# absolute path never begins with it — POSIX starts at "/", win32 at a drive -# letter or "\\" — so an encoded tag can never be mistaken for a raw one, in -# either direction, and the two namespaces stay disjoint without a version byte. -_TAG_ENCODED_PREFIX = "%enc%" - def project_tag(project: Path) -> str: - """Canonical project identity stored in PROJECT_OPTION. The single source of - normalization: both the tagging (at session/window creation) and the prune - comparison must route through this so symlinks/relative paths can't make a - project look foreign to its own sessions. - - The result always reaches a comparison site as it was written, so the tag is - authoritative and no caller needs a can-I-trust-this fallback. Two halves - make that true: nothing `splitlines()` breaks on survives here (those are - encoded), so a row cannot split; and a tab is carried intact by the bounded - field split in `BaseTmuxBackend.list_windows`, so it needs no encoding. - - Encoding is conditional on purpose, and that is the whole compatibility - story. Tags are persisted on live windows and sessions, so encoding *every* - path would strand every tag written before this change (AGENTS.md's - compatibility rule). Returning a transportable path byte-identical strands - none: the only tags whose spelling changes are the ones the transport was - already mangling, which by definition never compared equal to anything.""" - raw = str(project.resolve()) - if _survives_listing(raw): - return raw - # safe="" so no separator can hide in a reserved character; the output is - # unreserved ASCII plus "%", which is inert to both the row and field splits. - # surrogateescape turns a non-UTF-8 filename byte back into that byte before - # percent-encoding it — without it this call raises UnicodeEncodeError on - # exactly the paths the encoding exists to carry. - return _TAG_ENCODED_PREFIX + quote(raw, safe="", errors="surrogateescape") - - -def _survives_listing(tag: str) -> bool: - """Whether `tag` survives a multiplexer listing round trip intact. - - The backends emit one window per line with tab-separated fields and split - the result with `str.splitlines()`, so anything *that* treats as a line - break splits the tag across two rows — which no parse on the receiving side - can undo, because the row boundary is the framing itself. A comparison - against the truncated remainder does not merely fail to match: it makes a - window look like it belongs to *another* project, so the caller discards the - project's own windows. - - Asked of `splitlines` itself rather than by listing the characters. The set - is far wider than LF and CR — VT, FF, FS, GS, RS, NEL, U+2028, U+2029 all - split — and every one of them is a legal byte in a POSIX directory name, so - an enumeration here would be a second copy of CPython's table that silently - rots when it grows. Routing the question through the same function the - parser uses cannot drift from it. - - The comparison is against the whole tag, not a row count, because a - *trailing* separator is equally fatal and does not add a row: `"/p\\r"` - splits to `["/p"]`, one element, yet the tag read back is `"/p"` and no - longer equals what was written. `text=True` on the subprocess also folds - CR and CRLF to LF before any of this, which is one more reason not to - reason about individual characters here. - - Tabs are deliberately NOT rejected here, and `splitlines` does not split on - them. They are equally legal in a path, but the backends' bounded split lets - a trailing field carry them intact (see BaseTmuxBackend.list_windows), so a - tab round-trips and a tagged comparison stays exact. Widening this to tabs - would rewrite the stored tag of every project holding one, for nothing — and - would mask the parser's guarantee rather than rest on it. The two mechanisms - stay disjoint on purpose: delete the bounded split and the tab cases fail; - delete the encoding and the separator cases fail. Neither covers the other. - - Two ways to fail, and both are asked in the transport's own terms rather - than by enumerating characters. A separator splits the *row*, which the - framing puts beyond any receiving-side parse. A surrogateescaped byte — what - `os.fsdecode` leaves behind for a filename byte that is not valid in the - filesystem encoding, and legal in every POSIX name — cannot be encoded at - all, so the listing carries the original byte and the backend's strict - decode raises `UnicodeDecodeError` on the way back. Left raw, that turns an - attach or a stop into a crash rather than a mismatch. - - This selects `project_tag`'s spelling; it is not a question any comparison - site asks. An earlier shape exposed it to callers so they could fall back to - the untagged path when their own tag looked unsafe, but "stop comparing - tags" admits rows carrying *another* project's tag — the fallback restored - reach by giving up the discriminator that keeps a stop from crossing project - boundaries. Encoding the few paths that need it keeps the discriminator for - every project instead.""" - if tag.splitlines()[:1] != [tag]: - return False - try: - tag.encode(sys.getfilesystemencoding()) - except UnicodeEncodeError: - return False # a surrogateescaped byte — no codec can carry it as text - return True + """Canonical project identity used by both tag writers and prune readers. The + single source of normalization: both sides must route through this so symlinks + and relative paths can't make a project look foreign to its own sessions. + + Hashing the resolved path makes every value safe by construction, on both + transports a tag has to cross. It clears psmux's control line (#419), whose + gate refuses any value the CLI->server hop would mangle — a UNC share whose + name holds a space is refused verbatim, and that refusal left the session + untagged, which is weak ownership twice over. It equally clears the listing + round trip (#518): a hex digest holds nothing `str.splitlines()` breaks on, + no tab, and no byte outside ASCII, so it can neither split a row nor fail the + backends' strict decode. + + That subsumes the conditional percent-encoding this function briefly applied. + Encoding answered only the listing half, so a path the listing could carry but + the control line could not — the spaced UNC above — still went untagged. The + compatibility objection encoding was shaped around, that rewriting every tag + strands the ones already stored on live sessions and windows, is answered on + the read side instead, by `accepted_tags`. + + 16 hex characters are ample for one machine's project population. + """ + return hashlib.sha256(os.fsencode(str(project.resolve()))).hexdigest()[:16] + + +def accepted_tags(project: Path) -> frozenset[str]: + """Current digest plus the legacy resolved-path tag accepted during pruning. + + The legacy member is read-only compatibility for sessions and ctl windows that + survive an upgrade; remove it once no path-tagged multiplexer state can remain. + Returns the whole set rather than answering per tag so a read site resolves the + project once per prune instead of once per session. + + The two shapes cannot collide into false ownership: a legacy tag is an absolute + path, so it always holds a separator, while a digest is bare 16-hex. + + Deliberately two members and not three — a tag spelled with the `%enc%` prefix, + from the window when this module encoded rather than hashed, is not accepted. + Only a path the listing could not carry was ever spelled that way (one holding + a line separator, or a byte invalid in the filesystem encoding), and that + spelling never reached a release. An unaccepted tag reads as foreign, which + skips the session rather than pruning it, so the edge is fail-safe and clears + itself on the next tag write. + """ + return frozenset({project_tag(project), str(project.resolve())}) def mux_sessions() -> list[str]: @@ -412,16 +365,20 @@ def prunable_sessions(project: Path) -> tuple[list[str], list[str], set[str]]: The control session (bmad-loop-ctl) is never a candidate. Pruning is scoped to `project` via the PROJECT_OPTION tag set at session creation: - - tag == this project: ours — prunable unless a provably-alive engine pid is - running (covers finished/stopped/crashed *and* orphans whose run dir was - deleted, since engine_liveness reads 'dead' with no pid). + - tag proves this project (see accepted_tags): ours — prunable unless a + provably-alive engine pid is running (covers finished/stopped/crashed *and* + orphans whose run dir was deleted, since engine_liveness reads 'dead' with + no pid). - tag is another project: skipped — never touched. - - tag empty (pre-upgrade, untagged session): can't prove ownership, so fall - back to the run dir — prunable only when the dir exists under this project - and is dead; skipped when the dir is absent. + - tag empty (untagged session): can't prove ownership, so fall back to the run + dir — prunable only when the dir exists under this project and is dead; + skipped when the dir is absent. Reachable when the tag write failed, when + the option read degrades (session_options reads unset as "no answer", never + as proof nothing was written), or on a session predating a working tag + write — e.g. psmux path tags refused before the digest. """ tags = session_project_tags() - mine = project_tag(project) + mine = accepted_tags(project) prunable: list[str] = [] live: list[str] = [] unknown: set[str] = set() @@ -434,7 +391,7 @@ def prunable_sessions(project: Path) -> tuple[list[str], list[str], set[str]]: run_dir = run_dir_for(project, run_id) tag = tags.get(name, "") if tag: - if tag != mine: + if tag not in mine: continue # another project's session elif not is_run(run_dir): continue # untagged and no run dir here — ownership unprovable diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index d8710626..55ab0ff9 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -379,7 +379,7 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: for a fresh `run`, where recording is deliberately skipped.""" if not mux_available(): return None - mine = runs.project_tag(project) + mine = runs.accepted_tags(project) local = runs.is_run(runs.run_dir_for(project, run_id)) tagged: list[str] = [] untagged: list[str] = [] @@ -403,12 +403,17 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: m = _CTL_WINDOW_RE.match(name) if m is None or m.group(1) != run_id: continue - # An exact tag comparison, with no "the tag looks unsafe here" escape: - # runs.project_tag guarantees the value arrives as it was written, so a - # nonempty tag that is not ours belongs to another project and must not - # be a candidate — `x` resolves through here, and admitting a foreign - # row lets a stop cross a project boundary. - if tag == mine: + # Set membership, with no "the tag looks unsafe here" escape: the digest + # arrives as it was written, so a nonempty tag outside the accepted set + # belongs to another project and must not be a candidate — `x` resolves + # through here, and admitting a foreign row lets a stop cross a project + # boundary. The set is what keeps a window tagged by an earlier release + # reachable: the control session is long-lived and survives the upgrade + # that changes the tag's spelling, so comparing against the current + # digest alone would strand this project's own orchestrator — prunable + # by _ctl_window_candidates, which accepts the legacy tag, yet + # unreachable by `a` and `x`, which resolve through here. + if tag in mine: tagged.append(win_id) elif not tag and local: # untagged, and this project holds the run dir — ownership is @@ -645,17 +650,16 @@ def _ctl_window_candidates(project: Path) -> list[tuple[str, str]]: the ctl session never targets itself; live runs and the session's own shell window are excluded too. - The control session is shared across projects, so pruning is scoped to - `project` via the per-window PROJECT_OPTION tag (mirrors runs.prunable_sessions): - a window tagged for another project is left alone; an untagged (pre-upgrade) - window is only a candidate when its run dir exists under this project. + The control session is shared across projects, so its per-window PROJECT_OPTION + accepts current and legacy project tags; untagged windows still require a run + directory under this project (mirrors runs.prunable_sessions). """ mux = get_multiplexer() if not mux_usable(mux) or not session_exists(CTL_SESSION): return [] current = mux.current_window_id() rows = mux.list_windows(CTL_SESSION, ["window_id", "window_name", runs.PROJECT_OPTION]) - mine = runs.project_tag(project) + mine = runs.accepted_tags(project) candidates: list[tuple[str, str]] = [] for win_id, name, tag in rows: if not win_id or win_id == current: @@ -667,7 +671,7 @@ def _ctl_window_candidates(project: Path) -> list[tuple[str, str]]: continue # a foreign/mangled window name must not steer a run-dir path run_dir = runs.run_dir_for(project, m.group(1)) if tag: - if tag != mine: + if tag not in mine: continue # another project's window elif not runs.is_run(run_dir): continue # untagged and no run dir here — ownership unprovable diff --git a/tests/test_psmux_backend.py b/tests/test_psmux_backend.py index ae34f2fb..02689ddb 100644 --- a/tests/test_psmux_backend.py +++ b/tests/test_psmux_backend.py @@ -834,7 +834,9 @@ def test_set_window_option_resolves_a_name_token(monkeypatch): 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. + # The transport gate is the general contract for every `@` option value: a + # spaced value clears it via client quoting, and the channel must still pass + # it as one argv element. 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" diff --git a/tests/test_runs.py b/tests/test_runs.py index 49d276f0..f795ba68 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -6,13 +6,13 @@ import subprocess import sys import tarfile -from pathlib import Path import pytest from conftest import escalated_run, git from bmad_loop import platform_util, runs, verify from bmad_loop.adapters import tmux_base +from bmad_loop.adapters.psmux_backend import PsmuxMultiplexer from bmad_loop.journal import load_state, save_state from bmad_loop.model import RunState from bmad_loop.process_host import ProcessHost @@ -645,99 +645,28 @@ def test_mux_sessions_no_server(monkeypatch): _SEP_IDS = [i for i, _ in _LINE_SEPARATORS] +@pytest.mark.skipif(sys.platform == "win32", reason="a separator in a name is a POSIX concern") @pytest.mark.parametrize("separator", _SEP_VALUES, ids=_SEP_IDS) -@pytest.mark.parametrize("place", ["middle", "trailing"], ids=["mid", "tail"]) -def test_survives_listing_rejects_every_line_separator(separator, place): - """Everything `str.splitlines()` breaks on, not just LF. - - The listing is parsed with `splitlines()`, whose set is much wider than the - two obvious characters — and all of them are legal bytes in a POSIX - directory name. This predicate chooses `project_tag`'s spelling, so naming - only LF here would leave the rest riding the transport raw and arriving - truncated at every comparison site. - - `trailing` is a separate case on purpose: a separator at the end does not - add a row (`"/p\\r".splitlines()` is one element), so a row-count check - passes it while the tag still comes back changed.""" - tag = f"/home/u/p{separator}x" if place == "middle" else f"/home/u/p{separator}" - assert runs._survives_listing(tag) is False - - -def test_survives_listing_accepts_paths_that_survive_the_round_trip(): - # The control: ordinary paths, and a tab — which splitlines does not break - # on and the backends' bounded split carries intact, so encoding it would - # rewrite the stored tag of every project holding one, for nothing. - assert runs._survives_listing("/home/u/proj") is True - assert runs._survives_listing("/home/u/my proj") is True - assert runs._survives_listing("/home/u/my\tproj") is True - - -def test_project_tag_leaves_a_transportable_path_byte_identical(tmp_path): - """The compatibility half of the conditional encoding. - - Tags persist on live windows and sessions, so one written by an earlier - version has to keep comparing equal after an upgrade. Encoding only the - paths the transport cannot carry is what makes that true: drop the - `_survives_listing` branch from project_tag and every ordinary project's - stored tag stops matching itself. A tab, a space, a percent and non-ASCII - are all carried as-is.""" - for name in ("proj", "my proj", "my\tproj", "100%done", "prögram"): - project = tmp_path / name - assert runs.project_tag(project) == str(project.resolve()) - - -@pytest.mark.skipif(sys.platform == "win32", reason="separators are illegal in win32 names") -@pytest.mark.parametrize("separator", _SEP_VALUES, ids=_SEP_IDS) -def test_project_tag_encodes_a_path_the_listing_cannot_carry(tmp_path, separator): - """The correctness half: a comparison site receives the tag that was - written, for *every* project, so no caller needs a trust fallback. - - Two projects must also stay distinguishable. The fallback this replaced - stopped comparing tags when its own looked unsafe, which admitted rows - carrying another project's tag — so an encoding that collapsed two projects - together would reintroduce exactly the boundary crossing it removed.""" - mine = tmp_path / f"my{separator}proj" - tag = runs.project_tag(mine) - assert runs._survives_listing(tag) is True - assert tag.startswith(runs._TAG_ENCODED_PREFIX) +def test_project_tag_carries_a_path_the_listing_cannot_carry(tmp_path, separator): + """A listing splits on far more than LF, and every one of those is legal in a + POSIX directory name (#518). + + The digest makes this true by construction instead of by encoding the few paths + that needed it, but the property is the same one and still needs pinning: return + a raw path from project_tag again and these ride the transport raw, arriving + truncated at every comparison site — a truncated tag is non-empty, so it reads as + *another* project's and the scan discards the project's own windows. + + Trailing is not a separate case here as it was for the old predicate: a digest + has no separator anywhere, so there is no row-count blind spot left to probe. + Two projects must also stay distinguishable — a tag collapsing them would let a + prune cross the boundary the tag exists to hold.""" + tag = runs.project_tag(tmp_path / f"my{separator}proj") + assert re.fullmatch("[0-9a-f]{16}", tag) + assert tag.splitlines()[:1] == [tag] # one row, and the whole of it assert tag != runs.project_tag(tmp_path / "theirproj") -def test_survives_listing_rejects_a_surrogateescaped_filename_byte(): - """A byte that is not valid in the filesystem encoding is legal in a POSIX - name, and `os.fsdecode` leaves it as a lone surrogate. - - No codec can encode that surrogate, so the listing carries the original byte - and the backend's strict decode raises `UnicodeDecodeError` reading it back - — an attach or a stop *crashes* rather than mismatching. Checking only line - separators here reported such a tag as safe and left it raw.""" - assert runs._survives_listing("/home/u/proj\udcff") is False - assert runs._survives_listing("/home/u/pr\N{LATIN SMALL LETTER O WITH DIAERESIS}gram") is True - - -@pytest.mark.skipif(sys.platform == "win32", reason="win32 names are valid UTF-16 by construction") -def test_project_tag_encodes_a_non_utf8_filename_byte(tmp_path): - """The encoded tag must be pure ASCII, and the encoder must survive the very - bytes it exists to carry. - - `quote(safe="")` defaults to strict UTF-8 and raises `UnicodeEncodeError` on - a surrogate, so without `errors="surrogateescape"` project_tag itself blew up - on exactly these paths — worse than the mismatch it was added to prevent.""" - weird = Path(os.fsdecode(os.path.join(os.fsencode(tmp_path), b"proj\xff"))) - weird.mkdir() - tag = runs.project_tag(weird) - assert tag.startswith(runs._TAG_ENCODED_PREFIX) - assert tag.isascii() # nothing left for a strict decode to choke on - assert runs._survives_listing(tag) is True - - # A separator AND a bad byte together: the combined case reached the encoder. - both = Path(os.fsdecode(os.path.join(os.fsencode(tmp_path), b"p\nq\xff"))) - both.mkdir() - combined = runs.project_tag(both) - assert combined.isascii() - assert combined != tag # still injective across the two failure modes - - def test_prunable_sessions_partitions(tmp_path, monkeypatch): mine = runs.project_tag(tmp_path) # live run: real run dir with this process's pid, tagged ours @@ -780,6 +709,44 @@ def test_prunable_sessions_partitions(tmp_path, monkeypatch): assert unknown == set() +def test_project_tag_is_transportable_whatever_the_path(tmp_path): + """Tags have one safe shape even for paths psmux or UTF-8 cannot carry raw. + + Assert the shape, not just that the gate accepts it: an ordinary spaced Windows + path clears the gate on its own, so only "hex whatever the input" fails when + project_tag returns a raw path. + """ + assert not PsmuxMultiplexer._transportable(r"\\srv\share name\proj") # the premise + project = tmp_path / "share name" / "proj" + project.mkdir(parents=True) + tag = runs.project_tag(project) + assert re.fullmatch("[0-9a-f]{16}", tag) + assert PsmuxMultiplexer._transportable(tag) + assert re.fullmatch("[0-9a-f]{16}", runs.project_tag(tmp_path / f"bad{chr(0xDC80)}")) + assert len({tag, runs.project_tag(tmp_path / "other")}) == 2 + + +def test_prunable_sessions_accepts_legacy_path_tag(tmp_path, monkeypatch): + """A pre-digest tag stays ours; another project's path or digest stays foreign.""" + legacy = str(tmp_path.resolve()) + fin = _make_state_run(tmp_path, "legacy-fin") + (fin / "engine.pid").write_text(str(_dead_pid())) + sessions = ["bmad-loop-legacy-fin", "bmad-loop-legacy-other", "bmad-loop-legacy-digest"] + monkeypatch.setattr(runs, "mux_sessions", lambda: sessions) + monkeypatch.setattr( + runs, + "session_project_tags", + lambda: { + "bmad-loop-legacy-fin": legacy, + "bmad-loop-legacy-other": "/some/other/project", + "bmad-loop-legacy-digest": runs.project_tag(tmp_path / "other"), + }, + ) + prunable, live, unknown = runs.prunable_sessions(tmp_path) + assert prunable == ["legacy-fin"] + assert live == [] and unknown == set() + + def test_prunable_sessions_skips_invalid_run_ids(tmp_path, monkeypatch): """A session name is untrusted input (anyone can create one). Stripping the prefix off `bmad-loop-../../x` would hand `run_dir_for` a traversing id, and a diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index b6dc5898..9a88ddf6 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -381,6 +381,23 @@ def test_ctl_window_id_ignores_a_record_naming_another_projects_window(monkeypat assert launch.ctl_window_id(tmp_path, "RID") == "@2" +def test_ctl_window_id_accepts_a_legacy_path_tag(monkeypatch, tmp_path: Path): + # The ctl session is long-lived and shared across projects, so it survives the + # upgrade that changes the tag's spelling from a path to a digest. Comparing + # against the current digest alone strands this project's OWN orchestrator: + # _ctl_window_candidates accepts the legacy tag and would prune the window, + # while `a` and `x` resolve through here and could no longer reach it. + legacy = str(tmp_path.resolve()) + _ctl_listing(monkeypatch, f"@1\trun-RID\t{legacy}\n", tmp_path) + assert launch.ctl_window_id(tmp_path, "RID") == "@1" + + # Still scoped: another project's legacy path tag stays foreign, so accepting + # the legacy spelling does not widen the boundary a stop must not cross. + other = str((tmp_path / "elsewhere").resolve()) + _ctl_listing(monkeypatch, f"@1\trun-RID\t{other}\n", tmp_path) + assert launch.ctl_window_id(tmp_path, "RID") is None + + def test_ctl_window_id_admits_an_untagged_window_with_a_local_run(monkeypatch, tmp_path: Path): # The tag is written by a best-effort set_window_option that can fail, and a # window whose tag never landed must stay reachable by its own project @@ -892,9 +909,11 @@ def test_a_delimiter_in_the_project_path_does_not_hide_its_own_window( `a`/`x` then could not reach a run the pre-tag lookup found. Worse than a missed match, because the fallthrough for an unknown tag is exclusion. - Parametrized over all six on purpose: two different mechanisms carry them — - the tab by the backends' bounded field split, the separators by project_tag's - encoding — so one spelling passing says nothing about another.""" + Parametrized over all six on purpose: each is a byte a resolved project path + can legally hold, and project_tag hashes the path rather than carrying any + spelling of it, so none of them reaches the listing. The matrix pins that the + digest is the single mechanism — return a raw path here and the tab and the + separators fail again, by two different routes.""" project = tmp_path / odd_name project.mkdir() _make_run(project) @@ -914,8 +933,9 @@ def test_a_separator_in_the_project_path_does_not_admit_a_foreign_window( when its own could not survive the listing. That admits every row carrying the run id — including one tagged for another project — and `x` resolves through here, so a stop could kill a neighbouring project's orchestrator. - Reach and scoping are not a trade: project_tag encodes the tag instead, so - the comparison stays exact and this row is simply not ours. + Reach and scoping are not a trade: project_tag hashes the resolved path, so + the tag is listing-safe by construction, the comparison stays exact, and this + row is simply not ours. The two assertions differ only in whose tag the row carries, which is what makes the refusal about the tag rather than about the listing being @@ -1280,6 +1300,32 @@ def fake(argv, **kwargs): assert killed == [["tmux", "kill-window", "-t", "@3"]] +def test_prune_ctl_windows_accepts_legacy_path_tag(monkeypatch, tmp_path: Path): + """A ctl window carrying a pre-digest tag remains owned after upgrade.""" + from bmad_loop import runs + + legacy = str(tmp_path.resolve()) + windows = ( + f"@2\trun-20260101-000000-dead\t{legacy}\n" # our own pre-upgrade window — kill + "@3\trun-20260101-000000-alien\t/some/other/project\n" # foreign — skip + ) + + def fake(argv, **kwargs): + verb = argv[1] + if verb == "list-windows": + return subprocess.CompletedProcess(argv, 0, stdout=windows, stderr="") + if verb == "display-message": + return subprocess.CompletedProcess(argv, 0, stdout="@9\n", stderr="") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setenv("TMUX", "/tmp/tmux-1000/default,123,0") + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + + assert launch.prunable_ctl_windows(tmp_path) == ["run-20260101-000000-dead"] + assert runs.project_tag(tmp_path) != legacy # the shapes really are different + + def test_prune_ctl_windows_skips_invalid_run_ids(monkeypatch, tmp_path: Path): """A ctl-window name is untrusted input (anyone can rename a tmux window). Stripping the kind prefix off `run-../../x` would hand run_dir_for a