diff --git a/.github/labels.yml b/.github/labels.yml index 57beaf8c716..0a534d1dfcc 100644 --- a/.github/labels.yml +++ b/.github/labels.yml @@ -10,13 +10,13 @@ # Lifecycle and hold labels share blue; release-impact labels remain green. - name: pr:needs-reviewer color: 1D76DB - description: Ready for review but no reviewer has been requested + description: No pending review request or active submitted human review - name: pr:waiting-for-review color: 1D76DB - description: A requested reviewer owns the next action + description: A reviewer owns the next action; required approval is pending or unverifiable - name: pr:waiting-for-author color: 1D76DB - description: The author needs to address review feedback + description: The author needs to address human review feedback or human-endorsed inline feedback - name: pr:review-approved color: 1D76DB description: Required reviews are approved; checks may still be pending diff --git a/.github/scripts/pr-status-labels.mjs b/.github/scripts/pr-status-labels.mjs index cfabe36c3e3..890d68956ac 100644 --- a/.github/scripts/pr-status-labels.mjs +++ b/.github/scripts/pr-status-labels.mjs @@ -30,6 +30,12 @@ const MANUAL_LABELS = Object.freeze({ const MANAGED_LABELS = Object.freeze(Object.values(STATUS_LABELS)); +const SUBMITTED_REVIEW_STATES = Object.freeze([ + "COMMENTED", + "APPROVED", + "CHANGES_REQUESTED" +]); + // GitHub's merge-queue GraphQL fields require this feature header. const MERGE_QUEUE_HEADERS = Object.freeze({ "GraphQL-Features": "merge_queue" @@ -132,6 +138,7 @@ const REVIEW_THREAD_FRAGMENT = ` comments(first: 100, after: $commentsCursor) { nodes { state + createdAt author { __typename login @@ -425,7 +432,44 @@ async function inferredReviewDecision(github, core, owner, repo, number, pull) { return "APPROVED"; } -async function hasUnresolvedReviewFeedback(github, owner, repo, number, pull) { +function humanReviewEvidence(pull, reviews) { + const humanReviews = reviews.filter( + (review) => + review.user?.type === "User" && + review.user.login && + review.user.login.toLowerCase() !== pull.author?.login?.toLowerCase() && + Number.isFinite(Date.parse(review.submitted_at)) + ); + const decisive = latestDecisiveReviews(humanReviews); + const approvals = decisive.filter( + (review) => + review.state === "APPROVED" && + review.commit_id === pull.headRefOid && + typeof review.body === "string" && + review.body.trim().length > 0 + ); + return { + hasReviewer: humanReviews.some((review) => + SUBMITTED_REVIEW_STATES.includes(review.state) + ), + changeRequests: decisive.filter( + (review) => review.state === "CHANGES_REQUESTED" + ), + approvedFeedbackAt: approvals.reduce( + (latest, review) => Math.max(latest, Date.parse(review.submitted_at)), + -Infinity + ) + }; +} + +async function hasUnresolvedReviewFeedback( + github, + owner, + repo, + number, + pull, + approvedFeedbackAt +) { let cursor = null; do { const result = await github.graphql(REVIEW_THREADS_QUERY, { @@ -461,20 +505,34 @@ async function hasUnresolvedReviewFeedback(github, owner, repo, number, pull) { throw new Error(`Invalid review-comment response for #${number}`); } if ( - comments.nodes.some( - (comment) => - comment?.state === "SUBMITTED" && - comment.author?.__typename === "User" && - comment.author.login && - comment.author.login.toLowerCase() !== - pull.author?.login?.toLowerCase() && - ["COMMENTED", "APPROVED", "CHANGES_REQUESTED"].includes( + comments.nodes.some((comment) => { + const submittedAt = Date.parse( + comment?.pullRequestReview?.submittedAt + ); + if ( + comment?.state !== "SUBMITTED" || + !comment.author?.login || + comment.author.login.toLowerCase() === + pull.author?.login?.toLowerCase() || + !SUBMITTED_REVIEW_STATES.includes( comment.pullRequestReview?.state - ) && - Number.isFinite( - Date.parse(comment.pullRequestReview?.submittedAt) - ) - ) + ) || + !Number.isFinite(submittedAt) + ) { + return false; + } + if (comment.author.__typename === "User") { + return true; + } + // A nonempty approval summary conservatively endorses existing feedback, not later bot comments. + const createdAt = Date.parse(comment.createdAt); + return ( + comment.author.__typename === "Bot" && + Number.isFinite(createdAt) && + createdAt <= approvedFeedbackAt && + submittedAt <= approvedFeedbackAt + ); + }) ) { return true; } @@ -515,7 +573,9 @@ function desiredLabels( rereviewRequested = false, requiredChecksPassed = false, reviewDecision = pull.reviewDecision, - unresolvedReviewFeedback = false + unresolvedReviewFeedback = false, + hasSubmittedHumanReview = false, + hasOutstandingHumanChanges = false ) { const existing = currentLabels(pull); const desired = new Set(); @@ -556,16 +616,19 @@ function desiredLabels( unresolvedReviewFeedback ) { handoff = STATUS_LABELS.waitingForAuthor; - } else if (reviewDecision === "APPROVED") { - handoff = STATUS_LABELS.reviewApproved; - } else if (reviewDecision === "CHANGES_REQUESTED") { + } else if ( + hasOutstandingHumanChanges || + reviewDecision === "CHANGES_REQUESTED" + ) { handoff = rereviewRequested ? STATUS_LABELS.waitingForReview : STATUS_LABELS.waitingForAuthor; + } else if (reviewDecision === "APPROVED") { + handoff = STATUS_LABELS.reviewApproved; } else { handoff = - pull.reviewRequests.totalCount > 0 ? + pull.reviewRequests.totalCount > 0 || hasSubmittedHumanReview ? STATUS_LABELS.waitingForReview : STATUS_LABELS.needsReviewer; } @@ -594,14 +657,10 @@ function reviewerKey(reviewer) { return null; } -function hasRereviewRequest(pull, reviews, timeline) { - if (pull.reviewRequests.totalCount !== pull.reviewRequests.nodes.length) { - throw new Error("Cannot classify more than 100 pending review requests"); - } - +function latestDecisiveReviews(reviews) { const decisive = new Map(); const decisions = reviews.filter((review) => - ["CHANGES_REQUESTED", "APPROVED"].includes(review.state) + ["CHANGES_REQUESTED", "APPROVED", "DISMISSED"].includes(review.state) ); for (const review of decisions) { if ( @@ -621,9 +680,14 @@ function hasRereviewRequest(pull, reviews, timeline) { for (const review of decisions) { decisive.set(review.user.login.toLowerCase(), review); } - const changes = [...decisive.values()].filter( - (review) => review.state === "CHANGES_REQUESTED" - ); + return [...decisive.values()]; +} + +function hasRereviewRequest(pull, changes, timeline) { + if (pull.reviewRequests.totalCount !== pull.reviewRequests.nodes.length) { + throw new Error("Cannot classify more than 100 pending review requests"); + } + if (changes.length === 0) { return false; } @@ -841,6 +905,8 @@ async function syncPull(github, core, owner, repo, number) { core.info(`Removed held pull request #${number} from the merge queue`); } + let changeRequests = []; + let hasSubmittedHumanReview = false; let unresolvedReviewFeedback = false; if ( pull.state === "OPEN" && @@ -848,12 +914,25 @@ async function syncPull(github, core, owner, repo, number) { !existing.has(MANUAL_LABELS.doNotMerge) && !existing.has(MANUAL_LABELS.needsAuthorResponse) ) { + const submittedReviews = await github.paginate( + github.rest.pulls.listReviews, + { + owner, + repo, + pull_number: number, + per_page: 100 + } + ); + const evidence = humanReviewEvidence(pull, submittedReviews); + hasSubmittedHumanReview = evidence.hasReviewer; + changeRequests = evidence.changeRequests; unresolvedReviewFeedback = await hasUnresolvedReviewFeedback( github, owner, repo, number, - pull + pull, + evidence.approvedFeedbackAt ); } @@ -872,26 +951,24 @@ async function syncPull(github, core, owner, repo, number) { let rereviewRequested = false; if ( pull.state === "OPEN" && + !pull.isDraft && !unresolvedReviewFeedback && - reviewDecision === "CHANGES_REQUESTED" && - pull.reviewRequests.totalCount > 0 + (changeRequests.length > 0 || reviewDecision === "CHANGES_REQUESTED") && + pull.reviewRequests.totalCount > 0 && + !existing.has(MANUAL_LABELS.doNotMerge) && + !existing.has(MANUAL_LABELS.needsAuthorResponse) ) { - const [reviews, timeline] = await Promise.all([ - github.paginate(github.rest.pulls.listReviews, { - owner, - repo, - pull_number: number, - per_page: 100 - }), - github.paginate(github.rest.issues.listEventsForTimeline, { + const timeline = await github.paginate( + github.rest.issues.listEventsForTimeline, + { owner, repo, issue_number: number, per_page: 100 - }) - ]); - rereviewRequested = hasRereviewRequest(pull, reviews, timeline); - if (!reviews.some((review) => review.state === "CHANGES_REQUESTED")) { + } + ); + rereviewRequested = hasRereviewRequest(pull, changeRequests, timeline); + if (changeRequests.length === 0) { core.warning( `#${number} reports changes requested but has no active change-request review` ); @@ -927,7 +1004,9 @@ async function syncPull(github, core, owner, repo, number) { rereviewRequested, requiredChecksPassed, reviewDecision, - unresolvedReviewFeedback + unresolvedReviewFeedback, + hasSubmittedHumanReview, + changeRequests.length > 0 ); if (mergeabilityUnknown) { core.warning( diff --git a/docs/contributing/contributing-pull-requests/README.md b/docs/contributing/contributing-pull-requests/README.md index 8a13a2d4438..f2c769bfc9a 100644 --- a/docs/contributing/contributing-pull-requests/README.md +++ b/docs/contributing/contributing-pull-requests/README.md @@ -97,12 +97,16 @@ The `PR Required Labels` check explains which label is missing. Contributors who The [PR Status Labels workflow](../../../.github/workflows/pr.status-labels.yml) applies these **handoff labels** to open, non-draft pull requests. Exactly one applies unless the PR is on hold: -- **`pr:needs-reviewer`** — the PR needs a reviewer or team to be requested. -- **`pr:waiting-for-review`** — a reviewer or team has a pending request. When a reviewer requests changes, the author must re-request a review after addressing them; pushing a commit alone does not hand the PR back. -- **`pr:waiting-for-author`** — unresolved inline feedback from a human reviewer, a formal changes-requested review, or a manual author-response request needs action. -- **`pr:review-approved`** — required reviews are approved and no unresolved human-reviewer inline feedback remains; this does not mean checks have passed. +- **`pr:needs-reviewer`** — there is neither a pending reviewer or team request nor an active submitted human review from someone other than the author. +- **`pr:waiting-for-review`** — a reviewer or team has a pending request, or a human has submitted a review but required approval is still outstanding or cannot be verified. When a reviewer requests changes, the author must re-request a review after addressing them; pushing a commit alone does not hand the PR back. +- **`pr:waiting-for-author`** — unresolved human-reviewer inline feedback, inline feedback endorsed by a human approval summary, a formal changes-requested review, or a manual author-response request needs action. +- **`pr:review-approved`** — required reviews are approved and no qualifying unresolved inline feedback remains; this does not mean checks have passed. -An unresolved inline thread containing a submitted comment from a human reviewer other than the PR author takes precedence over pending review requests and approvals. Bot comments, author-only notes, reviews that are pending, not yet submitted, or dismissed, deleted feedback, and general top-level discussion do not qualify. Replying or pushing commits does not resolve a thread, and outdated but unresolved feedback still counts. Once all qualifying threads are resolved or their qualifying feedback is deleted or dismissed, the workflow recomputes the normal review state: pending requests return the turn to reviewers, approvals can restore approval and queue-ready labels, and an outstanding formal changes-requested review still requires an explicit re-request. +An unresolved inline thread containing a submitted comment from a human reviewer other than the PR author takes precedence over pending review requests and approvals. Bot comments alone, author-only notes, reviews that are pending, not yet submitted, or dismissed, deleted feedback, and general top-level discussion do not qualify. Replying or pushing commits does not resolve a thread, and outdated but unresolved feedback still counts. Once all qualifying threads are resolved or their qualifying feedback is deleted or dismissed, the workflow recomputes the normal review state: pending requests return the turn to reviewers, approvals can restore approval and queue-ready labels, and an outstanding formal changes-requested review still requires an explicit re-request. + +A human reviewer's latest submitted opinionated review also endorses existing inline feedback when it approves the current head, has a nonempty summary, and has not been dismissed. This conservative, deterministic rule counts unresolved submitted feedback, including bot comments, only if both the comment's creation and its review's submission were at or before that approval. It checks summary emptiness, not wording; it uses no language classifier. Empty summaries, stale approvals, and bot comments posted or submitted after the approval do not establish this handoff. An approval summary without qualifying unresolved threads does not request author action, and general issue comments never establish it. + +Submitted review evidence establishes who has participated, not whether merging is allowed. Required approvals, CODEOWNER review, and last-push approval safeguards apply to the PR's actual target branch, including stacked targets. If approval cannot be safely established, an active submitted human review keeps the PR waiting for review rather than requesting a new reviewer; qualifying author feedback takes precedence. An outstanding changes-requested review from a human other than the author takes precedence regardless of target protection or GitHub's aggregate review decision. It hands the PR back to reviewers only when a currently pending reviewer or team request was explicitly made after the outstanding change requests. Dismissing the review or superseding it with an approval clears that review's change request; a comment-only review or a push does not. The workflow also adds **`pr:needs-rebase`** when the PR has merge conflicts, and **`pr:ready-for-queue`** when review is approved, the PR is mergeable, required checks pass, and it is not already queued. A branch that is behind `main` can still be queue-ready once its required checks pass; the merge queue handles the branch update. These labels are signals, not substitutes for GitHub's merge rules or the merge queue.