From 7a52040e03ba31ebe326f4eec0c6a1912ea7f8fb Mon Sep 17 00:00:00 2001 From: Srinivas Annam Date: Sun, 13 Sep 2026 22:02:26 -0700 Subject: [PATCH] chore(skills): teach the PR skills that an open thread blocks the merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `main` now carries `required_conversation_resolution: true`, so an approving review is no longer the whole merge gate โ€” every unresolved review thread blocks on its own. The five PR skills all predate that and say otherwise. The change that prompted it: #924 and #926 both merged with 0 blockers, every Secondary thread still open and unanswered, and no follow-up filed. Two real defects went into `main` unrecorded and had to be reconstructed by hand, hours later, as #930 and #931. Neither was blocking. Both were right. pr-review gains the two steps that close that hole: - **Step 5.7** files a follow-up issue for any finding that outlives the run, reviewer-side, before the review posts so the body can cite the number. The bar is *not filing loses information* โ€” not "every Secondary": nothing for a finding Step 5.5 already fixed, a Step 5.6 suggestion, a trade-off the code documents, or anything Blocking, since an issue would launder that into a nit. On a `REQUEST_CHANGES` verdict it files only out-of-scope findings: the author is about to revise, and an in-scope Secondary belongs in that round. Each finding is re-verified where the defect lives โ€” code the PR adds on the PR branch, cited by path since the squash moves its lines; an out-of-scope copy on `origin/main`, cited by `file:line`. It never files under `--local`, which prints what it would file instead. - **Step 6.5** replies to and resolves the threads the reviewer itself disposed of (filed as `#N`, trade-off it agrees with), and leaves open exactly what the author owes an answer to. A Step 5.5 fix never opens a thread, so it is listed under `## Fixed in ` rather than resolved. The step runs after Step 6 because thread IDs do not exist until the comments are posted. Reply always precedes resolve: a silently resolved thread reads as a withdrawn finding. The listing keeps only threads whose first comment belongs to the review Step 6 just posted (`REVIEW_ID`, captured from whichever POST landed, the 422 retry included, and refused when empty rather than spliced into the jq), so a human's earlier thread that the auto-fix happened to cure gets a reply naming the SHA, never a resolve โ€” the same no-unilateral-resolve rule revise-pr holds the author to, enforced in the recipe rather than in prose. Suggestion blocks stay open, and Step 7 says so: GitHub marks an applied suggestion outdated, not resolved, so the author still has to close each one by hand (or run `/revise-pr`) before the merge unblocks. `--local` now suppresses every write it promises to: Step 5.5's auto-fix gate tests it, so a preview no longer commits, pushes or collapses someone's branch, and Step 0.6 skips the ๐Ÿ‘€ reaction, which `pr-ready` would otherwise read as a live review hold for a review that never posts. Two latent bugs the sweep turned up, both of which would have reported a blocked PR as ready: - **pr-ready's preflight was blind to the gate.** It keys on `.mergeable`, which is only `MERGEABLE`/`CONFLICTING`/`UNKNOWN` โ€” it answers "do the diffs conflict", not "does branch protection allow the merge". #906 reads `MERGEABLE` with four open threads. It now counts threads over GraphQL and shows the count beside each PR, and Phase 6 re-counts rather than reusing the preflight number, since a review arriving during the wait is what opens them. The count is disclosed, never an exclusion: the sibling skills leave threads open on purpose โ€” a suggestion for the author to apply and resolve, a pushback for the reviewer to adjudicate โ€” so excluding on it would withhold the ask from the PRs that need a reviewer next, and abort pr-autopilot's single-PR ask. - **pr-autopilot had no row for approved-with-open-threads.** The MERGE READY row's conditions read as a description rather than a test, so that state would have converged. It is now its own row, spends no human cycle (nobody rejected anything), and branches on whether `/revise-pr` pushed โ€” `dismiss_stale_reviews` means a fix commit silently drops the approval back to `REVIEW_REQUIRED`, and the pushed branch then takes the same Phase 1 โ†’ Phase 2 โ†’ Phase 3 route as the changes-requested row, so no human re-approves code a bot pass has not read. A second pass with threads still open ends as its own terminal state, `APPROVED ยท THREADS OPEN`, listing each thread and whose reply it waits on โ€” not BOT STUCK, since the usual cause is a pushback waiting on the reviewer. Phase 5 re-checks all four claims the MERGE READY line makes โ€” approval, thread count, `mergeable`, and every status check against pr-ready's green allowlist โ€” immediately before posting it. revise-pr's "don't resolve a deferral" rule splits under the new gate. A deferral the reviewer asked for or agreed to on the thread now resolves: the work has moved to an issue by agreement, and leaving the thread open blocks the PR on a task that has left it. A deferral on the author's judgement alone does not โ€” it is a pushback with an issue attached, and it stays open for the reviewer, the same rule pr-review Step 6.5 holds the reviewer to. collapse-pr's thread gate stays soft โ€” collapsing is not merging โ€” but now prints its count even under `--yes` and says it holds the merge. Every thread count and thread list โ€” collapse-pr's gate and revise-pr's work list included โ€” fails closed past 100 threads rather than undercounting, since the count now decides MERGE READY and a dropped thread is one nobody answers. collapse-pr reports that case as `unknown` rather than a blank count. `mergeStateStatus` is a cross-check only: it reads `BLOCKED` the same for an unresolved thread, a missing approval and a red check, so it can say that something blocks but never what โ€” though once all four specific checks pass, a `BLOCKED` that remains is reported rather than posted over. Every GraphQL snippet was run verbatim against a live PR before being written down, and Step 6.5's reply and `resolveReviewThread` calls were exercised on the two real threads on #924 and #926 โ€” each now carries a reply naming #930 / #931 and is resolved. --- .claude/skills/collapse-pr/SKILL.md | 16 +- .claude/skills/pr-autopilot/SKILL.md | 71 +++++++- .claude/skills/pr-ready/SKILL.md | 54 +++++- .claude/skills/pr-review/SKILL.md | 250 ++++++++++++++++++++++++--- .claude/skills/revise-pr/SKILL.md | 40 +++-- 5 files changed, 384 insertions(+), 47 deletions(-) diff --git a/.claude/skills/collapse-pr/SKILL.md b/.claude/skills/collapse-pr/SKILL.md index f1a0011e..8909d261 100644 --- a/.claude/skills/collapse-pr/SKILL.md +++ b/.claude/skills/collapse-pr/SKILL.md @@ -394,11 +394,13 @@ if [ -n "$PR_NUM" ]; then OPEN_THREADS="$(gh api graphql -f query=' query($owner:String!,$name:String!,$pr:Int!){ repository(owner:$owner,name:$name){ - pullRequest(number:$pr){ reviewThreads(first:100){ nodes{ isResolved path line } } } + pullRequest(number:$pr){ reviewThreads(first:100){ + pageInfo{ hasNextPage } nodes{ isResolved path line } } } } }' -f owner="$OWNER" -f name="$NAME" -F pr="$PR_NUM" \ - --jq '[.data.repository.pullRequest.reviewThreads.nodes[] - | select(.isResolved == false)] | length')" + --jq '.data.repository.pullRequest.reviewThreads + | if .pageInfo.hasNextPage then error("more than 100 review threads โ€” paginate before trusting this count") + else [.nodes[] | select(.isResolved == false)] | length end' || echo unknown)" fi ``` @@ -411,6 +413,14 @@ whole branch and costs them that diff.* This is `revise-pr` Step 5.1's rule; the decision of whether this round is the final one belongs to the caller, which is why the gate is soft. +**Print the count even when the caller passes `--yes`, and say it holds the merge.** +`main` carries `required_conversation_resolution: true`, so every one of these threads +blocks the merge until someone resolves it โ€” a collapsed, queue-ready branch with three +open threads is not mergeable, and a caller who read only "collapsed โœ“" will believe it +is. The gate stays **soft** regardless: collapsing is not merging, and a final-round +collapse alongside a thread the reviewer still owes an answer on is a legitimate state. +Report it; never decide it here. + #### 3c โ€” Branch ownership (hard) ```bash diff --git a/.claude/skills/pr-autopilot/SKILL.md b/.claude/skills/pr-autopilot/SKILL.md index dadd8007..3d6f59f7 100644 --- a/.claude/skills/pr-autopilot/SKILL.md +++ b/.claude/skills/pr-autopilot/SKILL.md @@ -13,7 +13,8 @@ conflict, or a review comment back and forth by hand: ``` resolve conflicts โ†’ loop( /pr-review โ†’ /revise-pr ) until bot-clean โ†’ /pr-ready (solicit a human) โ†’ read the outcome - โ”œโ”€ approved, clean โ†’ done: report MERGE READY, stop + โ”œโ”€ approved, 0 open threads โ†’ done: report MERGE READY, stop + โ”œโ”€ approved, threads open โ†’ /revise-pr to close them, then re-check โ”œโ”€ changes requested โ†’ fold back through the bot loop, re-solicit โ””โ”€ silent / still reviewing โ†’ done for this run: report the state, stop ``` @@ -174,11 +175,20 @@ From `/pr-ready`'s report for this PR: | `/pr-ready` state | What this skill does | |---|---| -| `REVIEWED (APPROVED)`, mergeable, checks green, no unresolved threads | **MERGE READY** โ€” go to Phase 5 (converged). | +| `REVIEWED (APPROVED)`, mergeable, checks green, **0 unresolved threads** | **MERGE READY** โ€” go to Phase 5 (converged). | +| `REVIEWED (APPROVED)` with **โ‰ฅ1 unresolved thread** | Approved but **not mergeable** โ€” `main` requires conversation resolution. Nobody rejected anything, so this does **not** spend a human cycle. Run `/revise-pr [--repo ]` once, then re-check. See the dismissal note below: if that run *pushed*, the approval is gone, and the pushed branch goes through Phase 1 (recheck conflicts) โ†’ Phase 2 (a fresh bot pass, bot-round counter reset to 0) โ†’ Phase 3 to re-solicit โ€” the same route the `CHANGES_REQUESTED` row takes after the same push, because a human must not be asked to re-approve code no bot pass or conflict check has seen; if it closed the threads with replies alone, the approval stands and this goes straight to **MERGE READY**. Allow this path **once per human cycle** โ€” a second pass through it with threads still open ends the run as **APPROVED ยท THREADS OPEN** (Phase 5), not a third attempt. It is not BOT STUCK: the usual cause is a `/revise-pr` pushback, which that skill leaves open for the reviewer by rule, so nothing is wrong with the bot and the next move is whoever each thread is waiting on. | | `REVIEWED (CHANGES_REQUESTED)` | A human found something. Increment the **human-cycle counter**. If `โ‰ค --max-human-cycles`: **run `/revise-pr [--repo ]` directly** โ€” not `/pr-review` first (see below) โ€” reset the bot-round counter to 0, then go through Phase 1 (recheck conflicts) โ†’ Phase 2 (a fresh bot pass โ€” now meaningful, since the human's thread is already addressed) โ†’ Phase 3 again to re-solicit. If the counter now exceeds the bound: **HUMAN STUCK** โ€” go to Phase 5 (stuck). | | `SILENT`, `ACKED`, `REVIEWING`, or `HELD BUT STALE` | Nobody rejected anything โ€” there is nothing for the bot loop to act on, and `/pr-ready`'s own wait for this run is already over. Go to Phase 5 and report this state as-is; re-running `/pr-autopilot` later will pick it up. | | `REVIEWED (COMMENTED)` with no `CHANGES_REQUESTED` from anyone | Not a blocking review. Treat like the row above โ€” report and stop; don't manufacture a revision cycle out of a non-blocking comment. | +**A push after an approval dismisses it โ€” `main` has `dismiss_stale_reviews: true`.** That +is why the approved-with-threads row branches on whether `/revise-pr` pushed rather than on +whether it "succeeded": a run that only replied and resolved leaves the approval intact and +converges immediately, while a run that committed a fix has silently put the PR back to +`REVIEW_REQUIRED`. Read `reviewDecision` after `/revise-pr` returns rather than assuming +either outcome โ€” treating a dismissed approval as still-standing is how this skill would +report `MERGE READY` on a PR that needs a whole new reviewer. + **Why `/revise-pr` runs first, not Phase 2's `/pr-review`.** `/pr-review` never reads existing review threads โ€” its findings come from the issue, the diff, and the offlinecv gates alone (see its own Steps 0โ€“4). Re-entering through @@ -212,11 +222,56 @@ approved, no conflicts, checks green, no unresolved threads. Merge is yours whenever you're ready. ``` -**BOT STUCK / HUMAN STUCK / not-yet-engaged:** post the equivalent short status -(round/cycle count, what's outstanding, and โ€” for the not-yet-engaged case โ€” that -`/pr-ready`'s wait already ran once and re-running `/pr-autopilot` will check -again) and say so in the session. Don't dress up an unresolved state as -converged. +**Verify that last clause before posting it; never carry it over from Phase 4.** Every +claim in the line is re-checkable in one call, and the thread count is the one that moves +most โ€” `/revise-pr` resolving threads, or a reviewer opening new ones, both land between +the two phases: + +```bash +# notGreen uses pr-ready's allowlist: a running check (empty/null conclusion) is not green +gh pr view --repo --json reviewDecision,mergeable,mergeStateStatus,statusCheckRollup \ + -q '{reviewDecision, mergeable, mergeStateStatus, + notGreen: [.statusCheckRollup[] + | select(((.conclusion // .state) as $s | ["SUCCESS","NEUTRAL","SKIPPED"] | index($s)) | not) + | "\(.name // .context): \(.conclusion // .state // "running")"]}' +gh api graphql -f query=' + query($owner:String!,$repo:String!,$pr:Int!){ + repository(owner:$owner,name:$repo){ pullRequest(number:$pr){ + reviewThreads(first:100){ pageInfo{ hasNextPage } nodes{ isResolved } } } } }' \ + -f owner= -f repo= -F pr= \ + --jq '.data.repository.pullRequest.reviewThreads + | if .pageInfo.hasNextPage then error("more than 100 review threads โ€” paginate before trusting this count") + else [.nodes[] | select(.isResolved==false)] | length end' +``` + +``/`` are `` split on the `/`, and `-F pr=` is typed โ€” `-f` 422s an +`Int!`. Past 100 threads the jq errors rather than undercounting โ€” a count that decides +MERGE READY fails closed. The line makes four claims, so four conditions each mean **not +converged**: + +- the thread count is not 0, or `reviewDecision` is no longer `APPROVED` โ†’ take the matching + Phase 4 row; +- `mergeable` is not `MERGEABLE` (re-poll `UNKNOWN` up to 5 times, ~5s apart, as + `/pr-ready`'s preflight does) โ†’ report the conflict; +- `notGreen` is non-empty โ†’ report each check and its state; a check re-run between Phase 4 + and now is exactly what this catches. + +Only when all four pass may the line be posted. `mergeStateStatus` is then the cross-check, +not a substitute: it reads `BLOCKED` alike for a thread, a dismissed approval and a red +required check, so it cannot name a cause โ€” but once those have all been checked, a +`BLOCKED` that remains is a real, unexplained block. Report it with the raw value instead of +posting MERGE READY over it. + +**BOT STUCK / HUMAN STUCK / APPROVED ยท THREADS OPEN / not-yet-engaged:** post the +equivalent short status (round/cycle count, what's outstanding, and โ€” for the +not-yet-engaged case โ€” that `/pr-ready`'s wait already ran once and re-running +`/pr-autopilot` will check again) and say so in the session. Don't dress up an +unresolved state as converged. + +For **APPROVED ยท THREADS OPEN**, the outstanding part is a per-thread list, not a count: +each open thread's `path:line`, who opened it, and whose reply it is waiting on (the +opener, if the last comment is someone else's; otherwise the PR author). The blocker count +the STUCK forms report is 0 here by construction, so reporting it would say nothing. Either way, print the same summary in this session as well โ€” the PR comment is for the PR's audience, the session output is for whoever ran this. @@ -234,6 +289,8 @@ for the PR's audience, the session output is for whoever ran this. a human on bot-flagged work. - **`--max-human-cycles` reached** โ†’ HUMAN STUCK, Phase 5, never re-solicit past the bound in the same run. +- **Approved, and threads still open after the one `/revise-pr` pass** โ†’ APPROVED ยท + THREADS OPEN, Phase 5, each thread named with the person it waits on. - **`/pr-ready`'s config is missing** (its own Phase 0 setup) โ†’ that failure surfaces from Phase 3 verbatim; this skill doesn't have a fallback channel of its own. diff --git a/.claude/skills/pr-ready/SKILL.md b/.claude/skills/pr-ready/SKILL.md index 0f3c5917..403619dd 100644 --- a/.claude/skills/pr-ready/SKILL.md +++ b/.claude/skills/pr-ready/SKILL.md @@ -212,6 +212,17 @@ against the login string, not the object): for PR_NUM in ; do gh pr view "$PR_NUM" --repo "$REPO" \ --json state,mergeable,statusCheckRollup,commits,author,url,title -q '.' + + # unresolved review threads โ€” a merge gate since `required_conversation_resolution`, + # and invisible to every field above (see the note under the table) + gh api graphql -f query=' + query($owner:String!,$repo:String!,$pr:Int!){ + repository(owner:$owner,name:$repo){ pullRequest(number:$pr){ + reviewThreads(first:100){ pageInfo{ hasNextPage } nodes{ isResolved } } } } }' \ + -f owner="${REPO%%/*}" -f repo="${REPO##*/}" -F pr="$PR_NUM" \ + --jq '.data.repository.pullRequest.reviewThreads + | if .pageInfo.hasNextPage then error("more than 100 review threads โ€” paginate before trusting this count") + else [.nodes[] | select(.isResolved==false)] | length end' done AUTHOR=$(gh pr view --repo "$REPO" --json author -q .author.login) ``` @@ -249,6 +260,26 @@ is no ask, and the abort message is every PR's reason, not just the first. | A check is not green | any `.statusCheckRollup[]` whose `(.conclusion // .state)` is not in `SUCCESS` / `NEUTRAL` / `SKIPPED` | "PR #N has a check that isn't green: `` is `` โ€” wait for it or fix it; a red or half-finished PR trains reviewers to skip the ask." | | More than one commit | `.commits \| length > 1` | "PR #N has `` commits โ€” collapse to one before asking for review: `/collapse-pr `." | +**Unresolved threads are disclosed, never excluded.** `required_conversation_resolution: +true` makes an open thread a merge gate, but unlike a red check it is not a sign the PR is +unready for a reviewer โ€” often it is waiting *on* one. The sibling skills leave threads open +on purpose: `/pr-review` Step 6.5 leaves a suggestion block for the author's Apply click and +a question for the author's answer, and `/revise-pr` Step 6 leaves a pushback open for the +reviewer to adjudicate. Excluding on the count would withhold the ask from exactly the PRs +that need a reviewer next โ€” and `/pr-autopilot` Phase 3 passes a single PR, so one exclusion +there is an aborted ask with no Phase 4 row to catch it. + +So the count goes in the Phase 2 pre-send print next to each PR (`#N ยท 2 threads open`), the +ping is sent unchanged, and Phase 6 re-counts. What it never does is drop the PR. + +**No `gh pr view` field reports this.** `.mergeable` is only ever +`MERGEABLE` / `CONFLICTING` / `UNKNOWN` โ€” it answers "do the diffs conflict", not "does +branch protection allow the merge", and a PR with five open threads reads `MERGEABLE`. +`.mergeStateStatus` *does* go `BLOCKED`, but it conflates unresolved threads with a +missing approval, a failing required check, and a stale branch, so it cannot name the +cause and must not be reported as if it did โ€” which is why the fetch above does not +request it. Count the threads directly. + **`UNKNOWN` is not a conflict.** GitHub computes mergeability asynchronously, so a freshly-pushed PR โ€” exactly the state `/pr-ready` runs in, right after `/open-pr` โ€” commonly reads `UNKNOWN` with zero conflicts. Re-poll the @@ -698,17 +729,26 @@ Then Phase 6. Re-evaluate the ack predicate one last time and print a terminal summary: the state reached per PR, by whom and through which signal, each PR's current -`state`/`mergeable`/check status, and the author's options. Stop there. Nothing -after this phase runs automatically. +`state`/`mergeable`/check status **and its unresolved-thread count**, and the author's +options. Stop there. Nothing after this phase runs automatically. + +**Re-count the threads here; don't reuse Phase 1's number.** The whole point of the ask +was to get someone to review, and a review that arrived during the wait opens threads โ€” +so the count that mattered at preflight is exactly the one most likely to be stale by +Phase 6. A PR that reaches `REVIEWED (APPROVED)` with open threads is **approved and not +mergeable**, and reporting only the approval is the failure this sweep exists to fix: +`/pr-review` leaves open precisely the threads it decided the author owes an answer to, +so an open thread beside an approval is a worklist, not a contradiction. Say +`APPROVED ยท 2 threads open โ†’ /revise-pr `, never `APPROVED` alone. **Report per PR โ€” one row each, never one verdict for the set.** -| PR | State | Who | Signal | Since | State / checks | +| PR | State | Who | Signal | Since | State / checks / threads | |---|---|---|---|---|---| -| #606 | REVIEWED (`APPROVED`) | `` | submitted review | 15:42 | OPEN / green | -| #607 | REVIEWING | `` | ๐Ÿ‘€ on the PR | 16:10 | OPEN / green | -| #608 | HELD BUT STALE | `` | ๐Ÿ‘€ at 14:05, grace lapsed 15:05 | โ€” | OPEN / green | -| #605 | SILENT | โ€” | โ€” | โ€” | OPEN / green | +| #606 | REVIEWED (`APPROVED`) | `` | submitted review | 15:42 | OPEN / green / **2 open โ†’ `/revise-pr 606`** | +| #607 | REVIEWING | `` | ๐Ÿ‘€ on the PR | 16:10 | OPEN / green / 0 open | +| #608 | HELD BUT STALE | `` | ๐Ÿ‘€ at 14:05, grace lapsed 15:05 | โ€” | OPEN / green / 0 open | +| #605 | SILENT | โ€” | โ€” | โ€” | OPEN / green / 0 open | Above the table, on their own line, name the **ask-level** acks โ€” a ๐Ÿ‘ or a thread reply on the ping. They say someone picked up the ask, but they cannot say which diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 8fa1f80b..2b2cdd4f 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -1,6 +1,6 @@ --- name: pr-review -description: Review an offlinecv pull request adversarially, the way a maintainer does โ€” signal ๐Ÿ‘€ that review started, judge the diff against the linked issue's acceptance criteria, run the generic /code-review correctness pass, layer offlinecv's own gates, audit description accuracy, structure findings (Blocking / Secondary / Nits), auto-fix & push small items if 0 blockers exist, collapse the branch back to one commit via /collapse-pr before the review lands, emit suggestion blocks for what it does not push, and post the PR review autonomously at the end โ€” approving the PR including its own fix commit, so a clean PR needs no second round-trip. +description: Review an offlinecv pull request adversarially, the way a maintainer does โ€” signal ๐Ÿ‘€ that review started, judge the diff against the linked issue's acceptance criteria, run the generic /code-review correctness pass, layer offlinecv's own gates, audit description accuracy, structure findings (Blocking / Secondary / Nits), auto-fix & push small items if 0 blockers exist, collapse the branch back to one commit via /collapse-pr before the review lands, emit suggestion blocks for what it does not push, file a follow-up issue for every finding that outlives the run, post the PR review autonomously at the end โ€” approving the PR including its own fix commit, so a clean PR needs no second round-trip โ€” then resolve the threads it disposed of itself, so no finding is lost to a merge and the open threads left blocking are only the ones the author owes an answer to. argument-hint: <#|#N> [--repo owner/repo] [--local] [--effort low|medium|high] [--no-commit] --- @@ -12,12 +12,18 @@ the thread": check out the diff โ†’ **signal that review started** โ†’ read the correctness pass โ†’ **layer the offlinecv-specific gates** โ†’ **then** read the PR description and audit it against what the code actually does โ†’ structure findings **Blocking / Secondary / Nits** โ†’ **if 0 blockers & small fixes exist**: apply fixes, -verify gates, commit, collapse the branch back to one commit, push โ†’ **post the `gh` PR -review automatically at the end** (verdict `APPROVE` if 0 blockers โ€” including when the run -pushed the fixes itself, so the PR leaves the run mergeable โ€” `REQUEST_CHANGES` if โ‰ฅ1 -blocker). Findings it does *not* push land as ```` ```suggestion ```` blocks the author can +verify gates, commit, collapse the branch back to one commit, push โ†’ **file a follow-up +issue for every finding that outlives the run** โ†’ **post the `gh` PR review automatically +at the end** (verdict `APPROVE` if 0 blockers โ€” including when the run pushed the fixes +itself โ€” `REQUEST_CHANGES` if โ‰ฅ1 blocker) โ†’ **reply to and resolve the threads it disposed +of itself**. Findings it does *not* push land as ```` ```suggestion ```` blocks the author can apply in one click. +`main` carries `required_conversation_resolution: true`, so an `APPROVE` is no longer the +whole merge gate: a thread left open blocks too. The run therefore ends with an explicit +ledger โ€” what it fixed, what it filed, and what it is leaving open for the author โ€” rather +than with a claim that the PR is mergeable. + This is the **reviewer-side** sibling of the author-side loop: `open-pr` creates the PR, `revise-pr` addresses the review โ€” this skill *is* the review in between. All three delegate the one-commit collapse to `/collapse-pr`. @@ -149,7 +155,11 @@ in the report and review against the diff alone; don't invent a spec. ### Step 0.6 โ€” Signal that the review has started -Post a ๐Ÿ‘€ reaction on the PR. Do this **now**, before the slow work, not at the +**Skip this step under `--local`.** A preview posts nothing, and a preview's ๐Ÿ‘€ is worse +than nothing: `pr-ready` reads it as a live `REVIEWING` hold and grants a grace window for a +review that will never post. + +Otherwise, post a ๐Ÿ‘€ reaction on the PR. Do this **now**, before the slow work, not at the end โ€” its entire value is telling everyone else that this PR is being read *while* you read it: @@ -188,9 +198,10 @@ claim degrades to naming a person to nudge, which beats a reaction that is worthless because it landed after the review was already done. **Honest limit, so nobody over-trusts it:** ๐Ÿ‘€ does not *block* anything. `main` -requires one approving review, and any approver can merge over a ๐Ÿ‘€. The only -merge-blocking signal GitHub offers is `REQUEST_CHANGES`, which is far too heavy -to mean "wait for me." This is a social hold that `pr-ready` surfaces by name, not +requires one approving review, and any approver can merge over a ๐Ÿ‘€. GitHub's +merge-blocking signals โ€” `REQUEST_CHANGES`, and under `required_conversation_resolution` +an unresolved thread โ€” both say "something must change", which is far too heavy to mean +"wait for me." This is a social hold that `pr-ready` surfaces by name, not enforcement. ### Step 1 โ€” Get the diff onto disk @@ -413,12 +424,24 @@ out, because the interaction with branch protection is otherwise invisible to a the push, so there is no prior approval to dismiss. This holds for the collapse's force-push too โ€” same slot, same ordering, and it is precisely why Step 5.5 collapses before the review goes up rather than after. +- **`required_conversation_resolution: true`, since #924/#926.** An `APPROVE` no longer + makes a PR mergeable on its own: every inline thread this review opens is a merge gate + until someone resolves it. So the approval and the threads now say two different things, + and both are load-bearing โ€” `APPROVE` still means *0 Blocking findings* and nothing here + softens that, but a finding left in an open thread stops the merge whether or not it was + Blocking. That is the point (#924 and #926 each merged with every Secondary thread open, + unanswered, and unrecorded โ€” the defects are now #930 and #931), and it is also a way to + jam a clean PR with noise. Step 5.7 and Step 6.5 are what keep the open set honest: + every thread the *reviewer* can close, the reviewer closes, so what stays open is + exactly what the author has to answer. That is the intended outcome, not a loophole being exploited. The alternative โ€” post the nits, wait for the author to apply them, watch the new push dismiss the approval, review again โ€” is the round-trip this skill exists to remove. One commit carries every -non-blocking finding, and the PR leaves the run approved and mergeable, with no follow-up -issues filed for nits that are already fixed. +non-blocking finding it could safely carry, and the PR leaves the run approved, with those +threads replied to and resolved by Step 6.5 and no follow-up issue filed for a nit that is +already fixed. Whether it also leaves *mergeable* depends on what Step 6.5 could not +close โ€” report that, don't assume it. **What keeps that safe is the bound on Step 5.5, so hold the bound.** The head commit lands unread by a second party, so the auto-fix is confined to changes that cannot alter @@ -430,8 +453,9 @@ and still `APPROVE`. ### Step 5.5 โ€” Auto-fix small items, then collapse (0 Blockers) -If 0 Blocking findings exist AND small fixes (Secondary or Nits) exist AND `--no-commit` -is NOT set: +If 0 Blocking findings exist AND small fixes (Secondary or Nits) exist AND neither +`--no-commit` nor `--local` is set (`--local` is a preview โ€” it commits, pushes and collapses +nothing; its fixes are printed as findings): **The order is the whole of this step**, and it is not rearrangeable: @@ -719,6 +743,76 @@ nobody reviewed. Rename a variable, tighten a comment, hoist a constant, drop a export โ€” yes. Change a condition, reorder an await, adjust a regex โ€” no; that is a finding written in prose, with the failing input spelled out, for the author to decide on. +### Step 5.7 โ€” File a follow-up issue for every finding that outlives the run + +**Never in `--local` mode.** Filing is a public write, and `--local` promises to stop without +posting to GitHub. Under `--local`, print each issue this step *would* file โ€” title, labels, +and the drafted body โ€” alongside the draft review, and file nothing. + +A finding this run neither fixed nor expects the author to act on before merge has, at +this point, exactly one home: a comment on a PR that is about to be merged and never read +again. **#924 and #926 are the proof** โ€” both merged with 0 Blockers, both left every +Secondary thread open and unanswered, and the two real defects in them (a currency-first +negative still losing its minus sign; a 19th hardcoded copy of the open-ended date +vocabulary, in the function that decides `is_current`) survived into `main` unrecorded +until they were reconstructed by hand, hours later, as #930 and #931. Neither was Blocking. +Both were right. + +**So the reviewer files them, at review time, not the author.** The contributor is not the +one who found it, the PR is not the place to track it, and "the author will open an issue" +is the assumption that failed twice. + +**File when *not* filing loses information.** That bar is the whole gate โ€” it is not "file +every Secondary": + +| Finding | Filed? | +|---|---| +| A defect that survives the merge with nobody acting on it | **yes** โ€” this is the #930/#931 class | +| Out of this PR's scope, but real and locatable (a sibling copy, an adjacent file) | **yes** | +| Already fixed by Step 5.5 | no โ€” it is in the head commit | +| Emitted as a Step 5.6 suggestion block | no โ€” one click and it is gone | +| A trade-off the code documents and you agree with | no โ€” reply saying so, and resolve | +| Blocking | no โ€” that is `REQUEST_CHANGES`, and an issue would launder it into a nit | + +**On a `REQUEST_CHANGES` verdict, file only the out-of-scope row.** The author is coming back +to revise, so an in-scope Secondary is not a defect nobody acts on โ€” it is one they are about +to act on, in the round where fixing it costs nothing. Leave it as a thread for that round. +Filing it would mint a backlog issue and let Step 6.5 resolve the very thread the author +needed to see. + +**Write it as a standalone issue, not as a pointer to the review.** The reader arrives from +the backlog with no memory of this PR: restate the defect, the failing input and the wrong +output, where it lives (the re-verify table below says which ref, and whether a line number +survives), why it matters, a proposed fix, and acceptance criteria. Link the **PR** as +provenance, never as the body โ€” not a review comment, because this step runs before Step 6 +posts one and no comment URL exists yet. The link runs the other way too: Step 6.5's reply +on the thread names the issue. One issue per finding. + +```bash +# from the repo root; body as a file so tables/backticks/fences survive +scripts/create-gh-issue.sh \ + --title "" \ + --body-file /tmp/pr-review-followup-.md \ + --labels bug # or refactor/improvement โ€” must already exist +# โ†’ prints "#\t" +``` + +No milestone and no assignee: new issues land in the backlog and get scheduled separately. + +**Re-verify each one before filing, on the ref where the defect actually lives** โ€” never +against your own review prose. A finding written at review time can already be stale by +the time the run reaches this step, and a follow-up issue that does not reproduce is +worse than the comment it replaced, because it outlives it. Which ref depends on the row: + +| Finding | Verify on | Cite | +|---|---|---| +| In code this PR adds (#930's shape) | the **PR branch** โ€” that code is not on `origin/main` yet, so verifying there reproduces nothing and files nothing | the path, **no line number** โ€” the squash moves it | +| Out of scope, already on `main` (#931's shape โ€” a sibling copy, an adjacent file) | **`origin/main`** | `file:line` on `main` | + +Carry the numbers into Step 6: each filed issue is named in the finding it came from (both +in the body and in its inline thread), so the author sees the disposition rather than an +open-ended complaint. Step 6.5 then resolves those threads. + ### Step 6 โ€” Draft & post (Autonomous) Assemble the review body (Markdown: a one-line stance, then `## Blocking` / @@ -825,9 +919,13 @@ cat > /tmp/review.json <<'JSON' ] } JSON -gh api "repos/$REPO/pulls/$PR_NUM/reviews" --method POST --input /tmp/review.json +REVIEW_ID="$(gh api "repos/$REPO/pulls/$PR_NUM/reviews" --method POST --input /tmp/review.json --jq .id)" ``` +Keep `REVIEW_ID`: Step 6.5 uses it to tell this review's threads from everyone else's. +**Every** POST in this step assigns it this way โ€” the 422 retry below included โ€” so it +always names the review that actually landed, never an empty or earlier value. + For a finding spanning a range, add `"start_line"` (with `"start_side": "RIGHT"`). **Don't fight a `422`.** A bad anchor rejects the *whole* review, and the usual cause @@ -837,7 +935,7 @@ twenty-minute anchoring exercise. Instead, **once**: 1. Re-run the `files` call. If the head SHA changed, the diff moved โ€” say so, and re-anchor against the new patch (the findings themselves usually still hold; the - *lines* moved). + *lines* moved), then re-post with the same `REVIEW_ID="$(gh api โ€ฆ --jq .id)"` assignment. 2. If it 422s again, **fall back to a body-only review** with the findings as `path:line` references and post it. A posted body-only review beats a perfect inline review that never lands. @@ -851,6 +949,84 @@ gh pr view "$PR_NUM" --repo "$REPO" --json headRefOid -q .headRefOid In `--local` mode, print the review (body + the inline comments with their anchors) and stop. +### Step 6.5 โ€” Close the threads you dispositioned yourself + +`main` has `required_conversation_resolution: true`, so every thread this review just +opened holds the merge until somebody resolves it. Most of them are not the author's to +answer โ€” they are findings the *reviewer* already disposed of, by filing them in Step 5.7 +or by agreeing with a documented trade-off โ€” and leaving those open turns a merge gate into noise, which +is how a gate stops being read. + +**This step runs after Step 6, not before it: thread IDs do not exist until the comments +are posted.** It never runs in `--local` mode, and it never runs on a review that posted +body-only (there are no threads). + +Resolve a thread when **you** closed the loop, and only then: + +| Thread | Reviewer action | +|---|---| +| Filed as a follow-up issue (5.7) | reply naming the issue โ†’ **resolve** | +| A trade-off you examined and agree with โ€” no action wanted | reply saying so โ†’ **resolve** | +| A Step 5.6 suggestion block | **leave open** โ€” the author's Apply commits the change but does **not** resolve the thread (GitHub marks it outdated, nothing more); Step 7 says so, so they resolve it by hand or run `/revise-pr`, which does | +| Asks the author a question, or needs their judgement | **leave open** โ€” this is the exit they owe | +| Any Blocking finding | **leave open** โ€” the verdict is `REQUEST_CHANGES` anyway | + +There is no *fixed by Step 5.5* row, and that is not an omission: a fix that landed never +opens a thread (Step 5.5 โ€” *a fix that landed is not a finding*); it is listed in the body's +`## Fixed in ` section instead. + +What remains open is then a real worklist, and the merge gate means what it says. + +**A thread this review did not open is never yours to resolve**, whatever Step 5.5 happened +to fix. A human's earlier Secondary that your auto-fix also cured gets a reply naming the SHA +and stays open for them. The listing below enforces +that rather than trusting the match: it keeps only threads whose first comment belongs to the +review Step 6 just posted (`REVIEW_ID`), so an earlier reviewer's thread โ€” or an earlier round +of this same skill โ€” never reaches the resolve call. + +```bash +# thread ids + the first comment, THIS review's threads only, so you can match them to findings. +# REVIEW_ID is the integer Step 6's POST returned; spliced into the jq, never user text. +# An empty one would splice to `== )` โ€” a jq parse error that lists, replies to and resolves +# nothing, leaving every thread this review opened silently holding the merge. Refuse instead. +[ -n "$REVIEW_ID" ] || { echo "REVIEW_ID is empty โ€” re-read it from the posted review before Step 6.5" >&2; exit 1; } +gh api graphql -f query=' + query($owner:String!,$repo:String!,$pr:Int!){ + repository(owner:$owner,name:$repo){ + pullRequest(number:$pr){ + reviewThreads(first:100){ pageInfo{ hasNextPage } nodes{ + id isResolved + comments(first:1){ nodes{ databaseId path line body + pullRequestReview{ databaseId } } } } } } } }' \ + -f owner="${REPO%%/*}" -f repo="${REPO##*/}" -F pr="$PR_NUM" \ + --jq '.data.repository.pullRequest.reviewThreads + | if .pageInfo.hasNextPage then error("more than 100 review threads โ€” paginate first") else .nodes[] end + | select(.isResolved==false) + | select(.comments.nodes[0].pullRequestReview.databaseId == '"$REVIEW_ID"') + | .comments.nodes[0] as $c + | "\(.id)\t\($c.databaseId)\t\($c.path):\($c.line // "outdated")"' +# โ†’ THREAD_ID COMMENT_ID path:line โ€” the two ids the next two commands take + +# reply into one thread (REST, by the FIRST comment's databaseId โ€” not the thread id) +gh api -X POST "repos/$REPO/pulls/$PR_NUM/comments/$COMMENT_ID/replies" -f body="$REPLY" + +# then resolve it (GraphQL only โ€” REST cannot resolve a thread) +gh api graphql -f query=' + mutation($id:ID!){ resolveReviewThread(input:{threadId:$id}){ + thread{ isResolved } } }' -f id="$THREAD_ID" +``` + +**Always reply before resolving.** A silently resolved thread reads as the finding being +withdrawn; the reply is what carries the disposition โ€” the fix SHA, the issue number, or +the reason no action is wanted. That reply is the whole point of the gate, and resolving +without one reproduces the failure this step exists to prevent, one level down. + +**Never resolve a thread to tidy the merge path.** If a finding is genuinely the author's +to answer, an open thread blocking the merge is the correct state, and an approving review +alongside it is not a contradiction โ€” it says *nothing here blocks on correctness, and this +still needs an answer*. Say that in the Step 7 report rather than resolving your way out of +it. + ### Step 7 โ€” Report Print: the verdict + the rule that produced it, the finding counts @@ -871,6 +1047,18 @@ semantic verdict is ` โ€” a reader (or a calling skill) scanning only for "REQUEST_CHANGES"/"APPROVE" in the printed report must not have to infer this from the GitHub review state, which no longer carries it. +Report **the thread ledger in one line**: how many threads the review opened, how many +Step 6.5 resolved and under which disposition (filed as `#N` / no action wanted โ€” Step 5.5's +fixes open no threads, so they are counted under `## Fixed in `, not here), and how many are **left open for the author** โ€” that last number is the merge gate +under `required_conversation_resolution`, so an `APPROVE` reported without it reads as +"mergeable" when it is not. Split that number: suggestion blocks are **not** closed by the +author's Apply click (GitHub marks them outdated, never resolved), so say how many of the +open threads are suggestions the author still has to resolve by hand after applying โ€” four +greyed-out threads with the merge still blocked is otherwise unexplained. Name every follow-up issue Step 5.7 filed, with its number and +title; a filed issue that only exists in a PR comment is the failure mode this whole path +exists to remove. If the run filed none, say why in three words (all fixed / all suggested +/ nothing survived). + Also report **the Step 5.5 push outcome in one line**, because it is the only part of the run that rewrote someone's branch: whether the branch was collapsed or left multi-commit and **which reason** (author class, lease lost, a `/collapse-pr` gate, `--no-commit`, โ‰ฅ1 @@ -932,11 +1120,15 @@ recoverable. Carry its classification across too (*already upstream* / *merely b - **One `422` is information, not a puzzle.** It almost always means the author pushed mid-review. Re-anchor against the fresh patch once; if that fails, post body-only and move on. Never loop on anchoring. -- **Autonomous execution; three unattended writes, in this order.** The ๐Ÿ‘€ reaction - (Step 0.6), the auto-fix commit + push (Step 5.5 โ€” a force-push when it collapses), and - the review post (Step 6). Nothing is confirmed with the user โ€” `--local` is the only - preview. The reaction is safe *early* because it carries no prose; the other two happen - only after the findings exist, and the push is bounded by the next rules. +- **Autonomous execution; five unattended writes, in this order.** The ๐Ÿ‘€ reaction + (Step 0.6), the auto-fix commit + push (Step 5.5 โ€” a force-push when it collapses), the + follow-up issues (Step 5.7), the review post (Step 6), and the thread replies + resolves + (Step 6.5). Nothing is confirmed with the user โ€” `--local` is the only preview, and it + suppresses all five: no reaction (Step 0.6 โ€” a preview's ๐Ÿ‘€ reads to `pr-ready` as a live + hold), no commit or push (Step 5.5's gate tests it), no issues (5.7 prints what it would + file), no review post, no replies or resolves. Outside `--local`, the reaction is safe + *early* because it carries no prose; everything after it happens only once the findings exist, and each is + bounded by the rules below. - **Auto-fix small items, then approve โ€” including your own commit.** If 0 Blockers exist and small fixes remain, apply them, verify (`npm run verify`), commit with a clean message (no trailers), push to the head branch, and post `APPROVE`. That the approval @@ -972,6 +1164,24 @@ recoverable. Carry its classification across too (*already upstream* / *merely b agent-authored in-repo branch. A named contributor's branch or a fork gets the plain push at most โ€” a force-push there destroys their local work and republishes it under our name. `/collapse-pr`'s ownership gate has no override flag, by design. +- **A finding that outlives the run gets an issue, filed by the reviewer (Step 5.7).** + The bar is *not filing loses information*: a defect that survives the merge with nobody + acting on it, or a real finding outside this PR's scope. Not for anything Step 5.5 fixed, + anything Step 5.6 suggested, a documented trade-off, or anything Blocking โ€” that last one + is `REQUEST_CHANGES`, and an issue would launder it into a nit. Write it standalone + and re-verify it where the defect lives โ€” the PR branch for code this PR adds (cite the + path; the squash moves the line), `origin/main` for an out-of-scope copy (cite `file:line`). + On `REQUEST_CHANGES`, file only the out-of-scope ones; the author is about to revise. + #930 and #931 are what this rule costs when it is missing. +- **Reply, then resolve โ€” and only what you disposed of yourself (Step 6.5).** + `required_conversation_resolution: true` makes every open thread a merge gate. Close the + ones you closed the loop on (filed as `#N`, trade-off you agree with), + always with a reply first โ€” a silently resolved thread reads as a withdrawn finding. + Leave open what the author owes an answer to, and leave suggestion blocks open โ€” Apply + does not resolve them, so Step 7 tells the author to. Resolve only threads *this review* + opened (the `REVIEW_ID` filter); another reviewer's thread gets a reply, never a resolve. + Never resolve a thread to clear the merge path; an `APPROVE` beside an open thread is a + coherent state and Step 7 reports it as one. - **When you don't push, suggest (Step 5.6).** Every Secondary/Nit that is a localized textual replacement of a `+` line becomes a ```` ```suggestion ```` block, so the author applies it in one click and owns the commit. Behavioural findings never do โ€” same bound diff --git a/.claude/skills/revise-pr/SKILL.md b/.claude/skills/revise-pr/SKILL.md index 0dcb03d8..358b64f4 100644 --- a/.claude/skills/revise-pr/SKILL.md +++ b/.claude/skills/revise-pr/SKILL.md @@ -141,14 +141,15 @@ gh api graphql -f query=' query($owner:String!,$name:String!,$pr:Int!){ repository(owner:$owner,name:$name){ pullRequest(number:$pr){ - reviewThreads(first:100){ nodes{ + reviewThreads(first:100){ pageInfo{ hasNextPage } nodes{ id isResolved isOutdated path line comments(first:50){ nodes{ databaseId author{login} body } } }} } } }' -f owner="$OWNER" -f name="$NAME" -F pr="$PR_NUM" \ - --jq '.data.repository.pullRequest.reviewThreads.nodes[] + --jq '.data.repository.pullRequest.reviewThreads + | if .pageInfo.hasNextPage then error("more than 100 review threads โ€” paginate before trusting this list") else .nodes[] end | select(.isResolved==false) | {threadId:.id, replyTo:.comments.nodes[0].databaseId, path, line, isOutdated, @@ -255,10 +256,12 @@ skill exists* has the full rationale and is the only copy of it. split is the point of this step: `/collapse-pr` cannot know whether this is the last round, and this skill does. So **do not collapse on every round.** Gate it: -- **Leaving any thread open *on the target*** (you pushed back, or deferred to a - follow-up issue) โ†’ the reviewer is coming back for another round. **Keep the - fixup commit separate.** They need to diff *just your delta*, not re-read the - whole change. +- **Leaving any thread open *on the target*** (you pushed back and the call is the + reviewer's) โ†’ the reviewer is coming back for another round. **Keep the fixup commit + separate.** They need to diff *just your delta*, not re-read the whole change. Note that + such a thread also holds the merge now (`required_conversation_resolution`), so the + round is not optional โ€” which is a reason to be sure the pushback is real, and not a + deferral you skipped filing the issue for. - **Every unresolved thread on the target is now addressed** and you're re-requesting a clean approval โ†’ **collapse.** This is the last round; the branch's single commit is what lands in `main`. @@ -342,7 +345,9 @@ reader is looking at a different PR than the one that changed: | Item | Reply on | Resolve? | |---|---|---| | Target's own thread, fixed | target thread | **yes** | -| Target's own thread, deferred / pushed back | target thread | no โ€” leave for the reviewer | +| Target's own thread, **deferred to a filed issue, and the thread's author proposed or agreed to the deferral** | target thread, naming the issue and quoting the agreement | **yes** โ€” the tracking moved by agreement; see below | +| Target's own thread, deferred to a filed issue on **your** judgement alone | target thread, naming the issue | no โ€” that is a pushback with an issue attached; leave for the reviewer | +| Target's own thread, pushed back (you disagree, it is the reviewer's call) | target thread | no โ€” leave for the reviewer | | Imported, `reproduces`, fixed in target | source thread | **only** if the source PR's own copy of the defect is also gone; otherwise no | | Imported, `does not apply` | source thread, with the reason | **no** | | Source PR is merged or closed | source thread (replies still work) | **never** | @@ -369,9 +374,24 @@ mutation($id:ID!){ resolveReviewThread(input:{threadId:$id}){ thread{ isResolved - Reply to **every** unresolved thread โ€” addressed, deferred, or non-reproducing. Silence isn't an option (PR-author signal norm), and that includes a reviewer on another PR whose finding you imported and then didn't act on. -- Resolve threads you fixed or that are outdated. **Don't** resolve a thread where - you pushed back or deferred โ€” leave it open for the reviewer to close, with your - rationale visible. +- Resolve threads you fixed, deferred **by agreement** to a filed issue, or that are + outdated. **Don't** resolve a thread where you **pushed back** โ€” leave that one open for + the reviewer to close, with your rationale visible. +- **Deferral and pushback split here, and `required_conversation_resolution: true` on + `main` is why.** An open thread now blocks the merge outright, so the two cases have + opposite consequences. A pushback *should* block: you are asking the reviewer to decide, + and the PR waiting on them is the correct state. A deferral should not: the work has + moved to an issue by agreement, nobody is going to do it on this branch, and leaving the + thread open blocks the PR on a task that has, by construction, left it. **File the issue + first, name it in the reply, then resolve** โ€” "Deferred to #N because โ€ฆ" with no `#N` in + it is a pushback wearing a deferral's words, and it merges nothing. +- **The agreement has to be on the thread, not in your head.** It exists when the reviewer + asked for a follow-up ("worth an issue", "out of scope here"), when the reviewer filed + the issue themselves (`/pr-review` Step 5.7 โ€” those arrive already resolved), or when they + replied agreeing. Otherwise filing an issue is a disposition *you* chose: name it in the + reply and leave the thread open, exactly like a pushback. Deciding unilaterally that your + own deferral clears the reviewer's merge gate is the move `/pr-review` Step 6.5 forbids the + reviewer โ€” *never resolve a thread to tidy the merge path* โ€” and it binds both sides. - The reply must match what you did: "Fixed in ``" only if the code changed; "Deferred to #N because โ€ฆ" otherwise. No claiming a fix you didn't make. For an imported fix, the sha lives in a different PR โ€” name that PR in the reply, or