From 8ac9ce2c14b53dc9f1c02c5426c734d0e844cd1f Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 10:21:45 -0700 Subject: [PATCH 1/5] chore(quest): claim quest/m0/helpers/wait-checks.md Co-Authored-By: Claude Opus 5.5 From 3b4f3dfa04e6fc2041f2d54cfd5cbc2c69a30a26 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 10:30:06 -0700 Subject: [PATCH 2/5] feat(wait): wake on required CI check results quest wait now reads the head's checks each poll and prints checks:failed at the first failing required check and checks:passed once every required check passes, once per head. Requiredness comes from GraphQL isRequired(pullRequestNumber:); a base branch requiring no checks requires them all. The merge and complete skills drop the separate check watcher. Completes quest/m0/helpers/wait-checks.md. Co-Authored-By: Claude Opus 5.5 --- README.md | 23 +++- assets/skills/complete.md | 2 +- assets/skills/merge.md | 9 +- quest/m0/helpers/README.md | 1 - quest/m0/helpers/wait-checks.md | 31 ----- src/cli/main.ts | 7 +- src/cli/wait.ts | 146 ++++++++++++++++++--- test/wait.test.ts | 218 +++++++++++++++++++++++++++++++- 8 files changed, 374 insertions(+), 63 deletions(-) delete mode 100644 quest/m0/helpers/wait-checks.md diff --git a/README.md b/README.md index 70ce515a..d2fb2f23 100644 --- a/README.md +++ b/README.md @@ -147,7 +147,7 @@ local readiness as `ready`; verify claims and outside conditions before starting `quest wait` blocks until a GitHub PR has a review, comment, review-thread reply, reaction to the PR or one of its reviews, push, merge, close, removal -from the merge queue, or disabled auto-merge: +from the merge queue, disabled auto-merge, or required CI result: ```sh quest wait 101 @@ -175,8 +175,21 @@ timeline, excluding the one that created the starting head. A merge through the queue prints `removed_from_merge_queue` about a second before `merged`, so a poll can catch the removal alone; check the PR's state before treating it as a failed merge. Actors are printed without deciding -whose review counts. Check runs and edits to existing comments or reviews are -outside this command's scope. +whose review counts. Edits to existing comments or reviews are outside this +command's scope. + +CI results are read for the current head. The first required check to fail +prints `checks:failed`, and every required check passing prints +`checks:passed`, each once per head, with the check's name (spaces as `_`) in +the actor field and its details URL. Neutral and skipped results pass. +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 +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. GitHub lists only checks that +have started, so a required check created late, such as a job that waits on +others, can start after `checks:passed` prints. Review-thread replies and reactions never reach the PR timeline, so they are read separately. A reply prints as `replied`; a thread's first comment is part @@ -192,8 +205,8 @@ visible after a newer reaction is still reported. Each poll makes one conditional timeline request and one conditional review-comment request every five minutes once it has read the existing pages, -following a new page when the current one fills, plus one GraphQL query for -reactions. It honors GitHub's rate-limit headers and fails loudly on invalid responses. +following a new page when the current one fills, plus one GraphQL query each +for reactions and checks. It honors GitHub's rate-limit headers and fails loudly on invalid responses. There is no deadline unless `--timeout ` is supplied. Activity exits 0, timeout exits 3, invalid arguments exit 2, and other failures exit 1; interrupt with Ctrl-C to stop waiting. A timeout prints a resume point as its only line: diff --git a/assets/skills/complete.md b/assets/skills/complete.md index e1b55c51..682c884b 100644 --- a/assets/skills/complete.md +++ b/assets/skills/complete.md @@ -24,7 +24,7 @@ Consider ordering, queuing any merges if they would result in conflicts. Run each `/quest-ready`, `/quest-merge`, or `/quest-iterate` in its own sub-agent with its own isolated worktree. Each sub-agent blocks on its own waits and reports back only when done or blocked. -For GitHub review or merge activity, it uses `quest wait` and resumes with the +For GitHub CI, review, or merge activity, it uses one `quest wait` and resumes with the returned cursor as described by `quest skill merge`, rather than polling GitHub. Limit the number of active agents to the CPU core count. Other sessions share this machine: hold new agents while the load average exceeds the core count. diff --git a/assets/skills/merge.md b/assets/skills/merge.md index a79cfc69..bc4f09d5 100644 --- a/assets/skills/merge.md +++ b/assets/skills/merge.md @@ -20,16 +20,17 @@ This includes deciding if we should `/quest-plan` for suggested follow-ups. A background agent that cannot prompt lists its decisions and suggested follow-ups in its report instead. -When waiting for a required review or for an enabled auto-merge to finish, run +When waiting for CI, a required review, or an enabled auto-merge to finish, run `quest wait --timeout 3300` as one blocking command instead of polling -GitHub in a loop. Inspect the events and decide whether they satisfy the wait; +GitHub in a loop; it also reports the head's required checks as `checks:failed` +or `checks:passed`, so no separate check watcher is needed. +Inspect the events and decide whether they satisfy the wait; the command does not decide whose review counts. If activity is irrelevant (including bot notices), call it again with `--since` set to the last field of its last line. Exit 3 means the deadline passed with no activity; re-run with `--since` set to the printed cursor, so nothing during the first wait is missed. Other failures must be reported rather than treated as activity. -Check CI with its own blocking check watcher, bounded under an hour the same -way; check runs are not PR timeline activity. Do not wait for optional reviews. +Do not wait for optional reviews. Run each wait so its exit wakes you, such as a background task that notifies on exit, and end your turn until then. Never poll its output or rerun it under a foreground time limit; each wake rereads your whole context. The hour bound diff --git a/quest/m0/helpers/README.md b/quest/m0/helpers/README.md index c80bce7c..963e609f 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -51,7 +51,6 @@ commands from Sep-Oct. Session counts in each child come from that window. - [PR status](/quest/m0/helpers/status.md) - one call for head, checks, merge state, reviews by head, and unresolved threads - [Move and delete quests](/quest/m0/helpers/move.md) - `quest mv` and `quest rm` keep every root-absolute link valid -- [CI checks in wait](/quest/m0/helpers/wait-checks.md) - `quest wait` wakes on required check results for the head - [quest start](/quest/m0/helpers/start.md) - claim a quest with a draft PR, the `## Decisions needed` hand-back, `quest unblocked --unclaimed`, and CI on ready PRs only - [PR state commands](/quest/m0/helpers/pr-states.md) - `quest iterate`, `ready`, `merge`, and `release` label the one owner of a PR - [Unattended triage](/quest/m0/helpers/afk.md) - an `afk` argument that only asks about merges and deletes diff --git a/quest/m0/helpers/wait-checks.md b/quest/m0/helpers/wait-checks.md deleted file mode 100644 index 2271ae05..00000000 --- a/quest/m0/helpers/wait-checks.md +++ /dev/null @@ -1,31 +0,0 @@ -# [M] Wake quest wait on required CI checks - -## Goal - -`quest wait` also reports the head's required CI results, so one blocking -wait covers reviews, merges, and CI. The `merge` skill drops its "own -blocking check watcher", which led 119 of 396 sessions to poll -`gh pr checks` or write their own `wait-ci.sh`. - -## Plan - -- Decided 2026-10-19 (user): add events to every wait rather than a separate - `--checks` mode. `checks:failed` fires at the head's first failing required - check; `checks:passed` fires once every required check on the head passes. - Each event names the check and links its details. -- Decided (user): only required checks wake the wait. Read requiredness from - GraphQL `isRequired(pullRequestNumber:)` on check runs and status contexts, - which needs only read access and covers branch protection and rulesets. A - repository with no required checks treats every check as required. - Neutral and skipped results pass. Non-required failures never wake; - `quest status` lists them. -- Report a check result reached after `--since`, so a wait started after CI - finished still reports it. A new push starts over for the new head. The - resume cursor records what was reported, so resuming never replays it. -- Read checks in the existing poll; keep its interval, rate-limit handling, - and exit codes. Drop "Check runs ... are not activity" from `--help`. -- Update the `merge` and `complete` skill prose: one `quest wait` covers - CI, so delete the separate check watcher. -- Test a required failure, an optional failure that does not wake, all - required passing, no required checks, a push mid-wait, and resuming from - a cursor after a reported result. diff --git a/src/cli/main.ts b/src/cli/main.ts index e75c0d52..4f9114cb 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -140,14 +140,17 @@ export function main(argv: string[], out: Output): number | Promise { .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, or disabled auto-merge. Polls every 5 minutes; rate limits delay requests. No timeout by default.\n\n" + + "merge-queue removal, disabled auto-merge, or required CI result. Polls every 5 minutes; rate limits 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. " + + "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. " + + "Neutral and skipped results pass; optional checks never wake the wait, unless the base branch requires none, which 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. Replies and reactions by that account are ignored. " + "Exits 0 for activity, 3 on timeout, 2 for invalid arguments, and 1 for other failures. " + - "Check runs and edits are not activity.", + "Edits are not activity.", ) .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)") diff --git a/src/cli/wait.ts b/src/cli/wait.ts index cb2fe082..aa57c757 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -21,6 +21,26 @@ const REACTIONS = `query($owner: String!, $repo: String!, $number: Int!, $after: } } fragment reaction on Reaction { id content createdAt user { login } }`; +/** The head's check runs and commit statuses, with whether the base branch requires each. */ +const CHECKS = `query($owner: String!, $repo: String!, $number: Int!, $after: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + headRefOid + commits(last: 1) { nodes { commit { oid statusCheckRollup { + contexts(first: 100, after: $after) { + pageInfo { hasNextPage endCursor } + nodes { + __typename + ... on CheckRun { name status conclusion completedAt detailsUrl isRequired(pullRequestNumber: $number) } + ... on StatusContext { context state createdAt targetUrl isRequired(pullRequestNumber: $number) } + } + } + } } } } + } + } +}`; +/** Branch protection and rulesets treat these as passing. */ +const PASSING = ["SUCCESS", "NEUTRAL", "SKIPPED"]; export interface PullRequest { owner: string; @@ -77,6 +97,16 @@ interface Cursor { commits: string[]; /** Force pushes at or before the starting point, at their repository-activity time. */ pushes: { sha: string; time: number }[]; + /** Check results reached at or after `since` are reported once per head, as `:`. */ + checks: { since: number; reported: string[] }; +} + +/** One check run or commit status on the head, with its result once it has one. */ +interface Check { + name: string; + required: boolean; + url?: string; + result?: { outcome: "passed" | "failed"; point: string; time: number }; } /** Accept a GitHub PR URL, or a number in the origin repository. */ @@ -291,6 +321,7 @@ async function watch( pushes: forced .filter((row) => !after(row)) .map((row) => ({ sha: string(row.after), time: timestamp(string(row.timestamp)) })), + checks: { since, reported: [] }, }; const plain = pushed.filter((row) => string(row.activity_type) !== "force_push" && after(row)); // A push racing setup is reported once, as a push, not again as a new commit. @@ -344,24 +375,27 @@ async function watch( } }; }; + const graphql = async (query: string, after: string | null): Promise => { + const response = await request("/graphql", undefined, { + query, + variables: { owner: pr.owner, repo: pr.repo, number: pr.number, after }, + }); + const body = object(await response.json()); + if (body.errors !== undefined) + throw new Error( + `GitHub GraphQL error: ${array(body.errors) + .map((error) => String(error.message)) + .join("; ")}`, + ); + return object(body.data); + }; /** REST has no endpoint for reactions on reviews, and GraphQL also names the waiter. */ const reactions = async (): Promise<{ viewer: string; events: Activity[] }> => { const events: Activity[] = []; let viewer: string; let after: string | null = null; do { - const response = await request("/graphql", undefined, { - query: REACTIONS, - variables: { owner: pr.owner, repo: pr.repo, number: pr.number, after }, - }); - const body = object(await response.json()); - if (body.errors !== undefined) - throw new Error( - `GitHub GraphQL error: ${array(body.errors) - .map((error) => String(error.message)) - .join("; ")}`, - ); - const data = object(body.data); + const data = await graphql(REACTIONS, after); viewer = string(object(data.viewer).login); const pull = object(object(data.repository).pullRequest); const reviews = object(pull.reviews); @@ -374,6 +408,26 @@ async function watch( } while (after !== null); return { viewer, events }; }; + /** GraphQL says which checks the base branch requires, through branch protection or rulesets. */ + const checks = async (): Promise<{ head: string; checks: Check[] } | undefined> => { + const result: Check[] = []; + let head: string | undefined; + let after: string | null = null; + do { + const pull = object(object((await graphql(CHECKS, after)).repository).pullRequest); + const commit = object(array(object(pull.commits).nodes)[0]?.commit); + // A push racing these pages is read whole on the next poll. + if (commit.oid !== pull.headRefOid || (head !== undefined && commit.oid !== head)) return undefined; + head = string(commit.oid); + if (!fullSha(head)) throw new Error("invalid GitHub head SHA"); + if (commit.statusCheckRollup === null) break; + const contexts = object(object(commit.statusCheckRollup).contexts); + result.push(...array(contexts.nodes).map(check)); + const page = object(contexts.pageInfo); + after = page.hasNextPage === true ? string(page.endCursor) : null; + } while (after !== null); + return { head: head!, checks: result }; + }; const timeline = pager(`${base}/issues/${pr.number}/timeline?per_page=100`); // `since` excludes its own instant and filters by update time, which is never before @@ -426,8 +480,11 @@ 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))); + // Check results are state rather than a log, so each poll reads them afresh. + const head = await checks(); + const reached = head ? checkActivities(head.head, head.checks, cursor.checks, prUrl) : []; - if (SOURCES.some((source) => found[source].length)) { + if (reached.length || SOURCES.some((source) => found[source].length)) { const marks = Object.fromEntries( SOURCES.map((source) => { const mark = cursor.marks[source]; @@ -444,8 +501,16 @@ async function watch( marks, commits: [...seenCommits], pushes: cursor.pushes.filter((push) => push.time + TIMELINE_LAG >= marks.timeline.time), + checks: { + since: cursor.checks.since, + // A new head starts over, so only the current head's reports are kept. + reported: [ + ...cursor.checks.reported.filter((id) => !head || id.startsWith(`${head.head}:`)), + ...reached.map((event) => event.id), + ], + }, }); - const all = SOURCES.flatMap((source) => found[source]).sort((a, b) => a.time - b.time); + const all = [...SOURCES.flatMap((source) => found[source]), ...reached].sort((a, b) => a.time - b.time); const unique = [...new Map(all.map((event) => [event.id, event])).values()]; return unique.map( (event, index) => @@ -476,6 +541,53 @@ function timelineActivities(row: ObjectValue, prUrl: string): Activity[] { return [timedActivity(row, type, type === "reviewed" ? "submitted_at" : "created_at", prUrl)]; } +/** A check run or commit status; neutral and skipped results pass, as branch protection treats them. */ +function check(row: ObjectValue): Check { + const run = row.__typename === "CheckRun"; + if (!run && row.__typename !== "StatusContext") throw new Error("invalid GitHub check"); + const link = run ? row.detailsUrl : row.targetUrl; + const found: Check = { + name: string(run ? row.name : row.context), + required: row.isRequired === true, + url: typeof link === "string" ? link : undefined, + }; + const state = run ? (row.status === "COMPLETED" ? string(row.conclusion) : undefined) : string(row.state); + if (state === undefined || state === "PENDING" || state === "EXPECTED") return found; + const point = string(run ? row.completedAt : row.createdAt); + found.result = { outcome: PASSING.includes(state) ? "passed" : "failed", point, time: timestamp(point) }; + return found; +} + +/** + * The head's first required failure, and every required check passing, each reported once per head + * when reached at or after the starting point. A repository that requires no checks requires them all. + */ +function checkActivities(head: string, all: Check[], state: Cursor["checks"], prUrl: string): Activity[] { + const required = all.some((entry) => entry.required) ? all.filter((entry) => entry.required) : 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 []; + return [ + { + point: entry.result.point, + time: entry.result.time, + type: `checks:${outcome}`, + actor: entry.name.replace(/\s/g, "_"), + url: entry.url ?? `${prUrl}/checks`, + id, + }, + ]; + }; + 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); + const passed = + settled.length === required.length && settled.every((entry) => entry.result.outcome === "passed") + ? settled.at(-1) + : undefined; + return [...reached("failed", failed), ...reached("passed", passed)]; +} + function timedActivity(row: ObjectValue, type: string, timeField: string, fallback: string): Activity { const point = string(row[timeField]); const id = row.id; @@ -561,6 +673,7 @@ function decodeCursor(value: string, pr: string): Cursor { try { const parsed = object(JSON.parse(Buffer.from(value.slice(3), "base64url").toString("utf8"))); const marks = object(parsed.marks); + const checks = object(parsed.checks); if ( parsed.pr !== pr || !SOURCES.every((source) => { @@ -575,7 +688,10 @@ function decodeCursor(value: string, pr: string): Cursor { !Array.isArray(parsed.commits) || !parsed.commits.every(fullSha) || !Array.isArray(parsed.pushes) || - !parsed.pushes.every((push: ObjectValue | null) => fullSha(push?.sha) && Number.isFinite(push?.time)) + !parsed.pushes.every((push: ObjectValue | null) => fullSha(push?.sha) && Number.isFinite(push?.time)) || + !Number.isFinite(checks.since) || + !Array.isArray(checks.reported) || + !checks.reported.every((id) => typeof id === "string") ) throw new Error(); return parsed as unknown as Cursor; diff --git a/test/wait.test.ts b/test/wait.test.ts index 4725a0ee..d6c300f9 100644 --- a/test/wait.test.ts +++ b/test/wait.test.ts @@ -69,11 +69,74 @@ function replied(id: number, time = POINT, login = "reviewer", inReplyTo: number }; } -/** Review comments and reactions come from their own queues, empty once drained. */ -function setup(responses: Response[], baseline = [A], side: { comments?: Response[]; graphql?: Response[] } = {}) { +interface CheckFixture { + name: string; + required?: boolean; + /** A check run's conclusion, or a commit status's state; omitted while it runs. */ + result?: string; + time?: string; + status?: boolean; +} + +/** The head's checks; `null` means the head has no checks at all. */ +function checks( + contexts: CheckFixture[] | null = null, + head = A, + page: { hasNextPage: boolean; endCursor: string | null } = { hasNextPage: false, endCursor: null }, + listed = head, +) { + const node = (entry: CheckFixture) => + entry.status + ? { + __typename: "StatusContext", + context: entry.name, + state: entry.result ?? "PENDING", + createdAt: entry.time ?? POINT, + targetUrl: null, + isRequired: entry.required ?? false, + } + : { + __typename: "CheckRun", + name: entry.name, + status: entry.result ? "COMPLETED" : "IN_PROGRESS", + conclusion: entry.result ?? null, + completedAt: entry.result ? (entry.time ?? POINT) : null, + detailsUrl: `https://github.com/example/project/actions/runs/1/job/${entry.name.length}`, + isRequired: entry.required ?? false, + }; + return response({ + data: { + repository: { + pullRequest: { + headRefOid: head, + commits: { + nodes: [ + { + commit: { + oid: listed, + statusCheckRollup: contexts && { + contexts: { pageInfo: page, nodes: contexts.map(node) }, + }, + }, + }, + ], + }, + }, + }, + }, + }); +} + +/** Review comments, reactions, and checks come from their own queues, empty once drained. */ +function setup( + responses: Response[], + baseline = [A], + side: { comments?: Response[]; graphql?: Response[]; checks?: Response[] } = {}, +) { let now = Date.parse("2026-10-07T12:10:00Z"); const calls: { path: string; headers: Headers }[] = []; const sideCalls: { path: string; headers: Headers; body?: unknown }[] = []; + const checkCalls: { path: string; headers: Headers; body?: unknown }[] = []; const sleeps: number[] = []; const warnings: string[] = []; const deps: WaitDependencies = { @@ -92,7 +155,12 @@ function setup(responses: Response[], baseline = [A], side: { comments?: Respons return side.comments?.shift() ?? response([]); } if (parsed.pathname === "/graphql") { - sideCalls.push({ ...call, body: JSON.parse(String(init?.body)) }); + const body = JSON.parse(String(init?.body)) as { query: string }; + if (body.query.includes("statusCheckRollup")) { + checkCalls.push({ ...call, body }); + return side.checks?.shift() ?? checks(); + } + sideCalls.push({ ...call, body }); return side.graphql?.shift() ?? reactions(); } calls.push(call); @@ -102,7 +170,7 @@ function setup(responses: Response[], baseline = [A], side: { comments?: Respons return next; }, }; - return { deps, calls, sideCalls, sleeps, warnings, responses }; + return { deps, calls, sideCalls, checkCalls, sleeps, warnings, responses }; } function graphql(fixture: ReturnType) { @@ -617,3 +685,145 @@ test("GraphQL errors fail loudly instead of hiding reactions", async () => { }); await expect(waitForActivity(pr, {}, fixture.deps)).rejects.toThrow("GitHub GraphQL error"); }); + +const LATER = "2026-10-07T12:05:00Z"; +const LATEST = "2026-10-07T12:08:00Z"; + +test("a required failure wakes the wait, naming the check and linking its details", async () => { + const fixture = setup([pull(), response([pushed()]), response([committed(A)])], [A], { + checks: [ + checks([ + { name: "lint", result: "FAILURE", time: LATER }, + { name: "build and test", required: true, result: "TIMED_OUT", time: LATEST }, + { name: "check", required: true }, + ]), + ], + }); + const lines = await waitForActivity(pr, {}, fixture.deps); + expect(lines).toHaveLength(1); + expect(lines[0]).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 every required check passing does", async () => { + const fixture = setup([pull(), response([pushed()]), 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" }, + ]), + ], + }); + const lines = await waitForActivity(pr, {}, fixture.deps); + expect(fixture.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\./, + ); +}); + +test("a repository with no required checks requires every check, commit statuses included", async () => { + const fixture = setup( + [pull(), response([pushed()]), response([committed(A)]), response([committed(A)]), response([committed(A)])], + [A], + { + checks: [ + checks([]), + checks([ + { name: "check", result: "NEUTRAL", time: LATER }, + { name: "ci/legacy", status: true }, + ]), + checks([ + { name: "check", result: "NEUTRAL", time: LATER }, + { name: "ci/legacy", status: true, result: "ERROR", time: LATEST }, + ]), + ], + }, + ); + const lines = await waitForActivity(pr, {}, fixture.deps); + expect(fixture.sleeps).toEqual([300_000, 300_000]); + expect(lines).toHaveLength(1); + expect(lines[0]).toMatch( + /^2026-10-07T12:08:00Z checks:failed ci\/legacy https:\/\/github.com\/example\/project\/pull\/1\/checks q1\./, + ); +}); + +test("results reached before an explicit starting point are not reported", async () => { + const fixture = setup([pull(), response([pushed()]), response([committed(A)])], [A], { + checks: [checks([{ name: "check", required: true, result: "FAILURE", time: LATER }])], + }); + const timeout = await waitForActivity(pr, { since: LATEST, timeout: 10 }, fixture.deps).catch( + (error: unknown) => error, + ); + expect(timeout).toBeInstanceOf(WaitTimeout); +}); + +test("a resumed wait never replays a reported result, but a push starts over for the new head", async () => { + const failing = (head: string, time = LATER) => + checks([{ name: "check", required: true, result: "FAILURE", time }], head); + const first = setup([pull(), response([pushed()]), response([committed(A)])], [A], { checks: [failing(A)] }); + const reported = await waitForActivity(pr, {}, first.deps); + expect(reported[0]).toContain("checks:failed check"); + // The same failure is not reported again; the push is. + const second = setup([response([committed(A)]), response([committed(A), committed(B)])], [A], { + checks: [failing(A), failing(A)], + }); + const pushedLines = await waitForActivity(pr, { since: cursor(reported) }, second.deps); + expect(second.sleeps).toEqual([300_000]); + expect(pushedLines).toHaveLength(1); + expect(pushedLines[0]).toMatch(new RegExp(`^${B} committed `)); + // The new head's failure is reported even though the old head's was. + const third = setup([response([committed(A), committed(B)])], [A], { checks: [failing(B, LATEST)] }); + const again = await waitForActivity(pr, { since: cursor(pushedLines) }, third.deps); + expect(again).toHaveLength(1); + expect(again[0]).toMatch(/^2026-10-07T12:08:00Z checks:failed check /); + // A later pass on that head is still reported once. + const fourth = setup([response([committed(A), committed(B)])], [A], { + checks: [checks([{ name: "check", required: true, result: "SUCCESS", time: "2026-10-07T12:09:00Z" }], B)], + }); + const passed = await waitForActivity(pr, { since: cursor(again) }, fourth.deps); + expect(passed).toHaveLength(1); + expect(passed[0]).toContain("checks:passed check"); +}); + +test("a timed-out wait's cursor still reports a result reached during the first wait", async () => { + const first = setup([pull(), response([pushed()]), response([committed(A)])], [A], { + checks: [checks([{ name: "check", required: true }])], + }); + const timeout = await waitForActivity(pr, { timeout: 10 }, first.deps).catch((error: unknown) => error); + const second = setup([response([committed(A)])], [A], { + checks: [checks([{ name: "check", required: true, result: "SUCCESS", time: LATER }])], + }); + const lines = await waitForActivity(pr, { since: (timeout as WaitTimeout).cursor }, second.deps); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain("checks:passed check"); +}); + +test("checks are read from every page, and a head racing the read waits for the next poll", async () => { + const page = { hasNextPage: true, endCursor: "next" }; + const fixture = setup([pull(), response([pushed()]), response([committed(A)]), response([committed(A)])], [A], { + checks: [ + checks([{ name: "check", required: true, result: "FAILURE", time: LATER }], A, undefined, B), + checks([{ name: "check", required: true, result: "SUCCESS", time: LATER }], A, page), + checks([{ name: "test", required: true, result: "FAILURE", time: LATEST }]), + ], + }); + const lines = await waitForActivity(pr, {}, fixture.deps); + expect(fixture.sleeps).toEqual([300_000]); + expect(lines).toHaveLength(1); + expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:failed test /); + expect(fixture.checkCalls.map((call) => (call.body as { variables: { after: unknown } }).variables.after)).toEqual([ + null, + null, + "next", + ]); +}); From 6dd9162f7115120e5690a49dbafa2e450130cc13 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 10:50:52 -0700 Subject: [PATCH 3/5] fix(wait): never let optional checks stand in for unstarted required ones Read the base branch's required check names from branch protection and rulesets once per wait. Only when it requires none does every check count as required, and a required check that has not started holds back checks:passed. Per-check requiredness still comes from isRequired. Co-Authored-By: Claude Opus 5.5 --- README.md | 15 +++++---- src/cli/main.ts | 3 +- src/cli/wait.ts | 50 +++++++++++++++++++++++----- test/wait.test.ts | 85 ++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 132 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index d2fb2f23..40ff6363 100644 --- a/README.md +++ b/README.md @@ -184,12 +184,12 @@ prints `checks:failed`, and every required check passing prints the actor field and its details URL. Neutral and skipped results pass. 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 -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. GitHub lists only checks that -have started, so a required check created late, such as a job that waits on -others, can start after `checks:passed` prints. +requires none, in which case every check is required. A required check that +has not started yet, such as a job that waits on others, holds back +`checks:passed`. 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. Review-thread replies and reactions never reach the PR timeline, so they are read separately. A reply prints as `replied`; a thread's first comment is part @@ -206,7 +206,8 @@ visible after a newer reaction is still reported. Each poll makes one conditional timeline request and one conditional review-comment request every five minutes once it has read the existing pages, following a new page when the current one fills, plus one GraphQL query each -for reactions and checks. It honors GitHub's rate-limit headers and fails loudly on invalid responses. +for reactions and checks. It reads the base branch's required checks once per +call. It honors GitHub's rate-limit headers and fails loudly on invalid responses. There is no deadline unless `--timeout ` is supplied. Activity exits 0, timeout exits 3, invalid arguments exit 2, and other failures exit 1; interrupt with Ctrl-C to stop waiting. A timeout prints a resume point as its only line: diff --git a/src/cli/main.ts b/src/cli/main.ts index 4f9114cb..fe614e55 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -145,7 +145,8 @@ export function main(argv: string[], out: Output): number | Promise { "Reactions print as reacted:, such as reacted:thumbs_up. " + "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. " + - "Neutral and skipped results pass; optional checks never wake the wait, unless the base branch requires none, which requires them all. " + + "Neutral and skipped results pass, and a required check not yet started holds back checks:passed. " + + "Optional checks never wake the wait, unless the base branch requires none, which 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. Replies and reactions by that account are ignored. " + diff --git a/src/cli/wait.ts b/src/cli/wait.ts index aa57c757..59557a35 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -25,6 +25,7 @@ fragment reaction on Reaction { id content createdAt user { login } }`; const CHECKS = `query($owner: String!, $repo: String!, $number: Int!, $after: String) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { + baseRefName headRefOid commits(last: 1) { nodes { commit { oid statusCheckRollup { contexts(first: 100, after: $after) { @@ -409,12 +410,13 @@ async function watch( return { viewer, events }; }; /** GraphQL says which checks the base branch requires, through branch protection or rulesets. */ - const checks = async (): Promise<{ head: string; checks: Check[] } | undefined> => { + const checks = async (): Promise<{ head: string; base: string; checks: Check[] } | undefined> => { const result: Check[] = []; let head: string | undefined; let after: string | null = null; + let pull: ObjectValue; do { - const pull = object(object((await graphql(CHECKS, after)).repository).pullRequest); + pull = object(object((await graphql(CHECKS, after)).repository).pullRequest); const commit = object(array(object(pull.commits).nodes)[0]?.commit); // A push racing these pages is read whole on the next poll. if (commit.oid !== pull.headRefOid || (head !== undefined && commit.oid !== head)) return undefined; @@ -426,7 +428,29 @@ async function watch( const page = object(contexts.pageInfo); after = page.hasNextPage === true ? string(page.endCursor) : null; } while (after !== null); - return { head: head!, checks: result }; + return { head: head!, base: string(pull.baseRefName), checks: result }; + }; + /** + * The check names the base branch requires, read once per wait. Only a requirement says whether a + * missing check has yet to start, or whether the repository requires none. + */ + const requirements = new Map>(); + const required = async (branch: string): Promise> => { + const known = requirements.get(branch); + if (known) return known; + const path = `${base}/branches/${encodeURIComponent(branch)}`; + const protection = object(object(await (await request(path)).json()).protection ?? {}); + const legacy = object(protection.required_status_checks ?? { checks: [] }); + const rules = array(await (await request(`${base}/rules/branches/${encodeURIComponent(branch)}`)).json()); + const names = new Set([ + ...(legacy.enforcement_level === "off" ? [] : array(legacy.checks).map((row) => string(row.context))), + ...rules + .filter((rule) => rule.type === "required_status_checks") + .flatMap((rule) => array(object(rule.parameters).required_status_checks)) + .map((row) => string(row.context)), + ]); + requirements.set(branch, names); + return names; }; const timeline = pager(`${base}/issues/${pr.number}/timeline?per_page=100`); @@ -482,7 +506,9 @@ async function watch( found.reactions.push(...events.filter((event) => event.actor !== viewer && fresh("reactions", event))); // Check results are state rather than a log, so each poll reads them afresh. const head = await checks(); - const reached = head ? checkActivities(head.head, head.checks, cursor.checks, prUrl) : []; + const reached = head + ? checkActivities(head.head, head.checks, await required(head.base), cursor.checks, prUrl) + : []; if (reached.length || SOURCES.some((source) => found[source].length)) { const marks = Object.fromEntries( @@ -560,10 +586,18 @@ function check(row: ObjectValue): Check { /** * The head's first required failure, and every required check passing, each reported once per head - * when reached at or after the starting point. A repository that requires no checks requires them all. + * when reached at or after the starting point. A base branch that requires no checks requires them all. */ -function checkActivities(head: string, all: Check[], state: Cursor["checks"], prUrl: string): Activity[] { - const required = all.some((entry) => entry.required) ? all.filter((entry) => entry.required) : all; +function checkActivities( + head: string, + all: Check[], + names: Set, + state: Cursor["checks"], + prUrl: string, +): Activity[] { + const required = names.size ? all.filter((entry) => entry.required) : all; + // GitHub lists a check only once it starts, so a required one created late is still to come. + const started = [...names].every((name) => all.some((entry) => entry.name === name)); 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 []; @@ -582,7 +616,7 @@ function checkActivities(head: string, all: Check[], state: Cursor["checks"], pr settled.sort((a, b) => a.result.time - b.result.time); const failed = settled.find((entry) => entry.result.outcome === "failed" && entry.result.time >= state.since); const passed = - settled.length === required.length && settled.every((entry) => entry.result.outcome === "passed") + started && settled.length === required.length && settled.every((entry) => entry.result.outcome === "passed") ? settled.at(-1) : undefined; return [...reached("failed", failed), ...reached("passed", passed)]; diff --git a/test/wait.test.ts b/test/wait.test.ts index d6c300f9..f42d63b8 100644 --- a/test/wait.test.ts +++ b/test/wait.test.ts @@ -108,6 +108,7 @@ function checks( data: { repository: { pullRequest: { + baseRefName: "main", headRefOid: head, commits: { nodes: [ @@ -127,16 +128,26 @@ function checks( }); } -/** Review comments, reactions, and checks come from their own queues, empty once drained. */ +/** + * Review comments, reactions, and checks come from their own queues, empty once drained. + * The base branch requires `protected` through branch protection and `ruled` through rulesets. + */ function setup( responses: Response[], baseline = [A], - side: { comments?: Response[]; graphql?: Response[]; checks?: Response[] } = {}, + side: { + comments?: Response[]; + graphql?: Response[]; + checks?: Response[]; + protected?: string[]; + ruled?: string[]; + } = {}, ) { let now = Date.parse("2026-10-07T12:10:00Z"); const calls: { path: string; headers: Headers }[] = []; const sideCalls: { path: string; headers: Headers; body?: unknown }[] = []; const checkCalls: { path: string; headers: Headers; body?: unknown }[] = []; + const ruleCalls: { path: string; headers: Headers }[] = []; const sleeps: number[] = []; const warnings: string[] = []; const deps: WaitDependencies = { @@ -163,6 +174,26 @@ function setup( sideCalls.push({ ...call, body }); return side.graphql?.shift() ?? reactions(); } + if (parsed.pathname === `${base}/branches/main`) { + ruleCalls.push(call); + const checks = (side.protected ?? []).map((context) => ({ context, app_id: null })); + return response({ + protection: { + enabled: checks.length > 0, + required_status_checks: { enforcement_level: checks.length ? "everyone" : "off", checks }, + }, + }); + } + if (parsed.pathname === `${base}/rules/branches/main`) { + ruleCalls.push(call); + const checks = (side.ruled ?? []).map((context) => ({ context, integration_id: 15368 })); + return response([ + { type: "deletion" }, + ...(checks.length + ? [{ type: "required_status_checks", parameters: { required_status_checks: checks } }] + : []), + ]); + } calls.push(call); if (parsed.pathname.endsWith("/commits")) return response(baseline.map((sha) => ({ sha }))); const next = responses.shift(); @@ -170,7 +201,7 @@ function setup( return next; }, }; - return { deps, calls, sideCalls, checkCalls, sleeps, warnings, responses }; + return { deps, calls, sideCalls, checkCalls, ruleCalls, sleeps, warnings, responses }; } function graphql(fixture: ReturnType) { @@ -698,6 +729,7 @@ test("a required failure wakes the wait, naming the check and linking its detail { name: "check", required: true }, ]), ], + protected: ["build and test", "check"], }); const lines = await waitForActivity(pr, {}, fixture.deps); expect(lines).toHaveLength(1); @@ -722,6 +754,7 @@ test("an optional failure never wakes the wait, and every required check passing { name: "slow" }, ]), ], + ruled: ["check", "test"], }); const lines = await waitForActivity(pr, {}, fixture.deps); expect(fixture.sleeps).toEqual([300_000]); @@ -729,6 +762,38 @@ test("an optional failure never wakes the wait, and every required check passing 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([ + `${base}/branches/main`, + `${base}/rules/branches/main`, + ]); +}); + +test("optional checks never stand in for required checks that have not started", 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 }]), + checks([ + { name: "lint", result: "FAILURE", time: LATER }, + { name: "check", required: true, result: "SUCCESS", time: LATER }, + ]), + checks([ + { name: "lint", result: "FAILURE", time: LATER }, + { name: "check", required: true, result: "SUCCESS", time: LATER }, + { name: "gate", required: true, result: "SUCCESS", time: LATEST }, + ]), + ], + protected: ["check"], + ruled: ["gate"], + }, + ); + const lines = await waitForActivity(pr, {}, fixture.deps); + expect(fixture.sleeps).toEqual([300_000, 300_000]); + expect(lines).toHaveLength(1); + expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:passed gate /); }); test("a repository with no required checks requires every check, commit statuses included", async () => { @@ -760,6 +825,7 @@ test("a repository with no required checks requires every check, commit statuses test("results reached before an explicit starting point are not reported", async () => { const fixture = setup([pull(), response([pushed()]), response([committed(A)])], [A], { checks: [checks([{ name: "check", required: true, result: "FAILURE", time: LATER }])], + protected: ["check"], }); const timeout = await waitForActivity(pr, { since: LATEST, timeout: 10 }, fixture.deps).catch( (error: unknown) => error, @@ -770,25 +836,31 @@ test("results reached before an explicit starting point are not reported", async test("a resumed wait never replays a reported result, but a push starts over for the new head", async () => { const failing = (head: string, time = LATER) => checks([{ name: "check", required: true, result: "FAILURE", time }], head); - const first = setup([pull(), response([pushed()]), response([committed(A)])], [A], { checks: [failing(A)] }); + const protect = { protected: ["check"] }; + const first = setup([pull(), response([pushed()]), response([committed(A)])], [A], { + checks: [failing(A)], + ...protect, + }); const reported = await waitForActivity(pr, {}, first.deps); expect(reported[0]).toContain("checks:failed check"); // The same failure is not reported again; the push is. const second = setup([response([committed(A)]), response([committed(A), committed(B)])], [A], { checks: [failing(A), failing(A)], + ...protect, }); const pushedLines = await waitForActivity(pr, { since: cursor(reported) }, second.deps); expect(second.sleeps).toEqual([300_000]); expect(pushedLines).toHaveLength(1); expect(pushedLines[0]).toMatch(new RegExp(`^${B} committed `)); // The new head's failure is reported even though the old head's was. - const third = setup([response([committed(A), committed(B)])], [A], { checks: [failing(B, LATEST)] }); + const third = setup([response([committed(A), committed(B)])], [A], { checks: [failing(B, LATEST)], ...protect }); const again = await waitForActivity(pr, { since: cursor(pushedLines) }, third.deps); expect(again).toHaveLength(1); expect(again[0]).toMatch(/^2026-10-07T12:08:00Z checks:failed check /); // A later pass on that head is still reported once. const fourth = setup([response([committed(A), committed(B)])], [A], { checks: [checks([{ name: "check", required: true, result: "SUCCESS", time: "2026-10-07T12:09:00Z" }], B)], + ...protect, }); const passed = await waitForActivity(pr, { since: cursor(again) }, fourth.deps); expect(passed).toHaveLength(1); @@ -798,10 +870,12 @@ test("a resumed wait never replays a reported result, but a push starts over for test("a timed-out wait's cursor still reports a result reached during the first wait", async () => { const first = setup([pull(), response([pushed()]), response([committed(A)])], [A], { checks: [checks([{ name: "check", required: true }])], + protected: ["check"], }); const timeout = await waitForActivity(pr, { timeout: 10 }, first.deps).catch((error: unknown) => error); const second = setup([response([committed(A)])], [A], { checks: [checks([{ name: "check", required: true, result: "SUCCESS", time: LATER }])], + protected: ["check"], }); const lines = await waitForActivity(pr, { since: (timeout as WaitTimeout).cursor }, second.deps); expect(lines).toHaveLength(1); @@ -816,6 +890,7 @@ test("checks are read from every page, and a head racing the read waits for the checks([{ name: "check", required: true, result: "SUCCESS", time: LATER }], A, page), checks([{ name: "test", required: true, result: "FAILURE", time: LATEST }]), ], + protected: ["check", "test"], }); const lines = await waitForActivity(pr, {}, fixture.deps); expect(fixture.sleeps).toEqual([300_000]); From 856e14bb6f305ec3e7cd6ad27fc34af35fa0994c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 12:47:50 -0700 Subject: [PATCH 4/5] fix(wait): require a ruleset's required workflow as a whole A workflows rule requires the workflow run to pass, not just the jobs visible so far. Read each check run's workflow file and suite status: every job in a required workflow counts as required, and checks:passed waits until each required workflow has a run on the head and that run has finished. Addresses Codex's review of 4d1a4a6. Co-Authored-By: Claude Opus 5.5 --- README.md | 5 +++-- src/cli/wait.ts | 45 ++++++++++++++++++++++++++-------------- test/wait.test.ts | 53 ++++++++++++++++++++++++++++++++++++++--------- 3 files changed, 76 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 17a7e7ac..f3b46385 100644 --- a/README.md +++ b/README.md @@ -188,8 +188,9 @@ the actor field and its details URL. Results count as `quest status` counts them, so neutral and skipped results pass. 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 ruleset that requires -workflows counts as requiring checks. A required check that has not started +requires none, in which case every check is required. A workflow a ruleset +requires passes as a whole: each of its jobs is required, and its run holds +back `checks:passed` until it finishes. A required check that has not started yet, such as a job that waits on others, holds back `checks:passed`; when the requirement names an app, only that app's check starts it. 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 diff --git a/src/cli/wait.ts b/src/cli/wait.ts index 58a14b1d..b04e0534 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -44,7 +44,7 @@ const CHECKS = `query($owner: String!, $repo: String!, $number: Int!, $after: St __typename ... on CheckRun { name status conclusion completedAt detailsUrl isRequired(pullRequestNumber: $number) - checkSuite { app { databaseId } } + checkSuite { status app { databaseId } workflowRun { file { path } } } } ... on StatusContext { context state createdAt targetUrl isRequired(pullRequestNumber: $number) } } @@ -103,6 +103,8 @@ interface Check { url: string | null; /** The GitHub App that reported a check run; commit statuses have none. */ app?: number; + /** The workflow file a GitHub Actions job runs in, and whether that workflow run has finished. */ + workflow?: { path: string; done: boolean }; result?: { outcome: "passed" | "failed"; point: string; time: number }; } @@ -110,8 +112,8 @@ interface Check { interface Requirements { /** Required checks by name, and the app that must report each when one is set. */ checks: { context: string; app?: number }[]; - /** A required workflow names no checks, but still means the base requires some. */ - workflows: boolean; + /** Workflow files that must pass as a whole, whatever jobs they create. */ + workflows: string[]; } /** Wait until activity, returning plain output lines. Network and time are injected. */ @@ -366,7 +368,10 @@ async function watch( .flatMap((rule) => array(object(rule.parameters).required_status_checks)) .map((row) => requirement(row.context, row.integration_id)), ], - workflows: rules.some((rule) => rule.type === "workflows"), + workflows: rules + .filter((rule) => rule.type === "workflows") + .flatMap((rule) => array(object(rule.parameters).workflows)) + .map((row) => string(row.path)), }; requirements.set(branch, found); return found; @@ -490,12 +495,14 @@ function timelineActivities(row: ObjectValue, prUrl: string): Activity[] { function check(row: ObjectValue): Check { const run = row.__typename === "CheckRun"; const { check: classified, outcome } = classifyCheck(row); - const app = run && row.checkSuite ? object(row.checkSuite).app : null; + const suite = run && row.checkSuite ? object(row.checkSuite) : undefined; + const file = suite?.workflowRun ? object(suite.workflowRun).file : null; const found: Check = { name: classified.name, required: classified.required, url: classified.url, - ...(app ? { app: Number(object(app).databaseId) } : {}), + ...(suite?.app ? { app: Number(object(suite.app).databaseId) } : {}), + ...(file ? { workflow: { path: string(object(file).path), done: suite!.status === "COMPLETED" } } : {}), }; if (outcome === "pending") return found; const point = string(run ? row.completedAt : row.createdAt); @@ -514,16 +521,24 @@ function checkActivities( state: Cursor["checks"], prUrl: string, ): Activity[] { - const required = needs.checks.length || needs.workflows ? all.filter((entry) => entry.required) : all; + const inWorkflow = (entry: Check) => entry.workflow !== undefined && needs.workflows.includes(entry.workflow.path); + const required = + needs.checks.length || needs.workflows.length + ? all.filter((entry) => entry.required || inWorkflow(entry)) + : all; // GitHub lists a check only once it starts, so a required one created late is still to come. - // A check from another app does not stand in for one bound to an app. - const started = needs.checks.every((need) => - all.some( - (entry) => - entry.name === need.context && - (need.app === undefined || entry.app === undefined || entry.app === need.app), - ), - ); + // A check from another app does not stand in for one bound to an app. A required workflow + // passes as a whole, so it holds back checks:passed until its run finishes. + const started = + needs.checks.every((need) => + all.some( + (entry) => + entry.name === need.context && + (need.app === undefined || entry.app === undefined || entry.app === need.app), + ), + ) && + needs.workflows.every((path) => all.some((entry) => entry.workflow?.path === path)) && + all.every((entry) => !inWorkflow(entry) || entry.workflow!.done); 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 []; diff --git a/test/wait.test.ts b/test/wait.test.ts index 73ab939f..c7e6e302 100644 --- a/test/wait.test.ts +++ b/test/wait.test.ts @@ -81,6 +81,9 @@ interface CheckFixture { status?: boolean; /** The GitHub App reporting a check run; GitHub Actions by default. */ app?: number; + /** The workflow file a job runs in, and whether its run is still going after this job. */ + workflow?: string; + running?: boolean; } /** The head's checks; `null` means the head has no checks at all. */ @@ -108,7 +111,11 @@ function checks( completedAt: entry.result ? (entry.time ?? POINT) : null, detailsUrl: `https://github.com/example/project/actions/runs/1/job/${entry.name.length}`, isRequired: entry.required ?? false, - checkSuite: { app: { databaseId: entry.app ?? ACTIONS } }, + checkSuite: { + status: entry.result && !entry.running ? "COMPLETED" : "IN_PROGRESS", + app: { databaseId: entry.app ?? ACTIONS }, + workflowRun: entry.workflow ? { file: { path: entry.workflow } } : null, + }, }; return response({ data: { @@ -851,21 +858,47 @@ test("required rules on a later page, and app-bound checks, hold back checks:pas ]); }); -test("a required workflow keeps optional checks optional before its checks start", async () => { - const fixture = setup([pull(), response([pushed()]), response([committed(A)]), response([committed(A)])], [A], { +const CI = ".github/workflows/ci.yml"; +const workflowRule = { type: "workflows", parameters: { workflows: [{ path: CI, repository_id: 1 }] } }; + +test("a required workflow passes as a whole, and keeps optional checks optional", 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 }]), + // The run's next job has not been created yet. + checks([ + { name: "lint", result: "FAILURE", time: LATER }, + { name: "build", result: "SUCCESS", time: LATER, workflow: CI, running: true }, + ]), + checks([ + { name: "lint", result: "FAILURE", time: LATER }, + { name: "build", result: "SUCCESS", time: LATER, workflow: CI }, + { name: "gate", result: "SUCCESS", time: LATEST, workflow: CI }, + ]), + ], + rules: [workflowRule], + }, + ); + const lines = await waitForActivity(pr, {}, fixture.deps); + expect(fixture.sleeps).toEqual([300_000, 300_000]); + expect(lines).toHaveLength(1); + expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:passed gate /); +}); + +test("a failing job in a required workflow wakes the wait", async () => { + const fixture = setup([pull(), response([pushed()]), response([committed(A)])], [A], { checks: [ - checks([{ name: "lint", result: "FAILURE", time: LATER }]), checks([ - { name: "lint", result: "FAILURE", time: LATER }, - { name: "build", required: true, result: "FAILURE", time: LATEST }, + { name: "lint", result: "SUCCESS", time: LATER }, + { name: "build", result: "FAILURE", time: LATEST, workflow: CI, running: true }, ]), ], - rules: [ - { type: "workflows", parameters: { workflows: [{ path: ".github/workflows/ci.yml", repository_id: 1 }] } }, - ], + rules: [workflowRule], }); const lines = await waitForActivity(pr, {}, fixture.deps); - expect(fixture.sleeps).toEqual([300_000]); expect(lines).toHaveLength(1); expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:failed build /); }); From 7168a69d0952a7cac80416b4bf8b4ca401f852cb Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 21 Oct 2026 13:13:19 -0700 Subject: [PATCH 5/5] fix(wait): date checks:passed when the head goes idle; require workflow-rule jobs - checks:passed now names and dates the last check to finish, so a wait started after the required checks finished still reports the pass once an optional check settles. - A ruleset's required workflow makes each of its jobs required (matched by workflow file path), and the workflow must be present before checks:passed, since GitHub may not mark those jobs isRequired. Addresses Codex's review of 5ed58e6. Co-Authored-By: Claude Opus 5.5 --- README.md | 11 ++++++----- src/cli/main.ts | 2 +- src/cli/wait.ts | 35 +++++++++++++++++++++++---------- test/wait.test.ts | 50 ++++++++++++++++++++++++++++++++++++++++------- 4 files changed, 75 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 6554fda3..3d46f906 100644 --- a/README.md +++ b/README.md @@ -213,11 +213,12 @@ 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 the base branch names is present, and -every required check passed; it names the last required check to finish. 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. +may still create jobs), every check and workflow the base branch names is +present, 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 +(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 diff --git a/src/cli/main.ts b/src/cli/main.ts index c15349e0..22372fc4 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -149,7 +149,7 @@ export function main(argv: string[], out: Output): number | Promise { "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. " + "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 the base branch names is present. " + + "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. " + "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" + diff --git a/src/cli/wait.ts b/src/cli/wait.ts index 7384db7e..160d08ba 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -44,7 +44,7 @@ const CHECKS = `query($owner: String!, $repo: String!, $number: Int!, $after: St __typename ... on CheckRun { name status conclusion completedAt detailsUrl isRequired(pullRequestNumber: $number) - checkSuite { status workflowRun { databaseId } } + checkSuite { status workflowRun { file { path } } } } ... on StatusContext { context state createdAt targetUrl isRequired(pullRequestNumber: $number) } } @@ -101,6 +101,8 @@ interface Check { name: string; required: boolean; url: string | null; + /** The workflow file a GitHub Actions job runs in. */ + workflow?: string; /** A GitHub Actions job whose workflow run has not finished, so it may still create jobs. */ running: boolean; result?: { outcome: "passed" | "failed"; point: string; time: number }; @@ -110,8 +112,8 @@ interface Check { interface Requirements { /** Required check names. */ names: Set; - /** A required workflow names no checks, but still means the base requires some. */ - workflows: boolean; + /** Workflow files that must pass as a whole, so each of their jobs is required. */ + workflows: string[]; } /** Wait until activity, returning plain output lines. Network and time are injected. */ @@ -349,7 +351,10 @@ async function watch( .flatMap((rule) => array(object(rule.parameters).required_status_checks)) .map((row) => string(row.context)), ]), - workflows: rules.some((rule) => rule.type === "workflows"), + workflows: rules + .filter((rule) => rule.type === "workflows") + .flatMap((rule) => array(object(rule.parameters).workflows)) + .map((row) => string(row.path)), }; requirements.set(branch, found); return found; @@ -474,10 +479,12 @@ function check(row: ObjectValue): Check { const run = row.__typename === "CheckRun"; const { check: classified, outcome } = classifyCheck(row); const suite = run && row.checkSuite ? object(row.checkSuite) : undefined; + const file = suite?.workflowRun ? object(suite.workflowRun).file : null; const found: Check = { name: classified.name, required: classified.required, url: classified.url, + ...(file ? { workflow: string(object(file).path) } : {}), running: suite?.workflowRun ? suite.status !== "COMPLETED" : false, }; if (outcome === "pending") return found; @@ -487,8 +494,9 @@ function check(row: ObjectValue): Check { } /** - * The head's first required failure, and every required check passing, each reported once per head - * when reached at or after the starting point. A base branch that requires no checks requires them all. + * The head's first required failure, and every required check passing once the head is idle, each + * reported once per head when reached at or after the starting point. A base branch that requires + * no checks requires them all. */ function checkActivities( head: string, @@ -497,7 +505,9 @@ function checkActivities( state: Cursor["checks"], prUrl: string, ): Activity[] { - const required = needs.names.size || needs.workflows ? all.filter((entry) => entry.required) : all; + 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 []; @@ -516,13 +526,18 @@ function checkActivities( 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 present. + // 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. const idle = all.every((entry) => entry.result && !entry.running) && - [...needs.names].every((name) => all.some((entry) => entry.name === name)); + [...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") - ? settled.at(-1) + ? all.reduce( + (last, entry) => (!last || entry.result!.time >= last.result!.time ? entry : last), + undefined, + ) : undefined; return [...reached("failed", failed), ...reached("passed", passed)]; } diff --git a/test/wait.test.ts b/test/wait.test.ts index 0cb95cc0..8081b614 100644 --- a/test/wait.test.ts +++ b/test/wait.test.ts @@ -79,7 +79,8 @@ interface CheckFixture { result?: string; time?: string; status?: boolean; - /** A finished job whose workflow run is still going, so it may yet create jobs. */ + /** The workflow file a job runs in, and whether its run is still going, so it may yet create jobs. */ + workflow?: string; running?: boolean; } @@ -110,7 +111,7 @@ function checks( isRequired: entry.required ?? false, checkSuite: { status: entry.result && !entry.running ? "COMPLETED" : "IN_PROGRESS", - workflowRun: { databaseId: 1 }, + workflowRun: { file: { path: entry.workflow ?? ".github/workflows/check.yml" } }, }, }; return response({ @@ -850,7 +851,10 @@ test("a required check named on a later page of rules holds back checks:passed", ]); }); -test("a required workflow keeps optional checks optional, and its unfinished run holds back checks:passed", async () => { +const CI = ".github/workflows/ci.yml"; +const workflowRule = { type: "workflows", parameters: { workflows: [{ path: CI, repository_id: 1 }] } }; + +test("a required workflow requires its jobs, keeps optional checks optional, and holds back checks:passed until its run finishes", async () => { const fixture = setup( [pull(), response([pushed()]), response([committed(A)]), response([committed(A)]), response([committed(A)])], [A], @@ -860,15 +864,15 @@ test("a required workflow keeps optional checks optional, and its unfinished run // The run's next job has not been created yet. checks([ { name: "lint", result: "FAILURE", time: LATER }, - { name: "build", required: true, result: "SUCCESS", time: LATER, running: true }, + { name: "build", result: "SUCCESS", time: LATER, workflow: CI, running: true }, ]), checks([ { name: "lint", result: "FAILURE", time: LATER }, - { name: "build", required: true, result: "SUCCESS", time: LATER }, - { name: "gate", required: true, result: "SUCCESS", time: LATEST }, + { name: "build", result: "SUCCESS", time: LATER, workflow: CI }, + { name: "gate", result: "SUCCESS", time: LATEST, workflow: CI }, ]), ], - rules: [{ type: "workflows", parameters: { workflows: [{ path: ".github/workflows/ci.yml" }] } }], + rules: [workflowRule], }, ); const lines = await waitForActivity(pr, {}, fixture.deps); @@ -877,6 +881,38 @@ test("a required workflow keeps optional checks optional, and its unfinished run expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:passed gate /); }); +test("a failing job in a required workflow wakes the wait", async () => { + const fixture = setup([pull(), response([pushed()]), response([committed(A)])], [A], { + checks: [ + checks([ + { name: "lint", result: "SUCCESS", time: LATER }, + { name: "build", result: "FAILURE", time: LATEST, workflow: CI, running: true }, + ]), + ], + rules: [workflowRule], + }); + const lines = await waitForActivity(pr, {}, fixture.deps); + expect(lines).toHaveLength(1); + expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:failed build /); +}); + +test("checks:passed is dated when the head goes idle, after required checks finished before the start", async () => { + const fixture = setup([pull(), response([pushed()]), response([committed(A)]), response([committed(A)])], [A], { + checks: [ + checks([{ name: "check", required: true, result: "SUCCESS", time: POINT }, { name: "slow" }]), + checks([ + { name: "check", required: true, result: "SUCCESS", time: POINT }, + { name: "slow", result: "SUCCESS", time: LATEST }, + ]), + ], + protected: ["check"], + }); + 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 /); +}); + test("with no required checks, an unfinished workflow run holds back checks:passed", async () => { const fixture = setup([pull(), response([pushed()]), response([committed(A)]), response([committed(A)])], [A], { checks: [