refactor(engine): extract worktree isolation flow into WorktreeFlow (#244, PR 1/2) by pbean · Pull Request #265 · bmad-code-org/bmad-loop

pbean · 2026-07-22T21:31:45Z

PR 1 of 2 for #244 (findings F-3 + F-9a). Worktree cluster only — the rollback/preserve cluster and the plugin hook bus stay in engine.py (PR 2 / out of scope).

What

engine.py's worktree isolation/integration band (_isolated … _reopen_unit) is an independent state machine. It moves into a new src/bmad_loop/worktree_flow.py as a WorktreeFlow collaborator, built from narrow deps (paths, policy, run state, journal, plugin registry) + a handful of engine callbacks — never the whole Engine.

Engine keeps same-name private delegators (_run_isolated, _integrate_unit, _gc_run_worktrees, _ensure_target_branch, …) that forward to the collaborator. This is the acceptance criterion: tests/test_engine*.py are byte-untouched, and SweepEngine/StoriesEngine (which override nothing in this cluster) see an unchanged surface.

Callback seams (why they exist)

emit is late-bound (a lambda over self._emit) so a test's monkeypatched Engine._emit still fires from the worktree paths.
adapters (getter) and the open_unit_workspace module global are read live — two existing tests rebind them after construction (engine.adapters = …, eng.open_unit_workspace = …) and expect the flow to observe it.
escalation_pause raises the engine's RunPaused, injected so worktree_flow need not import engine (that would re-introduce a runtime↔engine import cycle).
workspace_get/set read and swap the engine's live self.workspace.

F-9a — runtime no longer imports the installer

provision_worktree (and the _setup_mcp_agent_id helper) move out so engine.py drops from .install import provision_worktree. install.py re-exports provision_worktree lazily via PEP 562 __getattr__ (a module-level re-export would form an install↔worktree_flow cycle, since the moved function still uses installer helpers). engine re-exports _setup_mcp_agent_id for test_engine_plugin.py.

Behavior

Behavior-preserving. No CHANGELOG (internal refactor).

Tests

Full suite green. (The 2 test_module_skills_sync::…bmad-loop-setup failures are pre-existing local skill-tree drift — they skip on a clean checkout and touch nothing in this diff.)
trunk check --no-fix clean.
New tests/test_worktree_flow.py exercises the collaborator in isolation (no Engine): target-branch resolution incl. detached-HEAD pause, deferral on worktree-open failure, escalate/notify/pause, profile+agent-id dedup with live-adapter rebind, and the provision_worktree re-export identity.

Summary by CodeRabbit

New Features
- Added isolated worktree support for running units independently and integrating completed work.
- Added automatic worktree setup with project files, skills, hooks, and configuration.
- Added improved handling for deferred, escalated, resumed, and failed worktree runs.
- Added cleanup of completed worktrees and protection against accidental inclusion of injected files.
Bug Fixes
- Improved recovery when worktree setup or merge operations fail.
- Added safeguards for detached branches and missing worktrees.
Tests
- Added comprehensive coverage for isolation, integration, escalation, resumption, cleanup, and configuration behavior.

…244) Engine's worktree-isolation/integration cluster (the `_isolated` .. `_reopen_unit` band) is an independent state machine. Carve it into a `WorktreeFlow` collaborator built from narrow deps (paths, policy, run state, journal, plugin registry) plus engine callbacks — never the whole Engine: - `emit` is late-bound (a lambda over `self._emit`) so a monkeypatched `Engine._emit` still wins; `adapters` and the `open_unit_workspace` module global are read live for the same reason (tests rebind both); - `escalation_pause` raises the engine's `RunPaused`, injected so worktree_flow need not import `engine` (which would reintroduce a runtime<->engine cycle); - `workspace_get/set` read and swap the engine's live workspace. Engine keeps same-name private delegators (`_run_isolated`, `_gc_run_worktrees`, …) so test_engine*.py and the SweepEngine/StoriesEngine subclasses stay byte-untouched and green. F-9a: rehome `provision_worktree` (and `_setup_mcp_agent_id`) out of the installer so the runtime control loop no longer imports `install`; `install.py` re-exports `provision_worktree` lazily (PEP 562 `__getattr__`) to avoid an install<->worktree_flow import cycle, and engine re-exports `_setup_mcp_agent_id` for its plugin tests. Behavior-preserving; rollback/preserve cluster (PR 2) untouched. Adds tests/test_worktree_flow.py exercising the collaborator in isolation.

coderabbitai · 2026-07-22T21:32:04Z

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: 64e8cdf5-b850-47cb-8ebe-29ca8337bca7

📥 Commits

Reviewing files that changed from the base of the PR and between 24890ad and 84b67d0.

📒 Files selected for processing (4)

src/bmad_loop/engine.py
src/bmad_loop/install.py
src/bmad_loop/worktree_flow.py
tests/test_worktree_flow.py

Walkthrough

Worktree provisioning and per-unit isolation are extracted into WorktreeFlow. Engine now wires and delegates to the collaborator, while installation compatibility, escalation handling, resume support, cleanup, and focused tests are added.

Changes

Worktree flow extraction

Layer / File(s)	Summary
Provisioning and module contracts `src/bmad_loop/worktree_flow.py`, `src/bmad_loop/install.py`, `src/bmad_loop/engine.py`	Worktree provisioning and MCP profile mapping move into `worktree_flow`, `install` lazily re-exports provisioning, and Engine defines the workflow completion contract.
Isolation and integration orchestration `src/bmad_loop/worktree_flow.py`, `src/bmad_loop/engine.py`	`WorktreeFlow` coordinates target branches, isolated execution, integration, escalation, cleanup, and reopening; Engine supplies callbacks and delegates its compatibility methods.
Flow behavior validation `tests/test_worktree_flow.py`	Tests cover policy behavior, profile and agent discovery, branch handling, isolated failures, escalation, reopening, cleanup, and compatibility exports.

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

Possibly related issues

Issue 244 — Directly covers extracting worktree isolation into a collaborator, moving provisioning, and retaining Engine delegators.

Possibly related PRs

bmad-code-org/bmad-loop#155 — Also modifies worktree provisioning and seeded path shielding.
bmad-code-org/bmad-loop#233 — Also journals skipped worktree seed entries during isolated runs.

Suggested reviewers: dracic

Poem

A rabbit hops through branches green,
While WorktreeFlow keeps paths clean.
Engine delegates, hooks ring bright,
Seeds and merges land just right.
Tests thump softly: all is neat! 🐇

🚥 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 summarizes the main refactor: extracting worktree isolation into WorktreeFlow.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ 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 feat/engine-worktree-collaborator

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

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

…bmad-code-org#244) Findings F-3 of the brownfield refactor assessment, PR 2/2 (PR 1 was the worktree cluster, bmad-code-org#265). Engine's rollback/preserve cluster is an independent recovery state machine; carve it into a `RecoveryFlow` collaborator built from narrow deps (repo paths, policy, run state, journal, run dir) plus a getter for the engine's swappable active workspace and a few engine callbacks (emit a plugin hook, save state, escalate a task, escalation-pause). The collaborator never receives the whole Engine. Same pattern as WorktreeFlow: Engine keeps same-name delegating private methods (`_rollback_or_pause`, `_safe_reset`, `_restore_patch`, `_prune_preserve_refs`, `_preserve_attempt_*`, `_pause_for_manual_recovery`, `_protected_relpaths`) so test_engine*.py stays byte-untouched and the SweepEngine/StoriesEngine subclasses (which override nothing in this cluster, and call `_rollback_or_pause` /`_safe_reset` through the inherited delegators) are unchanged. `emit` is late-bound so a monkeypatched `Engine._emit` still wins; `RunPaused` is raised via the injected `escalation_pause`/`escalate` callbacks so recovery_flow need not import engine (avoids a runtime<->engine cycle). Behavior-preserving only. New unit tests exercise the collaborator in isolation in tests/test_recovery_flow.py. engine.py shrinks by ~333 lines. The hook-bus cluster is left in place (explicitly out of scope). Closes bmad-code-org#244.

PR bmad-code-org#265 moved provision_worktree out of install.py into worktree_flow.py after this branch was cut, so the seeding-loop half of the change conflicted. Resolution: install.py keeps only the _copy_traversable changes (the helper still lives there and worktree_flow imports it); the provision_worktree docstring + dir-entry seeding branch are re-applied verbatim in worktree_flow.py. Tests and docs merged clean.

pbean merged commit 951383a into main Jul 22, 2026
10 checks passed

pbean mentioned this pull request Jul 22, 2026

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

Merged

pbean mentioned this pull request Jul 23, 2026

feat(worktree): seed the absent children of an existing seed dir (#235) #257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(engine): extract worktree isolation flow into WorktreeFlow (#244, PR 1/2)#265

refactor(engine): extract worktree isolation flow into WorktreeFlow (#244, PR 1/2)#265
pbean merged 1 commit into
mainfrom
feat/engine-worktree-collaborator

pbean commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

pbean commented Jul 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Callback seams (why they exist)

F-9a — runtime no longer imports the installer

Behavior

Tests

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pbean commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading