[garretpremo]

Closes #178

Summary

If a review-issue cron run dies before posting a verdict (timeout, crash), the PR stays labeled review in progress, and find-review-candidate.sh never selects it again. PR #177 hit this: the run waited on queued CI, was killed at the 10-minute job timeout, and every later tick was skipped_preflight until someone reset the label by hand.

This PR implements the approach agreed in the #178 triage decisions:

• A: take back a stale review in progress, at most once per head commit.
• B: defer PRs whose head checks are still pending.
• C: never wait for CI in cron mode.

It also paginates the script's issue-events lookups, which the maintainer added to the scope from #183's triage. And it raises the job timeout from 10m to 25m: today's runs on #185, #186 and #187 all timed out because the review's second-opinion subagent runs past 10 minutes, not because of CI.

What changes

find-review-candidate.sh

• A. Taking back a stale review in progress
  • A PR labeled review in progress is eligible again once its latest labeled event is older than the new optional [stale-after-seconds] argument. The default is 1800: the job's 25-minute timeout plus 5 minutes.
  • claude-cron takes the job lock before preflight runs, so a live cron run is never taken back. The threshold only protects interactive /review-issue sessions.
  • Cap: the script counts labeled: review in progress events after the head commit's committer date, or after the last labeled: needs review if that is later. At 2 or more it skips the PR and prints a note on stderr. So a review that dies every time is retried once, not forever.
  • A new commit, or a reset to needs review, re-arms it. That includes a cron run that gives up because CI is pending (C), so giving up doesn't use up the take-back.
• B. Deferring pending checks: an otherwise-eligible PR with pending checks (see pending-checks.sh below) is skipped, and the script moves on to the next PR.
• Pagination
  • The events API returns events oldest first, 30 to a page, and the script used to read only page 1. On a long-lived PR, that meant a fresh label could look stale, recent take-backs went uncounted, and the changes requested branch compared new commits against an old label.
  • The new helpers labeled_events (gh api --paginate ".../events?per_page=100", one line per event, because --jq runs once per page) and label_times (exact label match, then sort) now serve both the changes requested branch and the new review in progress branch.
• A non-numeric argument exits 2 with usage.
• The changes requested branch was re-indented into an if/elif chain so that every path falls through to the shared pending-checks gate. Apart from the paginated lookup, its logic is unchanged.

New pending-checks.sh <pr-number>

• It prints how many check runs on the PR's head commit have a status other than completed.
• 0 means nothing is pending. That covers checks that completed with any conclusion (failed, skipped and cancelled included), and a head with no check runs at all. The second case is real here: ci.yml and e2e.yml are path-filtered.
• It emits one line per pending run under --paginate (because --jq runs once per page) and counts them in shell.
• It reads commits/<sha>/check-runs, not the combined /status endpoint (which reports pending when there are no statuses) and not gh pr checks (which says "no checks reported" and exits 1 when there are none).
• B and C both use it, so the preflight and the agent can't disagree about whether CI is still running. A disagreement would let C's needs review reset loop forever, because that reset re-arms the cap.

set-review-label.sh

review in progress is re-applied (DELETE, then POST) even when the PR already has it. GitHub records no labeled event when you add a label that is already there, so without this a take-back attempt would leave no trace and the cap could never trigger. Other labels keep the old behavior, because re-applying first pass reviewed would restart final-review's soak window.

review-issue/SKILL.md

• Step 1.5: run set-review-label.sh even when the PR already shows review in progress.
• Step 4, new "Cron mode" callout:
  • Run pending-checks.sh <pr> once, right before deciding.
  • If the count is more than 0, or the script fails, post nothing, set needs review, and stop.
  • If it's 0, decide normally. That includes zero check runs ("no checks reported") and checks that finished as failed, skipped or cancelled.
  • No --watch, sleep or poll loops, background watchers, Monitor, or ScheduleWakeup.

.claude-jobs/review-issue.yaml

• timeout: 10m becomes 25m. A full review of a medium-sized PR, including the reviewer subagent, needs roughly 10 to 15 minutes. Each of today's four timed-out runs was killed while its subagent was still working.
• Passes 1800 explicitly at both call sites, with a comment tying it to timeout.
• Adds a matching "never wait for CI" paragraph to append_system_prompt.
• Allowlists pending-checks.sh.
• Adds disallowed_tools: ["ScheduleWakeup", "Monitor"]. This was checked against claude-cron's strict JobSchema, and its argv builder emits --disallowed-tools "ScheduleWakeup Monitor".

Tests

• tests/fixtures/fake-gh-review.sh: a fake gh driven by a JSON fixture.
  • It answers only the calls the scripts make and fails loudly on anything else.
  • It mirrors the GitHub and gh behavior these scripts depend on:
    • Events come one page at a time unless --paginate is passed.
    • --jq runs once per page.
    • Label writes record labeled / unlabeled events.
    • Adding an already-present label records no event.
    • gh pr view fails for an unknown PR.
  • A failingCheckRuns knob makes the check-runs call fail with HTTP 502.
• tests/fixtures/fake-gh-review.ts: fixture builders and a runner, in the same pattern as tests/notify-shipped-issues.test.ts.
• tests/find-review-candidate.test.ts: 24 tests.
  • The existing rules, as a regression guard; the script had no tests before.
  • Stale take-back: the threshold, the argument and its validation, and staleness judged from the latest attempt.
  • The cap, and the two ways to re-arm it (new commit, reset).
  • Pending checks: queued and in_progress, moving on to the next PR, red CI not deferring, the gate on every eligible path, and failing closed (no PR selected) when the checks can't be read.
  • Label events past a 100-event first page, on both branches.
  • An end-to-end test: the take-back goes through set-review-label.sh, and the next lookup is capped.
• tests/set-review-label.test.ts: 3 tests.
  • Re-apply when the label is already present.
  • Swapping the review label leaves other labels alone.
  • Other labels already present are not re-applied.
• tests/pending-checks.test.ts: 6 tests.
  • The count.
  • Completed checks don't count as pending, whether they succeeded, failed, were skipped or were cancelled.
  • A head with zero checks prints 0.
  • The PR's current head is the one checked.
  • It fails without printing a count when the API call fails, rather than printing 0.
  • Usage error without a PR number.

All new tests describe.skipIf(win32), like the existing script tests.

Acceptance criteria

• A PR whose review in progress label is older than the threshold is selected. One younger than the threshold is not.
• A take-back leaves a fresh labeled: review in progress event, even though the label is already present.
• A PR with 2 or more labeled: review in progress events newer than its head commit is skipped, with a stderr warning. A newer head commit makes it eligible again.
• A PR whose head has any check run that isn't completed is skipped, and the script still returns a later eligible PR.
• A PR with zero check runs, or with all check runs completed (including failures), stays eligible.
• Existing rules are unchanged:
  • lowest PR number first
  • needs review eligible
  • changes requested only with a newer commit
  • first pass reviewed and approved never selected
• In cron mode, the review skill never waits on CI. If CI is pending, the label goes back to needs review and the run exits without posting.
• Issue-events lookups read every page, for both the changes requested and review in progress branches. (Added to the scope by the maintainer.)
• New tests/find-review-candidate.test.ts (fake-gh harness) covers all of the above, plus set-review-label.sh's re-apply path.
• Uses the chore: commit prefix.

Test plan

• bun test: 1124 pass / 0 fail

• bun run lint: 0 errors (the 118 warnings already exist on dev; none come from the new files)

• shellcheck v0.10.0 --severity=warning over CI's file set (scripts/, .claude/skills/**/scripts/, .claude-jobs/, tests/): clean

• Mutation check on find-review-candidate.sh. Each of these makes at least one test fail:

  • cap raised to 3
  • reset boundary dropped
  • attempts counted from the start of time
  • staleness check disabled
  • staleness judged from the first attempt
  • argument ignored
  • --paginate removed
  • changes requested judged from its earliest label
  • pending-checks gate skipped
  • only queued treated as pending

• Removing pipefail from pending-checks.sh, or adding || true to its pipeline, makes both fail-closed tests fail. Without them, an API error would print 0.

• Removing the set-review-label.sh re-apply fails the re-apply test and the end-to-end cap test.

• review-issue.yaml parses under claude-cron's JobSchema (timeout 25m).

• The threshold tests pin the 1800 default: a 26-minute-old label (just past the timeout) is left alone, so a default at or below the 25-minute timeout fails that test, and so does the old 900.

• Self-review: an independent reviewer subagent (superpowers:requesting-code-review) reviewed the diff, and its findings were addressed:

  • B and C now share pending-checks.sh.
  • Several tests were hardened.
  • Monitor was added to disallowed_tools.
  • Fail-closed coverage was added.

  A second pass on the delta found no Critical or Important issues.

This doesn't reach the live cron until it's on main and the shared checkout claude-cron runs from has been pulled. Until then, the manual set-review-label.sh <pr> "needs review" workaround still applies.

Notes

• set-review-label.sh's re-apply leaves a tiny window, between its DELETE and POST, in which a failed POST would leave the PR with no review-state label. The script exits non-zero in that case, so the agent sees the error.
• final-review's similar stuck and looping cases are tracked in final-review cron: PRs strand or loop when a merge fails, a commit lands after first pass reviewed, or a gate fails #183, which builds on labeled_events and pending-checks.sh.

[garretpremo]

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

Looks good. A, B and C are implemented as the #178 triage decisions describe, every acceptance criterion is met and covered by tests, and CI is green at 4270a84. Nothing blocking.

What I checked beyond the diff:

• The 33 new tests pass locally (bun test on the three new files).
• I ran the PR's find-review-candidate.sh 1800 and pending-checks.sh read-only against the real repo. Both PRs open right now are in review in progress. The script took back #186 (label from 18:55Z, with its head committed after that, so it had 0 attempts), and pending-checks.sh 187 printed 0. An unknown PR number exits 1 with no count, so it fails closed.
• review-issue.yaml parses under claude-cron's JobSchema (timeout: 25m), and buildClaudeArgv emits --disallowed-tools "ScheduleWakeup Monitor".
• claude-cron takes the job lock before preflight (executor/run.ts), so the 1800s threshold only matters for interactive sessions, as the header says. The abandoned-run sweep scales to 2 × timeout (50m), so the longer timeout doesn't trip it.

Non-blocking observations

• A check run that never completes (an orphaned queued run, or a job waiting on a runner or an environment approval) defers the PR indefinitely. The only signal is the deferred note in the preflight stderr of claude-cron's collapsed skipped_preflight row. That's the same invisible-stall shape #178 set out to fix, though much rarer. A later follow-up could stop treating a check as pending after some maximum age.
• A new commit re-arms the cap by its committer date, the same as the existing changes requested branch. A commit dated before the last attempt (committed locally earlier, pushed after the cap hit) won't re-arm it. The reset to needs review remains the way out.

Nitpicks

• When the stranded label predates the current head (this PR's own state: review in progress at 19:10Z, head committed at 19:31Z), the first run on the new head goes through the take-back path, and the cap still allows one more take-back after that. That means two take-backs on one head. It matches the triage rule exactly ("count events newer than the head commit; at 2 or more, skip"), and the invariant really is "at most two attempts per head commit or per reset". The header's "taken back at most once per head commit" is slightly loose for this case.
• pending-checks.sh re-resolves the head with gh pr view and re-sources gh-pin-account.sh even when find-review-candidate.sh already has the SHA. That's about two extra gh calls per gated PR inside the 30s preflight budget. Fine at today's PR volume.
• Step 1 of SKILL.md still runs gh pr checks up front, and the cron-mode guidance lives in step 4 and in the system prompt. A one-line pointer from step 1 to the step 4 callout would mean an agent that sees pending checks early doesn't have to rely on the system prompt alone.

[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

• A check run that never completes (an orphaned queued run, or a job waiting on a runner or an environment approval) defers the PR indefinitely. The only signal is the deferred note in the preflight stderr of claude-cron's collapsed skipped_preflight row. That's the same invisible-stall shape #178 set out to fix, though much rarer. A later follow-up could stop treating a check as pending after some maximum age.
• A new commit re-arms the cap by its committer date, the same as the existing changes requested branch. A commit dated before the last attempt (committed locally earlier, pushed after the cap hit) won't re-arm it. The reset to needs review remains the way out.