From b1208230a14949d5d87607808754d53dc915c2a8 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 20 Oct 2026 17:02:20 -0700 Subject: [PATCH] fix(web): don't offer merge when GitHub blocks it --- .../github-read-access.postgres.test.ts | 20 ++++++++++++++ apps/hub/src/github/github-refusal.test.ts | 9 +++++++ apps/hub/src/github/github-refusal.ts | 8 +++--- apps/hub/src/github/pr-details.ts | 25 ++++++++++++----- apps/web/src/lib/hub-api.ts | 8 ++++++ apps/web/src/lib/inbox-pane.ts | 27 +++++++++++++++---- apps/web/src/lib/pane-report.test.ts | 18 +++++++++++++ apps/web/src/lib/pane-report.ts | 24 +++++++++++++++-- apps/web/src/pages/PRDetail.tsx | 17 +++++++----- .../Corbits Triage User Manual.md | 4 +-- packages/github-reads/src/index.ts | 24 ++++++++++++++++- packages/github-tool/src/github.test.ts | 9 +++++++ packages/github-tool/src/github.ts | 1 + .../triage-workflows/src/pr-triage.test.ts | 2 +- 14 files changed, 169 insertions(+), 27 deletions(-) create mode 100644 apps/hub/src/github/github-refusal.test.ts diff --git a/apps/hub/src/github/github-read-access.postgres.test.ts b/apps/hub/src/github/github-read-access.postgres.test.ts index 053178b6d..6bd05045f 100644 --- a/apps/hub/src/github/github-read-access.postgres.test.ts +++ b/apps/hub/src/github/github-read-access.postgres.test.ts @@ -49,6 +49,11 @@ const READS: Record = { [`/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}/pulls/9`]: { number: 9, title: "Held by a ruleset", state: "open", user: { login: "octocat" }, head: { sha: HEAD, ref: "held" }, mergeable: true, mergeable_state: "blocked", labels: [], assignees: [] }, + [`/repos/${REPO}/pulls/9/commits`]: [], + [`/repos/${REPO}/pulls/9/files`]: [], + [`/repos/${REPO}/pulls/9/reviews`]: [], + [`/repos/${REPO}/issues/9/comments`]: [], [`/repos/${REPO}/commits/${HEAD}/check-runs`]: { check_runs: [] }, [`/repos/${REPO}/commits/${HEAD}/status`]: { statuses: [] }, "/orgs/acme/teams/secret/memberships/octocat": { state: "active" }, @@ -61,6 +66,7 @@ async function github(req: Request): Promise { if (req.method === "GET" && pathname in READS) return Response.json(READS[pathname]); if (pathname === `/repos/${REPO}/installation`) return Response.json({ id: 1 }); if (pathname === "/app") return Response.json({ slug: "triage" }); + if (req.method === "POST" && pathname === "/graphql") return Response.json({ data: { repository: null } }); if (req.method === "GET" && pathname === `/repos/${REPO}/pulls/8/reviews`) { return Response.json(sentReviews.map((review, index) => ({ id: 100 + index, user: { login: "triage[bot]" }, state: "COMMENTED", commit_id: review.commit_id, body: review.body }))); } @@ -186,6 +192,20 @@ test("PostgreSQL: holding read or use on the GitHub credential reads pull reques expect(requested.some((line) => line.startsWith("PUT"))).toBe(false); }, 60_000); +test("PostgreSQL: pull request details ask GitHub for the review decision only when the merge is blocked", async () => { + requested.length = 0; + await prDetails(new Request(`http://hub.test/pull?repo=${REPO}&number=8`), member("usr_member")); + expect(requested).not.toContain("POST /graphql"); + await prDetails(new Request(`http://hub.test/pull?repo=${REPO}&number=9`), member("usr_member")); + expect(requested).toContain("POST /graphql"); +}); + +test("PostgreSQL: a review decision GitHub does not give leaves it unknown and still serves the details", async () => { + const blocked = await prDetails(new Request(`http://hub.test/pull?repo=${REPO}&number=9`), member("usr_member")); + expect(blocked.status).toBe(200); + expect(await blocked.json()).toMatchObject({ pr: { number: 9, mergeableState: "blocked", reviewDecision: null } }); +}); + test("PostgreSQL: pull request details serve only repositories connected to the workspace", async () => { const res = await prDetails(new Request("http://hub.test/pull?repo=acme/other&number=8"), member("usr_member")); expect(res.status).toBe(409); diff --git a/apps/hub/src/github/github-refusal.test.ts b/apps/hub/src/github/github-refusal.test.ts new file mode 100644 index 000000000..bd5d14794 --- /dev/null +++ b/apps/hub/src/github/github-refusal.test.ts @@ -0,0 +1,9 @@ +import { expect, test } from "bun:test"; +import { githubFailed } from "./github-refusal.js"; + +test("a refusal keeps the status and GitHub's reason, never the method or path", async () => { + const err = new Error("github PUT /repos/acme/widgets/pulls/8/merge -> 405 At least 1 approving review is required by reviewers with write access."); + const res = githubFailed("merge acme/widgets#8", err); + expect(res.status).toBe(502); + expect(await res.json()).toEqual({ error: { code: "github_failed", message: "GitHub refused to merge acme/widgets#8 (HTTP 405): At least 1 approving review is required by reviewers with write access." } }); +}); diff --git a/apps/hub/src/github/github-refusal.ts b/apps/hub/src/github/github-refusal.ts index 439cf2f4f..0e9fd882a 100644 --- a/apps/hub/src/github/github-refusal.ts +++ b/apps/hub/src/github/github-refusal.ts @@ -16,11 +16,13 @@ export const GITHUB_VERB = { close: "close", } as const; -/** A host's failure as a sentence; the transport line (method, path, status) is for logs only. */ +/** A host's failure as a sentence with the host's own reason; the method and path are for logs only. */ function refusal(host: string, what: string, err: unknown): string { const message = errorText(err); - const status = /(?:-> |answered )(\d{3})\b/.exec(message)?.[1]; - return `${host} refused to ${what}${status ? ` (HTTP ${status})` : `: ${message}`}.`; + const transport = /-> (\d{3})\b\s*(.*)$/s.exec(message); + if (!transport) return `${host} refused to ${what}: ${message.replace(/\.$/, "")}.`; + const [, status, reason] = transport; + return `${host} refused to ${what} (HTTP ${status})${reason ? `: ${reason.replace(/\.$/, "")}` : ""}.`; } export function githubFailed(what: string, err: unknown): Response { diff --git a/apps/hub/src/github/pr-details.ts b/apps/hub/src/github/pr-details.ts index bbc2955fb..24ba7a221 100644 --- a/apps/hub/src/github/pr-details.ts +++ b/apps/hub/src/github/pr-details.ts @@ -6,7 +6,7 @@ import { eq } from "drizzle-orm"; import { type } from "arktype"; import { schema } from "@intx/db"; import * as bitbucket from "@corbits/bitbucket-datacenter-reads"; -import { appLogin, getChecks, getPr, getReviews, listIssueComments, listPrCommits, listPrFiles, listReferencedIssues, type GithubFetch } from "@corbits/github-reads"; +import { appLogin, getChecks, getPr, getReviews, listIssueComments, listPrCommits, listPrFiles, listReferencedIssues, readReviewDecision, type GithubFetch, type ReviewDecision } from "@corbits/github-reads"; import { errorText } from "@corbits/common"; import { failure, sourceCredentialGrant } from "./portal-credential.js"; import type { Member } from "../integrations/app.js"; @@ -17,7 +17,19 @@ export const GITHUB_PR_DETAILS_PATH = "/api/integrations/github-pull"; const Query = type({ repo: /^[\w.-]+\/[\w.-]+$/, number: "string.integer.parse" }).pipe((q, ctx) => (q.number > 0 ? q : ctx.error("number must be positive"))); -async function githubDetails(gh: GithubFetch, repo: string, number: number) { +type Log = (entry: Record) => void; + +/** Only a blocked merge asks whether a review is what blocks it; a decision GitHub will not give leaves the block unexplained, not the details unread. */ +async function reviewDecisionOf(gh: GithubFetch, repo: string, number: number, log: Log): Promise { + try { + return await readReviewDecision(gh, repo, number); + } catch (err) { + log({ level: "warn", msg: "review_decision_failed", repo, number, error: errorText(err) }); + return null; + } +} + +async function githubDetails(gh: GithubFetch, repo: string, number: number, log: Log) { const [pr, files, commits, comments] = await Promise.all([ getPr(gh, repo, number), listPrFiles(gh, repo, number), @@ -25,14 +37,15 @@ async function githubDetails(gh: GithubFetch, repo: string, number: number) { listIssueComments(gh, repo, number), ]); const text = [pr.title ?? "", pr.body ?? "", ...commits.map((c) => c.message)].join("\n"); - const [issues, checks, reviews] = await Promise.all([ + const [issues, checks, reviews, reviewDecision] = await Promise.all([ listReferencedIssues(gh, repo, number, text), getChecks(gh, repo, pr.sha), getReviews(gh, repo, number, function login() { return appLogin(gh); }), + pr.mergeableState === "blocked" ? reviewDecisionOf(gh, repo, number, log) : null, ]); - return { pr, files, commits, comments, issues, checks, reviews }; + return { pr: { ...pr, reviewDecision }, files, commits, comments, issues, checks, reviews }; } /** Data Center lists changes without patches or line counts and has no issue tracker of its own. */ @@ -48,13 +61,13 @@ async function bitbucketDetails(bb: bitbucket.BitbucketFetch, repo: string, numb return { pr, files: files.map((file) => ({ ...file, additions: 0, deletions: 0 })), commits, comments, issues: [], checks, reviews }; } -export function createGithubPrDetails(deps: SourceFetchDeps & { log: (entry: Record) => void }) { +export function createGithubPrDetails(deps: SourceFetchDeps & { log: Log }) { const sourceFetch = createSourceFetch(deps); async function detailsOf(tenantId: string, principalId: string, record: RepoRecord, number: number) { const source = await sourceFetch(tenantId, principalId, record); if (!source) throw new Error(`no readable ${sourceOf(record)} credential`); - return "bb" in source ? bitbucketDetails(source.bb, record.name, number) : githubDetails(source.gh, record.name, number); + return "bb" in source ? bitbucketDetails(source.bb, record.name, number) : githubDetails(source.gh, record.name, number, deps.log); } return async function handle(req: Request, { tenantId, principalId }: Member): Promise { diff --git a/apps/web/src/lib/hub-api.ts b/apps/web/src/lib/hub-api.ts index b9a621977..93c6013a3 100644 --- a/apps/web/src/lib/hub-api.ts +++ b/apps/web/src/lib/hub-api.ts @@ -1073,6 +1073,14 @@ export type GithubPullDetail = { sha: string; base: string; mergeable: boolean | null; + /** GitHub's `mergeable_state`, such as `blocked` when the branch's rules refuse the merge; absent on Bitbucket. */ + mergeableState?: string; + /** Read only when GitHub blocks the merge; null when no review is required or the read failed. */ + reviewDecision?: "APPROVED" | "CHANGES_REQUESTED" | "REVIEW_REQUIRED" | null; + /** Whether Bitbucket's merge checks allow the merge; absent on GitHub. */ + canMerge?: boolean; + /** Why Bitbucket's merge checks refuse the merge, one summary per check; absent on GitHub. */ + mergeVetoes?: string[]; requestedReviewers: number; additions: number; deletions: number; diff --git a/apps/web/src/lib/inbox-pane.ts b/apps/web/src/lib/inbox-pane.ts index 33f8b7bbc..3cb2cc9e7 100644 --- a/apps/web/src/lib/inbox-pane.ts +++ b/apps/web/src/lib/inbox-pane.ts @@ -11,15 +11,32 @@ export function titleText(title: string | null): string { return title === null ? UNTITLED : title; } -type PaneFacts = { title: string | null; author: string | null; draft: boolean | null; mergeable: boolean | null }; +/** Why the host holds a merge free of conflicts. */ +export type HeldBlock = "behind" | "review-required" | "changes-requested" | "blocked"; + +type HostBlock = "mergeability-unknown" | HeldBlock; + +/** `hostBlock` is known only once the host's detail has loaded. */ +type PaneFacts = { title: string | null; author: string | null; draft: boolean | null; mergeable: boolean | null; hostBlock: HostBlock | null }; + +const GITHUB_BLOCK: Partial> = { unknown: "mergeability-unknown", behind: "behind", blocked: "blocked" }; + +const REVIEW_BLOCK: Partial> = { REVIEW_REQUIRED: "review-required", CHANGES_REQUESTED: "changes-requested" }; + +function hostBlock(pr: GithubPullDetail["pr"]): HostBlock | null { + if (pr.canMerge === false) return "blocked"; + const block = pr.mergeableState === undefined ? undefined : GITHUB_BLOCK[pr.mergeableState]; + if (block === "blocked" && pr.reviewDecision) return REVIEW_BLOCK[pr.reviewDecision] ?? block; + return block ?? null; +} /** Once GitHub's detail has loaded it replaces the verdict's facts outright, so its null mergeable means GitHub is still computing it. */ export function paneFacts(item: PrItem, detail: GithubPullDetail | undefined): PaneFacts { - if (detail === undefined) return { title: item.title, author: item.author, draft: item.draft, mergeable: item.mergeable }; - return { title: detail.pr.title, author: detail.pr.author, draft: detail.pr.draft, mergeable: detail.pr.mergeable }; + if (detail === undefined) return { title: item.title, author: item.author, draft: item.draft, mergeable: item.mergeable, hostBlock: null }; + return { title: detail.pr.title, author: detail.pr.author, draft: detail.pr.draft, mergeable: detail.pr.mergeable, hostBlock: hostBlock(detail.pr) }; } -export type ActionBlocker = "no-number" | "running" | "read-only" | "busy" | "unchecked-commits" | "mergeability-unknown" | "not-mergeable" | "draft"; +export type ActionBlocker = "no-number" | "running" | "read-only" | "busy" | "unchecked-commits" | "not-mergeable" | "draft" | HostBlock; export function hasUncheckedCommits(item: Pick): boolean { const head = item.waitingHeadSha ?? item.headSha; @@ -45,7 +62,7 @@ function actionBlocker(kind: PaneKind, gate: PaneGate): ActionBlocker | null { if (gate.facts.mergeable === null) return "mergeability-unknown"; if (!gate.facts.mergeable) return "not-mergeable"; if (gate.facts.draft === true) return "draft"; - return null; + return gate.facts.hostBlock; } type PaneAction = { kind: K; blocker: ActionBlocker | null }; diff --git a/apps/web/src/lib/pane-report.test.ts b/apps/web/src/lib/pane-report.test.ts index 8cd3e0fe8..19d5396ef 100644 --- a/apps/web/src/lib/pane-report.test.ts +++ b/apps/web/src/lib/pane-report.test.ts @@ -195,3 +195,21 @@ describe("passedChecks", () => { expect(passedChecks([check({ check: "a" }), check({ check: "b", gate: true }), check({ check: "c", gate: true, result: "fail" })]).map((row) => row.check)).toEqual(["a", "b"]); }); }); + +describe("a merge the host blocks", () => { + const ready = { state: "ready", verdict: "pass", ci: "success" } as const; + + test("a pass free of conflicts that a ruleset holds for a review offers no merge and says why", () => { + const live = detail({ mergeable: true, mergeableState: "blocked", reviewDecision: "REVIEW_REQUIRED" }); + expect(barOf(item(ready), live).primary?.blocker).toBe("review-required"); + expect(statusOf(ready, { detail: live })).toMatchObject({ tone: "you", text: "Ready, but waiting for a required review" }); + }); + + test("other blocks on GitHub and Bitbucket's merge checks hold the merge too", () => { + expect(statusOf(ready, { detail: detail({ mergeableState: "blocked", reviewDecision: null }) }).text).toBe("Ready, but the branch's rules block the merge"); + expect(statusOf(ready, { detail: detail({ mergeableState: "behind" }) }).text).toBe("Ready, but behind its base branch"); + expect(statusOf(ready, { detail: detail({ mergeableState: "unknown" }) }).text).toBe("Ready, once GitHub has checked for conflicts"); + expect(statusOf(ready, { detail: detail({ canMerge: false, mergeVetoes: ["Needs 2 approvals"] }) }).text).toBe("Ready, but the merge checks say: Needs 2 approvals"); + expect(statusOf(ready, { detail: detail({ mergeableState: "clean" }) }).text).toBe("Ready to merge"); + }); +}); diff --git a/apps/web/src/lib/pane-report.ts b/apps/web/src/lib/pane-report.ts index 8f375021a..b2ffbe588 100644 --- a/apps/web/src/lib/pane-report.ts +++ b/apps/web/src/lib/pane-report.ts @@ -1,6 +1,6 @@ import { citationOf } from "@corbits/triage-contracts"; import type { CheckResult, GithubPullDetail, PrItem, QueueState, RunFailure } from "./hub-api.ts"; -import { currentHead, hasUncheckedCommits, paneActions, type ActionBlocker, type MenuEntry, type PaneGate, type PaneKind } from "./inbox-pane.ts"; +import { currentHead, hasUncheckedCommits, paneActions, type ActionBlocker, type HeldBlock, type MenuEntry, type PaneGate, type PaneKind } from "./inbox-pane.ts"; import { inboxAction, inboxStatus, isPostedToAuthor, isReReview, postedLabel, primaryLabel, waitsOn, type HandledMark } from "./inbox-view.ts"; import { upperFirst } from "./triage-view.ts"; @@ -53,6 +53,24 @@ const NOT_YET: Partial> = { "mergeability-unknown": "Ready, once GitHub has checked for conflicts", }; +const HELD_REASON: Record = { + behind: "behind its base branch", + "review-required": "waiting for a required review", + "changes-requested": "changes are requested", + blocked: "the branch's rules block the merge", +}; + +export function isHeldBlock(blocker: ActionBlocker | null): blocker is HeldBlock { + return blocker !== null && blocker in HELD_REASON; +} + +/** Why the host holds the merge; Bitbucket's merge checks name their own reasons. */ +export function heldReason(block: HeldBlock, detail: GithubPullDetail | undefined): string { + const vetoes = detail?.pr.mergeVetoes ?? []; + if (block === "blocked" && vetoes.length) return `the merge checks say: ${vetoes.join("; ")}`; + return HELD_REASON[block]; +} + function things(count: number): string { return `${count} ${count === 1 ? "thing" : "things"}`; } @@ -136,7 +154,9 @@ export function paneStatus({ item, detail, handled, approved, primary }: StatusI if (unsure > 0) return status("you", `Not sure about ${things(check)} — your call`); if (check > 0) return status("you", `Couldn't check ${things(check)}: ${problems.filter((problem) => problem.severity === "cant").map((problem) => problem.text ?? problem.check.check).join("; ")}`); if (item.state !== "ready") return status("you", inboxStatus(item)); - const blocked = primary?.kind === "merge" && primary.blocker !== null ? NOT_YET[primary.blocker] : undefined; + const blocker = primary?.kind === "merge" ? primary.blocker : null; + if (isHeldBlock(blocker)) return status("you", `Ready, but ${heldReason(blocker, detail)}`); + const blocked = blocker === null ? undefined : NOT_YET[blocker]; if (blocked !== undefined) return status("you", blocked); return status("ready", approved ? "Approved by Triage · ready for you to merge" : "Ready to merge"); } diff --git a/apps/web/src/pages/PRDetail.tsx b/apps/web/src/pages/PRDetail.tsx index 1e1a4ed21..c653bbc82 100644 --- a/apps/web/src/pages/PRDetail.tsx +++ b/apps/web/src/pages/PRDetail.tsx @@ -11,7 +11,8 @@ import { useApprovals } from "../lib/tenant-entities.ts"; import { QUEUE_STATE_LABEL, type CheckResult, type GithubPullDetail, type PrItem } from "../lib/hub-api.ts"; import { useGithubPull } from "../lib/github-pull.ts"; import { errorText } from "@corbits/common"; -import { approvedHead, checkedHead, hasUncheckedCommits, paneFacts, titleText } from "../lib/inbox-pane.ts"; +import { approvedHead, canRun, checkedHead, hasUncheckedCommits, paneFacts, titleText } from "../lib/inbox-pane.ts"; +import { heldReason, isHeldBlock } from "../lib/pane-report.ts"; import { offersClose, pendingDos, type PendingDo } from "../lib/pending-dos.ts"; import { useApproveDo, useHeldDoKeys } from "../lib/held-dos.ts"; import { ApprovalCard } from "../components/ApprovalCard.tsx"; @@ -30,6 +31,7 @@ import { checkValueText, findPrItem, relativeTime, + upperFirst, verdictText, } from "../lib/triage-view.ts"; @@ -320,10 +322,9 @@ export default function PRDetail() { const selected = useMemo(() => files.find((file) => file.path === filePath) ?? files[0], [files, filePath]); const pending = approval?.status.toLowerCase() === "pending"; const gated = Boolean(item?.needsHuman && pending); - const facts = item === undefined ? null : paneFacts(item, pull.data); - const mergeable = facts === null ? null : facts.mergeable; + const gate = item === undefined ? null : { item, facts: paneFacts(item, pull.data), readOnly, busy }; const closed = pull.data?.pr.state === "closed"; - const canMerge = facts !== null && facts.mergeable === true && facts.draft !== true; + const canMerge = gate !== null && canRun("merge", gate); const [handled, setHandled] = useState>({}); const head = pull.data?.pr.sha ?? item?.sha ?? ""; const handledKey = `${item?.key}@${head}`; @@ -476,8 +477,10 @@ export default function PRDetail() { function mergeBadge(): ReactNode { if (closed) return {pull.data?.pr.merged ? "Merged" : "Closed"}; - if (mergeable === null) return null; - return {mergeable ? "Mergeable" : "Not mergeable"}; + if (gate === null || gate.facts.mergeable === null || gate.facts.hostBlock === "mergeability-unknown") return null; + if (!gate.facts.mergeable) return Not mergeable; + if (isHeldBlock(gate.facts.hostBlock)) return {`Merge blocked · ${upperFirst(heldReason(gate.facts.hostBlock, pull.data))}`}; + return Mergeable; } function sliverText(what: string, loaded: string): string { @@ -566,7 +569,7 @@ export default function PRDetail() { {!closed && ( <> - + diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index 50190c995..278c08656 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -364,7 +364,7 @@ Clicking a pull request opens it on the right. The pane is built to be read top ![The reading pane for a failing pull request: the status line Blocked with 1 thing to fix, the problem Merge conflicts with base, the description, folded rows for linked issue, files and rules passed, and a bottom bar with Request changes, Close, Approve and More](images/12.png) 1. **The header**: repository and number, the title, the author with their chip, when it was opened, and how many lines were added and removed. The **View on GitHub** or **View on Bitbucket** link in the corner opens the same pull request there. The pull request's own page has the same link. -2. **The status line** in one sentence, such as **Ready to merge**, **Waiting for review**, or **Blocked · 2 things to fix, 1 to check**. +2. **The status line** in one sentence, such as **Ready to merge**, **Waiting for review**, or **Blocked · 2 things to fix, 1 to check**. A pull request that passed but that GitHub or Bitbucket will not merge yet says why instead of **Ready to merge**, such as **Ready, but waiting for a required review**, **Ready, but behind its base branch**, **Ready, but the branch's rules block the merge** or, on Bitbucket, the reason its merge checks give, and **Merge** stays off until it can. 3. **The problems**, if any. Each failed rule is listed under **To fix** with the evidence, such as the name of the failing check. A rule Triage could not evaluate is listed under **Couldn't check**. A pull request with no problems skips straight to the description. 4. **Code review**, when it is on for the repository: what the change does, then each bug it found as a comment from **Code review**, with the commit it reviewed, how severe it is (P0 is the most severe), the file and line, and the code it points at. A bug in a file code review could not read says **Not checked against the file**, here and on GitHub. If the reviewer's answer can't be read, the review shows **The last review did not finish** instead of **No findings**, with the reason under it. **Review again** reviews the latest commit once more. On a GitHub pull request, **Send to GitHub** posts the review to the pull request, inline where the lines are part of the change; it then says **Sent to GitHub**. It needs the Comment permission for the repository, and is refused once the pull request has newer commits: review again, then send. 5. **The description** the author wrote. @@ -385,7 +385,7 @@ The bar at the bottom of the pane always offers the next step first. For a passi ![The More menu open at the bottom of the reading pane, listing Edit feedback, Post reply, Comment, Merge and Triage again](images/15.png) -- **Merge** merges the exact commit Triage checked. If the author pushes a new commit in the meantime, the merge is refused and Triage checks the new commit first. +- **Merge** merges the exact commit Triage checked. If the author pushes a new commit in the meantime, the merge is refused and Triage checks the new commit first. Merge is off while GitHub or Bitbucket would not merge the pull request, here and on the pull request's own page, which shows the reason beside its status, such as **Merge blocked · Waiting for a required review**. If the host refuses a merge anyway, the message gives its reason. - **Request changes** sends the author a review listing the rules they need to fix. When they fix them, Triage withdraws the request on its own. - **Approve** approves the pull request on GitHub or Bitbucket. Triage never withdraws an approval you made yourself. - **Comment** posts a comment, and **Post reply** answers the author. **Edit feedback** lets you reword the comment Triage will post before it goes out. diff --git a/packages/github-reads/src/index.ts b/packages/github-reads/src/index.ts index f277a939b..7281c37d3 100644 --- a/packages/github-reads/src/index.ts +++ b/packages/github-reads/src/index.ts @@ -18,12 +18,22 @@ export function isTriageComment(body: string): boolean { return body.startsWith(TRIAGE_COMMENT_PREFIX); } +/** GitHub's reason for a refusal, such as the review a ruleset requires; empty when the body gives none. */ +async function errorMessage(res: Response): Promise { + try { + const body = await res.json() as { message?: unknown }; + return typeof body.message === "string" ? body.message : ""; + } catch { + return ""; + } +} + async function request(gh: GithubFetch, path: string, init?: RequestInit): Promise { const res = await gh(path, { ...init, headers: { accept: "application/vnd.github+json", ...init?.headers }, }); - if (!res.ok) throw new Error(`github ${init?.method ?? "GET"} ${path} -> ${res.status}`); + if (!res.ok) throw new Error(`github ${init?.method ?? "GET"} ${path} -> ${res.status} ${await errorMessage(res)}`.trimEnd()); return res; } @@ -351,6 +361,18 @@ export async function getReviews(gh: GithubFetch, repo: string, number: number, */ export type Review = { reviewer?: string; state?: string; submittedAt?: string; commitId?: string; triageRequest?: true; codeReview?: true }; +export type ReviewDecision = "APPROVED" | "CHANGES_REQUESTED" | "REVIEW_REQUIRED"; + +const ReviewDecisionReply = type({ data: { repository: { pullRequest: { reviewDecision: "'APPROVED' | 'CHANGES_REQUESTED' | 'REVIEW_REQUIRED' | null" } } } }); + +/** Whether the branch's rules still want a review, which only GraphQL says; null when no review is required. */ +export async function readReviewDecision(gh: GithubFetch, repo: string, number: number): Promise { + const [owner, name] = repo.split("/"); + const query = "query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { reviewDecision } } }"; + const reply = await json(gh, "/graphql", { method: "POST", body: JSON.stringify({ query, variables: { owner, name, number } }), headers: { "content-type": "application/json" } }); + return ReviewDecisionReply.assert(reply).data.repository.pullRequest.reviewDecision; +} + /** Reviewers whose latest review still stands on the current head, when the review names its commit. */ export function reviewedOnHead(reviews: Review[], headSha: string): string[] { const latest = new Map(); diff --git a/packages/github-tool/src/github.test.ts b/packages/github-tool/src/github.test.ts index fd4d818db..daddf7a17 100644 --- a/packages/github-tool/src/github.test.ts +++ b/packages/github-tool/src/github.test.ts @@ -284,6 +284,15 @@ describe("mergePr", () => { await expect(mergePr(gh, { repo: "acme/widgets", number: 8 })).rejects.toThrow("pull request is not mergeable"); expect(requests).toEqual([{ path: "/repos/acme/widgets/pulls/8", method: "GET", body: null }]); }); + + test("a merge the branch's rules refuse fails with GitHub's own reason", async () => { + const reason = "At least 1 approving review is required by reviewers with write access."; + const { gh } = recorder(function respond(_path, init) { + if (init?.method === "PUT") return Response.json({ message: reason }, { status: 405 }); + return { mergeable: true, mergeable_state: "blocked", head: { sha: "abc123" }, state: "open" }; + }); + await expect(mergePr(gh, { repo: "acme/widgets", number: 8 })).rejects.toThrow(`github PUT /repos/acme/widgets/pulls/8/merge -> 405 ${reason}`); + }); }); describe("sendCodeReview", () => { diff --git a/packages/github-tool/src/github.ts b/packages/github-tool/src/github.ts index 7f3bb48c1..3cdec7fec 100644 --- a/packages/github-tool/src/github.ts +++ b/packages/github-tool/src/github.ts @@ -367,6 +367,7 @@ export type MergePrInput = { sha?: string; }; +// Merge checks are GitHub's to make: a ruleset's bypass actors may merge what mergeable_state calls blocked. export async function mergePr(gh: GithubFetch, input: MergePrInput) { const pr = await getPr(gh, input.repo, input.number); if (pr.mergeable !== true) throw new Error("pull request is not mergeable"); diff --git a/packages/triage-workflows/src/pr-triage.test.ts b/packages/triage-workflows/src/pr-triage.test.ts index f1df67770..c6af36a50 100644 --- a/packages/triage-workflows/src/pr-triage.test.ts +++ b/packages/triage-workflows/src/pr-triage.test.ts @@ -773,7 +773,7 @@ describe("pr-triage body", () => { const { dos, verdict } = await triage({ ...emptyPack(REPO), actions: HELLO }, { failComments: true }); const effectId = ((verdict as Verdict).actions[0] as SuggestedDo).effectId; expect((verdict as Verdict).actions).toEqual([expect.objectContaining({ list: "pass", effectId })]); - expect(dos).toEqual([{ effectId, status: "failed", error: `github POST /repos/${REPO}/issues/8/comments -> 500` }]); + expect(dos).toEqual([{ effectId, status: "failed", error: `github POST /repos/${REPO}/issues/8/comments -> 500 Server Error` }]); }); test("facts that fail degrade the verdict and suggest no actions", async () => {