From 2ef603e0de3d5ccd9c5d538a23274f57a905309a Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 18:54:53 -0800 Subject: [PATCH 1/4] chore(quest): claim quest/m0/status-landing.md Quest-Claim: c39d63ef3bcbf9a5 From 1ea6dcf581b3992df70f078c5ee9a95801bbed7a Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 19:06:07 -0800 Subject: [PATCH 2/4] fix(status): report landing and checks as they are now quest status dropped no landing-stopped line once the PR was queued again, and counted a cancelled run beside a newer passing rerun of the same check as failing. A merge-queue entry or auto-merge enabled at or after the latest exit now clears it, and status, wait, and the land gate count only each check run's newest report per name and app. Co-Authored-By: Claude Opus 5.5 --- README.md | 9 +++-- cloudflare/dev/github.ts | 6 ++-- quest/m0/README.md | 1 - quest/m0/status-landing.md | 28 ---------------- src/cli/land.ts | 21 +++++------- src/cli/main.ts | 4 +-- src/cli/status.ts | 43 ++++++++++++++++++++++-- src/cli/wait.ts | 12 +++---- test/land.test.ts | 14 ++++++++ test/status.test.ts | 67 ++++++++++++++++++++++++++++++++++++++ test/wait.test.ts | 25 +++++++++++++- 11 files changed, 172 insertions(+), 58 deletions(-) delete mode 100644 quest/m0/status-landing.md diff --git a/README.md b/README.md index 31c9c53c..544e14d5 100644 --- a/README.md +++ b/README.md @@ -437,7 +437,7 @@ comes last, and a missing value prints as `-`: | `mergeable`, `merge-state`, `review-decision` | GitHub's values, lowercased. | | `auto-merge` | Method, who enabled it, when, and the head it was enabled on. | | `merge-queue` | Queue state, position, and when it was enqueued. | -| `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. | +| `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; `-` once the PR has entered the merge queue or auto-merge again since. | | `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` | `current` or `stale` against the head, state, reviewer, time, and URL of each reviewer's latest review. | @@ -447,7 +447,9 @@ comes last, and a missing value prints as `-`: | `starter-hold` | A draft its starter owns: last activity time, PR URL, and `last active` age. Last activity is not when ownership began. | The node ID is what GraphQL's `addPullRequestReviewThreadReply` and -`resolveReviewThread` take. Neutral and skipped checks pass. A required check +`resolveReviewThread` take. Neutral and skipped checks pass. Only each check's +newest run counts: a rerun, or the same workflow run again on the head, +supersedes an earlier run of that name from the same app. A required check is one branch protection or a ruleset requires for this PR. `--json` prints the same facts as one object with the fields `url`, `state`, `draft`, `head`, `mergeable`, `mergeState`, `reviewDecision`, `autoMerge`, `mergeQueue`, @@ -651,7 +653,8 @@ quest links with `quest resolve`. A failed batch halves the next batch's size, down to one PR, and each batch that lands grows it back by one; the order and size persist in the state directory. Conflicting additions stay queued. A PR whose required checks are still running stays queued -and sits out batches until they pass. If any are still pending two hours after +and sits out batches until they pass; only each check's newest run counts, as in +`quest status`. If any are still pending two hours after its pick (twice the default `--timeout`), the daemon comments once on that pick, naming them, since a required check that never reports would hold it silently. The sandbox checks the tip once, without bisection: success lands one squash commit per PR. A failure, or a base that diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index b988b536..d1e1b941 100644 --- a/cloudflare/dev/github.ts +++ b/cloudflare/dev/github.ts @@ -23,7 +23,7 @@ export type Pull = Issue & { /** Head branch; `quest/` associates the pull request with that quest. */ branch: string; checks: "SUCCESS" | "FAILURE" | "PENDING" | null; - /** The check runs behind `checks`. */ + /** The check runs behind `checks`, oldest first. */ /** `commit` is the one a run reports on; the head by default. */ runs?: { name: string; conclusion: string; url: string; app?: number; context?: boolean; commit?: string }[]; review: "APPROVED" | "CHANGES_REQUESTED" | "REVIEW_REQUIRED" | null; @@ -345,11 +345,13 @@ export const ACTIONS_APP = 15368; /** One page of a head's check runs and commit statuses, as `statusCheckRollup.contexts` pages them. */ function contextPage(pull: Pull, after: unknown) { const start = typeof after === "string" ? Number(after) : 0; - const all = (pull.runs ?? []).map((run) => + const all = (pull.runs ?? []).map((run, index) => run.context ? { __typename: "StatusContext", context: run.name, state: run.conclusion, targetUrl: run.url } : { __typename: "CheckRun", + // GitHub numbers runs as they are created. + databaseId: index + 1, name: run.name, status: "COMPLETED", conclusion: run.conclusion, diff --git a/quest/m0/README.md b/quest/m0/README.md index aa77a9c2..06a3328c 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -85,7 +85,6 @@ ownership-manifest design: - [Land through the merge queue with an async fixer](/quest/m0/queue-watcher/README.md) - GitHub's merge queue lands one PR at a time, and a host watcher fixes and re-enqueues each PR it drops - [Merge through a host gate](/quest/m0/host-land/README.md) - the host's fixer containers, and the frozen batched gate for the repositories not yet switched - [The land fixer resolves every mechanical failure](/quest/m0/land-fixer/README.md) - lockfile merges, protected edits as ready commits, workflow pushes, and the PR's own CI as context -- [Current landing status](/quest/m0/status-landing.md) - `quest status` drops a stale `landing-stopped` line and ignores superseded cancelled checks - [Agents don't stall](/quest/m0/agent-stalls.md) - long waits block in the same turn, pre-existing failures don't stop review, and reviewer outages become waits - [Follow-ups as agents report](/quest/m0/follow-ups-as-reports.md) - coordinators plan follow-ups as reports arrive, fewer of them, with standing milestone rules - [Context before decision prompts](/quest/m0/prompt-context.md) - every prompt explains the work first, and maintainer steps say why an agent can't take them diff --git a/quest/m0/status-landing.md b/quest/m0/status-landing.md deleted file mode 100644 index 8155fc6e..00000000 --- a/quest/m0/status-landing.md +++ /dev/null @@ -1,28 +0,0 @@ -# [XS] quest status reports landing and checks as they are now - -## Goal - -`quest status` stops printing a `landing-stopped` line once the PR has been -queued again, including through `quest merge --land`. A required check whose -newest run passed counts as passed, even if an older run of it was -cancelled. - -## Plan - -Decided 2026-10-31 (user, /quest-plan of the follow-ups from a moq-dev/moq -`/quest-complete` run). - -- `summarize` in `src/cli/status.ts` takes the last removal, auto-merge - disable, or ejection comment from the timeline, even if auto-merge was - turned on again later. Drop the line when auto-merge's `enabledAt`, or a - merge-queue entry, is newer than that event. -- Checks are classified one context at a time in `summarize` (`classifyCheck`). - A cancelled run and a newer passing run of the same check both count, so - the check still shows as failing. Count only the newest run of each check - name. - -Moved from m2 to m0 on 2026-11-01 (user, /quest-plan of the follow-ups from -the 2026-11-01 `/quest-complete` sessions): ✅ m0, beside the land quests · -leave it in m2. The moq-dev land gate and the agents picking merges read -`quest status` every day, so a stale `landing-stopped` line or a superseded -cancelled check sends them after PRs that are already queued and passing. diff --git a/src/cli/land.ts b/src/cli/land.ts index 4aaa366c..e21bf6f8 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -16,7 +16,7 @@ import { requirements, string, } from "./github"; -import { classifyCheck } from "./status"; +import { appOf, classifyCheck, newestChecks } from "./status"; import { committedSources, snapshot } from "./tree"; /** The check a gated base requires. Nothing reports it on a PR head, so GitHub's auto-merge only waits. */ @@ -132,7 +132,7 @@ export interface Waiting { const CONTEXTS = `pageInfo { hasNextPage endCursor } nodes { __typename - ... on CheckRun { name status conclusion detailsUrl checkSuite { app { databaseId } } } + ... on CheckRun { databaseId name status conclusion detailsUrl checkSuite { app { databaseId } } } ... on StatusContext { context state targetUrl } }`; @@ -436,17 +436,21 @@ async function contexts(github: GitHub, repository: Repository, node: ObjectValu } } -/** The required checks a head failed, and those pending or not yet reported, each from its app. */ +/** + * The required checks a head failed, and those pending or not yet reported, each from its app. Only + * each check's newest run counts, so a rerun supersedes a cancelled or failed run. + */ function checks(contexts: ObjectValue[], required: RequiredCheck[]): { failing: string[]; pending: string[] } { const failing: string[] = []; const pending: string[] = []; + const newest = newestChecks(contexts); for (const need of required) { let result: "passed" | "failing" | "pending" = "pending"; let reported = false; - for (const context of contexts) { + for (const context of newest) { const { check, outcome } = classifyCheck(context); if (check.name !== need.name || (need.app !== null && appOf(context) !== need.app)) continue; - // A name reported twice, as by a rerun, counts only once every report passes. + // A name reported by more than one app, or as both a run and a status, counts only once every report passes. if (!reported || result === "passed") result = outcome; reported = true; } @@ -457,13 +461,6 @@ function checks(contexts: ObjectValue[], required: RequiredCheck[]): { failing: return { failing: failing.sort(), pending: pending.sort() }; } -/** The GitHub App behind a check run; a commit status has none the gate can verify. */ -function appOf(context: ObjectValue): number | null { - if (context.__typename !== "CheckRun" || context.checkSuite == null) return null; - const app = object(context.checkSuite).app; - return app == null ? null : Number(object(app).databaseId); -} - /** A queued PR's squash commit on the chain. */ interface Link { entry: Entry; diff --git a/src/cli/main.ts b/src/cli/main.ts index e616d616..b090a9fc 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -455,8 +455,8 @@ export function main(argv: string[], out: Output): number | Promise { "push (- when it cannot be told: a quest land pin names it, and otherwise the head branch's push " + "activity, which GitHub lists up to minutes late); 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 " + + "actor, and time, until the PR enters the merge queue or auto-merge again. Then " + + "a check line per failing or pending check, marked required or optional, counting only each check's newest run; 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 1d7feb7e..170d5c33 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -112,7 +112,10 @@ export interface Status { /** `head` is the head it was enabled on, or null when that cannot be told; see {@link enabledHead}. */ autoMerge: { method: string; enabledBy: string; enabledAt: string; head: string | null } | null; mergeQueue: { state: string; position: number; enqueuedAt: string } | null; - /** The latest exit from the merge queue, auto-merge, or `quest land`'s queue, and the reason for it. */ + /** + * The latest exit from the merge queue, auto-merge, or `quest land`'s queue, and the reason for it; + * null once the PR is queued again. + */ landingStopped: { reason: string; actor: string; at: string } | null; checks: { state: string | null; passed: number; failing: Check[]; pending: Check[] }; /** Each reviewer's latest submitted review, oldest first. */ @@ -265,7 +268,7 @@ function summarize( failing: [], pending: [], } as Status["checks"]; - for (const context of contexts) { + for (const context of newestChecks(contexts)) { const { check, outcome } = classifyCheck(context); if (outcome === "passed") checks.passed++; else checks[outcome].push(check); @@ -317,9 +320,16 @@ function summarize( const autoMerge = pull.autoMergeRequest === null ? null : object(pull.autoMergeRequest); const queue = pull.mergeQueueEntry === null ? null : object(pull.mergeQueueEntry); - const stopped = array(object(pull.timelineItems).nodes) + const last = array(object(pull.timelineItems).nodes) .filter((node) => node.__typename !== "IssueComment" || ejection(node) !== null) .at(-1); + // Entering the merge queue or auto-merge at or after the latest exit supersedes it. `quest land`'s + // enqueue disables auto-merge just before enabling it again, often within the same second. + const entered = [autoMerge?.enabledAt, queue?.enqueuedAt].flatMap((at) => + at == null ? [] : [Date.parse(string(at))], + ); + const stopped = + last !== undefined && !entered.some((at) => at >= Date.parse(string(last.createdAt))) ? last : undefined; return { url, state: lower(pull.state) as Status["state"], @@ -370,6 +380,33 @@ function ejection(node: ObjectValue): string | null { return trusted && typeof node.body === "string" ? (EJECTED.exec(node.body)?.[1] ?? null) : null; } +/** + * Each check's newest report, in rollup order. A rerun, or a workflow run again on the same head, + * reports a check run again under the same name from the same app, and only the newest counts, as it + * does for GitHub's required checks; check run IDs increase as runs are created. A commit status is + * already its context's newest. Check runs need `databaseId` and `checkSuite { app { databaseId } }`. + */ +export function newestChecks(contexts: ObjectValue[]): ObjectValue[] { + const newest = new Map(); + for (const context of contexts) { + if (context.__typename !== "CheckRun") continue; + const key = JSON.stringify([appOf(context), context.name]); + const seen = newest.get(key); + if (seen === undefined || Number(context.databaseId) > Number(seen.databaseId)) newest.set(key, context); + } + return contexts.filter( + (context) => + context.__typename !== "CheckRun" || newest.get(JSON.stringify([appOf(context), context.name])) === context, + ); +} + +/** The GitHub App behind a check run; a commit status has none. */ +export function appOf(context: ObjectValue): number | null { + if (context.__typename !== "CheckRun" || context.checkSuite == null) return null; + const app = object(context.checkSuite).app; + return app == null ? null : Number(object(app).databaseId); +} + /** * A check run or commit status from a `statusCheckRollup` context, and whether it passed, failed, or * is still pending. `quest wait` shares this, so both commands agree on what passes. diff --git a/src/cli/wait.ts b/src/cli/wait.ts index 308b3934..f4a08145 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -15,7 +15,7 @@ import { string, } from "./github"; import { LAND_CHECK } from "./land"; -import { classifyCheck, enabledHead } from "./status"; +import { classifyCheck, enabledHead, newestChecks } from "./status"; const POLL = 5 * 60_000; /** @@ -60,8 +60,8 @@ const CHECKS = `query($owner: String!, $repo: String!, $number: Int!, $after: St nodes { __typename ... on CheckRun { - name status conclusion completedAt detailsUrl isRequired(pullRequestNumber: $number) - checkSuite { status workflowRun { file { path } } } + databaseId name status conclusion completedAt detailsUrl isRequired(pullRequestNumber: $number) + checkSuite { status app { databaseId } workflowRun { file { path } } } } ... on StatusContext { context state createdAt targetUrl isRequired(pullRequestNumber: $number) } } @@ -345,7 +345,7 @@ async function watch( const checks = async (): Promise< { head: string; base: string; checks: Check[]; pull: ObjectValue } | undefined > => { - const result: Check[] = []; + const rows: ObjectValue[] = []; let head: string | undefined; let after: string | null = null; let pull: ObjectValue; @@ -358,11 +358,11 @@ async function watch( 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)); + rows.push(...array(contexts.nodes)); const page = object(contexts.pageInfo); after = page.hasNextPage === true ? string(page.endCursor) : null; } while (after !== null); - return { head: head!, base: string(pull.baseRefName), checks: result, pull }; + return { head: head!, base: string(pull.baseRefName), checks: newestChecks(rows).map(check), pull }; }; /** * What the base branch requires, read once per wait. Only a requirement says whether a missing diff --git a/test/land.test.ts b/test/land.test.ts index 6cde2067..1166243b 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1031,6 +1031,20 @@ describe.skipIf(process.platform === "win32")("quest land", () => { expect(plan.waiting?.map(({ number }) => number)).toEqual([2, 3]); }); + test("counts only a required check's newest run from its app", async () => { + const forge = new Forge(); + const test = (conclusion: string, app?: number) => ({ name: "test", conclusion, url: "u", app }); + forge.pull("Rerun passed", { "1.txt": "1\n" }, { pull: { runs: [test("CANCELLED"), test("SUCCESS")] } }); + forge.pull("Rerun failed", { "2.txt": "2\n" }, { pull: { runs: [test("SUCCESS"), test("CANCELLED")] } }); + forge.pull("Other app failed", { "3.txt": "3\n" }, { pull: { runs: [test("FAILURE", 99), test("SUCCESS")] } }); + const plan = await resolveQueue(REPOSITORY, "main", await forge.deps()); + expect(plan.queue.map((entry) => entry.number)).toEqual([1]); + expect(plan.eject.map((ejected) => [ejected.number, ejected.reason])).toEqual([ + [2, "failed_checks"], + [3, "failed_checks"], + ]); + }); + test("refuses ruleset rules it does not uphold before reading the queue", async () => { const forge = new Forge(); forge.pull("Ready", { "1.txt": "1\n" }); diff --git a/test/status.test.ts b/test/status.test.ts index b68a30b9..43ac154c 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -404,6 +404,73 @@ test("reports quest land's ejection after the auto-merge it disabled, ignoring o expect(status.landingStopped).toEqual({ reason: "merge_conflict", actor: "gate", at: "2026-10-07T12:02:00Z" }); }); +test("drops the landing exit once the PR enters the merge queue or auto-merge again", async () => { + const disabled = { + __typename: "AutoMergeDisabledEvent", + createdAt: "2026-10-07T12:01:00Z", + reasonCode: "manually_disabled", + actor: { login: "kixelated" }, + }; + const removed = { + __typename: "RemovedFromMergeQueueEvent", + createdAt: "2026-10-07T12:01:00Z", + reason: "MERGE_CONFLICT", + actor: { login: "github-merge-queue" }, + }; + const ejected = { + __typename: "IssueComment", + createdAt: "2026-10-07T12:01:00Z", + body: "quest land removed this pull request from the queue: it conflicts.\n\n", + authorAssociation: "NONE", + author: { __typename: "Bot", login: "gate" }, + }; + const autoMerge = (enabledAt: string) => ({ enabledAt, mergeMethod: "SQUASH", enabledBy: { login: "kixelated" } }); + const queue = (enqueuedAt: string) => ({ state: "QUEUED", position: 1, enqueuedAt }); + const stopped = async (node: Node, fields: Node) => { + const body = pull({ + checks: null, + reviews: page([]), + threads: page([]), + timelineItems: { nodes: [node] }, + ...fields, + }); + return (await pullRequestStatus(pr, setup([body]).deps)).landingStopped?.reason ?? null; + }; + // `quest merge --land` disables auto-merge and enables it again, often within the same second. + expect(await stopped(disabled, { autoMergeRequest: autoMerge("2026-10-07T12:01:00Z") })).toBeNull(); + expect(await stopped(ejected, { autoMergeRequest: autoMerge("2026-10-07T12:05:00Z") })).toBeNull(); + expect(await stopped(removed, { mergeQueueEntry: queue("2026-10-07T12:05:00Z") })).toBeNull(); + // An entry that began before the exit is not a new one. + expect(await stopped(removed, { autoMergeRequest: autoMerge("2026-10-07T12:00:00Z") })).toBe("merge_conflict"); + expect(await stopped(ejected, { mergeQueueEntry: queue("2026-10-07T12:00:59Z") })).toBe("merge_conflict"); +}); + +test("counts only each check's newest run from its app", async () => { + const rerun = (name: string, conclusion: string, databaseId: number, app = 15368) => ({ + ...run(name, "COMPLETED", conclusion, true), + databaseId, + checkSuite: { app: { databaseId: app } }, + }); + const fixture = setup([ + pull({ + checks: page([ + rerun("build", "SUCCESS", 7), + rerun("build", "CANCELLED", 3), + rerun("test", "SUCCESS", 4), + rerun("test", "FAILURE", 8), + rerun("lint", "SUCCESS", 5), + rerun("lint", "FAILURE", 2, 99), + ]), + reviews: page([]), + threads: page([]), + }), + ]); + const status = await pullRequestStatus(pr, fixture.deps); + expect(status.checks.passed).toBe(2); + expect(status.checks.failing.map((check) => check.name)).toEqual(["test", "lint"]); + expect(status.checks.pending).toEqual([]); +}); + test("long lists page only the connections that have more, keeping the first page's PR fields", async () => { const fixture = setup([ pull({ diff --git a/test/wait.test.ts b/test/wait.test.ts index 64cc9c4a..16799dc3 100644 --- a/test/wait.test.ts +++ b/test/wait.test.ts @@ -86,6 +86,8 @@ interface CheckFixture { /** The workflow file a job runs in, and whether its run is still going, so it may yet create jobs. */ workflow?: string; running?: boolean; + /** A check run's ID, which grows with each run; its place in the page by default. */ + id?: number; } /** The head's checks; `null` means the head has no checks at all. */ @@ -96,7 +98,7 @@ function checks( listed = head, extra: Record = {}, ) { - const node = (entry: CheckFixture) => + const node = (entry: CheckFixture, index: number) => entry.status ? { __typename: "StatusContext", @@ -108,6 +110,7 @@ function checks( } : { __typename: "CheckRun", + databaseId: entry.id ?? index + 1, name: entry.name, status: entry.result ? "COMPLETED" : "IN_PROGRESS", conclusion: entry.result ?? null, @@ -1007,6 +1010,26 @@ test("a gated base's quest land check, which no head carries, never holds back c expect(lines[0]).toMatch(/^2026-10-07T12:05:00Z checks:passed check /); }); +test("a rerun supersedes a cancelled run of a required check, which never wakes the wait as failed", async () => { + const fixture = setup([pull(), response([pushed()]), response([committed(A)]), response([committed(A)])], [A], { + checks: [ + checks([ + { name: "check", required: true, result: "CANCELLED", time: LATER }, + { name: "check", required: true }, + ]), + checks([ + { name: "check", required: true, result: "SUCCESS", time: LATEST, id: 2 }, + { name: "check", required: true, result: "CANCELLED", time: LATER, id: 1 }, + ]), + ], + protected: ["check"], + }); + 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 check /); +}); + 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)])], From 3741b977de7e1305763fa2729da026aea5efbb70 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 19:12:32 -0800 Subject: [PATCH 3/4] fix(status): ask for the check run ID and app it deduplicates by Co-Authored-By: Claude Opus 5.5 --- src/cli/status.ts | 5 ++++- test/status.test.ts | 8 +++++++- test/wait.test.ts | 6 ++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/cli/status.ts b/src/cli/status.ts index 170d5c33..79c6784e 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -39,7 +39,10 @@ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, pageInfo { hasNextPage endCursor } nodes { __typename - ... on CheckRun { name status conclusion detailsUrl isRequired(pullRequestNumber: $number) } + ... on CheckRun { + databaseId name status conclusion detailsUrl isRequired(pullRequestNumber: $number) + checkSuite { app { databaseId } } + } ... on StatusContext { context state targetUrl isRequired(pullRequestNumber: $number) } } } diff --git a/test/status.test.ts b/test/status.test.ts index 43ac154c..baab6fbc 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -115,6 +115,7 @@ function setup( } = {}, ) { const queries: Record[] = []; + const texts: string[] = []; const holdQueries: Record[] = []; const eventRequests: string[] = []; let last: ReturnType | undefined; @@ -162,13 +163,14 @@ function setup( }); } queries.push(request.variables); + texts.push(request.query); const body = bodies.shift(); if (body === undefined) throw new Error("unexpected request"); last = body as ReturnType; return Response.json(body); }, }; - return { deps, queries, holdQueries, eventRequests, activityRequests }; + return { deps, queries, texts, holdQueries, eventRequests, activityRequests }; } test("reports checks by requiredness, each reviewer's latest review against the head, and open threads", async () => { @@ -466,6 +468,10 @@ test("counts only each check's newest run from its app", async () => { }), ]); const status = await pullRequestStatus(pr, fixture.deps); + // The fixture answers whatever is asked, so the query must ask for what tells runs apart. + expect(fixture.texts[0].replace(/\s+/g, " ")).toContain( + "... on CheckRun { databaseId name status conclusion detailsUrl isRequired(pullRequestNumber: $number) checkSuite { app { databaseId } } }", + ); expect(status.checks.passed).toBe(2); expect(status.checks.failing.map((check) => check.name)).toEqual(["test", "lint"]); expect(status.checks.pending).toEqual([]); diff --git a/test/wait.test.ts b/test/wait.test.ts index 16799dc3..df5409b4 100644 --- a/test/wait.test.ts +++ b/test/wait.test.ts @@ -1028,6 +1028,9 @@ test("a rerun supersedes a cancelled run of a required check, which never wakes expect(fixture.sleeps).toEqual([300_000]); expect(lines).toHaveLength(1); expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:passed check /); + const query = (fixture.checkCalls[0].body as { query: string }).query.replace(/\s+/g, " "); + expect(query).toContain("... on CheckRun { databaseId name"); + expect(query).toContain("checkSuite { status app { databaseId }"); }); test("optional checks never stand in for required checks that have not started", async () => { @@ -1145,6 +1148,9 @@ test("checks:passed is dated when the head goes idle, after required checks fini expect(fixture.sleeps).toEqual([300_000]); expect(lines).toHaveLength(1); expect(lines[0]).toMatch(/^2026-10-07T12:08:00Z checks:passed check /); + const query = (fixture.checkCalls[0].body as { query: string }).query.replace(/\s+/g, " "); + expect(query).toContain("... on CheckRun { databaseId name"); + expect(query).toContain("checkSuite { status app { databaseId }"); }); test("with no required checks, an unfinished workflow run holds back checks:passed", async () => { From 9f9e217e987618189151fc58976a63e7543bc060 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 3 Nov 2026 07:02:36 -0800 Subject: [PATCH 4/4] fix(checks): judge every check by its latest run, as GitHub does Complete quest/m0/latest-check-runs.md, which main split from this PR's quest in moq-dev/quest#422, and delete it. - Move the latest-run filter to src/github/checks.ts, shared by the CLI and the Worker, and key commit statuses by context and newest createdAt. - quest status derives its checks state from the latest runs instead of the rollup state, which counts superseded runs. - The Worker's job context lists only the latest runs, with a state derived from them. - Regression tests shaped after moq-dev/moq#7325 for status, wait, the land gate, and the Worker. Co-Authored-By: Claude Opus 5.5 --- README.md | 13 ++-- cloudflare/README.md | 3 +- cloudflare/src/github/service.ts | 16 +++-- cloudflare/test/publish.test.ts | 30 +++++++++ quest/m0/README.md | 1 - quest/m0/latest-check-runs.md | 104 ------------------------------- src/cli/land.ts | 12 ++-- src/cli/main.ts | 4 +- src/cli/status.ts | 47 +++----------- src/cli/wait.ts | 5 +- src/github/checks.ts | 59 ++++++++++++++++++ test/checks.test.ts | 77 +++++++++++++++++++++++ test/land.test.ts | 12 +++- test/status.test.ts | 24 +++++++ test/wait.test.ts | 22 +++++++ 15 files changed, 262 insertions(+), 167 deletions(-) delete mode 100644 quest/m0/latest-check-runs.md create mode 100644 src/github/checks.ts create mode 100644 test/checks.test.ts diff --git a/README.md b/README.md index 544e14d5..ce047080 100644 --- a/README.md +++ b/README.md @@ -361,7 +361,8 @@ present (except `quest land`'s, which no head carries), and every required 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 +`quest status` counts them, so neutral and skipped results pass and a rerun +supersedes an earlier run of the same check. Each job of a workflow a ruleset requires is required. Requiredness comes from the base branch's protection and rulesets, readable with read access. A failed optional check prints separately as `check:failed`, @@ -438,7 +439,7 @@ comes last, and a missing value prints as `-`: | `auto-merge` | Method, who enabled it, when, and the head it was enabled on. | | `merge-queue` | Queue state, position, and when it was enqueued. | | `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; `-` once the PR has entered the merge queue or auto-merge again since. | -| `checks` | The head's rollup state and `passed=`, `failing=`, `pending=` counts. | +| `checks` | `failure` if any check fails, else `pending` if any is pending, else `success`, and `passed=`, `failing=`, `pending=` counts, all from each check's latest run; `-` when the head has no checks. | | `check` | `failing` or `pending`, `required` or `optional`, the result, details URL, and name. | | `review` | `current` or `stale` against the head, state, reviewer, time, and URL of each reviewer's latest review. | | `reacted:` | Actor, time, and the PR or review reacted to, as `quest wait` names reactions. | @@ -448,8 +449,10 @@ comes last, and a missing value prints as `-`: The node ID is what GraphQL's `addPullRequestReviewThreadReply` and `resolveReviewThread` take. Neutral and skipped checks pass. Only each check's -newest run counts: a rerun, or the same workflow run again on the head, -supersedes an earlier run of that name from the same app. A required check +latest run counts, as for GitHub's mergeability: a rerun, or the same workflow +run again on the head, supersedes an earlier run of that name from the same +app, and a newer commit status supersedes an older one of its context. GitHub's +own rollup state counts the superseded runs too. A required check is one branch protection or a ruleset requires for this PR. `--json` prints the same facts as one object with the fields `url`, `state`, `draft`, `head`, `mergeable`, `mergeState`, `reviewDecision`, `autoMerge`, `mergeQueue`, @@ -653,7 +656,7 @@ quest links with `quest resolve`. A failed batch halves the next batch's size, down to one PR, and each batch that lands grows it back by one; the order and size persist in the state directory. Conflicting additions stay queued. A PR whose required checks are still running stays queued -and sits out batches until they pass; only each check's newest run counts, as in +and sits out batches until they pass; only each check's latest run counts, as in `quest status`. If any are still pending two hours after its pick (twice the default `--timeout`), the daemon comments once on that pick, naming them, since a required check that never reports would hold it silently. The sandbox checks the tip once, without diff --git a/cloudflare/README.md b/cloudflare/README.md index c05e060c..e68e79e9 100644 --- a/cloudflare/README.md +++ b/cloudflare/README.md @@ -169,7 +169,8 @@ Changes a workflow may not publish pause the job with a `publication` decision. The current attempt of a job on an issue or pull request reads it at `GET /api/runner/context?job=…&attempt=…`, as the App with the requester's current authority: an issue's text and comments, or a pull request's text, -comments, reviews, unresolved review threads, and checks at its head. A new +comments, reviews, unresolved review threads, and checks at its head, each by +its latest run as `quest status` judges them. A new plan's attempt reads its `idea` and `context` there instead. [GITHUB.md](GITHUB.md#publication) describes the request, its validation, and how interrupted publications are reconciled. diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 0cdefb16..3cc15c83 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -1,5 +1,6 @@ import { createHash, createPrivateKey, sign } from "node:crypto"; import { HOLD_PREFIX, isHoldLabel, latestHold, heldToken, check, isQuest, parse, ready } from "quest/core"; +import { checksState, latestContexts } from "../../../src/github/checks"; import { cleanAutoMergeRefusal, immediateMerge } from "../../../src/github/merge"; import { osiApproved } from "../licenses"; import { unreviewed } from "./review"; @@ -1222,12 +1223,12 @@ export class GitHubService { const reviews = `reviews(first: 50, after: $after) { nodes { author { login } state body submittedAt } ${pageInfo} }`; const threads = `reviewThreads(first: 100, after: $after) { nodes { id isResolved isOutdated path line comments(first: 50) { ${commentFields} ${pageInfo} } } ${pageInfo} }`; const contexts = (after: boolean) => - `contexts(first: 100${after ? ", after: $after" : ""}) { nodes { __typename ... on CheckRun { name status conclusion detailsUrl } ... on StatusContext { context state description targetUrl } } ${pageInfo} }`; + `contexts(first: 100${after ? ", after: $after" : ""}) { nodes { __typename ... on CheckRun { databaseId name status conclusion detailsUrl checkSuite { app { databaseId } } } ... on StatusContext { context state description createdAt targetUrl } } ${pageInfo} }`; const query = kind === "issues" ? `issue(number: $number) { number title state body author { login } ${comments} }` : `pullRequest(number: $number) { number title state body isDraft author { login } baseRefName headRefName headRefOid ${comments} ${reviews} ${threads} - commits(last: 1) { nodes { commit { oid statusCheckRollup { state ${contexts(false)} } } } } }`; + commits(last: 1) { nodes { commit { oid statusCheckRollup { ${contexts(false)} } } } } }`; type Connection = { nodes: T[]; pageInfo: { hasNextPage: boolean; endCursor: string | null } }; type Comment = { author: { login: string } | null; body: string; createdAt?: string; submittedAt?: string }; type Review = Comment & { state: string }; @@ -1241,6 +1242,8 @@ export class GitHubService { }; type Check = { __typename: string; + databaseId?: number; + checkSuite?: { app: { databaseId: number } | null } | null; name?: string; status?: string; conclusion?: string | null; @@ -1248,9 +1251,10 @@ export class GitHubService { context?: string; state?: string; description?: string | null; + createdAt?: string; targetUrl?: string | null; }; - type Rollup = { state: string; contexts: Connection }; + type Rollup = { contexts: Connection }; type Item = { number: number; title: string; @@ -1376,6 +1380,8 @@ export class GitHubService { "check contexts", ) : []; + // Only each check's latest run counts, as for mergeability; the rollup's own state counts them all. + const checks = latestContexts(allChecks); return { kind: "pull" as const, ...common, @@ -1387,8 +1393,8 @@ export class GitHubService { threads: unresolved, checks: { commit: head?.oid ?? null, - state: head?.statusCheckRollup?.state ?? null, - runs: allChecks.map((c) => + state: head?.statusCheckRollup ? checksState(checks) : null, + runs: checks.map((c) => c.__typename === "CheckRun" ? { name: c.name!, state: c.conclusion ?? c.status ?? null, url: c.detailsUrl ?? null } : { diff --git a/cloudflare/test/publish.test.ts b/cloudflare/test/publish.test.ts index d10e628a..84a9a465 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -1849,6 +1849,36 @@ describe("job context", () => { ); }); + it("lists each check's latest run, as on moq-dev/moq#7325", async () => { + // Concurrency cancelled the older workflow run, whose gate failed on purpose; the newer run + // passed. GitHub's rollup state counted both and read FAILURE, yet GitHub merges the head. + const pull = project.pulls[0]!; + pull.runs = [ + { name: "Scope", conclusion: "CANCELLED", url: "https://ci.test/1" }, + { name: "Release gate", conclusion: "FAILURE", url: "https://ci.test/1" }, + { name: "Scope", conclusion: "SUCCESS", url: "https://ci.test/2" }, + { name: "Release gate", conclusion: "SUCCESS", url: "https://ci.test/2" }, + { name: "Release gate", conclusion: "FAILURE", url: "https://other.test", app: 99 }, + ]; + const id = await running({ + workflow: "iterate", + target: "pulls/2", + revision: prHead, + branch: "contributor/fix", + trusted: false, + }); + const body = (await (await read(id, 1)).json()) as Record; + expect(body.checks.state).toBe("FAILURE"); + expect(body.checks.runs).toEqual([ + { name: "Scope", state: "SUCCESS", url: "https://ci.test/2" }, + { name: "Release gate", state: "SUCCESS", url: "https://ci.test/2" }, + { name: "Release gate", state: "FAILURE", url: "https://other.test" }, + ]); + pull.runs.pop(); + const passing = (await (await read(id, 1)).json()) as Record; + expect(passing.checks.state).toBe("SUCCESS"); + }); + it.each(["comments", "reviews", "reviewThreads", "thread comments", "checks"])( "refuses missing or cycling cursors in %s", async (connection) => { diff --git a/quest/m0/README.md b/quest/m0/README.md index a2928528..06a3328c 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -85,7 +85,6 @@ ownership-manifest design: - [Land through the merge queue with an async fixer](/quest/m0/queue-watcher/README.md) - GitHub's merge queue lands one PR at a time, and a host watcher fixes and re-enqueues each PR it drops - [Merge through a host gate](/quest/m0/host-land/README.md) - the host's fixer containers, and the frozen batched gate for the repositories not yet switched - [The land fixer resolves every mechanical failure](/quest/m0/land-fixer/README.md) - lockfile merges, protected edits as ready commits, workflow pushes, and the PR's own CI as context -- [Checks are judged by their latest run](/quest/m0/latest-check-runs.md) - `quest status`, `quest wait`, and the gate count each check's latest run on the head, as GitHub does, so a superseded cancelled or failed run stops failing them or refusing release-plz PRs GitHub would merge - [Agents don't stall](/quest/m0/agent-stalls.md) - long waits block in the same turn, pre-existing failures don't stop review, and reviewer outages become waits - [Follow-ups as agents report](/quest/m0/follow-ups-as-reports.md) - coordinators plan follow-ups as reports arrive, fewer of them, with standing milestone rules - [Context before decision prompts](/quest/m0/prompt-context.md) - every prompt explains the work first, and maintainer steps say why an agent can't take them diff --git a/quest/m0/latest-check-runs.md b/quest/m0/latest-check-runs.md deleted file mode 100644 index d5aeeaee..00000000 --- a/quest/m0/latest-check-runs.md +++ /dev/null @@ -1,104 +0,0 @@ -# [S] Checks are judged by their latest run on the head - -## Goal - -`quest status`, `quest wait`, and the `quest land` gate judge each check on -a PR's head by its latest run, matching the merge state GitHub reported on -[moq-dev/moq#7325](https://github.com/moq-dev/moq/pull/7325). An -older run that a newer run of the same check replaced no longer prints, -counts, wakes a wait, or fails the gate, whether it was cancelled, failed, -or passed. `quest status`'s `checks` state follows those latest runs, not the -rollup's own `state`, which counts every run. The Worker's job context for a -PR reports checks the same way. - -## Plan - -Decided 2026-11-02 (maintainer): plan it in m1, then, the same day, move -it to m0. Since [moq-dev/quest#229](https://github.com/moq-dev/quest/pull/229) -the `quest land` gate fails a check name if any run with that name failed, so -it can wrongly refuse release-plz PRs that GitHub would merge. That is land -work, which m0 ranks first. Raised by -[moq-dev/moq#7325](https://github.com/moq-dev/moq/pull/7325), release-plz's -release PR. Each refresh pushes and edits the description at once, starting -two `Release gate` runs, and concurrency cancels one. The cancelled run's -gate fails on purpose; the other run on the same head passes, and GitHub's -merge state counted the pass (CLEAN). `quest status` still printed -`check failing required failure … Release gate`, which misleads merge -decisions. Split from the landing-status quest, -which owned a one-line version of this; the two halves complete separately. - -Evidence on #7325's head `a15652d`, read 2026-11-02: - -- The rollup's `contexts` list both `Release gate` check runs, from the same - app (GitHub Actions), both `isRequired`. Check suite `112631417489` - (run [41564598600](https://github.com/moq-dev/moq/actions/runs/41564598600)) - has the failure, started 02:12:02Z, and a cancelled `Scope`. Check suite - `112631424074` (run [41564601164](https://github.com/moq-dev/moq/actions/runs/41564601164)) - has the success, started 02:34:46Z. -- The rollup's own `state` is `FAILURE`, and its - `checkRunCountsByState` counts the cancelled and failed runs. -- REST `commits/{sha}/check-runs?filter=latest` returns both runs too. It - keeps only the latest run per check suite, so it collapses a job's re-runs, - not separate workflow runs. -- GitHub does not document which run counts. "About protected branches" only - says duplicate job names across workflows give ambiguous results. The - observed rule is that the newer run of the name, from the app, counts. -- `gh pr checks` keeps the run with the latest `startedAt` per check name, - workflow name, and event (`eliminateDuplicates` in cli/cli's - `pkg/cmd/pr/checks/aggregate.go`). - -Where checks are judged today: - -- `summarize` in `src/cli/status.ts` classifies every context with - `classifyCheck` and prints `rollup.state` as the `checks` state. -- `quest wait` (`check` and `checkActivities` in `src/cli/wait.ts`) reads - every context, so a superseded failure can wake it with `checks:failed`. -- The gate's `checks` in `src/cli/land.ts` fails a name when any of its - reports fails: "A name reported twice, as by a rerun, counts only once - every report passes", from - [moq-dev/quest#229](https://github.com/moq-dev/quest/pull/229). Reverse - it. GitHub merges that head, so the stricter rule rejects PRs GitHub - accepts, such as each release PR that hits this race. -- `readTarget` in `cloudflare/src/github/service.ts` gives a job's agent - every run and the rollup's `state`. - -Decisions made while planning, for review in the PR: - -- One helper both builds import keeps the latest contexts. Put it in - `src/github/` (the Worker already imports `src/github/merge.ts`), not in - `src/cli/status.ts`. Status, wait, land, and the Worker filter through it - before classifying. -- Key a check run by its app (`checkSuite.app.databaseId`) and name. That is - the identity a required check has in branch protection and rulesets, and - the one `quest land` already matches a requirement on. Key a commit status - by its context. A check run and a status with the same name stay separate, - since GitHub requires both. Rejected: `gh pr checks`' key of name, - workflow name, and event. It can keep two runs that GitHub's requirement - counts once, so the list would disagree with mergeability. -- The latest check run is the one created last, the highest `databaseId`. - A newer run that is still queued has no `startedAt`, but it already makes - the check pending. The latest status is the one with the newest - `createdAt`; on a tie, the one listed later in the rollup. -- Every producer fetches the fields the helper needs, since none has them - all today: `CheckRun.databaseId` and `checkSuite { app { databaseId } }` - on check runs, and `createdAt` on statuses. `status.ts` has none of them, - `wait.ts` lacks the check run's id and app, `land.ts` has only the app, - and the Worker's `readTarget` query has none. -- The `checks` state is derived from the latest contexts: `failure` if any - fails, else `pending` if any is pending, else `success`. With no rollup it - stays `-`. Keep the line's format; update the README's `checks` row, which - says it is the rollup state. -- Only the latest runs count toward `passed=`, and only they print as - `check failing` or `check pending` lines. - -Tests: - -- A fixture recorded from #7325: a suite with a cancelled `Scope` and a - failed `Release gate`, and a newer suite where both passed. `quest status` - prints `checks success` with no failing line. `quest wait` never reports - `checks:failed` for it and reports `checks:passed` once the head is idle. - The gate counts the head passing. The Worker's job context lists the newer - runs only. -- A newer queued run of a passed check makes it pending. A newer failure - after a pass fails. The same name from two apps counts twice, and a check - run and a status that share a name both count. diff --git a/src/cli/land.ts b/src/cli/land.ts index e21bf6f8..26ed13c4 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -16,7 +16,8 @@ import { requirements, string, } from "./github"; -import { appOf, classifyCheck, newestChecks } from "./status"; +import { appOf, latestContexts } from "../github/checks"; +import { classifyCheck } from "./status"; import { committedSources, snapshot } from "./tree"; /** The check a gated base requires. Nothing reports it on a PR head, so GitHub's auto-merge only waits. */ @@ -133,7 +134,7 @@ const CONTEXTS = `pageInfo { hasNextPage endCursor } nodes { __typename ... on CheckRun { databaseId name status conclusion detailsUrl checkSuite { app { databaseId } } } - ... on StatusContext { context state targetUrl } + ... on StatusContext { context state createdAt targetUrl } }`; const QUEUE = `query($owner: String!, $name: String!, $base: String!, $qualified: String!, $after: String) { @@ -438,16 +439,17 @@ async function contexts(github: GitHub, repository: Repository, node: ObjectValu /** * The required checks a head failed, and those pending or not yet reported, each from its app. Only - * each check's newest run counts, so a rerun supersedes a cancelled or failed run. + * each check's latest run counts, as for GitHub's mergeability, so a rerun supersedes a cancelled or + * failed run. */ function checks(contexts: ObjectValue[], required: RequiredCheck[]): { failing: string[]; pending: string[] } { const failing: string[] = []; const pending: string[] = []; - const newest = newestChecks(contexts); + const latest = latestContexts(contexts); for (const need of required) { let result: "passed" | "failing" | "pending" = "pending"; let reported = false; - for (const context of newest) { + for (const context of latest) { const { check, outcome } = classifyCheck(context); if (check.name !== need.name || (need.app !== null && appOf(context) !== need.app)) continue; // A name reported by more than one app, or as both a run and a status, counts only once every report passes. diff --git a/src/cli/main.ts b/src/cli/main.ts index b090a9fc..e45cef62 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -450,13 +450,13 @@ export function main(argv: string[], out: Output): number | Promise { "in one call. It reports facts; it does not decide whether the PR is ready or whose review counts.\n\n" + "Plain output prints one record per line, its type first and any free text (a check name or path) last: " + "url, state, draft, head, mergeable, merge-state, review-decision, auto-merge, merge-queue, " + - "landing-stopped, and checks; auto-merge gives its method, who enabled it and when, and the head it was " + + "landing-stopped, and checks, whose state and counts follow each check's latest run; auto-merge gives its method, who enabled it and when, and the head it was " + "enabled on, which differs from head once a push follows, as GitHub keeps auto-merge across a writer's " + "push (- when it cannot be told: a quest land pin names it, and otherwise the head branch's push " + "activity, which GitHub lists up to minutes late); 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, until the PR enters the merge queue or auto-merge again. Then " + - "a check line per failing or pending check, marked required or optional, counting only each check's newest run; a review line per " + + "a check line per failing or pending check, marked required or optional, counting only each check's latest run; a review line per " + "reviewer's latest review, marked current or stale against the head; a reacted: line per " + "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 79c6784e..dbe06176 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -11,6 +11,7 @@ import { type PullRequest, string, } from "./github"; +import { checksState, contextOutcome, latestContexts } from "../github/checks"; import { EJECTED, pinned } from "./land"; import { readHoldStatus, type HoldFact, type HoldReport } from "./hold"; @@ -34,7 +35,7 @@ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, ... on IssueComment { createdAt body authorAssociation author { __typename login } } } } reactions(last: 100) { nodes { ...reaction } } - commits(last: 1) { nodes { commit { oid statusCheckRollup { state + commits(last: 1) { nodes { commit { oid statusCheckRollup { __typename contexts(first: 100, after: $checks) @include(if: $moreChecks) { pageInfo { hasNextPage endCursor } nodes { @@ -43,7 +44,7 @@ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, databaseId name status conclusion detailsUrl isRequired(pullRequestNumber: $number) checkSuite { app { databaseId } } } - ... on StatusContext { context state targetUrl isRequired(pullRequestNumber: $number) } + ... on StatusContext { context state createdAt targetUrl isRequired(pullRequestNumber: $number) } } } } } } } @@ -63,9 +64,6 @@ const STATUS = `query($owner: String!, $repo: String!, $number: Int!, } fragment reaction on Reaction { content createdAt user { login } }`; -/** Completed check-run conclusions that do not fail; anything else completed fails. */ -const PASSING = ["success", "neutral", "skipped"]; - export interface Check { name: string; required: boolean; @@ -265,13 +263,14 @@ function summarize( ): Status { const head = string(pull.headRefOid); const url = string(pull.url); + // The rollup's own state counts runs a newer run of the same check replaced. const checks = { - state: rollup === null ? null : lower(rollup.state), + state: rollup === null ? null : lower(checksState(contexts)), passed: 0, failing: [], pending: [], } as Status["checks"]; - for (const context of newestChecks(contexts)) { + for (const context of latestContexts(contexts)) { const { check, outcome } = classifyCheck(context); if (outcome === "passed") checks.passed++; else checks[outcome].push(check); @@ -383,33 +382,6 @@ function ejection(node: ObjectValue): string | null { return trusted && typeof node.body === "string" ? (EJECTED.exec(node.body)?.[1] ?? null) : null; } -/** - * Each check's newest report, in rollup order. A rerun, or a workflow run again on the same head, - * reports a check run again under the same name from the same app, and only the newest counts, as it - * does for GitHub's required checks; check run IDs increase as runs are created. A commit status is - * already its context's newest. Check runs need `databaseId` and `checkSuite { app { databaseId } }`. - */ -export function newestChecks(contexts: ObjectValue[]): ObjectValue[] { - const newest = new Map(); - for (const context of contexts) { - if (context.__typename !== "CheckRun") continue; - const key = JSON.stringify([appOf(context), context.name]); - const seen = newest.get(key); - if (seen === undefined || Number(context.databaseId) > Number(seen.databaseId)) newest.set(key, context); - } - return contexts.filter( - (context) => - context.__typename !== "CheckRun" || newest.get(JSON.stringify([appOf(context), context.name])) === context, - ); -} - -/** The GitHub App behind a check run; a commit status has none. */ -export function appOf(context: ObjectValue): number | null { - if (context.__typename !== "CheckRun" || context.checkSuite == null) return null; - const app = object(context.checkSuite).app; - return app == null ? null : Number(object(app).databaseId); -} - /** * A check run or commit status from a `statusCheckRollup` context, and whether it passed, failed, or * is still pending. `quest wait` shares this, so both commands agree on what passes. @@ -425,12 +397,7 @@ export function classifyCheck(context: ObjectValue): { check: Check; outcome: "p : lower(run ? context.conclusion : context.state), url: optional(run ? context.detailsUrl : context.targetUrl), }; - if (run ? context.status !== "COMPLETED" : ["pending", "expected"].includes(check.result)) - return { check, outcome: "pending" }; - return { - check, - outcome: (run ? PASSING.includes(check.result) : check.result === "success") ? "passed" : "failing", - }; + return { check, outcome: contextOutcome(context) }; } /** Stable plain lines: a record type, then space-separated fields, with any free text last. */ diff --git a/src/cli/wait.ts b/src/cli/wait.ts index f4a08145..c1de8ec5 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -15,7 +15,8 @@ import { string, } from "./github"; import { LAND_CHECK } from "./land"; -import { classifyCheck, enabledHead, newestChecks } from "./status"; +import { latestContexts } from "../github/checks"; +import { classifyCheck, enabledHead } from "./status"; const POLL = 5 * 60_000; /** @@ -362,7 +363,7 @@ async function watch( const page = object(contexts.pageInfo); after = page.hasNextPage === true ? string(page.endCursor) : null; } while (after !== null); - return { head: head!, base: string(pull.baseRefName), checks: newestChecks(rows).map(check), pull }; + return { head: head!, base: string(pull.baseRefName), checks: latestContexts(rows).map(check), pull }; }; /** * What the base branch requires, read once per wait. Only a requirement says whether a missing diff --git a/src/github/checks.ts b/src/github/checks.ts new file mode 100644 index 00000000..735136c6 --- /dev/null +++ b/src/github/checks.ts @@ -0,0 +1,59 @@ +// How a head's `statusCheckRollup` contexts are judged, shared by the CLI and the Worker. +// GitHub lists every check run on a head, including those a newer run of the same check replaced, +// and its rollup `state` counts them all; mergeability counts only each check's latest run. + +/** A `statusCheckRollup` context: a `CheckRun` or a `StatusContext`, as GraphQL returns it. */ +export type RollupContext = { [field: string]: unknown }; + +/** Completed check-run conclusions that do not fail; anything else completed fails. */ +const PASSING = ["SUCCESS", "NEUTRAL", "SKIPPED"]; + +/** The GitHub App behind a check run; a commit status has none. */ +export function appOf(context: RollupContext): number | null { + if (context.__typename !== "CheckRun") return null; + const app = (context.checkSuite as { app?: { databaseId?: unknown } | null } | null | undefined)?.app; + return app?.databaseId == null ? null : Number(app.databaseId); +} + +/** + * Each check's latest report on the head, in rollup order. A check run is keyed by its app and name, + * as a required check is, and the latest is the one created last: the highest `databaseId`. A commit + * status is keyed by its context, and the latest is the newest `createdAt`, or on a tie the one listed + * later. A check run and a status with the same name stay apart, since GitHub requires both. + * Queries must ask for a check run's `databaseId name checkSuite { app { databaseId } }` and a + * status's `context createdAt`. + */ +export function latestContexts(contexts: T[]): T[] { + const key = (context: T) => + context.__typename === "CheckRun" + ? JSON.stringify(["run", appOf(context), context.name]) + : JSON.stringify(["status", context.context]); + const newer = (context: T, seen: T) => + context.__typename === "CheckRun" + ? Number(context.databaseId) > Number(seen.databaseId) + : String(context.createdAt ?? "") >= String(seen.createdAt ?? ""); + const latest = new Map(); + for (const context of contexts) { + const seen = latest.get(key(context)); + if (seen === undefined || newer(context, seen)) latest.set(key(context), context); + } + return contexts.filter((context) => latest.get(key(context)) === context); +} + +/** Whether a context passed, failed, or is still pending. Neutral and skipped check runs pass. */ +export function contextOutcome(context: RollupContext): "passed" | "failing" | "pending" { + if (context.__typename === "CheckRun") { + if (context.status !== "COMPLETED") return "pending"; + return PASSING.includes(String(context.conclusion).toUpperCase()) ? "passed" : "failing"; + } + const state = String(context.state).toUpperCase(); + if (state === "PENDING" || state === "EXPECTED") return "pending"; + return state === "SUCCESS" ? "passed" : "failing"; +} + +/** The head's check state from its latest contexts: any failing fails, else any pending is pending. */ +export function checksState(contexts: RollupContext[]): "SUCCESS" | "FAILURE" | "PENDING" { + const outcomes = latestContexts(contexts).map(contextOutcome); + if (outcomes.includes("failing")) return "FAILURE"; + return outcomes.includes("pending") ? "PENDING" : "SUCCESS"; +} diff --git a/test/checks.test.ts b/test/checks.test.ts new file mode 100644 index 00000000..cbf00fd2 --- /dev/null +++ b/test/checks.test.ts @@ -0,0 +1,77 @@ +import { expect, test } from "vitest"; + +import { checksState, contextOutcome, latestContexts } from "../src/github/checks"; + +const ACTIONS = 15368; +const run = (name: string, databaseId: number, conclusion: string | null, app = ACTIONS) => ({ + __typename: "CheckRun", + databaseId, + name, + status: conclusion === null ? "QUEUED" : "COMPLETED", + conclusion, + checkSuite: { app: { databaseId: app } }, +}); +const status = (context: string, state: string, createdAt: string) => ({ + __typename: "StatusContext", + context, + state, + createdAt, +}); + +// The shape of moq-dev/moq#7325's head a15652d: concurrency cancelled the older workflow run, whose +// gate then failed on purpose, while the newer run passed. GitHub's rollup state was FAILURE, yet its +// merge state was CLEAN. +const moq7325 = [ + run("Scope", 1, "CANCELLED"), + run("Release gate", 2, "FAILURE"), + run("Scope", 3, "SUCCESS"), + run("Release gate", 4, "SUCCESS"), +]; + +test("only the latest run of each check counts, as on moq-dev/moq#7325", () => { + expect(latestContexts(moq7325)).toEqual([moq7325[2], moq7325[3]]); + expect(checksState(moq7325)).toBe("SUCCESS"); +}); + +test("a newer run decides the check whatever order the rollup lists it in", () => { + expect(checksState([run("test", 9, null), run("test", 3, "SUCCESS")])).toBe("PENDING"); + expect(checksState([run("test", 3, "SUCCESS"), run("test", 9, "FAILURE")])).toBe("FAILURE"); + expect(checksState([run("test", 9, "SUCCESS"), run("test", 3, "FAILURE")])).toBe("SUCCESS"); +}); + +test("the same name from two apps, or as a run and a status, counts as separate checks", () => { + const twoApps = [run("test", 1, "FAILURE", 99), run("test", 2, "SUCCESS")]; + expect(latestContexts(twoApps)).toEqual(twoApps); + expect(checksState(twoApps)).toBe("FAILURE"); + const runAndStatus = [run("deploy", 1, "SUCCESS"), status("deploy", "FAILURE", "2026-10-07T12:00:00Z")]; + expect(latestContexts(runAndStatus)).toEqual(runAndStatus); + expect(checksState(runAndStatus)).toBe("FAILURE"); +}); + +test("a commit status counts by its newest report, and on a tie the one listed later", () => { + const newest = status("ci", "SUCCESS", "2026-10-07T12:05:00Z"); + expect(latestContexts([newest, status("ci", "FAILURE", "2026-10-07T12:00:00Z")])).toEqual([newest]); + const tied = status("ci", "PENDING", "2026-10-07T12:05:00Z"); + expect(latestContexts([newest, tied])).toEqual([tied]); +}); + +test("outcomes follow GitHub's passing results", () => { + expect(["SUCCESS", "NEUTRAL", "SKIPPED"].map((c) => contextOutcome(run("x", 1, c)))).toEqual([ + "passed", + "passed", + "passed", + ]); + expect(["CANCELLED", "TIMED_OUT", "ACTION_REQUIRED"].map((c) => contextOutcome(run("x", 1, c)))).toEqual([ + "failing", + "failing", + "failing", + ]); + expect(contextOutcome(run("x", 1, null))).toBe("pending"); + expect(["PENDING", "EXPECTED", "SUCCESS", "ERROR"].map((s) => contextOutcome(status("x", s, "")))).toEqual([ + "pending", + "pending", + "passed", + "failing", + ]); + expect(checksState([])).toBe("SUCCESS"); +}); diff --git a/test/land.test.ts b/test/land.test.ts index 1166243b..62a7fa22 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1031,14 +1031,22 @@ describe.skipIf(process.platform === "win32")("quest land", () => { expect(plan.waiting?.map(({ number }) => number)).toEqual([2, 3]); }); - test("counts only a required check's newest run from its app", async () => { + test("counts only a required check's latest run from its app", async () => { const forge = new Forge(); const test = (conclusion: string, app?: number) => ({ name: "test", conclusion, url: "u", app }); forge.pull("Rerun passed", { "1.txt": "1\n" }, { pull: { runs: [test("CANCELLED"), test("SUCCESS")] } }); forge.pull("Rerun failed", { "2.txt": "2\n" }, { pull: { runs: [test("SUCCESS"), test("CANCELLED")] } }); forge.pull("Other app failed", { "3.txt": "3\n" }, { pull: { runs: [test("FAILURE", 99), test("SUCCESS")] } }); + // As on moq-dev/moq#7325: concurrency cancelled the older workflow run, whose gate failed on + // purpose, and the newer run passed. GitHub merges that head. + const scope = (conclusion: string) => ({ name: "Scope", conclusion, url: "u" }); + forge.pull( + "Superseded run", + { "4.txt": "4\n" }, + { pull: { runs: [scope("CANCELLED"), test("FAILURE"), scope("SUCCESS"), test("SUCCESS")] } }, + ); const plan = await resolveQueue(REPOSITORY, "main", await forge.deps()); - expect(plan.queue.map((entry) => entry.number)).toEqual([1]); + expect(plan.queue.map((entry) => entry.number)).toEqual([1, 4]); expect(plan.eject.map((ejected) => [ejected.number, ejected.reason])).toEqual([ [2, "failed_checks"], [3, "failed_checks"], diff --git a/test/status.test.ts b/test/status.test.ts index baab6fbc..7bce1b5c 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -477,6 +477,30 @@ test("counts only each check's newest run from its app", async () => { expect(status.checks.pending).toEqual([]); }); +test("judges checks by their latest runs, not the rollup's state, as on moq-dev/moq#7325", async () => { + // A cancelled run's gate failed on purpose; the newer run of the same workflow passed, and GitHub's + // merge state counted the pass while its rollup state stayed FAILURE. + const latest = (name: string, conclusion: string, databaseId: number) => ({ + ...run(name, "COMPLETED", conclusion, name === "Release gate"), + databaseId, + checkSuite: { app: { databaseId: 15368 } }, + }); + const body = pull({ + checks: page([ + latest("Scope", "CANCELLED", 1), + latest("Release gate", "FAILURE", 2), + latest("Scope", "SUCCESS", 3), + latest("Release gate", "SUCCESS", 4), + ]), + reviews: page([]), + threads: page([]), + }); + const fixture = setup([body]); + const status = await pullRequestStatus(pr, fixture.deps); + expect(statusLines(status)).toContain("checks success passed=2 failing=0 pending=0"); + expect(fixture.texts[0].replace(/\s+/g, " ")).toContain("... on StatusContext { context state createdAt targetUrl"); +}); + test("long lists page only the connections that have more, keeping the first page's PR fields", async () => { const fixture = setup([ pull({ diff --git a/test/wait.test.ts b/test/wait.test.ts index df5409b4..058e48f1 100644 --- a/test/wait.test.ts +++ b/test/wait.test.ts @@ -1033,6 +1033,28 @@ test("a rerun supersedes a cancelled run of a required check, which never wakes expect(query).toContain("checkSuite { status app { databaseId }"); }); +test("a superseded workflow run's failed gate never wakes the wait, as on moq-dev/moq#7325", async () => { + // Concurrency cancelled the older run, whose gate then failed on purpose; the newer run passed. + const older: CheckFixture[] = [ + { name: "Scope", result: "CANCELLED", time: LATER, id: 1 }, + { name: "Release gate", required: true, result: "FAILURE", time: LATER, id: 2 }, + ]; + const fixture = setup([pull(), response([pushed()]), response([committed(A)]), response([committed(A)])], [A], { + checks: [ + checks([...older, { name: "Scope", id: 3 }, { name: "Release gate", required: true, id: 4 }]), + checks([ + ...older, + { name: "Scope", result: "SUCCESS", time: LATEST, id: 3 }, + { name: "Release gate", required: true, result: "SUCCESS", time: LATEST, id: 4 }, + ]), + ], + protected: ["Release gate"], + }); + const lines = await waitForActivity(pr, {}, fixture.deps); + expect(fixture.sleeps).toEqual([300_000]); + expect(lines).toEqual([expect.stringMatching(/^2026-10-07T12:08:00Z checks:passed Release_gate /)]); +}); + 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)])],