From 5ddbfd26f62002097a57b8c5b2f56b74d085e4bc Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 31 Oct 2026 23:40:53 -0700 Subject: [PATCH 1/6] chore(quest): claim quest/m0/helpers/merge-land.md Quest-Claim: 0b8a7d99d5a697e1 From 6a1e88fba27aa430866ad5d3accaf75aa63d3676 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 00:06:35 -0700 Subject: [PATCH 2/6] feat(merge): close quest merge --land's gaps - Pick the repository's merge method: the one --squash or --rebase names, refused when disallowed, else the only one allowed, else a merge commit. - A bare PR number outside a checkout says to pass the PR URL. - A direct merge releases quest:merge once GitHub reports it merged. - quest status reports the head auto-merge was enabled on, from the quest land pin or the head branch's push activity, and quest wait prints auto-merge:moved when it differs from the current head. The iterate skill disables auto-merge before pushing. Deletes the finished quest quest/m0/helpers/merge-land.md. Co-Authored-By: Claude Opus 5.5 --- README.md | 34 +++++++--- assets/skills/iterate.md | 1 + assets/skills/merge.md | 4 +- quest/m0/helpers/README.md | 1 - quest/m0/helpers/merge-land.md | 42 ------------- src/cli/github.ts | 10 ++- src/cli/hold.ts | 60 +++++++++++++++--- src/cli/land.ts | 2 +- src/cli/main.ts | 21 +++++-- src/cli/status.ts | 54 +++++++++++++--- src/cli/wait.ts | 34 ++++++++-- test/hold.test.ts | 109 ++++++++++++++++++++++++++++++--- test/status.test.ts | 84 ++++++++++++++++++++++++- test/wait.test.ts | 61 +++++++++++++++++- 14 files changed, 424 insertions(+), 93 deletions(-) delete mode 100644 quest/m0/helpers/merge-land.md diff --git a/README.md b/README.md index ed9557bd..b2ea1260 100644 --- a/README.md +++ b/README.md @@ -289,7 +289,8 @@ quest command to register the current one. `quest wait` blocks until a GitHub PR has a review, comment, review-thread reply, reaction to the PR or one of its reviews, push, merge, close, removal -from the merge queue, disabled auto-merge, or required CI result: +from the merge queue, disabled auto-merge or auto-merge left on a moved head, or +required CI result: ```sh quest wait 101 @@ -297,7 +298,8 @@ quest wait https://github.com/moq-dev/quest/pull/101 --since 2026-10-07T12:00:00 quest wait 101 --since abcdef0123456789abcdef0123456789abcdef01 ``` -A number uses the repository's `origin`; a URL works from any directory. +A number uses the repository's `origin`, so it needs a checkout or `--root` +naming one; a URL works from any directory. Authentication comes from `GH_TOKEN`, then `GITHUB_TOKEN`, then `gh auth token`. Without `--since`, the point is the current head's push time, so an existing review on that head is included. GitHub lists a push seconds to minutes after @@ -346,6 +348,12 @@ was reached at or after the starting point, so a wait started after CI finished still reports it; a push starts over for the new head, and the cursor records what was reported, so resuming never replays it. +GitHub keeps auto-merge across a push by anyone with write access, so it can +land a head nobody approved. While auto-merge stays enabled on a head other +than the current one, `auto-merge:moved` prints once per head, with the account +that enabled it as the actor and `/commits/` naming the head it was +enabled on, read as `quest status` reads it. + Review-thread replies and reactions never reach the PR timeline, so they are read separately. A reply prints as `replied`; a thread's first comment is part of a review and prints as `reviewed`. Reactions on the PR body or a review @@ -404,7 +412,7 @@ comes last, and a missing value prints as `-`: | --- | --- | | `url`, `state`, `draft`, `head` | The PR URL; `open`, `closed`, or `merged`; `true` or `false`; the head SHA. | | `mergeable`, `merge-state`, `review-decision` | GitHub's values, lowercased. | -| `auto-merge` | Method, who enabled it, and when. | +| `auto-merge` | Method, who enabled it, when, and the head it was enabled on. | | `merge-queue` | Queue state, position, and when it was enqueued. | | `landing-stopped` | The reason the latest merge-queue entry, auto-merge, or [`quest land`](#land-a-batch-with-quest-land) queue entry ended (such as `merge_conflict`, `failed_checks`, `manual`, `manually_disabled`, or `merged`), who ended it, and when. | | `checks` | The head's rollup state and `passed=`, `failing=`, `pending=` counts. | @@ -427,6 +435,15 @@ same facts as one object with the fields `url`, `state`, `draft`, `head`, as in `quest wait`. Any PR state exits 0, invalid arguments exit 2, and other failures exit 1. +GitHub keeps auto-merge enabled across a writer's push without saying which head +it was enabled on, so the `auto-merge` line reads it from the auto-merge commit +body's `Quest-Land-Head` pin when [`quest land`](#land-a-batch-with-quest-land) +queued the PR. Otherwise it is the last push to the head branch before +auto-merge was enabled, from the head repository's activity; it prints `-` +until GitHub lists the current head's push, which takes seconds to minutes, and +when a push shares the second auto-merge was enabled. A head there that differs +from `head` means a push followed auto-merge: disable it, or land the new head. + ## Own a pull request Before a session pushes to an existing PR, it takes the PR, so two sessions @@ -455,9 +472,11 @@ at once, the first label added keeps the PR and the other backs off. before swapping labels, so the token you passed stays valid. If a step fails after a swap, the error names the new token. 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 +when the base has one, or merges at once when nothing is left to wait for. +`--squash` or `--rebase` picks the method, and it refuses one the repository +disallows; otherwise it uses the only method the repository allows, or else a +merge commit, and refuses when merge commits are off and more than one method +remains. 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. On an ungated base with auto-merge disabled, it refuses before changing labels and keeps the caller’s hold token valid. Follow the repository’s own landing @@ -469,7 +488,8 @@ 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. `quest:merge` stays on while the PR is queued or auto-merging; once the PR leaves the queue unmerged, the next command removes -it. The commands accept the same PR argument and authentication as +it, and a direct merge removes it once GitHub reports the PR merged. GitHub +keeps auto-merge across a writer's push, so disable it before pushing to the PR. The commands accept the same PR argument and authentication as `quest wait`, and exit 0 when done, 3 when refused, 2 for invalid arguments, and 1 for other failures. diff --git a/assets/skills/iterate.md b/assets/skills/iterate.md index b93c9532..5ef923a1 100644 --- a/assets/skills/iterate.md +++ b/assets/skills/iterate.md @@ -24,6 +24,7 @@ Address the automated review findings already posted, without waiting on optiona One round only; never request a review, including by toggling a ready PR to draft and back. Before pushing to a PR that is ready for review, review the new head locally as the guide's Reviews section describes. +If `quest status ` shows auto-merge enabled, disable it with `gh pr merge --disable-auto` before any push: GitHub keeps it across a writer's push and would land the new head unreviewed. Push your changes to the PR and update its summary. Summarize the changes made. diff --git a/assets/skills/merge.md b/assets/skills/merge.md index 8df3949f..acc98fa0 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -27,11 +27,11 @@ Before queueing, fetch the PR's base branch and its exact head, check out the he A finding means the PR leaves its quest inaccurate: recommend `/quest-iterate`. 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. +It uses the only merge method the repository allows, or else 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. -The `quest:merge` label stays on while the PR is queued or auto-merging. +The `quest:merge` label stays on while the PR is queued or auto-merging, and comes off once a direct merge completes. 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/helpers/README.md b/quest/m0/helpers/README.md index dbf75eed..ef879737 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -70,7 +70,6 @@ commands from Sep-Oct. Session counts in each child come from that window. ## Required -- [quest merge --land gaps](/quest/m0/helpers/merge-land.md) - squash on squash-only repositories, PR URLs outside a checkout, labels released after direct merges, and a warning when auto-merge outlives its head - [Visible merge-driver fallback](/quest/m0/helpers/merge-driver.md) - a merge without `quest` on PATH says so, and this repository's dev shell provides one - [Review rules](/quest/m0/helpers/review-rules.md) - a head whose diff matches the reviewed diff keeps its review - [Plan delegated follow-ups](/quest/m0/helpers/follow-up-plan.md) - a caller plans a background merge's follow-ups without asking diff --git a/quest/m0/helpers/merge-land.md b/quest/m0/helpers/merge-land.md deleted file mode 100644 index 3f4357e3..00000000 --- a/quest/m0/helpers/merge-land.md +++ /dev/null @@ -1,42 +0,0 @@ -# [M] Close quest merge --land's gaps - -## Goal - -`quest merge --land` lands a PR without per-repository workarounds, and -auto-merge never merges a head nobody landed. On a repository that allows only -squash it squashes without `--squash`. It works from outside a checkout when -given a PR URL. With auto-merge disabled, it refuses unless the user explicitly -requests `--direct`, preserving the repository’s own landing queue. A direct merge releases its `quest:merge` label. When -auto-merge is left on for a head other than the one landed, `quest status` -and `quest wait` say so. - -## Plan - -Implemented: the 2026-10-28 decision to refuse an ungated landing when repository -auto-merge is disabled, unless `--direct` was explicitly requested. Follow the -repository’s own landing instructions on refusal. The remaining gaps follow. - -Decided 2026-10-28 (user, /quest-plan of the follow-ups from a 25-PR /quest-complete run). Merge agents hit each of these repeatedly: - -- **Merge method.** With merge commits disabled, a bare `--land` tried a merge - commit and failed. Read the repository's allowed methods. Use the only one - allowed, or the one the flag names, and refuse a named method the repository - disallows. -- **Outside a checkout.** `quest merge 237` run from `/tmp` failed on - `git -C . remote get-url origin`, while the merge skill says to run a - `quest` the PR can't modify from outside the head checkout. Resolve the - repository from a full PR URL. A bare number keeps needing a checkout or - `--root`, and its error says so. -- **Direct merges keep their label.** The label is supposed to stay only while - the PR is queued or auto-merging, but it stayed after direct merges on - #253, #254, #257 and #259. Release it once the merge is confirmed. -- **Auto-merge carry-over.** #256's agent enabled squash auto-merge on a - reviewed head, then pushed a base merge, and GitHub merged the new head - before it was reviewed or landed. GitHub keeps auto-merge across pushes by - writers. - - The merge skill says to disable auto-merge before any push to a PR that - has it. - - `quest status` reports the head auto-merge was enabled on next to the - current one. `quest wait` prints an event when they differ. - - Rejected: `quest` disabling auto-merge itself, since it doesn't see the - push. diff --git a/src/cli/github.ts b/src/cli/github.ts index a4d5b275..621ed82b 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -38,8 +38,14 @@ export type ObjectValue = Record; /** Accept a GitHub PR URL, or a number in the origin repository. */ export function pullRequest(value: string, root: string): PullRequest { if (/^[1-9]\d*$/.test(value)) { - const { repository } = remoteRepository(root, "origin"); - if (!repository) throw new Error("origin must name a github.com repository; pass a GitHub PR URL instead"); + const outside = `a bare PR number is read from origin in ${root}; pass the full GitHub PR URL, or run inside a checkout or name one with --root`; + let repository: Repository | null; + try { + repository = remoteRepository(root, "origin").repository; + } catch { + throw new GitHubInputError(`${root} is not a checkout with an origin remote: ${outside}`); + } + if (!repository) throw new GitHubInputError(`origin does not name a github.com repository: ${outside}`); return { ...repository, number: Number(value) }; } const match = /^https:\/\/github\.com\/([\w.-]+)\/([\w.-]+)\/pull\/([1-9]\d*)\/?(?:[?#].*)?$/.exec(value); diff --git a/src/cli/hold.ts b/src/cli/hold.ts index 1fe9fb0e..a72d7e80 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -163,18 +163,49 @@ export async function finish(pr: PullRequest, token: string, deps: GitHubDepende }); } +/** The repository setting that allows each merge method. */ +const ALLOWS: Record = { + merge: "allow_merge_commit", + squash: "allow_squash_merge", + rebase: "allow_rebase_merge", +}; + +/** + * The method an ungated landing uses: the one `named`, which the repository must allow, else the + * only one it allows, else a merge commit. Settings hidden from the token leave GitHub to refuse. + */ +function mergeMethod(repository: ObjectValue, named: Method | undefined, url: string): Method { + const methods = Object.keys(ALLOWS) as Method[]; + if (methods.some((method) => typeof repository[ALLOWS[method]] !== "boolean")) return named ?? "merge"; + const allowed = methods.filter((method) => repository[ALLOWS[method]] === true); + if (named !== undefined) { + if (allowed.includes(named)) return named; + throw new Refusal( + `${url} repository does not allow ${named} merges; it allows ${allowed.join(", ") || "none"}`, + ); + } + if (allowed.length === 1) return allowed[0]; + if (allowed.includes("merge")) return "merge"; + if (allowed.length === 0) throw new Refusal(`${url} repository allows no merge method`); + throw new Refusal( + `${url} repository does not allow merge commits; pass --${allowed.join(" or --")}, as its landing instructions in CONTRIBUTING.md or AGENTS.md name`, + ); +} + /** * Take `pr` for merge, then land `sha`, which must still be its head: 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. + * A direct merge releases the hold once GitHub reports the PR merged. * A base that requires the `quest land` check queues it for `quest land` instead: * squash auto-merge whose commit body pins `sha`. + * `named` is the method the caller asked for; otherwise {@link mergeMethod} picks one. */ export async function land( pr: PullRequest, token: string, sha: string, - method: Method, + named: Method | undefined, deps: GitHubDependencies, direct = false, ): Promise { @@ -187,15 +218,19 @@ export async function land( // Pre-check before any label changes, preserving the caller's token on refusal. let gated = false; let checkedBase = ""; + // A merge queue applies its own method, so only an ungated, unqueued landing picks one. + let method: Method = named ?? "merge"; const { hold, snapshot } = await acquire(labels, "merge", token, async (snapshot) => { current(snapshot); checkedBase = snapshot.base; gated = (await requirements(labels.github, pr, snapshot.base)).checks.some( (check) => check.name === LAND_CHECK, ); + if (gated || snapshot.queue) return; + const repository = object(await (await labels.github.request(repositoryPath(pr))).json()); + method = mergeMethod(repository, named, snapshot.url); // A merge queue is the repository's own gate, so it decides; otherwise auto-merge must be on. - if (!gated && !direct && !snapshot.queue) { - const repository = object(await (await labels.github.request(repositoryPath(pr))).json()); + if (!direct) { // GitHub shows the merge settings only to writers; fail closed, with a next step. if (typeof repository.allow_auto_merge !== "boolean") throw new Refusal( @@ -214,7 +249,8 @@ export async function land( } current(snapshot); if (gated) { - if (method !== "squash") deps.warn(`${snapshot.url} lands through quest land, which squashes`); + if (named !== undefined && named !== "squash") + deps.warn(`${snapshot.url} lands through quest land, which squashes`); await enqueue(labels.github, snapshot.id, snapshot.head); return; } @@ -232,7 +268,7 @@ 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") { + if (snapshot.queue && named !== undefined && named !== "merge") { deps.warn(`${snapshot.url} lands through a merge queue, which applies its own merge method`); } const submit = (mutation: "mergePullRequest" | "enablePullRequestAutoMerge") => @@ -242,8 +278,18 @@ export async function land( }`, { id: snapshot.id, sha: snapshot.head, method: method.toUpperCase() }, ); - if (immediateMerge(snapshot.mergeState, snapshot.queue)) { + /** Merge at once, then release the hold: only a queued or auto-merging PR keeps it. */ + const merge = async () => { await submit("mergePullRequest"); + const merged = await labels.read(); + if (merged.state !== "MERGED") { + deps.warn(`${merged.url} is ${merged.state.toLowerCase()} after merging, so ${hold.label} stays on`); + return; + } + if (heldToken(merged, hold.label) === hold.token) await labels.remove(hold.label); + }; + if (immediateMerge(snapshot.mergeState, snapshot.queue)) { + await merge(); return; } try { @@ -261,7 +307,7 @@ export async function land( (await requirements(labels.github, pr, fresh.base)).checks.some((check) => check.name === LAND_CHECK) ) throw error; - await submit("mergePullRequest"); + await merge(); } }); } diff --git a/src/cli/land.ts b/src/cli/land.ts index fc19f542..f1043c6e 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -59,7 +59,7 @@ export async function enqueue(github: GitHub, id: string, head: string): Promise } /** The head an auto-merge commit body pins, if any. */ -function pinned(body: string): string | null { +export function pinned(body: string): string | null { return new RegExp(`^${PIN}: ([0-9a-f]{40})$`, "m").exec(body)?.[1] ?? null; } diff --git a/src/cli/main.ts b/src/cli/main.ts index ee0a587e..d56d49c1 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -357,7 +357,7 @@ export function main(argv: string[], out: Output): number | Promise { .summary("Wait for pull request activity on GitHub") .description( "Wait for a review, comment or comment edit, review-thread reply, reaction to the PR or a review, push, merge, close, " + - "merge-queue removal, disabled auto-merge, label change, or CI result. Polls every 5 minutes, and once more 30 seconds before the timeout; rate limits and longer GitHub poll intervals delay requests. No timeout by default.\n\n" + + "merge-queue removal, disabled auto-merge or auto-merge left on a moved head, label change, or CI result. Polls every 5 minutes, and once more 30 seconds before the timeout; rate limits and longer GitHub poll intervals delay requests. No timeout by default.\n\n" + "Prints one plain line per event: timestamp (or commit SHA), type, actor, URL; the last line adds a resume cursor. " + "Reactions print as reacted:, such as reacted:thumbs_up, and label changes as labeled: or unlabeled:. " + "The head's first failing required check prints as checks:failed, and every required check passing as checks:passed, " + @@ -365,6 +365,9 @@ export function main(argv: string[], out: Output): number | Promise { "Results count as quest status counts them, so neutral and skipped results pass. " + "checks:passed waits until no check is pending, no GitHub Actions run is unfinished, and every check and workflow the base branch names is present, then names the last required check that passed. " + "Optional failures print separately as check:failed; a base branch requiring no checks requires them all. " + + "When auto-merge stays enabled on a head other than the current one, which GitHub allows across a writer's push, " + + "auto-merge:moved prints once per head, with the account that enabled it as the actor and a URL naming the head it " + + "was enabled on, which it reads as quest status does. " + "Pass the final field of the last line to --since to resume without replaying events. " + "On timeout, the only line is a resume point for --since (a cursor, or the given --since or head SHA if setup did not finish), so a retry with it still reports activity from the first wait.\n\n" + "Uses GH_TOKEN, GITHUB_TOKEN, or gh auth token. Label changes, replies, and reactions by that account are ignored. " + @@ -397,7 +400,10 @@ export function main(argv: string[], out: Output): number | Promise { "in one call. It reports facts; it does not decide whether the PR is ready or whose review counts.\n\n" + "Plain output prints one record per line, its type first and any free text (a check name or path) last: " + "url, state, draft, head, mergeable, merge-state, review-decision, auto-merge, merge-queue, " + - "landing-stopped, and checks; landing-stopped is the latest exit from the merge queue, auto-merge, or " + + "landing-stopped, and checks; auto-merge gives its method, who enabled it and when, and the head it was " + + "enabled on, which differs from head once a push follows, as GitHub keeps auto-merge across a writer's " + + "push (- when it cannot be told: a quest land pin names it, and otherwise the head branch's push " + + "activity, which GitHub lists up to minutes late); landing-stopped is the latest exit from the merge queue, auto-merge, or " + "quest land's queue, with the reason (such as merge_conflict, failed_checks, manual, or manually_disabled), " + "actor, and time. Then " + "a check line per failing or pending check, marked required or optional; review-rounds counts distinct " + @@ -472,11 +478,14 @@ export function main(argv: string[], out: Output): number | Promise { "merge", "Take a PR to fix it until it lands", "--land enables auto-merge bound to that full head SHA, which enters the base's merge queue when it has " + - "one, or merges at once when nothing is left to wait for. It refuses 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 " + + "one, or merges at once when nothing is left to wait for. It refuses once the head has moved. It uses the " + + "method --squash or --rebase names, refusing one the repository disallows; otherwise the only method the " + + "repository allows, or else a merge commit. A merge queue applies its own. 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. The label stays on while the PR is queued or auto-merging, " + - "until it merges. On an ungated base with auto-merge disabled, it refuses unless --direct is passed. " + + "until it merges, and comes off once a direct merge completes. GitHub keeps auto-merge across a writer's " + + "push, so disable it before pushing to the PR; quest status and quest wait report a head that moved since " + + "auto-merge was enabled. On an ungated base with auto-merge disabled, it refuses unless --direct is passed. " + "Follow the repository landing instructions in CONTRIBUTING.md or AGENTS.md; never retry with --direct unless the user asks.", ) .option("--land ", "Land this full 40-character head SHA; needs the token") @@ -497,7 +506,7 @@ export function main(argv: string[], out: Output): number | Promise { } if (token === undefined) throw new UsageError("--land needs the hold token"); if (!/^[\da-f]{40}$/i.test(options.land)) throw new UsageError("--land needs a full 40-character SHA"); - const method = options.squash ? "squash" : options.rebase ? "rebase" : "merge"; + const method = options.squash ? "squash" : options.rebase ? "rebase" : undefined; printToken(land(pr, token, options.land, method, gitHubDependencies(warn), options.direct)); }, ); diff --git a/src/cli/status.ts b/src/cli/status.ts index 74a244db..dd7dd85b 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -4,12 +4,13 @@ import { array, GitHub, type GitHubDependencies, + link, object, type ObjectValue, type PullRequest, string, } from "./github"; -import { EJECTED } from "./land"; +import { EJECTED, pinned } from "./land"; import { readHoldStatus, type HoldFact, type HoldReport } from "./hold"; /** @@ -21,8 +22,9 @@ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, $moreChecks: Boolean!, $moreReviews: Boolean!, $moreThreads: Boolean!, $moreComments: Boolean!) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { - url state isDraft headRefOid mergeable mergeStateStatus reviewDecision - autoMergeRequest { enabledAt mergeMethod enabledBy { login } } + url state isDraft headRefOid headRefName mergeable mergeStateStatus reviewDecision + headRepository { nameWithOwner } + autoMergeRequest { enabledAt mergeMethod commitBody enabledBy { login } } mergeQueueEntry { state position enqueuedAt } timelineItems(last: 100, itemTypes: [REMOVED_FROM_MERGE_QUEUE_EVENT, AUTO_MERGE_DISABLED_EVENT, ISSUE_COMMENT]) { nodes { __typename @@ -110,7 +112,8 @@ export interface Status { mergeable: string; mergeState: string; reviewDecision: string | null; - autoMerge: { method: string; enabledBy: string; enabledAt: string } | null; + /** `head` is the head it was enabled on, or null when that cannot be told; see {@link enabledHead}. */ + autoMerge: { method: string; enabledBy: string; enabledAt: string; head: string | null } | null; mergeQueue: { state: string; position: number; enqueuedAt: string } | null; /** The latest exit from the merge queue, auto-merge, or `quest land`'s queue, and the reason for it. */ landingStopped: { reason: string; actor: string; at: string } | null; @@ -186,11 +189,41 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie const ownership = await readHoldStatus(pr, deps); if (ownership.head !== first!.headRefOid) throw new Error("the PR head changed while reading its holds; rerun quest status"); - return { - ...summarize(first!, rollup, contexts, reviewNodes, threadNodes, commentNodes), - holds: ownership.holds, - starterHold: ownership.starter, - }; + const status = summarize(first!, rollup, contexts, reviewNodes, threadNodes, commentNodes); + if (status.autoMerge !== null) status.autoMerge.head = await enabledHead(github, first!); + return { ...status, holds: ownership.holds, starterHold: ownership.starter }; +} + +/** + * The head a PR node's auto-merge request was enabled on, or null when that cannot be told. GitHub + * keeps auto-merge across a writer's push but does not say which head it was enabled on. A + * `quest land` pin names it; otherwise it is the last push to the head branch before auto-merge + * was enabled, read from the head repository's activity once that lists the current head's push. + * A push in the same second as enabling is ambiguous. The node needs `headRefOid`, `headRefName`, + * `headRepository { nameWithOwner }`, and `autoMergeRequest { enabledAt commitBody }`. + */ +export async function enabledHead(github: GitHub, pull: ObjectValue): Promise { + if (pull.autoMergeRequest == null) return null; + const request = object(pull.autoMergeRequest); + const pin = pinned(typeof request.commitBody === "string" ? request.commitBody : ""); + if (pin !== null || pull.headRepository == null) return pin; + const enabled = Date.parse(string(request.enabledAt)); + const head = string(pull.headRefOid); + const pushes: { sha: string; time: number }[] = []; + let path: string | undefined = + `/repos/${string(object(pull.headRepository).nameWithOwner)}/activity?ref=${encodeURIComponent(string(pull.headRefName))}&per_page=100`; + // Newest first: read until a push before enabling, which is the head auto-merge was enabled on. + while (path && !pushes.some((push) => push.time < enabled)) { + const response = await github.request(path, { allow: [404] }); + if (response.status === 404) return null; + for (const row of array(await response.json())) + if (["push", "force_push", "branch_creation"].includes(string(row.activity_type))) + pushes.push({ sha: string(row.after), time: Date.parse(string(row.timestamp)) }); + path = link(response, "next"); + } + // GitHub lists a push seconds to minutes late; until the current head's is listed, an earlier one may be missing. + if (!pushes.some((push) => push.sha === head) || pushes.some((push) => push.time === enabled)) return null; + return pushes.find((push) => push.time < enabled)?.sha ?? null; } function summarize( @@ -293,6 +326,7 @@ function summarize( method: lower(autoMerge.mergeMethod), enabledBy: actor(autoMerge.enabledBy), enabledAt: string(autoMerge.enabledAt), + head: null, }, mergeQueue: queue === null @@ -366,7 +400,7 @@ export function statusLines(status: Status): string[] { `review-decision ${dash(status.reviewDecision)}`, status.autoMerge === null ? "auto-merge -" - : `auto-merge ${status.autoMerge.method} ${status.autoMerge.enabledBy} ${status.autoMerge.enabledAt}`, + : `auto-merge ${status.autoMerge.method} ${status.autoMerge.enabledBy} ${status.autoMerge.enabledAt} ${dash(status.autoMerge.head)}`, status.mergeQueue === null ? "merge-queue -" : `merge-queue ${status.mergeQueue.state} ${status.mergeQueue.position} ${status.mergeQueue.enqueuedAt}`, diff --git a/src/cli/wait.ts b/src/cli/wait.ts index b95cdc07..20cadc1e 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -15,7 +15,7 @@ import { string, } from "./github"; import { LAND_CHECK } from "./land"; -import { classifyCheck } from "./status"; +import { classifyCheck, enabledHead } from "./status"; const POLL = 5 * 60_000; /** @@ -51,6 +51,9 @@ const CHECKS = `query($owner: String!, $repo: String!, $number: Int!, $after: St pullRequest(number: $number) { baseRefName headRefOid + headRefName + headRepository { nameWithOwner } + autoMergeRequest { enabledAt commitBody enabledBy { login } } commits(last: 1) { nodes { commit { oid statusCheckRollup { contexts(first: 100, after: $after) { pageInfo { hasNextPage endCursor } @@ -339,7 +342,9 @@ async function watch( return { viewer, events }; }; /** GraphQL says which checks the base branch requires, through branch protection or rulesets. */ - const checks = async (): Promise<{ head: string; base: string; checks: Check[] } | undefined> => { + const checks = async (): Promise< + { head: string; base: string; checks: Check[]; pull: ObjectValue } | undefined + > => { const result: Check[] = []; let head: string | undefined; let after: string | null = null; @@ -357,7 +362,7 @@ async function watch( const page = object(contexts.pageInfo); after = page.hasNextPage === true ? string(page.endCursor) : null; } while (after !== null); - return { head: head!, base: string(pull.baseRefName), checks: result }; + return { head: head!, base: string(pull.baseRefName), checks: result, pull }; }; /** * What the base branch requires, read once per wait. Only a requirement says whether a missing @@ -377,6 +382,24 @@ async function watch( return needs; }; + /** The head each auto-merge request was enabled on, by when it was enabled, once that can be told. */ + const enabledHeads = new Map(); + /** Auto-merge left on for a head other than `head`, reported once per head and request. */ + const movedAutoMerge = async (head: string, pull: ObjectValue): Promise => { + if (pull.autoMergeRequest == null) return []; + const request = object(pull.autoMergeRequest); + const enabledAt = string(request.enabledAt); + const id = `${head}:auto-merge:${enabledAt}`; + if (cursor.checks.reported.includes(id)) return []; + const enabled = enabledHeads.get(enabledAt) ?? (await enabledHead(github, pull)); + if (enabled === null) return []; + enabledHeads.set(enabledAt, enabled); + if (enabled === head) return []; + const point = new Date(deps.now()).toISOString(); + const url = `${prUrl}/commits/${enabled}`; + return [{ point, time: deps.now(), type: "auto-merge:moved", actor: actor(request.enabledBy), url, id }]; + }; + const timeline = pager(`${base}/issues/${pr.number}/timeline?per_page=${PAGE}`); // `since` excludes its own instant and filters by update time, which is never before // creation; start a second early so unseen replies at the mark's second are still read. @@ -454,7 +477,10 @@ async function watch( // Check results are state rather than a log, so each poll reads them afresh. const head = await checks(); const reached = head - ? checkActivities(head.head, head.checks, await required(head.base), cursor.checks, prUrl) + ? [ + ...checkActivities(head.head, head.checks, await required(head.base), cursor.checks, prUrl), + ...(await movedAutoMerge(head.head, head.pull)), + ] : []; if (reached.length || SOURCES.some((source) => found[source].length)) { diff --git a/test/hold.test.ts b/test/hold.test.ts index 39af01f3..58a6371b 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -28,6 +28,10 @@ class FakePull { mergeState = "BLOCKED"; queue = false; allowAutoMerge: boolean | null = true; + /** The repository's merge method settings, hidden from the token when null. */ + methods: Record | null = null; + /** GitHub accepts a merge but still reports the PR open. */ + mergeLeavesOpen = false; baseRef = "main"; /** Runs after each pull request read, to change it between reads. */ afterRead?: () => void; @@ -76,7 +80,10 @@ class FakePull { const body = init.body ? (JSON.parse(String(init.body)) as ObjectValue) : {}; const json = (status: number, value: unknown) => new Response(JSON.stringify(value), { status }); if (url.pathname === "/repos/example/project") - return json(200, this.allowAutoMerge === null ? {} : { allow_auto_merge: this.allowAutoMerge }); + return json(200, { + ...(this.allowAutoMerge === null ? {} : { allow_auto_merge: this.allowAutoMerge }), + ...this.methods, + }); if (url.pathname === "/graphql") return json(200, this.graphql(String(body.query), body.variables as ObjectValue)); if (url.pathname === "/repos/example/project/issues/7/labels" && method === "POST") { @@ -126,7 +133,7 @@ class FakePull { const body = variables.body ? ` ${String(variables.body)}` : ""; this.writes.push(method ? `${name} ${variables.sha} ${method}${body}` : name); if (name === "markPullRequestReadyForReview") this.draft = false; - if (name === "mergePullRequest") this.state = "MERGED"; + if (name === "mergePullRequest" && !this.mergeLeavesOpen) this.state = "MERGED"; if (name === "enablePullRequestAutoMerge") { const reason = this.beforeAutoMerge?.(); if (reason) return { errors: [{ message: reason }] }; @@ -345,7 +352,7 @@ describe("merge --land", () => { stderr: () => undefined, }), ).toBe(0); - expect(pull.writes.at(-1)).toBe(`mergePullRequest ${HEAD} MERGE`); + expect(pull.writes.slice(-2)).toEqual([`mergePullRequest ${HEAD} MERGE`, "remove quest:merge"]); } finally { vi.unstubAllEnvs(); vi.unstubAllGlobals(); @@ -385,16 +392,102 @@ describe("merge --land", () => { expect(pull.labels).toEqual(["quest:merge"]); }); - test("merges at once when nothing is left to wait for, with --squash or --rebase", async () => { + test("merges at once when nothing is left to wait for, with --squash or --rebase, releasing the hold", async () => { for (const method of ["squash", "rebase"] as const) { const pull = new FakePull(); pull.mergeState = "CLEAN"; const { token } = await take(PR, "merge", undefined, pull.deps()); await land(PR, token, HEAD, method, pull.deps()); - expect(pull.writes.at(-1)).toBe(`mergePullRequest ${HEAD} ${method.toUpperCase()}`); + expect(pull.writes.slice(-2)).toEqual([ + `mergePullRequest ${HEAD} ${method.toUpperCase()}`, + "remove quest:merge", + ]); + expect(pull.labels).toEqual([]); } }); + test("keeps the hold when GitHub does not report a direct merge merged", async () => { + const pull = new FakePull(); + pull.mergeState = "CLEAN"; + pull.mergeLeavesOpen = true; + const warnings: string[] = []; + const { token } = await take(PR, "merge", undefined, pull.deps()); + await land(PR, token, HEAD, "merge", pull.deps(warnings)); + expect(pull.writes.at(-1)).toBe(`mergePullRequest ${HEAD} MERGE`); + expect(pull.labels).toEqual(["quest:merge"]); + expect(warnings).toEqual([`${URL_} is open after merging, so quest:merge stays on`]); + }); + + describe("picks the repository's merge method", () => { + const allowing = (...methods: string[]) => { + const pull = new FakePull(); + pull.methods = { + allow_merge_commit: methods.includes("merge"), + allow_squash_merge: methods.includes("squash"), + allow_rebase_merge: methods.includes("rebase"), + }; + return pull; + }; + + test("uses the only method allowed without a flag", async () => { + for (const method of ["squash", "rebase"]) { + const pull = allowing(method); + const { token } = await take(PR, "merge", undefined, pull.deps()); + await land(PR, token, HEAD, undefined, pull.deps()); + expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} ${method.toUpperCase()}`); + } + }); + + test("keeps a merge commit by default while one is allowed", async () => { + const pull = allowing("merge", "squash"); + const { token } = await take(PR, "merge", undefined, pull.deps()); + await land(PR, token, HEAD, undefined, pull.deps()); + expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} MERGE`); + }); + + test("also applies with --direct", async () => { + const pull = allowing("squash"); + pull.allowAutoMerge = false; + pull.mergeState = "CLEAN"; + const { token } = await take(PR, "merge", undefined, pull.deps()); + await land(PR, token, HEAD, undefined, pull.deps(), true); + expect(pull.writes.slice(-2)).toEqual([`mergePullRequest ${HEAD} SQUASH`, "remove quest:merge"]); + }); + + test("refuses, before changing labels, a named method the repository disallows or a choice it leaves open", async () => { + const cases: [string[], "merge" | "squash" | "rebase" | undefined, string][] = [ + [["squash"], "merge", "does not allow merge merges; it allows squash"], + [["merge", "squash"], "rebase", "does not allow rebase merges; it allows merge, squash"], + [["squash", "rebase"], undefined, "pass --squash or --rebase"], + [[], undefined, "allows no merge method"], + ]; + for (const [methods, named, refusal] of cases) { + const pull = allowing(...methods); + const { token } = await take(PR, "iterate", undefined, pull.deps()); + const writes = [...pull.writes]; + expect(await refused(land(PR, token, HEAD, named, pull.deps()))).toContain(refusal); + expect(pull.writes).toEqual(writes); + expect(pull.labels).toEqual(["quest:iterate"]); + } + }); + + test("leaves a gated base or a merge queue to its own method", async () => { + const gated = allowing("merge"); + gated.required = ["quest land"]; + const { token } = await take(PR, "merge", undefined, gated.deps()); + await land(PR, token, HEAD, undefined, gated.deps()); + expect(gated.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} SQUASH Quest-Land-Head: ${HEAD}`); + + const queued = allowing("squash"); + queued.queue = true; + const warnings: string[] = []; + const queuedHold = await take(PR, "merge", undefined, queued.deps()); + await land(PR, queuedHold.token, HEAD, undefined, queued.deps(warnings)); + expect(queued.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} MERGE`); + expect(warnings).toEqual([]); + }); + }); + test("enters the merge queue through auto-merge even when clean", async () => { const pull = new FakePull(); pull.mergeState = "CLEAN"; @@ -453,11 +546,13 @@ describe("merge --land", () => { 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([ + expect(pull.writes.slice(-3)).toEqual([ `enablePullRequestAutoMerge ${HEAD} SQUASH`, `mergePullRequest ${HEAD} SQUASH`, + "remove quest:merge", ]); - expect(pull.reads - before).toBe(2); // The usual hold read, then one reread after refusal. + // The usual hold read, one reread after refusal, and one to confirm the merge. + expect(pull.reads - before).toBe(3); expect(pull.state).toBe("MERGED"); }, ); diff --git a/test/status.test.ts b/test/status.test.ts index cb79841e..f7846eb3 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -1,4 +1,4 @@ -import { expect, test } from "vitest"; +import { describe, expect, test } from "vitest"; import type { GitHubDependencies } from "../src/cli/github"; import { main } from "../src/cli/main"; @@ -111,6 +111,8 @@ function setup( now?: number; holdFields?: Node; eventPages?: { rows: Node[]; next?: string }[]; + /** Pages of the head repository's activity, newest first. */ + activity?: { rows: Node[]; next?: string }[]; } = {}, ) { const queries: Record[] = []; @@ -118,6 +120,8 @@ function setup( const eventRequests: string[] = []; let last: ReturnType | undefined; const eventPages = [...(options.eventPages ?? [])]; + const activityPages = [...(options.activity ?? [])]; + const activityRequests: string[] = []; const deps: GitHubDependencies = { token: "test-token", now: () => options.now ?? 0, @@ -125,6 +129,12 @@ function setup( warn: () => {}, fetch: async (url, init) => { const path = new URL(String(url)); + if (path.pathname === "/repos/fork/project/activity") { + activityRequests.push(`${path.pathname}${path.search}`); + const page = activityPages.shift(); + if (!page) return Response.json({ message: "Not Found" }, { status: 404 }); + return Response.json(page.rows, { headers: page.next ? { Link: `<${page.next}>; rel="next"` } : {} }); + } if (path.pathname !== "/graphql") { expect(path.pathname).toBe("/repos/example/project/issues/1/events"); eventRequests.push(String(url)); @@ -157,7 +167,7 @@ function setup( return Response.json(body); }, }; - return { deps, queries, holdQueries, eventRequests }; + return { deps, queries, holdQueries, eventRequests, activityRequests }; } test("reports checks by requiredness, each reviewer's latest review against the head, and open threads", async () => { @@ -246,7 +256,7 @@ test("a queued PR with auto-merge and no checks reports both, and JSON is determ "mergeable mergeable", "merge-state blocked", "review-decision -", - "auto-merge squash kixelated 2026-10-07T12:00:00Z", + "auto-merge squash kixelated 2026-10-07T12:00:00Z -", "merge-queue awaiting_checks 2 2026-10-07T12:01:00Z", "landing-stopped -", "checks - passed=0 failing=0 pending=0", @@ -255,6 +265,74 @@ test("a queued PR with auto-merge and no checks reports both, and JSON is determ expect(status.mergeQueue).toEqual({ state: "awaiting_checks", position: 2, enqueuedAt: "2026-10-07T12:01:00Z" }); }); +describe("the head auto-merge was enabled on", () => { + const ENABLED = "2026-10-07T12:00:00Z"; + const push = (after: string, timestamp: string, activity_type = "push") => ({ after, timestamp, activity_type }); + const enabled = async ( + request: Node, + activity: { rows: Node[]; next?: string }[] = [], + head = B, + ): Promise<{ line: string | undefined; requests: string[] }> => { + const body = pull({ + head, + checks: null, + reviews: page([]), + threads: page([]), + headRefName: "feature/x", + headRepository: { nameWithOwner: "fork/project" }, + autoMergeRequest: { + enabledAt: ENABLED, + mergeMethod: "MERGE", + enabledBy: { login: "kixelated" }, + ...request, + }, + }); + const fixture = setup([body], { activity }); + const status = await pullRequestStatus(pr, fixture.deps); + return { + line: statusLines(status).find((line) => line.startsWith("auto-merge ")), + requests: fixture.activityRequests, + }; + }; + + test("is the head a quest land pin names, without reading activity", async () => { + const result = await enabled({ commitBody: `Quest-Land-Head: ${A}` }); + expect(result.line).toBe(`auto-merge merge kixelated ${ENABLED} ${A}`); + expect(result.requests).toEqual([]); + }); + + test("is otherwise the last push before it was enabled, paging the head branch's activity", async () => { + const result = await enabled({ commitBody: null }, [ + { + rows: [push(B, "2026-10-07T12:05:00Z")], + next: "https://api.github.com/repos/fork/project/activity?page=2", + }, + { rows: [push(A, "2026-10-07T11:59:00Z", "force_push"), push("c".repeat(40), "2026-10-07T11:00:00Z")] }, + ]); + expect(result.line).toBe(`auto-merge merge kixelated ${ENABLED} ${A}`); + expect(result.requests).toEqual([ + "/repos/fork/project/activity?ref=feature%2Fx&per_page=100", + "/repos/fork/project/activity?page=2", + ]); + }); + + test("matches the head when no push followed", async () => { + const result = await enabled({ commitBody: "" }, [{ rows: [push(B, "2026-10-07T11:59:59Z")] }]); + expect(result.line).toBe(`auto-merge merge kixelated ${ENABLED} ${B}`); + }); + + test("is unknown until GitHub lists the current head's push, or when a push shares the second", async () => { + expect((await enabled({}, [{ rows: [push(A, "2026-10-07T11:59:00Z")] }])).line).toBe( + `auto-merge merge kixelated ${ENABLED} -`, + ); + expect((await enabled({}, [{ rows: [push(B, ENABLED), push(A, "2026-10-07T11:59:00Z")] }])).line).toBe( + `auto-merge merge kixelated ${ENABLED} -`, + ); + // A head repository the token cannot read. + expect((await enabled({}, [])).line).toBe(`auto-merge merge kixelated ${ENABLED} -`); + }); +}); + test("reports why the latest landing stopped, from a merge queue or auto-merge", async () => { const stopped = async (node: Node) => { const body = pull({ checks: null, reviews: page([]), threads: page([]), timelineItems: { nodes: [node] } }); diff --git a/test/wait.test.ts b/test/wait.test.ts index 8ae9598a..64cc9c4a 100644 --- a/test/wait.test.ts +++ b/test/wait.test.ts @@ -1,7 +1,11 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + import { expect, test } from "vitest"; import { main } from "../src/cli/main"; -import { type GitHubDependencies, pullRequest } from "../src/cli/github"; +import { type GitHubDependencies, GitHubInputError, pullRequest } from "../src/cli/github"; import { waitForActivity, WaitTimeout } from "../src/cli/wait"; const A = "a".repeat(40); @@ -90,6 +94,7 @@ function checks( head = A, page: { hasNextPage: boolean; endCursor: string | null } = { hasNextPage: false, endCursor: null }, listed = head, + extra: Record = {}, ) { const node = (entry: CheckFixture) => entry.status @@ -120,6 +125,7 @@ function checks( pullRequest: { baseRefName: "main", headRefOid: head, + ...extra, commits: { nodes: [ { @@ -747,6 +753,59 @@ test("PR URL parsing and timeout usage fail before authentication", () => { } }); +test("a bare PR number outside a checkout says to pass the URL instead", () => { + const outside = mkdtempSync(join(tmpdir(), "quest-wait-")); + try { + expect(() => pullRequest("1", outside)).toThrow(GitHubInputError); + expect(() => pullRequest("1", outside)).toThrow("pass the full GitHub PR URL, or run inside a checkout"); + } finally { + rmSync(outside, { recursive: true }); + } +}); + +test("auto-merge left on a moved head wakes the wait once per head, naming the head it was enabled on", async () => { + const pinned = (head: string) => + checks([], head, undefined, head, { + headRefName: "feature/a", + headRepository: { nameWithOwner: "contributor/fork" }, + autoMergeRequest: { + enabledAt: POINT, + commitBody: `Quest-Land-Head: ${A}`, + enabledBy: { login: "kixelated" }, + }, + }); + const first = setup([pull(undefined, B), response([pushed(B)]), response([committed(A), committed(B)])], [A, B], { + checks: [pinned(B)], + }); + const moved = await waitForActivity(pr, {}, first.deps); + expect(moved).toHaveLength(1); + expect(moved[0]).toMatch( + new RegExp( + `^2026-10-07T12:10:00.000Z auto-merge:moved kixelated https://github.com/example/project/pull/1/commits/${A} q1\\.`, + ), + ); + // Resuming reports other activity, not the same moved head again. + const next = setup([response([committed(A), committed(B), reviewed(2, LATEST)])], [A, B], { checks: [pinned(B)] }); + const lines = await waitForActivity(pr, { since: cursor(moved) }, next.deps); + expect(lines).toHaveLength(1); + expect(lines[0]).toMatch(/ reviewed reviewer /); +}); + +test("auto-merge on the current head is not activity", async () => { + const current = () => + checks([], A, undefined, A, { + autoMergeRequest: { + enabledAt: POINT, + commitBody: `Quest-Land-Head: ${A}`, + enabledBy: { login: "kixelated" }, + }, + }); + const fixture = setup([pull(), response([pushed()]), response([committed(A)]), response([committed(A)])], [A], { + checks: [current(), current()], + }); + await expect(waitForActivity(pr, { timeout: 60 }, fixture.deps)).rejects.toBeInstanceOf(WaitTimeout); +}); + test("a push racing initial setup is not swallowed by the baseline", async () => { const fixture = setup([pull(), response([pushed()]), response([committed(A), committed(B)])]); const lines = await waitForActivity(pr, {}, fixture.deps); From f7857aa693dd464a568736bd23f1d19c6e31cee5 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 00:10:18 -0700 Subject: [PATCH 3/6] fix(status): keep status output when the head's push activity is forbidden Co-Authored-By: Claude Opus 5.5 --- src/cli/status.ts | 29 +++++++++++++++++++++++------ src/cli/wait.ts | 8 +++++++- test/status.test.ts | 23 ++++++++++++++++++----- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/cli/status.ts b/src/cli/status.ts index dd7dd85b..2221f7cf 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -3,6 +3,7 @@ import { actor, array, GitHub, + GitHubDeadline, type GitHubDependencies, link, object, @@ -190,7 +191,7 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie if (ownership.head !== first!.headRefOid) throw new Error("the PR head changed while reading its holds; rerun quest status"); const status = summarize(first!, rollup, contexts, reviewNodes, threadNodes, commentNodes); - if (status.autoMerge !== null) status.autoMerge.head = await enabledHead(github, first!); + if (status.autoMerge !== null) status.autoMerge.head = await enabledHead(github, first!, deps.warn); return { ...status, holds: ownership.holds, starterHold: ownership.starter }; } @@ -199,10 +200,15 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie * keeps auto-merge across a writer's push but does not say which head it was enabled on. A * `quest land` pin names it; otherwise it is the last push to the head branch before auto-merge * was enabled, read from the head repository's activity once that lists the current head's push. - * A push in the same second as enabling is ambiguous. The node needs `headRefOid`, `headRefName`, - * `headRepository { nameWithOwner }`, and `autoMergeRequest { enabledAt commitBody }`. + * A push in the same second as enabling is ambiguous. An activity listing the token cannot read + * leaves it unknown, with a warning, rather than failing the caller. The node needs `headRefOid`, + * `headRefName`, `headRepository { nameWithOwner }`, and `autoMergeRequest { enabledAt commitBody }`. */ -export async function enabledHead(github: GitHub, pull: ObjectValue): Promise { +export async function enabledHead( + github: GitHub, + pull: ObjectValue, + warn: (message: string) => void, +): Promise { if (pull.autoMergeRequest == null) return null; const request = object(pull.autoMergeRequest); const pin = pinned(typeof request.commitBody === "string" ? request.commitBody : ""); @@ -210,11 +216,22 @@ export async function enabledHead(github: GitHub, pull: ObjectValue): Promise push.time < enabled)) { - const response = await github.request(path, { allow: [404] }); + let response: Response; + try { + response = await github.request(path, { allow: [404] }); + } catch (error) { + // A deadline is the caller's to handle; a forbidden listing, as for a token without contents access, is not. + if (error instanceof GitHubDeadline || !(error instanceof Error)) throw error; + warn( + `cannot read ${repository}'s push activity, so the head auto-merge was enabled on is unknown: ${error.message}`, + ); + return null; + } if (response.status === 404) return null; for (const row of array(await response.json())) if (["push", "force_push", "branch_creation"].includes(string(row.activity_type))) diff --git a/src/cli/wait.ts b/src/cli/wait.ts index 20cadc1e..308b3934 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -384,6 +384,12 @@ async function watch( /** The head each auto-merge request was enabled on, by when it was enabled, once that can be told. */ const enabledHeads = new Map(); + /** Warnings already printed, so a forbidden activity listing warns once per wait rather than per poll. */ + const warned = new Set(); + const warnOnce = (message: string) => { + if (!warned.has(message)) deps.warn(message); + warned.add(message); + }; /** Auto-merge left on for a head other than `head`, reported once per head and request. */ const movedAutoMerge = async (head: string, pull: ObjectValue): Promise => { if (pull.autoMergeRequest == null) return []; @@ -391,7 +397,7 @@ async function watch( const enabledAt = string(request.enabledAt); const id = `${head}:auto-merge:${enabledAt}`; if (cursor.checks.reported.includes(id)) return []; - const enabled = enabledHeads.get(enabledAt) ?? (await enabledHead(github, pull)); + const enabled = enabledHeads.get(enabledAt) ?? (await enabledHead(github, pull, warnOnce)); if (enabled === null) return []; enabledHeads.set(enabledAt, enabled); if (enabled === head) return []; diff --git a/test/status.test.ts b/test/status.test.ts index f7846eb3..0e6f146f 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -112,7 +112,8 @@ function setup( holdFields?: Node; eventPages?: { rows: Node[]; next?: string }[]; /** Pages of the head repository's activity, newest first. */ - activity?: { rows: Node[]; next?: string }[]; + activity?: { rows: Node[]; next?: string; status?: number }[]; + warnings?: string[]; } = {}, ) { const queries: Record[] = []; @@ -126,13 +127,15 @@ function setup( token: "test-token", now: () => options.now ?? 0, sleep: async () => {}, - warn: () => {}, + warn: (message) => options.warnings?.push(message), fetch: async (url, init) => { const path = new URL(String(url)); if (path.pathname === "/repos/fork/project/activity") { activityRequests.push(`${path.pathname}${path.search}`); const page = activityPages.shift(); if (!page) return Response.json({ message: "Not Found" }, { status: 404 }); + if (page.status === 403) + return Response.json({ message: "Resource not accessible by integration" }, { status: 403 }); return Response.json(page.rows, { headers: page.next ? { Link: `<${page.next}>; rel="next"` } : {} }); } if (path.pathname !== "/graphql") { @@ -270,8 +273,9 @@ describe("the head auto-merge was enabled on", () => { const push = (after: string, timestamp: string, activity_type = "push") => ({ after, timestamp, activity_type }); const enabled = async ( request: Node, - activity: { rows: Node[]; next?: string }[] = [], + activity: { rows: Node[]; next?: string; status?: number }[] = [], head = B, + warnings?: string[], ): Promise<{ line: string | undefined; requests: string[] }> => { const body = pull({ head, @@ -287,7 +291,7 @@ describe("the head auto-merge was enabled on", () => { ...request, }, }); - const fixture = setup([body], { activity }); + const fixture = setup([body], { activity, warnings }); const status = await pullRequestStatus(pr, fixture.deps); return { line: statusLines(status).find((line) => line.startsWith("auto-merge ")), @@ -328,9 +332,18 @@ describe("the head auto-merge was enabled on", () => { expect((await enabled({}, [{ rows: [push(B, ENABLED), push(A, "2026-10-07T11:59:00Z")] }])).line).toBe( `auto-merge merge kixelated ${ENABLED} -`, ); - // A head repository the token cannot read. + // A head repository the token cannot see. expect((await enabled({}, [])).line).toBe(`auto-merge merge kixelated ${ENABLED} -`); }); + + test("is unknown, with a warning, when the token may not read the activity, and status still prints", async () => { + const warnings: string[] = []; + const result = await enabled({}, [{ rows: [], status: 403 }], B, warnings); + expect(result.line).toBe(`auto-merge merge kixelated ${ENABLED} -`); + expect(warnings).toEqual([ + "cannot read fork/project's push activity, so the head auto-merge was enabled on is unknown: GitHub HTTP 403 for /repos/fork/project/activity; Resource not accessible by integration", + ]); + }); }); test("reports why the latest landing stopped, from a merge queue or auto-merge", async () => { From 8d7c22e614f06bf14c3a527c4630138e7816e8f4 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 17:42:31 -0800 Subject: [PATCH 4/6] quest: drop the deleted merge-land quest from pick's Related Co-Authored-By: Claude Opus 5.5 --- quest/m0/queue-watcher/pick.md | 1 - 1 file changed, 1 deletion(-) diff --git a/quest/m0/queue-watcher/pick.md b/quest/m0/queue-watcher/pick.md index bacf262c..b7866fff 100644 --- a/quest/m0/queue-watcher/pick.md +++ b/quest/m0/queue-watcher/pick.md @@ -41,5 +41,4 @@ changes: ## 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 From b8dfdbf3b18247e9d07051526c06d45ebe8aef21 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 17:46:11 -0800 Subject: [PATCH 5/6] fix(status): count merges into the head branch when finding auto-merge's head GitHub lists a merge into a branch as pr_merge or merge_queue_merge activity, which also moves its head. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/cli/status.ts | 7 +++++-- test/status.test.ts | 12 ++++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 03d97580..35caa5d4 100644 --- a/README.md +++ b/README.md @@ -438,7 +438,7 @@ failures exit 1. GitHub keeps auto-merge enabled across a writer's push without saying which head it was enabled on, so the `auto-merge` line reads it from the auto-merge commit body's `Quest-Land-Head` pin when [`quest land`](#land-a-batch-with-quest-land) -queued the PR. Otherwise it is the last push to the head branch before +queued the PR. Otherwise it is the last push or merge to the head branch before auto-merge was enabled, from the head repository's activity; it prints `-` until GitHub lists the current head's push, which takes seconds to minutes, and when a push shares the second auto-merge was enabled. A head there that differs diff --git a/src/cli/status.ts b/src/cli/status.ts index 0b25d830..b5c6cc27 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -184,10 +184,13 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie return { ...status, holds: ownership.holds, starterHold: ownership.starter }; } +/** Repository activity types that move a branch to a new head, including merges into it. */ +const BRANCH_UPDATES = ["push", "force_push", "branch_creation", "pr_merge", "merge_queue_merge"]; + /** * The head a PR node's auto-merge request was enabled on, or null when that cannot be told. GitHub * keeps auto-merge across a writer's push but does not say which head it was enabled on. A - * `quest land` pin names it; otherwise it is the last push to the head branch before auto-merge + * `quest land` pin names it; otherwise it is the last push or merge to the head branch before auto-merge * was enabled, read from the head repository's activity once that lists the current head's push. * A push in the same second as enabling is ambiguous. An activity listing the token cannot read * leaves it unknown, with a warning, rather than failing the caller. The node needs `headRefOid`, @@ -223,7 +226,7 @@ export async function enabledHead( } if (response.status === 404) return null; for (const row of array(await response.json())) - if (["push", "force_push", "branch_creation"].includes(string(row.activity_type))) + if (BRANCH_UPDATES.includes(string(row.activity_type))) pushes.push({ sha: string(row.after), time: Date.parse(string(row.timestamp)) }); path = link(response, "next"); } diff --git a/test/status.test.ts b/test/status.test.ts index 6d22d2a5..b68a30b9 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -317,6 +317,18 @@ describe("the head auto-merge was enabled on", () => { ]); }); + test("counts a merge into the head branch as moving its head", async () => { + const result = await enabled({}, [ + { + rows: [ + push(B, "2026-10-07T12:05:00Z", "pr_merge"), + push(A, "2026-10-07T11:59:00Z", "merge_queue_merge"), + ], + }, + ]); + expect(result.line).toBe(`auto-merge merge kixelated ${ENABLED} ${A}`); + }); + test("matches the head when no push followed", async () => { const result = await enabled({ commitBody: "" }, [{ rows: [push(B, "2026-10-07T11:59:59Z")] }]); expect(result.line).toBe(`auto-merge merge kixelated ${ENABLED} ${B}`); From 6596bb317e64ec22235779031c2bf2984ea09ffc Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 17:48:53 -0800 Subject: [PATCH 6/6] quest: drop submodule-links' link to the finished merge-land quest Co-Authored-By: Claude Opus 5.5 --- quest/m0/helpers/submodule-links.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/quest/m0/helpers/submodule-links.md b/quest/m0/helpers/submodule-links.md index 48945904..159b9002 100644 --- a/quest/m0/helpers/submodule-links.md +++ b/quest/m0/helpers/submodule-links.md @@ -29,7 +29,3 @@ pinned commit for snapshots; reuse that lookup rather than writing a second one. Decided 2026-10-29 (user, moq.pro follow-ups): plan only this fix upstream. The label-queue gaps it was planned with retire once moq.pro lands through `quest land`. - -## Related - -- [quest merge --land gaps](/quest/m0/helpers/merge-land.md) - the other merge-agent friction from these runs