From 1d01a28c59ee0283a3b7642c1396d637551f202f Mon Sep 17 00:00:00 2001 From: TEMP Date: Wed, 16 Sep 2026 21:22:44 -0400 Subject: [PATCH 1/4] refactor: deepen gate, round dialect, branch naming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - verify gate: one policy (timeout, PASS semantics) in devloop/gate.py — both copies lacked a timeout, so one hung gate wedged a build thread - review/repair round plumbing (guidance load, thread read, diff cap, LGTM verdict) lives once in devloop/rounds.py; _lgtm twin deleted - devloop branch naming owned by concrete Forge methods, six inline call sites gone - deliver() takes agent_name: str — it only ever read runtime.name devloop: status=none --- CONTEXT.md | 7 +++++-- devloop/core.py | 20 ++++++++------------ devloop/delivery.py | 31 +++++++++++++++--------------- devloop/forge/base.py | 19 +++++++++++++++++++ devloop/gate.py | 20 ++++++++++++++++++++ devloop/queue.py | 2 +- devloop/repair.py | 34 +++++++++++++-------------------- devloop/review.py | 16 +++++++--------- devloop/rounds.py | 38 +++++++++++++++++++++++++++++++++++++ tests/test_devloop.py | 44 ++++++++++++++++++++++++++++++++++++++++++- 10 files changed, 169 insertions(+), 62 deletions(-) create mode 100644 devloop/gate.py create mode 100644 devloop/rounds.py diff --git a/CONTEXT.md b/CONTEXT.md index 03e8bd9..f50bca6 100644 --- a/CONTEXT.md +++ b/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 diff --git a/devloop/core.py b/devloop/core.py index 19e3bc9..3ac6672 100644 --- a/devloop/core.py +++ b/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.] full argv wins for this # build; no section configured → the caller's global runtime agent = cfg.runtime.for_kind(kind) or runtime @@ -80,7 +80,7 @@ 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) @@ -99,9 +99,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 +143,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 +165,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 +187,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: diff --git a/devloop/delivery.py b/devloop/delivery.py index 2e9d64d..25a58ca 100644 --- a/devloop/delivery.py +++ b/devloop/delivery.py @@ -1,20 +1,20 @@ """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 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 +26,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. @@ -79,7 +78,7 @@ def deliver(cfg: Config, forge: Forge, runtime: AgentRuntime, issue: Issue, title=f"devloop({kind}): {issue.title} (#{issue.number})", body=( f"Closes #{issue.number}\n\n" - f"- agent: `{runtime.name}`\n" + f"- agent: `{agent_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." diff --git a/devloop/forge/base.py b/devloop/forge/base.py index af042b1..3a66428 100644 --- a/devloop/forge/base.py +++ b/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 diff --git a/devloop/gate.py b/devloop/gate.py new file mode 100644 index 0000000..0b7ce5a --- /dev/null +++ b/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 diff --git a/devloop/queue.py b/devloop/queue.py index 26a326a..aafdebc 100644 --- a/devloop/queue.py +++ b/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: diff --git a/devloop/repair.py b/devloop/repair.py index 6bed91c..7de7913 100644 --- a/devloop/repair.py +++ b/devloop/repair.py @@ -9,11 +9,10 @@ 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 .gate import run_gate +from .rounds import DIFF_CAP, is_lgtm, thread_block, thread_lines, with_repo_guidance from .runtime import TAIL, AgentRuntime REPAIR_PROMPT = ( @@ -42,10 +41,6 @@ ) -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: @@ -53,34 +48,31 @@ def repair_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int, 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)] + thread = thread_lines(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) + .replace("{diff}", diff[:DIFF_CAP])) + round_prompt += thread_block(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.") 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.") @@ -91,12 +83,12 @@ def repair_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int, # 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]), + .replace("{diff}", forge.pr_diff_by_number(pr_number)[:DIFF_CAP]), 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): + if is_lgtm(vres.output): return "" findings = vres.output.strip() if vres.ok else findings forge.pr_comment(pr_number, diff --git a/devloop/review.py b/devloop/review.py index e7063cd..f87159e 100644 --- a/devloop/review.py +++ b/devloop/review.py @@ -12,10 +12,10 @@ """ import re -from pathlib import Path from .config import Config from .forge import Forge +from .rounds import DIFF_CAP, is_lgtm, thread_block, thread_lines, with_repo_guidance from .runtime import TAIL, AgentRuntime REVIEW_PROMPT = ( @@ -36,8 +36,8 @@ def review_prompt(cfg: Config, issue_title: str = "", issue_body: str = "") -> s guidance from skills/pre-review/SKILL.md (the customization point) + the spec issue. Substitution is replace-based, not .format — injected content (issue bodies, diffs) may contain braces.""" - p = Path("skills/pre-review/SKILL.md") - prompt = REVIEW_PROMPT + "\n\n## Repo-specific review guidance\n" + p.read_text() if p.exists() else REVIEW_PROMPT + prompt = with_repo_guidance(REVIEW_PROMPT, "skills/pre-review/SKILL.md", + "Repo-specific review guidance") return (prompt .replace("{issue_title}", issue_title) .replace("{issue_body}", issue_body)) @@ -61,16 +61,14 @@ def review_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int, prompt = review_prompt(cfg, issue_title, issue_body) # the reviewer reads the PR thread once at the start — human replies # ("already fixed elsewhere", "out of scope") must not be ignored - thread = [f"- {c.author}: {c.body.strip()[:500]}" - for c in forge.pr_comments(pr_number)] + thread = thread_lines(forge.pr_comments(pr_number)) prior: list[str] = [] for rnd in range(1, cfg.pipeline.review_rounds + 1): # diff straight from the forge — GitHub computes it authoritatively; # local origin/HEAD-based diffs proved unreliable mid-build diff = forge.pr_diff_by_number(pr_number) - round_prompt = prompt.replace("{diff}", diff[:40000]) - if thread: - round_prompt += "\n\n## The PR thread so far\n" + "\n".join(thread) + round_prompt = prompt.replace("{diff}", diff[:DIFF_CAP]) + round_prompt += thread_block(thread) if prior: # rounds are isolated sessions — carry the prior findings in, so # round N verifies/extends rather than repeats round 1 @@ -84,6 +82,6 @@ def review_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int, prior.append(res.output.strip()) forge.pr_comment(pr_number, f"**AI pre-review, round {rnd}/{cfg.pipeline.review_rounds}**\n\n" + res.output.strip()[-TAIL:]) - if "LGTM" in res.output[-200:].upper(): + if is_lgtm(res.output): return "" return prior[-1] diff --git a/devloop/rounds.py b/devloop/rounds.py new file mode 100644 index 0000000..7d9cbe0 --- /dev/null +++ b/devloop/rounds.py @@ -0,0 +1,38 @@ +"""Shared round plumbing for the review and repair loops. + +Internal seam — not part of either module's interface: prompt assembly +(repo guidance + injected content), the PR-thread read, the diff +injection cap, and the LGTM verdict. review.py and repair.py call these, +so the dialect changes in one place. +""" + +from pathlib import Path + +# one cap for injected diffs — both loops truncate the same way +DIFF_CAP = 40000 + + +def with_repo_guidance(base: str, skill_path: str, header: str) -> str: + """Base prompt + repo-specific guidance from skills//SKILL.md when + present (the customization point). Replace-based substitution is done + by the caller — injected content may contain braces.""" + p = Path(skill_path) + if p.exists(): + base += f"\n\n## {header}\n" + p.read_text() + return base + + +def thread_lines(comments) -> list[str]: + """The PR thread as prompt lines — human replies ("already fixed", + "out of scope") must not be ignored by reviewer or fixer.""" + return [f"- {c.author}: {c.body.strip()[:500]}" for c in comments] + + +def thread_block(thread: list[str]) -> str: + """The thread appended to a round prompt ("" when there is none).""" + return ("\n\n## The PR thread so far\n" + "\n".join(thread)) if thread else "" + + +def is_lgtm(output: str) -> bool: + """The single-word LGTM verdict, judged on the tail of the output.""" + return "LGTM" in output[-200:].upper() diff --git a/tests/test_devloop.py b/tests/test_devloop.py index e4826cc..bcbef95 100644 --- a/tests/test_devloop.py +++ b/tests/test_devloop.py @@ -394,7 +394,7 @@ def test_conflict_gate_passes_disjoint_builds(): class R: name = "fake" - out = delivery.deliver(Config(repo="o/r"), forge, R(), issue, + out = delivery.deliver(Config(repo="o/r"), forge, R().name, issue, "devloop/issue-1", ".", "done") assert forge.prs == ["devloop/issue-1"] @@ -1326,3 +1326,45 @@ def g(*args, cwd): assert subprocess.run(["git", "config", "user.email"], cwd=co, capture_output=True, text=True).stdout.strip() \ == "human@repo" + + +def test_verify_gate_one_policy(): + """The gate module owns the verify policy: PASS/FAIL semantics and the + timeout — a hung gate is a FAIL, not a wedge. Both callers (delivery, + repair) gate through this one interface.""" + from devloop.gate import run_gate + + assert run_gate("true", ".", 10) is True + assert run_gate("exit 1", ".", 10) is False + # the latent hang: a gate that never returns is a FAIL, not a blocked + # build thread — this is why the policy lives in one place + assert run_gate("sleep 2", ".", timeout=1) is False + + +def test_branch_naming_one_owner(): + """The devloop branch convention is owned by the Forge interface: + build and parse in one place, inherited by every adapter and fake — + callers never touch the string.""" + from devloop.forge.base import Forge + + f = Forge() + assert f.branch_for(7) == "devloop/issue-7" + assert f.issue_of_branch("devloop/issue-7") == 7 + assert f.issue_of_branch("feature/x") is None # stranger's branch + assert f.issue_of_branch("devloop/issue-") is None # malformed + assert f.issue_of_branch("devloop/issue-x") is None # unparseable + + +def test_rounds_dialect_shared(): + """The review/repair round dialect: LGTM verdict parsing, thread + formatting, and repo-guidance loading exist once, in rounds.py.""" + from devloop.forge.base import Comment + from devloop.rounds import is_lgtm, thread_lines, with_repo_guidance + + assert is_lgtm("all good\nLGTM") is True + assert is_lgtm("LGTM was mentioned earlier\nP1: still broken") is False + assert thread_lines([Comment("ann", "already fixed"), + Comment("bob", " out of scope ")]) == \ + ["- ann: already fixed", "- bob: out of scope"] + base = with_repo_guidance("BASE", "/nonexistent/skill.md", "X") + assert base == "BASE" # missing guidance file leaves the prompt alone From 188f52067e25b834a15b42f5b7992391d41e7e6b Mon Sep 17 00:00:00 2001 From: TEMP Date: Wed, 16 Sep 2026 21:30:09 -0400 Subject: [PATCH 2/4] =?UTF-8?q?refactor(delivery):=20own=20the=20PR=20body?= =?UTF-8?q?=20format=20=E2=80=94=20template=20+=20Closes-#N=20parse=20in?= =?UTF-8?q?=20one=20module;=20review=5Fpr=20takes=20the=20Issue,=20dead=20?= =?UTF-8?q?branch=20param=20deleted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- devloop/core.py | 3 +-- devloop/delivery.py | 32 ++++++++++++++++++++++++-------- devloop/review.py | 26 ++++++++++++-------------- tests/test_devloop.py | 6 +++--- 4 files changed, 40 insertions(+), 27 deletions(-) diff --git a/devloop/core.py b/devloop/core.py index 3ac6672..21e1cfb 100644 --- a/devloop/core.py +++ b/devloop/core.py @@ -82,8 +82,7 @@ def process_issue(cfg: Config, forge: Forge, runtime: AgentRuntime, issue: Issue return Outcome(issue.number, branch, False) 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) diff --git a/devloop/delivery.py b/devloop/delivery.py index 25a58ca..b8354bf 100644 --- a/devloop/delivery.py +++ b/devloop/delivery.py @@ -8,6 +8,7 @@ a bug in one place, not a forgotten except clause in a caller. """ +import re from dataclasses import dataclass from . import ledger @@ -76,14 +77,7 @@ def deliver(cfg: Config, forge: Forge, agent_name: str, issue: Issue, forge.open_pr( branch, title=f"devloop({kind}): {issue.title} (#{issue.number})", - body=( - f"Closes #{issue.number}\n\n" - f"- agent: `{agent_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, @@ -94,3 +88,25 @@ def deliver(cfg: Config, forge: Forge, agent_name: str, 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 diff --git a/devloop/review.py b/devloop/review.py index f87159e..22824ad 100644 --- a/devloop/review.py +++ b/devloop/review.py @@ -11,10 +11,9 @@ to repair. """ -import re - from .config import Config -from .forge import Forge +from .delivery import issue_of_body +from .forge import Forge, Issue from .rounds import DIFF_CAP, is_lgtm, thread_block, thread_lines, with_repo_guidance from .runtime import TAIL, AgentRuntime @@ -45,19 +44,18 @@ def review_prompt(cfg: Config, issue_title: str = "", issue_body: str = "") -> s def review_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int, - branch: str = "", issue_title: str = "", issue_body: str = "") -> str: + issue: Issue | None = None) -> str: """AI pre-review rounds (pipeline.review_rounds) on one PR. Stops early on LGTM. Returns the last round's findings ("" on LGTM or failed run). - branch = head branch when known (build flow); empty = review-by-number - (`devloop review `), diff fetched from the forge. - issue_title/issue_body: the spec the diff is judged against (the builder - flow has it; review-by-number parses `Closes #N` from the PR body).""" - if not issue_title: - body = forge.pr_body(pr_number) - m = re.search(r"[Cc]loses #(\d+)", body) - if m: - it = forge.issue(int(m.group(1))) - issue_title, issue_body = it.title, it.body + issue = the spec the diff is judged against — the build flow has it; + None = review-by-number (`devloop review `), reconstructed from the + PR body's `Closes #N` marker (parsed by delivery, the format owner). + No marker on the body → review proceeds without spec context.""" + if issue is None: + n = issue_of_body(forge.pr_body(pr_number)) + issue = forge.issue(n) if n else None + issue_title, issue_body = (issue.title, issue.body) if issue else ("", "") + issue_title, issue_body = (issue.title, issue.body) if issue else ("", "") prompt = review_prompt(cfg, issue_title, issue_body) # the reviewer reads the PR thread once at the start — human replies # ("already fixed elsewhere", "out of scope") must not be ignored diff --git a/tests/test_devloop.py b/tests/test_devloop.py index bcbef95..9b75b5c 100644 --- a/tests/test_devloop.py +++ b/tests/test_devloop.py @@ -547,9 +547,9 @@ def test_build_flow_hands_review_findings_to_repair(): calls = [] - def fake_review(cfg, forge, runtime, pr, branch, issue_title="", issue_body=""): - calls.append(("review", issue_title)) - return "" if issue_title == "lgtm" else "P1: wrong" + def fake_review(cfg, forge, runtime, pr, issue=None): + calls.append(("review", issue.title)) + return "" if issue.title == "lgtm" else "P1: wrong" def fake_repair(cfg, forge, runtime, pr, branch, workdir, t, b, findings): calls.append(("repair", findings)) From f69ddcc07696697861cba9f6ad11af6a3cc6ef83 Mon Sep 17 00:00:00 2001 From: TEMP Date: Wed, 16 Sep 2026 21:30:16 -0400 Subject: [PATCH 3/4] =?UTF-8?q?refactor(cli):=20one=20read=5Fevent()=20bra?= =?UTF-8?q?cket=20for=20the=20CI=20entrypoints=20=E2=80=94=20path=20resolu?= =?UTF-8?q?tion,=20parse,=20silent=20exit=20owned=20once?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- devloop/cli.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/devloop/cli.py b/devloop/cli.py index 37f9f26..768d457 100644 --- a/devloop/cli.py +++ b/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 From 5d20867f53e25fe320970e129db8e75570f9c450 Mon Sep 17 00:00:00 2001 From: TEMP Date: Wed, 16 Sep 2026 21:31:11 -0400 Subject: [PATCH 4/4] =?UTF-8?q?refactor(rounds):=20deep=20round=20engine?= =?UTF-8?q?=20=E2=80=94=20run=5Fround=20owns=20diff+thread=20injection,=20?= =?UTF-8?q?announcement,=20run-failure=20comment;=20review/repair=20keep?= =?UTF-8?q?=20only=20what=20differs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- devloop/repair.py | 46 +++++++++++++++--------------------- devloop/review.py | 29 +++++++++-------------- devloop/rounds.py | 34 +++++++++++++++++++++++---- tests/test_devloop.py | 54 +++++++++++++++++++++++++++++++++++++++---- 4 files changed, 109 insertions(+), 54 deletions(-) diff --git a/devloop/repair.py b/devloop/repair.py index 7de7913..dd4d59e 100644 --- a/devloop/repair.py +++ b/devloop/repair.py @@ -12,8 +12,8 @@ from .config import Config from .forge import Forge from .gate import run_gate -from .rounds import DIFF_CAP, is_lgtm, thread_block, thread_lines, with_repo_guidance -from .runtime import TAIL, AgentRuntime +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 " @@ -53,21 +53,13 @@ def repair_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int, prompt = (prompt .replace("{issue_title}", issue_title) .replace("{issue_body}", issue_body)) - thread = thread_lines(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[:DIFF_CAP])) - round_prompt += thread_block(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:]) if cfg.pipeline.verify: # gate before push — the gate module owns the policy; a repair # that pushes failing code is worse than no repair, the finding @@ -77,20 +69,18 @@ def repair_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int, 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)[:DIFF_CAP]), - 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 is_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.") diff --git a/devloop/review.py b/devloop/review.py index 22824ad..297ffca 100644 --- a/devloop/review.py +++ b/devloop/review.py @@ -14,8 +14,8 @@ from .config import Config from .delivery import issue_of_body from .forge import Forge, Issue -from .rounds import DIFF_CAP, is_lgtm, thread_block, thread_lines, with_repo_guidance -from .runtime import TAIL, AgentRuntime +from .rounds import is_lgtm, run_round, with_repo_guidance +from .runtime import AgentRuntime REVIEW_PROMPT = ( "You are reviewing a pull request authored by another AI agent. Review " @@ -57,29 +57,22 @@ def review_pr(cfg: Config, forge: Forge, runtime: AgentRuntime, pr_number: int, issue_title, issue_body = (issue.title, issue.body) if issue else ("", "") issue_title, issue_body = (issue.title, issue.body) if issue else ("", "") prompt = review_prompt(cfg, issue_title, issue_body) - # the reviewer reads the PR thread once at the start — human replies - # ("already fixed elsewhere", "out of scope") must not be ignored - thread = thread_lines(forge.pr_comments(pr_number)) prior: list[str] = [] for rnd in range(1, cfg.pipeline.review_rounds + 1): - # diff straight from the forge — GitHub computes it authoritatively; - # local origin/HEAD-based diffs proved unreliable mid-build - diff = forge.pr_diff_by_number(pr_number) - round_prompt = prompt.replace("{diff}", diff[:DIFF_CAP]) - round_prompt += thread_block(thread) if prior: # rounds are isolated sessions — carry the prior findings in, so # round N verifies/extends rather than repeats round 1 - round_prompt += ("\n\n## Your earlier findings (verify against the " - "current diff; drop resolved ones, keep and " - "sharpen the rest)\n" + "\n---\n".join(prior)) - res = runtime.run(round_prompt, cwd=".", timeout=cfg.pipeline.timeout) - if not res.ok: - forge.pr_comment(pr_number, f"AI pre-review round {rnd}: reviewer run failed.") + extra = ("\n\n## Your earlier findings (verify against the " + "current diff; drop resolved ones, keep and " + "sharpen the rest)\n" + "\n---\n".join(prior)) + else: + extra = "" + res = run_round(forge, runtime, pr_number, "pre-review", prompt, + rnd, cfg.pipeline.review_rounds, ".", + cfg.pipeline.timeout, extra=extra) + if res is None: return "" prior.append(res.output.strip()) - forge.pr_comment(pr_number, f"**AI pre-review, round {rnd}/{cfg.pipeline.review_rounds}**\n\n" - + res.output.strip()[-TAIL:]) if is_lgtm(res.output): return "" return prior[-1] diff --git a/devloop/rounds.py b/devloop/rounds.py index 7d9cbe0..358ff11 100644 --- a/devloop/rounds.py +++ b/devloop/rounds.py @@ -1,17 +1,43 @@ """Shared round plumbing for the review and repair loops. -Internal seam — not part of either module's interface: prompt assembly -(repo guidance + injected content), the PR-thread read, the diff -injection cap, and the LGTM verdict. review.py and repair.py call these, -so the dialect changes in one place. +Internal seam — not part of either module's interface: one round of the +agent dialect (run_round: fresh diff + thread into the prompt, agent run, +announcement comment, run-failure comment), plus prompt assembly (repo +guidance), the diff injection cap, and the LGTM verdict. review.py and +repair.py call these, so the dialect changes in one place. """ from pathlib import Path +from .runtime import AgentRuntime, RunResult, TAIL + # one cap for injected diffs — both loops truncate the same way DIFF_CAP = 40000 +def run_round(forge, runtime: AgentRuntime, pr_number: int, label: str, + prompt: str, rnd: int, total: int, cwd: str, timeout: int, + extra: str = "") -> RunResult | None: + """One agent round of the review/repair dialect, end to end: fetch the + forge's authoritative diff (GitHub computes it; local origin/HEAD-based + diffs proved unreliable mid-build), inject it against {diff} (caller + leaves the placeholder in), append the fresh PR thread, run the agent, + announce the round. extra is appended after the thread (review's + prior-findings carry). Returns the run result, or None when the run + failed — the failure is already commented on the PR.""" + diff = forge.pr_diff_by_number(pr_number) + round_prompt = (prompt.replace("{diff}", diff[:DIFF_CAP]) + + thread_block(thread_lines(forge.pr_comments(pr_number)))) + round_prompt += extra + res = runtime.run(round_prompt, cwd=cwd, timeout=timeout) + if not res.ok: + forge.pr_comment(pr_number, f"AI {label} round {rnd}: run failed.") + return None + forge.pr_comment(pr_number, f"**AI {label}, round {rnd}/{total}**\n\n" + + res.output.strip()[-TAIL:]) + return res + + def with_repo_guidance(base: str, skill_path: str, header: str) -> str: """Base prompt + repo-specific guidance from skills//SKILL.md when present (the customization point). Replace-based substitution is done diff --git a/tests/test_devloop.py b/tests/test_devloop.py index 9b75b5c..53fbe00 100644 --- a/tests/test_devloop.py +++ b/tests/test_devloop.py @@ -523,7 +523,7 @@ def run(self, prompt, cwd, timeout): assert "Repo-specific repair guidance" in r.calls[0][0] # skills/repair carried in assert f.pushed == 1 assert "diff-v1" in r.calls[1][0] # verifier saw the post-fix diff - assert "AI verify after repair 1" in f.notes[-1] + assert "AI verify, round 1/1" in f.notes[-1] # unresolved findings: budget exhausts, findings returned for the human f = RepairForge() @@ -1356,10 +1356,12 @@ def test_branch_naming_one_owner(): def test_rounds_dialect_shared(): - """The review/repair round dialect: LGTM verdict parsing, thread - formatting, and repo-guidance loading exist once, in rounds.py.""" + """The round engine: LGTM verdict parsing, thread formatting, and + repo-guidance loading exist once in rounds.py — and run_round owns the + full round bracket: diff+thread injection, announcement, run-failure + comment. Plus the PR-body contract, owned by delivery.""" from devloop.forge.base import Comment - from devloop.rounds import is_lgtm, thread_lines, with_repo_guidance + from devloop.rounds import is_lgtm, run_round, thread_lines, with_repo_guidance assert is_lgtm("all good\nLGTM") is True assert is_lgtm("LGTM was mentioned earlier\nP1: still broken") is False @@ -1368,3 +1370,47 @@ def test_rounds_dialect_shared(): ["- ann: already fixed", "- bob: out of scope"] base = with_repo_guidance("BASE", "/nonexistent/skill.md", "X") assert base == "BASE" # missing guidance file leaves the prompt alone + + from devloop.delivery import issue_of_body + assert issue_of_body("context\nCloses #12\nmore") == 12 + assert issue_of_body("closes #7") == 7 + assert issue_of_body("no marker here") is None + + notes = [] + + class RoundForge: + def pr_diff_by_number(self, n): + return f"the diff" + + def pr_comments(self, n): + return [Comment("human", "already fixed")] + + def pr_comment(self, n, body): + notes.append(body) + + class R: + name = "fake" + + def run(self, prompt, cwd, timeout): + self.prompt, self.cwd = prompt, cwd + return type("Res", (), {"ok": True, "output": "P1: something"})() + + # one round: {diff} filled from the forge, thread appended, extra last, + # announcement posted + f, r = RoundForge(), R() + res = run_round(f, r, 55, "pre-review", "judge {diff}", 1, 2, ".", 60, + extra="\nPRIOR FINDINGS") + assert res is not None + assert "the diff" in r.prompt and "```diff" not in r.prompt.split("judge")[0] + assert "already fixed" in r.prompt and "PRIOR FINDINGS" in r.prompt + assert r.cwd == "." + assert notes[-1].startswith("**AI pre-review, round 1/2**") + + # failed run: failure commented on the PR, None returned + class Boom(R): + def run(self, prompt, cwd, timeout): + return type("Res", (), {"ok": False, "output": "boom"})() + + notes.clear() + assert run_round(RoundForge(), Boom(), 55, "repair", "x", 1, 1, ".", 60) is None + assert "AI repair round 1: run failed" in notes[-1]