From bf7729201302e63e74ca237a9f0445fbb9145b01 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 27 Oct 2026 21:46:17 -0700 Subject: [PATCH 1/2] chore(quest): claim quest/m0/kix/context-pages.md Quest-Claim: e1b5c66d90ad2af4 From acd4b1e623ce59e30b370e7ae755cd6dda483be2 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 27 Oct 2026 21:52:18 -0700 Subject: [PATCH 2/2] fix(runner): read every page of PR context --- cloudflare/dev/github.ts | 79 +++++++++------ cloudflare/src/github/service.ts | 167 +++++++++++++++++++++---------- cloudflare/test/publish.test.ts | 111 +++++++++++++++++++- quest/m0/kix/README.md | 1 - quest/m0/kix/context-pages.md | 20 ---- 5 files changed, 272 insertions(+), 106 deletions(-) delete mode 100644 quest/m0/kix/context-pages.md diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index b28a4f7e..7c3129d2 100644 --- a/cloudflare/dev/github.ts +++ b/cloudflare/dev/github.ts @@ -157,51 +157,66 @@ function pullNode(repo: Repo, pull: Pull) { }; } -const comments = (list: Comment[] = []) => ({ - nodes: list.map((c) => ({ author: c.author === null ? null : { login: c.author }, body: c.body, createdAt: c.at })), -}); +/** One connection page, using opaque positions just as the service uses GitHub's cursors. */ +function connection(nodes: T[], size: number, after: unknown = null) { + const start = typeof after === "string" ? Number(after) : 0; + const end = start + size; + return { + nodes: nodes.slice(start, end), + pageInfo: { hasNextPage: end < nodes.length, endCursor: end < nodes.length ? String(end) : null }, + }; +} + +const comments = (list: Comment[] = [], size = 100, after: unknown = null) => + connection( + list.map((c) => ({ author: c.author === null ? null : { login: c.author }, body: c.body, createdAt: c.at })), + size, + after, + ); /** An issue with its discussion, as a job's agent reads it. */ -const issueTarget = (item: Issue) => ({ +const issueTarget = (item: Issue, after: unknown = null) => ({ number: item.number, title: item.title, state: item.closed ? "CLOSED" : "OPEN", body: item.body ?? "", author: { login: item.author }, - comments: comments(item.comments), + comments: comments(item.comments, 100, after), }); +const threadId = (repo: Repo, pull: Pull, index: number) => `PRT_${databaseId(repo, pull.number)}_${index}`; + /** A pull request with its discussion, reviews, threads, and checks, as a job's agent reads it. */ function pullTarget(repo: Repo, pull: Pull, after: unknown) { const oid = headOf(repo, pull); - // Review threads are read 100 at a time, each page `after` the cursor of the one before. - const threads = pull.threads ?? []; - const start = typeof after === "string" ? Number(after) : 0; - const end = Math.min(start + 100, threads.length); return { - ...issueTarget(pull), + ...issueTarget(pull, after), isDraft: !!pull.draft, baseRefName: "main", headRefName: pull.branch, headRefOid: oid, - reviews: { - nodes: (pull.reviews ?? []).map((r) => ({ + reviews: connection( + (pull.reviews ?? []).map((r) => ({ author: { login: r.author }, state: r.state, body: r.body ?? "", submittedAt: updatedAt, })), - }, - reviewThreads: { - pageInfo: { hasNextPage: end < threads.length, endCursor: String(end) }, - nodes: threads.slice(start, end).map((t) => ({ + 50, + after, + ), + reviewThreads: connection( + (pull.threads ?? []).map((t, index) => ({ + id: threadId(repo, pull, index), isResolved: !!t.resolved, isOutdated: !!t.outdated, path: t.path, line: t.line, - comments: comments(t.comments), + comments: comments(t.comments, 50), })), - }, + 100, + after, + ), commits: { nodes: [ { @@ -209,15 +224,7 @@ function pullTarget(repo: Repo, pull: Pull, after: unknown) { oid, statusCheckRollup: pull.checks && { state: pull.checks, - contexts: { - nodes: (pull.runs ?? []).map((run) => ({ - __typename: "CheckRun", - name: run.name, - status: "COMPLETED", - conclusion: run.conclusion, - detailsUrl: run.url, - })), - }, + contexts: contextPage(pull, null), }, }, }, @@ -390,8 +397,8 @@ function graphql(repo: Repo, query: string, variables: Record, const issue = repo.issues.find((i) => i.number === variables.number); const found = field === "pullRequest" - ? pull && pullTarget(repo, pull, variables.threads) - : issue && issueTarget(issue); + ? pull && pullTarget(repo, pull, variables.after) + : issue && issueTarget(issue, variables.after); result[field] = found ?? null; if (!found) errors.push({ type: "NOT_FOUND", path: ["repository", field] }); } @@ -591,6 +598,20 @@ export function fakeGitHub(world: World) { } return json({ errors: [{ type: "NOT_FOUND" }] }); } + // Later pages of one review thread's comments, addressed by its node ID. + if (query.includes("... on PullRequestReviewThread")) { + for (const repo of world.repos) + for (const pull of repo.pulls) + for (const [index, thread] of (pull.threads ?? []).entries()) + if (threadId(repo, pull, index) === variables.id) { + if (!scope.repositories.has(`${repo.owner}/${repo.name}`)) + return json({ errors: [{ type: "NOT_FOUND" }] }); + return json({ + data: { node: { comments: comments(thread.comments, 50, variables.after) } }, + }); + } + return json({ data: { node: null } }); + } // A pull request by node ID, or turning its auto-merge off. if (query.includes("node(id:") || query.includes("disablePullRequestAutoMerge(")) { for (const repo of world.repos) diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 94fbb16c..50906da5 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -874,24 +874,28 @@ export class GitHubService { if (!kind) throw new GitHubError("Only issue and pull request jobs have a target to read"); const permissions: Permissions = kind === "issues" ? { issues: "read" } : { pull_requests: "read", checks: "read", statuses: "read" }; - const comments = "comments(last: 100) { nodes { author { login } body createdAt } }"; - // One page of review threads; a pull request's later pages are read `after` the one before. - const threads = - "reviewThreads(first: 100, after: $threads) { nodes { isResolved isOutdated path line comments(first: 50) { nodes { author { login } body createdAt } } } pageInfo { hasNextPage endCursor } }"; + const pageInfo = "pageInfo { hasNextPage endCursor }"; + const commentFields = "nodes { author { login } body createdAt }"; + const comments = `comments(first: 100, after: $after) { ${commentFields} ${pageInfo} }`; + const reviews = `reviews(first: 50, after: $after) { nodes { author { login } state body submittedAt } ${pageInfo} }`; + const threads = `reviewThreads(first: 100, after: $after) { nodes { id isResolved isOutdated path line comments(first: 50) { ${commentFields} ${pageInfo} } } ${pageInfo} }`; + const contexts = (after: boolean) => + `contexts(first: 100${after ? ", after: $after" : ""}) { nodes { __typename ... on CheckRun { name status conclusion detailsUrl } ... on StatusContext { context state description targetUrl } } ${pageInfo} }`; const query = kind === "issues" ? `issue(number: $number) { number title state body author { login } ${comments} }` - : `pullRequest(number: $number) { number title state body isDraft author { login } baseRefName headRefName headRefOid ${comments} - reviews(last: 50) { nodes { author { login } state body submittedAt } } - ${threads} - commits(last: 1) { nodes { commit { oid statusCheckRollup { state contexts(first: 100) { nodes { __typename ... on CheckRun { name status conclusion detailsUrl } ... on StatusContext { context state description targetUrl } } } } } } } }`; + : `pullRequest(number: $number) { number title state body isDraft author { login } baseRefName headRefName headRefOid ${comments} ${reviews} ${threads} + commits(last: 1) { nodes { commit { oid statusCheckRollup { state ${contexts(false)} } } } } }`; + type Connection = { nodes: T[]; pageInfo: { hasNextPage: boolean; endCursor: string | null } }; type Comment = { author: { login: string } | null; body: string; createdAt?: string; submittedAt?: string }; + type Review = Comment & { state: string }; type Thread = { + id: string; isResolved: boolean; isOutdated: boolean; path: string; line: number | null; - comments: { nodes: Comment[] }; + comments: Connection; }; type Check = { __typename: string; @@ -904,6 +908,7 @@ export class GitHubService { description?: string | null; targetUrl?: string | null; }; + type Rollup = { state: string; contexts: Connection }; type Item = { number: number; title: string; @@ -914,82 +919,134 @@ export class GitHubService { baseRefName?: string; headRefName?: string; headRefOid?: string; - comments: { nodes: Comment[] }; - reviews?: { nodes: (Comment & { state: string })[] }; - reviewThreads?: { nodes: Thread[]; pageInfo: { hasNextPage: boolean; endCursor: string | null } }; - commits?: { - nodes: { - commit: { oid: string; statusCheckRollup: { state: string; contexts: { nodes: Check[] } } | null }; - }[]; - }; + comments: Connection; + reviews?: Connection; + reviewThreads?: Connection; + commits?: { nodes: { commit: { oid: string; statusCheckRollup: Rollup | null } }[] }; }; return this.#authorized(repo, userId, permissions, async (token) => { - const read = async (fields: string, after: string | null = null) => { - const pull = kind === "pulls"; - const result = await this.#api<{ - data?: { repository: { issue?: Item | null; pullRequest?: Item | null } | null }; - errors?: unknown[]; - }>(token, "/graphql", "POST", { - query: `query($owner: String!, $name: String!, $number: Int!${pull ? ", $threads: String" : ""}) { repository(owner: $owner, name: $name) { ${fields} } }`, - variables: { - owner: repo.owner, - name: repo.name, - number: number(Number(value)), - ...(pull && { threads: after }), - }, + const request = async (query: string, variables: Record): Promise => { + const result = await this.#api<{ data?: T; errors?: unknown[] }>(token, "/graphql", "POST", { + query, + variables, }); - const item = result.data?.repository?.[pull ? "pullRequest" : "issue"]; - if (result.errors?.length || !item) throw new GitHubError("This issue or pull request is unavailable"); + if (result.errors?.length || !result.data) + throw new GitHubError("This issue or pull request is unavailable"); + return result.data; + }; + const read = async (fields: string, after: string | null = null) => { + const result = await request<{ repository: { issue?: Item | null; pullRequest?: Item | null } | null }>( + `query($owner: String!, $name: String!, $number: Int!, $after: String) { repository(owner: $owner, name: $name) { ${fields} } }`, + { owner: repo.owner, name: repo.name, number: number(Number(value)), after }, + ); + const item = result.repository?.[kind === "pulls" ? "pullRequest" : "issue"]; + if (!item) throw new GitHubError("This issue or pull request is unavailable"); return item; }; + // Each connection owns its cursor history, including nested comments and fixed-head checks. + const collect = async ( + first: Connection | undefined, + next: (after: string) => Promise | undefined>, + name: string, + ): Promise => { + const all: T[] = []; + const seen = new Set(); + let page = first; + for (;;) { + if (!page) throw new GitHubError(`This target's ${name} are unavailable`); + all.push(...page.nodes); + if (!page.pageInfo.hasNextPage) return all; + const cursor = page.pageInfo.endCursor; + if (!cursor || seen.has(cursor)) throw new GitHubError(`This target's ${name} are unavailable`); + seen.add(cursor); + page = await next(cursor); + } + }; const item = await read(query); - // Every thread is read before the unresolved ones are kept: a busy pull request has more than a page. - const all: Thread[] = []; - let after: string | null = null; - for (let page = item.reviewThreads; page;) { - all.push(...page.nodes); - if (!page.pageInfo.hasNextPage) break; - if (!page.pageInfo.endCursor || page.pageInfo.endCursor === after) - throw new GitHubError("This pull request's review threads are unavailable"); - after = page.pageInfo.endCursor; - page = (await read(`pullRequest(number: $number) { ${threads} }`, after)).reviewThreads; - if (!page) throw new GitHubError("This pull request's review threads are unavailable"); - } const comment = (c: Comment) => ({ author: c.author?.login ?? null, body: c.body, at: c.createdAt ?? c.submittedAt ?? null, }); + const discussion = await collect( + item.comments, + async (after) => + ( + await read( + `${kind === "pulls" ? "pullRequest" : "issue"}(number: $number) { ${comments} }`, + after, + ) + ).comments, + "comments", + ); const common = { number: item.number, title: item.title, state: item.state, author: item.author?.login ?? null, body: item.body, - comments: item.comments.nodes.map(comment), + comments: discussion.map(comment), }; if (kind === "issues") return { kind: "issue" as const, ...common }; + const allReviews = await collect( + item.reviews, + async (after) => (await read(`pullRequest(number: $number) { ${reviews} }`, after)).reviews, + "reviews", + ); + const allThreads = await collect( + item.reviewThreads, + async (after) => (await read(`pullRequest(number: $number) { ${threads} }`, after)).reviewThreads, + "review threads", + ); + const unresolved = []; + for (const thread of allThreads.filter((t) => !t.isResolved)) { + const replies = await collect( + thread.comments, + async (after) => { + const data = await request<{ node: { comments: Connection } | null }>( + `query($id: ID!, $after: String) { node(id: $id) { ... on PullRequestReviewThread { comments(first: 50, after: $after) { ${commentFields} ${pageInfo} } } } }`, + { id: thread.id, after }, + ); + return data.node?.comments; + }, + "review thread comments", + ); + unresolved.push({ + path: thread.path, + line: thread.line, + outdated: thread.isOutdated, + comments: replies.map(comment), + }); + } const head = item.commits?.nodes.at(-1)?.commit; + const allChecks = head?.statusCheckRollup + ? await collect( + head.statusCheckRollup.contexts, + async (after) => { + const data = await request<{ + repository: { object: { statusCheckRollup: Rollup | null } | null } | null; + }>( + `query($owner: String!, $name: String!, $oid: GitObjectID!, $after: String) { repository(owner: $owner, name: $name) { object(oid: $oid) { ... on Commit { statusCheckRollup { ${contexts(true)} } } } } }`, + { owner: repo.owner, name: repo.name, oid: head.oid, after }, + ); + return data.repository?.object?.statusCheckRollup?.contexts; + }, + "check contexts", + ) + : []; return { kind: "pull" as const, ...common, draft: item.isDraft ?? false, base: item.baseRefName ?? null, head: { ref: item.headRefName ?? null, oid: item.headRefOid ?? null }, - reviews: (item.reviews?.nodes ?? []).map((r) => ({ ...comment(r), state: r.state })), + reviews: allReviews.map((r) => ({ ...comment(r), state: r.state })), // Resolved threads need nothing more; outdated ones may still need an answer. - threads: all - .filter((t) => !t.isResolved) - .map((t) => ({ - path: t.path, - line: t.line, - outdated: t.isOutdated, - comments: t.comments.nodes.map(comment), - })), + threads: unresolved, checks: { commit: head?.oid ?? null, state: head?.statusCheckRollup?.state ?? null, - runs: (head?.statusCheckRollup?.contexts.nodes ?? []).map((c) => + runs: allChecks.map((c) => c.__typename === "CheckRun" ? { name: c.name!, state: c.conclusion ?? c.status ?? null, url: c.detailsUrl ?? null } : { diff --git a/cloudflare/test/publish.test.ts b/cloudflare/test/publish.test.ts index 2ef282a4..e8d547ac 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -1317,6 +1317,7 @@ describe("job context", () => { owner: "upstream", name: "project", number: 4, + after: null, }); // Only the current attempt of a known runner reads it. expect((await read(id, 2)).status).toBe(409); @@ -1382,10 +1383,118 @@ describe("job context", () => { expect( requests() .filter((r) => r.path === "/graphql") - .map((r) => r.body.variables.threads), + .map((r) => r.body.variables.after), ).toEqual([null, "100", "200"]); }); + it("reads every page of comments, reviews, nested replies, and mixed check contexts", async () => { + const pull = project.pulls[0]!; + pull.comments = Array.from({ length: 251 }, (_, i) => ({ + author: i ? "reader" : null, + body: `comment ${i}`, + at, + })); + pull.reviews = Array.from({ length: 151 }, (_, i) => ({ + author: "reviewer", + state: "COMMENTED", + body: `review ${i}`, + })); + pull.threads = Array.from({ length: 101 }, (_, i) => ({ + path: `file${i}.ts`, + line: i, + resolved: i !== 100, + outdated: i === 100, + comments: Array.from({ length: i === 100 ? 151 : 1 }, (_, j) => ({ + author: "reviewer", + body: `reply ${j}`, + at, + })), + })); + pull.runs = Array.from({ length: 251 }, (_, i) => ({ + name: `check ${i}`, + conclusion: "SUCCESS", + url: `https://ci.test/${i}`, + context: i % 2 === 1, + })); + const id = await running({ + workflow: "iterate", + target: "pulls/2", + revision: prHead, + branch: "contributor/fix", + trusted: false, + }); + const response = await read(id, 1); + expect(response.status).toBe(200); + const body = (await response.json()) as Record; + expect(body.comments).toEqual(pull.comments.map((c) => c)); + expect(body.reviews.map((r: { body: string }) => r.body)).toEqual(pull.reviews.map((r) => r.body)); + expect(body.threads).toEqual([ + { path: "file100.ts", line: 100, outdated: true, comments: pull.threads[100]!.comments }, + ]); + expect(body.checks.runs).toEqual( + pull.runs.map((r) => ({ + name: r.name, + state: r.conclusion, + url: r.url, + ...(r.context && { description: null }), + })), + ); + }); + + it.each(["comments", "reviews", "reviewThreads", "thread comments", "checks"])( + "refuses missing or cycling cursors in %s", + async (connection) => { + const pull = project.pulls[0]!; + pull.comments = Array.from({ length: 201 }, (_, i) => ({ author: "reader", body: String(i), at })); + pull.reviews = Array.from({ length: 101 }, () => ({ author: "reviewer", state: "COMMENTED" })); + pull.threads = Array.from({ length: 201 }, (_, i) => ({ + path: String(i), + line: i, + comments: Array.from({ length: 101 }, () => ({ author: "reviewer", body: "reply", at })), + })); + pull.runs = Array.from({ length: 201 }, (_, i) => ({ + name: String(i), + conclusion: "SUCCESS", + url: "https://ci.test", + })); + const id = await running({ + workflow: "iterate", + target: "pulls/2", + revision: prHead, + branch: "contributor/fix", + trusted: false, + }); + for (const invalid of ["missing", "cycle"] as const) { + let pages = 0; + vi.mocked(globalThis.fetch).mockImplementation(async (input, init) => { + const response = await github(input, init); + if (new URL(String(input)).pathname !== "/graphql") return response; + const data = (await response.json()) as Record; + const item = data.data?.repository?.pullRequest; + const page = + connection === "thread comments" + ? data.data?.node?.comments + : connection === "checks" + ? (data.data?.repository?.object?.statusCheckRollup?.contexts ?? + item?.commits?.nodes[0]?.commit?.statusCheckRollup?.contexts) + : item?.[connection]; + if (page) { + pages++; + page.pageInfo = { + hasNextPage: true, + endCursor: invalid === "missing" ? null : pages % 2 ? "100" : "200", + }; + } + return Response.json(data); + }); + const response = await read(id, 1); + expect(response.status).toBe(403); + expect(await response.json()).toMatchObject({ error: expect.stringContaining("unavailable") }); + expect(pages).toBeGreaterThan(0); + } + }, + ); + it("has nothing to read for a quest's job", async () => { const id = await running(); const response = await read(id, 1); diff --git a/quest/m0/kix/README.md b/quest/m0/kix/README.md index ffc40d3d..7a0798fb 100644 --- a/quest/m0/kix/README.md +++ b/quest/m0/kix/README.md @@ -124,7 +124,6 @@ fixes what it finds: - [Deploy kix.quest](/quest/m0/kix/deploy.md) - the operator registers the App and deploys the Worker with GitHub sign-in at kix.quest - [Upstream branches](/quest/m0/kix/upstream-branches.md) - jobs publish on the enrolled repository itself, with no service forks - [Operator setup](/quest/m0/kix/operator-setup.md) - pilot installations and a verified test PR on kixelated/quest and moq-dev/moq -- [Context pages](/quest/m0/kix/context-pages.md) - a job's `context` read returns every comment, review, and check of a busy PR - [Merge holds](/quest/m0/kix/merge-holds.md) - ended merge jobs never leave a `quest:merge` hold, through one shared hold rule - [App sign-in](/quest/m0/kix/app-sign-in.md) - one GitHub screen signs in and installs the App, with no separate OAuth app - [Pick repositories](/quest/m0/kix/pick-repos.md) - after installing, the maintainer enables repositories from a list diff --git a/quest/m0/kix/context-pages.md b/quest/m0/kix/context-pages.md deleted file mode 100644 index ad9fd4b6..00000000 --- a/quest/m0/kix/context-pages.md +++ /dev/null @@ -1,20 +0,0 @@ -# [S] Read every page of a PR's context - -## Goal - -The runner's `context` tool returns all of a pull request's comments, -reviews, review-thread comments, and check contexts, however busy the PR is. - -## Plan - -Decided 2026-10-24 (user) as a follow-up of -[kixelated/quest#210](https://github.com/kixelated/quest/pull/210), which -paged review threads in `cloudflare/src/github/service.ts`: - -- The same query still caps the first 50 comments in each thread, the last - 100 PR comments, the last 50 reviews, and 100 check contexts, so a busy PR - silently drops the rest. Follow each connection's `pageInfo` cursor, and - raise an error on a missing or repeated cursor instead of looping, as #210 - does. -- Test each list past its page size with the seeded fake GitHub - (`cloudflare/dev/github.ts`).