From bc06b362d0f7cc7aaa93b90303c592f4a8dd323c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 16 Oct 2026 11:41:06 -0700 Subject: [PATCH 1/2] fix(github): keep project availability live on busy repositories Every observed webhook, CI status included, invalidated the scan and restarted it from page 1, so a repository with more than one page of history never completed and availability stayed pending. Split the scan: a history pass pages every issue and PR and resumes through webhooks, while availability depends only on an occupancy pass over open PRs and quest branches, restarted only by events that can open, close, or claim a quest. Paused projects also cleared only ProjectState.available while the board read project.available, so they still offered /quest-start. Drop the duplicate field so the board reads the cleared value. Co-Authored-By: Claude Opus 5.5 --- cloudflare/GITHUB.md | 27 +++-- cloudflare/migrations/0007_project_scan.sql | 3 + cloudflare/src/github/service.ts | 32 +++--- cloudflare/src/github/state.ts | 105 +++++++++++------- cloudflare/src/github/webhook.ts | 20 +++- cloudflare/test/github.test.ts | 27 +---- cloudflare/test/projects.test.ts | 115 ++++++++++++++++++-- cloudflare/test/webhook.ts | 30 +++++ 8 files changed, 251 insertions(+), 108 deletions(-) create mode 100644 cloudflare/migrations/0007_project_scan.sql create mode 100644 cloudflare/test/webhook.ts diff --git a/cloudflare/GITHUB.md b/cloudflare/GITHUB.md index cf5eacd9..51edd528 100644 --- a/cloudflare/GITHUB.md +++ b/cloudflare/GITHUB.md @@ -215,23 +215,30 @@ snapshot reader and validates the Quest tree with the pure core. The authenticat board and nested quest pages use the existing `Project` seam. Source links retain the exact commit; live project state never replaces GitHub as authority. -Each refresh reconciles one page of native issues, **all** PRs (including human -contributions and closed/merged PRs), and `quest/` branches. Independent opaque -cursors persist across requests. Continue reconciliation until all connections -finish; candidates remain empty while a scan is incomplete or a webhook has -invalidated it. The next completed scan also detects deleted entities and removed -claims. Refreshing a completed scan starts again from the authoritative entity -connections, so missed webhooks and later changes to old entities are recovered. +Each refresh advances two passes with independent opaque cursors. The history +pass reads one page of native issues and **all** PRs (including human +contributions and closed/merged PRs); continue reconciliation until it finishes. +A completed history pass detects deleted entities, and refreshing after one starts +again, so missed webhooks and later changes to old entities are recovered. The +occupancy pass reads open PRs and `quest/` branches, the only activity that +decides candidates, and starts again on every refresh after it completes. Its +pages are small, so candidates are normally current after a single refresh no +matter how long the history is. Candidates remain pending while the occupancy +pass is incomplete or a webhook has invalidated it. This deliberately avoids GitHub's bounded, delayed activity feed; GitHub does [not automatically redeliver failed webhooks](https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries). There is no scheduled background poll in this stage. -A single GraphQL activity query reads up to 50 issues, 50 PRs and 100 branch -claims, keeping external request count independent of entity/file counts. Bulk +A single GraphQL activity query reads up to 50 issues, 50 PRs, 100 open PRs and +100 branch claims, keeping external request count independent of entity/file counts. Bulk D1 writes use `json_each`, avoiding a query per entity or SQLite's parameter limit. A revision comparison commits each page and cursor atomically; concurrent refreshes or an intervening webhook cannot publish over a newer observation. -A webhook restarts an unfinished scan on the next refresh. Installation tokens +Only a PR opening, closing or reopening, a `quest/` branch being created or +deleted, or a push to the default branch invalidates candidates and restarts the +occupancy pass. Checks, statuses, comments, reviews and other pushes wait for the +history pass, so a busy repository neither withholds candidates nor races a +refresh. Installation tokens remain read-only, repository-scoped, and revoked after use. Durable activity items use the GitHub repository ID, entity kind and full database diff --git a/cloudflare/migrations/0007_project_scan.sql b/cloudflare/migrations/0007_project_scan.sql new file mode 100644 index 00000000..5d7fee26 --- /dev/null +++ b/cloudflare/migrations/0007_project_scan.sql @@ -0,0 +1,3 @@ +-- Scans split into history and occupancy passes. Observations are a cache, so +-- drop scans in the old shape; the next refresh starts both passes. +UPDATE github_project_state SET scan=NULL; diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 02fa8098..e7caccb3 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -402,7 +402,7 @@ export class GitHubService { const state = await this.#state(repo, installation); await this.#gate(installation); if (state === null) throw new GitHubError("Project enrollment changed"); - if (result && state !== "enabled") result.available = []; + if (result && state !== "enabled") result.project.available = []; return result; } @@ -428,17 +428,18 @@ export class GitHubService { .bind(repo.id) .first<{ revision: number; scan: string | null; invalidated: number; epoch: string }>(); const previous: Scan | null = row?.scan ? JSON.parse(row.scan) : null; - const scan: Scan = - previous && !previous.complete && row?.invalidated !== 1 - ? previous - : { - id: crypto.randomUUID(), - issues: null, - pulls: null, - refs: null, - claims: [], - complete: false, - }; + // History resumes through any webhook; only occupancy, which is + // small, restarts to observe a possibly opened, closed, or claimed quest. + const scan: Scan = { + history: + previous && !previous.history.complete + ? previous.history + : { id: crypto.randomUUID(), issues: null, pulls: null, complete: false }, + occupancy: + previous && !previous.occupancy.complete && row!.invalidated !== 1 + ? previous.occupancy + : { open: null, refs: null, pulls: [], claims: [], complete: false }, + }; const snapshot = await this.#snapshot(token, repo, current); const response = await this.#api<{ data?: { repository: ActivityPage | null }; errors?: unknown[] }>( token, @@ -449,9 +450,10 @@ export class GitHubService { variables: { owner: repo.owner, name: repo.name, - issues: scan.issues || null, - pulls: scan.pulls || null, - refs: scan.refs || null, + issues: scan.history.issues || null, + pulls: scan.history.pulls || null, + open: scan.occupancy.open || null, + refs: scan.occupancy.refs || null, }, }, ); diff --git a/cloudflare/src/github/state.ts b/cloudflare/src/github/state.ts index 33b78c7e..963ff5d0 100644 --- a/cloudflare/src/github/state.ts +++ b/cloudflare/src/github/state.ts @@ -3,13 +3,12 @@ import type { Project, Snapshot } from "../board/project"; import { GitHubError, type GitHubRepository } from "./service"; import { positiveId } from "./webhook"; +type Cursor = string | null | false; export type Scan = { - id: string; - issues: string | null | false; - pulls: string | null | false; - refs: string | null | false; - claims: string[]; - complete: boolean; + /** Pages every issue and PR, so edits to old entities and deletions are found. */ + history: { id: string; issues: Cursor; pulls: Cursor; complete: boolean }; + /** Pages only what availability depends on: open PRs and quest branches. */ + occupancy: { open: Cursor; refs: Cursor; pulls: string[]; claims: string[]; complete: boolean }; }; type Connection = { nodes: T[]; pageInfo: { hasNextPage: boolean; endCursor: string | null } }; type Entity = { @@ -36,6 +35,7 @@ export type Pull = Entity & { export type ActivityPage = { issues?: Connection; pullRequests?: Connection; + open?: Connection; refs?: Connection<{ name: string; prefix: string }>; }; export type Activity = { @@ -51,31 +51,37 @@ export type ProjectState = { project: Project; activity: Activity[]; claims: string[]; - available: string[]; complete: boolean; observedAt: number; revision: number; }; const entityFields = "databaseId:fullDatabaseId number title state updatedAt author { login } comments { totalCount }"; +const pullFields = `${entityFields} headRefName headRefOid isDraft reviewDecision reviews { totalCount } latestReviews(first:20) { nodes { state author {login} commit {oid} } pageInfo {hasNextPage} } commits(last:1) { nodes { commit { oid statusCheckRollup { state } } } }`; // Cursor variables are declared only for unfinished connections (GraphQL rejects // unused variables). Fixed page sizes bound subrequests, independent of tree size. -export function activityQuery(scan: Scan) { +export function activityQuery({ history, occupancy }: Scan) { const fields: string[] = []; const variables = ["$owner: String!", "$name: String!"]; - if (scan.issues !== false) { + if (history.issues !== false) { variables.push("$issues: String"); fields.push( `issues(first:50,after:$issues,orderBy:{field:CREATED_AT,direction:ASC}) { nodes { ${entityFields} } pageInfo { hasNextPage endCursor } }`, ); } - if (scan.pulls !== false) { + if (history.pulls !== false) { variables.push("$pulls: String"); fields.push( - `pullRequests(first:50,after:$pulls,orderBy:{field:CREATED_AT,direction:ASC}) { nodes { ${entityFields} headRefName headRefOid isDraft reviewDecision reviews { totalCount } latestReviews(first:20) { nodes { state author {login} commit {oid} } pageInfo {hasNextPage} } commits(last:1) { nodes { commit { oid statusCheckRollup { state } } } } } pageInfo { hasNextPage endCursor } }`, + `pullRequests(first:50,after:$pulls,orderBy:{field:CREATED_AT,direction:ASC}) { nodes { ${pullFields} } pageInfo { hasNextPage endCursor } }`, + ); + } + if (occupancy.open !== false) { + variables.push("$open: String"); + fields.push( + `open:pullRequests(states:OPEN,first:100,after:$open,orderBy:{field:CREATED_AT,direction:ASC}) { nodes { ${pullFields} } pageInfo { hasNextPage endCursor } }`, ); } - if (scan.refs !== false) { + if (occupancy.refs !== false) { variables.push("$refs: String"); fields.push( 'refs(refPrefix:"refs/heads/quest/",first:100,after:$refs) { nodes { name prefix } pageInfo { hasNextPage endCursor } }', @@ -84,7 +90,7 @@ export function activityQuery(scan: Scan) { return `query(${variables.join(",")}) { repository(owner:$owner,name:$name) { ${fields.join("\n")} } }`; } -function next(connection: Connection | undefined, cursor: Scan["issues"]) { +function next(connection: Connection | undefined, cursor: Cursor) { if (cursor === false) return false; if (!connection || !Array.isArray(connection.nodes) || !connection.pageInfo) throw new GitHubError("Incomplete project activity"); @@ -111,29 +117,39 @@ export async function reconcile( epoch: string, ) { const now = Date.now(); + const { history, occupancy } = scan; const updated: Scan = { - id: scan.id, - issues: next(page.issues, scan.issues), - pulls: next(page.pullRequests, scan.pulls), - refs: next(page.refs, scan.refs), - claims: [...scan.claims], - complete: false, + history: { + id: history.id, + issues: next(page.issues, history.issues), + pulls: next(page.pullRequests, history.pulls), + complete: false, + }, + occupancy: { + open: next(page.open, occupancy.open), + refs: next(page.refs, occupancy.refs), + pulls: [...occupancy.pulls], + claims: [...occupancy.claims], + complete: false, + }, }; - if (scan.refs !== false) + if (occupancy.refs !== false) for (const ref of page.refs!.nodes) { if (typeof ref.name !== "string" || ref.prefix !== "refs/heads/quest/") throw new GitHubError("Invalid branch observation"); // Ref.name is relative to the requested prefix. - updated.claims.push(`quest/${ref.name}`); + updated.occupancy.claims.push(`quest/${ref.name}`); } - updated.claims = [...new Set(updated.claims)]; - updated.complete = updated.issues === false && updated.pulls === false && updated.refs === false; - const entities: { kind: string; item: Entity }[] = []; - for (const [kind, connection] of [ - ["issue", page.issues], - ["pull", page.pullRequests], + updated.history.complete = updated.history.issues === false && updated.history.pulls === false; + updated.occupancy.complete = updated.occupancy.open === false && updated.occupancy.refs === false; + // Keyed by identity: an open PR can arrive from both PR connections at once. + const entities = new Map(); + for (const [kind, connection, cursor, open] of [ + ["issue", page.issues, history.issues, false], + ["pull", page.pullRequests, history.pulls, false], + ["pull", page.open, occupancy.open, true], ] as const) { - if (scan[kind === "issue" ? "issues" : "pulls"] === false) continue; + if (cursor === false) continue; for (const item of connection!.nodes) { if ( typeof item.databaseId !== "string" || @@ -151,9 +167,12 @@ export async function reconcile( ) throw new GitHubError("Pull request head changed; refresh again"); } - entities.push({ kind, item }); + entities.set(`${kind}/${item.databaseId}`, { kind, item }); + if (open) updated.occupancy.pulls.push(item.databaseId); } } + updated.occupancy.claims = [...new Set(updated.occupancy.claims)]; + updated.occupancy.pulls = [...new Set(updated.occupancy.pulls)]; // json_each keeps the transaction to three statements and a few bindings, // within D1's free query budget and SQLite's bound-parameter limit. const statements = [ @@ -164,12 +183,12 @@ export async function reconcile( FROM json_each(?) WHERE EXISTS(SELECT 1 FROM github_project_state WHERE repository=? AND revision=? AND epoch=?) ON CONFLICT(repository,kind,id) DO UPDATE SET number=excluded.number,changedAt=CASE WHEN github_project_activity.data IS NOT excluded.data THEN excluded.changedAt ELSE github_project_activity.changedAt END,data=excluded.data,seenIn=excluded.seenIn,missing=0`, ) - .bind(repo.id, now, now, scan.id, JSON.stringify(entities), repo.id, revision, epoch), + .bind(repo.id, now, now, history.id, JSON.stringify([...entities.values()]), repo.id, revision, epoch), db .prepare( "UPDATE github_project_activity SET missing=1 WHERE repository=? AND (seenIn IS NULL OR seenIn<>?) AND ?=1 AND EXISTS(SELECT 1 FROM github_project_state WHERE repository=? AND revision=? AND epoch=?)", ) - .bind(repo.id, scan.id, updated.complete ? 1 : 0, repo.id, revision, epoch), + .bind(repo.id, history.id, updated.history.complete ? 1 : 0, repo.id, revision, epoch), db .prepare( "UPDATE github_project_state SET snapshot=?,scan=?,observedAt=?,invalidated=?,revision=revision+1 WHERE repository=? AND revision=? AND epoch=?", @@ -202,8 +221,11 @@ export async function readState(db: D1Database, repo: GitHubRepository): Promise const rows = records[1]!.results as (Omit & { data: string | null })[]; const activity: Activity[] = rows.map((item) => ({ ...item, data: item.data ? JSON.parse(item.data) : null })); const paths = new Set(snapshot.documents.map((d) => d.path)); + // A PR's activity row can still say OPEN until history rereads it, so only + // PRs in the latest occupancy pass count as open. + const open = new Set(scan.occupancy.pulls); const changes = activity - .filter((item) => item.kind === "pull" && !item.missing && item.data?.state === "OPEN") + .filter((item) => item.kind === "pull" && open.has(item.id) && !item.missing && item.data?.state === "OPEN") .flatMap((item) => { const pull = item.data as Pull; const quest = associatedQuest(pull.headRefName, paths); @@ -217,12 +239,16 @@ export async function readState(db: D1Database, repo: GitHubRepository): Promise ] : []; }); - const claims = scan.claims.map((ref) => associatedQuest(ref, paths)).filter((q): q is string => q !== null); - const complete = scan.complete && row.invalidated === 0; + const claims = scan.occupancy.claims + .map((ref) => associatedQuest(ref, paths)) + .filter((q): q is string => q !== null); const occupied = new Set([...claims, ...changes.map((c) => c.quest)]); - const available = complete - ? ready(snapshot.documents.map((d) => parse(d.path, d.content))).filter((path) => !occupied.has(path)) - : []; + // Unknown until every open PR and quest branch is read since the last + // webhook that could have opened, closed, or claimed a quest. + const available = + scan.occupancy.complete && row.invalidated === 0 + ? ready(snapshot.documents.map((d) => parse(d.path, d.content))).filter((path) => !occupied.has(path)) + : null; const project: Project = { ...snapshot, name: `github/${repo.owner}/${repo.name}`, @@ -234,14 +260,13 @@ export async function readState(db: D1Database, repo: GitHubRepository): Promise issues: activity .filter((item) => item.kind === "issue" && !item.missing && item.data?.state === "OPEN") .map((item) => ({ path: `issues/${item.number}`, title: item.data!.title })), - available: complete ? available : null, + available, }; return { project, activity, claims, - available, - complete, + complete: scan.history.complete, observedAt: row.observedAt, revision: row.revision, }; diff --git a/cloudflare/src/github/webhook.ts b/cloudflare/src/github/webhook.ts index 717653d1..15857fce 100644 --- a/cloudflare/src/github/webhook.ts +++ b/cloudflare/src/github/webhook.ts @@ -35,6 +35,15 @@ const lifecycle = new Map([ ["unsuspend", false], ["new_permissions_accepted", false], ]); +// Only opening, closing, or claiming a quest, or a new quest tree, can change +// availability. Checks, comments, and other pushes wait for the history scan, so +// a busy repository cannot keep restarting a refresh or withholding candidates. +function occupancy(event: string, payload: any) { + if (event === "pull_request") return ["opened", "reopened", "closed"].includes(payload.action); + if (event !== "push" || typeof payload.ref !== "string") return false; + if (payload.ref.startsWith("refs/heads/quest/")) return payload.created === true || payload.deleted === true; + return payload.ref === `refs/heads/${payload.repository.default_branch}`; +} // Far beyond GitHub's redelivery window. Older IDs could be accepted again, // which is harmless because observations never authorize or launch work. export const deliveryRetention = 30 * 24 * 60 * 60 * 1000; @@ -140,11 +149,12 @@ githubWebhooks.post("/webhooks/github", async (c) => { ), ); } - statements.push( - c.env.DB.prepare( - `UPDATE github_project_state SET invalidated=1,revision=revision+1 WHERE repository=? AND ${pending} AND ${enrolled}`, - ).bind(repository, delivery, digest, event, repository, installation), - ); + if (occupancy(event, payload)) + statements.push( + c.env.DB.prepare( + `UPDATE github_project_state SET invalidated=1,revision=revision+1 WHERE repository=? AND ${pending} AND ${enrolled}`, + ).bind(repository, delivery, digest, event, repository, installation), + ); } statements.push( diff --git a/cloudflare/test/github.test.ts b/cloudflare/test/github.test.ts index f3f696dd..7f8b32b5 100644 --- a/cloudflare/test/github.test.ts +++ b/cloudflare/test/github.test.ts @@ -5,37 +5,12 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import app from "../src/index"; import { GitHubService, type GitHubRepository } from "../src/github/service"; import { deliveryRetention } from "../src/github/webhook"; +import { delivery } from "./webhook"; const testEnv = { ...env, GITHUB_APP_PRIVATE_KEY: privateKey }; const repo: GitHubRepository = { owner: "upstream", name: "project", id: 10 }; const fork: GitHubRepository = { owner: "quest-bot-forks", name: "project", id: 20 }; -async function delivery( - event: string, - payload: unknown, - id = crypto.randomUUID(), - changes: Record = {}, -) { - const body = typeof payload === "string" ? payload : JSON.stringify(payload); - const key = await crypto.subtle.importKey( - "raw", - new TextEncoder().encode(env.GITHUB_WEBHOOK_SECRET), - { name: "HMAC", hash: "SHA-256" }, - false, - ["sign"], - ); - const digest = await crypto.subtle.sign("HMAC", key, new TextEncoder().encode(body)); - const signature = "sha256=" + Array.from(new Uint8Array(digest), (b) => b.toString(16).padStart(2, "0")).join(""); - return app.request( - "http://localhost:8787/webhooks/github", - { - method: "POST", - headers: { "X-GitHub-Event": event, "X-GitHub-Delivery": id, "X-Hub-Signature-256": signature, ...changes }, - body, - }, - env, - ); -} function installation(action: string) { return { action, installation: { id: 5, app_id: 123 } }; } diff --git a/cloudflare/test/projects.test.ts b/cloudflare/test/projects.test.ts index 0e7aa99a..2835a546 100644 --- a/cloudflare/test/projects.test.ts +++ b/cloudflare/test/projects.test.ts @@ -7,6 +7,7 @@ import type { ActivityPage } from "../src/github/state"; import { GitHubService } from "../src/github/service"; import { privateKey } from "./github-key"; import { identity, oauth, origin } from "./login-fixtures"; +import { delivery } from "./webhook"; const testEnv = { ...env, GITHUB_APP_PRIVATE_KEY: privateKey }; const repo = { owner: "upstream", name: "project", id: 10 }; @@ -701,12 +702,24 @@ function pull(branch = "quest/first", id = 200) { }; } function activity(): ActivityPage { + const pulls = [pull(), pull("human-branch", 201)]; return { issues: connection([issue()]), - pullRequests: connection([pull(), pull("human-branch", 201)]), + pullRequests: connection(pulls), + open: connection(pulls), refs: connection([{ name: "second", prefix: "refs/heads/quest/" }]), }; } +// A long history whose open PRs and quest branches fit in one page. +function history(): ActivityPage { + return { + issues: connection([issue()], true, "issue-cursor"), + pullRequests: connection([pull("human-branch", 201)]), + open: connection([]), + refs: connection([]), + }; +} +const event = { installation: { id: 5 }, repository: { id: 10, default_branch: "trunk" } }; describe("enrolled project observations", () => { async function enrolled() { @@ -718,9 +731,13 @@ describe("enrolled project observations", () => { const state = await service.projectState(repo, "enrollment-test", true); expect(state).toMatchObject({ complete: true, - available: [], claims: ["quest/second.md"], - project: { commit, changes: [{ quest: "quest/first.md" }], issues: [{ title: issue().title }] }, + project: { + commit, + available: [], + changes: [{ quest: "quest/first.md" }], + issues: [{ title: issue().title }], + }, }); expect(state!.activity).toHaveLength(3); expect(state!.activity.filter((item) => item.kind === "pull")).toHaveLength(2); @@ -761,22 +778,80 @@ describe("enrolled project observations", () => { page.issues = connection([issue()], true, "issue-cursor"); const first = await github({ activity: page }).service.projectState(repo, "enrollment-test", true); expect(first!.complete).toBe(false); - expect(first!.available).toEqual([]); - const second = github({ activity: { issues: connection([issue(101, 3)]) } }); + expect(first!.project.available).toEqual([]); + const second = github({ + activity: { issues: connection([issue(101, 3)]), open: activity().open, refs: activity().refs }, + }); 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)!; expect(query.body.variables.issues).toBe("issue-cursor"); - expect(query.body.query).not.toContain("pullRequests("); + expect(query.body.query).not.toContain("pullRequests(first:50"); const closed = activity(); closed.issues!.nodes[0]!.state = "CLOSED"; closed.pullRequests!.nodes.forEach((p) => (p.state = "CLOSED")); + closed.open = connection([]); closed.refs = connection([]); const last = await github({ activity: closed }).service.projectState(repo, "enrollment-test", true); expect(last!.project.changes).toEqual([]); expect(last!.claims).toEqual([]); - expect(last!.available).toEqual(["quest/first.md"]); + expect(last!.project.available).toEqual(["quest/first.md"]); + }); + it("keeps candidates and history progress through activity that cannot occupy a quest", async () => { + await enrolled(); + const first = await github({ activity: history() }).service.projectState(repo, "enrollment-test", true); + expect(first!.complete).toBe(false); + expect(first!.project.available).toEqual(["quest/first.md"]); + const unrelated = [ + ["check_run", {}], + ["check_suite", {}], + ["status", {}], + ["issues", { action: "opened", issue: { id: 102, number: 5 } }], + ["issue_comment", { action: "created", issue: { id: 100, number: 1 } }], + ["pull_request", { action: "synchronize", pull_request: { id: 201, number: 2 } }], + ["pull_request_review", { action: "submitted", pull_request: { id: 201, number: 2 } }], + ["push", { ref: "refs/heads/human-branch" }], + ["push", { ref: "refs/heads/quest/first", created: false, deleted: false }], + ] as const; + for (const [name, payload] of unrelated) + expect((await delivery(name, { ...event, ...payload })).status).toBe(202); + const cached = await github().service.projectState(repo, "enrollment-test"); + expect(cached!.project.available).toEqual(["quest/first.md"]); + // CI activity during a refresh does not race it either. + const second = github({ + activity: { issues: connection([issue(101, 3)]), open: connection([]), refs: connection([]) }, + 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)!; + expect(query.body.variables.issues).toBe("issue-cursor"); + expect(result!.complete).toBe(true); + expect(result!.project.available).toEqual(["quest/first.md"]); + }); + it.each([ + ["pull_request", { action: "opened", pull_request: { id: 202, number: 4 } }], + ["pull_request", { action: "reopened", pull_request: { id: 202, number: 4 } }], + ["push", { ref: "refs/heads/quest/first", created: true }], + ["push", { ref: "refs/heads/trunk" }], + ] as const)("withholds candidates after %s %j until a refresh rereads occupancy", async (name, payload) => { + await enrolled(); + await github({ activity: history() }).service.projectState(repo, "enrollment-test", true); + expect((await delivery(name, { ...event, ...payload })).status).toBe(202); + const cached = await github().service.projectState(repo, "enrollment-test"); + expect(cached!.project.available).toBeNull(); + expect(readBoard(cached!.project).quests.get("quest/first.md")!.status.kind).not.toBe("available"); + const second = github({ + activity: { + issues: connection([issue(101, 3)]), + open: connection([pull("quest/first", 202)]), + refs: connection([]), + }, + }); + 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(result!.project.available).toEqual([]); + expect(result!.project.changes).toMatchObject([{ quest: "quest/first.md" }]); }); it("never publishes partial GraphQL results or check state from a different PR head", async () => { await enrolled(); @@ -844,7 +919,12 @@ describe("enrolled project observations", () => { await enrolled(); await github().service.projectState(repo, "enrollment-test", true); const next = await github({ - activity: { issues: connection([]), pullRequests: connection([]), refs: connection([]) }, + activity: { + issues: connection([]), + pullRequests: connection([]), + open: connection([]), + refs: connection([]), + }, }).service.projectState(repo, "enrollment-test", true); expect(next!.activity).toHaveLength(3); expect(next!.activity.every((item) => item.missing === 1)).toBe(true); @@ -854,9 +934,17 @@ describe("enrolled project observations", () => { it("retains observations while paused, clears candidates, and cascades caches on disconnect", async () => { await enrolled(); - await github().service.projectState(repo, "enrollment-test", true); + const quiet = { + issues: connection([]), + pullRequests: connection([]), + open: connection([]), + refs: connection([]), + }; + await github({ activity: quiet }).service.projectState(repo, "enrollment-test", true); await github().service.setContributions(repo, "enrollment-test", "paused"); - expect((await github().service.projectState(repo, "enrollment-test"))!.available).toEqual([]); + const paused = await github().service.projectState(repo, "enrollment-test"); + expect(paused!.project.available).toEqual([]); + expect(readBoard(paused!.project).counts.available).toBe(0); await github().service.setContributions(repo, "enrollment-test", "disconnected"); expect((await env.DB.prepare("SELECT * FROM github_project_activity").all()).results).toEqual([]); expect(await env.DB.prepare("SELECT * FROM github_project_state").first()).toBeNull(); @@ -920,7 +1008,7 @@ describe("project activity routes", () => { }); it("keeps tree-ready quests pending until a claiming PR on an unvisited page is observed", async () => { const page = activity(); - page.pullRequests = connection([pull("human-branch", 201)], true, "pull-cursor"); + page.open = connection([pull("human-branch", 201)], true, "open-cursor"); page.refs = connection([]); const cookie = await session({ activity: page }); await app.request(`${base}/refresh`, { method: "POST", headers: { Cookie: cookie, Origin: origin } }, testEnv); @@ -933,7 +1021,10 @@ describe("project activity routes", () => { expect(quest).toContain("Availability pending"); expect(quest).not.toContain("/quest-start"); - const rest = github({ account: githubId, activity: { pullRequests: connection([pull()]) } }); + const rest = github({ + account: githubId, + activity: { issues: connection([]), pullRequests: connection([pull()]), open: connection([pull()]) }, + }); vi.spyOn(globalThis, "fetch").mockImplementation(rest.request as typeof fetch); await app.request(`${base}/refresh`, { method: "POST", headers: { Cookie: cookie, Origin: origin } }, testEnv); const board = await (await app.request(`${base}/board`, { headers: { Cookie: cookie } }, testEnv)).text(); diff --git a/cloudflare/test/webhook.ts b/cloudflare/test/webhook.ts new file mode 100644 index 00000000..2dd3deca --- /dev/null +++ b/cloudflare/test/webhook.ts @@ -0,0 +1,30 @@ +import { env } from "cloudflare:workers"; +import app from "../src/index"; + +/** A GitHub webhook delivery signed with the test secret. */ +export async function delivery( + event: string, + payload: unknown, + id = crypto.randomUUID(), + changes: Record = {}, +) { + const body = typeof payload === "string" ? payload : JSON.stringify(payload); + const key = await crypto.subtle.importKey( + "raw", + new TextEncoder().encode(env.GITHUB_WEBHOOK_SECRET), + { name: "HMAC", hash: "SHA-256" }, + false, + ["sign"], + ); + const digest = await crypto.subtle.sign("HMAC", key, new TextEncoder().encode(body)); + const signature = "sha256=" + Array.from(new Uint8Array(digest), (b) => b.toString(16).padStart(2, "0")).join(""); + return app.request( + "http://localhost:8787/webhooks/github", + { + method: "POST", + headers: { "X-GitHub-Event": event, "X-GitHub-Delivery": id, "X-Hub-Signature-256": signature, ...changes }, + body, + }, + env, + ); +} From f58a5bc31c13daa28d1c9eccf87c3bcee661c3ed Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 10:48:35 -0700 Subject: [PATCH 2/2] fix(jobs): reread an issue or pull request before binding a job to it Webhooks that close an issue or PR no longer restart the scan, so its activity row can still say open until history rereads it. jobSource now reads the target from GitHub and refuses one that closed or whose head moved since it was observed. Co-Authored-By: Claude Opus 5.5 --- cloudflare/README.md | 4 +++- cloudflare/src/github/service.ts | 19 ++++++++++++++++--- cloudflare/test/jobs.test.ts | 16 ++++++++++++++++ cloudflare/test/project-fixtures.ts | 8 ++++++-- 4 files changed, 41 insertions(+), 6 deletions(-) diff --git a/cloudflare/README.md b/cloudflare/README.md index 0050a2a4..5fb87f75 100644 --- a/cloudflare/README.md +++ b/cloudflare/README.md @@ -76,7 +76,9 @@ A job is one maintainer-authorized workflow (`plan`, `start`, `iterate`, or `bootstrap`) on one target, pinned to a revision (`src/jobs/`). Maintainers create, answer, and cancel jobs at `/projects/github///jobs`, and every request rechecks current GitHub authority. `start` takes only quests that -observations show ready and unclaimed. `iterate` jobs are untrusted and route +observations show ready and unclaimed. A job on an issue or pull request +rereads it from GitHub, refusing one that closed or whose head moved since +observations read it. `iterate` jobs are untrusted and route only to Linux runners that declare VM builds. Without a `flake.nix`, the route offers a `bootstrap` job instead. A paused project gets no new jobs and none are claimed; resuming it dispatches the jobs that waited. Disconnecting a project diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 38f17125..0cb2e241 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -538,15 +538,17 @@ export class GitHubService { /** * What a new job may start from: `resolve` picks it from current - * observations, then this checks whether its revision has a flake for the + * observations, then this checks that its issue or pull request is still + * open at the observed head, and whether its revision has a flake for the * runner's devShell. */ - jobSource( + jobSource( repo: GitHubRepository, userId: string, resolve: (state: ProjectState) => T, ) { - return this.#authorized(repo, userId, { contents: "read" }, async (token, current, installation) => { + const permissions = { contents: "read", issues: "read", pull_requests: "read" } as const; + return this.#authorized(repo, userId, permissions, async (token, current, installation) => { this.#public(current); const enrolled = await this.#state(repo, installation); if (enrolled === null) throw new GitHubError("Project is not enrolled for this installation"); @@ -556,6 +558,17 @@ export class GitHubService { throw new GitHubError("Refresh project activity before starting work"); // Check the revision the job will run, such as a pull request's head. const job = resolve(state); + // Only history rereads an issue or PR, so its activity row can still say + // open after it closed; read the target itself before binding a job to it. + const [, kind, target] = job.target.match(/^(issues|pulls)\/(\d+)$/) ?? []; + if (kind) { + const live = await this.#api<{ state: string; head?: { sha: string } }>( + token, + `${repositoryPath(repo)}/${kind}/${number(Number(target))}`, + ); + if (live.state !== "open" || (kind === "pulls" && live.head?.sha !== job.revision)) + throw new GitHubError("This changed on GitHub since it was read; refresh and review it again"); + } let flake = true; try { await this.#api(token, `${repositoryPath(repo)}/contents/flake.nix?ref=${sha(job.revision)}`); diff --git a/cloudflare/test/jobs.test.ts b/cloudflare/test/jobs.test.ts index 298ec86a..e1c406cf 100644 --- a/cloudflare/test/jobs.test.ts +++ b/cloudflare/test/jobs.test.ts @@ -908,6 +908,22 @@ describe("job routes", () => { expect(await stale.json()).toMatchObject({ error: "Refresh project activity before starting work" }); }); + it("refuses an issue or pull request that closed or moved since it was read", async () => { + // Closing or commenting leaves availability known, and history has not reread the target. + const changed = "This changed on GitHub since it was read; refresh and review it again"; + github.issue = "closed"; + const closed = await post("", { workflow: "plan", target: "issues/1" }); + expect(closed.status).toBe(403); + expect(await closed.json()).toEqual({ error: changed }); + github.pull = "closed"; + expect(await (await post("", { workflow: "iterate", target: "pulls/2" })).json()).toEqual({ error: changed }); + github.pull = "open"; + github.head = "e".repeat(40); + expect(await (await post("", { workflow: "iterate", target: "pulls/2" })).json()).toEqual({ error: changed }); + // Work that targets no issue or pull request still starts. + expect((await post("", { workflow: "start", target: "quest/first.md" })).status).toBe(201); + }); + it("offers a devShell bootstrap job instead of starting without a flake", async () => { github.flake = false; const refused = await post("", { workflow: "start", target: "quest/first.md" }); diff --git a/cloudflare/test/project-fixtures.ts b/cloudflare/test/project-fixtures.ts index f4031667..8c61ae42 100644 --- a/cloudflare/test/project-fixtures.ts +++ b/cloudflare/test/project-fixtures.ts @@ -55,7 +55,7 @@ export const issue = { * `mock` reinstalls it after another sign-in replaces it. */ export async function maintainerOf() { - const github = { flake: true, pullFlake: true, role: "maintain" }; + const github = { flake: true, pullFlake: true, role: "maintain", issue: "open", pull: "open", head }; const who = identity("github"); const cookie = (await oauth(who)).session!; const user = (await db @@ -88,7 +88,7 @@ export async function maintainerOf() { id: 5, app_id: 123, suspended_at: null, - permissions: { metadata: "read", contents: "read" }, + permissions: { metadata: "read", contents: "read", issues: "read", pull_requests: "read" }, }); if (path.endsWith("/access_tokens")) return Response.json({ token: "installation-token" }); if (path === "/installation/token") return new Response(null, { status: 204 }); @@ -115,6 +115,10 @@ export async function maintainerOf() { ? { user: { id: Number(who.id) }, permission: "write", role_name: github.role } : { user: { id: Number(permission[1]) }, permission: "read", role_name: "read" }, ); + // The live issue and pull request that a job binds, which observations can lag. + if (path === "/repos/upstream/project/issues/1") return Response.json({ state: github.issue }); + if (path === "/repos/upstream/project/pulls/2") + return Response.json({ state: github.pull, head: { sha: github.head } }); const ref = url.searchParams.get("ref"); if (path === "/repos/upstream/project/contents/flake.nix" && (ref === commit || ref === head)) return (ref === commit ? github.flake : github.pullFlake)