diff --git a/cloudflare/GITHUB.md b/cloudflare/GITHUB.md index 481cd0b5..94475cf6 100644 --- a/cloudflare/GITHUB.md +++ b/cloudflare/GITHUB.md @@ -247,6 +247,23 @@ 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. +The inbox says so before the click. Each project read takes every open PR's +body, its author's association with the repository, the writers' latest +reviews, and the head commit's parents and this App's `quest review` check in +one batched GraphQL request, the same request that classifies drafts, and +keeps only the land gate's reason (`observedGap`), never the body. A PR whose +merge is offered shows that reason beside **Merge this PR**. It counts what the +gate counts on the head itself. The author's record counts unless GitHub's +association (anything but owner, member, or collaborator) rules out write +access. A merge head's reason says that an earlier review may still carry +across it, since only the gate's tree reads can tell. The read does not look +at rulesets, so where GitHub itself requires the review, the item is flagged +and GitHub refuses the same head. The click's own check stays the authority: +it confirms write access, that no App committed the head, and any carried +review, and it reads the latest reviews, which a read can lag. The App's own +`quest review` check run, unlike CI's, asks for a read of the PRs it names, +so a runner's review clears the flag without a refresh. + 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 @@ -608,7 +625,8 @@ Paused and disconnected projects are not read. A failed or rate-limited read is recorded and shown next to the refresh fallback, and is not retried: only a request made during it gets another round, and otherwise the next webhook or a refresh reads again. Checks, statuses and other pushes name no entity and -change no candidate, so they cost no read. +change no candidate, so they cost no read, except this App's own `quest review` +check run, which rereads the PRs its payload names by number. The inbox also treats a PR whose row still says open as closed once a complete, current occupancy pass no longer lists it, so a missed close webhook @@ -636,7 +654,8 @@ escaped titles, state, comment/review counts, current PR head, that head's check rollup, aggregate review decision, and up to 20 latest reviewer records with an explicit pagination flag. Raw bodies and individual check logs are not cached: a PR's body is kept only as whether it has a `## Waiting` section (or the -older `## Decisions needed`). +older `## Decisions needed`) and as the land gate's reason its head needs a +review, read with the open PRs' bodies in one batched request per read. Unavailable/deleted entities retain their notification identity and are marked missing rather than disappearing or presenting old state as current. diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index 18373fac..f8ac7fe5 100644 --- a/cloudflare/dev/github.ts +++ b/cloudflare/dev/github.ts @@ -4,6 +4,7 @@ // refuses anything it does not model. Like GitHub, it scopes each installation // token to the repositories and permissions it was minted for. import { createHash } from "node:crypto"; +import { REVIEW_CHECK } from "../src/github/review"; export type User = { id: number; login: string; name: string; email: string }; /** A comment or review thread reply; a null author is a deleted account. */ @@ -165,12 +166,46 @@ function entity(repo: Repo, item: Issue) { const mergeState = (pull: Pull) => pull.merge ?? (pull.draft ? "DRAFT" : "BLOCKED"); -function pullNode(repo: Repo, pull: Pull) { +/** How GitHub relates a login to the repository: only an association that can come with write access names one. */ +const association = (repo: Repo, login: string) => + login === repo.owner + ? "OWNER" + : [...repo.maintainers, ...(repo.writers ?? [])].includes(login) + ? "COLLABORATOR" + : "CONTRIBUTOR"; + +/** A pull request as the project read pages it, with what the inbox's body batch adds for `app`'s review check. */ +function pullNode(repo: Repo, pull: Pull, app?: unknown) { const oid = headOf(repo, pull); const reviews = pull.reviews ?? []; + const checks = (pull.runs ?? []).filter( + (run) => !run.context && run.name === REVIEW_CHECK && run.app === app && (run.commit ?? oid) === oid, + ); return { ...entity(repo, pull), - body: pull.body ?? "", + ...reviewTarget(repo, pull), + authorAssociation: association(repo, pull.author), + tip: { + nodes: [ + { + commit: { + oid, + parents: { totalCount: repo.commits[oid]?.parents.length ?? 1 }, + checkSuites: { + nodes: checks.length + ? [ + { + checkRuns: { + nodes: [{ status: "COMPLETED", conclusion: checks.at(-1)!.conclusion }], + }, + }, + ] + : [], + }, + }, + }, + ], + }, headRefName: pull.branch, headRefOid: oid, isCrossRepository: !!pull.contributor, @@ -439,7 +474,7 @@ function graphql(repo: Repo, query: string, variables: Record) }; } - if (query.includes("latestOpinionatedReviews(")) { + if (query.includes("headRefOid baseRefName body")) { const pull = repo.pulls.find((p) => p.number === variables.number); return { data: { repository: { pullRequest: pull ? reviewTarget(repo, pull) : null } } }; } @@ -523,7 +558,8 @@ function graphql(repo: Repo, query: string, variables: Record) for (const [, alias, field, number] of query.matchAll(/([sb]\d+):(issue|pullRequest)\(number:(\d+)\)/g)) { const pull = repo.pulls.find((p) => p.number === Number(number)); const issue = repo.issues.find((i) => i.number === Number(number)); - const found = field === "pullRequest" ? pull && pullNode(repo, pull) : issue && entity(repo, issue); + const found = + field === "pullRequest" ? pull && pullNode(repo, pull, variables.app) : issue && entity(repo, issue); result[alias!] = found ?? null; if (!found) errors.push({ type: "NOT_FOUND", path: ["repository", alias] }); } diff --git a/cloudflare/src/github/review.ts b/cloudflare/src/github/review.ts index d4c9bd3f..149830df 100644 --- a/cloudflare/src/github/review.ts +++ b/cloudflare/src/github/review.ts @@ -79,17 +79,7 @@ export async function unreviewed(reads: Reads, landing: Landing): Promise - review.state === "APPROVED" && - review.author?.__typename === "User" && - review.author.login.toLowerCase() !== author && - review.commit - ? [review.commit.oid] - : [], - ), - ); + const approved = approvals(pull); const recorded = await ownRecord(reads, pull); const commits = new Map>(); const commit = (sha: string) => { @@ -109,30 +99,124 @@ export async function unreviewed(reads: Reads, landing: Landing): Promise null) : null; } - const head = short(landing.head); + return refusal(approved, landing.head, landing.branch); +} + +/** + * Why a head with no review that counts on it, of those `unreviewed` reads, + * cannot land. `unverified` is for a merge head whose earlier review may still + * carry across it, which a caller without the trees cannot tell. + */ +function refusal(approved: Set, sha: string, branch: string, unverified = false) { + const head = short(sha); 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.`; + const earlier = [...approved].map(short).join(", "); + if (unverified) + return `${approved.size ? `The approval of ${earlier} is of a commit before ${head}` : `No review counts on ${head} itself yet`}. ${head} is a merge, and a review carries across merges of ${branch} that resolved no conflict, which kix checks when you merge. ${ask}`; 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}`; + return `The approval of ${earlier} does not carry to ${head}: what changed since is more than conflict-free merges of ${branch}. ${ask}`; +} + +/** The commits a writer other than the pull request's author approved, as a person rather than an App. */ +function approvals(pull: Pick) { + const author = pull.author?.login.toLowerCase(); + return new Set( + pull.latestOpinionatedReviews.nodes.flatMap((review) => + review.state === "APPROVED" && + review.author?.__typename === "User" && + review.author.login.toLowerCase() !== author && + review.commit + ? [review.commit.oid] + : [], + ), + ); } /** * 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. + * when its author, a person, wrote the body as it stands; else none. */ -async function ownRecord(reads: Reads, pull: PullRow): Promise> { +function named(pull: Pick): Set { 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; + return new Set(section?.[1]!.match(/\b[0-9a-f]{40}\b/g) ?? []); +} + +/** + * The fields of an open pull request the inbox's reads add for + * `observedGap`: those `unreviewed` reads first, and the head commit's + * parents and the App's `quest review` check on it, for the App ID `$app`. + */ +export const OBSERVED = `authorAssociation body author { __typename login } + userContentEdits(first:1) { nodes { editor { __typename login } } } + latestOpinionatedReviews(first:100,writersOnly:true) { nodes { state author { __typename login } commit { oid } } } + tip:commits(last:1) { nodes { commit { oid parents { totalCount } + checkSuites(first:10,filterBy:{appId:$app}) { nodes { checkRuns(first:1,filterBy:{checkName:"${REVIEW_CHECK}",checkType:LATEST}) { nodes { status conclusion } } } } } } }`; +export type Observed = Pick & { + /** How GitHub relates the author to the repository, such as `MEMBER` or `CONTRIBUTOR`. */ + authorAssociation: string; + tip: { + nodes: { + commit: { + oid: string; + parents: { totalCount: number }; + /** Only the App's own suites: the query filters them by its ID. */ + checkSuites: { nodes: { checkRuns: { nodes: { status: string; conclusion: string | null }[] } }[] }; + }; + }[]; + }; +}; +/** The associations that can come with write access; any other rules it out. */ +const WRITERS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); + +/** + * Why `unreviewed` would refuse to land `head`, as far as one read of the + * pull request shows, or null when a review counts or may count on it. The + * inbox shows this before a maintainer picks merge; the click's own + * `unreviewed` stays the authority. + * + * It counts what `unreviewed` counts on the head itself: a writer's approval, + * the App's passing check, or the author's own record when GitHub's + * association leaves write access possible (the click confirms that, and that + * no App committed the head). A review from before a merge may carry across + * it, which only the click's reads of the trees can tell, so a merge head's + * reason says so. A ruleset that makes GitHub require the review itself is + * not read here; GitHub refuses the same unreviewed head there. + */ +export function observedGap(pull: Observed, head: string, branch: string): string | null { + const tip = pull.tip?.nodes?.at(-1)?.commit; + // A read missing what this weighs flags nothing; the click still checks. + if ( + tip?.oid !== head || + !Array.isArray(tip.checkSuites?.nodes) || + !Array.isArray(pull.latestOpinionatedReviews?.nodes) + ) + return null; + const approved = approvals(pull); + const checked = tip.checkSuites.nodes.some((suite) => + suite.checkRuns.nodes.some((run) => run.status === "COMPLETED" && run.conclusion === "SUCCESS"), + ); + if (approved.has(head) || checked) return null; + if (WRITERS.has(pull.authorAssociation) && named(pull).has(head)) return null; + return refusal(approved, head, branch, tip.parents.totalCount > 1); +} + +/** + * 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 recorded = named(pull); + if (!recorded.size) return recorded; try { const { permission } = await reads.get<{ permission: string }>( - `/collaborators/${encodeURIComponent(author.login)}/permission`, + `/collaborators/${encodeURIComponent(pull.author!.login)}/permission`, ); - return permission === "admin" || permission === "write" ? named : new Set(); + return permission === "admin" || permission === "write" ? recorded : new Set(); } catch { // An author kix cannot confirm can write is one whose word it does not take. return new Set(); diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 904bd9e2..f172e633 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -13,7 +13,7 @@ import { } from "quest/core"; import { cleanAutoMergeRefusal, immediateMerge, queueHead } from "../../../src/github/merge"; import { osiApproved } from "../licenses"; -import { REVIEW_CHECK, unreviewed } from "./review"; +import { OBSERVED, type Observed, observedGap, REVIEW_CHECK, unreviewed } from "./review"; import { GOAL_LIMIT, SETUP_BRANCH, STUBS, planCheck, planned, setupGoal, setupPull, treeCheckout } from "./setup"; import { positiveId } from "./webhook"; import { @@ -888,7 +888,7 @@ export class GitHubService { error.path?.length !== 2 || error.path[0] !== "repository" || !/^s\d+$/.test(String(error.path[1])), ); if (partial || !response.data?.repository) throw new GitHubError("Project activity unavailable"); - await this.#draftBodies(token, repo, scan, response.data.repository, stale); + await this.#bodies(token, repo, current, scan, response.data.repository, stale); // A rename, transfer, visibility change, or revocation during the read // must not publish fresh observations. const latest = await this.#api(token, repositoryPath(repo)); @@ -902,16 +902,22 @@ export class GitHubService { await reconcile(this.#env.DB, repo, snapshot, scan, response.data.repository, stale, row!.revision, row!.epoch); } - async #draftBodies( + /** + * Read every open pull request's body and the reviews on its head in one + * batch, then require the same identity and version before publishing. A + * draft's body says whether it is handed back, and the reviews say why the + * land gate would refuse its head (`observedGap`). Metadata pages, which + * also page closed history, never request bodies. + */ + async #bodies( token: string, repo: GitHubRepository, + current: Repository, scan: Scan, page: ActivityPage, stale: Awaited>, ) { - // Bodies are needed only to classify open drafts. Metadata pages never request them. - // Read those drafts in one batch, then require the same identity/version before publishing. - const drafts = new Map(); + const open = new Map(); const rows = [ ...(scan.history.pulls !== false ? (page.pullRequests?.nodes ?? []).filter( @@ -924,44 +930,41 @@ export class GitHubService { ...stale.map((_, i) => page[`s${i}`]), ]; for (const item of rows) { - if (!item || !("isDraft" in item) || !item.isDraft || item.state !== "OPEN") continue; - const draft = item as Pull & { body?: string }; - if (!positiveId(draft.number)) throw new GitHubError("Invalid draft observation"); - drafts.set(draft.number, [...(drafts.get(draft.number) ?? []), draft]); + if (!item || !("isDraft" in item) || item.state !== "OPEN") continue; + const pull = item as Pull & { body?: string }; + if (!positiveId(pull.number)) throw new GitHubError("Invalid pull request observation"); + open.set(pull.number, [...(open.get(pull.number) ?? []), pull]); } - if (drafts.size) { - const numbers = [...drafts.keys()]; - const result = await this.#api<{ - data?: { - repository?: Record< - string, - | (Pick & { - body: string; - }) - | null - >; - }; - errors?: unknown[]; - }>(token, "/graphql", "POST", { - query: `query($owner: String!, $name: String!) { repository(owner:$owner,name:$name) { ${numbers.map((number, i) => `b${i}:pullRequest(number:${number}) { databaseId:fullDatabaseId updatedAt headRefOid state isDraft body }`).join("\n")} } }`, - variables: { owner: repo.owner, name: repo.name }, - }); - if (result.errors?.length || !result.data?.repository) throw new GitHubError("Draft bodies unavailable"); - for (const [i, number] of numbers.entries()) { - const body = result.data.repository[`b${i}`]; - for (const item of drafts.get(number)!) { - if ( - !body || - typeof body.body !== "string" || - body.databaseId !== item.databaseId || - body.updatedAt !== item.updatedAt || - body.headRefOid !== item.headRefOid || - body.state !== "OPEN" || - !body.isDraft - ) - throw new ProjectChanged(); - item.body = body.body; - } + if (!open.size) return; + const numbers = [...open.keys()]; + const result = await this.#api<{ + data?: { + repository?: Record< + string, + (Pick & Observed) | null + >; + }; + errors?: unknown[]; + }>(token, "/graphql", "POST", { + query: `query($owner: String!, $name: String!, $app: Int!) { repository(owner:$owner,name:$name) { ${numbers.map((number, i) => `b${i}:pullRequest(number:${number}) { databaseId:fullDatabaseId updatedAt headRefOid state isDraft ${OBSERVED} }`).join("\n")} } }`, + variables: { owner: repo.owner, name: repo.name, app: Number(this.#env.GITHUB_APP_ID) }, + }); + if (result.errors?.length || !result.data?.repository) throw new GitHubError("Pull request bodies unavailable"); + for (const [i, number] of numbers.entries()) { + const read = result.data.repository[`b${i}`]; + for (const item of open.get(number)!) { + if ( + !read || + typeof read.body !== "string" || + read.databaseId !== item.databaseId || + read.updatedAt !== item.updatedAt || + read.headRefOid !== item.headRefOid || + read.state !== "OPEN" || + read.isDraft !== item.isDraft + ) + throw new ProjectChanged(); + item.body = read.body; + item.unreviewed = observedGap(read, read.headRefOid, current.default_branch); } } } diff --git a/cloudflare/src/github/state.ts b/cloudflare/src/github/state.ts index dbc14f3d..f2b894a3 100644 --- a/cloudflare/src/github/state.ts +++ b/cloudflare/src/github/state.ts @@ -52,6 +52,12 @@ export type Pull = Entity & { mergeStateStatus?: string; /** The hold label a session holds it with, such as a merge job's; absent in older observations. */ hold?: string | null; + /** + * Why kix's land gate would refuse its head, as far as the read shows + * (`observedGap`), or null when a review counts or may count on it. + * Absent in older observations, which flag nothing. + */ + unreviewed?: string | null; reviews: { totalCount: number }; latestReviews: { nodes: { state: string; author: { login: string } | null; commit: { oid: string } | null }[]; diff --git a/cloudflare/src/github/webhook.ts b/cloudflare/src/github/webhook.ts index ef9f0adf..ad042ef8 100644 --- a/cloudflare/src/github/webhook.ts +++ b/cloudflare/src/github/webhook.ts @@ -3,6 +3,7 @@ import { bodyLimit } from "hono/body-limit"; import * as landing from "../jobs/landing"; import { wake } from "../jobs/wake"; import { arrived } from "../push/notify"; +import { REVIEW_CHECK } from "./review"; import { admitted, GitHubService } from "./service"; export function positiveId(value: unknown): value is number { @@ -173,6 +174,30 @@ githubWebhooks.post("/webhooks/github", async (c) => { WHERE repository=? AND kind='pull' AND number=? AND ${pending} AND ${enrolled}`, ).bind(now, repository, issue.number, delivery, digest, event, repository, installation), ); + // CI's checks cost no read, but the App's own review check decides whether + // the inbox flags a head as needing a review, so it marks the pull requests + // its payload lists for a read by number; the read itself says whether + // anything changed. An untracked PR waits for history. + const run = event === "check_run" ? payload.check_run : null; + const reviewed = + run?.name === REVIEW_CHECK && + String(run.app?.id) === c.env.GITHUB_APP_ID && + Array.isArray(run.pull_requests) + ? [ + ...new Set( + run.pull_requests + .filter((pr: any) => pr?.base?.repo?.id === repository && positiveId(pr.number)) + .map((pr: any) => pr.number as number), + ), + ] + : []; + if (reviewed.length) + statements.push( + c.env.DB.prepare( + `UPDATE github_project_activity SET stale=stale+1 + WHERE repository=? AND kind='pull' AND number IN (SELECT value FROM json_each(?)) AND ${pending} AND ${enrolled}`, + ).bind(repository, JSON.stringify(reviewed), delivery, digest, event, repository, installation), + ); const invalidating = invalidates(event, payload); if (invalidating) statements.push( @@ -180,9 +205,9 @@ githubWebhooks.post("/webhooks/github", async (c) => { `UPDATE github_project_state SET invalidated=1,revision=revision+1 WHERE repository=? AND ${pending} AND ${enrolled}`, ).bind(repository, delivery, digest, event, repository, installation), ); - // What the inbox shows changed: ask for a background read. CI and other - // pushes change neither, so they cost no read. - if (named || discussed || invalidating) { + // What the inbox shows changed: ask for a background read. Other checks + // and pushes change neither, so they cost no read. + if (named || discussed || reviewed.length || invalidating) { reads = statements.length; statements.push( c.env.DB.prepare( diff --git a/cloudflare/src/inbox/page.tsx b/cloudflare/src/inbox/page.tsx index d2ba14ca..c07cb7fa 100644 --- a/cloudflare/src/inbox/page.tsx +++ b/cloudflare/src/inbox/page.tsx @@ -455,6 +455,8 @@ function Actions(props: { item: Item; view: ProjectView | undefined; runners: Ru const active = item.job && ["queued", "running", "waiting"].includes(item.job.state) && !item.job.cancel; return (
+ {/* Said before the click, which the land gate still decides. */} + {merge?.pull.unreviewed &&

{merge.pull.unreviewed}

} {merge?.clean && (
diff --git a/cloudflare/test/github.test.ts b/cloudflare/test/github.test.ts index 38ec874c..da69e0a3 100644 --- a/cloudflare/test/github.test.ts +++ b/cloudflare/test/github.test.ts @@ -284,7 +284,7 @@ function github( 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(")) + if (path === "/graphql" && body.query.includes("headRefOid baseRefName body")) return json({ data: { repository: { diff --git a/cloudflare/test/inbox.test.ts b/cloudflare/test/inbox.test.ts index aa0a3fdb..67ce2e8b 100644 --- a/cloudflare/test/inbox.test.ts +++ b/cloudflare/test/inbox.test.ts @@ -308,6 +308,21 @@ 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"); + // The item says up front when the read found no review counting on the head, and the click still checks. + expect(active).not.toContain("kix lands only a reviewed head"); + const reason = `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.`; + await db + .prepare( + "UPDATE github_project_activity SET data=json_set(data,'$.unreviewed',?) WHERE repository=10 AND id='21'", + ) + .bind(`${reason} Approve it.`) + .run(); + const flagged = thread(await inbox()); + expect(flagged).toMatch( + new RegExp( + `

${reason.replace(/[.()]/g, "\\$&")} <b>Approve it[\\s\\S]*action="${merge}"`, + ), + ); // 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]]); diff --git a/cloudflare/test/project-fixtures.ts b/cloudflare/test/project-fixtures.ts index 460f7e37..5363db61 100644 --- a/cloudflare/test/project-fixtures.ts +++ b/cloudflare/test/project-fixtures.ts @@ -149,7 +149,7 @@ export async function maintainerOf() { }, }); - if (path === "/graphql" && String(init?.body).includes("latestOpinionatedReviews(")) { + if (path === "/graphql" && String(init?.body).includes("headRefOid baseRefName body")) { const reviewer = { __typename: "User", login: "reviewer" }; return Response.json({ data: { diff --git a/cloudflare/test/projects.test.ts b/cloudflare/test/projects.test.ts index f2d74e31..3e7a7bf3 100644 --- a/cloudflare/test/projects.test.ts +++ b/cloudflare/test/projects.test.ts @@ -234,6 +234,12 @@ function github( return { calls, request, service: new GitHubService(testEnv, request as typeof fetch) }; } +/** A project read's activity query: not its quest blobs, nor its batch of open pull requests' bodies. */ +const activityRead = (call: ReturnType["calls"][number]) => + call.path === "/graphql" && + !call.body.query.includes("object(oid:") && + !call.body.query.includes("b0:pullRequest("); + async function state() { return env.DB.prepare("SELECT * FROM github_projects WHERE id=10").first<{ state: string }>(); } @@ -1188,7 +1194,7 @@ describe("enrolled project observations", () => { const result = await second.service.projectState(repo, "enrollment-test", true); expect(result!.complete).toBe(true); expect(result!.activity).toHaveLength(4); - const query = second.calls.filter((c) => c.path === "/graphql").at(-1)!; + const query = second.calls.filter(activityRead).at(-1)!; expect(query.body.variables.issues).toBe("issue-cursor"); expect(query.body.query).not.toContain("pullRequests(first:50"); const closed = activity(); @@ -1227,7 +1233,7 @@ describe("enrolled project observations", () => { onActivity: async () => void (await delivery("status", event)), }); const result = await second.service.projectState(repo, "enrollment-test", true); - const query = second.calls.filter((c) => c.path === "/graphql").at(-1)!; + const query = second.calls.filter(activityRead).at(-1)!; expect(query.body.variables.issues).toBe("issue-cursor"); expect(result!.complete).toBe(true); expect(result!.project.available).toEqual(["quest/first.md"]); @@ -1253,7 +1259,7 @@ describe("enrolled project observations", () => { }, }); const result = await second.service.projectState(repo, "enrollment-test", true); - expect(second.calls.filter((c) => c.path === "/graphql").at(-1)!.body.variables.issues).toBe("issue-cursor"); + expect(second.calls.filter(activityRead).at(-1)!.body.variables.issues).toBe("issue-cursor"); expect(result!.project.available).toEqual([]); expect(result!.project.changes).toMatchObject([{ quest: "quest/first.md" }]); }); @@ -1453,7 +1459,7 @@ describe("enrolled project observations", () => { expect((await readActivity(env.DB, 10)).items.some((item) => item.id === "200")).toBe(false); }); - it("reads bodies once per open draft and never in metadata/history pages", async () => { + it("reads open pull requests' bodies in one batch and never in metadata/history pages", async () => { await enrolled(); const page = activity(); Object.assign(page.pullRequests!.nodes[0]!, { isDraft: true, body: "## Decisions needed\n\nChoose." }); @@ -1465,13 +1471,52 @@ describe("enrolled project observations", () => { expect(queries).toHaveLength(2); expect(queries[0]!.body.query).not.toMatch(/\bbody\b/); expect(queries[1]!.body.query).toContain("b0:pullRequest(number:2)"); - expect(queries[1]!.body.query).not.toContain("number:3"); + // An open pull request that is not a draft is read too, for the reviews on its head. + expect(queries[1]!.body.query).toContain("b1:pullRequest(number:3)"); + expect(queries[1]!.body.variables.app).toBe(Number(testEnv.GITHUB_APP_ID)); expect(queries[1]!.body.query).not.toContain("number:5"); expect((result!.activity.find((item) => item.id === "200")!.data as { handedBack: boolean }).handedBack).toBe( true, ); }); + it("records why the land gate would refuse each open pull request's head, without keeping what it read", async () => { + await enrolled(); + const page = activity(); + const human = page.open!.nodes[1]!; + const read = { + ...human, + body: `## Review\n\nClean on ${commit}.`, + authorAssociation: "CONTRIBUTOR", + author: { __typename: "User", login: "human" }, + userContentEdits: { nodes: [] }, + latestOpinionatedReviews: { nodes: [] }, + tip: { nodes: [{ commit: { oid: commit, parents: { totalCount: 1 }, checkSuites: { nodes: [] } } }] }, + }; + const result = await github({ activity: page, entities: { 3: read } }).service.projectState( + repo, + "enrollment-test", + true, + ); + const data = (id: string) => result!.activity.find((item) => item.id === id)!.data as Record; + // A contributor's own record does not count, so the gate would refuse the head. + expect(data("201").unreviewed).toBe( + `No review counts on ${commit.slice(0, 7)} yet. kix lands only a reviewed head. Approve ${commit.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(data("201")).not.toHaveProperty("body"); + expect(data("201")).not.toHaveProperty("tip"); + // A read without the reviews flags nothing. + expect(data("200").unreviewed).toBeNull(); + // The same read with a member's record counts it. + const member = await github({ + activity: page, + entities: { 3: { ...read, authorAssociation: "MEMBER" } }, + }).service.projectState(repo, "enrollment-test", true); + expect((member!.activity.find((item) => item.id === "201")!.data as { unreviewed: unknown }).unreviewed).toBe( + null, + ); + }); + it("does not publish mixed draft-body and metadata revisions", async () => { await enrolled(); const page = activity(); @@ -1564,8 +1609,7 @@ describe("webhook reconciliation", () => { vi.spyOn(globalThis, "fetch").mockImplementation(mock.request as typeof fetch); return mock; } - const reads = (calls: ReturnType["calls"]) => - calls.filter((call) => call.path === "/graphql" && !call.body.query.includes("object(oid:")); + const reads = (calls: ReturnType["calls"]) => calls.filter(activityRead); const opened = (id: number, number: number, id_ = crypto.randomUUID()) => delivery("issues", { ...event, action: "opened", issue: { id, number } }, id_, {}, testEnv); async function row(id: string) { @@ -1720,6 +1764,30 @@ describe("webhook reconciliation", () => { expect(await row("999")).toBeNull(); }); + it("rereads the pull requests the App's own review check names, and no other check's", async () => { + await enrolled(); + const { calls } = mocked({ entities: { 2: { ...pull(), comments: { totalCount: 3 } } } }); + const run = (name: string, app: number, number = 2) => ({ + ...event, + action: "completed", + check_run: { name, app: { id: app }, pull_requests: [{ id: 200, number, base: { repo: { id: 10 } } }] }, + }); + const app = Number(testEnv.GITHUB_APP_ID); + // CI's checks, another App's \`quest review\`, and a pull request of another repository cost no read. + for (const other of [run("test", app), run("quest review", app + 1)]) + expect((await delivery("check_run", other, undefined, {}, testEnv)).status).toBe(202); + const foreign = run("quest review", app); + foreign.check_run.pull_requests[0]!.base.repo.id = 11; + await delivery("check_run", foreign, undefined, {}, testEnv); + expect(reads(calls)).toHaveLength(0); + await delivery("check_run", run("quest review", app), undefined, {}, testEnv); + expect(reads(calls)).toHaveLength(1); + expect(reads(calls)[0]!.body.query).toContain("s0:pullRequest(number:2)"); + const read = await row("200"); + expect(read).toMatchObject({ stale: 0 }); + expect(JSON.parse(read!.data!).comments).toEqual({ totalCount: 3 }); + }); + it("records a failed read without retrying, and a later webhook reads again", async () => { await enrolled(); const failed = mocked({ activityErrors: true }); diff --git a/cloudflare/test/review.test.ts b/cloudflare/test/review.test.ts index b651beea..a46756b9 100644 --- a/cloudflare/test/review.test.ts +++ b/cloudflare/test/review.test.ts @@ -1,6 +1,6 @@ import { createHash } from "node:crypto"; import { describe, expect, it } from "vitest"; -import { type Reads, unreviewed } from "../src/github/review"; +import { type Observed, type Reads, observedGap, unreviewed } from "../src/github/review"; const APP = 123; const sha = (name: string) => createHash("sha1").update(name).digest("hex"); @@ -272,3 +272,93 @@ describe("the review a landing needs", () => { }); }); }); + +describe("the review the inbox flags as missing before merge", () => { + const head = sha("head"); + /** One read of pull request #1 at `head`, as the inbox's body batch returns it. */ + function observed( + pull: { + approved?: string[]; + checks?: { status: string; conclusion: string | null }[]; + body?: string; + editor?: string; + association?: string; + parents?: number; + } = {}, + ): Observed { + return { + authorAssociation: pull.association ?? "MEMBER", + body: pull.body ?? "", + author: { __typename: "User", login: "author" }, + userContentEdits: { nodes: pull.editor ? [{ editor: { __typename: "User", login: pull.editor } }] : [] }, + latestOpinionatedReviews: { + nodes: (pull.approved ?? []).map((oid) => ({ + state: "APPROVED", + author: { __typename: "User", login: "maintainer" }, + commit: { oid }, + })), + }, + tip: { + nodes: [ + { + commit: { + oid: head, + parents: { totalCount: pull.parents ?? 1 }, + checkSuites: { nodes: pull.checks ? [{ checkRuns: { nodes: pull.checks } }] : [] }, + }, + }, + ], + }, + }; + } + const ask = `kix lands only a reviewed head. Approve ${short("head")} on GitHub`; + + it("flags nothing once an approval, the App's check, or the author's own record counts on the head", () => { + expect(observedGap(observed({ approved: [head] }), head, "main")).toBeNull(); + expect(observedGap(observed({ checks: [{ status: "COMPLETED", conclusion: "SUCCESS" }] }), head, "main")).toBe( + null, + ); + expect(observedGap(observed({ body: `## Review\n\nClean on ${head}.` }), head, "main")).toBeNull(); + }); + + it("gives the land gate's reason for a head nothing counts on", () => { + expect(observedGap(observed(), head, "main")).toBe( + `No review counts on ${short("head")} yet. ${ask} 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(observedGap(observed({ approved: [sha("old")] }), head, "main")).toMatch( + new RegExp( + `^The approval of ${short("old")} does not carry to ${short("head")}: .* merges of main\\. ${ask}`, + ), + ); + for (const conclusion of ["FAILURE", null]) + expect(observedGap(observed({ checks: [{ status: "IN_PROGRESS", conclusion }] }), head, "main")).toMatch( + /^No review counts/, + ); + }); + + it.each([ + ["the author's association rules out write access", { association: "CONTRIBUTOR" }], + ["someone else last edited the body", { editor: "maintainer" }], + ["the record names another commit", { body: `## Review\n\nClean on ${sha("old")}.` }], + ])("does not count the author's record when %s", (_, overrides) => { + const body = `## Review\n\nClean on ${head}.`; + expect(observedGap(observed({ body, ...overrides }), head, "main")).toMatch(/^No review counts/); + }); + + it("says a merge head's earlier review may still carry, which the click checks", () => { + expect(observedGap(observed({ parents: 2 }), head, "main")).toMatch( + new RegExp( + `^No review counts on ${short("head")} itself yet\\. ${short("head")} is a merge, .* which kix checks when you merge\\. ${ask}`, + ), + ); + expect(observedGap(observed({ parents: 2, approved: [sha("old")] }), head, "main")).toMatch( + new RegExp(`^The approval of ${short("old")} is of a commit before ${short("head")}\\.`), + ); + }); + + it("flags nothing from a read of another head or without the reviews", () => { + expect(observedGap(observed(), sha("other"), "main")).toBeNull(); + const { latestOpinionatedReviews: _, ...partial } = observed(); + expect(observedGap(partial as Observed, head, "main")).toBeNull(); + }); +}); diff --git a/quest/m1/kix/README.md b/quest/m1/kix/README.md index 6b4bd4e9..1b83b236 100644 --- a/quest/m1/kix/README.md +++ b/quest/m1/kix/README.md @@ -18,4 +18,3 @@ maintainers, so they need none of this first. - [Read-only CI](/quest/m1/kix/ci-read-only.md) - enrolled repositories keep pull-request CI read-only, checked at enrollment, with every trust boundary written down - [Contributor sandboxing](/quest/m1/kix/sandbox/README.md) - contributors' PR heads build in a VM, on stock Ubuntu and verified on macOS - [Onboarding guide](/quest/m1/kix/guide.md) - a complete maintainer path, from runner setup through a reviewed PR -- [Missing review flag](/quest/m1/kix/review-flag.md) - the inbox shows a PR's missing review before the merge click diff --git a/quest/m1/kix/review-flag.md b/quest/m1/kix/review-flag.md deleted file mode 100644 index fb5f949f..00000000 --- a/quest/m1/kix/review-flag.md +++ /dev/null @@ -1,20 +0,0 @@ -# [S] The inbox flags a missing review before the merge click - -## Goal - -kix's inbox shows, on each PR item, when no review counts on its head yet, -with the reason the land gate would give. A maintainer sees it before -picking merge, instead of having the click refused. - -## Plan - -Decided 2026-11-01 (user, /quest-plan of the follow-ups from the -2026-11-01 `/quest-complete` sessions): ✅ plan it, [S], m1, from -[moq-dev/quest#351](https://github.com/moq-dev/quest/pull/351)'s suggested -follow-ups. - -- #351 refuses the inbox's **Merge this PR** with the gate's reason only - after the click. Flag it up front from what the inbox already observes, - 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.