fix(worktree): detach kept worktree so branch_per=run survives a kept failure (#138)

[pbean]

Fixes #138.

Problem

Under branch_per="run" + keep_failed=true, the first story to end deferred has its worktree preserved by close_unit_workspace — left checked out on the single shared run branch. git refuses to check out a branch already held by another worktree, so every subsequent story's open_unit_workspace fails and is deferred with zero dev activity (attempt=0, no sessions, 0 tokens). One kept failure turns an N-story run into a 1-story run, and the top-line summary reports "0 done, N deferred" — conflating 1 attempted-and-deferred story with N−1 never attempted.

Confirmed end-to-end: close_unit_workspace's keep-path (workspace.py) had no branch_per awareness; unit_branch_name returns the shared bmad-loop/<run_id> for every unit; the next unit's worktree_add(create=False) raises verify.GitError, which _run_isolated catches and defers (worktree-open-failed). A deferred unit returns normally, so _loop drains every remaining unit into the same collision.

Fix (issue's option 1 — auto-detach, "minimal")

When close_unit_workspace preserves a failed worktree under branch_per="run", it now runs git checkout --detach in it. This frees the shared branch name for the next story while preserving everything inspection needs — the working tree, uncommitted changes, and the branch ref (still at the kept commit). Subsequent stories mount the run branch normally and get genuine attempts.

Why detach over pausing (issue's option 2): unlike the #126 env-fault (unfixable by the tool), this collision is tool-fixable in one git command — pausing would just make the operator run that exact command before a resume helps. Detaching restores the intended branch_per=run continuation and degrades safely: if detach ever fails, the existing worktree-open-failed defer still surfaces the collision (no regression).

Changes

• verify.py — new checkout_detach(repo) helper (git checkout --detach), mirroring checkout_branch.
• workspace.py — close_unit_workspace gains detach_kept: bool; on the keep_failed path it detaches the kept worktree (best-effort, swallows GitError).
• engine.py — passes detach_kept=scm.branch_per == "run" only at the DEFER call site. A boolean flag (not branch_per) lets the two callers that must not detach omit it honestly.

Why the other two close_unit_workspace callers don't detach

• success / _merge_local: removes the worktree anyway → branch already freed.
• _keep_branch_and_escalate: keeps a DONE-but-merge-conflict branch for manual merge and immediately pauses the run. That path can't cascade — it halts before any sibling is dispatched, and on resume the re-armed unit's worktree is torn down by the resume-restart discard_worktree before any mount. A regression test (test_branch_per_run_escalation_pauses_without_dispatching_next_unit) locks this in.

Tests

• test_checkout_detach_frees_branch (test_verify_worktree.py) — detach frees the branch for a sibling mount while preserving the ref + dirty tree.
• test_branch_per_run_kept_failure_detaches_so_next_unit_runs (test_engine_worktree.py) — rewritten from the old test that encoded the bug (deferred==2, done==0) to the fixed behavior (deferred==1, done==1, no worktree-open-failed, kept worktree detached).
• test_branch_per_run_escalation_pauses_without_dispatching_next_unit — escalate-path scoping guard.

Full suite green (2272 passed, 1 skipped); trunk clean.

Summary by CodeRabbit

• Bug Fixes
  • Fixed deferred runs using branch_per=run and keep_failed=true, preventing kept failed worktrees from blocking subsequent stories.
  • Kept worktrees are now detached while preserving their contents and branch references for inspection.
  • Improved handling of branch collisions and merge-conflict escalations without incorrectly dispatching later work.
• Documentation
  • Added a changelog entry describing the worktree and shared-branch fix.

[coderabbitai]

Walkthrough

Changes

Kept worktree branch reuse

Layer / File(s)	Summary
Detach preserved worktrees src/bmad_loop/verify.py, src/bmad_loop/workspace.py, tests/test_verify_worktree.py	Adds detached-HEAD support for kept worktrees while preserving branch refs and uncommitted contents.
Reuse shared run branches src/bmad_loop/engine.py, tests/test_engine_worktree.py, CHANGELOG.md	Detaches kept failed worktrees for branch_per="run", allowing subsequent units to proceed while preserving escalation behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• branch_per=run + keep_failed=true: the first kept-failed story's worktree holds the run branch, so every subsequent story in a multi-story run insta-defers on a branch/worktree collision #138 — Directly addresses the shared branch collision caused by kept-failed worktrees.

Possibly related PRs

• bmad-code-org/bmad-loop#98 — Also changes close_unit_workspace failure patch handling.

Suggested reviewers: dracic

Poem

A bunny found a branch held tight,
And freed its name by candlelight.
The worktree stayed, its changes there,
While later hops could safely share.
Merge on, little branches—bright!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main fix: detaching kept worktrees so branch_per=run can continue after a kept failure.
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 fix/branch-per-run-detach-kept-worktree

---

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.

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_engine_worktree.py (1)

352-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider iterable unpacking instead of list concatenation.

As indicated by static analysis tools, you can use iterable unpacking here for slightly cleaner and more idiomatic list construction.

♻️ Proposed refactor

-    script = _defer_script(project, "1-1-a") + [
-        wt_dev_effect(project, "1-2-b"),
-        wt_review_effect(project, "1-2-b", clean=True),
-    ]
+    script = [
+        *_defer_script(project, "1-1-a"),
+        wt_dev_effect(project, "1-2-b"),
+        wt_review_effect(project, "1-2-b", clean=True),
+    ]

🤖 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 352 - 355, Update the script
construction in the _defer_script test setup to use iterable unpacking for the
returned entries instead of list concatenation, while preserving the existing
wt_dev_effect and wt_review_effect ordering and values.

Source: Linters/SAST tools

🤖 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 `@tests/test_engine_worktree.py`:
- Around line 352-355: Update the script construction in the _defer_script test
setup to use iterable unpacking for the returned entries instead of list
concatenation, while preserving the existing wt_dev_effect and wt_review_effect
ordering and values.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ab6f57c5-3f73-4fa1-acf1-26866f6bd92b

📥 Commits

Reviewing files that changed from the base of the PR and between df42177 and 23fe46c.

📒 Files selected for processing (6)

• CHANGELOG.md
• src/bmad_loop/engine.py
• src/bmad_loop/verify.py
• src/bmad_loop/workspace.py
• tests/test_engine_worktree.py
• tests/test_verify_worktree.py

[pbean]

@coderabbitai Thanks for the review. Declining the single RUF005 nitpick ([*a, …] unpacking vs _defer_script(...) + [...] at tests/test_engine_worktree.py:352), intentionally:

• Not enforced by this repo. pyproject.toml selects no RUF rules, and the PR's lint (trunk)/ruff check is green — this is your profile's suggestion, not the project's linter.
• Consistency. Line 352 deliberately matches the identical _defer_script(project, "1-1-a") + [...] idiom in the sibling test it's modeled on (test_worktree_defer_then_next_story_succeeds, line 333). Changing only 352 would diverge from it, and widening the diff to restyle an unrelated existing test isn't warranted for a trivial preference.

Leaving the concatenation as-is.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@pbean Understood — that's a fair rationale, thanks for the detail. Since RUF isn't selected in pyproject.toml and the pattern matches the existing sibling test (test_worktree_defer_then_next_story_succeeds), keeping the concatenation style for consistency makes sense here. I'll leave it as-is.

ʅ(◔◇◔)ʃ

---

✏️ Learnings added

Learnt from: pbean
URL: https://github.com/bmad-code-org/bmad-loop/pull/152

Timestamp: 2026-07-15T17:31:50.773Z
Learning: In this repository (bmad-loop), `pyproject.toml` does not select any `RUF` (Ruff) lint rules, so RUF005-style suggestions (e.g., preferring `[*a, ...]` unpacking over `list + [...]` concatenation) are not enforced by the project's linter and should not be raised as review nitpicks unless the maintainer opts into RUF rules. Additionally, tests/test_engine_worktree.py deliberately uses the `_defer_script(project, "1-1-a") + [...]` idiom consistently across similar test cases (e.g., test_worktree_defer_then_next_story_succeeds) for stylistic consistency.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.