From 926237f642380c1aefcc2cf2aa414ca16456da8a Mon Sep 17 00:00:00 2001 From: Aliaksandr Date: Sat, 9 May 2026 15:15:27 +0300 Subject: [PATCH] ci/github-script/merge: share reviews fetch with bot.js, drop events `bot.js` already pulls reviews via GraphQL for the approval-count labels. Move that fetch above the `handleMerge` call, add `commit { oid }` to the query, and pass the result through. `handleMerge` no longer issues its own `listReviews` REST call. While here, the `approvals` set is now derived from the same reviews data. Two upsides: - A reviewer who approved and was later dismissed no longer counts towards the approval check; their review now surfaces with state DISMISSED instead of leaving a stale `reviewed` event behind. - `events` is no longer needed by `runChecklist` (it was only feeding the approvals filter); the parameter is dropped from both call sites. `handleMerge` still uses `events` for tracking the latest push and merge-command comments. Also drop the redundant `user` truthy checks (and the stale "some users have been deleted" comment) in `runChecklist`. The GraphQL query uses `author { ... on User { login id } }` and bot.js then filters via `r.user?.login`, so by the time reviews reach `runChecklist` every entry already has a populated `user`. Verified by querying real PRs: bots surface as `{__typename: "Bot"}` (no `login`/`id`, filtered out by bot.js), and deleted accounts surface as the "ghost" user (login `"ghost"`, id `10137`), which passes the filter but matches no committer - harmless for both checks. Assisted-by: claude-code with claude-opus-4-8[1m]-high (cherry picked from commit 0c1c3d48139703764cb5415b585313ef8b15d0fe) --- ci/github-script/bot.js | 29 ++++++++++++++++------------- ci/github-script/merge.js | 27 ++++++--------------------- 2 files changed, 22 insertions(+), 34 deletions(-) diff --git a/ci/github-script/bot.js b/ci/github-script/bot.js index c952499cec54..8e0043d5505c 100644 --- a/ci/github-script/bot.js +++ b/ci/github-script/bot.js @@ -206,20 +206,8 @@ module.exports = async ({ github, context, core, dry }) => { const maintainers = await getMaintainerMap(pull_request.base.ref) - const merge_bot_eligible = await handleMerge({ - github, - context, - core, - log, - dry, - pull_request, - events, - maintainers, - getTeamMembers, - getUser, - }) - // Check for any human reviews other than the PR author, GitHub actions and other GitHub apps. + // `commit { oid }` is needed by handleMerge to verify approvals are against the current head. const reviews = ( await github.graphql( `query($owner: String!, $repo: String!, $pr: Int!) { @@ -231,6 +219,7 @@ module.exports = async ({ github, context, core, dry }) => { reviews(first: 100) { nodes { state + commit { oid } user: author { # Only get users, no bots ... on User { @@ -266,6 +255,20 @@ module.exports = async ({ github, context, core, dry }) => { r.user.id !== pull_request.user?.id, ) + const merge_bot_eligible = await handleMerge({ + github, + context, + core, + log, + dry, + pull_request, + events, + reviews, + maintainers, + getTeamMembers, + getUser, + }) + const approvals = new Set( reviews .filter((review) => review.state === 'APPROVED') diff --git a/ci/github-script/merge.js b/ci/github-script/merge.js index 963d2c1e61bf..180a12b0fe4a 100644 --- a/ci/github-script/merge.js +++ b/ci/github-script/merge.js @@ -2,7 +2,6 @@ const { classify } = require('../supportedBranches.js') function runChecklist({ committers, - events, files, pull_request, log, @@ -28,18 +27,15 @@ function runChecklist({ .reduce((acc, cur) => acc?.intersection(cur) ?? cur) const approvals = new Set( - events + reviews .filter( - ({ event, state, commit_id }) => - event === 'reviewed' && - state === 'approved' && + ({ state, commit }) => + state === 'APPROVED' && // Only approvals for the current head SHA count, otherwise authors could push // bad code between the approval and the merge. - commit_id === pull_request.head.sha, + commit?.oid === pull_request.head.sha, ) - .map(({ user }) => user?.id) - // Some users have been deleted, so filter these out. - .filter(Boolean), + .map(({ user }) => user.id), ) // A "changes requested" review from a committer blocks both the merge queue and @@ -52,7 +48,6 @@ function runChecklist({ const committerReviewState = new Map() for (const { user, state } of reviews) { if ( - user && committers.has(user.id) && ['APPROVED', 'CHANGES_REQUESTED'].includes(state) ) { @@ -150,6 +145,7 @@ async function handleMerge({ dry, pull_request, events, + reviews, maintainers, getTeamMembers, getUser, @@ -183,15 +179,6 @@ async function handleMerge({ }) ).data.find(({ context }) => context === 'no PR failures')?.state - // Reviews are returned in chronological order; the latest review per user wins below. - // Dismissed reviews surface with state DISMISSED and comment-only follow-ups as - // COMMENTED, so both naturally drop out of the committer-blocking check. - const reviews = await github.paginate(github.rest.pulls.listReviews, { - ...context.repo, - pull_number, - per_page: 100, - }) - // Only look through comments *after* the latest (force) push. const lastPush = events.findLastIndex( ({ event, sha, commit_id }) => @@ -339,7 +326,6 @@ async function handleMerge({ const { result, eligible, checklist } = runChecklist({ committers, - events, files, pull_request, log, @@ -411,7 +397,6 @@ async function handleMerge({ const { result } = runChecklist({ committers, - events, files, pull_request, log,