From ee73c6d70335d6dd9cd38b89636a8754b2cdc062 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 19:43:35 -0800 Subject: [PATCH 1/2] chore(quest): claim quest/m1/kix/review-flag.md Quest-Claim: 4e9c19adbb9d3e07 From 12f9b2e0b027733cb14691a3b278c364eace6a6d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 20:05:19 -0800 Subject: [PATCH 2/2] feat(kix): flag a missing review in the inbox before the merge click Each project read now takes every open PR's body, author association, writers' latest reviews, and the head commit's parents and the App's `quest review` check in the one batched GraphQL request that already classified drafts. It keeps only the land gate's reason, which the inbox shows beside Merge this PR. The click's live check stays the authority. The App's own `quest review` check run now asks for a read of the PRs it names, so a runner's review clears the flag without a refresh. Finishes quest/m1/kix/review-flag.md. Co-Authored-By: Claude Opus 5.5 --- cloudflare/GITHUB.md | 23 +++++- cloudflare/dev/github.ts | 44 +++++++++- cloudflare/src/github/review.ts | 124 +++++++++++++++++++++++----- cloudflare/src/github/service.ts | 89 ++++++++++---------- cloudflare/src/github/state.ts | 6 ++ cloudflare/src/github/webhook.ts | 31 ++++++- cloudflare/src/inbox/page.tsx | 2 + cloudflare/test/github.test.ts | 2 +- cloudflare/test/inbox.test.ts | 15 ++++ cloudflare/test/project-fixtures.ts | 2 +- cloudflare/test/projects.test.ts | 82 ++++++++++++++++-- cloudflare/test/review.test.ts | 92 ++++++++++++++++++++- quest/m1/kix/README.md | 1 - quest/m1/kix/review-flag.md | 20 ----- 14 files changed, 430 insertions(+), 103 deletions(-) delete mode 100644 quest/m1/kix/review-flag.md diff --git a/cloudflare/GITHUB.md b/cloudflare/GITHUB.md index 2f722846..fbf9a3be 100644 --- a/cloudflare/GITHUB.md +++ b/cloudflare/GITHUB.md @@ -246,6 +246,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 @@ -508,7 +525,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 @@ -536,7 +554,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 b988b536..2b22cd91 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. */ @@ -157,12 +158,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, @@ -431,7 +466,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 } } }; } @@ -515,7 +550,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 0cdefb16..ca58a17e 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -2,7 +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 { OBSERVED, type Observed, observedGap, unreviewed } from "./review"; import { positiveId } from "./webhook"; import { activityQuery, @@ -808,7 +808,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)); @@ -822,16 +822,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( @@ -844,44 +850,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 45d200e2..0854fdf0 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 { GitHubService } from "./service"; export function positiveId(value: unknown): value is number { @@ -169,6 +170,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( @@ -176,9 +201,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 c8f09f5f..76da5665 100644 --- a/cloudflare/src/inbox/page.tsx +++ b/cloudflare/src/inbox/page.tsx @@ -424,6 +424,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 e4f110b0..f5cd83de 100644 --- a/cloudflare/test/github.test.ts +++ b/cloudflare/test/github.test.ts @@ -219,7 +219,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 04cf7301..e3c3cf56 100644 --- a/cloudflare/test/inbox.test.ts +++ b/cloudflare/test/inbox.test.ts @@ -261,6 +261,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 adbb9098..3cc5709e 100644 --- a/cloudflare/test/project-fixtures.ts +++ b/cloudflare/test/project-fixtures.ts @@ -148,7 +148,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 5aa6b833..d4b72e34 100644 --- a/cloudflare/test/projects.test.ts +++ b/cloudflare/test/projects.test.ts @@ -231,6 +231,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 }>(); } @@ -1145,7 +1151,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(); @@ -1184,7 +1190,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"]); @@ -1210,7 +1216,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" }]); }); @@ -1410,7 +1416,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." }); @@ -1422,13 +1428,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(); @@ -1521,8 +1566,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) { @@ -1677,6 +1721,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 0ba6942e..b5ae01ae 100644 --- a/quest/m1/kix/README.md +++ b/quest/m1/kix/README.md @@ -19,4 +19,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.