From 23fe46c081b454059404b9f1ddd52b9c3aed02b3 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 15 Jul 2026 08:59:23 -0700 Subject: [PATCH] fix(worktree): detach kept worktree so branch_per=run survives a kept failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under branch_per=run + keep_failed=true the first story to end deferred left its worktree 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 failed and insta-deferred with zero dev activity — one kept failure turned an N-story run into a 1-story run (#138). close_unit_workspace now detaches the kept worktree's HEAD when the run branch is shared (new detach_kept flag, wired only at the DEFER call site), freeing the branch name for the next story while preserving the working tree, uncommitted changes, and the branch ref for inspection. New verify.checkout_detach helper. Best effort: on failure the existing worktree-open-failed defer still surfaces the collision (no regression). The escalate-and-pause callers deliberately do not detach: that path halts the run rather than continuing, so no sibling is dispatched while the worktree holds the branch, and resume frees it via the resume-restart discard before any mount. Added a regression guard proving the escalation pauses without dispatching the next unit. --- CHANGELOG.md | 12 ++++++ src/bmad_loop/engine.py | 1 + src/bmad_loop/verify.py | 11 ++++++ src/bmad_loop/workspace.py | 20 +++++++++- tests/test_engine_worktree.py | 70 +++++++++++++++++++++++++++++++---- tests/test_verify_worktree.py | 25 +++++++++++++ 6 files changed, 131 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8526bac7..0bbf1f09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -145,6 +145,18 @@ PATH)`, the TUI notifies `multiplexer backend unavailable — launch/attach disa ### Fixed +- **`branch_per=run` + `keep_failed` no longer poisons a multi-story run after the first kept + failure (#138).** The first story to end deferred under `keep_failed=true` left its worktree + checked out on the single shared run branch, so every subsequent story's `git worktree add` + collided ("branch already checked out") and insta-deferred with zero dev activity — one kept + failure turned an N-story run into a 1-story run. A kept worktree under `branch_per=run` now + detaches its HEAD (`git checkout --detach`), freeing the shared branch name for the next story + while preserving the working tree, uncommitted changes, and the branch ref (still at the kept + commit) for inspection; subsequent stories mount the run branch normally and get genuine + attempts. Best effort — if the detach ever fails, the existing `worktree-open-failed` defer + still surfaces the collision (no regression). The escalate-and-pause path was already safe: it + halts the run rather than continuing, and resume frees the kept worktree before any sibling mounts. + - **Dev/review sessions can no longer livelock on their own wake nudges (#149).** The idle wake nudge is delivered as a submitted turn, so a session that merely _answers_ it ends in another result-less Stop — which refilled the nudge budget, re-armed the grace window, and diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 134e2ffb..aba578d6 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -561,6 +561,7 @@ def _integrate_unit(self, task: StoryTask, unit: UnitWorkspace) -> None: run_dir=self.run_dir, unit_key=task.story_key, delete_branch=scm.delete_branch, + detach_kept=scm.branch_per == "run", diff_max_file_bytes=self._failed_diff_max_bytes(), ) self.journal.append( diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index b782bea9..fcf4004a 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -647,6 +647,17 @@ def checkout_branch(repo: Path, name: str) -> None: raise GitError(f"git checkout {name} failed in {repo}: {out}") +def checkout_detach(repo: Path) -> None: + """Detach HEAD at its current commit, leaving working tree + index untouched. + + Frees a shared branch name held by a kept worktree so a sibling worktree can + check that branch out (git refuses a branch checked out in another worktree). + """ + rc, out = _git(repo, "checkout", "--detach") + if rc != 0: + raise GitError(f"git checkout --detach failed in {repo}: {out}") + + def worktree_remove(repo: Path, path: Path, force: bool = False) -> None: args = ["worktree", "remove"] if force: diff --git a/src/bmad_loop/workspace.py b/src/bmad_loop/workspace.py index df61a5cd..8956fa52 100644 --- a/src/bmad_loop/workspace.py +++ b/src/bmad_loop/workspace.py @@ -119,6 +119,7 @@ def close_unit_workspace( run_dir: Path, unit_key: str, delete_branch: bool = True, + detach_kept: bool = False, diff_max_file_bytes: int | None = None, ) -> Path | None: """Tear down (or preserve) a unit's worktree. @@ -130,6 +131,11 @@ def close_unit_workspace( and, if delete_branch, the branch deleted. Returns the patch path it wrote, or None. + detach_kept (branch_per=run only): when keep_failed preserves the worktree, + detach its HEAD so the shared run branch it holds is freed for the next unit + to mount — otherwise every later unit's `git worktree add` collides on the + already-checked-out branch. Best effort; see the keep_failed branch below. + diff_max_file_bytes caps the per-untracked-file size in that forensic patch (None = no cap); see verify.capture_diff. """ @@ -148,7 +154,19 @@ def close_unit_workspace( patch.parent.mkdir(parents=True, exist_ok=True) patch.write_text(diff, encoding="utf-8") if keep_failed: - return patch # leave the worktree + branch mounted + if detach_kept: + # branch_per=run shares one branch across the run; a kept worktree + # left checked out on it blocks every later unit's `git worktree + # add` (git refuses a branch checked out elsewhere). Detach HEAD to + # free the shared branch name while preserving the working tree, + # uncommitted changes, and the branch ref (still at the kept commit) + # for inspection. Best effort: on failure the later unit still + # surfaces the collision via the worktree-open-failed defer path. + try: + verify.checkout_detach(unit.path) + except verify.GitError: + pass + return patch # leave the worktree mounted (branch detached if shared) # success, or a failure we are not keeping: remove the worktree. A failed # tree is dirty, so force; a successful unit was committed + merged, so its diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index 32c15fa8..6b1f5810 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -342,17 +342,31 @@ def test_worktree_defer_then_next_story_succeeds(project): assert "change for 1-1-a" not in (project.project / "src.txt").read_text() -def test_branch_per_run_kept_failure_defers_next_unit_gracefully(project): - """branch_per=run shares one branch; a kept-failed unit holds it, so the - next unit can't mount it. That degrades to a deferral, never a crash.""" +def test_branch_per_run_kept_failure_detaches_so_next_unit_runs(project): + """branch_per=run shares one branch; keeping a kept-failed unit's worktree + checked out on it would block every later unit's mount and cascade the whole + run into never-attempted deferrals. close_unit_workspace detaches the kept + worktree's HEAD, freeing the shared branch so the next unit gets a genuine + attempt instead of insta-deferring on a collision (issue #138).""" commit_sprint(project, {"1-1-a": "ready-for-dev", "1-2-b": "ready-for-dev"}) - script = _defer_script(project, "1-1-a") + [wt_dev_effect(project, "1-2-b")] + script = _defer_script(project, "1-1-a") + [ + wt_dev_effect(project, "1-2-b"), + wt_review_effect(project, "1-2-b", clean=True), + ] engine, _ = make_engine(project, script, policy=wt_policy(branch_per="run", limits=_NO_DAMP)) summary = engine.run() - assert summary.deferred == 2 and summary.done == 0 and not summary.paused - assert "could not open worktree" in engine.state.tasks["1-2-b"].defer_reason - assert "worktree-open-failed" in journal_kinds(engine) + # 1-1-a defers (kept), but 1-2-b actually runs and lands — no collision cascade + assert summary.deferred == 1 and summary.done == 1 and not summary.paused + assert "worktree-open-failed" not in journal_kinds(engine) + assert engine.state.tasks["1-2-b"].phase == Phase.DONE + assert not engine.state.tasks["1-2-b"].defer_reason + assert "change for 1-2-b" in (project.project / "src.txt").read_text() + # the kept 1-1-a worktree is detached (freeing the shared run branch), while + # the branch ref itself survives for inspection + assert branch_exists(project.project, "bmad-loop/test-run") + kept = [p for p in worktree_list(project.project) if p.resolve() != project.project.resolve()] + assert len(kept) == 1 and current_branch(kept[0]) == "HEAD" def test_worktree_followup_damped_commits_and_integrates(project): @@ -442,6 +456,48 @@ def diverging_open(*a, **k): assert branch_exists(project.project, "bmad-loop/test-run/1-1-a") +def test_branch_per_run_escalation_pauses_without_dispatching_next_unit(project): + """Issue #138 scoping guard: the shared-branch collision cascade is a property + of the DEFER path, which *returns* and lets the loop dispatch the next unit + into the held branch. A merge-conflict escalation instead *pauses* the run + (RunPaused), so under branch_per=run no sibling is ever dispatched while the + kept worktree holds the shared branch — there is nothing to detach here, and + on resume the re-armed unit's worktree is freed by the resume-restart discard + (see test_worktree_crash_restart_discards_stale_worktree) before any mount.""" + commit_sprint(project, {"1-1-a": "ready-for-dev", "1-2-b": "ready-for-dev"}) + engine, _ = make_engine( + project, + [wt_dev_effect(project, "1-1-a"), wt_review_effect(project, "1-1-a", clean=True)], + policy=wt_policy(branch_per="run", merge_strategy="ff"), + ) + # diverge the target right after the (shared) worktree is cut so ff-only merge + # of 1-1-a cannot fast-forward → escalate + pause + import bmad_loop.engine as eng + + real_open = eng.open_unit_workspace + + def diverging_open(*a, **k): + unit = real_open(*a, **k) + if not (project.project / "diverge.txt").exists(): + (project.project / "diverge.txt").write_text("target moved\n") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "target diverges") + return unit + + eng.open_unit_workspace = diverging_open + try: + summary = engine.run() + finally: + eng.open_unit_workspace = real_open + + assert summary.paused and summary.escalated == 1 + assert engine.state.tasks["1-1-a"].phase == Phase.ESCALATED + # the run halted at the escalation: 1-2-b was never dispatched, so the + # shared-branch collision that cascades the DEFER path cannot arise here + assert "1-2-b" not in engine.state.tasks + assert "worktree-open-failed" not in journal_kinds(engine) + + # ----------------------------------------------------------------- resume diff --git a/tests/test_verify_worktree.py b/tests/test_verify_worktree.py index 85733416..528668d6 100644 --- a/tests/test_verify_worktree.py +++ b/tests/test_verify_worktree.py @@ -82,6 +82,31 @@ def test_worktree_remove_dirty_needs_force(project, tmp_path): assert not wt.exists() +def test_checkout_detach_frees_branch(project, tmp_path): + """A worktree checked out on a branch holds that branch — git refuses to mount + it elsewhere. Detaching the worktree's HEAD frees the branch name for a sibling + worktree while preserving the branch ref, the working tree, and uncommitted + changes (issue #138).""" + repo = project.project + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + (wt / "dirty.txt").write_text("uncommitted\n") # local edit that must survive + + # while 'feat' is checked out in wt, a sibling mount of it is refused + wt2 = tmp_path / "wt2" + with pytest.raises(verify.GitError): + verify.worktree_add(repo, wt2, "feat", create=False) + + verify.checkout_detach(wt) + + assert verify.current_branch(wt) == "HEAD" # detached + assert verify.branch_exists(repo, "feat") # branch ref preserved + assert (wt / "dirty.txt").read_text() == "uncommitted\n" # working tree preserved + # branch name is now free → the sibling mount succeeds + verify.worktree_add(repo, wt2, "feat", create=False) + assert wt2.is_dir() + + # ---------------------------------------------------------------- merge