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