Imported from kanetik/claude-skills (skills/pr-review-loop/SKILL.md). Install upstream with npx skills add kanetik/claude-skills --skill pr-review-loop. Copyright stays with the author.

PR Review Loop

Drives a pull request to convergence: engage reviewers → wait → evaluate → fix/answer → push → repeat, until every reviewer is satisfied.

The role split

There is one loop and one path from a finding to its resolution, and the two roles never swap.

The reviewer reviews. It never fixes. It reads the code, finds what is wrong with it, and posts its findings to the PR as threads. That is the whole job. This holds for a review bot and equally for the pr-review-skeptic skill, whose blind reviewers are the strongest reviewer this loop has and still touch nothing.

You are the author, and the only thing that touches code. You read the findings off the PR, decide each one, apply what should be applied, answer what shouldn't, record the decision where the reviewer's next run will see it, push, and ask for review again.

Everything below is the mechanics of that. Two rules are worth stating before them, because most of the ways this loop fails are one of the two coming apart:

• Every finding gets a decision, and the decision goes on the PR. Not "the blocking ones" — every one. A finding you fix, a finding you reject, a finding you defer to an issue: each gets a reply saying which, and its thread resolved. That reply is not politeness. It is the only record the next round has that this finding was dealt with, and without it the next round finds it again, and the round after that.
• If you changed code, it gets reviewed. No exceptions, and severity has nothing to do with it. Severity decides what must be fixed. It never decides what must be reviewed. A round where you fixed only MEDIUM and LOW findings still moved HEAD, so that HEAD is unreviewed, and the run is not done — "the findings were non-blocking" is not a reason to skip review, it is a statement about the findings you were given, not about the code you then wrote. This is the rule the loop exists to enforce, and the one it is easiest to talk yourself out of at the end of a long run, because the remaining findings look small and the fixes feel obvious. The fixes that feel obvious are the ones that have been breaking things all along.

So what does terminate it? A round that changes no code. Fix nothing and there is nothing new to review, so the reviewer is satisfied — whether the round ended in rejections, acknowledgements, deferrals, already-fixed replies, a re-answer of something you had already rejected, or a clean verdict. That is the fixed point, and it is a fact about the diff, not about severities. See step 4.

Stated as the pair of rules it actually is: a review must follow every change to the code, and the run may stop only when a review has been followed by no change. The reviewer found nothing worth acting on, or found things this loop decided were not worth acting on, or the round ended in any other way that left the code where it was — all of those are the same terminal state, because all of them mean the code now standing is code a reviewer has read. Every one of them is a statement about the code, so it is answered by comparing shas, not clocks ("Reading reviewer state"). A run that has changed code since its last review is not finished, whatever else is true of it.

Terminating is not the same as not answering. Every finding in that last round still gets its reply, its marker and its resolved thread — the round terminates because answering them changed no code, not instead of answering them. A finding left unanswered is an undecided finding whatever else was true of the round, and leaving one on an unresolved thread under a summary that says converged is the worst of both rules.

This skill is self-contained. The files below live in this skill's own directory, beside this SKILL.md — read them from there (paths are relative to this file, not the working directory). Load on demand:

• config/defaults.yml — config defaults.
• reference/configuration.md — config keys, override model, invocation modifiers, project procedural overrides.
• reference/mechanics.md — tool tiers, the GraphQL/REST queries the loop needs, Copilot/Codex trigger mechanics, and how the skeptic reviewer is invoked and read.
• reference/evaluation.md — the step-5 lens rubric, courses of action, disposition replies, issue creation, reactions, resolve criteria.
• reference/waiting.md — the step-3 wait: the polling model, re-entrancy, carried state, timeouts.

Requires: gh (authenticated), git. Bash forms also use jq (PowerShell forms don't). With skeptic in reviewers, also the pr-review-skeptic skill, installed and configured (Preconditions below). Optional: a GitHub MCP server; a scheduling primitive (/loop, ScheduleWakeup, CronCreate) for self-paced polling waits (feature-detected — degrades gracefully).

Snippet convention: <...> tokens (<num>, <owner>, <repo>, <path>, <tmp>) are placeholders you substitute with real values, not literal shell tokens.

Reporting style — terse

Status during iterations and waits is one or two lines: "Iter 3 wait, Copilot still cooking, back in ~4 min." / "Iter 4: skeptic 2 HIGH, 5 MEDIUM — fixing 5, rejecting 2." / "All reviewers clean, terminating." Don't restate reviewer text the user can read on the PR. The final summary is short bullets, not paragraphs.

Configuration (summary)

Read config/defaults.yml, then merge overrides per key, low → high: bundled defaults < optional ~/.claude/pr-review-loop.config.yml < orchestrator repo's .github/pr-review-loop.config.yml. Defaults: reviewers: [copilot], auto_review_grace_seconds: 0, wait_check_cadence_seconds: 180, max_iterations: 10, whole_change_taper: 2, mark_ready_on_convergence: false, fix_bar (a general sentence a project overrides with its own — step 5's question 2 reads it, and nothing else does). Parse natural-language modifiers from the invocation. Full model — including the retired gate keys and what to do with a config that still sets them: reference/configuration.md.

One reviewer list. reviewers is the set the loop engages: engaged when the PR is opened, re-engaged after every fix push, and the set convergence is gauged on. There is no second list and no out-of-loop role. A reviewer that shows up on its own — a bot that auto-reviews on push, a review the user posted themselves — is read and triaged like any other, but it isn't re-engaged by step 8 and doesn't hold the loop open.

whole_change_taper below 1 stops the run. At 0 step 4's third condition is vacuously true for ever, so the loop would converge having read the whole change never. Like the reviewers rule below it is an invariant tested twice — before step 2, and at every step-4 evaluation, because a wake resumes at step 3 and never re-runs the preamble — rather than a one-time file check. reference/configuration.md says why it is validated rather than asserted.

reviewers must be non-empty when step 2 runs — an invariant, not a one-time config check. With nothing to engage, step 2 engages nobody, step 3 has nothing to wait for, and step 4 finds the empty set vacuously happy: the loop terminates instantly and reports converged on a PR no reviewer has looked at. A clean finish on an unreviewed PR is the worst thing this skill can produce, because it is indistinguishable from a real one. The list can reach empty by more routes than a bad config file — a modifier that narrows it ("only copilot" where copilot isn't configured, "skip the skeptic" on a skeptic-only list), your own offer to drop an unusable reviewer in Preconditions, or a reviewer excused mid-run, which is the one route that fires after step 2 is long past. So it is checked twice: over reviewers before step 2, and over the accountable set at every step-4 evaluation (the convergence invariant, step 4 — which explains why it must not be the active set). Empty at either → stop and say so, naming what emptied it; never fall through to a vacuous pass.

Two kinds of reviewer, one role

An entry in reviewers is either a bot or skeptic (the sibling pr-review-skeptic skill). They do the same job and their findings are triaged identically; only how you engage them, and how you recognise their work on the PR, differ.

The last row is the one that breaks naive code. Skeptic's reviews are authored by a human account, so anything classifying reviewers by author type alone reads them as human participation and skips them entirely. Check the marker before the author type, everywhere disposition is derived.

Skeptic must be able to post, or this loop does not work. Its findings reach you as threads on the PR; those are what you reply to and resolve, and what its own next run reads to tell a settled decision from a fresh finding. An agent-invoked run posts only where the PR's repo has committed allow_agent_posting: true to its .github/pr-review-skeptic.config.yml and no layer sets confirm_before_posting: true, which reverts an agent-invoked run to no-post and so cancels the grant. The Preconditions check below tests both at kickoff rather than after the first round.

Context discipline when invoking skeptic. You have just read this PR's description, its threads, and possibly written its code — everything that skill exists to keep away from its reviewers, and the pile grows every round. Invoking it does not launder that: follow its Context discipline section exactly, which builds each reviewer's brief by substitution from project config and the diff and forbids adding a summary of the change, its purpose, or its rationale. Hand it the PR reference and nothing else — on round ten as on round one. A reviewer steered by your account of what the change does, or of what the last nine rounds already settled, is not an independent reviewer.

Three sets, and they answer different questions

Keep these apart. Two of them are about work still to do; the third is about who has to have signed off, and conflating that one with the others is how a loop reports success on an unreviewed PR.

• Tracked set (permanent, PR-derived): reviewers ∪ every reviewer that has posted a review or a review-style verdict comment (findings or a clean pass, judged on content — not routine CI/noise). One joins the moment it does so and never leaves; it's the same on every wake.

• Active set (run-scoped): tracked minus bots already gone happy, minus any excused. This is engagement bookkeeping only — who still needs asking. The loop's own fix push re-engages active ∩ reviewers; a reviewer that showed up on its own isn't re-engaged, and already-happy bots are skipped.

  Going happy removes a bot. It never removes skeptic, which is re-invoked on every fix push for as long as the run lasts (step 4) — so on a list containing skeptic the active set never empties, and it should not. Read the drop rule as a fact about bots rather than about reviewers in general: a bot's clean verdict is allowed to go stale because nothing will ask it again, and skeptic's is not, because something always will. Getting this backwards is how a run reports converged on a HEAD skeptic never read: skeptic leaves active, step 8 stops re-invoking it, and the invariant's staleness half — which is scoped to members still in active — is then satisfied by its stale verdict from three rounds ago. On an all-bot list an empty active set is the normal end state of a good run.

• Accountable set (run-scoped): reviewers minus any excused — and nothing else. Going happy does not remove a reviewer from it; going happy is how a member satisfies it. This is the set convergence is judged over (step 4), and it is deliberately insensitive to the loop's own progress: what it asks is "did everyone who was supposed to review this actually review it", which is a question the active set cannot answer because the active set empties precisely when they have.

Membership in tracked/active is decided by what posted, not who: a bot (__typename: Bot), or a review carrying the <!-- pr-review-skeptic --> marker. A genuine human reviewer is in none of the three — the loop reads their comments as evaluation input (step 5) and never re-pings them. Neither the active nor the accountable set is recorded on the PR as such, so a context-less wake reconstructs both from the carried wake payload — see reference/waiting.md.

Reading reviewer state — three surfaces

A reviewer's disposition can surface on any of three channels; always read and union all three — never derive state from reviews alone (a clean verdict sitting in an issue comment, misread as "still reviewing", is the classic failure that strands the loop):

1. Formal reviews — Review objects and their bodies.
2. Review threads — inline and file-level comments.
3. PR issue comments — some reviewers post findings, or their whole clean verdict, only here.

Judge disposition from what the reviewer wrote, not a fixed phrase list — has findings, happy (a clean pass), or work-in-progress, reading the meaning as a person would; the next new bot will word it differently again. Staleness is a question about code, not about the clock: has this reviewer seen the code as it stands now? So compare shas, not timestamps. A verdict counts for the current HEAD when it was produced against the current HEAD sha; a verdict produced against any earlier sha is stale, whatever its timestamp. Changed the code, and a review is owed; changed nothing, and the verdict you have is the verdict for this code.

Where each reviewer's sha comes from: skeptic writes it into its coverage record, <!-- pr-review-skeptic: reviewed=<sha> … --> in the review body (that skill's stage 7), which is the sha its reviewers actually read — and only in a review authored by the authenticated account, since anyone who can review the PR can write that record and a forged one would make this whole test pass (reference/mechanics.md); a bot's formal review carries its own commit. Fall back to timestamps only where neither is available — a verdict in a plain issue comment with no sha attached — and compare it against the most recent push event, or, where the PR has no push event at all (the branch was pushed before the PR was opened, which is the gh pr create flow), against the iteration-1 baseline from step 1. That fallback exists so a sha-less verdict can still be judged; it is not the rule. Testing the clock as the primary rule is what makes a PR with no push event unjudgeable, and a run that cannot judge a clean round 1 spends every iteration it has re-deriving the same non-answer. Re-derivation applies only to reviewers still in the active set: a bot already dropped happy is not pulled back when a loop fix makes its verdict stale (only an external push or a manual re-request re-engages it — step 4). Skeptic never drops, and step 4 says why.

Preconditions

• Find the target PR(s). Read the branch: branch=$(git branch --show-current). Non-empty → gh pr list --head "$branch" --json number,title,url (quote it). Empty (detached HEAD — CI/automated runs) → match by commit: gh pr list --search "$(git rev-parse HEAD)" --json number,title,url. Zero matches and none specified → surface it. Multiple → ask which.

• Cross-repo PRs are allowed in any phrasing (URL, sibling name + number, owner/repo#num). Resolve to (owner, repo, number) and pass --repo to every gh call for that PR. At least one PR in the run must be in the orchestrator repo (the working directory's repo) — else ask the user to add one or confirm.

• Working tree must be clean and gh authenticated.

• whole_change_taper must be at least 1 once the config layers are merged. At 0 step 4's third condition passes vacuously for ever and the loop converges having never read the whole change. Stop and name the layer that set it (Configuration, above).

• A configured skeptic reviewer must be usable — check at kickoff, not at first use, so a run that can't review says so at the start rather than after the first wait. Three things can be missing (the checks themselves: reference/mechanics.md): the skill isn't installed; any one of its five project keys is still empty once its own config layers are merged (all five must hold a value — a layer that fills three leaves it as unusable as one that fills none), because an agent-invoked run does not interview for them and instead stops and hands back what's missing; or an agent-invoked run won't post — either the PR repo hasn't committed allow_agent_posting: true, or confirm_before_posting: true is set in some layer and cancels it. Say which it is, and offer the options that fit: add the missing config to the PR repo's .github/pr-review-skeptic.config.yml, or — only if another reviewer would remain — drop skeptic from reviewers for this run. Where skeptic is the only reviewer, dropping it is not on offer: it empties the list and the loop would terminate reporting a converged run on an unreviewed PR (the invariant above). There the options are fix the config, add a bot, or don't run. Don't guess project facts on the user's behalf either — five invented answers steer the reviewer that is supposed to be the independent one.

  The posting check is the one worth being blunt about, because the run looks fine without it: skeptic returns a full review to you, you fix what it found, and nothing is ever recorded on the PR — so its next run's blind pass re-finds everything you rejected, its cross-check has no threads to settle them against, and the loop grinds to max_iterations re-litigating the same findings every round. If the user wants to proceed anyway, proceed, but say that is what will happen.

  This check is kickoff work: it runs once, not on every wake.

Run git fetch && git pull --ff-only before each iteration's analysis. Run multiple PRs concurrently only if your scheduler supports it, each in an isolated working directory (separate git worktree/clone) to avoid state collisions.

The loop

1. Initial state check

Gather from the PR (queries in reference/mechanics.md): the current HEAD sha (headRefOid) — the value every staleness question is answered against; unresolved review threads (paginated; inline + file-level); all reviews with state, body, submittedAt, commit_id, author type and body marker (skeptic's carries the coverage record naming the sha it reviewed); PR issue comments with bodies + timestamps; the most recent push timestamp (latest of HeadRefPushedEvent/HeadRefForcePushedEvent.createdAt — force-pushes count; NOT committedDate); the PR's createdAt (iter-1 grace baseline); and reviewRequests via GraphQL (NOT gh pr view --json reviewRequests, which drops bots).

Then branch:

• Any unresolved feedback — unresolved threads (inline AND file-level), an unaddressed concern in any review body, OR a review-style comment raising unaddressed concerns → jump to step 5, skipping steps 2–3.
• Else → step 2 (grace window; engage reviewers that haven't already covered this commit) → step 3 (wait). No push here.

One iteration is one pass through 3 → 4 → 5 → 6 → 7 → 8, and the counter (step 9) increments once at the end of it, after the push. A round in which step 4 finds everyone happy ends the loop instead and increments nothing. Waiting never counts.

2. Engage reviewers

Same flow on iteration 1, after every push (called from step 8), and for the one whole-change re-invocation step 4's convergence clause asks for. Mechanics: reference/mechanics.md.

1. Wait auto_review_grace_seconds from the baseline: iter 1 = max(PR createdAt, latest push event) (some auto-triggers fire on PR open even when the branch was pushed earlier); iter 2+ = the most recent push event. Default 0 = no wait. Skeptic ignores this — nothing auto-triggers it, so there is nothing to wait out.
2. Determine the engage set: active ∩ reviewers. An external push (new commits lacking a PR-Review-Loop trailer — code no reviewer has seen) is the exception: rebuild the active set from the tracked set and re-engage (tracked − excused) ∩ reviewers — which brings back previously-dropped-happy configured reviewers, and only those, since no prior verdict covers code nobody has seen (reference/waiting.md). Excused reviewers stay out: they never left tracked, so an intersection that forgets them re-engages a reviewer that already could not run, buying another timeout and the same question on every external push. Still intersected with reviewers too: an uninvited reviewer is never re-engaged, on an external push as on any other, and re-engaging one would mean invoking a reviewer the config never asked the loop to drive — for skeptic that is a worktree and up to max_reviewers subagents, posting under the user's account, on every external push. Say it in one line when it happens, with the cost: this re-arms the whole-change taper (reference/waiting.md), so the run owes a fresh cold read of the whole change on top of the re-engagement. Name what the signal actually supports, which is that the commits carry no trailer. It does not say who wrote them: step 6 calls the stamp among the first things a long run drops, and step 8 lists the loop's own routes to an untrailered commit, so this branch fires on the loop's own lapse as readily as on somebody else's work. Step 8's classification is what tells them apart — use its answer where it has one, and where it has not run or could not tell, say untrailered rather than naming an author who may have touched nothing. Where they are the author's, the common shape is scope added mid-run, and the loop is the only party positioned to price it. "Iter 12: two untrailered commits, taper re-armed — that's a whole-change re-read." Unsaid, the cost lands later as rounds nobody can account for.
3. Skip any that has already covered the current commit — started reviewing it or already delivered a verdict for it, judged against the current HEAD sha wherever a sha is available (skeptic's coverage record, a bot review's commit_id), and by timestamp only where none is ("Reading reviewer state"). Evaluate across all three surfaces, plus the loop's own trigger. Copilot = a reviewRequests entry (presence means already-requested, so skip; reviewRequests carries no timestamp — when the loop owns push→request ordering that presence implies at/after the push, otherwise compare the timeline ReviewRequestedEvent.createdAt against the latest push). Codex = a chatgpt-codex-connector post OR an existing @codex review trigger at/after the push. Skeptic = a marker-carrying review, authored by the authenticated account, whose coverage record names the current HEAD sha (reviewed=<sha>). One exception, and it is the only one: an invocation step 4's whole-change clause asked for is never skipped here. That clause fires precisely when skeptic's record does name current HEAD — so this check would otherwise skip the one invocation the loop needs, on the round it needs it, and a run that is clean and fully reviewed would spin to max_iterations re-deciding nothing. The bound lives in step 4, which asks once; this exception adds no trigger of its own and must not be read as one. The author check is not optional: anyone who can review the PR can write that record into a body, and without it a forged one skips the invocation here and satisfies step 4 below (reference/mechanics.md) — the check that makes a re-entered wake idempotent instead of dispatching a second eight-reviewer pass over a commit already reviewed, and one that needs no push event to be answerable. An older skeptic review carrying no coverage record falls back to the timestamp comparison.
4. Engage each one not skipped. A bot is requested or triggered. Skeptic is invoked — the pr-review-skeptic skill, given the PR reference and nothing else, which reviews HEAD and posts its findings itself. Don't tell it not to post; whether it may is the repo's committed answer, not yours to assert either way (reference/mechanics.md).
5. Proceed to step 3.

Never auto-re-ping a human. A person who commented on the PR is not re-requested by this loop; their input is read in step 5 and weighed there. Skeptic is not covered by that rule despite posting under a human account — it is a reviewer this loop drives, identified by its marker, and re-engaged on every push like any other.

3. Wait for new reviewer activity

Only bots are waited on. Skeptic runs synchronously — its review is on the PR by the time it returns — so it is never pending. Where reviewers has no bot in it, there is no wait at all: step 2 comes back with the round's findings already in hand and you go straight to step 4. A wait armed for a set with nothing pending in it never ends.

For bots: poll on a timer — no events reach a local terminal to wait on. A local terminal can't receive GitHub webhook/event deliveries, so the loop drives itself: schedule a self-wake every wait_check_cadence_seconds (default 180s; recommended band 120-240s, i.e. every 2-4 min) and reconcile on each tick. Arm the best self-wake available: (a) a scheduling primitive — /loop <cadence>, ScheduleWakeup, CronCreate — carrying the continuation payload, else (b) a background polling monitor that fingerprints head-commit/reviews/comments/CI/merge state and emits on change — auto-waking you only where the host turns that background output into a re-invocation (in a plain terminal it just notifies a human, so fall through to c), else (c) a single-pass hand-back ("re-invoke /pr-review-loop to continue"). Never foreground-sleep busy-wait — that blocks the turn instead of yielding. Re-arm on every wake until the PR is merged or closed. End the turn. On every wake, re-pull and do a full three-surface reconciliation against HEAD before concluding anything — the PR state is ground truth. Make every wake idempotent — reconstruct loop state from the PR plus the carried payload. Waiting does NOT count toward max_iterations. Full model, ladder, polling snippet, the --repo gotcha, lockstep, re-entrancy, and carried state: reference/waiting.md.

4. Detect "this reviewer is happy"

Two conditions, and they do different jobs. Both must hold.

1. Nothing left at or above the blocking severities — not nothing left at all. A reviewer whose latest verdict carries three MEDIUM observations and no CRITICAL/HIGH has nothing blocking. Hold out for an empty verdict and the loop never ends: a reviewer that reports everything real it sees, with no floor, always sees something.
2. The current HEAD has been reviewed. If the round that dispositioned those three MEDIUMs fixed any of them, HEAD moved and nobody has looked at it. That round is not terminal, however small the fixes were — push and go round again.

Condition 1 is about the findings you were handed. Condition 2 is about the code you wrote in response, and it is the one that gets skipped, because at the end of a long run the remaining findings look minor and the fixes feel safe. Six rounds of this loop's own history say fixes are where the defects come from, and the smallest-looking round is not exempt.

What actually terminates the loop is a round that changes no code. Reject, defer, reply already-fixed, or find nothing — any of those leave HEAD where it was, so there is nothing new for a reviewer to see and the reviewer is satisfied. A round whose findings are only re-raises of things you have already rejected on the record terminates too: the reviewer said nothing new, and re-answering it would just be the previous round again. Terminating is a property of the diff, not of the severities.

Which severities block is blocking_severities in the skeptic config ([CRITICAL, HIGH] by default), and the equivalent judgement for a bot that doesn't tag severities: does its latest verdict raise anything that would change the code on a path users reach, or is it down to polish? Read it as a person would.

For each reviewer still in the active set, all must hold:

• Zero unresolved threads attributed to it that carry a blocking finding. A non-blocking thread left unresolved does not by itself hold the loop open — but step 5 requires every finding be dispositioned and resolved, so in a round done properly there are none. It is never a licence to leave findings unanswered and call the round terminal.
• Its latest verdict for the current HEAD (formal review/body or an issue comment) names no blocking finding — judged from what it wrote per "Reading reviewer state."
• That signal was produced against the current HEAD sha — skeptic's coverage record in a review authored by the authenticated account, or a bot review's own commit, with the timestamp fallback of "Reading reviewer state" only where no sha is attached. The author check belongs here as much as in step 2: a forged record satisfies this clause too, and this is the one that lets a run report converged.

A reviewer that posts no formal review can still be happy on a clean issue comment alone — exactly the case a reviews-only check misses.

A settled blocking finding does not hold the loop open, and does not disappear either. Skeptic's cross-check moves a finding to settled when a prior thread weighed the same consequence and the project chose otherwise — which includes a rejection you wrote and a concern you deferred to an issue. Those are decisions, and re-litigating them is what a loop with no memory does. So they don't block. But a blocking one still appears in skeptic's verdict with its count and the thread that decided it, and it belongs in your final summary the same way. You are judging your own work here, and the only thing between that and a loop you converge by rejecting everything is that a settled CRITICAL stays visible. Never let settled become silent.

This subsumes the old all-rejections short-circuit and replaces it: it does not matter what mix of no-op courses the round took, because none of them changes the code the verdict was about. Any course that actually edits code does change it, so the round pushes and the reviewer looks again.

A reviewer the user excuses leaves the run. Three places offer that — an unresponsive bot (reference/waiting.md) and a skeptic round the engage set asked for whose reviewers produced nothing (reference/mechanics.md), and step 4's whole-change arm — and the offer is empty unless something acts on it, since a reviewer otherwise leaves active only by going happy. A run that could not stage is not one of them, however much it looks like one: no reviewer was dispatched, the staging lock it hit clears on its own, and excusing over it trades minutes of waiting for a run with nothing independently reviewing it. That row says so; this sentence is the one a reader reaches first. So: when the user says to skip or proceed without one, drop it from active for the rest of the run, exactly as a happy reviewer is dropped but for the opposite reason. It is not re-engaged by step 8, it ends any wait pending on it (reference/waiting.md lockstep), and it is named in the final summary as excused, not reviewed — never folded in with the happy ones.

Excusing the last reviewer is not convergence. Excusing is the one thing that removes a reviewer from the accountable set, and an excused reviewer is one that did not review — so where excusing it would empty that set, the invariant fails and the run reports nobody reviewed this HEAD. Say that when you make the offer, so the user is choosing between a review and no review rather than appearing to pick between two ways of finishing. One site does not make the offer there at all — step 4's whole-change arm below, where that outcome is the specific thing the arm exists to avoid reporting.

Carry the excused set in the wake payload alongside the dropped-happy set. An external push resets the dropped-happy set but not the excused set: a happy verdict is genuinely stale against code nobody has seen, whereas a reviewer that could not run does not become able to run because the diff changed — re-engaging it buys another twenty-minute timeout and the same question. Re-offer it if you like; don't re-arm the wait.

Happy is sticky for a bot, never for skeptic. Once a bot goes happy it leaves the active set for the rest of the run (it stays in the tracked set as history) — not re-requested, not re-evaluated — and this holds across the loop's own follow-up commits even as its clean verdict goes stale. Exactly two things bring it back: the user manually asking for it, or an external push (reference/waiting.md). Skeptic never drops. It is re-invoked on every fix push for as long as the loop runs, because reviewing what the fix rounds produced is the largest part of its value: the code with the least review behind it is the code written last, under the most accumulated confidence about why it's right. A clean skeptic verdict on round 4 says nothing about round 5's fix.

First, one config check that is not one of the conditions. Re-read whole_change_taper from the merged config here rather than trusting the kickoff read: at 0 the third condition below passes vacuously for ever, and a wake resumes at step 3 without re-running Preconditions, so a kickoff-only check never covers the evaluation that matters. Below 1 stops the run and names the layer that set it. Report it as stopped on an unusable whole_change_taper — it is not one of the five terminal states below and must not be reported as one, because nothing was reviewed wrongly: the run was told to evaluate against a condition that cannot fail. The shape is an empty reviewers before step 2, not at step 4 — an empty accountable set at step 4 is terminal state 4, so that is the wrong half of the analogy to reach for here.

The convergence invariant — judged over the accountable set, never the active one. Before reporting converged, all three of these must hold:

• The accountable set is non-empty. "Everyone is happy" is vacuously true of nobody, and three things can empty it: a modifier that narrowed reviewers to nothing, an unusable skeptic dropped at kickoff, and every member being excused.

• Every member has a happy verdict — and for every member still in the active set, that verdict was produced against the current HEAD sha. Code this loop changed that no active reviewer has since read is the state this whole skill exists to keep from being called done, and the sha is what says whether that is so.

  The staleness half is scoped to the active set for the same reason re-derivation is ("Reading reviewer state"): a bot that went happy left the active set by design, is deliberately never re-requested, and its clean verdict is expected to go stale as later rounds push. Demanding a current-HEAD verdict from it asks for something stickiness forbids the loop to obtain, so a [copilot, skeptic] run where Copilot went clean early could never converge no matter what skeptic said. A dropped-happy reviewer's stale verdict satisfies this clause; that is what dropping it means. If you want it to re-review, the ways to bring it back are in step 4's stickiness rule, and doing so puts it back in active where the staleness half applies again.

• Where skeptic is accountable, the whole change has been read at HEAD. Its coverage record carries a whole-change sha — the commit at which the whole change was last read (pr-review-skeptic SKILL.md, stage 7) — and it must equal the current HEAD. Skeptic gives most later runs no whole-change reader at all, because that read costs more than every other stage put together and does not have to happen every round (that skill's stage 3, Partition). So on a converging PR the sha is routinely several rounds behind HEAD, and this clause is the entire guarantee that the whole change gets read cold before anyone merges. Without it the cadence would simply be a hole.

  The equality is not the only way the clause can hold, because on a large change it has no fixed point. Every fix moves HEAD, so every fix makes the sha stale, so every fix owes another whole-change read — and a blind adversarial reader of several thousand lines samples a different slice of the edge space on each pass. On the run this paragraph exists because of, four consecutive whole-change reads returned one MEDIUM and two LOW apiece, none of them wrong; three rounds in between dispositioned every finding without changing a line and still could not terminate, because the sha was behind. That is a steady finding rate, not a queue draining, and a criterion that waits for it to reach zero waits for something that is not coming.

  So the clause also holds once whole_change_taper consecutive whole-change reads have produced no blocking finding (default 2 — reference/configuration.md). Count reads, not rounds — the count is re-derived from the PR rather than carried, and reference/waiting.md gives the derivation: the reads are what the guarantee is about, and the rounds between them are delta-scoped by construction. A blocking finding in a whole-change read resets the count to zero, with one exclusion: one the cross-check bucketed settled, which no more resets the count than it holds the loop open, and the clean reads the taper asks for have to be consecutive and have to come after it — that read is the cadence earning its cost, and it is exactly the case where another cold look is worth buying. Once met, the clause stays satisfied as later fixes move HEAD — but only while the read is still on the branch and everything after it is this loop's own. Both conditions are load-bearing: the taper's whole claim is that the unread part is repair work this loop wrote and delta-reviewed, so a commit from anyone else breaks it, and a rebase or force-push breaks it too even though it preserves the loop's trailers — which is why the test is not a list of events. reference/waiting.md states it, and shows what an unbounded count lets through.

  Nothing else relaxes. A fix is still a fix, a round that changed code is still not terminal (condition 2), and the delta review that round owes still has to come back happy before anything converges. Those rounds do reach a fixed point; it is only the whole-change re-read that does not, and this clause is the only place that distinction has to be made.

  What the taper must not be read as is a licence to merge unread code. It is stricter than the equality about the thing the clause exists for — two cold reads of the whole change rather than one — and looser only about when — and about how far, which is the cost the key buys rather than a detail of it. Once met, the taper holds across the run's remaining fix rounds, blocking ones included; each gets a delta review, none gets a fresh cold read. A long repair tail can therefore sit behind a met taper never read as one thing. The converged summary naming the sha is what makes that visible, and a reader seeing it several rounds back knows what they are merging. So the summary names the sha it was last read at, and the gap stays visible to whoever merges. Raising whole_change_taper buys more cold reads; no value switches the requirement off, and a small PR reaches the equality on its own long before the taper is in play.

  Behind HEAD — or none, or absent from the record — and the taper not yet met → re-invoke skeptic once, then evaluate again. HEAD has not moved since its last review, so step 2's skip check would otherwise skip this invocation, and its one exception is written for exactly this; ask for it there rather than routing around it. What that run does is skeptic's own business: normally its delta is empty and it dispatches a composition reviewer alone, but a record that establishes no whole-change read — absent, or none — sends it to first-run scope instead, which is a fuller and more expensive pass. Either way it reads the whole change and records the sha. A blocking finding means the round was not terminal after all — continue normally, and it is the round the cadence was paying for. A clean one leaves whole-change equal to HEAD and this clause holds. It counts as a round against max_iterations like any other.

  Ask once. Still behind after that one re-invocation — its reviewers returned nothing, or it ran and its whole-file-list reviewer did not — is not convergence, and it is its own outcome: the whole change could not be read at HEAD. Do not report it as nobody reviewed this HEAD. The two are different runs: that one has an accountable reviewer with no verdict at this commit, while this one routinely has every content unit read clean at this very commit and only the whole-change pass missing. Reporting the stronger name tells a reader their PR is unreviewed when it was reviewed and found clean, and the headline is the line they act on. So name what was reviewed alongside what was not, and, where there is one to name, the commit the whole change was last read at: on none and on an absent field there is no commit to name, and asserting one you do not have is the thing pr-review-skeptic stage 7 guards against in its own verdict. The cause is a dispatch failure rather than an unreviewable PR, so put the choice to the user rather than stopping flat. State the options here rather than borrowing the "No unit reviewed" row, which governs the returns from dispatches the engage set asked for, not the one this clause asks for: run the loop again, which starts with a fresh ask and is the remedy where the cause was transient; excuse skeptic, which makes this condition vacuous — and is the only thing a bot on the list is worth here, since adding one does not otherwise clear a condition that fires wherever skeptic is accountable — and not on offer at all on a skeptic-only list, where excusing the last reviewer empties the accountable set and produces nobody reviewed this HEAD, the outcome this arm exists to avoid, so unlike the other two excusal sites the offer goes rather than the warning; or merge with the whole-change read missing, naming what was read clean. What this step does not do is ask again on its own. A run that returned a verdict but could not post it is not this arm — like the staging-lock case below, that return value has its own row in reference/mechanics.md, which puts the choice to the user, and the row governs. One automatic ask is the whole bound, and it is what keeps step 2's exception bounded by this sentence rather than by a condition of its own. The user may still choose to run the loop again — that is their call to make on a reported outcome, not a second ask this step takes on its own initiative, and it is the same shape as every other terminal report here. A run that could not stage is not this arm at all: that return value has its own row in reference/mechanics.md, which pauses and does not count the round, and a lock that clears on its own is not a reviewer that could not read.

  This clause is skeptic's alone, and vacuous on an all-bot list. A bot reads the whole PR diff every time it runs, so its happy verdict already carries a whole-change read and there is nothing to gate; a clause demanding a whole-change sha where no skeptic is accountable would make convergence unreachable for every list without one — the same failure the active-set reading above produces.

Fail any and it is not convergence, and the reported outcome depends on which. The first two — an empty accountable set, or a member without a happy verdict, judged for staleness over the active set exactly as the bullets above state it — are nobody reviewed this HEAD: name the last commit and what emptied or unreviewed the set. Do not restate that condition more tightly than the bullet does; dropping the active-set scoping makes a [copilot, skeptic] run unable to converge once Copilot has dropped happy. The third alone is the separate outcome the bullet above defines, because it is routinely reached on a commit where everything else was read clean. This is the one mis-report nothing downstream recovers from, because a converged verdict is exactly what a reader stops checking.

Do not state this over the active set. A happy bot leaves the active set, so on an all-bot list active ∩ reviewers empties on success: an invariant demanding it be non-empty would declare every successful all-bot run unreviewed, and make convergence unreachable for any list without skeptic in it (skeptic being the one reviewer that never drops, so the only one whose presence keeps the set populated). The active set tracks who still needs asking; the accountable set tracks who had to answer. Only the second is a convergence question.

Terminal states, reported differently.

1. Converged — the invariant holds. Disposition and resolve any remaining non-blocking findings in the terminating verdict first (below), then take the PR out of draft where mark_ready_on_convergence is on and the PR is the orchestrator repo's (below — this state and no other, that repo and no other), then go to the final summary.
2. Paused for your answer — the only things still holding the loop open are Ask-user findings, or threads awaiting a human reply. Nothing the loop can do advances those: it cannot decide them, and a round that changes no code re-derives the same question next time round. Stop and ask, rather than iterating. Report it as paused, list the open questions with their threads, and say what is already settled. This is not a failure to converge, and reporting it as one buries a question behind a word that reads like a bug. Iterations spent spinning on an unanswerable finding are iterations the loop does not have.
3. Cap reached — max_iterations exhausted with blocking findings outstanding (step 9).
4. Nobody reviewed this HEAD — the convergence invariant's first two conditions failed: the accountable set is empty, or a member of it is not happy on the terms the invariant states, staleness scoped to the active set. Name the last commit and what emptied or unreviewed the set.
5. The whole change could not be read at HEAD — the invariant's third condition alone, after its one ask and with the taper unmet. Every content unit may have been read clean at this commit; what is missing is the whole-change pass. The taper narrows this state to what it was always meant to name — a whole-change read that could not be obtained — rather than one that ran, came back clean twice, and was merely some fix-rounds old. Name what was reviewed as well as what was not — and the commit the whole change was last read at only where there is one, per the bullet's own guard — and keep it distinct from state 4 — collapsing the two tells a reader their PR is unreviewed when it was reviewed and found clean.

The terminating round still dispositions its own findings. Terminal state 1 goes to the summary without passing through step 5, so the findings in the verdict that ended the loop — a clean pass carrying three MEDIUM nits, typically — would never get a reply or a resolved thread. That is the commonest terminating round there is, and leaving it undispositioned breaks the rule this skill leads with: every finding gets a decision. So before the summary, run step 5's recording half over them — reply, disposition marker, resolve.

In that pass the courses are rejected, acknowledged, deferred, already-fixed, and Ask-user — with acknowledged unavailable to any finding at a blocking severity, exactly as everywhere else (reference/evaluation.md). already-fixed belongs here for the reason it is easy to leave out of any course list: it stamps disposition=fixed and still moves no code, and this pass is reachable with one, since a dropped-happy bot's stale clean verdict can carry a finding a later round already fixed. Omit it and the author must stamp rejected on something that was in fact fixed, which the next run's cross-check reads as "the code was kept as-is, with a rationale" — a fix permanently recorded on the PR as a rejection. A terminating verdict normally carries no blocking findings at all, so the restriction rarely bites; where one is present it is not acknowledgeable just because the round is the last.

deferred keeps its own bar here too, and the round being the last is not a reason to reach for it. An issue is for work genuinely not what this PR is about — never for a real, related problem you would rather not re-open a clean round over. That the fix would be an unreviewed change on a PR whose review just came back clean is a fact about the cost of another round, not about relatedness, and the answer is to take the round (reference/evaluation.md). It is the deferral that looks most like discipline, which is why it is the one that gets past the bar.

acknowledged is the one that carries most terminating rounds, and the reason this pass is usually honest rather than a fudge. The typical terminating verdict is a clean pass with two or three correct LOW/MEDIUM observations that name no problem anyone reaches — imprecise phrasing, a test that could assert more. Those are Acknowledge-no-change: agree on the thread, say why it isn't worth changing, resolve (reference/evaluation.md). Before this course existed the only honest-looking exits were to fix them — which un-terminates the round and buys another review — or to file issues, which is how a backlog grows one terminating round at a time.

Ask-user has to be available, because the findings arriving in a terminating verdict are subject to the same default as any others: security/auth-adjacent, relatedness-boundary, conflicting and architectural calls go to the user. Without it, a security-adjacent MEDIUM in the last verdict would have to be stamped rejected and resolved — after which the next run's cross-check reads that thread, buckets the finding settled, and it neither blocks nor re-raises again. A question the skill mandates escalating would be decided unilaterally by the author of the code and permanently suppressed. So leave that thread open and unmarked, name it in the summary under threads left open, and report the run as terminal state 2 (paused) rather than 1 — an open question is an open question whether or not the finding was blocking.

The remaining courses: this pass commits and pushes nothing, so it cannot fix anything. Stamping disposition=fixed on a finding you did not fix hands the next round's cross-check a fixed marker on a defect still present at HEAD, which is the definition of unfixed and comes back a severity higher. And fixing it anyway leaves an edit either uncommitted (tripping the next run's clean-tree precondition) or pushed after convergence was declared, unreviewed. So a finding that names a real problem this change owns means the round is not terminal — whatever its severity — go through 5→6→7→8 normally and let the reviewer see the result. The judgement that separates those from the acknowledgeable ones is step 5's, unchanged by the round happening to be the last one: reference/evaluation.md, "Is the problem real, is it worth acting on, and is it ours?". A real finding that is not worth the round it would cost is acknowledged here like anywhere else, which is what makes most terminating rounds terminal.

Then, on terminal state 1 alone, take the PR out of draft. Off unless mark_ready_on_convergence is on (default false — reference/configuration.md); where it is off, do nothing and say nothing. And only for a PR in the orchestrator repo — a cross-repo PR is never undrafted, because this key merges from your config and the config governs every PR in the run, so honouring it there would undraft a PR in a repo that never opted in and has no file the loop reads to opt out. Where it is on and the PR is the orchestrator repo's, after the terminating round's dispositions and before the final summary: read the PR's draft state (gh pr view <num> --json isDraft), and where it is still a draft, gh pr ready <num>, then name the transition in the summary. Gate on the state now, not on the state the run found, which is what makes a re-entered wake idempotent: a PR a human took out of draft mid-run is already where this would put it, and a wake that re-derives convergence over an already-ready PR finds nothing to do rather than thrashing it. A failure here is not fatal — no permission to undraft, a PR that cannot be undrafted — so report what failed, and finish with the converged summary anyway. The outcome this run reports does not depend on the draft state.

What the transition sets off is another matter, and the summary has to say so. Where the repo auto-reviews on ready_for_review — a Ruleset that requests Copilot is the shape auto_review_grace_seconds is written for, and those triggers skip drafts — undrafting solicits a fresh review of HEAD moments after this run printed converged and stopped. Those findings land on a PR now in the human merge queue, unresolved and untriaged, under a summary saying every reviewer is happy: the same harm the "just fix what's there" modifier is written to avoid (reference/configuration.md), arriving as a side effect instead. The loop cannot wait for it — it has terminated — so say in the summary that a review arriving after this one is expected and is the next run's input. Say it whenever the transition fired, not only where you believe the repo has such a trigger: you cannot see the Ruleset from here, and a caveat that turns out to be unnecessary costs one sentence.

The other four terminal states leave the PR in draft, and that guard is the point of this rather than a detail of it. Marking ready is outward-facing: it notifies reviewers, and where the repo requires reviews it changes what can merge. Paused has a human question outstanding, so it is the opposite of ready-for-review; cap reached is a run that ended with blocking findings; and states 4 and 5 are the two the convergence invariant exists to keep from being reported as done — marking either ready converts an internal mis-report into a request for human sign-off on code no reviewer read. A round that is not terminal state 1 does not touch the draft state, whatever else it did.

A reviewer that turned up uninvited is not accountable. It is triaged in step 5 like any other, but it is not in reviewers, so step 8 never re-engages it and its verdict goes stale at the first fix push with nothing able to refresh it. Requiring it to be happy means the loop cannot terminate at all — it runs to the cap reporting an unhappy reviewer nobody asked for and nobody can satisfy. The commonest instance is the user running /pr-review-skeptic themselves mid-loop.

A reviewer whose blocking findings have all been dispositioned is happy, even with no new verdict. The second and third conditions above ask what its latest verdict says, and a round that decided every finding without changing any code — all Reject-with-explanation, all Create-issue-and-close, all Acknowledge-no-change where the findings allowed it, or a mix — moves neither HEAD nor the verdict. Step 6 commits nothing, step 8 pushes nothing, and step 2's skip check then skips the reviewer because its existing review is already at/after the (unmoved) push. Read literally, that reviewer is unhappy forever over findings you settled in round one, and the loop spins out its remaining iterations doing nothing.

So: a blocking finding you have dispositioned and resolved on the PR no longer holds the loop open — exactly as a settled one doesn't. That is a rule about what keeps the loop running, and not a widening of which courses were available: a blocking finding's dispositions are fixed, Create-issue-and-close, Reject-with-explanation and Ask-user, never Acknowledge-no-change (reference/evaluation.md). Whatever disposition it took, it took one of those. It has been decided, and there is nothing left for the reviewer to look at again. It is still reported: it goes in the final summary with its count and its thread, under the same rule that keeps a settled CRITICAL visible.

The test is whether HEAD moved, not which course you took. rejected, acknowledged and deferred never move it. Neither does the fourth no-op course, which is easy to miss because it wears a fixed marker: already-fixed, where you reply that an earlier round handled the finding and resolve the thread (reference/evaluation.md). A round of nothing but already-fixed dispositions produces no commit and no push, so a rule phrased as "only Fix-* needs a re-review" leaves it unable to converge — the reviewer's verdict still names the finding and nothing can refresh it. Only a course that actually changed HEAD needs the reviewer to look again.

5. Evaluate each finding, and record the decision

Where a reviewer could post, its findings must be on the PR before any of them is triaged — on a thread, in the review body for the ones that could not be anchored, or in a PR issue comment where that is the only surface a reviewer used ("Reading reviewer state"). Not posted, not decided, and above all not fixed. Until then they exist only in this context, and a wake, a crash, or a context boundary between the reviewer returning and the review landing loses them: the PR never knew, and the next round re-finds what this one silently fixed. The quiet ending is the dangerous one — fix first and the round can complete, the commit can land, and the review can never post, leaving a PR that shows fixes with no findings behind them under a verdict that says converged. That is indistinguishable from a PR the reviewer found nothing wrong with, and on an unattended run it is what the user wakes up to.

The way this breaks is inlining. pr-review-skeptic posts its own findings, at its own stage 7. A caller that runs that skill's stages itself rather than invoking the skill can walk straight past that stage, because nothing here asks whether it happened. Invoke the skill; let it post.

A reviewer that could not post is a different case, and its existing rule governs unchanged — the *verdict,

Truncated - read the full file at https://github.com/kanetik/claude-skills/blob/5eb098eb25bc33b6450edba8368bdcf40c743200/skills/pr-review-loop/SKILL.md.