From 2c25eaaa621c4f9856d25ce05fd02c39cb6af253 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 22:39:02 -0700 Subject: [PATCH 1/5] chore(quest): claim quest/m0/helpers/pr-states.md Quest-Claim: fad0dab25b515b9c From 5405f45507e2e7aa152028f9fef588baa52d694e Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 23:19:54 -0700 Subject: [PATCH 2/5] feat(cli): own PRs with quest iterate, ready, merge, and release Each command takes a PR with one quest: label and prints a hold token, the ID of the timeline event that added it. A token swaps the label or acts on it: ready --done hands a draft to reviewers, merge --land enables auto-merge bound to a head SHA with a merge commit by default, and release hands the PR back. The iterate, ready, merge, and complete skills take PRs before acting and skip PRs other sessions hold. Co-Authored-By: Claude Opus 5.5 --- README.md | 36 +++- assets/skills/complete.md | 4 +- assets/skills/iterate.md | 5 +- assets/skills/merge.md | 15 +- assets/skills/ready.md | 10 +- quest/m0/helpers/README.md | 4 +- quest/m0/helpers/complete-loop.md | 1 - quest/m0/helpers/guide-rules.md | 1 - quest/m0/helpers/pr-states.md | 76 ------- quest/m0/kix/merge-job.md | 6 +- src/cli/claims.ts | 4 +- src/cli/github.ts | 9 +- src/cli/hold.ts | 317 +++++++++++++++++++++++++++ src/cli/main.ts | 90 ++++++++ test/hold.test.ts | 343 ++++++++++++++++++++++++++++++ 15 files changed, 823 insertions(+), 98 deletions(-) delete mode 100644 quest/m0/helpers/pr-states.md create mode 100644 src/cli/hold.ts create mode 100644 test/hold.test.ts diff --git a/README.md b/README.md index e4355494..aa7d6f03 100644 --- a/README.md +++ b/README.md @@ -197,7 +197,7 @@ quest check --base origin/main --branch quest/m0/export ``` Fetch the base first, since a stale one can predate the quest. The -`quest-merge` skill runs this before enabling auto-merge. A repository with +`quest-merge` skill runs this before landing a PR. A repository with `quest` installed in CI can also run it on pull requests, where the checkout is detached, so pass the head branch: @@ -364,6 +364,40 @@ 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. +## Own a pull request + +Before a session pushes to an existing PR, it takes the PR, so two sessions +never iterate on or merge one PR at once: + +```sh +quest iterate 128 # prints a hold token +quest ready 128 # swap to quest:ready, printing a new token +quest ready 128 --done # drop "Decisions needed" and mark it ready +quest merge 128 # swap to quest:merge for the fix loop +quest merge 128 --land # auto-merge bound to that head +quest release 128 # hand it back +``` + +A taken PR carries one label, `quest:iterate`, `quest:ready`, or +`quest:merge`. Sessions often share a GitHub account, so taking a label prints +a hold token: the ID of the timeline event that added it. Every command refuses +while a `quest:*` label it holds no token for is on the PR, including its own +state's; a draft its starter still holds, one without a `## Decisions needed` +section; and a merged or closed PR, since a push after the merge brings the +branch back. The refusal names the holder's label, age, and token, so you can +decide whether to release a stale hold. When two sessions add different labels +at once, the first label added keeps the PR and the other backs off. + +`--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. `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 +`quest wait`, and exit 0 when done, 3 when refused, 2 for invalid arguments, +and 1 for other failures. + ## Gate expensive commands `quest gate [args...]` holds an expensive command until the host has diff --git a/assets/skills/complete.md b/assets/skills/complete.md index 44944eae..4427b8d3 100644 --- a/assets/skills/complete.md +++ b/assets/skills/complete.md @@ -5,7 +5,9 @@ description: Decide which open PRs to merge, then merge them in parallel. Before starting, run `quest guide`, `quest skill ready`, `quest skill merge`, and `quest skill plan`. The goal is to evaluate the open PRs in the repository, drafts included, and decide which ones to merge. -For a draft, summarize the open decisions its body lists and note whether another agent is still pushing to it. +For a draft, summarize the open decisions its body lists. +Skip each PR another session holds: one with a `quest:*` label, or a draft without a `## Decisions needed` section. +List those with how long ago each was last active, and offer to take over only one that looks stale. The skill argument can be used to filter the PRs in scope. Each merge is performed in parallel by a sub-agent. diff --git a/assets/skills/iterate.md b/assets/skills/iterate.md index a8b8fcc3..a0b2321b 100644 --- a/assets/skills/iterate.md +++ b/assets/skills/iterate.md @@ -6,6 +6,8 @@ Before you begin, run `quest guide` and `quest skill plan`, and read their outpu You own this PR until it is landable. Parse the arguments to determine the PR number. Any other text is the user's feedback on the PR; interpret it against the diff. +Take it with `quest iterate ` before acting, and keep the hold token it prints; `quest iterate --help` explains holds. +If it refuses because another session holds the PR, stop and report what it printed; taking over a stale hold is the user's choice. If you cannot push to the PR's branch, stop and report. Read the PR, its linked issues and quest, the reviews and comments posted on it, and the surrounding code. @@ -24,5 +26,6 @@ One round only; never request a review, including by toggling a ready PR to draf Push your changes to the PR and update its summary. Summarize the changes made, and offer `/quest-plan` for follow-ups that belong outside this PR. -Mark the PR ready once nothing is unfinished and no decisions need the user. +Mark the PR ready with `quest ready --done` once nothing is unfinished and no decisions need the user. +Hand it back with `quest release `, using the latest token printed, whenever you stop, including to ask. Never merge; that is `/quest-merge`, once the user picks it. diff --git a/assets/skills/merge.md b/assets/skills/merge.md index b3439d47..38b37ddc 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -6,6 +6,9 @@ Before you begin, run `quest guide` and read its output completely. Parse the arguments to determine the PR number, otherwise resolve it from the current context/branch. If a draft, stop and offer `/quest-ready` instead. +Take the PR with `quest merge ` before acting, passing a token you already hold from another state to swap it, and keep the hold token it prints; `quest merge --help` explains holds. +If it refuses because another session holds the PR, stop and report what it printed; taking over a stale hold is the user's choice. +Hand it back with `quest release ` whenever you stop before it lands, including to ask. Read the PR's head, checks, merge state, reviews by head, and unresolved threads with `quest status `, including after each `quest wait` wakes, instead of chaining `gh` queries. Fix any mechanical issues with the PR, such as merge conflicts and failing CI checks. @@ -41,14 +44,16 @@ run each wait in the foreground instead, bounded by the shell's own time limit, and re-run it with `--since` until it returns activity. Merge the PR *only* after all outstanding decisions have been confirmed. -Before enabling auto-merge, fetch the PR's base branch and head, check out the head, and run `quest --root check --base / --branch `. +Before landing it, fetch the PR's base branch and head, check out the head, and run `quest --root check --base / --branch `. Run a `quest` the PR cannot modify, such as an installed release or a build from the base, from a directory outside the head checkout: the head's files, including runtime config such as `bunfig.toml`, are untrusted. A stale local base can predate the quest and skip the check. On a finding, fix the branch: delete the finished quest with `quest rm`, or rewrite it to the remainder the PR body names. Then push, re-check, and continue; ask the user only when what remains is unclear. -Leave a summary of the changes and decisions made and enable auto-merge with the full 40-character head SHA. -If your caller said it watches the merge queue, report once auto-merge is enabled. +Leave a summary of the changes and decisions made, then land the head you checked 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. +The `quest:merge` label stays on while the PR is queued or auto-merging. +If your caller said it watches the merge queue, report once it is landing. Otherwise wait until the PR merges. If it leaves the queue unmerged or auto-merge -is disabled, confirm with `quest status `, then report why instead of -enabling auto-merge again; a new head needs the user's approval. +is disabled, confirm with `quest status `, release the PR, then report why +instead of landing it again; a new head needs the user's approval. Never close a PR to unstick it, and never work around a refused merge; ask instead. diff --git a/assets/skills/ready.md b/assets/skills/ready.md index faca72ec..420b51a5 100644 --- a/assets/skills/ready.md +++ b/assets/skills/ready.md @@ -7,7 +7,12 @@ Before you begin, run `quest guide` and `quest skill merge`, and read their outp Parse the arguments to determine the PR number, otherwise resolve it from the current context/branch. If its body lists open decisions or unfinished work, offer `/quest-iterate` first. -Flip a draft to "Ready for Review", then wait with `quest wait`, as `quest skill merge` describes, until an automated reviewer reviews or reacts. +Take the PR with `quest ready ` and keep the hold token it prints; `quest ready --help` explains holds. +If it refuses because another session holds the PR, stop and report what it printed; taking over a stale hold is the user's choice. +A draft you started in this session has no `## Decisions needed` section, so it is still yours and the command refuses it: mark it ready for review yourself, then take it. + +Flip a draft to "Ready for Review" with `quest ready --done`, which also drops its `## Decisions needed` section. +Then wait with `quest wait`, as `quest skill merge` describes, until an automated reviewer reviews or reacts. Skip the wait only if the repository has no automated reviewer. Summarize the reviews and CI from `quest status `, then interactively prompt the user with your recommendation: @@ -18,3 +23,6 @@ Summarize the reviews and CI from `quest status `, then interactively prompt Also offer `/quest-plan` for suggested follow-ups. A background agent that cannot prompt lists these in its report instead. + +Hand the PR back with `quest release ` before you ask or report. +To go straight on to `/quest-iterate` or `/quest-merge` in this session, pass the token to its command instead, which swaps the label. diff --git a/quest/m0/helpers/README.md b/quest/m0/helpers/README.md index 71061094..6ece7185 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -52,7 +52,8 @@ commands from Sep-Oct. Session counts in each child come from that window. maintainer time and change only prose. The complete loop ranks right after the PR state commands, which also rewrite `complete`. Findings that fit an existing child went into it: the merge method and the merged-PR refusal - into [PR state commands](/quest/m0/helpers/pr-states.md). Ready at + into the PR state commands, which landed in + [kixelated/quest#185](https://github.com/kixelated/quest/pull/185). Ready at hand-back went into [Guide rules](/quest/m0/helpers/guide-rules.md), since quest start landed without it. @@ -60,7 +61,6 @@ commands from Sep-Oct. Session counts in each child come from that window. - [Unattended work](/quest/m0/helpers/afk.md) - `/quest-afk` replaces the `afk` argument: it skips every decision and keeps starting agreed work; every skill ends with a summary - [Guide rules](/quest/m0/helpers/guide-rules.md) - pushing, briefs, ready at hand-back, follow-ups, whose review counts, and user-owned choices -- [PR state commands](/quest/m0/helpers/pr-states.md) - `quest iterate`, `ready`, `merge`, and `release` label the one owner of a PR - [Complete loop](/quest/m0/helpers/complete-loop.md) - `/quest-complete` re-scans PRs, keeps picks and holds, and counts rounds per PR - [Headless agents](/quest/m0/helpers/headless-agents.md) - `/quest-spawn api` launches, resumes, and stops billed agents through one command auto mode allows - [Upstream claims](/quest/m0/helpers/claims.md) - claims and readiness read from the upstream tree, ignoring merged branches diff --git a/quest/m0/helpers/complete-loop.md b/quest/m0/helpers/complete-loop.md index a63eddfd..999bd8fe 100644 --- a/quest/m0/helpers/complete-loop.md +++ b/quest/m0/helpers/complete-loop.md @@ -27,5 +27,4 @@ edits through this plan. ## Related -- [PR state commands](/quest/m0/helpers/pr-states.md) - also rewrites how `complete` dispatches and skips held PRs - [Unattended work](/quest/m0/helpers/afk.md) - carries on picks the user already made diff --git a/quest/m0/helpers/guide-rules.md b/quest/m0/helpers/guide-rules.md index 8aab88fa..ad1bdf4a 100644 --- a/quest/m0/helpers/guide-rules.md +++ b/quest/m0/helpers/guide-rules.md @@ -62,5 +62,4 @@ edits through this plan; follow `PROMPTING.md` (goals over prescriptions). ## Related -- [PR state commands](/quest/m0/helpers/pr-states.md) - refuses a merged PR mechanically and carries the merge method - [Unattended work](/quest/m0/helpers/afk.md) - the guide's final-summary rule diff --git a/quest/m0/helpers/pr-states.md b/quest/m0/helpers/pr-states.md deleted file mode 100644 index f593dcbb..00000000 --- a/quest/m0/helpers/pr-states.md +++ /dev/null @@ -1,76 +0,0 @@ -# [M] Own PRs with quest iterate, ready, and merge - -## Goal - -Before a session pushes to an existing PR, it takes the PR with -`quest iterate`, `quest ready`, or `quest merge`. Each command labels the PR -with its state and does that state's GitHub step. `quest release` hands it -back. Two sessions never iterate or merge one PR at once, and any PR on its -way in shows `quest:merge` on GitHub. - -## Plan - -Decided 2026-10-20 (user) from the transcript audit recorded in -[quest start](https://github.com/kixelated/quest/blob/d265755/quest/m0/helpers/start.md). Overlapping merge sessions in one -repository drew "another agent was merging, I told them to stop, can you take -over anything needed?", and the same-machine guard flagged sessions in other -repositories ("those are in another repo?"). - -- Decided (user): one owner per PR replaces "one merge agent per - repository". There is no repository-wide merge lock or coordinator. - GitHub's merge queue, or the repository's own queue, orders landings, and a - PR that conflicts goes back to its owner. -- Decided (user): a label per state, with no owner comment: - `quest:iterate`, `quest:ready`, and `quest:merge`. At most one is on a - PR. Planning PRs get none. A draft that its starter still holds, one with no - `## Decisions needed` section (the guide's Execution section), - counts as owned too, so every command refuses it until the starter hands - back; taking it over from a stale starter is the user's choice. Decided - 2026-10-21 (user), replacing a `quest:start` label. -- Decided (user): a command per state, each doing its GitHub step. Every - command refuses while any `quest:*` label is on the PR that this session - does not hold, including its own state's, then swaps in its own, using the - label helper, which this quest adds: take a label only when no `quest:*` - label is on the PR, and back off if another appears in the timeline - first. -- The label holds no owner identity, and sessions often share one GitHub - account, so taking a label prints a hold token: the ID of the timeline - event that added it. A swap, `--done`, `--land`, and `quest release` take - that token and refuse unless it is still the latest add of the PR's - current label. A stale label is offered to the user for takeover instead. - - `quest iterate ` only takes the PR. - - `quest ready ` takes a handed-back draft and, once its decisions are - settled, marks it ready for review with `--done`, removing the - `## Decisions needed` section. - - `quest merge ` takes it for the fix loop. `quest merge --land ` - enables auto-merge bound to that full head SHA, which also enters - GitHub's merge queue. - - `quest release ` removes the label that token holds. -- Decided (user): `quest:merge` stays on through the fix loop and while the - PR is queued or auto-merging, until it merges. If it leaves the queue - unmerged, the command that notices removes the label, and the PR is idle - and waits on the maintainer's pick again. -- Decided 2026-10-21 (user), replacing a carve-out for moq-dev/moq.pro's - `merge` label queue, which the user means to remove: `--land` makes a merge - commit by default (`gh pr merge --merge`), through GitHub's merge queue - when the base has one. `--squash` or `--rebase` selects another method; - the command never reads prose, so the merge skill picks the flag from the - method a repository's root instructions name for that base. The merge - skill named no method, so an agent squashed a PR into moq-dev/moq's - `release`, which needs a merge commit. -- Every command refuses a merged or closed PR, because a push after the merge - brings the deleted branch back. -- A merge pick covers the fixes its own agent pushes for conflicts, CI, and - review findings. A head moved by anyone else, a design change, or the merge - skill's design checkpoint asks the user again, as `complete` already says. -- Accept the same PR argument as `quest wait`. Commands never decide whether - a PR should merge; the agent and the user still do. -- Update the `iterate`, `ready`, `merge`, and `complete` skills to take the - PR before acting and release it at hand-back, and to skip PRs other - sessions hold. Replace the merge skill's raw auto-merge call with - `quest merge --land`. The user asked for those edits through this plan. -- Test a refused draft its starter still holds, a race for one PR, two - sessions taking the same state, a release - with a stale token, a refused swap, landing on a moved head, a PR - leaving the queue, a refused merged PR, the default merge commit, and - `--squash` and `--rebase`. diff --git a/quest/m0/kix/merge-job.md b/quest/m0/kix/merge-job.md index cbdf7062..47bd04d3 100644 --- a/quest/m0/kix/merge-job.md +++ b/quest/m0/kix/merge-job.md @@ -20,8 +20,9 @@ or needs them. - Decided (user): one owner per PR, not one merge agent per repository. kix never runs two jobs on one PR and never acts on a PR another session holds. GitHub's merge queue, or the repository's own queue, orders landings. -- Decided (user): use the `quest:merge` label from the - [PR state commands](/quest/m0/helpers/pr-states.md). The bot adds it when +- Decided (user): use the `quest:merge` label from `quest merge` + ([kixelated/quest#185](https://github.com/kixelated/quest/pull/185)), whose + `--help` states the label contract kix shares with local sessions. The bot adds it when the job is assigned and keeps it while the PR is queued or auto-merging. If the PR leaves the queue unmerged, the bot removes it and the PR becomes an idle decision item again. Runners hold no GitHub credentials, so the @@ -48,4 +49,3 @@ or needs them. ## Required - [Runner](/quest/m0/kix/runner.md) - jobs that run the merge skill -- [PR state commands](/quest/m0/helpers/pr-states.md) - the label contract kix shares with local sessions diff --git a/src/cli/claims.ts b/src/cli/claims.ts index b5ff04d2..aca0e76d 100644 --- a/src/cli/claims.ts +++ b/src/cli/claims.ts @@ -13,8 +13,8 @@ import { string, } from "./github"; -/** The heading that hands a draft back to the maintainer; only its presence matters. */ -const HAND_BACK = /^#{2}[ \t]+Decisions needed[ \t]*#*[ \t]*$/im; +/** The heading that hands a draft back to the maintainer; only its presence matters. Web edits end lines in CRLF. */ +export const HAND_BACK = /^#{2}[ \t]+Decisions needed[ \t]*#*[ \t]*\r?$/im; /** A quest's branch: its path without `.md`. */ export function questBranch(path: string): string { diff --git a/src/cli/github.ts b/src/cli/github.ts index 78c1ecb7..78ee4117 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -98,11 +98,12 @@ export class GitHub { /** * A 2xx or 304 response; anything else fails after rate-limit retries, with - * GitHub's reason when it gives one. `body` makes it a POST unless `method` says otherwise. + * GitHub's reason when it gives one, unless `allow` lists its status. `body` makes it a POST + * unless `method` says otherwise. */ async request( path: string, - options: { etag?: string; body?: unknown; method?: "POST" | "PATCH" } = {}, + options: { etag?: string; body?: unknown; method?: "POST" | "PATCH" | "DELETE"; allow?: number[] } = {}, ): Promise { // Follow only same-origin pagination links; never send a token to another host. const url = new URL(path, API); @@ -115,7 +116,7 @@ export class GitHub { try { response = await this.deps.fetch(url, { ...(options.body === undefined - ? {} + ? options.method && { method: options.method } : { method: options.method ?? "POST", body: JSON.stringify(options.body) }), headers: { Accept: "application/vnd.github+json", @@ -131,7 +132,7 @@ export class GitHub { throw error; } this.pollInterval = Math.max(this.pollInterval, seconds(response.headers.get("x-poll-interval")) ?? 0); - if (response.ok || response.status === 304) return response; + if (response.ok || response.status === 304 || options.allow?.includes(response.status)) return response; const body = await response.text(); const limited = response.status === 429 || diff --git a/src/cli/hold.ts b/src/cli/hold.ts new file mode 100644 index 00000000..f47b1ba0 --- /dev/null +++ b/src/cli/hold.ts @@ -0,0 +1,317 @@ +// `quest iterate`, `ready`, `merge`, and `release`: one session owns a PR at a +// time. It holds one `quest:` label, and the ID of the timeline event +// that added the label is its hold token, since sessions often share an account. +import { HAND_BACK, repositoryPath } from "./claims"; +import { array, GitHub, type GitHubDependencies, object, type ObjectValue, type PullRequest, string } from "./github"; +import { Refusal, ago } from "./start"; + +export type State = "iterate" | "ready" | "merge"; +export type Method = "merge" | "squash" | "rebase"; + +const PREFIX = "quest:"; +const MERGE = `${PREFIX}merge`; +/** Merge states GitHub merges at once, where it refuses to enable auto-merge. */ +const IMMEDIATE = ["CLEAN", "HAS_HOOKS", "UNSTABLE"]; + +/** The PR, its `quest:*` labels, and the timeline events that add a label or start a landing. */ +const READ = `query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + id url state isDraft body headRefOid mergeStateStatus isMergeQueueEnabled + autoMergeRequest { enabledAt } + mergeQueueEntry { state } + labels(first: 100) { nodes { name } } + timelineItems(last: 100, itemTypes: [LABELED_EVENT, AUTO_MERGE_ENABLED_EVENT, ADDED_TO_MERGE_QUEUE_EVENT]) { + nodes { + __typename + ... on LabeledEvent { id createdAt label { name } } + ... on AutoMergeEnabledEvent { id createdAt } + ... on AddedToMergeQueueEvent { id createdAt } + } + } + } + } +}`; + +interface Event { + id: string; + time: number; + /** The label it added, or `null` for auto-merge enabled or a merge-queue entry. */ + label: string | null; +} + +interface Snapshot { + id: string; + url: string; + state: "OPEN" | "CLOSED" | "MERGED"; + draft: boolean; + body: string; + head: string; + mergeState: string; + queue: boolean; + /** Auto-merge is enabled or the PR is in the merge queue. */ + landing: boolean; + /** The `quest:*` labels on the PR. */ + labels: string[]; + /** Oldest first. */ + events: Event[]; +} + +/** A session's hold: its label and the token naming the event that added it. */ +export interface Hold { + label: string; + token: string; +} + +/** Take `pr` for `state`, or swap to it from the label `token` holds. Prints nothing; returns the hold. */ +export async function take( + pr: PullRequest, + state: State, + token: string | undefined, + deps: GitHubDependencies, +): Promise { + const { hold } = await acquire(new Labels(pr, deps), state, token); + return hold; +} + +/** + * Take `pr` for ready, then hand it to reviewers: drop its `## Decisions needed` + * section and mark a draft ready for review. + */ +export async function finish(pr: PullRequest, token: string, deps: GitHubDependencies): Promise { + const labels = new Labels(pr, deps); + const { hold, snapshot } = await acquire(labels, "ready", token); + if (HAND_BACK.test(snapshot.body)) { + await labels.github.request(`${repositoryPath(pr)}/pulls/${pr.number}`, { + method: "PATCH", + body: { body: withoutHandBack(snapshot.body) }, + }); + } + if (snapshot.draft) { + await labels.github.graphql( + `mutation($id: ID!) { markPullRequestReadyForReview(input: { pullRequestId: $id }) { clientMutationId } }`, + { id: snapshot.id }, + ); + } + return hold; +} + +/** + * 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. + */ +export async function land( + pr: PullRequest, + token: string, + sha: string, + method: Method, + deps: GitHubDependencies, +): Promise { + const labels = new Labels(pr, deps); + const { hold, snapshot } = await acquire(labels, "merge", token); + if (snapshot.head.toLowerCase() !== sha.toLowerCase()) { + throw new Refusal(`${snapshot.url} head moved to ${snapshot.head}; check the new head before landing it`); + } + if (snapshot.queue && method !== "merge") { + deps.warn(`${snapshot.url} lands through a merge queue, which applies its own merge method`); + } + const mutation = + !snapshot.queue && IMMEDIATE.includes(snapshot.mergeState) ? "mergePullRequest" : "enablePullRequestAutoMerge"; + await labels.github.graphql( + `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!) { + ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha, mergeMethod: $method }) { clientMutationId } + }`, + { id: snapshot.id, sha: snapshot.head, method: method.toUpperCase() }, + ); + return hold; +} + +/** Remove the label `token` holds. A label already gone is released; one added again since is not ours. */ +export async function release(pr: PullRequest, token: string, deps: GitHubDependencies): Promise { + const labels = new Labels(pr, deps); + const snapshot = await labels.read(); + const event = snapshot.events.find((candidate) => candidate.id === token && candidate.label !== null); + if (!event?.label) throw new Refusal(unknown(snapshot, token)); + if (!snapshot.labels.includes(event.label)) { + deps.warn(`${event.label} is already off ${snapshot.url}`); + return event.label; + } + held(snapshot, token); + await labels.remove(event.label); + return event.label; +} + +/** + * Take `pr` for `state`, refusing while it is merged or closed, while another + * session holds a `quest:*` label, or while it is a draft its starter still + * holds. With `token`, the label it holds is swapped for this state's. + */ +async function acquire( + labels: Labels, + state: State, + token: string | undefined, +): Promise<{ hold: Hold; snapshot: Snapshot }> { + const label = PREFIX + state; + let snapshot = await labels.read(); + if (snapshot.state !== "OPEN") { + throw new Refusal(`${snapshot.url} is ${snapshot.state.toLowerCase()}; a push now would bring its branch back`); + } + snapshot = await labels.dequeued(snapshot); + const mine = token === undefined ? undefined : held(snapshot, token); + const other = snapshot.labels.find((name) => name !== mine); + if (other !== undefined) throw new Refusal(holder(snapshot, other, labels.deps.now())); + if (mine === label) return { hold: { label, token: token! }, snapshot }; + if (mine === undefined && snapshot.draft && !HAND_BACK.test(snapshot.body)) { + throw new Refusal( + `${snapshot.url} is a draft its starter still holds: its body has no ## Decisions needed section, so taking it over is the user's choice`, + ); + } + + await labels.add(label); + const after = await labels.read(); + const ours = latest(after, label); + if (!ours) throw new Error(`added ${label} to ${after.url}, but its timeline does not show it`); + // Another session took the PR at the same time: the first label added keeps it. + const rival = after.labels.find((name) => { + if (name === label || name === mine) return false; + const added = latest(after, name); + return !added || after.events.indexOf(added) < after.events.indexOf(ours); + }); + if (rival !== undefined) { + await labels.remove(label); + throw new Refusal(holder(after, rival, labels.deps.now())); + } + if (mine !== undefined) await labels.remove(mine); + return { hold: { label, token: ours.id }, snapshot: { ...after, labels: [label] } }; +} + +/** The label `token` holds on the PR, or a refusal when it holds none. */ +function held(snapshot: Snapshot, token: string): string { + const event = snapshot.events.find((candidate) => candidate.id === token && candidate.label !== null); + if (!event?.label) throw new Refusal(unknown(snapshot, token)); + if (!snapshot.labels.includes(event.label)) { + throw new Refusal( + `${event.label} is off ${snapshot.url} since ${token} added it, so this session no longer holds it`, + ); + } + if (latest(snapshot, event.label) !== event) { + throw new Refusal( + `${event.label} was added to ${snapshot.url} again after ${token}, so another session holds it`, + ); + } + return event.label; +} + +function unknown(snapshot: Snapshot, token: string): string { + return `${token} is not an event that added a label to ${snapshot.url} among its last 100`; +} + +/** Who holds `label`, for a person deciding whether to wait or take over a stale hold. */ +function holder(snapshot: Snapshot, label: string, now: number): string { + const added = latest(snapshot, label); + const since = added ? `, added ${ago(now, added.time)} as token ${added.id}` : ""; + return `${snapshot.url} is held by ${label}${since}; if it looks stale, ask the user before releasing it`; +} + +/** The latest event that added `label`. */ +function latest(snapshot: Snapshot, label: string): Event | undefined { + for (let index = snapshot.events.length - 1; index >= 0; index--) { + if (snapshot.events[index].label === label) return snapshot.events[index]; + } + return undefined; +} + +/** `body` without its `## Decisions needed` sections, each running to the next heading of level 1 or 2. */ +export function withoutHandBack(body: string): string { + const kept: string[] = []; + let skipping = false; + for (const line of body.split("\n")) { + if (HAND_BACK.test(line)) skipping = true; + else if (/^#{1,2}[ \t]/.test(line)) skipping = false; + if (!skipping) kept.push(line); + } + return kept.join("\n").trimEnd(); +} + +/** One PR's labels on GitHub. */ +class Labels { + readonly github: GitHub; + /** The PR's REST path as an issue, where its labels live. */ + readonly issue: string; + + constructor( + private readonly pr: PullRequest, + readonly deps: GitHubDependencies, + ) { + this.github = new GitHub(deps); + this.issue = `${repositoryPath(pr)}/issues/${pr.number}`; + } + + async read(): Promise { + const data = await this.github.graphql(READ, { + owner: this.pr.owner, + repo: this.pr.repo, + number: this.pr.number, + }); + const repository = data.repository; + if (repository === null) { + throw new Error(`cannot read ${this.pr.owner}/${this.pr.repo}; check the repository and token`); + } + const pull = object(repository).pullRequest; + if (pull === null) throw new Error(`${this.pr.owner}/${this.pr.repo}#${this.pr.number} is not a pull request`); + const row = object(pull); + return { + id: string(row.id), + url: string(row.url), + state: string(row.state) as Snapshot["state"], + draft: row.isDraft === true, + body: typeof row.body === "string" ? row.body : "", + head: string(row.headRefOid), + mergeState: string(row.mergeStateStatus), + queue: row.isMergeQueueEnabled === true, + landing: row.autoMergeRequest !== null || row.mergeQueueEntry !== null, + labels: array(object(row.labels).nodes) + .map((label) => string(label.name)) + .filter((name) => name.startsWith(PREFIX)), + events: array(object(row.timelineItems).nodes).map(event), + }; + } + + async add(label: string): Promise { + await this.github.request(`${this.issue}/labels`, { body: { labels: [label] } }); + } + + /** Remove `label`; one already gone is fine. */ + async remove(label: string): Promise { + await this.github.request(`${this.issue}/labels/${encodeURIComponent(label)}`, { + method: "DELETE", + allow: [404], + }); + } + + /** + * A PR that left the merge queue unmerged waits on the maintainer again, so + * its `quest:merge` label is spent once a landing started after it was added. + */ + async dequeued(snapshot: Snapshot): Promise { + const added = latest(snapshot, MERGE); + if (!added || snapshot.landing || !snapshot.labels.includes(MERGE)) return snapshot; + const landed = snapshot.events.slice(snapshot.events.indexOf(added) + 1).some((later) => later.label === null); + if (!landed) return snapshot; + await this.remove(MERGE); + this.deps.warn( + `${snapshot.url} left the merge queue unmerged, so ${MERGE} was removed; it waits on the user again`, + ); + return { ...snapshot, labels: snapshot.labels.filter((name) => name !== MERGE) }; + } +} + +function event(row: ObjectValue): Event { + const labeled = row.__typename === "LabeledEvent"; + return { + id: string(row.id), + time: Date.parse(string(row.createdAt)), + label: labeled ? string(object(row.label).name) : null, + }; +} diff --git a/src/cli/main.ts b/src/cli/main.ts index 44c2a2d9..c7cc0171 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -21,6 +21,7 @@ import { AgentInputError, HARNESSES, type Launch, processDependencies, runAgent, import { claims, questBranch } from "./claims"; import { gate } from "./gate"; import { gitHubDependencies, GitHubInputError, pullRequest } from "./github"; +import { type Hold, type State, finish, land, release, take } from "./hold"; import { landed } from "./landed"; import { type Report, moveQuest, removeQuest } from "./move"; import { init, uninstall } from "./setup"; @@ -352,6 +353,95 @@ export function main(argv: string[], out: Output): number | Promise { ); }); + const OWNERSHIP = + "Before a session pushes to an existing PR, it takes the PR with iterate, ready, or merge, and hands it " + + "back with release. A taken PR carries one quest: label; the command prints a hold token, the ID " + + "of the timeline event that added it, since sessions often share an account. A command refuses while a " + + "quest:* label it holds no token for is on the PR, including its own state's, and while the PR is merged, " + + "closed, or a draft its starter still holds (one whose body has no Decisions needed heading). A refusal " + + "names the holder's label, age, and token; releasing a stale hold with that token is the user's choice. " + + "Given a token, a command refuses unless the token is still the latest add of the PR's label, then swaps " + + "that label for its own. When two sessions add different labels at once, the first label added keeps the " + + "PR; GitHub records no event when a label already on the PR is added again, so two takes of one state " + + "within the same moment are not told apart. A quest:merge label is removed once the PR leaves the merge " + + "queue unmerged.\n\n" + + "Uses GH_TOKEN, GITHUB_TOKEN, or gh auth token. Exits 0 when done, 3 when refused, 2 for invalid " + + "arguments, and 1 for other failures."; + /** A PR state command: take the PR or swap to `state`, then do its step. Prints the hold token. */ + const ownership = (state: State, summary: string, step: string) => + program + .command(state) + .summary(summary) + .description(`${summary}.${step ? ` ${step}` : ""}\n\n${OWNERSHIP}`) + .argument("", "GitHub PR URL, or a PR number in origin") + .argument("[token]", "Hold token of the label this session holds, to swap it or act on it"); + const held = (work: Promise) => settle(work, (hold) => print(hold.token)); + + ownership("iterate", "Take a PR to settle its decisions, fix CI and review findings, and push", "").action( + (value: string, token: string | undefined) => { + held(take(pullRequest(value, root()), "iterate", token, gitHubDependencies(warn))); + }, + ); + + ownership( + "ready", + "Take a handed-back draft or a PR in review to hand it to reviewers", + "--done removes the body's Decisions needed section and marks a draft ready for review.", + ) + .option("--done", "Mark it ready for review; needs the token") + .action((value: string, token: string | undefined, options: { done?: boolean }) => { + const pr = pullRequest(value, root()); + if (!options.done) return held(take(pr, "ready", token, gitHubDependencies(warn))); + if (token === undefined) throw new UsageError("--done needs the hold token"); + held(finish(pr, token, gitHubDependencies(warn))); + }); + + ownership( + "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. The label " + + "stays on while the PR is queued or auto-merging, until it merges.", + ) + .option("--land ", "Land this full 40-character head SHA; needs the token") + .addOption(new Option("--squash", "Squash and merge").conflicts("rebase")) + .option("--rebase", "Rebase and merge") + .action( + ( + value: string, + token: string | undefined, + options: { land?: string; squash?: boolean; rebase?: boolean }, + ) => { + const pr = pullRequest(value, root()); + if (options.land === undefined) { + if (options.squash || options.rebase) throw new UsageError("--squash and --rebase need --land"); + return held(take(pr, "merge", token, gitHubDependencies(warn))); + } + 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"; + held(land(pr, token, options.land, method, gitHubDependencies(warn))); + }, + ); + + program + .command("release") + .summary("Hand back a PR by removing the label a hold token holds") + .description( + "Hand back a PR by removing the quest:* label a hold token holds. Refuses when the token is not the " + + "latest add of that label, since another session holds it now. A label already off the PR counts as " + + "released, with a warning. Unlike the other PR state commands, it works on a merged or closed PR.\n\n" + + OWNERSHIP, + ) + .argument("", "GitHub PR URL, or a PR number in origin") + .argument("", "Hold token printed when the label was taken") + .action((value: string, token: string) => { + settle(release(pullRequest(value, root()), token, gitHubDependencies(warn)), (label) => + print(`released ${label}`), + ); + }); + const worktree = program .command("worktree") .description("Reserve disk for Git checkouts and release it by pruning their scratch in place"); diff --git a/test/hold.test.ts b/test/hold.test.ts new file mode 100644 index 00000000..d0500dad --- /dev/null +++ b/test/hold.test.ts @@ -0,0 +1,343 @@ +// `quest iterate`, `ready`, `merge`, and `release`, against a PR replayed in memory. + +import { describe, expect, test } from "vitest"; + +import type { GitHubDependencies, ObjectValue } from "../src/cli/github"; +import { finish, land, release, take, withoutHandBack } from "../src/cli/hold"; +import { type Output, main } from "../src/cli/main"; +import { Refusal } from "../src/cli/start"; + +const PR = { owner: "example", repo: "project", number: 7 }; +const URL_ = "https://github.com/example/project/pull/7"; +const HEAD = "a".repeat(40); + +interface Event { + __typename: "LabeledEvent" | "AutoMergeEnabledEvent" | "AddedToMergeQueueEvent"; + id: string; + createdAt: string; + label?: { name: string }; +} + +/** One PR on GitHub: its labels, timeline, and landing state. */ +class FakePull { + state = "OPEN"; + draft = false; + body = "Summary."; + head = HEAD; + mergeState = "BLOCKED"; + queue = false; + autoMerge: ObjectValue | null = null; + queueEntry: ObjectValue | null = null; + labels: string[] = []; + events: Event[] = []; + /** Mutations and label writes, in order. */ + readonly writes: string[] = []; + /** Runs before each label is added, as when another session adds one at the same moment. */ + beforeAdd?: () => void; + private clock = Date.parse("2026-10-21T12:00:00Z"); + + /** Add a label as GitHub does: re-adding one already on the PR records no event. */ + label(name: string): string | undefined { + if (this.labels.includes(name)) return undefined; + this.labels.push(name); + return this.event("LabeledEvent", { label: { name } }); + } + + event(type: Event["__typename"], extra: Partial = {}): string { + const id = `${type}_${this.events.length + 1}`; + this.events.push({ __typename: type, id, createdAt: new Date(this.clock++).toISOString(), ...extra }); + return id; + } + + deps(warnings: string[] = []): GitHubDependencies { + return { + fetch: async (input, init) => this.handle(new URL(String(input)), init ?? {}), + now: () => this.clock, + sleep: async () => undefined, + token: "token", + warn: (message) => warnings.push(message), + }; + } + + private handle(url: URL, init: RequestInit): Response { + const method = init.method ?? "GET"; + 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 === "/graphql") + return json(200, this.graphql(String(body.query), body.variables as ObjectValue)); + if (url.pathname === "/repos/example/project/issues/7/labels" && method === "POST") { + this.beforeAdd?.(); + for (const name of body.labels as string[]) this.label(name); + this.writes.push(`add ${(body.labels as string[]).join(",")}`); + return json( + 200, + this.labels.map((name) => ({ name })), + ); + } + const removed = /^\/repos\/example\/project\/issues\/7\/labels\/(.+)$/.exec(url.pathname); + if (removed && method === "DELETE") { + const name = decodeURIComponent(removed[1]); + this.writes.push(`remove ${name}`); + if (!this.labels.includes(name)) return json(404, { message: "Label does not exist" }); + this.labels = this.labels.filter((label) => label !== name); + return json(200, []); + } + if (url.pathname === "/repos/example/project/pulls/7" && method === "PATCH") { + this.body = String(body.body); + this.writes.push("edit body"); + return json(200, {}); + } + return json(404, { message: `no route for ${method} ${url.pathname}` }); + } + + private graphql(query: string, variables: ObjectValue): ObjectValue { + if (query.startsWith("mutation")) { + const name = /\{\s*(\w+)\(/.exec(query)![1]; + if (variables.id !== "PR_7") throw new Error("mutation on the wrong PR"); + this.writes.push(variables.method ? `${name} ${variables.sha} ${variables.method}` : name); + if (name === "markPullRequestReadyForReview") this.draft = false; + if (name === "mergePullRequest") this.state = "MERGED"; + if (name === "enablePullRequestAutoMerge") { + this.autoMerge = { enabledAt: "2026-10-21T12:00:00Z" }; + this.event("AutoMergeEnabledEvent"); + } + return { data: { [name]: { clientMutationId: null } } }; + } + return { + data: { + repository: { + pullRequest: { + id: "PR_7", + url: URL_, + state: this.state, + isDraft: this.draft, + body: this.body, + headRefOid: this.head, + mergeStateStatus: this.mergeState, + isMergeQueueEnabled: this.queue, + autoMergeRequest: this.autoMerge, + mergeQueueEntry: this.queueEntry, + labels: { nodes: this.labels.map((name) => ({ name })) }, + timelineItems: { nodes: this.events.slice(-100) }, + }, + }, + }, + }; + } +} + +/** The refusal a promise rejects with. */ +async function refused(work: Promise): Promise { + const error = await work.then( + () => undefined, + (caught: unknown) => caught, + ); + expect(error).toBeInstanceOf(Refusal); + return (error as Refusal).message; +} + +describe("taking a PR", () => { + test("labels it with its state and prints the event that added the label as the token", async () => { + const pull = new FakePull(); + const hold = await take(PR, "iterate", undefined, pull.deps()); + expect(hold).toEqual({ label: "quest:iterate", token: "LabeledEvent_1" }); + expect(pull.labels).toEqual(["quest:iterate"]); + }); + + test("refuses a draft its starter still holds, and takes one handed back", async () => { + const pull = new FakePull(); + pull.draft = true; + expect(await refused(take(PR, "ready", undefined, pull.deps()))).toContain("its starter still holds"); + expect(pull.writes).toEqual([]); + + pull.body = "Summary.\r\n\r\n## Decisions needed\r\n\r\n1. Pick one.\r\n"; + expect((await take(PR, "ready", undefined, pull.deps())).label).toBe("quest:ready"); + }); + + test("refuses a merged or closed PR", async () => { + for (const state of ["MERGED", "CLOSED"]) { + const pull = new FakePull(); + pull.state = state; + expect(await refused(take(PR, "iterate", undefined, pull.deps()))).toContain( + `is ${state.toLowerCase()}; a push now would bring its branch back`, + ); + expect(pull.writes).toEqual([]); + } + }); + + test("refuses a second session taking the same state, naming the holder's token", async () => { + const pull = new FakePull(); + const first = await take(PR, "merge", undefined, pull.deps()); + const message = await refused(take(PR, "merge", undefined, pull.deps())); + expect(message).toContain(`held by quest:merge, added just now as token ${first.token}`); + expect(pull.labels).toEqual(["quest:merge"]); + }); + + test("in a race for one PR, the first label added keeps it and the later one backs off", async () => { + const pull = new FakePull(); + // Both sessions see no label, then both add theirs before either rereads. + const [iterate, merge] = await Promise.allSettled([ + take(PR, "iterate", undefined, pull.deps()), + take(PR, "merge", undefined, pull.deps()), + ]); + expect(iterate).toEqual({ status: "fulfilled", value: { label: "quest:iterate", token: "LabeledEvent_1" } }); + expect(merge.status).toBe("rejected"); + expect((merge as PromiseRejectedResult).reason).toBeInstanceOf(Refusal); + expect(pull.labels).toEqual(["quest:iterate"]); + expect(pull.writes).toEqual(["add quest:iterate", "add quest:merge", "remove quest:merge"]); + }); + + test("backs off when another session's label lands between its check and its add", async () => { + const pull = new FakePull(); + pull.beforeAdd = () => { + pull.beforeAdd = undefined; + pull.label("quest:merge"); + }; + expect(await refused(take(PR, "iterate", undefined, pull.deps()))).toContain("held by quest:merge"); + expect(pull.labels).toEqual(["quest:merge"]); + }); + + test("swaps the label its token holds, and taking its own state again keeps the token", async () => { + const pull = new FakePull(); + const iterate = await take(PR, "iterate", undefined, pull.deps()); + expect(await take(PR, "iterate", iterate.token, pull.deps())).toEqual(iterate); + const merge = await take(PR, "merge", iterate.token, pull.deps()); + expect(merge).toEqual({ label: "quest:merge", token: "LabeledEvent_2" }); + expect(pull.labels).toEqual(["quest:merge"]); + expect(pull.writes).toEqual(["add quest:iterate", "add quest:merge", "remove quest:iterate"]); + }); + + test("refuses a swap once another session holds the label", async () => { + const pull = new FakePull(); + const stale = await take(PR, "iterate", undefined, pull.deps()); + pull.labels = []; + const current = await take(PR, "iterate", undefined, pull.deps()); + expect(await refused(take(PR, "merge", stale.token, pull.deps()))).toContain( + `quest:iterate was added to ${URL_} again after ${stale.token}`, + ); + expect(await refused(take(PR, "merge", "LabeledEvent_99", pull.deps()))).toContain("is not an event"); + expect(pull.labels).toEqual(["quest:iterate"]); + expect(current.token).toBe("LabeledEvent_2"); + }); +}); + +describe("ready --done", () => { + test("drops the Decisions needed section and marks the draft ready", async () => { + const pull = new FakePull(); + pull.draft = true; + pull.body = "Summary.\n\n## Decisions needed\n\n1. Pick one.\n\n## Validation\n\nTests pass."; + const { token } = await take(PR, "iterate", undefined, pull.deps()); + const hold = await finish(PR, token, pull.deps()); + expect(hold.label).toBe("quest:ready"); + expect(pull.body).toBe("Summary.\n\n## Validation\n\nTests pass."); + expect(pull.draft).toBe(false); + expect(pull.writes.slice(-2)).toEqual(["edit body", "markPullRequestReadyForReview"]); + }); +}); + +describe("merge --land", () => { + test("enables auto-merge with a merge commit by default, bound to the head", async () => { + const pull = new FakePull(); + const { token } = await take(PR, "merge", undefined, pull.deps()); + await land(PR, token, HEAD, "merge", pull.deps()); + expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} MERGE`); + expect(pull.labels).toEqual(["quest:merge"]); + }); + + test("merges at once when nothing is left to wait for, with --squash or --rebase", 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()}`); + } + }); + + test("enters the merge queue through auto-merge even when clean", async () => { + const pull = new FakePull(); + pull.mergeState = "CLEAN"; + pull.queue = true; + const warnings: string[] = []; + const { token } = await take(PR, "merge", undefined, pull.deps()); + await land(PR, token, HEAD, "squash", pull.deps(warnings)); + expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} SQUASH`); + expect(warnings).toEqual([`${URL_} lands through a merge queue, which applies its own merge method`]); + }); + + test("refuses a head that moved", async () => { + const pull = new FakePull(); + const { token } = await take(PR, "merge", undefined, pull.deps()); + pull.head = "b".repeat(40); + expect(await refused(land(PR, token, HEAD, "merge", pull.deps()))).toContain(`head moved to ${"b".repeat(40)}`); + expect(pull.writes.at(-1)).toBe("add quest:merge"); + }); + + test("a PR that left the queue unmerged drops quest:merge and waits on the user", async () => { + const pull = new FakePull(); + const { token } = await take(PR, "merge", undefined, pull.deps()); + await land(PR, token, HEAD, "merge", pull.deps()); + // Auto-merge is disabled, as when a required check fails. + pull.autoMerge = null; + const warnings: string[] = []; + expect(await refused(land(PR, token, HEAD, "merge", pull.deps(warnings)))).toContain("no longer holds it"); + expect(warnings).toEqual([ + `${URL_} left the merge queue unmerged, so quest:merge was removed; it waits on the user again`, + ]); + expect(pull.labels).toEqual([]); + expect((await take(PR, "iterate", undefined, pull.deps())).label).toBe("quest:iterate"); + }); +}); + +describe("release", () => { + test("removes the label its token holds", async () => { + const pull = new FakePull(); + const { token } = await take(PR, "iterate", undefined, pull.deps()); + expect(await release(PR, token, pull.deps())).toBe("quest:iterate"); + expect(pull.labels).toEqual([]); + }); + + test("refuses a stale token and treats a label already off as released", async () => { + const pull = new FakePull(); + const stale = await take(PR, "iterate", undefined, pull.deps()); + pull.labels = []; + const warnings: string[] = []; + expect(await release(PR, stale.token, pull.deps(warnings))).toBe("quest:iterate"); + expect(warnings).toEqual([`quest:iterate is already off ${URL_}`]); + + await take(PR, "iterate", undefined, pull.deps()); + expect(await refused(release(PR, stale.token, pull.deps()))).toContain("another session holds it"); + expect(pull.labels).toEqual(["quest:iterate"]); + }); +}); + +test("drops each Decisions needed section up to the next heading", () => { + expect(withoutHandBack("Summary.\r\n\r\n## Decisions needed\r\n\r\n1. Pick.\r\n### Detail\r\nMore.\r\n")).toBe( + "Summary.", + ); + expect(withoutHandBack("# Title\n\n## Decisions needed ##\n- one\n# Next\nkept")).toBe("# Title\n\n# Next\nkept"); +}); + +describe("arguments", () => { + async function cli(...args: string[]) { + let stderr = ""; + const out: Output = { stdout: () => undefined, stderr: (text) => void (stderr += text) }; + return { code: await main(args, out), stderr }; + } + + test.each([ + [["ready", URL_, "--done"], "--done needs the hold token"], + [["merge", URL_, "--land", HEAD], "--land needs the hold token"], + [["merge", URL_, "token", "--land", "abc"], "--land needs a full 40-character SHA"], + [["merge", URL_, "--squash"], "--squash and --rebase need --land"], + [["iterate", "https://example.com/pull/1"], "expected a GitHub pull request URL"], + ])("%j is a usage error", async (args, message) => { + const result = await cli(...args); + expect(result.code).toBe(2); + expect(result.stderr).toContain(message); + }); + + test("--squash and --rebase conflict", async () => { + expect((await cli("merge", URL_, "token", "--land", HEAD, "--squash", "--rebase")).code).toBe(2); + }); +}); From 56500a60b9dd1827da36728dd30c9e3b1b245d73 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 08:44:01 -0700 Subject: [PATCH 3/5] fix(cli): exit 0 after printing a hold token process.stdout.write returns a boolean, and print passed it through, so settle took `true` for the exit code and `quest iterate` and `release` crashed after printing on a real terminal. print and warn now return nothing, and bin.ts drops the write result to honor Output's contract. Co-Authored-By: Claude Opus 5.5 --- src/cli/bin.ts | 4 ++-- src/cli/main.ts | 9 +++++++-- test/hold.test.ts | 21 ++++++++++++++++++++- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/cli/bin.ts b/src/cli/bin.ts index a4575cd8..afe927be 100644 --- a/src/cli/bin.ts +++ b/src/cli/bin.ts @@ -11,7 +11,7 @@ process.stdout.on("error", (error: NodeJS.ErrnoException) => { }); process.exitCode = await main(process.argv.slice(2), { - stdout: (text) => process.stdout.write(text), - stderr: (text) => process.stderr.write(text), + stdout: (text) => void process.stdout.write(text), + stderr: (text) => void process.stderr.write(text), terminal: detect(process.stdout.isTTY, process.env), }); diff --git a/src/cli/main.ts b/src/cli/main.ts index c4841940..f754d26b 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -48,8 +48,13 @@ class UsageError extends Error {} export function main(argv: string[], out: Output): number | Promise { let pending: Promise | undefined; let code = 0; - const print = (line: string) => out.stdout(`${line}\n`); - const warn = (line: string) => out.stderr(`quest: ${line}\n`); + // Block bodies: process.stdout.write returns a boolean, which settle would take for the exit code. + const print = (line: string): void => { + out.stdout(`${line}\n`); + }; + const warn = (line: string): void => { + out.stderr(`quest: ${line}\n`); + }; const theme = out.terminal ? new Theme(out.terminal) : null; const program = new Command("quest") diff --git a/test/hold.test.ts b/test/hold.test.ts index d0500dad..e39e3d2e 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -1,6 +1,6 @@ // `quest iterate`, `ready`, `merge`, and `release`, against a PR replayed in memory. -import { describe, expect, test } from "vitest"; +import { describe, expect, test, vi } from "vitest"; import type { GitHubDependencies, ObjectValue } from "../src/cli/github"; import { finish, land, release, take, withoutHandBack } from "../src/cli/hold"; @@ -341,3 +341,22 @@ describe("arguments", () => { expect((await cli("merge", URL_, "token", "--land", HEAD, "--squash", "--rebase")).code).toBe(2); }); }); + +// process.stdout.write returns a boolean; printing the token once made it the exit code. +test("iterate and release exit 0 when stdout returns a value", async () => { + const pull = new FakePull(); + vi.stubEnv("GH_TOKEN", "token"); + vi.stubGlobal("fetch", pull.deps().fetch); + try { + let stdout = ""; + const out = { stdout: (text: string) => ((stdout += text), true), stderr: () => true } as Output; + expect(await main(["iterate", URL_], out)).toBe(0); + const token = stdout.trim(); + expect(pull.labels).toEqual(["quest:iterate"]); + expect(await main(["release", URL_, token], out)).toBe(0); + expect(pull.labels).toEqual([]); + } finally { + vi.unstubAllEnvs(); + vi.unstubAllGlobals(); + } +}); From d5854a9422cb3a26d679bc5da1c3a8784accef33 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 09:29:32 -0700 Subject: [PATCH 4/5] fix(cli): keep the hold token when landing fails `quest merge --land` swapped the caller's label for quest:merge before checking the head, so a stale SHA or a failed merge mutation lost the new token and left the PR held by nobody who could release it. Check the head before any label changes, and name the swapped-in hold's token in any error from the steps after a swap (land and ready --done). Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++- src/cli/hold.ts | 79 +++++++++++++++++++++++++++++++---------------- src/cli/main.ts | 2 +- test/hold.test.ts | 22 +++++++++++++ 4 files changed, 78 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 9b408412..4fc55a9e 100644 --- a/README.md +++ b/README.md @@ -395,7 +395,9 @@ branch back. The refusal names the holder's label, age, and token, so you can decide whether to release a stale hold. When two sessions add different labels at once, the first label added keeps the PR and the other backs off. -`--land` needs the full 40-character head SHA and refuses once the head moves. +`--land` needs the full 40-character head SHA and refuses once the head moves, +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 diff --git a/src/cli/hold.ts b/src/cli/hold.ts index f47b1ba0..513193ab 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -81,19 +81,20 @@ export async function take( export async function finish(pr: PullRequest, token: string, deps: GitHubDependencies): Promise { const labels = new Labels(pr, deps); const { hold, snapshot } = await acquire(labels, "ready", token); - if (HAND_BACK.test(snapshot.body)) { - await labels.github.request(`${repositoryPath(pr)}/pulls/${pr.number}`, { - method: "PATCH", - body: { body: withoutHandBack(snapshot.body) }, - }); - } - if (snapshot.draft) { - await labels.github.graphql( - `mutation($id: ID!) { markPullRequestReadyForReview(input: { pullRequestId: $id }) { clientMutationId } }`, - { id: snapshot.id }, - ); - } - return hold; + return holding(hold, async () => { + if (HAND_BACK.test(snapshot.body)) { + await labels.github.request(`${repositoryPath(pr)}/pulls/${pr.number}`, { + method: "PATCH", + body: { body: withoutHandBack(snapshot.body) }, + }); + } + if (snapshot.draft) { + await labels.github.graphql( + `mutation($id: ID!) { markPullRequestReadyForReview(input: { pullRequestId: $id }) { clientMutationId } }`, + { id: snapshot.id }, + ); + } + }); } /** @@ -109,21 +110,42 @@ export async function land( deps: GitHubDependencies, ): Promise { const labels = new Labels(pr, deps); - const { hold, snapshot } = await acquire(labels, "merge", token); - if (snapshot.head.toLowerCase() !== sha.toLowerCase()) { - throw new Refusal(`${snapshot.url} head moved to ${snapshot.head}; check the new head before landing it`); - } - if (snapshot.queue && method !== "merge") { - deps.warn(`${snapshot.url} lands through a merge queue, which applies its own merge method`); + const current = (snapshot: Snapshot) => { + if (snapshot.head.toLowerCase() !== sha.toLowerCase()) { + throw new Refusal(`${snapshot.url} head moved to ${snapshot.head}; check the new head before landing it`); + } + }; + // Check the head before swapping labels, so a stale SHA leaves the caller's token valid. + const { hold, snapshot } = await acquire(labels, "merge", token, current); + return holding(hold, async () => { + current(snapshot); + if (snapshot.queue && method !== "merge") { + deps.warn(`${snapshot.url} lands through a merge queue, which applies its own merge method`); + } + const mutation = + !snapshot.queue && IMMEDIATE.includes(snapshot.mergeState) + ? "mergePullRequest" + : "enablePullRequestAutoMerge"; + await labels.github.graphql( + `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!) { + ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha, mergeMethod: $method }) { clientMutationId } + }`, + { id: snapshot.id, sha: snapshot.head, method: method.toUpperCase() }, + ); + }); +} + +/** + * Finish `work` under `hold`, which may be a label just swapped in for the caller's. + * A failure names the hold, so the caller keeps the token it needs to release or retry. + */ +async function holding(hold: Hold, work: () => Promise): Promise { + try { + await work(); + } catch (error) { + const message = `${error instanceof Error ? error.message : String(error)}; this session holds ${hold.label} as token ${hold.token}`; + throw error instanceof Refusal ? new Refusal(message) : new Error(message, { cause: error }); } - const mutation = - !snapshot.queue && IMMEDIATE.includes(snapshot.mergeState) ? "mergePullRequest" : "enablePullRequestAutoMerge"; - await labels.github.graphql( - `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!) { - ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha, mergeMethod: $method }) { clientMutationId } - }`, - { id: snapshot.id, sha: snapshot.head, method: method.toUpperCase() }, - ); return hold; } @@ -146,11 +168,13 @@ export async function release(pr: PullRequest, token: string, deps: GitHubDepend * Take `pr` for `state`, refusing while it is merged or closed, while another * session holds a `quest:*` label, or while it is a draft its starter still * holds. With `token`, the label it holds is swapped for this state's. + * `check` refuses before any label changes. */ async function acquire( labels: Labels, state: State, token: string | undefined, + check?: (snapshot: Snapshot) => void, ): Promise<{ hold: Hold; snapshot: Snapshot }> { const label = PREFIX + state; let snapshot = await labels.read(); @@ -161,6 +185,7 @@ async function acquire( const mine = token === undefined ? undefined : held(snapshot, token); const other = snapshot.labels.find((name) => name !== mine); if (other !== undefined) throw new Refusal(holder(snapshot, other, labels.deps.now())); + check?.(snapshot); if (mine === label) return { hold: { label, token: token! }, snapshot }; if (mine === undefined && snapshot.draft && !HAND_BACK.test(snapshot.body)) { throw new Refusal( diff --git a/src/cli/main.ts b/src/cli/main.ts index 67bf134c..fed159e8 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -390,7 +390,7 @@ export function main(argv: string[], out: Output): number | Promise { "closed, or a draft its starter still holds (one whose body has no Decisions needed heading). A refusal " + "names the holder's label, age, and token; releasing a stale hold with that token is the user's choice. " + "Given a token, a command refuses unless the token is still the latest add of the PR's label, then swaps " + - "that label for its own. When two sessions add different labels at once, the first label added keeps the " + + "that label for its own; if a later step then fails, the error names the new token. When two sessions add different labels at once, the first label added keeps the " + "PR; GitHub records no event when a label already on the PR is added again, so two takes of one state " + "within the same moment are not told apart. A quest:merge label is removed once the PR leaves the merge " + "queue unmerged.\n\n" + diff --git a/test/hold.test.ts b/test/hold.test.ts index e39e3d2e..0b8eec66 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -273,6 +273,28 @@ describe("merge --land", () => { expect(pull.writes.at(-1)).toBe("add quest:merge"); }); + test("refuses a stale head before swapping, so the caller's token stays valid", async () => { + const pull = new FakePull(); + const { token } = await take(PR, "iterate", undefined, pull.deps()); + pull.head = "b".repeat(40); + expect(await refused(land(PR, token, HEAD, "merge", pull.deps()))).toContain("head moved"); + expect(pull.labels).toEqual(["quest:iterate"]); + expect((await land(PR, token, pull.head, "merge", pull.deps())).label).toBe("quest:merge"); + }); + + test("names the swapped-in hold when landing fails after the swap", async () => { + const pull = new FakePull(); + const { token } = await take(PR, "iterate", undefined, pull.deps()); + pull.beforeAdd = () => { + pull.beforeAdd = undefined; + pull.head = "b".repeat(40); + }; + const message = await refused(land(PR, token, HEAD, "merge", pull.deps())); + expect(message).toContain("head moved"); + expect(message).toContain("this session holds quest:merge as token LabeledEvent_2"); + expect(pull.labels).toEqual(["quest:merge"]); + }); + test("a PR that left the queue unmerged drops quest:merge and waits on the user", async () => { const pull = new FakePull(); const { token } = await take(PR, "merge", undefined, pull.deps()); From 1a60dc0f73fde4ef8f91757733fc088c6e3c2c0a Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 10:39:35 -0700 Subject: [PATCH 5/5] fix(runner): skip quest hold commands in kix jobs Main's runner (#188) gives kix iterate jobs the iterate skill, which now takes and releases the PR with quest iterate and quest release. Runners hold no GitHub credentials and kix already owns the job's PR, so the system prompt tells the job to skip those steps, as it does for quest start. Co-Authored-By: Claude Opus 5.5 --- src/runner/prompt.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runner/prompt.ts b/src/runner/prompt.ts index c1b3aa7d..39ef7232 100644 --- a/src/runner/prompt.ts +++ b/src/runner/prompt.ts @@ -18,7 +18,7 @@ Report only through the kix tools: - \`deliver\`: when the work is done, with a summary and the pull request's title, body, and draft flag. kix publishes the working tree and opens or updates the pull request. End your turn after delivering. - \`context\`: on a job for an issue or pull request, read it as GitHub has it now: its text and comments, and a pull request's reviews, unresolved review threads, and checks. Use it wherever the workflow reads them with \`gh\` or \`quest status\`. -Where the workflow says to claim with \`quest start\`, skip it. Where it says to push, open or update a pull request, mark it ready, or comment on it, call \`deliver\` instead. Where it says to ask or prompt the user, call \`ask\`; you can always prompt, so ignore instructions for agents that cannot. Never run \`git push\`, \`gh\`, or anything else that writes to GitHub. Commit locally if you like; kix publishes your working tree as it is.`; +Where the workflow says to claim with \`quest start\`, or to take or release a pull request with \`quest iterate\` or \`quest release\`, skip it; kix holds the job's pull request. Where it says to push, open or update a pull request, mark it ready, or comment on it, call \`deliver\` instead. Where it says to ask or prompt the user, call \`ask\`; you can always prompt, so ignore instructions for agents that cannot. Never run \`git push\`, \`gh\`, or anything else that writes to GitHub. Commit locally if you like; kix publishes your working tree as it is.`; /** * The repository's `AGENTS.md` files, which not every CLI reads (Claude Code