feat: auto-resolve deferred-work entries a story declares (closes #234)#284
feat: auto-resolve deferred-work entries a story declares (closes #234)#284pbean wants to merge 36 commits into
Conversation
… closes_deferred (#234)
…ds + document the field (#234) Adds the stories-mode `validate` gate that names declared closes_deferred ids absent from the ledger (a typo, or an entry renumbered since the spec was written) as a warning — advisory only, so it never flips the exit code — and registers the new check id in VALIDATE_CHECKS. Also reconciles the gap phase 1 left: the close hook lived on the base Engine, i.e. sprint mode only, while StoriesEngine._post_dev_state_sync was a no-op. The deferred-work ledger is project-wide (the inherited review-followup refiles already write to it from stories runs), so the field would have been inert in exactly the mode this validate check preflights. StoriesEngine now closes declared entries too, resolving the spec by id rather than the session-claimed path. Documents closes_deferred in the canonical deferred-work format doc, README, and FEATURES; one consolidated CHANGELOG entry covers both phases.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughStories can declare ChangesDeferred-work story closure
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Story
participant StoriesEngine
participant Engine
participant Ledger
participant Git
Story->>StoriesEngine: declare closes_deferred IDs
StoriesEngine->>Engine: capture manifest and spec declarations
Engine->>Ledger: classify and mark open entries done
Engine->>Git: finalize story commit
Git-->>Engine: commit result
Engine->>Ledger: restore on failure or flush after integration
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… just the spec (#234) The frontmatter-only declaration had no producer: `bmad-dev-auto` generates each story spec and knows nothing of the deferred-work ledger — its spec-template frontmatter carries title/type/created/status/review_loop_iteration/ followup_review_recommended/context/warnings and nothing else, and the ledger appears in that skill only as an append target for review defers. So closing an entry from a story required hand-editing a generated spec, which is exactly what does not happen in the unattended multi-epic run this feature exists for. stories.yaml is the channel the orchestrator owns and the human authors at breakdown time, with the ledger in view. `StoryEntry` gains an optional `closes_deferred` list (strict about the container, lenient + deduped per item, like ids); an older manifest without the field parses unchanged. `Engine._close_declared_deferred` takes `extra_ids` and unions the two channels order-preservingly, so an id named in both is marked and journaled once. `StoriesEngine` feeds its manifest entry in. The validate check reads both, and now flags manifest ids even when the story has no spec on disk yet — which makes it a real pre-flight rather than an after-the-first-dispatch one.
|
Added the missing producer. Review caught that the first two commits were the consumer half only: nothing writes
- id: "3-2"
title: Export digests
description: …
closes_deferred: [DW-5, DW-6]
Sprint mode has no manifest, so there the frontmatter remains the only channel — a human or a customized skill. The loop-native version (the dev session declaring what it closed) still needs an upstream Suite: 2950 passed / 7 skipped / 0 failed (+9 tests). |
Comprehensive review: request changesI recommend holding merge until the two high-severity lifecycle/storage issues below are resolved. Four independent passes (adversarial, edge-case, verification-gap, and acceptance) converged on the primary defect. Findings
Recommended remediation plan
Verification
The green checks establish baseline compatibility, but they do not cover the complete verify/review/commit lifecycle where the primary defect occurs. |
… at dev sync (#234 review) Story-declared ledger closure ran in _post_dev_state_sync, i.e. before artifact verification, the verify commands, checkpoints, the review loop and the commit. A story that finalized its spec and then failed any of those left the ledger permanently claiming its work resolved: - the in-place defer path snapshots the ledger AFTER the mutation and restores it over the rollback (the snapshot exists to preserve review-filed appends, so it faithfully preserved the false close too); - _escalate rolls nothing back at all, leaking the close into the operator's checkout where the next story's `git add -A` sweeps it into an unrelated commit. Closure now runs from _finalize_commit_phase, after the pre_commit veto and before finalize_commit's `git add -A` — so it is gated on the whole story succeeding and the annotation still rides the story's own commit. Closing after the commit was not an option: it would leave the tree dirty and story N+1's step-01 HALTs on that. Ledger location decides where the write lands. An externally configured artifact dir is shared between worktrees (ProjectPaths.rebased leaves it in place) and can never be staged, so an isolated unit parks its ids on task.pending_deferred_closes and WorktreeFlow.integrate_unit applies them only after merge_local returns. A merge that escalates never reaches it. Also: - sprint mode's session-supplied spec path is now held to the same verify.spec_within_roots rule the frontmatter reconcile applies, so a stale/hostile absolute path cannot steer a ledger write; - deferredwork gains parse_declaration/classify/mark_done_many: one reading of the field shared by every caller (a wrong container is journaled, not silently empty), four-way classification (a present entry with a garbled status is reported, not skipped in silence), and one atomic write instead of N read-modify-writes; - StoriesEngine._post_dev_state_sync returns to a pure no-op and declares its manifest ids through a _manifest_closes_deferred seam; SweepEngine explicitly opts out of the base hook, since bundle closure is owned by _close_bundle_ledger_when_spec_status and verify_review_bundle depends on it running at dev time. Tests move from calling the hook directly to driving engine.run(): the ledger stays open through defer, escalation and a pre_commit pause veto; the annotation is asserted present in `git show HEAD -- <ledger>`; and the isolated external-ledger path is covered on both sides of integration.
…both modes (#234 review) The same `closes_deferred:` mistake meant different things depending on which file it was made in: a wrong container was a hard schema error in stories.yaml and a silent empty declaration in a story spec, where it closed nothing and said nothing. stories.py now reads the field through deferredwork.parse_declaration, the shared primitive the engine and validate already use, and only the severity differs by caller — the manifest parser raises, the engine journals, validate warns. The preflight also ran in stories mode only, so a sprint-mode typo reached the operator as nothing but a journal line after the run. It now runs in both branches: stories mode keeps the manifest + id-resolved spec union, and sprint mode scans the story specs already sitting in the artifacts dir — written by create-story ahead of the run, which is exactly while a typo is still cheap to fix. Check ids are mode-neutral now that the check is: stories.closes-deferred- unknown becomes deferred.closes-unknown, joined by deferred.closes- malformed for a declaration nothing can read. Both stay warnings; a stale traceability reference must never block a run that would otherwise start.
…234 review) Three claims did not hold: - "squashed into the story's own commit" is true only when the ledger lives inside the repo. An externally configured artifacts dir is shared between worktrees and `git add -A` can never stage it, so the docs now say where the annotation lands in each case, and that an isolated run waits for the merge. - "at clean story-close" described the old dev-sync placement. Closure is at the commit boundary; the docs now name what it sits behind (verification, review, checkpoints, pre-commit) and what that buys. - "makes this work unattended" oversold the stories.yaml channel. It avoids hand-editing each generated spec, but nothing upstream emits the field: it is human-authored at breakdown time exactly like spec_checkpoint / done_checkpoint, and re-deriving stories.yaml drops it unless the intent is logged in .memlog.md first. Filed upstream as BMAD-METHOD#2619 (schema row + the per-story Story Breakdown prompt). Also documents the two failure modes that used to be silent — an entry whose status reads as neither open nor done, and a non-list declaration — and the renamed validate checks (deferred.closes-unknown / deferred.closes-malformed), which now run in both queue modes.
Review remediation: all 7 findings addressedThree commits on this branch. I traced every finding against the code before acting — all 7 are real, with two corrections to the review noted at the bottom. 1 (High) — entries closed before the story succeeded →
|
Comprehensive code reviewRecommendation: request changes before merge. Two high-severity correctness issues violate the core guarantee that deferred work is closed only after a story succeeds, and that a resumable run eventually reconciles all durable work. Findings
Deferred observationConcurrent ledger writers remain unsafe: the update is an unlocked read-modify-write and all writers use the same The mode-neutral rename to Remediation plan
Verification contextAll GitHub checks are currently green, |
…write (#234 review) `mark_done_many`'s tmp-write-and-replace swaps a fresh inode over the ledger, so it silently reset everything carried by the old file rather than by its name: a 0600 ledger came back 0644, extended attributes were dropped, and a symlinked ledger was replaced by a regular file, orphaning the real one. The `mark_done` this replaced wrote in place and had none of these effects. New `platform_util.atomic_write_text` beside `atomic_replace`: resolve the target first, stage through a uniquely-named `mkstemp` sibling (no fixed `.tmp` for a concurrent writer of the same file to collide with), copy the mode and — best effort, Linux — the xattrs before replacing, and clean the temp up on any failure. Ownership cannot be preserved unprivileged; the docstring says so rather than implying otherwise.
…#234 review) Four ways the bookkeeping still outlived, was lost by, or lost track of the story it belonged to. **A failed commit left the entry claiming done.** 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 (a rejecting native pre-commit hook) and `_escalate` unwinds nothing. Worse, the usual recovery cements it: a resolved re-drive preserves the artifact folders' tracked content through `safe_reset`, so the false close survives the reset that would otherwise have reverted it. `_close_declared_deferred` now returns a snapshot and `_restore_deferred_closes` puts the ledger back before the escalation, on the record as `deferred-close-rolled-back`. Only the working tree is restored: every path that commits again starts with another `add -A`. **An out-of-repo ledger was written before the commit in place.** It can never ride a commit, so writing it early bought nothing and risked exactly the false claim above. Both modes now park the ids; in place they flush once `finalize_commit` returns (before the DONE advance, so a crash in the window re-drives the idempotent commit phase), under isolation at the merge as before. **A crash after the merge dropped the obligation.** `_flush_pending_deferred_closes` cleared `pending_deferred_closes` before applying the write, so a raising write unwound to the crash handler whose `finally: self._save()` persisted the emptied list. Clear after the write instead. And because the flush runs when the task is already DONE, a crash between `merge_local` and it left a terminal task holding an unsatisfied obligation that `_finish_inflight` skips: `StoryTask.unit_merged` is now recorded and persisted before that bookkeeping, and a resume reconcile pass retries the closure when it is set — or journals `deferred-close-abandoned` when the unit never merged, since `open` is then the truthful reading. **A transient spec read at the commit boundary silently dropped a declaration.** `_observed_frontmatter` degrades an unreadable spec to None, which there was indistinguishable from a spec declaring nothing — the story committed with its declared entry still open. The declaration is captured onto `StoryTask.declared_deferred` when the dev artifacts verify (the last point the spec is known good) and persisted, so the commit boundary needs no spec read at all and the COMMITTING resume arm, which never re-verifies, gets it too. `None` (never captured) is kept distinct from `[]` (captured, declares nothing) so only the former re-reads; the remaining fallback read journals `deferred-close-declaration-unreadable` rather than reading as silence. Tests drive `engine.run()` end to end: a native pre-commit hook rejecting the commit, the re-armed re-drive that follows it, an external ledger in place, a failing external write asserted against the *persisted* state, a crash after the merge resumed into the reconcile, and every spec read faulted from the commit phase onward.
…happened (#234 review) The format doc and CHANGELOG described "only once the story actually lands" as if reaching the commit boundary were landing. Both now say what happens when the commit itself fails, and that the out-of-repo case waits for durable success in BOTH modes (not only under isolation) and is retried on resume.
Review remediation: all 5 findings addressedThree commits on this branch. I traced each finding against the code before acting — all 5 are real, with two reclassifications noted at the bottom. 1 (High) — a failed commit left deferred work falsely resolved →
|
…the commit (#234 review) The parking rule got a test for the half where the commit fails and the entry must stay open, but none for the half where it succeeds and the held closure must actually be applied. Deleting the post-commit flush left the suite green: a parked-and-never-flushed closure reads exactly like a story that declared nothing.
`deferred-close-rolled-back` journaled everything the story declared, which overstates what changed: an id that was already `done` before the close is still `done` after the restore. The snapshot now carries the marked list, so the event names exactly what was undone — and the restore stops re-deriving the declaration just to describe itself.
|
Filed the follow-up: #286 — deferred-work.md: concurrent writers can lose entries, duplicate DW ids, and revert closures. Scoped to the ledger, as you suggested, and explicitly noting nothing in it was introduced here — this PR only added one more writer, and the unique temp name from Two things the write-up turned up that are worth surfacing:
|
Follow-up comprehensive review: request changesI recommend holding merge. The current head is strong on covered paths, but three high-severity lifecycle defects can still falsely close deferred work, and three medium defects can silently miss closure or violate the documented warning-only contract. Findings
Deferred observationConcurrent ledger writers can still lose updates; a synchronized two-process reproduction left one worker's entry reopened. This predates PR #284 and is already tracked as #286, so it is not a newly introduced blocker here. Remediation plan
VerificationReviewed exact head
The green checks establish compatibility on covered behavior; the findings above are lifecycle/recovery gaps the current suite does not exercise. |
…a snapshot (#234 review) Third-model review of PR #284. Six of eight findings hold; the two that do not are answered in the docstrings rather than the code. Symlinked ledgers were misclassified as in-repo. `is_relative_to` is lexical while `atomic_write_text` resolves, so an in-repo link pointing at a shared external ledger was written here as if it could ride the commit: the external target got flipped, `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. Decide locality on resolved paths. A failed commit was not the only way out of the close/commit window. The signal handler `run()` installs raises RunStopped from wherever the main thread is standing, and `_run_git` translates only TimeoutExpired, so a raw OSError on spawn escapes as itself — both past `except verify.GitError`, both leaving the ledger claiming a commit that does not exist. Restore on any exception and re-raise untouched. The declaration is now re-read at the commit boundary in both channels, with the verify-time capture demoted to the fallback for a read that faults. Closing against the capture could mark an id the author had since WITHDRAWN — a false close, the one outcome this path exists to prevent — and drop one added in the same edit. The manifest half reads directly rather than through `_entry_for`, whose one-warning-per-story dedupe could be spent at dispatch and leave the lost closure silent. Not changed, and now said so in place: `unit_merged` is saved after `merge_local` returns, so a host death in that window reports a merge that did land as abandoned. The entries stay open, which a sweep re-verifies; recording intent before the merge would instead close entries for a merge that never happened, and `squash` rewrites the commit so the target branch cannot arbitrate. A wrong container in `stories.yaml` stays a schema error like every other manifest field of the wrong type — it is the docs that overpromised a warning, in four places. Suite 2991 passed / 7 skipped. Every new regression mutation-checked: neuter the fix, confirm the test fails.
Remediation: 6 of 8 fixed in code, 2 answered in place (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/test_engine_worktree.py (1)
1473-1480: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
_external_pathsinstead of re-inlining it.Lines 1477-1479 rebuild exactly what
_external_pathsconstructs 25 lines below (sameshared-artifactsdir, samedataclasses.replace), and the helper is defined after its first would-be use site. Hoisting the helper above this test and calling it keeps the two from drifting if the external-paths shape changes.♻️ Proposed refactor (move `_external_paths` above this test, then)
- import dataclasses - from conftest import write_ledger - external = tmp_path / "shared-artifacts" - external.mkdir() - paths = dataclasses.replace(project, implementation_artifacts=external) + paths = _external_paths(project, tmp_path) + external = paths.implementation_artifacts write_ledger(paths, {"DW-1": "open"}, commit=False)Also applies to: 1503-1511
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_engine_worktree.py` around lines 1473 - 1480, Hoist the `_external_paths` helper above the affected test cases and replace the duplicated `external.mkdir()`/`dataclasses.replace(project, implementation_artifacts=external)` setup in both locations with calls to `_external_paths`. Preserve the existing shared-artifacts path and returned project-path behavior.tests/test_deferredwork.py (1)
565-570: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the failure propagates instead of swallowing it.
try/except OSError: passpasses just as happily ifmark_done_manyever starts swallowing the write error and returning[]— but callers rely on the raise reaching them (tests/test_engine.py:2302-2319keeps the pending obligation precisely because the write unwinds).pytest.raisespins both halves.♻️ Proposed refactor
- try: - mark_done_many(p, ["DW-1", "DW-3"], "2026-07-24", "note") - except OSError: - pass + with pytest.raises(OSError): + mark_done_many(p, ["DW-1", "DW-3"], "2026-07-24", "note") assert p.read_bytes() == before # nothing partially appliedRequires
pytestto be imported in this module — verify:#!/bin/bash rg -nP --type=py '^\s*import pytest' tests/test_deferredwork.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_deferredwork.py` around lines 565 - 570, Update the test around mark_done_many to use pytest.raises(OSError), ensuring the write failure must propagate rather than be swallowed or converted into a normal return. Keep the existing assertion that p remains unchanged, and add the pytest import if tests/test_deferredwork.py does not already import it.tests/test_worktree_flow.py (1)
304-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlso assert the durable-proof flag this seam now sets.
integrate_unitsetstask.unit_mergedand saves before invoking the callback, and_reconcile_pending_deferred_closeskeys the retry on exactly that flag. Asserting it here pins the ordering contract at the unit level rather than only in the end-to-end worktree test.💚 Proposed test addition
assert merged == [task] assert flow.calls.integrated == [task] + assert task.unit_merged # persisted before the bookkeeping it authorizes + assert flow.calls.saves == 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_worktree_flow.py` around lines 304 - 317, Extend test_integrate_unit_runs_post_integration_bookkeeping_after_a_merge to assert that task.unit_merged is true after flow.integrate_unit completes, preserving the existing merge and callback assertions to pin the durable-proof flag set by integrate_unit before its callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/cli.py`:
- Around line 609-674: Update _validate_closes_deferred to inspect
declared.malformed after deferredwork.classify and emit a
deferred.closes-malformed warning for those referenced ids, including the source
and malformed ids in the warning details. Perform this before the existing
declared.unknown check, while preserving the current unknown-id warning
behavior.
In `@src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md`:
- Around line 73-75: Update the authoring guidance in
src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md:73-75 and
README.md:185-187 to describe breakdown-time declaration authoring as
recommended rather than mandatory, and explicitly state that frontmatter edits
made before commit are read and remain honored.
In `@src/bmad_loop/engine.py`:
- Around line 2286-2288: Update the empty-result branch in the deferred-close
flow around _declared_deferred_ids to clear pending_deferred_closes before
returning when no declarations remain. Preserve the existing return behavior
while ensuring a fresh read with withdrawn declarations cannot flush stale
externally parked obligations.
In `@src/bmad_loop/platform_util.py`:
- Around line 195-210: Update atomic_write_text around the existing os.fdopen
and atomic_replace calls to flush the buffered file handle and invoke
os.fsync(fh.fileno()) before publishing the temporary file. Preserve the current
cleanup and atomic replacement behavior, with directory fsync remaining optional
and POSIX-only.
---
Nitpick comments:
In `@tests/test_deferredwork.py`:
- Around line 565-570: Update the test around mark_done_many to use
pytest.raises(OSError), ensuring the write failure must propagate rather than be
swallowed or converted into a normal return. Keep the existing assertion that p
remains unchanged, and add the pytest import if tests/test_deferredwork.py does
not already import it.
In `@tests/test_engine_worktree.py`:
- Around line 1473-1480: Hoist the `_external_paths` helper above the affected
test cases and replace the duplicated
`external.mkdir()`/`dataclasses.replace(project,
implementation_artifacts=external)` setup in both locations with calls to
`_external_paths`. Preserve the existing shared-artifacts path and returned
project-path behavior.
In `@tests/test_worktree_flow.py`:
- Around line 304-317: Extend
test_integrate_unit_runs_post_integration_bookkeeping_after_a_merge to assert
that task.unit_merged is true after flow.integrate_unit completes, preserving
the existing merge and callback assertions to pin the durable-proof flag set by
integrate_unit before its callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b002e0b-1bf7-4a22-9529-568c82390786
📒 Files selected for processing (23)
CHANGELOG.mdREADME.mddocs/FEATURES.mdsrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.mdsrc/bmad_loop/deferredwork.pysrc/bmad_loop/engine.pysrc/bmad_loop/model.pysrc/bmad_loop/platform_util.pysrc/bmad_loop/stories.pysrc/bmad_loop/stories_engine.pysrc/bmad_loop/sweep.pysrc/bmad_loop/worktree_flow.pytests/conftest.pytests/test_cli.pytests/test_deferredwork.pytests/test_engine.pytests/test_engine_worktree.pytests/test_platform_util.pytests/test_stories.pytests/test_stories_engine.pytests/test_worktree_flow.py
review) CodeRabbit's round on the re-read, plus the preflight gap it exposed. Re-reading the declaration at the commit boundary made a withdrawal reachable, and the "declares nothing" early return did not clear what a prior attempt had parked for an out-of-repo ledger. That obligation survives a failed commit on purpose, to be retried by the re-drive — but every other path recomputes the parked list wholesale from the fresh read, so only this one could flush ids the story no longer declares, after the commit, on the one ledger with no rollback behind it. Clear it, and journal `deferred-close-withdrawn`: an unreadable spec or manifest arrives here the same way, and the drop should not be silent. `validate` also promised more than it delivered. The close hook journals three things, and the preflight reported two — an id whose ledger entry carries neither an `open` nor a `done` status went unmentioned until the journal of the run it should have preceded, though README and docs/FEATURES.md both listed it. Its remedy is in the ledger rather than in the declaration, so it warns under its own `deferred.closes-entry-unreadable` rather than folded into `closes-unknown`. Docs: breakdown time is where a declaration belongs, not a deadline — say so, now that a spec edited before the commit is honored. Suite 2993 passed / 7 skipped. Both new guards mutation-checked.
review) `os.replace` is atomic against a concurrent reader, which says nothing about a machine losing power. Closing the temp hands its bytes to the page cache, so the rename can be durable while the data is not, and the ledger comes back zero-length — and an empty ledger *parses*, as no entries, so the failure reads as every hand-written entry having vanished rather than as corruption. The directory is deliberately not synced. That would make the rename durable, and losing the rename only leaves the old contents in place: stale, never torn. The regression asserts the ORDER (fsync before replace), not that fsync was called — calling it after the publish would be green under the weaker assertion and buy nothing.
CodeRabbit round: 4 actionable, all 4 fixed (
|
…vered (#234 review) Both from CodeRabbit's nitpick set, and both worth more than the label. `mark_done_many`'s all-or-nothing test caught the write error with a bare `except OSError: pass`, which passes just as happily if the function ever starts swallowing the error and returning []. The engine's flush clears `pending_deferred_closes` only after that call returns, so a swallowed write would discharge an obligation nothing performed — exactly the failure the surrounding test was written to prevent. `pytest.raises` pins both halves. `integrate_unit` persists `unit_merged` before the bookkeeping it authorizes, and the reconcile keys its post-crash retry on that flag. Asserting the flag after the call returns is true for either ordering, so the assertion is taken from inside the callback instead — where "already set, already saved" is the whole contract. Skipped the third (hoisting `_external_paths`): real duplication, but in tests from an earlier round that this change does not otherwise touch.
Correction: two of the three nitpicks were worth taking (
|
…lently (#234 review) The ledger read shared a try with the declaration scan, whose silent return is correct for its own reason: an unparseable manifest is already reported by `queue.stories-manifest` and must not be double-counted. Nothing else in `validate` reads the deferred-work ledger, though, so that silence meant an unreadable one produced no finding at all — preflight reporting success for a check that examined nothing, against the very file the run's own closure will fail on. Split the two. A ledger read fault now warns `deferred.ledger-unreadable` (registered in VALIDATE_CHECKS) and the declaration checks it could not run stay quiet rather than guessing; manifest faults keep their single report. Still a warning, never a gate.
Remediation: all 6 findings fixed, with 2 deviations from the planSix commits. I traced each finding against the head you reviewed before touching anything — all six are real, and the two High ones are the same class the previous three rounds were about: bookkeeping that outlives the story it was written for. 1 (High) — a raise inside the close bypasses rollback →
|
Follow-up review at
|
…rite marks (#234 review) `classify` indexed the LAST entry of a duplicated `DW-*` id while `_find_entry` — and so every mutation in the module — takes the FIRST. Both orders then closed nothing and said nothing: a done-first ledger classified the id `open`, sent it to `mark_done_many`, and had `_apply_done` refuse the done copy it found first, marking nothing at all (not even an unmatched warning); an open-first ledger classified it `already_done` and never attempted the write. Index first-wins so the classification names the entry the write acts on, and report the duplicate itself through `Declared.duplicates` / `deferred-close-duplicate-id`: one id naming two entries is a fault about the ledger (#286), not an answer about the work, and the second entry is neither read nor written either way. Duplicate reporting in `validate` stays with #286, which owns ledger corruption.
…nothing (#234 review) `read_frontmatter` flattens invalid YAML and non-UTF-8 to `{}`. Every status gate wants that — an unparseable spec reads as status "" and retries or repairs — but the deferred-work close is the one caller for which `{}` is not an absence, it is a RETRACTION: the story declares nothing. So a spec that turned unparseable between the dev-verify capture and the commit boundary cleared `declared_deferred` and the story committed with its declared entry still open, which is the exact fault class the capture exists to survive. `read_frontmatter_or_none` keeps the distinction (None = present but unparseable, {} = nothing to read) and `read_frontmatter` becomes it `or {}`, so every existing caller is unchanged. `_observed_frontmatter` grows a `strict` flag that extends its degrade-to-None to unreadable content; only `_capture_declared_deferred` passes it, and the fallback then journals like any other failed read.
#234 review) `WorktreeFlow.integrate_unit` calls the external-ledger flush after the unit has merged and the task is already terminal. A raise from there escaped `run_unit` and the story loop, so `_after_story` never fired — in stories mode that is the `done_checkpoint`, a mandatory human review silently skipped with the next story free to dispatch. Resume did not repair it either: `_finish_inflight` reconciles the parked closure and then skips terminal tasks by design. Every fault in that flush is an OSError on the out-of-repo mount that is the only reason it runs at all — the ledger read and write, the journal append, the state save. Trading a human review for a failed annotation is backwards, and the annotation is never a gate anywhere else in this feature. `_flush_after_integration` journals `deferred-close-flush-failed`, leaves `pending_deferred_closes` standing for the reconcile pass, and lets the story finish. Only the integration call site is guarded: the in-place flush runs before the DONE advance precisely so a raise leaves the task COMMITTING for the resume arm to re-drive idempotently. The generic window this shares with `merge_local`'s own tail (a raising post_merge plugin, a failed journal write) is pre-existing and not closed here.
…ls a close back (#234 review) `finalize_commit` stages with `git add -A` before the commit a native pre-commit hook can still reject, so `_restore_deferred_closes` rewriting only the working tree left the index holding `status: done` for a commit that never happened. The loop never reads that back — every path that commits again re-adds from the tree, 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 existed to remove. `verify.stage_path` re-stages the restored file. Advisory by design: it runs on a path already escalating a commit failure, so a refusal is journaled (`deferred-close-rollback-unstaged`) and never raised, where a repair that raises would mask the failure being reported.
…ll pay (#234 review) Two ways the external-ledger obligation lied about what would happen to it. A **dangling ledger symlink** is an outage wearing a shape the discriminator did not recognize: the artifact dir is present so the directory test passed, `is_file()` followed the link and said no, so every id classified as unknown and the obligation was discharged with a `deferred-close-unmatched` line — a typo warning for a mount that went away. The link existing at all is the evidence a ledger is expected there. And a **retained obligation outlived the only thing that could satisfy it**: the caller advances the task to DONE, the run finishes, and `resume` refuses a finished run — so `the closure stays owed and is retried` promised a retry the CLI cannot reach. `_reconcile_pending_deferred_closes(final=True)` now runs once more just before the run is marked finished: it retries what the outage held back (a mount that came back inside the run still closes), and releases anything still owed with `deferred-close-abandoned`. The entries stay `open`, which is the truthful reading and the one a sweep re-verifies. Deliberately not the other repair the review proposed — refusing to finish a run while a closure is owed. An annotation is never a gate in this feature, and a mount that stays gone would make the run unfinishable. The regression that missed all this asserted the pending list and then called the reconcile pass directly, which the CLI never would; it now drives `engine.run()` to the boundary, and `resume_engine` refuses a finished run like the CLI does.
…234 review) The skill doc promised the external-ledger closure was "retried on the next resume if that write did not get to happen", with no mention that a finished run cannot be resumed. Say what actually happens: retried before the run ends and on any later resume, and if the location is still unreachable when the run finishes, the entries stay `open` and are journaled for a sweep to re-verify. Same clause added to the README's ledger-outside-the-repo note.
Remediation: all 5 findings fixed, 2 of the 6 plan steps declinedSeven commits ( 1 (High → Low/Medium) — external-ledger outage →
|
| ledger | classify |
write | journal |
|---|---|---|---|
| DW-1 open, then done | already_done |
not attempted | silent |
| DW-1 done, then open | open_ids |
_apply_done refuses the done copy it finds first, marks nothing |
silent — no unmatched, no malformed |
Root cause is one line: classify indexed last-wins while _find_entry takes the first. It now indexes first-wins, so the classification names the entry the write acts on, and Declared.duplicates / deferred-close-duplicate-id reports the duplicate itself.
5 (Medium) — invalid final frontmatter erased the capture → 181f7b5
Confirmed. read_frontmatter flattens invalid YAML and non-UTF-8 to {}, and _observed_frontmatter degrades only on OSError — so the close site saw a spec that parsed and declared nothing, i.e. a retraction, and cleared the verified fallback for exactly the fault class the fallback exists to survive. read_frontmatter_or_none keeps the distinction and read_frontmatter is now it or {}, so every existing caller is unchanged; only _capture_declared_deferred reads strictly, and the fallback journals like any other failed read.
Two plan steps I did not take
Step 1's "do not permit finished=True while a closure obligation remains." That lets a failed annotation hold a completed run open, and a permanently-gone mount would make the run unfinishable. It contradicts this PR's own "Never a gate" design note. Reporting the truth costs nothing and keeps the run's lifecycle out of the ledger's hands.
Step 2's "persist a post-story/checkpoint obligation + a new resume pass to run/discharge _after_story." A new durable state field and a second reconcile pass to defend a checkpoint against a bookkeeping write. Making the write non-fatal is four lines and removes the failure rather than recovering from it.
Step 4, trimmed. Duplicate ids are journaled at runtime; the validate surface stays with #286, which owns ledger corruption and is already out of scope here.
Verification
- Full suite: 3017 passed, 7 skipped, 0 failed (+13 tests).
- Every regression mutation-checked: neuter the fix, confirm the paired test fails, restore — 12 mutations across the five fixes, 12 caught. The
resume_enginefinished-run guard is the one addition with nothing to mutate: it is a safety net against a future test, not a behaviour change. trunk check --no-fixclean; all 10 CI checks green, including both Windows jobs.
One coverage note: the end-to-end test for an unavailable ledger needs a dangling symlink, so it skips on Windows like every other symlink test here. Since a test that skips in CI is not a gate, the release semantics themselves are pinned separately by a platform-neutral test — kept mid-run, released at run end. (A permanently-missing artifact dir is not an alternative vehicle: sprintstatus.load raises on the missing board, so the run crashes rather than finishing, and the finished-run boundary is exactly what is under test.)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/verify.py`:
- Around line 1678-1683: Update stage_path’s path-resolution exception handling
to also catch OSError and RuntimeError, returning False when resolving either
path fails. Preserve the existing ValueError handling and subsequent _git
staging behavior for successfully resolved paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 902cd8d0-d6c5-45ce-bef9-f74b5b68ff49
📒 Files selected for processing (10)
README.mdsrc/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.mdsrc/bmad_loop/deferredwork.pysrc/bmad_loop/engine.pysrc/bmad_loop/frontmatter.pysrc/bmad_loop/verify.pytests/test_deferredwork.pytests/test_engine.pytests/test_engine_worktree.pytests/test_stories_engine.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md
- tests/test_engine_worktree.py
- src/bmad_loop/deferredwork.py
- README.md
- src/bmad_loop/engine.py
- tests/test_engine.py
The end-to-end reachability test for an unavailable ledger needs a dangling symlink, so it skips on Windows like every other symlink test here — which left the release semantics themselves unverified there, and a test that skips in CI is not a gate. Pin them directly instead, both ways round: mid-run the obligation is kept because a later pass can still pay it, at run end it is released because none can. (A permanently-missing artifact dir is not an alternative vehicle: `_pick_next` raises SprintStatusError on the missing board, so the run crashes rather than finishing, and the finished-run boundary is exactly what is under test.)
Comprehensive review — request changesReviewed against issue #234 at head I found 7 actionable issues: 3 high, 3 medium, 1 low. The current CI is green, but two failure paths below were independently reproduced and are not covered by the suite. Findings
Remediation plan
Verification
Separate pre-existing issueA crash after a worktree task’s |
… repairing (#234 review) `stage_path` documents "never raises" and caught only `ValueError`. Three escapes, all landing where `_restore_deferred_closes` is repairing an escalating commit failure — so the escalation the operator needs is replaced by a generic crash: - `resolve()` raises `RuntimeError` on a symlink loop under Python 3.11/3.12 (3.13+ returns the unresolved path, so the exposure is version-dependent and the requires-python floor is the affected one) - `resolve()` raises `OSError` when a path component is inaccessible - `_git` raises `GitError` on a timeout and lets a raw `OSError` from the spawn through — `_run_git` translates only `TimeoutExpired` The regression injects the resolve fault so it gates on every interpreter; the real symlink loop is covered separately, asserting only that nothing raises, because the return value legitimately differs by version.
#234 review) `_capture_declared_deferred` decided "the spec is gone" with `Path.is_file()`, which answers False for exactly ENOENT / ENOTDIR / EBADF / ELOOP and propagates every other OSError. That got the site wrong in both directions: - a symlink loop or an unreadable path component reported a confirmed withdrawal and cleared the verified capture - an EACCES / EIO / ESTALE rose through `_declared_deferred_ids` and `_close_declared_deferred` into `_finalize_commit_phase`'s `except BaseException`, which restores and re-raises — so the story crashed at the commit boundary with every gate already passed Reachable in the same mount outage the external-ledger path exists for: `ProjectPaths.rebased` can place `implementation_artifacts`, and so the spec, outside the project. Absence is now an explicit `stat`. FileNotFoundError, NotADirectoryError and a successful stat of a non-regular file are a confirmed withdrawal and clear; every other OSError keeps the capture and is reported as unreadable, matching how an OSError from *reading* the spec is already handled. The direction matters: clearing costs a closure the story did declare (a miss the ledger reports truthfully by staying open), while falling back is a false close if the declaration really was withdrawn.
…review) `_restore_deferred_closes` rewrote the entire document from the pre-close snapshot, which cannot tell this story's `status: done` flip 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. Synchronous, inside this story's own commit, so it is not the concurrent-writer problem tracked by #286. New `deferredwork.restore_entries(current, before, ids)` substitutes just the named entries' bodies inside whatever is on disk now, applying the splices last-first so each one leaves the earlier offsets valid. Entry-wide rather than line-wide because that is exactly as wide as `_apply_done`'s write: it rewrites the status line and inserts a `resolution:` line, both inside the entry. 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. An id that cannot be put back from either side is journaled as `deferred-close-rollback-partial` rather than guessed at.
…at it says (#234 review) Availability was probed in `_flush_pending_deferred_closes` and the contents read again in `_apply_deferred_closes`, each stat'ing the path afresh. A mount that dropped between them passed the probe, read as empty text, classified every declared id `unknown` and discharged the obligation with an `unmatched` line — blaming a typo for an outage, permanently. New `_read_ledger` returns (available, text) and is the only place that decides. The ORDER is the fix: the read is attempted first and the location probed only when it comes back absent, so "there is nothing to close" is never concluded from a probe that succeeded a moment earlier. Anything that drops in between falls out as unavailable, which is the retryable answer. It also widens what counts as unavailable to match reality: a ledger present but undecodable is an outage, not an empty ledger — and an empty ledger PARSES, so reading it as text would report every hand-written entry as vanished. Two in-repo shapes that were never routed here: - a dangling ledger symlink read as empty, so the story reported a typo - a looping one crashed the commit boundary: `resolve()` raises RuntimeError on Python 3.11/3.12 An unresolvable ledger path now closes nothing and says so. Neither guess is safe: 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 for story N+1's step-01 to halt on.
… them (#234 review) Round 6 made the post-merge flush non-throwing and left the other three callers bare: both reconcile passes (`_finish_inflight` and the run-end pass) and the in-place commit-boundary flush. A persistent failure — a read-only mount, an EACCES — therefore crashed the run through an 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 loop over an advisory annotation, in a feature whose stated contract is that closure is never a gate. 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 every attempt. The reconcile passes already retry, and more cheaply than re-running a commit phase. The catch now names UnicodeDecodeError and RuntimeError alongside OSError, because neither is one: an undecodable ledger raises a ValueError subclass, and `atomic_write_text` resolves the path first — `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. 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. Finding 7, same loop: a DONE task whose unit never merged journaled its abandonment and fell through without clearing and without consulting `final`, so a resumed run recorded it twice and a finished run kept the ids serialized forever. Now reported once per process and released on the final pass, like every other obligation nothing can service. `test_closes_deferred_external_write_failure_keeps_the_obligation` asserted the crash this removes. Its real contract — clear only after the write succeeds — is now driven with a one-shot fault, so what proves the obligation survived is that the run-end pass pays it.
… the run (#234 review) Two claims the round-7 fixes changed, in the sweep skill's format doc and the CHANGELOG entry for the unreleased feature: - the rollback reverts only the entries the story flipped, so a native pre-commit hook that edited the ledger before refusing the commit keeps its own edit - a ledger location that cannot be read or written is journaled and retried, never read as "no such entries" and never able to fail the story or crash the run that owns it
Remediation: all 7 findings fixed, 2 severities re-read, 2 plan steps declinedSix commits ( Round 7's theme, since the earlier rounds each had one: the guard exists but only on one of its call sites. All three highs are protections round 6 built correctly and then left off a second path reaching the same code. 1 (High) — persistent external-ledger failures crash a completed run →
|
|
Closing in favor of #300, which re-scopes this by subtraction. #234's own guard rail is "surface as a warning rather than failing the story" — an advisory annotation. This PR instead grew two-phase-commit semantics between the ledger and git (rollback arming, parked closures, a reconcile pass, a durable merge marker), and the eight review rounds were not manufacturing findings: transactional ledger↔git coupling has an unbounded adversarial surface, so each round found real defects of the approach. #300 ships the advisory core — the same declaration channels, the same commit-boundary write, the same validate warnings — with clean history and docstrings that claim only the advisory contract. The findings from these reviews that outlived the re-scope are tracked in #286 (concurrent ledger writers), #296 (post-merge tail can skip The branch stays up until #300 merges, for reference. |
1 participant
Closes #234.
The deferred-work ledger was one-way: the loop reliably files entries but never marked one resolved when a later story closed it. Across a multi-epic unattended run that leaves entries satisfied epics ago still reading
open, and the retro reconstructing by hand which story closed what (#234 observed +6 filed / 0 marked / 8 silently satisfied in a single epic).A story can now declare the entries its work closes:
on its
stories.yamlentry (stories mode) or in its story spec's frontmatter — the two unioned. When the story commits, the orchestrator writes the same annotation a sweep bundle already writes:status: done <date>plusresolution: resolved by story <id>.Where the write happens
At the commit boundary, from
_finalize_commit_phasebetween thepre_commitemit andfinalize_commit— behind artifact verification, the verify commands, every checkpoint, the review loop and thepre_commit_gateworkflows, and still ahead ofgit add -Aso an in-repo annotation lands in the story's own commit. A story that fails, blocks, is rejected by review or escalates closes nothing.It is not final until the commit is:
deferred-close-rolled-back). A rejecting nativepre-commithook raisesGitError; a SIGTERM landing mid-commit raisesRunStoppedfrom the handlerrun()installed, wherever the main thread was standing;_run_gittranslates onlyTimeoutExpired, so a rawOSErroron spawn arrives as itself. All three unwind through the restore, and anything that is not a commit failure is re-raised untouched. The snapshot is armed before the ledger is touched and reported through a caller-owned slot rather than the close's return value, so a raise inside the close unwinds too — the atomic write publishes and the journal line recording it is written after, and a stop signal can land between them._escalateunwinds nothing on its own, and the usual recovery would have cemented the false close: a resolved re-drive preserves the artifact folders' tracked content throughsafe_reset.add -Acan never stage it) — decided on resolved paths, so an in-repo symlink to a shared ledger counts as outside;is_relative_tois lexical while the write follows the link → the ids are parked on the task and applied only once the work is durably landed: afterfinalize_commitin place, aftermerge_localunder worktree isolation (deferred-close-external-ledger).StoryTask.unit_mergedis the durable proof the merge happened — stamped and saved the statement afterverify.merge_branchreturns, ahead of the journal/emit/teardown tail that says nothing about whether the merge landed — so a crash between the merge and the flush is retried by a resume reconcile pass instead of being lost. A flush that finds the artifact directory absent keeps the obligation (deferred-close-ledger-unavailable) rather than reading an invisible ledger as empty; a directory that is present with no ledger in it has genuinely nothing to close and discharges, as in-repo does._observed_frontmatterdegrades an unreadable spec to None, which at the close site is indistinguishable from a spec declaring nothing, and the COMMITTING resume arm never re-verifies.Validate
Pre-flight warnings in both queue modes:
deferred.closes-unknown(an id absent from the ledger — a typo, or an entry renumbered since the spec was written) anddeferred.closes-malformed(a story spec declaration that is not a list). Stories mode reads the manifest plus each id-resolved spec, and flags manifest ids even when the story has no spec on disk yet; sprint mode reads the story specs already in the artifacts dir. A ledger that cannot be read at all isdeferred.ledger-unreadable— nothing else invalidatereads it, so staying quiet reported success for a check that examined nothing. All are warnings, so none changes the exit code.The same wrong container in
stories.yamlis not a warning: the manifest is a schema its parser owns, so it fails to load like any other field of the wrong type, reported byqueue.stories-manifestbefore any story runs. That split is deliberate — the manifest is hand-authored before the run, where a typo is still cheap to fix, while a story spec is generated mid-run by a skill and a malformed field there must not be able to fail a story that succeeded.Design notes
status: done <date>+resolution:via the existingmark_doneprimitive. The issue text sketches aresolved:block, but this repo has no such field — sweep, verify and the TUI all recognize closure throughstatus:, so adding one would have forked the format.mark_done's return value, which conflates "already done" (a resume re-running a close that landed — must stay silent) with "absent from the ledger" (worth a warning).deferred-close-unmatched), a ledger entry whosestatus:is neitheropennordone(deferred-close-malformed), and a wrong-container declaration in a story spec are each journaled and dropped. Closes journal asstory-deferred-closed.unit_mergedis persisted the statement after the merge itself returns, so a host death in that one save reports a merge that did land as abandoned and leaves the entriesopen— which a sweep re-verifies against the codebase. Recording the intent before the merge would trade that for closing entries a merge never landed, andsquashrewrites the commit, so the target branch cannot arbitrate after the fact.spec_checkpoint/done_checkpoint. No upstream skill emits it yet — BMAD-METHOD#2619 proposes the schema row and the breakdown prompt — and re-derivingstories.yamldrops it unless the intent is logged in.memlog.md.Verification
Lifecycle coverage drives
engine.run()rather than the hook: the ledger stays open through verify-defer, escalation, apre_commitpause veto and a rejected commit; the annotation is asserted present ingit show HEAD -- <ledger>; the external-ledger path is checked on both sides of the commit and of integration; a crash after the merge is resumed into the reconcile; and the primitives have their own units (wrong container, malformed status, all-or-nothing write, mode/symlink preservation, resume idempotency).Every regression added for a review finding is mutation-checked: neuter the fix, confirm the test fails (11 mutations across the round-5 fixes, 11 caught). The interruption path fires a real in-process SIGTERM from inside
finalize_commitrather than raisingRunStoppeddirectly, so it exercises the handler that actually causes it.Full suite: 3004 passed, 7 skipped, 0 failed.
trunk check --no-fixclean.Summary by CodeRabbit
closes_deferredin manifests and/or story specs (unioned/deduped).donewith story-based resolution notes; external-ledger closures are deferred until integration and retried on resume when needed.validatenow emits warn-only diagnostics for unknown, malformed, or unreadablecloses_deferreddeclarations (no exit-code change).