fix: preserve reviewer routing fallback

.github/reviewer-routing.js

@@ -185,6 +185,58 @@ function addReviewer(reviewers, reviewer) {
   }
 }
 
+function reviewerCandidates({preferredReviewers, fallbackReviewers, eligibleReviewers}) {
+  const eligible = new Set(eligibleReviewers.map((reviewer) => reviewer.toLowerCase()));
+  const candidates = [];
+  for (const reviewer of [...preferredReviewers, ...fallbackReviewers]) {
+    if (
+      eligible.has(reviewer.toLowerCase()) &&
+      !candidates.some((candidate) => candidate.toLowerCase() === reviewer.toLowerCase())
+    ) {
+      candidates.push(reviewer);
+    }
+  }
+  return candidates;
+}
+
+async function requestReviewersWithFallback({
+  candidates,
+  requiredReviewers,
+  satisfiedReviewers = [],
+  requestReviewer,
+  onFailure = () => {},
+}) {
+  const alreadySatisfied = new Set(
+    satisfiedReviewers.map((reviewer) => reviewer.toLowerCase()),
+  );
+  const satisfied = new Set();
+  const requested = [];
+
+  for (const reviewer of candidates) {
+    if (satisfied.size >= requiredReviewers) {
+      break;
+    }
+    const normalizedReviewer = reviewer.toLowerCase();
+    if (alreadySatisfied.has(normalizedReviewer)) {
+      satisfied.add(normalizedReviewer);
+      continue;
+    }
+
+    try {
+      const shouldContinue = await requestReviewer(reviewer);
+      if (shouldContinue === false) {
+        return {requested, satisfiedReviewers: [...satisfied], aborted: true};
+      }
+      requested.push(reviewer);
+      satisfied.add(normalizedReviewer);
+    } catch (error) {
+      onFailure(reviewer, error);
+    }
+  }
+
+  return {requested, satisfiedReviewers: [...satisfied], aborted: false};
+}
+
 function resolveReviewRouting({files, author, latestPusher, fallbackReviewers = REVIEWER_POOL}) {
   const modules = classifyFiles(files);
   const unavailable = new Set([author, latestPusher].filter(Boolean).map((login) => login.toLowerCase()));
@@ -224,4 +276,10 @@ function resolveReviewRouting({files, author, latestPusher, fallbackReviewers =
   };
 }
 
-module.exports = {REVIEWER_POOL, classifyFiles, resolveReviewRouting};
+module.exports = {
+  REVIEWER_POOL,
+  classifyFiles,
+  requestReviewersWithFallback,
+  resolveReviewRouting,
+  reviewerCandidates,
+};

.github/reviewer-routing.test.js

@@ -1,7 +1,11 @@
 'use strict';
 
 const assert = require('node:assert/strict');
-const {resolveReviewRouting} = require('./reviewer-routing');
+const {
+  requestReviewersWithFallback,
+  resolveReviewRouting,
+  reviewerCandidates,
+} = require('./reviewer-routing');
 
 function route(files, author = 'author', latestPusher = author) {
   return resolveReviewRouting({files: files.map((filename) => ({filename})), author, latestPusher});
@@ -70,4 +74,75 @@ function route(files, author = 'author', latestPusher = author) {
   assert.equal(result.reason, 'unknown_paths');
 }
 
-console.log('reviewer routing policy tests passed');
+async function testSingleReviewerFallback() {
+  const candidates = reviewerCandidates({
+    preferredReviewers: ['wxianfeng'],
+    fallbackReviewers: ['wxianfeng', 'typefield', 'haofeng0705'],
+    eligibleReviewers: ['wxianfeng', 'typefield', 'haofeng0705'],
+  });
+  const attempts = [];
+  const result = await requestReviewersWithFallback({
+    candidates,
+    requiredReviewers: 1,
+    requestReviewer: async (reviewer) => {
+      attempts.push(reviewer);
+      if (reviewer === 'wxianfeng') {
+        throw Object.assign(new Error('cannot request primary'), {status: 422});
+      }
+      return true;
+    },
+  });
+  assert.deepEqual(attempts, ['wxianfeng', 'typefield']);
+  assert.deepEqual(result.requested, ['typefield']);
+  assert.equal(result.satisfiedReviewers.length, 1);
+}
+
+async function testTwoReviewerFallback() {
+  const candidates = reviewerCandidates({
+    preferredReviewers: ['haofeng0705', 'wxianfeng'],
+    fallbackReviewers: ['haofeng0705', 'wxianfeng', 'typefield', 'hlzjsong'],
+    eligibleReviewers: ['haofeng0705', 'wxianfeng', 'typefield', 'hlzjsong'],
+  });
+  const attempts = [];
+  const result = await requestReviewersWithFallback({
+    candidates,
+    requiredReviewers: 2,
+    requestReviewer: async (reviewer) => {
+      attempts.push(reviewer);
+      if (reviewer === 'wxianfeng') {
+        throw Object.assign(new Error('temporary failure'), {status: 503});
+      }
+      return true;
+    },
+  });
+  assert.deepEqual(attempts, ['haofeng0705', 'wxianfeng', 'typefield']);
+  assert.deepEqual(result.requested, ['haofeng0705', 'typefield']);
+  assert.equal(result.satisfiedReviewers.length, 2);
+}
+
+async function testLowerPriorityExistingRequestDoesNotReplaceOwner() {
+  const attempts = [];
+  const result = await requestReviewersWithFallback({
+    candidates: ['wxianfeng', 'typefield'],
+    requiredReviewers: 1,
+    satisfiedReviewers: ['typefield'],
+    requestReviewer: async (reviewer) => {
+      attempts.push(reviewer);
+      return true;
+    },
+  });
+  assert.deepEqual(attempts, ['wxianfeng']);
+  assert.deepEqual(result.requested, ['wxianfeng']);
+  assert.deepEqual(result.satisfiedReviewers, ['wxianfeng']);
+}
+
+Promise.all([
+  testSingleReviewerFallback(),
+  testTwoReviewerFallback(),
+  testLowerPriorityExistingRequestDoesNotReplaceOwner(),
+])
+  .then(() => console.log('reviewer routing policy tests passed'))
+  .catch((error) => {
+    console.error(error);
+    process.exitCode = 1;
+  });

.github/workflows/reviewer-router.yml

@@ -22,9 +22,10 @@ jobs:
     timeout-minutes: 5
     steps:
       - name: Check out trusted routing policy
-        uses: actions/checkout@v4
+        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
         with:
           ref: ${{ github.event.pull_request.base.sha }}
+          persist-credentials: false
       - name: Route review and enable auto-merge
         uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0
         with:
@@ -33,7 +34,12 @@ jobs:
             const repo = context.repo.repo;
             const pullNumber = context.payload.pull_request.number;
             const eventHeadSha = context.payload.pull_request.head.sha;
-            const {REVIEWER_POOL, resolveReviewRouting} = require('./.github/reviewer-routing.js');
+            const {
+              REVIEWER_POOL,
+              requestReviewersWithFallback,
+              resolveReviewRouting,
+              reviewerCandidates,
+            } = require('./.github/reviewer-routing.js');
             const reviewerPool = REVIEWER_POOL;
 
             async function getReadyEventPull(phase) {
@@ -191,21 +197,12 @@ jobs:
                 latestPusher,
                 fallbackReviewers: ranked,
               });
-              const desiredReviewers = [];
-              for (const reviewer of [...routing.reviewers, ...ranked]) {
-                if (
-                  desiredReviewers.length < routing.requiredReviewers &&
-                  eligible.some((candidate) => candidate.toLowerCase() === reviewer.toLowerCase()) &&
-                  !desiredReviewers.some((candidate) => candidate.toLowerCase() === reviewer.toLowerCase())
-                ) {
-                  desiredReviewers.push(reviewer);
-                }
-              }
-              const reviewersToRequest = desiredReviewers.filter(
-                reviewer =>
-                  !currentHeadReviewers.has(reviewer.toLowerCase()) &&
-                  !existingRequestedReviewers.has(reviewer.toLowerCase()),
-              );
+              const candidates = reviewerCandidates({
+                preferredReviewers: routing.reviewers,
+                fallbackReviewers: ranked,
+                eligibleReviewers: eligible,
+              });
+              const desiredReviewers = candidates.slice(0, routing.requiredReviewers);
               if (routing.reason === 'unknown_paths' && currentHeadReviewers.size > 0) {
                 core.info(
                   `PR #${pullNumber} has a current-head review for unknown paths; leaving manual ownership unchanged.`,
@@ -216,12 +213,20 @@ jobs:
                 `PR #${pullNumber} routing: ${routing.reason}; modules=${routing.modules.map(module => module.id).join(',') || 'unknown'}; reviewers=${desiredReviewers.join(',') || 'load-balanced fallback'}.`,
               );
 
-              const requestedByRouter = new Set();
-              for (const reviewer of reviewersToRequest) {
-                try {
+              const satisfiedReviewers = new Set([
+                ...currentHeadReviewers,
+                ...[...existingRequestedReviewers].filter((reviewer) =>
+                  candidates.some((candidate) => candidate.toLowerCase() === reviewer),
+                ),
+              ]);
+              const requestResult = await requestReviewersWithFallback({
+                candidates,
+                requiredReviewers: routing.requiredReviewers,
+                satisfiedReviewers: [...satisfiedReviewers],
+                requestReviewer: async (reviewer) => {
                   const currentPull = await getReadyEventPull('review request');
                   if (!currentPull) {
-                    return;
+                    return false;
                   }
                   await github.rest.pulls.requestReviewers({
                     owner,
@@ -229,19 +234,25 @@ jobs:
                     pull_number: pullNumber,
                     reviewers: [reviewer],
                   });
-                  requestedByRouter.add(reviewer.toLowerCase());
                   core.info(
                     `Requested @${reviewer} for PR #${pullNumber} (open request load: ${loads.get(reviewer)}).`,
                   );
-                } catch (error) {
+                  return true;
+                },
+                onFailure: (reviewer, error) => {
                   core.warning(
                     `Could not request @${reviewer} for PR #${pullNumber}; trying the next candidate (${error.status || 'unknown status'}).`,
                   );
-                }
+                },
+              });
+              if (requestResult.aborted) {
+                return;
               }
 
-              if (requestedByRouter.size === 0) {
-                core.warning(`No reviewer request could be created for PR #${pullNumber}.`);
+              if (requestResult.satisfiedReviewers.length < routing.requiredReviewers) {
+                core.warning(
+                  `Only ${requestResult.satisfiedReviewers.length} of ${routing.requiredReviewers} required reviewers could be satisfied for PR #${pullNumber}.`,
+                );
               }
             }