ci/github-script/bot: request reviewers (#458604)

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

@@ -112,18 +112,6 @@ jobs:
     with:
       headBranch: ${{ needs.prepare.outputs.headBranch }}
 
-  reviewers:
-    name: Reviewers
-    needs: [prepare, eval]
-    if: |
-      needs.prepare.outputs.targetSha &&
-      !contains(fromJSON(needs.prepare.outputs.headBranch).type, 'development')
-    uses: ./.github/workflows/reviewers.yml
-    secrets:
-      OWNER_APP_PRIVATE_KEY: ${{ secrets.OWNER_APP_PRIVATE_KEY }}
-    with:
-      artifact-prefix: ${{ inputs.artifact-prefix }}
-
   build:
     name: Build
     needs: [prepare]

.github/workflows/reviewers.yml

@@ -1,157 +0,0 @@
-# This workflow will request reviews from the maintainers of each package
-# listed in the PR's most recent eval comparison artifact.
-
-name: Reviewers
-
-on:
-  pull_request_target:
-    types: [ready_for_review]
-  workflow_call:
-    inputs:
-      artifact-prefix:
-        required: true
-        type: string
-    secrets:
-      OWNER_APP_PRIVATE_KEY:
-        required: true
-
-concurrency:
-  group: reviewers-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }}
-  cancel-in-progress: true
-
-permissions: {}
-
-defaults:
-  run:
-    shell: bash
-
-jobs:
-  request:
-    runs-on: ubuntu-24.04-arm
-    timeout-minutes: 20
-    steps:
-      - name: Check out the PR at the base commit
-        uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
-        with:
-          persist-credentials: false
-          path: trusted
-          sparse-checkout: ci
-
-      - name: Install Nix
-        uses: cachix/install-nix-action@456688f15bc354bef6d396e4a35f4f89d40bf2b7 # v31
-
-      - name: Build the requestReviews derivation
-        run: nix-build trusted/ci -A requestReviews
-
-      # For requesting reviewers, this job depends on a GitHub App with the following permissions:
-      # - Permissions:
-      #   - Repository > Administration: read-only
-      #   - Organization > Members: read-only
-      #   - Repository > Pull Requests: read-write
-      # - Install App on this repository, setting these variables:
-      #   - OWNER_APP_ID (variable)
-      #   - OWNER_APP_PRIVATE_KEY (secret)
-      #
-      # Can't use the token received from permissions above, because it can't get enough permissions.
-      - uses: actions/create-github-app-token@67018539274d69449ef7c02e8e71183d1719ab42 # v2.1.4
-        if: github.event_name == 'pull_request_target' && vars.OWNER_APP_ID
-        id: app-token
-        with:
-          app-id: ${{ vars.OWNER_APP_ID }}
-          private-key: ${{ secrets.OWNER_APP_PRIVATE_KEY }}
-          permission-administration: read
-          permission-members: read
-          permission-pull-requests: write
-
-      - name: Log current API rate limits (github.token)
-        env:
-          GH_TOKEN: ${{ github.token }}
-        run: gh api /rate_limit | jq
-
-      # In the regular case, this workflow is called via workflow_call from the eval workflow directly.
-      # In the more special case, when a PR is undrafted an eval run will have started already.
-      - name: Wait for comparison to be done
-        uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0
-        id: eval
-        env:
-          ARTIFACT: ${{ inputs.artifact-prefix }}comparison
-        with:
-          script: |
-            const run_id = (await github.rest.actions.listWorkflowRuns({
-              owner: context.repo.owner,
-              repo: context.repo.repo,
-              workflow_id: context.eventName === 'pull_request' ? 'test.yml' : 'pull-request-target.yml',
-              event: context.eventName,
-              head_sha: context.payload.pull_request.head.sha
-            })).data.workflow_runs[0].id
-
-            core.setOutput('run-id', run_id)
-
-            // Waiting 120 * 5 sec = 10 min. max.
-            // The extreme case is an Eval run that just started when the PR is undrafted.
-            // Eval takes max 5-6 minutes, normally.
-            for (let i = 0; i < 120; i++) {
-              const result = await github.rest.actions.listWorkflowRunArtifacts({
-                owner: context.repo.owner,
-                repo: context.repo.repo,
-                run_id,
-                name: process.env.ARTIFACT,
-              })
-              if (result.data.total_count > 0) return
-              await new Promise(resolve => setTimeout(resolve, 5000))
-            }
-            throw new Error("No comparison artifact found.")
-
-      - name: Log current API rate limits (github.token)
-        env:
-          GH_TOKEN: ${{ github.token }}
-        run: gh api /rate_limit | jq
-
-      - name: Download the comparison results
-        uses: actions/download-artifact@018cc2cf5baa6db3ef3c5f8a56943fffe632ef53 # v6.0.0
-        with:
-          run-id: ${{ steps.eval.outputs.run-id }}
-          github-token: ${{ github.token }}
-          pattern: ${{ inputs.artifact-prefix }}comparison
-          path: comparison
-          merge-multiple: true
-
-      - name: Log current API rate limits (app-token)
-        if: ${{ steps.app-token.outputs.token }}
-        env:
-          GH_TOKEN: ${{ steps.app-token.outputs.token }}
-        run: gh api /rate_limit | jq
-
-      - name: Log current API rate limits (github.token)
-        env:
-          GH_TOKEN: ${{ github.token }}
-        run: gh api /rate_limit | jq
-
-      - name: Requesting reviews
-        if: ${{ steps.app-token.outputs.token }}
-        env:
-          GH_TOKEN: ${{ github.token }}
-          APP_GH_TOKEN: ${{ steps.app-token.outputs.token }}
-          REPOSITORY: ${{ github.repository }}
-          NUMBER: ${{ github.event.number }}
-          AUTHOR: ${{ github.event.pull_request.user.login }}
-          # Don't request reviewers on draft PRs
-          DRY_MODE: ${{ github.event.pull_request.draft && '1' || '' }}
-        run: |
-          # maintainers.json contains GitHub IDs. Look up handles to request reviews from.
-          # There appears to be no API to request reviews based on GitHub IDs
-          jq -r 'keys[]' comparison/maintainers.json \
-            | while read -r id; do gh api /user/"$id" --jq .login; done \
-            | cat comparison/owners.txt - \
-            | GH_TOKEN="$APP_GH_TOKEN" result/bin/request-reviewers.sh "$REPOSITORY" "$NUMBER" "$AUTHOR"
-
-      - name: Log current API rate limits (app-token)
-        if: ${{ steps.app-token.outputs.token }}
-        env:
-          GH_TOKEN: ${{ steps.app-token.outputs.token }}
-        run: gh api /rate_limit | jq
-
-      - name: Log current API rate limits (github.token)
-        env:
-          GH_TOKEN: ${{ github.token }}
-        run: gh api /rate_limit | jq

.github/workflows/test.yml

@@ -69,7 +69,6 @@ jobs:
               '.github/workflows/eval.yml',
               '.github/workflows/lint.yml',
               '.github/workflows/pull-request-target.yml',
-              '.github/workflows/reviewers.yml',
               '.github/workflows/test.yml',
               'ci/github-script/bot.js',
               'ci/github-script/merge.js',

ci/default.nix

@@ -156,7 +156,6 @@ let
 in
 rec {
   inherit pkgs fmt;
-  requestReviews = pkgs.callPackage ./request-reviews { };
   codeownersValidator = pkgs.callPackage ./codeowners-validator { };
 
   # FIXME(lf-): it might be useful to test other Nix implementations

ci/github-script/bot.js

@@ -5,6 +5,7 @@ module.exports = async ({ github, context, core, dry }) => {
   const withRateLimit = require('./withRateLimit.js')
   const { classify } = require('../supportedBranches.js')
   const { handleMerge } = require('./merge.js')
+  const { handleReviewers } = require('./reviewers.js')
 
   const artifactClient = new DefaultArtifactClient()
 
@@ -209,10 +210,16 @@ module.exports = async ({ github, context, core, dry }) => {
       }
     }
 
-    const reviews = await github.paginate(github.rest.pulls.listReviews, {
+    // Check for any human reviews other than GitHub actions and other GitHub apps.
+    // Accounts could be deleted as well, so don't count them.
+    const reviews = (
+      await github.paginate(github.rest.pulls.listReviews, {
         ...context.repo,
         pull_number,
       })
+    ).filter(
+      (r) => r.user && !r.user.login.endsWith('[bot]') && r.user.type !== 'Bot',
+    )
 
     const approvals = new Set(
       reviews
@@ -282,13 +289,6 @@ module.exports = async ({ github, context, core, dry }) => {
     log('Last eval run', run_id ?? '<n/a>')
 
     if (conclusion === 'success') {
-      // Check for any human reviews other than GitHub actions and other GitHub apps.
-      // Accounts could be deleted as well, so don't count them.
-      const humanReviews = reviews.filter(
-        (r) =>
-          r.user && !r.user.login.endsWith('[bot]') && r.user.type !== 'Bot',
-      )
-
       Object.assign(prLabels, {
         // We only set this label if the latest eval run was successful, because if it was not, it
         // *could* have requested reviewers. We will let the PR author fix CI first, before "escalating"
@@ -301,7 +301,7 @@ module.exports = async ({ github, context, core, dry }) => {
         '9.needs: reviewer':
           !pull_request.draft &&
           pull_request.requested_reviewers.length === 0 &&
-          humanReviews.length === 0,
+          reviews.length === 0,
       })
     }
 
@@ -373,6 +373,33 @@ module.exports = async ({ github, context, core, dry }) => {
           maintainers[pkg]?.some((m) => approvals.has(m)),
         ),
       })
+
+      if (!pull_request.draft) {
+        // We set this label earlier already, but the current PR state can be very different
+        // after handleReviewers has requested reviews, so update it in this case to prevent
+        // this label from flip-flopping.
+        prLabels['9.needs: reviewer'] = await handleReviewers({
+          github,
+          context,
+          core,
+          log,
+          dry,
+          pull_request,
+          reviews,
+          // TODO: Use maintainer map instead of the artifact.
+          maintainers: Object.keys(
+            JSON.parse(
+              await readFile(`${pull_number}/maintainers.json`, 'utf-8'),
+            ),
+          ).map((id) => parseInt(id)),
+          // TODO: Create owner map similar to maintainer map.
+          owners: (await readFile(`${pull_number}/owners.txt`, 'utf-8')).split(
+            '\n',
+          ),
+          getTeamMembers,
+          getUser,
+        })
+      }
     }
 
     return prLabels
@@ -521,7 +548,7 @@ module.exports = async ({ github, context, core, dry }) => {
       const hasChanges = Object.keys(after).some(
         (name) => (before[name] ?? false) !== after[name],
       )
-      if (log('Has changes', hasChanges, !hasChanges)) return
+      if (log('Has label changes', hasChanges, !hasChanges)) return
 
       // Skipping labeling on a pull_request event, because we have no privileges.
       const labels = Object.entries(after)

ci/github-script/reviewers.js

@@ -0,0 +1,128 @@
+async function handleReviewers({
+  github,
+  context,
+  core,
+  log,
+  dry,
+  pull_request,
+  reviews,
+  maintainers,
+  owners,
+  getTeamMembers,
+  getUser,
+}) {
+  const pull_number = pull_request.number
+
+  const users = new Set([
+    ...(await Promise.all(
+      maintainers.map(async (id) => (await getUser(id)).login),
+    )),
+    ...owners.filter((handle) => handle && !handle.includes('/')),
+  ])
+  log('reviewers - users', Array.from(users).join(', '))
+
+  const teams = new Set(
+    owners
+      .map((handle) => handle.split('/'))
+      .filter(([org, slug]) => org === context.repo.owner && slug)
+      .map(([, slug]) => slug),
+  )
+  log('reviewers - teams', Array.from(teams).join(', '))
+
+  const team_members = new Set(
+    (await Promise.all(Array.from(teams, getTeamMembers)))
+      .flat(1)
+      .map(({ login }) => login),
+  )
+  log('reviewers - team_members', Array.from(team_members).join(', '))
+
+  const new_reviewers = users
+    .union(team_members)
+    // We can't request a review from the author.
+    .difference(new Set([pull_request.user?.login]))
+  log('reviewers - new_reviewers', Array.from(new_reviewers).join(', '))
+
+  // Filter users to repository collaborators. If they're not, they can't be requested
+  // for review. In that case, they probably missed their invite to the maintainers team.
+  const reviewers = (
+    await Promise.all(
+      Array.from(new_reviewers, async (username) => {
+        try {
+          await github.rest.repos.checkCollaborator({
+            ...context.repo,
+            username,
+          })
+          return username
+        } catch (e) {
+          if (e.status !== 404) throw e
+          core.warn(
+            `PR #${pull_number}: User ${username} cannot be requested for review because they don't exist or are not a repository collaborator, ignoring. They probably missed the automated invite to the maintainers team (see <https://github.com/NixOS/nixpkgs/issues/234293>).`,
+          )
+        }
+      }),
+    )
+  ).filter(Boolean)
+  log('reviewers - reviewers', reviewers.join(', '))
+
+  if (reviewers.length > 15) {
+    log(
+      `Too many reviewers (${reviewers.join(', ')}), skipping review requests.`,
+    )
+    // false indicates, that we do have reviewers and don't need the "needs: reviewers" label.
+    return false
+  }
+
+  const requested_reviewers = new Set(
+    pull_request.requested_reviewers.map(({ login }) => login),
+  )
+  log(
+    'reviewers - requested_reviewers',
+    Array.from(requested_reviewers).join(', '),
+  )
+
+  const existing_reviewers = new Set(
+    reviews.map(({ user }) => user?.login).filter(Boolean),
+  )
+  log(
+    'reviewers - existing_reviewers',
+    Array.from(existing_reviewers).join(', '),
+  )
+
+  const non_requested_reviewers = new Set(reviewers)
+    .difference(requested_reviewers)
+    // We don't want to rerequest reviews from people who already reviewed.
+    .difference(existing_reviewers)
+  log(
+    'reviewers - non_requested_reviewers',
+    Array.from(non_requested_reviewers).join(', '),
+  )
+
+  if (non_requested_reviewers.size === 0) {
+    log('Has reviewer changes', 'false (skipped)')
+  } else if (dry) {
+    core.info(
+      `Requesting reviewers for #${pull_number}: ${Array.from(non_requested_reviewers).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.
+    await github.rest.pulls.requestReviewers({
+      ...context.repo,
+      pull_number,
+      reviewers,
+    })
+  }
+
+  // Return a boolean on whether the "needs: reviewers" label should be set.
+  return (
+    new_reviewers.size === 0 &&
+    existing_reviewers.size === 0 &&
+    requested_reviewers.size === 0
+  )
+}
+
+module.exports = {
+  handleReviewers,
+}

ci/request-reviews/default.nix

@@ -1,33 +0,0 @@
-{
-  lib,
-  stdenvNoCC,
-  makeWrapper,
-  coreutils,
-  jq,
-  github-cli,
-}:
-stdenvNoCC.mkDerivation {
-  name = "request-reviews";
-  src = lib.fileset.toSource {
-    root = ./.;
-    fileset = lib.fileset.unions [
-      ./request-reviewers.sh
-    ];
-  };
-  nativeBuildInputs = [ makeWrapper ];
-  dontBuild = true;
-  installPhase = ''
-    mkdir -p $out/bin
-    for bin in *.sh; do
-      mv "$bin" "$out/bin"
-      wrapProgram "$out/bin/$bin" \
-        --set PATH ${
-          lib.makeBinPath [
-            coreutils
-            jq
-            github-cli
-          ]
-        }
-    done
-  '';
-}

ci/request-reviews/request-reviewers.sh

@@ -1,120 +0,0 @@
-#!/usr/bin/env bash
-
-# Request reviewers for a PR, reading line-separated usernames on stdin,
-# filtering for valid reviewers before using the API endpoint to request reviews:
-# https://docs.github.com/en/rest/pulls/review-requests?apiVersion=2022-11-28#request-reviewers-for-a-pull-request
-
-set -euo pipefail
-
-tmp=$(mktemp -d)
-trap 'rm -rf "$tmp"' exit
-
-log() {
-    echo "$@" >&2
-}
-
-effect() {
-    if [[ -n "${DRY_MODE:-}" ]]; then
-        log "Skipping in dry mode:" "${@@Q}"
-    else
-        "$@"
-    fi
-}
-
-if (( "$#" < 3 )); then
-    log "Usage: $0 BASE_REPO PR_NUMBER PR_AUTHOR"
-    exit 1
-fi
-
-baseRepo=$1
-prNumber=$2
-prAuthor=$3
-
-tmp=$(mktemp -d)
-trap 'rm -rf "$tmp"' exit
-
-# Associative array with the user as the key for easy de-duplication
-# Make sure to always lowercase keys to avoid duplicates with different casings
-declare -A users=()
-while read -r handle && [[ -n "$handle" ]]; do
-    if [[ "$handle" =~ (.*)/(.*) ]]; then
-        # Teams look like $org/$team
-        org=${BASH_REMATCH[1]}
-        team=${BASH_REMATCH[2]}
-
-        # Instead of requesting a review from the team itself,
-        # we request reviews from the individual users.
-        # This is because once somebody from a team reviewed the PR,
-        # the API doesn't expose that the team was already requested for a review,
-        # so we wouldn't be able to avoid rerequesting reviews
-        # without saving some some extra state somewhere
-
-        # We could also consider implementing a more advanced heuristic
-        # in the future that e.g. only pings one team member,
-        # but escalates to somebody else if that member doesn't respond in time.
-        gh api \
-            --cache=1h \
-            -H "Accept: application/vnd.github+json" \
-            -H "X-GitHub-Api-Version: 2022-11-28" \
-            "/orgs/$org/teams/$team/members" \
-            --jq '.[].login' > "$tmp/team-members"
-        readarray -t members < "$tmp/team-members"
-        log "Team $handle has these members: ${members[*]}"
-
-        for user in "${members[@]}"; do
-            users[${user,,}]=
-        done
-    else
-        # Everything else is a user
-        users[${handle,,}]=
-    fi
-done
-
-# Cannot request a review from the author
-if [[ -v users[${prAuthor,,}] ]]; then
-    log "One or more files are owned by the PR author, ignoring"
-    unset 'users[${prAuthor,,}]'
-fi
-
-gh api \
-    -H "Accept: application/vnd.github+json" \
-    -H "X-GitHub-Api-Version: 2022-11-28" \
-    "/repos/$baseRepo/pulls/$prNumber/reviews" \
-    --jq '.[].user.login' > "$tmp/already-reviewed-by"
-
-# And we don't want to rerequest reviews from people who already reviewed
-while read -r user; do
-    if [[ -v users[${user,,}] ]]; then
-        log "User $user is a potential reviewer, but has already left a review, ignoring"
-        unset 'users[${user,,}]'
-    fi
-done < "$tmp/already-reviewed-by"
-
-for user in "${!users[@]}"; do
-    if ! gh api \
-        -H "Accept: application/vnd.github+json" \
-        -H "X-GitHub-Api-Version: 2022-11-28" \
-        "/repos/$baseRepo/collaborators/$user" >&2; then
-        log "User $user is not a repository collaborator, probably missed the automated invite to the maintainers team (see <https://github.com/NixOS/nixpkgs/issues/234293>), ignoring"
-        unset 'users[$user]'
-    fi
-done
-
-if [[ "${#users[@]}" -gt 15 ]]; then
-    log "Too many reviewers (${!users[*]}), skipping review requests"
-    exit 0
-fi
-
-for user in "${!users[@]}"; do
-    log "Requesting review from: $user"
-
-    if ! response=$(jq -n --arg user "$user" '{ reviewers: [ $user ] }' | \
-        effect gh api \
-            --method POST \
-            -H "Accept: application/vnd.github+json" \
-            -H "X-GitHub-Api-Version: 2022-11-28" \
-            "/repos/$baseRepo/pulls/$prNumber/requested_reviewers" \
-            --input -); then
-        log "Failed to request review from $user: $response"
-    fi
-done