refactor(engine): extract rollback/preserve cluster into RecoveryFlow (#244, PR 2/2)

[pbean]

Findings F-3 of the brownfield CLI/TUI refactor assessment — PR 2 of 2, completing #244 (PR 1, the worktree cluster, landed in #265). Requires PR 1 merged; verified on main (951383a).

What

Engine's rollback/preserve cluster is an independent recovery state machine. Carve it into a RecoveryFlow collaborator (src/bmad_loop/recovery_flow.py) built from narrow deps — repo paths, policy, run state, journal, run dir — plus a getter for the engine's worktree-swappable active workspace and a handful of engine callbacks. The collaborator never receives the whole Engine.

Methods moved (same names, minus the leading _, on the collaborator):

• _rollback_or_pause · _safe_reset · _restore_patch · _prune_preserve_refs
• _preserve_attempt_commits · _preserve_attempt_worktree
• _pause_for_manual_recovery · _protected_relpaths

engine.py shrinks by ~333 lines (38 insertions, 371 deletions).

Same pattern & hard constraints as PR 1 (#265)

• Delegators: Engine keeps same-name delegating privates, so test_engine*.py is byte-untouched and green (339 engine/worktree/plugin/stories tests pass).
• Subclasses unaffected: SweepEngine/StoriesEngine override nothing in this cluster and reach _rollback_or_pause/_safe_reset through the inherited delegators.
• Late-binding traps: emit is injected late-bound (a lambda, not the bound method) so a test's monkeypatched Engine._emit still wins on pre_rollback/post_rollback; workspace_get reads the engine's swappable workspace live.
• No import cycle: RunPaused is raised via the injected escalation_pause/escalate callbacks, so recovery_flow never imports engine.
• Behavior-preserving only. No CHANGELOG. The hook-bus cluster is left in place (explicitly out of scope).

Tests

New tests/test_recovery_flow.py — 24 tests exercising the collaborator in isolation (narrow deps + stub callbacks, no Engine): protected-path derivation, the auto-recover / pause / worktree / resolved-re-drive / git-fault branches of rollback_or_pause, recovery-ref preservation (commit + dirty-worktree), the three manual-recovery notice shapes, prune retention + per-family error handling, and restore_patch apply/escalate.

Verification

• tests/test_engine.py tests/test_engine_worktree.py tests/test_engine_plugin.py tests/test_stories_engine.py → 339 passed
• tests/test_recovery_flow.py → 24 passed
• Full suite → 2766 passed, 1 skipped. The 2 test_module_skills_sync[...bmad-loop-setup] failures are pre-existing local install-tree drift (a gitignored .agents/.claude assets/module.yaml out of sync with canonical src); untouched by this PR and not reproduced on a clean CI checkout.
• uvx pyright@1.1.411 → 0 errors · full trunk check → clean

Closes #244.

Summary by CodeRabbit

• Reliability
  • Improved rollback and recovery handling for failed or stopped attempts.
  • Preserves committed and uncommitted work before recovery when appropriate.
  • Supports safer resets while retaining relevant artifacts and untracked files.
  • Provides clearer manual-recovery guidance when automatic recovery is unavailable.
  • Restores latched patches after resets and escalates consistently if restoration fails.
  • Maintains recovery history and cleanup behavior for preserved work.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: be6a1bef-7275-4a39-b4f6-c5d2b90dbfa9

📥 Commits

Reviewing files that changed from the base of the PR and between 951383a and 5124a51.

📒 Files selected for processing (3)

• src/bmad_loop/engine.py
• src/bmad_loop/recovery_flow.py
• tests/test_recovery_flow.py

---

Walkthrough

Engine’s rollback, preservation, manual-recovery, and patch-restoration logic is extracted into a new RecoveryFlow collaborator. Engine keeps delegating methods, and a dedicated test suite covers the collaborator’s decision paths and failure handling.

Changes

Recovery flow extraction

Layer / File(s)	Summary
RecoveryFlow contract and Engine wiring src/bmad_loop/recovery_flow.py, src/bmad_loop/engine.py, tests/test_recovery_flow.py	Defines narrow recovery dependencies, wires RecoveryFlow into Engine, and adds isolated test infrastructure.
Rollback and preserve-ref operations src/bmad_loop/recovery_flow.py, tests/test_recovery_flow.py	Centralizes protected-path calculation, rollback decisions, safe resets, preserve-ref snapshots, and pruning, with coverage for clean, dirty, worktree, and failure cases.
Patch restoration and manual recovery src/bmad_loop/recovery_flow.py, tests/test_recovery_flow.py	Centralizes patch application, escalation, manual-recovery notices, journaling, state saving, and pausing behavior with success and failure tests.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• bmad-code-org/bmad-loop#86: Implements behavior on the patch-restore path extracted into RecoveryFlow.
• bmad-code-org/bmad-loop#93: Updates resolve/resume handling that invokes patch restoration and rollback behavior.
• bmad-code-org/bmad-loop#107: Overlaps on patch-restore gating, phase advancement, and escalation behavior.

Poem

I’m a rabbit with a reset plan,
Preserve the commits while you can.
Patches hop back, journals sing,
Manual pauses guard everything.
Engine delegates—neat and bright!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: extracting the rollback/preserve cluster into RecoveryFlow.
Linked Issues check	✅ Passed	The PR matches #244 by extracting rollback/preserve logic into a narrow-dependency RecoveryFlow and keeping Engine delegators intact.
Out of Scope Changes check	✅ Passed	The changes stay within the recovery extraction and its tests, with no unrelated modules or features introduced.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/engine-recovery-flow

---

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.}

[pbean]

@CodeRabbit review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.