{ci,workflows}: allow multiple blocking reviews

ci/github-script/check-target-branch.js

@@ -151,11 +151,9 @@ async function checkTargetBranch({ github, context, core, dry }) {
       core,
       dry,
       body,
-      event: 'COMMENT',
+      event: 'REQUEST_CHANGES',
       reviewKey,
     })
-
-    throw new Error('This PR is against the wrong branch.')
   } else if (rebuildsAllTests && !isExemptKernelUpdate) {
     let branchText
     if (base === 'master' && maxRebuildCount >= 500) {
@@ -179,11 +177,9 @@ async function checkTargetBranch({ github, context, core, dry }) {
       core,
       dry,
       body,
-      event: 'COMMENT',
+      event: 'REQUEST_CHANGES',
       reviewKey,
     })
-
-    throw new Error('This PR is against the wrong branch.')
   } else if (
     maxRebuildCount >= 500 &&
     !isExemptKernelUpdate &&
@@ -204,7 +200,7 @@ async function checkTargetBranch({ github, context, core, dry }) {
       core,
       dry,
       body,
-      event: 'COMMENT',
+      event: 'REQUEST_CHANGES',
       reviewKey,
     })
   } else {

ci/github-script/manual-file-edits.js

@@ -7,9 +7,13 @@ const { getCommitDetailsForPR } = require('./get-pr-commit-details')
  *  context: import('@actions/github/lib/context').Context,
  *  core: import('@actions/core'),
  *  repoPath?: string,
+ *  dry: boolean,
  * }} CheckManualFileEditsProps
  */
-async function checkManualFileEdits({ github, context, core, repoPath }) {
+async function checkManualFileEdits({ github, context, core, repoPath, dry }) {
+  const { dismissReviews, postReview } = require('./reviews.js')
+  const reviewKey = 'manual-file-edits'
+
   const pull_number = context.payload.pull_request?.number
   if (!pull_number) {
     core.info('This is not a pull request. Skipping checks.')
@@ -35,8 +39,13 @@ async function checkManualFileEdits({ github, context, core, repoPath }) {
       changedPaths.includes('maintainers/github-teams.json'),
     )
   ) {
-    core.setFailed(
-      [
+    postReview({
+      github,
+      context,
+      core,
+      dry,
+      event: 'REQUEST_CHANGES',
+      body: [
         'maintainers/github-teams.json is supposed to accurately reflect the state of the teams in GitHub.\n',
         'Therefore, it should not be edited manually.\n',
         'All changes to teams listed in maintainers/github-teams.json should be performed in GitHub by a team maintainer.\n',
@@ -48,7 +57,16 @@ async function checkManualFileEdits({ github, context, core, repoPath }) {
         (prev, curr) => prev + (!prev || prev.endsWith('\n') ? '' : ' ') + curr,
         '',
       ),
-    )
+      reviewKey,
+    })
+  } else {
+    dismissReviews({
+      github,
+      context,
+      core,
+      dry,
+      reviewKey,
+    })
   }
 }
 

ci/github-script/prepare.js

@@ -172,14 +172,20 @@ module.exports = async ({ github, context, core, dry }) => {
           '  ```',
         ].join('\n')
 
-        await postReview({ github, context, core, dry, body, reviewKey })
-
-        throw new Error(`The PR contains commits from a different base.`)
+        await postReview({
+          github,
+          context,
+          core,
+          dry,
+          body,
+          event: 'REQUEST_CHANGES',
+          reviewKey,
+        })
       }
+    } else {
+      await dismissReviews({ github, context, core, dry, reviewKey })
     }
 
-    await dismissReviews({ github, context, core, dry, reviewKey })
-
     let mergedSha, targetSha
 
     if (prInfo.mergeable) {

ci/github-script/reviews.js

@@ -5,10 +5,28 @@ const eventToState = {
   REQUEST_CHANGES: 'CHANGES_REQUESTED',
 }
 
+// Use substring checks in order to allow testing in forks
+// Usernames must also end in "[bot]"
+const reviewUsers = [
+  'github-actions',
+  'nixpkgs-ci',
+  'branch-check',
+  'commit-check',
+  'manual-edit',
+]
+
+/**
+ * @typedef {InstanceType<import('@actions/github/lib/utils').GitHub>} GitHub
+ * @typedef {typeof import('@actions/github').context} Context
+ *
+ * @typedef {Awaited<ReturnType<GitHub['rest']['pulls']['listReviews']>>['data'][number]} Review
+ * @typedef {Review & { user: NonNullable<Review['user']> }} ReviewWithNonNullUser
+ */
+
 /**
  * @param {{
- *  github: InstanceType<import('@actions/github/lib/utils').GitHub>,
- *  context: import('@actions/github/lib/context').Context,
+ *  github: GitHub,
+ *  context: Context,
  *  core: import('@actions/core'),
  *  dry: boolean,
  *  reviewKey?: string,
@@ -25,18 +43,32 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) {
     return
   }
 
-  const reviews = (
-    await github.paginate(github.rest.pulls.listReviews, {
-      ...context.repo,
-      pull_number,
-    })
-  ).filter(
-    (review) =>
-      review.user?.login === 'github-actions[bot]' &&
-      review.state !== 'DISMISSED',
+  const allReviews = await github.paginate(github.rest.pulls.listReviews, {
+    ...context.repo,
+    pull_number,
+  })
+
+  const reviews = /** @type {ReviewWithNonNullUser[]} */ (
+    allReviews.filter(
+      (review) =>
+        review.user &&
+        review.state !== 'DISMISSED' &&
+        review.user.login.endsWith('[bot]') &&
+        reviewUsers.some((substr) => review.user?.login.includes(substr)),
+    )
   )
-  const changesRequestedReviews = reviews.filter(
-    (review) => review.state === 'CHANGES_REQUESTED',
+
+  const reviewsByUser = reviews.reduce(
+    (prev, curr) => {
+      if (!(curr.user.login in prev)) {
+        prev[curr.user.login] = []
+      }
+
+      prev[curr.user.login].push(curr)
+
+      return prev
+    },
+    /** @type {Record<string, ReviewWithNonNullUser[]> } */ ({}),
   )
 
   const commentRegex = new RegExp(
@@ -50,8 +82,8 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) {
   )
 
   let reviewsToMinimize = reviews
-  let /** @type {typeof reviews} */ reviewsToDismiss = []
-  let /** @type {typeof reviews} */ reviewsToResolve = []
+  const /** @type {ReviewWithNonNullUser[]} */ reviewsToDismiss = []
+  const /** @type {ReviewWithNonNullUser[]} */ reviewsToResolve = []
 
   if (reviewKey && reviews.every((review) => commentRegex.test(review.body))) {
     reviewsToMinimize = reviews.filter((review) =>
@@ -59,29 +91,39 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) {
     )
   }
 
-  // If we want to dismiss all reviews with the key reviewKey,
-  // but there are other requested changes from CI, we can't dismiss,
-  // because then the other requested changes will be dismissed too.
-  if (
-    changesRequestedReviews.every(
-      (review) =>
-        commentResolvedRegex.test(review.body) ||
-        (reviewKey && reviewKeyRegex.test(review.body)) ||
-        // If we are called by check-commits and the review body is clearly
-        // from `commits.js`, then we can safely dismiss the review.
-        // This helps with pre-existing reviews (before the comments were added).
-        (reviewKey &&
-          reviewKey === 'check-commits' &&
-          review.body.includes('PR / Check / cherry-pick')),
-    )
-  ) {
-    reviewsToDismiss = changesRequestedReviews
-  } else if (reviewsToMinimize.length) {
-    reviewsToResolve = reviewsToMinimize.filter(
-      (review) =>
-        review.state === 'CHANGES_REQUESTED' &&
-        !commentResolvedRegex.test(review.body),
-    )
+  for (const reviewsForUser of Object.values(reviewsByUser)) {
+    // Make sure that we don't dismiss all reviews by a user if they
+    // have any reviews we don't want to dismiss.
+    if (
+      reviewsForUser.every(
+        (review) =>
+          commentResolvedRegex.test(review.body) ||
+          (reviewKey && reviewKeyRegex.test(review.body)) ||
+          // If we are called by check-commits and the review body is clearly
+          // from `commits.js`, then we can safely dismiss the review.
+          // This helps with pre-existing reviews (before the comments were added).
+          (reviewKey &&
+            reviewKey === 'check-commits' &&
+            review.body.includes('PR / Check / cherry-pick')),
+      )
+    ) {
+      reviewsToDismiss.push(
+        ...reviewsForUser.filter(
+          (review) => review.state === 'CHANGES_REQUESTED',
+        ),
+      )
+    } else {
+      reviewsToResolve.push(
+        ...reviewsForUser.filter(
+          (review) =>
+            review.state === 'CHANGES_REQUESTED' &&
+            !commentResolvedRegex.test(review.body) &&
+            reviewsToMinimize.some(
+              (toMinimize) => toMinimize.node_id === review.node_id,
+            ),
+        ),
+      )
+    }
   }
 
   await Promise.all([
@@ -121,8 +163,8 @@ async function dismissReviews({ github, context, core, dry, reviewKey }) {
 
 /**
  * @param {{
- *  github: InstanceType<import('@actions/github/lib/utils').GitHub>,
- *  context: import('@actions/github/lib/context').Context
+ *  github: GitHub,
+ *  context: Context,
  *  core: import('@actions/core'),
  *  dry: boolean,
  *  body: string,
@@ -158,11 +200,13 @@ async function postReview({
     })
   ).filter(
     (review) =>
-      review.user?.login === 'github-actions[bot]' &&
-      review.state !== 'DISMISSED',
+      review.user &&
+      review.state !== 'DISMISSED' &&
+      review.user.login.endsWith('[bot]') &&
+      reviewUsers.some((substr) => review.user?.login.includes(substr)),
   )
 
-  /** @type {null | typeof reviews[number]} */
+  /** @type {null | Review} */
   let pendingReview
   const matchingReviews = reviews.filter((review) =>
     reviewKeyRegex.test(review.body),