From 3bc419e832674b211a53a66ddbb5a796a72d924b Mon Sep 17 00:00:00 2001 From: t Date: Sun, 18 Oct 2026 00:02:22 -0700 Subject: [PATCH 01/10] sweep dw-v7-sweep-shared-store-commits: DW-556, DW-557 via bmad-loop --- CHANGELOG.md | 6 + docs/sweep-guide.md | 25 ++-- docs/testing.md | 16 +-- src/bmad_loop/sweep.py | 69 ++++++++- src/bmad_loop/worktree_flow.py | 12 +- tests/test_conftest.py | 2 +- tests/test_sweep.py | 246 +++++++++++++++++++++++++++++++-- tests/test_tickets_e2e.py | 157 ++++++++++++++++++++- tests/test_worktree_flow.py | 29 ++++ 9 files changed, 515 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c657cfb8c..ec682fb10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -154,6 +154,12 @@ breaking changes may land in a minor release. - Name the T1 ticket plan an interrupted commit window left `done` or `blocked` when the resumed commit fails, instead of a bare `commit failed:` (DW-555). +- Commit a v7 sweep bundle's ledger close in the ticket store's own repository (T3/T4) once + its code lands, best effort with `Bmad-Code-*` trailers and withheld while the ledger is in + doubt, instead of leaving the store's ledger dirty (DW-556). +- Latch a worktree-isolated sweep bundle's artifact publication complete when its artifacts + dir is in a shared ticket store, instead of pausing on + `artifact publication intent is missing` after the merge (DW-557). - Resolve a relative v6 config path against the project root instead of the process cwd (DW-551). - Re-validate an ordinary unit's merge against the target on replay instead of trusting its diff --git a/docs/sweep-guide.md b/docs/sweep-guide.md index 37a6510bd..41bc7af32 100644 --- a/docs/sweep-guide.md +++ b/docs/sweep-guide.md @@ -90,7 +90,8 @@ A sweep runs on a v7 project the same way, with these differences: - **Store layout.** The store-topology check judges a v7 sweep as a tickets run, because its ledger lives in the ticket store. The sweep records the store's topology at its start, and a resume refuses a store that changed since, or whose initiative folder is gone. - **Pinned initiative.** A sweep records the active initiative it started on. A resume, `resolve` or the dashboard's re-arm refuses while `[core] active_initiative` names another: set it back, or start a new sweep. - **Decisions per initiative.** Each initiative has its own ledger and DW numbering, so pre-answers are kept per initiative in `.bmad-loop/decisions-.json`. `bmad-loop decisions` and the TUI answer the active initiative's decisions and offer only those its own sweeps raised. An answer given after another initiative was made active than the one its decision was listed from is refused; list the decisions again. With no initiative active, the project store stays `.bmad-loop/decisions.json` and only sweeps that ran with no initiative are offered. -- **Plans in a store repository.** When the ticket store is its own git repository (nested or outside the checkout), the bundle's code commit cannot carry its plan. Once the code has landed, the accepted plan gets its own commit there, `chore(sweep): plan [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code. This is best effort, like a tickets run's store commit: a failure is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and the plan stays uncommitted for a human. +- **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. +- **Plans in a store repository.** When the ticket store is its own git repository (nested or outside the checkout), the bundle's code commit cannot carry its plan. Once the code has landed, the accepted plan gets its own commit there, `chore(sweep): plan [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code. This is best effort, like a tickets run's store commit: a failure is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and the plan stays uncommitted for a human. The bundle's ledger close is committed there the same best-effort way, as `chore(sweep): close resolved deferred-work entries` with the same trailers and the same journaled failures, and is withheld (`sweep-ledger-commit-withheld`) while the run holds its ledger in doubt. ## Story-declared closure (`closes_deferred`) @@ -177,17 +178,17 @@ A stored answer is usable only if it is a JSON object whose `effect` is `build`, The sweep commits each ledger or store change it makes as its own file-scoped commit: only that file is staged, and the whole file is committed. -| Commit message | When | -| --------------------------------------------------------------------------- | ---------------------------------------------------------- | -| `chore(sweep): record deferred-work decisions` | after the decision phase | -| `chore(sweep): close resolved deferred-work entries` | already-resolved entries and decided closes | -| `chore(sweep): commit ledger at the sweep cycle boundary` | between repeat cycles | -| `chore(sweep): commit ledger after recovering in-flight bundles` | on resume, after in-flight bundles are recovered | -| `chore(sweep): commit a ledger write an interrupted phase left unpublished` | on resume, for a write an interrupted phase did not commit | -| `chore(sweep): drop consumed deferred-work pre-answers` | end-of-cycle prune of the project store | -| `chore(sweep): drop stale deferred-work pre-answer` | a `stale-option` drop | -| `chore(sweep): migrate legacy deferred-work entries to DW format` | legacy-ledger migration | -| `chore(sweep): ingest retro action items` | retro action items added to the ledger at sweep start | +| Commit message | When | +| --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | +| `chore(sweep): record deferred-work decisions` | after the decision phase | +| `chore(sweep): close resolved deferred-work entries` | already-resolved entries and decided closes; a landed bundle's close when the ledger is in a store repository | +| `chore(sweep): commit ledger at the sweep cycle boundary` | between repeat cycles | +| `chore(sweep): commit ledger after recovering in-flight bundles` | on resume, after in-flight bundles are recovered | +| `chore(sweep): commit a ledger write an interrupted phase left unpublished` | on resume, for a write an interrupted phase did not commit | +| `chore(sweep): drop consumed deferred-work pre-answers` | end-of-cycle prune of the project store | +| `chore(sweep): drop stale deferred-work pre-answer` | a `stale-option` drop | +| `chore(sweep): migrate legacy deferred-work entries to DW format` | legacy-ledger migration | +| `chore(sweep): ingest retro action items` | retro action items added to the ledger at sweep start | `bmad-loop decisions` commits each file it wrote separately as `chore(decisions): pre-answer `: the ledger only if a `decision:` line landed, and the project store only for `build` or `keep-open`. A ledger the sweep marked as in doubt blocks every further ledger publish for that run (`sweep-ledger-commit-withheld`). diff --git a/docs/testing.md b/docs/testing.md index b8fc432c4..2bad1a0bc 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -207,14 +207,14 @@ Rules for adding or touching a guard: **No test, at any layer, ever spends an LLM token.** AGENTS.md states the hard invariant for the live/E2E gates; in practice it holds everywhere, and the mechanism differs per gate: -| Gate | Real component | Zero-token mechanism | Runs where | -| ------------------------------------ | ------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------- | -| `test_generic_tmux.py` (`HAVE_TMUX`) | tmux server | The spawned "CLI" is a tiny shell script written by the test | CI Linux job + any POSIX dev box with tmux | -| `test_stories_e2e.py` | tmux + the real `bmad-loop run/resolve/resume` CLI | Scripted fake `claude` variants (bash string constants) wired in as a custom TOML profile; the fake writes its own SessionStart/Stop hook events | CI Linux job (the fakes use GNU coreutils + `setsid`) | -| `test_tickets_e2e.py` | tmux + the real `bmad-loop` CLI and the vendored `tickets.py` | A scripted fake `bmad-build-auto` (bash) resolves its ticket through `tickets.py find`, builds to `built` and writes its own SessionStart/Stop hook events; it never marks `done`. 13 rows cover the pauses, checkpoints, retro and closure, resolve, isolation, store repos and refusals, several parametrized over `topology` (T1–T4) | CI Linux job | -| `test_opencode_live.py` | A real `opencode serve` HTTP server | **Never sends a prompt**: only spawn/teardown and the health/OpenAPI/session/event endpoints of the detected major (1.x or 2.x); the prompt endpoint is checked against the OpenAPI schema, never called | Manual: a POSIX box with a runnable `opencode` | -| `test_opencode_live_turn.py` | A real OpenCode 2.x server running one full turn | **Prompts reach only a local fake**: the one test that prompts sends every LLM request to an in-process stdlib OpenAI-compatible server on 127.0.0.1, under a scrubbed env (scratch HOME/XDG, no credentials), a mock-only provider config and a deny on the hosted provider, and asserts the fake saw every request | Manual: a POSIX box with a runnable OpenCode 2.x `opencode` | -| `test_psmux_live.py` | A real psmux on Windows | **Parked windows only**: every window runs `pwsh -NoProfile -Command exit 0`. `test_premise_*` probes are inverted (red means a workaround became droppable); `test_adopted_*` probes are ordinary | CI Windows job + any Windows dev box with psmux 3.4.0+ and `pwsh` | +| Gate | Real component | Zero-token mechanism | Runs where | +| ------------------------------------ | ------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------- | +| `test_generic_tmux.py` (`HAVE_TMUX`) | tmux server | The spawned "CLI" is a tiny shell script written by the test | CI Linux job + any POSIX dev box with tmux | +| `test_stories_e2e.py` | tmux + the real `bmad-loop run/resolve/resume` CLI | Scripted fake `claude` variants (bash string constants) wired in as a custom TOML profile; the fake writes its own SessionStart/Stop hook events | CI Linux job (the fakes use GNU coreutils + `setsid`) | +| `test_tickets_e2e.py` | tmux + the real `bmad-loop` CLI and the vendored `tickets.py` | A scripted fake `bmad-build-auto` (bash) resolves its ticket through `tickets.py find`, builds to `built` and writes its own SessionStart/Stop hook events; it never marks `done`. 14 rows cover the pauses, checkpoints, retro and closure, resolve, isolation, store repos, refusals and an isolated sweep, several parametrized over `topology` (T1–T4) | CI Linux job | +| `test_opencode_live.py` | A real `opencode serve` HTTP server | **Never sends a prompt**: only spawn/teardown and the health/OpenAPI/session/event endpoints of the detected major (1.x or 2.x); the prompt endpoint is checked against the OpenAPI schema, never called | Manual: a POSIX box with a runnable `opencode` | +| `test_opencode_live_turn.py` | A real OpenCode 2.x server running one full turn | **Prompts reach only a local fake**: the one test that prompts sends every LLM request to an in-process stdlib OpenAI-compatible server on 127.0.0.1, under a scrubbed env (scratch HOME/XDG, no credentials), a mock-only provider config and a deny on the hosted provider, and asserts the fake saw every request | Manual: a POSIX box with a runnable OpenCode 2.x `opencode` | +| `test_psmux_live.py` | A real psmux on Windows | **Parked windows only**: every window runs `pwsh -NoProfile -Command exit 0`. `test_premise_*` probes are inverted (red means a workaround became droppable); `test_adopted_*` probes are ordinary | CI Windows job + any Windows dev box with psmux 3.4.0+ and `pwsh` | Never "fix" a gate to call a real CLI, and never add a completion path that trusts LLM output: sessions complete only on hook Stop events or window death, and the fakes exercise exactly diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index fe617e866..6583a1aaa 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -7449,7 +7449,8 @@ def _withhold_ledger_publish(self, message: str, *, dw_ids: list[str] | None = N it — DW-192) and `reason="ledger-in-doubt"`. `dw_ids` (DW-250) is the list of ids the declining publisher would otherwise have set out to prove, emitted ONLY when given: `_publish_stranded_close` names its cached plan's - already-resolved and decision ids, while the three DW-246 callers + already-resolved and decision ids, `_publish_bundle_close` (DW-556) the + landed bundle's `bundle_closes_intended`, while the three DW-246 callers deliberately pass none — `_close_resolved`'s arms have `closed` and `ids` in scope, but their DW-246 row shape stays unchanged. `diagnostics` already routes the field by name (`_JOURNAL_KEYLIST_FIELDS`). @@ -8779,19 +8780,73 @@ def store_topology(self) -> ticketstore.StoreTopology | None: return ticketstore.StoreTopology.from_dict(recorded) if recorded else None def _land_post_landing_writes(self, task: StoryTask) -> None: - """Once a bundle's code has landed, publish its accepted plan when the plan - lives in a ticket store with its own repository (T3/T4): the code commit - cannot carry a file in another repository. See - :meth:`_publish_bundle_plan`.""" + """Once a bundle's code has landed, publish its accepted plan and its + ledger close when they live in a ticket store with its own repository + (T3/T4): the code commit cannot carry a file in another repository. See + :meth:`_publish_bundle_plan` and :meth:`_publish_bundle_close`.""" super()._land_post_landing_writes(task) self._publish_bundle_plan(task) + self._publish_bundle_close(task) + + def _publish_bundle_close(self, task: StoryTask) -> None: + """Commit the ledger close a landed bundle wrote when the ledger lives in a + store repository (T3/T4), through + :class:`~bmad_loop.ticketstore.StoreCommitter` as ``chore(sweep): close + resolved deferred-work entries`` with the ``Bmad-Code-*`` trailers. + Elsewhere the close rides the bundle's own code commit (``commit_story``'s + ``git add -A``, or the merge-back and ``_carry_isolated_ledger_writes`` + under isolation); a store repository's ledger is in no tree that commit can + reach. + + Keyed on ``task.bundle_closes_intended``, as the isolated carry is: a + bundle that wrote no close publishes nothing. Withheld while the run holds + its ledger in doubt (``sweep-ledger-commit-withheld``). Otherwise best + effort, like :meth:`_publish_bundle_plan`: it runs inside + ``integrate_unit`` after the merge, so a failed commit journals + ``store-commit-failed`` (a staged foreign version of the ledger + ``store-commit-foreign-dirt``) and the close stays uncommitted for a human + or a later publisher. A ledger already at the store's HEAD journals + nothing.""" + topology = self.store_topology + if topology is None or topology.store_repo is None or not task.bundle_closes_intended: + return + ledger = self.workspace.paths.deferred_work + try: + inside = ledger.resolve().is_relative_to(topology.store_repo.resolve()) + except (OSError, RuntimeError, ValueError): + inside = False + if not inside or not ledger.is_file(): + return + message = "chore(sweep): close resolved deferred-work entries" + if self._ledger_unfit_to_publish(): + self._withhold_ledger_publish(message, dw_ids=list(task.bundle_closes_intended)) + return + committer = ticketstore.StoreCommitter(topology.store_repo, self.paths.repo_root) + try: + result = committer.commit( + task.story_key, + [ledger], + message, + code_commit=self._landed_code_commit(task), + code_baseline=task.baseline_commit, + ) + except ticketstore.StoreCommitError as e: + self.journal.append("store-commit-failed", story_key=task.story_key, error=str(e)) + return + if result.foreign: + self.journal.append( + "store-commit-foreign-dirt", story_key=task.story_key, count=len(result.foreign) + ) + return + if result.sha is not None: + self.journal.append("store-committed", story_key=task.story_key, commit=result.sha) def _publish_bundle_plan(self, task: StoryTask) -> None: """Commit ``task``'s accepted bundle plan in the store's repository through :class:`~bmad_loop.ticketstore.StoreCommitter`, as ``chore(sweep): plan [bmad-loop ]`` with the ``Bmad-Code-*`` trailers. The ledger is - not part of it: the sweep's ledger publishers already commit it in the - ledger's own repository. An unversioned store (T2/T6) journals + not part of it: :meth:`_publish_bundle_close` commits the bundle's ledger + close on its own. An unversioned store (T2/T6) journals ``store-unversioned`` once per run. Best effort, like a tickets run's retro commit, and never a pause: the plan diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index bee73f64c..f1d7c013c 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -4800,8 +4800,16 @@ def validate_committed_publication( ) def finish_publication(self, task: StoryTask, unit: UnitWorkspace | None) -> None: - """Publish and latch before successful teardown, including merge replay.""" - if task.dw_ids: + """Publish and latch before successful teardown, including merge replay. + An artifacts dir in the shared ticket store has nothing to publish (the + mount wrote the main checkout's one copy in place, so + :meth:`prepare_publication` recorded no intent): the publication is + latched complete as is.""" + if task.dw_ids and self.paths.in_shared_store(self.paths.implementation_artifacts): + if not task.artifact_publication_complete: + task.artifact_publication_complete = True + self._save() + elif task.dw_ids: try: try: artifact_publication.publish(task, self.paths) diff --git a/tests/test_conftest.py b/tests/test_conftest.py index dd53dfafd..e2955a95c 100644 --- a/tests/test_conftest.py +++ b/tests/test_conftest.py @@ -1691,7 +1691,7 @@ def _declares_loadgroup(addopts: object) -> bool: # `test_reap_identity_*`, `test_recorded_*`. _OTHER_DEFS: 28, }, - "test_tickets_e2e.py": {"test_e2e_tickets_": 13}, + "test_tickets_e2e.py": {"test_e2e_tickets_": 14}, } diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 2385b2b64..9a71ced81 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -38160,7 +38160,14 @@ def test_v7_bundle_plan_committed_in_store_repo(request, fixture, layout): rel = plan.resolve().relative_to(store.resolve()).as_posix() commits = git(store, "log", "--format=%H", "--", rel).split() assert len(commits) == 1 - assert [e["commit"] for e in _records(engine, "store-committed")] == commits + # the plan's row; the bundle's ledger close journals its own (DW-556) + plan_rows = [ + e["commit"] + for e in _records(engine, "store-committed") + if rel in git(store, "show", "--name-only", "--format=", e["commit"]).split() + ] + assert plan_rows == commits + assert len(_records(engine, "store-committed")) == 2 # the plan's and the close's body = git(store, "log", "-1", "--format=%B", commits[0]) assert body.splitlines()[0] == "chore(sweep): dw-fix-things plan [bmad-loop sweep-run]" assert f"Bmad-Code-Repo: {paths.repo_root.as_posix()}" in body @@ -38171,15 +38178,224 @@ def test_v7_bundle_plan_committed_in_store_repo(request, fixture, layout): assert verify.status_of(verify.read_frontmatter(plan)) == "built" -def test_v7_bundle_plan_trailers_name_the_landed_squash_commit(project_v7_store_repo): +_V7_LEDGER_CLOSE = "chore(sweep): close resolved deferred-work entries" +_V7_LEDGER_DECISIONS = "chore(sweep): record deferred-work decisions" + + +_V7_DW2_CLOSE_DECISION = { + "id": "DW-2", + "question": "close as moot?", + "context": "", + "options": [ + {"key": "1", "label": "Close it", "effect": "close", "resolution": "moot"}, + {"key": "2", "label": "Keep open", "effect": "keep-open"}, + ], + "recommendation": "1", +} + + +def _v7_store_ledger_sweep( + v7, layout, *, arm_doubt: bool = False, stage_foreign_ledger: bool = False +): + """A v7 sweep over a store with its own repository (``layout`` T3/T4) whose + ledger, `{DW-1: open, DW-2: open}`, is committed in that store: triage bundles + DW-1 (`fix-things`) and asks DW-2's `close` decision, answered `1`. + ``arm_doubt`` arms the run's ledger doubt (the real `_record_ledger_doubt` + latch) from inside the bundle's dev session; ``stage_foreign_ledger`` has that + session stage a hand-edited ledger in the store and put the disk bytes back, + so the index holds a version that is neither HEAD's nor the bundle close's. + Returns the engine, its paths, + the store, the ledger's store-relative path and the code repo's head before + the run.""" + store = v7.output_folder + paths = _v7_paths(v7) + code = paths.repo_root + write_ledger(paths, {"DW-1": "open", "DW-2": "open"}, commit=False) + git(store, "add", "-A") + git(store, "commit", "-q", "-m", "ledger") + ledger_rel = paths.deferred_work.resolve().relative_to(store.resolve()).as_posix() + # premise: the store repository versions the ledger, the code repository does not + assert git(store, "ls-files", "--", ledger_rel).strip() == ledger_rel + assert git(store, "status", "--porcelain") == "" + if paths.deferred_work.resolve().is_relative_to(code.resolve()): # T3: nested, ignored + code_rel = paths.deferred_work.resolve().relative_to(code.resolve()).as_posix() + assert git(code, "ls-files", "--", code_rel) == "" + code_head = rev_parse_head(code) + topology = ticketstore.assess_store(paths, "none") + assert isinstance(topology, ticketstore.StoreTopology) and topology.layout == layout + triage = triage_result( + ["DW-1", "DW-2"], + bundles=[{"name": "fix-things", "dw_ids": ["DW-1"], "intent": "fix it"}], + decisions=[_V7_DW2_CLOSE_DECISION], + ) + dev = _v7_bundle_dev_effect(paths, "fix-things", ["DW-1"], followup_review=False) + holder: list[SweepEngine] = [] + + def dev_effect(spec): + if arm_doubt: + holder[0]._record_ledger_doubt() + if stage_foreign_ledger: + ledger = paths.deferred_work + original = ledger.read_bytes() + ledger.write_bytes(original + b"\nhand-edited\n") + git(store, "add", "--", ledger_rel) + ledger.write_bytes(original) + return dev(spec) + + engine, _ = make_sweep( + paths, + [triage_effect(triage), dev_effect], + policy=_v7_review_policy(enabled=False), + answers=["1"], + prompting=True, + store_topology=topology.to_dict(), + ) + holder.append(engine) + return engine, paths, store, ledger_rel, code_head + + +def _store_closes(store: Path, ledger_rel: str) -> list[str]: + """The bodies of the store's ledger-close commits, each asserted to touch the + ledger alone.""" + log = [line.split(" ", 1) for line in git(store, "log", "--format=%H %s").splitlines()] + shas = [sha for sha, subject in log if subject == _V7_LEDGER_CLOSE] + for sha in shas: + assert git(store, "show", "--name-only", "--format=", sha).split() == [ledger_rel] + return [git(store, "log", "-1", "--format=%B", sha) for sha in shas] + + +@pytest.mark.parametrize( + ("fixture", "layout"), + [("project_v7_store_repo", "T3"), ("project_v7_external_store", "T4")], +) +def test_v7_sweep_ledger_writes_committed_in_store_repo(request, fixture, layout): + """On v7 the ledger lives in the initiative folder, so a ticket store with its + own repository (T3 nested, T4 outside) versions it. The sweep's own ledger + writes are published there, each commit touching only the ledger: the recorded + `close` decision for DW-2 (`_commit_ledger` runs its git in the ledger's + resolved parent, so the enclosing repository is the store's), and the bundle's + close of DW-1, which `_publish_bundle_close` commits through `StoreCommitter` + with the `Bmad-Code-*` trailers naming the bundle's code commit (elsewhere the + close rides the bundle's code commit, which cannot reach the store). The code + repository receives no `chore(sweep)` commit. + + ABLATIONS (performed): A1 run `_commit_ledger`'s git in `self.paths.repo_root` + instead of the ledger's resolved parent and both rows fail (no decisions + commit in the store); A2 drop the `_publish_bundle_close` call in + `_land_post_landing_writes` and both rows fail (no close commit carries the + bundle's `Bmad-Code-Commit` trailer).""" + engine, paths, store, ledger_rel, code_head = _v7_store_ledger_sweep( + request.getfixturevalue(fixture), layout + ) + + summary = engine.run() + + assert not summary.crashed and not summary.paused + task = engine.state.tasks["dw-fix-things"] + assert task.phase == Phase.DONE and task.commit_sha + entries = ledger_entries(paths) + assert entries["DW-1"].status.startswith("done") + assert entries["DW-2"].status.startswith("done") + assert "closed by human decision: moot" in entries["DW-2"].body + log = [line.split(" ", 1) for line in git(store, "log", "--format=%H %s").splitlines()] + decisions = [sha for sha, subject in log if subject == _V7_LEDGER_DECISIONS] + assert len(decisions) == 1, log + assert git(store, "show", "--name-only", "--format=", decisions[0]).split() == [ledger_rel] + bundle_closes = [ + body + for body in _store_closes(store, ledger_rel) + if f"Bmad-Code-Commit: {task.commit_sha}" in body + ] + assert len(bundle_closes) == 1, _store_closes(store, ledger_rel) + assert f"Bmad-Code-Range: {task.baseline_commit}..{task.commit_sha}" in bundle_closes[0] + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + code = paths.repo_root + code_new = git(code, "log", "--format=%s", f"{code_head}..HEAD").splitlines() + assert code_new and not [s for s in code_new if s.startswith("chore(sweep)")], code_new + + +def test_v7_sweep_bundle_close_withheld_while_the_ledger_is_in_doubt(project_v7_store_repo): + """T3: a run that holds its ledger in doubt (armed here during the bundle's + dev session) does not publish the landed bundle's close to the store + repository. It journals `sweep-ledger-commit-withheld` naming the close + message and the bundle's ids, and no store commit carries the close. + + ABLATION (performed): drop the `if self._ledger_unfit_to_publish():` gate in + `_publish_bundle_close` and this fails (no withheld row; a close commit with + the bundle's `Bmad-Code-Commit` trailer lands in the store).""" + engine, paths, store, ledger_rel, _ = _v7_store_ledger_sweep( + project_v7_store_repo, "T3", arm_doubt=True + ) + + engine.run() + + task = engine.state.tasks["dw-fix-things"] + assert task.phase == Phase.DONE and task.commit_sha # premise: the bundle landed + assert task.bundle_closes_intended == ["DW-1"] + assert ledger_entries(paths)["DW-1"].status.startswith("done") # the close was written + withheld = [ + r + for r in _records(engine, "sweep-ledger-commit-withheld") + if r["message"] == _V7_LEDGER_CLOSE and r.get("dw_ids") == ["DW-1"] + ] + assert len(withheld) == 1, _records(engine, "sweep-ledger-commit-withheld") + assert withheld[0]["reason"] == "ledger-in-doubt" + closes = _store_closes(store, ledger_rel) + assert not [b for b in closes if f"Bmad-Code-Commit: {task.commit_sha}" in b], closes + assert git(store, "status", "--porcelain", "--", ledger_rel) != "" # still unpublished + + +@pytest.mark.parametrize("fault", ["git", "foreign"]) +def test_v7_sweep_bundle_close_store_commit_failure_is_best_effort( + project_v7_store_repo, monkeypatch, fault +): + """T3: the landed bundle's ledger close fails to commit in the store repository + (a git fault, or a staged foreign version of the ledger). The publisher runs + after the merge, so the run neither crashes nor pauses: it journals + `store-commit-failed` / `store-commit-foreign-dirt` for the bundle, and the + close stays uncommitted in the store. + + ABLATIONS (performed): drop the `except ticketstore.StoreCommitError` clause + in `_publish_bundle_close` and `git` fails (the run crashes); drop its + `if result.foreign:` arm and `foreign` fails (no foreign-dirt row).""" + if fault == "git": + real = ticketstore.StoreCommitter.commit + + def commit(self, unit_id, paths, message, **kwargs): + if message == _V7_LEDGER_CLOSE: + raise ticketstore.StoreCommitError(f"{unit_id}: index.lock exists") + return real(self, unit_id, paths, message, **kwargs) + + monkeypatch.setattr(ticketstore.StoreCommitter, "commit", commit) + engine, paths, store, ledger_rel, _ = _v7_store_ledger_sweep( + project_v7_store_repo, "T3", stage_foreign_ledger=fault == "foreign" + ) + + summary = engine.run() + + assert not summary.crashed and not summary.paused + task = engine.state.tasks["dw-fix-things"] + assert task.phase == Phase.DONE and task.bundle_closes_intended == ["DW-1"] + assert ledger_entries(paths)["DW-1"].status.startswith("done") # the close was written + kind = "store-commit-failed" if fault == "git" else "store-commit-foreign-dirt" + assert [r["story_key"] for r in _records(engine, kind)] == ["dw-fix-things"] + closes = _store_closes(store, ledger_rel) + assert not [b for b in closes if f"Bmad-Code-Commit: {task.commit_sha}" in b], closes + assert git(store, "status", "--porcelain", "--", ledger_rel) != "" # still unpublished + + +@pytest.mark.parametrize("publisher", ["plan", "close"]) +def test_v7_bundle_store_trailers_name_the_landed_squash_commit(project_v7_store_repo, publisher): """Under worktree isolation a `squash` merge-back lands a different commit than the bundle's, and the unit branch's teardown leaves the bundle's - unreachable: the store trailers name the target's head, which holds the - code. Driven at the seam: a T3 bundle that landed in place, then re-published - as a merged-back unit whose own commit the target does not hold. - - ABLATION (performed): pass `task.commit_sha` in `_publish_bundle_plan` again - and the trailers name the unreachable unit commit — this FAILS.""" + unreachable: the store trailers on the bundle's plan commit and on its ledger + close commit name the target's head, which holds the code. Driven at the + seam: a T3 bundle that landed in place, then re-published as a merged-back + unit whose own commit the target does not hold. + + ABLATIONS (performed): pass `task.commit_sha` in `_publish_bundle_plan` + again and `plan` fails; the same in `_publish_bundle_close` and `close` + fails — each naming the unreachable unit commit.""" v7 = project_v7_store_repo store = v7.output_folder paths = _v7_paths(v7) @@ -38205,13 +38421,19 @@ def test_v7_bundle_plan_trailers_name_the_landed_squash_commit(project_v7_store_ landed = rev_parse_head(repo) unit = git(repo, "commit-tree", "HEAD^{tree}", "-p", "HEAD", "-m", "squashed away").strip() assert not verify.is_ancestor(repo, unit, landed) - plan = _v7_plan(paths, "fix-things") - plan.write_text(plan.read_text(encoding="utf-8") + "\nreviewed\n", encoding="utf-8") task = replace( engine.state.tasks["dw-fix-things"], worktree_path=str(repo / "unit"), commit_sha=unit ) - - engine._publish_bundle_plan(task) + if publisher == "plan": + plan = _v7_plan(paths, "fix-things") + plan.write_text(plan.read_text(encoding="utf-8") + "\nreviewed\n", encoding="utf-8") + engine._publish_bundle_plan(task) + else: + assert task.bundle_closes_intended == ["DW-1"] + ledger = paths.deferred_work + ledger.write_text(ledger.read_text(encoding="utf-8") + "\nreviewed\n", encoding="utf-8") + engine._publish_bundle_close(task) + assert git(store, "log", "-1", "--format=%s").strip() == _V7_LEDGER_CLOSE body = git(store, "log", "-1", "--format=%B") assert f"Bmad-Code-Commit: {landed}" in body diff --git a/tests/test_tickets_e2e.py b/tests/test_tickets_e2e.py index cd47775d5..afa405c91 100644 --- a/tests/test_tickets_e2e.py +++ b/tests/test_tickets_e2e.py @@ -1,6 +1,6 @@ """Deterministic full-CLI sandbox E2E for tickets mode (the BMAD v7 ticket tree). -Drives the REAL `bmad-loop run/resume/status` binaries through REAL tmux against +Drives the REAL `bmad-loop run/resume/status/sweep` binaries through REAL tmux against a v7 sandbox carrying the vendored upstream `tickets.py`, with a scripted fake `bmad-build-auto` wired in as a custom profile: no LLM, no cost, fully reproducible. Like `tests/test_stories_e2e.py`'s fake, it writes its own @@ -13,8 +13,11 @@ planning.` → stop at `ready-for-dev`; a committed `.block-` marker → `mark blocked`; `built`/`done` → a follow-up pass with no status change. The `/bmad-retrospective -H ` prompt writes the epic's retrospective document -with the verdict a `.retro-verdict-` marker names (default `accepted`). It -NEVER runs `mark done`: that write belongs to bmad-loop. +with the verdict a `.retro-verdict-` marker names (default `accepted`). A +`/bmad-loop-sweep` prompt writes a one-bundle triage partition, and a `dw-*` bundle +session edits code and leaves `plan-.md` at `built` in the initiative folder +its prompt names, with no `tickets.py` call. It NEVER runs `mark done`: that write +belongs to bmad-loop. Rows: (1) two tickets marked `done` by bmad-loop inside each unit commit, (2) `mark_done = "never"` leaves both at `built` and the second ticket still runs, @@ -25,8 +28,10 @@ a `rejected` verdict holding the next epic, (9) a session-blocked ticket re-armed by `resolve` through `mark`, (10) worktree isolation over a tracked (T1) and an ignored, redirected (T2) store, (11) plan and retrospective commits in a separate -store repository (T3 nested, T4 outside), and the preflight refusals of (12) a -tracker store and (13) a workspace layout (T5). Rows 7–9 run over T1 and T3. A run +store repository (T3 nested, T4 outside), the preflight refusals of (12) a +tracker store and (13) a workspace layout (T5), and (14) a worktree-isolated `sweep` +over a shared store (T2–T4): its bundle plan and ledger close committed in the store +repository, or `store-unversioned` for T2. Rows 7–9 run over T1 and T3. A run that finishes an epic it committed a ticket of always ends paused at `epic-boundary` for a person to close the epic. @@ -54,12 +59,14 @@ _store_repo, install_bmad_v7, install_build_auto_v7_skill, + install_sweep_skill, real_mux_e2e, remove_tree, ticket_slug, write_ticket_tree, ) +from bmad_loop import deferredwork from bmad_loop.frontmatter import parse_frontmatter, status_of # Linux-only, not merely non-win32: the fake is bash + GNU coreutils (`date +%s%N`). @@ -118,6 +125,28 @@ stop fi +# A sweep's triage (`/bmad-loop-sweep`): the triage adapter reads a real +# result.json, so write the partition (one bundle owning DW-1) and Stop. +if printf '%s' "$prompt" | grep -q "bmad-loop-sweep"; then + printf '%s' '{"workflow": "deferred-sweep-triage", "open_ids": ["DW-1"], "already_resolved": [], "bundles": [{"name": "fix-things", "dw_ids": ["DW-1"], "intent": "resolve DW-1"}], "blocked": [], "skip": [], "decisions": [], "escalations": []}' \ + > "$td/result.json" + stop +fi + +# A sweep bundle (`dw-`, free-form: no ticket, no `tickets.py`): edit code and +# leave `plan-.md` at `built` in the initiative folder the prompt names (the +# main checkout's store under a shared store), uncommitted — bmad-loop commits. +case "$ref" in + dw-*) + home=$(printf '%s' "$prompt" | sed -n 's/.*the initiative folder `\([^`]*\)`.*/\1/p' | head -1) + [ -n "$home" ] || home="$INIT" + echo "impl for $ref" >> src.txt + printf -- '---\ntitle: %s\nstatus: built\nbaseline_revision: %s\n---\n\n# %s plan\n\n## Auto Run Result\n\nStatus: built\n' \ + "$ref" "$baseline" "$ref" > "$home/plan-$ref.md" + printf '%s' "$(cd "$home" && pwd -P)/plan-$ref.md" > "$td/fake-plan.txt" + stop ;; +esac + # `ticket `: resolve the plan through the tree, as upstream step-01 does. found=$(tickets find "$ref") plan=$(printf '%s' "$found" | "$PY" -I -c 'import json, sys; print(json.load(sys.stdin)["plan"])') @@ -851,6 +880,124 @@ def test_e2e_tickets_multirepo_store_commits(project_v7, topology): assert doc["tickets"]["epic_closures"] == {EPIC: "accepted"} +SWEEP_POLICY = ( + '[adapter]\nname = "faketickets"\n\n' + "[review]\nenabled = false\n\n" + '[gates]\nmode = "none"\n\n' + '[scm]\nisolation = "worktree"\n' +) +SWEEP_LEDGER = ( + "# Deferred Work\n\n### DW-1: item DW-1\n\n" + "origin: test, 2026-06-01\nlocation: src.txt:1\nreason: test entry.\nstatus: open\n" +) + + +@pytest.mark.parametrize("topology", ["T2", "T3", "T4"]) +def test_e2e_tickets_isolated_sweep_shared_store(project_v7, topology): + """A v7 `bmad-loop sweep` under `isolation = "worktree"` over a store no + checkout carries (T2 ignored and unversioned, T3 its own nested repository, T4 + its own repository outside the project): the store is shared, so the bundle + prompt names the main checkout's initiative folder and the fake leaves its + `built` plan there, not in the unit mount. The unit's code merges back and DW-1 + is closed in the main ledger. T3/T4: once the code lands the plan gets its own + `chore(sweep): dw- plan [bmad-loop ]` commit in the store repository + and the bundle's ledger close its own `chore(sweep): close resolved + deferred-work entries` commit there, each touching that file alone with the + `Bmad-Code-Commit` trailer naming the merged unit commit, so both repositories + end clean. T2: nothing to commit the plan in, so `store-unversioned` is + journaled once, and no close is withheld or failed. The run must complete: + `sweep` exits 0 on a pause too. + + ABLATIONS (performed, singly): A1 drop the `_publish_bundle_plan` call in + `SweepEngine._land_post_landing_writes` and T3/T4 fail (no plan commit, the + store left dirty) and T2 too (no `store-unversioned`); A2 drop the shared-store + redirect (`with_shared_store` in `SweepEngine.__init__`) and T2/T3 fail (the + prompt names the unit mount's folder, so the fake writes its plan there; T4's + absolute store is unaffected by design); A3 drop the shared-store arm of + `WorktreeFlow.finish_publication` and all three fail (the run pauses on + `artifact publication intent is missing` after the merge); A4 drop the + `_publish_bundle_close` call and T3/T4 fail (no ledger close commit, the + store's ledger left dirty).""" + layout = _scaffold( + project_v7, _epic(EntrySpec(1, "Cart model")), topology=topology, isolation="worktree" + ) + root, store = layout.project, layout.output_folder + install_sweep_skill(root) + (root / ".bmad-loop" / "policy.toml").write_text(SWEEP_POLICY, encoding="utf-8") + _git(root, "add", "-A") + _git(root, "commit", "-q", "-m", "sweep policy") + ledger = layout.initiative_folder / "deferred-work.md" + ledger.write_text(SWEEP_LEDGER, encoding="utf-8") + if topology == "T2": + _git( + root, "check-ignore", "-q", ledger.relative_to(root).as_posix() + ) # raises unless ignored + else: + _git(store, "add", "--", ledger.relative_to(store).as_posix()) + _git(store, "commit", "-q", "-m", "ledger") + assert _clean(store) + assert _clean(root) + base = _commit_count(root) + + _ok(_run(root, "sweep", "--no-prompt")) + + run_id = _run_dir(root).name + assert _journal(root, "run-paused") == [], _journal(root, "run-paused") # rc 0 on a pause too + assert len(_journal(root, "run-complete")) == 1 + key = "dw-fix-things" + (cwd,) = [ + Path(p.read_text(encoding="utf-8")) + for p in _run_dir(root).glob(f"tasks/{key}-*/fake-cwd.txt") + ] + assert cwd.is_relative_to(_run_dir(root) / "worktrees"), cwd # the session ran in the mount + plan = layout.initiative_folder / f"plan-{key}.md" + # ...and wrote its plan straight into the main checkout's store, never the mount + (written,) = [ + Path(p.read_text(encoding="utf-8")) + for p in _run_dir(root).glob(f"tasks/{key}-*/fake-plan.txt") + ] + assert written == plan.resolve(), written + assert status_of(parse_frontmatter(plan.read_text(encoding="utf-8"))) == "built" + (entry,) = deferredwork.parse_ledger(ledger.read_text(encoding="utf-8")) + assert entry.status.startswith("done"), entry.status + assert "impl for dw-fix-things" in (root / "src.txt").read_text(encoding="utf-8") + assert _commit_count(root) > base + unit = _git(root, "show", "--name-only", "--format=", "HEAD^2").split() # the merged unit + assert unit == ["src.txt"], unit # the plan rides no code commit + assert _git(root, "log", "-1", "--format=%s", "HEAD^2").startswith(f"sweep {key}") + assert not any(s.startswith("chore(sweep)") for s in _subjects(root)), _subjects(root) + for kind in ( + "sweep-ledger-commit-withheld", + "store-commit-failed", + "store-commit-foreign-dirt", + ): + assert _journal(root, kind) == [], kind + if topology == "T2": + assert [row["layout"] for row in _journal(root, "store-unversioned")] == ["T2"] + assert _journal(root, "store-committed") == [] + else: + plan_rel = plan.relative_to(store).as_posix() + commits = _git(store, "log", "--format=%H", "--", plan_rel).split() + assert len(commits) == 1, commits + subject = _git(store, "log", "-1", "--format=%s", commits[0]).strip() + assert subject == f"chore(sweep): {key} plan [bmad-loop {run_id}]" + assert _git(store, "show", "--name-only", "--format=", commits[0]).split() == [plan_rel] + ledger_rel = ledger.relative_to(store).as_posix() + log = [line.split(" ", 1) for line in _git(store, "log", "--format=%H %s").splitlines()] + (close,) = [ + sha for sha, s in log if s == "chore(sweep): close resolved deferred-work entries" + ] + assert _git(store, "show", "--name-only", "--format=", close).split() == [ledger_rel] + unit_sha = _git(root, "rev-parse", "HEAD^2").strip() + assert f"Bmad-Code-Commit: {unit_sha}" in _git(store, "log", "-1", "--format=%B", close) + assert [row["commit"] for row in _journal(root, "store-committed")] == [commits[0], close] + assert _journal(root, "store-unversioned") == [] + assert _clean(store) + assert _git(root, "branch", "--list", "bmad-loop/*").strip() == "" + assert _git(root, "worktree", "list", "--porcelain").count("worktree ") == 1 + assert _clean(root) + + def _validate_problems(root: Path) -> set[str]: proc = _run(root, "validate", "--json") doc = json.loads(proc.stdout) diff --git a/tests/test_worktree_flow.py b/tests/test_worktree_flow.py index 3e6e39ae4..252d1847e 100644 --- a/tests/test_worktree_flow.py +++ b/tests/test_worktree_flow.py @@ -11,6 +11,7 @@ import json import os import shutil +from dataclasses import replace from pathlib import Path from types import SimpleNamespace @@ -984,6 +985,34 @@ def test_payload_replay_drops_the_snapshot_once_the_worktree_is_gone(tmp_path): assert flow.journal.entries == [] +def test_finish_publication_latches_a_shared_store_bundle_without_publishing(tmp_path, monkeypatch): + """An artifacts dir in the shared ticket store has nothing to publish: the + mount wrote the main checkout's one copy in place and `prepare_publication` + recorded no intent. `finish_publication` latches the bundle's publication + complete and saves, never calling `artifact_publication.publish` (which would + refuse the missing intent and pause the run after its merge). + + Ablation (performed): drop the shared-store arm of `finish_publication` and + `publish` is called (this row's stub raises).""" + flow = _artifact_flow(tmp_path) + flow.paths = replace(flow.paths, shared_store=flow.paths.project / "_bmad-output") + assert flow.paths.in_shared_store(flow.paths.implementation_artifacts) # premise + + def no_publish(task, paths): + raise AssertionError("published a shared-store bundle") + + monkeypatch.setattr(artifact_publication, "publish", no_publish) + task = StoryTask(story_key="dw-fix", epic=0, dw_ids=["DW-1"]) + task.worktree_path = str(tmp_path / "wt") + assert task.artifact_payload is None and not task.artifact_publication_complete + + flow.finish_publication(task, None) + + assert task.artifact_publication_complete + assert flow.calls.saves == 1 + assert flow.calls.pauses == [] and flow.journal.entries == [] + + # --------------------------------------------------------------- pinned config edits # DW-368: a pinned (skip-worktree) tracked hook config hides a story's own edit from # the unit commit, so success teardown compares it with the recorded rewrite and From 4ac1dc7c0749bed9b1d6a81e228a7a73409625c6 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 18 Oct 2026 11:27:13 -0700 Subject: [PATCH 02/10] sweep dw2-sweep-deferred-store-ledger-publish: DW-567 via bmad-loop --- CHANGELOG.md | 4 + docs/sweep-guide.md | 4 +- src/bmad_loop/engine.py | 4 +- src/bmad_loop/sweep.py | 97 ++++++++++- tests/test_sweep.py | 352 +++++++++++++++++++++++++++++++++++++++- 5 files changed, 449 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec682fb10..ede2964aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -157,6 +157,10 @@ breaking changes may land in a minor release. - Commit a v7 sweep bundle's ledger close in the ticket store's own repository (T3/T4) once its code lands, best effort with `Bmad-Code-*` trailers and withheld while the ledger is in doubt, instead of leaving the store's ledger dirty (DW-556). +- Commit a deferred v7 sweep bundle's ledger writes (its reopened close and harvested + deferrals) in the ticket store's own repository (T3/T4), best effort and withheld while + the ledger is in doubt; an isolated defer over a shared store reopens the bundle's close + first (DW-567). - Latch a worktree-isolated sweep bundle's artifact publication complete when its artifacts dir is in a shared ticket store, instead of pausing on `artifact publication intent is missing` after the merge (DW-557). diff --git a/docs/sweep-guide.md b/docs/sweep-guide.md index 41bc7af32..93217717a 100644 --- a/docs/sweep-guide.md +++ b/docs/sweep-guide.md @@ -90,8 +90,9 @@ A sweep runs on a v7 project the same way, with these differences: - **Store layout.** The store-topology check judges a v7 sweep as a tickets run, because its ledger lives in the ticket store. The sweep records the store's topology at its start, and a resume refuses a store that changed since, or whose initiative folder is gone. - **Pinned initiative.** A sweep records the active initiative it started on. A resume, `resolve` or the dashboard's re-arm refuses while `[core] active_initiative` names another: set it back, or start a new sweep. - **Decisions per initiative.** Each initiative has its own ledger and DW numbering, so pre-answers are kept per initiative in `.bmad-loop/decisions-.json`. `bmad-loop decisions` and the TUI answer the active initiative's decisions and offer only those its own sweeps raised. An answer given after another initiative was made active than the one its decision was listed from is refused; list the decisions again. With no initiative active, the project store stays `.bmad-loop/decisions.json` and only sweeps that ran with no initiative are offered. -- **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. +- **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. The bundle's ledger close lands in that shared ledger the same way, so a bundle that defers reopens its close before the defer is recorded. - **Plans in a store repository.** When the ticket store is its own git repository (nested or outside the checkout), the bundle's code commit cannot carry its plan. Once the code has landed, the accepted plan gets its own commit there, `chore(sweep): plan [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code. This is best effort, like a tickets run's store commit: a failure is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and the plan stays uncommitted for a human. The bundle's ledger close is committed there the same best-effort way, as `chore(sweep): close resolved deferred-work entries` with the same trailers and the same journaled failures, and is withheld (`sweep-ledger-commit-withheld`) while the run holds its ledger in doubt. +- **Deferred bundles in a store repository.** A bundle that defers never lands code, but its ledger writes (the reopen of its close, any harvested deferrals) are committed in the store repository as `chore(sweep): record a deferred bundle's ledger writes`, the same best-effort and withheld way, with only the `Bmad-Code-Repo` trailer. ## Story-declared closure (`closes_deferred`) @@ -182,6 +183,7 @@ The sweep commits each ledger or store change it makes as its own file-scoped co | --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | | `chore(sweep): record deferred-work decisions` | after the decision phase | | `chore(sweep): close resolved deferred-work entries` | already-resolved entries and decided closes; a landed bundle's close when the ledger is in a store repository | +| `chore(sweep): record a deferred bundle's ledger writes` | a deferred bundle's reopened close and harvested deferrals when the ledger is in a store repository | | `chore(sweep): commit ledger at the sweep cycle boundary` | between repeat cycles | | `chore(sweep): commit ledger after recovering in-flight bundles` | on resume, after in-flight bundles are recovered | | `chore(sweep): commit a ledger write an interrupted phase left unpublished` | on resume, for a write an interrupted phase did not commit | diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 9968dd6b1..d1416a8ca 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -10047,7 +10047,9 @@ def _reopen_ledger_after_defer(self, task: StoryTask) -> None: No-op on the base path; only ``SweepEngine`` writes reopenable bundle closes. This is reached on the in-place branch after a reset was attempted and the ledger was restored. An isolated defer returns earlier because the - unit worktree, including its close, is discarded without merging. + unit worktree, including its close, is discarded without merging — unless + the ledger lives in a shared ticket store, where ``SweepEngine._defer`` + calls this itself before the base defer. """ return diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 6583a1aaa..065d60539 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -7450,7 +7450,8 @@ def _withhold_ledger_publish(self, message: str, *, dw_ids: list[str] | None = N list of ids the declining publisher would otherwise have set out to prove, emitted ONLY when given: `_publish_stranded_close` names its cached plan's already-resolved and decision ids, `_publish_bundle_close` (DW-556) the - landed bundle's `bundle_closes_intended`, while the three DW-246 callers + landed bundle's `bundle_closes_intended`, `_publish_deferred_ledger` + (DW-567) the deferred bundle's `dw_ids`, while the three DW-246 callers deliberately pass none — `_close_resolved`'s arms have `closed` and `ids` in scope, but their DW-246 row shape stays unchanged. `diagnostics` already routes the field by name (`_JOURNAL_KEYLIST_FIELDS`). @@ -8821,14 +8822,97 @@ def _publish_bundle_close(self, task: StoryTask) -> None: if self._ledger_unfit_to_publish(): self._withhold_ledger_publish(message, dw_ids=list(task.bundle_closes_intended)) return - committer = ticketstore.StoreCommitter(topology.store_repo, self.paths.repo_root) + self._commit_store_ledger( + task, + topology.store_repo, + ledger, + message, + code_commit=self._landed_code_commit(task), + code_baseline=task.baseline_commit, + ) + + def _defer(self, task: StoryTask, reason: str) -> None: + """The base defer, then publish the ledger writes it left in a store + repository (T3/T4, DW-567) through :meth:`_publish_deferred_ledger`. A + deferred bundle never reaches ``_land_post_landing_writes``, so without + this its reopened close and harvested deferrals stay dirty in the store. + + On the isolated arm (``self._isolated or task.worktree_path``, the base's + own test) with the ledger in the shared ticket store, the bundle's close + was written straight into the one ledger every checkout shares: the + base's premise that the discarded worktree takes its close with it does + not hold, so the close is reopened FIRST, ahead of the base's terminal + save, and the publish never commits a ``done`` for discarded code. A raise + from that reopen propagates before any defer is recorded, so the task is + not yet terminal: resume takes ``_recover_inflight_bundle``'s restart arm + (a re-verify that defers again, or a re-drive), and a later defer of this + bundle undoes the same close, its operation id being per run and bundle. + An unshared ledger copy dies with the worktree, as before. A raise from + the base (a ``RunPaused`` from its rollback included) propagates + untouched, with no publish.""" + if (self._isolated or task.worktree_path) and self.paths.in_shared_store( + self.paths.deferred_work + ): + self._reopen_ledger_after_defer(task) + super()._defer(task, reason) + self._publish_deferred_ledger(task) + + def _publish_deferred_ledger(self, task: StoryTask) -> None: + """Commit the ledger writes a DEFERRED bundle left when the ledger lives in + a store repository (T3/T4), through + :class:`~bmad_loop.ticketstore.StoreCommitter` as ``chore(sweep): record a + deferred bundle's ledger writes``: the reopen of its close and any + harvested deferrals. No code landed, so only ``Bmad-Code-Repo`` is + stamped. Elsewhere the ledger is in the code checkout (or a worktree copy + that dies with the unit) and no store commit is owed. + + Withheld while the run holds its ledger in doubt + (``sweep-ledger-commit-withheld`` naming the bundle's ids). Otherwise best + effort, like :meth:`_publish_bundle_close`: a failed commit journals + ``store-commit-failed`` (a staged foreign version of the ledger + ``store-commit-foreign-dirt``) and the writes stay uncommitted for a human + or a later publisher; a ledger already at the store's HEAD journals + nothing.""" + topology = self.store_topology + if topology is None or topology.store_repo is None: + return + ledger = self.paths.deferred_work + try: + inside = ledger.resolve().is_relative_to(topology.store_repo.resolve()) + except (OSError, RuntimeError, ValueError): + inside = False + if not inside or not ledger.is_file(): + return + message = "chore(sweep): record a deferred bundle's ledger writes" + if self._ledger_unfit_to_publish(): + self._withhold_ledger_publish(message, dw_ids=list(task.dw_ids)) + return + self._commit_store_ledger(task, topology.store_repo, ledger, message, code_commit=None) + + def _commit_store_ledger( + self, + task: StoryTask, + store_repo: Path, + ledger: Path, + message: str, + *, + code_commit: str | None, + code_baseline: str | None = None, + ) -> None: + """The best-effort store commit both ledger publishers share: commit + ``ledger`` alone in ``store_repo`` under ``message`` and journal the + outcome — ``store-committed`` for a commit, ``store-commit-failed`` for a + :class:`~bmad_loop.ticketstore.StoreCommitError`, + ``store-commit-foreign-dirt`` for a staged foreign version, and nothing + when the ledger is already at the store's HEAD.""" + committer = ticketstore.StoreCommitter(store_repo, self.paths.repo_root) try: result = committer.commit( task.story_key, [ledger], message, - code_commit=self._landed_code_commit(task), - code_baseline=task.baseline_commit, + code_commit=code_commit, + code_baseline=code_baseline, ) except ticketstore.StoreCommitError as e: self.journal.append("store-commit-failed", story_key=task.story_key, error=str(e)) @@ -9132,7 +9216,10 @@ def _reopen_ledger_after_defer(self, task: StoryTask) -> None: ``Engine._defer`` deliberately restores the whole post-review ledger after rollback so harvested findings survive. That restore can also replay a - bundle close whose code no longer exists. The operation-specific undo marker + bundle close whose code no longer exists. :meth:`_defer` also calls this on + the isolated arm when the ledger is in the shared ticket store, where the + mount wrote its close straight into the one ledger every checkout shares + (DW-567). The operation-specific undo marker makes this "undo my close", not "open these ids": human, legacy, and earlier run closures remain untouched. Replaying this method is idempotent. """ diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 9a71ced81..01d0f408e 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -4513,6 +4513,8 @@ def test_bundle_ledger_close_reopened_when_review_disabled_defers_at_gate(projec assert "change for dw-fix" not in (project.project / "src.txt").read_text() assert deferredwork.open_ids(project.deferred_work.read_text(encoding="utf-8")) == {"DW-1"} assert "sweep-bundle-reopened" in kinds + # no store repository (v6): the deferred-bundle store publisher stays silent (DW-567) + assert "store-committed" not in kinds and "sweep-ledger-commit-withheld" not in kinds def test_bundle_pre_gate_state_sync_is_a_noop(project): @@ -37915,17 +37917,20 @@ def _v7_plan(paths: ProjectPaths, name: str) -> Path: return paths.implementation_artifacts / f"plan-dw-{name}.md" -def _v7_bundle_dev_effect(paths: ProjectPaths, name: str, dw_ids, *, followup_review: bool): +def _v7_bundle_dev_effect( + paths: ProjectPaths, name: str, dw_ids, *, followup_review: bool, deferred=None +): """A v7 build-auto bundle session: it edits code and finishes its plan at `built`, never `done` (the orchestrator owns `done`, and a bundle never gets - it). It leaves the ledger alone.""" + it). It leaves the ledger alone; ``deferred`` lists the plan's `deferred:` + findings, which the orchestrator harvests into the ledger.""" def effect(spec): baseline = rev_parse_head(paths.project) source = paths.project / "src.txt" source.write_text(source.read_text(encoding="utf-8") + f"change for dw-{name}\n", "utf-8") plan = _v7_plan(paths, name) - write_spec(plan, "built", baseline) + write_spec(plan, "built", baseline, deferred=deferred) return SessionResult( status="completed", result_json={ @@ -38356,8 +38361,9 @@ def test_v7_sweep_bundle_close_store_commit_failure_is_best_effort( close stays uncommitted in the store. ABLATIONS (performed): drop the `except ticketstore.StoreCommitError` clause - in `_publish_bundle_close` and `git` fails (the run crashes); drop its - `if result.foreign:` arm and `foreign` fails (no foreign-dirt row).""" + in `_commit_store_ledger` (the tail `_publish_bundle_close` shares) and `git` + fails (the run crashes); drop its `if result.foreign:` arm and `foreign` + fails (no foreign-dirt row).""" if fault == "git": real = ticketstore.StoreCommitter.commit @@ -38384,6 +38390,342 @@ def commit(self, unit_id, paths, message, **kwargs): assert git(store, "status", "--porcelain", "--", ledger_rel) != "" # still unpublished +_V7_LEDGER_DEFERRED = "chore(sweep): record a deferred bundle's ledger writes" + + +def _v7_store_deferred_sweep( + v7, + layout, + marker: Path, + *, + arm_doubt: bool = False, + stage_foreign_ledger: bool = False, + rollback: bool = True, +): + """A v7 sweep over a store with its own repository (``layout`` T3/T4) whose + ledger, `{DW-1: open}`, is committed in that store: triage bundles DW-1 + (`fix-things`), its dev session is accepted (the close is written, and the + plan's `deferred:` finding is harvested into the ledger as DW-2), and the + post-accept verify gate (``passes_once(marker)``) fails, so the bundle defers + IN PLACE with one dev attempt. The defer's reopen restores DW-1's bytes + exactly, so the harvested DW-2 is the ledger delta left for the store. + ``arm_doubt`` / ``stage_foreign_ledger`` act from inside the dev session as in + :func:`_v7_store_ledger_sweep`; ``rollback=False`` turns + ``scm.rollback_on_failure`` off, so the defer pauses before its reset. + Returns the engine, its paths, the store and the ledger's store-relative + path.""" + store = v7.output_folder + paths = _v7_paths(v7) + write_ledger(paths, {"DW-1": "open"}, commit=False) + git(store, "add", "-A") + git(store, "commit", "-q", "-m", "ledger") + ledger_rel = paths.deferred_work.resolve().relative_to(store.resolve()).as_posix() + assert git(store, "ls-files", "--", ledger_rel).strip() == ledger_rel # premise + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + topology = ticketstore.assess_store(paths, "none") + assert isinstance(topology, ticketstore.StoreTopology) and topology.layout == layout + triage = triage_result( + ["DW-1"], bundles=[{"name": "fix-things", "dw_ids": ["DW-1"], "intent": "fix it"}] + ) + dev = _v7_bundle_dev_effect( + paths, "fix-things", ["DW-1"], followup_review=False, deferred=[SWEEP_FINDING] + ) + holder: list[SweepEngine] = [] + + def dev_effect(spec): + if arm_doubt: + holder[0]._record_ledger_doubt() + if stage_foreign_ledger: + ledger = paths.deferred_work + original = ledger.read_bytes() + ledger.write_bytes(original + b"\nhand-edited\n") + git(store, "add", "--", ledger_rel) + ledger.write_bytes(original) + return dev(spec) + + policy = Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + review=ReviewPolicy(enabled=False), + dev=DevPolicy(skill="bmad-dev-auto"), + scm=ScmPolicy(rollback_on_failure=rollback), + verify=VerifyPolicy(commands=(passes_once(marker),)), + limits=LimitsPolicy(max_dev_attempts=1), + ) + engine, _ = make_sweep( + paths, + [triage_effect(triage), dev_effect], + policy=policy, + store_topology=topology.to_dict(), + ) + holder.append(engine) + return engine, paths, store, ledger_rel + + +def _store_deferred_commits(store: Path) -> list[str]: + """The shas of the store's deferred-bundle ledger commits, newest first.""" + log = [line.split(" ", 1) for line in git(store, "log", "--format=%H %s").splitlines()] + return [sha for sha, subject in log if subject == _V7_LEDGER_DEFERRED] + + +def _assert_deferred_in_place(engine, paths) -> None: + """Premise of the deferred-bundle rows: the bundle's close was written after + an accepted dev session that harvested DW-2, then the bundle deferred in + place and reopened its close, keeping the harvest.""" + task = engine.state.tasks["dw-fix-things"] + assert task.phase == Phase.DEFERRED and not task.worktree_path + assert task.bundle_closes_intended == ["DW-1"] + assert [r["dw_ids"] for r in _records(engine, "sweep-bundle-closed")] == [["DW-1"]] + assert [r["dw_ids"] for r in _records(engine, "sweep-bundle-reopened")] == [["DW-1"]] + entries = ledger_entries(paths) + assert entries["DW-1"].open and entries["DW-2"].open + assert entries["DW-2"].title == SWEEP_FINDING["summary"] + + +@pytest.mark.parametrize( + ("fixture", "layout"), + [("project_v7_store_repo", "T3"), ("project_v7_external_store", "T4")], +) +def test_v7_deferred_bundle_ledger_writes_committed_in_store_repo( + request, tmp_path, fixture, layout +): + """DW-567. A v7 bundle that DEFERS never reaches `_land_post_landing_writes`, + so `_publish_bundle_close` never runs for it; with the ledger in a store that + has its own repository (T3 nested, T4 outside) the close-undo the defer + wrote and the deferrals it harvested would stay dirty in the store. + `SweepEngine._defer` publishes them: one store commit touching only the + ledger, carrying `Bmad-Code-Repo` and no `Bmad-Code-Commit` (no code landed), + leaving the store's ledger clean with DW-1 reopened and the harvested DW-2 at + HEAD. + + ABLATION (performed): drop the `self._publish_deferred_ledger(task)` call in + `SweepEngine._defer` and both rows fail (no deferred-bundle commit; the + ledger stays dirty in the store).""" + engine, paths, store, ledger_rel = _v7_store_deferred_sweep( + request.getfixturevalue(fixture), layout, tmp_path / "verify-ran" + ) + + summary = engine.run() + + assert summary.deferred == 1 and not summary.crashed and not summary.paused + _assert_deferred_in_place(engine, paths) + commits = _store_deferred_commits(store) + assert len(commits) == 1, git(store, "log", "--format=%s") + assert git(store, "show", "--name-only", "--format=", commits[0]).split() == [ledger_rel] + body = git(store, "log", "-1", "--format=%B", commits[0]) + assert f"Bmad-Code-Repo: {paths.repo_root.as_posix()}" in body + assert "Bmad-Code-Commit" not in body and "Bmad-Code-Range" not in body + head_ledger = git(store, "show", f"{commits[0]}:{ledger_rel}") + assert deferredwork.open_ids(head_ledger) == {"DW-1", "DW-2"} + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + assert [r["commit"] for r in _records(engine, "store-committed")] == commits + + +def test_v7_deferred_bundle_ledger_publish_withheld_while_in_doubt(project_v7_store_repo, tmp_path): + """T3: a run that holds its ledger in doubt (armed during the bundle's dev + session) does not publish the deferred bundle's ledger writes: it journals + `sweep-ledger-commit-withheld` naming the deferred-bundle message and the + bundle's ids, and no store commit is made. + + ABLATION (performed): drop the `if self._ledger_unfit_to_publish():` gate in + `_publish_deferred_ledger` and this fails (no withheld row; the + deferred-bundle commit lands in the store).""" + engine, paths, store, ledger_rel = _v7_store_deferred_sweep( + project_v7_store_repo, "T3", tmp_path / "verify-ran", arm_doubt=True + ) + + engine.run() + + _assert_deferred_in_place(engine, paths) + withheld = [ + r + for r in _records(engine, "sweep-ledger-commit-withheld") + if r["message"] == _V7_LEDGER_DEFERRED + ] + assert len(withheld) == 1, _records(engine, "sweep-ledger-commit-withheld") + assert withheld[0]["dw_ids"] == ["DW-1"] and withheld[0]["reason"] == "ledger-in-doubt" + assert _store_deferred_commits(store) == [] + assert git(store, "status", "--porcelain", "--", ledger_rel) != "" # still unpublished + + +@pytest.mark.parametrize("fault", ["git", "foreign"]) +def test_v7_deferred_bundle_ledger_publish_failure_is_best_effort( + project_v7_store_repo, tmp_path, monkeypatch, fault +): + """T3: the deferred bundle's ledger writes fail to commit in the store + repository (a git fault, or a staged foreign version of the ledger). The + publish is best effort: the run neither crashes nor pauses, it journals + `store-commit-failed` / `store-commit-foreign-dirt` for the bundle, and the + writes stay uncommitted in the store. + + ABLATIONS (performed): drop the `except ticketstore.StoreCommitError` clause + in `_commit_store_ledger` and `git` fails (the run crashes); drop its + `if result.foreign:` arm and `foreign` fails (no foreign-dirt row).""" + if fault == "git": + real = ticketstore.StoreCommitter.commit + + def commit(self, unit_id, paths, message, **kwargs): + if message == _V7_LEDGER_DEFERRED: + raise ticketstore.StoreCommitError(f"{unit_id}: index.lock exists") + return real(self, unit_id, paths, message, **kwargs) + + monkeypatch.setattr(ticketstore.StoreCommitter, "commit", commit) + engine, paths, store, ledger_rel = _v7_store_deferred_sweep( + project_v7_store_repo, + "T3", + tmp_path / "verify-ran", + stage_foreign_ledger=fault == "foreign", + ) + + summary = engine.run() + + assert not summary.crashed and not summary.paused + _assert_deferred_in_place(engine, paths) + kind = "store-commit-failed" if fault == "git" else "store-commit-foreign-dirt" + assert [r["story_key"] for r in _records(engine, kind)] == ["dw-fix-things"] + assert _store_deferred_commits(store) == [] + assert git(store, "status", "--porcelain", "--", ledger_rel) != "" # still unpublished + + +def test_v7_deferred_bundle_paused_rollback_publishes_nothing(project_v7_store_repo, tmp_path): + """T3 with `scm.rollback_on_failure` off: the defer's rollback pauses before + the reset, so the bundle's close is never reopened. The `RunPaused` from the + base defer propagates with no publish: no deferred-bundle commit, and the + store's HEAD still holds DW-1 open (the un-reopened `done` stays + uncommitted for the human the pause hands back to). + + ABLATION (performed): run `self._publish_deferred_ledger(task)` in a + `finally:` around `super()._defer` and this fails (the store commits DW-1 + `done` for code the defer discarded).""" + engine, paths, store, ledger_rel = _v7_store_deferred_sweep( + project_v7_store_repo, "T3", tmp_path / "verify-ran", rollback=False + ) + + summary = engine.run() + + assert summary.paused and not summary.crashed + assert engine.state.tasks["dw-fix-things"].phase == Phase.DEFERRED + assert [r["dw_ids"] for r in _records(engine, "sweep-bundle-closed")] == [["DW-1"]] + assert _records(engine, "sweep-bundle-reopened") == [] + assert _store_deferred_commits(store) == [] + assert deferredwork.open_ids(git(store, "show", f"HEAD:{ledger_rel}")) == {"DW-1"} + + +def test_v7_isolated_deferred_bundle_reopens_its_shared_store_close(project_v7_store_repo): + """T3 under isolation, driven at the seam: a mounted bundle's close went + straight into the ledger in the SHARED store (the mount does not carry its + own copy), so the base's isolated arm, which trusts the discarded worktree to + take the close with it, would leave DW-1 `done` for code nobody kept. + `SweepEngine._defer` reopens the close first (`sweep-bundle-reopened`) and + then publishes, so the store's HEAD holds DW-1 open (the reopen restores the + committed bytes exactly, so nothing is left to commit) and the store's ledger + is clean. + + ABLATION (performed): drop the isolated-arm `_reopen_ledger_after_defer` call + in `SweepEngine._defer` and this fails (no reopen row; the publish commits + DW-1 `done` to the store's HEAD).""" + v7 = project_v7_store_repo + store = v7.output_folder + paths = _v7_paths(v7) + write_ledger(paths, {"DW-1": "open"}, commit=False) + git(store, "add", "-A") + git(store, "commit", "-q", "-m", "ledger") + ledger_rel = paths.deferred_work.resolve().relative_to(store.resolve()).as_posix() + topology = ticketstore.assess_store(paths, "none") + assert isinstance(topology, ticketstore.StoreTopology) and topology.layout == "T3" + engine, _ = make_sweep(paths, [], store_topology=topology.to_dict()) + # premise: the ledger lives in the shared store every checkout reads + assert engine.paths.in_shared_store(engine.paths.deferred_work) + task = StoryTask( + story_key="dw-fix-things", + epic=0, + dw_ids=["DW-1"], + phase=Phase.DEV_VERIFY, + worktree_path=str(paths.repo_root.parent / "unit"), + ) + engine.state.tasks[task.story_key] = task + deferredwork.mark_done_many_reopenable( + engine.paths.deferred_work, + task.dw_ids, + engine._today(), + engine._bundle_close_note(task), + engine._bundle_close_operation_id(task), + ) + assert not ledger_entries(paths)["DW-1"].open # premise: the close is in the main ledger + + engine._defer(task, "verify failed") + + assert task.phase == Phase.DEFERRED + assert [r["dw_ids"] for r in _records(engine, "sweep-bundle-reopened")] == [["DW-1"]] + assert ledger_entries(paths)["DW-1"].open + assert deferredwork.open_ids(git(store, "show", f"HEAD:{ledger_rel}")) == {"DW-1"} + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + assert _records(engine, "store-committed") == [] # already at HEAD: nothing journaled + + +def test_v7_isolated_deferred_bundle_publishes_its_shared_store_ledger(project_v7_store_repo): + """T3 under isolation, driven at the seam, with a real delta: the mounted + bundle's close of DW-1 is in the shared store's ledger and the task carries a + harvested deferral, which the base's isolated arm files into that ledger as + DW-2. `SweepEngine._defer` reopens DW-1 and publishes: exactly one + deferred-bundle store commit touching only the ledger, holding DW-1 and DW-2 + open, the store's ledger clean, and its `store-committed` row naming it. + + ABLATION (performed): gate the `_publish_deferred_ledger` call in + `SweepEngine._defer` to the in-place arm only and this fails (no + deferred-bundle commit; the ledger stays dirty in the store).""" + v7 = project_v7_store_repo + store = v7.output_folder + paths = _v7_paths(v7) + write_ledger(paths, {"DW-1": "open"}, commit=False) + git(store, "add", "-A") + git(store, "commit", "-q", "-m", "ledger") + ledger_rel = paths.deferred_work.resolve().relative_to(store.resolve()).as_posix() + topology = ticketstore.assess_store(paths, "none") + assert isinstance(topology, ticketstore.StoreTopology) and topology.layout == "T3" + engine, _ = make_sweep(paths, [], store_topology=topology.to_dict()) + assert engine.paths.in_shared_store(engine.paths.deferred_work) # premise + task = StoryTask( + story_key="dw-fix-things", + epic=0, + dw_ids=["DW-1"], + phase=Phase.DEV_VERIFY, + worktree_path=str(paths.repo_root.parent / "unit"), + harvested_deferrals=[ + { + "origin": "spec-deferred plan-dw-fix-things.md", + "title": SWEEP_FINDING["summary"], + "reason": SWEEP_FINDING["evidence"], + "location": SWEEP_FINDING["location"], + "severity": SWEEP_FINDING["severity"], + "source_spec": "plan-dw-fix-things.md", + } + ], + ) + engine.state.tasks[task.story_key] = task + deferredwork.mark_done_many_reopenable( + engine.paths.deferred_work, + task.dw_ids, + engine._today(), + engine._bundle_close_note(task), + engine._bundle_close_operation_id(task), + ) + assert not ledger_entries(paths)["DW-1"].open # premise: the close is in the main ledger + + engine._defer(task, "verify failed") + + assert task.phase == Phase.DEFERRED + entries = ledger_entries(paths) + assert entries["DW-1"].open and entries["DW-2"].open + commits = _store_deferred_commits(store) + assert len(commits) == 1, git(store, "log", "--format=%s") + assert git(store, "show", "--name-only", "--format=", commits[0]).split() == [ledger_rel] + head_ledger = git(store, "show", f"{commits[0]}:{ledger_rel}") + assert deferredwork.open_ids(head_ledger) == {"DW-1", "DW-2"} + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + assert [r["commit"] for r in _records(engine, "store-committed")] == commits + + @pytest.mark.parametrize("publisher", ["plan", "close"]) def test_v7_bundle_store_trailers_name_the_landed_squash_commit(project_v7_store_repo, publisher): """Under worktree isolation a `squash` merge-back lands a different commit From 47eabc4ebe4c3541ae74683d82cd17a17784e87b Mon Sep 17 00:00:00 2001 From: t Date: Sun, 18 Oct 2026 12:17:21 -0700 Subject: [PATCH 03/10] sweep dw3-harvest-carry-store-repo-arm: DW-570 via bmad-loop --- CHANGELOG.md | 3 + docs/sweep-guide.md | 27 ++-- docs/tickets-guide.md | 2 +- src/bmad_loop/engine.py | 113 ++++++++++++++++- src/bmad_loop/sweep.py | 75 ++++++----- src/bmad_loop/tickets_engine.py | 11 ++ tests/test_engine_worktree.py | 113 +++++++++++++++++ tests/test_sweep.py | 214 +++++++++++++++++++++++++++++++- 8 files changed, 500 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ede2964aa..458defa64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -161,6 +161,9 @@ breaking changes may land in a minor release. deferrals) in the ticket store's own repository (T3/T4), best effort and withheld while the ledger is in doubt; an isolated defer over a shared store reopens the bundle's close first (DW-567). +- Commit an isolated unit's harvested deferrals in the ticket store's own repository (T3/T4) + when no store publisher follows, best effort, instead of journaling + `harvest-carry-uncommitted` for a store path the code repository cannot commit (DW-570). - Latch a worktree-isolated sweep bundle's artifact publication complete when its artifacts dir is in a shared ticket store, instead of pausing on `artifact publication intent is missing` after the merge (DW-557). diff --git a/docs/sweep-guide.md b/docs/sweep-guide.md index 93217717a..0dff789e2 100644 --- a/docs/sweep-guide.md +++ b/docs/sweep-guide.md @@ -92,7 +92,7 @@ A sweep runs on a v7 project the same way, with these differences: - **Decisions per initiative.** Each initiative has its own ledger and DW numbering, so pre-answers are kept per initiative in `.bmad-loop/decisions-.json`. `bmad-loop decisions` and the TUI answer the active initiative's decisions and offer only those its own sweeps raised. An answer given after another initiative was made active than the one its decision was listed from is refused; list the decisions again. With no initiative active, the project store stays `.bmad-loop/decisions.json` and only sweeps that ran with no initiative are offered. - **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. The bundle's ledger close lands in that shared ledger the same way, so a bundle that defers reopens its close before the defer is recorded. - **Plans in a store repository.** When the ticket store is its own git repository (nested or outside the checkout), the bundle's code commit cannot carry its plan. Once the code has landed, the accepted plan gets its own commit there, `chore(sweep): plan [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code. This is best effort, like a tickets run's store commit: a failure is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and the plan stays uncommitted for a human. The bundle's ledger close is committed there the same best-effort way, as `chore(sweep): close resolved deferred-work entries` with the same trailers and the same journaled failures, and is withheld (`sweep-ledger-commit-withheld`) while the run holds its ledger in doubt. -- **Deferred bundles in a store repository.** A bundle that defers never lands code, but its ledger writes (the reopen of its close, any harvested deferrals) are committed in the store repository as `chore(sweep): record a deferred bundle's ledger writes`, the same best-effort and withheld way, with only the `Bmad-Code-Repo` trailer. +- **Deferred bundles in a store repository.** A bundle that defers never lands code, but its ledger writes (the reopen of its close, any harvested deferrals) are committed in the store repository as `chore(sweep): record a deferred bundle's ledger writes`, the same best-effort and withheld way, with only the `Bmad-Code-Repo` trailer. A landed bundle's harvested deferrals ride its close commit; one that landed with no close commits them as `chore(deferred-work): carry harvested findings from `, as does a resume that replays a deferred bundle's harvest. These are best effort too. ## Story-declared closure (`closes_deferred`) @@ -179,18 +179,19 @@ A stored answer is usable only if it is a JSON object whose `effect` is `build`, The sweep commits each ledger or store change it makes as its own file-scoped commit: only that file is staged, and the whole file is committed. -| Commit message | When | -| --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | -| `chore(sweep): record deferred-work decisions` | after the decision phase | -| `chore(sweep): close resolved deferred-work entries` | already-resolved entries and decided closes; a landed bundle's close when the ledger is in a store repository | -| `chore(sweep): record a deferred bundle's ledger writes` | a deferred bundle's reopened close and harvested deferrals when the ledger is in a store repository | -| `chore(sweep): commit ledger at the sweep cycle boundary` | between repeat cycles | -| `chore(sweep): commit ledger after recovering in-flight bundles` | on resume, after in-flight bundles are recovered | -| `chore(sweep): commit a ledger write an interrupted phase left unpublished` | on resume, for a write an interrupted phase did not commit | -| `chore(sweep): drop consumed deferred-work pre-answers` | end-of-cycle prune of the project store | -| `chore(sweep): drop stale deferred-work pre-answer` | a `stale-option` drop | -| `chore(sweep): migrate legacy deferred-work entries to DW format` | legacy-ledger migration | -| `chore(sweep): ingest retro action items` | retro action items added to the ledger at sweep start | +| Commit message | When | +| --------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `chore(sweep): record deferred-work decisions` | after the decision phase | +| `chore(sweep): close resolved deferred-work entries` | already-resolved entries and decided closes; a landed bundle's close when the ledger is in a store repository | +| `chore(sweep): record a deferred bundle's ledger writes` | a deferred bundle's reopened close and harvested deferrals when the ledger is in a store repository | +| `chore(deferred-work): carry harvested findings from ` | an isolated bundle's harvested deferrals when the ledger is in a store repository and the bundle landed with no close, or on resume of a deferred bundle's carry | +| `chore(sweep): commit ledger at the sweep cycle boundary` | between repeat cycles | +| `chore(sweep): commit ledger after recovering in-flight bundles` | on resume, after in-flight bundles are recovered | +| `chore(sweep): commit a ledger write an interrupted phase left unpublished` | on resume, for a write an interrupted phase did not commit | +| `chore(sweep): drop consumed deferred-work pre-answers` | end-of-cycle prune of the project store | +| `chore(sweep): drop stale deferred-work pre-answer` | a `stale-option` drop | +| `chore(sweep): migrate legacy deferred-work entries to DW format` | legacy-ledger migration | +| `chore(sweep): ingest retro action items` | retro action items added to the ledger at sweep start | `bmad-loop decisions` commits each file it wrote separately as `chore(decisions): pre-answer `: the ledger only if a `decision:` line landed, and the project store only for `build` or `keep-open`. A ledger the sweep marked as in doubt blocks every further ledger publish for that run (`sweep-ledger-commit-withheld`). diff --git a/docs/tickets-guide.md b/docs/tickets-guide.md index 94f1b3662..37368e784 100644 --- a/docs/tickets-guide.md +++ b/docs/tickets-guide.md @@ -47,7 +47,7 @@ In T1 a commit failure puts the pre-mark plan back; if a resumed commit window f For T2, T3, T4 and T6 the code commit cannot carry the plan, so the mark is latched on the task when the commit starts and made after `finalize_commit`, or after the merge-back under worktree isolation. A resume replays a latched mark. A failed mark pauses the run at `ticket-tree` and keeps the latch. An unversioned store journals `store-unversioned` once per run. -In T3 and T4 each marked plan (plus the ledger, when the unit wrote to it) gets its own commit, `chore(tickets): [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code commit. Under `mark_done = "never"` nothing is marked, and the plan is committed at `built`. A store commit never overwrites a version of one of its paths that a person has staged, even when the file is gone from disk. A failed store commit is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and listed by `status`, and the run goes on; it is retried before the run can finish, and one still owed pauses at `ticket-tree` (`store-commit-owed`) so the resume retries it. +In T3 and T4 each marked plan (plus the ledger, when the unit wrote to it) gets its own commit, `chore(tickets): [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code commit. Under `mark_done = "never"` nothing is marked, and the plan is committed at `built`. A store commit never overwrites a version of one of its paths that a person has staged, even when the file is gone from disk. A failed store commit is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and listed by `status`, and the run goes on; it is retried before the run can finish, and one still owed pauses at `ticket-tree` (`store-commit-owed`) so the resume retries it. A landed ticket's harvested deferrals ride its `chore(tickets)` commit. An isolated deferred ticket's harvest, or a resume's replay of it, gets its own best-effort commit, `chore(deferred-work): carry harvested findings from `, whose failure is journaled but not retried. Under worktree isolation a shared store is read in place: the unit worktree's `config.user.toml` is redirected to it. `tickets.own-repo-isolation` fails when that file is tracked or not ignored. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index d1416a8ca..a77bd623b 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -32,6 +32,7 @@ envvars, gates, operatoractions, + ticketstore, verify, ) from .adapters.base import CodingCLIAdapter, SessionResult, SessionSpec, SpecSnapshot @@ -10075,6 +10076,74 @@ def _carry_isolated_ledger_writes(self, task: StoryTask) -> None: self._carry_story_deferred_closes(task) self._carry_board_advance(task) + @property + def store_topology(self) -> ticketstore.StoreTopology | None: + """The ticket store this run recorded at its start. Base: None — a story + run has no ticket store. Modes with a ticket store (``SweepEngine`` on v7, + ``TicketsEngine``) override it to locate the recorded topology on the live + project.""" + return None + + def _harvest_carry_store_repo(self, ledger: Path) -> Path | None: + """The run's store repository when ``ledger`` resolves inside it (T3/T4), + else None: no recorded topology, an unversioned or in-checkout store + (``store_repo`` None), a ledger outside the store repository, or a + ``resolve()`` fault — which leaves the code-repo path, and its + ``commit_paths`` uncertainty raise, in charge (DW-570).""" + topology = self.store_topology + if topology is None or topology.store_repo is None: + return None + try: + inside = ledger.resolve().is_relative_to(topology.store_repo.resolve()) + except (OSError, RuntimeError, ValueError): + return None + return topology.store_repo if inside else None + + def _store_publishes_harvest_carry(self, task: StoryTask, *, terminal_composite: bool) -> bool: + """Whether a mode-owned store publisher commits the ledger right after + this harvested carry, so the carry's store arm must not (DW-570): one + publisher per write. Base: False — no publisher follows, so the carry + commits its own harvest in the store repository.""" + del task, terminal_composite + return False + + def _commit_store_ledger( + self, + task: StoryTask, + store_repo: Path, + ledger: Path, + message: str, + *, + code_commit: str | None, + code_baseline: str | None = None, + ) -> None: + """The best-effort store commit the sweep ledger publishers and the + harvested carry's store arm share: commit ``ledger`` alone in + ``store_repo`` under ``message`` and journal the outcome — + ``store-committed`` for a commit, ``store-commit-failed`` for a + :class:`~bmad_loop.ticketstore.StoreCommitError`, + ``store-commit-foreign-dirt`` for a staged foreign version, and nothing + when the ledger is already at the store's HEAD.""" + committer = ticketstore.StoreCommitter(store_repo, self.paths.repo_root) + try: + result = committer.commit( + task.story_key, + [ledger], + message, + code_commit=code_commit, + code_baseline=code_baseline, + ) + except ticketstore.StoreCommitError as e: + self.journal.append("store-commit-failed", story_key=task.story_key, error=str(e)) + return + if result.foreign: + self.journal.append( + "store-commit-foreign-dirt", story_key=task.story_key, count=len(result.foreign) + ) + return + if result.sha is not None: + self.journal.append("store-committed", story_key=task.story_key, commit=result.sha) + def _harvest_carry_commit_may_degrade(self, ledger: Path) -> bool: """Whether a carry may remain uncommitted because git cannot own its path. @@ -10279,6 +10348,23 @@ def _carry_harvested_deferrals( HEAD plus this task's rows (:meth:`_harvest_carry_holds_only_this_carry`) and otherwise pause for the operator without committing or clearing the latch (DW-355). A fresh first pass that appends rows is not proved. + + A ledger inside the run's store repository (T3/T4, + :meth:`_harvest_carry_store_repo`) takes the STORE ARM instead, after the + same DW-237 refusal (DW-570): the code-repo commit is never attempted, + since git there can only refuse the path. When a mode-owned store + publisher commits the ledger right after this carry + (:meth:`_store_publishes_harvest_carry`) the carry leaves the write to it; + otherwise it commits the ledger alone in the store through + :meth:`_commit_store_ledger` as ``chore(deferred-work): carry harvested + findings from ``, naming the landed code commit and baseline + on the terminal composite and no code commit elsewhere. Best effort, like + every store publisher: a failed commit journals ``store-commit-failed``, a + staged foreign version ``store-commit-foreign-dirt``; either way the latch + clears and ``harvest-carried`` is journaled. The DW-355/413 replay proof is + not run on the store arm: like every store publisher it publishes the whole + file, and :class:`~bmad_loop.ticketstore.StoreCommitter` refuses only a + staged foreign version. """ if not task.harvested_deferrals: return @@ -10360,7 +10446,9 @@ def _carry_harvested_deferrals( # nothing to retry — `target-undecodable` among them, because git # accepts any bytes and the fall-through would otherwise commit the # corrupt ledger (the guard splits that cause from this one for - # exactly this site; DW-237). + # exactly this site; DW-237). A ledger in a store repository takes + # the store arm below instead (DW-570): best effort, the latch + # clears, as its `may_degrade` degrade did before. refusal = None if refusal is not None: cause, error = refusal @@ -10374,6 +10462,29 @@ def _carry_harvested_deferrals( refuse_cause=cause, **extra, ) + elif (store_repo := self._harvest_carry_store_repo(ledger)) is not None: + # THE STORE ARM (DW-570): the ledger lives in a ticket store with + # its own repository (T3/T4), which `commit_paths` in the code repo + # can never own. Exactly one publisher per write: a mode-owned + # store publisher that commits right after this carry takes it + # (the sweep's `_publish_deferred_ledger` and + # `_publish_bundle_close`, the tickets `_commit_marked_plan`); + # otherwise the carry commits its harvest in the store itself. + # Best effort: never a raise, never a pause, and the latch clears + # below. + if not self._store_publishes_harvest_carry( + task, terminal_composite=terminal_composite + ): + self._commit_store_ledger( + task, + store_repo, + ledger, + f"chore(deferred-work): carry harvested findings from {task.story_key}", + code_commit=( + self._landed_code_commit(task) if terminal_composite else None + ), + code_baseline=task.baseline_commit if terminal_composite else None, + ) else: if latched_before or not carried: # Stages whatever the working-tree ledger holds now, so it is diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 065d60539..9c8547aee 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -8793,8 +8793,11 @@ def _publish_bundle_close(self, task: StoryTask) -> None: """Commit the ledger close a landed bundle wrote when the ledger lives in a store repository (T3/T4), through :class:`~bmad_loop.ticketstore.StoreCommitter` as ``chore(sweep): close - resolved deferred-work entries`` with the ``Bmad-Code-*`` trailers. - Elsewhere the close rides the bundle's own code commit (``commit_story``'s + resolved deferred-work entries`` with the ``Bmad-Code-*`` trailers. A + harvest the isolated terminal carry filed into the same ledger rides this + commit: the carry leaves it to this publisher rather than committing it + itself (:meth:`_store_publishes_harvest_carry`, DW-570). Elsewhere the + close rides the bundle's own code commit (``commit_story``'s ``git add -A``, or the merge-back and ``_carry_isolated_ledger_writes`` under isolation); a store repository's ledger is in no tree that commit can reach. @@ -8862,9 +8865,12 @@ def _publish_deferred_ledger(self, task: StoryTask) -> None: a store repository (T3/T4), through :class:`~bmad_loop.ticketstore.StoreCommitter` as ``chore(sweep): record a deferred bundle's ledger writes``: the reopen of its close and any - harvested deferrals. No code landed, so only ``Bmad-Code-Repo`` is - stamped. Elsewhere the ledger is in the code checkout (or a worktree copy - that dies with the unit) and no store commit is owed. + harvested deferrals, which the isolated defer's harvested carry leaves to + this publisher rather than committing itself + (:meth:`_store_publishes_harvest_carry`, DW-570). No code landed, so only + ``Bmad-Code-Repo`` is stamped. Elsewhere the ledger is in the code + checkout (or a worktree copy that dies with the unit) and no store commit + is owed. Withheld while the run holds its ledger in doubt (``sweep-ledger-commit-withheld`` naming the bundle's ids). Otherwise best @@ -8889,41 +8895,30 @@ def _publish_deferred_ledger(self, task: StoryTask) -> None: return self._commit_store_ledger(task, topology.store_repo, ledger, message, code_commit=None) - def _commit_store_ledger( - self, - task: StoryTask, - store_repo: Path, - ledger: Path, - message: str, - *, - code_commit: str | None, - code_baseline: str | None = None, - ) -> None: - """The best-effort store commit both ledger publishers share: commit - ``ledger`` alone in ``store_repo`` under ``message`` and journal the - outcome — ``store-committed`` for a commit, ``store-commit-failed`` for a - :class:`~bmad_loop.ticketstore.StoreCommitError`, - ``store-commit-foreign-dirt`` for a staged foreign version, and nothing - when the ledger is already at the store's HEAD.""" - committer = ticketstore.StoreCommitter(store_repo, self.paths.repo_root) - try: - result = committer.commit( - task.story_key, - [ledger], - message, - code_commit=code_commit, - code_baseline=code_baseline, - ) - except ticketstore.StoreCommitError as e: - self.journal.append("store-commit-failed", story_key=task.story_key, error=str(e)) - return - if result.foreign: - self.journal.append( - "store-commit-foreign-dirt", story_key=task.story_key, count=len(result.foreign) - ) - return - if result.sha is not None: - self.journal.append("store-committed", story_key=task.story_key, commit=result.sha) + def _store_publishes_harvest_carry(self, task: StoryTask, *, terminal_composite: bool) -> bool: + """Whether a sweep store publisher commits the ledger right after this + harvested carry (DW-570), so the carry's store arm leaves it alone. True + in two cases: + + - The direct carry the base :meth:`_defer` makes on the isolated arm: it + runs before the base advances the task to DEFERRED, and this class's + :meth:`_defer` follows it with :meth:`_publish_deferred_ledger`, the + documented publisher of the reopened close and the harvest together. + Committing there would sweep the reopen into a carry commit and leave + the deferred publisher nothing to do. + - The terminal composite of a bundle that landed with a close + (``bundle_closes_intended`` non-empty): ``worktree_flow`` runs the + carry, then :meth:`_land_post_landing_writes` → + :meth:`_publish_bundle_close`. Under a shared store the close is + already in the one ledger, so a carry commit would fold it into + ``chore(deferred-work): …`` and leave the close publisher nothing. + + A harvest-only terminal composite and the resume replay + (``_replay_unlatched_ledger_carries``, phase already DEFERRED) have no + store publisher after them, so the carry commits those itself.""" + if terminal_composite: + return bool(task.bundle_closes_intended) + return task.phase != Phase.DEFERRED def _publish_bundle_plan(self, task: StoryTask) -> None: """Commit ``task``'s accepted bundle plan in the store's repository through diff --git a/src/bmad_loop/tickets_engine.py b/src/bmad_loop/tickets_engine.py index 5a3487bfe..555828b07 100644 --- a/src/bmad_loop/tickets_engine.py +++ b/src/bmad_loop/tickets_engine.py @@ -1897,6 +1897,17 @@ def _post_landing_target(task: StoryTask) -> str: return verify.PLAN_DONE return verify.PLAN_BUILT if task.ticket_commit_pending else "" + def _store_publishes_harvest_carry(self, task: StoryTask, *, terminal_composite: bool) -> bool: + """True for the terminal composite of a ticket that owes a post-landing + write (:meth:`_post_landing_target` non-empty, DW-570): ``worktree_flow`` + runs the harvested carry, then :meth:`_land_post_landing_writes` → + :meth:`_commit_marked_plan`, the latched and retried publisher of the plan + plus the ledger when the task wrote to it. A carry commit first would take + the ledger out of that commit and trade its retry for a best-effort one. + An isolated defer and the resume replay owe no post-landing write, so the + carry commits those itself.""" + return terminal_composite and bool(self._post_landing_target(task)) + def _land_post_landing_writes(self, task: StoryTask) -> None: """Make the latched mark now that the unit's code is on the target: on the run's own tree (the main checkout's view of the shared store) and the plan diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index 9f97cd185..543c1571f 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -13736,6 +13736,119 @@ def test_t3_mark_done_commits_in_store_repo(project_v7_store_repo, monkeypatch): assert [(m.target, m.outcome) for m in tree.marks] == [("done", "written")] +def test_t3_harvest_carry_rides_the_marked_plan_commit(project_v7_store_repo, monkeypatch): + """DW-570, T3: an isolated ticket whose task carries one harvested deferral + the shared ledger does not yet hold reaches the terminal composite, whose + carry files it into the store's ledger, then `_land_post_landing_writes`. + `_commit_marked_plan` is the latched publisher of the plan plus the ledger, + so the carry leaves the ledger to it: no carry commit, the `chore(tickets): + 1.1 done …` commit touches the plan and the ledger with the harvested row at + HEAD, and no `harvest-carry-uncommitted` row. The harvest is injected at the + hook (the real `worktree_flow` order runs around it): a full run's own + harvest lands in the shared ledger at verify time, so the carry would dedupe + it and never reach a commit. + + Ablation (performed): drop `TicketsEngine._store_publishes_harvest_carry` + and this FAILS (the carry commits the ledger alone first, and the tickets + commit holds only the plan).""" + from test_tickets_engine import _kinds + + from bmad_loop.tickets_engine import TicketsEngine + + layout = project_v7_store_repo + store = layout.output_folder + real_carry = TicketsEngine._carry_isolated_ledger_writes + seen: list[str] = [] + + def carry_with_harvest(self, task): + assert self._post_landing_target(task) == "done" # premise: a mark is latched + seen.append(task.story_key) + task.harvested_deferrals = [ + { + "origin": "spec-deferred plan-1.1.md", + "title": "cart totals ignore discounts", + "reason": "found while building 1.1", + "location": "src.txt", + "severity": "medium", + "source_spec": "plan-1.1.md", + } + ] + real_carry(self, task) + + monkeypatch.setattr(TicketsEngine, "_carry_isolated_ledger_writes", carry_with_harvest) + engine, _, plan = _store_run(layout, "T3", monkeypatch) + + assert seen == ["1.1"] # premise: the terminal composite ran + task = load_state(engine.run_dir).tasks["1.1"] + assert task.phase == Phase.DONE and task.worktree_path and not task.ticket_done_pending + assert [e["dw_ids"] for e in _kinds(engine, "harvest-carried")] == [["DW-1"]] + subjects = git(store, "log", "--format=%s").splitlines() + assert not [s for s in subjects if s.startswith("chore(deferred-work): carry")], subjects + assert subjects[0] == "chore(tickets): 1.1 done [bmad-loop test-run]" + ledger = engine.paths.deferred_work + rels = {p.resolve().relative_to(store.resolve()).as_posix() for p in (plan, ledger)} + assert set(git(store, "show", "--name-only", "--format=", "HEAD").split()) == rels + ledger_rel = ledger.resolve().relative_to(store.resolve()).as_posix() + assert deferredwork.open_ids(git(store, "show", f"HEAD:{ledger_rel}")) == {"DW-1"} + assert git(store, "status", "--porcelain") == "" + assert _kinds(engine, "harvest-carry-uncommitted") == [] + + +def test_t3_tickets_replayed_deferred_harvest_commits_in_store_repo(project_v7_store_repo): + """DW-570, T3, driven at the seam: the resume replay of an isolated DEFERRED + ticket's latched harvest (`_replay_unlatched_ledger_carries`). A deferred + ticket owes no post-landing write, so no store publisher follows and the + carry commits the harvest in the store itself: exactly one `chore( + deferred-work): carry harvested findings from 1.1` commit touching only the + ledger, the store's ledger clean, the latch cleared and no + `harvest-carry-uncommitted` row. + + Ablation (performed): make `TicketsEngine._store_publishes_harvest_carry` + `return True` and this FAILS (no carry commit; the harvest stays dirty in the + store).""" + from test_tickets_engine import _kinds + + layout = project_v7_store_repo + store = layout.output_folder + engine, _ = _store_engine(layout, _store_tree(layout, "1.1"), "T3") + ledger = engine.paths.deferred_work + assert ledger.resolve().is_relative_to(store.resolve()) # premise: a store ledger + task = StoryTask( + "1.1", + 0, + phase=Phase.DEFERRED, + worktree_path=str(engine.paths.repo_root.parent / "unit"), + harvest_carry_commit_pending=True, + harvested_deferrals=[ + { + "origin": "spec-deferred plan-1.1.md", + "title": "cart totals ignore discounts", + "reason": "found while building 1.1", + "location": "src.txt", + "severity": "medium", + "source_spec": "plan-1.1.md", + } + ], + ) + engine.state.tasks["1.1"] = task + engine._save() + + engine._replay_unlatched_ledger_carries() + + assert [e["story_key"] for e in _kinds(engine, "resume-ledger-carry")] == ["1.1"] + assert [e["dw_ids"] for e in _kinds(engine, "harvest-carried")] == [["DW-1"]] + subject = "chore(deferred-work): carry harvested findings from 1.1" + log = [line.split(" ", 1) for line in git(store, "log", "--format=%H %s").splitlines()] + commits = [sha for sha, s in log if s == subject] + assert len(commits) == 1, git(store, "log", "--format=%s") + ledger_rel = ledger.resolve().relative_to(store.resolve()).as_posix() + assert git(store, "show", "--name-only", "--format=", commits[0]).split() == [ledger_rel] + assert deferredwork.open_ids(git(store, "show", f"HEAD:{ledger_rel}")) == {"DW-1"} + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + assert task.harvest_carry_commit_pending is False + assert _kinds(engine, "harvest-carry-uncommitted") == [] + + def test_squash_merge_back_trailers_name_the_landed_commit(project_v7_store_repo, monkeypatch): """A `squash` merge-back lands a different commit than the unit's, and the unit branch's teardown leaves the unit's unreachable: the store trailers name diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 01d0f408e..9c3cef1e3 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -38669,11 +38669,18 @@ def test_v7_isolated_deferred_bundle_publishes_its_shared_store_ledger(project_v harvested deferral, which the base's isolated arm files into that ledger as DW-2. `SweepEngine._defer` reopens DW-1 and publishes: exactly one deferred-bundle store commit touching only the ledger, holding DW-1 and DW-2 - open, the store's ledger clean, and its `store-committed` row naming it. + open, the store's ledger clean, and its `store-committed` row naming it. The + harvested carry takes its store arm (DW-570): it never asks the code repo to + commit a store path (no `harvest-carry-uncommitted`) and leaves the harvest to + the deferred publisher (no carry commit of its own). - ABLATION (performed): gate the `_publish_deferred_ledger` call in + ABLATIONS (performed): gate the `_publish_deferred_ledger` call in `SweepEngine._defer` to the in-place arm only and this fails (no - deferred-bundle commit; the ledger stays dirty in the store).""" + deferred-bundle commit; the ledger stays dirty in the store). Remove the + store arm from `Engine._carry_harvested_deferrals` and this fails on + `harvest-carry-uncommitted`. Make `SweepEngine._store_publishes_harvest_carry` + return False and this fails on the deferred-commit count (the carry commits + the reopen and the harvest itself, leaving the deferred publisher nothing).""" v7 = project_v7_store_repo store = v7.output_folder paths = _v7_paths(v7) @@ -38715,10 +38722,13 @@ def test_v7_isolated_deferred_bundle_publishes_its_shared_store_ledger(project_v engine._defer(task, "verify failed") assert task.phase == Phase.DEFERRED + assert [r["dw_ids"] for r in _records(engine, "harvest-carried")] == [["DW-2"]] # premise + assert _records(engine, "harvest-carry-uncommitted") == [] entries = ledger_entries(paths) assert entries["DW-1"].open and entries["DW-2"].open commits = _store_deferred_commits(store) assert len(commits) == 1, git(store, "log", "--format=%s") + assert _store_carry_commits(store, task.story_key) == [] assert git(store, "show", "--name-only", "--format=", commits[0]).split() == [ledger_rel] head_ledger = git(store, "show", f"{commits[0]}:{ledger_rel}") assert deferredwork.open_ids(head_ledger) == {"DW-1", "DW-2"} @@ -38726,6 +38736,204 @@ def test_v7_isolated_deferred_bundle_publishes_its_shared_store_ledger(project_v assert [r["commit"] for r in _records(engine, "store-committed")] == commits +def _store_carry_commits(store: Path, story_key: str) -> list[str]: + """The shas of the store's harvested-carry commits for ``story_key``, newest + first (DW-570).""" + subject = f"chore(deferred-work): carry harvested findings from {story_key}" + log = [line.split(" ", 1) for line in git(store, "log", "--format=%H %s").splitlines()] + return [sha for sha, s in log if s == subject] + + +def _v7_store_carry_seam(v7, layout, *, distinct_baseline=False): + """A v7 sweep engine over a store with its own repository (``layout`` T3/T4) + whose ledger, `{DW-1: open}`, is committed in that store, plus a mounted + bundle task that landed (``commit_sha`` at the code repo's HEAD) carrying one + harvested deferral. ``distinct_baseline`` lands an empty code commit first so + the task's ``baseline_commit`` is its parent rather than ``commit_sha``. + Returns the engine, its paths, the store, the ledger's store-relative path + and the task.""" + store = v7.output_folder + paths = _v7_paths(v7) + write_ledger(paths, {"DW-1": "open"}, commit=False) + git(store, "add", "-A") + git(store, "commit", "-q", "-m", "ledger") + ledger_rel = paths.deferred_work.resolve().relative_to(store.resolve()).as_posix() + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" # premise + topology = ticketstore.assess_store(paths, "none") + assert isinstance(topology, ticketstore.StoreTopology) and topology.layout == layout + engine, _ = make_sweep(paths, [], store_topology=topology.to_dict()) + baseline = rev_parse_head(paths.repo_root) + if distinct_baseline: + git(paths.repo_root, "commit", "-q", "--allow-empty", "-m", "the bundle's code") + head = rev_parse_head(paths.repo_root) + assert (head != baseline) == distinct_baseline # premise + task = StoryTask( + story_key="dw-fix-things", + epic=0, + dw_ids=["DW-1"], + phase=Phase.DONE, + worktree_path=str(paths.repo_root.parent / "unit"), + commit_sha=head, + baseline_commit=baseline, + harvested_deferrals=[ + { + "origin": "spec-deferred plan-dw-fix-things.md", + "title": SWEEP_FINDING["summary"], + "reason": SWEEP_FINDING["evidence"], + "location": SWEEP_FINDING["location"], + "severity": SWEEP_FINDING["severity"], + "source_spec": "plan-dw-fix-things.md", + } + ], + ) + engine.state.tasks[task.story_key] = task + return engine, paths, store, ledger_rel, task + + +@pytest.mark.parametrize( + ("fixture", "layout"), + [("project_v7_store_repo", "T3"), ("project_v7_external_store", "T4")], +) +def test_v7_terminal_harvest_carry_commits_in_store_repo(request, fixture, layout): + """DW-570, driven at the seam: the terminal-composite harvested carry of a + mounted bundle whose ledger lives in a store with its own repository (T3 + nested, T4 outside). No store publisher follows a harvest-only write there, + so the carry commits it in the store itself: exactly one carry commit + touching only the ledger, HEAD holding the harvested DW-2 open, the store's + ledger clean, one `store-committed` row naming it, the landed code commit in + its trailers, no `harvest-carry-uncommitted` row (the code repo is never + asked to commit a store path) and the commit latch cleared. + + ABLATIONS (performed): drop `code_baseline=` from the store arm's commit + and both rows fail on `Bmad-Code-Range` (the seam's baseline differs from the + commit). Remove the store arm from + `Engine._carry_harvested_deferrals` and both rows fail on the carry-commit + count (the code-repo commit is refused and degrades to + `harvest-carry-uncommitted`, so no carry commit lands in the store).""" + engine, paths, store, ledger_rel, task = _v7_store_carry_seam( + request.getfixturevalue(fixture), layout, distinct_baseline=True + ) + + engine._carry_harvested_deferrals(task, terminal_composite=True) + + assert [r["dw_ids"] for r in _records(engine, "harvest-carried")] == [["DW-2"]] + commits = _store_carry_commits(store, task.story_key) + assert len(commits) == 1, git(store, "log", "--format=%s") + assert git(store, "show", "--name-only", "--format=", commits[0]).split() == [ledger_rel] + head_ledger = git(store, "show", f"HEAD:{ledger_rel}") + assert deferredwork.open_ids(head_ledger) == {"DW-1", "DW-2"} + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + assert [r["commit"] for r in _records(engine, "store-committed")] == commits + body = git(store, "log", "-1", "--format=%B", commits[0]) + assert f"Bmad-Code-Commit: {task.commit_sha}" in body + assert task.baseline_commit != task.commit_sha # premise: the range is graded + assert f"Bmad-Code-Range: {task.baseline_commit}..{task.commit_sha}" in body + assert _records(engine, "harvest-carry-uncommitted") == [] + assert task.harvest_carry_commit_pending is False + + +def test_v7_harvest_carry_store_commit_failure_is_best_effort(project_v7_store_repo, monkeypatch): + """DW-570, T3: the harvested carry's store commit raises + `StoreCommitError`. The store arm is best effort: no raise, a + `store-commit-failed` row for the bundle, the commit latch cleared and + `harvest-carried` journaled as on success. + + ABLATION (performed): drop the `except ticketstore.StoreCommitError` clause + in `Engine._commit_store_ledger` and this fails (the error propagates out of + the carry).""" + engine, _paths, store, ledger_rel, task = _v7_store_carry_seam(project_v7_store_repo, "T3") + + def commit(self, unit_id, paths, message, **kwargs): + raise ticketstore.StoreCommitError(f"{unit_id}: index.lock exists") + + monkeypatch.setattr(ticketstore.StoreCommitter, "commit", commit) + + engine._carry_harvested_deferrals(task, terminal_composite=True) + + assert [r["story_key"] for r in _records(engine, "store-commit-failed")] == ["dw-fix-things"] + assert task.harvest_carry_commit_pending is False + assert [r["dw_ids"] for r in _records(engine, "harvest-carried")] == [["DW-2"]] + assert _records(engine, "harvest-carry-uncommitted") == [] + assert _store_carry_commits(store, task.story_key) == [] + assert git(store, "status", "--porcelain", "--", ledger_rel) != "" # still uncommitted + + +def test_v7_terminal_harvest_carry_rides_the_bundle_close_commit(project_v7_store_repo): + """DW-570, T3, driven at the seam: a mounted bundle that landed with a close + AND a harvest. Under the shared store its close of DW-1 is already in the one + ledger, and `_publish_bundle_close` follows the terminal composite, so the + carry leaves the ledger to it: no carry commit, and exactly one close commit + touching only the ledger whose HEAD holds DW-1 done and the harvested DW-2 + open, the store's ledger clean and no `harvest-carry-uncommitted` row. + + ABLATION (performed): drop the terminal-composite case from + `SweepEngine._store_publishes_harvest_carry` (return False there) and this + fails on the carry-commit count (the carry folds the close into + `chore(deferred-work): …` and leaves the close publisher nothing to commit).""" + engine, paths, store, ledger_rel, task = _v7_store_carry_seam(project_v7_store_repo, "T3") + assert engine.paths.in_shared_store(engine.paths.deferred_work) # premise + task.bundle_closes_intended = ["DW-1"] + deferredwork.mark_done_many_reopenable( + engine.paths.deferred_work, + task.dw_ids, + engine._today(), + engine._bundle_close_note(task), + engine._bundle_close_operation_id(task), + ) + assert not ledger_entries(paths)["DW-1"].open # premise: the close is in the main ledger + + engine._carry_isolated_ledger_writes(task) + engine._publish_bundle_close(task) + + assert [r["dw_ids"] for r in _records(engine, "harvest-carried")] == [["DW-2"]] # premise + assert _store_carry_commits(store, task.story_key) == [], git(store, "log", "--format=%s") + closes = _store_closes(store, ledger_rel) # each asserted to touch the ledger alone + assert len(closes) == 1, git(store, "log", "--format=%s") + head_ledger = git(store, "show", f"HEAD:{ledger_rel}") + assert git(store, "log", "-1", "--format=%s").strip() == _V7_LEDGER_CLOSE + assert deferredwork.open_ids(head_ledger) == {"DW-2"} + head_entries = {e.id: e for e in deferredwork.parse_ledger(head_ledger)} + assert set(head_entries) == {"DW-1", "DW-2"} and not head_entries["DW-1"].open + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + assert _records(engine, "harvest-carry-uncommitted") == [] + assert task.harvest_carry_commit_pending is False + + +def test_v7_resume_replay_commits_deferred_harvest_in_store_repo(project_v7_store_repo): + """DW-570, T3: the resume replay of a DEFERRED isolated bundle's latched + harvest (`_replay_unlatched_ledger_carries`). No store publisher follows the + replay, so the carry commits the harvest in the store itself: one carry + commit with DW-2 open at HEAD, the store's ledger clean, the latch cleared + and no `harvest-carry-uncommitted` row. + + ABLATION (performed): drop the `task.phase != Phase.DEFERRED` clause from + `SweepEngine._store_publishes_harvest_carry` (return True for every + non-terminal carry) and this fails on the carry-commit count (the replay + leaves the harvest to a publisher that never runs). Pass `code_commit=` the + landed commit on every carry and this fails on `Bmad-Code-Commit` (the task + keeps the seam's `commit_sha`, so only the terminal gate keeps it out).""" + engine, paths, store, ledger_rel, task = _v7_store_carry_seam(project_v7_store_repo, "T3") + task.phase = Phase.DEFERRED + task.harvest_carry_commit_pending = True + engine._save() + + engine._replay_unlatched_ledger_carries() + + assert [r["story_key"] for r in _records(engine, "resume-ledger-carry")] == [task.story_key] + assert [r["dw_ids"] for r in _records(engine, "harvest-carried")] == [["DW-2"]] + commits = _store_carry_commits(store, task.story_key) + assert len(commits) == 1, git(store, "log", "--format=%s") + assert git(store, "show", "--name-only", "--format=", commits[0]).split() == [ledger_rel] + head_ledger = git(store, "show", f"HEAD:{ledger_rel}") + assert deferredwork.open_ids(head_ledger) == {"DW-1", "DW-2"} + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + body = git(store, "log", "-1", "--format=%B", commits[0]) + assert "Bmad-Code-Repo: " in body # premise: the trailers are there + assert "Bmad-Code-Commit" not in body # no code landed on a deferred carry + assert task.harvest_carry_commit_pending is False + assert _records(engine, "harvest-carry-uncommitted") == [] + + @pytest.mark.parametrize("publisher", ["plan", "close"]) def test_v7_bundle_store_trailers_name_the_landed_squash_commit(project_v7_store_repo, publisher): """Under worktree isolation a `squash` merge-back lands a different commit From 37b1842e71c65f317b103d67671459cf7c494401 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 18 Oct 2026 13:24:39 -0700 Subject: [PATCH 04/10] sweep dw3-decision-dw-569: DW-569 via bmad-loop --- CHANGELOG.md | 4 + docs/sweep-guide.md | 2 +- src/bmad_loop/engine.py | 10 + src/bmad_loop/sweep.py | 74 ++++++- src/bmad_loop/worktree_flow.py | 19 +- tests/test_portability_guard.py | 1 + tests/test_sweep.py | 373 +++++++++++++++++++++++++++++++- 7 files changed, 476 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 458defa64..1364353be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -164,6 +164,10 @@ breaking changes may land in a minor release. - Commit an isolated unit's harvested deferrals in the ticket store's own repository (T3/T4) when no store publisher follows, best effort, instead of journaling `harvest-carry-uncommitted` for a store path the code repository cannot commit (DW-570). +- Reopen a worktree-isolated sweep bundle's close in a shared ticket store's ledger when the + bundle escalates (merge-back failures included) or a plugin vetoes it, instead of leaving + the entries `done` for code that never landed; a reopen fault journals + `sweep-bundle-reopen-failed` and the escalation still pauses (DW-569). - Latch a worktree-isolated sweep bundle's artifact publication complete when its artifacts dir is in a shared ticket store, instead of pausing on `artifact publication intent is missing` after the merge (DW-557). diff --git a/docs/sweep-guide.md b/docs/sweep-guide.md index 0dff789e2..b77fb6470 100644 --- a/docs/sweep-guide.md +++ b/docs/sweep-guide.md @@ -90,7 +90,7 @@ A sweep runs on a v7 project the same way, with these differences: - **Store layout.** The store-topology check judges a v7 sweep as a tickets run, because its ledger lives in the ticket store. The sweep records the store's topology at its start, and a resume refuses a store that changed since, or whose initiative folder is gone. - **Pinned initiative.** A sweep records the active initiative it started on. A resume, `resolve` or the dashboard's re-arm refuses while `[core] active_initiative` names another: set it back, or start a new sweep. - **Decisions per initiative.** Each initiative has its own ledger and DW numbering, so pre-answers are kept per initiative in `.bmad-loop/decisions-.json`. `bmad-loop decisions` and the TUI answer the active initiative's decisions and offer only those its own sweeps raised. An answer given after another initiative was made active than the one its decision was listed from is refused; list the decisions again. With no initiative active, the project store stays `.bmad-loop/decisions.json` and only sweeps that ran with no initiative are offered. -- **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. The bundle's ledger close lands in that shared ledger the same way, so a bundle that defers reopens its close before the defer is recorded. +- **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. The bundle's ledger close lands in that shared ledger the same way, so a bundle that ends without landing its code reopens its close before that end is recorded: when it defers, when it escalates (a failed merge-back included), and when a plugin vetoes it. A ledger fault during that reopen is journaled as `sweep-bundle-reopen-failed` and the escalation or veto goes on, leaving the close for a human to reopen. Resume skips an escalated bundle, so after you merge a failed merge-back's kept branch by hand, its entries stay open until a later sweep triages them as already resolved. - **Plans in a store repository.** When the ticket store is its own git repository (nested or outside the checkout), the bundle's code commit cannot carry its plan. Once the code has landed, the accepted plan gets its own commit there, `chore(sweep): plan [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code. This is best effort, like a tickets run's store commit: a failure is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and the plan stays uncommitted for a human. The bundle's ledger close is committed there the same best-effort way, as `chore(sweep): close resolved deferred-work entries` with the same trailers and the same journaled failures, and is withheld (`sweep-ledger-commit-withheld`) while the run holds its ledger in doubt. - **Deferred bundles in a store repository.** A bundle that defers never lands code, but its ledger writes (the reopen of its close, any harvested deferrals) are committed in the store repository as `chore(sweep): record a deferred bundle's ledger writes`, the same best-effort and withheld way, with only the `Bmad-Code-Repo` trailer. A landed bundle's harvested deferrals ride its close commit; one that landed with no close commits them as `chore(deferred-work): carry harvested findings from `, as does a resume that replays a deferred bundle's harvest. These are best effort too. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index a77bd623b..77e90bc6e 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -1033,6 +1033,7 @@ def __init__( gate_unit=self._gate_unit, carry_isolated_ledger_writes=lambda task: self._carry_isolated_ledger_writes(task), land_post_landing_writes=lambda task: self._land_post_landing_writes(task), + reopen_unlanded_close=lambda task: self._reopen_unlanded_close(task), escalation_pause=self._escalation_pause, workspace_get=lambda: self.workspace, workspace_set=lambda ws: setattr(self, "workspace", ws), @@ -4922,6 +4923,15 @@ def _land_post_landing_writes(self, task: StoryTask) -> None: write that fails raises ``RunPaused`` with the latch kept. Base: none.""" del task + def _reopen_unlanded_close(self, task: StoryTask) -> None: + """Undo the work source's writes that assumed ``task``'s code would land, + now that it ends without landing: before the terminal save of every + escalation (:meth:`_escalate`, and ``WorktreeFlow``'s ``escalate_unit`` / + ``keep_branch_and_escalate``) and every plugin veto (:meth:`_vetoed`). + A sweep reopens an isolated bundle's close in a shared-store ledger + (DW-569). Base: none.""" + del task + def _landed_code_commit(self, task: StoryTask) -> str | None: """The code commit a store commit's trailers name for ``task`` (a tickets run's marked plan, a v7 sweep's bundle plan): the unit's own commit while diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 9c8547aee..055db1ecb 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -56,6 +56,9 @@ from .statemachine import advance from .worksource import EpicProgress, Pick, WorkSource +if TYPE_CHECKING: + from .plugins import HookContext + def _read_json(path: Path) -> Any: return json.loads(path.read_text(encoding="utf-8")) @@ -3535,7 +3538,8 @@ def _escalate( ) -> None: """Escalate the migrate task along legal edges only (DW-405/407/314). - Every other task takes ``Engine._escalate`` unchanged. The migrate task + Every other task takes ``Engine._escalate`` unchanged, after + :meth:`_reopen_unlanded_close` (DW-569). The migrate task can refuse from two boundaries the phase graph gives no ESCALATED edge: a marked ``TRIAGE_RUNNING`` recovery walks through ``TRIAGE_VERIFY`` first, and a refusal while already ESCALATED (the escalated commit-tail @@ -3552,6 +3556,9 @@ def _escalate( advanced-HEAD refusal refused). """ if task.story_key != MIGRATE_KEY: + # No code landed: reopen an isolated bundle's shared-store close + # before the base's ESCALATED save (DW-569). + self._reopen_unlanded_close(task) super()._escalate(task, reason) return if task.phase == Phase.DONE: @@ -8867,8 +8874,10 @@ def _publish_deferred_ledger(self, task: StoryTask) -> None: deferred bundle's ledger writes``: the reopen of its close and any harvested deferrals, which the isolated defer's harvested carry leaves to this publisher rather than committing itself - (:meth:`_store_publishes_harvest_carry`, DW-570). No code landed, so only - ``Bmad-Code-Repo`` is stamped. Elsewhere the ledger is in the code + (:meth:`_store_publishes_harvest_carry`, DW-570). It also publishes the + reopen :meth:`_reopen_unlanded_close` makes when an isolated bundle + escalates or is vetoed (DW-569), under the same message. No code landed, + so only ``Bmad-Code-Repo`` is stamped. Elsewhere the ledger is in the code checkout (or a worktree copy that dies with the unit) and no store commit is owed. @@ -8895,6 +8904,62 @@ def _publish_deferred_ledger(self, task: StoryTask) -> None: return self._commit_store_ledger(task, topology.store_repo, ledger, message, code_commit=None) + def _vetoed(self, ctx: HookContext | None, task: StoryTask) -> bool: + """The base veto routing, after :meth:`_reopen_unlanded_close` when a + plugin really vetoed (DW-569). Every veto action (``pause``, ``defer``, + ``skip``) ends the unit without landing its code, so the reopen runs + ahead of the base's terminal save. A stage nobody vetoed is not an end + and reaches the base untouched.""" + if ctx is not None and ctx.resolved_veto() is not None: + self._reopen_unlanded_close(task) + return super()._vetoed(ctx, task) + + def _reopen_unlanded_close(self, task: StoryTask) -> None: + """Reopen an isolated bundle's shared-store close when the bundle ends + without landing (DW-569): before the terminal save of every escalation + (:meth:`_escalate`, ``WorktreeFlow.escalate_unit`` for an in-flight unit, + ``WorktreeFlow.keep_branch_and_escalate`` for a failed merge-back) and + every plugin veto (:meth:`_vetoed`). The defer has its own arm + (:meth:`_defer`). + + Acts only on the isolated arm (``self._isolated or task.worktree_path``) + with the ledger in the shared ticket store and a close on record + (``task.bundle_closes_intended``). There the accepted-dev close went + straight into the one ledger every checkout shares, and ``open_ids`` + re-bundles only ``open`` entries, so a ``done`` left for code that never + landed is invisible to every later sweep. Otherwise a no-op that reads + nothing: in place the rollback owns the ledger, and an unshared ledger + copy dies with the worktree. + + The reopen is :meth:`_reopen_ledger_after_defer` (the same operation + id, so only this bundle's close is undone), then + :meth:`_publish_deferred_ledger` commits it in a store repository. + ``bundle_closes_intended`` is kept, so a re-armed bundle that lands + re-closes through :meth:`_carry_isolated_ledger_writes`. + + A ``LedgerReadError`` or ``OSError`` from the reopen is a visible + degrade: it journals ``sweep-bundle-reopen-failed`` and skips the + publish, and the escalation or veto goes on. The escalation pause is the + operator stop; turning it into a crash would lose its reason.""" + if not task.bundle_closes_intended: + return + if not (self._isolated or task.worktree_path): + return + if not self.paths.in_shared_store(self.paths.deferred_work): + return + try: + self._reopen_ledger_after_defer(task) + except (deferredwork.LedgerReadError, OSError) as e: + # `LedgerReadError` is a plain `Exception` (DW-146), so it is named. + self.journal.append( + "sweep-bundle-reopen-failed", + story_key=task.story_key, + dw_ids=list(task.bundle_closes_intended), + error=str(e), + ) + return + self._publish_deferred_ledger(task) + def _store_publishes_harvest_carry(self, task: StoryTask, *, terminal_composite: bool) -> bool: """Whether a sweep store publisher commits the ledger right after this harvested carry (DW-570), so the carry's store arm leaves it alone. True @@ -9214,7 +9279,8 @@ def _reopen_ledger_after_defer(self, task: StoryTask) -> None: bundle close whose code no longer exists. :meth:`_defer` also calls this on the isolated arm when the ledger is in the shared ticket store, where the mount wrote its close straight into the one ledger every checkout shares - (DW-567). The operation-specific undo marker + (DW-567), and :meth:`_reopen_unlanded_close` calls it on the same arm when + the bundle escalates or is vetoed (DW-569). The operation-specific undo marker makes this "undo my close", not "open these ids": human, legacy, and earlier run closures remain untouched. Replaying this method is idempotent. """ diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index f1d7c013c..98951fa8f 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -1784,7 +1784,9 @@ class WorktreeFlow: ``Engine._emit`` still wins), ``save`` persists run state, ``gate_unit`` runs the per-unit ready gate, ``carry_isolated_ledger_writes`` applies Engine-owned ledger bookkeeping after a successful merge, ``land_post_landing_writes`` makes - the writes the engine latched for after landing, ``workspace_get``/``workspace_set`` + the writes the engine latched for after landing, ``reopen_unlanded_close`` undoes + the writes that assumed a landing before an escalation's terminal save, + ``workspace_get``/``workspace_set`` read and swap the engine's active workspace, and ``escalation_pause`` raises the engine's ``RunPaused`` (injected so this module need not import ``engine`` — that would reintroduce a runtime<->engine import cycle).""" @@ -1806,6 +1808,7 @@ def __init__( carry_isolated_ledger_writes: Callable[[StoryTask], None], escalation_pause: Callable[..., NoReturn], land_post_landing_writes: Callable[[StoryTask], None] = lambda task: None, + reopen_unlanded_close: Callable[[StoryTask], None] = lambda task: None, workspace_get: Callable[[], Workspace], workspace_set: Callable[[Workspace], None], unit_branch_epic: Callable[[StoryTask], str | None] = lambda task: None, @@ -1828,6 +1831,7 @@ def __init__( self._gate_unit = gate_unit self._carry_isolated_ledger_writes = carry_isolated_ledger_writes self._land_post_landing_writes = land_post_landing_writes + self._reopen_unlanded_close = reopen_unlanded_close self._pause = escalation_pause self._workspace_get = workspace_get self._workspace_set = workspace_set @@ -5099,6 +5103,9 @@ def keep_branch_and_escalate(self, task: StoryTask, unit: UnitWorkspace, reason: diff_max_file_bytes=self.failed_diff_max_bytes(), forensic_extra=self._pinned_config_forensic_extra(task, unit), ) + # A DONE unit whose code did not land: undo its pre-landing writes here, + # since `escalate_unit` leaves a terminal task's alone (DW-569). + self._reopen_unlanded_close(task) self.escalate_unit(task, reason) # always raises RunPaused def seed_workspace_trust( @@ -5168,7 +5175,17 @@ def escalate_unit(self, task: StoryTask, reason: str) -> None: rather than advanced: before dispatch, after a completed merge attempt, at a session launch (the Codex hook-trust gate, :meth:`gate_codex_hook_trust`, which can fire mid-drive), and from :meth:`reopen_unit` for in-flight units. + + An in-flight unit's code has not landed, so ``reopen_unlanded_close`` undoes + the writes that assumed it would (a sweep bundle's close in a shared-store + ledger, DW-569) before the phase is set. A task already terminal is left + alone: the resume replay reaches :meth:`reopen_unit` for a DONE unit whose + merge may already have landed, so its landing is unknown here. + :meth:`keep_branch_and_escalate`, which knows its DONE unit did not land, + makes that call itself. """ + if not task.terminal: + self._reopen_unlanded_close(task) task.phase = Phase.ESCALATED task.adopt_pending = False # an escalation spends any adoption (DW-386) self.journal.append("story-escalated", story_key=task.story_key, reason=reason) diff --git a/tests/test_portability_guard.py b/tests/test_portability_guard.py index 75ce9a585..bc48725a0 100644 --- a/tests/test_portability_guard.py +++ b/tests/test_portability_guard.py @@ -1750,6 +1750,7 @@ "sweep-bundle-name-discarded", "sweep-bundle-name-normalized", "sweep-bundle-reclosed", + "sweep-bundle-reopen-failed", "sweep-bundle-reopened", "sweep-bundle-skipped", "sweep-bundles-truncated", diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 9c3cef1e3..0c91dc49f 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -10,6 +10,7 @@ import time from dataclasses import replace from pathlib import Path +from types import SimpleNamespace from typing import Literal import pytest @@ -51,6 +52,7 @@ from bmad_loop import artifact_publication, deferredwork, platform_util, runs from bmad_loop import sweep as sweep_mod from bmad_loop import ticketstore, verify +from bmad_loop import worktree_flow as worktree_flow_mod from bmad_loop.adapters.base import SessionResult from bmad_loop.adapters.mock import MockAdapter from bmad_loop.bmadconfig import ProjectPaths, load_paths @@ -67,7 +69,7 @@ TokenUsage, VerifyOutcome, ) -from bmad_loop.plugins import Plugin, PluginManifest, PluginRegistry +from bmad_loop.plugins import HookContext, Plugin, PluginManifest, PluginRegistry from bmad_loop.plugins.model import LoadedPlugin from bmad_loop.policy import ( AdapterPolicy, @@ -38736,6 +38738,375 @@ def test_v7_isolated_deferred_bundle_publishes_its_shared_store_ledger(project_v assert [r["commit"] for r in _records(engine, "store-committed")] == commits +_V7_STORE_LAYOUTS = pytest.mark.parametrize( + ("fixture", "layout"), + [("project_v7_store_repo", "T3"), ("project_v7_external_store", "T4")], +) + + +def _v7_isolated_close_seam( + v7, + layout: str, + *, + phase: Phase = Phase.DEV_VERIFY, + worktree: bool = True, + intended: bool = True, + commit_close: bool = False, +): + """DW-569 seam setup: a v7 sweep over a store with its own repository + (``layout`` T3/T4) whose ledger, `{DW-1: open}`, is committed in that store, + and a bundle task for DW-1 whose accepted-dev close is written straight into + that SHARED ledger (reopenable, under the bundle's own note and operation id), + as an isolated mount writes it. ``worktree`` records a ``worktree_path`` (the + isolated arm); ``intended`` records ``bundle_closes_intended``, which only the + accepted-dev close assigns. ``commit_close`` commits the close into the + store, so the store's HEAD holds DW-1 `done` and a reopen leaves a real + delta. Returns the engine, its paths, the store, the ledger's + store-relative path and the task.""" + store = v7.output_folder + paths = _v7_paths(v7) + write_ledger(paths, {"DW-1": "open"}, commit=False) + git(store, "add", "-A") + git(store, "commit", "-q", "-m", "ledger") + ledger_rel = paths.deferred_work.resolve().relative_to(store.resolve()).as_posix() + topology = ticketstore.assess_store(paths, "none") + assert isinstance(topology, ticketstore.StoreTopology) and topology.layout == layout + engine, _ = make_sweep(paths, [], store_topology=topology.to_dict()) + # premise: the ledger lives in the shared store every checkout reads + assert engine.paths.in_shared_store(engine.paths.deferred_work) + assert not engine._isolated # premise: only `worktree_path` takes the isolated arm + task = StoryTask( + story_key="dw-fix-things", + epic=0, + dw_ids=["DW-1"], + phase=phase, + worktree_path=str(paths.repo_root.parent / "unit") if worktree else "", + ) + if intended: + task.bundle_closes_intended = list(task.dw_ids) + engine.state.tasks[task.story_key] = task + deferredwork.mark_done_many_reopenable( + engine.paths.deferred_work, + task.dw_ids, + engine._today(), + engine._bundle_close_note(task), + engine._bundle_close_operation_id(task), + ) + assert not ledger_entries(paths)["DW-1"].open # premise: the close is in the shared ledger + assert git(store, "status", "--porcelain", "--", ledger_rel) != "" # premise: uncommitted + if commit_close: + git(store, "add", "--", ledger_rel) + git(store, "commit", "-q", "-m", "close") + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" # premise + assert deferredwork.open_ids(git(store, "show", f"HEAD:{ledger_rel}")) == set() + return engine, paths, store, ledger_rel, task + + +def _assert_close_reopened(engine, paths, store: Path, ledger_rel: str) -> None: + """The bundle's close is undone in the shared store: one reopen row, DW-1 + open on disk and at the store's HEAD, and the store's ledger clean (the + reopen restores the committed bytes exactly, so the publish has nothing to + commit).""" + assert [r["dw_ids"] for r in _records(engine, "sweep-bundle-reopened")] == [["DW-1"]] + assert ledger_entries(paths)["DW-1"].open + assert deferredwork.open_ids(git(store, "show", f"HEAD:{ledger_rel}")) == {"DW-1"} + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + assert _records(engine, "sweep-bundle-reopen-failed") == [] + + +@_V7_STORE_LAYOUTS +def test_v7_isolated_escalated_bundle_reopens_its_shared_store_close(request, fixture, layout): + """DW-569. T3/T4 under isolation, driven at the seam: a mounted bundle wrote + its accepted-dev close of DW-1 straight into the SHARED store's ledger, then + escalated. No code landed, so `SweepEngine._escalate` reopens the close + before the base's ESCALATED save (`sweep-bundle-reopened`): DW-1 is open on + disk and at the store's HEAD and the store's ledger is clean, so a later + sweep re-bundles it. + + ABLATION (performed): drop the `_reopen_unlanded_close` call in + `SweepEngine._escalate` and both rows fail (no reopen row; DW-1 stays `done` in the shared ledger). + """ + engine, paths, store, ledger_rel, task = _v7_isolated_close_seam( + request.getfixturevalue(fixture), layout + ) + + with pytest.raises(RunPaused): + engine._escalate(task, "verify failed") + + assert task.phase == Phase.ESCALATED + assert task.bundle_closes_intended == ["DW-1"] # kept, so a re-drive re-closes + _assert_close_reopened(engine, paths, store, ledger_rel) + assert load_state(engine.run_dir).tasks[task.story_key].phase == Phase.ESCALATED + + +def test_v7_isolated_escalated_bundle_redrive_recloses(project_v7_store_repo): + """DW-569. T3: after an isolated escalation reopened the bundle's close, + the re-armed bundle lands and `_carry_isolated_ledger_writes` re-closes DW-1 + from the kept `bundle_closes_intended`. + + INVERSE ablation (performed): clear `task.bundle_closes_intended` at the end of + `_reopen_unlanded_close` and this fails (the carry finds no intended close; + DW-1 stays open).""" + engine, paths, _store, _ledger_rel, task = _v7_isolated_close_seam(project_v7_store_repo, "T3") + with pytest.raises(RunPaused): + engine._escalate(task, "verify failed") + assert ledger_entries(paths)["DW-1"].open # premise: the escalation reopened it + + engine._carry_isolated_ledger_writes(task) + + assert not ledger_entries(paths)["DW-1"].open + + +def test_v7_merge_back_escalation_reopens_its_shared_store_close( + project_v7_store_repo, monkeypatch +): + """DW-569. T3: a DONE bundle whose merge-back failed reaches + `WorktreeFlow.keep_branch_and_escalate`. A DONE unit whose code did not land + is a non-landing end, so its close is reopened before the ESCALATED save, + although `escalate_unit` alone leaves a terminal task's close standing. + + ABLATION (performed): drop the `_reopen_unlanded_close` call in + `WorktreeFlow.keep_branch_and_escalate` and this fails (no reopen row; DW-1 + stays `done`).""" + engine, paths, store, ledger_rel, task = _v7_isolated_close_seam( + project_v7_store_repo, "T3", phase=Phase.DONE + ) + closed: list[str] = [] + monkeypatch.setattr( + worktree_flow_mod, + "close_unit_workspace", + lambda unit, **kwargs: closed.append(kwargs["unit_key"]), + ) + unit = SimpleNamespace(path=Path(task.worktree_path)) + + with pytest.raises(RunPaused): + engine._worktree_flow.keep_branch_and_escalate(task, unit, "merge conflict") + + assert closed == [task.story_key] # premise: the merge-back failure path ran + assert task.phase == Phase.ESCALATED + _assert_close_reopened(engine, paths, store, ledger_rel) + + +def test_v7_inflight_escalate_unit_reopens_its_shared_store_close(project_v7_store_repo): + """DW-569. T3: an in-flight isolated unit escalated by `WorktreeFlow` (a + pruned worktree on resume, a workspace-trust fault, the Codex hook-trust + gate) reaches `escalate_unit` with no landing. Its close is reopened before + the ESCALATED save. + + ABLATION (performed): drop the `_reopen_unlanded_close` call in + `WorktreeFlow.escalate_unit` and this fails (no reopen row; DW-1 stays + `done`).""" + engine, paths, store, ledger_rel, task = _v7_isolated_close_seam(project_v7_store_repo, "T3") + + with pytest.raises(RunPaused): + engine._worktree_flow.escalate_unit(task, "worktree is gone") + + assert task.phase == Phase.ESCALATED + _assert_close_reopened(engine, paths, store, ledger_rel) + + +def test_v7_terminal_escalate_unit_keeps_its_shared_store_close(project_v7_store_repo): + """DW-569. T3: a DONE unit reaching `escalate_unit` directly is the resume + replay's `reopen_unit` over a unit whose merge may already have landed; + its landing is unknown, so the close stands: no reopen row, DW-1 still + `done`. + + ABLATION (performed): drop the `not task.terminal` gate in `WorktreeFlow.escalate_unit` + and this fails (the close is reopened).""" + engine, paths, _store, _ledger_rel, task = _v7_isolated_close_seam( + project_v7_store_repo, "T3", phase=Phase.DONE + ) + + with pytest.raises(RunPaused): + engine._worktree_flow.escalate_unit(task, "worktree is gone") + + assert task.phase == Phase.ESCALATED # premise: the escalation ran + assert _records(engine, "sweep-bundle-reopened") == [] + assert not ledger_entries(paths)["DW-1"].open + + +@pytest.mark.parametrize("end", ["escalate", "veto-defer"]) +def test_v7_unlanded_bundle_publishes_its_reopened_store_close(project_v7_store_repo, end): + """DW-569. T3 under isolation, with a real delta: the bundle's close of + DW-1 is already COMMITTED in the store, so the store's HEAD holds DW-1 + `done`. When the bundle escalates (or a plugin defers it), the reopen + changes the ledger and `_reopen_unlanded_close` publishes it: exactly one + deferred-bundle store commit touching only the ledger, DW-1 open in it, the + store's ledger clean, and its `store-committed` row naming it. + + ABLATION (performed): drop the `self._publish_deferred_ledger(task)` call at + the end of `SweepEngine._reopen_unlanded_close` and both rows fail (no + deferred-bundle commit; the reopen stays dirty in the store).""" + engine, paths, store, ledger_rel, task = _v7_isolated_close_seam( + project_v7_store_repo, "T3", commit_close=True + ) + + if end == "escalate": + with pytest.raises(RunPaused): + engine._escalate(task, "verify failed") + else: + ctx = HookContext("pre_review_phase", run_id=engine.state.run_id, story_key=task.story_key) + ctx.veto("defer", "policy says no") + assert engine._vetoed(ctx, task) is True + + assert [r["dw_ids"] for r in _records(engine, "sweep-bundle-reopened")] == [["DW-1"]] + assert ledger_entries(paths)["DW-1"].open + commits = _store_deferred_commits(store) + assert len(commits) == 1, git(store, "log", "--format=%s") + assert git(store, "show", "--name-only", "--format=", commits[0]).split() == [ledger_rel] + assert deferredwork.open_ids(git(store, "show", f"{commits[0]}:{ledger_rel}")) == {"DW-1"} + assert git(store, "status", "--porcelain", "--", ledger_rel) == "" + assert [r["commit"] for r in _records(engine, "store-committed")] == commits + + +@pytest.mark.parametrize("action", ["defer", "skip", "pause"]) +def test_v7_isolated_vetoed_bundle_reopens_its_shared_store_close(project_v7_store_repo, action): + """DW-569. T3 under isolation: a plugin vetoes the mounted bundle after its + accepted-dev close went into the shared store's ledger. Every veto action + is a non-landing end (`defer`/`skip` DEFERRED, `pause` ESCALATED), so + `SweepEngine._vetoed` reopens the close before the base's terminal save. + + ABLATION (performed): drop the `_reopen_unlanded_close` call in `SweepEngine._vetoed` + and all three rows fail (no reopen row; DW-1 stays `done`).""" + engine, paths, store, ledger_rel, task = _v7_isolated_close_seam(project_v7_store_repo, "T3") + ctx = HookContext("pre_review_phase", run_id=engine.state.run_id, story_key=task.story_key) + ctx.veto(action, "policy says no") + + if action == "pause": + with pytest.raises(RunPaused): + engine._vetoed(ctx, task) + assert task.phase == Phase.ESCALATED + else: + assert engine._vetoed(ctx, task) is True + assert task.phase == Phase.DEFERRED + assert [r["action"] for r in _records(engine, "plugin-veto")] == [action] # premise + _assert_close_reopened(engine, paths, store, ledger_rel) + + +def test_v7_unvetoed_bundle_keeps_its_shared_store_close(project_v7_store_repo): + """DW-569. T3 under isolation: a stage nobody vetoed is not an end, so + `_vetoed` returns False and the close stands. + + ABLATION (performed): drop the `ctx.resolved_veto() is not None` gate in + `SweepEngine._vetoed` and this fails (the close is reopened).""" + engine, paths, _store, _ledger_rel, task = _v7_isolated_close_seam(project_v7_store_repo, "T3") + ctx = HookContext("pre_review_phase", run_id=engine.state.run_id, story_key=task.story_key) + + assert engine._vetoed(ctx, task) is False + assert engine._vetoed(None, task) is False + + journal = engine.run_dir / "journal.jsonl" + assert not journal.exists() or _records(engine, "sweep-bundle-reopened") == [] + assert not ledger_entries(paths)["DW-1"].open + + +@pytest.mark.parametrize("shape", ["no-close-intended", "in-place"]) +def test_v7_escalated_bundle_reopen_is_isolated_close_only(project_v7_store_repo, shape): + """DW-569. T3: the escalation reopen acts only on an isolated bundle that + recorded its close. With no `bundle_closes_intended` (no accepted-dev close + on record), or in place (no `worktree_path`, not isolated: the base's + rollback owns the ledger), the shared ledger is untouched: no reopen row + and DW-1 still `done` as planted. + + Ablations (performed): drop the `bundle_closes_intended` guard in + `SweepEngine._reopen_unlanded_close` and `no-close-intended` fails; drop its + isolated-arm guard and `in-place` fails (each reopens the planted close).""" + engine, paths, _store, _ledger_rel, task = _v7_isolated_close_seam( + project_v7_store_repo, + "T3", + worktree=shape != "in-place", + intended=shape != "no-close-intended", + ) + + with pytest.raises(RunPaused): + engine._escalate(task, "verify failed") + + assert task.phase == Phase.ESCALATED # premise: the escalation ran + assert _records(engine, "sweep-bundle-reopened") == [] + assert _records(engine, "sweep-bundle-reopen-failed") == [] + assert not ledger_entries(paths)["DW-1"].open + + +def test_v7_escalated_bundle_with_unshared_ledger_keeps_its_close(project_v7): + """DW-569. T1 (the store is tracked inside the code checkout): each checkout + carries its own ledger, so an isolated bundle's close lives in the unit + worktree's copy and dies with it. The escalation reopen leaves this + checkout's ledger alone even with a close on record and a `worktree_path`: + no reopen row, and the close planted here stands. + + ABLATION (performed): drop the `in_shared_store` guard in + `SweepEngine._reopen_unlanded_close` and this fails (it reopens the close).""" + paths = _v7_paths(project_v7) + write_ledger(paths, {"DW-1": "open"}) + topology = ticketstore.assess_store(paths, "none") + assert isinstance(topology, ticketstore.StoreTopology) and topology.layout == "T1" + engine, _ = make_sweep(paths, [], store_topology=topology.to_dict()) + assert not engine.paths.in_shared_store(engine.paths.deferred_work) # premise + task = StoryTask( + story_key="dw-fix-things", + epic=0, + dw_ids=["DW-1"], + phase=Phase.DEV_VERIFY, + worktree_path=str(paths.repo_root.parent / "unit"), + bundle_closes_intended=["DW-1"], + ) + engine.state.tasks[task.story_key] = task + deferredwork.mark_done_many_reopenable( + engine.paths.deferred_work, + task.dw_ids, + engine._today(), + engine._bundle_close_note(task), + engine._bundle_close_operation_id(task), + ) + assert not ledger_entries(paths)["DW-1"].open # premise: a close to undo + + with pytest.raises(RunPaused): + engine._escalate(task, "verify failed") + + assert task.phase == Phase.ESCALATED # premise: the escalation ran + assert _records(engine, "sweep-bundle-reopened") == [] + assert _records(engine, "sweep-bundle-reopen-failed") == [] + assert not ledger_entries(paths)["DW-1"].open + + +@pytest.mark.parametrize("fault", ["ledger-read", "os"]) +def test_v7_isolated_escalation_reopen_fault_is_journaled_degrade( + project_v7_store_repo, monkeypatch, fault +): + """DW-569. T3: the escalation's reopen of the shared close faults + (`LedgerReadError`, or a raw `OSError` from the lock or write). The + escalation is the operator stop, so it still pauses with its own reason; + the fault is a visible degrade (`sweep-bundle-reopen-failed` naming the + bundle's ids and the error), the publish is skipped (no store commit), and + DW-1 stays `done`. + + ABLATION (performed): drop the `except (deferredwork.LedgerReadError, OSError)` arm in + `SweepEngine._reopen_unlanded_close` and both rows fail (the fault escapes + in place of `RunPaused`).""" + engine, paths, store, ledger_rel, task = _v7_isolated_close_seam(project_v7_store_repo, "T3") + head = git(store, "rev-parse", "HEAD").strip() + + def mark_open_many(*args, **kwargs): + if fault == "ledger-read": + raise deferredwork.LedgerReadError("injected reopen read fault") + raise OSError("injected reopen write fault") + + monkeypatch.setattr(deferredwork, "mark_open_many", mark_open_many) + + with pytest.raises(RunPaused) as exc: + engine._escalate(task, "verify failed") + + assert exc.value.args[0] == "verify failed" + assert task.phase == Phase.ESCALATED + failed = _records(engine, "sweep-bundle-reopen-failed") + assert [(r["story_key"], r["dw_ids"]) for r in failed] == [(task.story_key, ["DW-1"])] + assert "injected reopen" in failed[0]["error"] + assert _records(engine, "sweep-bundle-reopened") == [] + assert not ledger_entries(paths)["DW-1"].open + assert git(store, "rev-parse", "HEAD").strip() == head # no publish + assert _records(engine, "store-committed") == [] + + def _store_carry_commits(store: Path, story_key: str) -> list[str]: """The shas of the store's harvested-carry commits for ``story_key``, newest first (DW-570).""" From bd0a75d5a864cdc138db9931601b8a022dbfd6d5 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 18 Oct 2026 19:17:01 -0700 Subject: [PATCH 05/10] sweep dw-isolated-sweep-reopen-on-abandon: DW-571 via bmad-loop --- CHANGELOG.md | 7 + docs/reference/cli.md | 5 +- docs/sweep-guide.md | 27 +-- src/bmad_loop/cli.py | 94 ++++++++- src/bmad_loop/deferredwork.py | 12 ++ src/bmad_loop/runs.py | 216 ++++++++++++++++++- src/bmad_loop/sweep.py | 32 ++- src/bmad_loop/tui/app.py | 8 +- tests/test_cleanup.py | 159 ++++++++++++++ tests/test_cli.py | 80 ++++++++ tests/test_deferredwork.py | 34 +++ tests/test_engine_worktree.py | 54 ++++- tests/test_portability_guard.py | 8 + tests/test_runs.py | 354 +++++++++++++++++++++++++++++++- tests/test_sweep.py | 161 ++++++++++++++- tests/test_tui_app.py | 10 +- 16 files changed, 1224 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1364353be..ac403c7f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -144,6 +144,13 @@ breaking changes may land in a minor release. fsynced before the replace. A run with no recorded run-dir identity, or one whose device number changed, records one `journal-unpinned` row per writer; `resume` records or re-binds the identity before its first row (DW-559). +- Guard an isolated v7 sweep run that paused, stopped or crashed while holding shared-store + ledger closes for code that never landed: a new sweep warns and journals + `sweep-prior-run-holds-closes` (`sweep-prior-run-closes-unknown` when it cannot tell), + and `delete`, the TUI delete and `clean` refuse to remove that run, naming + `bmad-loop resume `. They also refuse any run, of any type, whose state cannot be + read, and such a sweep run whose config or ledger cannot be read or whose pinned + initiative is no longer active; `delete --force` waives neither refusal (DW-571). ### Removed diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 5f46253cc..08419b46d 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -364,6 +364,8 @@ bmad-loop delete [--project PROJECT] [--force] run_id | `--force` | Hard-stop a live engine first (this ends its agent session), and skip the live-agent-session guard (skipping it kills no session). | - Without `--force`, refuses (exit 1) when the engine is provably alive or an agent session that may belong to the project is live. Unverifiable liveness only warns. +- Refuses (exit 1), even with `--force` and before `--force` stops anything, an unfinished worktree-isolated sweep run that still holds ledger closes in a shared ticket store for code that never landed. The message names the entries and `bmad-loop resume `, which lands or reopens them. See [the sweep guide](../sweep-guide.md#sweeps-on-bmad-v7). +- Also refuses (exit 1), even with `--force`, any run of any type whose state cannot be read, and such a sweep run whose config, store topology or ledger cannot be read, or whose pinned initiative is no longer active while its store is shared, since it cannot be told whether it holds such closes. Repair what could not be read and retry; a run you have confirmed holds no closes can be removed by hand. - Also prunes git worktree entries for worktrees the run held. ### `archive` @@ -374,7 +376,7 @@ Compresses a run into `.bmad-loop/archive/.tar.gz`, then removes the run bmad-loop archive [--project PROJECT] [--force] run_id ``` -- `--force` and the refusals are the same as `delete`. The tarball holds the in-tree run directory only. +- `--force` and the live-engine and live-session refusals are the same as `delete`. `archive` does not refuse a run that holds unlanded sweep ledger closes. The tarball holds the in-tree run directory only. ### `cleanup` @@ -410,6 +412,7 @@ bmad-loop clean [--project PROJECT] [--dry-run] [--keep RUN_ID] [--retain N] [-- | `--json` | Emit one JSON document instead of text. | - Exits 0 once the policy loads; per-run faults are warnings on stderr or fields in the JSON document. +- A run past retention that `delete` would refuse because it holds unlanded sweep ledger closes is skipped on the delete arm (`--hard`, or `[cleanup] archive_old = false`) before anything of it is trimmed. Stderr names it and the entries; `--json` lists it under `protected`. The archive arm keeps the run in its tarball and is not affected. JSON: [`clean --json`](#clean---json). diff --git a/docs/sweep-guide.md b/docs/sweep-guide.md index b77fb6470..b49d514f6 100644 --- a/docs/sweep-guide.md +++ b/docs/sweep-guide.md @@ -91,6 +91,7 @@ A sweep runs on a v7 project the same way, with these differences: - **Pinned initiative.** A sweep records the active initiative it started on. A resume, `resolve` or the dashboard's re-arm refuses while `[core] active_initiative` names another: set it back, or start a new sweep. - **Decisions per initiative.** Each initiative has its own ledger and DW numbering, so pre-answers are kept per initiative in `.bmad-loop/decisions-.json`. `bmad-loop decisions` and the TUI answer the active initiative's decisions and offer only those its own sweeps raised. An answer given after another initiative was made active than the one its decision was listed from is refused; list the decisions again. With no initiative active, the project store stays `.bmad-loop/decisions.json` and only sweeps that ran with no initiative are offered. - **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. The bundle's ledger close lands in that shared ledger the same way, so a bundle that ends without landing its code reopens its close before that end is recorded: when it defers, when it escalates (a failed merge-back included), and when a plugin vetoes it. A ledger fault during that reopen is journaled as `sweep-bundle-reopen-failed` and the escalation or veto goes on, leaving the close for a human to reopen. Resume skips an escalated bundle, so after you merge a failed merge-back's kept branch by hand, its entries stay open until a later sweep triages them as already resolved. +- **A run that still holds closes.** A run that pauses for any other reason, is stopped or crashes after a bundle's accepted dev keeps that bundle's close: the entries read `done` in the shared ledger, and no later sweep re-bundles a `done` entry. Only that run can land or reopen them, so resume it (`bmad-loop resume `). Until then, each new sweep prints a warning naming the run and its entries, and journals `sweep-prior-run-holds-closes`; it still runs and leaves those entries alone. A run it cannot check (unreadable state, config or ledger) is journaled as `sweep-prior-run-closes-unknown`. `bmad-loop delete`, the dashboard's delete and `bmad-loop clean`'s delete arm refuse to remove such a run, and `--force` does not override that. - **Plans in a store repository.** When the ticket store is its own git repository (nested or outside the checkout), the bundle's code commit cannot carry its plan. Once the code has landed, the accepted plan gets its own commit there, `chore(sweep): plan [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code. This is best effort, like a tickets run's store commit: a failure is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and the plan stays uncommitted for a human. The bundle's ledger close is committed there the same best-effort way, as `chore(sweep): close resolved deferred-work entries` with the same trailers and the same journaled failures, and is withheld (`sweep-ledger-commit-withheld`) while the run holds its ledger in doubt. - **Deferred bundles in a store repository.** A bundle that defers never lands code, but its ledger writes (the reopen of its close, any harvested deferrals) are committed in the store repository as `chore(sweep): record a deferred bundle's ledger writes`, the same best-effort and withheld way, with only the `Bmad-Code-Repo` trailer. A landed bundle's harvested deferrals ride its close commit; one that landed with no close commits them as `chore(deferred-work): carry harvested findings from `, as does a resume that replays a deferred bundle's harvest. These are best effort too. @@ -213,18 +214,20 @@ An empty ledger is publishable. A store that is not valid JSON is still publisha `bmad-loop diagnose [run-id]` collects the publication entries in a "Sweep publication and repeat stops" section. For the raw `message`, `repo` and `error` text, read `.bmad-loop/runs//journal.jsonl`. -| Journal kind | Meaning | -| ----------------------------------- | ------------------------------------------------------------------------------------------------- | -| `sweep-ledger-commit` | committed (`message`, `commit`, `file`) | -| `sweep-ledger-commit-clean` | nothing to commit; also seen when the path is gitignored | -| `sweep-ledger-commit-refused` | refused before staging (`refuse_cause`) | -| `sweep-ledger-commit-unavailable` | the ledger is in no git repo, or a store commit failed (`repo`, `error`) | -| `sweep-ledger-commit-withheld` | blocked because the ledger is in doubt (`reason: ledger-in-doubt`) | -| `sweep-bundles-withheld` | bundles not dispatched (this cycle's, or in-flight ones on resume) because the ledger is in doubt | -| `sweep-bundle-skipped` | a bundle skipped whole (`entry-not-open`, `failed-or-escalated-earlier`, `human-chose-keep-open`) | -| `sweep-decision-effect-unavailable` | an answer's ledger effect did not land | -| `sweep-decision-effect-reapplied` | a stored `close` the ledger had not taken was applied again | -| `sweep-repeat-done` | a repeat run stopped (`stop_cause`) | +| Journal kind | Meaning | +| ----------------------------------- | ------------------------------------------------------------------------------------------------------------------------- | +| `sweep-ledger-commit` | committed (`message`, `commit`, `file`) | +| `sweep-ledger-commit-clean` | nothing to commit; also seen when the path is gitignored | +| `sweep-ledger-commit-refused` | refused before staging (`refuse_cause`) | +| `sweep-ledger-commit-unavailable` | the ledger is in no git repo, or a store commit failed (`repo`, `error`) | +| `sweep-ledger-commit-withheld` | blocked because the ledger is in doubt (`reason: ledger-in-doubt`) | +| `sweep-bundles-withheld` | bundles not dispatched (this cycle's, or in-flight ones on resume) because the ledger is in doubt | +| `sweep-bundle-skipped` | a bundle skipped whole (`entry-not-open`, `failed-or-escalated-earlier`, `human-chose-keep-open`) | +| `sweep-decision-effect-unavailable` | an answer's ledger effect did not land | +| `sweep-decision-effect-reapplied` | a stored `close` the ledger had not taken was applied again | +| `sweep-repeat-done` | a repeat run stopped (`stop_cause`) | +| `sweep-prior-run-holds-closes` | at a sweep's start: an earlier run still holds closes whose code never landed (`run_id`, `dw_ids`) | +| `sweep-prior-run-closes-unknown` | at a sweep's start: an earlier run could not be checked (`run_id`, absent when the runs dir could not be listed; `error`) | A ledger `git status` fault other than "not a repository", or a commit git refuses, ends the sweep as a crash: the run records `run-crash` and writes `crash.txt` in the run dir. diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 967ea54c8..adc16c17b 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -2183,6 +2183,46 @@ def _warn_bypass_dropped( ) +def _warn_prior_sweep_closes(project: Path, run_id: str, journal: Journal) -> None: + """Warn at a sweep's start about earlier runs that still hold sweep ledger + closes whose code never landed (``runs.prior_sweep_closes``). Such a close + reads ``done`` in the shared ticket store, so this sweep cannot re-bundle it. + Only the held run can land or reopen it. + + Writes one journal row and one stderr ``warning:`` line per finding: + ``sweep-prior-run-holds-closes`` for a held run, and + ``sweep-prior-run-closes-unknown`` when a run or the runs dir could not be + checked. Read-only. It never blocks or changes the new sweep, and the new + run and live runs are skipped.""" + for row in runs.prior_sweep_closes(project, exclude=run_id): + if row.run_id is None: + journal.append("sweep-prior-run-closes-unknown", error=row.fault) + print( + "warning: could not check earlier runs for sweep ledger closes whose code " + f"never landed: {row.fault}", + file=sys.stderr, + ) + elif row.fault is not None: + journal.append("sweep-prior-run-closes-unknown", run_id=row.run_id, error=row.fault) + print( + f"warning: could not check run {row.run_id} for sweep ledger closes whose " + f"code never landed: {row.fault}. If it holds any, `bmad-loop resume " + f"{row.run_id}` lands or reopens them", + file=sys.stderr, + ) + else: + journal.append( + "sweep-prior-run-holds-closes", run_id=row.run_id, dw_ids=list(row.dw_ids) + ) + print( + f"warning: run {row.run_id} holds sweep ledger closes whose code never " + f"landed ({', '.join(row.dw_ids)}). They read `done` in the shared ticket " + f"store, so this sweep will not re-bundle them; run `bmad-loop resume " + f"{row.run_id}` to land or reopen them", + file=sys.stderr, + ) + + def _fatal_plugin_problems(paths: bmadconfig.ProjectPaths, pol) -> list[str]: """The fatal plugin-trust refusals `_reject_plugin_trust` would print, as banner lines for `_warn_preflight_would_abort` (no stderr, never raises).""" @@ -4453,6 +4493,7 @@ def _start_sweep( profiles=profiles, journal=composed.journal, ) + _warn_prior_sweep_closes(project, composed.run_id, composed.journal) print(f"sweep {composed.run_id} starting (attach: bmad-loop attach)") summary = composed.engine.run() print(summary.render()) @@ -8101,12 +8142,23 @@ def cmd_delete(args: argparse.Namespace) -> int: print(str(e), file=sys.stderr) return 1 args.run_id = run_dir.name + if args.force: + try: + # Before `--force` can hard-stop a live engine: a held run is refused + # anyway, and stopping it first would leave it stopped with nothing + # deleted. Without `--force` a live run gets the live-engine refusal + # first. `delete_run` re-checks under the lock. + runs.refuse_held_sweep_closes(project, run_dir) + except runs.HeldSweepClosesError as e: + print(str(e), file=sys.stderr) + return 1 rc = _stop_or_block_live_engine(run_dir, args.run_id, args.force) if rc is not None: return rc try: runs.delete_run(project, run_dir, force=args.force) - except runs.LiveEngineError as e: + except (runs.LiveEngineError, runs.HeldSweepClosesError) as e: + # `--force` waives neither: the held-closes refusal names its own remedy. print(str(e), file=sys.stderr) return 1 except runs.LiveSessionError as e: @@ -8504,6 +8556,21 @@ def _human_bytes(n: int) -> str: return f"{size:.1f}TB" +def _held_closes_reason(e: runs.HeldSweepClosesError) -> str: + """`clean`'s one-line skip reason for a run `delete_run` refuses because it + holds unlanded sweep ledger closes, or could not be checked.""" + if e.reason is not None: + return ( + f"could not be checked for sweep ledger closes whose code never landed " + f"({e.reason}); repair it and retry, or `bmad-loop resume {e.run_id}` once it " + "reads; a run confirmed to hold none can be removed by hand" + ) + return ( + f"holds sweep ledger closes whose code never landed ({', '.join(e.dw_ids)}); " + f"`bmad-loop resume {e.run_id}` lands or reopens them" + ) + + def cmd_clean(args: argparse.Namespace) -> int: """Reclaim disk from concluded runs: tear down worktrees leaked by a mid-flight stop, trim heavy scaffolding from runs kept for history, and @@ -8576,6 +8643,22 @@ def cmd_clean(args: argparse.Namespace) -> int: file=sys.stderr, ) continue + if run_dir.name in past and (args.hard or not pol.cleanup.archive_old): + # A sweep run that holds ledger closes whose code never landed (or + # cannot be checked) is the only record that can land or reopen them. + # `delete_run` refuses it; asking first, before any trim or worktree + # removal, leaves it whole instead of half-reclaimed. The archive arm + # keeps the run's record in its tarball and is not gated. + try: + runs.refuse_held_sweep_closes(project, run_dir) + except runs.HeldSweepClosesError as e: + protected.append(run_dir.name) + if not args.json: + print( + f"run {run_dir.name}: {_held_closes_reason(e)} — not removed", + file=sys.stderr, + ) + continue if runs.engine_liveness(run_dir) == "unknown": # warn-only: unknown never blocks cleanup, but say so before removal. # In JSON mode this lives in the document instead (unverifiable_pid), @@ -8621,7 +8704,12 @@ def cmd_clean(args: argparse.Namespace) -> int: if not dry: runs.archive_run(project, run_dir, wait_for_lock=False) archived.append(run_dir.name) - except (runs.LiveEngineError, runs.LiveSessionError, LockUnavailableError) as e: + except ( + runs.LiveEngineError, + runs.LiveSessionError, + runs.HeldSweepClosesError, + LockUnavailableError, + ) as e: # A session or engine appeared between the loop-top sample and the # authoritative removal transaction — or the run's state lock is # held, which says the same thing one layer down and is the only @@ -8647,6 +8735,8 @@ def cmd_clean(args: argparse.Namespace) -> int: if not args.json: if isinstance(e, runs.LiveSessionError): reason = "agent session appeared mid-clean" + elif isinstance(e, runs.HeldSweepClosesError): + reason = _held_closes_reason(e) elif isinstance(e, LockUnavailableError): reason = "run state locked by another process" else: diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index f4bdf1bf9..6a538e5f9 100644 --- a/src/bmad_loop/deferredwork.py +++ b/src/bmad_loop/deferredwork.py @@ -1878,6 +1878,18 @@ def _apply_open_many( return text, reopened +def held_reopenable_closes( + text: str, dw_ids: Sequence[str], note: str, operation_id: str +) -> list[str]: + """The ids in `dw_ids` whose close in `text` :func:`mark_open_many` would undo + under `note` and `operation_id`, in the order given — the closes that + operation still holds. Pure and read-only: the same fold the writer decides + with (:func:`_apply_open_many`), so "would reopen" and "is held" cannot + drift. An entry already reopened, closed by another operation, or missing is + not held.""" + return _apply_open_many(text, dw_ids, note, _operation_digest(operation_id))[1] + + def mark_open_many(path: Path, dw_ids: Sequence[str], note: str, operation_id: str) -> list[str]: """Undo every close in `dw_ids` written by :func:`mark_done_many_reopenable` under `operation_id`, in ONE read and ONE atomic write. Returns the ids diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 8aa325b12..142fc57bb 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -40,6 +40,7 @@ envvars, platform_util, sprintstatus, + ticketstore, tickettree, verify, worksource, @@ -3600,6 +3601,198 @@ def _refuse_uncontained_run_dir(project: Path, run_dir: Path, action: str) -> No node = parent +# ------------------------------------------- sweep closes a run still holds + + +def bundle_close_operation_id(run_id: str, story_key: str) -> str: + """The stable operation id of a sweep bundle's reopenable ledger close and of + its undo: ``SweepEngine._bundle_close_operation_id`` and + :func:`held_sweep_closes` both use it, so there is only one format.""" + return f"{run_id}/{story_key}" + + +def bundle_close_note(story_key: str) -> str: + """The resolution note of a sweep bundle's ledger close and of its undo. This + is the only definition; ``SweepEngine._bundle_close_note`` calls it.""" + return f"resolved by sweep bundle {story_key}" + + +class HeldClosesUnknownError(Exception): + """Could not tell whether a run holds unlanded sweep ledger closes: its state, + the BMAD config, its recorded store topology or the ledger could not be + read. Never read this as "none held".""" + + +class HeldSweepClosesError(Exception): + """:func:`delete_run` refused a run that holds unlanded sweep ledger closes, + or one where that could not be told. ``dw_ids`` lists the held ids (empty + when ``reason`` names the fault that stopped the check).""" + + def __init__(self, run_id: str, dw_ids: list[str], reason: str | None = None) -> None: + self.run_id = run_id + self.dw_ids = list(dw_ids) + self.reason = reason + if reason is None: + what = ( + f"it holds sweep ledger closes whose code never landed " + f"({', '.join(self.dw_ids)}). They read `done` in the shared ticket " + "store's ledger, and no later sweep re-bundles a `done` entry" + ) + remedy = f"Run `bmad-loop resume {run_id}` to land or reopen them first" + else: + what = ( + "it could not be checked for sweep ledger closes whose code never " + f"landed: {reason}" + ) + remedy = ( + "Repair what could not be read and retry, or `bmad-loop resume " + f"{run_id}` once it reads to land or reopen any closes it holds; a run " + "you have confirmed holds none can be removed by hand" + ) + super().__init__(f"run {run_id}: refusing to delete it — {what}. {remedy}") + + +def held_sweep_closes(project: Path, run_dir: Path) -> list[str]: + """The deferred-work ids whose ledger close the sweep run at ``run_dir`` still + holds with no code landed, in task order. An isolated v7 bundle over a shared + ticket store (T2–T4, T6) writes its accepted-dev close straight into the one + shared ledger. If the run then pauses, stops or crashes before landing, that + `done` stays, and ``deferredwork.open_ids`` hides it from every later sweep. + The engine reopens it only when the run escalates, is vetoed or defers. + + A run holds a close when all of these are true: + + - it is an unfinished, isolated sweep (``state.target_branch`` or the task's + ``worktree_path``); + - it has a task that is not ``DONE`` with ``bundle_closes_intended`` set; + - the ledger lies in the shared store (``bmadconfig.load_paths`` with the + run's recorded topology, :func:`live_store_topology`); + - the ledger still shows that task's close as undoable under its note and + operation id (``deferredwork.held_reopenable_closes``), which is exactly + what ``deferredwork.mark_open_many`` would reopen. + + Read-only. An absent ``state.json`` or ledger answers ``[]``: there is no + record of a close. Any other fault raises :class:`HeldClosesUnknownError` + and is never folded into ``[]``. Used by the :func:`delete_run` refusal and + the sweep-start warning (:func:`prior_sweep_closes`).""" + try: + state = load_state(run_dir) + except (FileNotFoundError, NotADirectoryError): + return [] + except Exception as e: # any unreadable or unparseable state is "unknown" + raise HeldClosesUnknownError( + f"its run state could not be read ({type(e).__name__}: {e})" + ) from e + if state.run_type != "sweep" or state.finished: + return [] + candidates = [ + task + for task in state.tasks.values() + if task.phase != Phase.DONE + and task.bundle_closes_intended + and (state.target_branch or task.worktree_path) + ] + if not candidates: + return [] + try: + paths = ticketstore.with_shared_store( + bmadconfig.load_paths(project), live_store_topology(state, project) + ) + except Exception as e: # a config or topology fault is "unknown", never "none" + raise HeldClosesUnknownError( + f"the BMAD config or the run's store topology could not be read " + f"({type(e).__name__}: {e})" + ) from e + if paths.shared_store is None: + # No shared store under the recorded topology: every close went through a + # checkout, whichever initiative is active now. + return [] + from .runsetup import sweep_initiative_conflict # lazy: runsetup imports runs + + if (conflict := sweep_initiative_conflict(state, paths)) is not None: + # The live paths resolve another initiative's ledger, which holds nothing + # under this run's op id: answering `[]` there would wave the holder through. + raise HeldClosesUnknownError(conflict) + ledger = paths.deferred_work + if not paths.in_shared_store(ledger): + return [] + text, fault = deferredwork.observe_ledger(ledger) + if fault is not None: + raise HeldClosesUnknownError( + f"the deferred-work ledger {ledger} could not be read ({fault})" + ) + if text is None: + return [] + held: list[str] = [] + for task in candidates: + held.extend( + deferredwork.held_reopenable_closes( + text, + task.bundle_closes_intended, + bundle_close_note(task.story_key), + bundle_close_operation_id(state.run_id, task.story_key), + ) + ) + return list(dict.fromkeys(held)) + + +@dataclass(frozen=True) +class PriorSweepCloses: + """One row of :func:`prior_sweep_closes`. ``run_id`` is None when the runs + dir itself could not be listed. ``fault`` is None when the run was read and + holds ``dw_ids``.""" + + run_id: str | None + dw_ids: tuple[str, ...] = () + fault: str | None = None + + +def prior_sweep_closes(project: Path, *, exclude: str) -> list[PriorSweepCloses]: + """The sweep-start scan: every run, other than ``exclude`` (the new sweep) and + live runs, that holds unlanded sweep closes (:func:`held_sweep_closes`) or + could not be checked. A live run's close is still in flight, so it is skipped. + A runs dir that cannot be listed gives a single fault row with no run id. + Read-only. A row only warns the caller and never blocks the new sweep.""" + names, fault = _run_dir_names(project) + if names is None: + return [PriorSweepCloses(None, fault=fault)] + rows: list[PriorSweepCloses] = [] + for name in sorted(names): + if name == exclude: + continue + run_dir = project / RUNS_DIR / name + try: + alive = engine_liveness(run_dir) == "alive" + except ProcessHostError as e: + # A misconfigured process host must not block the new sweep: a fault row. + rows.append( + PriorSweepCloses(name, fault=f"its engine liveness could not be probed ({e})") + ) + continue + if alive: + continue + try: + held = held_sweep_closes(project, run_dir) + except HeldClosesUnknownError as e: + rows.append(PriorSweepCloses(name, fault=str(e))) + continue + if held: + rows.append(PriorSweepCloses(name, tuple(held))) + return rows + + +def refuse_held_sweep_closes(project: Path, run_dir: Path) -> None: + """Raise :class:`HeldSweepClosesError` when ``run_dir`` holds unlanded sweep + closes or could not be checked (:func:`held_sweep_closes`). This is the + :func:`delete_run` gate. ``clean`` also calls it before it trims anything.""" + try: + held = held_sweep_closes(project, run_dir) + except HeldClosesUnknownError as e: + raise HeldSweepClosesError(run_dir.name, [], str(e)) from e + if held: + raise HeldSweepClosesError(run_dir.name, held) + + def delete_run( project: Path, run_dir: Path, @@ -3644,14 +3837,22 @@ def delete_run( multiplexer and let the removal proceed anyway (DW-466); ``None`` prints it on stderr. See :func:`live_session_may_be_ours`. + After the liveness and session guards, still under the lock and before any + removal, a sweep run that holds unlanded ledger closes raises + :class:`HeldSweepClosesError` (:func:`refuse_held_sweep_closes`). So does a + run that could not be checked. ``force`` does not waive this; the failed-launch + unwind (``_expected_composer_claim``) skips it, because a run it just composed + holds nothing. + A run dir holding ``worktrees/`` (a paused isolated run's kept unit), or whose state records a unit mount (``worktrees/`` already removed by hand), is pruned from git's worktree admin after the removal, against the run's own code root (DW-538; a project-owned one rebased onto a renamed ``project``) — the rmtree alone left ``.git/worktrees/`` registered. A prune that leaves an entry under the run dir, or a listing that cannot say, is - reported through ``warn`` too; the deletion itself has already happened. An - unreadable state leaves the code root unknown: ``project`` is pruned as the - likely one, and ``warn`` always says the cleanup is unconfirmed.""" + reported through ``warn`` too; the deletion itself has already happened. A + missing state (an unreadable one is refused above, except on the failed-launch + unwind) leaves the code root unknown: ``project`` is pruned as the likely one, + and ``warn`` always says the cleanup is unconfirmed.""" _refuse_uncontained_run_dir(project, run_dir, "delete") with state_lock(run_dir, blocking=wait_for_lock): if _expected_composer_claim is not None: @@ -3680,6 +3881,13 @@ def delete_run( ) if not force: _refuse_live_session(project, run_dir.name, "delete", warn=warn) + if _expected_composer_claim is None: + # A sweep run holding ledger closes whose code never landed is the only + # record that can land or reopen them, so it is never deleted. Neither + # is a run where that cannot be told. `force` does not waive this: it + # overrides the session guard only. The failed-launch unwind skips it, + # because a run it just composed holds nothing. + refuse_held_sweep_closes(project, run_dir) # Read before the rmtree takes them: a kept unit worktree under the run dir # is registered with the run's code root, which the rmtree does not tell. A # recorded mount counts even with `worktrees/` already gone by hand — its @@ -3700,7 +3908,7 @@ def delete_run( # No state, no code root: prune the usual one, but a clean listing # there proves nothing for a run that recorded a `repo_root` override. fault = _prune_deleted_run_worktrees(project, run_res) or ( - "its run state was unreadable, so the repository its worktrees " + "its run state was missing or unreadable, so the repository its worktrees " f"were registered with is unknown; if that is not {project}, run " "`git worktree prune` there" ) diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 055db1ecb..928912d4e 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -51,7 +51,14 @@ open_dir_confined, safe_segment, ) -from .runs import StateRootError, _project_of_run_dir, events_dir_for, live_store_topology +from .runs import ( + StateRootError, + _project_of_run_dir, + bundle_close_note, + bundle_close_operation_id, + events_dir_for, + live_store_topology, +) from .signals import attribute_events, session_events from .statemachine import advance from .worksource import EpicProgress, Pick, WorkSource @@ -8940,7 +8947,16 @@ def _reopen_unlanded_close(self, task: StoryTask) -> None: A ``LedgerReadError`` or ``OSError`` from the reopen is a visible degrade: it journals ``sweep-bundle-reopen-failed`` and skips the publish, and the escalation or veto goes on. The escalation pause is the - operator stop; turning it into a crash would lose its reason.""" + operator stop; turning it into a crash would lose its reason. + + The other ends of such a run (a pause other than an escalation, a stop, + a crash) reopen nothing, and no reopen path is built for them (DW-571). + They are guarded instead, through one read-only discovery, + :func:`~bmad_loop.runs.held_sweep_closes`: a fresh sweep warns about a + prior run that still holds closes (``sweep-prior-run-holds-closes``, or + ``sweep-prior-run-closes-unknown`` when it cannot tell), and ``delete``, + the TUI delete and ``clean`` refuse to remove that run. ``resolve + --abandon`` already refuses every sweep run, which is never a replay.""" if not task.bundle_closes_intended: return if not (self._isolated or task.worktree_path): @@ -9190,12 +9206,16 @@ def _bundle_success_statuses(self, stage: Literal["dev", "review"]) -> frozenset return frozenset({"done"}) def _bundle_close_operation_id(self, task: StoryTask) -> str: - """Stable identity for a close and its possible defer-time undo.""" - return f"{self.state.run_id}/{task.story_key}" + """Stable identity for a close and its possible defer-time undo. The + format is :func:`~bmad_loop.runs.bundle_close_operation_id`, which the + held-close discovery (:func:`~bmad_loop.runs.held_sweep_closes`) shares.""" + return bundle_close_operation_id(self.state.run_id, task.story_key) def _bundle_close_note(self, task: StoryTask) -> str: - """Resolution note shared by a bundle close and its possible undo.""" - return f"resolved by sweep bundle {task.story_key}" + """Resolution note shared by a bundle close and its possible undo. The + format is :func:`~bmad_loop.runs.bundle_close_note`, which the held-close + discovery shares.""" + return bundle_close_note(task.story_key) def _close_declared_deferred( self, task: StoryTask, snapshot: list[_ArmedClose] | None = None diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index 745c0b51e..08b022ac8 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -2196,7 +2196,13 @@ def _delete_run_worker(self, run_id: str, run_dir: Path) -> None: guard_notes: list[str] = [] try: runs.delete_run(self.project, run_dir, warn=guard_notes.append) - except (OSError, runs.StateRootError, runs.LiveEngineError, runs.LiveSessionError) as e: + except ( + OSError, + runs.StateRootError, + runs.LiveEngineError, + runs.LiveSessionError, + runs.HeldSweepClosesError, + ) as e: # The modal's liveness sample is advisory. Surface authoritative # lifecycle refusals and lock/removal failures here rather than letting # them kill the worker thread or forgetting a run that still exists. diff --git a/tests/test_cleanup.py b/tests/test_cleanup.py index be08054f1..c6392f5b9 100644 --- a/tests/test_cleanup.py +++ b/tests/test_cleanup.py @@ -1154,3 +1154,162 @@ def test_cmd_clean_json_carries_the_sweep_count_in_both_modes(project, capsys): outcome = _clean_json(repo, capsys) assert outcome["state_dirs_swept"] == 1 assert not orphan.exists() + + +# ---------------------------------------- runs holding unlanded sweep closes (DW-571) + + +def _held_and_plain_stopped_runs(v7): + """Two stopped runs past any retention window. One is an isolated sweep run + still holding its shared-store close of DW-1, the other a plain run. Returns + (held engine, paths, plain run dir).""" + import test_sweep as ts + + engine, paths, *_ = ts._held_sweep_run(v7) + engine.state.paused_stage = None + engine.state.paused_story_key = None + engine.state.stopped = True + engine._save() + plain = paths.project / ".bmad-loop" / "runs" / "20260102-000000-bbbb" + save_state( + plain, RunState(run_id="p", project=str(paths.project), started_at="x", stopped=True) + ) + assert runs.reclaimable(engine.run_dir) and runs.reclaimable(plain) # premise + return engine, paths, plain + + +def test_cmd_clean_hard_skips_a_run_holding_unlanded_sweep_closes(project_v7_store_repo, capsys): + """`clean --hard` checks the delete arm's runs for held sweep closes before it + mutates anything. The holder is left whole (state, ledger, no trim) with a + stderr line naming DW-1 and `bmad-loop resume`, and its sibling is still + deleted. + + Ablation: delete the pre-check in `cmd_clean` and this fails. The sibling + still goes, but the holder is trimmed, and the refusal surfaces only from + `delete_run` as a mid-clean skip.""" + engine, paths, plain = _held_and_plain_stopped_runs(project_v7_store_repo) + heavy = engine.run_dir / VERIFY_DIR + heavy.mkdir() + (heavy / "verify.log").write_text("kept\n", encoding="utf-8") + assert heavy in runs.heavy_run_entries(engine.run_dir) # premise: a trim takes it + state_before = (engine.run_dir / "state.json").read_bytes() + ledger_before = paths.deferred_work.read_bytes() + + assert cli.cmd_clean(_clean_args(paths.project, retain=0, hard=True)) == 0 + + err = capsys.readouterr().err + assert not plain.exists() + assert engine.run_dir.is_dir() and heavy.is_dir() # untouched, not trimmed + assert (engine.run_dir / "state.json").read_bytes() == state_before + assert paths.deferred_work.read_bytes() == ledger_before + line = next(ln for ln in err.splitlines() if ln.startswith(f"run {engine.run_dir.name}:")) + assert "DW-1" in line and f"bmad-loop resume {engine.run_dir.name}" in line + assert line.endswith("— not removed") + + +def test_cmd_clean_names_the_fault_when_a_run_could_not_be_checked(project_v7_store_repo, capsys): + """A delete-arm run whose ledger cannot be read is skipped with a line naming + the fault, never with the held-ids wording and an empty id list. Its sibling + is still deleted. + + Ablation: make `_held_closes_reason` always return the held-ids text and this + fails (the line reads `holds sweep ledger closes whose code never landed ()`).""" + engine, paths, plain = _held_and_plain_stopped_runs(project_v7_store_repo) + paths.deferred_work.write_bytes(b"\xff\xfe not utf-8\n") + + assert cli.cmd_clean(_clean_args(paths.project, retain=0, hard=True)) == 0 + + err = capsys.readouterr().err + assert engine.run_dir.is_dir() and not plain.exists() + line = next(ln for ln in err.splitlines() if ln.startswith(f"run {engine.run_dir.name}:")) + assert "could not be checked" in line and str(paths.deferred_work) in line + assert "holds sweep ledger closes" not in line + + +def test_cmd_clean_json_reports_a_held_closes_run_as_protected(project_v7_store_repo, capsys): + """In `--json` mode the holder lands in the existing `protected` list and the + sibling in `deleted`. + + Green ablation: deleting the pre-check in `cmd_clean` leaves this green, + because with nothing to trim the removal's refusal handler also files the + holder as protected. `test_cmd_clean_hard_skips_a_run_holding_unlanded_sweep_closes` + pins the pre-check.""" + engine, paths, plain = _held_and_plain_stopped_runs(project_v7_store_repo) + + doc = _clean_json(paths.project, capsys, "--hard", "--retain", "0") + + assert doc["protected"] == [engine.run_dir.name] + assert doc["deleted"] == [plain.name] + assert engine.run_dir.is_dir() and not plain.exists() + + +def test_cmd_clean_reports_a_held_closes_refusal_from_delete_run( + project_v7_store_repo, monkeypatch, capsys +): + """A run that passed the pre-check can still be refused by `delete_run`, whose + under-lock check is authoritative. `clean` records it beside the other + mid-clean refusals and goes on, rather than aborting. + + Ablation: drop `runs.HeldSweepClosesError` from the removal's except tuple and + this fails (the refusal escapes `cmd_clean`).""" + engine, paths, plain = _held_and_plain_stopped_runs(project_v7_store_repo) + real = runs.held_sweep_closes + calls: list[Path] = [] + + def held(project, run_dir): + calls.append(run_dir) + if run_dir == engine.run_dir and calls.count(run_dir) == 1: + return [] # the pre-check sees nothing held + return real(project, run_dir) + + monkeypatch.setattr(runs, "held_sweep_closes", held) + + assert cli.cmd_clean(_clean_args(paths.project, retain=0, hard=True)) == 0 + + err = capsys.readouterr().err + assert calls.count(engine.run_dir) == 2 # premise: pre-check, then delete_run + assert engine.run_dir.is_dir() and not plain.exists() + assert f"run {engine.run_dir.name}: holds sweep ledger closes" in err + + +def test_cmd_clean_policy_delete_arm_skips_a_held_closes_run(project_v7_store_repo, capsys): + """Without `--hard`, `[cleanup] archive_old = false` also puts past-retention + runs on the delete arm, and the holder is skipped there the same way: left + whole and untrimmed, while its sibling is deleted. + + Ablation: narrow the pre-check in `cmd_clean` to `args.hard` and this fails + (the holder is trimmed).""" + engine, paths, plain = _held_and_plain_stopped_runs(project_v7_store_repo) + policy = paths.project / ".bmad-loop" / "policy.toml" + policy.write_text("[cleanup]\narchive_old = false\n", encoding="utf-8") + heavy = engine.run_dir / VERIFY_DIR + heavy.mkdir() + (heavy / "verify.log").write_text("kept\n", encoding="utf-8") + state_before = (engine.run_dir / "state.json").read_bytes() + ledger_before = paths.deferred_work.read_bytes() + + assert cli.cmd_clean(_clean_args(paths.project, retain=0)) == 0 + + err = capsys.readouterr().err + assert not plain.exists() + assert not (paths.project / ".bmad-loop" / "archive").exists() # premise: delete arm + assert engine.run_dir.is_dir() and heavy.is_dir() + assert (engine.run_dir / "state.json").read_bytes() == state_before + assert paths.deferred_work.read_bytes() == ledger_before + assert f"run {engine.run_dir.name}: holds sweep ledger closes" in err + + +def test_cmd_clean_dry_run_plans_a_held_closes_run_as_protected(project_v7_store_repo, capsys): + """`--dry-run --hard --json` predicts the skip: the holder is in `protected`, + not in `deleted`, and nothing is removed. + + Ablation: gate the pre-check in `cmd_clean` on `not dry` and this fails (the + plan lists the holder as deleted).""" + engine, paths, plain = _held_and_plain_stopped_runs(project_v7_store_repo) + + doc = _clean_json(paths.project, capsys, "--dry-run", "--hard", "--retain", "0") + + assert engine.run_dir.name in doc["protected"] + assert engine.run_dir.name not in doc["deleted"] + assert doc["deleted"] == [plain.name] + assert engine.run_dir.is_dir() and plain.is_dir() diff --git a/tests/test_cli.py b/tests/test_cli.py index 6f16c8cde..f94d4a9b9 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4878,6 +4878,86 @@ def test_delete_dead_run(tmp_path, capsys): assert not run_dir.exists() +@pytest.mark.parametrize("force", [False, True]) +def test_delete_refuses_a_run_holding_unlanded_sweep_closes(project_v7_store_repo, capsys, force): + """DW-571. `bmad-loop delete` on a paused isolated sweep run that still holds + its shared-store close of DW-1 exits 1. Stderr names the run, DW-1 and + `bmad-loop resume `. The run dir and the ledger are unchanged, and + `--force` does not waive the refusal. + + Without `--force` the refusal comes from `delete_run`'s under-lock check; + with it, from `cmd_delete`'s pre-check. + + Ablation: drop `runs.HeldSweepClosesError` from the except around + `delete_run` and the unforced case fails; drop the pre-check's except and the + forced case fails (either way the error escapes to `main`'s backstop, which + prints `error: ...` rather than the bare message).""" + import test_sweep as ts + + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + ledger_before = paths.deferred_work.read_bytes() + state_before = (engine.run_dir / "state.json").read_bytes() + argv = ["delete", "--project", str(paths.project), ts.HELD_RUN_ID] + + assert cli.main([*argv, "--force"] if force else argv) == 1 + + err = capsys.readouterr().err + assert err.startswith(f"run {ts.HELD_RUN_ID}: refusing to delete it") + assert "DW-1" in err and f"bmad-loop resume {ts.HELD_RUN_ID}" in err + assert (engine.run_dir / "state.json").read_bytes() == state_before + assert paths.deferred_work.read_bytes() == ledger_before + + +def test_delete_force_refuses_a_live_held_run_without_stopping_it( + project_v7_store_repo, monkeypatch, capsys +): + """`delete --force` on a live run that holds unlanded sweep closes is refused + before `--force` hard-stops the engine, so the run is not left stopped with + nothing deleted. + + Ablation: move the `refuse_held_sweep_closes` pre-check in `cmd_delete` after + `_stop_or_block_live_engine` and this fails (`stop_run` is called).""" + import test_sweep as ts + + from bmad_loop import runs + + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + monkeypatch.setattr(runs, "engine_liveness", lambda _rd: "alive") + monkeypatch.setattr( + runs, "stop_run", lambda _rd: pytest.fail("a held run was stopped before the refusal") + ) + + assert cli.main(["delete", "--project", str(paths.project), "--force", ts.HELD_RUN_ID]) == 1 + + err = capsys.readouterr().err + assert f"run {ts.HELD_RUN_ID}: refusing to delete it" in err and "DW-1" in err + assert engine.run_dir.is_dir() + + +def test_delete_without_force_gives_a_live_held_run_the_live_engine_refusal( + project_v7_store_repo, monkeypatch, capsys +): + """Without `--force`, a live run that holds sweep closes is refused as live. + Its close is still in flight, so the held-closes message and its `resume` + remedy would be wrong advice. + + Ablation: run the `refuse_held_sweep_closes` pre-check in `cmd_delete` + without `--force` too and this fails (the held-closes message wins).""" + import test_sweep as ts + + from bmad_loop import runs + + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + monkeypatch.setattr(runs, "engine_liveness", lambda _rd: "alive") + + assert cli.main(["delete", "--project", str(paths.project), ts.HELD_RUN_ID]) == 1 + + err = capsys.readouterr().err + assert f"run {ts.HELD_RUN_ID} is still live" in err + assert "refusing to delete it" not in err + assert engine.run_dir.is_dir() + + def test_delete_unknown_warns_but_proceeds(tmp_path, monkeypatch, capsys): from bmad_loop import runs diff --git a/tests/test_deferredwork.py b/tests/test_deferredwork.py index 3729378a3..f9f5d43a2 100644 --- a/tests/test_deferredwork.py +++ b/tests/test_deferredwork.py @@ -4592,6 +4592,40 @@ def test_mark_open_many_writes_nothing_when_no_id_is_eligible(tmp_path, monkeypa assert acquisitions == [] +def test_held_reopenable_closes_names_exactly_what_mark_open_many_would_reopen(tmp_path): + """`held_reopenable_closes` is the read-only twin of `mark_open_many`'s decision: + a close this operation wrote is held, while an entry that is still open, was + closed by another operation or by a human, or is missing is not. It writes + nothing, and `mark_open_many` then reopens exactly the ids it named. + + Ablation: return `dw_ids` unfiltered (skip the `_apply_open_many` fold) and + the DW-2 / DW-3 / DW-99 rows fail.""" + path = write_ledger(tmp_path) + close_reopenable(path, "DW-1", "by dw-a") + close_reopenable(path, "DW-3", "by dw-b", operation_id="other-run/dw-b") + text = path.read_text(encoding="utf-8") + ids = ["DW-1", "DW-2", "DW-3", "DW-99"] + + held = deferredwork.held_reopenable_closes(text, ids, "by dw-a", OPERATION_ID) + + assert held == ["DW-1"] + assert path.read_text(encoding="utf-8") == text # read-only + assert mark_open_many(path, ids, "by dw-a", OPERATION_ID) == held + reopened = path.read_text(encoding="utf-8") + assert deferredwork.held_reopenable_closes(reopened, ids, "by dw-a", OPERATION_ID) == [] + + +def test_held_reopenable_closes_requires_the_matching_note(tmp_path): + """The note is part of the decision, as it is for `mark_open_many`: the same + operation id under a different note holds nothing.""" + path = write_ledger(tmp_path) + close_reopenable(path, "DW-1", "by dw-a") + text = path.read_text(encoding="utf-8") + + assert deferredwork.held_reopenable_closes(text, ["DW-1"], "by dw-z", OPERATION_ID) == [] + assert deferredwork.held_reopenable_closes(text, ["DW-1"], "by dw-a", OPERATION_ID) == ["DW-1"] + + def test_record_decision_close_matches_the_serial_pair_bytes(tmp_path, monkeypatch): """One `record_decision(close_note=...)` writes exactly what the `append_decision` + `mark_done` pair writes, in one write rather than two. diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index 543c1571f..eab2be296 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -12833,16 +12833,18 @@ def test_delete_warns_when_a_kept_worktree_stays_registered(project, tmp_path, m assert str(unit) in note and "git worktree prune" in note -def test_delete_warns_when_the_run_state_is_unreadable(project, tmp_path, monkeypatch): - """An unreadable state.json leaves the code root unknown: `delete_run` still - prunes `project` (the usual root, so the unit is deregistered here), but a clean +def test_delete_warns_when_the_run_state_is_missing(project, tmp_path, monkeypatch): + """A missing state.json leaves the code root unknown: `delete_run` still prunes + `project` (the usual root, so the unit is deregistered here), but a clean listing there is not reported as a confirmed cleanup — the run might have - recorded a `repo_root` override. + recorded a `repo_root` override. (A present but unreadable state.json is + refused before any removal, since it might hold sweep ledger closes; + `test_delete_run_refuses_a_run_it_cannot_check_for_held_closes` pins that.) Ablation: make the state-is-None arm return `_prune_deleted_run_worktrees`'s result alone and `notes == []`, failing the unpack.""" _engine, replay_dir = _escalated_replay(project, tmp_path, monkeypatch) - (replay_dir / "state.json").write_text("{not json", encoding="utf-8") + (replay_dir / "state.json").unlink() notes: list[str] = [] runs.delete_run(project.project, replay_dir, warn=notes.append, force=True) @@ -12850,7 +12852,7 @@ def test_delete_warns_when_the_run_state_is_unreadable(project, tmp_path, monkey assert not replay_dir.exists() assert _registered_under(project, replay_dir) == [] [note] = notes - assert "run state was unreadable" in note and "git worktree prune" in note + assert "run state was missing or unreadable" in note and "git worktree prune" in note def test_delete_warns_when_the_worktree_listing_fails(project, tmp_path, monkeypatch): @@ -12917,6 +12919,46 @@ def test_abandon_refuses_a_run_that_is_not_a_replay(project, tmp_path, monkeypat assert Path(engine.state.tasks["1-1-a"].worktree_path).is_dir() +def test_abandon_refuses_an_isolated_sweep_run_holding_its_close( + project_v7_store_repo, monkeypatch, capsys +): + """DW-571. An isolated T3 sweep run paused at an escalation, with its close of + DW-1 still `done` in the shared ledger, cannot be abandoned: a sweep run is + never a replay, so `resolve --abandon` refuses it with "closes replay runs + only". The state, the journal and the ledger stay byte-identical, and DW-1 + stays `done`. + + Ablation: drop the `not state.replay_of` check in + `unitreplay.abandon_refusal` and the message assertion fails.""" + import test_sweep as ts + + engine, paths, _store, _rel, task = ts._held_sweep_run(project_v7_store_repo) + task.phase = Phase.ESCALATED + engine.state.paused_stage = PAUSE_ESCALATION + engine._save() + monkeypatch.setattr(runs, "engine_liveness", lambda _run_dir: "dead") + assert runs.held_sweep_closes(paths.project, engine.run_dir) == ["DW-1"] # premise + before = _state_bytes(engine.run_dir) + rows = len(Journal(engine.run_dir).entries()) + ledger_before = paths.deferred_work.read_bytes() + + assert _abandon_at(paths.project, engine.run_dir) == 1 + + assert "closes replay runs only" in capsys.readouterr().err + assert _state_bytes(engine.run_dir) == before + assert len(Journal(engine.run_dir).entries()) == rows + assert paths.deferred_work.read_bytes() == ledger_before + assert not ts.ledger_entries(paths)["DW-1"].open + + +def _abandon_at(project_root: Path, run_dir: Path) -> int: + from bmad_loop import cli + + return cli.main( + ["resolve", "--project", str(project_root), run_dir.name, "--abandon", "--no-interactive"] + ) + + def test_abandon_refuses_a_finished_run(project, tmp_path, monkeypatch, capsys): """A finished run is not paused at an escalation: the existing message, rc 1.""" engine, _marker = _finished_kept_unit(project, tmp_path) diff --git a/tests/test_portability_guard.py b/tests/test_portability_guard.py index bc48725a0..a13ef6e97 100644 --- a/tests/test_portability_guard.py +++ b/tests/test_portability_guard.py @@ -2024,6 +2024,14 @@ # already reaches `_loop`) and writes no `error`, since there is no fault # text to quote. "sweep-preanswer-prune-refused", + # DW-571. The sweep-start warning (`cli._warn_prior_sweep_closes`): an + # earlier run, not live and not this one, still holds isolated bundle + # closes in the shared store's ledger that no code landed for + # (`runs.held_sweep_closes`), or could not be checked. Read-only; the new + # sweep runs unchanged. `run_id` is benign, `dw_ids` a keylist, `error` + # (the fault) dropped by name. + "sweep-prior-run-closes-unknown", + "sweep-prior-run-holds-closes", "sweep-remaining-estimate-unreadable", "sweep-repeat-done", "sweep-resolved-closed", diff --git a/tests/test_runs.py b/tests/test_runs.py index 13e5d060b..b844f27a9 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -30,7 +30,7 @@ refuse_to_resolve, ) -from bmad_loop import envvars, platform_util, runs, verify +from bmad_loop import deferredwork, envvars, platform_util, runs, ticketstore, verify from bmad_loop.adapters import tmux_base from bmad_loop.adapters.multiplexer import MultiplexerError, TerminalMultiplexer from bmad_loop.adapters.psmux_backend import PsmuxMultiplexer @@ -11552,3 +11552,355 @@ def test_rebase_recorded_code_root_keeps_an_ancestor_root_that_still_exists(tmp_ repo.mkdir() state = RunState(run_id="r", project=str(repo / "app"), repo_root=str(repo), started_at="now") assert runs.rebase_recorded_code_root(state, repo / "sub" / "app") == repo + + +# ------------------------------------------- sweep closes a run still holds (DW-571) + + +def _seam(): + """The DW-569/571 seam helpers live with the sweep suite that owns them.""" + import test_sweep + + return test_sweep + + +@pytest.mark.parametrize( + ("fixture", "layout"), + [("project_v7_store_repo", "T3"), ("project_v7_external_store", "T4")], +) +def test_held_sweep_closes_names_an_unresumed_isolated_close(request, fixture, layout): + """DW-571, the original gap. An isolated T3/T4 sweep run paused after accepted + dev and never resumed leaves DW-1 `done` in the shared ledger. No later sweep + re-bundles it, and the discovery reports it.""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(request.getfixturevalue(fixture), layout) + + assert not ts.ledger_entries(paths)["DW-1"].open + assert "DW-1" not in deferredwork.open_ids(paths.deferred_work.read_text(encoding="utf-8")) + assert runs.held_sweep_closes(paths.project, engine.run_dir) == ["DW-1"] + + +def test_held_sweep_closes_counts_a_target_branch_as_isolation(project_v7_store_repo): + """`state.target_branch` alone marks the run isolated, as `worktree_path` does + for one task. + + Ablation: key isolation on `task.worktree_path` alone and this fails.""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo, worktree=False) + engine.state.target_branch = "main" + engine._save() + + assert runs.held_sweep_closes(paths.project, engine.run_dir) == ["DW-1"] + + +def _rewrite_state(engine, **changes) -> None: + for key, value in changes.items(): + setattr(engine.state, key, value) + engine._save() + + +@pytest.mark.parametrize( + "shape", ["landed", "not-isolated", "finished", "story-run", "no-close-intended"] +) +def test_held_sweep_closes_ignores_a_run_that_holds_nothing(project_v7_store_repo, shape): + """Each row breaks one condition of "holds": the task landed (DONE), the run is + not isolated, the run finished, the run is not a sweep, or no close was ever + intended. The close stays `done` in every row, so only the condition answers + `[]`. + + Ablation: delete the `task.phase != Phase.DONE` predicate and "landed" fails; + delete the isolation predicate and "not-isolated" fails; delete the + `state.finished` check and "finished" fails; delete the `run_type` check and + "story-run" fails.""" + ts = _seam() + seam = { + "landed": {"phase": Phase.DONE}, + "not-isolated": {"worktree": False}, + "no-close-intended": {"intended": False}, + }.get(shape, {}) + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo, **seam) + if shape == "finished": + _rewrite_state(engine, finished=True) + elif shape == "story-run": + _rewrite_state(engine, run_type="story") + assert not ts.ledger_entries(paths)["DW-1"].open # premise: the close stands + + assert runs.held_sweep_closes(paths.project, engine.run_dir) == [] + + +def test_held_sweep_closes_ignores_a_close_already_reopened(project_v7_store_repo): + """Once the close is undone (the escalation reopen ran), nothing is held. + + Ablation: return the task's `bundle_closes_intended` instead of + `held_reopenable_closes` and this fails.""" + ts = _seam() + engine, paths, _store, _rel, task = ts._held_sweep_run(project_v7_store_repo) + reopened = deferredwork.mark_open_many( + paths.deferred_work, + task.dw_ids, + engine._bundle_close_note(task), + engine._bundle_close_operation_id(task), + ) + assert reopened == ["DW-1"] # premise + + assert runs.held_sweep_closes(paths.project, engine.run_dir) == [] + + +@pytest.mark.parametrize("switched", [False, True]) +def test_held_sweep_closes_ignores_a_ledger_in_the_checkout(project_v7, switched): + """T1: the store is tracked in the code checkout, so an isolated bundle's + close lives in the unit worktree's copy and dies with it. A close in this + checkout's ledger under the run's identity is not held, and that stays true + after the active initiative is switched: with no shared store there is + nothing a switch could hide. + + Ablation: delete the `in_shared_store` check and the unswitched case fails; + delete the `shared_store is None` early return and the switched case fails + (the initiative conflict reads as "unknown").""" + from conftest import V7_USER_CONFIG_REL + + ts = _seam() + paths = ts._v7_paths(project_v7) + ts.write_ledger(paths, {"DW-1": "open"}) + topology = ticketstore.assess_store(paths, "none") + assert isinstance(topology, ticketstore.StoreTopology) and topology.layout == "T1" + engine, _ = ts.make_sweep(paths, [], store_topology=topology.to_dict(), run_id=ts.HELD_RUN_ID) + assert not engine.paths.in_shared_store(engine.paths.deferred_work) # premise + task = StoryTask( + story_key="dw-fix-things", + epic=0, + dw_ids=["DW-1"], + phase=Phase.DEV_VERIFY, + worktree_path=str(paths.repo_root.parent / "unit"), + bundle_closes_intended=["DW-1"], + ) + engine.state.tasks[task.story_key] = task + deferredwork.mark_done_many_reopenable( + paths.deferred_work, + task.dw_ids, + engine._today(), + engine._bundle_close_note(task), + engine._bundle_close_operation_id(task), + ) + engine.state.initiative = paths.active_initiative # the pin `compose_sweep` records + engine._save() + assert not ts.ledger_entries(paths)["DW-1"].open # premise + if switched: + (paths.project / V7_USER_CONFIG_REL).write_text( + '[core]\nactive_initiative = "another-initiative"\n', encoding="utf-8" + ) + + assert runs.held_sweep_closes(paths.project, engine.run_dir) == [] + + +def test_held_sweep_closes_reads_absence_as_nothing_held(project_v7_store_repo): + """An absent `state.json` has no record of a close, and an absent ledger holds + none: both answer `[]`, never "unknown".""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + paths.deferred_work.unlink() + assert runs.held_sweep_closes(paths.project, engine.run_dir) == [] + + (engine.run_dir / "state.json").unlink() + assert runs.held_sweep_closes(paths.project, engine.run_dir) == [] + + +@pytest.mark.parametrize("fault", ["state", "ledger", "topology"]) +def test_held_sweep_closes_never_folds_a_fault_into_nothing_held(project_v7_store_repo, fault): + """A present but unreadable `state.json`, a ledger that does not decode, and a + recorded store topology that does not parse each raise + `HeldClosesUnknownError`. None of them is "none held". + + Ablation: answer `[]` from any one of the three arms (the state `except + Exception`, the `observe_ledger` fault check, the config/topology `except`) + and its row fails.""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + if fault == "state": + (engine.run_dir / "state.json").write_text("{not json", encoding="utf-8") + elif fault == "ledger": + paths.deferred_work.write_bytes(b"\xff\xfe not utf-8\n") + else: + _rewrite_state(engine, store_topology={"layout": "T3"}) + + with pytest.raises(runs.HeldClosesUnknownError): + runs.held_sweep_closes(paths.project, engine.run_dir) + + +def test_prior_sweep_closes_skips_the_new_run_and_live_runs(project_v7_store_repo, monkeypatch): + """The sweep-start scan reports a held run, but not the run named by + `exclude` (the new sweep), and not a live run whose close is still in + flight. + + Ablation: drop the `engine_liveness` skip and the live row fails; drop the + `exclude` skip and the excluded row fails.""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + project = paths.project + + assert runs.prior_sweep_closes(project, exclude="other") == [ + runs.PriorSweepCloses(ts.HELD_RUN_ID, ("DW-1",)) + ] + assert runs.prior_sweep_closes(project, exclude=ts.HELD_RUN_ID) == [] + monkeypatch.setattr( + runs, "engine_liveness", lambda rd: "alive" if rd == engine.run_dir else "dead" + ) + assert runs.prior_sweep_closes(project, exclude="other") == [] + + +def test_prior_sweep_closes_reports_an_unlistable_runs_dir_as_a_fault(tmp_path, monkeypatch): + """A runs dir that cannot be listed gives one fault row with no run id, never + an empty answer.""" + monkeypatch.setattr(runs, "_run_dir_names", lambda _p: (None, "runs: EACCES")) + + assert runs.prior_sweep_closes(tmp_path, exclude="x") == [ + runs.PriorSweepCloses(None, fault="runs: EACCES") + ] + + +@pytest.mark.parametrize("force", [False, True]) +def test_delete_run_refuses_a_run_holding_unlanded_sweep_closes(project_v7_store_repo, force): + """`delete_run` refuses a run that holds a shared-store close no code landed + for: the error names the run, DW-1 and `bmad-loop resume`. The run dir, its + state and the ledger are byte-identical afterwards. `force` waives only the + session guard, so it does not waive this. + + Ablation: delete the `refuse_held_sweep_closes` call in `delete_run` and both + rows fail (the run is removed).""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + state_before = (engine.run_dir / "state.json").read_bytes() + ledger_before = paths.deferred_work.read_bytes() + + with pytest.raises(runs.HeldSweepClosesError) as info: + runs.delete_run(paths.project, engine.run_dir, force=force) + + assert info.value.run_id == ts.HELD_RUN_ID and info.value.dw_ids == ["DW-1"] + assert info.value.reason is None + message = str(info.value) + assert ts.HELD_RUN_ID in message and "DW-1" in message + assert f"bmad-loop resume {ts.HELD_RUN_ID}" in message + assert (engine.run_dir / "state.json").read_bytes() == state_before + assert paths.deferred_work.read_bytes() == ledger_before + + +def test_delete_run_refuses_a_run_it_cannot_check_for_held_closes(project_v7_store_repo): + """A present but unreadable `state.json` might hold closes, so `delete_run` + refuses it and names the fault. The garbage state is left in place.""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + (engine.run_dir / "state.json").write_text("{not json", encoding="utf-8") + + with pytest.raises(runs.HeldSweepClosesError) as info: + runs.delete_run(paths.project, engine.run_dir) + + assert info.value.dw_ids == [] and "could not be read" in (info.value.reason or "") + assert f"bmad-loop resume {ts.HELD_RUN_ID}" in str(info.value) + assert (engine.run_dir / "state.json").read_text(encoding="utf-8") == "{not json" + + +def test_delete_run_checks_held_closes_under_the_lock_after_the_liveness_guard( + project_v7_store_repo, monkeypatch +): + """The held-closes check runs under the run's state lock, and only after the + engine-liveness guard: a live run still gets `LiveEngineError`.""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + real = runs.held_sweep_closes + checked: list[Path] = [] + + def held(project, run_dir): + assert_run_state_lock_held(run_dir) + checked.append(run_dir) + return real(project, run_dir) + + monkeypatch.setattr(runs, "held_sweep_closes", held) + with pytest.raises(runs.HeldSweepClosesError): + runs.delete_run(paths.project, engine.run_dir) + assert checked == [engine.run_dir] + + monkeypatch.setattr(runs, "engine_liveness", lambda _rd: "alive") + with pytest.raises(runs.LiveEngineError): + runs.delete_run(paths.project, engine.run_dir) + assert checked == [engine.run_dir] # the liveness guard refused first + assert engine.run_dir.is_dir() + + +def test_delete_run_removes_a_sweep_run_that_holds_nothing(project_v7_store_repo): + """A sweep run whose bundle landed (DONE) holds no close, so it is deleted as + before.""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo, phase=Phase.DONE) + + runs.delete_run(paths.project, engine.run_dir, warn=lambda _note: None) + + assert not engine.run_dir.exists() + + +def test_delete_run_failed_launch_unwind_skips_the_held_closes_check( + project_v7_store_repo, monkeypatch +): + """The runsetup failed-launch unwind (`_expected_composer_claim`) removes a run + it just composed, which holds nothing, so the check is skipped. + + Ablation: drop the `_expected_composer_claim is None` condition and this fails + with `HeldSweepClosesError`.""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + claim = engine.run_dir.stat(follow_symlinks=False) + + runs.delete_run( + paths.project, + engine.run_dir, + warn=lambda _note: None, + _expected_composer_pid=os.getpid(), + _expected_composer_claim=claim, + ) + + assert not engine.run_dir.exists() + + +def test_held_sweep_closes_refuses_to_judge_under_a_switched_initiative(project_v7_store_repo): + """The live paths resolve the ACTIVE initiative's ledger. A sweep pinned to + another initiative would be judged against a ledger that holds nothing under + its op id, so a switched initiative is "unknown", never "none held": delete + refuses (naming the conflict), and the sweep-start scan reports a fault row. + + Ablation: delete the `sweep_initiative_conflict` check in `held_sweep_closes` + and this fails (delete removes the holder; the scan reports nothing).""" + from conftest import V7_USER_CONFIG_REL + + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + assert runs.held_sweep_closes(paths.project, engine.run_dir) == ["DW-1"] # premise + (paths.project / V7_USER_CONFIG_REL).write_text( + '[core]\nactive_initiative = "another-initiative"\n', encoding="utf-8" + ) + + [row] = runs.prior_sweep_closes(paths.project, exclude="other") + assert row.run_id == ts.HELD_RUN_ID and row.fault and "active initiative" in row.fault + with pytest.raises(runs.HeldSweepClosesError) as info: + runs.delete_run(paths.project, engine.run_dir) + assert info.value.reason and "active initiative" in info.value.reason + assert engine.run_dir.is_dir() + + +def test_prior_sweep_closes_records_a_liveness_probe_fault_as_a_fault_row(tmp_path, monkeypatch): + """A misconfigured process host makes `engine_liveness` raise + `ProcessHostError`. The scan records a fault row for that run rather than + letting the error abort the new sweep. + + Ablation: drop the `except ProcessHostError` around the liveness probe and + this fails with the raise.""" + from bmad_loop.process_host import ProcessHostError + + (tmp_path / ".bmad-loop" / "runs" / "20260101-000000-aaaa").mkdir(parents=True) + + def broken(_run_dir): + raise ProcessHostError("no such process host 'bogus'") + + monkeypatch.setattr(runs, "engine_liveness", broken) + + [row] = runs.prior_sweep_closes(tmp_path, exclude="x") + assert row.run_id == "20260101-000000-aaaa" and row.dw_ids == () + assert row.fault and "bogus" in row.fault diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 0c91dc49f..b6811ee62 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -38752,6 +38752,7 @@ def _v7_isolated_close_seam( worktree: bool = True, intended: bool = True, commit_close: bool = False, + run_id: str = "sweep-run", ): """DW-569 seam setup: a v7 sweep over a store with its own repository (``layout`` T3/T4) whose ledger, `{DW-1: open}`, is committed in that store, @@ -38761,8 +38762,8 @@ def _v7_isolated_close_seam( isolated arm); ``intended`` records ``bundle_closes_intended``, which only the accepted-dev close assigns. ``commit_close`` commits the close into the store, so the store's HEAD holds DW-1 `done` and a reopen leaves a real - delta. Returns the engine, its paths, the store, the ledger's - store-relative path and the task.""" + delta. ``run_id`` names the sweep run. Returns the engine, its paths, the + store, the ledger's store-relative path and the task.""" store = v7.output_folder paths = _v7_paths(v7) write_ledger(paths, {"DW-1": "open"}, commit=False) @@ -38771,7 +38772,7 @@ def _v7_isolated_close_seam( ledger_rel = paths.deferred_work.resolve().relative_to(store.resolve()).as_posix() topology = ticketstore.assess_store(paths, "none") assert isinstance(topology, ticketstore.StoreTopology) and topology.layout == layout - engine, _ = make_sweep(paths, [], store_topology=topology.to_dict()) + engine, _ = make_sweep(paths, [], store_topology=topology.to_dict(), run_id=run_id) # premise: the ledger lives in the shared store every checkout reads assert engine.paths.in_shared_store(engine.paths.deferred_work) assert not engine._isolated # premise: only `worktree_path` takes the isolated arm @@ -39161,6 +39162,160 @@ def _v7_store_carry_seam(v7, layout, *, distinct_baseline=False): return engine, paths, store, ledger_rel, task +HELD_RUN_ID = "20260101-000000-a571" + + +def _held_sweep_run(v7, layout: str = "T3", *, run_id: str = HELD_RUN_ID, **seam): + """DW-571: the DW-569 seam saved to disk as an unfinished run. An isolated + sweep paused at the story gate after accepted dev, with its close of DW-1 + still `done` in the shared store's ledger and no code landed. ``seam`` goes + to :func:`_v7_isolated_close_seam`. Returns what the seam returns.""" + engine, paths, store, ledger_rel, task = _v7_isolated_close_seam( + v7, layout, run_id=run_id, **seam + ) + engine.state.paused_stage = PAUSE_STORY_GATE + engine.state.paused_story_key = task.story_key + engine.state.initiative = paths.active_initiative # the pin `compose_sweep` records + engine._save() + on_disk = load_state(engine.run_dir) + assert on_disk.run_type == "sweep" and not on_disk.finished # premise + return engine, paths, store, ledger_rel, task + + +def _start_sweep_at_seam(project_root: Path, monkeypatch, run_id: str): + """Drive `cli._start_sweep` with `runsetup.compose_sweep` replaced by a fresh + run whose engine records the stderr written before it ran. Returns the rc, + the new run's journal, and the stderr seen at `engine.run()` (one item when + the engine ran).""" + from bmad_loop import cli + from bmad_loop import policy as policy_mod + from bmad_loop import runsetup + from bmad_loop.plugins.grants import GrantSet + + run_dir = project_root / ".bmad-loop" / "runs" / run_id + journal = Journal(run_dir) + seen: list[str] = [] + summary = SimpleNamespace(crashed=False, render=lambda: "SWEEP DONE") + + def run(): + seen.append(getattr(sys.stderr, "getvalue", str)()) + return summary + + composed = SimpleNamespace(run_id=run_id, engine=SimpleNamespace(run=run), journal=journal) + monkeypatch.setattr(runsetup, "compose_sweep", lambda **_kwargs: composed) + rc = cli._start_sweep( + project_root, + load_paths(project_root), + policy_mod.load(None), + prompting=False, + decisions_only=False, + max_bundles=None, + trigger="cli", + plugin_grants=GrantSet.empty(), + ) + return rc, journal, seen + + +def _journal_rows(journal: Journal, kind: str) -> list[dict]: + return [e for e in journal.entries() if e.get("kind") == kind] + + +@_V7_STORE_LAYOUTS +def test_sweep_start_warns_about_a_prior_run_holding_unlanded_closes( + request, fixture, layout, monkeypatch, capsys +): + """DW-571. T3/T4: a paused isolated sweep run still holds its close of DW-1 + in the shared ledger. A fresh sweep journals `sweep-prior-run-holds-closes` + (the held run and its ids) and prints one warning naming `bmad-loop resume`, + before its engine runs. Nothing is reopened, the prior run's state and the + ledger are byte-identical, and the new sweep still runs. + + Ablation: delete the `_warn_prior_sweep_closes` call in `cli._start_sweep` + and this fails (no row, no warning line).""" + engine, paths, _store, _ledger_rel, _task = _held_sweep_run( + request.getfixturevalue(fixture), layout + ) + state_before = (engine.run_dir / "state.json").read_bytes() + ledger_before = paths.deferred_work.read_bytes() + capsys.readouterr() + + rc, journal, _seen = _start_sweep_at_seam(paths.project, monkeypatch, "20990101-000000-b571") + + out, err = capsys.readouterr() + assert rc == 0 and "SWEEP DONE" in out # the new sweep ran + rows = _journal_rows(journal, "sweep-prior-run-holds-closes") + assert [(r["run_id"], r["dw_ids"]) for r in rows] == [(HELD_RUN_ID, ["DW-1"])] + lines = [ln for ln in err.splitlines() if ln.startswith("warning: run")] + assert len(lines) == 1 + assert HELD_RUN_ID in lines[0] and "DW-1" in lines[0] + assert f"bmad-loop resume {HELD_RUN_ID}" in lines[0] + assert not ledger_entries(paths)["DW-1"].open # not reopened + assert paths.deferred_work.read_bytes() == ledger_before + assert (engine.run_dir / "state.json").read_bytes() == state_before + + +def test_sweep_start_warning_precedes_the_engine_run(project_v7_store_repo, monkeypatch): + """The warning is written before `engine.run()`, so an operator sees it at + launch rather than after the sweep. Run under a plain `StringIO` stderr so the + order is visible.""" + _held_sweep_run(project_v7_store_repo) + err = io.StringIO() + monkeypatch.setattr(sys, "stderr", err) + + rc, _journal, seen = _start_sweep_at_seam( + project_v7_store_repo.project, monkeypatch, "20990101-000000-b572" + ) + + assert rc == 0 and len(seen) == 1 + assert f"warning: run {HELD_RUN_ID} holds sweep ledger closes" in seen[0] + + +def test_sweep_start_journals_a_prior_run_it_cannot_check( + project_v7_store_repo, monkeypatch, capsys +): + """A prior run whose `state.json` is present but unreadable might hold closes, + so it is never reported as holding none. The fresh sweep journals + `sweep-prior-run-closes-unknown` naming the run, prints a warning, and runs. + The garbage state is left as it was. + + Ablation: answer `[]` instead of raising from `held_sweep_closes`' unreadable + state arm and this fails (no row, no line).""" + engine, paths, *_ = _held_sweep_run(project_v7_store_repo) + (engine.run_dir / "state.json").write_text("{not json", encoding="utf-8") + capsys.readouterr() + + rc, journal, _seen = _start_sweep_at_seam(paths.project, monkeypatch, "20990101-000000-b573") + + out, err = capsys.readouterr() + assert rc == 0 and "SWEEP DONE" in out + rows = _journal_rows(journal, "sweep-prior-run-closes-unknown") + assert [r["run_id"] for r in rows] == [HELD_RUN_ID] + assert "could not be read" in rows[0]["error"] + assert _journal_rows(journal, "sweep-prior-run-holds-closes") == [] + assert f"warning: could not check run {HELD_RUN_ID}" in err + assert (engine.run_dir / "state.json").read_text(encoding="utf-8") == "{not json" + + +def test_sweep_start_journals_an_unlistable_runs_dir(project_v7_store_repo, monkeypatch, capsys): + """A runs dir that cannot be listed is not "no prior run holds closes": the + fresh sweep journals `sweep-prior-run-closes-unknown` with the fault and no + run id, prints a warning, and runs. + + Ablation: answer `[]` from `prior_sweep_closes` when the listing fails and + this fails.""" + paths = _v7_paths(project_v7_store_repo) + monkeypatch.setattr(runs, "_run_dir_names", lambda _p: (None, "runs: cannot list (EACCES)")) + capsys.readouterr() + + rc, journal, _seen = _start_sweep_at_seam(paths.project, monkeypatch, "20990101-000000-b574") + + out, err = capsys.readouterr() + assert rc == 0 and "SWEEP DONE" in out + rows = _journal_rows(journal, "sweep-prior-run-closes-unknown") + assert [(r.get("run_id"), r["error"]) for r in rows] == [(None, "runs: cannot list (EACCES)")] + assert "warning: could not check earlier runs" in err + + @pytest.mark.parametrize( ("fixture", "layout"), [("project_v7_store_repo", "T3"), ("project_v7_external_store", "T4")], diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index 2c16c9e23..7af9e7b95 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -7219,6 +7219,12 @@ async def confirm_lifecycle(pilot, key: str, helper: str, outcomes: list[str]) - runs_mod.StateRootError("no usable state root"), "delete failed", ), + ( + "D", + "delete_run", + runs_mod.HeldSweepClosesError("20260611-100000-aaaa", ["DW-1"]), + "delete failed", + ), ("A", "archive_run", runs_mod.LiveEngineError("engine resumed"), "archive failed"), ( "A", @@ -7237,7 +7243,9 @@ def test_lifecycle_workers_report_authoritative_failures_and_keep_the_run_visibl success. The Pilot twin below keeps one row per action in the running app. Ablation: omit either new exception type from the worker catch and its rows - raise out of the worker body instead of posting the toast. Verified.""" + raise out of the worker body instead of posting the toast. Verified. The same + holds for `HeldSweepClosesError` (a sweep run holding unlanded ledger closes, + DW-571) in the delete worker's catch.""" def fail(*_args, **_kwargs): raise failure From 087d6370104650d0a700f3836fe61fb1f76813d5 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 18 Oct 2026 19:34:40 -0700 Subject: [PATCH 06/10] docs(changelog): tighten the DW-556..571 sweep entries --- CHANGELOG.md | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac403c7f1..2494fd204 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -144,13 +144,9 @@ breaking changes may land in a minor release. fsynced before the replace. A run with no recorded run-dir identity, or one whose device number changed, records one `journal-unpinned` row per writer; `resume` records or re-binds the identity before its first row (DW-559). -- Guard an isolated v7 sweep run that paused, stopped or crashed while holding shared-store - ledger closes for code that never landed: a new sweep warns and journals - `sweep-prior-run-holds-closes` (`sweep-prior-run-closes-unknown` when it cannot tell), - and `delete`, the TUI delete and `clean` refuse to remove that run, naming - `bmad-loop resume `. They also refuse any run, of any type, whose state cannot be - read, and such a sweep run whose config or ledger cannot be read or whose pinned - initiative is no longer active; `delete --force` waives neither refusal (DW-571). +- Refuse `delete` (even `--force`), the TUI delete and `clean` on a run whose state + cannot be read, or on an isolated v7 sweep run still holding shared-store ledger + closes for unlanded code; a new sweep warns while such a run exists (DW-571). ### Removed @@ -161,23 +157,14 @@ breaking changes may land in a minor release. - Name the T1 ticket plan an interrupted commit window left `done` or `blocked` when the resumed commit fails, instead of a bare `commit failed:` (DW-555). -- Commit a v7 sweep bundle's ledger close in the ticket store's own repository (T3/T4) once - its code lands, best effort with `Bmad-Code-*` trailers and withheld while the ledger is in - doubt, instead of leaving the store's ledger dirty (DW-556). -- Commit a deferred v7 sweep bundle's ledger writes (its reopened close and harvested - deferrals) in the ticket store's own repository (T3/T4), best effort and withheld while - the ledger is in doubt; an isolated defer over a shared store reopens the bundle's close - first (DW-567). -- Commit an isolated unit's harvested deferrals in the ticket store's own repository (T3/T4) - when no store publisher follows, best effort, instead of journaling - `harvest-carry-uncommitted` for a store path the code repository cannot commit (DW-570). -- Reopen a worktree-isolated sweep bundle's close in a shared ticket store's ledger when the - bundle escalates (merge-back failures included) or a plugin vetoes it, instead of leaving - the entries `done` for code that never landed; a reopen fault journals - `sweep-bundle-reopen-failed` and the escalation still pauses (DW-569). -- Latch a worktree-isolated sweep bundle's artifact publication complete when its artifacts - dir is in a shared ticket store, instead of pausing on - `artifact publication intent is missing` after the merge (DW-557). +- Commit a v7 sweep's own ledger writes (bundle closes, reopened closes, harvested + deferrals) in a T3/T4 ticket store's repository, best effort, instead of leaving + them uncommitted (DW-556, DW-567, DW-570). +- Reopen an isolated sweep bundle's close in a shared ticket store's ledger when the + bundle escalates, defers or is vetoed, instead of leaving entries `done` for code + that never landed (DW-567, DW-569). +- Stop pausing an isolated sweep on `artifact publication intent is missing` when its + artifacts dir is in a shared ticket store (DW-557). - Resolve a relative v6 config path against the project root instead of the process cwd (DW-551). - Re-validate an ordinary unit's merge against the target on replay instead of trusting its From a117c90d8ee2be05f644a8ba16db371c49da67a8 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 19 Oct 2026 01:19:52 -0700 Subject: [PATCH 07/10] fix(sweep): crash rather than degrade a defer or skip veto's failed close reopen A defer or skip veto lets the run finish, and a finished run is out of held_sweep_closes' reach, so swallowing a reopen fault there left a shared-store close done for code that never landed with no later guard. The fault now propagates before DEFERRED is recorded, as _defer's does; escalations and a pause veto keep the journaled degrade. --- docs/sweep-guide.md | 3 ++- src/bmad_loop/sweep.py | 30 +++++++++++++++++++++--------- tests/test_sweep.py | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 10 deletions(-) diff --git a/docs/sweep-guide.md b/docs/sweep-guide.md index b49d514f6..8d4706246 100644 --- a/docs/sweep-guide.md +++ b/docs/sweep-guide.md @@ -90,7 +90,8 @@ A sweep runs on a v7 project the same way, with these differences: - **Store layout.** The store-topology check judges a v7 sweep as a tickets run, because its ledger lives in the ticket store. The sweep records the store's topology at its start, and a resume refuses a store that changed since, or whose initiative folder is gone. - **Pinned initiative.** A sweep records the active initiative it started on. A resume, `resolve` or the dashboard's re-arm refuses while `[core] active_initiative` names another: set it back, or start a new sweep. - **Decisions per initiative.** Each initiative has its own ledger and DW numbering, so pre-answers are kept per initiative in `.bmad-loop/decisions-.json`. `bmad-loop decisions` and the TUI answer the active initiative's decisions and offer only those its own sweeps raised. An answer given after another initiative was made active than the one its decision was listed from is refused; list the decisions again. With no initiative active, the project store stays `.bmad-loop/decisions.json` and only sweeps that ran with no initiative are offered. -- **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. The bundle's ledger close lands in that shared ledger the same way, so a bundle that ends without landing its code reopens its close before that end is recorded: when it defers, when it escalates (a failed merge-back included), and when a plugin vetoes it. A ledger fault during that reopen is journaled as `sweep-bundle-reopen-failed` and the escalation or veto goes on, leaving the close for a human to reopen. Resume skips an escalated bundle, so after you merge a failed merge-back's kept branch by hand, its entries stay open until a later sweep triages them as already resolved. +- **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. The bundle's ledger close lands in that shared ledger the same way, so a bundle that ends without landing its code reopens its close before that end is recorded: when it defers, when it escalates (a failed merge-back included), and when a plugin vetoes it. Resume skips an escalated bundle, so after you merge a failed merge-back's kept branch by hand, its entries stay open until a later sweep triages them as already resolved. +- **A reopen that faults.** A ledger fault during that reopen is journaled as `sweep-bundle-reopen-failed`. An escalation or a `pause` veto still pauses, leaving the close for a human to reopen. A `defer` or `skip` veto, which would let the run finish with the close standing, crashes the run instead, so it stays unfinished and resumable. - **A run that still holds closes.** A run that pauses for any other reason, is stopped or crashes after a bundle's accepted dev keeps that bundle's close: the entries read `done` in the shared ledger, and no later sweep re-bundles a `done` entry. Only that run can land or reopen them, so resume it (`bmad-loop resume `). Until then, each new sweep prints a warning naming the run and its entries, and journals `sweep-prior-run-holds-closes`; it still runs and leaves those entries alone. A run it cannot check (unreadable state, config or ledger) is journaled as `sweep-prior-run-closes-unknown`. `bmad-loop delete`, the dashboard's delete and `bmad-loop clean`'s delete arm refuse to remove such a run, and `--force` does not override that. - **Plans in a store repository.** When the ticket store is its own git repository (nested or outside the checkout), the bundle's code commit cannot carry its plan. Once the code has landed, the accepted plan gets its own commit there, `chore(sweep): plan [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code. This is best effort, like a tickets run's store commit: a failure is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and the plan stays uncommitted for a human. The bundle's ledger close is committed there the same best-effort way, as `chore(sweep): close resolved deferred-work entries` with the same trailers and the same journaled failures, and is withheld (`sweep-ledger-commit-withheld`) while the run holds its ledger in doubt. - **Deferred bundles in a store repository.** A bundle that defers never lands code, but its ledger writes (the reopen of its close, any harvested deferrals) are committed in the store repository as `chore(sweep): record a deferred bundle's ledger writes`, the same best-effort and withheld way, with only the `Bmad-Code-Repo` trailer. A landed bundle's harvested deferrals ride its close commit; one that landed with no close commits them as `chore(deferred-work): carry harvested findings from `, as does a resume that replays a deferred bundle's harvest. These are best effort too. diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 928912d4e..371debef9 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -8915,13 +8915,18 @@ def _vetoed(self, ctx: HookContext | None, task: StoryTask) -> bool: """The base veto routing, after :meth:`_reopen_unlanded_close` when a plugin really vetoed (DW-569). Every veto action (``pause``, ``defer``, ``skip``) ends the unit without landing its code, so the reopen runs - ahead of the base's terminal save. A stage nobody vetoed is not an end - and reaches the base untouched.""" - if ctx is not None and ctx.resolved_veto() is not None: - self._reopen_unlanded_close(task) + ahead of the base's terminal save. Only ``pause`` may degrade a reopen + fault: ``defer`` and ``skip`` let the run go on to finish, and a finished + run is out of :func:`~bmad_loop.runs.held_sweep_closes`' reach, so their + fault propagates before any DEFERRED is recorded, as :meth:`_defer`'s + does. A stage nobody vetoed is not an end and reaches the base + untouched.""" + veto = ctx.resolved_veto() if ctx is not None else None + if veto is not None: + self._reopen_unlanded_close(task, may_degrade=veto.action == "pause") return super()._vetoed(ctx, task) - def _reopen_unlanded_close(self, task: StoryTask) -> None: + def _reopen_unlanded_close(self, task: StoryTask, *, may_degrade: bool = True) -> None: """Reopen an isolated bundle's shared-store close when the bundle ends without landing (DW-569): before the terminal save of every escalation (:meth:`_escalate`, ``WorktreeFlow.escalate_unit`` for an in-flight unit, @@ -8944,10 +8949,15 @@ def _reopen_unlanded_close(self, task: StoryTask) -> None: ``bundle_closes_intended`` is kept, so a re-armed bundle that lands re-closes through :meth:`_carry_isolated_ledger_writes`. - A ``LedgerReadError`` or ``OSError`` from the reopen is a visible - degrade: it journals ``sweep-bundle-reopen-failed`` and skips the - publish, and the escalation or veto goes on. The escalation pause is the - operator stop; turning it into a crash would lose its reason. + A ``LedgerReadError`` or ``OSError`` from the reopen journals + ``sweep-bundle-reopen-failed`` and skips the publish. With + ``may_degrade`` (every escalation, a ``pause`` veto) that is a visible + degrade and the end goes on: the pause is the operator stop, turning it + into a crash would lose its reason, and the paused run stays guarded by + :func:`~bmad_loop.runs.held_sweep_closes`. Without it (a ``defer`` or + ``skip`` veto, which do not stop the run) the fault then propagates, so + the task is not yet terminal and the crashed run stays unfinished and + guarded. The other ends of such a run (a pause other than an escalation, a stop, a crash) reopen nothing, and no reopen path is built for them (DW-571). @@ -8973,6 +8983,8 @@ def _reopen_unlanded_close(self, task: StoryTask) -> None: dw_ids=list(task.bundle_closes_intended), error=str(e), ) + if not may_degrade: + raise return self._publish_deferred_ledger(task) diff --git a/tests/test_sweep.py b/tests/test_sweep.py index b6811ee62..a7bcc1785 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -39108,6 +39108,48 @@ def mark_open_many(*args, **kwargs): assert _records(engine, "store-committed") == [] +@pytest.mark.parametrize("action", ["defer", "skip", "pause"]) +def test_v7_isolated_veto_reopen_fault_degrades_only_under_pause( + project_v7_store_repo, monkeypatch, action +): + """DW-569. T3: a plugin veto's reopen of the shared close faults. `pause` + stops the run, which stays guarded by `held_sweep_closes`, so the fault is + the journaled degrade the escalation takes and the veto still pauses. + `defer` and `skip` would let the run finish, out of that guard's reach, with + a `done` for code that never landed, so the fault propagates before the base + records DEFERRED: the task keeps its in-flight phase, with no veto row. + Every row journals `sweep-bundle-reopen-failed` and publishes nothing. + + ABLATION (performed): drop the `if not may_degrade: raise` arm in + `SweepEngine._reopen_unlanded_close` and the `defer`/`skip` rows fail (the + veto returns True and the task is DEFERRED).""" + engine, paths, store, _ledger_rel, task = _v7_isolated_close_seam(project_v7_store_repo, "T3") + head = git(store, "rev-parse", "HEAD").strip() + phase = task.phase + + def mark_open_many(*args, **kwargs): + raise OSError("injected reopen write fault") + + monkeypatch.setattr(deferredwork, "mark_open_many", mark_open_many) + ctx = HookContext("pre_review_phase", run_id=engine.state.run_id, story_key=task.story_key) + ctx.veto(action, "policy says no") + + if action == "pause": + with pytest.raises(RunPaused): + engine._vetoed(ctx, task) + assert task.phase == Phase.ESCALATED + assert [r["action"] for r in _records(engine, "plugin-veto")] == ["pause"] + else: + with pytest.raises(OSError, match="injected reopen"): + engine._vetoed(ctx, task) + assert task.phase == phase + assert _records(engine, "plugin-veto") == [] + failed = _records(engine, "sweep-bundle-reopen-failed") + assert [(r["story_key"], r["dw_ids"]) for r in failed] == [(task.story_key, ["DW-1"])] + assert not ledger_entries(paths)["DW-1"].open + assert git(store, "rev-parse", "HEAD").strip() == head # no publish + + def _store_carry_commits(store: Path, story_key: str) -> list[str]: """The shas of the store's harvested-carry commits for ``story_key``, newest first (DW-570).""" From c3469c4e410f7ae96849d98356b41afd56212380 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 19 Oct 2026 01:37:35 -0700 Subject: [PATCH 08/10] fix(sweep): hold an unmerged DONE bundle's close and journal a lost store ledger held_sweep_closes treated every DONE task as landed, but an isolated unit saves its terminal phase before integrate_unit merges it back. A host lost in that window left the close `done` in the shared ledger with no code on the target, and delete was allowed. Landing now needs the merge-back's isolated_ledger_carried latch under isolation (DONE or AWAITING_OPERATOR). The store-repo ledger publishers returned silently when the ledger was gone, not a regular file, unreadable or unlocatable. They now journal store-commit-failed naming the ledger and the message. --- docs/sweep-guide.md | 2 +- src/bmad_loop/runs.py | 18 +++++++++++-- src/bmad_loop/sweep.py | 61 +++++++++++++++++++++++++++++++----------- tests/test_runs.py | 50 +++++++++++++++++++++++++++------- tests/test_sweep.py | 37 +++++++++++++++++++++++++ 5 files changed, 139 insertions(+), 29 deletions(-) diff --git a/docs/sweep-guide.md b/docs/sweep-guide.md index 8d4706246..95eeff498 100644 --- a/docs/sweep-guide.md +++ b/docs/sweep-guide.md @@ -93,7 +93,7 @@ A sweep runs on a v7 project the same way, with these differences: - **Worktree isolation over a shared store.** When the ticket store is shared rather than carried by each checkout (unversioned and ignored, or its own repository), the session in the unit worktree writes its plan straight into the main checkout's store, so nothing is copied back at the merge. The bundle's ledger close lands in that shared ledger the same way, so a bundle that ends without landing its code reopens its close before that end is recorded: when it defers, when it escalates (a failed merge-back included), and when a plugin vetoes it. Resume skips an escalated bundle, so after you merge a failed merge-back's kept branch by hand, its entries stay open until a later sweep triages them as already resolved. - **A reopen that faults.** A ledger fault during that reopen is journaled as `sweep-bundle-reopen-failed`. An escalation or a `pause` veto still pauses, leaving the close for a human to reopen. A `defer` or `skip` veto, which would let the run finish with the close standing, crashes the run instead, so it stays unfinished and resumable. - **A run that still holds closes.** A run that pauses for any other reason, is stopped or crashes after a bundle's accepted dev keeps that bundle's close: the entries read `done` in the shared ledger, and no later sweep re-bundles a `done` entry. Only that run can land or reopen them, so resume it (`bmad-loop resume `). Until then, each new sweep prints a warning naming the run and its entries, and journals `sweep-prior-run-holds-closes`; it still runs and leaves those entries alone. A run it cannot check (unreadable state, config or ledger) is journaled as `sweep-prior-run-closes-unknown`. `bmad-loop delete`, the dashboard's delete and `bmad-loop clean`'s delete arm refuse to remove such a run, and `--force` does not override that. -- **Plans in a store repository.** When the ticket store is its own git repository (nested or outside the checkout), the bundle's code commit cannot carry its plan. Once the code has landed, the accepted plan gets its own commit there, `chore(sweep): plan [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code. This is best effort, like a tickets run's store commit: a failure is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and the plan stays uncommitted for a human. The bundle's ledger close is committed there the same best-effort way, as `chore(sweep): close resolved deferred-work entries` with the same trailers and the same journaled failures, and is withheld (`sweep-ledger-commit-withheld`) while the run holds its ledger in doubt. +- **Plans in a store repository.** When the ticket store is its own git repository (nested or outside the checkout), the bundle's code commit cannot carry its plan. Once the code has landed, the accepted plan gets its own commit there, `chore(sweep): plan [bmad-loop ]`, with `Bmad-Code-*` trailers naming the code. This is best effort, like a tickets run's store commit: a failure is journaled (`store-commit-failed` or `store-commit-foreign-dirt`) and the plan stays uncommitted for a human. The bundle's ledger close is committed there the same best-effort way, as `chore(sweep): close resolved deferred-work entries` with the same trailers and the same journaled failures (a ledger gone or unreadable there is journaled `store-commit-failed` too), and is withheld (`sweep-ledger-commit-withheld`) while the run holds its ledger in doubt. - **Deferred bundles in a store repository.** A bundle that defers never lands code, but its ledger writes (the reopen of its close, any harvested deferrals) are committed in the store repository as `chore(sweep): record a deferred bundle's ledger writes`, the same best-effort and withheld way, with only the `Bmad-Code-Repo` trailer. A landed bundle's harvested deferrals ride its close commit; one that landed with no close commits them as `chore(deferred-work): carry harvested findings from `, as does a resume that replays a deferred bundle's harvest. These are best effort too. ## Story-declared closure (`closes_deferred`) diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 142fc57bb..d9c2c403f 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -3652,6 +3652,19 @@ def __init__(self, run_id: str, dw_ids: list[str], reason: str | None = None) -> super().__init__(f"run {run_id}: refusing to delete it — {what}. {remedy}") +def _bundle_code_landed(task: StoryTask) -> bool: + """Whether ``task``'s code is on the target. In place it lands with the + ``DONE`` (or ``AWAITING_OPERATOR``) commit. Under isolation the terminal phase + is saved BEFORE ``WorktreeFlow.integrate_unit`` merges the unit back, so the + phase alone does not prove a landing: only ``isolated_ledger_carried``, which + that merge-back (or ``Engine._replay_unlatched_ledger_carries``) latches once + the merge is done, does. A host lost between the two leaves the close held + until a resume replays the merge.""" + if task.phase not in (Phase.DONE, Phase.AWAITING_OPERATOR): + return False + return not task.worktree_path or task.isolated_ledger_carried + + def held_sweep_closes(project: Path, run_dir: Path) -> list[str]: """The deferred-work ids whose ledger close the sweep run at ``run_dir`` still holds with no code landed, in task order. An isolated v7 bundle over a shared @@ -3664,7 +3677,8 @@ def held_sweep_closes(project: Path, run_dir: Path) -> list[str]: - it is an unfinished, isolated sweep (``state.target_branch`` or the task's ``worktree_path``); - - it has a task that is not ``DONE`` with ``bundle_closes_intended`` set; + - it has a task whose code has not landed (:func:`_bundle_code_landed`) with + ``bundle_closes_intended`` set; - the ledger lies in the shared store (``bmadconfig.load_paths`` with the run's recorded topology, :func:`live_store_topology`); - the ledger still shows that task's close as undoable under its note and @@ -3688,7 +3702,7 @@ def held_sweep_closes(project: Path, run_dir: Path) -> list[str]: candidates = [ task for task in state.tasks.values() - if task.phase != Phase.DONE + if not _bundle_code_landed(task) and task.bundle_closes_intended and (state.target_branch or task.worktree_path) ] diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 371debef9..df2310a81 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -8823,19 +8823,16 @@ def _publish_bundle_close(self, task: StoryTask) -> None: ``integrate_unit`` after the merge, so a failed commit journals ``store-commit-failed`` (a staged foreign version of the ledger ``store-commit-foreign-dirt``) and the close stays uncommitted for a human - or a later publisher. A ledger already at the store's HEAD journals - nothing.""" + or a later publisher. So does a ledger gone or unreadable in the store + repository (:meth:`_store_ledger_to_publish`). A ledger already at the + store's HEAD journals nothing.""" topology = self.store_topology if topology is None or topology.store_repo is None or not task.bundle_closes_intended: return ledger = self.workspace.paths.deferred_work - try: - inside = ledger.resolve().is_relative_to(topology.store_repo.resolve()) - except (OSError, RuntimeError, ValueError): - inside = False - if not inside or not ledger.is_file(): - return message = "chore(sweep): close resolved deferred-work entries" + if not self._store_ledger_to_publish(task, topology.store_repo, ledger, message): + return if self._ledger_unfit_to_publish(): self._withhold_ledger_publish(message, dw_ids=list(task.bundle_closes_intended)) return @@ -8893,24 +8890,56 @@ def _publish_deferred_ledger(self, task: StoryTask) -> None: effort, like :meth:`_publish_bundle_close`: a failed commit journals ``store-commit-failed`` (a staged foreign version of the ledger ``store-commit-foreign-dirt``) and the writes stay uncommitted for a human - or a later publisher; a ledger already at the store's HEAD journals - nothing.""" + or a later publisher, as does a ledger gone or unreadable in the store + repository (:meth:`_store_ledger_to_publish`); a ledger already at the + store's HEAD journals nothing.""" topology = self.store_topology if topology is None or topology.store_repo is None: return ledger = self.paths.deferred_work - try: - inside = ledger.resolve().is_relative_to(topology.store_repo.resolve()) - except (OSError, RuntimeError, ValueError): - inside = False - if not inside or not ledger.is_file(): - return message = "chore(sweep): record a deferred bundle's ledger writes" + if not self._store_ledger_to_publish(task, topology.store_repo, ledger, message): + return if self._ledger_unfit_to_publish(): self._withhold_ledger_publish(message, dw_ids=list(task.dw_ids)) return self._commit_store_ledger(task, topology.store_repo, ledger, message, code_commit=None) + def _store_ledger_to_publish( + self, task: StoryTask, store_repo: Path, ledger: Path, message: str + ) -> bool: + """Whether a ledger publisher (:meth:`_publish_bundle_close`, + :meth:`_publish_deferred_ledger`) commits ``ledger`` in ``store_repo``. A + ledger outside the store repository answers False with nothing journaled: + the code commit carries it. A ledger that cannot be located (a + ``resolve()`` fault), or that is gone, not a regular file or unreadable + inside the store repository, journals ``store-commit-failed`` naming + ``message`` and answers False. The write it should hold goes unpublished, + and that must not read as a publisher with nothing to do.""" + try: + inside = ledger.resolve().is_relative_to(store_repo.resolve()) + except (OSError, RuntimeError, ValueError) as e: + fault: str | None = ( + f"the deferred-work ledger {ledger} could not be located " + f"({type(e).__name__}: {e})" + ) + else: + if not inside: + return False + text, read_fault = deferredwork.observe_ledger(ledger) + if read_fault is not None: + fault = f"the deferred-work ledger {ledger} could not be read ({read_fault})" + elif text is None: + fault = f"the deferred-work ledger {ledger} is gone from the store repository" + else: + return True + self.journal.append( + "store-commit-failed", + story_key=task.story_key, + error=f"{fault}; {message!r} was not committed", + ) + return False + def _vetoed(self, ctx: HookContext | None, task: StoryTask) -> bool: """The base veto routing, after :meth:`_reopen_unlanded_close` when a plugin really vetoed (DW-569). Every veto action (``pause``, ``defer``, diff --git a/tests/test_runs.py b/tests/test_runs.py index b844f27a9..a41707f59 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -11600,25 +11600,37 @@ def _rewrite_state(engine, **changes) -> None: @pytest.mark.parametrize( - "shape", ["landed", "not-isolated", "finished", "story-run", "no-close-intended"] + "shape", + [ + "landed", + "landed-parked", + "not-isolated", + "finished", + "story-run", + "no-close-intended", + ], ) def test_held_sweep_closes_ignores_a_run_that_holds_nothing(project_v7_store_repo, shape): - """Each row breaks one condition of "holds": the task landed (DONE), the run is - not isolated, the run finished, the run is not a sweep, or no close was ever - intended. The close stays `done` in every row, so only the condition answers - `[]`. + """Each row breaks one condition of "holds": the task landed (DONE or + AWAITING_OPERATOR, merged back), the run is not isolated, the run finished, + the run is not a sweep, or no close was ever intended. The close stays `done` + in every row, so only the condition answers `[]`. - Ablation: delete the `task.phase != Phase.DONE` predicate and "landed" fails; + Ablation: make `_bundle_code_landed` answer False and both "landed" rows fail; delete the isolation predicate and "not-isolated" fails; delete the `state.finished` check and "finished" fails; delete the `run_type` check and "story-run" fails.""" ts = _seam() seam = { "landed": {"phase": Phase.DONE}, + "landed-parked": {"phase": Phase.AWAITING_OPERATOR}, "not-isolated": {"worktree": False}, "no-close-intended": {"intended": False}, }.get(shape, {}) - engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo, **seam) + engine, paths, *_, task = ts._held_sweep_run(project_v7_store_repo, **seam) + if shape.startswith("landed"): + task.isolated_ledger_carried = True # the merge-back's latch + engine._save() if shape == "finished": _rewrite_state(engine, finished=True) elif shape == "story-run": @@ -11628,6 +11640,22 @@ def test_held_sweep_closes_ignores_a_run_that_holds_nothing(project_v7_store_rep assert runs.held_sweep_closes(paths.project, engine.run_dir) == [] +@pytest.mark.parametrize("phase", [Phase.DONE, Phase.AWAITING_OPERATOR]) +def test_held_sweep_closes_names_a_terminal_unit_not_yet_merged_back(project_v7_store_repo, phase): + """An isolated unit saves its terminal phase BEFORE `integrate_unit` merges it + back. A host lost between the two leaves a DONE (or AWAITING_OPERATOR) task + with `isolated_ledger_carried` False: its code is not on the target, and only + a resume replays the merge, so the run still holds its close. + + Ablation: key landing on the phase alone (drop the `isolated_ledger_carried` + clause of `_bundle_code_landed`) and this fails.""" + ts = _seam() + engine, paths, *_, task = ts._held_sweep_run(project_v7_store_repo, phase=phase) + assert not task.isolated_ledger_carried # premise: the merge-back never latched + + assert runs.held_sweep_closes(paths.project, engine.run_dir) == ["DW-1"] + + def test_held_sweep_closes_ignores_a_close_already_reopened(project_v7_store_repo): """Once the close is undone (the escalation reopen ran), nothing is held. @@ -11827,10 +11855,12 @@ def held(project, run_dir): def test_delete_run_removes_a_sweep_run_that_holds_nothing(project_v7_store_repo): - """A sweep run whose bundle landed (DONE) holds no close, so it is deleted as - before.""" + """A sweep run whose bundle landed (DONE, merged back) holds no close, so it + is deleted as before.""" ts = _seam() - engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo, phase=Phase.DONE) + engine, paths, *_, task = ts._held_sweep_run(project_v7_store_repo, phase=Phase.DONE) + task.isolated_ledger_carried = True # the merge-back's latch + engine._save() runs.delete_run(paths.project, engine.run_dir, warn=lambda _note: None) diff --git a/tests/test_sweep.py b/tests/test_sweep.py index a7bcc1785..0164e7695 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -39558,6 +39558,43 @@ def test_v7_bundle_store_trailers_name_the_landed_squash_commit(project_v7_store assert f"Bmad-Code-Range: {task.baseline_commit}..{landed}" in body +@pytest.mark.parametrize("publisher", ["close", "deferred"]) +@pytest.mark.parametrize("ledger_state", ["gone", "directory", "undecodable"]) +def test_v7_store_ledger_publisher_journals_a_ledger_it_cannot_publish( + project_v7_store_repo, publisher, ledger_state +): + """T3: the bundle's close went into the store repository's ledger, then the + ledger was deleted, replaced by a directory or left unreadable before the + publisher ran. The publisher commits nothing and journals + `store-commit-failed` naming the ledger and its message, rather than + returning as if it had nothing to publish. + + ABLATION (performed): answer False with nothing journaled for every ledger + `_store_ledger_to_publish` cannot read (the old `not ledger.is_file()` + early return) and every row fails (no row).""" + engine, paths, store, ledger_rel, task = _v7_isolated_close_seam(project_v7_store_repo, "T3") + head = git(store, "rev-parse", "HEAD").strip() + ledger = paths.deferred_work + ledger.unlink() + if ledger_state == "directory": + ledger.mkdir() + elif ledger_state == "undecodable": + ledger.write_bytes(b"\xff\xfe not utf-8") + + if publisher == "close": + engine._publish_bundle_close(task) + message = _V7_LEDGER_CLOSE + else: + engine._publish_deferred_ledger(task) + message = _V7_LEDGER_DEFERRED + + rows = _records(engine, "store-commit-failed") + assert [r["story_key"] for r in rows] == [task.story_key] + assert str(ledger) in rows[0]["error"] and message in rows[0]["error"] + assert git(store, "rev-parse", "HEAD").strip() == head # nothing committed + assert _records(engine, "store-committed") == [] + + def test_v7_bundle_plan_in_a_checkout_store_rides_the_code_commit(project_v7): """T1 (the store is tracked inside the code checkout): the code commit already carries the plan, so the sweep makes no store-repository commit and From 7b4451c4fcb5791394c6c89628d59aff03771e09 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 19 Oct 2026 01:46:50 -0700 Subject: [PATCH 09/10] fix(sweep): judge a v7 sweep on a mixed install under v7 paths held_sweep_closes and resume reloaded a sweep's paths auto-detected, which resolves a mixed v6/v7 install as v6: a sweep spawned by a tickets run there pinned the v7 initiative, so its own pin read as a switched initiative. Both now resolve under runs.resolves_as_tickets (a sweep's initiative pin or store topology, which compose_sweep records only on v7). --- src/bmad_loop/cli.py | 2 +- src/bmad_loop/runs.py | 20 +++++++++++++++++--- tests/test_cli.py | 33 +++++++++++++++++++++++++++++++++ tests/test_runs.py | 27 ++++++++++++++++++++++++++- 4 files changed, 77 insertions(+), 5 deletions(-) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index adc16c17b..af1404764 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -5023,7 +5023,7 @@ def _prepare_resume_locked(project: Path, run_dir: Path, *, accept_baseline: boo if (refusal := _unresumable_source(state)) is not None: print(f"cannot resume {run_dir.name}: {refusal}", file=sys.stderr) return 1 - paths = bmadconfig.load_paths(project, want_tickets=state.source == worksource.SOURCE_TICKETS) + paths = bmadconfig.load_paths(project, want_tickets=runs.resolves_as_tickets(state)) sweep_options = None if state.run_type == "sweep": try: diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index d9c2c403f..dcca48447 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -3679,8 +3679,9 @@ def held_sweep_closes(project: Path, run_dir: Path) -> list[str]: ``worktree_path``); - it has a task whose code has not landed (:func:`_bundle_code_landed`) with ``bundle_closes_intended`` set; - - the ledger lies in the shared store (``bmadconfig.load_paths`` with the - run's recorded topology, :func:`live_store_topology`); + - the ledger lies in the shared store (``bmadconfig.load_paths`` in the + run's era, :func:`resolves_as_tickets`, with its recorded topology, + :func:`live_store_topology`); - the ledger still shows that task's close as undoable under its note and operation id (``deferredwork.held_reopenable_closes``), which is exactly what ``deferredwork.mark_open_many`` would reopen. @@ -3710,7 +3711,8 @@ def held_sweep_closes(project: Path, run_dir: Path) -> list[str]: return [] try: paths = ticketstore.with_shared_store( - bmadconfig.load_paths(project), live_store_topology(state, project) + bmadconfig.load_paths(project, want_tickets=resolves_as_tickets(state)), + live_store_topology(state, project), ) except Exception as e: # a config or topology fault is "unknown", never "none" raise HeldClosesUnknownError( @@ -4741,6 +4743,18 @@ def judged_store_source(state: RunState, sweep_source: str) -> str: return sweep_source if state.run_type == "sweep" else state.source +def resolves_as_tickets(state: RunState) -> bool: + """Whether a recorded run reloads the project's BMAD config as v7 + (``bmadconfig.load_paths(want_tickets=...)``): a tickets run, or a sweep that + pinned an initiative or recorded a store topology, which + ``runsetup.compose_sweep`` writes only on v7. A mixed install (v6 keys left + beside the v7 marker) auto-detects as v6, so a v7 sweep reloaded without this + reads its own pin as a switched initiative (``sweep_initiative_conflict``).""" + if state.run_type == "sweep": + return bool(state.initiative or state.store_topology) + return state.source == worksource.SOURCE_TICKETS + + def live_store_topology(state: RunState, project_root: Path) -> dict[str, str | None]: """`state.store_topology` located on the live project: its `store_root` and `store_repo` moved with a renamed project (`rebase_recorded_project_path`), so a diff --git a/tests/test_cli.py b/tests/test_cli.py index f94d4a9b9..0e0be2c0a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -9438,6 +9438,39 @@ def test_sweep_resume_refuses_an_initiative_change_before_any_write(project, mon assert runs.read_trusted_config_digest(project.project, run_dir.name)[0] == "OLDPIN" +@pytest.mark.parametrize( + ("pins", "want_tickets"), + [ + ({"initiative": "initiative-demo"}, True), + ({"store_topology": {"layout": "T3"}}, True), + ({}, False), + ], +) +def test_sweep_resume_reloads_a_v7_sweep_as_tickets(project, monkeypatch, pins, want_tickets): + """A sweep persists no `source`, so resume reads its era off what + `compose_sweep` records only on v7 (an initiative pin, a store topology). On a + mixed install the auto-detected v6 paths would read the sweep's own pin as a + switched initiative and refuse the resume. + + Ablation: key `want_tickets` on `state.source` again and the two v7 rows fail.""" + from bmad_loop import bmadconfig + + run_dir = _paused_run_for_resume(project, monkeypatch, run_type="sweep", **pins) + seen: list[bool] = [] + + class Stop(Exception): + pass + + def load_paths(_project, *, want_tickets=False): + seen.append(want_tickets) + raise Stop + + monkeypatch.setattr(bmadconfig, "load_paths", load_paths) + with pytest.raises(Stop): + cli._resume_paused_run(project.project, run_dir) + assert seen == [want_tickets] + + def test_story_resume_is_not_gated_on_the_sweep_skill(project, monkeypatch): """A story run never dispatches `/bmad-loop-sweep`, so its resume ignores the skill. Ablation: drop the `run_type` guard on the resume gate and this reddens.""" diff --git a/tests/test_runs.py b/tests/test_runs.py index a41707f59..026cd304f 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -30,7 +30,7 @@ refuse_to_resolve, ) -from bmad_loop import deferredwork, envvars, platform_util, runs, ticketstore, verify +from bmad_loop import bmadconfig, deferredwork, envvars, platform_util, runs, ticketstore, verify from bmad_loop.adapters import tmux_base from bmad_loop.adapters.multiplexer import MultiplexerError, TerminalMultiplexer from bmad_loop.adapters.psmux_backend import PsmuxMultiplexer @@ -11915,6 +11915,31 @@ def test_held_sweep_closes_refuses_to_judge_under_a_switched_initiative(project_ assert engine.run_dir.is_dir() +def test_held_sweep_closes_judges_a_v7_sweep_on_a_mixed_install_as_v7(project_v7_store_repo): + """A tickets run on a mixed install (v6 keys left beside the v7 marker) spawns + its sweep on v7 paths, so the sweep pins the v7 initiative and records a store + topology. An auto-detecting `load_paths` resolves that install as v6, where no + initiative is active: the sweep would read as switched, and its held close as a + fault. The recorded v7 run is judged under v7 paths instead. + + Ablation: drop `want_tickets` from `held_sweep_closes`'s `load_paths` call and + this fails with "active initiative" raised.""" + ts = _seam() + engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) + assert engine.state.initiative and engine.state.store_topology # premise: a v7 sweep + custom = paths.project / "_bmad" / "custom" / "config.toml" + custom.parent.mkdir(parents=True, exist_ok=True) + with custom.open("a", encoding="utf-8") as f: + f.write( + "\n[modules.bmm]\n" + 'planning_artifacts = "{project-root}/_bmad-output/planning-artifacts"\n' + 'implementation_artifacts = "{project-root}/_bmad-output/implementation-artifacts"\n' + ) + assert bmadconfig.load_paths(paths.project).era == "v6" # premise: mixed install + + assert runs.held_sweep_closes(paths.project, engine.run_dir) == ["DW-1"] + + def test_prior_sweep_closes_records_a_liveness_probe_fault_as_a_fault_row(tmp_path, monkeypatch): """A misconfigured process host makes `engine_liveness` raise `ProcessHostError`. The scan records a fault row for that run rather than From c3d3843c1a121ced43f82f98c7e58a897f9f9b00 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 19 Oct 2026 02:04:55 -0700 Subject: [PATCH 10/10] test(cli): pin an empty mux in the held-sweep-closes delete refusal test The unforced delete probes for a live agent session before the held-closes check. With no tmux server running, the real listing printed a warning to stderr ahead of the refusal, which broke the startswith assertion. --- tests/test_cli.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 0e0be2c0a..cb57af8bb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4879,7 +4879,9 @@ def test_delete_dead_run(tmp_path, capsys): @pytest.mark.parametrize("force", [False, True]) -def test_delete_refuses_a_run_holding_unlanded_sweep_closes(project_v7_store_repo, capsys, force): +def test_delete_refuses_a_run_holding_unlanded_sweep_closes( + project_v7_store_repo, monkeypatch, capsys, force +): """DW-571. `bmad-loop delete` on a paused isolated sweep run that still holds its shared-store close of DW-1 exits 1. Stderr names the run, DW-1 and `bmad-loop resume `. The run dir and the ledger are unchanged, and @@ -4893,7 +4895,13 @@ def test_delete_refuses_a_run_holding_unlanded_sweep_closes(project_v7_store_rep forced case fails (either way the error escapes to `main`'s backstop, which prints `error: ...` rather than the bare message).""" import test_sweep as ts + from test_runs import _LivenessMux + + from bmad_loop import runs + # The unforced path probes for a live agent session first; with no tmux server + # up, the real listing warns on stderr ahead of the refusal. + monkeypatch.setattr(runs, "get_multiplexer", lambda: _LivenessMux([])) engine, paths, *_ = ts._held_sweep_run(project_v7_store_repo) ledger_before = paths.deferred_work.read_bytes() state_before = (engine.run_dir / "state.json").read_bytes()