diff --git a/CHANGELOG.md b/CHANGELOG.md index cc632f3e..56bf7a8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -309,6 +309,34 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Fixed +- **`resume`'s config-change baseline moves out of the agent-writable tree (#498).** The host-exec + baseline `resume` compares against — verify commands, launch binary/args/env, plugin allowlist + — round-tripped through `state.json`, inside the very tree the digest exists to police, so the + session that rewrote `policy.toml` could blank the field in the same breath and the warning + never fired. It now lives beside the events channel in the run's out-of-tree state dir + (`///config-digest`, #494), and is collected by the same + `delete`/`archive`/`clean` lifecycle. The auto-sweep gate is unchanged — it always compared + against an in-memory baseline no session can reach. + + `state.json` keeps a second copy, and `resume` consults it **only** when the run has no file + out of tree — so rewriting it silences nothing while that file is in reach. Two cases need it, + and in both the file is honestly absent rather than tampered away: a run paused under an older + version, whose baseline is there and nowhere else (this resume migrates it); and a run whose + project has been moved or renamed. The state root is keyed by the project's _resolved path_, so + a rename keys the run somewhere new and orphans the subtree holding its digest — the copy in + the run directory travels with the run, and the warning survives the move. A project that + later returns to a path it ran under before is the one case this does not get right: the old + subtree is still there, its pin is preferred, and one resume answers off it before re-stamping + and healing (#572). + + **What this does and does not buy.** It closes the _incidental_ path — nothing a session does + in the ordinary course of rewriting project files can blank the pin any more, because the pin + is no longer a project file. It is **not** a boundary against a deliberate session: sessions + launch with permission bypass by default (that is what an unattended loop is), and are handed + `BMAD_LOOP_EVENTS_DIR`, whose parent is the state dir — so a session that goes looking can + still delete or truncate the baseline and silence the warning. Closing that needs privilege + separation on the state dir, not a better hiding place; tracked in #571. + - **Worktree runs no longer stall when a seeded hook config carries the main repo's relay (#352).** `.claude/settings.json` is both a seeded file and the hook `config_path`, so it arrived in the worktree still naming the main repo's `$CLAUDE_PROJECT_DIR`-relative relay diff --git a/docs/FEATURES.md b/docs/FEATURES.md index f4e18b27..dc8c66c9 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -181,7 +181,7 @@ 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. +- 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, the run's `config-digest` (#498), 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. A move does not cost the run its config-change baseline, though: `state.json` carries a second copy that travels with the run directory, and `resume` falls back to it exactly when the out-of-tree file is out of reach (#498). - 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`, 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`. diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 6cdb87e4..df61ec30 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -2127,18 +2127,28 @@ def _resume_paused_run(project: Path, run_dir: Path) -> int: # Deliberately no --allow-config-change flag: a refuse-by-default gate here # would break resume-to-fix-a-setting, which is a supported workflow. # - # ADVISORY, not a control, and the asymmetry with the gate is deliberate. The + # ADVISORY, not a control, and the asymmetry with the gate is deliberate: the # auto-sweep refusal in `_sweep_factory` baselines off a digest threaded in - # MEMORY from launch (see cmd_run/the compose_resume call below), which a - # session cannot reach. This comparison baselines off `state.trusted_config_digest`, - # which round-trips through /.bmad-loop/runs//state.json — inside - # the same agent-writable tree the digest exists to police. A session that - # rewrites policy.toml can blank or re-stamp that field and silence the warning - # (blank suffices: the bool() guard reads "" as no prior pin). It catches the - # incidental change, not a deliberate one. Tracked in #498. - security_config_changed = bool(state.trusted_config_digest) and ( - new_digest != state.trusted_config_digest - ) + # MEMORY from launch (see cmd_run/the compose_resume call below), which nothing + # on disk can reach at all. This one has to survive the pause, so it is read + # back — and since #498 from the run's out-of-tree state dir rather than from + # /.bmad-loop/runs//state.json, which is inside the very tree the + # digest exists to police. It used to be readable there: a session that + # rewrote policy.toml could blank the field in the same breath and silence this + # warning (blank sufficed — the bool() guard reads "" as no prior pin), so the + # advisory caught the incidental change and never the deliberate one. + # + # The legacy fallback, for one release: a run PAUSED under the old code has no + # file in the state root, and its baseline is in state.json and nowhere else. + # `None` from the reader means exactly that "no file" — distinct from an empty + # file, which is a real answer of "no baseline" and must NOT reopen the tree — + # so the fallback fires only for those runs, and the re-stamp below migrates + # them on this very resume. Empty either way keeps its meaning: no prior pin, + # no warning. + pinned = runs.read_trusted_config_digest(project, run_dir.name) + if pinned is None: + pinned = state.trusted_config_digest + security_config_changed = bool(pinned) and new_digest != pinned fields: dict[str, object] = { # Scalars only, per the note above: a bool records THAT the pinned surface # moved without journaling a command, a binary path or a plugin name. @@ -2187,6 +2197,23 @@ def _resume_paused_run(project: Path, run_dir: Path) -> int: # this process is about to arm re-reads it from there. Leaving the launch # digest would make every auto-sweep after a legitimate resume-to-fix-a-setting # refuse — and would make the warning above fire forever, on every later resume. + # + # This is also the migration for a run paused under the old code: it read its + # baseline out of state.json above, and from here on it has a file in the state + # root, so the in-tree copy stops deciding anything for it. + runs.write_trusted_config_digest(project, run_dir.name, new_digest) + # ...and the in-tree secondary is re-stamped in the same breath, for the same + # reason it is written at launch: the state root is keyed by the project's + # RESOLVED PATH (`runs.project_tag`), so moving or renaming the project — a + # documented operation, FEATURES.md states the GC half of it — keys the run + # somewhere new and the file above becomes unreachable. Leaving only that copy + # made a move silently retire the pin, and the advisory this whole guard exists + # to raise never fired again. Writing both keeps the warning alive across a move + # without making this copy authoritative: the reader prefers the out-of-tree + # file whenever it exists, so a session that rewrites this one is still ignored + # (which is #498, and its test still holds). Same for a changed + # BMAD_LOOP_STATE_DIR. Tampering that removes the out-of-tree file is a + # different problem with no fix at equal privilege — #571. state.trusted_config_digest = new_digest state.clear_pause() # A resume is fresh user intent: discard any graceful-stop request left over from diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index a62d62b0..e98e6796 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -467,13 +467,33 @@ class RunState: project: str started_at: str policy_snapshot: dict[str, Any] = field(default_factory=dict) - # runsetup.config_digest over the agent-writable config that reaches HOST code - # execution — verify commands, the resolved launch binary/args/env, the plugin - # allowlist (#461 point 4). Stamped at launch and re-stamped on resume, beside - # policy_snapshot. The auto-sweep gate compares against its own closure - # baseline, not this; the field is the audit record plus the baseline `resume` - # warns off. Empty on runs persisted before the field existed, which is why the - # resume compare is guarded on it being non-empty — no prior pin, no warning. + # SECONDARY copy of the host-exec baseline (#498) — runsetup.config_digest over + # the agent-writable config that reaches HOST code execution: verify commands, + # the resolved launch binary/args/env, the plugin allowlist (#461 point 4). + # + # The one resume TRUSTS is out of the tree (`runs.write_trusted_config_digest`), + # because a baseline whose whole job is to police the agent-writable tree cannot + # live in it — a session that rewrote policy.toml could blank this field in the + # same breath and silence the warning `resume` owes the operator. So this copy is + # never preferred: `_resume_paused_run` consults it ONLY when the state root + # holds no file for the run, which is what keeps rewriting it pointless (the + # #498 attack test asserts exactly that). + # + # It is still written, and must be, for the two cases where the out-of-tree file + # is honestly absent rather than tampered away — in both, this copy is the run's + # only surviving pin: + # * the state root is keyed by the project's RESOLVED PATH (`runs.project_tag`), + # so moving or renaming the project keys the run somewhere new and orphans + # its state subtree (FEATURES.md documents the GC half of this). state.json + # lives in the run dir and travels with it. Same for a BMAD_LOOP_STATE_DIR + # that changes between launch and resume. + # * a run PAUSED before #498 has its baseline here and nowhere else; the first + # resume under this code reads it and mints the out-of-tree file. + # Dropping it would turn "this run has a pin" into "this run has none" in all of + # them. Empty means what it always did — no prior pin, hence no warning — which + # is why the resume compare is guarded on non-emptiness. The auto-sweep gate has + # never read this field: it compares against its own in-memory closure baseline, + # which no session can reach. trusted_config_digest: str = "" current_epic: int | None = None # the run's story scope + cap, as passed on the launching CLI (`--epic`, diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 2fda7d43..1fd10a60 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -9,6 +9,7 @@ import re import secrets import shutil +import stat import sys import tarfile import time @@ -21,6 +22,7 @@ from .platform_util import ( MAX_SEGMENT, atomic_replace, + atomic_write_text, has_parent_ref, is_absolute_path, retrying_unlink, @@ -37,6 +39,15 @@ # already means "hard stop". The engine stays the single writer of journal.jsonl; # requesters only ever touch this file. STOP_REQUEST_FILE = "stop-request.json" +# The host-exec config baseline's name inside a run's state dir (see +# `config_digest_path_for`). A bare hex digest, not JSON: one opaque token, and a +# format an operator can read with `cat`. +CONFIG_DIGEST_FILE = "config-digest" +# Read cap for the file above. A sha256 hex digest is 64 bytes; the slack is for +# a trailing newline and for saying "this is not the digest" out of a file that +# is merely wrong rather than hostile. The cap's real job is the hostile case — +# see `read_trusted_config_digest` on why a bound, not a bigger buffer. +_MAX_DIGEST_BYTES = 256 _INVALID_PID_IDENTITY = -1.0 # impossible process start/create time; forces "not ours" @@ -331,6 +342,146 @@ def events_dir_for(project: Path, run_id: str) -> Path: return state_dir_for(project, run_id) / "events" +def config_digest_path_for(project: Path, run_id: str) -> Path: + """The run's host-exec config baseline: ``runsetup.config_digest`` as of the + last time a human started or resumed this run (#498). + + Out here rather than in ``state.json`` because the baseline exists to police + the agent-writable tree, and until this move it *lived* in it: a session that + rewrote ``policy.toml`` could blank or re-stamp the field in the same breath + and the warning `resume` owes the operator never fired. The same reasoning the + events channel moved on (#494). + + **What moving it buys, stated exactly.** It closes the *incidental* path: the + pin is no longer a project file, so nothing a session does in the ordinary + course of rewriting the tree can collaterally blank it — which is the case the + advisory was documented to catch. It is **not** a boundary against a + deliberate one. Sessions run with permission bypass by default — every shipped + profile's ``bypass_args``, which ``GenericAdapter.interactive_argv`` uses + unless ``[adapter] extra_args`` overrides them; that is what an unattended loop + is — and are handed ``BMAD_LOOP_EVENTS_DIR``, whose parent is this directory. A + session that goes looking can *truncate* this file and the reader below answers + ``""`` — a real "no baseline" — or delete it and blank the in-tree copy + (``RunState.trusted_config_digest``, the secondary this falls back to) for the + same silence. Either way the result is indistinguishable from a run that never + had a baseline: any marker saying "this run *should* have one" would have to + live somewhere the same session cannot reach, and no such place exists at equal + privilege. Closing it needs privilege separation on the state root, not a better + hiding place — tracked in #571.""" + return state_dir_for(project, run_id) / CONFIG_DIGEST_FILE + + +def read_trusted_config_digest(project: Path, run_id: str) -> str | None: + """This run's persisted host-exec baseline, or ``None`` when the state root + holds none for it. + + ``None`` is "ask the in-tree copy", not "no pin" — the two are different + answers and the caller acts on the difference (see + ``cli._resume_paused_run``). No file here means this run's baseline is + reachable only through ``state.json``: it was paused before #498, or the + project moved and keyed its state subtree somewhere new + (:func:`project_state_root`). An *empty* file, by contrast, is a real answer + of "no baseline" and comes back as ``""``. + + **Known limit: a file at this key can be stale (#572).** The key is the + project's resolved path, so a project that moves away and later returns finds + its old subtree still here — nothing can sweep it in between (FEATURES.md) — + holding the baseline blessed before it left, while the blessing it picked up + in between is the one in ``state.json``. Preferring the file means that older + pin wins for one resume, which re-stamps this key and heals it. Preferring the + fresher-looking in-tree copy is *not* the fix: it is session-writable, so it + would hand any session the silencing #498 closed. Arbitrating by sequence + number needs a counterpart the session cannot forge, and at equal privilege + there is none — the same wall as #571, reached by re-keying instead of + tampering. + + Pure observation, so it degrades rather than raising: a state root this host + cannot name, or a file it cannot read, both answer ``None`` and hand the + decision to the in-tree copy. The write half raises — see + :func:`write_trusted_config_digest` — and the split is the standard one + (``platform_util.resolve_or_lexical`` states the doctrine). Degrading here + costs at most one advisory warning; a resume that *aborts* because an + advisory could not be read would be the worse failure, and the resume is + about to resolve the same state root for its events channel anyway, where + the error is owned and reported. + + **Deliberately not ``read_text``**, and for the same reason the write is + ``follow_symlinks=False``: this file sits in a directory the driven session + can reach (its parent is the ``BMAD_LOOP_EVENTS_DIR`` the engine exports), so + the *shape* of what is at the path has to be established before any bytes are + consumed. Degrading on a hostile path is not enough when the read itself is + the weapon: + + * ``O_NONBLOCK`` + an ``S_ISREG`` check **on the descriptor**. Opening a FIFO + for reading otherwise blocks until someone writes — indefinitely — and + ``resume`` is a foreground command a human is waiting on, so a planted FIFO + wedges the terminal rather than costing a warning. The check is on the fd, + not the path, so it cannot be raced: ``fstat`` describes the object actually + opened. + * ``O_NOFOLLOW``, so the name is read rather than wherever it points. + * At most :data:`_MAX_DIGEST_BYTES`. A link to an endless source + (``/dev/zero``) reads forever otherwise, and raises ``MemoryError`` — not + the ``OSError`` this promises never to leak. The cap removes the condition + instead of absorbing it. + + The POSIX-only flags degrade to 0 on win32, which has neither FIFOs at these + paths nor ``O_NOFOLLOW``; the size cap and the regular-file check carry there + on their own. This mirrors ``tui.launch._read_ctl_window`` deliberately — same + hazard, same shape, one idiom. It does **not** collapse empty to ``None`` the + way that twin does: here the two are different answers (above). + + None of this makes the baseline tamper-*proof* — a session can still delete + the file, and #571 carries that. It stops a tampered path from hanging or + exhausting the orchestrator, which is a different and fixable harm.""" + try: + path = config_digest_path_for(project, run_id) + except (StateRootError, OSError, RuntimeError): + return None + flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_NONBLOCK", 0) + flags |= getattr(os, "O_BINARY", 0) # win32: no CRLF translation on the raw fd + try: + fd = os.open(path, flags) + except OSError: + return None + try: + if not stat.S_ISREG(os.fstat(fd).st_mode): + return None + data = os.read(fd, _MAX_DIGEST_BYTES) + except OSError: + return None + finally: + os.close(fd) + try: + return data.decode("utf-8").strip() + except UnicodeDecodeError: + return None + + +def write_trusted_config_digest(project: Path, run_id: str, digest: str) -> None: + """Stamp ``digest`` as this run's host-exec baseline, creating the state dir. + + Raises rather than degrading — a repair write, and a silently skipped stamp + is the outcome hardest to detect later: the next resume reads no file and + decides on the in-tree copy alone, which is the tree this baseline exists to + police. The caller is starting or resuming a run and is about to resolve the + very same state root for its events channel, so a root that cannot be named + or written fails that run regardless; failing here just fails it sooner, + before the pid lands. + + **Call this only after the run dir exists.** Creating the state dir is what + makes this the earliest writer into it, and :func:`reconcile_orphan_state_dirs` + reads its entries *before* the live run-dir names on the strength of run dirs + being created strictly first — a state dir minted ahead of its run dir would + look like an orphan to a ``clean`` racing the launch.""" + path = config_digest_path_for(project, run_id) + path.parent.mkdir(parents=True, exist_ok=True) + # follow_symlinks=False: a machine-minted record under a root whose path the + # driven session is handed (BMAD_LOOP_EVENTS_DIR names its sibling), so a + # planted link here must be replaced, never written through to whatever it + # aims at. The trailing newline is for the operator who cats the file. + atomic_write_text(path, digest + "\n", follow_symlinks=False) + + # ---------------------------------------------------- run resolution / liveness @@ -1042,8 +1193,10 @@ def reconcile_orphan_state_dirs(project: Path, *, dry_run: bool = False) -> list **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 + ``Journal`` (which mkdirs the run dir) and only then stamps the config digest + (:func:`write_trusted_config_digest`, the earliest writer into the state dir + since #498) and calls ``make_adapters``, whose ``SignalWatcher`` mkdirs the + events dir alongside it. 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 diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index 4c46accf..a399ede6 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -744,21 +744,22 @@ def build_run_state( the weights the run actually launched under; ``source`` / ``spec_folder`` record which queue the run dispatches (a stories manifest vs sprint-status). - ``trusted_config_digest`` is passed in rather than derived from ``policy`` + - ``project`` here so the value stamped on the run is the same string the caller - handed the sweep factory — and so a broken profile still aborts where it always - did (``make_adapters``'s ``SystemExit``), not from inside this constructor.""" + ``trusted_config_digest`` is carried here **as well as** stamped out of the + tree by :func:`compose_run` (#498). The out-of-tree file is the one resume + trusts; this copy is the secondary that travels with the run directory — see + ``RunState.trusted_config_digest`` for why a run that outlives its state key + needs one.""" return RunState( run_id=run_id, project=str(project), started_at=time.strftime("%Y-%m-%dT%H:%M:%S"), policy_snapshot=policy.to_dict(), - trusted_config_digest=trusted_config_digest, epic_filter=epic_filter, story_filter=story_filter, max_stories=max_stories, source="stories" if stories_on else "sprint-status", spec_folder=spec_folder if stories_on else "", + trusted_config_digest=trusted_config_digest, ) @@ -806,6 +807,13 @@ def compose_run( baseline describes the bytes these adapters are built from rather than a second read of an agent-writable file (#461 point 4). ``None`` resolves fresh. + ``trusted_config_digest`` is stamped into the run's out-of-tree state dir + (#498), so the baseline ``resume`` warns off is not sitting in the tree the + driven sessions write to, **and** onto the :class:`RunState` as the secondary + that travels with the run dir. The out-of-tree copy is preferred whenever it + exists, which is what keeps the in-tree one from being worth tampering with; + ``RunState.trusted_config_digest`` states the split and why both are needed. + ``make_adapters`` and the engine classes are injected (rather than imported here) so ``cli`` supplies its own module-level names — keeping the test suite's ``monkeypatch.setattr(cli, "Engine"/"_make_adapters", ...)`` effective. @@ -825,6 +833,10 @@ def compose_run( trusted_config_digest=trusted_config_digest, ) save_state(run_dir, state) + # After the run dir exists (Journal mkdir'd it above) and before the pid lands: + # the ordering `reconcile_orphan_state_dirs` reads runs in, and a stamp that + # cannot be written fails the launch before an observer can see a live run. + runs.write_trusted_config_digest(project, run_id, trusted_config_digest) runs.write_pid(run_dir) adapters = make_adapters(project, run_dir, policy, profiles=profiles) journal.append( @@ -881,6 +893,8 @@ def compose_sweep( to ``make_adapters``. The child-sweep factory passes the same one it gated on, so the adapters are built from the validated bytes instead of a fresh read of an agent-writable file (#461 point 4); ``cmd_sweep`` (human-present) omits it. + ``trusted_config_digest`` lands in the run's out-of-tree state dir and, as the + travelling secondary, on the :class:`RunState` — see :func:`compose_run`. ``sweep.json`` freezes the launch options so a resume rebuilds the same sweep (see :func:`compose_resume`). ``make_adapters`` and ``sweep_engine_cls`` are @@ -895,10 +909,12 @@ def compose_sweep( project=str(project), started_at=time.strftime("%Y-%m-%dT%H:%M:%S"), policy_snapshot=policy.to_dict(), - trusted_config_digest=trusted_config_digest, run_type="sweep", + trusted_config_digest=trusted_config_digest, ) save_state(run_dir, state) + # Out of the tree, same ordering and same reason as compose_run's stamp. + runs.write_trusted_config_digest(project, run_id, trusted_config_digest) runs.write_pid(run_dir) options = { "prompting": prompting, diff --git a/tests/test_cli.py b/tests/test_cli.py index f46f86ed..b7efa8c3 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -5,6 +5,7 @@ import json import ntpath import os +import shutil import sys import types from pathlib import Path @@ -3434,15 +3435,18 @@ def test_resume_warns_when_the_pinned_host_exec_config_changed(project, monkeypa """#461 point 4, human-present half. A session may have rewritten the verify commands while the operator was away; `resume` is the moment they can still look, so the change stops being silent. It stays a WARNING — a human typed - `resume`, and refusing here would break resume-to-fix-a-setting.""" - from bmad_loop.journal import load_state + `resume`, and refusing here would break resume-to-fix-a-setting. + + The baseline round-trips through the out-of-tree state dir since #498; the + behaviour asserted here is the one that must NOT have changed with it.""" + from bmad_loop import runs run_dir = _paused_run_for_resume(project, monkeypatch) monkeypatch.setattr(cli, "Engine", _StubEngine) # First resume stamps the pin (the run predates the field, so it has none). assert cli._resume_paused_run(project.project, run_dir) == 0 assert _resume_entry(run_dir)["security_config_changed"] is False - pinned = load_state(run_dir).trusted_config_digest + pinned = runs.read_trusted_config_digest(project.project, run_dir.name) assert pinned # the launch/resume baseline is persisted, not just in memory capsys.readouterr() @@ -3456,7 +3460,125 @@ def test_resume_warns_when_the_pinned_host_exec_config_changed(project, monkeypa assert "verify commands" in err and "plugin allowlist" in err assert "touch pwned" not in err # ...and the resume re-blesses it, so the next one is quiet again. - assert load_state(run_dir).trusted_config_digest != pinned + assert runs.read_trusted_config_digest(project.project, run_dir.name) != pinned + + +def test_resume_still_warns_when_a_session_rewrote_the_digest_in_state_json( + project, monkeypatch, capsys +): + """#498, the whole point of the relocation. The baseline used to round-trip + through `/.bmad-loop/runs//state.json` — inside the very tree the + digest exists to police — so the session that rewrote `policy.toml` could blank + the field in the same breath and the warning never fired. Blanking sufficed: + the guard reads "" as "no prior pin". + + This is the attack, run end to end: change the host-exec config AND blank the + in-tree field, exactly as one session could. The warning must still fire, + because the baseline that decides it is now out of the tree. + + ABLATION: point `_resume_paused_run`'s read back at + `state.trusted_config_digest` (or make the reader prefer the legacy field over + the state-root file) and this goes quiet — no warning, `security_config_changed` + False.""" + from bmad_loop import runs + from bmad_loop.journal import load_state, save_state + + run_dir = _paused_run_for_resume(project, monkeypatch) + monkeypatch.setattr(cli, "Engine", _StubEngine) + assert cli._resume_paused_run(project.project, run_dir) == 0 + assert runs.read_trusted_config_digest(project.project, run_dir.name) + capsys.readouterr() + + # The session rewrites the verify commands... + _write_policy(project.project, RESUME_POLICY.replace('["true"]', '["touch pwned"]')) + # ...and covers its tracks in the one file it can reach. + state = load_state(run_dir) + state.trusted_config_digest = "" + save_state(run_dir, state) + assert load_state(run_dir).trusted_config_digest == "" # the cover-up landed + + assert cli._resume_paused_run(project.project, run_dir) == 0 + + assert _resume_entries(run_dir)[-1]["security_config_changed"] is True + assert "host-exec config pinned at launch has changed" in capsys.readouterr().err + + +def test_resume_migrates_a_pre_498_baseline_out_of_state_json(project, monkeypatch, capsys): + """The one-release legacy fallback. A run PAUSED under the old code has its + baseline in `state.json` and nowhere else, so reading only the state root would + silently drop the pin for exactly the runs that sat idle longest — turning + "this run has a pin" into "this run has none" on the first resume after the + upgrade, which is the empty-means-legacy contract read backwards. + + So: no state-root file + a non-empty in-tree field = compare against the field, + warn, and migrate. After this resume the run has a file out of tree, and the + in-tree copy stops deciding anything for it while that file stays reachable. + + ABLATION: drop the `if pinned is None` fallback in `_resume_paused_run` and the + warning assert fails (the migrated run reads no baseline and stays quiet).""" + from bmad_loop import runs + + # A run persisted by the old code: a pin in state.json, nothing out of tree. + run_dir = _paused_run_for_resume(project, monkeypatch, trusted_config_digest="stale-pin") + monkeypatch.setattr(cli, "Engine", _StubEngine) + assert runs.read_trusted_config_digest(project.project, run_dir.name) is None + + assert cli._resume_paused_run(project.project, run_dir) == 0 + + # It compared against the legacy field, so the change is caught on this resume. + assert _resume_entries(run_dir)[-1]["security_config_changed"] is True + assert "host-exec config pinned at launch has changed" in capsys.readouterr().err + # ...and migrated: the baseline now lives out of tree, and is the fresh one. + migrated = runs.read_trusted_config_digest(project.project, run_dir.name) + assert migrated and migrated != "stale-pin" + + +def test_resume_still_warns_after_the_project_is_renamed(project, monkeypatch, capsys): + """The out-of-tree baseline is keyed by the project's RESOLVED PATH + (`runs.project_tag`), so moving or renaming the project keys the run somewhere + new and orphans the subtree holding its `config-digest` — FEATURES.md documents + the GC half of the same fact. state.json travels with the run dir instead. + + Stamping ONLY out of tree therefore made a rename silently retire the pin: the + reader answers `None`, the in-tree copy was never written under that code, and + an empty fallback means "no prior pin, no warning" — so a host-exec config + change across a move was accepted in silence, and every later resume stayed + quiet too. No tampering involved; a documented operator action. Pre-#498 the + pin lived in state.json and survived this, which makes it a regression, not a + pre-existing hole. Writing both copies keeps the warning alive here WITHOUT + making the in-tree one authoritative — the test above is the other half of that + pair, and still passes: with the file present, this copy loses. + + ABLATION: drop `state.trusted_config_digest = new_digest` from + `_resume_paused_run` (or the `trusted_config_digest=` argument from + `runsetup.build_run_state`) and the warning assert fails.""" + from bmad_loop import runs + + run_dir = _paused_run_for_resume(project, monkeypatch) + monkeypatch.setattr(cli, "Engine", _StubEngine) + assert cli._resume_paused_run(project.project, run_dir) == 0 + assert runs.read_trusted_config_digest(project.project, run_dir.name) + capsys.readouterr() + + # The operator renames the project directory; the run dir goes with it. + src = project.project + dst = src.parent / (src.name + "-renamed") + shutil.move(str(src), str(dst)) + moved = dst / run_dir.relative_to(src) + assert moved.is_dir() + # Precondition, or this test would pass for the wrong reason: the rename really + # did put the out-of-tree baseline out of reach. + assert runs.project_tag(dst) != runs.project_tag(src) + assert runs.read_trusted_config_digest(dst, run_dir.name) is None + + # ...and the host-exec config changes, exactly as in the no-move case. + _write_policy(dst, RESUME_POLICY.replace('["true"]', '["touch pwned"]')) + assert cli._resume_paused_run(dst, moved) == 0 + + assert _resume_entries(moved)[-1]["security_config_changed"] is True + assert "host-exec config pinned at launch has changed" in capsys.readouterr().err + # The resume re-keys the run: from here the baseline is out of tree again. + assert runs.read_trusted_config_digest(dst, run_dir.name) def test_resume_under_an_unchanged_host_exec_config_reports_no_security_change( @@ -7635,7 +7757,7 @@ def test_auto_sweep_launches_the_profile_bytes_the_gate_validated(project, monke ABLATION: drop `profiles=` from either `runsetup.make_adapters` or `_trusted_config_digest` in `_start_sweep` and the matching assert fails.""" - from bmad_loop import bmadconfig + from bmad_loop import bmadconfig, runs from bmad_loop.adapters import profile as profile_mod monkeypatch.setattr(mux_mod, "_usable", lambda mux: True) @@ -7676,6 +7798,11 @@ def run(self): profile_mod.get_profile("mycli", project.project).binary == "rogue-cli" ), "the swap must actually have landed on disk, or this test proves nothing" assert captured["adapter"].profile.binary == "mycli" + run_id = captured["state"].run_id + assert runs.read_trusted_config_digest(project.project, run_id) == pin + # Both copies, and the same validated bytes in each: the in-tree secondary is + # what survives a project rename (the state root is keyed by resolved path), so + # a launch that stamped only out of tree loses the pin on the first move. assert captured["state"].trusted_config_digest == pin @@ -7707,6 +7834,7 @@ def test_run_pins_the_profile_bytes_it_launches(project, monkeypatch): after the swap and pins config the run never launched).""" from conftest import git, install_base_skills + from bmad_loop import runs from bmad_loop.adapters import profile as profile_mod monkeypatch.setattr(mux_mod, "_usable", lambda mux: True) @@ -7748,6 +7876,10 @@ def run(self): profile_mod.get_profile("mycli", project.project).binary == "rogue-cli" ), "the swap must actually have landed on disk, or this test proves nothing" assert captured["adapter"].profile.binary == "mycli" + run_id = captured["state"].run_id + assert runs.read_trusted_config_digest(project.project, run_id) == pin + # As in the sweep twin: the launch stamps both copies, out-of-tree (trusted) and + # in-tree (travels with the run dir when the project is renamed). assert captured["state"].trusted_config_digest == pin @@ -7804,7 +7936,7 @@ def __init__(self, **kwargs): profile_mod.get_profile("mycli", project.project).binary == "rogue-cli" ), "the swap must actually have landed on disk, or this test proves nothing" assert captured["adapter"].profile.binary == "mycli" - assert captured["state"].trusted_config_digest == pin + assert runs.read_trusted_config_digest(project.project, run_dir.name) == pin def test_dry_run_banner_names_the_isolation_refusal_first(project, capsys): diff --git a/tests/test_runs.py b/tests/test_runs.py index b5224ba7..dc8d5b8c 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -7,6 +7,7 @@ import sys import tarfile from pathlib import Path +from unittest import mock import pytest from conftest import escalated_run, git, refuse_to_resolve @@ -961,6 +962,269 @@ def test_state_dir_for_raises_when_the_project_cannot_be_canonicalized(tmp_path, runs.state_dir_for(project, "r1") +def test_config_digest_is_stamped_under_the_state_root_not_in_the_project(tmp_path): + """#498's whole point: the baseline `resume` TRUSTS leaves the tree the driven + sessions can write to. + + The negative half is the load-bearing one — asserting only that the state root + holds the digest would still pass if this writer also dropped a copy in the + project, and a file inside `.bmad-loop/` is exactly the thing a session edits + to silence the warning. + + Scope, so the negative assert is not read as more than it is: it pins THIS + function, which writes out of tree and nowhere else. The run as a whole does + keep a second copy in `state.json` (`RunState.trusted_config_digest`, itself + under `.bmad-loop/runs/`) — the travelling secondary a project move needs, and + deliberately never preferred over this file. `test_cli` owns that precedence: + `..._still_warns_when_a_session_rewrote_the_digest_in_state_json` proves the + in-tree copy loses, `..._still_warns_after_the_project_is_renamed` proves it is + consulted when this file is out of reach.""" + project = tmp_path / "proj" + (project / ".bmad-loop").mkdir(parents=True) + + runs.write_trusted_config_digest(project, "r1", "abc123") + + path = runs.config_digest_path_for(project, "r1") + assert path == runs.state_dir_for(project, "r1") / "config-digest" + assert runs.read_trusted_config_digest(project, "r1") == "abc123" + assert not any(p.is_file() for p in (project / ".bmad-loop").rglob("*")) + + +def test_read_trusted_config_digest_separates_an_absent_file_from_an_empty_one(tmp_path): + """`None` and `""` are different answers and the resume acts on the + difference: `None` means "this run predates #498, ask state.json", while `""` + means "a baseline was stamped and it is empty" and must NOT reopen the + agent-writable field. Collapsing them to `""` would retire the legacy runs' + fallback; collapsing them to `None` would let a session that truncates the + out-of-tree file fall back into the tree it controls. + + ABLATION: return `""` instead of `None` from the reader's except arm, or drop + the `.strip()`-of-an-empty-file distinction, and one of these two fails.""" + project = tmp_path / "proj" + project.mkdir() + + assert runs.read_trusted_config_digest(project, "r1") is None + + runs.write_trusted_config_digest(project, "r1", "") + assert runs.read_trusted_config_digest(project, "r1") == "" + + +@pytest.mark.parametrize( + "attr, exc", + [ + ("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", "symlink-loop-project"], +) +def test_trusted_config_digest_read_degrades_where_the_write_raises( + tmp_path, monkeypatch, attr, exc +): + """The halves are deliberately asymmetric, and each row runs both. + + Reading is observation feeding an advisory warning, so an unnameable state + root costs the warning and nothing else — and the resume is about to resolve + the same root for its events channel, where the error is owned and reported. + Writing is a repair write, and a silently skipped stamp is undetectable later: + the next resume finds no file, falls back to a legacy field that is empty for + any run this code started, and quietly declines to warn. + + The `RuntimeError` row is live below 3.13, where `Path.resolve` reports a + symlink loop that way — same reason `_discard_state_dir` holds it. + + ABLATION: widen the write to swallow these and the second half passes.""" + project = tmp_path / "proj" + project.mkdir() + monkeypatch.setattr(runs, attr, _raising(exc)) + + assert runs.read_trusted_config_digest(project, "r1") is None + with pytest.raises(type(exc)): + runs.write_trusted_config_digest(project, "r1", "abc123") + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX FIFOs") +def test_read_trusted_config_digest_refuses_a_planted_fifo_instead_of_hanging(tmp_path): + """The read is on a path the driven session can reach, so its *shape* has to be + established before any bytes are consumed. A FIFO opened for reading blocks + until someone writes — indefinitely — and `resume` is a foreground command a + human is waiting on, so this wedges the terminal rather than costing a warning. + + Guarded by `SIGALRM` because the failure mode under test IS a hang: an + unguarded call would not fail, it would never return, and the suite would sit + there until CI killed the job with no attributable test. The alarm converts + "never returns" into a named assertion. + + Deliberately not a `multiprocessing.Process` with a join timeout, which was + the first draft: the default start method on Linux is `fork`, and forking a + process pytest may already have threaded earns a DeprecationWarning on 3.12+ + and risks a child deadlock — trading a hang under ablation for a possible hang + in the ordinary run. The alarm stays in one process and needs no picklable + target. POSIX-only, which this test already is. + + ABLATION: restore `Path.read_text` in the reader, or drop `O_NONBLOCK`, and + the alarm fires. Dropping the `S_ISREG` check instead fails the assert rather + than the alarm — with no writer the FIFO reads EOF, so the reader answers `""` + where it owes `None`. Both are graded; the twin below covers the case where a + writer makes those bytes attacker-chosen instead of empty.""" + import signal + + project = tmp_path / "proj" + project.mkdir() + path = runs.config_digest_path_for(project, "r1") + path.parent.mkdir(parents=True) + os.mkfifo(path) + + def _blew_up(signum, frame): + raise AssertionError("the read blocked on the FIFO instead of refusing it") + + previous = signal.signal(signal.SIGALRM, _blew_up) + signal.alarm(20) + try: + assert runs.read_trusted_config_digest(project, "r1") is None + finally: + signal.alarm(0) + signal.signal(signal.SIGALRM, previous) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX FIFOs") +def test_read_trusted_config_digest_refuses_a_fed_fifo_instead_of_reading_it(tmp_path): + """The twin of the FIFO test above, on the half that is the actual attack. That + one plants an idle FIFO, so the harm is a hang and — with the `S_ISREG` check + gone — the bytes read are merely empty. Here a writer is holding it open and + feeding it, so a reader that got as far as `os.read` would come back with + whatever the session piped in and treat it as this run's baseline: feed the + digest of the config it just installed and `resume` is satisfied, feed noise + and the operator is warned off a change nobody made. Neither is a hang, so the + alarm above would never notice. + + Opened `O_RDWR` deliberately: a write-only open on a FIFO blocks until a reader + arrives, which would wedge the test itself, and `O_RDWR` never blocks. + + ABLATION: drop the `S_ISREG` check and this returns the piped text instead of + `None` — the assert names the value it got.""" + project = tmp_path / "proj" + project.mkdir() + path = runs.config_digest_path_for(project, "r1") + path.parent.mkdir(parents=True) + os.mkfifo(path) + + holder = os.open(path, os.O_RDWR | os.O_NONBLOCK) + try: + os.write(holder, b"ff" * 32 + b"\n") # a plausible-looking sha256 hex digest + assert runs.read_trusted_config_digest(project, "r1") is None + finally: + os.close(holder) + + +def test_read_trusted_config_digest_is_bounded(tmp_path): + """A link to an endless source (`/dev/zero`) would otherwise read until + `MemoryError` — a ValueError-family escape from a function that promises never + to raise. The cap removes the condition rather than absorbing it. + + A large regular file stands in for the endless one: same read path, same + bound, and it runs on every platform. + + What is asserted is the BOUND ON THE READ, which is not the same as the length + of what comes back, and the difference is the whole point: a reader that + slurped the file and only then truncated would return exactly + `_MAX_DIGEST_BYTES` too, pass a returned-length assert, and still exhaust + memory on /dev/zero — the condition this cap exists to remove rather than + absorb. So the requested counts are captured at `os.read` and totalled. (The + returned-length assert stays: it is what catches a cap applied to the read but + not honoured afterwards.) + + ABLATION: two rows, and the first is the one a length-only assert misses. + Slurp the whole file and truncate at the end — the total-bytes assert fails, + the length assert does not. Drop `_MAX_DIGEST_BYTES` from the `os.read` + outright and both fail.""" + project = tmp_path / "proj" + project.mkdir() + path = runs.config_digest_path_for(project, "r1") + path.parent.mkdir(parents=True) + path.write_text("a" * (1024 * 1024)) + + requested: list[int] = [] + real_read = os.read + + def _spy(fd: int, n: int) -> bytes: + requested.append(n) + return real_read(fd, n) + + with mock.patch.object(runs.os, "read", _spy): + got = runs.read_trusted_config_digest(project, "r1") + + assert got is not None + assert len(got) == runs._MAX_DIGEST_BYTES + # The read never asks for more than the cap, however many calls it makes. + assert requested, "the spy saw no read at all — the assertion below would be vacuous" + assert sum(requested) <= runs._MAX_DIGEST_BYTES + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_read_trusted_config_digest_does_not_follow_a_planted_symlink(tmp_path): + """`O_NOFOLLOW`: the name is read, not wherever it points. Without it a session + aims the orchestrator's read at any file the orchestrator can open, and the + "digest" it comes back with is that file's contents. + + ABLATION: drop `O_NOFOLLOW` from the flags and the read returns the target's + contents instead of `None`.""" + project = tmp_path / "proj" + project.mkdir() + secret = tmp_path / "elsewhere.txt" + secret.write_text("not-the-digest") + path = runs.config_digest_path_for(project, "r1") + path.parent.mkdir(parents=True) + path.symlink_to(secret) + + assert runs.read_trusted_config_digest(project, "r1") is None + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_write_trusted_config_digest_replaces_a_planted_symlink(tmp_path): + """`follow_symlinks=False`, and the reason is that this record lives under a + root whose path the driven session is handed — the engine exports the sibling + events dir as `BMAD_LOOP_EVENTS_DIR`. Following a link planted at the digest's + name would aim an orchestrator write at a path of the session's choosing. + + ABLATION: drop `follow_symlinks=False` and the target below is what gets + written.""" + project = tmp_path / "proj" + project.mkdir() + target = tmp_path / "outside.txt" + target.write_text("untouched") + path = runs.config_digest_path_for(project, "r1") + path.parent.mkdir(parents=True) + path.symlink_to(target) + + runs.write_trusted_config_digest(project, "r1", "abc123") + + assert target.read_text() == "untouched" + assert not path.is_symlink() + assert runs.read_trusted_config_digest(project, "r1") == "abc123" + + +def test_the_state_dir_gc_reclaims_the_config_digest(tmp_path): + """#498's GC is #494's GC — the digest is a file inside the run's state dir, so + the lifecycle that already reclaims that subtree reclaims this too. Asserted + rather than assumed: a digest stamped somewhere the sweep does not reach would + leak one file per run, outside the project, for the life of the machine. + + Deliberately the ORPHAN SWEEP and not `delete_run`, which was the first draft + and was fake green — it removes the run dir as well, so it passes whether the + digest is out of tree or sitting in `.bmad-loop/runs//`, which is the one + thing this needs to tell apart. `reconcile_orphan_state_dirs` reaches only + out-of-tree state, so a digest that drifted back into the project survives it + and this reddens.""" + runs.write_trusted_config_digest(tmp_path, "r1", "abc123") + digest = runs.config_digest_path_for(tmp_path, "r1") + assert digest.is_file() + + assert runs.reconcile_orphan_state_dirs(tmp_path) == [runs.state_dir_for(tmp_path, "r1")] + + assert not digest.exists() + + 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())