diff --git a/lib/agents/tools/github-pr-merge-ownership.test.ts b/lib/agents/tools/github-pr-merge-ownership.test.ts index e75a24cb..d45ef0b7 100644 --- a/lib/agents/tools/github-pr-merge-ownership.test.ts +++ b/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); + }); +}); diff --git a/lib/agents/tools/github-pr-merge-ownership.ts b/lib/agents/tools/github-pr-merge-ownership.ts index 8314dbd9..6248ffe9 100644 --- a/lib/agents/tools/github-pr-merge-ownership.ts +++ b/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; + /** Only asked when classic protection reports no review requirement. */ + rulesetRequiresReview: (branch: string) => Promise; +}; + +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 + loaders: MergeAuthorLoaders ): Promise { 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; } diff --git a/lib/agents/tools/github-pr-merge.ts b/lib/agents/tools/github-pr-merge.ts index fb6d296c..78bf0cdf 100644 --- a/lib/agents/tools/github-pr-merge.ts +++ b/lib/agents/tools/github-pr-merge.ts @@ -12,6 +12,7 @@ import { import { loadPullRequestOwnership, mergeAuthorBasis, + rulesetRequiresReview, type MergeAuthorBasis, } from "./github-pr-merge-ownership"; @@ -221,15 +222,19 @@ async function authorizeMergeAuthor( const pr = `${input.owner}/${input.repo}#${input.number}`; try { const ownership = await loadPullRequestOwnership(input); - const basis = await mergeAuthorBasis(ownership, loadUserGithubLogin); + const basis = await mergeAuthorBasis(ownership, { + userGithubLogin: loadUserGithubLogin, + rulesetRequiresReview: (branch) => + rulesetRequiresReview({ ...input, branch }), + }); if (basis) return { basis }; const author = ownership.authorLogin ?? "someone else"; const where = ownership.url ? ` at ${ownership.url}` : ""; return { - error: `${pr} was opened by ${author}, and the repository does not require a human review, so Mogplex merges it only when you or Mogplex opened it. Merge it on GitHub${where}.`, + error: `${pr} was opened by ${author}, and Mogplex could not confirm the repository requires a human review, so it merges only pull requests you or Mogplex opened. Merge it on GitHub${where}.`, audit: { decision: "needs_user_merge", - error: `author ${author} without a required review`, + error: `author ${author} without a confirmed review requirement`, }, }; } catch (error) { @@ -362,7 +367,7 @@ export function createGithubPullRequestMergeTool( ); if ("error" in author) { await recordMergeAttempt(options, { ...attempt, ...author.audit }); - return { ok: false, merged: false, queued: false, error: author.error }; + return { error: author.error }; } const result = await attemptMerge({ ...attempt, diff --git a/tests/unit/github-pr-merge-author-bound.test.ts b/tests/unit/github-pr-merge-author-bound.test.ts index 08e72ffa..c2414705 100644 --- a/tests/unit/github-pr-merge-author-bound.test.ts +++ b/tests/unit/github-pr-merge-author-bound.test.ts @@ -39,12 +39,11 @@ test("github_merge_pull_request refuses someone else's PR when the repo requires assert.deepEqual(events, [ mergeAuditEvent("needs_user_merge", { - error: "author mallory without a required review", + error: "author mallory without a confirmed review requirement", }), ]); assert.equal(requestLines(calls).includes(MERGE_CALL), false); - const { ok, error } = result as { ok: boolean; error: string }; - assert.equal(ok, false); + const { error } = result as { error: string }; assert.ok( error.includes("https://github.com/acme/widgets/pull/84"), "the refusal should send the user to GitHub" @@ -79,6 +78,22 @@ test("github_merge_pull_request leaves someone else's PR to a required human rev ]); }); +test("github_merge_pull_request leaves someone else's PR to a review a ruleset requires", async () => { + const events = await auditedTeamMerge( + mergeFetch([], { + author: { __typename: "User", login: "mallory" }, + rulesetReviewCount: 1, + mergeableState: "blocked", + }), + ["acme"], + { userGithubLogin: "charles" } + ); + + assert.deepEqual(events, [ + mergeAuditEvent("auto_merge_queued", { basis: "human_review" }), + ]); +}); + test("github_merge_pull_request does not merge when it cannot tell who opened the PR", async () => { const calls: MergeFetchCall[] = []; const events = await auditedTeamMerge( diff --git a/tests/unit/helpers/github-pr-merge-fixtures.ts b/tests/unit/helpers/github-pr-merge-fixtures.ts index 2d912883..0a8c2e34 100644 --- a/tests/unit/helpers/github-pr-merge-fixtures.ts +++ b/tests/unit/helpers/github-pr-merge-fixtures.ts @@ -49,6 +49,8 @@ type MergeStubState = { author?: { __typename: "Bot" | "User"; login: string }; reviewDecision?: string | null; ownershipStatus?: number; + /** Approvals an active ruleset on `main` requires; none by default. */ + rulesetReviewCount?: number; }; function pullResponse(pull: MergeStubState) { @@ -73,6 +75,7 @@ function ownershipResponse(pull: MergeStubState) { pullRequest: { url: "https://github.com/acme/widgets/pull/84", reviewDecision: pull.reviewDecision ?? null, + baseRefName: "main", author: pull.author ?? { __typename: "Bot", login: "mogplex-test" }, }, }, @@ -103,6 +106,20 @@ export function mergeFetch(calls: MergeFetchCall[], pull: MergeStubState = {}) { } if (pathname === "/repos/acme/widgets/pulls/84") return pullResponse(pull); if (pathname === "/graphql") return graphqlResponse(pull, body); + if (pathname === "/repos/acme/widgets/rules/branches/main") { + return Response.json( + pull.rulesetReviewCount + ? [ + { + type: "pull_request", + parameters: { + required_approving_review_count: pull.rulesetReviewCount, + }, + }, + ] + : [] + ); + } return Response.json({ merged: true, sha: "6a6add1716c3fd2dc8ca76600638b445df6a7a07",