diff --git a/README.md b/README.md index 292786ce..9b0075c8 100644 --- a/README.md +++ b/README.md @@ -322,22 +322,26 @@ failed merge. A label change prints as `labeled:` or `unlabeled:` (spaces as `_`), so a queue that dequeues by removing its label wakes the waiter; the authenticated account's own label changes are ignored. Actors are printed without deciding -whose review counts. Edits to existing comments or reviews are outside this -command's scope. +whose review counts. Edited issue comments print as `edited`, at their update +time; edits to the authenticated account's own comments are ignored. Comment +edits have their own cursor position and are read across every page, so editing +an older bot status comment wakes a resumed wait. Review edits remain outside +this command's scope. CI results are read for the current head. The first required check to fail prints `checks:failed` right away. `checks:passed` prints once nothing on the head is still going (no pending check and no unfinished GitHub Actions run, which may still create jobs), every check and workflow the base branch names is present (except `quest land`'s, which no head carries), and every required -check passed; it names the last check to finish, -when the head went idle. Each prints once per head, with the check's name +check passed; it names the last required check that passed and is dated when +the head went idle. Each prints once per head, with the check's name (spaces as `_`) in the actor field and its details URL. Results count as `quest status` counts them, so neutral and skipped results pass. Each job of a workflow a ruleset requires is required. Requiredness comes from the base branch's protection and rulesets, readable -with read access; optional checks never wake the wait, unless the base branch -requires none, in which case every check is required. A result counts when it +with read access. A failed optional check prints separately as `check:failed`, +with its own name and details URL; it never names the aggregate required pass. +If the base branch requires no checks, every check is required. A result counts when it was reached at or after the starting point, so a wait started after CI finished still reports it; a push starts over for the new head, and the cursor records what was reported, so resuming never replays it. @@ -386,6 +390,7 @@ auto-merge - merge-queue - landing-stopped - checks success passed=14 failing=0 pending=0 +review-rounds 2 - review stale commented kixelated 2026-10-16T06:14:04Z https://github.com/kixelated/quest/pull/128#pullrequestreview-5543745302 review current commented chatgpt-codex-connector 2026-10-16T17:57:16Z https://github.com/kixelated/quest/pull/128#pullrequestreview-5551703054 thread PRRT_kwDOUhuoys6tUUV2 current chatgpt-codex-connector https://github.com/kixelated/quest/pull/128#discussion_r4297708263 src/cli/gate.ts:115 @@ -404,6 +409,7 @@ comes last, and a missing value prints as `-`: | `landing-stopped` | The reason the latest merge-queue entry, auto-merge, or [`quest land`](#land-a-batch-with-quest-land) queue entry ended (such as `merge_conflict`, `failed_checks`, `manual`, `manually_disabled`, or `merged`), who ended it, and when. | | `checks` | The head's rollup state and `passed=`, `failing=`, `pending=` counts. | | `check` | `failing` or `pending`, `required` or `optional`, the result, details URL, and name. | +| `review-rounds` | Count of distinct commits with submitted reviews since the latest comment starting `Design change:`, followed by that marker's time, author, and URL (`-` when absent). Pending reviews and reviews without a commit do not count. This reports history, not whose review satisfies repository policy. | | `review` | `current` or `stale` against the head, state, reviewer, time, and URL of each reviewer's latest review. | | `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. | @@ -415,7 +421,8 @@ The node ID is what GraphQL's `addPullRequestReviewThreadReply` and 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`, and +`landingStopped`, `checks` (`state`, `passed`, `failing`, `pending`), `reviewRounds` +(`count`, and `since` with the marker's `at`, `actor`, `url`, or `null`), `reviews`, `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/AGENTS.md b/assets/AGENTS.md index c68d0ba1..171936d7 100644 --- a/assets/AGENTS.md +++ b/assets/AGENTS.md @@ -177,7 +177,7 @@ Where it says nothing, a review by the PR's author, or an AI-attributed comment Checkpoint the design across review rounds. A review round is one pushed head that drew reviews, counted from the PR's full history since its last comment that starts with `Design change:`. -Read the full history with `gh pr view --json reviews,comments`. +`quest status ` reports the round count and latest `Design change:` marker under `review-rounds`. Every fourth round, stop before fixing more findings and reconsider the approach. Report that judgment and what remains, recommending to continue, rethink the approach, merge as is, or drop the PR, and let the user decide. Record each design change, including a rethink the user picks, in a PR comment that starts with `Design change:`; the next agent counts rounds from it. diff --git a/quest/m0/helpers/README.md b/quest/m0/helpers/README.md index d61e247f..da741f8e 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -66,7 +66,6 @@ commands from Sep-Oct. Session counts in each child come from that window. ## Required -- [Wait and status accuracy](/quest/m0/helpers/wait-accuracy.md) - wake on edited bot comments, honest `checks:passed`, review rounds in `quest status` - [Headless agents](/quest/m0/helpers/headless-agents.md) - `/quest-spawn api` launches, resumes, and stops billed agents through one command auto mode allows - [Sibling PRs through a server-side merge](/quest/m0/helpers/queue-siblings.md) - children of one epic land back to back through a merge queue without conflicting on its link list - [kixelated/quest lands through quest land](/quest/m0/helpers/land-adopt.md) - this repository's `main` changes only through its own gate diff --git a/quest/m0/helpers/wait-accuracy.md b/quest/m0/helpers/wait-accuracy.md deleted file mode 100644 index 7dfdff0e..00000000 --- a/quest/m0/helpers/wait-accuracy.md +++ /dev/null @@ -1,30 +0,0 @@ -# [S] Report reviews and checks accurately in wait and status - -## Goal - -`quest wait` wakes when a bot edits its review-status comment and never -reports a failed check under `checks:passed`, and `quest status` prints how -many review rounds a PR has had since its last `Design change:` comment. - -## Plan - -Decided 2026-10-24 (user) from the merge wave that landed -[kixelated/quest#205](https://github.com/kixelated/quest/pull/205) through -[#219](https://github.com/kixelated/quest/pull/219): - -- **Edited bot comments.** Codex reports a finished or stuck review by - editing its summary comment. `quest wait` did not wake on those edits, so - agents on [#213](https://github.com/kixelated/quest/pull/213) and - [#219](https://github.com/kixelated/quest/pull/219) read the comment by - hand after each wait. Report an edit as an event. -- **Honest `checks:passed`.** On - [#207](https://github.com/kixelated/quest/pull/207), `quest wait` printed - `checks:passed native_Windows_(windows-11-arm)` six seconds after that - optional job failed: the event covers the required checks but is labelled - with the last check to finish. Name what passed, and report a failed - optional check separately. -- **Review rounds.** `quest status` prints the number of pushed heads that - drew reviews since the last `Design change:` comment, which the merge - skill's design checkpoint counts today with `gh pr view --json`. -- Dropped (user): removing the `quest:merge` label after a PR merges; holds - only count on open PRs, so the leftover label is cosmetic. diff --git a/src/cli/main.ts b/src/cli/main.ts index 60f5aebb..d23a80c0 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -339,20 +339,20 @@ export function main(argv: string[], out: Output): number | Promise { .command("wait") .summary("Wait for pull request activity on GitHub") .description( - "Wait for a review, comment, review-thread reply, reaction to the PR or a review, push, merge, close, " + - "merge-queue removal, disabled auto-merge, label change, or required CI result. Polls every 5 minutes, and once more 30 seconds before the timeout; rate limits and longer GitHub poll intervals delay requests. No timeout by default.\n\n" + + "Wait for a review, comment or comment edit, review-thread reply, reaction to the PR or a review, push, merge, close, " + + "merge-queue removal, disabled auto-merge, label change, or CI result. Polls every 5 minutes, and once more 30 seconds before the timeout; rate limits and longer GitHub poll intervals delay requests. No timeout by default.\n\n" + "Prints one plain line per event: timestamp (or commit SHA), type, actor, URL; the last line adds a resume cursor. " + "Reactions print as reacted:, such as reacted:thumbs_up, and label changes as labeled: or unlabeled:. " + "The head's first failing required check prints as checks:failed, and every required check passing as checks:passed, " + - "each once per head, with the check's name as the actor and its details URL. " + + "each once per head, with the required check's name as the actor and its details URL. " + "Results count as quest status counts them, so neutral and skipped results pass. " + - "checks:passed waits until no check is pending, no GitHub Actions run is unfinished, and every check and workflow the base branch names is present, then names the last check to finish. " + - "Optional checks never wake the wait, unless the base branch requires none, which requires them all. " + + "checks:passed waits until no check is pending, no GitHub Actions run is unfinished, and every check and workflow the base branch names is present, then names the last required check that passed. " + + "Optional failures print separately as check:failed; a base branch requiring no checks requires them all. " + "Pass the final field of the last line to --since to resume without replaying events. " + "On timeout, the only line is a resume point for --since (a cursor, or the given --since or head SHA if setup did not finish), so a retry with it still reports activity from the first wait.\n\n" + "Uses GH_TOKEN, GITHUB_TOKEN, or gh auth token. Label changes, replies, and reactions by that account are ignored. " + "Exits 0 for activity, 3 on timeout, 2 for invalid arguments, and 1 for other failures. " + - "Edits are not activity.", + "Issue-comment edits print as edited, including updates to older bot status comments. The account's own edits are ignored.", ) .argument("", "GitHub PR URL, or a PR number in origin") .option("--since ", "ISO timestamp, pushed-tip SHA, or resume cursor (default: current head push time)") @@ -383,7 +383,8 @@ export function main(argv: string[], out: Output): number | Promise { "landing-stopped, and checks; landing-stopped is the latest exit from the merge queue, auto-merge, or " + "quest land's queue, with the reason (such as merge_conflict, failed_checks, manual, or manually_disabled), " + "actor, and time. Then " + - "a check line per failing or pending check, marked required or optional; a review line per " + + "a check line per failing or pending check, marked required or optional; review-rounds counts distinct " + + "submitted review commits since the latest Design change: comment, with its time, author, and URL (or -); a review line per " + "reviewer's latest review, marked current or stale against the head; 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 d7185480..74a244db 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -17,8 +17,8 @@ import { readHoldStatus, type HoldFact, type HoldReport } from "./hold"; * connection already read in full is skipped on later pages. */ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, - $checks: String, $reviews: String, $threads: String, - $moreChecks: Boolean!, $moreReviews: Boolean!, $moreThreads: Boolean!) { + $checks: String, $reviews: String, $threads: String, $comments: String, + $moreChecks: Boolean!, $moreReviews: Boolean!, $moreThreads: Boolean!, $moreComments: Boolean!) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { url state isDraft headRefOid mergeable mergeStateStatus reviewDecision @@ -45,6 +45,10 @@ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, pageInfo { hasNextPage endCursor } nodes { author { login } state submittedAt url commit { oid } reactions(last: 100) { nodes { ...reaction } } } } + comments(first: 100, after: $comments) @include(if: $moreComments) { + pageInfo { hasNextPage endCursor } + nodes { body createdAt url author { login } } + } reviewThreads(first: 100, after: $threads) @include(if: $moreThreads) { pageInfo { hasNextPage endCursor } nodes { @@ -111,6 +115,8 @@ export interface Status { /** The latest exit from the merge queue, auto-merge, or `quest land`'s queue, and the reason for it. */ landingStopped: { reason: string; actor: string; at: string } | null; checks: { state: string | null; passed: number; failing: Check[]; pending: Check[] }; + /** Distinct commits with submitted reviews after the latest Design change marker. */ + reviewRounds: { count: number; since: { at: string; actor: string; url: string } | null }; /** Each reviewer's latest submitted review, oldest first. */ reviews: Review[]; reactions: Reaction[]; @@ -124,14 +130,16 @@ export interface Status { /** Read the paged PR state, then the state commands’ ownership snapshot. */ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencies): Promise { const github = new GitHub(deps); - const cursors: Record<"checks" | "reviews" | "threads", string | null | undefined> = { + const cursors: Record<"checks" | "reviews" | "threads" | "comments", string | null | undefined> = { checks: null, reviews: null, threads: null, + comments: null, }; const contexts: ObjectValue[] = []; const reviewNodes: ObjectValue[] = []; const threadNodes: ObjectValue[] = []; + const commentNodes: ObjectValue[] = []; let first: ObjectValue | undefined; let rollup: ObjectValue | null = null; // A cursor is null to read the first page and undefined once its list is complete. @@ -149,9 +157,11 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie checks: cursors.checks ?? null, reviews: cursors.reviews ?? null, threads: cursors.threads ?? null, + comments: cursors.comments ?? null, moreChecks: cursors.checks !== undefined, moreReviews: cursors.reviews !== undefined, moreThreads: cursors.threads !== undefined, + moreComments: cursors.comments !== undefined, }); const repository = data.repository; if (repository === null) throw new Error(`cannot read ${pr.owner}/${pr.repo}; check the repository and token`); @@ -171,12 +181,13 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie } if (cursors.reviews !== undefined) page(current.reviews, "reviews", reviewNodes); if (cursors.threads !== undefined) page(current.reviewThreads, "threads", threadNodes); + if (cursors.comments !== undefined) page(current.comments, "comments", commentNodes); } const ownership = await readHoldStatus(pr, deps); if (ownership.head !== first!.headRefOid) throw new Error("the PR head changed while reading its holds; rerun quest status"); return { - ...summarize(first!, rollup, contexts, reviewNodes, threadNodes), + ...summarize(first!, rollup, contexts, reviewNodes, threadNodes, commentNodes), holds: ownership.holds, starterHold: ownership.starter, }; @@ -188,6 +199,7 @@ function summarize( contexts: ObjectValue[], reviewNodes: ObjectValue[], threadNodes: ObjectValue[], + commentNodes: ObjectValue[], ): Status { const head = string(pull.headRefOid); const url = string(pull.url); @@ -216,6 +228,20 @@ function summarize( reacted(pull, url); // A pending review is the viewer's unsubmitted draft, not a review. const submitted = reviewNodes.filter((row) => row.state !== "PENDING"); + const changed = commentNodes + .filter((row) => typeof row.body === "string" && row.body.startsWith("Design change:")) + .sort((a, b) => Date.parse(string(a.createdAt)) - Date.parse(string(b.createdAt))) + .at(-1); + const since = changed + ? { at: string(changed.createdAt), actor: actor(changed.author), url: string(changed.url) } + : null; + const rounds = new Set( + submitted.flatMap((row) => + row.commit !== null && (!since || Date.parse(string(row.submittedAt)) >= Date.parse(since.at)) + ? [string(object(row.commit).oid)] + : [], + ), + ); const latest = new Map(); for (const row of submitted) { reacted(row, string(row.url)); @@ -281,6 +307,7 @@ function summarize( at: string(stopped.createdAt), }, checks, + reviewRounds: { count: rounds.size, since }, reviews: [...latest.values()], reactions, threads, @@ -349,6 +376,7 @@ export function statusLines(status: Status): string[] { `checks ${dash(status.checks.state)} passed=${status.checks.passed} failing=${status.checks.failing.length} pending=${status.checks.pending.length}`, ...status.checks.failing.map((c) => check("failing", c)), ...status.checks.pending.map((c) => check("pending", c)), + `review-rounds ${status.reviewRounds.count} ${status.reviewRounds.since ? `${status.reviewRounds.since.at} ${status.reviewRounds.since.actor} ${status.reviewRounds.since.url}` : "-"}`, ...status.reviews.map( (r) => `review ${r.current ? "current" : "stale"} ${r.state} ${r.reviewer} ${r.submittedAt} ${r.url}`, ), diff --git a/src/cli/wait.ts b/src/cli/wait.ts index 71522e92..b95cdc07 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -1,4 +1,6 @@ // GitHub-specific PR activity waiting stays outside the repository-independent core. +import { createHash } from "node:crypto"; + import { actor, array, @@ -89,8 +91,8 @@ interface Activity { } /** Each source is read separately, so each keeps its own position. */ -type Source = "timeline" | "replies" | "reactions"; -const SOURCES: Source[] = ["timeline", "replies", "reactions"]; +type Source = "timeline" | "replies" | "reactions" | "edits"; +const SOURCES: Source[] = ["timeline", "replies", "reactions", "edits"]; /** The newest event time read from a source, and the IDs of events at that time. */ interface Mark { time: number; @@ -250,6 +252,7 @@ async function watch( timeline: { time: since, ids: [] }, replies: { time: since, ids: [] }, reactions: { time: since, ids: [] }, + edits: { time: since, ids: [] }, }, commits: [], pushes: forced @@ -380,7 +383,20 @@ async function watch( const comments = pager( `${base}/pulls/${pr.number}/comments?per_page=${PAGE}&since=${new Date(cursor.marks.replies.time - 1000).toISOString()}`, ); - const found: Record = { timeline: historical, replies: [], reactions: [] }; + // Issue comments are ordered by creation: an old comment can be edited on an + // earlier page, so reread this update-filtered list from its first page. + const edits = async () => { + const rows: ObjectValue[] = []; + let path: string | undefined = + `${base}/issues/${pr.number}/comments?per_page=${PAGE}&since=${new Date(cursor.marks.edits.time - 1000).toISOString()}`; + while (path) { + const page = await github.list(path); + rows.push(...page.rows); + path = link(page.response, "next"); + } + return rows; + }; + const found: Record = { timeline: historical, replies: [], reactions: [], edits: [] }; for (;;) { github.pollInterval = 0; const labels: Activity[] = []; @@ -427,6 +443,14 @@ async function watch( if (event.actor !== viewer && fresh("replies", event)) found.replies.push(event); } found.reactions.push(...events.filter((event) => event.actor !== viewer && fresh("reactions", event))); + for (const row of await edits()) { + if (timestamp(string(row.updated_at)) <= timestamp(string(row.created_at))) continue; + const event = timedActivity(row, "edited", "updated_at", prUrl); + // GitHub timestamps have second precision: two different edits in that + // second remain distinct, without storing the comment text in a cursor. + event.id += `:${createHash("sha256").update(string(row.body)).digest("hex")}`; + if (event.actor !== viewer && fresh("edits", event)) found.edits.push(event); + } // Check results are state rather than a log, so each poll reads them afresh. const head = await checks(); const reached = head @@ -529,14 +553,19 @@ function checkActivities(head: string, all: Check[], needs: Needs, state: Cursor const inWorkflow = (entry: Check) => entry.workflow !== undefined && needs.workflows.includes(entry.workflow); const required = needs.names.size || needs.workflows.length ? all.filter((entry) => entry.required || inWorkflow(entry)) : all; - const reached = (outcome: "passed" | "failed", entry: Check | undefined): Activity[] => { - const id = `${head}:${outcome}`; - if (!entry?.result || entry.result.time < state.since || state.reported.includes(id)) return []; + const reached = ( + outcome: "passed" | "failed", + entry: Check | undefined, + optional = false, + at = entry?.result, + ): Activity[] => { + const id = optional ? `${head}:optional:${entry?.name}:${entry?.url}:${at?.point}` : `${head}:${outcome}`; + if (!entry?.result || !at || at.time < state.since || state.reported.includes(id)) return []; return [ { - point: entry.result.point, - time: entry.result.time, - type: `checks:${outcome}`, + point: at.point, + time: at.time, + type: `${optional ? "check" : "checks"}:${outcome}`, actor: entry.name.replace(/\s/g, "_"), url: entry.url ?? `${prUrl}/checks`, id, @@ -546,21 +575,29 @@ function checkActivities(head: string, all: Check[], needs: Needs, state: Cursor const settled = required.flatMap((entry) => (entry.result ? [{ ...entry, result: entry.result }] : [])); settled.sort((a, b) => a.result.time - b.result.time); const failed = settled.find((entry) => entry.result.outcome === "failed" && entry.result.time >= state.since); - // GitHub lists a check only once it starts, so a pass waits until nothing on the head is still - // going: no pending check, no workflow run that may yet create a job, and every required name - // and workflow present. It is dated by the last check to finish, when the head went idle. + // A pass waits for the head to go idle, including jobs a running workflow + // may still create. Its date is the last completion, but its name and link + // always identify a required check that passed. const idle = all.every((entry) => entry.result && !entry.running) && [...needs.names].every((name) => all.some((entry) => entry.name === name)) && needs.workflows.every((path) => all.some((entry) => entry.workflow === path)); const passed = idle && settled.length === required.length && settled.every((entry) => entry.result.outcome === "passed") - ? all.reduce( - (last, entry) => (!last || entry.result!.time >= last.result!.time ? entry : last), - undefined, - ) + ? settled.at(-1) : undefined; - return [...reached("failed", failed), ...reached("passed", passed)]; + const last = idle + ? all.reduce( + (latest, entry) => (!latest || entry.result!.time >= latest.result!.time ? entry : latest), + undefined, + ) + : undefined; + const optional = all.filter((entry) => !required.includes(entry) && entry.result?.outcome === "failed"); + return [ + ...reached("failed", failed), + ...optional.flatMap((entry) => reached("failed", entry, true)), + ...reached("passed", passed, false, last?.result), + ]; } function timedActivity(row: ObjectValue, type: string, timeField: string, fallback: string): Activity { @@ -609,6 +646,9 @@ function decodeCursor(value: string, pr: string): Cursor { const parsed = object(JSON.parse(Buffer.from(value.slice(3), "base64url").toString("utf8"))); const marks = object(parsed.marks); const checks = object(parsed.checks); + // Older resumable cursors predate edited-comment tracking. Start that + // source at the original baseline so an edit is never silently skipped. + marks.edits ??= { time: checks.since, ids: [] }; if ( parsed.pr !== pr || !SOURCES.every((source) => { diff --git a/test/status.test.ts b/test/status.test.ts index 35536942..cb79841e 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -22,10 +22,11 @@ function pull( checks?: ReturnType | null; reviews?: ReturnType; threads?: ReturnType; + comments?: ReturnType; reactions?: Node[]; } & Node = {}, ) { - const { head = A, checks, reviews, threads, reactions = [], ...rest } = fields; + const { head = A, checks, reviews, threads, comments = page([]), reactions = [], ...rest } = fields; return { data: { repository: { @@ -46,6 +47,7 @@ function pull( autoMergeRequest: null, mergeQueueEntry: null, timelineItems: { nodes: [] }, + comments, reactions: { nodes: reactions }, commits: { nodes: [ @@ -210,6 +212,7 @@ test("reports checks by requiredness, each reviewer's latest review against the "check failing optional error - ci/legacy", "check pending required in_progress https://github.com/example/project/actions/runs/1/job/test test", "check pending required expected - deploy", + "review-rounds 2 -", `review stale changes_requested bob 2026-10-07T12:02:00Z ${URL_}#pullrequestreview-2`, `review current commented alice 2026-10-07T12:03:00Z ${URL_}#pullrequestreview-3`, `review stale commented gone 2026-10-07T12:05:00Z ${URL_}#pullrequestreview-5`, @@ -360,6 +363,44 @@ test("an invalid PR argument is a usage error before authentication", () => { expect(errors.join("")).toContain("GitHub pull request URL"); }); +test("review rounds count distinct submitted commits since the latest design marker across every comment page", async () => { + const marker = (id: number, time: string, body = "Design change: simpler boundary") => ({ + body, + createdAt: time, + url: `${URL_}#issuecomment-${id}`, + author: { login: "maintainer" }, + }); + const fixture = setup([ + pull({ + checks: null, + threads: page([]), + reviews: page([ + review("alice", "APPROVED", A, 1), + review("bob", "COMMENTED", B, 4), + review("alice", "COMMENTED", B, 5), + review("pending", "PENDING", A, 6), + ]), + comments: page([marker(1, "2026-10-07T12:00:00Z")], "comments-next"), + }), + pull({ + comments: page([marker(2, "2026-10-07T12:03:00Z"), marker(3, "2026-10-07T12:06:00Z", "A normal comment")]), + }), + ]); + const status = await pullRequestStatus(pr, fixture.deps); + expect(status.reviewRounds).toEqual({ + count: 1, + since: { at: "2026-10-07T12:03:00Z", actor: "maintainer", url: `${URL_}#issuecomment-2` }, + }); + expect(statusLines(status)).toContain(`review-rounds 1 2026-10-07T12:03:00Z maintainer ${URL_}#issuecomment-2`); + expect(fixture.queries[1]).toMatchObject({ + comments: "comments-next", + moreComments: true, + moreReviews: false, + moreChecks: false, + moreThreads: false, + }); +}); + const holdEvent = (id: string, label: string, at = "2026-10-07T11:00:00Z") => ({ __typename: "LabeledEvent", id, diff --git a/test/wait.test.ts b/test/wait.test.ts index 51b806a8..8ae9598a 100644 --- a/test/wait.test.ts +++ b/test/wait.test.ts @@ -148,6 +148,7 @@ function setup( baseline = [A], side: { comments?: Response[]; + edits?: Response[]; graphql?: Response[]; checks?: Response[]; protected?: string[]; @@ -161,6 +162,7 @@ function setup( const calls: { path: string; headers: Headers }[] = []; const sideCalls: { path: string; headers: Headers; body?: unknown }[] = []; const checkCalls: { path: string; headers: Headers; body?: unknown }[] = []; + const editCalls: { path: string; headers: Headers }[] = []; const ruleCalls: { path: string; headers: Headers }[] = []; const sleeps: number[] = []; const warnings: string[] = []; @@ -175,6 +177,10 @@ function setup( fetch: async (url, init) => { const parsed = new URL(String(url)); const call = { path: parsed.pathname + parsed.search, headers: new Headers(init?.headers) }; + if (parsed.pathname.endsWith("/issues/1/comments")) { + editCalls.push(call); + return side.edits?.shift() ?? response([]); + } if (parsed.pathname.endsWith("/pulls/1/comments")) { sideCalls.push(call); return side.comments?.shift() ?? response([]); @@ -224,7 +230,7 @@ function setup( return next; }, }; - return { deps, calls, sideCalls, checkCalls, ruleCalls, sleeps, warnings, responses }; + return { deps, calls, sideCalls, checkCalls, editCalls, ruleCalls, sleeps, warnings, responses }; } function graphql(fixture: ReturnType) { @@ -864,48 +870,51 @@ test("a required failure wakes the wait, naming the check and linking its detail protected: ["build and test", "check"], }); const lines = await waitForActivity(pr, {}, fixture.deps); - expect(lines).toHaveLength(1); - expect(lines[0]).toMatch( + expect(lines).toHaveLength(2); + expect(lines[0]).toContain("check:failed lint"); + expect(lines[1]).toMatch( /^2026-10-07T12:08:00Z checks:failed build_and_test https:\/\/github.com\/example\/project\/actions\/runs\/1\/job\/14 q1\./, ); expect(fixture.checkCalls[0].body).toMatchObject({ variables: { owner: "example", repo: "project", number: 1 } }); }); -test("an optional failure never wakes the wait, and checks:passed waits for every check to finish", async () => { - const fixture = setup( - [pull(), response([pushed()]), response([committed(A)]), response([committed(A)]), response([committed(A)])], - [A], - { - checks: [ - checks([ - { name: "lint", result: "FAILURE", time: LATER }, - { name: "check", required: true, result: "SUCCESS", time: LATER }, - { name: "test", required: true }, - ]), - checks([ - { name: "lint", result: "FAILURE", time: LATER }, - { name: "check", required: true, result: "SUCCESS", time: LATER }, - { name: "test", required: true, result: "SKIPPED", time: LATEST }, - { name: "slow" }, - ]), - checks([ - { name: "lint", result: "FAILURE", time: LATER }, - { name: "check", required: true, result: "SUCCESS", time: LATER }, - { name: "test", required: true, result: "SKIPPED", time: LATEST }, - { name: "slow", result: "SUCCESS", time: LATER }, - ]), - ], - ruled: ["check", "test"], - }, - ); - const lines = await waitForActivity(pr, {}, fixture.deps); - expect(fixture.sleeps).toEqual([300_000, 300_000]); +test("an optional failure wakes separately, and resuming still waits for all checks before reporting a required pass", async () => { + const first = setup([pull(), response([pushed()]), response([committed(A)])], [A], { + checks: [ + checks([ + { name: "lint", result: "FAILURE", time: LATER }, + { name: "check", required: true, result: "SUCCESS", time: LATER }, + { name: "test", required: true }, + ]), + ], + ruled: ["check", "test"], + }); + const failed = await waitForActivity(pr, {}, first.deps); + expect(failed).toHaveLength(1); + expect(failed[0]).toMatch(/^2026-10-07T12:05:00Z check:failed lint /); + const next = setup([response([committed(A)]), response([committed(A)])], [A], { + checks: [ + checks([ + { name: "lint", result: "FAILURE", time: LATER }, + { name: "check", required: true, result: "SUCCESS", time: LATER }, + { name: "test", required: true, result: "SKIPPED", time: LATEST }, + { name: "slow" }, + ]), + checks([ + { name: "lint", result: "FAILURE", time: LATER }, + { name: "check", required: true, result: "SUCCESS", time: LATER }, + { name: "test", required: true, result: "SKIPPED", time: LATEST }, + { name: "slow", result: "SUCCESS", time: LATER }, + ]), + ], + ruled: ["check", "test"], + }); + const lines = await waitForActivity(pr, { since: cursor(failed) }, next.deps); + expect(next.sleeps).toEqual([300_000]); expect(lines).toHaveLength(1); - expect(lines[0]).toMatch( - /^2026-10-07T12:08:00Z checks:passed test https:\/\/github.com\/example\/project\/actions\/runs\/1\/job\/4 q1\./, - ); - // The base branch's requirements are read once per wait. - expect(fixture.ruleCalls.map((call) => call.path)).toEqual([ + expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:passed test /); + // Every wait reads the base's requirements just once. + expect(next.ruleCalls.map((call) => call.path)).toEqual([ `${base}/branches/main`, `${base}/rules/branches/main?per_page=100`, ]); @@ -945,13 +954,13 @@ test("optional checks never stand in for required checks that have not started", [A], { checks: [ - checks([{ name: "lint", result: "FAILURE", time: LATER }]), + checks([{ name: "lint", result: "FAILURE", time: "2026-10-07T11:59:00Z" }]), checks([ - { name: "lint", result: "FAILURE", time: LATER }, + { name: "lint", result: "FAILURE", time: "2026-10-07T11:59:00Z" }, { name: "check", required: true, result: "SUCCESS", time: LATER }, ]), checks([ - { name: "lint", result: "FAILURE", time: LATER }, + { name: "lint", result: "FAILURE", time: "2026-10-07T11:59:00Z" }, { name: "check", required: true, result: "SUCCESS", time: LATER }, { name: "gate", required: true, result: "SUCCESS", time: LATEST }, ]), @@ -1003,14 +1012,14 @@ test("a required workflow requires its jobs, keeps optional checks optional, and [A], { checks: [ - checks([{ name: "lint", result: "FAILURE", time: LATER }]), + checks([{ name: "lint", result: "FAILURE", time: "2026-10-07T11:59:00Z" }]), // The run's next job has not been created yet. checks([ - { name: "lint", result: "FAILURE", time: LATER }, + { name: "lint", result: "FAILURE", time: "2026-10-07T11:59:00Z" }, { name: "build", result: "SUCCESS", time: LATER, workflow: CI, running: true }, ]), checks([ - { name: "lint", result: "FAILURE", time: LATER }, + { name: "lint", result: "FAILURE", time: "2026-10-07T11:59:00Z" }, { name: "build", result: "SUCCESS", time: LATER, workflow: CI }, { name: "gate", result: "SUCCESS", time: LATEST, workflow: CI }, ]), @@ -1053,7 +1062,7 @@ test("checks:passed is dated when the head goes idle, after required checks fini const lines = await waitForActivity(pr, { since: LATER }, fixture.deps); expect(fixture.sleeps).toEqual([300_000]); expect(lines).toHaveLength(1); - expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:passed slow /); + expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:passed check /); }); test("with no required checks, an unfinished workflow run holds back checks:passed", async () => { @@ -1178,3 +1187,81 @@ test("checks are read from every page, and a head racing the read waits for the "next", ]); }); + +function edited(id: number, updated = LATER, body = "Review completed", login = "codex[bot]") { + return { + id, + body, + created_at: "2026-10-06T12:00:00Z", + updated_at: updated, + user: { login }, + html_url: `https://github.com/example/project/pull/1#issuecomment-${id}`, + }; +} + +test("edited bot comments wake once, including different edits in the same second", async () => { + const first = setup([pull(), response([pushed()]), response([committed(A)])], [A], { + edits: [response([edited(1)])], + }); + const lines = await waitForActivity(pr, {}, first.deps); + expect(lines).toHaveLength(1); + expect(lines[0]).toMatch(/^2026-10-07T12:05:00Z edited codex\[bot\] /); + expect(Buffer.from(cursor(lines).slice(3), "base64url").toString()).not.toContain("Review completed"); + const resumed = setup([response([committed(A)]), response([committed(A)])], [A], { + edits: [response([edited(1)]), response([edited(1, LATER, "Security review completed")])], + }); + const changed = await waitForActivity(pr, { since: cursor(lines) }, resumed.deps); + expect(resumed.sleeps).toEqual([300_000]); + expect(changed[0]).toContain(" edited codex[bot] "); +}); + +test("edits reread every creation-ordered page, so an old first-page comment can change", async () => { + const page2 = `${base}/issues/1/comments?per_page=100&page=2`; + const untouched = { ...edited(1, POINT), created_at: POINT }; + const fixture = setup([pull(), response([pushed()]), response([committed(A)]), response([committed(A)])], [A], { + edits: [ + response([untouched], { link: `; rel="next"` }), + response([]), + response([edited(1)]), + ], + }); + expect((await waitForActivity(pr, {}, fixture.deps))[0]).toContain(" edited codex[bot] "); + expect(fixture.editCalls.map((call) => call.path)).toEqual([ + `${base}/issues/1/comments?per_page=100&since=2026-10-07T11:59:59.000Z`, + page2, + `${base}/issues/1/comments?per_page=100&since=2026-10-07T11:59:59.000Z`, + ]); +}); + +test("a later reaction does not hide an older edit, and old cursors learn the edit source", async () => { + const first = setup([pull(), response([pushed()]), response([committed(A)])], [A], { + graphql: [reactions([reacted("r1", LATEST)])], + }); + const previous = JSON.parse( + Buffer.from(cursor(await waitForActivity(pr, {}, first.deps)).slice(3), "base64url").toString(), + ); + delete previous.marks.edits; + const old = `q1.${Buffer.from(JSON.stringify(previous)).toString("base64url")}`; + const next = setup([response([committed(A)])], [A], { + edits: [response([edited(1), edited(2, LATEST, "My reply", "waiter")])], + }); + const lines = await waitForActivity(pr, { since: old }, next.deps); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain(" edited codex[bot] "); +}); + +test("a later failing optional check cannot name the aggregate required pass", async () => { + const fixture = setup([pull(), response([pushed()]), response([committed(A)])], [A], { + protected: ["check"], + checks: [ + checks([ + { name: "check", required: true, result: "SUCCESS", time: LATER }, + { name: "optional", result: "FAILURE", time: LATEST }, + ]), + ], + }); + const lines = await waitForActivity(pr, {}, fixture.deps); + expect(lines).toHaveLength(2); + expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z check:failed optional /); + expect(lines[1]).toMatch(/^2026-10-07T12:08:00Z checks:passed check /); +});