From 404237ab3f2da41a868a5e3dc63e8b2d8648e936 Mon Sep 17 00:00:00 2001 From: pbean Date: Fri, 24 Jul 2026 23:59:41 -0700 Subject: [PATCH 1/4] feat(engine,deferredwork,stories): stories close their declared deferred-work entries (#234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A story declares the DW- entries its work closes — `closes_deferred:` on its stories.yaml entry (stories mode), in its spec frontmatter, or both, the two unioned — and at the commit boundary the orchestrator flips each declared open entry to `status: done ` + `resolution: resolved by story `, the same annotation a sweep bundle writes. The ledger stops being one-way: filed automatically, resolved only by hand. Advisory by contract: the write sits behind every verify gate, checkpoint and review cycle, immediately before finalize_commit's `git add -A`, so an in-repo ledger rides the story's own commit (worktree isolation included) and a story that fails, is rejected by review, or escalates closes nothing. The declaration is re-read live at that boundary, so a late edit counts in both directions; an already-done id is a silent no-op across a resume; unknown, duplicate and malformed ids and an unavailable or undecodable ledger location are journaled warnings, never failures. A commit that fails after the write restores the ledger from a whole-document pre-close snapshot. An out-of-repo ledger (shared across worktrees, part of no commit) is written at the same moment and journaled `deferred-close-external-ledger`. The ledger write path gains `platform_util.atomic_write_text` (resolve first, unique temp sibling, mode/xattr copy, fsync-before-replace) so a close cannot tear the ledger or swap a symlink for a regular file. --- src/bmad_loop/deferredwork.py | 152 ++++++++- src/bmad_loop/engine.py | 272 +++++++++++++++- src/bmad_loop/platform_util.py | 85 ++++- src/bmad_loop/stories.py | 48 ++- src/bmad_loop/stories_engine.py | 42 ++- src/bmad_loop/sweep.py | 15 +- tests/conftest.py | 44 ++- tests/test_deferredwork.py | 167 ++++++++++ tests/test_engine.py | 542 +++++++++++++++++++++++++++++++- tests/test_engine_worktree.py | 37 ++- tests/test_platform_util.py | 137 ++++++++ tests/test_stories.py | 49 +++ tests/test_stories_engine.py | 246 ++++++++++++++- 13 files changed, 1793 insertions(+), 43 deletions(-) diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index 6c90aaf3..9c104fab 100644 --- a/src/bmad_loop/deferredwork.py +++ b/src/bmad_loop/deferredwork.py @@ -13,9 +13,12 @@ import hashlib import re +from collections.abc import Sequence from dataclasses import dataclass from pathlib import Path +from .platform_util import atomic_write_text + HEADING_RE = re.compile(r"^### (DW-\d+): (.+?)\s*$", re.MULTILINE) ANY_HEADING_RE = re.compile(r"^#{1,6} ", re.MULTILINE) STATUS_RE = re.compile(r"^status:[ \t]*(.*)$", re.MULTILINE) @@ -64,6 +67,102 @@ def open_ids(text: str) -> set[str]: return {e.id for e in parse_ledger(text) if e.open} +def parse_declaration(raw: object) -> tuple[tuple[str, ...], str | None]: + """The single reading of a ``closes_deferred:`` declaration (#234), shared by + the ``stories.yaml`` parser, the engine's close hook, and ``validate``. + + Returns the normalized ids plus an error describing a wrong *container*. + Missing / YAML-null is an empty declaration, not an error. + + Strict about the container, lenient about each item. A bare + ``closes_deferred: DW-1`` is a schema error rather than a silently-wrapped + single id — a string is iterable, so a lenient reading would quietly turn one + id into a list of characters — while items are ``str()``-normalized and + stripped, because an LLM-authored manifest may emit an unquoted ``DW-1`` as a + string but a bare ``5`` as an int. Blanks drop and duplicates collapse + (order-preserving): both are noise, not a contradiction. + + Callers decide the severity: the manifest parser raises, the engine journals, + ``validate`` warns. What they must NOT do is disagree — before this, a wrong + container was a hard schema error in ``stories.yaml`` and a silent empty + declaration in frontmatter, so the same mistake either failed the parse or + vanished depending on which file it was made in. + + Whether an id names a real entry is not decided here; that needs the ledger + (:func:`classify`). + """ + if raw is None: + return (), None + if not isinstance(raw, list): + return (), f"must be a list of deferred-work ids (got {type(raw).__name__})" + return tuple(dict.fromkeys(item for item in (str(x).strip() for x in raw) if item)), None + + +@dataclass(frozen=True) +class Declared: + """How declared ids line up against one ledger snapshot (#234). + + Four outcomes, not two, because "not open" hides two very different cases. + ``already_done`` is a satisfied declaration — a resume re-driving a close that + already landed — and must stay silent. ``malformed`` is an entry that exists + but carries neither an ``open`` nor a ``done`` status: nothing can be marked, + and saying nothing would leave the operator believing it was. + + ``duplicates`` cross-cuts the other four: it names the declared ids the ledger + carries more than once, whichever bucket they landed in. A duplicate id is a + corrupt ledger (#286), and the entry this classification describes is only one + of them — so the close is reported, never silent. + """ + + open_ids: tuple[str, ...] = () + already_done: tuple[str, ...] = () + unknown: tuple[str, ...] = () + malformed: tuple[str, ...] = () + duplicates: tuple[str, ...] = () + + +def classify(text: str, ids: Sequence[str]) -> Declared: + """Partition `ids` against a single ledger snapshot, preserving order. + + Classifying from a snapshot rather than from :func:`mark_done`'s return value + is deliberate: that return conflates "already done" with "absent from the + ledger", and those need opposite treatment (silence vs. a warning). + + **The FIRST entry of a duplicated id wins**, because that is the one + :func:`_find_entry` — and so every mutation in this module — acts on. Indexing + last-wins instead made the two disagree, and a ledger carrying one `DW-1` open + and another done then closed nothing while saying nothing, in either order: a + done-first ledger classified the id `open`, sent it to + :func:`mark_done_many`, and had :func:`_apply_done` refuse the done copy it + found first (marked nothing, so not even an unmatched warning); an open-first + ledger classified it `already_done` and never attempted the write at all + (#284 round-6 review, finding 4). The duplicate itself is reported through + ``duplicates`` rather than swallowed — one id naming two entries is a fault + about the ledger, not an answer about the work.""" + by_id: dict[str, DWEntry] = {} + duplicated: set[str] = set() + for e in parse_ledger(text): + if e.id in by_id: + duplicated.add(e.id) + continue # first wins: `_find_entry` mutates that one + by_id[e.id] = e + buckets: dict[str, list[str]] = {"open": [], "done": [], "unknown": [], "malformed": []} + for dw_id in ids: + entry = by_id.get(dw_id) + if entry is None: + buckets["unknown"].append(dw_id) + continue + word = entry.status.split()[0] if entry.status else "" + buckets[word if word in ("open", "done") else "malformed"].append(dw_id) + return Declared( + open_ids=tuple(buckets["open"]), + already_done=tuple(buckets["done"]), + unknown=tuple(buckets["unknown"]), + malformed=tuple(buckets["malformed"]), + duplicates=tuple(dw_id for dw_id in dict.fromkeys(ids) if dw_id in duplicated), + ) + + def _find_entry(text: str, dw_id: str) -> DWEntry | None: for entry in parse_ledger(text): if entry.id == dw_id: @@ -82,15 +181,13 @@ def _insert_after_status(text: str, entry: DWEntry, line: str) -> str: return text[:insert_at] + "\n" + line + text[insert_at:] -def mark_done(path: Path, dw_id: str, date: str, note: str) -> bool: - """Flip one entry to `status: done ` and record a resolution note. - Returns False (no write) when the entry is missing or already done.""" - if not path.is_file(): - return False - text = path.read_text(encoding="utf-8") +def _apply_done(text: str, dw_id: str, date: str, note: str) -> str | None: + """Flip one entry to `status: done ` + a resolution note *within* `text`. + None when the entry is missing or not open. The entry is re-located after the + status rewrite because that edit shifts every later span offset.""" entry = _find_entry(text, dw_id) if entry is None or not entry.open: - return False + return None status_m = STATUS_RE.search(entry.body) assert status_m is not None # open implies a status line start = entry.span[0] + status_m.start() @@ -98,9 +195,44 @@ def mark_done(path: Path, dw_id: str, date: str, note: str) -> bool: text = text[:start] + f"status: done {date}" + text[end:] entry = _find_entry(text, dw_id) assert entry is not None - text = _insert_after_status(text, entry, f"resolution: {note}") - path.write_text(text, encoding="utf-8") - return True + return _insert_after_status(text, entry, f"resolution: {note}") + + +def mark_done_many(path: Path, dw_ids: Sequence[str], date: str, note: str) -> list[str]: + """Flip every entry in `dw_ids` to `status: done ` + a resolution note, + in ONE read and ONE atomic write. Returns the ids actually flipped (missing + and already-done ids are skipped), in the order given. + + All-or-nothing on purpose. A per-id read-modify-write loop leaves marks on + disk when it raises partway through several ids — a half-applied closure the + caller never gets to journal, so the ledger claims resolutions the run has no + record of. Here a failure writes nothing, and the returned list is exactly + what landed. + + The write goes through :func:`~bmad_loop.platform_util.atomic_write_text` + rather than a bare tmp+replace: swapping a fresh inode over the ledger + otherwise resets its mode (a ``0600`` ledger silently becoming world-readable) + and turns a symlinked ledger into a regular file.""" + if not path.is_file(): + return [] + text = path.read_text(encoding="utf-8") + marked: list[str] = [] + for dw_id in dw_ids: + updated = _apply_done(text, dw_id, date, note) + if updated is None: + continue + text = updated + marked.append(dw_id) + if not marked: + return [] + atomic_write_text(path, text) + return marked + + +def mark_done(path: Path, dw_id: str, date: str, note: str) -> bool: + """Flip one entry to `status: done ` and record a resolution note. + Returns False (no write) when the entry is missing or already done.""" + return bool(mark_done_many(path, [dw_id], date, note)) def append_decision(path: Path, dw_id: str, date: str, label: str, detail: str) -> bool: diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 33e20387..d636ee48 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -19,7 +19,7 @@ import traceback from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING, Callable, NoReturn +from typing import TYPE_CHECKING, Callable, NoReturn, Sequence from . import deferredwork, devcontract, envvars, gates, verify from .adapters.base import CodingCLIAdapter, SessionResult, SessionSpec, SpecSnapshot @@ -43,7 +43,7 @@ SessionRecord, StoryTask, ) -from .platform_util import atomic_replace, retrying_unlink, safe_segment +from .platform_util import atomic_replace, atomic_write_text, retrying_unlink, safe_segment from .plugins import HookBus, HookContext, PluginRegistry from .policy import Policy from .recovery_flow import RecoveryFlow @@ -1719,7 +1719,15 @@ def _finalize_commit_phase(self, task: StoryTask) -> None: self._escalate(task, f"plugin {veto.plugin_id!r} vetoed pre_commit: {veto.reason}") if ctx.proposed_commit_message: message = ctx.proposed_commit_message + # The success boundary for story-declared ledger closure (#234): every + # verify gate, checkpoint, review cycle and pre-commit workflow is behind + # us, and finalize_commit's `git add -A` is still ahead, so an in-repo + # annotation rides this story's own commit. `snapshot` is armed inside + # the close, before its write, so both failure arms below hold the + # pre-close text no matter where in the window a raise lands. + snapshot: list[tuple[Path, str]] = [] try: + self._close_declared_deferred(task, snapshot) # bmad-dev-auto commits its own work each iteration; the orchestrator # squashes that chain plus its uncommitted bookkeeping back onto the # pre-dev baseline as one commit carrying `message`. None means there @@ -1733,7 +1741,16 @@ def _finalize_commit_phase(self, task: StoryTask) -> None: task.resolved_redrive = False task.restore_patch = None except verify.GitError as e: + self._restore_deferred_closes(task, snapshot) self._escalate(task, f"commit failed: {e}") + except BaseException: + # A failed commit is not the only way out of this window: the signal + # handler `_run` installed raises RunStopped from wherever the main + # thread is standing, and a raw OSError on spawn escapes `_run_git` + # as itself. Either leaves the ledger flipped for a commit that does + # not exist. Restore, then re-raise untouched. + self._restore_deferred_closes(task, snapshot) + raise advance(task, Phase.DONE) self.journal.append("story-done", story_key=task.story_key, commit=task.commit_sha) self._emit("post_commit", task) @@ -1768,6 +1785,10 @@ def _dev_review_enabled(self) -> bool: return False return self.policy.review.enabled + # the date stamped into ledger edits; isolated for tests + def _today(self) -> str: + return time.strftime("%Y-%m-%d") + def _observed_frontmatter(self, spec_path: Path, story_key: str, site: str) -> dict | None: """Read a spec's frontmatter on a *bookkeeping* path, degrading an unreadable spec to ``None`` (journaled) instead of a whole-run crash. @@ -1785,8 +1806,7 @@ def _observed_frontmatter(self, spec_path: Path, story_key: str, site: str) -> d Repair writes (``reset_spec_status``, ``mark_done``) deliberately do the opposite and let OSError raise: silently skipping a rewrite would leave the spec in a state the caller believes it fixed. Observation degrades, repair - raises. - """ + raises.""" try: return verify.read_frontmatter(spec_path) except OSError as e: @@ -2049,6 +2069,250 @@ def _post_dev_state_sync(self, task: StoryTask, result_json: dict | None) -> Non target = "review" if review_enabled else "done" sprint_advance(self.workspace.paths.sprint_status, task.story_key, target) + def _manifest_closes_deferred(self, task: StoryTask) -> tuple[str, ...]: + """Deferred-work ids declared for this story by a *manifest* the + orchestrator reads, as opposed to the story spec's own frontmatter. + + Empty here: sprint mode has no manifest, so frontmatter is its only + channel. ``StoriesEngine`` overrides this with its ``stories.yaml`` + entry — the channel that matters for an unattended run, since the spec is + generated later by a dev skill that knows nothing of the ledger.""" + return () + + def _declared_deferred_ids(self, task: StoryTask) -> tuple[str, ...]: + """The ids this story declares it closes, unioned across both channels + and order-preserving (a story that names the same id in the manifest and + in its spec must be marked once and reported once). + + Both halves are read live here, at the commit boundary: what the spec + and the manifest say at the moment of the close is what the story + declares — a declaration edited after the dev artifacts verified must + not be closed against a stale snapshot, because the stale half of that + can close an entry the final spec no longer names. + + Every degraded reading declares nothing, which is the safe direction + for an advisory annotation: the entries stay ``open`` — a miss the next + sweep or retro re-verifies — rather than closed on evidence nobody + could read. An unreadable spec is journaled by + ``_observed_frontmatter``; an unparseable one flattens to ``{}`` there + like every other status gate; and a session-supplied path outside the + orchestrator's roots is refused under the same containment rule the + frontmatter-status reconcile applies, so a surprising absolute path can + never steer a ledger write.""" + ids: list[str] = list(self._manifest_closes_deferred(task)) + spec_path = Path(task.spec_file) if task.spec_file else None + if spec_path is not None: + try: + within = verify.spec_within_roots(spec_path, self.workspace.paths) + except (OSError, RuntimeError): + # resolve() faulted (a symlink loop, an unreadable component): + # containment can vouch for nothing, so refuse the same way. + within = False + if not within: + self.journal.append( + "deferred-close-skipped-out-of-tree", + story_key=task.story_key, + spec=str(spec_path), + ) + else: + fm = self._observed_frontmatter(spec_path, task.story_key, "deferred-close") + declared, error = deferredwork.parse_declaration((fm or {}).get("closes_deferred")) + if error: + self.journal.append( + "deferred-close-malformed", + story_key=task.story_key, + spec=str(spec_path), + error=f"closes_deferred {error}", + ) + ids += declared + return tuple(dict.fromkeys(ids)) + + def _close_declared_deferred( + self, task: StoryTask, snapshot: list[tuple[Path, str]] | None = None + ) -> None: + """At the commit boundary, flip every ledger entry the story declares + via ``closes_deferred:`` to ``status: done `` + a ``resolution:`` + note (#234) — the regular-story counterpart of the sweep bundle close + at ``SweepEngine._close_bundle_ledger_when_spec_status``. + + Declaration is the only signal: closure is never inferred from a diff. + + **Advisory by contract.** The annotation is traceability for the next + sweep or retro, never a gate: no reading of the spec, the manifest or + the ledger can fail the story, and every degraded reading leaves the + entries ``open`` — the direction a sweep re-verifies and a retro can + repair. That contract is the design: closure needs no transactional + coupling to git, because the ledger's consumers re-verify entries + against the codebase anyway. + + **Placement.** Runs from ``_finalize_commit_phase`` — after artifact + verification, the verify commands, every checkpoint, the review loop, + the ``pre_commit_gate`` workflows and the ``pre_commit`` veto — and + still before ``finalize_commit``, whose ``git add -A`` stages an + in-repo annotation into the story's own commit (worktree isolation + included: the unit's ledger rides the unit commit and reaches the + target branch with the ordinary merge). Marking at dev-sync time + instead let a story that later failed verification or review leave the + ledger permanently claiming its work resolved. + + A ledger outside the repo (``ProjectPaths.rebased`` deliberately + shares an external artifact dir between worktrees) is written at the + same moment; the annotation is simply part of no commit, which is + journaled (``deferred-close-external-ledger``). A merge that later + fails leaves that annotation standing — visible, journaled and + human-attended, the accepted advisory trade-off. + + ``snapshot``, when given, receives ``(ledger, pre-close text)`` the + statement before the write, so the caller's failure arms can undo the + edit no matter where inside the window a raise lands — including a + journal append failing after the write published. Cleared again when + nothing was flipped: an empty close needs no restore. + + Idempotent, so the COMMITTING resume arm may re-drive the commit phase + freely — ids are classified against the ledger text, and an entry + already ``done`` is a satisfied declaration, not an unmatched one.""" + ids = self._declared_deferred_ids(task) + if not ids: + return + ledger = self.workspace.paths.deferred_work + try: + text = ledger.read_text(encoding="utf-8") + except FileNotFoundError: + # Nothing at the path — but a dangling link IS something: the link + # names a ledger on a mount that is not answering, and blaming a + # typo (`unmatched`) would misreport an outage. + try: + dangling = ledger.is_symlink() + except OSError: + dangling = True + if dangling: + self._journal_ledger_unavailable(task, ids, ledger, "dangling symlink") + return + text = "" # no ledger: classify reports every id unmatched; nothing is written + except (OSError, UnicodeDecodeError) as e: + # An unavailable or undecodable location is not an answer about the + # entries: close nothing and say so, rather than read an outage as + # "no such entries". + self._journal_ledger_unavailable(task, ids, ledger, f"{e.__class__.__name__}: {e}") + return + if snapshot is not None: + snapshot.append((ledger, text)) + marked = self._apply_deferred_closes(task, ids, ledger, text) + if snapshot is not None and not marked: + # `mark_done_many` writes only when it marks: the ledger is + # byte-identical, so a restore would record a rollback of nothing. + snapshot.clear() + if marked and not self._ledger_in_repo(ledger): + self.journal.append( + "deferred-close-external-ledger", + story_key=task.story_key, + dw_ids=list(marked), + ledger=str(ledger), + note="ledger is outside the repo; the annotation is not part of any commit", + ) + + def _journal_ledger_unavailable( + self, task: StoryTask, ids: Sequence[str], ledger: Path, error: str + ) -> None: + self.journal.append( + "deferred-close-ledger-unavailable", + story_key=task.story_key, + dw_ids=list(ids), + ledger=str(ledger), + error=error, + ) + + def _ledger_in_repo(self, ledger: Path) -> bool: + """Whether the ledger's annotation rides the story's commit. Decided on + RESOLVED paths: an in-repo symlink to a shared external ledger must + count as external. A path that cannot be resolved reports as external — + the write has already happened either way, and "part of no commit" is + the claim that stays true when nothing else is known.""" + try: + return ledger.resolve().is_relative_to(self.workspace.root.resolve()) + except (OSError, RuntimeError): + return False + + def _restore_deferred_closes(self, task: StoryTask, snapshot: list[tuple[Path, str]]) -> None: + """Put the ledger back the way ``_close_declared_deferred`` found it, + after the commit its closures were written for failed (#234): left + alone the entries read ``done`` for work that is in no commit, and the + likeliest recovery makes that permanent — a human-resolved re-drive + sets ``resolved_redrive``, which has ``safe_reset`` preserve the + artifact folders' tracked content through the rollback. + + Whole-document, from the pre-close text. Within the commit window the + engine is the only writer this restore can know about; anything else + that edited the ledger inside it (a native pre-commit hook, say) is + restored away with the close — an accepted advisory trade-off, and the + escalation hands the tree to a human either way. + + Advisory itself, twice over: a failed restore is journaled, never + raised, and the journaling is suppressed rather than allowed to become + the crash — this runs inside except arms whose exception must travel + unchanged.""" + if not snapshot: + return + ledger, before = snapshot[-1] + try: + atomic_write_text(ledger, before) + except OSError as e: + with contextlib.suppress(Exception): + self.journal.append( + "deferred-close-rollback-failed", + story_key=task.story_key, + ledger=str(ledger), + error=str(e), + ) + return + with contextlib.suppress(Exception): + self.journal.append( + "deferred-close-rolled-back", story_key=task.story_key, ledger=str(ledger) + ) + + def _apply_deferred_closes( + self, task: StoryTask, ids: Sequence[str], ledger: Path, text: str + ) -> list[str]: + """Write the closure for `ids`, journal exactly what landed, and return + the ids actually flipped. + + ``text`` is the ledger snapshot the caller already read, never re-read + here: classification and the write have to describe the same document, and + a second read is a second chance for the location to have gone away + underneath them.""" + declared = deferredwork.classify(text, ids) + marked = deferredwork.mark_done_many( + ledger, declared.open_ids, self._today(), f"resolved by story {task.story_key}" + ) + if marked: + self.journal.append("story-deferred-closed", story_key=task.story_key, dw_ids=marked) + if declared.unknown: + self.journal.append( + "deferred-close-unmatched", story_key=task.story_key, dw_ids=list(declared.unknown) + ) + if declared.malformed: + # Present in the ledger but carrying neither an `open` nor a `done` + # status: nothing was marked, and staying quiet would read to the + # operator exactly like a successful close. + self.journal.append( + "deferred-close-malformed", + story_key=task.story_key, + dw_ids=list(declared.malformed), + error="ledger entry status is neither open nor done", + ) + if declared.duplicates: + # One id, two entries: only the first was classified and only the + # first can be marked, so whatever the second says about this work is + # neither read nor written. A corrupt ledger (#286) must not close + # quietly — the operator has to know which id to go look at. + self.journal.append( + "deferred-close-duplicate-id", + story_key=task.story_key, + dw_ids=list(declared.duplicates), + error="the ledger carries more than one entry for this id; only the first was read", + ) + return marked + def _extra_session_env( self, task: StoryTask, role: str, label: str | None = None ) -> dict[str, str]: diff --git a/src/bmad_loop/platform_util.py b/src/bmad_loop/platform_util.py index 36ae4a5b..ba19b38c 100644 --- a/src/bmad_loop/platform_util.py +++ b/src/bmad_loop/platform_util.py @@ -6,8 +6,9 @@ configuration, not a process-lifecycle primitive, so it does not belong on the host. On Linux/macOS — and WSL, which *is* Linux — these preserve today's exact behavior. The file-replace and segment helpers below (``atomic_replace``, -``safe_segment``, ``safe_ref_segment``) are exercised by the platform tests; the pid -kill/liveness Windows branch degrades gracefully and is not yet exercised. +``atomic_write_text``, ``safe_segment``, ``safe_ref_segment``) are exercised by the +platform tests; the pid kill/liveness Windows branch degrades gracefully and is not +yet exercised. ``safe_segment`` and ``safe_ref_segment`` share a contract but not a rule set: the first coerces a Windows *filename* segment, the second a *git ref* component, and @@ -22,10 +23,12 @@ import os import random import re +import shutil import subprocess import sys +import tempfile import time -from contextlib import contextmanager +from contextlib import contextmanager, suppress from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Callable, Iterator @@ -143,6 +146,82 @@ def atomic_replace(tmp: Path, target: Path) -> None: _retry_on_sharing_violation(lambda: os.replace(tmp, target)) +def _copy_xattrs(src: Path, dst: Path) -> None: + """Best-effort extended-attribute copy (Linux). Absent everywhere else, and + unsupported by many filesystems even there, so every failure is ignored: an + xattr we could not carry over is not worth failing a ledger write for.""" + listxattr = getattr(os, "listxattr", None) + if listxattr is None: # portability: xattr syscalls are Linux-only + return + try: + names = listxattr(src) + except OSError: + return + for name in names: + try: + os.setxattr(dst, name, os.getxattr(src, name)) + except OSError: + continue + + +def atomic_write_text(path: Path, text: str) -> None: + """Replace ``path``'s contents with ``text`` atomically, preserving what the + replacement would otherwise silently discard. + + ``os.replace`` swaps a *new inode* into place, so a naive tmp-write-and-replace + quietly resets everything carried by the old file rather than by its name. This + restores the parts that matter: + + * **Symlinks are followed.** ``path.resolve()`` first, so a ledger symlinked + into the repo keeps being a symlink and the real file is what gets rewritten + — a replace against the link itself would turn it into a regular file and + orphan the target. + * **Permission bits survive.** A ``0600`` file stays ``0600`` instead of + becoming ``0644 & ~umask``, which on a shared artifact dir is the difference + between "the group can still write this" and a silent lockout (or a + disclosure). + * **Extended attributes survive** where the platform has them (best effort). + + Ownership is NOT preserved — an unprivileged process cannot chown — so a file + written by another user changes hands. Callers writing genuinely shared, + multi-user state need more than this helper. A target that does not exist yet + is created with ``mkstemp``'s private ``0600``, not the umask default: there is + no prior mode to carry over, and the restrictive choice is the safe one. + + The temp file is uniquely named in the target's own directory: same filesystem + (``os.replace`` cannot cross one), and no fixed ``.tmp`` sibling for a + concurrent writer of the same file to collide with. A failure anywhere leaves + the original untouched and removes the temp. + + The contents are **fsynced before the replace publishes them**. Closing a file + only hands the data to the page cache, so a machine that loses power just + after the rename can come back with the new name pointing at blocks that were + never written — a zero-length or torn ledger, which parses as *no entries* and + so reads as the whole file's worth of hand-written work having vanished. + Ordering the flush before the rename means a crash yields either the old file + or the complete new one. The directory itself is deliberately not synced: that + would make the *rename* durable, and losing the rename just leaves the old + contents in place — stale, never corrupt.""" + target = path.resolve() + fd, tmp_name = tempfile.mkstemp(dir=str(target.parent), prefix=target.name + ".", suffix=".tmp") + tmp = Path(tmp_name) + try: + # newline default (translating) deliberately: matches the Path.write_text + # this replaced, so a ledger's line endings do not change under Windows. + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write(text) + fh.flush() # userspace buffer -> kernel, so there is something to sync + os.fsync(fh.fileno()) + if target.exists(): + shutil.copymode(target, tmp) + _copy_xattrs(target, tmp) + atomic_replace(tmp, target) + except BaseException: + with suppress(OSError): + tmp.unlink() + raise + + def retrying_unlink(path: Path) -> None: """``path.unlink()`` with the same win32 retry as :func:`atomic_replace`. diff --git a/src/bmad_loop/stories.py b/src/bmad_loop/stories.py index 327b3294..84d898cd 100644 --- a/src/bmad_loop/stories.py +++ b/src/bmad_loop/stories.py @@ -6,10 +6,10 @@ execution order — **there is no ``depends_on`` field**, so the schedule is a single left-to-right scan, not a DAG. Each entry pins a stable, prefix-free, machine-opaque ``id`` plus ``title``/``description`` and the caller-only knobs -``spec_checkpoint`` / ``done_checkpoint`` / ``invoke_dev_with``. ``status`` is -deliberately absent: bmad-spec is the sole writer of ``stories.yaml`` and -bmad-dev-auto is the sole writer of each story spec's status — the orchestrator -writes neither. +``spec_checkpoint`` / ``done_checkpoint`` / ``invoke_dev_with`` / +``closes_deferred``. ``status`` is deliberately absent: bmad-spec is the sole +writer of ``stories.yaml`` and bmad-dev-auto is the sole writer of each story +spec's status — the orchestrator writes neither. This module is the strict, typed parser the orchestrator reads it through. The upstream schema (validity rule 4) already says ids are quoted strings of @@ -28,6 +28,7 @@ import yaml +from . import deferredwork from .frontmatter import read_frontmatter, status_of # Fixed-name discovery, like SPEC.md / .memlog.md — never listed in companions. @@ -84,7 +85,14 @@ class StoryEntry: """One story in the breakdown. ``id`` is stable once its spec file exists; the checkpoint flags are independent (a story may set both and pause twice). ``invoke_dev_with`` is free text appended verbatim to the dispatch prompt — - the single planner->dev channel, never interpreted here.""" + the single planner->dev channel, never interpreted here. + + ``closes_deferred`` names the deferred-work ledger ids this story closes + (#234). It is a *declaration channel*, not a status: the orchestrator marks + those entries resolved at clean close, and the ids are checked against the + ledger at preflight. It lives here as well as in the story spec's + frontmatter because the breakdown is written while the ledger is in view, + whereas the spec is generated later by a skill that knows nothing of it.""" id: str title: str @@ -92,6 +100,7 @@ class StoryEntry: spec_checkpoint: bool = False done_checkpoint: bool = False invoke_dev_with: str = "" + closes_deferred: tuple[str, ...] = () @dataclass(frozen=True) @@ -124,6 +133,15 @@ def load_stories(spec_folder: Path | str) -> Stories: # fault — every caller already catches that and prints a clean "stories mode:" # error instead of crashing preflight/dry-run/status with a traceback. raise StoriesError(f"stories.yaml is not valid UTF-8: {path}: {e}") from e + except OSError as e: + # Same rule for a manifest that is present but cannot be read (permissions, + # an I/O error, a dead mount). `is_file()` above only rules out absence, so + # this escaped as a bare OSError and crashed the run from whichever caller + # happened to hit it first — including the commit-boundary read of + # `closes_deferred`, whose whole contract is to journal an unreadable + # manifest and carry on with the spec channel (#284 round-5 review, + # finding 5). + raise StoriesError(f"stories.yaml could not be read: {path}: {e}") from e try: doc = yaml.safe_load(raw) except yaml.YAMLError as e: @@ -174,6 +192,7 @@ def _parse_entry(raw: object, index: int) -> StoryEntry: spec_checkpoint=_bool_field(raw, "spec_checkpoint", story_id), done_checkpoint=_bool_field(raw, "done_checkpoint", story_id), invoke_dev_with=_text_field(raw, "invoke_dev_with", story_id), + closes_deferred=_id_list_field(raw, "closes_deferred", story_id), ) @@ -231,6 +250,25 @@ def _text_field(raw: dict, key: str, story_id: str) -> str: return value +def _id_list_field(raw: dict, key: str, story_id: str) -> tuple[str, ...]: + """Optional list of ledger ids, defaulting empty when missing/null (#234). + + The reading itself lives in :func:`deferredwork.parse_declaration`, shared + with the engine's close hook and ``validate`` so the same mistake cannot mean + different things in the manifest and in a story spec's frontmatter. Only the + *severity* differs: a manifest is a schema the parser owns, so a wrong + container raises here, where the engine journals and ``validate`` warns. + + Whether an id names a real entry is not checked here: this module never reads + the ledger, and a stale reference is a `validate` warning and a journaled + close-time note — never a parse failure. + """ + ids, error = deferredwork.parse_declaration(raw.get(key)) + if error: + raise StoriesError(f"stories.yaml story {story_id!r} field {key!r} {error}") + return ids + + def _validate_prefix_free(ids: list[str]) -> None: """No id may equal another id plus a ``-suffix`` (schema validity rule 2). diff --git a/src/bmad_loop/stories_engine.py b/src/bmad_loop/stories_engine.py index f60378ae..38f280bf 100644 --- a/src/bmad_loop/stories_engine.py +++ b/src/bmad_loop/stories_engine.py @@ -413,11 +413,47 @@ def _plan_halt_leg(self, task: StoryTask, entry: stories.StoryEntry | None) -> b # ---------------------------------------------------------- sync + verify def _post_dev_state_sync(self, task: StoryTask, result_json: dict | None) -> None: - """No-op: stories mode has no sprint board (and no deferred-work ledger to - flip). Honors the contract's "the orchestrator writes nothing" on the - happy path — the dev skill is the sole writer of each story spec's status.""" + """No-op: stories mode has no sprint board. Honors the contract's "the + orchestrator writes nothing" on the happy path — the dev skill is the sole + writer of each story spec's status. + + Deferred-work closure is deliberately NOT here. It is project-wide state + (a story's ``closes_deferred:`` declaration applies in this mode too), but + it belongs at the commit boundary rather than at dev-sync time, so a story + that later fails verification or review never leaves the ledger claiming + its work resolved — see ``Engine._close_declared_deferred`` (#234).""" return + def _manifest_closes_deferred(self, task: StoryTask) -> tuple[str, ...]: + """The ``stories.yaml`` entry's ``closes_deferred`` ids. + + This is the channel that makes story-declared closure work unattended: + ``bmad-dev-auto`` writes the story spec and knows nothing of the ledger, + so with the spec frontmatter alone a human would have to hand-edit every + generated spec. The breakdown, by contrast, is authored while the ledger + is in view. Both channels compose — the base hook unions them. + + Read here rather than through ``_entry_for`` because the two want opposite + things from an unreadable manifest. ``_entry_for`` is a dispatch helper: it + warns once per story and falls back to a bare folder+id dispatch that still + works. Here the fallback is silence — the ids are not persisted anywhere, + unlike the spec half's capture, so a parse failure at the commit boundary + drops a declared closure with no trace beyond a generic one-time warning + that may already have been spent earlier in the run. Say what was actually + lost, at the site that lost it.""" + try: + entry = self._load_stories().get(task.story_key) + except stories.StoriesError as e: + self.journal.append( + "deferred-close-declaration-unreadable", + story_key=task.story_key, + source="stories.yaml", + error=str(e), + note="manifest-declared ids cannot be read; nothing is closed for them", + ) + return () + return entry.closes_deferred if entry else () + def _verify_dev_artifacts(self, task: StoryTask, result_json: dict | None): # The adapter marks a plan-halt leg's synthesized result `plan_halt`; latch # it onto the task so _drive_story pauses for plan review (and clears it on diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 4a25d76d..988d57b7 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -13,7 +13,6 @@ import json import re -import time from dataclasses import dataclass from pathlib import Path from typing import Any, Callable @@ -403,10 +402,6 @@ def __init__( self._skipped_decisions: set[str] = set() self.state.run_type = "sweep" - # the date stamped into ledger edits; isolated for tests - def _today(self) -> str: - return time.strftime("%Y-%m-%d") - def _remaining_estimate(self) -> int | None: """Sweep override of the graceful-stop hint: how many deferred-work entries are still open in the ledger — the work a resume would pick up. @@ -1230,6 +1225,16 @@ def _post_dev_state_sync(self, task: StoryTask, result_json: dict | None) -> Non success_status = "in-review" if self._dev_review_enabled() else "done" self._close_bundle_ledger_when_spec_status(task, str(spec_file), success_status) + def _close_declared_deferred( + self, task: StoryTask, snapshot: list[tuple[Path, str]] | None = None + ) -> None: + """No-op: a bundle's ledger closure is owned by + ``_close_bundle_ledger_when_spec_status``, which runs at dev-sync time + because ``verify_review_bundle`` *requires* those entries closed before it + will pass. Letting the base class's commit-boundary hook (#234) also fire + here would re-derive closure for a task whose ids come from + ``task.dw_ids``, not from a ``closes_deferred:`` declaration.""" + def _close_bundle_ledger_when_spec_status( self, task: StoryTask, diff --git a/tests/conftest.py b/tests/conftest.py index 274fc56e..cc58dfba 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -308,15 +308,32 @@ def set_sprint(paths: ProjectPaths, key: str, status: str) -> None: paths.sprint_status.write_text(yaml.safe_dump(doc, sort_keys=False)) -def write_spec(path: Path, status: str, baseline: str, *, prose_status: str | None = None) -> None: +def write_spec( + path: Path, + status: str, + baseline: str, + *, + prose_status: str | None = None, + closes_deferred: object = None, +) -> None: """Write a spec the way the real bmad-dev-auto skill does. The skill's step-03 stamps `baseline_revision` and NEVER `baseline_commit` (that name exists only in the orchestrator's synthesized result.json), so this fixture stamps the same key — a reader that only knows `baseline_commit` must fail a test here, - not sail through production (issue #89).""" + not sail through production (issue #89). + + ``closes_deferred`` writes the story-declared ledger-closure field (#234): a + list renders as a YAML flow sequence, and a bare string renders as a scalar — + the wrong-container mistake whose handling must not depend on which file it + was made in.""" + declare = "" + if isinstance(closes_deferred, list): + declare = f"closes_deferred: [{', '.join(closes_deferred)}]\n" + elif closes_deferred is not None: + declare = f"closes_deferred: {closes_deferred}\n" body = ( f"---\ntitle: 'test'\ntype: 'feature'\nstatus: '{status}'\n" - f"baseline_revision: '{baseline}'\n---\n\n## Intent\n\ntest spec\n" + f"baseline_revision: '{baseline}'\n{declare}---\n\n## Intent\n\ntest spec\n" ) if prose_status is not None: # mirror bmad-dev-auto's terminal finalize: it appends a `## Auto Run @@ -387,6 +404,7 @@ def dev_effect( prose_status: str | None = None, seen: list[str] | None = None, write_src: bool = True, + closes_deferred: object = None, ): """Simulate a successful bmad-dev-auto session: it self-finalizes the spec (no in-review handoff — always straight to ``done``) but never touches the @@ -414,7 +432,9 @@ def effect(spec: SessionSpec) -> SessionResult: if write_src: source.write_text(source.read_text() + f"change for {story_key}\n") sp = spec_path(paths, story_key) - write_spec(sp, final_status, baseline, prose_status=prose_status) + write_spec( + sp, final_status, baseline, prose_status=prose_status, closes_deferred=closes_deferred + ) # deliberately NO set_sprint: the dev skill does not write sprint-status return SessionResult( status="completed", @@ -460,7 +480,9 @@ def effect(spec: SessionSpec) -> SessionResult: sp = spec_path(paths, story_key) baseline = _spec_baseline(sp) status = "done" if finalized else "in-progress" - write_spec(sp, status, baseline) + # A review pass rewrites the status, not the whole frontmatter — carry any + # `closes_deferred:` declaration through verbatim, as the real skill does. + write_spec(sp, status, baseline, closes_deferred=_spec_closes_deferred(sp)) if finalized: set_sprint(paths, story_key, "done") return SessionResult( @@ -479,6 +501,18 @@ def effect(spec: SessionSpec) -> SessionResult: return effect +def _spec_closes_deferred(path: Path) -> object: + """The spec's `closes_deferred:` declaration as `write_spec` would re-render + it — a list for a flow sequence, the raw text otherwise. None when absent.""" + for line in path.read_text().splitlines(): + if line.startswith("closes_deferred:"): + value = line.split(":", 1)[1].strip() + if value.startswith("[") and value.endswith("]"): + return [p.strip() for p in value[1:-1].split(",") if p.strip()] + return value + return None + + def _spec_baseline(path: Path) -> str: """Read back whichever baseline key a spec carries: `write_spec` stamps `baseline_revision` like the real skill, but hand-rolled fixture specs (and diff --git a/tests/test_deferredwork.py b/tests/test_deferredwork.py index ffa89906..0d419d57 100644 --- a/tests/test_deferredwork.py +++ b/tests/test_deferredwork.py @@ -2,15 +2,20 @@ from pathlib import Path +import pytest + from bmad_loop.deferredwork import ( append_decision, append_entry, + classify, field_line_present, field_severity, has_legacy, mark_done, + mark_done_many, next_seq, open_ids, + parse_declaration, parse_ledger, parse_legacy, ) @@ -485,3 +490,165 @@ def test_field_line_present_matches_field_not_substring(): assert not field_line_present(body, "origin", "review-budget") # a value that only appears incidentally inside `reason:` is not a field line assert not field_line_present(body, "source_spec", "spec-foobar.md") + + +# ------------------------------ closes_deferred declaration primitives (#234) + + +def test_parse_declaration_normalizes_items_and_dedupes(): + """Lenient about items, exactly as the id parser is: an LLM-authored manifest + may emit an unquoted id as a string and a bare number as an int.""" + ids, error = parse_declaration(["DW-1", " DW-2 ", 5, "", "DW-1"]) + assert ids == ("DW-1", "DW-2", "5") # stripped, blanks dropped, order-preserving dedupe + assert error is None + + +def test_parse_declaration_defaults_empty_when_absent(): + assert parse_declaration(None) == ((), None) + assert parse_declaration([]) == ((), None) + + +def test_parse_declaration_rejects_a_non_list_container(): + """A string is iterable, so a lenient reading would silently turn one id into + a list of characters. Callers pick the severity; the reading is shared.""" + ids, error = parse_declaration("DW-1") + assert ids == () + assert error is not None and "must be a list" in error and "str" in error + + +def test_classify_partitions_open_done_unknown_and_malformed(): + """Four outcomes, not two: `not open` hides a satisfied declaration (a resume + re-driving a landed close, which must stay silent) and an entry whose status + line cannot be read at all (which must not).""" + text = ( + "# Deferred Work\n\n" + "### DW-1: a\nstatus: open\n\n" + "### DW-2: b\nstatus: done 2026-06-01\n\n" + "### DW-3: c\nstatus: in-progress\n\n" + "### DW-4: d\nreason: no status line at all\n" + ) + declared = classify(text, ["DW-1", "DW-2", "DW-3", "DW-4", "DW-99"]) + + assert declared.open_ids == ("DW-1",) + assert declared.already_done == ("DW-2",) + assert declared.unknown == ("DW-99",) + assert declared.malformed == ("DW-3", "DW-4") + + +DUPLICATE_OPEN_FIRST = ( + "# Deferred Work\n\n" + "### DW-1: the first copy\nstatus: open\n\n" + "### DW-1: the second copy\nstatus: done 2026-06-01\n" +) +DUPLICATE_DONE_FIRST = ( + "# Deferred Work\n\n" + "### DW-1: the first copy\nstatus: done 2026-06-01\n\n" + "### DW-1: the second copy\nstatus: open\n" +) + + +@pytest.mark.parametrize( + ("text", "expected_open", "expected_done"), + [ + (DUPLICATE_OPEN_FIRST, ("DW-1",), ()), + (DUPLICATE_DONE_FIRST, (), ("DW-1",)), + ], + ids=["open-first", "done-first"], +) +def test_classify_reads_the_same_duplicate_the_mutation_writes(text, expected_open, expected_done): + """`classify` must name the entry `_find_entry` acts on. Indexing last-wins + while the mutation takes the first made them disagree, and BOTH orders then + closed nothing while saying nothing: done-first classified `open`, then + `_apply_done` refused the done copy it found first (so nothing was marked and + not even an unmatched warning followed); open-first classified `already_done` + and never attempted the write (#284 round-6 review, finding 4).""" + declared = classify(text, ["DW-1"]) + + assert declared.open_ids == expected_open + assert declared.already_done == expected_done + assert declared.unknown == () + assert declared.duplicates == ("DW-1",) # reported either way — the ledger is corrupt + + +@pytest.mark.parametrize( + ("text", "expected", "after"), + [ + (DUPLICATE_OPEN_FIRST, ["DW-1"], ["done", "done"]), + (DUPLICATE_DONE_FIRST, [], ["done", "open"]), + ], + ids=["open-first", "done-first"], +) +def test_mark_done_many_agrees_with_classify_on_a_duplicated_id(tmp_path, text, expected, after): + """The other half of the same contract: what `classify` calls open is exactly + what the write marks, so an id is never reported closed without being closed — + nor, as before, classified open and then silently left open. + + The done-first ledger still ends with an open copy nobody touched. That is the + honest outcome of a corrupt ledger (#286) and it is why the close is journaled + as a duplicate rather than passed over.""" + p = tmp_path / "deferred-work.md" + p.write_text(text, encoding="utf-8") + declared = classify(text, ["DW-1"]) + + assert list(mark_done_many(p, declared.open_ids, "2026-07-24", "note")) == expected + + entries = parse_ledger(p.read_text(encoding="utf-8")) + assert [e.status.split()[0] for e in entries] == after + + +def test_classify_reports_no_duplicates_for_a_well_formed_ledger(): + declared = classify(LEDGER, ["DW-1", "DW-2"]) + + assert declared.duplicates == () + + +def test_mark_done_many_writes_once_and_reports_what_landed(tmp_path): + p = tmp_path / "deferred-work.md" + p.write_text(LEDGER, encoding="utf-8") + + # DW-2 is already done in the fixture, DW-99 is absent — both skipped, neither + # an error: only the entries that were open get marked, in the order given. + marked = mark_done_many( + p, ["DW-1", "DW-3", "DW-2", "DW-99"], "2026-07-24", "resolved by story 1" + ) + + assert marked == ["DW-1", "DW-3"] + entries = {e.id: e for e in parse_ledger(p.read_text(encoding="utf-8"))} + for dw in ("DW-1", "DW-3"): + assert entries[dw].status == "done 2026-07-24" + assert "resolution: resolved by story 1" in entries[dw].body + + +def test_mark_done_many_is_all_or_nothing_on_a_write_failure(tmp_path, monkeypatch): + """A per-id read-modify-write loop leaves earlier marks on disk when it raises + partway through — a half-applied closure the caller never gets to journal, so + the ledger claims resolutions the run has no record of.""" + p = tmp_path / "deferred-work.md" + p.write_text(LEDGER, encoding="utf-8") + before = p.read_bytes() + monkeypatch.setattr( + "bmad_loop.deferredwork.atomic_write_text", + lambda path, text: (_ for _ in ()).throw(OSError("disk full")), + ) + + # the raise must REACH the caller, not just leave the file alone: a swallowed + # error returning [] reads as "nothing declared was open" — the engine disarms + # its restore snapshot and no journal line ever records the failed write + with pytest.raises(OSError): + mark_done_many(p, ["DW-1", "DW-3"], "2026-07-24", "note") + + assert p.read_bytes() == before # nothing partially applied + + +def test_mark_done_many_skips_an_already_done_entry(tmp_path): + """Idempotent for a resume that re-drives a close that already landed: no + second resolution line, and the id is not reported as newly marked.""" + p = tmp_path / "deferred-work.md" + p.write_text(LEDGER, encoding="utf-8") + mark_done_many(p, ["DW-1"], "2026-07-24", "resolved by story 1") + + again = mark_done_many(p, ["DW-1"], "2026-07-25", "resolved by story 1") + + assert again == [] + body = next(e for e in parse_ledger(p.read_text(encoding="utf-8")) if e.id == "DW-1").body + assert body.count("resolution: resolved by story 1") == 1 diff --git a/tests/test_engine.py b/tests/test_engine.py index 78703b72..70c8e735 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -21,11 +21,12 @@ review_effect, set_sprint, spec_path, + write_ledger, write_spec, write_sprint, ) -from bmad_loop import platform_util, verify +from bmad_loop import deferredwork, platform_util, verify from bmad_loop.adapters.base import SessionResult from bmad_loop.adapters.mock import MockAdapter from bmad_loop.engine import Engine, RunPaused, RunStopped, _run_depth @@ -90,6 +91,10 @@ def make_engine(project, script, policy=None, **kwargs) -> tuple[Engine, MockAda def resume_engine(project, engine, script, policy=None) -> tuple[Engine, MockAdapter]: state = load_state(engine.run_dir) + # `cli._resume_paused_run` refuses a finished run outright. Without the same + # refusal here a test can "resume" what the CLI never would, and prove a + # recovery path that does not exist (#284 round-6 review, finding 1). + assert not state.finished, "cli._resume_paused_run refuses a finished run" state.clear_pause() adapter = MockAdapter(script) new_engine = Engine( @@ -1941,6 +1946,541 @@ def test_post_dev_state_sync_skips_on_unreadable_spec(project, monkeypatch): assert events[0]["story_key"] == "1-1-a" +# ------------------------------------------- closes_deferred auto-resolve (#234) + + +def _ledger_entries(project) -> dict: + return { + e.id: e + for e in deferredwork.parse_ledger(project.deferred_work.read_text(encoding="utf-8")) + } + + +def _vetoing_emit(engine, stage: str, action: str): + """Wrap `engine._emit` so `stage` resolves to a plugin veto of `action`, + without a real plugin on disk — `_emit` returns None on the zero-plugin fast + path, so the context is built the same way the bus would.""" + from bmad_loop.plugins.context import Veto + + original = engine._emit + + def emit(s, task=None, **fields): + ctx = original(s, task, **fields) + if s != stage: + return ctx + ctx = ctx or engine._make_context(s, task, **fields) + ctx.add_veto(Veto(action, "test veto", "test-plugin")) + return ctx + + return emit + + +def _closes_deferred_run(project, dw_ids, *, ledger=None, **dev_kwargs): + """A whole sprint-mode story run whose dev session declares `closes_deferred: + dw_ids`, over a committed ledger. `followup_review=False` so the story takes + the skip-review path straight to commit — these assert the *lifecycle* + placement of the close, not the review loop. + + Everything is committed up front and the run is driven end to end, because + the defect this covers (#234 review) only appears downstream of the dev + session: the close used to land at dev-sync time, before verification, review + and commit could still reject the story.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + write_ledger(project, ledger or {"DW-1": "open"}) + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a", followup_review=False, closes_deferred=dw_ids, **dev_kwargs)], + ) + return engine + + +def test_closes_deferred_marks_declared_entries_done(project): + """A story spec declaring `closes_deferred:` flips each referenced ledger + entry to `status: done ` + a `resolution:` note when the story commits — + the write the loop never made, forcing retros to reconstruct closure by hand + (#234). Closure is declared, never inferred from the diff.""" + engine = _closes_deferred_run(project, ["DW-1"]) + + summary = engine.run() + + assert summary.done == 1 + entry = _ledger_entries(project)["DW-1"] + assert entry.status.startswith("done") and not entry.open + assert "resolution: resolved by story 1-1-a" in entry.body + closed = [e for e in engine.journal.entries() if e["kind"] == "story-deferred-closed"] + assert len(closed) == 1 + assert closed[0]["dw_ids"] == ["DW-1"] and closed[0]["story_key"] == "1-1-a" + + +def test_closes_deferred_annotation_rides_the_story_commit(project): + """The annotation must be IN the story's own commit, not left dirty behind it: + the loop has to end each story on a clean tree (story N+1's step-01 HALTs on a + dirty one), which is why closure happens just before `finalize_commit`'s + `git add -A` rather than after the commit.""" + engine = _closes_deferred_run(project, ["DW-1"]) + + engine.run() + + committed = git(project.project, "show", "HEAD", "--", str(project.deferred_work)) + assert "+status: done" in committed + assert "+resolution: resolved by story 1-1-a" in committed + assert worktree_clean(project.project) + + +def test_closes_deferred_stays_open_when_verify_defers_the_story(project): + """The reviewer's repro (#234 review, finding 1). The dev session finalizes + its spec — declaration and all — but the story then fails deterministic + verification and defers. Closing at dev-sync time made that permanent: the + in-place defer path snapshots the ledger AFTER the mutation and restores it + over the rollback, so a rejected story left the ledger claiming its work + resolved. At the commit boundary there is nothing to undo.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + write_ledger(project, {"DW-1": "open"}) + # write_src=False: no diff since baseline, so the proof-of-work gate fails and + # every attempt defers — the spec still reaches `done` with its declaration. + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a", write_src=False, closes_deferred=["DW-1"])] * 3, + ) + + summary = engine.run() + + assert summary.deferred == 1 and summary.done == 0 + assert _ledger_entries(project)["DW-1"].open # never claimed resolved + kinds = {e["kind"] for e in engine.journal.entries()} + assert "story-deferred-closed" not in kinds + assert "story-deferred" in kinds # the story really did defer + + +def test_closes_deferred_stays_open_when_the_story_escalates(project): + """`_escalate` rolls nothing back — it advances to ESCALATED and pauses. A + close written before that point survives into the operator's checkout, where + the NEXT story's `git add -A` sweeps it into an unrelated commit.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + write_ledger(project, {"DW-1": "open"}) + engine, _ = make_engine( + project, [dev_effect(project, "1-1-a", followup_review=False, closes_deferred=["DW-1"])] + ) + engine._emit = _vetoing_emit(engine, "pre_commit", "pause") + + summary = engine.run() + + assert summary.paused and summary.done == 0 + assert _ledger_entries(project)["DW-1"].open + assert "story-deferred-closed" not in {e["kind"] for e in engine.journal.entries()} + + +def test_closes_deferred_idempotent_on_resume(project): + """A host death in the commit window re-drives `_finalize_commit_phase` on + resume, so the close runs twice. The second pass must write nothing and stay + *silent*: an id that is present-but-already-done is a satisfied declaration, + not an unmatched one.""" + engine = _closes_deferred_run(project, ["DW-1"]) + engine.run() + task = load_state(engine.run_dir).tasks["1-1-a"] + + # re-drive the exact commit-phase step a resume performs + engine._close_declared_deferred(task) + + body = _ledger_entries(project)["DW-1"].body + assert body.count("resolution: resolved by story 1-1-a") == 1 # not doubled + kinds = [e["kind"] for e in engine.journal.entries()] + assert kinds.count("story-deferred-closed") == 1 # only the first pass wrote + assert "deferred-close-unmatched" not in kinds # already-done is NOT unknown + + +def test_closes_deferred_warns_on_unknown_id(project): + """An id absent from the ledger (a typo, or an entry since reworded) is + journaled and dropped — never a story failure, and never a ledger write.""" + engine = _closes_deferred_run(project, ["DW-99"]) + before = project.deferred_work.read_bytes() + + summary = engine.run() + + assert summary.done == 1 # the annotation is traceability, not a gate + assert project.deferred_work.read_bytes() == before # ledger untouched + events = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-unmatched"] + assert len(events) == 1 + assert events[0]["dw_ids"] == ["DW-99"] and events[0]["story_key"] == "1-1-a" + assert "story-deferred-closed" not in [e["kind"] for e in engine.journal.entries()] + + +def test_closes_deferred_reports_a_malformed_ledger_entry(project): + """An entry that exists but carries neither an `open` nor a `done` status + cannot be marked. Reporting only present-vs-absent left that case in silence, + which reads to the operator exactly like a successful close (#234 review).""" + engine = _closes_deferred_run(project, ["DW-1"], ledger={"DW-1": "in-progress"}) + + engine.run() + + assert not _ledger_entries(project)["DW-1"].status.startswith("done") + events = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-malformed"] + assert len(events) == 1 and events[0]["dw_ids"] == ["DW-1"] + + +@pytest.mark.parametrize( + ("first", "second", "closed"), + [("open", "done 2026-06-01", True), ("done 2026-06-01", "open", False)], + ids=["open-first", "done-first"], +) +def test_closes_deferred_reports_a_duplicated_ledger_id(project, first, second, closed): + """One id, two entries: only the first is read and only the first can be + written, so the second is neither. Both orders used to pass in total silence — + `classify` indexed the LAST entry while the mutation took the first, so a + done-first ledger classified the id open and then marked nothing at all + (#284 round-6 review, finding 4). + + The close itself still behaves as the first entry dictates. What changes is + that the operator is told the ledger names one id twice (#286).""" + engine = _closes_deferred_run(project, ["DW-1"]) + project.deferred_work.write_text( + "# Deferred Work\n\n" + f"### DW-1: the first copy\nstatus: {first}\n\n" + f"### DW-1: the second copy\nstatus: {second}\n", + encoding="utf-8", + ) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "duplicate id") + + summary = engine.run() + + assert summary.done == 1 # never a gate + kinds = [e["kind"] for e in engine.journal.entries()] + events = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-duplicate-id"] + assert len(events) == 1 and events[0]["dw_ids"] == ["DW-1"] + # the first entry decides, and the close is reported only when it happened + assert ("story-deferred-closed" in kinds) is closed + parsed = deferredwork.parse_ledger(project.deferred_work.read_text(encoding="utf-8")) + assert [e.status.split()[0] for e in parsed] == ( + ["done", "done"] if closed else ["done", "open"] + ) + + +def test_closes_deferred_reports_a_wrong_container_declaration(project): + """`closes_deferred: DW-1` (a scalar, not a list) is a real declaration of + intent. Reading it as an empty list closed nothing and said nothing, while + the same mistake in `stories.yaml` was a hard schema error — the two channels + now agree that it is a mistake worth surfacing.""" + engine = _closes_deferred_run(project, "DW-1") # bare scalar + before = project.deferred_work.read_bytes() + + summary = engine.run() + + assert summary.done == 1 # still never a gate + assert project.deferred_work.read_bytes() == before + events = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-malformed"] + assert len(events) == 1 # the one live read, at the commit boundary + assert "must be a list" in events[0]["error"] + + +def test_closes_deferred_noop_when_field_absent(project): + """The default spec declares nothing: no ledger write and no journal noise on + the close path every ordinary story takes.""" + engine = _closes_deferred_run(project, None) + before = project.deferred_work.read_bytes() + + engine.run() + + assert project.deferred_work.read_bytes() == before + kinds = {e["kind"] for e in engine.journal.entries()} + assert not kinds & { + "story-deferred-closed", + "deferred-close-unmatched", + "deferred-close-malformed", + } + + +def test_closes_deferred_refuses_an_out_of_tree_spec(project, tmp_path): + """The sprint-mode spec path comes from the session's own result.json. A + stale or hostile absolute path carrying `status: done` + `closes_deferred` + must not be able to steer a ledger write — the same root-containment rule the + frontmatter-status reconcile already applies to its one session-supplied + path (#234 review, finding 4).""" + write_ledger(project, {"DW-1": "open"}) + engine, _ = make_engine(project, []) + outside = tmp_path / "elsewhere" / "spec.md" + outside.parent.mkdir(parents=True, exist_ok=True) + write_spec(outside, "done", "abc123", closes_deferred=["DW-1"]) + task = StoryTask(story_key="1-1-a", epic=1) + task.spec_file = str(outside) + + engine._close_declared_deferred(task) + + assert _ledger_entries(project)["DW-1"].open + events = [ + e for e in engine.journal.entries() if e["kind"] == "deferred-close-skipped-out-of-tree" + ] + assert len(events) == 1 and events[0]["story_key"] == "1-1-a" + + +def _reject_commits(project): + """Install a native `pre-commit` hook that rejects every commit — the + real-world shape of a `finalize_commit` failure (a lint/secret hook saying no) + that no amount of orchestrator correctness prevents.""" + hooks = project.project / ".git" / "hooks" + hooks.mkdir(parents=True, exist_ok=True) + hook = hooks / "pre-commit" + hook.write_text("#!/bin/sh\nexit 1\n", encoding="utf-8") + hook.chmod(0o755) + return hook + + +def test_closes_deferred_rolls_back_when_the_commit_fails(project): + """The close is written just BEFORE `finalize_commit` so an in-repo annotation + rides the story's own commit — but that commit can still fail, and `_escalate` + unwinds nothing. Left alone the entry claims work that is in no commit, and + the usual recovery makes it permanent: a resolved re-drive preserves the + artifact folders' tracked content through `safe_reset` (#284 review, finding 1). + """ + engine = _closes_deferred_run(project, ["DW-1"]) + before = project.deferred_work.read_bytes() + _reject_commits(project) + + summary = engine.run() + + assert summary.done == 0 and summary.paused # the commit really did fail + assert _ledger_entries(project)["DW-1"].open + assert project.deferred_work.read_bytes() == before # byte-identical, not just re-opened + kinds = [e["kind"] for e in engine.journal.entries()] + # the write happened and was undone: both are on the record, in that order + assert kinds.index("story-deferred-closed") < kinds.index("deferred-close-rolled-back") + + +def test_closes_deferred_lands_once_when_a_failed_commit_is_re_driven(project): + """The rollback must leave the story re-drivable: once the hook is gone, the + resumed commit phase re-applies the close exactly once (no doubled + `resolution:` line) and carries it in the commit it belongs to.""" + engine = _closes_deferred_run(project, ["DW-1"]) + hook = _reject_commits(project) + engine.run() + hook.unlink() + # the resolve workflow's re-arm: a resolved re-drive, which is precisely the + # recovery that PRESERVES the artifact folders' tracked content through + # `safe_reset` — so a close left standing here would never be reverted. + rearm_escalation(engine.run_dir) + + resumed, _ = resume_engine( + project, + engine, + [dev_effect(project, "1-1-a", followup_review=False, closes_deferred=["DW-1"])], + ) + summary = resumed.run() + + assert summary.done == 1 + entry = _ledger_entries(project)["DW-1"] + assert not entry.open + assert entry.body.count("resolution: resolved by story 1-1-a") == 1 + committed = git(project.project, "show", "HEAD", "--", str(project.deferred_work)) + assert "status: done" in committed + + +def _external_ledger_paths(project, tmp_path): + """`project` with its artifact dir configured OUTSIDE the repo — the shared, + uncommittable ledger configuration `ProjectPaths.rebased` leaves in place.""" + external = tmp_path / "shared-artifacts" + external.mkdir(exist_ok=True) + return dataclasses.replace(project, implementation_artifacts=external) + + +def test_closes_deferred_external_ledger_is_written_and_journaled(project, tmp_path): + """A ledger outside the repo can ride no commit (`ProjectPaths.rebased` + deliberately shares an external artifact dir between worktrees, and + `git add -A` can never stage it). The advisory contract writes it at the + same commit-boundary moment anyway, and tells the operator the annotation + is part of no commit (`deferred-close-external-ledger`).""" + paths = _external_ledger_paths(project, tmp_path) + write_sprint(paths, {"1-1-a": "ready-for-dev"}) + write_ledger(paths, {"DW-1": "open"}, commit=False) + engine, _ = make_engine( + paths, + [dev_effect(paths, "1-1-a", followup_review=False, closes_deferred=["DW-1"])], + ) + + summary = engine.run() + + assert summary.done == 1 + entry = next( + e + for e in deferredwork.parse_ledger(paths.deferred_work.read_text(encoding="utf-8")) + if e.id == "DW-1" + ) + assert not entry.open and "resolution: resolved by story 1-1-a" in entry.body + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-external-ledger" in kinds # told it rides no commit + assert "story-deferred-closed" in kinds + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +@pytest.mark.parametrize("shape", ["dangling", "looping"]) +def test_closes_deferred_in_repo_broken_ledger_link_is_an_outage_not_a_typo(project, shape): + """A broken ledger link must read as an outage, not as an empty ledger: an + empty read classifies every declared id `unknown`, and the story then + reports a typo (`unmatched`) for a mount that went away. + + Both shapes land in the same answer by different routes: the loop is + refused at the read (ELOOP), the dangling link by the symlink check behind + `FileNotFoundError` — the link existing at all is the evidence a ledger is + expected there.""" + engine = _closes_deferred_run(project, ["DW-1"]) + ledger = project.deferred_work + ledger.unlink() + if shape == "dangling": + ledger.symlink_to(project.project / "gone-mount" / "deferred-work.md") + else: + other = project.project / "ledger-loop" + ledger.symlink_to(other) + other.symlink_to(ledger) + + summary = engine.run() + + assert summary.done == 1 and not summary.crashed # the story still lands + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-ledger-unavailable" in kinds + assert "deferred-close-unmatched" not in kinds + assert "story-deferred-closed" not in kinds + + +def test_closes_deferred_re_reads_the_declaration_at_the_commit_boundary(project, monkeypatch): + """The spec on disk at the commit is what the story declares. Closing against + any earlier snapshot instead lets an id the author has since withdrawn be + marked resolved — a false close, the one outcome this whole path exists to + prevent — and drops one added in the same edit (#284 follow-up review, + finding 3). Both directions must follow the final spec.""" + engine = _closes_deferred_run(project, ["DW-1"], ledger={"DW-1": "open", "DW-3": "open"}) + sp = spec_path(project, "1-1-a") + finalize = engine._finalize_commit_phase + + def edited_after_verification(task): + # the shape of a review session rewriting the frontmatter, or a human + # editing the spec while the review loop runs: DW-1 withdrawn, DW-3 added + write_spec(sp, "done", task.baseline_commit, closes_deferred=["DW-3"]) + return finalize(task) + + monkeypatch.setattr(engine, "_finalize_commit_phase", edited_after_verification) + + summary = engine.run() + + assert summary.done == 1 + entries = _ledger_entries(project) + assert entries["DW-1"].open # withdrawn before the commit: never closed + assert not entries["DW-3"].open # named by the final spec: closed + closed = [e for e in engine.journal.entries() if e["kind"] == "story-deferred-closed"] + assert [e["dw_ids"] for e in closed] == [["DW-3"]] + + +def test_closes_deferred_rolls_back_when_a_signal_stops_the_commit(project, monkeypatch): + """A failed commit is not the only way out of the close→commit window. The + handler `run()` installs raises RunStopped from wherever the main thread is + standing, and `run()` catches it to finalize a *stopped* run — not to repair + bookkeeping. Left alone the ledger claims work that is in no commit, exactly + as a rejecting pre-commit hook did, but past `except verify.GitError` + (#284 follow-up review, finding 2).""" + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: None) + engine = _closes_deferred_run(project, ["DW-1"]) + before = project.deferred_work.read_bytes() + real_finalize = verify.finalize_commit + + def sigterm_mid_commit(*a, **kw): + # in-process, catchable, and routed through the REAL installed handler — + # this is the mechanism, not a stand-in for it + signal.raise_signal(signal.SIGTERM) + return real_finalize(*a, **kw) # unreachable: the handler raises first + + monkeypatch.setattr(verify, "finalize_commit", sigterm_mid_commit) + + summary = engine.run() + + assert summary.done == 0 + assert load_state(engine.run_dir).stopped is True # the stop really landed + assert _ledger_entries(project)["DW-1"].open + assert project.deferred_work.read_bytes() == before # byte-identical + kinds = [e["kind"] for e in engine.journal.entries()] + assert kinds.index("story-deferred-closed") < kinds.index("deferred-close-rolled-back") + + +def test_closes_deferred_rolls_back_when_the_commit_raises_a_non_git_error(project, monkeypatch): + """`_run_git` translates only TimeoutExpired, so a raw OSError from the spawn + itself (a fork failure, git gone from PATH) reaches the commit boundary as + itself and slips past the GitError arm.""" + engine = _closes_deferred_run(project, ["DW-1"]) + before = project.deferred_work.read_bytes() + + def cannot_spawn(*a, **kw): + raise OSError("cannot allocate memory") + + monkeypatch.setattr(verify, "finalize_commit", cannot_spawn) + + summary = engine.run() + + assert summary.crashed # re-raised untouched: the disposition is not ours + assert _ledger_entries(project)["DW-1"].open + assert project.deferred_work.read_bytes() == before + + +def test_closes_deferred_rolls_back_when_the_journal_fails_after_the_ledger_write( + project, monkeypatch +): + """The ledger publishes atomically and the journal line recording it is + written *after*. A full disk between the two used to leave the entry reading + `done` with the rollback un-armed: the snapshot was bound from the close's + RETURN value, which a raise inside it never produces, so the restore had + nothing to work from and `finalize_commit` never ran either — a resolution + claimed for a commit that does not exist (#284 round-5 review, finding 1). + + The snapshot is armed before the write now, so the failure unwinds whether it + lands in the commit or inside the close itself.""" + engine = _closes_deferred_run(project, ["DW-1"]) + before = project.deferred_work.read_bytes() + real_append = engine.journal.append + + def full_disk_recording_the_close(kind, **fields): + # fault ONLY the close record: the rollback's own journal line has to + # survive, or the test could not tell a restore from a crash + if kind == "story-deferred-closed": + raise OSError("No space left on device") + return real_append(kind, **fields) + + monkeypatch.setattr(engine.journal, "append", full_disk_recording_the_close) + + summary = engine.run() + + assert summary.crashed + assert _ledger_entries(project)["DW-1"].open + assert project.deferred_work.read_bytes() == before # byte-identical + rolled = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-rolled-back"] + assert len(rolled) == 1 + + +def test_closes_deferred_rolls_back_when_a_signal_stops_the_close_itself(project, monkeypatch): + """The stop signal can land INSIDE the close, not only inside the commit: the + handler raises from wherever the main thread stands, and the ledger is already + published by the time `mark_done_many` returns. Nothing downstream runs — no + close record, no commit — so without an early-armed snapshot the entry stayed + `done` for work that never landed (#284 round-5 review, finding 1).""" + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: None) + engine = _closes_deferred_run(project, ["DW-1"]) + before = project.deferred_work.read_bytes() + real_mark = deferredwork.mark_done_many + + def sigterm_after_publication(*a, **kw): + marked = real_mark(*a, **kw) # the flip is on disk now + signal.raise_signal(signal.SIGTERM) + return marked # unreachable: the handler raises first + + monkeypatch.setattr(deferredwork, "mark_done_many", sigterm_after_publication) + + summary = engine.run() + + assert summary.done == 0 + assert load_state(engine.run_dir).stopped is True # the stop really landed + assert _ledger_entries(project)["DW-1"].open + assert project.deferred_work.read_bytes() == before + kinds = {e["kind"] for e in engine.journal.entries()} + # the close never got to report itself, and the restore still happened + assert "story-deferred-closed" not in kinds + assert "deferred-close-rolled-back" in kinds + + def test_transient_spec_read_fault_does_not_crash_run(project, monkeypatch): """Integration capstone for #97. A single transient OSError on the spec — a TOCTOU truncation while the dev skill rewrites the file the orchestrator is diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index e3ccac4b..a0670fa5 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -57,7 +57,9 @@ def commit_sprint(project, statuses: dict[str, str]) -> None: git(project.project, "commit", "-q", "-m", "sprint") -def wt_dev_effect(project, story_key, *, final_status="done", followup_review=True): +def wt_dev_effect( + project, story_key, *, final_status="done", followup_review=True, closes_deferred=None +): """Dev session running inside the unit worktree (spec.cwd). Mirrors the bmad-dev-auto skill: self-finalizes the spec to done, never writes the sprint board (the orchestrator advances it via the B2 seam, inside the worktree). @@ -71,7 +73,7 @@ def effect(spec): src = cwd / "src.txt" src.write_text(src.read_text() + f"change for {story_key}\n") sp = wt.implementation_artifacts / f"spec-{story_key}.md" - write_spec(sp, final_status, baseline) + write_spec(sp, final_status, baseline, closes_deferred=closes_deferred) # NO set_sprint: the orchestrator is the single sprint-status writer return SessionResult( status="completed", @@ -1430,3 +1432,34 @@ def bad_dev(spec): src = (project.project / "src.txt").read_text() assert "change for 1-1-a" in src and "bad attempt" not in src assert worktree_clean(project.project) + + +# ------------------------------------ story-declared deferred-work closure (#234) + + +def _ledger_entry(paths, dw_id: str): + from bmad_loop import deferredwork + + text = paths.deferred_work.read_text(encoding="utf-8") + return next(e for e in deferredwork.parse_ledger(text) if e.id == dw_id) + + +def test_worktree_in_repo_ledger_closure_reaches_the_target_branch(project): + """An in-repo ledger is rebased into the unit worktree, so the closure rides + the unit's own commit and arrives on the target branch with the merge (#234).""" + from conftest import write_ledger + + commit_sprint(project, {"1-1-a": "ready-for-dev"}) + write_ledger(project, {"DW-1": "open"}) + engine, _ = make_engine( + project, + [wt_dev_effect(project, "1-1-a", followup_review=False, closes_deferred=["DW-1"])], + ) + + summary = engine.run() + + assert summary.done == 1 + entry = _ledger_entry(project, "DW-1") + assert entry.status.startswith("done") and not entry.open + assert "resolution: resolved by story 1-1-a" in entry.body + assert worktree_clean(project.project) diff --git a/tests/test_platform_util.py b/tests/test_platform_util.py index fbacc61a..07a0b5eb 100644 --- a/tests/test_platform_util.py +++ b/tests/test_platform_util.py @@ -8,6 +8,7 @@ from __future__ import annotations import os +import stat import subprocess import sys @@ -131,6 +132,142 @@ def denied(src, dst): assert sleeps == [] # zero backoff — a real POSIX error surfaces at once +# ------------------------------------------------------------- atomic_write_text + + +def test_atomic_write_text_replaces_contents(tmp_path): + target = tmp_path / "ledger.md" + target.write_text("before", encoding="utf-8") + + platform_util.atomic_write_text(target, "after") + + assert target.read_text(encoding="utf-8") == "after" + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits") +def test_atomic_write_text_preserves_the_target_mode(tmp_path): + """`os.replace` swaps in a NEW inode, so a naive tmp-write-and-replace resets + the file's permissions to the umask default — silently widening a 0600 ledger + to world-readable, or dropping group-write on a shared artifact dir.""" + target = tmp_path / "ledger.md" + target.write_text("before", encoding="utf-8") + target.chmod(0o600) + + platform_util.atomic_write_text(target, "after") + + assert stat.S_IMODE(target.stat().st_mode) == 0o600 + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_atomic_write_text_writes_through_a_symlink(tmp_path): + """Replacing the LINK would turn it into a regular file and orphan the real + ledger, so the link is resolved first and the target is what gets rewritten.""" + real = tmp_path / "real-ledger.md" + real.write_text("before", encoding="utf-8") + link = tmp_path / "ledger.md" + link.symlink_to(real) + + platform_util.atomic_write_text(link, "after") + + assert link.is_symlink() + assert real.read_text(encoding="utf-8") == "after" + + +def test_atomic_write_text_preserves_extended_attributes(tmp_path): + """`os.replace` swaps a fresh inode into place, so anything carried by the old + inode rather than by its name is silently reset — xattrs included, which on a + ledger is where an SELinux label or a backup tool's marker lives. Deleting + `_copy_xattrs` left every other test green (#284 follow-up review, finding 8). + + Skipped where the platform or filesystem has no user xattrs (Windows, macOS's + different API, tmpfs mounted `nouser_xattr`) — the helper is best-effort by + design and must stay silent there, which the write below also proves.""" + target = tmp_path / "ledger.md" + target.write_text("before", encoding="utf-8") + setxattr = getattr(os, "setxattr", None) + if setxattr is None: + pytest.skip("no os.setxattr on this platform") + try: + setxattr(target, "user.bmad-loop-test", b"kept") + except OSError as e: + pytest.skip(f"filesystem does not support user xattrs: {e}") + + platform_util.atomic_write_text(target, "after") + + assert target.read_text(encoding="utf-8") == "after" + assert os.getxattr(target, "user.bmad-loop-test") == b"kept" + + +def test_atomic_write_text_fsyncs_before_it_publishes(tmp_path, monkeypatch): + """`os.replace` is atomic against concurrent readers, but that says nothing + about a machine losing power: closing the temp only hands its bytes to the + page cache, so the rename can be durable while the data is not, and the new + name comes back pointing at a zero-length file. An empty ledger *parses* — as + no entries — so the failure reads as every hand-written entry having vanished + rather than as corruption (CodeRabbit, review round 3).""" + order = [] + real_fsync, real_replace = os.fsync, os.replace + monkeypatch.setattr( + platform_util.os, "fsync", lambda fd: (order.append("fsync"), real_fsync(fd))[1] + ) + monkeypatch.setattr( + platform_util.os, "replace", lambda s, d: (order.append("replace"), real_replace(s, d))[1] + ) + target = tmp_path / "ledger.md" + target.write_text("before", encoding="utf-8") + + platform_util.atomic_write_text(target, "after") + + assert order == ["fsync", "replace"] # the sync must precede the publish + assert target.read_text(encoding="utf-8") == "after" + + +def test_atomic_write_text_leaves_no_temp_behind(tmp_path): + target = tmp_path / "ledger.md" + target.write_text("before", encoding="utf-8") + + platform_util.atomic_write_text(target, "after") + + assert [p.name for p in tmp_path.iterdir()] == ["ledger.md"] + + +def test_atomic_write_text_cleans_up_and_keeps_the_original_on_failure(tmp_path, monkeypatch): + target = tmp_path / "ledger.md" + target.write_text("before", encoding="utf-8") + + def boom(src, dst): + raise OSError("disk full") + + monkeypatch.setattr(platform_util.os, "replace", boom) + + with pytest.raises(OSError): + platform_util.atomic_write_text(target, "after") + + assert target.read_text(encoding="utf-8") == "before" + assert [p.name for p in tmp_path.iterdir()] == ["ledger.md"] # no orphaned temp + + +def test_atomic_write_text_temp_name_is_unique_per_call(tmp_path, monkeypatch): + """A fixed `.tmp` sibling is a collision between two writers of the same + file — the second clobbers the first's staged content and one replace lands + a half-written mix.""" + target = tmp_path / "ledger.md" + target.write_text("before", encoding="utf-8") + seen: list[str] = [] + real_replace = os.replace + + def record(src, dst): + seen.append(os.path.basename(src)) + real_replace(src, dst) + + monkeypatch.setattr(platform_util.os, "replace", record) + + platform_util.atomic_write_text(target, "a") + platform_util.atomic_write_text(target, "b") + + assert len(set(seen)) == 2 + + # --------------------------------------------------------------- retrying_unlink diff --git a/tests/test_stories.py b/tests/test_stories.py index dae10978..b943f352 100644 --- a/tests/test_stories.py +++ b/tests/test_stories.py @@ -4,6 +4,7 @@ from pathlib import Path import pytest +from conftest import fault_read_text from bmad_loop import stories @@ -67,6 +68,19 @@ def test_load_non_utf8_raises_stories_error(tmp_path): stories.load_stories(tmp_path) +def test_load_unreadable_raises_stories_error(tmp_path, monkeypatch): + """A manifest that is present but unreadable — permissions, an I/O error, a + dead mount — must be a StoriesError like every other manifest fault. `is_file` + only rules out absence, so the OSError escaped raw and crashed the run from + whichever caller reached it first, including the commit-boundary read of + `closes_deferred`, whose contract is to journal and carry on + (#284 round-5 review, finding 5).""" + write_stories(tmp_path, "- id: 1\n title: t\n description: d\n") + fault_read_text(monkeypatch, tmp_path / stories.STORIES_FILENAME) + with pytest.raises(stories.StoriesError, match="could not be read"): + stories.load_stories(tmp_path) + + def test_id_unquoted_int_normalized(tmp_path): # An LLM-authored file may emit `id: 1` unquoted (PyYAML -> int); we str()-normalize. write_stories(tmp_path, "- id: 1\n title: t\n description: d\n") @@ -217,6 +231,41 @@ def test_invoke_dev_with_non_string_rejected(tmp_path): stories.load_stories(tmp_path) +def test_closes_deferred_defaults_empty(tmp_path): + """The overwhelmingly common entry declares nothing — and an older manifest, + written before the field existed, must keep parsing unchanged (#234).""" + write_stories(tmp_path, '- id: "1"\n title: t\n description: d\n') + assert stories.load_stories(tmp_path).entries[0].closes_deferred == () + + +def test_closes_deferred_parses_ledger_ids(tmp_path): + write_stories( + tmp_path, + '- id: "1"\n title: t\n description: d\n closes_deferred: [DW-5, DW-6]\n', + ) + assert stories.load_stories(tmp_path).entries[0].closes_deferred == ("DW-5", "DW-6") + + +def test_closes_deferred_normalizes_items(tmp_path): + """Items are str()-normalized, stripped, blank-dropped, and de-duplicated, the + same leniency ids get: an LLM-authored manifest may emit a bare `5` as an int + or repeat an id, and neither is a contradiction worth failing a run over.""" + write_stories( + tmp_path, + '- id: "1"\n title: t\n description: d\n' + ' closes_deferred: [" DW-5 ", 5, "DW-5", ""]\n', + ) + assert stories.load_stories(tmp_path).entries[0].closes_deferred == ("DW-5", "5") + + +def test_closes_deferred_non_list_rejected(tmp_path): + """Strict about the container: a bare string is a schema error, never silently + wrapped — a lenient reading would iterate it into single characters.""" + write_stories(tmp_path, '- id: "1"\n title: t\n description: d\n closes_deferred: DW-5\n') + with pytest.raises(stories.StoriesError, match="must be a list of deferred-work ids"): + stories.load_stories(tmp_path) + + def test_top_level_must_be_list(tmp_path): write_stories(tmp_path, "development_status:\n a: b\n") with pytest.raises(stories.StoriesError, match="top-level list"): diff --git a/tests/test_stories_engine.py b/tests/test_stories_engine.py index 23ab58fc..0efb0769 100644 --- a/tests/test_stories_engine.py +++ b/tests/test_stories_engine.py @@ -33,7 +33,7 @@ ) from bmad_loop.runs import STOP_REQUEST_FILE, graceful_stop_requested from bmad_loop.stories_engine import StoriesEngine -from bmad_loop.verify import read_frontmatter, rev_parse_head, status_of +from bmad_loop.verify import read_frontmatter, rev_parse_head, status_of, worktree_clean QUIET = NotifyPolicy(desktop=False, file=True) SPEC_FOLDER = "_bmad-output/epic-1" # under output_folder -> excluded from proof-of-work @@ -63,11 +63,20 @@ def setup_stories(paths, entries: list[dict], *, spec_folder: str = SPEC_FOLDER) def stories_dev_effect( - *, final_status: str = "done", followup_review: bool = False, prose_status: str | None = None + *, + final_status: str = "done", + followup_review: bool = False, + prose_status: str | None = None, + closes_deferred: object = None, + write_src: bool = True, ): """Simulate a bmad-dev-auto folder+id dispatch: read the story id + spec folder from the session env (as the real adapter does), write the id-keyed - story spec, and touch real code so proof-of-work passes.""" + story spec, and touch real code so proof-of-work passes. + + ``write_src=False`` skips the code change, so the proof-of-work gate fails and + the story defers with its spec still finalized — the shape a story-declared + ledger closure must survive without claiming anything resolved.""" def effect(spec) -> SessionResult: story_id = spec.env["BMAD_LOOP_STORY_KEY"] @@ -77,8 +86,11 @@ def effect(spec) -> SessionResult: stories_dir.mkdir(parents=True, exist_ok=True) sp = stories_dir / f"{story_id}-slug.md" src = Path(spec.cwd) / "src.txt" - src.write_text(src.read_text() + f"work for {story_id}\n") - write_spec(sp, final_status, baseline, prose_status=prose_status) + if write_src: + src.write_text(src.read_text() + f"work for {story_id}\n") + write_spec( + sp, final_status, baseline, prose_status=prose_status, closes_deferred=closes_deferred + ) return SessionResult( status="completed", result_json={ @@ -1220,3 +1232,227 @@ def test_entry_for_unreadable_manifest_journals_warning_once(project): # a second call for the same story does not re-journal (dedup per story key) assert engine._entry_for(task) is None assert len(_kinds(engine.journal, "stories-manifest-unreadable")) == 1 + + +# ------------------- closes_deferred auto-resolve in stories mode (#234) ------ + + +def _entries(project) -> dict: + from bmad_loop import deferredwork + + text = project.deferred_work.read_text(encoding="utf-8") + return {e.id: e for e in deferredwork.parse_ledger(text)} + + +def _closing_run(project, *, manifest=None, spec=None, ledger=None, attempts=1, **effect_kwargs): + """A whole stories-mode run of one story, declaring `manifest` ids on its + stories.yaml entry and `spec` ids in the generated story spec, over a + committed ledger. Driven end to end: the close lives at the commit boundary, + so only a real run exercises where it actually lands. `attempts` scripts the + same session repeatedly for a story expected to retry before deferring.""" + from conftest import write_ledger + + setup_stories(project, [entry("1", **({"closes_deferred": manifest} if manifest else {}))]) + write_ledger(project, ledger or {"DW-1": "open"}) + effect = stories_dev_effect(closes_deferred=spec, **effect_kwargs) + return make_engine(project, [effect] * attempts)[0] + + +def test_stories_mode_closes_declared_deferred_entries(project): + """#234: stories mode has no sprint board, but it shares the project's + deferred-work ledger — so a story declaring `closes_deferred:` must still get + those entries annotated. Without this the field is inert in exactly the mode + whose declarations `validate` preflights.""" + engine = _closing_run(project, spec=["DW-1"]) + + summary = engine.run() + + assert summary.done == 1 + dw1 = _entries(project)["DW-1"] + assert dw1.status.startswith("done") and not dw1.open + assert "resolution: resolved by story 1" in dw1.body + closed = _kinds(engine.journal, "story-deferred-closed") + assert len(closed) == 1 and closed[0]["dw_ids"] == ["DW-1"] + + +def test_stories_mode_annotation_rides_the_story_commit(project): + """An in-repo ledger annotation is part of the story's own commit, so the run + ends on the clean tree the next story's step-01 requires.""" + engine = _closing_run(project, spec=["DW-1"]) + + engine.run() + + committed = git(project.project, "show", "HEAD", "--", str(project.deferred_work)) + assert "+resolution: resolved by story 1" in committed + assert worktree_clean(project.project) + + +def test_stories_mode_closes_nothing_when_the_story_defers(project): + """The story finalizes its spec — declaration and all — then fails the + proof-of-work gate and defers. Closing at dev-sync time made that permanent + (#234 review, finding 1); at the commit boundary the entry is never touched.""" + engine = _closing_run(project, manifest=["DW-1"], spec=["DW-1"], write_src=False, attempts=3) + + summary = engine.run() + + assert summary.deferred == 1 and summary.done == 0 + assert _entries(project)["DW-1"].open + assert not _kinds(engine.journal, "story-deferred-closed") + + +def test_stories_mode_close_is_silent_without_declaration(project): + """The ordinary story declares nothing: no ledger write, no journal noise — + and the sprint board stays untouched in a mode that has none.""" + engine = _closing_run(project) + before = project.deferred_work.read_bytes() + + engine.run() + + assert project.deferred_work.read_bytes() == before + assert not _kinds(engine.journal, "story-deferred-closed") + assert not _kinds(engine.journal, "deferred-close-unmatched") + + +def test_stories_mode_dev_sync_writes_nothing(project): + """Stories mode keeps the contract's "the orchestrator writes nothing" at dev + time: the post-dev sync is a pure no-op, and in particular does NOT close + declared entries — that belongs after verification, at commit.""" + engine = _closing_run(project, manifest=["DW-1"], spec=["DW-1"]) + before = project.deferred_work.read_bytes() + + engine._post_dev_state_sync(StoryTask(story_key="1", epic=0), {}) + + assert project.deferred_work.read_bytes() == before + assert not _kinds(engine.journal, "story-deferred-closed") + + +def test_stories_mode_closes_ids_declared_in_the_manifest(project): + """The channel that makes this work unattended (#234): `bmad-dev-auto` writes + the story spec and knows nothing of the ledger, so a frontmatter-only + declaration would have to be hand-edited into every generated spec. Declaring + on the stories.yaml entry — authored while the ledger is in view — closes the + loop with no upstream change.""" + engine = _closing_run(project, manifest=["DW-1"]) # spec declares nothing + + engine.run() + + dw1 = _entries(project)["DW-1"] + assert dw1.status.startswith("done") and "resolution: resolved by story 1" in dw1.body + assert _kinds(engine.journal, "story-deferred-closed")[0]["dw_ids"] == ["DW-1"] + + +def test_stories_mode_unreadable_manifest_at_commit_does_not_crash_the_run(project, monkeypatch): + """`_manifest_closes_deferred` advertises a fallback — journal the unreadable + manifest, carry on with the spec channel — but it catches only StoriesError, + and `load_stories` let an OSError from the read escape raw. A commit-time + permission fault therefore crashed the whole run instead of costing one + channel (#284 round-5 review, finding 5). + + The fault is transient and scoped to the close, which is the shape that + matters: a manifest unreadable for the whole run is a preflight failure, not + this.""" + engine = _closing_run(project, spec=["DW-1"]) + manifest = project.project / SPEC_FOLDER / "stories.yaml" + real_read = Path.read_text + faulted = {"on": False} + + def maybe_fault(self, *a, **kw): + if faulted["on"] and self == manifest: + raise PermissionError(13, "Permission denied") + return real_read(self, *a, **kw) + + monkeypatch.setattr(Path, "read_text", maybe_fault) + finalize = engine._finalize_commit_phase + + def unreadable_across_the_close(task): + faulted["on"] = True + try: + return finalize(task) + finally: + faulted["on"] = False + + monkeypatch.setattr(engine, "_finalize_commit_phase", unreadable_across_the_close) + + summary = engine.run() + + assert summary.done == 1 # the run survives; only the manifest channel is lost + assert not _entries(project)["DW-1"].open # the spec channel still closed it + lost = _kinds(engine.journal, "deferred-close-declaration-unreadable") + assert lost and lost[-1]["source"] == "stories.yaml" + + +def test_stories_mode_unions_manifest_and_frontmatter_declarations(project): + """Both channels are honored, and an id named in both is marked and reported + once — the natural case once a planner declares it and the spec echoes it.""" + engine = _closing_run( + project, + manifest=["DW-1", "DW-2"], + spec=["DW-2", "DW-3"], # DW-2 in both + ledger={"DW-1": "open", "DW-2": "open", "DW-3": "open"}, + ) + + engine.run() + + entries = _entries(project) + assert all(not entries[dw].open for dw in ("DW-1", "DW-2", "DW-3")) + assert entries["DW-2"].body.count("resolution: resolved by story 1") == 1 # not doubled + closed = _kinds(engine.journal, "story-deferred-closed") + assert closed[0]["dw_ids"] == ["DW-1", "DW-2", "DW-3"] # manifest first, deduped + + +def test_stories_mode_reports_a_manifest_unreadable_at_the_commit_boundary(project, monkeypatch): + """The manifest half is re-read at the commit and, unlike the spec half, is + persisted nowhere — so a parse failure there drops a declared closure with + nothing to fall back on. Routing it through `_entry_for` reported that as a + generic `stories-manifest-unreadable` warning that is emitted at most ONCE per + story, so an earlier dispatch-time failure spent the one line and the lost + closure passed in silence (#284 follow-up review, finding 4). + + The spec half is unaffected: one unreadable channel must not take the other + down with it.""" + from bmad_loop import stories as stories_mod + + engine = _closing_run( + project, + manifest=["DW-1"], + spec=["DW-2"], + ledger={"DW-1": "open", "DW-2": "open"}, + ) + finalize = engine._finalize_commit_phase + real_load = engine._load_stories + + def unreadable(*_a, **_kw): + raise stories_mod.StoriesError("stories.yaml is not valid YAML") + + def unreadable_from_the_commit_phase_on(task): + monkeypatch.setattr(engine, "_load_stories", unreadable) + try: + return finalize(task) + finally: + monkeypatch.setattr(engine, "_load_stories", real_load) + + monkeypatch.setattr(engine, "_finalize_commit_phase", unreadable_from_the_commit_phase_on) + + summary = engine.run() + + assert summary.done == 1 # never a gate + entries = _entries(project) + assert entries["DW-1"].open # the manifest-declared id is lost... + events = _kinds(engine.journal, "deferred-close-declaration-unreadable") + assert len(events) == 1 and events[0]["source"] == "stories.yaml" # ...but not silently + assert not entries["DW-2"].open # the spec half still closed + + +def test_stories_mode_unknown_id_is_journaled_not_fatal(project): + """An id naming no ledger entry — a typo, or an entry renumbered since the + breakdown was written — is journaled and dropped. The annotation is + traceability, so a stale reference must never fail a story that succeeded.""" + engine = _closing_run(project, manifest=["DW-99"]) + before = project.deferred_work.read_bytes() + + summary = engine.run() + + assert summary.done == 1 + assert project.deferred_work.read_bytes() == before + unmatched = _kinds(engine.journal, "deferred-close-unmatched") + assert len(unmatched) == 1 and unmatched[0]["dw_ids"] == ["DW-99"] From 8f72144990b15bc0fc07f30348abf4cc34fe037d Mon Sep 17 00:00:00 2001 From: pbean Date: Fri, 24 Jul 2026 23:59:41 -0700 Subject: [PATCH 2/4] feat(validate): pre-flight warnings for closes_deferred declarations (#234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bmad-loop validate` warns — never fails — in both queue modes: `deferred.closes-unknown` for an id absent from the ledger, `deferred.closes-malformed` for a spec declaration in a non-list shape, `deferred.closes-entry-unreadable` for a ledger entry whose status parses as neither open nor done, and `deferred.ledger-unreadable` for a ledger that cannot be read at all. A non-list declaration in stories.yaml itself stays a manifest schema error, reported by `queue.stories-manifest`. --- src/bmad_loop/checks.py | 4 + src/bmad_loop/cli.py | 151 ++++++++++++++++++++++++++++ tests/test_cli.py | 215 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 370 insertions(+) diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py index 033a8233..b45238c6 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -75,6 +75,10 @@ "skills.stories-dispatch", "skills.stories-dispatch-missing", "skills.stories-dispatch-stale", + "deferred.closes-unknown", + "deferred.closes-malformed", + "deferred.closes-entry-unreadable", + "deferred.ledger-unreadable", } ) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 120b97de..0527d096 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -20,6 +20,7 @@ decisions, deferredwork, envvars, + frontmatter, gates, install, machine, @@ -229,7 +230,9 @@ def cmd_validate(args: argparse.Namespace) -> int: _validate_stories_queue( project, paths, spec_folder, [p.skill_tree for p in profiles], report ) + _validate_closes_deferred(paths, report, spec_folder=spec_folder) else: + _validate_closes_deferred(paths, report) try: ss = sprintstatus.load(paths.sprint_status) actionable = [s for s in ss.stories if s.status in sprintstatus.ACTIONABLE_STATUSES] @@ -603,6 +606,154 @@ def _validate_stories_queue( report.extend(stories_probs) +def _spec_closes_deferred(path: Path) -> tuple[tuple[str, ...], str | None]: + """One story spec's ``closes_deferred:`` declaration, degrading an unreadable + spec to an empty one — other gates own spec readability, and an advisory + check must never be the thing that crashes the preflight it is advising.""" + try: + raw = frontmatter.read_frontmatter(path).get("closes_deferred") + except (OSError, UnicodeDecodeError): + return (), None + return deferredwork.parse_declaration(raw) + + +def _validate_closes_deferred( + paths: bmadconfig.ProjectPaths, + report: ValidationReport, + *, + spec_folder: str | None = None, +) -> None: + """Warn when a story declares ``closes_deferred:`` ids the deferred-work + ledger does not carry, or declares them in a shape nothing can read (#234). + + At clean close the orchestrator flips each declared id to ``status: done + `` + a ``resolution:`` line. An id that names no entry — a typo, or an + entry reworded/renumbered since the spec was written — annotates nothing, and + the run says so only in the journal, where nobody looks until the retro that + the annotation exists to spare. Saying it at preflight is the whole point: + the declaration is fixable before the run, not after. + + Runs in **both** queue modes, because the declaration exists in both. With + ``spec_folder`` (stories mode) each manifest entry is checked together with + its id-resolved spec, unioned — the manifest half is what makes this genuinely + a *pre*-flight, since those ids are readable before the story has ever been + dispatched. Without it (sprint mode) the story specs already sitting in the + artifacts dir are scanned instead; those are written by `create-story` ahead + of the run, which is exactly while a typo is still cheap to fix. Reporting + only in stories mode left sprint-mode operators with nothing but a journal + line after the fact. + + An id present but already ``done`` is a declaration a prior run already + satisfied (a resume re-drives the same close), so it stays silent — the same + classification the engine's close hook makes, for the same reason. Everything + else it can journal is reported here: an absent id, an id whose ledger entry + carries neither an ``open`` nor a ``done`` status (nothing will be marked, and + the remedy is in the ledger rather than in the declaration), and a + wrong-container declaration, which names real intent that would otherwise + close nothing and say nothing. Covering only two of the three left the third + to be discovered in the journal after the run it should have preceded. + + A ledger that cannot be read at all is reported as well. Staying quiet there + is not the same trade as staying quiet about an unparseable manifest: the + manifest is already reported by ``queue.stories-manifest``, while nothing + else in ``validate`` reads the ledger, so silence meant reporting success for + a preflight that checked nothing. + + Never a failure. The annotation is traceability, not a gate, so a stale + reference must not be able to block a run that would otherwise start. + """ + ledger = paths.deferred_work + try: + text = ledger.read_text(encoding="utf-8") if ledger.is_file() else "" + except (OSError, UnicodeDecodeError) as e: + # Split from the manifest read below, which is silent for a good reason + # that does not apply here: nothing else in `validate` reads the ledger, so + # returning quietly reported success for a preflight that checked nothing, + # against the very file the run's closure will fail on + # (#284 round-5 review, finding 6). + report.warn( + "deferred.ledger-unreadable", + f"{ledger} cannot be read ({e}) — closes_deferred declarations were not " + "checked against it, and the run's own closure will fail the same way", + {"ledger": str(ledger), "error": str(e)}, + ) + return + try: + sources = ( + _stories_declarations(paths, spec_folder) + if spec_folder is not None + else _sprint_declarations(paths) + ) + except (OSError, UnicodeDecodeError, stories_mod.StoriesError): + # An unparseable manifest is already a `queue.stories-manifest` failure from + # the gate above — don't double-report it. + return + for label, ids, error in sources: + if error: + report.warn( + "deferred.closes-malformed", + f"{label} declares closes_deferred in a shape that cannot be read: " + f"{error} — nothing will be marked resolved for it", + {"source": label, "error": error}, + ) + declared = deferredwork.classify(text, ids) + if declared.malformed: + malformed = list(declared.malformed) + report.warn( + "deferred.closes-entry-unreadable", + f"{label} declares closes_deferred ids whose {ledger.name} entries carry " + f"neither an `open` nor a `done` status: {', '.join(malformed)} — nothing " + "will be marked resolved for them until the entry status is repaired", + {"source": label, "dw_ids": malformed}, + ) + if declared.unknown: + unknown = list(declared.unknown) + report.warn( + "deferred.closes-unknown", + f"{label} declares closes_deferred ids that are not in " + f"{ledger.name}: {', '.join(unknown)} — nothing will be marked " + f"resolved for them (typo, or a renumbered/reworded entry?)", + {"source": label, "unknown_ids": unknown}, + ) + + +def _stories_declarations( + paths: bmadconfig.ProjectPaths, spec_folder: str +) -> list[tuple[str, tuple[str, ...], str | None]]: + """Per manifest entry: the union of its ``stories.yaml`` ids and its + id-resolved spec's, deduped across both channels.""" + folder = stories_mod.resolve_spec_folder(paths.project, spec_folder) + out = [] + for entry in stories_mod.load_stories(folder).entries: + ids = list(entry.closes_deferred) + error = None + state = stories_mod.resolve_story_spec(folder, entry.id) + if state.kind == stories_mod.KIND_PRESENT and state.path is not None: + spec_ids, error = _spec_closes_deferred(state.path) + ids += spec_ids + out.append((f"story {entry.id}", tuple(dict.fromkeys(ids)), error)) + return out + + +def _sprint_declarations( + paths: bmadconfig.ProjectPaths, +) -> list[tuple[str, tuple[str, ...], str | None]]: + """Sprint mode has no manifest, so the declarations that exist yet are the + ones in story specs already on disk — the flat ``*.md`` layout the artifacts + dir holds.""" + impl = paths.implementation_artifacts + if not impl.is_dir(): + return [] + out = [] + for path in sorted(impl.glob("*.md")): + if path == paths.deferred_work: + continue + ids, error = _spec_closes_deferred(path) + if ids or error: + out.append((f"spec {path.name}", ids, error)) + return out + + def _warn_unknown_keys(ss: sprintstatus.SprintStatus) -> None: """Surface sprint-status keys the parser could not classify. Silently dropping one reads to the operator as "that story is done, or not mine to diff --git a/tests/test_cli.py b/tests/test_cli.py index a92adbd8..22b5e592 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -9,10 +9,15 @@ import yaml from conftest import ( escalated_run, + fault_read_text, git, install_bmad_config, install_dev_base_skills, machine_json, + mark_ledger_done, + spec_path, + write_ledger, + write_spec, write_sprint, ) @@ -3332,6 +3337,216 @@ def test_validate_silent_when_desktop_notifier_available(project, monkeypatch, c assert all(f["check"] != "notify.desktop-unavailable" for f in doc["findings"]) +def _declare_closes_deferred(project, declared, *, ledger_ids=("DW-1",), manifest=None): + """A stories-mode fixture for one story: `manifest` ids declared on its + stories.yaml entry, `declared` ids in its spec frontmatter (None writes no spec + at all), over a ledger holding `ledger_ids` — all open.""" + over = {"closes_deferred": list(manifest)} if manifest is not None else {} + folder = _setup_stories_fixture(project, [_stories_entry("1", **over)]) + write_ledger(project, {dw: "open" for dw in ledger_ids}, commit=False) + if declared is not None: + (folder / "stories" / "1-slug.md").write_text( + f"---\ntitle: 'test'\nstatus: 'ready-for-dev'\n" + f"closes_deferred: [{', '.join(declared)}]\n---\n\n## Intent\n\ntest\n", + encoding="utf-8", + ) + return folder + + +def _closes_deferred_findings(capsys): + doc = json.loads(capsys.readouterr().out) + return [f for f in doc["findings"] if f["check"] == "deferred.closes-unknown"] + + +def test_validate_warns_on_unknown_closes_deferred(project, capsys): + """#234: a story declaring an id the ledger doesn't carry — a typo, or an entry + renumbered since the spec was written — would annotate nothing at close, and + only the journal would say so. Preflight names it while it is still fixable.""" + install_bmad_config(project) + _write_policy(project.project, STORIES_POLICY) + _declare_closes_deferred(project, ["DW-99"]) + args = argparse.Namespace(project=str(project.project), spec=None, json=True) + + cli.cmd_validate(args) # rc varies by host (binary/skills) — parse the document + findings = _closes_deferred_findings(capsys) + assert len(findings) == 1 + assert findings[0]["severity"] == "warning" # advisory: never blocks a run + assert findings[0]["detail"] == {"source": "story 1", "unknown_ids": ["DW-99"]} + assert "DW-99" in findings[0]["message"] + + +def test_validate_silent_when_closes_deferred_all_present(project, capsys): + """The mirror case: every declared id is in the ledger, so nothing is emitted. + A present entry an earlier run already closed is a *satisfied* declaration, not + a mismatch — the check keys off presence, never status, so a resumed project + doesn't accumulate warnings for work that landed.""" + install_bmad_config(project) + _write_policy(project.project, STORIES_POLICY) + _declare_closes_deferred(project, ["DW-1", "DW-2"], ledger_ids=("DW-1", "DW-2")) + mark_ledger_done(project, ["DW-2"]) # already closed by an earlier run + args = argparse.Namespace(project=str(project.project), spec=None, json=True) + + cli.cmd_validate(args) + assert _closes_deferred_findings(capsys) == [] + + +def test_validate_warns_when_a_declared_entry_status_is_unreadable(project, capsys): + """A declared id can also point at an entry the ledger carries but whose + `status:` reads as neither `open` nor `done`. Nothing gets marked for it and + the close hook journals `deferred-close-malformed`, but preflight covered only + absent ids and unreadable declarations — leaving this third case to be found in + the journal after the run it should have preceded (CodeRabbit, round 3). + + The remedy is in the ledger, not the declaration, so it is its own check id + rather than folded into `deferred.closes-unknown`.""" + install_bmad_config(project) + _write_policy(project.project, STORIES_POLICY) + _declare_closes_deferred(project, ["DW-1"]) + ledger = project.deferred_work + ledger.write_text( + ledger.read_text(encoding="utf-8").replace("status: open", "status: in-progress"), + encoding="utf-8", + ) + args = argparse.Namespace(project=str(project.project), spec=None, json=True) + + cli.cmd_validate(args) + doc = json.loads(capsys.readouterr().out) + findings = [f for f in doc["findings"] if f["check"] == "deferred.closes-entry-unreadable"] + assert len(findings) == 1 + assert findings[0]["severity"] == "warning" # advisory, like the rest + assert findings[0]["detail"] == {"source": "story 1", "dw_ids": ["DW-1"]} + # not misreported as a typo: the id IS in the ledger + assert not [f for f in doc["findings"] if f["check"] == "deferred.closes-unknown"] + + +def test_validate_warns_on_unknown_closes_deferred_in_the_manifest(project, capsys): + """The manifest channel is what makes this a genuine *pre*-flight: a stories.yaml + entry declares its ids before the story has ever been dispatched, so a typo is + caught while no spec exists yet — the frontmatter check can't see that far.""" + install_bmad_config(project) + _write_policy(project.project, STORIES_POLICY) + _declare_closes_deferred(project, None, manifest=["DW-99"]) # no story spec on disk + args = argparse.Namespace(project=str(project.project), spec=None, json=True) + + cli.cmd_validate(args) + findings = _closes_deferred_findings(capsys) + assert len(findings) == 1 + assert findings[0]["detail"] == {"source": "story 1", "unknown_ids": ["DW-99"]} + + +def test_validate_closes_deferred_dedupes_across_both_channels(project, capsys): + """An id declared in both the manifest and the spec is one mismatch, reported + once — not the same typo twice.""" + install_bmad_config(project) + _write_policy(project.project, STORIES_POLICY) + _declare_closes_deferred(project, ["DW-99"], manifest=["DW-99"]) + args = argparse.Namespace(project=str(project.project), spec=None, json=True) + + cli.cmd_validate(args) + findings = _closes_deferred_findings(capsys) + assert len(findings) == 1 + assert findings[0]["detail"]["unknown_ids"] == ["DW-99"] # once, not twice + + +def test_validate_closes_deferred_warning_does_not_fail_the_run(project, capsys, monkeypatch): + """The warning must not flip the exit code. An otherwise-clean project carrying + a stale declaration still validates ok/rc 0 — a typo in a traceability field can + never be the thing that blocks a run from starting.""" + _make_validate_pass(project, monkeypatch, capsys) + _write_policy(project.project, CLAUDE_ONLY_POLICY + STORIES_POLICY) # same, stories mode + _declare_closes_deferred(project, ["DW-99"]) + git(project.project, "add", "-A") # the fixture above dirties the worktree gate + git(project.project, "commit", "-q", "-m", "stories fixture") + capsys.readouterr() + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) # rc 0 + assert doc["ok"] is True + assert doc["counts"]["problem"] == 0 + assert [f for f in doc["findings"] if f["check"] == "deferred.closes-unknown"] + + +def test_validate_warns_on_unknown_closes_deferred_in_sprint_mode(project, capsys): + """The check runs in BOTH queue modes (#234 review, finding 6). Sprint mode + has no manifest, but `create-story` writes its story specs into the artifacts + dir before the run — so a typo there is caught at preflight too, rather than + surfacing only as a journal line nobody reads until the retro.""" + install_bmad_config(project) + _write_policy(project.project) # sprint mode: no [stories] source + write_sprint(project, {"1-1-a": "ready-for-dev"}) + write_ledger(project, {"DW-1": "open"}, commit=False) + write_spec(spec_path(project, "1-1-a"), "ready-for-dev", "abc123", closes_deferred=["DW-99"]) + args = argparse.Namespace(project=str(project.project), spec=None, json=True) + + cli.cmd_validate(args) + + findings = _closes_deferred_findings(capsys) + assert len(findings) == 1 + assert findings[0]["severity"] == "warning" + assert findings[0]["detail"] == {"source": "spec spec-1-1-a.md", "unknown_ids": ["DW-99"]} + + +def test_validate_warns_when_the_ledger_itself_is_unreadable(project, capsys, monkeypatch): + """The ledger read shared a `try` with the manifest read, and that arm returns + silently — correctly for the manifest, which `queue.stories-manifest` already + reports, but nothing else in `validate` reads the ledger. So an unreadable one + produced no finding at all: preflight reported success for a check that + examined nothing, against the very file the run's closure will fail on + (#284 round-5 review, finding 6).""" + install_bmad_config(project) + _write_policy(project.project) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + write_ledger(project, {"DW-1": "open"}, commit=False) + write_spec(spec_path(project, "1-1-a"), "ready-for-dev", "abc123", closes_deferred=["DW-1"]) + fault_read_text(monkeypatch, project.deferred_work) + args = argparse.Namespace(project=str(project.project), spec=None, json=True) + + cli.cmd_validate(args) + + doc = json.loads(capsys.readouterr().out) + findings = [f for f in doc["findings"] if f["check"] == "deferred.ledger-unreadable"] + assert len(findings) == 1 + assert findings[0]["severity"] == "warning" # advisory: still never a gate + assert findings[0]["detail"]["ledger"] == str(project.deferred_work) + # and the declaration checks it could not run stay quiet rather than guessing + assert not [f for f in doc["findings"] if f["check"] == "deferred.closes-unknown"] + + +def test_validate_warns_on_a_malformed_closes_deferred_declaration(project, capsys): + """`closes_deferred: DW-1` (a scalar, not a list) declares real intent that + reads as empty. Silence there was indistinguishable from having no + declaration at all, so the same mistake meant nothing in a spec and a hard + schema error in stories.yaml — preflight now names it either way.""" + install_bmad_config(project) + _write_policy(project.project) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + write_ledger(project, {"DW-1": "open"}, commit=False) + write_spec(spec_path(project, "1-1-a"), "ready-for-dev", "abc123", closes_deferred="DW-1") + args = argparse.Namespace(project=str(project.project), spec=None, json=True) + + cli.cmd_validate(args) + + doc = json.loads(capsys.readouterr().out) + findings = [f for f in doc["findings"] if f["check"] == "deferred.closes-malformed"] + assert len(findings) == 1 and findings[0]["severity"] == "warning" + assert "must be a list" in findings[0]["detail"]["error"] + + +def test_validate_sprint_mode_silent_without_declarations(project, capsys): + """The ordinary sprint project declares nothing: scanning the artifacts dir + must add no findings at all.""" + install_bmad_config(project) + _write_policy(project.project) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + write_ledger(project, {"DW-1": "open"}, commit=False) + write_spec(spec_path(project, "1-1-a"), "ready-for-dev", "abc123") + args = argparse.Namespace(project=str(project.project), spec=None, json=True) + + cli.cmd_validate(args) + + doc = json.loads(capsys.readouterr().out) + assert [f for f in doc["findings"] if f["check"].startswith("deferred.closes")] == [] + + OPENCODE_QUALIFIED_POLICY = '[adapter]\nname = "opencode"\nmodel = "anthropic/claude-haiku-4-5"\n' From c8b8775c01f5bcbdb0939765fc499a2c74286a7b Mon Sep 17 00:00:00 2001 From: pbean Date: Fri, 24 Jul 2026 23:59:41 -0700 Subject: [PATCH 3/4] docs: document story-declared deferred-work closure (#234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README (the stories.yaml field + the sweep section), the FEATURES bullet, the sweep skill's deferred-work-format.md, and a CHANGELOG entry — each claiming the advisory contract only: declared, written at the commit boundary, riding the story's own commit when the ledger is in-repo, journaled and never fatal everywhere else. --- CHANGELOG.md | 25 ++++++++ README.md | 31 +++++++++- docs/FEATURES.md | 1 + .../bmad-loop-sweep/deferred-work-format.md | 60 +++++++++++++++++++ 4 files changed, 116 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcad42f3..80e9e111 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,31 @@ breaking changes may land in a minor release. ### Added +- **Stories can close deferred-work entries (#234).** The ledger was one-way: entries are filed + automatically but were only ever marked resolved by hand. A story can now declare the entries + its work closes — `closes_deferred: [DW-5, DW-6]`, on its `stories.yaml` entry (stories mode) + or in the story spec's frontmatter, the two unioned — and when the story commits, the + orchestrator flips each declared entry to `status: done ` + `resolution: resolved by +story `, the same annotation a sweep bundle writes. Both sprint and stories mode. + + Advisory by contract: the annotation is written at the commit boundary — behind every verify + gate, checkpoint and review cycle, just before the squash — so an in-repo ledger carries it in + the story's own commit (worktree isolation included) and a story that fails, is rejected by + review, or escalates closes nothing; a commit that then fails takes the annotation back with + it. The declaration is re-read at that boundary, so a late edit counts in both directions, and + an id already `done` is a silent no-op across a resume. Nothing about it can fail a story: + unknown ids, duplicate ids, unreadable entry statuses, an unreadable ledger location and a + non-list spec declaration are journaled warnings (a non-list `closes_deferred` in `stories.yaml` + is a manifest schema error like any other). A ledger outside the repo is written at the same + moment; its annotation is part of no commit, which is journaled + (`deferred-close-external-ledger`). + + `bmad-loop validate` warns up front in both queue modes (`deferred.closes-unknown`, + `deferred.closes-malformed`, `deferred.closes-entry-unreadable`, `deferred.ledger-unreadable`); + warnings never change the exit code. The field is human-authored, like `spec_checkpoint` — no + upstream skill emits it yet (BMAD-METHOD#2619 proposes it at breakdown), and re-deriving + `stories.yaml` drops it unless the intent is logged in `.memlog.md`. + - **`review.on_timeout` policy knob (#271).** A timeout-like review verdict (`timeout` / `stalled` / `over_budget`) previously always burned a review cycle (RETRY) until `max_review_cycles`, then deferred — even when the dev product was already finalized and diff --git a/README.md b/README.md index 28e4ec5c..56ce37a6 100644 --- a/README.md +++ b/README.md @@ -173,7 +173,20 @@ Turn it on per project with `[stories] source = "stories"` + `spec_folder = "` ledger ids that story's work closes (see [Deferred-work sweeps](#deferred-work-sweeps)): + +```yaml +- id: '3-2' + title: Export digests + description: … + closes_deferred: [DW-5, DW-6] +``` + +Declaring it here rather than in the story spec is what lets the annotation happen without touching each generated spec: `bmad-dev-auto` generates the spec and knows nothing of the ledger, whereas the breakdown is authored while the ledger is in view. A spec that _does_ carry the field in frontmatter is honored too — the two are unioned. + +Like `spec_checkpoint` and `done_checkpoint`, this is a **human-authored, caller-only field**. Breakdown time, with the ledger open, is where it belongs — but it is not a deadline: the declaration is read when the story commits, so adding one to a story spec's frontmatter mid-run is honored, and withdrawing one before the commit means it is not closed. Upstream Story Breakdown does not emit it yet ([BMAD-METHOD#2619](https://github.com/bmad-code-org/BMAD-METHOD/issues/2619)), and re-deriving `stories.yaml` rewrites the file — so record the intent in `.memlog.md` alongside the story, or the declaration is lost on the next re-derive. + +A story may set both checkpoints (it pauses twice); `gates.mode` pauses stack on top. A blocked story escalates exactly as in sprint mode — `bmad-loop resolve`, then re-arm + resume — now with the story's title/description and the blocking condition surfaced; a pre-planning-halt **sentinel** spec is auto-deleted (a copy preserved under the run dir) on re-arm for a clean re-dispatch. `bmad-loop run --dry-run --spec ` prints the linear schedule (list order, checkpoint markers, live on-disk state); `bmad-loop status` shows the same stories board. @@ -235,6 +248,22 @@ bmad-loop sweep [--no-prompt] [--decisions-only] [--max-bundles N] [--repeat] [- `status: done` in the ledger. ``` +**Closing entries from a story.** Sweeps are not the only way an entry gets closed. A story spec can declare the entries its work closes, in frontmatter: + +```yaml +closes_deferred: [DW-5, DW-6] # DW- ids this story closes +``` + +In stories mode the same declaration can live on the `stories.yaml` entry instead (`closes_deferred: [DW-5, DW-6]`), which is where it belongs for an unattended run — the breakdown is authored while the ledger is in view, whereas the story spec is generated later by a dev skill that knows nothing of the ledger. Both channels are unioned. Either way the field is human-authored — like `spec_checkpoint` / `done_checkpoint`, no upstream skill emits it yet, and re-deriving `stories.yaml` drops it unless the intent is logged in `.memlog.md`. + +The orchestrator writes the same annotation a bundle close writes — `status: done ` and `resolution: resolved by story ` — into each declared entry. Without it the ledger is one-way: entries are filed automatically but only ever marked resolved by hand, so a multi-epic run ends with entries that were satisfied epics ago still reading `open`. + +**Closure happens at the commit boundary**, after artifact verification, the verify commands, every checkpoint, the review loop and the pre-commit workflows — and just before the story's commit is squashed, so an in-repo ledger carries the annotation in that same commit. A story that fails, blocks, is rejected by review, or escalates closes nothing, and there is nothing to undo. Closure is declared, never inferred from the diff; a resume re-driving the same close is a no-op; and an id that matches no entry is journaled rather than failing the story. The declaration is re-read at that boundary, so an edit made after the story was implemented is the one that counts. + +A declaration in a shape nothing can read — a bare `closes_deferred: DW-5` where a list belongs — depends on which channel it is in. In a **story spec** it is journaled and dropped, like an unknown id: the spec is generated mid-run by a dev skill, and a malformed field there must not be able to fail a story that succeeded. In **`stories.yaml`** it is a schema error, exactly like every other manifest field of the wrong type, and the manifest fails to load — the breakdown is hand-authored before the run, where `bmad-loop validate` reports it up front and a typo is still cheap to fix. `validate` warns about unknown ids in both queue modes and about a malformed spec declaration; a malformed manifest is the manifest's own `queue.stories-manifest` failure. + +> **Ledger outside the repo.** If `implementation_artifacts` is configured outside the project tree, the ledger is shared between worktrees and cannot be part of any commit. The annotation is written all the same, at the same moment, and the run journals `deferred-close-external-ledger` so its absence from git history is not a surprise. A location that cannot be read or written when the write comes due (a shared mount that has gone away) closes nothing and is journaled — those entries stay `open` for a sweep to re-verify, and an outage is never read as "no such entries". + **Answering missed decisions later.** An unattended sweep (`--no-prompt`) skips decisions, and an interactive one can be abandoned before you answer them all — those answers would otherwise be lost, since triage re-derives the decision set from the ledger every run. `bmad-loop decisions` (or press `d` in the TUI) surfaces every decision past sweeps left unanswered, reconstructed from their triage output, and lets you answer them out of band. A `close` is applied immediately; a `build`/`keep-open` is saved to `.bmad-loop/decisions.json` and consumed by the next sweep (build → bundle, keep-open → recorded) with no re-prompt. `--list` shows them without answering; `bmad-loop status` reports the outstanding count. Sweeps are their own resumable runs (`bmad-loop resume `). `[sweep] auto` in the policy fires an unattended sweep automatically at epic boundaries or run end; a failed/paused child sweep never interrupts the parent run. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index d66e4e80..53e8da5e 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -104,6 +104,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Deferred-work sweeps - Skills accumulate an append-only ledger (`deferred-work.md`, `DW-` entries): split-off goals, pre-existing findings, "needs human decision" items. +- Story-declared closure (`closes_deferred: [DW-5, DW-6]`, human-authored on a `stories.yaml` entry or in a story spec's frontmatter — the two are unioned): when the story commits, the orchestrator flips each declared entry to `status: done ` + `resolution: resolved by story ` — the same annotation a sweep bundle writes — so the ledger stops being one-way (filed automatically, marked resolved by hand). Written at the commit boundary, after verification/review/checkpoints and just before the squash, so an in-repo ledger carries the annotation in the story's own commit and a story that fails, is rejected by review, or escalates closes nothing. Declared, never inferred from a diff; re-read at the commit so a declaration edited after implementation still counts; idempotent across a resume; an unknown id, an unreadable entry status, or a non-list declaration in a story spec is journaled, never fatal (a non-list declaration in `stories.yaml` is a manifest schema error like any other, caught before the run). `bmad-loop validate` warns about all of those before the run starts, in both queue modes. An artifact dir configured outside the repo is shared across worktrees and cannot be committed — the annotation is written all the same and journaled (`deferred-close-external-ledger`). - `bmad-loop sweep` triages every open entry against the actual code (ledger statuses treated as unreliable) → partition: already-resolved (auto-closed with evidence) / bundles / blocked / skip / decisions. - Bundles run the full pipeline (dev `--dw-bundle` → review → verify → commit); the review gate checks every bundle entry is `status: done`. - Interactive decision walkthrough (build / close / keep-open per option, with a recommendation); answers written back as `decision:` lines. Unattended runs leave decisions open. diff --git a/src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md b/src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md index 98685173..e84f89c0 100644 --- a/src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md +++ b/src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md @@ -59,3 +59,63 @@ decision: — writes it when closing entries triage proved already resolved. - `decision:` records a human's sweep-time choice on an entry. It does not by itself change `status:` — a `keep-open` decision leaves the entry open. + +## Closure declared by a story + +A sweep bundle is not the only thing that closes an entry. A regular story may +declare the entries its work closes — on its `stories.yaml` entry (stories mode), +or in its story spec's frontmatter. The two are unioned: + +```yaml +closes_deferred: [DW-5, DW-6] # DW- ids this story closes +``` + +Both are written by a human, and breakdown time — with this file open — is where +it belongs, though not a deadline: the declaration is read when the story +commits, so one added to a spec's frontmatter mid-run still counts. No upstream +skill emits the field yet, and re-deriving `stories.yaml` will drop it unless the +intent is recorded in `.memlog.md` first. + +When the story commits, the orchestrator annotates each declared id exactly as a +bundle close does — `status: done ` plus a `resolution:` line naming the +story: + +```markdown +status: done 2026-07-23 +resolution: resolved by story 3-2-export +``` + +The rules that keep this safe: + +- **Declared, never inferred.** Closure comes only from this field; the + orchestrator does not guess it from a diff. +- **Only once the story actually lands.** The annotation is written at the + commit boundary — after verification, the review loop and every checkpoint, + and just before the story's commit is squashed. A story that fails, blocks, is + rejected by review, or escalates closes nothing; a commit that then fails + takes the annotation back with it, restored to the pre-close text. +- **In the story's own commit**, when this file lives inside the repo — + worktree isolation included: the unit's copy rides the unit commit and + reaches the target branch with the merge. If the artifacts dir is configured + outside the repo, the file is shared between worktrees and no commit can + carry it; the annotation is written all the same and the run journals + `deferred-close-external-ledger`. A location that cannot be read or written + when the write comes due closes nothing and is journaled — the entries stay + `open` for a sweep to re-verify, and an outage is never read as "no such + entries", never allowed to fail the story or crash the run. +- **Idempotent.** An id already `done` is left untouched, so a resumed run + re-driving the same close neither doubles the `resolution:` line nor warns. +- **Never a gate.** An id that matches no entry, an entry whose `status:` reads + as neither `open` nor `done`, and a story spec declaring a bare + `closes_deferred: DW-5` where a list belongs are each journaled and dropped — + none can fail the story. `bmad-loop validate` reports the same mismatches as + warnings before the run starts. The one exception is that same wrong container + in `stories.yaml`: the manifest is a schema the parser owns, so it fails to + load there like any other field of the wrong type — before any story runs, and + reported by `validate` up front. +- **Read at the commit.** The declaration that counts is the one on disk when the + story commits, not the one it was implemented from — edit it late and the edit + is honored, in both directions. + +Keep the ids stable when editing this file: a reworded title is fine, but +renumbering an entry orphans any declaration that already references it. From 6bfeb6d767b9d1e5341d8ce4c1797edd6c34c4ad Mon Sep 17 00:00:00 2001 From: pbean Date: Sat, 25 Jul 2026 01:23:32 -0700 Subject: [PATCH 4/4] fix(engine): keep a failed ledger rollback from displacing the commit failure (#300 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_restore_deferred_closes` runs inside the commit window's except arms, and its docstring promises a failed restore is "journaled, never raised — this runs inside except arms whose exception must travel unchanged". `except OSError` honored that for one exception type. `atomic_write_text` resolves the path before its own try, and below 3.13 `Path.resolve` reports a symlink loop as `RuntimeError` — for a ledger the helper explicitly supports being a symlink, and whose other resolve (`_ledger_in_repo`) already catches that type. 3.11 and 3.12 are both CI legs. Escaping, it displaced the exception each arm exists to carry: the `GitError` arm skipped `_escalate`, stranding the story in COMMITTING with the commit diagnosis nowhere on the record, and the `BaseException` arm skipped its bare `raise`, swapping a graceful `RunStopped` for a write complaint. The ablation is unambiguous — reverting the width returns `escalated=0, paused=False, crashed=True, crash_error="RuntimeError: Symlink loop..."`. `Exception`, not `BaseException`: `RunStopped` is an `Exception`, so a second stop landing inside the restore is absorbed while the first still travels, and a genuine KeyboardInterrupt still gets out. The journal line now names the type the way `_journal_ledger_unavailable` does — with every type admitted, a bare message cannot say whether the disk or the path was at fault. `deferred-close-rollback-failed` had no coverage at any width; the new test is the branch's first. The fault is injected rather than built from a real symlink loop, which 3.13+ resolves without raising — that version would pass here and fail only on the 3.11/3.12 legs. --- src/bmad_loop/engine.py | 20 ++++++++++++++++--- tests/test_engine.py | 44 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index d636ee48..7afbf82b 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -2250,19 +2250,33 @@ def _restore_deferred_closes(self, task: StoryTask, snapshot: list[tuple[Path, s Advisory itself, twice over: a failed restore is journaled, never raised, and the journaling is suppressed rather than allowed to become the crash — this runs inside except arms whose exception must travel - unchanged.""" + unchanged. Skipping the `raise` would replace a `RunStopped` with a + symlink complaint; skipping the `GitError` arm's `_escalate` would + strand the story in COMMITTING with no diagnosis on the record. + + The guard is type-agnostic on purpose, and `OSError` is not wide enough + to hold it: `atomic_write_text` resolves the path before its own try, + and below 3.13 `Path.resolve` reports a symlink loop as `RuntimeError` + — the same non-OSError `_ledger_in_repo` already catches for this very + path. Catching `Exception` and not `BaseException` is the other half: + `RunStopped` is an `Exception`, so a second stop signal landing inside + the restore is absorbed while the first still travels, and a genuine + KeyboardInterrupt still gets out.""" if not snapshot: return ledger, before = snapshot[-1] try: atomic_write_text(ledger, before) - except OSError as e: + except Exception as e: with contextlib.suppress(Exception): self.journal.append( "deferred-close-rollback-failed", story_key=task.story_key, ledger=str(ledger), - error=str(e), + # named, not bare `str(e)`: now that every type is admitted, + # a message alone cannot say whether the disk or the path + # was the fault. Matches `_journal_ledger_unavailable`. + error=f"{e.__class__.__name__}: {e}", ) return with contextlib.suppress(Exception): diff --git a/tests/test_engine.py b/tests/test_engine.py index 70c8e735..48e12198 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2481,6 +2481,50 @@ def sigterm_after_publication(*a, **kw): assert "deferred-close-rolled-back" in kinds +def test_failed_rollback_does_not_displace_the_commit_failure(project, monkeypatch): + """The restore runs INSIDE the commit window's except arms, so anything it + raises replaces the exception those arms exist to carry: the `GitError` arm + would skip `_escalate` and strand the story in COMMITTING with no diagnosis, + and the `BaseException` arm would skip its bare `raise` and swap a graceful + `RunStopped` for a write complaint. + + `OSError` was too narrow to hold that. `atomic_write_text` resolves the path + before its own try, and below 3.13 `Path.resolve` reports a symlink loop as + `RuntimeError` — for a ledger the helper explicitly supports being a symlink, + and whose OTHER resolve (`_ledger_in_repo`) already catches that type. + + The fault is injected rather than built from a real symlink loop on purpose: + 3.13+ resolves loops without raising, so a loop-based version would pass on + the interpreter this suite usually runs and only ever fail on the 3.11/3.12 + legs — green here, red in CI, for a guard that was never exercised.""" + engine = _closes_deferred_run(project, ["DW-1"]) + _reject_commits(project) # the commit fails, so the restore is reached + + def unresolvable(*a, **kw): + raise RuntimeError("Symlink loop from '/w/deferred-work.md'") + + # patched as bound in `engine` (its sole call site is the restore), NOT in + # `deferredwork` — the forward close must still publish, or there would be + # nothing for the rollback to fail at. + monkeypatch.setattr("bmad_loop.engine.atomic_write_text", unresolvable) + + summary = engine.run() + + # the disposition is the failed COMMIT's, not the failed rollback's + assert summary.paused and summary.escalated == 1 and not summary.crashed + assert load_state(engine.run_dir).tasks["1-1-a"].phase == Phase.ESCALATED + reasons = [e["reason"] for e in engine.journal.entries() if e["kind"] == "story-escalated"] + assert len(reasons) == 1 and reasons[0].startswith("commit failed:") + # and the rollback's own failure is on the record, naming the type — `str(e)` + # alone cannot say whether the disk or the path was at fault + failed = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-rollback-failed"] + assert len(failed) == 1 and failed[0]["error"].startswith("RuntimeError: ") + # honest about what did NOT happen: the restore never landed, so the entry + # still reads `done` for a commit that does not exist. Advisory, journaled, + # human-attended — not silently papered over. + assert not _ledger_entries(project)["DW-1"].open + + def test_transient_spec_read_fault_does_not_crash_run(project, monkeypatch): """Integration capstone for #97. A single transient OSError on the spec — a TOCTOU truncation while the dev skill rewrites the file the orchestrator is