From 3ba80353d5078f6ddc3962477d8ed8f25f548e4b Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 20:48:02 -0700 Subject: [PATCH 1/6] fix(resume): move the config-change baseline out of the agent-writable tree (#498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The host-exec digest `resume` warns off round-tripped through state.json, inside the very tree it exists to police: 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". It now lives beside the events channel at `///config-digest` (#494), written with `atomic_write_text(follow_symlinks=False)` — the sibling events dir is handed to the session as BMAD_LOOP_EVENTS_DIR, so a planted link at this name must be replaced, not written through. All three stamp sites move together (compose_run, compose_sweep, the resume re-stamp); `RunState.trusted_config_digest` is no longer written. The auto-sweep gate is untouched — it always compared against an in-memory closure baseline no session can reach. The field is kept one release as a read-only 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 turn "this run has a pin" into "this run has none" for exactly the runs that sat idle longest — the empty-means-legacy contract read backwards. The reader returns None for "no file" and "" for an empty one; only None consults the field, and the re-stamp migrates the run on that same resume. Empty keeps its meaning either way: no prior pin, no warning. GC is free — the digest is a file in the state dir that delete/archive/clean already reclaim. Read degrades, write raises, per the observation/repair split. Not a boundary against --dangerously-skip-permissions: the env var names the directory. Same honest limit as #494. --- CHANGELOG.md | 12 ++++ docs/FEATURES.md | 2 +- src/bmad_loop/cli.py | 41 +++++++++---- src/bmad_loop/model.py | 26 ++++++--- src/bmad_loop/runs.py | 74 +++++++++++++++++++++++- src/bmad_loop/runsetup.py | 24 +++++--- tests/test_cli.py | 93 +++++++++++++++++++++++++++--- tests/test_runs.py | 117 ++++++++++++++++++++++++++++++++++++++ 8 files changed, 351 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc632f3e..d70ed58f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -309,6 +309,18 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Fixed +- **A session can no longer silence `resume`'s config-change warning (#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. Runs paused under an older version keep + their old baseline for this release: `resume` falls back to `state.json` when the run has no + file out of tree, warns as before, and migrates it. The field is no longer written; it is not + a hard boundary against a `--dangerously-skip-permissions` session, which can name the + directory. - **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..7a418338 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. - 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..d64868ca 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,7 +2197,14 @@ 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. - state.trusted_config_digest = new_digest + # + # 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 fallback never fires for it again. The legacy field is left as it + # was found rather than cleared — it is that run's honest record of what it + # launched under, nothing reads it once the file exists, and erasing persisted + # data to tidy up is not this change's business. + runs.write_trusted_config_digest(project, run_dir.name, new_digest) state.clear_pause() # A resume is fresh user intent: discard any graceful-stop request left over from # a prior stopped-gracefully run so the re-armed engine does not consume it at the diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index a62d62b0..3d3f8440 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -467,13 +467,25 @@ 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. + # LEGACY, read-only, one release (#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). It used + # to be stamped here at launch and re-stamped on resume, beside policy_snapshot; + # it is now stamped out of the tree instead (`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. + # + # Nothing writes it any more, so it is "" on every run this code starts. It is + # still PARSED and still round-tripped by to_dict, for the runs that were paused + # under the old code and are resumed under this one: their baseline is here and + # nowhere else, and `_resume_paused_run` falls back to it when the state root + # holds no file for the run. Dropping it outright would turn "this run has a + # pin" into "this run has none" for exactly those runs, which is the empty-means + # -legacy contract read backwards. Empty still 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..0121e8ad 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -21,6 +21,7 @@ from .platform_util import ( MAX_SEGMENT, atomic_replace, + atomic_write_text, has_parent_ref, is_absolute_path, retrying_unlink, @@ -37,6 +38,10 @@ # 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" _INVALID_PID_IDENTITY = -1.0 # impossible process start/create time; forces "not ours" @@ -331,6 +336,69 @@ 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. A pin a session can + edit is not a pin. The same reasoning the events channel moved on (#494), and + with the same honest limit — see :func:`state_root`.""" + 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 legacy field", not "no pin" — the two are different + answers and the caller acts on the difference (see + ``cli._resume_paused_run``). A run started before #498 has no file here and + its baseline is still in ``state.json``; an *empty* file, by contrast, is a + real answer of "no baseline" and comes back as ``""``. + + 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 legacy fallback. 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.""" + try: + return config_digest_path_for(project, run_id).read_text(encoding="utf-8").strip() + except (StateRootError, OSError, RuntimeError, 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 one outcome that cannot be detected later: the next resume reads no + file, falls back to a legacy field that is empty for any run this new code + started, and quietly declines to warn. 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 +1110,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..f3d775fb 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -736,7 +736,6 @@ def build_run_state( max_stories: int | None, stories_on: bool, spec_folder: str, - trusted_config_digest: str, ) -> RunState: """Assemble the launch-time :class:`RunState` for a fresh run. @@ -744,16 +743,15 @@ 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.""" + No ``trusted_config_digest``: since #498 the host-exec baseline is stamped out + of the project tree by :func:`compose_run`, not carried on the state (see + ``RunState.trusted_config_digest``, kept one release as a read-only legacy + fallback).""" 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, @@ -806,6 +804,10 @@ 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 + rather than onto the :class:`RunState` (#498), so the one baseline ``resume`` + warns off is not sitting in the tree the driven sessions write to. + ``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. @@ -822,9 +824,12 @@ def compose_run( max_stories=max_stories, stories_on=stories_on, spec_folder=spec_folder, - 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 +886,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, not 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 +902,11 @@ 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", ) 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..53d1b704 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -3434,15 +3434,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 +3459,78 @@ 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 legacy field = compare against the legacy + field, warn, and migrate. The second half is what bounds the fallback to one + release — after this resume the run has a file out of tree and never consults + the field again. + + 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_under_an_unchanged_host_exec_config_reports_no_security_change( @@ -7635,7 +7709,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,7 +7750,8 @@ 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" - assert captured["state"].trusted_config_digest == pin + run_id = captured["state"].run_id + assert runs.read_trusted_config_digest(project.project, run_id) == pin def test_run_pins_the_profile_bytes_it_launches(project, monkeypatch): @@ -7707,6 +7782,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,7 +7824,8 @@ 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" - assert captured["state"].trusted_config_digest == pin + run_id = captured["state"].run_id + assert runs.read_trusted_config_digest(project.project, run_id) == pin def test_resume_pins_the_profile_bytes_it_launches(project, monkeypatch): @@ -7804,7 +7881,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..5e340af8 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -961,6 +961,123 @@ 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` warns off 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 the project also kept a copy, and a copy + inside `.bmad-loop/` is exactly the thing a session edits to silence the + warning.""" + 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 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()) From 43e92cffd960975cada946b5209aaad9a3d27094 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 20:58:47 -0700 Subject: [PATCH 2/6] docs(resume): say exactly what moving the baseline buys, and what it does not (#571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CHANGELOG headline claimed a session can no longer silence resume's config-change warning. It contradicted the caveat three sentences later, and codex is right that the caveat is the main case, not a corner: every shipped profile launches with permission bypass by default, and the session is handed BMAD_LOOP_EVENTS_DIR, whose parent is the state dir. Deleting or truncating the baseline there silences the warning exactly as blanking the state.json field used to. What the move actually buys is 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 is documented to catch. Say that, and say the rest is not closed. No sound fix exists at equal privilege: any marker distinguishing "should have a baseline" from "genuinely legacy" must live where the same session cannot reach, and nowhere does. The direction is privilege separation on the state root, which also covers the events channel's identical exposure — filed as #571 rather than bolted on here. --- CHANGELOG.md | 15 +++++++++++---- src/bmad_loop/runs.py | 21 ++++++++++++++++++--- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d70ed58f..1b8fa99e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -309,7 +309,7 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Fixed -- **A session can no longer silence `resume`'s config-change warning (#498).** The host-exec +- **`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 @@ -318,9 +318,16 @@ whose seams had diverged enough that several ports needed a different fix, and t `delete`/`archive`/`clean` lifecycle. The auto-sweep gate is unchanged — it always compared against an in-memory baseline no session can reach. Runs paused under an older version keep their old baseline for this release: `resume` falls back to `state.json` when the run has no - file out of tree, warns as before, and migrates it. The field is no longer written; it is not - a hard boundary against a `--dangerously-skip-permissions` session, which can name the - directory. + file out of tree, warns as before, and migrates it. The field is no longer written. + + **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/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 0121e8ad..39830363 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -343,9 +343,24 @@ def config_digest_path_for(project: Path, run_id: str) -> Path: 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. A pin a session can - edit is not a pin. The same reasoning the events channel moved on (#494), and - with the same honest limit — see :func:`state_root`.""" + 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, so a session that + goes looking can delete or truncate this file and the reader below will answer + "no baseline" exactly as it would for a run that never had one. That is + undetectable from here: any marker saying "this run *should* have a baseline" + 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 From 1436ec472e658fc144b9312876e83ba885c025a8 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 21:13:31 -0700 Subject: [PATCH 3/6] fix(runs): establish the digest file's shape before reading it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write was hardened against a planted link (follow_symlinks=False); the read was left on Path.read_text, which is the asymmetry CodeRabbit caught. The file's parent is the BMAD_LOOP_EVENTS_DIR the engine hands the session, so degrading on a hostile path is not enough when the read itself is the weapon: - O_NONBLOCK + an S_ISREG check on the DESCRIPTOR (fstat cannot be raced). Opening a FIFO for reading otherwise blocks indefinitely, and resume is a foreground command a human is waiting on — a planted FIFO wedges the terminal rather than costing a warning. - O_NOFOLLOW, so the name is read rather than wherever it points. - A 256-byte cap. A link to /dev/zero otherwise reads to MemoryError, which is not the OSError this function promises never to leak. Mirrors tui.launch._read_ctl_window, which solved the same hazard for the ctl-window record; one idiom, stated as such. It does NOT collapse empty to None the way that twin does — here "" and None are different answers. This does not make the baseline tamper-proof (a session can still delete it; #571 carries that). It stops a tampered path from hanging or exhausting the orchestrator, which is a separate and fixable harm. Every flag ablated singly: dropping O_NONBLOCK, O_NOFOLLOW, S_ISREG or the cap reddens its own row, and the absent-vs-empty control still passes. --- src/bmad_loop/runs.py | 59 ++++++++++++++++++++++++++++-- tests/test_runs.py | 85 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 3 deletions(-) diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 39830363..c5624532 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 @@ -42,6 +43,11 @@ # `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" @@ -382,10 +388,57 @@ def read_trusted_config_digest(project: Path, run_id: str) -> str | None: 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.""" + 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: - return config_digest_path_for(project, run_id).read_text(encoding="utf-8").strip() - except (StateRootError, OSError, RuntimeError, UnicodeDecodeError): + 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 diff --git a/tests/test_runs.py b/tests/test_runs.py index 5e340af8..eb526b5f 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -1033,6 +1033,91 @@ def test_trusted_config_digest_read_degrades_where_the_write_raises( 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.""" + 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) + + +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, not just + that the call returned — a reader that slurped the whole file and then + truncated would satisfy "returns quickly" on a 1 MB file and still blow up on + /dev/zero. + + ABLATION: drop `_MAX_DIGEST_BYTES` from the `os.read` and the length assert + fails.""" + 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)) + + got = runs.read_trusted_config_digest(project, "r1") + + assert got is not None + assert len(got) == 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 From 33dc0fd4367741ad3c8b4da5a3603adba7fa0672 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 21:34:00 -0700 Subject: [PATCH 4/6] fix(resume): keep the config-change pin when the project is renamed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The state root is keyed by the project's RESOLVED PATH (`runs.project_tag`), so moving or renaming a project keys its runs somewhere new and orphans the subtree holding `config-digest` — FEATURES.md already documented the GC half of that fact. Stamping the baseline only out of tree therefore made a rename silently retire the pin: the reader answers `None`, the in-tree field was no longer written, and an empty fallback means "no prior pin, no warning". A host-exec config change across a move was accepted in silence, and every later resume stayed quiet too. Pre-#498 the pin lived in `state.json` and travelled with the run dir, so this was a regression this branch introduced, on a documented operator action with no session tampering involved. The same applies to a `BMAD_LOOP_STATE_DIR` that changes between launch and resume. Both launch sites and the resume re-stamp now write the in-tree copy alongside the out-of-tree file. Precedence is unchanged and is what keeps the in-tree copy not worth tampering with: `_resume_paused_run` reads the file first and consults `state.trusted_config_digest` only when the file is absent (`None`), so #498's attack — rewrite `policy.toml`, blank the field — still warns. The `None` vs `""` split is untouched: a truncated file is a real "no baseline" and does not reopen the tree. Tampering that deletes the out-of-tree file remains out of reach at equal privilege and stays with #571; this fixes only the honest-absence cases. Ablations, run singly: dropping the resume re-stamp, the `compose_run` pass-through, or the `compose_sweep` stamp each redden their own test. Controls both ways — making the in-tree copy authoritative, or letting an empty field override a populated file, each redden the #498 attack test. --- CHANGELOG.md | 12 ++++++-- docs/FEATURES.md | 2 +- src/bmad_loop/cli.py | 18 ++++++++--- src/bmad_loop/model.py | 40 +++++++++++++++---------- src/bmad_loop/runs.py | 41 +++++++++++++------------ src/bmad_loop/runsetup.py | 24 ++++++++++----- tests/test_cli.py | 63 ++++++++++++++++++++++++++++++++++++--- tests/test_runs.py | 19 ++++++++---- 8 files changed, 159 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b8fa99e..e0fb9e98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -316,9 +316,15 @@ whose seams had diverged enough that several ports needed a different fix, and t 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. Runs paused under an older version keep - their old baseline for this release: `resume` falls back to `state.json` when the run has no - file out of tree, warns as before, and migrates it. The field is no longer written. + 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. **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 diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 7a418338..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, 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. +- 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 d64868ca..df61ec30 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -2200,11 +2200,21 @@ def _resume_paused_run(project: Path, run_dir: Path) -> int: # # 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 fallback never fires for it again. The legacy field is left as it - # was found rather than cleared — it is that run's honest record of what it - # launched under, nothing reads it once the file exists, and erasing persisted - # data to tidy up is not this change's business. + # 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 # a prior stopped-gracefully run so the re-armed engine does not consume it at the diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index 3d3f8440..e98e6796 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -467,25 +467,33 @@ class RunState: project: str started_at: str policy_snapshot: dict[str, Any] = field(default_factory=dict) - # LEGACY, read-only, one release (#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). It used - # to be stamped here at launch and re-stamped on resume, beside policy_snapshot; - # it is now stamped out of the tree instead (`runs.write_trusted_config_digest`), + # 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. + # 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). # - # Nothing writes it any more, so it is "" on every run this code starts. It is - # still PARSED and still round-tripped by to_dict, for the runs that were paused - # under the old code and are resumed under this one: their baseline is here and - # nowhere else, and `_resume_paused_run` falls back to it when the state root - # holds no file for the run. Dropping it outright would turn "this run has a - # pin" into "this run has none" for exactly those runs, which is the empty-means - # -legacy contract read backwards. Empty still 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. + # 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 c5624532..e0db3425 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -359,14 +359,15 @@ def config_digest_path_for(project: Path, run_id: str) -> Path: 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, so a session that - goes looking can delete or truncate this file and the reader below will answer - "no baseline" exactly as it would for a run that never had one. That is - undetectable from here: any marker saying "this run *should* have a baseline" - 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.""" + 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 @@ -374,15 +375,17 @@ 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 legacy field", not "no pin" — the two are different + ``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``). A run started before #498 has no file here and - its baseline is still in ``state.json``; an *empty* file, by contrast, is a - real answer of "no baseline" and comes back as ``""``. + ``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 ``""``. 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 legacy fallback. The write half raises — see + 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 @@ -446,12 +449,12 @@ 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 one outcome that cannot be detected later: the next resume reads no - file, falls back to a legacy field that is empty for any run this new code - started, and quietly declines to warn. 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. + 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` diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index f3d775fb..a399ede6 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -736,6 +736,7 @@ def build_run_state( max_stories: int | None, stories_on: bool, spec_folder: str, + trusted_config_digest: str, ) -> RunState: """Assemble the launch-time :class:`RunState` for a fresh run. @@ -743,10 +744,11 @@ 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). - No ``trusted_config_digest``: since #498 the host-exec baseline is stamped out - of the project tree by :func:`compose_run`, not carried on the state (see - ``RunState.trusted_config_digest``, kept one release as a read-only legacy - fallback).""" + ``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), @@ -757,6 +759,7 @@ def build_run_state( 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, ) @@ -805,8 +808,11 @@ def compose_run( 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 - rather than onto the :class:`RunState` (#498), so the one baseline ``resume`` - warns off is not sitting in the tree the driven sessions write to. + (#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 @@ -824,6 +830,7 @@ def compose_run( max_stories=max_stories, stories_on=stories_on, spec_folder=spec_folder, + 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: @@ -886,8 +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, not on the - :class:`RunState` — see :func:`compose_run`. + ``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 @@ -903,6 +910,7 @@ def compose_sweep( started_at=time.strftime("%Y-%m-%dT%H:%M:%S"), policy_snapshot=policy.to_dict(), 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. diff --git a/tests/test_cli.py b/tests/test_cli.py index 53d1b704..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 @@ -3509,10 +3510,9 @@ def test_resume_migrates_a_pre_498_baseline_out_of_state_json(project, monkeypat "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 legacy field = compare against the legacy - field, warn, and migrate. The second half is what bounds the fallback to one - release — after this resume the run has a file out of tree and never consults - the field again. + 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).""" @@ -3533,6 +3533,54 @@ def test_resume_migrates_a_pre_498_baseline_out_of_state_json(project, monkeypat 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( project, monkeypatch, capsys ): @@ -7752,6 +7800,10 @@ def run(self): 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 def test_run_pins_the_profile_bytes_it_launches(project, monkeypatch): @@ -7826,6 +7878,9 @@ def run(self): 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 def test_resume_pins_the_profile_bytes_it_launches(project, monkeypatch): diff --git a/tests/test_runs.py b/tests/test_runs.py index eb526b5f..4a4794e2 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -962,13 +962,22 @@ def test_state_dir_for_raises_when_the_project_cannot_be_canonicalized(tmp_path, def test_config_digest_is_stamped_under_the_state_root_not_in_the_project(tmp_path): - """#498's whole point: the baseline `resume` warns off leaves the tree the - driven sessions can write to. + """#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 the project also kept a copy, and a copy - inside `.bmad-loop/` is exactly the thing a session edits to silence the - warning.""" + 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) From e2a6ee943d92ca2f6bf509d3b088aa62f416f3b8 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 21:45:19 -0700 Subject: [PATCH 5/6] docs(resume): record the stale-pin limit a returning project hits (#572) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A project that moves away from a path and later returns finds its old state-root subtree still there — unsweepable in between, as FEATURES.md already says — so the reader prefers that older pin over the blessing that travelled in state.json. One resume answers off it and re-stamps, healing the key. Documented rather than fixed, on the same boundary as #571: preferring the fresher-looking in-tree copy would hand every session the silencing #498 closed, and arbitrating by sequence number needs a counterpart no session can forge, which at equal privilege does not exist. Filed as #572 with the evidence, including that it reproduces identically at 1436ec4 and so is not a consequence of the dual write. --- CHANGELOG.md | 5 ++++- src/bmad_loop/runs.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0fb9e98..56bf7a8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -324,7 +324,10 @@ whose seams had diverged enough that several ports needed a different fix, and t 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. + 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 diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index e0db3425..1fd10a60 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -383,6 +383,18 @@ def read_trusted_config_digest(project: Path, run_id: str) -> str | None: (: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 From 3454bee6763262bab988406309fdf42eeb1ecccc Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 22:10:39 -0700 Subject: [PATCH 6/6] test(runs): grade the read bound at the read, and the FIFO guard when fed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps CodeRabbit found in the hardened reader's own tests. `..._is_bounded` asserted only the length of what came back, while its docstring claimed it pinned the bound on the READ and named the exact reader that would slip through — one that slurps the file and truncates at the end. That reader returns _MAX_DIGEST_BYTES too, so it passed: confirmed by ablation, rc=0. The requested counts are now captured at `os.read` and totalled, which fails that row at `1114112 <= 256`; the returned-length assert stays, since it is what catches a cap honoured on the read and lost afterwards. The FIFO test is NOT vacuous for `S_ISREG`, contrary to the finding — ablating the check reddens it, because an idle FIFO reads EOF and the reader answers "" where it owes None. But an idle FIFO is the weaker half. A twin now holds one open with a writer feeding it, which is the attack: without the descriptor check the reader would return whatever the session piped in and treat it as this run's baseline — feed the digest of the config just installed and resume is satisfied. Neither outcome is a hang, so the sibling's alarm would never have caught it. Opened O_RDWR because a write-only open on a FIFO blocks until a reader arrives. Ablated singly: slurp-then-truncate and an uncapped read each redden the bounded test; dropping S_ISREG reddens both FIFO tests. --- tests/test_runs.py | 71 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 62 insertions(+), 9 deletions(-) diff --git a/tests/test_runs.py b/tests/test_runs.py index 4a4794e2..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 @@ -1062,7 +1063,10 @@ def test_read_trusted_config_digest_refuses_a_planted_fifo_instead_of_hanging(tm target. POSIX-only, which this test already is. ABLATION: restore `Path.read_text` in the reader, or drop `O_NONBLOCK`, and - the alarm fires.""" + 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" @@ -1083,29 +1087,78 @@ def _blew_up(signum, frame): 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, not just - that the call returned — a reader that slurped the whole file and then - truncated would satisfy "returns quickly" on a 1 MB file and still blow up on - /dev/zero. - - ABLATION: drop `_MAX_DIGEST_BYTES` from the `os.read` and the length assert - fails.""" + 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)) - got = runs.read_trusted_config_digest(project, "r1") + 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")