diff --git a/README.md b/README.md index 2166eab1..b37bdee6 100644 --- a/README.md +++ b/README.md @@ -291,7 +291,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 @@ -299,7 +300,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 @@ -348,6 +350,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 @@ -406,7 +414,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 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 +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 agents @@ -458,9 +475,11 @@ 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 +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. Follow the repository’s own landing instructions in CONTRIBUTING.md or AGENTS.md, such as its label queue; never @@ -471,6 +490,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. A PR that leaves the queue unmerged shows why under `landing-stopped` in `quest status`. +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. 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 da6a69f9..a45cef63 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 2bd3d0be..f3d934f1 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -27,7 +27,7 @@ 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. diff --git a/quest/m0/helpers/README.md b/quest/m0/helpers/README.md index dab6554d..bc803bfc 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -70,6 +70,5 @@ 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 - [Headless agents](/quest/m0/helpers/headless-agents.md) - `/quest-spawn api` launches, resumes, and stops billed agents through one command auto mode allows - [Links into an uninitialized submodule aren't broken](/quest/m0/helpers/submodule-links.md) - `quest check` passes in a fresh checkout without populating submodules diff --git a/quest/m0/helpers/merge-land.md b/quest/m0/helpers/merge-land.md deleted file mode 100644 index b75121b6..00000000 --- a/quest/m0/helpers/merge-land.md +++ /dev/null @@ -1,41 +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. 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. Since 2026-10-31 (user), -`quest merge` takes no label, so direct merges have no label to keep. 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. -- **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/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 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 diff --git a/src/cli/github.ts b/src/cli/github.ts index 65a8d303..51d5b130 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -61,9 +61,16 @@ export interface RequestOptions { /** 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 origin = readRemote(root, "origin"); - const repository = origin && remoteRepository(origin).repository; - 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 origin: Remote | null; + try { + origin = readRemote(root, "origin"); + } catch { + origin = null; + } + if (!origin) throw new GitHubInputError(`${root} is not a checkout with an origin remote: ${outside}`); + const { repository } = remoteRepository(origin); + 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 c6137001..2f22d187 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -149,17 +149,54 @@ 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", +}; + +/** Each merge method as GitHub's settings name it. */ +const METHOD_NAMES: Record = { + merge: "merge commits", + squash: "squash merges", + rebase: "rebase merges", +}; + +/** + * 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 ${METHOD_NAMES[named]}; it allows ${allowed.map((method) => METHOD_NAMES[method]).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`, + ); +} + /** * 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. * 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. + * `named` is the method the caller asked for; otherwise {@link mergeMethod} picks one. */ export async function land( pr: PullRequest, sha: string, - method: Method, + named: Method | undefined, deps: GitHubDependencies, direct = false, ): Promise { @@ -187,23 +224,28 @@ export async function land( (check) => check.name === LAND_CHECK, ); 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 unchanged(); 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: it applies its own method and needs no auto-merge setting. + let method: Method = named ?? "merge"; + if (!snapshot.queue) { 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") - throw new Refusal( - `${snapshot.url} repository's auto-merge setting is not visible to this token, so a direct merge cannot be ruled out; follow its landing instructions in CONTRIBUTING.md or AGENTS.md`, - ); - if (!repository.allow_auto_merge) - throw new Refusal( - `${snapshot.url} repository has auto-merge disabled; follow its landing instructions in CONTRIBUTING.md or AGENTS.md (for example, its label queue). Only use --direct if the user explicitly asks`, - ); + method = mergeMethod(repository, named, snapshot.url); + 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( + `${snapshot.url} repository's auto-merge setting is not visible to this token, so a direct merge cannot be ruled out; follow its landing instructions in CONTRIBUTING.md or AGENTS.md`, + ); + if (!repository.allow_auto_merge) + throw new Refusal( + `${snapshot.url} repository has auto-merge disabled; follow its landing instructions in CONTRIBUTING.md or AGENTS.md (for example, its label queue). Only use --direct if the user explicitly asks`, + ); + } } // The batch gate tests its own current-base chain. Ungated landings need that base in the PR head. const comparison = object( @@ -219,7 +261,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`); } await unchanged(); diff --git a/src/cli/land.ts b/src/cli/land.ts index 71d9dfdd..4aaa366c 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -60,7 +60,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 8ed8bfe3..e639eafc 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -361,7 +361,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, " + @@ -369,6 +369,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. " + @@ -401,7 +404,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; a review line per " + @@ -477,12 +483,15 @@ export function main(argv: string[], out: Output): number | Promise { "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 " + + "release your own hold first, and 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. 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.\n\n" + + "AGENTS.md; never retry with --direct unless the user asks. 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.\n\n" + OWNERSHIP, ) .argument("", "GitHub PR URL, or a PR number in origin") @@ -493,7 +502,7 @@ export function main(argv: string[], out: Output): number | Promise { .action((value: string, options: { land: string; squash?: boolean; rebase?: boolean; direct?: boolean }) => { const pr = pullRequest(value, root()); 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; settle(land(pr, options.land, method, gitHubDependencies(warn), options.direct), () => undefined); }); diff --git a/src/cli/status.ts b/src/cli/status.ts index d7185480..b5c6cc27 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -3,13 +3,15 @@ import { actor, array, GitHub, + GitHubDeadline, 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 +23,9 @@ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, $moreChecks: Boolean!, $moreReviews: Boolean!, $moreThreads: 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 @@ -106,7 +109,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; @@ -175,11 +179,60 @@ 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), - holds: ownership.holds, - starterHold: ownership.starter, - }; + const status = summarize(first!, rollup, contexts, reviewNodes, threadNodes); + if (status.autoMerge !== null) status.autoMerge.head = await enabledHead(github, first!, deps.warn); + 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 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`, + * `headRefName`, `headRepository { nameWithOwner }`, and `autoMergeRequest { enabledAt commitBody }`. + */ +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 : ""); + 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 }[] = []; + const repository = string(object(pull.headRepository).nameWithOwner); + let path: string | undefined = + `/repos/${repository}/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)) { + 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 (BRANCH_UPDATES.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( @@ -267,6 +320,7 @@ function summarize( method: lower(autoMerge.mergeMethod), enabledBy: actor(autoMerge.enabledBy), enabledAt: string(autoMerge.enabledAt), + head: null, }, mergeQueue: queue === null @@ -339,7 +393,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..308b3934 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,30 @@ 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(); + /** 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 []; + 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, warnOnce)); + 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 +483,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 4f35eb83..edcf19f2 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -28,6 +28,8 @@ 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; baseRef = "main"; /** Runs after each pull request read, to change it between reads. */ afterRead?: () => void; @@ -78,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") { @@ -425,6 +430,68 @@ describe("merge --land", () => { } }); + 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); + await land(PR, 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"); + await land(PR, 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"; + await land(PR, HEAD, undefined, pull.deps(), true); + expect(pull.writes).toEqual([`mergePullRequest ${HEAD} SQUASH`]); + }); + + test("refuses 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 commits; it allows squash merges"], + [["merge", "squash"], "rebase", "does not allow rebase merges; it allows merge commits, squash merges"], + [["squash", "rebase"], undefined, "pass --squash or --rebase"], + [[], undefined, "allows no merge method"], + ]; + for (const [methods, named, refusal] of cases) { + const pull = allowing(...methods); + expect(await refused(land(PR, HEAD, named, pull.deps()))).toContain(refusal); + expect(pull.writes).toEqual([]); + } + }); + + test("leaves a gated base or a merge queue to its own method", async () => { + const gated = allowing("merge"); + gated.required = ["quest land"]; + await land(PR, 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[] = []; + await land(PR, 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"; diff --git a/test/status.test.ts b/test/status.test.ts index 8520f57a..b68a30b9 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"; @@ -109,6 +109,9 @@ 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; status?: number }[]; + warnings?: string[]; } = {}, ) { const queries: Record[] = []; @@ -116,13 +119,23 @@ 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, 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") { expect(path.pathname).toBe("/repos/example/project/issues/1/events"); eventRequests.push(String(url)); @@ -155,7 +168,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 () => { @@ -243,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", @@ -252,6 +265,96 @@ 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; status?: number }[] = [], + head = B, + warnings?: string[], + ): 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, warnings }); + 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("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}`); + }); + + 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 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 () => { 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);