refactor: Forge owns its checkouts

CHANGELOG.md

@@ -3,6 +3,22 @@
 All notable changes to devloop. Semver-ish: minor bumps add features,
 patch bumps fix behavior bugs. Tag = release.
 
+## Unreleased — Forge owns its checkouts
+
+- **Forge adapter interface changed** (custom adapters need a two-line
+  update): `start_work(number, branch) -> str` now allocates the build
+  checkout itself and returns its path; callers never name paths. New
+  `finish_work(number)` removes the checkout (no-op when the issue never
+  started). Fixes a live bug: `/retry` ran with the default
+  `workdir="."`, and the old adapter deleted that path — wiping the
+  repo checkout. Deleting caller-supplied paths is now structurally
+  impossible: the adapter only ever removes checkouts it created.
+- Fixed repair's verification round running in the default checkout
+  (`cwd="."`) instead of the PR branch's worktree
+- Fixed build prompts using `.format()`: braces in an issue body
+  (`def f(): return {'a': 1}`) crashed the build before the agent ran;
+  substitution is now replace-based, like review and repair
+
 ## v0.3.0 — repair phase
 
 - `pipeline.repair_rounds` (default 1, 0 = off): after pre-review finds

devloop/core.py

@@ -4,9 +4,7 @@
 the ledger protocol is devloop/ledger.py. This module owns only the
 build flow and the sweep that drives it."""
 
-import shutil
 import sys
-import tempfile
 from concurrent.futures import ThreadPoolExecutor
 
 from . import ledger
@@ -40,10 +38,13 @@
 
 
 def process_issue(cfg: Config, forge: Forge, runtime: AgentRuntime, issue: Issue,
-                  workdir: str = ".") -> Outcome:
+                  workdir: str | None = None) -> Outcome:
     """One build: prompt the agent, then hand the finished work to the
     delivery module. Agent-run failures report here; everything after a
-    successful run (gate, commit, conflict gate, PR) is deliver()'s job."""
+    successful run (gate, commit, conflict gate, PR) is deliver()'s job.
+    Owns the build's create/cleanup bracket: start_work → finish_work,
+    even when the run explodes. workdir=None means the Forge allocates
+    its own checkout; callers never name paths."""
     kind = cfg.kind_for(issue.labels)  # raises if triggers are not exclusive
     branch = f"devloop/issue-{issue.number}"
     # progress heartbeat: the issue timeline shows when a build starts and
@@ -53,28 +54,37 @@ def process_issue(cfg: Config, forge: Forge, runtime: AgentRuntime, issue: Issue
                   f"build started — attempt {ledger.count(forge, issue) + 1}/"
                   f"{cfg.pipeline.max_attempts}, kind `{kind}`, agent `{runtime.name}`, "
                   f"branch `{branch}`")
-    forge.start_work(issue.number, branch, workdir)
-    res = None
+    # Forge allocates the private checkout (one per build — parallel agents
+    # must never share a working tree); process_issue owns the cleanup bracket.
+    workdir = forge.start_work(issue.number, branch)
     try:
-        res = runtime.run(PROMPTS[kind].format(n=issue.number, title=issue.title, body=issue.body),
-                          cwd=workdir, timeout=cfg.pipeline.timeout)
-    except Exception as e:
-        # Timeout/explosion mid-run: no delivery, but the human must know.
-        ledger.failure(forge, issue, "agent", note=f"no PR opened ({type(e).__name__})", tail=str(e))
-        return Outcome(issue.number, branch, False, False)
-    if not res.ok:
-        # A failed agent run must not ship: no commit, no gate, no PR — the
-        # error tail goes to the issue for the human, the branch stays local.
-        ledger.failure(forge, issue, "agent", note="no PR opened", tail=res.output)
-        return Outcome(issue.number, branch, False)
-    out = deliver(cfg, forge, runtime, issue, branch, workdir, res.output)
-    if out.pr:
-        findings = review_pr(cfg, forge, runtime, out.pr, branch,
-                             issue_title=issue.title, issue_body=issue.body)
-        if findings and cfg.pipeline.repair_rounds > 0:
-            repair_pr(cfg, forge, runtime, out.pr, branch, workdir,
-                      issue.title, issue.body, findings)
-    return out
+        res = None
+        try:
+            res = runtime.run(
+                PROMPTS[kind]
+                .replace("{n}", str(issue.number))
+                .replace("{title}", issue.title)
+                .replace("{body}", issue.body),
+                cwd=workdir, timeout=cfg.pipeline.timeout)
+        except Exception as e:
+            # Timeout/explosion mid-run: no delivery, but the human must know.
+            ledger.failure(forge, issue, "agent", note=f"no PR opened ({type(e).__name__})", tail=str(e))
+            return Outcome(issue.number, branch, False, False)
+        if not res.ok:
+            # A failed agent run must not ship: no commit, no gate, no PR — the
+            # error tail goes to the issue for the human, the branch stays local.
+            ledger.failure(forge, issue, "agent", note="no PR opened", tail=res.output)
+            return Outcome(issue.number, branch, False)
+        out = deliver(cfg, forge, runtime, issue, branch, workdir, res.output)
+        if out.pr:
+            findings = review_pr(cfg, forge, runtime, out.pr, branch,
+                                 issue_title=issue.title, issue_body=issue.body)
+            if findings and cfg.pipeline.repair_rounds > 0:
+                repair_pr(cfg, forge, runtime, out.pr, branch, workdir,
+                          issue.title, issue.body, findings)
+        return out
+    finally:
+        forge.finish_work(issue.number)
 
 
 def rebase_stale(cfg: Config, forge: Forge, devloop_heads: list[str]) -> None:
@@ -173,24 +183,14 @@ def run_once(cfg: Config, forge: Forge, runtime: AgentRuntime) -> list[Outcome]:
     if not candidates:
         return []
 
-    # One private git worktree per build: parallel agents must not share a
-    # working tree (they race on git state). Removed when the build ends.
-    dirs: dict[int, str] = {}
-    parents: dict[int, str] = {}
-    for issue in candidates:
-        parents[issue.number] = tempfile.mkdtemp(prefix=f"devloop-{issue.number}-")
-        dirs[issue.number] = parents[issue.number] + "/tree"
-
     def worker(issue: Issue) -> Outcome:
         try:
-            return process_issue(cfg, forge, runtime, issue, dirs[issue.number])
+            return process_issue(cfg, forge, runtime, issue)
         except Exception as e:
             # One broken issue must not block the queue (head-of-line blocking
             # would retry it forever in watch mode and starve everything else).
             print(f"#{issue.number}: failed: {e}", file=sys.stderr)
             return Outcome(issue.number, f"devloop/issue-{issue.number}", False, False)
-        finally:
-            shutil.rmtree(parents[issue.number], ignore_errors=True)
 
     out: list[Outcome] = []
     with ThreadPoolExecutor(max_workers=slots) as pool:

devloop/forge/base.py

@@ -42,10 +42,17 @@ def comments(self, number: int) -> list[Comment]:
         raise NotImplementedError
 
     # --- write side ------------------------------------------------------
-    def start_work(self, number: int, branch: str, workdir: str = ".") -> None:
-        """Create `branch` from the default branch, in its own git worktree
-        at `workdir` (parallel builds each get a private checkout — agents
-        must never share a working tree)."""
+    def start_work(self, number: int, branch: str) -> str:
+        """Create `branch` from the default branch in a private checkout
+       (parallel builds must never share a working tree — agents race on
+        git state). Returns the checkout path. The Forge owns its checkouts:
+        callers never name paths, and the adapter only ever deletes a
+        checkout it created itself (finish_work)."""
+        raise NotImplementedError
+
+    def finish_work(self, number: int) -> None:
+        """Remove the checkout start_work made for this issue. No-op when
+        the issue never started (normal: kind_for can raise first)."""
         raise NotImplementedError
 
     def commit_all(self, message: str, workdir: str = ".") -> bool:

devloop/forge/github.py

@@ -23,6 +23,11 @@ def _run(args: list[str], cwd: str = ".") -> str:
 class GitHub(Forge):
     def __init__(self, repo: str) -> None:
         self.repo = repo
+        # checkout registry: issue number → worktree path. start_work and
+        # finish_work are the only code that creates or deletes these, so
+        # the adapter can never rmtree a path it did not create itself
+        # (the old caller-supplied-workdir interface allowed rmtree('.')).
+        self._checkouts: dict[int, str] = {}
 
     def issues_with_labels(self, labels: list[str]) -> list[Issue]:
         issues: list[Issue] = []
@@ -132,14 +137,18 @@ def is_collaborator(self, author: str) -> bool:
         return self._permission(author) in {"admin", "maintain", "write"}
 
     # --- git side (assumes the working tree IS the target repo) ----------
-    def start_work(self, number: int, branch: str, workdir: str = ".") -> None:
+    def start_work(self, number: int, branch: str) -> str:
+        """Create the build checkout: one git worktree per issue, path owned
+        by the adapter and registered for finish_work."""
         _run(["git", "fetch", "origin"])
         # heal crash leftovers: an uncleanly-removed worktree leaves admin
         # entries that block worktree add on the same branch/path forever
         _run(["git", "worktree", "prune"])
-        shutil.rmtree(workdir, ignore_errors=True)
+        parent = tempfile.mkdtemp(prefix=f"devloop-{number}-")
+        workdir = parent + "/tree"
         default = _run(["git", "symbolic-ref", "--short", "refs/remotes/origin/HEAD"]).strip()
         _run(["git", "worktree", "add", "-B", branch, workdir, default])
+        self._checkouts[number] = workdir
         # force when the remote branch already exists (a deferred or abandoned
         # build): this branch is devloop-owned, the worktree is fresh from main
         exists = subprocess.run(
@@ -148,6 +157,17 @@ def start_work(self, number: int, branch: str, workdir: str = ".") -> None:
         push = ["git", "push", "--force", "-u", "origin", branch] if exists \
             else ["git", "push", "-u", "origin", branch]
         _run(push, cwd=workdir)
+        return workdir
+
+    def finish_work(self, number: int) -> None:
+        """Remove this issue's checkout — only ever a path this adapter
+        created. Deleting caller-supplied paths (the old interface allowed
+        rmtree('.')) is structurally impossible."""
+        path = self._checkouts.pop(number, None)
+        if path is None:
+            return  # never started (normal) or already finished
+        shutil.rmtree(os.path.dirname(path), ignore_errors=True)
+        _run(["git", "worktree", "prune"])
 
     def commit_all(self, message: str, workdir: str = ".") -> bool:
         """Commit + push all changes. Returns True when the branch carries

devloop/repair.py

@@ -92,7 +92,7 @@ def repair_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int,
         # now on the branch, not the diff the fixer was handed
         vres = runtime.run(VERIFY_PROMPT.replace("{findings}", findings)
                                      .replace("{diff}", forge.pr_diff_by_number(pr_number)[:40000]),
-                           cwd=".", timeout=cfg.pipeline.timeout)
+                           cwd=workdir, timeout=cfg.pipeline.timeout)
         if vres.ok:
             forge.pr_comment(pr_number, f"**AI verify after repair {rnd}**\n\n"
                                      + vres.output.strip()[-TAIL:])

tests/test_devloop.py

@@ -125,7 +125,7 @@ def run(self, prompt, cwd, timeout):
 
     orig = core.process_issue
 
-    def spy(cfg, forge, runtime, issue, workdir="."):
+    def spy(cfg, forge, runtime, issue, workdir=None):
         processed.append((issue.number, workdir))
         return type("O", (), {"issue": issue.number, "branch": "", "delivered": True, "gate": True})()
 
@@ -292,6 +292,7 @@ def __init__(self, open_heads=(), pr_files=None):
         self.prs = []
         self.notes = []
         self.cwds = []
+        self.finished = []
         self._open_heads = list(open_heads)
         self._pr_files = pr_files or {}
 
@@ -304,8 +305,12 @@ def open_pr_head_branches(self):
     def pr_for_branch(self, _b):
         return None
 
-    def start_work(self, number, branch, workdir="."):
-        self.cwds.append(workdir)
+    def start_work(self, number, branch):
+        self.cwds.append(f"/fake/wt-{number}")
+        return self.cwds[-1]
+
+    def finish_work(self, number):
+        self.finished.append(number)
 
     def commit_all(self, message, workdir="."):
         return True
@@ -343,8 +348,10 @@ def run(self, prompt, cwd, timeout):
     out = core.run_once(Config(repo="o/r", pipeline=Pipeline(max_parallel=2)), forge, R())
     assert sorted(o.issue for o in out) == [1, 2]
     assert len(forge.prs) == 2
-    # two distinct private worktrees, neither the shared checkout
+    # two distinct private worktrees, neither the shared checkout —
+    # allocated by the Forge, cleaned up after each build
     assert len(set(forge.cwds)) == 2 and "." not in forge.cwds
+    assert sorted(forge.finished) == [1, 2]
 
 
 def test_conflict_gate_defers_overlapping_builds():
@@ -547,15 +554,15 @@ def fake_review(cfg, forge, runtime, pr, branch, issue_title="", issue_body=""):
 
     def fake_repair(cfg, forge, runtime, pr, branch, workdir, t, b, findings):
         calls.append(("repair", findings))
-
     review_pr, repair_pr = core.review_pr, core.repair_pr
     core.review_pr, core.repair_pr = fake_review, fake_repair
     try:
         runtime = type("R", (), {"name": "fake",
             "run": lambda self, *a, **k: type("Res", (), {"ok": True, "output": "work"})()})()
         for title in ("finds", "lgtm"):  # findings → repair; LGTM → review only
             forge = _cmd_forge()
-            forge.start_work = lambda *a, **k: None
+            forge.start_work = lambda *a, **k: "/fake/wt"
+            forge.finish_work = lambda *a, **k: None
             forge.commit_all = lambda msg, workdir: True
             issue = forge.issue(9)
             issue.title = title
@@ -565,6 +572,59 @@ def fake_repair(cfg, forge, runtime, pr, branch, workdir, t, b, findings):
     assert calls == [("review", "finds"), ("repair", "P1: wrong"), ("review", "lgtm")]
 
 
+def test_braced_issue_body_and_cleanup_on_failure():
+    """Braces in an issue body are data, not format fields (the .format()
+    crash is a regression); and finish_work runs even when the agent run
+    explodes mid-build."""
+    import devloop.core as core
+    from devloop.config import Config
+    from devloop.forge.base import Issue
+
+    class F(Forge):
+        def __init__(self):
+            self.finished, self.notes, self.prs = [], [], []
+
+        def start_work(self, n, branch): return f"/fake/wt-{n}"
+        def finish_work(self, n): self.finished.append(n)
+        def comment(self, n, body): self.notes.append(body)
+        def comments(self, n): return [Comment("x", b) for b in self.notes]
+        def commit_all(self, msg, workdir): return True
+        def pr_for_branch(self, b): return None
+        def open_pr_head_branches(self): return []
+        def branch_files(self, b): return []
+        def open_pr(self, branch, title, body): self.prs.append(branch)
+        def is_owner(self, a): return True
+        def is_maintainer(self, a): return True
+        def is_collaborator(self, a): return True
+
+    issue = Issue(5, "t5", "def f(): return {'a': 1} — braces are data", ["ai-fix"])
+    cfg = Config(repo="o/r", pipeline=Pipeline(review_rounds=0))
+
+    class R:
+        name = "fake"
+        def __init__(self, boom=False): self.boom = boom
+        def run(self, prompt, cwd, timeout):
+            if self.boom:
+                raise TimeoutError()
+            self.prompt, self.cwd = prompt, cwd
+            return type("Res", (), {"ok": True, "output": "work"})()
+
+    # braces survive substitution; the Forge-allocated cwd reaches the agent
+    f, r = F(), R()
+    core.process_issue(cfg, f, r, issue)
+    assert "{'a': 1}" in r.prompt  # body braces intact — .format() would crash
+    assert r.cwd == "/fake/wt-5"
+    assert f.prs == ["devloop/issue-5"]
+    assert f.finished == [5]
+
+    # agent run explodes → failure posted to the issue AND checkout cleaned up
+    f2, b = F(), R(boom=True)
+    out = core.process_issue(cfg, f2, b, issue)
+    assert not out.agent_ok
+    assert any(n.startswith("agent run FAILED") for n in f2.notes)
+    assert f2.finished == [5]
+
+
 def test_comment_commands():
     import devloop.core as core
     from devloop.config import Config
@@ -708,6 +768,7 @@ def test_version_bump():
     test_review_rounds_carry_prior_findings()
     test_repair_pushes_gates_and_verifies()
     test_build_flow_hands_review_findings_to_repair()
+    test_braced_issue_body_and_cleanup_on_failure()
     test_comment_commands()
     test_rebase_stage_rebases_clean_and_rebuilds_conflicts()
     test_version_bump()