refactor(triage): decide whether a pull request may merge in one place (CL-12170)

packages/bitbucket-datacenter-tool/src/action.ts

@@ -1,5 +1,5 @@
-import { ACTION_KINDS, type ActionKind, type ResolvedTarget } from "@corbits/triage-contracts";
-import { ciRefusal, mergeableCi, summarizeChecks } from "@corbits/github-reads";
+import { ACTION_KINDS, mergeGate, type ActionKind, type ResolvedTarget } from "@corbits/triage-contracts";
+import { summarizeChecks } from "@corbits/github-reads";
 import { defaultReviewers, getChecks, unresolvedThreadsHold } from "@corbits/bitbucket-datacenter-reads";
 import { type } from "arktype";
 import {
@@ -149,12 +149,10 @@ export async function executeAction(bb: BitbucketFetch, { request, step }: Actio
     case "merge": {
       onHead(pr.sha, request, step);
       const ci = summarizeChecks(await getChecks(bb, step.headSha));
-      if (!mergeableCi(ci, "ciOptional" in target)) throw new ActionNotRunnableError(ciRefusal(ci));
       const me = await user();
-      const requested = pr.reviewers.filter((r) => r.status === "NEEDS_WORK" && r.user.id !== me.id).map((r) => r.user.slug);
-      if (requested.length) throw new ActionNotRunnableError(`changes are requested by ${requested.join(", ")}`);
-      const threads = await unresolvedThreadsHold(bb, repo, number, me.slug);
-      if (threads !== undefined) throw new ActionNotRunnableError(threads);
+      const changesRequestedBy = pr.reviewers.filter((r) => r.status === "NEEDS_WORK" && r.user.id !== me.id).map((r) => r.user.slug);
+      const gate = mergeGate({ ci, ciOptional: "ciOptional" in target, changesRequestedBy, threadsHold: await unresolvedThreadsHold(bb, repo, number, me.slug) });
+      if (!gate.allowed) throw new ActionNotRunnableError(gate.reason);
       // Triage's own needs-work is its verdict on this head, which now merges; a "no needs work" merge check would veto it.
       if (statusOf(pr, me) === "NEEDS_WORK") await setStatus(bb, { repo, number, slug: me.slug, status: "UNAPPROVED" });
       const veto = await mergeVeto(bb, repo, number);

packages/github-reads/src/index.ts

@@ -2,6 +2,7 @@ import { type } from "arktype";
 import { minimatch } from "minimatch";
 import { asRecord, cachedLoads, compare, errorText } from "@corbits/common";
 import { parseFindingHeadline, threadsHold, threadsUnread } from "@corbits/triage-contracts/code-review";
+import type { HeadCi } from "@corbits/triage-contracts/merge-gate";
 import { contributionRuleRank, isDocPath } from "@corbits/triage-contracts/paths";
 import { teamSlug } from "@corbits/triage-contracts/team";
 
@@ -368,33 +369,14 @@ export function checkCancelled(run: CheckRun): boolean {
   return run.status === "completed" && CANCELLED.has(run.conclusion ?? "");
 }
 
-export type CiState = "success" | "failure" | "pending" | "cancelled" | "none";
-
-export function summarizeChecks(runs: CheckRun[]): CiState {
+export function summarizeChecks(runs: CheckRun[]): HeadCi {
   if (!runs.length) return "none";
   if (runs.some(checkFailed)) return "failure";
   if (runs.some(checkPending)) return "pending";
   if (runs.some(checkCancelled)) return "cancelled";
   return "success";
 }
 
-/** A head may merge once its CI passed, or with no CI at all when the policy does not require it. */
-export function mergeableCi(ci: CiState, ciOptional: boolean): boolean {
-  return ci === "success" || (ci === "none" && ciOptional);
-}
-
-const CI_TEXT: Record<CiState, string> = {
-  success: "CI passed on this head",
-  failure: "CI has failed on this head",
-  pending: "CI has not finished on this head",
-  cancelled: "CI was cancelled on this head",
-  none: "There is no CI on this head",
-};
-
-export function ciRefusal(ci: CiState): string {
-  return CI_TEXT[ci];
-}
-
 /** Each reviewer's latest decisive review state; triage's own requests for changes are its verdict, not a reviewer's. */
 export function latestDecisive(reviews: Review[]): Map<string, string> {
   const latest = new Map<string, string>();

packages/github-tool/src/do.ts

@@ -1,15 +1,13 @@
 import { errorText } from "@corbits/common";
-import { ACTION_KINDS, type ActionKind, type ResolvedTarget } from "@corbits/triage-contracts";
+import { ACTION_KINDS, mergeGate, type ActionKind, type ResolvedTarget } from "@corbits/triage-contracts";
 import { type } from "arktype";
 import { checkRepo } from "@corbits/repo-read-tool/host-reads";
 import {
-  ciRefusal,
   codeownersForPr,
   getChecks,
   getReviews,
   latestDecisive,
   listIssueComments,
-  mergeableCi,
   missingFrom,
   readPr,
   REQUEST_CHANGES_DO_MARKER,
@@ -179,12 +177,10 @@ export async function executeDo(gh: GithubFetch, { request, step }: DoInput, log
     case "merge": {
       onHead(pr.sha, request, step);
       const ci = summarizeChecks(await getChecks(gh, repo, step.headSha));
-      if (!mergeableCi(ci, "ciOptional" in target)) throw new DoNotRunnableError(ciRefusal(ci));
       const reviews = await getReviews(gh, repo, number, login);
-      const requested = [...latestDecisive(reviews)].filter(([, state]) => state === "CHANGES_REQUESTED").map(([reviewer]) => reviewer);
-      if (requested.length) throw new DoNotRunnableError(`changes are requested by ${requested.join(", ")}`);
-      const threads = await unresolvedThreadsHold(gh, repo, number, login);
-      if (threads !== undefined) throw new DoNotRunnableError(threads);
+      const changesRequestedBy = [...latestDecisive(reviews)].filter(([, state]) => state === "CHANGES_REQUESTED").map(([reviewer]) => reviewer);
+      const gate = mergeGate({ ci, ciOptional: "ciOptional" in target, changesRequestedBy, threadsHold: await unresolvedThreadsHold(gh, repo, number, login) });
+      if (!gate.allowed) throw new DoNotRunnableError(gate.reason);
       // Execute runs before the mirror, and a protected branch refuses the merge while triage's own request on this head stands; one on an earlier head is the mirror's to take back.
       const ownRequest = reviews.some((review) => review.triageRequest && review.state === "CHANGES_REQUESTED" && review.commitId === step.headSha);
       const kept = ownRequest ? await takeBackOwnRequest(gh, { repo, number, login, sha: step.headSha }) : undefined;

packages/triage-contracts/package.json

@@ -7,6 +7,7 @@
   "exports": {
     ".": "./src/index.ts",
     "./code-review": "./src/code-review.ts",
+    "./merge-gate": "./src/merge-gate.ts",
     "./paths": "./src/paths.ts",
     "./team": "./src/team.ts"
   },

packages/triage-contracts/src/index.ts

@@ -27,6 +27,7 @@ export * from "./code-review.js";
 export * from "./stage-result.js";
 export * from "./listener.js";
 export * from "./rules-gate.js";
+export * from "./merge-gate.js";
 export * from "./policy-export.js";
 export * from "./policy-patch.js";
 export * from "./code-patterns.js";

packages/triage-contracts/src/merge-gate.ts

@@ -0,0 +1,34 @@
+/** What CI says of a pull request's head, summed over its check runs. */
+export type HeadCi = "success" | "failure" | "pending" | "cancelled" | "none";
+
+const CI_TEXT: Record<HeadCi, string> = {
+  success: "CI passed on this head",
+  failure: "CI has failed on this head",
+  pending: "CI has not finished on this head",
+  cancelled: "CI was cancelled on this head",
+  none: "There is no CI on this head",
+};
+
+export function ciRefusal(ci: HeadCi): string {
+  return CI_TEXT[ci];
+}
+
+/**
+ * What a merge is decided on. `changesRequestedBy` is each reviewer, other than Triage, whose latest review requests
+ * changes; `threadsHold` is why the review threads hold the merge, or undefined when they do not.
+ */
+export type MergeInput = { ci: HeadCi; ciOptional: boolean; changesRequestedBy: readonly string[]; threadsHold: string | undefined };
+
+/** A person's review or thread waits on that person; CI waits on the head's checks. */
+export type MergeRefusal = { allowed: false; cause: "changes-requested" | "threads"; reason: string } | { allowed: false; cause: "ci"; ci: Exclude<HeadCi, "success">; reason: string };
+
+/**
+ * Whether a pull request may merge: no reviewer requests changes, no review thread holds it, and its CI passed, or it
+ * has none and the policy does not require it. People are asked first, so CI never stands in for their answer.
+ */
+export function mergeGate({ ci, ciOptional, changesRequestedBy, threadsHold }: MergeInput): { allowed: true } | MergeRefusal {
+  if (changesRequestedBy.length > 0) return { allowed: false, cause: "changes-requested", reason: `changes are requested by ${changesRequestedBy.join(", ")}` };
+  if (threadsHold !== undefined) return { allowed: false, cause: "threads", reason: threadsHold };
+  if (ci === "success" || (ci === "none" && ciOptional)) return { allowed: true };
+  return { allowed: false, cause: "ci", ci, reason: ciRefusal(ci) };
+}

packages/triage-workflows/src/logic/actions.test.ts

@@ -146,7 +146,7 @@ describe("evaluateActions", () => {
   test("a merge is held with the real CI state until CI passes, and goes ahead without CI only when the policy switched CI green off", () => {
     function merge(ci: PrFacts["checks"], ciGreen?: boolean) {
       const pack = { ...emptyPack("acme/widgets"), checks: ciGreen === undefined ? {} : { "ci-green": { enabled: ciGreen } }, actions: { ...defaultActions(), pass: [{ do: "merge" }] satisfies Step[] } };
-      return heldApprovals(evaluateActions({ facts: { ...facts, checks: ci }, checks: [], verdict: "pass", reason: "passes", pack }), undefined, { checks: ci })[0];
+      return heldApprovals(evaluateActions({ facts: { ...facts, checks: ci }, checks: [], verdict: "pass", reason: "passes", pack }), undefined, { checks: ci, changesRequested: [] })[0];
     }
     expect(merge("pending", false)).toMatchObject({ effectId: expect.any(String), held: true, reason: "CI has not finished on this head.", ci: "pending" });
     expect(merge("none", true)).toMatchObject({ held: true, reason: "There is no CI on this head.", ci: "none" });

packages/triage-workflows/src/logic/actions.ts

@@ -1,7 +1,7 @@
 import { sha256 } from "@noble/hashes/sha2.js";
 import { bytesToHex, utf8ToBytes } from "@noble/hashes/utils.js";
-import { ciRefusal, mergeableCi, missingFrom } from "@corbits/github-reads";
-import { alwaysHeld, authorGroupsOf, CATALOG_IDS, catalogCheckEnabled, checkName, classificationSources, labelRules, LEVEL_LABELS, levelLabel, RISK_MARKS, ruleLabel, whenHolds, type ActionKind, type CheckPack, type RepoSource, type ResolvedTarget, type CiState, type Step, type StepSubject, teamSlug, type TriageEvent, type VerdictWord, VERDICTS } from "@corbits/triage-contracts";
+import { missingFrom } from "@corbits/github-reads";
+import { alwaysHeld, authorGroupsOf, CATALOG_IDS, catalogCheckEnabled, checkName, ciRefusal, classificationSources, labelRules, LEVEL_LABELS, levelLabel, mergeGate, RISK_MARKS, ruleLabel, whenHolds, type ActionKind, type CheckPack, type RepoSource, type ResolvedTarget, type CiState, type Step, type StepSubject, teamSlug, type TriageEvent, type VerdictWord, VERDICTS } from "@corbits/triage-contracts";
 import { issueReference, type CheckResult, type PrFacts } from "./checks.js";
 import { authorFailures, bounded, inlineCode, isLabelledFinding } from "./render.js";
 import { compare, errorText } from "@corbits/common";
@@ -319,16 +319,20 @@ export function changesWanted(input: Pick<ActionInput, "checks" | "pack" | "fact
 
 /**
  * An approve that may not stand is skipped before the CI hold, so running checks never turn it into a held approval. A
- * merge the review threads hold is skipped before it too, rather than held, so no person can approve it past them and
- * a later pass writes it under the same effect id once they allow it.
+ * merge a reviewer's requested changes or the review threads hold is skipped before it too, rather than held, so no
+ * person can approve it past them and a later pass writes it under the same effect id once they allow it.
  */
-export function heldApprovals(actions: SuggestedAction[], approval: ApprovalHold | undefined, { checks: ci, mergeHold }: Pick<PrFacts, "checks" | "mergeHold">): SuggestedAction[] {
+export function heldApprovals(actions: SuggestedAction[], approval: ApprovalHold | undefined, { checks: ci, changesRequested, mergeHold }: Pick<PrFacts, "checks" | "changesRequested" | "mergeHold">): SuggestedAction[] {
   return actions.map(function held(action): SuggestedAction {
     if (!("effectId" in action)) return action;
     if (action.kind === "approve" && approval !== undefined) return { list: action.list, index: action.index, kind: action.kind, skipped: true, ...approval };
-    if (action.kind === "merge" && mergeHold !== undefined) return { list: action.list, index: action.index, kind: action.kind, skipped: true, waits: true, reason: mergeHold };
-    if (ci === "pending") return { ...action, held: true, reason: `${ciRefusal(ci)}.`, ci };
-    if (ci !== "success" && action.kind === "merge" && !mergeableCi(ci, "ciOptional" in action.target)) return { ...action, held: true, reason: `${ciRefusal(ci)}.`, ci };
+    if (action.kind === "merge") {
+      const gate = mergeGate({ ci, ciOptional: "ciOptional" in action.target, changesRequestedBy: changesRequested, threadsHold: mergeHold });
+      if (gate.allowed) return action;
+      if (gate.cause === "ci") return { ...action, held: true, reason: sentence(gate.reason), ci: gate.ci };
+      return { list: action.list, index: action.index, kind: action.kind, skipped: true, waits: true, reason: gate.cause === "threads" ? gate.reason : sentence(gate.reason) };
+    }
+    if (ci === "pending") return { ...action, held: true, reason: sentence(ciRefusal(ci)), ci };
     return action;
   });
 }

tests/model/rules.postgres.test.ts

@@ -33,6 +33,8 @@ const labellingReviewed = await modelStack(`${OWNER}/labelling-reviewed`, { pack
 const UNLABELLED: typeof PACK = { ...PACK, repo: `${OWNER}/unlabelled` };
 delete UNLABELLED.labels;
 const unlabelled = await modelStack(UNLABELLED.repo, { pack: UNLABELLED, automatic: ["labels", "comment"] });
+const MERGING: typeof PACK = { ...PACK, repo: `${OWNER}/merging`, actions: { ...PACK.actions, fail: [{ do: "merge" }] } };
+const merging = await modelStack(MERGING.repo, { pack: MERGING, automatic: ["labels", "comment", "merge"] });
 setDefaultTimeout(5 * 60_000);
 afterAll(async () => {
   await stack.stop();
@@ -41,6 +43,7 @@ afterAll(async () => {
   await labelling.stop();
   await labellingReviewed.stop();
   await unlabelled.stop();
+  await merging.stop();
 });
 
 let pulls = 0;
@@ -181,3 +184,15 @@ test("with labels off, Triage adds no label and takes an earlier verdict label o
   expect(labelled(writes)).toEqual([]);
   expect(removed).toEqual([VERDICT_LABELS.pass]);
 });
+
+test("a merge waits while a reviewer requests changes", async () => {
+  const number = ++pulls;
+  const pull: Played = { number, heads: [sha(`${number}-0`)], draft: false };
+  const review = { id: 1, user: { login: "maya", type: "User" }, state: "CHANGES_REQUESTED", body: "", commit_id: pull.heads[0]!, comments: [] };
+  await merging.open({ ...pullOf(number, pull.heads[0]!), reviews: [review] });
+  await settle(merging, pull);
+  const writes = merging.writes(number);
+  expect(writes.filter((write) => write.path.endsWith("/merge"))).toEqual([]);
+  expect(merging.github.misses.filter((miss) => miss.endsWith("/merge"))).toEqual([]);
+  expect(commentsAndEarlyLabels(writes).comments.at(-1)).toContain("Changes are requested by maya.");
+});