fix(hub): run the portal's Merge through the merge gate (CL-12170)

apps/hub/src/github/github-read-access.postgres.test.ts

@@ -35,6 +35,8 @@ const HEAD = "abc1230000000000000000000000000000000000";
 const testSchema = `corbits_read_access_${randomBytes(6).toString("hex")}`;
 const requested: string[] = [];
 const reviewed: unknown[] = [];
+let reviews: unknown[] = [];
+let checkRuns: unknown[] = [];
 
 const READS: Record<string, unknown> = {
   [`/repos/${REPO}/pulls`]: [
@@ -45,7 +47,6 @@ const READS: Record<string, unknown> = {
   [`/repos/${REPO}/pulls/8/commits`]: [{ sha: HEAD, commit: { message: "fix: the widget" } }],
   [`/repos/${REPO}/pulls/8/files`]: [{ filename: "src/widget.ts", status: "modified", additions: 1, deletions: 0, patch: "@@ -1 +1 @@\n+fixed" }],
   [`/repos/${REPO}/issues/8/comments`]: [],
-  [`/repos/${REPO}/commits/${HEAD}/check-runs`]: { check_runs: [] },
   [`/repos/${REPO}/commits/${HEAD}/status`]: { statuses: [] },
 };
 
@@ -55,7 +56,9 @@ async function github(req: Request): Promise<Response> {
   if (pathname === "/app/installations/1/access_tokens") return Response.json({ token: "installation-token", expires_at: new Date(Date.now() + 3_600_000).toISOString() });
   if (req.method === "GET" && pathname in READS) return Response.json(READS[pathname]);
   if (pathname === `/repos/${REPO}/installation`) return Response.json({ id: 1 });
-  if (req.method === "GET" && pathname === `/repos/${REPO}/pulls/8/reviews`) return Response.json([]);
+  if (req.method === "GET" && pathname === `/repos/${REPO}/pulls/8/reviews`) return Response.json(reviews);
+  if (req.method === "POST" && pathname === "/graphql") return Response.json({ data: { repository: { pullRequest: { reviewThreads: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } } } } } });
+  if (req.method === "GET" && pathname === `/repos/${REPO}/commits/${HEAD}/check-runs`) return Response.json({ total_count: checkRuns.length, check_runs: checkRuns });
   if (req.method === "POST" && pathname === `/repos/${REPO}/pulls/8/reviews`) {
     reviewed.push(await req.json());
     return Response.json({ id: 1, state: "APPROVED" });
@@ -125,6 +128,7 @@ beforeAll(async () => {
     { id: "prn_admin", tenantId: TENANT, kind: "user", refId: "usr_admin", status: "active" },
     { id: "prn_approver", tenantId: TENANT, kind: "user", refId: "usr_approver", status: "active" },
     { id: "prn_lent", tenantId: TENANT, kind: "user", refId: "usr_lent", status: "active" },
+    { id: "prn_merger", tenantId: TENANT, kind: "user", refId: "usr_merger", status: "active" },
   ]);
   await db.insert(schema.role).values([
     { id: "rol_member", tenantId: TENANT, name: "member", isSystem: true },
@@ -146,6 +150,8 @@ beforeAll(async () => {
     { id: "grt_approver_labels", tenantId: TENANT, principalId: "prn_approver", resource: `triage-do:labels:github:${REPO}`, action: "invoke", effect: "allow", origin: "system" },
     { id: "grt_approver_comment", tenantId: TENANT, principalId: "prn_approver", resource: `triage-do:comment:github:${REPO}`, action: "invoke", effect: "allow", origin: "system" },
     { id: "grt_lent_use", tenantId: TENANT, principalId: "prn_lent", resource: "credential:crd_github", action: "use", effect: "allow", origin: "system" },
+    { id: "grt_merger_use", tenantId: TENANT, principalId: "prn_merger", resource: "credential:crd_github", action: "use", effect: "allow", origin: "system" },
+    { id: "grt_merger_merge", tenantId: TENANT, principalId: "prn_merger", resource: `triage-do:merge:github:${REPO}`, action: "invoke", effect: "allow", origin: "system" },
     { id: "grt_triage_approve", tenantId: TENANT, roleId: "rol_triage", resource: "triage-do:approve:*", action: "invoke", effect: "allow", origin: "role" },
   ]);
   await db.insert(schema.provider).values({ id: "prv_github", tenantId: TENANT, name: "github", plugin: "http", apiBaseUrl: githubApiOrigin });
@@ -173,7 +179,7 @@ test("PostgreSQL: holding read or use on the GitHub credential reads pull reques
     expect(await details.json()).toMatchObject({ pr: { number: 8 }, files: [{ path: "src/widget.ts" }] });
   }
 
-  const act = await prActions({ action: "merge", repo: REPO, number: 8 }, member("usr_member"));
+  const act = await prActions({ action: "merge", repo: REPO, number: 8, headSha: HEAD }, member("usr_member"));
   expect(act.status).toBe(403);
   expect(await act.json()).toEqual({ error: { code: "forbidden", message: "You do not have access to the GitHub App credential." } });
   expect(requested.some((line) => line.startsWith("PUT"))).toBe(false);
@@ -196,7 +202,7 @@ test("PostgreSQL: approving needs the approve Do grant on top of use, and pins t
   expect(approved.status).toBe(200);
   expect(reviewed).toEqual([{ body: "", event: "APPROVE", commit_id: HEAD }]);
 
-  const merge = await prActions({ action: "merge", repo: REPO, number: 8 }, member("usr_approver"));
+  const merge = await prActions({ action: "merge", repo: REPO, number: 8, headSha: HEAD }, member("usr_approver"));
   expect(merge.status).toBe(403);
   expect(await merge.json()).toEqual({ error: { code: "forbidden", message: "You do not have the grant for this kind of Do in this repository." } });
 
@@ -206,6 +212,32 @@ test("PostgreSQL: approving needs the approve Do grant on top of use, and pins t
   expect(reviewed).toHaveLength(1);
 });
 
+test("PostgreSQL: the portal's Merge is refused while a reviewer requests changes or CI has failed, or without the head it was shown, and nothing is merged", async () => {
+  const merge = { action: "merge", repo: REPO, number: 8, headSha: HEAD } as const;
+  try {
+    reviews = [{ user: { login: "hubot" }, state: "CHANGES_REQUESTED", commit_id: HEAD }];
+    checkRuns = [{ name: "ci", status: "completed", conclusion: "success" }];
+    const changes = await prActions(merge, member("usr_merger"));
+    expect({ status: changes.status, body: await changes.json() }).toMatchObject({ status: 409, body: { error: { code: "merge_refused", message: expect.stringContaining("hubot") } } });
+
+    reviews = [];
+    checkRuns = [{ name: "ci", status: "completed", conclusion: "failure" }];
+    const failing = await prActions(merge, member("usr_merger"));
+    expect({ status: failing.status, body: await failing.json() }).toMatchObject({ status: 409, body: { error: { code: "merge_refused", message: expect.stringContaining("CI has failed") } } });
+
+    checkRuns = [{ name: "ci", status: "completed", conclusion: "success" }];
+    const app = guardedApp(handle.db, sessionOf, grants, { prActions });
+    const unpinned = await app.request(`/api/integrations/github-actions/${TENANT}`, {
+      method: "POST", headers: { "x-test-user": "usr_merger", authorization: "Bearer test", "content-type": "application/json" }, body: JSON.stringify({ action: "merge", repo: REPO, number: 8 }),
+    });
+    expect({ status: unpinned.status, body: await unpinned.json() }).toMatchObject({ status: 400, body: { error: { code: "invalid_request" } } });
+    expect(requested).not.toContain(`PUT /repos/${REPO}/pulls/8/merge`);
+  } finally {
+    reviews = [];
+    checkRuns = [];
+  }
+});
+
 test("PostgreSQL: every verdict route needs workflow-run read, which holding the GitHub credential does not give", async () => {
   const unused = {} as never;
   const app = guardedApp(handle.db, sessionOf, grants, {

apps/hub/src/github/pr-actions.ts

@@ -15,8 +15,10 @@ import {
   sendCodeReview,
   upsertTriageComment,
 } from "@corbits/github-tool/github";
+import { readMergeGate } from "@corbits/github-tool/do";
 import { codeownersForPr, type GithubFetch } from "@corbits/github-reads";
-import { ownedLabels, TRIAGE_LABELS, type ActionKind, type SendCodeReviewInput, type SentThreads } from "@corbits/triage-contracts";
+import { ownedLabels, TRIAGE_LABELS, type ActionKind, type CheckPack, type SendCodeReviewInput, type SentThreads } from "@corbits/triage-contracts";
+import { mergesWithoutCi, refusalText } from "@corbits/triage-workflows/logic/actions";
 import { codeReviewSummary } from "@corbits/triage-workflows/logic/render";
 import type { CodeReviewRuns, CompletedReview } from "../code-review-runs.js";
 import type { PolicyReader } from "./repo-policy.js";
@@ -51,7 +53,7 @@ export const ActionBody = type({ repo: /^[\w.-]+\/[\w.-]+$/, number: "number.int
     .or({ action: "'review'", event: "'REQUEST_CHANGES'", body: "string" })
     .or({ action: "'review'", event: "'APPROVE'", body: "string", headSha: "string > 0" })
     .or({ action: "'code-review'", headSha: "string > 0" })
-    .or({ action: "'merge'", "headSha?": "string > 0" })
+    .or({ action: "'merge'", headSha: "string > 0" })
     .or({ action: "'close'", labels: "string[]", comment: "string" }),
 );
 
@@ -99,7 +101,7 @@ async function runAction(gh: GithubFetch, login: string, body: Exclude<typeof Ac
     case "review":
       return createReview(gh, { repo, number, body: body.body, event: body.event, ...("headSha" in body ? { commitId: body.headSha } : {}) });
     case "merge":
-      return mergePr(gh, { repo, number, ...(body.headSha ? { sha: body.headSha } : {}) });
+      return mergePr(gh, { repo, number, sha: body.headSha });
     case "close":
       return mirror(gh, { repo, number, labels: body.labels, owned, comment: body.comment, close: true }, login);
   }
@@ -133,10 +135,18 @@ export function createGithubPrActions(deps: PortalCredentialDeps & {
 }) {
   const appFetch = createGithubAppCredentialFetch({ apiOrigin: deps.githubApiOrigin });
 
-  async function ownedBy(tenantId: string, body: { action: string; repo: string }): Promise<readonly string[]> {
-    if (body.action !== "close") return TRIAGE_LABELS;
+  /** Only a close and a merge depend on the repository's policy. */
+  async function policyOf(tenantId: string, body: typeof ActionBody.infer): Promise<CheckPack | undefined> {
+    if (body.action !== "close" && body.action !== "merge") return undefined;
     const read = await deps.readCheckPack(tenantId, body.repo, "github");
-    return read.status === "ok" ? ownedLabels(read.pack.labels) : TRIAGE_LABELS;
+    return read.status === "ok" ? read.pack : undefined;
+  }
+
+  /** The portal's Merge is held by the same gate as triage's own; bypassing it is a GitHub admin's call, made on GitHub. */
+  async function mergeRefused(gh: GithubFetch, login: string, pack: CheckPack | undefined, { repo, number, headSha }: { repo: string; number: number; headSha: string }) {
+    const ciOptional = pack !== undefined && mergesWithoutCi(pack);
+    const { gate } = await readMergeGate(gh, { repo, number, sha: headSha, login, ciOptional });
+    return gate.allowed ? undefined : failure(409, "merge_refused", refusalText(gate));
   }
 
   async function act(gh: GithubFetch, login: string, tenantId: string, body: typeof ActionBody.infer) {
@@ -146,7 +156,12 @@ export function createGithubPrActions(deps: PortalCredentialDeps & {
         return sendCodeReview(gh, input, login);
       }, body, completed, carriedTo);
     }
-    return runAction(gh, login, body, await ownedBy(tenantId, body));
+    const pack = await policyOf(tenantId, body);
+    if (body.action === "merge") {
+      const refused = await mergeRefused(gh, login, pack, body);
+      if (refused) return refused;
+    }
+    return runAction(gh, login, body, pack ? ownedLabels(pack.labels) : TRIAGE_LABELS);
   }
 
   return async function handle(body: typeof ActionBody.infer, { tenantId, principalId }: Member): Promise<Response> {

docs/Corbits Triage User Manual/Corbits Triage User Manual.md

@@ -488,7 +488,7 @@ Clicking a pull request opens it on the right. The pane is built to be read top
      - **Request changes**: **Request changes from @ada: Linked issue**. When no rule failed it says what GitHub or Bitbucket holds it for, such as **Request changes from @ada: behind its base branch**, or just **Request changes from @ada**.
      - **Approve**: **Review and approve** with every rule met, or **Approve anyway, or ask @ada to fix: Linked issue** when one failed.
      - **Reply**: **Reply to @ada**, or **Reply to @ada · not met: Linked issue** when a rule failed and Triage drafted no feedback.
-     - **Merge**: **Ready to merge**, or **Ready to merge · still not met: Linked issue** when a rule failed. A pull request that passed but that GitHub or Bitbucket will not merge yet says why instead, such as **Can't merge yet: waiting for a required review**, **Can't merge yet: behind its base branch**, **Can't merge yet: the branch's rules block the merge** or, on Bitbucket, the reason its merge checks give, and **Merge** stays off until it can. A pull request Triage opened itself is merged rather than approved, so it reads **Ready to merge** too; with no button and a rule failing it reads **Not met: Linked issue**, and when GitHub or Bitbucket holds it with nothing failed, **Blocked:** and why, such as **Blocked: behind its base branch**.
+     - **Merge**: **Ready to merge**, or **Ready to merge · still not met: Linked issue** when a rule failed. A pull request that passed but that GitHub or Bitbucket will not merge yet says why instead, such as **Can't merge yet: waiting for a required review**, **Can't merge yet: behind its base branch**, **Can't merge yet: the branch's rules block the merge** or, on Bitbucket, the reason its merge checks give, and **Merge** stays off until it can. Pressing **Merge** follows the same rules as Triage's own merge: it is refused, with the reason, while a reviewer requests changes, a person's review thread is open, or CI has not passed (a pull request with no CI merges only when the policy turns **CI green** off); a GitHub admin can still merge on GitHub. A pull request Triage opened itself is merged rather than approved, so it reads **Ready to merge** too; with no button and a rule failing it reads **Not met: Linked issue**, and when GitHub or Bitbucket holds it with nothing failed, **Blocked:** and why, such as **Blocked: behind its base branch**.
 
      Some decisions come before the button. A pull request the author updated after your review reads **Re-review: @ada updated it**, then what is still open, such as **· still not met: Linked issue** or **· still to decide: Focused**, or **· all rules met** when nothing is. A rule you need to decide reads **Decide: Focused (Triage wasn't sure)** when the model was unsure, **Decide: Linked issue (your policy asks you)** when the policy asks you about it when it fails, or **(see why below)** for both. A rule that could not be checked reads **Couldn't check Merge conflicts · use Triage again in More**. After you approve, it says only what is still open or what holds the merge, such as **You approved · still not met: Linked issue** or **You approved · ready to merge**. When an action waits for your approval on a pull request that is otherwise the author's, the line asks about it, such as **Needs you: Approve this pull request?** or **Needs you: Add label bug?**.
    - **Done:** what the run ended with: the verdict as its label on the pull request says it (**Ready**, **Not ready** or **Needs human review**), what Triage posted on its own, and whose turn it is, such as **Done: Not ready · added label risk: medium · waiting on @ada to fix: Linked issue**. A merged or closed pull request reads **Done: Merged** or **Done: Closed**, a draft with a verdict **Done: Not ready · draft · waiting on @ada**, and once you approve with nothing left for you, **Done: You approved**. **Reply** stays available if you want to add something. A verdict Triage could not settle reads **Done: Needs human review · Triage could not settle it · Triage again**. When the author pushed commits Triage has not checked, the line adds **newer commits not yet checked**, and **Triage again** when nothing in it is yours. A passing pull request is never merged before Triage checks them, so it reads **Done: Ready to merge · newer commits not yet checked · Triage again**. On the author's turn it reads, for example, **Done: Not ready · waiting on @ada · newer commits not yet checked · Triage again**.

packages/github-tool/src/do.ts

@@ -102,6 +102,15 @@ function closedOutcome(kind: ActionKind, merged: boolean): DoOutcome {
   throw new DoNotRunnableError(NOT_OPEN);
 }
 
+/** Reads the head's CI, each reviewer's latest review and the open review threads, and asks the merge gate. */
+export async function readMergeGate(gh: GithubFetch, { repo, number, sha, login, ciOptional }: { repo: string; number: number; sha: string; login: string; ciOptional: boolean }) {
+  const ci = summarizeChecks(await getChecks(gh, repo, sha));
+  const reviews = await getReviews(gh, repo, number, login);
+  const changesRequestedBy = [...latestDecisive(reviews)].filter(([, state]) => state === "CHANGES_REQUESTED").map(([reviewer]) => reviewer);
+  const gate = mergeGate({ ci, ciOptional, changesRequestedBy, threadsHold: await unresolvedThreadsHold(gh, repo, number, login) });
+  return { gate, reviews };
+}
+
 /** Writes one Do unless GitHub already shows it, so a repeat after a lost audit write changes nothing. `login` names the App's bot account, whose reviews are triage's own. */
 export async function executeDo(gh: GithubFetch, { request, step }: DoInput, login: string): Promise<DoOutcome> {
   const { repo, number } = request;
@@ -176,10 +185,7 @@ export async function executeDo(gh: GithubFetch, { request, step }: DoInput, log
     // CI, reviews and review threads are read again here because a held merge can be approved long after its verdict; branch protection stays the backstop.
     case "merge": {
       onHead(pr.sha, request, step);
-      const ci = summarizeChecks(await getChecks(gh, repo, step.headSha));
-      const reviews = await getReviews(gh, repo, number, login);
-      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) });
+      const { gate, reviews } = await readMergeGate(gh, { repo, number, sha: step.headSha, login, ciOptional: "ciOptional" in target });
       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);