From 6c3803ea91302838b44ca1900af6ba7f7db22792 Mon Sep 17 00:00:00 2001 From: TEMP Date: Sat, 12 Sep 2026 16:27:15 -0400 Subject: [PATCH] refactor: Forge owns its checkouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit start_work(number, branch) allocates the build checkout and returns its path; new finish_work(number) removes it (no-op when never started). Callers never name paths, so the adapter can never rmtree one it didn't create — fixes /retry wiping the checkout via the old workdir='.' path. process_issue owns the create/cleanup bracket; run_once sheds its worktree plumbing. Also: repair verifier runs in the PR worktree (was cwd='.'), and build prompts switch to replace-based substitution (braces in issue bodies crashed .format() before the agent ran). Adapter interface changed — custom adapters need a two-line update. python3 tests/test_devloop.py: all checks passed --- CHANGELOG.md | 16 +++++++++ devloop/core.py | 72 ++++++++++++++++++++-------------------- devloop/forge/base.py | 15 ++++++--- devloop/forge/github.py | 24 ++++++++++++-- devloop/repair.py | 2 +- tests/test_devloop.py | 73 +++++++++++++++++++++++++++++++++++++---- 6 files changed, 153 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0584b9..ea87947 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/devloop/core.py b/devloop/core.py index ea3e2a6..95fc36c 100644 --- a/devloop/core.py +++ b/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: diff --git a/devloop/forge/base.py b/devloop/forge/base.py index c30ab03..7522fd8 100644 --- a/devloop/forge/base.py +++ b/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: diff --git a/devloop/forge/github.py b/devloop/forge/github.py index a5f08f6..db63ca9 100644 --- a/devloop/forge/github.py +++ b/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 diff --git a/devloop/repair.py b/devloop/repair.py index aa68910..6bed91c 100644 --- a/devloop/repair.py +++ b/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:]) diff --git a/tests/test_devloop.py b/tests/test_devloop.py index 2879464..1d5a16b 100644 --- a/tests/test_devloop.py +++ b/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,7 +554,6 @@ 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: @@ -555,7 +561,8 @@ def fake_repair(cfg, forge, runtime, pr, branch, workdir, t, b, findings): "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()