fix(agents): honor ruleset-required reviews in the merge author bound

lib/agents/tools/github-pr-merge-ownership.test.ts

@@ -1,7 +1,8 @@
-import { afterEach, beforeEach, describe, expect, it } from "vitest";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import {
   loadPullRequestOwnership,
   mergeAuthorBasis,
+  rulesetRequiresReview,
   type PullRequestOwnership,
 } from "./github-pr-merge-ownership";
 
@@ -22,12 +23,16 @@ function ownership(
     authorLogin: "mallory",
     authorIsBot: false,
     reviewDecision: null,
+    baseRefName: "main",
     url: "https://github.com/acme/widgets/pull/84",
     ...overrides,
   };
 }
 
-const userLogin = (login: string | null) => async () => login;
+const userLogin = (login: string | null, rulesetReview = false) => ({
+  userGithubLogin: async () => login,
+  rulesetRequiresReview: async () => rulesetReview,
+});
 
 describe("mergeAuthorBasis", () => {
   it("should allow a PR the Mogplex app opened", async () => {
@@ -66,6 +71,22 @@ describe("mergeAuthorBasis", () => {
     ).resolves.toBeNull();
   });
 
+  it("should match the app when GITHUB_APP_NAME carries the [bot] suffix", async () => {
+    process.env.GITHUB_APP_NAME = "mogplex[bot]";
+    await expect(
+      mergeAuthorBasis(
+        ownership({ authorLogin: "mogplex", authorIsBot: true }),
+        userLogin(null)
+      )
+    ).resolves.toBe("mogplex_author");
+  });
+
+  it("should defer someone else's PR to a review a ruleset requires", async () => {
+    await expect(
+      mergeAuthorBasis(ownership(), userLogin("charles", true))
+    ).resolves.toBe("human_review");
+  });
+
   it("should defer someone else's PR to a required human review", async () => {
     await expect(
       mergeAuthorBasis(
@@ -92,6 +113,7 @@ describe("loadPullRequestOwnership", () => {
             pullRequest: {
               url: "https://github.com/acme/widgets/pull/84",
               reviewDecision: "APPROVED",
+              baseRefName: "main",
               author: { __typename: "Bot", login: "mogplex" },
             },
           },
@@ -104,6 +126,7 @@ describe("loadPullRequestOwnership", () => {
       authorLogin: "mogplex",
       authorIsBot: true,
       reviewDecision: "APPROVED",
+      baseRefName: "main",
       url: "https://github.com/acme/widgets/pull/84",
     });
   });
@@ -120,3 +143,124 @@ describe("loadPullRequestOwnership", () => {
     ).rejects.toThrow("Could not resolve to a PullRequest");
   });
 });
+
+describe("rulesetRequiresReview", () => {
+  const input = {
+    githubToken: "t",
+    owner: "acme",
+    repo: "widgets",
+    branch: "main",
+  };
+  const respond = (body: unknown, status = 200) =>
+    (async () => Response.json(body, { status })) as unknown as typeof fetch;
+
+  it("should see an approving-review requirement in the branch rules", async () => {
+    await expect(
+      rulesetRequiresReview({
+        ...input,
+        fetchImpl: respond([
+          {
+            type: "pull_request",
+            parameters: { required_approving_review_count: 1 },
+          },
+        ]),
+      })
+    ).resolves.toBe(true);
+  });
+
+  it("should not count a pull request rule that needs no approvals", async () => {
+    await expect(
+      rulesetRequiresReview({
+        ...input,
+        fetchImpl: respond([
+          {
+            type: "pull_request",
+            parameters: { required_approving_review_count: 0 },
+          },
+        ]),
+      })
+    ).resolves.toBe(false);
+  });
+
+  it("should honor code-owner and named-reviewer requirements", async () => {
+    for (const parameters of [
+      { require_code_owner_review: true },
+      { required_reviewers: [{ minimum_approvals: 1 }] },
+      { require_last_push_approval: true },
+    ]) {
+      await expect(
+        rulesetRequiresReview({
+          ...input,
+          fetchImpl: respond([{ type: "pull_request", parameters }]),
+        })
+      ).resolves.toBe(true);
+    }
+  });
+
+  it("should find a review requirement past the first page of rules", async () => {
+    const filler = Array.from({ length: 100 }, () => ({ type: "deletion" }));
+    const pages: string[] = [];
+    const fetchImpl = (async (url: string) => {
+      pages.push(new URL(url).searchParams.get("page") ?? "");
+      return Response.json(
+        pages.length === 1
+          ? filler
+          : [
+              {
+                type: "pull_request",
+                parameters: { required_approving_review_count: 2 },
+              },
+            ]
+      );
+    }) as unknown as typeof fetch;
+
+    await expect(rulesetRequiresReview({ ...input, fetchImpl })).resolves.toBe(
+      true
+    );
+    expect(pages).toEqual(["1", "2"]);
+  });
+
+  it("should treat a network failure as no confirmed requirement, and log it", async () => {
+    const fetchImpl = (async () => {
+      throw new Error("socket hang up");
+    }) as unknown as typeof fetch;
+    const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined);
+
+    await expect(rulesetRequiresReview({ ...input, fetchImpl })).resolves.toBe(
+      false
+    );
+    expect(warn).toHaveBeenCalledWith(
+      "[github-merge] branch rules unreadable",
+      {
+        repo: "acme/widgets",
+        branch: "main",
+        reason: "socket hang up",
+      }
+    );
+    warn.mockRestore();
+  });
+
+  it("should encode every path segment of the rules URL", async () => {
+    const urls: string[] = [];
+    const fetchImpl = (async (url: string) => {
+      urls.push(url);
+      return Response.json([]);
+    }) as unknown as typeof fetch;
+
+    await rulesetRequiresReview({
+      ...input,
+      owner: "acme/evil",
+      branch: "release/1.0",
+      fetchImpl,
+    });
+    expect(new URL(urls[0] ?? "").pathname).toBe(
+      "/repos/acme%2Fevil/widgets/rules/branches/release%2F1.0"
+    );
+  });
+
+  it("should treat unreadable rules as no confirmed requirement", async () => {
+    await expect(
+      rulesetRequiresReview({ ...input, fetchImpl: respond({}, 404) })
+    ).resolves.toBe(false);
+  });
+});

lib/agents/tools/github-pr-merge-ownership.ts

@@ -7,14 +7,19 @@ import { githubHeaders } from "@/lib/github-merge";
  * prompt-injected merge is the pull request itself: the agent merges PRs the
  * user authored or a Mogplex run opened for them (authored by the Mogplex
  * GitHub App, which outsiders cannot impersonate). Anyone else's PR merges
- * only where the repository requires a human review, so GitHub keeps a person
- * in the loop; otherwise the user merges it on GitHub.
+ * only where the repository requires a human review, through classic branch
+ * protection or a ruleset, so GitHub keeps a person in the loop; otherwise
+ * the user merges it on GitHub.
  */
 export type PullRequestOwnership = {
   authorLogin: string | null;
   authorIsBot: boolean;
-  /** GitHub's review decision; null when the repo requires no review. */
+  /**
+   * GitHub's review decision. Null when classic branch protection requires no
+   * review; rulesets never populate it, so those are read separately.
+   */
   reviewDecision: string | null;
+  baseRefName: string | null;
   url: string | null;
 };
 
@@ -29,6 +34,7 @@ type OwnershipGraphqlPayload = {
       pullRequest?: {
         url?: string | null;
         reviewDecision?: string | null;
+        baseRefName?: string | null;
         author?: { __typename?: string; login?: string | null } | null;
       } | null;
     } | null;
@@ -41,6 +47,7 @@ const OWNERSHIP_QUERY = `query PullRequestOwnership($owner: String!, $repo: Stri
     pullRequest(number: $number) {
       url
       reviewDecision
+      baseRefName
       author { __typename login }
     }
   }
@@ -97,13 +104,15 @@ function readOwnership(
     authorLogin: author.login ?? null,
     authorIsBot: author.__typename === "Bot",
     reviewDecision: pullRequest.reviewDecision ?? null,
+    baseRefName: pullRequest.baseRefName ?? null,
     url: pullRequest.url ?? null,
   };
 }
 
 /** The Mogplex GitHub App's login, as GitHub reports a bot author. */
 export function mogplexAppLogin() {
-  return process.env.GITHUB_APP_NAME?.trim().toLowerCase() || null;
+  const appName = process.env.GITHUB_APP_NAME?.trim();
+  return appName ? botLogin(appName) : null;
 }
 
 function sameLogin(a: string | null | undefined, b: string | null | undefined) {
@@ -115,19 +124,126 @@ function botLogin(login: string) {
   return lower.endsWith("[bot]") ? lower.slice(0, -"[bot]".length) : lower;
 }
 
+type RulesetRule = {
+  type?: string;
+  parameters?: {
+    required_approving_review_count?: number;
+    require_code_owner_review?: boolean;
+    require_last_push_approval?: boolean;
+    required_reviewers?: Array<{ minimum_approvals?: number }>;
+  };
+};
+
+const RULES_PER_PAGE = 100;
+/** Bounds the read against a misbehaving API, not a product limit. */
+const MAX_RULE_PAGES = 10;
+
+/** A pull request rule that makes a person approve before merging. */
+function requiresApproval(rule: RulesetRule) {
+  if (rule.type !== "pull_request") return false;
+  const parameters = rule.parameters ?? {};
+  return (
+    (parameters.required_approving_review_count ?? 0) > 0 ||
+    parameters.require_code_owner_review === true ||
+    parameters.require_last_push_approval === true ||
+    (parameters.required_reviewers ?? []).some(
+      (reviewer) => (reviewer.minimum_approvals ?? 0) > 0
+    )
+  );
+}
+
+type RulesInput = {
+  githubToken: string;
+  owner: string;
+  repo: string;
+  branch: string;
+  fetchImpl?: typeof fetch;
+};
+
+/** Logged so a transient read failure is told apart from no requirement. */
+function unreadableRules(input: RulesInput, reason: string) {
+  console.warn("[github-merge] branch rules unreadable", {
+    repo: `${input.owner}/${input.repo}`,
+    branch: input.branch,
+    reason,
+  });
+  return null;
+}
+
+/** One page of the branch's active rules, or null when it can't be read. */
+async function loadRulePage(input: RulesInput, page: number) {
+  const doFetch = input.fetchImpl ?? fetch;
+  const path = [input.owner, input.repo, "rules", "branches", input.branch]
+    .map(encodeURIComponent)
+    .join("/");
+  const url = `https://api.github.com/repos/${path}?per_page=${RULES_PER_PAGE}&page=${page}`;
+  try {
+    const res = await doFetch(url, {
+      headers: githubHeaders(input.githubToken),
+    });
+    if (!res.ok) return unreadableRules(input, `HTTP ${res.status}`);
+    const rules: unknown = await res.json();
+    return Array.isArray(rules)
+      ? (rules as RulesetRule[])
+      : unreadableRules(input, "unexpected response body");
+  } catch (error) {
+    return unreadableRules(
+      input,
+      error instanceof Error ? error.message : String(error)
+    );
+  }
+}
+
+/**
+ * Whether an active ruleset on the branch requires a person's approval:
+ * an approval count, code-owner review, named reviewers, or approval of the
+ * most recent push. GitHub returns
+ * only enforced rules here, readable with read access; an unreadable answer
+ * counts as "not confirmed". Ruleset bypass actors are invisible to readers,
+ * so a bypass granted to the app is outside this check, as it is for classic
+ * branch protection.
+ */
+export async function rulesetRequiresReview(input: RulesInput) {
+  for (let page = 1; page <= MAX_RULE_PAGES; page += 1) {
+    const rules = await loadRulePage(input, page);
+    if (!rules) return false;
+    if (rules.some(requiresApproval)) return true;
+    if (rules.length < RULES_PER_PAGE) return false;
+  }
+  unreadableRules(input, `more than ${MAX_RULE_PAGES} pages of rules`);
+  return false;
+}
+
+type MergeAuthorLoaders = {
+  userGithubLogin: () => Promise<string | null>;
+  /** Only asked when classic protection reports no review requirement. */
+  rulesetRequiresReview: (branch: string) => Promise<boolean>;
+};
+
+async function requiresHumanReview(
+  ownership: PullRequestOwnership,
+  loaders: MergeAuthorLoaders
+) {
+  if (ownership.reviewDecision) return true;
+  if (!ownership.baseRefName) return false;
+  return loaders.rulesetRequiresReview(ownership.baseRefName);
+}
+
 /**
  * Why this PR may be merged, or null when the user must merge it on GitHub.
- * The user's GitHub login is loaded only for a human-authored PR.
+ * The user's login and the rulesets are loaded only when needed.
  */
 export async function mergeAuthorBasis(
   ownership: PullRequestOwnership,
-  loadUserGithubLogin: () => Promise<string | null>
+  loaders: MergeAuthorLoaders
 ): Promise<MergeAuthorBasis | null> {
   const { authorLogin } = ownership;
   if (ownership.authorIsBot && authorLogin) {
     if (botLogin(authorLogin) === mogplexAppLogin()) return "mogplex_author";
-  } else if (sameLogin(authorLogin, await loadUserGithubLogin())) {
+  } else if (sameLogin(authorLogin, await loaders.userGithubLogin())) {
     return "user_author";
   }
-  return ownership.reviewDecision ? "human_review" : null;
+  return (await requiresHumanReview(ownership, loaders))
+    ? "human_review"
+    : null;
 }