[Backport release-26.05] ci/github-script/merge: surface auto-merge blockers in the bot's checklist (#535335)

ci/README.md

@@ -51,6 +51,16 @@ To ensure security and a focused utility, the bot adheres to specific limitation
   - opened by [@r-ryantm](https://nix-community.github.io/nixpkgs-update/r-ryantm/).
 - The user attempting to merge is a member of [@NixOS/nixpkgs-maintainers].
 - The user attempting to merge is a maintainer of all packages touched by the PR.
+- No [committer][@NixOS/nixpkgs-committers] has an outstanding "changes requested" review.
+  These block both the merge queue and auto-merge, so the bot refuses to merge until the review is addressed or dismissed.
+
+Once these constraints are met, the bot picks a merge strategy based on the `no PR failures` commit status:
+
+- CI passing: the PR is added to the merge queue.
+- CI unfinished (pending or missing status): the bot enables [Auto Merge], which queues the PR once required checks succeed.
+  Note that if CI later fails, nothing happens until it is fixed and passes.
+- CI already failing (`error`/`failure` status): the bot does not enable Auto Merge, because it would never trigger, and fixing CI requires a new push that invalidates the merge command.
+  A fresh `@NixOS/nixpkgs-merge-bot merge` comment is needed once CI is green again.
 
 ### Approving merge bot changes
 
@@ -104,3 +114,4 @@ This script can also be run locally to print basic test cases.
 [@NixOS/nixpkgs-ci]: https://github.com/orgs/NixOS/teams/nixpkgs-ci
 [@NixOS/nixpkgs-core]: https://github.com/orgs/NixOS/teams/nixpkgs-core
 [RFC 172]: https://github.com/NixOS/rfcs/pull/172
+[Auto Merge]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request

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')

ci/github-script/merge.js

@@ -2,11 +2,11 @@ const { classify } = require('../supportedBranches.js')
 
 function runChecklist({
   committers,
-  events,
   files,
   pull_request,
   log,
   maintainers,
+  reviews,
   user,
   userIsMaintainer,
 }) {
@@ -27,18 +27,35 @@ 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
+  // auto-merge, even if it was made on an older commit (unlike approvals, GitHub does
+  // not auto-dismiss changes-requested reviews on push). For each committer, take their
+  // latest actionable review; if it's CHANGES_REQUESTED, they're blocking the PR.
+  // Dismissed reviews surface as DISMISSED and comment-only follow-ups as COMMENTED, so
+  // both are skipped naturally — the prior actionable review still stands until the
+  // committer explicitly approves or requests changes again.
+  const committerReviewState = new Map()
+  for (const { user, state } of reviews) {
+    if (
+      committers.has(user.id) &&
+      ['APPROVED', 'CHANGES_REQUESTED'].includes(state)
+    ) {
+      committerReviewState.set(user.id, state)
+    }
+  }
+  const noBlockingReviews = !Array.from(committerReviewState.values()).includes(
+    'CHANGES_REQUESTED',
   )
 
   const checklist = {
@@ -57,6 +74,11 @@ function runChecklist({
         pull_request.user.login === 'r-ryantm',
     },
     'PR is not a draft': !pull_request.draft,
+    // CI state is intentionally *not* a checklist item: auto-merge exists precisely to
+    // cover unfinished CI, and an already-failed CI is reported via the merge message
+    // (see merge() below) rather than a blanket refusal.
+    'PR is not blocked by a "changes requested" review from a [committer](https://github.com/orgs/NixOS/teams/nixpkgs-committers).':
+      noBlockingReviews,
   }
 
   if (user) {
@@ -123,6 +145,7 @@ async function handleMerge({
   dry,
   pull_request,
   events,
+  reviews,
   maintainers,
   getTeamMembers,
   getUser,
@@ -148,6 +171,14 @@ async function handleMerge({
   // including an early exit when the first non-by-name file is found.
   if (files.length >= 100) return false
 
+  const noPrFailuresState = (
+    await github.rest.repos.listCommitStatusesForRef({
+      ...context.repo,
+      ref: pull_request.head.sha,
+      per_page: 100,
+    })
+  ).data.find(({ context }) => context === 'no PR failures')?.state
+
   // Only look through comments *after* the latest (force) push.
   const lastPush = events.findLastIndex(
     ({ event, sha, commit_id }) =>
@@ -173,10 +204,12 @@ async function handleMerge({
         )),
   )
 
+  // Returns `{ reaction, messages }`: the reaction to leave on the merge comment and the
+  // lines to append to the bot's reply. Throws only on an unexpected API error.
   async function merge() {
     if (dry) {
       core.info(`Merging #${pull_number}... (dry)`)
-      return ['Merge completed (dry)']
+      return { reaction: 'ROCKET', messages: ['Merge completed (dry)'] }
     }
 
     // Using GraphQL mutations instead of the REST /merge endpoint, because the latter
@@ -197,16 +230,37 @@ async function handleMerge({
         { node_id: pull_request.node_id, sha: pull_request.head.sha },
       )
       log('merge', 'Queued for merge')
-      return [
-        `:heavy_check_mark: [Queued](${resp.enqueuePullRequest.mergeQueueEntry.mergeQueue.url}) for merge (#306934)`,
-      ]
+      return {
+        reaction: 'ROCKET',
+        messages: [
+          `:heavy_check_mark: [Queued](${resp.enqueuePullRequest.mergeQueueEntry.mergeQueue.url}) for merge (#306934)`,
+        ],
+      }
     } catch (e) {
       log('Enqueuing failed', e.response.errors[0].message)
     }
 
-    // If required status checks are not satisfied, yet, the above will fail. In this case
-    // we can enable auto-merge. We could also only use auto-merge, but this often gets
-    // stuck for no apparent reason.
+    // Enqueuing fails when the required status checks are not satisfied, yet. If CI has
+    // already failed, enabling auto-merge would be pointless: it would never fire, and
+    // fixing CI requires a new push, which invalidates this merge command anyway (we only
+    // act on comments after the latest push). So we don't enable auto-merge and instead
+    // ask for a fresh command once CI is green again.
+    if (['error', 'failure'].includes(noPrFailuresState)) {
+      log('merge', 'CI has failed, not enabling auto-merge')
+      return {
+        reaction: 'THUMBS_DOWN',
+        messages: [
+          ':x: Pull Request could not be merged: CI has failed (#305350).',
+          '',
+          '> [!TIP]',
+          '> PRs cannot be merged while CI is failing.',
+          '> Once CI is passing, comment `@NixOS/nixpkgs-merge-bot merge` again.',
+        ],
+      }
+    }
+
+    // CI has not finished yet, so we enable auto-merge. We could also only use auto-merge,
+    // but this often gets stuck for no apparent reason.
     try {
       await github.graphql(
         `mutation($node_id: ID!, $sha: GitObjectID) {
@@ -219,12 +273,17 @@ async function handleMerge({
         { node_id: pull_request.node_id, sha: pull_request.head.sha },
       )
       log('merge', 'Auto-merge enabled')
-      return [
-        `:heavy_check_mark: Enabled Auto Merge (#306934)`,
-        '',
-        '> [!TIP]',
-        '> Sometimes GitHub gets stuck after enabling Auto Merge. In this case, leaving another approval should trigger the merge.',
-      ]
+      return {
+        reaction: 'ROCKET',
+        messages: [
+          `:heavy_check_mark: Enabled Auto Merge (#306934)`,
+          '',
+          '> [!TIP]',
+          '> [Auto Merge](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request) will queue this PR once required CI checks succeed.',
+          '> If CI fails instead, fixing it needs a new push, which disables Auto Merge and invalidates this command — comment `@NixOS/nixpkgs-merge-bot merge` again once CI is green.',
+          '> If GitHub gets stuck even though CI passed (it sometimes does), leaving another approval should kick off the merge.',
+        ],
+      }
     } catch (e) {
       log('Auto Merge failed', e.response.errors[0].message)
       throw new Error(e.response.errors[0].message)
@@ -267,11 +326,11 @@ async function handleMerge({
 
     const { result, eligible, checklist } = runChecklist({
       committers,
-      events,
       files,
       pull_request,
       log,
       maintainers,
+      reviews,
       user: comment.user,
       userIsMaintainer: await isMaintainer(comment.user.login),
     })
@@ -308,10 +367,12 @@ async function handleMerge({
     }
 
     if (result) {
-      await react('ROCKET')
       try {
-        body.push(...(await merge()))
+        const { reaction, messages } = await merge()
+        await react(reaction)
+        body.push(...messages)
       } catch (e) {
+        await react('THUMBS_DOWN')
         // Remove the HTML comment with node_id reference to allow retrying this merge on the next run.
         body.shift()
         body.push(`:x: Merge failed with: ${e} (#371492)`)
@@ -336,11 +397,11 @@ async function handleMerge({
 
   const { result } = runChecklist({
     committers,
-    events,
     files,
     pull_request,
     log,
     maintainers,
+    reviews,
   })
 
   // Returns a boolean, which indicates whether the PR is merge-bot eligible in principle.