Reapply {ci,workflows}: allow multiple blocking reviews"

A couple of bugfixes, but the problem was that the apps weren't installed.

.github/workflows/check.yml

@@ -16,6 +16,14 @@ on:
         required: true
         type: string
     secrets:
+      # Can be provided in pull requests because the job it is used in does
+      # not evaluate untrusted code.
+      NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY:
+        required: false
+      # Can be provided in pull requests because the job it is used in does
+      # not evaluate untrusted code.
+      NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY:
+        required: false
       # Should only be provided in the merge queue, not in pull requests,
       # where we're evaluating untrusted code.
       CACHIX_AUTH_TOKEN_GHA:
@@ -45,9 +53,17 @@ jobs:
       - name: Install dependencies
         run: npm install bottleneck@2.19.5
 
+      - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1
+        if: github.event_name != 'pull_request' && vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID
+        id: app-token
+        with:
+          client-id: ${{ vars.NIXPKGS_COMMIT_CHECK_CLIENT_ID }}
+          private-key: ${{ secrets.NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY }}
+          permission-pull-requests: write
+
       - name: Log current API rate limits
         env:
-          GH_TOKEN: ${{ github.token }}
+          GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }}
         run: gh api /rate_limit | jq
 
       - name: Check commits
@@ -56,6 +72,7 @@ jobs:
         env:
           TARGETS_STABLE: ${{ fromJSON(inputs.baseBranch).stable && !contains(fromJSON(inputs.headBranch).type, 'development') }}
         with:
+          github-token: ${{ steps.app-token.outputs.token || github.token }}
           script: |
             const targetsStable = JSON.parse(process.env.TARGETS_STABLE)
             require('./trusted/ci/github-script/commits.js')({
@@ -68,7 +85,7 @@ jobs:
 
       - name: Log current API rate limits
         env:
-          GH_TOKEN: ${{ github.token }}
+          GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }}
         run: gh api /rate_limit | jq
 
   manual-file-edits:
@@ -85,25 +102,35 @@ jobs:
           sparse-checkout: |
             ci/github-script
 
+      - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1
+        if: github.event_name != 'pull_request' && vars.NIXPKGS_MANUAL_EDIT_CHECK_CLIENT_ID
+        id: app-token
+        with:
+          client-id: ${{ vars.NIXPKGS_MANUAL_EDIT_CHECK_CLIENT_ID }}
+          private-key: ${{ secrets.NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY }}
+          permission-pull-requests: write
+
       - name: Log current API rate limits
         env:
-          GH_TOKEN: ${{ github.token }}
+          GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }}
         run: gh api /rate_limit | jq
 
       - name: Discourage manual edits to certain files
         uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
         with:
+          github-token: ${{ steps.app-token.outputs.token || github.token }}
           script: |
             require('./trusted/ci/github-script/manual-file-edits.js')({
               github,
               context,
               core,
+              dry: context.eventName == 'pull_request',
               repoPath: 'trusted',
             })
 
       - name: Log current API rate limits
         env:
-          GH_TOKEN: ${{ github.token }}
+          GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }}
         run: gh api /rate_limit | jq
 
   owners:

.github/workflows/eval.yml

@@ -23,6 +23,10 @@ on:
         default: false
         type: boolean
     secrets:
+      # Can be provided in pull requests because the job it is used in does
+      # not evaluate untrusted code.
+      NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY:
+        required: false
       # Should only be provided in the merge queue, not in pull requests,
       # where we're evaluating untrusted code.
       CACHIX_AUTH_TOKEN_GHA:
@@ -349,10 +353,22 @@ jobs:
               description,
               target_url
             })
+
+      - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1
+        if: github.event_name == 'pull_request_target' && vars.NIXPKGS_BRANCH_CHECK_CLIENT_ID
+        id: app-token
+        with:
+          client-id: ${{ vars.NIXPKGS_BRANCH_CHECK_CLIENT_ID }}
+          private-key: ${{ secrets.NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY }}
+          permission-pull-requests: write
+
+      # It's fine to reuse this app in the 'pull-request-target / prepare' job,
+      # because that job has to run before this one.
       - name: Request changes if PR is against an inappropriate branch
         if: ${{ github.event_name == 'pull_request_target' }}
         uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
         with:
+          github-token: ${{ steps.app-token.outputs.token || github.token }}
           script: |
             require('./nixpkgs/trusted/ci/github-script/check-target-branch.js')({
               github,

.github/workflows/pull-request-target.yml

@@ -10,6 +10,12 @@ on:
     secrets:
       NIXPKGS_CI_APP_PRIVATE_KEY:
         required: true
+      NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY:
+        required: true
+      NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY:
+        required: true
+      NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY:
+        required: true
 
 concurrency:
   group: pr-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }}
@@ -36,9 +42,21 @@ jobs:
           sparse-checkout-cone-mode: true # default, for clarity
           sparse-checkout: |
             ci/github-script
+
+      # It's fine to reuse this app in the 'eval / compare' job,
+      # because this job has to run before that one.
+      - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1
+        if: vars.NIXPKGS_BRANCH_CHECK_CLIENT_ID
+        id: app-token
+        with:
+          client-id: ${{ vars.NIXPKGS_BRANCH_CHECK_CLIENT_ID }}
+          private-key: ${{ secrets.NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY }}
+          permission-pull-requests: write
+
       - id: prepare
         uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
         with:
+          github-token: ${{ steps.app-token.outputs.token || github.token }}
           retries: 10
           # The default for this includes code 422, which happens regularly for us when comparing commits:
           #   422 - Server Error: Sorry, this diff is taking too long to generate.
@@ -60,6 +78,9 @@ jobs:
     permissions:
       # cherry-picks
       pull-requests: write
+    secrets:
+      NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY }}
+      NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY }}
     with:
       baseBranch: ${{ needs.prepare.outputs.baseBranch }}
       headBranch: ${{ needs.prepare.outputs.headBranch }}
@@ -82,6 +103,8 @@ jobs:
       # compare
       pull-requests: write
       statuses: write
+    secrets:
+      NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY }}
     with:
       artifact-prefix: ${{ inputs.artifact-prefix }}
       mergedSha: ${{ needs.prepare.outputs.mergedSha }}

.github/workflows/test.yml

@@ -116,5 +116,8 @@ jobs:
       statuses: write # unused on pull_request, required by PR workflow
     secrets:
       NIXPKGS_CI_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_CI_APP_PRIVATE_KEY }}
+      NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_BRANCH_CHECK_APP_PRIVATE_KEY }}
+      NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_COMMIT_CHECK_APP_PRIVATE_KEY }}
+      NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_MANUAL_EDIT_CHECK_APP_PRIVATE_KEY }}
     with:
       artifact-prefix: pr-

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