[PR Bot] Select single main and backup reviewer using label priority

.github/REVIEWERS.yml

@@ -17,14 +17,18 @@
 # add yourself to that label's exclusionList
 # FallbackReviewers is for reviewers who can review any area of the code base that might
 # not receive a label. These should generally be more experienced committers.
+# Priority determines which label is used to select a reviewer when a PR has multiple labels
+# (lower number = higher priority).
 labels:
   - name: Go
+    priority: 2
     reviewers:
       - jrmccluskey
       - lostluck
       - shunping
     exclusionList: []
   - name: Python
+    priority: 2
     reviewers:
       - claudevdm
       - damccorm
@@ -33,6 +37,7 @@ labels:
       - shunping
     exclusionList: []
   - name: Java
+    priority: 2
     reviewers:
       - Abacn
       - ahmedabu98
@@ -41,35 +46,42 @@ labels:
       - kennknowles
     exclusionList: []
   - name: spanner
+    priority: 1
     reviewers:
       - nielm
   - name: bigtable
+    priority: 1
     reviewers:
       - igorbernstein2
       - mutianf
     exclusionList: []
   - name: delta
+    priority: 1
     reviewers:
       - ahmedabu98
       - chamikaramj
     exclusionList: []
   - name: iceberg
+    priority: 1
     reviewers:
       - ahmedabu98
       - chamikaramj
     exclusionList: []
   - name: yaml
+    priority: 1
     reviewers:
       - chamikaramj
       - derrickaw
   - name: Build
+    priority: 3
     reviewers:
       - Abacn
       - Amar3tto
       - damccorm
       - derrickaw
     exclusionList: []
   - name: website
+    priority: 3
     reviewers:
       - Abacn
       - damccorm

scripts/ci/pr-bot/findPrsNeedingAttention.ts

@@ -112,18 +112,28 @@ async function assignToNewReviewers(
   reviewersToExclude.push(pull.user.login);
   const reviewersForLabels: { [key: string]: string[] } =
     reviewerConfig.getReviewersForLabels(labelObjects, reviewersToExclude);
-  const fallbackReviewers = reviewerConfig.getFallbackReviewers();
-  for (const labelObject of labelObjects) {
-    const label = labelObject.name;
+  const fallbackReviewers =
+    reviewerConfig.getFallbackReviewers(reviewersToExclude);
+  let backupReviewersForLabels: { [key: string]: string } = {};
+  prState.reviewersAssignedForLabels = {};
+  for (const label of Object.keys(reviewersForLabels)) {
     let availableReviewers = reviewersForLabels[label];
     if (availableReviewers && availableReviewers.length > 0) {
       let reviewersState = await stateClient.getReviewersForLabelState(label);
       let chosenReviewer = await reviewersState.assignNextCommitter(
         availableReviewers,
         fallbackReviewers
       );
+      let backupReviewer = reviewersState.getBackupReviewer(
+        availableReviewers,
+        chosenReviewer,
+        fallbackReviewers
+      );
       reviewerStateToUpdate[label] = reviewersState;
       prState.reviewersAssignedForLabels[label] = chosenReviewer;
+      if (backupReviewer) {
+        backupReviewersForLabels[label] = backupReviewer;
+      }
     }
   }
 
@@ -132,6 +142,7 @@ async function assignToNewReviewers(
     pull.number,
     commentStrings.assignNewReviewer(prState.reviewersAssignedForLabels, {
       labels: pull.labels,
+      backupReviewers: backupReviewersForLabels,
     })
   );
   await github.requestPrReviewers(

scripts/ci/pr-bot/processNewPrs.ts

@@ -344,8 +344,11 @@ export async function processPull(
 
   // Pick reviewers to assign. Store them in reviewerStateToUpdate and update the prState object with those reviewers (and their associated labels)
   let reviewerStateToUpdate: { [key: string]: typeof ReviewersForLabel } = {};
+  let backupReviewersForLabels: { [key: string]: string } = {};
+  const exclusionList = [pull.user.login];
   const reviewersForLabels: { [key: string]: string[] } =
-    reviewerConfig.getReviewersForLabels(pull.labels, [pull.user.login]);
+    reviewerConfig.getReviewersForLabels(pull.labels, exclusionList);
+  const fallbackReviewers = reviewerConfig.getFallbackReviewers(exclusionList);
   var labels = Object.keys(reviewersForLabels);
   if (!labels || labels.length === 0) {
     return;
@@ -354,15 +357,24 @@ export async function processPull(
     let availableReviewers = reviewersForLabels[label];
     let reviewersState = await stateClient.getReviewersForLabelState(label);
     let chosenReviewer = reviewersState.assignNextReviewer(availableReviewers);
+    let backupReviewer = reviewersState.getBackupReviewer(
+      availableReviewers,
+      chosenReviewer,
+      fallbackReviewers
+    );
     reviewerStateToUpdate[label] = reviewersState;
     prState.reviewersAssignedForLabels[label] = chosenReviewer;
+    if (backupReviewer) {
+      backupReviewersForLabels[label] = backupReviewer;
+    }
   }
 
   console.log(`Assigning reviewers for PR ${pull.number}`);
   await github.addPrComment(
     pull.number,
     commentStrings.assignReviewer(prState.reviewersAssignedForLabels, {
       labels: pull.labels,
+      backupReviewers: backupReviewersForLabels,
     })
   );
   await github.requestPrReviewers(

scripts/ci/pr-bot/shared/commentStrings.ts

@@ -22,6 +22,7 @@ const { NO_MATCHING_LABEL } = require("./constants");
 export interface AssignReviewerOptions {
   labels?: (string | Label)[];
   notices?: string[];
+  backupReviewers?: { [label: string]: string };
 }
 
 // Custom notices for specific labels
@@ -63,11 +64,15 @@ export function assignReviewer(
 
   for (let label in labelToReviewerMapping) {
     let reviewer = labelToReviewerMapping[label];
+    let backupReviewer = options?.backupReviewers?.[label];
     if (label === NO_MATCHING_LABEL) {
       commentString += `R: @${reviewer} added as fallback since no labels match configuration\n`;
     } else {
       commentString += `R: @${reviewer} for label ${label}.\n`;
     }
+    if (backupReviewer) {
+      commentString += `Backup reviewer: ${backupReviewer} (if the main reviewer is unresponsive, you can escalate to them by tagging their username)\n`;
+    }
   }
 
   commentString += formatNotices(labelToReviewerMapping, options);
@@ -158,11 +163,15 @@ export function assignNewReviewer(
 
   for (const label in labelToReviewerMapping) {
     const reviewer = labelToReviewerMapping[label];
+    const backupReviewer = options?.backupReviewers?.[label];
     if (label === NO_MATCHING_LABEL) {
       commentString += `R: @${reviewer} added as fallback since no labels match configuration\n`;
     } else {
       commentString += `R: @${reviewer} for label ${label}.\n`;
     }
+    if (backupReviewer) {
+      commentString += `Backup reviewer: ${backupReviewer} (if the main reviewer is unresponsive, you can escalate to them by tagging their username)\n`;
+    }
   }
 
   commentString += formatNotices(labelToReviewerMapping, options);

scripts/ci/pr-bot/shared/reviewerConfig.ts

@@ -44,30 +44,52 @@ export class ReviewerConfig {
   }
 
   // Given a list of labels and an exclusion list of reviewers not to include (e.g. the author)
-  // returns all possible reviewers for each label
+  // returns all possible reviewers for the single highest-priority matching label
+  // (lower priority number = higher priority; ties broken randomly).
   getReviewersForLabels(
     labels: Label[],
     exclusionList: string[]
   ): { [key: string]: string[] } {
-    let reviewersFound = false;
-    let labelToReviewerMapping = {};
-    labels.forEach((label) => {
+    let labelToReviewerMapping: { [key: string]: string[] } = {};
+    let bestPriority: number = Number.MAX_SAFE_INTEGER;
+    let bestMatches: { label: string; reviewers: string[] }[] = [];
+
+    for (const label of labels || []) {
       let reviewers = this.getReviewersForLabel(label.name, exclusionList);
       if (reviewers.length > 0) {
-        labelToReviewerMapping[label.name] = reviewers;
-        reviewersFound = true;
+        const priority = this.getPriorityForLabel(label.name);
+        if (bestMatches.length === 0 || priority < bestPriority) {
+          bestPriority = priority;
+          bestMatches = [{ label: label.name, reviewers }];
+        } else if (priority === bestPriority) {
+          bestMatches.push({ label: label.name, reviewers });
+        }
       }
-    });
-    if (!reviewersFound) {
+    }
+    if (bestMatches.length > 0) {
+      const chosenMatch =
+        bestMatches[Math.floor(Math.random() * bestMatches.length)];
+      labelToReviewerMapping[chosenMatch.label] = chosenMatch.reviewers;
+    } else {
       const fallbackReviewers = this.getFallbackReviewers(exclusionList);
       if (fallbackReviewers.length > 0) {
-        labelToReviewerMapping[NO_MATCHING_LABEL] =
-          this.getFallbackReviewers(exclusionList);
+        labelToReviewerMapping[NO_MATCHING_LABEL] = fallbackReviewers;
       }
     }
     return labelToReviewerMapping;
   }
 
+  getPriorityForLabel(label: string): number {
+    const labelObjects = this.config.labels;
+    const labelObject = labelObjects.find(
+      (labelObject) => labelObject.name.toLowerCase() === label.toLowerCase()
+    );
+    if (!labelObject || typeof labelObject.priority !== "number") {
+      return Number.MAX_SAFE_INTEGER;
+    }
+    return labelObject.priority;
+  }
+
   // Get possible reviewers excluding the author.
   getReviewersForLabel(label: string, exclusionList: string[]): string[] {
     const labelObjects = this.config.labels;
@@ -106,6 +128,8 @@ export class ReviewerConfig {
 # add yourself to that label's exclusionList
 # FallbackReviewers is for reviewers who can review any area of the code base that might
 # not receive a label. These should generally be more experienced committers.
+# Priority determines which label is used to select a reviewer when a PR has multiple labels
+# (lower number = higher priority).
 ${yaml.dump(this.config)}`;
 
     fs.writeFileSync(this.configPath, contents, { encoding: "utf-8" });

scripts/ci/pr-bot/shared/reviewersForLabel.ts

@@ -40,16 +40,14 @@ export class ReviewersForLabel {
     }
   }
 
-  // Given a list of available reviewers,
-  // returns the next reviewer up based on who has reviewed least recently.
-  // Updates this object to reflect their assignment.
-  assignNextReviewer(availableReviewers: string[]): string {
-    if (availableReviewers.length === 0) {
-      throw new Error(`No reviewers available for label ${this.label}`);
+  private findLeastRecentlyAssignedReviewer(
+    availableReviewers: string[]
+  ): string {
+    if (!availableReviewers || availableReviewers.length === 0) {
+      return "";
     }
 
     if (!this.dateOfLastReviewAssignment[availableReviewers[0]]) {
-      this.dateOfLastReviewAssignment[availableReviewers[0]] = Date.now();
       return availableReviewers[0];
     }
 
@@ -59,7 +57,6 @@ export class ReviewersForLabel {
     for (let i = 0; i < availableReviewers.length; i++) {
       let availableReviewer = availableReviewers[i];
       if (!this.dateOfLastReviewAssignment[availableReviewer]) {
-        this.dateOfLastReviewAssignment[availableReviewer] = Date.now();
         return availableReviewer;
       }
       if (earliestDate > this.dateOfLastReviewAssignment[availableReviewer]) {
@@ -68,10 +65,44 @@ export class ReviewersForLabel {
       }
     }
 
-    this.dateOfLastReviewAssignment[earliestReviewer] = Date.now();
     return earliestReviewer;
   }
 
+  // Given a list of available reviewers,
+  // returns the next reviewer up based on who has reviewed least recently.
+  // Updates this object to reflect their assignment.
+  assignNextReviewer(availableReviewers: string[]): string {
+    if (availableReviewers.length === 0) {
+      throw new Error(`No reviewers available for label ${this.label}`);
+    }
+
+    const chosenReviewer =
+      this.findLeastRecentlyAssignedReviewer(availableReviewers);
+    this.dateOfLastReviewAssignment[chosenReviewer] = Date.now();
+    return chosenReviewer;
+  }
+
+  // Returns a backup reviewer from the same label (excluding the main reviewer)
+  // without updating dateOfLastReviewAssignment. Falls back to fallbackReviewers
+  // if the label has no other eligible reviewers.
+  getBackupReviewer(
+    availableReviewers: string[],
+    mainReviewer: string,
+    fallbackReviewers: string[] = []
+  ): string {
+    const remainingLabelReviewers = (availableReviewers || []).filter(
+      (reviewer) => reviewer !== mainReviewer
+    );
+    if (remainingLabelReviewers.length > 0) {
+      return this.findLeastRecentlyAssignedReviewer(remainingLabelReviewers);
+    }
+
+    const remainingFallbackReviewers = (fallbackReviewers || []).filter(
+      (reviewer) => reviewer !== mainReviewer
+    );
+    return this.findLeastRecentlyAssignedReviewer(remainingFallbackReviewers);
+  }
+
   // Given the up to date list of available reviewers (excluding the author),
   // returns the next reviewer up based on who has reviewed least recently.
   // Updates this object to reflect their assignment.