[CI] Stop infinite reviewer reassignment, batch state commits, clean up stale state

scripts/ci/pr-bot/README.md

@@ -23,6 +23,33 @@ This directory holds all the code (except for Actions Workflows) for our PR bot
 For a list of commands to use when interacting with the bot, see [Commands.md](./Commands.md).
 For a design doc explaining the design and implementation, see [Automate Reviewer Assignment](https://docs.google.com/document/d/1FhRPRD6VXkYlLAPhNfZB7y2Yese2FCWBzjx67d3TjBo/edit#)
 
+## PR Bot Logic
+
+The bot consists of three core workflows and a persistent state tracking system:
+
+### 1. New PR Processing (`processNewPrs.ts`)
+* Runs periodically on a schedule (every 30 minutes).
+* Checks eligible open PRs (skips WIP, drafts, closed, PRs < 20 minutes old, PRs with notifications silenced, or PRs labeled `awaiting triage`).
+* Once CI checks pass, assigns reviewers based on configured label mappings in `.github/REVIEWERS.yml` (prioritizing least-recently-assigned reviewers).
+* If a non-committer reviewer approves, automatically assigns a committer for final review and merge.
+* Sets `Next Action: Reviewers` label.
+
+### 2. PR Updates & Commands (`processPrUpdate.ts`)
+* Triggered on PR pushes (`synchronize`) and comments (`issue_comment: created`).
+* Shifts attention back to reviewers (`Next Action: Reviewers`) when author pushes new commits or posts comments.
+* Removes `slow-review` label upon receiving a comment from a non-author reviewer.
+* Processes commands like `assign to next reviewer`, `waiting on author`, `stop reviewer notifications`, `assign set of reviewers`, and `remind me after tests pass`.
+
+### 3. Reviewer Reminders & Stale PRs (`findPrsNeedingAttention.ts`)
+* Runs daily to identify PRs needing action.
+* Flags PRs awaiting reviewer response as `slow-review` if inactive for ≥ 7 days (or ≥ 2 weekdays without comments).
+* If still no response after 2 more weekdays, reassigns to new reviewers, removes `slow-review`, and adds `reassigned-reviewers`.
+* **Stale PR Cutoff**: If a PR has both `reassigned-reviewers` and `Next Action: Reviewers` labels and review started > 60 days ago, it stops reviewer assignment loops and adds `awaiting triage`. PRs labeled `awaiting triage` are skipped.
+* **Stale State Cleanup**: Cleans up the oldest 100 state files for PRs that are no longer open to incrementally prune closed PR metadata from the state branch.
+
+### 4. Persistent State (`PersistentState`)
+* Stores PR review progress and label assignment rotations on the `pr-bot-state` Git branch under `state/pr-state/pr-<number>.json` and `state/reviewers-for-label-<label>.json`.
+
 ## Build/Test
 
 To build, run:

scripts/ci/pr-bot/findPrsNeedingAttention.ts

@@ -26,15 +26,13 @@ const {
   REPO,
   PATH_TO_CONFIG_FILE,
   SLOW_REVIEW_LABEL,
+  REASSIGNED_REVIEWERS_LABEL,
+  AWAITING_TRIAGE_LABEL,
+  NEXT_ACTION_REVIEWERS_LABEL,
 } = require("./shared/constants");
+const { hasLabel } = github;
 const ONE_DAY_MS = 24 * 60 * 60 * 1000;
 
-function hasLabel(pull: any, labelName: string): boolean {
-  return pull.labels.some(
-    (label) => label.name.toLowerCase() === labelName.toLowerCase()
-  );
-}
-
 function getTwoWeekdaysAgo(): Date {
   const twoWeekDaysAgo = new Date(Date.now() - 2 * ONE_DAY_MS);
   const currentDay = new Date(Date.now()).getDay();
@@ -49,7 +47,7 @@ function getTwoWeekdaysAgo(): Date {
 }
 
 async function isSlowReview(pull: any): Promise<boolean> {
-  if (!hasLabel(pull, "Next Action: Reviewers")) {
+  if (!hasLabel(pull, NEXT_ACTION_REVIEWERS_LABEL)) {
     return false;
   }
   const lastModified = new Date(pull.updated_at);
@@ -102,15 +100,18 @@ async function assignToNewReviewers(
   let prState = await stateClient.getPrState(pull.number);
   let reviewerStateToUpdate = {};
   const labelObjects = pull.labels;
-  let reviewersToExclude: string[] = Object.values(prState.reviewersAssignedForLabels) as string[];
+  let reviewersToExclude: string[] = Object.values(
+    prState.reviewersAssignedForLabels
+  ) as string[];
   if (pull.requested_reviewers) {
-    reviewersToExclude = reviewersToExclude.concat(pull.requested_reviewers.map((r: any) => r.login));
+    reviewersToExclude = reviewersToExclude.concat(
+      pull.requested_reviewers.map((r: any) => r.login)
+    );
   }
   reviewersToExclude.push(pull.user.login);
   const reviewersForLabels: { [key: string]: string[] } =
     reviewerConfig.getReviewersForLabels(labelObjects, reviewersToExclude);
-  const fallbackReviewers =
-    reviewerConfig.getFallbackReviewers();
+  const fallbackReviewers = reviewerConfig.getFallbackReviewers();
   for (const labelObject of labelObjects) {
     const label = labelObject.name;
     let availableReviewers = reviewersForLabels[label];
@@ -156,6 +157,32 @@ async function processPull(
     console.log(`Skipping PR ${pull.number} - notifications silenced`);
     return;
   }
+  if (hasLabel(pull, AWAITING_TRIAGE_LABEL)) {
+    console.log(`Skipping PR ${pull.number} - awaiting triage`);
+    return;
+  }
+
+  const sixtyDaysAgo = new Date(Date.now() - 60 * ONE_DAY_MS);
+  const initialReviewDate = prState.reviewersAssignedAt
+    ? new Date(prState.reviewersAssignedAt)
+    : new Date(pull.created_at);
+  if (
+    hasLabel(pull, REASSIGNED_REVIEWERS_LABEL) &&
+    hasLabel(pull, NEXT_ACTION_REVIEWERS_LABEL) &&
+    initialReviewDate.getTime() < sixtyDaysAgo.getTime()
+  ) {
+    console.log(
+      `PR ${pull.number} has reassigned-reviewers and Next Action: Reviewers labels and review started >60 days ago - adding awaiting triage label`
+    );
+    await github.getGitHubClient().rest.issues.addLabels({
+      owner: REPO_OWNER,
+      repo: REPO,
+      issue_number: pull.number,
+      labels: [AWAITING_TRIAGE_LABEL],
+    });
+    return;
+  }
+
   if (hasLabel(pull, SLOW_REVIEW_LABEL)) {
     const lastModified = new Date(pull.updated_at);
     const twoWeekDaysAgo = getTwoWeekdaysAgo();
@@ -177,7 +204,7 @@ async function processPull(
         owner: REPO_OWNER,
         repo: REPO,
         issue_number: pull.number,
-        labels: ["reassigned-reviewers"],
+        labels: [REASSIGNED_REVIEWERS_LABEL],
       });
     }
 
@@ -186,9 +213,13 @@ async function processPull(
 
   if (await isSlowReview(pull)) {
     const client = github.getGitHubClient();
-    let reviewersToPing = Object.values(prState.reviewersAssignedForLabels || {});
+    let reviewersToPing = Object.values(
+      prState.reviewersAssignedForLabels || {}
+    );
     if (pull.requested_reviewers) {
-      reviewersToPing = reviewersToPing.concat(pull.requested_reviewers.map((r: any) => r.login));
+      reviewersToPing = reviewersToPing.concat(
+        pull.requested_reviewers.map((r: any) => r.login)
+      );
     }
     reviewersToPing = [...new Set(reviewersToPing as string[])];
 
@@ -226,6 +257,8 @@ async function processOldPrs() {
   for (const pull of openPulls) {
     await processPull(pull, reviewerConfig, stateClient);
   }
+
+  await stateClient.deleteStalePrStates(openPulls, 100);
 }
 
 processOldPrs();

scripts/ci/pr-bot/processNewPrs.ts

@@ -27,6 +27,7 @@ const {
   REPO,
   PATH_TO_CONFIG_FILE,
   REVIEWERS_ACTION,
+  AWAITING_TRIAGE_LABEL,
 } = require("./shared/constants");
 import { CheckStatus } from "./shared/checks";
 
@@ -44,6 +45,12 @@ import { CheckStatus } from "./shared/checks";
  * (in which case that's all we need to do).
  */
 function needsProcessed(pull: any, prState: typeof Pr): boolean {
+  if (github.hasLabel(pull, AWAITING_TRIAGE_LABEL)) {
+    console.log(
+      `Skipping PR ${pull.number} because it has awaiting triage label`
+    );
+    return false;
+  }
   const firstPythonPrToProcess = new Date(2022, 5, 16, 14); // June 16 2022, 14:00 UTC (note that JavaScript months are 0 indexed)
   const firstPrToProcess = new Date(2022, 6, 15, 23); // July 15 2022, 23:00 UTC (note that JavaScript months are 0 indexed)
   const createdAt = new Date(pull.created_at);
@@ -167,7 +174,9 @@ async function approvedBy(pull: any): Promise<string[]> {
 async function isAnyGithubReviewerCommitter(pull: any): Promise<boolean> {
   let reviewers: string[] = [];
   if (pull.requested_reviewers && pull.requested_reviewers.length > 0) {
-    reviewers = reviewers.concat(pull.requested_reviewers.map((r: any) => r.login));
+    reviewers = reviewers.concat(
+      pull.requested_reviewers.map((r: any) => r.login)
+    );
   }
   for (const reviewer of reviewers) {
     if (await github.checkIfCommitter(reviewer)) {
@@ -194,8 +203,8 @@ async function processPull(
       await github.addPrComment(
         pull.number,
         "Closing this PR because dependabot updates for container/** are not allowed due to generated files " +
-        "and excluded_paths is disabled due to dependabot/dependabot-core#14408. " +
-        "Once issue is resolved, please remove this step."
+          "and excluded_paths is disabled due to dependabot/dependabot-core#14408. " +
+          "Once issue is resolved, please remove this step."
       );
       await github.closePr(pull.number);
       return;
@@ -210,8 +219,10 @@ async function processPull(
   console.log(`Processing PR ${pull.number}`);
 
   // If reviewers are already assigned, we just need to check if we should assign a committer.
-  const hasReviewersAssignedForLabels = Object.keys(prState.reviewersAssignedForLabels).length > 0;
-  const hasGithubReviewers = pull.requested_reviewers && pull.requested_reviewers.length > 0;
+  const hasReviewersAssignedForLabels =
+    Object.keys(prState.reviewersAssignedForLabels).length > 0;
+  const hasGithubReviewers =
+    pull.requested_reviewers && pull.requested_reviewers.length > 0;
 
   if (hasReviewersAssignedForLabels || hasGithubReviewers) {
     if (prState.committerAssigned) {
@@ -237,7 +248,11 @@ async function processPull(
       // we can try to guess a label from the PR to assign a committer to.
       if (!labelOfReviewer) {
         let isGithubReviewer = false;
-        if (pull.requested_reviewers && pull.requested_reviewers.some((r: any) => r.login === approver)) isGithubReviewer = true;
+        if (
+          pull.requested_reviewers &&
+          pull.requested_reviewers.some((r: any) => r.login === approver)
+        )
+          isGithubReviewer = true;
 
         if (isGithubReviewer && pull.labels && pull.labels.length > 0) {
           const validLabels = reviewerConfig.getReviewersForAllLabels();
@@ -272,8 +287,7 @@ async function processPull(
         );
         const availableReviewers =
           reviewerConfig.getReviewersForLabel(labelOfReviewer);
-        const fallbackReviewers =
-          reviewerConfig.getFallbackReviewers();
+        const fallbackReviewers = reviewerConfig.getFallbackReviewers();
         const chosenCommitter = await reviewersState.assignNextCommitter(
           availableReviewers,
           fallbackReviewers
@@ -348,6 +362,7 @@ async function processPull(
 
   github.nextActionReviewers(pull.number, pull.labels);
   prState.nextAction = "Reviewers";
+  prState.reviewersAssignedAt = Date.now();
 
   await stateClient.writePrState(pull.number, prState);
   let labelsToUpdate = Object.keys(reviewerStateToUpdate);

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

@@ -26,7 +26,7 @@ export interface AssignReviewerOptions {
 
 // Custom notices for specific labels
 const LABEL_NOTICES: Record<string, string> = {
-  core: "This pull request likely touches a core component (\"core\" label). Please review with scrutiny.",
+  core: 'This pull request likely touches a core component ("core" label). Please review with scrutiny.',
 };
 
 function formatNotices(
@@ -59,8 +59,7 @@ export function assignReviewer(
   labelToReviewerMapping: any,
   options?: AssignReviewerOptions
 ): string {
-  let commentString =
-    "Assigning reviewers:\n\n";
+  let commentString = "Assigning reviewers:\n\n";
 
   for (let label in labelToReviewerMapping) {
     let reviewer = labelToReviewerMapping[label];

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

@@ -31,3 +31,7 @@ export const BOT_NAME = "github-actions";
 export const REVIEWERS_ACTION = "Reviewers";
 export const SLOW_REVIEW_LABEL = "slow-review";
 export const NO_MATCHING_LABEL = "no-matching-label";
+export const REASSIGNED_REVIEWERS_LABEL = "reassigned-reviewers";
+export const AWAITING_TRIAGE_LABEL = "awaiting triage";
+export const NEXT_ACTION_REVIEWERS_LABEL = "Next Action: Reviewers";
+export const PR_STATE_DIR = "state/pr-state";

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

@@ -123,3 +123,11 @@ function removeNextActionLabel(existingLabels: Label[]): string[] {
     )
     .map((label) => label.name);
 }
+
+export function hasLabel(pull: any, labelName: string): boolean {
+  return (pull?.labels || []).some(
+    (label: any) =>
+      (typeof label === "string" ? label : label?.name || "").toLowerCase() ===
+      labelName.toLowerCase()
+  );
+}

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

@@ -21,7 +21,7 @@ const fs = require("fs");
 const path = require("path");
 const { Pr } = require("./pr");
 const { ReviewersForLabel } = require("./reviewersForLabel");
-const { BOT_NAME } = require("./constants");
+const { BOT_NAME, PR_STATE_DIR } = require("./constants");
 
 function getPrFileName(prNumber) {
   return `pr-${prNumber}.json`.toLowerCase();
@@ -41,7 +41,7 @@ async function commitStateToRepo() {
   }
   // Print changes for observability
   await exec.exec("git status", [], { ignoreReturnCode: true });
-  await exec.exec("git add state/*");
+  await exec.exec("git add -A state");
   const changes = await exec.exec(
     "git diff --quiet --cached origin/pr-bot-state state",
     [],
@@ -63,13 +63,13 @@ export class PersistentState {
   // Returns a Pr object representing the current saved state of the pr.
   async getPrState(prNumber: number): Promise<typeof Pr> {
     var fileName = getPrFileName(prNumber);
-    return new Pr(await this.getState(fileName, "state/pr-state"));
+    return new Pr(await this.getState(fileName, PR_STATE_DIR));
   }
 
   // Writes a Pr object representing the current saved state of the pr to persistent storage.
   async writePrState(prNumber: number, newState: any) {
     var fileName = getPrFileName(prNumber);
-    await this.writeState(fileName, "state/pr-state", new Pr(newState));
+    await this.writeState(fileName, PR_STATE_DIR, new Pr(newState));
   }
 
   // Returns a ReviewersForLabel object representing the current saved state of which reviewers have reviewed recently.
@@ -90,6 +90,55 @@ export class PersistentState {
     );
   }
 
+  // Deletes up to maxToDelete state files for PRs that are no longer open, starting from the oldest PRs.
+  async deleteStalePrStates(
+    openPulls: any[],
+    maxToDelete: number = 100
+  ): Promise<number> {
+    if (openPulls.length === 0) {
+      return 0;
+    }
+    await this.ensureCorrectBranch();
+    if (!fs.existsSync(PR_STATE_DIR)) {
+      return 0;
+    }
+    const openPrSet = new Set(openPulls.map((p) => p.number));
+    const files = fs.readdirSync(PR_STATE_DIR);
+    const stalePrs: { prNumber: number; filePath: string }[] = [];
+
+    for (const file of files) {
+      const match = file.match(/^pr-(\d+)\.json$/);
+      if (match) {
+        const prNumber = parseInt(match[1], 10);
+        if (!openPrSet.has(prNumber)) {
+          stalePrs.push({
+            prNumber,
+            filePath: path.join(PR_STATE_DIR, file),
+          });
+        }
+      }
+    }
+
+    // Sort by PR number ascending so the oldest PRs are deleted first
+    stalePrs.sort((a, b) => a.prNumber - b.prNumber);
+
+    const prsToDelete = stalePrs.slice(0, maxToDelete);
+    for (const pr of prsToDelete) {
+      fs.unlinkSync(pr.filePath);
+    }
+
+    if (prsToDelete.length > 0) {
+      console.log(
+        `Deleted ${prsToDelete.length} stale PR state files (oldest: PR ${
+          prsToDelete[0].prNumber
+        }, newest: PR ${prsToDelete[prsToDelete.length - 1].prNumber})`
+      );
+      await commitStateToRepo();
+    }
+
+    return prsToDelete.length;
+  }
+
   private async getState(fileName, baseDirectory) {
     await this.ensureCorrectBranch();
     fileName = path.join(baseDirectory, fileName);
@@ -122,7 +171,7 @@ export class PersistentState {
       await exec.exec(`git config user.name ${BOT_NAME}`);
       await exec.exec(`git config user.email ${BOT_NAME}@github.com`);
       await exec.exec("git config pull.rebase false");
-      await exec.exec("git fetch origin pr-bot-state");
+      await exec.exec("git fetch origin pr-bot-state --depth=1");
       await exec.exec("git checkout pr-bot-state");
     } catch {
       console.log(