/pr-loop skill — contract¶
Project-local Claude Code skill at .claude/skills/pr-loop/SKILL.md. Invoked with /pr-loop <PR#> [--max-rounds N] [--merge].
Why this exists¶
The two halves of the Codex↔Claude review loop already existed and worked: review-claude-pr (Codex reviews, posts findings as khalilouardini) and /review-comments (Claude fixes and replies as ogur-claude-bot). What was missing was the thing that decides whether there is another round — so every cycle needed a human to run the next command.
/pr-loop is that driver, and nothing else. It does not review and it does not answer reviews; it sequences the two skills that do, and owns the stop conditions.
Roles¶
| Piece | Runs as | Writes |
|---|---|---|
.agents/skills/review-claude-pr/SKILL.md |
Codex, via codex exec |
PR review comments as khalilouardini — never code |
.claude/skills/review-comments/SKILL.md |
Claude | code fixes + replies as ogur-claude-bot |
.claude/skills/pr-loop/SKILL.md (this) |
Claude | sequences the other two; no repository writes of its own, but it does write durable filesystem state — a per-PR answered-findings ledger at ${XDG_STATE_HOME:-~/.local/state}/ogur-pr-loop/<owner>-<repo>-<pr>.answered, outside the repo and outside $SCRATCH. Never cleared by the loop; deleting a line is how a human reopens a settled disagreement |
The separation is deliberate: the reviewer must not be able to fix what it flagged, and the responder must not be able to grade its own fix.
The one override it applies¶
/review-comments has a confirm gate — it drafts every reply and waits for approval before posting. That gate assumes an interactive run. /pr-loop skips it and posts directly; every other invariant in that skill (identity subshell, $ORIG restore, PENDING/[bot] filtering, never resolving threads, make lint && make test-fast) still applies.
The override lives here rather than in review-comments itself, so invoking /review-comments manually still asks first.
The reviewer half (.agents/skills/review-claude-pr/) is tracked by this PR for the same reason: it existed only as an untracked local file, so a clean clone had a driver whose first paid step invoked a skill that was not there — codex exec would have run, produced no review and no marker, and the loop would have reported BLOCKED for what looked like a Codex failure. Preflight now checks the file exists before spending anything.
Binding to the PR branch¶
/review-comments scopes its comment lookup to the PR number, but its edits, commits, and pushes go to whatever is checked out. Invoked from the wrong branch, /pr-loop 123 would fix a different branch and leave PR 123 untouched.
The driver therefore asserts current branch == headRefName, HEAD == headRefOid, and a clean tracked worktree at the start of each responder pass — not once at startup, since the head moves every round and, per this repo's working style, concurrent sessions share one working directory.
The pre-push guard is a different check, and conflating the two is a bug worth naming: by push time the responder has committed its fix, so local HEAD is ahead of headRefOid by construction and the equality assertion would fail on exactly the rounds that fixed something. What the push needs is that the remote has not moved and that the local commits descend from the reviewed head:
test "$(git rev-parse --abbrev-ref HEAD)" = "$HEAD_REF" # still the PR's branch
test "$(git rev-parse "origin/$HEAD_REF")" = "$HEAD_OID" # nobody pushed mid-round
git merge-base --is-ancestor "$HEAD_OID" HEAD # no rewrite of reviewed history
test -z "$(comm -3 "$SCRATCH/untracked-baseline.txt" \
<(git status --porcelain --untracked-files=all | sort))" # untracked set unchanged
Two details in there are load-bearing, and both fail silently if dropped. The
branch-name assertion is kept, because ancestry establishes history and not which branch is
checked out: a concurrent session switching this shared worktree to another branch that also
descends from $HEAD_OID passes every other check while the push lands somewhere else. And the
cleanliness check drops -uno, because -uno hides untracked files — the responder is asked for
new regression tests, and an unstaged one would let the code change push without its test. Since
this repo carries a large pre-existing untracked set, the guard diffs against a baseline captured
in preflight rather than demanding an empty tree.
That diff is two-way (comm -3) and wrapped in test -z, and both details are the guard. A
one-way comm -13 sees additions only, so a responder using git add -A would sweep the
pre-existing untracked files into its commit, their ?? lines would vanish, the diff would come
back empty, and the push would publish unrelated workspace data. And an unwrapped comm prints
the differences while exiting 0 — documenting it that way would teach a guard that always passes.
Keep this snippet byte-identical to the one in the skill.
Either guard failing is BLOCKED. The driver never checkouts, resets, or rebases its way out — uncommitted work in a shared tree is not its to relocate.
Authorship scope¶
review-claude-pr limits itself to PRs attributable to ogur-claude-bot, but it has an explicit-request exception — and this driver always passes an exact PR number, which trips that exception on every run. Left alone, /pr-loop would silently widen the reviewer to any PR including the maintainer's own.
Preflight therefore reads author and the commit authors itself and stops unless one is ogur-claude-bot. --any-author is the one exception, must be typed by the user, and is reported in the run summary so an out-of-scope run is never mistaken for an in-scope one.
Termination — the part that matters¶
An unbounded loop between two models is the failure mode this skill exists to prevent. Four independent brakes:
| Brake | Mechanism | Prevents |
|---|---|---|
| Head-SHA skip | <!-- codex-review head:<sha> --> marker already written by review-claude-pr; a round whose marker exists skips the Codex call. The lookup uses gh api --paginate — the reviews endpoint pages at 30, and a marker on page 2 read as absent buys a duplicate review |
Paying for a second review of unchanged code |
| Round cap | 5 by default, --max-rounds overrides. Counts rounds in one invocation — --max-rounds N means "N more rounds", not a total across reruns |
Slow divergence within a run |
| P3 floor | CLEAN fires when a round yields no P0–P2 finding |
Buying a round for style nits |
| Repeat-finding guard | A P0–P2 finding recurring after it was answered exits BLOCKED. A finding is recorded once its response has landed — reply posted, plus the push if there was one — not only when code changed: a prose-only answer left out of the ledger live-locks the loop, since the head never moves, the marker reuses the review for free, and review-comments skips the thread because the bot commented last. Answered findings are keyed on path + normalized title and appended to a per-PR ledger under $XDG_STATE_HOME/ogur-pr-loop/, not $SCRATCH, which the trap … EXIT erases — so the guard survives a rerun. The path is in the key because the ledger is permanent: reviewers reuse generic titles, and a title-only key would block an unrelated same-named finding in another file before the responder read it |
The two models arguing to a standstill, across invocations as well as within one — without false-blocking a distinct finding |
Verdicts are CLEAN, BLOCKED, ROUNDS_EXHAUSTED. Design pushback is always BLOCKED — an unattended round does not concede an architecture argument on the maintainer's behalf.
Resuming after ROUNDS_EXHAUSTED¶
Re-invoking continues from the current head; it does not restart from round 1. But assume it costs one Codex review, because the cap fires at the top of a round: ROUNDS_EXHAUSTED is raised only after the previous round ran to completion, answered its P0–P2 findings and pushed. The usual exhausted state is therefore a new head that has never been reviewed, and paying for one review of it is correct. What resumes is the work — earlier fixes are pushed, earlier findings are in the ledger, and only the unreviewed delta is bought.
The head-SHA marker skips the Codex call only when the head has not moved and preflight passes, and the second condition disqualifies most of the exits that look eligible. The binding block runs before step 1: a BLOCKED on a failed gate left the responder's edits uncommitted and fails its cleanliness assertion; a BLOCKED on a moved remote left a local commit ahead of headRefOid and fails its head assertion. Both stop in preflight until a human reconciles the tree, by design. The free reread belongs to exits that leave the tree clean at the reviewed head — a clean interruption, or the step-3 BLOCKEDs (repeat finding, design pushback), which re-block on the ledger anyway.
N resets on every invocation and ROUNDS_EXHAUSTED is not persisted, so the round cap places no bound on a sequence of reruns. The ledger does, which is why it lives outside $SCRATCH.
Merging¶
Off by default. With --merge, all of: verdict CLEAN, every check in gh pr checks green (both required jobs from .github/workflows/tests.yml), mergeable == MERGEABLE, and no open thread carrying a P0–P2 finding. The merge runs as $ORIG, never as the bot.
Codex posting No findings. is not a merge authorization. CI is. The reviewer skill never submits APPROVE, and this skill does not treat its silence as approval.
Cost model¶
One codex exec run plus one /review-comments pass per round, capped at 5, with the head-SHA skip removing any round where nothing was pushed. This is the local, no-CI-minutes alternative to running the same loop as GitHub Actions on every push.
Harness scope¶
Claude Code only, like /spec and /triage — it drives Codex as a subprocess rather than being run by it, so there is no .agents/ counterpart to keep in sync. The skill it invokes on the Codex side (review-claude-pr) is already canonical under .agents/skills/.
Sandbox note¶
codex exec is run with -s danger-full-access because the reviewer needs the network and gh; workspace-write blocks both. The reviewer is therefore unsandboxed against the working repo, and its "make no repository edits" boundary is the only thing keeping it read-only. Treat that line in review-claude-pr as a safety control, not a style preference.
Implementing rule of thumb¶
Spend a Codex round only on code that changed, and stop the moment the two sides repeat themselves.