From d4266d75d8d9a2fe5d48f87b84db9ef8918d81b9 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 27 Oct 2026 21:58:41 -0700 Subject: [PATCH 1/2] chore(quest): claim quest/m0/helpers/merge-state-race.md Quest-Claim: 999272d9d6bb3a74 From 42ab05c57dcf2f67f0e30119c59ac1d63ea540ea Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 27 Oct 2026 22:04:17 -0700 Subject: [PATCH 2/2] fix: handle GitHub's clean auto-merge refusal --- cloudflare/src/github/service.ts | 76 +++++++++++++++++++--------- cloudflare/test/publish.test.ts | 69 +++++++++++++++++++++++++ quest/m0/helpers/README.md | 1 - quest/m0/helpers/merge-state-race.md | 41 --------------- src/cli/hold.ts | 37 ++++++++++---- src/github/merge.ts | 12 +++++ test/hold.test.ts | 71 ++++++++++++++++++++++++++ 7 files changed, 230 insertions(+), 77 deletions(-) delete mode 100644 quest/m0/helpers/merge-state-race.md create mode 100644 src/github/merge.ts diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 94fbb16c..ed3aa3fe 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -1,5 +1,6 @@ import { createHash, createPrivateKey, sign } from "node:crypto"; import { check, isQuest, parse, ready } from "quest/core"; +import { cleanAutoMergeRefusal, immediateMerge } from "../../../src/github/merge"; import { osiApproved } from "../licenses"; import { positiveId } from "./webhook"; import { @@ -1477,33 +1478,60 @@ export class GitHubService { // Landing is a contribution like opening a pull request, so a paused project lands nothing. if ((await this.#state(repo, installation)) !== "enabled") throw new GitHubError("Contributions are paused"); + const check = (holds: Holds) => { + if (holds.state !== "OPEN") throw new PublicationBlocked("The pull request is no longer open."); + if ( + !input.hold || + !holds.labels.includes(MERGE_LABEL) || + added(holds, MERGE_LABEL)?.id !== input.hold + ) + throw new PublicationBlocked( + "This job's quest:merge label was removed or replaced, so it no longer holds the pull request.", + ); + if (holds.head !== sha(input.head)) + throw new PublicationBlocked("The pull request's head moved outside this job."); + }; const holds = await this.#holds(token, repo, pull); - if (holds.state !== "OPEN") throw new PublicationBlocked("The pull request is no longer open."); - if (!input.hold || !holds.labels.includes(MERGE_LABEL) || added(holds, MERGE_LABEL)?.id !== input.hold) - throw new PublicationBlocked( - "This job's quest:merge label was removed or replaced, so it no longer holds the pull request.", - ); - if (holds.head !== sha(input.head)) - throw new PublicationBlocked("The pull request's head moved outside this job."); + check(holds); const gated = await this.#gated(token, repo, holds.base); - const mutation = - !gated && !holds.queue && IMMEDIATE.includes(holds.mergeState) + let mutation: "mergePullRequest" | "enablePullRequestAutoMerge" = + !gated && immediateMerge(holds.mergeState, holds.queue) ? "mergePullRequest" : "enablePullRequestAutoMerge"; - const result = await this.#api<{ errors?: { message?: unknown }[] }>(token, "/graphql", "POST", { - query: `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!, $body: String) { ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha, mergeMethod: $method, commitBody: $body }) { clientMutationId } }`, - variables: { - id: holds.id, - sha: input.head, - method: gated ? "SQUASH" : (input.method ?? mergeMethod(current)).toUpperCase(), - body: gated ? `${LAND_PIN}: ${input.head}` : null, - }, - }); - if (result.errors?.length) { - const reason = result.errors[0]?.message; - throw new PublicationBlocked( - `GitHub refused to land the pull request${typeof reason === "string" && line(reason) ? `: ${line(reason)}` : "."}`, - ); + const submit = async () => { + const result = await this.#api<{ errors?: { message?: unknown }[] }>(token, "/graphql", "POST", { + query: `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!, $body: String) { ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha, mergeMethod: $method, commitBody: $body }) { clientMutationId } }`, + variables: { + id: holds.id, + sha: input.head, + method: gated ? "SQUASH" : (input.method ?? mergeMethod(current)).toUpperCase(), + body: gated ? `${LAND_PIN}: ${input.head}` : null, + }, + }); + if (result.errors?.length) { + const reason = result.errors[0]?.message; + throw new PublicationBlocked( + `GitHub refused to land the pull request${typeof reason === "string" && line(reason) ? `: ${line(reason)}` : "."}`, + ); + } + }; + try { + await submit(); + } catch (error) { + if ( + mutation !== "enablePullRequestAutoMerge" || + gated || + holds.queue || + !cleanAutoMergeRefusal(error instanceof Error ? error.message : null) + ) + throw error; + // The refusal can race GitHub computing UNKNOWN. Reread once, keeping the original head pinned. + const fresh = await this.#holds(token, repo, pull); + check(fresh); + if (!immediateMerge(fresh.mergeState, fresh.queue) || (await this.#gated(token, repo, fresh.base))) + throw error; + mutation = "mergePullRequest"; + await submit(); } await this.#invalidate(repo); return { merged: mutation === "mergePullRequest" }; @@ -1525,8 +1553,6 @@ function mergeMethod(current: Repository): MergeMethod { /** The label prefix and merge label `quest iterate`, `ready`, and `merge` hold pull requests with. */ const HOLD_PREFIX = "quest:"; export const MERGE_LABEL = "quest:merge"; -/** Merge states GitHub merges at once, where it refuses to enable auto-merge. */ -const IMMEDIATE = ["CLEAN", "HAS_HOOKS", "UNSTABLE"]; /** The check a base gated by `quest land` requires, and the commit body trailer that queues a head for it; see `quest land --help`. */ const LAND_CHECK = "quest land"; const LAND_PIN = "Quest-Land-Head"; diff --git a/cloudflare/test/publish.test.ts b/cloudflare/test/publish.test.ts index 2ef282a4..b5792505 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -1178,6 +1178,75 @@ describe("merge jobs", () => { expect(pull()).toMatchObject({ merged: true, closed: true }); }); + it("lands an UNKNOWN head after GitHub refuses auto-merge as clean, with one reread", async () => { + const id = await merging(); + pull().merge = "UNKNOWN"; + vi.mocked(globalThis.fetch).mockImplementation(async (input, init) => { + if (String(init?.body ?? "").includes("enablePullRequestAutoMerge(")) { + pull().merge = "CLEAN"; + return Response.json({ errors: [{ message: "Pull request is in clean status" }] }); + } + return github(input, init); + }); + const reads = () => + requests().filter((r) => r.path === "/graphql" && r.body.query.includes("timelineItems(")).length; + const before = reads(); + const response = await publish({ job: id, attempt: 1, head: prHead, land: { method: "squash" } }); + expect(await response.json()).toEqual({ head: prHead, landed: { merged: true } }); + expect(lands().map((l) => [l.mutation, l.variables.sha, l.variables.method])).toEqual([ + ["enablePullRequestAutoMerge", prHead, "SQUASH"], + ["mergePullRequest", prHead, "SQUASH"], + ]); + expect(reads() - before).toBe(2); + }); + + it("leaves an accepted UNKNOWN head on auto-merge", async () => { + const id = await merging(); + pull().merge = "UNKNOWN"; + const response = await publish({ job: id, attempt: 1, head: prHead, land: { method: "squash" } }); + expect(await response.json()).toEqual({ head: prHead, landed: { merged: false } }); + expect(lands().map((l) => l.mutation)).toEqual(["enablePullRequestAutoMerge"]); + }); + + it.each(["Pull request is in clean status", "Auto merge is not allowed for this repository"])( + "preserves an UNKNOWN auto-merge refusal: %s", + async (reason) => { + const id = await merging(); + pull().merge = "UNKNOWN"; + refuseLanding(reason); + const reads = () => + requests().filter((r) => r.path === "/graphql" && r.body.query.includes("timelineItems(")).length; + const before = reads(); + const response = await publish({ job: id, attempt: 1, head: prHead, land: {} }); + expect(response.status).toBe(202); + expect((await job(id)).questions.at(-1)!.prompt).toEqual({ + type: "publication", + blocked: `GitHub refused to land the pull request: ${reason}`, + }); + expect(lands().map((l) => l.mutation)).toEqual(["enablePullRequestAutoMerge"]); + expect(reads() - before).toBe(reason.includes("clean status") ? 2 : 1); + }, + ); + + it.each(["head", "hold", "gate"])("does not merge if its %s changes after a clean refusal", async (change) => { + const id = await merging(); + pull().merge = "UNKNOWN"; + vi.mocked(globalThis.fetch).mockImplementation(async (input, init) => { + if (String(init?.body ?? "").includes("enablePullRequestAutoMerge(")) { + pull().merge = "CLEAN"; + if (change === "head") project.branches[pull().branch] = "c".repeat(40); + if (change === "hold") pull().labels = []; + if (change === "gate") project.required = ["quest land"]; + return Response.json({ errors: [{ message: "Pull request is in clean status" }] }); + } + return github(input, init); + }); + const response = await publish({ job: id, attempt: 1, head: prHead, land: {} }); + expect(response.status).toBe(202); + expect(lands().map((l) => l.mutation)).toEqual(["enablePullRequestAutoMerge"]); + expect(pull().merged).not.toBe(true); + }); + it("refuses only the workflow changes its merge makes to the pull request's head", async () => { // The pull request and its base branch both changed a workflow. const workflow = ".github/workflows/ci.yml"; diff --git a/quest/m0/helpers/README.md b/quest/m0/helpers/README.md index 55b7af5e..7af96768 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -66,7 +66,6 @@ commands from Sep-Oct. Session counts in each child come from that window. ## Required -- [Merge state race](/quest/m0/helpers/merge-state-race.md) - `--land` and merge jobs land a PR whose merge state GitHub is still computing, on the first try - [Landing rules](/quest/m0/helpers/landing.md) - land only heads that contain the current base, ask on a missing review, one design checkpoint - [Wait and status accuracy](/quest/m0/helpers/wait-accuracy.md) - wake on edited bot comments, honest `checks:passed`, review rounds in `quest status` - [Status holds](/quest/m0/helpers/status-holds.md) - `quest status` shows a PR's `quest:*` hold diff --git a/quest/m0/helpers/merge-state-race.md b/quest/m0/helpers/merge-state-race.md deleted file mode 100644 index 2be22888..00000000 --- a/quest/m0/helpers/merge-state-race.md +++ /dev/null @@ -1,41 +0,0 @@ -# [S] Land through a merge state GitHub is still computing - -## Goal - -`quest merge --land ` and a kix merge job land a PR on the -first try while GitHub still reports its merge state as `UNKNOWN`. Each merges -at once when nothing is left to wait for and enables auto-merge otherwise, -always pinned to ``, and neither fails with "Pull request is in clean -status". Labels on merged PRs are out of scope. - -## Plan - -Decided 2026-10-26 (user), from the merge of -[kixelated/quest#222](https://github.com/kixelated/quest/pull/222): its first -`--land` failed with "Pull request is in clean status", and the same command -merged it on a re-run. `land()` in `src/cli/hold.ts` and `landPullRequest` in -`cloudflare/src/github/service.ts` each pick `mergePullRequest` only when one -read of `mergeStateStatus` is `CLEAN`, `HAS_HOOKS`, or `UNSTABLE`; a state -GitHub has not computed yet reads `UNKNOWN` and falls through to -`enablePullRequestAutoMerge`, which GitHub refuses for a clean PR. - -- **Retry on refusal (user).** Keep enabling auto-merge pinned to the head - when the state is not known to be immediate. When GitHub refuses it because - the PR is clean, re-read the state once and merge at once, still pinned to - the head, if it is now `CLEAN`, `HAS_HOOKS`, or `UNSTABLE`; otherwise report - the refusal. Rejected: re-reading while `UNKNOWN` (a timed poll), and trying - `mergePullRequest` first (an admin token merges past required checks). -- **CLI and Worker (user).** Fix both in this quest, sharing the decision - rather than keeping two copies of `IMMEDIATE` if the Worker can import it. -- **Labels stay (user).** A merged PR keeps its `quest:merge` label, as every - merged PR does today; no cleanup and no backfill. -- Test each path: an `UNKNOWN` state that GitHub refuses as clean merges at - once, an `UNKNOWN` state that accepts auto-merge stays on auto-merge, and a - refusal for any other reason is reported unchanged. -- `land()` sends a base gated by `quest land` to auto-merge ahead of this - choice ([kixelated/quest#229](https://github.com/kixelated/quest/pull/229)), - so it applies only to ungated bases. - -## Related - -- [Landing rules](/quest/m0/helpers/landing.md) - also changes what `quest merge --land` refuses diff --git a/src/cli/hold.ts b/src/cli/hold.ts index 4d035f1b..3a538ff7 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -2,6 +2,7 @@ // time. It holds one `quest:` label, and the ID of the timeline event // that added the label is its hold token, since sessions often share an account. import { handedBack } from "../core"; +import { cleanAutoMergeRefusal, immediateMerge } from "../github/merge"; import { repositoryPath } from "./claims"; import { array, @@ -21,8 +22,6 @@ export type Method = "merge" | "squash" | "rebase"; const PREFIX = "quest:"; const MERGE = `${PREFIX}merge`; -/** Merge states GitHub merges at once, where it refuses to enable auto-merge. */ -const IMMEDIATE = ["CLEAN", "HAS_HOOKS", "UNSTABLE"]; /** The PR, its `quest:*` labels, and the timeline events that add a label or start a landing. */ const READ = `query($owner: String!, $repo: String!, $number: Int!) { @@ -149,16 +148,34 @@ export async function land( if (snapshot.queue && method !== "merge") { deps.warn(`${snapshot.url} lands through a merge queue, which applies its own merge method`); } - const mutation = - !snapshot.queue && IMMEDIATE.includes(snapshot.mergeState) - ? "mergePullRequest" - : "enablePullRequestAutoMerge"; - await labels.github.graphql( - `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!) { + const submit = (mutation: "mergePullRequest" | "enablePullRequestAutoMerge") => + labels.github.graphql( + `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!) { ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha, mergeMethod: $method }) { clientMutationId } }`, - { id: snapshot.id, sha: snapshot.head, method: method.toUpperCase() }, - ); + { id: snapshot.id, sha: snapshot.head, method: method.toUpperCase() }, + ); + if (immediateMerge(snapshot.mergeState, snapshot.queue)) { + await submit("mergePullRequest"); + return; + } + try { + await submit("enablePullRequestAutoMerge"); + } catch (error) { + if (snapshot.queue || !cleanAutoMergeRefusal(error instanceof Error ? error.message : null)) throw error; + // GitHub may finish computing UNKNOWN while it handles auto-merge. Reread once, without polling. + const fresh = await labels.read(); + current(fresh); + held(fresh, hold.token); + if ( + fresh.state !== "OPEN" || + fresh.draft || + !immediateMerge(fresh.mergeState, fresh.queue) || + (await requirements(labels.github, pr, fresh.base)).checks.some((check) => check.name === LAND_CHECK) + ) + throw error; + await submit("mergePullRequest"); + } }); } diff --git a/src/github/merge.ts b/src/github/merge.ts new file mode 100644 index 00000000..56a1945d --- /dev/null +++ b/src/github/merge.ts @@ -0,0 +1,12 @@ +// GitHub-specific landing decisions shared by the CLI and the Worker. +// Keep these outside the tracker-independent quest core. + +/** GitHub merges these states at once and refuses auto-merge because the PR is clean. */ +export function immediateMerge(state: string, queue: boolean): boolean { + return !queue && ["CLEAN", "HAS_HOOKS", "UNSTABLE"].includes(state); +} + +/** The one refusal that warrants rereading an initially unknown merge state. */ +export function cleanAutoMergeRefusal(message: unknown): boolean { + return typeof message === "string" && /\bpull request is in clean status\b/i.test(message); +} diff --git a/test/hold.test.ts b/test/hold.test.ts index b6b67199..22dd4192 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -36,6 +36,8 @@ class FakePull { readonly writes: string[] = []; /** Runs before each label is added, as when another session adds one at the same moment. */ beforeAdd?: () => void; + beforeAutoMerge?: () => string | undefined; + reads = 0; private clock = Date.parse("2026-10-21T12:00:00Z"); /** Add a label as GitHub does: re-adding one already on the PR records no event. */ @@ -112,11 +114,14 @@ class FakePull { if (name === "markPullRequestReadyForReview") this.draft = false; if (name === "mergePullRequest") this.state = "MERGED"; if (name === "enablePullRequestAutoMerge") { + const reason = this.beforeAutoMerge?.(); + if (reason) return { errors: [{ message: reason }] }; this.autoMerge = { enabledAt: "2026-10-21T12:00:00Z" }; this.event("AutoMergeEnabledEvent"); } return { data: { [name]: { clientMutationId: null } } }; } + this.reads++; return { data: { repository: { @@ -290,6 +295,72 @@ describe("merge --land", () => { expect(warnings).toEqual([`${URL_} lands through quest land, which squashes`]); }); + test.each(["CLEAN", "HAS_HOOKS", "UNSTABLE"])( + "rereads an UNKNOWN state once after a clean refusal, then merges %s at the same head", + async (state) => { + const pull = new FakePull(); + pull.mergeState = "UNKNOWN"; + pull.beforeAutoMerge = () => { + pull.mergeState = state; + return "Pull request is in clean status"; + }; + const { token } = await take(PR, "merge", undefined, pull.deps()); + const before = pull.reads; + await land(PR, token, HEAD, "squash", pull.deps()); + expect(pull.writes.slice(-2)).toEqual([ + `enablePullRequestAutoMerge ${HEAD} SQUASH`, + `mergePullRequest ${HEAD} SQUASH`, + ]); + expect(pull.reads - before).toBe(2); // The usual hold read, then one reread after refusal. + expect(pull.state).toBe("MERGED"); + }, + ); + + test("keeps UNKNOWN on auto-merge when GitHub accepts it", async () => { + const pull = new FakePull(); + pull.mergeState = "UNKNOWN"; + const { token } = await take(PR, "merge", undefined, pull.deps()); + const before = pull.reads; + await land(PR, token, HEAD, "merge", pull.deps()); + expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} MERGE`); + expect(pull.reads - before).toBe(1); + expect(pull.state).toBe("OPEN"); + }); + + test.each(["Pull request is in clean status", "Auto merge is not allowed for this repository"])( + "preserves a refused UNKNOWN auto-merge: %s", + async (reason) => { + const pull = new FakePull(); + pull.mergeState = "UNKNOWN"; + pull.beforeAutoMerge = () => reason; + const { token } = await take(PR, "merge", undefined, pull.deps()); + const before = pull.reads; + await expect(land(PR, token, HEAD, "merge", pull.deps())).rejects.toThrow(reason); + expect(pull.reads - before).toBe(reason.includes("clean status") ? 2 : 1); + expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} MERGE`); + }, + ); + + test.each(["head", "hold", "gate", "queue"])( + "does not merge after the clean refusal changes its %s", + async (change) => { + const pull = new FakePull(); + pull.mergeState = "UNKNOWN"; + pull.beforeAutoMerge = () => { + pull.mergeState = "CLEAN"; + if (change === "head") pull.head = "b".repeat(40); + if (change === "hold") pull.labels = []; + if (change === "gate") pull.required = ["quest land"]; + if (change === "queue") pull.queue = true; + return "Pull request is in clean status"; + }; + const { token } = await take(PR, "merge", undefined, pull.deps()); + await expect(land(PR, token, HEAD, "merge", pull.deps())).rejects.toThrow(); + expect(pull.state).toBe("OPEN"); + expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} MERGE`); + }, + ); + test("refuses a head that moved", async () => { const pull = new FakePull(); const { token } = await take(PR, "merge", undefined, pull.deps());