diff --git a/CHANGELOG.md b/CHANGELOG.md index 8526bac..0bbf1f0 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 134e2ff..aba578d 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 b782bea..fcf4004 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 df61a5c..8956fa5 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 32c15fa..6b1f581 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 8573341..528668d 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