diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 8dc63c0a..31000f6c 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -10,6 +10,7 @@ import { parse, ready, } from "quest/core"; +import { cleanAutoMergeRefusal, immediateMerge } from "../../../src/github/merge"; import { osiApproved } from "../licenses"; import { positiveId } from "./webhook"; import { @@ -1512,33 +1513,56 @@ 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 || heldToken(holds, MERGE_LABEL) !== 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 || heldToken(holds, MERGE_LABEL) !== 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" }; @@ -1557,8 +1581,6 @@ function mergeMethod(current: Repository): MergeMethod { throw new GitHubError("The repository allows no merge method"); } -/** 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 f0b12523..442d9ad6 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 9ae4d51b..bbc380e0 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 - [Wait and status accuracy](/quest/m0/helpers/wait-accuracy.md) - wake on edited bot comments, honest `checks:passed`, review rounds in `quest status` - [Headless agents](/quest/m0/helpers/headless-agents.md) - `/quest-spawn api` launches, resumes, and stops billed agents through one command auto mode allows - [Sibling PRs through a server-side merge](/quest/m0/helpers/queue-siblings.md) - children of one epic land back to back through a merge queue without conflicting on its link list diff --git a/quest/m0/helpers/merge-state-race.md b/quest/m0/helpers/merge-state-race.md deleted file mode 100644 index 95369486..00000000 --- a/quest/m0/helpers/merge-state-race.md +++ /dev/null @@ -1,37 +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. diff --git a/src/cli/hold.ts b/src/cli/hold.ts index 166e9c2f..f1a1e7e9 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 { HOLD_PREFIX, MERGE_LABEL, handedBack, isHoldLabel, latestHold, heldToken, spentMergeHold } from "../core"; +import { cleanAutoMergeRefusal, immediateMerge } from "../github/merge"; import { repositoryPath } from "./claims"; import { array, @@ -19,9 +20,6 @@ import { Refusal, ago } from "./start"; export type State = "iterate" | "ready" | "merge"; export type Method = "merge" | "squash" | "rebase"; -/** 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 HoldOwnership($owner: String!, $repo: String!, $number: Int!) { repository(owner: $owner, name: $repo) { @@ -219,16 +217,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 016af9a2..65242290 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -40,6 +40,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. */ @@ -120,11 +122,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: { @@ -326,6 +331,72 @@ describe("merge --land", () => { expect(pull.comparisons).toEqual([]); }); + 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());