diff --git a/packages/bitbucket-datacenter-tool/src/action.ts b/packages/bitbucket-datacenter-tool/src/action.ts index 024ac5fa..e3225c67 100644 --- a/packages/bitbucket-datacenter-tool/src/action.ts +++ b/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); diff --git a/packages/github-reads/src/index.ts b/packages/github-reads/src/index.ts index bbc0ac50..eb0612ee 100644 --- a/packages/github-reads/src/index.ts +++ b/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,9 +369,7 @@ 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"; @@ -378,23 +377,6 @@ export function summarizeChecks(runs: CheckRun[]): CiState { 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 = { - 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 { const latest = new Map(); diff --git a/packages/github-tool/src/do.ts b/packages/github-tool/src/do.ts index b1f1851f..8f2ca28c 100644 --- a/packages/github-tool/src/do.ts +++ b/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; diff --git a/packages/triage-contracts/package.json b/packages/triage-contracts/package.json index 674ced25..f9195efe 100644 --- a/packages/triage-contracts/package.json +++ b/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" }, diff --git a/packages/triage-contracts/src/index.ts b/packages/triage-contracts/src/index.ts index 631d5ffd..2b080b3d 100644 --- a/packages/triage-contracts/src/index.ts +++ b/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"; diff --git a/packages/triage-contracts/src/merge-gate.ts b/packages/triage-contracts/src/merge-gate.ts new file mode 100644 index 00000000..5d48d9de --- /dev/null +++ b/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 = { + 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; 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) }; +} diff --git a/packages/triage-workflows/src/logic/actions.test.ts b/packages/triage-workflows/src/logic/actions.test.ts index 535544f4..91c62ffd 100644 --- a/packages/triage-workflows/src/logic/actions.test.ts +++ b/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" }); diff --git a/packages/triage-workflows/src/logic/actions.ts b/packages/triage-workflows/src/logic/actions.ts index 3f15a034..9ad86009 100644 --- a/packages/triage-workflows/src/logic/actions.ts +++ b/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): SuggestedAction[] { +export function heldApprovals(actions: SuggestedAction[], approval: ApprovalHold | undefined, { checks: ci, changesRequested, mergeHold }: Pick): 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; }); } diff --git a/tests/model/rules.postgres.test.ts b/tests/model/rules.postgres.test.ts index 73f344ae..8dc6f9c5 100644 --- a/tests/model/rules.postgres.test.ts +++ b/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."); +});