Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/labels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
167 changes: 123 additions & 44 deletions .github/scripts/pr-status-labels.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -132,6 +138,7 @@ const REVIEW_THREAD_FRAGMENT = `
comments(first: 100, after: $commentsCursor) {
nodes {
state
createdAt
author {
__typename
login
Expand Down Expand Up @@ -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, {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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 ?
Comment thread
DariuszPorowski marked this conversation as resolved.
STATUS_LABELS.waitingForReview
: STATUS_LABELS.needsReviewer;
Comment thread
DariuszPorowski marked this conversation as resolved.
}
Expand Down Expand Up @@ -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 (
Expand All @@ -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;
}
Expand Down Expand Up @@ -841,19 +905,34 @@ 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" &&
!pull.isDraft &&
!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
);
}

Expand All @@ -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`
);
Expand Down Expand Up @@ -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(
Expand Down
14 changes: 9 additions & 5 deletions docs/contributing/contributing-pull-requests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
Loading