diff --git a/README.md b/README.md index 14f60cc8..64c555b9 100644 --- a/README.md +++ b/README.md @@ -431,8 +431,12 @@ It enables auto-merge bound to that SHA, which enters GitHub's merge queue when the base has one, or merges at once when nothing is left to wait for. It makes a merge commit; `--squash` or `--rebase` picks another method, and a merge queue applies its own. On a base that requires the `quest land` check, it -queues the PR for [`quest land`](#land-a-batch-with-quest-land) instead. `quest:merge` stays on while the PR is queued or -auto-merging; once the PR leaves the queue unmerged, the next command removes +queues the PR for [`quest land`](#land-a-batch-with-quest-land) instead. +On an ungated base, it compares the live base against that head and refuses a +head that is behind or diverged: merge the base, rerun the repository's checks, +and land the new head. This is a preflight check; GitHub can still move the base +between that comparison and its head-pinned merge mutation. `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. diff --git a/assets/AGENTS.md b/assets/AGENTS.md index a85cf862..6762cf2f 100644 --- a/assets/AGENTS.md +++ b/assets/AGENTS.md @@ -170,6 +170,13 @@ Any other change, including a base merge with a hand-resolved conflict, waits fo The repository's review policy decides whose review counts. Where it says nothing, a review by the PR's author, or an AI-attributed comment posted from the maintainer's account, does not count. +Checkpoint the design across review rounds. +A review round is one pushed head that drew reviews, counted from the PR's full history since its last comment that starts with `Design change:`. +Read the full history with `gh pr view --json reviews,comments`. +Every fourth round, stop before fixing more findings and reconsider the approach. +Report that judgment and what remains, recommending to continue, rethink the approach, merge as is, or drop the PR, and let the user decide. +Record each design change, including a rethink the user picks, in a PR comment that starts with `Design change:`; the next agent counts rounds from it. + Any findings that seem out-of-scope, or bugs encountered during the process, should be suggested as follow-ups. ## Deletion diff --git a/assets/skills/complete.md b/assets/skills/complete.md index 81cb439c..6d37022f 100644 --- a/assets/skills/complete.md +++ b/assets/skills/complete.md @@ -9,7 +9,7 @@ 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. +Each merge is performed in parallel by a sub-agent, following the guide's Reviews section for the shared design checkpoint across review rounds. Ask synchronously with one question per PR, never grouping PRs into one question (a prompt may hold a few questions), following the guide's Questions section and including your recommendation: - `/quest-ready`: Are drafts ready for review. diff --git a/assets/skills/iterate.md b/assets/skills/iterate.md index 5beb09cd..4e892e86 100644 --- a/assets/skills/iterate.md +++ b/assets/skills/iterate.md @@ -13,10 +13,7 @@ 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. `quest status ` reports its checks, reviews by head, and unresolved threads in one call. Judge the approach before touching anything: would you take a different one? -Checkpoint the design across review rounds, as the merge skill does. -A review round is one pushed head that drew reviews, counted from the PR's full history (`gh pr view --json reviews,comments`) since its last comment that starts with `Design change:`. -On every fourth round, report your judgment of the approach and what remains before fixing more findings, recommending to continue, rethink the approach, or drop the PR; that choice is the user's. -Record a design change you make, including a rethink the user picks, in a PR comment that starts with `Design change:`. +Follow the guide's Reviews section for the shared design checkpoint across review rounds. Settle every decision this PR needs with the grill skill's interview, starting from the user's feedback: those its `## Decisions needed` section lists, those your judgment of the approach or the review findings raise, and any you find along the way. Record the settled decisions in this PR's description as the paper trail, then implement them. diff --git a/assets/skills/merge.md b/assets/skills/merge.md index e2a61f62..62ff1ac2 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -13,12 +13,7 @@ Hand it back with `quest release ` whenever you stop before it lands 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. Address any review findings, fixing those you agree with and leaving a comment on any you disagree with. -Automated reviewers often find something new on every push, so keep fixing their findings without asking, but checkpoint the design. -A review round is one pushed head that drew reviews; count rounds from the PR, not your session, since its last design change (one that reshapes the approach; fixing a finding is not one). -`quest status` shows only each reviewer's latest review, so read the full history with `gh pr view --json reviews,comments`. -Every fourth round, stop before the next fix and reconsider the design: is something fundamentally wrong with the approach? -Report that judgment and what remains unresolved, recommending to continue, rethink the approach, merge as is, or drop the PR, and let the user decide. -Record each design change, including a rethink the user picks, in a PR comment that starts with `Design change:`; the next agent counts rounds from it. +Keep fixing automated review findings, following the guide's Reviews section for the shared design checkpoint across rounds. Never request a review, including by toggling a ready PR to draft and back. Abort the merge if there is a significant decision to be made. @@ -34,8 +29,9 @@ or `checks:passed`, so no separate check watcher is needed. Inspect the events and decide whether they satisfy the wait; the command does not decide whose review counts. If activity is irrelevant (including bot notices), call it again with `--since` set to the last field of -its last line. Exit 3 means the deadline passed with no activity; re-run with -`--since` set to the printed cursor, so nothing during the first wait is missed. +its last line. Exit 3 means the deadline passed with no activity. +If waiting for a required review of a changed head, stop and ask the user, as the guide's Reviews section requires; do not start another wait for that review. +For other waits, re-run with `--since` set to the printed cursor, so nothing during the first wait is missed. Other failures must be reported rather than treated as activity. Do not wait for optional reviews. Run each wait so its exit wakes you, such as a background task that notifies on @@ -44,7 +40,7 @@ foreground time limit; each wake rereads your whole context. The hour bound keeps that reread cached; a wake after the prompt cache expires rewrites it all. A headless process ends when its turn ends, killing its background commands: 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. +and apply the timeout rule above. Merge the PR *only* after all outstanding decisions have been confirmed. Before landing it, fetch the PR's base branch and head, check out the head, and run `quest --root check --base / --branch `. @@ -54,7 +50,8 @@ On a finding, fix the branch: delete the finished quest with `quest rm`, or rewr Then push, re-check, and continue; ask the user only when what remains is unclear. 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. -On a base that requires the `quest land` check, it queues the PR for `quest land`, the repository's batched merge gate, which squashes. +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; do not merge the base or rerun checks per PR just to queue it. +On an ungated base, the command refuses a head that does not contain the current base. Merge the base locally (never rebase), rerun the repository's checks, push, and land the new head after satisfying the guide's review rule for those changes. The `quest:merge` label stays on while the PR is queued or auto-merging. Report once it is landing; do not wait for it to merge, since the queue re-checks it without you. A PR that left the queue unmerged or lost auto-merge shows why under `landing-stopped` in `quest status `. diff --git a/quest/m0/helpers/README.md b/quest/m0/helpers/README.md index 55b7af5e..7ea5e699 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -67,7 +67,6 @@ commands from Sep-Oct. Session counts in each child come from that window. ## Required - [Merge state race](/quest/m0/helpers/merge-state-race.md) - `--land` and merge jobs land a PR whose merge state GitHub is still computing, on the first try -- [Landing rules](/quest/m0/helpers/landing.md) - land only heads that contain the current base, ask on a missing review, one design checkpoint - [Wait and status accuracy](/quest/m0/helpers/wait-accuracy.md) - wake on edited bot comments, honest `checks:passed`, review rounds in `quest status` - [Status holds](/quest/m0/helpers/status-holds.md) - `quest status` shows a PR's `quest:*` hold - [Headless agents](/quest/m0/helpers/headless-agents.md) - `/quest-spawn api` launches, resumes, and stops billed agents through one command auto mode allows diff --git a/quest/m0/helpers/land-adopt.md b/quest/m0/helpers/land-adopt.md index 69c486f0..51cb8b26 100644 --- a/quest/m0/helpers/land-adopt.md +++ b/quest/m0/helpers/land-adopt.md @@ -10,8 +10,7 @@ land around it. A PR from a fork lands through it too. ## Plan Decided 2026-10-26 (user): the repository adopts its own gate first. Being -user-owned, it cannot use GitHub's merge queue -([landing](/quest/m0/helpers/landing.md)). +user-owned, it cannot use GitHub's merge queue. [kixelated/quest#236](https://github.com/kixelated/quest/pull/236) added the Land workflow, a test that only its land job uses the `land` environment, and diff --git a/quest/m0/helpers/landing.md b/quest/m0/helpers/landing.md deleted file mode 100644 index 018f047f..00000000 --- a/quest/m0/helpers/landing.md +++ /dev/null @@ -1,41 +0,0 @@ -# [S] Land only heads tested against the current base - -## Goal - -A PR lands only on a head that contains the base branch as it is at landing -time, and the merge skill stops for the user, rather than waiting again, when -a required review of a changed head does not arrive. The merge skill, iterate, -and complete share one definition of the design checkpoint. - -## Plan - -Decided 2026-10-24 (user) from the merge wave that landed -[kixelated/quest#205](https://github.com/kixelated/quest/pull/205) through -[#219](https://github.com/kixelated/quest/pull/219). The user asked for these -skill and guide edits through this plan. - -- **Refuse a stale head (user).** GitHub reported - [kixelated/quest#213](https://github.com/kixelated/quest/pull/213) clean, - but landing it would have broken a test #205 had just added. `quest merge - --land ` refuses when the head does not contain the base branch's - current tip, naming both. The merge skill then merges the base (never - rebasing), reruns the repository's checks, and lands the new head; a - conflict-free base merge still needs no new review. Rejected: skill prose - alone, and GitHub's "require branches to be up to date" protection. - GitHub merge queues need an organization-owned repository, which - kixelated/quest is not. -- **Ask on a missing review.** `assets/skills/merge.md` says to rerun - `quest wait` with `--since` after a timeout. When the wait is for a - required review of a changed head, ask the user instead, as the guide's - Reviews section already says - ([kixelated/quest#206](https://github.com/kixelated/quest/pull/206)). -- **One checkpoint.** Move the every-fourth-round design checkpoint that - [kixelated/quest#207](https://github.com/kixelated/quest/pull/207) wrote - into both `merge` and `iterate` into the guide's Reviews section, and point - `merge`, `iterate`, and `complete` at it. -- Test a stale head refused, the same head landing after a base merge, and a - head the base already contains. - -## Related - -- [Wait and status accuracy](/quest/m0/helpers/wait-accuracy.md) - the review-round count the checkpoint reads diff --git a/quest/m0/helpers/merge-state-race.md b/quest/m0/helpers/merge-state-race.md index 2be22888..95369486 100644 --- a/quest/m0/helpers/merge-state-race.md +++ b/quest/m0/helpers/merge-state-race.md @@ -35,7 +35,3 @@ GitHub has not computed yet reads `UNKNOWN` and falls through to - `land()` sends a base gated by `quest land` to auto-merge ahead of this choice ([kixelated/quest#229](https://github.com/kixelated/quest/pull/229)), so it applies only to ungated bases. - -## Related - -- [Landing rules](/quest/m0/helpers/landing.md) - also changes what `quest merge --land` refuses diff --git a/quest/m0/helpers/queue-siblings.md b/quest/m0/helpers/queue-siblings.md index 5d924a25..55fdf579 100644 --- a/quest/m0/helpers/queue-siblings.md +++ b/quest/m0/helpers/queue-siblings.md @@ -47,7 +47,3 @@ Open (this quest decides, with a recommendation to the user): Whatever it picks must keep `quest check`'s rule that a finished quest leaves no dangling reference once all siblings land. Test it with two sibling PRs landing through a merge that does not run the driver. - -## Related - -- [Land only heads tested against the current base](/quest/m0/helpers/landing.md) - the same landing path diff --git a/quest/m0/helpers/wait-accuracy.md b/quest/m0/helpers/wait-accuracy.md index b5abeab2..c8dd5360 100644 --- a/quest/m0/helpers/wait-accuracy.md +++ b/quest/m0/helpers/wait-accuracy.md @@ -31,5 +31,4 @@ Decided 2026-10-24 (user) from the merge wave that landed ## Related -- [Landing rules](/quest/m0/helpers/landing.md) - the checkpoint that reads the round count - [Status holds](/quest/m0/helpers/status-holds.md) - also extends `quest status` diff --git a/src/cli/hold.ts b/src/cli/hold.ts index 4d035f1b..88b1f64e 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -146,6 +146,20 @@ export async function land( ); return; } + // The batch gate tests its own current-base chain. Ungated landings need that base in the PR head. + const comparison = object( + await ( + await labels.github.request( + `${repositoryPath(pr)}/compare/${encodeURIComponent(snapshot.base)}...${snapshot.head}?per_page=1`, + ) + ).json(), + ); + if (comparison.status !== "ahead" && comparison.status !== "identical") { + const base = string(object(comparison.base_commit).sha); + throw new Refusal( + `${snapshot.url} head ${snapshot.head} does not contain ${snapshot.base} at ${base}; merge the base and rerun the repository's checks before landing`, + ); + } if (snapshot.queue && method !== "merge") { deps.warn(`${snapshot.url} lands through a merge queue, which applies its own merge method`); } diff --git a/test/hold.test.ts b/test/hold.test.ts index b6b67199..5ead9082 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -10,6 +10,7 @@ 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); +const BASE = "b".repeat(40); interface Event { __typename: "LabeledEvent" | "AutoMergeEnabledEvent" | "AddedToMergeQueueEvent"; @@ -26,6 +27,9 @@ class FakePull { head = HEAD; mergeState = "BLOCKED"; queue = false; + comparison = "ahead"; + base = BASE; + readonly comparisons: string[] = []; /** Check names the base branch's rulesets require. */ required: string[] = []; autoMerge: ObjectValue | null = null; @@ -85,6 +89,10 @@ class FakePull { return json(200, []); } if (url.pathname === "/repos/example/project/branches/main") return json(200, { protection: {} }); + if (url.pathname === `/repos/example/project/compare/main...${this.head}`) { + this.comparisons.push(`${url.pathname}${url.search}`); + return json(200, { status: this.comparison, base_commit: { sha: this.base } }); + } if (url.pathname === "/repos/example/project/rules/branches/main") { const checks = this.required.map((context) => ({ context })); return json( @@ -251,6 +259,31 @@ describe("ready --done", () => { }); describe("merge --land", () => { + test.each(["behind", "diverged"])( + "refuses a stale ungated head (%s), then accepts it after a base merge", + async (status) => { + const pull = new FakePull(); + pull.comparison = status; + const { token } = await take(PR, "merge", undefined, pull.deps()); + const refusal = await refused(land(PR, token, HEAD, "squash", pull.deps())); + expect(refusal).toContain(`head ${HEAD} does not contain main at ${BASE}`); + expect(pull.writes.at(-1)).toBe("add quest:merge"); + pull.head = "c".repeat(40); + pull.comparison = "ahead"; + await land(PR, token, pull.head, "squash", pull.deps()); + expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${pull.head} SQUASH`); + }, + ); + + test("accepts a head identical to the current base", async () => { + const pull = new FakePull(); + pull.comparison = "identical"; + pull.base = HEAD; + 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`); + }); + 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()); @@ -283,11 +316,13 @@ describe("merge --land", () => { test("queues a gated base's PR for quest land: squash auto-merge pinning the head", async () => { const pull = new FakePull(); pull.required = ["test", "quest land"]; + pull.comparison = "diverged"; const warnings: string[] = []; const { token } = await take(PR, "merge", undefined, pull.deps()); await land(PR, token, HEAD, "merge", pull.deps(warnings)); expect(pull.writes.at(-1)).toBe(`enablePullRequestAutoMerge ${HEAD} SQUASH Quest-Land-Head: ${HEAD}`); expect(warnings).toEqual([`${URL_} lands through quest land, which squashes`]); + expect(pull.comparisons).toEqual([]); }); test("refuses a head that moved", async () => {