feat(kix): flag a missing review in the inbox before the merge click

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.
 

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<string, unknown>)
 		};
 	}
 
-	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<string, unknown>)
 	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] });
 	}

cloudflare/src/github/review.ts

@@ -79,17 +79,7 @@ export async function unreviewed(reads: Reads, landing: Landing): Promise<string
 	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 approved = approvals(pull);
 	const recorded = await ownRecord(reads, pull);
 	const commits = new Map<string, Promise<GitCommit>>();
 	const commit = (sha: string) => {
@@ -109,30 +99,124 @@ export async function unreviewed(reads: Reads, landing: Landing): Promise<string
 		// 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);
+	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<string>, 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<PullRow, "author" | "latestOpinionatedReviews">) {
+	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<Set<string>> {
+function named(pull: Pick<PullRow, "author" | "body" | "userContentEdits">): Set<string> {
 	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<PullRow, "body" | "author" | "userContentEdits" | "latestOpinionatedReviews"> & {
+	/** 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<Set<string>> {
+	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();

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<Repository>(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<ReturnType<typeof staleEntities>>,
 	) {
-		// 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<number, (Pull & { body?: string })[]>();
+		const open = new Map<number, (Pull & { body?: string })[]>();
 		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<Pull, "databaseId" | "updatedAt" | "headRefOid" | "state" | "isDraft"> & {
-								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<Pull, "databaseId" | "updatedAt" | "headRefOid" | "state" | "isDraft"> & 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);
 			}
 		}
 	}