Update pytorchbot comment to have a PR status section

torchci/lib/bot/index.ts

@@ -10,6 +10,7 @@ import crcrOncallBot from "./crcrOncallBot";
 import drciBot from "./drciBot";
 import logUploader from "./logUploader";
 import nitpickBot from "./nitpickBot";
+import prStatusBot from "./prStatusBot";
 import pytorchBot from "./pytorchBot";
 import retryBot from "./retryBot";
 import stripApprovalBot from "./stripApprovalBot";
@@ -28,6 +29,7 @@ export default function bot(app: Probot) {
   drciBot(app);
   logUploader(app);
   nitpickBot(app);
+  prStatusBot(app);
   pytorchBot(app);
   retryBot(app);
   stripApprovalBot(app);

torchci/lib/bot/prStatusBot.ts

@@ -0,0 +1,101 @@
+// Keeps the PR Status section of the Dr.CI comment current between sweeps.
+//
+// The section is rendered by the Dr.CI sweep like every other part of the
+// comment, but the sweep is woken by CI activity and runs at most every 15
+// minutes -- so without this a contributor whose PR was just triaged reads a
+// stale stage, or none at all, for a quarter of an hour after the event that
+// changed it. The label and review webhooks are exactly the events that move a
+// PR between stages, so they poke the section directly.
+//
+// It splices the section into the existing comment in place rather than
+// rebuilding it: a rebuild here would have no CI results to put back and would
+// blank out everything the sweep rendered.
+
+import { upsertPrStatusSection } from "lib/drciUtils";
+import {
+  hasPrStatusLabel,
+  PR_STATUS_LABEL_TRIAGED,
+  PR_STATUS_LABELS,
+} from "lib/prStatus";
+import { Context, Probot } from "probot";
+// isDrCIEnabled already calls isPyTorchbotSupportedOrg, so it is the only gate
+// needed here.
+import { isDrCIEnabled } from "./utils";
+
+async function handle(
+  context: Context<"pull_request" | "pull_request_review">
+) {
+  const owner = context.payload.repository.owner.login;
+  const repo = context.payload.repository.name;
+  if (!isDrCIEnabled(owner, repo)) {
+    context.log(`${__filename} isn't enabled on ${owner}/${repo}`);
+    return;
+  }
+
+  const pullRequest = context.payload.pull_request;
+  if (pullRequest.state !== "open") {
+    return;
+  }
+
+  const labels = pullRequest.labels.map((label) => label.name);
+
+  // pytorch/pytorch churns ciflow/*, module:* and friends constantly, and every
+  // one of these events would otherwise cost at least a listComments on the
+  // shared installation token. Only the events that can actually change what
+  // this section renders are worth a request.
+  const payload = context.payload as any;
+  if (
+    (payload.action === "labeled" || payload.action === "unlabeled") &&
+    payload.label
+  ) {
+    // A label object is present, so this is a specific label going on or off
+    // one PR and can be judged directly.
+    if (!PR_STATUS_LABELS.includes(payload.label.name)) {
+      return;
+    }
+  } else if (
+    payload.action === "review_requested" ||
+    payload.action === "review_request_removed"
+  ) {
+    // The reviewer list is only named by the pre-review message.
+    if (!labels.includes(PR_STATUS_LABEL_TRIAGED)) {
+      return;
+    }
+  } else if (!hasPrStatusLabel(labels)) {
+    // A review on a PR outside the workflow -- or an `unlabeled` with no label
+    // object, which GitHub sends when a label is deleted repo-wide rather than
+    // removed from one PR. Either way the section could only render empty, and
+    // a stale one cannot be present on a PR whose labels say it is out of the
+    // workflow. Returning here is what keeps every review comment on every open
+    // PR in every Dr.CI repo from costing a listComments.
+    return;
+  }
+
+  await upsertPrStatusSection(
+    context.octokit as any,
+    owner,
+    repo,
+    pullRequest.number,
+    labels,
+    pullRequest.user?.login
+  );
+}
+
+export default function prStatusBot(app: Probot): void {
+  app.on(
+    [
+      // Move between stages.
+      "pull_request.labeled",
+      "pull_request.unlabeled",
+      // Change the assigned-reviewer list the pre-review message names. The spec
+      // requires it stay up to date with removals and additions, and a sweep
+      // that only CI activity wakes cannot promise that.
+      "pull_request.review_requested",
+      "pull_request.review_request_removed",
+      // Approval outranks the labels, so it changes the stage on its own.
+      "pull_request_review.submitted",
+      "pull_request_review.dismissed",
+    ],
+    handle
+  );
+}

torchci/lib/bot/pytorchBotHandler.ts

@@ -4,6 +4,13 @@ import { updateDrciComments } from "pages/api/drci/drci";
 import shlex from "shlex";
 import { queryClickhouseSaved } from "../clickhouse";
 import { fetchCrcrAllowlist } from "../crcrAllowlist";
+// lib/reviewApproval owns the approval rules, so the merge command and the
+// Dr.CI PR Status line cannot disagree about whether a PR is approved.
+import {
+  getApprovalStatusFromReviews,
+  PR_APPROVED,
+  PR_CHANGES_REQUESTED,
+} from "../reviewApproval";
 import { getHelp, getParser } from "./cliParser";
 import { cherryPickClassifications } from "./Constants";
 import { downstreamRepoFromCheckRunName } from "./crcrOncallBot";
@@ -35,11 +42,6 @@ export interface PytorchbotParams {
   cachedConfigTracker: CachedConfigTracker;
 }
 
-const PR_COMMENTED = "commented";
-const PR_DISMISSED = "dismissed";
-const PR_CHANGES_REQUESTED = "changes_requested";
-const PR_APPROVED = "approved";
-
 class PytorchBotHandler {
   ctx: any;
   useReactions: boolean;
@@ -165,7 +167,7 @@ The explanation needs to be clear on why this is needed. Here are some good exam
   }
 
   async getApprovalStatus(): Promise<string> {
-    var reviews: PullRequestReview[] = await this.ctx.octokit.paginate(
+    const reviews: PullRequestReview[] = await this.ctx.octokit.paginate(
       this.ctx.octokit.pulls.listReviews,
       {
         owner: this.owner,
@@ -180,91 +182,13 @@ The explanation needs to be clear on why this is needed. Here are some good exam
       return "no_reviews";
     }
 
-    // From https://docs.github.com/en/graphql/reference/enums#commentauthorassociation
-    const ALLOWED_APPROVER_ASSOCIATIONS = [
-      "COLLABORATOR",
-      "CONTRIBUTOR",
-      "MEMBER",
-      "OWNER",
-    ];
-
-    // GitHub App bots authenticate via installations rather than as repo
-    // collaborators, so their reviews always carry author_association=NONE.
-    // Allowlist trusted App identities so their approvals are still honored,
-    // but only on pytorch/pytorch since that is the sole repo these bots
-    // review -- other supported orgs/repos must not honor the exemption.
-    const ALLOWED_APPROVER_BOT_LOGINS = ["pytorchgreenlight[bot]"];
-    const isPyTorchPyTorchRepo = isPyTorchPyTorch(this.owner, this.repo);
-
-    // Find the latest review offered by each authroized reviewer
-    // But first sort them in case Github ever returns the list unsorted
-    var latest_reviews: { [user: string]: string } = reviews
-      .sort((a: PullRequestReview, b: PullRequestReview) => {
-        return Date.parse(a.submitted_at + "") < Date.parse(b.submitted_at + "")
-          ? -1
-          : 1;
-      })
-      .reduce(
-        (
-          latest_reviews: { [user: string]: string },
-          curr_review: PullRequestReview
-        ) => {
-          if (
-            !ALLOWED_APPROVER_ASSOCIATIONS.includes(
-              curr_review.author_association
-            ) &&
-            !(
-              isPyTorchPyTorchRepo &&
-              ALLOWED_APPROVER_BOT_LOGINS.includes(
-                curr_review.user?.login ?? ""
-              )
-            )
-          ) {
-            // Not an authorized approver
-            return latest_reviews;
-          }
-
-          // Casing is werid here. The typescript defintion says state will be lower case, yet github
-          // returns upper case. We can't trust that to remain that way, so always conver the state
-          // to lowercase before any comparisons
-          switch (curr_review.state.toLocaleLowerCase()) {
-            case PR_COMMENTED: // Ignore mere comments
-              break;
-            case PR_DISMISSED: // Ignore previous reviews by this person
-              delete latest_reviews[curr_review.user.login];
-              break;
-            case PR_CHANGES_REQUESTED:
-              latest_reviews[curr_review.user.login] = curr_review.state;
-              break;
-            case PR_APPROVED:
-              latest_reviews[curr_review.user.login] = curr_review.state;
-              break;
-            default:
-              this.ctx.log(
-                `Found an invalid review state '${curr_review.state}' on review id ${curr_review.id}. See ${curr_review.html_url}`
-              );
-          }
-
-          return latest_reviews;
-        },
-        {}
-      );
-
-    // Aggregate the reviews to figure out the overall status.
-    // One approval is all that's needed
-    // If there are any changes requested, the status is changes requested
-    let approval_status = "";
-    for (let [_, review_state] of Object.entries(latest_reviews)) {
-      if (review_state.toLocaleLowerCase() == PR_APPROVED) {
-        approval_status = review_state;
-      } else if (review_state.toLocaleLowerCase() == PR_CHANGES_REQUESTED) {
-        // If there are any changes requested, we exit early and just return changes requested
-        approval_status = review_state;
-        break;
-      }
-    }
-
-    return approval_status.toLocaleLowerCase();
+    // The rules themselves live in lib/reviewApproval so the Dr.CI PR Status
+    // line reaches the same verdict this does; see the note there.
+    return getApprovalStatusFromReviews(
+      reviews,
+      isPyTorchPyTorch(this.owner, this.repo),
+      (message: string) => this.ctx.log(message)
+    );
   }
 
   async handleMerge(

torchci/lib/drciUtils.ts

@@ -7,6 +7,13 @@ import {
   isFailureFromPrevMergeCommit,
   isSameFailure,
 } from "lib/jobUtils";
+import {
+  extractPrStatusSection,
+  fetchPrStatusState,
+  hasPrStatusLabel,
+  renderPrStatusSection,
+  splicePrStatusSection,
+} from "lib/prStatus";
 import { MAX_SIZE, OLDEST_FIRST, querySimilarFailures } from "lib/searchUtils";
 import { RecentWorkflowsData } from "lib/types";
 import _ from "lodash";
@@ -104,10 +111,15 @@ export function formDrciComment(
   owner: string = OWNER,
   repo: string = REPO,
   pr_results: string = "",
-  sevs: string = ""
+  sevs: string = "",
+  // Pre-rendered PR Status section, carrying its own delimiters and trailing
+  // newline (empty unless the PR is in the contributor workflow). It leads the
+  // comment: it is the one line telling the contributor what stage the PR is at
+  // and who owes the next step, so it must not sit below the CI results.
+  prStatusSection: string = ""
 ): string {
   const header = formDrciHeader(owner, repo, pr_num);
-  const comment = `${DRCI_COMMENT_START}
+  const comment = `${DRCI_COMMENT_START}${prStatusSection}
 ${header}
 ${sevs}
 ${pr_results}
@@ -214,12 +226,19 @@ export async function upsertDrCiComment(
   const sev = getActiveSEVs(
     await fetchIssuesByLabel("ci: sev", /*cache*/ true)
   );
+  // This render has no status inputs of its own -- it runs on open/synchronize,
+  // neither of which can change the stage -- so the existing section is carried
+  // across verbatim. Rebuilding without it would delete the status line on every
+  // push and leave it gone until the next sweep, which would undo the whole
+  // point of the section being webhook-maintained. Resetting the CI results the
+  // same way is long-standing behaviour and is left alone.
   const drciComment = formDrciComment(
     prNum,
     owner,
     repo,
     "",
-    formDrciSevBody(sev)
+    formDrciSevBody(sev),
+    extractPrStatusSection(existingDrciComment)
   );
 
   if (existingDrciComment === drciComment) {
@@ -253,6 +272,64 @@ export async function upsertDrCiComment(
   }
 }
 
+/**
+ * Refresh only the PR Status section of an existing Dr.CI comment, leaving the
+ * rest of the body -- above all the CI results the sweep rendered -- untouched.
+ *
+ * Called from the label and review webhooks, which fire between sweeps and have
+ * no CI classification of their own to render. A PR with no Dr.CI comment yet is
+ * a no-op: creating a resultless one here would race the sweep that is about to
+ * write the real thing.
+ */
+export async function upsertPrStatusSection(
+  octokit: Octokit,
+  owner: string,
+  repo: string,
+  prNum: number,
+  labels: string[],
+  // The PR author, so they are never listed as a reviewer of their own PR even
+  // if the reviewer read degrades. The webhook payload always carries it.
+  authorLogin?: string
+) {
+  if (!isDrCIEnabled(owner, repo)) {
+    return;
+  }
+
+  const { id, body } = await getDrciComment(octokit, owner, repo, prNum);
+  if (id === 0) {
+    return;
+  }
+
+  // An unlabelled PR renders an empty section, which the splice uses to REMOVE
+  // a stale one -- so this cannot be short-circuited on "no status label" the
+  // way the sweep's render is. The GitHub reads behind the state are still
+  // skipped in that case, since an empty section needs no inputs.
+  const section = hasPrStatusLabel(labels)
+    ? renderPrStatusSection(
+        await fetchPrStatusState(
+          octokit,
+          owner,
+          repo,
+          prNum,
+          labels,
+          authorLogin
+        )
+      )
+    : "";
+
+  const updated = splicePrStatusSection(body, section, DRCI_COMMENT_START);
+  if (updated === body) {
+    return;
+  }
+
+  await octokit.rest.issues.updateComment({
+    body: updated,
+    owner,
+    repo,
+    comment_id: id,
+  });
+}
+
 export async function hasSimilarFailures(
   job: RecentWorkflowsData,
   baseCommitDate: string,