From d44c9132170e6b2868976b6bcbd75cdafe755e91 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 10:49:30 -0800 Subject: [PATCH 01/10] docs(context): wait for an online review whose approval the base requires Repositories may now require an automated reviewer's approval, such as Pullfrog's, to merge. Agents wait for that approval instead of never waiting on online reviews, and resolve the threads they fix or refute so the reviewer can approve. Co-Authored-By: Claude Opus 5.5 --- assets/CONTEXT.md | 4 +++- assets/skills/merge.md | 5 +++-- assets/skills/run.md | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index e6028897..7345cb5d 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, and reply to feedback you disagree with. +Wait for an online review only when the base branch requires its approval, and request one only when its reviewer never ran on the head. +Resolve an automated reviewer's thread once you fix or refute its finding, 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/merge.md b/assets/skills/merge.md index 3814327c..3ef82a2a 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -16,9 +16,10 @@ 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; +- its review decision does not block it: any approval its base requires is in, and no review requests changes. -While its required checks are only pending, report that the pick stands until they pass; do not wait for them. +While its required checks, or a required approval with no finding left to address, are only pending, report that the pick stands until they arrive; do not wait for them. 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/run.md b/assets/skills/run.md index caf1c817..b6eaf0e9 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 or required approval are still pending for a later round; an automated reviewer whose approval the base does not require, such as one rate-limited or skipping drafts, is not waited on. 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. From bcd3d3cbaa3f37ee56082801d98e2fe0844c63f4 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 11:54:23 -0800 Subject: [PATCH 02/10] feat(land): hold a pick awaiting a required approval instead of ejecting it An automated reviewer such as Pullfrog can approve a queued head, so the queue waits for that approval as it waits for pending checks, and names it in the stall comment. The merge skill queues such a head instead of leaving it for a round that never comes, and review-caused ejections go to /quest-iterate. Co-Authored-By: Claude Opus 5.5 --- README.md | 17 ++++++++++------- assets/CONTEXT.md | 6 +++--- assets/skills/merge.md | 5 +++-- assets/skills/run.md | 4 ++-- src/cli/land-serve.ts | 21 +++++++++++++-------- src/cli/land.ts | 21 +++++++++++++-------- test/land.test.ts | 33 +++++++++++++++++++++++++++------ 7 files changed, 71 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 33904db8..69e4bb4e 100644 --- a/README.md +++ b/README.md @@ -744,11 +744,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 @@ -1016,9 +1018,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 7345cb5d..2fe4fe95 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -67,9 +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. -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, and reply to feedback you disagree with. -Wait for an online review only when the base branch requires its approval, and request one only when its reviewer never ran on the head. -Resolve an automated reviewer's thread once you fix or refute its finding, since such a reviewer may approve only after its threads are resolved. +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/merge.md b/assets/skills/merge.md index 3ef82a2a..89ccc8d5 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -17,9 +17,10 @@ The head is landable when: - 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; -- its review decision does not block it: any approval its base requires is in, and no review requests changes. +- no review requests changes; an approval its base requires may still be to come, since the queue waits for it. -While its required checks, or a required approval with no finding left to address, are only pending, report that the pick stands until they arrive; do not wait for them. +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/run.md b/assets/skills/run.md index b6eaf0e9..2eafaa49 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 or required approval are still pending for a later round; an automated reviewer whose approval the base does not require, 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-serve.ts b/src/cli/land-serve.ts index b2415e37..93c37799 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 + * which. 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..1928f755 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,23 @@ 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 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. */ @@ -417,8 +421,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 +430,18 @@ 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, pending: unmet.pending, + approval, }); } else { plan.queue.push(entry); diff --git a/test/land.test.ts b/test/land.test.ts index 26f0e732..9aadac36 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1012,7 +1012,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", @@ -1030,17 +1030,24 @@ describe.skipIf(process.platform === "win32")("quest land", () => { 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", + pending: [], + approval: true, + }, { number: 5, url: "https://github.com/upstream/project/pull/5", enabledAt: "2026-10-26T00:00:05Z", pending: ["test"], + approval: false, }, ]); }); @@ -1209,9 +1216,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 () => { @@ -1620,7 +1626,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([]); @@ -1634,6 +1640,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" }); From 776385d277b2e6714ff8e561478ae92248d40cb3 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 12:36:33 -0800 Subject: [PATCH 03/10] docs(land): say resolveQueue holds a pick awaiting approval Co-Authored-By: Claude Opus 5.5 --- src/cli/land.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cli/land.ts b/src/cli/land.ts index 1928f755..d920794b 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -307,8 +307,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, From 65cbf78ff619976485cda6a99912582f3c9ee60d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 13:12:48 -0800 Subject: [PATCH 04/10] feat(status): print an online reviewer's design checkpoint A bot reviewer that approves PRs, such as Pullfrog, reviews every push, so its rounds count toward /quest-review's every-third-round design checkpoint: quest status prints a checkpoint line when its third, sixth, ... round since the latest Design change comment leaves findings on the head, and /quest-iterate and /quest-review stop on it. Co-Authored-By: Claude Opus 5.5 --- README.md | 3 +- assets/skills/iterate.md | 2 +- assets/skills/review.md | 2 +- src/cli/main.ts | 5 ++- src/cli/status.ts | 42 +++++++++++++++++++++++- test/status.test.ts | 70 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 119 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 69e4bb4e..66e35733 100644 --- a/README.md +++ b/README.md @@ -497,6 +497,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` | A 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, is not an approval, and ends its third, sixth, ... round: [`/quest-review`'s design checkpoint](#review-locally) for an online reviewer. A round is one commit it reviewed since the PR's latest `Design change:` comment. 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`. | @@ -511,7 +512,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`, `checkpoint` (`reviewer`, `round`, `url`, or null), `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. diff --git a/assets/skills/iterate.md b/assets/skills/iterate.md index 1a16936d..79438c0c 100644 --- a/assets/skills/iterate.md +++ b/assets/skills/iterate.md @@ -15,7 +15,7 @@ Your `Implementer:` line ends with the size your brief gives the work, as `Imple 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? -`/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. +`/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; a `checkpoint` line in `quest status` is one an online reviewer's rounds put on the head. 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/review.md b/assets/skills/review.md index 7d7816df..4f1dfa93 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, or a `checkpoint` line in `quest status `, which an online reviewer's does, stop before fixing them and reconsider the approach. 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/src/cli/main.ts b/src/cli/main.ts index 90cfef1f..3e840765 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -545,7 +545,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 naming a bot " + + "reviewer that approves PRs, its round, and its review, when that review is of the head, approves " + + "nothing, and ends its third, sixth, ... round (one reviewed commit since the latest `Design change:` " + + "comment), 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..2819eba2 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[]; + /** An online reviewer's design checkpoint on the head, as {@link checkpoint} finds it. */ + checkpoint: { reviewer: string; round: number; url: string } | null; /** Null when the `## Review` section names no full SHA. */ reviewRecord: ReviewRecord | null; reactions: Reaction[]; @@ -362,6 +364,8 @@ function summarize( }); } + const comments = array(object(pull.timelineItems).nodes).filter((node) => node.__typename === "IssueComment"); + const threads = threadNodes .filter((row) => row.isResolved !== true) .map((row): Thread => { @@ -420,6 +424,7 @@ function summarize( }, checks, reviews: [...latest.values()], + checkpoint: checkpoint(head, submitted, comments), reviewRecord: null, reactions, threads, @@ -428,6 +433,38 @@ 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 leaves findings there, as anything but an approval, in its third, sixth, ... round. A round is one commit it + * reviewed since the PR's latest `Design change:` comment, which restarts the count as `quest review --new` does. + */ +function checkpoint(head: string, submitted: ObjectValue[], comments: ObjectValue[]): Status["checkpoint"] { + const since = Math.max( + 0, + ...comments + .filter( + (comment) => typeof comment.body === "string" && comment.body.trimStart().startsWith("Design change:"), + ) + .map((comment) => Date.parse(string(comment.createdAt))), + ); + 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) => row.state === "APPROVED" || row.state === "CHANGES_REQUESTED")) continue; + const counted = rows.filter((row) => row.commit != null && Date.parse(string(row.submittedAt)) > since); + const last = counted.at(-1); + if (last === undefined || string(object(last.commit).oid) !== head || last.state === "APPROVED") continue; + const round = new Set(counted.map((row) => string(object(row.commit).oid))).size; + if (round % 3 === 0) return { reviewer, round, url: string(last.url) }; + } + return null; +} + /** * 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 +526,9 @@ export function statusLines(status: Status): string[] { ...status.reviews.map( (r) => `review ${r.current ? "current" : "stale"} ${r.state} ${r.reviewer} ${r.submittedAt} ${r.url}`, ), + ...(status.checkpoint + ? [`checkpoint ${status.checkpoint.reviewer} round ${status.checkpoint.round} ${status.checkpoint.url}`] + : []), ...status.reactions.map((r) => `reacted:${r.content} ${r.actor} ${r.createdAt} ${r.url}`), ...status.threads.map( (t) => diff --git a/test/status.test.ts b/test/status.test.ts index ba7a8f83..45846364 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -691,3 +691,73 @@ 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 comment = (body: string, minute: number) => ({ + __typename: "IssueComment", + body, + createdAt: `2026-10-07T12:${String(minute).padStart(2, "0")}:30Z`, + authorAssociation: "OWNER", + author: { __typename: "User", login: "kixelated" }, + }); + const line = async (reviews: Node[], comments: Node[] = []) => + statusLines( + await pullRequestStatus( + pr, + setup([ + pull({ + checks: null, + reviews: page(reviews), + threads: page([]), + timelineItems: { nodes: comments }, + }), + ]).deps, + ), + ).find((entry) => entry.startsWith("checkpoint ")) ?? null; + + 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)).toBe(`checkpoint pullfrog round 3 ${URL_}#pullrequestreview-3`); + // An approval of the head, a second review of the same commit, or a head it has not reviewed is no checkpoint. + expect(await line([...rounds.slice(0, 2), bot("pullfrog", "APPROVED", A, 3)])).toBeNull(); + expect(await line([...rounds.slice(0, 2), bot("pullfrog", "CHANGES_REQUESTED", C, 3)])).toBeNull(); + expect(await line([...rounds, bot("pullfrog", "COMMENTED", A, 4)])).toBe( + `checkpoint pullfrog round 3 ${URL_}#pullrequestreview-4`, + ); + }); + + 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)).toBeNull(); + const person = [B, C, A].map((commit, index) => ({ + ...bot("alice", "CHANGES_REQUESTED", commit, index + 1), + author: { __typename: "User", login: "alice" }, + })); + expect(await line(person)).toBeNull(); + }); + + test("restarts the count after a Design change comment", 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, [comment("Design change: split the parser.", 1)])).toBeNull(); + expect(await line(rounds, [comment("Not a design change.", 1)])).toBe( + `checkpoint pullfrog round 3 ${URL_}#pullrequestreview-3`, + ); + }); +}); From 321124e35eec688ca2e82a4fdba0dd420da9b28c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 13:30:30 -0800 Subject: [PATCH 05/10] fix(status): count every online review round, and stop /quest-iterate at a checkpoint Drop the Design change reset: anyone could post one, and it needed the whole timeline, while a rethink at a checkpoint leaves the next one three rounds on anyway. /quest-iterate now takes the checkpoint before fixing the findings. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- assets/skills/iterate.md | 3 ++- assets/skills/review.md | 2 +- src/cli/main.ts | 6 +++--- src/cli/status.ts | 18 ++++-------------- test/status.test.ts | 22 +--------------------- 6 files changed, 12 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 66e35733..969973f3 100644 --- a/README.md +++ b/README.md @@ -497,7 +497,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` | A 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, is not an approval, and ends its third, sixth, ... round: [`/quest-review`'s design checkpoint](#review-locally) for an online reviewer. A round is one commit it reviewed since the PR's latest `Design change:` comment. Only printed when due. | +| `checkpoint` | A 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, is not an approval, and ends its third, sixth, ... round: [`/quest-review`'s design checkpoint](#review-locally) for an online reviewer. A round is one commit it reviewed. 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`. | diff --git a/assets/skills/iterate.md b/assets/skills/iterate.md index 79438c0c..08f6d09d 100644 --- a/assets/skills/iterate.md +++ b/assets/skills/iterate.md @@ -15,7 +15,8 @@ Your `Implementer:` line ends with the size your brief gives the work, as `Imple 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? -`/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; a `checkpoint` line in `quest status` is one an online reviewer's rounds put on the head. +`/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, take that checkpoint before fixing them. 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/review.md b/assets/skills/review.md index 4f1dfa93..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, or a `checkpoint` line in `quest status `, which an online reviewer's does, 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/src/cli/main.ts b/src/cli/main.ts index 3e840765..defe047d 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -546,9 +546,9 @@ export function main(argv: string[], out: Output): number | Promise { "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 checkpoint line naming a bot " + - "reviewer that approves PRs, its round, and its review, when that review is of the head, approves " + - "nothing, and ends its third, sixth, ... round (one reviewed commit since the latest `Design change:` " + - "comment), as /quest-review's design checkpoint; a reacted: line per " + + "reviewer that approves or requests changes, its round, and its review, when that review is of the " + + "head, is not an approval, 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 2819eba2..416b35bd 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -364,8 +364,6 @@ function summarize( }); } - const comments = array(object(pull.timelineItems).nodes).filter((node) => node.__typename === "IssueComment"); - const threads = threadNodes .filter((row) => row.isResolved !== true) .map((row): Thread => { @@ -424,7 +422,7 @@ function summarize( }, checks, reviews: [...latest.values()], - checkpoint: checkpoint(head, submitted, comments), + checkpoint: checkpoint(head, submitted), reviewRecord: null, reactions, threads, @@ -437,17 +435,9 @@ 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 leaves findings there, as anything but an approval, in its third, sixth, ... round. A round is one commit it - * reviewed since the PR's latest `Design change:` comment, which restarts the count as `quest review --new` does. + * reviewed; a rethink at a checkpoint leaves the next one three rounds on, so nothing restarts the count. */ -function checkpoint(head: string, submitted: ObjectValue[], comments: ObjectValue[]): Status["checkpoint"] { - const since = Math.max( - 0, - ...comments - .filter( - (comment) => typeof comment.body === "string" && comment.body.trimStart().startsWith("Design change:"), - ) - .map((comment) => Date.parse(string(comment.createdAt))), - ); +function checkpoint(head: string, submitted: ObjectValue[]): Status["checkpoint"] { const rounds = new Map(); for (const row of submitted) { if (row.author == null || object(row.author).__typename !== "Bot") continue; @@ -456,7 +446,7 @@ function checkpoint(head: string, submitted: ObjectValue[], comments: ObjectValu } for (const [reviewer, rows] of rounds) { if (!rows.some((row) => row.state === "APPROVED" || row.state === "CHANGES_REQUESTED")) continue; - const counted = rows.filter((row) => row.commit != null && Date.parse(string(row.submittedAt)) > since); + const counted = rows.filter((row) => row.commit != null); const last = counted.at(-1); if (last === undefined || string(object(last.commit).oid) !== head || last.state === "APPROVED") continue; const round = new Set(counted.map((row) => string(object(row.commit).oid))).size; diff --git a/test/status.test.ts b/test/status.test.ts index 45846364..7d917072 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -702,14 +702,7 @@ describe("checkpoint", () => { commit: { oid: commit }, reactions: { nodes: [] }, }); - const comment = (body: string, minute: number) => ({ - __typename: "IssueComment", - body, - createdAt: `2026-10-07T12:${String(minute).padStart(2, "0")}:30Z`, - authorAssociation: "OWNER", - author: { __typename: "User", login: "kixelated" }, - }); - const line = async (reviews: Node[], comments: Node[] = []) => + const line = async (reviews: Node[]) => statusLines( await pullRequestStatus( pr, @@ -718,7 +711,6 @@ describe("checkpoint", () => { checks: null, reviews: page(reviews), threads: page([]), - timelineItems: { nodes: comments }, }), ]).deps, ), @@ -748,16 +740,4 @@ describe("checkpoint", () => { })); expect(await line(person)).toBeNull(); }); - - test("restarts the count after a Design change comment", 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, [comment("Design change: split the parser.", 1)])).toBeNull(); - expect(await line(rounds, [comment("Not a design change.", 1)])).toBe( - `checkpoint pullfrog round 3 ${URL_}#pullrequestreview-3`, - ); - }); }); From 910f095b98c6a4e8698ff9ec442546d0e130d3e7 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 13:57:18 -0800 Subject: [PATCH 06/10] fix(land): show a waiting pick what it waits for, and name every due checkpoint The own queue now shows a waiting pick a Waiting check naming its pending checks and any required approval, so an approval wait is never silent. quest status lists every bot reviewer due a checkpoint and skips dismissed reviews, and /quest-iterate points at /quest-review's checkpoint. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++-- assets/skills/iterate.md | 2 +- src/cli/land-queue.ts | 15 +++++++++++++-- src/cli/land-serve.ts | 2 +- src/cli/land.ts | 3 +++ src/cli/main.ts | 4 ++-- src/cli/status.ts | 25 +++++++++++++------------ test/land.test.ts | 17 +++++++++++++++-- test/status.test.ts | 25 ++++++++++++++++--------- 9 files changed, 66 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 969973f3..2aa41975 100644 --- a/README.md +++ b/README.md @@ -497,7 +497,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` | A 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, is not an approval, and ends its third, sixth, ... round: [`/quest-review`'s design checkpoint](#review-locally) for an online reviewer. A round is one commit it reviewed. Only printed when due. | +| `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, is neither an approval nor dismissed, and ends its third, sixth, ... round: [`/quest-review`'s design checkpoint](#review-locally) for an online reviewer. A round is one commit it reviewed. 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`. | @@ -512,7 +512,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`, `checkpoint` (`reviewer`, `round`, `url`, or null), `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. diff --git a/assets/skills/iterate.md b/assets/skills/iterate.md index 08f6d09d..0227f1fc 100644 --- a/assets/skills/iterate.md +++ b/assets/skills/iterate.md @@ -16,7 +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, take that checkpoint before fixing them. +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/src/cli/land-queue.ts b/src/cli/land-queue.ts index 709168ef..cfc1621d 100644 --- a/src/cli/land-queue.ts +++ b/src/cli/land-queue.ts @@ -715,8 +715,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 93c37799..7b716257 100644 --- a/src/cli/land-serve.ts +++ b/src/cli/land-serve.ts @@ -314,7 +314,7 @@ async function reportOverlaps(github: GitHub, plan: Plan, state: string, overlap /** * How long after its pick a PR may wait on pending required checks or an approval before its author is told - * which. Twice the default check deadline (`--timeout`), so a slow but running CI rarely trips it. + * 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; diff --git a/src/cli/land.ts b/src/cli/land.ts index d920794b..708f1750 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -162,6 +162,8 @@ export interface Waiting { url: string; /** 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. */ @@ -441,6 +443,7 @@ export async function resolveQueue( number: entry.number, url: entry.url, enabledAt: entry.enabledAt, + head: entry.head, pending: unmet.pending, approval, }); diff --git a/src/cli/main.ts b/src/cli/main.ts index defe047d..2708bb20 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -545,9 +545,9 @@ 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 checkpoint line naming a bot " + + "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, is not an approval, and ends its third, sixth, ... round (one commit it reviewed), as " + + "head, is neither an approval nor dismissed, 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" + diff --git a/src/cli/status.ts b/src/cli/status.ts index 416b35bd..cfa020a9 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -133,8 +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[]; - /** An online reviewer's design checkpoint on the head, as {@link checkpoint} finds it. */ - checkpoint: { reviewer: string; round: number; url: string } | null; + /** 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[]; @@ -422,7 +422,7 @@ function summarize( }, checks, reviews: [...latest.values()], - checkpoint: checkpoint(head, submitted), + checkpoints: checkpoints(head, submitted), reviewRecord: null, reactions, threads, @@ -434,10 +434,12 @@ 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 leaves findings there, as anything but an approval, in its third, sixth, ... round. A round is one commit it - * reviewed; a rethink at a checkpoint leaves the next one three rounds on, so nothing restarts the count. + * and leaves findings there, as neither an approval nor a dismissed review, in its third, sixth, ... round. A round + * is one commit it reviewed; a rethink at a checkpoint leaves the next one three rounds on, so nothing restarts the + * count. */ -function checkpoint(head: string, submitted: ObjectValue[]): Status["checkpoint"] { +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; @@ -448,11 +450,12 @@ function checkpoint(head: string, submitted: ObjectValue[]): Status["checkpoint" if (!rows.some((row) => row.state === "APPROVED" || row.state === "CHANGES_REQUESTED")) continue; const counted = rows.filter((row) => row.commit != null); const last = counted.at(-1); - if (last === undefined || string(object(last.commit).oid) !== head || last.state === "APPROVED") continue; + if (last === undefined || string(object(last.commit).oid) !== head) continue; + if (last.state === "APPROVED" || last.state === "DISMISSED") continue; const round = new Set(counted.map((row) => string(object(row.commit).oid))).size; - if (round % 3 === 0) return { reviewer, round, url: string(last.url) }; + if (round % 3 === 0) due.push({ reviewer, round, url: string(last.url) }); } - return null; + return due; } /** @@ -516,9 +519,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.checkpoint - ? [`checkpoint ${status.checkpoint.reviewer} round ${status.checkpoint.round} ${status.checkpoint.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 9aadac36..b5e518cf 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1021,9 +1021,9 @@ 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()); @@ -1039,6 +1039,7 @@ describe.skipIf(process.platform === "win32")("quest land", () => { number: 3, url: "https://github.com/upstream/project/pull/3", enabledAt: "2026-10-26T00:00:03Z", + head: unreviewed.head, pending: [], approval: true, }, @@ -1046,6 +1047,7 @@ describe.skipIf(process.platform === "win32")("quest land", () => { number: 5, url: "https://github.com/upstream/project/pull/5", enabledAt: "2026-10-26T00:00:05Z", + head: running.head, pending: ["test"], approval: false, }, @@ -4607,6 +4609,17 @@ 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 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 7d917072..c868e9ff 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -714,7 +714,7 @@ describe("checkpoint", () => { }), ]).deps, ), - ).find((entry) => entry.startsWith("checkpoint ")) ?? null; + ).filter((entry) => entry.startsWith("checkpoint ")); test("falls due when an approving reviewer's third round leaves findings on the head", async () => { const rounds = [ @@ -722,22 +722,29 @@ describe("checkpoint", () => { bot("pullfrog", "CHANGES_REQUESTED", C, 2), bot("pullfrog", "CHANGES_REQUESTED", A, 3), ]; - expect(await line(rounds)).toBe(`checkpoint pullfrog round 3 ${URL_}#pullrequestreview-3`); - // An approval of the head, a second review of the same commit, or a head it has not reviewed is no checkpoint. - expect(await line([...rounds.slice(0, 2), bot("pullfrog", "APPROVED", A, 3)])).toBeNull(); - expect(await line([...rounds.slice(0, 2), bot("pullfrog", "CHANGES_REQUESTED", C, 3)])).toBeNull(); - expect(await line([...rounds, bot("pullfrog", "COMMENTED", A, 4)])).toBe( + 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`, - ); + ]); + // 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)).toBeNull(); + 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)).toBeNull(); + expect(await line(person)).toEqual([]); }); }); From 353e6f8d847032198ce3a0cb039531d50b6c8f9e Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 14:13:55 -0800 Subject: [PATCH 07/10] fix(land): refresh a queue check whose summary changed Co-Authored-By: Claude Opus 5.5 --- src/cli/land-queue.ts | 3 +++ test/land.test.ts | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/cli/land-queue.ts b/src/cli/land-queue.ts index cfc1621d..6d571cfa 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); diff --git a/test/land.test.ts b/test/land.test.ts index b5e518cf..d2400c45 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -4620,6 +4620,19 @@ describe.skipIf(process.platform === "win32")("the watcher's own queue", () => { 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" }); From 9c624e73354f61ed517ed4a2260cc1fcf61bcd6a Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 14:17:17 -0800 Subject: [PATCH 08/10] fix(status): keep a reviewer whose approval a push dismissed, and skip a head it approved Pullfrog posts findings as comments and approves only when clean, and a push dismisses its approval, so a dismissed review now shows a reviewer gates. A comment after approving the head is no checkpoint. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/cli/main.ts | 2 +- src/cli/status.ts | 13 ++++++++----- test/status.test.ts | 10 ++++++++++ 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 2aa41975..53ca3a0c 100644 --- a/README.md +++ b/README.md @@ -497,7 +497,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, is neither an approval nor dismissed, and ends its third, sixth, ... round: [`/quest-review`'s design checkpoint](#review-locally) for an online reviewer. A round is one commit it reviewed. Only printed when due. | +| `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 it has not approved the head, 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`. | diff --git a/src/cli/main.ts b/src/cli/main.ts index 2708bb20..d7fd1bf9 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -547,7 +547,7 @@ export function main(argv: string[], out: Output): number | Promise { "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 checkpoint line per bot " + "reviewer that approves or requests changes, its round, and its review, when that review is of the " + - "head, is neither an approval nor dismissed, and ends its third, sixth, ... round (one commit it reviewed), as " + + "head, while it has not approved the head, 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" + diff --git a/src/cli/status.ts b/src/cli/status.ts index cfa020a9..99cd15fb 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -434,10 +434,12 @@ 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 leaves findings there, as neither an approval nor a dismissed review, in its third, sixth, ... round. A round - * is one commit it reviewed; a rethink at a checkpoint leaves the next one three rounds on, so nothing restarts the - * count. + * and that has not approved the head, 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(); @@ -447,11 +449,12 @@ function checkpoints(head: string, submitted: ObjectValue[]): Status["checkpoint rounds.set(reviewer, [...(rounds.get(reviewer) ?? []), row]); } for (const [reviewer, rows] of rounds) { - if (!rows.some((row) => row.state === "APPROVED" || row.state === "CHANGES_REQUESTED")) continue; + 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; - if (last.state === "APPROVED" || last.state === "DISMISSED") continue; + const onHead = counted.filter((row) => string(object(row.commit).oid) === head); + if (onHead.some((row) => row.state === "APPROVED" || row.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) }); } diff --git a/test/status.test.ts b/test/status.test.ts index c868e9ff..d9ae012b 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -730,6 +730,16 @@ describe("checkpoint", () => { 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([]); // 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([ From 900be9f0212995dee383e7f9cdf1b425de9f0582 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 14:47:33 -0800 Subject: [PATCH 09/10] fix(status): let the latest verdict on the head decide a checkpoint Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/cli/main.ts | 2 +- src/cli/status.ts | 9 ++++++--- test/status.test.ts | 8 ++++++++ 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 53ca3a0c..f25203a0 100644 --- a/README.md +++ b/README.md @@ -497,7 +497,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 it has not approved the head, 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. | +| `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 no approval, 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`. | diff --git a/src/cli/main.ts b/src/cli/main.ts index d7fd1bf9..61fe4dbd 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -547,7 +547,7 @@ export function main(argv: string[], out: Output): number | Promise { "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 checkpoint line per bot " + "reviewer that approves or requests changes, its round, and its review, when that review is of the " + - "head, while it has not approved the head, and ends its third, sixth, ... round (one commit it reviewed), as " + + "head, while its latest verdict on the head is no approval, 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" + diff --git a/src/cli/status.ts b/src/cli/status.ts index 99cd15fb..571f9b76 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -434,7 +434,8 @@ 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 that has not approved the head, in its third, sixth, ... round. A dismissed review was an approval or a change + * 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. */ @@ -453,8 +454,10 @@ function checkpoints(head: string, submitted: ObjectValue[]): Status["checkpoint const counted = rows.filter((row) => row.commit != null); const last = counted.at(-1); if (last === undefined || string(object(last.commit).oid) !== head) continue; - const onHead = counted.filter((row) => string(object(row.commit).oid) === head); - if (onHead.some((row) => row.state === "APPROVED" || row.state === "DISMISSED")) 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) }); } diff --git a/test/status.test.ts b/test/status.test.ts index d9ae012b..51da6e17 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -740,6 +740,14 @@ describe("checkpoint", () => { 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([ From a92e3bc1f6be9b39e3ebf28946e27a1447c8baa6 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 12 Nov 2026 14:50:57 -0800 Subject: [PATCH 10/10] docs(status): say a dismissed verdict also suppresses a checkpoint Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/cli/main.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index f25203a0..0e9cb65e 100644 --- a/README.md +++ b/README.md @@ -497,7 +497,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 no approval, 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. | +| `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`. | diff --git a/src/cli/main.ts b/src/cli/main.ts index 61fe4dbd..320a0df0 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -547,7 +547,7 @@ export function main(argv: string[], out: Output): number | Promise { "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 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 no approval, and ends its third, sixth, ... round (one commit it reviewed), as " + + "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" +