fix(engine): emit defer record when the rollback pauses (#342)

[pbean]

Closes #342.

Problem

Engine._defer advances the task to terminal Phase.DEFERRED before recovering the tree. When
_rollback_or_pause pauses instead of returning, pause_for_manual_recovery persists that
terminal phase (recovery_flow.py saves before raising RunPaused), so the tail of _defer — the
story-deferred journal entry, the defer notification, the final save — is skipped permanently:
on resume _finish_inflight skips terminal tasks and _pick_next skips anything already in
state.tasks.

Validation of #342 confirmed every mechanical claim and found the reachability is wider than the
issue states: besides the preserve-ref failure path and the post-#341 snapshot path, the
production-default rollback-OFF arm (scm.rollback_on_failure=false) pauses on every
in-place defer with a dirty tree — no git failure needed. A second consumer the issue does not
name: bmad-loop diagnose derives defer_count from the journal, so it under-counts on this path
(phase-derived summary.deferred stays correct).

Fix — issue's shape 2 (catch + re-raise)

A narrow except RunPaused around the rollback inside _defer: emit the defer record, re-raise.
The record reads accurate post-pause state — shape 1 (record before rollback) would emit a
stale/absent preserve_ref (the rollback clears it and re-sets it only after preserving), and
shape 3 (advance after rollback) would leave a paused task at DEV_VERIFY/REVIEW_VERIFY, which
resume's _finish_inflight re-drives rather than re-entering _defer — contradicting the
defer decision. The catch is record-then-reraise, not a swallow; the comment names the deliberate
exception to the unwind-to-the-top doctrine.

One honesty detail: on the snapshot-failure pause path preserve_partial is latched while the
reset was refused, so _defer_recovery_note's "did not survive the rollback" claim would be
false. The except arm therefore uses a fixed pause-aware note pointing at the ACTION REQUIRED
notice (the authoritative pointer, written to the same ATTENTION file one entry earlier).

The three now-identical journal+notify+save sites (isolated arm, in-place tail, except arm) are
factored into one _record_defer helper.

Tests

• test_defer_record_survives_rollback_pause — full-run drive under the production default
  (rollback OFF, no monkeypatch): asserts the story-deferred entry, journal ordering
  (rollback-manual-required → story-deferred → run-paused), and the ATTENTION ordering
  (ACTION REQUIRED before the defer notice).
• test_defer_record_on_snapshot_failure_pause_stays_honest — the Decide whether a failed worktree snapshot should block the rollback reset #340/fix(recovery): refuse the rollback reset when the snapshot fails (#340) #341 path: commits parked,
  snapshot fails, reset refused; asserts the record names the parked attempt-preserve/* ref and
  that the false "did not survive the rollback" wording does not appear.
• Ablation run: with the except RunPaused arm deleted, both tests fail (no story-deferred
  entry at all).

Accepted risk (deliberate, not guarded)

journal.append/_save in the except arm on a dying disk (the very ENOSPC scenario behind the
snapshot path) would replace RunPaused with OSError, crashing rather than pausing. This is
consistent with pause_for_manual_recovery's own unguarded journal/notify/save one frame below —
a disk that dies between the pause's save and this append crashes the run either way.

Full suite: 3403 passed, 24 skipped. trunk check clean.

[coderabbitai]

Warning

Review limit reached

@pbean, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1223e3a1-d16d-4197-a405-4ac471676548

📥 Commits

Reviewing files that changed from the base of the PR and between e131bfa and 9fdc30b.

📒 Files selected for processing (4)

• CHANGELOG.md
• docs/FEATURES.md
• src/bmad_loop/engine.py
• tests/test_engine.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/342-defer-record-survives-pause

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR fixes a bug (#342) where Engine._defer advanced the task to terminal Phase.DEFERRED before recovering the tree, causing RunPaused from _rollback_or_pause to permanently skip the defer record on every pause path — rollback OFF (the production default) with a dirty tree, a preserve-ref failure, and the post-#341 snapshot failure.

• Wraps _rollback_or_pause in a narrow except RunPaused block inside _defer that emits the defer record before re-raising, with a custom honest note replacing the standard note's stale parked/destroyed claims (the tree was never touched on any pause path).
• Factors the three previously-duplicate journal+notify+save sites into a single _record_defer helper, with an optional note parameter to override the recovery-note tail on the pause path.
• Two new regression tests cover the rollback-OFF path and the snapshot-failure path, each with journal-ordering and ATTENTION-ordering assertions and named ablation targets.

Confidence Score: 5/5

The change is narrow and additive — a single try/except wrapping one existing call, a helper extraction, and two well-targeted tests. No existing code path is altered.

The fix is minimal and its scope is tightly bounded to _defer. The except RunPaused catches only the one exception class that signals a deliberate pause, re-raises it untouched, and the only side-effect added in the handler is the same journal+notify+save already performed on every other defer path. The ablation evidence in the PR description confirms the except arm is load-bearing. The accepted ENOSPC risk is pre-existing and acknowledged by the author.

Files Needing Attention: No files require special attention. The core logic change in engine.py is self-contained and thoroughly tested by tests/test_engine.py.

Important Files Changed

Filename	Overview
src/bmad_loop/engine.py	Adds _record_defer helper to consolidate three duplicate journal+notify+save sites, and wraps _rollback_or_pause in _defer with a narrow except RunPaused that emits the defer record before re-raising — the targeted fix for the lost-record bug.
tests/test_engine.py	Adds two targeted regression tests: rollback-OFF (production default) path and snapshot-failure path. Both check journal ordering, ATTENTION ordering, and the honesty of the note text; ablation targets named in docstrings.
CHANGELOG.md	New 'Fixed' entry for #342 inserted above the existing #343 entry; accurately describes the affected paths and the fix shape.
docs/FEATURES.md	Plateau-defer bullet extended with one sentence covering the #342 case (recovery-pausing defer now emits the record and points at the ACTION REQUIRED notice).

_{Reviews (1): Last reviewed commit: "test(engine): cover defer-record surviva..." | Re-trigger Greptile}