diff --git a/CHANGELOG.md b/CHANGELOG.md index ff817c6f..bea6dd70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,6 +160,36 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Fixed +- **A tab in the project path no longer truncates the project tag a window listing carries.** + The multiplexer listing is tab-delimited and the tag holds a resolved filesystem path, where a tab + is a legal byte — so the parse split one row into extra fields and dropped the tail, leaving a + 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 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 + `RID` — and sorted ahead of it, so `x` could kill the neighbouring run's live orchestrator. + Window names are parsed and the run id compared whole, as the prune scan already did. + +- **Attach, return-stamp and kill follow the run's live control window, not an older one (#482).** + `-` window names are not unique, so the lookup answered the first match — `a`, the + return stamp and `x` all landed on a parked run's dead window while the live one ran on. Each + launch records the window id it minted and the lookup prefers it while the listing still shows it + under this run id; with no record the answer is unchanged, and a resume whose id was not captured + warns rather than reporting plain success. **Adapter authors:** the re-prove pairs + `new_parked_window`'s id with the `window_id` column of `list_windows`, which the seam previously + left free to diverge — a backend where they differ degrades to the by-name resolve. + - **A native-Windows install driven from a WSL shell now says so (#332).** WSL appends the Windows `PATH` to its own, so a bash prompt can reach a Windows-installed `bmad-loop`: that interpreter reports `win32`, takes the psmux platform default, and never sees the distro's tmux — while diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 2ee41089..829c0979 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -101,7 +101,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Every run is a resumable on-disk state machine: `bmad-loop resume ` continues from a gate, escalation, or interruption. - A graceful stop (`stop --graceful` / TUI `S`) is resumable too: unlike a hard stop killed mid-item, it lets the in-flight item finish through commit and finalizes cleanly, ending as a `stopped` run that `resume` picks up at the next item. -- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `events/` (hook signals); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`. +- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `events/` (hook signals); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`; `ctl-window` (the control-session window id the last TUI launch minted, so attach/stop follow the live window, #482). - `journal.jsonl` records `session-end` for every session unconditionally — even a teardown that throws still lands one (status `aborted` when the outcome is unknowable). A timed-out session's entry carries `fired_at` (wall time the deadline was declared), `teardown_s` (wall seconds from that fire to this entry — the teardown gap), and `expired_clock` (`monotonic` / `wall` / `both` — `wall` alone fingerprints a host suspend that froze the monotonic clock). Every entry whose usage was read carries `tokens` (raw) and `tokens_weighted` (cache reads at `limits.cache_read_weight`), keeping per-session spend reconstructible; both are `null` when the usage read failed, and both are absent on an `aborted` end. `tokens_weighted` is the end-of-session total — distinct from a tripped session's `budget_weighted`, the guard's mid-session sample at trip time. ### Hook-based transport (no pane-scraping) diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 44e9e06c..12e9ee2f 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -136,7 +136,11 @@ the backend owns those conditions, and applies them uniformly, so the Both are replayed opaquely; neither is parsed by core. psmux applies the same qualification to `new_parked_window`, the `window_id` columns of `list_windows` and `current_window_id`; the latter two must agree, since the ctl-window prune -compares them to skip its own window.) tmux consumes the token natively (it coincides with tmux exact-match +compares them to skip its own window. To preserve unambiguous lookup, +`new_parked_window` must agree with the `list_windows` column too; a backend that +qualifies one side only remains usable but falls back to resolving parked +windows by name, which is ambiguous whenever several kinds share a run id +(#482). tmux consumes the token natively (it coincides with tmux exact-match syntax), so `BaseTmuxBackend` passes it straight through. A native-id backend calls `parse_target()` first — `None` means "already a native id, use as-is", otherwise resolve `(session, window)` yourself; the herdr adapter's diff --git a/docs/tui-guide.md b/docs/tui-guide.md index 953a4716..d213ba2e 100644 --- a/docs/tui-guide.md +++ b/docs/tui-guide.md @@ -43,7 +43,10 @@ The TUI never runs an engine in-process. The two halves: lives in a separate `bmad-loop-` session; it is torn down when the run finishes (unless `[adapter] cleanup_session_on_finish = false`). These parked `bmad-loop-ctl` windows and any leftover `bmad-loop-` sessions can be - swept with `c` (see [Cleaning up sessions](#cleaning-up-sessions-c)). + swept with `c` (see [Cleaning up sessions](#cleaning-up-sessions-c)). Each + launch over an existing run records the id of the window it minted in the + run dir (`ctl-window`), so attach/stop follow the run's live window even + while an older same-run-id window is still parked (#482). - **Observer** — the dashboard reads only the artifacts the engine writes atomically into `.bmad-loop/runs//`: `state.json`, `journal.jsonl`, `logs/.log`, `ATTENTION`, `engine.pid`. It polls the selected run diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index fe8c0286..40016be2 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -158,7 +158,8 @@ def new_parked_window( """Create a window that runs ``argv`` then *parks* — waiting on a key so the exit status stays inspectable instead of the window closing the moment the process exits — and finally returns an attached client to its origin - (keyed by the per-window ``return_opt``). Returns the native window id.""" + (keyed by the per-window ``return_opt``). Returns the native window id; + for its required form see :meth:`list_window_ids`'s note on #482.""" @abstractmethod def list_window_ids(self, session: str) -> list[str]: @@ -172,10 +173,11 @@ def list_window_ids(self, session: str) -> list[str]: server per session), so a bare ``@N`` replayed as a ``-t`` target routes by the *caller's* server instead of the owning one. - :meth:`new_parked_window` is *outside* the rule — nothing - membership-tests a parked id, it is only replayed as a ``-t`` target by - the TUI — so a backend MAY mint it in a form this list never carries - (psmux happens to qualify it too, #291). + :meth:`new_parked_window` is outside *this* list's rule. To preserve + #482's unambiguous lookup, however, its id must match the ``window_id`` + column of :meth:`list_windows` (psmux qualifies both, #291). A backend + that diverges remains usable, but falls back to the ambiguous by-name + lookup whenever several kinds share a run id. Raises :class:`MultiplexerError` if the transport itself fails (timeout / missing binary): an empty list means "no windows" and must not be diff --git a/src/bmad_loop/adapters/tmux_base.py b/src/bmad_loop/adapters/tmux_base.py index 399e2516..0fd315bd 100644 --- a/src/bmad_loop/adapters/tmux_base.py +++ b/src/bmad_loop/adapters/tmux_base.py @@ -361,7 +361,16 @@ def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]] return [] rows: list[tuple[str, ...]] = [] for line in probe.stdout.splitlines(): - parts = line.split("\t") + # 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.) + parts = line.split("\t", len(fields) - 1) parts += [""] * (len(fields) - len(parts)) # tolerate unset trailing fields rows.append(tuple(parts[: len(fields)])) return rows diff --git a/src/bmad_loop/platform_util.py b/src/bmad_loop/platform_util.py index 09fcdefd..b8e5ae36 100644 --- a/src/bmad_loop/platform_util.py +++ b/src/bmad_loop/platform_util.py @@ -242,7 +242,7 @@ def _copy_xattrs(src: Path, dst: Path) -> None: continue -def atomic_write_text(path: Path, text: str) -> None: +def atomic_write_text(path: Path, text: str, *, follow_symlinks: bool = True) -> None: """Replace ``path``'s contents with ``text`` atomically, preserving what the replacement would otherwise silently discard. @@ -253,7 +253,11 @@ def atomic_write_text(path: Path, text: str) -> None: * **Symlinks are followed.** ``path.resolve()`` first, so a ledger symlinked into the repo keeps being a symlink and the real file is what gets rewritten — a replace against the link itself would turn it into a regular file and - orphan the target. + orphan the target. Pass ``follow_symlinks=False`` to invert that: the name + is replaced, whatever it points at. Right for a machine-minted file living + somewhere a less-trusted writer can reach, where honouring a planted link + would aim this write at a path of that writer's choosing; wrong for the + operator-curated ledgers this helper was built for, hence the default. * **Permission bits survive.** A ``0600`` file stays ``0600`` instead of becoming ``0644 & ~umask``, which on a shared artifact dir is the difference between "the group can still write this" and a silent lockout (or a @@ -285,7 +289,7 @@ def atomic_write_text(path: Path, text: str) -> None: above and differ only in the ``os.fdopen`` mode. Text mode's *newline* default (translating) is deliberate here — it matches the ``Path.write_text`` this replaced, so a ledger's line endings do not change under Windows.""" - _atomic_write(path, text, mode="w", encoding="utf-8") + _atomic_write(path, text, mode="w", encoding="utf-8", follow_symlinks=follow_symlinks) def atomic_write_bytes(path: Path, data: bytes) -> None: @@ -304,14 +308,41 @@ def atomic_write_bytes(path: Path, data: bytes) -> None: _atomic_write(path, data, mode="wb", encoding=None) -def _atomic_write(path: Path, payload: str | bytes, *, mode: str, encoding: str | None) -> None: +def _atomic_write( + path: Path, + payload: str | bytes, + *, + mode: str, + encoding: str | None, + follow_symlinks: bool = True, +) -> None: """The shared body of the two public helpers above — see :func:`atomic_write_text` for the contract every step here implements. Written through ``os.fdopen`` rather than a raw ``os.write`` loop on purpose: it routes to ``io.open``, the one seam a test can inject a short write at for - both variants at once (tests/test_install.py's #375 case).""" - target = path.resolve() + both variants at once (tests/test_install.py's #375 case). + + ``follow_symlinks=False`` skips the resolve, so the *name* is what gets + replaced. It needs no preflight ``is_symlink`` check to be safe, and that is + the reason to prefer it over one: ``os.replace`` does not dereference its + destination, so a link planted at any moment — including between a check and + this call — is overwritten rather than written through. + + Mode and xattrs are then not inherited **at all**, and nothing is probed to + decide that. A name being replaced rather than updated should carry nothing + of whatever it used to point at, and in this mode there is no trustworthy + prior to carry over anyway: the caller asked for no-follow precisely because + a less-trusted writer can reach the name, so the mode found there is that + writer's choice as much as anyone's. Probing first and copying after would + also reopen by the back door the very window the paragraph above closes — + ``shutil.copymode`` re-resolves the path it is handed, so a link planted + between the probe and the copy hands the new record the mode of a file of + the planter's choosing (the contents stay safe; ``os.replace`` still does not + dereference). Taking no probe leaves no window to race, and ``mkstemp``'s + private ``0600`` is the right mode for the machine-minted file this mode + exists for.""" + target = path.resolve() if follow_symlinks else path fd, tmp_name = tempfile.mkstemp(dir=str(target.parent), prefix=target.name + ".", suffix=".tmp") tmp = Path(tmp_name) try: @@ -319,7 +350,7 @@ def _atomic_write(path: Path, payload: str | bytes, *, mode: str, encoding: str fh.write(payload) fh.flush() # userspace buffer -> kernel, so there is something to sync os.fsync(fh.fileno()) - if target.exists(): + if follow_symlinks and target.exists(): shutil.copymode(target, tmp) _copy_xattrs(target, tmp) atomic_replace(tmp, target) @@ -329,6 +360,109 @@ def _atomic_write(path: Path, payload: str | bytes, *, mode: str, encoding: str raise +# Whether this platform has the `*at()` family the two helpers below need. The +# probe is `O_DIRECTORY` rather than `os.supports_dir_fd`: that set tracks only +# the literal `dir_fd` parameter, so `os.replace` — which spells it +# `src_dir_fd`/`dst_dir_fd` — is absent from it even on Linux, where renameat +# works. CPython gates the whole family on one configure pass, so the flag's +# presence answers for all of them: it is defined on Linux/macOS and absent on +# Windows, whose pyconfig has neither HAVE_RENAMEAT nor HAVE_OPENAT. +DIR_FD_ANCHORED_WRITES = hasattr(os, "O_DIRECTORY") + + +def open_dir_confined(root: Path, target: Path) -> int | None: + """An open descriptor for ``target``, reached from ``root`` without + traversing a symlink at any component below it — or None when that cannot be + established. The caller owns the descriptor and must ``os.close`` it. + + A *descriptor*, not a verdict, and that is the whole point. A boolean + "is this path confined?" is answered about a path, and the answer is stale + the instant it returns: whoever can write those directories can swap one for + a symlink before the caller gets around to opening anything. The descriptor + this hands back is bound to the directory that was actually walked, so a + later swap of any name along the way renames a path this no longer consults. + Pair it with :func:`atomic_write_text_at`, which never names a path again. + + Each component is opened ``O_NOFOLLOW | O_DIRECTORY`` relative to the one + above it, so a link anywhere below ``root`` fails the open rather than being + followed. ``root`` itself is opened without ``O_NOFOLLOW``: the operator + chooses where the project lives and may keep it behind a link, while + everything under it is session-writable. + + POSIX only — see :data:`DIR_FD_ANCHORED_WRITES`. Callers need a fallback for + win32, which has no ``*at()`` family to anchor against.""" + if not DIR_FD_ANCHORED_WRITES: + return None + try: + relative = target.relative_to(root) + except ValueError: + return None # not under root at all + try: + fd = os.open(root, os.O_RDONLY | os.O_DIRECTORY) + except OSError: + return None + for part in relative.parts: + try: + nested = os.open(part, os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW, dir_fd=fd) + except OSError: + os.close(fd) + return None # a link, a missing component, or one we cannot probe + os.close(fd) + fd = nested + return fd + + +# Draws before giving up on a unique temp name. O_EXCL makes a collision +# harmless, so this only bounds a pathological loop. +_TMP_NAME_ATTEMPTS = 100 + + +def atomic_write_text_at(dir_fd: int, name: str, text: str) -> None: + """:func:`atomic_write_text`, anchored at an open directory descriptor. + + Every syscall here is relative to ``dir_fd``, so nothing resolves a path a + concurrent writer could have redirected — the directory is the one + :func:`open_dir_confined` walked to, whatever its name points at now. That + closes the window a preflight path check leaves open, rather than narrowing + it. ``name`` must be a single component. + + Shares the shape of the path-based helper: unique temp in the same + directory, contents fsynced *before* the replace publishes them, temp + removed on any failure. It deliberately does NOT inherit mode or xattrs — + this exists for machine-minted files under a session-writable root, where + the prior file's mode is as untrusted as the rest of it, so the new record + keeps the private ``0600`` it is created with. Text is written UTF-8 with + no newline translation; the callers are records, not operator-edited files. + + No win32 sharing-violation retry, unlike :func:`atomic_replace`: there is no + win32 here at all — the ``*at()`` family this is built on does not exist + there, so a caller reaching this is on POSIX by construction.""" + for _ in range(_TMP_NAME_ATTEMPTS): + # os.urandom, not `random`: this name is created in a directory a + # less-trusted writer can reach, and a predictable one lets them + # pre-create it and fail every record write (O_EXCL turns the collision + # into a refusal rather than a clobber, so the harm is a stuck hint + # rather than a redirect — but an unguessable name removes even that). + tmp = f"{name}.{os.getpid():x}.{os.urandom(4).hex()}.tmp" + try: + fd = os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600, dir_fd=dir_fd) + except FileExistsError: + continue # astronomically unlikely; costs one more draw + break + else: + raise OSError(f"no free temp name beside {name!r} after {_TMP_NAME_ATTEMPTS} tries") + try: + with os.fdopen(fd, "w", encoding="utf-8", newline="") as fh: + fh.write(text) + fh.flush() # userspace buffer -> kernel, so there is something to sync + os.fsync(fh.fileno()) + os.replace(tmp, name, src_dir_fd=dir_fd, dst_dir_fd=dir_fd) + except BaseException: + with suppress(OSError): + os.unlink(tmp, dir_fd=dir_fd) + raise + + def retrying_unlink(path: Path) -> None: """``path.unlink()`` with the same win32 retry as :func:`atomic_replace`. diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 59532744..71d8bc3c 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -8,9 +8,11 @@ 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 @@ -294,13 +296,99 @@ 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.""" - return str(project.resolve()) + 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 def mux_sessions() -> list[str]: diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index a03cc5d6..cea10f2e 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -415,7 +415,7 @@ def action_attach(self) -> None: self.notify("no run selected", severity="warning") return session = runs.session_name(run_id) - win_id = launch.ctl_window_id(run_id) + win_id = launch.ctl_window_id(self.project, run_id) ok, agent_live = self._mux_guarded(lambda: launch.session_exists(session)) if not ok: return @@ -522,6 +522,16 @@ def _launch_resolve(self, run_id: str) -> None: if not win_id: self.notify("resolve launched but its window id was not captured", severity="error") return + if not launch.ctl_window_recorded(self.project, run_id, win_id): + # Not an error and not a reason to abort: this attach targets the id + # in hand, so the resolve session itself is reached correctly. What + # is lost is the record *later* verbs read, so `a`/`x` after this + # window is minted may answer an older one (#482's symptom). + self.notify( + "resolve launched but its window id was not recorded — " + "later attach/stop may target an older window for this run", + severity="warning", + ) launch.select_ctl_window_id(win_id) self._attach_to_target(launch.ctl_target(), return_window=win_id) @@ -709,10 +719,22 @@ def _do_resume(self, run_id: str) -> None: self.notify(f"run {run_id} may still be live — stop it first", severity="warning") return try: - launch.resume_detached(self.project, run_id) + win_id = launch.resume_detached(self.project, run_id) except launch.LaunchError as e: self.notify(str(e), severity="error") return + if not win_id: + # The resume itself is running; only the disambiguation record is + # lost, so `a`/`x` may target an older same-run_id window (#482's + # symptom). Warn instead of masking it behind the success toast. + # "not recorded", not "not captured": resume_detached reports the + # uncaptured id and the unwritten record through this one signal + # because they leave the operator in the same place. + self.notify( + "resume launched but its window id was not recorded — " + "attach/stop may target an older window for this run", + severity="warning", + ) self.notify(f"resume of {run_id} launched (control session {launch.CTL_SESSION})") def _do_replan(self, run_id: str, spec_path: Path) -> None: @@ -874,7 +896,7 @@ def done(ok: bool | None) -> None: def _stop_run_worker(self, run_id: str, run_dir: Path) -> None: try: runs.stop_run(run_dir) - launch.kill_ctl_window(run_id) + launch.kill_ctl_window(self.project, run_id) except (OSError, StopRunError, ProcessHostError) as e: self.call_from_thread(self.notify, f"stop failed: {e}", severity="error") return diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index 23312094..d8710626 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -12,7 +12,9 @@ from __future__ import annotations +import os import re +import stat import subprocess import sys from enum import StrEnum @@ -21,6 +23,12 @@ from .. import runs from ..adapters.multiplexer import MultiplexerError, get_multiplexer, mux_usable from ..journal import Journal +from ..platform_util import ( + DIR_FD_ANCHORED_WRITES, + atomic_write_text, + atomic_write_text_at, + open_dir_confined, +) CTL_SESSION = "bmad-loop-ctl" @@ -43,7 +51,292 @@ def session_exists(session: str) -> bool: return get_multiplexer().has_session(session) -def ctl_window_id(run_id: str) -> str | None: +# Run-dir sidecar naming the ctl-session window start_detached minted last for +# this run. `-` is not unique across the four kinds, so the window +# listing alone cannot tell a live resume window from the parked run window it +# superseded — this file names the one we actually created. A hint, never a +# target on its own: ctl_window_id re-proves it against the live listing. +_CTL_WINDOW_FILE = "ctl-window" + + +# Generous ceiling on the hint: the value is a window id (`@7`, or a +# session-qualified `bmad-loop-ctl:@7`), and anything longer is already not one. +_MAX_RECORD_BYTES = 256 + + +def _read_ctl_window(project: Path, run_id: str) -> str | None: + """The window id recorded by the run's last launch, or None when there is + none / it cannot be read. Never raises, and that includes decoding: a torn + record can raise UnicodeDecodeError, a ValueError rather than an OSError, + which action_attach (no covering except at all) and _stop_run_worker (whose + except does not include it) would let escape. An unreadable hint is not an + error — it just leaves the caller with the name scan. + + The file is the only channel on purpose: `bmad-loop attach` resolves the same + run from its own process, and one resolve feeding every consumer is the + property ctl_window_id sells. A per-process memo of what this process last + minted would answer a different window than the CLI does. + + Deliberately not `read_text`. The record sits under the project root every + coding session can write, and this read runs on Textual's event loop + (`action_attach` calls it directly), so the *shape* of what is at the path + has to be established before any bytes are consumed: + + * `O_NONBLOCK` + an `S_ISREG` check on the opened descriptor. Opening a FIFO + for reading otherwise blocks until someone writes — indefinitely, freezing + the dashboard on a keypress. + * `O_NOFOLLOW`, so the name is read rather than wherever it points. + * At most `_MAX_RECORD_BYTES`. A record pointed at an endless source reads + forever otherwise, and it raises `MemoryError` rather than the OSError + this promises never to leak — `Exception` would catch that but also mask + real bugs, where a cap removes the condition instead of absorbing it. + + The check is on the descriptor, not the path, so it cannot be raced: fstat + describes the object actually opened. The POSIX-only flags degrade to 0 on + win32, which has neither FIFOs at these paths nor O_NOFOLLOW; the size cap + and the regular-file check carry there on their own.""" + record = runs.run_dir_for(project, run_id) / _CTL_WINDOW_FILE + flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_NONBLOCK", 0) + flags |= getattr(os, "O_BINARY", 0) # win32: no CRLF translation on the raw fd + try: + fd = os.open(record, flags) + except OSError: + return None + try: + if not stat.S_ISREG(os.fstat(fd).st_mode): + return None + data = os.read(fd, _MAX_RECORD_BYTES) + except OSError: + return None + finally: + os.close(fd) + try: + return data.decode("utf-8").strip() or None + except UnicodeDecodeError: + return None + + +def _forget_ctl_window(project: Path, run_id: str) -> None: + """Drop the record. A launch that cannot name the window it just minted must + not leave the *previous* launch's id authoritative — that id now names a + superseded window, and the honest answer is no record at all, which puts the + lookup back on the name scan. + + Ceiling: when the removal fails, or is declined because the path cannot be + vouched for, the superseded id survives on disk. It still has to pass + ctl_window_id's re-prove, so the worst it can answer is a live window + carrying this run's name — the pre-fix by-name result, never a wilder target. + Retaining a stale hint is strictly the cheaper failure here, which is why + this declines rather than deleting on a path it cannot stand behind. + + Anchored exactly like the write in _record_ctl_window, and for a sharper + reason: a delete needs no race at all. `unlink` does not follow a link at the + *final* component, but the ancestors resolve normally, so a run dir standing + as a link to an external directory makes `run_dir / ctl-window` name a file + over there — another project's live record — and this deletes it. The write + path's escape needed the attacker to win a window between check and write; + a planted link just sits there until the next launch fails to capture an id. + So the descriptor from `open_dir_confined` is what the unlink is relative to, + and no path is named. win32 keeps the check-then-delete fallback on the same + terms as the write — see `_run_dir_is_confined` for that residual. + + A plain unlink, not retrying_unlink: launches run on the Textual event + loop, and dropping a best-effort hint is not worth ~5s of blocked win32 + backoff — the ceiling above already covers the miss.""" + run_dir = runs.run_dir_for(project, run_id) + try: + if DIR_FD_ANCHORED_WRITES: + dir_fd = open_dir_confined(project, run_dir) + if dir_fd is None: + return # a component we cannot vouch for — see the ceiling + try: + os.unlink(_CTL_WINDOW_FILE, dir_fd=dir_fd) + except FileNotFoundError: + pass # already gone: missing_ok, by hand + finally: + os.close(dir_fd) + else: + if not _run_dir_is_confined(project, run_dir): + return # see the ceiling + (run_dir / _CTL_WINDOW_FILE).unlink(missing_ok=True) + except OSError: + pass # a removal we cannot force — see the ceiling + + +def _is_link_of_any_kind(path: Path) -> bool: + """Whether `path` is a link that redirects traversal — symlink or, on win32, + a junction. Raises `OSError` for a component that cannot be probed, which + the caller turns into a refusal. + + `is_symlink()` alone is not enough, and the gap is win32-shaped. It answers + for the symlink reparse tag only and returns **False** for a directory + junction, which redirects traversal identically. A junction is also the + *easier* plant of the two: `mklink /J` needs neither elevation nor Developer + Mode, while a symlink needs one of them. So the check this backs would have + been blind on win32 to the cheaper version of the very attack it exists for. + + Detected by the reparse-point attribute rather than `os.path.isjunction`, + which only exists from 3.12 — this project supports 3.11, and that leg is + one CI runs on win32. One `lstat`, no version branch: `st_file_attributes` + is win32-only, so the bit test degrades to False on POSIX, where `S_ISLNK` + is already the whole answer. + + Any reparse point counts, not just the junction tag. Other kinds (cloud + placeholders, app-exec links) have no business being a run dir, and the + failure this produces is a refusal to write a best-effort hint — the lookup + degrades to the name scan. Over-refusing is the cheap direction here.""" + info = os.lstat(path) + if stat.S_ISLNK(info.st_mode): + return True + attributes = getattr(info, "st_file_attributes", 0) # win32-only field + return bool(attributes & stat.FILE_ATTRIBUTE_REPARSE_POINT) + + +def _run_dir_is_confined(project: Path, run_dir: Path) -> bool: + """Whether `run_dir` is reached from `project` without traversing a link. + + `follow_symlinks=False` refuses a link at the *final* component only, which + leaves the ancestors: a session that replaces `.bmad-loop/runs/` + with a link to an external directory holding a `state.json` passes + `runs.is_run` — it follows the link — and then `mkstemp`/`os.replace` land + the record inside the linked-to directory. The escape is narrower than the + final-component one (the name written is always `ctl-window`, so the reach + is another project's record rather than any file), but it is the same shape. + + Every component below `project` is checked, and `project` itself is not: the + operator chooses where the project lives and may well keep it behind a link, + while everything under it is session-writable. `lstat`-based throughout, so + the check never resolves through what it is testing for. + + Each component goes through `_is_link_of_any_kind`, not `is_symlink()` — + on win32 the latter is blind to a junction, which redirects the same way and + is the easier of the two to plant. That also fixes a quieter gap: `Path`'s + predicates swallow the `OSError` from a component that cannot be probed and + answer False, so an unreadable ancestor used to be walked *past* as "not a + link" — the opposite of the sentence below. Raising from the probe is what + makes that sentence true. + + A check, not a race-free open: the portable answer would be to walk the + components with `dir_fd`, which POSIX has and win32 does not, and this + record is atomic precisely for the win32 leg. So the standing redirect — + plant a link, wait for a launch — is what this removes; a session that + re-plants inside the window between check and write still wins. That + residual is bounded by the two facts above: same uid as the writer, and a + fixed filename carrying a window id.""" + try: + if not run_dir.is_relative_to(project): + return False + cursor = run_dir + while cursor != project: + if _is_link_of_any_kind(cursor): + return False + cursor = cursor.parent + except OSError: + return False # a component we cannot probe is one we cannot vouch for + return True + + +def _record_ctl_window(project: Path, run_id: str, win_id: str) -> None: + """Record the window a launch just minted, so ctl_window_id can prefer it + over an older window sharing the run id. + + Best-effort on purpose. The window is already running by the time this + writes, so a failed write must not fail the launch — the lookup degrades to + the name scan, i.e. to the behaviour before this record existed. A failure + forgets the previous record rather than leaving it: degrading to the scan is + the intended fallback, answering a superseded window is not. + + Skipped when there is no run yet: a fresh `run`/`sweep` mints the only + window carrying its run id (nothing to disambiguate), and the run dir is + created by the detached child — this record deliberately never mkdirs one, + and must not be written into a run-dir-shaped directory (pruned, partial) + that runs.is_run reports as not a run. + + That skip forgets too, and the "nothing to disambiguate" clause above is + exactly why it must. The clause holds for the case it was written for — + `new_run_id` mints a fresh id, so no other window carries it — but it does + not hold for every way of reaching this branch. resume/resolve read state, + raise a confirm modal, and launch from the callback, so anything that + removes `state.json` inside that human-length window arrives here with a + predecessor window live and a previous launch's record still on disk. That + record names the window this launch just superseded, and ctl_window_id + prefers any record that still resolves — so `a` and `x` would answer the + parked predecessor while the orchestrator just minted keeps running, which + is #482's symptom reintroduced by the record meant to fix it. Dropping it + puts the lookup back on the name scan, which is this file's stated + preference throughout: degrading to the scan is the intended fallback, + answering a superseded window is not. + + Atomic, not a bare write_text: the record is read cross-process (`bmad-loop + attach`), and on win32 an AV/indexer holding the previous record open fails + a plain overwrite with a transient sharing violation — which would swallow + into the forget path and quietly degrade the lookup. atomic_replace retries + exactly that violation, turning most real-world failures into successes. + + The guard is type-agnostic on purpose, and `OSError` is not wide enough to + hold it: `atomic_write_text` resolves the path before its own try, and below + 3.13 `Path.resolve` reports a symlink loop as `RuntimeError` — which would + crash the launch this docstring promises to spare, on the interpreters the + 3.11/3.12 legs run. Same widening, same reason, as the engine's deferred-close + rollback (`Engine._restore_deferred_closes`). `Exception` and not + `BaseException`, so a genuine KeyboardInterrupt still gets out. + + `follow_symlinks=False`, so a symlink at the path is replaced rather than + written through. Following one is the helper's default contract ("a ledger + symlinked into the repo keeps being a symlink"), and it is right there — for + an operator-curated ledger. This sidecar is the opposite: machine-minted, + per-run, disposable, and living under the project root that every coding + session can write. Honouring a link here would let a session aim a + *host-side* write at any path the user can write — reach that the adapters + confining a session to the workspace otherwise deny it. The payload is only + a window id, so the primitive is truncation rather than injection, which + bounds the damage without making it acceptable. + + Replacing rather than refusing, and no preflight `is_symlink` check: a check + leaves the window between itself and the write, which a session that + re-plants the link wins. `os.replace` does not dereference its destination, + so the link is clobbered whenever it was planted. That also self-heals — the + record ends up a plain file again — where a refusal would leave the planted + link in place for the next launch to trip over. + + Anchored at a directory descriptor where the platform has one. The final + component is covered by `follow_symlinks=False` above, but the *ancestors* + are not, and a path check over them (`_run_dir_is_confined`) is answered + about a path — stale the moment it returns, so a session that re-plants + `.bmad-loop/runs/` between check and write still redirects the + record out of the workspace. `open_dir_confined` walks those components + `O_NOFOLLOW` and hands back the descriptor for the directory it reached, and + `atomic_write_text_at` then never names a path again — so a later swap + renames something this no longer consults, and there is no window to win. + + win32 keeps the check-then-write path: it has no `*at()` family to anchor + against (its CPython config defines neither HAVE_RENAMEAT nor HAVE_OPENAT), + so the descriptor cannot be opened there at all. The residual is documented + on `_run_dir_is_confined` and bounded by the two facts it names — same uid + as the writer, and a fixed filename carrying a window id. + """ + run_dir = runs.run_dir_for(project, run_id) + if not runs.is_run(run_dir): + _forget_ctl_window(project, run_id) + return + try: + if DIR_FD_ANCHORED_WRITES: + dir_fd = open_dir_confined(project, run_dir) + if dir_fd is None: + return # unconfined, or a component we cannot vouch for + try: + atomic_write_text_at(dir_fd, _CTL_WINDOW_FILE, win_id) + finally: + os.close(dir_fd) + else: + if not _run_dir_is_confined(project, run_dir): + return + atomic_write_text(run_dir / _CTL_WINDOW_FILE, win_id, follow_symlinks=False) + except Exception: + _forget_ctl_window(project, run_id) + + +def ctl_window_id(project: Path, run_id: str) -> str | None: """Stable window id (bare `@N` on tmux, session-qualified on psmux) of the control-session window hosting this run's orchestrator process (start_detached names windows -), or None when the run was @@ -52,20 +345,112 @@ def ctl_window_id(run_id: str) -> str | None: An id, not a name, because every consumer replays the value as a select/kill/option target: one resolve feeds all of them, so a rename or a window minted between two verbs cannot send them to different windows, and - the value survives tmux's automatic-rename. It does NOT disambiguate the - run_id — `-` is not unique (a resume launched over a still- - parked run window shares it), and this scan takes the first match, the - same window a by-name lookup returned.""" + the value survives tmux's automatic-rename. + + `-` is not unique — a resume launched over a still-parked run + window shares the run id, and nothing reaps the parked one in between — so + the name scan alone answers whichever match the listing emits first (tmux + orders by window *index*, and it gives a new window the lowest free index, + so a superseded window usually but not always sorts ahead of the live one). + The id the run's last launch minted is recorded in the run dir and wins + whenever the listing still shows it under this run id. A record that is gone + (killed, pruned) or now carries another run's name is ignored rather than + replayed: a target that no longer resolves is the dangerous kind of stale — + on psmux an unresolvable `-t` lands on the *active* window (psmux/psmux#545; + tmux merely errors, which the best-effort consumers turn into a silent + no-op). With no record at all the answer is the first match, exactly as + before. + + Scoped to `project` by the PROJECT_OPTION tag, on the same rule as + _ctl_window_candidates: the control session is shared across projects, and a + run id is only unique within one (`--run-id` is caller-supplied), so a + same-id window belonging to another project would otherwise be a legal match + here — for `x` that means killing a *live* orchestrator next door. An + untagged window is admitted when this project has the run dir, which keeps a + window whose (best-effort) tag write failed reachable by its own project + rather than by nobody. + + Untagged is a *fallback*, not a peer: an untagged window proves nothing + about who owns it, so it is consulted only when nothing carries this + project's tag. Merged into one listing-ordered list they would compete on + index, and a neighbouring project's untagged window listed first would beat + this project's correctly tagged one — for `x`, killing next door's + orchestrator. That case is not hypothetical: the record cannot break the tie + for a fresh `run`, where recording is deliberately skipped.""" if not mux_available(): return None - for win_id, name in get_multiplexer().list_windows(CTL_SESSION, ["window_id", "window_name"]): + mine = runs.project_tag(project) + local = runs.is_run(runs.run_dir_for(project, run_id)) + tagged: list[str] = [] + untagged: list[str] = [] + rows = get_multiplexer().list_windows( + CTL_SESSION, ["window_id", "window_name", runs.PROJECT_OPTION] + ) + for win_id, name, tag in rows: # win_id can be "": psmux's qualifier passes a falsy id through. An # empty id must never become a target — an empty `-t` resolves against - # the *current* window. (The base's short-row padding cannot produce it - # here: it fills TRAILING fields, and window_id is field 0 of 2.) - if win_id and name.endswith(f"-{run_id}"): - return win_id - return None + # the *current* window. (The base's short-row padding CAN produce an + # empty *tag* — it fills trailing fields — which is exactly the untagged + # case below; window_id stays field 0 of 3.) + if not win_id: + continue + # The whole run id, not a suffix of the name: RUN_ID_RE admits `-`, so + # `--run-id other-RID` mints `run-other-RID`, which ends with `-RID` and + # would answer a lookup for `RID` — and sorts ahead of it, so `x` kills + # the neighbour's LIVE orchestrator. Parsed with the same regex + # _ctl_window_candidates uses, which also confines a match to the four + # kinds start_detached mints rather than any name ending this way. + 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: + tagged.append(win_id) + elif not tag and local: + # untagged, and this project holds the run dir — ownership is + # plausible but unproven, so it only counts if nothing is tagged + untagged.append(win_id) + matches = tagged or untagged + if not matches: + return None + # Membership in `matches`, not mere presence in the listing: it re-proves the + # name and the project too, so neither a backend that reuses a freed window + # id nor a record naming a neighbouring project's window can be replayed. + recorded = _read_ctl_window(project, run_id) + return recorded if recorded in matches else matches[0] + + +def ctl_window_recorded(project: Path, run_id: str, win_id: str) -> bool: + """Whether `ctl_window_id` now answers `win_id` for this run — i.e. whether + the launch's disambiguation actually took. + + False means the launch itself succeeded but the lookup is back on the + ambiguous first-match scan, which is exactly #482's symptom and so is + operator-visible: every launcher that mints a second window under a run id + should report it rather than let an unqualified success toast imply the + targeting is sound. Split out of resume_detached's return so the resolve + path can warn while still keeping the captured id it attaches with. + + Asks `ctl_window_id` rather than comparing the record to `win_id`, because + a round-tripped record is not the same claim. A backend whose + `new_parked_window` id is shaped differently from its `list_windows` + `window_id` column — a divergence the seam explicitly tolerates — writes and + reads the record back intact while `ctl_window_id` rejects it against the + listing and falls through to the first match. File equality would report + that as sound; it is the precise case the warning exists for. + + An unanswerable listing counts as not recorded. The probe is observation, so + it degrades rather than raising into the launchers (neither has a handler + for it), and "could not confirm" is closer to the warning's own hedge — + attach/stop *may* target an older window — than silence would be.""" + try: + return ctl_window_id(project, run_id) == win_id + except MultiplexerError: + return False def ctl_target() -> str: @@ -230,7 +615,7 @@ def attach_plan(project: Path, run_id: str) -> tuple[list[str], str | None] | No live agent session. Returns (tmux argv, return_window) or None when there is nothing to attach to.""" session = runs.session_name(run_id) - win_id = ctl_window_id(run_id) + win_id = ctl_window_id(project, run_id) agent_live = session_exists(session) if win_id is not None and ( decision_pending(runs.run_dir_for(project, run_id)) or not agent_live @@ -242,10 +627,10 @@ def attach_plan(project: Path, run_id: str) -> tuple[list[str], str | None] | No return None -def kill_ctl_window(run_id: str) -> None: +def kill_ctl_window(project: Path, run_id: str) -> None: """Kill the control-session window hosting this run's orchestrator process, if any. A no-op when the run was not launched from the TUI or tmux is gone.""" - win_id = ctl_window_id(run_id) + win_id = ctl_window_id(project, run_id) if win_id is not None: get_multiplexer().kill_window(win_id) @@ -341,7 +726,9 @@ def start_detached(project: Path, argv_tail: list[str], run_id: str, kind: str) Returns the new window's stable backend id (bare `@N` on tmux, session-qualified on psmux) so callers can target it unambiguously (window - names collide when several kinds share a run_id). + names collide when several kinds share a run_id). The same id is recorded in + the run dir so ctl_window_id answers this window rather than an older one + under the same run id — see _record_ctl_window. """ mux = get_multiplexer() if not mux_usable(mux): @@ -364,9 +751,19 @@ def start_detached(project: Path, argv_tail: list[str], run_id: str, kind: str) except MultiplexerError as e: raise LaunchError(f"multiplexer new-window failed: {e}") from e if win_id: + # Record before tagging: a window minted but unrecorded puts the lookup + # back on the ambiguous scan, while an *untagged* window already has a + # documented fallback in _ctl_window_candidates — so even a + # non-conforming backend raising from the (contractually best-effort) + # set_window_option must not cost the record. + _record_ctl_window(project, run_id, win_id) # Tag the window with its project so a cleanup in another project never # closes it (the ctl session is shared across projects). mux.set_window_option(win_id, runs.PROJECT_OPTION, runs.project_tag(project)) + else: + # No id to record: the backend did not capture one. Whatever the previous + # launch recorded now names a superseded window, so drop it. + _forget_ctl_window(project, run_id) return win_id @@ -409,8 +806,32 @@ def start_sweep_detached( start_detached(project, tail, run_id, "sweep") -def resume_detached(project: Path, run_id: str) -> None: - start_detached(project, ["resume", "--project", str(project), run_id], run_id, "resume") +def resume_detached(project: Path, run_id: str) -> str | None: + """Resume in a ctl-session window; returns the window id, or None when the + lookup cannot name that window afterwards — the caller should warn, because + resume is the launch that mints a *second* window under the run id, so this + is exactly when the ambiguous scan starts answering the superseded one while + the launch itself succeeded. + + Two ways to land there, one signal: the backend captured no id, or it did but + the record did not survive (refused, unwritable, run dir pruned mid-launch). + Both leave `ctl_window_id` on the scan, so reporting only the first would let + the rest degrade behind an unqualified success toast. + + Verified by re-reading rather than by threading the write's outcome up: it + asks the question the consumers actually ask — will `ctl_window_id` prefer + this window — of the same file they will read, instead of a proxy for it. + + Folded into the return here, rather than reported alongside the id as the + resolve path does, because resume has no immediate use for a window it + cannot record: it launches and leaves, where resolve attaches to the window + it just minted and still needs that id to do so.""" + win_id = start_detached( + project, ["resume", "--project", str(project), run_id], run_id, "resume" + ) + if win_id and not ctl_window_recorded(project, run_id, win_id): + return None + return win_id def start_resolve_detached(project: Path, run_id: str) -> str | None: diff --git a/tests/test_cli.py b/tests/test_cli.py index ad6e6ee0..5b1ef12d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1501,10 +1501,12 @@ def test_attach_records_return_pane_inside_tmux(project, monkeypatch): from bmad_loop.tui import launch _make_run_with_decision(project, run_id="20260101-000000-aaaa") + planned: list = [] monkeypatch.setattr( launch, "attach_plan", - lambda proj, rid: ( + lambda proj, rid: planned.append((proj, rid)) + or ( ["tmux", "switch-client", "-t", "=bmad-loop-ctl"], "=bmad-loop-ctl:sweep-RID", ), @@ -1519,6 +1521,9 @@ def test_attach_records_return_pane_inside_tmux(project, monkeypatch): assert cli.main(["attach", "--project", str(project.project), "20260101-000000-aaaa"]) == 0 assert recorded == [("=bmad-loop-ctl:sweep-RID", "=main:%3")] assert called == [["tmux", "switch-client", "-t", "=bmad-loop-ctl"]] + # The *value* of the project argument, not just the arity: attach_plan finds + # the run's recorded ctl window only under the --project root (#482). + assert planned == [(project.project, "20260101-000000-aaaa")] def test_attach_records_detach_outside_tmux(project, monkeypatch): diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index 45f13794..d2bbfb9b 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -300,6 +300,38 @@ def fake_run(argv, **kwargs): assert seen["argv"] == ["tmux", "list-panes", "-t", "@7", "-F", "#{pane_pid}"] +def test_tmux_list_windows_keeps_tabs_inside_the_trailing_field(monkeypatch): + """The last requested field may legally contain the delimiter. + + PROJECT_OPTION carries a resolved filesystem path, and a tab is a legal + POSIX filename byte — so an unbounded split turns one row into four parts + and the field slice then drops the tail, handing the caller a *truncated* + path. That does not read as "no tag", it reads as another project's tag, so + the comparison sites discard the project's own windows. The bounded split + keeps the remainder in the field it belongs to.""" + mux = TmuxMultiplexer() + monkeypatch.setattr( + tmux_base.subprocess, + "run", + lambda argv, **k: subprocess.CompletedProcess( + argv, 0, stdout="@7\tresume-RID\t/home/u/my\tproj\n", stderr="" + ), + ) + rows = mux.list_windows("ctl", ["window_id", "window_name", "@bmad_project"]) + assert rows == [("@7", "resume-RID", "/home/u/my\tproj")] + + # Unchanged where it always held: short rows still pad trailing fields, and + # a single-field request still takes the whole line. + monkeypatch.setattr( + tmux_base.subprocess, + "run", + lambda argv, **k: subprocess.CompletedProcess(argv, 0, stdout="@7\trun-RID\n", stderr=""), + ) + assert mux.list_windows("ctl", ["window_id", "window_name", "@bmad_project"]) == [ + ("@7", "run-RID", "") + ] + + @pytest.mark.parametrize( "outcome", [ diff --git a/tests/test_platform_util.py b/tests/test_platform_util.py index 7db84827..79e99427 100644 --- a/tests/test_platform_util.py +++ b/tests/test_platform_util.py @@ -316,6 +316,68 @@ def test_atomic_write_text_writes_through_a_symlink(tmp_path): assert real.read_text(encoding="utf-8") == "after" +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_atomic_write_text_no_follow_replaces_the_link(tmp_path): + """The inverse contract, for a machine-minted file somewhere a less-trusted + writer can reach: honouring a planted link would aim the write at a path of + that writer's choosing, so the *name* is what gets replaced. + + No preflight check is what makes it safe — `os.replace` does not dereference + its destination, so a link planted at any moment, including after a check + would have run, is clobbered rather than written through.""" + real = tmp_path / "someone-elses-file" + real.write_text("before", encoding="utf-8") + link = tmp_path / "record" + link.symlink_to(real) + + platform_util.atomic_write_text(link, "after", follow_symlinks=False) + + assert not link.is_symlink() + assert link.read_text(encoding="utf-8") == "after" + assert real.read_text(encoding="utf-8") == "before" # untouched + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits") +def test_atomic_write_text_no_follow_does_not_inherit_a_link_targets_mode(tmp_path): + """A name being replaced rather than updated carries nothing of whatever it + used to point at — inheriting the target's mode would let a planted link + choose the new record's permissions.""" + real = tmp_path / "someone-elses-file" + real.write_text("before", encoding="utf-8") + real.chmod(0o666) + link = tmp_path / "record" + link.symlink_to(real) + + platform_util.atomic_write_text(link, "after", follow_symlinks=False) + + assert stat.S_IMODE(link.stat().st_mode) == 0o600 # mkstemp's private default + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits") +def test_atomic_write_text_no_follow_does_not_inherit_a_plain_files_mode(tmp_path): + """No-follow inherits nothing, and takes no probe to decide it — the sibling + above covers the link; this covers the plain file, which is the case a probe + would have said yes to. + + Inheriting here needs a shape check and then a `copymode`, and `copymode` + re-resolves: a writer who plants a link in that gap chooses the new record's + permissions. The probe is what makes that gap exist, so there is none. 0o640, + for the reason the follow-mode pins give — `mkstemp` already arrives at 0600, + so only a mode it does NOT arrive with can tell inheritance from its absence. + + The pairing is the ablation: restore the probe-and-copy and this reddens + while the link sibling stays green, so it bites on inheritance itself rather + than on anything the no-follow path does incidentally.""" + target = tmp_path / "record" + target.write_text("before", encoding="utf-8") + target.chmod(0o640) + + platform_util.atomic_write_text(target, "after", follow_symlinks=False) + + assert target.read_text(encoding="utf-8") == "after" + assert stat.S_IMODE(target.stat().st_mode) == 0o600 + + def test_atomic_write_text_preserves_extended_attributes(tmp_path): """`os.replace` swaps a fresh inode into place, so anything carried by the old inode rather than by its name is silently reset — xattrs included, which on a diff --git a/tests/test_runs.py b/tests/test_runs.py index 609fb053..49d276f0 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -6,6 +6,7 @@ import subprocess import sys import tarfile +from pathlib import Path import pytest from conftest import escalated_run, git @@ -623,6 +624,120 @@ def test_mux_sessions_no_server(monkeypatch): assert runs.mux_sessions() == [] +# Everything `str.splitlines()` breaks on. Spelled as escapes on purpose: the +# literals are invisible in a diff, and a raw U+2028/U+2029 in a source file +# makes every splitlines()-based tool disagree with Python's tokenizer about +# which line anything after it is on. +_LINE_SEPARATORS = [ + ("LF", "\n"), + ("CR", "\r"), + ("CRLF", "\r\n"), + ("VT", "\v"), + ("FF", "\f"), + ("FS", "\x1c"), + ("GS", "\x1d"), + ("RS", "\x1e"), + ("NEL", "\x85"), + ("LS", "\u2028"), + ("PS", "\u2029"), +] +_SEP_VALUES = [s for _, s in _LINE_SEPARATORS] +_SEP_IDS = [i for i, _ in _LINE_SEPARATORS] + + +@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) + 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 diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index d32f6480..bf5c6499 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -1960,6 +1960,31 @@ async def test_resume_confirm_launches(project, monkeypatch): await until(pilot, lambda: calls == ["20260611-100000-aaaa"]) +async def test_resume_uncaptured_window_id_warns(project, monkeypatch): + # The resume itself is running; only the #482 disambiguation record is lost, + # so attach/stop may target an older same-run_id window. The success toast + # must not mask that (the resolve path already errors on this condition). + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "resume_detached", lambda proj, rid: None) + monkeypatch.setattr(data, "liveness", lambda run_dir: "dead") + make_run( + project.project, + "20260611-100000-aaaa", + paused_stage="DEV_VERIFY", + paused_reason="verify failed", + ) + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await until(pilot, lambda: dashboard(app).selected_run_id is not None) + await pilot.press("e") + await until(pilot, lambda: isinstance(app.screen, ConfirmResumeModal)) + await pilot.click(await ready(pilot, "#ok")) + await until( + pilot, lambda: any("window id was not recorded" in m for m in notifications(app)) + ) + + async def test_resume_unknown_pid_warns(project, monkeypatch): monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(data, "liveness", lambda run_dir: "unknown") @@ -2082,7 +2107,7 @@ async def test_attach_without_mux_notifies(project, monkeypatch): async def test_attach_without_agent_session_notifies(project, monkeypatch): monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: False) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: None) make_run(project.project, "20260611-100000-aaaa") app = BmadLoopApp(project.project) async with app.run_test() as pilot: @@ -2099,7 +2124,7 @@ async def test_attach_multiplexer_error_notifies(project, monkeypatch): # TUI must surface the error as a toast, not crash the app. monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: True) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: None) def boom(_target): raise MultiplexerError("backend server not reachable") @@ -2123,7 +2148,7 @@ async def test_attach_session_probe_error_notifies(project, monkeypatch): # torn down in between). action_attach routes it through _mux_guarded, so the # TUI toasts the error and aborts the attach instead of crashing the app. monkeypatch.setattr(launch, "mux_available", lambda: True) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: None) def boom(_session): raise MultiplexerError("session probe unreachable") @@ -2214,7 +2239,7 @@ async def test_attach_targets_ctl_window_when_decision_pending(project, monkeypa selected: list[str] = [] monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: True) # agent up too - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: "@5") monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) calls, stamps = _patch_attach_exec(monkeypatch) app = BmadLoopApp(project.project) @@ -2229,6 +2254,42 @@ async def test_attach_targets_ctl_window_when_decision_pending(project, monkeypa assert stamps == [("@5", "=main:%9")] +@pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals +async def test_attach_uses_the_recorded_ctl_window(project, monkeypatch): + # The one attach test that does NOT replace ctl_window_id, so it pins the + # seam every other one stubs out: that the TUI hands it the same project root + # the launch recorded the window under (#482). Point app.py at anything else + # — the run dir, an unresolved path — and the record is unfindable, the scan + # answers the parked `run-` corpse, and attach + return-stamp both go there. + import subprocess as _subprocess + + from bmad_loop.adapters import tmux_base + + rid = "20260611-100000-aaaa" + run_dir = make_run(project.project, rid, run_type="sweep", alive=True) + Journal(run_dir).append("decision-pending", dw_id="DW-7", question="q?") + (run_dir / launch._CTL_WINDOW_FILE).write_text("@2", encoding="utf-8") + selected: list[str] = [] + + def fake(argv, **kwargs): + out = f"@1\trun-{rid}\n@2\tresume-{rid}\n" if argv[1] == "list-windows" else "" + return _subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + monkeypatch.setattr(launch, "session_exists", lambda session: True) + monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) + calls, stamps = _patch_attach_exec(monkeypatch) + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await until(pilot, lambda: dashboard(app).decision_pending is not None) + await pilot.press("a") + await until(pilot, lambda: bool(calls)) + assert selected == ["@2"] + assert stamps == [("@2", "=main:%9")] + + @pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals async def test_attach_outside_tmux_stamps_detach(project, monkeypatch): # No TMUX: a throwaway client attaches under suspend, so the ctl window is @@ -2240,7 +2301,7 @@ async def test_attach_outside_tmux_stamps_detach(project, monkeypatch): stamps: list[tuple[str, str]] = [] monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: True) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: "@5") monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: None) monkeypatch.setattr(launch, "set_return_pane", lambda w, p: stamps.append((w, p))) app = BmadLoopApp(project.project) @@ -2257,7 +2318,7 @@ async def test_attach_prefers_agent_session_without_decision(project, monkeypatc make_run(project.project, "20260611-100000-aaaa", alive=True) monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: True) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: "@5") calls, stamps = _patch_attach_exec(monkeypatch) app = BmadLoopApp(project.project) async with app.run_test() as pilot: @@ -2276,7 +2337,7 @@ async def test_attach_falls_back_to_ctl_window(project, monkeypatch): selected: list[str] = [] monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(launch, "session_exists", lambda session: False) - monkeypatch.setattr(launch, "ctl_window_id", lambda run_id: "@5") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, run_id: "@5") monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) calls, stamps = _patch_attach_exec(monkeypatch) app = BmadLoopApp(project.project) @@ -2304,6 +2365,10 @@ def fake_start_resolve(proj, rid): monkeypatch.setattr(launch, "start_resolve_detached", fake_start_resolve) monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) calls, stamps = _patch_attach_exec(monkeypatch) + # The healthy path: the lookup answers the window the launch minted, so no + # warning. Stubbed at the same seam every other attach test stubs — the + # helper's own listing/record logic is pinned in tests/test_tui_launch.py. + monkeypatch.setattr(launch, "ctl_window_recorded", lambda proj, rid, wid: True) make_run( project.project, "20260611-100000-aaaa", @@ -2318,6 +2383,7 @@ def fake_start_resolve(proj, rid): await until(pilot, lambda: isinstance(app.screen, ConfirmModal)) await pilot.click(await ready(pilot, "#ok")) await until(pilot, lambda: bool(calls)) + assert not any("was not recorded" in m for m in notifications(app)) assert launched == ["20260611-100000-aaaa"] assert selected == ["@7"] assert calls == [["tmux", "switch-client", "-t", "=bmad-loop-ctl"]] @@ -2325,6 +2391,37 @@ def fake_start_resolve(proj, rid): assert stamps == [("@7", "=main:%9")] +@pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals +async def test_resolve_warns_when_the_record_did_not_survive(project, monkeypatch): + # The resolve path kept the captured id (it attaches with it) but never + # asked whether the record landed, so a failed write left `a`/`x` on the + # ambiguous scan behind a clean attach. Warn, and attach anyway: this + # window is reached by the id in hand, only later verbs are degraded. + selected: list[str] = [] + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(data, "liveness", lambda run_dir: "dead") + monkeypatch.setattr(launch, "start_resolve_detached", lambda proj, rid: "@7") + monkeypatch.setattr(launch, "ctl_window_recorded", lambda proj, rid, wid: False) + monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: selected.append(w)) + calls, _stamps = _patch_attach_exec(monkeypatch) + make_run( + project.project, + "20260611-100000-aaaa", + paused_stage="escalation", + paused_reason="CRITICAL escalation", + ) + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await until(pilot, lambda: dashboard(app).selected_run_id is not None) + await pilot.press("R") + await until(pilot, lambda: isinstance(app.screen, ConfirmModal)) + await pilot.click(await ready(pilot, "#ok")) + await until(pilot, lambda: any("was not recorded" in m for m in notifications(app))) + assert selected == ["@7"] # still attached to the window it minted + assert calls == [["tmux", "switch-client", "-t", "=bmad-loop-ctl"]] + + async def test_resolve_unknown_pid_refused(project, monkeypatch): launched: list[str] = [] monkeypatch.setattr(launch, "mux_available", lambda: True) @@ -2628,10 +2725,11 @@ async def test_story_checkpoint_stop_marks_stopped(project, monkeypatch): from bmad_loop import runs stops: list[Path] = [] + kills: list[tuple[Path, str]] = [] monkeypatch.setattr(launch, "mux_available", lambda: True) monkeypatch.setattr(data, "liveness", lambda run_dir: "dead") monkeypatch.setattr(runs, "stop_run", lambda rd: stops.append(rd) or True) - monkeypatch.setattr(launch, "kill_ctl_window", lambda rid: None) + monkeypatch.setattr(launch, "kill_ctl_window", lambda proj, rid: kills.append((proj, rid))) _stories_paused_run( project.project, stage="story-checkpoint", @@ -2643,7 +2741,9 @@ async def test_story_checkpoint_stop_marks_stopped(project, monkeypatch): async with app.run_test() as pilot: await _open_review(app, pilot, StoryCheckpointModal) await pilot.click(await ready(pilot, "#act-stop")) - await until(pilot, lambda: len(stops) == 1) + await until(pilot, lambda: len(kills) == 1) + assert stops == [project.project / runs.RUNS_DIR / "20260611-100000-aaaa"] + assert kills == [(project.project, "20260611-100000-aaaa")] def test_checkpoint_gate_line_pluralization(): diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index 48af8e22..b6dc5898 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -10,15 +10,19 @@ from __future__ import annotations import json +import os import shlex +import signal +import stat import subprocess import sys from pathlib import Path import pytest +from bmad_loop import runs from bmad_loop.adapters import tmux_base -from bmad_loop.adapters.multiplexer import get_multiplexer +from bmad_loop.adapters.multiplexer import MultiplexerError, get_multiplexer from bmad_loop.tui import launch # Every test here asserts tmux-specific argv/behaviour through the multiplexer @@ -29,16 +33,24 @@ class FakeRun: - """Records argv; scripts the returncode of `tmux has-session`.""" + """Records argv; scripts the returncode of `tmux has-session` and the rows + `list-windows` answers. The listing defaults to showing the window + `new-window` just minted, which is what a real backend does — and what + ctl_window_recorded re-proves the record against.""" - def __init__(self, has_session_rc: int = 1): + def __init__(self, has_session_rc: int = 1, windows: str = "@7\tresume-RID\n"): self.calls: list[list[str]] = [] self.has_session_rc = has_session_rc + self.windows = windows def __call__(self, argv, **kwargs): self.calls.append(list(argv)) rc = self.has_session_rc if argv[1] == "has-session" else 0 - out = "@7\n" if argv[1] == "new-window" else "" + out = "" + if argv[1] == "new-window": + out = "@7\n" + elif argv[1] == "list-windows": + out = self.windows return subprocess.CompletedProcess(argv, rc, stdout=out, stderr="") def by_verb(self, verb: str) -> list[list[str]]: @@ -189,7 +201,16 @@ def test_existing_ctl_session_reused(monkeypatch, tmp_path: Path): monkeypatch.setattr(tmux_base.subprocess, "run", fake) monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") launch.resume_detached(tmp_path, "RID") - assert [c[1] for c in fake.calls] == ["has-session", "new-window", "set-option"] + # No new-session: the ctl session already answered has-session. The trailing + # list-windows is resume's own check that the lookup now names the window it + # minted — the one launch that mints a second window under a run id pays for + # the answer it warns on. + assert [c[1] for c in fake.calls] == [ + "has-session", + "new-window", + "set-option", + "list-windows", + ] def test_launch_without_mux_raises(monkeypatch, tmp_path: Path): @@ -256,58 +277,306 @@ def test_session_exists(monkeypatch): assert fake.calls[0] == ["tmux", "has-session", "-t", "=bmad-loop-x"] -def test_ctl_window_id_matches_run_id_suffix(monkeypatch): - # The id, not the name: consumers replay the value as select/kill/option - # targets, where a by-name resolve can land on a duplicate. +def _ctl_listing(monkeypatch, rows: str, project: Path | None = None) -> list[list[str]]: + """Script the ctl-session window listing; returns the recorded argv. + + Rows are written as `\\tab`, and every row that does not already + carry a third field is tagged for `project` — the state start_detached + leaves behind, since it stamps PROJECT_OPTION on every window it mints. Pass + a row with its own third field to script another project's window (or an + empty one for the untagged, pre-tag-write case). + """ + if project is not None: + tag = runs.project_tag(project) + rows = "".join( + (line if line.count("\t") >= 2 else f"{line}\t{tag}") + "\n" + for line in rows.splitlines() + ) + calls: list[list[str]] = [] + def fake(argv, **kwargs): - out = "@1\trun-AAAA\n@2\tsweep-RID\n@3\tresume-BBBB\n" if argv[1] == "list-windows" else "" + calls.append(list(argv)) + out = rows if argv[1] == "list-windows" else "" return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") monkeypatch.setattr(tmux_base.subprocess, "run", fake) monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") - assert launch.ctl_window_id("RID") == "@2" - assert launch.ctl_window_id("CCCC") is None + return calls + + +def _write_record(project: Path, run_id: str, win_id: str) -> Path: + """Stand in for a launch having minted `win_id` for this run.""" + run_dir = runs.run_dir_for(project, run_id) + run_dir.mkdir(parents=True, exist_ok=True) + record = run_dir / launch._CTL_WINDOW_FILE + record.write_text(win_id, encoding="utf-8") + return record -def test_ctl_window_id_skips_empty_id_rows(monkeypatch): +def test_ctl_window_id_matches_run_id_suffix(monkeypatch, tmp_path: Path): + # The id, not the name: consumers replay the value as select/kill/option + # targets, where a by-name resolve can land on a duplicate. With no record + # of what the run's last launch minted, the answer is the first match. + _ctl_listing(monkeypatch, "@1\trun-AAAA\n@2\tsweep-RID\n@3\tresume-BBBB\n", tmp_path) + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + assert launch.ctl_window_id(tmp_path, "CCCC") is None + + +def test_ctl_window_id_requires_the_whole_run_id(monkeypatch, tmp_path: Path): + # `--run-id` is caller-supplied and RUN_ID_RE admits `-`, so one run id can + # be a suffix of another. A suffix test on `-RID` admits `run-other-RID`, + # which sorts first, so `x` would kill the LIVE other-RID orchestrator — + # the same wrong-window class as #482, one run over. The name is parsed and + # the captured id compared whole. + _ctl_listing(monkeypatch, "@1\trun-other-RID\n@2\tresume-RID\n", tmp_path) + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + # Positive control: the neighbour is still reachable under its own id, so + # this pins whole-id matching rather than merely refusing the collision. + assert launch.ctl_window_id(tmp_path, "other-RID") == "@1" + + +def test_ctl_window_id_prefers_the_window_the_last_launch_minted(monkeypatch, tmp_path: Path): + # #482: `e` over a parked run leaves `run-RID` in front of the live + # `resume-RID`, and the scan alone answers the parked corpse. The recorded + # id names the window we actually created. + _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n", tmp_path) + _write_record(tmp_path, "RID", "@2") + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + + +def test_ctl_window_id_ignores_a_record_the_listing_no_longer_shows(monkeypatch, tmp_path: Path): + # The recorded window was killed (`x`) or pruned. Replaying a target that no + # longer resolves is the dangerous kind of stale — an unresolvable `-t` + # lands on the *active* window — so fall back to a window that exists. + _ctl_listing(monkeypatch, "@1\trun-RID\n", tmp_path) + _write_record(tmp_path, "RID", "@2") + assert launch.ctl_window_id(tmp_path, "RID") == "@1" + + +def test_ctl_window_id_ignores_a_record_that_now_names_another_run(monkeypatch, tmp_path: Path): + # A backend that reuses a freed window id must not let a stale record hand + # back a foreign run's window: the record is re-proved against the name too. + _ctl_listing(monkeypatch, "@2\trun-OTHER\n@5\tresume-RID\n", tmp_path) + _write_record(tmp_path, "RID", "@2") + assert launch.ctl_window_id(tmp_path, "RID") == "@5" + + +def test_ctl_window_id_ignores_another_projects_window(monkeypatch, tmp_path: Path): + # The ctl session is shared across projects and `--run-id` is caller-supplied, + # so the same run id can name a window next door. Matching on the name alone + # makes that a legal answer — and `x` would kill a LIVE orchestrator in the + # other project. Only this project's tag counts. + other = runs.project_tag(tmp_path / "elsewhere") + _ctl_listing(monkeypatch, f"@1\trun-RID\t{other}\n@2\tresume-RID\n", tmp_path) + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + + +def test_ctl_window_id_ignores_a_record_naming_another_projects_window(monkeypatch, tmp_path: Path): + # And the record cannot smuggle one back in: it is re-proved against the + # scoped matches, so a record naming the neighbour's window is ignored + # rather than replayed as a kill/select target. + other = runs.project_tag(tmp_path / "elsewhere") + _ctl_listing(monkeypatch, f"@1\trun-RID\t{other}\n@2\tresume-RID\n", tmp_path) + _write_record(tmp_path, "RID", "@1") + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + + +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 + # rather than by nobody. Same rule as _ctl_window_candidates: untagged is + # admitted exactly when this project holds the run dir. + _ctl_listing(monkeypatch, "@4\tresume-RID\t\n", tmp_path) + _make_run(tmp_path) + assert launch.ctl_window_id(tmp_path, "RID") == "@4" + + +def test_ctl_window_id_refuses_an_untagged_window_without_a_local_run(monkeypatch, tmp_path: Path): + # The other half: untagged and no run dir here means ownership is + # unprovable, so the window is not claimed. Delete the `elif` and this + # returns "@4" — a window that may belong to any project on the box. + _ctl_listing(monkeypatch, "@4\tresume-RID\t\n", tmp_path) + assert launch.ctl_window_id(tmp_path, "RID") is None + + +def test_ctl_window_id_prefers_a_tagged_window_over_an_untagged_one(monkeypatch, tmp_path: Path): + # Untagged is a fallback, not a peer. Merged into one listing-ordered list, + # a neighbour's untagged window listed first beats this project's correctly + # tagged one — and for `x` that closes next door's orchestrator. The record + # cannot break the tie for a fresh `run`, where recording is skipped. + _ctl_listing(monkeypatch, "@1\trun-RID\t\n@2\trun-RID\n", tmp_path) + _make_run(tmp_path) # local run dir: the untagged row is otherwise admitted + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + + +def test_ctl_window_id_none_when_no_window_carries_the_run_id(monkeypatch, tmp_path: Path): + # A record can never resurrect a run whose windows are all gone. + _ctl_listing(monkeypatch, "@1\trun-OTHER\n@3\tshell\n", tmp_path) + _write_record(tmp_path, "RID", "@1") + assert launch.ctl_window_id(tmp_path, "RID") is None + + +def test_ctl_window_id_unreadable_record_falls_back(monkeypatch, tmp_path: Path): + # An unreadable hint is not an error — it just leaves the name scan. + _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n", tmp_path) + run_dir = runs.run_dir_for(tmp_path, "RID") + (run_dir / launch._CTL_WINDOW_FILE).mkdir(parents=True) # a dir, not a file + assert launch.ctl_window_id(tmp_path, "RID") == "@1" + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX FIFOs") +def test_read_record_does_not_block_on_a_fifo(tmp_path: Path): + # A session can replace its own workspace-writable record with a FIFO, and + # opening one for reading blocks until somebody writes. action_attach reads + # this on Textual's event loop, so that freezes the dashboard on a keypress. + # O_NONBLOCK returns immediately; the S_ISREG check on the opened descriptor + # then rejects it. Under an alarm because a regression here HANGS the suite — + # and with a handler that RAISES, so the ablation fails this test rather than + # letting the default SIGALRM disposition kill the whole pytest process. + run_dir = runs.run_dir_for(tmp_path, "RID") + run_dir.mkdir(parents=True) + os.mkfifo(run_dir / launch._CTL_WINDOW_FILE) + + # NOT TimeoutError: that is a subclass of OSError, so _read_ctl_window's own + # `except OSError` swallows it and the ablated code still returns None — the + # first version of this test passed against the bug, five seconds slower. + class Blocked(Exception): + pass + + def _blocked(_signum, _frame): + raise Blocked("_read_ctl_window blocked on a FIFO") + + previous = signal.signal(signal.SIGALRM, _blocked) + signal.setitimer(signal.ITIMER_REAL, 5) + try: + assert launch._read_ctl_window(tmp_path, "RID") is None + finally: + signal.setitimer(signal.ITIMER_REAL, 0) + signal.signal(signal.SIGALRM, previous) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX FIFOs") +def test_read_record_rejects_a_fifo_that_already_has_data(tmp_path: Path): + # The case only the S_ISREG check catches, and the reason it is not redundant + # with the other two guards: a writer holding the FIFO open with bytes queued + # means the open does not block (so O_NONBLOCK is not what refuses it) and + # the path is not a link (so O_NOFOLLOW is not either). Without the check the + # queued bytes are simply read, letting a session forge the record through a + # pipe it controls rather than a file. Ablate S_ISREG and this returns "@2". + run_dir = runs.run_dir_for(tmp_path, "RID") + run_dir.mkdir(parents=True) + fifo = run_dir / launch._CTL_WINDOW_FILE + os.mkfifo(fifo) + + writer = os.open(fifo, os.O_RDWR | os.O_NONBLOCK) # RDWR: no peer needed + try: + os.write(writer, b"@2") + assert launch._read_ctl_window(tmp_path, "RID") is None + finally: + os.close(writer) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX device nodes") +def test_read_record_rejects_a_non_regular_file(tmp_path: Path): + # O_NOFOLLOW, against the worst target a link can name. An endless source + # is what bites hardest — reading one raises MemoryError, not an OSError, so + # it would escape this function's "never raises" promise out through + # action_attach, which has no handler at all — but the open refuses the link + # before any of that, so this pins the refusal rather than the cap. + # + # NOT the S_ISREG check, despite reaching a device: O_NOFOLLOW fails the + # open first, so the descriptor never exists to fstat. Ablating S_ISREG + # leaves this test green (verified) — the queued-FIFO case above is the one + # that pins it, because there the open succeeds. + run_dir = runs.run_dir_for(tmp_path, "RID") + run_dir.mkdir(parents=True) + (run_dir / launch._CTL_WINDOW_FILE).symlink_to("/dev/zero") + + assert launch._read_ctl_window(tmp_path, "RID") is None + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_read_record_does_not_follow_a_symlink(tmp_path: Path): + # O_NOFOLLOW: the name is read, not wherever it points. The target here is a + # perfectly ordinary file holding a perfectly plausible window id, so every + # other guard passes it — only the no-follow refuses. Symmetry with the + # write side, which replaces the name rather than the link's target. + run_dir = runs.run_dir_for(tmp_path, "RID") + run_dir.mkdir(parents=True) + elsewhere = tmp_path / "somewhere-else" + elsewhere.write_text("@99", encoding="utf-8") + (run_dir / launch._CTL_WINDOW_FILE).symlink_to(elsewhere) + + assert launch._read_ctl_window(tmp_path, "RID") is None + + +def test_read_record_is_bounded(tmp_path: Path): + # The cap stands on its own, without the flags: a plain regular file can be + # arbitrarily large, and a hint is at most a window id either way. + run_dir = runs.run_dir_for(tmp_path, "RID") + run_dir.mkdir(parents=True) + (run_dir / launch._CTL_WINDOW_FILE).write_text("@" + "9" * 5_000_000, encoding="utf-8") + + recorded = launch._read_ctl_window(tmp_path, "RID") + assert recorded is not None and len(recorded) <= launch._MAX_RECORD_BYTES + + +def test_ctl_window_id_invalid_utf8_record_falls_back(monkeypatch, tmp_path: Path): + _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n", tmp_path) + record = _write_record(tmp_path, "RID", "@2") + record.write_bytes(b"\xff") + assert launch.ctl_window_id(tmp_path, "RID") == "@1" + + +def test_ctl_window_id_skips_empty_id_rows(monkeypatch, tmp_path: Path): # An empty id must never be returned as a target — an empty `-t` resolves # against the current window. psmux's qualifier passes a falsy id through. - def fake(argv, **kwargs): - out = "\tsweep-RID\n@7\tsweep-RID\n" if argv[1] == "list-windows" else "" - return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") - - monkeypatch.setattr(tmux_base.subprocess, "run", fake) - monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") - assert launch.ctl_window_id("RID") == "@7" + _ctl_listing(monkeypatch, "\tsweep-RID\n@7\tsweep-RID\n", tmp_path) + assert launch.ctl_window_id(tmp_path, "RID") == "@7" -def test_kill_ctl_window_kills_by_resolved_id_not_a_name_token(monkeypatch): +def test_kill_ctl_window_kills_by_resolved_id_not_a_name_token(monkeypatch, tmp_path: Path): # The kill replays the id this listing resolved, never a `=session:name` - # token the backend would resolve again. Which of two same-named windows - # the scan picks is unchanged (first match, `@7`); what the id buys is that - # a rename or a new window between two verbs cannot re-point the second. - calls: list[list[str]] = [] + # token the backend would resolve again. With no record the scan picks the + # first match (`@7`); what the id buys is that a rename or a new window + # between two verbs cannot re-point the second. + calls = _ctl_listing(monkeypatch, "@2\trun-x\n@7\tsweep-RID\n@9\tsweep-RID\n", tmp_path) + launch.kill_ctl_window(tmp_path, "RID") + assert ["tmux", "kill-window", "-t", "@7"] in calls - def fake(argv, **kwargs): - calls.append(list(argv)) - out = "@2\trun-x\n@7\tsweep-RID\n@9\tsweep-RID\n" if argv[1] == "list-windows" else "" - return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") - monkeypatch.setattr(tmux_base.subprocess, "run", fake) - monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") - launch.kill_ctl_window("RID") - assert ["tmux", "kill-window", "-t", "@7"] in calls +def test_attach_plan_selects_and_returns_the_recorded_window(monkeypatch, tmp_path: Path): + # #482's first two consequences: the window the attach lands on, and the one + # its return_window stamps @bmad_return_pane on, are the same live window. + calls = _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n", tmp_path) + _write_record(tmp_path, "RID", "@2") + monkeypatch.setattr(launch, "session_exists", lambda s: False) + monkeypatch.setattr(launch, "decision_pending", lambda rd: False) + plan = launch.attach_plan(tmp_path, "RID") + assert plan is not None + _argv, return_window = plan + assert return_window == "@2" + assert ["tmux", "select-window", "-t", "@2"] in calls + +def test_kill_ctl_window_follows_the_record(monkeypatch, tmp_path: Path): + # #482's third consequence: `x` must not close the parked window and leave + # the live one running. + calls = _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n", tmp_path) + _write_record(tmp_path, "RID", "@2") + launch.kill_ctl_window(tmp_path, "RID") + assert ["tmux", "kill-window", "-t", "@2"] in calls -def test_ctl_window_id_no_session_or_tmux(monkeypatch): + +def test_ctl_window_id_no_session_or_tmux(monkeypatch, tmp_path: Path): def fake(argv, **kwargs): return subprocess.CompletedProcess(argv, 1, stdout="", stderr="no session") monkeypatch.setattr(tmux_base.subprocess, "run", fake) monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") - assert launch.ctl_window_id("RID") is None + assert launch.ctl_window_id(tmp_path, "RID") is None monkeypatch.setattr(tmux_base.shutil, "which", lambda name: None) - assert launch.ctl_window_id("RID") is None # no subprocess call attempted + assert launch.ctl_window_id(tmp_path, "RID") is None # no subprocess call attempted def test_set_return_pane_argv(fake_run): @@ -370,6 +639,605 @@ def test_start_detached_returns_window_id(fake_run, tmp_path: Path): assert launch.start_resolve_detached(tmp_path, "RID") == "@7" +def _make_run(project: Path, run_id: str = "RID") -> Path: + """A run dir runs.is_run accepts — the state a resume/resolve launches over.""" + run_dir = runs.run_dir_for(project, run_id) + run_dir.mkdir(parents=True, exist_ok=True) + (run_dir / "state.json").write_text("{}", encoding="utf-8") + return run_dir + + +def test_start_detached_records_the_window_it_minted(fake_run, tmp_path: Path): + run_dir = _make_run(tmp_path) + launch.resume_detached(tmp_path, "RID") + assert (run_dir / launch._CTL_WINDOW_FILE).read_text(encoding="utf-8") == "@7" + + +def test_start_detached_records_nothing_without_a_run(fake_run, tmp_path: Path): + # A fresh `run` mints the only window carrying its run id — nothing to + # disambiguate — and the record must never conjure a directory that + # runs.is_run would then report as not a run. The explicit skip keeps this + # expected case out of the OSError swallow; this test pins the outcome. + launch.start_run_detached(tmp_path, "RID") + assert not runs.run_dir_for(tmp_path, "RID").exists() + + +def test_no_record_into_a_dir_that_is_not_a_run(fake_run, tmp_path: Path): + # The case the is_run guard actually gates (the missing-dir sibling above is + # also covered by the OSError swallow — deleting the guard leaves it green): + # a run-dir-shaped directory without state.json (pruned, partial). Here the + # write would *succeed*, so only the guard keeps the sidecar out. + run_dir = runs.run_dir_for(tmp_path, "RID") + run_dir.mkdir(parents=True) + launch.resume_detached(tmp_path, "RID") + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +def test_start_detached_survives_an_unwritable_record(fake_run, tmp_path: Path, monkeypatch): + # The window is already running by the time the record is written, so a + # failed write degrades to the name scan rather than failing the launch. + from bmad_loop import platform_util + + run_dir = _make_run(tmp_path) + (run_dir / launch._CTL_WINDOW_FILE).mkdir() # a dir, not a file + # On win32 the replace-over-a-directory denial looks like the transient + # sharing violation atomic_replace retries; skip the ~5s backoff. + monkeypatch.setattr(platform_util, "_REPLACE_ATTEMPTS", 1) + assert launch.start_resolve_detached(tmp_path, "RID") == "@7" + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_symlinked_run_dir_is_refused(fake_run, tmp_path: Path): + # follow_symlinks=False refuses a link at the FINAL component only. Swap an + # ancestor — the run dir itself — for a link to an external directory that + # holds a state.json, and runs.is_run follows it, then mkstemp/os.replace + # land the record inside the linked-to directory. Narrower than the + # final-component escape (the name written is always `ctl-window`) but the + # same shape, so the path has to be confined before the write. + outside = tmp_path / "outside" + outside.mkdir() + (outside / "state.json").write_text("{}", encoding="utf-8") # looks like a run + run_dir = runs.run_dir_for(tmp_path, "RID") + run_dir.parent.mkdir(parents=True) + run_dir.symlink_to(outside) + + # The launch still succeeds, and the lookup is not warned about: only one + # window carries the run id, so the scan answers it correctly with no record. + assert launch.resume_detached(tmp_path, "RID") == "@7" + assert not (outside / launch._CTL_WINDOW_FILE).exists() # nothing escaped + + +@pytest.mark.skipif(not launch.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only") +def test_record_write_is_anchored_against_an_ancestor_swap(fake_run, tmp_path, monkeypatch): + """The race a path check cannot close: the session re-plants the run dir as + a link *after* confinement is established. A preflight check answers about a + path and is stale the moment it returns, so the write follows the new link; + the descriptor `open_dir_confined` hands back is bound to the directory it + actually walked, so the swap renames something the write no longer consults. + + The swap is forced rather than raced with threads: hooking the helper is the + exact interleaving an attacker who wins the window achieves, and it is + deterministic. The positive control is the second assertion — the record + must actually LAND (in the real, now-renamed-aside directory), so this + cannot pass by the write simply having failed.""" + run_dir = _make_run(tmp_path) + outside = tmp_path / "outside" + outside.mkdir() + real_open = launch.open_dir_confined + + def swap_after_the_walk(project: Path, target: Path): + fd = real_open(project, target) + # attacker wins: the name now points outside, the fd still points home + target.rename(tmp_path / "moved-aside") + target.symlink_to(outside) + return fd + + monkeypatch.setattr(launch, "open_dir_confined", swap_after_the_walk) + launch.resume_detached(tmp_path, "RID") + + assert not (outside / launch._CTL_WINDOW_FILE).exists() # nothing escaped + landed = tmp_path / "moved-aside" / launch._CTL_WINDOW_FILE + assert landed.read_text(encoding="utf-8") == "@7" # and the write did happen + assert run_dir.is_symlink() # the swap really was in place for the write + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_record_falls_back_to_the_confinement_check_without_dir_fd(fake_run, tmp_path, monkeypatch): + # win32 has no *at() family to anchor against, so it keeps check-then-write. + # Exercised here from POSIX so the fallback is not left to the Windows legs + # alone: it still has to refuse an ancestor link, just with the weaker + # (racy, and documented as such) guarantee. + monkeypatch.setattr(launch, "DIR_FD_ANCHORED_WRITES", False) + outside = tmp_path / "outside" + outside.mkdir() + (outside / "state.json").write_text("{}", encoding="utf-8") + run_dir = runs.run_dir_for(tmp_path, "RID") + run_dir.parent.mkdir(parents=True) + run_dir.symlink_to(outside) + + assert launch.resume_detached(tmp_path, "RID") == "@7" + assert not (outside / launch._CTL_WINDOW_FILE).exists() + + # Positive control: the `@7` above only says the lookup fell back to the one + # listed window, which it would do whether or not the write was attempted. + # With a regular run dir the same fallback branch really does write, so the + # refusal is a refusal rather than a write that never got as far as trying. + run_dir.unlink() + _make_run(tmp_path) + assert launch.resume_detached(tmp_path, "RID") == "@7" + assert (run_dir / launch._CTL_WINDOW_FILE).read_text(encoding="utf-8") == "@7" + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_forget_refuses_a_linked_run_dir(tmp_path: Path): + """The forget path is a *delete*, so it needs no race to be redirected. + + `unlink` leaves a link at the final component alone, but the ancestors + resolve normally: a run dir standing as a link to an external directory + makes `run_dir / ctl-window` name a file over there, and dropping the hint + drops that instead. Unlike the write's escape there is no window to win — + the link can be planted whenever and simply waits for the next launch that + fails to capture a window id. + + What this pins is the *refusal*: the standing link is caught by the + confinement walk (`open_dir_confined` answers None), so deleting the whole + guard is what reddens it. The residual race — a swap landing after that walk + — is not covered here at all; `test_forget_is_anchored_against_an_ancestor_swap` + is the one that pins the anchoring.""" + project = tmp_path / "proj" + project.mkdir() + outside = tmp_path / "other-project" + outside.mkdir() + victim = outside / launch._CTL_WINDOW_FILE + victim.write_text("@99", encoding="utf-8") # another project's live record + + run_dir = runs.run_dir_for(project, "RID") + run_dir.parent.mkdir(parents=True) + run_dir.symlink_to(outside) + + launch._forget_ctl_window(project, "RID") + assert victim.read_text(encoding="utf-8") == "@99" # the neighbour survived + + # Positive control: with the link gone the removal still happens, so this + # cannot pass by _forget_ctl_window having quietly become a no-op. + run_dir.unlink() + run_dir.mkdir() + (run_dir / launch._CTL_WINDOW_FILE).write_text("@2", encoding="utf-8") + launch._forget_ctl_window(project, "RID") + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +@pytest.mark.skipif(not launch.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only") +def test_forget_is_anchored_against_an_ancestor_swap(tmp_path: Path, monkeypatch): + """The window the confinement walk above cannot close: the session re-plants + the run dir as a link *after* the walk and before the removal. A path-based + unlink resolves the new link and drops the neighbour's record; the unlink + relative to the walked descriptor names no path, so the swap renames + something it no longer consults. + + Forced by hooking the helper rather than raced with threads — the same + deterministic interleaving as the write's anchoring test.""" + project = tmp_path / "proj" + project.mkdir() + outside = tmp_path / "other-project" + outside.mkdir() + victim = outside / launch._CTL_WINDOW_FILE + victim.write_text("@99", encoding="utf-8") # another project's live record + + run_dir = runs.run_dir_for(project, "RID") + run_dir.parent.mkdir(parents=True) + run_dir.mkdir() + (run_dir / launch._CTL_WINDOW_FILE).write_text("@2", encoding="utf-8") + real_open = launch.open_dir_confined + + def swap_after_the_walk(proj: Path, target: Path): + fd = real_open(proj, target) + # attacker wins: the name now points next door, the fd still points home + target.rename(tmp_path / "moved-aside") + target.symlink_to(outside) + return fd + + monkeypatch.setattr(launch, "open_dir_confined", swap_after_the_walk) + launch._forget_ctl_window(project, "RID") + + assert victim.read_text(encoding="utf-8") == "@99" # the neighbour survived + # Positive control: the real record was still dropped, through the + # descriptor — so this cannot pass by the removal simply not happening. + assert not (tmp_path / "moved-aside" / launch._CTL_WINDOW_FILE).exists() + assert run_dir.is_symlink() # the swap really was in place for the unlink + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_forget_falls_back_to_the_confinement_check_without_dir_fd(tmp_path: Path, monkeypatch): + # The win32 branch of the same refusal, exercised from POSIX rather than + # left to the Windows legs: no *at() family there, so it check-then-deletes. + monkeypatch.setattr(launch, "DIR_FD_ANCHORED_WRITES", False) + project = tmp_path / "proj" + project.mkdir() + outside = tmp_path / "other-project" + outside.mkdir() + victim = outside / launch._CTL_WINDOW_FILE + victim.write_text("@99", encoding="utf-8") + + run_dir = runs.run_dir_for(project, "RID") + run_dir.parent.mkdir(parents=True) + run_dir.symlink_to(outside) + + launch._forget_ctl_window(project, "RID") + assert victim.read_text(encoding="utf-8") == "@99" + + # Positive control: the same branch still removes a record it can vouch for, + # so the refusal above is not this branch having quietly become a no-op. + run_dir.unlink() + run_dir.mkdir() + (run_dir / launch._CTL_WINDOW_FILE).write_text("@2", encoding="utf-8") + launch._forget_ctl_window(project, "RID") + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="tab/newline are legal POSIX name bytes") +@pytest.mark.parametrize( + "odd_name", + ["my\tproj", "my\nproj", "my\rproj", "my\vproj", "my\x85proj", "my\u2028proj"], + ids=["tab", "LF", "CR", "VT", "NEL", "LS"], +) +def test_a_delimiter_in_the_project_path_does_not_hide_its_own_window( + monkeypatch, tmp_path: Path, odd_name: str +): + """The project tag rides the same tab-delimited, line-per-window listing it + is compared against, and a resolved project path can legally hold any of + these bytes. A tab truncated the tag; every separator `splitlines()` knows + split the row. Either way the tag read back was not the tag written, so this + project's own window looked like a *neighbour's* and was discarded — and + `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.""" + project = tmp_path / odd_name + project.mkdir() + _make_run(project) + tag = runs.project_tag(project) + _ctl_listing(monkeypatch, f"@7\tresume-RID\t{tag}\n") + + assert launch.ctl_window_id(project, "RID") == "@7" + + +@pytest.mark.skipif(sys.platform == "win32", reason="separators are illegal in win32 names") +def test_a_separator_in_the_project_path_does_not_admit_a_foreign_window( + monkeypatch, tmp_path: Path +): + """The other half of the delimiter story, and the dangerous half. + + An earlier fix restored reach for these projects by *not comparing* tags + 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. + + 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 + unusable.""" + mine = tmp_path / "my\nproj" + theirs = tmp_path / "theirproj" + mine.mkdir() + theirs.mkdir() + _make_run(mine) # so `local` is True — ownership-by-run-dir would say yes + + _ctl_listing(monkeypatch, f"@9\trun-RID\t{runs.project_tag(theirs)}\n") + assert launch.ctl_window_id(mine, "RID") is None + + # Positive control: the identical row tagged for THIS project is found, so + # the None above is the tag comparison refusing, not a listing that parsed + # to nothing or a run id that never matched. + _ctl_listing(monkeypatch, f"@9\trun-RID\t{runs.project_tag(mine)}\n") + assert launch.ctl_window_id(mine, "RID") == "@9" + + +def test_a_skipped_record_forgets_the_previous_one(fake_run, tmp_path: Path): + """Skipping the record because there is no run must still drop the old one. + + `_record_ctl_window` returns early when `runs.is_run` says no, and the + rationale for that is a fresh `run`/`sweep`, where nothing shares the id yet. + But the same early return is reachable with a *superseded* window live: the + TUI reads state, shows a confirm modal, and launches from the callback, so + anything that removes `state.json` during that human-length window (an + external cleanup, a concurrent prune) lands here with a previous launch's + record still on disk. That record names a window this launch just + superseded, and `ctl_window_id` prefers a record that still resolves — so + `a` attaches to and `x` kills the parked predecessor while the orchestrator + this launch minted keeps running. #482's exact symptom. + + The listing puts the live window first on purpose: the fix has to be visible + as *the record no longer steering*, not as the record happening to agree + with first-match order.""" + tag = runs.project_tag(tmp_path) + fake_run.windows = f"@7\tresume-RID\t{tag}\n@2\trun-RID\t{tag}\n" + run_dir = _write_record(tmp_path, "RID", "@2") # a previous launch's record + assert not runs.is_run(run_dir) # premise: no state.json, so recording skips + + assert launch.resume_detached(tmp_path, "RID") == "@7" + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + assert launch.ctl_window_id(tmp_path, "RID") == "@7" # not the parked @2 + + +def test_confinement_check_refuses_a_reparse_point_ancestor(tmp_path: Path, monkeypatch): + """win32's junction, reachable from POSIX by faking the attribute. + + `is_symlink()` answers for the symlink reparse tag only, so a directory + junction — which redirects traversal identically, and needs neither + elevation nor Developer Mode to create — used to walk straight past this + check. There is no way to make a real junction on POSIX, so the win32-only + `st_file_attributes` field is what gets faked; `test_confinement_check_ + refuses_a_real_junction` is the same assertion against `mklink /J` and runs + on the Windows legs.""" + project = tmp_path / "proj" + run_dir = runs.run_dir_for(project, "RID") + run_dir.mkdir(parents=True) + real_lstat = os.lstat + + def lstat_with_a_reparse_bit(path, **kwargs): + info = real_lstat(path, **kwargs) + if Path(path) != run_dir: + return info + # A junction: the reparse bit is set, but S_ISLNK stays False — which is + # exactly why is_symlink() missed it. + return type( + "FakeStat", + (), + { + "st_mode": info.st_mode, + "st_file_attributes": stat.FILE_ATTRIBUTE_REPARSE_POINT, + }, + )() + + monkeypatch.setattr(launch.os, "lstat", lstat_with_a_reparse_bit) + assert not launch._run_dir_is_confined(project, run_dir) + # Positive control: the same dir without the bit is confined, so this cannot + # pass by the walk having become a blanket refusal. + monkeypatch.setattr(launch.os, "lstat", real_lstat) + assert launch._run_dir_is_confined(project, run_dir) + + +@pytest.mark.skipif(sys.platform != "win32", reason="junctions are win32-only") +def test_confinement_check_refuses_a_real_junction(tmp_path: Path): + # The unfaked version of the test above, on the platform that has junctions. + # `mklink /J` needs no elevation, unlike `mklink /D`, so this is the cheap + # plant a coding session can actually make. + project = tmp_path / "proj" + outside = tmp_path / "outside" + outside.mkdir() + run_dir = runs.run_dir_for(project, "RID") + run_dir.parent.mkdir(parents=True) + created = subprocess.run( + ["cmd", "/c", "mklink", "/J", str(run_dir), str(outside)], + capture_output=True, + text=True, + ) + assert created.returncode == 0, created.stderr # never silently skip the point + assert not run_dir.is_symlink() # the blindness this covers: not a symlink + assert not launch._run_dir_is_confined(project, run_dir) + + +def test_confinement_check_refuses_an_unprobeable_ancestor(tmp_path: Path, monkeypatch): + # `Path.is_symlink()` swallows OSError and answers False, so an ancestor + # that cannot be probed used to be walked past as "not a link" — the + # opposite of what the docstring promised. The probe raises now. + project = tmp_path / "proj" + run_dir = runs.run_dir_for(project, "RID") + run_dir.mkdir(parents=True) + real_lstat = os.lstat + + def lstat_denied(path, **kwargs): + if Path(path) == run_dir: + raise PermissionError("cannot probe") + return real_lstat(path, **kwargs) + + monkeypatch.setattr(launch.os, "lstat", lstat_denied) + assert not launch._run_dir_is_confined(project, run_dir) + # Positive control: the same directory, once it can be probed again, IS + # confined. Without this the refusal above is satisfied by the walk having + # become a blanket no — which is every reason a negative assertion can pass. + monkeypatch.setattr(launch.os, "lstat", real_lstat) + assert launch._run_dir_is_confined(project, run_dir) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_symlinked_record_is_replaced_not_followed(fake_run, tmp_path: Path): + # `atomic_write_text` follows a symlink under its default contract, and the + # run dir lives under the project root every coding session can write — so a + # session that plants a link here would aim this *host-side* write at any + # path the user can write, reach a workspace-confined adapter otherwise + # denies it. The write must land on the name, never on the link's target. + run_dir = _make_run(tmp_path) + outside = tmp_path / "pyproject.toml" + outside.write_text("[project]\n", encoding="utf-8") + record = run_dir / launch._CTL_WINDOW_FILE + record.symlink_to(outside) + + assert launch.resume_detached(tmp_path, "RID") == "@7" # the launch still succeeds + assert outside.read_text(encoding="utf-8") == "[project]\n" # not redirected + # Clobbered, not refused: the record self-heals into a plain file, so the + # next launch does not trip over a link left in place. + assert not record.is_symlink() + assert record.read_text(encoding="utf-8") == "@7" + + +def _fail_the_record(monkeypatch, exc: BaseException) -> None: + """Make the record write raise, whichever writer this platform records with. + + POSIX anchors the write at a directory descriptor (`atomic_write_text_at`) + and win32 falls back to the path-based `atomic_write_text`; patching both + keeps these tests about the degradation rather than about which branch ran. + + The two forget tests write a record FIRST and assert it is gone afterwards, + so a patch that reached neither writer would leave that record in place and + fail — their assertions are not satisfiable by the write simply never + happening. `test_resume_reports_a_record_that_did_not_survive` does not use + that shape: its control is its sibling + `test_resume_returns_the_id_when_the_record_survives`, which runs the same + two-window listing unpatched and gets `@7`, so the `None` here can only come + from the record failing to land.""" + + def boom(*_a, **_k): + raise exc + + monkeypatch.setattr(launch, "atomic_write_text", boom) + monkeypatch.setattr(launch, "atomic_write_text_at", boom) + + +def test_resume_reports_a_record_that_did_not_survive(fake_run, tmp_path: Path, monkeypatch): + # The window id was captured, so start_detached returns it — but the record + # did not land, which leaves ctl_window_id on the same ambiguous scan an + # uncaptured id does. One signal for both, or the rest of the degradation + # hides behind the success toast. + # + # #482's actual shape, not a bare fake: the parked `run-RID` is still listed + # in front of the live `resume-RID`, so without the record the scan answers + # the corpse (`@1`) and the degradation is real rather than notional. + fake_run.windows = "@1\trun-RID\n@7\tresume-RID\n" + _make_run(tmp_path) + + _fail_the_record(monkeypatch, OSError("read-only file system")) + assert launch.resume_detached(tmp_path, "RID") is None + + +def test_resume_returns_the_id_when_the_record_survives(fake_run, tmp_path: Path): + # The other half of the signal: over the same two-window listing, a landed + # record makes the lookup answer the live window, so the launch is reported + # plainly and the warning stays specific to real degradation. + fake_run.windows = "@1\trun-RID\n@7\tresume-RID\n" + _make_run(tmp_path) + assert launch.resume_detached(tmp_path, "RID") == "@7" + + +def test_resume_does_not_warn_when_the_scan_is_unambiguous(fake_run, tmp_path: Path, monkeypatch): + # No record, but only one window carries the run id, so the scan answers the + # right one anyway. The question is whether targeting is sound, not whether + # a file was written — warning here would cry wolf on every launch that has + # nothing to disambiguate. + fake_run.windows = "@7\tresume-RID\n" + _make_run(tmp_path) + + def boom(*_a, **_k): + raise OSError("read-only file system") + + monkeypatch.setattr(launch, "atomic_write_text", boom) + assert launch.resume_detached(tmp_path, "RID") == "@7" + + +def test_recorded_probe_degrades_when_the_listing_is_unreachable(tmp_path: Path, monkeypatch): + # The probe is observation, so it degrades rather than raising into the + # launchers — neither _do_resume nor _launch_resolve handles a + # MultiplexerError, so an uncaught one crashes the TUI after a launch that + # already succeeded. "Could not confirm" warns, matching the toast's hedge. + def boom(*_a, **_k): + raise MultiplexerError("backend server not reachable") + + monkeypatch.setattr(launch, "ctl_window_id", boom) + assert launch.ctl_window_recorded(tmp_path, "RID", "@7") is False + + +def test_resume_reports_a_record_the_listing_does_not_carry(fake_run, tmp_path: Path): + # The divergence the seam tolerates: a backend whose new_parked_window id is + # shaped differently from its list_windows window_id column. The record + # round-trips intact, so file equality would call this sound — but + # ctl_window_id rejects it against the listing and falls through to the + # first match, which is the ambiguity the warning exists for. + fake_run.windows = "@1\trun-RID\nctl:@7\tresume-RID\n" + _make_run(tmp_path) + assert launch.resume_detached(tmp_path, "RID") is None + # The record itself landed — the divergence is in the id's shape, not the write. + assert (runs.run_dir_for(tmp_path, "RID") / launch._CTL_WINDOW_FILE).read_text() == "@7" + + +def test_failed_record_forgets_the_previous_one(fake_run, tmp_path: Path, monkeypatch): + # A launch that cannot record the window it minted must not leave the + # *previous* launch's id authoritative — that id names a window this launch + # just superseded, so the honest state is no record at all. + run_dir = _make_run(tmp_path) + _write_record(tmp_path, "RID", "@2") + + _fail_the_record(monkeypatch, OSError("disk full")) + launch.resume_detached(tmp_path, "RID") + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +def test_failed_record_survives_a_non_oserror(fake_run, tmp_path: Path, monkeypatch): + """`OSError` was too narrow to keep the docstring's promise that a failed + write must not fail the launch. `atomic_write_text` resolves the path before + its own try, and below 3.13 `Path.resolve` reports a symlink loop as + `RuntimeError` — so a run dir reached through a looping link crashed the + launch of a window that is *already running*, on the 3.11/3.12 legs. + + The fault is injected rather than built from a real symlink loop on purpose: + 3.13+ resolves loops without raising, so a loop-based version would pass on + the interpreter this suite usually runs and only ever fail on the older legs + — green here, red in CI, for a guard that was never exercised. Same reasoning + as tests/test_engine.py's `test_failed_rollback_does_not_displace_the_commit_failure`. + """ + run_dir = _make_run(tmp_path) + _write_record(tmp_path, "RID", "@2") + + _fail_the_record(monkeypatch, RuntimeError("Symlink loop from '/x'")) + launch.resume_detached(tmp_path, "RID") # must not raise + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +def test_record_survives_a_raising_window_tag(fake_run, tmp_path: Path, monkeypatch): + # Record-before-tag ordering: the seam declares set_window_option + # best-effort, but a non-conforming backend raising from it must not cost + # the record — swap the two calls in start_detached and this fails. + run_dir = _make_run(tmp_path) + + def boom(self, *_a, **_k): + raise MultiplexerError("tag failed") + + monkeypatch.setattr(type(get_multiplexer()), "set_window_option", boom) + with pytest.raises(MultiplexerError): + launch.resume_detached(tmp_path, "RID") + assert (run_dir / launch._CTL_WINDOW_FILE).read_text(encoding="utf-8") == "@7" + + +def test_uncaptured_window_id_forgets_the_previous_record(monkeypatch, tmp_path: Path): + # new-window answered no id: nothing to record, and the stale record must go. + run_dir = _make_run(tmp_path) + _write_record(tmp_path, "RID", "@2") + + def fake(argv, **kwargs): + # rc 0 throughout, incl. has-session: the ctl session exists, and + # new-window succeeds but answers no id on stdout. + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + assert launch.start_resolve_detached(tmp_path, "RID") is None + assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + + +def test_record_round_trips_a_session_qualified_id(monkeypatch, tmp_path: Path): + # The re-prove is a pure string match, so any qualified form works as long + # as the mint and the window_id column agree (multiplexer's symmetry note); + # `session:@N` is the shape psmux actually emits on both sides. + _ctl_listing( + monkeypatch, + "bmad-loop-ctl:@1\trun-RID\nbmad-loop-ctl:@2\tresume-RID\n", + tmp_path, + ) + _write_record(tmp_path, "RID", "bmad-loop-ctl:@2") + assert launch.ctl_window_id(tmp_path, "RID") == "bmad-loop-ctl:@2" + + +def test_record_with_trailing_newline_still_matches(monkeypatch, tmp_path: Path): + # A newline-terminated record (hand-edited, foreign writer) must not fail + # the `recorded in matches` check and silently answer the parked corpse. + _ctl_listing(monkeypatch, "@1\trun-RID\n@2\tresume-RID\n", tmp_path) + _write_record(tmp_path, "RID", "@2\n") + assert launch.ctl_window_id(tmp_path, "RID") == "@2" + + def test_prune_ctl_windows(monkeypatch, tmp_path: Path): from bmad_loop import runs @@ -630,7 +1498,7 @@ def test_decision_pending_false_when_empty(tmp_path: Path): def test_attach_plan_prefers_ctl_when_decision_pending(monkeypatch): monkeypatch.delenv("TMUX", raising=False) - monkeypatch.setattr(launch, "ctl_window_id", lambda rid: "@2") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, rid: "@2") monkeypatch.setattr(launch, "session_exists", lambda s: True) monkeypatch.setattr(launch, "decision_pending", lambda rd: True) selected: list[str] = [] @@ -643,7 +1511,7 @@ def test_attach_plan_prefers_ctl_when_decision_pending(monkeypatch): def test_attach_plan_prefers_ctl_when_no_agent_session(monkeypatch): monkeypatch.delenv("TMUX", raising=False) - monkeypatch.setattr(launch, "ctl_window_id", lambda rid: "@2") + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, rid: "@2") monkeypatch.setattr(launch, "session_exists", lambda s: False) monkeypatch.setattr(launch, "decision_pending", lambda rd: False) monkeypatch.setattr(launch, "select_ctl_window_id", lambda w: None) @@ -654,7 +1522,7 @@ def test_attach_plan_prefers_ctl_when_no_agent_session(monkeypatch): def test_attach_plan_agent_session_when_no_decision(monkeypatch): monkeypatch.delenv("TMUX", raising=False) - monkeypatch.setattr(launch, "ctl_window_id", lambda rid: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, rid: None) monkeypatch.setattr(launch, "session_exists", lambda s: True) monkeypatch.setattr(launch, "decision_pending", lambda rd: False) assert launch.attach_plan(Path("/proj"), "RID") == ( @@ -664,7 +1532,7 @@ def test_attach_plan_agent_session_when_no_decision(monkeypatch): def test_attach_plan_none_when_nothing_to_attach(monkeypatch): - monkeypatch.setattr(launch, "ctl_window_id", lambda rid: None) + monkeypatch.setattr(launch, "ctl_window_id", lambda proj, rid: None) monkeypatch.setattr(launch, "session_exists", lambda s: False) monkeypatch.setattr(launch, "decision_pending", lambda rd: False) assert launch.attach_plan(Path("/proj"), "RID") is None