From 0bd2cd959e301634a3f1372bec487a761de2c3dc Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 17:18:29 -0800 Subject: [PATCH 1/2] chore(quest): claim quest/m0/queue-watcher/pick.md Quest-Claim: da6234678647cffa From ed57b2bf29ebb78ddaf2955d8b70af66e67a66f8 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 17:28:45 -0800 Subject: [PATCH 2/2] feat(merge): enqueue picks into GitHub's merge queue and record them On a base with a merge queue, `quest merge --land` now comments the pick (``) for the queue watcher, then enqueues the pinned head with `enqueuePullRequest`, or arms auto-merge bound to it while required checks may be pending. It no longer refuses a head behind the base there, since the queue builds each PR on the current base. A pick GitHub refuses to enqueue stays recorded, and the command warns that the watcher fixes it instead of failing. Co-Authored-By: Claude Opus 5.5 --- README.md | 19 +++-- assets/skills/merge.md | 2 +- quest/m0/queue-watcher/README.md | 1 - quest/m0/queue-watcher/bad-picks.md | 17 ++-- quest/m0/queue-watcher/pick.md | 45 ----------- quest/m0/queue-watcher/removals.md | 13 ++-- src/cli/github.ts | 19 ++++- src/cli/hold.ts | 101 ++++++++++++++++++++---- src/cli/main.ts | 13 ++-- src/cli/pick.ts | 28 +++++++ test/github.test.ts | 16 ++-- test/hold.test.ts | 115 +++++++++++++++++++++++++++- 12 files changed, 297 insertions(+), 92 deletions(-) delete mode 100644 quest/m0/queue-watcher/pick.md create mode 100644 src/cli/pick.ts diff --git a/README.md b/README.md index b52024f1..72595c2a 100644 --- a/README.md +++ b/README.md @@ -457,16 +457,23 @@ ignored. queue signal, so a queued PR shows no hold. It refuses while any session holds the PR, so release your own hold first. `--land` needs the full 40-character head SHA and refuses once the head moves. -It enables auto-merge bound to that SHA, which enters GitHub's merge queue -when the base has one, or merges at once when nothing is left to wait for. It -makes a merge commit; `--squash` or `--rebase` picks another method, and a -merge queue applies its own. On a base that requires the `quest land` check, it -queues the PR for [`quest land`](#land-a-batch-with-quest-land) instead. +It enables auto-merge bound to that SHA, or merges at once when nothing is +left to wait for. It makes a merge commit; `--squash` or `--rebase` picks +another method. On a base that requires the `quest land` check, it queues the +PR for [`quest land`](#land-a-batch-with-quest-land) instead. +On a base with GitHub's merge queue, which applies its own method, it first +records the pick for the queue watcher: a comment naming the head, ending in +``. It then enqueues that head, or arms auto-merge +bound to it while required checks may still be pending. The queue builds each +PR on the current base, so a head behind it is enqueued as is. When GitHub +refuses to enqueue the pick, such as for a conflict or failing required checks, +the pick stays recorded and the command warns that the watcher fixes it, rather +than failing; where no watcher runs, fix it and land it again. On an ungated base with auto-merge disabled, it refuses. Follow the repository’s own landing instructions in CONTRIBUTING.md or AGENTS.md, such as its label queue; never retry with `--direct` unless the user asks. `--direct` permits the usual landing behavior despite that repository setting. -On an ungated base, it compares the live base against that head and refuses a +On an ungated base without a merge queue, it compares the live base against that head and refuses a head that is behind or diverged: merge the base, rerun the repository's checks, and land the new head. This is a preflight check; GitHub can still move the base between that comparison and its head-pinned merge mutation. A PR that leaves diff --git a/assets/skills/merge.md b/assets/skills/merge.md index 2bd3d0be..5f79012e 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -30,7 +30,7 @@ Queue the head with `quest merge --land `. It makes a merge commit; add `--squash` or `--rebase` when the repository's root instructions name that method for the PR's base. On a base that requires the `quest land` check, it queues the PR for `quest land`, whose chain merges and tests against the current base. On an ungated base with auto-merge disabled, it refuses. Follow the repository’s own landing instructions (for example, its label queue); never retry with `--direct` unless the user explicitly asks. -On an ungated base, it refuses a head that does not contain the current base: recommend `/quest-iterate` to merge the base. +On an ungated base without a merge queue, it refuses a head that does not contain the current base: recommend `/quest-iterate` to merge the base. Report once it is queued, and do not wait for it to merge. Plan the PR's suggested follow-ups as soon as it is queued, as if it lands, rather than waiting for the merge, unless a coordinator such as `/quest-complete` called this skill and batches them. diff --git a/quest/m0/queue-watcher/README.md b/quest/m0/queue-watcher/README.md index 39aed80e..f6a4354c 100644 --- a/quest/m0/queue-watcher/README.md +++ b/quest/m0/queue-watcher/README.md @@ -107,7 +107,6 @@ Shared shape, for the children to follow: ## Required -- [Picks enter the merge queue](/quest/m0/queue-watcher/pick.md) - `quest merge --land` enqueues the pinned head into GitHub's merge queue and records the pick where the watcher reads it - [Queue removals](/quest/m0/queue-watcher/removals.md) - the watcher sees each PR the queue drops, why, and the failing build's log - [Fix and re-enqueue](/quest/m0/queue-watcher/fix.md) - the fixer runs on each dropped PR in the background, and the watcher re-enqueues it at the back - [Stop rules](/quest/m0/queue-watcher/stop.md) - the fixer keeps going while each round changes something, and hands back on a no-op fix, a repeated failure, or three flaky re-enqueues diff --git a/quest/m0/queue-watcher/bad-picks.md b/quest/m0/queue-watcher/bad-picks.md index 207114e8..efd743d8 100644 --- a/quest/m0/queue-watcher/bad-picks.md +++ b/quest/m0/queue-watcher/bad-picks.md @@ -32,10 +32,18 @@ the fixer with the failing check's log (decided 2026-11-01, settling [moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)'s decision 8), since on a merge-queue base such a PR is a pick GitHub won't enqueue. -- The watcher learns of refused picks from the marker - [Picks enter the merge queue](/quest/m0/queue-watcher/pick.md) records, and - from picks whose auto-merge stays armed while the PR is conflicting or its - required checks failed. +- The watcher learns of refused picks from the pick comments + `quest merge --land` posts (see + [Queue removals](/quest/m0/queue-watcher/removals.md)) on PRs not in the + queue, and from picks whose auto-merge stays armed while the PR is + conflicting or its required checks failed. `quest merge` arms auto-merge + only while required checks may be pending (`BLOCKED` or `UNKNOWN`); + otherwise it calls `enqueuePullRequest`, and GitHub's refusal leaves just + the comment. +- Enqueue a pick whose auto-merge is armed on a ready PR that never entered + the queue: since about 2026-09-10 GitHub often leaves such PRs out + ([community#209348](https://github.com/orgs/community/discussions/209348)), + while `enqueuePullRequest` works. - Merge locally with `mergeBase` (`src/cli/land.ts`), which already uses the driver; the host gate's `prepareHead` (`src/cli/land-head.ts`) shows the push. A clean driver merge needs no review: it is a conflict-free base merge. @@ -46,4 +54,3 @@ since on a merge-queue base such a PR is a pick GitHub won't enqueue. ## Required - [Fix and re-enqueue](/quest/m0/queue-watcher/fix.md) - the fixer path and re-enqueue this reuses -- [Picks enter the merge queue](/quest/m0/queue-watcher/pick.md) - the pick marker a refused pick leaves diff --git a/quest/m0/queue-watcher/pick.md b/quest/m0/queue-watcher/pick.md deleted file mode 100644 index bacf262c..00000000 --- a/quest/m0/queue-watcher/pick.md +++ /dev/null @@ -1,45 +0,0 @@ -# [S] quest merge --land enqueues into GitHub's merge queue - -## Goal - -On a base with a merge queue, `quest merge --land ` -enqueues the pinned head into GitHub's merge queue, or arms auto-merge bound -to it while its required checks are pending, and records the pick so the -watcher knows which head the maintainer picked. It never refuses a head for -being behind the base, since the queue builds each PR on the current base. A -pick GitHub refuses to enqueue (conflicts, failing required checks, or another -mergeability refusal) is still recorded, and the command says the watcher will -fix it rather than failing. - -## Plan - -Decided 2026-11-01 (maintainer, /quest-plan; see the -[epic](/quest/m0/queue-watcher/README.md)): picks enqueue into GitHub's merge -queue, and the watcher fixes a pick GitHub refuses. - -`land` in `src/cli/hold.ts` already enables auto-merge bound to the head on a -queue base (`snapshot.queue`), which enters the queue once checks pass. What -changes: - -- Skip the behind-or-diverged comparison on a queue base. It exists for - ungated bases, where nothing else tests the PR on the current base. -- Prefer `enqueuePullRequest` with `expectedHeadOid` when the PR is already - mergeable, so the head is pinned and enqueued at once. GitHub enqueues a PR - only once its required checks pass on the PR itself. -- Record the pick durably. On the host gate the pin lives in squash - auto-merge's commit body, which a conflicting PR cannot get, and GitHub drops - auto-merge when the queue removes a PR. Pick a marker the watcher can read - for a refused or removed PR, such as a comment naming the picked head, and - coordinate with [moq-dev/quest#326](https://github.com/moq-dev/quest/pull/326), - which drops the `quest:merge` label. The pick covers that head and the heads - the fixer pushes from it. -- `quest:merge` (or its successor) must not be released when the queue drops - the PR while the watcher still holds it (`dequeued` in `src/cli/hold.ts`); - only a handback ends the pick. -- Bases that require the `quest land` check keep queuing for the host gate - until their repository [follows](/quest/m0/queue-watcher/follow.md). - -## Related - -- [quest merge --land gaps](/quest/m0/helpers/merge-land.md) - the same command's other gaps, in [moq-dev/quest#347](https://github.com/moq-dev/quest/pull/347) -- [Bad picks](/quest/m0/queue-watcher/bad-picks.md) - what the watcher does with a refused pick diff --git a/quest/m0/queue-watcher/removals.md b/quest/m0/queue-watcher/removals.md index d8fb410e..77585795 100644 --- a/quest/m0/queue-watcher/removals.md +++ b/quest/m0/queue-watcher/removals.md @@ -17,14 +17,19 @@ Decided 2026-11-01 (maintainer, /quest-plan; see the - Poll, don't receive webhooks: the host has no public endpoint. Reconcile from the picks, not from queue snapshots alone: a PR enqueued and dropped between two polls, or while the watcher was down, never shows in a - snapshot. Each pass reads every open PR holding an active pick (the marker - [Picks enter the merge queue](/quest/m0/queue-watcher/pick.md) records) with + snapshot. Each pass reads every open PR holding an active pick with its `mergeQueueEntry` and its `RemovedFromMergeQueueEvent`s newer than the last one handled, with `reason`, `actor`, and `enqueuer`. `quest status` already reads that event (`src/cli/status.ts`). The base's `mergeQueue { entries }` can still serve as a cheap change signal. The App's installation token has its own rate limit, separate from the maintainers' sessions. +- A pick is the PR comment `quest merge --land` posts on a merge-queue base, + ending in ``; `pickedHead` in `src/cli/pick.ts` + reads it. Anyone can comment, so honor a pick only from an author who can + write to the repository. A pick covers the head it names and the heads the + fixer pushes from it, and stays active until a handback, a push by anyone + else, or a person's removal; a newer pick comment starts a new one. - For a failed build, find the PR's `merge_group` workflow runs (event `merge_group`, head branch `gh-readonly-queue//pr--`) and fetch the failing jobs' logs through the App's `actions: read`, trimmed the @@ -39,7 +44,3 @@ Decided 2026-11-01 (maintainer, /quest-plan; see the - Start it as `quest land watch` beside `quest land serve`, sharing its App credential options, state directory, and comment marker and deduplication. - -## Required - -- [Picks enter the merge queue](/quest/m0/queue-watcher/pick.md) - the durable pick marker the watcher reconciles from diff --git a/src/cli/github.ts b/src/cli/github.ts index 65a8d303..a67584a2 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -33,6 +33,16 @@ export class GitHubDeadline extends Error { } } +/** GitHub answered a GraphQL operation with errors; `types` are their types, `undefined` for an untyped one. */ +export class GitHubGraphQLError extends Error { + constructor( + readonly types: (string | undefined)[], + messages: string[], + ) { + super(`GitHub GraphQL error: ${messages.join("; ")}`); + } +} + export type ObjectValue = Record; /** Retries of a 5xx or a dropped connection, waiting 1, 2, 4, then 8 seconds unless GitHub says how long. */ @@ -417,7 +427,10 @@ export class GitHub { if (body.errors === undefined) return object(body.data); const errors = array(body.errors); if (!errors.some((error) => error.type === "RATE_LIMITED")) - throw new Error(`GitHub GraphQL error: ${errors.map((error) => String(error.message)).join("; ")}`); + throw new GitHubGraphQLError( + errors.map((error) => (typeof error.type === "string" ? error.type : undefined)), + errors.map((error) => String(error.message)), + ); // GitHub turned the operation away unprocessed, so any operation repeats. await this.waitOut(response.headers, limits, "GitHub GraphQL rate limit persisted"); } @@ -451,6 +464,10 @@ export class GitHub { */ const PULL_MUTATIONS: Record = { mergePullRequest: { fields: "merged", landed: (pull) => pull.merged === true }, + enqueuePullRequest: { + fields: "merged mergeQueueEntry { state }", + landed: (pull) => pull.merged === true || pull.mergeQueueEntry != null, + }, enablePullRequestAutoMerge: { fields: "merged autoMergeRequest { enabledAt } mergeQueueEntry { state }", landed: (pull) => pull.merged === true || pull.autoMergeRequest != null || pull.mergeQueueEntry != null, diff --git a/src/cli/hold.ts b/src/cli/hold.ts index c6137001..e59648d0 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -9,6 +9,7 @@ import { array, GitHub, type GitHubDependencies, + GitHubGraphQLError, object, type ObjectValue, type PullRequest, @@ -16,6 +17,7 @@ import { string, } from "./github"; import { LAND_CHECK, enqueue } from "./land"; +import { recordPick } from "./pick"; import { Refusal, ago } from "./start"; export type Method = "merge" | "squash" | "rebase"; @@ -24,7 +26,7 @@ export type Method = "merge" | "squash" | "rebase"; const READ = `query HoldOwnership($owner: String!, $repo: String!, $number: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { - id url state isDraft body updatedAt baseRefName headRefOid mergeStateStatus isMergeQueueEnabled + id url state isDraft body updatedAt baseRefName headRefOid mergeStateStatus isMergeQueueEnabled isInMergeQueue labels(first: 100) { nodes { name } } timelineItems(last: 100, itemTypes: [LABELED_EVENT]) { nodes { __typename ... on LabeledEvent { id createdAt label { name } } } @@ -51,6 +53,8 @@ interface Snapshot { head: string; mergeState: string; queue: boolean; + /** Whether the PR is in its base's merge queue. */ + queued: boolean; /** The hold labels on the PR. */ labels: string[]; /** Oldest first. */ @@ -151,8 +155,8 @@ export async function finish(pr: PullRequest, token: string, deps: GitHubDepende /** * Land `sha`, which must still be the head of `pr`, while no session holds it: - * enable auto-merge bound to it, which enters the merge queue when the base has one, - * or merge at once when GitHub would refuse auto-merge because nothing is left to wait for. + * enable auto-merge bound to it, or merge at once when GitHub would refuse auto-merge because + * nothing is left to wait for. A base with a merge queue gets {@link pick} instead. * A base that requires the `quest land` check queues it for `quest land` instead: * squash auto-merge whose commit body pins `sha`. It takes no label. */ @@ -164,11 +168,7 @@ export async function land( direct = false, ): Promise { const labels = new Labels(pr, deps); - const current = (snapshot: Snapshot) => { - if (snapshot.head.toLowerCase() !== sha.toLowerCase()) { - throw new Refusal(`${snapshot.url} head moved to ${snapshot.head}; check the new head before landing it`); - } - }; + const current = (snapshot: Snapshot) => headAt(snapshot, sha); const snapshot = await labels.read(); open(snapshot); current(snapshot); @@ -192,8 +192,16 @@ export async function land( await enqueue(labels.github, snapshot.id, snapshot.head); return; } - // A merge queue is the repository's own gate, so it decides; otherwise auto-merge must be on. - if (!direct && !snapshot.queue) { + // A merge queue is the repository's own gate, so it decides. + if (snapshot.queue) { + if (method !== "merge") + deps.warn(`${snapshot.url} lands through a merge queue, which applies its own merge method`); + await unchanged(); + await pick(labels, pr, snapshot, method, deps); + return; + } + // Otherwise auto-merge must be on. + if (!direct) { const repository = object(await (await labels.github.request(repositoryPath(pr))).json()); // GitHub shows the merge settings only to writers; fail closed, with a next step. if (typeof repository.allow_auto_merge !== "boolean") @@ -219,9 +227,6 @@ export async function land( `${snapshot.url} head ${snapshot.head} does not contain ${snapshot.base} at ${base}; merge the base and rerun the repository's checks before landing`, ); } - if (snapshot.queue && method !== "merge") { - deps.warn(`${snapshot.url} lands through a merge queue, which applies its own merge method`); - } await unchanged(); const submit = (mutation: "mergePullRequest" | "enablePullRequestAutoMerge") => labels.github.graphql( @@ -237,7 +242,7 @@ export async function land( try { await submit("enablePullRequestAutoMerge"); } catch (error) { - if (snapshot.queue || !cleanAutoMergeRefusal(error instanceof Error ? error.message : null)) throw error; + if (!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); @@ -254,6 +259,66 @@ export async function land( } } +/** Merge states in which GitHub may still be running required checks, so only auto-merge can wait for them. */ +const PENDING = ["BLOCKED", "UNKNOWN"]; + +/** GraphQL error types that fault the token or the request, not the PR GitHub would enqueue. */ +const UNREFUSED = ["FORBIDDEN", "NOT_FOUND", "INSUFFICIENT_SCOPES", "INTERNAL"]; + +/** + * Pick `snapshot`'s head to land through the base's merge queue, as {@link land} checked it: + * record the pick for the queue watcher, then enqueue that head, or arm auto-merge bound to it + * while its required checks may be pending. The queue builds each PR on the current base, so a + * head behind it is enqueued as is. A pick GitHub refuses, such as one that conflicts or fails + * its required checks, stays recorded: the watcher fixes it, so this warns instead of failing. + */ +async function pick( + labels: Labels, + pr: PullRequest, + snapshot: Snapshot, + method: Method, + deps: GitHubDependencies, +): Promise { + await recordPick(labels.github, pr, snapshot.head); + const enqueuePull = () => + labels.github.graphql( + `mutation($id: ID!, $sha: GitObjectID!) { + enqueuePullRequest(input: { pullRequestId: $id, expectedHeadOid: $sha }) { clientMutationId } + }`, + { id: snapshot.id, sha: snapshot.head }, + ); + try { + if (!PENDING.includes(snapshot.mergeState)) { + await enqueuePull(); + return; + } + try { + await labels.github.graphql( + `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!) { + enablePullRequestAutoMerge(input: { pullRequestId: $id, expectedHeadOid: $sha, mergeMethod: $method }) { clientMutationId } + }`, + { id: snapshot.id, sha: snapshot.head, method: method.toUpperCase() }, + ); + } catch (error) { + // An UNKNOWN state GitHub finished computing as ready: nothing is left to wait for. + if (!cleanAutoMergeRefusal(error instanceof Error ? error.message : null)) throw error; + await enqueuePull(); + } + } catch (error) { + if (!(error instanceof GitHubGraphQLError) || error.types.some((type) => UNREFUSED.includes(type ?? ""))) + throw error; + // A head that moved since is no longer the pick; say so rather than promise a fix. + const fresh = await labels.read(); + open(fresh); + headAt(fresh, snapshot.head); + // Already queued, as when this pick repeats an earlier one: GitHub refused nothing that matters. + if (fresh.queued) return; + deps.warn( + `${snapshot.url}: GitHub refused to enqueue ${snapshot.head} (${error.message}). The pick is recorded, so the queue watcher fixes it and enqueues it; where none runs, fix it and land it again`, + ); + } +} + /** * Finish `work` under `hold`, which may be a label just swapped in for the caller's. * A failure names the hold, so the caller keeps the token it needs to release or retry. @@ -315,6 +380,13 @@ async function acquire(labels: Labels, token: string | undefined): Promise<{ hol return { hold: { label, token: ours.id }, snapshot: { ...after, labels: [label] } }; } +/** Refuse once the head is no longer `sha`, the head being landed. */ +function headAt(snapshot: Snapshot, sha: string): void { + if (snapshot.head.toLowerCase() !== sha.toLowerCase()) { + throw new Refusal(`${snapshot.url} head moved to ${snapshot.head}; check the new head before landing it`); + } +} + function open(snapshot: Snapshot): void { if (snapshot.state !== "OPEN") { throw new Refusal(`${snapshot.url} is ${snapshot.state.toLowerCase()}; a push now would bring its branch back`); @@ -413,6 +485,7 @@ class Labels { head: string(row.headRefOid), mergeState: string(row.mergeStateStatus), queue: row.isMergeQueueEnabled === true, + queued: row.isInMergeQueue === true, labels: array(object(row.labels).nodes) .map((label) => string(label.name)) .filter(isHoldLabel), diff --git a/src/cli/main.ts b/src/cli/main.ts index 88734528..9c3c4dcd 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -474,11 +474,14 @@ export function main(argv: string[], out: Output): number | Promise { .command("merge") .summary("Land a PR's reviewed head") .description( - "Land a PR's reviewed head: enable auto-merge bound to the full head SHA --land names, which enters the " + - "base's merge queue when it has one, or merge at once when nothing is left to wait for. It takes no " + - "label: the auto-merge it enables is the queue signal. It refuses while a session holds the PR, so " + - "release your own hold first, and once the head has moved. It makes a merge commit unless --squash " + - "or --rebase picks another method; a merge queue applies its own. On a base " + + "Land a PR's reviewed head: enable auto-merge bound to the full head SHA --land names, or merge at " + + "once when nothing is left to wait for. It takes no label: the auto-merge it enables is the queue " + + "signal. It refuses while a session holds the PR, so release your own hold first, and once the head " + + "has moved. It makes a merge commit unless --squash or --rebase picks another method. On a base with " + + "a merge queue, which applies its own method, it comments the pick for the queue watcher, then " + + "enqueues the head, or arms auto-merge bound to it while required checks may be pending; a head " + + "behind the base is enqueued as is, and a pick GitHub refuses to enqueue warns that the watcher fixes " + + "it instead of failing. On a base " + `that requires the \`${LAND_CHECK}\` check, it queues the PR for \`quest land\` instead: squash ` + "auto-merge whose commit body pins the head. On an ungated base with auto-merge disabled, it refuses " + "unless --direct is passed. Follow the repository landing instructions in CONTRIBUTING.md or " + diff --git a/src/cli/pick.ts b/src/cli/pick.ts new file mode 100644 index 00000000..d39d1e3d --- /dev/null +++ b/src/cli/pick.ts @@ -0,0 +1,28 @@ +// The pick `quest merge --land` records on a merge-queue base: a PR comment naming the head +// the maintainer picked. GitHub keeps no trace of a pick it refused to enqueue, and drops the +// queue entry and auto-merge when its queue removes a PR, so the queue watcher reads the pick here. +// Anyone can comment, so a reader honors only a pick by an author who can write to the repository. +import { repositoryPath } from "./claims"; +import type { GitHub, PullRequest } from "./github"; + +const MARKER = /^$/m; + +/** The comment that picks `sha` to land through the merge queue. */ +export function pickComment(sha: string): string { + const head = sha.toLowerCase(); + return ( + `Picked \`${head}\` to land through the merge queue. If the queue drops it or GitHub refuses to ` + + "enqueue it, the queue watcher fixes it and enqueues it again, where one runs for this repository; " + + `a push by anyone else, or a person removing it from the queue, ends the pick.\n\n` + ); +} + +/** The head a pick comment's body names, if it is one. */ +export function pickedHead(body: string): string | null { + return MARKER.exec(body)?.[1] ?? null; +} + +/** Record the pick of `sha` on `pr`. Each pick is its own comment, so a pick after a handback starts anew. */ +export async function recordPick(github: GitHub, pr: PullRequest, sha: string): Promise { + await github.request(`${repositoryPath(pr)}/issues/${pr.number}/comments`, { body: { body: pickComment(sha) } }); +} diff --git a/test/github.test.ts b/test/github.test.ts index 0260baa8..41278a00 100644 --- a/test/github.test.ts +++ b/test/github.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "vitest"; -import { GitHub, GitHubDeadline, type GitHubDependencies } from "../src/cli/github"; +import { GitHub, GitHubDeadline, type GitHubDependencies, GitHubGraphQLError } from "../src/cli/github"; const repo = "/repos/example/project"; const HEAD = "a".repeat(40); @@ -324,6 +324,7 @@ describe("GraphQL", () => { test.each([ ["mergePullRequest", { merged: true }], ["enablePullRequestAutoMerge", { merged: false, autoMergeRequest: null, mergeQueueEntry: { state: "QUEUED" } }], + ["enqueuePullRequest", { merged: false, mergeQueueEntry: { state: "QUEUED" } }], ["markPullRequestReadyForReview", { isDraft: false }], ])("%s is confirmed from the pull request at its pinned head", async (mutation, node) => { const query = `mutation($id: ID!, $sha: GitObjectID!) { ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha }) { clientMutationId } }`; @@ -377,10 +378,15 @@ describe("GraphQL", () => { }); test("other GraphQL errors fail without waiting", async () => { - const { github, sleeps } = fixture([json({ errors: [{ type: "NOT_FOUND", message: "Not Found" }] })]); - await expect(github.graphql("query { viewer { login } }", {})).rejects.toThrow( - "GitHub GraphQL error: Not Found", - ); + const { github, sleeps } = fixture([ + json({ errors: [{ type: "NOT_FOUND", message: "Not Found" }, { message: "Not mergeable" }] }), + ]); + const error = await github.graphql("query { viewer { login } }", {}).catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(GitHubGraphQLError); + expect(error).toMatchObject({ + message: "GitHub GraphQL error: Not Found; Not mergeable", + types: ["NOT_FOUND", undefined], + }); expect(sleeps).toEqual([]); }); diff --git a/test/hold.test.ts b/test/hold.test.ts index 4f35eb83..f45617d0 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -5,6 +5,7 @@ import { describe, expect, test, vi } from "vitest"; import type { GitHubDependencies, ObjectValue } from "../src/cli/github"; import { finish, land, release, take, withoutHandBack } from "../src/cli/hold"; import { type Output, main } from "../src/cli/main"; +import { pickComment, pickedHead } from "../src/cli/pick"; import { Refusal } from "../src/cli/start"; const PR = { owner: "example", repo: "project", number: 7 }; @@ -47,6 +48,10 @@ class FakePull { /** Runs after each label is added, as when another session adds one a moment later. */ afterAdd?: () => void; beforeAutoMerge?: () => string | undefined; + /** The error GitHub answers `enqueuePullRequest` with, if any. */ + beforeEnqueue?: () => { message: string; type?: string } | undefined; + /** Comment bodies on the PR, oldest first. */ + readonly comments: string[] = []; reads = 0; private clock = Date.parse("2026-10-21T12:00:00Z"); @@ -99,6 +104,16 @@ class FakePull { this.labels = this.labels.filter((label) => label !== name); return json(200, []); } + if (url.pathname === "/repos/example/project/issues/7/comments") { + if (method === "GET") + return json( + 200, + this.comments.map((body, id) => ({ id, body })), + ); + this.comments.push(String(body.body)); + this.writes.push(`comment ${String(body.body)}`); + return json(201, { id: this.comments.length - 1, body: body.body }); + } if (url.pathname === "/repos/example/project/branches/main") return json(200, { protection: {} }); if (url.pathname === `/repos/example/project/compare/main...${this.head}`) { this.comparisons.push(`${url.pathname}${url.search}`); @@ -127,7 +142,7 @@ class FakePull { if (variables.id !== "PR_7") throw new Error("mutation on the wrong PR"); const method = variables.method ?? /mergeMethod: (\w+)/.exec(query)?.[1]; const body = variables.body ? ` ${String(variables.body)}` : ""; - this.writes.push(method ? `${name} ${variables.sha} ${method}${body}` : name); + this.writes.push([name, variables.sha, method].filter(Boolean).join(" ") + body); if (name === "markPullRequestReadyForReview") this.draft = false; if (name === "mergePullRequest") this.state = "MERGED"; if (name === "enablePullRequestAutoMerge") { @@ -138,6 +153,11 @@ class FakePull { this.event("AutoMergeEnabledEvent"); } if (name === "disablePullRequestAutoMerge") this.autoMerge = null; + if (name === "enqueuePullRequest") { + const error = this.beforeEnqueue?.(); + if (error) return { errors: [error] }; + this.queueEntry = { state: "QUEUED" }; + } return { data: { [name]: { clientMutationId: null } } }; } if (query.includes("node(id: $id)")) return { data: { node: { autoMergeRequest: this.autoMerge } } }; @@ -164,6 +184,7 @@ class FakePull { isMergeQueueEnabled: this.queue, autoMergeRequest: this.autoMerge, mergeQueueEntry: this.queueEntry, + isInMergeQueue: this.queueEntry !== null, labels: { nodes: this.labels.map((name) => ({ name })) }, timelineItems: { nodes: this.events.slice(-100) }, }, @@ -364,6 +385,94 @@ describe("merge --land", () => { expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} MERGE`); }); + test.each(["CLEAN", "HAS_HOOKS", "UNSTABLE", "BEHIND", "DIRTY"])( + "records the pick, then enqueues a %s head on a queue base without comparing it to the base", + async (state) => { + const pull = new FakePull(); + pull.queue = true; + pull.mergeState = state; + pull.comparison = "diverged"; + const warnings: string[] = []; + await land(PR, HEAD, "merge", pull.deps(warnings)); + expect(pull.writes).toEqual([`comment ${pickComment(HEAD)}`, `enqueuePullRequest ${HEAD}`]); + expect(pull.queueEntry).not.toBeNull(); + expect(pull.comparisons).toEqual([]); + expect(warnings).toEqual([]); + }, + ); + + test("arms auto-merge bound to the head while a queue base's required checks are pending", async () => { + const pull = new FakePull(); + pull.queue = true; + pull.mergeState = "BLOCKED"; + await land(PR, HEAD, "merge", pull.deps()); + expect(pull.writes).toEqual([`comment ${pickComment(HEAD)}`, `enablePullRequestAutoMerge ${HEAD} MERGE`]); + }); + + test("enqueues an UNKNOWN head GitHub finds clean when arming auto-merge", async () => { + const pull = new FakePull(); + pull.queue = true; + pull.mergeState = "UNKNOWN"; + pull.beforeAutoMerge = () => "Pull request is in clean status"; + await land(PR, HEAD, "squash", pull.deps()); + expect(pull.writes.slice(1)).toEqual([ + `enablePullRequestAutoMerge ${HEAD} SQUASH`, + `enqueuePullRequest ${HEAD}`, + ]); + }); + + test("keeps a pick GitHub refuses to enqueue, and says the watcher fixes it", async () => { + const pull = new FakePull(); + pull.queue = true; + pull.mergeState = "DIRTY"; + pull.beforeEnqueue = () => ({ message: "Pull request is not mergeable", type: "UNPROCESSABLE" }); + const warnings: string[] = []; + await land(PR, HEAD, "merge", pull.deps(warnings)); + expect(pull.comments).toEqual([pickComment(HEAD)]); + expect(warnings).toEqual([ + `${URL_}: GitHub refused to enqueue ${HEAD} (GitHub GraphQL error: Pull request is not mergeable). The pick is recorded, so the queue watcher fixes it and enqueues it; where none runs, fix it and land it again`, + ]); + }); + + test("says nothing when GitHub refuses a pick already in the queue", async () => { + const pull = new FakePull(); + pull.queue = true; + pull.mergeState = "CLEAN"; + pull.beforeEnqueue = () => { + pull.queueEntry = { state: "QUEUED" }; + return { message: "Pull request is already queued" }; + }; + const warnings: string[] = []; + await land(PR, HEAD, "merge", pull.deps(warnings)); + expect(warnings).toEqual([]); + }); + + test("refuses when the head moved before GitHub refused the pick", async () => { + const pull = new FakePull(); + pull.queue = true; + pull.mergeState = "CLEAN"; + pull.beforeEnqueue = () => { + pull.head = "c".repeat(40); + return { message: "Head ref has moved" }; + }; + expect(await refused(land(PR, HEAD, "merge", pull.deps()))).toContain(`head moved to ${"c".repeat(40)}`); + }); + + test("fails on a refusal that faults the token rather than the pick", async () => { + const pull = new FakePull(); + pull.queue = true; + pull.mergeState = "CLEAN"; + pull.beforeEnqueue = () => ({ message: "Resource not accessible by integration", type: "FORBIDDEN" }); + await expect(land(PR, HEAD, "merge", pull.deps())).rejects.toThrow("Resource not accessible by integration"); + expect(pull.comments).toEqual([pickComment(HEAD)]); + }); + + test("names the picked head in a marker only a pick comment carries", () => { + expect(pickedHead(pickComment(HEAD.toUpperCase()))).toBe(HEAD); + expect(pickedHead(`quoted: \`\``)).toBeNull(); + expect(pickedHead("Summary.")).toBeNull(); + }); + test("--direct explicitly permits a clean merge with auto-merge off", async () => { const pull = new FakePull(); pull.allowAutoMerge = false; @@ -425,13 +534,13 @@ describe("merge --land", () => { } }); - test("enters the merge queue through auto-merge even when clean", async () => { + test("warns that a merge queue applies its own method", async () => { const pull = new FakePull(); pull.mergeState = "CLEAN"; pull.queue = true; const warnings: string[] = []; await land(PR, HEAD, "squash", pull.deps(warnings)); - expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} SQUASH`); + expect(pull.writes.at(-1)).toBe(`enqueuePullRequest ${HEAD}`); expect(warnings).toEqual([`${URL_} lands through a merge queue, which applies its own merge method`]); });