refactor: deepen PR-body contract, CLI event bracket, round engine

CONTEXT.md

@@ -38,7 +38,9 @@ in `devloop/spec.py`).
 
 Turning a finished agent run into a PR — or telling the issue why not. The
 **delivery module** (`devloop/delivery.py`) owns this interface: one function,
-`deliver()`, behind which live the verify gate, commit, half-delivery heal,
+`deliver()`, behind which live the verify gate (owned by its own module,
+`devloop/gate.py` — one gate policy for every caller), commit,
+half-delivery heal,
 the delivery conflict gate, self-delivery bookkeeping, and the PR body. It
 never raises: every failure path posts its own ledger comment and returns an
 `Outcome` (`pr=None` means nothing shipped). A silent delivery failure is a
@@ -136,7 +138,8 @@ code. Review returns its final findings — the build flow hands them to Repair.
 Acting on review findings before a human reads them. The **repair module**
 (`devloop/repair.py`) owns this interface: one function, `repair_pr()`, behind
 which live the fixer prompt (findings + diff + spec issue + PR thread, with
-repo guidance from `skills/repair/SKILL.md`), the verify gate before any push,
+repo guidance from `skills/repair/SKILL.md`), the verify gate before any push
+(`devloop/gate.py`, same policy as delivery),
 the commit-and-push (the fixer commits, the pipeline pushes), and the
 one-round verification re-review that decides fixed vs still open. Repair is
 not Review — review finds, repair acts; review stays findings-only. Repair is

devloop/cli.py

@@ -39,6 +39,16 @@ def _warn_bad_skills() -> None:
         print(f"warning: {w}", file=sys.stderr)
 
 
+def _event(args: argparse.Namespace) -> dict | None:
+    """Read the CI event payload ($GITHUB_EVENT_PATH or --event). None = not
+    a CI event context — the caller exits silently (zero token spend).
+    Handlers extract their own fields: command and merged events differ."""
+    path = args.event or os.environ.get("GITHUB_EVENT_PATH", "")
+    if not path or not Path(path).exists():
+        return None
+    return json.loads(Path(path).read_text())
+
+
 def _runtime(cfg):
     return get_forge(cfg.forge_kind, cfg.repo, cfg.base_url), get_runtime(cfg.runtime.engine, cfg.runtime.argv)
 
@@ -80,10 +90,9 @@ def cmd_command(_args: argparse.Namespace) -> None:
     """Execute one comment command. Runs from CI's issue_comment event:
     reads the event payload, access-gates the author, executes.
     Silent exit when the comment isn't a command (zero token spend)."""
-    path = _args.event or os.environ.get("GITHUB_EVENT_PATH", "")
-    if not path or not Path(path).exists():
-        return  # not a CI comment context — nothing to do
-    ev = json.loads(Path(path).read_text())
+    ev = _event(args)
+    if ev is None:
+        return
     comment = ev.get("comment") or {}
     body = (comment.get("body") or "").strip()
     if not body.startswith("/"):
@@ -106,10 +115,9 @@ def cmd_merged(_args: argparse.Namespace) -> None:
     """Close out an issue whose devloop PR a human just merged. Runs from
     CI's pull_request(closed, merged) event; silent exit otherwise (zero
     token spend — no agent run here, just forge calls)."""
-    path = _args.event or os.environ.get("GITHUB_EVENT_PATH", "")
-    if not path or not Path(path).exists():
+    ev = _event(args)
+    if ev is None:
         return
-    ev = json.loads(Path(path).read_text())
     pr = ev.get("pull_request") or {}
     if not pr.get("merged"):
         return

devloop/core.py

@@ -48,7 +48,7 @@ def process_issue(cfg: Config, forge: Forge, runtime: AgentRuntime, issue: Issue
     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}"
+    branch = forge.branch_for(issue.number)
     # per-kind runtime override: [runtime.<kind>] full argv wins for this
     # build; no section configured → the caller's global runtime
     agent = cfg.runtime.for_kind(kind) or runtime
@@ -80,10 +80,9 @@ def process_issue(cfg: Config, forge: Forge, runtime: AgentRuntime, issue: Issue
             # 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)
+        out = deliver(cfg, forge, runtime.name, 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)
+            findings = review_pr(cfg, forge, runtime, out.pr, issue)
             if findings and cfg.pipeline.repair_rounds > 0:
                 repair_pr(cfg, forge, runtime, out.pr, branch, workdir,
                           issue.title, issue.body, findings)
@@ -99,9 +98,8 @@ def rebase_stale(cfg: Config, forge: Forge) -> None:
     agent labor is cheaper than spending human conflict resolution."""
     for pr in forge.open_devloop_prs():
         head = pr.head
-        try:
-            n = int(head.rsplit("-", 1)[-1])
-        except ValueError:
+        n = forge.issue_of_branch(head)
+        if n is None:
             continue
         try:
             clean = forge.rebase_branch(head)
@@ -144,7 +142,7 @@ def handle_command(cfg: Config, forge: Forge, runtime: AgentRuntime,
             return f"reviewed PR #{pr}"
         if cmd == "/retry":
             n = int(arg) if arg else context_number
-            existing = forge.pr_for_branch(f"devloop/issue-{n}")
+            existing = forge.pr_for_branch(forge.branch_for(n))
             if existing:
                 # the human sanctioned discarding the delivery — devloop is
                 # executing that command, not judging the work itself
@@ -166,11 +164,8 @@ def handle_merge(cfg: Config, forge: Forge, pr_number: int, head_branch: str) ->
     issue. Same carve-out as close_pr — the merge IS the human's sanction;
     this fires only from a real forge merge event, never agent output.
     None = not a devloop PR (caller's YAML gate should already know)."""
-    if not head_branch.startswith("devloop/issue-"):
-        return None
-    try:
-        n = int(head_branch.rsplit("-", 1)[-1])
-    except ValueError:
+    n = forge.issue_of_branch(head_branch)
+    if n is None:
         return None
     ledger.merged(forge, n, pr_number)
     forge.complete_issue(n)
@@ -191,7 +186,7 @@ def worker(issue: Issue) -> Outcome:
             # 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)
+            return Outcome(issue.number, forge.branch_for(issue.number), False, False)
 
     out: list[Outcome] = []
     with ThreadPoolExecutor(max_workers=len(candidates)) as pool:

devloop/delivery.py

@@ -1,20 +1,21 @@
 """Delivery: gate the work, ship it as a PR, or tell the issue why not.
 
 One interface function: deliver(). Every rule about how finished agent work
-becomes a PR — the verify gate, commit, half-delivery heal, the delivery
-conflict gate, self-delivery bookkeeping, the PR body — lives behind it.
-Never raises: every failure path posts its own ledger comment and returns
-an Outcome, so a silent delivery failure is a bug in one place, not a
-forgotten except clause in a caller.
+becomes a PR — the verify gate (owned by devloop/gate.py), commit,
+half-delivery heal, the delivery conflict gate, self-delivery bookkeeping,
+the PR body — lives behind it. Never raises: every failure path posts its
+own ledger comment and returns an Outcome, so a silent delivery failure is
+a bug in one place, not a forgotten except clause in a caller.
 """
 
-import subprocess
+import re
 from dataclasses import dataclass
 
 from . import ledger
 from .config import Config
 from .forge import Forge, Issue
-from .runtime import TAIL, AgentRuntime
+from .gate import run_gate
+from .runtime import TAIL
 
 
 @dataclass
@@ -26,24 +27,23 @@ class Outcome:
     pr: int | None = None  # None = nothing shipped (failed, empty, or deferred)
 
 
-def deliver(cfg: Config, forge: Forge, runtime: AgentRuntime, issue: Issue,
+def deliver(cfg: Config, forge: Forge, agent_name: str, issue: Issue,
             branch: str, workdir: str, agent_output: str) -> Outcome:
     """Ship one finished agent run. The agent already succeeded (res.ok);
     everything from here to PR-or-ledger-comment is delivery."""
     kind = cfg.kind_for(issue.labels)
     gate_ok = True
+    if cfg.pipeline.verify:
+        # runs in the build's worktree — the gate judges what will be
+        # delivered, not the (possibly older) default checkout; the gate
+        # module owns the policy (timeout, PASS semantics)
+        gate_ok = run_gate(cfg.pipeline.verify, workdir, cfg.pipeline.timeout)
     try:
-        if cfg.pipeline.verify:
-            # runs in the build's worktree — the gate judges what will be
-            # delivered, not the (possibly older) default checkout
-            r = subprocess.run(cfg.pipeline.verify, shell=True,
-                               capture_output=True, text=True, cwd=workdir)
-            gate_ok = r.returncode == 0
         existing = forge.pr_for_branch(branch)
         # half-delivery rule lives behind the Forge seam: commit_all returns
         # True for staged, unpushed, or already-pushed-but-no-PR work.
         delivered = forge.commit_all(
-            f"devloop({kind}): fixes #{issue.number} [agent: {runtime.name}]", workdir)
+            f"devloop({kind}): fixes #{issue.number} [agent: {agent_name}]", workdir)
         if not existing and not delivered:
             # No diff AND no PR — nothing delivered. The agent said something —
             # that's the finding (question, verdict, or stall); surface it.
@@ -77,14 +77,7 @@ def deliver(cfg: Config, forge: Forge, runtime: AgentRuntime, issue: Issue,
             forge.open_pr(
                 branch,
                 title=f"devloop({kind}): {issue.title} (#{issue.number})",
-                body=(
-                    f"Closes #{issue.number}\n\n"
-                    f"- agent: `{runtime.name}`\n"
-                    f"- gate: {'PASS' if gate_ok else 'FAIL'}"
-                    + (f" (`{cfg.pipeline.verify}`)" if cfg.pipeline.verify else " (none configured)")
-                    + "\n\nHuman merge required — agents never merge."
-                    + "\n\n## Agent report\n\n" + agent_output[-TAIL:].strip()
-                ),
+                body=_pr_body(issue, agent_name, gate_ok, cfg.pipeline.verify, agent_output),
             )
             forge.comment(issue.number, f"Work delivered on `{branch}` — gate {'PASS' if gate_ok else 'FAIL'}.")
         return Outcome(issue.number, branch, True, gate_ok,
@@ -95,3 +88,25 @@ def deliver(cfg: Config, forge: Forge, runtime: AgentRuntime, issue: Issue,
         # not just on the runner's stderr.
         ledger.failure(forge, issue, "delivery", note=f"no PR opened ({type(e).__name__})", tail=str(e))
         return Outcome(issue.number, branch, True, gate_ok)
+
+
+def _pr_body(issue: Issue, agent_name: str, gate_ok: bool, verify: str,
+             agent_output: str) -> str:
+    """The devloop PR body. This module owns the format — written here,
+    parsed by issue_of_body() below; the `Closes #N` marker is load-bearing
+    for review-by-number."""
+    return (
+        f"Closes #{issue.number}\n\n"
+        f"- agent: `{agent_name}`\n"
+        f"- gate: {'PASS' if gate_ok else 'FAIL'}"
+        + (f" (`{verify}`)" if verify else " (none configured)")
+        + "\n\nHuman merge required — agents never merge."
+        + "\n\n## Agent report\n\n" + agent_output[-TAIL:].strip()
+    )
+
+
+def issue_of_body(body: str) -> int | None:
+    """The issue a devloop PR closes, from the body this module writes.
+    None when the body carries no marker (human-authored PR, edited body)."""
+    m = re.search(r"[Cc]loses #(\d+)", body)
+    return int(m.group(1)) if m else None

devloop/forge/base.py

@@ -32,6 +32,25 @@ class Forge:
     """Adapter for one git forge. Subclasses implement the primitives;
     guardrails are enforced here in the base so no adapter can forget."""
 
+    # --- devloop branch naming: one owner. The prefix is load-bearing
+    # (the workflow template filters on `devloop/`), so it lives behind the
+    # Forge interface as concrete methods — the convention is identical
+    # across adapters, and fake test adapters inherit it for free.
+    BRANCH_PREFIX = "devloop/issue-"
+
+    def branch_for(self, number: int) -> str:
+        return f"{self.BRANCH_PREFIX}{number}"
+
+    def issue_of_branch(self, branch: str) -> int | None:
+        """The issue a devloop branch carries, or None (not devloop-owned,
+        or not parseable)."""
+        if not branch.startswith(self.BRANCH_PREFIX):
+            return None
+        try:
+            return int(branch[len(self.BRANCH_PREFIX):])
+        except ValueError:
+            return None
+
     # --- read side -------------------------------------------------------
     def issues_with_labels(self, labels: list[str]) -> list[Issue]:
         raise NotImplementedError

devloop/gate.py

@@ -0,0 +1,20 @@
+"""Verify gate: one policy for how the pipeline judges work before shipping.
+
+One interface function: run_gate(). Behind it: the subprocess call, the
+timeout, and PASS semantics. delivery and repair both gate through here —
+gate policy changes land in one module, not in every caller.
+"""
+
+import subprocess
+
+
+def run_gate(verify: str, cwd: str, timeout: int) -> bool:
+    """Run the pipeline verify command in the worktree being judged.
+    True = PASS. Never raises: a hung or exploding gate is a FAIL, not a
+    crash that wedges the build thread."""
+    try:
+        r = subprocess.run(verify, shell=True, capture_output=True,
+                           text=True, cwd=cwd, timeout=timeout)
+        return r.returncode == 0
+    except (subprocess.TimeoutExpired, OSError):
+        return False

devloop/queue.py

@@ -35,7 +35,7 @@ def next_builds(cfg: Config, forge: Forge) -> list[Issue]:
     delivered = {p.head for p in prs}
     candidates = []
     for issue in forge.issues_with_labels(cfg.labels.triggers):
-        if f"devloop/issue-{issue.number}" in delivered:
+        if forge.branch_for(issue.number) in delivered:
             continue
         attempts, today = ledger.budget(forge, issue)
         if attempts >= cfg.pipeline.max_attempts:

devloop/repair.py

@@ -9,12 +9,11 @@
 merges; it only pushes commits to the PR branch that already exists.
 """
 
-import subprocess
-from pathlib import Path
-
 from .config import Config
 from .forge import Forge
-from .runtime import TAIL, AgentRuntime
+from .gate import run_gate
+from .rounds import is_lgtm, run_round, with_repo_guidance
+from .runtime import AgentRuntime
 
 REPAIR_PROMPT = (
     "You are repairing a pull request based on AI review findings. The "
@@ -42,63 +41,46 @@
 )
 
 
-def _lgtm(output: str) -> bool:
-    return "LGTM" in output[-200:].upper()
-
-
 def repair_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int,
               branch: str, workdir: str, issue_title: str, issue_body: str,
               findings: str) -> str:
     """Repair rounds (pipeline.repair_rounds) on one PR. Each round: fixer
     run → verify gate → push → one verification review round. Returns the
     unresolved findings ("" when verification LGTMs). Never opens, closes,
     or merges a PR — the PR already exists; humans own those buttons."""
-    p = Path("skills/repair/SKILL.md")
-    prompt = REPAIR_PROMPT + "\n\n## Repo-specific repair guidance\n" + p.read_text() if p.exists() else REPAIR_PROMPT
+    prompt = with_repo_guidance(REPAIR_PROMPT, "skills/repair/SKILL.md",
+                                "Repo-specific repair guidance")
     prompt = (prompt
               .replace("{issue_title}", issue_title)
               .replace("{issue_body}", issue_body))
-    thread = [f"- {c.author}: {c.body.strip()[:500]}"
-              for c in forge.pr_comments(pr_number)]
     for rnd in range(1, cfg.pipeline.repair_rounds + 1):
-        # fresh diff every round — the fixer and verifier must judge what
-        # is on the branch now, not what review round 1 saw
-        diff = forge.pr_diff_by_number(pr_number)
-        round_prompt = (prompt
-                        .replace("{findings}", findings)
-                        .replace("{diff}", diff[:40000]))
-        if thread:
-            round_prompt += "\n\n## The PR thread so far\n" + "\n".join(thread)
-        res = runtime.run(round_prompt, cwd=workdir, timeout=cfg.pipeline.timeout)
-        if not res.ok:
-            forge.pr_comment(pr_number, f"AI repair round {rnd}: fixer run failed.")
+        res = run_round(forge, runtime, pr_number, "repair",
+                        prompt.replace("{findings}", findings),
+                        rnd, cfg.pipeline.repair_rounds, workdir,
+                        cfg.pipeline.timeout)
+        if res is None:
             return findings
-        forge.pr_comment(pr_number, f"**AI repair, round {rnd}/{cfg.pipeline.repair_rounds}**\n\n"
-                                 + res.output.strip()[-TAIL:])
-        # gate before push — a repair that pushes failing code is worse
-        # than no repair; the finding stays open for the human instead
         if cfg.pipeline.verify:
-            gate = subprocess.run(cfg.pipeline.verify, shell=True,
-                                  capture_output=True, text=True, cwd=workdir)
-            if gate.returncode != 0:
+            # gate before push — the gate module owns the policy; a repair
+            # that pushes failing code is worse than no repair, the finding
+            # stays open for the human instead
+            if not run_gate(cfg.pipeline.verify, workdir, cfg.pipeline.timeout):
                 forge.pr_comment(pr_number,
                                  f"AI repair round {rnd}: verify gate FAILED — "
                                  "fix not pushed, findings remain open.")
                 return findings
-        forge.commit_all(f"devloop(repair): address AI review findings", workdir)
+        forge.commit_all("devloop(repair): address AI review findings", workdir)
         # one verification round per repair (cheap: it re-checks the
-        # findings against the current diff, it does not re-review the PR)
-        # diff re-fetched AFTER the fixer — the verifier judges what is
-        # 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=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:])
-            if _lgtm(vres.output):
-                return ""
-        findings = vres.output.strip() if vres.ok else findings
+        # findings against the current diff, it does not re-review the PR).
+        # run_round fetches the diff AFTER the fixer committed — the verifier
+        # judges what is now on the branch, not the diff the fixer was handed
+        vres = run_round(forge, runtime, pr_number, "verify",
+                         VERIFY_PROMPT.replace("{findings}", findings),
+                         1, 1, workdir, cfg.pipeline.timeout)
+        if vres and is_lgtm(vres.output):
+            return ""
+        if vres:
+            findings = vres.output.strip()
     forge.pr_comment(pr_number,
                      f"AI repair budget exhausted ({cfg.pipeline.repair_rounds} "
                      "round(s)) — unresolved findings above; human decides.")