diff --git a/README.md b/README.md index 7fb6af9a..f328c7c3 100644 --- a/README.md +++ b/README.md @@ -379,7 +379,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`, @@ -455,8 +456,8 @@ 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. | -| `checks` | The head's rollup state and `passed=`, `failing=`, `pending=` counts. | +| `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` | `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. | @@ -465,7 +466,11 @@ 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 +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`, @@ -672,7 +677,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 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 bisection: success lands one squash commit per PR. A failure, or a base that diff --git a/cloudflare/README.md b/cloudflare/README.md index 87f0ca2a..8cbcbde6 100644 --- a/cloudflare/README.md +++ b/cloudflare/README.md @@ -178,7 +178,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. After a delivery, the runner reviews the head the job published with the other vendor's model diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index 18373fac..2c788bbf 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; @@ -353,11 +353,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/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 904bd9e2..e1686d6c 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -11,6 +11,7 @@ import { planInit, ready, } from "quest/core"; +import { checksState, latestContexts } from "../../../src/github/checks"; import { cleanAutoMergeRefusal, immediateMerge, queueHead } from "../../../src/github/merge"; import { osiApproved } from "../licenses"; import { REVIEW_CHECK, unreviewed } from "./review"; @@ -1647,12 +1648,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 }; @@ -1666,6 +1667,8 @@ export class GitHubService { }; type Check = { __typename: string; + databaseId?: number; + checkSuite?: { app: { databaseId: number } | null } | null; name?: string; status?: string; conclusion?: string | null; @@ -1673,9 +1676,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; @@ -1801,6 +1805,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, @@ -1812,8 +1818,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 8a59885b..b128771e 100644 --- a/cloudflare/test/publish.test.ts +++ b/cloudflare/test/publish.test.ts @@ -2073,6 +2073,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 eed55c6c..2d42bb14 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -85,8 +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) - 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 -- [Current landing status](/quest/m0/status-landing.md) - `quest status` clears a stale `landing-stopped` reason once the PR is queued again - [Kix decision UI](/quest/m0/kix/README.md) - planning and implementation through prompts and summaries, run on the maintainer's own runners - [Codex cloud off](/quest/m0/codex-cloud-off.md) - automatic Codex cloud reviews stop once no open PR relies on them - [Bun EFAULT report](/quest/m0/bun-efault.md) - report concurrent `rmSync` `EFAULT` on Windows, and drop the workaround once Bun fixes it diff --git a/quest/m0/latest-check-runs.md b/quest/m0/latest-check-runs.md deleted file mode 100644 index 813f5794..00000000 --- a/quest/m0/latest-check-runs.md +++ /dev/null @@ -1,108 +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 [Current landing status](/quest/m0/status-landing.md), -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. - -## Related - -- [Current landing status](/quest/m0/status-landing.md) - the other half of the old quest: `quest status` clears a stale `landing-stopped` reason diff --git a/quest/m0/status-landing.md b/quest/m0/status-landing.md deleted file mode 100644 index a2189336..00000000 --- a/quest/m0/status-landing.md +++ /dev/null @@ -1,33 +0,0 @@ -# [XS] quest status reports landing as it is now - -## Goal - -`quest status` prints `landing-stopped -` once the PR has been queued again, -including through `quest merge --land`, instead of the reason the earlier -entry ended. - -## 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. Report no reason (`landing-stopped -`, as - `statusLines` already prints for none) when auto-merge's `enabledAt`, or a - merge-queue entry, is newer than that event. - -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 sends them after -PRs that are already queued. - -Its other half, counting only the newest run of each check, moved to -[Checks are judged by their latest run](/quest/m0/latest-check-runs.md) on -2026-11-02 for its fuller plan, which also covers `quest wait`, the gate, and -the Worker. It stays in m0 beside this quest. - -## Related - -- [Checks are judged by their latest run](/quest/m0/latest-check-runs.md) - superseded runs stop failing `quest status`, `quest wait`, and the gate diff --git a/src/cli/land.ts b/src/cli/land.ts index 6e090b47..d2872ec2 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -16,6 +16,7 @@ import { requirements, string, } from "./github"; +import { appOf, latestContexts } from "../github/checks"; import { classifyCheck } from "./status"; import { committedSources, snapshot } from "./tree"; @@ -132,8 +133,8 @@ export interface Waiting { const CONTEXTS = `pageInfo { hasNextPage endCursor } nodes { __typename - ... on CheckRun { name status conclusion detailsUrl checkSuite { app { databaseId } } } - ... on StatusContext { context state targetUrl } + ... on CheckRun { databaseId name status conclusion detailsUrl checkSuite { app { databaseId } } } + ... on StatusContext { context state createdAt targetUrl } }`; const QUEUE = `query($owner: String!, $name: String!, $base: String!, $qualified: String!, $after: String) { @@ -436,17 +437,22 @@ 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 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 latest = latestContexts(contexts); for (const need of required) { let result: "passed" | "failing" | "pending" = "pending"; let reported = false; - for (const context of contexts) { + 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 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 +463,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 3316e689..9704dd02 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -463,13 +463,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. 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 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 1eafac34..2b85bc54 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,13 +35,16 @@ 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 { __typename - ... on CheckRun { name status conclusion detailsUrl isRequired(pullRequestNumber: $number) } - ... on StatusContext { context state targetUrl isRequired(pullRequestNumber: $number) } + ... on CheckRun { + databaseId name status conclusion detailsUrl isRequired(pullRequestNumber: $number) + checkSuite { app { databaseId } } + } + ... on StatusContext { context state createdAt targetUrl isRequired(pullRequestNumber: $number) } } } } } } } @@ -60,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; @@ -112,7 +113,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. */ @@ -259,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 contexts) { + for (const context of latestContexts(contexts)) { const { check, outcome } = classifyCheck(context); if (outcome === "passed") checks.passed++; else checks[outcome].push(check); @@ -317,9 +322,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"], @@ -385,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 775726ac..75438b5e 100644 --- a/src/cli/wait.ts +++ b/src/cli/wait.ts @@ -15,6 +15,7 @@ import { string, } from "./github"; import { LAND_CHECK } from "./land"; +import { latestContexts } from "../github/checks"; import { BRANCH_UPDATES, classifyCheck, enabledHead } from "./status"; const POLL = 5 * 60_000; @@ -63,8 +64,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) } } @@ -357,7 +358,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; @@ -370,11 +371,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: 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 e2b48c79..a9f3b79b 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1032,6 +1032,28 @@ describe.skipIf(process.platform === "win32")("quest land", () => { expect(plan.waiting?.map(({ number }) => number)).toEqual([2, 3]); }); + 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, 4]); + 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..7bce1b5c 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 () => { @@ -404,6 +406,101 @@ 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); + // 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([]); +}); + +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 79ff266f..f3233bb6 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, @@ -1054,6 +1057,51 @@ 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 /); + 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("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)])], @@ -1169,6 +1217,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 () => {