[garretpremo]

Closes #183

Summary

The final-review cron could leave a PR stuck, or keep re-processing it every tick:

1. Stuck in approved: approved was applied before the merge, so a failed merge (409 because the head moved, 405 because it isn't mergeable, an API error) or a run killed in between left the PR open with a label neither cron selects.
2. Stuck in first pass reviewed: a commit pushed after that label made final-review skip the PR as stale, and the review cron never looked at that label either.
3. A comment every 5 minutes: a failed gate posted a comment without changing the label, so the next tick re-selected the PR and commented again.

This PR implements the #183 triage decisions, building on #178's pending-checks.sh and paginated label-event helpers. It also makes #178's take-back cap independent of the review agent, which the maintainer added to the scope.

What changes

final-review: merge first, then label (finding 1)

• SKILL.md and final-review.yaml swap the order. It is now merge-pr.sh <pr> "$HEAD_SHA", then set-review-label.sh <pr> "approved", then label-merged-issues.sh. The approval comment still goes out before the merge.
• On a merge failure: first check gh pr view --json state. If the merge landed anyway (for example, a client-side timeout), it carries on with the label steps. Otherwise:
  1. Write final-<pr>-merge-fail.md, quoting the error merge-pr.sh printed.
  2. Post it with scripts/pr-comment.sh.
  3. Run set-review-label.sh <pr> "needs review".
  4. STOP, without retrying.
• A run killed before the merge leaves first pass reviewed, so the next tick re-checks the gates (or, if the head moved, the review cron re-reviews the PR). A run killed after the merge leaves a closed PR, so the missing approved label strands nothing.
• Corrected text: SKILL.md step 6, the red flags, the skill description, the yaml description and header, and the merge-pr.sh header.

Re-review a stale first pass reviewed (finding 2)

• In find-review-candidate.sh, the changes requested branch now covers first pass reviewed as well: the PR is eligible when its head commit is strictly newer than the latest label. That's the same comparison find-final-review-candidate.sh uses, so each first pass reviewed PR belongs to exactly one of the two crons.
• It goes through review-issue cron: a run that dies mid-review strands the PR in review in progress #178's pending-checks gate.
• approved is still never selected.

Stop the gate-failure loop (finding 3)

• Preflight: find-final-review-candidate.sh defers any PR whose head has a check run that isn't completed. It uses review-issue's pending-checks.sh (the same check-runs query), prints a stderr note, and moves on to the next PR. Red CI stays eligible, so the gate can report it.
• Preflight, drafts: find-final-review-candidate.sh also skips draft PRs. GitHub refuses to merge a draft (405), and with the new merge-failure path, selecting one would bounce it through a full re-review every cycle.
• Skill gate: split into four outcomes in SKILL.md and the yaml, using gh's real mergeable values (MERGEABLE / CONFLICTING / UNKNOWN). When both hard and soft conditions hold, hard wins. The missing-body check (step 2) runs only after the gate passes.

Outcome	Conditions	Action
Nothing to do	PR isn't OPEN, is a draft, or first pass reviewed is gone	STOP silently
Hard	a check failed, was cancelled or timed out; CONFLICTING / DIRTY; an active request-changes review	One comment (naming the gate and how to re-arm), then changes requested, then STOP
Soft	a check pending / queued / in_progress; mergeable is UNKNOWN	STOP, with no comment and no label change
Hard, no first pass	no prior first-pass review body	One comment, then needs review, then STOP

After a hard failure, a changes requested PR stays out of both crons until a newer commit lands. The comment also says how to re-arm the PR once a CI re-run turns a flaky failure green: run set-review-label.sh <pr> "first pass reviewed", which restarts the soak. For an active request-changes review, it also says the review has to be dismissed first.

Paginate the final-review events read

find-final-review-candidate.sh read only the first 30 issue events. It now uses gh api --paginate ".../events?per_page=100", emitting one line per event (--jq runs once per page) and taking sort | tail -n 1. find-review-candidate.sh has done this since #178.

Take-back cap independent of the review agent (maintainer addition)

• The gap: review-issue cron: a run that dies mid-review strands the PR in review in progress #178 caps take-backs by counting labeled: review in progress events, but it relied on the review agent re-running set-review-label.sh in step 1.5. A take-back run whose agent skipped that step and then died left no event, so the PR was taken back again on every tick.
• New claim-review-candidate.sh:
  • It runs find-review-candidate.sh, then set-review-label.sh --reapply <pr> "review in progress" (label listing to stderr), then prints the PR number.
  • review-issue.yaml's prompt_cmd now calls it under set -euo pipefail.
  • If the claim fails, prompt_cmd exits non-zero, and claude-cron doesn't start the run (executor/prompt.ts).
• set-review-label.sh: review-issue cron: a run that dies mid-review strands the PR in review in progress #178's special case (always re-apply review in progress) becomes an explicit --reapply flag. Without it, a label that's already present is left alone, so the agent's own step-1.5 call adds no second event.
• Take-backs only: the claim fires only when the picked PR is already review in progress, i.e. a take-back. A first pick is still labeled by the agent in step 1.5. If claude fails fast (usage limit, expired auth, a broken CLI), the PR stays needs review and recovers by itself once claude is back; it isn't claimed, taken back and capped. An outage can cap at most a PR whose earlier run had actually started.
• Result: every take-back run leaves exactly one labeled event, whatever the agent does. Step 1.5's text now explains a PR that already shows review in progress.
• Six-hour attempt window (maintainer ruling): the cap now counts attempts only after the latest of the head commit, the last reset to needs review, and 6 hours ago (attempt_window=21600, next to the threshold, documented in the script header and the yaml comment). A PR capped during an outage re-arms by itself, and a review that dies every time costs at most two runs per 6 hours.
• Label swap order: set-review-label.sh now adds the target before removing the other review-state labels, so a failure never leaves a PR with none. It also retries the POST once, because under --reapply the label has just been removed.

Tests

• Fake gh: now honors pr list --label and isDraft, and has a failLabelPosts counter. strandedReview moved to the shared fixtures module, and there's a new runArgvWithFakeGh.
• tests/find-review-candidate.test.ts:
  • "first pass reviewed is never selected" becomes "never selected without a newer commit".
  • New: a stale first pass reviewed is selected; a commit in the same second as the label is left to final-review; a stale first pass reviewed is deferred while checks are pending.
  • New: an attempt older than 6 hours doesn't count toward the cap, and one inside the window does. A 5-hour or an 8-hour window each fails one of these tests.
  • The old end-to-end cap test moved to the claim tests.
• New tests/find-final-review-candidate.test.ts: 13 tests.
  • The existing rules: soak, min-age argument, stale commit, label filter.
  • A commit in the same second as the label stays with final-review (the mirror of the review-side test).
  • Drafts are skipped.
  • Deferral on queued and in_progress, moving on to a later PR, red CI not deferring, and failing closed when the checks can't be read.
  • The latest label found past a 100-event first page.
• New tests/claim-review-candidate.test.ts: 9 tests.
  • A first pick's label is left to the agent.
  • Nothing is claimed when nothing is eligible.
  • A take-back records a fresh event.
  • A take-back run that dies is capped, whether its agent ran step 1.5 or not.
  • A first run whose agent never starts leaves the PR eligible as it was.
  • A first run that dies still gets its take-back (no double count).
  • The job's real prompt_cmd, run through bash -c: it claims a take-back and prompts with only /review-issue <n>; with nothing eligible it fails without a prompt.
• tests/set-review-label.test.ts: updated for --reapply, plus two new tests: a failed POST is retried once, and a swap whose POSTs fail keeps the old label.

Acceptance criteria

• final-review applies approved only after merge-pr.sh succeeds.
• When the merge fails, the PR ends in needs review with exactly one comment naming the failure, and the run doesn't retry.
• SKILL.md step 6, its red flags and the matching final-review.yaml text describe the actual recovery path.
• find-review-candidate.sh selects a first pass reviewed PR whose head commit is newer than its latest first pass reviewed label, subject to review-issue cron: a run that dies mid-review strands the PR in review in progress #178's pending-checks deferral. It doesn't select one whose head is older than the label. approved is still never selected.
• find-final-review-candidate.sh skips a PR whose head has any check run that isn't completed, and still returns a later eligible PR. A PR with zero check runs, or with every check run completed (including failures), stays eligible.
• A hard gate failure produces one comment and a label move: to changes requested, or to needs review when there's no first-pass body. A soft failure produces no comment and no label change. After a hard changes requested failure, neither candidate script selects the PR until a newer commit lands. (After the no-first-pass case, the review cron picks the PR up right away, which is intended.)
• Both candidate scripts find the latest labeled event past the first page of issue events. This is tested with a fake gh that returns only the first page unless --paginate is passed.
• Tests: tests/find-review-candidate.test.ts is extended, and tests/find-final-review-candidate.test.ts is added.
• Maintainer addition: every take-back run leaves exactly one labeled: review in progress event, whether or not the agent runs step 1.5. A first pick whose run fails before the agent starts leaves the PR eligible as it was.
• Uses the chore: commit prefix.

Test plan

• bun test: 1221 pass / 0 fail
• bun run lint: 0 errors (the 117 warnings already exist on dev; none come from the touched files)
• shellcheck v0.10.0 --severity=warning over CI's file set: clean
• review-issue.yaml and final-review.yaml parse under claude-cron's JobSchema.
• Self-review: an independent reviewer subagent (superpowers:requesting-code-review) reviewed the diff, and its findings were addressed:
  • The claim is scoped to take-backs, so a Claude outage no longer caps every eligible PR.
  • The POST comes first in a label swap, and is retried once.
  • Drafts are filtered.
  • Tests were added for the prompt_cmd failure path and the final-review same-second case.
  • Prose was added for dismissing a request-changes review, for a merge that landed despite an error, and for check precedence.
• Mutation check. Each of these makes at least one test fail:
  • --paginate dropped from find-final-review-candidate.sh
  • its pending gate skipped
  • the first pass reviewed branch dropped
  • >= instead of >
  • the claim without --reapply
  • the claim's label listing on stdout
  • set-review-label.sh always re-applying
  • claiming every pick, not just take-backs
  • no POST retry
  • drafts not filtered
  • >= on the final-review side
  • set -euo pipefail removed from prompt_cmd

The final-review skill and yaml changes are agent instructions with no automated test. Like #178, this takes effect only once the release reaches main and the shared checkout claude-cron runs from is pulled.

Known limitations

• First-run gap (existed before review-issue cron: a run that dies mid-review strands the PR in review in progress #178): an agent that skips step 1.5 on a first run and then dies leaves the PR needs review, so it's picked again on every tick. The claim covers only take-backs, by design: claiming every pick would close this gap, but then a Claude outage would leave every eligible PR capped for up to 6 hours. The maintainer chose take-backs only.
• A review that always dies keeps costing: with the 6-hour window, a PR whose review dies every time (for example, one that consistently overruns the 25-minute timeout) costs at most two runs per 6 hours, about 8 a day, until someone notices. The only signal is the cap's stderr line in claude-cron's preflight log. A one-time PR comment when the cap is reached could be a follow-up.
• Both POSTs failing under --reapply: set-review-label.sh --reapply is a DELETE followed by a POST, retried once. If both POSTs fail, the PR is left with no review-state label. The claim then exits non-zero, so the run doesn't start, but the PR needs a manual label.
• prompt_cmd budget: claude-cron gives prompt_cmd a hardcoded 30 seconds. That now covers the finder, one labels GET, and the label calls on a take-back. It's well within budget at current PR volume, and the yaml comment notes the limit.
• Committer-date staleness (out of scope, as the triage noted): both crons decide staleness by the head commit's committer date. A commit made before a label but pushed after it looks already reviewed. Comparing the head SHA with the commit_id of the latest automated review would close that gap.
• Possible follow-up (existed before this PR): final-review's request-changes query (state == "CHANGES_REQUESTED" and .dismissed_at == null) counts superseded change-request reviews.

[garretpremo]

🤖 Automated review by claude — generated by the review-issue skill. Treat as advisory; a human still owns the merge decision.

Every acceptance criterion from #183's triage, plus the maintainer's take-back addition, is met. CI is green at 6baf425, and the settled decisions (merge then label, the merge-failure path, the stale first pass reviewed rule, the preflight's pending and draft filters, the gate tiers, the take-back-only claim with its 6-hour window, POST before DELETE) are implemented as described. I also checked locally:

• The four affected test files pass (55 tests).
• Both yamls parse under claude-cron's JobSchema.
• Six spot-check mutations each fail at least one test: claim without --reapply, claim on every pick, no POST retry, no attempt window, final side -ge, no draft filter.

Non-blocking observations

• Interactive take-over regression.
  • Step 1.5 no longer refreshes review in progress, because set-review-label.sh without --reapply leaves a present label alone.
  • So an interactive /review-issue on a stranded PR is no longer protected: once the dead run's label is 30 minutes old, the cron takes the PR back, and two reviews run at once.
  • Under #187, step 1.5's re-apply both restarted the timer and reached the cap.
  • This PR is a live example: a killed cron run labeled it at 21:05Z, and this review is an interactive stand-in.
  • find-review-candidate.sh:32-35 still says the threshold protects interactive sessions.
  • Fix: in step 1.5, have interactive mode pass --reapply when the PR already shows review in progress.
• The final-review gate and the preflight disagree about checks.
  • The gate reads gh pr checks, which prints "no checks reported" and exits 1 when the head has zero check runs. That happens after a follow-up commit that touches only paths outside CI: ci.yml and e2e.yml are path-filtered, and the auto-labeler only runs on opened.
  • The preflight treats that head as eligible, but the gate table doesn't say it passes. An agent that reads it as a failure now sends the PR to changes requested, where before it only posted a comment.
  • gh pr checks also counts commit statuses as pending, and pending-checks.sh doesn't read those. A stuck status would end in a soft STOP every tick. This is latent: the repo posts no statuses.
  • Mirroring the cron-mode note in review-issue step 4, or using pending-checks.sh for the pending check, would align the two.
• mergeStateStatus BLOCKED is neither hard nor soft.
  • The gate passes, and the merge returns 405. The PR goes to needs review, gets a full re-review, returns to first pass reviewed, and gets another 405. Each cycle costs a review run and three comments.
  • This is latent: the "dev and next" ruleset has enforcement disabled today.
  • Adding BLOCKED to the hard row closes it without changing the merge-failure decision.
• Nothing caps a re-review of a stale first pass reviewed PR.
  • Suppose the agent skips step 1.5, or stops early because it sees a passing automated review. Then the verdict call set-review-label.sh <pr> "first pass reviewed" changes nothing, the label time stays put, and the PR is re-selected every tick. A completed run ends up looping.
  • changes requested already has the same mechanism.
  • Two ways to cover both labels: add a sentence to step 1.5 ("a cron pick may still show first pass reviewed because a commit landed after the first pass; review the new head"), or pass --reapply on the verdict calls.
• Pre-existing, possible follow-up.
  • final-review captures HEAD_SHA after the preflight's staleness check, and step 1 doesn't compare the head with the label again.
  • So a push in that gap whose head has no check runs (paths outside CI only) would pass the gate and be merged unreviewed.

Nitpicks

• final-review.yaml:14-16 says every path ends in a state that "won't be re-selected unchanged", then lists the soft failure, which is re-selected unchanged by design.
• The pending-checks.sh header still names only the review cron and the review-issue skill as its consumers. The final-review preflight now uses it too.
• Two test gaps:
  • no final-side test that a changes requested PR isn't selected (only the label filter covers that);
  • no set-review-label.sh test that a failed DELETE leaves both labels, which is the other half of the POST-first guarantee.
• claim-review-candidate.sh:27-29 decides it's a take-back by re-reading the labels. That pair became possible once a swap's DELETE can fail, so a first pick showing both needs review and a leftover review in progress gets --reapply'd. It's harmless (it clears the extra label), but it contradicts the header's "a first pick is left to the agent".

[garretpremo]

🤖 Final review by claude — generated by the final-review skill. CI is green, no blockers, soak window elapsed. Marking approved and merging.

Approved.

Outstanding observations from first-pass review

• Interactive take-over regression.
  • Step 1.5 no longer refreshes review in progress, because set-review-label.sh without --reapply leaves a present label alone.
  • So an interactive /review-issue on a stranded PR is no longer protected: once the dead run's label is 30 minutes old, the cron takes the PR back, and two reviews run at once.
  • Under #187, step 1.5's re-apply both restarted the timer and reached the cap.
  • This PR is a live example: a killed cron run labeled it at 21:05Z, and this review is an interactive stand-in.
  • find-review-candidate.sh:32-35 still says the threshold protects interactive sessions.
  • Fix: in step 1.5, have interactive mode pass --reapply when the PR already shows review in progress.
• The final-review gate and the preflight disagree about checks.
  • The gate reads gh pr checks, which prints "no checks reported" and exits 1 when the head has zero check runs. That happens after a follow-up commit that touches only paths outside CI: ci.yml and e2e.yml are path-filtered, and the auto-labeler only runs on opened.
  • The preflight treats that head as eligible, but the gate table doesn't say it passes. An agent that reads it as a failure now sends the PR to changes requested, where before it only posted a comment.
  • gh pr checks also counts commit statuses as pending, and pending-checks.sh doesn't read those. A stuck status would end in a soft STOP every tick. This is latent: the repo posts no statuses.
  • Mirroring the cron-mode note in review-issue step 4, or using pending-checks.sh for the pending check, would align the two.
• mergeStateStatus BLOCKED is neither hard nor soft.
  • The gate passes, and the merge returns 405. The PR goes to needs review, gets a full re-review, returns to first pass reviewed, and gets another 405. Each cycle costs a review run and three comments.
  • This is latent: the "dev and next" ruleset has enforcement disabled today.
  • Adding BLOCKED to the hard row closes it without changing the merge-failure decision.
• Nothing caps a re-review of a stale first pass reviewed PR.
  • Suppose the agent skips step 1.5, or stops early because it sees a passing automated review. Then the verdict call set-review-label.sh <pr> "first pass reviewed" changes nothing, the label time stays put, and the PR is re-selected every tick. A completed run ends up looping.
  • changes requested already has the same mechanism.
  • Two ways to cover both labels: add a sentence to step 1.5 ("a cron pick may still show first pass reviewed because a commit landed after the first pass; review the new head"), or pass --reapply on the verdict calls.
• Pre-existing, possible follow-up.
  • final-review captures HEAD_SHA after the preflight's staleness check, and step 1 doesn't compare the head with the label again.
  • So a push in that gap whose head has no check runs (paths outside CI only) would pass the gate and be merged unreviewed.