diff --git a/CHANGELOG.md b/CHANGELOG.md index b711d728..84d0c8cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -180,6 +180,14 @@ breaking changes may land in a minor release. ### Fixed +- Prove ownership of an untagged control window before targeting it (#531). `ctl_window_id` + admitted an untagged row whenever this project merely held a run dir for the run id, and + `--run-id` is caller-supplied, so two projects scripting the same id each admitted the + _other's_ window — `a` attached to it, the return stamp landed on it, and `x` killed a live + orchestrator next door. An untagged row now needs the record this project's own launch + wrote for that exact window; with no record the lookup answers nothing rather than guessing + by listing order. A window minted before its record exists (a fresh `run`/`sweep`) is + unreachable by `a`/`x` until a relaunch records one. - Anchor the TUI's paused-spec read and its `Request replan` write on the tree the run owns. Under isolation both resolved against the main checkout, so the review modals showed that copy of the spec and the replan reset it — reporting success while the run's diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index b9ae3efc..b2895ac2 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -373,34 +373,92 @@ 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, 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.""" + 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. + 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. + + 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. + + 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.""" if not mux_available(): return None mine = runs.accepted_tags(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(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 + # 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 + # 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 = _read_ctl_window(project, run_id) 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 @@ -430,17 +488,21 @@ 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 local: - # untagged, and this project holds the run dir — ownership is - # plausible but unproven, so it only counts if nothing is tagged + 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 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) + # 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. return recorded if recorded in matches else matches[0] diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index 7bfbbc44..7e1c83fb 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -3071,8 +3071,11 @@ 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. + # — 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. import subprocess as _subprocess from bmad_loop.adapters import tmux_base diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index 920a4adc..12c5b813 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -401,31 +401,115 @@ def test_ctl_window_id_accepts_a_legacy_path_tag(monkeypatch, tmp_path: Path): assert launch.ctl_window_id(tmp_path, "RID") is None -def test_ctl_window_id_admits_an_untagged_window_with_a_local_run(monkeypatch, tmp_path: Path): +def test_ctl_window_id_admits_an_untagged_window_the_record_names(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. + # 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) - _make_run(tmp_path) + _make_run(tmp_path) # _record_ctl_window refuses to write without one + _write_record(tmp_path, "RID", "@4") 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. +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 + # order is load-bearing. Read the record FIRST and this call holds the id the + # relaunch just superseded while the listing already carries both rows, and + # `recorded in matches` replays the corpse. Hoist the read above the + # list_windows call and this answers "@1". + tag = runs.project_tag(tmp_path) + _make_run(tmp_path) + _write_record(tmp_path, "RID", "@1") + + def fake(argv, **kwargs): + out = "" + if argv[1] == "list-windows": + # the relaunch lands here: its window is listed and its record written + _write_record(tmp_path, "RID", "@2") + out = f"@1\trun-RID\t{tag}\n@2\tresume-RID\t{tag}\n" + 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(tmp_path, "RID") == "@2" + + +def test_ctl_window_id_refuses_an_untagged_window_without_a_record(monkeypatch, tmp_path: Path): + # A fresh `run`: the run dir exists but _record_ctl_window skipped it, so + # nothing proves the untagged window is ours. Fail closed — restore the old + # gate (`runs.is_run(run_dir_for(...))`) and this answers "@4", a window + # that may belong to any project on the box. _ctl_listing(monkeypatch, "@4\tresume-RID\t\n", tmp_path) + _make_run(tmp_path) assert launch.ctl_window_id(tmp_path, "RID") is None +def test_ctl_window_id_refuses_untagged_windows_the_record_does_not_name( + monkeypatch, tmp_path: Path +): + # 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) + _make_run(tmp_path) + _write_record(tmp_path, "RID", "@9") # killed, pruned, or never in this listing + assert launch.ctl_window_id(tmp_path, "RID") is None + + +def test_ctl_window_id_refuses_an_untagged_neighbour_on_a_run_id_collision( + monkeypatch, tmp_path: Path +): + # #531: `--run-id` is caller-supplied, so two projects can hold a run dir + # for the same id, and the control session is shared across them. Ownership + # inferred from that collision let this project attach to, return-stamp and + # kill the neighbour's LIVE orchestrator window. + mine, theirs = tmp_path / "mine", tmp_path / "theirs" + mine.mkdir() + 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") + + assert launch.ctl_window_id(mine, "RID") is None + + # Positive control: the same row, resolved by the project that recorded it, + # so the None above is the record gate refusing rather than a listing that + # parsed to nothing or a run id that never matched. + 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 + _make_run(tmp_path) + _write_record(tmp_path, "RID", "@4") + assert launch.ctl_window_id(tmp_path, "RID") == "@4" + + 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. + # Untagged is a fallback, not a peer, even now that it takes a record to get + # in. Merged into one listing-ordered list the recorded untagged row beats + # 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) - _make_run(tmp_path) # local run dir: the untagged row is otherwise admitted + _write_record(tmp_path, "RID", "@1") # recorded: the untagged row is otherwise admitted assert launch.ctl_window_id(tmp_path, "RID") == "@2" @@ -788,6 +872,11 @@ def test_symlinked_run_dir_is_refused(fake_run, tmp_path: Path): # 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. + # Tagged as start_detached leaves it, which FakeRun does not fold into its + # scripted listing: the record write is the thing refused here, so since #531 + # the tag is the only proof of ownership left and an untagged row would + # answer None for that reason rather than for the confinement this is about. + fake_run.windows = f"@7\tresume-RID\t{runs.project_tag(tmp_path)}\n" assert launch.resume_detached(tmp_path, "RID") == "@7" assert not (outside / launch._CTL_WINDOW_FILE).exists() # nothing escaped @@ -840,6 +929,9 @@ def test_record_falls_back_to_the_confinement_check_without_dir_fd(fake_run, tmp run_dir.parent.mkdir(parents=True) run_dir.symlink_to(outside) + # Tagged for the same reason as the dir-fd sibling above: the refused write + # leaves the tag as the only ownership proof this listing can carry. + fake_run.windows = f"@7\tresume-RID\t{runs.project_tag(tmp_path)}\n" assert launch.resume_detached(tmp_path, "RID") == "@7" assert not (outside / launch._CTL_WINDOW_FILE).exists() @@ -1012,7 +1104,7 @@ def test_a_separator_in_the_project_path_does_not_admit_a_foreign_window( theirs = tmp_path / "theirproj" mine.mkdir() theirs.mkdir() - _make_run(mine) # so `local` is True — ownership-by-run-dir would say yes + _make_run(mine) # a run dir here, which the pre-#531 untagged gate accepted _ctl_listing(monkeypatch, f"@9\trun-RID\t{runs.project_tag(theirs)}\n") assert launch.ctl_window_id(mine, "RID") is None @@ -1185,7 +1277,13 @@ def test_resume_reports_a_record_that_did_not_survive(fake_run, tmp_path: Path, # #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" + # + # Tagged as start_detached leaves them, which FakeRun does not fold into its + # scripted listing: untagged rows have needed the record to be candidates at + # all since #531, so they would answer None here for the wrong reason — the + # bucket being empty rather than the scan preferring the corpse. + tag = runs.project_tag(tmp_path) + fake_run.windows = f"@1\trun-RID\t{tag}\n@7\tresume-RID\t{tag}\n" _make_run(tmp_path) _fail_the_record(monkeypatch, OSError("read-only file system")) @@ -1196,7 +1294,8 @@ 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" + tag = runs.project_tag(tmp_path) # the same listing, tagged for the same reason + fake_run.windows = f"@1\trun-RID\t{tag}\n@7\tresume-RID\t{tag}\n" _make_run(tmp_path) assert launch.resume_detached(tmp_path, "RID") == "@7" @@ -1206,13 +1305,15 @@ def test_resume_does_not_warn_when_the_scan_is_unambiguous(fake_run, tmp_path: P # 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" + # + # Carrying the tag start_detached stamps, which FakeRun does not fold into + # its scripted listing: with the record write failing, the tag is the only + # proof of ownership left, and an untagged row would answer None for that + # reason rather than for the unambiguous scan this is about. + fake_run.windows = f"@7\tresume-RID\t{runs.project_tag(tmp_path)}\n" _make_run(tmp_path) - def boom(*_a, **_k): - raise OSError("read-only file system") - - monkeypatch.setattr(launch, "atomic_write_text", boom) + _fail_the_record(monkeypatch, OSError("read-only file system")) assert launch.resume_detached(tmp_path, "RID") == "@7" @@ -1234,7 +1335,12 @@ def test_resume_reports_a_record_the_listing_does_not_carry(fake_run, tmp_path: # 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" + # Tagged, so the fallthrough this is about has candidates: an untagged row + # is admitted only by an id-equal record, which is the very thing diverging + # here, so both rows would drop out and the None would be about the empty + # bucket rather than about the shape. + tag = runs.project_tag(tmp_path) + fake_run.windows = f"@1\trun-RID\t{tag}\nctl:@7\tresume-RID\t{tag}\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.