From 1483d9774f97757bbb516d5d2e46072734c037af Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 16:43:43 -0700 Subject: [PATCH 01/11] feat(relay): importable event-write twin + `bmad-loop relay ` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of the #494/#498 events relocation. The hardened event write lives in `data/bmad_loop_hook.py`, which is copied into target projects and is stdlib-only by contract — it cannot import `bmad_loop`, and it ships as package data rather than an importable module, so neither side can import the other. Add `events.py` as an explicit twin instead, and pin the copy with an AST parity test over the five twinned names so the two writers of the events control plane cannot be hardened differently by accident. The half `ast.get_source_segment` cannot reach (the payload shaping the hook does inline in `main()`) is pinned behaviorally against the real hook subprocess. `bmad-loop relay ` is the #461 Phase 2 hook target: an installed console script instead of a workspace file path a branch switch can take away. It honours the hook contract exactly — silent no-op outside a driven session, garbage stdin tolerated, OSError degraded, nothing ever on stdout, rc 0 always — and dispatches ahead of `main()`'s shared try/except and its `_configure_mux` call, whose arms print `error: …` and return 1/130, the CLI-window failure that contract forbids. Relay touches neither mux nor policy, so an unparseable policy.toml must not stop a session reporting that it stopped. Retarget the stale #461 Phase 2 coupling note on `hooks.relay-present`, which still named the abandoned `-m bmad_loop.hookrelay` spelling; the mandate to retarget rather than drop the check is unchanged. The `events.py` env reads get their own `ENV_READ_ALLOW` entry with its own rationale: the existing one justifies entries by "cannot import bmad_loop", which does not apply in-package — the justification here is parity with the twin. --- CHANGELOG.md | 9 + src/bmad_loop/cli.py | 65 +++- src/bmad_loop/events.py | 263 ++++++++++++++ tests/test_events.py | 587 ++++++++++++++++++++++++++++++++ tests/test_portability_guard.py | 10 + 5 files changed, 930 insertions(+), 4 deletions(-) create mode 100644 src/bmad_loop/events.py create mode 100644 tests/test_events.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f49cad39..e02b7ff5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,15 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Added +- **`bmad-loop relay `: write a session event without the copied-in script (#494).** The + hardened event write gains an importable twin, `events.py`, held byte-identical to the stdlib-only + hook relay by an AST parity test — the two writers of the events control plane can no longer be + hardened differently by accident. The new `relay` subcommand takes the same hook payload on stdin + and honours the same contract: nothing on stdout, rc 0 always, a silent no-op outside a driven + session. It dispatches ahead of `main()`'s shared error handler and its mux configuration, so + neither a broken `policy.toml` nor an unexpected exception can turn a session's Stop signal into a + failed hook. First phase of moving `events/` to a control-plane root outside the project tree. + - **Coding-CLI adapter registry: a new adapter class ships out-of-tree (#226).** The transport axis has long been extensible out-of-tree; the CLI axis had no equivalent, so a CLI needing its own adapter _class_ forced a name-branch in the run bootstrap. A profile's new `adapter` field names a diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 0db5f723..66bad4b5 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -21,6 +21,7 @@ deferredwork, devcontract, envvars, + events, frontmatter, gates, install, @@ -495,10 +496,13 @@ def cmd_validate(args: argparse.Namespace) -> int: # A distinct id, not a repurposed `hooks.registered` — the two answer different # questions and an operator needs to see which one failed. # - # COUPLING (#461 Phase 2): Phase 2 moves the relay to - # ` -m bmad_loop.hookrelay` and retires HOOK_SCRIPT_REL. It must - # RETARGET this check to stat the registered interpreter (`hooks.interpreter`), - # not drop it — the stall it guards against survives the move. + # COUPLING (#461 Phase 2): Phase 2 moves the relay to the installed console + # script — `bmad-loop relay ` (cmd_relay / events.py), NOT the + # ` -m bmad_loop.hookrelay` spelling this once anticipated — and + # retires HOOK_SCRIPT_REL. It must RETARGET this check to stat what the + # registration actually points at (the resolved `bmad-loop` executable), not + # drop it — the stall it guards against survives the move: an entry point that + # is gone or unreadable strands every hook event exactly like a missing script. if any_hooks_registered: relay = project / install.HOOK_SCRIPT_REL # Existence is not enough: `is_file()` stays True for a mode-000 file, and @@ -3639,6 +3643,32 @@ def cmd_init(args: argparse.Namespace) -> int: return install_into(project, clis=clis, skills=args.skills, force_skills=args.force_skills) +def cmd_relay(args: argparse.Namespace) -> int: + """``bmad-loop relay `` — the hook relay as an installed console script. + + Total by contract, unlike every other handler: a coding CLI runs this INSIDE + the session whose completion it reports, and several of them surface a + non-zero hook exit as a failed tool call in that session. So nothing here + escalates. :func:`events.relay` already swallows the relay-level failures + (unset env, garbage stdin, a hostile events dir); the backstop below covers + the rest — an unexpected exception is a bug in this file, and a bug must not + be the reason a run's Stop signal turns into a broken session. It reports on + stderr, never stdout: hook stdout is parsed by the host. + + Dispatched from ``main()`` BEFORE its shared ``try``/``except`` on purpose — + see the comment there. + """ + try: + return events.relay(args.event, sys.stdin) + except Exception as e: + # Deliberately broad, and deliberately not re-raised: the alternative to a + # diagnostic line here is a traceback on the host's hook channel plus a + # non-zero rc. Narrower than the BaseException it could be — a Ctrl+C or a + # SystemExit is not a relay-level problem and keeps its own exit path. + print(f"bmad-loop relay: {type(e).__name__}: {e}", file=sys.stderr) + return ExitCode.OK + + def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser( prog="bmad-loop", @@ -3978,7 +4008,34 @@ def add(name: str, func, help: str, *, aliases=()) -> argparse.ArgumentParser: "fixes repaint tearing over slow/SSH links; also settable via [tui] low_frame_rate", ) + # Registered directly rather than through `add()`: `relay` takes no --project. + # It is handed a run directory by the engine (via the session env) and must + # work with no project state at all — see the dispatch below. + relay_p = sub.add_parser( + "relay", + help="write one session event file from a coding-CLI hook payload on stdin " + "(invoked by the installed hooks, not by hand)", + ) + relay_p.add_argument( + "event", + nargs="?", + default="Unknown", + help="canonical event name (Stop, SessionStart, …); the hooks always pass one", + ) + relay_p.set_defaults(func=cmd_relay) + args = parser.parse_args(argv) + # `relay` dispatches HERE, ahead of everything below, and the placement is the + # contract rather than an optimization. A coding CLI runs `bmad-loop relay Stop` + # inside the session whose completion it reports, and a hook that exits non-zero + # is surfaced by several hosts as a failed tool call in that session — so the + # `except` arms below, which print `error: …` and return 1 (or 130), are exactly + # the outcome the hook contract forbids. `_configure_mux(_project(args))` is + # skipped for the same reason and one more: relay touches neither mux nor policy, + # and a project whose policy.toml is broken must still be able to report that its + # session stopped. `cmd_relay` is total, so nothing is lost by not wrapping it. + if args.func is cmd_relay: + return cmd_relay(args) try: # Install the policy [mux] backend choice before dispatch: several # handlers (probe/diagnose/attach/stop/cleanup/tui) reach the mux diff --git a/src/bmad_loop/events.py b/src/bmad_loop/events.py new file mode 100644 index 00000000..2df5498c --- /dev/null +++ b/src/bmad_loop/events.py @@ -0,0 +1,263 @@ +"""Importable twin of the hardened event write in ``data/bmad_loop_hook.py``. + +The hook script is COPIED into every target project and runs inside the coding +CLI's process under whatever interpreter the host has, so it is stdlib-only by +contract (its docstring says so) and cannot import ``bmad_loop`` to reach this +module — and this module cannot import it back, since it ships as package DATA +rather than as an importable module. Hence a twin rather than shared code: +``_LINK_REPARSE_TAGS``, ``_first_workspace``, ``_is_link_like``, ``_write_all`` +and ``_write_event`` below are byte-identical copies of the hook's, pinned that +way by ``tests/test_events.py::test_the_twinned_source_is_identical`` — which +AST-extracts both sides and compares the source segments, so a fix applied to one +writer of the events control plane and not the other cannot pass review silently. + +Because they are byte-identical, their docstrings and comments are written from +the hook script's vantage point ("this relay runs under whatever interpreter the +host has") and stay that way on purpose: rewording either side to suit its own +file breaks parity, and diverging is exactly what two separately-hardened writers +of one control plane must not do. What the shared text says about the attack, the +platform branches, and the residual Windows windows holds for both. + +The rest of the module is this side's own: the payload shaping the hook does +inline in its ``main()``, and :func:`relay`, which backs ``bmad-loop relay +`` — the #461 Phase 2 hook target, an installed console script instead of +a file path inside the workspace that a branch switch can take away. +""" + +from __future__ import annotations + +import json +import os +import stat +import time +from typing import IO, Any + +# Windows reparse tags that make a directory entry REDIRECT somewhere else, +# compared against os.lstat().st_reparse_tag (Windows, 3.8+). Deliberately not +# os.path.isjunction(), which is 3.12+ — this relay runs under whatever +# interpreter the host has, not under the orchestrator's. Deliberately not "any +# reparse tag" either: cloud placeholders (OneDrive) and dedup stubs are reparse +# points too, and refusing those would stall a legitimate run. Empty on POSIX. +_LINK_REPARSE_TAGS = tuple( + tag + for tag in ( + getattr(stat, "IO_REPARSE_TAG_SYMLINK", None), + getattr(stat, "IO_REPARSE_TAG_MOUNT_POINT", None), + ) + if tag is not None +) + + +def _first_workspace(payload): + paths = payload.get("workspacePaths") + if isinstance(paths, list) and paths and isinstance(paths[0], str): + return paths[0] + return None + + +def _is_link_like(path): + """True when `path` redirects elsewhere: a POSIX symlink, or a Windows + symlink OR DIRECTORY JUNCTION. + + `os.path.islink()` is False for a junction — junctions are a distinct + reparse kind, which is why `os.path.isjunction()` exists at all. On Windows + the junction is the arm that matters: `mklink /J` needs no elevation, while + a directory symlink needs SeCreateSymbolicLinkPrivilege or Developer Mode — + so the unprivileged attack is exactly the one `islink()` misses. + """ + if os.path.islink(path): + return True + try: + return getattr(os.lstat(path), "st_reparse_tag", 0) in _LINK_REPARSE_TAGS + except OSError: + return False + + +def _write_all(fd, data): + """Write every byte of `data` to `fd`. + + `os.write()` may write FEWER bytes than asked and simply return the count. A + truncated event file is not merely retried, it is lost: `SignalWatcher.poll` + adds a filename to its consumed set BEFORE parsing it (signals.py), so + malformed JSON is skipped and never re-read — the session's Stop signal is + gone for good and the run waits out `session_timeout_min`. The buffered + `open()` this replaced looped internally; the raw fd needed for + O_NOFOLLOW/dir_fd does not, so loop here. + """ + view = memoryview(data) + while view: + written = os.write(fd, view) + if written <= 0: # not observed in practice; a spinning hook is worse + raise OSError("short write to the event file") + view = view[written:] + + +def _write_event(events_dir, name, event): + """Write one event file into `events_dir`, refusing to follow a redirect. + + The events dir is the orchestrator's control plane. A driven session has + write access to the project, so it could plant `/events` as a + symlink (or, on Windows, a junction) and redirect — or swallow — the + completion signal, stalling the run to `session_timeout_min` instead of + completing. `os.makedirs(exist_ok=True)` `isdir()`-checks THROUGH such a + link, so the refusal has to come before it. That refusal works on every + platform. + + Where the platform has them, the create+replace is anchored to a dir_fd + opened O_NOFOLLOW: every later operation goes through that fd, so a swap + after the check cannot reach the write. Windows has neither + O_NOFOLLOW/O_DIRECTORY nor a handle-relative open (`os.supports_dir_fd` is + empty — dir_fd is implemented with the POSIX `*at` calls), so its fallback + re-resolves the path and the check-to-write window stays open there. It is + NARROWED, not closed: the redirect check runs again after the payload is + written and before it is published, so a swap still in place is refused and + the temp file removed. Two windows stay open on that path (#494), both + measured: a swap-and-restore around the create is undetectable from stdlib + Python, and a swap landing after the second check leaves the path-based + publish unable to find the temp file it wrote — it either raises or renames + a file the attacker planted inside the attacker's own directory. Neither + redirects the payload, and both end where a refusal ends: no event, so the + run waits out session_timeout_min. That is the same outcome an attacker gets + for free by leaving a redirect in place, which is refused without any race — + winning the race buys no capability, which is why the residual is accepted + rather than chased into ctypes/NtCreateFile inside a stdlib-only relay. + + Mode is 0o600 (narrowed from the umask-derived mode an ordinary `open()` + produced): only the operator running the loop reads these. + + Raises OSError on any refusal or failure; the caller degrades to a no-op. + """ + if _is_link_like(events_dir): + raise OSError(f"refusing to write events into a redirected directory: {events_dir}") + os.makedirs(events_dir, exist_ok=True) + data = json.dumps(event).encode("utf-8") + tmp = name + ".tmp" + o_nofollow = getattr(os, "O_NOFOLLOW", 0) + o_directory = getattr(os, "O_DIRECTORY", 0) + # O_BINARY is a no-op flag on POSIX; on Windows it stops the fd from + # newline-translating what os.write() puts through it. + create = os.O_WRONLY | os.O_CREAT | os.O_EXCL | o_nofollow | getattr(os, "O_BINARY", 0) + # Probe os.rename, not os.replace: CPython omits os.replace from + # supports_dir_fd on Linux even though it accepts src_dir_fd/dst_dir_fd, so + # probing it would leave this whole branch dead everywhere. This branch is + # POSIX-only by construction, and there rename(2) IS the atomic-replace + # primitive os.replace wraps — probe the function actually called. + if o_nofollow and o_directory and {os.open, os.rename} <= os.supports_dir_fd: + dir_fd = os.open(events_dir, os.O_RDONLY | o_directory | o_nofollow) + try: + fd = os.open(tmp, create, 0o600, dir_fd=dir_fd) + try: + _write_all(fd, data) + finally: + os.close(fd) + os.rename(tmp, name, src_dir_fd=dir_fd, dst_dir_fd=dir_fd) + finally: + os.close(dir_fd) + return + # Fallback (Windows): no dir_fd to anchor to, so the create below re-resolves + # events_dir by path. A swap into a junction between the check above and this + # create would have put the temp file inside the attacker's directory. Check + # again before publishing, so a swap that is still in place is refused rather + # than followed — the realistic shape, since a junction has to persist to + # capture the events the attacker is after. + tmp_path = os.path.join(events_dir, tmp) + fd = os.open(tmp_path, create, 0o600) + try: + _write_all(fd, data) + finally: + os.close(fd) + if _is_link_like(events_dir): + try: + os.unlink(tmp_path) + except OSError: + pass + raise OSError(f"events directory was redirected mid-write: {events_dir}") + os.replace(tmp_path, os.path.join(events_dir, name)) + + +# --------------------------------------------------------------- this side only + + +def event_file_name(ts: int, task_id: str, event_name: str) -> str: + """The event file's name. Sorted-by-time by construction (``ts`` first, fixed + width in practice), and carrying the task id so ``SignalWatcher`` can attribute + a file without opening it. Mirrors the hook's f-string exactly; the twin above + stops at the write, so this and :func:`shape_event` are pinned behaviorally + instead (``test_relay_and_hook_produce_the_same_event``).""" + return f"{ts}-{task_id}-{event_name}.json" + + +def shape_event(ts: int, event_name: str, task_id: str, payload: dict[str, Any]) -> dict[str, Any]: + """The event record the orchestrator consumes, built from one hook payload.""" + return { + "ts": ts, + "event": event_name, + "task_id": task_id, + # Payload keys vary by CLI: snake_case (claude/codex), conversation_id + # (cursor), or camelCase (copilot's sessionId/transcriptPath, agy's + # conversationId). Try each. + "session_id": ( + payload.get("session_id") + or payload.get("conversation_id") + or payload.get("sessionId") + or payload.get("conversationId") + ), + "transcript_path": payload.get("transcript_path") or payload.get("transcriptPath"), + # agy sends no cwd — it sends workspacePaths, a list of workspace roots. + "cwd": payload.get("cwd") or _first_workspace(payload), + } + + +def _read_payload(stdin: IO[str]) -> dict[str, Any]: + """The hook payload, or an empty dict for anything unreadable. + + A hook that fires with nothing on stdin, half a JSON document, undecodable + bytes, or a bare list is not an error the operator can act on — the event still + has to be written, because the run's completion signal rides on it. Every + non-dict outcome collapses to ``{}`` and the shaped event simply carries nulls. + ``UnicodeDecodeError`` and ``json.JSONDecodeError`` are both ``ValueError`` + subclasses and ride the same arm; ``OSError`` covers a closed or unreadable + descriptor. + """ + try: + payload = json.load(stdin) + except (ValueError, OSError): + return {} + return payload if isinstance(payload, dict) else {} + + +def relay(event_name: str, stdin: IO[str]) -> int: + """Write one event file for the session this process was spawned inside. + + The contract is the hook script's, because the hook config points at one or + the other and the orchestrator must not be able to tell which ran: a silent + no-op when the session was not spawned by bmad-loop (the env vars are the + detector), garbage stdin tolerated, and any ``OSError`` from a hostile or + broken events dir degrading to rc 0. Never anything on stdout — the CLI hosts + parse hook stdout — and never a non-zero rc, which several of them surface as + a failed tool call inside the very session whose completion this reports. + + Returns 0 unconditionally. Nothing here is worth failing a session over: the + orchestrator's fallback for a missing event is ``session_timeout_min``, and an + attacker who can suppress the event can already get that outcome by planting + the redirect ``_write_event`` refuses. + """ + run_dir = os.environ.get("BMAD_LOOP_RUN_DIR") + task_id = os.environ.get("BMAD_LOOP_TASK_ID") + if not run_dir or not task_id: + return 0 + ts = time.time_ns() + event = shape_event(ts, event_name, task_id, _read_payload(stdin)) + try: + # The events dir is derived from the run dir here; #494 Phase 3 gives it + # its own env var and prefers that, keeping this as the fallback for + # hooks installed before the move. + _write_event( + os.path.join(run_dir, "events"), event_file_name(ts, task_id, event_name), event + ) + except OSError: + # A hostile or broken events dir must degrade to the orchestrator's normal + # session_timeout_min path, never surface as a hook failure that fails the + # CLI window (mirrors the hook script's own write wrap). + return 0 + return 0 diff --git a/tests/test_events.py b/tests/test_events.py new file mode 100644 index 00000000..4e590bbd --- /dev/null +++ b/tests/test_events.py @@ -0,0 +1,587 @@ +"""`events.py` — the importable twin of the stdlib-only hook relay, and the +`bmad-loop relay` command built on it. + +Two things are under test. The twin's *source parity* with +`data/bmad_loop_hook.py`, because two separately-maintained writers of one +control plane is how the hardening in one silently stops applying to the other. +And the twin's *behavior*, mirrored from the #493 hardening tests in +test_hook_script.py — a copy that is byte-identical today can still be edited on +both sides at once, and these are what say the shape still holds. +""" + +from __future__ import annotations + +import ast +import io +import json +import os +import stat +import subprocess +import sys +from pathlib import Path + +import pytest + +from bmad_loop import cli, events +from bmad_loop import policy as policy_mod + +HOOK = Path(events.__file__).resolve().parent / "data" / "bmad_loop_hook.py" +EVENTS = Path(events.__file__).resolve() + +# Everything `events.py` copies verbatim out of the hook script. The write path and +# the two helpers it stands on; the payload SHAPING is not here because the hook +# does it inline in its `main()` and there is no source segment to compare — it is +# pinned behaviorally instead, by test_relay_and_the_hook_shape_the_same_event. +TWINNED = ( + "_LINK_REPARSE_TAGS", + "_first_workspace", + "_is_link_like", + "_write_all", + "_write_event", +) + + +def _top_level_sources(path: Path) -> dict[str, str]: + """`name -> the exact source segment that defines it`, for the module's + top-level defs and plain assignments.""" + src = path.read_text(encoding="utf-8") + found: dict[str, str] = {} + for node in ast.parse(src).body: + if isinstance(node, ast.FunctionDef): + found[node.name] = ast.get_source_segment(src, node) or "" + elif isinstance(node, ast.Assign): + for target in node.targets: + if isinstance(target, ast.Name): + found[target.id] = ast.get_source_segment(src, node) or "" + return found + + +def _relay(event: str, payload, monkeypatch, run_dir: Path | None, task_id: str = "t1") -> int: + """Drive `bmad-loop relay ` in-process with `payload` on stdin.""" + if run_dir is None: + monkeypatch.delenv("BMAD_LOOP_RUN_DIR", raising=False) + monkeypatch.delenv("BMAD_LOOP_TASK_ID", raising=False) + else: + monkeypatch.setenv("BMAD_LOOP_RUN_DIR", str(run_dir)) + monkeypatch.setenv("BMAD_LOOP_TASK_ID", task_id) + text = payload if isinstance(payload, str) else json.dumps(payload) + monkeypatch.setattr(sys, "stdin", io.StringIO(text)) + return cli.main(["relay", event]) + + +# ------------------------------------------------------------------- parity + + +def test_the_twinned_source_is_identical(): + """The hook ships as package DATA and is stdlib-only by contract, so it cannot + import `events.py` and `events.py` cannot import it: the hardened write exists + twice on purpose. A fix applied to one copy and not the other leaves one writer + of the events control plane unhardened while every behavioral test stays green + — both copies pass their own suites either way. Compare the source directly. + + Ablation guard: change so much as a comment in either copy's `_write_event` + and this fails. The `missing` assertion is the second half: rename a twinned + function on one side and the comparison loop would otherwise have nothing to + compare and pass vacuously.""" + hook = _top_level_sources(HOOK) + twin = _top_level_sources(EVENTS) + + missing = {name: (name in hook, name in twin) for name in TWINNED} + assert all( + all(present) for present in missing.values() + ), f"a twinned name is gone from one side (name: in-hook, in-events): {missing}" + for name in TWINNED: + assert twin[name] == hook[name], ( + f"{name} has drifted between {HOOK.name} and {EVENTS.name} — " + f"fix both copies, or the two writers of the events control plane " + f"stop being hardened the same way" + ) + + +def test_relay_and_the_hook_shape_the_same_event(tmp_path, monkeypatch): + """The payload shaping and the event file name are the half of the twin that + `ast.get_source_segment` cannot reach — the hook does both inline in `main()`. + Pin them the only way left: feed one payload to both writers and compare what + lands. Every CLI's key spelling is in the payload, so a fallback dropped from + one side shows up as a null on that side only. + + Ablation guard: drop any `or payload.get(...)` arm from `events.shape_event`, + or reorder the event file name's fields, and this fails.""" + payload = { + "conversationId": "agy-3", + "transcriptPath": "/ws/transcript.jsonl", + "workspacePaths": ["/ws"], + } + hook_run, relay_run = tmp_path / "hook", tmp_path / "relay" + + proc = subprocess.run( + [sys.executable, str(HOOK), "Stop"], + input=json.dumps(payload), + env={ + "PATH": os.environ.get("PATH", ""), + "BMAD_LOOP_RUN_DIR": str(hook_run), + "BMAD_LOOP_TASK_ID": "1-1-a-dev-1", + }, + capture_output=True, + text=True, + timeout=30, + ) + assert proc.returncode == 0, proc.stderr + assert _relay("Stop", payload, monkeypatch, relay_run, task_id="1-1-a-dev-1") == 0 + + from_hook = next((hook_run / "events").glob("*.json")) + from_relay = next((relay_run / "events").glob("*.json")) + # `ts` is a timestamp and the name is built from it; compare the rest. + assert json.loads(from_relay.read_text()) | {"ts": 0} == ( + json.loads(from_hook.read_text()) | {"ts": 0} + ) + assert from_relay.name.split("-", 1)[1] == from_hook.name.split("-", 1)[1] + + +# -------------------------------------------------------- hardening (twinned) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_symlinked_events_dir_writes_nothing_and_exits_zero(tmp_path, monkeypatch, capsys): + """#461 (Low): a driven session can write inside the run dir, so it can plant + `events/` as a symlink and redirect the orchestrator's control-plane event + stream — swallowing the Stop signal and stalling the run to timeout. The relay + must refuse the link and degrade to a no-op, never write through it. + + This is the OUTCOME, which is what the operator has: rc 0, nothing said, + nothing anywhere. Two independent layers produce it on POSIX and deleting + either one alone leaves this green — the `_is_link_like` pre-check, and the + `O_NOFOLLOW` on the anchored dir open, which makes `os.open` of a symlinked + final component fail ELOOP on its own. So each layer is pinned where it is the + only thing standing: the pre-check by + `test_the_precheck_refuses_before_makedirs_on_the_fallback_path` (the fallback + has no dir open to lean on), and `O_NOFOLLOW`'s branch by + `test_the_anchored_branch_is_actually_taken_on_posix`. + + Ablation guard: delete the pre-`makedirs` `_is_link_like` refusal AND drop + `o_nofollow` from the `os.open(events_dir, …)` flags — with both layers gone + the payload lands in the attacker's directory and this fails. Verified; + removing only one does not redden it, which is why the two tests above + exist.""" + target = tmp_path / "attacker" + target.mkdir() + run_dir = tmp_path / "run" + run_dir.mkdir() + (run_dir / "events").symlink_to(target, target_is_directory=True) + + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir) == 0 + assert capsys.readouterr() == ("", "") + assert list(target.iterdir()) == [] + assert list((run_dir / "events").iterdir()) == [] + + +class _ReparseStat: + """Stand-in for the os.lstat() result of a Windows junction: a DIRECTORY + mode (which is why os.path.islink() answers False) carrying a reparse tag.""" + + st_mode = stat.S_IFDIR | 0o755 + st_reparse_tag = 0xA0000003 # IO_REPARSE_TAG_MOUNT_POINT + + +def test_is_link_like_refuses_a_reparse_tagged_dir(tmp_path, monkeypatch): + """A Windows directory junction is a reparse point but NOT a symlink, so + `os.path.islink` is False for it while `os.makedirs`/`os.open` follow it — + and `mklink /J` needs no elevation, unlike a directory symlink, so it is the + cheaper attack. The refusal keys on the reparse tag instead. That branch is + reachable only on Windows; drive its logic here so it is not shipped + unexercised (the `stat.IO_REPARSE_TAG_*` constants do not exist on POSIX, + hence the substituted tuple). + + Ablation guard: dropping the `st_reparse_tag` arm of `_is_link_like` makes + the last assertion fail.""" + plain = tmp_path / "events" + plain.mkdir() + assert events._is_link_like(plain) is False + + real_lstat = os.lstat + monkeypatch.setattr(events, "_LINK_REPARSE_TAGS", (_ReparseStat.st_reparse_tag,)) + monkeypatch.setattr( + os, + "lstat", + lambda p, *a, **k: _ReparseStat() if str(p) == str(plain) else real_lstat(p), + ) + assert events._is_link_like(plain) is True + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_the_precheck_refuses_before_makedirs_on_the_fallback_path(tmp_path, monkeypatch): + """What the pre-check is for, in the one place it is load-bearing. On POSIX the + anchored branch's `O_NOFOLLOW` dir open refuses a symlinked `events/` by + itself, so the pre-check's absence is invisible there. The Windows fallback has + no dir open at all: `O_NOFOLLOW` on the create applies to the temp file's own + (non-link) name, so the create happily resolves THROUGH the redirected + directory and the payload is written into the attacker's tree before the + post-write re-check unlinks it again. + + Hence the two assertions. The refusal is the pre-check's, identified by its + message rather than by "an OSError happened" — the post-write re-check raises + one too and would mask this entirely. And `os.makedirs` never runs, which is + the docstring's own claim (`makedirs(exist_ok=True)` `isdir()`-checks THROUGH + the link, so the refusal has to come before it) stated as behavior. + + Ablation guard: delete the pre-`makedirs` `_is_link_like` refusal and this + fails on both counts — the message becomes `redirected mid-write` and + `makedirs` has run.""" + target = tmp_path / "attacker" + target.mkdir() + events_dir = tmp_path / "events" + events_dir.symlink_to(target, target_is_directory=True) + + made = [] + real_makedirs = os.makedirs + monkeypatch.setattr(os, "supports_dir_fd", frozenset()) # force the fallback + monkeypatch.setattr(os, "makedirs", lambda p, **kw: (made.append(p), real_makedirs(p, **kw))[1]) + + with pytest.raises(OSError, match="refusing to write events into a redirected directory"): + events._write_event(str(events_dir), "1-t1-Stop.json", {"event": "Stop"}) + + assert made == [], "the refusal has to come BEFORE makedirs, which follows the link" + assert list(target.iterdir()) == [] + + +def test_fallback_refuses_a_redirect_that_appears_mid_write(tmp_path, monkeypatch): + """The Windows fallback has no dir_fd to anchor to, so it re-resolves + `events_dir` by path and a swap between the check and the create lands the + temp file in the attacker's directory. The post-write re-check catches a + swap that is still in place. Driven here with the dir_fd branch disabled, + because on POSIX that branch is always taken and the fallback would ship + unexercised. + + Ablation guard: deleting the post-write `_is_link_like` block makes this + fail — the event gets published instead of refused.""" + events_dir = tmp_path / "events" + calls = [] + real = events._is_link_like + + def swapped_after_the_check(path): + calls.append(path) + return len(calls) > 1 and real(path) is False # clean at check, dirty after + + monkeypatch.setattr(os, "supports_dir_fd", frozenset()) # force the fallback + monkeypatch.setattr(events, "_is_link_like", swapped_after_the_check) + + with pytest.raises(OSError, match="redirected mid-write"): + events._write_event(str(events_dir), "1-t1-Stop.json", {"event": "Stop", "task_id": "t1"}) + + assert len(calls) == 2 # the check ran on both sides of the write + assert list(events_dir.iterdir()) == [] # nothing published, no .tmp left behind + + +@pytest.mark.skipif(os.name == "nt", reason="dir_fd is implemented with the POSIX *at() calls") +def test_the_anchored_branch_is_actually_taken_on_posix(tmp_path, monkeypatch): + """The capability probe must resolve to True where the capability exists, or + the TOCTOU-closing layer ships dead while every other test stays green — the + `islink` refusal covers the same cases, so nothing else would redden. + + This is not hypothetical: `os.replace` is NOT in `os.supports_dir_fd` on + Linux (only `os.rename` is) even though it accepts src_dir_fd/dst_dir_fd, so + probing `os.replace` — the function the code used to call — made the branch + unreachable on every platform. Observe the anchoring behaviorally rather + than re-deriving the probe, so editing the probe reddens this. + + Ablation guard: change the probe to `os.replace` (or drop the branch) and + this fails; no other test notices.""" + real_open, real_supports = os.open, os.supports_dir_fd + # The premise, asserted rather than assumed: were a future CPython to drop + # rename from the set, the branch would go dead and this says so directly. + assert {real_open, os.rename} <= real_supports + anchored = [] + + def spy(path, flags, mode=0o777, *, dir_fd=None): + anchored.append(dir_fd) + if dir_fd is None: + return real_open(path, flags, mode) + return real_open(path, flags, mode, dir_fd=dir_fd) + + monkeypatch.setattr(os, "open", spy) + # The probe tests os.open by IDENTITY, so the spy has to be in the capability + # set too or the probe answers False and this measures the fallback instead + # of the branch it exists to pin — which is how it first went red. + monkeypatch.setattr(os, "supports_dir_fd", real_supports | {spy}) + events._write_event(str(tmp_path / "events"), "1-t1-Stop.json", {"event": "Stop"}) + monkeypatch.undo() + + assert any(fd is not None for fd in anchored), "the create was never anchored to a dir_fd" + + +@pytest.mark.parametrize("forced_fallback", [False, True]) +def test_a_short_os_write_still_publishes_the_whole_payload(tmp_path, monkeypatch, forced_fallback): + """`os.write` may write FEWER bytes than asked and just return the count. The + buffered `open()` this replaced looped internally; the raw fd needed for + O_NOFOLLOW/dir_fd does not. A truncated event file is not retried but LOST: + `SignalWatcher.poll` adds a name to `_consumed` before parsing it, so + malformed JSON is skipped and never re-read — the Stop signal is gone and the + run waits out `session_timeout_min`. + + Both branches are driven: the loop is shared, but the fallback is the only + path Windows takes and POSIX would otherwise never exercise it. + + Ablation guard: collapsing `_write_all` back to a single `os.write` makes + this fail. Nothing else in the suite catches that — a real `os.write` + returns the full count, so the normal-path tests pass either way.""" + events_dir = tmp_path / "events" + real_write = os.write + + def a_byte_at_a_time(fd, data): + return real_write(fd, bytes(data)[:1]) # a legal short write + + if forced_fallback: + monkeypatch.setattr(os, "supports_dir_fd", frozenset()) + event = {"event": "Stop", "task_id": "t1", "session_id": "s" * 300} + monkeypatch.setattr(os, "write", a_byte_at_a_time) + events._write_event(str(events_dir), "1-t1-Stop.json", event) + monkeypatch.undo() + + published = list(events_dir.glob("*.json")) + assert len(published) == 1 + assert json.loads(published[0].read_text()) == event + assert list(events_dir.glob("*.tmp")) == [] + + +def test_a_zero_length_write_raises_instead_of_spinning(tmp_path, monkeypatch): + """`_write_all` loops on short writes, so a descriptor that always accepts 0 + bytes would spin forever. Refuse instead: the caller degrades to a no-op and + the run takes the timeout path, which beats a hook process that never exits. + + Ablation guard: dropping the `written <= 0` arm hangs this test.""" + monkeypatch.setattr(os, "write", lambda fd, data: 0) + with pytest.raises(OSError, match="short write"): + events._write_event(str(tmp_path / "events"), "1-t1-Stop.json", {"event": "Stop"}) + + +@pytest.mark.skipif(os.name != "nt", reason="directory junctions are Windows-only") +def test_junctioned_events_dir_writes_nothing_and_exits_zero(tmp_path, monkeypatch, capsys): + """The Windows half of the symlink test — Windows CI is its only oracle, a + junction cannot be created on POSIX.""" + # If either tag constant were misnamed the tuple is empty and the refusal + # silently never fires. Assert it directly rather than inferring from below. + assert events._LINK_REPARSE_TAGS + + target = tmp_path / "attacker" + target.mkdir() + run_dir = tmp_path / "run" + run_dir.mkdir() + subprocess.run( + ["cmd", "/c", "mklink", "/J", str(run_dir / "events"), str(target)], + check=True, + capture_output=True, + ) + # The premise, asserted rather than assumed: were a future CPython to start + # reporting junctions as links, this says so directly instead of going green + # for the wrong reason. + assert os.path.islink(run_dir / "events") is False + + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir) == 0 + assert capsys.readouterr() == ("", "") + assert list(target.iterdir()) == [] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX file modes") +def test_event_file_mode_is_0600(tmp_path, monkeypatch): + """Event files carry the orchestrator's control plane; nothing but the + operator running the loop needs to read them (narrowed from the umask-derived + 0644 a plain `open()` produced). + + Ablation guard: drop the `0o600` argument from either `os.open` call and this + fails (the default 0o777 lands as 0o755 under the usual umask).""" + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, tmp_path) == 0 + written = next((tmp_path / "events").glob("*.json")) + assert stat.S_IMODE(written.stat().st_mode) == 0o600 + + +# ------------------------------------------------------------- the relay command + + +def test_relay_writes_the_event_and_says_nothing(tmp_path, monkeypatch, capsys): + """The happy path, and the stdout invariant that rides on every path with it: + the hosts parse hook stdout, so a single stray line there is a protocol + violation even when the event itself landed.""" + payload = { + "session_id": "abc-123", + "transcript_path": "/home/u/.claude/projects/x/abc-123.jsonl", + "cwd": "/proj", + } + assert _relay("Stop", payload, monkeypatch, tmp_path, task_id="1-1-a-dev-1") == 0 + assert capsys.readouterr() == ("", "") + + files = list((tmp_path / "events").glob("*.json")) + assert len(files) == 1 + assert "1-1-a-dev-1" in files[0].name and "Stop" in files[0].name + event = json.loads(files[0].read_text()) + assert event["event"] == "Stop" + assert event["task_id"] == "1-1-a-dev-1" + assert event["session_id"] == "abc-123" + assert event["transcript_path"].endswith("abc-123.jsonl") + assert not list((tmp_path / "events").glob("*.tmp")) + + +def test_relay_is_a_silent_noop_outside_a_driven_session(tmp_path, monkeypatch, capsys): + """An operator (or a stray hook config) can invoke `bmad-loop relay` in a + session bmad-loop never spawned. The session-protocol env is the detector, and + the answer is to write nothing and say nothing at rc 0 — a normal interactive + session must be unaffected by having the hooks installed.""" + monkeypatch.chdir(tmp_path) + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, None) == 0 + assert capsys.readouterr() == ("", "") + assert list(tmp_path.iterdir()) == [] + + +@pytest.mark.parametrize( + "garbage", + ["", " ", "{not json", "[1, 2, 3]", '"a bare string"', "null"], + ids=["empty", "blank", "truncated", "list", "string", "null"], +) +def test_relay_tolerates_garbage_on_stdin(tmp_path, monkeypatch, capsys, garbage): + """A hook that fires with nothing (or something non-dict) on stdin still has to + produce the event: the run's completion signal rides on the file, not on the + payload. Every unusable payload collapses to nulls, never to a refusal. + + Ablation guard: delete the `isinstance(payload, dict)` collapse and the + list/string/null rows fail on the `.get` that follows. Every row here is a + `json.JSONDecodeError`, so the rest of `_read_payload`'s `except` is pinned + separately by `test_relay_tolerates_an_unreadable_stdin`.""" + assert _relay("SessionEnd", garbage, monkeypatch, tmp_path) == 0 + assert capsys.readouterr() == ("", "") + files = list((tmp_path / "events").glob("*.json")) + assert len(files) == 1 + assert json.loads(files[0].read_text())["session_id"] is None + + +class _UnreadableStream: + """A stdin `json.load` cannot read. `json.load` calls `fp.read()`, so the + failure surfaces from there exactly as a real one would.""" + + def __init__(self, exc: BaseException): + self._exc = exc + + def read(self, *_a): + raise self._exc + + +@pytest.mark.parametrize( + "exc", + [ + OSError("broken pipe"), + UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte"), + ], + ids=["oserror", "undecodable"], +) +def test_relay_tolerates_an_unreadable_stdin(tmp_path, monkeypatch, capsys, exc): + """Beyond malformed JSON, the read itself can fail: the host can hand the hook + a closed descriptor, or bytes that are not valid UTF-8 (a transcript path from + a differently-encoded filesystem is the realistic source). Neither is + actionable and neither may cost the event — the run's completion signal rides + on the file landing. + + These are the two arms the malformed-JSON rows do NOT reach: + `UnicodeDecodeError` is a `ValueError` but not a `json.JSONDecodeError`, and + `OSError` is neither. + + Ablation guard: narrow `_read_payload`'s `except (ValueError, OSError)` to + `json.JSONDecodeError` and both rows fail — the exception escapes to + `cmd_relay`'s backstop, so no event is written and stderr is no longer + empty.""" + monkeypatch.setenv("BMAD_LOOP_RUN_DIR", str(tmp_path)) + monkeypatch.setenv("BMAD_LOOP_TASK_ID", "t1") + monkeypatch.setattr(sys, "stdin", _UnreadableStream(exc)) + + assert cli.main(["relay", "Stop"]) == 0 + assert capsys.readouterr() == ("", "") + files = list((tmp_path / "events").glob("*.json")) + assert len(files) == 1 + assert json.loads(files[0].read_text())["session_id"] is None + + +def test_relay_degrades_to_zero_when_the_write_fails(tmp_path, monkeypatch, capsys): + """Any OSError out of the write — a full disk, a read-only run dir, the + redirect refusals above — degrades to the orchestrator's normal + `session_timeout_min` path. A non-zero rc here is surfaced by several hosts as + a failed tool call inside the very session whose completion this reports. + + Ablation guard: delete the `except OSError` arm in `events.relay` and this + fails with the OSError escaping to the caller.""" + + def boom(*_a, **_k): + raise OSError("no space left on device") + + monkeypatch.setattr(events, "_write_event", boom) + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, tmp_path) == 0 + assert capsys.readouterr() == ("", "") + + +def test_relay_survives_an_unexpected_exception(tmp_path, monkeypatch, capsys): + """The backstop for a bug in this code path rather than a hostile events dir. + Same reason as the OSError arm — a hook that exits non-zero breaks the session + — but it reports on stderr, which the hosts do not parse, instead of failing + silently. + + Ablation guard: delete `cmd_relay`'s `except Exception` and this fails with + the RuntimeError escaping `main()`.""" + + def boom(*_a, **_k): + raise RuntimeError("a bug, not a hostile events dir") + + monkeypatch.setattr(events, "relay", boom) + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, tmp_path) == 0 + out, err = capsys.readouterr() + assert out == "" + assert "RuntimeError" in err + + +def test_relay_runs_without_the_mux_or_a_readable_policy(tmp_path, monkeypatch, capsys): + """`main()` configures the mux backend from policy before dispatch, for the + handlers that reach the mux without ever loading policy. Relay reaches neither, + and a project whose policy.toml is unparseable must still be able to report + that its session stopped — so relay dispatches ahead of that call entirely. + + Ablation guard: move the relay dispatch below `_configure_mux(_project(args))` + and this fails — `main()`'s `except PolicyError` arm prints `error: …` and + returns 1, which is exactly the CLI-window failure the hook contract forbids.""" + + def explode(_project): + raise policy_mod.PolicyError("policy.toml is not valid TOML") + + monkeypatch.setattr(cli, "_configure_mux", explode) + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, tmp_path) == 0 + assert capsys.readouterr() == ("", "") + assert len(list((tmp_path / "events").glob("*.json"))) == 1 + + +def test_relay_dispatches_outside_the_shared_error_handler(tmp_path, monkeypatch): + """The placement of the dispatch, stated directly rather than inferred: relay + is not wrapped by `main()`'s shared try/except, whose arms print `error: …` + and return 1 or 130. `cmd_relay` is total, so in production nothing reaches + this — the test forces the question by making the handler itself raise. + + Ablation guard: delete the early dispatch and this fails; the raise is caught + by `main()`'s typed arm and turned into rc 1.""" + + def explode(_args): + raise policy_mod.PolicyError("would be swallowed by main()'s handler") + + monkeypatch.setattr(cli, "cmd_relay", explode) + monkeypatch.setattr(sys, "stdin", io.StringIO("{}")) + with pytest.raises(policy_mod.PolicyError): + cli.main(["relay", "Stop"]) + + +def test_relay_defaults_the_event_name_like_the_hook(tmp_path, monkeypatch, capsys): + """The hook script reads `sys.argv[1] if len(sys.argv) > 1 else "Unknown"`, so + a misconfigured registration that forgets the event name still produces a file + the operator can see. argparse would otherwise turn that into a usage error at + rc 2, before any handler runs — nothing `cmd_relay` does could take it back.""" + assert _relay_no_event({"session_id": "s1"}, monkeypatch, tmp_path) == 0 + assert capsys.readouterr() == ("", "") + assert "Unknown" in next((tmp_path / "events").glob("*.json")).name + + +def _relay_no_event(payload, monkeypatch, run_dir: Path) -> int: + monkeypatch.setenv("BMAD_LOOP_RUN_DIR", str(run_dir)) + monkeypatch.setenv("BMAD_LOOP_TASK_ID", "t1") + monkeypatch.setattr(sys, "stdin", io.StringIO(json.dumps(payload))) + return cli.main(["relay"]) diff --git a/tests/test_portability_guard.py b/tests/test_portability_guard.py index bf4339df..36a4dc37 100644 --- a/tests/test_portability_guard.py +++ b/tests/test_portability_guard.py @@ -164,6 +164,16 @@ # producing side is what these readers consume, not a second source of truth. ENV_READ_ALLOW = { "envvars.py": tuple(REGISTRY_NAMES.values()), + # `events.py` is the ONE in-package entry here, and the "cannot import + # bmad_loop" justification above does not reach it — it obviously can. It is + # exempt as the importable PARITY TWIN of the stdlib-only hook relay: the same + # two session-protocol vars, read at the same point in the same protocol, by + # the code the hook config points at when it points at `bmad-loop relay` + # instead of the copied script. Routing one twin through `envvars` and leaving + # the other on `os.environ` would put the reads out of parity, and parity is + # what the AST test on those two files exists to keep. Family-scoped like the + # rest, so a core knob read inline here is still an offender. + "events.py": SESSION_PROTOCOL_ENV, "data/bmad_loop_hook.py": SESSION_PROTOCOL_ENV, "data/bmad_loop_probe_hook.py": SESSION_PROTOCOL_ENV, "data/plugins/unity/unity_cleanup.py": UNITY_ENV, From 11d10f8697edc1f74af201af40426cae64483fd4 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 16:58:58 -0700 Subject: [PATCH 02/11] feat(state): user-scoped state root for the run control plane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of moving `events/` out of the project tree (#494/#498): the root the channel relocates into, plus its accessors. Nothing consumes it yet. - `BMAD_LOOP_STATE_DIR` joins the envvars registry — the operator override, honoured as spelled (a stated override must not be swapped for a guess), with the empty string reading as unset since `Path("")` is the launch cwd. - `runs.state_root()` resolves the override, then `$XDG_STATE_HOME/bmad-loop` (absolute values only, per the XDG spec) or `~/.local/state/bmad-loop`, and on win32 `%LOCALAPPDATA%\bmad-loop\state` or `%USERPROFILE%\AppData\Local\...`. Never `Path.home()` there: it prefers HOMEDRIVE+HOMEPATH once USERPROFILE is absent, which on a domain-joined machine can put the control plane on a network share. Every derived base must be absolute and not the filesystem root; when none answers this raises `StateRootError` rather than degrading — a write path, and each degraded outcome is silent. - `state_dir_for()` keys `//`, reusing `project_tag` so a symlinked or relative spelling of one project cannot open a second control plane; `events_dir_for()` names the channel. - An autouse fixture points the root at a temp dir for every test — the one variable outranks the whole cascade — so no test writes into a real `~/.local/state` once phase 3 makes SignalWatcher mkdir it. --- CHANGELOG.md | 7 ++ README.md | 15 +-- src/bmad_loop/envvars.py | 34 ++++++- src/bmad_loop/runs.py | 144 ++++++++++++++++++++++++++- tests/conftest.py | 29 +++++- tests/test_envvars.py | 46 +++++++-- tests/test_runs.py | 206 ++++++++++++++++++++++++++++++++++++++- 7 files changed, 461 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e02b7ff5..db9e430a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,13 @@ whose seams had diverged enough that several ports needed a different fix, and t neither a broken `policy.toml` nor an unexpected exception can turn a session's Stop signal into a failed hook. First phase of moving `events/` to a control-plane root outside the project tree. + The root itself lands next: a user-scoped state directory (`$XDG_STATE_HOME/bmad-loop` or + `~/.local/state/bmad-loop`; `%LOCALAPPDATA%\bmad-loop\state` on Windows), keyed + `///` by the same project identity that scopes session ownership, so two + spellings of one project cannot end up with two control planes. `BMAD_LOOP_STATE_DIR` overrides + the whole cascade for a host where none of those is derivable or writable. Nothing reads the root + yet — the events channel moves into it in the next phase. + - **Coding-CLI adapter registry: a new adapter class ships out-of-tree (#226).** The transport axis has long been extensible out-of-tree; the CLI axis had no equivalent, so a CLI needing its own adapter _class_ forced a name-branch in the run bootstrap. A profile's new `adapter` field names a diff --git a/README.md b/README.md index 8a514aac..fa06f84f 100644 --- a/README.md +++ b/README.md @@ -563,13 +563,14 @@ For `per_worktree`, set `editor_mode = "per_worktree"` with `[scm] isolation = " ## Environment variables -A handful of `BMAD_LOOP_*` variables override behavior at runtime, taking precedence over the policy file. Most operators only ever touch `BMAD_LOOP_MUX_BACKEND`; the other two are override/test hooks. - -| Variable | Value | Effect | -| ----------------------------- | ---------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `BMAD_LOOP_MUX_BACKEND` | registered backend name (e.g. `tmux`, `psmux`) | Forces the terminal-multiplexer backend, outranking the `[mux] backend` policy key and auto-selection. A name matching no registered backend is an error — it never silently falls back. Unset ⇒ auto-select. | -| `BMAD_LOOP_PROCESS_HOST` | registered host name (e.g. `posix`, `windows`) | Forces the process-lifecycle host (an override/test hook). A name matching no registered host raises rather than silently using POSIX. Unset ⇒ this platform's default. | -| `BMAD_LOOP_SESSION_TIMEOUT_S` | seconds (float) | Overrides the per-session wall-clock budget (normally `limits.session_timeout_min × 60`) — mainly a test/E2E hook for sub-minute timeouts. A value that is not a finite positive number is ignored — non-positive, unparseable, or non-finite (`inf`, `1e999`), the last of which would otherwise disable the timeout outright. A large finite value is honoured. Unset ⇒ the policy value. | +A handful of `BMAD_LOOP_*` variables override behavior at runtime, taking precedence over the policy file. Most operators only ever touch `BMAD_LOOP_MUX_BACKEND` and, on a host with an unusual home directory, `BMAD_LOOP_STATE_DIR`; the rest are override/test hooks. + +| Variable | Value | Effect | +| ----------------------------- | ---------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `BMAD_LOOP_MUX_BACKEND` | registered backend name (e.g. `tmux`, `psmux`) | Forces the terminal-multiplexer backend, outranking the `[mux] backend` policy key and auto-selection. A name matching no registered backend is an error — it never silently falls back. Unset ⇒ auto-select. | +| `BMAD_LOOP_PROCESS_HOST` | registered host name (e.g. `posix`, `windows`) | Forces the process-lifecycle host (an override/test hook). A name matching no registered host raises rather than silently using POSIX. Unset ⇒ this platform's default. | +| `BMAD_LOOP_STATE_DIR` | directory path | Overrides the user-scoped **state root** — the out-of-tree home of per-run control-plane state, keyed `///`. Used as the root itself, so nothing is appended to it. Unset ⇒ `$XDG_STATE_HOME/bmad-loop` when that names an absolute path, else `~/.local/state/bmad-loop`; on Windows `%LOCALAPPDATA%\bmad-loop\state`, else `%USERPROFILE%\AppData\Local\bmad-loop\state`. Set this when none of those is derivable or writable (a home on a network share, a locked-down service account). | +| `BMAD_LOOP_SESSION_TIMEOUT_S` | seconds (float) | Overrides the per-session wall-clock budget (normally `limits.session_timeout_min × 60`) — mainly a test/E2E hook for sub-minute timeouts. A value that is not a finite positive number is ignored — non-positive, unparseable, or non-finite (`inf`, `1e999`), the last of which would otherwise disable the timeout outright. A large finite value is honoured. Unset ⇒ the policy value. | Game-engine (Unity) runs read a wider `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` set documented in the [Game Engine MCP guide](docs/game-engine-mcp-guide.md). diff --git a/src/bmad_loop/envvars.py b/src/bmad_loop/envvars.py index e67d1c02..e0902628 100644 --- a/src/bmad_loop/envvars.py +++ b/src/bmad_loop/envvars.py @@ -8,9 +8,14 @@ Each reader preserves its call site's exact semantics — same parse, same fallback — so routing a site through here changes nothing observable. Only the -three core vars belong here; the `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` -family read by the bundled Unity plugin's stand-alone helper scripts is that -plugin's own contract (documented in the game-engine guide) and stays with it. +core operator/test knobs belong here (the count is deliberately not stated — it +has already changed once); the `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` family +read by the bundled Unity plugin's stand-alone helper scripts is that plugin's +own contract (documented in the game-engine guide) and stays with it. Nor do the +session-protocol vars the engine *injects* into a child session +(`BMAD_LOOP_RUN_DIR`, `BMAD_LOOP_TASK_ID`, …): those have a producing side inside +the orchestrator, and the stdlib-only relays that read them back cannot import +this module at all. """ from __future__ import annotations @@ -24,6 +29,9 @@ MUX_BACKEND = "BMAD_LOOP_MUX_BACKEND" #: Forces the process-host implementation by registered name (test / override). PROCESS_HOST = "BMAD_LOOP_PROCESS_HOST" +#: Overrides the user-scoped state root that per-run control-plane state lives +#: under (see :func:`runs.state_root`), replacing the whole platform cascade. +STATE_DIR = "BMAD_LOOP_STATE_DIR" def session_timeout_s() -> float | None: @@ -69,3 +77,23 @@ def mux_backend() -> str | None: def process_host() -> str | None: """The forced process-host name, or ``None`` when unset.""" return os.environ.get(PROCESS_HOST) + + +def state_dir() -> str | None: + """The overriding bmad-loop state root, or ``None`` when unset. + + Verbatim like the two name readers above — :func:`runs.state_root` uses the + value as the state root itself, so an operator who names a directory gets that + directory, relative spelling included. Silently ignoring a stated override in + favour of the platform cascade would be the same failure + :func:`mux_backend` refuses: a loud misconfiguration turned into a quiet + auto-select, discoverable only by noticing where a run's events did *not* + appear. + + The one value not passed through is the empty string, which reads as unset. + ``export BMAD_LOOP_STATE_DIR=`` is what an unset-looking export leaves behind, + and it is not a directory an operator can have meant: ``Path("")`` is the + *current directory*, so honouring it would silently root the control plane at + whatever cwd the loop happened to be launched from. + """ + return os.environ.get(STATE_DIR) or None diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index e70150d4..a2cfd090 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -9,11 +9,12 @@ import re import secrets import shutil +import sys import tarfile import time from pathlib import Path -from . import devcontract, verify +from . import devcontract, envvars, verify from .adapters.multiplexer import MultiplexerError, get_multiplexer from .journal import STATE_FILE, Journal, load_state, save_state from .model import PAUSE_ESCALATION, Phase, RunState, StoryTask @@ -151,6 +152,147 @@ def attach_argv(run_id: str) -> list[str]: return attach_target_argv(session_target(run_id)) +# ------------------------------------------------------- user-scoped state root + + +class StateRootError(Exception): + """No user-scoped state root could be derived from this environment — every + candidate base was unset, empty, relative, or named the filesystem root. The + control plane has nowhere to live, and the caller must fail rather than guess + (see :func:`state_root`).""" + + +def _state_base(value: str | None) -> Path | None: + """``value`` as a usable base directory, or ``None`` when it cannot be one. + + The single rule every *derived* candidate below is held to, so the POSIX and + win32 branches cannot drift into judging their inputs differently. A base is + rejected when it is unset, empty, relative, or names the filesystem root + itself. The last three are the answers a broken environment gives *instead* of + raising, which is what makes them worth naming: + + - **empty**: ``os.path.expanduser("~")`` answers ``""`` on Windows for a + set-but-empty ``USERPROFILE``, and ``Path("")`` is the current directory. + - **relative**: including ``"~"`` itself, which is what ``expanduser`` returns + when it cannot expand at all. The state root would then move with the + launch cwd, and a run whose control plane it cannot find again is a run + that stalls to ``session_timeout_min`` rather than one that fails. + - **the root**: ``expanduser("~")`` answers ``"/"`` on POSIX for a set-but-empty + ``HOME`` (``posixpath`` folds the empty prefix to the root), which would put + ``/.local/state/bmad-loop`` on the filesystem root — a permission error for + an ordinary user and, for a containerised root, a silent write to ``/``. + ``base == base.parent`` is the root test on both flavours. + + ``os.path.isabs`` rather than :func:`platform_util.is_absolute_path`: the + latter is purpose-built for "must stay inside the project" guards and is + strictly broader — it calls the drive-*relative* ``C:foo`` absolute, which is + exactly the value that must not become a state root. The question here is the + platform's own, and each branch below only ever runs on its own platform. + """ + if not value or not os.path.isabs(value): + return None + base = Path(value) + return None if base == base.parent else base + + +def state_root() -> Path: + """The bmad-loop state root for this user: the out-of-tree home of per-run + control-plane state — the events channel (#494) and, later, the config digest + (#498). Outside the project tree because a branch switch, a worktree mount or + a rollback must not be able to take a live run's control plane away. + + Resolution, first answer wins: + + 1. ``BMAD_LOOP_STATE_DIR``, used as the state root **itself** — no + ``bmad-loop`` segment is appended, because the variable names our root + rather than a base to build one under. It is honoured as spelled (see + :func:`envvars.state_dir`), so it is the one candidate ``_state_base`` does + not filter: skipping a stated override would be a silent countermand, where + skipping a *derived* base only moves on to the next guess. + 2. POSIX — ``$XDG_STATE_HOME/bmad-loop`` when that variable names an absolute + path, else ``~/.local/state/bmad-loop``. A relative ``XDG_STATE_HOME`` is + *ignored*, which the XDG base-directory spec requires of its consumers. + (``install._shield_inherited_excludes`` resolves a relative + ``XDG_CONFIG_HOME`` instead of ignoring it — the opposite call for the + opposite reason: there we reproduce *git's* reading of the variable, here + we are the spec's own consumer.) + 3. win32 — ``%LOCALAPPDATA%\\bmad-loop\\state``, else + ``%USERPROFILE%\\AppData\\Local\\bmad-loop\\state``. ``LOCALAPPDATA`` names + the per-user, per-machine, non-roaming store Windows intends for exactly + this, and the second form is its documented default location. + + **Never** ``Path.home()`` on the win32 arm. It is ``ntpath.expanduser("~")``, + which prefers ``USERPROFILE`` and then falls back to ``HOMEDRIVE`` + + ``HOMEPATH`` — a pair that on a domain-joined machine may name a network home + share. A control plane whose atomic renames and ``O_NOFOLLOW``-anchored writes + live on an SMB share is not the local directory this needs, and the derivation + also disagrees with the one git uses for its own ``$HOME`` + (``install._shield_home_git_ignore`` documents that split in full). Reading + ``LOCALAPPDATA``/``USERPROFILE`` directly asks for the store by name instead of + inferring it from a home. + + Raises :class:`StateRootError` when no candidate answers. This is a write + path, so it raises rather than degrading to a plausible-looking default: + ``platform_util.resolve_or_lexical`` states the doctrine (observation may + degrade, repair writes must raise), and the degraded outcomes here are all + silent — a control plane at the cwd, or at ``/``, that the *next* process to + ask resolves somewhere else. + """ + override = envvars.state_dir() + if override: + return Path(override) + if sys.platform == "win32": + local = _state_base(os.environ.get("LOCALAPPDATA")) + if local: + return local / "bmad-loop" / "state" + profile = _state_base(os.environ.get("USERPROFILE")) + if profile: + return profile / "AppData" / "Local" / "bmad-loop" / "state" + else: + xdg = _state_base(os.environ.get("XDG_STATE_HOME")) + if xdg: + return xdg / "bmad-loop" + home = _state_base(os.path.expanduser("~")) + if home: + return home / ".local" / "state" / "bmad-loop" + raise StateRootError( + "cannot locate a state directory for bmad-loop's run control plane: " + + ( + "neither %LOCALAPPDATA% nor %USERPROFILE% names an absolute directory" + if sys.platform == "win32" + else "neither $XDG_STATE_HOME nor $HOME names an absolute directory" + ) + + f" — set {envvars.STATE_DIR} to the directory it should live in" + ) + + +def state_dir_for(project: Path, run_id: str) -> Path: + """This run's control-plane directory: ``//``. + + The project key is :func:`project_tag`, reused verbatim rather than re-derived: + it already resolves the project before digesting it, so the two spellings of + one project a caller can arrive with — a symlinked path, a relative one — key + to the same directory. They must, or a run started through one spelling would + write its events where a poll through the other never looks, and the run would + wait out ``session_timeout_min`` with the completion signal sitting on disk. + Its ``resolve()`` raising on a project the OS cannot canonicalize is correct + here for the same reason: an unknowable location cannot be keyed at all, and + guessing one is the wrong-directory write the tag exists to prevent. + + ``run_id`` needs no sanitizing — the id contract (see :data:`RUN_ID_RE`) is + already "a legal path segment on every platform", pinned by + :func:`is_valid_run_id`, and an id from outside is rejected there rather than + coerced here. + """ + return state_root() / project_tag(project) / run_id + + +def events_dir_for(project: Path, run_id: str) -> Path: + """The run's hook-event channel: the directory the relay writes a session's + events into and ``SignalWatcher`` polls for them.""" + return state_dir_for(project, run_id) / "events" + + # ---------------------------------------------------- run resolution / liveness diff --git a/tests/conftest.py b/tests/conftest.py index 429285e7..a0f29e2d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,7 +13,7 @@ import pytest import yaml -from bmad_loop import cli, documents, platform_util +from bmad_loop import cli, documents, envvars, platform_util from bmad_loop.adapters.base import SessionResult, SessionSpec from bmad_loop.bmadconfig import ProjectPaths from bmad_loop.checks import ValidationReport @@ -283,6 +283,33 @@ def _isolate_ambient_git_ignores(tmp_path_factory: pytest.TempPathFactory): mp.undo() +@pytest.fixture(autouse=True) +def _isolate_state_root(tmp_path_factory: pytest.TempPathFactory, monkeypatch): + """Point the user-scoped state root at a per-test temp dir, for every test. + + `runs.state_root()` resolves to `~/.local/state/bmad-loop` (POSIX) or + `%LOCALAPPDATA%\\bmad-loop\\state` (win32) when nothing overrides it, and the + run control plane does not merely *read* that location — it mkdirs into it. + Without this every test that constructs an adapter would write into the + developer's (or the CI runner's) real state directory and leave it there, one + stray tree per run id, on a path no fixture cleans up. + + One variable is enough: `BMAD_LOOP_STATE_DIR` is checked before the platform + cascade and outranks all of it, so this cannot be defeated by whatever + XDG/LOCALAPPDATA the host happens to export. + + Deliberately NOT a blanket reset of HOME / USERPROFILE / LOCALAPPDATA / + XDG_STATE_HOME. Those are not ours: git reads HOME (`install`'s shield + probes it), the coding CLIs discover their own config under them, and + `sanitize.redact_home` measures against the real one — shadowing them + suite-wide would change what unrelated tests measure, which is the argument + `_isolate_ambient_git_ignores` makes for shadowing only the two it must. + Tests that grade the cascade itself `delenv` this variable and monkeypatch + the ones they need, and share this fixture's monkeypatch instance, so the + override comes off cleanly for exactly that test.""" + monkeypatch.setenv(envvars.STATE_DIR, str(tmp_path_factory.mktemp("state-root"))) + + @pytest.fixture(scope="session") def _project_template( tmp_path_factory: pytest.TempPathFactory, _isolate_ambient_git_ignores: None diff --git a/tests/test_envvars.py b/tests/test_envvars.py index 8907e05b..93caa81f 100644 --- a/tests/test_envvars.py +++ b/tests/test_envvars.py @@ -1,12 +1,15 @@ -"""Registry tests for the three core `BMAD_LOOP_*` runtime overrides. +"""Registry tests for the core `BMAD_LOOP_*` runtime overrides. `envvars` is the one place each core var is named, typed, and given a reader, so what is pinned here is the *contract* the call sites (`engine`, -`adapters.multiplexer`, `cli`, `process_host`) and the README's "Environment -variables" table both depend on: the literal names, and each reader's parse and -fallback. The two name readers pass their value through verbatim on purpose — -validation lives downstream in the registry that resolves the name — so a test -asserting rejection here would be asserting the wrong module's job. +`adapters.multiplexer`, `cli`, `process_host`, `runs`) and the README's +"Environment variables" table both depend on: the literal names, and each +reader's parse and fallback. The two name readers pass their value through +verbatim on purpose — validation lives downstream in the registry that resolves +the name — so a test asserting rejection here would be asserting the wrong +module's job. `state_dir` is verbatim for a related reason (a stated override +must not be silently swapped for a guess) with one exception, the empty string, +which is graded here because `runs.state_root` trusts what this reader returns. Contract parity: `test_engine.py::test_session_timeout_s_env_override` pins the same rejection set one layer up, through `Engine._session_timeout_s` (does the @@ -33,6 +36,7 @@ def test_constants_are_the_literal_env_var_names(): assert envvars.SESSION_TIMEOUT_S == "BMAD_LOOP_SESSION_TIMEOUT_S" assert envvars.MUX_BACKEND == "BMAD_LOOP_MUX_BACKEND" assert envvars.PROCESS_HOST == "BMAD_LOOP_PROCESS_HOST" + assert envvars.STATE_DIR == "BMAD_LOOP_STATE_DIR" def test_session_timeout_s_is_none_when_unset(monkeypatch): @@ -154,3 +158,33 @@ def test_process_host_is_a_verbatim_passthrough(monkeypatch): monkeypatch.setenv(envvars.PROCESS_HOST, "bogus") assert envvars.process_host() == "bogus" + + +def test_state_dir_is_a_verbatim_passthrough_except_for_the_empty_string(monkeypatch): + """`runs.state_root` uses this value as the state root itself, so the reader + hands back what the operator wrote — a relative spelling included. Filtering it + would put the loop in the position `mux_backend` refuses: an override the + operator can see they exported, silently swapped for the platform guess, and + detectable only by noticing where a run's events did *not* appear. + + The empty string is the exception, and it is a value this reader must hold + rather than the caller: `export BMAD_LOOP_STATE_DIR=` is what an unset-looking + export leaves behind, `Path("")` is the *current directory*, and rooting the + control plane at the launch cwd is neither what was meant nor something a + later process would resolve the same way. Reading it as unset falls through to + the platform cascade, which is the same thing not exporting it at all does. + + Ablation target: spell the reader `os.environ.get(STATE_DIR)` and only the + empty row fails — the unset and value rows pass under it, so neither pins the + rule on its own.""" + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + assert envvars.state_dir() is None + + monkeypatch.setenv(envvars.STATE_DIR, "") + assert envvars.state_dir() is None + + monkeypatch.setenv(envvars.STATE_DIR, "/var/lib/bmad-loop") + assert envvars.state_dir() == "/var/lib/bmad-loop" + + monkeypatch.setenv(envvars.STATE_DIR, "relative/state") + assert envvars.state_dir() == "relative/state" diff --git a/tests/test_runs.py b/tests/test_runs.py index f20c7ef8..bf85bf5a 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -6,11 +6,12 @@ import subprocess import sys import tarfile +from pathlib import Path import pytest -from conftest import escalated_run, git +from conftest import escalated_run, git, refuse_to_resolve -from bmad_loop import platform_util, runs, verify +from bmad_loop import envvars, platform_util, runs, verify from bmad_loop.adapters import tmux_base from bmad_loop.adapters.multiplexer import MultiplexerError from bmad_loop.adapters.psmux_backend import PsmuxMultiplexer @@ -727,6 +728,207 @@ def test_project_tag_is_transportable_whatever_the_path(tmp_path): assert len({tag, runs.project_tag(tmp_path / "other")}) == 2 +# ------------------------------------------------- user-scoped state root (#494) +# +# Every row here clears the suite-wide `_isolate_state_root` override first: that +# fixture exists so no test writes into the real state directory, and it is the +# first thing `state_root` consults, so a cascade row that left it set would grade +# nothing. `sys.platform` is faked per branch (the house idiom — see +# test_journal.py), and the fake home is written to HOME *and* USERPROFILE because +# `expanduser` reads the first on POSIX and the second on Windows, so a row must +# set both to mean the same thing on either host (tests/test_diagnostics.py:630). + + +def _fake_home(monkeypatch, home) -> None: + """Point `expanduser("~")` at `home` on whichever host is running, and clear + everything else `state_root` would answer from first.""" + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + monkeypatch.delenv("XDG_STATE_HOME", raising=False) + monkeypatch.delenv("LOCALAPPDATA", raising=False) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.setenv("USERPROFILE", str(home)) + + +def test_state_root_precedence_override_then_xdg_then_home(tmp_path, monkeypatch): + """Three answers, ranked, and the ranking is what each assertion removes. + + The override outranks a *set* XDG_STATE_HOME and not merely an unset one — + graded by leaving XDG set throughout — because it is the operator's stated + answer and the suite's own isolation depends on it winning against whatever a + host exports. + + The asymmetry in the middle is deliberate and pinned here rather than left to + the reader: `XDG_STATE_HOME` is a *base* to build under, so `bmad-loop` is + appended to it, while `BMAD_LOOP_STATE_DIR` names our root itself and is used + as spelled. Appending to the override would silently move every path a + phase-3 hook computes from that same variable.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + home = tmp_path / "home" + _fake_home(monkeypatch, home) + monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "xdg")) + monkeypatch.setenv(envvars.STATE_DIR, str(tmp_path / "override")) + + assert runs.state_root() == tmp_path / "override" # verbatim: no "bmad-loop" tail + + monkeypatch.delenv(envvars.STATE_DIR) + assert runs.state_root() == tmp_path / "xdg" / "bmad-loop" + + monkeypatch.delenv("XDG_STATE_HOME") + assert runs.state_root() == home / ".local" / "state" / "bmad-loop" + + +@pytest.mark.parametrize("value", ["state", "./state", "~/state", ""], ids=repr) +def test_state_root_ignores_an_xdg_state_home_that_is_not_absolute(tmp_path, monkeypatch, value): + """The XDG base-directory spec says a relative value "must be ignored", and + ignoring it means falling through to the home default — not resolving it + against the cwd, which is what `Path(value) / "bmad-loop"` would do. + + That distinction is the whole test: a cwd-relative control plane is not a + failure anyone sees, it is a run whose events land somewhere the next process + to ask does not look, and a run that finds no completion signal waits out + `session_timeout_min`. `~/state` is here because expansion is not this + reader's job either — nothing expands it, so it stays relative. The empty + string is the same rule reached from the other side: set-but-empty is how an + unset-looking export reads, and `Path("")` is the cwd. + + Ablation target: drop the `os.path.isabs` half of `_state_base` and the three + relative rows fail together, each on a cwd-relative root. The empty row is + held by BOTH halves — `os.path.isabs("")` is already False — so no single + ablation reddens it here, and it is kept as the spelling an operator produces + rather than as an independent gate. What the emptiness half holds alone is the + *unset* variable, where `os.path.isabs(None)` raises; the refusal rows below + are what grade it.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + home = tmp_path / "home" + _fake_home(monkeypatch, home) + monkeypatch.setenv("XDG_STATE_HOME", value) + + assert runs.state_root() == home / ".local" / "state" / "bmad-loop" + + +def test_state_root_on_win32_prefers_localappdata_over_the_user_profile(tmp_path, monkeypatch): + """Windows keeps this class of per-user, per-machine state under + `%LOCALAPPDATA%`, and `%USERPROFILE%\\AppData\\Local` is that variable's + documented default location — the fallback, not a second opinion. + + XDG_STATE_HOME stays set across both assertions: it is a POSIX variable, and a + branch that consulted it on Windows would answer from an operator's WSL or + MSYS environment instead of the local store.""" + monkeypatch.setattr(runs.sys, "platform", "win32") + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "xdg")) + monkeypatch.setenv("LOCALAPPDATA", str(tmp_path / "local")) + monkeypatch.setenv("USERPROFILE", str(tmp_path / "profile")) + + assert runs.state_root() == tmp_path / "local" / "bmad-loop" / "state" + + monkeypatch.delenv("LOCALAPPDATA") + expected = tmp_path / "profile" / "AppData" / "Local" / "bmad-loop" / "state" + assert runs.state_root() == expected + + +def test_state_root_on_win32_refuses_a_home_derived_from_homedrive(tmp_path, monkeypatch): + """With neither `%LOCALAPPDATA%` nor `%USERPROFILE%` set, refuse — do not fall + back to a home directory. + + `Path.home()` is `ntpath.expanduser("~")`, which prefers `USERPROFILE` and then + `HOMEDRIVE` + `HOMEPATH`; on a domain-joined machine that pair can name a + network home share, and the control plane's `O_NOFOLLOW`-anchored writes and + atomic renames are not something to move onto SMB by inference. So the + variables are read by name and an absent store raises. + + Ablation target: replace the `USERPROFILE` read with `Path.home()` and this + row fails — on Windows it derives `Z:\\users\\x`, and on a POSIX host running + the faked branch `expanduser` falls back to the passwd entry. Both answer + where the guard refuses to.""" + monkeypatch.setattr(runs.sys, "platform", "win32") + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + monkeypatch.delenv("LOCALAPPDATA", raising=False) + monkeypatch.delenv("USERPROFILE", raising=False) + monkeypatch.delenv("HOME", raising=False) + monkeypatch.setenv("HOMEDRIVE", "Z:") + monkeypatch.setenv("HOMEPATH", "\\users\\x") + + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.state_root() + + +@pytest.mark.parametrize("home", ["", "relative-home", "~"], ids=repr) +def test_state_root_refuses_a_home_that_cannot_root_a_control_plane(monkeypatch, home): + """A write path fails loud rather than picking a plausible-looking directory. + + Each row is an answer `expanduser` really gives: `""` for a set-but-empty + `USERPROFILE` on Windows — and, on POSIX, `"/"`, since `posixpath` folds the + empty prefix to the root; `"relative-home"` for a `HOME` that is not a path at + all; and `"~"` for the input handed back when nothing can expand it. All three + would otherwise mkdir the control plane somewhere silently wrong — the launch + cwd, or `/.local/state`, which is a permission error for an ordinary user and + a real write to `/` for a containerised root. + + The message names the override, because that is the one remedy an operator + always has.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + _fake_home(monkeypatch, home) + + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.state_root() + + +def test_state_dir_for_is_keyed_on_project_identity_not_spelling(tmp_path, monkeypatch): + """One project reached by two spellings must key to ONE control plane. + + It is the same requirement `project_tag` was written for, and it reaches here + because the run and the hook that signals its completion can arrive with + different spellings of the project: the engine holds a resolved path, a relay + computes from what it was handed. Two keys would mean the poller watching one + directory while the events land in the other — no completion signal, and a run + that waits out `session_timeout_min` with the signal sitting on disk. + + Distinctness is asserted alongside identity: a key that collapsed every + project would satisfy the first half by making all runs share one plane.""" + project = tmp_path / "proj" + project.mkdir() + monkeypatch.chdir(tmp_path) + + absolute = runs.state_dir_for(project, "20260812-101500-ab12") + relative = runs.state_dir_for(Path("proj"), "20260812-101500-ab12") + assert absolute == relative + assert absolute == runs.state_root() / runs.project_tag(project) / "20260812-101500-ab12" + assert runs.events_dir_for(project, "20260812-101500-ab12") == absolute / "events" + + assert runs.state_dir_for(project, "20260812-101500-cd34") != absolute + assert runs.state_dir_for(tmp_path / "other", "20260812-101500-ab12") != absolute + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_state_dir_for_follows_a_symlinked_project_to_one_key(tmp_path): + """The symlink half of the spelling problem, and the one a lexical comparison + of the two paths would never catch — they share no component.""" + project = tmp_path / "proj" + project.mkdir() + link = tmp_path / "link" + link.symlink_to(project) + + assert runs.state_dir_for(link, "r1") == runs.state_dir_for(project, "r1") + + +def test_state_dir_for_raises_when_the_project_cannot_be_canonicalized(tmp_path, monkeypatch): + """A project the OS refuses to canonicalize (#552: a registered-but-not-serving + WSL UNC provider) has no knowable identity, so it gets no key. + + `project_tag`'s bare `resolve()` raising is the correct behaviour to inherit + rather than soften. Degrading to the lexical spelling would hand two spellings + of the one project two different control planes — the failure the test above + exists to prevent — and this is a write path, where the doctrine is to raise + (`platform_util.resolve_or_lexical`).""" + project = tmp_path / "proj" + project.mkdir() + refuse_to_resolve(monkeypatch, project) + + with pytest.raises(OSError): + runs.state_dir_for(project, "r1") + + def test_prunable_sessions_accepts_legacy_path_tag(tmp_path, monkeypatch): """A pre-digest tag stays ours; another project's path or digest stays foreign.""" legacy = str(tmp_path.resolve()) From a2ac5fbb85b0cfc49b5d876557cc7c2770d7fd6b Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 17:28:14 -0700 Subject: [PATCH 03/11] feat(events): relocate the hook-event channel out of the project tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 of #494/#498: the channel actually moves to `///events/`, with a version-skew guard on both sides so an already installed relay keeps completing sessions. The skew is the whole risk. `bmad_loop_hook.py` is COPIED into a target project by `init`, so an upgraded orchestrator routinely drives sessions whose relay knows only the legacy location. Old relay + new orchestrator with no fallback = no Stop is ever observed = every session stalls to `session_timeout_min`, which fails no test suite and hangs an overnight run. - Producer: `Engine._run_session` exports `BMAD_LOOP_EVENTS_DIR`. That env dict is the ONE required site — dev/review, sweep bundles, stories and injected plugin-workflow sessions all dispatch through it. The non-sites fail closed without it: `resolve.py` sets no `BMAD_LOOP_TASK_ID` (the relay no-ops, so an interactive resolve produces no events), `probe.py` captures through `BMAD_LOOP_PROBE_CAPTURE_DIR` and its own probe relay, `plugins/bus.py` spawns plain shells with no task id, and the Unity helpers are not CLI sessions. - Relays: both the copied hook script and `bmad-loop relay` prefer that variable and fall back to `/events`. `or`, not a presence test — an exported-but-empty value names the launch cwd. The var is deliberately NOT part of the no-op detector: the mirror-image skew (older orchestrator, newer relay) must still write its events. - Consumer: `SignalWatcher` takes an optional `legacy_dir` and polls primary then legacy, ordering by the parsed `ts` across both. `_consumed` keys on (dir, name) so a name consumed in one dir cannot mask a different event in the other. Only the primary is created — re-creating the in-tree dir would undo the move for the operator's `git status` and gain nothing. A missing legacy dir is the ordinary case and is skipped; a missing primary still raises. - Wiring: `runsetup.make_adapters` resolves `events_dir_for(project, run_id)` and hands it to every kind. It is the only layer holding both halves of the key — an adapter sees a run dir and nothing else, and `run_dir.parents[2]` is a shape real run dirs have and test run dirs do not. Handed to every family rather than gated like `mux`: a path probes no host and can refuse no run. Tests: dual-poll unit rows (legacy-only event seen, cross-dir ts ordering, missing legacy tolerated, primary-only mkdir, colliding names both delivered); hook + relay env preference and fallback, with #493's symlink refusal re-pinned against the env-directed dir; producer/consumer agreement pinned on both sides; the tmux and stories E2E fakes now write to `$BMAD_LOOP_EVENTS_DIR`, each keeping a legacy-location twin as an end-to-end skew regression. Every new guard was ablated singly (hook side and watcher side separately), requiring rc 1. --- CHANGELOG.md | 16 +++- README.md | 4 +- docs/FEATURES.md | 3 +- src/bmad_loop/adapters/generic.py | 15 +++- src/bmad_loop/adapters/opencode_http.py | 7 ++ src/bmad_loop/cli.py | 22 +++++ src/bmad_loop/data/bmad_loop_hook.py | 23 ++++- src/bmad_loop/engine.py | 17 +++- src/bmad_loop/events.py | 15 ++-- src/bmad_loop/runsetup.py | 14 ++- src/bmad_loop/signals.py | 108 ++++++++++++++++++------ src/bmad_loop/worktree_flow.py | 8 +- tests/test_adapter_registry.py | 64 ++++++++++++++ tests/test_engine.py | 32 +++++++ tests/test_events.py | 81 +++++++++++++++++- tests/test_generic_tmux.py | 78 +++++++++++++++-- tests/test_hook_script.py | 79 +++++++++++++++++ tests/test_multiplexer.py | 3 + tests/test_portability_guard.py | 3 +- tests/test_signals.py | 90 ++++++++++++++++++++ tests/test_stories_e2e.py | 65 +++++++++++--- 21 files changed, 686 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db9e430a..974261cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,12 +23,22 @@ whose seams had diverged enough that several ports needed a different fix, and t neither a broken `policy.toml` nor an unexpected exception can turn a session's Stop signal into a failed hook. First phase of moving `events/` to a control-plane root outside the project tree. - The root itself lands next: a user-scoped state directory (`$XDG_STATE_HOME/bmad-loop` or + The root itself is a user-scoped state directory (`$XDG_STATE_HOME/bmad-loop` or `~/.local/state/bmad-loop`; `%LOCALAPPDATA%\bmad-loop\state` on Windows), keyed `///` by the same project identity that scopes session ownership, so two spellings of one project cannot end up with two control planes. `BMAD_LOOP_STATE_DIR` overrides - the whole cascade for a host where none of those is derivable or writable. Nothing reads the root - yet — the events channel moves into it in the next phase. + the whole cascade for a host where none of those is derivable or writable. + + **The events channel now lives there**, at `///events/` — out of the + project tree, where a branch switch, a worktree mount or a rollback cannot take a live run's + control plane away. Every engine-driven session is told the directory through + `BMAD_LOOP_EVENTS_DIR`, and both relays (the copied hook script and `bmad-loop relay`) prefer it, + falling back to the legacy in-tree `/events`. The orchestrator keeps polling that legacy + location too, and the fallback pair is load-bearing rather than tidy: the hook script is COPIED + into the project by `init`, so an upgraded orchestrator routinely drives sessions whose relay + predates the move — without both halves every such session would observe no Stop and stall to + `session_timeout_min`. Re-run `bmad-loop init` to refresh the relay. `--dry-run` previews the + directory the run would use. - **Coding-CLI adapter registry: a new adapter class ships out-of-tree (#226).** The transport axis has long been extensible out-of-tree; the CLI axis had no equivalent, so a CLI needing its own diff --git a/README.md b/README.md index fa06f84f..f38757da 100644 --- a/README.md +++ b/README.md @@ -576,7 +576,9 @@ Game-engine (Unity) runs read a wider `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` ## Run state -Everything about a run lives in `.bmad-loop/runs//` (gitignored): `state.json` (resumable engine state), `journal.jsonl` (every decision), `events/` (hook signals), `tasks//` (per-session prompt + result + escalations, plus diagnostic breadcrumbs — `session-lifecycle.jsonl` records when a timeout fired, `heartbeat.json` is the wait loop's proof-of-life, `resultless-stops.jsonl` records give-up Stops), `logs/` (raw pane output, debugging only), `deferred/` (stashed specs from deferred stories), `resolve//` (escalation `context.json` + the resolve agent's `resolution.json`), `ATTENTION` (human-readable alerts), and — only while a graceful stop is pending — `stop-request.json` (the control file the engine consumes at the next item boundary). +Everything about a run lives in `.bmad-loop/runs//` (gitignored): `state.json` (resumable engine state), `journal.jsonl` (every decision), `tasks//` (per-session prompt + result + escalations, plus diagnostic breadcrumbs — `session-lifecycle.jsonl` records when a timeout fired, `heartbeat.json` is the wait loop's proof-of-life, `resultless-stops.jsonl` records give-up Stops), `logs/` (raw pane output, debugging only), `deferred/` (stashed specs from deferred stories), `resolve//` (escalation `context.json` + the resolve agent's `resolution.json`), `ATTENTION` (human-readable alerts), and — only while a graceful stop is pending — `stop-request.json` (the control file the engine consumes at the next item boundary). + +One piece deliberately lives elsewhere: the **hook-event channel** (the session completion signals the orchestrator waits on) sits under the user-scoped state root at `///events/`, outside the project tree — a branch switch, a worktree mount or a rollback must not be able to take a live run's control plane away. See `BMAD_LOOP_STATE_DIR` above for where that root resolves. The orchestrator also keeps polling the old in-tree `events/` location, so a project whose installed hook relay predates the move still completes its sessions; re-run `bmad-loop init` to refresh the relay. A run can be stopped two ways. A **hard stop** (`bmad-loop stop`, TUI `x`, Ctrl+C) SIGTERMs the engine mid-item and always kills the agent session. A **graceful stop** (`bmad-loop stop --graceful`, TUI `S`) instead writes `stop-request.json` — no signal, so it works on every platform and multiplexer backend — which the engine consumes at the next item boundary: the in-flight story (or sweep bundle) finishes through commit — or, mid-triage, the sweep's triage session completes and no bundles start — the run finalizes as `stopped` (not `finished`) and stays resumable, and pending auto-sweeps are suppressed. Its session teardown follows `cleanup_session_on_finish` like a normal finish, rather than the hard stop's unconditional kill; a hard stop always supersedes a pending graceful request. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 087e2df6..32a08463 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -102,12 +102,13 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Every run is a resumable on-disk state machine: `bmad-loop resume ` continues from a gate, escalation, or interruption. - A graceful stop (`stop --graceful` / TUI `S`) is resumable too: unlike a hard stop killed mid-item, it lets the in-flight item finish through commit and finalizes cleanly, ending as a `stopped` run that `resume` picks up at the next item. -- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `events/` (hook signals); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194), a mux session lost under the run (`session-vanished`, #489) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`; `ctl-window` (the control-session window id the last TUI launch minted, so attach/stop follow the live window, #482). +- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194), a mux session lost under the run (`session-vanished`, #489) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`; `ctl-window` (the control-session window id the last TUI launch minted, so attach/stop follow the live window, #482). - `journal.jsonl` records `session-end` for every session unconditionally — even a teardown that throws still lands one (status `aborted` when the outcome is unknowable). A timed-out session's entry carries `fired_at` (wall time the deadline was declared), `teardown_s` (wall seconds from that fire to this entry — the teardown gap), and `expired_clock` (`monotonic` / `wall` / `both` — `wall` alone fingerprints a host suspend that froze the monotonic clock). Every entry whose usage was read carries `tokens` (raw) and `tokens_weighted` (cache reads at `limits.cache_read_weight`), keeping per-session spend reconstructible; both are `null` when the usage read failed, and both are absent on an `aborted` end. `tokens_weighted` is the end-of-session total — distinct from a tripped session's `budget_weighted`, the guard's mid-session sample at trip time. ### Hook-based transport (no pane-scraping) - Coding-agent hooks (`Stop` / `SessionStart` / `SessionEnd` / `PreCompact`) write structured event files the orchestrator watches; skills write a machine-readable `result.json`. +- The event channel lives **outside the project tree** (#494), at `///events/` — a branch switch, a worktree mount or a rollback must not be able to take a live run's control plane away. Each session is told where to write via `BMAD_LOOP_EVENTS_DIR`; the state root itself resolves per `BMAD_LOOP_STATE_DIR` (see the env-var table in the README). The relay falls back to the legacy in-tree `/events` when that variable is absent, and the orchestrator keeps polling that location too — the hook script is copied into the project by `init`, so an upgraded orchestrator regularly drives sessions whose relay predates the move, and without both halves every such session would stall to `session_timeout_min`. ### Deferred-work sweeps diff --git a/src/bmad_loop/adapters/generic.py b/src/bmad_loop/adapters/generic.py index 94a82deb..8f98c1a9 100644 --- a/src/bmad_loop/adapters/generic.py +++ b/src/bmad_loop/adapters/generic.py @@ -410,6 +410,7 @@ def __init__( usage_grace_s: float | None = None, stop_without_result_nudges: int | None = None, mux: TerminalMultiplexer | None = None, + events_dir: Path | None = None, ): self.run_dir = run_dir self.policy = policy @@ -441,7 +442,19 @@ def __init__( self.name = f"{profile.name}-tmux" self.binary = binary or profile.binary self.session_name = f"bmad-loop-{run_dir.name}" - self.watcher = SignalWatcher(run_dir / "events") + # The run's hook-event channel (#494): the out-of-tree directory the run + # bootstrap resolved, plus the legacy in-tree one kept under poll so a + # project whose installed relay predates the move still completes its + # sessions. `events_dir` is handed in rather than derived here because + # deriving it needs the PROJECT, and the only project this class can + # reach is `run_dir.parents[2]` — a shape real run dirs have and test run + # dirs do not, so a derivation would key the watcher off a directory that + # is not the project (see `_ensure_session`, which accepts exactly that + # weakness for a session tag but must not for the completion channel). + # Defaulting to the legacy dir keeps direct construction (tests, any + # caller outside `runsetup.make_adapters`) working unchanged; the + # bootstrap always passes one, pinned by a test. + self.watcher = SignalWatcher(events_dir or run_dir / "events", run_dir / "events") self.tasks_dir = run_dir / "tasks" self.logs_dir = run_dir / LOGS_DIR self.tasks_dir.mkdir(parents=True, exist_ok=True) diff --git a/src/bmad_loop/adapters/opencode_http.py b/src/bmad_loop/adapters/opencode_http.py index 28bb4968..09d9c9e5 100644 --- a/src/bmad_loop/adapters/opencode_http.py +++ b/src/bmad_loop/adapters/opencode_http.py @@ -390,7 +390,14 @@ def __init__( extra_args: tuple[str, ...] | None = None, usage_grace_s: float | None = None, stop_without_result_nudges: int | None = None, + events_dir: Path | None = None, ): + # `events_dir` is accepted and unused: this family observes over SSE and + # fires no hooks, so it has no event channel to point at. It is part of + # the run description `runsetup.make_adapters` hands every family (#494), + # and refusing the kwarg here would make the bootstrap branch per family + # on a value that costs nothing to carry. + del events_dir self._httpx = _require_httpx() self.run_dir = run_dir self.policy = policy diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 66bad4b5..4b9fbbe4 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -1742,6 +1742,24 @@ def _render_invocation(pol, project: Path, role: str, prompt: str) -> str: return " ".join(argv) +def _events_dir_preview(project: Path) -> str | None: + """``BMAD_LOOP_EVENTS_DIR`` as a real run would set it, with a ```` + placeholder standing in for the id no dry run has (a preview creates nothing, + so the placeholder never reaches a filesystem). Printed once per preview + rather than on every story's ``env:`` line: only the run id varies, and the + path is long. + + ``None`` — plus the state root's own error on stderr — when no state root can + be resolved. A real run resolves the same path while building its adapters, so + silently dropping the line would turn a preview into a promise the run cannot + keep.""" + try: + return str(runs.events_dir_for(project, "")) + except runs.StateRootError as e: + print(f"warning: {e}", file=sys.stderr) + return None + + def _dry_run( paths: bmadconfig.ProjectPaths, pol, @@ -1770,6 +1788,8 @@ def render(role: str, prompt: str) -> str: print("no actionable stories") return 0 print(f"would process {len(queue)} stories (gates={pol.gates.mode}):") + if (events_dir := _events_dir_preview(paths.project)) is not None: + print(f" env (every session): BMAD_LOOP_EVENTS_DIR={events_dir}") dev_skill = _dev_skill_for_role(pol, paths.project, "dev") review_skill = _dev_skill_for_role(pol, paths.project, "review") for story in queue: @@ -1814,6 +1834,8 @@ def _dry_run_stories( f"stories mode: {len(rows)} stories from {folder}/stories.yaml " f"(gates={pol.gates.mode}){spec_ok}" ) + if (events_dir := _events_dir_preview(paths.project)) is not None: + print(f" env (every session): BMAD_LOOP_EVENTS_DIR={events_dir}") print("linear schedule (list order — no depends_on, strictly serial):") dev_skill = _dev_skill_for_role(pol, paths.project, "dev") for row in rows: diff --git a/src/bmad_loop/data/bmad_loop_hook.py b/src/bmad_loop/data/bmad_loop_hook.py index a1eac048..7b22db52 100644 --- a/src/bmad_loop/data/bmad_loop_hook.py +++ b/src/bmad_loop/data/bmad_loop_hook.py @@ -10,9 +10,23 @@ field extraction below tries each. agy alone carries no cwd, sending the workspacePaths list instead. Reads the hook payload from stdin and writes one event file -into the orchestrator's run directory. No-ops (exit 0) unless the session was +into the orchestrator's events directory. No-ops (exit 0) unless the session was spawned by bmad-loop (detected via env vars set on the tmux window), so normal interactive sessions are unaffected. + +Where that directory is: $BMAD_LOOP_EVENTS_DIR when the orchestrator names one, +else the legacy $BMAD_LOOP_RUN_DIR/events. The channel moved out of the project +tree in #494 so a branch switch or a worktree mount cannot take a live run's +control plane away, and this script is COPIED into the target project at init +time — so an upgraded orchestrator regularly drives sessions whose installed +copy is this file's older self, which knows only the legacy path. That pairing +is what the orchestrator's own dual poll (signals.SignalWatcher) covers. + +$BMAD_LOOP_EVENTS_DIR is deliberately NOT part of the no-op detector above: the +mirror-image skew — an older orchestrator that sets only RUN_DIR/TASK_ID driving +a session whose installed relay is this newer file — must still write its +events, and requiring the new variable would silently stall every one of those +runs instead. """ import json @@ -195,8 +209,13 @@ def main() -> int: # agy sends no cwd — it sends workspacePaths, a list of workspace roots. "cwd": payload.get("cwd") or _first_workspace(payload), } + # The orchestrator's own events dir when it named one, else the legacy + # in-tree location this file's older selves are still installed at (see the + # module docstring). `or`, not a presence test: an exported-but-empty value + # names the launch cwd, which is not a control plane. + events_dir = os.environ.get("BMAD_LOOP_EVENTS_DIR") or os.path.join(run_dir, "events") try: - _write_event(os.path.join(run_dir, "events"), f"{ts}-{task_id}-{event_name}.json", event) + _write_event(events_dir, f"{ts}-{task_id}-{event_name}.json", event) except OSError: # A hostile or broken events dir must degrade to the orchestrator's # normal session_timeout_min path, never surface as a hook failure that diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 9579a5c7..1d99dc28 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -54,7 +54,7 @@ from .plugins import HookBus, HookContext, PluginRegistry from .policy import Policy from .recovery_flow import RecoveryFlow -from .runs import clear_graceful_stop, graceful_stop_requested, kill_session +from .runs import clear_graceful_stop, events_dir_for, graceful_stop_requested, kill_session from .sprintstatus import ACTIONABLE_STATUSES from .sprintstatus import advance as sprint_advance from .sprintstatus import load as load_sprint_status @@ -4057,6 +4057,21 @@ def _run_session( env = { "BMAD_LOOP_MODE": "1", "BMAD_LOOP_RUN_DIR": str(self.run_dir), + # Where this session's hook relay writes its events (#494). The one + # required producer site: every engine-driven session — dev/review, + # sweep bundles, stories, injected plugin workflows — is dispatched + # through this dict. The deliberate non-sites all fail closed without + # it: `resolve.py` sets no BMAD_LOOP_TASK_ID, so the relay no-ops and + # an interactive resolve session produces no events at all; `probe.py` + # captures through BMAD_LOOP_PROBE_CAPTURE_DIR and its own probe relay; + # `plugins/bus.py` spawns plain shells with no task id; the Unity + # plugin's helper scripts are not CLI sessions. + # + # Keyed on the run's project and id, not on `self.run_dir`, so the + # value cannot drift from the directory `runsetup.make_adapters` + # pointed this run's SignalWatcher at — the producer and the consumer + # of one channel, derived by one function from the same two inputs. + "BMAD_LOOP_EVENTS_DIR": str(events_dir_for(self.paths.project, self.run_dir.name)), "BMAD_LOOP_TASK_ID": task_id, "BMAD_LOOP_STORY_KEY": task.story_key, } diff --git a/src/bmad_loop/events.py b/src/bmad_loop/events.py index 2df5498c..273439f9 100644 --- a/src/bmad_loop/events.py +++ b/src/bmad_loop/events.py @@ -248,13 +248,16 @@ def relay(event_name: str, stdin: IO[str]) -> int: return 0 ts = time.time_ns() event = shape_event(ts, event_name, task_id, _read_payload(stdin)) + # $BMAD_LOOP_EVENTS_DIR when the orchestrator names one (#494 moved the + # channel out of the project tree), else the legacy in-tree location — the + # same preference, spelled the same way, as the copied hook script's `main()`. + # `or`, not a presence test: an exported-but-empty value names the launch cwd. + # The no-op detector above stays RUN_DIR + TASK_ID for the reason the hook's + # docstring gives: an older orchestrator sets neither the new variable nor any + # expectation that this relay needs it, and its sessions must still complete. + events_dir = os.environ.get("BMAD_LOOP_EVENTS_DIR") or os.path.join(run_dir, "events") try: - # The events dir is derived from the run dir here; #494 Phase 3 gives it - # its own env var and prefers that, keeping this as the fallback for - # hooks installed before the move. - _write_event( - os.path.join(run_dir, "events"), event_file_name(ts, task_id, event_name), event - ) + _write_event(events_dir, event_file_name(ts, task_id, event_name), event) except OSError: # A hostile or broken events dir must degrade to the orchestrator's normal # session_timeout_min path, never surface as a hook failure that fails the diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index 603edc4e..4c46accf 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -412,7 +412,10 @@ def make_adapters( """Build the per-role adapters. ``profiles`` is an already-resolved mapping from :func:`resolve_profiles`; when given, no profile is re-read from disk, so a caller that gated on :func:`config_digest` launches the *same* bytes it - validated (#461 point 4). Omitted, each role resolves fresh as before.""" + validated (#461 point 4). Omitted, each role resolves fresh as before. + + Also the single resolution point for this run's out-of-tree events directory + (#494), handed to every family it builds — see the ``events_dir`` note below.""" from .adapters.multiplexer import fold_version, get_multiplexer, mux_usable from .adapters.profile import ProfileError, get_profile from .adapters.registry import AdapterError, get_adapter_kind @@ -483,6 +486,15 @@ def make_adapters( extra_args=cfg.extra_args, usage_grace_s=cfg.usage_grace_s, stop_without_result_nudges=cfg.stop_without_result_nudges, + # The run's out-of-tree hook-event channel (#494). Resolved HERE, + # from the `project` this function is handed, because it is the + # only layer that holds both halves of the key — the adapter sees + # a run dir and nothing else. Handed to every family rather than + # gated like `mux`: this is a description of the run, not a + # capability, and unlike resolving a multiplexer it costs no probe + # and can refuse no host. The engine derives the same value from + # the same two inputs for the producing side. + events_dir=runs.events_dir_for(project, run_dir.name), ) if kind.needs_mux: # Resolve and probe the shared multiplexer only when a kind diff --git a/src/bmad_loop/signals.py b/src/bmad_loop/signals.py index b9cad325..2a547630 100644 --- a/src/bmad_loop/signals.py +++ b/src/bmad_loop/signals.py @@ -1,8 +1,19 @@ """Watch the per-run events directory for hook-written event files. -The hook script (hooks/bmad_loop_hook.py) writes one JSON file per event, -atomically (tmp + rename), named "--.json". Plain -polling of a near-empty directory is cheap and crash-safe; no inotify. +The hook script (data/bmad_loop_hook.py) and its importable twin (:mod:`events`, +behind ``bmad-loop relay``) write one JSON file per event, atomically (tmp + +rename), named "--.json". Plain polling of a near-empty +directory is cheap and crash-safe; no inotify. + +Two directories, not one (#494). The channel now lives out of the project tree, +under the user-scoped state root (``runs.events_dir_for``), because a branch +switch, a worktree mount or a rollback must not be able to take a live run's +control plane away. The *legacy* in-tree ``/events`` stays polled as +well, and that is the whole version-skew guard: the relay a target project has +installed is a COPY taken at init time, so an upgraded orchestrator routinely +drives sessions whose hook only knows the old location. Without the second poll +that pairing loses every Stop event and every session stalls to +``session_timeout_min`` — the loudest possible regression, delivered silently. """ from __future__ import annotations @@ -25,35 +36,84 @@ class HookEvent: class SignalWatcher: - def __init__(self, events_dir: Path): + """Poll one or two event directories for a run's hook events. + + ``events_dir`` is the primary — the out-of-tree channel this orchestrator + directs its sessions to via ``BMAD_LOOP_EVENTS_DIR``, and the only one + created here. ``legacy_dir`` is the pre-#494 in-tree ``/events``, + polled when given so a project carrying an older installed relay still + completes its sessions (see the module docstring). It is deliberately NOT + created: an orchestrator that recreated the in-tree directory would undo the + move for the operator's `git status` while gaining nothing — a legacy relay + that writes there makes the directory itself. + + The single-positional-argument form is unchanged, and stays the shape the + probe (``probe.py``, watching its own capture dir) and the unit tests use. + """ + + def __init__(self, events_dir: Path, legacy_dir: Path | None = None): self.events_dir = events_dir - self._consumed: set[str] = set() + self.legacy_dir = legacy_dir + # Keyed by (directory, filename), not by filename: one name identifies an + # event only WITHIN a directory, and the two dirs are written by two + # independent relays. Keying on the name alone would let a file consumed + # from one dir mask a different event of the same name in the other, and a + # masked event here is a lost Stop — the run then waits out + # session_timeout_min with its completion signal sitting on disk. + self._consumed: set[tuple[str, str]] = set() self._pending: list[HookEvent] = [] # polled but not yet delivered via wait_for events_dir.mkdir(parents=True, exist_ok=True) + def _dirs(self) -> list[Path]: + """Primary first, then the legacy dir when there is a distinct one. The + equality check keeps a caller that passes the same path twice from + double-scanning; it cannot produce duplicate events either way (the + ``_consumed`` key would repeat), but the second scan would be pure waste.""" + if self.legacy_dir is None or self.legacy_dir == self.events_dir: + return [self.events_dir] + return [self.events_dir, self.legacy_dir] + def poll(self) -> list[HookEvent]: - """Return new, well-formed events since the last poll, oldest first.""" + """Return new, well-formed events since the last poll, oldest first. + + Ordering is by the parsed ``ts`` across BOTH directories, so which relay + wrote an event never affects where it lands in the sequence. + + A missing *legacy* directory is the normal case (nothing installed writes + there any more) and is skipped silently. A missing *primary* still raises, + as it always has: this watcher created it, so its absence means something + removed the live control plane out from under the run. + """ events: list[HookEvent] = [] - for entry in self.events_dir.iterdir(): - if entry.name in self._consumed or entry.suffix != ".json": - continue - self._consumed.add(entry.name) + dirs = self._dirs() + for directory in dirs: try: - data = json.loads(entry.read_text(encoding="utf-8")) - except (json.JSONDecodeError, OSError): - continue - if not isinstance(data, dict) or "event" not in data or "task_id" not in data: - continue - events.append( - HookEvent( - ts=int(data.get("ts", 0)), - event=str(data["event"]), - task_id=str(data["task_id"]), - session_id=data.get("session_id"), - transcript_path=data.get("transcript_path"), - path=entry, + entries = list(directory.iterdir()) + except OSError: + if directory == self.events_dir: + raise + continue # legacy dir absent — the ordinary case + for entry in entries: + key = (str(directory), entry.name) + if key in self._consumed or entry.suffix != ".json": + continue + self._consumed.add(key) + try: + data = json.loads(entry.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError): + continue + if not isinstance(data, dict) or "event" not in data or "task_id" not in data: + continue + events.append( + HookEvent( + ts=int(data.get("ts", 0)), + event=str(data["event"]), + task_id=str(data["task_id"]), + session_id=data.get("session_id"), + transcript_path=data.get("transcript_path"), + path=entry, + ) ) - ) events.sort(key=lambda e: e.ts) return events diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index fbeabb4d..1ec985a6 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -483,8 +483,12 @@ def provision_worktree( project that commits its own skill tree (e.g. .agents/) or config keeps it untouched (no diff merged back); - the hook points at the MAIN repo's already-installed relay via an absolute - path (the relay locates the run dir from $BMAD_LOOP_RUN_DIR, not its own - location), so nothing is written into the worktree's .bmad-loop/; + path (the relay locates its events directory from $BMAD_LOOP_EVENTS_DIR, + falling back to $BMAD_LOOP_RUN_DIR/events — never from its own location), + so nothing is written into the worktree's .bmad-loop/. Since #494 the + primary channel is out of the project tree entirely, so a worktree cannot + carry a run's control plane at all — but the fallback still resolves under + the MAIN run dir, so the guarantee holds for an older installed relay too; - everything we wrote is excluded from git, in a file private to THIS worktree (`.git/worktrees//info/exclude`, activated per-worktree) that dies with it when the worktree is removed. It is never the repository-wide diff --git a/tests/test_adapter_registry.py b/tests/test_adapter_registry.py index 596cd69d..c50505e4 100644 --- a/tests/test_adapter_registry.py +++ b/tests/test_adapter_registry.py @@ -693,6 +693,70 @@ def test_make_adapters_unknown_kind_systemexit_names_profile(fresh_adapter_regis runsetup.make_adapters(project.project, _run_dir(project.project), pol) +def test_make_adapters_hands_every_kind_the_run_events_dir( + fresh_adapter_registry, project, monkeypatch +): + """#494: the events channel lives outside the project tree now, so an adapter + can no longer derive it — the only path it holds is a run dir, and the key + needs the PROJECT too. `make_adapters` is the one layer with both, so it + resolves the directory and hands it down. + + To EVERY kind, unlike `mux`: a path costs nothing to carry, probes no host and + can refuse no run, so gating it would make the bootstrap branch per family over + a value it can always compute. Both variants are checked because they take + separate `__init__` paths (the dev one threads `paths` as well).""" + from bmad_loop import runs + + registry = fresh_adapter_registry + registry.register_adapter("noxport", needs_mux=False, load=lambda: _stub_builder()) + monkeypatch.setattr(mux_mod, "get_multiplexer", lambda: pytest.fail("no mux for this kind")) + install_bmad_config(project) + _write_profile(project.project, "noxport", adapter="noxport") + _write_policy(project.project, '[adapter]\nname = "noxport"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + run_dir = _run_dir(project.project) + adapters = runsetup.make_adapters(project.project, run_dir, pol) + + expected = runs.events_dir_for(project.project, run_dir.name) + assert adapters["dev"].kwargs["events_dir"] == expected # dev variant + assert adapters["triage"].kwargs["events_dir"] == expected # plain variant + # and it is genuinely out of the project tree — the whole point of the move + assert not expected.is_relative_to(project.project) + + +def test_the_generic_watcher_polls_the_state_root_first_and_the_legacy_dir_too( + fresh_adapter_registry, project, monkeypatch +): + """The consumer half, on the real builtin family: the watcher's PRIMARY is the + out-of-tree channel this run's sessions are pointed at, and the pre-#494 in-tree + dir stays under poll so a project whose installed relay predates the move still + completes its sessions. + + This is also the producer/consumer agreement. Both sides derive the primary + from `runs.events_dir_for(project, run_id)` — this one and + `test_session_env_names_the_out_of_tree_events_dir` in test_engine.py, which + asserts the engine exports that same call's result. A run whose two halves + disagreed would poll a directory nothing writes to and stall every session to + `session_timeout_min` with the Stop sitting on disk. + + Ablation guard: drop `events_dir` from `make_adapters`' kwargs and the primary + falls back to the in-tree dir — the first assertion fails.""" + from bmad_loop import runs + + monkeypatch.setattr(mux_mod, "_usable", lambda mux: True) + install_bmad_config(project) + run_dir = _run_dir(project.project) + + adapters = runsetup.make_adapters(project.project, run_dir, policy_mod.load(None)) + + watcher = adapters["dev"].watcher + assert watcher.events_dir == runs.events_dir_for(project.project, run_dir.name) + assert watcher.legacy_dir == run_dir / "events" + assert watcher.events_dir.is_dir() # created for the relay to write into + assert not (run_dir / "events").exists() # never re-created in the project tree + + def test_make_adapters_generic_shares_synthesizing_but_not_triage( fresh_adapter_registry, project, monkeypatch ): diff --git a/tests/test_engine.py b/tests/test_engine.py index 0c7876e3..94519ec8 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2012,6 +2012,38 @@ def test_happy_path(project): assert adapter.sessions[1].prompt.startswith("/bmad-dev-auto ") +def test_session_env_names_the_out_of_tree_events_dir(project): + """#494 producer side: every engine-driven session is told where to write its + hook events, and the answer is the out-of-tree channel — not `/events`, + which a branch switch, a worktree mount or a rollback can take away mid-run. + + `Engine._run_session`'s env dict is the ONE required producer site: dev, + review, sweep bundles, stories and injected plugin-workflow sessions are all + dispatched through it, so both roles below carry the variable from one edit. + + The value is `runs.events_dir_for(project, run_id)` — the same call + `runsetup.make_adapters` points this run's SignalWatcher at (see + `test_the_generic_watcher_polls_the_state_root_first_and_the_legacy_dir_too`). + That agreement is the invariant: a producer and consumer that disagreed would + leave every Stop unobserved and stall the run to `session_timeout_min`. + + Ablation guard: delete the env entry and this fails; point it at + `self.run_dir / "events"` and it fails on the value.""" + from bmad_loop import runs + + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + engine, adapter = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + ) + engine.run() + + expected = str(runs.events_dir_for(project.project, engine.run_dir.name)) + assert [s.role for s in adapter.sessions] == ["dev", "review"] + assert {s.env["BMAD_LOOP_EVENTS_DIR"] for s in adapter.sessions} == {expected} + assert not Path(expected).is_relative_to(project.project) + + def test_post_kill_rescued_result_flows_and_journals(project): """A result rescued by the adapter's post-kill reconcile (#61) reaches the engine as an ordinary completed result — it must flow the completed path diff --git a/tests/test_events.py b/tests/test_events.py index 4e590bbd..71f68faa 100644 --- a/tests/test_events.py +++ b/tests/test_events.py @@ -56,14 +56,29 @@ def _top_level_sources(path: Path) -> dict[str, str]: return found -def _relay(event: str, payload, monkeypatch, run_dir: Path | None, task_id: str = "t1") -> int: - """Drive `bmad-loop relay ` in-process with `payload` on stdin.""" +def _relay( + event: str, + payload, + monkeypatch, + run_dir: Path | None, + task_id: str = "t1", + events_dir: Path | str | None = None, +) -> int: + """Drive `bmad-loop relay ` in-process with `payload` on stdin. + + ``events_dir`` sets (or, as None, explicitly CLEARS) BMAD_LOOP_EVENTS_DIR: + cleared by default so an operator shell that happens to export it cannot + redirect a test's events out from under its assertions.""" if run_dir is None: monkeypatch.delenv("BMAD_LOOP_RUN_DIR", raising=False) monkeypatch.delenv("BMAD_LOOP_TASK_ID", raising=False) else: monkeypatch.setenv("BMAD_LOOP_RUN_DIR", str(run_dir)) monkeypatch.setenv("BMAD_LOOP_TASK_ID", task_id) + if events_dir is None: + monkeypatch.delenv("BMAD_LOOP_EVENTS_DIR", raising=False) + else: + monkeypatch.setenv("BMAD_LOOP_EVENTS_DIR", str(events_dir)) text = payload if isinstance(payload, str) else json.dumps(payload) monkeypatch.setattr(sys, "stdin", io.StringIO(text)) return cli.main(["relay", event]) @@ -583,5 +598,67 @@ def test_relay_defaults_the_event_name_like_the_hook(tmp_path, monkeypatch, caps def _relay_no_event(payload, monkeypatch, run_dir: Path) -> int: monkeypatch.setenv("BMAD_LOOP_RUN_DIR", str(run_dir)) monkeypatch.setenv("BMAD_LOOP_TASK_ID", "t1") + monkeypatch.delenv("BMAD_LOOP_EVENTS_DIR", raising=False) monkeypatch.setattr(sys, "stdin", io.StringIO(json.dumps(payload))) return cli.main(["relay"]) + + +# ------------------------------------------- where the event is written (#494) + + +def test_relay_prefers_the_events_dir_env(tmp_path, monkeypatch, capsys): + """The relay is the OTHER writer of this control plane, so it resolves the + directory exactly as the copied hook script does — the two are pointed at one + channel by the same variable, and a preference that held on only one of them + would split the channel in half depending on which target a project's hook + config happens to name.""" + run_dir = tmp_path / "run" + run_dir.mkdir() + events = tmp_path / "state" / "runs" / "RID" / "events" + + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir, events_dir=events) == 0 + assert capsys.readouterr() == ("", "") + + files = list(events.glob("*.json")) + assert len(files) == 1 and json.loads(files[0].read_text())["session_id"] == "s1" + assert not (run_dir / "events").exists() + + +@pytest.mark.parametrize("value", [None, ""], ids=["unset", "empty"]) +def test_relay_falls_back_to_the_run_dir(tmp_path, monkeypatch, capsys, value): + """An orchestrator predating #494 names no events dir; its sessions must still + write where it polls. Empty is the same case in disguise — `export + BMAD_LOOP_EVENTS_DIR=` leaves an empty value behind, and an empty path names + the launch cwd rather than a control plane. + + Ablation guard: swap the `or` for a presence test and the empty case writes + into the cwd — this fails.""" + run_dir = tmp_path / "run" + run_dir.mkdir() + + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir, events_dir=value) == 0 + assert capsys.readouterr() == ("", "") + + files = list((run_dir / "events").glob("*.json")) + assert len(files) == 1 and json.loads(files[0].read_text())["session_id"] == "s1" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_relay_refuses_a_symlinked_env_directed_events_dir(tmp_path, monkeypatch, capsys): + """The #493 hardening is a property of the directory the relay is pointed at, + not of how that directory was derived — so it must hold for an env-named one, + with the same silent rc 0 degrade.""" + target = tmp_path / "attacker" + target.mkdir() + state = tmp_path / "state" + state.mkdir() + (state / "events").symlink_to(target, target_is_directory=True) + run_dir = tmp_path / "run" + run_dir.mkdir() + + rc = _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir, events_dir=state / "events") + assert rc == 0 + assert capsys.readouterr() == ("", "") + assert list(target.iterdir()) == [] + assert list((state / "events").iterdir()) == [] + assert not (run_dir / "events").exists() # no silent fallback to the legacy dir diff --git a/tests/test_generic_tmux.py b/tests/test_generic_tmux.py index 66e0936e..9f905b35 100644 --- a/tests/test_generic_tmux.py +++ b/tests/test_generic_tmux.py @@ -40,21 +40,33 @@ # raises UnicodeDecodeError — a ValueError, NOT an OSError. _BAD_UTF8 = b"\xff\xfe\x00\x01 not utf-8 \x80\x81" +# The line that decides where the fake relay writes its events. Swapped below to +# build the version-skew twin, so the two scripts differ in nothing else. +_EVENTS_LINE = 'ed="$BMAD_LOOP_EVENTS_DIR"' +_LEGACY_EVENTS_LINE = 'ed="$BMAD_LOOP_RUN_DIR/events"' + FAKE_CLI = """#!/bin/bash # fake CLI: last positional arg is the prompt; env comes from tmux -e prompt="${@: -1}" ts=$(date +%s%N) -mkdir -p "$BMAD_LOOP_RUN_DIR/events" "$BMAD_LOOP_RUN_DIR/tasks/$BMAD_LOOP_TASK_ID" +ed="$BMAD_LOOP_EVENTS_DIR" +mkdir -p "$ed" "$BMAD_LOOP_RUN_DIR/tasks/$BMAD_LOOP_TASK_ID" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \\ - "$ts" "$BMAD_LOOP_TASK_ID" > "$BMAD_LOOP_RUN_DIR/events/$ts-$BMAD_LOOP_TASK_ID-SessionStart.json" + "$ts" "$BMAD_LOOP_TASK_ID" > "$ed/$ts-$BMAD_LOOP_TASK_ID-SessionStart.json" echo "{\\"workflow\\": \\"auto-dev\\", \\"prompt\\": \\"$prompt\\"}" \\ > "$BMAD_LOOP_RUN_DIR/tasks/$BMAD_LOOP_TASK_ID/result.json" ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \\ - "$ts2" "$BMAD_LOOP_TASK_ID" > "$BMAD_LOOP_RUN_DIR/events/$ts2-$BMAD_LOOP_TASK_ID-Stop.json" + "$ts2" "$BMAD_LOOP_TASK_ID" > "$ed/$ts2-$BMAD_LOOP_TASK_ID-Stop.json" sleep 60 # stay alive like an idle interactive session """ +# What a relay installed before #494 knows: only the in-tree location. Pairing it +# with a current orchestrator is the version-skew case the dual poll covers, and +# it is the ordinary state of any project whose `.bmad-loop/bmad_loop_hook.py` +# copy predates the move. +LEGACY_EVENTS_FAKE_CLI = FAKE_CLI.replace(_EVENTS_LINE, _LEGACY_EVENTS_LINE) + def make_adapter( tmp_path, profile_name="claude", binary=None, extra_args=None, mux=None, **policy_kw @@ -73,6 +85,12 @@ def make_adapter( binary=binary, extra_args=extra_args, mux=mux, + # As `runsetup.make_adapters` does: the primary channel is out of the + # project tree (#494), keyed by run. Under `tmp_path` rather than the real + # state root only because these are unit tests; what matters is that it is + # NOT `run_dir / "events"`, so every test here drives the production shape + # (out-of-tree primary, in-tree legacy still under poll). + events_dir=tmp_path / "state" / run_dir.name / "events", ) @@ -2976,9 +2994,9 @@ def test_read_usage_none_without_transcript(tmp_path): assert adapter.read_usage(SessionResult(status="completed")) is None -def _write_fake_cli(tmp_path): +def _write_fake_cli(tmp_path, script: str = FAKE_CLI): fake = tmp_path / "fake-cli" - fake.write_text(FAKE_CLI) + fake.write_text(script) fake.chmod(0o755) return fake @@ -2995,6 +3013,7 @@ def test_tmux_end_to_end_with_fake_cli(tmp_path, profile_name): spec_env = { "BMAD_LOOP_MODE": "1", "BMAD_LOOP_RUN_DIR": str(adapter.run_dir), + "BMAD_LOOP_EVENTS_DIR": str(adapter.watcher.events_dir), "BMAD_LOOP_TASK_ID": "t-int-1", } spec = SessionSpec( @@ -3042,7 +3061,11 @@ def test_tmux_reused_task_id_ignores_stale_artifacts(tmp_path): role="dev", prompt="/bmad-dev-auto 1-1-a", cwd=tmp_path, - env={"BMAD_LOOP_RUN_DIR": str(adapter.run_dir), "BMAD_LOOP_TASK_ID": task_id}, + env={ + "BMAD_LOOP_RUN_DIR": str(adapter.run_dir), + "BMAD_LOOP_EVENTS_DIR": str(adapter.watcher.events_dir), + "BMAD_LOOP_TASK_ID": task_id, + }, timeout_s=30.0, ) try: @@ -3055,6 +3078,49 @@ def test_tmux_reused_task_id_ignores_stale_artifacts(tmp_path): assert result.session_id == "fake-1" # fresh session, not "old" +@pytest.mark.skipif(not HAVE_TMUX, reason="tmux not available") +def test_tmux_end_to_end_with_a_relay_that_only_knows_the_legacy_dir(tmp_path): + """The version-skew guard, end to end through real tmux: a CURRENT + orchestrator (it exports BMAD_LOOP_EVENTS_DIR and waits on the out-of-tree + channel) driving a session whose relay is an OLD copy that writes only to + `/events`. That pairing is not exotic — the relay is copied into the + target project at init, so every project not re-inited after an upgrade is in + it, and without the watcher's legacy poll EVERY such session stalls to + `session_timeout_min` instead of completing. + + Ablation guard: drop `legacy_dir` from `SignalWatcher._dirs()` and this fails + (as a 30s timeout, not an assertion — which is precisely the production + symptom).""" + assert "$BMAD_LOOP_EVENTS_DIR" not in LEGACY_EVENTS_FAKE_CLI, "the twin still reads the new var" + assert LEGACY_EVENTS_FAKE_CLI != FAKE_CLI, "the swap did not take" + + fake = _write_fake_cli(tmp_path, LEGACY_EVENTS_FAKE_CLI) + adapter = make_adapter(tmp_path, binary=str(fake), extra_args=()) + spec = SessionSpec( + task_id="t-legacy-1", + role="dev", + prompt="/bmad-dev-auto 1-1-a", + cwd=tmp_path, + env={ + "BMAD_LOOP_RUN_DIR": str(adapter.run_dir), + "BMAD_LOOP_EVENTS_DIR": str(adapter.watcher.events_dir), + "BMAD_LOOP_TASK_ID": "t-legacy-1", + }, + timeout_s=30.0, + ) + try: + result = adapter.run(spec) + finally: + subprocess.run(["tmux", "kill-session", "-t", adapter.session_name], capture_output=True) + + assert result.status == "completed" + assert result.session_id == "fake-1" + # the premise, asserted rather than assumed: the events really did land in the + # legacy location and nowhere else, so the completion came through the fallback + assert list((adapter.run_dir / "events").glob("*.json")) + assert not list(adapter.watcher.events_dir.glob("*.json")) + + @pytest.mark.skipif(not HAVE_TMUX, reason="tmux not available") def test_tmux_crash_detected(tmp_path): """A session that dies without writing result.json -> crashed. Also the diff --git a/tests/test_hook_script.py b/tests/test_hook_script.py index 96e70138..3c81351a 100644 --- a/tests/test_hook_script.py +++ b/tests/test_hook_script.py @@ -331,6 +331,85 @@ def test_event_file_mode_is_0600(tmp_path): assert stat.S_IMODE(written.stat().st_mode) == 0o600 +# --------------------------------------------- where the event is written (#494) + + +def test_the_events_dir_env_wins_over_the_run_dir(tmp_path): + """#494: the channel moved out of the project tree, and this variable is how + the orchestrator names it. Nothing may land in the legacy in-tree location + when it is set — an event written there is only found by the orchestrator's + compatibility poll, and the whole point is that a branch switch or a worktree + mount cannot take the live channel away.""" + run_dir = tmp_path / "run" + run_dir.mkdir() + events = tmp_path / "state" / "runs" / "RID" / "events" + env = { + "BMAD_LOOP_RUN_DIR": str(run_dir), + "BMAD_LOOP_EVENTS_DIR": str(events), + "BMAD_LOOP_TASK_ID": "t1", + } + proc = run_hook("Stop", env, {"session_id": "s1"}) + + assert proc.returncode == 0 + files = list(events.glob("*.json")) + assert len(files) == 1 and json.loads(files[0].read_text())["session_id"] == "s1" + assert not (run_dir / "events").exists() + + +@pytest.mark.parametrize("value", [None, ""], ids=["unset", "empty"]) +def test_the_events_dir_falls_back_to_the_run_dir(tmp_path, value): + """The version-skew half, from the producing side: an orchestrator that + predates #494 sets no BMAD_LOOP_EVENTS_DIR, and its sessions must still write + their events somewhere the orchestrator polls — the legacy location it knows. + + The empty case is not hypothetical: `export BMAD_LOOP_EVENTS_DIR=` is what an + unset-looking export leaves behind, and an empty path names the launch cwd, + which is not a control plane. + + Ablation guard: change the `or` to a presence test (`is not None`) and the + empty case writes into the CLI's working directory instead — this fails.""" + run_dir = tmp_path / "run" + run_dir.mkdir() + env = {"BMAD_LOOP_RUN_DIR": str(run_dir), "BMAD_LOOP_TASK_ID": "t1"} + if value is not None: + env["BMAD_LOOP_EVENTS_DIR"] = value + proc = run_hook("Stop", env, {"session_id": "s1"}) + + assert proc.returncode == 0 + files = list((run_dir / "events").glob("*.json")) + assert len(files) == 1 and json.loads(files[0].read_text())["session_id"] == "s1" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_a_symlinked_env_directed_events_dir_writes_nothing_and_exits_zero(tmp_path): + """The #493 hardening is about the DIRECTORY the relay is pointed at, so it + has to hold for the one an env var names just as it did for the one derived + from the run dir. Under isolation a driven session can write into the project + but not into the state root — yet the variable itself is inherited env, and a + session that can plant a link at the named path must still not redirect the + control plane through it.""" + target = tmp_path / "attacker" + target.mkdir() + state = tmp_path / "state" + state.mkdir() + (state / "events").symlink_to(target, target_is_directory=True) + run_dir = tmp_path / "run" + run_dir.mkdir() + + env = { + "BMAD_LOOP_RUN_DIR": str(run_dir), + "BMAD_LOOP_EVENTS_DIR": str(state / "events"), + "BMAD_LOOP_TASK_ID": "t1", + } + proc = run_hook("Stop", env, {"session_id": "s1"}) + + assert proc.returncode == 0 + assert list(target.iterdir()) == [] + assert list((state / "events").iterdir()) == [] + # and it did not silently fall back to the legacy location either + assert not (run_dir / "events").exists() + + def test_installed_copy_matches_source(tmp_path): from bmad_loop.install import install_into diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index 92b06d4a..86ffc144 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -154,6 +154,9 @@ def test_generic_adapter_drives_only_the_mux(tmp_path, no_tmux): policy=Policy(limits=LimitsPolicy()), profile=get_profile("claude"), mux=stub, + # out of the project tree, as `runsetup.make_adapters` resolves it (#494), + # so the seeded Stop below has to be observed on the PRIMARY channel + events_dir=tmp_path / "state" / "events", ) spec = _spec(tmp_path) diff --git a/tests/test_portability_guard.py b/tests/test_portability_guard.py index 36a4dc37..4ae1bc44 100644 --- a/tests/test_portability_guard.py +++ b/tests/test_portability_guard.py @@ -131,6 +131,7 @@ # and these scripts read back what was handed to them. SESSION_PROTOCOL_ENV = ( "BMAD_LOOP_RUN_DIR", + "BMAD_LOOP_EVENTS_DIR", "BMAD_LOOP_TASK_ID", "BMAD_LOOP_WORKTREE", "BMAD_LOOP_REPO_ROOT", @@ -167,7 +168,7 @@ # `events.py` is the ONE in-package entry here, and the "cannot import # bmad_loop" justification above does not reach it — it obviously can. It is # exempt as the importable PARITY TWIN of the stdlib-only hook relay: the same - # two session-protocol vars, read at the same point in the same protocol, by + # session-protocol vars, read at the same points in the same protocol, by # the code the hook config points at when it points at `bmad-loop relay` # instead of the copied script. Routing one twin through `envvars` and leaving # the other on `os.environ` would put the reads out of parity, and parity is diff --git a/tests/test_signals.py b/tests/test_signals.py index 588e5fd5..ce6a7620 100644 --- a/tests/test_signals.py +++ b/tests/test_signals.py @@ -1,10 +1,13 @@ import json +import pytest + from bmad_loop.signals import SignalWatcher def write_event(events_dir, ts, task_id, event, **extra): payload = {"ts": ts, "event": event, "task_id": task_id, **extra} + events_dir.mkdir(parents=True, exist_ok=True) (events_dir / f"{ts}-{task_id}-{event}.json").write_text(json.dumps(payload)) @@ -89,3 +92,90 @@ def sleep(seconds): assert watcher.wait_for("t1", {"Stop"}, timeout_s=10, clock=clock, sleep=sleep) is None assert now["t"] >= 10 + + +# ------------------------------------------------------ dual poll (#494 skew guard) + + +def test_poll_sees_an_event_written_only_to_the_legacy_dir(tmp_path): + """THE version-skew case, and the reason the legacy dir is polled at all. + + The relay a target project runs is a COPY taken at init time, so an upgraded + orchestrator routinely drives sessions whose hook knows only the pre-#494 + in-tree `/events`. Nothing in the new location, everything in the old + one, and the Stop must still be observed — the alternative is that EVERY + session under such a project stalls to `session_timeout_min`. + + Ablation guard: drop `legacy_dir` from `_dirs()` and this fails.""" + watcher = SignalWatcher(tmp_path / "state" / "events", tmp_path / "run" / "events") + write_event(tmp_path / "run" / "events", 1, "t1", "Stop", session_id="legacy") + + event = watcher.wait_for("t1", {"Stop"}, timeout_s=1) + assert event is not None and event.session_id == "legacy" + + +def test_poll_orders_both_dirs_by_ts(tmp_path): + """Ordering is by the event's own `ts`, not by which directory it came from — + a run mid-upgrade could take a SessionStart from one relay and a Stop from + another, and `wait_for`'s buffering hands them out in poll order.""" + primary = tmp_path / "state" / "events" + legacy = tmp_path / "run" / "events" + watcher = SignalWatcher(primary, legacy) + write_event(legacy, 3, "t1", "Stop") + write_event(primary, 2, "t1", "PreCompact") + write_event(legacy, 1, "t1", "SessionStart") + + assert [e.event for e in watcher.poll()] == ["SessionStart", "PreCompact", "Stop"] + + +def test_poll_tolerates_a_missing_legacy_dir(tmp_path): + """The ordinary case once every relay is current: nothing ever creates the + in-tree dir, so it simply is not there. That must not raise — and must not be + papered over by creating it either (see the next test).""" + primary = tmp_path / "state" / "events" + watcher = SignalWatcher(primary, tmp_path / "run" / "events") + write_event(primary, 1, "t1", "Stop", session_id="s1") + + assert [e.session_id for e in watcher.poll()] == ["s1"] + + +def test_only_the_primary_dir_is_created(tmp_path): + """The whole point of #494 is that the run's control plane stops living in the + project tree. An orchestrator that re-created `/events` to poll it + would put the directory back in the operator's `git status` for nothing: a + legacy relay makes it itself, and a current one never writes there.""" + legacy = tmp_path / "run" / "events" + SignalWatcher(tmp_path / "state" / "events", legacy) + + assert (tmp_path / "state" / "events").is_dir() + assert not legacy.exists() + + +def test_the_same_file_name_in_both_dirs_yields_both_events(tmp_path): + """`_consumed` is keyed by (dir, name), so consuming a name from one directory + cannot mask a different event of that name in the other. The names collide on + (ts, task_id, event), which two independent relays can produce; a masked event + here would be a lost Stop. + + Ablation guard: key `_consumed` on `entry.name` alone and this fails.""" + primary = tmp_path / "state" / "events" + legacy = tmp_path / "run" / "events" + watcher = SignalWatcher(primary, legacy) + write_event(primary, 1, "t1", "Stop", session_id="from-primary") + write_event(legacy, 1, "t1", "Stop", session_id="from-legacy") + + assert sorted(e.session_id for e in watcher.poll()) == ["from-legacy", "from-primary"] + assert watcher.poll() == [] # both consumed + + +def test_poll_still_raises_when_the_primary_dir_is_gone(tmp_path): + """The legacy dir's absence is expected; the primary's is not — this watcher + created it, so something removed a live run's control plane out from under it. + Unchanged behavior, pinned here so the tolerance added for the legacy dir is + not quietly widened to both.""" + primary = tmp_path / "state" / "events" + watcher = SignalWatcher(primary, tmp_path / "run" / "events") + primary.rmdir() + + with pytest.raises(OSError): + watcher.poll() diff --git a/tests/test_stories_e2e.py b/tests/test_stories_e2e.py index a2ca2f71..8b81feb4 100644 --- a/tests/test_stories_e2e.py +++ b/tests/test_stories_e2e.py @@ -62,6 +62,7 @@ install_dev_base_skills, ) +from bmad_loop import runs from bmad_loop.install import ( BMAD_SCRIPTS_SEED_REL, CENTRAL_CONFIG_REL, @@ -88,9 +89,10 @@ story="$BMAD_LOOP_STORY_KEY"; folder="$BMAD_LOOP_SPEC_FOLDER" prompt="${1:-}" ts=$(date +%s%N) -mkdir -p "$rd/events" "$rd/tasks/$tid" +ed="$BMAD_LOOP_EVENTS_DIR" +mkdir -p "$ed" "$rd/tasks/$tid" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts" "$tid" > "$rd/events/$ts-$tid-SessionStart.json" + "$ts" "$tid" > "$ed/$ts-$tid-SessionStart.json" # argv as it ARRIVED, after profile render + tmux quoting — the orchestrator's own # tasks//prompt.txt records the pre-render prompt, so only this file can prove a # dispatched skill NAME actually reached the binary. @@ -109,7 +111,7 @@ > "$tdir/result.json" ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts2" "$tid" > "$rd/events/$ts2-$tid-Stop.json" + "$ts2" "$tid" > "$ed/$ts2-$tid-Stop.json" sleep 30 exit 0 fi @@ -158,7 +160,7 @@ fi ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts2" "$tid" > "$rd/events/$ts2-$tid-Stop.json" + "$ts2" "$tid" > "$ed/$ts2-$tid-Stop.json" sleep 30 exit 0 fi @@ -196,10 +198,16 @@ ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts2" "$tid" > "$rd/events/$ts2-$tid-Stop.json" + "$ts2" "$tid" > "$ed/$ts2-$tid-Stop.json" sleep 30 """ +# The same script as a relay installed BEFORE #494 would be: it knows only the +# in-tree `/events` and ignores the variable the orchestrator now exports. +# Built by swapping the one line that names the directory, so it can differ from +# FAKE_CLI in nothing else. +LEGACY_EVENTS_FAKE_CLI = FAKE_CLI.replace('ed="$BMAD_LOOP_EVENTS_DIR"', 'ed="$rd/events"') + PROFILE_TOML = """\ name = "fakestories" binary = "{binary}" @@ -225,9 +233,10 @@ set -e rd="$BMAD_LOOP_RUN_DIR"; tid="$BMAD_LOOP_TASK_ID" ts=$(date +%s%N) -mkdir -p "$rd/events" +ed="$BMAD_LOOP_EVENTS_DIR" +mkdir -p "$ed" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts" "$tid" > "$rd/events/$ts-$tid-SessionStart.json" + "$ts" "$tid" > "$ed/$ts-$tid-SessionStart.json" # Background + wait keeps the same process group as a foreground sleep, but # records the child's pid so the test can prove teardown reaped descendants, # not just this shell (whose cmdline is all the pgrep check can see). @@ -247,9 +256,10 @@ set -e rd="$BMAD_LOOP_RUN_DIR"; tid="$BMAD_LOOP_TASK_ID"; story="$BMAD_LOOP_STORY_KEY" ts=$(date +%s%N) -mkdir -p "$rd/events" +ed="$BMAD_LOOP_EVENTS_DIR" +mkdir -p "$ed" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts" "$tid" > "$rd/events/$ts-$tid-SessionStart.json" + "$ts" "$tid" > "$ed/$ts-$tid-SessionStart.json" baseline=$(git rev-parse HEAD) # Detach a straggler into a NEW session (setsid): $! is the setsid'd process itself @@ -269,7 +279,7 @@ ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \ - "$ts2" "$tid" > "$rd/events/$ts2-$tid-Stop.json" + "$ts2" "$tid" > "$ed/$ts2-$tid-Stop.json" # Stay alive like an idle interactive session so the pane shell is still live when # the engine kills it: the harvest sees the detached child as our descendant only # while we (its parent) are still around. Long enough to outlast Stop -> kill_window @@ -1014,6 +1024,41 @@ def test_e2e_sweep_intent_gap_patch_restore(tmp_path): assert any(str(spec) in p for p in prompts) +def test_e2e_a_relay_that_only_knows_the_legacy_events_dir_still_completes(tmp_path): + """The #494 version-skew guard, through the real CLI and real tmux. + + `bmad_loop_hook.py` is COPIED into the target project by `init`, so a project + that upgraded bmad-loop without re-initing runs a relay that has never heard of + BMAD_LOOP_EVENTS_DIR and writes only to `/events`. The orchestrator + exports the variable and waits on the out-of-tree channel regardless — so + without the watcher's second poll, that pairing observes no Stop at all and + EVERY session in the project stalls to `session_timeout_min`. It would not fail + a test suite; it would hang a user's overnight run. + + Asserted on the outcome (the story reaches `done` and commits), plus the + premise: the events really did land only in the legacy location, so the + completion cannot have come through the primary channel. + + Ablation guard: drop `legacy_dir` from `SignalWatcher._dirs()` and this fails — + slowly, as the session timeout, which is exactly the production symptom.""" + assert "$BMAD_LOOP_EVENTS_DIR" not in LEGACY_EVENTS_FAKE_CLI, "the twin still reads the new var" + assert LEGACY_EVENTS_FAKE_CLI != FAKE_CLI, "the swap did not take" + + root = tmp_path / "sbx" + story = "1-1-thing" + _scaffold_sprint(root, story, fake_cli=LEGACY_EVENTS_FAKE_CLI) + base = _commit_count(root) + + proc = _run(root, "run") + assert proc.returncode == 0, proc.stderr or proc.stdout + assert _sprint_status(root, story) == "done" + assert _commit_count(root) == base + 1 + + run_id = _run_id(root) + assert list((root / ".bmad-loop" / "runs" / run_id / "events").glob("*.json")) + assert not list(runs.events_dir_for(root, run_id).glob("*.json")) + + def test_e2e_sprint_mode_regression(tmp_path): # Scenario 6 (audit MAJOR-2): the new folder+id-capable bmad-dev-auto skill is # installed, but this is a plain SPRINT-mode run. It must drive dev → verify → From 2ef7e5d061af1b29700df8320c284878d9b57b0b Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 17:57:58 -0700 Subject: [PATCH 04/11] feat(observability): make validate and diagnose honest about the relocated events channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 4 of #494/#498. Phase 3 moved the hook-event channel out of the project tree to the user state root and left the in-tree location live as a fallback; both observation surfaces still described the pre-move world. `validate` gains `hooks.relay-stale`. `hooks.relay-present` stats the relay but never asks which version it is, and the relay is COPIED into the project by `init` — so an upgraded orchestrator routinely drives sessions through a relay that predates the move and still writes in-tree, with validate reading green. The installed copy is now compared against the packaged source (as text, which is exactly the read_text/write_text round trip `install_into` performs — a byte compare would call every Windows install permanently stale) and a difference is reported with the `init` that fixes it. A warning, never a problem: the dual-poll fallback keeps a stale relay working, so the exit code must not move. `diagnose`'s `events` file group listed a run-dir-relative name, so after the move it did not redden — it silently vanished, because a category with no directory is a no-op. It now stats the state-root location, derived from the run's own recorded project and id, and sums it with the still-live legacy root into the one category. Unchanged payload, no SCHEMA_VERSION bump. Every step of the derivation is a host fact a dump read elsewhere may not have, so all of them degrade to the legacy count rather than sinking the run. Tests ablation-checked: the validate arms against a forced-True comparison, a deleted block and a hoisted gate; the diagnose arms against each root dropped singly and an escaping raise; both `_ABS_HOME_RE` arms against a state-root path in the dump. The unregistered-hooks gate test was vacuous in its first form (no relay on disk means the comparison degrades either way) and now leaves the relay installed and stale. --- CHANGELOG.md | 10 +++ src/bmad_loop/checks.py | 1 + src/bmad_loop/cli.py | 32 ++++++++ src/bmad_loop/diagnostics.py | 101 ++++++++++++++++++----- src/bmad_loop/install.py | 37 +++++++++ tests/test_cli.py | 114 ++++++++++++++++++++++++++ tests/test_diagnostics.py | 152 +++++++++++++++++++++++++++++++++++ 7 files changed, 428 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 974261cf..cde0dff0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,16 @@ whose seams had diverged enough that several ports needed a different fix, and t `session_timeout_min`. Re-run `bmad-loop init` to refresh the relay. `--dry-run` previews the directory the run would use. + Both observation surfaces are honest about the split. `validate` gains `hooks.relay-stale`: a + present, readable relay is not necessarily a current one, so the installed copy is compared + against the packaged source and a difference is reported with the `init` that refreshes it. It is + a **warning** and never moves the exit code — the fallback keeps a stale relay working, so this + reports a lost property, not a broken run. `diagnose`'s `events` file group is retargeted to the + state root and sums both roots into the one category (unchanged payload, no schema bump); it had + silently reported nothing after the move, since a category with no directory is a no-op. An + underivable state root — a dump read on a machine that did not produce the run — costs the events + count and nothing else. + - **Coding-CLI adapter registry: a new adapter class ships out-of-tree (#226).** The transport axis has long been extensible out-of-tree; the CLI axis had no equivalent, so a CLI needing its own adapter _class_ forced a name-branch in the run bootstrap. A profile's new `adapter` field names a diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py index 4077272a..0fb60268 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -64,6 +64,7 @@ "hooks.config-parse", "hooks.registered", "hooks.relay-present", + "hooks.relay-stale", "mux.backend", "mux.preflight", "mux.backends-detected", diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 4b9fbbe4..49fd820e 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -541,6 +541,38 @@ def cmd_validate(args: argparse.Namespace) -> int: {"path": str(relay)}, ) + # #494 Phase 4: present-and-readable is not current. The relay is COPIED + # into the project by `init`, so an upgraded orchestrator routinely drives + # sessions through a relay written by an older wheel — and the #494 move + # is exactly the kind of change that skew hides: a pre-move relay writes + # its events to the in-tree `/events` while the operator believes + # the channel left the project tree, so a branch switch can still take the + # control plane away mid-run. + # + # A WARNING, never a problem, and validate's exit code must not move: + # Phase 3's fallback pair keeps a stale relay FUNCTIONAL (it writes the + # legacy directory, which SignalWatcher still polls), so the run completes + # — the operator is losing the property, not the loop. `passed` counts + # only problems, so `warn` is what says "degraded but working". + stale = install.hook_script_current(project) + if stale is False: + report.warn( + "hooks.relay-stale", + f"the installed hook relay {relay} differs from this bmad-loop's " + f"— it is from another version, or was edited. Events may still be " + f"written inside the project tree; run `bmad-loop init` to refresh it", + {"path": str(relay)}, + ) + elif stale is True: + report.ok( + "hooks.relay-stale", + f"hook relay script up to date: {relay}", + {"path": str(relay)}, + ) + # `None` (unreadable/undecodable on either side) reports nothing: the + # relay-present block above already spoke for the cases an operator can + # act on, and "I could not compare" is not a finding about their project. + # Adapter-kind validity is enforced against the LIVE registry, never a # hardcoded set: a profile.adapter naming no registered kind is a config error # (a typo, or an uninstalled plugin package). External adapter/profile packages diff --git a/src/bmad_loop/diagnostics.py b/src/bmad_loop/diagnostics.py index dcb76b01..edc0cb4c 100644 --- a/src/bmad_loop/diagnostics.py +++ b/src/bmad_loop/diagnostics.py @@ -63,10 +63,14 @@ SCHEMA_VERSION = 1 DEFAULT_JOURNAL_CAP = 200 -# Run-dir subdirectories whose mere existence/size is diagnostic but whose -# CONTENTS are off-limits (raw tmux panes = code, prompts, feedback prose, -# patches, full worktree checkouts). We stat them; we never read into output. +# Subdirectories whose mere existence/size is diagnostic but whose CONTENTS are +# off-limits (raw tmux panes = code, prompts, feedback prose, patches, full +# worktree checkouts). We stat them; we never read into output. +# +# All are run-dir-relative EXCEPT "events", which since #494 lives out of the +# project tree at the user state root — see `_category_roots`. _FILE_CATEGORIES = ("logs", "tasks", "feedback", "bundles", "failed", "worktrees", "events") +_EVENTS_CATEGORY = "events" # Journal fields that name a proprietary identifier — pseudonymized, not dropped, # so events stay correlatable. Maps field name -> alias namespace. @@ -297,30 +301,62 @@ def collect_env(project: Path) -> EnvInfo: ) -def summarize_files(run_dir: Path) -> list[FileGroup]: - """Counts/sizes only — file contents are NEVER opened into the output.""" +def _category_roots(category: str, run_dir: Path, events_dir: Path | None) -> list[Path]: + """Where a category's files live. One directory for all but ``events``. + + The events channel has TWO live roots since #494 (Phase 3): the primary is + out of the project tree under the user state root, and the legacy in-tree + ``/events`` is still both written and polled. It has to be, because + the hook relay is COPIED into the project by ``init`` — an upgraded + orchestrator routinely drives sessions whose relay predates the move and + still writes in-tree, so the watcher dual-polls. Counting only one root would + report zero events for precisely the runs whose event routing is what a + maintainer is reading the dump to understand. + + Both are summed into ONE ``FileGroup`` named ``events``: the payload shape is + the schema, and splitting the category (or adding a field) would be a break + for a v1 consumer. Which root the events came from is not what the count is + for — "did the hooks fire at all" is. + """ + if category != _EVENTS_CATEGORY: + return [run_dir / category] + legacy = run_dir / _EVENTS_CATEGORY + # Deduped by spelling so a state root deliberately pointed inside the run dir + # (BMAD_LOOP_STATE_DIR is honoured as spelled) cannot double-count. + if events_dir is None or events_dir == legacy: + return [legacy] + return [events_dir, legacy] + + +def summarize_files(run_dir: Path, *, events_dir: Path | None = None) -> list[FileGroup]: + """Counts/sizes only — file contents are NEVER opened into the output. + + ``events_dir`` is this run's out-of-tree event channel (#494); ``None`` when + the caller could not derive one, which degrades to the legacy in-tree + location alone rather than dropping the category. + """ groups: list[FileGroup] = [] for category in _FILE_CATEGORIES: - root = run_dir / category - if not root.is_dir(): - continue count = 0 total_bytes = 0 total_lines = 0 - for p in root.rglob("*"): - if not p.is_file(): + for root in _category_roots(category, run_dir, events_dir): + if not root.is_dir(): continue - count += 1 - try: - total_bytes += p.stat().st_size - except OSError: - pass - if category == "logs": + for p in root.rglob("*"): + if not p.is_file(): + continue + count += 1 try: - with p.open("rb") as f: - total_lines += sum(1 for _ in f) + total_bytes += p.stat().st_size except OSError: pass + if category == "logs": + try: + with p.open("rb") as f: + total_lines += sum(1 for _ in f) + except OSError: + pass if count: groups.append( FileGroup( @@ -500,6 +536,33 @@ def _coarsen_date(started_at: str | None) -> str | None: return head if sanitize.looks_like_identifier(head.replace("-", "0")) else None +def _events_dir(state: RunState) -> Path | None: + """This run's out-of-tree event channel, or ``None`` if it is not derivable. + + Derived from the run's OWN recorded project and id rather than threaded down + from ``cmd_diagnose``'s ``--project``, which is the smaller change and the + truer one: ``--all`` dumps every run under a project, and a run carries the + project it was started against. ``run_dir`` cannot answer this — the state + root is keyed by a digest of the resolved project, not by the run dir's + ancestry. + + Every failure mode is observation, so every one of them degrades. The + derivation resolves the project (``runs.project_tag``) and consults the host + for a state root, and a dump is routinely read on a machine that is not the + one that produced it: an unresolvable project, or a host with no derivable + state root, must cost the events count and nothing else. + """ + from . import runs + + try: + return runs.events_dir_for(Path(state.project), state.run_id) + except Exception: + # No `# nosec`: bandit's B110/B112 are about `pass`/`continue` bodies, and + # a directive naming a rule that never fires here would read as a waived + # finding. The breadth is deliberate and stated above, not silenced. + return None + + def collect_run(run_dir: Path, *, pseudo: sanitize.Pseudonymizer, cap: int) -> RunDiag: state: RunState = load_state(run_dir) tasks = list(state.tasks.values()) @@ -541,7 +604,7 @@ def collect_run(run_dir: Path, *, pseudo: sanitize.Pseudonymizer, cap: int) -> R session_tally=_session_tally(tasks), tasks=[_task_diag(t, pseudo, weight) for t in tasks], journal=summarize_journal(Journal(run_dir).entries(), pseudo, epic_by_key, cap=cap), - files=summarize_files(run_dir), + files=summarize_files(run_dir, events_dir=_events_dir(state)), ) diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 885c33fb..8fc44afc 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -1010,6 +1010,43 @@ def _review_findings(project: Path, tree: str) -> list[Finding]: return findings +def hook_script_current(project: Path) -> bool | None: + """Does the project's installed relay match the one this wheel would write? + + ``True`` yes, ``False`` stale (or otherwise divergent), ``None`` unknowable — + the installed copy or the packaged source could not be read as text. The + unknown arm is a third state and not a coerced ``False`` on purpose: the sole + caller (``cmd_validate``'s ``hooks.relay-stale``) reports what it knows, and + "I could not look" is not "your relay is out of date". + + Lives here, beside :func:`install_into`'s write of the same two paths, so the + packaged-source spelling has exactly one home — a comparison that resolved + the source differently from the writer would answer a different question. + + Compared as TEXT read with universal newlines, not as raw bytes. That is + precisely the round trip ``install_into`` performs (``read_text`` then + ``write_text``), and ``write_text`` translates ``\\n`` to ``os.linesep`` — so + on Windows every freshly-installed relay differs from the packaged source + byte-for-byte while being exactly what ``init`` writes. A byte compare would + call those installs permanently stale. + """ + try: + installed = (project / HOOK_SCRIPT_REL).read_text(encoding="utf-8") + packaged = ( + resources.files("bmad_loop.data") + .joinpath("bmad_loop_hook.py") + .read_text(encoding="utf-8") + ) + except (OSError, ValueError): + # ValueError covers UnicodeDecodeError (a relay overwritten with non-UTF-8 + # bytes); OSError covers missing/unreadable on either side. Observation + # degrades — the missing and unreadable cases already have their own + # finding (`hooks.relay-present`), and a packaged source this process + # cannot read is a broken wheel, not a stale project. + return None + return installed == packaged + + def _hook_command(project: Path, profile: CLIProfile, canonical_event: str) -> str: host = get_process_host() interp = host.hook_interpreter() diff --git a/tests/test_cli.py b/tests/test_cli.py index 719edca5..f46f86ed 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4834,6 +4834,120 @@ def test_validate_flags_registered_hooks_with_an_unreadable_relay_script(project relay.chmod(0o644) # so the sandbox tears down cleanly +def _validate_json(project_dir, capsys): + """`validate --json` as (rc, document). Not `machine_json`: that helper takes + the expected rc as an input, and these tests are ABOUT the rc — the sandbox's + baseline verdict comes from findings that have nothing to do with the relay, + so it must be observed, never asserted.""" + rc = cli.main(["validate", "--project", str(project_dir), "--json"]) + out, err = capsys.readouterr() + assert err == "" # the machine.py purity contract still holds + return rc, json.loads(out) + + +def test_validate_reports_a_fresh_relay_as_up_to_date(project, capsys): + """#494 Phase 4: a relay `init` just wrote matches the packaged source. + + Pinned separately from the stale case because the two arms fail for opposite + reasons — a comparison wrong in the *other* direction (a raw byte compare + against a `write_text`-translated file, which on Windows calls every fresh + install stale) reddens here and nowhere else.""" + install_bmad_config(project) + _write_policy(project.project) + assert cli.main(["init", "--project", str(project.project), "--no-skills"]) == 0 + args = argparse.Namespace(project=str(project.project), spec=None) + + cli.cmd_validate(args) + text = _validate_output(capsys) + assert "hook relay script up to date" in text + assert "differs from this bmad-loop" not in text + + +def test_validate_warns_when_the_installed_relay_is_stale(project, capsys): + """#494 Phase 4: the relay is COPIED into the project by `init`, so an + upgraded orchestrator drives sessions through whatever version the project + happens to hold — and a pre-#494 relay still writes its events INSIDE the + project tree. `hooks.relay-present` cannot see that: the file is there and + readable, so it reads green. + + The verdict must not move — the dual-poll fallback keeps a stale relay + working, so this reports a lost property, not a broken run. Asserted as + "unchanged from the same project one edit earlier" rather than a literal 0: + the sandbox's baseline comes from unrelated findings, and hardcoding it would + pin this test to those instead of to the relay. + + Ablation guard: deleting the `hook_script_current` block in cmd_validate (or + forcing it to `True`) makes this FAIL on the `hooks.relay-stale` lookup.""" + from bmad_loop import install as install_mod + + install_bmad_config(project) + _write_policy(project.project) + assert cli.main(["init", "--project", str(project.project), "--no-skills"]) == 0 + capsys.readouterr() # drop init's chatter; _validate_json parses the WHOLE stream + + baseline_rc, baseline = _validate_json(project.project, capsys) + + relay = project.project / install_mod.HOOK_SCRIPT_REL + relay.write_text( + relay.read_text(encoding="utf-8") + "\n# an older wheel wrote this\n", encoding="utf-8" + ) + rc, doc = _validate_json(project.project, capsys) + + # The id is the matchable identity (checks.py) — match on it, not the prose. + stale = [f for f in doc["findings"] if f["check"] == "hooks.relay-stale"] + assert len(stale) == 1 + assert stale[0]["severity"] == "warning" + assert stale[0]["detail"]["path"] == str(relay) + assert "bmad-loop init" in stale[0]["message"] + + # The verdict and exit code are untouched by a warning. + assert (rc, doc["ok"]) == (baseline_rc, baseline["ok"]) + # And the two checks it must not be confused with still read exactly as before. + by_id = {f["check"]: f["severity"] for f in doc["findings"]} + assert by_id["hooks.registered"] == "ok" + assert by_id["hooks.relay-present"] == "ok" + + +def test_validate_is_silent_about_relay_staleness_when_hooks_are_unregistered(project, capsys): + """The check hangs off `any_hooks_registered`, like `hooks.relay-present`: a + project not routing events through any relay is not owed a note about which + version of one it holds — it is owed the registration FAIL, and nothing on + top of it. + + The relay is left INSTALLED and STALE on purpose. A test that simply skips + `init` proves nothing: with no relay on disk the comparison degrades to + "unknowable" and emits no finding whether it is gated or not, so the gate + could be deleted outright and such a test would stay green (it was, and it + did). This shape is the one that separates them. + + Ablation guard: hoisting the `hooks.relay-stale` block out of the + `if any_hooks_registered:` block makes this FAIL.""" + from bmad_loop import install as install_mod + from bmad_loop.adapters.profile import load_profiles + + install_bmad_config(project) + _write_policy(project.project) + assert cli.main(["init", "--project", str(project.project), "--no-skills"]) == 0 + + # Unregister: keep the relay, drop every registration that points at it. + for profile in load_profiles(project.project).values(): + config = project.project / profile.hooks.config_path + if config.is_file(): + config.write_text("{}\n", encoding="utf-8") + relay = project.project / install_mod.HOOK_SCRIPT_REL + relay.write_text(relay.read_text(encoding="utf-8") + "\n# stale\n", encoding="utf-8") + capsys.readouterr() + + _rc, doc = _validate_json(project.project, capsys) + # The premise, asserted rather than assumed — a still-registered project would + # make the absence below prove nothing. + assert any( + f["check"] == "hooks.registered" and f["severity"] == "problem" for f in doc["findings"] + ) + assert relay.is_file() # and the stale artifact really is still there + assert not [f for f in doc["findings"] if f["check"] == "hooks.relay-stale"] + + def test_validate_sprint_mode_still_gates_on_sprint_status(project, capsys): """Item 8 regression: the default (sprint) mode still requires sprint-status.""" install_bmad_config(project) diff --git a/tests/test_diagnostics.py b/tests/test_diagnostics.py index a86c8e56..3b2817ac 100644 --- a/tests/test_diagnostics.py +++ b/tests/test_diagnostics.py @@ -657,6 +657,158 @@ def test_non_ascii_survives_the_utf8_round_trip(tmp_path, monkeypatch): assert json.loads(path.read_text(encoding="utf-8"))["note"] == "café — naïve ✓" +# --------------------------------------------- the out-of-tree events channel + + +def _seed_bare_run(project_dir, run_id="20260812-090000-bbbb"): + """A run dir with just enough state to collect: this test family is about + WHERE `summarize_files` looks, not about the canary sweep.""" + run_dir = project_dir / ".bmad-loop" / "runs" / run_id + save_state( + run_dir, + RunState( + run_id=run_id, + project=str(project_dir), + started_at="2026-08-12T09:00:00", + run_type="story", + ), + ) + return run_dir + + +def _events_group(run_dir, project_dir): + diag = diagnostics.collect( + [run_dir], pseudo=sanitize.Pseudonymizer(), project=Path(project_dir) + ) + return next((g for g in diag.runs[0].files if g.category == "events"), None) + + +def _write_events(events_dir, n, *, prefix=""): + events_dir.mkdir(parents=True, exist_ok=True) + for i in range(n): + (events_dir / f"{prefix}17550000{i}-t-Stop.json").write_text("{}", encoding="utf-8") + + +def test_events_are_counted_at_the_out_of_tree_state_root(project, tmp_path, monkeypatch): + """#494: the events channel left the project tree for the user state root, and + `_FILE_CATEGORIES` listed "events" as a run-dir-relative name. That made the + group VANISH silently rather than redden — the `is_dir()` guard turns a + category with no directory into a no-op, so the dump simply stopped mentioning + events at all, on exactly the runs a maintainer reads a dump to understand. + + The count is derived through the real `collect` path, not by handing + `summarize_files` the directory: what has to hold is that a collector given + only a run dir finds the channel, and the derivation (from the run's OWN + recorded project and id) is the part that can break. + + Ablation guard: making `_events_dir` return None, or dropping the primary from + `_category_roots`, makes this FAIL on the count.""" + from bmad_loop import envvars, runs + + monkeypatch.setenv(envvars.STATE_DIR, str(tmp_path / "state")) + run_dir = _seed_bare_run(project.project) + _write_events(runs.events_dir_for(project.project, run_dir.name), 3) + + group = _events_group(run_dir, project.project) + assert group is not None and group.count == 3 + assert group.total_bytes > 0 + # The in-tree location is genuinely empty — the count above came from the + # state root, not from a legacy directory a fixture happened to create. + assert not (run_dir / "events").exists() + + +def test_legacy_in_tree_events_still_counted_and_summed_with_the_primary( + project, tmp_path, monkeypatch +): + """Both roots are live at once. The hook relay is COPIED into the project by + `init`, so an upgraded orchestrator routinely drives sessions whose relay + predates the move and still writes in-tree; `SignalWatcher` dual-polls for + exactly that reason. A dump that counted only the primary would report zero + for such a session, which is the same blind spot in a different place. + + Ablation guard: dropping the legacy root from `_category_roots` makes this + FAIL (2 instead of 5).""" + from bmad_loop import envvars, runs + + monkeypatch.setenv(envvars.STATE_DIR, str(tmp_path / "state")) + run_dir = _seed_bare_run(project.project) + _write_events(runs.events_dir_for(project.project, run_dir.name), 3, prefix="new-") + _write_events(run_dir / "events", 2, prefix="old-") + + group = _events_group(run_dir, project.project) + assert group is not None + # ONE group, summed — the payload shape is the v1 schema and does not split. + assert group.count == 5 + + +def test_events_degrade_to_the_legacy_root_when_the_state_root_is_underivable( + project, tmp_path, monkeypatch +): + """A dump is routinely read on a machine that did not produce the run, and the + state root is a HOST fact. `runs.state_root()` raises rather than guessing when + no candidate answers (it is a write path); observation must not inherit that — + the events count is worth losing, the dump is not. + + Ablation guard: narrowing `_events_dir`'s except clause so the raise escapes + makes this FAIL — and the way it fails is the point. `collect` catches per + run, so the escape does not crash the dump; it demotes the WHOLE run to + `_unreadable_run`, losing its tasks, journal and every other file group to a + host fact that has nothing to do with the run. Silent, and far worse than the + count this degradation gives up.""" + from bmad_loop import envvars, runs + + monkeypatch.delenv(envvars.STATE_DIR, raising=False) + monkeypatch.setattr(runs, "state_root", _raise_no_state_root) + run_dir = _seed_bare_run(project.project) + _write_events(run_dir / "events", 2) + + group = _events_group(run_dir, project.project) + assert group is not None and group.count == 2 + + +def _raise_no_state_root(): + from bmad_loop.runs import StateRootError + + raise StateRootError("no state root on this host") + + +def test_state_root_path_is_redacted_in_the_dump(project, tmp_path, monkeypatch): + """#494 put a control-plane directory under the user's home, and every home is + an egress hazard: `/home//.local/state/bmad-loop/...` and + `C:\\Users\\\\AppData\\Local\\bmad-loop\\state\\...` both name a real + person. The dump only ever *stats* that directory, so no field carries it + today — this pins the two backstops that must hold if one ever does. + + Both spellings are asserted on ONE host on purpose: `_ABS_HOME_RE` is + platform-independent by construction (a Windows-shaped path is diagnosed on + POSIX whenever a Windows run's dump is read on Linux), and a rule that only + fired on its native platform would pass CI on the runner that never sees it. + Neither uses the host's own home, which `redact_home` would rewrite to `~` + before the rule was ever consulted — that would prove the wrong mechanism.""" + monkeypatch.setenv("BMAD_LOOP_STATE_DIR", str(tmp_path / "state")) + posix_root = "/home/canaryoperator/.local/state/bmad-loop/9f1c2d3e4a5b6c7d/r/events" + win_root = r"C:\Users\canaryoperator\AppData\Local\bmad-loop\state\9f1c2d3e4a5b6c7d\r\events" + + # The realistic vector: an unknown journal field. Unknown fields fall to + # `scrub_json`, so this half proves per-field routing catches the path. + for i, planted in enumerate((posix_root, win_root)): + run_dir = _seed_bare_run(project.project, run_id=f"20260812-09000{i}-cccc") + Journal(run_dir).append("events-routed", events_dir=planted) + _diag, _pseudo, combined = _render_all([run_dir]) + assert planted not in combined + assert "canaryoperator" not in combined + + # And this half proves the egress guard would have refused the path anyway — + # which is what actually covers a field nobody has written yet. It runs LAST + # and in its own loop: `_render_json_over` leaves `_to_jsonable` monkeypatched + # for the rest of the test, so a later `_render_all` would re-render this + # payload instead of its own run. + for planted in (posix_root, win_root): + with pytest.raises(diagnostics.LeakDetected) as exc: + _render_json_over(monkeypatch, {"events_dir": planted}) + assert "absolute-home-path" in exc.value.rules + + # The pure guard-mechanics tests (hard-rule refusal, repair tally, cyclic # termination) live in tests/test_sanitize.py since #199 made guard shared API; # this file keeps the integration surface: real collectors, real renders. From ff8967d5448d4bf1bf2841bf4cd5be6cadca4e11 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 18:34:19 -0700 Subject: [PATCH 05/11] feat(cleanup): collect the out-of-tree events channel with the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The events channel now lives under the user-scoped state root, so removing a run dir no longer removes everything the run owns — every delete/archive would leak `///` forever. - `delete_run`/`archive_run` gain a never-raise counterpart-removal tail (#139 doctrine): the run dir is already gone by then, so failing the operator's delete over an unnameable state root would report a removal that happened. Placed at the runs.py level, so every caller inherits it. `trim_run_dir` is deliberately untouched — a trimmed run is still resumable. - `clean` sweeps this project's orphans under the state root: entries with no run dir left to own them. Keyed on the run directory EXISTING, not on its state.json parsing, so a corrupt run an operator is recovering keeps its control plane. Symlinks are skipped and every path is containment-tested against the root (which is what catches a Windows junction, where `is_symlink` is False but `resolve()` follows). Unreadable runs dir sweeps nothing; a missing one sweeps everything, since that is `rm -rf .bmad-loop`. - `clean --json` gains `state_dirs_swept` as an additive field on the existing schema version, populated under `--dry-run` too. Their bytes stay out of `freed_bytes` (consumed event files, kilobytes at most). - Docs name one truth: archived tarballs no longer contain `events/`; the out-of-tree move eliminates the in-tree redirect class and binds a sandboxed session, but not a bypass-permissions one (the env var necessarily names the dir) — which is why the relay keeps `_is_link_like`. Known limit documented: a moved/renamed project re-keys, stranding its old subtree. Every gate above was ablated singly (13 rows, rc==1 required). --- CHANGELOG.md | 56 +++---- README.md | 3 + docs/FEATURES.md | 11 +- docs/setup-guide.md | 16 +- src/bmad_loop/cli.py | 19 ++- src/bmad_loop/data/settings/core.toml | 2 +- src/bmad_loop/documents.py | 11 ++ src/bmad_loop/runs.py | 133 ++++++++++++++- tests/test_cleanup.py | 96 +++++++++++ tests/test_runs.py | 229 ++++++++++++++++++++++++++ 10 files changed, 529 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cde0dff0..4d360f25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,41 +14,27 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Added -- **`bmad-loop relay `: write a session event without the copied-in script (#494).** The - hardened event write gains an importable twin, `events.py`, held byte-identical to the stdlib-only - hook relay by an AST parity test — the two writers of the events control plane can no longer be - hardened differently by accident. The new `relay` subcommand takes the same hook payload on stdin - and honours the same contract: nothing on stdout, rc 0 always, a silent no-op outside a driven - session. It dispatches ahead of `main()`'s shared error handler and its mux configuration, so - neither a broken `policy.toml` nor an unexpected exception can turn a session's Stop signal into a - failed hook. First phase of moving `events/` to a control-plane root outside the project tree. - - The root itself is a user-scoped state directory (`$XDG_STATE_HOME/bmad-loop` or - `~/.local/state/bmad-loop`; `%LOCALAPPDATA%\bmad-loop\state` on Windows), keyed - `///` by the same project identity that scopes session ownership, so two - spellings of one project cannot end up with two control planes. `BMAD_LOOP_STATE_DIR` overrides - the whole cascade for a host where none of those is derivable or writable. - - **The events channel now lives there**, at `///events/` — out of the - project tree, where a branch switch, a worktree mount or a rollback cannot take a live run's - control plane away. Every engine-driven session is told the directory through - `BMAD_LOOP_EVENTS_DIR`, and both relays (the copied hook script and `bmad-loop relay`) prefer it, - falling back to the legacy in-tree `/events`. The orchestrator keeps polling that legacy - location too, and the fallback pair is load-bearing rather than tidy: the hook script is COPIED - into the project by `init`, so an upgraded orchestrator routinely drives sessions whose relay - predates the move — without both halves every such session would observe no Stop and stall to - `session_timeout_min`. Re-run `bmad-loop init` to refresh the relay. `--dry-run` previews the - directory the run would use. - - Both observation surfaces are honest about the split. `validate` gains `hooks.relay-stale`: a - present, readable relay is not necessarily a current one, so the installed copy is compared - against the packaged source and a difference is reported with the `init` that refreshes it. It is - a **warning** and never moves the exit code — the fallback keeps a stale relay working, so this - reports a lost property, not a broken run. `diagnose`'s `events` file group is retargeted to the - state root and sums both roots into the one category (unchanged payload, no schema bump); it had - silently reported nothing after the move, since a category with no directory is a no-op. An - underivable state root — a dump read on a machine that did not produce the run — costs the events - count and nothing else. +- **The hook-event channel moves out of the project tree (#494).** A run's session-completion + signals now land under a user-scoped state root — `$XDG_STATE_HOME/bmad-loop` (else + `~/.local/state/bmad-loop`), `%LOCALAPPDATA%\bmad-loop\state` on Windows, or wherever + `BMAD_LOOP_STATE_DIR` points — keyed `///events/`, so a branch switch, a + worktree mount or a rollback can no longer take a live run's control plane away. + + - **Older relays keep working.** Sessions are told the directory via `BMAD_LOOP_EVENTS_DIR`; both + relays fall back to the in-tree `/events` and the orchestrator polls both locations. + `init` copies the relay into the project, so an upgraded orchestrator regularly drives sessions + whose relay predates the move — re-run `bmad-loop init` to refresh it. + - **`bmad-loop relay `** writes a session event without the copied-in script, on the same + contract (nothing on stdout, rc 0 always, silent no-op outside a driven session). It is backed + by a new `events.py`, held byte-identical to the stdlib-only relay by an AST parity test, and + dispatches ahead of the shared error handler so a broken `policy.toml` cannot fail a hook. + - **`validate` gains `hooks.relay-stale`** — the installed relay compared against the packaged + one, a warning that never moves the exit code (the fallback keeps a stale relay working). + `diagnose`'s `events` group now counts both locations; payload and schema unchanged. + - **`delete`/`archive`/`clean` collect the out-of-tree dir** with the run, and `clean` sweeps + orphans whose run dir is already gone (`--json`: `state_dirs_swept`, an additive field). An + archived tarball therefore no longer contains `events/` — consumed transient signals. + - `run`/`sweep` `--dry-run` previews the events directory a session would use. - **Coding-CLI adapter registry: a new adapter class ships out-of-tree (#226).** The transport axis has long been extensible out-of-tree; the CLI axis had no equivalent, so a CLI needing its own diff --git a/README.md b/README.md index f38757da..8e8f0740 100644 --- a/README.md +++ b/README.md @@ -570,6 +570,7 @@ A handful of `BMAD_LOOP_*` variables override behavior at runtime, taking preced | `BMAD_LOOP_MUX_BACKEND` | registered backend name (e.g. `tmux`, `psmux`) | Forces the terminal-multiplexer backend, outranking the `[mux] backend` policy key and auto-selection. A name matching no registered backend is an error — it never silently falls back. Unset ⇒ auto-select. | | `BMAD_LOOP_PROCESS_HOST` | registered host name (e.g. `posix`, `windows`) | Forces the process-lifecycle host (an override/test hook). A name matching no registered host raises rather than silently using POSIX. Unset ⇒ this platform's default. | | `BMAD_LOOP_STATE_DIR` | directory path | Overrides the user-scoped **state root** — the out-of-tree home of per-run control-plane state, keyed `///`. Used as the root itself, so nothing is appended to it. Unset ⇒ `$XDG_STATE_HOME/bmad-loop` when that names an absolute path, else `~/.local/state/bmad-loop`; on Windows `%LOCALAPPDATA%\bmad-loop\state`, else `%USERPROFILE%\AppData\Local\bmad-loop\state`. Set this when none of those is derivable or writable (a home on a network share, a locked-down service account). | +| `BMAD_LOOP_EVENTS_DIR` | directory path | **Session protocol, not an operator knob.** The orchestrator exports it into every session it drives, naming that run's hook-event directory under the state root; the hook relay writes there, falling back to the legacy in-tree `/events` when it is absent. Setting it yourself in a shell has no effect on a run (the engine overwrites it per session) and only misdirects a hand-invoked relay. | | `BMAD_LOOP_SESSION_TIMEOUT_S` | seconds (float) | Overrides the per-session wall-clock budget (normally `limits.session_timeout_min × 60`) — mainly a test/E2E hook for sub-minute timeouts. A value that is not a finite positive number is ignored — non-positive, unparseable, or non-finite (`inf`, `1e999`), the last of which would otherwise disable the timeout outright. A large finite value is honoured. Unset ⇒ the policy value. | Game-engine (Unity) runs read a wider `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` set documented in the [Game Engine MCP guide](docs/game-engine-mcp-guide.md). @@ -580,6 +581,8 @@ Everything about a run lives in `.bmad-loop/runs//` (gitignored): `state One piece deliberately lives elsewhere: the **hook-event channel** (the session completion signals the orchestrator waits on) sits under the user-scoped state root at `///events/`, outside the project tree — a branch switch, a worktree mount or a rollback must not be able to take a live run's control plane away. See `BMAD_LOOP_STATE_DIR` above for where that root resolves. The orchestrator also keeps polling the old in-tree `events/` location, so a project whose installed hook relay predates the move still completes its sessions; re-run `bmad-loop init` to refresh the relay. +That out-of-tree directory is collected with the run: `delete`, `archive` and `clean` remove it alongside the run dir, and `clean` also sweeps this project's orphans there — control planes whose run dir is already gone, e.g. from a hand-removed run (`clean --dry-run` previews the count; `--json` reports it as `state_dirs_swept`). Two consequences worth knowing: an archived run's tarball no longer contains `events/` (transient completion signals, consumed while the run was live), and a project that is deleted, moved or renamed leaves its old subtree behind — the key is derived from the project's resolved path, so after a move the project itself now keys somewhere new and nothing can name the old key to sweep it. Remove it by hand if you care; it is events-sized, not run-sized. + A run can be stopped two ways. A **hard stop** (`bmad-loop stop`, TUI `x`, Ctrl+C) SIGTERMs the engine mid-item and always kills the agent session. A **graceful stop** (`bmad-loop stop --graceful`, TUI `S`) instead writes `stop-request.json` — no signal, so it works on every platform and multiplexer backend — which the engine consumes at the next item boundary: the in-flight story (or sweep bundle) finishes through commit — or, mid-triage, the sweep's triage session completes and no bundles start — the run finalizes as `stopped` (not `finished`) and stays resumable, and pending auto-sweeps are suppressed. Its session teardown follows `cleanup_session_on_finish` like a normal finish, rather than the hard stop's unconditional kill; a hard stop always supersedes a pending graceful request. `journal.jsonl` records a `session-end` for every session unconditionally — even a teardown that throws still lands one (status `aborted` when the outcome is unknowable). A timed-out session's entry carries `fired_at` (wall time the deadline was declared elapsed), `teardown_s` (wall seconds from that fire to this entry — the teardown gap), and `expired_clock` (`monotonic` / `wall` / `both`); `wall` alone fingerprints a host suspend (e.g. macOS sleep) that froze the monotonic clock. Every entry whose usage was read also carries `tokens` (raw) and `tokens_weighted` (cache reads at `limits.cache_read_weight`), keeping per-session spend reconstructible; both are `null` when the read failed and absent on an `aborted` end. `tokens_weighted` is the end-of-session total, distinct from a tripped session's `budget_weighted`, the guard's mid-session sample at trip time. Per-session `tokens_weighted` sums to within a token or two of the run total, which rounds per story rather than per session. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 32a08463..f4e18b27 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -103,12 +103,14 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Every run is a resumable on-disk state machine: `bmad-loop resume ` continues from a gate, escalation, or interruption. - A graceful stop (`stop --graceful` / TUI `S`) is resumable too: unlike a hard stop killed mid-item, it lets the in-flight item finish through commit and finalizes cleanly, ending as a `stopped` run that `resume` picks up at the next item. - All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194), a mux session lost under the run (`session-vanished`, #489) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`; `ctl-window` (the control-session window id the last TUI launch minted, so attach/stop follow the live window, #482). +- One piece deliberately lives **outside** that directory: the hook-event channel (#494) is at `///events/` under the user-scoped state root (`BMAD_LOOP_STATE_DIR`, see the [transport section](#hook-based-transport-no-pane-scraping) below and the README's env-var table), not `/events/`. The orchestrator still polls the legacy in-tree location, so a project whose installed relay predates the move keeps completing its sessions. `delete`, `archive` and `clean` remove the out-of-tree counterpart along with the run dir, and `clean` sweeps counterparts whose run dir is already gone; an **archived** run's tarball therefore no longer contains `events/` — those files are transient completion signals, consumed while the run was live, and everything an archive is read for later is in the run dir. - `journal.jsonl` records `session-end` for every session unconditionally — even a teardown that throws still lands one (status `aborted` when the outcome is unknowable). A timed-out session's entry carries `fired_at` (wall time the deadline was declared), `teardown_s` (wall seconds from that fire to this entry — the teardown gap), and `expired_clock` (`monotonic` / `wall` / `both` — `wall` alone fingerprints a host suspend that froze the monotonic clock). Every entry whose usage was read carries `tokens` (raw) and `tokens_weighted` (cache reads at `limits.cache_read_weight`), keeping per-session spend reconstructible; both are `null` when the usage read failed, and both are absent on an `aborted` end. `tokens_weighted` is the end-of-session total — distinct from a tripped session's `budget_weighted`, the guard's mid-session sample at trip time. ### Hook-based transport (no pane-scraping) - Coding-agent hooks (`Stop` / `SessionStart` / `SessionEnd` / `PreCompact`) write structured event files the orchestrator watches; skills write a machine-readable `result.json`. - The event channel lives **outside the project tree** (#494), at `///events/` — a branch switch, a worktree mount or a rollback must not be able to take a live run's control plane away. Each session is told where to write via `BMAD_LOOP_EVENTS_DIR`; the state root itself resolves per `BMAD_LOOP_STATE_DIR` (see the env-var table in the README). The relay falls back to the legacy in-tree `/events` when that variable is absent, and the orchestrator keeps polling that location too — the hook script is copied into the project by `init`, so an upgraded orchestrator regularly drives sessions whose relay predates the move, and without both halves every such session would stall to `session_timeout_min`. +- What the move is and is not worth: it eliminates the whole in-tree redirect class — nothing an agent writes inside the project can any longer point the completion channel somewhere else — and it is a hard boundary for a sandboxed session, which cannot reach outside the tree at all. It is **not** a boundary against a session running with permissions bypassed: that session is told the directory by `BMAD_LOOP_EVENTS_DIR`, so it can reach it by construction. This is why the relay keeps its own `_is_link_like` refusal on the write path (#493) as belt-and-braces rather than retiring it as redundant. ### Deferred-work sweeps @@ -179,8 +181,9 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Disk reclamation (`[cleanup]`) - `bmad-loop clean` reclaims **disk** (distinct from `cleanup`, which is only tmux). It tears down git worktrees a mid-flight stop left mounted — the main accumulation source: each carries a real Unity `Library/` (incl. the MCP-server build), which `git worktree remove` cannot reach once the engine was killed before teardown. It then trims the heavy `worktrees/` tree from runs kept for history (the run still lists in the dashboard — discovery reads `state.json`, not the worktree), and archives or deletes runs past the retention window. +- It also collects the **out-of-tree** half of a run. Removing a run dir no longer removes everything the run owns (#494), so `delete`/`archive`/`clean` remove the run's control-plane dir under the state root too, and `clean` additionally sweeps this project's orphans there — subtrees whose run dir is gone, from a hand-removed run or a delete that predates this. The sweep keys on the run directory _existing_, not on its `state.json` parsing, so a corrupt run an operator is trying to recover keeps its control plane; a trimmed run keeps its own for the same reason (it is still resumable). Their bytes are not in the reclaim estimate — a state dir holds consumed event files and little else. Known limit: the state root is keyed by the project's resolved path, so a project that is deleted, moved or renamed leaves its old subtree unsweepable — after a move the project keys somewhere new, and no project can name the old key. - Safe by construction: only **finished or stopped** runs are touched; running, unknown-host, paused and interrupted (resumable) runs are never reclaimed. `--keep ` protects a specific run (e.g. a finished one whose Editor is still live), `--dry-run` previews, `--retain N`/`--hard` tune the window and archive-vs-delete. -- `--json` emits a stable machine-readable document per the [contract below](#machine-readable-output---json) (schema-versioned; the effective retention policy, `freed_bytes` as a raw integer, and the paths and run ids under `worktrees`/`trimmed`/`archived`/`deleted`/`protected`) instead of the text. Plan and outcome share one schema, with `dry_run` saying which one you are holding, so a script can pre-flight a reclaim and compare it against what happened — though values are each invocation's own sample, not a promise the two agree. It names every item the text only counts or renders, and the unverifiable-pid warning text mode writes to stderr becomes `unverifiable_pid` in the document, leaving stderr empty. +- `--json` emits a stable machine-readable document per the [contract below](#machine-readable-output---json) (schema-versioned; the effective retention policy, `freed_bytes` as a raw integer, and the paths and run ids under `worktrees`/`trimmed`/`archived`/`deleted`/`protected`, and `state_dirs_swept` as a count) instead of the text. Plan and outcome share one schema, with `dry_run` saying which one you are holding, so a script can pre-flight a reclaim and compare it against what happened — though values are each invocation's own sample, not a promise the two agree. It names every item the text only counts or renders, and the unverifiable-pid warning text mode writes to stderr becomes `unverifiable_pid` in the document, leaving stderr empty. - Prevention is automatic: every `run`/`sweep` start reconciles worktrees leaked by a prior **finished** run (`[cleanup] auto_clean_on_finish`), and the Unity plugin's `post_run` hook removes the IvanMurzak MCP server's downloaded `/tmp///*.zip` and truncates its unbounded editor log (`[cleanup] clean_tmp`). For recurring housekeeping of stopped runs, schedule `bmad-loop clean`. ### Setup & install @@ -207,11 +210,11 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - `bmad-loop diagnose []` (`diag`) — emit a sanitized diagnostic dump of a run/sweep to hand maintainers (histograms, counts, env, file sizes — no code/spec/prompts/paths/PII); a stray pseudonymized identifier is auto-substituted with its alias (disclosed in the report), while PII/secret hits still refuse to emit; defaults to the latest run (`--all`, `--out`, `--max-journal-entries`). `--json` emits the same dump as a stable machine-readable document per the [contract below](#machine-readable-output---json) instead of the markdown report. - `bmad-loop attach []` — tmux-attach to a run's live agent session. - `bmad-loop stop ` — stop a live run. The default is a **hard stop**: SIGTERM the engine mid-item and kill its agent session. `--graceful` instead requests a **graceful stop** — the engine finishes the in-flight item (a story through commit, a sweep bundle through commit, or an in-progress sweep triage — after which no bundles start), then finalizes cleanly and stops as a resumable `stopped` run, suppressing any pending auto-sweeps; `--cancel-graceful` withdraws a pending request. Delivery is a `stop-request.json` control file consumed at the next item boundary (no signal, so it works on every platform and multiplexer backend); a hard stop always supersedes a pending graceful one. The TUI surfaces the same pair: `x` hard-stops, `S` requests a graceful stop. -- `bmad-loop delete ` — delete a run directory (`--force` stops it first if live). -- `bmad-loop archive ` — compress a run into `.bmad-loop/archive` and remove it (`--force` stops it first if live). +- `bmad-loop delete ` — delete a run directory and its out-of-tree control-plane dir (`--force` stops it first if live). +- `bmad-loop archive ` — compress a run into `.bmad-loop/archive` and remove it, control-plane dir included (`--force` stops it first if live). The tarball holds the run dir, so it carries no `events/`. - Removal refuses while a matching agent session is live that the project cannot prove is another one's, even when the engine is dead: for an untagged session the run dir is the last ownership proof `cleanup` can read, so removing it would leak the session ([#419](https://github.com/bmad-code-org/bmad-loop/issues/419)). A session tagged to another project carries its own proof and never blocks. Run `cleanup` first, having confirmed the session is this project's (`attach`): for an _untagged_ session `cleanup` proves ownership by that same run dir, so two projects sharing a run id can prune each other's. Or pass `--force`, which removes anyway and kills nothing. `clean` leaves such a run untouched and reports it as protected. - `bmad-loop cleanup` — remove leftover tmux artifacts for finished/stopped runs. `--json` emits the sessions and ctl windows removed (or, with `--dry-run`, that would be) as a stable machine-readable document per the [contract below](#machine-readable-output---json). -- `bmad-loop clean` — reclaim disk from concluded runs per `[cleanup]`: tear down worktrees a mid-flight stop orphaned, trim heavy `worktrees/` from runs kept for history, archive/delete past the retention window (`--dry-run`, `--keep`, `--retain N`, `--hard`). `--json` emits what was reclaimed (or would be) as a stable machine-readable document per the [contract below](#machine-readable-output---json), with `freed_bytes` a raw integer. +- `bmad-loop clean` — reclaim disk from concluded runs per `[cleanup]`: tear down worktrees a mid-flight stop orphaned, trim heavy `worktrees/` from runs kept for history, archive/delete past the retention window, and sweep orphaned run control-plane dirs from the out-of-tree state root (`--dry-run`, `--keep`, `--retain N`, `--hard`). `--json` emits what was reclaimed (or would be) as a stable machine-readable document per the [contract below](#machine-readable-output---json), with `freed_bytes` a raw integer. - `bmad-loop tui` — the interactive dashboard (`--low-frame-rate` for slow/SSH links). - `bmad-loop probe-adapter ` (`collect-adapter-data`) — collect + sanitize adapter-finalization data for a CLI profile; default zero-launch scan, opt-in `--probe` live capture. - Every command takes `--project ` (default: current directory). Any `` accepts a partial — the tail after the last `-`, shortened to any unique prefix. diff --git a/docs/setup-guide.md b/docs/setup-guide.md index ca10d4d6..b86f8324 100644 --- a/docs/setup-guide.md +++ b/docs/setup-guide.md @@ -331,7 +331,9 @@ bmad-loop cleanup --project # kill leftover tmux sessions ``` `clean --hard` permanently deletes runs instead of archiving them (we're removing the tool, so -there's nothing to keep). See the disk-reclamation coverage in +there's nothing to keep). Running it **first** also matters for the out-of-tree half: each run's +control-plane directory lives under the user-scoped state root, and `clean` is what collects it +(step 2 covers what is left over). See the disk-reclamation coverage in [docs/FEATURES.md](FEATURES.md) and the [command reference](../README.md#command-reference) for what each command touches. Make sure no run is still live (Editor open, session attached) first. @@ -345,6 +347,18 @@ Delete the `.bmad-loop/` directory. This removes the hook relay script rm -rf .bmad-loop/ ``` +One piece is **not** under `.bmad-loop/`: each run's hook-event channel lives in a user-scoped +state root outside the project (#494) — `$XDG_STATE_HOME/bmad-loop`, else +`~/.local/state/bmad-loop`; `%LOCALAPPDATA%\bmad-loop\state` on Windows; or whatever +`BMAD_LOOP_STATE_DIR` names. Step 1 collected this project's runs from it, leaving an empty +per-project directory whose name is a digest of the project path — not something to pick out by +hand. That root is shared by **every** project on this machine, so delete the whole thing only if +you are removing bmad-loop everywhere: + +```bash +rm -rf "${XDG_STATE_HOME:-$HOME/.local/state}/bmad-loop" # all projects on this machine +``` + ### 3. Remove the bundled skills `init` installed the three bundled `bmad-loop-*` skill directories — delete only those. **Leave diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 49fd820e..8120adab 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -3385,6 +3385,13 @@ def cmd_clean(args: argparse.Namespace) -> int: freed += wt_bytes trimmed.append(run_dir.name) + # After the loop, so the counterparts the removals above already took are gone + # from the enumeration rather than counted twice. Their bytes stay out of + # `freed` on purpose: a state dir holds consumed event files and nothing else, + # so sizing every one of them would buy kilobytes of accuracy for a walk of a + # second tree. The count is the honest report of what went. + swept = len(runs.reconcile_orphan_state_dirs(project, dry_run=dry)) + if args.json: machine.emit( clean_document( @@ -3398,12 +3405,12 @@ def cmd_clean(args: argparse.Namespace) -> int: deleted=deleted, protected=protected, unverifiable_pid=unverifiable, + state_dirs_swept=swept, ) ) return 0 - if not worktrees and not trimmed and not archived and not deleted: - print("nothing to reclaim") - else: + reclaimed = bool(worktrees or trimmed or archived or deleted) + if reclaimed: head = "would reclaim" if dry else "reclaimed" print( f"{head} ~{_human_bytes(freed)}: {len(worktrees)} worktree(s), " @@ -3413,6 +3420,12 @@ def cmd_clean(args: argparse.Namespace) -> int: print(f" archived {name} -> .bmad-loop/archive/{name}.tar.gz") for name in deleted: print(f" deleted {name}") + if swept: + # its own line rather than a fifth count in the summary above, which is a + # reclaimed-bytes report these dirs are deliberately outside of + print(f"{'would sweep' if dry else 'swept'} {swept} orphaned run state dir(s)") + if not reclaimed and not swept: + print("nothing to reclaim") if protected: print(f"left {len(protected)} live/resumable run(s) untouched") return 0 diff --git a/src/bmad_loop/data/settings/core.toml b/src/bmad_loop/data/settings/core.toml index fab18aad..c4571a3e 100644 --- a/src/bmad_loop/data/settings/core.toml +++ b/src/bmad_loop/data/settings/core.toml @@ -374,7 +374,7 @@ description = "⚠ ON: capture failed-unit diffs with NO size limit (overrides t [[section]] name = "cleanup" -description = "disk reclamation for .bmad-loop/runs (terminal runs only)" +description = "disk reclamation for .bmad-loop/runs, plus each run's out-of-tree control-plane dir (terminal runs only)" [[section.field]] key = "run_retention" kind = "int" diff --git a/src/bmad_loop/documents.py b/src/bmad_loop/documents.py index 58d66f3a..2c14a09f 100644 --- a/src/bmad_loop/documents.py +++ b/src/bmad_loop/documents.py @@ -454,6 +454,7 @@ def clean_document( deleted: list[str], protected: list[str], unverifiable_pid: list[str], + state_dirs_swept: int, ) -> dict[str, object]: """The `clean --json` document: the disk this invocation reclaimed, or — under ``--dry-run`` — would reclaim. @@ -483,6 +484,15 @@ def clean_document( `[cleanup] run_retention`. The other three are the configured policy as loaded. Note `--hard` overrides `archive_old` for this invocation only, so it does not change the reported value; the outcome shows in `deleted`. + + `state_dirs_swept` counts the orphaned out-of-tree control-plane dirs the + invocation reclaimed (#494) — run state dirs under the user-scoped state root + with no run dir left to own them. A count rather than a list, because that is + exactly what the text mode reports and the paths name a location outside the + project that no caller acts on per-item. It is an additive field on the + existing schema version: a v1 consumer reads every field it already knew. + Those dirs hold only consumed event files, so their bytes are not in + `freed_bytes` — an accepted under-count of a few kilobytes at most. """ return { "schema_version": CLEAN_SCHEMA_VERSION, @@ -500,4 +510,5 @@ def clean_document( "deleted": list(deleted), "protected": list(protected), "unverifiable_pid": list(unverifiable_pid), + "state_dirs_swept": state_dirs_swept, } diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index a2cfd090..803c577a 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -266,6 +266,15 @@ def state_root() -> Path: ) +def project_state_root(project: Path) -> Path: + """The subtree of :func:`state_root` holding every run of this project: + ``/``. Split out from :func:`state_dir_for` because + the GC reads it as a *directory to enumerate* rather than composing one run's + path — see :func:`reconcile_orphan_state_dirs`, whose whole job is the entries + under here that no longer have a run dir.""" + return state_root() / project_tag(project) + + def state_dir_for(project: Path, run_id: str) -> Path: """This run's control-plane directory: ``//``. @@ -284,7 +293,7 @@ def state_dir_for(project: Path, run_id: str) -> Path: :func:`is_valid_run_id`, and an id from outside is rejected there rather than coerced here. """ - return state_root() / project_tag(project) / run_id + return project_state_root(project) / run_id def events_dir_for(project: Path, run_id: str) -> Path: @@ -776,6 +785,35 @@ def _refuse_live_session(project: Path, run_id: str, verb: str) -> None: ) +def _discard_state_dir(project: Path, run_id: str) -> None: + """Remove the run's out-of-tree control-plane counterpart, best-effort. + + The events channel (#494) lives outside the project tree, so removing a run + dir no longer removes everything the run owns: without this every + delete/archive would leak ``///`` forever. It + lives here rather than in the CLI so every caller inherits it — `delete`, + `archive`, `clean`, the TUI's removal actions and the engine's own + finish-time reclamation alike. + + A **never-raise tail**, per the teardown doctrine (#139): the run dir is + already gone by the time this runs, and failing the operator's delete over an + unreachable state root would report a removal that in fact happened. The two + catchable outcomes are both "the counterpart could not even be named" — + :class:`StateRootError` for an environment with no derivable root, ``OSError`` + for a project path the OS cannot canonicalize (:func:`project_tag` resolves + before digesting). Removal failures are absorbed by ``ignore_errors``. Either + way the orphan sweep in :func:`reconcile_orphan_state_dirs` is the backstop. + + Deliberately not called by :func:`trim_run_dir`: a trimmed run is still live + on disk and resumable, and its control plane must outlive the scaffolding. + """ + try: + target = state_dir_for(project, run_id) + except (StateRootError, OSError): + return + shutil.rmtree(target, ignore_errors=True) + + def delete_run(project: Path, run_dir: Path, *, force: bool = False) -> None: """Permanently remove a run directory. Callers enforce the engine-liveness guard; the session guard is enforced here (see :func:`_refuse_live_session`), @@ -789,6 +827,9 @@ def delete_run(project: Path, run_dir: Path, *, force: bool = False) -> None: if not force: _refuse_live_session(project, run_dir.name, "delete") shutil.rmtree(run_dir) + # after the run dir, never before: a raise above leaves the run whole, and a + # whole run keeps its control plane (see _discard_state_dir). + _discard_state_dir(project, run_dir.name) def archive_run(project: Path, run_dir: Path, *, force: bool = False) -> Path: @@ -797,7 +838,13 @@ def archive_run(project: Path, run_dir: Path, *, force: bool = False) -> Path: place so a partial archive never appears. Callers enforce the engine-liveness guard; the session guard is enforced here (see :func:`_refuse_live_session`, and :func:`delete_run` for ``force``) and runs before the tarball is written, - so a refusal leaves nothing behind.""" + so a refusal leaves nothing behind. + + The tarball holds the run dir only, so since #494 an archive no longer carries + the run's ``events/``: the channel moved out of the tree, and its files are + transient completion signals the watcher has already consumed — the recorded + decision accepts losing them from the archive. Everything an archive is read + for later (state, journal, tasks, logs) is in the run dir and unaffected.""" if not force: _refuse_live_session(project, run_dir.name, "archive") archive_dir = project / ARCHIVE_DIR @@ -808,6 +855,7 @@ def archive_run(project: Path, run_dir: Path, *, force: bool = False) -> Path: tar.add(run_dir, arcname=run_dir.name) atomic_replace(tmp, dest) shutil.rmtree(run_dir) + _discard_state_dir(project, run_dir.name) # same tail as delete_run return dest @@ -897,10 +945,89 @@ def reconcile_stale_worktrees(repo: Path, project: Path, *, dry_run: bool = Fals return handled +def _run_dir_names(project: Path) -> set[str] | None: + """Every *directory name* under the runs dir, or ``None`` when that listing + could not be taken. + + Deliberately not :func:`list_run_dirs`, which is ``state.json``-gated: this + answers "does a run dir by this name exist", and a run whose ``state.json`` is + missing or corrupt still owns its control plane. Gating on state.json would + sweep the counterpart out from under exactly the run an operator is trying to + recover. + + The two failures are distinguished because they mean opposite things. A + *missing* runs dir is a real answer — no runs, so nothing is live — while an + unreadable one answers nothing at all, and a sweep run against "no live names" + would remove every state dir this project has. ``None`` is that second case. + """ + try: + return {entry.name for entry in os.scandir(project / RUNS_DIR) if entry.is_dir()} + except FileNotFoundError: + return set() + except OSError: + return None + + +def reconcile_orphan_state_dirs(project: Path, *, dry_run: bool = False) -> list[Path]: + """Remove this project's out-of-tree control-plane dirs whose run dir is gone. + + The GC backstop for the events channel (#494). :func:`_discard_state_dir` + removes the counterpart on every ordinary delete/archive, so this catches what + that path could not: a run dir removed by hand or by an `rm -rf .bmad-loop`, + a delete that ran before this version existed, and any tail that failed + quietly. Without it the state root accumulates one dead subtree per run + forever, on a path outside the project that no operator thinks to look at. + + Shaped like :func:`reconcile_orphan_worktrees`: enumerate on-disk truth, + containment-test each path, remove with failures tolerated. Returns what was + removed (or, under ``dry_run``, what would be). + + Every path is built from an entry name this function itself enumerated — + never from a caller-supplied ref, which is what :func:`_is_path_escape` + refuses on the ref-resolution path. Entries that are not real directories are + skipped, symlinks included: a link is not a state dir we created, and + reporting one swept would be a false count even where ``rmtree`` refuses it. + The containment test then covers what ``is_symlink`` cannot — a Windows + *junction* reads as a plain directory while ``resolve()`` follows it, so + without the test ``rmtree`` would empty a target sitting outside the root. + That case is POSIX-invisible, and the tests say so rather than claim it. + + Degrades to no-op rather than raising, in either direction: an underivable + state root, an unreadable root, or an unreadable runs dir all sweep nothing. + This is reclamation, not repair — leaving disk behind is the cheap outcome, + and removing a live run's control plane is not. + """ + live = _run_dir_names(project) + if live is None: + return [] + try: + root = project_state_root(project) + entries = sorted(root.iterdir()) + root_res = root.resolve() + except (StateRootError, OSError): + return [] + handled: list[Path] = [] + for entry in entries: + if entry.name in live or entry.is_symlink() or not entry.is_dir(): + continue + try: + entry.resolve().relative_to(root_res) + except (OSError, ValueError): + continue + handled.append(entry) + if not dry_run: + shutil.rmtree(entry, ignore_errors=True) + return handled + + def trim_run_dir(run_dir: Path, *, dry_run: bool = False) -> list[Path]: """Delete heavy scaffolding (the ``worktrees/`` tree) from a concluded run dir, preserving its TUI-visible core so the run still appears in the - dashboard with full status/journal/logs. Returns the paths removed.""" + dashboard with full status/journal/logs. Returns the paths removed. + + The run's out-of-tree control plane is deliberately left alone (see + :func:`_discard_state_dir`): a trimmed run still exists and is still + resumable, so its state dir has to outlive its scaffolding.""" removed: list[Path] = [] for name in _HEAVY_RUN_ENTRIES: p = run_dir / name diff --git a/tests/test_cleanup.py b/tests/test_cleanup.py index 6895238d..269a228d 100644 --- a/tests/test_cleanup.py +++ b/tests/test_cleanup.py @@ -526,5 +526,101 @@ def test_cmd_clean_json_nothing_to_reclaim_is_a_valid_empty_document(project, ca assert doc["schema_version"] == cli.CLEAN_SCHEMA_VERSION assert doc["freed_bytes"] == 0 + assert doc["state_dirs_swept"] == 0 for key in ("worktrees", "trimmed", "archived", "deleted", "protected", "unverifiable_pid"): assert doc[key] == [], key + + +# ------------------------------------------- cmd_clean: out-of-tree state (#494) + + +def _seed_state_dir(project, run_id): + events = runs.events_dir_for(project, run_id) + events.mkdir(parents=True) + (events / "1700000000-t1-Stop.json").write_text("{}") + return runs.state_dir_for(project, run_id) + + +def test_cmd_clean_removes_the_state_counterpart_of_a_deleted_run(project): + """The whole reason `clean` needed changing: past the retention window it + removes the run dir, and since #494 the run's control plane is no longer + inside it. Asserted through the command rather than `delete_run` alone, since + `clean` is the path an operator schedules and the only one that runs + unattended.""" + install_bmad_config(project) + repo = project.project + run_dir = repo / ".bmad-loop" / "runs" / "20260101-000000-aaaa" + save_state(run_dir, RunState(run_id="r", project=str(repo), started_at="x", finished=True)) + state_dir = _seed_state_dir(repo, "20260101-000000-aaaa") + + assert cli.cmd_clean(_clean_args(repo, retain=0, hard=True)) == 0 + + assert not run_dir.exists() + assert not state_dir.exists() + + +def test_cmd_clean_keeps_the_state_counterpart_of_a_run_it_only_trimmed(project): + """A trimmed run is still on disk and still resumable, so its control plane + has to survive its scaffolding. Two paths could take it: `trim_run_dir` (which + deliberately does not) and the orphan sweep (whose live-name check is what + keeps it). This grades them together, at the level where a mistake in either + would strand a resumable run's completion channel.""" + install_bmad_config(project) + repo = project.project + run_dir = repo / ".bmad-loop" / "runs" / "20260101-000000-aaaa" + (run_dir / "worktrees" / "u").mkdir(parents=True) + save_state(run_dir, RunState(run_id="r", project=str(repo), started_at="x", stopped=True)) + state_dir = _seed_state_dir(repo, "20260101-000000-aaaa") + + assert cli.cmd_clean(_clean_args(repo)) == 0 + + assert run_dir.is_dir() and not (run_dir / "worktrees").exists() # trimmed, not removed + assert state_dir.is_dir() + + +def test_cmd_clean_sweeps_an_orphaned_state_dir_and_reports_it(project, capsys): + """The backstop reaching a leak `clean` did not create: a run dir removed by + hand leaves a control plane nothing else will ever collect. The text mode says + so on its own line — the reclaim summary above it is a byte estimate these + dirs are deliberately outside of.""" + install_bmad_config(project) + repo = project.project + orphan = _seed_state_dir(repo, "20260101-000000-aaaa") + + assert cli.cmd_clean(_clean_args(repo)) == 0 + + assert not orphan.exists() + out = capsys.readouterr().out + assert "swept 1 orphaned run state dir(s)" in out + assert "nothing to reclaim" not in out # something WAS reclaimed + + +def test_cmd_clean_dry_run_plans_the_sweep_without_removing(project, capsys): + """Plan and outcome share one shape: the preview names the same work the real + run would do, and provably leaves the disk alone.""" + install_bmad_config(project) + repo = project.project + orphan = _seed_state_dir(repo, "20260101-000000-aaaa") + + assert cli.cmd_clean(_clean_args(repo, dry_run=True)) == 0 + + assert orphan.is_dir() + assert "would sweep 1 orphaned run state dir(s)" in capsys.readouterr().out + + +def test_cmd_clean_json_carries_the_sweep_count_in_both_modes(project, capsys): + """The additive field, on the existing schema version (nothing a v1 consumer + already read changed shape). Populated under `--dry-run` too, or a caller + pre-flighting a reclaim would see the sweep appear only after committing.""" + install_bmad_config(project) + repo = project.project + orphan = _seed_state_dir(repo, "20260101-000000-aaaa") + + plan = _clean_json(repo, capsys, "--dry-run") + assert plan["schema_version"] == cli.CLEAN_SCHEMA_VERSION + assert plan["state_dirs_swept"] == 1 + assert orphan.is_dir() + + outcome = _clean_json(repo, capsys) + assert outcome["state_dirs_swept"] == 1 + assert not orphan.exists() diff --git a/tests/test_runs.py b/tests/test_runs.py index bf85bf5a..34d7609c 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -1051,12 +1051,70 @@ def test_prune_sessions_returns_unknown_from_same_sample(tmp_path, monkeypatch): assert killed == ["odd-1"] +def _seed_state_dir(project, run_id) -> Path: + """The out-of-tree control plane a driven run leaves behind: its events dir + holding one already-consumed completion signal.""" + events = runs.events_dir_for(project, run_id) + events.mkdir(parents=True) + (events / "1700000000-t1-Stop.json").write_text("{}") + return runs.state_dir_for(project, run_id) + + +def _raising(exc: Exception): + def _fail(*_args, **_kwargs): + raise exc + + return _fail + + def test_delete_run(tmp_path): run_dir = _make_state_run(tmp_path, "r1") runs.delete_run(tmp_path, run_dir) assert not run_dir.exists() +def test_delete_run_removes_the_out_of_tree_state_counterpart(tmp_path): + """#494 moved the events channel out of the project tree, so removing the run + dir stopped removing everything the run owns. Without this tail every delete + leaks a subtree under the user-scoped state root — outside the project, where + no operator thinks to look — one per run, for the life of the machine.""" + run_dir = _make_state_run(tmp_path, "r1") + state_dir = _seed_state_dir(tmp_path, "r1") + + runs.delete_run(tmp_path, run_dir) + + assert not run_dir.exists() + assert not state_dir.exists() + + +@pytest.mark.parametrize( + "attr, exc", + [ + ("state_root", runs.StateRootError("no root")), + ("project_tag", OSError("cannot canonicalize")), + ], + ids=["no-derivable-state-root", "unresolvable-project"], +) +def test_delete_run_survives_a_counterpart_it_cannot_name(tmp_path, monkeypatch, attr, exc): + """The counterpart removal is a never-raise tail (#139 teardown doctrine). + + Both rows are the counterpart being *unnameable*, which is the only failure + that can escape: an environment with no derivable state root, and a project + the OS refuses to canonicalize (#552). Removal failures are absorbed + separately, by `ignore_errors`. + + Raising would be worse than the leak it reports. The run dir is already gone + by this point, so the exception would fail a delete that in fact happened and + send the operator to retry a removal that can only fail the same way — while + `reconcile_orphan_state_dirs` already backstops the leak.""" + run_dir = _make_state_run(tmp_path, "r1") + monkeypatch.setattr(runs, attr, _raising(exc)) + + runs.delete_run(tmp_path, run_dir) + + assert not run_dir.exists() + + def test_delete_run_refuses_while_the_agent_session_is_live(tmp_path, monkeypatch): """The #419 backstop: every caller's guard is keyed on engine pid liveness, so an orphan (engine dead, session alive) reaches here. For an untagged session the run @@ -1408,3 +1466,174 @@ def test_archive_run_refuses_while_the_agent_session_is_live(tmp_path, monkeypat runs.archive_run(tmp_path, run_dir) assert run_dir.exists() assert not (tmp_path / ".bmad-loop" / "archive").exists() + + +def test_archive_run_removes_the_out_of_tree_state_counterpart(tmp_path): + """Archive inherits delete's tail — it removes the run dir just the same, so + it would leak the same subtree. + + The ordering is what the assertions pin: the tarball is complete before the + counterpart goes. Since #494 that tarball no longer carries the run's + `events/` — the channel is out of the tree and never enters the tar — which + the recorded decision accepts: those files are transient completion signals + the watcher consumed while the run was live, and everything an archive is + read for later (state, journal, tasks, logs) is in the run dir.""" + run_dir = _make_state_run(tmp_path, "20260611-100000-aaaa") + state_dir = _seed_state_dir(tmp_path, "20260611-100000-aaaa") + + dest = runs.archive_run(tmp_path, run_dir) + + with tarfile.open(dest) as tar: + assert "20260611-100000-aaaa/state.json" in tar.getnames() + assert not state_dir.exists() + + +# ------------------------------------------------- orphan state-dir sweep (#494) + + +def test_reconcile_orphan_state_dirs_removes_only_what_has_no_run_dir(tmp_path): + """The GC backstop for everything `_discard_state_dir` cannot reach: a run dir + removed by hand, an `rm -rf .bmad-loop`, a delete from before that tail + existed. Distinctness is the load-bearing half — a sweep that took the live + run's control plane too would strand a resumable run's completion channel.""" + _make_state_run(tmp_path, "live-1") + kept = _seed_state_dir(tmp_path, "live-1") + orphan = _seed_state_dir(tmp_path, "gone-1") + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [orphan] + + assert not orphan.exists() + assert kept.is_dir() + + +def test_reconcile_orphan_state_dirs_keeps_a_run_dir_with_no_state_json(tmp_path): + """Existence of the *directory* is the test, not `list_run_dirs`, which is + state.json-gated. + + A run whose state.json is missing or corrupt is exactly the run an operator is + trying to recover, and it still owns its control plane. Reading liveness from + the gated listing would sweep the counterpart out from under it — deleting + state on the strength of state being unreadable.""" + _make_run(tmp_path, "corrupt-1", with_state=False) + kept = _seed_state_dir(tmp_path, "corrupt-1") + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [] + + assert kept.is_dir() + + +def test_reconcile_orphan_state_dirs_dry_run_reports_without_removing(tmp_path): + """`clean --dry-run` promises a preview: the plan must name the work and leave + the disk alone, so the count a caller pre-flights is the count they get.""" + orphan = _seed_state_dir(tmp_path, "gone-1") + + assert runs.reconcile_orphan_state_dirs(tmp_path, dry_run=True) == [orphan] + + assert orphan.is_dir() + + +def test_reconcile_orphan_state_dirs_sweeps_a_project_whose_runs_dir_is_gone(tmp_path): + """`rm -rf .bmad-loop` is the leak this exists for, and it is the case a + missing runs dir has to answer *as an answer*: no runs exist, so every state + dir under this project's key is an orphan. Reading it as "cannot tell" would + leave the whole subtree behind permanently — nothing will ever re-create the + runs dir with those ids in it.""" + orphan = _seed_state_dir(tmp_path, "gone-1") + assert not (tmp_path / ".bmad-loop" / "runs").exists() + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [orphan] + + assert not orphan.exists() + + +def test_reconcile_orphan_state_dirs_sweeps_nothing_when_the_runs_dir_cannot_be_read( + tmp_path, monkeypatch +): + """The mirror of the test above, and the reason the two failures are told + apart. An unreadable runs dir answers nothing at all — treating it like the + missing one would sweep every control plane this project has, live runs + included, on the strength of a transient permission error. + + The fault is scoped to the runs dir on purpose. A blanket `os.scandir` raise + also takes out the state-root enumeration below it (and `rmtree`), so the + sweep returns `[]` whatever the arm under test does — measured: with the + degradation ablated to `set()` the test still passed, which is the negative + assertion holding for a reason that has nothing to do with the gate.""" + _make_state_run(tmp_path, "live-1") + kept = _seed_state_dir(tmp_path, "live-1") + runs_dir = tmp_path / ".bmad-loop" / "runs" + real_scandir = runs.os.scandir + + def _refuse_only_the_runs_dir(path, *args, **kwargs): + if Path(path) == runs_dir: + raise PermissionError("nope") + return real_scandir(path, *args, **kwargs) + + monkeypatch.setattr(runs.os, "scandir", _refuse_only_the_runs_dir) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [] + + assert kept.is_dir() + + +def test_reconcile_orphan_state_dirs_leaves_another_projects_subtree_alone(tmp_path): + """One state root holds every project's control planes, keyed by project + identity. The sweep enumerates its own key's subtree only — a sweep from the + root would let one project's `clean` delete another project's live runs, and + the two need not even be on the same disk.""" + mine = tmp_path / "mine" + theirs = tmp_path / "theirs" + (mine / ".bmad-loop" / "runs").mkdir(parents=True) + _make_state_run(theirs, "live-1") + foreign = _seed_state_dir(theirs, "live-1") + # my own orphan, so the sweep provably enumerates rather than finding nothing + orphan = _seed_state_dir(mine, "gone-1") + + assert runs.reconcile_orphan_state_dirs(mine) == [orphan] + + assert not orphan.exists() + assert foreign.is_dir() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_reconcile_orphan_state_dirs_never_removes_through_a_symlink(tmp_path): + """A symlink is not a state dir we created, so it is skipped rather than + followed — and this row points *inside* the root, where the containment test + below it cannot help. + + Reporting it would be a false count even where the removal fails harmlessly + (`rmtree` refuses a symlink): `clean` would claim a sweep that never happened + and go on claiming it every run. On Windows the containment test carries the + case this one cannot: a **junction** reads as a plain directory + (`is_symlink()` is False) but `resolve()` follows it, so without the + containment test `rmtree` would delete the target's contents outside the + root. That row is POSIX-invisible and is not graded here.""" + _make_state_run(tmp_path, "live-1") + target = _seed_state_dir(tmp_path, "live-1") + link = runs.project_state_root(tmp_path) / "ghost-1" + link.symlink_to(target) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [] + + assert link.is_symlink() # not followed, not removed + assert target.is_dir() + + +@pytest.mark.parametrize( + "attr, exc", + [ + ("state_root", runs.StateRootError("no root")), + ("project_tag", OSError("cannot canonicalize")), + ], + ids=["no-derivable-state-root", "unresolvable-project"], +) +def test_reconcile_orphan_state_dirs_degrades_when_the_root_cannot_be_named( + tmp_path, monkeypatch, attr, exc +): + """Reclamation, not repair: a sweep that cannot name its root sweeps nothing + and says so, rather than failing the whole `clean` around it. Leaving disk + behind is the cheap outcome here — the caller's real work (worktrees, trims, + archives) has already been done by the time this runs.""" + monkeypatch.setattr(runs, attr, _raising(exc)) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [] From 6570e1eb580bd8b37f2c14343b7f72b4bc584b49 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 18:39:15 -0700 Subject: [PATCH 06/11] docs(changelog): name the twin's write path, not the whole module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The events relocation entry read as if `events.py` were byte-identical to `data/bmad_loop_hook.py`. It is not — the parity test pins the five twinned names (the hardened write path and its two helpers), and the rest of each module is its own. Say which half is held identical. --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d360f25..2d398574 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,8 +26,9 @@ whose seams had diverged enough that several ports needed a different fix, and t whose relay predates the move — re-run `bmad-loop init` to refresh it. - **`bmad-loop relay `** writes a session event without the copied-in script, on the same contract (nothing on stdout, rc 0 always, silent no-op outside a driven session). It is backed - by a new `events.py`, held byte-identical to the stdlib-only relay by an AST parity test, and - dispatches ahead of the shared error handler so a broken `policy.toml` cannot fail a hook. + by a new `events.py`, whose write path an AST parity test holds byte-identical to the + stdlib-only relay's, and dispatches ahead of the shared error handler so a broken + `policy.toml` cannot fail a hook. - **`validate` gains `hooks.relay-stale`** — the installed relay compared against the packaged one, a warning that never moves the exit code (the fallback keeps a stale relay working). `diagnose`'s `events` group now counts both locations; payload and schema unchanged. From debce19b9fc06b866e9c2b992ba85d0a40b4bb27 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 19:00:22 -0700 Subject: [PATCH 07/11] fix(runs): hold RuntimeError in the state-dir never-raise guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `project_tag` resolves before digesting, and below 3.13 `Path.resolve` reports a symlink loop as `RuntimeError` rather than `OSError`. Measured across the support matrix rather than assumed: 3.11 and 3.12 raise, 3.13 and 3.14 return the unresolved path — so on two of the four supported interpreters, both of which CI runs, a loop escaped guards whose entire contract is to degrade. Three sites, each with its own consequence: - `_discard_state_dir` promises a never-raise tail (#139). The run dir is already gone by then, so an escaping loop failed the operator's delete over a removal that in fact happened. - `reconcile_orphan_state_dirs`' root guard took the whole `clean` down after its real work (worktrees, trims, archives) was already finished. - Its containment guard is the per-entry one: an unresolvable entry mid-sweep aborted the sweep half-done and reported none of what it had just removed. Skipping is the safe arm — an entry that cannot be resolved cannot be proven inside the root, and that proof is the only thing standing between `rmtree` and a Windows junction's target. Each of the three was ablated singly, requiring rc 1: the two injected rows redden their own parametrization, and the new entry-resolution test reddens only when `RuntimeError` leaves the containment guard. Also from the same review round: - `docs/setup-guide.md`: the state-root removal command hardcoded the XDG default while the paragraph above it names `BMAD_LOOP_STATE_DIR` — an operator with the override set deleted a directory that was not their state root and kept the one that was. Now the same cascade the orchestrator resolves. - `tests/test_events.py`: `env=` replaces the environment, so the hook subprocess started without SYSTEMROOT, where a Windows Python child can fail to load its side-by-side assemblies — a start failure that would read as the relay misbehaving. Passed through on nt only; the env stays minimal otherwise, because the relay is stdlib-only and must need nothing else. - `tests/test_events.py`: a local `events` Path shadowed the imported `events` module for the rest of its test. - `install.py`: `hook_script_current`'s docstring claimed the packaged-source spelling "has exactly one home" while the literal also appears in `install_into`. Say what is true — reader and writer stay in one module. --- docs/setup-guide.md | 3 ++- src/bmad_loop/install.py | 5 ++-- src/bmad_loop/runs.py | 31 ++++++++++++++++------- tests/test_events.py | 13 +++++++--- tests/test_runs.py | 53 +++++++++++++++++++++++++++++++++++++--- 5 files changed, 86 insertions(+), 19 deletions(-) diff --git a/docs/setup-guide.md b/docs/setup-guide.md index b86f8324..99811ca3 100644 --- a/docs/setup-guide.md +++ b/docs/setup-guide.md @@ -356,7 +356,8 @@ hand. That root is shared by **every** project on this machine, so delete the wh you are removing bmad-loop everywhere: ```bash -rm -rf "${XDG_STATE_HOME:-$HOME/.local/state}/bmad-loop" # all projects on this machine +# all projects on this machine — same cascade the orchestrator resolves +rm -rf "${BMAD_LOOP_STATE_DIR:-${XDG_STATE_HOME:-$HOME/.local/state}/bmad-loop}" ``` ### 3. Remove the bundled skills diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 8fc44afc..311bb163 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -1020,8 +1020,9 @@ def hook_script_current(project: Path) -> bool | None: "I could not look" is not "your relay is out of date". Lives here, beside :func:`install_into`'s write of the same two paths, so the - packaged-source spelling has exactly one home — a comparison that resolved - the source differently from the writer would answer a different question. + reader and the writer of the relay stay in one module and one reviewer's view + — a comparison that resolved the source differently from the writer would + answer a different question. Compared as TEXT read with universal newlines, not as raw bytes. That is precisely the round trip ``install_into`` performs (``read_text`` then diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 803c577a..d3ebc340 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -797,19 +797,24 @@ def _discard_state_dir(project: Path, run_id: str) -> None: A **never-raise tail**, per the teardown doctrine (#139): the run dir is already gone by the time this runs, and failing the operator's delete over an - unreachable state root would report a removal that in fact happened. The two - catchable outcomes are both "the counterpart could not even be named" — - :class:`StateRootError` for an environment with no derivable root, ``OSError`` - for a project path the OS cannot canonicalize (:func:`project_tag` resolves - before digesting). Removal failures are absorbed by ``ignore_errors``. Either - way the orphan sweep in :func:`reconcile_orphan_state_dirs` is the backstop. + unreachable state root would report a removal that in fact happened. Every + catchable outcome is "the counterpart could not even be named" — + :class:`StateRootError` for an environment with no derivable root, and + ``OSError``/``RuntimeError`` for a project path the OS cannot canonicalize + (:func:`project_tag` resolves before digesting). ``RuntimeError`` is not + optional there: below 3.13 ``Path.resolve`` reports a symlink loop that way + rather than as ``OSError`` (measured — 3.11 and 3.12 raise, 3.13 and 3.14 + return the unresolved path), so on two supported interpreters an ``OSError`` + -only guard lets a loop escape and breaks the promise in this paragraph. + Removal failures are absorbed by ``ignore_errors``. Either way the orphan + sweep in :func:`reconcile_orphan_state_dirs` is the backstop. Deliberately not called by :func:`trim_run_dir`: a trimmed run is still live on disk and resumable, and its control plane must outlive the scaffolding. """ try: target = state_dir_for(project, run_id) - except (StateRootError, OSError): + except (StateRootError, OSError, RuntimeError): return shutil.rmtree(target, ignore_errors=True) @@ -996,6 +1001,14 @@ def reconcile_orphan_state_dirs(project: Path, *, dry_run: bool = False) -> list state root, an unreadable root, or an unreadable runs dir all sweep nothing. This is reclamation, not repair — leaving disk behind is the cheap outcome, and removing a live run's control plane is not. + + Both guards hold ``RuntimeError`` alongside ``OSError`` for the same reason + :func:`_discard_state_dir` does: every path here is resolved (the project by + :func:`project_tag`, then the root, then each entry), and below 3.13 + ``Path.resolve`` reports a symlink loop as ``RuntimeError``. A loop planted + among the entries would otherwise escape a sweep whose whole contract is to + degrade, and take the operator's ``clean`` down with it after its real work + was already done. """ live = _run_dir_names(project) if live is None: @@ -1004,7 +1017,7 @@ def reconcile_orphan_state_dirs(project: Path, *, dry_run: bool = False) -> list root = project_state_root(project) entries = sorted(root.iterdir()) root_res = root.resolve() - except (StateRootError, OSError): + except (StateRootError, OSError, RuntimeError): return [] handled: list[Path] = [] for entry in entries: @@ -1012,7 +1025,7 @@ def reconcile_orphan_state_dirs(project: Path, *, dry_run: bool = False) -> list continue try: entry.resolve().relative_to(root_res) - except (OSError, ValueError): + except (OSError, RuntimeError, ValueError): continue handled.append(entry) if not dry_run: diff --git a/tests/test_events.py b/tests/test_events.py index 71f68faa..9656ac2c 100644 --- a/tests/test_events.py +++ b/tests/test_events.py @@ -132,8 +132,14 @@ def test_relay_and_the_hook_shape_the_same_event(tmp_path, monkeypatch): proc = subprocess.run( [sys.executable, str(HOOK), "Stop"], input=json.dumps(payload), + # `env=` REPLACES the environment rather than extending it, and on Windows + # a Python child started without SYSTEMROOT can fail to load its + # side-by-side assemblies — a start failure that would read here as the + # relay misbehaving. Kept minimal otherwise: the relay is stdlib-only and + # must need nothing else. env={ "PATH": os.environ.get("PATH", ""), + **({"SYSTEMROOT": os.environ.get("SYSTEMROOT", "")} if os.name == "nt" else {}), "BMAD_LOOP_RUN_DIR": str(hook_run), "BMAD_LOOP_TASK_ID": "1-1-a-dev-1", }, @@ -614,12 +620,13 @@ def test_relay_prefers_the_events_dir_env(tmp_path, monkeypatch, capsys): config happens to name.""" run_dir = tmp_path / "run" run_dir.mkdir() - events = tmp_path / "state" / "runs" / "RID" / "events" + # Not `events` — that name is the imported module for the rest of this file. + events_dir = tmp_path / "state" / "runs" / "RID" / "events" - assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir, events_dir=events) == 0 + assert _relay("Stop", {"session_id": "s1"}, monkeypatch, run_dir, events_dir=events_dir) == 0 assert capsys.readouterr() == ("", "") - files = list(events.glob("*.json")) + files = list(events_dir.glob("*.json")) assert len(files) == 1 and json.loads(files[0].read_text())["session_id"] == "s1" assert not (run_dir / "events").exists() diff --git a/tests/test_runs.py b/tests/test_runs.py index 34d7609c..d8d5f9ab 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -1092,17 +1092,26 @@ def test_delete_run_removes_the_out_of_tree_state_counterpart(tmp_path): [ ("state_root", runs.StateRootError("no root")), ("project_tag", OSError("cannot canonicalize")), + ("project_tag", RuntimeError("Symlink loop from '/p'")), ], - ids=["no-derivable-state-root", "unresolvable-project"], + ids=["no-derivable-state-root", "unresolvable-project", "symlink-loop-project"], ) def test_delete_run_survives_a_counterpart_it_cannot_name(tmp_path, monkeypatch, attr, exc): """The counterpart removal is a never-raise tail (#139 teardown doctrine). - Both rows are the counterpart being *unnameable*, which is the only failure + Every row is the counterpart being *unnameable*, which is the only failure that can escape: an environment with no derivable state root, and a project the OS refuses to canonicalize (#552). Removal failures are absorbed separately, by `ignore_errors`. + The `RuntimeError` row is not a hypothetical type: `project_tag` resolves + before digesting, and below 3.13 `Path.resolve` reports a symlink loop as + `RuntimeError` rather than `OSError` — measured across the support matrix, + 3.11 and 3.12 raise it where 3.13 and 3.14 return the unresolved path. So on + two supported interpreters this is the live arm, and it is injected here + rather than built from real symlinks because the loop would have to sit on + the *project* path, which the sandbox fixtures own. + Raising would be worse than the leak it reports. The run dir is already gone by this point, so the exception would fail a delete that in fact happened and send the operator to retry a removal that can only fail the same way — while @@ -1624,8 +1633,9 @@ def test_reconcile_orphan_state_dirs_never_removes_through_a_symlink(tmp_path): [ ("state_root", runs.StateRootError("no root")), ("project_tag", OSError("cannot canonicalize")), + ("project_tag", RuntimeError("Symlink loop from '/p'")), ], - ids=["no-derivable-state-root", "unresolvable-project"], + ids=["no-derivable-state-root", "unresolvable-project", "symlink-loop-project"], ) def test_reconcile_orphan_state_dirs_degrades_when_the_root_cannot_be_named( tmp_path, monkeypatch, attr, exc @@ -1633,7 +1643,42 @@ def test_reconcile_orphan_state_dirs_degrades_when_the_root_cannot_be_named( """Reclamation, not repair: a sweep that cannot name its root sweeps nothing and says so, rather than failing the whole `clean` around it. Leaving disk behind is the cheap outcome here — the caller's real work (worktrees, trims, - archives) has already been done by the time this runs.""" + archives) has already been done by the time this runs. + + `RuntimeError` is the below-3.13 spelling of a symlink loop out of + `Path.resolve`, which `project_tag` calls; see the sibling delete test for + the measured version split.""" monkeypatch.setattr(runs, attr, _raising(exc)) assert runs.reconcile_orphan_state_dirs(tmp_path) == [] + + +def test_reconcile_orphan_state_dirs_skips_an_entry_it_cannot_resolve(tmp_path, monkeypatch): + """The containment test resolves each candidate, so it inherits the same + below-3.13 `RuntimeError` a symlink loop raises — and here it lands *per + entry*, mid-sweep, after earlier entries have already been removed. An + unguarded loop would abort `clean` half-done and report none of what it had + just deleted. + + Skipping is the safe arm rather than sweeping: an entry that cannot be + resolved cannot be proven inside the root, and that proof is the only thing + standing between `rmtree` and a Windows junction's target. + + Ablation guard: drop `RuntimeError` from the containment guard and this + raises instead of returning the resolvable orphan.""" + _make_state_run(tmp_path, "live-1") + _seed_state_dir(tmp_path, "live-1") + good = _seed_state_dir(tmp_path, "ghost-good") + bad = _seed_state_dir(tmp_path, "ghost-loop") + + real_resolve = Path.resolve + + def _resolve(self: Path, *args, **kwargs): + if self == bad: + raise RuntimeError(f"Symlink loop from {str(self)!r}") + return real_resolve(self, *args, **kwargs) + + monkeypatch.setattr(Path, "resolve", _resolve) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [good] + assert not good.exists() and bad.is_dir() From 5175f5391cf6d46d93c07e7f24eb6ac475e199f9 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 19:23:33 -0700 Subject: [PATCH 08/11] fix(runs): order the orphan sweep's reads, and refuse a relative state root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the new control plane could take a live run down, both found by review on `debce19`. **The orphan sweep raced a starting run (P1).** `reconcile_orphan_state_dirs` sampled live run-dir names *before* enumerating state-root entries, and `clean` holds no lock against a run starting. A run created in that gap was missing from `live` and present in `entries`, so the sweep deleted the control plane of a run that was starting right then — after which its watcher polls a primary that no longer exists, or never sees the Stop and waits out `session_timeout_min`. The fix is the read order, not a second check. A run creates its run dir strictly before its state dir (`compose_run` builds the `Journal`, which mkdirs the run dir, and only then calls `make_adapters`, whose `SignalWatcher` mkdirs the events dir), so enumerating entries FIRST makes that ordering carry the guarantee: anything in `entries` had its run dir on disk even earlier, and the later `live` read cannot miss it. A run dir that disappears between the reads is the opposite case and still swept — by then it is a real orphan. **A relative `BMAD_LOOP_STATE_DIR` named two directories (P2).** The override was the one candidate `_state_base` did not filter, so a relative spelling was returned raw. But the root is read by two processes with different working directories: the engine exports it to the session as `BMAD_LOOP_EVENTS_DIR` and the multiplexer launches that session at `spec.cwd` — a worktree, under isolation — while the watcher polls from the orchestrator's cwd. The relay then writes its Stop where nothing is watching and the run stalls silently, which is the exact outcome `_state_base`'s docstring already cites for rejecting relative *derived* bases, and the one this channel was moved out of the tree to prevent. It raises rather than absolutizing. Raising is not the silent countermand that paragraph refuses — it names the variable and the fix — whereas resolving against whichever cwd this process happens to have is the guess, and would pick one of the two directories at random. `_state_base`'s not-the-root half stays unapplied: that half exists to stop a broken environment's `""` from landing a guess at `/`, and an override is not a guess. Both were ablated singly, requiring rc 1: reverting the read order sweeps the mid-startup run, and dropping the `os.path.isabs` arm fails all three relative rows. The race is simulated by starting a run *inside* the `live` read, so whichever read runs second sees it — what a real interleaving does. Docs follow the behavior: `envvars.state_dir`'s "relative spelling included" was made false by this change (the reader stays verbatim; `state_root` judges), and the README env-var table and CHANGELOG now state the absolute requirement. --- CHANGELOG.md | 4 ++- README.md | 14 ++++---- src/bmad_loop/envvars.py | 16 ++++++--- src/bmad_loop/runs.py | 55 +++++++++++++++++++++++++---- tests/test_runs.py | 74 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 144 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d398574..f3cbab06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,9 @@ whose seams had diverged enough that several ports needed a different fix, and t - **The hook-event channel moves out of the project tree (#494).** A run's session-completion signals now land under a user-scoped state root — `$XDG_STATE_HOME/bmad-loop` (else `~/.local/state/bmad-loop`), `%LOCALAPPDATA%\bmad-loop\state` on Windows, or wherever - `BMAD_LOOP_STATE_DIR` points — keyed `///events/`, so a branch switch, a + `BMAD_LOOP_STATE_DIR` points (absolute paths only — the orchestrator and the session it + launches read the root from different working directories) — keyed + `///events/`, so a branch switch, a worktree mount or a rollback can no longer take a live run's control plane away. - **Older relays keep working.** Sessions are told the directory via `BMAD_LOOP_EVENTS_DIR`; both diff --git a/README.md b/README.md index 8e8f0740..73e1ed59 100644 --- a/README.md +++ b/README.md @@ -565,13 +565,13 @@ For `per_worktree`, set `editor_mode = "per_worktree"` with `[scm] isolation = " A handful of `BMAD_LOOP_*` variables override behavior at runtime, taking precedence over the policy file. Most operators only ever touch `BMAD_LOOP_MUX_BACKEND` and, on a host with an unusual home directory, `BMAD_LOOP_STATE_DIR`; the rest are override/test hooks. -| Variable | Value | Effect | -| ----------------------------- | ---------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `BMAD_LOOP_MUX_BACKEND` | registered backend name (e.g. `tmux`, `psmux`) | Forces the terminal-multiplexer backend, outranking the `[mux] backend` policy key and auto-selection. A name matching no registered backend is an error — it never silently falls back. Unset ⇒ auto-select. | -| `BMAD_LOOP_PROCESS_HOST` | registered host name (e.g. `posix`, `windows`) | Forces the process-lifecycle host (an override/test hook). A name matching no registered host raises rather than silently using POSIX. Unset ⇒ this platform's default. | -| `BMAD_LOOP_STATE_DIR` | directory path | Overrides the user-scoped **state root** — the out-of-tree home of per-run control-plane state, keyed `///`. Used as the root itself, so nothing is appended to it. Unset ⇒ `$XDG_STATE_HOME/bmad-loop` when that names an absolute path, else `~/.local/state/bmad-loop`; on Windows `%LOCALAPPDATA%\bmad-loop\state`, else `%USERPROFILE%\AppData\Local\bmad-loop\state`. Set this when none of those is derivable or writable (a home on a network share, a locked-down service account). | -| `BMAD_LOOP_EVENTS_DIR` | directory path | **Session protocol, not an operator knob.** The orchestrator exports it into every session it drives, naming that run's hook-event directory under the state root; the hook relay writes there, falling back to the legacy in-tree `/events` when it is absent. Setting it yourself in a shell has no effect on a run (the engine overwrites it per session) and only misdirects a hand-invoked relay. | -| `BMAD_LOOP_SESSION_TIMEOUT_S` | seconds (float) | Overrides the per-session wall-clock budget (normally `limits.session_timeout_min × 60`) — mainly a test/E2E hook for sub-minute timeouts. A value that is not a finite positive number is ignored — non-positive, unparseable, or non-finite (`inf`, `1e999`), the last of which would otherwise disable the timeout outright. A large finite value is honoured. Unset ⇒ the policy value. | +| Variable | Value | Effect | +| ----------------------------- | ---------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `BMAD_LOOP_MUX_BACKEND` | registered backend name (e.g. `tmux`, `psmux`) | Forces the terminal-multiplexer backend, outranking the `[mux] backend` policy key and auto-selection. A name matching no registered backend is an error — it never silently falls back. Unset ⇒ auto-select. | +| `BMAD_LOOP_PROCESS_HOST` | registered host name (e.g. `posix`, `windows`) | Forces the process-lifecycle host (an override/test hook). A name matching no registered host raises rather than silently using POSIX. Unset ⇒ this platform's default. | +| `BMAD_LOOP_STATE_DIR` | absolute directory path | Overrides the user-scoped **state root** — the out-of-tree home of per-run control-plane state, keyed `///`. Used as the root itself, so nothing is appended to it. Must be **absolute**: the root is read both by the orchestrator and by the session it launches, which run from different working directories, so a relative value names two different places — bmad-loop refuses it rather than picking one. Unset ⇒ `$XDG_STATE_HOME/bmad-loop` when that names an absolute path, else `~/.local/state/bmad-loop`; on Windows `%LOCALAPPDATA%\bmad-loop\state`, else `%USERPROFILE%\AppData\Local\bmad-loop\state`. Set this when none of those is derivable or writable (a home on a network share, a locked-down service account). | +| `BMAD_LOOP_EVENTS_DIR` | directory path | **Session protocol, not an operator knob.** The orchestrator exports it into every session it drives, naming that run's hook-event directory under the state root; the hook relay writes there, falling back to the legacy in-tree `/events` when it is absent. Setting it yourself in a shell has no effect on a run (the engine overwrites it per session) and only misdirects a hand-invoked relay. | +| `BMAD_LOOP_SESSION_TIMEOUT_S` | seconds (float) | Overrides the per-session wall-clock budget (normally `limits.session_timeout_min × 60`) — mainly a test/E2E hook for sub-minute timeouts. A value that is not a finite positive number is ignored — non-positive, unparseable, or non-finite (`inf`, `1e999`), the last of which would otherwise disable the timeout outright. A large finite value is honoured. Unset ⇒ the policy value. | Game-engine (Unity) runs read a wider `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` set documented in the [Game Engine MCP guide](docs/game-engine-mcp-guide.md). diff --git a/src/bmad_loop/envvars.py b/src/bmad_loop/envvars.py index e0902628..c3a20e12 100644 --- a/src/bmad_loop/envvars.py +++ b/src/bmad_loop/envvars.py @@ -84,11 +84,17 @@ def state_dir() -> str | None: Verbatim like the two name readers above — :func:`runs.state_root` uses the value as the state root itself, so an operator who names a directory gets that - directory, relative spelling included. Silently ignoring a stated override in - favour of the platform cascade would be the same failure - :func:`mux_backend` refuses: a loud misconfiguration turned into a quiet - auto-select, discoverable only by noticing where a run's events did *not* - appear. + directory. Silently ignoring a stated override in favour of the platform + cascade would be the same failure :func:`mux_backend` refuses: a loud + misconfiguration turned into a quiet auto-select, discoverable only by + noticing where a run's events did *not* appear. + + Reading verbatim is not the same as accepting anything: this reader reports + what is set, and :func:`runs.state_root` judges it. A **relative** value is + refused there rather than resolved, because the root is read by two processes + with different working directories — see that function for the full reason. + The split is deliberate; a reader that silently rewrote its variable would + make the refusal impossible to state. The one value not passed through is the empty string, which reads as unset. ``export BMAD_LOOP_STATE_DIR=`` is what an unset-looking export leaves behind, diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index d3ebc340..2fda7d43 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -206,9 +206,28 @@ def state_root() -> Path: 1. ``BMAD_LOOP_STATE_DIR``, used as the state root **itself** — no ``bmad-loop`` segment is appended, because the variable names our root rather than a base to build one under. It is honoured as spelled (see - :func:`envvars.state_dir`), so it is the one candidate ``_state_base`` does - not filter: skipping a stated override would be a silent countermand, where - skipping a *derived* base only moves on to the next guess. + :func:`envvars.state_dir`) and is not passed through ``_state_base``: + *skipping* a stated override would be a silent countermand, where skipping + a derived base only moves on to the next guess. + + It must still be **absolute**, and a relative spelling raises rather than + being resolved for the operator. Absoluteness is not a matter of taste + here — the root is read by two processes with different working + directories. The engine exports it to the session as + ``BMAD_LOOP_EVENTS_DIR`` and the multiplexer launches that session at + ``spec.cwd`` (a worktree under isolation), while the watcher polls it from + the orchestrator's own cwd. A relative root therefore names two different + directories at once: the relay writes its Stop where nothing is watching, + and the run waits out ``session_timeout_min`` — the exact silent stall + ``_state_base`` rejects relative *derived* bases to avoid, and the one + this whole channel was moved out of the tree to prevent. + + Raising is not the countermand the paragraph above refuses: it names the + variable and the fix, where absolutizing against whichever cwd this + process happens to have would be the guess. The not-the-root half of + ``_state_base``'s rule is deliberately *not* applied — that half exists to + stop a broken environment's ``""`` from landing a guess at ``/``, and an + override is not a guess. 2. POSIX — ``$XDG_STATE_HOME/bmad-loop`` when that variable names an absolute path, else ``~/.local/state/bmad-loop``. A relative ``XDG_STATE_HOME`` is *ignored*, which the XDG base-directory spec requires of its consumers. @@ -240,6 +259,16 @@ def state_root() -> Path: """ override = envvars.state_dir() if override: + # `os.path.isabs` on the raw string, matching `_state_base` exactly rather + # than `Path.is_absolute` — the rule and its reason are stated there. + if not os.path.isabs(override): + raise StateRootError( + f"{envvars.STATE_DIR} must name an absolute directory: {override!r} is " + "relative, and the state root is read by both this process and the " + "session it launches — which run from different working directories, " + "so a relative root names two different places and the run's " + "completion signal is written where nothing is watching" + ) return Path(override) if sys.platform == "win32": local = _state_base(os.environ.get("LOCALAPPDATA")) @@ -1009,16 +1038,30 @@ def reconcile_orphan_state_dirs(project: Path, *, dry_run: bool = False) -> list among the entries would otherwise escape a sweep whose whole contract is to degrade, and take the operator's ``clean`` down with it after its real work was already done. + + **The two reads are ordered, and the order is the whole race guard.** State + entries are enumerated *before* the live run-dir names, because a run creates + its run dir strictly before its state dir — ``compose_run`` builds the + ``Journal`` (which mkdirs the run dir) and only then calls ``make_adapters``, + whose ``SignalWatcher`` mkdirs the events dir. Reading entries first makes + that ordering carry the guarantee: anything in ``entries`` had its state dir + on disk at the first read, so its run dir was on disk *before* that, so the + later ``live`` read is certain to contain it. Read the other way round, a run + starting in the gap is missing from ``live`` and present in ``entries``, and + an operator's ``clean`` deletes the control plane of a run that is starting + right now — whose watcher then polls a primary that no longer exists, or + simply never sees the Stop. A run dir that disappears *between* the reads is + the opposite case and correctly swept: it is a real orphan by then. """ - live = _run_dir_names(project) - if live is None: - return [] try: root = project_state_root(project) entries = sorted(root.iterdir()) root_res = root.resolve() except (StateRootError, OSError, RuntimeError): return [] + live = _run_dir_names(project) + if live is None: + return [] handled: list[Path] = [] for entry in entries: if entry.name in live or entry.is_symlink() or not entry.is_dir(): diff --git a/tests/test_runs.py b/tests/test_runs.py index d8d5f9ab..b5224ba7 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -777,6 +777,38 @@ def test_state_root_precedence_override_then_xdg_then_home(tmp_path, monkeypatch assert runs.state_root() == home / ".local" / "state" / "bmad-loop" +@pytest.mark.parametrize("value", ["state", "./state", "~/state"], ids=repr) +def test_state_root_refuses_a_relative_override(tmp_path, monkeypatch, value): + """`BMAD_LOOP_STATE_DIR` is honoured as spelled, but a relative spelling is + not a root — it is two roots. The engine exports this path to the session as + `BMAD_LOOP_EVENTS_DIR` and the multiplexer launches that session at + `spec.cwd` (a worktree, under isolation), while the watcher polls from the + orchestrator's cwd. So the relay writes its Stop into one directory and + nothing watches it, and the run waits out `session_timeout_min` — silent, and + exactly the stall moving this channel out of the tree was meant to prevent. + + It RAISES rather than falling through to the cascade, which is the split from + the sibling XDG test above: a derived base that fails its check is a guess we + move on from, an override is a statement we cannot honour and must not + silently replace. It equally does not absolutize — resolving against whichever + cwd this process happens to have is the guess, and picking one of the two + directories at random is how the stall gets harder to see rather than gone. + + `~/state` is here for the same reason it is in the XDG rows: nothing expands + it, so it stays relative. The empty string is deliberately NOT a row — empty + reads as *unset* and falls through to the cascade, which + `test_state_root_precedence_override_then_xdg_then_home` already grades. + + Ablation target: drop the `os.path.isabs` guard from the override arm and all + three rows fail — each returning a cwd-relative root instead of raising.""" + monkeypatch.setattr(runs.sys, "platform", "linux") + _fake_home(monkeypatch, tmp_path / "home") + monkeypatch.setenv(envvars.STATE_DIR, value) + + with pytest.raises(runs.StateRootError, match=envvars.STATE_DIR): + runs.state_root() + + @pytest.mark.parametrize("value", ["state", "./state", "~/state", ""], ids=repr) def test_state_root_ignores_an_xdg_state_home_that_is_not_absolute(tmp_path, monkeypatch, value): """The XDG base-directory spec says a relative value "must be ignored", and @@ -1653,6 +1685,48 @@ def test_reconcile_orphan_state_dirs_degrades_when_the_root_cannot_be_named( assert runs.reconcile_orphan_state_dirs(tmp_path) == [] +def test_reconcile_orphan_state_dirs_keeps_a_run_that_starts_mid_sweep(tmp_path, monkeypatch): + """`clean` is an operator command with no lock against a run starting, and the + two reads it makes are of different trees. A run creates its run dir strictly + before its state dir (`compose_run` builds the `Journal`, which mkdirs the run + dir, and only then calls `make_adapters`, whose `SignalWatcher` mkdirs the + events dir) — so reading state entries FIRST is what makes the ordering carry + the guarantee: an entry seen there had its run dir on disk even earlier, and + the later `live` read cannot miss it. + + Read the other way round, a run that starts in the gap is absent from `live` + and present in `entries`, and `clean` deletes the control plane of a run that + is starting right now. The cost is not a lost directory — it is the run, which + then polls a primary that no longer exists or never sees its Stop and waits + out `session_timeout_min`. + + The gap is simulated where it actually lives, by starting a run *inside* the + `live` read rather than by patching the sweep: whichever read runs second is + the one that sees `racer`, which is exactly what a real interleaving does. + + Ablation guard: move the `live = _run_dir_names(project)` read back above the + `entries` enumeration and this fails, sweeping `racer` mid-startup.""" + _make_state_run(tmp_path, "live-1") + _seed_state_dir(tmp_path, "live-1") + orphan = _seed_state_dir(tmp_path, "ghost-1") + + real_names = runs._run_dir_names + racer: list[Path] = [] + + def _names_then_a_new_run(project: Path): + names = real_names(project) # the snapshot, taken before `racer` exists + _make_state_run(project, "racer") + racer.append(_seed_state_dir(project, "racer")) + return names + + monkeypatch.setattr(runs, "_run_dir_names", _names_then_a_new_run) + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [orphan] + + assert not orphan.exists() + assert racer[0].is_dir(), "swept the control plane of a run that was starting" + + def test_reconcile_orphan_state_dirs_skips_an_entry_it_cannot_resolve(tmp_path, monkeypatch): """The containment test resolves each candidate, so it inherits the same below-3.13 `RuntimeError` a symlink loop raises — and here it lands *per From f1f75547adeb9ae39ae206a9e4289e8091009384 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 19:59:58 -0700 Subject: [PATCH 09/11] docs(adapters): state that the bootstrap's keyword set grows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `runsetup.make_adapters` builds every adapter class with `cls(**build_kwargs)`, and this PR is the first change to add a keyword to that dict — `events_dir`. An out-of-tree class with a closed signature raises `TypeError` on the first keyword it has not heard of, which the guide left the author to discover. Say it instead: accept `**kwargs` in both variants, because core adds to the run description as the run gains things worth describing. The bundled families carry closed signatures and are deliberately named as NOT the pattern to copy — they are edited in the same commit that adds the keyword, and an out-of-tree class cannot be. What they do model is the other half, accepting what you have no use for: `opencode_http` takes `events_dir` and `del`s it, because that family observes over SSE and fires no hooks. No version number is claimed for when `events_dir` arrived; the next release number is not this branch's to assert. The core-side half — that a signature mismatch escapes as a bare traceback after `compose_run` has already written the run state, where the sibling `ImportError` arm produces a clean `error:` line — is #569, against the seam that owns it. --- docs/adapter-authoring-guide.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 8e9b6501..8efade05 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -699,6 +699,20 @@ Two seam facts worth internalizing: on disk; every other role builds `plain`. Both variants of a family share the `(*args, paths, **kwargs)` dev `__init__` contract, so honoring it is all an out-of-tree class must do to slot into that machinery. +- **The bootstrap's keyword set grows, so accept `**kwargs`in both variants.**`runsetup.make*adapters`builds every class with`cls(\*\*build_kwargs)`, and that + dict is a \_description of the run* — `run_dir`, `policy`, `profile`, the usage and + nudge settings, `events_dir` (the run's hook-event channel), plus `mux` for a + `needs_mux=True` kind and `paths` for the `dev` variant. Core adds to it as the run + gains things worth describing; `events_dir` arrived that way (#494). A class with a + closed signature raises `TypeError` on the first keyword it has not heard of, + before the session starts. + + The bundled families carry closed signatures instead, which is not a second + pattern to copy: they are edited in the same commit that adds the keyword, and an + out-of-tree class cannot be. What they do model is the other half of the + obligation — accept what you have no use for rather than refusing it. + `opencode_http` takes `events_dir` and immediately `del`s it, because that family + observes over SSE and fires no hooks, so there is no channel for it to point at. An entry-point profile is held to the same invariants a TOML profile is (hook dialect, path containment, `env_fault_patterns` compilation, …): it is validated on From 051752c54d96e3c7fe63bae05689cba59ea76b1b Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 20:09:26 -0700 Subject: [PATCH 10/11] docs(cli): stop `relay`'s help claiming the installed hooks invoke it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The subcommand's argparse help read "(invoked by the installed hooks, not by hand)". They do not: `install._hook_command` still emits ` /.bmad-loop/bmad_loop_hook.py `, so no registration reaches `cmd_relay` today. The rest of the tree was already honest about this — the COUPLING note at cli.py:499 says Phase 2 *moves* the relay, and events.py calls it "the #461 Phase 2 hook target" — so the help string was the one place stating it as done. Corrected rather than made true. Retargeting the registrations is #461 Phase 2, and that same note reserves an obligation for whoever does it: `hooks.relay- present` must be RETARGETED to stat what the registration actually points at, not dropped, because the stall it guards survives the move. Switching `_hook_command` here would drag that check's rework into an events-relocation PR. `cmd_relay`'s docstring now says outright that nothing points at it yet, because a console script that exists and is documented reads as the live path — and an operator debugging a lost Stop needs to know which relay actually ran. The CHANGELOG entry says the same, so the feature is not read as changing a run it does not touch. --- CHANGELOG.md | 4 +++- src/bmad_loop/cli.py | 11 ++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3cbab06..cc632f3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,9 @@ whose seams had diverged enough that several ports needed a different fix, and t `init` copies the relay into the project, so an upgraded orchestrator regularly drives sessions whose relay predates the move — re-run `bmad-loop init` to refresh it. - **`bmad-loop relay `** writes a session event without the copied-in script, on the same - contract (nothing on stdout, rc 0 always, silent no-op outside a driven session). It is backed + contract (nothing on stdout, rc 0 always, silent no-op outside a driven session). `init` does + not point hooks at it yet — that retargeting is #461 Phase 2 — so it changes no run today. It + is backed by a new `events.py`, whose write path an AST parity test holds byte-identical to the stdlib-only relay's, and dispatches ahead of the shared error handler so a broken `policy.toml` cannot fail a hook. diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 8120adab..6cdb87e4 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -3713,6 +3713,15 @@ def cmd_init(args: argparse.Namespace) -> int: def cmd_relay(args: argparse.Namespace) -> int: """``bmad-loop relay `` — the hook relay as an installed console script. + **Nothing points at it yet.** ``init`` still registers the copied workspace + relay (``install._hook_command`` emits `` /.bmad-loop/ + bmad_loop_hook.py ``), so no installed hook reaches this handler today; + it is the target #461 Phase 2 retargets those registrations to, and that move + carries its own obligation — see the COUPLING note on ``hooks.relay-present``, + which must be retargeted rather than dropped in the same change. Said here + because a console script that exists and is documented reads as the live path, + and an operator debugging a lost Stop needs to know which relay actually ran. + Total by contract, unlike every other handler: a coding CLI runs this INSIDE the session whose completion it reports, and several of them surface a non-zero hook exit as a failed tool call in that session. So nothing here @@ -4081,7 +4090,7 @@ def add(name: str, func, help: str, *, aliases=()) -> argparse.ArgumentParser: relay_p = sub.add_parser( "relay", help="write one session event file from a coding-CLI hook payload on stdin " - "(invoked by the installed hooks, not by hand)", + "(a hook target for machines, not a command to run by hand)", ) relay_p.add_argument( "event", From 95e622d5f1f85bae6037975ea195cb4b2bc511d6 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 20:18:18 -0700 Subject: [PATCH 11/11] docs(adapters): unbreak the bootstrap-contract bullet's markdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bullet added in f1f7554 nested a code span containing asterisks inside a bold span — ``**… `**kwargs` in both variants.**`` — which prettier cannot disambiguate. `trunk fmt` rewrote it in place and the committed text was wrong in four ways at once: the identifier became `runsetup.make*adapters`, the call became `cls(\*\*build_kwargs)` with the backslashes rendering literally, the bold and emphasis runs broke, and the spaces around three code spans were eaten. Restructured so no asterisk-bearing code span sits inside a bold span: the lead-in is bold and plain, and `**kwargs` moves into the sentence after it — which is how the neighbouring `(*args, paths, **kwargs)` line has always been written, and why that one survived. Re-run through `trunk fmt` and re-read to confirm the text is stable rather than assuming the fix took. --- docs/adapter-authoring-guide.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 8efade05..5ac4fbca 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -699,8 +699,9 @@ Two seam facts worth internalizing: on disk; every other role builds `plain`. Both variants of a family share the `(*args, paths, **kwargs)` dev `__init__` contract, so honoring it is all an out-of-tree class must do to slot into that machinery. -- **The bootstrap's keyword set grows, so accept `**kwargs`in both variants.**`runsetup.make*adapters`builds every class with`cls(\*\*build_kwargs)`, and that - dict is a \_description of the run* — `run_dir`, `policy`, `profile`, the usage and +- **The bootstrap's keyword set grows.** Accept `**kwargs` in both variants. + `runsetup.make_adapters` builds every class with `cls(**build_kwargs)`, and that + dict is a _description of the run_ — `run_dir`, `policy`, `profile`, the usage and nudge settings, `events_dir` (the run's hook-event channel), plus `mux` for a `needs_mux=True` kind and `paths` for the `dev` variant. Core adds to it as the run gains things worth describing; `events_dir` arrived that way (#494). A class with a