From 843634b667dc58f859118dd22fad3906f911ffc1 Mon Sep 17 00:00:00 2001 From: Michael Daniels Date: Mon, 18 May 2026 17:26:09 -0400 Subject: [PATCH] Revert "ci/github-script/reviewers: revoke stale review requests" This reverts commit 2d34258228fcced6323f40871322afa7d1b3cdb6. --- ci/github-script/bot.js | 1 - ci/github-script/reviewers.js | 129 ++++++++-------------------------- 2 files changed, 28 insertions(+), 102 deletions(-) diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index 0fa2851ff38b..c952499cec54 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -483,7 +483,6 @@ module.exports = async ({ github, context, core, dry }) => { dry, pull_request, reviews, - events, // TODO: Use maintainer map instead of the artifact. user_maintainers: Object.keys( JSON.parse( diff --git a/ci/github-script/reviewers.js b/ci/github-script/reviewers.js index 6fc6ede00f96..be458ba4eb32 100644 --- a/ci/github-script/reviewers.js +++ b/ci/github-script/reviewers.js @@ -6,7 +6,6 @@ async function handleReviewers({ dry, pull_request, reviews, - events, user_maintainers, team_maintainers, owners, @@ -15,28 +14,20 @@ async function handleReviewers({ }) { const pull_number = pull_request.number - // Users currently requested for review (pending). - const pending_users = new Set( - pull_request.requested_reviewers.map(({ login }) => login.toLowerCase()), - ) - // Users who actually submitted a review in this PR (any state, including DISMISSED). - const users_engaged = new Set( - reviews.map(({ user }) => user.login.toLowerCase()), - ) - // Users the PR has already reached: pending OR engaged. - const users_reached = pending_users.union(users_engaged) + // Users that the PR has already reached, e.g. they've left a review or have been requested for one + const users_reached = new Set([ + ...pull_request.requested_reviewers.map(({ login }) => login.toLowerCase()), + ...reviews.map(({ user }) => user.login.toLowerCase()), + ]) log('reviewers - users_reached', Array.from(users_reached).join(', ')) - // Same for teams. A team is engaged only via `onBehalfOf` reviews. - const pending_teams = new Set( - pull_request.requested_teams.map(({ slug }) => slug.toLowerCase()), - ) - const teams_engaged = new Set( - reviews.flatMap(({ onBehalfOf }) => + // Same for teams + const teams_reached = new Set([ + ...pull_request.requested_teams.map(({ slug }) => slug.toLowerCase()), + ...reviews.flatMap(({ onBehalfOf }) => onBehalfOf.nodes.map(({ slug }) => slug.toLowerCase()), ), - ) - const teams_reached = pending_teams.union(teams_engaged) + ]) log('reviewers - teams_reached', Array.from(teams_reached).join(', ')) // Early sanity check, before we start making any API requests. The list of maintainers @@ -158,101 +149,37 @@ async function handleReviewers({ ) log('reviewers - teams_not_yet_reached', teams_not_yet_reached.join(', ')) - // The usernames of bots that make review requests we may auto-revoke. - const revokable_requesters = ['github-actions[bot]', 'nixpkgs-ci[bot]'] - - // Find latest `review_requested` actor per reviewer / team. - const last_request_actor_for_user = new Map() - const last_request_actor_for_team = new Map() - for (const ev of events) { - if (ev.event !== 'review_requested') continue - if (ev.requested_reviewer?.login) { - last_request_actor_for_user.set( - ev.requested_reviewer.login.toLowerCase(), - ev.actor?.login ?? '', - ) - } - if (ev.requested_team?.slug) { - last_request_actor_for_team.set( - ev.requested_team.slug.toLowerCase(), - ev.actor?.login ?? '', - ) - } - } - - // Pending requests no longer in the to_reach set, excluding the engaged - // and anything not requested by our own bot. - const users_to_remove = Array.from( - pending_users.difference(users_to_reach).difference(users_engaged), - ).filter((login) => - revokable_requesters.includes(last_request_actor_for_user.get(login)), - ) - log('reviewers - users_to_remove', users_to_remove.join(', ')) - - // Same for teams. - const teams_to_remove = Array.from( - pending_teams.difference(teams_to_reach).difference(teams_engaged), - ).filter((slug) => - revokable_requesters.includes(last_request_actor_for_team.get(slug)), - ) - log('reviewers - teams_to_remove', teams_to_remove.join(', ')) - - const has_adds = - users_not_yet_reached.length > 0 || teams_not_yet_reached.length > 0 - const has_removals = users_to_remove.length > 0 || teams_to_remove.length > 0 - - if (!has_adds && !has_removals) { + if ( + users_not_yet_reached.length === 0 && + teams_not_yet_reached.length === 0 + ) { log('Has reviewer changes', 'false (skipped)') } else if (dry) { - if (has_adds) { - core.info( - `Requesting user reviewers for #${pull_number}: ${users_not_yet_reached.join(', ')} (dry)`, - ) - core.info( - `Requesting team reviewers for #${pull_number}: ${teams_not_yet_reached.join(', ')} (dry)`, - ) - } - if (has_removals) { - core.info( - `Revoking stale reviewers for #${pull_number}: users=[${users_to_remove.join(', ')}], teams=[${teams_to_remove.join(', ')}] (dry)`, - ) - } + core.info( + `Requesting user reviewers for #${pull_number}: ${users_not_yet_reached.join(', ')} (dry)`, + ) + core.info( + `Requesting team reviewers for #${pull_number}: ${teams_not_yet_reached.join(', ')} (dry)`, + ) } else { // We had tried the "request all reviewers at once" thing in the past, but it didn't work out: // https://github.com/NixOS/nixpkgs/commit/034613f860fcd339bd2c20c8f6bc259a2f9dc034 // If we're hitting API errors here again, we'll need to investigate - and possibly reverse // course. - // Add and remove sets are disjoint by construction. Parallel is safe. - await Promise.all( - [ - has_adds && - github.rest.pulls.requestReviewers({ - ...context.repo, - pull_number, - reviewers: users_not_yet_reached, - team_reviewers: teams_not_yet_reached, - }), - has_removals && - github.rest.pulls.removeRequestedReviewers({ - ...context.repo, - pull_number, - reviewers: users_to_remove, - team_reviewers: teams_to_remove, - }), - ].filter(Boolean), - ) + await github.rest.pulls.requestReviewers({ + ...context.repo, + pull_number, + reviewers: users_not_yet_reached, + team_reviewers: teams_not_yet_reached, + }) } - // Subtract the just-revoked so revoking the last pending reviewer flips the label. - const users_still_reached = users_reached.difference(new Set(users_to_remove)) - const teams_still_reached = teams_reached.difference(new Set(teams_to_remove)) - // Return a boolean on whether the "needs: reviewers" label should be set. return ( users_not_yet_reached.length === 0 && teams_not_yet_reached.length === 0 && - users_still_reached.size === 0 && - teams_still_reached.size === 0 + users_reached.size === 0 && + teams_reached.size === 0 ) }