diff --git a/apps/server/src/orchestration-v2/Orchestrator.ts b/apps/server/src/orchestration-v2/Orchestrator.ts index eab5cda62027e..94ba1bbff25d5 100644 --- a/apps/server/src/orchestration-v2/Orchestrator.ts +++ b/apps/server/src/orchestration-v2/Orchestrator.ts @@ -4275,6 +4275,9 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio passedChecks: existing.endedWatch.passedChecks, conflicting: existing.endedWatch.conflicting, }), + ...(existing.endedWatch?.failedRuns !== undefined && { + failedRuns: existing.endedWatch.failedRuns, + }), ...(existing.endedWatch?.owedChecks !== undefined && { owedChecks: existing.endedWatch.owedChecks, }), diff --git a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts index f37a69c08ee0b..ee2af87bfbd55 100644 --- a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts @@ -175,6 +175,7 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc left.startedAt === right.startedAt && left.headSha === right.headSha && left.failedChecks.join("\n") === right.failedChecks.join("\n") && + left.failedRuns?.join("\n") === right.failedRuns?.join("\n") && left.passed === right.passed && left.passedChecks.join("\n") === right.passedChecks.join("\n") && (left.owedChecks ?? []).join("\n") === (right.owedChecks ?? []).join("\n") && diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index dece3e3449831..0b3939800497c 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -236,6 +236,44 @@ describe("evaluatePullRequestWatch", () => { ]); }); + it("reports a rerun that fails again between two reads", () => { + const run = (job: number): PullRequestCheck => ({ + ...check("lint", "failure"), + url: `https://ci.example/lint/job/${job}`, + required: true, + }); + const first = evaluatePullRequestWatch(watch(), detail({ checks: [run(1)] }), noRemarks); + assert.deepEqual(first.changes, [{ kind: "checks-failed", failed: [run(1)] }]); + const again = evaluatePullRequestWatch(first.next, detail({ checks: [run(2)] }), noRemarks); + assert.deepEqual(again.changes, [{ kind: "checks-failed", failed: [run(2)] }]); + // A status can link a build shared with another check; it is that check's run, not lint's. + const shared = { ...check("test", "failure"), url: "https://ci.example/lint/job/3" }; + const both = evaluatePullRequestWatch( + again.next, + detail({ checks: [run(2), shared] }), + noRemarks, + ); + assert.deepEqual( + evaluatePullRequestWatch(both.next, detail({ checks: [run(3), shared] }), noRemarks).changes, + [{ kind: "checks-failed", failed: [run(3)] }], + ); + // Neither the same run nor one told earlier on this head is news again. + for (const job of [2, 1]) { + assert.deepEqual( + evaluatePullRequestWatch(again.next, detail({ checks: [run(job)] }), noRemarks).changes, + [], + ); + } + }); + + it("does not repeat a failure an older watch told by name", () => { + const older = watch({ headSha: "aaaaaaaaaa", failedChecks: ["lint"] }); + const failed = detail({ checks: [check("lint", "failure")] }); + const report = evaluatePullRequestWatch(older, failed, noRemarks); + assert.deepEqual(report.changes, []); + assert.deepEqual(report.next.failedRuns, [JSON.stringify(["lint", "https://ci.example/lint"])]); + }); + it("reports passed once the required checks pass, whatever the others do", () => { const required = (name: string, status: PullRequestCheck["status"]) => ({ ...check(name, status), diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index 046c48e1858bc..e5f87081fe376 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -51,6 +51,7 @@ export function evaluatePullRequestWatch( // Hosts can return no checks on read failure; preserve prior state. let failedChecks = headMoved ? [] : watch.failedChecks; + let failedRuns = headMoved ? [] : watch.failedRuns; let passed = headMoved ? false : watch.passed; let passedChecks = headMoved ? [] : watch.passedChecks; let owedChecks = headMoved ? [] : (watch.owedChecks ?? []); @@ -62,13 +63,27 @@ export function evaluatePullRequestWatch( const failed = checks.filter(isFailedCheck); const awaiting = checks.filter((check) => check.status === "action-required"); if (checks.length > 0) { - const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name)); + // A new run URL under a failing name is a rerun that failed again between reads. + const toldRuns = failedRuns; + const runOf = (check: PullRequestCheck) => + check.url === null ? null : JSON.stringify([check.name, check.url]); + const newlyFailed = failed.filter((check) => { + const run = runOf(check); + return ( + !failedChecks.includes(check.name) || + (toldRuns !== undefined && run !== null && !toldRuns.includes(run)) + ); + }); if (newlyFailed.length > 0) changes.push({ kind: "checks-failed", failed: newlyFailed }); if (!approvalAnnounced && awaiting.length > 0) { changes.push({ kind: "checks-awaiting-approval", checks: awaiting }); approvalAnnounced = true; } failedChecks = failed.map((check) => check.name); + // Runs told stay told for the head: GitHub can show either event's failed run of one check. + failedRuns = [ + ...new Set([...(toldRuns ?? []), ...failed.flatMap((check) => runOf(check) ?? [])]), + ]; // Approval reads can omit gated runs. Keep each hold until its jobs finish. const awaitingNow = [...new Set(awaiting.map(approvalKey))]; @@ -195,6 +210,7 @@ export function evaluatePullRequestWatch( startedAt: watch.startedAt, headSha, failedChecks, + ...(failedRuns !== undefined && { failedRuns }), passed, passedChecks, owedChecks, diff --git a/packages/contracts/src/threadPullRequest.ts b/packages/contracts/src/threadPullRequest.ts index bd0a65a3c0bc9..70306fb30c8c6 100644 --- a/packages/contracts/src/threadPullRequest.ts +++ b/packages/contracts/src/threadPullRequest.ts @@ -83,6 +83,8 @@ export const ThreadPullRequestWatch = Schema.Struct({ startedAt: IsoDateTime, headSha: Schema.NullOr(TrimmedNonEmptyString), failedChecks: Schema.Array(TrimmedNonEmptyString), + /** Name and run URL of each failed run told on this head, so a rerun that fails again is news. */ + failedRuns: Schema.optional(Schema.Array(Schema.String)), passed: Schema.Boolean, passedChecks: Schema.Array(TrimmedNonEmptyString).pipe( Schema.withDecodingDefault(Effect.succeed([])), diff --git a/packages/source-control-core/src/server/pullRequestChecks.test.ts b/packages/source-control-core/src/server/pullRequestChecks.test.ts index 4f6538f833f9a..f083735804653 100644 --- a/packages/source-control-core/src/server/pullRequestChecks.test.ts +++ b/packages/source-control-core/src/server/pullRequestChecks.test.ts @@ -59,6 +59,21 @@ describe("dedupeChecks", () => { expect([forward[0]?.status, reversed[0]?.status]).toEqual(["failure", "failure"]); }); + it("reports the same run for two events' failures finished at once, whichever is listed first", () => { + const failed = (event: string) => ({ + ...entry("lint", "failure", { at: "2026-11-07T00:01:00Z" }), + check: { name: "lint", status: "failure" as const, description: null, url: event }, + event, + }); + + expect( + [ + [failed("push"), failed("pull_request")], + [failed("pull_request"), failed("push")], + ].map((runs) => dedupeChecks(runs)[0]?.url), + ).toEqual(["push", "push"]); + }); + it("takes the higher sequence when instants tie, whichever is listed first", () => { const final = entry("build", "success", { at: "2026-10-12T10:14:29Z", sequence: 15 }); const pending = entry("build", "pending", { at: "2026-10-12T10:14:29Z", sequence: 14 }); diff --git a/packages/source-control-core/src/server/pullRequestChecks.ts b/packages/source-control-core/src/server/pullRequestChecks.ts index 18b34d39baea5..c9c388f57280f 100644 --- a/packages/source-control-core/src/server/pullRequestChecks.ts +++ b/packages/source-control-core/src/server/pullRequestChecks.ts @@ -46,9 +46,18 @@ export function sortChecks( ); } +// Runs of one check for different events: a failure outranks a run in progress, which outranks a +// pass, which outranks a run cancelled beside it (a concurrency group superseding it). +function eventSeverity(status: PullRequestCheck["status"]): number { + if (status === "failure" || status === "action-required") return 3; + if (status === "pending") return 2; + return status === "success" ? 1 : 0; +} + /** * Rollups repeat checks during reruns or overlapping runs, and checks carry no ids. * Identify checks by workflow plus name because different workflows may share job names. + * Within one event the newest run wins; across events the worse run does. */ export function dedupeChecks( entries: ReadonlyArray<{ @@ -57,12 +66,15 @@ export function dedupeChecks( readonly at: string | null; // Hosts with one-second timestamps can date a final and an earlier status alike. readonly sequence?: number; + /** What triggered the run, where the host says (GitHub's `push` or `pull_request`). */ + readonly event?: string | null; }>, ): ReadonlyArray { - const newestByCheck = new Map(); + type Entry = (typeof entries)[number]; + const newestByRun = new Map(); for (const entry of entries) { - const key = JSON.stringify([entry.workflowName ?? "", entry.check.name]); - const kept = newestByCheck.get(key); + const key = JSON.stringify([entry.workflowName ?? "", entry.check.name, entry.event ?? null]); + const kept = newestByRun.get(key); if (kept !== undefined) { if (!isAtLeastAsNew(entry.at, kept.at)) continue; if ( @@ -73,7 +85,29 @@ export function dedupeChecks( ) continue; } - newestByCheck.set(key, entry); + newestByRun.set(key, entry); + } + const newestByCheck = new Map(); + for (const entry of newestByRun.values()) { + const key = JSON.stringify([entry.workflowName ?? "", entry.check.name]); + const kept = newestByCheck.get(key); + if (kept === undefined) { + newestByCheck.set(key, entry); + continue; + } + const severity = eventSeverity(entry.check.status) - eventSeverity(kept.check.status); + const newer = isAtLeastAsNew(entry.at, kept.at); + // Equal times break on the event, so a reordered rollup cannot flip the run a watch was told. + const tied = newer && isAtLeastAsNew(kept.at, entry.at); + const later = tied ? (entry.event ?? "") > (kept.event ?? "") : newer; + const worse = severity > 0 || (severity === 0 && later); + const chosen = worse ? entry : kept; + newestByCheck.set( + key, + entry.check.required === true || kept.check.required === true + ? { ...chosen, check: { ...chosen.check, required: true } } + : chosen, + ); } const survivors = [...newestByCheck.values()]; const countsByName = new Map(); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 8daf82277a083..9ccf57d7ed7ca 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -3524,13 +3524,17 @@ layer("GitHubPullRequestApi.layer", (it) => { name: "build", status: "COMPLETED", conclusion: "SUCCESS", - checkSuite: { workflowRun: { workflow: { name: "linux" } } }, + checkSuite: { + workflowRun: { event: "push", workflow: { name: "linux" } }, + }, }), checkRun({ name: "build", status: "COMPLETED", conclusion: "FAILURE", - checkSuite: { workflowRun: { workflow: { name: "windows" } } }, + checkSuite: { + workflowRun: { event: "push", workflow: { name: "windows" } }, + }, }), ], pageInfo: { hasNextPage: false, endCursor: null }, diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts index acccedcb40f38..ad5af344c4be2 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts @@ -361,7 +361,7 @@ describe("pull request detail decoding", () => { const inWorkflow = ({ workflowName, ...fields }: Record) => check({ ...fields, - checkSuite: { workflowRun: { workflow: { name: workflowName } } }, + checkSuite: { workflowRun: { event: "pull_request", workflow: { name: workflowName } } }, }); const coreJson = ( @@ -616,6 +616,76 @@ describe("pull request detail decoding", () => { } }); + it("keeps a failed push run failing while the pull_request run of it runs or passes", () => { + const run = (event: string, fields: Record) => + check({ + name: "lint", + checkSuite: { workflowRun: { event, workflow: { name: "CI" } } }, + ...fields, + }); + const pushFailed = run("push", { + status: "COMPLETED", + conclusion: "FAILURE", + startedAt: "2026-10-08T04:06:10Z", + completedAt: "2026-10-08T04:08:00Z", + }); + // Only the losing run says the check is required; the merged check must still say so. + const pullRequestRunning = run("pull_request", { + isRequired: true, + status: "IN_PROGRESS", + conclusion: null, + startedAt: "2026-10-08T04:06:13Z", + completedAt: "0001-01-01T00:00:00Z", + }); + const pullRequestPassed = run("pull_request", { + isRequired: true, + status: "COMPLETED", + conclusion: "SUCCESS", + startedAt: "2026-10-08T04:06:13Z", + completedAt: "2026-10-08T04:12:00Z", + }); + const checksOf = (rollup: ReadonlyArray>) => { + const detail = detailOf(rollup); + return [ + detail.checks.map((check) => [check.name, check.status, check.required]), + detail.checksState, + ]; + }; + + const rollups = [ + [pushFailed, pullRequestRunning], + [pullRequestRunning, pushFailed], + [pushFailed, pullRequestPassed], + [pullRequestPassed, pushFailed], + ]; + expect(rollups.map(checksOf)).toEqual( + rollups.map(() => [[["lint", "failure", true]], "failing"]), + ); + }); + + it("does not fail a check whose run of one event was cancelled beside a passing run", () => { + const run = (event: string, conclusion: string, completedAt: string) => + check({ + name: "lint", + conclusion, + completedAt, + checkSuite: { workflowRun: { event, workflow: { name: "CI" } } }, + }); + const passed = run("push", "SUCCESS", "2026-10-08T04:10:00Z"); + const cancelled = run("pull_request", "CANCELLED", "2026-10-08T04:12:00Z"); + + for (const rollup of [ + [passed, cancelled], + [cancelled, passed], + ]) { + const detail = detailOf(rollup); + expect([detail.checks.map((check) => check.status), detail.checksState]).toEqual([ + ["success"], + "passing", + ]); + } + }); + it("merges reviews with comments in time order and keeps a bodyless approval", () => { const detail = expectSuccess(activity(JSON.parse(detailJson) as Record)); expect(detail.comments.map((comment) => comment.id)).toEqual(["r1", "c1", "r2"]); diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index df4756fcc0dda..a412d04b055eb 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -138,6 +138,8 @@ interface CheckFields { readonly targetUrl?: string | null | undefined; /** Empty for app-owned runs; absent for commit statuses. */ readonly workflowName?: string | null | undefined; + /** Absent for commit statuses and `gh` JSON; null for app-owned runs. */ + readonly workflowRun?: { readonly event: string } | null | undefined; readonly startedAt?: string | null | undefined; readonly completedAt?: string | null | undefined; /** Branch protection requires this check; asked for on github.com only. */ @@ -608,11 +610,12 @@ function isNamelessCheck(raw: CheckFields): boolean { return trimmed(raw.name) === null && trimmed(raw.context) === null; } -/** Unfinished runs outrank finished runs of the same check across push/PR workflows. */ +/** Unfinished runs outrank finished runs of the same check, so a rerun replaces its run. */ function toCheckEntries(raw: ReadonlyArray | null | undefined): ReadonlyArray<{ readonly check: PullRequestCheck; readonly workflowName: string | null; readonly at: string | null; + readonly event: string | null; }> { return (raw ?? []).flatMap((check) => { const name = trimmed(check.name) ?? trimmed(check.context); @@ -628,6 +631,7 @@ function toCheckEntries(raw: ReadonlyArray | null | undefined): Rea ...(typeof check.isRequired === "boolean" ? { required: check.isRequired } : {}), }, workflowName: trimmed(check.workflowName), + event: trimmed(check.workflowRun?.event), at: status === "pending" ? STILL_RUNNING @@ -1199,7 +1203,11 @@ export type GitHubCheckContext = CheckFields; function toCheckContexts(nodes: ReadonlyArray): ReadonlyArray { return nodes.map((check) => check.__typename === "CheckRun" - ? { ...check, workflowName: check.checkSuite.workflowRun?.workflow.name ?? null } + ? { + ...check, + workflowName: check.checkSuite.workflowRun?.workflow.name ?? null, + workflowRun: check.checkSuite.workflowRun, + } : check, ); } diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql b/packages/source-control-github/src/server/pullRequestReads.graphql index ef40a4ab46bcd..e63653f892ca1 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql +++ b/packages/source-control-github/src/server/pullRequestReads.graphql @@ -42,6 +42,7 @@ fragment CheckContext on StatusCheckRollupContext { isRequired(pullRequestNumber: $number) @include(if: $required) checkSuite { workflowRun { + event workflow { name } diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql.ts b/packages/source-control-github/src/server/pullRequestReads.graphql.ts index 0a0f1aec68a3a..c2f384b7671c1 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestReads.graphql.ts @@ -30,6 +30,8 @@ export interface Workflow extends Schema.Struct.Type {} /** A workflow run. */ export const WorkflowRun = Schema.Struct({ + /** The event that triggered the workflow run */ + event: Schema.String, /** The workflow executed in this workflow run. */ workflow: Workflow }) @@ -506,7 +508,7 @@ export const PullRequestCore = GraphQL.query("PullRequestCore", { { name: "repository", body: ["(owner:$", "owner", " name:$", "name", "){mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed autoMergeAllowed viewerPermission pullRequest(number:$", "number", "){number title url body state isDraft mergeable mergeStateStatus reviewDecision additions deletions changedFiles createdAt updatedAt mergedAt closedAt headRefName baseRefName headRefOid isCrossRepository maintainerCanModify headRepositoryOwner{__typename login}headRepository{nameWithOwner viewerPermission}headRef{id}author{__typename login avatarUrl...on User{id name}}autoMergeRequest{mergeMethod}viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch baseRef{compare(headRef:$", "headRef", "){behindBy}}reviewRequests(first:$", "requestedReviewers", "){nodes{requestedReviewer{__typename...on User{login name}...on Bot{login}...on Team{slug combinedSlug teamName:name teamAvatarUrl:avatarUrl}}}}labels(first:100){nodes{name color}}commits(last:1){nodes{commit{statusCheckRollup{contexts(first:100){nodes{__typename...", "CheckContext", "}pageInfo{hasNextPage endCursor}}}}}}}}"] } ], fragments: [ - { name: "CheckContext", body: ["on StatusCheckRollupContext{__typename...on StatusContext{context state targetUrl createdAt description isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")}...on CheckRun{name status conclusion startedAt completedAt detailsUrl isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")checkSuite{workflowRun{workflow{name}}}}}"] } + { name: "CheckContext", body: ["on StatusCheckRollupContext{__typename...on StatusContext{context state targetUrl createdAt description isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")}...on CheckRun{name status conclusion startedAt completedAt detailsUrl isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")checkSuite{workflowRun{event workflow{name}}}}}"] } ] }, variables: { @@ -557,7 +559,7 @@ export const PullRequestCheckContexts = GraphQL.query("PullRequestCheckContexts" { name: "repository", body: ["(owner:$", "owner", " name:$", "name", "){pullRequest(number:$", "number", "){headRefOid commits(last:1){nodes{commit{statusCheckRollup{contexts(first:100 after:$", "after", "){nodes{__typename...", "CheckContext", "}pageInfo{hasNextPage endCursor}}}}}}}}"] } ], fragments: [ - { name: "CheckContext", body: ["on StatusCheckRollupContext{__typename...on StatusContext{context state targetUrl createdAt description isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")}...on CheckRun{name status conclusion startedAt completedAt detailsUrl isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")checkSuite{workflowRun{workflow{name}}}}}"] } + { name: "CheckContext", body: ["on StatusCheckRollupContext{__typename...on StatusContext{context state targetUrl createdAt description isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")}...on CheckRun{name status conclusion startedAt completedAt detailsUrl isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")checkSuite{workflowRun{event workflow{name}}}}}"] } ] }, variables: {