diff --git a/cloudflare/GITHUB.md b/cloudflare/GITHUB.md index 0d74374c..eaaa9c7a 100644 --- a/cloudflare/GITHUB.md +++ b/cloudflare/GITHUB.md @@ -161,10 +161,11 @@ URL, or closing keyword. Every delivery renders the section again between past GitHub's 65,536-character body limit are counted rather than cut. GitHub keeps pull request status, checks, and reviews. `mergePullRequest` -merges only an exact head a maintainer explicitly chose, with the first method -the repository allows: squash, then a merge commit, then rebase. GitHub's -branch rules and refusals stand, and an agent's recommendation is never -approval. The decision inbox offers it on a pull request item only when +merges only an exact head a maintainer explicitly chose, once a review counts +on it (see [Landing only reviewed heads](#landing-only-reviewed-heads)), with +the first method the repository allows: squash, then a merge commit, then +rebase. GitHub's branch rules and refusals stand, and an agent's +recommendation is never approval. The decision inbox offers it on a pull request item only when GitHub reports the PR's merge state as `CLEAN` or `HAS_HOOKS` and no job or session holds the PR. The form binds the head on screen. A moved head asks for another look, GitHub's refusal is shown in its own words, and neither is @@ -194,15 +195,73 @@ on a decision. refuse auto-merge because nothing is left to wait for. It uses the method the job names, else the first the repository allows. A base that requires the `quest land` check queues the PR for that gate instead, as `quest merge` - does: squash auto-merge whose commit body pins the head. A refusal - pauses the job on a decision, in GitHub's words. + does: squash auto-merge whose commit body pins the head. Either way, kix + first requires a review that counts on that head; without one, the job + pauses on a decision that says why. A refusal also pauses the job on a + decision, in GitHub's words. - The label stays while the job runs or waits, and while the PR is queued or auto-merging. When the job ends without a landing in progress, or a `pull_request` webhook arrives and GitHub shows the landing ended unmerged, the App removes its own label. The PR is then an idle decision item again. -Labels need `issues: write` on the upstream; landing needs `contents: write` -and `pull_requests: write` there, as a direct merge does. +Labels need `issues: write` on the upstream; landing needs `contents: write`, +`pull_requests: write`, and `checks: read` there, as a direct merge does. + +### Landing only reviewed heads + +Picking merge is a maintainer's decision about the head they saw, not a review +of whatever a runner, or anyone, pushes after it. So both a direct merge and a +merge job's landing first require a review that counts on the exact head being +landed (`src/github/review.ts`): + +- an approval on GitHub of that commit by a person with write access other than + the pull request's author; reviews by Apps never count; +- a completed, successful `quest review` check run on that commit, written by + this App. Only the App can write one, and GitHub binds it to the commit; +- or the author's own `## Review` record, as the guide's default review policy + counts it in every enrolled repository: the `## Review` section of the PR + body names the commit's full SHA, the author has write access, and the body's + latest edit is the author's. The record is self-reported, so it never counts + on a commit GitHub attributes to an App's bot, as it does every commit a + runner publishes; a runner's head needs one of the other two. + +A reviewer's suggested patch applied on GitHub is its applier's own commit, so +the head it makes needs a new review. + +A review also carries to a head whose only changes since the reviewed commit +are merges of commits the default branch already holds that resolved nothing: +every file holds the reviewed side's version or the default branch's, whichever +changed it. kix verifies this from the commits' trees, up to five merges back. +A file both sides changed, even one Git merged line by line without a +conflict, needs a new review, as does any other commit, a truncated tree, or a +read that fails. +An author's record carries only across merges no App committed. + +kix defers to GitHub only where GitHub enforces the same rule: a ruleset on the +base that requires an approval and either dismisses stale approvals or requires +approval of the most recent push, which GitHub reports this App cannot bypass. +Classic branch protection's review settings need the administration permission +to read, so kix keeps its own check there. + +When no review counts, a direct merge is refused and a merge job pauses on a +decision. Both say which commit needs a review and how to give one. + +GitHub keeps auto-merge enabled across a push by anyone with write access, the +App's included, and checks the `expectedHeadOid` kix pins only when auto-merge +is enabled. Its merge queue likewise keeps a queued pull request across such a +push. So once a merge job arms auto-merge on a reviewed head while its checks +run, whatever a writer pushes after it lands with it, unreviewed. Only the +`quest land` gate's pin, or a ruleset GitHub enforces that requires a new +approval of every push, refuses such a head. The maintainer accepted this gap +on 2026-11-02, matching `quest merge --land`, rather than make a merge job wait +for its checks and merge only at once. + +kix closes it for its own pushes: a job pushing to a pull request that is +auto-merging would land its new head with no review of it. Before a job moves +or resets a branch that heads an open pull request, its own or an iterated +one, kix turns that pull request's auto-merge off. If GitHub refuses or does +not show whether it is on, the job pushes nothing and pauses on a decision. The +new head lands only through a new landing. kix pull requests come from upstream branches, and GitHub gives workflows on same-repository branches the repository's secrets, unlike fork pull requests. @@ -217,11 +276,12 @@ grants runners no GitHub credentials. Installation grants are broader than any one publication: `contents: write` and `pull_requests: write` cover every installed repository. Each token names one repository ID and only the permissions that step needs. Branch writes get -`contents: write` on the upstream alone, and pull request delivery gets -`pull_requests: write` there. That token could write any unprotected upstream -branch; the service restricts it to the job's branch or the pull request's head -and refuses the default branch. Repositories should still protect their default -branch. +`contents: write` on the upstream alone, plus `pull_requests: write` to turn +off the auto-merge of a pull request the branch heads, and pull request +delivery gets `pull_requests: write` there. That token could write any +unprotected upstream branch; the service restricts it to the job's branch or +the pull request's head and refuses the default branch. Repositories should +still protect their default branch. Mocked tests cover this path. These GitHub behaviors still need a live check against real installations ([operator setup](../quest/m0/kix/operator-setup.md)): diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index 1ebb75b1..5b6f471a 100644 --- a/cloudflare/dev/github.ts +++ b/cloudflare/dev/github.ts @@ -24,15 +24,19 @@ export type Pull = Issue & { branch: string; checks: "SUCCESS" | "FAILURE" | "PENDING" | null; /** The check runs behind `checks`. */ - runs?: { name: string; conclusion: string; url: string; app?: number; context?: boolean }[]; + /** `commit` is the one a run reports on; the head by default. */ + runs?: { name: string; conclusion: string; url: string; app?: number; context?: boolean; commit?: string }[]; review: "APPROVED" | "CHANGES_REQUESTED" | "REVIEW_REQUIRED" | null; - reviews?: { state: string; author: string; body?: string }[]; + /** Oldest first; `commit` is the one a review was submitted on, the head by default. */ + reviews?: { state: string; author: string; body?: string; commit?: string }[]; threads?: Thread[]; draft?: boolean; /** Opened from this contributor's own fork, which the bot cannot update. */ contributor?: string; /** A `## Decisions needed` section in it hands a draft back to the maintainer. */ body?: string; + /** Who last edited the body; never edited when absent. */ + editor?: string; /** Closed by merging it; set with `closed`. */ merged?: boolean; /** Optional squash result supplied by fixtures that model real git merges. */ @@ -63,6 +67,8 @@ export type Commit = { parents: string[]; tree: string; message: string; + /** The account GitHub attributes the commit to; the App's bot for commits written through the API. */ + committer?: string; /** GitHub's authenticated signature result, independent of author identity. */ signature?: { isValid: boolean; signer: { databaseId: number; login: string } | null } | null; }; @@ -74,6 +80,8 @@ export type Repo = { name: string; /** Logins with the maintain role; everyone else can only read. */ maintainers: string[]; + /** Logins that can write without maintaining, whose approvals count as GitHub's writers' do. */ + writers?: string[]; /** Defaults to main. */ defaultBranch?: string; /** The default branch's files. */ @@ -100,6 +108,8 @@ export type Repo = { rules?: Record[]; /** The default branch's classic protection, as `GET /branches/main/protection` returns it; none when absent. */ protection?: Record; + /** Rulesets by ID, as `GET /rulesets/` shows them to the App. */ + rulesets?: Record; }; export type World = { app: { id: number; slug: string }; @@ -245,6 +255,34 @@ function pullTarget(repo: Repo, pull: Pull, after: unknown) { }; } +const actor = (login: string) => ({ __typename: login.endsWith("[bot]") ? "Bot" : "User", login }); + +/** A pull request's author and each writer's latest approval or change request, as the review gate reads them. */ +function reviewTarget(repo: Repo, pull: Pull) { + const latest = new Map[number]>(); + for (const review of pull.reviews ?? []) + if ( + ["APPROVED", "CHANGES_REQUESTED", "DISMISSED"].includes(review.state) && + [...repo.maintainers, ...(repo.writers ?? [])].includes(review.author) + ) + latest.set(review.author, review); + return { + state: pull.merged ? "MERGED" : pull.closed ? "CLOSED" : "OPEN", + headRefOid: headOf(repo, pull), + baseRefName: pull.base ?? "main", + body: pull.body ?? "", + author: actor(pull.author), + userContentEdits: { nodes: pull.editor ? [{ editor: actor(pull.editor) }] : [] }, + latestOpinionatedReviews: { + nodes: [...latest.values()].map((r) => ({ + state: r.state, + author: actor(r.author), + commit: { oid: r.commit ?? headOf(repo, pull) }, + })), + }, + }; +} + /** A pull request's labels, the timeline that added them, and its landing, as a merge hold reads them. */ function holdsTarget(repo: Repo, pull: Pull) { return { @@ -393,6 +431,10 @@ function graphql(repo: Repo, query: string, variables: Record) }; } + if (query.includes("latestOpinionatedReviews(")) { + const pull = repo.pulls.find((p) => p.number === variables.number); + return { data: { repository: { pullRequest: pull ? reviewTarget(repo, pull) : null } } }; + } if (query.includes("timelineItems(")) { const pull = repo.pulls.find((p) => p.number === variables.number); return { data: { repository: { pullRequest: pull ? holdsTarget(repo, pull) : null } } }; @@ -808,13 +850,26 @@ export function fakeGitHub(world: World) { if (permission) { const found = world.users.find((u) => u.login === permission[1]); if (!found) return notFound(); - const role = repo.maintainers.includes(found.login) ? "maintain" : "read"; + const role = repo.maintainers.includes(found.login) + ? "maintain" + : repo.writers?.includes(found.login) + ? "write" + : "read"; return json({ user: { id: found.id }, - permission: role === "maintain" ? "write" : "read", + permission: role === "read" ? "read" : "write", role_name: role, }); } + const listed = tail.match(/^\/commits\/([0-9a-f]{40})$/)?.[1]; + if (listed && method === "GET") { + const found = repo.commits[listed]; + if (!found) return notFound(); + return json({ + sha: listed, + committer: found.committer ? { login: found.committer, type: actor(found.committer).__typename } : null, + }); + } if (tail === "/commits/main") return json({ sha: commitOf(repo), commit: { tree: { sha: treeOf(repo) } } }); // Read access sees whether classic protection is on and its required checks; the rest needs administration. if (tail === "/branches/main" && method === "GET") { @@ -837,6 +892,32 @@ export function fakeGitHub(world: World) { if (!scope.permissions.administration) return forbidden(); return repo.protection ? json(repo.protection) : json({ message: "Branch not protected" }, 404); } + const ruleset = tail.match(/^\/rulesets\/(\d+)$/); + if (ruleset && method === "GET") { + const found = repo.rulesets?.[Number(ruleset[1])]; + return found ? json({ id: Number(ruleset[1]), ...found }) : notFound(); + } + const runs = tail.match(/^\/commits\/([0-9a-f]{40})\/check-runs$/); + if (runs && method === "GET") { + if (!scope.permissions.checks) return forbidden(); + const name = url.searchParams.get("check_name"); + const app = url.searchParams.get("app_id"); + const found = repo.pulls.flatMap((pull) => + (pull.runs ?? []) + .filter((run) => !run.context && (run.commit ?? headOf(repo, pull)) === runs[1]) + .map((run) => ({ + name: run.name, + head_sha: runs[1], + status: "completed", + conclusion: run.conclusion.toLowerCase(), + app: { id: run.app ?? ACTIONS_APP }, + })), + ); + const check_runs = found.filter( + (run) => (!name || run.name === name) && (!app || String(run.app.id) === app), + ); + return json({ total_count: check_runs.length, check_runs }); + } if (tail.startsWith("/rules/branches/main") && method === "GET") { const contexts = (repo.required ?? []).map((context) => ({ context })); return json([ @@ -990,13 +1071,18 @@ export function fakeGitHub(world: World) { pull.labels = current.filter((label) => label !== name); return json(pull.labels.map((label) => ({ name: label }))); } - const compare = tail.match(/^\/compare\/([0-9a-f]{40})\.\.\.main$/); + const compare = tail.match(/^\/compare\/([0-9a-f]{40})\.\.\.(main|[0-9a-f]{40})$/); if (compare && method === "GET") { - const base = repo.branches.main ?? commitOf(repo); const from = compare[1]!; - if (!repo.commits[from]) return notFound(); - const reaches = (sha: string, target: string): boolean => - sha === target || (repo.commits[sha]?.parents ?? []).some((parent) => reaches(parent, target)); + const base = compare[2] === "main" ? (repo.branches.main ?? commitOf(repo)) : compare[2]!; + if (!repo.commits[from] || !repo.commits[base]) return notFound(); + const ancestors = (tip: string, seen = new Set()): Set => { + if (seen.has(tip)) return seen; + seen.add(tip); + for (const parent of repo.commits[tip]?.parents ?? []) ancestors(parent, seen); + return seen; + }; + const reaches = (sha: string, target: string) => ancestors(sha).has(target); const status = from === base ? "identical" @@ -1005,7 +1091,10 @@ export function fakeGitHub(world: World) { : reaches(from, base) ? "behind" : "diverged"; - return json({ status }); + // The nearest common ancestor: one no other common ancestor descends from. + const common = [...ancestors(from)].filter((sha) => ancestors(base).has(sha)); + const nearest = common.find((sha) => !common.some((other) => other !== sha && reaches(other, sha))); + return json({ status, merge_base_commit: nearest ? { sha: nearest } : null }); } if (tail === "/contents/.github" && method === "GET") { const tree = repo.commits[url.searchParams.get("ref") ?? ""]?.tree; @@ -1124,7 +1213,13 @@ export function fakeGitHub(world: World) { return invalid("Invalid commit"); // GitHub stamps each commit with the time, so even identical changes get a new commit. const sha = sha1(`commit ${Object.keys(network.commits).length} ${body.tree} ${parents} ${body.message}`); - network.commits[sha] = { parents, tree: body.tree, message: body.message }; + // No author or committer: GitHub attributes the commit to the App's bot. + network.commits[sha] = { + parents, + tree: body.tree, + message: body.message, + committer: `${world.app.slug}[bot]`, + }; return json({ sha }, 201); } return null; diff --git a/cloudflare/dev/seed.ts b/cloudflare/dev/seed.ts index 37841327..e78a574e 100644 --- a/cloudflare/dev/seed.ts +++ b/cloudflare/dev/seed.ts @@ -27,6 +27,7 @@ export function world(): World { owner: "example", name: "rocket", maintainers: ["dev-maintainer"], + writers: ["reviewer"], files: [ { path: "flake.nix", content: "{ outputs = _: { }; }\n" }, { diff --git a/cloudflare/src/github/review.ts b/cloudflare/src/github/review.ts new file mode 100644 index 00000000..d4c9bd3f --- /dev/null +++ b/cloudflare/src/github/review.ts @@ -0,0 +1,231 @@ +// kix lands a pull request only at a head with a review that counts on that +// exact commit, whether a maintainer merges it from the inbox or a merge job +// lands it. A maintainer's earlier pick never stands in for a review of what a +// runner, or anyone, pushed since. Everything read here is GitHub's own record, +// except the `## Review` record a write-access author keeps in their own pull +// request's body, which counts only on commits no App committed. + +/** The check run a runner's `quest review` result becomes, written only by the App. */ +export const REVIEW_CHECK = "quest review"; +/** How many merges of the base since its review a head carries the review across; each costs about ten reads. */ +const MERGES = 5; + +/** Reads on the pull request's repository, through the caller's scoped token. */ +export type Reads = { + /** A REST read below `/repos//`, such as `/pulls/1`. */ + get(path: string): Promise; + graphql(query: string, variables: Record): Promise; +}; + +export type Landing = { + owner: string; + name: string; + pull: number; + /** The exact head being landed. */ + head: string; + /** The repository's default branch: a merge carries a review only when it merges commits already there. */ + branch: string; + /** The GitHub App's ID: only its check runs count. */ + app: number; +}; + +type Actor = { __typename: string; login: string } | null; +const PULL = `query($owner:String!,$name:String!,$number:Int!) { repository(owner:$owner,name:$name) { pullRequest(number:$number) { + state headRefOid baseRefName body author { __typename login } + userContentEdits(first:1) { nodes { editor { __typename login } } } + latestOpinionatedReviews(first:100,writersOnly:true) { nodes { state author { __typename login } commit { oid } } } } } }`; +type PullRow = { + state: string; + headRefOid: string; + baseRefName: string; + body: string; + author: Actor; + /** The latest edit of the body first, whose editor wrote what it says now; none when it was never edited. */ + userContentEdits: { nodes: { editor: Actor }[] } | null; + latestOpinionatedReviews: { nodes: { state: string; author: Actor; commit: { oid: string } | null }[] }; +}; +type Rule = { type: string; ruleset_id?: number; parameters?: Record }; +type GitCommit = { sha: string; tree: { sha: string }; parents: { sha: string }[] }; +type GitTree = { truncated: boolean; tree: { path: string; type: string; mode: string; sha: string }[] }; + +const short = (sha: string) => sha.slice(0, 7); + +/** + * Why `landing.head` cannot land yet, or null when a review counts on it: + * + * - GitHub enforces one itself: a ruleset on the base the App cannot bypass + * requires an approval and dismisses stale ones or requires approval of the + * last push. Classic branch protection's review settings need the + * administration permission to read, so kix keeps its own check there. + * - Someone with write access other than the pull request's author approved + * the head on GitHub, as a person rather than an App. + * - The App's `quest review` check run passed on the head. + * - The pull request's author, who has write access, names the head's full SHA + * in the `## Review` section of the body they last edited themselves, as the + * guide's default review policy counts it, and no App committed the head. + * + * Any of these also counts across merges of the default branch since the + * reviewed commit that resolved no conflict: every path holds the reviewed + * side's or the default branch's version, whichever changed it, so the merge + * adds nothing the review or the branch did not already hold. + */ +export async function unreviewed(reads: Reads, landing: Landing): Promise { + const response = await reads.graphql<{ + data?: { repository: { pullRequest: PullRow | null } | null }; + errors?: unknown[]; + }>(PULL, { owner: landing.owner, name: landing.name, number: landing.pull }); + const pull = response.data?.repository?.pullRequest; + if (response.errors?.length || !pull) return "kix could not read the pull request's reviews."; + if (pull.state !== "OPEN") return "The pull request is no longer open."; + if (pull.headRefOid !== landing.head) return "The pull request's head moved since you looked; review it again."; + if (await enforced(reads, pull.baseRefName)) return null; + const author = pull.author?.login.toLowerCase(); + const approved = new Set( + pull.latestOpinionatedReviews.nodes.flatMap((review) => + review.state === "APPROVED" && + review.author?.__typename === "User" && + review.author.login.toLowerCase() !== author && + review.commit + ? [review.commit.oid] + : [], + ), + ); + const recorded = await ownRecord(reads, pull); + const commits = new Map>(); + const commit = (sha: string) => { + if (!commits.has(sha)) commits.set(sha, reads.get(`/git/commits/${sha}`)); + return commits.get(sha)!; + }; + let reviewed: string | null = landing.head; + // Whether an App committed the head or a merge between it and `reviewed`: a runner's + // commit needs a review the runner cannot write, so no body record counts past it. + let appCommitted = false; + for (let merges = 0; reviewed && merges <= MERGES; merges++) { + if (approved.has(reviewed) || (await checked(reads, landing.app, reviewed))) return null; + if (recorded.size && !appCommitted) { + appCommitted = await botCommitted(reads, reviewed); + if (!appCommitted && recorded.has(reviewed)) return null; + } + // A merge kix cannot read in full is one it cannot verify, so it carries no review. + reviewed = merges < MERGES ? await baseMerge(reads, commit, landing.branch, reviewed).catch(() => null) : null; + } + const head = short(landing.head); + const ask = `kix lands only a reviewed head. Approve ${head} on GitHub as someone with write access other than the pull request's author, have a runner's \`${REVIEW_CHECK}\` check pass on it, or, as its author with write access, name it in the body's \`## Review\` record if no App committed it.`; + if (!approved.size) return `No review counts on ${head} yet. ${ask}`; + return `The approval of ${[...approved].map(short).join(", ")} does not carry to ${head}: what changed since is more than conflict-free merges of ${landing.branch}. ${ask}`; +} + +/** + * The full SHAs the `## Review` section of the pull request's body names, + * when its author has write access and wrote the body as it stands; else none. + * The record is self-reported, so it counts only as the author's own word. + */ +async function ownRecord(reads: Reads, pull: PullRow): Promise> { + const author = pull.author; + if (author?.__typename !== "User") return new Set(); + const editor = pull.userContentEdits?.nodes[0]?.editor; + if (editor !== undefined && editor?.login.toLowerCase() !== author.login.toLowerCase()) return new Set(); + const section = /^##[ \t]+Review[ \t]*$([\s\S]*?)(?=^##?[ \t]|(?![\s\S]))/m.exec(pull.body.replace(/\r/g, "")); + const named = new Set(section?.[1]!.match(/\b[0-9a-f]{40}\b/g) ?? []); + if (!named.size) return named; + try { + const { permission } = await reads.get<{ permission: string }>( + `/collaborators/${encodeURIComponent(author.login)}/permission`, + ); + return permission === "admin" || permission === "write" ? named : new Set(); + } catch { + // An author kix cannot confirm can write is one whose word it does not take. + return new Set(); + } +} + +/** Whether GitHub attributes `sha`'s commit to an App, as it does every commit a runner publishes through kix. */ +async function botCommitted(reads: Reads, sha: string) { + const { committer } = await reads.get<{ committer: { type: string } | null }>(`/commits/${sha}`); + return committer?.type === "Bot"; +} + +/** Whether a ruleset the App cannot bypass makes GitHub itself require a review of the current head. */ +async function enforced(reads: Reads, base: string) { + const rules = await reads.get(`/rules/branches/${encodeURIComponent(base)}?per_page=100`); + for (const rule of rules) { + const p = rule.parameters ?? {}; + if ( + rule.type !== "pull_request" || + !(typeof p.required_approving_review_count === "number" && p.required_approving_review_count > 0) || + !(p.dismiss_stale_reviews_on_push === true || p.require_last_push_approval === true) || + typeof rule.ruleset_id !== "number" + ) + continue; + try { + const ruleset = await reads.get<{ current_user_can_bypass?: string }>(`/rulesets/${rule.ruleset_id}`); + if (ruleset.current_user_can_bypass === "never") return true; + } catch { + // A ruleset kix cannot read is one it cannot rely on; its own check stands. + } + } + return false; +} + +/** Whether the App's `quest review` check run passed on `sha`. */ +async function checked(reads: Reads, app: number, sha: string) { + const { check_runs } = await reads.get<{ + check_runs: { + name: string; + head_sha: string; + status: string; + conclusion: string | null; + app: { id: number } | null; + }[]; + }>( + `/commits/${sha}/check-runs?check_name=${encodeURIComponent(REVIEW_CHECK)}&app_id=${app}&filter=latest&per_page=100`, + ); + return check_runs.some( + (run) => + run.name === REVIEW_CHECK && + run.app?.id === app && + run.head_sha === sha && + run.status === "completed" && + run.conclusion === "success", + ); +} + +/** + * The reviewed side of `sha` when it is a merge of commits the default branch + * already holds that resolved no conflict, or else null. + */ +async function baseMerge( + reads: Reads, + commit: (sha: string) => Promise, + branch: string, + sha: string, +): Promise { + const merge = await commit(sha); + if (merge.parents.length !== 2) return null; + const [ours, theirs] = merge.parents.map((parent) => parent.sha) as [string, string]; + const onBranch = await reads.get<{ status: string }>(`/compare/${theirs}...${encodeURIComponent(branch)}`); + if (onBranch.status !== "identical" && onBranch.status !== "ahead") return null; + const { merge_base_commit } = await reads.get<{ merge_base_commit: { sha: string } | null }>( + `/compare/${ours}...${theirs}`, + ); + if (!merge_base_commit) return null; + const files = async (tree: string) => { + const read = await reads.get(`/git/trees/${tree}?recursive=1`); + if (read.truncated) return null; + return new Map(read.tree.filter((e) => e.type !== "tree").map((e) => [e.path, `${e.mode} ${e.sha}`])); + }; + const [base, a, b, merged] = await Promise.all( + [merge_base_commit.sha, ours, theirs] + .map(async (parent) => files((await commit(parent)).tree.sha)) + .concat(files(merge.tree.sha)), + ); + if (!base || !a || !b || !merged) return null; + for (const path of new Set([...base.keys(), ...a.keys(), ...b.keys(), ...merged.keys()])) { + const [o, x, y] = [base.get(path), a.get(path), b.get(path)]; + // Both sides changed the path differently: whatever the merge holds was resolved by hand or by Git's + // line merge, which a review of either side never saw. + const expected = x === y ? x : x === o ? y : y === o ? x : null; + if (expected === null || merged.get(path) !== expected) return null; + } + return ours; +} diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 8166d483..0cdefb16 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -2,6 +2,7 @@ import { createHash, createPrivateKey, sign } from "node:crypto"; import { HOLD_PREFIX, isHoldLabel, latestHold, heldToken, check, isQuest, parse, ready } from "quest/core"; import { cleanAutoMergeRefusal, immediateMerge } from "../../../src/github/merge"; import { osiApproved } from "../licenses"; +import { unreviewed } from "./review"; import { positiveId } from "./webhook"; import { activityQuery, @@ -1413,7 +1414,9 @@ export class GitHubService { /** * Hand `action` the one upstream branch a job may write: its own branch, - * or an iterated pull request's head on the enrolled repository. + * or an iterated pull request's head on the enrolled repository. Before + * moving either, it turns off the auto-merge of any open pull request the + * branch heads. * Rechecks the requester's current authority and the project's enrollment. */ contribute( @@ -1426,7 +1429,7 @@ export class GitHubService { return this.#authorized( upstream, userId, - pulled ? { contents: "read", pull_requests: "read" } : { contents: "write" }, + pulled ? { contents: "read", pull_requests: "read" } : { contents: "write", pull_requests: "write" }, async (token, current, installation) => { this.#public(current); if ((await this.#state(upstream, installation)) === null) @@ -1446,7 +1449,16 @@ export class GitHubService { } }; // A job's own branch starts from the default branch's current commit, so it merges nothing in. - if (!pulled) return action(this.#branch(token, current, job.branch, async () => false)); + // A delivered job, or one started over, may publish again onto its open pull request. + if (!pulled) + return action( + this.#disarming( + token, + current, + job.branch, + this.#branch(token, current, job.branch, async () => false), + ), + ); const pull = await this.#api( token, `${repositoryPath(upstream)}/pulls/${pullNumber(job.target)}`, @@ -1456,8 +1468,18 @@ export class GitHubService { const head = pull.head.repo; const base = based(pull.base.ref); if (head?.id === upstream.id) - return this.#withRepository(upstream, { contents: "write" }, async (writeToken, latest) => - action(this.#branch(writeToken, latest, job.branch, base)), + return this.#withRepository( + upstream, + { contents: "write", pull_requests: "write" }, + async (writeToken, latest) => + action( + this.#disarming( + writeToken, + latest, + job.branch, + this.#branch(writeToken, latest, job.branch, base), + ), + ), ); throw new PublicationBlocked( "The pull request's head is in a contributor's fork, which the bot cannot update. Ask its author to apply the changes, or close it and start new work.", @@ -1466,6 +1488,66 @@ export class GitHubService { ); } + /** + * Wrap `branch`, named `name` on `repo`, so that moving it, or resetting it, + * first turns off auto-merge on every open pull request it heads, whatever + * their bases. Creating the branch needs nothing, since no pull request can + * come from it yet. + */ + #disarming(token: string, repo: Repository, name: string, branch: Branch): Branch { + const label = `${repo.owner.login}:${name}`; + const disarm = async () => { + const open = await this.#api( + token, + `/repos/${repo.owner.login}/${repo.name}/pulls?state=open&per_page=100&head=${encodeURIComponent(label)}`, + ); + for (const pull of open) await this.#disarm(token, pull.node_id); + }; + return { + ...branch, + move: async (from, to) => { + if (from !== null) await disarm(); + return branch.move(from, to); + }, + reset: async (to) => { + await disarm(); + await branch.reset(to); + }, + }; + } + + /** + * Turn off auto-merge on a pull request before kix moves its head. GitHub + * keeps auto-merge across a writer's push, the App's included, so it would + * land the new head with no review that counts on it. + */ + async #disarm(token: string, pull: string) { + const read = await this.#api<{ data?: { node: { autoMergeRequest?: unknown } | null }; errors?: unknown[] }>( + token, + "/graphql", + "POST", + { + query: `query($id: ID!) { node(id: $id) { ... on PullRequest { autoMergeRequest { enabledAt } } } }`, + variables: { id: pull }, + }, + ); + // Only a read that shows no auto-merge lets the push go ahead without turning it off. + const node = read.data?.node; + if (read.errors?.length || !node || node.autoMergeRequest === undefined) + throw new PublicationBlocked( + "GitHub did not say whether the pull request is auto-merging, which would land this push unreviewed, so nothing was pushed.", + ); + if (node.autoMergeRequest === null) return; + const result = await this.#api<{ errors?: unknown[] }>(token, "/graphql", "POST", { + query: `mutation($id: ID!) { disablePullRequestAutoMerge(input: { pullRequestId: $id }) { clientMutationId } }`, + variables: { id: pull }, + }); + if (result.errors?.length) + throw new PublicationBlocked( + "GitHub refused to turn off the pull request's auto-merge, which would land this push unreviewed, so nothing was pushed.", + ); + } + #branch(token: string, repo: Repository, branch: string, based: Branch["based"]): Branch { if (!refName(branch) || branch === repo.default_branch) throw new PublicationBlocked("Jobs never write a repository's default branch"); @@ -1664,14 +1746,37 @@ export class GitHubService { }); } + /** Why kix may not land `head` yet, or null when a review counts on it; see `unreviewed`. */ + #unreviewed(token: string, repo: GitHubRepository, current: Repository, pull: number, head: string) { + const path = repositoryPath(repo); + return unreviewed( + { + get: (tail) => this.#api(token, `${path}${tail}`), + graphql: (query, variables) => this.#api(token, "/graphql", "POST", { query, variables }), + }, + { + owner: repo.owner, + name: repo.name, + pull: number(pull), + head: sha(head), + branch: current.default_branch, + app: Number(this.#env.GITHUB_APP_ID), + }, + ); + } + /** * Merge exactly `expectedHead` with the first method the repository - * allows: squash, then a merge commit, then rebase. GitHub enforces branch - * rules, reviews, and checks; a refusal says why and is never retried. + * allows: squash, then a merge commit, then rebase, once a review counts on + * it. GitHub enforces branch rules, reviews, and checks; a refusal says why + * and is never retried. */ mergePullRequest(repo: GitHubRepository, userId: string, pull: number, expectedHead: string) { - return this.#authorized(repo, userId, { contents: "write", pull_requests: "write" }, async (token, current) => { + const permissions: Permissions = { contents: "write", pull_requests: "write", checks: "read" }; + return this.#authorized(repo, userId, permissions, async (token, current) => { const method = mergeMethod(current); + const waiting = await this.#unreviewed(token, repo, current, pull, expectedHead); + if (waiting) throw new GitHubError(waiting); let merged: { sha: string }; try { merged = await this.#api<{ sha: string }>( @@ -1824,7 +1929,7 @@ export class GitHubService { * repository's instructions name, or else the first the repository allows. * A base that requires the `quest land` check queues it for that gate * instead, as `quest merge` does: squash auto-merge whose commit body - * pins the head. + * pins the head. Either way, only once a review counts on the head. */ landPullRequest( repo: GitHubRepository, @@ -1835,7 +1940,7 @@ export class GitHubService { return this.#authorized( repo, userId, - { contents: "write", pull_requests: "write" }, + { contents: "write", pull_requests: "write", checks: "read" }, async (token, current, installation) => { // Landing is a contribution like opening a pull request, so a paused project lands nothing. if ((await this.#state(repo, installation)) !== "enabled") @@ -1851,6 +1956,9 @@ export class GitHubService { }; const holds = await this.#holds(token, repo, pull); check(holds); + // The maintainer's pick of merge never stands in for a review of what the job published since. + const waiting = await this.#unreviewed(token, repo, current, pull, input.head); + if (waiting) throw new PublicationBlocked(waiting); const gated = await this.#gated(token, repo, holds.base); let mutation: "mergePullRequest" | "enablePullRequestAutoMerge" = !gated && immediateMerge(holds.mergeState, holds.queue) diff --git a/cloudflare/src/index.tsx b/cloudflare/src/index.tsx index 2f27e551..73e5ab65 100644 --- a/cloudflare/src/index.tsx +++ b/cloudflare/src/index.tsx @@ -276,7 +276,7 @@ app.post("/projects/github/:owner/:repository/skip", async (c) => { await inbox.skip(c.env.DB, session.user.id, repo.id, item, version); return c.redirect(nextURL("skip"), 303); }); -// A merge is the maintainer's own click on the head they saw; GitHub's rules decide whether it lands. +// A merge is the maintainer's own click on the head they saw; it lands once a review counts on that head and GitHub's rules allow it. app.post("/projects/github/:owner/:repository/pulls/:number/merge", async (c) => { const session = await createAuth(c.env).api.getSession({ headers: c.req.raw.headers }); if (!session) throw new GitHubError("Sign in required"); diff --git a/cloudflare/test/github.test.ts b/cloudflare/test/github.test.ts index d9329e6a..e4f110b0 100644 --- a/cloudflare/test/github.test.ts +++ b/cloudflare/test/github.test.ts @@ -218,6 +218,32 @@ function github( if (path === "/installation/token") return options.cleanupFails ? json({}, 500) : new Response(null, { status: 204 }); if (path === "/user/7") return json({ id: options.account ?? 7, login: "maintainer" }); + // Pull request #1's head, which another maintainer approved. + if (path === "/graphql" && body.query.includes("latestOpinionatedReviews(")) + return json({ + data: { + repository: { + pullRequest: { + state: "OPEN", + headRefOid: "a".repeat(40), + baseRefName: "main", + body: "", + author: { __typename: "User", login: "contributor" }, + userContentEdits: { nodes: [] }, + latestOpinionatedReviews: { + nodes: [ + { + state: "APPROVED", + author: { __typename: "User", login: "reviewer" }, + commit: { oid: "a".repeat(40) }, + }, + ], + }, + }, + }, + }, + }); + if (path.endsWith("/rules/branches/main")) return json([]); if (path.endsWith("/permission")) return json({ user: { id: 7 }, @@ -319,6 +345,7 @@ describe("trusted App operations", () => { metadata: "read", contents: "write", pull_requests: "write", + checks: "read", }); }); it("refuses to merge when the repository allows no merge method", async () => { diff --git a/cloudflare/test/inbox.test.ts b/cloudflare/test/inbox.test.ts index b50c7b0a..04cf7301 100644 --- a/cloudflare/test/inbox.test.ts +++ b/cloudflare/test/inbox.test.ts @@ -261,6 +261,15 @@ describe("decision inbox", () => { const moved = await submit(merge, [["head", "b".repeat(40)]]); expect(moved.status).toBe(403); expect(await moved.text()).toContain("head moved since you looked; review it again"); + // A pick of merge never lands a head without a review that counts on it, and says why. + github.approved = ["c".repeat(40)]; + const unreviewed = await submit(merge, [["head", head]]); + expect(unreviewed.status).toBe(403); + expect(await unreviewed.text()).toContain( + `The approval of ccccccc does not carry to ${head.slice(0, 7)}: what changed since is more than conflict-free merges of trunk. kix lands only a reviewed head.`, + ); + expect(github.merged).toHaveLength(0); + github.approved = [head]; github.refuse = "At least 1 approving review is required by reviewers with write access."; const refused = await submit(merge, [["head", head]], { Referer: `${origin}/inbox?project=10&item=pulls%2F2`, @@ -275,7 +284,7 @@ describe("decision inbox", () => { ); expect(explained).toContain('Return to the item'); // Each click is one request bound to its head; a refusal is never retried. - expect(github.merged).toHaveLength(2); + expect(github.merged).toHaveLength(1); github.refuse = null; github.methods = ["squash"]; const merged = await submit(merge, [ diff --git a/cloudflare/test/project-fixtures.ts b/cloudflare/test/project-fixtures.ts index 10481f71..adbb9098 100644 --- a/cloudflare/test/project-fixtures.ts +++ b/cloudflare/test/project-fixtures.ts @@ -65,6 +65,7 @@ export const issue = { * null removes the signed-in user from the repository, so GitHub answers 404. * `labels` are #2's labels and `events` its label and landing timeline, which * label requests change; `landing` says it is queued or auto-merging. + * `approved` lists the commits of #2 another maintainer approved. */ export async function maintainerOf() { const github = { @@ -83,6 +84,7 @@ export async function maintainerOf() { labels: [] as string[], events: [] as { id: string; label: string | null }[], landing: false, + approved: [head] as string[], }; const who = identity("github"); const cookie = (await oauth(who)).session!; @@ -116,7 +118,13 @@ export async function maintainerOf() { id: 5, app_id: 123, suspended_at: null, - permissions: { metadata: "read", contents: "write", issues: "write", pull_requests: "write" }, + permissions: { + metadata: "read", + contents: "write", + issues: "write", + pull_requests: "write", + checks: "read", + }, }); if (path === "/graphql" && String(init?.body).includes("signature {")) return Response.json({ @@ -140,6 +148,35 @@ export async function maintainerOf() { }, }); + if (path === "/graphql" && String(init?.body).includes("latestOpinionatedReviews(")) { + const reviewer = { __typename: "User", login: "reviewer" }; + return Response.json({ + data: { + repository: { + pullRequest: { + state: github.pull.toUpperCase(), + headRefOid: github.head, + baseRefName: "trunk", + body: "", + author: { __typename: "User", login: "contributor" }, + userContentEdits: { nodes: [] }, + latestOpinionatedReviews: { + nodes: github.approved.map((oid) => ({ + state: "APPROVED", + author: reviewer, + commit: { oid }, + })), + }, + }, + }, + }, + }); + } + if (path === "/repos/upstream/project/rules/branches/trunk") return Response.json([]); + if (/^\/repos\/upstream\/project\/commits\/[0-9a-f]{40}\/check-runs$/.test(path)) + return Response.json({ total_count: 0, check_runs: [] }); + const commitRead = path.match(/^\/repos\/upstream\/project\/git\/commits\/([0-9a-f]{40})$/)?.[1]; + if (commitRead) return Response.json({ sha: commitRead, tree: { sha: commit }, parents: [{ sha: commit }] }); if (path === "/graphql" && String(init?.body).includes("timelineItems")) { return Response.json({ data: { diff --git a/cloudflare/test/publish.test.ts b/cloudflare/test/publish.test.ts index dd75409d..d10e628a 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -276,7 +276,9 @@ describe("runner publication", () => { const scopes = requests() .filter((r) => r.path.endsWith("/access_tokens")) .map((r) => r.body); - expect(scopes).toEqual([{ repository_ids: [10], permissions: { metadata: "read", contents: "write" } }]); + expect(scopes).toEqual([ + { repository_ids: [10], permissions: { metadata: "read", contents: "write", pull_requests: "write" } }, + ]); expect((await job(id)).head).toBe(head); // A stale base is refused without writing anything. @@ -686,6 +688,42 @@ describe("runner publication", () => { expect(opened()).toHaveLength(1); }); + it("turns off the job's own pull request's auto-merge before publishing onto it again or starting over", async () => { + const id = await running(); + const delivered = await publish({ + job: id, + attempt: 1, + head: null, + message: "one", + changes: [file("a", "1")], + pull: { title: "T", body: "B" }, + }); + const { head } = (await delivered.json()) as { head: string }; + const pull = opened()[0]!; + pull.landing = true; + const next = await publish({ job: id, attempt: 1, head, message: "two", changes: [file("a", "2")] }); + expect(next.status).toBe(200); + expect(pull.landing).toBe(false); + + // Starting over resets the branch only after turning auto-merge off again. + const { head: pushed } = (await next.json()) as { head: string }; + project.branches["quest/first"] = prHead; + const blocked = await publish({ + job: id, + attempt: 1, + head: pushed, + session: "s", + message: "three", + changes: [file("a", "3")], + }); + expect(blocked.status).toBe(202); + pull.landing = true; + const waiting = await job(id); + await restart(testEnv, user, upstream, waiting, waiting.questions[0]!.id); + expect(project.branches["quest/first"]).toBe(revision); + expect(pull.landing).toBe(false); + }); + it("lets only the requester start over in place of another maintainer's answer awaiting approval", async () => { project.branches["quest/first"] = prHead; const id = await running(); @@ -1025,7 +1063,7 @@ describe("runner publication", () => { .map((r) => r.body.permissions), ).toEqual([ { metadata: "read", contents: "read", pull_requests: "read" }, - { metadata: "read", contents: "write" }, + { metadata: "read", contents: "write", pull_requests: "write" }, ]); expect((await publish({ job: id, attempt: 1, head, pull: { title: "T", body: "B" } })).status).toBe(409); @@ -1045,6 +1083,72 @@ describe("runner publication", () => { }); }); + it("turns off an iterated pull request's auto-merge before pushing, so the new head never lands unreviewed", async () => { + const id = await running({ + workflow: "iterate", + target: "pulls/2", + revision: prHead, + branch: "contributor/fix", + trusted: false, + }); + const disabled = () => + requests().filter( + (r) => r.path === "/graphql" && String(r.body.query).includes("disablePullRequestAutoMerge("), + ); + project.pulls[0]!.landing = true; + // Another open pull request from the same branch, to another base, would land the push too. + const sibling = { + ...project.pulls[0]!, + number: 9, + base: "release", + threads: [], + runs: [], + }; + project.pulls.push(sibling); + const response = await publish({ + job: id, + attempt: 1, + head: prHead, + message: "fix: review", + changes: [file("a", "1")], + }); + expect(response.status).toBe(200); + const { head } = (await response.json()) as { head: string }; + expect(project.branches["contributor/fix"]).toBe(head); + expect(project.pulls[0]!.landing).toBe(false); + expect(sibling.landing).toBe(false); + expect(disabled()).toHaveLength(2); + + // With auto-merge already off, it pushes without turning anything off. + const next = await publish({ job: id, attempt: 1, head, message: "fix: more", changes: [file("a", "2")] }); + expect(next.status).toBe(200); + expect(disabled()).toHaveLength(2); + + // A read that does not show the auto-merge state pushes nothing. + const { head: pushed } = (await next.json()) as { head: string }; + project.pulls[0]!.landing = true; + vi.mocked(globalThis.fetch).mockImplementation((input, init) => + String(init?.body).includes("autoMergeRequest { enabledAt } } } }") + ? Promise.resolve(Response.json({ errors: [{ message: "Something went wrong" }] })) + : github(input, init), + ); + const blocked = await publish({ + job: id, + attempt: 1, + head: pushed, + session: "s", + message: "fix: last", + changes: [file("a", "3")], + }); + expect(blocked.status).toBe(202); + expect(project.branches["contributor/fix"]).toBe(pushed); + expect(project.pulls[0]!.landing).toBe(true); + expect((await job(id)).questions[0]!.prompt).toEqual({ + type: "publication", + blocked: expect.stringContaining("did not say whether the pull request is auto-merging"), + }); + }); + it("validates the request before touching the job", async () => { const id = await running(); for (const body of [ @@ -1078,8 +1182,12 @@ describe("merge jobs", () => { return github.call(this, input, init); }); } - /** A running merge job on pull request #2 that holds it, as picking merge leaves it. */ - async function merging() { + /** A maintainer's approval of `commit` on pull request #2. */ + const approve = (commit: string) => + (pull().reviews = [...(pull().reviews ?? []), { state: "APPROVED", author: "maintainer", commit }]); + /** A running merge job on pull request #2 that holds it, as picking merge leaves it, its head approved unless `reviewed` is false. */ + async function merging(reviewed = true) { + if (reviewed) approve(prHead); const id = await running({ workflow: "merge", target: "pulls/2", @@ -1108,12 +1216,13 @@ describe("merge jobs", () => { expect(await refused.json()).toEqual({ error: "A merge commit may only merge in a commit its upstream base branch already holds", }); + // The merge resolved nothing, so the approval of the pull request's head carries to it. const response = await publish({ job: id, attempt: 1, head: prHead, message: "Merge main", - changes: [file("a", "1")], + changes: [], merge: main, land: { method: "merge" }, }); @@ -1422,6 +1531,155 @@ describe("merge jobs", () => { expect(lands().map((l) => l.variables.method)).toEqual(["SQUASH"]); }); + describe("lands only a reviewed head", () => { + const waiting = (head: string) => + `No review counts on ${head.slice(0, 7)} yet. kix lands only a reviewed head. Approve ${head.slice(0, 7)} on GitHub as someone with write access other than the pull request's author, have a runner's \`quest review\` check pass on it, or, as its author with write access, name it in the body's \`## Review\` record if no App committed it.`; + /** Land pull request #2 at `head`, returning the decision the job paused on, or null once it landed. */ + async function land(id: string, head = prHead) { + const response = await publish({ job: id, attempt: 1, head, session: "s", land: {} }); + if (response.status === 200) return null; + expect(response.status).toBe(202); + return ((await job(id)).questions.at(-1)!.prompt as { blocked: string }).blocked; + } + + it("waits, saying why, for a review of the exact head, then lands once a maintainer approves it", async () => { + const id = await merging(false); + expect(await land(id)).toBe(waiting(prHead)); + expect(lands()).toEqual([]); + await store.cancel(db, id, user, 0); + pull().labels = []; + const again = await merging(); + expect(await land(again)).toBeNull(); + expect(lands().map((l) => l.variables.sha)).toEqual([prHead]); + }); + + it.each([ + [ + "an approval by the pull request's own author", + () => pull().reviews!.push({ state: "APPROVED", author: "contributor" }), + ], + ["an approval by a bot", () => pull().reviews!.push({ state: "APPROVED", author: "reviewer[bot]" })], + [ + "a comment from the author's account", + () => pull().comments!.push({ author: "contributor", body: "LGTM (written by an AI)", at }), + ], + ["a body-only Review record", () => (pull().body = `Fixes it\n\n## Review\n\nNo findings on ${prHead}.`)], + [ + "another App's quest review check", + () => pull().runs!.push({ name: "quest review", conclusion: "SUCCESS", url: "u", app: 1 }), + ], + ["a request for changes", () => pull().reviews!.push({ state: "CHANGES_REQUESTED", author: "maintainer" })], + ])("counts no review from %s", async (_, add) => { + pull().comments = []; + add(); + const id = await merging(false); + expect(await land(id)).toBe(waiting(prHead)); + expect(lands()).toEqual([]); + }); + + it.each([ + ["lands at the head it passed on", false], + ["waits at a later head", true], + ])("counts the App's passing quest review check, which %s", async (_, moved) => { + pull().runs!.push({ + name: "quest review", + conclusion: "SUCCESS", + url: "u", + app: Number(env.GITHUB_APP_ID), + }); + const id = await merging(false); + if (!moved) return expect(await land(id)).toBeNull(); + // The check stays on the commit it passed on when someone pushes past it. + pull().runs!.at(-1)!.commit = prHead; + const decision = await publish({ + job: id, + attempt: 1, + head: prHead, + session: "s", + message: "fix", + changes: [file("a", "1")], + land: {}, + }); + expect(decision.status).toBe(202); + const head = project.branches["contributor/fix"]!; + expect((await job(id)).questions.at(-1)!.prompt).toMatchObject({ blocked: waiting(head) }); + expect(lands()).toEqual([]); + }); + + it.each([ + ["lands at the head it names", false], + ["waits at a head a runner committed", true], + ])("counts a writer's own Review record in their pull request, which %s", async (_, moved) => { + pull().author = "maintainer"; + pull().body = `Fixes it\n\n## Review\n\nNo findings on ${prHead}.`; + const id = await merging(false); + if (!moved) return expect(await land(id)).toBeNull(); + const decision = await publish({ + job: id, + attempt: 1, + head: prHead, + session: "s", + message: "fix", + changes: [file("a", "1")], + land: {}, + }); + expect(decision.status).toBe(202); + const head = project.branches["contributor/fix"]!; + expect((await job(id)).questions.at(-1)!.prompt).toMatchObject({ blocked: waiting(head) }); + expect(lands()).toEqual([]); + }); + + it("does not carry an approval across a merge that changed what it merged", async () => { + const id = await merging(); + const main = "f".repeat(40); + project.commits[main] = { parents: [revision], tree: "1".repeat(40), message: "main" }; + project.branches.main = main; + const response = await publish({ + job: id, + attempt: 1, + head: prHead, + session: "s", + message: "Merge main", + changes: [file("a", "resolved by hand")], + merge: main, + land: {}, + }); + expect(response.status).toBe(202); + const head = project.branches["contributor/fix"]!; + expect((await job(id)).questions.at(-1)!.prompt).toEqual({ + type: "publication", + blocked: `The approval of ${prHead.slice(0, 7)} does not carry to ${head.slice(0, 7)}: what changed since is more than conflict-free merges of main. kix lands only a reviewed head. Approve ${head.slice(0, 7)} on GitHub as someone with write access other than the pull request's author, have a runner's \`quest review\` check pass on it, or, as its author with write access, name it in the body's \`## Review\` record if no App committed it.`, + }); + expect(lands()).toEqual([]); + }); + + it.each([ + ["dismisses stale approvals", { dismiss_stale_reviews_on_push: true }, "never", true], + ["requires approval of the last push", { require_last_push_approval: true }, "never", true], + ["accepts an approval of an older head", {}, "never", false], + [ + "dismisses stale approvals but lets the App bypass it", + { dismiss_stale_reviews_on_push: true }, + "always", + false, + ], + ] as const)( + "defers to a ruleset that %s only when it enforces the current head", + async (_, rule, bypass, defers) => { + project.rules = [ + { + type: "pull_request", + ruleset_id: 42, + parameters: { required_approving_review_count: 1, ...rule }, + }, + ]; + project.rulesets = { 42: { current_user_can_bypass: bypass } }; + const id = await merging(false); + expect(await land(id)).toBe(defers ? null : waiting(prHead)); + }, + ); + }); + it("gives the pull request back when the job fails, and lands nothing for other workflows", async () => { const id = await merging(); await fail(id); diff --git a/cloudflare/test/review.test.ts b/cloudflare/test/review.test.ts new file mode 100644 index 00000000..b651beea --- /dev/null +++ b/cloudflare/test/review.test.ts @@ -0,0 +1,274 @@ +import { createHash } from "node:crypto"; +import { describe, expect, it } from "vitest"; +import { type Reads, unreviewed } from "../src/github/review"; + +const APP = 123; +const sha = (name: string) => createHash("sha1").update(name).digest("hex"); +const short = (name: string) => sha(name).slice(0, 7); +/** Commits by name: their parents, and their files as `path: content`. */ +type Commits = Record }>; + +/** + * A repository whose default branch `main` is at `main`, whose pull request #1 + * is at `head`, and whose reviews, check runs, and rules are as given. + */ +function reads( + commits: Commits, + pull: { + head: string; + main: string; + approved?: string[]; + checked?: string[]; + rules?: unknown[]; + bypass?: string; + truncated?: boolean; + /** The pull request's body, last edited by `editor` when given. */ + body?: string; + editor?: string; + /** The author's permission on the repository. */ + permission?: string; + /** Commits GitHub attributes to an App's bot. */ + bots?: string[]; + }, +): Reads { + const ids = new Map(Object.keys(commits).map((name) => [sha(name), name])); + const ancestors = (name: string, seen = new Set()): Set => { + if (seen.has(name)) return seen; + seen.add(name); + for (const parent of commits[name]!.parents) ancestors(parent, seen); + return seen; + }; + const tree = (name: string) => sha(`tree ${name}`); + const named = (id: string) => [...ids.values()].find((name) => tree(name) === id); + return { + async graphql() { + return { + data: { + repository: { + pullRequest: { + state: "OPEN", + headRefOid: sha(pull.head), + baseRefName: "main", + body: pull.body ?? "", + author: { __typename: "User", login: "author" }, + userContentEdits: { + nodes: pull.editor ? [{ editor: { __typename: "User", login: pull.editor } }] : [], + }, + latestOpinionatedReviews: { + nodes: (pull.approved ?? []).map((name) => ({ + state: "APPROVED", + author: { __typename: "User", login: "maintainer" }, + commit: { oid: sha(name) }, + })), + }, + }, + }, + }, + } as T; + }, + async get(path: string) { + const url = new URL(path, "https://api.github.test"); + let match: RegExpMatchArray | null; + if (url.pathname === "/rules/branches/main") return (pull.rules ?? []) as T; + if ((match = url.pathname.match(/^\/rulesets\/(\d+)$/))) + return { current_user_can_bypass: pull.bypass ?? "never" } as T; + if ((match = url.pathname.match(/^\/commits\/(\w+)\/check-runs$/))) { + const name = ids.get(match[1]!)!; + return { + check_runs: (pull.checked ?? []).includes(name) + ? [ + { + name: "quest review", + head_sha: match[1], + status: "completed", + conclusion: "success", + app: { id: APP }, + }, + ] + : [], + } as T; + } + if (url.pathname === "/collaborators/author/permission") + return { permission: pull.permission ?? "write" } as T; + if ((match = url.pathname.match(/^\/commits\/(\w+)$/))) { + const bot = (pull.bots ?? []).includes(ids.get(match[1]!)!); + return { committer: bot ? { login: "kix[bot]", type: "Bot" } : { login: "author", type: "User" } } as T; + } + if ((match = url.pathname.match(/^\/git\/commits\/(\w+)$/))) { + const name = ids.get(match[1]!)!; + return { + sha: match[1], + tree: { sha: tree(name) }, + parents: commits[name]!.parents.map((parent) => ({ sha: sha(parent) })), + } as T; + } + if ((match = url.pathname.match(/^\/compare\/(\w+)\.\.\.(\w+)$/))) { + const from = ids.get(match[1]!)!; + const to = match[2] === "main" ? pull.main : ids.get(match[2]!)!; + const status = + from === to + ? "identical" + : ancestors(to).has(from) + ? "ahead" + : ancestors(from).has(to) + ? "behind" + : "diverged"; + const common = [...ancestors(from)].filter((name) => ancestors(to).has(name)); + const nearest = common.find( + (name) => !common.some((other) => other !== name && ancestors(other).has(name)), + ); + return { status, merge_base_commit: { sha: sha(nearest!) } } as T; + } + if ((match = url.pathname.match(/^\/git\/trees\/(\w+)$/))) { + const name = named(match[1]!)!; + return { + truncated: !!pull.truncated, + tree: [ + { path: "src", type: "tree", mode: "040000", sha: "d".repeat(40) }, + ...Object.entries(commits[name]!.files).map(([path, content]) => ({ + path, + type: "blob", + mode: "100644", + sha: sha(`blob ${content}`), + })), + ], + } as T; + } + throw new Error(`Unexpected read ${path}`); + }, + }; +} + +/** Base `o`; the pull request's reviewed commit `pr` changed `a`; `main` moved on, changing `b` and `c`. */ +const history = (merged: Record, pr: Record = { a: "1", b: "0", c: "0" }): Commits => ({ + o: { parents: [], files: { a: "0", b: "0", c: "0" } }, + pr: { parents: ["o"], files: pr }, + main: { parents: ["o"], files: { a: "0", b: "2", c: "2" } }, + merge: { parents: ["pr", "main"], files: merged }, +}); +const landing = (head: string) => ({ owner: "o", name: "r", pull: 1, head: sha(head), branch: "main", app: APP }); +const carried = `kix lands only a reviewed head.`; + +describe("the review a landing needs", () => { + it("counts an approval or the App's check on the exact head", async () => { + const commits = history({}); + expect( + await unreviewed(reads(commits, { head: "pr", main: "main", approved: ["pr"] }), landing("pr")), + ).toBeNull(); + expect( + await unreviewed(reads(commits, { head: "pr", main: "main", checked: ["pr"] }), landing("pr")), + ).toBeNull(); + expect(await unreviewed(reads(commits, { head: "pr", main: "main" }), landing("pr"))).toBe( + `No review counts on ${short("pr")} yet. kix lands only a reviewed head. Approve ${short("pr")} on GitHub as someone with write access other than the pull request's author, have a runner's \`quest review\` check pass on it, or, as its author with write access, name it in the body's \`## Review\` record if no App committed it.`, + ); + }); + + it("carries a review across a merge of the base that resolved no conflict", async () => { + const commits = history({ a: "1", b: "2", c: "2" }); + for (const review of [{ approved: ["pr"] }, { checked: ["pr"] }]) + expect( + await unreviewed(reads(commits, { head: "merge", main: "main", ...review }), landing("merge")), + ).toBeNull(); + // The same merge is unverifiable when GitHub truncates a tree. + expect( + await unreviewed( + reads(commits, { head: "merge", main: "main", approved: ["pr"], truncated: true }), + landing("merge"), + ), + ).toContain(carried); + }); + + it.each([ + ["slips in a change neither side made", { a: "1", b: "2", c: "2", d: "new" }], + ["reverts the base's change", { a: "1", b: "0", c: "2" }], + ["drops the reviewed change", { a: "0", b: "2", c: "2" }], + ])("does not carry a review across a merge that %s", async (_, merged) => { + const read = reads(history(merged), { head: "merge", main: "main", approved: ["pr"] }); + expect(await unreviewed(read, landing("merge"))).toBe( + `The approval of ${short("pr")} does not carry to ${short("merge")}: what changed since is more than conflict-free merges of main. kix lands only a reviewed head. Approve ${short("merge")} on GitHub as someone with write access other than the pull request's author, have a runner's \`quest review\` check pass on it, or, as its author with write access, name it in the body's \`## Review\` record if no App committed it.`, + ); + }); + + it("does not carry a review across a conflict resolved by hand, whichever side it keeps", async () => { + // The pull request also changed `b`, which the base changed differently. + for (const b of ["1", "2", "3"]) { + const commits = history({ a: "1", b, c: "2" }, { a: "1", b: "1", c: "0" }); + expect( + await unreviewed(reads(commits, { head: "merge", main: "main", approved: ["pr"] }), landing("merge")), + ).toContain(carried); + } + }); + + it("does not carry a review across a merge of commits the default branch does not hold", async () => { + const commits: Commits = { + ...history({ a: "1", b: "2", c: "2" }), + main: { parents: ["o"], files: { a: "0", b: "2", c: "2" } }, + trunk: { parents: ["o"], files: { a: "0", b: "0", c: "0" } }, + }; + const read = reads(commits, { head: "merge", main: "trunk", approved: ["pr"] }); + expect(await unreviewed(read, landing("merge"))).toContain(carried); + }); + + it("defers to a ruleset only when it enforces a review of the current head and binds the App", async () => { + const commits = history({}); + const rule = (parameters: Record) => [{ type: "pull_request", ruleset_id: 1, parameters }]; + const enforced = rule({ required_approving_review_count: 1, dismiss_stale_reviews_on_push: true }); + expect( + await unreviewed(reads(commits, { head: "pr", main: "main", rules: enforced }), landing("pr")), + ).toBeNull(); + for (const [rules, bypass] of [ + [enforced, "always"], + [enforced, "pull_requests_only"], + [rule({ required_approving_review_count: 1 }), "never"], + [rule({ required_approving_review_count: 0, require_last_push_approval: true }), "never"], + ] as const) + expect( + await unreviewed(reads(commits, { head: "pr", main: "main", rules, bypass }), landing("pr")), + ).toContain(carried); + }); + + describe("the author's own Review record", () => { + const record = (name: string) => + `Summary\n\n## Review\n\nRound 1 on head ${sha(name)}: no findings.\n\n(written by a model)`; + + it("counts when a writer names the exact head in the body they last edited", async () => { + const commits = history({}); + for (const editor of [undefined, "author", "Author"]) + expect( + await unreviewed( + reads(commits, { head: "pr", main: "main", body: record("pr"), editor }), + landing("pr"), + ), + ).toBeNull(); + }); + + it("counts a recorded skip that names the exact head", async () => { + const body = `## Review\n\nSkipped on head ${sha("pr")}: a conflict-free merge of the base.`; + const read = reads(history({}), { head: "pr", main: "main", body }); + expect(await unreviewed(read, landing("pr"))).toBeNull(); + }); + + it.each([ + ["the author cannot write", { permission: "read" }], + ["someone else last edited the body", { editor: "maintainer" }], + ["an App committed the head", { bots: ["pr"] }], + ["it names another commit", { body: record("o") }], + ["it names no commit, as a skip does", { body: "## Review\n\nSkipped: trivial." }], + [ + "the SHA is outside the Review section", + { body: `${sha("pr")}\n\n## Review\n\nDone.\n\n## Next\n\n${sha("pr")}` }, + ], + ])("does not count when %s", async (_, overrides) => { + const read = reads(history({}), { head: "pr", main: "main", body: record("pr"), ...overrides }); + expect(await unreviewed(read, landing("pr"))).toContain(carried); + }); + + it("carries across a merge of the base that resolved no conflict, unless an App committed the merge", async () => { + const commits = history({ a: "1", b: "2", c: "2" }); + const merged = (bots: string[]) => + unreviewed(reads(commits, { head: "merge", main: "main", body: record("pr"), bots }), landing("merge")); + expect(await merged([])).toBeNull(); + expect(await merged(["merge"])).toContain(carried); + }); + }); +}); diff --git a/quest/m0/kix/README.md b/quest/m0/kix/README.md index 9fda0ef8..dd17bce6 100644 --- a/quest/m0/kix/README.md +++ b/quest/m0/kix/README.md @@ -138,5 +138,4 @@ fixes what it finds: - [Private repositories](/quest/m0/kix/private.md) - moq-dev/moq.pro enrolls, visible only to signed-in users who can read it - [Job sandbox](/quest/m0/kix/sandbox/README.md) - head approval for kix-pushed heads, and opencode sandboxed - [Web execution](/quest/m0/kix/execution.md) - spawn and iterate through the UI, ending in contribution PRs -- [Land review](/quest/m0/kix/land-review.md) - kix lands only heads with a review that counts on that exact head - [Runner review checks](/quest/m0/kix/review-check.md) - runners publish the App's `quest review` check, so their heads land without a human approval diff --git a/quest/m0/kix/land-review.md b/quest/m0/kix/land-review.md deleted file mode 100644 index ec76b1a0..00000000 --- a/quest/m0/kix/land-review.md +++ /dev/null @@ -1,49 +0,0 @@ -# [M] Land only reviewed heads - -## Goal - -kix lands a PR only when the exact head being landed has a review that counts -under the repository's review policy. This covers both an inbox merge and a -merge job's `land` publication. A maintainer's earlier merge pick alone never -lands a head that a runner published or changed since the last review. When -landing waits, kix says why. - -## Plan - -Decided 2026-10-28 (user, /quest-plan trust audit): - -- Today a merge job lands a runner-published head on the maintainer's earlier - pick, bounded only by branch rules. moq-dev/quest's `main` requires no - reviews, so nothing enforces the review policy at that point. -- Apply the guide's rule: a review counts only on the head being landed. The - only exemptions are a conflict-free base merge and a reviewer's patch applied - unchanged. Where a repository's policy names whose review counts (for - moq-dev/quest, the PR body's `## Review` record naming the head, in - `CONTRIBUTING.md`), - follow it. Otherwise require a review from someone other than the PR's - author and the App. -- Defer to GitHub branch protection only where it enforces the same - current-head rule: it requires approval of the most recent push, or it - dismisses stale approvals. Protection that accepts an approval of an older - head does not count; keep kix's exact-head check there. -- Test landing with a review of an older head, with a conflict-free base merge - since the review, with a hand-resolved conflict since the review, with only - an AI comment from the author's account, and under protection that requires - reviews without dismissing stale ones. - -Decided 2026-10-28 (user, /quest-plan of the follow-ups from -[moq-dev/quest#271](https://github.com/moq-dev/quest/pull/271)): - -- The PR body's `## Review` record is self-reported: anyone who can edit the - body can write one. For a head a runner published, kix counts a local - review only when the runner published its `quest review` result as a - GitHub App check run on that exact head SHA. Only the App can write one, - and GitHub binds it to the commit. A maintainer's own `## Review` record - still counts for the maintainer's PRs. -- Test a body-only `## Review` record on a runner head, which does not count, - and a check run on an older head. - -## Related - -- [Web execution](/quest/m0/kix/execution.md) - merge jobs this rule gates -- [Require read-only pull-request CI](/quest/m1/kix/ci-read-only.md) - the trust model this completes diff --git a/quest/m0/kix/review-check.md b/quest/m0/kix/review-check.md index 41de425d..ee11b5e4 100644 --- a/quest/m0/kix/review-check.md +++ b/quest/m0/kix/review-check.md @@ -26,7 +26,3 @@ follow-ups. add the operator step to `cloudflare/GITHUB.md`. - Choose which jobs run it, such as iterate and merge jobs after they publish. - -## Required - -- [Land review](/quest/m0/kix/land-review.md) - the gate that counts the App's check diff --git a/quest/m1/kix/ci-read-only.md b/quest/m1/kix/ci-read-only.md index 3f64d233..72c63192 100644 --- a/quest/m1/kix/ci-read-only.md +++ b/quest/m1/kix/ci-read-only.md @@ -55,5 +55,4 @@ moq-dev/quest#246's Codex Security P1): ## Related - [Operator setup](/quest/m0/kix/operator-setup.md) - pilot repositories should pass this check -- [Land review](/quest/m0/kix/land-review.md) - the land stage's trust rule - [Head approval](/quest/m0/kix/sandbox/head-approval.md) - the devShell stage's trust rule for heads without signatures diff --git a/quest/m1/kix/review-flag.md b/quest/m1/kix/review-flag.md index c954aee7..fb5f949f 100644 --- a/quest/m1/kix/review-flag.md +++ b/quest/m1/kix/review-flag.md @@ -18,7 +18,3 @@ follow-ups. such as the PR's `latestReviews` and the App's `quest review` check, and keep the click's live check as the authority. - Keep the extra GitHub reads within the Worker's subrequest budget. - -## Required - -- [Land review](/quest/m0/kix/land-review.md) - the gate whose reason the inbox shows