diff --git a/cloudflare/GITHUB.md b/cloudflare/GITHUB.md index c3c93af1..efda8d12 100644 --- a/cloudflare/GITHUB.md +++ b/cloudflare/GITHUB.md @@ -431,6 +431,10 @@ branch claims exclude that quest from candidate data; branch claims display as Claimed on the board. So does a draft PR its starter still holds, one without a `## Decisions needed` section, which the inbox lists as in progress rather than as a decision; a ready PR or a handed-back draft is Ready to turn in. +As in the CLI, a `quest/` branch whose tip is the head of a merged PR from it +claims nothing, since its work landed; one whose PR closed unmerged, or with +commits pushed after the merge, still does. A push to an existing branch does +not invalidate candidates, so such a push shows on the next occupancy pass. Runner/run associations belong to job coordination. Every cached read and refresh proves current maintainer authority, live repository diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index 674f285a..2b2f30a8 100644 --- a/cloudflare/dev/github.ts +++ b/cloudflare/dev/github.ts @@ -33,6 +33,10 @@ export type Pull = Issue & { contributor?: string; /** A `## Decisions needed` section in it hands a draft back to the maintainer. */ body?: string; + /** Closed by merging it; set with `closed`. */ + merged?: boolean; + /** The commit it ended at, when its branch has moved on since it closed; defaults to the branch's tip. */ + head?: string; /** GitHub's mergeStateStatus; only CLEAN merges. Defaults to BLOCKED, or DRAFT for a draft. */ merge?: "CLEAN" | "BLOCKED" | "DIRTY" | "UNSTABLE" | "UNKNOWN"; /** When it was opened, in milliseconds; defaults to the world's fixed update time. */ @@ -54,7 +58,7 @@ export type Repo = { /** * Upstream branches and their commits. A pull request's branch that is not * listed is at a commit derived from its number; a `quest/*` branch with no - * pull request is a claim. + * open pull request is a claim, unless it is still at a merged one's head. */ branches: Record; /** Branches on the repository's service fork, `/`, and their commits. */ @@ -89,8 +93,9 @@ export const blob = (content: string | Uint8Array) => { /** The default branch's commit. */ export const commitOf = (repo: Repo) => sha1(`commit ${repo.owner}/${repo.name}`); const treeOf = (repo: Repo) => sha1(`tree ${repo.owner}/${repo.name}`); -/** A pull request's head commit: its branch's tip, where the world lists one. */ +/** A pull request's head commit: the one it ended at, or its branch's tip where the world lists one. */ export const headOf = (repo: Repo, pull: Pull) => + pull.head ?? (pull.fork ? repo.fork : pull.contributor ? {} : repo.branches)[pull.branch] ?? sha1(`head ${repo.owner}/${repo.name}#${pull.number}`); // GitHub's database IDs differ from numbers; derive stable ones per repository. @@ -105,7 +110,7 @@ function entity(repo: Repo, item: Issue) { databaseId: databaseId(repo, item.number), number: item.number, title: item.title, - state: item.closed ? "CLOSED" : "OPEN", + state: (item as Pull).merged ? "MERGED" : item.closed ? "CLOSED" : "OPEN", updatedAt, closedAt: item.closed ? updatedAt : null, author: { login: item.author }, @@ -223,10 +228,28 @@ function graphql(repo: Repo, query: string, variables: Record) if (/(? pullNode(repo, pull))); if (query.includes("open:pullRequests(")) result.open = page(open.map((pull) => pullNode(repo, pull))); if (query.includes("refs(")) { - const branches = [...Object.keys(repo.branches), ...upstreamPulls(repo).map((p) => p.branch)]; + // Quest branches at their tips, with the pull requests from them that merged, newest first. + const tips = new Map(); + for (const pull of upstreamPulls(repo)) tips.set(pull.branch, headOf(repo, pull)); + for (const [branch, oid] of Object.entries(repo.branches)) tips.set(branch, oid); + const merged = (branch: string) => + repo.pulls + .filter((pull) => pull.merged && !pull.fork && !pull.contributor && pull.branch === branch) + .reverse(); result.refs = page( - [...new Set(branches.filter((b) => b.startsWith("quest/")).map((b) => b.slice("quest/".length)))].map( - (name) => ({ name, prefix: "refs/heads/quest/" }), + [...tips].flatMap(([branch, oid]) => + branch.startsWith("quest/") + ? [ + { + name: branch.slice("quest/".length), + prefix: "refs/heads/quest/", + target: { oid }, + associatedPullRequests: page( + merged(branch).map((pull) => ({ headRefOid: headOf(repo, pull) })), + ), + }, + ] + : [], ), ); } @@ -526,6 +549,7 @@ export function fakeGitHub(world: World) { if (body.sha !== headOf(repo, pull)) return json({ message: "Head branch was modified" }, 409); if (pull.merge !== "CLEAN") return json({ message: "Pull Request is not mergeable" }, 405); pull.closed = true; + pull.merged = true; return json({ sha: commitOf(repo), merged: true, message: "Pull Request successfully merged" }); } if (tail.startsWith("/contents/")) { diff --git a/cloudflare/src/github/state.ts b/cloudflare/src/github/state.ts index 581d576d..0d3cc040 100644 --- a/cloudflare/src/github/state.ts +++ b/cloudflare/src/github/state.ts @@ -1,4 +1,4 @@ -import { branchQuest, handedBack, parse, ready } from "quest/core"; +import { branchQuest, handedBack, landed, parse, ready } from "quest/core"; import type { Project, Snapshot } from "../board/project"; import { GitHubError, type GitHubRepository, ProjectChanged } from "./service"; import { positiveId } from "./webhook"; @@ -38,11 +38,18 @@ export type Pull = Entity & { }; commits: { nodes: { commit: { oid: string; statusCheckRollup: { state: string } | null } }[] }; }; +/** A quest branch, its tip, and the merged PRs from it, newest first. */ +type QuestRef = { + name: string; + prefix: string; + target: { oid: string } | null; + associatedPullRequests: { nodes: { headRefOid: string }[] }; +}; export type ActivityPage = { issues?: Connection; pullRequests?: Connection; open?: Connection; - refs?: Connection<{ name: string; prefix: string }>; + refs?: Connection; /** Stale entities read by number, in the order requested; null when GitHub no longer has one. */ [stale: `s${number}`]: Entity | Pull | null | undefined; }; @@ -107,7 +114,7 @@ export function activityQuery({ history, occupancy }: Scan, stale: Stale[] = []) 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 } }', + 'refs(refPrefix:"refs/heads/quest/",first:100,after:$refs) { nodes { name prefix target { oid } associatedPullRequests(states:MERGED,first:10,orderBy:{field:CREATED_AT,direction:DESC}) { nodes { headRefOid } } } pageInfo { hasNextPage endCursor } }', ); } // Numbers come from validated webhook pointers and are interpolated as integers. @@ -218,8 +225,18 @@ export async function reconcile( }; if (occupancy.refs !== false) for (const ref of page.refs!.nodes) { - if (typeof ref.name !== "string" || ref.prefix !== "refs/heads/quest/") + const tip = ref.target?.oid; + const merged = ref.associatedPullRequests?.nodes; + if ( + typeof ref.name !== "string" || + ref.prefix !== "refs/heads/quest/" || + typeof tip !== "string" || + !Array.isArray(merged) + ) throw new GitHubError("Invalid branch observation"); + // A branch left at a merged PR's head holds nothing: its work landed. + const pulls = merged.map((pull) => ({ head: pull.headRefOid, merged: true })); + if (landed(tip, pulls) !== undefined) continue; // Ref.name is relative to the requested prefix. updated.occupancy.claims.push(`quest/${ref.name}`); } diff --git a/cloudflare/test/projects.test.ts b/cloudflare/test/projects.test.ts index aa22ea63..8e86e8f3 100644 --- a/cloudflare/test/projects.test.ts +++ b/cloudflare/test/projects.test.ts @@ -692,6 +692,15 @@ describe("project setup routes", () => { function connection(nodes: T[], more = false, cursor: string | null = null) { return { nodes, pageInfo: { hasNextPage: more, endCursor: cursor } }; } +/** A quest branch that no merged PR left behind. */ +function ref(name: string) { + return { + name, + prefix: "refs/heads/quest/", + target: { oid: "b".repeat(40) }, + associatedPullRequests: { nodes: [] }, + }; +} function issue(id = 100, number = 1) { return { databaseId: String(id), @@ -724,7 +733,7 @@ function activity(): ActivityPage { issues: connection([issue()]), pullRequests: connection(pulls), open: connection(pulls), - refs: connection([{ name: "second", prefix: "refs/heads/quest/" }]), + refs: connection([ref("second")]), }; } // A long history whose open PRs and quest branches fit in one page. @@ -1286,7 +1295,7 @@ describe("webhook reconciliation", () => { refs: connection([]), }; await github({ activity: quiet }).service.projectState(repo, "enrollment-test", true); - mocked({ activity: { ...quiet, refs: connection([{ name: "first", prefix: "refs/heads/quest/" }]) } }); + mocked({ activity: { ...quiet, refs: connection([ref("first")]) } }); await delivery("push", { ...event, ref: "refs/heads/quest/first", created: true }, undefined, {}, testEnv); const state = await github().service.projectState(repo, "enrollment-test"); expect(state!.claims).toEqual(["quest/first.md"]); diff --git a/cloudflare/test/seed.test.ts b/cloudflare/test/seed.test.ts index 57e45868..8c4ad9f8 100644 --- a/cloudflare/test/seed.test.ts +++ b/cloudflare/test/seed.test.ts @@ -15,8 +15,11 @@ const testEnv = { ...env, GITHUB_APP_PRIVATE_KEY: privateKey }; const db = env.DB; afterEach(() => vi.restoreAllMocks()); -/** Seed a fresh world against the fake GitHub, returning it, a request helper, and the session cookie. */ -async function seeded() { +/** + * Seed a fresh world, after `edit` changes it, against the fake GitHub, + * returning it, a request helper, and the session cookie. + */ +async function seeded(edit: (world: World) => void = () => {}) { await db.batch([ db.prepare("DELETE FROM jobs"), db.prepare("DELETE FROM runners"), @@ -24,6 +27,7 @@ async function seeded() { db.prepare("DELETE FROM github_installations"), ]); const data = { ...world(), forks: env.GITHUB_FORK_OWNER }; + edit(data); vi.spyOn(globalThis, "fetch").mockImplementation(fakeGitHub(data)); // Webhooks read and notify in the background, so each request finishes its `waitUntil` work. const request = async (path: string, init?: RequestInit) => { @@ -144,6 +148,38 @@ it("seeds a representative inbox through the Worker's own routes", async () => { expect(data.repos[0]!.pulls.find((p) => p.number === 12)!.closed).toBe(true); }); +it("ignores a quest branch left at the head of its merged pull request", async () => { + const { request, cookie } = await seeded((data) => { + const rocket = data.repos[0]!; + const closed = { author: "contributor", checks: "SUCCESS", review: "APPROVED", closed: true } as const; + rocket.pulls.push( + // Its work landed, so the branch holds nothing. + { ...closed, number: 21, title: "Passkeys", branch: "quest/m0/sign-in", merged: true }, + // Closed unmerged: that work may be wanted back. + { ...closed, number: 22, title: "Offline", branch: "quest/m1/offline" }, + // Pushed to after its merge: new work. + { ...closed, number: 23, title: "Export", branch: "quest/m0/export", merged: true, head: "e".repeat(40) }, + ); + // Each branch outlived its pull request. + Object.assign(rocket.branches, { + "quest/m0/sign-in": "a".repeat(40), + "quest/m1/offline": "b".repeat(40), + "quest/m0/export": "c".repeat(40), + }); + }); + const board = await ( + await request("/projects/github/example/rocket/board", { headers: { Cookie: cookie } }) + ).text(); + const row = (title: string) => board.match(new RegExp(`${title}[^<]*[\\s\\S]*?`))?.[0] ?? ""; + expect(row("Add passkey sign-in")).toContain("Available"); + expect(row("Keep working offline")).toContain("Claimed"); + expect(row("Export a project as a ZIP archive")).toContain("Claimed"); + // The inbox agrees: it offers the landed quest, and not the one new work claims. + const inbox = await (await request("/inbox", { headers: { Cookie: cookie } })).text(); + expect(inbox).toContain("Add passkey sign-in"); + expect(inbox).not.toContain("Export a project as a ZIP archive"); +}); + it("gets failed and blocked jobs moving again from the inbox", async () => { const { rocket, page, post } = await seeded(); diff --git a/quest/m0/kix/README.md b/quest/m0/kix/README.md index 7bdbf44b..33c15d08 100644 --- a/quest/m0/kix/README.md +++ b/quest/m0/kix/README.md @@ -124,4 +124,3 @@ fixes what it finds: - [Merge jobs](/quest/m0/kix/merge-job.md) - a merge pick fixes conflicts, CI, and reviews on a runner, then lands the PR - [Web execution](/quest/m0/kix/execution.md) - spawn and iterate through the UI, ending in contribution PRs - [Onboarding guide](/quest/m0/kix/guide.md) - a complete maintainer path, from runner setup through a reviewed PR -- [Merged claims](/quest/m0/kix/merged-claims.md) - the board ignores branches whose pull request merged, as the CLI does diff --git a/quest/m0/kix/merged-claims.md b/quest/m0/kix/merged-claims.md deleted file mode 100644 index 2ec1668c..00000000 --- a/quest/m0/kix/merged-claims.md +++ /dev/null @@ -1,27 +0,0 @@ -# [XS] Ignore merged branches on the board - -## Goal - -The kix board and inbox agree with `quest unblocked --unclaimed` about which -quests are claimed: a `quest/*` branch whose tip is the head of a merged pull -request no longer counts as a claim, while a branch whose pull request closed -unmerged still does. - -## Plan - -Decided 2026-10-22 (user) as a follow-up of -[kixelated/quest#192](https://github.com/kixelated/quest/pull/192), which -gave the CLI this rule: - -- `cloudflare/src/github/state.ts` maps every observed `quest/*` ref to a - claim (`associatedQuest`). Apply the same merged-head rule when reading - those refs, and keep the CLI and Worker on one definition of it rather - than a second copy. -- Ranked last in the epic: moq-dev/moq, moq-dev/moq.pro, and kixelated/quest - all delete head branches on merge, so only projects without that setting - see stale claims today. -- Test a merged-head branch, a closed-unmerged branch, and a merged branch - with commits pushed after the merge, with the seeded fake GitHub - (`cloudflare/dev/github.ts`). -- Dropped (user): a CLI sweep that reports or deletes merged quest branches, - since every current repository deletes them on merge. diff --git a/src/cli/claims.ts b/src/cli/claims.ts index 8f704152..57721b06 100644 --- a/src/cli/claims.ts +++ b/src/cli/claims.ts @@ -4,7 +4,7 @@ // records a claim the same way, judging readiness by the upstream tree. import { execFileSync } from "node:child_process"; -import { handedBack } from "../core"; +import { handedBack, landed } from "../core"; import { type GitHubDependencies, type ObjectValue, @@ -257,8 +257,15 @@ export async function mergedPull( ): Promise { const query = new URLSearchParams({ state: "closed", head: `${head.owner}:${branch}`, per_page: "100" }); const rows = await github.all(`${repositoryPath(repository)}/pulls?${query}`); - const pull = rows.find((row) => typeof row.merged_at === "string" && object(row.head).sha === tip); - return pull === undefined ? null : string(pull.html_url); + const pull = landed( + tip, + rows.map((row) => ({ + head: string(object(row.head).sha), + merged: typeof row.merged_at === "string", + url: string(row.html_url), + })), + ); + return pull?.url ?? null; } /** Where a claim lands: the remote, its upstream, and the upstream default branch's tip and quest tree. */ diff --git a/src/core/claim.ts b/src/core/claim.ts index 9b441da4..6bf8bc4f 100644 --- a/src/core/claim.ts +++ b/src/core/claim.ts @@ -1,6 +1,7 @@ -// How a quest's PR says whose move it is. A draft is its starter's claim until -// its body hands it back with a `## Decisions needed` section (the guide's -// Execution section); the CLI and the Worker read that the same way. +// How a quest's PR and branch say whose move it is. A draft is its starter's +// claim until its body hands it back with a `## Decisions needed` section, and +// a branch whose work merged holds nothing (the guide's Execution section); +// the CLI and the Worker read both the same way. // Web edits end lines in CRLF. const HAND_BACK = /^#{2}[ \t]+Decisions needed[ \t]*#*[ \t]*\r?$/im; @@ -9,3 +10,20 @@ const HAND_BACK = /^#{2}[ \t]+Decisions needed[ \t]*#*[ \t]*\r?$/im; export function handedBack(body: string): boolean { return HAND_BACK.test(body); } + +/** A closed PR from a quest's branch: the commit it ended at, and whether it merged. */ +export interface ClosedPull { + head: string; + merged: boolean; +} + +/** + * The PR that landed a quest branch at `tip`: one from it that merged with + * `tip` as its head, so the branch holds no claim. One whose PR closed unmerged + * still holds its quest, since that work may be wanted back, and so does one + * with commits pushed after its merge. The CLI and the Worker share this rule. + */ +export function landed

(tip: string, pulls: Iterable

): P | undefined { + for (const pull of pulls) if (pull.merged && pull.head === tip) return pull; + return undefined; +} diff --git a/src/core/index.ts b/src/core/index.ts index 5eae59a9..6c7da526 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -25,7 +25,7 @@ export { resolve, rooted, } from "./doc"; -export { handedBack } from "./claim"; +export { type ClosedPull, handedBack, landed } from "./claim"; export { type Missed, type Moved, type Source, move, relocate, remove } from "./edit"; export { comparePaths, normalize } from "./path"; export { type Blocker, blockers, label, lookup, ready, renderBlocker } from "./ready";