diff --git a/CHANGELOG.md b/CHANGELOG.md index 15906aa5..24be5a90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,58 @@ breaking changes may land in a minor release. ### Added +- **Stories can close deferred-work entries (#234).** The ledger was one-way: the loop reliably + _files_ `deferred-work.md` entries but never _marked_ one resolved when a later story closed it, + so a multi-epic run ended with entries satisfied epics ago still reading `open` and the retro + reconstructing by hand which story closed what. A story spec 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 writes the same annotation a sweep bundle writes: `status: done ` plus + `resolution: resolved by story `. It fires in both sprint and stories mode. + + The write sits at the **commit boundary** — after artifact verification, the verify commands, + every checkpoint, the review loop and the `pre_commit` workflows, and immediately before + `finalize_commit` squashes the story — so a story that fails verification, is rejected by + review, or escalates closes nothing, and an in-repo ledger still carries the annotation in the + story's own commit. A commit that then fails (a rejecting native `pre-commit` hook) or never + happens at all (a SIGTERM landing mid-commit) rolls the annotation back, so it never outlives the + commit it was written for; the rollback reverts only the entries that story flipped, so a hook + that edited the ledger before refusing the commit keeps its own edit. An artifact dir configured _outside_ the repo — including one reached + through a symlink that only looks in-repo — is shared between worktrees and cannot be committed + at all; that closure is held until the work is durably landed — after the commit in place, after + the branch merges under isolation (journaled `deferred-close-external-ledger`) — and a write that + never got to happen is retried on the next resume rather than being dropped. A ledger location + that cannot be read or written at all — a dropped mount, a broken symlink, undecodable bytes — is + journaled and retried, never read as "no such entries" and never able to fail the story or crash + the run that owns it. + + Closure is declared, never inferred from a diff, and both channels are re-read at that boundary + so a declaration edited after the story was implemented is the one that counts — a withdrawn id + is not closed, and one added late is. A read that fails there falls back to the declaration + captured when the dev artifacts verified rather than to "declares nothing" + (`deferred-close-declaration-unreadable` when even that is unavailable). An id already `done` is + a silent no-op, so a resumed run re-driving the same close neither doubles the `resolution:` line + nor warns. Nothing at the commit boundary can fail a story: an id matching no ledger entry + (`deferred-close-unmatched`), a ledger entry whose `status:` reads as neither `open` nor `done` + (`deferred-close-malformed`), and a story spec declaring `closes_deferred: DW-5` where a list + belongs are journaled and dropped. The same wrong container in `stories.yaml` is a schema error + like any other manifest field of the wrong type — the manifest fails to load, before any story + runs, where `validate` has already reported it. Closes are journaled as `story-deferred-closed`. + + The `stories.yaml` channel is the one that avoids hand-editing each generated spec: + `bmad-dev-auto` writes the story spec and knows nothing of the ledger, whereas the Story + Breakdown is authored while the ledger is in view. Like `spec_checkpoint` / `done_checkpoint` + it is human-authored — no upstream skill emits it yet (BMAD-METHOD#2619 proposes the schema row + and the breakdown prompt), and re-deriving `stories.yaml` drops it unless the intent is logged + in `.memlog.md`. + + `bmad-loop validate` adds matching pre-flight warnings in **both** queue modes — + `deferred.closes-unknown` for an id absent from the ledger (a typo or a renumbered entry) and + `deferred.closes-malformed` for a spec declaration in an unreadable shape. Stories mode reads the + manifest plus each id-resolved spec; sprint mode reads the story specs already on disk in the + artifacts dir. Both are warnings, so neither changes the exit code; a malformed declaration in + `stories.yaml` itself is reported by `queue.stories-manifest`, which does. + - **`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 715d9419..31c13235 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. Closure still happens, but for an isolated (`scm.isolation = "worktree"`) run it is held until the unit's branch has merged — a story whose integration fails leaves its entries `open`. The run journals `deferred-close-external-ledger` so the annotation's absence from git history is not a surprise. If the location itself is unavailable when the write comes due (a shared mount that has gone away), the closure is retried before the run ends and on any later resume; a run that finishes with it still unreachable leaves those entries `open` and journals `deferred-close-abandoned` for a sweep to re-verify. + **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 f73edd02..601c08dd 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 — an isolated run holds its closure until the story's branch merges. - `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/checks.py b/src/bmad_loop/checks.py index 2ffc2feb..7ffea0f3 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -71,6 +71,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 e8a2ac63..55558cc7 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] @@ -592,6 +595,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/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..916966d3 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,69 @@ 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. If the commit itself then + fails — a rejecting native `pre-commit` hook, say — the annotation is rolled + back rather than left claiming work that is in no commit. The rollback reverts + only the entries that story flipped, so a hook that edited this file before + refusing the commit keeps its own edit. +- **In the story's own commit**, when this file lives inside the repo. If the + artifacts dir is configured outside it, the file is shared between worktrees + and no commit can carry it; the closure is then held until the work is durably + landed — after the commit in place, after the branch has merged under worktree + isolation. A write that could not happen — the location is on a mount that is + gone — is retried once more before the run ends, and on the next resume while + the run is still resumable. A run that _finishes_ with the location still + unavailable leaves those entries `open` and says so + (`deferred-close-abandoned`); a sweep re-verifies them against the codebase. + The closure never holds a completed run open — nor crashes one: a ledger that + cannot be read or written is journaled and retried, never allowed to fail the + story or the run it belongs to. +- **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. diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index 6c90aaf3..6e4e8ddc 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,81 @@ 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 restore_entries(current: str, before: str, dw_ids: Sequence[str]) -> tuple[str, list[str]]: + """Put just the named entries back the way `before` had them, inside whatever + `current` says now. Returns the rewritten text plus the ids that could not be + restored, in the order given. + + This is the undo half of :func:`mark_done_many`, and it is deliberately + narrower than "write `before` back". A rollback runs after a commit failed, + and the thing that failed the commit can be a native `pre-commit` hook that + edited this same ledger before rejecting — appending a fresh entry, say. A + whole-document restore silently takes that edit with it, because it cannot + tell the story's own `status: done` flip from anybody else's work + (#284 round-7 review, finding 3). + + Substituting whole entry bodies rather than just the two lines the close wrote + keeps the undo exactly as wide as the do: :func:`_apply_done` rewrites the + status line and inserts a `resolution:` line, both inside the entry, so + replacing the entry restores precisely that and nothing else. An id missing + from either side is reported rather than guessed at — the caller decides + whether an entry the hook deleted outright is worth complaining about. + + Spans are applied last-first so each splice leaves the earlier offsets valid. + """ + before_by_id = {e.id: e for e in parse_ledger(before)} + current_by_id = {e.id: e for e in parse_ledger(current)} + edits: list[tuple[tuple[int, int], str]] = [] + missing: list[str] = [] + for dw_id in dict.fromkeys(dw_ids): + was, now = before_by_id.get(dw_id), current_by_id.get(dw_id) + if was is None or now is None: + missing.append(dw_id) + continue + edits.append((now.span, was.body)) + for (start, end), body in sorted(edits, key=lambda e: e[0][0], reverse=True): + current = current[:start] + body + current[end:] + return current, missing + + +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..3543f551 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -14,12 +14,13 @@ import hashlib import shutil import signal +import stat as stat_mod import sys import time 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 +44,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 @@ -260,6 +261,11 @@ def __init__( # best-effort hint (None when the estimate could not be computed). self._graceful_stopped = False self._graceful_remaining: int | None = None + # Story keys whose parked closure has already been reported unservicable + # (DONE, but the unit never merged). In-memory on purpose: the point is one + # line per process, not one line ever — a later run that re-derives the same + # standing fact should say so again. + self._unmerged_closes_reported: set[str] = set() # Per-unit worktree isolation + integration flow (issue #244 F-3/F-9a). # Built from narrow deps + engine callbacks; the same-name Engine._* worktree # methods below delegate to it. `emit` is late-bound (a lambda, not the bound @@ -281,6 +287,7 @@ def __init__( escalation_pause=self._escalation_pause, workspace_get=lambda: self.workspace, workspace_set=lambda ws: setattr(self, "workspace", ws), + on_integrated=lambda t: self._flush_after_integration(t), ) # Attempt rollback + recovery-ref preservation flow (issue #244 PR 2/2). # Same narrow-deps + engine-callbacks pattern as _worktree_flow: `emit` is @@ -365,6 +372,11 @@ def _run_inner(self) -> RunSummary: self._ensure_target_branch() self._prune_preserve_refs() self._loop() + # Last chance to settle an external-ledger closure held back by an + # unavailable location: past the line below the run is finished, + # and `resume` refuses a finished run, so a retained obligation + # would promise a retry nothing can reach (#234). + self._reconcile_pending_deferred_closes(final=True) self.state.finished = True self._gc_run_worktrees() self._emit("post_run") @@ -775,8 +787,83 @@ def _pause_for_manual_recovery( task, baseline, preserve_failed=preserve_failed ) + def _reconcile_pending_deferred_closes(self, *, final: bool = False) -> None: + """Finish external-ledger closures a previous process left parked (#234). + + These sit outside the phase machine the rest of `_finish_inflight` walks: + the write happens after the task is already DONE (after `merge_local` + under isolation), so a crash in that window leaves a terminal task holding + an unsatisfied obligation — and terminal tasks are exactly what that loop + skips. `unit_merged` is the durable proof the work landed; without it the + unit never reached the target branch, and the ledger reading `open` is the + truthful answer, so the obligation is reported rather than discharged. + + `unit_merged` is saved the statement after the merge itself returns, so + the window in which a host death reports a merge that did land as + abandoned is that save alone. It used to span `merge_local`'s whole tail — + a journal append, the `post_merge` emit, the worktree teardown — none of + which is evidence about whether the merge happened. + + That residual direction is deliberate: the entries stay open (which a + sweep re-verifies against the codebase, and which `deferred-close-abandoned` + names for the operator), where recording the intent before the merge would + instead close entries for a merge that never happened. Proving it from the + target branch is not available as a tiebreaker either — `squash` rewrites + the commit, so the unit's sha is not on the target under every strategy. + + ``final`` is the run's last pass, immediately before it is marked finished. + An obligation is only worth retaining while some later process can act on + it, and ``resume`` refuses a finished run — so one carried past this point + promises a retry that can never be reached. Anything still owed here is + reported and released: the entries stay ``open``, which is the truthful + reading and the one a sweep re-verifies against the codebase. Deliberately + NOT a reason to hold the run open — an annotation is never a gate in this + feature, and a mount that stays gone would make the run unfinishable + (#284 round-6 review, finding 1).""" + for task in list(self.state.tasks.values()): + if not task.pending_deferred_closes: + continue + if task.phase != Phase.DONE: + continue # still re-drivable; the commit phase applies it + if not (task.unit_merged or not self._isolated): + # Reported once per process, not once per pass: a resumed run walks + # this loop from `_finish_inflight` and again at the end, and the + # condition is the same standing fact both times. + if task.story_key not in self._unmerged_closes_reported: + self._unmerged_closes_reported.add(task.story_key) + self.journal.append( + "deferred-close-abandoned", + story_key=task.story_key, + dw_ids=list(task.pending_deferred_closes), + reason="story is done but its unit never merged; entries stay open", + ) + if final: + # Nothing after this point can service it either — same reason + # as the release below — so it is let go rather than left + # serialized on a finished run as an obligation with no owner + # (#284 round-7 review, finding 7). + task.pending_deferred_closes = [] + self._save() + continue + self._flush_after_integration(task) + if final and task.pending_deferred_closes: + # the flush kept it: the location was unavailable, and this run + # has no later pass — nor any resume — left to try again in. + self.journal.append( + "deferred-close-abandoned", + story_key=task.story_key, + dw_ids=list(task.pending_deferred_closes), + reason=( + "the ledger location was still unavailable at run end; the entries " + "stay open for a sweep to re-verify" + ), + ) + task.pending_deferred_closes = [] + self._save() + def _finish_inflight(self) -> None: """Complete or roll back tasks interrupted by a pause or crash.""" + self._reconcile_pending_deferred_closes() for task in list(self.state.tasks.values()): if task.terminal: continue @@ -1216,6 +1303,12 @@ def _dev_phase(self, task: StoryTask, resume_result: SessionResult | None = None else: task.followup_review_recommended = self._followup_from_spec(task, rj) outcome = self._verify_dev_artifacts(task, result.result_json) + if outcome.ok: + # the spec is verified and on disk: the one moment its + # `closes_deferred:` declaration is known readable (#234). + # The commit boundary re-reads it — this capture is what that + # read falls back on when it faults. + self._capture_declared_deferred(task, site="dev-verify") if outcome.ok and self._run_verify_commands_after_dev(task, result.result_json): # deterministic gates run here too: a broken build must not # reach the (far more expensive) review loop @@ -1719,7 +1812,25 @@ 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 a pre_commit pause veto has already raised out of _escalate + # above — but finalize_commit's `git add -A` is still ahead, so an in-repo + # annotation lands in this story's own commit. It is not final until that + # commit is: the snapshot armed below unwinds it if the commit fails. + # + # The close runs INSIDE this try, and reports its snapshot through `armed` + # rather than through the return value, because the window needing a + # rollback opens before it returns: `mark_done_many` publishes the ledger + # atomically and the journal append recording that publication can still + # raise (a full disk), as can the SIGTERM handler `run()` installed, from + # anywhere in between. Binding the snapshot only on return would leave + # every one of those raises with nothing to undo — the entry left reading + # `done` for a commit that never happened, the one outcome this whole + # path exists to prevent. + armed: list[tuple[Path, str, list[str]]] = [] try: + self._close_declared_deferred(task, armed) # 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 +1844,33 @@ 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, armed[-1] if armed else None) self._escalate(task, f"commit failed: {e}") + except BaseException: + # A failed commit is not the only way out of this window. The signal + # handler installed by `_run` raises RunStopped from wherever the main + # thread is standing — including inside finalize_commit — and `_run_git` + # translates only TimeoutExpired, so a raw OSError on spawn escapes as + # itself. Both leave the ledger flipped for a commit that does not + # exist, and nothing above unwinds it: RunStopped is caught by `run()` + # to finalize a *stopped* run, not to repair bookkeeping. Restore, then + # re-raise untouched — the caller's disposition is not ours to change. + self._restore_deferred_closes(task, armed[-1] if armed else None) + raise + if not self._isolated: + # An out-of-repo ledger could not ride the commit, so it was parked; + # the commit has now landed, which in place is the whole of "durably + # landed" (there is no integration step). Deliberately BEFORE the DONE + # advance: a crash in this window leaves the task COMMITTING, which + # the resume arm re-drives — and both finalize_commit and the close + # are idempotent. After DONE it would be terminal, and unreachable. + # + # Guarded like every other flush: a persistent ledger failure re-drives + # into the same failure on every attempt, so leaving it bare traded an + # advisory annotation for a crash loop (#284 round-7 review, finding 1). + # A transient one is still retried, now by the reconcile pass rather + # than by re-running the commit phase. + self._flush_after_integration(task) advance(task, Phase.DONE) self.journal.append("story-done", story_key=task.story_key, commit=task.commit_sha) self._emit("post_commit", task) @@ -1768,7 +1905,13 @@ def _dev_review_enabled(self) -> bool: return False return self.policy.review.enabled - def _observed_frontmatter(self, spec_path: Path, story_key: str, site: str) -> dict | None: + # 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, *, strict: bool = False + ) -> dict | None: """Read a spec's frontmatter on a *bookkeeping* path, degrading an unreadable spec to ``None`` (journaled) instead of a whole-run crash. @@ -1786,12 +1929,34 @@ def _observed_frontmatter(self, spec_path: Path, story_key: str, site: str) -> d 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. + + ``strict`` extends the same degrade-to-None to a spec whose *content* is + unreadable — invalid YAML, non-UTF-8 — which ``read_frontmatter`` + otherwise flattens to ``{}``. Every status gate wants that flattening (an + unparseable spec reads as status "" and retries or repairs), so it stays + the default. The deferred-work close needs the opposite: there ``{}`` says + the story declares nothing, which is a retraction, and reading an + unparseable spec as one destroyed the verified capture that exists to + survive precisely this (#284 round-6 review, finding 5). """ try: - return verify.read_frontmatter(spec_path) + fm = ( + verify.read_frontmatter_or_none(spec_path) + if strict + else verify.read_frontmatter(spec_path) + ) except OSError as e: self._journal_spec_read_failed(spec_path, story_key, site, e) return None + if fm is None: # strict only: present but unparseable + self.journal.append( + "spec-read-failed", + story_key=story_key, + spec=str(spec_path), + site=site, + error="frontmatter is not parseable (invalid YAML or non-UTF-8)", + ) + return fm def _journal_spec_read_failed( self, spec_path: Path, story_key: str, site: str, e: OSError @@ -2049,6 +2214,646 @@ 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 _capture_declared_deferred(self, task: StoryTask, *, site: str) -> bool: + """Read the spec's ``closes_deferred:`` declaration onto the task (#234). + + Called from two sites, and which one is authoritative matters. + + **The commit boundary is the authority.** What the spec says at the + moment of the close is what the story declares; a declaration edited + after the dev artifacts verified — a review session rewriting the + frontmatter, a human editing the spec while the review loop runs — must + not be closed against a stale snapshot, because the stale half of that + can close an entry the final spec no longer names. + + **The verify-time capture is what makes re-reading safe to attempt.** + ``_observed_frontmatter`` degrades an unreadable spec to None, and at the + close site that is indistinguishable from a spec declaring nothing — so a + transient read fault there would let the story commit with its declared + entry still open, leaving a journal line as the only trace. A failed read + leaves the last good capture standing to be closed against instead. It is + the same value the COMMITTING resume arm leans on when its own re-read + faults; that arm finishes a commit WITHOUT re-verifying, so a persisted + declaration is its only other source. + + The read is therefore ``strict``: unreadable here means the *content* too, + not only an OSError. ``read_frontmatter`` flattens invalid YAML and + non-UTF-8 to ``{}``, which every status gate wants and which this one site + must not have — ``{}`` is a spec declaring nothing, so the flattening + turned a spec nobody could parse into a retraction and cleared the very + capture it should have fallen back to (#284 round-6 review, finding 5). + + The path is a verified one — ``task.spec_file`` is recorded only by a + passing ``verify_dev``/``verify_dev_stories`` gate, and stories mode + resolves it by id rather than trusting the session's claim. Sprint mode's + path still came from the session, so it is held to the same + root-containment rule the frontmatter-status reconcile applies: a + surprising absolute path must not be able to steer a ledger write. + + A wrong-container declaration (``closes_deferred: DW-5``) is journaled + rather than silently read as empty: it names real intent that would + otherwise close nothing and say nothing. Both reads report it, tagged with + the ``site`` that found it — two lines for one mistake is the honest + record of two reads, and suppressing the second would need per-read error + state to avoid also suppressing a spec that only turned malformed after + verification. + + A failed read leaves any earlier capture standing — a later attempt's + silence is not a retraction. False only when there IS a spec to read and + reading it failed; the caller decides whether that costs anything. + + A spec that is **gone** or has moved **out of the roots** is a different + answer from a spec that could not be read, and it clears the capture. Both + used to return success without touching it, so a `DW-1` captured at + dev-verify was still closed after a pre-commit workflow deleted, renamed + or redirected the spec — closing against a declaration no readable spec + makes, which is the stale half this re-read exists to eliminate. Only a + genuine read fault keeps the fallback standing, and the withdrawal is + journaled rather than inferred silently (#284 round-5 review, finding 2). + + Absence is therefore decided by an explicit ``stat``, not by + ``Path.is_file()``. That helper answers False for exactly four errnos — + ENOENT, ENOTDIR, EBADF, ELOOP — and *propagates* every other OSError, so it + gets this site wrong in both directions at once: a symlink loop or an + unreadable path component was reported as a confirmed withdrawal and + cleared the capture, while an EACCES, EIO or ESTALE — the shape of the + very mount outage this feature's external-ledger path exists for, and + `spec_file` can live on that mount — raised straight through + ``_declared_deferred_ids`` and ``_close_declared_deferred`` into + ``_finalize_commit_phase``'s ``except BaseException``, crashing the story + at the commit boundary with all of its work already passed + (#284 round-7 review, finding 5). + + The split is not cosmetic, because the two answers move in opposite + directions. Clearing costs a closure the story really did declare — a miss, + which the ledger reports truthfully by staying ``open``. Falling back + closes against the last good capture, which is a FALSE close if the + declaration was genuinely withdrawn. So only a fault that says nothing + about whether the spec is there falls back, and anything that positively + establishes there is no file to read clears. + + The path is not re-derived here: ``task.spec_file`` is recorded only by a + passing verify gate, and the root-containment rule below still holds at + both sites.""" + spec_path = Path(task.spec_file) if task.spec_file else None + absent = True + if spec_path is not None: + try: + absent = not stat_mod.S_ISREG(spec_path.stat().st_mode) + except (FileNotFoundError, NotADirectoryError): + absent = True # nothing is at that path: a confirmed withdrawal + except OSError: + # the location said nothing about the declaration. Keep the + # verified capture; `_declared_deferred_ids` reports the fault. + return False + if spec_path is None or absent: + if task.declared_deferred: + self.journal.append( + "deferred-close-declaration-absent", + story_key=task.story_key, + spec=str(spec_path) if spec_path else None, + site=site, + dw_ids=list(task.declared_deferred), + ) + task.declared_deferred = [] + return True + if not verify.spec_within_roots(spec_path, self.workspace.paths): + self.journal.append( + "deferred-close-skipped-out-of-tree", + story_key=task.story_key, + spec=str(spec_path), + site=site, + dw_ids=list(task.declared_deferred or []), + ) + task.declared_deferred = [] + return True + fm = self._observed_frontmatter( + spec_path, task.story_key, f"deferred-close-{site}", strict=True + ) + if fm is None: + return False + declared, error = deferredwork.parse_declaration(fm.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}", + site=site, + ) + task.declared_deferred = list(declared) + return True + + 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 re-read here, because the spec and the manifest on disk at + the commit are what the story declares — a declaration edited after the + dev artifacts verified would otherwise be closed against a snapshot the + final spec no longer agrees with, and closing an id the author took back + is the one failure this whole path exists to prevent. + + A read that FAILS falls back rather than to "declares nothing": the spec + half to the last good capture (``StoryTask.declared_deferred``, which is + also all a COMMITTING resume has when its own re-read faults), the + manifest half to nothing at all — it is not persisted, so there is nothing + to fall back to. Either way the loss is the one place a failed read can + still cost a closure, so both say so + (``deferred-close-declaration-unreadable``) rather than passing in + silence.""" + ids: list[str] = list(self._manifest_closes_deferred(task)) + if task.spec_file and not self._capture_declared_deferred(task, site="commit-boundary"): + self.journal.append( + "deferred-close-declaration-unreadable", + story_key=task.story_key, + source="spec", + spec=task.spec_file, + note=( + "closing against the last good capture" + if task.declared_deferred + else "no captured declaration to fall back on" + ), + ) + ids += task.declared_deferred or [] + return tuple(dict.fromkeys(ids)) + + def _close_declared_deferred( + self, task: StoryTask, rollback: list[tuple[Path, str, list[str]]] | None = None + ) -> tuple[Path, str, list[str]] | 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. + + **Placement is the whole safety story.** This 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 the annotation into the + story's own commit. Marking at dev-sync time instead (where this first + landed) let a story that later failed verification, was rejected by + review, or escalated leave the ledger permanently claiming its work + resolved: the in-place defer path snapshots the ledger *after* the + mutation and restores it over the rollback, and ``_escalate`` rolls back + nothing at all. + + Where the write lands depends on where the ledger lives. + + **Inside the repo** it is written here so it rides the story's own commit + — and the caller must hand the returned snapshot back to + ``_restore_deferred_closes`` if that commit then fails, because + ``_escalate`` rolls nothing back and the annotation would otherwise stand + in the operator's checkout claiming work that was never committed. + + **Outside the repo** (``ProjectPaths.rebased`` deliberately shares an + external artifact dir between worktrees, and ``add -A`` can never stage + it) nothing is written here at all: no pre-commit write can ride a commit + that cannot carry the file, so the ids are stashed on the task and applied + only once the work is durably landed — after ``finalize_commit`` in place, + after ``merge_local`` under isolation (see + ``_flush_pending_deferred_closes``). + + Returns the pre-close ledger text — with its path and the ids actually + flipped — when an in-repo write marked something, else None. + + ``rollback`` is the same snapshot, reported through a list the CALLER + owns and **before the ledger is touched**, because the return value + cannot cover the window that needs covering: the atomic write publishes, + and then the journal append recording it can raise, as can the stop + signal, before this function ever returns. A caller that only had the + return value would meet those raises holding nothing to undo. The slot is + cleared again when nothing was flipped — ``mark_done_many`` writes only + when it marks — so an empty close never arms a pointless restore. + + Never a gate: an unmatched or malformed id is journaled, never fatal. + Idempotent, so the resume arm may re-drive the commit phase freely — ids + are classified against a ledger snapshot rather than from ``mark_done``'s + return value, which conflates "already done" (a resume re-running a close + that landed — must stay silent) with "absent" (a typo — worth saying).""" + ids = self._declared_deferred_ids(task) + if not ids: + # A prior attempt may have parked ids for an out-of-repo ledger and + # then failed its commit; that obligation deliberately survives, to be + # retried by the re-drive. But the re-drive re-reads the declaration, + # and if it is gone the obligation goes with it — every other path + # recomputes the parked list wholesale from the fresh read, and + # flushing a withdrawn one after the commit would be exactly the false + # close the re-read exists to prevent, on the one ledger that has no + # rollback behind it. Journaled, because an unreadable spec or + # manifest reaches here the same way (reported by + # `_declared_deferred_ids`, but indistinguishable once it returns). + if task.pending_deferred_closes: + self.journal.append( + "deferred-close-withdrawn", + story_key=task.story_key, + dw_ids=list(task.pending_deferred_closes), + reason="the story no longer declares them; the parked closure is dropped", + ) + task.pending_deferred_closes = [] + return None + ledger = self.workspace.paths.deferred_work + # Decided on RESOLVED paths. `is_relative_to` is lexical while the write + # below follows symlinks (`atomic_write_text` resolves first, so a + # symlinked ledger stays a symlink instead of being replaced by a regular + # file) — so an in-repo link pointing at a shared external ledger would + # otherwise be treated as committable: the external target gets flipped + # here, `add -A` stages only the unchanged link, and under isolation a + # merge that never lands leaves shared work marked done with nothing left + # to roll it back. Only the last path component can differ: ProjectPaths + # resolves the artifact dir at construction and again in `rebased`. + try: + in_repo = ledger.resolve().is_relative_to(self.workspace.root.resolve()) + except (OSError, RuntimeError): + # A symlink loop (`resolve()` raises RuntimeError for one under Python + # 3.11/3.12, the requires-python floor) or an unreadable path + # component. Nothing can be decided about where this ledger lives, and + # both answers are wrong to guess: writing it here could flip a shared + # external ledger no commit will carry, while parking it would write an + # IN-repo ledger after the commit and leave the tree dirty for story + # N+1's step-01 to halt on. So close nothing and say so — the entries + # stay open for a sweep, which is what an unreadable ledger location + # honestly amounts to (#284 round-7 review, finding 6). + self.journal.append( + "deferred-close-ledger-unavailable", + story_key=task.story_key, + dw_ids=list(ids), + ledger=str(ledger), + note="the ledger path could not be resolved; nothing was closed", + ) + return None + if not in_repo: + task.pending_deferred_closes = list(ids) + self.journal.append( + "deferred-close-pending-integration", + story_key=task.story_key, + dw_ids=list(ids), + ledger=str(ledger), + ) + return None + available, before = self._read_ledger(ledger) + if not available: + # In repo and yet not readable — a dangling or looping link, a + # permission fault. There is no park to fall back on here (an in-repo + # annotation exists to ride this commit), so say so and close nothing + # rather than read the outage as "no such entries". + self.journal.append( + "deferred-close-ledger-unavailable", + story_key=task.story_key, + dw_ids=list(ids), + ledger=str(ledger), + note="the in-repo ledger could not be read; nothing was closed", + ) + return None + if rollback is not None and before is not None: + # Armed here, not on the way out. Restoring text nothing changed is a + # no-op rewrite, so arming early costs an over-broad window nothing, + # while arming late would miss the whole publication window. + rollback.append((ledger, before, [])) + marked = self._apply_deferred_closes(task, ids, ledger, before or "", rollback) + snapshot = (ledger, before, marked) if marked and before is not None else None + if rollback is not None: + # `mark_done_many` writes only when it marks, so an empty close left + # the ledger byte-identical: disarm rather than record a restore that + # would journal a rollback of nothing. + rollback[:] = [snapshot] if snapshot is not None else [] + return snapshot + + def _restore_deferred_closes( + self, task: StoryTask, snapshot: tuple[Path, str, list[str]] | None + ) -> None: + """Put the ledger back the way ``_close_declared_deferred`` found it, after + the commit those closures were written for failed (#234). + + The close is written *before* ``finalize_commit`` precisely so an in-repo + annotation lands in the story's own commit — but a commit can still fail + (a rejecting native pre-commit hook, a full disk), and ``_escalate`` then + raises without unwinding anything. Left alone the entry reads ``done`` for + work that is not in any commit, and the most likely recovery makes it + permanent: a human-resolved re-drive sets ``resolved_redrive``, which has + ``safe_reset`` preserve the artifact folders' tracked content through the + rollback, so the false close survives the reset that would otherwise have + reverted it. + + The index is restored with the tree. Every path this orchestrator takes to + commit again begins with another ``add -A`` (restaging from the tree) and + every rollback path goes through ``safe_reset``, so for the loop itself + the index is not authoritative — but the failed ``finalize_commit`` left + its ``add -A`` staged, and an escalation is precisely where a human takes + over. Fixing the rejecting hook and running ``git commit`` by hand would + otherwise publish the false ``done`` this restore exists to remove + (#284 round-6 review, finding 3). Advisory: a refused re-stage is + journaled, never raised, because it must not mask the commit failure being + escalated. + + **Only the entries this story flipped are put back.** What failed the + commit can be a native ``pre-commit`` hook that edited this same ledger + before rejecting it, and rewriting the whole document takes that edit with + it: the restore cannot tell the story's own ``status: done`` from anybody + else's work, and the entry the hook appended simply disappears + (#284 round-7 review, finding 3). ``restore_entries`` substitutes the named + entries inside whatever is on disk now, which is exactly as wide as the + write it undoes. + + The whole-document rewrite stays as the fallback for the one case with + nothing to target: ``marked`` is empty when a stop signal landed *inside* + the write, before the close could report which ids it had flipped. There + the restore is by content or not at all, and giving it up would hand that + window back its false close.""" + if snapshot is None: + return + ledger, before, marked = snapshot + text, unrestored = before, [] + if marked: + try: + text, unrestored = deferredwork.restore_entries( + ledger.read_text(encoding="utf-8"), before, marked + ) + except OSError: + # unreadable now; the snapshot is still a better answer than a + # ledger left claiming work that was never committed. + text, unrestored = before, [] + try: + atomic_write_text(ledger, text) + except OSError as e: + # bookkeeping repair must not mask the commit failure being escalated + self.journal.append( + "deferred-close-rollback-failed", + story_key=task.story_key, + ledger=str(ledger), + error=str(e), + ) + return + if unrestored: + # the entry is not in the ledger any more, so there is no `status: done` + # left standing — but the operator should know the rollback found the + # document rearranged underneath it. + self.journal.append( + "deferred-close-rollback-partial", + story_key=task.story_key, + dw_ids=list(unrestored), + ledger=str(ledger), + note="these entries were gone from the ledger by the time the close was undone", + ) + if not verify.stage_path(self.workspace.root, ledger): + self.journal.append( + "deferred-close-rollback-unstaged", + story_key=task.story_key, + ledger=str(ledger), + note="the working tree is restored; the index still holds the rolled-back close", + ) + self.journal.append( + "deferred-close-rolled-back", + story_key=task.story_key, + # what was actually flipped and is now un-flipped, NOT everything the + # story declared: an id that was already done stays done either way. + # Empty means the close was interrupted before it could report what it + # had flipped (a stop signal landing inside the write). The restore is + # by content, not by id, so it is complete either way — only the record + # of which entries it touched is missing. + dw_ids=list(marked), + ledger=str(ledger), + ) + + def _read_ledger(self, ledger: Path) -> tuple[bool, str | None]: + """The one place that decides whether a ledger location is answering. + + Returns ``(available, text)``. ``available`` False means the location said + nothing — the artifact directory is gone, the link dangles, a component is + unreadable, the bytes are not decodable — and a caller holding an + obligation must keep it. ``(True, None)`` is a live location with no ledger + in it, which genuinely has nothing to close and discharges. ``(True, text)`` + is the ledger. + + Collapsing the probe and the read into one function is the point. They used + to sit in different methods — the availability test in + ``_flush_pending_deferred_closes``, the read in ``_apply_deferred_closes`` + — each stat'ing the path again, so a mount that dropped between them passed + the test, read as empty text, classified every declared id as ``unknown`` + and cleared the obligation with an ``unmatched`` line blaming a typo for an + outage (#284 round-7 review, finding 6). Here any fault raised by any step + lands in the same handler and answers "unavailable", which is the truthful + reading and the retryable one. + + A ledger that is present but not decodable is unavailable too, not empty: + an empty ledger PARSES, so reading it as text would say every hand-written + entry had vanished. + + Note the ORDER. The read is attempted first and the location is probed only + when it comes back absent, so "there is nothing to close" is never + concluded from a probe that succeeded a moment before the read — the very + ordering that made the race. Anything that drops in between now falls out + as unavailable, which is the retryable answer and the safe direction.""" + try: + return True, ledger.read_text(encoding="utf-8") + except FileNotFoundError: + pass # nothing at that path — but why is decided below + except (OSError, UnicodeDecodeError, RuntimeError): + return False, None + try: + # A live artifact directory with no ledger in it has genuinely nothing + # to close. A directory that is gone, or a link left pointing at a + # mount that is, is an outage — and the link existing at all is the + # evidence a ledger is expected there. + present = ledger.parent.is_dir() and not ledger.is_symlink() + except OSError: + return False, None + return (True, None) if present else (False, None) + + def _apply_deferred_closes( + self, + task: StoryTask, + ids: Sequence[str], + ledger: Path, + text: str, + rollback: list[tuple[Path, str, list[str]]] | None = None, + ) -> 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. + + ``rollback`` is the caller's already-armed restore slot (see + ``_close_declared_deferred``); the ids are written into it the moment they + are known, which is the statement after the write and the one before the + first thing here that can raise. Refining it costs a line and buys the + operator a rolled-back record that names entries instead of an empty + list.""" + declared = deferredwork.classify(text, ids) + marked = deferredwork.mark_done_many( + ledger, declared.open_ids, self._today(), f"resolved by story {task.story_key}" + ) + if rollback and marked: + rollback[0] = (ledger, text, marked) + 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 _flush_after_integration(self, task: StoryTask) -> None: + """The flush every caller goes through, and which may not raise (#234). + + ``WorktreeFlow.integrate_unit`` calls it after the unit has merged and the + task is already terminal, and a raise from there escapes ``run_unit`` and + the whole story loop — so ``_after_story`` never fires. In stories mode + that is the ``done_checkpoint``: a mandatory human review silently skipped, + with the next story free to dispatch. Resume does not repair it either, + because ``_finish_inflight`` skips terminal tasks by design. + + Everything this flush touches can fail — the ledger read and write, the + journal append, the state save — and all of it lives on the out-of-repo + mount that is the only reason the flush does anything at all. Trading a + checkpoint for a failed annotation is exactly backwards, and the annotation + is never a gate anywhere else in this feature: journal the failure, leave + ``pending_deferred_closes`` standing for a later pass to retry, and let the + story finish (#284 round-6 review, finding 2). + + **Every retry site goes through here, not just integration.** Round 6 + guarded this one call and left the reconcile passes + (``_reconcile_pending_deferred_closes``, from ``_finish_inflight`` and from + the end of ``run()``) calling the flush bare, and the in-place + commit-boundary call bare as well. A *persistent* failure — a read-only + mount, an EACCES — then crashed the run through the unguarded site; the + crash handler leaves ``finished`` False, ``resume`` accepts a crashed run, + ``_finish_inflight`` retries at the same bare call and crashes again. An + unbounded crash loop over an advisory annotation, in a feature whose + stated contract is that closure is never a gate + (#284 round-7 review, finding 1). + + Guarding the in-place call reverses round 6's reasoning deliberately. + Leaving it bare bought an idempotent COMMITTING re-drive, which is a real + retry for a *transient* fault and no retry at all for a persistent one — + just the same crash on every attempt. The reconcile passes already provide + the retry, and more cheaply than re-driving a whole commit phase, so the + story now advances to DONE holding its obligation and the run-end pass + retries and then releases it. + + The catch names ``UnicodeDecodeError`` and ``RuntimeError`` alongside + ``OSError`` because neither is one: an undecodable ledger raises a + ``ValueError`` subclass and ``Path.resolve()`` raises ``RuntimeError`` on a + symlink loop under Python 3.11/3.12. It stays an explicit tuple rather than + ``Exception`` so a ``RunStopped`` or ``RunPaused`` still travels. And the + diagnostic append is itself suppressed — a journal write that fails while + reporting a failed write must not become the crash the guard exists to + prevent (#284 round-7 review, finding 2).""" + try: + self._flush_pending_deferred_closes(task) + except (OSError, UnicodeDecodeError, RuntimeError) as e: + with contextlib.suppress(Exception): + self.journal.append( + "deferred-close-flush-failed", + story_key=task.story_key, + dw_ids=list(task.pending_deferred_closes), + error=f"{e.__class__.__name__}: {e}", + note="the closure stays owed; the story's own bookkeeping continues", + ) + + def _flush_pending_deferred_closes(self, task: StoryTask) -> None: + """Apply closures held back for an out-of-repo ledger, once the story's + work is durably landed: after ``finalize_commit`` in place, from the + integration chokepoint under isolation. A story whose commit escalates or + whose merge fails never reaches either call, and the shared ledger keeps + reading ``open``. No-op in every in-repo configuration. + + **Clear only after the write succeeds.** Clearing first looks harmless — + the flush is about to happen — but a raising ledger write then unwinds to + ``run()``'s crash handler, whose ``finally: self._save()`` persists the + emptied list: the obligation is destroyed by the very failure that made it + worth retrying. + + **An unavailable location is not an answer about the entries.** An absent + file does not raise: it reads as empty text, every id classifies as + unknown, and the ids were then cleared on the strength of a ledger nobody + could see. That is a real risk here and only here — this path exists + *because* the ledger is out of the repo, so it can live on a mount that is + temporarily gone. The artifact directory + is what distinguishes the two: gone means the location is unavailable and + the obligation is retryable; present-but-no-ledger means there is + genuinely nothing to close, which is the same answer the in-repo path + gives and stays a discharge rather than an obligation nothing could ever + satisfy (#284 round-5 review, finding 4). + + A **dangling symlink** is the same outage wearing the other shape: the + artifact dir is present, so the directory test passes, but the ledger the + link names is on the mount that went away. ``is_file()`` follows the link + and says no, so every id classified as unknown and the obligation was + discharged with an ``unmatched`` line blaming a typo for an outage. The + link existing at all is the evidence a ledger is expected there + (#284 round-6 review, finding 1). + + Availability and content come from ONE ``_read_ledger`` call. Probing here + and reading again inside ``_apply_deferred_closes`` left a window for the + mount to drop in between: the probe passed, the read saw nothing, and the + obligation was discharged against a ledger that was no longer there + (#284 round-7 review, finding 6).""" + ids = tuple(task.pending_deferred_closes) + if not ids: + return + ledger = self.workspace.paths.deferred_work + available, text = self._read_ledger(ledger) + if not available: + self.journal.append( + "deferred-close-ledger-unavailable", + story_key=task.story_key, + dw_ids=list(ids), + ledger=str(ledger), + note=( + "the ledger location is not present; the closure stays owed and is retried " + "while the run is resumable" + ), + ) + return + self.journal.append( + "deferred-close-external-ledger", + story_key=task.story_key, + dw_ids=list(ids), + ledger=str(ledger), + note="ledger is outside the repo; the annotation is not part of any commit", + ) + self._apply_deferred_closes(task, ids, ledger, text or "") + task.pending_deferred_closes = [] + self._save() + def _extra_session_env( self, task: StoryTask, role: str, label: str | None = None ) -> dict[str, str]: diff --git a/src/bmad_loop/frontmatter.py b/src/bmad_loop/frontmatter.py index e478d476..c40eb9b1 100644 --- a/src/bmad_loop/frontmatter.py +++ b/src/bmad_loop/frontmatter.py @@ -38,28 +38,46 @@ def _split_frontmatter(text: str) -> tuple[str, str, str] | None: return None -def read_frontmatter(path: Path) -> dict[str, Any]: +def read_frontmatter_or_none(path: Path) -> dict[str, Any] | None: + """Like :func:`read_frontmatter`, but distinguishes *unreadable* from *absent*: + ``None`` when the content exists and could not be parsed, ``{}`` when there is + genuinely no frontmatter (or no file) to read. + + Every status gate wants the flattening — a spec that cannot be parsed reads as + status ``""`` and retries or repairs — so :func:`read_frontmatter` keeps it. + One caller needs the difference: the deferred-work close reads a declaration + it will otherwise fall back to a verified capture for, and ``{}`` there says + "this spec declares nothing", which is a *retraction*. Collapsing an + unparseable spec into that retraction destroyed the fallback for exactly the + fault class it exists to survive (#284 round-6 review, finding 5).""" if not path.is_file(): return {} try: text = path.read_text(encoding="utf-8") except UnicodeDecodeError: - # A non-UTF-8 file carries no readable frontmatter — degrade exactly like - # unparseable YAML below. Every status gate then reads status "" and - # returns a clean retry/repair outcome instead of crashing mid-verify - # (UnicodeDecodeError is a ValueError, so it slipped past callers' - # except-OSError guards). - return {} + # A non-UTF-8 file carries no readable frontmatter (UnicodeDecodeError is + # a ValueError, so it slips past callers' except-OSError guards). + return None split = _split_frontmatter(text) if split is None: - return {} + return {} # no frontmatter block is an answer, not a failure to read one try: doc = yaml.safe_load(split[1]) except yaml.YAMLError: - return {} + return None return doc if isinstance(doc, dict) else {} +def read_frontmatter(path: Path) -> dict[str, Any]: + """A spec's frontmatter as a dict, with anything unreadable flattened to ``{}``. + + Every status gate then reads status "" and returns a clean retry/repair + outcome instead of crashing mid-verify. A caller that must tell an unparseable + spec from one carrying no such field wants + :func:`read_frontmatter_or_none` instead.""" + return read_frontmatter_or_none(path) or {} + + def status_of(fm: dict[str, Any]) -> str: """Normalized spec status from a frontmatter dict: stripped + lowercased. diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index 8e8c6ea0..d9668157 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -220,6 +220,38 @@ class StoryTask: # rendered intent file handed to dev sessions dw_ids: list[str] = field(default_factory=list) bundle_file: str | None = None + # the normalized `closes_deferred:` declaration read from this story's spec + # frontmatter, captured when the dev artifacts VERIFIED (the last point the + # spec is known good) rather than re-read at the commit boundary. A transient + # read failure there used to be indistinguishable from "declares nothing", so + # the story committed with the entry still open and only a journal line to + # show for it; and the COMMITTING resume arm never re-verifies, so it had no + # other way to learn the declaration. The stories.yaml manifest channel is + # read from memory and needs no capture. Survives the round-trip. + # + # None means "never captured" and is NOT the same as `[]` ("captured; the spec + # declares nothing"): only the first may re-read the spec at the commit + # boundary, and conflating them re-runs — and re-journals — the read for every + # story that declares nothing. + declared_deferred: list[str] | None = None + # deferred-work ids this story declared via `closes_deferred:` whose ledger + # annotation could not ride the story's own commit, because the ledger is + # configured OUTSIDE the repo (an external artifact dir is shared between + # worktrees, so `rebased` deliberately leaves it in place and `git add -A` + # can never stage it). Stashed at the commit boundary and applied only once + # the work is durably landed — after `finalize_commit` in place, after the + # unit's branch has merged under isolation — so a story whose commit or + # integration fails never claims work resolved. Empty in every in-repo + # configuration. Survives the resume serialization round-trip. + pending_deferred_closes: list[str] = field(default_factory=list) + # worktree-isolation mode only: set once `merge_local` has put this unit's + # branch on the target branch, and PERSISTED before the integration + # bookkeeping that follows it runs. That bookkeeping (the external-ledger + # flush above) is not re-driven by resume — the task is already terminal, so + # `_finish_inflight` skips it — so a crash in that window needs durable proof + # the work actually landed before a later run may apply it. Survives the + # round-trip. + unit_merged: bool = False # worktree-isolation mode only (scm.isolation = "worktree"): the unit's # mounted worktree dir and branch, recorded so a paused/crashed run can # reconstruct or discard the in-flight worktree on resume. @@ -274,6 +306,9 @@ def to_dict(self) -> dict[str, Any]: "restore_patch": self.restore_patch, "dw_ids": self.dw_ids, "bundle_file": self.bundle_file, + "declared_deferred": self.declared_deferred, + "pending_deferred_closes": self.pending_deferred_closes, + "unit_merged": self.unit_merged, "worktree_path": self.worktree_path, "branch": self.branch, "sessions": [s.to_dict() for s in self.sessions], @@ -321,6 +356,13 @@ def from_dict(cls, d: dict[str, Any]) -> "StoryTask": restore_patch=d.get("restore_patch"), dw_ids=[str(i) for i in d.get("dw_ids", [])], bundle_file=d.get("bundle_file"), + declared_deferred=( + [str(i) for i in d["declared_deferred"]] + if d.get("declared_deferred") is not None + else None + ), + pending_deferred_closes=[str(i) for i in d.get("pending_deferred_closes", [])], + unit_merged=bool(d.get("unit_merged", False)), worktree_path=str(d.get("worktree_path", "")), branch=str(d.get("branch", "")), sessions=[SessionRecord.from_dict(s) for s in d.get("sessions", [])], 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..1335381d 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,17 @@ 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, rollback: list[tuple[Path, str, list[str]]] | None = None + ) -> tuple[Path, str, list[str]] | 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.""" + return None + def _close_bundle_ledger_when_spec_status( self, task: StoryTask, diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index e36adf68..d46758dd 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -17,6 +17,7 @@ from . import deferredwork from .bmadconfig import ProjectPaths +from .frontmatter import read_frontmatter_or_none # noqa: F401 — re-export from .frontmatter import set_frontmatter_status # noqa: F401 — re-export from .frontmatter import _split_frontmatter, read_frontmatter, status_of from .model import StoryTask, VerifyOutcome @@ -1663,6 +1664,38 @@ def patch_new_files(patch_path: Path) -> set[str]: return new_files +def stage_path(repo: Path, path: Path) -> bool: + """Stage one path so the index matches what is on disk. Returns False when the + path is outside `repo` or git refused; never raises. + + For repairing an index a failed commit left behind. `finalize_commit` stages + with `add -A` *before* the commit a native hook can still reject, so undoing + that commit's bookkeeping in the working tree alone leaves the index holding + the version that was rolled back — which an operator who fixes the hook and + runs `git commit` by hand would then publish. Advisory by design: this runs on + a path that is already escalating a commit failure, and a repair that raises + would mask it — so "never raises" has to hold against every step, not just the + containment check that motivated the guard. + + Both halves can fail in ways that are not `ValueError`. `resolve()` raises + `RuntimeError` on a symlink loop under Python 3.11 and 3.12 (3.13 returns the + unresolved path instead, so the exposure is version-dependent and the floor is + the affected one), and `OSError` when a path component is inaccessible. `_git` + raises `GitError` on a timeout and lets a raw `OSError` from the spawn itself + through — `_run_git` translates only `TimeoutExpired`. Any of the three would + replace the commit escalation being reported with a generic crash + (#284 round-7 review, finding 4).""" + try: + rel = str(Path(path).resolve().relative_to(repo.resolve())) + except (OSError, RuntimeError, ValueError): + return False + try: + rc, _ = _git(repo, "add", "--", rel) + except (GitError, OSError): + return False + return rc == 0 + + def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: """Commit exactly `paths` (and nothing else), leaving any unrelated working or staged changes untouched. Unlike commit_story's `add -A`, this is safe to diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index 5b999a5b..69d02b2f 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -275,6 +275,7 @@ def __init__( escalation_pause: Callable[..., NoReturn], workspace_get: Callable[[], Workspace], workspace_set: Callable[[Workspace], None], + on_integrated: Callable[[StoryTask], None], ) -> None: self.paths = paths self.policy = policy @@ -294,6 +295,9 @@ def __init__( self._pause = escalation_pause self._workspace_get = workspace_get self._workspace_set = workspace_set + # Late-bound like the rest: post-integration bookkeeping the engine owns + # (the out-of-repo deferred-work closure a unit's commit could not carry). + self._on_integrated = on_integrated @property def isolated(self) -> bool: @@ -475,7 +479,20 @@ def integrate_unit(self, task: StoryTask, unit: UnitWorkspace) -> None: # Merge the unit branch into the target branch locally. We open PRs # ourselves by hand once the branch has landed; the orchestrator only # commits the worktree onto the selected target. + # The unit's work is now on the target branch — the only point at + # which bookkeeping that could NOT ride the unit's own commit (an + # out-of-repo deferred-work ledger) may safely claim it landed. A + # merge that escalates raises out of merge_local and never reaches + # here, leaving the shared ledger untouched (#234). + # + # `merge_local` stamps and persists `task.unit_merged` the moment the + # merge itself returns — durable proof the work landed, which the + # reconcile pass keys its retry on. The task is already DONE, so a + # crash in this window is not re-driven by resume (`_finish_inflight` + # skips terminal tasks), and "phase is DONE" is not a substitute for + # that proof: it is stamped by the commit, before any of this. self.merge_local(task, unit) + self._on_integrated(task) else: # DEFERRED — capture the diff, keep or drop per keep_failed patch = close_unit_workspace( unit, @@ -544,6 +561,17 @@ def merge_local(self, task: StoryTask, unit: UnitWorkspace) -> None: ) self.keep_branch_and_escalate(task, unit, reason) # always raises RunPaused return # defensive: never fall through to the success teardown below + # The merge has landed. Stamp and persist that BEFORE the tail below, + # every line of which can still raise — a journal write on a full disk, a + # post_merge plugin, a teardown callback — while saying nothing about + # whether the merge happened. Stamping after the tail left the code + # durably merged with persisted state claiming it was not, so the resume + # reconcile abandoned the external-ledger closure it was owed + # (#284 round-5 review, finding 3). The residual window is now the merge + # and this save, which is the narrowest it can be: nothing but the merge + # itself is evidence about the merge (#234). + task.unit_merged = True + self._save() self.journal.append( "unit-merged", story_key=task.story_key, 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_cli.py b/tests/test_cli.py index 76ae8f10..fa952341 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, ) @@ -3307,6 +3312,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' diff --git a/tests/test_deferredwork.py b/tests/test_deferredwork.py index ffa89906..77e1a817 100644 --- a/tests/test_deferredwork.py +++ b/tests/test_deferredwork.py @@ -2,17 +2,23 @@ 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, + restore_entries, ) LEDGER = """\ @@ -485,3 +491,225 @@ 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: the engine's + # flush clears `pending_deferred_closes` only after the write returns, so a + # swallowed error returning [] would discharge an obligation nothing performed + 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 + + +def test_restore_entries_reverts_only_the_named_entries(): + """The undo half of `mark_done_many`, and deliberately no wider than the do. + + A rollback runs after a commit failed, and what failed it can be a native + `pre-commit` hook that edited this same ledger before rejecting. Rewriting the + whole document from the pre-close snapshot takes that edit with it, because it + cannot tell the story's own flip from anybody else's work + (#284 round-7 review, finding 3).""" + before = "# Deferred Work\n\n### DW-1: one\n\nstatus: open\n\n### DW-2: two\n\nstatus: open\n" + current = ( + "# Deferred Work\n\n" + "### DW-1: one\n\nstatus: done 2026-07-25\nresolution: resolved by story 1-1-a\n\n" + "### DW-2: two\n\nstatus: done 2026-07-25\nresolution: closed by hand\n\n" + "### DW-9: appended later\n\nstatus: open\n" + ) + + text, missing = restore_entries(current, before, ["DW-1"]) + + assert missing == [] + entries = {e.id: e for e in parse_ledger(text)} + assert entries["DW-1"].open # reverted + assert not entries["DW-2"].open # somebody else's close, untouched + assert "closed by hand" in text + assert "DW-9" in entries # an entry that did not exist at snapshot time survives + + +def test_restore_entries_multiple_entries_keep_their_neighbours(tmp_path): + """Several ids at once. Each splice shifts every offset after it, so the edits + are applied last-first; front-first would rewrite the second entry using spans + measured against the pre-splice text and corrupt the document.""" + p = tmp_path / "deferred-work.md" + p.write_text(LEDGER, encoding="utf-8") + before = LEDGER + marked = mark_done_many(p, ["DW-1", "DW-3"], "2026-07-25", "resolved by story 1-1-a") + assert marked == ["DW-1", "DW-3"] # the fixture's DW-2 is already done + + text, missing = restore_entries(p.read_text(encoding="utf-8"), before, marked) + + assert missing == [] + assert text == before # nothing else changed, so a full revert is byte-identical + entries = {e.id: e for e in parse_ledger(text)} + assert entries["DW-1"].open and entries["DW-3"].open + assert "resolved by story 1-1-a" not in text + + +def test_restore_entries_reports_an_id_it_cannot_put_back(): + """Unrestorable in either direction is reported, never guessed at: an entry the + hook deleted outright is gone from `current`, and one absent from the snapshot + has no prior state to restore. Neither is resurrected — the rollback undoes a + status flip, it does not re-author the ledger.""" + before = "# Deferred Work\n\n### DW-1: one\n\nstatus: open\n" + current = "# Deferred Work\n\n### DW-2: two\n\nstatus: open\n" + + text, missing = restore_entries(current, before, ["DW-1", "DW-2"]) + + assert missing == ["DW-1", "DW-2"] # gone from `current` / absent from `before` + assert text == current # nothing to splice either way + assert "DW-1" not in text diff --git a/tests/test_engine.py b/tests/test_engine.py index 78703b72..01b60215 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,1340 @@ 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"] + # once per read of the declaration — the verify-time capture and the + # commit-boundary re-read that supersedes it — each saying which site it is, + # so two lines read as two reads rather than as two closes. + assert [e["site"] for e in events] == ["dev-verify", "commit-boundary"] + assert all("must be a list" in e["error"] for e in events) + + +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_rollback_restores_the_index_too(project): + """`finalize_commit` stages with `add -A` before the hook can reject, so + restoring only the working tree left the INDEX holding `status: done`. The + loop itself never reads it back — every re-commit re-adds and every rollback + goes through `safe_reset` — but an escalation is exactly where a human takes + over, and fixing the hook then running `git commit` by hand would publish the + close the rollback removed (#284 round-6 review, finding 3).""" + engine = _closes_deferred_run(project, ["DW-1"]) + _reject_commits(project) + + engine.run() + + # neither view of the repo carries the annotation the commit never took + assert "status: done" not in git(project.project, "diff", "--", str(project.deferred_work)) + staged = git(project.project, "diff", "--cached", "--", str(project.deferred_work)) + assert "status: done" not in staged + assert "deferred-close-rollback-unstaged" not in [e["kind"] for e in engine.journal.entries()] + + +def test_closes_deferred_rollback_keeps_an_edit_the_rejecting_hook_made(project): + """The rollback used to rewrite the whole ledger from the pre-close snapshot, + which cannot tell this story's `status: done` from anybody else's work. + + The writer that can land inside that window is the very thing that fails the + commit: a native `pre-commit` hook that edits `deferred-work.md` and then + rejects. Its entry was silently deleted along with the close being undone — + synchronously, inside this story's own commit, so it is not the concurrent + -writer problem tracked by #286 (#284 round-7 review, finding 3).""" + engine = _closes_deferred_run(project, ["DW-1"]) + ledger = project.deferred_work + hooks = project.project / ".git" / "hooks" + hooks.mkdir(parents=True, exist_ok=True) + hook = hooks / "pre-commit" + # files an entry of its own (the shape of a lint/secret hook recording what it + # objected to) and only then refuses the commit + hook.write_text( + "#!/bin/sh\n" + f"cat >> '{ledger}' <<'EOF'\n\n" + "### DW-9: filed by the hook\n\n" + "origin: hook, 2026-06-01\nlocation: src.txt:1\nreason: hook entry.\nstatus: open\n" + "EOF\n" + "exit 1\n", + encoding="utf-8", + ) + hook.chmod(0o755) + + summary = engine.run() + + assert summary.done == 0 and summary.paused # the commit really did fail + entries = _ledger_entries(project) + assert entries["DW-1"].open # the story's close is undone... + assert "DW-9" in entries and entries["DW-9"].open # ...and the hook's entry survives + 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_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_lands_after_the_commit_in_place(project, tmp_path): + """The other half of the parking rule: in place there is no integration step, + so a successful commit IS the durable landing and the held closure must + actually be applied — parking it and never flushing would read exactly like a + story that declared nothing.""" + 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()] + # parked at the commit boundary, applied only once the commit returned + assert kinds.index("deferred-close-pending-integration") < kinds.index("story-deferred-closed") + assert "deferred-close-external-ledger" in kinds # the operator is told it is uncommitted + assert load_state(engine.run_dir).tasks["1-1-a"].pending_deferred_closes == [] + + +def test_closes_deferred_external_ledger_waits_for_the_commit_in_place(project, tmp_path): + """An out-of-repo ledger cannot ride any commit, so writing it before one buys + nothing and risks claiming work that never committed. In place there is no + integration step, so the flush happens once `finalize_commit` returns.""" + 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"])], + ) + _reject_commits(project) + + summary = engine.run() + + assert summary.done == 0 and summary.paused + entries = { + e.id: e for e in deferredwork.parse_ledger(paths.deferred_work.read_text(encoding="utf-8")) + } + assert entries["DW-1"].open # never written: the commit it waited on failed + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-pending-integration" in kinds + assert "story-deferred-closed" not in kinds + # the obligation survives for the re-drive rather than being silently dropped + assert load_state(engine.run_dir).tasks["1-1-a"].pending_deferred_closes == ["DW-1"] + + +def test_closes_deferred_external_obligation_drops_when_the_declaration_is_withdrawn( + project, tmp_path +): + """A parked out-of-repo obligation survives a failed commit on purpose, to be + retried by the re-drive — but the re-drive re-reads the declaration, and the + early return for "declares nothing" left the old parked ids standing while + every other path recomputes them wholesale. The re-drive then flushed a + withdrawn closure after its commit: the false close the re-read exists to + prevent, on the one ledger with no rollback (CodeRabbit, review round 3).""" + 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"])] + ) + hook = _reject_commits(project) + + engine.run() # parks DW-1, then the commit fails + + assert load_state(engine.run_dir).tasks["1-1-a"].pending_deferred_closes == ["DW-1"] + + hook.unlink() + rearm_escalation(engine.run_dir) + # the re-drive's dev session writes the spec with the declaration withdrawn + resumed, _ = resume_engine(paths, engine, [dev_effect(paths, "1-1-a", followup_review=False)]) + summary = resumed.run() + + assert summary.done == 1 # the story itself lands + entries = { + e.id: e for e in deferredwork.parse_ledger(paths.deferred_work.read_text(encoding="utf-8")) + } + assert entries["DW-1"].open # never closed: nothing declares it any more + assert load_state(resumed.run_dir).tasks["1-1-a"].pending_deferred_closes == [] + dropped = [e for e in resumed.journal.entries() if e["kind"] == "deferred-close-withdrawn"] + assert len(dropped) == 1 and dropped[0]["dw_ids"] == ["DW-1"] + + +def test_closes_deferred_external_write_failure_keeps_the_obligation(project, tmp_path): + """Clearing `pending_deferred_closes` before the write looks harmless — the + flush is about to happen — but a failing write would then have destroyed the + record of the retry it made necessary (#284 review, finding 2). + + Driven with a ONE-SHOT fault, because what proves the obligation survived is + that a later pass can still pay it: the run-end reconcile retries and DW-1 + closes. Asserting a crash instead (as this did) pinned the wrong half — the + crash was itself the round-7 defect, not the contract.""" + 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"])], + ) + real_apply = engine._apply_deferred_closes + attempts: list[int] = [] + + def fails_once(*a, **kw): + attempts.append(1) + if len(attempts) == 1: + raise OSError("disk full") + return real_apply(*a, **kw) + + engine._apply_deferred_closes = fails_once + + summary = engine.run() + + assert summary.done == 1 and not summary.crashed + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-flush-failed" in kinds # the first attempt is on the record + # ...and the obligation it kept was paid by the run-end pass + assert not _ledger_entries(paths)["DW-1"].open + assert load_state(engine.run_dir).tasks["1-1-a"].pending_deferred_closes == [] + + +@pytest.mark.parametrize( + "exc", + [ + PermissionError(13, "read-only file system"), + RuntimeError("Symlink loop"), + UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte"), + ], + ids=["oserror", "runtimeerror", "unicodedecodeerror"], +) +def test_closes_deferred_persistent_write_failure_never_crashes_the_run(project, tmp_path, exc): + """The other side of that fault, and the round-7 defect itself. + + Parametrized over all three failure types the guard names, because two of them + are not `OSError` and that is exactly how they got past it: `atomic_write_text` + resolves the path first and `resolve()` raises `RuntimeError` on a symlink loop + under Python 3.11/3.12, while an undecodable ledger raises `UnicodeDecodeError` + — a `ValueError`. Whichever step inside the flush produces one, the story must + still land. + + Round 6 guarded only the post-merge flush; the reconcile passes and the + in-place commit-boundary call stayed bare. A PERSISTENT failure — a read-only + mount, an EACCES — therefore crashed the run through an unguarded site, and + `resume` accepts a crashed run, so `_finish_inflight` retried at the same bare + call and crashed again: an unbounded loop over an advisory annotation, in a + feature whose stated contract is that closure is never a gate + (#284 round-7 review, finding 1). + + The story lands, the failure is journaled, and the run-end pass releases what + it cannot pay — the entry stays `open` for a sweep to re-verify.""" + 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"])], + ) + engine._apply_deferred_closes = lambda *a, **kw: (_ for _ in ()).throw(exc) + + summary = engine.run() + + assert summary.done == 1 and not summary.crashed + persisted = load_state(engine.run_dir) + assert persisted.finished # the run completes rather than looping on the retry + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-flush-failed" in kinds + abandoned = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-abandoned"] + assert len(abandoned) == 1 and abandoned[0]["dw_ids"] == ["DW-1"] + assert _ledger_entries(paths)["DW-1"].open # truthful: nothing was closed + assert persisted.tasks["1-1-a"].pending_deferred_closes == [] + + +def test_closes_deferred_resume_reconcile_does_not_crash_on_a_dead_ledger(project, tmp_path): + """The half that made finding 1 unbounded. `_finish_inflight` opens with the + reconcile pass, and that call was bare — so a resume met the same persistent + failure the first run crashed on and crashed identically, before it could + touch anything else. + + Driven through the reconcile pass directly with a task already DONE and owing, + which is exactly the state a resume loads.""" + paths = _external_ledger_paths(project, tmp_path) + engine, _ = make_engine(paths, []) + task = StoryTask(story_key="1-1-a", epic=1) + task.phase = Phase.DONE + task.pending_deferred_closes = ["DW-1"] + engine.state.tasks["1-1-a"] = task + engine._apply_deferred_closes = lambda *a, **kw: (_ for _ in ()).throw( + PermissionError(13, "read-only file system") + ) + write_ledger(paths, {"DW-1": "open"}, commit=False) + + engine._reconcile_pending_deferred_closes() # must not raise + + assert task.pending_deferred_closes == ["DW-1"] # kept: a later pass may pay it + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-flush-failed" in kinds + + +def test_closes_deferred_flush_guard_survives_a_failing_diagnostic(project, tmp_path): + """The guard's own journal append can fail — it writes to the same host that + just refused the ledger. Reporting a failed write must not become the crash + the guard exists to prevent (#284 round-7 review, finding 2).""" + 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"])], + ) + engine._apply_deferred_closes = lambda *a, **kw: (_ for _ in ()).throw(OSError("disk full")) + real_append = engine.journal.append + + def append_fails_for_the_diagnostic(kind, **fields): + if kind == "deferred-close-flush-failed": + raise OSError("journal is on the same dead mount") + return real_append(kind, **fields) + + engine.journal.append = append_fails_for_the_diagnostic + + summary = engine.run() + + assert summary.done == 1 and not summary.crashed + + +def test_closes_deferred_external_ledger_unavailable_keeps_the_obligation(project, tmp_path): + """A read that raises keeps the obligation by unwinding, but an ABSENT ledger + does not raise: it reads as empty text, every id classifies as unknown, and + the flush cleared the parked ids on the strength of a ledger nobody could see. + + That risk is specific to this path — it exists *because* the ledger is out of + the repo, so it can sit on a mount that is temporarily gone. The artifact + directory tells the two apart: gone is an unavailable location, not an answer + about the entries (#284 round-5 review, finding 4). + + Kept, and then paid: the run-end pass retries what the outage held back, so a + mount that comes back within the run still closes the entry.""" + 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"])], + ) + external = paths.implementation_artifacts + unmounted = tmp_path / "unmounted" + real_flush = engine._flush_pending_deferred_closes + attempts: list[int] = [] + + def flush_with_the_mount_away(task): + attempts.append(1) + if len(attempts) > 1: + return real_flush(task) # the mount is back for the run-end retry + external.rename(unmounted) # the shape of a shared mount dropping out + try: + return real_flush(task) + finally: + unmounted.rename(external) # ...and back, so the rest of the run is normal + + engine._flush_pending_deferred_closes = flush_with_the_mount_away + + summary = engine.run() + + assert summary.done == 1 + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-ledger-unavailable" in kinds + # the id is not reported as a typo against a ledger nobody could see + assert "deferred-close-unmatched" not in kinds + # the mount came back, so the run-end pass finished the job it kept owed + entries = { + e.id: e for e in deferredwork.parse_ledger(paths.deferred_work.read_text(encoding="utf-8")) + } + assert not entries["DW-1"].open + assert kinds.index("deferred-close-ledger-unavailable") < kinds.index("story-deferred-closed") + assert load_state(engine.run_dir).tasks["1-1-a"].pending_deferred_closes == [] + + +@pytest.mark.parametrize( + "ledger_bytes, label", + [(b"\xff\xfe not utf-8\n", "undecodable"), (None, "unreadable")], + ids=["undecodable", "unreadable"], +) +def test_closes_deferred_external_ledger_that_cannot_be_read_stays_owed( + project, tmp_path, monkeypatch, ledger_bytes, label +): + """A ledger that is THERE but cannot be turned into text is an outage, not an + empty ledger — and emphatically not a crash. + + Two shapes, one answer. Undecodable bytes raise `UnicodeDecodeError`, which is + a `ValueError` and so slipped past every `except OSError` on this path; an + unreadable file raises `PermissionError` from inside the close, where the + read used to live. Reading either as empty text would classify every declared + id `unknown` and discharge the obligation with a typo warning — and an empty + ledger PARSES, so there is nothing to make that look wrong + (#284 round-7 review, findings 2 and 6).""" + paths = _external_ledger_paths(project, tmp_path) + write_sprint(paths, {"1-1-a": "ready-for-dev"}) + write_ledger(paths, {"DW-1": "open"}, commit=False) + if ledger_bytes is not None: + paths.deferred_work.write_bytes(ledger_bytes) + else: + real_read = Path.read_text + target = paths.deferred_work + + def denied(self, *a, **kw): + if self == target: + raise PermissionError(13, "Permission denied") + return real_read(self, *a, **kw) + + monkeypatch.setattr(Path, "read_text", denied) + engine, _ = make_engine( + paths, + [dev_effect(paths, "1-1-a", followup_review=False, closes_deferred=["DW-1"])], + ) + + summary = engine.run() + + assert summary.done == 1 and not summary.crashed # never a gate, never a crash + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-ledger-unavailable" in kinds + assert "deferred-close-unmatched" not in kinds # an outage is not a typo + assert "story-deferred-closed" not 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): + """The in-repo half of the round-6 dangling-symlink fix, which only ever + covered the external path. + + An in-repo link pointing at a mount that went away read as an empty ledger, so + every declared id came back `unknown` and the story reported a typo for an + outage. A looping link was worse: `resolve()` raises RuntimeError on Python + 3.11/3.12 — the requires-python floor — and that rose through the commit + boundary (#284 round-7 review, finding 6). + + Both shapes reach the same answer on every interpreter, by different routes: + the loop is refused at the containment check on 3.11/3.12 and at the read on + 3.13+, where `resolve()` returns the unresolved path instead of raising.""" + 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_survives_a_ledger_path_it_cannot_resolve(project, monkeypatch): + """The containment check resolves both paths, and `resolve()` is not total — + RuntimeError on a symlink loop, OSError on an unreadable component. Injected + rather than built from a real loop because the real trigger only raises on + Python 3.11/3.12, so the platform decides whether the guard is exercised at + all; here every interpreter runs it. + + Unresolvable closes nothing and says so: neither guess is safe, since writing + could flip a shared external ledger no commit will carry and parking would + write an in-repo ledger after the commit and leave the tree dirty.""" + engine = _closes_deferred_run(project, ["DW-1"]) + finalize = engine._finalize_commit_phase + real_resolve = Path.resolve + ledger = project.deferred_work + faulting: list[int] = [] + + def resolve_fails(self, *a, **kw): + # only the ledger: `finalize_commit` and the worktree machinery resolve + # paths of their own, and faulting those would prove something else + if faulting and self == ledger: + raise RuntimeError("Symlink loop") + return real_resolve(self, *a, **kw) + + def commit_with_an_unresolvable_ledger(task): + faulting.append(1) + try: + return finalize(task) + finally: + faulting.clear() + + monkeypatch.setattr(Path, "resolve", resolve_fails) + monkeypatch.setattr(engine, "_finalize_commit_phase", commit_with_an_unresolvable_ledger) + + summary = engine.run() + + assert summary.done == 1 and not summary.crashed + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-ledger-unavailable" in kinds + assert "story-deferred-closed" not in kinds + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_closes_deferred_owes_nothing_a_finished_run_can_no_longer_pay(project, tmp_path): + """The other end of that promise. `resume` refuses a finished run, so an + obligation carried past the end of a run that completed promises a retry + nothing can reach — the regression that missed this asserted the pending list + and then called the reconcile pass directly, which the CLI never would. + + A dangling ledger symlink is the second shape of the same outage: the artifact + dir is present, so the directory test passed, `is_file()` followed the link and + said no, and every id was discharged as `unmatched` — a typo warning for a + mount that went away (#284 round-6 review, finding 1).""" + paths = _external_ledger_paths(project, tmp_path) + write_sprint(paths, {"1-1-a": "ready-for-dev"}) + # the ledger is a link onto a mount that is not there; the link existing at all + # is the evidence a ledger is expected + paths.deferred_work.symlink_to(tmp_path / "gone-mount" / "deferred-work.md") + engine, _ = make_engine( + paths, + [dev_effect(paths, "1-1-a", followup_review=False, closes_deferred=["DW-1"])], + ) + + summary = engine.run() + + assert summary.done == 1 # never a gate: 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 # not a typo — an outage + assert "story-deferred-closed" not in kinds + abandoned = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-abandoned"] + assert len(abandoned) == 1 and abandoned[0]["dw_ids"] == ["DW-1"] + assert "sweep" in abandoned[0]["reason"] # says what will re-verify the open entry + persisted = load_state(engine.run_dir) + assert persisted.finished # ...which is exactly why the obligation is not kept + assert persisted.tasks["1-1-a"].pending_deferred_closes == [] + + +@pytest.mark.parametrize("final", [False, True], ids=["mid-run", "run-end"]) +def test_closes_deferred_obligation_is_released_only_by_the_final_pass(project, tmp_path, final): + """The release semantics on their own, on every platform (the end-to-end + reachability test above needs a symlink, so it is POSIX-only). + + Mid-run the obligation is kept: a later pass — this run's own retry, or a + resume — can still pay it. At run end nothing can, because `resume` refuses a + finished run, so keeping it would promise a retry that never comes. Released + means the entries stay `open` for a sweep, never that they are marked done.""" + paths = _external_ledger_paths(project, tmp_path) + engine, _ = make_engine(paths, []) + task = StoryTask(story_key="1-1-a", epic=1) + task.phase = Phase.DONE + task.pending_deferred_closes = ["DW-1"] + engine.state.tasks["1-1-a"] = task + paths.implementation_artifacts.rename(tmp_path / "unmounted") # the location is gone + + engine._reconcile_pending_deferred_closes(final=final) + + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-ledger-unavailable" in kinds # never read as "no entries" + abandoned = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-abandoned"] + assert bool(abandoned) is final + assert task.pending_deferred_closes == ([] if final else ["DW-1"]) + + +@pytest.mark.parametrize("final", [False, True], ids=["mid-run", "run-end"]) +def test_closes_deferred_unmerged_unit_is_reported_once_and_released_at_the_end( + project, tmp_path, final +): + """A DONE task whose unit never merged holds an obligation nothing will ever + service — `_finish_inflight` skips terminal tasks by design, so no later pass + re-integrates it. + + That branch journaled the abandonment and then fell through without clearing + and without consulting `final`, so a resumed run recorded it twice (once from + `_finish_inflight`, once from the run-end pass) and a finished run kept the + pending ids serialized forever (#284 round-7 review, finding 7).""" + paths = _external_ledger_paths(project, tmp_path) + # worktree isolation: the merge, not the commit, is what makes the work durable + engine, _ = make_engine( + paths, + [], + policy=Policy( + gates=GatesPolicy(mode="none"), notify=QUIET, scm=ScmPolicy(isolation="worktree") + ), + ) + task = StoryTask(story_key="1-1-a", epic=1) + task.phase = Phase.DONE + task.unit_merged = False # committed, never integrated + task.pending_deferred_closes = ["DW-1"] + engine.state.tasks["1-1-a"] = task + + engine._reconcile_pending_deferred_closes() # the `_finish_inflight` pass + engine._reconcile_pending_deferred_closes(final=final) + + abandoned = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-abandoned"] + assert len(abandoned) == 1 # one standing fact, one line — not one per pass + assert abandoned[0]["dw_ids"] == ["DW-1"] + # kept while some later pass could still act, let go once none can + assert task.pending_deferred_closes == ([] if final else ["DW-1"]) + assert "story-deferred-closed" not in {e["kind"] for e in engine.journal.entries()} + + +def test_closes_deferred_external_ledger_absent_from_a_live_dir_still_discharges(project, tmp_path): + """The other side of that discriminator. An artifact dir that IS there and + simply has no ledger is not an outage — there is genuinely nothing to close, + which is the same answer the in-repo path gives. Retaining here would leave an + obligation nothing could ever satisfy, re-journaled by every later resume.""" + paths = _external_ledger_paths(project, tmp_path) + write_sprint(paths, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine( + paths, + [dev_effect(paths, "1-1-a", followup_review=False, closes_deferred=["DW-9"])], + ) + assert not paths.deferred_work.exists() # no ledger was ever filed + + summary = engine.run() + + assert summary.done == 1 + kinds = [e["kind"] for e in engine.journal.entries()] + assert "deferred-close-unmatched" in kinds # reported as the stale reference it is + assert "deferred-close-ledger-unavailable" not in kinds + assert load_state(engine.run_dir).tasks["1-1-a"].pending_deferred_closes == [] + + +def test_closes_deferred_uses_the_verified_capture_when_the_commit_read_faults( + project, monkeypatch +): + """`_observed_frontmatter` degrades an unreadable spec to None, which at the + close site was indistinguishable from a spec declaring nothing: a transient + read fault let the story commit with its declared entry still open. The + declaration is captured when the dev artifacts verify, and the commit + boundary's own re-read falls back to that capture rather than to "declares + nothing" (#284 review, finding 3).""" + engine = _closes_deferred_run(project, ["DW-1"]) + finalize = engine._finalize_commit_phase + + def unreadable_from_the_commit_phase_on(task): + # the spec becomes unreadable exactly when the close needs it — the TOCTOU + # shape `_observed_frontmatter` exists to absorb + monkeypatch.setattr(engine, "_observed_frontmatter", lambda *a, **kw: None) + return finalize(task) + + monkeypatch.setattr(engine, "_finalize_commit_phase", unreadable_from_the_commit_phase_on) + + summary = engine.run() + + assert summary.done == 1 + assert not _ledger_entries(project)["DW-1"].open + assert engine.state.tasks["1-1-a"].declared_deferred == ["DW-1"] + + +def test_closes_deferred_keeps_the_capture_when_the_final_spec_is_unparseable(project): + """The same fallback, reached through the real read rather than a patched one. + + `read_frontmatter` flattens invalid YAML and non-UTF-8 to `{}`, so the close + site saw a spec that *parsed* and declared nothing — a retraction — and + cleared the verified capture for the one fault class the capture exists to + survive. Unreadable content is now unreadable, not empty + (#284 round-6 review, finding 5).""" + engine = _closes_deferred_run(project, ["DW-1"]) + finalize = engine._finalize_commit_phase + sp = spec_path(project, "1-1-a") + + def corrupt_the_spec_then_finalize(task): + # an unterminated flow sequence: real YAML the loader rejects, of exactly + # the shape a mid-run frontmatter rewrite can leave behind + sp.write_text( + "---\ntitle: t\nstatus: done\ncloses_deferred: [DW-1\n---\n\nbody\n", encoding="utf-8" + ) + return finalize(task) + + engine._finalize_commit_phase = corrupt_the_spec_then_finalize + + summary = engine.run() + + assert summary.done == 1 + assert not _ledger_entries(project)["DW-1"].open # closed against the verified capture + assert engine.state.tasks["1-1-a"].declared_deferred == ["DW-1"] # capture NOT retracted + read_failed = [ + e + for e in engine.journal.entries() + if e["kind"] == "spec-read-failed" and e["site"] == "deferred-close-commit-boundary" + ] + assert len(read_failed) == 1 and "not parseable" in read_failed[0]["error"] + fell_back = [ + e for e in engine.journal.entries() if e["kind"] == "deferred-close-declaration-unreadable" + ] + assert len(fell_back) == 1 and "last good capture" in fell_back[0]["note"] + + +def test_closes_deferred_reports_an_unreadable_declaration(project, monkeypatch): + """A commit-boundary read that faults with nothing captured to fall back on — + a state file written before the capture existed — is the one place a fault can + still cost a closure, so it says so instead of reading as "declares + nothing".""" + write_ledger(project, {"DW-1": "open"}) + engine, _ = make_engine(project, []) + sp = spec_path(project, "1-1-a") + write_spec(sp, "done", "abc123", closes_deferred=["DW-1"]) + task = StoryTask(story_key="1-1-a", epic=1) + task.spec_file = str(sp) # no declared_deferred: a pre-capture state file + monkeypatch.setattr(engine, "_observed_frontmatter", lambda *a, **kw: None) + + 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-declaration-unreadable" + ] + assert len(events) == 1 and events[0]["story_key"] == "1-1-a" + assert events[0]["source"] == "spec" + assert "no captured declaration" in events[0]["note"] + + +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 + the verify-time capture instead let an id the author had since WITHDRAWN be + marked resolved — a false close, the one outcome this whole path exists to + prevent — and dropped one added in the same edit (#284 follow-up review, + finding 3). + + The declaration is re-read here and the capture demoted to the fallback for a + read that faults, so both directions 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_drops_the_capture_when_the_final_spec_is_gone(project, monkeypatch): + """A spec that is GONE at the commit is not a spec that could not be read. The + absent branch returned success without clearing the verify-time capture, and + the close then unioned it in regardless — so a pre-commit workflow that + archived, renamed or deleted the spec still had its ids marked resolved, + against a declaration no readable spec makes (#284 round-5 review, finding 2). + + Withdrawal by deletion is still withdrawal: a missed close beats a false + one.""" + engine = _closes_deferred_run(project, ["DW-1"]) + sp = spec_path(project, "1-1-a") + finalize = engine._finalize_commit_phase + + def spec_removed_before_the_commit(task): + assert engine.state.tasks["1-1-a"].declared_deferred == ["DW-1"] # captured at verify + sp.unlink() # the shape of a pre_commit_gate workflow archiving the spec + return finalize(task) + + monkeypatch.setattr(engine, "_finalize_commit_phase", spec_removed_before_the_commit) + + summary = engine.run() + + assert summary.done == 1 # the story still commits; only the closure is dropped + assert _ledger_entries(project)["DW-1"].open + events = [ + e for e in engine.journal.entries() if e["kind"] == "deferred-close-declaration-absent" + ] + assert len(events) == 1 and events[0]["dw_ids"] == ["DW-1"] + assert "story-deferred-closed" not in {e["kind"] for e in engine.journal.entries()} + + +def test_closes_deferred_keeps_the_capture_when_the_spec_stat_faults(project, monkeypatch): + """The other side of the deletion discriminator, and the reason it cannot be + `Path.is_file()`. + + That helper answers False for exactly ENOENT / ENOTDIR / EBADF / ELOOP and + PROPAGATES every other OSError, so it was wrong in both directions at this + site. An EACCES / EIO / ESTALE — the shape of the same mount outage the + external-ledger path exists for, and `spec_file` can live on that mount — rose + through `_declared_deferred_ids` and `_close_declared_deferred` into + `_finalize_commit_phase`'s `except BaseException`, which restores and + re-raises: the story crashed at the commit boundary with every gate already + passed (#284 round-7 review, finding 5). + + A fault that says nothing about whether the spec is there is not a withdrawal, + so the verified capture stands and the closure still lands. Paired with + `..._when_the_final_spec_is_gone` above, which pins the opposite direction: a + confirmed absence must still clear.""" + engine = _closes_deferred_run(project, ["DW-1"]) + sp = spec_path(project, "1-1-a") + finalize = engine._finalize_commit_phase + real_stat = Path.stat + faulting: list[int] = [] + + def stat_denied_on_the_spec(self, *a, **kw): + if faulting and self == sp: + raise PermissionError(13, "Permission denied") + return real_stat(self, *a, **kw) + + def commit_with_the_spec_unreadable(task): + assert engine.state.tasks["1-1-a"].declared_deferred == ["DW-1"] # captured at verify + faulting.append(1) # scoped to the commit boundary: the capture must stay good + try: + return finalize(task) + finally: + faulting.clear() + + monkeypatch.setattr(Path, "stat", stat_denied_on_the_spec) + monkeypatch.setattr(engine, "_finalize_commit_phase", commit_with_the_spec_unreadable) + + summary = engine.run() + + assert summary.done == 1 and not summary.crashed # never a gate, and never a crash + assert not _ledger_entries(project)["DW-1"].open # closed from the verified capture + events = [ + e for e in engine.journal.entries() if e["kind"] == "deferred-close-declaration-unreadable" + ] + assert len(events) == 1 and "last good capture" in events[0]["note"] + # an unreadable spec is not a deletion, so it must not report one + assert "deferred-close-declaration-absent" not in {e["kind"] for e in engine.journal.entries()} + + +def test_closes_deferred_drops_the_capture_when_the_final_spec_leaves_the_roots( + project, tmp_path, monkeypatch +): + """Same hole through the other early return. The root-containment guard + journaled and returned success while leaving the capture standing, so a spec + redirected out of the approved roots between verification and the commit was + refused a read and closed from the stale snapshot anyway — the guard reporting + a skip it did not perform.""" + engine = _closes_deferred_run(project, ["DW-1"]) + finalize = engine._finalize_commit_phase + outside = tmp_path / "elsewhere" / "story.md" + outside.parent.mkdir(parents=True, exist_ok=True) + outside.write_text("---\nstatus: done\ncloses_deferred: [DW-1]\n---\n", encoding="utf-8") + + def spec_redirected_before_the_commit(task): + task.spec_file = str(outside) + return finalize(task) + + monkeypatch.setattr(engine, "_finalize_commit_phase", spec_redirected_before_the_commit) + + summary = engine.run() + + assert summary.done == 1 + assert _ledger_entries(project)["DW-1"].open # not closed from the stale capture + skipped = [ + e for e in engine.journal.entries() if e["kind"] == "deferred-close-skipped-out-of-tree" + ] + assert skipped and skipped[-1]["dw_ids"] == ["DW-1"] + + +def test_closes_deferred_capture_survives_a_committing_resume(project, monkeypatch): + """The COMMITTING resume arm finishes a commit WITHOUT re-verifying, so when + its own re-read faults the persisted capture is all that knows what the story + declared. Drive it through a real save/reload rather than trusting the + round-trip by inspection: `declared_deferred` distinguishes None (never + captured) from [] (captured, declares nothing), and a `to_dict`/`from_dict` + that flattened the two would close nothing here while every direct-hook test + stayed green (#284 follow-up review, finding 7).""" + write_ledger(project, {"DW-1": "open"}) + engine, _ = make_engine(project, []) + committing_crash_state(project, engine) + engine.state.tasks["1-1-a"].declared_deferred = ["DW-1"] + engine._save() + + resumed, _ = resume_engine(project, engine, []) + # survived state.json — the resumed engine re-read it from disk + assert resumed.state.tasks["1-1-a"].declared_deferred == ["DW-1"] + # ...and the spec the crash left behind carries no declaration to re-read, + # so make the re-read fault: the fallback is the only path to the close + monkeypatch.setattr(resumed, "_observed_frontmatter", lambda *a, **kw: None) + + resumed.run() + + assert not _ledger_entries(project)["DW-1"].open + committed = git(project.project, "show", "HEAD", "--", str(project.deferred_work)) + assert "status: done" in committed + + +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"] + # the ids are refined into the armed slot right after the write, so the record + # names what it un-flipped rather than reporting an empty rollback + assert len(rolled) == 1 and rolled[0]["dw_ids"] == ["DW-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 + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_closes_deferred_parks_a_ledger_symlinked_out_of_the_repo(project, tmp_path): + """Locality is decided on resolved paths. `is_relative_to` is lexical while + the write follows symlinks, so an in-repo link pointing at a shared external + ledger read as committable: the external target was flipped before the commit, + `add -A` staged only the unchanged link, and under isolation a merge that + never landed left shared work marked done with nothing left to roll it back + (#284 follow-up review, finding 1).""" + write_ledger(project, {"DW-1": "open"}, commit=False) + external = tmp_path / "shared" / "deferred-work.md" + external.parent.mkdir(parents=True, exist_ok=True) + external.write_text(project.deferred_work.read_text(encoding="utf-8"), encoding="utf-8") + project.deferred_work.unlink() + project.deferred_work.symlink_to(external) # in the repo by path, not by inode + + engine, _ = make_engine(project, []) + sp = spec_path(project, "1-1-a") + write_spec(sp, "done", "abc123", closes_deferred=["DW-1"]) + task = StoryTask(story_key="1-1-a", epic=1) + task.spec_file = str(sp) + + snapshot = engine._close_declared_deferred(task) + + assert snapshot is None # nothing written here that a commit could carry + assert "status: open" in external.read_text(encoding="utf-8") + assert task.pending_deferred_closes == ["DW-1"] # held for the durable landing + assert "deferred-close-pending-integration" in [e["kind"] for e in engine.journal.entries()] + + engine._flush_pending_deferred_closes(task) + + assert "status: done" in external.read_text(encoding="utf-8") + assert project.deferred_work.is_symlink() # written through, not replaced + + 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..e102086d 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,149 @@ 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) + + +def test_worktree_external_ledger_closure_waits_for_integration(project, tmp_path): + """An artifact dir configured OUTSIDE the project is shared between worktrees + and can never be staged by the unit's `git add -A`. Writing it at commit time + would let a unit whose merge later fails still claim its work resolved, so the + ids are held until integration succeeds (#234 review, finding 2).""" + import dataclasses + + from conftest import write_ledger + + external = tmp_path / "shared-artifacts" + external.mkdir() + paths = dataclasses.replace(project, implementation_artifacts=external) + write_ledger(paths, {"DW-1": "open"}, commit=False) + engine, _ = make_engine(paths, []) + task = StoryTask(story_key="1-1-a", epic=1) + sp = external / "spec-1-1-a.md" + write_spec(sp, "done", "abc123", closes_deferred=["DW-1"]) + task.spec_file = str(sp) + + engine._close_declared_deferred(task) + + # held back: nothing written, the ids parked on the task for after the merge + assert _ledger_entry(paths, "DW-1").open + assert task.pending_deferred_closes == ["DW-1"] + assert "deferred-close-pending-integration" in journal_kinds(engine) + + engine._flush_pending_deferred_closes(task) + + assert not _ledger_entry(paths, "DW-1").open + assert task.pending_deferred_closes == [] # cleared, so a re-drive is a no-op + kinds = journal_kinds(engine) + assert "deferred-close-external-ledger" in kinds # the operator is told it is uncommitted + assert "story-deferred-closed" in kinds + + +def _external_paths(project, tmp_path): + """`project` with its artifact dir configured OUTSIDE the repo. The sprint + board lives there too and is shared with every worktree rather than committed, + so it is written, not committed, for these runs.""" + import dataclasses + + external = tmp_path / "shared-artifacts" + external.mkdir(exist_ok=True) + return dataclasses.replace(project, implementation_artifacts=external) + + +def test_worktree_crash_after_merge_retries_the_closure_on_resume(project, tmp_path): + """The external-ledger flush runs AFTER the task is already DONE, so a crash + between `merge_local` and the flush leaves a terminal task holding an + unsatisfied obligation — and `_finish_inflight` skips terminal tasks, so + nothing ever retried it. `unit_merged` is the durable proof the work landed, + recorded before the bookkeeping it authorizes (#284 review, finding 2).""" + from conftest import write_ledger + + paths = _external_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, + [wt_dev_effect(paths, "1-1-a", followup_review=False, closes_deferred=["DW-1"])], + ) + # patched at the guarded outer seam, which is where the process would have + # died: a *fault* inside the flush is deliberately caught now and no longer + # ends the run (see the stories-mode done_checkpoint regression), so raising + # from the inner method would model something else entirely. + engine._flush_after_integration = lambda task: (_ for _ in ()).throw(OSError("host died")) + + summary = engine.run() + + assert summary.crashed + saved = load_state(engine.run_dir).tasks["1-1-a"] + assert saved.phase == Phase.DONE and saved.unit_merged # the merge did land + assert saved.pending_deferred_closes == ["DW-1"] # the obligation survived + assert _ledger_entry(paths, "DW-1").open + + state = load_state(engine.run_dir) + state.clear_pause() + resumed = Engine( + paths=paths, + policy=engine.policy, + adapter=MockAdapter([]), + run_dir=engine.run_dir, + journal=engine.journal, + state=state, + ) + resumed.run() + + assert not _ledger_entry(paths, "DW-1").open # reconciled, not lost + assert load_state(engine.run_dir).tasks["1-1-a"].pending_deferred_closes == [] + + +def test_worktree_pending_closure_without_a_merge_stays_open(project, tmp_path): + """A DONE task whose unit never reached the target branch has nothing to + claim. `open` is the truthful reading, so the obligation is reported rather + than discharged — silently applying it would close entries for work that is + on no branch anyone will see.""" + from conftest import write_ledger + + paths = _external_paths(project, tmp_path) + write_sprint(paths, {"1-1-a": "done"}) + write_ledger(paths, {"DW-1": "open"}, commit=False) + engine, _ = make_engine(paths, []) + task = StoryTask(story_key="1-1-a", epic=1) + task.phase = Phase.DONE + task.pending_deferred_closes = ["DW-1"] + task.unit_merged = False # crashed before, or during, the merge + engine.state.tasks["1-1-a"] = task + + engine._finish_inflight() + + assert _ledger_entry(paths, "DW-1").open + events = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-abandoned"] + assert len(events) == 1 and events[0]["dw_ids"] == ["DW-1"] 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..5e4470a2 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={ @@ -827,6 +839,50 @@ def test_story_checkpoint_pause_after_commit(project): assert load_state(resumed.run_dir).tasks["2"].phase == Phase.DONE +def test_a_failed_integration_flush_cannot_skip_the_done_checkpoint(project, tmp_path, monkeypatch): + """The external-ledger flush runs from `integrate_unit`, after the task is + terminal. A raise there escaped `run_unit` and the whole story loop, so + `_after_story` never fired — a mandatory `done_checkpoint` silently dropped, + with the next story free to dispatch — and resume did not repair it either, + because `_finish_inflight` skips terminal tasks by design. + + Everything that flush touches (the ledger read and write, the journal append, + the state save) lives on the shared mount that is the only reason it runs at + all. Trading a human review for a failed annotation is backwards, and the + annotation is never a gate anywhere else here (#284 round-6 review, finding 2).""" + import dataclasses + + from conftest import write_ledger + + from bmad_loop import deferredwork + + paths = dataclasses.replace(project, implementation_artifacts=tmp_path / "shared-artifacts") + paths.implementation_artifacts.mkdir(exist_ok=True) + setup_stories(paths, [entry("1", closes_deferred=["DW-1"], done_checkpoint=True), entry("2")]) + write_ledger(paths, {"DW-1": "open"}, commit=False) # shared: never committable + engine, _ = make_engine( + paths, + [stories_dev_effect()], + policy=_stories_policy(scm=ScmPolicy(isolation="worktree")), + ) + + def the_mount_goes_away(*a, **kw): + raise OSError("shared artifact mount went away") + + monkeypatch.setattr(deferredwork, "mark_done_many", the_mount_goes_away) + + summary = engine.run() + + assert summary.paused and summary.done == 1 # the checkpoint still fired + persisted = load_state(engine.run_dir) + assert persisted.paused_stage == PAUSE_STORY_CHECKPOINT + assert "2" not in persisted.tasks # no story leapfrogged the human review + # owed, not lost: the reconcile pass retries it on the next resume + assert persisted.tasks["1"].pending_deferred_closes == ["DW-1"] + failed = [e for e in engine.journal.entries() if e["kind"] == "deferred-close-flush-failed"] + assert len(failed) == 1 and failed[0]["dw_ids"] == ["DW-1"] + + def test_story_checkpoint_still_fires_when_manifest_unreadable_after_commit(project): """A manifest that goes unreadable between the commit and the after-story check makes the done_checkpoint flag unknowable — the conservative default is @@ -1220,3 +1276,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"] diff --git a/tests/test_verify.py b/tests/test_verify.py index c49e9869..ed96c1a7 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -132,6 +132,77 @@ def spying_run(cmd, **kwargs): assert seen["timeout"] == 7 +def test_stage_path_stages_a_repo_file(project): + """The happy path the advisory repair exists for: a working-tree edit reaches + the index so a hand-run `git commit` publishes what is on disk.""" + target = project.project / "src.txt" + target.write_text("staged by the repair\n") + assert verify.stage_path(project.project, target) is True + rc, out = verify._git(project.project, "diff", "--cached", "--name-only") + assert rc == 0 and "src.txt" in out + + +def test_stage_path_refuses_a_path_outside_the_repo(project, tmp_path): + """Containment: a path that is not under `repo` is refused rather than staged.""" + outside = tmp_path / "elsewhere.md" + outside.write_text("not ours\n") + assert verify.stage_path(project.project, outside) is False + + +@pytest.mark.parametrize("exc", [RuntimeError("symlink loop"), OSError("stale handle")]) +def test_stage_path_returns_false_when_resolve_raises(project, monkeypatch, exc): + """`resolve()` is not total. It raises RuntimeError on a symlink loop and + OSError when a component is inaccessible, and neither is the ValueError the + containment check was written for — so on the paths that hit them the advisory + repair replaced the commit escalation it is supposed to be assisting with a + generic crash (#284 round-7 review, finding 4). + + Injected rather than built from a real symlink loop, because the real trigger + is version-dependent: `resolve()` raises on Python 3.11/3.12 (the + requires-python floor) and returns the unresolved path on 3.13+, so a test + driven by an actual loop would assert a different outcome per interpreter.""" + target = project.project / "src.txt" + target.write_text("changed\n") + monkeypatch.setattr(verify.Path, "resolve", lambda self, *a, **kw: (_ for _ in ()).throw(exc)) + + assert verify.stage_path(project.project, target) is False + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_stage_path_survives_a_real_symlink_loop(project): + """The mechanism above with nothing injected. The return value is deliberately + not asserted — on 3.11/3.12 `resolve()` raises and this refuses, while on 3.13+ + it resolves and git happily stages the link itself — but "never raises" holds + on every interpreter, and it is the half CI's 3.11 job would otherwise not + exercise at all.""" + a, b = project.project / "loop-a", project.project / "loop-b" + a.symlink_to(b) + b.symlink_to(a) + + assert isinstance(verify.stage_path(project.project, a), bool) + + +def test_stage_path_returns_false_when_git_times_out(project, monkeypatch): + """`_run_git` translates TimeoutExpired to GitError, which is still an + exception escaping an advisory repair. Same for a raw OSError on the spawn, + which `_run_git` does not translate at all.""" + target = project.project / "src.txt" + target.write_text("changed\n") + monkeypatch.setattr(verify.subprocess, "run", _timing_out_run) + assert verify.stage_path(project.project, target) is False + + +def test_stage_path_returns_false_when_the_spawn_fails(project, monkeypatch): + """The untranslated half: `_run_git` re-raises anything that is not a + TimeoutExpired, so a fork failure or a missing git binary arrives as itself.""" + target = project.project / "src.txt" + target.write_text("changed\n") + monkeypatch.setattr( + verify.subprocess, "run", lambda *a, **kw: (_ for _ in ()).throw(OSError("fork failed")) + ) + assert verify.stage_path(project.project, target) is False + + def test_run_git_forces_c_locale(project, monkeypatch): """Every git child runs with LC_ALL=C so message text stays stable English — the chokepoint fix for #236 (safe_rollback's "did not match" tolerance must not diff --git a/tests/test_worktree_flow.py b/tests/test_worktree_flow.py index 4ada5b0f..998e55ea 100644 --- a/tests/test_worktree_flow.py +++ b/tests/test_worktree_flow.py @@ -86,7 +86,9 @@ def _make_flow( ): """Build a WorktreeFlow wired to recording stubs. The returned flow carries a ``.calls`` namespace tallying the injected callbacks for assertions.""" - calls = SimpleNamespace(saves=0, emits=[], gates=[], pauses=[], workspaces=[workspace]) + calls = SimpleNamespace( + saves=0, emits=[], gates=[], pauses=[], workspaces=[workspace], integrated=[] + ) def _save() -> None: calls.saves += 1 @@ -132,6 +134,7 @@ def _pause(reason, story_key="", *, cause=None): escalation_pause=_pause, workspace_get=lambda: calls.workspaces[-1], workspace_set=lambda ws: calls.workspaces.append(ws), + on_integrated=lambda task: calls.integrated.append(task), ) flow.calls = calls return flow @@ -298,6 +301,85 @@ def test_escalate_unit_marks_escalated_notifies_and_pauses(tmp_path): assert "CRITICAL escalation: 2-3" in (tmp_path / ATTENTION_FILE).read_text() +def test_integrate_unit_runs_post_integration_bookkeeping_after_a_merge(tmp_path): + """The integration chokepoint is the only safe point for bookkeeping the + unit's own commit could not carry (an out-of-repo deferred-work ledger, #234). + It fires after merge_local, so it is reached only by a unit that landed.""" + flow = _make_flow(tmp_path, state=SimpleNamespace(target_branch="main", run_id="r", tasks={})) + merged = [] + + def merge(task, unit): + # the stamp lives inside merge_local, next to the merge it is proof of + # (see test_merge_local_persists_unit_merged_before_its_journal_tail) + merged.append(task) + task.unit_merged = True + flow._save() + + flow.merge_local = merge + task = StoryTask(story_key="1-1", epic=1) + task.phase = Phase.DONE + # capture the durable-proof flag AS the bookkeeping runs, not after it returns: + # asserting `task.unit_merged` at the end passes for either ordering, and the + # ordering is the contract — `_reconcile_pending_deferred_closes` keys the + # post-crash retry on this flag, and phase DONE is not a substitute for it + # (the commit stamps DONE, before integration). + seen = [] + inner = flow._on_integrated + flow._on_integrated = lambda t: (seen.append((t.unit_merged, flow.calls.saves)), inner(t))[1] + + flow.integrate_unit(task, unit=None) + + assert merged == [task] + assert flow.calls.integrated == [task] + assert seen == [(True, 1)] # flag set AND persisted before the callback fired + + +def test_merge_local_persists_unit_merged_before_its_journal_tail(tmp_path, monkeypatch): + """`unit_merged` is the durable proof `_reconcile_pending_deferred_closes` + keys its post-crash retry on, so the merge itself must stamp it — not + everything that follows the merge. + + The tail after `verify.merge_branch` returns (a journal append, the post_merge + emit, the worktree teardown) can each still raise while saying nothing about + whether the merge happened. Stamping after that tail left the code durably + merged with persisted state claiming it was not, and resume then abandoned the + external-ledger closure it was owed (#284 round-5 review, finding 3).""" + flow = _make_flow(tmp_path, state=SimpleNamespace(target_branch="main", run_id="r", tasks={})) + monkeypatch.setattr(verify, "clean_incoming_collisions", lambda *a, **kw: []) + monkeypatch.setattr(verify, "merge_branch", lambda *a, **kw: None) + + def full_disk(event, **fields): + raise OSError("No space left on device") + + flow.journal.append = full_disk # the first fallible step after the merge + task = StoryTask(story_key="1-1", epic=1) + task.phase = Phase.DONE + + with pytest.raises(OSError): + flow.merge_local(task, unit=SimpleNamespace(branch="unit/1-1")) + + assert task.unit_merged # the merge landed, and the proof of it survived + assert flow.calls.saves == 1 # persisted, not just set in memory + + +def test_integrate_unit_skips_bookkeeping_when_the_merge_escalates(tmp_path): + """A merge that escalates raises out of merge_local. Nothing downstream may + claim the unit's work landed — a shared ledger must still read `open`.""" + flow = _make_flow(tmp_path, state=SimpleNamespace(target_branch="main", run_id="r", tasks={})) + + def boom(task, unit): + raise _Pause("merge blocked", task.story_key) + + flow.merge_local = boom + task = StoryTask(story_key="1-1", epic=1) + task.phase = Phase.DONE + + with pytest.raises(_Pause): + flow.integrate_unit(task, unit=None) + + assert flow.calls.integrated == [] + + def test_reopen_unit_escalates_when_worktree_missing(tmp_path): flow = _make_flow(tmp_path, state=SimpleNamespace(target_branch="main", run_id="r", tasks={})) task = StoryTask(story_key="1-1", epic=1)