Repository navigation
pr-review: don't skip labeled PRs that are already approved #11
Description
Problem
pr-review-scanner.sh drops PRs whose reviewDecision is APPROVED, even if they carry ready-for-ai-review:
# Filter out bot's own PRs, drafts, and already-approved PRs
select(.reviewDecision != "APPROVED")(skills/repoman/scripts/pr-review-scanner.sh, at b3f128c)
So whether a labeled PR gets an AI review depends on timing, not intent:
- If the scanner reaches it before a human approves it, it gets an AI review, and later a human approval: 2 reviews.
- If a human approves first, the scanner never picks it up: 1 review.
Real case: rossoctl/cortex#1535. Labeled at 15:37Z, approved at 15:42Z, merged at 16:44Z, with no AI review. The reviewer was healthy: the OpenClaw scan logs show "0 eligible" across that window. OpenClaw has the same filter, so moving to RepoMan won't fix this by itself.
An AI review from a different model is useful even after a human approval. The author added the label to ask for one.
Proposal
Treat ready-for-ai-review as an explicit opt-in. Labeled PRs are eligible no matter what reviewDecision is. Keep the other filters (bot-authored, draft).
Duplicates are already handled by state.json (in_progress / reviewed) and the <!-- reviewed: <sha> --> marker. This change still gives one review per labeled head SHA. (Note: #9 did get a second review on an unchanged head, 81f19ab, so that path may need a look too.)
Optional: if a skip-approved default is still wanted for other setups (e.g., future org-wide or unlabeled enrollment), make it a per-program config flag, e.g. skip_approved in ~/.repoman/programs/pr-review.json. Default it to false when labels_required is set.
Acceptance criteria
- A labeled, non-draft PR with
reviewDecision == APPROVEDshows up inlatest.jsonas a candidate - A PR already reviewed at its current head SHA is still skipped (no duplicate)
- A new push to an approved, labeled PR gets reviewed at the new head
- SKILL.md / docs describe the eligibility rule
Context
Raised by @huang195 in #rosso-automation on 2026-10-29. Root-caused with scan logs from @rubambiza.
Generated by RepoMan
Activity
jimcadden commentedon Oct 29, 2026
I think this is right. The human reviewer can remove the "ready-for-ai-review" label if an AL pass isn't necessary. Worst care, a 2nd opinion.
clawgenti commentedon Oct 29, 2026
Maintainer context from @rubambiza (#rosso-automation, 2026-10-29): skipping approved PRs is intentional. The rossoctl loop:
ready-for-ai-review.ready-for-human-reviewand pings Slack for a second human (B).ready-for-ai-reviewis re-added.In this loop, an approval marks the end of the AI phase. cortex#1535 took a different path: it was labeled, then a human approved it before the first sweep, so the AI phase never ran.
Options:
ready-for-ai-reviewwas applied after the latest approval (from the timelinelabeledevent vs. reviewsubmitted_at).Generated by RepoMan
clawgenti commentedon Oct 29, 2026
Scope agreed in #rosso-automation (2026-10-29): @rubambiza chose the narrow fix, and @huang195 and @jimcadden agree. The rule is that
ready-for-ai-reviewmeans an AI review happens whether or not a human has reviewed the PR.Revised acceptance criteria (supersedes the ones in the issue body)
reviewDecision == APPROVEDis eligible if:ready-for-ai-reviewwas applied after the latest approval. Compare the timelinelabeledevent with the review'ssubmitted_at.state.jsonplus the<!-- reviewed: <sha> -->marker).ready-for-human-review→ human approval) and this eligibility rule.Note: in the normal loop the author swaps
ready-for-ai-reviewforready-for-human-reviewbefore the final approval. So a PR that still carries the AI label after approval almost always means someone wants an AI review. In practice that makes the narrow fix close to "label present ⇒ review once per head".Generated by RepoMan