feat: story-declared deferred-work closure — the advisory contract (#234)#300
Conversation
…red-work entries (#234) A story declares the DW-<n> entries its work closes — `closes_deferred:` on its stories.yaml entry (stories mode), in its spec frontmatter, or both, the two unioned — and at the commit boundary the orchestrator flips each declared open entry to `status: done <date>` + `resolution: resolved by story <id>`, the same annotation a sweep bundle writes. The ledger stops being one-way: filed automatically, resolved only by hand. Advisory by contract: the write sits behind every verify gate, checkpoint and review cycle, immediately before finalize_commit's `git add -A`, so an in-repo ledger rides the story's own commit (worktree isolation included) and a story that fails, is rejected by review, or escalates closes nothing. The declaration is re-read live at that boundary, so a late edit counts in both directions; an already-done id is a silent no-op across a resume; unknown, duplicate and malformed ids and an unavailable or undecodable ledger location are journaled warnings, never failures. A commit that fails after the write restores the ledger from a whole-document pre-close snapshot. An out-of-repo ledger (shared across worktrees, part of no commit) is written at the same moment and journaled `deferred-close-external-ledger`. The ledger write path gains `platform_util.atomic_write_text` (resolve first, unique temp sibling, mode/xattr copy, fsync-before-replace) so a close cannot tear the ledger or swap a symlink for a regular file.
…234) `bmad-loop validate` warns — never fails — in both queue modes: `deferred.closes-unknown` for an id absent from the ledger, `deferred.closes-malformed` for a spec declaration in a non-list shape, `deferred.closes-entry-unreadable` for a ledger entry whose status parses as neither open nor done, and `deferred.ledger-unreadable` for a ledger that cannot be read at all. A non-list declaration in stories.yaml itself stays a manifest schema error, reported by `queue.stories-manifest`.
README (the stories.yaml field + the sweep section), the FEATURES bullet, the sweep skill's deferred-work-format.md, and a CHANGELOG entry — each claiming the advisory contract only: declared, written at the commit boundary, riding the story's own commit when the ledger is in-repo, journaled and never fatal everywhere else.
WalkthroughStories can declare deferred-work IDs through ChangesDeferred-work story closure
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
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 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bmad_loop/engine.py (1)
2257-2267: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider catching
Exceptionfor the restore, not justOSError. The docstring promises a failed restore is "journaled, never raised", but this runs insideexceptarms — any non-OSErrorescapingatomic_write_textwould replace the original exception the caller must propagate unchanged.♻️ Broaden the guard
try: atomic_write_text(ledger, before) - except OSError as e: + except Exception as e: with contextlib.suppress(Exception):🤖 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 `@src/bmad_loop/engine.py` around lines 2257 - 2267, Broaden the restore guard around atomic_write_text in the deferred-close rollback path to catch Exception rather than only OSError, while preserving the suppressed journal append and unchanged return behavior. Ensure any restore failure is journaled without escaping and replacing the original exception.
🤖 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.
Nitpick comments:
In `@src/bmad_loop/engine.py`:
- Around line 2257-2267: Broaden the restore guard around atomic_write_text in
the deferred-close rollback path to catch Exception rather than only OSError,
while preserving the suppressed journal append and unchanged return behavior.
Ensure any restore failure is journaled without escaping and replacing the
original exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 18f1a280-b708-448f-909c-384a36926444
📒 Files selected for processing (20)
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/platform_util.pysrc/bmad_loop/stories.pysrc/bmad_loop/stories_engine.pysrc/bmad_loop/sweep.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.py
… failure (#300 review) `_restore_deferred_closes` runs inside the commit window's except arms, and its docstring promises a failed restore is "journaled, never raised — this runs inside except arms whose exception must travel unchanged". `except OSError` honored that for one exception type. `atomic_write_text` resolves the path before its own try, and below 3.13 `Path.resolve` reports a symlink loop as `RuntimeError` — for a ledger the helper explicitly supports being a symlink, and whose other resolve (`_ledger_in_repo`) already catches that type. 3.11 and 3.12 are both CI legs. Escaping, it displaced the exception each arm exists to carry: the `GitError` arm skipped `_escalate`, stranding the story in COMMITTING with the commit diagnosis nowhere on the record, and the `BaseException` arm skipped its bare `raise`, swapping a graceful `RunStopped` for a write complaint. The ablation is unambiguous — reverting the width returns `escalated=0, paused=False, crashed=True, crash_error="RuntimeError: Symlink loop..."`. `Exception`, not `BaseException`: `RunStopped` is an `Exception`, so a second stop landing inside the restore is absorbed while the first still travels, and a genuine KeyboardInterrupt still gets out. The journal line now names the type the way `_journal_ledger_unavailable` does — with every type admitted, a bare message cannot say whether the disk or the path was at fault. `deferred-close-rollback-failed` had no coverage at any width; the new test is the branch's first. The fault is injected rather than built from a real symlink loop, which 3.13+ resolves without raising — that version would pass here and fail only on the 3.11/3.12 legs.
Resolution — nitpick applied (
|
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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/engine.py`:
- Line 2270: Add a targeted Ruff BLE001 suppression to the broad `except
Exception as e` in the rollback handling, preserving the intentional rollback
behavior without suppressing the rule more broadly.
🪄 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: 781fa730-c172-4c62-a1e4-19d2b9de9095
📒 Files selected for processing (2)
src/bmad_loop/engine.pytests/test_engine.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_engine.py
1 participant
Closes #234. Supersedes #284 (re-scoped by subtraction; rationale in its closing comment).
What
A story declares the ledger entries its work closes —
closes_deferred: [DW-5, DW-6]on itsstories.yamlentry (stories mode), in its spec frontmatter, or both, the two unioned — and at the commit boundary the orchestrator flips each declared open entry tostatus: done <date>+resolution: resolved by story <id>, the same annotation a sweep bundle writes. The ledger stops being one-way: filed automatically, resolved only by hand.bmad-loop validatewarns up front, in both queue modes, on unknown ids, malformed declarations, unreadable entry statuses and an unreadable ledger.The contract (advisory — stated verbatim in the docstrings)
finalize_commit'sgit add -A— so an in-repo ledger rides the story's own commit (worktree isolation included: the unit's ledger reaches the target branch with the ordinary merge) and a story that fails, is rejected by review, or escalates closes nothing.doneid is a silent no-op, so a resume re-driving the commit phase neither doubles the annotation nor warns.open, the direction a sweep re-verifies.ProjectPaths.rebaseddeliberately shares an external artifact dir across worktrees) is written at the same moment; the annotation is part of no commit, which is journaled (deferred-close-external-ledger). A merge that later fails leaves a visible, journaled, human-attended stale annotation — the accepted advisory trade-off.Why this shape, and not #284's
#234 asked for an advisory annotation ("surface as a warning rather than failing the story"). #284 grew two-phase-commit semantics between the ledger and git — rollback arming, parked closures, a reconcile pass, a durable merge marker — and eight review rounds kept finding real defects of that approach: transactional ledger↔git coupling has an unbounded adversarial surface. This PR ships the same feature with the contract the issue actually asked for, as fresh history; because the docstrings claim advisory semantics only, review has a stated contract to check against instead of an implied stronger one.
The crash-window and transactional concerns that outlived the re-scope are tracked on their own: #286 (concurrent ledger writers), #296 (post-merge tail can skip
done_checkpoint), #299 (DONE-but-unmerged stranded unit branch).Verification
trunk check --no-fixclean;pyright0 errors.stays_open_when_verify_defers_the_story,stays_open_when_the_story_escalatesanddev_sync_writes_nothing; misreading an absent declaration as a declaration failsclose_is_silent_without_declaration.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
closes_deferreddeclaration and “story-driven closure” semantics.Validation
validatenow emits non-blocking warnings for unknown/malformed/unreadable deferred-closure declarations.