From 3e09c8af99e90a61911bc4d154a08e4b4c8324a5 Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Wed, 22 Jul 2026 22:51:47 -0700 Subject: [PATCH] feat: Crash recovery salvage: persist verified candidate + resume (not rebuild) + orphan sweep .gN fix (#91) --- coder_seam.py | 29 ++++++ loop.py | 102 +++++++++++++++++-- store.py | 46 +++++++++ tests/test_coder_seam.py | 92 +++++++++++++++++ tests/test_loop.py | 215 +++++++++++++++++++++++++++++++++++++++ tests/test_store.py | 36 +++++++ tests/test_worktree.py | 18 ++++ worktree.py | 22 ++++ 8 files changed, 553 insertions(+), 7 deletions(-) diff --git a/coder_seam.py b/coder_seam.py index 611191a..f9fc966 100644 --- a/coder_seam.py +++ b/coder_seam.py @@ -873,6 +873,11 @@ async def _run_acceptance_tests(self, candidate_wt: str): RecordGens = Callable[[int], None] +# The verify-boundary salvage record (#91): called with (branch, sha, worktree) once a +# candidate has PASSED its acceptance tests and been promoted — the loop persists it on +# the bead (store.record_verified_candidate) so a crash between here and open_pr can +# resume the verified build instead of rebuilding fresh. +RecordVerified = Callable[[str, str, str], None] def _record_gens_best_effort(record_gens: RecordGens, fid: str, n: int) -> None: @@ -912,6 +917,8 @@ async def dispatch( fusion_max_file_chars: int = FUSION_MAX_FILE_CHARS_DEFAULT, env_passthrough: Iterable[str] = (), tier: str = "", + record_verified: RecordVerified | None = None, + commit_message: str = "", _solve=None, _budget_cls=None, _verdict_cls=None, @@ -930,6 +937,12 @@ async def dispatch( ``record_gens`` (if given) is called with ``result.gens_spent`` exactly once, win or lose — the cost accounting (ADR 0064) must survive a failed search too. + ``record_verified`` (if given) is called once with the promoted winner's + ``(branch, sha, worktree)`` at the verify boundary — the crash-salvage record + (#91), persisted by the loop so a crash between here and ``open_pr`` resumes the + verified build instead of rebuilding fresh. ``commit_message`` names the commit + that gives the verified tree its sha (the loop passes the PR title, so the + shipped commit message is unchanged from what ``open_pr`` would have written). ``_solve``/``_budget_cls``/``_verdict_cls`` are test-injection seams for ``solve()``/``Budget``/``Verdict``; production callers never pass them (the real import happens here, deferred so this module carries no hard dependency on the @@ -1041,6 +1054,22 @@ async def dispatch( win_wt = result.solution win_branch = next(b for wt, b in adapter.candidates if wt == win_wt) canon_wt, canon_branch = await worktree.promote_worktree(repo, win_wt, win_branch, fid, root) + # The verify boundary's crash-salvage record (#91): the candidate PASSED its + # acceptance tests but open_pr is still ahead (fixups + the pre-PR gate can take + # minutes) — a crash in that window used to throw the whole verified build away. + # Commit the verified tree so its content has a real sha, then persist + # {branch, sha, worktree} via `record_verified` (the loop writes it on the bead) + # so recovery can resume at promote→fixups→gate→open_pr instead of re-solving. + # Best-effort: a bookkeeping failure must never fail a build that already passed. + if record_verified is not None: + try: + await worktree.commit_worktree(canon_wt, commit_message or f"feat: {fid} (verified candidate)") + rc, head, _err = await worktree._git(canon_wt, "rev-parse", "HEAD") + sha = (head or "").strip() + if rc == 0 and sha: + record_verified(canon_branch, sha, canon_wt) + except Exception: # noqa: BLE001 — fire-and-forget salvage bookkeeping + log.warning("[project_board] %s could not record the verified candidate (ignored)", fid, exc_info=True) for wt, branch in adapter.candidates: if wt != win_wt: await worktree.remove_worktree(repo, wt, branch) diff --git a/loop.py b/loop.py index 7c15d90..48cbd68 100644 --- a/loop.py +++ b/loop.py @@ -518,18 +518,94 @@ async def stop(self): # ── crash recovery (runs once, before the puller claims new work) ────────── async def _reconcile_orphan(self, fid: str): """A claimed feature with no live drive: if its PR actually got opened (a crash - between ``open_pr`` and ``open_review``) adopt it → ``in_review``; otherwise - reset it to ``ready`` for a clean rebuild (a stale worktree is cleaned when the + between ``open_pr`` and ``open_review``) adopt it → ``in_review``; else, if a + VERIFIED candidate was recorded at coder_seam's verify boundary and still checks + out on disk (a crash between verify and ``open_pr``), salvage it — resume at + promote → fixups → gate → open_pr instead of re-solving (#91); otherwise reset + it to ``ready`` for a clean rebuild (a stale worktree is cleaned when the puller re-claims it). Shared by boot recovery and the health sweep.""" store = self._store() pr_url = await worktree.pr_url_for_branch(f"feat/{fid}", cwd=self._store_kw["repo"]) if pr_url: store.open_review(fid, pr_url=pr_url) log.info("[project_board] %s already had a PR → in_review (%s)", fid, pr_url) + elif await self._salvage_verified_candidate(store, fid): + pass # resumed + PR opened → in_review (logged inside) else: store.requeue(fid) log.info("[project_board] %s reset to ready (no PR — rebuild fresh)", fid) + @staticmethod + def _clear_verified(store, fid: str) -> None: + """Best-effort drop of the salvage record — bookkeeping only, never raises.""" + try: + store.clear_verified_candidate(fid) + except Exception: # noqa: BLE001 — a failed clear must not fail recovery + log.warning("[project_board] %s clear_verified_candidate failed (ignored)", fid, exc_info=True) + + async def _salvage_verified_candidate(self, store, fid: str) -> bool: + """Crash salvage (#91): resume a build whose candidate already PASSED its + acceptance tests but crashed before ``open_pr``. + + ``coder_seam.dispatch`` records the verified candidate at its verify boundary + (a ``verified:`` label + a bead comment with {branch, sha, worktree}). If + that record still checks out EXACTLY — the canonical worktree dir exists, it + has the recorded branch checked out at the recorded sha, and the pre-PR gate + passes on it NOW — resume the tail of the drive (promote → fixups → gate → + open_pr → in_review) instead of throwing a verified build away to re-solve. + ANY doubt (no record, worktree gone, branch/sha drift, gate red now, any error + anywhere) → False, and the caller falls through to today's rebuild-fresh + unchanged — a wrong salvage ships unverified code; a skipped one only costs a + rebuild.""" + try: + f = store.get_feature(fid) or {} + sha = str(f.get("verified_sha") or "").strip() + if not sha: + return False + repo = f.get("repo") or self._store_kw["repo"] + base = f.get("base_branch") or self._store_kw.get("base_branch") or "main" + branch = f"feat/{fid}" + wt = os.path.join(repo, self.root, f"feat-{fid}") + if not os.path.isdir(wt): + log.info("[project_board] %s salvage: verified worktree gone — rebuild fresh", fid) + self._clear_verified(store, fid) + return False + rc, head, _err = await worktree._git(wt, "rev-parse", "HEAD") + if rc != 0 or head.strip() != sha: + log.info( + "[project_board] %s salvage: sha drift (%s ≠ recorded %s) — rebuild fresh", + fid, + head.strip()[:12], + sha[:12], + ) + self._clear_verified(store, fid) + return False + rc, cur, _err = await worktree._git(wt, "rev-parse", "--abbrev-ref", "HEAD") + if rc != 0 or cur.strip() != branch: + log.info("[project_board] %s salvage: branch drift (%s ≠ %s) — rebuild fresh", fid, cur.strip(), branch) + self._clear_verified(store, fid) + return False + # Resume the drive's tail in its normal order: promote (a no-op — the + # record is written post-promote, so the candidate already holds the + # canonical name) → fixups → gate → open_pr. The gate re-runs NOW: a + # candidate that verified before the crash but fails today (base moved, + # env changed) is a doubt, not a ship. + wt, branch = await worktree.promote_worktree(repo, wt, branch, fid, self.root) + await self._run_fixups(wt) + if await self._run_local_gate(wt) is not None: + log.info("[project_board] %s salvage: gate fails on the candidate now — rebuild fresh", fid) + self._clear_verified(store, fid) + return False + title = f"feat: {f.get('title') or fid}" + pr_url = await worktree.open_pr(wt, branch, base=base, title=title, body=_pr_body("", f)) + store.open_review(fid, pr_url=pr_url) + self._clear_verified(store, fid) + log.info("[project_board] %s salvaged the verified candidate → %s (no re-solve)", fid, pr_url) + return True + except Exception: # noqa: BLE001 — ANY doubt/error → today's rebuild-fresh path + log.warning("[project_board] %s salvage attempt failed — rebuild fresh", fid, exc_info=True) + return False + async def _recover(self): """On boot, reconcile every ``in_progress`` feature the previous run left mid-drive (a drive doesn't survive a restart). ``in_review`` features are NOT @@ -567,16 +643,22 @@ async def _sweep(self): await self._reconcile_orphan(fid) except Exception: # noqa: BLE001 log.warning("[project_board] sweep reconcile for %s failed", fid, exc_info=True) - for fid in worktree.list_feature_worktrees(repo, self.root): + for wtid in worktree.list_feature_worktrees(repo, self.root): + # A `.gN`/`.cN` candidate worktree is not a feature id (bd-1cp.g1) — its + # board state lives on the PARENT feature, so resolve through that (#91): + # skip while the parent's drive is live, reap when the parent is gone or + # done. The old raw-id `get_feature` lookup failed every sweep and just + # warned forever without ever reaping the candidate. + fid = worktree.parent_feature_id(wtid) if fid in self._inflight_files: - continue # a live drive owns this worktree + continue # a live drive owns this worktree (or its candidates) try: f = store.get_feature(fid) if f is None or f["board_state"] == "done": - await worktree.reap_feature_worktree(repo, self.root, fid) - log.info("[project_board] sweep: reaped orphaned worktree feat-%s", fid) + await worktree.reap_feature_worktree(repo, self.root, wtid) + log.info("[project_board] sweep: reaped orphaned worktree feat-%s", wtid) except Exception: # noqa: BLE001 - log.warning("[project_board] sweep reap for %s failed", fid, exc_info=True) + log.warning("[project_board] sweep reap for %s failed", wtid, exc_info=True) # ── the puller ──────────────────────────────────────────────────────────── async def _run(self): @@ -963,6 +1045,12 @@ async def _drive(self, feature: dict): # get — keep the whitelist consistent across every subprocess. env_passthrough=self.env_passthrough, tier=tier, # #84: label each solve gen with the current tier + # #91: persist the verified candidate on the bead at the + # verify boundary, so a crash before open_pr is salvageable. + record_verified=lambda br_name, sha, wt_path: store.record_verified_candidate( + fid, branch=br_name, sha=sha, worktree=wt_path + ), + commit_message=title, ) self._inflight[fid] = (repo, wt, branch) elif self.max_mode_n > 1 and not self._ci_feedback.get(fid): diff --git a/store.py b/store.py index 3d1c2c2..8ef2064 100644 --- a/store.py +++ b/store.py @@ -90,6 +90,14 @@ # seam) — `gens:`, replaced (not accumulated as separate labels) each time so a # single label always carries the running total for `portfolio_rollup` to read. LABEL_GENS_PREFIX = "gens:" +# Crash-salvage record (#91) — `verified:`, replaced (never accumulated) each time +# coder.solve()'s verify boundary promotes a test-PASSING candidate. Written on the bead +# (not loop memory) so it survives a crash between verify and open_pr; recovery's no-PR +# path checks it and resumes at promote→fixups→gate→open_pr instead of rebuilding fresh. +# The branch/worktree are the CANONICAL `feat/` / `feat-` names (the record is +# written post-promote), so the sha is the only piece that must ride the label; the full +# {branch, sha, worktree} triple lands in a comment for the audit trail. +LABEL_VERIFIED_PREFIX = "verified:" # difficulty → initial model tier (the escalation ladder's first rung, D10). DIFFICULTY_TIER = {"small": "smart", "medium": "reasoning", "large": "reasoning", "architectural": "opus"} @@ -685,6 +693,37 @@ def record_gens_spent(self, fid: str, n: int) -> dict: self._run(*args) return self.get_feature(fid) + # ── verified-candidate salvage record (#91) ─────────────────────────────── + def record_verified_candidate(self, fid: str, *, branch: str, sha: str, worktree: str) -> dict: + """Persist the verified candidate's identity — a single, replaced + `verified:` label (the `gens:` pattern) plus a comment carrying the full + {branch, sha, worktree} — written at coder_seam's verify boundary so a crash + between verify and open_pr can salvage the already-test-passing build instead + of rebuilding fresh. Fire-and-forget like record_gens_spent: a `br` hiccup + here must never fail a build whose tests already passed.""" + f = self._require(fid) + args = ["update", fid] + for stale in [l for l in f.get("labels") or [] if l.startswith(LABEL_VERIFIED_PREFIX)]: + args += ["--remove-label", stale] + args += ["--add-label", f"{LABEL_VERIFIED_PREFIX}{sha}"] + self._run(*args) + self._comment(fid, f"verified candidate: branch={branch} sha={sha} worktree={worktree}") + return self.get_feature(fid) + + def clear_verified_candidate(self, fid: str) -> dict: + """Drop the `verified:` salvage record — the crash window it covers has closed + (the PR opened) or the record failed its recovery checks (worktree/sha drift), + so it must not linger to confuse a later recovery. No-op without the label.""" + f = self._require(fid) + stale = [l for l in f.get("labels") or [] if l.startswith(LABEL_VERIFIED_PREFIX)] + if not stale: + return f + args = ["update", fid] + for label in stale: + args += ["--remove-label", label] + self._run(*args) + return self.get_feature(fid) + # ── reads (the projection) ──────────────────────────────────────────────── def get_feature(self, fid: str) -> dict | None: rows = self._run("show", fid, want_json=True) @@ -835,6 +874,12 @@ def _project(self, bead: dict) -> dict: ), 0, ) + # The crash-salvage record (#91): the sha of the last test-verified candidate, + # from the single replaced `verified:` label — "" when none was recorded. + verified_sha = next( + (l[len(LABEL_VERIFIED_PREFIX) :] for l in labels if l.startswith(LABEL_VERIFIED_PREFIX)), + "", + ) # `dag_blocked`: marked `ready` but a `blocks` dependency is still open, so # the puller won't claim it. Only `br show` carries dependencies (`br list` # doesn't); list_features patches this by cross-referencing the puller. @@ -864,6 +909,7 @@ def _project(self, bead: dict) -> dict: "difficulty": diff, "attempts": attempts, "gens_spent": gens_spent, + "verified_sha": verified_sha, "labels": labels, "repo": self.repo, "base_branch": self.base_branch, diff --git a/tests/test_coder_seam.py b/tests/test_coder_seam.py index 6fa4ca7..30efd44 100644 --- a/tests/test_coder_seam.py +++ b/tests/test_coder_seam.py @@ -224,6 +224,98 @@ async def _fake_solve(task, *, generate, verify, budget, k, tree_depth, fusion_g assert gens == [1] +async def test_dispatch_records_the_verified_candidate_at_the_verify_boundary(monkeypatch): + """#91: once a candidate PASSES its tests and is promoted, dispatch() commits the + verified tree (so its content has a real sha — the loop's PR title keeps the + shipped commit message unchanged) and hands {branch, sha, worktree} to + ``record_verified`` — the crash-salvage record recovery resumes from if the + process dies before open_pr.""" + _stub_worktree(monkeypatch) + committed = [] + + async def _commit(wt, message): + committed.append((wt, message)) + + monkeypatch.setattr(worktree, "commit_worktree", _commit) + + async def _git(wt, *args, timeout=60): + assert args == ("rev-parse", "HEAD") and wt == "/wt/feat-bd-1" + return (0, "abc123\n", "") + + monkeypatch.setattr(worktree, "_git", _git) + + async def _fake_solve(task, *, generate, verify, budget, k, tree_depth, fusion_generate=None, fusion_k=2): + c0 = await generate(task, feedback=None) + return _FakeResult(solution=c0, passed=True, rung="greedy", gens_spent=1, candidates_tried=1) + + recorded = [] + wt, branch, _result = await dispatch( + task="t", + coder=object(), + repo="/repo", + base="main", + root=".worktrees", + fid="bd-1", + dispatch_timeout=None, + test_cmd="pytest -q", + test_timeout=30, + budget=6, + k=3, + tree_depth=2, + record_verified=lambda b, s, w: recorded.append((b, s, w)), + commit_message="feat: the title", + _solve=_fake_solve, + _budget_cls=_FakeBudget, + _verdict_cls=_FakeVerdict, + ) + assert committed == [("/wt/feat-bd-1", "feat: the title")] # the CANONICAL (promoted) tree + assert recorded == [("feat/bd-1", "abc123", "/wt/feat-bd-1")] + assert (wt, branch) == ("/wt/feat-bd-1", "feat/bd-1") + + +async def test_dispatch_returns_the_winner_even_if_record_verified_raises(monkeypatch): + """The salvage record is fire-and-forget bookkeeping (like record_gens): a `br` + hiccup persisting it must never discard a build whose tests already passed.""" + _stub_worktree(monkeypatch) + + async def _commit(wt, message): + pass + + monkeypatch.setattr(worktree, "commit_worktree", _commit) + + async def _git(wt, *args, timeout=60): + return (0, "abc123\n", "") + + monkeypatch.setattr(worktree, "_git", _git) + + async def _fake_solve(task, *, generate, verify, budget, k, tree_depth, fusion_generate=None, fusion_k=2): + c0 = await generate(task, feedback=None) + return _FakeResult(solution=c0, passed=True, rung="greedy", gens_spent=1, candidates_tried=1) + + def _boom(b, s, w): + raise RuntimeError("br hiccup: lock contention") + + wt, branch, _result = await dispatch( + task="t", + coder=object(), + repo="/repo", + base="main", + root=".worktrees", + fid="bd-1", + dispatch_timeout=None, + test_cmd="pytest -q", + test_timeout=30, + budget=6, + k=3, + tree_depth=2, + record_verified=_boom, + _solve=_fake_solve, + _budget_cls=_FakeBudget, + _verdict_cls=_FakeVerdict, + ) + assert (wt, branch) == ("/wt/feat-bd-1", "feat/bd-1") # dispatch() itself never raised + + async def test_dispatch_promotes_the_winner_even_if_record_gens_raises(monkeypatch): """`store.record_gens_spent` documents itself as fire-and-forget ("a br hiccup here must never fail the build the way a missing PR would") — a `BoardError` diff --git a/tests/test_loop.py b/tests/test_loop.py index 1f7e865..7e08072 100644 --- a/tests/test_loop.py +++ b/tests/test_loop.py @@ -36,6 +36,14 @@ def record_gens_spent(self, fid, n): self.gens_spent[fid] = self.gens_spent.get(fid, 0) + n return {"id": fid} + def record_verified_candidate(self, fid, *, branch, sha, worktree): + self.calls.append(("record_verified", fid, branch, sha, worktree)) + return {"id": fid} + + def clear_verified_candidate(self, fid): + self.calls.append(("clear_verified", fid)) + return {"id": fid} + def names(self): return [c[0] for c in self.calls] @@ -497,13 +505,19 @@ async def _fake_dispatch( fusion_max_file_chars=None, env_passthrough=(), tier="", + record_verified=None, + commit_message="", ): seen["fid"] = fid seen["test_cmd"] = test_cmd seen["task"] = task seen["env_passthrough"] = env_passthrough seen["tier"] = tier + seen["commit_message"] = commit_message record_gens(4) + # dispatch() calls this at the verify boundary (#91) — the loop must have + # threaded a recorder that lands the record on THIS feature's bead. + record_verified(f"feat/{fid}", "abc123", f"/wt/feat-{fid}") return (f"/wt/feat-{fid}", f"feat/{fid}", "[coder.solve rung=best-of-k gens=4] solved") monkeypatch.setattr(coder_seam, "_import_solve", lambda: object()) @@ -517,7 +531,10 @@ async def _open_pr(wt, branch, *, base, title, body): ) assert seen["fid"] == "bd-1" and seen["test_cmd"] == "pytest -q" assert "Add a thing" in seen["task"] # the same built prompt, not a different one + assert seen["commit_message"] == "feat: Add a thing" # the verified commit keeps the PR title assert store.gens_spent.get("bd-1") == 4 + # The verify-boundary salvage record (#91) landed on the bead via the store. + assert ("record_verified", "bd-1", "feat/bd-1", "abc123", "/wt/feat-bd-1") in store.calls assert ("open_review", "bd-1", "https://example/pr/42") in store.calls assert store.creates == [] # solve()'s own per-candidate worktrees replaced the single create @@ -1571,6 +1588,9 @@ def __init__(self, in_progress): def list_features(self, state=None): return self._in_progress if state == "in_progress" else [] + def get_feature(self, fid): + return {"id": fid} # no verified_sha → the salvage check declines cleanly + def open_review(self, fid, *, pr_url): self.calls.append(("open_review", fid, pr_url)) @@ -1609,6 +1629,169 @@ async def _pr_url(branch, *, cwd="."): assert all(c[1] != "bd-1" for c in store.calls) +# ── crash salvage: resume a verified candidate instead of rebuilding (#91) ────── + + +class _SalvageStore: + """A recovery store whose one in_progress feature carries a verified-candidate + record (the `verified:` label projected as ``verified_sha``).""" + + def __init__(self, feature): + self.feature = feature + self.calls = [] + + def list_features(self, state=None): + return [{"id": self.feature["id"]}] if state == "in_progress" else [] + + def get_feature(self, fid): + return self.feature + + def open_review(self, fid, *, pr_url): + self.calls.append(("open_review", fid, pr_url)) + + def requeue(self, fid): + self.calls.append(("requeue", fid)) + + def clear_verified_candidate(self, fid): + self.calls.append(("clear_verified", fid)) + + +def _salvage_git(*, head="abc123", branch="feat/bd-1"): + """A ``worktree._git`` fake answering the salvage probes (HEAD sha + branch).""" + + async def _git(wt, *args, timeout=60): + if args == ("rev-parse", "HEAD"): + return (0, head + "\n", "") + if args == ("rev-parse", "--abbrev-ref", "HEAD"): + return (0, branch + "\n", "") + return (0, "", "") + + return _git + + +async def _recover_salvage(monkeypatch, tmp_path, *, make_wt=True, head="abc123", gate_out=None): + """Run boot recovery over ONE in_progress feature with a recorded verified sha + of ``abc123``. Returns (store, promoted, opened, gates) for the assertions.""" + feature = { + "id": "bd-1", + "title": "Add a thing", + "repo": str(tmp_path), + "base_branch": "main", + "verified_sha": "abc123", + } + store = _SalvageStore(feature) + monkeypatch.setattr("project_board.loop.get_store", lambda **_kw: store) + + async def _no_pr(branch, *, cwd="."): + return "" + + monkeypatch.setattr(worktree, "pr_url_for_branch", _no_pr) + monkeypatch.setattr(worktree, "_git", _salvage_git(head=head)) + if make_wt: + (tmp_path / ".worktrees" / "feat-bd-1").mkdir(parents=True) + + promoted = [] + + async def _promote(repo, src_wt, src_branch, fid, root=".worktrees"): + promoted.append((src_wt, src_branch, fid)) + return (src_wt, src_branch) # already canonical → the real one no-ops too + + monkeypatch.setattr(worktree, "promote_worktree", _promote) + + opened = [] + + async def _open_pr(wt, branch, *, base, title, body): + opened.append((wt, branch, base, title)) + return "https://example/pr/91" + + monkeypatch.setattr(worktree, "open_pr", _open_pr) + + loop = BoardLoop({"repo": str(tmp_path)}) + gates = [] + + async def _gate(wt): + gates.append(wt) + return gate_out + + monkeypatch.setattr(loop, "_run_local_gate", _gate) + await loop._recover() + return store, promoted, opened, gates + + +async def test_recover_salvages_a_verified_candidate(monkeypatch, tmp_path): + """A crash between verify and open_pr: the recorded candidate's worktree exists, + its branch+sha match, and the gate passes NOW → resume at promote → fixups → + gate → open_pr → in_review. No re-solve, no rebuild-fresh requeue.""" + store, promoted, opened, gates = await _recover_salvage(monkeypatch, tmp_path) + assert promoted and promoted[0][1:] == ("feat/bd-1", "bd-1") # resumed at promote + assert gates # the gate re-ran on the candidate now + assert opened and opened[0][1] == "feat/bd-1" and opened[0][3] == "feat: Add a thing" + assert ("open_review", "bd-1", "https://example/pr/91") in store.calls + assert ("clear_verified", "bd-1") in store.calls # the record's window closed + assert ("requeue", "bd-1") not in store.calls # never fell through to rebuild + + +async def test_recover_salvage_worktree_gone_rebuilds_fresh(monkeypatch, tmp_path): + """The record exists but the worktree dir is gone → doubt → today's rebuild-fresh + path (requeue), with no PR opened off a missing tree.""" + store, promoted, opened, _gates = await _recover_salvage(monkeypatch, tmp_path, make_wt=False) + assert ("requeue", "bd-1") in store.calls + assert not promoted and not opened + assert ("clear_verified", "bd-1") in store.calls # stale record dropped + + +async def test_recover_salvage_sha_mismatch_rebuilds_fresh(monkeypatch, tmp_path): + """The worktree exists but its HEAD is not the recorded sha (someone/something + moved it since verify) → doubt → rebuild fresh, never ship the drifted tree.""" + store, promoted, opened, _gates = await _recover_salvage(monkeypatch, tmp_path, head="0ther5ha") + assert ("requeue", "bd-1") in store.calls + assert not promoted and not opened + + +async def test_recover_salvage_gate_failing_now_rebuilds_fresh(monkeypatch, tmp_path): + """Worktree+branch+sha all check out, but the pre-PR gate FAILS on the candidate + now (base moved, env changed) → doubt → rebuild fresh, no PR.""" + store, _promoted, opened, gates = await _recover_salvage(monkeypatch, tmp_path, gate_out="FAILED tests: boom") + assert gates # the gate did run against the candidate + assert not opened # ...and its failure stopped the salvage before open_pr + assert ("requeue", "bd-1") in store.calls + assert ("clear_verified", "bd-1") in store.calls + + +async def test_recover_salvage_open_pr_error_falls_back_to_rebuild(monkeypatch, tmp_path): + """ANY error inside the salvage (here: open_pr blowing up) must degrade to the + rebuild-fresh path, never crash recovery or strand the feature in_progress.""" + feature = {"id": "bd-1", "title": "T", "repo": str(tmp_path), "base_branch": "main", "verified_sha": "abc123"} + store = _SalvageStore(feature) + monkeypatch.setattr("project_board.loop.get_store", lambda **_kw: store) + + async def _no_pr(branch, *, cwd="."): + return "" + + monkeypatch.setattr(worktree, "pr_url_for_branch", _no_pr) + monkeypatch.setattr(worktree, "_git", _salvage_git()) + (tmp_path / ".worktrees" / "feat-bd-1").mkdir(parents=True) + + async def _promote(repo, src_wt, src_branch, fid, root=".worktrees"): + return (src_wt, src_branch) + + monkeypatch.setattr(worktree, "promote_worktree", _promote) + + async def _boom_pr(wt, branch, *, base, title, body): + raise worktree.WorktreeError("gh exploded") + + monkeypatch.setattr(worktree, "open_pr", _boom_pr) + loop = BoardLoop({"repo": str(tmp_path)}) + + async def _gate(wt): + return None + + monkeypatch.setattr(loop, "_run_local_gate", _gate) + await loop._recover() # must not raise + assert ("requeue", "bd-1") in store.calls + assert all(c[0] != "open_review" for c in store.calls) # nothing pretended a PR opened + + # ── periodic health sweep ─────────────────────────────────────────────────────── @@ -1662,6 +1845,38 @@ async def _reap(repo, root, fid): assert set(reaped) == {"bd-done", "bd-gone"} +async def test_sweep_treats_candidate_worktrees_by_parent_feature(monkeypatch): + """A leftover `.gN`/`.cN` candidate worktree is NOT a feature id (bd-1cp.g1) — the + sweep must resolve its PARENT feature's state (#91): parent done/gone → the + candidate is reaped (by its FULL worktree id, so the right dir+branch go); parent + with a live drive → left alone. And the store is never asked for the raw candidate + id — that lookup was the old warning-spam-every-sweep path.""" + store = _SweepStore(features={"bd-done": "done", "bd-live": "in_progress"}) + looked_up = [] + orig_get = store.get_feature + + def _spy(fid): + looked_up.append(fid) + return orig_get(fid) + + store.get_feature = _spy + monkeypatch.setattr("project_board.loop.get_store", lambda **_kw: store) + monkeypatch.setattr( + worktree, "list_feature_worktrees", lambda repo, root: ["bd-done.g1", "bd-done.c2", "bd-gone.g3", "bd-live.g1"] + ) + reaped = [] + + async def _reap(repo, root, fid): + reaped.append(fid) + + monkeypatch.setattr(worktree, "reap_feature_worktree", _reap) + loop = BoardLoop({}) + loop._inflight_files = {"bd-live": {"a.py"}} # bd-live's drive is live → its candidates stay + await loop._sweep() + assert set(reaped) == {"bd-done.g1", "bd-done.c2", "bd-gone.g3"} # full worktree ids + assert looked_up and all("." not in fid for fid in looked_up) # never the raw candidate id + + async def test_maybe_sweep_is_rate_limited(monkeypatch): loop = BoardLoop({"health_sweep_interval_s": 300}) calls = [] diff --git a/tests/test_store.py b/tests/test_store.py index 362c240..122eaff 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -170,6 +170,42 @@ def test_record_gens_spent_accumulates_and_replaces_the_old_label(make_board, mo assert ("update", "bd-1", "--remove-label", "gens:5", "--add-label", "gens:9") in br.calls +# ── verified-candidate salvage record (#91) ───────────────────────────────────── + + +def test_record_verified_candidate_replaces_the_label_and_comments(make_board, monkeypatch): + br = Br() + b = make_board(br) + monkeypatch.setattr(b, "get_feature", lambda fid: {"id": fid, "labels": []}) + monkeypatch.setattr(b, "_require", lambda fid: {"id": fid, "labels": ["verified:old5ha", "ready"]}) + b.record_verified_candidate("bd-1", branch="feat/bd-1", sha="abc123", worktree="/wt/feat-bd-1") + # single replaced label (the gens: pattern) — never two verified: labels at once + assert ("update", "bd-1", "--remove-label", "verified:old5ha", "--add-label", "verified:abc123") in br.calls + # the full triple rides a comment for the audit trail + comment = next(a for a in br.calls if a[0] == "comments") + assert "branch=feat/bd-1" in comment[3] and "sha=abc123" in comment[3] and "worktree=/wt/feat-bd-1" in comment[3] + + +def test_clear_verified_candidate_drops_the_label_and_noops_without_one(make_board, monkeypatch): + br = Br() + b = make_board(br) + monkeypatch.setattr(b, "get_feature", lambda fid: {"id": fid, "labels": []}) + monkeypatch.setattr(b, "_require", lambda fid: {"id": fid, "labels": ["verified:abc123", "ready"]}) + b.clear_verified_candidate("bd-1") + assert ("update", "bd-1", "--remove-label", "verified:abc123") in br.calls + br2 = Br() + b2 = make_board(br2) + monkeypatch.setattr(b2, "_require", lambda fid: {"id": fid, "labels": ["ready"]}) + b2.clear_verified_candidate("bd-1") + assert not br2.cmds("update") # nothing to drop → no br write + + +def test_project_exposes_verified_sha(make_board): + b = make_board(Br()) + assert b._project({"id": "x", "status": "in_progress", "labels": ["verified:abc123"]})["verified_sha"] == "abc123" + assert b._project({"id": "y", "status": "open", "labels": []})["verified_sha"] == "" + + # ── _project: the bead → feature view mapping ─────────────────────────────────── diff --git a/tests/test_worktree.py b/tests/test_worktree.py index 9fd7aca..be1a042 100644 --- a/tests/test_worktree.py +++ b/tests/test_worktree.py @@ -230,6 +230,24 @@ def test_list_feature_worktrees_absent_dir(tmp_path): assert worktree.list_feature_worktrees(str(tmp_path), "does-not-exist") == [] +# ── parent_feature_id: candidate worktrees resolve to their owning feature (#91) ── + + +def test_parent_feature_id_strips_candidate_suffixes(): + assert worktree.parent_feature_id("bd-1cp.g1") == "bd-1cp" # coder.solve candidate + assert worktree.parent_feature_id("bd-1.c2") == "bd-1" # Max-Mode candidate + assert worktree.parent_feature_id("bd-1.test") == "bd-1" # test-rung diagnostic + assert worktree.parent_feature_id("bd-1.test.g2") == "bd-1" # stacked: test-rung's own candidate + assert worktree.parent_feature_id("bd-1cp.g12") == "bd-1cp" # multi-digit gen + + +def test_parent_feature_id_leaves_canonical_ids_alone(): + assert worktree.parent_feature_id("bd-13w") == "bd-13w" + assert worktree.parent_feature_id("bd-1cp") == "bd-1cp" + # A dot that is NOT a candidate suffix is not a candidate marker — untouched. + assert worktree.parent_feature_id("bd-1.gx") == "bd-1.gx" + + # ── reap_feature_worktree: the shared id → worktree/branch reap ────────────────── diff --git a/worktree.py b/worktree.py index 754a7bf..ee05bd0 100644 --- a/worktree.py +++ b/worktree.py @@ -20,6 +20,7 @@ import json import logging import os +import re log = logging.getLogger("protoagent.plugins.project_board") @@ -191,6 +192,27 @@ async def promote_worktree( return os.path.abspath(canon_path), canon_branch +# Candidate-worktree id suffixes: `.g` (coder.solve candidates), `.c` (Max-Mode +# candidates), `.test` (the operator-only test-rung diagnostic — whose own candidates +# stack as `.test.g`). A real feature id never contains a dot, so stripping these is +# unambiguous. +_CANDIDATE_SUFFIX_RE = re.compile(r"\.(?:g\d+|c\d+|test)$") + + +def parent_feature_id(wt_id: str) -> str: + """The feature id that OWNS a `feat-` worktree — `wt_id` itself for a + canonical worktree, the `.gN`/`.cN`/`.test` suffixes stripped (repeatedly, for the + stacked `bd-1.test.g2` shape) for a candidate one. The health sweep resolves board + state through this so a leftover candidate worktree is reaped by its PARENT + feature's state instead of warning every sweep on a non-feature id (#91).""" + out = wt_id + while True: + stripped = _CANDIDATE_SUFFIX_RE.sub("", out) + if stripped == out: + return out + out = stripped + + def list_feature_worktrees(repo: str, worktrees_root: str) -> list[str]: """The feature ids that currently have a ``feat-`` worktree dir under ``/`` — for the health sweep's orphan check. Sync (a quick