diff --git a/README.md b/README.md index 42b15c8d..c6d3f856 100644 --- a/README.md +++ b/README.md @@ -504,6 +504,7 @@ comes last, and a missing value prints as `-`: | `checks` | `failure` if any check fails, else `pending` if any is pending, else `success`, and `passed=`, `failing=`, `pending=` counts, all from each check's latest run; `-` when the head has no checks. | | `check` | `failing` or `pending`, `required` or `optional`, the result, details URL, and name. | | `review` | `current` or `stale` against the head, state, reviewer, time, and URL of each reviewer's latest review. | +| `checkpoint` | Each bot reviewer that approves or requests changes, such as one whose approval the base requires, its round, and its review's URL, when that review is of the head and its latest verdict there, ignoring plain comments, is neither an approval nor a dismissal, at the end of its third, sixth, ... round: [`/quest-review`'s design checkpoint](#review-locally) for an online reviewer. A round is one commit it reviewed; a dismissed review counts as one that approves or requests changes. Only printed when due. | | `reacted:` | Actor, time, and the PR or review reacted to, as `quest wait` names reactions. | | `thread` | Node ID, `current` or `outdated`, last commenter, latest comment URL, and `path:line` of each unresolved thread. | | `hold` | Label, acquisition token, acquisition time, verified GitHub event API URL, and age; `hold -` when no label holds it. Unknown acquisition facts print as `-` and `unknown age`. | @@ -518,7 +519,7 @@ own rollup state counts the superseded runs too. A required check is one branch protection or a ruleset requires for this PR. `--json` prints the same facts as one object with the fields `url`, `state`, `draft`, `head`, `mergeable`, `mergeState`, `reviewDecision`, `autoMerge`, `mergeQueue`, -`landingStopped`, `checks` (`state`, `passed`, `failing`, `pending`), `reviews`, `reactions`, +`landingStopped`, `checks` (`state`, `passed`, `failing`, `pending`), `reviews`, `checkpoints` (`reviewer`, `round`, `url`), `reactions`, `threads`, `holds` (`label`, `token`, `takenAt`, `age`, `eventUrl`), and `starterHold` (`lastActive`, `age`, `url`, or null). Ownership uses the state commands' shared reader without changing labels, and event links match the opaque acquisition token against GitHub's event listing. Long lists are paged; reactions are the newest 100 per PR or review, as in `quest wait`. Any PR state exits 0, invalid arguments exit 2, and other failures exit 1. @@ -751,11 +752,13 @@ up to floor(log2 n) further PRs that merge cleanly in that order, resolving quest links with `quest resolve`. A failed batch halves the next batch's size, down to one PR, and each batch that lands grows it back by one; the order and size persist in the state directory. Conflicting -additions stay queued. A PR whose required checks are still running stays queued -and sits out batches until they pass; only each check's latest run counts, as in -`quest status`. If any are still pending two hours after +additions stay queued. A PR whose required checks are still running, or that +still needs an approving review the base requires, stays queued and sits out +batches until they pass and it is approved; only each check's latest run counts, +as in `quest status`. If it still waits two hours after its pick (twice the default `--timeout`), the daemon comments once on that pick, -naming them, since a required check that never reports would hold it silently. The sandbox checks the tip once, without +naming what it waits for, since a check that never reports or an approval nobody +gives would hold it silently. The sandbox checks the tip once, without bisection: success lands one commit per PR, as `--method` says. A failure, or a base that conflicts with the batch's first PR, parks those PRs for the fixer: they keep their auto-merge and place, get no comment, and sit out batches while its @@ -1023,9 +1026,10 @@ watcher is the base's only writer outside an admin's emergency bypass. Its picks are those `quest land serve` reads: open PRs whose auto-merge `quest merge --land` armed, pinned to their head, in the order they were armed, -with the same ejections for a moved head, a missing pin, a blocking review, or -a failing required check. A pick still waiting on its required checks keeps its -place, and one another session holds waits, armed, until its hold clears. +with the same ejections for a moved head, a missing pin, a requested change, or +a failing required check. A pick still waiting on its required checks or a +required approval keeps its place, and one another session holds waits, armed, +until its hold clears. For the oldest pick it merges the base into the picked head with the quest merge driver, in the daemon's private repository, and pushes that merge to diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index e6028897..2fe4fe95 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -67,7 +67,9 @@ Merge the base only when the branch conflicts with it or a landing step, such as Before each push to a PR's branch, confirm the PR is still open, since a push after it merges restores the deleted branch. Before a PR is marked ready for review, a model other than its latest writer reviews the committed branch locally, as `/quest-review` describes; a later push needs a new review unless `/quest-merge` exempts its changes. -Online reviewers such as CodeRabbit are optional, and anything AI-attributed is a suggestion, not a maintainer's decision: address findings already posted on the head, never wait for or request a review, and reply to feedback you disagree with. +Anything AI-attributed, such as an online reviewer's findings, is a suggestion, not a maintainer's decision: address findings already posted on the head, reply to feedback you disagree with, and never wait for an online review, since the queue waits for any approval the base requires. +Request a review only from a reviewer whose approval the base requires and that never ran on the head. +Once you fix or refute an automated reviewer's finding, say so in its thread and resolve it, since such a reviewer may approve only after its threads are resolved. The PR body's `## Models` section records its size as `Size: M` (its quest's, or for a skill's own PR, as a quest is sized) and one `Implementer: Codex, gpt-6.1-sol, high effort` line per implementing session, moved last whenever it pushes; write the model's ID, and `unknown` for whatever the harness doesn't report. diff --git a/assets/skills/iterate.md b/assets/skills/iterate.md index 1a16936d..0227f1fc 100644 --- a/assets/skills/iterate.md +++ b/assets/skills/iterate.md @@ -16,6 +16,7 @@ Read the PR, its linked issues and quest, the reviews and comments posted on it, `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? `/quest-review`'s design checkpoint counts the rounds of each reviewer's session on the branch, including those earlier agents ran, so it may fall due in this one. +At a `checkpoint` line in `quest status`, an online reviewer's every third round with findings, stop before fixing them and take `/quest-review`'s design checkpoint. Settle every decision this PR needs with the grill skill's interview, starting from the user's feedback: those its `## Waiting` 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 description's `## Decisions` section as the paper trail, then implement them. diff --git a/assets/skills/merge.md b/assets/skills/merge.md index ee573231..0ae0f840 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -16,9 +16,11 @@ The head is landable when: - the PR is not a draft; - a review counts for it, as below; - every review finding posted on it, including automated reviewers' such as CodeRabbit or Codex, is fixed on the head or answered with a refutation, and no decision is open; -- no check failed on it, required or not, its required checks passed, other than the gate's own `quest land` check, which never reports on a head, and it does not conflict with its base. +- no check failed on it, required or not, its required checks passed, other than the gate's own `quest land` check, which never reports on a head, and it does not conflict with its base; +- no review requests changes; an approval its base requires may still be to come, since the queue waits for it. While its required checks are only pending, report that the pick stands until they pass; do not wait for them. +A change request that stands once its findings are fixed or refuted and their threads resolved is the user's call. Any other head needs `/quest-iterate`: run it, saying what the head needs, such as a review, fixes, a failed check fixed or rerun, flaky ones included, or decisions, then run this skill again once it finishes; if it stops for the user instead, report that. Run it in this session, where its interview can ask the user; under a coordinator such as `/quest-run`, return the PR to the loop, which dispatches it in a sub-agent. A PR that adds follow-ups also needs the pick round's answers recorded before queueing, as the guide's Creation section describes: ask about each follow-up that round has not asked, in a question of its own, then run `/quest-iterate` with the answers unless the head already records them. diff --git a/assets/skills/review.md b/assets/skills/review.md index 7d7816df..0fad8943 100644 --- a/assets/skills/review.md +++ b/assets/skills/review.md @@ -35,7 +35,7 @@ Refute a finding only with evidence the reviewer cannot rebut: a `path:line`, a A finding the reviewer keeps against such a refutation is a decision for the user. Write each round's `--responses` file in the checkout's `.tmp/`. -At a `next checkpoint` line, which a reviewer's every third round prints when it reports findings, stop before fixing them and reconsider the approach. +At a `next checkpoint` line, which a reviewer's every third round prints when it reports findings, stop before fixing them and reconsider the approach; `quest status ` prints a `checkpoint` line when an online reviewer's round is due the same way. Findings that keep coming usually mean the approach is wrong, not the last fix: brainstorm other designs that would avoid the whole class of findings, favoring simpler ones, and compare them with the current one. Report the alternatives 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:`, and start the next review with `quest review --new`, which restarts the count. diff --git a/assets/skills/run.md b/assets/skills/run.md index 05cd5080..ff3ef0cd 100644 --- a/assets/skills/run.md +++ b/assets/skills/run.md @@ -40,7 +40,7 @@ Run each `/quest-merge` inline; dispatch each `/quest-iterate` and `/quest-revie Dispatch `/quest-iterate` without asking for a PR that needs work but no decision, such as a conflict, failing checks, unaddressed findings, or no review that counts for its head as `/quest-merge` defines. A PR with open decisions, its `## Waiting` section's or an issue you would solve differently, goes to the user: settle them with the grill skill's rounds, then dispatch `/quest-iterate` with the answers in its brief, since a sub-agent cannot prompt. A PR you would skip or delete rather than fix, such as a superseded one, gets that question instead. -Leave a PR whose required checks are still running for a later round; an optional automated reviewer that has not posted, such as one rate-limited or skipping drafts, is not waited on. +Leave a PR whose required checks are still running for a later round; an automated reviewer is not waited on, since the queue waits for an approval the base requires. A PR whose author has write access and that changes no user-facing behavior carries a standing `/quest-merge` pick, unless the repository's `CONTRIBUTING.md` opts out: run `/quest-merge` on it without asking, and name it as auto-picked in the summary. That covers only tests, benchmarks, CI, internal docs or comments, and behavior-preserving code. Anything users or other repositories get, such as CLI behavior or output, shipped skills or rules, user-facing docs, dependencies, or release artifacts, needs the user's pick, as does adding or expanding quests. @@ -57,7 +57,7 @@ A `/quest-merge` pick covers the PR until it lands or needs the user: when `/que Order merges that would otherwise conflict. A PR that left the queue unmerged shows why under `landing-stopped` in `quest status `. Where the queue runs a fixer, such as `quest land`, it hands back only a decision: ask the user, with its handback comment's diagnosis and recommendation. -Without one, after `merge_conflict` or `failed_checks` the pick holds, even from an earlier session: dispatch `/quest-iterate`. After `checks_timed_out`, run `/quest-merge` again once; a second timeout of the same head is the user's call. +Without one, after `merge_conflict`, `failed_checks`, `changes_requested`, or `unresolved_conversations` the pick holds, even from an earlier session: dispatch `/quest-iterate`. After `checks_timed_out`, run `/quest-merge` again once; a second timeout of the same head is the user's call. Any other reason, such as a person removing it, is the user's call. ## Loop diff --git a/src/cli/land-queue.ts b/src/cli/land-queue.ts index d7fa35de..3d282c1a 100644 --- a/src/cli/land-queue.ts +++ b/src/cli/land-queue.ts @@ -115,6 +115,7 @@ interface Shown { id: number; status: View["status"]; title: string; + summary: string; url?: string; } @@ -166,6 +167,7 @@ class Queue { shown?.head === head && shown.status === view.status && shown.title === view.title && + shown.summary === view.summary && shown.url === view.url ) return; @@ -200,6 +202,7 @@ class Queue { id, status: view.status, title: view.title, + summary: view.summary, ...(view.url ? { url: view.url } : {}), }; save(shownPath(this.queue.state), this.shown); @@ -714,8 +717,19 @@ export async function ownPass( }, ); } - // A pick waiting on its own required checks keeps the check it last showed. - for (const waiting of plan.waiting ?? []) open.add(waiting.number); + // A pick waiting on its own required checks or an approval shows what, so the wait is never silent. + for (const waiting of plan.waiting ?? []) { + open.add(waiting.number); + const waits = [ + ...waiting.pending.map((name) => `the required check \`${name}\` to pass`), + ...(waiting.approval ? [`an approving review, which ${base} requires`] : []), + ]; + await own.show(waiting.number, waiting.head, { + status: "queued", + title: "Waiting", + summary: `It keeps its place and joins the queue once nothing is left to wait for: ${waits.join(", ")}.`, + }); + } await own.leave(open, "The pick ended: its auto-merge was disabled, or it closed."); if (fix) own.lines.push( diff --git a/src/cli/land-serve.ts b/src/cli/land-serve.ts index 6c0be0b5..6ea67006 100644 --- a/src/cli/land-serve.ts +++ b/src/cli/land-serve.ts @@ -313,15 +313,15 @@ async function reportOverlaps(github: GitHub, plan: Plan, state: string, overlap } /** - * How long after its pick a PR may wait on pending required checks before its author is told which. - * Twice the default check deadline (`--timeout`), so a slow but running CI rarely trips it. + * How long after its pick a PR may wait on pending required checks or an approval before its author is told + * what it waits for. Twice the default check deadline (`--timeout`), so a slow but running CI rarely trips it. */ export const STALLED_AFTER = 2 * 60 * 60_000; /** - * Comment once per pick on each PR still waiting on required checks `STALLED_AFTER` its pick, naming - * them: a check that never reports, such as one whose workflow skips the PR, would otherwise hold it - * in the queue without a word. + * Comment once per pick on each PR still waiting `STALLED_AFTER` its pick, naming what it waits for: a check + * that never reports, such as one whose workflow skips the PR, or an approval nobody gives would otherwise + * hold it in the queue without a word. */ async function reportStalled(github: GitHub, plan: Plan, state: string, now: number): Promise { const path = join(state, "stalled.json"); @@ -335,15 +335,20 @@ async function reportStalled(github: GitHub, plan: Plan, state: string, now: num const output: string[] = []; for (const waiting of plan.waiting ?? []) { if (kept[waiting.number] === waiting.enabledAt || now - Date.parse(waiting.enabledAt) < STALLED_AFTER) continue; - const list = waiting.pending.map((name) => `- ${excerpt(name)}`).join("\n"); + const list = [ + ...waiting.pending.map((name) => `- the required check ${excerpt(name)}`), + ...(waiting.approval ? [`- an approving review, which ${plan.base.ref} requires`] : []), + ].join("\n"); await github.request(`/repos/${plan.repository}/issues/${waiting.number}/comments`, { body: { - body: `quest land: this pull request is queued, but ${waiting.pending.length === 1 ? "a required check is" : "required checks are"} still pending on its head ${STALLED_AFTER / 3_600_000} hours after it was picked:\n\n${list}\n\nIt keeps its place and joins a batch once they pass. A check that never reports, such as one whose workflow does not run on this pull request, holds it here: run that check, or ask a maintainer to change the checks ${plan.base.ref} requires.`, + body: `quest land: this pull request is queued, but its head still waits ${STALLED_AFTER / 3_600_000} hours after it was picked for:\n\n${list}\n\nIt keeps its place and joins a batch once nothing is left. A check that never reports, such as one whose workflow does not run on this pull request, or an approval nobody gives holds it here: run that check or get that review, or ask a maintainer to change what ${plan.base.ref} requires.`, }, }); kept[waiting.number] = waiting.enabledAt; save(path, kept); - output.push(`stalled ${waiting.number} ${waiting.pending.join(",")}`); + output.push( + `stalled ${waiting.number} ${[...waiting.pending, ...(waiting.approval ? ["approval"] : [])].join(",")}`, + ); } save(path, kept); return output; diff --git a/src/cli/land.ts b/src/cli/land.ts index 8d495439..708f1750 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -101,7 +101,6 @@ export function pinned(body: string): string | null { export type Reason = | "merge_conflict" | "failed_checks" - | "review_required" | "changes_requested" | "unresolved_conversations" | "head_moved" @@ -150,18 +149,25 @@ export interface Plan { /** Queued PRs in enqueue order. */ queue: Entry[]; eject: Ejection[]; - /** Queued PRs whose required checks have not finished: they stay queued and sit out batches until they pass. */ + /** + * Queued PRs whose required checks have not finished or that still need a required approval: they stay + * queued and sit out batches until both are in. + */ waiting?: Waiting[]; } -/** A queued PR waiting on required checks that are still running or have not reported. */ +/** A queued PR waiting on required checks that have not reported, or on an approving review. */ export interface Waiting { number: number; url: string; - /** Its pick, from which the wait for its checks is measured. */ + /** Its pick, from which the wait is measured. */ enabledAt: string; + /** The pinned head it waits on. */ + head: string; /** The required checks still pending on its head. */ pending: string[]; + /** It still needs an approving review its base requires, which an automated reviewer may give. */ + approval: boolean; } /** A page of a commit's check runs and commit statuses, with the app behind each run. */ @@ -303,8 +309,9 @@ async function unresolved(github: GitHub, id: string): Promise { /** * Read the queue: open PRs to `base` with auto-merge enabled, in the order it * was enabled. A PR is queued while its auto-merge commit body pins its current - * head; one whose required checks are still running waits, and the rest of the - * auto-merging PRs are ejected with a reason. + * head; one whose required checks are still running, or that still needs a + * required approval, waits, and the rest of the auto-merging PRs are ejected + * with a reason. */ export async function resolveQueue( repository: Repository, @@ -417,8 +424,6 @@ export async function resolveQueue( eject("not_pinned", "auto-merge was enabled without `quest merge --land`, which pins the head to land"); } else if (approved !== head && !acceptHead?.(entry)) { eject("head_moved", `its head moved from ${approved} to ${head} after it was queued`); - } else if (node.reviewDecision === "REVIEW_REQUIRED") { - eject("review_required", "it still needs an approving review"); } else if (node.reviewDecision === "CHANGES_REQUESTED") { eject("changes_requested", "a reviewer requested changes"); } else if (resolution && (await unresolved(github, entry.id)) > 0) { @@ -428,15 +433,19 @@ export async function resolveQueue( const commit = object(object(array(object(node.commits).nodes)[0]).commit); const rollup = commit.oid === approved ? commit.statusCheckRollup : undefined; const unmet = checks(await contexts(github, repository, approved, rollup), required); + // An approval can still arrive on the pinned head, such as an automated reviewer's, so it waits like a check. + const approval = node.reviewDecision === "REVIEW_REQUIRED"; if (unmet.failing.length > 0) { eject("failed_checks", `required checks failed on its head: ${unmet.failing.join(", ")}`); - } else if (unmet.pending.length > 0) { - // Still running: it keeps its auto-merge and its place, and joins a batch once they pass. + } else if (unmet.pending.length > 0 || approval) { + // It keeps its auto-merge and its place, and joins a batch once its checks pass and it is approved. plan.waiting!.push({ number: entry.number, url: entry.url, enabledAt: entry.enabledAt, + head: entry.head, pending: unmet.pending, + approval, }); } else { plan.queue.push(entry); diff --git a/src/cli/main.ts b/src/cli/main.ts index 4bc819f6..0c5a9c2b 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -549,7 +549,10 @@ export function main(argv: string[], out: Output): number | Promise { "quest land's queue, with the reason (such as merge_conflict, failed_checks, manual, or manually_disabled), " + "actor, and time, until the PR enters the merge queue or auto-merge again. Then " + "a check line per failing or pending check, marked required or optional, counting only each check's latest run; a review line per " + - "reviewer's latest review, marked current or stale against the head; a reacted: line per " + + "reviewer's latest review, marked current or stale against the head; a checkpoint line per bot " + + "reviewer that approves or requests changes, its round, and its review, when that review is of the " + + "head, while its latest verdict on the head is neither an approval nor a dismissal, and ends its third, sixth, ... round (one commit it reviewed), as " + + "/quest-review's design checkpoint; a reacted: line per " + "reaction on the PR or a review; and a thread line per unresolved review thread, led by its node ID. " + "A missing value prints as -. --json prints the same facts as one deterministic object.\n\n" + "Uses GH_TOKEN, GITHUB_TOKEN, or gh auth token. Exits 0 for any PR state, 2 for invalid arguments, " + diff --git a/src/cli/status.ts b/src/cli/status.ts index d4c84dd0..571f9b76 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -51,7 +51,7 @@ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, } } } } reviews(first: 100, after: $reviews) @include(if: $moreReviews) { pageInfo { hasNextPage endCursor } - nodes { author { login } state submittedAt url commit { oid } reactions(last: 100) { nodes { ...reaction } } } + nodes { author { __typename login } state submittedAt url commit { oid } reactions(last: 100) { nodes { ...reaction } } } } reviewThreads(first: 100, after: $threads) @include(if: $moreThreads) { pageInfo { hasNextPage endCursor } @@ -133,6 +133,8 @@ export interface Status { checks: { state: string | null; passed: number; failing: Check[]; pending: Check[] }; /** Each reviewer's latest submitted review, oldest first. */ reviews: Review[]; + /** The design checkpoints online reviewers put on the head, as {@link checkpoints} finds them. */ + checkpoints: { reviewer: string; round: number; url: string }[]; /** Null when the `## Review` section names no full SHA. */ reviewRecord: ReviewRecord | null; reactions: Reaction[]; @@ -420,6 +422,7 @@ function summarize( }, checks, reviews: [...latest.values()], + checkpoints: checkpoints(head, submitted), reviewRecord: null, reactions, threads, @@ -428,6 +431,39 @@ function summarize( }; } +/** + * The design checkpoint `/quest-review` takes every third round, put on the head by an online reviewer: a bot that + * approves or requests changes, such as one whose approval the base requires, whose latest review is of the head + * and whose latest verdict on the head, ignoring plain comments, is not an approval or a dismissal, in its third, + * sixth, ... round. A dismissed review was an approval or a change + * request, so it shows the bot gates too, as when a push dismisses its approval. A round is one commit it reviewed; + * a rethink at a checkpoint leaves the next one three rounds on, so nothing restarts the count. + */ +const GATING = ["APPROVED", "CHANGES_REQUESTED", "DISMISSED"]; + +function checkpoints(head: string, submitted: ObjectValue[]): Status["checkpoints"] { + const due: Status["checkpoints"] = []; + const rounds = new Map(); + for (const row of submitted) { + if (row.author == null || object(row.author).__typename !== "Bot") continue; + const reviewer = actor(row.author); + rounds.set(reviewer, [...(rounds.get(reviewer) ?? []), row]); + } + for (const [reviewer, rows] of rounds) { + if (!rows.some((row) => GATING.includes(string(row.state)))) continue; + const counted = rows.filter((row) => row.commit != null); + const last = counted.at(-1); + if (last === undefined || string(object(last.commit).oid) !== head) continue; + const verdict = counted + .filter((row) => string(object(row.commit).oid) === head && GATING.includes(string(row.state))) + .at(-1); + if (verdict?.state === "APPROVED" || verdict?.state === "DISMISSED") continue; + const round = new Set(counted.map((row) => string(object(row.commit).oid))).size; + if (round % 3 === 0) due.push({ reviewer, round, url: string(last.url) }); + } + return due; +} + /** * The reason in a `quest land` ejection comment, or null for any other comment. Only an app or a * person with write access speaks for the gate. @@ -489,6 +525,7 @@ export function statusLines(status: Status): string[] { ...status.reviews.map( (r) => `review ${r.current ? "current" : "stale"} ${r.state} ${r.reviewer} ${r.submittedAt} ${r.url}`, ), + ...status.checkpoints.map((c) => `checkpoint ${c.reviewer} round ${c.round} ${c.url}`), ...status.reactions.map((r) => `reacted:${r.content} ${r.actor} ${r.createdAt} ${r.url}`), ...status.threads.map( (t) => diff --git a/test/land.test.ts b/test/land.test.ts index 66063753..a497eebe 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1072,7 +1072,7 @@ describe.skipIf(process.platform === "win32")("quest land", () => { ).toBe(false); }); - test("ejects auto-merging PRs that are not queued or not ready, in enqueue order, and waits on running checks", async () => { + test("ejects auto-merging PRs that are not queued or not ready, in enqueue order, and waits on running checks and approvals", async () => { const forge = new Forge(); forge.pull( "Unpinned", @@ -1081,26 +1081,35 @@ describe.skipIf(process.platform === "win32")("quest land", () => { ); const moved = forge.pull("Moved", { "2.txt": "2\n" }); moved.autoMerge = { ...moved.autoMerge!, commitBody: pin("f".repeat(40)) }; - forge.pull("Unreviewed", { "3.txt": "3\n" }, { pull: { review: "REVIEW_REQUIRED" } }); + const unreviewed = forge.pull("Unreviewed", { "3.txt": "3\n" }, { pull: { review: "REVIEW_REQUIRED" } }); forge.pull("Red", { "4.txt": "4\n" }, { pull: { runs: [{ name: "test", conclusion: "FAILURE", url: "u" }] } }); - forge.pull("Running", { "5.txt": "5\n" }, { pull: { runs: [] } }); + const running = forge.pull("Running", { "5.txt": "5\n" }, { pull: { runs: [] } }); forge.pull("Ready", { "6.txt": "6\n" }); forge.pull("Not landing", { "7.txt": "7\n" }, { pull: { landing: false } }); const plan = await resolveQueue(REPOSITORY, { base: "main", method: "squash" }, await forge.deps()); expect(plan.queue.map((entry) => entry.number)).toEqual([6]); expect(plan.eject.map((ejected) => [ejected.number, ejected.reason])).toEqual([ [2, "head_moved"], - [3, "review_required"], [4, "failed_checks"], [1, "not_pinned"], ]); - // Its required checks are still running: it stays queued, keeping its auto-merge, and sits out batches. + // Its approval or its required checks are still to come: it stays queued, keeping its auto-merge, and sits out batches. expect(plan.waiting).toEqual([ + { + number: 3, + url: "https://github.com/upstream/project/pull/3", + enabledAt: "2026-10-26T00:00:03Z", + head: unreviewed.head, + pending: [], + approval: true, + }, { number: 5, url: "https://github.com/upstream/project/pull/5", enabledAt: "2026-10-26T00:00:05Z", + head: running.head, pending: ["test"], + approval: false, }, ]); }); @@ -1269,9 +1278,8 @@ describe.skipIf(process.platform === "win32")("quest land", () => { expect(plan.queue.map((entry) => entry.number)).toEqual([1]); expect(plan.eject.map((ejected) => [ejected.number, ejected.reason])).toEqual([ [3, "unresolved_conversations"], - [4, "review_required"], ]); - expect(plan.waiting?.map(({ number }) => number)).toEqual([2]); + expect(plan.waiting?.map(({ number }) => number)).toEqual([2, 4]); }); test("refuses classic protection settings it does not uphold, or cannot read", async () => { @@ -1680,7 +1688,7 @@ describe.skipIf(process.platform === "win32")("host daemon", () => { expect(await round(picked + STALLED_AFTER - 1)).toEqual([]); expect(forge.comment(1)).toBeUndefined(); expect(await round(picked + STALLED_AFTER)).toEqual(["stalled 1 test"]); - expect(forge.comment(1)).toContain("test"); + expect(forge.comment(1)).toContain("the required check test"); expect(forge.comment(1)).toContain("2 hours after it was picked"); // It stays queued, and is told only once for this pick. expect(await round(picked + 2 * STALLED_AFTER)).toEqual([]); @@ -1694,6 +1702,21 @@ describe.skipIf(process.platform === "win32")("host daemon", () => { expect(silent.comments).toHaveLength(2); }); + test("waits on a required approval like a check, and names it once the wait stalls", async () => { + const forge = new Forge(); + const unapproved = forge.pull("Unapproved", { "1.txt": "1\n" }, { pull: { review: "REVIEW_REQUIRED" } }); + const options = sandbox(); + const root = forge.clone(); + const picked = Date.parse(unapproved.autoMerge!.enabledAt); + const deps = { ...(await forge.deps()), now: () => picked + STALLED_AFTER }; + const plan = await resolveQueue(REPOSITORY, SQUASH, deps); + expect(plan.queue).toEqual([]); + expect(plan.eject).toEqual([]); + expect(await serveBatch(root, plan, options, deps, async () => deps.token)).toEqual(["stalled 1 approval"]); + expect(forge.comment(1)).toContain("an approving review, which main requires"); + expect(unapproved.landing).toBe(true); + }); + test("collects finished fixer jobs while a check runs", async () => { const forge = new Forge(); forge.pull("One", { "one.txt": "yes" }); @@ -4647,6 +4670,30 @@ describe.skipIf(process.platform === "win32")("the watcher's own queue", () => { expect(queued(forge, 1)).toBeNull(); }); + test("a pick waiting on a required approval shows it, and builds once approved", async () => { + const { forge, pass } = setup(); + const pick = forge.pull("Unapproved", { "one.txt": "1\n" }, { pull: { review: "REVIEW_REQUIRED" } }); + expect(await pass()).toEqual([]); + expect(shown(forge)).toEqual({ 1: "queued Waiting" }); + const run = forge.repo.checkRuns!.filter((item) => item.name === QUEUE_CHECK).at(-1)!; + expect(String(run.output.summary)).toContain("an approving review, which main requires"); + pick.review = "APPROVED"; + expect(await pass()).toEqual(["building 1"]); + }); + + test("a waiting pick's check follows what it still waits for", async () => { + const { forge, pass } = setup(); + const pick = forge.pull("Both", { "one.txt": "1\n" }, { pull: { review: "REVIEW_REQUIRED", runs: [] } }); + const summary = () => + String(forge.repo.checkRuns!.filter((item) => item.name === QUEUE_CHECK).at(-1)!.output.summary); + await pass(); + expect(summary()).toContain("the required check `test` to pass, an approving review, which main requires"); + pick.review = "APPROVED"; + await pass(); + expect(summary()).toContain("the required check `test` to pass."); + expect(summary()).not.toContain("approving review"); + }); + test("a held pick keeps its place unbuilt, and stale queue branches and their runs are cleared on the first pass", async () => { const { forge, pass } = setup(); const one = forge.pull("One", { "one.txt": "1\n" }); diff --git a/test/status.test.ts b/test/status.test.ts index ba7a8f83..51da6e17 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -691,3 +691,78 @@ describe("the review record", () => { expect(await line(`Fixes ${A}.\n\n## Review\n\nSkipped.\n\n## Models\n\n${C}`)).toBe("review-record -"); }); }); + +describe("checkpoint", () => { + const C = "c".repeat(40); + const bot = (login: string, state: string, commit: string, minute: number) => ({ + author: { __typename: "Bot", login }, + state, + submittedAt: `2026-10-07T12:${String(minute).padStart(2, "0")}:00Z`, + url: `${URL_}#pullrequestreview-${minute}`, + commit: { oid: commit }, + reactions: { nodes: [] }, + }); + const line = async (reviews: Node[]) => + statusLines( + await pullRequestStatus( + pr, + setup([ + pull({ + checks: null, + reviews: page(reviews), + threads: page([]), + }), + ]).deps, + ), + ).filter((entry) => entry.startsWith("checkpoint ")); + + test("falls due when an approving reviewer's third round leaves findings on the head", async () => { + const rounds = [ + bot("pullfrog", "CHANGES_REQUESTED", B, 1), + bot("pullfrog", "CHANGES_REQUESTED", C, 2), + bot("pullfrog", "CHANGES_REQUESTED", A, 3), + ]; + expect(await line(rounds)).toEqual([`checkpoint pullfrog round 3 ${URL_}#pullrequestreview-3`]); + // An approval or a dismissed review of the head, a second review of one commit, or an unreviewed head is none. + expect(await line([...rounds.slice(0, 2), bot("pullfrog", "APPROVED", A, 3)])).toEqual([]); + expect(await line([...rounds.slice(0, 2), bot("pullfrog", "DISMISSED", A, 3)])).toEqual([]); + expect(await line([...rounds.slice(0, 2), bot("pullfrog", "CHANGES_REQUESTED", C, 3)])).toEqual([]); + expect(await line([...rounds, bot("pullfrog", "COMMENTED", A, 4)])).toEqual([ + `checkpoint pullfrog round 3 ${URL_}#pullrequestreview-4`, + ]); + // A reviewer whose only approval a push dismissed still gates, and a comment after approving the head is none. + const dismissed = [ + bot("pullfrog", "DISMISSED", B, 1), + bot("pullfrog", "COMMENTED", C, 2), + bot("pullfrog", "COMMENTED", A, 3), + ]; + expect(await line(dismissed)).toEqual([`checkpoint pullfrog round 3 ${URL_}#pullrequestreview-3`]); + expect( + await line([...rounds.slice(0, 2), bot("pullfrog", "APPROVED", A, 3), bot("pullfrog", "COMMENTED", A, 4)]), + ).toEqual([]); + // The latest verdict on the head wins: a change request after an approval of it is due. + expect( + await line([ + ...rounds.slice(0, 2), + bot("pullfrog", "APPROVED", A, 3), + bot("pullfrog", "CHANGES_REQUESTED", A, 4), + ]), + ).toEqual([`checkpoint pullfrog round 3 ${URL_}#pullrequestreview-4`]); + // Every reviewer due on the head is named. + const other = [B, C, A].map((commit, index) => bot("other", "CHANGES_REQUESTED", commit, index + 5)); + expect(await line([...rounds, ...other])).toEqual([ + `checkpoint pullfrog round 3 ${URL_}#pullrequestreview-3`, + `checkpoint other round 3 ${URL_}#pullrequestreview-7`, + ]); + }); + + test("ignores reviewers that only comment, and people", async () => { + const commented = [B, C, A].map((commit, index) => bot("coderabbitai", "COMMENTED", commit, index + 1)); + expect(await line(commented)).toEqual([]); + const person = [B, C, A].map((commit, index) => ({ + ...bot("alice", "CHANGES_REQUESTED", commit, index + 1), + author: { __typename: "User", login: "alice" }, + })); + expect(await line(person)).toEqual([]); + }); +});