From 8cf2360e9ea3672e77707a1c5f81221a65906999 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Fri, 2 Oct 2026 20:52:36 +0200 Subject: [PATCH 01/10] fix(tui): prove a recorded ctl window by its pane pid, not its id alone A recorded control-window id is a handle, not an identity (#750). The record could readmit an untagged neighbour's window in two ways: a window id reused after a control-server restart, or a record forged by anything able to write under the project root (CWE-284, raised on #749). Measured before choosing the remedy: neither psmux 3.3.8 nor tmux 3.4 reuses window ids within one server's life (psmux's next_win_id only increments), but both restart at @1/@0 after a server restart. #{pane_pid} works as a list-windows field on both backends, so no seam change is needed. - The ctl-window record is now "\n". The pid is read at mint through the same list_windows column the lookup uses; if it cannot be read, the record keeps the id alone (best-effort, never fails the launch). - ctl_window_id admits an untagged row only when both the id and the pid match the listing. A pid-less record (an older release's) or a malformed one still breaks ties among tagged rows, and never admits an untagged row: otherwise a forger would simply write the old format. - The lookup never writes the record or the tag. Limits, stated in the docstring: a writer with mux access to the ctl server can read the pid, but that access can already set the tag directly. Pid reuse after a restart is a residual. An active-pane change fails closed. Known gaps: - An untagged window minted before this change has a pid-less record, so it is unreachable by a/x until relaunched. Tagged windows are unaffected. - After the last test-only edit, only test_tui_launch.py was re-run on Windows (112 passed). WSL ran all three touched files after it (926 passed). The rest of the Linux suite is unverified locally. Closes #750 --- CHANGELOG.md | 3 + src/bmad_loop/tui/launch.py | 148 +++++++++++++++++++------------ tests/test_tui_app.py | 8 +- tests/test_tui_launch.py | 168 +++++++++++++++++++++++++++--------- 4 files changed, 232 insertions(+), 95 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa01e0558..3a1059980 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,9 @@ breaking changes may land in a minor release. - Escalate an environment fault at the review-budget rescue gate instead of deferring the story as unconverged (DW-523). +- Prove an untagged control-session window by the pane pid recorded at launch, + not its reusable window id, so `a`/`x` never reach a neighbour's window after + an id reuse or a forged `ctl-window` record (#750). ## [0.13.1] — 2026-10-01 diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index a23ed8203..beef3e42a 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -24,6 +24,7 @@ from .. import runs from ..adapters.multiplexer import ( MultiplexerError, + TerminalMultiplexer, get_multiplexer, mux_usable, ) @@ -75,12 +76,25 @@ def session_exists(session: str) -> bool: # 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. +# session-qualified `bmad-loop-ctl:@7`) plus, on its own line, the window's pane +# pid (#750), and anything longer is already not one. _MAX_RECORD_BYTES = 256 +def _parse_ctl_record(record: str | None) -> tuple[str | None, str | None]: + """Split a record into `(window id, pane pid)`. The pid line is optional — a + record written before #750 carries the id alone — and anything that is not + plain ASCII digits reads as absent rather than as a pid.""" + if record is None: + return None, None + win_id, _, pid = record.partition("\n") + pid = pid.strip() + return win_id.strip() or None, pid if pid.isascii() and pid.isdigit() else None + + 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 + """The raw record the run's last launch wrote (window id, optionally a pane + pid on a second line — see _parse_ctl_record), 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 @@ -251,9 +265,11 @@ def _run_dir_is_confined(project: Path, run_dir: Path) -> bool: return True -def _record_ctl_window(project: Path, run_id: str, win_id: str) -> None: +def _record_ctl_window(project: Path, run_id: str, win_id: str, pane_pid: str | None) -> None: """Record the window a launch just minted, so ctl_window_id can prefer it - over an older window sharing the run id. + over an older window sharing the run id. The payload is ``, plus + `\\n` when the mint's pane pid could be read — the pid is what lets + ctl_window_id admit the window even if its tag write fails (#750). 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 @@ -334,19 +350,20 @@ def _record_ctl_window(project: Path, run_id: str, win_id: str) -> None: if not runs.is_run(run_dir): _forget_ctl_window(project, run_id) return + payload = f"{win_id}\n{pane_pid}" if pane_pid else win_id 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) + atomic_write_text_at(dir_fd, _CTL_WINDOW_FILE, payload) 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) + atomic_write_text(run_dir / _CTL_WINDOW_FILE, payload, follow_symlinks=False) except Exception: _forget_ctl_window(project, run_id) @@ -387,16 +404,38 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: run dir this one stopped trusting, because the pruning consumer of that shape is #419's and the fix is partitioned there. - An untagged row is admitted only when the record names that exact window. - Holding the run dir was the earlier gate, and a run dir is a coincidence - rather than a claim: `--run-id` is caller-supplied and deterministic, so two - projects scripting the same id both hold one, and each then admitted the - *other's* untagged window — machine-wide on tmux, where the control session - carries a fixed name (#531). The record is the opposite kind of fact: this - project wrote it, about the window its own launch minted. It survives the - failure this fallback exists for because start_detached records *before* it - tags, so a window whose (best-effort) set_window_option never landed still - has one. + An untagged row is admitted only when the record names that exact window + AND the pane pid recorded at mint matches the pid the listing shows for it + (#750). Holding the run dir was the earlier gate, and a run dir is a + coincidence rather than a claim: `--run-id` is caller-supplied and + deterministic, so two projects scripting the same id both hold one, and each + then admitted the *other's* untagged window — machine-wide on tmux, where the + control session carries a fixed name (#531). The window id alone was the + next gate, and an id is a reusable handle: tmux 3.4 and psmux 3.3.8 both + restart at `@0`/`@1` after a server restart, so a neighbour's untagged + window could inherit the id this project recorded — and the record sits + under the project root, so a session could equally write any id there. The + pid is the process the mint actually started, and start_detached records it + *before* it tags, so a window whose (best-effort) set_window_option never + landed is still reachable. A record without a pid (written before #750, or + by a mint whose pid read failed) still breaks ties among tagged rows but + never admits an untagged one. + + The honest limit: this proves the record was written by something that saw + the window's pid, not that this project minted it. Anything with access to + the control session's multiplexer can list `pane_pid` and write a matching + record — and that same access lets it set the PROJECT_OPTION tag directly, + so the pid raises the bar to exactly the tag's and no further. What it does + close is the id-reuse case and a forged record that merely guesses an id. + + Two residuals, both stated rather than handled. The pid is a reusable + handle too: after a server restart, an untagged neighbour under this run's + name could inherit both the recorded id AND, from the OS, the recorded pid — + a conjunction of two recycles that closing would need a process creation + identity the seam does not carry. And `pane_pid` answers the window's + *active* pane, so a window someone splits by hand and re-focuses fails the + compare; that fails closed (the untagged fallback answers None, exactly as a + recordless window does) and bmad-loop itself never splits the ctl window. What the proof costs is reach for a window minted before any record exists — a fresh `run`/`sweep`, where _record_ctl_window deliberately skips because @@ -416,36 +455,18 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: one, and the tag is the stronger proof of the two, so untagged is consulted only when nothing carries this project's tag. - One residual survives, and it is a conjunction rather than a case: a backend - that reuses a freed window id (a supported divergence — see the id-reuse row - in the tests) hands the neighbour the id this project recorded, while both - projects script the same run id AND the neighbour's own tag write failed AND - the window this record named is already gone. Name and id then both match and - the neighbour is admitted. Closing it needs a channel that says *this window - is mine* rather than *an id I once minted*, and the only one available is the - tag — which this function must read, never write: re-tagging on read is - claiming, not proving, and would hand a neighbour's window this project's - tag. So it is left open, deliberately and visibly. Wherever `runs.is_run` still - holds, every condition in that conjunction was already satisfied by the gate - this replaces — which admitted the neighbour on the run-id collision alone, - with no id reuse and no dead window required — so over those states this is a - strict narrowing. It is not a narrowing everywhere, and the exception is the - residual reached from the other side: _read_ctl_window asks nothing about the - run dir, so an untagged row named by a readable record whose run `runs.is_run` - now rejects (a partial prune is one route there, not the only one) is admitted - here and would have been refused by the old gate — and if that recorded id has - since been reused by an untagged neighbour under this run's name, the row - admitted is the neighbour's. That is the whole of what this gate trades away. - Narrowing it is a mechanism question - — an identity channel written at mint time and read here — and this function - is the wrong place to decide it.""" + Read-only throughout: this never writes the record and never touches the + tag — re-tagging on read is claiming, not proving, and would hand a + neighbour's window this project's tag.""" if not mux_available(): return None mine = runs.accepted_tags(project) tagged: list[str] = [] untagged: list[str] = [] + # pane_pid last: the base's bounded split leaves any stray tab in the final + # field, and a pid that does not read as digits simply fails the compare. rows = get_multiplexer().list_windows( - ctl_session(project), ["window_id", "window_name", runs.PROJECT_OPTION] + ctl_session(project), ["window_id", "window_name", runs.PROJECT_OPTION, "pane_pid"] ) # Below the listing, above the loop. The loop needs it — it is what admits an # untagged row — but listing and record are two reads of a state a concurrent @@ -458,13 +479,13 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: # carry yet — so it fails the re-prove and the answer falls back to a match # that was at least live when the listing was taken. `rows` is materialized # (list_windows returns a list), so the loop pays nothing for the move. - recorded = _read_ctl_window(project, run_id) - for win_id, name, tag in rows: + recorded, recorded_pid = _parse_ctl_record(_read_ctl_window(project, run_id)) + for win_id, name, tag, pane_pid 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 CAN produce an - # empty *tag* — it fills trailing fields — which is exactly the untagged - # case below; window_id stays field 0 of 3.) + # empty *tag* or pid — it fills trailing fields — which is the untagged + # case below; window_id stays field 0 of 4.) if not win_id: continue # The whole run id, not a suffix of the name: RUN_ID_RE admits `-`, so @@ -488,10 +509,15 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: # unreachable by `a` and `x`, which resolve through here. if tag in mine: tagged.append(win_id) - elif not tag and win_id == recorded: - # untagged, but this project's own launch recorded this window — - # proof of the mint, not of the tag, so it only counts if nothing - # is tagged + elif ( + not tag + and win_id == recorded + and recorded_pid is not None + and pane_pid.strip() == recorded_pid + ): + # untagged, but the record names this window and the pid its mint + # read — proof of the mint, not of the tag, so it only counts if + # nothing is tagged. A pid-less record never gets here (#750). untagged.append(win_id) matches = tagged or untagged if not matches: @@ -499,10 +525,10 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: # Membership in `matches`, not mere presence in the listing: it re-checks the # name and the project-scoping predicates, so a record whose id is absent from # the scoped matches — killed, pruned, renamed onto another run, or naming a - # row this project cannot claim — is not replayed. Not a proof of identity: a - # reused id under this run's name still passes, which is the residual the - # docstring names. But it is what turns a stale id from a replayed target - # into a fallthrough. + # row this project cannot claim — is not replayed. Among tagged rows this is a + # tie-break only (the tag already proved ownership), so a pid-less record + # still counts here. It is what turns a stale id from a replayed target into + # a fallthrough. return recorded if recorded in matches else matches[0] @@ -880,6 +906,22 @@ def cli_argv(*tail: str) -> list[str]: return [sys.executable, "-m", "bmad_loop.cli", *tail] +def _minted_pane_pid(mux: TerminalMultiplexer, ctl: str, win_id: str) -> str | None: + """The pane pid of the window just minted, for the ctl-window record (#750), + or None when the listing does not show it or the pid is not plain digits. + Best-effort: the window is already running, so a failed read only costs the + untagged fallback in ctl_window_id, never the launch.""" + try: + rows = mux.list_windows(ctl, ["window_id", "pane_pid"]) + except MultiplexerError: + return None + for row_id, pid in rows: + pid = pid.strip() + if row_id == win_id and pid.isascii() and pid.isdigit(): + return pid + return None + + def start_detached(project: Path, argv_tail: list[str], run_id: str, kind: str) -> str | None: """Run a bmad-loop command in a new window of the control session. @@ -933,7 +975,7 @@ def start_detached(project: Path, argv_tail: list[str], run_id: str, kind: str) # 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) + _record_ctl_window(project, run_id, win_id, _minted_pane_pid(mux, ctl, 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)) diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index a842a766e..53a173d79 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -3896,11 +3896,15 @@ async def test_attach_uses_the_recorded_ctl_window(project_tree, monkeypatch): rid = "20260611-100000-aaaa" run_dir = make_run(project_tree.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") + # id + the pane pid its mint read: an untagged row needs both (#750) + (run_dir / launch._CTL_WINDOW_FILE).write_text("@2\n4242", 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 "" + # rows as `window_id, window_name, tag, pane_pid` — the lookup's fields + out = ( + f"@1\trun-{rid}\t\t11\n@2\tresume-{rid}\t\t4242\n" if argv[1] == "list-windows" else "" + ) return _subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") monkeypatch.setattr(tmux_base.subprocess, "run", fake) diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index df68076a2..bad52d493 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -11,6 +11,7 @@ import json import os +import re import shlex import signal import stat @@ -31,6 +32,26 @@ # tmux — so pin tmux by name (a no-op on a stock POSIX box). pytestmark = pytest.mark.usefixtures("force_tmux_backend") +# Scripted listing rows are written positionally in this order; fakes project +# them onto whatever fields the caller's `-F` format (or `fields`) asks for, so a +# lookup asking for `pane_pid` sees the pid and the mint's own `window_id, +# pane_pid` read sees it too, rather than the name in the pid slot. +_ROW_FIELDS = ["window_id", "window_name", runs.PROJECT_OPTION, "pane_pid"] + + +def _project_row(row: tuple[str, ...] | list[str], fields: list[str]) -> tuple[str, ...]: + cols = list(row) + [""] * (len(_ROW_FIELDS) - len(row)) + return tuple(cols[_ROW_FIELDS.index(f)] for f in fields) + + +def _project_listing(rows: str, argv: list[str]) -> str: + """`rows` reshaped to the fields a `list-windows -F` argv requests.""" + fields = re.findall(r"#\{([^}]*)\}", argv[argv.index("-F") + 1]) + return "".join( + "\t".join(_project_row(line.split("\t", len(_ROW_FIELDS) - 1), fields)) + "\n" + for line in rows.splitlines() + ) + class FakeRun: """Records argv; scripts the returncode of `tmux has-session` and the rows @@ -50,7 +71,7 @@ def __call__(self, argv, **kwargs): if argv[1] == "new-window": out = "@7\n" elif argv[1] == "list-windows": - out = self.windows + out = _project_listing(self.windows, argv) return subprocess.CompletedProcess(argv, rc, stdout=out, stderr="") def by_verb(self, verb: str) -> list[list[str]]: @@ -80,13 +101,14 @@ def test_start_run_detached_argv(fake_run, tmp_path: Path): nw0 = fake_run.by_verb("new-window")[0] assert nw0[nw0.index("-F") + 1] == "#{window_id}" - # control session was missing: has-session, new-session, new-window, then - # the project tag is stamped on the new window so cross-project cleanup - # never closes it + # control session was missing: has-session, new-session, new-window, the + # pane-pid read for the ctl-window record (#750), then the project tag is + # stamped on the new window so cross-project cleanup never closes it assert [c[1] for c in fake_run.calls] == [ "has-session", "new-session", "new-window", + "list-windows", "set-option", ] from bmad_loop import runs @@ -208,13 +230,15 @@ def test_existing_ctl_session_reused(monkeypatch, tmp_path: Path): monkeypatch.setattr(tmux_base.subprocess, "run", fake) monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") launch.resume_detached(tmp_path, "RID") - # No new-session: the ctl session already answered has-session. The trailing - # list-windows is resume's own check that the lookup now names the window it + # No new-session: the ctl session already answered has-session. The first + # list-windows reads the minted pane's pid for the record (#750); the + # trailing one 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", + "list-windows", "set-option", "list-windows", ] @@ -303,7 +327,7 @@ def _ctl_listing(monkeypatch, rows: str, project: Path | None = None) -> list[li def fake(argv, **kwargs): calls.append(list(argv)) - out = rows if argv[1] == "list-windows" else "" + out = _project_listing(rows, argv) if argv[1] == "list-windows" else "" return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") monkeypatch.setattr(tmux_base.subprocess, "run", fake) @@ -311,12 +335,13 @@ def fake(argv, **kwargs): 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.""" +def _write_record(project: Path, run_id: str, win_id: str, pane_pid: str | None = None) -> Path: + """Stand in for a launch having minted `win_id` (with `pane_pid`, when the + mint could read one) for this run. No pid is the pre-#750 record shape.""" 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") + record.write_text(f"{win_id}\n{pane_pid}" if pane_pid else win_id, encoding="utf-8") return record @@ -345,7 +370,8 @@ def test_ctl_window_id_requires_the_whole_run_id(monkeypatch, tmp_path: Path): 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. + # id names the window we actually created. A pid-less (pre-#750) record on + # purpose: among tagged rows the record is only a tie-break, and that stays. _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" @@ -409,11 +435,37 @@ def test_ctl_window_id_admits_an_untagged_window_the_record_names(monkeypatch, t # 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. start_detached records BEFORE it tags, so the - # record still names the window — and a record is a claim this project - # wrote, where a run dir is only a coincidence of the id. - _ctl_listing(monkeypatch, "@4\tresume-RID\t\n", tmp_path) + # record still names the window and the pane pid its mint read (#750). + _ctl_listing(monkeypatch, "@4\tresume-RID\t\t111\n", tmp_path) _make_run(tmp_path) # _record_ctl_window refuses to write without one + _write_record(tmp_path, "RID", "@4", "111") + assert launch.ctl_window_id(tmp_path, "RID") == "@4" + + +def test_ctl_window_id_refuses_an_untagged_window_on_a_pidless_record(monkeypatch, tmp_path: Path): + # A record written before #750 (or by a mint whose pid read failed) carries + # the id alone, which is exactly the reusable handle — never enough to admit + # an untagged row. The row's own pid is present, so the refusal is about the + # record, not an empty listing column. + _ctl_listing(monkeypatch, "@4\tresume-RID\t\t111\n", tmp_path) + _make_run(tmp_path) _write_record(tmp_path, "RID", "@4") + assert launch.ctl_window_id(tmp_path, "RID") is None + + +@pytest.mark.parametrize( + "pid_line", ["abc", "-111", "١١١", " "], ids=["word", "sign", "arabic", "blank"] +) +def test_ctl_window_id_treats_a_malformed_pid_as_pidless(monkeypatch, tmp_path: Path, pid_line): + # A pid line that is not plain ASCII digits reads as no pid: it can still + # break a tie among tagged rows, but never admits an untagged one — even + # when the listing's pid column carries the very same malformed text. + _ctl_listing(monkeypatch, f"@4\tresume-RID\t\t{pid_line}\n", tmp_path) + _make_run(tmp_path) + _write_record(tmp_path, "RID", "@4", pid_line) + assert launch.ctl_window_id(tmp_path, "RID") is None + # Tie-break among tagged rows still honours the id. + _ctl_listing(monkeypatch, "@1\trun-RID\n@4\tresume-RID\n", tmp_path) assert launch.ctl_window_id(tmp_path, "RID") == "@4" @@ -456,10 +508,12 @@ def test_ctl_window_id_refuses_untagged_windows_the_record_does_not_name( ): # A record that resolves to nothing must not license the *other* untagged # rows: drop the per-row equality and the bucket fills by listing order, so - # `a` and `x` land on whatever sorted first. - _ctl_listing(monkeypatch, "@1\trun-RID\t\n@2\tresume-RID\t\n", tmp_path) + # `a` and `x` land on whatever sorted first. The recorded pid matches @1's + # on purpose, so the pid gate cannot mask a dropped id comparison: the id + # and the pid must both match the same row. + _ctl_listing(monkeypatch, "@1\trun-RID\t\t111\n@2\tresume-RID\t\t222\n", tmp_path) _make_run(tmp_path) - _write_record(tmp_path, "RID", "@9") # killed, pruned, or never in this listing + _write_record(tmp_path, "RID", "@9", "111") # killed, pruned, or never listed assert launch.ctl_window_id(tmp_path, "RID") is None @@ -475,8 +529,8 @@ def test_ctl_window_id_refuses_an_untagged_neighbour_on_a_run_id_collision( theirs.mkdir() _make_run(mine) # the collision: both projects hold a run dir for RID _make_run(theirs) - _write_record(theirs, "RID", "@4") # theirs minted it; its tag write failed - _ctl_listing(monkeypatch, "@4\tresume-RID\t\n") + _write_record(theirs, "RID", "@4", "111") # theirs minted it; its tag write failed + _ctl_listing(monkeypatch, "@4\tresume-RID\t\t111\n") assert launch.ctl_window_id(mine, "RID") is None @@ -486,24 +540,19 @@ def test_ctl_window_id_refuses_an_untagged_neighbour_on_a_run_id_collision( assert launch.ctl_window_id(theirs, "RID") == "@4" -def test_ctl_window_id_admits_a_record_naming_a_window_it_never_minted(monkeypatch, tmp_path: Path): - # Characterization (#750), not an endorsement: the record is a claim, and it - # sits under the project root every coding session can write (see - # _read_ctl_window), so its content proves the mint only as far as it is - # unforgeable — which it is not. A record naming an untagged window this - # project never minted is admitted here, and `x` resolves through here. - # - # Not a regression, which is the whole reason it is pinned rather than - # fixed: the gate this replaced was `runs.is_run(run_dir_for(...))`, and - # anything that can write the record can equally mint the run dir — which - # admitted EVERY untagged row under the name, with no id to guess. Closing - # it needs an identity channel the session does not own (the window's pane - # pid, recorded at mint and re-proven here), so this test is the state that - # fix has to change. - _ctl_listing(monkeypatch, "@4\tresume-RID\t\n") # untagged, and not ours +def test_ctl_window_id_refuses_a_record_naming_a_window_it_never_minted( + monkeypatch, tmp_path: Path +): + # #750: the record sits under the project root every coding session can + # write (see _read_ctl_window), so an id in it is a claim anyone can make — + # and window ids are reused after a server restart. A record naming an + # untagged window this project never minted carries a pid that window does + # not have, and `x` resolves through here, so it must answer None. + # Ablate the pid comparison in ctl_window_id and this answers "@4". + _ctl_listing(monkeypatch, "@4\tresume-RID\t\t222\n") # untagged, and not ours _make_run(tmp_path) - _write_record(tmp_path, "RID", "@4") - assert launch.ctl_window_id(tmp_path, "RID") == "@4" + _write_record(tmp_path, "RID", "@4", "111") + 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): @@ -512,8 +561,8 @@ def test_ctl_window_id_prefers_a_tagged_window_over_an_untagged_one(monkeypatch, # this project's correctly tagged one on index — and for `x` that # closes the wrong window. The tag is the stronger of the two proofs, so it # wins. - _ctl_listing(monkeypatch, "@1\trun-RID\t\n@2\trun-RID\n", tmp_path) - _write_record(tmp_path, "RID", "@1") # recorded: the untagged row is otherwise admitted + _ctl_listing(monkeypatch, "@1\trun-RID\t\t111\n@2\trun-RID\n", tmp_path) + _write_record(tmp_path, "RID", "@1", "111") # recorded: the untagged row is otherwise admitted assert launch.ctl_window_id(tmp_path, "RID") == "@2" @@ -758,6 +807,7 @@ class _NamespacedStub: def __init__(self): self.created = [] self.parked = [] + self.listed = [] def has_registry_namespace(self): return True @@ -772,6 +822,10 @@ def new_parked_window(self, session, name, cwd, argv, return_opt): self.parked.append((session, name)) return "@7" + def list_windows(self, session, fields): + self.listed.append(session) # the mint's pane-pid read (#750) + return [] + def set_window_option(self, window, option, value): pass @@ -784,6 +838,7 @@ def set_window_option(self, window, option, value): assert expected.startswith(runs.CTL_SESSION + "-") assert stub.created == [expected] assert stub.parked == [(expected, "run-RID")] + assert stub.listed == [expected] @pytest.mark.parametrize( @@ -821,8 +876,39 @@ def _make_run(project: Path, run_id: str = "RID") -> Path: def test_start_detached_records_the_window_it_minted(fake_run, tmp_path: Path): + # The id and, on its own line, the pane pid the mint's listing shows for it + # (#750) — the pid is what later admits the window if its tag never lands. + fake_run.windows = "@3\trun-RID\t\t999\n@7\tresume-RID\t\t4242\n" 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\n4242" + # And the record it wrote admits the still-untagged window on lookup. + assert launch.ctl_window_id(tmp_path, "RID") == "@7" + + +@pytest.mark.parametrize( + "windows", ["@3\tresume-RID\t\t999\n", "@7\tresume-RID\t\tnope\n"], ids=["absent", "non-digit"] +) +def test_start_detached_records_the_id_alone_without_a_readable_pid( + fake_run, tmp_path: Path, windows: str +): + # Best-effort: a listing that does not show the minted window, or shows a + # pid that is not digits, costs the pid line — never the record or launch. + fake_run.windows = windows + run_dir = _make_run(tmp_path) + launch.start_resolve_detached(tmp_path, "RID") + assert (run_dir / launch._CTL_WINDOW_FILE).read_text(encoding="utf-8") == "@7" + + +def test_start_detached_survives_a_raising_pid_read(fake_run, tmp_path: Path, monkeypatch): + # A backend that raises from the (best-effort) listing still gets its + # window recorded, id alone, and the launch still returns the id. + def boom(*_a, **_k): + raise MultiplexerError("listing unreachable") + + monkeypatch.setattr(type(get_multiplexer()), "list_windows", boom) + run_dir = _make_run(tmp_path) + assert launch.start_resolve_detached(tmp_path, "RID") == "@7" assert (run_dir / launch._CTL_WINDOW_FILE).read_text(encoding="utf-8") == "@7" @@ -1240,13 +1326,15 @@ def test_symlinked_record_is_replaced_not_followed(fake_run, tmp_path: Path): outside.write_text("[project]\n", encoding="utf-8") record = run_dir / launch._CTL_WINDOW_FILE record.symlink_to(outside) + # Untagged, so the lookup below admits @7 only through the record's pid (#750). + fake_run.windows = "@7\tresume-RID\t\t4242\n" 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" + assert record.read_text(encoding="utf-8") == "@7\n4242" def _fail_the_record(monkeypatch, exc: BaseException) -> None: @@ -1836,7 +1924,7 @@ def current_window_id(self): def list_windows(self, session, fields): self.sessions.append(session) - return list(self._rows) + return [_project_row(row, fields) for row in self._rows] def list_window_ids(self, session): self.sessions.append(session) From 78a71935497caefdfc8bb07a88aeabb6330da0d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Sat, 3 Oct 2026 06:08:28 +0200 Subject: [PATCH 02/10] fix(tui): keep the ctl-window record readable by older releases The previous commit wrote "\n" into ctl-window. A pre-change release reads that whole payload as the window id, matches no row, and falls back to the first tagged window, so with a parked run-RID beside a live resume-RID it could attach to or stop the stale one (the #482 failure). ctl-window is again the window id alone, byte-identical to the old format. The pane pid moves to a sibling file, ctl-window-pid, written by the same atomic writer and read with the same hardening (O_NOFOLLOW, O_NONBLOCK, S_ISREG on the descriptor, size cap). - Writes go pid first, record second; lookup reads record first, pid second. A crash between the writes, or a lookup racing a relaunch, leaves a newer pid beside an older id, which no row satisfies, so it fails closed. - Admission is unchanged: an untagged row needs the id and the pid to match; a missing or malformed pid never admits it; tagged tie-break uses the id. - Forgetting a ctl window removes both files. Known gaps: - Two concurrent launches of the same run can interleave into one launch's id beside the other's pid. That fails closed (no untagged admission, tagged rows unaffected, the next launch repairs it) and is documented rather than locked. - Tests were updated but not run locally; CI verifies them. Refs #750 --- CHANGELOG.md | 4 +- src/bmad_loop/tui/launch.py | 117 +++++++++++++++++++++++++----------- tests/test_tui_app.py | 6 +- tests/test_tui_launch.py | 64 +++++++++++++++++--- 4 files changed, 145 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a1059980..78bc72013 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,9 @@ breaking changes may land in a minor release. deferring the story as unconverged (DW-523). - Prove an untagged control-session window by the pane pid recorded at launch, not its reusable window id, so `a`/`x` never reach a neighbour's window after - an id reuse or a forged `ctl-window` record (#750). + an id reuse or a forged `ctl-window` record. The pid goes to a sibling + `ctl-window-pid` file, so the record stays the id alone that older releases + read (#750). ## [0.13.1] — 2026-10-01 diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index beef3e42a..97ad7d7c3 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -74,27 +74,33 @@ def session_exists(session: str) -> bool: # 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`) plus, on its own line, the window's pane -# pid (#750), and anything longer is already not one. +# Its sibling: the pane pid of that same window, read at mint (#750). A file of +# its own rather than a second line in the record, because the record's format +# is a compatibility contract: a release before #750 reads `ctl-window` whole as +# the id, so an `@7` + pid payload would match no row there and its tie-break +# would fall back to the first window, the parked corpse (#482). Written BEFORE +# the record and read AFTER it, so no interleaving pairs a newer id with an +# older pid — see _record_ctl_window. +_CTL_PID_FILE = "ctl-window-pid" + + +# Generous ceiling on either hint: the record is a window id (`@7`, or a +# session-qualified `bmad-loop-ctl:@7`) and its sibling a decimal pid, and +# anything longer is already not one. _MAX_RECORD_BYTES = 256 -def _parse_ctl_record(record: str | None) -> tuple[str | None, str | None]: - """Split a record into `(window id, pane pid)`. The pid line is optional — a - record written before #750 carries the id alone — and anything that is not - plain ASCII digits reads as absent rather than as a pid.""" - if record is None: - return None, None - win_id, _, pid = record.partition("\n") - pid = pid.strip() - return win_id.strip() or None, pid if pid.isascii() and pid.isdigit() else None +def _read_ctl_pid(project: Path, run_id: str) -> str | None: + """The pane pid the run's last launch recorded, or None when there is none — + including a file that is not plain ASCII digits, which reads as absent + rather than as a pid. The same hardened read as the record.""" + pid = _read_ctl_window(project, run_id, _CTL_PID_FILE) + return pid if pid is not None and pid.isascii() and pid.isdigit() else None -def _read_ctl_window(project: Path, run_id: str) -> str | None: - """The raw record the run's last launch wrote (window id, optionally a pane - pid on a second line — see _parse_ctl_record), or None when there is +def _read_ctl_window(project: Path, run_id: str, name: str = _CTL_WINDOW_FILE) -> str | None: + """The window id recorded by the run's last launch (or, with `name`, the + content of its pid sibling — see _read_ctl_pid), 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 @@ -124,7 +130,7 @@ def _read_ctl_window(project: Path, run_id: str) -> str | None: 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 + record = runs.run_dir_for(project, run_id) / name 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: @@ -146,7 +152,7 @@ def _read_ctl_window(project: Path, run_id: str) -> str | 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 + """Drop the record and its pid sibling, record first. 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. @@ -179,19 +185,27 @@ def _forget_ctl_window(project: Path, run_id: str) -> None: 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 + for name in (_CTL_WINDOW_FILE, _CTL_PID_FILE): + _unlink_at(dir_fd, name) 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) + for name in (_CTL_WINDOW_FILE, _CTL_PID_FILE): + (run_dir / name).unlink(missing_ok=True) except OSError: pass # a removal we cannot force — see the ceiling +def _unlink_at(dir_fd: int, name: str) -> None: + """`unlink` relative to an anchored directory descriptor; missing_ok by hand.""" + try: + os.unlink(name, dir_fd=dir_fd) + except FileNotFoundError: + pass + + 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 @@ -267,9 +281,29 @@ def _run_dir_is_confined(project: Path, run_dir: Path) -> bool: def _record_ctl_window(project: Path, run_id: str, win_id: str, pane_pid: str | None) -> None: """Record the window a launch just minted, so ctl_window_id can prefer it - over an older window sharing the run id. The payload is ``, plus - `\\n` when the mint's pane pid could be read — the pid is what lets - ctl_window_id admit the window even if its tag write fails (#750). + over an older window sharing the run id. The record holds `` alone, + byte-identical to what releases before #750 read; the mint's pane pid goes + to the `_CTL_PID_FILE` sibling, and is what lets ctl_window_id admit the + window even if its tag write fails (#750). + + Two files are never written as one, so the order is the consistency rule: + pid first, record second, and ctl_window_id reads them the other way round. + A crash or failure between the two writes leaves a newer pid beside the + older id — the old window's pid is not that one, so the pair admits nothing + untagged and the lookup fails closed. A lookup racing a relaunch sees either + a matching pair or that same unsatisfiable one, never a newer id with an + older pid: it reads the pid after the record, and the pid was written + before it. A mint whose pid could not be read removes the sibling, so an + older pid never stands beside the newer id. + + What the order does not serialize is two launches for the same run racing + each other: pid(A), pid(B), id(B), id(A) leaves A's id beside B's pid with + both writes succeeding. That pair also admits nothing untagged, so it fails + closed exactly like a crash between the writes, and the next launch repairs + it; a tagged window is unaffected. A single file kept the pair coherent by + construction, but older releases read that file whole as the id, so the + trade is deliberate. A cross-process lock per run would close it, and is + not worth taking on the launch path for an untagged-only, fail-closed race. 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 @@ -350,20 +384,29 @@ def _record_ctl_window(project: Path, run_id: str, win_id: str, pane_pid: str | if not runs.is_run(run_dir): _forget_ctl_window(project, run_id) return - payload = f"{win_id}\n{pane_pid}" if pane_pid else win_id 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, payload) + # pid first, so a failure between the two leaves a newer pid + # beside the older id — a pair no listed window can satisfy + if pane_pid: + atomic_write_text_at(dir_fd, _CTL_PID_FILE, pane_pid) + else: + _unlink_at(dir_fd, _CTL_PID_FILE) + 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, payload, follow_symlinks=False) + if pane_pid: + atomic_write_text(run_dir / _CTL_PID_FILE, pane_pid, follow_symlinks=False) + else: + (run_dir / _CTL_PID_FILE).unlink(missing_ok=True) + atomic_write_text(run_dir / _CTL_WINDOW_FILE, win_id, follow_symlinks=False) except Exception: _forget_ctl_window(project, run_id) @@ -417,9 +460,11 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: under the project root, so a session could equally write any id there. The pid is the process the mint actually started, and start_detached records it *before* it tags, so a window whose (best-effort) set_window_option never - landed is still reachable. A record without a pid (written before #750, or - by a mint whose pid read failed) still breaks ties among tagged rows but - never admits an untagged one. + landed is still reachable. The pid lives in a sibling file, not in the + record, so the record stays the id alone that releases before #750 read. A + record with no readable pid beside it (written before #750, by a mint whose + pid read failed, or a malformed sibling) still breaks ties among tagged rows + but never admits an untagged one. The honest limit: this proves the record was written by something that saw the window's pid, not that this project minted it. Anything with access to @@ -478,8 +523,12 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: # leaves a record newer than the listing, naming a window the listing does not # carry yet — so it fails the re-prove and the answer falls back to a match # that was at least live when the listing was taken. `rows` is materialized - # (list_windows returns a list), so the loop pays nothing for the move. - recorded, recorded_pid = _parse_ctl_record(_read_ctl_window(project, run_id)) + # (list_windows returns a list), so the loop pays nothing for the move. The + # pid sibling is read after the record, the reverse of the write order, so a + # relaunch in between yields a matching pair or an unsatisfiable one, never a + # newer id beside an older pid (see _record_ctl_window). + recorded = _read_ctl_window(project, run_id) + recorded_pid = _read_ctl_pid(project, run_id) for win_id, name, tag, pane_pid 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 @@ -517,7 +566,7 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: ): # untagged, but the record names this window and the pid its mint # read — proof of the mint, not of the tag, so it only counts if - # nothing is tagged. A pid-less record never gets here (#750). + # nothing is tagged. A record with no pid never gets here (#750). untagged.append(win_id) matches = tagged or untagged if not matches: diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index 53a173d79..d803f86a4 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -3896,8 +3896,10 @@ async def test_attach_uses_the_recorded_ctl_window(project_tree, monkeypatch): rid = "20260611-100000-aaaa" run_dir = make_run(project_tree.project, rid, run_type="sweep", alive=True) Journal(run_dir).append("decision-pending", dw_id="DW-7", question="q?") - # id + the pane pid its mint read: an untagged row needs both (#750) - (run_dir / launch._CTL_WINDOW_FILE).write_text("@2\n4242", encoding="utf-8") + # the id + the pane pid its mint read, in the sibling: an untagged row + # needs both (#750) + (run_dir / launch._CTL_WINDOW_FILE).write_text("@2", encoding="utf-8") + (run_dir / launch._CTL_PID_FILE).write_text("4242", encoding="utf-8") selected: list[str] = [] def fake(argv, **kwargs): diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index bad52d493..d13935031 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -337,11 +337,13 @@ def fake(argv, **kwargs): def _write_record(project: Path, run_id: str, win_id: str, pane_pid: str | None = None) -> Path: """Stand in for a launch having minted `win_id` (with `pane_pid`, when the - mint could read one) for this run. No pid is the pre-#750 record shape.""" + mint could read one) for this run. No pid sibling is the pre-#750 shape.""" 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(f"{win_id}\n{pane_pid}" if pane_pid else win_id, encoding="utf-8") + record.write_text(win_id, encoding="utf-8") + if pane_pid is not None: + (run_dir / launch._CTL_PID_FILE).write_text(pane_pid, encoding="utf-8") return record @@ -457,7 +459,7 @@ def test_ctl_window_id_refuses_an_untagged_window_on_a_pidless_record(monkeypatc "pid_line", ["abc", "-111", "١١١", " "], ids=["word", "sign", "arabic", "blank"] ) def test_ctl_window_id_treats_a_malformed_pid_as_pidless(monkeypatch, tmp_path: Path, pid_line): - # A pid line that is not plain ASCII digits reads as no pid: it can still + # A pid file that is not plain ASCII digits reads as no pid: it can still # break a tie among tagged rows, but never admits an untagged one — even # when the listing's pid column carries the very same malformed text. _ctl_listing(monkeypatch, f"@4\tresume-RID\t\t{pid_line}\n", tmp_path) @@ -469,6 +471,18 @@ def test_ctl_window_id_treats_a_malformed_pid_as_pidless(monkeypatch, tmp_path: assert launch.ctl_window_id(tmp_path, "RID") == "@4" +def test_ctl_window_id_refuses_a_newer_pid_beside_an_older_record(monkeypatch, tmp_path: Path): + # The pid sibling is written BEFORE the record, so a launch that dies + # between the two writes leaves the newer mint's pid (4242, window @7) + # beside the older id (@3, whose pane pid is 999). No row satisfies that + # pair — @3 has the wrong pid and @7 is not the window the record names — + # so it admits nothing untagged and the lookup fails closed. + _ctl_listing(monkeypatch, "@3\trun-RID\t\t999\n@7\tresume-RID\t\t4242\n", tmp_path) + _make_run(tmp_path) + _write_record(tmp_path, "RID", "@3", "4242") + assert launch.ctl_window_id(tmp_path, "RID") is None + + def test_ctl_window_id_reads_the_record_after_the_listing(monkeypatch, tmp_path: Path): # Listing and record are two reads of a state a concurrent relaunch moves # between them — it mints its window, records it, then tags it — so their @@ -876,12 +890,14 @@ def _make_run(project: Path, run_id: str = "RID") -> Path: def test_start_detached_records_the_window_it_minted(fake_run, tmp_path: Path): - # The id and, on its own line, the pane pid the mint's listing shows for it - # (#750) — the pid is what later admits the window if its tag never lands. + # The id, byte-identical to the pre-#750 record that older releases read + # whole, and the pane pid the mint's listing shows for it in the sibling + # file — the pid is what later admits the window if its tag never lands. fake_run.windows = "@3\trun-RID\t\t999\n@7\tresume-RID\t\t4242\n" 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\n4242" + assert (run_dir / launch._CTL_WINDOW_FILE).read_bytes() == b"@7" + assert (run_dir / launch._CTL_PID_FILE).read_text(encoding="utf-8") == "4242" # And the record it wrote admits the still-untagged window on lookup. assert launch.ctl_window_id(tmp_path, "RID") == "@7" @@ -893,11 +909,14 @@ def test_start_detached_records_the_id_alone_without_a_readable_pid( fake_run, tmp_path: Path, windows: str ): # Best-effort: a listing that does not show the minted window, or shows a - # pid that is not digits, costs the pid line — never the record or launch. + # pid that is not digits, costs the pid — never the record or launch. And + # an older mint's pid must not survive beside the newer id: it is removed. fake_run.windows = windows run_dir = _make_run(tmp_path) + (run_dir / launch._CTL_PID_FILE).write_text("999", encoding="utf-8") launch.start_resolve_detached(tmp_path, "RID") assert (run_dir / launch._CTL_WINDOW_FILE).read_text(encoding="utf-8") == "@7" + assert not (run_dir / launch._CTL_PID_FILE).exists() def test_start_detached_survives_a_raising_pid_read(fake_run, tmp_path: Path, monkeypatch): @@ -1070,8 +1089,10 @@ def test_forget_refuses_a_linked_run_dir(tmp_path: Path): run_dir.unlink() run_dir.mkdir() (run_dir / launch._CTL_WINDOW_FILE).write_text("@2", encoding="utf-8") + (run_dir / launch._CTL_PID_FILE).write_text("999", encoding="utf-8") launch._forget_ctl_window(project, "RID") assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + assert not (run_dir / launch._CTL_PID_FILE).exists() # the pid sibling goes too @pytest.mark.skipif(not launch.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only") @@ -1138,8 +1159,10 @@ def test_forget_falls_back_to_the_confinement_check_without_dir_fd(tmp_path: Pat run_dir.unlink() run_dir.mkdir() (run_dir / launch._CTL_WINDOW_FILE).write_text("@2", encoding="utf-8") + (run_dir / launch._CTL_PID_FILE).write_text("999", encoding="utf-8") launch._forget_ctl_window(project, "RID") assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + assert not (run_dir / launch._CTL_PID_FILE).exists() # the pid sibling goes too @pytest.mark.skipif(sys.platform == "win32", reason="tab/newline are legal POSIX name bytes") @@ -1334,7 +1357,29 @@ def test_symlinked_record_is_replaced_not_followed(fake_run, tmp_path: Path): # 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\n4242" + assert record.read_text(encoding="utf-8") == "@7" + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +@pytest.mark.parametrize("anchored", [True, False], ids=["dir-fd", "path-fallback"]) +def test_symlinked_pid_sibling_is_replaced_not_followed( + fake_run, tmp_path: Path, monkeypatch, anchored: bool +): + # The pid sibling is the same kind of host-side write as the record, so it + # carries the same rule on both writers: the write lands on the name, never + # on a link a session planted there. + monkeypatch.setattr(launch, "DIR_FD_ANCHORED_WRITES", anchored) + run_dir = _make_run(tmp_path) + outside = tmp_path / "pyproject.toml" + outside.write_text("[project]\n", encoding="utf-8") + sibling = run_dir / launch._CTL_PID_FILE + sibling.symlink_to(outside) + fake_run.windows = "@7\tresume-RID\t\t4242\n" + + assert launch.resume_detached(tmp_path, "RID") == "@7" + assert outside.read_text(encoding="utf-8") == "[project]\n" # not redirected + assert not sibling.is_symlink() + assert sibling.read_text(encoding="utf-8") == "4242" def _fail_the_record(monkeypatch, exc: BaseException) -> None: @@ -1444,11 +1489,12 @@ def test_failed_record_forgets_the_previous_one(fake_run, tmp_path: Path, monkey # *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") + _write_record(tmp_path, "RID", "@2", "999") _fail_the_record(monkeypatch, OSError("disk full")) launch.resume_detached(tmp_path, "RID") assert not (run_dir / launch._CTL_WINDOW_FILE).exists() + assert not (run_dir / launch._CTL_PID_FILE).exists() def test_failed_record_survives_a_non_oserror(fake_run, tmp_path: Path, monkeypatch): From 212296299371b6f3f5605950c63885b624486449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Sat, 3 Oct 2026 19:21:23 +0200 Subject: [PATCH 03/10] fix(tui): admit only tagged control windows; drop the pane-pid proof A pane pid is an observable identifier, not proof of ownership. On a host where same-UID processes can read the process table, code with only project-write access could find a neighbour's pane pid and forge both ctl-window and ctl-window-pid. Measured before choosing: - A pane's own environment exposes the tmux socket path, so on an unsandboxed host process-table read already implies mux access. - Anything minted into the window at creation travels through the client's argv: a naive /proc watcher saw a window-name nonce in 15 of 20 mints. No mint-time secret reaches the mux-access bar without a seam or name-contract change. So ctl_window_id now admits only rows carrying this project's tag, the same bar as writing the tag itself (a mux write). The ctl-window record (still the window id alone, as older releases read it) only breaks ties among tagged rows. The ctl-window-pid sibling and all its machinery are removed. Cost, accepted: a window whose best-effort tag write failed is no longer reachable by a/x until relaunched. That fails closed, the price #749 already accepts for recordless windows. Known gap: the ablation was not run locally; CI verifies the tests. The portability guard passes (446). Refs #750 --- CHANGELOG.md | 10 +- docs/tui-guide.md | 5 +- src/bmad_loop/tui/launch.py | 253 +++++++++------------------------- tests/test_tui_app.py | 17 +-- tests/test_tui_launch.py | 261 +++++++++--------------------------- 5 files changed, 142 insertions(+), 404 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78bc72013..68925121b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,11 +38,11 @@ breaking changes may land in a minor release. - Escalate an environment fault at the review-budget rescue gate instead of deferring the story as unconverged (DW-523). -- Prove an untagged control-session window by the pane pid recorded at launch, - not its reusable window id, so `a`/`x` never reach a neighbour's window after - an id reuse or a forged `ctl-window` record. The pid goes to a sibling - `ctl-window-pid` file, so the record stays the id alone that older releases - read (#750). +- Reach only control-session windows carrying this project's tag from `a`/`x`, + so a reused window id or a forged `ctl-window` record can no longer steer + them onto a neighbour's window; the record now only breaks ties among tagged + windows, and a window whose tag write failed is left alone until relaunched + (#750). ## [0.13.1] — 2026-10-01 diff --git a/docs/tui-guide.md b/docs/tui-guide.md index 51b59440d..62f897929 100644 --- a/docs/tui-guide.md +++ b/docs/tui-guide.md @@ -88,7 +88,10 @@ The TUI never runs an engine in-process. The two halves: 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). + while an older same-run-id window is still parked (#482). Attach/stop only + ever reach a window carrying this project's tag: the record breaks ties + among tagged windows but never vouches for an untagged one, so a window + whose tag write failed is left alone until a relaunch tags one (#750). - **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/tui/launch.py b/src/bmad_loop/tui/launch.py index 97ad7d7c3..73f959985 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -24,7 +24,6 @@ from .. import runs from ..adapters.multiplexer import ( MultiplexerError, - TerminalMultiplexer, get_multiplexer, mux_usable, ) @@ -74,33 +73,14 @@ def session_exists(session: str) -> bool: # target on its own: ctl_window_id re-proves it against the live listing. _CTL_WINDOW_FILE = "ctl-window" -# Its sibling: the pane pid of that same window, read at mint (#750). A file of -# its own rather than a second line in the record, because the record's format -# is a compatibility contract: a release before #750 reads `ctl-window` whole as -# the id, so an `@7` + pid payload would match no row there and its tie-break -# would fall back to the first window, the parked corpse (#482). Written BEFORE -# the record and read AFTER it, so no interleaving pairs a newer id with an -# older pid — see _record_ctl_window. -_CTL_PID_FILE = "ctl-window-pid" - -# Generous ceiling on either hint: the record is a window id (`@7`, or a -# session-qualified `bmad-loop-ctl:@7`) and its sibling a decimal pid, and -# anything longer is already not one. +# 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_pid(project: Path, run_id: str) -> str | None: - """The pane pid the run's last launch recorded, or None when there is none — - including a file that is not plain ASCII digits, which reads as absent - rather than as a pid. The same hardened read as the record.""" - pid = _read_ctl_window(project, run_id, _CTL_PID_FILE) - return pid if pid is not None and pid.isascii() and pid.isdigit() else None - - -def _read_ctl_window(project: Path, run_id: str, name: str = _CTL_WINDOW_FILE) -> str | None: - """The window id recorded by the run's last launch (or, with `name`, the - content of its pid sibling — see _read_ctl_pid), or None when there is +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 @@ -130,7 +110,7 @@ def _read_ctl_window(project: Path, run_id: str, name: str = _CTL_WINDOW_FILE) - 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) / name + 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: @@ -152,7 +132,7 @@ def _read_ctl_window(project: Path, run_id: str, name: str = _CTL_WINDOW_FILE) - def _forget_ctl_window(project: Path, run_id: str) -> None: - """Drop the record and its pid sibling, record first. A launch that cannot name the window it just minted must + """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. @@ -185,27 +165,19 @@ def _forget_ctl_window(project: Path, run_id: str) -> None: if dir_fd is None: return # a component we cannot vouch for — see the ceiling try: - for name in (_CTL_WINDOW_FILE, _CTL_PID_FILE): - _unlink_at(dir_fd, name) + 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 - for name in (_CTL_WINDOW_FILE, _CTL_PID_FILE): - (run_dir / name).unlink(missing_ok=True) + (run_dir / _CTL_WINDOW_FILE).unlink(missing_ok=True) except OSError: pass # a removal we cannot force — see the ceiling -def _unlink_at(dir_fd: int, name: str) -> None: - """`unlink` relative to an anchored directory descriptor; missing_ok by hand.""" - try: - os.unlink(name, dir_fd=dir_fd) - except FileNotFoundError: - pass - - 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 @@ -279,31 +251,9 @@ def _run_dir_is_confined(project: Path, run_dir: Path) -> bool: return True -def _record_ctl_window(project: Path, run_id: str, win_id: str, pane_pid: str | None) -> None: +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. The record holds `` alone, - byte-identical to what releases before #750 read; the mint's pane pid goes - to the `_CTL_PID_FILE` sibling, and is what lets ctl_window_id admit the - window even if its tag write fails (#750). - - Two files are never written as one, so the order is the consistency rule: - pid first, record second, and ctl_window_id reads them the other way round. - A crash or failure between the two writes leaves a newer pid beside the - older id — the old window's pid is not that one, so the pair admits nothing - untagged and the lookup fails closed. A lookup racing a relaunch sees either - a matching pair or that same unsatisfiable one, never a newer id with an - older pid: it reads the pid after the record, and the pid was written - before it. A mint whose pid could not be read removes the sibling, so an - older pid never stands beside the newer id. - - What the order does not serialize is two launches for the same run racing - each other: pid(A), pid(B), id(B), id(A) leaves A's id beside B's pid with - both writes succeeding. That pair also admits nothing untagged, so it fails - closed exactly like a crash between the writes, and the next launch repairs - it; a tagged window is unaffected. A single file kept the pair coherent by - construction, but older releases read that file whole as the id, so the - trade is deliberate. A cross-process lock per run would close it, and is - not worth taking on the launch path for an untagged-only, fail-closed race. + 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 @@ -390,22 +340,12 @@ def _record_ctl_window(project: Path, run_id: str, win_id: str, pane_pid: str | if dir_fd is None: return # unconfined, or a component we cannot vouch for try: - # pid first, so a failure between the two leaves a newer pid - # beside the older id — a pair no listed window can satisfy - if pane_pid: - atomic_write_text_at(dir_fd, _CTL_PID_FILE, pane_pid) - else: - _unlink_at(dir_fd, _CTL_PID_FILE) 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 - if pane_pid: - atomic_write_text(run_dir / _CTL_PID_FILE, pane_pid, follow_symlinks=False) - else: - (run_dir / _CTL_PID_FILE).unlink(missing_ok=True) atomic_write_text(run_dir / _CTL_WINDOW_FILE, win_id, follow_symlinks=False) except Exception: _forget_ctl_window(project, run_id) @@ -433,72 +373,44 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: 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 among the rows - that got in — which, since #531, means the first *tagged* match: a row with - no tag needs the record to be a candidate at all (below), so the recordless - case never has an untagged one to fall back to. - - Scoped to `project` by the PROJECT_OPTION tag: 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. _ctl_window_candidates reads tags by the same rule, - but only that half is still shared: its untagged windows are admitted by the - run dir this one stopped trusting, because the pruning consumer of that - shape is #419's and the fix is partitioned there. - - An untagged row is admitted only when the record names that exact window - AND the pane pid recorded at mint matches the pid the listing shows for it - (#750). Holding the run dir was the earlier gate, and a run dir is a - coincidence rather than a claim: `--run-id` is caller-supplied and - deterministic, so two projects scripting the same id both hold one, and each - then admitted the *other's* untagged window — machine-wide on tmux, where the - control session carries a fixed name (#531). The window id alone was the - next gate, and an id is a reusable handle: tmux 3.4 and psmux 3.3.8 both - restart at `@0`/`@1` after a server restart, so a neighbour's untagged - window could inherit the id this project recorded — and the record sits - under the project root, so a session could equally write any id there. The - pid is the process the mint actually started, and start_detached records it - *before* it tags, so a window whose (best-effort) set_window_option never - landed is still reachable. The pid lives in a sibling file, not in the - record, so the record stays the id alone that releases before #750 read. A - record with no readable pid beside it (written before #750, by a mint whose - pid read failed, or a malformed sibling) still breaks ties among tagged rows - but never admits an untagged one. - - The honest limit: this proves the record was written by something that saw - the window's pid, not that this project minted it. Anything with access to - the control session's multiplexer can list `pane_pid` and write a matching - record — and that same access lets it set the PROJECT_OPTION tag directly, - so the pid raises the bar to exactly the tag's and no further. What it does - close is the id-reuse case and a forged record that merely guesses an id. - - Two residuals, both stated rather than handled. The pid is a reusable - handle too: after a server restart, an untagged neighbour under this run's - name could inherit both the recorded id AND, from the OS, the recorded pid — - a conjunction of two recycles that closing would need a process creation - identity the seam does not carry. And `pane_pid` answers the window's - *active* pane, so a window someone splits by hand and re-focuses fails the - compare; that fails closed (the untagged fallback answers None, exactly as a - recordless window does) and bmad-loop itself never splits the ctl window. - - What the proof costs is reach for a window minted before any record exists — - a fresh `run`/`sweep`, where _record_ctl_window deliberately skips because - the run dir is not minted yet, and anything that lost its record since. Those - answer None until a relaunch records one, and the two consumers wear that - differently: `a` falls through to the live agent session, or says "nothing to - attach" when there is none, while `x` kills nothing — kill_ctl_window no-ops - on None — and still reports the run stopped, so the orchestrator window is - left running with no notice. That is the honest price of the gate, and it is - the cheaper half: fail closed is right here because the alternative is not - "reach my window" but "reach *a* window": with nothing proving ownership the - untagged bucket is filled by listing order, and `x` would kill whatever - sorted first — possibly a neighbour's live orchestrator. - - Untagged stays a *fallback*, not a peer: merged into one listing-ordered - list a recorded untagged row would compete on index with a correctly tagged - one, and the tag is the stronger proof of the two, so untagged is consulted - only when nothing carries this project's tag. + no-op). With no record at all the answer is the first tagged match. + + Scoped to `project` by the PROJECT_OPTION tag, and by nothing else: only a + row carrying one of this project's accepted tags is a candidate. 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. _ctl_window_candidates reads tags by the same + rule, but only that half is shared: its untagged windows are still admitted + by the run dir, because the pruning consumer of that shape is #419's. + + An untagged row is never a candidate, however it is vouched for (#750). + Every proof tried for one was a fact the workspace can forge. The run dir is + a coincidence of a caller-supplied id (#531). The record lives under the + project root every coding session can write, and the window id it names is + a reusable handle — tmux 3.4 and psmux 3.3.8 both restart at `@0`/`@1` after + a server restart. A pane pid recorded beside it is observable rather than + secret: any same-UID process reads it from the process table, along with the + `TMUX` socket path in the pane's environment, and anything minted into the + window at creation passes through the multiplexer client's argv, which the + process table also shows. The tag is the one claim that needs a write + through the multiplexer to forge, so it is the whole proof. + + The honest bar, then: forging ownership here requires a mux write, exactly + as forging the tag does — no more, and no less. On a host where the coding + session runs unsandboxed as the same user, that is any process that can + find the socket, which the pane environment hands out; the gate is only as + strong as the session's isolation from the multiplexer. + + What it costs is reach for a window whose best-effort tag write failed: + start_detached records before it tags, but the record no longer admits + anything on its own, so that window answers None until a relaunch tags one. + The two consumers wear that differently: `a` falls through to the live agent + session, or says "nothing to attach" when there is none, while `x` kills + nothing — kill_ctl_window no-ops on None — and still reports the run + stopped, so the orchestrator window is left running with no notice. Fail + closed is right here because the alternative is not "reach my window" but + "reach *a* window" — possibly a neighbour's live orchestrator. Read-only throughout: this never writes the record and never touches the tag — re-tagging on read is claiming, not proving, and would hand a @@ -507,34 +419,26 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: return None mine = runs.accepted_tags(project) tagged: list[str] = [] - untagged: list[str] = [] - # pane_pid last: the base's bounded split leaves any stray tab in the final - # field, and a pid that does not read as digits simply fails the compare. rows = get_multiplexer().list_windows( - ctl_session(project), ["window_id", "window_name", runs.PROJECT_OPTION, "pane_pid"] + ctl_session(project), ["window_id", "window_name", runs.PROJECT_OPTION] ) - # Below the listing, above the loop. The loop needs it — it is what admits an - # untagged row — but listing and record are two reads of a state a concurrent + # Below the listing. Listing and record are two reads of a state a concurrent # relaunch can move between them, never one snapshot, so the ordering is the # only thing to get right and this keeps the one the record has always had. # Read FIRST, a relaunch landing in the gap leaves a record older than the # listing: it names the window that relaunch superseded, the listing shows it, - # and `recorded in matches` replays the corpse. Read here, the same relaunch + # and `recorded in tagged` replays the corpse. Read here, the same relaunch # leaves a record newer than the listing, naming a window the listing does not # carry yet — so it fails the re-prove and the answer falls back to a match # that was at least live when the listing was taken. `rows` is materialized - # (list_windows returns a list), so the loop pays nothing for the move. The - # pid sibling is read after the record, the reverse of the write order, so a - # relaunch in between yields a matching pair or an unsatisfiable one, never a - # newer id beside an older pid (see _record_ctl_window). + # (list_windows returns a list), so the loop pays nothing for the move. recorded = _read_ctl_window(project, run_id) - recorded_pid = _read_ctl_pid(project, run_id) - for win_id, name, tag, pane_pid in rows: + 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 CAN produce an - # empty *tag* or pid — it fills trailing fields — which is the untagged - # case below; window_id stays field 0 of 4.) + # empty *tag* — it fills trailing fields — which the tag test below + # refuses; 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 @@ -556,29 +460,18 @@ def ctl_window_id(project: Path, run_id: str) -> str | None: # digest alone would strand this project's own orchestrator — prunable # by _ctl_window_candidates, which accepts the legacy tag, yet # unreachable by `a` and `x`, which resolve through here. + # An empty tag is refused the same way (#750): see the docstring. if tag in mine: tagged.append(win_id) - elif ( - not tag - and win_id == recorded - and recorded_pid is not None - and pane_pid.strip() == recorded_pid - ): - # untagged, but the record names this window and the pid its mint - # read — proof of the mint, not of the tag, so it only counts if - # nothing is tagged. A record with no pid never gets here (#750). - untagged.append(win_id) - matches = tagged or untagged - if not matches: + if not tagged: return None - # Membership in `matches`, not mere presence in the listing: it re-checks the + # Membership in `tagged`, not mere presence in the listing: it re-checks the # name and the project-scoping predicates, so a record whose id is absent from # the scoped matches — killed, pruned, renamed onto another run, or naming a - # row this project cannot claim — is not replayed. Among tagged rows this is a - # tie-break only (the tag already proved ownership), so a pid-less record - # still counts here. It is what turns a stale id from a replayed target into - # a fallthrough. - return recorded if recorded in matches else matches[0] + # row this project cannot claim — is not replayed. A tie-break among rows the + # tag already proved, never a proof on its own: it is what turns a stale id + # from a replayed target into a fallthrough. + return recorded if recorded in tagged else tagged[0] def ctl_window_recorded(project: Path, run_id: str, win_id: str) -> bool: @@ -955,22 +848,6 @@ def cli_argv(*tail: str) -> list[str]: return [sys.executable, "-m", "bmad_loop.cli", *tail] -def _minted_pane_pid(mux: TerminalMultiplexer, ctl: str, win_id: str) -> str | None: - """The pane pid of the window just minted, for the ctl-window record (#750), - or None when the listing does not show it or the pid is not plain digits. - Best-effort: the window is already running, so a failed read only costs the - untagged fallback in ctl_window_id, never the launch.""" - try: - rows = mux.list_windows(ctl, ["window_id", "pane_pid"]) - except MultiplexerError: - return None - for row_id, pid in rows: - pid = pid.strip() - if row_id == win_id and pid.isascii() and pid.isdigit(): - return pid - return None - - def start_detached(project: Path, argv_tail: list[str], run_id: str, kind: str) -> str | None: """Run a bmad-loop command in a new window of the control session. @@ -1024,7 +901,7 @@ def start_detached(project: Path, argv_tail: list[str], run_id: str, kind: str) # 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, _minted_pane_pid(mux, ctl, win_id)) + _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)) diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index d803f86a4..ef0e65f82 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -3885,10 +3885,9 @@ async def test_attach_uses_the_recorded_ctl_window(project_tree, monkeypatch): # 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 under that - # root, so these untagged rows prove nothing and the lookup answers None - # (#531). session_exists is stubbed True here, so `a` then takes the live - # agent session instead of the ctl window: nothing is selected and nothing is - # stamped, and both assertions below fail. + # root, so the tie-break among these tagged rows falls back to the parked + # `@1` and both assertions below fail. Tagged as start_detached leaves them: + # an untagged row is never a candidate (#750), whatever the record says. import subprocess as _subprocess from bmad_loop.adapters import tmux_base @@ -3896,17 +3895,13 @@ async def test_attach_uses_the_recorded_ctl_window(project_tree, monkeypatch): rid = "20260611-100000-aaaa" run_dir = make_run(project_tree.project, rid, run_type="sweep", alive=True) Journal(run_dir).append("decision-pending", dw_id="DW-7", question="q?") - # the id + the pane pid its mint read, in the sibling: an untagged row - # needs both (#750) (run_dir / launch._CTL_WINDOW_FILE).write_text("@2", encoding="utf-8") - (run_dir / launch._CTL_PID_FILE).write_text("4242", encoding="utf-8") + tag = runs_mod.project_tag(project_tree.project) selected: list[str] = [] def fake(argv, **kwargs): - # rows as `window_id, window_name, tag, pane_pid` — the lookup's fields - out = ( - f"@1\trun-{rid}\t\t11\n@2\tresume-{rid}\t\t4242\n" if argv[1] == "list-windows" else "" - ) + rows = f"@1\trun-{rid}\t{tag}\n@2\tresume-{rid}\t{tag}\n" + out = rows if argv[1] == "list-windows" else "" return _subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") monkeypatch.setattr(tmux_base.subprocess, "run", fake) diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index d13935031..47296fd7f 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -11,7 +11,6 @@ import json import os -import re import shlex import signal import stat @@ -32,37 +31,23 @@ # tmux — so pin tmux by name (a no-op on a stock POSIX box). pytestmark = pytest.mark.usefixtures("force_tmux_backend") -# Scripted listing rows are written positionally in this order; fakes project -# them onto whatever fields the caller's `-F` format (or `fields`) asks for, so a -# lookup asking for `pane_pid` sees the pid and the mint's own `window_id, -# pane_pid` read sees it too, rather than the name in the pid slot. -_ROW_FIELDS = ["window_id", "window_name", runs.PROJECT_OPTION, "pane_pid"] - - -def _project_row(row: tuple[str, ...] | list[str], fields: list[str]) -> tuple[str, ...]: - cols = list(row) + [""] * (len(_ROW_FIELDS) - len(row)) - return tuple(cols[_ROW_FIELDS.index(f)] for f in fields) - - -def _project_listing(rows: str, argv: list[str]) -> str: - """`rows` reshaped to the fields a `list-windows -F` argv requests.""" - fields = re.findall(r"#\{([^}]*)\}", argv[argv.index("-F") + 1]) - return "".join( - "\t".join(_project_row(line.split("\t", len(_ROW_FIELDS) - 1), fields)) + "\n" - for line in rows.splitlines() - ) - class FakeRun: """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.""" + ctl_window_recorded re-proves the record against. + + Like a real backend, a PROJECT_OPTION tag a `set-option` stamps on a window + shows up in its listed row — the only proof of ownership ctl_window_id + accepts (#750). A scripted row with its own third field (an empty one for + an untagged window) keeps it.""" 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 + self.tags: dict[str, str] = {} def __call__(self, argv, **kwargs): self.calls.append(list(argv)) @@ -70,10 +55,17 @@ def __call__(self, argv, **kwargs): out = "" if argv[1] == "new-window": out = "@7\n" + elif argv[1:4] == ["set-option", "-w", "-t"] and argv[5:6] == [runs.PROJECT_OPTION]: + self.tags[argv[4]] = argv[6] # tmux set-option -w -t