From bc784e39d19d96a24d05ef26a4b3e44c0fc42253 Mon Sep 17 00:00:00 2001 From: t Date: Fri, 14 Aug 2026 21:26:04 -0700 Subject: [PATCH 01/11] fix: stop an auto-sweep child from killing or silencing its parent (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_maybe_auto_sweep` promises in its own docstring, and in docs/FEATURES.md, that a failed child sweep never interrupts the parent run. The guard was written over `Exception`, but the contract is stated over "a paused or failed child" — and those two sets differ in both directions. `SystemExit` is a failed child the guard did not cover. `runsetup.make_adapters` raises it at five sites (unresolvable profile, unknown adapter kind, kind that fails to load, failed construction, unusable multiplexer). It is a `BaseException`, so it was missed here, by every arm of `_run_inner`, and by `cli.main`: it unwound through the `finally` — persisting the already-burned trigger latch — and ended the process at exit 1 with the parent left `finished=False`, `crashed=False`, no `run-complete`, and an orphaned agent session. The reachable site is the unusable-mux refusal: `mux_usable` bottoms out in a bare `shutil.which`, live and uncached on every call, so a child sweep can hit it in a run that launched fine. `RunStopped` is the reverse — an `Exception` that is not a failure. The child's hard-stop arm re-raises it precisely so the owner records the stop; eating it as `sweep-auto-failed` let the parent run on to `finished = True`, and left the run unstoppable: the signal handler latches `self._stopping = True` before raising, and its first line returns early when that latch is set, so every later SIGTERM was ignored. Fixed in the one clause: `except RunStopped: raise` above `except (Exception, SystemExit)`. Deliberately not `BaseException` — `KeyboardInterrupt` must keep escaping, because `_run_inner`'s nested-child re-raise depends on it reaching the owner. Three tests, each ablated singly against a `cp` backup of the fixed file (restored byte-identical by sha256 between runs), all rc == 1 — no rc == 4 collection errors: A1 drop `SystemExit` from the tuple -> FAILED test_auto_sweep_system_exit_does_not_kill_the_parent (1 failed, 3 passed) A2 delete the `except RunStopped: raise` arm -> FAILED test_auto_sweep_run_stopped_stops_the_parent (1 failed, 3 passed) A3 INVERSE (the guard is an absence): widen to `except (BaseException)` -> FAILED test_auto_sweep_keyboard_interrupt_still_propagates (1 failed, 3 passed) The three reddened sets are disjoint singletons, which is what proves each test pins its own arm rather than the clause as a whole. Full gates: pytest -n logical 5465 passed / 44 skipped / 5 xfailed, pyright 0 errors, trunk fmt clean, trunk check --all --no-fix clean. --- CHANGELOG.md | 14 +++++ src/bmad_loop/engine.py | 30 ++++++++++- tests/test_engine.py | 111 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17361651..4ae01be9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,20 @@ breaking changes may land in a minor release. ### Fixed +- **A failing auto-sweep can no longer kill its parent run, and a stop during one is no longer + swallowed (#501).** The child-sweep guard promised never to interrupt the parent, but was written + over `Exception`, and that set differs from "a paused or failed child" in both directions. A + `SystemExit` — what `runsetup.make_adapters` raises for an unusable multiplexer, an unresolvable + profile or an adapter kind that fails to load — is a `BaseException`, so it escaped the guard, + every arm of the engine's run handler, and `main()`, ending the process at exit 1 with the parent + left neither `finished` nor `crashed`, no `run-complete`, and an orphaned agent session. The + unusable-multiplexer gate re-probes live on every call, so a child could hit it in a run that + launched fine. In the other direction `RunStopped` _is_ an `Exception` but is not a failure: the + child re-raises it so the owner records the stop, and eating it as `sweep-auto-failed` let the + parent run on to `finished` — and left it unstoppable, since the signal handler latches + `_stopping` before raising, so every later SIGTERM returned at that latch. `KeyboardInterrupt` + deliberately still escapes; the nested-child re-raise depends on it. + - **A failed write can no longer truncate the sprint board, a story spec, your CLI settings or your policy file (#379).** Seven writers read a file, merged into it, and wrote the whole thing back through a truncating `Path.write_*` — so a fault partway through (ENOSPC, EIO, a quota) published diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 1de7c5ba..58a4b527 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -5617,7 +5617,31 @@ def _escalate(self, task: StoryTask, reason: str) -> None: def _maybe_auto_sweep(self, kind: str, trigger: str) -> None: """Run a child deferred-work sweep when policy [sweep].auto matches. The child is its own resumable run; a paused or failed child is - journaled + notified but never interrupts this run.""" + journaled + notified but never interrupts this run. + + That contract is stated over "a paused or failed child" while the guard + below was written over ``Exception``, and the two sets differ in BOTH + directions — which is why the arms are shaped the way they are: + + - ``SystemExit`` is a failed child the guard did not cover. It is a + ``BaseException``, so it was missed here, by every arm of + :meth:`_run_inner`, and by ``cli.main`` — it unwound through the + ``finally`` (persisting this trigger's already-burned latch) and + killed the process at exit 1, leaving the parent neither ``finished`` + nor ``crashed``, with no ``run-complete`` and an orphaned session. + ``runsetup.make_adapters`` raises it, reachably: the unusable-mux + refusal sits behind ``mux_usable``, a live uncached ``shutil.which`` + re-run on every call. + - ``RunStopped`` is the reverse — an ``Exception`` that is not a failure + at all. The child's hard-stop arm re-raises it precisely so the owner + records the stop; swallowing it as ``sweep-auto-failed`` let the + parent run on to ``finished`` AND left it unstoppable, since the + signal handler latches ``_stopping`` before raising and every later + SIGTERM then returns early. + + Deliberately NOT ``BaseException``: ``KeyboardInterrupt`` must keep + escaping, because the nested-child re-raise in :meth:`_run_inner` + depends on it reaching the owner.""" if self.policy.sweep.auto != kind or self.sweep_factory is None: return if trigger in self.state.sweeps_triggered: @@ -5646,7 +5670,9 @@ def _maybe_auto_sweep(self, kind: str, trigger: str) -> None: try: self.sweep_factory(trigger) self.journal.append("sweep-auto-finished", trigger=trigger) - except Exception as e: # child must never break the parent + except RunStopped: + raise # a stop is not a failed child — let the owner record it + except (Exception, SystemExit) as e: # child must never break the parent self.journal.append("sweep-auto-failed", trigger=trigger, error=str(e)) gates.notify(self.policy, self.run_dir, "auto sweep failed", f"{trigger}: {e}") diff --git a/tests/test_engine.py b/tests/test_engine.py index d7018ac8..204204a4 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -7616,6 +7616,117 @@ def exploding(trigger): assert "sweep-auto-failed" in journal and "child sweep blew up" in journal +def test_auto_sweep_system_exit_does_not_kill_the_parent(project): + """#501: a child sweep that dies on `SystemExit` is a failed child like any + other, and the "never interrupts this run" contract has to cover it. It is a + `BaseException`, so a guard written over `Exception` missed it here, in every + arm of `_run_inner`, and in `cli.main`: it unwound to process exit 1 with the + parent left neither `finished` nor `crashed`, no `run-complete`, and an + orphaned agent session. + + Not a hypothetical shape — `runsetup.make_adapters` raises exactly this for + an unresolvable profile, an unknown/unloadable adapter kind, a failed adapter + construction, and an unusable multiplexer; that last gate re-probes live + (`mux_usable` bottoms out in a bare `shutil.which`) on every call, so a child + sweep can hit it in a parent run that launched fine. + + Ablation: drop `SystemExit` from the `except (Exception, SystemExit)` tuple + in `_maybe_auto_sweep` and this test fails alone — the SystemExit escapes + `engine.run()` instead of being journaled.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) + + def exiting(trigger): + raise SystemExit("error: multiplexer backend is not usable on this host") + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + policy=policy, + sweep_factory=exiting, + ) + summary = engine.run() + + assert summary.done == 1 and not summary.paused + assert engine.state.finished + journal = (engine.run_dir / "journal.jsonl").read_text() + assert "sweep-auto-failed" in journal and "not usable on this host" in journal + assert "run-complete" in journal + + +def test_auto_sweep_run_stopped_stops_the_parent(project, monkeypatch): + """#501, the mirror image: `RunStopped` subclasses `Exception` but is not a + failed child at all. The child's hard-stop arm re-raises it *so the owner + records the stop*, so swallowing it as `sweep-auto-failed` was doubly wrong — + the parent ran on to `finished`, and it became unstoppable, because the + signal handler latches `_stopping = True` before raising and every later + SIGTERM then returns at that latch. + + Ablation: delete the `except RunStopped: raise` arm from `_maybe_auto_sweep` + and this test fails alone — the stop is swallowed and the parent finishes.""" + killed = [] + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: killed.append(rid)) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) + + def stopping(trigger): + raise RunStopped() # hard (graceful=False), as the child's stop arm re-raises + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + policy=policy, + sweep_factory=stopping, + ) + engine.run() + + saved = load_state(engine.run_dir) + assert saved.stopped is True + assert not saved.finished # the whole point: a stop must not read as a finish + assert killed == ["test-run"] + journal = (engine.run_dir / "journal.jsonl").read_text() + assert "run-stop" in journal + assert "sweep-auto-failed" not in journal # a stop is not a failure + assert "run-complete" not in journal + + +def test_auto_sweep_keyboard_interrupt_still_propagates(project, monkeypatch): + """#501, the control on the fix's shape: the two arms above must never be + widened to a bare `BaseException`. `KeyboardInterrupt` has to keep escaping + `_maybe_auto_sweep`, because `_run_inner`'s own KeyboardInterrupt arm is what + records the controlled stop — and, for a nested child, re-raises it for the + owning engine. + + INVERSE ablation (the guard here is an *absence* — deleting code cannot + reproduce the bug): widen the clause to `except (BaseException) as e:` and + this test fails alone — the interrupt is swallowed as `sweep-auto-failed` + and the parent finishes instead of stopping.""" + killed = [] + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: killed.append(rid)) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) + + def interrupting(trigger): + raise KeyboardInterrupt() + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + policy=policy, + sweep_factory=interrupting, + ) + engine.run() + + saved = load_state(engine.run_dir) + assert saved.stopped is True + assert not saved.finished + assert killed == ["test-run"] + entries = engine.journal.entries() + stops = [e for e in entries if e["kind"] == "run-stop"] + assert stops and stops[0]["reason"] == "KeyboardInterrupt" + assert not [e for e in entries if e["kind"] == "sweep-auto-failed"] + + def test_auto_sweep_config_digest_refusal_journals_and_spares_the_parent(project): """#461 point 4, end to end through the REAL factory rather than a stand-in exploder: the config-integrity gate's raise has to land on the same From 9c7a28438236773b3e8a1c001a36b39ecd1782fd Mon Sep 17 00:00:00 2001 From: t Date: Fri, 14 Aug 2026 21:35:54 -0700 Subject: [PATCH 02/11] fix: unwind a run whose composition aborts standing up the adapters (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `compose_run` and `compose_sweep` both publish a run before they can know the run will start: `save_state` writes `state.json`, then the out-of-tree config digest and the pid land, and only then is `make_adapters` called. That call raises `SystemExit` at five sites (`runsetup.py:452, 463, 478, 512, 531` — unresolvable profile, unknown adapter kind, a kind that fails to load, a construction failure, an unusable multiplexer), so an escape there left a run dir carrying `finished=False`, `crashed=False` and no `run-start`. Nothing reconciled that shape. `runs.reconcile_stale_worktrees` only visits `is_finished` runs, so the dir lingered — listing, and looking resumable, as an empty run. The unusable-multiplexer site is the reachable one: `mux_usable` bottoms out in a live `shutil.which` on every call, so an auto-triggered child sweep can hit it inside a parent that launched fine. Composition is now atomic from `save_state` onward in both functions: the remainder is wrapped, and on failure `_unwind_composition` removes the run before re-raising. Copies the repo's own idiom (`platform_util`'s two atomic writers, `runs.archive_run`) — `except BaseException: with suppress(...): ; raise`. `BaseException` is not a widening for its own sake: `SystemExit` is the whole failure this exists for. Three decisions worth naming, all recorded in `_unwind_composition`'s docstring: * `runs.delete_run`, not a bare `rmtree` — it drops the out-of-tree state dir too (`_discard_state_dir`), which is what covers the config-digest stamp written between the state and the pid. * `force=False`. `force` is documented as the *operator's* explicit override and there is no operator on an automatic unwind. It would skip the one guard protecting the one state where a run dir is load-bearing: an untagged live `bmad-loop-` session, whose only ownership proof it is. A run id is caller-supplied, so such a session can exist at the id this launch just claimed and cannot be this launch's — none was ever spawned. When the guard fires the cost is exactly the pre-fix behavior, a stranded dir; `force=True` trades that bounded cost for an unbounded one. * `suppress(Exception)`, so a cleanup failure can never replace the launch failure in flight. The enumerable set is `LiveSessionError` / `OSError` / `RuntimeError`, but `delete_run` reaches the multiplexer registry through `live_session_may_be_ours`, an extension point an out-of-tree backend can make raise anything — an enumerated tuple is a list a third party falsifies. Not `BaseException`: a `KeyboardInterrupt` during cleanup is the operator's. `make_adapters`' comment at :468-469, which recorded the stranded run dir as an accepted consequence, is rewritten rather than deleted — the surrounding `except ImportError` is narrow for a separate reason that still holds, and the comment now says so explicitly. Two tests, on `tmp_path` for the reason the neighbouring `pinned` fixture states (the composers touch only `.bmad-loop/runs/` and the conftest-redirected state root). The injected `make_adapters` records that the run dir, its `state.json` and the state dir all exist at the moment it is called, so the after-assertions grade a *removal* — "is it gone" passes just as happily for a dir never written. Ablations, each against a `cp` backup of the FIXED file, restored byte-identical by sha256 between runs, all rc == 1: A1 delete compose_run's `_unwind_composition` call -> FAILED test_compose_run_unwinds_the_run_when_the_adapters_abort (1 failed, 25 passed) A2 delete compose_sweep's `_unwind_composition` call -> FAILED test_compose_sweep_unwinds_the_run_when_the_adapters_abort (1 failed, 25 passed) A3 swap `delete_run` for a bare `shutil.rmtree` (drops the state-dir half) -> BOTH failed, at the state-dir assertion rather than the run-dir one (2 failed, 24 passed) A1 and A2 redden disjoint singletons, which is what proves each test pins its own composer rather than coasting on the other's wrapper; A3 grades the `delete_run`-not-`rmtree` choice on its own axis. Full gates: pytest -n logical 5467 passed / 44 skipped / 5 xfailed, pyright 0 errors, trunk fmt clean, trunk check --all --no-fix clean (256 files). --- CHANGELOG.md | 13 +++ src/bmad_loop/runsetup.py | 208 +++++++++++++++++++++++++------------- tests/test_runsetup.py | 135 ++++++++++++++++++++++++- 3 files changed, 284 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ae01be9..bf57decd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,19 @@ breaking changes may land in a minor release. ### Fixed +- **A launch that aborts while standing up its adapters no longer strands an empty run (#501).** + Both composers published the run directory, its `state.json` and the out-of-tree config-digest + stamp before calling `make_adapters` — which raises `SystemExit` from five sites (an unresolvable + profile, an unknown adapter kind, a kind that fails to load, a construction failure, an unusable + multiplexer). An escape there left a run carrying `finished=False`, `crashed=False` and no + `run-start`, and nothing reconciled it: the stale-worktree sweep only visits finished runs, so it + lingered in `bmad-loop list` looking resumable. Composition is now atomic from the first published + artifact onward — on any escape the run dir and its out-of-tree state dir are both removed and the + original exception is re-raised unchanged. The removal is best-effort and keeps the live-session + guard (no `force`), so on the one state where a run dir is load-bearing — an untagged live agent + session, for which it is the only ownership proof — the dir is left alone rather than leaking the + session. + - **A failing auto-sweep can no longer kill its parent run, and a stop during one is no longer swallowed (#501).** The child-sweep guard promised never to interrupt the parent, but was written over `Exception`, and that set differs from "a paused or failed child" in both directions. A diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index b7b60c2c..371f68de 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -34,6 +34,7 @@ import json import sys import time +from contextlib import suppress from dataclasses import dataclass from pathlib import Path from typing import TYPE_CHECKING, Protocol @@ -465,8 +466,13 @@ def make_adapters( # dependency they pull in — are first imported, and it is deliberately # never invoked by `validate` or `bmad-loop adapters` (both stay free # of heavy imports), so a thunk that raises has had no earlier gate. - # By here `compose_run` has already written the run state and pid, so - # an escaping ImportError strands a run directory behind a traceback. + # By here `compose_run` has already written the run state and pid. An + # escaping ImportError used to strand that run directory behind a + # traceback, recorded as an accepted consequence; it no longer does — + # both composers unwind the whole composition on any escape (see + # `_unwind_composition`), and this raise is one of the five SystemExits + # that path exists for. What that changes is the run dir, not the + # message: the narrowing below is a separate decision and still holds. # ImportError ONLY, on the same rule as `construct_error` below: a # missing dependency is a lazy loader's DECLARED failure, while # anything else is a bug in that package and must surface as itself @@ -782,6 +788,53 @@ class ComposedRun: journal: Journal +def _unwind_composition(project: Path, run_dir: Path) -> None: + """Remove the run a failed ``compose_*`` had already published, so a launch + that aborts partway leaves nothing behind. + + Reached from an ``except BaseException`` arm, because the failure it exists + for is a :class:`SystemExit`: :func:`make_adapters` raises one at five sites + (unresolvable profile, unknown adapter kind, a kind that fails to load, a + construction failure, an unusable multiplexer), every one of them *after* + ``save_state`` has published a run dir carrying ``finished=False`` / + ``crashed=False`` and no ``run-start``. Nothing reconciles that shape — + :func:`runs.reconcile_stale_worktrees` only touches ``is_finished`` runs — so + it lingers as a resumable-looking empty run. + + :func:`runs.delete_run` is the right primitive rather than a bare ``rmtree`` + because it also drops the run's out-of-tree state dir (``_discard_state_dir``), + which is what covers the config-digest stamp the composers write between the + state and the pid. + + ``force=False``, deliberately. ``force`` is documented there as the + *operator's* explicit override, and there is no operator here — this is an + automatic unwind. What it would skip is the one guard protecting the one state + where a run dir is load-bearing: an untagged live ``bmad-loop-`` session, + for which that directory is the only ownership proof a later prune can read. A + run id is caller-supplied (``--run-id``), so such a session can exist at the id + this launch just claimed, and it cannot be this launch's — no session was ever + spawned. Deleting the directory would leak it for the life of the machine. + When the guard does fire the cost is exactly the pre-fix behavior, a stranded + run dir, which is no worse than what this replaces; ``force=True`` would trade + that bounded cost for an unbounded one. + + Best-effort, and that is the whole point of the suppression: the caller is + already unwinding an exception the operator has to see, and a cleanup failure + replacing it is the one outcome that must not happen. The enumerable failures + are :class:`runs.LiveSessionError` (the guard refusing), ``OSError`` (the + removal, or ``project.resolve()`` on a path the OS cannot canonicalize) and + ``RuntimeError`` (how ``Path.resolve`` reports a symlink loop below 3.13 — see + ``runs._discard_state_dir``). It is not written as that tuple because + ``delete_run`` reaches the multiplexer registry through + :func:`runs.live_session_may_be_ours`, an extension point an out-of-tree + backend can make raise anything, so an enumerated list is one a third-party + backend falsifies. ``Exception`` and not ``BaseException``: a + ``KeyboardInterrupt`` arriving during the cleanup still belongs to the + operator.""" + with suppress(Exception): + runs.delete_run(project, run_dir) + + def compose_run( *, project: Path, @@ -834,39 +887,46 @@ def compose_run( 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( - "run-start", - run_id=run_id, - source=state.source, - adapter_dev=policy.adapter.resolved("dev").name, - adapter_review=policy.adapter.resolved("review").name, - ) - common = dict( - paths=paths, - policy=policy, - adapter=adapters["dev"], - review_adapter=adapters["review"], - run_dir=run_dir, - journal=journal, - state=state, - max_stories=max_stories, - epic_filter=epic_filter, - story_filter=story_filter, - sweep_factory=sweep_factory, - ) - # heterogeneous **kwargs: pyright unions the dict values; per-arg error is spurious - engine: Engine = ( - stories_engine_cls(**common, spec_folder=spec_folder) - if stories_on - else engine_cls(**common) # pyright: ignore[reportArgumentType] - ) + # Composition is atomic from the first published artifact onward: everything + # below either lands whole or is unwound (see :func:`_unwind_composition`, + # which also states why the arm is `BaseException` and not `Exception`). + try: + 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( + "run-start", + run_id=run_id, + source=state.source, + adapter_dev=policy.adapter.resolved("dev").name, + adapter_review=policy.adapter.resolved("review").name, + ) + common = dict( + paths=paths, + policy=policy, + adapter=adapters["dev"], + review_adapter=adapters["review"], + run_dir=run_dir, + journal=journal, + state=state, + max_stories=max_stories, + epic_filter=epic_filter, + story_filter=story_filter, + sweep_factory=sweep_factory, + ) + # heterogeneous **kwargs: pyright unions the dict values; per-arg error is spurious + engine: Engine = ( + stories_engine_cls(**common, spec_folder=spec_folder) + if stories_on + else engine_cls(**common) # pyright: ignore[reportArgumentType] + ) + except BaseException: + _unwind_composition(project, run_dir) + raise return ComposedRun(engine=engine, run_id=run_id, run_dir=run_dir, state=state, journal=journal) @@ -914,42 +974,48 @@ def compose_sweep( 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, - "decisions_only": decisions_only, - "max_bundles": max_bundles, - "repeat": repeat, - "max_cycles": max_cycles, - "trigger": trigger, - } - # Persist the sweep options atomically (tmp + os.replace), the way save_state - # writes state.json: a resume reads this back to rebuild the SweepEngine, so a - # crash mid-write must not leave a torn file the recovery path then chokes on. - sweep_path = run_dir / "sweep.json" - sweep_tmp = sweep_path.with_suffix(".json.tmp") - sweep_tmp.write_text(json.dumps(options, indent=2), encoding="utf-8") - atomic_replace(sweep_tmp, sweep_path) - adapters = make_adapters(project, run_dir, policy, profiles=profiles) - journal.append("run-start", run_id=run_id, run_type="sweep", trigger=trigger) - engine: Engine = sweep_engine_cls( - paths=paths, - policy=policy, - adapter=adapters["dev"], - review_adapter=adapters["review"], - triage_adapter=adapters["triage"], - run_dir=run_dir, - journal=journal, - state=state, - prompting=prompting, - decisions_only=decisions_only, - max_bundles=max_bundles, - repeat=repeat, - max_cycles=max_cycles, - ) + # Atomic from the first published artifact onward, exactly as in `compose_run` + # — same reason, and one more artifact to unwind (`sweep.json`). + try: + 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, + "decisions_only": decisions_only, + "max_bundles": max_bundles, + "repeat": repeat, + "max_cycles": max_cycles, + "trigger": trigger, + } + # Persist the sweep options atomically (tmp + os.replace), the way save_state + # writes state.json: a resume reads this back to rebuild the SweepEngine, so a + # crash mid-write must not leave a torn file the recovery path then chokes on. + sweep_path = run_dir / "sweep.json" + sweep_tmp = sweep_path.with_suffix(".json.tmp") + sweep_tmp.write_text(json.dumps(options, indent=2), encoding="utf-8") + atomic_replace(sweep_tmp, sweep_path) + adapters = make_adapters(project, run_dir, policy, profiles=profiles) + journal.append("run-start", run_id=run_id, run_type="sweep", trigger=trigger) + engine: Engine = sweep_engine_cls( + paths=paths, + policy=policy, + adapter=adapters["dev"], + review_adapter=adapters["review"], + triage_adapter=adapters["triage"], + run_dir=run_dir, + journal=journal, + state=state, + prompting=prompting, + decisions_only=decisions_only, + max_bundles=max_bundles, + repeat=repeat, + max_cycles=max_cycles, + ) + except BaseException: + _unwind_composition(project, run_dir) + raise return ComposedRun(engine=engine, run_id=run_id, run_dir=run_dir, state=state, journal=journal) diff --git a/tests/test_runsetup.py b/tests/test_runsetup.py index f3de22e2..d1749aaf 100644 --- a/tests/test_runsetup.py +++ b/tests/test_runsetup.py @@ -6,14 +6,22 @@ under-covers lets a mid-run rewrite through the auto-sweep gate; one that over-covers refuses every auto-sweep after a `[limits]` live-edit #189 documents as supported. Both halves are pinned below. + +The second concern here is composition atomicity: `compose_run` and +`compose_sweep` publish a run dir before they can know the run will start, and +`make_adapters` raises `SystemExit` from five sites after that point. The unwind +that keeps a failed launch from stranding a resumable-looking empty run is pinned +at the end of the file. """ import dataclasses +import types import pytest +from bmad_loop import bmadconfig from bmad_loop import policy as policy_mod -from bmad_loop import runsetup +from bmad_loop import runs, runsetup from bmad_loop.adapters.profile import ProfileError # A profile overlay carrying the whole launch surface the digest covers. It lives @@ -293,3 +301,128 @@ def test_digest_raises_on_an_unresolvable_profile(pinned): hashing a hole where the launch surface should be.""" with pytest.raises(ProfileError): _digest(pinned, '[adapter]\nname = "no-such-cli"\n') + + +# --------------------------------------------------------- composition unwind + +# A fixed, well-formed run id, so the assertions can name the two directories a +# composer publishes rather than fish them back out of the failed call. +RUN_ID = "20260812-101500-ab12" + +# The message `make_adapters` raises for an unusable multiplexer — the one of its +# five SystemExit sites that is reachable in a run that launched fine, since +# `mux_usable` bottoms out in a live `shutil.which` on every call. +BOOM = "error: multiplexer backend TmuxBackend is not usable on this host" + + +class _NeverBuilt: + """Engine stand-in for the composers' `*_cls` seams. + + Raises on construction rather than being a no-op: every test below fails at + `make_adapters`, which both composers call before they build an engine, so a + class that cannot be built is a second assertion that the failure landed where + the test says it did.""" + + def __init__(self, *args, **kwargs): + raise AssertionError("engine construction reached despite a failed make_adapters") + + +def _fake_paths(project): + """A hand-built ProjectPaths rather than `bmadconfig.load_paths`, which would + need a `_bmad/bmm/config.yaml` on disk. `paths` is read only when the engine is + constructed — after `make_adapters` — so nothing here ever dereferences it.""" + return bmadconfig.ProjectPaths( + project=project, + implementation_artifacts=project / "impl", + planning_artifacts=project / "plan", + ) + + +@pytest.fixture +def unwinding(tmp_path): + """A project plus a `make_adapters` that fails the way the real one does. + + `runsetup.make_adapters` raises `SystemExit` at five sites (an unresolvable + profile, an unknown adapter kind, a kind that fails to load, a construction + failure, an unusable multiplexer), and every one lands *after* the composer has + published the run dir, its `state.json` and the out-of-tree config-digest stamp. + The fake records that all three exist at the moment it is called, so the + assertions after the raise grade a *removal* — an "is it gone" assertion passes + just as happily for a run dir that was never written. + + `tmp_path` rather than the `project` sandbox, on the same reasoning the `pinned` + fixture states: the composers touch only `.bmad-loop/runs/` and the out-of-tree + state root (which conftest's `_isolate_state_root` already redirects), so the + sandbox's git repo and BMAD artifact dirs would buy nothing. The end-to-end + launch path is covered on the real sandbox in tests/test_cli.py.""" + published: dict[str, bool] = {} + + def make_adapters(project, run_dir, policy, *, profiles=None): + published["run_dir"] = run_dir.is_dir() + published["state"] = (run_dir / "state.json").is_file() + published["state_dir"] = runs.state_dir_for(tmp_path, RUN_ID).is_dir() + raise SystemExit(BOOM) + + return types.SimpleNamespace(project=tmp_path, make_adapters=make_adapters, published=published) + + +def _assert_unwound(probe): + """Composition published all three artifacts, then left none of them.""" + assert probe.published == {"run_dir": True, "state": True, "state_dir": True} + assert not runs.run_dir_for(probe.project, RUN_ID).exists() + assert not runs.state_dir_for(probe.project, RUN_ID).exists() + + +def test_compose_run_unwinds_the_run_when_the_adapters_abort(unwinding): + """A failed `make_adapters` must leave no run behind, and must still abort. + + Without the unwind the run dir survives carrying `state.json` with + `finished=False` and `crashed=False` and no `run-start` line — and nothing + reconciles that shape, since `runs.reconcile_stale_worktrees` only visits + `is_finished` runs. It lingers as a resumable-looking empty run. + + The `SystemExit` itself is re-raised unchanged: the cleanup is best-effort + precisely so it can never replace the failure the operator has to read.""" + with pytest.raises(SystemExit, match="not usable on this host"): + runsetup.compose_run( + project=unwinding.project, + paths=_fake_paths(unwinding.project), + policy=policy_mod.loads(""), + run_id=RUN_ID, + epic_filter=None, + story_filter=None, + max_stories=None, + stories_on=False, + spec_folder="", + sweep_factory=lambda _trigger: None, + make_adapters=unwinding.make_adapters, + engine_cls=_NeverBuilt, + stories_engine_cls=_NeverBuilt, + trusted_config_digest="deadbeef", + ) + _assert_unwound(unwinding) + + +def test_compose_sweep_unwinds_the_run_when_the_adapters_abort(unwinding): + """The sweep composer publishes the same artifacts (plus `sweep.json`) ahead of + the same `make_adapters` call, so it owns its own unwind — separately, since a + sweep is the run type most likely to hit the reachable SystemExit: an + auto-triggered child re-probes the multiplexer live in a parent that started + fine.""" + with pytest.raises(SystemExit, match="not usable on this host"): + runsetup.compose_sweep( + project=unwinding.project, + paths=_fake_paths(unwinding.project), + policy=policy_mod.loads(""), + run_id=RUN_ID, + prompting=False, + decisions_only=False, + max_bundles=None, + repeat=None, + max_cycles=None, + trigger="auto", + make_adapters=unwinding.make_adapters, + sweep_engine_cls=_NeverBuilt, + trusted_config_digest="deadbeef", + ) + _assert_unwound(unwinding) From 6d240c624cbad03441620ddbd724890242559a01 Mon Sep 17 00:00:00 2001 From: t Date: Fri, 14 Aug 2026 22:20:56 -0700 Subject: [PATCH 03/11] fix: record an auto-sweep trigger only once its child has started (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_maybe_auto_sweep` appended the trigger to `RunState.sweeps_triggered` and persisted it before anything had been attempted, so every way a child sweep could decline to launch still spent it. The sharpest was the tree check underneath: `verify.worktree_clean` fails closed on `GitError`, and `_run_git` reports a `subprocess.TimeoutExpired` as exactly that (verify.py:161-162), so a slow `git status` — not a dirty tree — permanently consumed the run's one sweep. Hoist the check above the record, and split its journal line with a `reason` so a git fault reads differently from real local changes. Then define a `SweepFactory` Protocol beside `RunPaused`/`RunStopped` (no cycle: engine imports neither cli, runsetup nor sweep) carrying a required keyword-only `started` thunk, thread it through `cli._start_sweep` as `on_started`, and fire it from `compose_sweep` once composition has succeeded. A plain return latches too — the thunk's only job is to classify a raise. `latch()` is idempotent and sets its in-memory flag before the write. The never-launched case journals a new `sweep-auto-not-started` and keeps its own `gates.notify`: the #461 refusal is a security event and must not go quiet just because it became recoverable. GROUND TRUTH, because the issue's framing is misleading and the retry it implies is nearly unreachable. Traced both call sites; the verdict is CONFIRMED — an un-latched trigger survives only in a crash window: * run-end fires at engine.py:852, `_loop` returns, and `state.finished = True` lands at :516. `cli._resume_paused_run` refuses a finished run outright (cli.py:2096-2098). There is no later ask. * per-epic fires at :5682 and the boundary is `state.current_epic != story.epic` — advanced at :5666-5667 before the gate's RunPaused, else at :860 on return. Nothing between can pause: `gates.notify` never raises (it swallows its own OSError), and `bus.emit` isolates both hook kinds (`_HookError` for declarative, `except Exception` for in-process) and returns vetoes rather than raising them — which `_epic_boundary` does not even resolve. So the window is a process death or a BaseException across a few statements. What the change buys the other 100% of the time is that `sweeps_triggered` is TRUE: it is durable state `bmad-loop diagnose` renders, and a record's value is that it does not claim work that never happened. Four stale comments asserting "burns the trigger for the life of the run" are corrected rather than deleted — a wrong config pin still refuses every trigger, it just no longer spends them. No code, comment, test or changelog line here promises a retry. The `on_started` boundary sits at composition-success, not at `save_state`, only because `9c7a284` made a failed composition unwind its own run dir; read `_unwind_composition` rather than trusting that. Its docstring now records the exception to the "nothing resumable" premise instead of asserting the premise flatly: the unwind is best-effort (`force=False` keeps the live-session guard, and the call sits under `suppress(Exception)`), so a refused unwind could strand a resumable child. Reaching it on the auto path needs a live `bmad-loop-` session at a freshly minted id — the factory calls `_start_sweep` with no `run_id`, and only `cmd_sweep` supplies one, which passes no `on_started`. That takes a `new_run_id` collision with a concurrently live run, whose `state.json` this composition's own `save_state` has already clobbered several statements earlier. Not a case the latch boundary should be answering. Self-review tightened one clause of that docstring's closing paragraph, which overreached. `on_started` fires as the LAST statement inside the composition block, so a raising latch unwinds the child too — and the at-most-once claim is stated over the flag, not over the unwind: `latch` sets the parent's in-memory flag BEFORE the write, so a second attempt is refused whether the unwind succeeded or was refused. What the unwind decides is only what that refusal costs — nothing left behind, or a composed and resumable child, which is the better of the two. Neither is a second launch. The old wording asserted "a child that left nothing behind" flatly, which the paragraph directly above it had already documented as not always true. Tests: all 16 injection sites updated (a shared `recording_factory` for the `calls.append` ones). The four refusal rows pass `started=lambda: pytest.fail("started before the gate")`, and the stubbed `_start_sweep` now RELAYS the thunk — without that the guard is decorative, since nothing could call it. First tests to reach `sweep-auto-skipped-dirty` at all, one per `reason`; the git one drives a real `TimeoutExpired` out of `subprocess.run` rather than stubbing `worktree_clean`, because the translation is the claim. `test_auto_sweep_no_refire_on_resume` gains a green-ablation record: it stays green with the latch gone, because `current_epic` also closes that boundary. Ablations, singly, against `cp` backups, all rc == 1, all three sources restored byte-identical afterwards (md5 verified). Baseline: 24 rows green. A1a pre-#501 append hoisted back above the check -> 10 rows A1b `latch()` in the except arm, unconditionally -> 5 rows A2 `if not clean:` deleted -> 1 (dirty) A3 `except verify.GitError` deleted -> 1 (git fault) A4 `latch()` body emptied -> 4 rows A5 plain-return `latch()` deleted -> 1 (never-signalled) A6 not-started `gates.notify` deleted -> 1 (notify wording) A7a `compose_sweep`'s `on_started()` deleted -> 1 (real boundary) A7b factory's `on_started=` dropped -> 2 rows A8 digest gate moved below `_start_sweep` -> 3, each naming "started before the gate" A9/A10/A11 re-run for the arms this touched (SystemExit dropped from the tuple; `except RunStopped: raise` deleted; INVERSE widen to BaseException) -> 1 each A12 not-started arm reuses "auto sweep failed" -> 1, on the title assert Several records I first wrote claimed "fails alone" and were wrong; they now name the observed sets. `uv run pytest -q -n logical`: 5473 passed, 44 skipped, 5 xfailed. `uv run pyright`: 0 errors. `trunk check --all --no-fix`: no issues. --- CHANGELOG.md | 20 +++ src/bmad_loop/cli.py | 40 +++-- src/bmad_loop/engine.py | 126 ++++++++++++-- src/bmad_loop/runsetup.py | 77 +++++++-- tests/test_cli.py | 127 ++++++++++---- tests/test_engine.py | 350 +++++++++++++++++++++++++++++++++++--- tests/test_runsetup.py | 2 +- 7 files changed, 641 insertions(+), 101 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf57decd..4b3c1797 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,26 @@ breaking changes may land in a minor release. `_stopping` before raising, so every later SIGTERM returned at that latch. `KeyboardInterrupt` deliberately still escapes; the nested-child re-raise depends on it. +- **A run's `sweeps_triggered` records only auto-sweeps that actually started (#501).** The trigger + was recorded — and the record persisted — before anything had been attempted, so every way a + child sweep could decline to launch still spent it: the `[verify]`/profile/plugin + config-integrity refusal, the worktree-isolation refusal, an unparseable `policy.toml`, an + unusable multiplexer. Worst of the set was the tree check, which fails closed on a git error and + reaches one on a plain `git status` timeout — so a slow filesystem, not a dirty tree, could + silently consume a run's one and only sweep. The check now runs ahead of the record and journals + a `reason` telling a git fault from real local changes, and the launcher signals the engine once + the child owns a published run dir, which is what the record now means. + + Not a retry mechanism, and it should not be read as one: both triggers close their own boundary + within a few statements — a `run-end` return lands on `finished`, which `resume` refuses, and the + per-epic boundary disappears as soon as `current_epic` advances. What changes is that + `bmad-loop diagnose` stops reporting sweeps that never happened, and that a crash in that narrow + window leaves the trigger recoverable rather than spent. A child that fails _after_ its run dir + exists still spends it — that run is resumable, so re-firing would duplicate it — and is + journaled `sweep-auto-failed` as before; the never-launched case is the new + `sweep-auto-not-started`, which keeps its own notification, because the loudest thing reaching it + is a config-integrity refusal. + - **A failed write can no longer truncate the sprint board, a story spec, your CLI settings or your policy file (#379).** Seven writers read a file, merged into it, and wrote the whole thing back through a truncating `Path.write_*` — so a fault partway through (ENOSPC, EIO, a quota) published diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index f2947f01..e664caa0 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -91,6 +91,8 @@ from .sweep import SweepEngine if TYPE_CHECKING: + from collections.abc import Callable + # Type-only: annotate the profile-lookup map without a module-level adapter # import (cli.py imports the adapter package lazily inside functions). from .adapters.profile import CLIProfile @@ -216,10 +218,10 @@ def _launch_profiles(pol, project: Path) -> dict[str, CLIProfile]: baseline a session cannot reach (`_sweep_factory` holds every later auto-sweep to it from MEMORY, unlike resume's disk-backed advisory), so a pin describing bytes the run did not launch makes those children refuse the config the parent - has been running all along. `engine._maybe_auto_sweep` appends the trigger to - `sweeps_triggered` BEFORE calling the factory and returns early on an - already-recorded one, so that refusal burns the trigger for the life of the run - — it is not retried, not even across a resume. + has been running all along — every trigger, for the life of the run. The + refusal no longer *burns* each trigger (#501: `sweeps_triggered` records only + a child that started), but that buys nothing here: a wrong pin refuses the next + trigger exactly as it refused the last. `cmd_sweep` deliberately keeps its fresh read: a human started it, and the pin it stamps gates no child. @@ -1920,6 +1922,7 @@ def _start_sweep( trigger: str, run_id: str | None = None, profiles=None, + on_started: Callable[[], None] | None = None, ) -> int: # The composition (run dir + state + pid + sweep.json + adapters + engine) # lives in runsetup; this stays compose -> render. SweepEngine and @@ -1930,6 +1933,10 @@ def _start_sweep( # both stamps the pin and builds the adapters, so neither re-reads # profiles/*.toml after the gate compared it. `cmd_sweep` passes None: a human # started that one, so a fresh read is the point. + # + # `on_started` is the auto-sweep parent's latch, likewise absent for + # `cmd_sweep`; `compose_sweep` fires it at the boundary where this child owns a + # published, resumable run dir. composed = runsetup.compose_sweep( project=project, paths=paths, @@ -1945,6 +1952,7 @@ def _start_sweep( sweep_engine_cls=SweepEngine, trusted_config_digest=_trusted_config_digest(pol, project, profiles=profiles), profiles=profiles, + on_started=on_started, ) print(f"sweep {composed.run_id} starting (attach: bmad-loop attach)") summary = composed.engine.run() @@ -1956,6 +1964,10 @@ def _sweep_factory(project: Path, paths: bmadconfig.ProjectPaths, trusted_digest """Child-sweep launcher injected into story-run engines. Auto-triggered sweeps are unattended: never prompt, never run decision bundles. + The returned callable implements :class:`engine.SweepFactory`: every refusal + below raises *before* the keyword-only ``started`` thunk can fire, which is + what leaves the parent run's trigger unspent for a child that never launched. + ``trusted_digest`` is the caller's launch-time :func:`runsetup.config_digest` — the integrity pin for the config this factory re-reads from disk below. Required, with no default: an omitted baseline would silently disable the @@ -1963,7 +1975,7 @@ def _sweep_factory(project: Path, paths: bmadconfig.ProjectPaths, trusted_digest re-read happens once and is frozen, so the gate validates the bytes the child actually launches from rather than a separate read of the same files.""" - def factory(trigger: str) -> None: + def factory(trigger: str, *, started: Callable[[], None]) -> None: pol = policy_mod.load(_policy_path(project)) # Read the agent-writable config EXACTLY ONCE, here, and run the child off # these two objects: `pol` and `profiles` are threaded through the gate @@ -1995,14 +2007,15 @@ def factory(trigger: str) -> None: " Run `bmad-loop sweep` yourself to proceed under the new config." ) # Raise rather than return the rc the other three sites return. By the time - # the engine calls this it has already latched the trigger and journaled - # `sweep-auto-trigger`, and it reads a plain return as success — so a bare - # decline would be recorded as `sweep-auto-finished`, which `engine.py` - # defines as "a clean completion from the parent's perspective": a child - # sweep that ran and finished when none was ever launched. Raising lands on - # the same `sweep-auto-failed` + notify path the `load` above already takes - # on an unparseable policy.toml, which is the same kind of event — the - # config on disk changed under a run that had already started. + # the engine calls this it has journaled `sweep-auto-trigger`, and it reads + # a plain return as a child that ran — latching the trigger on one whether + # or not `started` fired — so a bare decline would be recorded as + # `sweep-auto-finished`, which `engine.py` defines as "a clean completion + # from the parent's perspective": a child sweep that ran and finished when + # none was ever launched. Raising lands on the `sweep-auto-not-started` + + # notify path the `load` above already takes on an unparseable policy.toml, + # which is the same kind of event — the config on disk changed under a run + # that had already started. conflict = bmadconfig.worktree_isolation_conflict(paths, pol.scm.isolation) if conflict is not None: raise RuntimeError(conflict) @@ -2015,6 +2028,7 @@ def factory(trigger: str) -> None: max_bundles=None, trigger=trigger, profiles=profiles, + on_started=started, ) return factory diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 58a4b527..49a2c1db 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -20,7 +20,7 @@ import traceback from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING, Callable, NoReturn, Sequence +from typing import TYPE_CHECKING, Callable, NoReturn, Protocol, Sequence from . import deferredwork, devcontract, envvars, gates, operatoractions, verify from .adapters.base import CodingCLIAdapter, SessionResult, SessionSpec, SpecSnapshot @@ -114,6 +114,27 @@ def __init__(self, graceful: bool = False): self.graceful = graceful +class SweepFactory(Protocol): + """Call shape of the child-sweep launcher :meth:`Engine._maybe_auto_sweep` + drives — in the product, the inner function ``cli._sweep_factory`` returns, + injected so this module need not import ``cli``, ``runsetup`` or ``sweep``. + + Spelled as a Protocol rather than a ``Callable[[str], None]`` alias, mirroring + :class:`runsetup.MakeAdapters`, only because the keyword-only ``started`` + thunk is part of the contract and a positional callable alias cannot say so. + + ``started`` fires once the child sweep is composed and its run dir published + (:func:`runsetup.compose_sweep`), and is what lets the engine spend the run's + trigger on a child that actually started. It is **required, with no default**: + the engine reads "raised without calling it" as "no child was ever launched" + and leaves the trigger unspent, so a defaulted no-op would let an un-updated + implementation make that claim for a child that ran — the one direction that + costs a duplicate sweep. It is idempotent, so an implementation in doubt + should call it.""" + + def __call__(self, trigger: str, *, started: Callable[[], None]) -> None: ... + + @dataclass(frozen=True) class RunSummary: run_id: str @@ -338,7 +359,7 @@ def __init__( epic_filter: int | None = None, story_filter: str | None = None, review_adapter: CodingCLIAdapter | None = None, - sweep_factory: Callable[[str], None] | None = None, + sweep_factory: SweepFactory | None = None, registry: PluginRegistry | None = None, ): self.paths = paths @@ -5619,6 +5640,44 @@ def _maybe_auto_sweep(self, kind: str, trigger: str) -> None: The child is its own resumable run; a paused or failed child is journaled + notified but never interrupts this run. + ``state.sweeps_triggered`` spends the trigger only once a child sweep has + actually started. The ``started`` thunk handed to the factory + (:class:`SweepFactory`) fires at ``compose_sweep``'s success boundary, and + a plain return latches too — so the thunk's only job is to classify a + *raise*, and a raise that never reached that boundary leaves the trigger + unspent. + + What that is worth, stated precisely, because the intuitive answer is + wrong. It is NOT "the trigger gets retried later": at both call sites the + retry closes within a few statements of an un-latched return. + + - ``run-end`` fires from :meth:`_loop`, whose return lands directly on + ``self.state.finished = True`` in :meth:`_run_inner`; a finished run is + refused outright by ``cli._resume_paused_run``. There is no later ask. + - ``per-epic`` fires from :meth:`_epic_boundary`, and that boundary is + detected as ``state.current_epic != story.epic`` — a field that advances + within the same frame, either before the gate's ``RunPaused`` or on + return into ``_loop``. Nothing in between can pause the run: ``gates`` + notification never raises (it swallows its own ``OSError``), and + :meth:`_emit` isolates both hook kinds (``_HookError`` for a declarative + hook, ``except Exception`` for an in-process one) and *returns* vetoes + rather than raising them — which this caller does not even resolve. + + So the retry survives only in a crash window: a process death, or a + ``BaseException``, between the un-latched return and the state write that + closes the boundary. Do not widen that claim in a comment or a changelog. + What the ordering buys on every non-crashing run is that + ``sweeps_triggered`` is *true* — it is durable run state, rendered by + ``bmad-loop diagnose`` (``diagnostics.py``), and a record's whole value is + that it does not claim work that never happened. + + The worktree check therefore sits AHEAD of the latch rather than behind + it. ``verify.worktree_clean`` fails closed on a ``GitError`` and + ``_run_git`` maps a ``subprocess.TimeoutExpired`` onto exactly that, so a + `git status` that merely timed out used to spend the run's one and only + sweep trigger, permanently and silently. Both refusals carry a ``reason`` + so the journal separates a genuinely dirty tree from a git fault. + That contract is stated over "a paused or failed child" while the guard below was written over ``Exception``, and the two sets differ in BOTH directions — which is why the arms are shaped the way they are: @@ -5650,31 +5709,70 @@ def _maybe_auto_sweep(self, kind: str, trigger: str) -> None: # A pending graceful stop suppresses new child sweeps. Return (not # raise): at the run-end call site the story queue is already empty, # so finishing this run as `finished` is truthful — the finally clears - # the superseded control file. Crucially this precedes the append - # below, so the trigger stays UNrecorded and a later resume (after the - # request is cleared) can still fire the sweep it would have run. + # the superseded control file. The trigger stays unspent, which is + # honest bookkeeping rather than a deferral: see the docstring's + # crash-window verdict, which covers this return like the two below. self.journal.append("sweep-auto-suppressed", trigger=trigger) return - self.state.sweeps_triggered.append(trigger) - self._save() try: clean = verify.worktree_clean(self.workspace.root) - except verify.GitError: - clean = False + except verify.GitError as e: + # Fails closed — but ahead of the latch, because unlike the dirty-tree + # arm this one is transient-reachable: `_run_git` reports a + # `subprocess.TimeoutExpired` as GitError (verify.py), so a slow + # `git status` used to permanently spend this run's sweep trigger. + self.journal.append( + "sweep-auto-skipped-dirty", trigger=trigger, reason="git-error", error=str(e) + ) + return if not clean: # should not happen at these call sites (everything committed or # reset); refuse rather than sweep on top of stray changes - self.journal.append("sweep-auto-skipped-dirty", trigger=trigger) + self.journal.append("sweep-auto-skipped-dirty", trigger=trigger, reason="dirty") return + + latched = False + + def latch() -> None: + """Spend this run's trigger. Idempotent, so the factory may call it + without knowing whether the plain-return arm below already will. The + in-memory flag and the state list are set BEFORE the write: a `_save` + that fails must not re-open a trigger whose child is already composed + and resumable.""" + nonlocal latched + if latched: + return + latched = True + self.state.sweeps_triggered.append(trigger) + self._save() + self.journal.append("sweep-auto-trigger", trigger=trigger) try: - self.sweep_factory(trigger) - self.journal.append("sweep-auto-finished", trigger=trigger) + self.sweep_factory(trigger, started=latch) except RunStopped: raise # a stop is not a failed child — let the owner record it except (Exception, SystemExit) as e: # child must never break the parent - self.journal.append("sweep-auto-failed", trigger=trigger, error=str(e)) - gates.notify(self.policy, self.run_dir, "auto sweep failed", f"{trigger}: {e}") + if latched: + self.journal.append("sweep-auto-failed", trigger=trigger, error=str(e)) + gates.notify(self.policy, self.run_dir, "auto sweep failed", f"{trigger}: {e}") + else: + # The raise beat composition, so there is no child run dir and + # nothing to resume — recording the trigger would be a claim about + # work that does not exist. Still notified, and with its own + # wording rather than none: the loudest raise that lands here is + # the #461 config-integrity refusal, a security event that must not + # go quiet just because it stopped being permanent. + self.journal.append("sweep-auto-not-started", trigger=trigger, error=str(e)) + gates.notify( + self.policy, self.run_dir, "auto sweep did not start", f"{trigger}: {e}" + ) + else: + # A plain return is a child that ran, whether or not the factory + # bothered with the thunk — the thunk exists to classify raises. + # Outside the try on purpose: `latch` writes the PARENT's state, and a + # failure there is this run's, not a child failure to swallow. + latch() + self.journal.append("sweep-auto-finished", trigger=trigger) def _epic_boundary(self, finished_epic: int, next_epic: int) -> None: self.journal.append("epic-boundary", finished=finished_epic, next=next_epic) diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index 371f68de..7498b9fb 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -53,7 +53,7 @@ from .adapters.base import CodingCLIAdapter from .adapters.profile import CLIProfile - from .engine import Engine + from .engine import Engine, SweepFactory from .policy import Policy from .stories_engine import StoriesEngine from .sweep import SweepEngine @@ -93,10 +93,11 @@ def resolve_profiles(policy: Policy, project: Path) -> dict[str, CLIProfile]: file: a session that leaves a background writer flipping ``.bmad-loop/profiles/*.toml`` between a benign and a hostile copy needs only the digest's read to catch the benign one and the adapter's read to catch the - other. That race is cheap to retry — a lost round raises `sweep-auto-failed`, - which `_maybe_auto_sweep` swallows, and the next auto-sweep trigger deals - again — so "narrow window" is not a defense. Resolving once and threading the - result removes the second read rather than shrinking the window. + other. That race is cheap to retry — a lost round raises + `sweep-auto-not-started`, which `_maybe_auto_sweep` swallows, and the next + auto-sweep trigger deals again — so "narrow window" is not a defense. + Resolving once and threading the result removes the second read rather than + shrinking the window. ``cmd_run`` and ``_resume_paused_run`` thread it too, for a DIFFERENT reason — they stamp a baseline rather than compare against one, and at launch the @@ -210,10 +211,12 @@ def config_digest( restating two builders' precedence rules inside the control that polices them, where drift is silent and lands in the UNDER-covering direction — the failure this function has already made four times by reasoning from one - builder. Over-coverage fails the other way, loudly: ``sweep-auto-failed`` + - notify, with the message naming ``bmad-loop sweep`` as the human-present path. - Not free (the refusal burns the trigger for the life of the run, #501), but it - needs a writer, and nothing under ``src/`` writes ``.bmad-loop/profiles/*.toml`` + builder. Over-coverage fails the other way, loudly: ``sweep-auto-not-started`` + + notify, with the message naming ``bmad-loop sweep`` as the human-present + path. Not free — #501 stopped a refusal from *spending* the trigger, but that + is honest bookkeeping rather than a reprieve, since the same wrong answer + refuses the next trigger too. It needs a writer, though, and nothing under + ``src/`` writes ``.bmad-loop/profiles/*.toml`` at all — that overlay is hand-authored, and the TUI settings screen writes ``policy.toml`` (``extra_args`` included). So a dead-field rewrite arriving mid-run is a config change nobody automated made under a running loop, which @@ -268,9 +271,9 @@ def config_digest( it — this repo's own ``write_script_launcher`` is a stub that execs an interpreter on a sidecar, so hashing the stub misses the payload. Nor is the target ours to pin: it is normally a third-party CLI that self-updates, - and a mid-run update would move a content hash and burn the auto-sweep - trigger for the life of the run (``_maybe_auto_sweep`` records the trigger - BEFORE calling the factory, and early-returns on it forever after). + and a mid-run update would move a content hash and refuse every auto-sweep + for the life of the run (the digest is pinned in memory at launch, so + nothing on disk can re-bless it). Confinement is the instrument, not hashing — and as a ``validate`` warning rather than a refusal, since "resolves inside the project" does not decide it either: under an active project venv ``which("python")`` IS @@ -846,7 +849,7 @@ def compose_run( max_stories: int | None, stories_on: bool, spec_folder: str, - sweep_factory: Callable[[str], None], + sweep_factory: SweepFactory, make_adapters: MakeAdapters, engine_cls: type[Engine], stories_engine_cls: type[StoriesEngine], @@ -946,6 +949,7 @@ def compose_sweep( sweep_engine_cls: type[SweepEngine], trusted_config_digest: str, profiles: dict[str, CLIProfile] | None = None, + on_started: Callable[[], None] | None = None, ) -> ComposedRun: """Stand up a sweep run: allocate the run dir, persist state + pid, record the sweep options, build the adapters, and wire the ``SweepEngine`` — everything @@ -962,7 +966,48 @@ def compose_sweep( (see :func:`compose_resume`). ``make_adapters`` and ``sweep_engine_cls`` are injected so ``cli`` supplies its own module-level names — keeping the test suite's ``monkeypatch.setattr(cli, "SweepEngine"/"_make_adapters", ...)`` - effective.""" + effective. + + ``on_started`` is the auto-sweep parent's latch (``engine.SweepFactory``'s + ``started`` thunk, threaded through ``cli._start_sweep``): a parent run spends + its one trigger for this ``trigger`` string only if this fires. + ``cmd_sweep`` passes nothing — a human started that one, and there is no + trigger to spend. + + It fires as the LAST statement of the composition block, which is the boundary + that makes "started" mean something the parent can act on: from here the child + owns a published run dir, ``sweep.json`` and a live pid file, so a later + failure leaves a run ``bmad-loop resume`` can pick up rather than nothing at + all. Before commit ``9c7a284`` the boundary had to sit at ``save_state`` + instead — an abort anywhere after it stranded a resumable-looking run dir, and + :func:`compose_resume` will rebuild a sweep from ``state.json`` alone, + tolerating a missing ``sweep.json``, so "it never got far enough to resume" + was not true of the intervening steps. What moved it here is that block's + ``except BaseException`` arm, added by that commit, which unwinds the whole + partial composition. + + That premise has one documented exception, and it is worth reading rather than + waving at: :func:`_unwind_composition` is best-effort — ``force=False`` leaves + ``runs.delete_run``'s live-session guard armed, and the call sits under + ``suppress(Exception)`` — so a refused or failed unwind CAN leave a resumable + child behind while ``on_started`` never fired. On the auto path that needs a + live ``bmad-loop-`` session at this run's id, and the path mints the id + here: ``cli._sweep_factory`` calls ``_start_sweep`` with no ``run_id``, and the + only caller that supplies one is ``cmd_sweep`` (``--run-id``), which passes no + ``on_started``. So reaching it takes a :func:`runs.new_run_id` collision with a + concurrently live run — in which case this composition's own ``save_state`` + has already overwritten that run's ``state.json`` several statements earlier, + a pre-existing hazard of far greater consequence than a re-fired sweep. The + latch boundary is not the place to answer it. + + Firing inside the block rather than after it is deliberate for the same + reason: should the latch itself raise, the unwind covers it, and the parent's + in-memory flag — set BEFORE its write, see ``engine._maybe_auto_sweep`` — + refuses a second attempt either way. At-most-once therefore holds independently + of the unwind; what the unwind decides is only what that refusal costs. Normally + it refuses a child that left nothing behind; under the refused unwind above it + refuses one that is composed and resumable, which is the better of the two. + Neither is a second launch, and that is the safe direction for a launcher.""" run_id = run_id or runs.new_run_id() run_dir = project / RUNS_DIR / run_id journal = Journal(run_dir) @@ -1013,6 +1058,8 @@ def compose_sweep( repeat=repeat, max_cycles=max_cycles, ) + if on_started is not None: + on_started() except BaseException: _unwind_composition(project, run_dir) raise @@ -1027,7 +1074,7 @@ def compose_resume( state: RunState, policy: Policy, journal: Journal, - sweep_factory: Callable[[str], None], + sweep_factory: SweepFactory, make_adapters: MakeAdapters, engine_cls: type[Engine], stories_engine_cls: type[StoriesEngine], diff --git a/tests/test_cli.py b/tests/test_cli.py index f77d889e..9737b584 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -7654,17 +7654,21 @@ def test_auto_sweep_refuses_worktree_isolation_under_a_repo_root_override(projec the parent's own start was allowed not to check. It RAISES rather than returning, which is the whole point: `_maybe_auto_sweep` - has already journaled `sweep-auto-trigger` and latched the trigger by the time it - calls the factory, and it reads a plain return as success — so a quiet decline is - recorded as `sweep-auto-finished`, a child sweep that ran and finished when none - was launched. Raising lands on the `sweep-auto-failed` + notify path instead, + has already journaled `sweep-auto-trigger` by the time it calls the factory, and + it reads a plain return as a child that ran — so a quiet decline is recorded as + `sweep-auto-finished`, a child sweep that ran and finished when none was + launched. Raising lands on the `sweep-auto-not-started` + notify path instead, which is the same one an unparseable policy.toml already takes, and the parent - run is still unaffected (`_maybe_auto_sweep` swallows it).""" + run is still unaffected (`_maybe_auto_sweep` swallows it). + + The `started` thunk turns "no launch" into a positive assertion rather than an + absence: the refusal has to precede the boundary `compose_sweep` fires it at, + or the parent spends a trigger on a child that never existed (#501).""" from bmad_loop import bmadconfig _split_root_project(project) - started = [] - monkeypatch.setattr(cli, "_start_sweep", lambda *a, **kw: started.append(kw) or 0) + launched = [] + monkeypatch.setattr(cli, "_start_sweep", _stub_start_sweep(launched)) # Hand it a MATCHING config digest so the #461 gate (which sits ahead of the # isolation check) stays quiet and this test still measures the isolation @@ -7675,8 +7679,8 @@ def test_auto_sweep_refuses_worktree_isolation_under_a_repo_root_override(projec _config_pin(project), ) with pytest.raises(RuntimeError, match=REFUSAL): - factory("epic-boundary") - assert started == [] + factory("epic-boundary", started=_never_started) + assert launched == [] # --- #461 point 4: the auto-sweep child's config re-read is integrity-pinned --- @@ -7706,6 +7710,40 @@ def test_auto_sweep_refuses_worktree_isolation_under_a_repo_root_override(projec DIGEST_REFUSAL = "changed under a running loop before an auto-sweep" +def _never_started() -> None: + """The `started` thunk for every factory call that must be refused. Firing it + means the child sweep reached `compose_sweep`'s success boundary and the parent + would spend its trigger (#501), so a refusal that signals first is a bug even + when it goes on to raise — an assertion this makes positive, where `launched == + []` alone would pass for any reason `_start_sweep` did not run. + + Armed only because `_stub_start_sweep` relays the thunk: a stub that recorded + its kwargs and dropped them would make this decorative, since nothing could + then call it. + + Ablation, one for all four rows that use it: move `_sweep_factory`'s digest + gate BELOW its `_start_sweep` call (the refusal still raises, just too late) + and the three `DIGEST_REFUSAL` rows fail with "started before the gate" — + naming the boundary, where the `launched == []` assert alone would only say + something launched.""" + pytest.fail("started before the gate") + + +def _stub_start_sweep(launched: list): + """A `cli._start_sweep` stand-in that records its kwargs AND relays + `on_started`, the way the real one does through `compose_sweep`. Relaying is + the point: it is what lets a refusal test assert the gate ran first rather + than merely that nothing launched.""" + + def stub(*_a, **kw): + launched.append(kw) + if kw.get("on_started") is not None: + kw["on_started"]() + return 0 + + return stub + + def _pin_profile(project, text=PIN_PROFILE) -> None: profiles = project.project / ".bmad-loop" / "profiles" profiles.mkdir(parents=True, exist_ok=True) @@ -7716,19 +7754,25 @@ def _pinned_sweep_factory(project, monkeypatch, *, policy_text=PIN_POLICY): """A child-sweep factory pinned to the config as it stands right now — the launch baseline `cmd_run` hands it. Anything a test writes AFTER this returns is exactly the mid-run rewrite #461 point 4 describes: no human asked for it, - and the factory re-reads both files off disk on the engine's next trigger.""" + and the factory re-reads both files off disk on the engine's next trigger. + + Returns the factory plus the `_start_sweep` kwargs it reached, one entry per + launch. The stub relays `on_started` (see `_stub_start_sweep`) so the refusal + rows can assert the gate ran BEFORE it; the real boundary inside + `compose_sweep` is driven by + `test_auto_sweep_launches_the_profile_bytes_the_gate_validated`.""" from bmad_loop import bmadconfig install_bmad_config(project) _write_policy(project.project, policy_text) _pin_profile(project) write_sprint(project, {"1-1-a": "ready-for-dev"}) - started = [] - monkeypatch.setattr(cli, "_start_sweep", lambda *a, **kw: started.append(kw) or 0) + launched = [] + monkeypatch.setattr(cli, "_start_sweep", _stub_start_sweep(launched)) factory = cli._sweep_factory( project.project, bmadconfig.load_paths(project.project), _config_pin(project) ) - return factory, started + return factory, launched def test_auto_sweep_refuses_a_rewritten_verify_command(project, monkeypatch): @@ -7736,12 +7780,12 @@ def test_auto_sweep_refuses_a_rewritten_verify_command(project, monkeypatch): sandbox — and policy.toml sits in the workspace every driven session can write. The parent loop froze its Policy at launch, so this factory's fresh reload is the one path where that rewrite reaches execution with no human in the loop.""" - factory, started = _pinned_sweep_factory(project, monkeypatch) + factory, launched = _pinned_sweep_factory(project, monkeypatch) _write_policy(project.project, PIN_POLICY.replace('["true"]', '["touch pwned"]')) with pytest.raises(RuntimeError, match=DIGEST_REFUSAL): - factory("epic-boundary") - assert started == [] + factory("epic-boundary", started=_never_started) + assert launched == [] def test_auto_sweep_refuses_a_rewritten_profile_binary(project, monkeypatch): @@ -7749,24 +7793,24 @@ def test_auto_sweep_refuses_a_rewritten_profile_binary(project, monkeypatch): is read from profiles/*.toml through get_profile and never appears in the snapshot at all. This is why config_digest resolves profiles rather than hashing the policy dict.""" - factory, started = _pinned_sweep_factory(project, monkeypatch) + factory, launched = _pinned_sweep_factory(project, monkeypatch) _pin_profile(project, PIN_PROFILE.replace('binary = "mycli"', 'binary = "rogue-cli"')) with pytest.raises(RuntimeError, match=DIGEST_REFUSAL): - factory("epic-boundary") - assert started == [] + factory("epic-boundary", started=_never_started) + assert launched == [] def test_auto_sweep_refuses_a_widened_plugin_allowlist(project, monkeypatch): """`[plugins] enabled` is the trust gate for in-process Python import (plugins/trust.py) — adding a name to it mid-run is a straight path from a workspace write to code running inside the orchestrator itself.""" - factory, started = _pinned_sweep_factory(project, monkeypatch) + factory, launched = _pinned_sweep_factory(project, monkeypatch) _write_policy(project.project, PIN_POLICY + '\n[plugins]\nenabled = ["rogue"]\n') with pytest.raises(RuntimeError, match=DIGEST_REFUSAL): - factory("epic-boundary") - assert started == [] + factory("epic-boundary", started=_never_started) + assert launched == [] def test_auto_sweep_proceeds_after_a_benign_limits_edit(project, monkeypatch): @@ -7774,23 +7818,27 @@ def test_auto_sweep_proceeds_after_a_benign_limits_edit(project, monkeypatch): live-editing `[limits]` under a running loop as supported; a file hash would refuse every auto-sweep after one, turning a correctness feature into a regression. Nothing in `[limits]` reaches host exec, so nothing here fires.""" - factory, started = _pinned_sweep_factory(project, monkeypatch) + factory, launched = _pinned_sweep_factory(project, monkeypatch) _write_policy(project.project, PIN_POLICY + "\n[limits]\ncache_read_weight = 0.5\n") + signalled: list[str] = [] - factory("epic-boundary") # no raise + factory("epic-boundary", started=lambda: signalled.append("started")) # no raise - assert len(started) == 1 + assert len(launched) == 1 + # The thunk reaches `_start_sweep` rather than being dropped at this frame — + # the stub relays it, standing in for `compose_sweep`'s boundary (#501). + assert signalled == ["started"] def test_auto_sweep_proceeds_when_the_pinned_config_is_untouched(project, monkeypatch): """The gate's own null case — and the one that would catch a digest that is unstable across two reads of an unchanged tree, which would refuse every auto-sweep in the product.""" - factory, started = _pinned_sweep_factory(project, monkeypatch) + factory, launched = _pinned_sweep_factory(project, monkeypatch) - factory("epic-boundary") # no raise + factory("epic-boundary", started=lambda: None) # no raise - assert len(started) == 1 + assert len(launched) == 1 def test_auto_sweep_launches_the_profile_bytes_the_gate_validated(project, monkeypatch): @@ -7799,7 +7847,7 @@ def test_auto_sweep_launches_the_profile_bytes_the_gate_validated(project, monke file the driven sessions can write. A session that leaves a background writer behind swaps the overlay the instant the compare succeeds — modelled here by flipping it from inside `config_digest`'s own return — and the window is not a - defense, because a lost round only raises `sweep-auto-failed`, which + defense, because a lost round only raises `sweep-auto-not-started`, which `_maybe_auto_sweep` swallows before the next trigger deals again. So `_sweep_factory` resolves the profiles ONCE and threads that mapping through @@ -7807,8 +7855,18 @@ def test_auto_sweep_launches_the_profile_bytes_the_gate_validated(project, monke harm, not the plumbing: the adapter must carry the validated `binary`, and the child must not re-baseline itself onto the swapped config. + Doubles as the one place the `started` boundary runs FOR REAL — `_start_sweep` + is not stubbed here, so `compose_sweep` fires the thunk itself once the child + owns a published run dir. Every other #501 site either stubs `_start_sweep` + (kwargs only) or asserts the thunk never fires. + ABLATION: drop `profiles=` from either `runsetup.make_adapters` or - `_trusted_config_digest` in `_start_sweep` and the matching assert fails.""" + `_trusted_config_digest` in `_start_sweep` and the matching assert fails. Two + more lanes for the #501 thunk, and they redden different sets: dropping + `compose_sweep`'s `on_started()` call fails this row alone, while dropping + `on_started=` from the factory's `_start_sweep` call also reddens + `test_auto_sweep_proceeds_after_a_benign_limits_edit` — which is the seam + below this one, and the reason both exist.""" from bmad_loop import bmadconfig, runs from bmad_loop.adapters import profile as profile_mod @@ -7843,13 +7901,16 @@ def run(self): monkeypatch.setattr(cli, "SweepEngine", _Recorder) + signalled: list[str] = [] factory = cli._sweep_factory(project.project, bmadconfig.load_paths(project.project), pin) - factory("epic-boundary") # the gate saw the honest bytes and passed + # the gate saw the honest bytes and passed + factory("epic-boundary", started=lambda: signalled.append("started")) assert ( 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 signalled == ["started"] # #501: the child composed, so the parent may latch 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 @@ -7869,8 +7930,8 @@ def test_run_pins_the_profile_bytes_it_launches(project, monkeypatch): all. The harm is over-refusal: the pin is the one baseline a session cannot reach (`_sweep_factory` holds children to it from memory), so a pin over unlaunched bytes makes those children refuse the config the parent is running — - and `engine._maybe_auto_sweep` records the trigger BEFORE calling the factory, - so the refusal burns it for the life of the run. + every trigger, for the life of the run. (#501 stopped a refusal from *spending* + each trigger; nothing about a wrong pin gets better for that.) Asserts the invariant, not the plumbing: the stamp and the adapter agree. diff --git a/tests/test_engine.py b/tests/test_engine.py index 204204a4..cf0e4c00 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -7505,6 +7505,23 @@ def test_max_stories_survives_a_pause_resume(project): assert set(final.tasks) == {"1-1-a", "2-1-b"} # 2-2-c never dispatched — cap durable +def recording_factory(calls: list): + """An `engine.SweepFactory` double for a child sweep that composes fine: + records the trigger and signals `started`, the way `runsetup.compose_sweep` + does once the child owns a published run dir. + + It signals deliberately rather than leaning on the engine's latch-on-plain- + return arm — a double that never called `started` would quietly measure the + nothing-was-launched path in every test that uses it. The plain-return arm has + its own test (`test_auto_sweep_latches_a_factory_that_never_signalled`).""" + + def factory(trigger: str, *, started) -> None: + started() + calls.append(trigger) + + return factory + + def test_run_end_auto_sweep_fires_once(project): write_sprint(project, {"1-1-a": "ready-for-dev"}) policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) @@ -7513,7 +7530,7 @@ def test_run_end_auto_sweep_fires_once(project): project, [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], policy=policy, - sweep_factory=calls.append, + sweep_factory=recording_factory(calls), ) summary = engine.run() assert summary.done == 1 and not summary.paused @@ -7544,7 +7561,7 @@ def test_per_epic_auto_sweep_fires_at_boundary(project): review_effect(project, "2-1-b", clean=True), ], policy=policy, - sweep_factory=calls.append, + sweep_factory=recording_factory(calls), ) summary = engine.run() assert summary.done == 2 @@ -7552,8 +7569,16 @@ def test_per_epic_auto_sweep_fires_at_boundary(project): def test_auto_sweep_no_refire_on_resume(project): - """The per-epic trigger is recorded before the gate pause, so resuming - the run must not fire the same sweep again.""" + """The per-epic trigger is recorded before the gate pause, so resuming the run + must not fire the same sweep again. + + Green-ablation record, so this row is not miscounted as coverage of the latch: + it stays green with `sweeps_triggered` never written at all, because + `_epic_boundary` also advances `state.current_epic` before pausing and the + resumed run therefore never re-detects the epic-1 boundary. Both mechanisms + hold here; only one of them is the latch. + `test_auto_sweep_that_started_then_failed_keeps_the_trigger_spent` isolates + it.""" write_sprint( project, { @@ -7573,7 +7598,7 @@ def test_auto_sweep_no_refire_on_resume(project): project, [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], policy=policy, - sweep_factory=calls.append, + sweep_factory=recording_factory(calls), ) assert engine.run().paused assert calls == ["epic-1"] @@ -7590,17 +7615,28 @@ def test_auto_sweep_no_refire_on_resume(project): run_dir=engine.run_dir, journal=engine.journal, state=state, - sweep_factory=calls.append, + sweep_factory=recording_factory(calls), ) assert resumed.run().done == 2 assert calls == ["epic-1"] # not re-fired def test_auto_sweep_failure_does_not_pause_parent(project): + """A child that raises before signalling `started` never composed, so the + parent journals `sweep-auto-not-started` and leaves the trigger unspent — + read back off disk, because the whole subject is durable run state and an + in-memory list can agree with a state.json that does not. + + Ablation: make the `except (Exception, SystemExit)` arm call `latch()` before + its `if latched:` branch — spending the trigger on any raise, which is what + #501 changed — and the reloaded-state assert fails. Not alone: it shares that + mutation with the three other never-started rows (`..._re_asks_...`, + `..._system_exit_...`, `..._config_digest_refusal_...`), which is coverage + rather than duplication, since each names a different way a child declines.""" write_sprint(project, {"1-1-a": "ready-for-dev"}) policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) - def exploding(trigger): + def exploding(trigger, *, started): raise RuntimeError("child sweep blew up") engine, _ = make_engine( @@ -7613,7 +7649,245 @@ def exploding(trigger): assert summary.done == 1 and not summary.paused assert engine.state.finished journal = (engine.run_dir / "journal.jsonl").read_text() - assert "sweep-auto-failed" in journal and "child sweep blew up" in journal + assert "sweep-auto-not-started" in journal and "child sweep blew up" in journal + assert "sweep-auto-failed" not in journal # nothing ran, so nothing failed + assert load_state(engine.run_dir).sweeps_triggered == [] + + +def test_auto_sweep_latches_a_factory_that_never_signalled(project): + """The at-most-once control on the other side: a factory that returns + normally has run a child sweep, whether or not it bothered to call `started` + — the thunk exists to classify a *raise*. Without this the plain-return arm + could be dropped and every remaining test would still pass, because the + product's own factory signals. + + Ablation: delete the `latch()` call from `_maybe_auto_sweep`'s `else` arm and + this test fails alone.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) + calls = [] + + def silent(trigger, *, started): + calls.append(trigger) # deliberately never calls `started` + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + policy=policy, + sweep_factory=silent, + ) + engine.run() + + assert calls == ["run-end"] + assert load_state(engine.run_dir).sweeps_triggered == ["run-end"] + assert "sweep-auto-finished" in (engine.run_dir / "journal.jsonl").read_text() + + +def _re_ask(project, engine, policy, factory) -> Engine: + """A second engine over the run's state as it was persisted — the shape a + resume rebuilds, and the only way an already-answered trigger gets asked + again (see `_maybe_auto_sweep`'s crash-window verdict). Returned unrun so the + caller drives `_maybe_auto_sweep` directly: reaching the same trigger through + a whole second `run()` would depend on `finished`/`current_epic`, which is + exactly what these two tests must not measure.""" + return Engine( + paths=project, + policy=policy, + adapter=MockAdapter([]), + run_dir=engine.run_dir, + journal=engine.journal, + state=load_state(engine.run_dir), + sweep_factory=factory, + ) + + +def test_auto_sweep_that_started_then_failed_keeps_the_trigger_spent(project): + """The at-most-once control, without which the fix is indistinguishable from + "never latch on a failure". Once `started` fires the child owns a published, + resumable run dir, so a failure after that point must still spend the trigger + — and durably, since the re-ask arrives through a rebuilt engine. + + Ablation: empty out `latch()` (keep the def, drop its body) and this test + fails — the re-ask fires a second child. Not alone: that mutation stops the + trigger being recorded at all, so it also reddens `test_run_end_auto_sweep_fires_once`, + `..._latches_a_factory_that_never_signalled` and `..._run_stopped_stops_the_parent`. + Those three grade "it is recorded"; only this one grades "it survives a + failure that came after.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) + calls = [] + + def started_then_failed(trigger, *, started): + started() + calls.append(trigger) + raise RuntimeError("child sweep died after its run dir was published") + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + policy=policy, + sweep_factory=started_then_failed, + ) + summary = engine.run() + + assert summary.done == 1 and engine.state.finished # parent unaffected + assert calls == ["run-end"] + assert load_state(engine.run_dir).sweeps_triggered == ["run-end"] + journal = (engine.run_dir / "journal.jsonl").read_text() + assert "sweep-auto-failed" in journal # it ran, and then it failed + assert "sweep-auto-not-started" not in journal + + _re_ask(project, engine, policy, started_then_failed)._maybe_auto_sweep("run-end", "run-end") + assert calls == ["run-end"] # refused by the persisted latch + + +def test_auto_sweep_re_asks_a_trigger_whose_child_never_started(project): + """The twin, and the narrow thing the reordering actually buys: a trigger the + factory refused before composing is still askable. Not a retry the product + schedules — `_maybe_auto_sweep`'s docstring establishes that both call sites + close their own boundary within a few statements — but it is what makes the + crash window recoverable rather than a permanently spent trigger, and it is + the observable that grades the whole change. + + Two ablations, and the second is the sharper one. Restore the pre-#501 + ordering — insert `sweeps_triggered.append(trigger)` + `_save()` above the + `verify.worktree_clean` block — and this test fails, along with nine other + rows, because that mutation also double-records every successful sweep. Make + the `except (Exception, SystemExit)` arm call `latch()` unconditionally + instead, which is the same semantics without the noise, and it fails with + four (see `test_auto_sweep_failure_does_not_pause_parent`). Neither is + "alone"; what is unique to this row is the second ask.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) + calls = [] + + def refusing(trigger, *, started): + calls.append(trigger) + raise RuntimeError("policy.toml changed under a running loop") + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + policy=policy, + sweep_factory=refusing, + ) + engine.run() + assert calls == ["run-end"] + assert load_state(engine.run_dir).sweeps_triggered == [] + + _re_ask(project, engine, policy, refusing)._maybe_auto_sweep("run-end", "run-end") + assert calls == ["run-end", "run-end"] # asked again, because nothing was spent + + +def test_auto_sweep_not_started_still_notifies_with_its_own_wording(project, monkeypatch): + """`sweep-auto-not-started` keeps a `gates.notify`, and keeps a distinct one. + Recoverable is not the same as unremarkable: the loudest raise that reaches + this arm is #461 point 4's config-integrity refusal — a session rewrote the + verify commands, the launch binary or the plugin allowlist under a running + loop — and that is a security event whether or not the trigger survived it. + Losing the alert while gaining the retry would be a bad trade. + + Ablation: delete the `gates.notify` call from the not-started arm and this + test fails alone. Ablate the WORDING instead — pass the failed arm's "auto + sweep failed" — and it fails alone again, on the title assert, which is the + point of asserting the title at all: the two arms describe different things to + a human deciding whether to intervene.""" + notes = [] + monkeypatch.setattr( + "bmad_loop.gates.notify", + lambda policy, rd, title, message: notes.append((title, message)), + ) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) + + def refusing(trigger, *, started): + raise RuntimeError("policy.toml/profiles changed under a running loop") + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + policy=policy, + sweep_factory=refusing, + ) + engine.run() + + # filtered off the run-finished notice the clean-finish path also emits + sweep_notes = [n for n in notes if "sweep" in n[0]] + assert len(sweep_notes) == 1 + title, message = sweep_notes[0] + assert title == "auto sweep did not start" + assert title != "auto sweep failed" # the failed arm's wording, deliberately not reused + assert "run-end" in message and "changed under a running loop" in message + + +def _sweep_gate_engine(project, sweep_factory): + """An engine parked at the auto-sweep gate: `run-end` policy, a published + state.json (so the reload asserts below read a real file rather than error), + and no story loop — the two refusals under test are driven by calling + `_maybe_auto_sweep` directly, which is where they are decided.""" + policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) + engine, _ = make_engine(project, [], policy=policy, sweep_factory=sweep_factory) + engine._save() + return engine + + +def _skipped_dirty(engine) -> list[dict]: + return [e for e in engine.journal.entries() if e["kind"] == "sweep-auto-skipped-dirty"] + + +def test_auto_sweep_skips_a_dirty_tree_and_keeps_the_trigger(project): + """First test to reach `sweep-auto-skipped-dirty` at all. A stray uncommitted + change should not happen at either call site — both sit after a commit or a + reset — so this arm is a backstop, and until #501 it was a backstop that spent + the run's one sweep trigger on its way out. + + Ablation: delete the `if not clean:` block and this test fails alone — the + factory runs on top of the stray change. Second axis, for the ordering rather + than the refusal: restore the pre-#501 `sweeps_triggered.append` above the + check and the trigger assert fails (in a set of ten — see + `test_auto_sweep_re_asks_a_trigger_whose_child_never_started`).""" + calls = [] + (project.project / "stray.txt").write_text("uncommitted\n", encoding="utf-8") + engine = _sweep_gate_engine(project, recording_factory(calls)) + + engine._maybe_auto_sweep("run-end", "run-end") + + assert calls == [] + assert [e["reason"] for e in _skipped_dirty(engine)] == ["dirty"] + assert load_state(engine.run_dir).sweeps_triggered == [] + + +def test_auto_sweep_skips_a_git_fault_and_keeps_the_trigger(project, monkeypatch): + """The arm that motivated the reordering, and the one a `reason` field now + tells apart from a genuinely dirty tree. `verify.worktree_clean` fails closed + on a `GitError` — right, since an unknown tree state is no basis for a sweep — + but `_run_git` reports a `subprocess.TimeoutExpired` as exactly that, so a + `git status` that merely ran long used to spend this run's only sweep trigger, + permanently and with nothing in the journal to say a *fault* had happened. + + Driven through a real `TimeoutExpired` out of `subprocess.run` rather than a + stubbed `worktree_clean`, because the translation IS the claim: stubbing the + GitError would assume the very step that makes this arm transient-reachable. + + Ablation: delete the `except verify.GitError` arm and this test fails alone — + the GitError escapes `_maybe_auto_sweep` and crashes the run. Second axis, as + for the dirty twin: restore the pre-#501 `sweeps_triggered.append` above the + check and the trigger assert fails (in a set of ten).""" + calls = [] + engine = _sweep_gate_engine(project, recording_factory(calls)) + + def timing_out(cmd, **kwargs): + raise verify.subprocess.TimeoutExpired(cmd, kwargs.get("timeout", 1)) + + monkeypatch.setattr(verify.subprocess, "run", timing_out) + + engine._maybe_auto_sweep("run-end", "run-end") + + assert calls == [] + skipped = _skipped_dirty(engine) + assert [e["reason"] for e in skipped] == ["git-error"] + assert "timed out" in skipped[0]["error"] # the fault is named, not swallowed + assert load_state(engine.run_dir).sweeps_triggered == [] def test_auto_sweep_system_exit_does_not_kill_the_parent(project): @@ -7630,13 +7904,18 @@ def test_auto_sweep_system_exit_does_not_kill_the_parent(project): (`mux_usable` bottoms out in a bare `shutil.which`) on every call, so a child sweep can hit it in a parent run that launched fine. + Every one of those five sites is inside `compose_sweep`, ahead of the + `on_started` boundary, so this models the raise WITHOUT signalling and the + record is `sweep-auto-not-started`: no child run dir survives an adapter build + that exits. + Ablation: drop `SystemExit` from the `except (Exception, SystemExit)` tuple in `_maybe_auto_sweep` and this test fails alone — the SystemExit escapes `engine.run()` instead of being journaled.""" write_sprint(project, {"1-1-a": "ready-for-dev"}) policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) - def exiting(trigger): + def exiting(trigger, *, started): raise SystemExit("error: multiplexer backend is not usable on this host") engine, _ = make_engine( @@ -7650,8 +7929,9 @@ def exiting(trigger): assert summary.done == 1 and not summary.paused assert engine.state.finished journal = (engine.run_dir / "journal.jsonl").read_text() - assert "sweep-auto-failed" in journal and "not usable on this host" in journal + assert "sweep-auto-not-started" in journal and "not usable on this host" in journal assert "run-complete" in journal + assert load_state(engine.run_dir).sweeps_triggered == [] def test_auto_sweep_run_stopped_stops_the_parent(project, monkeypatch): @@ -7662,6 +7942,11 @@ def test_auto_sweep_run_stopped_stops_the_parent(project, monkeypatch): signal handler latches `_stopping = True` before raising and every later SIGTERM then returns at that latch. + Signals `started` first, as the real shape does: a child only reaches its own + stop arm by running, which is well past `compose_sweep`'s boundary. So the + trigger stays spent here — the stop arm is the one raise that neither + classifies as a failure nor un-spends anything. + Ablation: delete the `except RunStopped: raise` arm from `_maybe_auto_sweep` and this test fails alone — the stop is swallowed and the parent finishes.""" killed = [] @@ -7669,7 +7954,8 @@ def test_auto_sweep_run_stopped_stops_the_parent(project, monkeypatch): write_sprint(project, {"1-1-a": "ready-for-dev"}) policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) - def stopping(trigger): + def stopping(trigger, *, started): + started() raise RunStopped() # hard (graceful=False), as the child's stop arm re-raises engine, _ = make_engine( @@ -7683,10 +7969,12 @@ def stopping(trigger): saved = load_state(engine.run_dir) assert saved.stopped is True assert not saved.finished # the whole point: a stop must not read as a finish + assert saved.sweeps_triggered == ["run-end"] # it ran; the stop does not un-spend it assert killed == ["test-run"] journal = (engine.run_dir / "journal.jsonl").read_text() assert "run-stop" in journal assert "sweep-auto-failed" not in journal # a stop is not a failure + assert "sweep-auto-not-started" not in journal assert "run-complete" not in journal @@ -7706,7 +7994,8 @@ def test_auto_sweep_keyboard_interrupt_still_propagates(project, monkeypatch): write_sprint(project, {"1-1-a": "ready-for-dev"}) policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) - def interrupting(trigger): + def interrupting(trigger, *, started): + started() # Ctrl-C reaches a child that is already running raise KeyboardInterrupt() engine, _ = make_engine( @@ -7724,19 +8013,25 @@ def interrupting(trigger): entries = engine.journal.entries() stops = [e for e in entries if e["kind"] == "run-stop"] assert stops and stops[0]["reason"] == "KeyboardInterrupt" - assert not [e for e in entries if e["kind"] == "sweep-auto-failed"] + assert not [e for e in entries if e["kind"] in ("sweep-auto-failed", "sweep-auto-not-started")] def test_auto_sweep_config_digest_refusal_journals_and_spares_the_parent(project): """#461 point 4, end to end through the REAL factory rather than a stand-in - exploder: the config-integrity gate's raise has to land on the same - `sweep-auto-failed` + notify path every other child-sweep failure takes, and - the parent story loop has to finish anyway. + exploder: the config-integrity gate's raise has to land on the journal + + notify path every other unlaunched child takes, and the parent story loop has + to finish anyway. Pinning it here rather than trusting the exploder test above is the point — that one proves `_maybe_auto_sweep` catches *something*; this proves the gate raises (rather than returning quietly, which the engine would record as - `sweep-auto-finished`: a child sweep that ran when none was ever launched).""" + `sweep-auto-finished`: a child sweep that ran when none was ever launched). + + The gate sits ahead of `_start_sweep`, so it cannot have signalled `started` + and the trigger survives the refusal — #501. A security refusal is still the + loudest thing here, which is why `sweep-auto-not-started` keeps its own + `gates.notify` (pinned by + `test_auto_sweep_not_started_still_notifies_with_its_own_wording`).""" from bmad_loop import cli write_sprint(project, {"1-1-a": "ready-for-dev"}) @@ -7756,9 +8051,10 @@ def test_auto_sweep_config_digest_refusal_journals_and_spares_the_parent(project assert summary.done == 1 and not summary.paused assert engine.state.finished journal = (engine.run_dir / "journal.jsonl").read_text() - assert "sweep-auto-failed" in journal + assert "sweep-auto-not-started" in journal assert "changed under a running loop before an auto-sweep" in journal assert "sweep-auto-finished" not in journal + assert load_state(engine.run_dir).sweeps_triggered == [] def test_no_auto_sweep_by_default(project): @@ -7767,7 +8063,7 @@ def test_no_auto_sweep_by_default(project): engine, _ = make_engine( project, [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], - sweep_factory=calls.append, + sweep_factory=recording_factory(calls), ) engine.run() assert calls == [] @@ -8393,7 +8689,7 @@ def test_epic_boundary_auto_sweep_suppressed_by_graceful_stop(project, monkeypat review_effect(project, "1-1-a", clean=True), ], policy=policy, - sweep_factory=calls.append, + sweep_factory=recording_factory(calls), ) engine.run() @@ -8406,13 +8702,17 @@ def test_epic_boundary_auto_sweep_suppressed_by_graceful_stop(project, monkeypat def test_maybe_auto_sweep_suppressed_when_graceful_stop_pending(project): """The run-end race: a request landing after the loop-head check reaches - _maybe_auto_sweep, which suppresses the sweep (return, not raise) and — because - the guard precedes the sweeps_triggered append — leaves the trigger unrecorded - so a later resume can still fire it.""" + _maybe_auto_sweep, which suppresses the sweep (return, not raise) and leaves + the trigger unspent. + + Unspent is honest bookkeeping, NOT a promised retry — at this call site the + return lands in `_loop`'s exit and then `finished = True`, which + `cli._resume_paused_run` refuses to resume. See `_maybe_auto_sweep`'s + docstring for the same verdict at both call sites.""" policy = Policy(gates=GatesPolicy(mode="none"), notify=QUIET, sweep=SweepPolicy(auto="run-end")) calls = [] run_dir = project.project / ".bmad-loop" / "runs" / "test-run" - engine, _ = make_engine(project, [], policy=policy, sweep_factory=calls.append) + engine, _ = make_engine(project, [], policy=policy, sweep_factory=recording_factory(calls)) _lodge_stop_request(run_dir) engine._maybe_auto_sweep("run-end", "run-end") @@ -8421,7 +8721,7 @@ def test_maybe_auto_sweep_suppressed_when_graceful_stop_pending(project): kinds = [e["kind"] for e in engine.journal.entries()] assert "sweep-auto-suppressed" in kinds assert "sweep-auto-trigger" not in kinds - assert "run-end" not in engine.state.sweeps_triggered # unrecorded → resumable + assert "run-end" not in engine.state.sweeps_triggered # nothing started, nothing spent def test_pause_wins_over_pending_graceful_stop(project, monkeypatch): diff --git a/tests/test_runsetup.py b/tests/test_runsetup.py index d1749aaf..50308f7f 100644 --- a/tests/test_runsetup.py +++ b/tests/test_runsetup.py @@ -394,7 +394,7 @@ def test_compose_run_unwinds_the_run_when_the_adapters_abort(unwinding): max_stories=None, stories_on=False, spec_folder="", - sweep_factory=lambda _trigger: None, + sweep_factory=lambda _trigger, *, started: None, make_adapters=unwinding.make_adapters, engine_cls=_NeverBuilt, stories_engine_cls=_NeverBuilt, From aab2b19ac6fbc88d7599a66dbaa814836d1d1871 Mon Sep 17 00:00:00 2001 From: t Date: Fri, 14 Aug 2026 22:40:56 -0700 Subject: [PATCH 04/11] fix: claim a fresh run dir so a launch cannot compose over an existing run (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of this PR, P2, and CONFIRMED against the code rather than taken on faith. Both composers went `run_id -> run_dir -> Journal(run_dir)`, and `Journal.__init__` mkdirs with `exist_ok=True` — so the hidden `--run-id` flag (present on both `run` and `sweep`, used by the TUI) pointed at an EXISTING run adopted that run's directory. Two consequences, one of them shipped and one of them mine: * Shipped: `save_state` published this composition's `state.json` over the prior run's, silently. * Introduced by `9c7a284` in this PR: the reachable `make_adapters` SystemExit then drove `_unwind_composition`, which `runs.delete_run`s the whole directory and discards the out-of-tree state dir. `delete_run`'s only guard refuses a LIVE session, and a paused, stopped or finished run has none — so the prior run's journal, logs, tasks and state were erased permanently. That escalated the blast radius from a clobbered `state.json` to total loss. `_unwind_composition`'s docstring had already reasoned about `--run-id` collisions and concluded the `force=False` guard covered them. It does not: the guard is scoped to liveness alone, and the dangerous case is precisely the run that is NOT live. A collapsed prerequisite, argued confidently. The fix is a CLAIM, not a check: `_claim_run_dir` creates the run dir with `exist_ok=False`, so creation and collision-refusal are one atomic step and everything downstream may treat "this run dir is ours" as PROVEN. That is what an unconditional `rmtree` needs; an `exists()` probe would only have inferred it, and racily. It sits OUTSIDE the composers' try on purpose — a refusal reaching the unwind arm would delete the very run it exists to protect — and raises SystemExit with an `error:` line, matching `_reject_bad_run_id` and `make_adapters`' five sites. Applied to minted ids too, not just supplied ones: `new_run_id` is a timestamp plus two random bytes, so a same-second collision is remote rather than impossible, and a guard that holds for every id lets callers state their run dir's freshness flatly instead of qualifying it by provenance. Verified the TUI mints `runs.new_run_id()` and passes it through `--run-id` WITHOUT creating the directory (tui/app.py), so no legitimate caller composes over an existing dir and the refusal breaks nothing. `compose_resume` is untouched and was never at risk — it does not unwind. Two docstrings corrected rather than left standing, since the guard invalidates their reasoning: `_unwind_composition`'s `force=False` paragraph now argues from the narrower case that survives (an ORPHANED session outliving its run dir, not a pre-existing run at that id), and `compose_sweep`'s refused-unwind paragraph drops its "`save_state` has already clobbered that run's state.json" premise, which the claim makes unreachable. CHANGELOG entry covers the shipped half only — composing over an existing run — and does not claim to fix the unwind hazard, which never shipped. Tests: one row per composer, because a shared guard only one composer calls is the failure a single row would hide. The load-bearing assertion is `probe.published == {}` — a positive control proving the refusal beat `make_adapters`, hence `save_state` and the `except BaseException` arm. "Is the prior run still there" alone would pass just as happily for a composer that published over it and then failed to unwind. Ablations, singly, against a `cp` backup of the FIXED state; all rc == 1, source restored byte-identical afterwards (md5 verified). A-a claim dropped from `compose_run` -> 1 red (the run row), 1 green A-b claim dropped from `compose_sweep` -> 1 red (the sweep row), 1 green A-c `exist_ok=False` -> `True` -> 2 red (claim inert everywhere) A-d both claims moved INSIDE the try -> 2 red A-c and A-d redden both rows rather than isolating; recorded as measured, not as "fails alone". A-d is the placement axis and is worth reading: the SystemExit still raises and still matches "already exists", so `pytest.raises` stays green — what fails is `FileNotFoundError` on the prior run's `state.json`, because the unwind deleted it. That is the destructive behavior itself, caught. `uv run pytest -q -n logical`: 5475 passed, 44 skipped, 5 xfailed. `uv run pyright`: 0 errors. `trunk check --all --no-fix`: no issues. --- CHANGELOG.md | 10 +++++ src/bmad_loop/runsetup.py | 76 +++++++++++++++++++++++++++----- tests/test_runsetup.py | 93 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b3c1797..8c239d49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,16 @@ breaking changes may land in a minor release. ### Fixed +- **Launching with a `--run-id` that already names a run is refused instead of composing over it + (#501).** Both composers went straight into a journal whose `mkdir` accepts an existing directory, + so the hidden `--run-id` flag pointed at a previous run adopted that run's directory and published + its own `state.json` over it. The id is now claimed exclusively when the run directory is created, + and a collision aborts the launch before anything is written, leaving the earlier run untouched. + This also bounds the unwind below: the directory it removes on a failed composition is one this + launch is known to have created, never a pre-existing run — which for a paused, stopped or + finished run would have taken its journal, logs and state with it, since the removal's only guard + refuses a _live_ session. + - **A launch that aborts while standing up its adapters no longer strands an empty run (#501).** Both composers published the run directory, its `state.json` and the out-of-tree config-digest stamp before calling `make_adapters` — which raises `SystemExit` from five sites (an unresolvable diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index 7498b9fb..d5b519d6 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -791,10 +791,55 @@ class ComposedRun: journal: Journal +def _claim_run_dir(run_dir: Path) -> None: + """Take exclusive ownership of a fresh run directory, refusing an id that + already names a run. + + A **claim**, not a check, and that distinction is the whole point: + ``exist_ok=False`` makes the directory's creation and the collision refusal one + atomic operation, so what follows may treat "this run dir is ours" as PROVEN + rather than inferred. :func:`_unwind_composition` deletes this directory + wholesale on a failed composition, and inference is not good enough to license + an ``rmtree``. + + The hazard is not hypothetical. ``run_id`` is caller-supplied through the + hidden ``--run-id`` flag on both ``run`` and ``sweep``, and the composers ran + straight into ``Journal(run_dir)``, whose ``mkdir(parents=True, + exist_ok=True)`` adopts an existing directory without complaint. So pointing + ``--run-id`` at a *pre-existing* paused, stopped or finished run published this + composition's ``state.json`` over that run's, and then — once ``make_adapters`` + raised its reachable ``SystemExit`` — unwound the whole thing: journal, logs, + tasks and out-of-tree state, permanently. ``delete_run``'s guard does not cover + it, since that guard refuses only a *live* session and a paused or finished run + has none. + + Refusing before anything is published is what makes that unreachable, so this + MUST stay outside the composers' ``try`` — a refusal that reached the unwind + arm would delete the very run it exists to protect. ``SystemExit`` matches the + other launch-time refusals an operator reads as an ``error:`` line + (``_reject_bad_run_id``, and ``make_adapters``' five sites). + + Applied to a minted id too, not just a supplied one. ``new_run_id`` is a + timestamp plus two random bytes, so a same-second collision is remote rather + than impossible — and a guard that holds for every id lets callers state the + freshness of their run dir flatly instead of qualifying it by provenance.""" + try: + run_dir.mkdir(parents=True, exist_ok=False) + except FileExistsError as e: + raise SystemExit( + f"error: run {run_dir.name} already exists — refusing to compose over it. " + "`--run-id` must name a run that does not exist yet." + ) from e + + def _unwind_composition(project: Path, run_dir: Path) -> None: """Remove the run a failed ``compose_*`` had already published, so a launch that aborts partway leaves nothing behind. + Safe as a wholesale removal only because :func:`_claim_run_dir` created this + directory with ``exist_ok=False`` moments earlier: the run being deleted is + provably this composition's, never a pre-existing one the caller named. + Reached from an ``except BaseException`` arm, because the failure it exists for is a :class:`SystemExit`: :func:`make_adapters` raises one at five sites (unresolvable profile, unknown adapter kind, a kind that fails to load, a @@ -813,13 +858,15 @@ def _unwind_composition(project: Path, run_dir: Path) -> None: *operator's* explicit override, and there is no operator here — this is an automatic unwind. What it would skip is the one guard protecting the one state where a run dir is load-bearing: an untagged live ``bmad-loop-`` session, - for which that directory is the only ownership proof a later prune can read. A - run id is caller-supplied (``--run-id``), so such a session can exist at the id - this launch just claimed, and it cannot be this launch's — no session was ever - spawned. Deleting the directory would leak it for the life of the machine. - When the guard does fire the cost is exactly the pre-fix behavior, a stranded - run dir, which is no worse than what this replaces; ``force=True`` would trade - that bounded cost for an unbounded one. + for which that directory is the only ownership proof a later prune can read. + :func:`_claim_run_dir` rules out a session belonging to a *pre-existing run* at + this id — there is no such run — but not an orphaned session outliving the run + dir it was named for, which this launch would then be deleting the only + ownership proof of while never having spawned a session of its own. Narrower + than the case this paragraph used to argue, and still real. When the guard does + fire the cost is exactly the pre-fix behavior, a stranded run dir, which is no + worse than what this replaces; ``force=True`` would trade that bounded cost for + an unbounded one. Best-effort, and that is the whole point of the suppression: the caller is already unwinding an exception the operator has to see, and a cleanup failure @@ -878,6 +925,9 @@ def compose_run( """ run_id = run_id or runs.new_run_id() run_dir = project / RUNS_DIR / run_id + # Outside the try below, and it must stay there: a collision refusal that + # reached `_unwind_composition` would delete the run it exists to protect. + _claim_run_dir(run_dir) journal = Journal(run_dir) state = build_run_state( run_id=run_id, @@ -994,11 +1044,11 @@ def compose_sweep( live ``bmad-loop-`` session at this run's id, and the path mints the id here: ``cli._sweep_factory`` calls ``_start_sweep`` with no ``run_id``, and the only caller that supplies one is ``cmd_sweep`` (``--run-id``), which passes no - ``on_started``. So reaching it takes a :func:`runs.new_run_id` collision with a - concurrently live run — in which case this composition's own ``save_state`` - has already overwritten that run's ``state.json`` several statements earlier, - a pre-existing hazard of far greater consequence than a re-fired sweep. The - latch boundary is not the place to answer it. + ``on_started``. So reaching it needs a live session at an id that names no run + of its own — an orphan outliving its run dir — because a collision with a run + that still EXISTS is now refused before anything is published + (:func:`_claim_run_dir`), and a :func:`runs.new_run_id` collision is remote to + begin with. The latch boundary is not the place to answer what is left. Firing inside the block rather than after it is deliberate for the same reason: should the latch itself raise, the unwind covers it, and the parent's @@ -1010,6 +1060,8 @@ def compose_sweep( Neither is a second launch, and that is the safe direction for a launcher.""" run_id = run_id or runs.new_run_id() run_dir = project / RUNS_DIR / run_id + # Same claim, same reason, same placement outside the try as in `compose_run`. + _claim_run_dir(run_dir) journal = Journal(run_dir) state = RunState( run_id=run_id, diff --git a/tests/test_runsetup.py b/tests/test_runsetup.py index 50308f7f..fe456c5a 100644 --- a/tests/test_runsetup.py +++ b/tests/test_runsetup.py @@ -426,3 +426,96 @@ def test_compose_sweep_unwinds_the_run_when_the_adapters_abort(unwinding): trusted_config_digest="deadbeef", ) _assert_unwound(unwinding) + + +PRIOR_STATE = '{"run_id": "prior", "finished": true}' +PRIOR_JOURNAL = '{"kind": "run-complete"}\n' +PRIOR_STAMP = "prior-digest" + + +def _seed_prior_run(project): + """A finished run already occupying RUN_ID — dir, state, journal, and the + out-of-tree state dir the unwind's `_discard_state_dir` also reaches. + + Finished, deliberately: `delete_run`'s only guard refuses a LIVE session, so a + run that has none is exactly the case that guard does not cover.""" + run_dir = runs.run_dir_for(project, RUN_ID) + run_dir.mkdir(parents=True) + (run_dir / "state.json").write_text(PRIOR_STATE, encoding="utf-8") + (run_dir / "journal.jsonl").write_text(PRIOR_JOURNAL, encoding="utf-8") + state_dir = runs.state_dir_for(project, RUN_ID) + state_dir.mkdir(parents=True, exist_ok=True) + (state_dir / "config-digest").write_text(PRIOR_STAMP, encoding="utf-8") + return run_dir, state_dir + + +def _assert_prior_run_untouched(probe, run_dir, state_dir): + """The prior run survives BYTE-IDENTICAL, and the refusal beat publication. + + `probe.published == {}` is the positive control and the load-bearing half: an + "is it still there" assertion alone passes just as happily for a composer that + published over the run and then failed to unwind it. An empty dict says + `make_adapters` was never reached, so `save_state` never ran and the + `except BaseException` arm was never entered — which is the claim, since the + guard's whole placement requirement is that it sits OUTSIDE that try.""" + assert probe.published == {} + assert (run_dir / "state.json").read_text(encoding="utf-8") == PRIOR_STATE + assert (run_dir / "journal.jsonl").read_text(encoding="utf-8") == PRIOR_JOURNAL + assert (state_dir / "config-digest").read_text(encoding="utf-8") == PRIOR_STAMP + + +def test_compose_run_refuses_a_run_id_that_already_exists(unwinding): + """`--run-id` naming an existing run must be refused before anything is + published, leaving that run untouched. + + Without the claim this was destructive, not merely sloppy: `Journal.__init__` + mkdirs with `exist_ok=True`, so the composer adopted the existing directory, + `save_state` overwrote its `state.json`, and the reachable `make_adapters` + SystemExit then drove `_unwind_composition` — which `rmtree`s the whole run dir + and discards its out-of-tree state. A paused, stopped or finished run has no + live session, so `delete_run`'s guard never fires: the prior run's journal, + logs, tasks and state were erased permanently.""" + run_dir, state_dir = _seed_prior_run(unwinding.project) + with pytest.raises(SystemExit, match="already exists"): + runsetup.compose_run( + project=unwinding.project, + paths=_fake_paths(unwinding.project), + policy=policy_mod.loads(""), + run_id=RUN_ID, + epic_filter=None, + story_filter=None, + max_stories=None, + stories_on=False, + spec_folder="", + sweep_factory=lambda _trigger, *, started: None, + make_adapters=unwinding.make_adapters, + engine_cls=_NeverBuilt, + stories_engine_cls=_NeverBuilt, + trusted_config_digest="deadbeef", + ) + _assert_prior_run_untouched(unwinding, run_dir, state_dir) + + +def test_compose_sweep_refuses_a_run_id_that_already_exists(unwinding): + """The sweep composer carries its own copy of the claim, so it gets its own + row — `cmd_sweep --run-id` is a caller-supplied id on the same hidden flag, and + a shared guard that only one composer actually calls is the failure mode a + single test here would hide.""" + run_dir, state_dir = _seed_prior_run(unwinding.project) + with pytest.raises(SystemExit, match="already exists"): + runsetup.compose_sweep( + project=unwinding.project, + paths=_fake_paths(unwinding.project), + policy=policy_mod.loads(""), + run_id=RUN_ID, + prompting=False, + decisions_only=False, + max_bundles=None, + repeat=None, + max_cycles=None, + trigger="auto", + make_adapters=unwinding.make_adapters, + sweep_engine_cls=_NeverBuilt, + trusted_config_digest="deadbeef", + ) + _assert_prior_run_untouched(unwinding, run_dir, state_dir) From 8c95b457f74d8a2746a924dc5d633acbd5ce2b06 Mon Sep 17 00:00:00 2001 From: t Date: Fri, 14 Aug 2026 22:43:24 -0700 Subject: [PATCH 05/11] test: pin the unwind covering a raising started-latch (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `compose_sweep`'s docstring argues that should the latch itself raise, the unwind covers it — so at-most-once holds on the parent's in-memory flag and the only question is whether the refused retry cost nothing or a composed, resumable child. That was prose. Nothing tested it. This is also the one arm the two adapter-abort rows cannot reach: they fail early in the try, at `make_adapters`, so they stay green if the composition block's extent is narrowed to end before `on_started`. The new row fails LATE — at the block's last statement — which is the placement the whole latch boundary rests on. Prompted by a CodeRabbit finding asking for cleanup coverage from a raise site other than `make_adapters`. Its literal ask (an engine constructor raising SystemExit, in both composers) is declined: that is the same `except BaseException` arm reached from a neighbouring statement, so it duplicates a mechanism rather than covering a path. The latch is the raise site that is genuinely uncovered AND load-bearing for a claim this PR makes in prose. Graded as a REMOVAL, not an absence: the latch records all four artifacts (`run_dir`, `state.json`, `sweep.json`, the out-of-tree state dir) at the moment it fires, so "is it gone" cannot pass for a run that was never published. Ablation: `on_started()` moved out of the try, below the `except` arm. rc == 1, source restored byte-identical (md5 verified). It isolates cleanly — the `pytest.raises` still matches and the four-artifact positive control still passes; the single failing line is `assert not run_dir.exists()`, i.e. the child was not unwound. That is exactly the coupling asserted, and nothing else. `uv run pytest -q -n logical`: 5476 passed, 44 skipped, 5 xfailed. `uv run pyright`: 0 errors. `trunk check --all --no-fix`: no issues. --- tests/test_runsetup.py | 54 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/tests/test_runsetup.py b/tests/test_runsetup.py index fe456c5a..94fe5fa5 100644 --- a/tests/test_runsetup.py +++ b/tests/test_runsetup.py @@ -496,6 +496,60 @@ def test_compose_run_refuses_a_run_id_that_already_exists(unwinding): _assert_prior_run_untouched(unwinding, run_dir, state_dir) +class _BuiltEngine: + """Engine stand-in that constructs cleanly — the inverse of `_NeverBuilt`, for + the one test that must get PAST `make_adapters`.""" + + def __init__(self, *args, **kwargs): + pass + + +def test_compose_sweep_unwinds_when_the_started_latch_raises(tmp_path): + """`on_started` fires as the LAST statement inside the composition block, so a + latch that raises must unwind the child like any other escape. + + This is the one arm the adapter-abort rows above cannot reach: they fail early + in the try, at `make_adapters`, so they would stay green if the block's extent + were narrowed to end before the latch. It is also the case `compose_sweep`'s + docstring reasons about — the parent's in-memory flag is set before its write, + so at-most-once holds, and what the unwind decides is only whether the refused + retry cost nothing or a composed, resumable child. That argument is prose until + something pins the unwind actually covering a raising latch.""" + published: dict[str, bool] = {} + + def make_adapters(project, run_dir, policy, *, profiles=None): + return {"dev": object(), "review": object(), "triage": object()} + + def boom() -> None: + published["run_dir"] = runs.run_dir_for(tmp_path, RUN_ID).is_dir() + published["state"] = (runs.run_dir_for(tmp_path, RUN_ID) / "state.json").is_file() + published["sweep"] = (runs.run_dir_for(tmp_path, RUN_ID) / "sweep.json").is_file() + published["state_dir"] = runs.state_dir_for(tmp_path, RUN_ID).is_dir() + raise RuntimeError("latch write failed") + + with pytest.raises(RuntimeError, match="latch write failed"): + runsetup.compose_sweep( + project=tmp_path, + paths=_fake_paths(tmp_path), + policy=policy_mod.loads(""), + run_id=RUN_ID, + prompting=False, + decisions_only=False, + max_bundles=None, + repeat=None, + max_cycles=None, + trigger="auto", + make_adapters=make_adapters, + sweep_engine_cls=_BuiltEngine, + trusted_config_digest="deadbeef", + on_started=boom, + ) + # Graded as a removal, not an absence: the latch saw all four artifacts. + assert published == {"run_dir": True, "state": True, "sweep": True, "state_dir": True} + assert not runs.run_dir_for(tmp_path, RUN_ID).exists() + assert not runs.state_dir_for(tmp_path, RUN_ID).exists() + + def test_compose_sweep_refuses_a_run_id_that_already_exists(unwinding): """The sweep composer carries its own copy of the claim, so it gets its own row — `cmd_sweep --run-id` is a caller-supplied id on the same hidden flag, and From 8e3524783b45a74ec703b3f57943d5c79d9b5e9a Mon Sep 17 00:00:00 2001 From: t Date: Fri, 14 Aug 2026 22:56:29 -0700 Subject: [PATCH 06/11] fix: report a failed composition unwind instead of swallowing it (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two codex findings from the review of this branch, both P1, both validated against the code before acting. FINDING 1 (runsetup.py, AGENTS.md's "repair writes must raise") — TAKEN. `_unwind_composition` wrapped `runs.delete_run` in a bare `with suppress(Exception)`. The docstring defended that at length, but it defended the wrong proposition: the argument establishes only that a cleanup failure must not REPLACE the launch error the operator has to read, and says nothing about surfacing it. Those are separate decisions and had been conflated. The cost of conflating them: a refused or failed unwind left exactly the resumable-looking ghost run this function exists to prevent, and left it inferable only from the ABSENCE of an effect. The operator reads the adapter failure, and nothing anywhere says the cleanup after it did not happen. "Repair writes must raise" cannot be honored literally here — raising is precisely what would swallow the launch error — so the obligation it encodes is discharged by reporting: a `warning:` line on stderr naming the run and the `bmad-loop delete ` remedy (verified that command exists and takes a positional run_id; I first wrote `bmad-loop rm`, which does not exist), plus a `composition-unwind-failed` journal entry. The journal write is itself suppressed, and that is load-bearing rather than defensive habit. The journal lives INSIDE the run dir, so it lands for every failure that leaves one behind — the live-session guard refusing, or a failed `rmtree` — which is also the only case where a ghost run is what the operator will find. When `_discard_state_dir` is instead what failed, the dir is already gone and `Journal.append` opens with "a" WITHOUT a mkdir: it raises `FileNotFoundError` rather than resurrecting the run it just removed. Unsuppressed, that would propagate and replace the launch error — the exact outcome the arm forbids. FINDING 2 (CHANGELOG length) — TAKEN, though its framing was overstated. Codex called the entries a direct violation of the changelog contract. The contract does say terse/scannable/imperative, and two of my four entries were multi-paragraph narratives, so the core is right and they are now 4/6/6/8 lines (from 9/12/13/18). But "directly violates" does not survive measurement: the pre-existing `Unreleased` entries already on main run 21, 26 and 38 lines. Mine were never the longest thing in that section and are now well under it. Implementation rationale moved to code comments and history, where it belongs; the user-visible outcome stayed, including the "not a retry" clarification, which is meaning rather than rationale. The unwind-reporting change folds into the existing composition entry rather than opening a new one — same subject, and a separate bullet would fragment it. Tests: two rows, deliberately split, because the two failure modes are disjoint and one row would hide that. Row 1 — `delete_run` raises with the run dir surviving — asserts the stderr report, the journal entry, and the ghost actually being there. Row 2 — `delete_run` removes the dir and THEN raises — asserts the report still lands when the journal is gone. In both, `pytest.raises(SystemExit, match=...)` is the load-bearing half: it pins that the error reaching the operator is still `make_adapters`', not the cleanup's. A bare `pytest.raises` would pass just as happily for a cleanup failure that replaced it. Ablations, singly, against a `cp` backup of the FIXED state; all rc == 1, source restored byte-identical afterwards (md5 verified). B-a whole arm back to the silent `with suppress` -> 2 red B-b stderr report deleted -> 2 red B-c `journal.append` deleted -> 1 red (row 1 only) B-d suppress around `journal.append` deleted -> 1 red (row 2 only) B-c and B-d redden DISJOINT rows, and that disjointness is the proof the split was right rather than duplication — verified by name, not inferred: B-d fails row 2 with the `FileNotFoundError` replacing the `SystemExit`, which is the whole reason the suppression is there. `uv run pytest -q -n logical`: 5478 passed, 44 skipped, 5 xfailed. `uv run pyright`: 0 errors. `trunk check --all --no-fix`: no issues. --- CHANGELOG.md | 75 +++++++++++------------------------ src/bmad_loop/runsetup.py | 54 +++++++++++++++++++------- tests/test_runsetup.py | 82 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c239d49..b258af7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,62 +39,33 @@ breaking changes may land in a minor release. ### Fixed -- **Launching with a `--run-id` that already names a run is refused instead of composing over it - (#501).** Both composers went straight into a journal whose `mkdir` accepts an existing directory, - so the hidden `--run-id` flag pointed at a previous run adopted that run's directory and published - its own `state.json` over it. The id is now claimed exclusively when the run directory is created, - and a collision aborts the launch before anything is written, leaving the earlier run untouched. - This also bounds the unwind below: the directory it removes on a failed composition is one this - launch is known to have created, never a pre-existing run — which for a paused, stopped or - finished run would have taken its journal, logs and state with it, since the removal's only guard - refuses a _live_ session. - -- **A launch that aborts while standing up its adapters no longer strands an empty run (#501).** - Both composers published the run directory, its `state.json` and the out-of-tree config-digest - stamp before calling `make_adapters` — which raises `SystemExit` from five sites (an unresolvable - profile, an unknown adapter kind, a kind that fails to load, a construction failure, an unusable - multiplexer). An escape there left a run carrying `finished=False`, `crashed=False` and no - `run-start`, and nothing reconciled it: the stale-worktree sweep only visits finished runs, so it - lingered in `bmad-loop list` looking resumable. Composition is now atomic from the first published - artifact onward — on any escape the run dir and its out-of-tree state dir are both removed and the - original exception is re-raised unchanged. The removal is best-effort and keeps the live-session - guard (no `force`), so on the one state where a run dir is load-bearing — an untagged live agent - session, for which it is the only ownership proof — the dir is left alone rather than leaking the - session. +- **Launching with a `--run-id` that already names a run is refused (#501).** The flag pointed at an + existing run adopted that run's directory and published its own `state.json` over it. The id is + now claimed exclusively as the directory is created, so a collision aborts the launch before + anything is written and the earlier run is left untouched. + +- **A launch that aborts while standing up its adapters no longer strands an empty run (#501).** An + adapter failure left a run directory with no `run-start` that nothing reconciled, so it lingered + in `bmad-loop list` looking resumable. Composition is now atomic from the first published + artifact: on any escape the run directory and its out-of-tree state are removed and the original + error is re-raised unchanged. The removal keeps the live-session guard, and a removal that itself + fails is now reported instead of passing silently. - **A failing auto-sweep can no longer kill its parent run, and a stop during one is no longer - swallowed (#501).** The child-sweep guard promised never to interrupt the parent, but was written - over `Exception`, and that set differs from "a paused or failed child" in both directions. A - `SystemExit` — what `runsetup.make_adapters` raises for an unusable multiplexer, an unresolvable - profile or an adapter kind that fails to load — is a `BaseException`, so it escaped the guard, - every arm of the engine's run handler, and `main()`, ending the process at exit 1 with the parent - left neither `finished` nor `crashed`, no `run-complete`, and an orphaned agent session. The - unusable-multiplexer gate re-probes live on every call, so a child could hit it in a run that - launched fine. In the other direction `RunStopped` _is_ an `Exception` but is not a failure: the - child re-raises it so the owner records the stop, and eating it as `sweep-auto-failed` let the - parent run on to `finished` — and left it unstoppable, since the signal handler latches - `_stopping` before raising, so every later SIGTERM returned at that latch. `KeyboardInterrupt` - deliberately still escapes; the nested-child re-raise depends on it. + swallowed (#501).** A `SystemExit` from the child — what an unusable multiplexer or an + unresolvable profile raises — escaped every handler and ended the process at exit 1, leaving the + parent neither `finished` nor `crashed` with an orphaned agent session. In the other direction a + stop was recorded as a child failure, letting the parent run on to `finished` and leaving it + unstoppable. `KeyboardInterrupt` deliberately still escapes. - **A run's `sweeps_triggered` records only auto-sweeps that actually started (#501).** The trigger - was recorded — and the record persisted — before anything had been attempted, so every way a - child sweep could decline to launch still spent it: the `[verify]`/profile/plugin - config-integrity refusal, the worktree-isolation refusal, an unparseable `policy.toml`, an - unusable multiplexer. Worst of the set was the tree check, which fails closed on a git error and - reaches one on a plain `git status` timeout — so a slow filesystem, not a dirty tree, could - silently consume a run's one and only sweep. The check now runs ahead of the record and journals - a `reason` telling a git fault from real local changes, and the launcher signals the engine once - the child owns a published run dir, which is what the record now means. - - Not a retry mechanism, and it should not be read as one: both triggers close their own boundary - within a few statements — a `run-end` return lands on `finished`, which `resume` refuses, and the - per-epic boundary disappears as soon as `current_epic` advances. What changes is that - `bmad-loop diagnose` stops reporting sweeps that never happened, and that a crash in that narrow - window leaves the trigger recoverable rather than spent. A child that fails _after_ its run dir - exists still spends it — that run is resumable, so re-firing would duplicate it — and is - journaled `sweep-auto-failed` as before; the never-launched case is the new - `sweep-auto-not-started`, which keeps its own notification, because the loudest thing reaching it - is a config-integrity refusal. + was spent before anything had been attempted, so every way a child could decline to launch + consumed it — including a plain `git status` timeout, which fails closed and could silently burn + a run's one and only sweep. The tree check now runs ahead of the record and journals a `reason` + distinguishing a git fault from real local changes. Not a retry: what this buys is that + `bmad-loop diagnose` stops reporting sweeps that never ran. A child that fails after its run + directory exists still spends the trigger; the never-launched case journals + `sweep-auto-not-started` and keeps its own notification. - **A failed write can no longer truncate the sprint board, a story spec, your CLI settings or your policy file (#379).** Seven writers read a file, merged into it, and wrote the whole thing back diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index d5b519d6..2d80844c 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -832,7 +832,7 @@ def _claim_run_dir(run_dir: Path) -> None: ) from e -def _unwind_composition(project: Path, run_dir: Path) -> None: +def _unwind_composition(project: Path, run_dir: Path, journal: Journal) -> None: """Remove the run a failed ``compose_*`` had already published, so a launch that aborts partway leaves nothing behind. @@ -868,21 +868,45 @@ def _unwind_composition(project: Path, run_dir: Path) -> None: worse than what this replaces; ``force=True`` would trade that bounded cost for an unbounded one. - Best-effort, and that is the whole point of the suppression: the caller is - already unwinding an exception the operator has to see, and a cleanup failure - replacing it is the one outcome that must not happen. The enumerable failures - are :class:`runs.LiveSessionError` (the guard refusing), ``OSError`` (the - removal, or ``project.resolve()`` on a path the OS cannot canonicalize) and - ``RuntimeError`` (how ``Path.resolve`` reports a symlink loop below 3.13 — see - ``runs._discard_state_dir``). It is not written as that tuple because - ``delete_run`` reaches the multiplexer registry through - :func:`runs.live_session_may_be_ours`, an extension point an out-of-tree + Best-effort, and never raising: the caller is already unwinding an exception + the operator has to see, and a cleanup failure replacing it is the one outcome + that must not happen. The enumerable failures are :class:`runs.LiveSessionError` + (the guard refusing), ``OSError`` (the removal, or ``project.resolve()`` on a + path the OS cannot canonicalize) and ``RuntimeError`` (how ``Path.resolve`` + reports a symlink loop below 3.13 — see ``runs._discard_state_dir``). It is not + written as that tuple because ``delete_run`` reaches the multiplexer registry + through :func:`runs.live_session_may_be_ours`, an extension point an out-of-tree backend can make raise anything, so an enumerated list is one a third-party backend falsifies. ``Exception`` and not ``BaseException``: a - ``KeyboardInterrupt`` arriving during the cleanup still belongs to the - operator.""" - with suppress(Exception): + ``KeyboardInterrupt`` arriving during the cleanup still belongs to the operator. + + But not *silent*, which is a separate decision from not *raising* and was + previously conflated with it. "Repair writes must raise" (AGENTS.md) cannot be + honored literally here — raising is precisely what would swallow the launch + error — so the obligation it encodes is discharged by reporting instead. + Swallowing a failed unwind leaves exactly the resumable-looking ghost run this + function exists to prevent, and leaves it inferable only from the ABSENCE of an + effect: the operator reads the launch error, and nothing anywhere says the + cleanup after it did not happen.""" + try: runs.delete_run(project, run_dir) + except Exception as e: + detail = f"{type(e).__name__}: {e}" + print( + f"warning: could not remove the partially composed run {run_dir.name}: " + f"{detail} — it may look resumable; remove it with " + f"`bmad-loop delete {run_dir.name}`", + file=sys.stderr, + ) + # The journal lives INSIDE the run dir, so this lands for every failure that + # leaves one behind — the guard refusing, or a failed `rmtree` — which is + # also the only case where a ghost run is what the operator will find. When + # `_discard_state_dir` is instead what failed the dir is already gone, and + # `Journal.append` opens with "a" WITHOUT a mkdir, so it raises rather than + # resurrecting the run it just removed. Suppressed, and the stderr line + # above still carries the report. + with suppress(Exception): + journal.append("composition-unwind-failed", run_id=run_dir.name, error=detail) def compose_run( @@ -978,7 +1002,7 @@ def compose_run( else engine_cls(**common) # pyright: ignore[reportArgumentType] ) except BaseException: - _unwind_composition(project, run_dir) + _unwind_composition(project, run_dir, journal) raise return ComposedRun(engine=engine, run_id=run_id, run_dir=run_dir, state=state, journal=journal) @@ -1113,7 +1137,7 @@ def compose_sweep( if on_started is not None: on_started() except BaseException: - _unwind_composition(project, run_dir) + _unwind_composition(project, run_dir, journal) raise return ComposedRun(engine=engine, run_id=run_id, run_dir=run_dir, state=state, journal=journal) diff --git a/tests/test_runsetup.py b/tests/test_runsetup.py index 94fe5fa5..225ca0cc 100644 --- a/tests/test_runsetup.py +++ b/tests/test_runsetup.py @@ -15,6 +15,7 @@ """ import dataclasses +import shutil import types import pytest @@ -23,6 +24,7 @@ from bmad_loop import policy as policy_mod from bmad_loop import runs, runsetup from bmad_loop.adapters.profile import ProfileError +from bmad_loop.journal import Journal # A profile overlay carrying the whole launch surface the digest covers. It lives # under .bmad-loop/profiles/, inside the tree every driven session can write. @@ -573,3 +575,83 @@ def test_compose_sweep_refuses_a_run_id_that_already_exists(unwinding): trusted_config_digest="deadbeef", ) _assert_prior_run_untouched(unwinding, run_dir, state_dir) + + +def _run_compose_sweep(project, make_adapters, engine_cls=_NeverBuilt): + """Drive `compose_sweep` to whatever the injected `make_adapters` decides.""" + return runsetup.compose_sweep( + project=project, + paths=_fake_paths(project), + policy=policy_mod.loads(""), + run_id=RUN_ID, + prompting=False, + decisions_only=False, + max_bundles=None, + repeat=None, + max_cycles=None, + trigger="auto", + make_adapters=make_adapters, + sweep_engine_cls=engine_cls, + trusted_config_digest="deadbeef", + ) + + +def test_a_failed_unwind_is_reported_and_does_not_replace_the_launch_error( + unwinding, monkeypatch, capsys +): + """A cleanup that fails must be SURFACED, and must still not become the error + the operator reads. + + "Repair writes must raise" (AGENTS.md) cannot be honored literally here — + raising is exactly what would swallow the launch failure — so the obligation is + discharged by reporting. Suppressing silently left the resumable-looking ghost + run this unwind exists to prevent, detectable only as the ABSENCE of an effect. + + The `match=` is the load-bearing half: it pins that the SystemExit reaching the + operator is still `make_adapters`', not the cleanup's. A bare `pytest.raises` + would pass just as happily for a cleanup failure that replaced it.""" + + def boom(project, run_dir, *, force=False): + raise OSError(13, "Permission denied") + + monkeypatch.setattr(runs, "delete_run", boom) + with pytest.raises(SystemExit, match="not usable on this host"): + _run_compose_sweep(unwinding.project, unwinding.make_adapters) + + warning = capsys.readouterr().err + assert "warning: could not remove the partially composed run" in warning + assert RUN_ID in warning + assert f"bmad-loop delete {RUN_ID}" in warning + # The ghost the operator was just warned about is really there, and the run's + # own journal carries the record — which is where anyone investigating it looks. + run_dir = runs.run_dir_for(unwinding.project, RUN_ID) + assert run_dir.is_dir() + kinds = [e["kind"] for e in Journal(run_dir).entries()] + assert "composition-unwind-failed" in kinds + + +def test_a_failed_unwind_still_reports_when_the_run_dir_is_already_gone( + unwinding, monkeypatch, capsys +): + """The other failure mode, split into its own row: `delete_run` removes the run + dir and THEN raises (`_discard_state_dir` is the real site — it runs after the + `rmtree`). + + `Journal.append` opens with "a" and does not mkdir, so appending here raises + `FileNotFoundError`. Unsuppressed that would propagate out of the unwind and + replace the launch error — the one outcome this whole arm forbids — so the + suppression around the journal write is load-bearing and gets its own test. + The stderr report must still land, since it is now the only channel left.""" + + def boom(project, run_dir, *, force=False): + shutil.rmtree(run_dir) + raise RuntimeError("state dir removal failed") + + monkeypatch.setattr(runs, "delete_run", boom) + with pytest.raises(SystemExit, match="not usable on this host"): + _run_compose_sweep(unwinding.project, unwinding.make_adapters) + + warning = capsys.readouterr().err + assert "warning: could not remove the partially composed run" in warning + assert "state dir removal failed" in warning + assert not runs.run_dir_for(unwinding.project, RUN_ID).exists() From b88ad82412615f1abf5aafcb75a7cca6535fb256 Mon Sep 17 00:00:00 2001 From: t Date: Fri, 14 Aug 2026 23:10:47 -0700 Subject: [PATCH 07/11] feat: surface a refused auto-sweep outside the journal (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #501's closing note: a refused auto-sweep left no trace in the run's terminal status or the `--json` read model, only in the journal. Under `[sweep] auto = "run-end"` there is one trigger per run, and phase 3 established that an un-latched trigger is not re-asked once the run finishes — so for that shape visibility is the entire remedy, not a retry. `RunState` gains `sweeps_refused` (trigger -> reason), a sibling of `sweeps_triggered` rather than a widening of it: that list is the re-fire latch the #461 gate depends on, and a mapping there would silently degrade the per-element sanitizer loop in `diagnostics.py`. A trigger can legitimately appear in both — `failed` means a child started and then failed. The reason is a CLOSED vocabulary of short slugs (`not-started`, `failed`, `dirty`), never `str(e)`. `diagnostics.render_json`/`render_markdown` run the whole dump through `sanitize.guard`, which *raises* `LeakDetected` on a home-path hit rather than redacting it — genuine PII never auto-repairs — so a free-form reason would make `bmad-loop diagnose` fail outright on exactly the runs worth dumping, and `looks_like_identifier` would blank it anyway. Surfaced on four paths: `Engine.summary()`/`RunSummary.render()` following the `CRASHED:`/`PAUSED:` idiom; `documents.status_document`; the human `cmd_status` print; and both `diagnose` renders, filtering key AND value through `looks_like_identifier`. The follow-up names `bmad-loop sweep` *and* a clean worktree, because `cmd_sweep` hard-refuses an unclean tree — a `dirty` refusal would otherwise send the operator straight into a second refusal. No new call site was needed for the notification: `_run_inner` already pipes `summary.render()` into `gates.notify`, so the line reaches the ATTENTION file and the desktop toast too. `RunSummary` carries the field as `tuple[tuple[str, str], ...]`, not the dict `RunState` holds it in, because that dataclass is `frozen=True`: a dict field leaves the "snapshot" mutable through its own container and — silently — makes every `RunSummary` unhashable, since frozen+eq synthesizes `__hash__` from the field tuple. Nothing hashes one today, which is exactly why it would go unnoticed; it is pinned by an assertion. `"sweeps_refused"` is always present in the status document, `{}` when nothing was refused, so a consumer cannot confuse "swept fine" with "old state.json". No version bump: machine.py's contract is that "Evolution is additive-only: new fields may appear, but anything breaking — removing or renaming a field, changing a type or the meaning of a value — bumps that command's version." A new key is additive, so `STATUS_SCHEMA_VERSION` stays at 1. Deliberately NOT recorded for `sweep-auto-suppressed`: that arm returns ahead of the latch, so a resume can still fire the trigger and the entry would go stale. Pinned by an inverse ablation in `test_maybe_auto_sweep_suppressed_when_graceful_stop_pending`. No exit code moves — `cmd_run` returns 0 for crashed and paused runs alike, and `ExitCode` allocation is closed. The git-fault arm folds in with the dirty arm under one `dirty` slug: the journal keeps `git-error` vs `dirty` apart for forensics, while the operator-facing next action is identical either way. Every new assertion was ablated (17 axes, each restored with `cp`): the four recording arms redden one test each and are disjoint; the two exclusions (RunStopped, suppressed) are inverse ablations; the render block and its clean-worktree clause each fail alone. Two records were corrected against measurement rather than left as first written — aliasing the state dict reddens EIGHT tests, not one (six die unpacking bare dict keys in `render()`), and the diagnostics key/value filters were graded by reading the assertion diff, since both redden the same single test and only the diff shows the other two seeded entries staying identical. The TUI is untouched: it reads state.json directly, not `status_document`, and references neither field. --- CHANGELOG.md | 11 ++++ src/bmad_loop/cli.py | 4 ++ src/bmad_loop/diagnostics.py | 20 +++++++ src/bmad_loop/documents.py | 6 ++ src/bmad_loop/engine.py | 60 ++++++++++++++++++++ src/bmad_loop/model.py | 20 +++++++ tests/test_cli.py | 68 ++++++++++++++++++++++ tests/test_diagnostics.py | 56 +++++++++++++++++- tests/test_engine.py | 106 +++++++++++++++++++++++++++++++++-- tests/test_model.py | 48 +++++++++++++++- 10 files changed, 393 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b258af7a..748247df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ breaking changes may land in a minor release. ## [Unreleased] +### Added + +- **A refused auto-sweep is now visible outside the journal (#501).** A run whose deferred-work + sweep was refused ended looking exactly like one that swept, and under `[sweep] auto = "run-end"` + there is one trigger per run that is never re-asked once the run finishes — so the journal was + the only trace. Runs now record `sweeps_refused` (trigger → reason), surfaced by the end-of-run + summary, `bmad-loop status`, `status --json` and `bmad-loop diagnose`; the follow-up they name + is `bmad-loop sweep`, which needs a clean worktree. The reason is a fixed slug — `not-started`, + `failed` or `dirty` — never an exception message, which `diagnose` would refuse to emit at all. + The `--json` key is additive and always present, so `STATUS_SCHEMA_VERSION` is unchanged. + ### Changed - **Files the orchestrator replaces by name now land at `0600`.** Those writes pass diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index e664caa0..0fff9817 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -2981,6 +2981,10 @@ def cmd_status(args: argparse.Namespace) -> int: print("status: in progress — graceful stop pending (will stop after the current item)") else: print("status: in progress (or interrupted)") + if state.sweeps_refused: + detail = ", ".join(f"{trigger} ({why})" for trigger, why in state.sweeps_refused.items()) + print(f"auto-sweep not run: {detail} — deferred work is untouched") + print(" run `bmad-loop sweep` with a clean worktree") raw_total, weighted_total, weight = run_token_totals(state) if raw_total: print( diff --git a/src/bmad_loop/diagnostics.py b/src/bmad_loop/diagnostics.py index edc0cb4c..e481fdea 100644 --- a/src/bmad_loop/diagnostics.py +++ b/src/bmad_loop/diagnostics.py @@ -232,6 +232,7 @@ class RunDiag: current_epic: int | None sweep_cycle: int sweeps_triggered: list[str] + sweeps_refused: dict[str, str] plugin_shared_keys: int policy: dict n_tasks: int @@ -596,6 +597,17 @@ def collect_run(run_dir: Path, *, pseudo: sanitize.Pseudonymizer, cap: int) -> R s if sanitize.looks_like_identifier(str(s)) else "" for s in state.sweeps_triggered ], + # BOTH halves are filtered. The value is a closed SWEEP_REFUSED_* slug by + # construction, but the key is a trigger string off state.json — the same + # untrusted footing as sweeps_triggered above — and a hand-edited or + # foreign state file must not be able to route a home path into a report + # that `sanitize.guard` would then refuse to emit at all. + sweeps_refused={ + (k if sanitize.looks_like_identifier(str(k)) else ""): ( + v if sanitize.looks_like_identifier(str(v)) else "" + ) + for k, v in state.sweeps_refused.items() + }, plugin_shared_keys=len(state.plugin_shared), policy=_scrub_policy(state.policy_snapshot), n_tasks=len(tasks), @@ -645,6 +657,7 @@ def _unreadable_run(run_dir: Path, err: Exception) -> RunDiag: current_epic=None, sweep_cycle=0, sweeps_triggered=[], + sweeps_refused={}, plugin_shared_keys=0, policy={}, n_tasks=0, @@ -745,6 +758,13 @@ def render_markdown( out.append(_fmt_kv("epic / sweep_cycle", f"{r.current_epic} / {r.sweep_cycle}")) if r.sweeps_triggered: out.append(_fmt_kv("sweeps_triggered", ", ".join(f"`{s}`" for s in r.sweeps_triggered))) + if r.sweeps_refused: + out.append( + _fmt_kv( + "sweeps_refused", + ", ".join(f"`{k}`: {v}" for k, v in r.sweeps_refused.items()), + ) + ) out.append(_fmt_kv("tasks", r.n_tasks)) out.append(_fmt_kv("phase histogram", _dict_inline(r.phase_histogram))) out.append(_fmt_kv("token totals", _dict_inline(r.token_totals))) diff --git a/src/bmad_loop/documents.py b/src/bmad_loop/documents.py index 2c14a09f..484c17e4 100644 --- a/src/bmad_loop/documents.py +++ b/src/bmad_loop/documents.py @@ -329,6 +329,12 @@ def status_document(state: RunState, *, graceful_stop_pending: bool = False) -> "weighted": weighted_total, }, "adapters": adapters, + # auto-sweep triggers the run did not deliver, trigger -> reason slug + # (model.SWEEP_REFUSED_*). Always present, `{}` when nothing was refused: + # a key that appears only on the failing run makes "absent" ambiguous + # between "swept fine" and "old state.json". Additive per machine.py, so + # STATUS_SCHEMA_VERSION does not move. + "sweeps_refused": dict(state.sweeps_refused), "tasks": tasks, } diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 49a2c1db..098251e0 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -44,6 +44,9 @@ PAUSE_ESCALATION, PAUSE_SPEC_APPROVAL, PAUSE_STORY_GATE, + SWEEP_REFUSED_DIRTY, + SWEEP_REFUSED_FAILED, + SWEEP_REFUSED_NOT_STARTED, Phase, RunState, SessionRecord, @@ -158,6 +161,16 @@ class RunSummary: awaiting_operator: int = 0 crashed: bool = False crash_error: str | None = None + # auto-sweep triggers this run did not deliver, as (trigger, SWEEP_REFUSED_*) + # pairs (#501). Defaulted for the same reason as awaiting_operator: empty is + # the honest value on every run that refused nothing. + # + # A tuple of pairs, NOT the dict `RunState` holds it in, because this class is + # `frozen=True`: a dict field leaves the "snapshot" mutable through its own + # container, and — silently — makes every RunSummary unhashable, since + # frozen+eq synthesizes `__hash__` from the field tuple. Nothing hashes one + # today, which is exactly why that would go unnoticed. `summary()` converts. + sweeps_refused: tuple[tuple[str, str], ...] = () def render(self) -> str: # Lead with weighted (what spend actually costs) and name both units: @@ -185,6 +198,19 @@ def render(self) -> str: lines.append(f"CRASHED: {self.crash_error}") if self.paused: lines.append(f"PAUSED: {self.paused_reason}") + # Appended only when it fired, like `parked` above. Under + # `[sweep] auto = "run-end"` there is exactly one trigger per run and it + # is never re-asked once the run finishes (see `_maybe_auto_sweep`), so + # this line IS the remedy — the refusal is otherwise journal-only, and + # the operator never learns the deferred work went untouched. The clean + # worktree is named because `cmd_sweep` hard-refuses an unclean tree: + # without it the follow-up lands the operator in a second refusal. + if self.sweeps_refused: + detail = ", ".join(f"{trigger} ({why})" for trigger, why in self.sweeps_refused) + lines.append( + f"SWEEP NOT RUN: {detail} — deferred work is untouched; " + "run `bmad-loop sweep` with a clean worktree" + ) return "\n".join(lines) @@ -819,6 +845,10 @@ def summary(self) -> RunSummary: weighted_tokens=sum(t.tokens.weighted_total(weight) for t in tasks), crashed=self.state.crashed, crash_error=self.state.crash_error, + # Snapshotted, not aliased: this dict is still live on the engine's + # state, and the tuple makes the copy structural rather than a + # convention a later edit could drop. + sweeps_refused=tuple(self.state.sweeps_refused.items()), ) def _remaining_estimate(self) -> int | None: @@ -5635,6 +5665,25 @@ def _escalate(self, task: StoryTask, reason: str) -> None: self._save() raise RunPaused(reason, PAUSE_ESCALATION, task.story_key) + def _record_sweep_refusal(self, trigger: str, reason: str) -> None: + """Record, durably, that this trigger's auto-sweep did not deliver. + + The remedy for #501's closing note: every refusal below was journal-only, + so a run whose one sweep trigger was refused finished looking exactly like + a run that swept — nothing in ``summary().render()``, ``status`` or + ``status --json`` said otherwise, and under ``auto = "run-end"`` there is + no later ask to notice the gap. + + ``reason`` must be one of the ``SWEEP_REFUSED_*`` slugs, never a formatted + exception — see their definition in ``model.py`` for why a free-form + string breaks ``bmad-loop diagnose`` outright rather than being redacted. + + Written before the arm's journal/notify calls, mirroring ``latch``: the + durable record is the point, and it must not be lost to an OSError from a + journal append.""" + self.state.sweeps_refused[trigger] = reason + self._save() + def _maybe_auto_sweep(self, kind: str, trigger: str) -> None: """Run a child deferred-work sweep when policy [sweep].auto matches. The child is its own resumable run; a paused or failed child is @@ -5721,6 +5770,7 @@ def _maybe_auto_sweep(self, kind: str, trigger: str) -> None: # arm this one is transient-reachable: `_run_git` reports a # `subprocess.TimeoutExpired` as GitError (verify.py), so a slow # `git status` used to permanently spend this run's sweep trigger. + self._record_sweep_refusal(trigger, SWEEP_REFUSED_DIRTY) self.journal.append( "sweep-auto-skipped-dirty", trigger=trigger, reason="git-error", error=str(e) ) @@ -5728,6 +5778,7 @@ def _maybe_auto_sweep(self, kind: str, trigger: str) -> None: if not clean: # should not happen at these call sites (everything committed or # reset); refuse rather than sweep on top of stray changes + self._record_sweep_refusal(trigger, SWEEP_REFUSED_DIRTY) self.journal.append("sweep-auto-skipped-dirty", trigger=trigger, reason="dirty") return @@ -5743,6 +5794,13 @@ def latch() -> None: if latched: return latched = True + # Clear any refusal this trigger carries from an earlier ask, so the + # two records cannot contradict each other. Reachable only through the + # narrow crash window the docstring above bounds — a per-epic trigger + # refused, the process dying before the boundary closed, and a resume + # re-asking it. Not a retry path; a guard against a stale claim if one + # happens. The `failed` arm re-records after this, by design. + self.state.sweeps_refused.pop(trigger, None) self.state.sweeps_triggered.append(trigger) self._save() @@ -5753,6 +5811,7 @@ def latch() -> None: raise # a stop is not a failed child — let the owner record it except (Exception, SystemExit) as e: # child must never break the parent if latched: + self._record_sweep_refusal(trigger, SWEEP_REFUSED_FAILED) self.journal.append("sweep-auto-failed", trigger=trigger, error=str(e)) gates.notify(self.policy, self.run_dir, "auto sweep failed", f"{trigger}: {e}") else: @@ -5762,6 +5821,7 @@ def latch() -> None: # wording rather than none: the loudest raise that lands here is # the #461 config-integrity refusal, a security event that must not # go quiet just because it stopped being permanent. + self._record_sweep_refusal(trigger, SWEEP_REFUSED_NOT_STARTED) self.journal.append("sweep-auto-not-started", trigger=trigger, error=str(e)) gates.notify( self.policy, self.run_dir, "auto sweep did not start", f"{trigger}: {e}" diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index 68d966a4..cc537f68 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -63,6 +63,18 @@ class Phase(StrEnum): PAUSE_PLAN_CHECKPOINT = "plan-checkpoint" PAUSE_STORY_CHECKPOINT = "story-checkpoint" +# Reasons recorded in RunState.sweeps_refused (trigger -> reason). A CLOSED +# vocabulary of short slugs, deliberately not a formatted exception: `bmad-loop +# diagnose` renders run state through `sanitize.guard`, which *raises* +# LeakDetected on a home-path hit rather than redacting it (genuine PII never +# auto-repairs — sanitize.py). A free-form `str(e)` here would therefore make the +# dump fail outright on exactly the runs worth dumping, and the per-value +# `looks_like_identifier` filter would blank it anyway. Add a slug, never a +# message. +SWEEP_REFUSED_NOT_STARTED = "not-started" # the launch raised before a child existed +SWEEP_REFUSED_FAILED = "failed" # a child started, then failed +SWEEP_REFUSED_DIRTY = "dirty" # the worktree was unclean, or `git status` faulted + @dataclass class TokenUsage: @@ -546,6 +558,12 @@ class RunState: # auto-sweep triggers already fired this run (e.g. "epic-1", "run-end"); # guards re-fire on resume sweeps_triggered: list[str] = field(default_factory=list) + # auto-sweep triggers this run did NOT deliver, trigger -> SWEEP_REFUSED_*. + # Kept apart from sweeps_triggered rather than folded into it: that list is + # the re-fire latch, and widening it to a mapping would silently degrade the + # per-element sanitizer loop in diagnostics.py. A trigger may appear in both + # (SWEEP_REFUSED_FAILED = a child that started and then failed). + sweeps_refused: dict[str, str] = field(default_factory=dict) # worktree-isolation mode only: the branch every unit merges back into, # resolved once at run start (default = the branch checked out then) and # pinned so resume keeps targeting the same branch. @@ -614,6 +632,7 @@ def to_dict(self) -> dict[str, Any]: "spec_folder": self.spec_folder, "sweep_cycle": self.sweep_cycle, "sweeps_triggered": self.sweeps_triggered, + "sweeps_refused": self.sweeps_refused, "target_branch": self.target_branch, "plugin_shared": self.plugin_shared, "tasks": {k: t.to_dict() for k, t in self.tasks.items()}, @@ -643,6 +662,7 @@ def from_dict(cls, d: dict[str, Any]) -> "RunState": spec_folder=str(d.get("spec_folder", "")), sweep_cycle=int(d.get("sweep_cycle", 1)), sweeps_triggered=[str(s) for s in d.get("sweeps_triggered", [])], + sweeps_refused={str(k): str(v) for k, v in d.get("sweeps_refused", {}).items()}, target_branch=str(d.get("target_branch", "")), plugin_shared=dict(d.get("plugin_shared", {})), tasks={k: StoryTask.from_dict(t) for k, t in d.get("tasks", {}).items()}, diff --git a/tests/test_cli.py b/tests/test_cli.py index 9737b584..0c10d237 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1317,6 +1317,74 @@ def test_status_reports_a_parked_story_on_both_surfaces(project, capsys): assert parked_line.index("dev×") == done_line.index("dev×") +def _run_with_refusals(project, refusals): + """A finished run carrying `sweeps_refused`. Written through load/mutate/save + rather than by widening `_make_run_with_tokens`, which a dozen weight tests + share and none of them need this field.""" + from bmad_loop.journal import load_state, save_state + from bmad_loop.model import Phase, StoryTask + + done = StoryTask(story_key="1-1-login", epic=1, phase=Phase.DONE) + run_dir = _make_run_with_tokens(project, {"1-1-login": done}, weight=0.1) + state = load_state(run_dir) + state.sweeps_refused.update(refusals) + save_state(run_dir, state) + return run_dir + + +def test_status_reports_a_refused_auto_sweep_on_both_surfaces(project, capsys): + """#501's closing note: the refusal lived only in the journal, so neither + surface distinguished a run whose deferred-work sweep was refused from one + that swept cleanly. Under `[sweep] auto = "run-end"` the trigger is not + re-asked once the run finishes, so surfacing it IS the fix. + + Additive per machine.py — a new key is not a breaking change — so + STATUS_SCHEMA_VERSION does not move, and this asserts it stayed at 1. + + The text side names the clean worktree because `cmd_sweep` hard-refuses an + unclean tree; without it the operator's next command is a second refusal. + + Ablation: delete the `"sweeps_refused"` entry from `documents.status_document` + and the json asserts fail; delete the `if state.sweeps_refused:` block in + `cmd_status` and only the text asserts fail. Disjoint — the read model and + the human surface are separate code paths.""" + _run_with_refusals(project, {"run-end": "dirty"}) + + doc = _status_json(project, capsys) + assert doc["sweeps_refused"] == {"run-end": "dirty"} + assert doc["schema_version"] == 1 # additive: no consumer breaks + + assert cli.main(["status", "--project", str(project.project)]) == 0 # exit code unchanged + out = capsys.readouterr().out + assert "auto-sweep not run: run-end (dirty)" in out + assert "bmad-loop sweep" in out and "clean worktree" in out + + +def test_status_json_carries_sweeps_refused_even_when_nothing_was_refused(project, capsys): + """Always present, `{}` on a clean run. A key that appears only on the failing + run leaves "absent" ambiguous between "swept fine" and "state.json predates + #501", and a consumer cannot tell those apart — which is the whole point of a + document it can rely on. + + The text line is the opposite call and follows the `awaiting operator` idiom: + printed only when it fires, because a standing "auto-sweep not run: none" + trains the reader to skip the line that matters. + + Ablation for the text half: drop the `if state.sweeps_refused:` guard (print + unconditionally) and the last assert fails while the json assert stays green.""" + from bmad_loop.journal import load_state + from bmad_loop.model import Phase, StoryTask + + done = StoryTask(story_key="1-1-login", epic=1, phase=Phase.DONE) + run_dir = _make_run_with_tokens(project, {"1-1-login": done}, weight=0.1) + assert load_state(run_dir).sweeps_refused == {} # the run really refused nothing + + assert _status_json(project, capsys)["sweeps_refused"] == {} + + assert cli.main(["status", "--project", str(project.project)]) == 0 + assert "auto-sweep not run" not in capsys.readouterr().out + + def test_status_json_stories_mode_is_pure_json(project, capsys): """--json must skip every text trailer (stories board, backlog, decisions nudge) — the stories-mode board would otherwise corrupt the document.""" diff --git a/tests/test_diagnostics.py b/tests/test_diagnostics.py index 3b2817ac..0757b450 100644 --- a/tests/test_diagnostics.py +++ b/tests/test_diagnostics.py @@ -65,13 +65,24 @@ ] -def _seed_run(root, run_id="20260627-120000-aaaa", *, extra_journal=None, sweeps_triggered=()): +def _seed_run( + root, + run_id="20260627-120000-aaaa", + *, + extra_journal=None, + sweeps_triggered=(), + sweeps_refused=None, +): """Build a run dir loaded with canaries in every readable sink. ``sweeps_triggered`` seeds a routing gap the MARKDOWN report can reach: the collector passes identifier-shaped entries through verbatim, and the report renders them inline. (``extra_journal`` seeds a gap only the JSON document reaches — markdown renders journal aggregates, never per-entry fields.) + + ``sweeps_refused`` is the #501 sibling and reaches both renders the same way, + except that it is a mapping — so a seed can aim a canary at the key half, the + value half, or both independently. """ run_dir = root / ".bmad-loop" / "runs" / run_id @@ -128,6 +139,7 @@ def _seed_run(root, run_id="20260627-120000-aaaa", *, extra_journal=None, sweeps plugin_shared={"unity": {"creds": SECRET_AWS}}, tasks={STORY_KEY: task}, sweeps_triggered=list(sweeps_triggered), + sweeps_refused=dict(sweeps_refused or {}), ) save_state(run_dir, state) @@ -203,6 +215,48 @@ def test_known_safe_values_survive(project): assert "input_tokens" in combined # token count keys survive +def test_sweeps_refused_redacts_both_halves(project): + """#501: `sweeps_refused` is a mapping, so it has two redaction surfaces. + + The value is a closed `SWEEP_REFUSED_*` slug wherever the orchestrator wrote + it — but neither half is re-validated on load, and the key is a trigger string + off state.json, the same untrusted footing as `sweeps_triggered`. This matters + more than a usual scrub: a home path reaching `sanitize.guard` is not redacted + there, it RAISES `LeakDetected` and the whole dump is refused. Filtering here + is what keeps a malformed run diagnosable at all. + + The structure is asserted before rendering on purpose. Under ablation the + render would raise `LeakDetected` rather than fail an assert, which says "a + dump was refused" and not which half leaked. + + Ablation, three axes, verified by reading the diff and not just the red: + drop the key's `looks_like_identifier` branch and ONLY the path-shaped-key + entry differs (pytest reports the other two as identical); drop the value's + and only the `run-end` entry does; delete the markdown `sweeps_refused` row + and only the render asserts at the end fail.""" + run_dir = _seed_run( + project.project, + sweeps_refused={HOME_PATH: "dirty", "run-end": HOME_PATH, "epic-1": "not-started"}, + ) + pseudo = sanitize.Pseudonymizer() + diag = diagnostics.collect([run_dir], pseudo=pseudo, project=ANY_PROJECT) + + (r,) = diag.runs + assert r.sweeps_refused == { + "": "dirty", # path-shaped KEY + "run-end": "", # path-shaped VALUE + "epic-1": "not-started", # the well-formed pair is untouched + } + + md = diagnostics.render_markdown(diag, pseudo=pseudo) + js = diagnostics.render_json(diag, pseudo=pseudo) + assert HOME_PATH not in md + js + # and the row actually renders — a field collected but never surfaced would + # satisfy every leak assertion above while shipping nothing. + assert "**sweeps_refused:**" in md and "`epic-1`: not-started" in md + assert json.loads(js)["runs"][0]["sweeps_refused"]["epic-1"] == "not-started" + + def test_env_names_the_platform_and_the_win32_on_wsl_path_verdict(project, monkeypatch): """#332: `platform.system()` says "Windows" for both a native shell and a WSL interop launch, so the raw sys.platform token plus the verdict are what explain diff --git a/tests/test_engine.py b/tests/test_engine.py index cf0e4c00..bbb3e55f 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2,6 +2,7 @@ import dataclasses import hashlib +import json import os import re import signal @@ -42,6 +43,9 @@ PAUSE_ESCALATION, PAUSE_SPEC_APPROVAL, PAUSE_STORY_GATE, + SWEEP_REFUSED_DIRTY, + SWEEP_REFUSED_FAILED, + SWEEP_REFUSED_NOT_STARTED, Phase, RunState, SessionRecord, @@ -1235,6 +1239,62 @@ def test_run_summary_render_names_parked_stories_only_when_there_are_any(project assert "1 awaiting operator" in engine.summary().render() +def test_run_summary_projects_and_renders_a_refused_auto_sweep(project): + """#501's closing note: a refused auto-sweep was journal-only, so the run's + terminal output was byte-identical to a run that swept. Under + `[sweep] auto = "run-end"` there is one trigger per run and it is not re-asked + once the run finishes, so this line is the whole remedy. + + The clean-worktree clause is not decoration: `cmd_sweep` hard-refuses an + unclean tree, so a `dirty` refusal whose follow-up omitted it would walk the + operator straight into a second refusal. + + Ablation, three disjoint axes: (a) drop `sweeps_refused=` from `summary()` and + both the projection and the render assert fail while the absence assert stays + green; (b) delete the `if self.sweeps_refused:` block in `render()` and only + the render asserts fail; (c) delete the `sweep` clause from that block's text + and only the last assert fails.""" + engine = _cache_heavy_engine(project, snapshot_weight=0.5, live_weight=0.5, usage=TokenUsage()) + assert "SWEEP NOT RUN" not in engine.summary().render() # absent when nothing refused + + engine.state.sweeps_refused["run-end"] = SWEEP_REFUSED_DIRTY + + summary = engine.summary() + assert summary.sweeps_refused == (("run-end", SWEEP_REFUSED_DIRTY),) + rendered = summary.render() + assert "SWEEP NOT RUN: run-end (dirty)" in rendered + assert "bmad-loop sweep" in rendered and "clean worktree" in rendered + + +def test_run_summary_snapshots_rather_than_aliases_the_refusal_record(project): + """`summary()` is a pure projection of `self.state` (see its docstring), and a + snapshot that keeps mutating with the engine is not one. + + `RunSummary` is `frozen=True`, so the field is a tuple of pairs rather than + the dict `RunState` holds — which buys two things a dict cannot. The copy is + structural, not a `dict(...)` convention a later edit could quietly drop; and + the class stays hashable, since frozen+eq synthesizes `__hash__` over the + fields and a dict field makes that raise. Nothing hashes a RunSummary today — + precisely why that regression would ship unnoticed — so it is pinned here. + + Ablation: change `sweeps_refused=tuple(self.state.sweeps_refused.items())` to + `sweeps_refused=self.state.sweeps_refused`. This reddens EIGHT tests, not one + — recorded as measured, not as first guessed. Only two fail on the snapshot + claim; the other six die in `render()` with `ValueError: too many values to + unpack`, because iterating a dict yields bare keys and the line unpacks pairs. + That is the same "a dict yields keys" degradation that keeps this field out of + `sweeps_triggered`, and it means the ablation is loud rather than subtle. This + test is the one that grades the snapshot semantics specifically.""" + engine = _cache_heavy_engine(project, snapshot_weight=0.5, live_weight=0.5, usage=TokenUsage()) + engine.state.sweeps_refused["run-end"] = SWEEP_REFUSED_DIRTY + summary = engine.summary() + + engine.state.sweeps_refused["epic-1"] = SWEEP_REFUSED_FAILED + + assert summary.sweeps_refused == (("run-end", SWEEP_REFUSED_DIRTY),) + assert hash(summary) # frozen means hashable; a dict field would TypeError + + # ---------------------------------------- awaiting-operator park path (#335) @@ -7732,7 +7792,12 @@ def started_then_failed(trigger, *, started): assert summary.done == 1 and engine.state.finished # parent unaffected assert calls == ["run-end"] - assert load_state(engine.run_dir).sweeps_triggered == ["run-end"] + saved = load_state(engine.run_dir) + assert saved.sweeps_triggered == ["run-end"] + # The one shape that lands in BOTH records, and the reason `sweeps_refused` + # is a sibling of the latch rather than a widening of it: the trigger really + # was spent (a resumable child exists), and the sweep really did not deliver. + assert saved.sweeps_refused == {"run-end": SWEEP_REFUSED_FAILED} journal = (engine.run_dir / "journal.jsonl").read_text() assert "sweep-auto-failed" in journal # it ran, and then it failed assert "sweep-auto-not-started" not in journal @@ -7854,7 +7919,13 @@ def test_auto_sweep_skips_a_dirty_tree_and_keeps_the_trigger(project): assert calls == [] assert [e["reason"] for e in _skipped_dirty(engine)] == ["dirty"] - assert load_state(engine.run_dir).sweeps_triggered == [] + saved = load_state(engine.run_dir) + assert saved.sweeps_triggered == [] + # ...and, since #501's visibility phase, the refusal is durable rather than + # journal-only. Third ablation axis: delete the `_record_sweep_refusal` call + # from the `if not clean:` block and this line fails while the two above stay + # green — they grade the refusal, this one grades the record of it. + assert saved.sweeps_refused == {"run-end": SWEEP_REFUSED_DIRTY} def test_auto_sweep_skips_a_git_fault_and_keeps_the_trigger(project, monkeypatch): @@ -7887,7 +7958,13 @@ def timing_out(cmd, **kwargs): skipped = _skipped_dirty(engine) assert [e["reason"] for e in skipped] == ["git-error"] assert "timed out" in skipped[0]["error"] # the fault is named, not swallowed - assert load_state(engine.run_dir).sweeps_triggered == [] + saved = load_state(engine.run_dir) + assert saved.sweeps_triggered == [] + # The durable record folds this arm in with the dirty twin: the journal keeps + # `git-error` vs `dirty` apart for forensics, while the operator-facing slug + # answers only "the sweep did not run" — and its next action (`bmad-loop + # sweep` on a clean tree) is the same either way. + assert saved.sweeps_refused == {"run-end": SWEEP_REFUSED_DIRTY} def test_auto_sweep_system_exit_does_not_kill_the_parent(project): @@ -7931,7 +8008,13 @@ def exiting(trigger, *, started): journal = (engine.run_dir / "journal.jsonl").read_text() assert "sweep-auto-not-started" in journal and "not usable on this host" in journal assert "run-complete" in journal - assert load_state(engine.run_dir).sweeps_triggered == [] + saved = load_state(engine.run_dir) + assert saved.sweeps_triggered == [] + # The slug, never `str(e)`: the SystemExit message above is free-form operator + # text and could carry a home path, which `sanitize.guard` refuses to redact — + # it raises, taking the whole `diagnose` dump down with it. See model.py. + assert saved.sweeps_refused == {"run-end": SWEEP_REFUSED_NOT_STARTED} + assert "not usable on this host" not in json.dumps(saved.to_dict()) def test_auto_sweep_run_stopped_stops_the_parent(project, monkeypatch): @@ -7970,6 +8053,11 @@ def stopping(trigger, *, started): assert saved.stopped is True assert not saved.finished # the whole point: a stop must not read as a finish assert saved.sweeps_triggered == ["run-end"] # it ran; the stop does not un-spend it + # And it is not a refusal either — the `except RunStopped: raise` arm is ahead + # of both recording arms. INVERSE ablation (an absence): add a + # `_record_sweep_refusal(trigger, SWEEP_REFUSED_FAILED)` to that arm and this + # line fails, while the journal asserts below stay green. + assert saved.sweeps_refused == {} assert killed == ["test-run"] journal = (engine.run_dir / "journal.jsonl").read_text() assert "run-stop" in journal @@ -8722,6 +8810,16 @@ def test_maybe_auto_sweep_suppressed_when_graceful_stop_pending(project): assert "sweep-auto-suppressed" in kinds assert "sweep-auto-trigger" not in kinds assert "run-end" not in engine.state.sweeps_triggered # nothing started, nothing spent + # ...and deliberately NOT in `sweeps_refused` either, unlike every other + # non-delivering arm. This return sits AHEAD of the latch, so a resume can + # still fire the trigger — recording a refusal here would go stale the moment + # it does, which is exactly the dishonesty the record exists to prevent. The + # operator already has a louder signal: they asked for the stop. + # + # INVERSE ablation (the guard is an absence — deleting code cannot reproduce + # it): add `self._record_sweep_refusal(trigger, SWEEP_REFUSED_DIRTY)` above + # the suppressed journal append and this line fails alone. + assert engine.state.sweeps_refused == {} def test_pause_wins_over_pending_graceful_stop(project, monkeypatch): diff --git a/tests/test_model.py b/tests/test_model.py index aea589d8..43573472 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -4,7 +4,15 @@ import pytest -from bmad_loop.model import Phase, RunState, SessionRecord, StoryTask, TokenUsage +from bmad_loop.model import ( + SWEEP_REFUSED_DIRTY, + SWEEP_REFUSED_NOT_STARTED, + Phase, + RunState, + SessionRecord, + StoryTask, + TokenUsage, +) def _state(**kw) -> RunState: @@ -38,6 +46,44 @@ def test_run_state_stories_fields_default_when_absent_from_dict(): assert back.source == "sprint-status" and back.spec_folder == "" +def test_sweeps_refused_round_trips(): + """#501's visibility record: trigger -> a closed SWEEP_REFUSED_* slug. + + Kept apart from `sweeps_triggered` deliberately — that list is the re-fire + latch, and a refusal must not spend it. The two are independent here.""" + state = _state() + state.sweeps_refused["run-end"] = SWEEP_REFUSED_DIRTY + state.sweeps_refused["epic-1"] = SWEEP_REFUSED_NOT_STARTED + back = RunState.from_dict(json.loads(json.dumps(state.to_dict()))) + assert back.sweeps_refused == {"run-end": "dirty", "epic-1": "not-started"} + assert back.sweeps_triggered == [] + + +def test_sweeps_refused_defaults_when_absent_from_dict(): + """A state.json written before #501 carries no `sweeps_refused` key at all. + + Ablation: change from_dict's `d.get("sweeps_refused", {})` to + `d["sweeps_refused"]`. This test fails with KeyError; the round-trip above + stays green, because to_dict always writes the key. The two tests cover + disjoint halves — neither substitutes for the other.""" + d = _state().to_dict() + del d["sweeps_refused"] + assert RunState.from_dict(d).sweeps_refused == {} + + +def test_sweeps_refused_coerces_both_halves(): + """Both halves are coerced with str(). The value is the JSON-reachable one — + a number survives a dumps/loads round trip as a number — and the key is + reachable from a hand-edited or foreign state file. Coercion here is the + precondition for diagnostics' `looks_like_identifier` filter, which is typed + over strings on both sides. + + Ablation: drop either `str()` in from_dict and the matching half fails.""" + d = _state().to_dict() + d["sweeps_refused"] = {1: 2} + assert RunState.from_dict(d).sweeps_refused == {"1": "2"} + + def test_attach_session_usage_folds_usage_into_record_and_totals(): task = _task_with_session() task.attach_session_usage("1-1-a-dev-1", TokenUsage(input_tokens=10, output_tokens=5)) From c5ae67f4e38154be7fa3d4fe98899b4416a66baa Mon Sep 17 00:00:00 2001 From: t Date: Sat, 15 Aug 2026 08:57:05 -0700 Subject: [PATCH 08/11] docs: correct the claims the auto-sweep fix invalidates (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FEATURES.md stated flatly that "a failed/paused child sweep never interrupts the parent run" — which was false in one direction before this branch (a child `SystemExit` ended the parent at exit 1) and is deliberately false in another after it (a stop or Ctrl-C delivered through the child propagates, so a parent stays stoppable mid-sweep). It also said nothing about `sweeps_refused`, which is now the operator-facing record of a refusal. `resolve_profiles` argued that a lost race round costs the writer nothing because "the next auto-sweep trigger deals again". The repeat is the writer's, never the orchestrator's: a refused trigger is now left unspent but nothing re-asks it, and under `[sweep] auto = "run-end"` a run has exactly one. It is `per-epic` that hands out the further rounds. `_maybe_auto_sweep`'s lead sentence keeps the contract but names its two exceptions, so a reader who stops at the lead does not carry away the claim the four paragraphs below it spend their length qualifying. --- docs/FEATURES.md | 3 ++- src/bmad_loop/engine.py | 5 ++++- src/bmad_loop/runsetup.py | 10 +++++++--- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 682bc0cb..e4bdc5c1 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -126,7 +126,8 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Bundles run the full pipeline (dev `--dw-bundle` → review → verify → commit); the review gate checks every bundle entry is `status: done`. - Interactive decision walkthrough (build / close / keep-open per option, with a recommendation); answers written back as `decision:` lines. Unattended runs leave decisions open. - Answer skipped/missed decisions out of band with `bmad-loop decisions` (or `d` in the TUI): reconstructed from past triage output, saved to `.bmad-loop/decisions.json`, and consumed by the next sweep with no re-prompt (build → bundle, close → closed, keep-open → recorded). -- Auto-sweep at epic boundaries or run-end (`[sweep] auto`); a failed/paused child sweep never interrupts the parent run. +- Auto-sweep at epic boundaries or run-end (`[sweep] auto`); a failed or paused child sweep is journaled + notified and leaves the parent running — including the `SystemExit` an unusable multiplexer or an unresolvable profile raises, which used to end the parent at exit 1 with an orphaned session (#501). A stop (`bmad-loop stop`) or a Ctrl-C delivered through the child is the deliberate exception and propagates, so a parent stays stoppable while a child sweep is mid-flight. +- A trigger is spent only once its child has actually started, so a refusal no longer consumes it (#501) — but nothing re-asks it either, since both call sites close their boundary within a few statements of the refusal. What the run keeps instead is a record: `sweeps_refused` (trigger → `not-started` / `failed` / `dirty`), surfaced by the end-of-run summary, `bmad-loop status`, `status --json` and `bmad-loop diagnose`, naming `bmad-loop sweep` (which needs a clean worktree) as the human-present follow-up. - Repeat mode (`--repeat` / `[sweep] repeat`): re-triages after each cycle to absorb newly generated deferred work, stopping when a cycle does nothing addressable or hits `max_cycles`. - Sweeps are their own resumable runs (`bmad-loop resume `). An escalated bundle resolves like a story escalation, including intent-gap patch-restore: `bmad-loop resolve --restore-patch ` re-arms the bundle spec to `in-review` and the re-driven bundle session resumes review on the re-applied patch instead of re-implementing. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 098251e0..713c1316 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -5687,7 +5687,10 @@ def _record_sweep_refusal(self, trigger: str, reason: str) -> None: def _maybe_auto_sweep(self, kind: str, trigger: str) -> None: """Run a child deferred-work sweep when policy [sweep].auto matches. The child is its own resumable run; a paused or failed child is - journaled + notified but never interrupts this run. + journaled + notified but never interrupts this run — "failed" including a + ``SystemExit`` (#501). A stop or a ``KeyboardInterrupt`` delivered through + the child is the deliberate exception and propagates to the owner; the + arms below carry why, in both directions. ``state.sweeps_triggered`` spends the trigger only once a child sweep has actually started. The ``started`` thunk handed to the factory diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index 2d80844c..bc9c9337 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -93,9 +93,13 @@ def resolve_profiles(policy: Policy, project: Path) -> dict[str, CLIProfile]: file: a session that leaves a background writer flipping ``.bmad-loop/profiles/*.toml`` between a benign and a hostile copy needs only the digest's read to catch the benign one and the adapter's read to catch the - other. That race is cheap to retry — a lost round raises - `sweep-auto-not-started`, which `_maybe_auto_sweep` swallows, and the next - auto-sweep trigger deals again — so "narrow window" is not a defense. + other. That race is cheap to repeat — a lost round raises + `sweep-auto-not-started`, which `_maybe_auto_sweep` swallows, so the parent + runs on and the next epic boundary deals a fresh hand — so "narrow window" is + not a defense. The repeat is the writer's, never the orchestrator's: #501 + leaves a refused trigger unspent, but nothing re-asks that trigger (see + `_maybe_auto_sweep`'s docstring), and under ``[sweep] auto = "run-end"`` a run + has exactly one. It is `per-epic` that hands out the further rounds. Resolving once and threading the result removes the second read rather than shrinking the window. From 329bd7bbd7a426bba74133c03f58c580863cd64c Mon Sep 17 00:00:00 2001 From: t Date: Sat, 15 Aug 2026 09:02:31 -0700 Subject: [PATCH 09/11] docs: cite the issue that describes each defect in the changelog (#501) The SystemExit, RunStopped and stranded-composition defects were found while fixing #501 and filed as #600, #601 and #602. A reader hitting `(#501)` on the SystemExit entry lands on an issue about a spent sweep trigger, which does not describe that defect at all. --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 748247df..f7f12856 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,12 +50,12 @@ breaking changes may land in a minor release. ### Fixed -- **Launching with a `--run-id` that already names a run is refused (#501).** The flag pointed at an +- **Launching with a `--run-id` that already names a run is refused (#602).** The flag pointed at an existing run adopted that run's directory and published its own `state.json` over it. The id is now claimed exclusively as the directory is created, so a collision aborts the launch before anything is written and the earlier run is left untouched. -- **A launch that aborts while standing up its adapters no longer strands an empty run (#501).** An +- **A launch that aborts while standing up its adapters no longer strands an empty run (#602).** An adapter failure left a run directory with no `run-start` that nothing reconciled, so it lingered in `bmad-loop list` looking resumable. Composition is now atomic from the first published artifact: on any escape the run directory and its out-of-tree state are removed and the original @@ -63,7 +63,7 @@ breaking changes may land in a minor release. fails is now reported instead of passing silently. - **A failing auto-sweep can no longer kill its parent run, and a stop during one is no longer - swallowed (#501).** A `SystemExit` from the child — what an unusable multiplexer or an + swallowed (#600, #601).** A `SystemExit` from the child — what an unusable multiplexer or an unresolvable profile raises — escaped every handler and ended the process at exit 1, leaving the parent neither `finished` nor `crashed` with an orphaned agent session. In the other direction a stop was recorded as a child failure, letting the parent run on to `finished` and leaving it From 5f914b704c8b37e66c32255714f0b451799c203c Mon Sep 17 00:00:00 2001 From: t Date: Sat, 15 Aug 2026 09:03:59 -0700 Subject: [PATCH 10/11] docs: cite #600/#601 where the two child-sweep exceptions are named (#501) --- docs/FEATURES.md | 2 +- src/bmad_loop/engine.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index e4bdc5c1..bbdfe5fc 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -126,7 +126,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Bundles run the full pipeline (dev `--dw-bundle` → review → verify → commit); the review gate checks every bundle entry is `status: done`. - Interactive decision walkthrough (build / close / keep-open per option, with a recommendation); answers written back as `decision:` lines. Unattended runs leave decisions open. - Answer skipped/missed decisions out of band with `bmad-loop decisions` (or `d` in the TUI): reconstructed from past triage output, saved to `.bmad-loop/decisions.json`, and consumed by the next sweep with no re-prompt (build → bundle, close → closed, keep-open → recorded). -- Auto-sweep at epic boundaries or run-end (`[sweep] auto`); a failed or paused child sweep is journaled + notified and leaves the parent running — including the `SystemExit` an unusable multiplexer or an unresolvable profile raises, which used to end the parent at exit 1 with an orphaned session (#501). A stop (`bmad-loop stop`) or a Ctrl-C delivered through the child is the deliberate exception and propagates, so a parent stays stoppable while a child sweep is mid-flight. +- Auto-sweep at epic boundaries or run-end (`[sweep] auto`); a failed or paused child sweep is journaled + notified and leaves the parent running — including the `SystemExit` an unusable multiplexer or an unresolvable profile raises, which used to end the parent at exit 1 with an orphaned session (#600). A stop (`bmad-loop stop`) or a Ctrl-C delivered through the child is the deliberate exception and propagates, so a parent stays stoppable while a child sweep is mid-flight (#601). - A trigger is spent only once its child has actually started, so a refusal no longer consumes it (#501) — but nothing re-asks it either, since both call sites close their boundary within a few statements of the refusal. What the run keeps instead is a record: `sweeps_refused` (trigger → `not-started` / `failed` / `dirty`), surfaced by the end-of-run summary, `bmad-loop status`, `status --json` and `bmad-loop diagnose`, naming `bmad-loop sweep` (which needs a clean worktree) as the human-present follow-up. - Repeat mode (`--repeat` / `[sweep] repeat`): re-triages after each cycle to absorb newly generated deferred work, stopping when a cycle does nothing addressable or hits `max_cycles`. - Sweeps are their own resumable runs (`bmad-loop resume `). An escalated bundle resolves like a story escalation, including intent-gap patch-restore: `bmad-loop resolve --restore-patch ` re-arms the bundle spec to `in-review` and the re-driven bundle session resumes review on the re-applied patch instead of re-implementing. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 713c1316..9ecff908 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -5688,9 +5688,9 @@ def _maybe_auto_sweep(self, kind: str, trigger: str) -> None: """Run a child deferred-work sweep when policy [sweep].auto matches. The child is its own resumable run; a paused or failed child is journaled + notified but never interrupts this run — "failed" including a - ``SystemExit`` (#501). A stop or a ``KeyboardInterrupt`` delivered through - the child is the deliberate exception and propagates to the owner; the - arms below carry why, in both directions. + ``SystemExit`` (#600). A stop or a ``KeyboardInterrupt`` delivered through + the child is the deliberate exception and propagates to the owner (#601); + the arms below carry why, in both directions. ``state.sweeps_triggered`` spends the trigger only once a child sweep has actually started. The ``started`` thunk handed to the factory From 6d96b3636ac7a7e80305ecf8333987238558464e Mon Sep 17 00:00:00 2001 From: t Date: Sat, 15 Aug 2026 09:12:50 -0700 Subject: [PATCH 11/11] fix: guard the composition window the run-dir claim opens (#602) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_claim_run_dir` publishes the first artifact — the run directory itself, which is what a later `--run-id` collides with — but the guard opened two statements later, at `save_state`. An abort in between left an empty run dir nothing removes and nothing shows: `bmad-loop list` is state.json gated, so it is invisible there, and the only way to meet it is a later launch being refused by a directory holding nothing. Neither intervening statement can realistically fail on its own (`Journal` mkdirs `exist_ok=True` over a directory this frame just created, and `build_run_state` is a pure constructor), but the arm is `BaseException` precisely because a signal lands between arbitrary statements, and the window it covers should not have a hole two statements wide. The claim itself stays outside the guard, unchanged: a collision refusal reaching the unwind would delete the run it exists to protect. `_unwind_composition` now takes `Journal | None`, since aborting before the Journal is built is exactly the new case. Guarded explicitly rather than left to the surrounding `suppress(Exception)` — an AttributeError on None IS an Exception, so it would have worked by accident while reading as though a journal were guaranteed. Reported by codex on #604. --- src/bmad_loop/runsetup.py | 68 ++++++++++++++++++----------- tests/test_runsetup.py | 90 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 25 deletions(-) diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index bc9c9337..73a5f153 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -836,7 +836,7 @@ def _claim_run_dir(run_dir: Path) -> None: ) from e -def _unwind_composition(project: Path, run_dir: Path, journal: Journal) -> None: +def _unwind_composition(project: Path, run_dir: Path, journal: Journal | None) -> None: """Remove the run a failed ``compose_*`` had already published, so a launch that aborts partway leaves nothing behind. @@ -909,8 +909,16 @@ def _unwind_composition(project: Path, run_dir: Path, journal: Journal) -> None: # `Journal.append` opens with "a" WITHOUT a mkdir, so it raises rather than # resurrecting the run it just removed. Suppressed, and the stderr line # above still carries the report. - with suppress(Exception): - journal.append("composition-unwind-failed", run_id=run_dir.name, error=detail) + # + # ``journal`` is None when the composer aborted between claiming the run dir + # and building the Journal — a window only a signal can realistically land + # in. Guarded explicitly rather than left to the ``suppress`` above: an + # AttributeError on None IS an Exception and would be swallowed, so the + # code would work by accident while reading as though a Journal were + # guaranteed. The stderr report is the part that matters and is unaffected. + if journal is not None: + with suppress(Exception): + journal.append("composition-unwind-failed", run_id=run_dir.name, error=detail) def compose_run( @@ -956,22 +964,30 @@ def compose_run( # Outside the try below, and it must stay there: a collision refusal that # reached `_unwind_composition` would delete the run it exists to protect. _claim_run_dir(run_dir) - journal = Journal(run_dir) - state = build_run_state( - run_id=run_id, - project=project, - policy=policy, - epic_filter=epic_filter, - story_filter=story_filter, - max_stories=max_stories, - stories_on=stories_on, - spec_folder=spec_folder, - trusted_config_digest=trusted_config_digest, - ) # Composition is atomic from the first published artifact onward: everything # below either lands whole or is unwound (see :func:`_unwind_composition`, # which also states why the arm is `BaseException` and not `Exception`). + # The guard opens on the statement immediately after the claim, because the + # claim is what publishes that first artifact — the run DIRECTORY itself, which + # is what a later `--run-id` collides with. Neither statement below can + # realistically fail (`Journal` mkdirs `exist_ok=True` over a directory this + # frame just created, and `build_run_state` is a pure constructor), but a + # signal can land between any two statements, and the arm is `BaseException` + # exactly so that case unwinds instead of stranding an empty run dir. + journal: Journal | None = None try: + journal = Journal(run_dir) + state = build_run_state( + run_id=run_id, + project=project, + policy=policy, + epic_filter=epic_filter, + story_filter=story_filter, + 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 @@ -1090,18 +1106,20 @@ def compose_sweep( run_dir = project / RUNS_DIR / run_id # Same claim, same reason, same placement outside the try as in `compose_run`. _claim_run_dir(run_dir) - journal = Journal(run_dir) - state = RunState( - run_id=run_id, - project=str(project), - started_at=time.strftime("%Y-%m-%dT%H:%M:%S"), - policy_snapshot=policy.to_dict(), - run_type="sweep", - trusted_config_digest=trusted_config_digest, - ) # Atomic from the first published artifact onward, exactly as in `compose_run` - # — same reason, and one more artifact to unwind (`sweep.json`). + # — same reason, same opening on the statement after the claim, and one more + # artifact to unwind (`sweep.json`). + journal: Journal | None = None try: + journal = Journal(run_dir) + state = RunState( + run_id=run_id, + project=str(project), + 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. runs.write_trusted_config_digest(project, run_id, trusted_config_digest) diff --git a/tests/test_runsetup.py b/tests/test_runsetup.py index 225ca0cc..cd38bee8 100644 --- a/tests/test_runsetup.py +++ b/tests/test_runsetup.py @@ -430,6 +430,96 @@ def test_compose_sweep_unwinds_the_run_when_the_adapters_abort(unwinding): _assert_unwound(unwinding) +def test_compose_run_unwinds_a_claim_abandoned_before_save_state(unwinding, monkeypatch): + """The guard opens on the statement after the claim, not at `save_state`. + + `_claim_run_dir` publishes the first artifact — the run DIRECTORY — so an abort + between it and `save_state` used to strand an empty dir the guard never saw. It + is invisible to `bmad-loop list` (state.json-gated), which is precisely why it + is worth removing: nothing surfaces it, and a later launch reusing that + `--run-id` is refused by a directory holding nothing. + + A `KeyboardInterrupt` because that is the only realistic way in: `Journal` + mkdirs `exist_ok=True` over a directory this frame just created and + `build_run_state` is a pure constructor, so neither fails on its own — but a + signal lands between arbitrary statements, and the arm is `BaseException`. + + `seen` is the positive control, on the fixture's own doctrine: "is it gone" + passes just as happily for a directory that was never created. + + Ablation: move the `try` back below `build_run_state` and this fails alone.""" + seen: dict[str, bool] = {} + + def exploding_build_run_state(**kwargs): + seen["run_dir"] = runs.run_dir_for(unwinding.project, RUN_ID).is_dir() + raise KeyboardInterrupt + + monkeypatch.setattr(runsetup, "build_run_state", exploding_build_run_state) + with pytest.raises(KeyboardInterrupt): + runsetup.compose_run( + project=unwinding.project, + paths=_fake_paths(unwinding.project), + policy=policy_mod.loads(""), + run_id=RUN_ID, + epic_filter=None, + story_filter=None, + max_stories=None, + stories_on=False, + spec_folder="", + sweep_factory=lambda _trigger, *, started: None, + make_adapters=unwinding.make_adapters, + engine_cls=_NeverBuilt, + stories_engine_cls=_NeverBuilt, + trusted_config_digest="deadbeef", + ) + assert seen == {"run_dir": True} # the claim published it... + assert not runs.run_dir_for(unwinding.project, RUN_ID).exists() # ...the unwind took it back + assert unwinding.published == {} # and it aborted well ahead of `make_adapters` + + +def test_compose_sweep_unwinds_when_the_journal_itself_cannot_be_built(unwinding, monkeypatch): + """The same window in the sweep composer, at its earliest statement — which is + also the one case that reaches `_unwind_composition` with NO journal. + + That is why this drives `Journal` rather than the `RunState` build: the unwind + writes a `composition-unwind-failed` entry through the journal it is handed, so + a `None` there has to be handled rather than left to the surrounding + `suppress(Exception)` (an `AttributeError` on `None` is an `Exception`, so the + code would work by accident while reading as though a journal were guaranteed). + + Ablation: restore `_unwind_composition`'s `journal: Journal` annotation and drop + the `if journal is not None` guard — this stays GREEN, because the suppress + absorbs the AttributeError. The guard is graded by the annotation and by this + docstring, not by an exit code; what this test does pin is that the run dir is + removed on this path at all, which fails alone if the `try` moves back down.""" + built: dict[str, bool] = {} + + def exploding_journal(run_dir): + built["run_dir"] = run_dir.is_dir() + raise OSError("journal unavailable") + + monkeypatch.setattr(runsetup, "Journal", exploding_journal) + with pytest.raises(OSError, match="journal unavailable"): + runsetup.compose_sweep( + project=unwinding.project, + paths=_fake_paths(unwinding.project), + policy=policy_mod.loads(""), + run_id=RUN_ID, + prompting=False, + decisions_only=False, + max_bundles=None, + repeat=None, + max_cycles=None, + trigger="auto", + make_adapters=unwinding.make_adapters, + sweep_engine_cls=_NeverBuilt, + trusted_config_digest="deadbeef", + ) + assert built == {"run_dir": True} + assert not runs.run_dir_for(unwinding.project, RUN_ID).exists() + assert unwinding.published == {} + + PRIOR_STATE = '{"run_id": "prior", "finished": true}' PRIOR_JOURNAL = '{"kind": "run-complete"}\n' PRIOR_STAMP = "prior-digest"