feat(kix): land only heads with a review that counts on them

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)):

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<string, unknown>[];
 	/** The default branch's classic protection, as `GET /branches/main/protection` returns it; none when absent. */
 	protection?: Record<string, any>;
+	/** Rulesets by ID, as `GET /rulesets/<id>` shows them to the App. */
+	rulesets?: Record<number, { current_user_can_bypass: "always" | "pull_requests_only" | "never" }>;
 };
 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<string, NonNullable<Pull["reviews"]>[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<string, unknown>)
 		};
 	}
 
+	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<string>()): Set<string> => {
+				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;

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" },
 			{