chore(skills): teach the PR skills that an open thread blocks the merge - #932
Open
s-annam wants to merge 1 commit into
Open
chore(skills): teach the PR skills that an open thread blocks the merge#932s-annam wants to merge 1 commit into
s-annam wants to merge 1 commit into
Conversation
`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 weeks 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. - **Step 6.5** replies to and resolves the threads the reviewer itself disposed of (fixed in `<sha>`, filed as `#N`, trade-off it agrees with), and leaves open exactly what the author owes an answer to. It 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. 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 excludes the PR from the ask, and Phase 6 re-counts rather than reusing the preflight number, since a review arriving during the wait is what opens them. - **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`. revise-pr's "don't resolve a deferral" rule inverted under the new gate: the work has moved to an issue by agreement, so leaving the thread open blocks the PR on a task that has left it. Deferral-with-a-filed-issue now resolves; pushback still does not, because asking the reviewer to decide *should* block. 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. `mergeStateStatus` is documented throughout as corroboration 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. Both new GraphQL snippets were run verbatim against #906 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.
Deploying offlinecv with
|
| Latest commit: |
1c98cd1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c2df855b.offlinecv.pages.dev |
| Branch Preview URL: | https://chore-pr-skills-conversation.offlinecv.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mainnow carriesrequired_conversation_resolution: true(set today, after #924 and #926 both merged with every Secondary thread open and unanswered — the two real defects in them had to be reconstructed by hand as #930 and #931). All five PR skills predate that setting and treat an approving review as the whole merge gate. This updates them.pr-reviewgains Step 5.7 (file a follow-up issue for any finding that outlives the run, reviewer-side, before the review posts so the body can cite the number) and Step 6.5 (reply to and resolve the threads the reviewer itself disposed of, leaving open exactly what the author owes an answer to). The other four are corrected where they would now report a blocked PR as ready.No issue — per the standing convention that skill changes skip the issue overhead.
Review focus
.claude/skills/pr-review/SKILL.mdStep 5.7 — is the not filing loses information bar drawn tightly enough that a normal review files zero or one issue rather than one per Secondary? The exclusions (fixed in 5.5, emitted as a 5.6 suggestion, a documented trade-off, anything Blocking) are meant to carry most of that weight..claude/skills/pr-review/SKILL.mdStep 6.5 — the reviewer resolving its own threads is new authority. Does the disposition table leave anything resolvable that a human should have seen first? The intended invariant is that a resolved thread always carries a reply naming a SHA, an issue, or a reason..claude/skills/pr-autopilot/SKILL.mdPhase 4, approved-with-open-threads row — it branches on whether/revise-prpushed, becausedismiss_stale_reviews: truedrops the approval when it did. Does the once-per-human-cycle bound actually terminate, or can a reviewer opening a new thread each round keep it alive?.claude/skills/revise-pr/SKILL.mdStep 6 — deferral now resolves where it used to stay open. Is "file the issue first, name it in the reply, then resolve" enough to stop a pushback being relabelled as a deferral to clear the merge path?Test plan
npm run verifygreen (pre-push hook; typecheck, lint, gates, full suite, build, fallow —✓ No issues in 5 changed files)4, matching the PR's actual unresolved countpulls/<n>/comments/<id>/replies) andresolveReviewThreadmutation exercised live on the two real threads on fix(rewrite): keep the minus sign on a line-initial negative number (#821) #924 and refactor(heuristics): one date-token lexicon in regex.ts (#916) #926 — each now carries a reply naming fix(rewrite): a currency-first negative still loses its minus sign in cleanRewriteLine #930 / refactor(heuristics): parseDateRange still hardcodes the open-ended date vocabulary (19th copy, #916 missed it) #931 and is resolvedmergeStateStatusbehaviour confirmed rather than assumed: feat(letters): job → company → standard resolution, picker, and customize-from #906 readsmergeable: MERGEABLEwithmergeStateStatus: BLOCKEDand 4 open threads, which is the bugpr-ready's preflight had.claude/skills/— no fixture binaries (Step 3.5 n/a), nosrc/change